Skip to content

Commit 4fdb8aa

Browse files
peyton-altclaude
andcommitted
fix(checkpoint): harden the write-probe fetcher per re-review
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 <noreply@anthropic.com>
1 parent 96013ab commit 4fdb8aa

9 files changed

Lines changed: 153 additions & 27 deletions

File tree

cmd/entire/cli/checkpoint/open.go

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -167,7 +167,11 @@ func buildMirrors(ctx context.Context, env OpenEnv, cfg *settings.CheckpointsCon
167167
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)
168168
}
169169
seen[m.Type] = true
170-
store, err := build(ctx, env, m.Type, m.Config)
170+
// Mirrors are best-effort write-only copies whose failures are logged
171+
// and dropped; never pay on-demand ref-fetch network probes for them.
172+
mirrorEnv := env
173+
mirrorEnv.RefFetcher = nil
174+
store, err := build(ctx, mirrorEnv, m.Type, m.Config)
171175
if err != nil {
172176
return nil, fmt.Errorf("checkpoints.mirrors[%d]: %w", i, err)
173177
}

cmd/entire/cli/checkpoint/persistent.go

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1736,6 +1736,9 @@ func (s *GitStore) backfillSummary(ctx context.Context, checkpointID id.Checkpoi
17361736
//
17371737
// Returns ErrCheckpointNotFound if the checkpoint doesn't exist.
17381738
func (s *GitStore) backfillTranscript(ctx context.Context, opts UpdateOptions) error {
1739+
if err := ctx.Err(); err != nil {
1740+
return err //nolint:wrapcheck // Propagating context cancellation
1741+
}
17391742
if opts.CheckpointID.IsEmpty() {
17401743
return errors.New("invalid update options: checkpoint ID is required")
17411744
}

cmd/entire/cli/checkpoint/refs_store.go

Lines changed: 18 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -38,9 +38,12 @@ type gitRefsStore struct {
3838

3939
// fetchFailureMu guards fetchFailure: the first transport-level ref-fetch
4040
// failure, memoized for the store's lifetime so a loop over N missing refs
41-
// (e.g. a stop hook finalizing every checkpoint of a turn) pays a dead
42-
// network once instead of N times. Genuine remote absence is per-ref and
43-
// is never memoized.
41+
// (e.g. a stop hook finalizing every checkpoint of a turn) pays a dead —
42+
// or too-slow-for-the-budget — network once instead of N times. Genuine
43+
// remote absence is per-ref and
44+
// is never memoized. The memo never clears — safe because every
45+
// fetcher-wired store today is opened per command/hook invocation; a
46+
// long-lived fetcher-wired store would need an expiry before reusing this.
4447
fetchFailureMu sync.Mutex
4548
fetchFailure error
4649
}
@@ -339,7 +342,9 @@ func (s *gitRefsStore) resolveRefMaybeFetch(ctx context.Context, cid id.Checkpoi
339342
priorFailure := s.fetchFailure
340343
s.fetchFailureMu.Unlock()
341344
if priorFailure != nil {
342-
return nil, fmt.Errorf("fetch checkpoint ref %s: skipping after earlier fetch failure: %w", refName, priorFailure)
345+
// Note the cause may name a DIFFERENT ref — it is the first failure
346+
// of this operation, remembered so the outage is paid once.
347+
return nil, fmt.Errorf("fetch checkpoint ref %s: skipped, an earlier checkpoint-ref fetch already failed in this operation: %w", refName, priorFailure)
343348
}
344349
if fetchErr := s.refFetcher(ctx, refName); fetchErr != nil {
345350
if errors.Is(fetchErr, plumbing.ErrReferenceNotFound) {
@@ -350,11 +355,16 @@ func (s *gitRefsStore) resolveRefMaybeFetch(ctx context.Context, cid id.Checkpoi
350355
slog.String("ref", refName.String()))
351356
return nil, plumbing.ErrReferenceNotFound
352357
}
353-
s.fetchFailureMu.Lock()
354-
if s.fetchFailure == nil {
355-
s.fetchFailure = fetchErr
358+
// Memoize only network verdicts: a cancellation originating from the
359+
// CALLER's context says nothing about the remote and must not poison
360+
// later fetches on this store.
361+
if ctx.Err() == nil {
362+
s.fetchFailureMu.Lock()
363+
if s.fetchFailure == nil {
364+
s.fetchFailure = fetchErr
365+
}
366+
s.fetchFailureMu.Unlock()
356367
}
357-
s.fetchFailureMu.Unlock()
358368
logging.Debug(ctx, "git-refs: on-demand checkpoint ref fetch failed",
359369
slog.String("ref", refName.String()), slog.String("error", fetchErr.Error()))
360370
return nil, fmt.Errorf("fetch checkpoint ref %s: %w", refName, fetchErr)

cmd/entire/cli/checkpoint/refs_store_test.go

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -306,6 +306,36 @@ func TestGitRefsStore_FetchFailureMemoized(t *testing.T) {
306306
assert.Equal(t, 1, calls, "the outage must be paid once, not per checkpoint")
307307
})
308308

309+
t.Run("caller cancellation not memoized", func(t *testing.T) {
310+
t.Parallel()
311+
store := newRefsStore(t)
312+
calls := 0
313+
cancelCtx, cancel := context.WithCancel(context.Background())
314+
store.SetRefFetcher(func(_ context.Context, _ plumbing.ReferenceName) error {
315+
calls++
316+
cancel() // the CALLER's context dies mid-fetch (e.g. Ctrl-C)
317+
return context.Canceled
318+
})
319+
err := store.Write(cancelCtx, SessionSummary{
320+
CheckpointID: id.MustCheckpointID("aaaaaaaaaaaa"),
321+
Summary: &Summary{Intent: "x"},
322+
})
323+
require.Error(t, err)
324+
325+
// A later fetch on the same store must still run: the cancellation
326+
// said nothing about the network.
327+
store.SetRefFetcher(func(_ context.Context, _ plumbing.ReferenceName) error {
328+
calls++
329+
return assert.AnError
330+
})
331+
err = store.Write(context.Background(), SessionSummary{
332+
CheckpointID: id.MustCheckpointID("bbbbbbbbbbbb"),
333+
Summary: &Summary{Intent: "x"},
334+
})
335+
require.ErrorIs(t, err, assert.AnError)
336+
assert.Equal(t, 2, calls, "a caller cancellation must not be memoized as a network failure")
337+
})
338+
309339
t.Run("remote absence not memoized", func(t *testing.T) {
310340
t.Parallel()
311341
store := newRefsStore(t)

cmd/entire/cli/checkpoint/remote/checkpoint_ref.go

Lines changed: 42 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ import (
44
"bytes"
55
"context"
66
"fmt"
7+
"strings"
78
"time"
89

910
"github.com/go-git/go-git/v6/plumbing"
@@ -27,11 +28,20 @@ const readFetchTimeout = 2 * time.Minute
2728
// resolution fails, it falls back to the origin remote name so callers can
2829
// still attempt a fetch.
2930
func CheckpointFetchTarget(ctx context.Context) string {
30-
url, err := FetchURL(ctx)
31+
target, _ := checkpointFetchTarget(ctx)
32+
return target
33+
}
34+
35+
// checkpointFetchTarget is CheckpointFetchTarget plus whether the target is
36+
// authoritative for checkpoint refs (see fetchURLAuthoritative). The bare
37+
// "origin" fallbacks are non-authoritative: they exist so a fetch can still
38+
// be attempted, not to certify where checkpoint refs live.
39+
func checkpointFetchTarget(ctx context.Context) (string, bool) {
40+
url, authoritative, err := fetchURLAuthoritative(ctx)
3141
if err == nil && url != "" {
32-
return url
42+
return url, authoritative
3343
}
34-
return "origin"
44+
return "origin", false
3545
}
3646

3747
// FetchCheckpointRef fetches a single per-checkpoint ref
@@ -52,7 +62,7 @@ func FetchCheckpointRef(ctx context.Context, ref plumbing.ReferenceName) error {
5262
ctx, cancel := context.WithTimeout(ctx, readFetchTimeout)
5363
defer cancel()
5464

55-
fetchTarget := CheckpointFetchTarget(ctx)
65+
fetchTarget, authoritative := checkpointFetchTarget(ctx)
5666

5767
out, err := LsRemoteInDir(ctx, "", fetchTarget, ref.String())
5868
if err != nil {
@@ -61,20 +71,47 @@ func FetchCheckpointRef(ctx context.Context, ref plumbing.ReferenceName) error {
6171
return fmt.Errorf("probe checkpoint ref %s on %s: %w", ref, RedactURL(fetchTarget), err)
6272
}
6373
if len(bytes.TrimSpace(out)) == 0 {
74+
if !authoritative {
75+
// The probe hit an origin FALLBACK while a checkpoint_remote is
76+
// configured (or undeterminable) — a remote that may simply never
77+
// host the configured checkpoint refs. Emptiness there proves
78+
// nothing; classifying it as absence would silently drop backfills
79+
// for checkpoints that exist on the real checkpoint remote.
80+
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))
81+
}
6482
return fmt.Errorf("checkpoint ref %s not found on %s: %w", ref, RedactURL(fetchTarget), plumbing.ErrReferenceNotFound)
6583
}
6684

6785
refSpec := "+" + ref.String() + ":" + ref.String()
68-
if _, err := Fetch(ctx, FetchOptions{
86+
if fetchOut, err := Fetch(ctx, FetchOptions{
6987
Remote: fetchTarget,
7088
RefSpecs: []string{refSpec},
7189
NoTags: true,
7290
}); err != nil {
91+
// Fold git's own output into the error (redacted): a bare
92+
// "exit status 128" is undebuggable in hook Warn logs.
93+
msg := strings.TrimSpace(string(fetchOut))
94+
msg = strings.ReplaceAll(msg, fetchTarget, RedactURL(fetchTarget))
95+
if msg != "" {
96+
return fmt.Errorf("fetch checkpoint ref %s from %s: %s: %w", ref, RedactURL(fetchTarget), msg, err)
97+
}
7398
return fmt.Errorf("fetch checkpoint ref %s from %s: %w", ref, RedactURL(fetchTarget), err)
7499
}
75100
return nil
76101
}
77102

103+
// HookCheckpointRefFetcher returns the write-probe fetcher for git-hook
104+
// contexts (post-commit attribution, stop-time transcript finalize): the
105+
// bounded budget plus BatchMode SSH, so a passphrase-protected key can never
106+
// prompt — or invisibly hang — inside a hook the user's git command is
107+
// waiting on.
108+
func HookCheckpointRefFetcher() func(context.Context, plumbing.ReferenceName) error {
109+
bounded := BoundedCheckpointRefFetcher(WriteProbeFetchBudget)
110+
return func(ctx context.Context, ref plumbing.ReferenceName) error {
111+
return bounded(WithNonInteractiveSSH(ctx), ref)
112+
}
113+
}
114+
78115
// BoundedCheckpointRefFetcher returns a RefFetchFunc-shaped fetcher whose
79116
// per-call budget is capped at d, for wiring into write-path checkpoint
80117
// stores (see WriteProbeFetchBudget).

cmd/entire/cli/checkpoint/remote/checkpoint_ref_test.go

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -65,6 +65,23 @@ func TestFetchCheckpointRef_PresentRefFetches(t *testing.T) {
6565
require.NoError(t, err, "fetched ref must exist locally: %s", out)
6666
}
6767

68+
// TestFetchCheckpointRef_FallbackTargetNeverClassifiesAbsence: when a
69+
// checkpoint_remote is configured but cannot be resolved (unknown provider +
70+
// an origin whose protocol can't be mapped), the probe runs against an origin
71+
// FALLBACK that never hosts the configured checkpoint refs. Emptiness there
72+
// must be a failure, not absence — absence would silently drop backfills for
73+
// checkpoints that exist on the real checkpoint remote.
74+
func TestFetchCheckpointRef_FallbackTargetNeverClassifiesAbsence(t *testing.T) {
75+
workDir, ref := checkpointRefFixture(t, false)
76+
testutil.WriteFile(t, workDir, ".entire/settings.json",
77+
`{"enabled": true, "strategy_options": {"checkpoint_remote": {"provider": "bogusforge", "repo": "acme/checkpoints"}}}`)
78+
79+
err := FetchCheckpointRef(context.Background(), ref)
80+
require.Error(t, err)
81+
require.NotErrorIs(t, err, plumbing.ErrReferenceNotFound,
82+
"emptiness on a non-authoritative fallback target must not classify as absence")
83+
}
84+
6885
// TestFetchCheckpointRef_UnreachableRemoteIsFailure: a transport-level
6986
// failure (unreachable remote) must NOT classify as absence.
7087
func TestFetchCheckpointRef_UnreachableRemoteIsFailure(t *testing.T) {

cmd/entire/cli/checkpoint/remote/git.go

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ package remote
33
import (
44
"context"
55
"encoding/base64"
6+
"errors"
67
"fmt"
78
"log/slog"
89
"os"
@@ -461,6 +462,15 @@ func lsRemote(ctx context.Context, dir, remote string, patterns ...string) ([]by
461462
disableTerminalPrompt(cmd)
462463
out, err := cmd.Output()
463464
if err != nil {
465+
// Fold git's stderr into the error (redacted): a bare "exit status
466+
// 128" leaves auth, DNS, and missing-repo failures indistinguishable.
467+
var exitErr *exec.ExitError
468+
if errors.As(err, &exitErr) {
469+
if msg := strings.TrimSpace(string(exitErr.Stderr)); msg != "" {
470+
msg = strings.ReplaceAll(msg, remote, RedactURL(remote))
471+
return out, fmt.Errorf("git ls-remote: %w: %s", err, msg)
472+
}
473+
}
464474
return out, fmt.Errorf("git ls-remote: %w", err)
465475
}
466476
return out, nil

cmd/entire/cli/checkpoint/remote/util.go

Lines changed: 26 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,18 @@ type FetchURLOptions struct {
3939
// If ENTIRE_CHECKPOINT_TOKEN is set and a checkpoint remote is configured, HTTPS is
4040
// forced so the token can be used even when origin is configured via SSH.
4141
func FetchURL(ctx context.Context, opts ...FetchURLOptions) (string, error) {
42+
url, _, err := fetchURLAuthoritative(ctx, opts...)
43+
return url, err
44+
}
45+
46+
// fetchURLAuthoritative is FetchURL plus whether the returned URL is
47+
// authoritative for checkpoint refs. It is false exactly when a
48+
// checkpoint_remote IS configured (or cannot be determined) but resolution
49+
// fell back to the origin URL — a remote that by construction does not host
50+
// the configured checkpoint refs. Callers that classify "ref absent on the
51+
// remote" (FetchCheckpointRef's ls-remote probe) must not treat emptiness on
52+
// a non-authoritative target as absence.
53+
func fetchURLAuthoritative(ctx context.Context, opts ...FetchURLOptions) (string, bool, error) {
4254
var opt FetchURLOptions
4355
if len(opts) > 0 {
4456
opt = opts[0]
@@ -69,17 +81,20 @@ func FetchURL(ctx context.Context, opts ...FetchURLOptions) (string, error) {
6981
if err != nil {
7082
if originURL != "" {
7183
logFallback(ctx, "fetch", originURL, "load settings", err)
72-
return originURL, nil
84+
// Settings unreadable → checkpoint_remote unknown; conservative:
85+
// do not certify origin as authoritative for checkpoint refs.
86+
return originURL, false, nil
7387
}
74-
return "", fmt.Errorf("load settings: %w", err)
88+
return "", false, fmt.Errorf("load settings: %w", err)
7589
}
7690

7791
config := s.GetCheckpointRemote()
7892
if config == nil {
7993
if originURL == "" {
80-
return "", fmt.Errorf("no fetch URL found: %w", originErr)
94+
return "", false, fmt.Errorf("no fetch URL found: %w", originErr)
8195
}
82-
return originURL, nil
96+
// No checkpoint_remote configured: origin IS the checkpoint host.
97+
return originURL, true, nil
8398
}
8499

85100
if withToken {
@@ -90,25 +105,25 @@ func FetchURL(ctx context.Context, opts ...FetchURLOptions) (string, error) {
90105
Host: host,
91106
}, config)
92107
if err == nil {
93-
return checkpointURL, nil
108+
return checkpointURL, true, nil
94109
}
95110
}
96111

97112
// In token-based execution path, short-circuit to avoid additional
98113
// change in protocol.
99114
if originURL != "" {
100-
return originURL, nil
115+
return originURL, false, nil
101116
}
102117
}
103118

104119
if originURL == "" {
105-
return "", fmt.Errorf("no fetch URL found: %w", originErr)
120+
return "", false, fmt.Errorf("no fetch URL found: %w", originErr)
106121
}
107122

108123
info, err := ParseURL(originURL)
109124
if err != nil {
110125
logFallback(ctx, "fetch", originURL, "parse origin remote URL", err)
111-
return originURL, nil
126+
return originURL, false, nil
112127
}
113128

114129
checkpointURL, err := deriveCheckpointURLFromInfo(info, config)
@@ -118,13 +133,13 @@ func FetchURL(ctx context.Context, opts ...FetchURLOptions) (string, error) {
118133
// provider). Honor the configured checkpoint_remote by targeting the
119134
// provider's canonical host over HTTPS rather than falling back to origin.
120135
if providerURL, ok := resolveProviderCheckpointURL(config, opt.WorktreeRoot); ok {
121-
return providerURL, nil
136+
return providerURL, true, nil
122137
}
123138
logFallback(ctx, "fetch", originURL, "derive checkpoint remote URL", err)
124-
return originURL, nil
139+
return originURL, false, nil
125140
}
126141

127-
return checkpointURL, nil
142+
return checkpointURL, true, nil
128143
}
129144

130145
// PushURL returns the effective checkpoint push URL for the current repository.

cmd/entire/cli/strategy/manual_commit_hooks.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1068,7 +1068,7 @@ func (s *ManualCommitStrategy) updateCombinedAttributionForCheckpoint(
10681068
// exists remotely but not locally. Bounded budget + the store's failure
10691069
// memo keep a dead network from stalling the post-commit hook.
10701070
stores, err := checkpoint.Open(ctx, repo, checkpoint.OpenOptions{
1071-
RefFetcher: remote.BoundedCheckpointRefFetcher(remote.WriteProbeFetchBudget),
1071+
RefFetcher: remote.HookCheckpointRefFetcher(),
10721072
})
10731073
if err != nil {
10741074
return fmt.Errorf("open checkpoint store: %w", err)
@@ -2908,7 +2908,7 @@ func (s *ManualCommitStrategy) finalizeAllTurnCheckpoints(ctx context.Context, s
29082908
// sessions). Bounded budget + the store's failure memo keep a dead
29092909
// network from stalling the stop hook N times.
29102910
stores, err := checkpoint.Open(ctx, repo, checkpoint.OpenOptions{
2911-
RefFetcher: remote.BoundedCheckpointRefFetcher(remote.WriteProbeFetchBudget),
2911+
RefFetcher: remote.HookCheckpointRefFetcher(),
29122912
})
29132913
if err != nil {
29142914
logging.Warn(logCtx, "finalize: failed to open checkpoint store",

0 commit comments

Comments
 (0)