attach: route review-attach metadata guard through Primary

bb0c6f6→main·

pfleidi·1mo ago·3 files·+17 added/-19 removed

checkpointHasSessionMetadata is a write-side defense-in-depth check that asks whether the next write would overwrite a session slot. Reading via refs.Read can miss a slot that exists on Primary, letting the guard pass and the write reuse the slot silently. Bind the read to Primary.

Also tighten three nearby docstrings to drop version-specific examples and caller references.

Sessions

3f6a0ffedf11View transcript

Changes

3

• cmd/entire/cli

• Mattach.go+12/-11

• checkpoint

• Mcommitted.go+3/-4

• Mcommitted_refs.go+2/-4

264 unmodified lines

265
266
267
268
268
269
270
271
72 unmodified lines

344
345
346
347
347
348
349
350
351
352
353
354
355
356
85 unmodified lines

442
443
444
439
440
441
442
443
444
445
446
447
448
449
450
447
448
449
451
452
453

264 unmodified lines

// review-attach on such a session silently overwrites the existing
    // session's metadata in the checkpoint.
    if opts.Review && isExistingCheckpoint {
        exists, readErr := checkpointHasSessionMetadata(ctx, store, checkpointID, sessionID)
        exists, readErr := checkpointHasSessionMetadata(ctx, repo, refs, checkpointID, sessionID)
        if readErr != nil {
            return fmt.Errorf("failed to check checkpoint %s for session %s: %w", checkpointID.String(), sessionID, readErr)
        }
    72 unmodified lines

return nil
}

func checkpointHasSessionMetadata(ctx context.Context, store *cpkg.GitStore, checkpointID id.CheckpointID, sessionID string) (bool, error) {
// checkpointHasSessionMetadata reports whether sessionID has existing metadata
// at Primary. Reads target Primary directly, not refs.Read, because this guard
// must reflect what the next write would target.
func checkpointHasSessionMetadata(ctx context.Context, repo *git.Repository, refs cpkg.CommittedRefs, checkpointID id.CheckpointID, sessionID string) (bool, error) {
    primaryRefs := refs
    primaryRefs.Read = refs.Primary
    store := cpkg.NewGitStore(repo, primaryRefs)
summary, err := store.ReadCommitted(ctx, checkpointID)
if err != nil {
    return false, fmt.Errorf("read checkpoint summary: %w", err)
}
85 unmodified lines

return repo, err
}

// checkpointPresentLocally reports whether the checkpoint already exists on
// the local primary ref we would write to. Remote-tracking alone is not
// enough; see ensureCheckpointAvailable. Reads target Primary directly even
// when the configured Read ref differs (e.g. v1.1 mirror) because the
// question is "is the write target up to date," not "what's visible to
// readers."
// checkpointPresentLocally reports whether the checkpoint already exists at
// Primary locally. Reads target Primary directly, not refs.Read, because this
// asks what the next write would find, not what readers see. A missing local
// ref is reported as absent; the caller is responsible for any remote refresh.
func checkpointPresentLocally(ctx context.Context, repo *git.Repository, refs cpkg.CommittedRefs, checkpointID id.CheckpointID) (bool, error) {
    if _, err := repo.Reference(refs.Primary, true); err != nil {
        // Local ref doesn't exist — treat as "not present locally". We
        // deliberately do not fall back to remote-tracking: see
        // ensureCheckpointAvailable's docstring.
        return false, nil //nolint:nilerr // Missing ref is the "absent" signal, not an error.
    }
    primaryRefs := refs

Mcmd/entire/cli/attach.go+12/-11

1780 unmodified lines

1781
1782
1783
1784
1785
1786
1787
1784
1785
1786
1787
1788
1789

1780 unmodified lines

return NewFetchingTree(ctx, tree, s.repo.Storer, s.blobFetcher), nil
}

// getSessionsBranchTree returns the tree object at the configured read ref.
// Falls back to origin's remote-tracking ref when reads are bootstrappable
// from origin. When reads target a local-only mirror, the fallback skips
// because origin doesn't track the mirror.
// getSessionsBranchTree returns the tree object at refs.Read. Falls back to
// origin's remote-tracking ref for Primary when ReadBootstrappableFromOrigin
// is true.
func (s *GitStore) getSessionsBranchTree() (*object.Tree, error) {
    ref, err := s.repo.Reference(s.refs.Read, true)
    if err != nil {

Mcmd/entire/cli/checkpoint/committed.go+3/-4

41 unmodified lines

42
43
44
45
46
47
48
45
46
47
48
49

41 unmodified lines

return r.Primary.IsBranch() && slices.Contains(r.Push, r.Primary)
}

// ReadBootstrappableFromOrigin reports whether the read ref can be bootstrapped
// from origin. True when reads target Primary AND Primary is fetchable from
// origin. False when reads target a local-only mirror (origin doesn't track
// it), even if Primary is itself fetchable.
// ReadBootstrappableFromOrigin reports whether reads can be bootstrapped from
// origin: true when reads target Primary and Primary is fetchable from origin.
func (r CommittedRefs) ReadBootstrappableFromOrigin() bool {
    return r.Read == r.Primary && r.PrimaryFetchableFromOrigin()
}

Mcmd/entire/cli/checkpoint/committed_refs.go+2/-4