diff --git a/.github/workflows/unbloat-docs.lock.yml b/.github/workflows/unbloat-docs.lock.yml index 5cbc6d1dd..01f29d787 100644 --- a/.github/workflows/unbloat-docs.lock.yml +++ b/.github/workflows/unbloat-docs.lock.yml @@ -943,16 +943,16 @@ jobs: output: ${{ steps.collect_output.outputs.output }} output_types: ${{ steps.collect_output.outputs.output_types }} steps: + - name: Checkout repository + uses: actions/checkout@93cb6efe18208431cddfb8368fd83d5badbf9bfd + with: + persist-credentials: false - name: Setup Node.js uses: actions/setup-node@2028fbc5c25fe9cf00d9f06a71cc4710d4507903 # v6 with: node-version: '24' cache: 'npm' cache-dependency-path: 'docs/package-lock.json' - - name: Checkout repository - uses: actions/checkout@93cb6efe18208431cddfb8368fd83d5badbf9bfd - with: - persist-credentials: false - name: Install dependencies run: npm ci working-directory: ./docs diff --git a/pkg/workflow/checkout_runtime_order_test.go b/pkg/workflow/checkout_runtime_order_test.go new file mode 100644 index 000000000..0d7ad6864 --- /dev/null +++ b/pkg/workflow/checkout_runtime_order_test.go @@ -0,0 +1,268 @@ +package workflow + +import ( + "os" + "path/filepath" + "strings" + "testing" + + "github.com/githubnext/gh-aw/pkg/constants" +) + +// TestCheckoutRuntimeOrderInCustomSteps verifies that when custom steps contain +// a checkout step, runtime setup steps are inserted AFTER the checkout step, +// not before it. This ensures that checkout is always the first step. +func TestCheckoutRuntimeOrderInCustomSteps(t *testing.T) { + workflowContent := `--- +on: push +permissions: + contents: read + issues: read + pull-requests: read +engine: copilot +steps: + - name: Checkout code + uses: actions/checkout@v5 + - name: Use Node + run: node --version +--- + +# Test workflow with checkout in custom steps +` + + // Create temporary directory for test + tempDir, err := os.MkdirTemp("", "checkout-runtime-order-test") + if err != nil { + t.Fatalf("Failed to create temp dir: %v", err) + } + defer os.RemoveAll(tempDir) + + // Create workflows directory + workflowsDir := filepath.Join(tempDir, constants.GetWorkflowDir()) + if err := os.MkdirAll(workflowsDir, 0755); err != nil { + t.Fatalf("Failed to create workflows directory: %v", err) + } + + // Write test workflow file + workflowPath := filepath.Join(workflowsDir, "test-workflow.md") + if err := os.WriteFile(workflowPath, []byte(workflowContent), 0644); err != nil { + t.Fatalf("Failed to write workflow file: %v", err) + } + + // Compile workflow + compiler := NewCompiler(false, "", "test-version") + if err := compiler.CompileWorkflow(workflowPath); err != nil { + t.Fatalf("Failed to compile workflow: %v", err) + } + + // Read generated lock file + lockPath := filepath.Join(workflowsDir, "test-workflow.lock.yml") + lockContent, err := os.ReadFile(lockPath) + if err != nil { + t.Fatalf("Failed to read lock file: %v", err) + } + lockStr := string(lockContent) + + // Extract the agent job section + agentJobStart := strings.Index(lockStr, " agent:") + if agentJobStart == -1 { + t.Fatal("Could not find agent job in compiled workflow") + } + + // Find the next job (starts with " " followed by a non-space character, e.g., " activation:") + // We need to skip the agent job content which has more indentation + remainingContent := lockStr[agentJobStart+10:] + nextJobStart := -1 + lines := strings.Split(remainingContent, "\n") + for i, line := range lines { + // A new job starts with exactly 2 spaces followed by a letter/number (not more spaces) + if len(line) > 2 && line[0] == ' ' && line[1] == ' ' && line[2] != ' ' && line[2] != '\t' { + // Calculate the position in the original string + nextJobStart = 0 + for j := 0; j < i; j++ { + nextJobStart += len(lines[j]) + 1 // +1 for newline + } + break + } + } + + var agentJobSection string + if nextJobStart == -1 { + agentJobSection = lockStr[agentJobStart:] + } else { + agentJobSection = lockStr[agentJobStart : agentJobStart+10+nextJobStart] + } + + // Debug: print first 1000 chars of agent job section + sampleSize := min(1000, len(agentJobSection)) + t.Logf("Agent job section (first %d chars):\n%s", sampleSize, agentJobSection[:sampleSize]) + + // Find all step names in order + stepNames := []string{} + stepLines := strings.Split(agentJobSection, "\n") + for _, line := range stepLines { + // Check if line contains "- name:" (with any amount of leading whitespace) + if strings.Contains(line, "- name:") { + // Extract the name part after "- name:" + parts := strings.SplitN(line, "- name:", 2) + if len(parts) == 2 { + name := strings.TrimSpace(parts[1]) + stepNames = append(stepNames, name) + } + } + } + + t.Logf("Found %d steps: %v", len(stepNames), stepNames) + + if len(stepNames) < 3 { + t.Fatalf("Expected at least 3 steps, got %d: %v", len(stepNames), stepNames) + } + + // Verify the order: + // 1. First step should be "Checkout code" (from custom steps) + // 2. Second step should be "Setup Node.js" (runtime setup, inserted after checkout) + // 3. Third step should be "Use Node" (from custom steps) + + if stepNames[0] != "Checkout code" { + t.Errorf("First step should be 'Checkout code', got '%s'", stepNames[0]) + } + + if stepNames[1] != "Setup Node.js" { + t.Errorf("Second step should be 'Setup Node.js' (runtime setup after checkout), got '%s'", stepNames[1]) + } + + if stepNames[2] != "Use Node" { + t.Errorf("Third step should be 'Use Node', got '%s'", stepNames[2]) + } + + // Additional check: verify Setup Node.js appears AFTER Checkout code + checkoutIndex := strings.Index(agentJobSection, "Checkout code") + setupNodeIndex := strings.Index(agentJobSection, "Setup Node.js") + + if checkoutIndex == -1 { + t.Fatal("Could not find 'Checkout code' step in agent job") + } + + if setupNodeIndex == -1 { + t.Fatal("Could not find 'Setup Node.js' step in agent job") + } + + if setupNodeIndex < checkoutIndex { + t.Error("Setup Node.js appears before Checkout code, should be after") + } + + t.Logf("Step order is correct:") + for i, name := range stepNames[:3] { + t.Logf(" %d. %s", i+1, name) + } +} + +// TestCheckoutFirstWhenNoCustomSteps verifies that when there are no custom steps, +// the automatic checkout is added first. +func TestCheckoutFirstWhenNoCustomSteps(t *testing.T) { + workflowContent := `--- +on: push +permissions: + contents: read + issues: read + pull-requests: read +engine: copilot +--- + +# Test workflow without custom steps + +Run node --version to check the Node.js version. +` + + // Create temporary directory for test + tempDir, err := os.MkdirTemp("", "checkout-first-test") + if err != nil { + t.Fatalf("Failed to create temp dir: %v", err) + } + defer os.RemoveAll(tempDir) + + // Create workflows directory + workflowsDir := filepath.Join(tempDir, constants.GetWorkflowDir()) + if err := os.MkdirAll(workflowsDir, 0755); err != nil { + t.Fatalf("Failed to create workflows directory: %v", err) + } + + // Write test workflow file + workflowPath := filepath.Join(workflowsDir, "test-workflow.md") + if err := os.WriteFile(workflowPath, []byte(workflowContent), 0644); err != nil { + t.Fatalf("Failed to write workflow file: %v", err) + } + + // Compile workflow + compiler := NewCompiler(false, "", "test-version") + if err := compiler.CompileWorkflow(workflowPath); err != nil { + t.Fatalf("Failed to compile workflow: %v", err) + } + + // Read generated lock file + lockPath := filepath.Join(workflowsDir, "test-workflow.lock.yml") + lockContent, err := os.ReadFile(lockPath) + if err != nil { + t.Fatalf("Failed to read lock file: %v", err) + } + lockStr := string(lockContent) + + // Extract the agent job section + agentJobStart := strings.Index(lockStr, " agent:") + if agentJobStart == -1 { + t.Fatal("Could not find agent job in compiled workflow") + } + + // Find the next job (starts with " " followed by a non-space character, e.g., " activation:") + // We need to skip the agent job content which has more indentation + remainingContent := lockStr[agentJobStart+10:] + nextJobStart := -1 + lines := strings.Split(remainingContent, "\n") + for i, line := range lines { + // A new job starts with exactly 2 spaces followed by a letter/number (not more spaces) + if len(line) > 2 && line[0] == ' ' && line[1] == ' ' && line[2] != ' ' && line[2] != '\t' { + // Calculate the position in the original string + nextJobStart = 0 + for j := 0; j < i; j++ { + nextJobStart += len(lines[j]) + 1 // +1 for newline + } + break + } + } + + var agentJobSection string + if nextJobStart == -1 { + agentJobSection = lockStr[agentJobStart:] + } else { + agentJobSection = lockStr[agentJobStart : agentJobStart+10+nextJobStart] + } + + // Find all step names in order + stepNames := []string{} + stepLines := strings.Split(agentJobSection, "\n") + for _, line := range stepLines { + // Check if line contains "- name:" (with any amount of leading whitespace) + if strings.Contains(line, "- name:") { + // Extract the name part after "- name:" + parts := strings.SplitN(line, "- name:", 2) + if len(parts) == 2 { + name := strings.TrimSpace(parts[1]) + stepNames = append(stepNames, name) + } + } + } + + if len(stepNames) < 1 { + t.Fatalf("Expected at least 1 step, got %d: %v", len(stepNames), stepNames) + } + + // Verify the order: + // 1. First step should be "Checkout repository" (automatic) + + if stepNames[0] != "Checkout repository" { + t.Errorf("First step should be 'Checkout repository', got '%s'", stepNames[0]) + } + + t.Logf("Step order is correct:") + t.Logf(" 1. %s (first step is checkout)", stepNames[0]) +} diff --git a/pkg/workflow/compiler_yaml.go b/pkg/workflow/compiler_yaml.go index 76e4312ba..b14053507 100644 --- a/pkg/workflow/compiler_yaml.go +++ b/pkg/workflow/compiler_yaml.go @@ -191,12 +191,11 @@ func (c *Compiler) generateMainJobSteps(yaml *strings.Builder, data *WorkflowDat // Add automatic runtime setup steps if needed // This detects runtimes from custom steps and MCP configs - // Runtime steps are added BEFORE custom steps so the runtimes are available runtimeRequirements := DetectRuntimeRequirements(data) // Deduplicate runtime setup steps from custom steps // This removes any runtime setup action steps (like actions/setup-go) from custom steps - // since we're adding them above. It also preserves user-customized setup actions and + // since we're adding them. It also preserves user-customized setup actions and // filters those runtimes from requirements so we don't generate duplicates. if len(runtimeRequirements) > 0 && data.CustomSteps != "" { deduplicatedCustomSteps, filteredRequirements, err := DeduplicateRuntimeSetupStepsFromCustomSteps(data.CustomSteps, runtimeRequirements) @@ -211,27 +210,37 @@ func (c *Compiler) generateMainJobSteps(yaml *strings.Builder, data *WorkflowDat // Generate runtime setup steps (after filtering out user-customized ones) runtimeSetupSteps := GenerateRuntimeSetupSteps(runtimeRequirements) compilerYamlLog.Printf("Detected runtime requirements: %d runtimes, %d setup steps", len(runtimeRequirements), len(runtimeSetupSteps)) - for _, step := range runtimeSetupSteps { - for _, line := range step { - yaml.WriteString(line + "\n") + + // Decision logic for where to place runtime steps: + // 1. If we added checkout above (needsCheckout == true), add runtime steps now (after checkout, before custom steps) + // 2. If custom steps contain checkout, add runtime steps AFTER the first checkout in custom steps + // 3. Otherwise, add runtime steps now (before custom steps) + + customStepsContainCheckout := data.CustomSteps != "" && ContainsCheckout(data.CustomSteps) + compilerYamlLog.Printf("Custom steps contain checkout: %t (len(customSteps)=%d)", customStepsContainCheckout, len(data.CustomSteps)) + + if needsCheckout || !customStepsContainCheckout { + // Case 1 or 3: Add runtime steps before custom steps + // This ensures checkout -> runtime -> custom steps order + compilerYamlLog.Printf("Adding %d runtime steps before custom steps (needsCheckout=%t, !customStepsContainCheckout=%t)", len(runtimeSetupSteps), needsCheckout, !customStepsContainCheckout) + for _, step := range runtimeSetupSteps { + for _, line := range step { + yaml.WriteString(line + "\n") + } } } // Add custom steps if present if data.CustomSteps != "" { - // Remove "steps:" line and adjust indentation - lines := strings.Split(data.CustomSteps, "\n") - if len(lines) > 1 { - for _, line := range lines[1:] { - // Skip empty lines - if strings.TrimSpace(line) == "" { - yaml.WriteString("\n") - continue - } - - // Simply add 6 spaces for job context indentation - yaml.WriteString(" " + line + "\n") - } + if customStepsContainCheckout && len(runtimeSetupSteps) > 0 { + // Custom steps contain checkout and we have runtime steps to insert + // Insert runtime steps after the first checkout step + compilerYamlLog.Printf("Calling addCustomStepsWithRuntimeInsertion: %d runtime steps to insert after checkout", len(runtimeSetupSteps)) + c.addCustomStepsWithRuntimeInsertion(yaml, data.CustomSteps, runtimeSetupSteps) + } else { + // No checkout in custom steps or no runtime steps, just add custom steps as-is + compilerYamlLog.Printf("Calling addCustomStepsAsIs (customStepsContainCheckout=%t, runtimeStepsCount=%d)", customStepsContainCheckout, len(runtimeSetupSteps)) + c.addCustomStepsAsIs(yaml, data.CustomSteps) } } @@ -1020,3 +1029,107 @@ func generatePinnedActionsComment(usedPins map[string]ActionPin) string { return comment.String() } + +// addCustomStepsAsIs adds custom steps without modification +func (c *Compiler) addCustomStepsAsIs(yaml *strings.Builder, customSteps string) { + // Remove "steps:" line and adjust indentation + lines := strings.Split(customSteps, "\n") + if len(lines) > 1 { + for _, line := range lines[1:] { + // Skip empty lines + if strings.TrimSpace(line) == "" { + yaml.WriteString("\n") + continue + } + + // Simply add 6 spaces for job context indentation + yaml.WriteString(" " + line + "\n") + } + } +} + +// addCustomStepsWithRuntimeInsertion adds custom steps and inserts runtime steps after the first checkout +func (c *Compiler) addCustomStepsWithRuntimeInsertion(yaml *strings.Builder, customSteps string, runtimeSetupSteps []GitHubActionStep) { + // Remove "steps:" line and adjust indentation + lines := strings.Split(customSteps, "\n") + if len(lines) <= 1 { + return + } + + insertedRuntime := false + i := 1 // Start from index 1 to skip "steps:" line + + for i < len(lines) { + line := lines[i] + + // Skip empty lines + if strings.TrimSpace(line) == "" { + yaml.WriteString("\n") + i++ + continue + } + + // Add the line with proper indentation + yaml.WriteString(" " + line + "\n") + + // Check if this line starts a step with "- name:" or "- uses:" + trimmed := strings.TrimSpace(line) + isStepStart := strings.HasPrefix(trimmed, "- name:") || strings.HasPrefix(trimmed, "- uses:") + + if isStepStart && !insertedRuntime { + // This is the start of a step, check if it's a checkout step + isCheckoutStep := false + + // Look ahead to find "uses:" line with "checkout" + for j := i + 1; j < len(lines); j++ { + nextLine := lines[j] + nextTrimmed := strings.TrimSpace(nextLine) + + // Stop if we hit the next step + if strings.HasPrefix(nextTrimmed, "- name:") || strings.HasPrefix(nextTrimmed, "- uses:") { + break + } + + // Check if this is a uses line with checkout + if strings.Contains(nextTrimmed, "uses:") && strings.Contains(nextTrimmed, "checkout") { + isCheckoutStep = true + break + } + } + + if isCheckoutStep { + // This is a checkout step, copy all its lines until the next step + i++ + for i < len(lines) { + nextLine := lines[i] + nextTrimmed := strings.TrimSpace(nextLine) + + // Stop if we hit the next step + if strings.HasPrefix(nextTrimmed, "- name:") || strings.HasPrefix(nextTrimmed, "- uses:") { + break + } + + // Add the line + if nextTrimmed == "" { + yaml.WriteString("\n") + } else { + yaml.WriteString(" " + nextLine + "\n") + } + i++ + } + + // Now insert runtime steps after the checkout step + compilerYamlLog.Printf("Inserting %d runtime setup steps after checkout in custom steps", len(runtimeSetupSteps)) + for _, step := range runtimeSetupSteps { + for _, stepLine := range step { + yaml.WriteString(stepLine + "\n") + } + } + insertedRuntime = true + continue // Continue with the next iteration (i is already advanced) + } + } + + i++ + } +}