From ca8346aca0049bef1ea73e5890767d177aca4ee6 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sun, 19 Apr 2026 01:29:21 +0000 Subject: [PATCH 1/8] Add jobs..pre-steps support with import merge ordering Agent-Logs-Url: https://github.com/github/gh-aw/sessions/a95be5cc-b3cc-4172-8353-49380b87d1bc Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- .../src/content/docs/reference/frontmatter.md | 8 ++ pkg/parser/schemas/main_workflow_schema.json | 8 ++ pkg/workflow/compiler_jobs.go | 83 +++++++++----- pkg/workflow/compiler_jobs_test.go | 103 ++++++++++++++++++ .../compiler_orchestrator_workflow_test.go | 36 ++++++ pkg/workflow/workflow_import_merge.go | 48 ++++++++ 6 files changed, 258 insertions(+), 28 deletions(-) diff --git a/docs/src/content/docs/reference/frontmatter.md b/docs/src/content/docs/reference/frontmatter.md index 9ae9dda9b32..62749f2920a 100644 --- a/docs/src/content/docs/reference/frontmatter.md +++ b/docs/src/content/docs/reference/frontmatter.md @@ -755,6 +755,7 @@ The following job-level fields are supported in custom jobs: | `continue-on-error` | Allow the workflow to continue if this job fails | | `container` | Docker container to run steps in | | `services` | Service containers (e.g. databases) | +| `pre-steps` | Steps injected after compiler setup steps and before checkout/`steps` in that job | | `steps` | List of steps — supports complete GitHub Actions step specification | | `uses` | Reusable workflow to call | | `with` | Input parameters for a reusable workflow | @@ -774,6 +775,13 @@ jobs: - uses: actions/checkout@v6 ``` +When `jobs..pre-steps` is set, step execution order is deterministic: + +1. Compiler-injected setup steps +2. `jobs..pre-steps` +3. Checkout steps +4. Remaining `jobs..steps` + The following example uses `timeout-minutes` and `env`: ```yaml wrap diff --git a/pkg/parser/schemas/main_workflow_schema.json b/pkg/parser/schemas/main_workflow_schema.json index 8cd69389fcd..84f5cace55d 100644 --- a/pkg/parser/schemas/main_workflow_schema.json +++ b/pkg/parser/schemas/main_workflow_schema.json @@ -2387,6 +2387,14 @@ } } }, + "pre-steps": { + "allOf": [ + { + "$ref": "#/properties/jobs/additionalProperties/properties/steps" + } + ], + "description": "Optional steps inserted after setup-injected steps and before any checkout step in this custom job. Uses the same schema as `steps`." + }, "if": { "type": "string", "description": "Conditional execution for the job" diff --git a/pkg/workflow/compiler_jobs.go b/pkg/workflow/compiler_jobs.go index 57dbaefc69b..12def23d7fa 100644 --- a/pkg/workflow/compiler_jobs.go +++ b/pkg/workflow/compiler_jobs.go @@ -765,35 +765,28 @@ func (c *Compiler) buildCustomJobs(data *WorkflowData, activationJobCreated bool } } } else { - // Add basic steps if specified (only for non-reusable workflow jobs) - if steps, hasSteps := configMap["steps"]; hasSteps { - if stepsList, ok := steps.([]any); ok { - // Prepend GH_HOST configuration step for GHES/GHEC compatibility. - // Custom frontmatter jobs run as independent GitHub Actions jobs that - // don't inherit GITHUB_ENV from the agent job, so the gh CLI won't - // know which host to target without this step. - job.Steps = append(job.Steps, generateGHESHostConfigurationStep()) - - for _, step := range stepsList { - if stepMap, ok := step.(map[string]any); ok { - // Convert to typed step for action pinning - typedStep, err := MapToStep(stepMap) - if err != nil { - return fmt.Errorf("failed to convert step to typed step for job '%s': %w", jobName, err) - } - - // Apply action pinning using type-safe version - pinnedStep := applyActionPinToTypedStep(typedStep, data) + // Add basic steps if specified (only for non-reusable workflow jobs). + // `pre-steps` are inserted after setup-injected steps and before the + // regular `steps` list so they run before any checkout in steps. + preSteps, err := c.extractPinnedJobSteps(configMap, "pre-steps", jobName, data) + if err != nil { + return err + } + regularSteps, err := c.extractPinnedJobSteps(configMap, "steps", jobName, data) + if err != nil { + return err + } - // Convert back to map for YAML generation - stepYAML, err := ConvertStepToYAML(pinnedStep.ToMap()) - if err != nil { - return fmt.Errorf("failed to convert step to YAML for job '%s': %w", jobName, err) - } - job.Steps = append(job.Steps, stepYAML) - } - } - } + _, hasPreStepsField := configMap["pre-steps"] + _, hasStepsField := configMap["steps"] + if hasPreStepsField || hasStepsField { + // Prepend GH_HOST configuration step for GHES/GHEC compatibility. + // Custom frontmatter jobs run as independent GitHub Actions jobs that + // don't inherit GITHUB_ENV from the agent job, so the gh CLI won't + // know which host to target without this step. + job.Steps = append(job.Steps, generateGHESHostConfigurationStep()) + job.Steps = append(job.Steps, preSteps...) + job.Steps = append(job.Steps, regularSteps...) } } @@ -808,6 +801,40 @@ func (c *Compiler) buildCustomJobs(data *WorkflowData, activationJobCreated bool return nil } +func (c *Compiler) extractPinnedJobSteps(configMap map[string]any, fieldName string, jobName string, data *WorkflowData) ([]string, error) { + raw, hasField := configMap[fieldName] + if !hasField { + return nil, nil + } + + stepsList, ok := raw.([]any) + if !ok { + return nil, nil + } + + steps := make([]string, 0, len(stepsList)) + for _, step := range stepsList { + stepMap, ok := step.(map[string]any) + if !ok { + continue + } + + typedStep, err := MapToStep(stepMap) + if err != nil { + return nil, fmt.Errorf("failed to convert %s to typed step for job '%s': %w", fieldName, jobName, err) + } + + pinnedStep := applyActionPinToTypedStep(typedStep, data) + stepYAML, err := ConvertStepToYAML(pinnedStep.ToMap()) + if err != nil { + return nil, fmt.Errorf("failed to convert %s to YAML for job '%s': %w", fieldName, jobName, err) + } + steps = append(steps, stepYAML) + } + + return steps, nil +} + // shouldAddCheckoutStep returns true if the workflow requires a checkout step. // The repository checkout is needed in the agent job to access workflow files, // custom agent files, and other repository content. diff --git a/pkg/workflow/compiler_jobs_test.go b/pkg/workflow/compiler_jobs_test.go index 364cd781bde..9ac0f819bbe 100644 --- a/pkg/workflow/compiler_jobs_test.go +++ b/pkg/workflow/compiler_jobs_test.go @@ -799,6 +799,109 @@ Test content` // custom_build has explicit needs so should keep that } +// TestCustomJobPreStepsAreInsertedBeforeCheckout verifies jobs..pre-steps +// are emitted after setup-injected steps and before checkout in custom jobs. +func TestCustomJobPreStepsAreInsertedBeforeCheckout(t *testing.T) { + tmpDir := testutil.TempDir(t, "custom-job-pre-steps") + + frontmatter := `--- +on: push +permissions: + contents: read +engine: copilot +strict: false +jobs: + custom_job: + runs-on: ubuntu-latest + pre-steps: + - name: Pre setup + run: echo "pre" + - name: Prepare token + run: echo "token" + steps: + - name: Checkout repo + uses: actions/checkout@v6 + - name: Main work + run: echo "work" +--- + +# Test Workflow +` + + testFile := filepath.Join(tmpDir, "test.md") + if err := os.WriteFile(testFile, []byte(frontmatter), 0644); err != nil { + t.Fatal(err) + } + + compiler := NewCompiler() + if err := compiler.CompileWorkflow(testFile); err != nil { + t.Fatalf("CompileWorkflow() error: %v", err) + } + + lockFile := filepath.Join(tmpDir, "test.lock.yml") + content, err := os.ReadFile(lockFile) + if err != nil { + t.Fatalf("Failed to read lock file: %v", err) + } + + yamlStr := string(content) + customJobSection := extractJobSection(yamlStr, "custom_job") + if customJobSection == "" { + t.Fatal("Expected custom_job section in lock file") + } + + ghesSetupIdx := indexInNonCommentLines(customJobSection, "- name: Configure GH_HOST for enterprise compatibility") + preSetupIdx := indexInNonCommentLines(customJobSection, "- name: Pre setup") + prepareTokenIdx := indexInNonCommentLines(customJobSection, "- name: Prepare token") + checkoutIdx := indexInNonCommentLines(customJobSection, "- name: Checkout repo") + + if ghesSetupIdx == -1 || preSetupIdx == -1 || prepareTokenIdx == -1 || checkoutIdx == -1 { + t.Fatalf("Missing expected steps in custom job section:\n%s", customJobSection) + } + + if ghesSetupIdx >= preSetupIdx || preSetupIdx >= prepareTokenIdx || prepareTokenIdx >= checkoutIdx { + t.Fatalf("Expected order setup -> pre-steps -> checkout, got indexes setup=%d pre1=%d pre2=%d checkout=%d\n%s", + ghesSetupIdx, preSetupIdx, prepareTokenIdx, checkoutIdx, customJobSection) + } +} + +func TestCustomJobPreStepsSchemaValidation(t *testing.T) { + tmpDir := testutil.TempDir(t, "custom-job-pre-steps-schema") + + frontmatter := `--- +on: push +permissions: + contents: read +engine: copilot +strict: false +jobs: + custom_job: + runs-on: ubuntu-latest + pre-steps: + name: Invalid pre-steps + run: echo "invalid" + steps: + - run: echo "work" +--- + +# Test Workflow +` + + testFile := filepath.Join(tmpDir, "test.md") + if err := os.WriteFile(testFile, []byte(frontmatter), 0644); err != nil { + t.Fatal(err) + } + + compiler := NewCompiler() + err := compiler.CompileWorkflow(testFile) + if err == nil { + t.Fatal("Expected schema validation error for non-array jobs..pre-steps, got nil") + } + if !strings.Contains(err.Error(), "pre-steps") { + t.Fatalf("Expected error to mention pre-steps, got: %v", err) + } +} + // TestBuildSafeOutputsJobsCreatesExpectedJobs tests that safe output steps are created correctly // in the consolidated safe_outputs job func TestBuildSafeOutputsJobsCreatesExpectedJobs(t *testing.T) { diff --git a/pkg/workflow/compiler_orchestrator_workflow_test.go b/pkg/workflow/compiler_orchestrator_workflow_test.go index 431cfdf29ea..ab0ef87d8d6 100644 --- a/pkg/workflow/compiler_orchestrator_workflow_test.go +++ b/pkg/workflow/compiler_orchestrator_workflow_test.go @@ -758,6 +758,42 @@ func TestMergeJobsFromYAMLImports_MainJobTakesPrecedence(t *testing.T) { assert.Equal(t, "ubuntu-latest", testJob["runs-on"]) } +// TestMergeJobsFromYAMLImports_MergesPreStepsOnConflict tests deterministic merging of +// jobs..pre-steps when main and imported workflows define the same job. +func TestMergeJobsFromYAMLImports_MergesPreStepsOnConflict(t *testing.T) { + compiler := NewCompiler() + + mainJobs := map[string]any{ + "test": map[string]any{ + "runs-on": "ubuntu-latest", + "pre-steps": []any{ + map[string]any{"name": "main pre", "run": "echo main"}, + }, + "steps": []any{ + map[string]any{"run": "echo main job"}, + }, + }, + } + + importedJobsJSON := `{"test": {"runs-on": "macos-latest", "pre-steps": [{"name": "import pre", "run": "echo import"}], "steps": [{"run": "echo imported job"}]}}` + result := compiler.mergeJobsFromYAMLImports(mainJobs, importedJobsJSON) + + assert.Len(t, result, 1) + testJob := result["test"].(map[string]any) + + // Main job fields still take precedence. + assert.Equal(t, "ubuntu-latest", testJob["runs-on"]) + + preSteps, ok := testJob["pre-steps"].([]any) + require.True(t, ok, "Expected merged pre-steps array") + require.Len(t, preSteps, 2, "Expected imported+main pre-steps to be merged") + + first := preSteps[0].(map[string]any) + second := preSteps[1].(map[string]any) + assert.Equal(t, "import pre", first["name"], "Imported pre-steps should run first") + assert.Equal(t, "main pre", second["name"], "Main workflow pre-steps should run after imported pre-steps") +} + // TestMergeJobsFromYAMLImports_MultipleImportedJobs tests merging multiple imported jobs func TestMergeJobsFromYAMLImports_MultipleImportedJobs(t *testing.T) { compiler := NewCompiler() diff --git a/pkg/workflow/workflow_import_merge.go b/pkg/workflow/workflow_import_merge.go index a8364942333..2c25907d100 100644 --- a/pkg/workflow/workflow_import_merge.go +++ b/pkg/workflow/workflow_import_merge.go @@ -109,6 +109,15 @@ func (c *Compiler) mergeJobsFromYAMLImports(mainJobs map[string]any, mergedJobsJ workflowImportMergeLog.Printf("Adding imported job: %s", jobName) result[jobName] = jobConfig } else { + // Keep main workflow job precedence, but merge pre-steps deterministically + // when both imported and main define pre-steps for the same job. + mergedJob, merged := mergeJobPreSteps(result[jobName], jobConfig) + if merged { + workflowImportMergeLog.Printf("Merged pre-steps for conflicting job %s (imported first, then main)", jobName) + result[jobName] = mergedJob + continue + } + workflowImportMergeLog.Printf("Skipping imported job %s (already defined in main workflow)", jobName) } } @@ -117,3 +126,42 @@ func (c *Compiler) mergeJobsFromYAMLImports(mainJobs map[string]any, mergedJobsJ workflowImportMergeLog.Printf("Successfully merged jobs: total=%d, imported=%d", len(result), len(result)-len(mainJobs)) return result } + +func mergeJobPreSteps(mainJob any, importedJob any) (map[string]any, bool) { + mainMap, ok := mainJob.(map[string]any) + if !ok { + return nil, false + } + importedMap, ok := importedJob.(map[string]any) + if !ok { + return nil, false + } + + mainPreSteps, okMain := extractJobPreSteps(mainMap) + importedPreSteps, okImported := extractJobPreSteps(importedMap) + if !okMain || !okImported { + return nil, false + } + + merged := make(map[string]any, len(mainMap)) + maps.Copy(merged, mainMap) + + mergedPreSteps := make([]any, 0, len(importedPreSteps)+len(mainPreSteps)) + mergedPreSteps = append(mergedPreSteps, importedPreSteps...) + mergedPreSteps = append(mergedPreSteps, mainPreSteps...) + merged["pre-steps"] = mergedPreSteps + + return merged, true +} + +func extractJobPreSteps(jobConfig map[string]any) ([]any, bool) { + raw, exists := jobConfig["pre-steps"] + if !exists { + return nil, false + } + steps, ok := raw.([]any) + if !ok { + return nil, false + } + return steps, true +} From d24b6f31480cb30c5934ed621b6d7f31851519c3 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sun, 19 Apr 2026 01:35:27 +0000 Subject: [PATCH 2/8] Address review feedback for jobs pre-steps support Agent-Logs-Url: https://github.com/github/gh-aw/sessions/a95be5cc-b3cc-4172-8353-49380b87d1bc Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- pkg/workflow/compiler_jobs.go | 4 ++-- pkg/workflow/compiler_jobs_test.go | 9 +++++---- 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/pkg/workflow/compiler_jobs.go b/pkg/workflow/compiler_jobs.go index 12def23d7fa..983efbc33c5 100644 --- a/pkg/workflow/compiler_jobs.go +++ b/pkg/workflow/compiler_jobs.go @@ -770,11 +770,11 @@ func (c *Compiler) buildCustomJobs(data *WorkflowData, activationJobCreated bool // regular `steps` list so they run before any checkout in steps. preSteps, err := c.extractPinnedJobSteps(configMap, "pre-steps", jobName, data) if err != nil { - return err + return fmt.Errorf("failed to process pre-steps for job '%s': %w", jobName, err) } regularSteps, err := c.extractPinnedJobSteps(configMap, "steps", jobName, data) if err != nil { - return err + return fmt.Errorf("failed to process steps for job '%s': %w", jobName, err) } _, hasPreStepsField := configMap["pre-steps"] diff --git a/pkg/workflow/compiler_jobs_test.go b/pkg/workflow/compiler_jobs_test.go index 9ac0f819bbe..f95bd2c2359 100644 --- a/pkg/workflow/compiler_jobs_test.go +++ b/pkg/workflow/compiler_jobs_test.go @@ -854,14 +854,15 @@ jobs: preSetupIdx := indexInNonCommentLines(customJobSection, "- name: Pre setup") prepareTokenIdx := indexInNonCommentLines(customJobSection, "- name: Prepare token") checkoutIdx := indexInNonCommentLines(customJobSection, "- name: Checkout repo") + mainWorkIdx := indexInNonCommentLines(customJobSection, "- name: Main work") - if ghesSetupIdx == -1 || preSetupIdx == -1 || prepareTokenIdx == -1 || checkoutIdx == -1 { + if ghesSetupIdx == -1 || preSetupIdx == -1 || prepareTokenIdx == -1 || checkoutIdx == -1 || mainWorkIdx == -1 { t.Fatalf("Missing expected steps in custom job section:\n%s", customJobSection) } - if ghesSetupIdx >= preSetupIdx || preSetupIdx >= prepareTokenIdx || prepareTokenIdx >= checkoutIdx { - t.Fatalf("Expected order setup -> pre-steps -> checkout, got indexes setup=%d pre1=%d pre2=%d checkout=%d\n%s", - ghesSetupIdx, preSetupIdx, prepareTokenIdx, checkoutIdx, customJobSection) + if ghesSetupIdx >= preSetupIdx || preSetupIdx >= prepareTokenIdx || prepareTokenIdx >= checkoutIdx || checkoutIdx >= mainWorkIdx { + t.Fatalf("Expected order setup -> pre-steps -> checkout -> steps, got indexes setup=%d pre1=%d pre2=%d checkout=%d main=%d\n%s", + ghesSetupIdx, preSetupIdx, prepareTokenIdx, checkoutIdx, mainWorkIdx, customJobSection) } } From 91398fb7098a10eb9bfce3a4b1f5ef9529a75200 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sun, 19 Apr 2026 01:41:18 +0000 Subject: [PATCH 3/8] Refine jobs pre-steps extraction and ordering assertions Agent-Logs-Url: https://github.com/github/gh-aw/sessions/a95be5cc-b3cc-4172-8353-49380b87d1bc Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- pkg/workflow/compiler_jobs.go | 26 +++++++++++++++++--------- 1 file changed, 17 insertions(+), 9 deletions(-) diff --git a/pkg/workflow/compiler_jobs.go b/pkg/workflow/compiler_jobs.go index 983efbc33c5..c0336529734 100644 --- a/pkg/workflow/compiler_jobs.go +++ b/pkg/workflow/compiler_jobs.go @@ -768,17 +768,25 @@ func (c *Compiler) buildCustomJobs(data *WorkflowData, activationJobCreated bool // Add basic steps if specified (only for non-reusable workflow jobs). // `pre-steps` are inserted after setup-injected steps and before the // regular `steps` list so they run before any checkout in steps. - preSteps, err := c.extractPinnedJobSteps(configMap, "pre-steps", jobName, data) - if err != nil { - return fmt.Errorf("failed to process pre-steps for job '%s': %w", jobName, err) + var preSteps []string + var regularSteps []string + _, hasPreStepsField := configMap["pre-steps"] + _, hasStepsField := configMap["steps"] + if hasPreStepsField { + var err error + preSteps, err = c.extractPinnedJobSteps(configMap, "pre-steps", jobName, data) + if err != nil { + return fmt.Errorf("failed to process pre-steps for job '%s': %w", jobName, err) + } } - regularSteps, err := c.extractPinnedJobSteps(configMap, "steps", jobName, data) - if err != nil { - return fmt.Errorf("failed to process steps for job '%s': %w", jobName, err) + if hasStepsField { + var err error + regularSteps, err = c.extractPinnedJobSteps(configMap, "steps", jobName, data) + if err != nil { + return fmt.Errorf("failed to process steps for job '%s': %w", jobName, err) + } } - _, hasPreStepsField := configMap["pre-steps"] - _, hasStepsField := configMap["steps"] if hasPreStepsField || hasStepsField { // Prepend GH_HOST configuration step for GHES/GHEC compatibility. // Custom frontmatter jobs run as independent GitHub Actions jobs that @@ -809,7 +817,7 @@ func (c *Compiler) extractPinnedJobSteps(configMap map[string]any, fieldName str stepsList, ok := raw.([]any) if !ok { - return nil, nil + return nil, fmt.Errorf("%s for job '%s' must be an array of step objects", fieldName, jobName) } steps := make([]string, 0, len(stepsList)) From a8ea0b76aec49b25ea29ce9e7bc37c8c138eb0a6 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sun, 19 Apr 2026 01:47:22 +0000 Subject: [PATCH 4/8] Harden job step parsing and simplify ordering assertions Agent-Logs-Url: https://github.com/github/gh-aw/sessions/a95be5cc-b3cc-4172-8353-49380b87d1bc Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- pkg/workflow/compiler_jobs.go | 4 ++-- pkg/workflow/compiler_jobs_test.go | 38 +++++++++++++++++++----------- 2 files changed, 26 insertions(+), 16 deletions(-) diff --git a/pkg/workflow/compiler_jobs.go b/pkg/workflow/compiler_jobs.go index c0336529734..a6539b1ed36 100644 --- a/pkg/workflow/compiler_jobs.go +++ b/pkg/workflow/compiler_jobs.go @@ -821,10 +821,10 @@ func (c *Compiler) extractPinnedJobSteps(configMap map[string]any, fieldName str } steps := make([]string, 0, len(stepsList)) - for _, step := range stepsList { + for i, step := range stepsList { stepMap, ok := step.(map[string]any) if !ok { - continue + return nil, fmt.Errorf("%s for job '%s' contains invalid step at index %d: expected object", fieldName, jobName, i) } typedStep, err := MapToStep(stepMap) diff --git a/pkg/workflow/compiler_jobs_test.go b/pkg/workflow/compiler_jobs_test.go index f95bd2c2359..2602cd38ab8 100644 --- a/pkg/workflow/compiler_jobs_test.go +++ b/pkg/workflow/compiler_jobs_test.go @@ -850,20 +850,13 @@ jobs: t.Fatal("Expected custom_job section in lock file") } - ghesSetupIdx := indexInNonCommentLines(customJobSection, "- name: Configure GH_HOST for enterprise compatibility") - preSetupIdx := indexInNonCommentLines(customJobSection, "- name: Pre setup") - prepareTokenIdx := indexInNonCommentLines(customJobSection, "- name: Prepare token") - checkoutIdx := indexInNonCommentLines(customJobSection, "- name: Checkout repo") - mainWorkIdx := indexInNonCommentLines(customJobSection, "- name: Main work") - - if ghesSetupIdx == -1 || preSetupIdx == -1 || prepareTokenIdx == -1 || checkoutIdx == -1 || mainWorkIdx == -1 { - t.Fatalf("Missing expected steps in custom job section:\n%s", customJobSection) - } - - if ghesSetupIdx >= preSetupIdx || preSetupIdx >= prepareTokenIdx || prepareTokenIdx >= checkoutIdx || checkoutIdx >= mainWorkIdx { - t.Fatalf("Expected order setup -> pre-steps -> checkout -> steps, got indexes setup=%d pre1=%d pre2=%d checkout=%d main=%d\n%s", - ghesSetupIdx, preSetupIdx, prepareTokenIdx, checkoutIdx, mainWorkIdx, customJobSection) - } + assertStepOrderInSection(t, customJobSection, + "- name: Configure GH_HOST for enterprise compatibility", + "- name: Pre setup", + "- name: Prepare token", + "- name: Checkout repo", + "- name: Main work", + ) } func TestCustomJobPreStepsSchemaValidation(t *testing.T) { @@ -903,6 +896,23 @@ jobs: } } +func assertStepOrderInSection(t *testing.T, section string, orderedSteps ...string) { + t.Helper() + + prev := -1 + for _, step := range orderedSteps { + idx := indexInNonCommentLines(section, step) + if idx == -1 { + t.Fatalf("Expected step %q in section:\n%s", step, section) + } + if prev >= idx { + t.Fatalf("Expected step order %v in section, but %q appeared at %d after previous index %d\n%s", + orderedSteps, step, idx, prev, section) + } + prev = idx + } +} + // TestBuildSafeOutputsJobsCreatesExpectedJobs tests that safe output steps are created correctly // in the consolidated safe_outputs job func TestBuildSafeOutputsJobsCreatesExpectedJobs(t *testing.T) { From 34237f1df77171c8798c9a69fe57e558f7b41d0d Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sun, 19 Apr 2026 01:53:21 +0000 Subject: [PATCH 5/8] Polish pre-steps implementation per validation feedback Agent-Logs-Url: https://github.com/github/gh-aw/sessions/a95be5cc-b3cc-4172-8353-49380b87d1bc Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- pkg/workflow/compiler_jobs.go | 6 +++--- pkg/workflow/compiler_jobs_test.go | 16 +++++++++++++++- pkg/workflow/workflow_import_merge.go | 3 +++ 3 files changed, 21 insertions(+), 4 deletions(-) diff --git a/pkg/workflow/compiler_jobs.go b/pkg/workflow/compiler_jobs.go index a6539b1ed36..4c33a0e6eed 100644 --- a/pkg/workflow/compiler_jobs.go +++ b/pkg/workflow/compiler_jobs.go @@ -820,7 +820,7 @@ func (c *Compiler) extractPinnedJobSteps(configMap map[string]any, fieldName str return nil, fmt.Errorf("%s for job '%s' must be an array of step objects", fieldName, jobName) } - steps := make([]string, 0, len(stepsList)) + pinnedSteps := make([]string, 0, len(stepsList)) for i, step := range stepsList { stepMap, ok := step.(map[string]any) if !ok { @@ -837,10 +837,10 @@ func (c *Compiler) extractPinnedJobSteps(configMap map[string]any, fieldName str if err != nil { return nil, fmt.Errorf("failed to convert %s to YAML for job '%s': %w", fieldName, jobName, err) } - steps = append(steps, stepYAML) + pinnedSteps = append(pinnedSteps, stepYAML) } - return steps, nil + return pinnedSteps, nil } // shouldAddCheckoutStep returns true if the workflow requires a checkout step. diff --git a/pkg/workflow/compiler_jobs_test.go b/pkg/workflow/compiler_jobs_test.go index 2602cd38ab8..18c3a7e2f0f 100644 --- a/pkg/workflow/compiler_jobs_test.go +++ b/pkg/workflow/compiler_jobs_test.go @@ -901,7 +901,7 @@ func assertStepOrderInSection(t *testing.T, section string, orderedSteps ...stri prev := -1 for _, step := range orderedSteps { - idx := indexInNonCommentLines(section, step) + idx := indexInNonCommentLinesInSection(section, step) if idx == -1 { t.Fatalf("Expected step %q in section:\n%s", step, section) } @@ -913,6 +913,20 @@ func assertStepOrderInSection(t *testing.T, section string, orderedSteps ...stri } } +func indexInNonCommentLinesInSection(content string, target string) int { + lines := strings.Split(content, "\n") + for i, line := range lines { + trimmed := strings.TrimSpace(line) + if strings.HasPrefix(trimmed, "#") { + continue + } + if strings.Contains(line, target) { + return i + } + } + return -1 +} + // TestBuildSafeOutputsJobsCreatesExpectedJobs tests that safe output steps are created correctly // in the consolidated safe_outputs job func TestBuildSafeOutputsJobsCreatesExpectedJobs(t *testing.T) { diff --git a/pkg/workflow/workflow_import_merge.go b/pkg/workflow/workflow_import_merge.go index 2c25907d100..2fb247830d1 100644 --- a/pkg/workflow/workflow_import_merge.go +++ b/pkg/workflow/workflow_import_merge.go @@ -144,6 +144,9 @@ func mergeJobPreSteps(mainJob any, importedJob any) (map[string]any, bool) { } merged := make(map[string]any, len(mainMap)) + // Intentionally shallow-copy the top-level job map: this merge operation only + // replaces the "pre-steps" key with a newly allocated slice and does not mutate + // any nested structures from other keys. maps.Copy(merged, mainMap) mergedPreSteps := make([]any, 0, len(importedPreSteps)+len(mainPreSteps)) From c719756b1bd45d9cac24fe9fb214fcf7825d9a34 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sun, 19 Apr 2026 01:59:19 +0000 Subject: [PATCH 6/8] Tighten pre-steps comments and helper signature ordering Agent-Logs-Url: https://github.com/github/gh-aw/sessions/a95be5cc-b3cc-4172-8353-49380b87d1bc Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- pkg/workflow/compiler_jobs.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/pkg/workflow/compiler_jobs.go b/pkg/workflow/compiler_jobs.go index 4c33a0e6eed..0bf0af4b758 100644 --- a/pkg/workflow/compiler_jobs.go +++ b/pkg/workflow/compiler_jobs.go @@ -767,21 +767,21 @@ func (c *Compiler) buildCustomJobs(data *WorkflowData, activationJobCreated bool } else { // Add basic steps if specified (only for non-reusable workflow jobs). // `pre-steps` are inserted after setup-injected steps and before the - // regular `steps` list so they run before any checkout in steps. + // regular `steps` list (including any checkout step it may contain). var preSteps []string var regularSteps []string _, hasPreStepsField := configMap["pre-steps"] _, hasStepsField := configMap["steps"] if hasPreStepsField { var err error - preSteps, err = c.extractPinnedJobSteps(configMap, "pre-steps", jobName, data) + preSteps, err = c.extractPinnedJobSteps("pre-steps", jobName, configMap, data) if err != nil { return fmt.Errorf("failed to process pre-steps for job '%s': %w", jobName, err) } } if hasStepsField { var err error - regularSteps, err = c.extractPinnedJobSteps(configMap, "steps", jobName, data) + regularSteps, err = c.extractPinnedJobSteps("steps", jobName, configMap, data) if err != nil { return fmt.Errorf("failed to process steps for job '%s': %w", jobName, err) } @@ -809,7 +809,7 @@ func (c *Compiler) buildCustomJobs(data *WorkflowData, activationJobCreated bool return nil } -func (c *Compiler) extractPinnedJobSteps(configMap map[string]any, fieldName string, jobName string, data *WorkflowData) ([]string, error) { +func (c *Compiler) extractPinnedJobSteps(fieldName string, jobName string, configMap map[string]any, data *WorkflowData) ([]string, error) { raw, hasField := configMap[fieldName] if !hasField { return nil, nil From 5ae4f39c61d4f6d424233cd34357a8d60f003e43 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sun, 19 Apr 2026 13:47:33 +0000 Subject: [PATCH 7/8] Add builtin pre-steps insertion tests and handling Agent-Logs-Url: https://github.com/github/gh-aw/sessions/4ac2fef2-7499-451e-9bdb-114658c1d76f Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- .../compiler_builtin_presteps_test.go | 80 +++++++++++++++++ pkg/workflow/compiler_jobs.go | 87 +++++++++++++++++++ pkg/workflow/compiler_pre_activation_job.go | 5 +- 3 files changed, 170 insertions(+), 2 deletions(-) create mode 100644 pkg/workflow/compiler_builtin_presteps_test.go diff --git a/pkg/workflow/compiler_builtin_presteps_test.go b/pkg/workflow/compiler_builtin_presteps_test.go new file mode 100644 index 00000000000..f52b19e150a --- /dev/null +++ b/pkg/workflow/compiler_builtin_presteps_test.go @@ -0,0 +1,80 @@ +//go:build !integration + +package workflow + +import ( + "os" + "path/filepath" + "testing" + + "github.com/github/gh-aw/pkg/testutil" +) + +func TestBuiltinJobsPreStepsInsertionOrder(t *testing.T) { + tmpDir := testutil.TempDir(t, "builtin-pre-steps") + + workflowContent := `--- +on: + issue_comment: + types: [created] + roles: [admin] +permissions: + contents: read + issues: read + pull-requests: read +engine: claude +strict: false +jobs: + pre-activation: + pre-steps: + - name: Pre-activation pre-step + run: echo "pre-activation" + activation: + pre-steps: + - name: Activation pre-step + run: echo "activation" +--- + +# Builtin pre-steps ordering + +Run builtin pre-step ordering checks. +` + + workflowFile := filepath.Join(tmpDir, "builtin-pre-steps.md") + if err := os.WriteFile(workflowFile, []byte(workflowContent), 0644); err != nil { + t.Fatal(err) + } + + compiler := NewCompiler() + if err := compiler.CompileWorkflow(workflowFile); err != nil { + t.Fatalf("CompileWorkflow() returned error: %v", err) + } + + lockFile := filepath.Join(tmpDir, "builtin-pre-steps.lock.yml") + lockContent, err := os.ReadFile(lockFile) + if err != nil { + t.Fatalf("Failed to read lock file: %v", err) + } + lockYAML := string(lockContent) + + activationSection := extractJobSection(lockYAML, "activation") + if activationSection == "" { + t.Fatal("Expected activation job section") + } + assertStepOrderInSection(t, activationSection, + "id: setup", + "- name: Activation pre-step", + "- name: Checkout .github and .agents folders", + ) + + preActivationSection := extractJobSection(lockYAML, "pre_activation") + if preActivationSection == "" { + t.Fatal("Expected pre_activation job section") + } + assertStepOrderInSection(t, preActivationSection, + "id: setup", + "- name: Pre-activation pre-step", + "- name: Check team membership", + ) + +} diff --git a/pkg/workflow/compiler_jobs.go b/pkg/workflow/compiler_jobs.go index 0bf0af4b758..e99cb8f29dc 100644 --- a/pkg/workflow/compiler_jobs.go +++ b/pkg/workflow/compiler_jobs.go @@ -4,6 +4,7 @@ import ( "fmt" "os" "path/filepath" + "regexp" "sort" "strings" @@ -17,6 +18,7 @@ import ( ) var compilerJobsLog = logger.New("workflow:compiler_jobs") +var exactSetupStepIDPattern = regexp.MustCompile(`(?m)^\s*id:\s*setup\s*$`) // This file contains job building functions extracted from compiler.go // These functions are responsible for constructing the various jobs that make up @@ -222,6 +224,12 @@ func (c *Compiler) buildJobs(data *WorkflowData, markdownPath string) error { return fmt.Errorf("failed to build safe outputs jobs: %w", err) } + // Apply jobs..pre-steps customizations to already-created built-in jobs + // before processing non-built-in custom jobs. + if err := c.applyBuiltinJobPreSteps(data); err != nil { + return fmt.Errorf("failed to apply built-in job pre-steps: %w", err) + } + // Build additional custom jobs from frontmatter jobs section if len(data.Jobs) > 0 { compilerJobsLog.Printf("Building %d custom jobs from frontmatter", len(data.Jobs)) @@ -490,6 +498,13 @@ func (c *Compiler) buildCustomJobs(data *WorkflowData, activationJobCreated bool continue } + // Built-in jobs are already created before buildCustomJobs; treat jobs. + // entries as customization-only and do not create duplicate jobs. + if _, exists := c.jobManager.GetJob(jobName); exists { + compilerJobsLog.Printf("Skipping jobs.%s (built-in job already exists)", jobName) + continue + } + if configMap, ok := jobConfig.(map[string]any); ok { job := &Job{ Name: jobName, @@ -809,6 +824,78 @@ func (c *Compiler) buildCustomJobs(data *WorkflowData, activationJobCreated bool return nil } +func (c *Compiler) applyBuiltinJobPreSteps(data *WorkflowData) error { + if data == nil || data.Jobs == nil { + return nil + } + + for jobName, jobConfig := range data.Jobs { + targetJobName := jobName + if jobName == "pre-activation" { + targetJobName = string(constants.PreActivationJobName) + } + + job, exists := c.jobManager.GetJob(targetJobName) + if !exists { + continue + } + + configMap, ok := jobConfig.(map[string]any) + if !ok { + return fmt.Errorf("jobs.%s must be an object, got %T", jobName, jobConfig) + } + if _, hasPreSteps := configMap["pre-steps"]; !hasPreSteps { + continue + } + + preSteps, err := c.extractPinnedJobSteps("pre-steps", jobName, configMap, data) + if err != nil { + return fmt.Errorf("failed to process pre-steps for built-in job '%s': %w", jobName, err) + } + if len(preSteps) == 0 { + continue + } + + job.Steps = insertPreStepsAfterSetupBeforeCheckout(job.Steps, preSteps) + compilerJobsLog.Printf("Inserted %d pre-steps into built-in job '%s'", len(preSteps), targetJobName) + } + + return nil +} + +func insertPreStepsAfterSetupBeforeCheckout(steps []string, preSteps []string) []string { + if len(preSteps) == 0 { + return steps + } + + firstCheckoutIdx := -1 + lastSetupIdx := -1 + for i, step := range steps { + if firstCheckoutIdx == -1 && strings.Contains(step, "uses: actions/checkout@") { + firstCheckoutIdx = i + } + if exactSetupStepIDPattern.MatchString(step) { + lastSetupIdx = i + } + } + + insertIdx := len(steps) + if lastSetupIdx >= 0 { + insertIdx = lastSetupIdx + 1 + } else if firstCheckoutIdx >= 0 { + insertIdx = firstCheckoutIdx + } + if insertIdx > len(steps) { + insertIdx = len(steps) + } + + result := make([]string, 0, len(steps)+len(preSteps)) + result = append(result, steps[:insertIdx]...) + result = append(result, preSteps...) + result = append(result, steps[insertIdx:]...) + return result +} + func (c *Compiler) extractPinnedJobSteps(fieldName string, jobName string, configMap map[string]any, data *WorkflowData) ([]string, error) { raw, hasField := configMap[fieldName] if !hasField { diff --git a/pkg/workflow/compiler_pre_activation_job.go b/pkg/workflow/compiler_pre_activation_job.go index 4a7b286cb19..405979e891c 100644 --- a/pkg/workflow/compiler_pre_activation_job.go +++ b/pkg/workflow/compiler_pre_activation_job.go @@ -471,8 +471,9 @@ func (c *Compiler) extractPreActivationCustomFields(jobs map[string]any) ([]stri // Validate that only steps and outputs fields are present allowedFields := map[string]bool{ - "steps": true, - "outputs": true, + "steps": true, + "outputs": true, + "pre-steps": true, // handled by generic built-in pre-steps insertion in compiler_jobs.go } for field := range configMap { From 3e68d5e143a53a5d06a3b7e20df8301cc4edc282 Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Sun, 19 Apr 2026 13:59:51 +0000 Subject: [PATCH 8/8] docs(adr): add draft ADR-27138 for job-level pre-steps support Generated by Design Decision Gate workflow run 24630708768. --- ...dd-job-level-pre-steps-to-workflow-jobs.md | 96 +++++++++++++++++++ 1 file changed, 96 insertions(+) create mode 100644 docs/adr/27138-add-job-level-pre-steps-to-workflow-jobs.md diff --git a/docs/adr/27138-add-job-level-pre-steps-to-workflow-jobs.md b/docs/adr/27138-add-job-level-pre-steps-to-workflow-jobs.md new file mode 100644 index 00000000000..2dd11175cac --- /dev/null +++ b/docs/adr/27138-add-job-level-pre-steps-to-workflow-jobs.md @@ -0,0 +1,96 @@ +# ADR-27138: Add `jobs..pre-steps` Support for Custom and Built-in Jobs + +**Date**: 2026-04-19 +**Status**: Draft +**Deciders**: pelikhan, Copilot + +--- + +## Part 1 — Narrative (Human-Friendly) + +### Context + +The gh-aw workflow compiler supports a top-level `pre-steps` frontmatter field that injects steps into the agent job before the checkout step. However, workflows can also define additional *custom jobs* and reference *built-in framework jobs* (e.g., `activation`, `pre_activation`) via the frontmatter `jobs` map. No mechanism existed to inject steps at a precise lifecycle position within these per-job step sequences: before checkout or remaining framework steps, but after the compiler-generated setup step. Users who need job-level environment preparation (credential configuration, environment variable injection, pre-flight validation) have no way to insert steps at that specific position without restructuring their entire `steps` list. + +### Decision + +We will add a `pre-steps` field under `jobs.` in the frontmatter schema. For custom jobs, `pre-steps` are inserted after the compiler-generated GHES host configuration step and before any regular `steps`. For built-in jobs (`activation`, `pre_activation`), `pre-steps` are inserted after the step with `id: setup` and before the first `actions/checkout` step. The `pre-activation` key in the `jobs` map is treated as an alias for the internal `pre_activation` built-in job. When both a main workflow and an imported workflow define `pre-steps` for the same job, they are merged deterministically: imported `pre-steps` run first, main workflow `pre-steps` run after. Frontmatter entries that target an already-existing built-in job are treated as customization-only and do not create duplicate jobs. + +### Alternatives Considered + +#### Alternative 1: Positional metadata on individual steps + +A `position: before-checkout` annotation could be added to individual `steps` entries to express ordering intent. This was rejected because it requires users to annotate each step individually, makes the schema more complex, and does not compose cleanly with import merging — the merged order of individually-annotated steps from multiple sources would be ambiguous. + +#### Alternative 2: A separate top-level field per job (e.g., `custom-job-pre-steps`) + +A new top-level frontmatter key could hold a map from job name to pre-step lists. This was rejected because it fragments step configuration across multiple top-level keys and diverges from the established pattern where all per-job configuration lives under `jobs.`. Keeping `pre-steps` nested under `jobs.` makes configuration co-located and consistent with `steps`, `runs-on`, `env`, and other job-level fields. + +#### Alternative 3: Allow only custom jobs to have `pre-steps`, not built-in jobs + +Built-in jobs could remain opaque to `pre-steps` injection, requiring users to define entirely separate custom jobs for any pre-flight logic against built-in job step sequences. This was rejected because built-in jobs such as `activation` and `pre_activation` perform critical lifecycle operations (permission checks, role validation) and legitimate use cases exist for injecting steps at a known position within them (e.g., pre-activation audit logging or environment setup). Treating built-in jobs differently would create an inconsistent extension model. + +### Consequences + +#### Positive +- Users can inject steps at a well-defined lifecycle point within any job — custom or built-in — without restructuring the entire `steps` list. +- The field name (`pre-steps`) mirrors the existing top-level `pre-steps` convention, making the extension point discoverable. +- Import merging preserves contributions from both imported and main workflows; neither silently drops pre-steps defined by the other. +- Built-in job customization via `jobs.` is now explicitly recognized: the compiler skips duplicate job creation when a frontmatter entry targets an already-built-in job. + +#### Negative +- The step insertion logic depends on detecting the `id: setup` marker in the serialized YAML step string, which is a fragile heuristic. If a future compiler change renames or restructures the setup step, the insertion point will silently fall back to appending before the first checkout step. +- The `pre-activation` → `pre_activation` alias adds an implicit name-mapping layer that must be documented and maintained as built-in job names evolve. +- The import merge strategy for `pre-steps` (concatenate rather than let main take precedence) differs from the behavior of all other conflicting job fields, which increases cognitive overhead for users reasoning about import precedence. + +#### Neutral +- The `extractPinnedJobSteps` helper is introduced to share step-extraction and action-pinning logic between `pre-steps` and `steps`, reducing duplication in the compiler. +- The `docs/adr/` filename uses the PR number as the sequence identifier, consistent with existing ADR naming in this repository. + +--- + +## Part 2 — Normative Specification (RFC 2119) + +> The key words **MUST**, **MUST NOT**, **REQUIRED**, **SHALL**, **SHALL NOT**, **SHOULD**, **SHOULD NOT**, **RECOMMENDED**, **MAY**, and **OPTIONAL** in this section are to be interpreted as described in [RFC 2119](https://www.rfc-editor.org/rfc/rfc2119). + +### Frontmatter Schema + +1. Workflows **MAY** define a `pre-steps` field under any `jobs.` entry, containing an array of GitHub Actions step definition objects. +2. The `pre-steps` field **MUST** conform to the same step schema as the `steps` field under the same job (i.e., `$ref` to `jobs.additionalProperties.properties.steps`). +3. Implementations **MUST NOT** treat `jobs..pre-steps` as equivalent to the top-level `pre-steps` field; they operate at different scopes (per-job vs. agent job) and **MUST** be processed independently. + +### Compilation and Placement — Custom Jobs + +1. For custom jobs (jobs not already created as built-in jobs), implementations **MUST** prepend all compiler-generated setup steps (specifically the GHES host configuration step) before `pre-steps`. +2. `pre-steps` **MUST** be placed immediately after the compiler-generated GHES host configuration step and immediately before any `steps` entries. +3. If neither `pre-steps` nor `steps` are defined for a custom job, implementations **MUST NOT** emit the GHES host configuration step. + +### Compilation and Placement — Built-in Jobs + +1. For built-in jobs, implementations **MUST** insert `pre-steps` after the step identified by `id: setup` and before the first step containing `uses: actions/checkout@`. +2. If no step with `id: setup` is present, implementations **MUST** insert `pre-steps` before the first checkout step. +3. If no checkout step is present, implementations **MUST** append `pre-steps` at the end of the existing step list. +4. Implementations **MUST** recognize `pre-activation` as an alias for the `pre_activation` built-in job when processing `jobs..pre-steps`. + +### Duplicate Job Prevention + +1. When a `jobs.` entry in the frontmatter targets an already-created built-in job, implementations **MUST NOT** create a duplicate custom job for that name. +2. Built-in job entries in the `jobs` map **SHOULD** be treated as customization-only; only `pre-steps` (and other explicitly supported customization fields) are applied to the existing built-in job. + +### Import Merge Ordering + +1. When a main workflow and one or more imported workflows both define `pre-steps` under the same `jobs.` key, implementations **MUST** merge the lists rather than letting the main workflow's definition silently discard the imported definition. +2. Merged `pre-steps` **MUST** be ordered: imported `pre-steps` first, followed by main workflow `pre-steps`. +3. For all other conflicting fields under a job entry, the main workflow **MUST** take precedence (existing behavior is preserved). + +### Action Pinning + +1. Implementations **MUST** apply action pin resolution (SHA substitution) to `uses:` references within `pre-steps` entries, consistent with how pinning is applied to `steps`. + +### Conformance + +An implementation is considered conformant with this ADR if it satisfies all **MUST** and **MUST NOT** requirements above. Specifically: custom job `pre-steps` appear after the GHES host step and before `steps`; built-in job `pre-steps` are inserted after the `id: setup` step and before the first checkout step; import merging preserves both imported and main `pre-steps` in the specified order; no duplicate built-in jobs are created; and action pinning is applied to all `pre-steps` entries. Failure to meet any **MUST** or **MUST NOT** requirement constitutes non-conformance. + +--- + +*This is a DRAFT ADR generated by the [Design Decision Gate](https://github.com/github/gh-aw/actions/runs/24630708768) workflow. The PR author must review, complete, and finalize this document before the PR can merge.*