Review Checkpoint API Extraction Changes

Codex·GPT-5.5·Unknown author·3mo ago·11hr 38min·9 Checkpoints·128 file changes·+771/-743·230.3K tokens

Review the code changes introduced by commit f9000bc1a ("checkpoint: extract the persistent contract to api/checkpoint"). Provide prioritized, actionable findings.

3mo ago·4.1s

{"findings":[],"overall_correctness":"patch is correct","overall_explanation":"The extraction preserves the existing CLI-facing aliases and the git-backed implementation continues to satisfy the moved interfaces. Unit and integration checks did not reveal regressions.","overall_confidence_score":0.91}

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
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
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