remove the unreachable persistent task-checkpoint writer - #2059
Conversation
There was a problem hiding this comment.
Pull request overview
Removes the unreachable “persistent task-checkpoint writer” code path from the committed-checkpoint stores (git-branch + git-refs), and updates surrounding docs/comments/tests so committed checkpoints are described accurately as session-only records (no durable per-subagent/task subtree yet).
Changes:
- Deleted the persistent
tasks/<tool-use-id>/...write path and its associated commit trailer plumbing fromcmd/entire/cli/checkpoint/persistent.go(shared by both backends). - Simplified the persistent write contract by removing now-unused task-writer-only fields from
api/checkpoint.WriteOptions, and removed the dead test that only exercised that path. - Updated architecture docs and in-code commentary to reflect current durable-storage behavior and the rationale for skipping subagent transcript re-reads during condensation.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| docs/architecture/sessions-and-checkpoints.md | Clarifies that committed checkpoints store one record per session and do not persist tasks/ detail. |
| docs/architecture/ref-checkpoint-backend.md | Aligns ref-backend description with actual committed subtree layout and OPF limitations. |
| cmd/entire/cli/strategy/manual_commit_condensation.go | Fixes/updates condensation rationale comment (cost vs. “agents cleaned up transcripts”). |
| cmd/entire/cli/checkpoint/refs_store.go | Removes now-dead persistent task writer plumbing from the git-refs store write path. |
| cmd/entire/cli/checkpoint/persistent.go | Removes the unreachable persistent task writer and related commit-message/trailer scaffolding. |
| cmd/entire/cli/checkpoint/checkpoint_test.go | Drops the test that only covered the removed persistent task writer path. |
| api/checkpoint/metadata.go | Removes task-writer-only fields from WriteOptions and documents IsTask/ToolUseID as a currently-unused seam. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| // IsTask and ToolUseID mark a write as belonging to a subagent task rather | ||
| // than to the session as a whole. They are the write-side seam for the read | ||
| // tier below (SessionMetadata.IsTask -> CheckpointSummary.IsTask -> explain, | ||
| // checkpoint list), and no producer sets them yet: durable per-subagent | ||
| // storage is still being designed (issue #2058), so today every persistent | ||
| // checkpoint is a session checkpoint. Do not read this seam as evidence that | ||
| // subagent detail is stored — the writer that consumed these fields was | ||
| // unreachable and has been removed. |
`persistent.go` carried a full task-checkpoint writer — `tasks/<tool-use-id>/` with per-task `checkpoint.json`, incremental `NNN-<id>.json` files and a redacted subagent transcript — behind `if opts.IsTask && opts.ToolUseID != ""`. No producer in the tree ever set `IsTask`: condensation builds one `WriteOptions` per session, and neither `attach` nor `agentimport` sets it either. The `api/checkpoint` write union has no task variant at all, so it could not be expressed. Measured on this repo's v1 branch: 0 of the 150 newest commits contain any `tasks/` path. Deleting the three functions alone would have left the scaffolding that made this look like working storage, so the always-empty parts go with them: the `IsTask` guard in `applySessionWrite`, `incrementalCheckpointData`/`taskCheckpointData`, and the `taskMetadataPath` return value plus `buildCommitMessage` parameter that was always "" — so a committed checkpoint no longer carries an `Entire-Metadata-Task` trailer it never actually populated. The ephemeral formatter that does write that trailer, and the shadow branch's live `tasks/<tool-use-id>/` writer, are untouched. Also drops the `WriteOptions` fields only that writer read: `AgentID`, `CheckpointUUID`, `SubagentTranscriptPath`, and the four `Incremental*` fields. `IsTask`/`ToolUseID` stay — they feed stored metadata that live readers consume (`explain`, `checkpoint list`) — now with a comment saying no producer sets them and pointing at the design issue, so the seam is not mistaken for storage again. `TestWriteCommitted_SubagentTranscript_JSONLFallback` exercised only the dead path and goes too. Its live twin `TestWriteTemporaryTask_SubagentTranscript_ RedactsSecrets` covers the same invalid-JSONL redaction fallback on the ephemeral store, so the behaviour stays covered. Doc and comment fixes for things this made visible: - `manual_commit_condensation.go` claimed re-reading subagent transcripts "finds nothing on this path once the agent has cleaned the transcripts up". Agents do not clean them up (Claude Code keeps 180 `agent-*.jsonl`, 121 older than two days; Codex 164 rollouts, all older). Cost is the real and sufficient reason, so the false sentence is replaced by the measured fact. - `ref-checkpoint-backend.md` listed pre-push OPF among the lifecycle paths that follow the primary, contradicting its own limitations section, and described a checkpoint ref's tree as containing `tasks/…`. - `sessions-and-checkpoints.md` now states outright that a committed checkpoint holds one record per session and none per subagent. Refs #2058 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Entire-Checkpoint: 01M0DEZ22YFAGQSP9B7MVWBNTT
8dae672 to
bec02ae
Compare
|
Closing — superseded by #2032, which I didn't know about when I opened this. #2032's last ~9 commits ( Two of this PR's doc edits would also become wrong once #2032 lands: it states in What survives, split out:
|
https://entire.io/gh/entireio/cli/trails/1096
persistent.gocarried a full task-checkpoint writer —tasks/<tool-use-id>/with per-taskcheckpoint.json, incrementalNNN-<id>.jsonfiles, and a redacted subagent transcript — behindif opts.IsTask && opts.ToolUseID != "".Nothing ever reached it. No producer in the tree sets
IsTask: condensation builds oneWriteOptionsper session, and neitherattachnoragentimportsets it. Theapi/checkpointwrite union has no task variant at all, so a task write could not even be expressed. Measured on this repo'sentire/checkpoints/v1: 0 of the 150 newest commits contain anytasks/path.Deleting the three functions alone would have left the scaffolding that made this look like working storage, so the always-empty parts go with them:
IsTaskguard inapplySessionWrite, and itstaskMetadataPathreturn valuebuildCommitMessage'staskMetadataPathparameter — so a committed checkpoint no longer advertises anEntire-Metadata-Tasktrailer it never populatedincrementalCheckpointData/taskCheckpointDataWriteOptionsfields only that writer read:AgentID,CheckpointUUID,SubagentTranscriptPath, and the fourIncremental*fieldsIsTask/ToolUseIDstay: they feed stored metadata that live readers consume (explain,checkpoint list), and the ephemeral store genuinely setsIsTaskCheckpointon the pending path. They now carry a comment saying no producer sets them and pointing at #2058, so the seam isn't mistaken for storage again.Untouched: the shadow branch's live
tasks/<tool-use-id>/writer inephemeral.go, and the trailer formatter it uses.Coverage
TestWriteCommitted_SubagentTranscript_JSONLFallbackexercised only the dead path and goes with it. Its live twinTestWriteTemporaryTask_SubagentTranscript_RedactsSecretscovers the same invalid-JSONL redaction fallback on the ephemeral store, so the behaviour stays covered.Doc and comment fixes
Things this made visible:
manual_commit_condensation.goclaimed re-reading subagent transcripts "finds nothing on this path once the agent has cleaned the transcripts up." Agents do not clean them up — Claude Code keeps 180agent-*.jsonl(121 older than two days), Codex 164 rollouts (all older). Cost is the real and sufficient reason forsubagentsDir="", so the false sentence is replaced by the measured fact.ref-checkpoint-backend.mdlisted pre-push OPF among the lifecycle paths that follow the primary, contradicting its own limitations section 200 lines later; and described a checkpoint ref's tree as containingtasks/….sessions-and-checkpoints.mdnow states outright that a committed checkpoint holds one record per session and none per subagent.Verification
mise run fmt && mise run lint && mise run test:ci— 0 lint issues, no failures, 56 vogon + 4 roger-roger canary tests pass.Refs #2058. This is the cleanup half of that issue; the storage design (payload location, OPF gate) is still open there.
🤖 Generated with Claude Code