Fix deferred runtime setup for unnamed custom checkout steps#47545
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ Test Quality Sentinel completed test quality analysis. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ PR Code Quality Reviewer completed the code quality review. |
There was a problem hiding this comment.
Pull request overview
Fixes deferred runtime setup after unnamed custom checkout steps.
Changes:
- Adds shared, exact checkout-action detection.
- Inserts runtime setup after the first checkout.
- Expands ordering and false-positive regression tests.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/compiler_workflow_helpers.go |
Adds structured checkout detection; bare snippets remain unsupported. |
pkg/workflow/compiler_yaml_runtime_setup.go |
Inserts setup using the detected checkout index. |
pkg/workflow/permissions_parser_test.go |
Tightens false-positive expectations. |
pkg/workflow/compiler_yaml_main_job_test.go |
Updates unnamed-checkout insertion coverage. |
pkg/workflow/checkout_runtime_order_test.go |
Adds end-to-end ordering variants. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 5/5 changed files
- Comments generated: 1
- Review effort level: Medium
🧪 Test Quality Sentinel Report✅ Test Quality Score: 82/100 — Excellent
📊 Metrics (8 analyzed tests)
Test Coverage Strength✅ Strong behavioral contracts: All three new Go tests validate critical workflow step ordering invariants — checkout position, runtime setup placement, and deferred runtime setup injection. These are high-value regression tests that verify user-visible behavior (step ordering in compiled workflows). ✅ Comprehensive edge cases:
✅ Error assertions abundant: 45+ assertions across the file; most use ✅ No mocking: All tests use actual workflow compilation → file I/O → YAML parsing. This is genuine integration testing of the compiler, validating real end-to-end behavior. Why This Test Score is Trustworthy
Verdict
|
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (259 new lines in 📄 Draft ADR committed:
📋 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. 📋 Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: Fix deferred runtime setup for unnamed custom checkout steps
This is a clean, well-reasoned fix. The core change — replacing fragile substring scanning with proper YAML parsing via findFirstCheckoutStepIndex — eliminates a class of false positives and correctly handles unnamed checkout steps (bare - uses: actions/checkout@... without a name: field).
Key observations:
isCheckoutActionReferenceis correctly strict: onlyactions/checkoutoractions/checkout@*match, rejectingmy-actions/checkoutand comment/run text.- The step-index approach in
addCustomStepsWithRuntimeInsertioncorrectly handles multi-step lookahead by pre-computingcheckoutStepIndexfrom parsed YAML, then matching it against the incrementingcurrentStepIndexas lines are scanned. - Malformed YAML now causes
ContainsCheckoutto returnfalse(previously it matched by substring). This is intentional, explicitly tested, and the right behavior for a safety-first detection. - Test coverage is thorough: named, unnamed, with options, after a deterministic step, multiple checkouts, and custom runtime deduplication.
No blocking issues found.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 46 AIC · ⌖ 4.55 AIC · ⊞ 5K
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — requesting changes on three issues.
📋 Key Themes & Highlights
Issues Found
-
Detection/insertion double-parse with sanitization gap (
compiler_yaml_runtime_setup.go:327):findFirstCheckoutStepIndexis called both before (viaContainsCheckout) and insideaddCustomStepsWithRuntimeInsertion. The inner call happens aftersanitizeAndWarnCustomSteps, so a${{ }}expression in auses:field could cause the outer detection and inner insertion to operate on different strings, reinstating the detection/insertion mismatch this PR is fixing. -
Inner loop block-scalar blindspot (
compiler_yaml_runtime_setup.go:378): The inner loop that drains checkout step lines stops on any--prefixed line at the step indent, but does not trackblockScalarState. Asparse-checkout: |value containing lines like- srcwould prematurely terminate the checkout step drain and insert runtime setup mid-step. -
assertOrderedMarkersposition arithmetic bug (checkout_runtime_order_test.go:562): The helper uses relative offsets that produce incorrect absolute positions whenindex == 0, meaning it may pass when markers appear in the wrong order in edge cases.
Positive Highlights
- ✅ Excellent decision to route all detection through a single YAML-parsing helper, eliminating the substring scan false-positives completely.
- ✅ Pre-computing
checkoutStepIndexvia the same codepath as detection is exactly the right fix for the original bug. - ✅ Comprehensive test matrix in
TestCheckoutRuntimeOrderInCustomStepVariantscovering named/unnamed checkout, insertion ordering, multiple checkouts, and runtime deduplication. - ✅
isCheckoutActionReferencecorrectly handles quotedusesvalues and is case-insensitive — solid defensive coding.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 55.8 AIC · ⌖ 5.14 AIC · ⊞ 6.7K
Comment /matt to run again
There was a problem hiding this comment.
REQUEST_CHANGES — three correctness bugs need fixing before merge
The YAML-parsing-based checkout detection is a solid improvement over substring scanning. However two related bugs in the line-level insertion loop and one bug in the new test helper undermine the fix.
Blocking issues (3)
1. Block scalar content increments step counter (high) — isStepStart in the outer insertion loop is evaluated without checking isBS. Any run: | step whose body contains a - -prefixed line at the step-list indent will falsely increment currentStepIndex, making the runtime insert at the wrong step or not at all.
2. Inner checkout-copy loop fires on block scalar payload (high) — the inner loop that copies the checkout step's remaining lines also lacks a block-scalar guard on the boundary check. A checkout step with a run: | sub-field that contains - -prefixed lines could be silently truncated mid-copy.
3. Off-by-two byte slice in extractAgentJobSection (medium) — agentJobStart+10 skips 10 bytes past " agent:" (8 bytes), corrupting both remainingContent and the final return slice. All new tests in TestCheckoutRuntimeOrderInCustomStepVariants run over a mis-sliced section, so passing tests don't prove what they claim.
🔎 Code quality review by PR Code Quality Reviewer · sonnet46 76.5 AIC · ⌖ 5 AIC · ⊞ 5.7K
Comment /review to run again
Comments that could not be inline-anchored
pkg/workflow/compiler_yaml_runtime_setup.go:365
Block scalar content not guarded from step-index counting: isStepStart is evaluated without checking isBS returned by blockScalarState.update on the same iteration. A run: | step whose body contains a line starting with - at the same indent as the step list will falsely increment currentStepIndex, causing runtime insertion to target the wrong step or be skipped entirely.
<details>
<summary>💡 Suggested fix</summary>
Gate the step-start check on !isBS:
isStepStart := …
</details>
<details><summary>pkg/workflow/compiler_yaml_runtime_setup.go:378</summary>
**Inner checkout-copy loop does not guard block scalar content at step boundary**: the inner loop that copies the checkout step's remaining lines checks `strings.HasPrefix(nextTrimmed, "- ") && nextIndent == stepIndent` to detect the next step's start, but never tracks block scalar state for the lines it processes. A checkout step with a `run: |` sub-field whose content includes `- `-prefixed lines at the step list indent will prematurely terminate the copy, silently truncating the checkout ste…
</details>
<details><summary>pkg/workflow/checkout_runtime_order_test.go:194</summary>
**Off-by-two byte slice corrupts the extracted agent-job section**: `" agent:"` is 8 bytes, but `agentJobStart+10` skips 10 bytes, silently discarding the first 2 characters of the value following the key. This propagates into the `nextJobStart` reconstruction at line 211, making the returned section boundaries consistently wrong and causing order-marker assertions to search over a mis-sliced string.
<details>
<summary>💡 Suggested fix</summary>
Use the correct key length or `strings.Index` …
</details>
|
@copilot please run the Unresolved review threads (newest first):
|
… assertOrderedMarkers fix, comments Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
🔍 PR Triage — §30008120537
Notes: Fixes runtime setup being silently dropped for unnamed custom checkout steps — correctness bug in the compiler. 7 files, 379+/43−. Multiple swe-agent comment cycles indicate iteration; check if resolved.
|
|
@copilot please run the Unresolved review threads (newest first):
Several threads already have a follow-up from
|
All five review threads have been addressed in commits Thread r3637791417 (bare-sequence fallback in Thread r3637841491 (double Thread r3637841508 (block scalar false-positive in inner-loop step-boundary check): Fixed — Thread r3637841517 (malformed YAML policy undocumented): Fixed — Thread r3637841530 ( Local validation:
Hand-off: CI must be re-triggered by a maintainer (close/reopen PR, |
…eference Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
The CI failure was |
|
@copilot please run the Unresolved review threads (newest first):
Failed checks:
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
The All five review threads were addressed in earlier commits:
Local validation ( |
Custom checkout shorthand (
- uses: actions/checkout@...) could trigger runtime deferral during detection but be missed during insertion, dropping required runtime setup from the generated workflow. This only surfaced later at execution time and made named vs unnamed checkout forms behave differently.Unify checkout detection
actions/checkoutexactly instead of relying on substring scans.steps:YAML and bare custom-step snippets used by older call sites.run:text, or unrelated actions whose names containcheckout.Insert runtime setup after the first applicable checkout
- uses:line, not only on followinguses:lines.Expand regression coverage
with:options.Example of the previously broken form now handled equivalently to a named checkout step:
Generated ordering remains:
$