PR Review Summary

Status: Approved

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve
• Rationale: Delivers #16343, answers the concern I raised on PR #16345 with a distinction I did not propose, and adds a defect class the originating ticket's taxonomy did not have. I ran the gate red and green myself. No required actions; one non-blocking note about what the design makes load-bearing.

Peer-Review Opening: Iris — I flagged on #16345 that suspects: 0 was the number to watch and that the lint's value would depend on suspect being trustworthy. You did not just handle it; you split it into two questions I had collapsed into one.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #16343; my own #16345 review note on the untested suspect branch; #16334's three-class taxonomy (mine, and the ACs are mine); ai/mcp/ToolService.mjs:107-111; the census at origin/dev post-#16345 merge.
• Expected Solution Shape: Turn the census into a CI gate that fails on the defect classes. Must NOT duplicate the classification predicate — a gate with its own copy of "what counts as a defect" drifts from the census within a release. Must NOT fail on suspect without a stated rationale, since a gate that fires on "I cannot tell" gets muted. Test isolation: the gate must be provable in both directions in CI — green on a clean tree, red on a planted defect — without depending on an external worktree.
• Patch Verdict: Improves on the expected shape, in two ways I did not ask for. DEFECT_KLASSES is a single exported Set consumed by both the gate and --fail-on-defects, with the reason stated inline — "never two copies that can drift." And the suspect question is answered by splitting it: suspect does not fail; unresolved does.
• Premise Coherence: Coheres with verify-before-assert at gate level. The distinction it draws — can't classify versus can't see — is the same discipline as reporting null over false on a missing instrument: ignorance about a row is human-judged, ignorance about whether a row exists at all is a failure, because "a census that cannot see is not green."

🕸️ Context & Graph Linking

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

🔬 Depth Floor

The falsifier I ran, both directions, from the same suite. My checkout sits on ticket-16310 — pre-#16330, so the defects are present. Running your gate there:

✗ no operation silently degrades under its dispatch mode (defect set empty)
github-workflow/get_pull_request_diff [class 3 · truncation]
  handler declares 1 param(s) [options] for 4 contract arg(s) [pr_number,file,sha,files_only]
  — [file,sha,files_only] silently dropped (PullRequestService.getPullRequestDiff)
knowledge-base/get_ingestion_progress [class 2 · 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)

Swapping in origin/dev's two annotated contracts and re-running: 4/4 pass. So the gate is red on the defect and green on the fix, proven from one suite in one sitting. The failure message is the part that will age well — it names the handler, the arity, the dropped arguments and the binding, so a contributor who trips it does not need to open the census to know what to do.

Class 2M is a scope addition beyond #16334, and it is a real one. My ticket's taxonomy had three classes, all "annotation missing." 2M is the mirror: annotation present, handler positional, so object dispatch binds the whole args object to the first param and every contract-named param after it receives undefined. That is the inverse failure and my taxonomy did not have a slot for it. Worth naming because it is the direction a fix introduces — someone annotating a positional handler to "be safe" creates exactly this.

Challenge (non-blocking, and it is the honest evolution of my #16345 note): suspect is now load-bearing as an escape hatch, so its precision matters more than it did yesterday.

Your rationale is right and I would not change the decision — "a gate that cries on 'I cannot tell' trains contributors to dismiss it." But the consequence is that a genuine defect misclassified as suspect passes the gate silently. Before this PR, suspect was a reporting nicety; now it is the one classification that buys exemption from CI. That moves the risk rather than removing it, which is the correct trade and worth stating out loud somewhere the next reader will find it.

Concretely: nested-object-destructure-unverified fires when destructured keys are absent from the contract's declared properties. A contract with a loosely-declared object property could route a real class-2 into suspect. I am not asking for a change — I am asking that when a suspect first appears in the wild, someone reads it rather than filing it, because that first one is the calibration.

Also verified rather than reasoned:

• Is the predicate genuinely shared? Yes — DEFECT_KLASSES = new Set([2, '2M', 3]) exported from the census, isDefectRow consuming it, and both the spec and --fail-on-defects calling that. One definition, two consumers.
• Does the suspect branch now have a live exercise? Yes, and this closes the exact gap I named on #16345. The fixture plants get_x: args => args.staleAfterMs — a generic bag name under positional dispatch — and asserts it classifies suspect and stays out of the defect set. That is the branch exercised end to end rather than assumed.
• Does unresolved fail? Yes, its own test: "every binding resolves — a census that cannot see is not green." This is the half I would have got wrong; I would probably have lumped it with suspect.

Rhetorical-Drift Audit (per guide §7.4):

• PR description: framing matches the diff
• Anchor & Echo summaries: the module docblock states which classes fail and why suspects do not — the rationale is in the source, not only the PR
• [RETROSPECTIVE] tag: N/A — none added
• Linked anchors: #16343 / #16334 genuinely establish the lineage

Findings: Pass.

🧠 Graph Ingestion Notes

• [RETROSPECTIVE]: The transferable move is splitting one "unknown" into two. Can't classify (suspect) and can't see (unresolved) look like the same ignorance and have opposite gate semantics — one is a human's call, the other means the instrument is blind and must fail. I had collapsed them in #16334's ACs and would have shipped a gate that either muted itself or blocked on ambiguity. Worth reaching for whenever a classifier grows an escape hatch.
• [RETROSPECTIVE]: Class 2M is the failure a remediation introduces — annotate a positional handler to "be safe" and every contract-named param after the first binds undefined. A census built only from the original defect's shape would not have had a slot for it. When gating a bug class, ask what the obvious fix breaks.

N/A Audits — 📑 📡 🪜 🔗

N/A across listed dimensions: no consumed-contract surface changes (the gate only reads), no OpenAPI touch, close-target ACs fully covered by the suite at exact head, and no new workflow convention beyond the CI gate #16343 asked for.

🎯 Close-Target Audit

• Close-targets identified: #16343
• #16343 confirmed not epic-labeled, OPEN, authored and assigned to the PR author

Findings: Pass. Single Resolves #16343; no Closes / Fixes.

🧪 Test-Evidence & Location Audit

• Execution evidence: exact-head required CI 14/14 green at b5256d1df16ddcc95ea56286e4c014a9bbadbad7
• Reviewer falsifier: named concern "does the gate actually trip, and does suspect really escape it?" — ran the suite on a pre-fix tree (1 failed, both defects named with class and detail) and again against origin/dev's contracts (4/4 pass). Suspect fixture asserts classification and absence from the defect set.
• Test location: test/playwright/unit/ai/mcp/validation/ alongside OpenApiValidatorCompliance.spec.mjs, which is where contract-vs-implementation checks already live

Findings: Pass. The negative-receipt test is the one that keeps this honest long-term: it asserts the gate trips on both known-positive shapes, so a future refactor that quietly narrows DEFECT_KLASSES fails there rather than going green everywhere.

📋 Required Actions

No required actions — eligible for human merge.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 100 - Gate lives beside the existing contract-compliance suite; the predicate is exported once and consumed twice; the census is read, never re-implemented. Checked specifically for a duplicated defect-class list and there is none.
• [CONTENT_COMPLETENESS]: 100 - The docblock states which classes fail, which do not, and why — including the "trains contributors to dismiss it" reasoning, which is the part a future maintainer needs to not "fix" suspects into failures.
• [EXECUTION_QUALITY]: 98 - Red and green both reproduced by me; suspect and unresolved correctly split; failure output actionable without opening the census. 2 deducted only for the residual named in the challenge — a defect misclassified as suspect now passes silently, which is the right trade and still a moved risk.
• [PRODUCTIVITY]: 100 - #16343 delivered, plus a fourth defect class the originating ticket did not specify.
• [IMPACT]: 82 - Converts a bug class that took an external contributor two reports to surface into a PR-time failure across 159 operations and six servers. Bounded only because it gates rather than repairs.
• [COMPLEXITY]: 38 - One shared predicate, four focused tests, one synthetic fixture server; the subtlety is entirely in the suspect/unresolved split.
• [EFFORT_PROFILE]: Quick Win - Small diff on top of the census, closing the loop from report → class → census → gate.

The arc is worth stating: an external contributor reported one broken call, corrected our census scope before we spent a day on the wrong population, and the class now fails at PR time. Four tickets and four PRs from one bug report, and the scope correction is visible in the finished taxonomy.

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