/simplify + review: fix fallback-on-error, dedup List, share sort helper · Entire

/simplify + review: fix fallback-on-error, dedup List, share sort helper

80c7a4e→main·Soph·1w ago·4 files·+83 added/-19 removed

Addresses the two Cursor Bugbot findings on the read-routing PR plus one reuse cleanup from /simplify (the rest of /simplify was judged already-clean — notably firstResolved/readOrder stay centralized, which the fallback fix relies on):

Tests: added refs-fetch-error-falls-back-to-branch and List-dedup cases.

Skipped (noted): inlining firstResolved/readOrder (centralizing is worth more, especially now the fallback logic lives in one place); the metaAndPrompts wrapper and build-both-stores (negligible per efficiency review); routing-as-a-backend / backends-declare-their-id-kind (real generalization but over-engineering for two backends). List querying both backends is intentional (union completeness) — a bounded per-command cost, not a hot path.

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

Sessions

022f449bbfdaView transcript

Changes

4

1563 unmodified lines

1564
1565
1566
1567
1568
1569
1570
1567
1568
1569
1570

1563 unmodified lines

return nil
    })

// Sort by time (most recent first)
sort.Slice(checkpoints, func(i, j int) bool {
    return checkpoints[i].CreatedAt.After(checkpoints[j].CreatedAt)
})
sortCheckpointInfosByRecency(checkpoints) // most recent first

return checkpoints, nil
}

Mcmd/entire/cli/checkpoint/persistent.go+1/-4

4 unmodified lines

5
6
7
8
8
9
10
380 unmodified lines

391
392
393
395
396
397
394
395
396
397

4 unmodified lines

"errors"
    "fmt"
    "log/slog"
    "sort"
    "strconv"

"github.com/go-git/go-git/v6"
380 unmodified lines

return nil, fmt.Errorf("iterate checkpoint refs: %w", err)
    }

sort.Slice(checkpoints, func(i, j int) bool {
        return checkpoints[i].CreatedAt.After(checkpoints[j].CreatedAt)
    })
sortCheckpointInfosByRecency(checkpoints)
    return checkpoints, nil
}

Mcmd/entire/cli/checkpoint/refs_store.go+1/-4

61 unmodified lines

62
63
64
65
66
67
68
65
66
67
68
69
70
71
72
73
74
72
75
76
74
77
78
79
80
28 unmodified lines

109
110
111
109
110
111
112
113
114
115
116
115
116
117
118
119
120
121
122
123
124
125
126
127
128
129
130
131
132
133
134
135
136
137
138
139
140
141

61 unmodified lines

}

// firstResolved calls read on each store in order and returns the first result
// that is not "absent" (per absent). A real (non-absent) error short-circuits.
// When every store reports absent, the last absent result is returned so callers
// still see the backend's own not-found signal.
// firstResolved calls read on each store in order and returns the first genuine
// hit (a non-absent result with no error). A non-final store that reports absent
// OR errors falls through to the next store, so a transient failure in one
// backend (e.g. a git-refs on-demand fetch error) does not hide a checkpoint that
// resolves in the fallback backend. The final store's result is returned as-is
// (hit, absent, or error), so callers still see the backend's own not-found /
// error signal when nothing resolved.
func firstResolved[T any](stores []PersistentStore, read func(PersistentStore) (T, error), absent func(T, error) bool) (T, error) {
    var v T
    var err error
    for _, st := range stores {
    for i, st := range stores {
        v, err = read(st)
        if !absent(v, err) {
            if i == len(stores)-1 || (err == nil && !absent(v, err)) {
                return v, err
            }
        }
    }
    if err != nil {
        return nil, err //nolint:wrapcheck // in-package store error surfaced verbatim
    }
    // Disjoint by construction (hex on the branch, ULID in refs; a migrated hex
    // ref would appear only in refs). Concatenate and re-sort most-recent-first to
    // match each backend's own List ordering.
    merged := make([]CheckpointInfo, 0, len(branchList)+len(refsList))
    merged = append(merged, branchList...)
    merged = append(merged, refsList...)
sort.Slice(merged, func(i, j int) bool { return merged[i].CreatedAt.After(merged[j].CreatedAt) })
    return merged, nil
    sortCheckpointInfosByRecency(merged)
    // Dedup by ID: during coexistence/migration the same checkpoint can appear in
    // both backends (a ULID mirrored to the branch, or a hex still on the branch
    // and also migrated into refs). Keep the first occurrence — i.e. the most
    // recent after the sort.
    deduped := merged[:0]
    seen := make(map[id.CheckpointID]struct{}, len(merged))
    for _, info := range merged {
        if _, dup := seen[info.CheckpointID]; dup {
            continue
        }
        seen[info.CheckpointID] = struct{}{}
        deduped = append(deduped, info)
    }
    return deduped, nil
}

// sortCheckpointInfosByRecency orders checkpoints most-recent-first by CreatedAt.
// Shared by the git-branch, git-refs, and routing List implementations so they
// present a consistent order.
func sortCheckpointInfosByRecency(checkpoints []CheckpointInfo) {
    sort.Slice(checkpoints, func(i, j int) bool {
        return checkpoints[i].CreatedAt.After(checkpoints[j].CreatedAt)
    })
}

func (s *kindRoutingStore) ReadSessionContent(ctx context.Context, checkpointID id.CheckpointID, sessionIndex int) (*SessionContent, error) {

Mcmd/entire/cli/checkpoint/routing_store.go+33/-11

1 unmodified line

2
3
4
5
6
7
8
9
10
11
76 unmodified lines

88
89
90
91
92
93
94
95
96
97
98
99
100
101
102
103
104
105
106
107
108
109
110
111
112
113
54 unmodified lines

168
169
170
171
172
173
174
175
176
177
178
179
180
181
182
183
184
185
186
187
188
189
190
191
192
193
194
195
196
197
198
199

1 unmodified line

import (
    "context"
    "errors"
    "testing"

"github.com/go-git/go-git/v6/plumbing"
    "github.com/stretchr/testify/assert"
    "github.com/stretchr/testify/require"
)

76 unmodified lines

require.NotNil(t, got, "a hex checkpoint migrated into refs should resolve under a git-refs primary")
})

t.Run("git-refs primary: a refs fetch error falls back to the branch", func(t *testing.T) {
     t.Parallel()
     _, repo, _ := newTestRepo(t)
     branch := NewGitStore(repo, DefaultV1Refs())
     refs := newGitRefsStore(repo)
     // A missing local ref triggers an on-demand fetch; simulate that fetch
     // failing (network down) so the refs read returns a hard error rather than
     // ErrCheckpointNotFound.
     refs.SetRefFetcher(func(context.Context, plumbing.ReferenceName) error {
        return errors.New("network down")
    })
     writeRoutingCheckpoint(t, branch, hexID, "hex-on-branch")

router := newKindRoutingStore(refs, branch, refs, BackendTypeGitRefs)

got, err := router.Read(ctx, hexID)
     require.NoError(t, err, "a refs fetch error must not block the branch fallback")
     require.NotNil(t, got, "hex checkpoint on the branch should resolve even when the refs read errors")
})

t.Run("a ULID is never read from the branch", func(t *testing.T) {
    t.Parallel()
     _, repo, _ := newTestRepo(t)
54 unmodified lines

assert.True(t, seen[ulidID.String()], "list should include the ULID checkpoint from refs")
}

func TestKindRoutingStore_ListDedupesAcrossBackends(t *testing.T) {
    t.Parallel()
     ctx := context.Background()
     _, repo, _ := newTestRepo(t)
     branch := NewGitStore(repo, DefaultV1Refs())
     refs := newGitRefsStore(repo)

// The same checkpoint present in BOTH backends (as happens for a mirrored
    // checkpoint or a migrated one) must appear only once in the merged list.
     dupID := id.MustCheckpointID("a1b2c3d4e5f6")
     writeRoutingCheckpoint(t, branch, dupID, "on-branch")
     writeRoutingCheckpoint(t, refs, dupID, "in-refs")

router := newKindRoutingStore(branch, branch, refs, BackendTypeGitRefs)

infos, err := router.List(ctx)
     require.NoError(t, err)
     count := 0
     for _, info := range infos {
         if info.CheckpointID == dupID {
             count++
         }
     }
     assert.Equal(t, 1, count, "a checkpoint present in both backends should appear once")
}

func TestKindRoutingStore_GetCheckpointAuthorRoutes(t *testing.T) {
    t.Parallel()
     ctx := context.Background()