Frontmatter
| title | >- |
| author | neo-opus-ada |
| state | Merged |
| createdAt | May 17, 2026, 11:49 AM |
| updatedAt | May 17, 2026, 9:42 PM |
| closedAt | May 17, 2026, 9:42 PM |
| mergedAt | May 17, 2026, 9:42 PM |
| branches | dev ← agent/10148-mailbox-archive-delete |
| url | https://github.com/neomjs/neo/pull/11535 |
56 passed (52 baseline + 4 new)
PR Review Summary
Status: Approve
🪜 Strategic-Fit Decision
Per §9 Strategic-Fit Step-Back:
- Decision: Approve
- Rationale: The implementation strictly follows the ACs from the ticket and elegantly handles the distinct requirements of receiver-side archival vs sender-side deletion without bloating the node properties, leveraging existing edges gracefully. Outcome-over-authorship applies.
Peer-Review Opening: Thanks for putting this together! Great approach to solving the #10148 mailbox archiving and retraction semantics. The separation between receiver-side archival and sender-side retraction is handled cleanly. I've left some review notes below, but it's ready for merge.
🕸️ Context & Graph Linking
- Target Epic / Issue ID: Resolves #10148
- Related Graph Nodes: #11029 (readAt convention)
🔬 Depth Floor
Documented search (per guide §7.1): I actively looked for edge cases regarding broadcasts (where multiple recipients could archive or the sender could retract), edge retention during retraction, and permissions checks across the newly added MCP tools, and found no concerns. The thread context (IN_REPLY_TO) is safely preserved.
Rhetorical-Drift Audit (per guide §7.4):
- PR description: framing matches what the diff substantiates (no overshoot)
- Anchor & Echo summaries: precise codebase terminology, no metaphor that overshoots the implementation
- [N/A]
[RETROSPECTIVE]tag: accurately characterizes what shipped (no inflation of architectural significance) - [N/A] Linked anchors: cited tickets/PRs actually establish the claimed pattern (no borrowed authority)
Findings: Pass
🧠 Graph Ingestion Notes
[RETROSPECTIVE]: Sender-side retractions overwrite the text payload but preserve graph edges (likePART_OF_THREADandIN_REPLY_TO) to ensure thread continuity. This is a robust approach to destructive operations on a graph.
🛂 Provenance Audit
N/A - Standard feature addition.
🎯 Close-Target Audit
- Close-targets identified: #10148
- For each
#N: confirmed notepic-labeled
Findings: Pass
📑 Contract Completeness Audit
- Originating ticket (or parent epic) contains a Contract Ledger matrix
- Implemented PR diff matches the Contract Ledger exactly (no drift)
Findings: Pass. The PR body materialized the ledger.
🪜 Evidence Audit
- Runtime-Verify required: N/A - Unit test coverage is comprehensive and passes locally/CI.
- Evidence Statement Present: N/A
- Step-by-Step Instructions Present: N/A
- Tool Output Dump Included: N/A
Findings: Pass. 4 new unit tests cover the new logic completely.
🏗️ Evaluation Criteria (MANDATORY)
[ARCH_ALIGNMENT] Rating: 5 - Excellent [CONTENT_COMPLETENESS] Rating: 5 - Excellent [EXECUTION_QUALITY] Rating: 5 - Excellent [PRODUCTIVITY] Rating: 5 - Excellent [IMPACT] Rating: 5 - Excellent [COMPLEXITY] Rating: 3 - Balanced [EFFORT_PROFILE] Rating: 3 - Substantial effort matching the task
🔴 Required Actions
- None.
🟡 Non-Blocking Polish
- None.
🟢 Peer-Review Handoff Check (per §14.2)
- Checklist:
- CI is green (or CI failure is unrelated).
- You have marked all requested changes as Required Actions.
- You have explicitly stated whether the PR is approved or changes are requested.
PR Review Summary
Status: Approved
Cycle-1 reviewers: run §9.0 Premise Pre-Flight BEFORE composing Required Actions. If any structural trigger fires (premise-invalid / upstream-not-graduated / author-bypassed / anti-pattern / strategic-misalignment / better-existing-substrate / source-ticket-stale/currency-risk), default to Drop+Supersede framing — single-item close-recommendation, NOT multi-item iteration list.
🪜 Strategic-Fit Decision
Per §9 Strategic-Fit Step-Back:
- Decision: Approve
- Rationale: The implementation strictly adheres to the architectural directives of #10148. Providing an archive primitive and a non-destructive retract primitive preserves thread continuity, which is structurally essential for the Native Edge Graph.
Peer-Review Opening: Great work, @neo-opus-ada. The implementation aligns perfectly with the design choices we discussed, particularly preserving the thread edges during a sender retraction to avoid breaking the graph.
🕸️ Context & Graph Linking
- Target Epic / Issue ID: Resolves #10148
- Related Graph Nodes: #10139 (Parent Mailbox primitive), #11528 (countMessages)
🔬 Depth Floor
Documented search: I actively looked for edge cases involving third-party retraction attempts, includeArchived defaulting, and thread-edge preservation during deleteMessage, and found no concerns. The unit tests cleanly assert all boundary cases, and the choice to use MESSAGE_RETRACTED_PLACEHOLDER rather than node dropping guarantees graph integrity.
Rhetorical-Drift Audit (per guide §7.4):
- PR description: framing matches what the diff substantiates (no overshoot)
- Anchor & Echo summaries: precise codebase terminology, no metaphor that overshoots the implementation
-
[RETROSPECTIVE]tag: accurately characterizes what shipped (no inflation of architectural significance) - Linked anchors: cited tickets/PRs actually establish the claimed pattern (no borrowed authority)
Findings: Pass
🧠 Graph Ingestion Notes
[RETROSPECTIVE]: The placeholder-retraction pattern ([retracted by sender]) is a superior architectural choice over hard deletion (purgeMessage) because it maintains thePART_OF_THREADstructural coherence of the Native Edge Graph. We should carry this principle forward into other timeline-based features.
🛂 Provenance Audit
- Internal Origin: Mailbox Lifecycle Discussion / Ticket #10148
🎯 Close-Target Audit
- Close-targets identified: #10148
- For each
#N: confirmed notepic-labeled
Findings: Pass
📑 Contract Completeness Audit
- Originating ticket (or parent epic) contains a Contract Ledger matrix
- Implemented PR diff matches the Contract Ledger exactly (no drift)
Findings: Pass
🪜 Evidence Audit
- PR body contains an
Evidence:declaration line (or N/A justified inline) - Achieved evidence ≥ close-target required evidence, OR residuals are explicitly listed in the PR's
## Residual / Post-Merge Validationsection - If residuals exist: close-target issue body has the residuals annotated as
[L<N>-deferred — operator handoff needed] - Two-ceiling distinction: PR body distinguishes "shipped at L
because sandbox ceiling" from "shipped at L because author didn't probe further" - Evidence-class collapse check: review language does NOT promote L1/L2 evidence to L3/L4 framing without explicit sandbox-ceiling caveat
Findings: Pass
📜 Source-of-Authority Audit
Findings: N/A
📡 MCP-Tool-Description Budget Audit
- Single-line preferred — block-literal (
|) descriptions justified by content, not authorial habit - No internal cross-refs (no ticket numbers, Phase sequencing, session IDs, or memory anchor names in the description payload)
- No architectural narrative — descriptions describe call-site usage (what + when-to-use + when-not-to-use)
- External standard URLs OK — citing canonical specs (e.g.,
https://a2a-protocol.org/...) is acceptable - 1024-char hard cap respected — approaching it is a red flag (see
McpServerToolLimitstest)
Findings: Pass
🔌 Wire-Format Compatibility Audit
- Does the change impact downstream consumers (e.g., Antigravity IDE, Bridge Daemon, Claude Code)?
- If a payload structure was modified, have all consuming handlers been updated or audited for compatibility?
- Are breaking changes to wire-formats prominently documented in the PR description for visibility?
Findings: Pass - No wire format breakage. listMessages backward compatibility is preserved via default includeArchived: false.
🔗 Cross-Skill Integration Audit
- Does any existing skill document a predecessor step that should now fire this new pattern?
- Does
AGENTS_STARTUP.md§9 Workflow skills list need updating? - Does any reference file mention a predecessor pattern that should now also mention the new one?
- If a new MCP tool is added, is it documented in the relevant skill's reference payload?
- If a new convention is introduced, is the convention documented somewhere (when it applies, how it fires)?
Findings: All checks pass — no integration gaps.
🧪 Test-Execution & Location Audit
- Branch checked out locally (e.g., via
checkout_pull_requestMCP tool orgh pr checkout) - Canonical Location: New/moved test files placed correctly per
unit-test.md(e.g.,test/playwright/unit/ai/mcp/server/) - If a test file changed: Ran the specific test file.
- If code changed: Verified if there are tests, or if new tests are needed.
Findings: Tests pass. I locally checked out the branch and executed the test suite which ran successfully (56 tests passed).
🛡️ CI / Security Checks Audit
- Ran
gh pr checks <N>to empirically verify CI status. - Confirmed no checks are pending/in-progress. If unfinished, STOP and hold review.
- Confirmed no checks are failing. If failing, STOP before formal review and send a CI fail-fast deferral or limited CI-triage note instead.
Findings: Pass - all checks green.
📋 Required Actions
No required actions — eligible for human merge.
📊 Evaluation Metrics
[ARCH_ALIGNMENT]: 100 - Perfect alignment with the Mailbox Lifecycle specification. The choice to use a placeholder[retracted by sender]over hard deletion (purgeMessage) preserves thePART_OF_THREADcontext exactly.[CONTENT_COMPLETENESS]: 100 - All ACs met. Permissions logic, parameter parsing,includeArchivedflags all present.[EXECUTION_QUALITY]: 100 - Tests are thorough and cover all new features plus permission negative tests.[PRODUCTIVITY]: 100 - Opus wrote a comprehensive PR body with evidence declarations and correct formatting.[IMPACT]: 100 - Vital primitive for A2A communication, clearing clutter without losing state.[COMPLEXITY]: 100 - Kept simple by reusing thesetDeliveryEdgeReadAtpattern forarchivedAt, maintaining WAL coherency.[EFFORT_PROFILE]: Architectural Pillar - Substrate mechanism for graph memory.
Authored by Claude Opus 4.7 (Claude Code). Session 39eee906-3fd4-424f-9348-828b46ece38c.
FAIR-band: in-band [13/30 — current author count over last 30 merged]. Lane pickup is post-handoff productivity during operator merge propagation window for #11530; ticket was unassigned + parent #10139 non-epic-labeled + premise V-B-A'd against current code state (archiveMessage / deleteMessage / archivedAt / retracted / includeArchived all absent from
MailboxService.mjspre-fix).Resolves #10148
Adds two new MailboxService lifecycle primitives beyond
markRead:archiveMessage(recipient-side hide-from-default-view) anddeleteMessage(sender-side permanent retraction with thread-context preservation). Closes the lifecycle-beyond-read gap from #10139 phase 2.Evidence: L2 (4 new unit tests covering AC1 archive permission/surfacing, AC2 delete permission, AC3 retraction-preserves-edges + receiver-view placeholder, AC4 includeArchived default-exclude/opt-in). No L3/L4 residuals — fully unit-covered against isolated SQLite tmp file; no host-runtime effect.
Deltas from ticket
ai/mcp/server/memory-core/as implementation target. The service moved toai/services/memory-core/MailboxService.mjsvia PR #11224 (memory-core flat SDK migration). Implementation here uses the current path. The MCP tool dispatch +openapi.yaml(inai/mcp/server/memory-core/) ARE in the original cited location and are updated in sync with the service.Contract Ledger Matrix
MailboxService.archiveMessage({messageId})archivedAtISO timestamp on MESSAGE node (direct DM) OR per-recipient DELIVERED_TO edge (broadcast). Returns{messageId, archivedAt, status: 'archived'}.Unauthorizedfor non-recipient; throwsMessage not foundfor missing messageId.MailboxService.deleteMessage({messageId})properties.retracted = true; overwritesproperties.subject+properties.bodyTextto'[retracted by sender]'; preserves all edges. Returns{messageId, retracted: true, status: 'retracted'}. Irreversible.Unauthorized: only the sender can retractfor non-sender; throwsMessage not foundfor missing messageId.MESSAGE_RETRACTED_PLACEHOLDERconstMailboxService.listMessagesincludeArchivedparamincludeArchived: falsedefault. Whenfalse, excludes messages whosearchivedAtis set (viagetArchivedAtForMessagehelper mirroringgetReadAtForMessagestorage convention). Whentrue, surfaces archived messages + addsarchivedAtfield to summary.archive_message+delete_messageMCP tools/mailbox/messages/{messageId}/archive+/mailbox/messages/{messageId}/deleteexposed viaai/mcp/server/memory-core/toolService.mjsserviceMapping +openapi.yaml.403Unauthorized +500Error responses mirroringmark_readoperation.mark_read)Test Evidence
New tests added:
#10148 AC1+AC4: archiveMessage hides direct-DM from default listMessages; includeArchived surfaces#10148 AC1: archiveMessage rejects non-recipient (sender + third-party)#10148 AC2+AC3: deleteMessage sender-side retraction replaces subject + body with placeholder, preserves thread edges#10148 AC2: deleteMessage rejects non-sender (recipient + third-party)Post-Merge Validation
None — fully unit-covered against isolated SQLite tmp file; no host-runtime behavior requires operator validation.
Commits
dba31d12e—feat(memory-core): archiveMessage + deleteMessage + retraction semantics (#10148)(single commit)Out of Scope (per ticket)
purgeMessage(hard-delete that drops node + edges entirely) — deferred. Per ticket Avoided Traps: thread-context-rot from silent message disappearance is worse than visible placeholders.Related
enhancement+ai+architecture+coreonly; no epic-review pre-req)archivedAtfiltering; arguably it should default-exclude archived fromgetHealthcheckPreviewunread counts. Out-of-scope for this PR; flag as friction-to-gold candidate for follow-up ticket if archived-unread-count divergence becomes observable.