Add jobs.<job-id>.pre-steps support for custom and built-in jobs#27138
Add jobs.<job-id>.pre-steps support for custom and built-in jobs#27138
jobs.<job-id>.pre-steps support for custom and built-in jobs#27138Conversation
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>
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>
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>
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>
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>
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>
|
Great work on adding
|
|
@copilot add tests to check that "activation" pre-steps are properly inserted in activation job. Same for all other "builtin" jobs |
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>
Added coverage and handling for builtin jobs. New test Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
jobs.<job-id>.pre-steps support for custom and built-in jobs
🧪 Test Quality Sentinel ReportTest Quality Score: 85/100✅ Excellent test quality
Test Classification Details
Flagged Tests — Requires ReviewNo tests are flagged as implementation tests. The following minor suggestions are offered to improve coverage and guideline compliance:
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ Test Quality Sentinel: 85/100. Test quality is excellent — 0% of new tests are implementation tests (threshold: 30%). All 4 new tests verify observable behavioral contracts with proper build tags and no mock library usage. Minor suggestions: add error/empty-input cases to the two ordering tests, and add missing assertion messages in TestMergeJobsFromYAMLImports_MergesPreStepsOnConflict.
Generated by Design Decision Gate workflow run 24630708768.
|
Commit pushed:
|
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (456 new lines in AI has analyzed the PR diff and generated a draft ADR to help you get started: 📄 Draft ADR: What to do next
Once an ADR is linked in the PR body, this gate will re-run and verify the implementation matches the decision. Key design decisions captured in the draftThe draft ADR documents the following non-obvious decisions found in the diff:
Why ADRs Matter
ADRs create a searchable, permanent record of why the codebase looks the way it does. Future contributors (and your future self) will thank you. 📋 Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in
References: §24630708768
|
There was a problem hiding this comment.
Pull request overview
Adds support for jobs.<job-id>.pre-steps in frontmatter, including deterministic import-merging and insertion into both custom jobs and selected built-in jobs.
Changes:
- Merge conflicting imported/main job
pre-stepsdeterministically (imported first, then main). - Insert
pre-stepsfor custom jobs (after compiler-injected setup step(s), before the job’s regularsteps). - Add built-in job
pre-stepsinsertion support (includingpre-activationalias mapping) plus tests and docs updates.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/workflow_import_merge.go | Adds deterministic merge logic for pre-steps when imported and main jobs conflict. |
| pkg/workflow/compiler_pre_activation_job.go | Allows pre-steps in pre-activation custom field validation (handled elsewhere for insertion). |
| pkg/workflow/compiler_orchestrator_workflow_test.go | Adds unit test for deterministic pre-steps merge behavior. |
| pkg/workflow/compiler_jobs.go | Implements custom-job and built-in-job pre-steps insertion, plus duplicate built-in job handling changes. |
| pkg/workflow/compiler_jobs_test.go | Adds tests for custom job pre-steps ordering and schema validation; adds step-order helper. |
| pkg/workflow/compiler_builtin_presteps_test.go | Adds test verifying built-in job pre-steps insertion ordering and alias handling. |
| pkg/parser/schemas/main_workflow_schema.json | Extends schema to support jobs.<job-id>.pre-steps. |
| docs/src/content/docs/reference/frontmatter.md | Documents pre-steps and ordering guarantees. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 9/9 changed files
- Comments generated: 3
| 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) | ||
| } |
| // Built-in jobs are already created before buildCustomJobs; treat jobs.<builtin> | ||
| // 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 | ||
| } |
| func assertStepOrderInSection(t *testing.T, section string, orderedSteps ...string) { | ||
| t.Helper() | ||
|
|
||
| prev := -1 | ||
| for _, step := range orderedSteps { | ||
| idx := indexInNonCommentLinesInSection(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 | ||
| } | ||
| } | ||
|
|
||
| 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 | ||
| } |
|
@copilot recompile |
Ran Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
Summary
jobs.<job-id>.pre-stepsin frontmatterpre-stepsafter setup-injected steps and before regularsteps(including checkout when present)pre-stepsdeterministically (imported first, then main)jobs.<builtin-job>.pre-steps:pre-stepsto existing built-in jobs (includingactivationandpre_activation)pre-activationalias mapping topre_activationjobs.<name>targets an already-generated built-in jobValidation
TestCustomJobPreStepsAreInsertedBeforeCheckoutTestCustomJobPreStepsSchemaValidationTestMergeJobsFromYAMLImports_MergesPreStepsOnConflictTestBuiltinJobsPreStepsInsertionOrdermake agent-finishwas run multiple times; it fails only on an external-network-dependent JS live API test in this environment:frontmatter_hash_github_api.test.cjs(Blocked by DNS monitoring proxy)