From f30a34c70cf17ba8e667814531e3c780e75aa210 Mon Sep 17 00:00:00 2001 From: Peyton Montei Date: Tue, 21 Jul 2026 12:20:51 -0700 Subject: [PATCH 1/5] fix(checkpoint): fetch a locally-missing ref before declaring a backfill target absent MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The git-refs store's backfill writes (summary, transcript finalize, attribution) probed for the checkpoint ref with a local-only lookup, while reads on-demand fetch a missing ref when a fetcher is configured. For a checkpoint written or migrated on another machine that means the read path sees it but a backfill declares it absent — under kind routing the write then lands on a stale fallback copy (or errors), and refs-first reads never surface it. Backfills now resolve their base via resolveRefMaybeFetch: a ref missing locally is fetched once before deciding. Absent after the fetch still reports ErrCheckpointNotFound (legitimate fallthrough); a fetch FAILURE surfaces as-is — transient unavailability must not read as absence, or the router would fork the write. Creates keep the local-only probe: their ref never exists yet, and fetch-on-create would add a doomed network round-trip to every condensation and break offline writes. Common-case cost is zero: the ref exists locally and no fetch fires. Co-Authored-By: Claude Fable 5 --- cmd/entire/cli/checkpoint/refs_store.go | 41 +++++++- cmd/entire/cli/checkpoint/refs_store_test.go | 104 +++++++++++++++++++ 2 files changed, 140 insertions(+), 5 deletions(-) diff --git a/cmd/entire/cli/checkpoint/refs_store.go b/cmd/entire/cli/checkpoint/refs_store.go index a8849b965d..a184358b3a 100644 --- a/cmd/entire/cli/checkpoint/refs_store.go +++ b/cmd/entire/cli/checkpoint/refs_store.go @@ -70,8 +70,12 @@ func (s *gitRefsStore) Write(ctx context.Context, req WriteRequest) error { } // refBase resolves a checkpoint ref's current tip commit (the parent for the -// next write) and subtree object (the checkpoint's current contents). A missing -// ref yields (ZeroHash, nil) so the next write becomes an orphan commit. +// next write) and subtree object (the checkpoint's current contents) with a +// 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. func (s *gitRefsStore) refBase(cid id.CheckpointID) (plumbing.Hash, *object.Tree, error) { refName, err := RefName(cid) if err != nil { @@ -86,6 +90,33 @@ func (s *gitRefsStore) refBase(cid id.CheckpointID) (plumbing.Hash, *object.Tree // rather than silently starting a fresh orphan history over the ref. return plumbing.ZeroHash, nil, fmt.Errorf("resolve checkpoint ref %s: %w", refName, err) } + return s.refTip(cid, ref) +} + +// refBaseForBackfill resolves like refBase, but a ref missing locally is +// 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. +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) { + return plumbing.ZeroHash, nil, nil // genuinely absent → backfill reports not-found + } + if err != nil { + return plumbing.ZeroHash, nil, err + } + return s.refTip(cid, ref) +} + +// refTip reads the commit and tree at a resolved checkpoint ref. +func (s *gitRefsStore) refTip(cid id.CheckpointID, ref *plumbing.Reference) (plumbing.Hash, *object.Tree, error) { commit, err := s.repo.CommitObject(ref.Hash()) if err != nil { return plumbing.ZeroHash, nil, fmt.Errorf("read checkpoint commit %s: %w", ref.Hash(), err) @@ -165,7 +196,7 @@ func (s *gitRefsStore) backfillTranscript(ctx context.Context, opts UpdateOption return errors.New("invalid update options: checkpoint ID is required") } - parentHash, existing, err := s.refBase(opts.CheckpointID) + parentHash, existing, err := s.refBaseForBackfill(ctx, opts.CheckpointID) if err != nil { return err } @@ -192,7 +223,7 @@ func (s *gitRefsStore) backfillSummary(ctx context.Context, checkpointID id.Chec return err //nolint:wrapcheck // Propagating context cancellation } - parentHash, existing, err := s.refBase(checkpointID) + parentHash, existing, err := s.refBaseForBackfill(ctx, checkpointID) if err != nil { return err } @@ -216,7 +247,7 @@ func (s *gitRefsStore) backfillAttribution(ctx context.Context, checkpointID id. return err //nolint:wrapcheck // Propagating context cancellation } - parentHash, existing, err := s.refBase(checkpointID) + parentHash, existing, err := s.refBaseForBackfill(ctx, checkpointID) if err != nil { return err } diff --git a/cmd/entire/cli/checkpoint/refs_store_test.go b/cmd/entire/cli/checkpoint/refs_store_test.go index 3cd101504c..636a120214 100644 --- a/cmd/entire/cli/checkpoint/refs_store_test.go +++ b/cmd/entire/cli/checkpoint/refs_store_test.go @@ -118,6 +118,110 @@ 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. +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"}} + }, + "transcript": func(cid id.CheckpointID) WriteRequest { + return SessionTranscript{ + CheckpointID: cid, + SessionID: "sess-1", + Transcript: redact.AlreadyRedacted([]byte("finalized")), + } + }, + "attribution": func(cid id.CheckpointID) WriteRequest { + return CheckpointAttribution{CheckpointID: cid, Attribution: &Attribution{AgentLines: 3}} + }, + } + + for name, makeReq := range backfills { + t.Run(name, func(t *testing.T) { + t.Parallel() + store := newRefsStore(t) + cid := id.MustCheckpointID("a1b2c3d4e5f6") + refsWrite(t, store, cid, "sess-1", "transcript") + + ref, err := store.repo.Reference(mustRefName(t, cid), true) + require.NoError(t, err) + commitHash := ref.Hash() + require.NoError(t, store.repo.Storer.RemoveReference(mustRefName(t, cid))) + + fetched := 0 + store.SetRefFetcher(func(_ context.Context, rn plumbing.ReferenceName) error { + fetched++ + return store.repo.Storer.SetReference(plumbing.NewHashReference(rn, commitHash)) + }) + + require.NoError(t, store.Write(ctx, 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") + }) + } +} + +// 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. +func TestGitRefsStore_BackfillFetchFailureAborts(t *testing.T) { + t.Parallel() + store := newRefsStore(t) + store.SetRefFetcher(func(_ context.Context, _ plumbing.ReferenceName) error { + return assert.AnError // offline / network failure + }) + + err := store.Write(context.Background(), SessionSummary{ + CheckpointID: id.MustCheckpointID("ffffffffffff"), + Summary: &Summary{Intent: "must not land"}, + }) + 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") +} + +// 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). +func TestGitRefsStore_BackfillAbsentAfterFetchIsNotFound(t *testing.T) { + t.Parallel() + store := newRefsStore(t) + store.SetRefFetcher(func(_ context.Context, _ plumbing.ReferenceName) error { + return nil // fetch "succeeds" but the remote has no such ref + }) + + err := store.Write(context.Background(), SessionSummary{ + CheckpointID: id.MustCheckpointID("ffffffffffff"), + Summary: &Summary{Intent: "orphan"}, + }) + require.ErrorIs(t, err, ErrCheckpointNotFound) +} + +// TestGitRefsStore_CreateNeverFetches pins the deliberate split: a create's +// ref never exists yet (locally or remotely), so writeSession must not probe +// the remote — fetch-on-create would add a doomed network round-trip to every +// condensation and break offline writes. +func TestGitRefsStore_CreateNeverFetches(t *testing.T) { + t.Parallel() + store := newRefsStore(t) + store.SetRefFetcher(func(_ context.Context, _ plumbing.ReferenceName) error { + t.Error("a create must never invoke the ref fetcher") + return nil + }) + + refsWrite(t, store, id.MustCheckpointID("a1b2c3d4e5f6"), "sess-new", "fresh transcript") +} + func TestGitRefsStore_WriteAllVariantsAndRead(t *testing.T) { t.Parallel() store := newRefsStore(t) From 2b4b7362c7701e5eb70baec4b5510cd72c8618bd Mon Sep 17 00:00:00 2001 From: Peyton Montei Date: Tue, 21 Jul 2026 12:57:14 -0700 Subject: [PATCH 2/5] fix(checkpoint): harden refs backfill probe per review; rescope to store layer MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- cmd/entire/cli/checkpoint/refs_store.go | 26 +++-- cmd/entire/cli/checkpoint/refs_store_test.go | 103 +++++++++++++++---- cmd/entire/cli/git_operations.go | 8 +- docs/architecture/ref-checkpoint-backend.md | 6 +- 4 files changed, 110 insertions(+), 33 deletions(-) diff --git a/cmd/entire/cli/checkpoint/refs_store.go b/cmd/entire/cli/checkpoint/refs_store.go index a184358b3a..279646005a 100644 --- a/cmd/entire/cli/checkpoint/refs_store.go +++ b/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) { diff --git a/cmd/entire/cli/checkpoint/refs_store_test.go b/cmd/entire/cli/checkpoint/refs_store_test.go index 636a120214..2bae8803fb 100644 --- a/cmd/entire/cli/checkpoint/refs_store_test.go +++ b/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) diff --git a/cmd/entire/cli/git_operations.go b/cmd/entire/cli/git_operations.go index 195103485b..f02e41401e 100644 --- a/cmd/entire/cli/git_operations.go +++ b/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/ // /) 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() diff --git a/docs/architecture/ref-checkpoint-backend.md b/docs/architecture/ref-checkpoint-backend.md index 31f4988113..1b0a8556e3 100644 --- a/docs/architecture/ref-checkpoint-backend.md +++ b/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". From b33b3c2dee8a6ab8d02ff1807a8a9ad49424e543 Mon Sep 17 00:00:00 2001 From: Peyton Montei Date: Tue, 21 Jul 2026 13:22:29 -0700 Subject: [PATCH 3/5] feat(checkpoint): wire the backfill ref fetcher into write paths MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Completes the #1824 fix — previously the fetch-probing backfill base was store-layer plumbing with no production caller wiring a RefFetcher. The two prerequisites the review named are now in place: - remote.FetchCheckpointRef (moved from the cli package so strategy hooks can wire it; cli keeps thin aliases) probes with ls-remote before fetching: a ref the remote genuinely lacks returns an error wrapping plumbing.ErrReferenceNotFound (absence — safe fallthrough under kind routing), while transport failures on either leg surface as-is. git fetch of a missing refspec fails indistinguishably from a network error, which is why wiring was unsafe before this discrimination existed. - Write probes are budget-bounded (remote.WriteProbeFetchBudget, 15s via remote.BoundedCheckpointRefFetcher) and the git-refs store memoizes the first transport-level fetch failure per store, so a stop hook finalizing N checkpoints on a dead network pays the outage once, briefly. Remote absence is per-ref and never memoized. Wired at the three backfill write sites: explain --generate's write store, the stop-hook transcript finalize, and post-commit attribution. The carry-forward and other creates stay unwired — creates never probe. Tests: remote-level absence/present/transport-failure fixtures against a local bare origin; store-level absence classification chain-through and failure-memo behavior (transport memoized once, absence never). Co-Authored-By: Claude Fable 5 --- cmd/entire/cli/checkpoint/refs_store.go | 28 ++++++ cmd/entire/cli/checkpoint/refs_store_test.go | 74 ++++++++++++++++ .../cli/checkpoint/remote/checkpoint_ref.go | 87 +++++++++++++++++++ .../checkpoint/remote/checkpoint_ref_test.go | 79 +++++++++++++++++ cmd/entire/cli/explain.go | 8 +- cmd/entire/cli/git_operations.go | 41 ++------- .../cli/strategy/manual_commit_hooks.go | 16 +++- 7 files changed, 297 insertions(+), 36 deletions(-) create mode 100644 cmd/entire/cli/checkpoint/remote/checkpoint_ref.go create mode 100644 cmd/entire/cli/checkpoint/remote/checkpoint_ref_test.go diff --git a/cmd/entire/cli/checkpoint/refs_store.go b/cmd/entire/cli/checkpoint/refs_store.go index 279646005a..92169a2b77 100644 --- a/cmd/entire/cli/checkpoint/refs_store.go +++ b/cmd/entire/cli/checkpoint/refs_store.go @@ -6,6 +6,7 @@ import ( "fmt" "log/slog" "strconv" + "sync" "github.com/go-git/go-git/v6" "github.com/go-git/go-git/v6/plumbing" @@ -34,6 +35,14 @@ type gitRefsStore struct { blobFetcher BlobFetchFunc refFetcher RefFetchFunc + + // fetchFailureMu guards fetchFailure: the first transport-level ref-fetch + // failure, memoized for the store's lifetime so a loop over N missing refs + // (e.g. a stop hook finalizing every checkpoint of a turn) pays a dead + // network once instead of N times. Genuine remote absence is per-ref and + // is never memoized. + fetchFailureMu sync.Mutex + fetchFailure error } // newGitRefsStore constructs the per-checkpoint-ref store for a repository. @@ -323,7 +332,26 @@ func (s *gitRefsStore) resolveRefMaybeFetch(ctx context.Context, cid id.Checkpoi if s.refFetcher == nil { return nil, err //nolint:wrapcheck // genuinely absent; caller maps ErrReferenceNotFound to ErrCheckpointNotFound } + s.fetchFailureMu.Lock() + priorFailure := s.fetchFailure + s.fetchFailureMu.Unlock() + if priorFailure != nil { + return nil, fmt.Errorf("fetch checkpoint ref %s: skipping after earlier fetch failure: %w", refName, priorFailure) + } if fetchErr := s.refFetcher(ctx, refName); fetchErr != nil { + if errors.Is(fetchErr, plumbing.ErrReferenceNotFound) { + // The fetcher probed the remote and it genuinely lacks this ref + // (remote.FetchCheckpointRef's absence signal) — absence, not a + // failure, and per-ref, so it is not memoized. + logging.Debug(ctx, "git-refs: remote has no such checkpoint ref", + slog.String("ref", refName.String())) + return nil, plumbing.ErrReferenceNotFound + } + s.fetchFailureMu.Lock() + if s.fetchFailure == nil { + s.fetchFailure = fetchErr + } + s.fetchFailureMu.Unlock() logging.Debug(ctx, "git-refs: on-demand checkpoint ref fetch failed", slog.String("ref", refName.String()), slog.String("error", fetchErr.Error())) return nil, fmt.Errorf("fetch checkpoint ref %s: %w", refName, fetchErr) diff --git a/cmd/entire/cli/checkpoint/refs_store_test.go b/cmd/entire/cli/checkpoint/refs_store_test.go index 2bae8803fb..1a1d35f7b5 100644 --- a/cmd/entire/cli/checkpoint/refs_store_test.go +++ b/cmd/entire/cli/checkpoint/refs_store_test.go @@ -2,6 +2,7 @@ package checkpoint import ( "context" + "fmt" "testing" git "github.com/go-git/go-git/v6" @@ -252,6 +253,79 @@ func TestGitRefsStore_BackfillFetchFailureAborts(t *testing.T) { "a fetch failure must not read as absence") } +// TestGitRefsStore_RemoteAbsenceFromFetcherIsNotFound pins the classification +// chain for a fetcher that reports "the remote has no such ref" by wrapping +// plumbing.ErrReferenceNotFound (remote.FetchCheckpointRef's absence signal): +// both backfills and reads must treat it as checkpoint-not-found, not as a +// hard failure. +func TestGitRefsStore_RemoteAbsenceFromFetcherIsNotFound(t *testing.T) { + t.Parallel() + ctx := context.Background() + store := newRefsStore(t) + store.SetRefFetcher(func(_ context.Context, rn plumbing.ReferenceName) error { + return fmt.Errorf("checkpoint ref %s not found on origin: %w", rn, plumbing.ErrReferenceNotFound) + }) + + err := store.Write(ctx, SessionSummary{ + CheckpointID: id.MustCheckpointID("ffffffffffff"), + Summary: &Summary{Intent: "orphan"}, + }) + require.ErrorIs(t, err, ErrCheckpointNotFound, "remote absence must classify as not-found for backfills") + + summary, err := store.Read(ctx, id.MustCheckpointID("ffffffffffff")) + require.NoError(t, err) + assert.Nil(t, summary, "remote absence must classify as not-found for reads") +} + +// TestGitRefsStore_FetchFailureMemoized: a transport-level fetch failure is +// remembered for the store's lifetime, so a loop backfilling N checkpoints on +// a dead network pays the outage once instead of N times (stop hooks finalize +// every checkpoint of a turn). The memoized error stays a hard error — never +// absence. Genuine remote absence is NOT memoized (per-ref, not an outage). +func TestGitRefsStore_FetchFailureMemoized(t *testing.T) { + t.Parallel() + ctx := context.Background() + + t.Run("transport failure fetched once", func(t *testing.T) { + t.Parallel() + store := newRefsStore(t) + calls := 0 + store.SetRefFetcher(func(_ context.Context, _ plumbing.ReferenceName) error { + calls++ + return assert.AnError + }) + + for _, cid := range []string{"aaaaaaaaaaaa", "bbbbbbbbbbbb"} { + err := store.Write(ctx, SessionSummary{ + CheckpointID: id.MustCheckpointID(cid), + Summary: &Summary{Intent: "x"}, + }) + require.ErrorIs(t, err, assert.AnError) + require.NotErrorIs(t, err, ErrCheckpointNotFound) + } + assert.Equal(t, 1, calls, "the outage must be paid once, not per checkpoint") + }) + + t.Run("remote absence not memoized", func(t *testing.T) { + t.Parallel() + store := newRefsStore(t) + calls := 0 + store.SetRefFetcher(func(_ context.Context, rn plumbing.ReferenceName) error { + calls++ + return fmt.Errorf("ref %s not on remote: %w", rn, plumbing.ErrReferenceNotFound) + }) + + for _, cid := range []string{"aaaaaaaaaaaa", "bbbbbbbbbbbb"} { + err := store.Write(ctx, SessionSummary{ + CheckpointID: id.MustCheckpointID(cid), + Summary: &Summary{Intent: "x"}, + }) + require.ErrorIs(t, err, ErrCheckpointNotFound) + } + assert.Equal(t, 2, calls, "absence is per-ref and must not suppress later fetches") + }) +} + // 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 diff --git a/cmd/entire/cli/checkpoint/remote/checkpoint_ref.go b/cmd/entire/cli/checkpoint/remote/checkpoint_ref.go new file mode 100644 index 0000000000..7fc6f434ba --- /dev/null +++ b/cmd/entire/cli/checkpoint/remote/checkpoint_ref.go @@ -0,0 +1,87 @@ +package remote + +import ( + "bytes" + "context" + "fmt" + "time" + + "github.com/go-git/go-git/v6/plumbing" +) + +// WriteProbeFetchBudget bounds the on-demand ref fetch performed by a +// BACKFILL's absence probe. Write paths run inside git hooks (post-commit, +// stop-time finalize) where a dead network must not stall the user's +// workflow for the read path's full fetch window; combined with the +// per-store failure memo in the git-refs store, a loop over N checkpoints +// pays a dead network once, briefly. +const WriteProbeFetchBudget = 15 * time.Second + +// readFetchTimeout bounds the interactive read-path fetch (unchanged from +// the historical FetchCheckpointRef behavior). +const readFetchTimeout = 2 * time.Minute + +// CheckpointFetchTarget returns the git remote (URL or name) that checkpoint +// data is fetched from. It prefers the effective URL resolved by FetchURL, +// which is the source of truth for checkpoint fetch location. If URL +// resolution fails, it falls back to the origin remote name so callers can +// still attempt a fetch. +func CheckpointFetchTarget(ctx context.Context) string { + url, err := FetchURL(ctx) + if err == nil && url != "" { + return url + } + return "origin" +} + +// FetchCheckpointRef fetches a single per-checkpoint ref +// (refs/entire/checkpoints//) 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. +// +// Contract — absence is distinguishable from failure: +// - The remote genuinely lacking the ref returns an error wrapping +// plumbing.ErrReferenceNotFound (probed via ls-remote before fetching, +// because `git fetch` of a missing refspec fails indistinguishably from a +// transport error). Store probes classify this as "checkpoint not found", +// which write routing may legitimately act on. +// - Any transport-level failure (probe or fetch) is surfaced as a real +// error, never mapped to absence — a false "absent" would misdirect a +// backfill onto another backend instead of retrying. +func FetchCheckpointRef(ctx context.Context, ref plumbing.ReferenceName) error { + ctx, cancel := context.WithTimeout(ctx, readFetchTimeout) + defer cancel() + + fetchTarget := CheckpointFetchTarget(ctx) + + out, err := LsRemoteInDir(ctx, "", fetchTarget, ref.String()) + if err != nil { + // Redact: fetchTarget can be a remote URL with embedded credentials + // (CI origin URLs), and this error is logged and shown to users. + return fmt.Errorf("probe checkpoint ref %s on %s: %w", ref, RedactURL(fetchTarget), err) + } + if len(bytes.TrimSpace(out)) == 0 { + return fmt.Errorf("checkpoint ref %s not found on %s: %w", ref, RedactURL(fetchTarget), plumbing.ErrReferenceNotFound) + } + + refSpec := "+" + ref.String() + ":" + ref.String() + if _, err := Fetch(ctx, FetchOptions{ + Remote: fetchTarget, + RefSpecs: []string{refSpec}, + NoTags: true, + }); err != nil { + return fmt.Errorf("fetch checkpoint ref %s from %s: %w", ref, RedactURL(fetchTarget), err) + } + return nil +} + +// BoundedCheckpointRefFetcher returns a RefFetchFunc-shaped fetcher whose +// per-call budget is capped at d, for wiring into write-path checkpoint +// stores (see WriteProbeFetchBudget). +func BoundedCheckpointRefFetcher(d time.Duration) func(context.Context, plumbing.ReferenceName) error { + return func(ctx context.Context, ref plumbing.ReferenceName) error { + ctx, cancel := context.WithTimeout(ctx, d) + defer cancel() + return FetchCheckpointRef(ctx, ref) + } +} diff --git a/cmd/entire/cli/checkpoint/remote/checkpoint_ref_test.go b/cmd/entire/cli/checkpoint/remote/checkpoint_ref_test.go new file mode 100644 index 0000000000..67a44a8f42 --- /dev/null +++ b/cmd/entire/cli/checkpoint/remote/checkpoint_ref_test.go @@ -0,0 +1,79 @@ +package remote + +import ( + "context" + "os/exec" + "testing" + + "github.com/go-git/go-git/v6/plumbing" + "github.com/stretchr/testify/require" + + "github.com/entireio/cli/cmd/entire/cli/testutil" +) + +// checkpointRefFixture creates a work repo whose origin is a local bare repo +// holding (or not holding) a checkpoint ref, and chdirs into the work repo so +// fetch-target resolution finds origin. +func checkpointRefFixture(t *testing.T, withRef bool) (workDir string, ref plumbing.ReferenceName) { + t.Helper() + bareDir := t.TempDir() + out, err := exec.CommandContext(t.Context(), "git", "init", "--bare", bareDir).CombinedOutput() + require.NoError(t, err, "git init --bare: %s", out) + + workDir = t.TempDir() + testutil.InitRepo(t, workDir) + testutil.WriteFile(t, workDir, "f.txt", "content") + testutil.GitAdd(t, workDir, "f.txt") + testutil.GitCommit(t, workDir, "init") + out, err = exec.CommandContext(t.Context(), "git", "-C", workDir, "remote", "add", "origin", bareDir).CombinedOutput() + require.NoError(t, err, "git remote add: %s", out) + + ref = plumbing.ReferenceName("refs/entire/checkpoints/Z9/01KVBJCWYA4YW6J5M9GP655HZ9") + if withRef { + out, err = exec.CommandContext(t.Context(), "git", "-C", workDir, "push", "--quiet", "origin", "HEAD:"+ref.String()).CombinedOutput() + require.NoError(t, err, "git push checkpoint ref: %s", out) + } + + t.Chdir(workDir) + return workDir, ref +} + +// TestFetchCheckpointRef_RemoteMissingRefIsAbsence: a remote that does not +// have the requested checkpoint ref is ABSENCE, not a transport failure — the +// error must wrap plumbing.ErrReferenceNotFound so store probes (reads and +// backfill writes) classify it as "checkpoint not found" and, under kind +// routing, may legitimately fall through to another backend. Before this +// distinction, git fetch of a missing refspec failed like a network error, +// which made wiring a fetcher into write paths unsafe. +func TestFetchCheckpointRef_RemoteMissingRefIsAbsence(t *testing.T) { + _, ref := checkpointRefFixture(t, false) + + err := FetchCheckpointRef(context.Background(), ref) + require.Error(t, err) + require.ErrorIs(t, err, plumbing.ErrReferenceNotFound, + "a ref the remote does not have must classify as absence") +} + +// TestFetchCheckpointRef_PresentRefFetches: the ref exists on the remote but +// not locally; the fetch must create the local ref of the same name. +func TestFetchCheckpointRef_PresentRefFetches(t *testing.T) { + workDir, ref := checkpointRefFixture(t, true) + + require.NoError(t, FetchCheckpointRef(context.Background(), ref)) + + out, err := exec.CommandContext(t.Context(), "git", "-C", workDir, "show-ref", "--verify", ref.String()).CombinedOutput() + require.NoError(t, err, "fetched ref must exist locally: %s", out) +} + +// TestFetchCheckpointRef_UnreachableRemoteIsFailure: a transport-level +// failure (unreachable remote) must NOT classify as absence. +func TestFetchCheckpointRef_UnreachableRemoteIsFailure(t *testing.T) { + workDir, ref := checkpointRefFixture(t, false) + out, err := exec.CommandContext(t.Context(), "git", "-C", workDir, "remote", "set-url", "origin", workDir+"/nonexistent-remote").CombinedOutput() + require.NoError(t, err, "%s", out) + + err = FetchCheckpointRef(context.Background(), ref) + require.Error(t, err) + require.NotErrorIs(t, err, plumbing.ErrReferenceNotFound, + "a transport failure must stay distinguishable from absence") +} diff --git a/cmd/entire/cli/explain.go b/cmd/entire/cli/explain.go index 48f1d9a260..8f4c1a1e0f 100644 --- a/cmd/entire/cli/explain.go +++ b/cmd/entire/cli/explain.go @@ -23,6 +23,7 @@ import ( "github.com/entireio/cli/cmd/entire/cli/agent/types" "github.com/entireio/cli/cmd/entire/cli/checkpoint" "github.com/entireio/cli/cmd/entire/cli/checkpoint/id" + "github.com/entireio/cli/cmd/entire/cli/checkpoint/remote" "github.com/entireio/cli/cmd/entire/cli/interactive" "github.com/entireio/cli/cmd/entire/cli/logging" "github.com/entireio/cli/cmd/entire/cli/palette" @@ -719,7 +720,12 @@ func runExplainCheckpointWithLookup(ctx context.Context, w, errW io.Writer, chec return err } stopLoad(false) // generation prints its own progress to w/errW - writeStores, openErr := checkpoint.Open(ctx, lookup.repo, checkpoint.OpenOptions{}) + // RefFetcher: the summary backfill's absence probe fetches a ref that + // exists remotely but not locally (written/migrated on another + // machine), bounded by the write-probe budget. + writeStores, openErr := checkpoint.Open(ctx, lookup.repo, checkpoint.OpenOptions{ + RefFetcher: remote.BoundedCheckpointRefFetcher(remote.WriteProbeFetchBudget), + }) if openErr != nil { return fmt.Errorf("open checkpoint store: %w", openErr) } diff --git a/cmd/entire/cli/git_operations.go b/cmd/entire/cli/git_operations.go index f02e41401e..1580b5ca45 100644 --- a/cmd/entire/cli/git_operations.go +++ b/cmd/entire/cli/git_operations.go @@ -462,43 +462,18 @@ func FetchMetadataFromCheckpointRemote(ctx context.Context) error { } // resolveCheckpointFetchTarget returns the fetch target for checkpoint data. -// It prefers the effective URL resolved by checkpoint/remote.FetchURL, which is -// the source of truth for checkpoint fetch location. If URL resolution fails, it -// falls back to the origin remote name so callers can still attempt a fetch. +// Thin alias for remote.CheckpointFetchTarget (the single source of truth). func resolveCheckpointFetchTarget(ctx context.Context) string { - url, err := remote.FetchURL(ctx) - if err == nil && url != "" { - return url - } - return "origin" + return remote.CheckpointFetchTarget(ctx) } -// FetchCheckpointRef fetches a single per-checkpoint ref (refs/entire/checkpoints/ -// /) 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. -// -// 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. +// FetchCheckpointRef fetches a single per-checkpoint ref from the checkpoint +// remote. Thin alias for remote.FetchCheckpointRef, kept so existing cli-side +// call sites and OpenOptions wiring stay unchanged; see that function for the +// absence-vs-failure contract (remote-lacks-ref wraps +// plumbing.ErrReferenceNotFound; transport failures surface as-is). func FetchCheckpointRef(ctx context.Context, ref plumbing.ReferenceName) error { - ctx, cancel := context.WithTimeout(ctx, 2*time.Minute) - defer cancel() - - fetchTarget := resolveCheckpointFetchTarget(ctx) - refSpec := "+" + ref.String() + ":" + ref.String() - if _, err := remote.Fetch(ctx, remote.FetchOptions{ - Remote: fetchTarget, - RefSpecs: []string{refSpec}, - NoTags: true, - }); err != nil { - // Redact: fetchTarget can be a remote URL with embedded credentials - // (CI origin URLs), and this error is logged and shown to users. - return fmt.Errorf("fetch checkpoint ref %s from %s: %w", ref, remote.RedactURL(fetchTarget), err) - } - return nil + return remote.FetchCheckpointRef(ctx, ref) //nolint:wrapcheck // thin alias; the remote error carries full context } // FetchBlobsByHash fetches specific blob objects from the remote by their SHA-1 hashes. diff --git a/cmd/entire/cli/strategy/manual_commit_hooks.go b/cmd/entire/cli/strategy/manual_commit_hooks.go index fbe29dbef6..802cbc397a 100644 --- a/cmd/entire/cli/strategy/manual_commit_hooks.go +++ b/cmd/entire/cli/strategy/manual_commit_hooks.go @@ -21,6 +21,7 @@ import ( "github.com/entireio/cli/cmd/entire/cli/checkpoint" "github.com/entireio/cli/cmd/entire/cli/checkpoint/id" + "github.com/entireio/cli/cmd/entire/cli/checkpoint/remote" "github.com/entireio/cli/cmd/entire/cli/checkpointpolicy" "github.com/entireio/cli/cmd/entire/cli/gitops" "github.com/entireio/cli/cmd/entire/cli/interactive" @@ -1063,7 +1064,12 @@ func (s *ManualCommitStrategy) updateCombinedAttributionForCheckpoint( repoDir string, ) error { logCtx := logging.WithComponent(ctx, "attribution") - stores, err := checkpoint.Open(ctx, repo, checkpoint.OpenOptions{}) + // RefFetcher: the attribution backfill's absence probe fetches a ref that + // exists remotely but not locally. Bounded budget + the store's failure + // memo keep a dead network from stalling the post-commit hook. + stores, err := checkpoint.Open(ctx, repo, checkpoint.OpenOptions{ + RefFetcher: remote.BoundedCheckpointRefFetcher(remote.WriteProbeFetchBudget), + }) if err != nil { return fmt.Errorf("open checkpoint store: %w", err) } @@ -2897,7 +2903,13 @@ func (s *ManualCommitStrategy) finalizeAllTurnCheckpoints(ctx context.Context, s // Post-commit emits regex-only blobs; the writer joins + redacts // via checkpoint.redactedJoinedPrompts. OPF runs later, once per // push, in the pre-push rewrite path. - stores, err := checkpoint.Open(ctx, repo, checkpoint.OpenOptions{}) + // RefFetcher: the transcript finalize targets existing checkpoints whose + // refs may live only on the remote (resumed/adopted multi-machine + // sessions). Bounded budget + the store's failure memo keep a dead + // network from stalling the stop hook N times. + stores, err := checkpoint.Open(ctx, repo, checkpoint.OpenOptions{ + RefFetcher: remote.BoundedCheckpointRefFetcher(remote.WriteProbeFetchBudget), + }) if err != nil { logging.Warn(logCtx, "finalize: failed to open checkpoint store", slog.String("error", err.Error()), From 96013abcbf6b1495a50d7d30b5fd882a5ba4d271 Mon Sep 17 00:00:00 2001 From: Peyton Montei Date: Tue, 21 Jul 2026 13:54:51 -0700 Subject: [PATCH 4/5] fix(checkpoint): short-circuit cancelled contexts in backfillTranscript Copilot review finding on #1824: backfillTranscript lacked the ctx.Err() guard its two backfill siblings have, so a cancelled context could still reach the (now potentially fetching) base resolution. Co-Authored-By: Claude Fable 5 --- cmd/entire/cli/checkpoint/refs_store.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/cmd/entire/cli/checkpoint/refs_store.go b/cmd/entire/cli/checkpoint/refs_store.go index 92169a2b77..0ccb841596 100644 --- a/cmd/entire/cli/checkpoint/refs_store.go +++ b/cmd/entire/cli/checkpoint/refs_store.go @@ -209,6 +209,9 @@ func (s *gitRefsStore) writeSession(ctx context.Context, opts WriteOptions) erro } func (s *gitRefsStore) backfillTranscript(ctx context.Context, opts UpdateOptions) error { + if err := ctx.Err(); err != nil { + return err //nolint:wrapcheck // Propagating context cancellation + } if opts.CheckpointID.IsEmpty() { return errors.New("invalid update options: checkpoint ID is required") } From 4fdb8aac627f3bc59eedab14c50ec28cf2cee76c Mon Sep 17 00:00:00 2001 From: Peyton Montei Date: Tue, 21 Jul 2026 14:12:37 -0700 Subject: [PATCH 5/5] fix(checkpoint): harden the write-probe fetcher per re-review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Findings from the wiring commit's multi-agent re-review: - False absence on a fallback target (HIGH): when a checkpoint_remote is configured but unresolvable (unknown provider + unmappable origin protocol), FetchURL falls back to origin — a remote that never hosts the configured checkpoint refs. The ls-remote probe treated its emptiness as absence, silently dropping backfills for checkpoints that exist on the real checkpoint remote. fetchURLAuthoritative now reports whether the resolved URL is authoritative for checkpoint refs; probe-emptiness on a non-authoritative fallback is a failure, never absence. - SSH hygiene in hooks (HIGH): the two hook wirings now use remote.HookCheckpointRefFetcher, which marks the context non-interactive (BatchMode SSH) like the pre-push path — a passphrase-protected key can no longer prompt or hang inside post-commit/stop hooks. - Actionable errors (HIGH): lsRemote folds git's redacted stderr into its error, and FetchCheckpointRef folds the fetch output — a bare 'exit status 128' left auth, DNS, and missing-repo failures indistinguishable in hook Warn logs. - Caller cancellation is no longer memoized as a network failure (a Ctrl-C mid-operation said nothing about the remote); the memoized-skip message notes its cause may name a different ref; mirrors never receive a RefFetcher (best-effort write-only copies must not pay network probes); the git-branch backfillTranscript gains the ctx.Err() guard its siblings have (pre-existing gap surfaced by the same review). Co-Authored-By: Claude Fable 5 --- cmd/entire/cli/checkpoint/open.go | 6 ++- cmd/entire/cli/checkpoint/persistent.go | 3 ++ cmd/entire/cli/checkpoint/refs_store.go | 26 ++++++---- cmd/entire/cli/checkpoint/refs_store_test.go | 30 ++++++++++++ .../cli/checkpoint/remote/checkpoint_ref.go | 47 +++++++++++++++++-- .../checkpoint/remote/checkpoint_ref_test.go | 17 +++++++ cmd/entire/cli/checkpoint/remote/git.go | 10 ++++ cmd/entire/cli/checkpoint/remote/util.go | 37 ++++++++++----- .../cli/strategy/manual_commit_hooks.go | 4 +- 9 files changed, 153 insertions(+), 27 deletions(-) diff --git a/cmd/entire/cli/checkpoint/open.go b/cmd/entire/cli/checkpoint/open.go index 13e954b922..391e4d1b8b 100644 --- a/cmd/entire/cli/checkpoint/open.go +++ b/cmd/entire/cli/checkpoint/open.go @@ -167,7 +167,11 @@ func buildMirrors(ctx context.Context, env OpenEnv, cfg *settings.CheckpointsCon return nil, fmt.Errorf("checkpoints.mirrors[%d]: backend type %q is already used by the primary or another mirror; each backend type may appear at most once", i, m.Type) } seen[m.Type] = true - store, err := build(ctx, env, m.Type, m.Config) + // Mirrors are best-effort write-only copies whose failures are logged + // and dropped; never pay on-demand ref-fetch network probes for them. + mirrorEnv := env + mirrorEnv.RefFetcher = nil + store, err := build(ctx, mirrorEnv, m.Type, m.Config) if err != nil { return nil, fmt.Errorf("checkpoints.mirrors[%d]: %w", i, err) } diff --git a/cmd/entire/cli/checkpoint/persistent.go b/cmd/entire/cli/checkpoint/persistent.go index 6d82820c75..67d135219c 100644 --- a/cmd/entire/cli/checkpoint/persistent.go +++ b/cmd/entire/cli/checkpoint/persistent.go @@ -1736,6 +1736,9 @@ func (s *GitStore) backfillSummary(ctx context.Context, checkpointID id.Checkpoi // // Returns ErrCheckpointNotFound if the checkpoint doesn't exist. func (s *GitStore) backfillTranscript(ctx context.Context, opts UpdateOptions) error { + if err := ctx.Err(); err != nil { + return err //nolint:wrapcheck // Propagating context cancellation + } if opts.CheckpointID.IsEmpty() { return errors.New("invalid update options: checkpoint ID is required") } diff --git a/cmd/entire/cli/checkpoint/refs_store.go b/cmd/entire/cli/checkpoint/refs_store.go index 0ccb841596..756fd5778b 100644 --- a/cmd/entire/cli/checkpoint/refs_store.go +++ b/cmd/entire/cli/checkpoint/refs_store.go @@ -38,9 +38,12 @@ type gitRefsStore struct { // fetchFailureMu guards fetchFailure: the first transport-level ref-fetch // failure, memoized for the store's lifetime so a loop over N missing refs - // (e.g. a stop hook finalizing every checkpoint of a turn) pays a dead - // network once instead of N times. Genuine remote absence is per-ref and - // is never memoized. + // (e.g. a stop hook finalizing every checkpoint of a turn) pays a dead — + // or too-slow-for-the-budget — network once instead of N times. Genuine + // remote absence is per-ref and + // is never memoized. The memo never clears — safe because every + // fetcher-wired store today is opened per command/hook invocation; a + // long-lived fetcher-wired store would need an expiry before reusing this. fetchFailureMu sync.Mutex fetchFailure error } @@ -339,7 +342,9 @@ func (s *gitRefsStore) resolveRefMaybeFetch(ctx context.Context, cid id.Checkpoi priorFailure := s.fetchFailure s.fetchFailureMu.Unlock() if priorFailure != nil { - return nil, fmt.Errorf("fetch checkpoint ref %s: skipping after earlier fetch failure: %w", refName, priorFailure) + // Note the cause may name a DIFFERENT ref — it is the first failure + // of this operation, remembered so the outage is paid once. + return nil, fmt.Errorf("fetch checkpoint ref %s: skipped, an earlier checkpoint-ref fetch already failed in this operation: %w", refName, priorFailure) } if fetchErr := s.refFetcher(ctx, refName); fetchErr != nil { if errors.Is(fetchErr, plumbing.ErrReferenceNotFound) { @@ -350,11 +355,16 @@ func (s *gitRefsStore) resolveRefMaybeFetch(ctx context.Context, cid id.Checkpoi slog.String("ref", refName.String())) return nil, plumbing.ErrReferenceNotFound } - s.fetchFailureMu.Lock() - if s.fetchFailure == nil { - s.fetchFailure = fetchErr + // Memoize only network verdicts: a cancellation originating from the + // CALLER's context says nothing about the remote and must not poison + // later fetches on this store. + if ctx.Err() == nil { + s.fetchFailureMu.Lock() + if s.fetchFailure == nil { + s.fetchFailure = fetchErr + } + s.fetchFailureMu.Unlock() } - s.fetchFailureMu.Unlock() logging.Debug(ctx, "git-refs: on-demand checkpoint ref fetch failed", slog.String("ref", refName.String()), slog.String("error", fetchErr.Error())) return nil, fmt.Errorf("fetch checkpoint ref %s: %w", refName, fetchErr) diff --git a/cmd/entire/cli/checkpoint/refs_store_test.go b/cmd/entire/cli/checkpoint/refs_store_test.go index 1a1d35f7b5..f3b5fed363 100644 --- a/cmd/entire/cli/checkpoint/refs_store_test.go +++ b/cmd/entire/cli/checkpoint/refs_store_test.go @@ -306,6 +306,36 @@ func TestGitRefsStore_FetchFailureMemoized(t *testing.T) { assert.Equal(t, 1, calls, "the outage must be paid once, not per checkpoint") }) + t.Run("caller cancellation not memoized", func(t *testing.T) { + t.Parallel() + store := newRefsStore(t) + calls := 0 + cancelCtx, cancel := context.WithCancel(context.Background()) + store.SetRefFetcher(func(_ context.Context, _ plumbing.ReferenceName) error { + calls++ + cancel() // the CALLER's context dies mid-fetch (e.g. Ctrl-C) + return context.Canceled + }) + err := store.Write(cancelCtx, SessionSummary{ + CheckpointID: id.MustCheckpointID("aaaaaaaaaaaa"), + Summary: &Summary{Intent: "x"}, + }) + require.Error(t, err) + + // A later fetch on the same store must still run: the cancellation + // said nothing about the network. + store.SetRefFetcher(func(_ context.Context, _ plumbing.ReferenceName) error { + calls++ + return assert.AnError + }) + err = store.Write(context.Background(), SessionSummary{ + CheckpointID: id.MustCheckpointID("bbbbbbbbbbbb"), + Summary: &Summary{Intent: "x"}, + }) + require.ErrorIs(t, err, assert.AnError) + assert.Equal(t, 2, calls, "a caller cancellation must not be memoized as a network failure") + }) + t.Run("remote absence not memoized", func(t *testing.T) { t.Parallel() store := newRefsStore(t) diff --git a/cmd/entire/cli/checkpoint/remote/checkpoint_ref.go b/cmd/entire/cli/checkpoint/remote/checkpoint_ref.go index 7fc6f434ba..8cd045e775 100644 --- a/cmd/entire/cli/checkpoint/remote/checkpoint_ref.go +++ b/cmd/entire/cli/checkpoint/remote/checkpoint_ref.go @@ -4,6 +4,7 @@ import ( "bytes" "context" "fmt" + "strings" "time" "github.com/go-git/go-git/v6/plumbing" @@ -27,11 +28,20 @@ const readFetchTimeout = 2 * time.Minute // resolution fails, it falls back to the origin remote name so callers can // still attempt a fetch. func CheckpointFetchTarget(ctx context.Context) string { - url, err := FetchURL(ctx) + target, _ := checkpointFetchTarget(ctx) + return target +} + +// checkpointFetchTarget is CheckpointFetchTarget plus whether the target is +// authoritative for checkpoint refs (see fetchURLAuthoritative). The bare +// "origin" fallbacks are non-authoritative: they exist so a fetch can still +// be attempted, not to certify where checkpoint refs live. +func checkpointFetchTarget(ctx context.Context) (string, bool) { + url, authoritative, err := fetchURLAuthoritative(ctx) if err == nil && url != "" { - return url + return url, authoritative } - return "origin" + return "origin", false } // FetchCheckpointRef fetches a single per-checkpoint ref @@ -52,7 +62,7 @@ func FetchCheckpointRef(ctx context.Context, ref plumbing.ReferenceName) error { ctx, cancel := context.WithTimeout(ctx, readFetchTimeout) defer cancel() - fetchTarget := CheckpointFetchTarget(ctx) + fetchTarget, authoritative := checkpointFetchTarget(ctx) out, err := LsRemoteInDir(ctx, "", fetchTarget, ref.String()) if err != nil { @@ -61,20 +71,47 @@ func FetchCheckpointRef(ctx context.Context, ref plumbing.ReferenceName) error { return fmt.Errorf("probe checkpoint ref %s on %s: %w", ref, RedactURL(fetchTarget), err) } if len(bytes.TrimSpace(out)) == 0 { + if !authoritative { + // The probe hit an origin FALLBACK while a checkpoint_remote is + // configured (or undeterminable) — a remote that may simply never + // host the configured checkpoint refs. Emptiness there proves + // nothing; classifying it as absence would silently drop backfills + // for checkpoints that exist on the real checkpoint remote. + return fmt.Errorf("checkpoint ref %s not visible on fallback remote %s, and the configured checkpoint remote could not be resolved; refusing to treat this as absence", ref, RedactURL(fetchTarget)) + } return fmt.Errorf("checkpoint ref %s not found on %s: %w", ref, RedactURL(fetchTarget), plumbing.ErrReferenceNotFound) } refSpec := "+" + ref.String() + ":" + ref.String() - if _, err := Fetch(ctx, FetchOptions{ + if fetchOut, err := Fetch(ctx, FetchOptions{ Remote: fetchTarget, RefSpecs: []string{refSpec}, NoTags: true, }); err != nil { + // Fold git's own output into the error (redacted): a bare + // "exit status 128" is undebuggable in hook Warn logs. + msg := strings.TrimSpace(string(fetchOut)) + msg = strings.ReplaceAll(msg, fetchTarget, RedactURL(fetchTarget)) + if msg != "" { + return fmt.Errorf("fetch checkpoint ref %s from %s: %s: %w", ref, RedactURL(fetchTarget), msg, err) + } return fmt.Errorf("fetch checkpoint ref %s from %s: %w", ref, RedactURL(fetchTarget), err) } return nil } +// HookCheckpointRefFetcher returns the write-probe fetcher for git-hook +// contexts (post-commit attribution, stop-time transcript finalize): the +// bounded budget plus BatchMode SSH, so a passphrase-protected key can never +// prompt — or invisibly hang — inside a hook the user's git command is +// waiting on. +func HookCheckpointRefFetcher() func(context.Context, plumbing.ReferenceName) error { + bounded := BoundedCheckpointRefFetcher(WriteProbeFetchBudget) + return func(ctx context.Context, ref plumbing.ReferenceName) error { + return bounded(WithNonInteractiveSSH(ctx), ref) + } +} + // BoundedCheckpointRefFetcher returns a RefFetchFunc-shaped fetcher whose // per-call budget is capped at d, for wiring into write-path checkpoint // stores (see WriteProbeFetchBudget). diff --git a/cmd/entire/cli/checkpoint/remote/checkpoint_ref_test.go b/cmd/entire/cli/checkpoint/remote/checkpoint_ref_test.go index 67a44a8f42..d07c1ed28d 100644 --- a/cmd/entire/cli/checkpoint/remote/checkpoint_ref_test.go +++ b/cmd/entire/cli/checkpoint/remote/checkpoint_ref_test.go @@ -65,6 +65,23 @@ func TestFetchCheckpointRef_PresentRefFetches(t *testing.T) { require.NoError(t, err, "fetched ref must exist locally: %s", out) } +// TestFetchCheckpointRef_FallbackTargetNeverClassifiesAbsence: when a +// checkpoint_remote is configured but cannot be resolved (unknown provider + +// an origin whose protocol can't be mapped), the probe runs against an origin +// FALLBACK that never hosts the configured checkpoint refs. Emptiness there +// must be a failure, not absence — absence would silently drop backfills for +// checkpoints that exist on the real checkpoint remote. +func TestFetchCheckpointRef_FallbackTargetNeverClassifiesAbsence(t *testing.T) { + workDir, ref := checkpointRefFixture(t, false) + testutil.WriteFile(t, workDir, ".entire/settings.json", + `{"enabled": true, "strategy_options": {"checkpoint_remote": {"provider": "bogusforge", "repo": "acme/checkpoints"}}}`) + + err := FetchCheckpointRef(context.Background(), ref) + require.Error(t, err) + require.NotErrorIs(t, err, plumbing.ErrReferenceNotFound, + "emptiness on a non-authoritative fallback target must not classify as absence") +} + // TestFetchCheckpointRef_UnreachableRemoteIsFailure: a transport-level // failure (unreachable remote) must NOT classify as absence. func TestFetchCheckpointRef_UnreachableRemoteIsFailure(t *testing.T) { diff --git a/cmd/entire/cli/checkpoint/remote/git.go b/cmd/entire/cli/checkpoint/remote/git.go index 1de3ef55fb..68f6e823dd 100644 --- a/cmd/entire/cli/checkpoint/remote/git.go +++ b/cmd/entire/cli/checkpoint/remote/git.go @@ -3,6 +3,7 @@ package remote import ( "context" "encoding/base64" + "errors" "fmt" "log/slog" "os" @@ -461,6 +462,15 @@ func lsRemote(ctx context.Context, dir, remote string, patterns ...string) ([]by disableTerminalPrompt(cmd) out, err := cmd.Output() if err != nil { + // Fold git's stderr into the error (redacted): a bare "exit status + // 128" leaves auth, DNS, and missing-repo failures indistinguishable. + var exitErr *exec.ExitError + if errors.As(err, &exitErr) { + if msg := strings.TrimSpace(string(exitErr.Stderr)); msg != "" { + msg = strings.ReplaceAll(msg, remote, RedactURL(remote)) + return out, fmt.Errorf("git ls-remote: %w: %s", err, msg) + } + } return out, fmt.Errorf("git ls-remote: %w", err) } return out, nil diff --git a/cmd/entire/cli/checkpoint/remote/util.go b/cmd/entire/cli/checkpoint/remote/util.go index aad8668cd7..f57122f504 100644 --- a/cmd/entire/cli/checkpoint/remote/util.go +++ b/cmd/entire/cli/checkpoint/remote/util.go @@ -39,6 +39,18 @@ type FetchURLOptions struct { // If ENTIRE_CHECKPOINT_TOKEN is set and a checkpoint remote is configured, HTTPS is // forced so the token can be used even when origin is configured via SSH. func FetchURL(ctx context.Context, opts ...FetchURLOptions) (string, error) { + url, _, err := fetchURLAuthoritative(ctx, opts...) + return url, err +} + +// fetchURLAuthoritative is FetchURL plus whether the returned URL is +// authoritative for checkpoint refs. It is false exactly when a +// checkpoint_remote IS configured (or cannot be determined) but resolution +// fell back to the origin URL — a remote that by construction does not host +// the configured checkpoint refs. Callers that classify "ref absent on the +// remote" (FetchCheckpointRef's ls-remote probe) must not treat emptiness on +// a non-authoritative target as absence. +func fetchURLAuthoritative(ctx context.Context, opts ...FetchURLOptions) (string, bool, error) { var opt FetchURLOptions if len(opts) > 0 { opt = opts[0] @@ -69,17 +81,20 @@ func FetchURL(ctx context.Context, opts ...FetchURLOptions) (string, error) { if err != nil { if originURL != "" { logFallback(ctx, "fetch", originURL, "load settings", err) - return originURL, nil + // Settings unreadable → checkpoint_remote unknown; conservative: + // do not certify origin as authoritative for checkpoint refs. + return originURL, false, nil } - return "", fmt.Errorf("load settings: %w", err) + return "", false, fmt.Errorf("load settings: %w", err) } config := s.GetCheckpointRemote() if config == nil { if originURL == "" { - return "", fmt.Errorf("no fetch URL found: %w", originErr) + return "", false, fmt.Errorf("no fetch URL found: %w", originErr) } - return originURL, nil + // No checkpoint_remote configured: origin IS the checkpoint host. + return originURL, true, nil } if withToken { @@ -90,25 +105,25 @@ func FetchURL(ctx context.Context, opts ...FetchURLOptions) (string, error) { Host: host, }, config) if err == nil { - return checkpointURL, nil + return checkpointURL, true, nil } } // In token-based execution path, short-circuit to avoid additional // change in protocol. if originURL != "" { - return originURL, nil + return originURL, false, nil } } if originURL == "" { - return "", fmt.Errorf("no fetch URL found: %w", originErr) + return "", false, fmt.Errorf("no fetch URL found: %w", originErr) } info, err := ParseURL(originURL) if err != nil { logFallback(ctx, "fetch", originURL, "parse origin remote URL", err) - return originURL, nil + return originURL, false, nil } checkpointURL, err := deriveCheckpointURLFromInfo(info, config) @@ -118,13 +133,13 @@ func FetchURL(ctx context.Context, opts ...FetchURLOptions) (string, error) { // provider). Honor the configured checkpoint_remote by targeting the // provider's canonical host over HTTPS rather than falling back to origin. if providerURL, ok := resolveProviderCheckpointURL(config, opt.WorktreeRoot); ok { - return providerURL, nil + return providerURL, true, nil } logFallback(ctx, "fetch", originURL, "derive checkpoint remote URL", err) - return originURL, nil + return originURL, false, nil } - return checkpointURL, nil + return checkpointURL, true, nil } // PushURL returns the effective checkpoint push URL for the current repository. diff --git a/cmd/entire/cli/strategy/manual_commit_hooks.go b/cmd/entire/cli/strategy/manual_commit_hooks.go index 802cbc397a..17f14a86d5 100644 --- a/cmd/entire/cli/strategy/manual_commit_hooks.go +++ b/cmd/entire/cli/strategy/manual_commit_hooks.go @@ -1068,7 +1068,7 @@ func (s *ManualCommitStrategy) updateCombinedAttributionForCheckpoint( // exists remotely but not locally. Bounded budget + the store's failure // memo keep a dead network from stalling the post-commit hook. stores, err := checkpoint.Open(ctx, repo, checkpoint.OpenOptions{ - RefFetcher: remote.BoundedCheckpointRefFetcher(remote.WriteProbeFetchBudget), + RefFetcher: remote.HookCheckpointRefFetcher(), }) if err != nil { return fmt.Errorf("open checkpoint store: %w", err) @@ -2908,7 +2908,7 @@ func (s *ManualCommitStrategy) finalizeAllTurnCheckpoints(ctx context.Context, s // sessions). Bounded budget + the store's failure memo keep a dead // network from stalling the stop hook N times. stores, err := checkpoint.Open(ctx, repo, checkpoint.OpenOptions{ - RefFetcher: remote.BoundedCheckpointRefFetcher(remote.WriteProbeFetchBudget), + RefFetcher: remote.HookCheckpointRefFetcher(), }) if err != nil { logging.Warn(logCtx, "finalize: failed to open checkpoint store",