Fix built-in pre-steps insertion to avoid splitting Setup Scripts YAML blocks#27750
Merged
Fix built-in pre-steps insertion to avoid splitting Setup Scripts YAML blocks#27750
pre-steps insertion to avoid splitting Setup Scripts YAML blocks#27750Conversation
7 tasks
Agent-Logs-Url: https://github.com/github/gh-aw/sessions/b186c272-d416-4042-b387-29d1ffe56add Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Agent-Logs-Url: https://github.com/github/gh-aw/sessions/b186c272-d416-4042-b387-29d1ffe56add Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…ecks Agent-Logs-Url: https://github.com/github/gh-aw/sessions/b186c272-d416-4042-b387-29d1ffe56add Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Copilot
AI
changed the title
[WIP] Fix YAML splicing corruption in jobs.pre_activation and jobs.activation
Fix built-in Apr 22, 2026
pre-steps insertion to avoid splitting Setup Scripts YAML blocks
Contributor
🧪 Test Quality Sentinel ReportTest Quality Score: 90/100✅ Excellent
Test Classification Details
Flagged Tests — Requires ReviewNo tests require review. The single new test is a well-structured design test. Test Inflation NoteThe test file added 93 lines against 16 lines in the production fix (ratio ≈ 5.8×), triggering the inflation flag. However, this is expected and appropriate for an end-to-end regression test for a YAML generation bug: the test must create a temp directory, write a workflow file, invoke the real compiler, parse the generated lock file, extract job sections, and assert step ordering for two distinct jobs. The verbosity reflects thorough coverage, not padding. No points deducted beyond the automatic 10-point inflation penalty (which still leaves the score at 90/100). Test Analysis:
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
✅ Test Quality Sentinel: 90/100. Test quality is excellent — 0% of new tests are implementation tests (threshold: 30%). The single new test TestPreStepsInsertAfterSetupBoundary is a well-structured end-to-end design test that enforces the behavioral contract of the fix and would catch a regression if deleted.
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes built-in job pre-steps injection so it inserts at a YAML step boundary (after the full “Setup Scripts” step block), preventing malformed YAML when setup is emitted as multiple lines.
Changes:
- Update
insertPreStepsAfterSetupBeforeCheckoutto scan for the next-step-item boundary afterid: setupbefore inserting pre-steps. - Add a regression test to ensure built-in
pre_activationandactivationpre-steps are inserted after setup and before downstream built-in/checkout steps, and that the generated lock YAML parses.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/compiler_jobs.go | Changes pre-step insertion logic to locate a real YAML step boundary after the setup step. |
| pkg/workflow/compiler_jobs_test.go | Adds regression coverage for built-in job pre-step insertion ordering and YAML validity. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 2/2 changed files
- Comments generated: 1
| t.Fatal(err) | ||
| } | ||
|
|
||
| compiler := NewCompiler() |
There was a problem hiding this comment.
This test’s assertions depend on the setup step being emitted in action mode (it searches for job-name: ${{ github.job }}), so it can fail if the environment forces GH_AW_ACTION_MODE=script (script mode uses INPUT_JOB_NAME instead). To keep the test deterministic, explicitly set the compiler action mode (e.g. compiler.SetActionMode(ActionModeDev)) or temporarily unset/restore GH_AW_ACTION_MODE within the test before calling CompileWorkflow.
Suggested change
| compiler := NewCompiler() | |
| compiler := NewCompiler() | |
| compiler.SetActionMode(ActionModeDev) |
Contributor
|
Pull request created: #27758 Caution Security scanning requires review for Design Decision Gate 🏗️ DetailsPotential security threats were detected in the agent output. The workflow output should be reviewed before merging. Review the workflow run logs for details.
|
Contributor
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (109 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. 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. 📋 Draft ADR Summary — What the AI inferredDecision: Determine the pre-steps insertion index by scanning forward from the last setup-step line to the next YAML step-item boundary (a line starting with Driver: The steps slice uses raw YAML strings where one logical step spans multiple entries. Inserting at Alternatives considered:
Key trade-off noted: String-scanning for YAML structure is fragile (a 📋 Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in
Caution Security scanning requires review for Design Decision Gate 🏗️ DetailsPotential security threats were detected in the agent output. The workflow output should be reviewed before merging. Review the workflow run logs for details.
|
pelikhan
reviewed
Apr 22, 2026
|
|
||
| "github.com/github/gh-aw/pkg/constants" | ||
| "github.com/github/gh-aw/pkg/testutil" | ||
| "go.yaml.in/yaml/v3" |
Contributor
Author
There was a problem hiding this comment.
Updated in 7877754 — pkg/workflow/compiler_jobs_test.go now uses the existing github.com/goccy/go-yaml import to align with the workflow test package conventions.
Agent-Logs-Url: https://github.com/github/gh-aw/sessions/f467ec20-239b-4f4f-ae0e-67c61885bdd3 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Contributor
Author
|
Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details. 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:
|
This was referenced Apr 22, 2026
Contributor
There was a problem hiding this comment.
💥 Automated smoke test review - all systems nominal! This PR correctly addresses the YAML block splitting issue by finding step boundaries rather than blindly inserting after lastSetupIdx + 1.
Note
🔒 Integrity filter blocked 1 item
The following item was blocked because it doesn't meet the GitHub integrity level.
- #27750
pull_request_read: has lower integrity than agent requires. The agent cannot read data with integrity below "approved".
To allow these resources, lower min-integrity in your GitHub frontmatter:
tools:
github:
min-integrity: approved # merged | approved | unapproved | none💥 [THE END] — Illustrated by Smoke Claude · ● 250.8K
| // If no boundary is found (e.g. setup is the final step), insertIdx stays len(steps) | ||
| // and pre-steps are appended by the slice insertion logic below. | ||
| for i := lastSetupIdx + 1; i < len(steps); i++ { | ||
| trimmed := strings.TrimLeft(steps[i], " ") |
Contributor
There was a problem hiding this comment.
Consider using strings.TrimSpace instead of strings.TrimLeft(steps[i], " ") to handle tabs as well as spaces, since YAML indentation could theoretically use tabs (though in practice YAML prohibits tabs in indentation).
| break | ||
| } | ||
| } | ||
| if insertIdx == len(steps) { |
Contributor
There was a problem hiding this comment.
The log message here is helpful for debugging. Consider also logging the lastSetupIdx value to make it easier to correlate this log entry with the specific step being processed.
Collaborator
|
@copilot add more tests |
Agent-Logs-Url: https://github.com/github/gh-aw/sessions/670bfc30-e990-48ea-b4e6-9b68da2e2b04 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Agent-Logs-Url: https://github.com/github/gh-aw/sessions/670bfc30-e990-48ea-b4e6-9b68da2e2b04 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Agent-Logs-Url: https://github.com/github/gh-aw/sessions/670bfc30-e990-48ea-b4e6-9b68da2e2b04 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Contributor
Author
Added more coverage in b360dcc. I included additional tests for setup-final append behavior, checkout fallback (including 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:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
jobs.pre_activation.pre-stepsandjobs.activation.pre-stepscould be injected in the middle of the generatedSetup Scriptsstep, producing malformed YAML (duplicateuses/withkeys). This change moves insertion to a true step boundary so pre-steps land after setup and before checkout/next built-in step.Compiler fix: insert at step boundary, not
id: setup + 1insertPreStepsAfterSetupBeforeCheckoutinpkg/workflow/compiler_jobs.go.id: setupand inserted on the next slice entry, which can still be inside the same YAML step block.-after indentation trim) and inserts there.Checkout fallback boundary fix (when setup step is absent)
uses: actions/checkout@..., insertion now resolves to the nearest enclosing-step-item boundary so pre-steps are inserted before the full checkout step.Regression coverage for built-in jobs
TestPreStepsInsertAfterSetupBoundaryinpkg/workflow/compiler_jobs_test.go.pre_activationwith auses:pre-stepactivationwith arun:pre-stepExpanded unit coverage for insertion helper
TestInsertPreStepsAfterSetupBeforeCheckouttable-driven tests for:- uses:) casepre-stepsno-opstepsinsertionExample of the boundary behavior
> [!WARNING]
>
>