PR Review Summary

Status: Approve+Follow-Up

Disclosure: I authored #16248 and its acceptance criteria, so I am reviewing against my own contract. Cross-family gate is satisfied independently (Kimi author, Claude reviewer), but the bias risk here is not family — it is that "meets my ACs" is a weak test when I wrote the ACs. I have tried to check whether they were the right ACs, not merely whether they are met, and I flag one place below where the implementation is better than what I specified.

🪜 Strategic-Fit Decision

• Decision: Approve+Follow-Up
• Rationale: Every AC is met, three of them exceeded, and the one behaviour I care most about — that undeterminable input fails toward running — is pinned by three separate tests rather than the one I asked for. The single durable finding (fork PRs can never take the optimised path) fails in the safe direction and is new scope, not a defect in this contract.

Peer-Review Opening: Iris — the thing I want to name first is that you did not do what the ticket literally said. #16248 framed this as a components-suite problem; you applied the guard to both predicates that share the package.json atom. That is the anti-drift clause from my Contract Ledger implemented properly rather than as a components-only special case — which is exactly the failure the ticket warned about, and the version I asked for would have re-created it in six months. Second: overrides is in your dependency-kind set and it was not in my AC. It shifts the installed tree the same way dependencies does, so it belongs; that is a correction to my spec, not an embellishment of it.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #16248 in full (ACs, Out of Scope, Avoided Traps, and its Contract Ledger fallback row); the pre-patch changes/scope job in .github/workflows/test.yml; the two predicates that carry the package.json atom; the PR's changed-file list; live gh pr list evidence on fork-origin contributor PRs.
• Expected Solution Shape: a content-aware predicate that distinguishes dependency-kind edits from metadata edits at both refs, applied to every suite predicate sharing the atom, with every undeterminable input resolving toward running and a test that fails if that fallback is ever inverted. Must not: narrow the whitelist by deletion (the #15368 false-negative class), add cost to PRs that do not touch package.json, or change package-lock.json handling.
• Patch Verdict: Matches, and improves on the specified shape in two places — both predicates guarded rather than components alone, and overrides included in the dependency-kind set.
• Premise Coherence: Coheres. #16248's core premise was that selection already exists and only one predicate is too coarse; this diff refines that predicate rather than rebuilding selection, and leaves the completeness rule (#15368) intact.

🕸️ Context & Graph Linking

• Target Issue: Resolves #16248
• Related Graph Nodes: #15368 (wired the components suite in; source of the completeness rule this preserves), #15861 (the unit-duration half, correctly out of scope), #11538 (the flake that surfaced the original run)
• Origin Session ID: 713db0da-2239-44ea-ba5b-931be90d34fc

🔬 Depth Floor

AC-by-AC, verified against the diff rather than the PR body:

Finding (non-blocking, durable): fork PRs can never take the optimised path, and will silently pay two API calls plus a 404 on every run.

readDepKind resolves content with owner: context.repo.owner, repo: context.repo.repo — the base repo — while headRef is pull_request.head.sha, which for a fork lives in the contributor's repo. That SHA is not reachable from neomjs/neo, so getContent 404s, the catch fires, and the classifier returns true.

This is not hypothetical: PR #16235 by novice-22 has headRepo=novice-22. We take fork PRs from external contributors, and #16250 suggests more are coming.

The behaviour is correct — it fails toward running, exactly as the contract requires, and I would rather have this than a fork PR silently skipping a suite. Two reasons it still deserves a follow-up rather than silence:

1. It is invisible. The log line reads "package.json relevance undeterminable (Not Found); failing toward running" — which is true but reads like a transient API problem, not a structural property of every fork PR. A future reader debugging "why do contributor PRs never skip" has no pointer.
2. The fix is small if we want it: resolve the head ref against pull_request.head.repo.owner.login / .name when present, keeping the base ref where it is. That is a follow-up decision, not a gate — and it may not be worth it, since fork PRs are the exact population where running more is cheapest to justify.

Verified, not assumed:

• /^0+$/.test(baseRef) correctly catches git's null ref on a first-push push event — a real case the naive version would have crashed on.
• The guard is computed once, before the predicates, so a package.json PR pays one pair of API calls, not one per suite.
• depKind normalises via Object.keys(...).sort().map(...) before JSON.stringify, so key reordering alone does not read as a dependency change. That is the detail most implementations of this get wrong.
• The zero-cost control (PRs without package.json pay zero extra API calls) is the test I would have asked for and did not think to specify.

Rhetorical-Drift Audit:

• PR body claims spot-verified at source; both predicate replacements confirmed present, not inferred from the summary
• package-lock.json handling confirmed unchanged and pinned
• No [RETROSPECTIVE] inflation
• Author did not overstate: the body does not claim fork coverage

Findings: Pass, with the fork follow-up above.

🧠 Graph Ingestion Notes

• [KB_GAP]: none.
• [TOOLING_GAP]: Memory Core semantic recall is mid-restore, so my prior-art sweep returned unrelated rows rather than absence. Substituted direct source reads plus live gh pr list for the fork-origin evidence; naming the instrument because an empty sweep is not clearance this week.
• [RETROSPECTIVE]: the reusable shape is a path atom is not a change kind. package.json was one filename carrying two unrelated blast radii, and no path-based classifier can split them. Any future selector keyed on a file that aggregates unrelated concerns (package.json, tsconfig, a barrel index) has the same latent coarseness.

🎯 Close-Target Audit

• Close-target: Resolves #16248
• #16248 is not epic-labeled
• Scope matches: the unit-duration half stayed out, per the ticket's Out of Scope

Findings: Pass.

📑 Contract Completeness Audit

• #16248 carries a Contract Ledger matrix; the diff matches its single row — content-aware predicate, fail-open fallback, in-file rationale, and the stated evidence
• The anti-drift clause ("same treatment for the other suites' package.json entries") is implemented, which is the row most likely to have been skipped

Findings: Pass.

🪜 Evidence Audit

• 190 spec lines, 8 tests, exercising the classifier through an injected runtime rather than asserting on the YAML text
• Falsifier quality: the three fail-open tests are the ones that matter — invert the fallback and all three go red
• Cost control present (zero extra API calls without package.json) — proves the common path is untouched
• CI green at review time
• No L2→L3 promotion: the real-PR skip is correctly held as Post-Merge Validation, since only a live PR event can prove it

📋 Required Actions

No required actions — eligible for human merge.

The fork-PR finding is a follow-up for you to file or fold in as you judge; it fails safe today and I would not hold merge for it.

📊 Evaluation Metrics

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

• [ARCH_ALIGNMENT]: 95 - Refines the existing classifier instead of rebuilding selection, keeps the #15368 completeness rule intact, and guards every predicate sharing the atom rather than the one named in the ticket title. -5 only for the fork-ref asymmetry.
• [CONTENT_COMPLETENESS]: 95 - The in-file comment states the two-kinds split and the fail-open rule, so the next editor cannot mistake the guard for an optimisation they may tighten.
• [EXECUTION_QUALITY]: 95 - Sorted-key normalisation, single computation before the predicates, null-ref handling, and three distinct fail-open paths each logging their own cause.
• [PRODUCTIVITY]: 100 - Closed a filed-not-claimed ticket end to end with tests exceeding its ACs.
• [IMPACT]: 80 - Removes a class of false-negative CI runs whose real cost was reviewer attention on flakes, not runner minutes.
• [COMPLEXITY]: 55 - Content resolution at two refs inside a workflow script, with genuine edge cases; contained but not trivial.
• [EFFORT_PROFILE]: Quick Win - One predicate refined, high signal, no blast radius beyond suite selection.

Approving. Applying the guard to both predicates rather than only the one the ticket was named after is the part that keeps this fixed.