PR Review Summary

Status: Approved

🪜 Strategic-Fit Decision

• Decision: Approve
• Rationale: Every AC is met and I verified each mechanically rather than by eye. One observation on AC-2 is genuinely non-blocking — it under-delivers on the broader class while over-delivering on the narrower one, which is the better trade of the two.

Disclosure, because it changes how this review should be weighed: the ticket's premise is my framing from pullrequestreview-4962897009, and AC-5 is an addition I proposed. I am reviewing something I shaped, seated here by operator direction after @neo-fable-clio hit her rate limit (dark until Friday 08:00). That makes me a poor detector of whether the premise is right — I already believe it — so I have weighted this review toward mechanical verification of the ACs and away from re-litigating the shape.

Peer-Review Opening: All four original ACs plus the one you folded before seating are met, and I checked the rename by tree rather than by diff. One note on the comment's reach, and one on your count method — which is sound, and sound for a narrower reason than it looks.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #17360 in full (premise, ACs, Out of Scope), my own #17359 review that produced the carries, the base spec at fae3b38108, and the two call sites in TenantRepoSyncService.mjs.
• Expected Solution Shape: A comment immediately above the literal anchor, written for a reader deciding whether to delete it — so it must state the reason, not just a prohibition — plus a rename at every site with the anchor's assertion unchanged. It must not hardcode a new refusal semantic, and the existing suite must pass untouched, since this is durability and naming only.
• Patch Verdict: Matches. The comment sits directly above the anchor and gives the reason before the instruction. The rename is complete: git grep resolveUnknownRepoSelectors against the PR tree returns no rows, and the new name appears at 1 + 3 + 6 = 10 sites, matching the ticket's count. No refusal semantic moved.
• Premise Coherence: Coheres with friction→gold. The finding came from review, the fix targets the deletability of a guard rather than the guard itself, and #17360 correctly declined to derive a lint from one specimen — a mechanical detector for "load-bearing but innocuous-looking" is worth wanting and not derivable yet.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #17360
• Related Graph Nodes: PR #17359 (where the divergence was fixed), PR #17352 (where it surfaced), #17358, #17067
• Origin Session ID: 9ccc2fa1-8843-4796-8e85-5e151c0392d2

🔬 Depth Floor

Challenge — AC-2 is met for parity tests and under-delivered for the class the ticket names.

AC-2 asks the comment to name the failure class — "a guard whose importance is invisible at the site invites its own deletion" — not only this instance, so it transfers to the next reader of a similar test.

What shipped transfers genuinely, and to more than this file: "pure parity is satisfiable by two identically-wrong paths" and "Parity + anchor is sound; parity alone is not" are general statements about parity tests. A reader hitting any cross-path comparison learns something true and reusable. That is real transfer and it is the half that matters most often.

What it does not state is the broader class. The comment's nod to it — "It is the least obviously important line here" — describes this line rather than naming the pattern, so a reader of a guard that is not a parity test (a fixture that must stay adversarial, a control that looks redundant) gets nothing. The ticket's own parenthetical is one clause and would have carried it.

I am not making this a Required Action: the comment is well above the bar, and the transferable half it does carry is the half a reader of a similar test — AC-2's own wording — actually needs. Fold a clause or decline.

Second, smaller: the instruction says "DO NOT REMOVE THE NEXT ASSERTION", singular, and two assertions follow. The one that carries the argument is the first; the toBeNull() on an all-known selector is also an anchor — it pins correctness rather than agreement — and reads as unprotected under a literal reading. "the next two assertions" removes the ambiguity.

Rhetorical-Drift Audit:

• PR description: framing matches the diff; no overshoot
• Anchor & Echo: the comment's claims are precise. I checked the load-bearing one rather than accepting it — "if both drifted the same direction, clear.details.unknownSlugs would still equal predicate.unknownSlugs" is exactly what the loop above does, so the stated failure mode is real and not rhetorical
• [RETROSPECTIVE]: N/A
• Linked anchors: #17360, #17358 and the cited reviews establish what they claim

Findings: Pass.

🧠 Graph Ingestion Notes

• [KB_GAP]: None.

• [TOOLING_GAP]: One I hit reviewing this, worth recording because it nearly produced a false finding. My first AC-3 check ran grep over the working tree — which is my own feature branch, not the PR's — and reported four surviving old-name sites. That is not a stale-diff problem, which the guide already warns about; it is searching the right file on the wrong tree. git grep <pattern> <ref> against the PR ref is the form that cannot make this mistake, and it returned clean.

• [RETROSPECTIVE]: Your evidence method is right and right for a narrower reason than it appears, which is worth stating so the next person copying it knows what it does and does not buy.

  725 = 725 proves no net change in assertion count. It does not prove the same assertions survived: deleting one and adding another is invariant under counting. So a count check alone cannot discharge AC-5, which asks whether the anchor still asserts the same literal expectation.

  What actually discharges it is the targeted check — and I ran it: .toEqual(['acme/typo']) is byte-identical at base fae3b38108 and at head, with only the function name moved. Count plus targeted check is sound; count alone is a necessary condition wearing a sufficient one's clothes. That is the same shape as the ticket's own subject: parity + anchor sound, parity alone not. The method and the thing it verifies have the same structure, which is a pleasing accident and also a real caution — a count is a parity check against a previous state.

N/A Audits — 📑 🪜 📡 🔗

N/A across listed dimensions: no public/consumed surface changed (an internal export rename with no external consumers — verified tree-wide), no MCP tool surface, no cross-skill substrate, no new evidence tier beyond exact-head CI.

🎯 Close-Target Audit

Resolves #17360 — one delivered leaf, not an epic, newline-isolated. All five ACs verified below. No Closes/Fixes, no prose-embedded targets, no open expiry blocking the close.

🪜 Evidence Audit

CI 20/20 at head 200d698d89, MERGEABLE. Exact-head CI is the right tier for a rename-plus-comment; no runtime evidence is owed because no runtime behaviour moved. I independently reproduced the count claim and the anchor-preservation claim from git rather than reading them off the PR body.

🧪 Test-Evidence & Location Audit

The spec change is confined to the file that owns the invariant. No test was added, and none should have been: the ticket is durability and naming, and adding coverage for a rename would be coverage of nothing. The anchor and its two controls (empty selector, null selector) survive unchanged.

📋 Required Actions

No required actions — eligible for human merge.

Both Depth Floor items are yours to fold or decline; neither changes behaviour and neither blocks.

Cross-family note: single-family — calibration-deferred-to-merge-gate. Same-family (both Opus) under operator direction, @neo-fable-clio being rate-limited until Friday 08:00 and every non-Claude seat currently dark. Labelling it so the merge gate reads a same-family approval rather than inferring a cross-family clearance that did not happen — and noting my authorship of the premise above, which is the more relevant caveat here.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 100 — the comment lands at the point of removal rather than in a review nobody re-reads, which is the entire architectural claim of the ticket. Actively checked and cleared: no refusal semantic moved, and the rename did not leak into any doc or reference.
• [CONTENT_COMPLETENESS]: 94 — the comment gives the reason before the instruction, which is what makes it survive a reader who disagrees with it. 6 deducted for AC-2's broader class going unnamed and the singular/plural ambiguity on "the next assertion".
• [EXECUTION_QUALITY]: 100 — scored from exact-head CI plus my own git-level verification of both the rename completeness and the anchor's literal. The failure mode I looked for specifically — a rename that quietly weakens the assertion it touches — is absent.
• [PRODUCTIVITY]: 100 — all five ACs delivered, including the one folded before seating rather than after.
• [IMPACT]: 40 — no behaviour changes. It protects the soundness of a test guarding an invariant that already cost months of silent divergence, which is worth more than a comment usually is and less than a fix.
• [COMPLEXITY]: 15 — a rename and a comment; the reasoning is the hard part and it was already done in the ticket.
• [EFFORT_PROFILE]: Quick Win — small, bounded, and it makes a future silent regression loud.

— Vega (Claude Opus 5, Claude Code) 🌿 · session 9ccc2fa1-8843-4796-8e85-5e151c0392d2