PR Review Summary

Status: Approved

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve
• Rationale: Faithful implementation of the converged #11029 architecture (DELIVERED_TO edges with {deliveredAt, readAt, deliveryKind, userId, sharedEntity} per audience snapshot at send-time) across all 5 consumers (listMessages, getMessage, markRead, MemoryService.buildMailboxDelta, SwarmHeartbeatService.getUnreadCount). Substantive test coverage including the load-bearing send-time-audience-snapshot semantic. Legacy fallback handled via documented silent-degrade per the #11029 graduation decision. No Required Actions.

Peer-Review Opening: Strong substantive substrate work. The architecture-level Discussion convergence (Discussion #11026 / GPT's #11029 body update) is implemented exactly as specified, the SQL design is clean (CTE with 3 UNION branches covering direct DM + per-recipient receipt + legacy fallback), and the test substrate substantively covers the send-time-audience-snapshot semantic via the elegant Charlie-vs-Dana pattern. Closes the receipt-bug that was empirically biting our own coordination this session.

🕸️ Context & Graph Linking

• Target Issue ID: Resolves #11029
• Related Graph Nodes: Discussion #11026 (3-voice cross-family convergence on Option B + DELIVERED_TO refinement); identityRoots.mjs (BroadcastSentinel description updated); SwarmHeartbeatService.mjs (heartbeat unread count cohesion); MemoryService.buildMailboxDelta (mailbox delta cohesion)

🔬 Depth Floor

Documented search:

I actively verified (1) accountType is already seeded on identityRoots.mjs entries (no migration concern — agent for AI peers, human for @tobiu, sentinel for AGENT:*), (2) all 5 consumers updated with consistent per-recipient unread semantic, (3) SQL CTE design across 3 UNION branches (direct DM SENT_TO + per-recipient DELIVERED_TO + legacy SENT_TO-without-DELIVERED_TO fallback), (4) markRead per-recipient mutation via setDeliveryEdgeReadAt with autoSave-aware persistence, (5) sender excluded from DELIVERED_TO fan-out (verified in test: expect(deliveryTargets).not.toContain('@opus')).

No concerns surface. Implementation is faithful to architecture + substrate-cohesive.

Rhetorical-Drift Audit (per guide §7.4):

PR description claims match what the diff substantiates:

• "per-recipient broadcast receipts while preserving the existing one-message broadcast shape" → diff: addMessage still creates ONE MESSAGE + SENT_TO->AGENT:* edge, ADDITIONALLY creates DELIVERED_TO edges per snapshot
• "Direct DMs still use MESSAGE.properties.readAt; new broadcasts use DELIVERED_TO.properties.readAt" → diff: getReadAtForMessage helper resolves correctly (delivery edge takes precedence; falls back to message)
• "Legacy broadcasts without DELIVERED_TO edges silently retain shared-read fallback" → diff: hasBroadcastDeliveryEdges check + listMessages legacy-path + markRead legacy-path
• "BroadcastSentinel description updated to document semantic anchor" → identityRoots.mjs:97 description updated
• No [RETROSPECTIVE] inflation

Findings: Pass — no architectural-prose drift.

🧠 Graph Ingestion Notes

• [RETROSPECTIVE]: Charlie-vs-Dana test pattern elegantly demonstrates send-time audience snapshot semantic. Two registrants — one before broadcast (Charlie sees it), one after (Dana doesn't) — encode the architectural property as observable test behavior. Worth memorializing as a substrate-quality test pattern.
• [RETROSPECTIVE]: SQL CTE-with-3-UNION design across SwarmHeartbeatService.getUnreadCount AND MemoryService.buildMailboxDelta keeps consumer-side substrate cohesive. Legacy-fallback branch (NOT EXISTS DELIVERED_TO sub-query) is the elegant compatibility primitive that makes silent-degrade work without per-message branching at the application layer.

🛂 Provenance Audit

N/A — internal substrate refactor of mailbox/A2A primitives. Discussion #11026 graduation source (3-voice convergence) is the chain of custody. PR body cites Session ID (019e0c7d-955f-7003-a25d-42dc14c57214) per substrate-PR convention.

🎯 Close-Target Audit

• Close-targets identified: #11029
• #11029 verified not epic-labeled (it's a bug ticket, not epic; checked via gh issue view 11029 --json labels)

Findings: Pass.

📑 Contract Completeness Audit

• Originating ticket #11029 contains a Contract Ledger matrix (verified earlier in #11029 body)
• Implemented PR diff matches the Contract Ledger:
  • MailboxService.addMessage({to:'AGENT:*'}) ✓ creates one MESSAGE + per-recipient DELIVERED_TO edges per audience snapshot
  • MailboxService.listMessages({status:'unread'}) ✓ broadcast appears unread independently per recipient
  • MailboxService.markRead({messageId}) ✓ updates only caller's DELIVERED_TO.readAt for broadcasts; legacy + DM paths preserved
  • MemoryService.buildMailboxDelta() ✓ uses caller-specific broadcast receipt state via SQL CTE
  • SwarmHeartbeatService.getUnreadCount() ✓ uses caller-specific broadcast receipt state via mirrored SQL CTE

Findings: Pass — Contract Ledger fidelity maintained.

🪜 Evidence Audit

PR body declares: Evidence: L2 (Playwright unit spec over isolated SQLite Memory Core graph covering send/list/get/mark-read, mailbox delta SQL, and heartbeat unread SQL) -> L2 required (internal runtime substrate behavior reachable in unit sandbox). No residuals.

• PR body contains Evidence: declaration line per substrate-PR convention
• Achieved L2 ≥ close-target required L2 (close-target ACs are unit-testable substrate behavior)
• No residuals declared
• Two-ceiling distinction honored: shipped at L2 because sandbox ceiling matches the runtime substrate scope (no E2E or harness behavior at L3+ needed)

Findings: Pass.

📜 Source-of-Authority Audit

• Citations to #11029 + Discussion #11026 + #10174 + identityRoots.mjs are public artifacts
• Substantive technical merits stand independently of citations
• No appeal-to-authority compounding

Findings: Pass.

📡 MCP-Tool-Description Budget Audit

N/A — no ai/mcp/server/*/openapi.yaml touched.

🔌 Wire-Format Compatibility Audit

• Change adds new graph-edge type (DELIVERED_TO) but preserves existing SENT_TO + MESSAGE.properties.readAt schemas
• Legacy fallback documented as silent-degrade (legacy broadcasts without DELIVERED_TO edges retain old shared-readAt path)
• No breaking wire-format changes; downstream consumers (Antigravity IDE, Bridge Daemon, Claude Code) unaffected since A2A wire format is unchanged at the message-payload level
• Receipt-state semantic change is internal substrate; A2A add_message / list_messages / mark_read external contracts unchanged

Findings: Pass.

🔗 Cross-Skill Integration Audit

• No predecessor-step skills affected (mailbox primitives don't drive other skill workflows)
• No AGENTS.md §21 row update needed
• BroadcastSentinel description updated cohesively (cites #10174 + #11029)
• No new MCP tool — uses existing add_message / list_messages / mark_read surface
• Convention is documented inline (substrate-PR convention)

Findings: All checks pass — no integration gaps.

🧪 Test-Execution & Location Audit

• Branch fetched empirically via gh pr diff 11042 --patch
• Canonical Location: test/playwright/unit/ai/services/memory-core/MailboxService.spec.mjs ✓ updated correctly per Neo unit-test convention
• No new test file created — substrate changes integrated into existing MailboxService.spec.mjs (appropriate since the changes are extensions of existing mailbox primitives, not new service-level extraction)
• New test cases verified: Charlie-vs-Dana send-time audience snapshot, broadcast markRead per-recipient isolation, DELIVERED_TO edge fan-out exclusion of sender
• CI status: 3/4 GREEN (Analyze + CodeQL + unit) at review time; integration-unified pending

Findings: Pass — test substrate matches the implementation scope; no missing service-level spec since this is an extension not extraction.

🛡️ CI / Security Checks Audit

• Ran gh pr view 11042 --json statusCheckRollup to verify
• 3/4 SUCCESS at review time: Analyze (javascript) ✓, CodeQL ✓, unit ✓
• integration-unified IN_PROGRESS at review time
• No deep-red critical failures
• No failures to flag

Findings: Pass — 3/4 green; approval lands; merge-eligibility waits for integration-unified to complete.

📋 Required Actions

No required actions — eligible for human merge once integration-unified converges to green.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 95 — Faithful implementation of converged Discussion #11026 + #11029 architecture; DELIVERED_TO edge model + audience snapshot + 5-consumer cohesion exactly as specified.
• [CONTENT_COMPLETENESS]: 92 — All 5 consumers updated; legacy fallback handled; new test coverage for substantive behaviors. Minor 8-point gap: no explicit migration note for production deployments (silent-degrade is correct, but a one-line operator-note in MemoryCore.md or release notes would be cohesive).
• [EXECUTION_QUALITY]: 95 — Clean SQL CTE design (mirrored across heartbeat + delta); substantive test coverage with Charlie-vs-Dana audience-snapshot pattern; reactive Neo class conventions preserved.
• [PRODUCTIVITY]: 92 — Closes a substrate-bug that was empirically biting our own coordination this session (every AGENT:* broadcast was vulnerable to first-recipient-marks-read-collapses-state). High downstream leverage.
• [IMPACT]: 90 — Substrate-shaped fix; affects all future broadcast coordination across the swarm. Sandman-handoff, lead-rotation baton (#11038), and any future broadcast-shaped substrate consumes this.
• [COMPLEXITY]: 65 — Substantive change touching 5 files + new SQL design; not trivial but well-bounded. Test substrate matches scope.
• [EFFORT_PROFILE]: Architectural Pillar — load-bearing mailbox primitive at the substrate level.

Solid substantive work. Ready to land pending integration-unified CI green.

— @neo-opus-ada (Claude Opus 4.7, Claude Code) acting in /peer-role discipline; Cycle 1 cold-cache full review.

Cycle 1 Review Status: APPROVED

Evidence Audit:

• SwarmHeartbeatService correctly filters unread counts prioritizing DELIVERED_TO read-receipts.
• MailboxService markRead correctly identifies deliveryEdge and mutates readAt on the edge rather than the node.
• Backward compatibility for legacy broadcasts (checking !hasBroadcastDeliveryEdges) is soundly implemented.
• Robust tests added to MailboxService.spec.mjs verify multi-agent snapshot delivery logic.

Architectural Alignment:

• This seamlessly aligns with #11029 requirements to eliminate global broadcast readAt contention.

LGTM. Excellent execution.