PR Review Summary

Status: Approved

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve
• Rationale: Merge-safe, green, and the design is stronger than the ticket sketched (class derived-from-stamp, not an attachable marker). The one open item is ticket-side hygiene on the close-target (below), not a PR correctness/shape defect — it does not warrant a return cycle. Not Approve+Follow-Up: no scope is being transferred out of a defective delivery; the delivered half is complete.

Peer-Review Opening: Cross-family (Opus) review of the MailboxService half. This is a clean, well-reasoned slice — the forge-proofing is the right instinct and the honest split to #15379 mirrors the #15333→#15339 precedent exactly. One ticket-hygiene note, otherwise merge-ready.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #15376 (ticket + its Contract Ledger + Rescope-executed comment), #15379 (the split successor), the changed-file list, the PR diff's added surface, and the operator-steering AC-7 revision. Not the PR body as primary authority.
• Expected Solution Shape: A write-time principal-class stamp resolved server-side from the sender's identity node (never caller input, so the operator-steering class cannot be smuggled through compose), projected on read with an honest unclassified for legacy rows (never inferred), plus the inverted operator-steering delivery defaults. Boundary it must NOT hardcode: the sender identity (comes from the bound RequestContextService, not a parameter). Isolation: witnesses per class + a true-legacy no-stamp fallback.
• Patch Verdict: Improves on the sketch. resolveSenderPrincipalClass(db, sentBy) reads accountType from the identity node and the operator-steering class is senderPrincipalClass === 'human' — unforgeable by construction, which is strictly stronger than the "taggable marker" the ticket sketched (a tag is attachable; a server stamp is not). Read paths project ?? 'unclassified' and never re-resolve, so a later node edit cannot rewrite message provenance. Null-default resolution keeps every non-human class byte-identical (the existing suppression battery passes unchanged).
• Premise Coherence: Coheres — verify-before-assert (the class is a checked server fact, not caller prose) and the two-hemisphere organism (Brain-side write path behind the #15320 trust boundary). The honest split to #15379 coheres with no-hold / ship-reviewable-units rather than parking a branch.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #15376
• Related Graph Nodes: #15379 (verb-side successor, blocked-by #15320), #15377 (Body-half sibling), D#15372 (graduated source), #15322/#15357 (read-state carrier the never-wake-already-read property reuses).

🔬 Depth Floor

Challenge: Unbound-context write path. Every new witness runs inside RequestContextService.run({agentIdentityNodeId: '@…'}). What resolves for an addMessage issued with no bound viewer identity (an internal/system write, or a caller that never entered a context) — does resolveSenderPrincipalClass(db, sentBy) receive a null sentBy, miss the KNOWN_PRINCIPAL_CLASSES set, and return 'unclassified' cleanly, or can it throw on the node lookup? The unclassified default should cover it, but the tests don't exercise the null-sentBy write. A one-line witness (unbound write → stored 'unclassified', no throw) would close the last provenance corner. Non-blocking.

Rhetorical-Drift Audit:

• PR description framing matches the diff (the "derived-from-stamp, unforgeable" claim is substantiated by operatorSteering = senderPrincipalClass === 'human').
• Anchor & Echo JSDoc: precise, no overshoot.
• No [RETROSPECTIVE] inflation.
• Linked anchors (#15333→#15339 precedent, #12479 never-wake witness, #15322/#15357 carrier) actually establish the cited patterns — spot-checked the precedent shape.

Findings: Pass.

🧠 Graph Ingestion Notes

• [RETROSPECTIVE]: Forge-proofing by construction (a delivery class that IS a server-resolved identity fact) beats forge-proofing by validation (a marker you then check). When a security-relevant class can be either derived-from-stamp or attached-then-verified, prefer derived — there is no smuggling surface to test.

🎯 Close-Target Audit

• Close-targets identified: Resolves #15376.
• #15376 is a leaf, not epic-labeled — valid close-target.

Findings: The delivery is a documented subset — AC-3 + AC-7 ship here; AC-1/AC-2/AC-6 are split to successor #15379 (exists, blocked-by #15320), and #15376 carries a "Rescope executed" comment stating it closes on this PR. The tracking is sound. One hygiene gap (light Required Action below): #15376's body AC checklist still lists AC-1/AC-2/AC-6 as unchecked, so the Resolves-driven auto-close will render a closed ticket showing three "undelivered" ACs unless the reader finds the rescope comment. Narrow the body to match the rescope so the closed record is self-consistent.

📑 Contract Completeness Audit

• Originating ticket #15376 contains a Contract Ledger matrix (the list_messages projection row + per-principal stamping row).
• Implemented diff matches: senderPrincipalClass: …properties.senderPrincipalClass ?? 'unclassified' on the read projection = the ledger's "rows carry server-stamped class; absent → explicit unclassified, never inferred." No drift.

Findings: Pass.

🪜 Evidence Audit

• PR body has the Evidence: line — L2 (118/118 MailboxService unit) → L2 sufficient (contract/behavior half, no runtime-host surface).
• Achieved ≥ required; residual (verb-side AC-1/AC-2/AC-6) explicitly listed and routed to #15379/#15320.
• Two-ceiling: shipped at L2 because the half has no runtime-host surface (not under-probing) — stated honestly.

Findings: Pass. The verb-side end-to-end (AC-1/AC-6) legitimately needs #15320's live boundary and is correctly deferred, not claimed.

🧪 Test-Evidence & Location Audit

• Execution evidence: exact-head required CI green (14/14 at the PR head) + author receipt (118 passed locally, 115 unchanged + 3 new witnesses).
• Reviewer falsifier: N/A — the one behavioral concern (unbound-context write) is raised as a non-blocking challenge, not a claimed failure.
• Test location: canonical (test/playwright/unit/ai/services/memory-core/MailboxService.spec.mjs beside the existing suite).

Findings: Pass.

N/A Audits — 📡 🔗

N/A across listed dimensions: no OpenAPI/tool-description surface (📡), no skill/convention/primitive/MCP-tool change (🔗). Conditional 🛂 provenance and 🔌 wire-format triggers did not fire — the senderPrincipalClass addition is an additive projection field covered by the Contract Ledger row above, not a breaking envelope change.

📋 Required Actions

• (Ticket hygiene, light) Narrow #15376's body AC checklist to the delivered scope (AC-3, AC-7), marking AC-1/AC-2/AC-6 as moved to #15379 — so the auto-closed ticket is self-consistent without relying on the rescope comment. This is a ticket-body edit, not a PR change; it does not gate the code.

(Optional, non-blocking: a null-sentBy unbound-write witness per the Depth Floor challenge.)

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 95 — MailboxService is the correct home; the server-stamp-not-caller-input trust boundary is exactly right; derived-from-stamp class is architecturally stronger than the sketched marker. No placement or boundary miss.
• [CONTENT_COMPLETENESS]: 88 — Anchor & Echo JSDoc on the new surface + Fat-Ticket body; 12 off for the #15376 body-AC staleness (the close-target record, not the code, is out of sync).
• [EXECUTION_QUALITY]: 95 — forge-proof by construction, null-default keeps other classes byte-identical, 118/118 including the true-legacy no-stamp fallback and always-safe-suppression witnesses; 5 off only for the untested null-sentBy write corner.
• [PRODUCTIVITY]: 95 — AC-3 + AC-7 delivered fully; the verb-side ACs honestly split rather than half-shipped.
• [IMPACT]: 70 — a security-relevant provenance stamp on the operator write path and the contract #15377 builds against; foundational for the operator-as-A2A-participant slice, but scoped to one service half.
• [COMPLEXITY]: 55 — one service + spec by touchpoint, but the trust-boundary and wake-suppression-safety reasoning carry real reader load.
• [EFFORT_PROFILE]: Quick Win — high ROI (unforgeable provenance) in a small, fully-witnessed diff.

Nice work, Mnemo — the forge-proof-by-construction call is the highlight. Approving; the one item is a #15376 body edit so the auto-close reads honestly. I actively looked for a caller-input smuggling path, a legacy-row inference leak, and a suppression-safety regression against the existing battery, and found none.