git-refs: don't mask real errors as not-found; honor checkpoint policy on pre-push · Entire

git-refs: don't mask real errors as not-found; honor checkpoint policy on pre-push

7bbdad0→main·
Soph·2w ago·4 files·+80 added/-20 removed

Addresses the open review threads on the git-refs store.

Error handling (was: any ref-resolution error treated as "missing"):

Pre-push policy (was: git-refs skipped the check the v1 path runs):

Updates the fetch-failure test to assert the corrected contract (error propagates; genuine absence still reads as not-found).

Co-Authored-By: Claude Opus 4.8 (1M context) noreply@anthropic.com

Sessions

7c18df0a2b25View transcript

Changes

4

78 unmodified lines

return plumbing.ZeroHash, nil, err
}
ref, err := s.repo.Reference(refName, true)
if errors.Is(err, plumbing.ErrReferenceNotFound) {
    return plumbing.ZeroHash, nil, nil // no ref yet → new checkpoint (orphan)
}
if err != nil {
    return plumbing.ZeroHash, nil, nil //nolint:nilerr // no ref yet → new checkpoint
    // A real lookup failure (IO/corruption), not an absent ref: surface it
    // rather than silently starting a fresh orphan history over the ref.
    return plumbing.ZeroHash, nil, fmt.Errorf("resolve checkpoint ref %s: %w", refName, err)
}
commit, err := s.repo.CommitObject(ref.Hash())
if err != nil {
153 unmodified lines
}
ref, err := s.resolveRefMaybeFetch(ctx, cid)
if err != nil {
    return nil, ErrCheckpointNotFound
if errors.Is(err, plumbing.ErrReferenceNotFound) {
    return nil, ErrCheckpointNotFound
}
return nil, err
}
commit, err := s.repo.CommitObject(ref.Hash())
if err != nil {
    return nil, ErrCheckpointNotFound
    // The ref resolved but its commit object doesn't — corruption/IO, not an
    // absent checkpoint. Surface it instead of masking as "not found".
    return nil, fmt.Errorf("read checkpoint commit %s for %s: %w", ref.Hash(), cid, err)
}
tree, err := commit.Tree()
if err != nil {
    return nil, ErrCheckpointNotFound
    return nil, fmt.Errorf("read checkpoint tree for %s: %w", cid, err)
}
return NewFetchingTree(ctx, tree, s.repo.Storer, s.blobFetcher), nil
}

// resolveRefMaybeFetch resolves a checkpoint ref, fetching it from the remote
// once when it is missing locally and a ref fetcher is configured (the
// checkpoint may have been written on another machine). A failed fetch returns
// the original not-found error so the read resolves to ErrCheckpointNotFound.
// checkpoint may have been written on another machine). It distinguishes a
// genuinely absent ref (returns a plumbing.ErrReferenceNotFound-wrapped error,
// which callers map to ErrCheckpointNotFound) from a real failure — an IO error,
// or a fetch that failed for network/context reasons — which is returned as-is
// so it is not silently swallowed as "checkpoint not found".
func (s *gitRefsStore) resolveRefMaybeFetch(ctx context.Context, cid id.CheckpointID) (*plumbing.Reference, error) {
refName, err := RefName(cid)
if err != nil {
3 unmodified lines
}
if err == nil {
    return ref, nil
}
if !errors.Is(err, plumbing.ErrReferenceNotFound) {
    return nil, fmt.Errorf("resolve checkpoint ref %s: %w", refName, err)
}
if s.refFetcher == nil {
    return nil, err //nolint:wrapcheck // caller maps any error to ErrCheckpointNotFound
    return nil, err //nolint:wrapcheck // genuinely absent; caller maps ErrReferenceNotFound to ErrCheckpointNotFound
}
if fetchErr := s.refFetcher(ctx, refName); fetchErr != nil {
    logging.Debug(ctx, "git-refs: on-demand checkpoint ref fetch failed",
        slog.String("ref", refName.String()), slog.String("error", fetchErr.Error()))
    return nil, err //nolint:wrapcheck // caller maps any error to ErrCheckpointNotFound
    return nil, fmt.Errorf("fetch checkpoint ref %s: %w", refName, fetchErr)
}
return s.repo.Reference(refName, true) //nolint:wrapcheck // caller maps any error to ErrCheckpointNotFound
// Re-resolve after a successful fetch. ErrReferenceNotFound here means the
// remote genuinely has no such checkpoint; anything else is a real error.
ref, err = s.repo.Reference(refName, true)
if err != nil {
    return nil, err //nolint:wrapcheck // ErrReferenceNotFound (absent) or a real error; caller distinguishes via errors.Is
}
return ref, nil
}

// sessionTree resolves the FetchingTree for one session within a checkpoint ref.
14 unmodified lines

func (s *gitRefsStore) Read(ctx context.Context, checkpointID id.CheckpointID) (*CheckpointSummary, error) {
ct, err := s.checkpointTree(ctx, checkpointID)
if err != nil {
    return nil, nil //nolint:nilnil,nilerr // No ref means no checkpoint exists
if errors.Is(err, ErrCheckpointNotFound) {
    return nil, nil //nolint:nilnil // absent ref → no checkpoint; contract normalizes to ErrCheckpointNotFound
}
return nil, err
}
return readSummaryFromCheckpointTree(ct)
}

Mcmd/entire/cli/checkpoint/refs_store.go+35/-10


```go
86 unmodified lines

assert.Equal(t, 1, fetched, "fetcher invoked once for the missing ref")
}

func TestGitRefsStore_OnDemandRefFetch_FailureIsNotFound(t *testing.T) {
// TestGitRefsStore_OnDemandRefFetch_FailurePropagates: a fetch that fails
// (offline, network error, context cancellation) must surface as a real error
// rather than be masked as "checkpoint not found" — otherwise a transient
// failure looks like missing data. A fetch that succeeds but still finds no such
// ref on the remote is a genuine not-found and reads as (nil, nil).
func TestGitRefsStore_OnDemandRefFetch_FailurePropagates(t *testing.T) {
    t.Parallel()
    store := newRefsStore(t)
    store.SetRefFetcher(func(_ context.Context, _ plumbing.ReferenceName) error {
        return assert.AnError // fetch fails (e.g. offline / unknown checkpoint)
}

t.Run("fetch error propagates", func(t *testing.T) {
            Parallel()
        store := newRefsStore(t)
        store.SetRefFetcher(func(_ context.Context, _ plumbing.ReferenceName) error {
            return assert.AnError // fetch fails (e.g. offline / network)
        })
        summary, err := store.Read(context.Background(), id.MustCheckpointID("ffffffffffff"))
        require.ErrorIs(t, err, assert.AnError, "a failed fetch must not be masked as not-found")
        assert.Nil(t, summary)
    })

summary, err := store.Read(context.Background(), id.MustCheckpointID("ffffffffffff"))
    require.NoError(t, err)
    assert.Nil(t, summary, "a failed fetch reads as not-found, not an error")
    t.Run("successful fetch with still-absent ref reads as not-found", func(t *testing.T) {
            Parallel()
        store := newRefsStore(t)
        store.SetRefFetcher(func(_ context.Context, _ plumbing.ReferenceName) error {
            return nil // fetch "succeeds" but the ref still doesn't exist
        })
        summary, err := store.Read(context.Background(), id.MustCheckpointID("ffffffffffff"))
        require.NoError(t, err, "a genuinely absent checkpoint reads as not-found")
        assert.Nil(t, summary)
    })
}

func TestGitRefsStore_WriteAllVariantsAndRead(t *testing.T) {

Mcmd/entire/cli/checkpoint/refs_store_test.go+26/-7


```go
135 unmodified lines

// swallowed — like the v1 path, they must not block the user's git push — and the
// refs stay queued for the next pre-push. OPF is not applied (it is descoped for
// the git-refs store for now).
//
// It honors the checkpoint policy exactly like the v1 path: the policy gates on
// checkpoint *format* compatibility (diverged from the remote, or an unsupported
// local format), which is independent of the storage backend, so a blocked
// policy skips the ref push (leaving refs queued) rather than pushing.
func (s *ManualCommitStrategy) prePushCheckpointRefs(ctx context.Context, ps pushSettings) error {
if !syncCheckpointPolicyForPrePush(ctx, ps) {
    return nil
}

repo, err := OpenRepository(ctx)
if err != nil {
    logging.Warn(ctx, "git-refs pre-push: open repo failed; skipping checkpoint push",