Skip to content

checkpoint: centralize store construction behind Open (Phase 0) - #1451

Merged
Soph merged 1 commit into
mainfrom
soph/pluggable-stores-phase0
Jun 18, 2026
Merged

checkpoint: centralize store construction behind Open (Phase 0)#1451
Soph merged 1 commit into
mainfrom
soph/pluggable-stores-phase0

Conversation

@Soph

@Soph Soph commented Jun 17, 2026

Copy link
Copy Markdown
Collaborator

https://entire.io/gh/entireio/cli/trails/593

Phase 0 of #1433 (pluggable checkpoint stores).

What

Replace the scattered NewGitStore(repo, ResolveCommittedRefs(ctx)) construction across cli, strategy, dispatch, and the in-package LookupSessionLog with a single seam:

checkpoint.Open(ctx, repo, OpenOptions) (*Stores, error)

This lands the final facade signature now so call sites migrate only once. Stores.Primary holds the concrete *GitStore today and the same instance backs Temporary(); later phases narrow Primary to a pluggable committed-store interface and add independent-backend mirrors without further call-site churn. The facade exposes Temporary() / Refs() / Repository() so callers no longer reach for the concrete type, and OpenOptions carries the CLI-level BlobFetcher plus explicit Settings / Refs overrides (attach keeps its injected-settings / PrimaryAsRead topology exactly).

Pure mechanical, no behavior change.

Notes / decisions

  • All 26 production NewGitStore sites migrated; the ~131 test-file sites and benchutil keep using NewGitStore, which Open now wraps.
  • getCheckpointStore returns (*GitStore, error) (propagated through its callers); the old withBlobFetcher folds into OpenOptions.BlobFetcher.
  • generateCheckpointSummary takes the facade since it needs both the committed writer and Repository(); its mirror still resolves refs from settings (ResolveCommittedRefs) to preserve exact behavior.
  • Type is checkpoint.Stores (not CheckpointStores) to avoid the revive stutter; Open's always-nil error is documented as the forward-looking facade contract.

Verification

go build ./... ✓ · mise run lint → 0 issues · checkpoint / strategy / dispatch / cli package tests ✓.

Note: the keychain-isolation gap surfaced while testing this is fixed independently in #1450.

Refs #1433

🤖 Generated with Claude Code


Note

Low Risk
Refactor-only consolidation of store construction across many files; git backend behavior is unchanged and errors are propagated consistently at new open sites.

Overview
Phase 0 introduces checkpoint.Open(ctx, repo, OpenOptions) and a Stores facade (Primary, Temporary(), Refs(), Repository()) so committed-ref resolution, optional BlobFetcher, and Settings / Refs overrides live in one place instead of repeated NewGitStore(repo, ResolveCommittedRefs(ctx)) wiring.

Production call sites across attach, explain, resume, rewind, attribution, dispatch, strategy hooks, and LookupSessionLog now open stores through this seam. Attach uses openAttachStore with explicit Refs (including PrimaryAsRead()) so injected settings and read pinning stay intact. Manual commit folds withBlobFetcher into OpenOptions and makes getCheckpointStore return (*GitStore, error); paths that need shadow-branch I/O use Stores.Temporary() rather than treating the committed store as the temp capability.

Intended as a mechanical migration with no deliberate behavior change; tests largely still construct NewGitStore directly where convenient.

Reviewed by Cursor Bugbot for commit 18b938d. Configure here.

@Soph
Soph requested a review from a team as a code owner June 17, 2026 17:32
Copilot AI review requested due to automatic review settings June 17, 2026 17:32

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Centralizes checkpoint store construction behind a new checkpoint.Open(ctx, repo, OpenOptions) facade (Phase 0 of pluggable checkpoint stores), migrating production call sites away from scattered NewGitStore(repo, ResolveCommittedRefs(ctx)) creation while preserving current behavior and keeping *GitStore as the concrete backing.

Changes:

  • Introduces cmd/entire/cli/checkpoint/open.go with OpenOptions and Stores facade (Primary, Temporary(), Refs(), Repository()).
  • Migrates strategy/CLI/dispatch paths to use checkpoint.Open(...) and threads errors where the facade will eventually become fallible.
  • Updates affected tests and function signatures (notably generateCheckpointSummary).

Reviewed changes

Copilot reviewed 19 out of 19 changed files in this pull request and generated no comments.

Show a summary per file
File Description
cmd/entire/cli/checkpoint/open.go Adds the centralized Open seam and Stores facade for committed + temporary access.
cmd/entire/cli/checkpoint/committed.go Switches in-package LookupSessionLog to construct stores via Open.
cmd/entire/cli/strategy/manual_commit.go Replaces strategy-local store construction with checkpoint.Open, returns (*GitStore, error).
cmd/entire/cli/strategy/manual_commit_git.go Updates SaveStep / SaveTaskStep to handle new getCheckpointStore error return.
cmd/entire/cli/strategy/manual_commit_condensation.go Migrates committed checkpoint reads to Open and propagates open errors.
cmd/entire/cli/strategy/manual_commit_rewind.go Migrates store construction in rewind paths (including RestoreLogsOnly) to Open.
cmd/entire/cli/strategy/manual_commit_hooks.go Uses Open in post-commit/finalize paths; uses stores.Temporary() for temporary writes and stores.Refs() for mirroring.
cmd/entire/cli/strategy/common.go Uses Open for ListCheckpoints committed listing.
cmd/entire/cli/strategy/cleanup.go Uses Open for committed checkpoint listing during orphan detection.
cmd/entire/cli/strategy/manual_commit_test.go Updates tests to handle getCheckpointStore now returning an error.
cmd/entire/cli/rewind.go Uses Open and routes committed vs temporary operations via stores.Primary / stores.Temporary().
cmd/entire/cli/review_context.go Uses Open to read committed checkpoint context (best-effort on failures).
cmd/entire/cli/resume.go Uses Open with blob fetcher injection and routes topology reads via stores.Refs().
cmd/entire/cli/head_checkpoint_flags.go Uses Open for committed summary read (best-effort on failures).
cmd/entire/cli/explain.go Uses Open for temporary explain path and summary generation; updates generateCheckpointSummary to take *checkpoint.Stores.
cmd/entire/cli/explain_test.go Updates summary-generation test to use Open/Stores facade.
cmd/entire/cli/dispatch/mode_local.go Uses Open to list committed checkpoints with repo-scoped settings context.
cmd/entire/cli/attribution.go Uses Open with blob fetcher injection for attribution resolver store creation.
cmd/entire/cli/attach.go Adds openAttachStore helper and migrates attach code paths to Open while preserving explicit ref pinning.

Replace the scattered NewGitStore(repo, ResolveCommittedRefs(ctx))
construction across cli, strategy, dispatch, and the in-package
LookupSessionLog with a single seam: checkpoint.Open(ctx, repo,
OpenOptions) (*Stores, error).

This lands the final facade signature now (issue #1433 Phase 0) so call
sites migrate only once: Stores.Primary holds the concrete *GitStore
today and the same instance backs Temporary(); later phases narrow
Primary to a pluggable committed-store interface and add
independent-backend mirrors without further call-site churn. The facade
exposes Temporary()/Refs()/Repository() so callers no longer reach for
the concrete type, and OpenOptions carries the CLI-level BlobFetcher plus
explicit Settings/Refs overrides (attach keeps its injected-settings /
PrimaryAsRead topology).

Pure mechanical, no behavior change.

Notes:
- getCheckpointStore now returns (*GitStore, error) (propagated through
  its callers); the old withBlobFetcher folds into OpenOptions.BlobFetcher.
- generateCheckpointSummary takes the facade since it needs both the
  committed writer and Repository(); its mirror still resolves refs from
  settings (ResolveCommittedRefs) to preserve exact behavior.
- Type is checkpoint.Stores (not CheckpointStores) to avoid the revive
  stutter; Open's always-nil error is the forward-looking facade contract.
- benchutil and test files keep using NewGitStore, which Open wraps.

Refs #1433

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Entire-Checkpoint: c11527631d6b
@Soph
Soph force-pushed the soph/pluggable-stores-phase0 branch from 18b938d to 0747ca9 Compare June 17, 2026 19:13
@Soph
Soph merged commit 4dc8e73 into main Jun 18, 2026
9 checks passed
@Soph
Soph deleted the soph/pluggable-stores-phase0 branch June 18, 2026 12:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants