diff --git a/cmd/entire/cli/doctor.go b/cmd/entire/cli/doctor.go index bbbf009690..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 := 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) - } - } } } @@ -1597,26 +1590,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.ListSessionStates(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 new file mode 100644 index 0000000000..f8bdfe9904 --- /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 := strategy.CanDeleteShadowBranch(t.Context(), name, "other") + require.Error(t, err) + require.False(t, allowed) + require.NoError(t, strategy.SaveSessionState(t.Context(), state)) + allowed, err = strategy.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..56c55c7b91 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 @@ -26,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 @@ -36,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 { @@ -52,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() } @@ -234,28 +268,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/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/session/state.go b/cmd/entire/cli/session/state.go index b7cb7142ef..c703a48e53 100644 --- a/cmd/entire/cli/session/state.go +++ b/cmd/entire/cli/session/state.go @@ -967,6 +967,17 @@ 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.HasShadowBranchContent()) || + 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. @@ -1127,8 +1138,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) { return s.load(ctx, sessionID, true) } @@ -1166,10 +1177,7 @@ func (s *StateStore) load(ctx context.Context, sessionID string, deleteStale boo } state.NormalizeAfterLoad(ctx) - if state.IsStale() { - if !deleteStale { - return &state, nil - } + if deleteStale && state.IsStale() && !state.hasPendingCheckpointContent() { logCtx := logging.WithComponent(ctx, "session") logging.Debug(logCtx, "deleting stale session state", slog.String("session_id", sessionID), @@ -1273,22 +1281,28 @@ func (s *StateStore) RemoveAll() error { return nil } -// List returns all session states, deleting any that have gone stale. +// List returns readable session states, skipping individual load failures. +// Expired records are deleted only when they have no pending checkpoint content. func (s *StateStore) List(ctx context.Context) ([]*State, error) { - return s.list(ctx, true) + return s.list(ctx, false, true) } -// ListReadOnly returns every persisted session without deleting or hiding -// stale records. -// -// `entire status` is the caller this exists for: asking what is happening must -// not change what is happening. Cleanup stays with the callers that own it — -// doctor and the session sweeper — which keep using List. +// 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, true) +} + +// ListReadOnly returns readable persisted sessions without deleting or hiding +// stale records, skipping individual load failures. func (s *StateStore) ListReadOnly(ctx context.Context) ([]*State, error) { - return s.list(ctx, false) + return s.list(ctx, false, false) } -func (s *StateStore) list(ctx context.Context, deleteStale bool) ([]*State, error) { +func (s *StateStore) list(ctx context.Context, strict, deleteStale 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) @@ -1303,17 +1317,26 @@ func (s *StateStore) list(ctx context.Context, deleteStale bool) ([]*State, erro 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 strings.HasSuffix(entry.Name(), ".tmp") { - continue // Skip temp files + if entry.IsDir() { + if strict { + return nil, fmt.Errorf("session state %s is a directory", entry.Name()) + } + continue } sessionID := strings.TrimSuffix(entry.Name(), ".json") state, err := s.load(ctx, sessionID, deleteStale) 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 8ebeef8abc..363312c0bc 100644 --- a/cmd/entire/cli/strategy/cleanup.go +++ b/cmd/entire/cli/strategy/cleanup.go @@ -2,10 +2,10 @@ package strategy import ( "context" + "errors" "fmt" "log/slog" "os" - "os/exec" "path/filepath" "regexp" "sort" @@ -14,10 +14,12 @@ 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" + "github.com/go-git/go-git/v6" "github.com/go-git/go-git/v6/plumbing" "github.com/go-git/go-git/v6/plumbing/object" ) @@ -210,9 +212,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 @@ -234,7 +236,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) } @@ -268,14 +270,14 @@ 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)), ) } - return len(deleted), nil + return len(deleted), safetyErr } // DeleteShadowBranchesIfUnchanged deletes shadow branches only if each branch @@ -286,39 +288,63 @@ 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 + } + 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, 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) @@ -326,9 +352,11 @@ func DeleteShadowBranchesIfUnchanged(ctx context.Context, branches map[string]pl } deleted = append(deleted, branch) } - return deleted, failed + 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 @@ -336,8 +364,77 @@ 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 + } + // 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.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 := 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..1989b5c355 --- /dev/null +++ b/cmd/entire/cli/strategy/cleanup_inventory_test.go @@ -0,0 +1,190 @@ +package strategy + +import ( + "io" + "os" + "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() + 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 requireShadowBranchAt(t *testing.T, env *shadowCleanupEnv, shadow string, hash plumbing.Hash) { + t.Helper() + 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) { + 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) + requireShadowBranchAt(t, env, shadow, 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 + var warnings strings.Builder + resetErr := NewManualCommitStrategy().ResetSession(ctx, &output, &warnings, "reset-me") + if corrupt { + 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) + 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()) + 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")) + require.False(t, env.branchExists(shadow)) + } + }) + } +} + +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() + 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.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) + require.NoError(t, os.Chmod(refPath, 0600)) + deleted, err := CleanupPushedShadowBranches(ctx) + require.NoError(t, err) + require.Zero(t, deleted) + requireShadowBranchAt(t, env, shadow, 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)) + requireShadowBranchAt(t, env, shadow, 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/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/common.go b/cmd/entire/cli/strategy/common.go index df708383a3..4f48767adb 100644 --- a/cmd/entire/cli/strategy/common.go +++ b/cmd/entire/cli/strategy/common.go @@ -1618,16 +1618,6 @@ func DeleteBranchCLI(ctx context.Context, branchName string) error { return nil } -// branchExistsCLI checks if a branch exists using git CLI. -// Returns nil if the branch exists, or an error if it does not. -func branchExistsCLI(ctx context.Context, branchName string) error { - cmd := exec.CommandContext(ctx, "git", "show-ref", "--verify", "--quiet", "refs/heads/"+branchName) - if err := cmd.Run(); err != nil { - return fmt.Errorf("branch %s not found: %w", branchName, err) - } - return nil -} - // collectUntrackedFiles collects untracked files in the working directory that are // NOT ignored by .gitignore. This is used to capture the initial state when starting // a session, distinguishing files present at session start from ones it created. diff --git a/cmd/entire/cli/strategy/manual_commit_condensation.go b/cmd/entire/cli/strategy/manual_commit_condensation.go index 802a44d00c..e597345a1d 100644 --- a/cmd/entire/cli/strategy/manual_commit_condensation.go +++ b/cmd/entire/cli/strategy/manual_commit_condensation.go @@ -1867,7 +1867,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()), @@ -2038,7 +2038,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()), @@ -2047,35 +2047,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 { - // List all session states to check if any other session uses this shadow branch - allStates, err := s.listAllSessionStates(ctx) - if err != nil { - return fmt.Errorf("failed to list session states: %w", 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 - } - } - - // 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 9fb5d35612..740fa064f0 100644 --- a/cmd/entire/cli/strategy/manual_commit_reset.go +++ b/cmd/entire/cli/strategy/manual_commit_reset.go @@ -117,14 +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 { - // 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) - } + } else if deleted { + 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 366a256584..6cec7db083 100644 --- a/cmd/entire/cli/strategy/manual_commit_session.go +++ b/cmd/entire/cli/strategy/manual_commit_session.go @@ -155,7 +155,26 @@ 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 { + 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 { + 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 store.Clear(ctx, state.SessionID) diff --git a/cmd/entire/cli/strategy/session_identity_test.go b/cmd/entire/cli/strategy/session_identity_test.go index e7fbfc57be..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() @@ -224,6 +226,58 @@ 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) +} + +// 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 { @@ -425,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) 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, diff --git a/docs/development/checkpoint-implementation.md b/docs/development/checkpoint-implementation.md index 4b06179cd3..2327f6c8ea 100644 --- a/docs/development/checkpoint-implementation.md +++ b/docs/development/checkpoint-implementation.md @@ -134,7 +134,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