fix: apply_samples derivePrHeadRef uses target-repo config for siderepo PR lookups#41295
fix: apply_samples derivePrHeadRef uses target-repo config for siderepo PR lookups#41295Copilot wants to merge 2 commits into
Conversation
…derepo PR lookups (#41292) Co-authored-by: dsyme <7204669+dsyme@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Fixes deterministic sample replay for siderepo push_to_pull_request_branch workflow_dispatch runs by ensuring PR head-ref lookups target the configured side repository (instead of defaulting to the host GITHUB_REPOSITORY, which can produce 404s).
Changes:
- Updates
derivePrHeadRefto resolve the repo slug via:entry.arguments.repo→ safe-outputs configtarget-repo→GITHUB_REPOSITORY. - Adds a regression test covering siderepo
pull_request_numbersamples without an explicitrepooverride. - Adds a changeset documenting the fix.
Show a summary per file
| File | Description |
|---|---|
| actions/setup/js/apply_samples.cjs | Uses per-tool target-repo from the safe-outputs config when building the PR API URL for head-ref derivation. |
| actions/setup/js/apply_samples.test.cjs | Adds coverage ensuring the PR fetch targets the configured siderepo, not the host repo. |
| .changeset/fix-apply-samples-siderepo-target-repo.md | Records the bugfix and its impact for release notes/versioning. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 3/3 changed files
- Comments generated: 0
- Review effort level: Low
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #41295 does not have the 'implementation' label and has 0 new lines of code in business logic directories (threshold: 100). |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Test Quality Sentinel completed test quality analysis. TQS analysis for PR #41295 already completed in the prior session of this workflow run (28237417912). Comment and APPROVE review were successfully submitted then. No further actions required. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnose, /tdd, and /zoom-out — approving with two non-blocking suggestions.
📋 Key Themes & Highlights
Key Themes
- Tier-1 priority untested: The new middle tier is proven to work, but no test verifies that an explicit
entry.arguments.repostill beats the configtarget-repo. A future refactor could swap priorities silently. - Style asymmetry: The
||-chained assignment inderivePrHeadRefdiverges from theif/elsestyle used for the same logic inresolvePatchWorkspace.
Positive Highlights
- ✅ Surgical 1-line fix that correctly mirrors the existing three-tier logic from
resolvePatchWorkspace— minimal blast radius. - ✅ Test explicitly asserts both the positive (side-repo URL) and negative (host-repo URL not called) — good regression hygiene.
- ✅ Resolution order is clearly documented in the comment block preceding the changed line.
- ✅ Changeset entry is accurate and references the issue number.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 51.1 AIC · ⌖ 7.18 AIC · ⊞ 6.5K
| else process.env.GITHUB_REPOSITORY = prevRepo; | ||
| if (prevConfig === undefined) delete process.env.GH_AW_SAFE_OUTPUTS_CONFIG_PATH; | ||
| else process.env.GH_AW_SAFE_OUTPUTS_CONFIG_PATH = prevConfig; | ||
| } |
There was a problem hiding this comment.
[/tdd] Missing edge-case: arguments.repo overriding the config target-repo is untested.
The new test correctly covers tier-2 (config → side repo). But there is no assertion that tier-1 (arguments.repo) still wins when both are set. Without it, a future refactor could silently break the priority order.
💡 Suggested complementary test
it("entry.arguments.repo takes precedence over target-repo in safe-outputs config", async () => {
// setup: write config with target-repo, but also set entry.arguments.repo
const entry = {
tool: "push_to_pull_request_branch",
arguments: { repo: "explicit/override-repo", pull_request_number: 123 },
sidecars: { patch: newFileDiff("src/file.py", "# content\n") },
};
await preStagePatch(entry, 0, workspace);
// tier-1 explicit repo must win over config target-repo
expect(fetchSpy).toHaveBeenCalledWith(
expect.stringContaining("/repos/explicit/override-repo/pulls/123"),
expect.anything()
);
expect(fetchSpy).not.toHaveBeenCalledWith(
expect.stringContaining("/repos/githubnext/gh-aw-side-repo/pulls/123"),
expect.anything()
);
});| // for the tool — covers siderepo workflow_dispatch where the sample arguments | ||
| // carry `pull_request_number` but not a `repo` override (issue #41292). | ||
| // c. GITHUB_REPOSITORY — host repo fallback. | ||
| const repoSlug = (typeof entry.arguments.repo === "string" && entry.arguments.repo.trim()) || readConfiguredTargetRepo(entry.tool) || process.env.GITHUB_REPOSITORY || ""; |
There was a problem hiding this comment.
[/zoom-out] The single-line || chain is harder to scan than the equivalent if/else block used in resolvePatchWorkspace (lines 308–312).
Keeping the two functions stylistically consistent would lower cognitive load when future contributors need to modify the resolution order.
💡 Suggested style alignment with resolvePatchWorkspace
let repoSlug = "";
if (typeof entry.arguments.repo === "string" && entry.arguments.repo.trim()) {
repoSlug = entry.arguments.repo.trim(); // a: explicit per-sample override
} else {
repoSlug = readConfiguredTargetRepo(entry.tool); // b: config target-repo
}
if (!repoSlug) {
repoSlug = process.env.GITHUB_REPOSITORY || ""; // c: host repo fallback
}This matches the three-tier structure that is already documented in the comment block above.
🧪 Test Quality Sentinel Report✅ Test Quality Score: 90/100 — Excellent
📊 Metrics & Test Classification (1 test analyzed)
Go: 0 ( i️ Inflation NoteTest file inflation (6.9:1): The test added 55 lines against 8 production lines. The 10-point "Proportional Growth" component is zeroed per the binary inflation rule. However, the verbose line count is almost entirely driven by necessary test infrastructure — Verdict
|
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 new regression test covers the behavioral contract directly: correct repo slug in the PR fetch URL, and explicitly asserts the wrong (host) repo is not used.
There was a problem hiding this comment.
Fix is correct, targeted, and mirrors the existing three-tier pattern in resolvePatchWorkspace. No blocking issues.
### Review summary
What was reviewed: The single-line change inserting readConfiguredTargetRepo(entry.tool) as a middle tier in derivePrHeadRef's repo-slug resolution, plus the regression test.
Correctness: ✅ The fix correctly intercepts the case where a workflow_dispatch sample carries pull_request_number but no explicit repo override and the safe-outputs config has target-repo set. Both path 2 (issue-comment events) and path 3 (explicit pull_request_number) now benefit from the fix because repoSlug is resolved once before both branches.
Test: ✅ The new test faithfully reproduces the 404 scenario — host repo as GITHUB_REPOSITORY, side repo only in config — and asserts the correct URL is fetched. One non-blocking gap noted inline: path 2 (issue-comment event with payload.issue.pull_request) is not exercised.
Maintainability note: resolvePatchWorkspace and derivePrHeadRef now independently implement the same three-tier chain with slightly different syntax. A small resolveTargetRepo(entry) helper would make the shared contract explicit and prevent silent re-divergence in future edits.
Security: readConfiguredTargetRepo reads from workflow-controlled bootstrap config, not from agent output. The use in derivePrHeadRef is read-only (one GitHub API call). No new attack surface introduced.
🔎 Code quality review by PR Code Quality Reviewer · 88.5 AIC · ⌖ 6.94 AIC · ⊞ 5.2K
| expect(git(["rev-parse", "--abbrev-ref", "HEAD"], workspace).trim()).toBe(headRef); | ||
| // Must fetch from the side repo, NOT the host repo. | ||
| expect(fetchSpy).toHaveBeenCalledWith(expect.stringContaining("/repos/githubnext/gh-aw-side-repo/pulls/447"), expect.anything()); | ||
| expect(fetchSpy).not.toHaveBeenCalledWith(expect.stringContaining("/repos/githubnext/gh-aw-test/pulls/447"), expect.anything()); |
There was a problem hiding this comment.
The new test exercises path 3 of derivePrHeadRef (pull_request_number with no event payload). Path 2 — issue_comment/pull_request_review_comment events where payload.issue.pull_request is set — also resolves through the same repoSlug and would have fetched from the wrong repo before this fix.
💡 Suggested complementary test
Add a second it block that populates GITHUB_EVENT_PATH with an issue-comment payload containing issue.pull_request and issue.number, sets push_to_pull_request_branch.target-repo in the config, and asserts that fetchSpy hits githubnext/gh-aw-side-repo/pulls/<issue.number>. This nails down both paths that consume repoSlug and guards against any future regression specific to the event-driven flow.
The current test already does the heavy lifting; this would be a near-copy with a different event payload wired in.
|
|
Siderepo
push_to_pull_request_branchworkflow_dispatch samples fail with HTTP 404 becausederivePrHeadRefbuilds the PR fetch URL fromGITHUB_REPOSITORY(the host repo) instead of the side repo declared in the safe-outputs config.Changes
actions/setup/js/apply_samples.cjs— InsertreadConfiguredTargetRepo(entry.tool)as a middle tier inderivePrHeadRef's repo-slug resolution chain, between the explicitentry.arguments.repooverride and theGITHUB_REPOSITORYfallback. This mirrors the identical three-tier logic already used inresolvePatchWorkspace.Previously, samples carrying
pull_request_numberbut norepoargument (the normal agent output for siderepo dispatch) always hit tier 3, fetching from the wrong repo.actions/setup/js/apply_samples.test.cjs— Add test asserting that whenpush_to_pull_request_branch.target-repois set in the config, the PR fetch URL targets the side repo (githubnext/gh-aw-side-repo/pulls/447) rather than the host repo.