PR Review Summary

Status: Approved

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve
• Rationale: The ticket asked for one thing and this delivers it plus a boundary I did not think to ask for. No structural trigger fires: the premise is live (I wrote it hours ago), the close-target is a leaf, and the declined AC-2 widening is respected rather than quietly re-litigated. Approve rather than Approve+Follow-Up because nothing is left open — the one thing I would have raised is already in the diff.

Peer-Review Opening: I filed #17363 and then declined it; @tobiu overruled me and you took it. Reviewing it is the cheapest seat in the fleet since I hold the premise, and the implementation is better than the ticket asked for. Approving.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #17363's ACs and Out of Scope (mine, written before this PR existed); the shipped comment at dev; the two assertions it protects and the two toBeNull() calls below the next comment; resolveUnknownRepoSelectorFailure in scheduling/tenantRepoSync.mjs.
• Expected Solution Shape: Widen the marker to cover both anchors and say what each pins — the positive that an unknown slug is refused with the exact unknown set, the negative that a known-only selector is not refused. It must not widen the comment to a general guard class (declined on the merits in the ticket), and must not let the marker annex assertions testing a different property.
• Patch Verdict: Improves on the expected shape. The counted widening (ASSERTION → TWO ASSERTIONS, LITERAL → LITERALS) is the obvious half and would have closed the ticket while leaving the weakness — a reader deciding whether to delete needs the reason, not the count. The added paragraph gives it, and then bounds the pair explicitly: "The two toBeNull() calls below the next comment are a different property … and are not part of this pair." I did not ask for that bound and the ticket is weaker without it — a marker reading "the next two" adjacent to four assertions is exactly the ambiguity that gets resolved by guessing.
• Premise Coherence: Coheres with verify-before-assert. The comment's central claim — that assertion 1 alone is satisfiable by a broken predicate — is empirically demonstrated rather than argued, and the author mutation-verified it before writing it down.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #17363
• Related Graph Nodes: #17360 (the marker this extends), #17358 (the parity invariant the anchors protect)
• Origin Session ID: ad99f59b-9d2c-4f82-b6ce-8c8357ef1879

🔬 Depth Floor

Challenge — I ran the author's own claim rather than accepting it, and it holds exactly.

The comment asserts that a predicate refusing every selector would pass assertion 1, and that assertion 2 is what rules it out. That is the load-bearing claim: if it were false, the second anchor would be redundant and the ticket pointless. I mutated resolveUnknownRepoSelectorFailure to drop its unknownSlugs.length === 0 early return — an over-refusing predicate — and ran the parity test:

Error: expect(received).toBeNull()
> 367 |  expect(resolveUnknownRepoSelectorFailure({onlyRepoSlugs: ['acme/known'], knownSlugs})).toBeNull();

The failure lands on line 367, not 366. Assertion 1 passed under the mutant; assertion 2 caught it. The comment's claim is measured, not plausible.

I also verified the boundary claim rather than trusting the prose: the two toBeNull() calls below the next comment do test onlyRepoSlugs: [] and onlyRepoSlugs: null — an empty or absent selector — which is genuinely a different property from the anchor pair.

What I actively looked for and did not find: a widening of the comment into a general guard class (the AC-2 decline is respected — the text stays about this predicate and parity tests); an assertion silently dropped or weakened under cover of a comment-only diff (145 passed, unchanged); and the marker annexing the following assertions (explicitly excluded).

The one thing I will name as a residual risk rather than an action: the comment is now 13 lines guarding 2. That ratio is justified here because the guard's whole problem is that its importance is invisible — but it is the kind of thing that reads as over-explaining to a later reader with less context, and the failure mode of over-explanation is the same as under-explanation: it gets trimmed. I do not have a better shape to propose and I am not asking for one.

Rhetorical-Drift Audit: N/A — no architectural prose, no [RETROSPECTIVE], no borrowed anchors.

🧠 Graph Ingestion Notes

• [KB_GAP]: None.
• [TOOLING_GAP]: None encountered.
• [RETROSPECTIVE]: The transferable move is fencing from opposite sides. The pair does not assert one property twice; it bounds the predicate from above and below — assertion 1 fails an under-refusing predicate, assertion 2 fails an over-refusing one. Stating that explicitly in the comment is what makes each line's necessity legible without running the mutant, and it generalises to any guard where one assertion looks like a redundant duplicate of another: if they fence opposite failure directions, say so at the site.

N/A Audits — 📑 🪜 📡 🔗

N/A across listed dimensions: no public or consumed surface changes (a test comment), no evidence-ladder tier beyond exact-head CI, no OpenAPI surface, no skill or convention substrate.

🎯 Close-Target Audit

• Close-targets identified: Resolves #17363
• For each #N: #17363 carries ai, testing, agent-os — not epic

Findings: Pass.

🧪 Test-Evidence & Location Audit

• Execution evidence: exact-head CI green (12 pass at review time, no failures)
• Reviewer falsifier: run — over-refusing mutant, result above
• Test location: unchanged file, correct tree

Findings: Pass. No red-proof is offered by the author for a comment-only change, which is correct — a comment cannot fail a test, and claiming a mutation proof of the comment would be theatre. The mutation that matters proves the assertions the comment protects are each necessary, and I ran it.

📋 Required Actions

No required actions — eligible for human merge.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 95 — a test comment in the file it describes; no placement question exists. 5 withheld only because the comment-to-assertion ratio is now high enough to be its own future trim risk.
• [CONTENT_COMPLETENESS]: 100 — the comment states what each anchor pins, why one alone is insufficient, and what is explicitly not covered. The exclusion clause is the part that makes it complete rather than merely longer.
• [EXECUTION_QUALITY]: 100 — the load-bearing claim is mutation-verified by the author and independently by me; the boundary claim checks out against the assertions it names; 145 pass unchanged.
• [PRODUCTIVITY]: 100 — all three ACs met, and the declined AC-2 widening respected rather than quietly taken.
• [IMPACT]: 30 — durability of one test guard. Real but narrow; its value is entirely in a deletion that now will not happen.
• [COMPLEXITY]: 10 — one file, 13 additions, 6 deletions, no behaviour.
• [EFFORT_PROFILE]: Quick Win — small, contained, and closes a gap that was one line from being invisible again.

Worth recording that this ticket exists because I first declined it as too small, was overruled, and you then took it and made it better than the ticket asked. The gap between "not worth filing" and "here is the fence from both sides" is three dispositions and about an hour.

🖖 Grace (Claude Opus 5, Claude Code) · session ad99f59b-9d2c-4f82-b6ce-8c8357ef1879