diff --git a/pkg/workflow/compiler_yaml_main_job_test.go b/pkg/workflow/compiler_yaml_main_job_test.go index 34d1e3cd96b..cf0ea09d5e1 100644 --- a/pkg/workflow/compiler_yaml_main_job_test.go +++ b/pkg/workflow/compiler_yaml_main_job_test.go @@ -378,6 +378,7 @@ func TestAddCustomStepsWithRuntimeInsertion(t *testing.T) { customSteps string runtimeSetupSteps []GitHubActionStep tools *ToolsConfig + ensureArcNodePath bool expectInOutput []string expectStepOrder []string notInOutput []string @@ -462,16 +463,19 @@ func TestAddCustomStepsWithRuntimeInsertion(t *testing.T) { {" - name: Setup Node.js", " uses: actions/setup-node@v4"}, {" - name: Setup Python", " uses: actions/setup-python@v5"}, }, - tools: &ToolsConfig{}, + tools: &ToolsConfig{}, + ensureArcNodePath: true, expectInOutput: []string{ "- name: Checkout", "- name: Setup Node.js", + "- name: Ensure Node.js is at daemon-visible path", "- name: Setup Python", "- name: Deploy", }, expectStepOrder: []string{ "Checkout", "Setup Node.js", + "Ensure Node.js is at daemon-visible path", "Setup Python", "Deploy", }, @@ -499,7 +503,7 @@ func TestAddCustomStepsWithRuntimeInsertion(t *testing.T) { compiler := NewCompiler() var yaml strings.Builder - compiler.addCustomStepsWithRuntimeInsertion(&yaml, tt.customSteps, tt.runtimeSetupSteps, tt.tools) + compiler.addCustomStepsWithRuntimeInsertion(&yaml, tt.customSteps, tt.runtimeSetupSteps, tt.tools, tt.ensureArcNodePath) result := yaml.String() for _, expected := range tt.expectInOutput { @@ -756,17 +760,19 @@ func TestGenerateMainJobStepsArcDindRedirectsToolCacheToRunnerTemp(t *testing.T) assert.NotContains(t, result, "/tmp/gh-aw/tool-cache") } -func TestGenerateMainJobStepsArcDindSkipsNodePathStepWhenRuntimeStepsDeferred(t *testing.T) { - // When custom steps contain a checkout action, needsCheckout is false and - // customStepsContainCheckout is true, so runtime steps (including setup-node) are - // deferred to after the custom checkout. In that case the Node relocation step must - // NOT be emitted here because setup-node hasn't run yet and `command -v node` may be - // empty or point to the wrong binary. +func TestGenerateMainJobStepsArcDindInsertsNodePathStepWhenRuntimeStepsDeferred(t *testing.T) { + // When runtime setup is deferred to after a custom checkout, ARC/DinD should still + // emit the Node relocation step immediately after Setup Node.js. compiler := NewCompiler() compiler.stepOrderTracker = NewStepOrderTracker() - customStepsWithCheckout := ` - name: Custom checkout - uses: actions/checkout@v4 + customStepsWithCheckout := `steps: + - name: Custom checkout + uses: actions/checkout@v4 + with: + fetch-depth: 0 + - name: Prepare context + run: echo ready ` data := &WorkflowData{ @@ -781,6 +787,9 @@ func TestGenerateMainJobStepsArcDindSkipsNodePathStepWhenRuntimeStepsDeferred(t }, CustomSteps: customStepsWithCheckout, ParsedTools: NewTools(nil), + Runtimes: map[string]any{ + "node": map[string]any{}, + }, } var yaml strings.Builder @@ -790,10 +799,225 @@ func TestGenerateMainJobStepsArcDindSkipsNodePathStepWhenRuntimeStepsDeferred(t result := yaml.String() // Tool cache redirect is always emitted on ARC/DinD regardless of checkout placement. assert.Contains(t, result, "- name: Redirect tool cache and install paths for ARC/DinD") - // Node relocation step must be suppressed when runtime steps are deferred. + assert.Contains(t, result, "- name: Setup Node.js") + assert.Contains(t, result, "- name: Ensure Node.js is at daemon-visible path") + + customCheckoutIndex := strings.Index(result, "- name: Custom checkout") + setupNodeIndex := strings.Index(result, "- name: Setup Node.js") + nodePathIndex := strings.Index(result, "- name: Ensure Node.js is at daemon-visible path") + prepareContextIndex := strings.Index(result, "- name: Prepare context") + assert.NotEqual(t, -1, customCheckoutIndex) + assert.NotEqual(t, -1, setupNodeIndex) + assert.NotEqual(t, -1, nodePathIndex) + assert.NotEqual(t, -1, prepareContextIndex) + assert.Greater(t, setupNodeIndex, customCheckoutIndex) + assert.Greater(t, nodePathIndex, setupNodeIndex) + assert.Greater(t, prepareContextIndex, nodePathIndex) + assert.Equal(t, 1, strings.Count(result, "- name: Ensure Node.js is at daemon-visible path")) +} + +func TestGenerateMainJobStepsArcDindDeferredNodePathStepPreservesRuntimeIfCondition(t *testing.T) { + compiler := NewCompiler() + compiler.stepOrderTracker = NewStepOrderTracker() + + customStepsWithCheckout := `steps: + - name: Custom checkout + uses: actions/checkout@v4 + - name: Prepare context + run: echo ready +` + + data := &WorkflowData{ + Name: "Test Workflow", + AI: "copilot", + MarkdownContent: "Test prompt", + EngineConfig: &EngineConfig{ + ID: "copilot", + }, + RunnerConfig: &RunnerConfig{ + Topology: RunnerTopologyArcDind, + }, + CustomSteps: customStepsWithCheckout, + ParsedTools: NewTools(nil), + Runtimes: map[string]any{ + "node": map[string]any{ + "if": "hashFiles('package.json') != ''", + }, + }, + } + + var yaml strings.Builder + err := compiler.generateMainJobSteps(&yaml, data) + require.NoError(t, err) + + result := yaml.String() + assert.Contains(t, result, "- name: Setup Node.js") + assert.Contains(t, result, "if: hashFiles('package.json') != ''") + + nodePathIndex := strings.Index(result, "- name: Ensure Node.js is at daemon-visible path") + require.NotEqual(t, -1, nodePathIndex) + afterNodePath := result[nodePathIndex:] + lines := strings.SplitN(afterNodePath, "\n", 4) + require.GreaterOrEqual(t, len(lines), 3) + assert.Equal(t, " if: hashFiles('package.json') != ''", lines[1]) + assert.Equal(t, " run: |", lines[2]) +} + +func TestGenerateMainJobStepsArcDindSkipsNodePathStepWithoutGeneratedNodeSetup(t *testing.T) { + compiler := NewCompiler() + compiler.stepOrderTracker = NewStepOrderTracker() + + customStepsWithCheckoutAndOwnedSetupNode := `steps: + - name: Custom checkout + uses: actions/checkout@v4 + - name: Setup Node + uses: actions/setup-node@v6 + with: + node-version: "20" + - name: Prepare context + run: echo ready +` + + data := &WorkflowData{ + Name: "Test Workflow", + AI: "copilot", + MarkdownContent: "Run npm --version", + EngineConfig: &EngineConfig{ + ID: "copilot", + }, + RunnerConfig: &RunnerConfig{ + Topology: RunnerTopologyArcDind, + }, + CustomSteps: customStepsWithCheckoutAndOwnedSetupNode, + ParsedTools: NewTools(nil), + Runtimes: map[string]any{ + "node": map[string]any{}, + }, + } + + var yaml strings.Builder + err := compiler.generateMainJobSteps(&yaml, data) + require.NoError(t, err) + + result := yaml.String() + // User-owned setup-node remains in custom steps; no generated Setup Node.js means no ARC relocation step. + assert.NotContains(t, result, "- name: Setup Node.js") assert.NotContains(t, result, "- name: Ensure Node.js is at daemon-visible path") } +func TestGenerateMainJobStepsNonArcDeferredNodeSetupDoesNotInsertNodePathStep(t *testing.T) { + compiler := NewCompiler() + compiler.stepOrderTracker = NewStepOrderTracker() + + customStepsWithCheckout := `steps: + - name: Custom checkout + uses: actions/checkout@v4 + - name: Prepare context + run: echo ready +` + + data := &WorkflowData{ + Name: "Test Workflow", + AI: "copilot", + MarkdownContent: "Run npm --version", + EngineConfig: &EngineConfig{ + ID: "copilot", + }, + CustomSteps: customStepsWithCheckout, + ParsedTools: NewTools(nil), + Runtimes: map[string]any{ + "node": map[string]any{}, + }, + } + + var yaml strings.Builder + err := compiler.generateMainJobSteps(&yaml, data) + require.NoError(t, err) + + result := yaml.String() + assert.Contains(t, result, "- name: Setup Node.js") + assert.NotContains(t, result, "- name: Ensure Node.js is at daemon-visible path") +} + +func TestGenerateMainJobStepsArcDindManagedCheckoutKeepsNodePathStep(t *testing.T) { + compiler := NewCompiler() + compiler.stepOrderTracker = NewStepOrderTracker() + + data := &WorkflowData{ + Name: "Test Workflow", + AI: "copilot", + MarkdownContent: "Run npm --version", + EngineConfig: &EngineConfig{ + ID: "copilot", + }, + RunnerConfig: &RunnerConfig{ + Topology: RunnerTopologyArcDind, + }, + ParsedTools: NewTools(nil), + Runtimes: map[string]any{ + "node": map[string]any{}, + }, + } + + var yaml strings.Builder + err := compiler.generateMainJobSteps(&yaml, data) + require.NoError(t, err) + + result := yaml.String() + redirectIndex := strings.Index(result, "- name: Redirect tool cache and install paths for ARC/DinD") + setupNodeIndex := strings.Index(result, "- name: Setup Node.js") + nodePathIndex := strings.Index(result, "- name: Ensure Node.js is at daemon-visible path") + assert.NotEqual(t, -1, redirectIndex) + assert.NotEqual(t, -1, setupNodeIndex) + assert.NotEqual(t, -1, nodePathIndex) + assert.Greater(t, setupNodeIndex, redirectIndex) + assert.Greater(t, nodePathIndex, setupNodeIndex) +} + +func TestGenerateMainJobStepsArcDindDeferredMultipleRuntimesInsertSingleNodePathStep(t *testing.T) { + compiler := NewCompiler() + compiler.stepOrderTracker = NewStepOrderTracker() + + customStepsWithCheckout := `steps: + - name: Custom checkout + uses: actions/checkout@v4 + - name: Prepare context + run: npm --version && python --version +` + + data := &WorkflowData{ + Name: "Test Workflow", + AI: "copilot", + MarkdownContent: "Test prompt", + EngineConfig: &EngineConfig{ + ID: "copilot", + }, + RunnerConfig: &RunnerConfig{ + Topology: RunnerTopologyArcDind, + }, + CustomSteps: customStepsWithCheckout, + ParsedTools: NewTools(nil), + Runtimes: map[string]any{ + "node": map[string]any{}, + "python": map[string]any{}, + }, + } + + var yaml strings.Builder + err := compiler.generateMainJobSteps(&yaml, data) + require.NoError(t, err) + + result := yaml.String() + setupNodeIndex := strings.Index(result, "- name: Setup Node.js") + nodePathIndex := strings.Index(result, "- name: Ensure Node.js is at daemon-visible path") + setupPythonIndex := strings.Index(result, "- name: Setup Python") + assert.NotEqual(t, -1, setupNodeIndex) + assert.NotEqual(t, -1, nodePathIndex) + assert.NotEqual(t, -1, setupPythonIndex) + assert.Greater(t, nodePathIndex, setupNodeIndex) + assert.Equal(t, 1, strings.Count(result, "- name: Ensure Node.js is at daemon-visible path")) +} + func TestGenerateMainJobStepsWithDevMode_GhAwRuntimeBuildsFromSource(t *testing.T) { originalRelease := IsRelease() t.Cleanup(func() { diff --git a/pkg/workflow/compiler_yaml_runtime_setup.go b/pkg/workflow/compiler_yaml_runtime_setup.go index adbadd6747d..047dab557c6 100644 --- a/pkg/workflow/compiler_yaml_runtime_setup.go +++ b/pkg/workflow/compiler_yaml_runtime_setup.go @@ -120,24 +120,7 @@ func (c *Compiler) emitRuntimeSetupPrelude(yaml *strings.Builder, data *Workflow // 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) - yaml.WriteByte('\n') - } - } - } - - // ARC/DinD: ensure Node.js is at a daemon-visible path. - // On ARC runners, setup-node may find a pre-cached node at the original tool cache - // (e.g. /home/runner/_work/_tool/node/...) which is NOT under RUNNER_TEMP and therefore - // not bind-mounted into the AWF container. This step copies node to the redirected - // tool cache if needed and sets GH_AW_NODE_BIN for the AWF entrypoint. - // Only emit when runtime steps (including setup-node) were already emitted above; - // when they are deferred to after a custom checkout, this step would run before - // setup-node and could relocate an absent or wrong node binary. - if isArcDindTopology(data) && runtimeStepsEmittedEarly { - c.generateArcDindNodePathStep(yaml) + c.emitRuntimeSetupSteps(yaml, runtimeSetupSteps, isArcDindTopology(data)) } } @@ -150,8 +133,11 @@ func (c *Compiler) generateArcDindToolCacheRedirectStep(yaml *strings.Builder) { yaml.WriteString(" echo \"GOPATH=${RUNNER_TEMP}/gh-aw/tool-cache/go\" >> \"$GITHUB_ENV\"\n") } -func (c *Compiler) generateArcDindNodePathStep(yaml *strings.Builder) { +func (c *Compiler) generateArcDindNodePathStep(yaml *strings.Builder, ifCondition string) { yaml.WriteString(" - name: Ensure Node.js is at daemon-visible path\n") + if ifCondition != "" { + fmt.Fprintf(yaml, " if: %s\n", ifCondition) + } yaml.WriteString(" run: |\n") yaml.WriteString(" NODE_BIN=\"$(command -v node)\"\n") yaml.WriteString(" NODE_PREFIX=\"$(dirname \"$(dirname \"$NODE_BIN\")\")\"\n") @@ -165,6 +151,31 @@ func (c *Compiler) generateArcDindNodePathStep(yaml *strings.Builder) { yaml.WriteString(" fi\n") } +func (c *Compiler) emitRuntimeSetupSteps(yaml *strings.Builder, runtimeSetupSteps []GitHubActionStep, ensureArcDindNodePath bool) { + nodePathStepEmitted := false + for _, step := range runtimeSetupSteps { + for _, line := range step { + yaml.WriteString(line) + yaml.WriteByte('\n') + } + if ensureArcDindNodePath && !nodePathStepEmitted && extractStepName(strings.Join(step, "\n")) == "Setup Node.js" { + c.generateArcDindNodePathStep(yaml, extractStepIfCondition(step)) + nodePathStepEmitted = true + } + } +} + +// extractStepIfCondition returns the unwrapped if: expression from a generated step. +// It returns empty string when no if: line is present and also when an if: line has no value. +func extractStepIfCondition(step GitHubActionStep) string { + for _, line := range step { + if after, ok := strings.CutPrefix(strings.TrimSpace(line), "if:"); ok { + return strings.TrimSpace(after) + } + } + return "" +} + func (c *Compiler) emitCustomSteps(yaml *strings.Builder, data *WorkflowData, customStepsContainCheckout bool, runtimeSetupSteps []GitHubActionStep) { // Add custom steps if present if data.CustomSteps == "" { @@ -182,7 +193,7 @@ func (c *Compiler) emitCustomSteps(yaml *strings.Builder, data *WorkflowData, cu // 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, customStepsToEmit, runtimeSetupSteps, data.ParsedTools) + c.addCustomStepsWithRuntimeInsertion(yaml, customStepsToEmit, runtimeSetupSteps, data.ParsedTools, isArcDindTopology(data)) } 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)) @@ -322,7 +333,7 @@ func (c *Compiler) addCustomStepsAsIs(yaml *strings.Builder, customSteps string) // addCustomStepsWithRuntimeInsertion adds custom steps and inserts runtime steps after the first checkout. // Like addCustomStepsAsIs it sanitizes any ${{ ... }} expressions found in run: fields before writing. -func (c *Compiler) addCustomStepsWithRuntimeInsertion(yaml *strings.Builder, customSteps string, runtimeSetupSteps []GitHubActionStep, tools *ToolsConfig) { +func (c *Compiler) addCustomStepsWithRuntimeInsertion(yaml *strings.Builder, customSteps string, runtimeSetupSteps []GitHubActionStep, tools *ToolsConfig, ensureArcDindNodePath bool) { customSteps = c.sanitizeAndWarnCustomSteps(customSteps) // Remove "steps:" line and adjust indentation lines := strings.Split(customSteps, "\n") @@ -393,11 +404,7 @@ func (c *Compiler) addCustomStepsWithRuntimeInsertion(yaml *strings.Builder, cus // 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") - } - } + c.emitRuntimeSetupSteps(yaml, runtimeSetupSteps, ensureArcDindNodePath) insertedRuntime = true continue // Continue with the next iteration (i is already advanced)