diff --git a/pkg/cli/codemod_engine_steps.go b/pkg/cli/codemod_engine_steps.go new file mode 100644 index 0000000000..d1588cca98 --- /dev/null +++ b/pkg/cli/codemod_engine_steps.go @@ -0,0 +1,269 @@ +package cli + +import ( + "strings" + + "github.com/github/gh-aw/pkg/logger" +) + +var engineStepsCodemodLog = logger.New("cli:codemod_engine_steps") + +// getEngineStepsToTopLevelCodemod creates a codemod for moving engine.steps to the top-level steps field +func getEngineStepsToTopLevelCodemod() Codemod { + return Codemod{ + ID: "engine-steps-to-top-level", + Name: "Move engine.steps to top-level steps", + Description: "Moves the 'steps' field from under 'engine' to the top-level 'steps' field, as 'engine.steps' is no longer supported", + IntroducedIn: "0.11.0", + Apply: func(content string, frontmatter map[string]any) (string, bool, error) { + // Check if engine.steps exists in frontmatter + engineValue, hasEngine := frontmatter["engine"] + if !hasEngine { + return content, false, nil + } + + engineMap, isMap := engineValue.(map[string]any) + if !isMap { + // engine is a string, no steps to move + return content, false, nil + } + + if _, hasSteps := engineMap["steps"]; !hasSteps { + return content, false, nil + } + + // Parse frontmatter lines + frontmatterLines, markdown, err := parseFrontmatterLines(content) + if err != nil { + return content, false, err + } + + // Find engine block and the steps field within it + engineIndent := "" + stepsStartIdx := -1 + inEngineBlock := false + + for i, line := range frontmatterLines { + trimmed := strings.TrimSpace(line) + + if isTopLevelKey(line) && strings.HasPrefix(trimmed, "engine:") { + engineIndent = getIndentation(line) + inEngineBlock = true + engineStepsCodemodLog.Printf("Found 'engine:' block at line %d", i+1) + continue + } + + // Check if we've exited the engine block + if inEngineBlock && len(trimmed) > 0 && !strings.HasPrefix(trimmed, "#") { + lineIndent := getIndentation(line) + if len(lineIndent) <= len(engineIndent) { + inEngineBlock = false + } + } + + // Look for steps: within engine block + if inEngineBlock && stepsStartIdx == -1 && strings.HasPrefix(trimmed, "steps:") { + stepsStartIdx = i + engineStepsCodemodLog.Printf("Found 'engine.steps' at line %d", i+1) + } + } + + if stepsStartIdx == -1 { + return content, false, nil + } + + // Find end of the steps block within engine + stepsIndent := getIndentation(frontmatterLines[stepsStartIdx]) + stepsEndIdx := stepsStartIdx + for j := stepsStartIdx + 1; j < len(frontmatterLines); j++ { + line := frontmatterLines[j] + trimmed := strings.TrimSpace(line) + + if len(trimmed) == 0 { + continue + } + + lineIndent := getIndentation(line) + if len(lineIndent) > len(stepsIndent) { + stepsEndIdx = j + } else { + break + } + } + + engineStepsCodemodLog.Printf("'engine.steps' spans lines %d to %d", stepsStartIdx+1, stepsEndIdx+1) + + // Extract the steps lines and un-indent them (remove the engine-level indentation) + topLevelStepsLines := make([]string, 0, stepsEndIdx-stepsStartIdx+1) + for i := stepsStartIdx; i <= stepsEndIdx; i++ { + line := frontmatterLines[i] + trimmed := strings.TrimSpace(line) + if trimmed == "" { + topLevelStepsLines = append(topLevelStepsLines, "") + continue + } + // Strip the stepsIndent prefix to un-indent to top level + if strings.HasPrefix(line, stepsIndent) { + topLevelStepsLines = append(topLevelStepsLines, line[len(stepsIndent):]) + } else { + topLevelStepsLines = append(topLevelStepsLines, trimmed) + } + } + + // Find existing top-level steps block (if any) + // Only treat as existing steps if it's actually a sequence + topLevelStepsEndIdx := -1 + hasTopLevelSteps := false + if stepsVal, exists := frontmatter["steps"]; exists { + if _, isSlice := stepsVal.([]any); isSlice { + hasTopLevelSteps = true + engineStepsCodemodLog.Print("Found existing top-level 'steps'") + } else { + engineStepsCodemodLog.Print("Top-level 'steps' exists but is not a sequence; treating as absent") + } + } + + if hasTopLevelSteps { + // Find the end of the top-level steps block in the lines + for i, line := range frontmatterLines { + trimmed := strings.TrimSpace(line) + if isTopLevelKey(line) && strings.HasPrefix(trimmed, "steps:") { + topStepsIndent := getIndentation(line) + topLevelStepsEndIdx = i + for j := i + 1; j < len(frontmatterLines); j++ { + l := frontmatterLines[j] + t := strings.TrimSpace(l) + if len(t) == 0 { + continue + } + if len(getIndentation(l)) > len(topStepsIndent) { + topLevelStepsEndIdx = j + } else { + break + } + } + engineStepsCodemodLog.Printf("Top-level 'steps:' ends at line %d", topLevelStepsEndIdx+1) + break + } + } + } + + // Build new frontmatter: remove engine.steps lines and insert at top level + // Pass 1: build lines without engine.steps + withoutEngineSteps := make([]string, 0, len(frontmatterLines)) + for i, line := range frontmatterLines { + if i >= stepsStartIdx && i <= stepsEndIdx { + continue + } + withoutEngineSteps = append(withoutEngineSteps, line) + } + + // Pass 1b: if the engine block is now empty (only blank lines or id: key), + // check whether any non-steps content remains under engine: + engineBlockIsEmpty := func() bool { + inEngine := false + engineIndentLen := 0 + for _, line := range withoutEngineSteps { + trimmed := strings.TrimSpace(line) + if isTopLevelKey(line) && strings.HasPrefix(trimmed, "engine:") { + inEngine = true + engineIndentLen = len(getIndentation(line)) + // Check for inline value (e.g., "engine: claude") + val := strings.TrimPrefix(trimmed, "engine:") + if strings.TrimSpace(val) != "" { + return false + } + continue + } + if inEngine { + if len(trimmed) == 0 { + continue + } + lineIndentLen := len(getIndentation(line)) + if lineIndentLen <= engineIndentLen { + // Exited engine block with no content found + return true + } + // There is content under engine (e.g., id:, model:, env:) + return false + } + } + return inEngine // if we're still in engine at EOF, it's empty + }() + + if engineBlockIsEmpty { + engineStepsCodemodLog.Print("Engine block is empty after removing 'steps', removing it") + // Remove the engine block (the engine: line and any blank lines around it) + cleaned := make([]string, 0, len(withoutEngineSteps)) + engineIndentLen := 0 + inEngine := false + for i, line := range withoutEngineSteps { + trimmed := strings.TrimSpace(line) + if isTopLevelKey(line) && strings.HasPrefix(trimmed, "engine:") { + inEngine = true + engineIndentLen = len(getIndentation(line)) + // Remove trailing blank lines already added + for len(cleaned) > 0 && strings.TrimSpace(cleaned[len(cleaned)-1]) == "" { + cleaned = cleaned[:len(cleaned)-1] + } + _ = i + continue + } + if inEngine { + if len(trimmed) == 0 { + continue + } + if len(getIndentation(line)) <= engineIndentLen { + inEngine = false + } else { + continue + } + } + cleaned = append(cleaned, line) + } + withoutEngineSteps = cleaned + } + + // Pass 2: insert engine steps at top level + var result []string + if !hasTopLevelSteps { + // Append engine steps at the end (as new top-level steps field) + result = append(withoutEngineSteps, topLevelStepsLines...) + engineStepsCodemodLog.Print("Added engine steps as new top-level 'steps'") + } else { + // Append engine step items after the top-level steps block + // Since we removed engine.steps lines, re-find the end of top-level steps + adjustedTopLevelEnd := topLevelStepsEndIdx + removedCount := stepsEndIdx - stepsStartIdx + 1 + // Only adjust if the engine.steps came before the top-level steps end + if stepsEndIdx < topLevelStepsEndIdx { + adjustedTopLevelEnd -= removedCount + } else if stepsStartIdx <= topLevelStepsEndIdx && stepsEndIdx >= topLevelStepsEndIdx { + // engine.steps overlaps with top-level steps end (shouldn't happen but handle gracefully) + adjustedTopLevelEnd -= removedCount + } + + result = make([]string, 0, len(withoutEngineSteps)+len(topLevelStepsLines)) + insertedSteps := false + for i, line := range withoutEngineSteps { + result = append(result, line) + if !insertedSteps && i == adjustedTopLevelEnd { + // Append the step items (skip the "steps:" header since one already exists) + for _, stepLine := range topLevelStepsLines { + if strings.TrimSpace(stepLine) == "steps:" { + continue + } + result = append(result, stepLine) + } + insertedSteps = true + engineStepsCodemodLog.Print("Appended engine steps to existing top-level 'steps'") + } + } + } + + newContent := reconstructContent(result, markdown) + engineStepsCodemodLog.Print("Successfully migrated 'engine.steps' to top-level 'steps'") + return newContent, true, nil + }, + } +} diff --git a/pkg/cli/codemod_engine_steps_test.go b/pkg/cli/codemod_engine_steps_test.go new file mode 100644 index 0000000000..c66ab40d54 --- /dev/null +++ b/pkg/cli/codemod_engine_steps_test.go @@ -0,0 +1,677 @@ +//go:build !integration + +package cli + +import ( + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestGetEngineStepsToTopLevelCodemod_Metadata(t *testing.T) { + codemod := getEngineStepsToTopLevelCodemod() + + assert.Equal(t, "engine-steps-to-top-level", codemod.ID) + assert.Equal(t, "Move engine.steps to top-level steps", codemod.Name) + assert.NotEmpty(t, codemod.Description) + assert.Equal(t, "0.11.0", codemod.IntroducedIn) + require.NotNil(t, codemod.Apply) +} + +// TestEngineStepsToTopLevelCodemod_NoOp tests cases where the codemod should not apply +func TestEngineStepsToTopLevelCodemod_NoOp(t *testing.T) { + tests := []struct { + name string + content string + frontmatter map[string]any + }{ + { + name: "no engine field", + content: `--- +on: push +--- + +# Test workflow`, + frontmatter: map[string]any{ + "on": "push", + }, + }, + { + name: "engine is a string", + content: `--- +on: push +engine: claude +--- + +# Test workflow`, + frontmatter: map[string]any{ + "on": "push", + "engine": "claude", + }, + }, + { + name: "engine object without steps", + content: `--- +on: push +engine: + id: claude + model: claude-3-5-sonnet +--- + +# Test workflow`, + frontmatter: map[string]any{ + "on": "push", + "engine": map[string]any{ + "id": "claude", + "model": "claude-3-5-sonnet", + }, + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + codemod := getEngineStepsToTopLevelCodemod() + result, applied, err := codemod.Apply(tt.content, tt.frontmatter) + require.NoError(t, err) + assert.False(t, applied, "Should not apply") + assert.Equal(t, tt.content, result, "Content should be unchanged") + }) + } +} + +// TestEngineStepsToTopLevelCodemod_SingleStep tests moving a single engine step to top level +func TestEngineStepsToTopLevelCodemod_SingleStep(t *testing.T) { + codemod := getEngineStepsToTopLevelCodemod() + + content := `--- +on: push +engine: + id: codex + steps: + - name: Run step + run: echo "hello" +--- + +# Test workflow` + + frontmatter := map[string]any{ + "on": "push", + "engine": map[string]any{ + "id": "codex", + "steps": []any{ + map[string]any{"name": "Run step", "run": `echo "hello"`}, + }, + }, + } + + result, applied, err := codemod.Apply(content, frontmatter) + + require.NoError(t, err) + assert.True(t, applied, "Should apply when engine has steps") + assert.Contains(t, result, "steps:") + assert.Contains(t, result, "name: Run step") + assert.Contains(t, result, "engine:") + assert.Contains(t, result, "id: codex") + // engine block should not contain steps: + assert.NotContains(t, result, " steps:") +} + +// TestEngineStepsToTopLevelCodemod_MultipleSteps tests moving multiple steps preserves order +func TestEngineStepsToTopLevelCodemod_MultipleSteps(t *testing.T) { + codemod := getEngineStepsToTopLevelCodemod() + + content := `--- +on: push +engine: + id: codex + model: gpt-4o + steps: + - name: Step 1 + run: echo "step1" + - name: Step 2 + run: echo "step2" +--- + +# Test workflow` + + frontmatter := map[string]any{ + "on": "push", + "engine": map[string]any{ + "id": "codex", + "model": "gpt-4o", + "steps": []any{ + map[string]any{"name": "Step 1", "run": `echo "step1"`}, + map[string]any{"name": "Step 2", "run": `echo "step2"`}, + }, + }, + } + + result, applied, err := codemod.Apply(content, frontmatter) + + require.NoError(t, err) + assert.True(t, applied) + + // Steps should be at top level in order + step1Pos := strings.Index(result, "name: Step 1") + step2Pos := strings.Index(result, "name: Step 2") + require.Positive(t, step1Pos, "Should contain 'name: Step 1'") + require.Positive(t, step2Pos, "Should contain 'name: Step 2'") + assert.Less(t, step1Pos, step2Pos, "Step 1 should appear before Step 2") + + // Engine should still have id and model + assert.Contains(t, result, "id: codex") + assert.Contains(t, result, "model: gpt-4o") + + // Engine should no longer have steps (check each line) + lines := strings.Split(result, "\n") + inEngine := false + for _, line := range lines { + trimmed := strings.TrimSpace(line) + if strings.HasPrefix(line, "engine:") { + inEngine = true + } else if inEngine && len(trimmed) > 0 && !strings.HasPrefix(line, " ") && !strings.HasPrefix(line, "\t") { + inEngine = false + } + if inEngine && trimmed == "steps:" { + t.Error("engine block should not contain 'steps:' after codemod") + } + } +} + +// TestEngineStepsToTopLevelCodemod_UsesStep tests a step that uses an action (not run:) +func TestEngineStepsToTopLevelCodemod_UsesStep(t *testing.T) { + codemod := getEngineStepsToTopLevelCodemod() + + content := `--- +on: push +engine: + id: codex + steps: + - name: Run AI Inference + uses: actions/ai-inference@v1 + with: + prompt-file: ${{ env.GH_AW_PROMPT }} + model: gpt-4o-mini +--- + +# Test workflow` + + frontmatter := map[string]any{ + "on": "push", + "engine": map[string]any{ + "id": "codex", + "steps": []any{ + map[string]any{ + "name": "Run AI Inference", + "uses": "actions/ai-inference@v1", + "with": map[string]any{ + "prompt-file": "${{ env.GH_AW_PROMPT }}", + "model": "gpt-4o-mini", + }, + }, + }, + }, + } + + result, applied, err := codemod.Apply(content, frontmatter) + + require.NoError(t, err) + assert.True(t, applied) + assert.Contains(t, result, "name: Run AI Inference") + assert.Contains(t, result, "uses: actions/ai-inference@v1") + assert.Contains(t, result, "prompt-file:") + assert.Contains(t, result, "model: gpt-4o-mini") + // Should be at top level, not inside engine + assert.NotContains(t, result, " steps:") +} + +// TestEngineStepsToTopLevelCodemod_EngineFieldsAfterSteps tests that engine fields after steps +// are preserved correctly in the engine block +func TestEngineStepsToTopLevelCodemod_EngineFieldsAfterSteps(t *testing.T) { + codemod := getEngineStepsToTopLevelCodemod() + + content := `--- +on: push +engine: + id: codex + steps: + - name: Prep + run: echo "prep" + model: gpt-4o +--- + +# Test workflow` + + frontmatter := map[string]any{ + "on": "push", + "engine": map[string]any{ + "id": "codex", + "model": "gpt-4o", + "steps": []any{ + map[string]any{"name": "Prep", "run": `echo "prep"`}, + }, + }, + } + + result, applied, err := codemod.Apply(content, frontmatter) + + require.NoError(t, err) + assert.True(t, applied) + // Engine fields should still be present + assert.Contains(t, result, "id: codex") + assert.Contains(t, result, "model: gpt-4o") + // Step should be at top level + assert.Contains(t, result, "name: Prep") +} + +// TestEngineStepsToTopLevelCodemod_MergeWithExistingSteps tests appending engine steps +// after existing top-level steps +func TestEngineStepsToTopLevelCodemod_MergeWithExistingSteps(t *testing.T) { + codemod := getEngineStepsToTopLevelCodemod() + + content := `--- +on: push +engine: + id: codex + steps: + - name: Engine Step + run: echo "engine" +steps: + - name: Existing Step + run: echo "existing" +--- + +# Test workflow` + + frontmatter := map[string]any{ + "on": "push", + "engine": map[string]any{ + "id": "codex", + "steps": []any{ + map[string]any{"name": "Engine Step", "run": `echo "engine"`}, + }, + }, + "steps": []any{ + map[string]any{"name": "Existing Step", "run": `echo "existing"`}, + }, + } + + result, applied, err := codemod.Apply(content, frontmatter) + + require.NoError(t, err) + assert.True(t, applied) + + // Both steps should be present + assert.Contains(t, result, "name: Engine Step") + assert.Contains(t, result, "name: Existing Step") + + // Should have only one top-level "steps:" header + stepsCount := strings.Count(result, "\nsteps:\n") + assert.Equal(t, 1, stepsCount, "Should have exactly one top-level 'steps:' header") + + // Engine block should not have steps + assert.NotContains(t, result, " steps:") + + // Existing step should come before the engine step (engine steps are appended) + existingPos := strings.Index(result, "name: Existing Step") + enginePos := strings.Index(result, "name: Engine Step") + require.Positive(t, existingPos) + require.Positive(t, enginePos) + assert.Less(t, existingPos, enginePos, "Existing step should come before appended engine step") +} + +// TestEngineStepsToTopLevelCodemod_NoMarkdownBody tests a workflow without a body section +func TestEngineStepsToTopLevelCodemod_NoMarkdownBody(t *testing.T) { + codemod := getEngineStepsToTopLevelCodemod() + + content := `--- +on: push +engine: + id: claude + steps: + - name: Setup + run: echo "setup" +---` + + frontmatter := map[string]any{ + "on": "push", + "engine": map[string]any{ + "id": "claude", + "steps": []any{ + map[string]any{"name": "Setup", "run": "echo \"setup\""}, + }, + }, + } + + result, applied, err := codemod.Apply(content, frontmatter) + + require.NoError(t, err) + assert.True(t, applied) + assert.Contains(t, result, "steps:") + assert.Contains(t, result, "name: Setup") + assert.NotContains(t, result, " steps:") +} + +// TestEngineStepsToTopLevelCodemod_Idempotent tests that applying the codemod twice +// (simulated by running on output with updated frontmatter) does not change the content +func TestEngineStepsToTopLevelCodemod_Idempotent(t *testing.T) { + codemod := getEngineStepsToTopLevelCodemod() + + // After codemod is applied, engine no longer has steps in frontmatter + alreadyMigratedContent := `--- +on: push +engine: + id: codex +steps: + - name: Run step + run: echo "hello" +--- + +# Test workflow` + + // Frontmatter reflects the already-migrated state (no engine.steps) + alreadyMigratedFrontmatter := map[string]any{ + "on": "push", + "engine": map[string]any{ + "id": "codex", + }, + "steps": []any{ + map[string]any{"name": "Run step", "run": `echo "hello"`}, + }, + } + + result, applied, err := codemod.Apply(alreadyMigratedContent, alreadyMigratedFrontmatter) + + require.NoError(t, err) + assert.False(t, applied, "Should not apply again when engine.steps is already gone") + assert.Equal(t, alreadyMigratedContent, result) +} + +// TestEngineStepsToTopLevelCodemod_StepsBeforeEngine tests when top-level steps field +// comes before the engine field in the YAML +func TestEngineStepsToTopLevelCodemod_StepsBeforeEngine(t *testing.T) { + codemod := getEngineStepsToTopLevelCodemod() + + content := `--- +on: push +steps: + - name: First Step + run: echo "first" +engine: + id: codex + steps: + - name: Engine Step + run: echo "engine" +--- + +# Test workflow` + + frontmatter := map[string]any{ + "on": "push", + "steps": []any{ + map[string]any{"name": "First Step", "run": `echo "first"`}, + }, + "engine": map[string]any{ + "id": "codex", + "steps": []any{ + map[string]any{"name": "Engine Step", "run": `echo "engine"`}, + }, + }, + } + + result, applied, err := codemod.Apply(content, frontmatter) + + require.NoError(t, err) + assert.True(t, applied) + + // Both steps should be present + assert.Contains(t, result, "name: First Step") + assert.Contains(t, result, "name: Engine Step") + + // Only one top-level steps: header + stepsCount := strings.Count(result, "\nsteps:\n") + assert.Equal(t, 1, stepsCount, "Should have exactly one top-level 'steps:' header") + + // Engine should not have steps + assert.NotContains(t, result, " steps:") +} + +// TestEngineStepsToTopLevelCodemod_PreservesMarkdownBody tests that the markdown body +// is preserved after the frontmatter when applying the codemod +func TestEngineStepsToTopLevelCodemod_PreservesMarkdownBody(t *testing.T) { + codemod := getEngineStepsToTopLevelCodemod() + + content := `--- +on: push +engine: + id: claude + steps: + - name: Install deps + run: npm install +--- + +# My Workflow + +This workflow does something useful. + +## Instructions + +Follow these steps carefully.` + + frontmatter := map[string]any{ + "on": "push", + "engine": map[string]any{ + "id": "claude", + "steps": []any{ + map[string]any{"name": "Install deps", "run": "npm install"}, + }, + }, + } + + result, applied, err := codemod.Apply(content, frontmatter) + + require.NoError(t, err) + assert.True(t, applied) + + // Markdown body should be preserved + assert.Contains(t, result, "# My Workflow") + assert.Contains(t, result, "This workflow does something useful.") + assert.Contains(t, result, "## Instructions") + assert.Contains(t, result, "Follow these steps carefully.") + + // Frontmatter changes should also be present + assert.Contains(t, result, "name: Install deps") +} + +// TestEngineStepsToTopLevelCodemod_TableDriven is a comprehensive table-driven test +func TestEngineStepsToTopLevelCodemod_TableDriven(t *testing.T) { + tests := []struct { + name string + content string + frontmatter map[string]any + wantApplied bool + wantContains []string + wantAbsent []string + }{ + { + name: "claude engine with single run step", + content: `--- +on: issues +engine: + id: claude + steps: + - name: Checkout + uses: actions/checkout@v4 +--- + +Do something`, + frontmatter: map[string]any{ + "on": "issues", + "engine": map[string]any{ + "id": "claude", + "steps": []any{ + map[string]any{"name": "Checkout", "uses": "actions/checkout@v4"}, + }, + }, + }, + wantApplied: true, + wantContains: []string{"name: Checkout", "uses: actions/checkout@v4", "steps:\n"}, + wantAbsent: []string{" steps:"}, + }, + { + name: "engine with env and steps - env preserved", + content: `--- +on: push +engine: + id: codex + env: + MY_VAR: value + steps: + - name: Test + run: echo test +---`, + frontmatter: map[string]any{ + "on": "push", + "engine": map[string]any{ + "id": "codex", + "env": map[string]any{ + "MY_VAR": "value", + }, + "steps": []any{ + map[string]any{"name": "Test", "run": "echo test"}, + }, + }, + }, + wantApplied: true, + wantContains: []string{"env:", "MY_VAR: value", "name: Test"}, + wantAbsent: []string{" steps:"}, + }, + { + name: "copilot engine with steps", + content: `--- +on: pull_request +engine: + id: copilot + steps: + - name: Setup Node + uses: actions/setup-node@v4 + with: + node-version: "20" +--- + +Review this PR`, + frontmatter: map[string]any{ + "on": "pull_request", + "engine": map[string]any{ + "id": "copilot", + "steps": []any{ + map[string]any{ + "name": "Setup Node", + "uses": "actions/setup-node@v4", + "with": map[string]any{"node-version": "20"}, + }, + }, + }, + }, + wantApplied: true, + wantContains: []string{"name: Setup Node", "uses: actions/setup-node@v4"}, + wantAbsent: []string{" steps:"}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + codemod := getEngineStepsToTopLevelCodemod() + result, applied, err := codemod.Apply(tt.content, tt.frontmatter) + + require.NoError(t, err, "Should not return error") + assert.Equal(t, tt.wantApplied, applied, "Applied state mismatch") + + for _, want := range tt.wantContains { + assert.Contains(t, result, want, "Should contain %q", want) + } + for _, absent := range tt.wantAbsent { + assert.NotContains(t, result, absent, "Should not contain %q", absent) + } + }) + } +} + +// TestEngineStepsToTopLevelCodemod_EmptyEngineBlockRemoved tests that a dangling +// engine: block (containing only steps) is removed after migration +func TestEngineStepsToTopLevelCodemod_EmptyEngineBlockRemoved(t *testing.T) { + codemod := getEngineStepsToTopLevelCodemod() + + content := `--- +on: push +engine: + steps: + - name: Only step + run: echo "only" +--- + +# Test workflow` + + frontmatter := map[string]any{ + "on": "push", + "engine": map[string]any{ + "steps": []any{ + map[string]any{"name": "Only step", "run": `echo "only"`}, + }, + }, + } + + result, applied, err := codemod.Apply(content, frontmatter) + + require.NoError(t, err) + assert.True(t, applied) + + // Step should be at top level + assert.Contains(t, result, "name: Only step") + + // The empty engine block should be removed (it only contained steps) + assert.NotContains(t, result, "engine:") +} + +// TestEngineStepsToTopLevelCodemod_NonSequenceTopLevelSteps tests that when +// top-level steps exists but is not a sequence, a fresh steps block is inserted +func TestEngineStepsToTopLevelCodemod_NonSequenceTopLevelSteps(t *testing.T) { + codemod := getEngineStepsToTopLevelCodemod() + + content := `--- +on: push +engine: + id: codex + steps: + - name: Engine Step + run: echo "engine" +steps: invalid-scalar +--- + +# Test workflow` + + frontmatter := map[string]any{ + "on": "push", + "engine": map[string]any{ + "id": "codex", + "steps": []any{ + map[string]any{"name": "Engine Step", "run": `echo "engine"`}, + }, + }, + // steps is a scalar, not a slice + "steps": "invalid-scalar", + } + + result, applied, err := codemod.Apply(content, frontmatter) + + require.NoError(t, err) + assert.True(t, applied) + + // Engine step should be present as a new top-level block + assert.Contains(t, result, "name: Engine Step") + // Engine block should not have steps any more + assert.NotContains(t, result, " steps:") +} diff --git a/pkg/cli/fix_codemods.go b/pkg/cli/fix_codemods.go index 47c48f95f8..b10612b05c 100644 --- a/pkg/cli/fix_codemods.go +++ b/pkg/cli/fix_codemods.go @@ -34,9 +34,10 @@ func GetAllCodemods() []Codemod { getDiscussionFlagRemovalCodemod(), getMCPModeToTypeCodemod(), getInstallScriptURLCodemod(), - getBashAnonymousRemovalCodemod(), // Replace bash: with bash: false - getActivationOutputsCodemod(), // Transform needs.activation.outputs.* to steps.sanitized.outputs.* - getRolesToOnRolesCodemod(), // Move top-level roles to on.roles - getBotsToOnBotsCodemod(), // Move top-level bots to on.bots + getBashAnonymousRemovalCodemod(), // Replace bash: with bash: false + getActivationOutputsCodemod(), // Transform needs.activation.outputs.* to steps.sanitized.outputs.* + getRolesToOnRolesCodemod(), // Move top-level roles to on.roles + getBotsToOnBotsCodemod(), // Move top-level bots to on.bots + getEngineStepsToTopLevelCodemod(), // Move engine.steps to top-level steps } } diff --git a/pkg/cli/fix_codemods_test.go b/pkg/cli/fix_codemods_test.go index d89a0d9840..aa9e256062 100644 --- a/pkg/cli/fix_codemods_test.go +++ b/pkg/cli/fix_codemods_test.go @@ -43,7 +43,7 @@ func TestGetAllCodemods_ReturnsAllCodemods(t *testing.T) { codemods := GetAllCodemods() // Verify we have the expected number of codemods - expectedCount := 20 + expectedCount := 21 assert.Len(t, codemods, expectedCount, "Should return all %d codemods", expectedCount) // Verify all codemods have required fields @@ -124,6 +124,7 @@ func TestGetAllCodemods_InExpectedOrder(t *testing.T) { "activation-outputs-to-sanitized-step", "roles-to-on-roles", "bots-to-on-bots", + "engine-steps-to-top-level", } require.Len(t, codemods, len(expectedOrder), "Should have expected number of codemods") diff --git a/pkg/cli/mcp_registry_live_test.go b/pkg/cli/mcp_registry_live_test.go index 60df509e19..0e726643f6 100644 --- a/pkg/cli/mcp_registry_live_test.go +++ b/pkg/cli/mcp_registry_live_test.go @@ -138,6 +138,13 @@ func TestMCPRegistryClient_LiveGetServer(t *testing.T) { // Now test GetServer with that name server, err := client.GetServer(serverName) if err != nil { + if strings.Contains(err.Error(), "network") || strings.Contains(err.Error(), "firewall") || + strings.Contains(err.Error(), "403") || strings.Contains(err.Error(), "connection") || + strings.Contains(err.Error(), "503") || strings.Contains(err.Error(), "502") || + strings.Contains(err.Error(), "upstream") || strings.Contains(err.Error(), "reset") { + t.Skipf("Skipping due to registry unavailability: %v", err) + return + } t.Fatalf("GetServer failed for '%s': %v", serverName, err) } @@ -313,6 +320,8 @@ func TestMCPRegistryClient_GitHubRegistryAccessibility(t *testing.T) { t.Logf("✓ GitHub MCP registry is accessible and returned 200 OK") case http.StatusForbidden: t.Logf("✓ GitHub MCP registry is reachable but returned 403 (expected due to network restrictions)") + case http.StatusServiceUnavailable, http.StatusBadGateway, http.StatusGatewayTimeout: + t.Skipf("GitHub MCP registry returned %d (service temporarily unavailable)", resp.StatusCode) default: t.Errorf("GitHub MCP registry returned unexpected status: %d", resp.StatusCode) } diff --git a/pkg/parser/schemas/main_workflow_schema.json b/pkg/parser/schemas/main_workflow_schema.json index 86ead0ca20..06c653f5e5 100644 --- a/pkg/parser/schemas/main_workflow_schema.json +++ b/pkg/parser/schemas/main_workflow_schema.json @@ -6922,14 +6922,6 @@ "type": "string" } }, - "steps": { - "type": "array", - "description": "Custom GitHub Actions steps for 'custom' engine. Define your own deterministic workflow steps instead of using AI processing.", - "items": { - "type": "object", - "additionalProperties": true - } - }, "error_patterns": { "type": "array", "description": "Custom error patterns for validating agent logs", diff --git a/pkg/workflow/claude_engine.go b/pkg/workflow/claude_engine.go index c3f80c32e6..33f75f7fad 100644 --- a/pkg/workflow/claude_engine.go +++ b/pkg/workflow/claude_engine.go @@ -142,8 +142,7 @@ func (e *ClaudeEngine) GetDeclaredOutputFiles() []string { func (e *ClaudeEngine) GetExecutionSteps(workflowData *WorkflowData, logFile string) []GitHubActionStep { claudeLog.Printf("Generating execution steps for Claude engine: workflow=%s, firewall=%v", workflowData.Name, isFirewallEnabled(workflowData)) - // Handle custom steps if they exist in engine config - steps := InjectCustomEngineSteps(workflowData, e.convertStepToYAML) + var steps []GitHubActionStep // Build claude CLI arguments based on configuration var claudeArgs []string diff --git a/pkg/workflow/codex_engine.go b/pkg/workflow/codex_engine.go index dbff6ce1c8..241eb42e18 100644 --- a/pkg/workflow/codex_engine.go +++ b/pkg/workflow/codex_engine.go @@ -130,8 +130,7 @@ func (e *CodexEngine) GetExecutionSteps(workflowData *WorkflowData, logFile stri codexEngineLog.Printf("Building Codex execution steps: workflow=%s, model=%s, has_agent_file=%v, firewall=%v", workflowData.Name, model, workflowData.AgentFile != "", firewallEnabled) - // Handle custom steps if they exist in engine config - steps := InjectCustomEngineSteps(workflowData, e.convertStepToYAML) + var steps []GitHubActionStep // Build model parameter only if specified in engineConfig // Otherwise, model can be set via GH_AW_MODEL_AGENT_CODEX or GH_AW_MODEL_DETECTION_CODEX environment variable diff --git a/pkg/workflow/collect_packages_test.go b/pkg/workflow/collect_packages_test.go index 3813a6d2df..f712b72b3e 100644 --- a/pkg/workflow/collect_packages_test.go +++ b/pkg/workflow/collect_packages_test.go @@ -4,6 +4,7 @@ package workflow import ( "reflect" + "strings" "testing" ) @@ -88,95 +89,6 @@ func TestCollectPackagesFromWorkflow_CustomSteps(t *testing.T) { } } -// TestCollectPackagesFromWorkflow_EngineSteps tests package extraction from engine steps -func TestCollectPackagesFromWorkflow_EngineSteps(t *testing.T) { - tests := []struct { - name string - engineSteps []map[string]any - extractor func(string) []string - expected []string - }{ - { - name: "Single run step with package", - engineSteps: []map[string]any{ - {"run": "pip install requests"}, - }, - extractor: func(s string) []string { - return []string{"requests"} - }, - expected: []string{"requests"}, - }, - { - name: "Multiple run steps with packages", - engineSteps: []map[string]any{ - {"run": "pip install requests"}, - {"run": "pip install flask"}, - }, - extractor: func(s string) []string { - if s == "pip install requests" { - return []string{"requests"} - } - return []string{"flask"} - }, - expected: []string{"requests", "flask"}, - }, - { - name: "Step without run command", - engineSteps: []map[string]any{ - {"name": "Setup"}, - }, - extractor: func(s string) []string { - return []string{} - }, - expected: []string{}, - }, - { - name: "Run command with non-string value", - engineSteps: []map[string]any{ - {"run": 123}, - }, - extractor: func(s string) []string { - return []string{"package"} - }, - expected: []string{}, - }, - { - name: "Duplicate packages across steps", - engineSteps: []map[string]any{ - {"run": "pip install requests"}, - {"run": "pip install requests flask"}, - }, - extractor: func(s string) []string { - if s == "pip install requests" { - return []string{"requests"} - } - return []string{"requests", "flask"} - }, - expected: []string{"requests", "flask"}, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - workflowData := &WorkflowData{ - EngineConfig: &EngineConfig{ - Steps: tt.engineSteps, - }, - } - - packages := collectPackagesFromWorkflow(workflowData, tt.extractor, "") - - if len(packages) != len(tt.expected) { - t.Errorf("Expected %v, got %v", tt.expected, packages) - return - } - if len(packages) > 0 && !reflect.DeepEqual(packages, tt.expected) { - t.Errorf("Expected %v, got %v", tt.expected, packages) - } - }) - } -} - // TestCollectPackagesFromWorkflow_MCPConfig tests package extraction from MCP server configurations func TestCollectPackagesFromWorkflow_MCPConfig(t *testing.T) { tests := []struct { @@ -326,12 +238,7 @@ func TestCollectPackagesFromWorkflow_Combined(t *testing.T) { { name: "Packages from all sources with deduplication", workflowData: &WorkflowData{ - CustomSteps: "npm install axios", - EngineConfig: &EngineConfig{ - Steps: []map[string]any{ - {"run": "npm install lodash"}, - }, - }, + CustomSteps: "npm install axios\nnpm install lodash", Tools: map[string]any{ "server1": map[string]any{ "command": "npx", @@ -340,13 +247,14 @@ func TestCollectPackagesFromWorkflow_Combined(t *testing.T) { }, }, extractor: func(s string) []string { - if s == "npm install axios" { - return []string{"axios"} + var result []string + if strings.Contains(s, "npm install axios") { + result = append(result, "axios") } - if s == "npm install lodash" { - return []string{"lodash"} + if strings.Contains(s, "npm install lodash") { + result = append(result, "lodash") } - return []string{} + return result }, toolCommand: "npx", expected: []string{"axios", "lodash"}, @@ -355,10 +263,7 @@ func TestCollectPackagesFromWorkflow_Combined(t *testing.T) { name: "Empty sources", workflowData: &WorkflowData{ CustomSteps: "", - EngineConfig: &EngineConfig{ - Steps: []map[string]any{}, - }, - Tools: map[string]any{}, + Tools: map[string]any{}, }, extractor: func(s string) []string { return []string{} diff --git a/pkg/workflow/copilot_engine_execution.go b/pkg/workflow/copilot_engine_execution.go index ab1c1ecaa4..9bb8e256ee 100644 --- a/pkg/workflow/copilot_engine_execution.go +++ b/pkg/workflow/copilot_engine_execution.go @@ -36,8 +36,7 @@ var copilotExecLog = logger.New("workflow:copilot_engine_execution") func (e *CopilotEngine) GetExecutionSteps(workflowData *WorkflowData, logFile string) []GitHubActionStep { copilotExecLog.Printf("Generating execution steps for Copilot: workflow=%s, firewall=%v", workflowData.Name, isFirewallEnabled(workflowData)) - // Handle custom steps if they exist in engine config - steps := InjectCustomEngineSteps(workflowData, e.convertStepToYAML) + var steps []GitHubActionStep // Build copilot CLI arguments based on configuration var copilotArgs []string diff --git a/pkg/workflow/engine.go b/pkg/workflow/engine.go index d5484d5a4f..f87ffedf88 100644 --- a/pkg/workflow/engine.go +++ b/pkg/workflow/engine.go @@ -20,7 +20,6 @@ type EngineConfig struct { UserAgent string Command string // Custom executable path (when set, skip installation steps) Env map[string]string - Steps []map[string]any Config string Args []string Firewall *FirewallConfig // AWF firewall configuration @@ -168,18 +167,6 @@ func (c *Compiler) ExtractEngineConfig(frontmatter map[string]any) (string, *Eng } } - // Extract optional 'steps' field (array of step objects) - if steps, hasSteps := engineObj["steps"]; hasSteps { - if stepsArray, ok := steps.([]any); ok { - config.Steps = make([]map[string]any, 0, len(stepsArray)) - for _, step := range stepsArray { - if stepMap, ok := step.(map[string]any); ok { - config.Steps = append(config.Steps, stepMap) - } - } - } - } - // Extract optional 'config' field (additional TOML configuration) if config_field, hasConfig := engineObj["config"]; hasConfig { if configStr, ok := config_field.(string); ok { diff --git a/pkg/workflow/engine_config_test.go b/pkg/workflow/engine_config_test.go index 2323c99f42..baf9e23195 100644 --- a/pkg/workflow/engine_config_test.go +++ b/pkg/workflow/engine_config_test.go @@ -3,7 +3,6 @@ package workflow import ( - "fmt" "os" "path/filepath" "strings" @@ -251,27 +250,6 @@ func TestExtractEngineConfig(t *testing.T) { } } - if len(config.Steps) != len(test.expectedConfig.Steps) { - t.Errorf("Expected config.Steps length %d, got %d", len(test.expectedConfig.Steps), len(config.Steps)) - } else { - for i, expectedStep := range test.expectedConfig.Steps { - if i >= len(config.Steps) { - t.Errorf("Expected step at index %d", i) - continue - } - actualStep := config.Steps[i] - for key, expectedValue := range expectedStep { - if actualValue, exists := actualStep[key]; !exists { - t.Errorf("Expected step[%d] to contain key '%s'", i, key) - } else { - // For nested maps, do a simple string comparison for now - if fmt.Sprintf("%v", actualValue) != fmt.Sprintf("%v", expectedValue) { - t.Errorf("Expected step[%d]['%s'] = '%v', got '%v'", i, key, expectedValue, actualValue) - } - } - } - } - } } }) } diff --git a/pkg/workflow/engine_helpers.go b/pkg/workflow/engine_helpers.go index 4e9e38438e..9df9fdfca5 100644 --- a/pkg/workflow/engine_helpers.go +++ b/pkg/workflow/engine_helpers.go @@ -201,39 +201,6 @@ func BuildStandardNpmEngineInstallSteps( ) } -// InjectCustomEngineSteps processes custom steps from engine config and converts them to GitHubActionSteps. -// This shared function extracts the common pattern used by Copilot, Codex, and Claude engines. -// -// Parameters: -// - workflowData: The workflow data containing engine configuration -// - convertStepFunc: A function that converts a step map to YAML string (engine-specific) -// -// Returns: -// - []GitHubActionStep: Array of custom steps ready to be included in the execution pipeline -func InjectCustomEngineSteps( - workflowData *WorkflowData, - convertStepFunc func(map[string]any) (string, error), -) []GitHubActionStep { - var steps []GitHubActionStep - - // Handle custom steps if they exist in engine config - if workflowData.EngineConfig != nil && len(workflowData.EngineConfig.Steps) > 0 { - engineHelpersLog.Printf("Injecting %d custom engine steps", len(workflowData.EngineConfig.Steps)) - for _, step := range workflowData.EngineConfig.Steps { - stepYAML, err := convertStepFunc(step) - if err != nil { - engineHelpersLog.Printf("Failed to convert custom step: %v", err) - // Log error but continue with other steps - continue - } - steps = append(steps, GitHubActionStep{stepYAML}) - } - engineHelpersLog.Printf("Successfully injected %d custom engine steps", len(steps)) - } - - return steps -} - // RenderCustomMCPToolConfigHandler is a function type that engines must provide to render their specific MCP config // FormatStepWithCommandAndEnv formats a GitHub Actions step with command and environment variables. // This shared function extracts the common pattern used by Copilot and Codex engines. diff --git a/pkg/workflow/engine_helpers_shared_test.go b/pkg/workflow/engine_helpers_shared_test.go index fbb848b38d..1adada0646 100644 --- a/pkg/workflow/engine_helpers_shared_test.go +++ b/pkg/workflow/engine_helpers_shared_test.go @@ -8,124 +8,6 @@ import ( "testing" ) -// TestInjectCustomEngineSteps verifies that custom steps from engine config are properly injected -// Note: This is used by all engines (claude, codex, copilot) to support custom steps in engine config -func TestInjectCustomEngineSteps(t *testing.T) { - tests := []struct { - name string - workflowData *WorkflowData - expectedSteps int - expectedErr bool - convertErrStep int // Which step should fail conversion (0 = none) - }{ - { - name: "No custom steps", - workflowData: &WorkflowData{ - EngineConfig: nil, - }, - expectedSteps: 0, - }, - { - name: "Empty custom steps", - workflowData: &WorkflowData{ - EngineConfig: &EngineConfig{ - Steps: []map[string]any{}, - }, - }, - expectedSteps: 0, - }, - { - name: "Single custom step", - workflowData: &WorkflowData{ - EngineConfig: &EngineConfig{ - Steps: []map[string]any{ - { - "name": "Test Step", - "run": "echo 'test'", - }, - }, - }, - }, - expectedSteps: 1, - }, - { - name: "Multiple custom steps", - workflowData: &WorkflowData{ - EngineConfig: &EngineConfig{ - Steps: []map[string]any{ - { - "name": "Step 1", - "run": "echo 'step1'", - }, - { - "name": "Step 2", - "run": "echo 'step2'", - }, - { - "name": "Step 3", - "run": "echo 'step3'", - }, - }, - }, - }, - expectedSteps: 3, - }, - { - name: "Step conversion error - should continue", - workflowData: &WorkflowData{ - EngineConfig: &EngineConfig{ - Steps: []map[string]any{ - { - "name": "Step 1", - "run": "echo 'step1'", - }, - { - "name": "Step 2 - will fail", - "run": "echo 'step2'", - }, - { - "name": "Step 3", - "run": "echo 'step3'", - }, - }, - }, - }, - expectedSteps: 2, // Only 2 steps should succeed - convertErrStep: 2, // Second step fails - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - // Create a mock convert function - stepCounter := 0 - convertStepFunc := func(stepMap map[string]any) (string, error) { - stepCounter++ - // Simulate conversion error for specific step - if tt.convertErrStep > 0 && stepCounter == tt.convertErrStep { - return "", fmt.Errorf("conversion error for step %d", stepCounter) - } - // Return a simple YAML representation - name := stepMap["name"] - return fmt.Sprintf(" - name: %v\n run: test\n", name), nil - } - - steps := InjectCustomEngineSteps(tt.workflowData, convertStepFunc) - - if len(steps) != tt.expectedSteps { - t.Errorf("Expected %d steps, got %d", tt.expectedSteps, len(steps)) - } - - // Verify each step contains valid YAML - for i, step := range steps { - if len(step) == 0 { - t.Errorf("Step %d is empty", i) - } - } - }) - } -} - // TestHandleCustomMCPToolInSwitch verifies custom MCP tool handling in switch statements func TestHandleCustomMCPToolInSwitch(t *testing.T) { tests := []struct { @@ -243,48 +125,6 @@ func TestHandleCustomMCPToolInSwitch(t *testing.T) { } } -// TestInjectCustomEngineStepsWithRealConversion tests with actual ConvertStepToYAML function -func TestInjectCustomEngineStepsWithRealConversion(t *testing.T) { - workflowData := &WorkflowData{ - EngineConfig: &EngineConfig{ - Steps: []map[string]any{ - { - "name": "Install dependencies", - "run": "npm install", - }, - { - "name": "Run tests", - "run": "npm test", - }, - }, - }, - } - - steps := InjectCustomEngineSteps(workflowData, ConvertStepToYAML) - - if len(steps) != 2 { - t.Fatalf("Expected 2 steps, got %d", len(steps)) - } - - // Verify the YAML content of the first step - firstStepYAML := steps[0][0] - if !strings.Contains(firstStepYAML, "Install dependencies") { - t.Errorf("First step should contain 'Install dependencies', got: %s", firstStepYAML) - } - if !strings.Contains(firstStepYAML, "npm install") { - t.Errorf("First step should contain 'npm install', got: %s", firstStepYAML) - } - - // Verify the YAML content of the second step - secondStepYAML := steps[1][0] - if !strings.Contains(secondStepYAML, "Run tests") { - t.Errorf("Second step should contain 'Run tests', got: %s", secondStepYAML) - } - if !strings.Contains(secondStepYAML, "npm test") { - t.Errorf("Second step should contain 'npm test', got: %s", secondStepYAML) - } -} - // TestFormatStepWithCommandAndEnv verifies step formatting with command and environment variables func TestFormatStepWithCommandAndEnv(t *testing.T) { tests := []struct { diff --git a/pkg/workflow/engine_includes_test.go b/pkg/workflow/engine_includes_test.go index 983afc911c..d2dd1222d6 100644 --- a/pkg/workflow/engine_includes_test.go +++ b/pkg/workflow/engine_includes_test.go @@ -377,7 +377,7 @@ This should fail due to multiple engine specifications in includes. } } -// TestImportedEngineWithCustomSteps tests importing a codex engine configuration with steps +// TestImportedEngineWithCustomSteps tests importing a codex engine configuration with top-level steps func TestImportedEngineWithCustomSteps(t *testing.T) { // Create temporary directory for test files tmpDir := testutil.TempDir(t, "test-*") @@ -387,16 +387,16 @@ func TestImportedEngineWithCustomSteps(t *testing.T) { t.Fatal(err) } - // Create shared file with codex engine and steps + // Create shared file with codex engine and top-level steps sharedContent := `--- engine: id: codex - steps: - - name: Run AI Inference - uses: actions/ai-inference@v1 - with: - prompt-file: ${{ env.GH_AW_PROMPT }} - model: gpt-4o-mini +steps: + - name: Run AI Inference + uses: actions/ai-inference@v1 + with: + prompt-file: ${{ env.GH_AW_PROMPT }} + model: gpt-4o-mini ---