diff --git a/cmd/entire/cli/strategy/manual_commit_hooks.go b/cmd/entire/cli/strategy/manual_commit_hooks.go index 185fa0d4f..47ad3ec7b 100644 --- a/cmd/entire/cli/strategy/manual_commit_hooks.go +++ b/cmd/entire/cli/strategy/manual_commit_hooks.go @@ -638,19 +638,20 @@ func (h *postCommitActionHandler) HandleCondenseIfFilesTouched(state *session.St // into this commit. Requires both that hasNew is true AND that the session's files // overlap with the committed files with matching content. // -// This prevents stale sessions (ACTIVE sessions where the agent was killed, or -// ENDED/IDLE sessions with carry-forward files) from being condensed into every -// unrelated commit. +// This prevents IDLE/ENDED sessions with carry-forward files from being condensed +// into every unrelated commit. +// +// For ACTIVE sessions, we fail-open when file overlap cannot be confirmed. This is +// because the agent may commit files mid-turn before SaveStep records them in +// FilesTouched, causing the overlap check to see stale data. Failing closed in this +// case orphans the checkpoint trailer (metadata never written to entire/checkpoints/v1). +// The tradeoff: stale ACTIVE sessions (agent killed without Stop hook) may be +// condensed into unrelated commits as a side effect. This is acceptable because +// orphaned checkpoints (data loss) are worse than extra metadata (data duplication). // // filesTouchedBefore is populated from: // - state.FilesTouched for IDLE/ENDED sessions (set via SaveStep/SaveTaskStep -> mergeFilesTouched) // - transcript extraction for ACTIVE sessions when FilesTouched is empty -// -// When filesTouchedBefore is empty: -// - For ACTIVE sessions: fail-open (trust hasNew) because the agent may be -// mid-turn before any files are saved to state. -// - For IDLE/ENDED sessions: return false because there are no files to -// overlap with the commit. func (h *postCommitActionHandler) shouldCondenseWithOverlapCheck(isActive bool) bool { if !h.hasNew { return false @@ -670,7 +671,10 @@ func (h *postCommitActionHandler) shouldCondenseWithOverlapCheck(isActive bool) } } if len(committedTouchedFiles) == 0 { - return false + // ACTIVE sessions fail-open: the agent may have committed files mid-turn + // that aren't yet in FilesTouched (SaveStep hasn't run). Without this, + // the checkpoint trailer added by PrepareCommitMsg would be orphaned. + return isActive } return filesOverlapWithContent(h.ctx, h.repo, h.shadowBranchName, h.commit, committedTouchedFiles, overlapOpts{ headTree: h.headTree, diff --git a/cmd/entire/cli/strategy/phase_postcommit_test.go b/cmd/entire/cli/strategy/phase_postcommit_test.go index 6efa60c8b..225ec0858 100644 --- a/cmd/entire/cli/strategy/phase_postcommit_test.go +++ b/cmd/entire/cli/strategy/phase_postcommit_test.go @@ -1680,18 +1680,19 @@ func TestPostCommit_EndedSessionCarryForward_NotCondensedIntoUnrelatedCommit(t * "New ACTIVE session BaseCommit should be updated after condensation") } -// TestPostCommit_StaleActiveSession_NotCondensed verifies that a stale ACTIVE -// session (agent killed without Stop hook) is NOT condensed into an unrelated -// commit from a different session. +// TestPostCommit_StaleActiveSession_CondensedAlongside verifies that a stale ACTIVE +// session (agent killed without Stop hook) IS condensed alongside the new session +// when the commit has a checkpoint trailer. // -// Root cause: when an agent is killed without the Stop hook firing, its session -// remains in ACTIVE phase permanently. Previously, PostCommit unconditionally -// set hasNew=true for ACTIVE sessions and skipped the filesOverlapWithContent -// check, so stale ACTIVE sessions got condensed into every commit. +// Background: when an agent is killed without the Stop hook firing, its session +// remains in ACTIVE phase permanently. ACTIVE sessions fail-open in the overlap +// check to prevent orphaned checkpoint trailers (where PrepareCommitMsg adds a +// trailer but PostCommit never creates the matching metadata). // -// The fix applies the overlap check to ALL sessions (including ACTIVE) using -// filesTouchedBefore, so stale sessions with unrelated files are filtered out. -func TestPostCommit_StaleActiveSession_NotCondensed(t *testing.T) { +// The tradeoff: stale sessions may be condensed into unrelated commits as a side +// effect, attaching extra metadata. This is acceptable because orphaned checkpoints +// (data loss) are worse than extra metadata (data duplication). +func TestPostCommit_StaleActiveSession_CondensedAlongside(t *testing.T) { dir := setupGitRepo(t) t.Chdir(dir) @@ -1713,9 +1714,6 @@ func TestPostCommit_StaleActiveSession_NotCondensed(t *testing.T) { staleState.FilesTouched = []string{"test.txt"} require.NoError(t, s.saveSessionState(context.Background(), staleState)) - staleOriginalBaseCommit := staleState.BaseCommit - staleOriginalStepCount := staleState.StepCount - // Move HEAD forward with an unrelated commit (no trailer) wt, err := repo.Worktree() require.NoError(t, err) @@ -1783,40 +1781,124 @@ func TestPostCommit_StaleActiveSession_NotCondensed(t *testing.T) { err = s.PostCommit(context.Background()) require.NoError(t, err) - // --- Verify: stale ACTIVE session was NOT condensed --- + // --- Verify: stale ACTIVE session WAS condensed (fail-open for ACTIVE) --- staleState, err = s.loadSessionState(context.Background(), staleSessionID) require.NoError(t, err) - // StepCount should be unchanged (not reset by condensation) - assert.Equal(t, staleOriginalStepCount, staleState.StepCount, - "Stale ACTIVE session StepCount should NOT be reset (no condensation)") + // StepCount is 1 after condensation because carry-forward writes remaining + // files to the new shadow branch (condensation resets to 0, then carry-forward sets to 1). + assert.Equal(t, 1, staleState.StepCount, + "Stale ACTIVE session StepCount should be 1 (condensed + carry-forward)") - // BaseCommit IS updated for ACTIVE sessions (updateBaseCommitIfChanged) + // BaseCommit updated to new HEAD assert.Equal(t, newHead, staleState.BaseCommit, - "Stale ACTIVE session BaseCommit should be updated (ACTIVE sessions always get BaseCommit updated)") - assert.NotEqual(t, staleOriginalBaseCommit, staleState.BaseCommit, - "Stale ACTIVE session BaseCommit should have changed") + "Stale ACTIVE session BaseCommit should be updated after condensation") // Phase stays ACTIVE assert.Equal(t, session.PhaseActive, staleState.Phase, "Stale ACTIVE session should remain ACTIVE") - // --- Verify: new ACTIVE session WAS condensed --- + // --- Verify: new ACTIVE session WAS also condensed --- newState, err = s.loadSessionState(context.Background(), newSessionID) require.NoError(t, err) - // StepCount reset to 0 by condensation - assert.Equal(t, 0, newState.StepCount, - "New ACTIVE session StepCount should be reset by condensation") + // StepCount may be 0 (no carry-forward) or 1 (carry-forward for remaining files) + assert.LessOrEqual(t, newState.StepCount, 1, + "New ACTIVE session StepCount should be 0 or 1 after condensation") // BaseCommit updated to new HEAD assert.Equal(t, newHead, newState.BaseCommit, "New ACTIVE session BaseCommit should be updated after condensation") - // Verify entire/checkpoints/v1 exists (new session was condensed) + // Verify entire/checkpoints/v1 exists (both sessions were condensed) + _, err = repo.Reference(plumbing.NewBranchReferenceName(paths.MetadataBranchName), true) + require.NoError(t, err, + "entire/checkpoints/v1 should exist (sessions were condensed)") +} + +// TestPostCommit_ActiveSession_CommitsFileNotInFilesTouched_Condenses verifies that +// an ACTIVE session is condensed when the agent commits a file that isn't yet tracked +// in FilesTouched. +// +// This reproduces a real bug: during an active turn, the agent creates and commits a +// new file before SaveStep records it in FilesTouched. The carry-forward set from the +// previous checkpoint contains different files (e.g., from a subagent). The overlap +// check in shouldCondenseWithOverlapCheck found no intersection between the committed +// files and FilesTouched, so it returned false — leaving an orphaned checkpoint ID +// in the commit trailer with no corresponding metadata on entire/checkpoints/v1. +// +// The fix: when committedTouchedFiles is empty for an ACTIVE session, fail-open +// (return true) instead of returning false. ACTIVE sessions may have stale +// FilesTouched that doesn't include files the agent is working on mid-turn. +func TestPostCommit_ActiveSession_CommitsFileNotInFilesTouched_Condenses(t *testing.T) { + dir := setupGitRepo(t) + t.Chdir(dir) + + repo, err := git.PlainOpen(dir) + require.NoError(t, err) + + s := &ManualCommitStrategy{} + + // --- Create an ACTIVE session with a checkpoint --- + sessionID := "active-mid-turn-session" + setupSessionWithCheckpoint(t, s, repo, dir, sessionID) + + state, err := s.loadSessionState(context.Background(), sessionID) + require.NoError(t, err) + state.Phase = session.PhaseActive + // Simulate carry-forward: previous checkpoint touched test.txt, but the agent + // is now working on a completely different file mid-turn. FilesTouched only + // contains the carry-forward files from the last SaveStep/SaveTaskStep. + state.FilesTouched = []string{"test.txt"} + require.NoError(t, s.saveSessionState(context.Background(), state)) + + // --- Agent creates and commits a NEW file not in FilesTouched --- + // This is the key scenario: the agent writes new-agent-work.txt mid-turn, + // then commits it. SaveStep hasn't run yet, so FilesTouched is stale. + newFile := filepath.Join(dir, "new-agent-work.txt") + require.NoError(t, os.WriteFile(newFile, []byte("agent created this mid-turn"), 0o644)) + + wt, err := repo.Worktree() + require.NoError(t, err) + _, err = wt.Add("new-agent-work.txt") + require.NoError(t, err) + + cpID := "ba1ba2ba3ba4" + commitMsg := "add new agent work\n\n" + trailers.CheckpointTrailerKey + ": " + cpID + "\n" + _, err = wt.Commit(commitMsg, &git.CommitOptions{ + Author: &object.Signature{Name: "Test", Email: "test@test.com", When: time.Now()}, + }) + require.NoError(t, err) + + head, err := repo.Head() + require.NoError(t, err) + newHead := head.Hash().String() + + // Run PostCommit + err = s.PostCommit(context.Background()) + require.NoError(t, err) + + // --- Verify: ACTIVE session WAS condensed --- + state, err = s.loadSessionState(context.Background(), sessionID) + require.NoError(t, err) + + // StepCount is 1 after condensation because carry-forward writes remaining + // files to the new shadow branch (condensation resets to 0, then carry-forward sets to 1). + assert.Equal(t, 1, state.StepCount, + "ACTIVE session should be condensed even when committed file is not in FilesTouched") + + // BaseCommit updated to new HEAD + assert.Equal(t, newHead, state.BaseCommit, + "ACTIVE session BaseCommit should be updated after condensation") + + // Phase stays ACTIVE + assert.Equal(t, session.PhaseActive, state.Phase, + "ACTIVE session should remain ACTIVE") + + // Verify entire/checkpoints/v1 exists (session was condensed) _, err = repo.Reference(plumbing.NewBranchReferenceName(paths.MetadataBranchName), true) require.NoError(t, err, - "entire/checkpoints/v1 should exist (new session was condensed)") + "entire/checkpoints/v1 should exist — checkpoint must not be orphaned") } // TestPostCommit_IdleSessionEmptyFilesTouched_NotCondensed verifies that an IDLE