Review Checkpoint Store Rename Refactoring

Codex·GPT-5.5·Soph·3mo ago·23hr 27min·12 Checkpoints·164 file changes·+3068/-2775·80.4K tokens

Review the code changes introduced by commit e8670487d ("checkpoint: rename store impl/test files to persistent/ephemeral"). Provide prioritized, actionable findings.

3mo ago·7.6s

{"findings":[],"overall_correctness":"patch is correct","overall_explanation":"The commit consists of 100% file renames with no content changes in the checkpoint package, and the package tests pass. I did not find a behavioral regression introduced by the rename.","overall_confidence_score":0.95}

a267dd8docs: update sessions-and-checkpoints for the persistent/ephemeral split Rewrite the checkpoint storage interface section for the two-store facade (Stores.Persistent / Stores.Ephemeral()), the generic Read/Write/List surface, and the Write request unions. Update the Type enum, tables, and the CLAUDE.md one-liner to the persistent/ephemeral vocabulary. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Entire-Checkpoint: 6ae608e3158b+47/-51
7c4c381checkpoint: fix stale API references in doc comments Address Copilot review on #1495: update doc comments that named the pre-union methods (EphemeralStore.WriteTemporary/WriteTemporaryTask, PersistentStore.WriteCommitted) to the current Write(request) surface, and fix a comment join mangled by an earlier rename pass (multiple// -> two lines). Comments only; no behavior change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Entire-Checkpoint: 020a6a198c75+5/-4
f9000bccheckpoint: extract the persistent contract to api/checkpoint Move the persistent-checkpoint contract — the persisted document types (Metadata, CheckpointSummary, Summary, Attribution, ...), the option types (WriteOptions/UpdateOptions/PrecomputedTranscriptBlobs), the reader/writer interfaces, the Write request union, the sentinel errors, and the CheckpointVersionBranchV1 const — into a new github.com/entireio/cli/api/checkpoint package, born with the persistent vocabulary. The contract depends only on leaf packages (agent/types, checkpoint/id, redact, go-git plumbing), so a storage backend can implement it without the CLI's agent/TUI/git machinery. The git implementation (GitStore, Open, the Stores facade, AuthorReader, and the ephemeral shadow-branch surface) stays in cmd/entire/cli/checkpoint, which imports the contract and re-exports every moved symbol via aliases (aliases.go) — all existing call sites compile unchanged. Notes: - PrecomputedTranscriptBlobs.isUsable is now exported (IsUsable); its one caller is across the new package boundary. - The Write union is sealed to api/checkpoint, so the impl-package unknown-request test was removed (no longer expressible); per-request dispatch coverage remains. - normalizeCheckpointSummary stays in the impl (read-time normalization). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Entire-Checkpoint: a808b47c0633+739/-653
3f7067ccheckpoint: align contract terminology to persistent Address Copilot review on #1504: the verbatim-moved WriteOptions/UpdateOptions doc comments still said 'committed checkpoint' (the store concept) — reword to 'persistent checkpoint' to match the rest of the api/checkpoint contract. Rename the ReadCheckpoint tests (TestReadCommittedCheckpoint* -> TestReadCheckpoint*) to match the renamed function. References to the git 'committed tree' and the TotalCommitted/total_committed attribution field are left as-is — those correctly describe git commits, not the store. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Entire-Checkpoint: 6206fb18d245+4/-4
72eee65checkpoint: name Write requests by scope (Session / Checkpoint*) Rename the Write request union to noun-based, scope-explicit names — the verb is already Write, and the tier (session vs checkpoint) should be visible in the type: WriteSession -> Session (session-level: write/replace a session) BackfillTranscript -> SessionTranscript (session-level) BackfillSummary -> SessionSummary (session-level) BackfillAttribution -> CheckpointAttribution (checkpoint-level: root combined attribution) The old BackfillAttribution name hid that it targets the checkpoint root's combined attribution across all sessions, not a single session — CheckpointAttribution makes the one checkpoint-level write explicit alongside the three session-level ones. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Entire-Checkpoint: 1dc8e6fe2d64+296/-294
3210376checkpoint: tier the read surface into CheckpointReader / SessionReader Mirror the write-side session/checkpoint split on reads: replace the mixed PersistentReader/PersistentListReader with CheckpointReader { Read, List } (checkpoint-level) SessionReader { ReadSession{Content,Metadata,Prompts,MetadataAndPrompts} } (session-level) and compose PersistentStore = CheckpointReader + SessionReader + Writer. Helpers now take the minimal tier they need (ReadCheckpoint: CheckpointReader, ReadLatestSessionContent: SessionReader); callers and stubs updated. Also drops a now-dead type assertion in readSessionMetadataForExport (SessionReader guarantees ReadSessionMetadata). No behavior change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Entire-Checkpoint: 8e1b3d190167+88/-45
a74a801checkpoint: extract the persistent contract to api/checkpoint Move the persistent-checkpoint contract — the persisted document types (Metadata, CheckpointSummary, Summary, Attribution, ...), the option types (WriteOptions/UpdateOptions/PrecomputedTranscriptBlobs), the reader/writer interfaces, the Write request union, the sentinel errors, and the CheckpointVersionBranchV1 const — into a new github.com/entireio/cli/api/checkpoint package, born with the persistent vocabulary. The contract depends only on leaf packages (agent/types, checkpoint/id, redact, go-git plumbing), so a storage backend can implement it without the CLI's agent/TUI/git machinery. The git implementation (GitStore, Open, the Stores facade, AuthorReader, and the ephemeral shadow-branch surface) stays in cmd/entire/cli/checkpoint, which imports the contract and re-exports every moved symbol via aliases (aliases.go) — all existing call sites compile unchanged. Notes: - PrecomputedTranscriptBlobs.isUsable is now exported (IsUsable); its one caller is across the new package boundary. - The Write union is sealed to api/checkpoint, so the impl-package unknown-request test was removed (no longer expressible); per-request dispatch coverage remains. - normalizeCheckpointSummary stays in the impl (read-time normalization). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Entire-Checkpoint: a808b47c0633+753/-662
221c89fcheckpoint: align contract terminology to persistent Address Copilot review on #1504: the verbatim-moved WriteOptions/UpdateOptions doc comments still said 'committed checkpoint' (the store concept) — reword to 'persistent checkpoint' to match the rest of the api/checkpoint contract. Rename the ReadCheckpoint tests (TestReadCommittedCheckpoint* -> TestReadCheckpoint*) to match the renamed function. References to the git 'committed tree' and the TotalCommitted/total_committed attribution field are left as-is — those correctly describe git commits, not the store. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Entire-Checkpoint: 6206fb18d245+4/-4
2904262checkpoint: extract shared getSessionTree for session-level reads The four SessionReader methods (ReadSessionMetadata, ReadSessionPrompts, ReadSessionMetadataAndPrompts, ReadSessionContent) each repeated the same prologue: context check, getFetchingTree, navigate to the checkpoint tree, then the session subtree, with identical ErrCheckpointNotFound wrapping. Extract that navigation into getSessionTree so each reader is just "resolve session tree, read the files it needs." Behavior is unchanged — the helper returns the same ErrCheckpointNotFound (wrapping "session N not found" when the session subtree is missing). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Entire-Checkpoint: 9cd97a095dda+24/-55
f35faa5checkpoint: name Write requests by scope (Session / Checkpoint*) Rename the Write request union to noun-based, scope-explicit names — the verb is already Write, and the tier (session vs checkpoint) should be visible in the type: WriteSession -> Session (session-level: write/replace a session) BackfillTranscript -> SessionTranscript (session-level) BackfillSummary -> SessionSummary (session-level) BackfillAttribution -> CheckpointAttribution (checkpoint-level: root combined attribution) The old BackfillAttribution name hid that it targets the checkpoint root's combined attribution across all sessions, not a single session — CheckpointAttribution makes the one checkpoint-level write explicit alongside the three session-level ones. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Entire-Checkpoint: 1dc8e6fe2d64+179/-177
5a4925dcheckpoint: tier the read surface into CheckpointReader / SessionReader Mirror the write-side session/checkpoint split on reads: replace the mixed PersistentReader/PersistentListReader with CheckpointReader { Read, List } (checkpoint-level) SessionReader { ReadSession{Content,Metadata,Prompts,MetadataAndPrompts} } (session-level) and compose PersistentStore = CheckpointReader + SessionReader + Writer. Helpers now take the minimal tier they need (ReadCheckpoint: CheckpointReader, ReadLatestSessionContent: SessionReader); callers and stubs updated. Also drops a now-dead type assertion in readSessionMetadataForExport (SessionReader guarantees ReadSessionMetadata). No behavior change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Entire-Checkpoint: 8e1b3d190167+91/-48
5b29668checkpoint: extract the persistent contract to api/checkpoint Move the persistent-checkpoint contract — the persisted document types (Metadata, CheckpointSummary, Summary, Attribution, ...), the option types (WriteOptions/UpdateOptions/PrecomputedTranscriptBlobs), the reader/writer interfaces, the Write request union, the sentinel errors, and the CheckpointVersionBranchV1 const — into a new github.com/entireio/cli/api/checkpoint package, born with the persistent vocabulary. The contract depends only on leaf packages (agent/types, checkpoint/id, redact, go-git plumbing), so a storage backend can implement it without the CLI's agent/TUI/git machinery. The git implementation (GitStore, Open, the Stores facade, AuthorReader, and the ephemeral shadow-branch surface) stays in cmd/entire/cli/checkpoint, which imports the contract and re-exports every moved symbol via aliases (aliases.go) — all existing call sites compile unchanged. Notes: - PrecomputedTranscriptBlobs.isUsable is now exported (IsUsable); its one caller is across the new package boundary. - The Write union is sealed to api/checkpoint, so the impl-package unknown-request test was removed (no longer expressible); per-request dispatch coverage remains. - normalizeCheckpointSummary stays in the impl (read-time normalization). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Entire-Checkpoint: a808b47c0633+753/-662
46a42a2checkpoint: align contract terminology to persistent Address Copilot review on #1504: the verbatim-moved WriteOptions/UpdateOptions doc comments still said 'committed checkpoint' (the store concept) — reword to 'persistent checkpoint' to match the rest of the api/checkpoint contract. Rename the ReadCheckpoint tests (TestReadCommittedCheckpoint* -> TestReadCheckpoint*) to match the renamed function. References to the git 'committed tree' and the TotalCommitted/total_committed attribution field are left as-is — those correctly describe git commits, not the store. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Entire-Checkpoint: 6206fb18d245+4/-4
089e4b6checkpoint: extract shared getSessionTree for session-level reads The four SessionReader methods (ReadSessionMetadata, ReadSessionPrompts, ReadSessionMetadataAndPrompts, ReadSessionContent) each repeated the same prologue: context check, getFetchingTree, navigate to the checkpoint tree, then the session subtree, with identical ErrCheckpointNotFound wrapping. Extract that navigation into getSessionTree so each reader is just "resolve session tree, read the files it needs." Behavior is unchanged — the helper returns the same ErrCheckpointNotFound (wrapping "session N not found" when the session subtree is missing). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Entire-Checkpoint: 9cd97a095dda+33/-64
99982c4checkpoint: name ephemeral writes by scope (Step / TaskStep) Mirror the persistent union's verb-free, scope-based naming on the ephemeral side. The store method is already Write, so the request noun should say what is written, not repeat the verb: WriteCheckpoint -> Step (a session step) and WriteTask -> TaskStep (a subagent task step). Both still produce an ephemeral shadow-branch checkpoint; the names match the strategy's SaveStep / SaveTaskStep vocabulary. Bare nouns like checkpoint.Checkpoint were rejected — they stutter on the package's own central domain noun and read as a distinct kind when both requests write the same artifact from different sources. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Entire-Checkpoint: ed0141adaf02+48/-48