diff --git a/pkg/parser/schema_additional_properties_test.go b/pkg/parser/schema_additional_properties_test.go index d928202e36..c6c1f0dd87 100644 --- a/pkg/parser/schema_additional_properties_test.go +++ b/pkg/parser/schema_additional_properties_test.go @@ -251,7 +251,7 @@ func TestValidProperties_NotRejected(t *testing.T) { "tools": map[string]any{ "github": nil, }, - "timeout_minutes": 10, + "timeout-minutes": 10, "runs-on": "ubuntu-latest", "safe-outputs": map[string]any{ "create-issue": nil, diff --git a/pkg/parser/schema_deprecated_test.go b/pkg/parser/schema_deprecated_test.go index 59c06c0012..e442144522 100644 --- a/pkg/parser/schema_deprecated_test.go +++ b/pkg/parser/schema_deprecated_test.go @@ -12,25 +12,18 @@ func TestGetMainWorkflowDeprecatedFields(t *testing.T) { t.Fatalf("GetMainWorkflowDeprecatedFields() error = %v", err) } - // Check that timeout_minutes is in the list + // Check that timeout_minutes is NOT in the list (it was removed from schema completely) + // Users should use the timeout-minutes-migration codemod to migrate their workflows found := false for _, field := range deprecatedFields { if field.Name == "timeout_minutes" { found = true - // Check that it has a replacement suggestion - if field.Replacement == "" { - t.Errorf("Expected timeout_minutes to have a replacement suggestion, got empty string") - } - // Check that replacement is timeout-minutes - if field.Replacement != "timeout-minutes" { - t.Errorf("Expected replacement 'timeout-minutes', got '%s'", field.Replacement) - } break } } - if !found { - t.Error("Expected timeout_minutes to be in the deprecated fields list") + if found { + t.Error("timeout_minutes should NOT be in the deprecated fields list (removed from schema)") } } diff --git a/pkg/parser/schema_test.go b/pkg/parser/schema_test.go index e41e6c213e..45e981980e 100644 --- a/pkg/parser/schema_test.go +++ b/pkg/parser/schema_test.go @@ -27,7 +27,7 @@ func TestValidateMainWorkflowFrontmatterWithSchema(t *testing.T) { "permissions": "read-all", "run-name": "Test Run", "runs-on": "ubuntu-latest", - "timeout_minutes": 30, + "timeout-minutes": 30, "concurrency": "test", "env": map[string]string{"TEST": "value"}, "if": "true", @@ -175,14 +175,6 @@ func TestValidateMainWorkflowFrontmatterWithSchema(t *testing.T) { wantErr: true, errContains: "additional properties", }, - { - name: "invalid type for timeout_minutes", - frontmatter: map[string]any{ - "timeout_minutes": "not-a-number", - }, - wantErr: true, - errContains: "got string, want integer", - }, { name: "valid frontmatter with complex on object", frontmatter: map[string]any{ @@ -856,7 +848,7 @@ func TestValidateMainWorkflowFrontmatterWithSchema(t *testing.T) { name: "missing required on field with other valid fields", frontmatter: map[string]any{ "engine": "copilot", - "timeout_minutes": 30, + "timeout-minutes": 30, "permissions": map[string]any{ "issues": "write", }, diff --git a/pkg/parser/schemas/main_workflow_schema.json b/pkg/parser/schemas/main_workflow_schema.json index e938b30a00..32df848cca 100644 --- a/pkg/parser/schemas/main_workflow_schema.json +++ b/pkg/parser/schemas/main_workflow_schema.json @@ -1791,12 +1791,6 @@ "description": "Workflow timeout in minutes (GitHub Actions standard field). Defaults to 20 minutes for agentic workflows. Has sensible defaults and can typically be omitted.", "examples": [5, 10, 30] }, - "timeout_minutes": { - "type": "integer", - "description": "Deprecated: Use 'timeout-minutes' instead. Workflow timeout in minutes. Defaults to 20 minutes for agentic workflows.", - "examples": [5, 10, 30], - "deprecated": true - }, "concurrency": { "description": "Concurrency control to limit concurrent workflow runs (GitHub Actions standard field). Supports two forms: simple string for basic group isolation, or object with cancel-in-progress option for advanced control. Agentic workflows enhance this with automatic per-engine concurrency policies (defaults to single job per engine across all workflows) and token-based rate limiting. Default behavior: workflows in the same group queue sequentially unless cancel-in-progress is true. See https://docs.github.com/en/actions/using-jobs/using-concurrency", "oneOf": [ diff --git a/pkg/workflow/claude_engine.go b/pkg/workflow/claude_engine.go index 8765cd3e6d..26166f5d53 100644 --- a/pkg/workflow/claude_engine.go +++ b/pkg/workflow/claude_engine.go @@ -538,9 +538,8 @@ mkdir -p "$HOME/.cache" // Add timeout at step level (GitHub Actions standard) if workflowData.TimeoutMinutes != "" { - // Strip both possible prefixes (timeout_minutes or timeout-minutes) - timeoutValue := strings.TrimPrefix(workflowData.TimeoutMinutes, "timeout_minutes: ") - timeoutValue = strings.TrimPrefix(timeoutValue, "timeout-minutes: ") + // Strip timeout-minutes prefix + timeoutValue := strings.TrimPrefix(workflowData.TimeoutMinutes, "timeout-minutes: ") stepLines = append(stepLines, fmt.Sprintf(" timeout-minutes: %s", timeoutValue)) } else { stepLines = append(stepLines, fmt.Sprintf(" timeout-minutes: %d", int(constants.DefaultAgenticWorkflowTimeout/time.Minute))) // Default timeout for agentic workflows diff --git a/pkg/workflow/compiler_orchestrator_workflow.go b/pkg/workflow/compiler_orchestrator_workflow.go index a6aaf91960..4a4c248113 100644 --- a/pkg/workflow/compiler_orchestrator_workflow.go +++ b/pkg/workflow/compiler_orchestrator_workflow.go @@ -2,10 +2,8 @@ package workflow import ( "fmt" - "os" "strings" - "github.com/githubnext/gh-aw/pkg/console" "github.com/githubnext/gh-aw/pkg/logger" "github.com/githubnext/gh-aw/pkg/parser" "github.com/goccy/go-yaml" @@ -145,15 +143,8 @@ func (c *Compiler) extractYAMLSections(frontmatter map[string]any, workflowData workflowData.Features = c.extractFeatures(frontmatter) workflowData.If = c.extractIfCondition(frontmatter) - // Prefer timeout-minutes (new) over timeout_minutes (deprecated) + // Extract timeout-minutes (canonical form) workflowData.TimeoutMinutes = c.extractTopLevelYAMLSection(frontmatter, "timeout-minutes") - if workflowData.TimeoutMinutes == "" { - workflowData.TimeoutMinutes = c.extractTopLevelYAMLSection(frontmatter, "timeout_minutes") - if workflowData.TimeoutMinutes != "" { - // Emit deprecation warning - fmt.Fprintln(os.Stderr, console.FormatWarningMessage("Field 'timeout_minutes' is deprecated. Please use 'timeout-minutes' instead to follow GitHub Actions naming convention.")) - } - } workflowData.RunsOn = c.extractTopLevelYAMLSection(frontmatter, "runs-on") workflowData.Environment = c.extractTopLevelYAMLSection(frontmatter, "environment") diff --git a/pkg/workflow/compiler_timeout_default_test.go b/pkg/workflow/compiler_timeout_default_test.go index a85dc66e49..9e2f7431a8 100644 --- a/pkg/workflow/compiler_timeout_default_test.go +++ b/pkg/workflow/compiler_timeout_default_test.go @@ -46,19 +46,6 @@ engine: copilot expectedTimeout: 30, description: "When timeout-minutes is explicitly specified, that value should be used", }, - { - name: "deprecated timeout_minutes specified - should use that value", - frontmatter: `--- -on: workflow_dispatch -permissions: - contents: read -timeout_minutes: 45 -engine: copilot -strict: false ----`, - expectedTimeout: 45, - description: "When deprecated timeout_minutes is specified, that value should be used", - }, } for _, tt := range tests { @@ -167,10 +154,10 @@ func TestSchemaDocumentationMatchesConstant(t *testing.T) { expectedText, schemaPath, int(constants.DefaultAgenticWorkflowTimeout/time.Minute)) } - // Count occurrences - should appear at least twice (timeout-minutes and timeout_minutes deprecated) + // Count occurrences - should appear exactly once (only timeout-minutes, timeout_minutes removed) occurrences := strings.Count(string(schemaContent), expectedText) - if occurrences < 2 { - t.Errorf("Expected to find at least 2 occurrences of %q in schema (for timeout-minutes and timeout_minutes fields), but found %d", + if occurrences != 1 { + t.Errorf("Expected to find exactly 1 occurrence of %q in schema (for timeout-minutes field only), but found %d", expectedText, occurrences) } } diff --git a/pkg/workflow/copilot_engine_execution.go b/pkg/workflow/copilot_engine_execution.go index e3eca6b275..9ddc30643f 100644 --- a/pkg/workflow/copilot_engine_execution.go +++ b/pkg/workflow/copilot_engine_execution.go @@ -547,9 +547,8 @@ COPILOT_CLI_INSTRUCTION="$(cat /tmp/gh-aw/aw-prompts/prompt.txt)" // Add timeout at step level (GitHub Actions standard) if workflowData.TimeoutMinutes != "" { - // Strip both possible prefixes (timeout_minutes or timeout-minutes) - timeoutValue := strings.TrimPrefix(workflowData.TimeoutMinutes, "timeout_minutes: ") - timeoutValue = strings.TrimPrefix(timeoutValue, "timeout-minutes: ") + // Strip timeout-minutes prefix + timeoutValue := strings.TrimPrefix(workflowData.TimeoutMinutes, "timeout-minutes: ") stepLines = append(stepLines, fmt.Sprintf(" timeout-minutes: %s", timeoutValue)) } else { stepLines = append(stepLines, fmt.Sprintf(" timeout-minutes: %d", int(constants.DefaultAgenticWorkflowTimeout/time.Minute))) // Default timeout for agentic workflows diff --git a/pkg/workflow/jobs.go b/pkg/workflow/jobs.go index e6d82ed541..e762b03646 100644 --- a/pkg/workflow/jobs.go +++ b/pkg/workflow/jobs.go @@ -311,7 +311,7 @@ func (jm *JobManager) renderJob(job *Job) string { fmt.Fprintf(&yaml, " %s\n", job.Concurrency) } - // Add timeout_minutes if specified + // Add timeout-minutes if specified if job.TimeoutMinutes > 0 { fmt.Fprintf(&yaml, " timeout-minutes: %d\n", job.TimeoutMinutes) } diff --git a/pkg/workflow/timeout_minutes_test.go b/pkg/workflow/timeout_minutes_test.go deleted file mode 100644 index 6c08769ed9..0000000000 --- a/pkg/workflow/timeout_minutes_test.go +++ /dev/null @@ -1,71 +0,0 @@ -//go:build !integration - -package workflow - -import ( - "strings" - "testing" -) - -func TestTimeoutMinutesDeprecation(t *testing.T) { - tests := []struct { - name string - frontmatter map[string]any - expectedTimeout string - expectDeprecation bool - }{ - { - name: "timeout-minutes (new format)", - frontmatter: map[string]any{ - "timeout-minutes": 15, - }, - expectedTimeout: "timeout-minutes: 15", - expectDeprecation: false, - }, - { - name: "timeout_minutes (deprecated format)", - frontmatter: map[string]any{ - "timeout_minutes": 20, - }, - expectedTimeout: "timeout_minutes: 20", - expectDeprecation: true, - }, - { - name: "timeout-minutes takes precedence", - frontmatter: map[string]any{ - "timeout-minutes": 15, - "timeout_minutes": 20, - }, - expectedTimeout: "timeout-minutes: 15", - expectDeprecation: false, - }, - { - name: "no timeout specified", - frontmatter: map[string]any{}, - expectedTimeout: "", - expectDeprecation: false, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - c := &Compiler{} - - // Test timeout-minutes extraction - timeoutHyphen := c.extractTopLevelYAMLSection(tt.frontmatter, "timeout-minutes") - timeoutUnderscore := c.extractTopLevelYAMLSection(tt.frontmatter, "timeout_minutes") - - // Verify the extraction logic matches our expected behavior - var actualTimeout string - if timeoutHyphen != "" { - actualTimeout = "timeout-minutes: " + strings.TrimPrefix(timeoutHyphen, "timeout-minutes: ") - } else if timeoutUnderscore != "" { - actualTimeout = "timeout_minutes: " + strings.TrimPrefix(timeoutUnderscore, "timeout_minutes: ") - } - - if actualTimeout != tt.expectedTimeout { - t.Errorf("Expected timeout %q, got %q", tt.expectedTimeout, actualTimeout) - } - }) - } -} diff --git a/pkg/workflow/tools.go b/pkg/workflow/tools.go index 7c7ac662eb..f4c3a6c819 100644 --- a/pkg/workflow/tools.go +++ b/pkg/workflow/tools.go @@ -143,7 +143,7 @@ func (c *Compiler) applyDefaults(data *WorkflowData, markdownPath string) error } if data.TimeoutMinutes == "" { - data.TimeoutMinutes = fmt.Sprintf("timeout_minutes: %d", int(constants.DefaultAgenticWorkflowTimeout/time.Minute)) + data.TimeoutMinutes = fmt.Sprintf("timeout-minutes: %d", int(constants.DefaultAgenticWorkflowTimeout/time.Minute)) } if data.RunsOn == "" { diff --git a/scripts/generate-schema-docs.test.js b/scripts/generate-schema-docs.test.js index e02d3fd994..564af4128b 100755 --- a/scripts/generate-schema-docs.test.js +++ b/scripts/generate-schema-docs.test.js @@ -124,16 +124,16 @@ for (const ref of allRefs) { } } -// Test 9: Verify that deprecated fields are excluded from output +// Test 9: Verify that deprecated fields are excluded from output and schema allPassed &= assertNotContains(output, "timeout_minutes:", "Deprecated field timeout_minutes should NOT be in output"); allPassed &= assertContains(output, "timeout-minutes:", "Non-deprecated field timeout-minutes should be in output"); -// Verify the schema actually has the deprecated field (sanity check) -if (schema.properties && schema.properties.timeout_minutes && schema.properties.timeout_minutes.deprecated === true) { - console.log("✓ PASS: Schema has timeout_minutes marked as deprecated"); +// Verify the schema does NOT have the deprecated field (it was removed completely) +if (schema.properties && !schema.properties.timeout_minutes) { + console.log("✓ PASS: Schema does not have timeout_minutes field (removed completely)"); } else { - console.error("❌ FAIL: Schema should have timeout_minutes marked as deprecated"); + console.error("❌ FAIL: Schema should NOT have timeout_minutes field"); allPassed = false; }