From 3cb17a1517d378fea48fab597ce1911fe8c8bea2 Mon Sep 17 00:00:00 2001 From: Muskan Paliwal Date: Fri, 11 Sep 2026 11:10:08 +0530 Subject: [PATCH 1/6] fix(cleanup): preserve sessions when cleanup safety is uncertain Entire-Checkpoint: 01M27FNYWAG56R5H9CV0BCZSZK --- CLAUDE.md | 2 +- cmd/entire/cli/doctor.go | 2 +- .../cli/doctor_cleanup_inventory_test.go | 32 ++++ cmd/entire/cli/gitrepo/ref_absence.go | 41 +++++ cmd/entire/cli/gitrepo/ref_absence_test.go | 47 +++++ cmd/entire/cli/gitrepo/ref_cas.go | 25 +-- cmd/entire/cli/session/state.go | 42 ++++- .../cli/session/state_retention_test.go | 80 +++++++++ cmd/entire/cli/strategy/cleanup.go | 4 +- .../cli/strategy/cleanup_inventory_test.go | 164 ++++++++++++++++++ .../cli/strategy/manual_commit_reset.go | 12 +- .../cli/strategy/manual_commit_session.go | 11 +- cmd/entire/cli/strategy/session_state.go | 14 ++ cmd/entire/cli/strategy/session_state_test.go | 2 + 14 files changed, 435 insertions(+), 43 deletions(-) create mode 100644 cmd/entire/cli/doctor_cleanup_inventory_test.go create mode 100644 cmd/entire/cli/gitrepo/ref_absence.go create mode 100644 cmd/entire/cli/gitrepo/ref_absence_test.go create mode 100644 cmd/entire/cli/session/state_retention_test.go create mode 100644 cmd/entire/cli/strategy/cleanup_inventory_test.go diff --git a/CLAUDE.md b/CLAUDE.md index 0b762c458d..ef3c27ec59 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -1908,7 +1908,7 @@ The manual-commit strategy (`manual_commit*.go`) does not modify the active bran - `manual_commit_opf_rewrite.go` - Pre-push OPF re-redaction: walks unpushed v1 commits, runs OPF over their blobs, rebuilds commits with `Entire-OPF-Applied: true` trailer, CAS-updates the local ref. Sentinel error types (use `errors.As`): `V1DivergedError`, `BootstrapTooLargeError`, `V1RefMovedError`, `OPFRuntimeFailedError`, `OPFBatchTooLargeError`, `OPFRawBytesTooLargeError`, `OPFNoCategoriesError` (OPF enabled with zero effective categories — the pre-push decision and the rewrite both fail closed instead of stamping the trailer without a scan). - `manual_commit_opf_refs.go` - The git-refs half of pre-push OPF: `RewriteQueuedCheckpointRefsWithOPF` walks the push queue and rewrites every unpushed commit on each queued ref, reusing the v1 rewrite's blob walk, caps, and error types. See the OPF bullet above for why the two backends fail closed differently. - `settings/opf_command_trust.go` - Ownership gate for the executed OPF `command` (local-only + untracked); see the OPF trust-boundary bullet above -- `cleanup.go` - Cleanup discovery/deletion for shadow branches, session states, and checkpoint metadata +- `cleanup.go` - Cleanup discovery/deletion for shadow branches, session states, and checkpoint metadata. Destructive protection scans use `StateStore.ListStrict` (via `ListSessionStatesStrict` where needed); unreadable state cannot authorize deletion. Session expiry retains pending checkpoint content, and orphan cleanup requires `gitrepo.ReferenceIsAbsent` to distinguish unreadable loose refs from absent refs. - `session_state.go` - Package-level session state functions - `hooks.go` - Git hook installation diff --git a/cmd/entire/cli/doctor.go b/cmd/entire/cli/doctor.go index 2b4e973778..299a3fdd86 100644 --- a/cmd/entire/cli/doctor.go +++ b/cmd/entire/cli/doctor.go @@ -1473,7 +1473,7 @@ func writeCodexTrackedHooksRemedy(w io.Writer) { // canDeleteShadowBranch checks if a shadow branch can be safely deleted. // Returns true if no other sessions (besides excludeSessionID) need this branch. func canDeleteShadowBranch(ctx context.Context, shadowBranch, excludeSessionID string) (bool, error) { - states, err := strategy.ListSessionStates(ctx) + states, err := strategy.ListSessionStatesStrict(ctx) if err != nil { return false, fmt.Errorf("failed to list session states: %w", err) } diff --git a/cmd/entire/cli/doctor_cleanup_inventory_test.go b/cmd/entire/cli/doctor_cleanup_inventory_test.go new file mode 100644 index 0000000000..a4ad986f50 --- /dev/null +++ b/cmd/entire/cli/doctor_cleanup_inventory_test.go @@ -0,0 +1,32 @@ +package cli + +import ( + "os" + "path/filepath" + "testing" + "time" + + "github.com/entireio/cli/cmd/entire/cli/checkpoint" + "github.com/entireio/cli/cmd/entire/cli/session" + "github.com/entireio/cli/cmd/entire/cli/strategy" + "github.com/entireio/cli/cmd/entire/cli/testutil" + "github.com/stretchr/testify/require" +) + +func TestCanDeleteShadowBranch_RejectsIncompleteInventory(t *testing.T) { + dir := t.TempDir() + testutil.InitRepo(t, dir) + t.Chdir(dir) + state := &session.State{SessionID: "protected", BaseCommit: "abc123456789", StartedAt: time.Now(), StepCount: 1} + require.NoError(t, strategy.SaveSessionState(t.Context(), state)) + name := checkpoint.ShadowBranchNameForCommit(state.BaseCommit, "") + file := filepath.Join(dir, ".git", session.SessionStateDirName, "protected.json") + require.NoError(t, os.WriteFile(file, []byte(`{"session_id":`), 0o600)) + allowed, err := canDeleteShadowBranch(t.Context(), name, "other") + require.Error(t, err) + require.False(t, allowed) + require.NoError(t, strategy.SaveSessionState(t.Context(), state)) + allowed, err = canDeleteShadowBranch(t.Context(), name, "other") + require.NoError(t, err) + require.False(t, allowed) +} diff --git a/cmd/entire/cli/gitrepo/ref_absence.go b/cmd/entire/cli/gitrepo/ref_absence.go new file mode 100644 index 0000000000..3e4287f3e1 --- /dev/null +++ b/cmd/entire/cli/gitrepo/ref_absence.go @@ -0,0 +1,41 @@ +package gitrepo + +import ( + "errors" + "fmt" + "os" + + "github.com/go-git/go-git/v6" + "github.com/go-git/go-git/v6/plumbing" + gitfilesystem "github.com/go-git/go-git/v6/storage/filesystem" +) + +// ReferenceIsAbsent requires both loose storage and the reference reader to +// establish absence. A read failure must not authorize destructive cleanup. +func ReferenceIsAbsent(repo *git.Repository, refName plumbing.ReferenceName) (bool, error) { + if err := refName.Validate(); err != nil { + return false, fmt.Errorf("validate reference: %w", err) + } + // go-git falls back to packed refs after any loose-ref read error, so its + // not-found result alone cannot distinguish a directory from absence. + if storage, ok := repo.Storer.(*gitfilesystem.Storage); ok { + info, statErr := storage.Filesystem().Lstat(refName.String()) + if statErr == nil { + if info.IsDir() { + return false, fmt.Errorf("ref %s is a directory", refName) + } + return false, nil + } + if !errors.Is(statErr, os.ErrNotExist) { + return false, fmt.Errorf("inspect ref %s: %w", refName, statErr) + } + } + _, err := repo.Reference(refName, false) + if errors.Is(err, plumbing.ErrReferenceNotFound) { + return true, nil + } + if err != nil { + return false, fmt.Errorf("read ref %s: %w", refName, err) + } + return false, nil +} diff --git a/cmd/entire/cli/gitrepo/ref_absence_test.go b/cmd/entire/cli/gitrepo/ref_absence_test.go new file mode 100644 index 0000000000..796556c437 --- /dev/null +++ b/cmd/entire/cli/gitrepo/ref_absence_test.go @@ -0,0 +1,47 @@ +package gitrepo + +import ( + "os" + "path/filepath" + "testing" + + "github.com/entireio/cli/cmd/entire/cli/testutil/gitenv" + "github.com/go-git/go-git/v6/plumbing" + "github.com/stretchr/testify/require" +) + +func TestReferenceIsAbsent(t *testing.T) { + t.Parallel() + for _, backend := range refCASBackends() { + t.Run(backend.name, func(t *testing.T) { + t.Parallel() + dir, _, _ := backend.init(t) + repo, err := OpenPath(dir) + require.NoError(t, err) + defer repo.Close() + absent, err := ReferenceIsAbsent(repo, plumbing.NewBranchReferenceName("main")) + require.NoError(t, err) + require.False(t, absent) + absent, err = ReferenceIsAbsent(repo, plumbing.NewBranchReferenceName("missing")) + require.NoError(t, err) + require.True(t, absent) + gitenv.Run(t, dir, "pack-refs", "--all") + absent, err = ReferenceIsAbsent(repo, plumbing.NewBranchReferenceName("main")) + require.NoError(t, err) + require.False(t, absent) + }) + } +} + +func TestReferenceIsAbsent_RefDirectoryIsNotAbsence(t *testing.T) { + t.Parallel() + dir, _, _ := initFilesRefCASRepo(t) + repo, err := OpenPath(dir) + require.NoError(t, err) + defer repo.Close() + ref := plumbing.NewBranchReferenceName("broken") + require.NoError(t, os.Mkdir(filepath.Join(dir, ".git", ref.String()), 0o700)) + absent, err := ReferenceIsAbsent(repo, ref) + require.Error(t, err) + require.False(t, absent) +} diff --git a/cmd/entire/cli/gitrepo/ref_cas.go b/cmd/entire/cli/gitrepo/ref_cas.go index 76ce35e60a..e17a5f72c4 100644 --- a/cmd/entire/cli/gitrepo/ref_cas.go +++ b/cmd/entire/cli/gitrepo/ref_cas.go @@ -7,14 +7,12 @@ import ( "errors" "fmt" "io" - "os" "os/exec" "strings" "sync/atomic" "time" "github.com/go-git/go-git/v6/plumbing" - gitfilesystem "github.com/go-git/go-git/v6/storage/filesystem" ) const refCASWaitDelay = 3 * time.Second @@ -234,28 +232,7 @@ func refIsAbsent(repoRoot string, refName plumbing.ReferenceName) (bool, error) return false, fmt.Errorf("open repository to verify missing ref: %w", err) } defer repo.Close() - // go-git falls back to packed refs after any loose-ref read error, so its - // not-found result alone cannot distinguish a directory from absence. - if storage, ok := repo.Storer.(*gitfilesystem.Storage); ok { - info, statErr := storage.Filesystem().Lstat(refName.String()) - if statErr == nil { - if info.IsDir() { - return false, fmt.Errorf("ref %s is a directory", refName) - } - return false, nil - } - if !errors.Is(statErr, os.ErrNotExist) { - return false, fmt.Errorf("inspect ref %s: %w", refName, statErr) - } - } - _, err = repo.Reference(refName, false) - if errors.Is(err, plumbing.ErrReferenceNotFound) { - return true, nil - } - if err != nil { - return false, fmt.Errorf("read ref %s: %w", refName, err) - } - return false, nil + return ReferenceIsAbsent(repo, refName) } func symbolicRefTarget(ctx context.Context, repoRoot string, refName plumbing.ReferenceName) (string, bool, error) { diff --git a/cmd/entire/cli/session/state.go b/cmd/entire/cli/session/state.go index 16a3930a21..f10e5b6994 100644 --- a/cmd/entire/cli/session/state.go +++ b/cmd/entire/cli/session/state.go @@ -787,6 +787,11 @@ func (s *State) IsEnded() bool { return s.Phase == PhaseEnded || s.EndedAt != nil } +func (s *State) hasPendingCheckpointContent() bool { + return (!s.FullyCondensed && (s.StepCount > 0 || len(s.FilesTouched) > 0)) || + s.HasTaskContent() || len(s.TurnCheckpointIDs) > 0 || s.CondensationAttempt != nil +} + func (s *State) IsStale() bool { // Imported sessions are historical, read-only records reconstructed from // pre-existing transcripts; their timestamps are always old by nature. @@ -947,8 +952,8 @@ func NewStateStoreWithDir(stateDir string) *StateStore { } // Load loads the session state for the given session ID. -// Returns (nil, nil) when session file doesn't exist or session is stale (not an error condition). -// Stale sessions (ended longer than StaleSessionThreshold ago) are automatically deleted. +// Returns (nil, nil) for missing files or expired records with no pending content. +// Stale sessions are deleted only when they have no pending checkpoint content. func (s *StateStore) Load(ctx context.Context, sessionID string) (*State, error) { // Validate session ID to prevent path traversal if err := validation.ValidateSessionID(sessionID); err != nil { @@ -979,7 +984,7 @@ func (s *StateStore) Load(ctx context.Context, sessionID string) (*State, error) } state.NormalizeAfterLoad(ctx) - if state.IsStale() { + if state.IsStale() && !state.hasPendingCheckpointContent() { logCtx := logging.WithComponent(ctx, "session") logging.Debug(logCtx, "deleting stale session state", slog.String("session_id", sessionID), @@ -1083,8 +1088,21 @@ func (s *StateStore) RemoveAll() error { return nil } -// List returns all session states. +// List returns readable session states, skipping individual load failures. func (s *StateStore) List(ctx context.Context) ([]*State, error) { + return s.list(ctx, false) +} + +// ListStrict refuses an incomplete inventory. Destructive consumers must use +// this instead of treating an unreadable session as absent. +func (s *StateStore) ListStrict(ctx context.Context) ([]*State, error) { + return s.list(ctx, true) +} + +func (s *StateStore) list(ctx context.Context, strict bool) ([]*State, error) { + if err := ctx.Err(); err != nil { + return nil, fmt.Errorf("list session states: %w", err) + } root, err := s.dirRoot() if err != nil { return nil, fmt.Errorf("failed to open session state directory: %w", err) @@ -1099,7 +1117,16 @@ func (s *StateStore) List(ctx context.Context) ([]*State, error) { var states []*State for _, entry := range entries { - if entry.IsDir() || !strings.HasSuffix(entry.Name(), ".json") { + if err := ctx.Err(); err != nil { + return nil, fmt.Errorf("list session states: %w", err) + } + if !strings.HasSuffix(entry.Name(), ".json") { + continue + } + if entry.IsDir() { + if strict { + return nil, fmt.Errorf("session state %s is a directory", entry.Name()) + } continue } if strings.HasSuffix(entry.Name(), ".tmp") { @@ -1109,7 +1136,10 @@ func (s *StateStore) List(ctx context.Context) ([]*State, error) { sessionID := strings.TrimSuffix(entry.Name(), ".json") state, err := s.Load(ctx, sessionID) if err != nil { - continue // Skip corrupted state files + if strict { + return nil, fmt.Errorf("load session %s: %w", sessionID, err) + } + continue } if state == nil { continue // Not found or stale (Load handles cleanup) diff --git a/cmd/entire/cli/session/state_retention_test.go b/cmd/entire/cli/session/state_retention_test.go new file mode 100644 index 0000000000..adb5d38f09 --- /dev/null +++ b/cmd/entire/cli/session/state_retention_test.go @@ -0,0 +1,80 @@ +package session + +import ( + "os" + "path/filepath" + "testing" + "time" + + "github.com/stretchr/testify/require" +) + +func TestStateStore_Load_PreservesPendingContentAfterExpiry(t *testing.T) { + t.Parallel() + cases := []struct { + name string + state State + retained bool + }{ + {name: "steps", state: State{StepCount: 1}, retained: true}, + {name: "files", state: State{FilesTouched: []string{"main.go"}}, retained: true}, + {name: "tasks", state: State{TaskRecords: []TaskRecord{{}}}, retained: true}, + {name: "pending-turn", state: State{FullyCondensed: true, TurnCheckpointIDs: []string{"a1b2c3d4e5f6"}}, retained: true}, + {name: "attempt", state: State{CondensationAttempt: &CondensationAttempt{}}, retained: true}, + {name: "empty", state: State{}}, + {name: "condensed", state: State{FullyCondensed: true, StepCount: 1, FilesTouched: []string{"main.go"}}}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + dir := filepath.Join(t.TempDir(), SessionStateDirName) + store := NewStateStoreWithDir(dir) + tc.state.SessionID = "expired" + tc.state.StartedAt = time.Now().Add(-8 * 24 * time.Hour) + require.NoError(t, store.Save(t.Context(), &tc.state)) + state, err := store.Load(t.Context(), tc.state.SessionID) + require.NoError(t, err) + if tc.retained { + require.NotNil(t, state) + require.FileExists(t, filepath.Join(dir, "expired.json")) + } else { + require.Nil(t, state) + require.NoFileExists(t, filepath.Join(dir, "expired.json")) + } + }) + } +} + +func TestStateStore_ListStrict_RefusesIncompleteInventory(t *testing.T) { + t.Parallel() + for _, directory := range []bool{false, true} { + name := "malformed" + if directory { + name = "directory" + } + t.Run(name, func(t *testing.T) { + t.Parallel() + ctx := t.Context() + dir := filepath.Join(t.TempDir(), SessionStateDirName) + store := NewStateStoreWithDir(dir) + require.NoError(t, store.Save(ctx, &State{SessionID: "valid", StartedAt: time.Now()})) + bad := filepath.Join(dir, "unreadable.json") + if directory { + require.NoError(t, os.Mkdir(bad, 0o700)) + } else { + require.NoError(t, os.WriteFile(bad, []byte(`{"session_id":`), 0o600)) + } + states, err := store.List(ctx) + require.NoError(t, err) + require.Len(t, states, 1) + states, err = store.ListStrict(ctx) + require.ErrorContains(t, err, "unreadable") + require.Nil(t, states) + require.FileExists(t, filepath.Join(dir, "valid.json")) + require.NoError(t, os.Remove(bad)) + states, err = store.ListStrict(ctx) + require.NoError(t, err) + require.Len(t, states, 1) + }) + } +} diff --git a/cmd/entire/cli/strategy/cleanup.go b/cmd/entire/cli/strategy/cleanup.go index 47cace09c8..d35815032f 100644 --- a/cmd/entire/cli/strategy/cleanup.go +++ b/cmd/entire/cli/strategy/cleanup.go @@ -230,7 +230,7 @@ func CleanupPushedShadowBranches(ctx context.Context) (int, error) { return 0, nil } - states, err := ListSessionStates(ctx) + states, err := ListSessionStatesStrict(ctx) if err != nil { return 0, fmt.Errorf("list session states: %w", err) } @@ -333,7 +333,7 @@ func protectedShadowBranchForSession(s *SessionState) (string, bool) { } func shadowBranchProtectedByCurrentState(ctx context.Context, branch string) (bool, error) { - states, err := ListSessionStates(ctx) + states, err := ListSessionStatesStrict(ctx) if err != nil { return false, err } diff --git a/cmd/entire/cli/strategy/cleanup_inventory_test.go b/cmd/entire/cli/strategy/cleanup_inventory_test.go new file mode 100644 index 0000000000..6d0220cf45 --- /dev/null +++ b/cmd/entire/cli/strategy/cleanup_inventory_test.go @@ -0,0 +1,164 @@ +package strategy + +import ( + "io" + "os" + "os/exec" + "path/filepath" + "strings" + "testing" + "time" + + "github.com/entireio/cli/cmd/entire/cli/checkpoint" + "github.com/entireio/cli/cmd/entire/cli/session" + "github.com/go-git/go-git/v6/plumbing" + "github.com/stretchr/testify/require" +) + +func newShadowOnlyCommit(t *testing.T, env *shadowCleanupEnv, shadow string) plumbing.Hash { + t.Helper() + cmd := exec.CommandContext(t.Context(), "git", "mktree") + output, treeErr := cmd.CombinedOutput() + require.NoError(t, treeErr, string(output)) + hash, err := checkpoint.CreateCommit(t.Context(), env.repo, plumbing.NewHash("4b825dc642cb6eb9a060e54bf8d69288fbee4904"), env.baseHash, "shadow-only history", "test", "test@test.com") + require.NoError(t, err) + require.NoError(t, env.repo.Storer.SetReference(plumbing.NewHashReference(plumbing.NewBranchReferenceName(shadow), hash))) + return hash +} + +func requireShadowReachable(t *testing.T, hash plumbing.Hash) { + t.Helper() + cmd := exec.CommandContext(t.Context(), "git", "fsck", "--unreachable", "--no-reflogs") + output, err := cmd.CombinedOutput() + require.NoError(t, err, string(output)) + require.NotContains(t, string(output), "unreachable commit "+hash.String()) + t.Log("shadow commit remains reachable:", hash.String()) +} + +func TestCleanupPushedShadowBranches_PreservesExpiredUncondensed(t *testing.T) { + for _, phase := range []session.Phase{session.PhaseActive, session.PhaseEnded} { + t.Run(string(phase), func(t *testing.T) { + env := newShadowCleanupEnv(t) + ctx := t.Context() + shadow := env.addShadowBranch(env.baseHash.String(), "") + hash := newShadowOnlyCommit(t, env, shadow) + recent := time.Now().Add(-time.Hour) + state := &SessionState{SessionID: "pending-session", BaseCommit: env.baseHash.String(), StartedAt: recent, LastInteractionTime: &recent, Phase: phase, StepCount: 1, FullyCondensed: false} + if phase == session.PhaseEnded { + state.EndedAt = &recent + } + require.NoError(t, SaveSessionState(ctx, state)) + deleted, err := CleanupPushedShadowBranches(ctx) + require.NoError(t, err) + require.Zero(t, deleted) + require.True(t, env.branchExists(shadow)) + old := time.Now().Add(-8 * 24 * time.Hour) + state.StartedAt = old + state.LastInteractionTime = &old + if phase == session.PhaseEnded { + state.EndedAt = &old + } + require.NoError(t, SaveSessionState(ctx, state)) + deleted, err = CleanupPushedShadowBranches(ctx) + require.NoError(t, err) + require.Zero(t, deleted) + require.True(t, env.branchExists(shadow)) + _, err = os.Stat(filepath.Join(env.dir, ".git", "entire-sessions", state.SessionID+".json")) + require.NoError(t, err) + t.Logf("phase=%s control_deleted=0 aged_deleted=%d state_preserved=true", phase, deleted) + requireShadowReachable(t, hash) + }) + } +} + +func TestResetSession_PreservesCorruptSiblingShadow(t *testing.T) { + for _, corrupt := range []bool{false, true} { + name := "valid" + if corrupt { + name = "corrupt" + } + t.Run(name, func(t *testing.T) { + env := newShadowCleanupEnv(t) + ctx := t.Context() + shadow := env.addShadowBranch(env.baseHash.String(), "") + hash := newShadowOnlyCommit(t, env, shadow) + for _, id := range []string{"reset-me", "keep-me"} { + require.NoError(t, SaveSessionState(ctx, &SessionState{SessionID: id, BaseCommit: env.baseHash.String(), StartedAt: time.Now(), Phase: session.PhaseActive, StepCount: 1})) + } + sibling := filepath.Join(env.dir, ".git", "entire-sessions", "keep-me.json") + if corrupt { + require.NoError(t, os.WriteFile(sibling, []byte(`{"session_id":`), 0600)) + } + var output strings.Builder + resetErr := NewManualCommitStrategy().ResetSession(ctx, &output, io.Discard, "reset-me") + if corrupt { + require.Error(t, resetErr) + } else { + require.NoError(t, resetErr) + } + require.True(t, env.branchExists(shadow)) + _, err := os.Stat(sibling) + require.NoError(t, err) + t.Logf("corrupt=%v shared_branch_exists=%v sibling_state_exists=true output=%q", corrupt, env.branchExists(shadow), output.String()) + requireShadowReachable(t, hash) + if corrupt { + require.NoError(t, SaveSessionState(ctx, &SessionState{SessionID: "keep-me", BaseCommit: env.baseHash.String(), StartedAt: time.Now(), Phase: session.PhaseActive, StepCount: 1})) + require.NoError(t, NewManualCommitStrategy().ResetSession(ctx, &output, io.Discard, "keep-me")) + require.False(t, env.branchExists(shadow)) + } + }) + } +} + +func TestListAllSessionStates_PreservesUnreadableShadow(t *testing.T) { + env := newShadowCleanupEnv(t) + ctx := t.Context() + shadow := env.addShadowBranch(env.baseHash.String(), "") + hash := newShadowOnlyCommit(t, env, shadow) + state := &SessionState{SessionID: "idle-pending", BaseCommit: env.baseHash.String(), StartedAt: time.Now(), Phase: session.PhaseIdle, StepCount: 1} + require.NoError(t, SaveSessionState(ctx, state)) + strat := NewManualCommitStrategy() + states, err := strat.listAllSessionStates(ctx) + require.NoError(t, err) + require.Len(t, states, 1) + refPath := filepath.Join(env.dir, ".git", "refs", "heads", shadow) + require.NoError(t, os.Chmod(refPath, 0000)) + t.Cleanup(func() { require.NoError(t, os.Chmod(refPath, 0o600)) }) + _, rawErr := os.ReadFile(refPath) + if !os.IsPermission(rawErr) { + t.Skip("filesystem does not enforce unreadable file permissions") + } + _, readErr := env.repo.Reference(plumbing.NewBranchReferenceName(shadow), true) + require.Error(t, readErr) + require.ErrorIs(t, readErr, plumbing.ErrReferenceNotFound) + states, err = strat.listAllSessionStates(ctx) + require.Error(t, err) + require.Nil(t, states) + _, statErr := os.Stat(filepath.Join(env.dir, ".git", "entire-sessions", state.SessionID+".json")) + require.NoError(t, statErr) + t.Logf("ref_error=%v list_error=%v state_preserved=true", readErr, err) + require.NoError(t, os.Chmod(refPath, 0600)) + deleted, err := CleanupPushedShadowBranches(ctx) + require.NoError(t, err) + require.Zero(t, deleted) + requireShadowReachable(t, hash) +} + +func TestCleanupPushedShadowBranches_PreservesCorruptState(t *testing.T) { + env := newShadowCleanupEnv(t) + ctx := t.Context() + shadow := env.addShadowBranch(env.baseHash.String(), "") + hash := newShadowOnlyCommit(t, env, shadow) + env.addSessionState("pending", env.baseHash.String(), "", nil, nil, false) + path := filepath.Join(env.dir, ".git", "entire-sessions", "pending.json") + require.NoError(t, os.WriteFile(path, []byte(`{"session_id":`), 0o600)) + deleted, err := CleanupPushedShadowBranches(ctx) + require.Error(t, err) + require.Zero(t, deleted) + require.True(t, env.branchExists(shadow)) + requireShadowReachable(t, hash) + env.addSessionState("pending", env.baseHash.String(), "", nil, nil, false) + deleted, err = CleanupPushedShadowBranches(ctx) + require.NoError(t, err) + require.Zero(t, deleted) +} diff --git a/cmd/entire/cli/strategy/manual_commit_reset.go b/cmd/entire/cli/strategy/manual_commit_reset.go index 9fb5d35612..30f63e9d77 100644 --- a/cmd/entire/cli/strategy/manual_commit_reset.go +++ b/cmd/entire/cli/strategy/manual_commit_reset.go @@ -118,13 +118,11 @@ func (s *ManualCommitStrategy) ResetSession(ctx context.Context, w, errW io.Writ // Clean up shadow branch if no other sessions need it if err := s.cleanupShadowBranchIfUnused(ctx, repo, shadowBranchName, sessionID); err != nil { - fmt.Fprintf(errW, "Warning: failed to clean up shadow branch %s: %v\n", shadowBranchName, err) - } else { - // Check if it was actually deleted via git CLI (go-git's cache - // may be stale after CLI-based deletion with packed refs) - if err := branchExistsCLI(ctx, shadowBranchName); err != nil { - fmt.Fprintf(w, "✓ Deleted shadow branch %s\n", shadowBranchName) - } + return fmt.Errorf("failed to clean up shadow branch %s: %w", shadowBranchName, err) + } + // go-git's cached refs may be stale after CLI deletion. + if err := branchExistsCLI(ctx, shadowBranchName); err != nil { + fmt.Fprintf(w, "✓ Deleted shadow branch %s\n", shadowBranchName) } return nil diff --git a/cmd/entire/cli/strategy/manual_commit_session.go b/cmd/entire/cli/strategy/manual_commit_session.go index cc6188a6c9..5e04710dba 100644 --- a/cmd/entire/cli/strategy/manual_commit_session.go +++ b/cmd/entire/cli/strategy/manual_commit_session.go @@ -112,7 +112,7 @@ func (s *ManualCommitStrategy) listAllSessionStates(ctx context.Context) ([]*Ses return nil, fmt.Errorf("failed to get state store: %w", err) } - sessionStates, err := store.List(ctx) + sessionStates, err := store.ListStrict(ctx) if err != nil { return nil, fmt.Errorf("failed to list session states: %w", err) } @@ -154,7 +154,14 @@ func (s *ManualCommitStrategy) listAllSessionStates(ctx context.Context) ([]*Ses // Record-bearing sessions hold condensable content off the shadow branch — never orphaned. shadowBranch := getShadowBranchNameForCommit(state.BaseCommit, state.WorktreeID) refName := plumbing.NewBranchReferenceName(shadowBranch) - if _, err := repo.Reference(refName, true); err != nil { + if _, refErr := repo.Reference(refName, true); refErr != nil { + absent, err := gitrepo.ReferenceIsAbsent(repo, refName) + if err != nil { + return nil, fmt.Errorf("verify shadow branch %s: %w", shadowBranch, err) + } + if !absent { + return nil, fmt.Errorf("shadow branch %s is present but could not be read: %w", shadowBranch, refErr) + } if !state.Phase.IsActive() && state.LastCheckpointID.IsEmpty() && !state.HasTaskContent() { //nolint:errcheck,gosec // G104: Cleanup is best-effort, shouldn't fail the list operation store.Clear(ctx, state.SessionID) diff --git a/cmd/entire/cli/strategy/session_state.go b/cmd/entire/cli/strategy/session_state.go index 8c5c07d485..95b78c58e1 100644 --- a/cmd/entire/cli/strategy/session_state.go +++ b/cmd/entire/cli/strategy/session_state.go @@ -173,6 +173,20 @@ func ListSessionStates(ctx context.Context) ([]*SessionState, error) { return states, nil } +// ListSessionStatesStrict returns a complete inventory or an error. +// Use it when an absent session would permit deleting checkpoint data. +func ListSessionStatesStrict(ctx context.Context) ([]*SessionState, error) { + store, err := session.NewStateStore(ctx) + if err != nil { + return nil, fmt.Errorf("failed to create state store: %w", err) + } + states, err := store.ListStrict(ctx) + if err != nil { + return nil, fmt.Errorf("failed to list session states: %w", err) + } + return states, nil +} + // FindMostRecentSessionInCurrentWorktree returns the most recently interacted // session from the current worktree only, never falling back to another // worktree's. diff --git a/cmd/entire/cli/strategy/session_state_test.go b/cmd/entire/cli/strategy/session_state_test.go index 25132e8ebb..5bd1b57f39 100644 --- a/cmd/entire/cli/strategy/session_state_test.go +++ b/cmd/entire/cli/strategy/session_state_test.go @@ -490,6 +490,8 @@ func TestLoadSessionState_DeletesStaleSession(t *testing.T) { staleInteracted := time.Now().Add(-2 * 7 * 24 * time.Hour) state := &SessionState{ SessionID: "stale-load-test", + FullyCondensed: true, + Phase: session.PhaseEnded, BaseCommit: "abc123def456", StartedAt: time.Now().Add(-3 * 7 * 24 * time.Hour), LastInteractionTime: &staleInteracted, From 80aa5afde2884535ab07c8a1ff1962fe4e0eeb9c Mon Sep 17 00:00:00 2001 From: Muskan Paliwal Date: Fri, 11 Sep 2026 15:10:39 +0530 Subject: [PATCH 2/6] fix(cleanup): preserve safety at destructive boundaries Entire-Checkpoint: 01M27XEBHFT7E8K000W92QZ05E --- cmd/entire/cli/strategy/cleanup.go | 24 ++++++++++++------- .../cli/strategy/cleanup_inventory_test.go | 13 ++++++---- .../strategy/cleanup_pushed_shadow_test.go | 24 +++++++++++++++++-- .../strategy/manual_commit_condensation.go | 3 +-- .../cli/strategy/manual_commit_reset.go | 11 +++++---- .../cli/strategy/manual_commit_session.go | 2 +- .../cli/strategy/session_identity_test.go | 16 +++++++++++++ 7 files changed, 71 insertions(+), 22 deletions(-) diff --git a/cmd/entire/cli/strategy/cleanup.go b/cmd/entire/cli/strategy/cleanup.go index d35815032f..6f51875d75 100644 --- a/cmd/entire/cli/strategy/cleanup.go +++ b/cmd/entire/cli/strategy/cleanup.go @@ -2,6 +2,7 @@ package strategy import ( "context" + "errors" "fmt" "log/slog" "os" @@ -206,9 +207,9 @@ func listShadowBranchHeads(ctx context.Context) (map[string]plumbing.Hash, error // so the caller knows any condensed checkpoint data already reached // the remote. // -// Returns the count of branches deleted. Failures (e.g., one branch -// fails to delete due to a stale lock) are logged but don't abort -// the operation — remaining branches are still attempted. +// Returns the count of branches deleted. Expected compare-and-swap or deletion +// failures are logged without aborting the operation. Failures to establish +// whether current session state protects a branch are returned to the caller. // // Safety properties: // - Skips any shadow branch referenced by a session with EndedAt @@ -264,13 +265,16 @@ func CleanupPushedShadowBranches(ctx context.Context) (int, error) { return 0, nil } - deleted, failed := DeleteShadowBranchesIfUnchanged(ctx, toDelete) + deleted, failed, safetyErr := DeleteShadowBranchesIfUnchanged(ctx, toDelete) if len(failed) > 0 { logging.Warn(ctx, "some shadow branches failed to delete during post-push cleanup", slog.Int("failed_count", len(failed)), slog.Int("deleted_count", len(deleted)), ) } + if safetyErr != nil { + return len(deleted), safetyErr + } return len(deleted), nil } @@ -282,11 +286,14 @@ func CleanupPushedShadowBranches(ctx context.Context) (int, error) { // already have filtered to isAutoDeletableShadowBranch before calling this -- // the same check is repeated here as a second, independent gate rather than // relying solely on the caller's filtering, since this function is the one -// place that actually deletes a ref with no human confirmation. -func DeleteShadowBranchesIfUnchanged(ctx context.Context, branches map[string]plumbing.Hash) (deleted []string, failed []string) { +// place that actually deletes a ref with no human confirmation. The returned +// error is reserved for failures to establish that deletion is safe; expected +// compare-and-swap and deletion failures are reported only in failed. +func DeleteShadowBranchesIfUnchanged(ctx context.Context, branches map[string]plumbing.Hash) (deleted []string, failed []string, safetyErr error) { if len(branches) == 0 { - return []string{}, []string{} + return []string{}, []string{}, nil } + var safetyErrors []error for branch, expected := range branches { if !isAutoDeletableShadowBranch(branch) || expected.IsZero() { failed = append(failed, branch) @@ -298,6 +305,7 @@ func DeleteShadowBranchesIfUnchanged(ctx context.Context, branches map[string]pl slog.String("branch", branch), slog.String("error", err.Error()), ) + safetyErrors = append(safetyErrors, fmt.Errorf("recheck protection for shadow branch %s: %w", branch, err)) failed = append(failed, branch) continue } @@ -322,7 +330,7 @@ func DeleteShadowBranchesIfUnchanged(ctx context.Context, branches map[string]pl } deleted = append(deleted, branch) } - return deleted, failed + return deleted, failed, errors.Join(safetyErrors...) } func protectedShadowBranchForSession(s *SessionState) (string, bool) { diff --git a/cmd/entire/cli/strategy/cleanup_inventory_test.go b/cmd/entire/cli/strategy/cleanup_inventory_test.go index 6d0220cf45..42be803edf 100644 --- a/cmd/entire/cli/strategy/cleanup_inventory_test.go +++ b/cmd/entire/cli/strategy/cleanup_inventory_test.go @@ -90,16 +90,21 @@ func TestResetSession_PreservesCorruptSiblingShadow(t *testing.T) { require.NoError(t, os.WriteFile(sibling, []byte(`{"session_id":`), 0600)) } var output strings.Builder - resetErr := NewManualCommitStrategy().ResetSession(ctx, &output, io.Discard, "reset-me") + var warnings strings.Builder + resetErr := NewManualCommitStrategy().ResetSession(ctx, &output, &warnings, "reset-me") if corrupt { - require.Error(t, resetErr) + require.NoError(t, resetErr) + require.Contains(t, warnings.String(), "Warning: failed to clean up shadow branch") } else { require.NoError(t, resetErr) + require.Empty(t, warnings.String()) } + _, err := os.Stat(filepath.Join(env.dir, ".git", "entire-sessions", "reset-me.json")) + require.ErrorIs(t, err, os.ErrNotExist) require.True(t, env.branchExists(shadow)) - _, err := os.Stat(sibling) + _, err = os.Stat(sibling) require.NoError(t, err) - t.Logf("corrupt=%v shared_branch_exists=%v sibling_state_exists=true output=%q", corrupt, env.branchExists(shadow), output.String()) + t.Logf("corrupt=%v shared_branch_exists=%v sibling_state_exists=true output=%q warnings=%q", corrupt, env.branchExists(shadow), output.String(), warnings.String()) requireShadowReachable(t, hash) if corrupt { require.NoError(t, SaveSessionState(ctx, &SessionState{SessionID: "keep-me", BaseCommit: env.baseHash.String(), StartedAt: time.Now(), Phase: session.PhaseActive, StepCount: 1})) diff --git a/cmd/entire/cli/strategy/cleanup_pushed_shadow_test.go b/cmd/entire/cli/strategy/cleanup_pushed_shadow_test.go index 18be3ef5a3..1beb387e9e 100644 --- a/cmd/entire/cli/strategy/cleanup_pushed_shadow_test.go +++ b/cmd/entire/cli/strategy/cleanup_pushed_shadow_test.go @@ -2,6 +2,8 @@ package strategy import ( "context" + "os" + "path/filepath" "testing" "time" @@ -188,9 +190,10 @@ func TestDeleteShadowBranchesIfUnchanged_PreservesMovedBranch(t *testing.T) { require.NoError(t, env.repo.Storer.SetReference( plumbing.NewHashReference(plumbing.NewBranchReferenceName(shadow), newHash))) - deleted, failed := DeleteShadowBranchesIfUnchanged(context.Background(), map[string]plumbing.Hash{ + deleted, failed, err := DeleteShadowBranchesIfUnchanged(context.Background(), map[string]plumbing.Hash{ shadow: env.baseHash, }) + require.NoError(t, err) require.Empty(t, deleted) require.Equal(t, []string{shadow}, failed) require.True(t, env.branchExists(shadow)) @@ -209,7 +212,24 @@ func TestDeleteShadowBranchesIfUnchanged_PreservesBranchProtectedAfterSnapshot(t env.addSessionState("s-race", env.baseHash.String(), "", nil, nil, false) - deleted, failed := DeleteShadowBranchesIfUnchanged(context.Background(), snapshot) + deleted, failed, err := DeleteShadowBranchesIfUnchanged(context.Background(), snapshot) + require.NoError(t, err) + require.Empty(t, deleted) + require.Equal(t, []string{shadow}, failed) + require.True(t, env.branchExists(shadow)) +} + +func TestDeleteShadowBranchesIfUnchanged_ReturnsProtectionRecheckError(t *testing.T) { + env := newShadowCleanupEnv(t) + shadow := env.addShadowBranch(env.baseHash.String(), "") + path := filepath.Join(env.dir, ".git", "entire-sessions", "malformed.json") + require.NoError(t, os.MkdirAll(filepath.Dir(path), 0o700)) + require.NoError(t, os.WriteFile(path, []byte(`{"session_id":`), 0o600)) + + deleted, failed, err := DeleteShadowBranchesIfUnchanged(context.Background(), map[string]plumbing.Hash{ + shadow: env.baseHash, + }) + require.ErrorContains(t, err, "recheck protection for shadow branch") require.Empty(t, deleted) require.Equal(t, []string{shadow}, failed) require.True(t, env.branchExists(shadow)) diff --git a/cmd/entire/cli/strategy/manual_commit_condensation.go b/cmd/entire/cli/strategy/manual_commit_condensation.go index ec921a2b14..3cd659c159 100644 --- a/cmd/entire/cli/strategy/manual_commit_condensation.go +++ b/cmd/entire/cli/strategy/manual_commit_condensation.go @@ -2005,8 +2005,7 @@ func (s *ManualCommitStrategy) CondenseAndMarkFullyCondensed(ctx context.Context // cleanupShadowBranchIfUnused deletes a shadow branch if no other active sessions reference it. func (s *ManualCommitStrategy) cleanupShadowBranchIfUnused(ctx context.Context, _ *git.Repository, shadowBranchName, excludeSessionID string) error { - // List all session states to check if any other session uses this shadow branch - allStates, err := s.listAllSessionStates(ctx) + allStates, err := ListSessionStatesStrict(ctx) if err != nil { return fmt.Errorf("failed to list session states: %w", err) } diff --git a/cmd/entire/cli/strategy/manual_commit_reset.go b/cmd/entire/cli/strategy/manual_commit_reset.go index 30f63e9d77..990d5fde73 100644 --- a/cmd/entire/cli/strategy/manual_commit_reset.go +++ b/cmd/entire/cli/strategy/manual_commit_reset.go @@ -118,11 +118,12 @@ func (s *ManualCommitStrategy) ResetSession(ctx context.Context, w, errW io.Writ // Clean up shadow branch if no other sessions need it if err := s.cleanupShadowBranchIfUnused(ctx, repo, shadowBranchName, sessionID); err != nil { - return fmt.Errorf("failed to clean up shadow branch %s: %w", shadowBranchName, err) - } - // go-git's cached refs may be stale after CLI deletion. - if err := branchExistsCLI(ctx, shadowBranchName); err != nil { - fmt.Fprintf(w, "✓ Deleted shadow branch %s\n", shadowBranchName) + fmt.Fprintf(errW, "Warning: failed to clean up shadow branch %s: %v\n", shadowBranchName, err) + } else { + // go-git's cached refs may be stale after CLI deletion. + if err := branchExistsCLI(ctx, shadowBranchName); err != nil { + fmt.Fprintf(w, "✓ Deleted shadow branch %s\n", shadowBranchName) + } } return nil diff --git a/cmd/entire/cli/strategy/manual_commit_session.go b/cmd/entire/cli/strategy/manual_commit_session.go index 5e04710dba..8043adbc8e 100644 --- a/cmd/entire/cli/strategy/manual_commit_session.go +++ b/cmd/entire/cli/strategy/manual_commit_session.go @@ -112,7 +112,7 @@ func (s *ManualCommitStrategy) listAllSessionStates(ctx context.Context) ([]*Ses return nil, fmt.Errorf("failed to get state store: %w", err) } - sessionStates, err := store.ListStrict(ctx) + sessionStates, err := store.List(ctx) if err != nil { return nil, fmt.Errorf("failed to list session states: %w", err) } diff --git a/cmd/entire/cli/strategy/session_identity_test.go b/cmd/entire/cli/strategy/session_identity_test.go index e7fbfc57be..670dfc18b7 100644 --- a/cmd/entire/cli/strategy/session_identity_test.go +++ b/cmd/entire/cli/strategy/session_identity_test.go @@ -224,6 +224,22 @@ func TestFindSessionsForCommitLinking_IdentityAddsGuestSession(t *testing.T) { assert.Contains(t, ids, "sess-agent-elsewhere") } +// Not parallel: uses t.Chdir(). +func TestFindSessionsForCommitLinking_ToleratesMalformedUnrelatedState(t *testing.T) { + ctx := context.Background() + dir := identityTestRepo(t) + saveIdentitySession(t, "sess-here", func(st *SessionState) { + st.WorktreePath = dir + }) + malformed := filepath.Join(dir, ".git", "entire-sessions", "malformed.json") + require.NoError(t, os.WriteFile(malformed, []byte(`{"session_id":`), 0o600)) + + got, err := NewManualCommitStrategy().findSessionsForCommitLinking(ctx, dir) + require.NoError(t, err) + require.Len(t, got, 1) + require.Equal(t, "sess-here", got[0].SessionID) +} + // addSiblingWorktree creates a real git worktree of dir so fallback matching // (which verifies a shared git common dir) can see sessions recorded there. func addSiblingWorktree(t *testing.T, dir, name string) string { From 62770d280393bd997b6ac9086397df9ce7545b1f Mon Sep 17 00:00:00 2001 From: Muskan Paliwal Date: Fri, 11 Sep 2026 16:38:19 +0530 Subject: [PATCH 3/6] fix(cleanup): make shadow ref deletion safety atomic Entire-Checkpoint: 01M282EWYTJR6CC1QPSK5CW3PJ --- cmd/entire/cli/gitrepo/ref_cas.go | 36 ++++++++++++++++++++++ cmd/entire/cli/gitrepo/ref_cas_test.go | 37 +++++++++++++++++++++++ cmd/entire/cli/strategy/cleanup.go | 42 ++++++++++++++++++++------ 3 files changed, 105 insertions(+), 10 deletions(-) diff --git a/cmd/entire/cli/gitrepo/ref_cas.go b/cmd/entire/cli/gitrepo/ref_cas.go index e17a5f72c4..56c55c7b91 100644 --- a/cmd/entire/cli/gitrepo/ref_cas.go +++ b/cmd/entire/cli/gitrepo/ref_cas.go @@ -24,6 +24,8 @@ var ( ErrRefLocked = errors.New("git reference lock is unavailable") // ErrRefSymbolic means the requested CAS target is a symbolic reference. ErrRefSymbolic = errors.New("git reference is symbolic") + // ErrRefCASAbort means a prepared guarded update did not abort cleanly. + ErrRefCASAbort = errors.New("git reference transaction abort failed") ) // CompareAndSwapRef atomically updates a direct ref through native Git and @@ -34,6 +36,32 @@ func CompareAndSwapRef( repoRoot string, refName plumbing.ReferenceName, newHash, expectedHash plumbing.Hash, +) error { + return compareAndSwapRef(ctx, repoRoot, refName, newHash, expectedHash, nil) +} + +// CompareAndSwapRefGuarded atomically updates a direct ref only when guard +// succeeds. Git holds the prepared ref lock while guard runs, but the ref is +// not changed until after guard returns nil. +func CompareAndSwapRefGuarded( + ctx context.Context, + repoRoot string, + refName plumbing.ReferenceName, + newHash, expectedHash plumbing.Hash, + guard func() error, +) error { + if guard == nil { + return errors.New("compare-and-swap ref guard is nil") + } + return compareAndSwapRef(ctx, repoRoot, refName, newHash, expectedHash, guard) +} + +func compareAndSwapRef( + ctx context.Context, + repoRoot string, + refName plumbing.ReferenceName, + newHash, expectedHash plumbing.Hash, + guard func() error, ) error { tx, err := prepareRefCAS(ctx, repoRoot, refName, newHash, expectedHash) if err != nil { @@ -50,6 +78,14 @@ func CompareAndSwapRef( tx.abort(), ) } + if guard != nil { + if err := guard(); err != nil { + if abortErr := tx.abort(); abortErr != nil { + return errors.Join(err, fmt.Errorf("abort guarded ref update: %w", errors.Join(ErrRefCASAbort, abortErr))) + } + return err + } + } return tx.commit() } diff --git a/cmd/entire/cli/gitrepo/ref_cas_test.go b/cmd/entire/cli/gitrepo/ref_cas_test.go index 1a0f6365ad..a194085aa4 100644 --- a/cmd/entire/cli/gitrepo/ref_cas_test.go +++ b/cmd/entire/cli/gitrepo/ref_cas_test.go @@ -2,6 +2,7 @@ package gitrepo import ( "context" + "errors" "os" "os/exec" "path/filepath" @@ -90,6 +91,42 @@ func TestCompareAndSwapRef_RejectsSymbolicRef(t *testing.T) { } } +func TestCompareAndSwapRefGuarded_RejectsBeforeCommit(t *testing.T) { + t.Parallel() + for _, tt := range refCASBackends() { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + repoDir, initial, replacement := tt.init(t) + refName := plumbing.ReferenceName("refs/entire/guarded-delete") + gitenv.Run(t, repoDir, "update-ref", refName.String(), initial) + unsafe := errors.New("ref is still protected") + + err := CompareAndSwapRefGuarded( + t.Context(), + repoDir, + refName, + plumbing.ZeroHash, + plumbing.NewHash(initial), + func() error { + cmd := exec.Command("git", "update-ref", refName.String(), replacement, initial) //nolint:noctx // must run while the guarded transaction is open + cmd.Dir = repoDir + cmd.Env = gitenv.Isolated() + output, updateErr := cmd.CombinedOutput() + require.Error(t, updateErr, "the guard must run after Git locks the ref") + require.Contains(t, strings.ToLower(string(output)), "lock") + return unsafe + }, + ) + + require.ErrorIs(t, err, unsafe) + require.Equal(t, initial, strings.TrimSpace(gitenv.Run(t, repoDir, "rev-parse", refName.String()))) + + gitenv.Run(t, repoDir, "update-ref", refName.String(), replacement, initial) + require.Equal(t, replacement, strings.TrimSpace(gitenv.Run(t, repoDir, "rev-parse", refName.String()))) + }) + } +} + func TestPreparedRefCASPreventsConcurrentSymbolicConversion(t *testing.T) { t.Parallel() for _, tt := range refCASBackends() { diff --git a/cmd/entire/cli/strategy/cleanup.go b/cmd/entire/cli/strategy/cleanup.go index 6f51875d75..aa3e84b917 100644 --- a/cmd/entire/cli/strategy/cleanup.go +++ b/cmd/entire/cli/strategy/cleanup.go @@ -6,7 +6,6 @@ import ( "fmt" "log/slog" "os" - "os/exec" "path/filepath" "regexp" "sort" @@ -15,6 +14,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/gitdir" + "github.com/entireio/cli/cmd/entire/cli/gitrepo" "github.com/entireio/cli/cmd/entire/cli/logging" "github.com/entireio/cli/cmd/entire/cli/paths" "github.com/entireio/cli/cmd/entire/cli/session" @@ -293,36 +293,56 @@ func DeleteShadowBranchesIfUnchanged(ctx context.Context, branches map[string]pl if len(branches) == 0 { return []string{}, []string{}, nil } + repoRoot, err := paths.WorktreeRoot(ctx) + if err != nil { + for branch := range branches { + failed = append(failed, branch) + } + return []string{}, failed, fmt.Errorf("resolve worktree root for shadow branch cleanup: %w", err) + } var safetyErrors []error for branch, expected := range branches { if !isAutoDeletableShadowBranch(branch) || expected.IsZero() { failed = append(failed, branch) continue } - protected, err := shadowBranchProtectedByCurrentState(ctx, branch) - if err != nil { + + var protectionErr error + refName := plumbing.NewBranchReferenceName(branch) + runErr := gitrepo.CompareAndSwapRefGuarded(ctx, repoRoot, refName, plumbing.ZeroHash, expected, func() error { + protected, err := shadowBranchProtectedByCurrentState(ctx, branch) + if err != nil { + protectionErr = fmt.Errorf("recheck protection for shadow branch %s: %w", branch, err) + return protectionErr + } + if protected { + return errShadowBranchProtected + } + return nil + }) + if protectionErr != nil { logging.Debug(ctx, "shadow branch unchanged-delete skipped after protection recheck failed", slog.String("branch", branch), - slog.String("error", err.Error()), + slog.String("error", runErr.Error()), ) - safetyErrors = append(safetyErrors, fmt.Errorf("recheck protection for shadow branch %s: %w", branch, err)) + safetyErrors = append(safetyErrors, runErr) failed = append(failed, branch) continue } - if protected { + if errors.Is(runErr, errShadowBranchProtected) { logging.Debug(ctx, "shadow branch unchanged-delete skipped because current session state protects it", slog.String("branch", branch), ) + if errors.Is(runErr, gitrepo.ErrRefCASAbort) { + safetyErrors = append(safetyErrors, fmt.Errorf("abort protected shadow branch deletion: %w", runErr)) + } failed = append(failed, branch) continue } - ref := "refs/heads/" + branch - cmd := exec.CommandContext(ctx, "git", "update-ref", "-d", ref, expected.String()) - if output, runErr := cmd.CombinedOutput(); runErr != nil { + if runErr != nil { logging.Debug(ctx, "shadow branch unchanged-delete skipped", slog.String("branch", branch), slog.String("expected", expected.String()), - slog.String("output", strings.TrimSpace(string(output))), slog.String("error", runErr.Error()), ) failed = append(failed, branch) @@ -333,6 +353,8 @@ func DeleteShadowBranchesIfUnchanged(ctx context.Context, branches map[string]pl return deleted, failed, errors.Join(safetyErrors...) } +var errShadowBranchProtected = errors.New("shadow branch is protected by current session state") + func protectedShadowBranchForSession(s *SessionState) (string, bool) { if s.Phase == session.PhaseEnded && s.FullyCondensed && len(s.TurnCheckpointIDs) == 0 { return "", false // safe — session ended cleanly and finalized From eb4edad897b9fa0b444320f54c5cdeb57fb20826 Mon Sep 17 00:00:00 2001 From: Muskan Paliwal Date: Fri, 11 Sep 2026 18:03:26 +0530 Subject: [PATCH 4/6] fix(strategy): tolerate unreadable unrelated session refs Entire-Checkpoint: 01M287AR8Y7EZMJ3CRXDGFQYV8 --- .../cli/strategy/cleanup_inventory_test.go | 4 +- .../cli/strategy/manual_commit_session.go | 16 ++++++- .../cli/strategy/session_identity_test.go | 44 +++++++++++++++++-- 3 files changed, 57 insertions(+), 7 deletions(-) diff --git a/cmd/entire/cli/strategy/cleanup_inventory_test.go b/cmd/entire/cli/strategy/cleanup_inventory_test.go index 42be803edf..86ead5a3c0 100644 --- a/cmd/entire/cli/strategy/cleanup_inventory_test.go +++ b/cmd/entire/cli/strategy/cleanup_inventory_test.go @@ -137,8 +137,8 @@ func TestListAllSessionStates_PreservesUnreadableShadow(t *testing.T) { require.Error(t, readErr) require.ErrorIs(t, readErr, plumbing.ErrReferenceNotFound) states, err = strat.listAllSessionStates(ctx) - require.Error(t, err) - require.Nil(t, states) + require.NoError(t, err) + require.Empty(t, states) _, statErr := os.Stat(filepath.Join(env.dir, ".git", "entire-sessions", state.SessionID+".json")) require.NoError(t, statErr) t.Logf("ref_error=%v list_error=%v state_preserved=true", readErr, err) diff --git a/cmd/entire/cli/strategy/manual_commit_session.go b/cmd/entire/cli/strategy/manual_commit_session.go index 8043adbc8e..3c8fa24ddf 100644 --- a/cmd/entire/cli/strategy/manual_commit_session.go +++ b/cmd/entire/cli/strategy/manual_commit_session.go @@ -157,10 +157,22 @@ func (s *ManualCommitStrategy) listAllSessionStates(ctx context.Context) ([]*Ses if _, refErr := repo.Reference(refName, true); refErr != nil { absent, err := gitrepo.ReferenceIsAbsent(repo, refName) if err != nil { - return nil, fmt.Errorf("verify shadow branch %s: %w", shadowBranch, err) + logging.Debug(logging.WithComponent(ctx, "session"), + "session discovery skipped state because shadow branch absence could not be verified", + slog.String("session_id", state.SessionID), + slog.String("branch", shadowBranch), + slog.String("error", err.Error()), + ) + continue } if !absent { - return nil, fmt.Errorf("shadow branch %s is present but could not be read: %w", shadowBranch, refErr) + logging.Debug(logging.WithComponent(ctx, "session"), + "session discovery skipped state because its shadow branch could not be read", + slog.String("session_id", state.SessionID), + slog.String("branch", shadowBranch), + slog.String("error", refErr.Error()), + ) + continue } if !state.Phase.IsActive() && state.LastCheckpointID.IsEmpty() && !state.HasTaskContent() { //nolint:errcheck,gosec // G104: Cleanup is best-effort, shouldn't fail the list operation diff --git a/cmd/entire/cli/strategy/session_identity_test.go b/cmd/entire/cli/strategy/session_identity_test.go index 670dfc18b7..01db18de1f 100644 --- a/cmd/entire/cli/strategy/session_identity_test.go +++ b/cmd/entire/cli/strategy/session_identity_test.go @@ -19,6 +19,8 @@ import ( "github.com/stretchr/testify/require" ) +const unrelatedWorktreePath = "/somewhere/else/entirely" + // identityTestRepo initializes an isolated repo and chdirs into it. func identityTestRepo(t *testing.T) string { t.Helper() @@ -81,7 +83,7 @@ func TestFindSessionByCommitAncestry(t *testing.T) { anc := selfAncestorOwner(t) saveIdentitySession(t, "sess-agent", func(st *SessionState) { st.Owner = anc - st.WorktreePath = "/somewhere/else/entirely" + st.WorktreePath = unrelatedWorktreePath }) s := NewManualCommitStrategy() @@ -212,7 +214,7 @@ func TestFindSessionsForCommitLinking_IdentityAddsGuestSession(t *testing.T) { anc := selfAncestorOwner(t) saveIdentitySession(t, "sess-agent-elsewhere", func(st *SessionState) { st.Owner = anc - st.WorktreePath = "/somewhere/else/entirely" + st.WorktreePath = unrelatedWorktreePath }) s := NewManualCommitStrategy() @@ -240,6 +242,42 @@ func TestFindSessionsForCommitLinking_ToleratesMalformedUnrelatedState(t *testin require.Equal(t, "sess-here", got[0].SessionID) } +// Not parallel: uses t.Chdir(). +func TestFindSessionsForCommitLinking_ToleratesUnreadableUnrelatedRef(t *testing.T) { + ctx := context.Background() + dir := identityTestRepo(t) + saveIdentitySession(t, "sess-here", func(st *SessionState) { + st.WorktreePath = dir + }) + const unreadableWorktreeID = "unreadable" + saveIdentitySession(t, "sess-unreadable", func(st *SessionState) { + st.WorktreeID = unreadableWorktreeID + st.WorktreePath = unrelatedWorktreePath + }) + + repo, err := gitrepo.OpenPath(dir) + require.NoError(t, err) + defer repo.Close() + head, err := repo.Head() + require.NoError(t, err) + shadow := getShadowBranchNameForCommit("deadbeefdeadbeefdeadbeefdeadbeefdeadbeef", unreadableWorktreeID) + refName := plumbing.NewBranchReferenceName(shadow) + require.NoError(t, repo.Storer.SetReference(plumbing.NewHashReference(refName, head.Hash()))) + refPath := filepath.Join(dir, ".git", refName.String()) + require.NoError(t, os.Chmod(refPath, 0o000)) + t.Cleanup(func() { require.NoError(t, os.Chmod(refPath, 0o600)) }) + _, rawErr := os.ReadFile(refPath) + if !os.IsPermission(rawErr) { + t.Skip("filesystem does not enforce unreadable file permissions") + } + + got, err := NewManualCommitStrategy().findSessionsForCommitLinking(ctx, dir) + require.NoError(t, err) + require.Len(t, got, 1) + require.Equal(t, "sess-here", got[0].SessionID) + require.FileExists(t, filepath.Join(dir, ".git", "entire-sessions", "sess-unreadable.json")) +} + // addSiblingWorktree creates a real git worktree of dir so fallback matching // (which verifies a shared git common dir) can see sessions recorded there. func addSiblingWorktree(t *testing.T, dir, name string) string { @@ -441,7 +479,7 @@ func TestUpdateBaseCommitIfChanged_GuestWorktreeGating(t *testing.T) { guest := &SessionState{ SessionID: "sess-guest-gate", BaseCommit: "1111111111111111111111111111111111111111", - WorktreePath: "/somewhere/else/entirely", + WorktreePath: unrelatedWorktreePath, Phase: session.PhaseActive, } s.updateBaseCommitIfChanged(ctx, guest, "2222222222222222222222222222222222222222", dir) From 8981acaf07bca5b396f43c330b78f1a7782a1685 Mon Sep 17 00:00:00 2001 From: Muskan Paliwal Date: Sat, 12 Sep 2026 18:44:50 +0530 Subject: [PATCH 5/6] refactor(cleanup): share shadow branch protection checks Entire-Checkpoint: 01M2AW38T6Y7300KGQ45Z0VPHS --- cmd/entire/cli/doctor.go | 25 +------------------ .../cli/doctor_cleanup_inventory_test.go | 4 +-- cmd/entire/cli/strategy/cleanup.go | 20 +++++++++++++++ .../cli/strategy/cleanup_inventory_test.go | 24 +++++++----------- .../strategy/manual_commit_condensation.go | 17 +++---------- 5 files changed, 36 insertions(+), 54 deletions(-) diff --git a/cmd/entire/cli/doctor.go b/cmd/entire/cli/doctor.go index 299a3fdd86..841f257eb5 100644 --- a/cmd/entire/cli/doctor.go +++ b/cmd/entire/cli/doctor.go @@ -432,7 +432,7 @@ func discardSession(ctx context.Context, ss stuckSession, _ *git.Repository, err // Delete shadow branch if it exists and no other sessions need it if ss.HasShadowBranch { - if shouldDelete, err := canDeleteShadowBranch(ctx, ss.ShadowBranch, ss.State.SessionID); err != nil { + if shouldDelete, err := strategy.CanDeleteShadowBranch(ctx, ss.ShadowBranch, ss.State.SessionID); err != nil { fmt.Fprintf(errW, "Warning: could not check other sessions for shadow branch: %v\n", err) } else if shouldDelete { if err := strategy.DeleteBranchCLI(ctx, ss.ShadowBranch); err != nil { @@ -1469,26 +1469,3 @@ func writeCodexTrackedHooksRemedy(w io.Writer) { fmt.Fprintln(w, " .codex/hooks.json is tracked — commit it and make sure the root worktree has it") fmt.Fprintln(w, " (merge to the default branch, or check that branch out there).") } - -// canDeleteShadowBranch checks if a shadow branch can be safely deleted. -// Returns true if no other sessions (besides excludeSessionID) need this branch. -func canDeleteShadowBranch(ctx context.Context, shadowBranch, excludeSessionID string) (bool, error) { - states, err := strategy.ListSessionStatesStrict(ctx) - if err != nil { - return false, fmt.Errorf("failed to list session states: %w", err) - } - - for _, state := range states { - if state.SessionID == excludeSessionID { - continue - } - // Task records never live on the shadow branch, so only SaveStep - // checkpoints pin it alive. - otherShadow := checkpoint.ShadowBranchNameForCommit(state.BaseCommit, state.WorktreeID) - if otherShadow == shadowBranch && state.StepCount > 0 { - return false, nil - } - } - - return true, nil -} diff --git a/cmd/entire/cli/doctor_cleanup_inventory_test.go b/cmd/entire/cli/doctor_cleanup_inventory_test.go index a4ad986f50..f8bdfe9904 100644 --- a/cmd/entire/cli/doctor_cleanup_inventory_test.go +++ b/cmd/entire/cli/doctor_cleanup_inventory_test.go @@ -22,11 +22,11 @@ func TestCanDeleteShadowBranch_RejectsIncompleteInventory(t *testing.T) { name := checkpoint.ShadowBranchNameForCommit(state.BaseCommit, "") file := filepath.Join(dir, ".git", session.SessionStateDirName, "protected.json") require.NoError(t, os.WriteFile(file, []byte(`{"session_id":`), 0o600)) - allowed, err := canDeleteShadowBranch(t.Context(), name, "other") + allowed, err := strategy.CanDeleteShadowBranch(t.Context(), name, "other") require.Error(t, err) require.False(t, allowed) require.NoError(t, strategy.SaveSessionState(t.Context(), state)) - allowed, err = canDeleteShadowBranch(t.Context(), name, "other") + allowed, err = strategy.CanDeleteShadowBranch(t.Context(), name, "other") require.NoError(t, err) require.False(t, allowed) } diff --git a/cmd/entire/cli/strategy/cleanup.go b/cmd/entire/cli/strategy/cleanup.go index aa3e84b917..6808c4f5b6 100644 --- a/cmd/entire/cli/strategy/cleanup.go +++ b/cmd/entire/cli/strategy/cleanup.go @@ -362,6 +362,26 @@ func protectedShadowBranchForSession(s *SessionState) (string, bool) { return getShadowBranchNameForCommit(s.BaseCommit, s.WorktreeID), true } +// CanDeleteShadowBranch reports whether any other session still pins the branch. +func CanDeleteShadowBranch(ctx context.Context, shadowBranch, excludeSessionID string) (bool, error) { + states, err := ListSessionStatesStrict(ctx) + if err != nil { + return false, fmt.Errorf("failed to list session states: %w", err) + } + for _, state := range states { + if state.SessionID == excludeSessionID { + continue + } + // Task records never live on the shadow branch, so only SaveStep + // checkpoints pin it alive. + otherShadow := getShadowBranchNameForCommit(state.BaseCommit, state.WorktreeID) + if otherShadow == shadowBranch && state.StepCount > 0 { + return false, nil + } + } + return true, nil +} + func shadowBranchProtectedByCurrentState(ctx context.Context, branch string) (bool, error) { states, err := ListSessionStatesStrict(ctx) if err != nil { diff --git a/cmd/entire/cli/strategy/cleanup_inventory_test.go b/cmd/entire/cli/strategy/cleanup_inventory_test.go index 86ead5a3c0..6c9d7c7800 100644 --- a/cmd/entire/cli/strategy/cleanup_inventory_test.go +++ b/cmd/entire/cli/strategy/cleanup_inventory_test.go @@ -3,7 +3,6 @@ package strategy import ( "io" "os" - "os/exec" "path/filepath" "strings" "testing" @@ -17,22 +16,17 @@ import ( func newShadowOnlyCommit(t *testing.T, env *shadowCleanupEnv, shadow string) plumbing.Hash { t.Helper() - cmd := exec.CommandContext(t.Context(), "git", "mktree") - output, treeErr := cmd.CombinedOutput() - require.NoError(t, treeErr, string(output)) - hash, err := checkpoint.CreateCommit(t.Context(), env.repo, plumbing.NewHash("4b825dc642cb6eb9a060e54bf8d69288fbee4904"), env.baseHash, "shadow-only history", "test", "test@test.com") + hash, err := checkpoint.CreateCommit(t.Context(), env.repo, emptyTreeHash(t, env.repo), env.baseHash, "shadow-only history", "test", "test@test.com") require.NoError(t, err) require.NoError(t, env.repo.Storer.SetReference(plumbing.NewHashReference(plumbing.NewBranchReferenceName(shadow), hash))) return hash } -func requireShadowReachable(t *testing.T, hash plumbing.Hash) { +func requireShadowBranchAt(t *testing.T, env *shadowCleanupEnv, shadow string, hash plumbing.Hash) { t.Helper() - cmd := exec.CommandContext(t.Context(), "git", "fsck", "--unreachable", "--no-reflogs") - output, err := cmd.CombinedOutput() - require.NoError(t, err, string(output)) - require.NotContains(t, string(output), "unreachable commit "+hash.String()) - t.Log("shadow commit remains reachable:", hash.String()) + ref, err := env.repo.Reference(plumbing.NewBranchReferenceName(shadow), false) + require.NoError(t, err) + require.Equal(t, hash, ref.Hash()) } func TestCleanupPushedShadowBranches_PreservesExpiredUncondensed(t *testing.T) { @@ -66,7 +60,7 @@ func TestCleanupPushedShadowBranches_PreservesExpiredUncondensed(t *testing.T) { _, err = os.Stat(filepath.Join(env.dir, ".git", "entire-sessions", state.SessionID+".json")) require.NoError(t, err) t.Logf("phase=%s control_deleted=0 aged_deleted=%d state_preserved=true", phase, deleted) - requireShadowReachable(t, hash) + requireShadowBranchAt(t, env, shadow, hash) }) } } @@ -105,7 +99,7 @@ func TestResetSession_PreservesCorruptSiblingShadow(t *testing.T) { _, err = os.Stat(sibling) require.NoError(t, err) t.Logf("corrupt=%v shared_branch_exists=%v sibling_state_exists=true output=%q warnings=%q", corrupt, env.branchExists(shadow), output.String(), warnings.String()) - requireShadowReachable(t, hash) + requireShadowBranchAt(t, env, shadow, hash) if corrupt { require.NoError(t, SaveSessionState(ctx, &SessionState{SessionID: "keep-me", BaseCommit: env.baseHash.String(), StartedAt: time.Now(), Phase: session.PhaseActive, StepCount: 1})) require.NoError(t, NewManualCommitStrategy().ResetSession(ctx, &output, io.Discard, "keep-me")) @@ -146,7 +140,7 @@ func TestListAllSessionStates_PreservesUnreadableShadow(t *testing.T) { deleted, err := CleanupPushedShadowBranches(ctx) require.NoError(t, err) require.Zero(t, deleted) - requireShadowReachable(t, hash) + requireShadowBranchAt(t, env, shadow, hash) } func TestCleanupPushedShadowBranches_PreservesCorruptState(t *testing.T) { @@ -161,7 +155,7 @@ func TestCleanupPushedShadowBranches_PreservesCorruptState(t *testing.T) { require.Error(t, err) require.Zero(t, deleted) require.True(t, env.branchExists(shadow)) - requireShadowReachable(t, hash) + requireShadowBranchAt(t, env, shadow, hash) env.addSessionState("pending", env.baseHash.String(), "", nil, nil, false) deleted, err = CleanupPushedShadowBranches(ctx) require.NoError(t, err) diff --git a/cmd/entire/cli/strategy/manual_commit_condensation.go b/cmd/entire/cli/strategy/manual_commit_condensation.go index 3cd659c159..fa43460f4c 100644 --- a/cmd/entire/cli/strategy/manual_commit_condensation.go +++ b/cmd/entire/cli/strategy/manual_commit_condensation.go @@ -2005,21 +2005,12 @@ func (s *ManualCommitStrategy) CondenseAndMarkFullyCondensed(ctx context.Context // cleanupShadowBranchIfUnused deletes a shadow branch if no other active sessions reference it. func (s *ManualCommitStrategy) cleanupShadowBranchIfUnused(ctx context.Context, _ *git.Repository, shadowBranchName, excludeSessionID string) error { - allStates, err := ListSessionStatesStrict(ctx) + canDelete, err := CanDeleteShadowBranch(ctx, shadowBranchName, excludeSessionID) if err != nil { - return fmt.Errorf("failed to list session states: %w", err) + return err } - - for _, state := range allStates { - if state.SessionID == excludeSessionID { - continue - } - otherShadow := getShadowBranchNameForCommit(state.BaseCommit, state.WorktreeID) - // Only SaveStep checkpoints live on the shadow branch; task records do - // not, so they no longer pin the branch alive. - if otherShadow == shadowBranchName && state.StepCount > 0 { - return nil - } + if !canDelete { + return nil } // No other sessions need it, delete the shadow branch via CLI From 2e78baebfa65d451d55db8943b3fcd00f1a5560e Mon Sep 17 00:00:00 2001 From: Muskan Paliwal Date: Thu, 17 Sep 2026 17:16:46 +0530 Subject: [PATCH 6/6] fix(strategy): guard shared shadow branch deletion Entire-Checkpoint: 01M2QK1KZNWNVHDXW8KSTQS688 --- cmd/entire/cli/doctor.go | 11 +--- cmd/entire/cli/session/state.go | 11 ++-- cmd/entire/cli/strategy/cleanup.go | 61 ++++++++++++++++--- .../cli/strategy/cleanup_inventory_test.go | 27 ++++++++ .../strategy/manual_commit_condensation.go | 26 +------- .../cli/strategy/manual_commit_reset.go | 9 +-- 6 files changed, 95 insertions(+), 50 deletions(-) diff --git a/cmd/entire/cli/doctor.go b/cmd/entire/cli/doctor.go index e79c9315c1..1f01c99213 100644 --- a/cmd/entire/cli/doctor.go +++ b/cmd/entire/cli/doctor.go @@ -434,7 +434,7 @@ func promptSessionAction(ss stuckSession) (string, error) { } // discardSession removes session state and cleans up the shadow branch. -func discardSession(ctx context.Context, ss stuckSession, _ *git.Repository, errW io.Writer) error { +func discardSession(ctx context.Context, ss stuckSession, repo *git.Repository, errW io.Writer) error { // Clear session state file if err := strategy.ClearSessionStateWithProgress(ctx, ss.State.SessionID, errW, strategy.SessionLockNoticeDelay); err != nil { return fmt.Errorf("failed to clear session state: %w", err) @@ -442,15 +442,8 @@ func discardSession(ctx context.Context, ss stuckSession, _ *git.Repository, err // Delete shadow branch if it exists and no other sessions need it if ss.HasShadowBranch { - if shouldDelete, err := strategy.CanDeleteShadowBranch(ctx, ss.ShadowBranch, ss.State.SessionID); err != nil { + if _, err := strategy.DeleteShadowBranchIfUnused(ctx, repo, ss.ShadowBranch, ss.State.SessionID); err != nil { fmt.Fprintf(errW, "Warning: could not check other sessions for shadow branch: %v\n", err) - } else if shouldDelete { - if err := strategy.DeleteBranchCLI(ctx, ss.ShadowBranch); err != nil { - // Branch already gone is not an error — keeps discard idempotent - if !errors.Is(err, strategy.ErrBranchNotFound) { - return fmt.Errorf("failed to delete shadow branch: %w", err) - } - } } } diff --git a/cmd/entire/cli/session/state.go b/cmd/entire/cli/session/state.go index 3d0d47e871..4eef969eea 100644 --- a/cmd/entire/cli/session/state.go +++ b/cmd/entire/cli/session/state.go @@ -967,8 +967,14 @@ func (s *State) IsEnded() bool { return s.Phase == PhaseEnded || s.EndedAt != nil } +// HasShadowBranchContent reports whether this state records checkpoint content +// written to its shadow branch. +func (s *State) HasShadowBranchContent() bool { + return s.StepCount > 0 || len(s.FilesTouched) > 0 +} + func (s *State) hasPendingCheckpointContent() bool { - return (!s.FullyCondensed && (s.StepCount > 0 || len(s.FilesTouched) > 0)) || + return (!s.FullyCondensed && s.HasShadowBranchContent()) || s.HasTaskContent() || len(s.TurnCheckpointIDs) > 0 || s.CondensationAttempt != nil } @@ -1309,9 +1315,6 @@ func (s *StateStore) list(ctx context.Context, strict bool) ([]*State, error) { } continue } - if strings.HasSuffix(entry.Name(), ".tmp") { - continue // Skip temp files - } sessionID := strings.TrimSuffix(entry.Name(), ".json") state, err := s.Load(ctx, sessionID) diff --git a/cmd/entire/cli/strategy/cleanup.go b/cmd/entire/cli/strategy/cleanup.go index e01dfca6f1..363312c0bc 100644 --- a/cmd/entire/cli/strategy/cleanup.go +++ b/cmd/entire/cli/strategy/cleanup.go @@ -19,6 +19,7 @@ import ( "github.com/entireio/cli/cmd/entire/cli/paths" "github.com/entireio/cli/cmd/entire/cli/session" + "github.com/go-git/go-git/v6" "github.com/go-git/go-git/v6/plumbing" "github.com/go-git/go-git/v6/plumbing/object" ) @@ -276,10 +277,7 @@ func CleanupPushedShadowBranches(ctx context.Context) (int, error) { slog.Int("deleted_count", len(deleted)), ) } - if safetyErr != nil { - return len(deleted), safetyErr - } - return len(deleted), nil + return len(deleted), safetyErr } // DeleteShadowBranchesIfUnchanged deletes shadow branches only if each branch @@ -376,16 +374,65 @@ func CanDeleteShadowBranch(ctx context.Context, shadowBranch, excludeSessionID s if state.SessionID == excludeSessionID { continue } - // Task records never live on the shadow branch, so only SaveStep - // checkpoints pin it alive. + // SaveStep increments StepCount, while incremental task checkpoints + // record FilesTouched. Either proves this session wrote to the branch. otherShadow := getShadowBranchNameForCommit(state.BaseCommit, state.WorktreeID) - if otherShadow == shadowBranch && state.StepCount > 0 { + if otherShadow == shadowBranch && state.HasShadowBranchContent() { return false, nil } } return true, nil } +// DeleteShadowBranchIfUnused deletes a branch only if its tip and protecting +// session inventory remain unchanged while Git holds the ref lock. +func DeleteShadowBranchIfUnused( + ctx context.Context, + repo *git.Repository, + shadowBranch, excludeSessionID string, +) (bool, error) { + refName := plumbing.NewBranchReferenceName(shadowBranch) + ref, err := repo.Reference(refName, false) + if err != nil { + absent, absenceErr := gitrepo.ReferenceIsAbsent(repo, refName) + if absenceErr != nil { + return false, fmt.Errorf("verify shadow branch %s absence: %w", shadowBranch, absenceErr) + } + if absent { + return false, nil + } + return false, fmt.Errorf("read shadow branch %s: %w", shadowBranch, err) + } + + repoRoot, err := paths.WorktreeRoot(ctx) + if err != nil { + return false, fmt.Errorf("resolve worktree root for shadow branch cleanup: %w", err) + } + runErr := gitrepo.CompareAndSwapRefGuarded(ctx, repoRoot, refName, plumbing.ZeroHash, ref.Hash(), func() error { + canDelete, guardErr := CanDeleteShadowBranch(ctx, shadowBranch, excludeSessionID) + if guardErr != nil { + return guardErr + } + if !canDelete { + return errShadowBranchProtected + } + return nil + }) + if errors.Is(runErr, errShadowBranchProtected) { + if errors.Is(runErr, gitrepo.ErrRefCASAbort) { + return false, fmt.Errorf("abort protected shadow branch deletion: %w", runErr) + } + return false, nil + } + if errors.Is(runErr, gitrepo.ErrRefCASConflict) { + return false, nil + } + if runErr != nil { + return false, fmt.Errorf("delete shadow branch %s: %w", shadowBranch, runErr) + } + return true, nil +} + func shadowBranchProtectedByCurrentState(ctx context.Context, branch string) (bool, error) { states, err := ListSessionStatesStrict(ctx) if err != nil { diff --git a/cmd/entire/cli/strategy/cleanup_inventory_test.go b/cmd/entire/cli/strategy/cleanup_inventory_test.go index 6c9d7c7800..1989b5c355 100644 --- a/cmd/entire/cli/strategy/cleanup_inventory_test.go +++ b/cmd/entire/cli/strategy/cleanup_inventory_test.go @@ -109,6 +109,33 @@ func TestResetSession_PreservesCorruptSiblingShadow(t *testing.T) { } } +func TestResetSession_PreservesSiblingIncrementalTaskCheckpoint(t *testing.T) { + env := newShadowCleanupEnv(t) + ctx := t.Context() + shadow := env.addShadowBranch(env.baseHash.String(), "") + hash := newShadowOnlyCommit(t, env, shadow) + recent := time.Now() + require.NoError(t, SaveSessionState(ctx, &SessionState{ + SessionID: "reset-me", + BaseCommit: env.baseHash.String(), + StartedAt: recent, + Phase: session.PhaseActive, + StepCount: 1, + })) + require.NoError(t, SaveSessionState(ctx, &SessionState{ + SessionID: "task-session", + BaseCommit: env.baseHash.String(), + StartedAt: recent, + Phase: session.PhaseActive, + FilesTouched: []string{"task.go"}, + })) + + var warnings strings.Builder + require.NoError(t, NewManualCommitStrategy().ResetSession(ctx, io.Discard, &warnings, "reset-me")) + require.Empty(t, warnings.String()) + requireShadowBranchAt(t, env, shadow, hash) +} + func TestListAllSessionStates_PreservesUnreadableShadow(t *testing.T) { env := newShadowCleanupEnv(t) ctx := t.Context() diff --git a/cmd/entire/cli/strategy/manual_commit_condensation.go b/cmd/entire/cli/strategy/manual_commit_condensation.go index be5c5e8c42..aa0ef3d1e7 100644 --- a/cmd/entire/cli/strategy/manual_commit_condensation.go +++ b/cmd/entire/cli/strategy/manual_commit_condensation.go @@ -1876,7 +1876,7 @@ func (s *ManualCommitStrategy) CondenseSessionByID(ctx context.Context, sessionI return nil } - if err := s.cleanupShadowBranchIfUnused(ctx, repo, shadowBranchName, sessionID); err != nil { + if _, err := DeleteShadowBranchIfUnused(ctx, repo, shadowBranchName, sessionID); err != nil { logging.Warn(logCtx, "failed to clean up shadow branch", slog.String("shadow_branch", shadowBranchName), slog.String("error", err.Error()), @@ -2047,7 +2047,7 @@ func (s *ManualCommitStrategy) CondenseAndMarkFullyCondensed(ctx context.Context } if didCondense && shadowBranchName != "" { - if err := s.cleanupShadowBranchIfUnused(ctx, repo, shadowBranchName, sessionID); err != nil { + if _, err := DeleteShadowBranchIfUnused(ctx, repo, shadowBranchName, sessionID); err != nil { logging.Warn(logCtx, "eager condense: failed to clean up shadow branch", slog.String("shadow_branch", shadowBranchName), slog.String("error", err.Error()), @@ -2056,25 +2056,3 @@ func (s *ManualCommitStrategy) CondenseAndMarkFullyCondensed(ctx context.Context } return nil } - -// cleanupShadowBranchIfUnused deletes a shadow branch if no other active sessions reference it. -func (s *ManualCommitStrategy) cleanupShadowBranchIfUnused(ctx context.Context, _ *git.Repository, shadowBranchName, excludeSessionID string) error { - canDelete, err := CanDeleteShadowBranch(ctx, shadowBranchName, excludeSessionID) - if err != nil { - return err - } - if !canDelete { - return nil - } - - // No other sessions need it, delete the shadow branch via CLI - // (go-git v5's RemoveReference doesn't persist with packed refs/worktrees) - if err := DeleteBranchCLI(ctx, shadowBranchName); err != nil { - // Branch already gone is not an error - if errors.Is(err, ErrBranchNotFound) { - return nil - } - return fmt.Errorf("failed to remove shadow branch: %w", err) - } - return nil -} diff --git a/cmd/entire/cli/strategy/manual_commit_reset.go b/cmd/entire/cli/strategy/manual_commit_reset.go index 990d5fde73..740fa064f0 100644 --- a/cmd/entire/cli/strategy/manual_commit_reset.go +++ b/cmd/entire/cli/strategy/manual_commit_reset.go @@ -117,13 +117,10 @@ func (s *ManualCommitStrategy) ResetSession(ctx context.Context, w, errW io.Writ defer repo.Close() // Clean up shadow branch if no other sessions need it - if err := s.cleanupShadowBranchIfUnused(ctx, repo, shadowBranchName, sessionID); err != nil { + if deleted, err := DeleteShadowBranchIfUnused(ctx, repo, shadowBranchName, sessionID); err != nil { fmt.Fprintf(errW, "Warning: failed to clean up shadow branch %s: %v\n", shadowBranchName, err) - } else { - // go-git's cached refs may be stale after CLI deletion. - if err := branchExistsCLI(ctx, shadowBranchName); err != nil { - fmt.Fprintf(w, "✓ Deleted shadow branch %s\n", shadowBranchName) - } + } else if deleted { + fmt.Fprintf(w, "✓ Deleted shadow branch %s\n", shadowBranchName) } return nil