Fix deferred runtime setup for unnamed custom checkout steps

docs/adr/47545-yaml-parsed-checkout-detection-for-workflow-compiler.md

@@ -0,0 +1,48 @@
+# ADR-47545: YAML-Parsed Checkout Detection for Workflow Compiler
+
+**Date**: 2026-07-23
+**Status**: Draft
+**Deciders**: Unknown
+
+---
+
+### Context
+
+The workflow compiler inserts runtime setup steps (e.g., Node.js, ARC/DinD) immediately after the first `actions/checkout` step in user-defined custom steps. This deferral is a two-phase operation: a detection phase determines whether custom steps contain a checkout (triggering deferral), and an insertion phase locates the checkout step to insert runtime setup after it. The original implementation used substring string matching in both phases. However, the insertion phase looked ahead from `i + 1` lines, so when a checkout appeared as `- uses: actions/checkout@v6` (the unnamed shorthand), the action reference was on the current line — never examined by the look-ahead — causing the deferred runtime setup to be silently dropped from the generated workflow. Named checkout forms (`- name: Checkout\n  uses: actions/checkout@v6`) worked because the `uses:` key appeared on a subsequent line. Any workow using the unnamed shorthand was silently broken.
+
+### Decision
+
+We will replace the dual substring-based checkout matching with a single YAML-parsed helper (`findFirstCheckoutStepIndex`) that unmarshals the custom-steps YAML, iterates the parsed step list, and returns the zero-based index of the first step whose `uses` field exactly matches `actions/checkout` or starts with `actions/checkout@`. Both the `ContainsCheckout` detection function and the `addCustomStepsWithRuntimeInsertion` insertion function will share this helper, eliminating the detection/insertion mismatch. The insertion phase uses the pre-computed step index rather than per-line look-ahead to identify the target step during line-by-line YAML rendering.
+
+### Alternatives Considered
+
+#### Alternative 1: Fix the Look-Ahead in the Insertion Phase Only
+
+The minimal fix would be to also examine the current `- uses:` line (not just `i + 1` onwards) in the existing look-ahead loop inside `addCustomStepsWithRuntimeInsertion`. This would resolve the immediate bug without changing the detection architecture.
+
+Why not chosen: This fix would leave two independent and semantically inconsistent checkout detectors in the codebase — one using substring matching (detection phase) and one using look-ahead string scanning (insertion phase). Future checkout-form edge cases (e.g., quoted action references, whitespace variants) could reintroduce a mismatch. The substring detector also produced false positives on comments, `run:` field text, and action names containing "checkout" (as documented by the test cases changed in `permissions_parser_test.go`).
+
+#### Alternative 2: Parse YAML Once at a Higher Level and Pass a Shared Representation
+
+A more architecturally ambitious approach would parse the full custom-steps YAML once at the call site of `emitCustomSteps` and pass structured step data into both detection and insertion functions, eliminating re-parsing.
+
+Why not chosen: This approach requires refactoring multiple function signatures and callers that currently pass `customSteps` as a raw string throughout the compiler pipeline. The single shared `findFirstCheckoutStepIndex` helper achieves consistent detection at lower refactoring cost, since it is called once per compilation path and its result is threaded into both phases within the same call chain.
+
+### Consequences
+
+#### Positive
+- Checkout detection is now exact: YAML parsing rejects false positives from comments, `run:` field text (`echo "actions/checkout@..."`), and action names that merely contain "checkout" (e.g., `my-actions/checkout@...`).
+- Named and unnamed checkout forms (`- name: Checkout\n  uses:` vs `- uses:`) are handled equivalently, eliminating the behavioral asymmetry that caused the bug.
+- The first checkout step index is computed once and shared between detection and insertion, removing the structural possibility of the two phases diverging.
+
+#### Negative
+- The `go-yaml` package is now imported directly in `compiler_workflow_helpers.go`; previously this helper file had no YAML dependency. If the YAML library ever changes parse behavior, checkout detection will change with it.
+- Malformed custom-steps YAML (e.g., missing `steps:` key or a top-level syntax error) now causes `findFirstCheckoutStepIndex` to return `(0, false)`, meaning checkout is not detected and no deferral occurs. Previously the substring matcher would still detect checkout in malformed YAML. The practical impact is low: malformed YAML also fails later compilation stages.
+
+#### Neutral
+- The `isCheckoutActionReference` helper normalizes the `uses` value (trims whitespace, strips surrounding quotes, lowercases) before comparison, making the check robust to minor formatting variation without adding regex complexity.
+- Step boundary detection in the insertion loop is generalized from `- name:` or `- uses:` prefix checks to any `- ` prefix at the same indentation level, matching actual YAML list semantics more closely.
+
+---
+
+*ADR created by [adr-writer agent]. Review and finalize before changing status from Draft to Accepted.*

pkg/workflow/checkout_runtime_order_test.go

@@ -220,6 +220,126 @@ steps:
 	}
 }
 
+func TestCheckoutRuntimeOrderInCustomStepVariants(t *testing.T) {
+	tests := []struct {
+		name                 string
+		customSteps          string
+		orderedMarkers       []string
+		wantSetupNodeNames   int
+		wantSetupNodeActions int
+	}{
+		{
+			name: "unnamed checkout with options as first custom step",
+			customSteps: `  - uses: actions/checkout@v5
+    with:
+      fetch-depth: 0
+      persist-credentials: false
+  - name: Prepare context
+    run: echo ready`,
+			orderedMarkers: []string{
+				"uses: actions/checkout@",
+				"- name: Setup Node.js",
+				"- name: Prepare context",
+			},
+			wantSetupNodeNames:   1,
+			wantSetupNodeActions: 1,
+		},
+		{
+			name: "named checkout with options as first custom step",
+			customSteps: `  - name: Checkout repository
+    uses: actions/checkout@v5
+    with:
+      fetch-depth: 0
+      persist-credentials: false
+  - name: Prepare context
+    run: echo ready`,
+			orderedMarkers: []string{
+				"uses: actions/checkout@",
+				"- name: Setup Node.js",
+				"- name: Prepare context",
+			},
+			wantSetupNodeNames:   1,
+			wantSetupNodeActions: 1,
+		},
+		{
+			name: "checkout after deterministic step",
+			customSteps: `  - name: Prepare context
+    run: echo before
+  - uses: actions/checkout@v5
+    with:
+      persist-credentials: false
+  - name: After checkout
+    run: echo after`,
+			orderedMarkers: []string{
+				"- name: Prepare context",
+				"uses: actions/checkout@",
+				"- name: Setup Node.js",
+				"- name: After checkout",
+			},
+			wantSetupNodeNames:   1,
+			wantSetupNodeActions: 1,
+		},
+		{
+			name: "multiple checkout steps insert runtime once after first checkout",
+			customSteps: `  - name: Prepare context
+    run: echo before
+  - uses: actions/checkout@v5
+    with:
+      persist-credentials: false
+  - name: After first checkout
+    run: echo middle
+  - uses: actions/checkout@v5
+    with:
+      persist-credentials: false
+  - name: After second checkout
+    run: echo done`,
+			orderedMarkers: []string{
+				"- name: Prepare context",
+				"uses: actions/checkout@",
+				"- name: Setup Node.js",
+				"- name: After first checkout",
+				"uses: actions/checkout@",
+				"- name: After second checkout",
+			},
+			wantSetupNodeNames:   1,
+			wantSetupNodeActions: 1,
+		},
+		{
+			name: "customized runtime setup action is preserved without duplication",
+			customSteps: `  - uses: actions/checkout@v5
+    with:
+      persist-credentials: false
+  - name: Setup Node from file
+    uses: actions/setup-node@v5
+    with:
+      node-version-file: .nvmrc
+  - name: Prepare context
+    run: echo ready`,
+			orderedMarkers: []string{
+				"uses: actions/checkout@",
+				"- name: Setup Node from file",
+				"- name: Prepare context",
+			},
+			wantSetupNodeNames:   0,
+			wantSetupNodeActions: 1,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			agentJobSection := compileWorkflowAndGetAgentJobSection(t, tt.customSteps)
+			assertOrderedMarkers(t, agentJobSection, tt.orderedMarkers)
+
+			if got := strings.Count(agentJobSection, "- name: Setup Node.js"); got != tt.wantSetupNodeNames {
+				t.Fatalf("expected %d generated Setup Node.js steps, got %d\n%s", tt.wantSetupNodeNames, got, agentJobSection)
+			}
+			if got := strings.Count(agentJobSection, "uses: actions/setup-node@"); got != tt.wantSetupNodeActions {
+				t.Fatalf("expected %d setup-node actions, got %d\n%s", tt.wantSetupNodeActions, got, agentJobSection)
+			}
+		})
+	}
+}
+
 // TestCheckoutFirstWhenNoCustomSteps verifies that when there are no custom steps,
 // the automatic checkout is added first.
 func TestCheckoutFirstWhenNoCustomSteps(t *testing.T) {
@@ -356,3 +476,93 @@ Run node --version to check the Node.js version.
 	t.Logf("  3. %s", stepNames[2])
 	t.Logf("  4. %s", stepNames[3])
 }
+
+func compileWorkflowAndGetAgentJobSection(t *testing.T, customSteps string) string {
+	t.Helper()
+
+	workflowContent := `---
+on: workflow_dispatch
+engine: copilot
+runs-on: self-hosted
+permissions:
+  contents: read
+  issues: read
+  pull-requests: read
+steps:
+` + customSteps + `
+---
+
+# Test workflow with custom checkout steps
+`
+
+	tempDir, err := os.MkdirTemp("", "checkout-runtime-order-variant")
+	if err != nil {
+		t.Fatalf("Failed to create temp dir: %v", err)
+	}
+	defer os.RemoveAll(tempDir)
+
+	workflowsDir := filepath.Join(tempDir, constants.GetWorkflowDir())
+	if err := os.MkdirAll(workflowsDir, 0755); err != nil {
+		t.Fatalf("Failed to create workflows directory: %v", err)
+	}
+
+	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)
+	}
+
+	compiler := NewCompiler()
+	compiler.SetActionMode(ActionModeDev)
+	if err := compiler.CompileWorkflow(workflowPath); err != nil {
+		t.Fatalf("Failed to compile workflow: %v", err)
+	}
+
+	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)
+	}
+
+	return extractAgentJobSection(t, string(lockContent))
+}
+
+func extractAgentJobSection(t *testing.T, lockStr string) string {
+	t.Helper()
+
+	agentJobStart := strings.Index(lockStr, "  agent:")
+	if agentJobStart == -1 {
+		t.Fatal("Could not find agent job in compiled workflow")
+	}
+
+	remainingContent := lockStr[agentJobStart+10:]
+	nextJobStart := -1
+	lines := strings.Split(remainingContent, "\n")
+	for i, line := range lines {
+		if len(line) > 2 && line[0] == ' ' && line[1] == ' ' && line[2] != ' ' && line[2] != '\t' {
+			nextJobStart = 0
+			for j := range i {
+				nextJobStart += len(lines[j]) + 1
+			}
+			break
+		}
+	}
+
+	if nextJobStart == -1 {
+		return lockStr[agentJobStart:]
+	}
+
+	return lockStr[agentJobStart : agentJobStart+10+nextJobStart]
+}
+
+func assertOrderedMarkers(t *testing.T, content string, markers []string) {
+	t.Helper()
+
+	cursor := 0
+	for _, marker := range markers {
+		idx := strings.Index(content[cursor:], marker)
+		if idx == -1 {
+			t.Fatalf("marker %q not found after offset %d\n%s", marker, cursor, content)
+		}
+		cursor += idx + len(marker)
+	}
+}

pkg/workflow/compiler_workflow_helpers.go

@@ -4,33 +4,70 @@ import (
 	"path/filepath"
 	"strings"
 
+	"github.com/goccy/go-yaml"
+
 	"github.com/github/gh-aw/pkg/logger"
 )
 
 var compilerWorkflowHelpersLog = logger.New("workflow:compiler_workflow_helpers")
 
 // ContainsCheckout returns true if the given custom steps contain an actions/checkout step
 func ContainsCheckout(customSteps string) bool {
+	_, found := findFirstCheckoutStepIndex(customSteps)
+	return found
+}
+
+// findFirstCheckoutStepIndex returns the zero-based index of the first
+// actions/checkout step in customSteps and true, or (0, false) if none is found.
+//
+// It accepts both the wrapped form (with a "steps:" key) and the bare sequence
+// form (a YAML list) that older call sites may produce.  If the YAML cannot be
+// parsed in either form the function returns (0, false): the caller should not
+// attempt checkout-step insertion when the step list is unparseable.
+func findFirstCheckoutStepIndex(customSteps string) (int, bool) {
 	if customSteps == "" {
-		return false
+		return 0, false
 	}
 
-	// Look for actions/checkout usage patterns
-	checkoutPatterns := []string{
-		"actions/checkout@",
-		"uses: actions/checkout",
-		"- uses: actions/checkout",
+	// Try the wrapped form first: "steps:\n  - ...\n"
+	var wrapper struct {
+		Steps []map[string]any `yaml:"steps"`
+	}
+	if err := yaml.Unmarshal([]byte(customSteps), &wrapper); err == nil {
+		if len(wrapper.Steps) > 0 {
+			for i, step := range wrapper.Steps {
+				uses, ok := step["uses"].(string)
+				if ok && isCheckoutActionReference(uses) {
+					compilerWorkflowHelpersLog.Print("Detected actions/checkout in custom steps")
+					return i, true
+				}
+			}
+			return 0, false
+		}
 	}
 
-	lowerSteps := strings.ToLower(customSteps)
-	for _, pattern := range checkoutPatterns {
-		if strings.Contains(lowerSteps, strings.ToLower(pattern)) {
+	// Fall back to the bare sequence form: "- uses: ...\n"
+	var steps []map[string]any
+	if err := yaml.Unmarshal([]byte(customSteps), &steps); err != nil {
+		// Malformed YAML: we cannot safely determine a checkout step index.
+		return 0, false
+	}
+
+	for i, step := range steps {
+		uses, ok := step["uses"].(string)
+		if ok && isCheckoutActionReference(uses) {
 			compilerWorkflowHelpersLog.Print("Detected actions/checkout in custom steps")
-			return true
+			return i, true
 		}
 	}
 
-	return false
+	return 0, false
+}
+
+func isCheckoutActionReference(uses string) bool {
+	trimmed := strings.TrimSpace(strings.Trim(uses, `"'`))
+	return strings.EqualFold(trimmed, "actions/checkout") ||
+		strings.HasPrefix(strings.ToLower(trimmed), "actions/checkout@")
 }
 
 // GetWorkflowIDFromPath extracts the workflow ID from a markdown file path.

pkg/workflow/compiler_yaml_line_writer.go

@@ -72,6 +72,13 @@ func (s *yamlBlockScalarState) update(sourceLine string) bool {
 	return false
 }
 
+// IsInPayload reports whether the state machine is currently inside a block
+// scalar payload (i.e. the last consumed line was block-scalar content).
+// Unlike update, this is a pure read that does not advance the state.
+func (s *yamlBlockScalarState) IsInPayload() bool {
+	return s.active
+}
+
 // appendYAMLLine writes content to b with the given prefix.
 //
 //   - Blank content (empty or whitespace-only) is always written as a bare