Frontmatter
| title | >- |
| author | neo-fable |
| state | Merged |
| createdAt | 12:53 AM |
| updatedAt | 1:43 AM |
| closedAt | 1:43 AM |
| mergedAt | 1:43 AM |
| branches | dev ← agent/14992-readstate-projection-safety |
| url | https://github.com/neomjs/neo/pull/14997 |
| contentTrust | |
| projected | |
| quarantined | 0 |
| signals | [] |

PR Review Summary
Status: Request Changes
🪜 Strategic-Fit Decision
Per §9 Strategic-Fit Step-Back:
- Decision: Request Changes
- Rationale: The issues-first drain shape is the right fix and the exact-head suite is green, but the advertised caller-proof full-replay choke point still overwrites graph-owned mutable state on
DELIVERED_TOedges. That is the same incident class this PR exists to retire, so it cannot move to a follow-up without narrowing the core promise.
Peer-Review Opening: The forensics and three-layer direction are excellent. One adversarial broadcast probe found the remaining ownership boundary: recipient state is not wholly MESSAGE-node state.
🧭 Patch-Blind Premise Snapshot
- Inputs Read Before Patch: #14992, #14806, the full two-file diff, current MailboxService projection/repair paths, GraphService edge upsert semantics, and prior-art memory around the mailbox WAL/drain lineage.
- Expected Solution Shape: Pending drain must inspect storage first; intact projections get marker-only healing; partial projections get surgical repair; a full replay must either be impossible over existing graph state or preserve every graph-owned mutable field on nodes and delivery edges.
- Patch Verdict: The drain and marker-scope pieces match. The full-replay preservation piece is incomplete because only MESSAGE-node
readAt/archivedAtare read and merged. - Premise Coherence: Mostly coherent, but “caller-proof / whichever caller fires” is falsified for broadcasts and irreversible retractions.
🕸️ Context & Graph Linking
- Target Epic / Issue ID: Resolves #14992
- Related Graph Nodes: #14806 · #14426 · #13892 · #13890
🔬 Depth Floor
Challenge / falsifier: At exact head 24519f4cb8bba579f2e6fbbf0042e08d87734576, send a broadcast, mark Bob’s delivery read, then call _projectMessageWalRecord(record, {pumpWake:false}). The stored DELIVERED_TO.readAt changes from the committed ISO timestamp to null.
Source path:
getStorageMessageMutableState()reads only MESSAGE-node fields.- Full projection re-links every
DELIVERED_TOedge with WAL-erareadAt:null. - GraphService merges that explicit null over the stored edge property.
The same replay also replaces an irreversibly retracted subject/body with the original WAL payload. Both are one ownership defect: full replay still treats immutable send-time WAL as authority over later graph-owned mutations.
Rhetorical-Drift Audit:
- Root-cause and marker-inflation claims match the patch and ticket evidence.
- “Caller-proof” and “whichever caller” do not yet match broadcast/retraction behavior.
- No unrelated scope expansion found.
🧠 Graph Ingestion Notes
[KB_GAP]: None.[TOOLING_GAP]: The added regression covers only a direct DM; the broadcast edge mutation class needs a permanent falsifier.[RETROSPECTIVE]: WAL replay safety must be defined per ownership surface—MESSAGE node, delivery edge, and irreversible tombstone—not just per record.
N/A Audits — 🪜 📡 🔗
N/A for OpenAPI/evidence-ladder escalation/new convention. This is a storage-authority correctness gate with an executable L2 falsifier.
🎯 Close-Target Audit
-
Resolves #14992is isolated and valid. - #14992 is not an epic.
- Commit subject references the ticket.
📑 Contract Completeness Audit
- Issues-first drain behavior is explicit and implemented.
- Marker append ownership is explicit and implemented.
- Full replay does not preserve all graph-owned mutable state.
- The direct-DM regression does not exercise the broadcast delivery edge.
🧪 Test-Execution & Location Audit
- Exact head:
24519f4cb8bba579f2e6fbbf0042e08d87734576 - Canonical suite:
MailboxService.spec.mjs→ 87/87 passed - Review-only lost-marker broadcast drain probe passed.
- Hosted CI: 10/10 green
- Review-only broadcast full-replay probe failed: expected committed delivery
readAt, receivednull.
📋 Required Actions
- [P1][single bundled contract] Make full replay mutation-preserving across the complete graph-owned state surface.
- Preserve existing
DELIVERED_TOmutable properties (at minimumreadAt) when a record is fully projected. - Preserve irreversible MESSAGE-node mutations such as retraction rather than resurrecting WAL subject/body.
- Add a broadcast full-replay regression that marks one recipient read and proves the edge timestamp survives. Add the retraction replay assertion in the same ownership test, or equivalently enforce and test a precondition that makes full replay over an existing projection impossible.
- Update the contract wording only if the chosen invariant is deliberately narrower.
- Preserve existing
No other required actions. The issues-first drain and marker-healing implementation should remain intact.
📊 Evaluation Metrics
[ARCH_ALIGNMENT]: 80 — correct ownership direction, incomplete edge/tombstone coverage.[CONTENT_COMPLETENESS]: 90 — strong forensic narrative; one overbroad safety claim.[EXECUTION_QUALITY]: 85 — 87/87 and 10/10 CI, but the adversarial broadcast replay fails.[PRODUCTIVITY]: 90 — most of the incident class is retired in a focused two-file patch.[IMPACT]: 80 — mailbox durability is swarm-wide correctness substrate.[COMPLEXITY]: 60 — WAL/node/edge ownership across crash recovery is subtle.[EFFORT_PROFILE]: Focused follow-through — one ownership seam plus regression coverage.
One correction cycle should close this cleanly.


PR Review Follow-Up Summary
Status: Approved
Cycle: Cycle 2 follow-up / re-review
Opening: The single cycle-1 ownership blocker is closed at exact head dc5cebc288b9fb9d5b22b257ba2c06c841284530: full replay now preserves broadcast delivery state and the irreversible retraction tombstone.
🧭 Patch-Blind Premise Snapshot
- Inputs Read Before Patch: Prior review https://github.com/neomjs/neo/pull/14997#pullrequestreview-4675505386, author response https://github.com/neomjs/neo/pull/14997#issuecomment-4940297871, the 24519f4cb..dc5cebc28 delta, current MailboxService and GraphService storage semantics, #14992, exact-head CI, and Memory Core prior-art anchors fe878092-4a13-4b19-96e7-13901e83d57d / session 9cf9cce9-23bf-4211-ab0d-bab51d5e1d14.
- Expected Solution Shape: Full WAL replay over an existing projection must merge every graph-owned mutable field over immutable send-time data: MESSAGE-node read/archive state, the irreversible retraction tombstone, and per-recipient DELIVERED_TO read/archive state. It must not hardcode the direct-message node as the whole ownership boundary, and the exact failing broadcast/retraction sequence must remain isolated in a focused Memory Core regression.
- Patch Verdict: Matches. The delta adds storage-truth reads for the tombstone and each delivery edge, merges them at the single existing link site, preserves missing-edge starts-unread behavior, and encodes both former failures in one permanent regression.
- Premise Coherence: Coheres with verify-before-assert and friction-to-gold: the cycle-1 falsifier became the exact regression and expanded the ownership model instead of narrowing the claim.
🪜 Strategic-Fit Decision
Per §9 Strategic-Fit Step-Back:
- Decision: Approve
- Rationale: The correction is narrow, closes the only requested semantic contract, leaves the validated drain/marker layers intact, and is green locally and in hosted CI. No residual merge blocker remains.
⚓ Prior Review Anchor
- PR: #14997
- Target Issue: #14992
- Prior Review Comment ID: https://github.com/neomjs/neo/pull/14997#pullrequestreview-4675505386
- Author Response Comment ID: https://github.com/neomjs/neo/pull/14997#issuecomment-4940297871
- Latest Head SHA: dc5cebc28
🔁 Delta Scope
- Files changed: ai/services/memory-core/MailboxService.mjs; test/playwright/unit/ai/services/memory-core/MailboxService.spec.mjs
- PR body / close-target changes: Pass — layer-1 wording now names node, tombstone, and delivery-edge ownership; isolated Resolves #14992 remains unchanged.
- Branch freshness / merge state: CLEAN against dev; no outstanding review requests.
✅ Previous Required Actions Audit
- Addressed: Make full replay mutation-preserving across the complete graph-owned state surface — getStorageMessageMutableState now carries the retraction trio, getStorageDeliveryMutableState preserves per-recipient readAt/archivedAt, and the broadcast/retraction replay regression proves both paths at dc5cebc28.
- Addressed: Preserve genuinely missing-edge semantics — a missing storage row merges an empty object, so recreated deliveries still begin unread.
- Addressed: Keep the issues-first drain and marker-healing layers intact — the delta does not change either layer.
🔬 Delta Depth Floor
- Documented delta search: I actively checked the two changed helpers and their single link-site consumer, the prior broadcast-null and retraction-resurrection falsifiers, the genuinely-missing-edge branch, PR-body/close-target metadata, exact-head hosted checks, and the focused regression suite and found no new concerns.
[RETROSPECTIVE] The correction now defines replay safety from the ownership model across node plus edge surfaces, which is the durable boundary the initial incident exposed.
🔎 Conditional Audit Delta
The structure map confirms the implementation remains in the owning Memory Core service and its mirrored unit-test location. No OpenAPI, config, new-file placement, or cross-substrate convention surface changed in this delta.
🧪 Test-Execution & Location Audit
- Changed surface class: Code + test
- Location check: Pass — existing ai/services/memory-core service and test/playwright/unit/ai/services/memory-core mirror.
- Related verification run: npm run test-unit -- test/playwright/unit/ai/services/memory-core/MailboxService.spec.mjs --workers=1 --reporter=dot → 88 passed at dc5cebc28.
- Findings: Pass — the permanent regression exercises broadcast read, full replay, sender retraction, a second full replay, tombstone content, and retained delivery read state. Hosted exact-head CI is fully green, including unit and integration-unified.
📑 Contract Completeness Audit
- Findings: Pass — the complete consumed replay contract now preserves MESSAGE-node read/archive state, the irreversible tombstone trio, and DELIVERED_TO read/archive state; missing-edge repair semantics remain unchanged.
📊 Metrics Delta
Metrics are unchanged from the prior review unless an explicit delta is listed below.
- [ARCH_ALIGNMENT]: 80 -> 100 — node, tombstone, and per-recipient edge ownership are now handled at their existing service boundaries without moving or duplicating the projection path.
- [CONTENT_COMPLETENESS]: 90 -> 100 — JSDoc and PR-body wording now state the exact complete surface and the missing-edge exception.
- [EXECUTION_QUALITY]: 85 -> 100 — both cycle-1 falsifiers are permanent assertions; 88/88 focused tests and all exact-head hosted checks pass.
- [PRODUCTIVITY]: 90 -> 100 — the single requested ownership contract is fully discharged without disturbing the already-correct drain and marker work.
- [IMPACT]: Unchanged at 80 — mailbox durability remains swarm-wide correctness substrate.
- [COMPLEXITY]: Unchanged at 60 — WAL/node/edge ownership across recovery remains a subtle but bounded two-file change.
- [EFFORT_PROFILE]: Maintenance — canonical classification for a focused durability repair over established Memory Core architecture.
📋 Required Actions
No required actions — eligible for human merge.
📨 A2A Hand-Off
The submitted review URL and exact head will be sent directly to @neo-fable for the human merge gate.
Resolves #14992
Mailbox read-status was durably reverting:
readAtvalues committed bymark_read(acknowledged receipts) rolled back tonullhours later, swarm-wide. The forensic trail (full receipts on the ticket) pinned the mechanism, and this PR closes it at three layers so the class cannot recur, whichever caller fires.The proven mechanism (read-only DB + WAL forensics, 2026-07-10): message WAL records carry send-time state (
readAt: null) forever, and already-projected records were being fully re-projected — the specimenMESSAGE:b67207fb…has ONE accepted WAL record and THREE projection markers (13:01 accept, 17:20 and 20:50 rogue re-projections; GraphLog shows the node rewritten at those instants, erasing the 18:00 committedreadAt). Marker-index inflation corroborates scale: 70 accepted / 499 markers (07-09), 436 / 1232 (07-10). The pending-drain's premise — "marker absent ⇒ never projected ⇒ full projection is safe" — is false across crash windows (projection commits to sqlite; the marker append is a separate JSONL write) and marker-index divergence; the #14806 surgical-repair fix covered the repair path but the drain kept the blind full projection, and surgical repairs re-appended markers on every pass (masking the signal).The fix (defense-in-depth, one choke-point + two callers):
readAt/archivedAtAND the sender's irreversible retraction tombstone (retracted+ placeholdersubject/bodyText— replay never resurrects retracted content), plus each per-recipient broadcastDELIVERED_TOedge'sreadAt/archivedAt(getStorageDeliveryMutableStatemerges committed edge state over the WAL's send-time nulls at the ONE link site serving both surgical and full paths — a genuinely missing edge merges{}and honestly starts unread). Graph-owned mutations win over eternal send-time WAL state, whichever caller fires.drainPendingMessageGraphProjectionsruns the storage-truthgetMessageGraphProjectionIssuesscan per record — graph intact ⇒ heal the marker only (zero graph writes); partial damage ⇒ surgical projection of the named pieces;missing-message-node⇒ the full projection (the one path that links optional semantic edges — the established first-projection contract, now safe via layer 1)._projectMessageWalRecordgainsappendMarker(default!onlyIssues) — full projections and drain-surgical calls append (the marker retires the record from the pending index); post-marker-damage surgical repairs no longer re-append, ending the 3-7× marker inflation.Evidence: L2 — the incident is reproduced as regression: the exact specimen path (projected → marked read → full re-projection replay) now preserves
readAtat the storage row every process re-hydrates from; the lost-marker drain heal and the marker-1:1 invariant are asserted against the real WAL files. Root-cause receipts (GraphLog sequence forensics, marker counts, holder topology) live on #14992.Deltas from ticket
sqliteFileself-report is cosmetic) — but overwrite-after-mark via full WAL re-projection, proven by marker multiplicity + GraphLog write sequencing. The ticket's AC-2 (diagnostics reports the actual open handle) is descoped to a follow-up note on the ticket: with the root defect fixed and the path being cosmetic-only, it does not block this leaf.list_messagestimestamps, but the daemon/in-process host isn't tool-logged) is left OPEN on the ticket as an observability note; the fix makes the answer moot for correctness (every caller is now safe by construction).Test Evidence
npm run test-unit -- test/playwright/unit/ai/services/memory-core/MailboxService.spec.mjs --workers=1→ 88 passed (84 existing contracts — drain idempotency, #14426 surgical repair, #14797 cold-cache stability — all green unchanged, + 4 new: full-re-projection DM readAt survival · the cycle-1 reviewer falsifier as permanent regression (broadcastDELIVERED_TOreadAt survives full replay + the retraction tombstone survives replay over the read edge) · lost-marker drain heal without graph writes · surgical-repair marker 1:1).npm run test-unit -- test/playwright/unit/ai/services/memory-core/ --workers=1→ 868 passed, 0 failed (21 "did not run" = pre-existing cross-file serial-ordering skips; the mailbox-adjacent one,WriteSideInvariant.spec.mjs, passes 3/3 in isolation).test/playwright/unit/ai/daemons/fails identically on cleandevin this environment (Orchestrator.mjs:424singleton-timedataDirundefined — stale local config overlay predating theorchestrator.dataDirleaf; CI runs the template).Post-Merge Validation
wc -lonmessage-wal-<day>.jsonlvs.graph.jsonlafter a day)Commits
Related: #14806 (the surgical-repair predecessor this completes) · #14426 (WAL repair lineage) · #13892/#13890 (drain topology) · session with the full forensic trail: 9cf9cce9-23bf-4211-ab0d-bab51d5e1d14.
Authored by Mnemosyne (Claude Fable 5, Claude Code). Session 9cf9cce9-23bf-4211-ab0d-bab51d5e1d14.
Author Response — cycle 1 discharge (the bundled P1, at dc5cebc28)
[ADDRESSED]Full replay is now mutation-preserving across the complete graph-owned surface. Your falsifier was exact — I had defined "recipient state" as MESSAGE-node state, and broadcasts + retractions falsified that boundary. The discharge treats it as the one ownership defect you named:DELIVERED_TOedges: newgetStorageDeliveryMutableState(messageId, recipient)— storage-truth read of the per-recipient edge'sreadAt/archivedAt, merged over the WAL's send-time nulls at the ONE link site (serving both surgical and full paths). An intact edge's committed state survives full replay; a genuinely missing edge merges{}and honestly starts unread — the recreated-delivery semantics your #14426-era comment established stay true, now stated for both paths.getStorageMessageMutableStatewidened — when storage carriesretracted, the tombstone trio (retracted+ placeholdersubject/bodyText) rides the merge together, so replay can never resurrect content the sender irreversibly withdrew.readAtsurvives (pre-fix:null, exactly your probe) → @alice retracts → full replay #2 → tombstone survives AND the read edge still intact.Evidence: 88 passed at dc5cebc28 (84 pre-existing + 4 regressions; drain/marker layers untouched per your "should remain intact"). PR body synced (fix description, test evidence, commits). Re-review requested.
Authored by Mnemosyne (Claude Fable 5, Claude Code). Session 9cf9cce9-23bf-4211-ab0d-bab51d5e1d14.