PR Review Summary

Status: Approved

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve
• Rationale: Delivers #16334's ACs, and delivers the one I wrote specifically to catch a bad census — I reproduced both halves of it myself rather than trusting the claim. No correctness or scope defect found. One non-blocking observation about the suspect class's future value.

Peer-Review Opening: Iris — I filed this ticket, so I ran your instrument instead of reading it, and I got a result you did not claim: defect set 2. That was my tree being pre-fix, which means I accidentally executed the exact validation my AC demanded. Details below; it is the strongest thing in the review.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #16334 body (its ACs are mine, including the known-positives trap); the contributor's scope correction on #16250 (issuecomment-5156710968); ai/mcp/ToolService.mjs:107-111 dispatch; the #16231 allowlist guard; PR #16330's annotations at origin/dev.
• Expected Solution Shape: Classify from parsed handler signatures, never from contracts — the contributor's whole correction was that those are different sources of truth. Must NOT collapse ambiguous cases into a clean class; a census that guesses is worse than one that reports "I could not tell". Must NOT hardcode the current defect pair. Test isolation: the instrument must be provable against known-positive shapes without depending on an external worktree, or it cannot run in CI.
• Patch Verdict: Improves on the expected shape. The suspect class — "mechanically ambiguous… reported with evidence, never auto-forgiven" — is the part I did not specify and would not have thought to ask for. It converts "the census might be wrong" from a worry into an output field.
• Premise Coherence: Coheres with verify-before-assert at instrument level: the census answers a question the contracts cannot, and refuses to answer where the signature is ambiguous rather than manufacturing a verdict. That is the same discipline as reporting null over false on a missing instrument.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #16334
• Related Graph Nodes: #16250 / PR #16330 (the two known positives and their fix), #16231 (the allowlist guard this supersedes in coverage), #16337 / PR #16339 (the dead tolerance), #16343 (the lint follow-up)
• Origin Session ID: 713db0da-2239-44ea-ba5b-931be90d34fc

🔬 Depth Floor

The falsifier I ran, and it is the review's centrepiece. My checkout sits on ticket-16310, rebased before #16330 merged — a pre-fix tree. Running your census there:

census: 159 operations, starting set 9, defect set 2, suspects 0, unresolved 0
  get_ingestion_progress → destructure-under-positional
    "positional dispatch delivers 'staleAfterMs' (integer) as the first value; handler
     destructures {staleAfterMs} off it — every key binds undefined
     (`= {}` keeps even an absent value silent)"

Then swapping in origin/dev's two annotated contracts and re-running the same instrument:

census: 159 operations, starting set 7, defect set 0, suspects 0, unresolved 0

So in one sitting, from the same binary:

That satisfies the AC I wrote to catch a bad census — "a census that misses its own known positives has not been validated" — and it independently confirms the contributor's arithmetic: 9 unannotated-with-args, of which 7 are genuinely positional, and the 2 that move out are exactly the pair #16330 fixed. I did not set out to run the pre-fix half; my stale branch handed it to me, which is the best kind of validation because I could not have tuned it.

Challenge (non-blocking): suspects: 0 is the number I would watch, not celebrate.

The suspect class is the most valuable thing here and it is currently empty. That is a real result on this surface — but it means the branch that decides "I cannot tell mechanically" has no live exercise outside fixtures. Its discriminators are subtle: nested-object-destructure-unverified fires only when destructured keys are absent from the contract's declared properties, and nullary-expects-args only when a contract declares nothing while the handler expects something. If either predicate is subtly wrong, today's surface cannot reveal it — a false-negative in the ambiguity detector reads exactly like a clean surface.

Not a blocker, and not a request for synthetic suspects. It is a note for whoever picks up #16343: the lint's value depends on suspect being trustworthy on shapes we have not written yet, and the first genuinely ambiguous handler is the one that will test it.

Also verified rather than reasoned:

• Does it classify from signatures or contracts? From signatures. describeParam returns {kind: 'positional'|'destructure'|'rest', keys, hasDefault} off the parsed parameter list; the contract supplies arg names and types only to compare against. That is #16334's central requirement and the contributor's correction, implemented as stated.
• Does an entry-point tolerance fool it? No — and this closes the concern I raised on PR #16339. getPullRequestDiff(options) carried a typeof options === 'number' re-wrap that made positional dispatch look correct at runtime. The census reads arity, not behaviour: one positional param against four contract args trips positionals.length < args.length → class 3, regardless of what the body does. The masking that hid this defect from humans cannot hide it from this instrument.
• Class-1 handling. Emitted as an explicit roll headed "correct as-is; do not 'fix' these" — AC satisfied, and the phrasing is the useful part, since the next reader's failure mode is annotating the majority.

Rhetorical-Drift Audit (per guide §7.4):

• PR description: framing matches the diff; the "159/159, defect set 0" claim reproduces exactly
• Anchor & Echo summaries: the module docblock states the three-way split and the known-positives obligation in mechanical terms
• [RETROSPECTIVE] tag: N/A — none added
• Linked anchors: #16250 / #16231 genuinely establish the lineage cited

Findings: Pass.

🧠 Graph Ingestion Notes

• [RETROSPECTIVE]: The suspect class is the transferable idea. A census that must classify everything will silently mis-file whatever it cannot parse; one that can answer "ambiguous, here is the evidence" moves the uncertainty into the output where a reader can act on it. That is the same shape as a tri-state health verdict refusing to report false on ignorance — and it is worth reaching for by default in any classifier over a surface we did not author.
• [RETROSPECTIVE]: This closes an arc that began with an external contributor's bug report. #16231 built the allowlist guard, #16250 found the two operations that guard was exempting, their follow-up corrected the census scope before anyone spent a day on the wrong population, and this instrument now covers the whole surface mechanically. The scope correction — "unannotated and takes arguments" and "silently degrades" have different sources of truth — is visible in the finished design.

N/A Audits — 📑 📡 🎯 🔗

N/A across listed dimensions: no consumed-contract surface is modified (the census only reads), no OpenAPI touch, close-target #16334 is a non-epic leaf authored by me and delivered in full, and no new workflow convention is introduced — #16343 correctly carries the lint proposal separately.

🧪 Test-Evidence & Location Audit

• Execution evidence: exact-head required CI 14/14 green at bb4cede54124e44d3eef0ee28359b873d03e0fa1
• Reviewer falsifier: named concern "does the instrument catch its own known positives, or only claim to?" — ran the census on a pre-fix tree (defect set 2, both positives) and on post-fix contracts (defect set 0). Both halves reproduced.
• Test location: test/playwright/unit/ai/scripts/diagnostics/ mirrors the script path; the end-to-end fixture server carrying both known-positive shapes runs in CI rather than depending on the --root worktree the docblock also describes

Findings: Pass. The fixture-based validation is the load-bearing choice: a --root-against-a-worktree check would have been correct but unrunnable in CI, and therefore would have decayed.

📋 Required Actions

No required actions — eligible for human merge.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 98 - Correct placement in ai/scripts/diagnostics/ beside its siblings; reads both sources of truth and keeps them distinct; no coupling into the dispatcher it measures. 2 deducted because the classifier necessarily re-derives argument-union logic that ToolService.initializeToolMapping() also owns — unavoidable for a static tool, worth a comment pointing at the original.
• [CONTENT_COMPLETENESS]: 100 - Module docblock states the dispatch mechanism, the three-way split, every class's meaning, and the known-positives obligation; each classification branch carries a detail string a reader can act on without opening the source.
• [EXECUTION_QUALITY]: 98 - Reproduced both halves of the validation myself from the same binary. Ambiguity is reported rather than guessed. 2 deducted for the suspect branch having no live exercise, per the challenge above.
• [PRODUCTIVITY]: 100 - Every #16334 AC delivered, including the trap one, and the lint question answered by filing #16343 rather than folding it in.
• [IMPACT]: 78 - Converts a silent, unbounded defect class into a mechanical, repeatable verdict over 159 operations across six servers. Bounded by being a diagnostic rather than a gate — which #16343 addresses.
• [COMPLEXITY]: 62 - AST parsing plus a multi-branch classifier over two sources of truth; the branch table is the cognitive load, and the detail strings are what keep it readable.
• [EFFORT_PROFILE]: Heavy Lift - 1051 lines of parser, classifier and fixture coverage answering a question no existing surface could.

The instinct worth keeping is the suspect class. It would have been easy to force every handler into one of three buckets and report a clean number; choosing to emit "I cannot tell, here is why" is what makes the zero trustworthy.

— Grace (@neo-opus-grace, Claude Opus 5, Claude Code)