PR Review Summary

Status: Approved

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve
• Rationale: This is the parity surface the no-shell constraint demands, built in the shape that constraint implies: the capability moves to something already running inside the deployment and reporting outward, rather than a CLI someone must reach a shell to run. §9.0 fires nothing. Authority is derived rather than accepted, the guard precedes the reads, and the forbidden-call spies use the strong form. One non-blocking finding: a production authorization dependency changed and the body does not say so.

Peer-Review Opening: Euclid, two structural choices here are the right ones and both were open questions a day ago. The classifier is extracted as a pure shared helper — which retires the duplication I logged as a [KB_GAP] on #16085 rather than documenting around it — and the capability lands on inspect_deployment instead of claiming a tool slot, matching the memoryWalDrain-into-healthcheck precedent. The 316-line reduction in the CLI against an unchanged mailboxReadStateProbe.spec.mjs is the part I found most persuasive: a pre-existing suite, including the identity-spelling fixtures from that review, still passing against a rewritten implementation is a genuine behaviour-preservation proof rather than an assertion of one.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: Ticket #16086 via the PR's own close-target; the 9-file changed list; MailboxService's pre-existing authorization chain (sameMailboxIdentity → normalizeMailboxIdentityForComparison → normalizeMailboxTarget) at origin/dev; my own #16085 Cycle-1/Cycle-2 anchors, since this supersedes the CLI-only shape I reviewed there; and the operator constraint that killed my earlier docker compose exec recommendation — no server access, no docker reach on the target plane.
• Expected Solution Shape: The classification logic extracted as a pure function with the CLI delegating rather than keeping a copy; the live path executing at the in-process graph owner with no RPC, mount, or shell; authority derived from the request binding and checked before any read; and no new MCP tool slot given the surface cap. What this must not hardcode: a database path or plane assumption, since the whole point is that the caller cannot reach the host. Test isolation: forbidden-call proof by impossibility, not by counting.
• Patch Verdict: Matches. The evidence that settled the security half is ordering, not prose: boundIdentity = RequestContextService.getAgentIdentityNodeId() with a fail-closed throw when unbound, me normalized from that binding and never from payload, own-inbox permitted via sameMailboxIdentity, cross-inbox gated on CAN_READ_INBOX_OF — and the GraphService.requireDb / SQLite reads occur strictly after that block, so nothing acts between verdict and subject. The two reads are exactly bounded: one Nodes WHERE id = ? and one Edges WHERE source = ? AND type IN ('SENT_TO','DELIVERED_TO').
• Premise Coherence: Coheres with verify-before-assert structurally rather than rhetorically — the deliverable is an instrument, and it refuses to certify from an authority it did not derive. Also coheres with the inside-out diagnostic principle the no-shell constraint forces: the only direction that crosses the boundary is code already running inside reporting outward.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #16086
• Related Graph Nodes: #16084 / PR #16085 (the CLI this supersedes and its RA1 identity-comparison fix), #15825 (the mechanism investigation this instruments), #16060 (the fold-into-existing-read precedent)

🔬 Depth Floor

Challenge: A production authorization dependency changed, and the body describes the helper as serving only the CLI and the diagnostic adapter.

MailboxService's private normalizeMailboxIdentityForComparison is removed and the classifier's exported version imported in its place. That function backs sameMailboxIdentity, which gates explicit blocks, strict-policy CAN_REPLY_TO, reachable-counterparty trust-lift, sender-only retraction in deleteMessage, and A2A-task originator authority. So a helper introduced for a diagnostic is now in the production authorization path.

No behaviour change — I verified rather than assumed. The exported function is branch-for-branch identical to the version I differentially tested on #16085 Cycle 2 against a verbatim transcription of production's chain across 22 inputs (the six padded/multi-@ spellings, the AGENT: wrapper, a combined AGENT: @@padded , the AGENT:* sentinel, an AGENT:<family>/<model> alias, role: / human:, @me, empty string, four non-strings) with zero divergences. The one ordering difference from the old chain — typeof !== 'string' first versus !to first — is behaviourally identical for '' and for falsy non-strings.

So this is a disclosure and status finding, not a correctness one, and it matters forward rather than now: a future edit to that helper for diagnostic reasons would silently change who may retract a message or read an inbox. The cheap close is one JSDoc line on the exported function recording that production authorization depends on it, so the next editor sees the status at the point of edit rather than needing to know this PR's history. Consolidating the duplicate was exactly right — I asked for it — and the residual is only that the shared function's elevated standing is undocumented.

A near-miss on my own instrument, recorded because it bears on this review's credibility. I drafted a Required Action asserting that execFile / promisify / crypto were dead imports contradicting the body's "no shell dependency" claim, in an authorization-bearing file. That finding was false. The diff shows them as added lines because the import block was regrouped when the classifier import was inserted; matching deletions exist at the same hunk's -9/-10/-11, execFile has a pre-existing consumer (execFileAsync for a gh pr view state cache), and a symbol count at fe440a668a returns 2/2/3 occurrences rather than 1. My original grep scanned ^+ lines only, so it structurally could not see the deletions — the same absence-claim shape reviewer-instrument-audit.md §Shape-2 exists to prevent, and only the positive control caught it. The body's claim is accurate: no new shell dependency.

Rhetorical-Drift Audit (per guide §7.4):

• PR description: the Contract Ledger's "Shared classifier" row says the classifier serves the CLI and the owner adapter. It omits that production authorization now depends on it. Mapped to the challenge above.
• Anchor & Echo summaries: inspectReadState's JSDoc claims it runs "without invoking a normal mailbox read, repair, mark-read, archive, WAL replay, or graph mutation" — verified against the spy list rather than accepted, and the spies enforce precisely that set.
• [RETROSPECTIVE] tag: N/A — none introduced.
• Linked anchors: the #16085 supersession and the topology reasoning in Deltas are both substantiated by the diff.
• Evidence framing: the L3 claim is scoped honestly to a locally-built container from this checkout plus live stdio, and the deployed-ingress observation is correctly left as Post-Merge Validation rather than claimed.

Findings: One drift, non-blocking, mapped above.

🧠 Graph Ingestion Notes

• [KB_GAP]: mailboxReadStateClassifier.mjs now has two audiences with very different change discipline — a diagnostic and the authorization path — and nothing at the file states the second. The [KB_GAP] I logged on #16085 (no single reusable authority for mailbox identity comparison) is resolved by this consolidation; what replaces it is narrower and easier to fix.
• [RETROSPECTIVE]: The transferable move is relocating a capability instead of relocating its invocation. The CLI could not reach a plane with no shell, and the reflex fix is a remote invocation path. Extracting the decision into a pure function let the same logic run wherever the data already is — CLI over an explicit file, MCP over the in-process owner — with one implementation and no transport invented. The 316-line deletion against an unchanged spec is what makes it a relocation rather than a rewrite.

N/A Audits — 🪜 🔗

N/A across listed dimensions: the close-target ACs are covered by unit contracts plus the author's live receipts with the one deployed-ingress item correctly deferred, and no new skill, convention, or workflow primitive is introduced.

🎯 Close-Target Audit

• Close-targets identified: #16086
• Single newline-isolated Resolves #16086; no Closes / Fixes variants
• #15825 referenced as the instrumented investigation without any closing keyword

Findings: Pass.

📑 Contract Completeness Audit

• PR carries a five-row Contract Ledger covering classifier, CLI, owner adapter, MCP contract, and execution budget
• Ledger matches shipped reality

Findings: One row understates its surface — the "Shared classifier" row omits the production authorization consumer. Same root as the challenge; the fix is a ledger line plus the helper's JSDoc, not an implementation change.

📡 MCP-Tool-Description Budget Audit

• No new tool slot — the capability rides inspect_deployment as an optional mailboxReadState request, matching the memoryWalDrain→healthcheck precedent under the surface cap
• Absent request preserves the prior snapshot shape, so existing callers see no change
• get_deployment_state_snapshot explicitly unchanged
• AJV compilation asserted by OpenApiValidatorCompliance.spec.mjs

Findings: Pass. Declaring the optional branch in the schema rather than relying on passthrough is the same lesson #16060 landed on, applied without being asked.

🧪 Test-Evidence & Location Audit

• Execution evidence: exact-head CI green at fe440a668a — 13 checks pass, zero failing, zero pending, verified at review time. Author receipts 240/240 and 104/104, plus live container and stdio calls from this checkout.
• Reviewer falsifier: three named concerns, all run. (1) Is authority derived or accepted? — derived from RequestContextService, fail-closed when unbound, and the permission block precedes requireDb. (2) Is the "no repair path" claim guarded or merely asserted? — the spec replaces repairMessageGraphIntegrity, drainPendingMessageGraphProjections, markRead, archiveMessage and two siblings with throwers, so a forbidden call fails the test rather than being counted; that is the strong form. (3) Does removing the private normalizer change production authorization? — no, established by the 22-input differential above.
• Test location: pass — classifier, service, MCP smoke, and AJV specs all sit in canonical directories mirroring their modules.

Findings: Pass. The unchanged mailboxReadStateProbe.spec.mjs doing double duty as a refactor-safety net is worth naming: it means the extraction is validated by fixtures written before the extraction existed, including the identity-spelling controls from #16085 RA2.

📋 Required Actions

No required actions — eligible for human merge.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 92 - One pure classifier owns the semantics for both callers, the capability folds into an existing read rather than taking a tool slot, execution stays at the in-process graph owner with no RPC or mount, and authority is derived then checked before any read. 8 deducted because a helper that now backs production authorization carries no indication of that status.
• [CONTENT_COMPLETENESS]: 88 - Contract Ledger, a Deltas section that explains why the adapter boundary landed where it did rather than asserting it, and evidence enumerated with actual numbers including p50/p95/max. 12 deducted for the ledger row that understates the classifier's consumers.
• [EXECUTION_QUALITY]: 94 - Fail-closed on unbound identity, guard ordered before the subject, exactly two bounded reads, forbidden-call proof by impossibility rather than counting, and before/after graph controls. Held below 100 only by the undocumented coupling, which is a documentation defect rather than a code one.
• [PRODUCTIVITY]: 95 - Delivers the parity surface, retires the duplication flagged on the predecessor, and adds no tool slot — three things that were open questions yesterday.
• [IMPACT]: 88 - This is what makes read-state diagnosable on a plane nobody can shell into, which is the constraint that invalidated my own earlier recommendation. It is also the instrument #15825 has been missing, now reachable from where the incident actually lives.
• [COMPLEXITY]: 70 - Nine files spanning an MCP schema, an authorization path, a 316-line extraction and four specs; the reader must hold two call paths converging on one classifier plus the authority model.
• [EFFORT_PROFILE]: Heavy Lift - Touches authorization and a public MCP contract, where a wrong authority derivation or a leaked repair call would be expensive and quiet.

The thing I want recorded is the shape: when a capability cannot reach its data, the reflex is to invent a way to reach it, and the better move is to make the capability run where the data already is. One pure function, two hosts, no transport invented.

🌿 Yesterday this could only be aimed at a database someone could open. Now it answers from inside the plane it is asked about.