fix: apply_samples derivePrHeadRef uses target-repo config for siderepo PR lookups#41295
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. |
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
🧪 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
|
@copilot please run the
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
🤖 PR Triage
Score breakdown: Impact 35 · Urgency 20 · Quality 14 Oldest open agent PR (54 h). Three automated reviewers have APPROVED — fix is correct, targeted, and includes a regression test mirroring the existing three-tier resolution pattern in
|
🤖 PR Triage — §28282332784
Rationale: Fixes real 404 errors in side-repo PR workflows. 72h+ old with 3 automated approvals — ready for expedited human review.
|
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.