Repository navigation
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality SentinelOutcome: The agent completed and reported no action. A no-op output alone does not verify that all required work was completed or that publication was possible. Agent messages are omitted from this comment. The category uses structured run status, not interpretation of agent prose.
|
|
✅ PR Code Quality Reviewer completed the code quality review.
|
|
✅ PR Specification Reviewer completed successfully!
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. See the comment below for the result and any generated ADR draft. Design Decision Gate 🏗️Outcome: The agent completed and reported no action. A no-op output alone does not verify that all required work was completed or that publication was possible. Agent messages are omitted from this comment. The category uses structured run status, not interpretation of agent prose.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ Ponytail Reviewer completed successfully!
|
Comment MemoryPeek at saved memory (pr-code-quality-reviewer)Note This comment is managed by comment memory.Expand the saved memory block to view or edit the persistent context for this thread.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
The replacement manifest is written through a destructive unlink-then-write sequence. If the write is interrupted or fails, the runner is again left without action.yml and cannot resolve post-job hooks. Use an atomic replacement or direct overwrite that preserves the existing manifest until the new one is ready.
Highlights
- One high-confidence correctness issue identified in checkout finalization.
- No tests or builds were run; this was a static review.
🔎 Code quality review by PR Code Quality Reviewer · copilot · gpt50 · ~2 AIC · ⌖ ~1 AIC · ⊞ 1.6K
Comment /review to run again
| const actionDir = workspacePath(config.workspace, `.gh-aw/checkouts/${config.index}`); | ||
| try { | ||
| // The runner reloads this composite for checkout post hooks; retain a token-free no-op. | ||
| fs.unlinkSync(path.join(actionDir, "action.yml")); |
There was a problem hiding this comment.
Unlinking the manifest before writing its replacement can leave the runner with no action.yml when the write fails, recreating the post-job reload failure this change is intended to prevent.
💡 Preserve the existing manifest until the replacement is ready
Write the no-op manifest to a temporary file in the same directory and atomically rename it over action.yml (or at least overwrite directly without unlinking first). This keeps a valid manifest available across I/O errors or interruption and avoids a destructive gap.
There was a problem hiding this comment.
Fixed in 2bf6463. Finalization writes an exclusive temporary manifest and atomically renames it over action.yml. Write/rename failure tests verify the original remains intact; a symlink regression verifies its target is not modified.
There was a problem hiding this comment.
Reviewed head 70230e9 against checkout-behavior-specification.md v1.3.4, especially §3.4 and T-CHK-013. No blocking contract violation found: the implementation preserves a valid token-free composite manifest for runner post-job resolution, and the added tests cover empty, single, and multi-repository selections plus token/expression absence.
Proposed specification synchronization (not applied): bump the working draft to v1.3.5 and add this requirement to §3.4:
After runtime checkout finalization, the runtime MUST retain the runtime checkout action directory so that the runner can reload the composite action for post-job checkout hooks. It MUST unlink the active action.yml and replace it with a valid composite-action manifest containing only a token-free no-op step (shell: bash, run: true). The replacement manifest MUST NOT contain resolved credentials, credential-bearing environment references, or runtime checkout expression references. If replacement cannot be completed, finalization MUST fail rather than remove the action directory or leave the prior manifest in place.Add required test T-CHK-022 for this lifecycle across empty, single, and multi selections, and add its §7.2 checklist row and v1.3.5 changelog entry. The PR tests provide the executable evidence for the proposed clarification. No changed specification line provides a valid inline anchor, so this proposal is included in the overall review. Coverage was limited to the captured diff, checkout behavior specification, and related implementation/tests; no code or tests were executed per static-review policy.
Generated by PR Specification Reviewer for #75381 · copilot · gpt50 · ~3 AIC · ⌖ ~1 AIC · ⊞ 1.6K
Comment /review-specs to run again
There was a problem hiding this comment.
What this change does: finish() now leaves a token-free composite action in place after runtime checkout cleanup, so post-job hooks can reload the manifest. The tests cover empty, single-repository, and multi-repository batches plus token and expression absence.
Should fix:
-
Duplicate no-op step definition (
actions/setup/js/checkout_dynamic.cjs:L395-407)- What this is: The empty-batch path already defines the same
bash truestep. - Problem:
finishduplicates that step, so a future change to the required no-op action can leave the post-job replacement and empty-batch manifest inconsistent. - Fix: Define one
NO_OP_STEPconstant and use it in both manifests. - If we skip it: The two paths can drift and one can stop satisfying the composite-action contract.
- What this is: The empty-batch path already defines the same
Nice to have:
-
The shell assertion repeats a literal (
actions/setup/js/checkout_dynamic.test.cjs:L106)- What this is: The preceding assertion already requires the retained step to be exactly
shell: bash, run: true. - Problem: This
execFileSynccall only launches Bash to execute the hard-codedtruethat the test just asserted; it adds no coverage ofcheckout_dynamic.cjs. - Fix: Delete this assertion and keep the exact step assertion.
- If we skip it: The test carries an external shell dependency for a behavior already fully covered by the object equality check.
- What this is: The preceding assertion already requires the retained step to be exactly
Not checked: Test suite execution was not run.
Verdict: fix 1 first.
Lean: -1 lines possible.
Generated by ✂️ Ponytail Reviewer for #75381 · codex · routed: gpt56 max · ~3 AIC · ⌖ ~1 AIC · ⊞ 118
Comment /ponytail to run again
| JSON.stringify({ | ||
| name: "Runtime repository checkout", | ||
| description: "Runtime repository checkout finalized", | ||
| runs: { using: "composite", steps: [{ shell: "bash", run: "true" }] }, |
There was a problem hiding this comment.
What this is: The empty-batch path in checkout_dynamic.cjs already defines the same bash true no-op at line 231. Problem: finish duplicates that step, so a future change to the required no-op action can leave the post-job replacement and empty-batch manifest inconsistent. Fix: Define one NO_OP_STEP constant and use it in both manifests. If we skip it: The two paths can drift and one can stop satisfying the composite-action contract.
There was a problem hiding this comment.
Fixed in 2bf6463. Both the empty-batch and finalized manifests now use the same NO_OP_STEP constant.
| expect(action.description).toBeTruthy(); | ||
| expect(action.runs).toEqual({ using: "composite", steps: [{ shell: "bash", run: "true" }] }); | ||
| expect(manifest).not.toMatch(/token|GH_AW_DYNAMIC|\$\{\{/i); | ||
| expect(execFileSync("bash", ["-c", action.runs.steps[0].run], { encoding: "utf8" })).toBe(""); |
There was a problem hiding this comment.
What this is: The preceding assertion already requires the retained step to be exactly { shell: "bash", run: "true" }. Problem: This execFileSync call only launches Bash to execute the hard-coded true that the test just asserted; it adds no coverage of checkout_dynamic.cjs. Fix: Delete this assertion and keep the exact step assertion. If we skip it: The test carries an external shell dependency for a behavior already fully covered by the object equality check.
There was a problem hiding this comment.
Fixed in 2bf6463. Removed the redundant Bash execution assertion and retained the exact composite-step equality assertion.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd. The fix is sound: the composite manifest is kept as a token-free no-op instead of being deleted, and credential cleanup still runs before it is touched. Two small follow-ups below.
📋 Verification and open items
Verification
- Ran the real
finish()in plain Node 24 against temp workspaces for empty, single-repo, and multi-repo batches. Each keeps a parseable manifest with the expectedruns, no token or${{references, and abashstep that runs cleanly. A missing manifest still fails with the renamed error. - The vitest suite could not run here: npm registry access fails on a self-signed certificate, so
checkout_dynamic.test.cjsis unverified. Please confirm CI is green.
Open items
.gh-aw/checkouts/N/action.ymlnow persists afterfinish(). The main patch path diffs base-to-tip, so it should not be staged.apply_samples.cjsrunsgit add -Ain a repo cwd, though, so confirm that path never runs in a workspace that holds this file.
Positive highlights
- ✅ Lifecycle fix is minimal and leaves
cleanCheckoutCredentialsunchanged. - ✅ Tests cover empty, single, and multi batches and assert no token leakage.
@copilot please address the review comments above.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · routed: haiku55 xhigh · ~5 AIC · ⌖ ~1 AIC · ⊞ 5.4K
Comment /matt to run again
| expect(action.runs.steps).toEqual([{ shell: "bash", run: "true" }]); | ||
| }); | ||
|
|
||
| it.each(["", "org/a", "org/a\norg/b"])("keeps a token-free no-op composite available for post-job resolution: %s", async repositories => { |
There was a problem hiding this comment.
[/tdd] The it.each title renders the empty case as nothing and the multi-line case as a literal newline, which makes failures hard to identify. Use labelled cases, for example it.each([["empty", ""], ["single", "org/a"], ["multi", "org/a\norg/b"]]) with (name, repositories) in the callback, so the title reads as a specification.
@copilot please address this.
There was a problem hiding this comment.
Fixed in 2bf6463. The parameterized cases now have explicit empty, single, and multi labels, with batch names in the test titles.
| const actionDir = workspacePath(config.workspace, `.gh-aw/checkouts/${config.index}`); | ||
| try { | ||
| // The runner reloads this composite for checkout post hooks; retain a token-free no-op. | ||
| fs.unlinkSync(path.join(actionDir, "action.yml")); |
There was a problem hiding this comment.
[/diagnosing-bugs] unlinkSync followed by flag: "wx" looks redundant but both are load-bearing: wx fails when the path exists, so the unlink is what permits the rewrite, and wx makes the write fail rather than follow anything re-created in the gap. A one-line comment here would stop a future cleanup from dropping either, since that makes every finalize fail with EEXIST.
@copilot please address this.
There was a problem hiding this comment.
Addressed in 2bf6463 by replacing unlink-then-write with atomic rename, superseding this sequence. A comment explains preservation on failure and symlink safety; exclusive creation is retained for the temporary manifest.
There was a problem hiding this comment.
🟡 Changes recommended
The retained runner-owned manifest is not Git-ignored, so staging can cause safe_outputs collisions and cleanup can delete it, reintroducing post-job failures.
What changed in this PR
This PR preserves generated runtime checkout manifests so GitHub Actions can resolve post-job hooks for multi-repository checkouts in agent and safe-output jobs, addressing #75199.
Changes:
- Replace manifest deletion with a token-free no-op composite, leaving credential cleanup unchanged.
- Add regression coverage for empty, single-repository, and multi-repository batches, and update manifest-retention assertions.
| File | Description |
|---|---|
actions/setup/js/checkout_dynamic.test.cjs |
Verify retained manifests are valid, token-free no-ops across batch sizes. |
actions/setup/js/checkout_dynamic.cjs |
Retain a minimal composite manifest for post-job resolution. |
Review effort: Max (auto)
| { flag: "wx" } | ||
| ); |
|
Copilot assignment requested for the following outstanding work:
Once you start work, post this work summary as a PR comment without the leading Copilot mention, including the Sous-chef head, work, and state lines below, to record that the assignment is running. Apply the necessary fixes on this PR's existing branch, reply to each listed review thread and resolve it when addressed. Ignore feedback already answered or resolved; stop when only human review or CI remains. Do not merge or trigger CI. Sous-chef requested-head: 70230e9
|
Assignment running:
Sous-chef requested-head: 70230e9
Addressed in 2bf6463: labelled regression cases, atomic replacement with an explanatory comment, and a self-ignoring runtime directory. A real Git test verifies staging excludes the manifest and clean/stash leave it intact. All 187 checkout tests, typecheck, lint, and the final validation gate pass; the existing CJS CI run 42237290312 also passed. Latest main was merged in cfa6ecb. Thread resolution is not supported by the available PR tools. |
…i-fail-fix 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>
checkout: multi: truejobs fail during post-checkout cleanup becausefinish()deletes the composite manifest the runner must reload. This affects both agent and safe-output jobs, even when their work succeeds.