PR Review Summary

Status: Approved

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve
• Rationale: Both defects closed, and closed in the shape that strengthens enforcement rather than documenting the fix — net −8 lines. No correctness, safety, or scope defect found. The one consequence I surfaced (a live behavior change on get_pull_request_diff) is the intended fix, not a deferred correctness issue, so it belongs in post-merge validation rather than a return cycle.

Peer-Review Opening: Iris — the part I want to name is what you didn't write. This adds no new test. It deletes the two allowlist entries that were holding the defect open, which converts the annotation from present to enforced. That is the better fix and it is smaller.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #16250 body and the contributor's follow-up (issuecomment-5156710968); my own source verification of both handlers on 08-01; ai/mcp/ToolService.mjs:107-111 dispatch; the #16231 recurrence guard at OpenApiValidatorCompliance.spec.mjs:729 including its allowlist semantics; PullRequestService.getPullRequestDiff and IngestionService.getIngestionProgress signatures at dev.
• Expected Solution Shape: Annotate both operations with x-pass-as-object: true. Must NOT remove the typeof options === 'number' || 'string' legacy tolerance — that is a separate named decision both the contributor and I said should not ride along. Test isolation: the fix must be enforced, not merely asserted once; the pre-existing #16231 guard already covers the whole surface, so the correct move is to stop exempting these two rather than to add a parallel test.
• Patch Verdict: Matches, and improves on, the expected shape. The evidence that settled it: the removed allowlist entries carried their own admission — "Listed to keep this guard green, NOT because the dispatch is right." Deleting them is what makes the annotation load-bearing. The legacy tolerance is untouched, as it should be.
• Premise Coherence: Coheres with verify-before-assert and friction→gold at the right size. #16231 (this contributor's first report) built the repo-wide guard; its allowlist was then used to hold this defect open with an honest comment; #16250 (their second) named the class; this PR closes the loop by removing the suppression. Substrate shrinks while enforcement tightens — the shape a guard is supposed to have.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #16250
• Related Graph Nodes: #16231 (the guard this re-arms), #16334 (the census, scope-corrected by the contributor), #14542 (the neural-link recurrence guard this generalizes from)
• Origin Session ID: 713db0da-2239-44ea-ba5b-931be90d34fc

🔬 Depth Floor

Challenge: This is a live behavior change for every agent seat, and it activates on plane rebuild rather than on merge.

get_pull_request_diff currently ignores file, sha and files_only — arity drops them, and the bare-number tolerance re-wraps pr_number so the call succeeds and returns the entire diff. After this PR it honours all four. That is the fix. But the consequence is that a peer who has internalized "scoping the diff to a file doesn't work, I always get everything" will start receiving less than they expect, mid-session, with no announcement.

Two specifics worth pinning:

1. Any caller that worked around the bug by post-filtering would now double-filter. In-repo this is covered — CI is green and the seven PullRequestService.spec.mjs call sites pass objects and assert scoped behavior. The risk is agent call sites, which are not in the repo.
2. x-pass-as-object is consumed server-side (ToolService.mjs:107-111, if (tool.passAsObject)), not by the client's cached schema. So it takes effect when the container is rebuilt, not when a client reconnects — a different activation boundary from #16320's client-side pin, and worth stating so nobody debugs the wrong layer.

Neither blocks. Both belong in the announcement when the plane next rebuilds.

Also looked at and cleared:

• Whether the annotation is actually enforced, or just present. Named falsifier, run: removing x-pass-as-object from get_pull_request_diff at PR head fails the #16231 guard with github-workflow.get_pull_request_diff (pr_number, file, sha, files_only). The annotation and the allowlist removal are jointly load-bearing. Restored after.
• Whether the legacy tolerance was quietly removed. It is untouched. Correct — and per the answer I gave the contributor, it lands dead on merge, since every in-repo call site passes an object and the sole bare-number producer was the dispatcher this PR fixes. That is its own decision, not this PR's.
• Whether the remaining allowlist is now clean. Counted at head: 9 unannotated argument-taking operations, of which 7 are genuinely positional — 6× get_mcp_tool_handbook (one per server, exempted by the unprefixed key) and github-workflow:get_local_issue_by_id — plus exactly these 2 defects. The contributor's arithmetic reconciles to the operation. After this merges the allowlist holds only true positional handlers and the known defect set is empty.

Rhetorical-Drift Audit (per guide §7.4):

• PR description: framing matches what the diff substantiates
• Anchor & Echo summaries: N/A — no new prose surface; the change removes commentary
• [RETROSPECTIVE] tag: N/A — none added
• Linked anchors: #16231 and #14542 genuinely establish the guard lineage cited

Findings: Pass.

🧠 Graph Ingestion Notes

• [RETROSPECTIVE]: The instructive artifact here is the deleted comment: "Listed to keep this guard green, NOT because the dispatch is right." An allowlist entry that documents a known defect is a decision to keep a test green at the cost of the thing it guards. It was honest — which is why it was findable — but it meant the guard reported clean over two live defects for as long as it stood. Worth remembering that an exemption with a candid reason is still an exemption.
• [RETROSPECTIVE]: The whole arc came from one external contributor across two reports: #16231 built the repo-wide guard, #16250 named the class it could not see, and their follow-up corrected the census scope before anyone spent a lane on it. Their sharpest contribution was a distinction, not a defect — "unannotated and takes arguments" and "silently degrades" have different sources of truth, and only the handler signature answers the second.

N/A Audits — 🪜 🔗

N/A across listed dimensions: close-target ACs are fully covered by the re-armed guard at exact head (no runtime ladder beyond it), and no skill file, workflow convention, or new architectural primitive is introduced.

🎯 Close-Target Audit

• Close-targets identified: #16250
• #16250 confirmed not epic-labeled (bug, contributor-experience), OPEN, authored by @novice-22

Findings: Pass. Single newline-isolated Resolves #16250; no Closes / Fixes. Both of the ticket's operations are addressed, so the leaf closes fully delivered.

📑 Contract Completeness Audit

• The modified surface is the MCP dispatch contract for two operations
• Implemented diff matches the ticket's stated scope

Findings: Pass with a note. #16250 is an external contributor's report and carries no formal Contract Ledger matrix — correctly, since we do not impose our internal ticket template on contributors. The consumed contract is nonetheless pinned mechanically by the #16231 guard, which is stronger than a ledger row: the exemption is gone, so the annotation cannot be dropped without a red test. I am not raising a Required Action to backfill a ledger onto a contributor's ticket.

📡 MCP-Tool-Description Budget Audit

• No description: block added or modified — the diff adds two one-line x-pass-as-object: true keys
• No internal cross-refs, no narrative, no cap pressure

Findings: Pass — N/A in substance; the OpenAPI touch is annotation-only.

🧪 Test-Evidence & Location Audit

• Execution evidence: exact-head required CI 12/12 green at a6d5089b33eb9165b0b5933726c194eb9fef8b88
• Reviewer falsifier: named concern "is the annotation enforced or merely present?" — removed it at PR head, guard failed naming all four dropped arguments, restored. Enforcement confirmed.
• Test location: no new test; the pre-existing canonical guard is re-armed by deletion, which is the correct location by construction

Findings: Pass. Worth stating explicitly because it inverts the usual expectation: the absence of a new test is the strength here, not a gap. A new assertion would have duplicated a guard that already covers the whole surface; removing the exemption makes the existing one do its job.

📋 Required Actions

No required actions — eligible for human merge.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 98 - The annotation belongs in the contract and the enforcement belongs in the surface-wide guard; both landed there. 2 deducted only because x-pass-as-object remains undocumented as a required-for-correctness annotation, so a new server still inherits the trap — pre-existing, named in the guard's own comment, not this PR's to carry.
• [CONTENT_COMPLETENESS]: 95 - PR body is accurate and scoped; the diff removes explanatory comments whose subject no longer exists, which is correct. 5 deducted: nothing records that get_pull_request_diff begins honouring three previously-dropped arguments — a consequence worth one line for the next reader.
• [EXECUTION_QUALITY]: 98 - Minimal, correct, and enforced rather than asserted; legacy tolerance deliberately untouched. Verified the enforcement by falsifier rather than reading. 2 deducted for the unannounced live behavior change covered above.
• [PRODUCTIVITY]: 100 - Both reported operations closed, the guard re-armed, and the known defect set emptied — confirmed by count, not assumed.
• [IMPACT]: 74 - Restores four arguments to two live tools, one of which silently returned whole-repo diffs for file-scoped requests. Bounded to two operations, which is why not higher.
• [COMPLEXITY]: 12 - Two YAML keys and one allowlist deletion; the reasoning was the hard part and it happened in #16250.
• [EFFORT_PROFILE]: Quick Win - High-ratio: a two-line contract change plus a deletion closes two silent-degradation defects and re-arms a surface-wide guard.

The instinct I would keep from this PR is preferring deletion of an exemption over addition of an assertion when a guard already exists. It is the rarer move and it leaves the substrate smaller than it found it.

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