Review GitStore Temporary Store Split

Codex·GPT-5.5·Soph·3mo ago·22hr 32min·18 Checkpoints·285 file changes·+3634/-3302·125.2K tokens

Review the code changes introduced by commit 496411d5a ("checkpoint: split temporary store out of GitStore"). Provide prioritized, actionable findings.

3mo ago·2m

{"findings":[],"overall_correctness":"patch is correct","overall_explanation":"The temporary-store split is consistently wired through Open, strategy call sites, tests, and benchmarks, and I did not find an actionable correctness regression in the changed code.","overall_confidence_score":0.88}

dee4e53checkpoint: rename InitialAttribution to Attribution There is only one attribution struct; 'Initial' was misleading because the same type also carries the combined, post-commit attribution (CombinedAttribution *Attribution). Drop the qualifier. Whole-codebase rename, no behavior change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Entire-Checkpoint: 64c76c6df888+131/-131
698e477checkpoint: collapse persistent reads to generic Read/List Rename the committed read methods to the generic surface differentiated by return type: (PersistentStore).ReadCommitted -> Read (returns *CheckpointSummary) (PersistentStore).ListCommitted -> List ReadCommittedCheckpoint (helper) -> ReadCheckpoint Sets up sto.Persistent.Read(...) once the facade is reshaped. No behavior change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Entire-Checkpoint: 88d8a34b0471+100/-100
ef0c5a0checkpoint: make Write(WriteRequest) the only public persistent write The committed write operations are now internal dispatch targets of Write(ctx, WriteRequest); callers use the request union exclusively: WriteCommitted -> writeSession (Write WriteSession{...}) UpdateCommitted -> backfillTranscript (Write BackfillTranscript{...}) UpdateSummary -> backfillSummary (Write BackfillSummary{...}) UpdateCheckpointSummary -> backfillAttribution (Write BackfillAttribution{...}) Production already wrote through the union; this migrates the test callers and unexports the dispatch targets, so 'Committed' is gone from the persistent write surface and Write is the single entry point. No behavior change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Entire-Checkpoint: 608f3d762503+138/-137
5710439checkpoint: rename temporary types to ephemeral vocabulary Type-level rename of the shadow-branch surface (method names and the generic Read/Write collapse follow): TemporaryStore -> EphemeralStore WriteTemporaryOptions -> WriteEphemeralOptions WriteTemporaryResult -> WriteEphemeralResult WriteTemporaryTaskOptions -> WriteEphemeralTaskOptions ReadTemporaryResult -> ReadEphemeralResult TemporaryInfo -> EphemeralInfo TemporaryCheckpointInfo -> EphemeralCheckpointInfo Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Entire-Checkpoint: 7c586656fb39+96/-96
ae36a26checkpoint: collapse ephemeral reads/lists to generic Read/List Mirror the persistent generic surface on the shadow-branch store: ReadTemporary -> Read (added to the EphemeralStore interface) ListTemporary -> List ListTemporaryCheckpoints -> ListCheckpoints ListAllTemporaryCheckpoints -> ListAllCheckpoints Write methods become a Write request union in the next commit. No behavior change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Entire-Checkpoint: 59955a4431e2+19/-18
3387cbbcheckpoint: unify ephemeral writes behind Write(EphemeralWriteRequest) Mirror the persistent Write union on the shadow-branch store: a single ephemeralStore.Write(ctx, EphemeralWriteRequest) dispatches a sealed request union, replacing the two exported write methods. WriteTemporary -> writeCheckpoint (Write WriteCheckpoint{...}) WriteTemporaryTask -> writeTask (Write WriteTask{...}) Write returns WriteEphemeralResult{CommitHash, Skipped} for both requests. The EphemeralStore interface now exposes Read/Write/List, symmetric with the persistent surface. Callers (strategy, benchmarks, tests) migrated. No behavior change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Entire-Checkpoint: 63e8dc2bdb3d+81/-44
7b1005dcheckpoint: reshape facade to Stores.Persistent / Stores.Ephemeral() The Open facade now exposes the two stores under lifecycle-named accessors: Stores.Primary -> Stores.Persistent (field) Stores.Temporary() -> Stores.Ephemeral() (method) and the strategy helpers follow: getCheckpointStore -> getPersistentStore getTemporaryStore -> getEphemeralStore Callers read sto.Persistent.Read(...) / sto.Ephemeral().Write(...), the symmetric surface from the review feedback. No behavior change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Entire-Checkpoint: 67c4161a54a9+49/-49
e867048checkpoint: rename store impl/test files to persistent/ephemeral Pure file renames (no content change) to match the vocabulary: committed.go -> persistent.go committed_write.go -> persistent_write.go committed_reader_resolve.go -> persistent_reader.go temporary.go -> ephemeral.go committed_*_test.go / temporary_test.go -> persistent_*/ephemeral_test.go committed_refs.go is left as-is pending the CommittedRefs type rename. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Entire-Checkpoint: 29637d428ff3
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