From 02f9c98ab5761142c9e756894c1bda57afd9efaa Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 8 Apr 2026 05:00:29 +0000 Subject: [PATCH 1/4] Initial plan From 308a55abdf498f69b032783ac638760c4544d495 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 8 Apr 2026 05:27:46 +0000 Subject: [PATCH 2/4] feat: add pre-steps support for agent job (runs before checkout) Pre-steps allow users to mint short-lived tokens or perform other setup that must run in the same job as checkout. This avoids the GitHub Actions runner masked-value bug (v2.308+) where tokens minted in a separate job are silently dropped when passed via job outputs. Users can now reference pre-step outputs in checkout.token: pre-steps: - name: Mint token id: mint uses: some-org/token-action@v1 checkout: - repository: target-org/target-repo token: ${{ steps.mint.outputs.token }} Changes: - JSON schema: add pre-steps property - FrontmatterConfig: add PreSteps field - WorkflowData: add PreSteps field - compiler_orchestrator_workflow: add processAndMergePreSteps - compiler_yaml: add generatePreSteps function - compiler_yaml_main_job: emit pre-steps before checkout/app-token steps - strict_mode_steps_validation: include pre-steps in secret validation - compiler_presteps_test: add tests for pre-steps functionality Agent-Logs-Url: https://github.com/github/gh-aw/sessions/86c17acc-cd92-4ba9-9c18-c2f101070c71 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- pkg/parser/schemas/main_workflow_schema.json | 35 +++ .../compiler_orchestrator_workflow.go | 42 +++ pkg/workflow/compiler_presteps_test.go | 288 ++++++++++++++++++ pkg/workflow/compiler_types.go | 1 + pkg/workflow/compiler_yaml.go | 25 ++ pkg/workflow/compiler_yaml_main_job.go | 8 + pkg/workflow/frontmatter_types.go | 4 + pkg/workflow/strict_mode_steps_validation.go | 4 +- 8 files changed, 405 insertions(+), 2 deletions(-) create mode 100644 pkg/workflow/compiler_presteps_test.go diff --git a/pkg/parser/schemas/main_workflow_schema.json b/pkg/parser/schemas/main_workflow_schema.json index 9b47e5a6ba..d15163ed5f 100644 --- a/pkg/parser/schemas/main_workflow_schema.json +++ b/pkg/parser/schemas/main_workflow_schema.json @@ -3317,6 +3317,41 @@ } ] }, + "pre-steps": { + "description": "Custom workflow steps to run at the very beginning of the agent job, before checkout and any other built-in steps. Use pre-steps to mint short-lived tokens or perform any setup that must happen before the repository is checked out. Step outputs are available via ${{ steps..outputs. }} and can be referenced in checkout.token to avoid masked-value cross-job-boundary issues.", + "oneOf": [ + { + "type": "object", + "additionalProperties": true + }, + { + "type": "array", + "items": { + "oneOf": [ + { + "type": "string" + }, + { + "type": "object", + "additionalProperties": true + } + ] + }, + "examples": [ + [ + { + "name": "Mint short-lived token", + "id": "mint", + "uses": "some-org/token-minting-action@v1", + "with": { + "scope": "target-org/target-repo" + } + } + ] + ] + } + ] + }, "post-steps": { "description": "Custom workflow steps to run after AI execution", "oneOf": [ diff --git a/pkg/workflow/compiler_orchestrator_workflow.go b/pkg/workflow/compiler_orchestrator_workflow.go index 6e43cc1f94..da7f6243a5 100644 --- a/pkg/workflow/compiler_orchestrator_workflow.go +++ b/pkg/workflow/compiler_orchestrator_workflow.go @@ -147,6 +147,9 @@ func (c *Compiler) ParseWorkflowFile(markdownPath string) (*WorkflowData, error) // Process and merge custom steps with imported steps c.processAndMergeSteps(result.Frontmatter, workflowData, engineSetup.importsResult) + // Process and merge pre-steps + c.processAndMergePreSteps(result.Frontmatter, workflowData) + // Process and merge post-steps c.processAndMergePostSteps(result.Frontmatter, workflowData) @@ -493,6 +496,45 @@ func (c *Compiler) processAndMergeSteps(frontmatter map[string]any, workflowData } } +// processAndMergePreSteps handles the processing of pre-steps with action pinning. +// Pre-steps run at the very beginning of the agent job, before checkout and any other +// built-in steps, allowing users to mint tokens or perform other setup that must happen +// before the repository is checked out. +func (c *Compiler) processAndMergePreSteps(frontmatter map[string]any, workflowData *WorkflowData) { + orchestratorWorkflowLog.Print("Processing pre-steps") + + workflowData.PreSteps = c.extractTopLevelYAMLSection(frontmatter, "pre-steps") + + // Apply action pinning to pre-steps if any + if workflowData.PreSteps != "" { + var preStepsWrapper map[string]any + if err := yaml.Unmarshal([]byte(workflowData.PreSteps), &preStepsWrapper); err == nil { + if preStepsVal, hasPreSteps := preStepsWrapper["pre-steps"]; hasPreSteps { + if preSteps, ok := preStepsVal.([]any); ok { + // Convert to typed steps for action pinning + typedPreSteps, err := SliceToSteps(preSteps) + if err != nil { + orchestratorWorkflowLog.Printf("Failed to convert pre-steps to typed steps: %v", err) + } else { + // Apply action pinning to pre-steps using type-safe version + typedPreSteps = ApplyActionPinsToTypedSteps(typedPreSteps, workflowData) + // Convert back to []any for YAML marshaling + preSteps = StepsToSlice(typedPreSteps) + } + + // Convert back to YAML with "pre-steps:" wrapper + stepsWrapper := map[string]any{"pre-steps": preSteps} + stepsYAML, err := yaml.Marshal(stepsWrapper) + if err == nil { + // Remove quotes from uses values with version comments + workflowData.PreSteps = unquoteUsesWithComments(string(stepsYAML)) + } + } + } + } + } +} + // processAndMergePostSteps handles the processing of post-steps with action pinning func (c *Compiler) processAndMergePostSteps(frontmatter map[string]any, workflowData *WorkflowData) { orchestratorWorkflowLog.Print("Processing post-steps") diff --git a/pkg/workflow/compiler_presteps_test.go b/pkg/workflow/compiler_presteps_test.go new file mode 100644 index 0000000000..e1c14fee3b --- /dev/null +++ b/pkg/workflow/compiler_presteps_test.go @@ -0,0 +1,288 @@ +//go:build !integration + +package workflow + +import ( + "os" + "path/filepath" + "strings" + "testing" + + "github.com/github/gh-aw/pkg/testutil" +) + +// TestPreStepsGeneration verifies that pre-steps are emitted before checkout and all +// other built-in steps in the agent job. +func TestPreStepsGeneration(t *testing.T) { + tmpDir := testutil.TempDir(t, "pre-steps-test") + + testContent := `--- +on: push +permissions: + contents: read + issues: read + pull-requests: read +tools: + github: + allowed: [list_issues] +pre-steps: + - name: Mint short-lived token + id: mint + uses: some-org/token-minting-action@v1 + with: + scope: target-org/target-repo +steps: + - name: Custom Setup Step + run: echo "Custom setup" +post-steps: + - name: Post AI Step + run: echo "This runs after AI" +engine: claude +strict: false +--- + +# Test Pre-Steps Workflow + +This workflow tests the pre-steps functionality. +` + + testFile := filepath.Join(tmpDir, "test-pre-steps.md") + if err := os.WriteFile(testFile, []byte(testContent), 0644); err != nil { + t.Fatal(err) + } + + compiler := NewCompiler() + + if err := compiler.CompileWorkflow(testFile); err != nil { + t.Fatalf("Unexpected error compiling workflow with pre-steps: %v", err) + } + + lockFile := filepath.Join(tmpDir, "test-pre-steps.lock.yml") + content, err := os.ReadFile(lockFile) + if err != nil { + t.Fatalf("Failed to read generated lock file: %v", err) + } + + lockContent := string(content) + + // Verify all three step types appear (check name value, not "- name:" prefix + // since steps with an id field have id: first in the YAML output) + if !strings.Contains(lockContent, "name: Mint short-lived token") { + t.Error("Expected pre-step 'Mint short-lived token' to be in generated workflow") + } + if !strings.Contains(lockContent, "name: Custom Setup Step") { + t.Error("Expected custom step 'Custom Setup Step' to be in generated workflow") + } + if !strings.Contains(lockContent, "name: Post AI Step") { + t.Error("Expected post-step 'Post AI Step' to be in generated workflow") + } + + // Pre-steps must appear before checkout, custom steps, and AI execution + preStepIndex := indexInNonCommentLines(lockContent, "name: Mint short-lived token") + checkoutIndex := indexInNonCommentLines(lockContent, "- name: Checkout repository") + customStepIndex := indexInNonCommentLines(lockContent, "- name: Custom Setup Step") + aiStepIndex := indexInNonCommentLines(lockContent, "- name: Execute Claude Code CLI") + postStepIndex := indexInNonCommentLines(lockContent, "- name: Post AI Step") + + if preStepIndex == -1 { + t.Fatal("Could not find pre-step in generated workflow") + } + if checkoutIndex == -1 { + t.Fatal("Could not find checkout step in generated workflow") + } + if customStepIndex == -1 { + t.Fatal("Could not find custom step in generated workflow") + } + if aiStepIndex == -1 { + t.Fatal("Could not find AI execution step in generated workflow") + } + if postStepIndex == -1 { + t.Fatal("Could not find post-step in generated workflow") + } + + if preStepIndex >= checkoutIndex { + t.Errorf("Pre-step (%d) should appear before checkout step (%d)", preStepIndex, checkoutIndex) + } + if preStepIndex >= customStepIndex { + t.Errorf("Pre-step (%d) should appear before custom step (%d)", preStepIndex, customStepIndex) + } + if preStepIndex >= aiStepIndex { + t.Errorf("Pre-step (%d) should appear before AI execution step (%d)", preStepIndex, aiStepIndex) + } + if postStepIndex <= aiStepIndex { + t.Errorf("Post-step (%d) should appear after AI execution step (%d)", postStepIndex, aiStepIndex) + } + + t.Logf("Step order verified: pre-step(%d) < checkout(%d) < custom(%d) < AI(%d) < post(%d)", + preStepIndex, checkoutIndex, customStepIndex, aiStepIndex, postStepIndex) +} + +// TestPreStepsTokenAvailableForCheckout verifies that a token minted in a pre-step +// can be referenced in checkout.token via a steps expression, avoiding the cross-job +// masked-value issue. +func TestPreStepsTokenAvailableForCheckout(t *testing.T) { + tmpDir := testutil.TempDir(t, "pre-steps-token-test") + + testContent := `--- +on: workflow_dispatch +permissions: + contents: read + id-token: write +pre-steps: + - name: Mint token + id: mint + uses: some-org/token-action@v1 + with: + scope: target-org/target-repo +checkout: + - repository: target-org/target-repo + path: target + token: ${{ steps.mint.outputs.token }} + current: false + - path: . +engine: claude +strict: false +--- + +Read a file from the checked-out repo. +` + + testFile := filepath.Join(tmpDir, "test-pre-steps-token.md") + if err := os.WriteFile(testFile, []byte(testContent), 0644); err != nil { + t.Fatal(err) + } + + compiler := NewCompiler() + + if err := compiler.CompileWorkflow(testFile); err != nil { + t.Fatalf("Unexpected error compiling workflow: %v", err) + } + + lockFile := filepath.Join(tmpDir, "test-pre-steps-token.lock.yml") + content, err := os.ReadFile(lockFile) + if err != nil { + t.Fatalf("Failed to read generated lock file: %v", err) + } + + lockContent := string(content) + + // The minting step must appear in the agent job + agentJobSection := extractJobSection(lockContent, "agent") + if agentJobSection == "" { + t.Fatal("Agent job section not found in generated workflow") + } + + if !strings.Contains(agentJobSection, "name: Mint token") { + t.Error("Expected pre-step 'Mint token' to be in the agent job") + } + + // The token reference must appear in the checkout step + if !strings.Contains(agentJobSection, "steps.mint.outputs.token") { + t.Error("Expected steps.mint.outputs.token reference in agent job checkout step") + } + + // The pre-step must appear before the checkout step + mintIndex := indexInNonCommentLines(agentJobSection, "name: Mint token") + checkoutIndex := indexInNonCommentLines(agentJobSection, "- name: Checkout target-org/target-repo into target") + if mintIndex == -1 { + t.Fatal("Could not find mint step in agent job") + } + if checkoutIndex == -1 { + t.Fatal("Could not find cross-repo checkout step in agent job") + } + if mintIndex >= checkoutIndex { + t.Errorf("Pre-step mint (%d) should appear before cross-repo checkout (%d)", mintIndex, checkoutIndex) + } +} + +// TestPreStepsOnly verifies that a workflow with only pre-steps (no custom steps or post-steps) +// compiles correctly. +func TestPreStepsOnly(t *testing.T) { + tmpDir := testutil.TempDir(t, "pre-steps-only-test") + + testContent := `--- +on: issues +permissions: + contents: read + issues: read +pre-steps: + - name: Only Pre Step + run: echo "This runs before checkout" +engine: claude +strict: false +--- + +# Test Pre-Steps Only Workflow + +This workflow tests pre-steps without custom steps or post-steps. +` + + testFile := filepath.Join(tmpDir, "test-pre-steps-only.md") + if err := os.WriteFile(testFile, []byte(testContent), 0644); err != nil { + t.Fatal(err) + } + + compiler := NewCompiler() + + if err := compiler.CompileWorkflow(testFile); err != nil { + t.Fatalf("Unexpected error compiling workflow with pre-steps only: %v", err) + } + + lockFile := filepath.Join(tmpDir, "test-pre-steps-only.lock.yml") + content, err := os.ReadFile(lockFile) + if err != nil { + t.Fatalf("Failed to read generated lock file: %v", err) + } + + lockContent := string(content) + + if !strings.Contains(lockContent, "- name: Only Pre Step") { + t.Error("Expected pre-step 'Only Pre Step' to be in generated workflow") + } + + // Default checkout must still be present and after the pre-step + preStepIndex := indexInNonCommentLines(lockContent, "- name: Only Pre Step") + checkoutIndex := indexInNonCommentLines(lockContent, "- name: Checkout repository") + aiStepIndex := indexInNonCommentLines(lockContent, "- name: Execute Claude Code CLI") + + if preStepIndex == -1 { + t.Fatal("Could not find pre-step in generated workflow") + } + if checkoutIndex == -1 { + t.Error("Expected default checkout step to still be present") + } + if aiStepIndex == -1 { + t.Fatal("Could not find AI execution step in generated workflow") + } + + if checkoutIndex != -1 && preStepIndex >= checkoutIndex { + t.Errorf("Pre-step (%d) should appear before checkout step (%d)", preStepIndex, checkoutIndex) + } + if preStepIndex >= aiStepIndex { + t.Errorf("Pre-step (%d) should appear before AI execution step (%d)", preStepIndex, aiStepIndex) + } +} + +// TestPreStepsSecretsValidation verifies that secrets in pre-steps trigger the same +// strict-mode error and non-strict warning as secrets in steps and post-steps. +func TestPreStepsSecretsValidation(t *testing.T) { + compiler := NewCompiler() + compiler.strictMode = true + + frontmatter := map[string]any{ + "pre-steps": []any{ + map[string]any{ + "name": "Use secret in pre-step", + "run": "echo ${{ secrets.MY_SECRET }}", + }, + }, + } + + err := compiler.validateStepsSecrets(frontmatter) + if err == nil { + t.Error("Expected strict-mode error for secrets in pre-steps but got nil") + } + if !strings.Contains(err.Error(), "pre-steps") { + t.Errorf("Expected error to mention 'pre-steps', got: %v", err) + } +} diff --git a/pkg/workflow/compiler_types.go b/pkg/workflow/compiler_types.go index dd9c1ad6e4..0245de556d 100644 --- a/pkg/workflow/compiler_types.go +++ b/pkg/workflow/compiler_types.go @@ -424,6 +424,7 @@ type WorkflowData struct { If string TimeoutMinutes string CustomSteps string + PreSteps string // steps to run at the very start of the agent job, before checkout PostSteps string // steps to run after AI execution RunsOn string RunsOnSlim string // runner override for all framework/generated jobs (activation, safe-outputs, unlock, etc.) diff --git a/pkg/workflow/compiler_yaml.go b/pkg/workflow/compiler_yaml.go index c4d56475ba..1337e0300d 100644 --- a/pkg/workflow/compiler_yaml.go +++ b/pkg/workflow/compiler_yaml.go @@ -603,6 +603,31 @@ func writePromptBashStep(yaml *strings.Builder, name, script string) { fmt.Fprintf(yaml, " run: bash ${RUNNER_TEMP}/gh-aw/actions/%s\n", script) } +func (c *Compiler) generatePreSteps(yaml *strings.Builder, data *WorkflowData) { + if data.PreSteps != "" { + // Remove "pre-steps:" line and adjust indentation, similar to PostSteps processing + lines := strings.Split(data.PreSteps, "\n") + if len(lines) > 1 { + for _, line := range lines[1:] { + // Trim trailing whitespace + trimmed := strings.TrimRight(line, " ") + // Skip empty lines + if strings.TrimSpace(trimmed) == "" { + yaml.WriteString("\n") + continue + } + // Steps need 6-space indentation ( - name:) + // Nested properties need 8-space indentation ( run:) + if strings.HasPrefix(line, " ") { + yaml.WriteString(" " + line[2:] + "\n") + } else { + yaml.WriteString(" " + line + "\n") + } + } + } + } +} + func (c *Compiler) generatePostSteps(yaml *strings.Builder, data *WorkflowData) { if data.PostSteps != "" { // Remove "post-steps:" line and adjust indentation, similar to CustomSteps processing diff --git a/pkg/workflow/compiler_yaml_main_job.go b/pkg/workflow/compiler_yaml_main_job.go index 392040f3b6..13876546e7 100644 --- a/pkg/workflow/compiler_yaml_main_job.go +++ b/pkg/workflow/compiler_yaml_main_job.go @@ -20,6 +20,14 @@ func (c *Compiler) generateMainJobSteps(yaml *strings.Builder, data *WorkflowDat yaml.WriteString(generateOTLPHeadersMaskStep()) } + // Add pre-steps before checkout and any other built-in steps. + // This allows users to mint short-lived tokens (via custom actions) in the same + // job as checkout, so the tokens are never dropped by the GitHub Actions runner's + // add-mask behaviour that silently suppresses masked values across job boundaries. + // Step outputs are available as ${{ steps..outputs. }} and can be + // referenced directly in checkout.token. + c.generatePreSteps(yaml, data) + // Determine if we need to add a checkout step needsCheckout := c.shouldAddCheckoutStep(data) compilerYamlLog.Printf("Checkout step needed: %t", needsCheckout) diff --git a/pkg/workflow/frontmatter_types.go b/pkg/workflow/frontmatter_types.go index e0d8a38f0f..0bd65c9f2a 100644 --- a/pkg/workflow/frontmatter_types.go +++ b/pkg/workflow/frontmatter_types.go @@ -187,6 +187,7 @@ type FrontmatterConfig struct { RunsOn string `json:"runs-on,omitempty"` RunsOnSlim string `json:"runs-on-slim,omitempty"` // Runner for all framework/generated jobs (activation, safe-outputs, unlock, etc.) RunName string `json:"run-name,omitempty"` + PreSteps []any `json:"pre-steps,omitempty"` // Pre-workflow steps (run before checkout) Steps []any `json:"steps,omitempty"` // Custom workflow steps PostSteps []any `json:"post-steps,omitempty"` // Post-workflow steps Environment map[string]any `json:"environment,omitempty"` // GitHub environment @@ -666,6 +667,9 @@ func (fc *FrontmatterConfig) ToMap() map[string]any { if fc.RunName != "" { result["run-name"] = fc.RunName } + if fc.PreSteps != nil { + result["pre-steps"] = fc.PreSteps + } if fc.Steps != nil { result["steps"] = fc.Steps } diff --git a/pkg/workflow/strict_mode_steps_validation.go b/pkg/workflow/strict_mode_steps_validation.go index 815758e59c..d3fb14c7f6 100644 --- a/pkg/workflow/strict_mode_steps_validation.go +++ b/pkg/workflow/strict_mode_steps_validation.go @@ -19,13 +19,13 @@ import ( "github.com/github/gh-aw/pkg/sliceutil" ) -// validateStepsSecrets checks both the "steps" and "post-steps" frontmatter sections +// validateStepsSecrets checks the "pre-steps", "steps", and "post-steps" frontmatter sections // for secrets expressions (e.g. ${{ secrets.MY_SECRET }}). // // In strict mode the presence of any such expression is treated as an error. // In non-strict mode a warning is emitted instead. func (c *Compiler) validateStepsSecrets(frontmatter map[string]any) error { - for _, sectionName := range []string{"steps", "post-steps"} { + for _, sectionName := range []string{"pre-steps", "steps", "post-steps"} { if err := c.validateStepsSectionSecrets(frontmatter, sectionName); err != nil { return err } From a511f6042f077a37b91bf03c4bc7e3392402e9a3 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 8 Apr 2026 05:53:30 +0000 Subject: [PATCH 3/4] feat: make pre-steps work for imported agentic workflows; fix review comments - Add preStepsBuilder to importAccumulator in import_field_extractor.go and extract pre-steps from imported workflow files - Add MergedPreSteps field to ImportsResult in import_processor.go - Update processAndMergePreSteps to accept importsResult and merge imported pre-steps before the main workflow's pre-steps - Update processAndMergePostSteps to accept importsResult and merge imported post-steps after the main workflow's post-steps (fixing a pre-existing gap where MergedPostSteps was built but never consumed) - Fix comment wording in compiler_yaml_main_job.go per reviewer suggestion - Use pinned SHAs (40-char) in test fixtures to avoid network calls - Add unit tests for processAndMergePreSteps and imported post-steps Agent-Logs-Url: https://github.com/github/gh-aw/sessions/088642a3-c500-49a7-a249-a7db5353583e Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- pkg/parser/import_field_extractor.go | 10 +- pkg/parser/import_processor.go | 1 + .../compiler_orchestrator_workflow.go | 150 +++++++++++------- .../compiler_orchestrator_workflow_test.go | 94 ++++++++++- pkg/workflow/compiler_presteps_test.go | 4 +- pkg/workflow/compiler_yaml_main_job.go | 5 +- 6 files changed, 201 insertions(+), 63 deletions(-) diff --git a/pkg/parser/import_field_extractor.go b/pkg/parser/import_field_extractor.go index b051e0a698..98b5d7fdaf 100644 --- a/pkg/parser/import_field_extractor.go +++ b/pkg/parser/import_field_extractor.go @@ -23,6 +23,7 @@ type importAccumulator struct { importPaths []string // Import paths for runtime-import macro generation stepsBuilder strings.Builder copilotSetupStepsBuilder strings.Builder // Steps from copilot-setup-steps.yml (inserted at start) + preStepsBuilder strings.Builder runtimesBuilder strings.Builder servicesBuilder strings.Builder networkBuilder strings.Builder @@ -72,7 +73,7 @@ func newImportAccumulator() *importAccumulator { // extractAllImportFields extracts all frontmatter fields from a single imported file // and accumulates the results. Handles tools, engines, mcp-servers, safe-outputs, // mcp-scripts, steps, runtimes, services, network, permissions, secret-masking, bots, -// skip-roles, skip-bots, post-steps, labels, cache, and features. +// skip-roles, skip-bots, pre-steps, post-steps, labels, cache, and features. func (acc *importAccumulator) extractAllImportFields(content []byte, item importQueueItem, visited map[string]bool) error { log.Printf("Extracting all import fields: path=%s, section=%s, inputs=%d, content_size=%d bytes", item.fullPath, item.sectionName, len(item.inputs), len(content)) @@ -310,6 +311,12 @@ func (acc *importAccumulator) extractAllImportFields(content []byte, item import } } + // Extract pre-steps from imported file (prepend in order) + preStepsContent, err := extractYAMLFieldFromMap(fm, "pre-steps") + if err == nil && preStepsContent != "" { + acc.preStepsBuilder.WriteString(preStepsContent + "\n") + } + // Extract post-steps from imported file (append in order) postStepsContent, err := extractYAMLFieldFromMap(fm, "post-steps") if err == nil && postStepsContent != "" { @@ -408,6 +415,7 @@ func (acc *importAccumulator) toImportsResult(topologicalOrder []string) *Import ImportPaths: acc.importPaths, MergedSteps: acc.stepsBuilder.String(), CopilotSetupSteps: acc.copilotSetupStepsBuilder.String(), + MergedPreSteps: acc.preStepsBuilder.String(), MergedRuntimes: acc.runtimesBuilder.String(), MergedRunInstallScripts: acc.runInstallScripts, MergedServices: acc.servicesBuilder.String(), diff --git a/pkg/parser/import_processor.go b/pkg/parser/import_processor.go index f1ab1ba9e2..31bb85a6f9 100644 --- a/pkg/parser/import_processor.go +++ b/pkg/parser/import_processor.go @@ -23,6 +23,7 @@ type ImportsResult struct { ImportPaths []string // List of import file paths for runtime-import macro generation (replaces MergedMarkdown) MergedSteps string // Merged steps configuration from all imports (excluding copilot-setup-steps) CopilotSetupSteps string // Steps from copilot-setup-steps.yml (inserted at start) + MergedPreSteps string // Merged pre-steps configuration from all imports (prepended in order) MergedRuntimes string // Merged runtimes configuration from all imports MergedRunInstallScripts bool // true if any imported workflow sets run-install-scripts: true (global or node-level) MergedServices string // Merged services configuration from all imports diff --git a/pkg/workflow/compiler_orchestrator_workflow.go b/pkg/workflow/compiler_orchestrator_workflow.go index da7f6243a5..fb16db9f8e 100644 --- a/pkg/workflow/compiler_orchestrator_workflow.go +++ b/pkg/workflow/compiler_orchestrator_workflow.go @@ -148,10 +148,10 @@ func (c *Compiler) ParseWorkflowFile(markdownPath string) (*WorkflowData, error) c.processAndMergeSteps(result.Frontmatter, workflowData, engineSetup.importsResult) // Process and merge pre-steps - c.processAndMergePreSteps(result.Frontmatter, workflowData) + c.processAndMergePreSteps(result.Frontmatter, workflowData, engineSetup.importsResult) // Process and merge post-steps - c.processAndMergePostSteps(result.Frontmatter, workflowData) + c.processAndMergePostSteps(result.Frontmatter, workflowData, engineSetup.importsResult) // Process and merge services c.processAndMergeServices(result.Frontmatter, workflowData, engineSetup.importsResult) @@ -496,79 +496,117 @@ func (c *Compiler) processAndMergeSteps(frontmatter map[string]any, workflowData } } -// processAndMergePreSteps handles the processing of pre-steps with action pinning. -// Pre-steps run at the very beginning of the agent job, before checkout and any other +// processAndMergePreSteps handles the processing and merging of pre-steps with action pinning. +// Pre-steps run at the very beginning of the agent job, before checkout and the subsequent // built-in steps, allowing users to mint tokens or perform other setup that must happen -// before the repository is checked out. -func (c *Compiler) processAndMergePreSteps(frontmatter map[string]any, workflowData *WorkflowData) { - orchestratorWorkflowLog.Print("Processing pre-steps") - - workflowData.PreSteps = c.extractTopLevelYAMLSection(frontmatter, "pre-steps") - - // Apply action pinning to pre-steps if any - if workflowData.PreSteps != "" { - var preStepsWrapper map[string]any - if err := yaml.Unmarshal([]byte(workflowData.PreSteps), &preStepsWrapper); err == nil { - if preStepsVal, hasPreSteps := preStepsWrapper["pre-steps"]; hasPreSteps { - if preSteps, ok := preStepsVal.([]any); ok { - // Convert to typed steps for action pinning - typedPreSteps, err := SliceToSteps(preSteps) +// before the repository is checked out. Imported pre-steps are merged before the main +// workflow's pre-steps so that the main workflow can override or extend the imports. +func (c *Compiler) processAndMergePreSteps(frontmatter map[string]any, workflowData *WorkflowData, importsResult *parser.ImportsResult) { + orchestratorWorkflowLog.Print("Processing and merging pre-steps") + + mainPreStepsYAML := c.extractTopLevelYAMLSection(frontmatter, "pre-steps") + + // Parse imported pre-steps if present (these go before the main workflow's pre-steps) + var importedPreSteps []any + if importsResult.MergedPreSteps != "" { + if err := yaml.Unmarshal([]byte(importsResult.MergedPreSteps), &importedPreSteps); err == nil { + typedImported, err := SliceToSteps(importedPreSteps) + if err != nil { + orchestratorWorkflowLog.Printf("Failed to convert imported pre-steps to typed steps: %v", err) + } else { + typedImported = ApplyActionPinsToTypedSteps(typedImported, workflowData) + importedPreSteps = StepsToSlice(typedImported) + } + } + } + + // Parse main workflow pre-steps if present + var mainPreSteps []any + if mainPreStepsYAML != "" { + var mainWrapper map[string]any + if err := yaml.Unmarshal([]byte(mainPreStepsYAML), &mainWrapper); err == nil { + if mainVal, ok := mainWrapper["pre-steps"]; ok { + if steps, ok := mainVal.([]any); ok { + mainPreSteps = steps + typedMain, err := SliceToSteps(mainPreSteps) if err != nil { - orchestratorWorkflowLog.Printf("Failed to convert pre-steps to typed steps: %v", err) + orchestratorWorkflowLog.Printf("Failed to convert main pre-steps to typed steps: %v", err) } else { - // Apply action pinning to pre-steps using type-safe version - typedPreSteps = ApplyActionPinsToTypedSteps(typedPreSteps, workflowData) - // Convert back to []any for YAML marshaling - preSteps = StepsToSlice(typedPreSteps) - } - - // Convert back to YAML with "pre-steps:" wrapper - stepsWrapper := map[string]any{"pre-steps": preSteps} - stepsYAML, err := yaml.Marshal(stepsWrapper) - if err == nil { - // Remove quotes from uses values with version comments - workflowData.PreSteps = unquoteUsesWithComments(string(stepsYAML)) + typedMain = ApplyActionPinsToTypedSteps(typedMain, workflowData) + mainPreSteps = StepsToSlice(typedMain) } } } } } + + // Merge in order: imported pre-steps first, then main workflow's pre-steps + var allPreSteps []any + if len(importedPreSteps) > 0 || len(mainPreSteps) > 0 { + allPreSteps = append(allPreSteps, importedPreSteps...) + allPreSteps = append(allPreSteps, mainPreSteps...) + + stepsWrapper := map[string]any{"pre-steps": allPreSteps} + stepsYAML, err := yaml.Marshal(stepsWrapper) + if err == nil { + workflowData.PreSteps = unquoteUsesWithComments(string(stepsYAML)) + } + } } -// processAndMergePostSteps handles the processing of post-steps with action pinning -func (c *Compiler) processAndMergePostSteps(frontmatter map[string]any, workflowData *WorkflowData) { - orchestratorWorkflowLog.Print("Processing post-steps") +// processAndMergePostSteps handles the processing and merging of post-steps with action pinning. +// Imported post-steps are appended after the main workflow's post-steps. +func (c *Compiler) processAndMergePostSteps(frontmatter map[string]any, workflowData *WorkflowData, importsResult *parser.ImportsResult) { + orchestratorWorkflowLog.Print("Processing and merging post-steps") - workflowData.PostSteps = c.extractTopLevelYAMLSection(frontmatter, "post-steps") + mainPostStepsYAML := c.extractTopLevelYAMLSection(frontmatter, "post-steps") - // Apply action pinning to post-steps if any - if workflowData.PostSteps != "" { - var postStepsWrapper map[string]any - if err := yaml.Unmarshal([]byte(workflowData.PostSteps), &postStepsWrapper); err == nil { - if postStepsVal, hasPostSteps := postStepsWrapper["post-steps"]; hasPostSteps { - if postSteps, ok := postStepsVal.([]any); ok { - // Convert to typed steps for action pinning - typedPostSteps, err := SliceToSteps(postSteps) + // Parse imported post-steps if present (these go after the main workflow's post-steps) + var importedPostSteps []any + if importsResult.MergedPostSteps != "" { + if err := yaml.Unmarshal([]byte(importsResult.MergedPostSteps), &importedPostSteps); err == nil { + typedImported, err := SliceToSteps(importedPostSteps) + if err != nil { + orchestratorWorkflowLog.Printf("Failed to convert imported post-steps to typed steps: %v", err) + } else { + typedImported = ApplyActionPinsToTypedSteps(typedImported, workflowData) + importedPostSteps = StepsToSlice(typedImported) + } + } + } + + // Parse main workflow post-steps if present + var mainPostSteps []any + if mainPostStepsYAML != "" { + var mainWrapper map[string]any + if err := yaml.Unmarshal([]byte(mainPostStepsYAML), &mainWrapper); err == nil { + if mainVal, ok := mainWrapper["post-steps"]; ok { + if steps, ok := mainVal.([]any); ok { + mainPostSteps = steps + typedMain, err := SliceToSteps(mainPostSteps) if err != nil { - orchestratorWorkflowLog.Printf("Failed to convert post-steps to typed steps: %v", err) + orchestratorWorkflowLog.Printf("Failed to convert main post-steps to typed steps: %v", err) } else { - // Apply action pinning to post steps using type-safe version - typedPostSteps = ApplyActionPinsToTypedSteps(typedPostSteps, workflowData) - // Convert back to []any for YAML marshaling - postSteps = StepsToSlice(typedPostSteps) - } - - // Convert back to YAML with "post-steps:" wrapper - stepsWrapper := map[string]any{"post-steps": postSteps} - stepsYAML, err := yaml.Marshal(stepsWrapper) - if err == nil { - // Remove quotes from uses values with version comments - workflowData.PostSteps = unquoteUsesWithComments(string(stepsYAML)) + typedMain = ApplyActionPinsToTypedSteps(typedMain, workflowData) + mainPostSteps = StepsToSlice(typedMain) } } } } } + + // Merge in order: main workflow's post-steps first, then imported post-steps + var allPostSteps []any + if len(mainPostSteps) > 0 || len(importedPostSteps) > 0 { + allPostSteps = append(allPostSteps, mainPostSteps...) + allPostSteps = append(allPostSteps, importedPostSteps...) + + stepsWrapper := map[string]any{"post-steps": allPostSteps} + stepsYAML, err := yaml.Marshal(stepsWrapper) + if err == nil { + workflowData.PostSteps = unquoteUsesWithComments(string(stepsYAML)) + } + } } // processAndMergeServices handles the merging of imported services with main workflow services diff --git a/pkg/workflow/compiler_orchestrator_workflow_test.go b/pkg/workflow/compiler_orchestrator_workflow_test.go index 0887dbb6fe..df4f506f25 100644 --- a/pkg/workflow/compiler_orchestrator_workflow_test.go +++ b/pkg/workflow/compiler_orchestrator_workflow_test.go @@ -401,8 +401,9 @@ func TestProcessAndMergePostSteps_NoPostSteps(t *testing.T) { compiler := NewCompiler() workflowData := &WorkflowData{} frontmatter := map[string]any{} + importsResult := &parser.ImportsResult{} - compiler.processAndMergePostSteps(frontmatter, workflowData) + compiler.processAndMergePostSteps(frontmatter, workflowData, importsResult) assert.Empty(t, workflowData.PostSteps) } @@ -430,14 +431,103 @@ func TestProcessAndMergePostSteps_WithPostSteps(t *testing.T) { }, }, } + importsResult := &parser.ImportsResult{} - compiler.processAndMergePostSteps(frontmatter, workflowData) + compiler.processAndMergePostSteps(frontmatter, workflowData, importsResult) assert.NotEmpty(t, workflowData.PostSteps) assert.Contains(t, workflowData.PostSteps, "Cleanup") assert.Contains(t, workflowData.PostSteps, "Upload logs") } +// TestProcessAndMergePostSteps_WithImportedPostSteps tests that imported post-steps are appended +func TestProcessAndMergePostSteps_WithImportedPostSteps(t *testing.T) { + compiler := NewCompiler() + workflowData := &WorkflowData{} + + frontmatter := map[string]any{ + "post-steps": []any{ + map[string]any{"name": "Main post step", "run": "echo 'main'"}, + }, + } + + importedPostStepsYAML, _ := yaml.Marshal([]any{ + map[string]any{"name": "Imported post step", "run": "echo 'imported'"}, + }) + importsResult := &parser.ImportsResult{ + MergedPostSteps: string(importedPostStepsYAML), + } + + compiler.processAndMergePostSteps(frontmatter, workflowData, importsResult) + + assert.Contains(t, workflowData.PostSteps, "Main post step") + assert.Contains(t, workflowData.PostSteps, "Imported post step") + + // Main workflow's post-steps should come before imported ones + mainIdx := strings.Index(workflowData.PostSteps, "Main post step") + importedIdx := strings.Index(workflowData.PostSteps, "Imported post step") + assert.Less(t, mainIdx, importedIdx, "Main post-steps should come before imported ones") +} + +// TestProcessAndMergePreSteps_NoPreSteps tests processAndMergePreSteps with no pre-steps +func TestProcessAndMergePreSteps_NoPreSteps(t *testing.T) { + compiler := NewCompiler() + workflowData := &WorkflowData{} + frontmatter := map[string]any{} + importsResult := &parser.ImportsResult{} + + compiler.processAndMergePreSteps(frontmatter, workflowData, importsResult) + + assert.Empty(t, workflowData.PreSteps) +} + +// TestProcessAndMergePreSteps_WithPreSteps tests processAndMergePreSteps with pre-steps defined +func TestProcessAndMergePreSteps_WithPreSteps(t *testing.T) { + compiler := NewCompiler() + workflowData := &WorkflowData{} + + frontmatter := map[string]any{ + "pre-steps": []any{ + map[string]any{"name": "Mint token", "run": "echo 'minting'"}, + }, + } + importsResult := &parser.ImportsResult{} + + compiler.processAndMergePreSteps(frontmatter, workflowData, importsResult) + + assert.NotEmpty(t, workflowData.PreSteps) + assert.Contains(t, workflowData.PreSteps, "Mint token") +} + +// TestProcessAndMergePreSteps_WithImportedPreSteps tests that imported pre-steps are prepended +func TestProcessAndMergePreSteps_WithImportedPreSteps(t *testing.T) { + compiler := NewCompiler() + workflowData := &WorkflowData{} + + frontmatter := map[string]any{ + "pre-steps": []any{ + map[string]any{"name": "Main pre step", "run": "echo 'main'"}, + }, + } + + importedPreStepsYAML, _ := yaml.Marshal([]any{ + map[string]any{"name": "Imported pre step", "run": "echo 'imported'"}, + }) + importsResult := &parser.ImportsResult{ + MergedPreSteps: string(importedPreStepsYAML), + } + + compiler.processAndMergePreSteps(frontmatter, workflowData, importsResult) + + assert.Contains(t, workflowData.PreSteps, "Main pre step") + assert.Contains(t, workflowData.PreSteps, "Imported pre step") + + // Imported pre-steps should come before the main workflow's pre-steps + importedIdx := strings.Index(workflowData.PreSteps, "Imported pre step") + mainIdx := strings.Index(workflowData.PreSteps, "Main pre step") + assert.Less(t, importedIdx, mainIdx, "Imported pre-steps should come before main pre-steps") +} + // TestProcessAndMergeServices_NoServices tests processAndMergeServices with no services func TestProcessAndMergeServices_NoServices(t *testing.T) { compiler := NewCompiler() diff --git a/pkg/workflow/compiler_presteps_test.go b/pkg/workflow/compiler_presteps_test.go index e1c14fee3b..53773b8cc9 100644 --- a/pkg/workflow/compiler_presteps_test.go +++ b/pkg/workflow/compiler_presteps_test.go @@ -28,7 +28,7 @@ tools: pre-steps: - name: Mint short-lived token id: mint - uses: some-org/token-minting-action@v1 + uses: some-org/token-minting-action@a1b2c3d4e5f6a1b2c3d4e5f6a1b2c3d4e5f6a1b2 with: scope: target-org/target-repo steps: @@ -131,7 +131,7 @@ permissions: pre-steps: - name: Mint token id: mint - uses: some-org/token-action@v1 + uses: some-org/token-action@b1c2d3e4f5a6b1c2d3e4f5a6b1c2d3e4f5a6b1c2 with: scope: target-org/target-repo checkout: diff --git a/pkg/workflow/compiler_yaml_main_job.go b/pkg/workflow/compiler_yaml_main_job.go index 13876546e7..6d99417089 100644 --- a/pkg/workflow/compiler_yaml_main_job.go +++ b/pkg/workflow/compiler_yaml_main_job.go @@ -20,12 +20,13 @@ func (c *Compiler) generateMainJobSteps(yaml *strings.Builder, data *WorkflowDat yaml.WriteString(generateOTLPHeadersMaskStep()) } - // Add pre-steps before checkout and any other built-in steps. + // Add pre-steps before checkout and the subsequent built-in steps in this agent job. // This allows users to mint short-lived tokens (via custom actions) in the same // job as checkout, so the tokens are never dropped by the GitHub Actions runner's // add-mask behaviour that silently suppresses masked values across job boundaries. // Step outputs are available as ${{ steps..outputs. }} and can be - // referenced directly in checkout.token. + // referenced directly in checkout.token. Some compiler-injected setup steps may + // still be emitted earlier than these pre-steps. c.generatePreSteps(yaml, data) // Determine if we need to add a checkout step From 0668ade58584f521f6aede8566439f846a32bee3 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 8 Apr 2026 05:55:45 +0000 Subject: [PATCH 4/4] fix: address code review feedback - add error logging and require.NoError in tests Agent-Logs-Url: https://github.com/github/gh-aw/sessions/088642a3-c500-49a7-a249-a7db5353583e Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- pkg/workflow/compiler_orchestrator_workflow.go | 8 ++++++-- pkg/workflow/compiler_orchestrator_workflow_test.go | 6 ++++-- 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/pkg/workflow/compiler_orchestrator_workflow.go b/pkg/workflow/compiler_orchestrator_workflow.go index fb16db9f8e..079677fc54 100644 --- a/pkg/workflow/compiler_orchestrator_workflow.go +++ b/pkg/workflow/compiler_orchestrator_workflow.go @@ -509,7 +509,9 @@ func (c *Compiler) processAndMergePreSteps(frontmatter map[string]any, workflowD // Parse imported pre-steps if present (these go before the main workflow's pre-steps) var importedPreSteps []any if importsResult.MergedPreSteps != "" { - if err := yaml.Unmarshal([]byte(importsResult.MergedPreSteps), &importedPreSteps); err == nil { + if err := yaml.Unmarshal([]byte(importsResult.MergedPreSteps), &importedPreSteps); err != nil { + orchestratorWorkflowLog.Printf("Failed to unmarshal imported pre-steps: %v", err) + } else { typedImported, err := SliceToSteps(importedPreSteps) if err != nil { orchestratorWorkflowLog.Printf("Failed to convert imported pre-steps to typed steps: %v", err) @@ -564,7 +566,9 @@ func (c *Compiler) processAndMergePostSteps(frontmatter map[string]any, workflow // Parse imported post-steps if present (these go after the main workflow's post-steps) var importedPostSteps []any if importsResult.MergedPostSteps != "" { - if err := yaml.Unmarshal([]byte(importsResult.MergedPostSteps), &importedPostSteps); err == nil { + if err := yaml.Unmarshal([]byte(importsResult.MergedPostSteps), &importedPostSteps); err != nil { + orchestratorWorkflowLog.Printf("Failed to unmarshal imported post-steps: %v", err) + } else { typedImported, err := SliceToSteps(importedPostSteps) if err != nil { orchestratorWorkflowLog.Printf("Failed to convert imported post-steps to typed steps: %v", err) diff --git a/pkg/workflow/compiler_orchestrator_workflow_test.go b/pkg/workflow/compiler_orchestrator_workflow_test.go index df4f506f25..1a75bb592f 100644 --- a/pkg/workflow/compiler_orchestrator_workflow_test.go +++ b/pkg/workflow/compiler_orchestrator_workflow_test.go @@ -451,9 +451,10 @@ func TestProcessAndMergePostSteps_WithImportedPostSteps(t *testing.T) { }, } - importedPostStepsYAML, _ := yaml.Marshal([]any{ + importedPostStepsYAML, err := yaml.Marshal([]any{ map[string]any{"name": "Imported post step", "run": "echo 'imported'"}, }) + require.NoError(t, err, "yaml.Marshal should not fail for well-formed post-steps") importsResult := &parser.ImportsResult{ MergedPostSteps: string(importedPostStepsYAML), } @@ -510,9 +511,10 @@ func TestProcessAndMergePreSteps_WithImportedPreSteps(t *testing.T) { }, } - importedPreStepsYAML, _ := yaml.Marshal([]any{ + importedPreStepsYAML, err := yaml.Marshal([]any{ map[string]any{"name": "Imported pre step", "run": "echo 'imported'"}, }) + require.NoError(t, err, "yaml.Marshal should not fail for well-formed pre-steps") importsResult := &parser.ImportsResult{ MergedPreSteps: string(importedPreStepsYAML), }