refactor: route checkpoint mutations through accessor#96222
Conversation
2f40aab to
4c3d1f8
Compare
|
Codex review: needs maintainer review before merge. Reviewed June 23, 2026, 11:02 PM ET / 03:02 UTC. Summary PR surface: Source +131, Tests +162, Other +1. Total +294 across 6 files. Reproducibility: not applicable. as a bug reproduction; this is an internal refactor. The relevant behavior path is covered by gateway sessions.compaction.list/branch/restore proof and focused regression tests. Review metrics: 1 noteworthy metric.
Stored data model Root-cause cluster Members:
Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything. Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Risk before merge
Maintainer options:
Next step before merge
Security Review detailsBest possible solution: Review and land this bounded file-backed accessor slice, leaving SQLite backend and migration proof to #88838 and later PRs. Do we have a high-confidence way to reproduce the issue? Not applicable as a bug reproduction; this is an internal refactor. The relevant behavior path is covered by gateway sessions.compaction.list/branch/restore proof and focused regression tests. Is this the best way to solve the issue? Yes, this is the best current shape for the tracker slice: it moves the checkpoint write boundary to the session accessor without flipping storage or adding runtime dual-read behavior. Leaving the direct gateway updateSessionStore mutation in place would keep the file-backed coupling the tracker is trying to remove. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against d15e89a83e61. Label changesLabel changes:
Label justifications:
Evidence reviewedPR surface: Source +131, Tests +162, Other +1. Total +294 across 6 files. View PR surface stats
What I checked:
Likely related people:
What the crustacean ranks mean
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics. How this review workflow works
|
What Problem This Solves
Path 3 is moving session and transcript state toward SQLite-backed storage, but compaction checkpoint branch/restore still had storage mutation logic coupled directly to file-backed store helpers. This PR narrows that boundary so checkpoint branch/restore owns transcript fork/copy plus session-entry metadata mutation as one domain store operation.
Why This Change Was Made
The future SQLite implementation needs to copy transcript rows and update
session_entries.entry_jsoninside one write transaction. Routing the current file-backed implementation throughCompactionCheckpointStoreand session-accessor operations preserves today's behavior while making the intended transaction boundary explicit for the SQLite-backed store.User Impact
No user-visible behavior change is intended. Existing compaction checkpoint list/get/branch/restore behavior continues to fork from the stored checkpoint boundary and persist branch/restore session metadata; the change is internal migration preparation for #88838.
Evidence
pnpm lint:tmp:session-accessor-boundarypassed.node scripts/run-vitest.mjs src/config/sessions/session-accessor.test.ts src/gateway/session-compaction-checkpoints.test.ts test/scripts/check-session-accessor-boundary.test.tspassed: 28 unit-fast tests, 64 gateway tests, 49 runtime-config tests.pnpm tsgo:test:srcpassed.upstream/main: no accepted/actionable findings./Users/phaedrus/Projects/clawdbotusing head2f40aabd3e5before the non-live rebase follow-up.upstream/mainadvanced toc70accc86f8: branch rebased to4c3d1f83ce0, localmerge-base HEAD upstream/mainequalsc70accc86f8, and localgit merge-treereports no conflicts.pnpm lint:tmp:session-accessor-boundary,node scripts/run-vitest.mjs src/config/sessions/session-accessor.test.ts src/gateway/session-compaction-checkpoints.test.ts test/scripts/check-session-accessor-boundary.test.ts,pnpm tsgo:test:src, and autoreview againstupstream/main.Real behavior proof
Behavior addressed: compaction checkpoint branch/restore storage boundary, including transcript fork/copy from a stored checkpoint boundary and session-entry metadata persistence through the accessor-backed store operation.
Real environment tested: live local OpenClaw gateway in
/Users/phaedrus/Projects/clawdbot, loopback gateway on port 18789, agentmain, synthetic temporary proof session only.Exact steps or command run after this patch:
Evidence after fix:
sessions.compaction.listreturnedok: truewith the synthetic checkpoint.sessions.compaction.branchreturnedok: true, branch keyagent:main:dashboard:a0d9bba0-926c-42ec-8114-f877bd608359, branch session id9ad7d9d2-1f06-4b36-b564-10731e7a7234, parent session keyagent:main:path3-checkpoint-proof-rebased-20260623164953-30482, andtotalTokens: 34/totalTokensFresh: true.sessions.compaction.restorereturnedok: true, restored session idfe11b495-db40-4605-a2e6-fa81010475fb, preserved one checkpoint in the restored entry, and persistedtotalTokens: 34/totalTokensFresh: true.sessions.jsonand both generated transcript files. The branch and restore transcripts containedPATH3_CHECKPOINT_SOURCE path3-checkpoint-proof-rebased-20260623164953-30482, did not containPATH3_CHECKPOINT_FUTURE path3-checkpoint-proof-rebased-20260623164953-30482, and the store entry matched the restored session id/file.path3-checkpoint-proofkeys.mainat6f2869c296f, rebuilt, restarted, and/readyzsettled toready: truewitheventLoop.degraded: false;/healthzreturned{ "ok": true, "status": "live" }.Observed result after fix: branch/restore kept the existing file-backed behavior through the real gateway surface while proving the store/accessor mutation boundary persists session-entry metadata and forks transcripts from the stored checkpoint boundary. No synthetic proof sessions or transcript files remained afterward.
What was not tested: SQLite-backed session/transcript storage is not enabled yet, so this proof covers the current file-backed store implementation and the intended transaction/accessor boundary only. Live gateway proof was not rerun after the non-live rebase to
4c3d1f83ce0because the serialized live lane is assigned to the boot-session mapping thread; exact-head proof for4c3d1f83ce0is focused local proof plus clean merge-tree. Fresh CI is running on the rebased head.