fix(checkpoint): harden refs backfill probe per review; rescope to store layer

Review findings from the PR #1824 multi-agent pass:

- Tests: pin the zero-cost claim (backfill with a locally-present ref never
  fetches), verify the fetched-then-backfilled data reads back for all three
  request types, and assert the backfill commit parents on the fetched tip
  rather than orphaning over the checkpoint's history.
- Comments rephrased order-agnostically: the write-fallthrough they cited is
  PR #1811's unmerged behavior; they now state the absence-vs-failure
  contract instead of a phantom mechanism. refBase's doc names its other
  deliberate callers (migrate.go local import; attach's externally-guarded
  writeSession).
- FetchCheckpointRef's stale 'caller treats a fetch failure as not found'
  comment inverted the actual contract this PR depends on; it now documents
  failure-surfaces-never-absence, including the remote-lacks-ref caveat.
- ref-checkpoint-backend.md: the write shape and on-demand-fetch sections now
  describe the create/backfill probe split.

Scope note (review CRITICAL): no production write path currently wires a
RefFetcher, so this PR is store-layer semantics + plumbing; the wiring is a
follow-up gated on (1) FetchCheckpointRef distinguishing remote-lacks-ref
(absence) from transport failure — today wiring would break the
pre-migration-hex fallthrough — and (2) a hook-appropriate fetch timeout.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

Author: peyton-alt

cmd/entire/cli/checkpoint/refs_store.go

@@ -74,8 +74,14 @@ func (s *gitRefsStore) Write(ctx context.Context, req WriteRequest) error {
 // LOCAL-ONLY lookup. A missing ref yields (ZeroHash, nil) so the next write
 // becomes an orphan commit — correct for creates, whose ref never exists yet
 // (locally or remotely); probing the remote would add a doomed round-trip to
-// every condensation and break offline writes. Backfills, which target an
-// existing checkpoint, use refBaseForBackfill instead.
+// every condensation and, with a fetcher configured, fail offline writes.
+// Backfills, which target an existing checkpoint, use refBaseForBackfill
+// instead. Migration (migrate.go) also uses refBase deliberately: it imports
+// from the LOCAL v1 branch and must never probe the remote, even though its
+// target ref may already exist. One writeSession caller does target an
+// existing checkpoint — attach, which adds a session to it — but attach
+// pre-fetches and verifies the ref's presence itself (refreshCheckpoint)
+// before writing, so the local-only probe is safe there too.
 func (s *gitRefsStore) refBase(cid id.CheckpointID) (plumbing.Hash, *object.Tree, error) {
 	refName, err := RefName(cid)
 	if err != nil {
@@ -97,13 +103,15 @@ func (s *gitRefsStore) refBase(cid id.CheckpointID) (plumbing.Hash, *object.Tree
 // first fetched once from the remote (resolveRefMaybeFetch) when a fetcher is
 // configured: a backfill targets an EXISTING checkpoint that may have been
 // written or migrated on another machine, and declaring it absent without
-// looking remotely diverges from the read path — the write would land on a
-// fallback backend (or fail) while reads, which DO fetch, serve the refs
-// copy, leaving the backfill permanently invisible. A ref absent even after
-// the fetch yields (ZeroHash, nil), which the backfill helpers report as
-// ErrCheckpointNotFound; a fetch FAILURE is returned as-is — transient
-// unavailability must not read as absence, or the kind-routing store's
-// fallthrough would fork the write onto a stale copy.
+// looking remotely diverges from the read path — the backfill would be
+// handled as targeting a nonexistent checkpoint while reads, which DO fetch,
+// serve the refs copy, leaving the backfilled data permanently invisible.
+// A ref absent even after the fetch yields (ZeroHash, nil), which the
+// backfill helpers report as ErrCheckpointNotFound — the signal that the
+// checkpoint does not exist in this backend. A fetch FAILURE is returned
+// as-is: transient unavailability must never masquerade as absence, because
+// a caller or routing layer acting on a false "absent" would misdirect the
+// backfill (e.g. onto a stale copy in another backend) instead of retrying.
 func (s *gitRefsStore) refBaseForBackfill(ctx context.Context, cid id.CheckpointID) (plumbing.Hash, *object.Tree, error) {
 	ref, err := s.resolveRefMaybeFetch(ctx, cid)
 	if errors.Is(err, plumbing.ErrReferenceNotFound) {

cmd/entire/cli/checkpoint/refs_store_test.go

@@ -121,30 +121,60 @@ func TestGitRefsStore_OnDemandRefFetch_FailurePropagates(t *testing.T) {
 // TestGitRefsStore_BackfillFetchesMissingRef: a backfill targets an EXISTING
 // checkpoint, which may have been written or migrated on another machine — so
 // like reads, backfills must on-demand fetch a ref that is missing locally
-// before declaring the checkpoint absent. Otherwise the write lands on a
-// fallback store (or errors) while reads — which DO fetch — serve the refs
-// copy, and the backfill is permanently invisible.
+// before declaring the checkpoint absent. Otherwise the backfill is handled
+// as targeting a nonexistent checkpoint while reads — which DO fetch — serve
+// the refs copy, and the backfilled data is permanently invisible.
 func TestGitRefsStore_BackfillFetchesMissingRef(t *testing.T) {
 	t.Parallel()
 	ctx := context.Background()
 
-	backfills := map[string]func(cid id.CheckpointID) WriteRequest{
-		"summary": func(cid id.CheckpointID) WriteRequest {
-			return SessionSummary{CheckpointID: cid, Summary: &Summary{Intent: "fetched intent"}}
+	backfills := map[string]struct {
+		makeReq func(cid id.CheckpointID) WriteRequest
+		verify  func(t *testing.T, store *gitRefsStore, cid id.CheckpointID)
+	}{
+		"summary": {
+			makeReq: func(cid id.CheckpointID) WriteRequest {
+				return SessionSummary{CheckpointID: cid, Summary: &Summary{Intent: "fetched intent"}}
+			},
+			verify: func(t *testing.T, store *gitRefsStore, cid id.CheckpointID) {
+				t.Helper()
+				meta, err := store.ReadSessionMetadata(context.Background(), cid, 0)
+				require.NoError(t, err)
+				require.NotNil(t, meta.Summary)
+				assert.Equal(t, "fetched intent", meta.Summary.Intent)
+			},
 		},
-		"transcript": func(cid id.CheckpointID) WriteRequest {
-			return SessionTranscript{
-				CheckpointID: cid,
-				SessionID:    "sess-1",
-				Transcript:   redact.AlreadyRedacted([]byte("finalized")),
-			}
+		"transcript": {
+			makeReq: func(cid id.CheckpointID) WriteRequest {
+				return SessionTranscript{
+					CheckpointID: cid,
+					SessionID:    "sess-1",
+					Transcript:   redact.AlreadyRedacted([]byte("finalized")),
+				}
+			},
+			verify: func(t *testing.T, store *gitRefsStore, cid id.CheckpointID) {
+				t.Helper()
+				content, err := store.ReadSessionContent(context.Background(), cid, 0)
+				require.NoError(t, err)
+				assert.Equal(t, []byte("finalized"), content.Transcript)
+			},
 		},
-		"attribution": func(cid id.CheckpointID) WriteRequest {
-			return CheckpointAttribution{CheckpointID: cid, Attribution: &Attribution{AgentLines: 3}}
+		"attribution": {
+			makeReq: func(cid id.CheckpointID) WriteRequest {
+				return CheckpointAttribution{CheckpointID: cid, Attribution: &Attribution{AgentLines: 3}}
+			},
+			verify: func(t *testing.T, store *gitRefsStore, cid id.CheckpointID) {
+				t.Helper()
+				summary, err := store.Read(context.Background(), cid)
+				require.NoError(t, err)
+				require.NotNil(t, summary)
+				require.NotNil(t, summary.CombinedAttribution)
+				assert.Equal(t, 3, summary.CombinedAttribution.AgentLines)
+			},
 		},
 	}
 
-	for name, makeReq := range backfills {
+	for name, tc := range backfills {
 		t.Run(name, func(t *testing.T) {
 			t.Parallel()
 			store := newRefsStore(t)
@@ -162,17 +192,50 @@ func TestGitRefsStore_BackfillFetchesMissingRef(t *testing.T) {
 				return store.repo.Storer.SetReference(plumbing.NewHashReference(rn, commitHash))
 			})
 
-			require.NoError(t, store.Write(ctx, makeReq(cid)),
+			require.NoError(t, store.Write(ctx, tc.makeReq(cid)),
 				"a backfill must fetch the missing ref instead of declaring the checkpoint absent")
 			assert.Equal(t, 1, fetched, "fetcher invoked once for the missing ref")
+
+			// The write must have landed on the fetched history, not orphaned
+			// over it: the new tip's parent is the pre-removal commit.
+			newRef, err := store.repo.Reference(mustRefName(t, cid), true)
+			require.NoError(t, err)
+			newTip, err := store.repo.CommitObject(newRef.Hash())
+			require.NoError(t, err)
+			require.Len(t, newTip.ParentHashes, 1)
+			assert.Equal(t, commitHash, newTip.ParentHashes[0],
+				"the backfill commit must parent on the fetched tip")
+
+			tc.verify(t, store, cid)
 		})
 	}
 }
 
+// TestGitRefsStore_BackfillLocalRefNeverFetches pins the zero-cost claim: a
+// backfill whose ref exists locally must not touch the remote — otherwise
+// every summary/attribution/transcript backfill pays a network round-trip and
+// offline finalization breaks.
+func TestGitRefsStore_BackfillLocalRefNeverFetches(t *testing.T) {
+	t.Parallel()
+	store := newRefsStore(t)
+	cid := id.MustCheckpointID("a1b2c3d4e5f6")
+	refsWrite(t, store, cid, "sess-1", "transcript")
+	store.SetRefFetcher(func(_ context.Context, _ plumbing.ReferenceName) error {
+		t.Error("a backfill with a locally-present ref must not fetch")
+		return nil
+	})
+
+	require.NoError(t, store.Write(context.Background(), SessionSummary{
+		CheckpointID: cid,
+		Summary:      &Summary{Intent: "local"},
+	}))
+}
+
 // TestGitRefsStore_BackfillFetchFailureAborts: a failed fetch is a transient
 // availability problem, not evidence of absence. The backfill must surface it
-// as a real error — NOT ErrCheckpointNotFound, which the kind-routing store
-// treats as "try the next backend" and would fork the write onto a stale copy.
+// as a real error — NOT ErrCheckpointNotFound, which callers treat as "the
+// checkpoint does not exist in this backend", a signal a routing layer may
+// act on to select a different backend (forking the write onto a stale copy).
 func TestGitRefsStore_BackfillFetchFailureAborts(t *testing.T) {
 	t.Parallel()
 	store := newRefsStore(t)
@@ -186,13 +249,13 @@ func TestGitRefsStore_BackfillFetchFailureAborts(t *testing.T) {
 	})
 	require.ErrorIs(t, err, assert.AnError, "the fetch failure must surface")
 	require.NotErrorIs(t, err, ErrCheckpointNotFound,
-		"a fetch failure must not read as absence — the router would fall through and fork the write")
+		"a fetch failure must not read as absence")
 }
 
 // TestGitRefsStore_BackfillAbsentAfterFetchIsNotFound pins the genuine-absence
 // contract: a fetch that succeeds but restores no ref means the checkpoint
 // really does not exist in this backend, and the backfill reports the
-// not-found sentinel (which the router may legitimately fall through on).
+// not-found sentinel (which a routing layer may legitimately act on).
 func TestGitRefsStore_BackfillAbsentAfterFetchIsNotFound(t *testing.T) {
 	t.Parallel()
 	store := newRefsStore(t)

cmd/entire/cli/git_operations.go

@@ -476,7 +476,13 @@ func resolveCheckpointFetchTarget(ctx context.Context) string {
 // FetchCheckpointRef fetches a single per-checkpoint ref (refs/entire/checkpoints/
 // <shard>/<id>) from the checkpoint remote into the local ref of the same name,
 // so the git-refs store can resolve a checkpoint written on another machine.
-// Best-effort: the caller treats a fetch failure as "checkpoint not found".
+//
+// Contract: a fetch failure is surfaced as a real error, never mapped to
+// "checkpoint not found" — callers (resolveRefMaybeFetch, for both reads and
+// backfill writes) deliberately keep transient unavailability distinguishable
+// from absence. Note this includes a ref the remote does not have: git fetch
+// of an explicit refspec fails outright, so "absent on the remote" currently
+// surfaces as a fetch error, not as absence.
 func FetchCheckpointRef(ctx context.Context, ref plumbing.ReferenceName) error {
 	ctx, cancel := context.WithTimeout(ctx, 2*time.Minute)
 	defer cancel()

docs/architecture/ref-checkpoint-backend.md

@@ -69,7 +69,7 @@ The git-refs store (`gitRefsStore`, `checkpoint/refs_store.go`) shares the check
 
 Every persistent write (`WriteSession`, and the `Backfill*` operations for transcript / summary / attribution) follows the same shape:
 
-1. **Resolve the ref's current tip** (`refBase`). A missing ref → `(ZeroHash, nil)`, so the first write to a checkpoint becomes an **orphan commit**. A real lookup failure (IO/corruption) is surfaced, never silently treated as "new checkpoint".
+1. **Resolve the ref's current tip.** Creates (`WriteSession`) use the local-only `refBase`: a missing ref → `(ZeroHash, nil)`, so the first write to a checkpoint becomes an **orphan commit**. The `Backfill*` operations use `refBaseForBackfill`, which first on-demand fetches a locally-missing ref (when a ref fetcher is configured — the checkpoint may have been written or migrated on another machine); a ref still absent after the fetch surfaces as `ErrCheckpointNotFound` rather than orphaning, and a fetch failure surfaces as a real error, never as absence. A real lookup failure (IO/corruption) is surfaced, never silently treated as "new checkpoint".
 2. **Build the updated checkpoint subtree** from the existing tree plus the new content (shared `treeWriter` logic).
 3. **Create a commit** with the current tip as parent (orphan on first write, parented thereafter), so each checkpoint accretes its **own per-checkpoint history**.
 4. **Point the ref at the new commit** (`setRef`) and **enqueue it for push**.
@@ -111,9 +111,9 @@ All checkpoint-ref pushes are **fast-forward-only — never a force push.** Ther
 
 When a push *is* rejected as non-fast-forward — genuine divergence, e.g. the same checkpoint was written on two machines — recovery **fetches the remote ref and replays the local-only commits on top** (`fetchAndRebaseRefCommon`), then retries. After the replay the local ref is a fast-forward over the remote, so the retry is *still* non-force and the remote commit is preserved as an ancestor rather than overwritten. A genuine cherry-pick conflict (both sides rewrote the same file, e.g. root `metadata.json`) leaves the ref queued — degrading to the safe state, never forcing.
 
-### On-demand fetch for reads
+### On-demand fetch (reads and backfill writes)
 
-A checkpoint written on another machine has no local ref. When a read misses locally and a **ref fetcher** is configured, `resolveRefMaybeFetch` fetches that one ref from the remote and retries once. It carefully distinguishes:
+A checkpoint written on another machine has no local ref. When a read — or a backfill write's base resolution (`refBaseForBackfill`) — misses locally and a **ref fetcher** is configured, `resolveRefMaybeFetch` fetches that one ref from the remote and retries once. It carefully distinguishes:
 
 - **genuinely absent** (remote has no such checkpoint) → maps to `ErrCheckpointNotFound`;
 - **a real failure** (IO, network, context cancellation) → returned as-is, never swallowed as "not found".