Checkpoint Backend Registry and Settings-Driven Selection · Entire

Home

Log in

can you take a look at https://github.com/entireio/cli/issues/1433#issuecomment-4803079531 and can you help me plan phase 2?

3w ago·25m·14 calls

Locked in: leaner {type, config} schema, drop v1.1, include a reference backend, plan stays here. Here's the finalized Phase 2.

Phase 2 — topology + registry (final plan)

A 4-PR stack. Each PR is independently shippable; the default config (git primary, no mirrors) is byte-identical to today at every step.

PR 2a — Registry + factory (no behavior change)

Introduce a backend registry in cmd/entire/cli/checkpoint (the git deps live here, so the registry can't sit in api/checkpoint).

1
2
3
4
5
6
7
8
9
10

// cmd/entire/cli/checkpoint/registry.go
type OpenEnv struct {
    Repo        *git.Repository // git-specific; non-git backends ignore it
    BlobFetcher BlobFetchFunc
    Refs        PersistentRefs
}
type Factory func(ctx context.Context, env OpenEnv, cfg json.RawMessage) (cpkg.PersistentStore, error)

func Register(typ string, f Factory)               // package-init registration
func build(ctx, env, BackendConfig) (PersistentStore, error)

PR 2b — Selection settings (leaner shape)

1
2
3
4
5
6
7
8
9
10

// settings package
type CheckpointsConfig struct {
    Primary BackendConfig   `json:"primary,omitempty"`
    Mirrors []BackendConfig `json:"mirrors,omitempty"`
}
type BackendConfig struct {
    Type   string          `json:"type"`             // "git", "fs", ...
    Config json.RawMessage `json:"config,omitempty"` // backend-specific
}
// EntireSettings.Checkpoints *CheckpointsConfig `json:"checkpoints,omitempty"`

PR 2c — Mirror fan-out topology

A fanoutStore implementing PersistentStore so none of the 5 write call sites change:

1
2
3
4
5
6
7

type fanoutStore struct {
    primary cpkg.PersistentStore
    mirrors []cpkg.Writer
}
// all reads (Read/List/ReadSession*) → primary only  (spec: primary serves all reads)
// Write: primary.Write(...) first; on success, fan out to each mirror best-effort,
//        logging.Warn on mirror failure, never returning it.

PR 2e — Reference backend (filesystem/JSON) + seam test

A real second implementation to exercise the registry and fan-out end-to-end.

Notably out of scope (and why)

Sequencing note

2a → 2b → 2c are a strict chain (each adds an inert layer). 2e can land after 2c, or in parallel against 2a's registry if you want to prove the factory shape early. The whole stack keeps git-primary/no-mirrors as the untouched default throughout.

Want me to start on PR 2a, or refine any of the type shapes (e.g. the Factory/OpenEnv signature or the fsstore on-disk layout) before we kick off?

äcan you give this to codex and let it review?

3w ago·36m·14 calls

Codex reviewed the plan against the real code (read-only, in the repo) and found several genuine gaps — mostly things the "5 write sites / 21 Open sites" framing missed. Here's the distilled review with my take on each.

Codex's findings (verified against code)

🔴 High — writes do not all funnel through PersistentStore.Write. Two paths mutate refs.Primary directly, so a mirror fan-out wrapper would silently miss them:

This is the most important finding. It means "mirror = best-effort write fan-out" is only safe if we explicitly accept that mirrors can hold un-OPF'd content and stale (un-deleted) checkpoints — or we extend fan-out to cover delete + OPF. Worth an explicit decision.

🔴 High — tokens profile is a 22nd construction site Phase 0 missed. It bypasses checkpoint.Open and is typed directly to *checkpoint.GitStore (tokens_profile.go:116,135,195), so a configured primary would be ignored. Needs to go through Open first.

🔴 High — non-git primary breaks far more than rewind. Attach (attach.go:468), resume (resume.go:423), push (manual_commit_push.go:46), doctor, OPF, and cleanup all assume a git refs.Primary. So my "one-line guard to disable rewind if fs is primary" is wrong — Phase 2 should reject a non-git primary outright (fs is mirror-only) rather than pretend it's a supported primary.

🟡 Medium:

It confirmed the rest of the grounding (contract shape, Stores facade, v1.1 absence, DTO plumbing.Hash leak) and agreed with the wrapper approach, primary-first ordering, dropping v1.1, and fsstore being a meaningful seam test (with: cover all fourWriteRequest variants, not just Session).

How I'd fold this into the plan

Two things genuinely need your call before I revise the written plan:

  1. Cleanup/OPF coverage — accept mirrors-can-be-stale-and-un-OPF'd (option a, tight scope) vs. extend fan-out to delete + pre-push OPF now (option b, privacy-safe but bigger).
  2. fsstore data sensitivity — does the reference backend ever hold real transcript content (which makes the OPF gap matter), or is it purely a structural seam test?

Want me to update the plan doc with these folded in, and/or send a follow-up round to Codex on the cleanup/OPF decision?

what is OPF again?

3w ago·17.2s

OPF = OpenAI Privacy Filter.

It's an extra redaction layer for checkpoint content, controlled by the redaction.openai_privacy_filter.enabled setting. The key facts (from this repo's architecture):

Why it matters for your Phase 2 plan: Codex flagged that OPF rewrites the v1 ref directly at pre-push, outside PersistentStore.Write. So if you fan out writes to a mirror at checkpoint time, that mirror gets the 7-layer content but never the OPF re-redaction — the 8th privacy layer that's supposed to apply before content leaves the box. If a mirror is a remote/independent backend (S3, gmeta), you'd be shipping less-redacted data to the mirror than to the primary remote. That's the privacy angle behind my question #2 (does the reference backend ever hold real transcript content) and the cleanup/OPF decision.

the reference backend should only be an example and if at all used in tests only (would even guard against anyone being able to enable it)

3w ago·32.6s

That simplifies things nicely and resolves the OPF concern for Phase 2.

If the reference backend is test-only and impossible to enable in production, then the cleanest guard is: production code never registers it. The fsstore package exposes its factory, but only test setup wires it in (e.g. fsstore.RegisterForTesting(checkpoint.Register) called from test init/helpers). The shipped binary's registry only knows "git", so a config with type: "fs" in production fails with a plain unknown checkpoint backend type "fs" — there's no flag, env var, or setting that turns it on. That's a stronger guard than a runtime testing.Testing() check, because the code path literally doesn't exist in the production binary.

How this folds into the revised plan:

So the open decisions are now settled:

  1. Cleanup/OPF → option (a), documented as a constraint, safe because mirrors hold no real data in Phase 2.
  2. fsstore data sensitivity → never holds real data; test-only, unregisterable in production.

The still-real Codex findings that carry into implementation regardless: route tokens profile through Open, reject non-git primary, forward optional interfaces (AuthorReader) through the fan-out wrapper, make Open's settings-read fail-soft, and reject/guard a git-typed mirror that would double-write the same ref.

Want me to update the plan doc (phase2-plan.md) with all of this folded in — and optionally send that revised version back to Codex for a second pass?

yes, do the second pass

3w ago·9m·7 calls

Done — the plan is now at revision 2 (final) in phase2-plan.md, with both remaining Codex items folded in. Where it landed:

Final Phase 2 PR stack:

Settled decisions: lean schema; drop v1.1; test-only reference backend; mirrors are best-effort write-only (no delete/OPF fan-out — safe because no real data reaches a mirror in Phase 2); sync is Phase 3.

Codex's two-pass verdict: all prior findings addressed; the only thing that was blocking "implementable as-is" — Open using strict settings loading — is now specified as a lenient checkpoint-only loader. Phase 2 tests must assert write fan-out only (never mirror deletion or OPF parity).

Both review transcripts are saved (codex-review.txt, codex-review2.txt) alongside the plan.

Next steps, your call:

Want me to do any of those?

let's start implementing, do meaningful commits and run codex as a review, when done run simplify and codex review again

3w ago·9h 13m·131 calls

Now /simplify on the diff.

082ce88Route tokens profile through checkpoint.Open\ \ tokens profile was the one remaining production CLI path constructing a\ checkpoint store directly (checkpoint.NewGitStore) instead of going through\ checkpoint.Open, so a future settings-driven primary backend would be\ silently ignored there. Route it through Open and widen the report helpers\ from *checkpoint.GitStore to the checkpoint.PersistentStore interface.\ \ benchutil keeps its direct NewGitStore (documented benchmark fixture that\ pins the v1 topology and never exercises backend selection).\ \ No behavior change: Open with default options resolves the same v1 git store.\ \ Co-Authored-By: Claude Opus 4.8 (1M context) noreply@anthropic.com\ Entire-Checkpoint: 3bbb48203da7+12/-6

82ccc32Add checkpoint backend registry and factory\ \ Introduce a backend registry in the checkpoint package: a Factory type, an\ OpenEnv construction context, Register/build, and a built-in "git" backend.\ Open now builds its primary store through the registry (defaulting to the git\ backend) rather than calling NewGitStore directly.\ \ This is the seam Phase 2 needs: a new persistent backend becomes a factory\ registered under a type name. Production registers only the git backend;\ test-only backends will register through their own RegisterForTesting helpers\ so a production binary can never select them. The registry default of git\ matches today's Open exactly, so there is no behavior change at default config.\ \ Co-Authored-By: Claude Opus 4.8 (1M context) noreply@anthropic.com\ Entire-Checkpoint: 56c7fecd2a55+139/-7

c7b6a00Add checkpoints backend selection settings and mirror fan-out\ \ Wire the registry to settings-driven backend selection and add independent\ backend mirroring.\ \ Settings: a new `checkpoints.{primary, mirrors}` block using a discriminated\ {type, config} shape, read through a dedicated lenient loader\ (settings.LoadCheckpointsConfig). The loader is fail-soft by design — a missing\ file, whole-file JSON syntax error, or unrelated invalid settings all resolve\ to "no config" so checkpoint construction defaults to git; it errors only when\ a present `checkpoints` block is itself invalid. This avoids making Open newly\ fail on unrelated malformed settings (the strict settings.Load path still\ surfaces those for normal commands). The field also lives on EntireSettings so\ the strict loader accepts a `checkpoints` key.\ \ Open: builds the primary and mirrors through the registry. The primary must be\ git (attach/resume/push/doctor/cleanup/OPF all assume a git refs.Primary), so a\ non-git primary is rejected. A git-typed mirror is rejected too: it would share\ the primary ref topology and double-write the same ref.\ \ fanoutStore: serves all reads from the primary and writes to the primary first,\ then fans out best-effort to each mirror (failures logged, never surfaced).\ With no mirrors it returns the primary unwrapped, preserving its concrete type\ and optional capabilities. When wrapping, it preserves the optional AuthorReader\ iff the primary implements it, so explain's author fallback keeps working.\ \ Mirrors are write-only and may lag the primary (no cleanup-delete or pre-push\ OPF fan-out); they must not become a read/sync source without reconciliation.\ \ Co-Authored-By: Claude Opus 4.8 (1M context) noreply@anthropic.com\ Entire-Checkpoint: c9cefdc270e2+690/-4

8b512abAdd fsstore: test-only reference checkpoint backend\ \ Add a filesystem/JSON reference backend that implements the full\ api/checkpoint contract, plus a seam integration test that drives a git\ primary with the fsstore as a configured mirror through checkpoint.Open and\ asserts all four WriteRequest variants land in both backends while reads\ resolve from the git primary.\ \ fsstore is registered only via RegisterForTesting (idempotent), never by\ production code, so a production binary cannot select the "fs" type. It\ intentionally ignores the contract's git-specific blob-hash fields and stores\ transcript bytes directly — keeping the example small and making the\ contract's remaining git leakage concrete. It serves as both the seam's test\ exercise and a worked example for implementing a new backend.\ \ Co-Authored-By: Claude Opus 4.8 (1M context) noreply@anthropic.com\ Entire-Checkpoint: 5b6ab3e1bdb2+686

067d26fAddress first review pass on Phase 2 checkpoint stores\ \ Fixes from the implementation review:\ - Open no longer newly fails on a non-ENOENT settings read error: the lenient\ checkpoints loader treats an unreadable settings file (bad perms, settings.json\ is a directory, etc.) as fail-soft too, defaulting to the git backend and\ logging at debug. Preserves "no behavior change at default config" even on a\ broken settings setup.\ - Document that Open resolves backend config from the context/cwd (like\ settings.Load), not from the repo argument, so callers opening a non-cwd repo\ wrap the context.\ - fsstore fidelity: default a zero CreatedAt to now (per contract), carry\ transcript offsets, prompt attributions, review/investigation fields, the\ HasReview/HasInvestigation summary flags, and migration-path CombinedAttribution;\ document that cross-session token aggregation is intentionally omitted.\ - fsstore writes atomically (temp + rename) so a reader never sees partial JSON.\ \ Co-Authored-By: Claude Opus 4.8 (1M context) noreply@anthropic.com\ Entire-Checkpoint: ca5542401c18+123/-28

Review target: main...HEAD

/simplify → 4 cleanup agents in parallel → apply the fixes

You are improving the quality of the changed code, not hunting for bugs. Review it for reuse, simplification, efficiency, and altitude issues, then fix what you find. Do not look for correctness bugs — that is what /code-review is for.

Phase 0 — Gather the diff

Run git diff @{upstream}...HEAD (or git diff main...HEAD / git diff HEAD~1 if there's no upstream) to get the unified diff under review. If there are uncommitted changes, or the range diff is empty, also run git diff HEAD and include the working-tree changes in scope — the review often runs before the commit. If a PR number, branch name, or file path was passed as an argument, review that target instead. Treat this diff as the review scope.

Phase 1 — Review (4 cleanup agents in parallel)

Launch 4 independent review agents via the Agent tool, all in a single message so they run concurrently. Pass each agent the diff and one of the four angles below. Each returns its findings with file, line, a one-line summary, and the concrete cost (what is duplicated, wasted, or harder to maintain).

Reuse

Flag new code that re-implements something the codebase already has — Grep shared/utility modules and files adjacent to the change, and name the existing helper to call instead.

Simplification

Flag unnecessary complexity the diff adds: redundant or derivable state, copy-paste with slight variation, deep nesting, dead code left behind. Name the simpler form that does the same job.

Efficiency

Flag wasted work the diff introduces: redundant computation or repeated I/O, independent operations run sequentially, blocking work added to startup or hot paths. Also flag long-lived objects built from closures or captured environments — they keep the entire enclosing scope alive for the object's lifetime (a memory leak when that scope holds large values); prefer a class/struct that copies only the fields it needs. Name the cheaper alternative.

Altitude

Check that each change is implemented at the right depth, not as a fragile bandaid. Special cases layered on shared infrastructure are a sign the fix isn't deep enough — prefer generalizing the underlying mechanism over adding special cases.

Phase 2 — Apply the fixes

Wait for all four agents to complete, dedup findings that point at the same line or mechanism, and fix each remaining one directly. Skip any finding whose fix would change intended behavior, require changes well outside the reviewed diff, or that you judge to be a false positive — note the skip rather than arguing with it. Finish with a brief summary of what was fixed and what was skipped (or confirm the code was already clean).

3w ago·32m·42 calls

Done. Phase 2 is implemented, reviewed by Codex twice, simplified, and the full CI suite (unit + integration + e2e canary) is green.

What landed

Branch phase2-pluggable-checkpoint-stores, 7 commits off main:

  1. Route tokens profile through checkpoint.Open — closed the one production store-construction site Phase 0 missed; exempted benchutil.
  2. Backend registry + factory — Register/build/OpenEnv/Factory; built-in git backend; Open resolves the primary through the registry (default = git, identical behavior).
  3. Selection settings + mirror fan-out — checkpoints.{primary,mirrors}``{type,config} schema with a fail-soft checkpoint-only loader; rejects non-git primary and git mirrors; fanoutStore (reads→primary, best-effort mirror writes, AuthorReader preserved, zero-mirror→raw store).
  4. fsstore reference backend (test-only) — full contract impl over JSON files, RegisterForTesting-only, plus a seam integration test driving git-primary + fs-mirror through Open across all four WriteRequest variants.
  5. First-review fixes — Open fail-soft on all settings read errors; fsstore CreatedAt default, extra metadata fields, atomic writes; doc on cwd-context resolution.
  6. /simplify — exported RedactedJoinedPrompts; extracted shared settings-path helpers (removed triplication).
  7. Final-review fix — fsstore redacts summaries via the shared RedactSummary.

Review trajectory

Still open (future phases, unchanged)

Nothing is pushed and no PR is opened — the stack is local on the branch for you to review. Want me to push it / open PRs (one per commit, or one for the whole stack), or update the tracking issue #1433?

54f7f52Simplify: reuse prompt-join and settings-path helpers\ \ Quality pass (/simplify) findings:\ - Export checkpoint.RedactedJoinedPrompts (was the private redactedJoinedPrompts)\ and call it from fsstore instead of inlining join+redact twice, so alternate\ backends produce identically-redacted prompt blobs through the canonical path.\ - Extract settingsAbsPaths/worktreeSettingsPaths helpers in the settings package\ and use them from Load, loadForWorktreeRoot, and checkpointsSettingsPaths,\ removing the triplicated base+local path resolution.\ \ No behavior change; full unit suite green.\ \ Co-Authored-By: Claude Opus 4.8 (1M context) noreply@anthropic.com\ Entire-Checkpoint: 52172b38461a+33/-31

64ef4e7Redact summaries in fsstore via shared checkpoint.RedactSummary\ \ Final review caveat: the git store redacts checkpoint summaries before\ storing them, but fsstore stored opts.Summary / SessionSummary directly. Export\ checkpoint.RedactSummary (was the private redactSummary) and use it in fsstore's\ session write and summary write so the reference backend models the same\ redaction the git store performs. Broaden the package doc to note the remaining\ intentional gaps (cross-session token aggregation, derived CLI/skill-events\ version stamps).\ \ Co-Authored-By: Claude Opus 4.8 (1M context) noreply@anthropic.com\ Entire-Checkpoint: 4917f3fb8f7f+17/-15

can you open a PR first so I can have a look?

3w ago·1m·2 calls

PR opened: https://github.com/entireio/cli/pull/1533 — "Phase 2: pluggable checkpoint stores (registry + topology)", base main, 7 commits.

The body walks through each commit, links it to #1433, calls out the two non-blocking Codex caveats, and lists what's explicitly deferred to Phase 3. The branch is pushed and CI should kick off on the PR.

A couple of things worth noting now that it's up:

Want me to update issue #1433, or leave it until you've looked?

can you look at the PR comments

3w ago·4m·11 calls

I reviewed the PR comments (#1533) and addressed both bot findings. Pushed as a new commit.

What the bots flagged

Cursor Bugbot (Medium) — a real bug I'd introduced.LoadCheckpointsConfig read settings.json first and returned early if the basecheckpoints block failed to decode — so a valid block in settings.local.json could never override a broken base, contradicting the documented "local replaces base wholesale" rule. Fixed: the loader now extracts the raw block from local-then-base and decodes/validates only the block that wins, so a malformed block in the overridden file is irrelevant. Added a regression test (valid local overrides invalid base).

Copilot (Low).fsstore.save left the temp file behind if rename failed, and pointed at the existing jsonutil.WriteFileAtomic. Fixed by reusing it — unique temp name, fsync, cleanup-on-failure, parent-dir fsync. Also a reuse win (drops my hand-rolled temp+rename) and incidentally hardens the cross-instance collision Codex had noted, since the helper uses randomized temp names.

The other entries were the bots' PR-overview summaries, nothing else actionable.

fmt + lint (0 issues) + the checkpoint/settings suites are green, and it's pushed — the PR will re-run CI and the bots will re-review the new commit.

Note: the Cursor "fix in Cursor/Web" links in those comments are just the bot's autofix CTAs — I fixed it directly instead, so you can resolve those threads. Want me to reply on the PR threads noting they're addressed (with the commit SHA), or leave them for you?

1136bb3Address PR bot review on Phase 2 checkpoint stores\ - Fix local-settings checkpoints override (Cursor Bugbot, medium): the loader\ returned early if the BASE checkpoints block failed to decode, so a valid\ block in settings.local.json never got to override it — contradicting the\ documented "local replaces base wholesale" semantics. Reworked to extract the\ raw block from local-then-base and decode/validate only the block that wins,\ so a malformed block in the overridden file never blocks the winner. Added a\ regression test (valid local overrides invalid base).\ - Reuse jsonutil.WriteFileAtomic in fsstore.save instead of the hand-rolled\ temp+rename (Copilot): it uses a unique temp name, fsyncs, and cleans up the\ temp file on failure rather than leaving it behind.\ \ Co-Authored-By: Claude Opus 4.8 (1M context) noreply@anthropic.com\ Entire-Checkpoint: 6ce520beb1cc+51/-44

can you help me understand why we claim "primary must be git" ?

3w ago·1m·2 calls

Good instinct to push on it — "must be git" is a strong claim, so here's the actual basis, grounded in the current code.

The short version

The PersistentStore interface (Read/List/ReadSession*/Write) is genuinely backend-agnostic. But a lot of the surrounding lifecycle code doesn't go through that interface — it reaches around the store and operates directly on git refs via stores.Refs(), which returns PersistentRefs{Primary, Read, Push}, all of which are plumbing.ReferenceName (git refs). Those paths treat "the committed record" as "a git branch + tree in this repo," not as "whatever the primary store says." A non-git primary would leave those paths reading and advancing a git ref that the primary never populates — incoherent, not merely unimplemented.

The concrete coupling (all outside the store abstraction)

From the grep, the source-of-truth-is-a-git-ref assumption is baked into:

None of these would function against, say, an S3 primary — there's no git ref for them to fetch, advance, reconcile, or diff.

Why a non-git mirror is fine but a primary isn't

It's an asymmetry, not arbitrary: mirrors are write-only fan-out. Nothing reads, bootstraps, reconciles, or rewinds a mirror — fanoutStore only ever calls mirror.Write. So a mirror never touches any of the git-ref code paths above. The primary is the source of truth for all of them, which is exactly why it has to be the backend those paths understand.

Is it fundamental, or a Phase-2 boundary?

It's a Phase-2 boundary, and that's the honest framing. Non-git primary isn't impossible in principle — it's that the lifecycle (sync/bootstrap/repair/rewind/OPF) hasn't moved behind the store yet. That relocation is Phase 3 ("move push/fetch — and doctor's mirror repair — into the store"). Once those operations live behind a Syncable/admin capability instead of reaching for refs.Primary, the git assumptions get encapsulated and a non-git primary becomes feasible.

Given that, rejecting it explicitly at Open is the deliberately safe choice: Codex's first review flagged that the earlier "just disable rewind on non-git primary" guard was too narrow precisely because so many other command families assume git refs. A clear "only git may be the primary" error up front beats a config that half-works and then fails deep inside resume/doctor/OPF.

So the claim is accurate but worth stating precisely: the checkpoint document store is pluggable today; the checkpoint lifecycle is not — and the lifecycle is what pins the primary to git until Phase 3. If you'd like, I can tighten the comment in buildPrimary to say exactly that (lifecycle coupling + "revisit when Phase 3 moves sync into the store") rather than just listing command names.

hmm ok, "git backend" is I think the issue here, the current backend is "git-branch" backend for me more and not "the git backend" and we will have "git-refs" too... Maybe we should reframe that?

3w ago·4m·1 call

What would you like to clarify? Happy to dig into any of it before we settle on scope — e.g. how the "git-refs" backend would actually store things, what "primary-capable / git-ref-backed" should precisely mean as a capability, whether the rename should ripple to the Go types, or how this interacts with Phase 3 moving the lifecycle behind the store. What's on your mind?

"primary must be a git-ref-backed backend (e.g. git-branch)".

This makes no sense, or is confusing, is "git-backed" more correct?

3w ago·38.0s

Yes — "git-backed" is the right term. "git-ref-backed backend" both over-specifies and stutters ("...-backed backend").

The distinction that actually matters is where the committed record lives, not how it's addressed within git:

"ref-backed" was me leaking an implementation detail (the data happens to be anchored by a ref) into the concept. Every git-backed backend anchors its data with a ref anyway, so that detail buys nothing in the constraint and just makes it clunky.

So the framing becomes:

Cleaner error:

1
2

return fmt.Errorf("checkpoints.primary.type %q cannot be the primary: "+
    "only git-backed backends (e.g. %q) may be primary", typ, BackendTypeGitBranch)

Does "git-backed" as the family term (with git-branch/git-refs as the variants, and GitBacked as the primary-eligibility capability) match how you're thinking about it? If so I'll fold that vocabulary into the scope options rather than the "git-ref-backed" wording I had.

ok, but don't mention "git-notes" anywhere I don't think we ever do that, otherwise continue

3w ago·7m·15 calls

What would you like to clarify? Happy to go deeper on any of it — e.g. whether a git-backed mirror could ever be legitimate (different branch/remote/repo) and how per-mirror ref config would change this, what actually happens to v1 history without the block, how this should interact with Phase 3 once sync moves into the store, or whether the constraint belongs in buildMirrors at all vs. somewhere else. What's on your mind?

the thing is that we probably roll out refs to branch in parallel and with the current block we couldn't right?

3w ago·2m

That's the decisive point — you're right, and it means my guard is wrong, not just mis-worded.

The parallel rollout you're describing (dual-write to the v1 branch and the new refs scheme during a transition) is a git-backed primary + git-backed mirror — e.g. primary: git-branch, mirrors: [git-refs]. The fan-out is exactly the mechanism for it. A blanket "git-backed can't be a mirror" block would forbid the migration path.

Where I went wrong: I generalized "reject git mirror" into "reject any gitBacked mirror," but the real hazard was never git-backedness — it's two backends writing the same refs. And that only happens with two same-type git-backed backends, because they share env.Refs and the one ref namespace. A git-refs mirror under a git-branch primary writes refs/entire/checkpoints/*, not the v1 branch — no collision. So:

So the correct guard is "reject a git-backed mirror only when it's the same backend type as the primary," not "reject all git-backed mirrors." Today, with git-branch as the only git-backed type, that produces identical behavior to the current block — but it's the forward-compatible predicate that lets git-refs slot in as a mirror the moment it exists, with zero further change to buildMirrors.

Concretely the check becomes b.gitBacked && m.Type == primaryType (instead of just b.gitBacked), with a message like "a git-backed mirror must be a different git-backed backend than the primary, so they write distinct refs." It's a conservative proxy — "same type" stands in for "same resolved refs" until per-mirror ref config exists, at which point it relaxes further.

That keeps the real foot-gun guarded (two git-branch entries silently doubling v1 history) while unblocking the branch↔refs parallel rollout.

Want me to apply that narrowing? Or, if you'd rather not carry even the same-type guard, I can drop the mirror-side check entirely and rely on the rollout configs simply not being silly — but I'd lean toward keeping the narrowed version since it's cheap and the failure mode (doubled history) is annoying to debug.

yes, block the same backend, and even there we could do some loosening in the future (like if the same backend is used but both write to different repos it's fine too) but let's just say we can have only one of each type

3w ago·4m·7 calls

Done — pushed. Here's what changed and the reasoning, now that you've landed the design.

What I implemented

Taxonomy: the registered type "git" → "git-branch" (it stores the committed record on the entire/checkpoints/v1 branch), leaving the git-refs name free for the same git-backed family.

Two clean axes instead of string-matching:

  1. gitBacked capability — marks backends whose record lives in this repo's git object store. Only git-backed backends may be the primary (the lifecycle drives the primary through the repo + refs). buildPrimary checks the capability, not == "git".
  2. One-of-each-type topology rule — a backend type may appear at most once across primary + mirrors. This is the conservative form of "no two backends may write the same target": today same-type ⇒ same refs ⇒ collision, so a git-branch mirror under a git-branch primary (double-writing v1) is blocked — but a different git-backed backend like git-refs mirroring alongside a git-branch primary is allowed. That's exactly your branch↔refs parallel-rollout topology, which the old "no git-backed mirror" block would have killed.

The comment explicitly notes the future loosening you mentioned (same type pointed at distinct repos/refs via per-mirror config).

Why this is the right shape for the rollout

During the transition you'd configure primary: git-branch, mirrors: [git-refs] (or the reverse). Fan-out dual-writes both schemes; reads stay on the primary; later you flip the primary. Different types → distinct ref namespaces → no collision → allowed. When git-refs lands it's just Register(...) + a registry entry; buildPrimary/buildMirrors don't change.

Verification

Tests updated (RejectsMirrorOfPrimaryType, RejectsDuplicateMirrorType, RejectsNonGitBackedPrimary, RejectsUnknownPrimary; seam + config tests now use git-branch). fmt + lint (0 issues) + full unit suite (6859 tests) green. Pushed to PR #1533, so CI and the bots will re-run.

One thing worth flagging for the PR description: I should update the PR body, which still says the constraint is "primary must be git" and "git mirror rejected" — it's now "git-backed primary" + "one of each type". Want me to update the PR description to match, and/or reply to the Cursor/Copilot threads noting they're resolved?

d0e4056Reframe backend taxonomy: git-branch type + one-of-each-type mirrors\ \ The registered type "git" conflated "uses git" with "stores on a git branch".\ Rename it to "git-branch" (the current backend stores the committed record on\ the entire/checkpoints/v1 branch); this leaves room for a future git-refs\ backend in the same git-backed family.\ \ Replace the string-equality constraints with a declared capability and a\ topology rule:\ - registeredBackend.gitBacked marks backends whose committed record lives in\ this repo's git object store. Only git-backed backends may be the primary\ (the lifecycle — resume/doctor/explain/push/cleanup/OPF — drives the primary\ through the repo and its refs). buildPrimary checks the capability instead of\ comparing against the literal "git".\ - A backend type may appear at most once across the topology (primary +\ mirrors). This is the conservative form of "no two backends may write the\ same target": two backends of the same type share the same refs/storage\ today, so a git-branch mirror under a git-branch primary would double-write\ v1. It deliberately permits a different git-backed backend (e.g. a future\ git-refs) to mirror alongside a git-branch primary — the branch<->refs\ parallel-rollout topology — which the previous "no git-backed mirror" block\ would have forbidden.\ \ A future per-mirror config (same type, distinct repo/refs) could relax the\ one-of-each-type rule further.\ \ Co-Authored-By: Claude Opus 4.8 (1M context) noreply@anthropic.com\ Entire-Checkpoint: b6014c28999a+155/-65

can you update the PR description and the trail description too? (entire trail)

3w ago·3m·6 calls

Both updated.

Both now drop the stale "primary must be git / git mirrors rejected" framing.

Still open if you want them: replying to the Cursor/Copilot PR threads to mark them resolved (with the fix commit SHAs), and ticking the Phase 2 box / linking the PR on issue #1433. Want either of those?

yeah close/resolve comments, and link pr to 1433

3w ago·2h 25m·6 calls

Done — everything's wired up.

Review threads (PR #1533): both resolved, each with a reply documenting the fix commit 1136bb3474:

Linked to #1433:

That closes out the review-comment and tracking-link tasks. The PR is up to date (description rewritten earlier), threads are clean, and #1433 now points at it. Anything else you want before you hand it off for human review — e.g. mark the trail status, or leave it as building?

feedback on 1533, can you take a look

RegisterForTesting is public on a "production" package. It may be worth using a _test instead. The use of DisallowUnknownFields results on the CLI not being forward compatible. We just will need to be extra careful everytime we rollout new fields. On the error, I'd explicitly call out the user should confirm they are using the latest version of the CLI. os.ReadFile is problematic, even with the checks we are doing, this then becomes vulnerable to TOCTOU, the use of os.Root would be ideal.

3w ago·18m·22 calls

Pushed (the "checkpoints saved locally but not synced" line is just the checkpoint-remote warning — the git push itself succeeded). All three review points addressed:

1. RegisterForTesting public on a production package → fixed structurally. The factory, its config struct, the "fs" type name, and the registration helper all moved into register_test.go (now unexported registerForTesting). The production fsstore package surface is just Store — there's no symbol a production binary could call to register the backend. (Confirmed nothing outside tests imports fsstore, so this is purely tightening, no behavior change.)

2. DisallowUnknownFields forward-compat. Kept it per your call (it catches typos like primry instead of silently dropping them), but documented the cost: it's not forward-compatible, so adding a field is a coordinated rollout (ship the reader before any writer emits it). The decode error now ends with "an unrecognized field can also mean this file was written by a newer CLI — confirm you are on the latest version."

3. os.ReadFile TOCTOU → os.Root. Settings reads now go through readConfined, which opens an os.Root anchored at the file's parent dir and reads the basename within it, so the open can't be redirected outside that directory by a swapped/symlinked path. An escaping symlink surfaces as a non-ENOENT error and is handled fail-soft; added TestLoadCheckpointsConfig_RejectsEscapingSymlink to prove confinement.

Two things worth flagging on #3, so they're a conscious decision rather than a surprise:

Want me to do the settings-wide os.Root consistency follow-up now, or leave that for a separate change?

9e4597eAddress review: test-only registration, fwd-compat error, confined reads\ \ Three reviewer points on the Phase 2 stack:\ - fsstore registration is no longer a public symbol on the production package.\ The factory, its config, the backend-type name, and registerForTesting moved\ into register_test.go, so the production fsstore package exposes only the\ Store — a production binary has no way to register the "fs" backend.\ - The checkpoints loader keeps DisallowUnknownFields (so typos surface instead\ of being silently dropped), but it is not forward-compatible: a field added\ by a newer CLI errors in an older one. Documented that adding a field is a\ coordinated rollout (ship the reader first), and the decode error now tells\ the user an unrecognized field may mean the file was written by a newer CLI\ and to confirm they are on the latest version.\ - Settings reads go through an os.Root anchored at the file's parent dir\ (readConfined) instead of a bare os.ReadFile of an absolute path, so the open\ cannot be redirected outside that directory by a swapped/symlinked path\ (TOCTOU). An escaping symlink surfaces as a non-ENOENT error and is handled\ fail-soft; added a regression test. Note: the rest of the settings package\ still uses os.ReadFile, so making it consistent is a worthwhile follow-up.\ \ Co-Authored-By: Claude Opus 4.8 (1M context) noreply@anthropic.com\ Entire-Checkpoint: e9de15786526+114/-58

do it now

3w ago·15m·15 calls

Done — pushed (9e4597e17a..41b7522cec). The settings package now reads all its files through the confined os.Root helper consistently.

What changed: promoted readConfined to settings.go and routed every settings/preferences read through it — loadFromFile (base), loadMergedSettings (local override), LoadProjectRaw, LoadLocalRaw, and loadClonePreferencesFromFile. Each now opens an os.Root at the file's parent dir and reads the basename within it, so no read can be redirected outside that directory by a swapped/symlinked path. Callers switched from os.IsNotExist to errors.Is(err, fs.ErrNotExist) since the helper wraps its errors.

Net effect: the earlier split-brain is gone — a symlinked .entire/settings.json is now treated the same way (refused, fail-soft) by both Load and the checkpoints loader, rather than working for one and not the other.

Verification: fmt + lint (0 issues) + full unit suite (6860 tests) green. The existing Load/clone-prefs/raw-load tests all pass unchanged through the confined reads, and the escaping-symlink regression test covers the confinement behavior of the shared helper.

That closes out all three review points plus the consistency follow-up. The same symlink trade-off I flagged now applies package-wide (a user who symlinks any settings file to an absolute target would have it refused) — worth a line in the PR/review reply so it's a known, deliberate behavior. Want me to reply on the review thread noting the follow-up landed, or leave it?

41b7522Route all settings-package file reads through confined os.Root\ \ Follow-up to the checkpoints-loader TOCTOU fix: make the rest of the settings\ package consistent. Promote readConfined to settings.go and route every\ settings/preferences file read through it — loadFromFile (base settings),\ loadMergedSettings (local override), LoadProjectRaw, LoadLocalRaw, and\ loadClonePreferencesFromFile — instead of bare os.ReadFile of an absolute path.\ \ Each read now opens an os.Root anchored at the file's parent directory, so a\ swapped or symlinked path can't redirect the open outside that directory. The\ helper wraps its errors, so callers classify "missing" with\ errors.Is(err, fs.ErrNotExist) rather than os.IsNotExist. This removes the\ earlier split-brain where a symlinked settings.json worked for Load but was\ refused only by the checkpoints loader.\ \ Co-Authored-By: Claude Opus 4.8 (1M context) noreply@anthropic.com\ Entire-Checkpoint: 50640125952d+40/-39