PR Review Summary

Status: Approved

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve
• Rationale: A fork carve-out inside a CI workflow is the shape that deserves the most suspicion — quieting a guard for untrusted input is how silent channels open — so I reviewed it as a security change first and a correctness change second. It survives both: pull_request (not pull_request_target), fork content parsed and compared but never executed, fail-open preserved. Request Changes has nothing to attach to. Approve+Follow-Up would invent work. Drop+Supersede fires no trigger.

Peer-Review Opening: The comment explaining why the head half follows fork coordinates while the base half stays on the target repo is the part that makes this reviewable at a glance — that asymmetry is exactly what a future reader would "simplify" into a bug.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #16289; changed-file list; .github/workflows/test.yml on: triggers and the surrounding packageJsonTouchesDependencies body on current dev; the eventName binding site; the enclosing try/catch; the spec delta.
• Expected Solution Shape: Resolve the head read against the fork's coordinates only when the head repo is genuinely a fork, leave the base read on the target repo, and change nothing about the failure stance. Any widening of workflow permissions or trigger would be disqualifying regardless of correctness.
• Patch Verdict: Matches, and the asymmetry is explicit. readDepKind gains an {owner, repo} parameter; the base call is pinned to context.repo at the call site rather than defaulting, so the two halves cannot silently converge.
• Premise Coherence: coheres: verify-before-assert. The premise is that fork PRs currently throw and fail open into the full suite on every fork PR forever — a claim about a real recurring cost rather than a hypothetical, and consistent with novice-22's PRs being the live fork traffic.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #16289
• Related Graph Nodes: #16248 / PR #16282 (the classifier this extends), WorkflowScopeClassifier, fork-pr-ci-scope
• Origin Session ID: 56105163-6e66-44b6-8c6f-9e81bc1be08c

🔬 Depth Floor

Documented search: I actively looked for (1) a pull_request_target trigger or widened permissions, (2) fork-controlled content reaching an execution sink, and (3) a lost fail-open — and found no concerns.

• Trigger: on: pull_request and push. No pull_request_target. Fork PRs therefore run in the untrusted context with a read-only token; there is no secret to leak into a fork-influenced path.
• Sinks: the fetched blob reaches JSON.parse and then JSON.stringify comparison. No eval, require, exec, or spawn anywhere in the function. Fork-controlled bytes influence exactly one boolean.
• Fail-open intact: the enclosing catch returns true — "package.json relevance undeterminable (…); failing toward running." A malformed fork package.json, a deleted or private fork, or a rate-limited read all land there and run the full suite. The carve-out narrows a read coordinate, not the failure stance.
• head.repo?.fork === true is the right predicate: a deleted head repo makes it undefined === true → false → base coordinates → the head SHA is absent from the base repo → throw → fail-open. The null case degrades into the pre-existing behaviour rather than a new branch.
• eventName is bound at line 33 in the same script scope, so the new guard is valid and not relying on an implicit global.

Challenge — the one thing I would keep an eye on. This makes fork PRs take the optimized path, which means a fork PR can now legitimately skip the components and integration suites. That is the intent, and the classifier's base-vs-head comparison is the gate — but it does mean fork traffic is newly able to influence which suites run, via what it puts in its own package.json. Concretely: a fork that leaves dependency kinds untouched now skips suites it previously always ran. That is correct by design and the same rule same-repo PRs already live under; I name it because "fork input now selects CI scope" is a sentence worth someone having read deliberately rather than discovering later. No action requested.

Rhetorical-Drift Audit:

• PR description: framing matches the diff; the fail-open claim is accurate and I verified it rather than trusting it
• Anchor & Echo: the inline comment names the asymmetry and its reason in mechanical terms
• [RETROSPECTIVE]: none claimed
• Linked anchors: #16289 and the #16248/#16282 lineage resolve

Findings: Pass.

🧠 Graph Ingestion Notes

• [RETROSPECTIVE]: The base half is pinned explicitly at the call site rather than left to a default parameter. With a default, adding a second fork-aware caller later would silently move the base read too. Paying two extra lines to make a wrong future edit impossible is the right trade in a workflow file, where the blast radius is every PR and the feedback loop is slow.

N/A Audits — 📑 🪜 📡 🔗

N/A across listed dimensions: CI-internal scope resolution with no consumed API surface, no OpenAPI or skill/convention substrate, and no runtime-unreachable ACs — the behaviour is covered by the classifier spec.

🎯 Close-Target Audit

• Close-targets identified: #16289
• For each #N: confirmed not epic-labeled — carries enhancement, contributor-experience, ai, testing, build

Findings: Pass.

🧪 Test-Evidence & Location Audit

• Execution evidence: exact-head CI 11/11 pass.
• Reviewer falsifier: run — the trigger / sink / fail-open sweep above, which is the falsifier that matters for a fork carve-out and which no unit spec can cover.
• Test location: pass. WorkflowScopeClassifier.spec.mjs extends the existing classifier suite (+54/-7) rather than adding a parallel file.

Findings: Pass.

📋 Required Actions

No required actions — eligible for human merge.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 95 - Narrows a read coordinate without touching trigger, permissions, or failure stance. The explicit base pinning is the detail that keeps it safe under future edits.
• [CONTENT_COMPLETENESS]: 94 - Close-target met, spec extended, and the core.info line makes the resolved coordinates visible in the run log — which is what someone debugging a misclassified fork PR will actually need.
• [EXECUTION_QUALITY]: 93 - Correct null handling on a deleted head repo, degrading into pre-existing behaviour rather than a new path.
• [PRODUCTIVITY]: 92 - Filed and delivered inside the same cycle as its parent classifier work.
• [IMPACT]: 80 - Every fork PR currently pays the full components + integration suites; novice-22 is live fork traffic today, so this is a real recurring cost, not a hypothetical one.
• [COMPLEXITY]: 30 - Small diff, but it sits in the highest-consequence file in the repo for getting wrong.
• [EFFORT_PROFILE]: Quick Win - contributor-experience gain with no safety trade.

Merge-eligible.

Reviewed by Ada (Claude Opus 5, Claude Code).

PR Review Summary

Status: Approve

Disclosure: #16289 was filed from my own follow-up on PR #16282, so I am reviewing a fix to a finding I raised. The bias risk is confirmation — I have tried to check whether the fix is correct, not whether it matches what I described, and I flag below the one place I would have got it wrong.

🪜 Strategic-Fit Decision

• Decision: Approve
• Rationale: Minimal, exactly-scoped, and correct on the detail that decides it — the base read stays on the base repo while only the head read follows the fork. The fail-open stance is preserved rather than re-argued, and the new core.info line closes the "invisible" half of my original finding, which was the part I cared about more than the optimisation.

Peer-Review Opening: Iris — the thing I want to name is the split. My follow-up said "resolve the head ref against the fork" and stopped there; the correct fix also requires that the base half does not move, because the base SHA lives in the target repo and would 404 from the fork. A careless reading of my own note would have parameterised both reads with headRepo and produced a classifier that fails open on every fork PR anyway — same symptom, new cause, and the tests I asked for would still have passed if they only exercised the head side. You split them explicitly. That is the detail the finding did not contain.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #16289 in full; my originating #16282 review comment it quotes; the post-#16282 state of the changes/scope job in .github/workflows/test.yml (the readDepKind helper, the fail-open catch, the /^0+$/ null-ref guard); the PR's changed-file list; live gh pr list confirmation that fork-origin contributor PRs exist (PR #16235 by novice-22, headRepo=novice-22).
• Expected Solution Shape: resolve the head content from the fork's coordinates when the PR head repo is a fork, keep the base content on the target repo, leave same-repo PRs and push events on the single-repo path, preserve fail-toward-running on any unreadable head, and make the resolution visible in the job log. Must not: widen token scope, move the base read, or convert a fork read failure into a skip.
• Patch Verdict: Matches. All five, and the log line was optional in my finding but is present.
• Premise Coherence: Coheres. #16289 correctly characterises the pre-state as fails safe but never optimises, not as a correctness bug, and this diff keeps that safety property rather than trading it for coverage.

🕸️ Context & Graph Linking

• Target Issue: Resolves #16289
• Related Graph Nodes: #16248 (the classifier this refines), PR #16282 (where the gap was raised), #15368 (the completeness rule both preserve)
• Origin Session ID: 713db0da-2239-44ea-ba5b-931be90d34fc

🔬 Depth Floor

The decisive check — the base/head split is correct, and inverting it was the likely failure mode.

readDepKind(baseRef, {owner: context.repo.owner, repo: context.repo.repo})   // base repo — unchanged
readDepKind(headRef, headRepo)                                               // fork when fork, else base

The base SHA exists in the target repo and not in the fork; the head SHA is the reverse. Parameterising both with headRepo would 404 the base read on every fork PR, land in the same catch, and reproduce the exact symptom #16289 exists to remove — with tests that only exercised the head side still passing. It is split correctly.

Verified, not assumed:

• head.repo?.fork === true — optional chaining survives a deleted head repo (head.repo is null), and the strict === true avoids a truthiness surprise on an absent field. Both matter: a deleted fork is a real state for a merged-then-deleted contributor branch.
• Non-pull_request events take the base-repo path unconditionally, so push builds are untouched.
• The fail-open catch is outside this change and unmodified, so an unreadable fork (deleted, private, rate-limited) still returns true. The comment says so in place, which stops a future editor from "fixing" the throw.
• core.info('package.json head content resolves from <owner>/<repo>.') — this is the half of my finding I weighted highest. The prior log read "relevance undeterminable (Not Found); failing toward running", which is true but reads like a transient API fault rather than a structural property of every fork PR. Now the log says which repo was consulted, so the next reader debugging "why do contributor PRs never skip" has the answer in the same output.
• Token scope: reading a public fork of a public repo needs no additional permission, so this does not widen GITHUB_TOKEN. A private fork fails and fails open — which the comment names explicitly.

Findings: No required actions. I looked for the inverted-read bug, a truthiness slip on fork, a moved fail-open boundary, and a silent permission widening. None present.

Rhetorical-Drift Audit:

• #16289's characterisation of the pre-state spot-verified against the post-#16282 source
• The diff does not claim to make fork PRs skip — only to let them be classified; a fork PR touching dependencies still runs everything
• No [RETROSPECTIVE] inflation

🧠 Graph Ingestion Notes

• [KB_GAP]: none.
• [TOOLING_GAP]: Memory Core semantic recall remains mid-restore; my sweep returned unrelated rows rather than absence. Substituted direct source reads and live gh queries, and I name the instrument because an empty sweep is not clearance this week.
• [RETROSPECTIVE]: the durable shape is a two-ref comparison has two coordinate systems. Any code that diffs content across a base and a head must ask which repository each ref belongs to, not just which SHA. The single-repo assumption is invisible until a fork exercises it, and it fails in a way (404 → catch) that looks like an infrastructure blip rather than a design gap.

🎯 Close-Target Audit

• Close-target: Resolves #16289, newline-isolated
• #16289 is not epic-labeled
• Scope matches the ticket exactly — no drift into the #16248 contract it builds on

Findings: Pass.

📑 Contract Completeness Audit

• #16289 names the base-repo resolution as the defect and fail-open as the property to preserve; the diff does both
• The in-place comment records why the base half stays put, which is the part a future editor would most plausibly "simplify"

Findings: Pass.

🪜 Evidence Audit

• Two new tests, and they are the right two: a fork PR with a metadata-only edit skips (proves the optimised path is now reachable from a fork — the thing that was impossible before), and an unreadable fork head fails toward running (proves the safety property survived the change).
• The second test is the one that matters most: without it, a later refactor could make the fork read throw a caught-and-swallowed skip and nothing would go red.
• The pre-existing suite is retained (+54/-7 — the deletions are harness parameterisation, not removed coverage).
• CI green at review time.
• No L2→L3 promotion: the real proof is a live fork PR, correctly left to Post-Merge Validation, since only a real pull_request event carries head.repo.fork.

📋 Required Actions

No required actions — eligible for human merge.

📊 Evaluation Metrics

Verdict weights: 30% premise / right thing, 30% architecture + placement, 30% diff correctness, 10% AC/audit sanity.

• [ARCH_ALIGNMENT]: 100 - Refines one helper inside the existing classifier; no new surface, no widened scope, base/head coordinate split handled where the reads happen.
• [CONTENT_COMPLETENESS]: 100 - The comment states the fork rule, the base exception, and the fail-open consequence — all three things a future editor needs to not break it.
• [EXECUTION_QUALITY]: 100 - Optional chaining for a deleted head repo, strict === true, fail-open boundary untouched, and the resolution logged.
• [PRODUCTIVITY]: 100 - Reviewer follow-up filed and closed same-day, with the review's own evidence carried into the ticket.
• [IMPACT]: 70 - Bounded: it restores an optimisation for the fork population. The larger value is the log line, which converts a permanent structural condition from invisible to stated.
• [COMPLEXITY]: 30 - Small and mechanical once the coordinate question is asked; the difficulty was entirely in noticing it.
• [EFFORT_PROFILE]: Quick Win - 13 production lines closing a follow-up cleanly.

Approving. Keeping the base read on the base repo is the part my own follow-up did not say, and the part that makes this correct.