Checkpoint Backend Registry and Settings-Driven Selection

Claude Code·Opus 4.8[1m]·

Soph·2mo ago·17hr 30min·11 Checkpoints·46 file changes·+2060/-297·1.6M tokens

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

Register the built-in "git" factory (wraps NewGitStore + SetBlobFetcher).
Open resolves the primary type (default "git") through build(...) instead of calling NewGitStore directly.
Tests: registry round-trip; unknown type → clear error; default path produces the same *GitStore as before.

PR 2b — Selection settings (leaner shape)

Helpers settings.CheckpointPrimary() (defaults to {Type:"git"} when nil) and CheckpointMirrors().
Open reads them; only "git" registered so far → opting in does nothing yet, default unchanged.
Tests: nil/empty config → git primary, no mirrors; round-trip parse of a config with mirrors.

PR 2c — Mirror fan-out topology

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

Open builds primary + mirrors from the registry; wraps in fanoutStore only when mirrors are configured, assigns to Stores.Persistent.
Ephemeral stays git-only and independent of primary (temporary capture is always git shadow branches).
Tests: fan-out hits primary + every mirror; a failing mirror doesn't fail the write; reads never touch mirrors; zero mirrors → primary returned unwrapped (no overhead).

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

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

New package cmd/entire/cli/checkpoint/fsstore implementing the full PersistentStore against a directory tree (one JSON file per checkpoint/session). Registered as "fs".
The plumbing.Hash fields (SessionContent.TranscriptBlobHashes, PrecomputedTranscriptBlobs) are git-only optimizations — fsstore ignores them and stores transcript bytes directly. This makes the contract leak concrete (good input for the follow-up that de-gits the DTOs).
Tests: integration test with checkpoints.primary = git, mirrors = [{type:"fs", config:{path}}] — write a checkpoint, assert it materializes in both git and the fs dir; reads still resolve from git.

Notably out of scope (and why)

v1.1 re-expression — dropped per your call; the version knobs are already gone from this branch.
Sync relocation / Syncable — that's Phase 3.
Rewind on a non-git primary — the reference backend is intended as a mirror (primary stays git), so rewind is unaffected. If someone sets fs as primary, I'd follow the issue's recommendation (a): disable rewind. Worth a one-line guard, but not core to Phase 2.
De-gitting the api/checkpoint DTOs (plumbing.Hash) — surfaced by PR 2e, handled as a separate follow-up.

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?

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:

DeleteOrphanedCheckpoints rewrites the primary ref directly (strategy/cleanup.go:395,482) → mirrors never see checkpoint deletes.
OPF re-redaction rewrites entire/checkpoints/v1 at pre-push (strategy/manual_commit_opf_rewrite.go:213,731) → a mirror written at checkpoint time never gets OPF re-redacted. That's a privacy hazard, not just a sync gap.

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:

A git mirror sharing OpenEnv.Refs would write the same ref twice → duplicate metadata commits. mirrors:[{type:"git"}] is not inert; must be rejected or require distinct config.
Making Open read settings can newly fail default-config commands if unrelated settings are malformed (top-level parsing is strict, settings.go:601,741). Only fail when checkpoints is present-and-invalid.
The fan-out wrapper, by implementing only PersistentStore, drops the optional AuthorReader that explain.go:733 type-asserts for. The wrapper must forward optional interfaces.
Stale fact in my plan: PersistentRefs.Push is already []ReferenceName (not a single ref).

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 four WriteRequest variants, not just Session).

How I'd fold this into the plan

New PR 2a-pre: route tokens profile through checkpoint.Open (drop the direct *GitStore) — a Phase-0-style mechanical fix, lands first.
Reject non-git primary in the registry/Open (clear error), instead of the rewind guard. fs is mirror-only.
Fan-out wrapper forwards optional interfaces (AuthorReader, and anything else type-asserted) via pass-through; reject/guard a git-typed mirror that resolves to the same refs.
Open settings-read is fail-soft: only error when a checkpoints block exists and is invalid.
Explicit decision needed on cleanup + OPF: either (a) document that mirrors are write-only, may retain deleted/un-OPF'd checkpoints, and are never a read/sync source without reconciliation; or (b) extend fan-out to cover delete and pre-push OPF. I lean (a) for Phase 2 (keeps scope tight, matches "best-effort" framing) and defer (b)'s reconciliation to Phase 3 with sync — but the OPF/privacy angle makes (b) defensible if mirrors might ever hold real data.
Codex also suggests merging 2b+2c so mirrors settings never exist without behavior. Reasonable.

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

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).
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?

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

Normal redaction is a 7-layer pipeline that runs per-commit during condensation (fast path).
OPF is an 8th layer that's heavier, so it does not run per-commit. Instead it runs at pre-push: when enabled, the PrePush hook re-redacts any unpushed entire/checkpoints/v1 commits through the OPF layer, builds new commits carrying an Entire-OPF-Applied: true trailer, and atomically updates the local v1 ref before pushing.
So a checkpoint can be written locally without OPF, and only gets the OPF pass when it's about to leave the machine (push). Implementation lives in strategy/manual_commit_opf_rewrite.go; the full flow is in docs/security-and-privacy.md.

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.

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:

OPF/cleanup gap is no longer a Phase 2 blocker. Since no real user data ever reaches a mirror (the only mirror backend is test-only), the missing OPF re-redaction and missing delete-fan-out aren't a privacy hazard now. I'll document them as known constraints of the mirror seam ("mirrors are best-effort write-only; they do not receive deletes or pre-push OPF re-redaction, and must never be promoted to a read/sync source without reconciliation") — and that reconciliation + OPF-on-mirror becomes a prerequisite for any real mirror backend, which is Phase 3 (sync) territory.
Non-git primary stays rejected outright (unchanged from the Codex-revised plan) — fs is mirror-only, and even as a mirror it's test-only.
PR 2e becomes "example + test-only reference backend": the fsstore package ships as a documented example of how to implement the contract, with RegisterForTesting, and is exercised by the seam integration test (covering all four WriteRequest variants). No production registration.

So the open decisions are now settled:

Cleanup/OPF → option (a), documented as a constraint, safe because mirrors hold no real data in Phase 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?

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:

2a-pre — route tokens profile through checkpoint.Open (drop the direct *GitStore); explicitly exempt benchutil as bench fixture.
2a — registry + Factory/OpenEnv; production registers only "git" (validates Repo != nil); unknown type → clear error. fsstore import-cycle handled via RegisterForTesting.
2b+2c (merged) — lean checkpoints.{primary, mirrors} {type, config} schema; lenient checkpoint-only settings loader (not strict settings.Load); reject non-git primary and same-ref git mirror; fanoutStore (reads→primary, best-effort mirror writes, forwards AuthorReader, zero-mirror → raw store).
2e — filesystem/JSON reference backend, example + test-only (RegisterForTesting, unregisterable in production); integration test exercises all four WriteRequest variants.

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:

Start implementing 2a-pre (smallest, mechanical, no behavior change) to kick off the stack, or
Post the finalized plan to issue #1433 as the Phase 2 work breakdown, or
A third Codex pass isn't needed — it's down to wording, not substance.

Want me to do any of those?

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

2mo ago·32m

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:

Route tokens profile through checkpoint.Open — closed the one production store-construction site Phase 0 missed; exempted benchutil.
Backend registry + factory — Register/build/OpenEnv/Factory; built-in git backend; Open resolves the primary through the registry (default = git, identical behavior).
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).
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.
First-review fixes — Open fail-soft on all settings read errors; fsstore CreatedAt default, extra metadata fields, atomic writes; doc on cwd-context resolution.
/simplify — exported RedactedJoinedPrompts; extracted shared settings-path helpers (removed triplication).
Final-review fix — fsstore redacts summaries via the shared RedactSummary.

Review trajectory

Codex pass 1: "not mergeable" — 4 findings (settings read-error regression, fsstore contract gaps, non-atomic writes, cwd-vs-repo). All addressed.
/simplify: altitude/simplification clean; applied the two reuse wins, skipped the suggestions that would lose determinism or add indirection. Skipped the per-Open settings-I/O efficiency note (the only fix is a cache, which adds staleness — not worth it for Phase 2's tiny files).
Codex pass 2: "mergeable as-is." Two non-blocking test-only caveats; I closed the summary-redaction one for fidelity and documented the rest.

Still open (future phases, unchanged)

Phase 3 — relocate push/fetch sync into the store; cleanup-delete + pre-push OPF fan-out to mirrors (the documented mirror limitation); doctor mirror repair.
Follow-up — de-gitting the api/checkpoint plumbing.Hash DTOs; a real backend (S3/gmeta) when wanted.

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?

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:

I haven't linked it on the tracking issue #1433 or updated the Phase 2 checkbox there — say the word and I'll post a comment / tick the box.
Nothing is merged; it's purely up for your review. If you want changes after reading, I can amend commits in place (the stack is structured so each is independently reviewable).

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

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 base checkpoints 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?

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:

resume — bootstraps the metadata branch from origin and reads trees off it: refs.ReadBootstrappableFromOrigin(), repo.Reference(refs.Primary), strategy.GetMetadataRefTree(repo, refs.Primary), SafelyAdvanceLocalRef(..., refs.Primary, ...) (resume.go:212,431,452,845).
attach — guards on repo.Reference(refs.Primary) before reading (attach.go:469).
explain — loadPrimaryMetadataRootTree reads the git tree at refs.Primary (explain.go:846).
doctor — reconciles the local metadata branch against origin/<primary> (doctor.go:362,388).
git_operations.go — asserts refs.Primary.IsBranch() and advances it from origin (:455,500).
cleanup / OPF / push — delete-orphans rewrites the primary ref directly, pre-push OPF rewrites the v1 ref, push iterates refs.Push (all bypass Write).
ephemeral (shadow branches) — identifies the committed branch by comparing to refs.Primary.Short() (ephemeral.go:240).

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.

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:

git-backed — the record lives in this repo's git object store, so the lifecycle code (resume bootstrap, doctor reconcile, explain tree-read, push, cleanup, OPF) can drive it through the repo + a ref. Both git-branch (today) and a future git-refs are git-backed; they differ only in the storage scheme inside git.
not git-backed — S3/gmeta/etc. The lifecycle has nothing to fetch/advance/reconcile, so these are mirror-only until Phase 3 moves the lifecycle behind the store.

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

Family: git-backed backends. Variants: git-branch (now), git-refs / git-notes (later).
Constraint: "the primary must use a git-backed store (e.g. git-branch)."
Capability: the registration flag is naturally GitBacked (or, read as the predicate, "may be primary" ⇔ "is git-backed").

Cleaner error:

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.

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:

git-branch primary + git-branch mirror → same env.Refs, same branch → genuine double-write. Block.
git-branch primary + git-refs mirror → distinct ref namespaces → fine. Allow (your rollout).

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.

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:

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".
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?

Both updated.

PR #1533 description — rewritten around the current design: the git-branch backend type, the two topology rules (primary must be git-backed, at most one backend of each type, with the note that this allows a future git-refs mirror alongside a git-branch primary), the test-only fsstore, supporting changes, and the review trail.
Trail #672 body — updated in its Problem/Solution prose style with the same corrections (git-branch naming, git-backed-primary + one-of-each-type rules, the branch↔refs parallel-rollout intent).

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?

Done — everything's wired up.

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

Cursor Bugbot (local-override loader bug) → resolved.
Copilot (atomic-write temp cleanup) → resolved.

Linked to #1433:

Posted a comment on #1433 noting Phase 2 is implemented in #1533, with the two design divergences (the git-branch type name; git-backed-primary + one-of-each-type rules instead of "primary must be git"), and that v1.1 re-expression was dropped.
Filled in the Phase 2 _PR: TBD_ pointer in the issue body → _PR: #1533 …_ (left the checkbox unchecked since it's PR'd, not merged — matching the issue's convention where [x] = merged).

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 <package>_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.

2mo ago·18m

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:

Consistency: the rest of the settings package (loadFromFile, clone-prefs, etc.) still uses bare os.ReadFile on the same files. So today a symlinked .entire/settings.json works for settings.Load but is refused by LoadCheckpointsConfig — a slight split-brain. Making the whole settings package confined is the clean follow-up; I scoped this to the reviewed code. Happy to do the broader pass (or file it).
Symlink precedent: this repo previously hit a real bug where os.Root rejected a legitimately-symlinked config and silently dropped it (checkpoint/configloader.go exists to work around exactly that for global git config). For repo-local .entire/settings.json that's far less common, and here the failure is fail-soft (defaults to git-branch) rather than a crash — but if anyone symlinks their settings file, their checkpoints config would now be ignored. Acceptable given the threat model (we only parse-and-discard the content), but you should know it's the same class of trade-off.

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

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?