fix: fail-open overlap check for ACTIVE sessions in PostCommit#575
fix: fail-open overlap check for ACTIVE sessions in PostCommit#575
Conversation
When an agent commits a file mid-turn before SaveStep records it in FilesTouched, the overlap check in shouldCondenseWithOverlapCheck found no intersection between committed files and FilesTouched, returning false. This orphaned the checkpoint trailer added by PrepareCommitMsg (no matching metadata on entire/checkpoints/v1). The fix: when committedTouchedFiles is empty for an ACTIVE session, return true (fail-open) instead of false. This matches the existing fail-open behavior when filesTouchedBefore is empty entirely. Tradeoff: stale ACTIVE sessions (agent killed without Stop hook) may now be condensed into unrelated commits as a side effect. This is acceptable because orphaned checkpoints (data loss) are worse than extra metadata (data duplication). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: b1b803dfc6d4
PR SummaryMedium Risk Overview Tests were updated/added to lock in the new tradeoff: a stale Written by Cursor Bugbot for commit 19de42a. Configure here. |
There was a problem hiding this comment.
Pull request overview
Fixes a PostCommit condensation edge case where ACTIVE sessions could leave orphaned Entire-Checkpoint trailers when the committed files don’t intersect with FilesTouched (common when an agent commits mid-turn before SaveStep updates state). This aligns PostCommit behavior with PrepareCommitMsg’s trailer behavior to avoid losing checkpoint metadata.
Changes:
- Adjust
shouldCondenseWithOverlapCheckto fail-open for ACTIVE sessions whencommittedTouchedFilesis empty. - Update the stale ACTIVE-session test expectations to reflect the new fail-open behavior.
- Add a regression test covering the “commit mid-turn before FilesTouched is updated” scenario.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
cmd/entire/cli/strategy/manual_commit_hooks.go |
Implements ACTIVE fail-open when overlap can’t be confirmed because no touched files were committed. |
cmd/entire/cli/strategy/phase_postcommit_test.go |
Updates stale-session behavior expectations and adds a regression test for the orphaned-trailer scenario. |
| // 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") |
There was a problem hiding this comment.
In this test, the assertions used to prove the stale ACTIVE session was condensed are ambiguous: StepCount == 1 is already true immediately after setupSessionWithCheckpoint() (SaveStep increments it), and BaseCommit == newHead can also happen via updateBaseCommitIfChanged even when no condensation occurs. Consider asserting a field that only changes on condensation (e.g., AttributionBaseCommit should update to newHead, and/or PromptAttributions should be cleared) so the test actually fails if the stale session is not condensed.
| // shouldCondenseWithOverlapCheck returns true if the session should be condensed | ||
| // 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). |
There was a problem hiding this comment.
The function comment says condensation "Requires ... that the session's files overlap with the committed files with matching content", but the implementation now explicitly returns true for ACTIVE sessions when committedTouchedFiles is empty (fail-open) and therefore does not require overlap/content matching in that case. Please adjust the wording to reflect the ACTIVE fail-open exception so the comment matches behavior.
Problem
When an agent commits a file mid-turn before
SaveSteprecords it inFilesTouched, the overlap check inshouldCondenseWithOverlapCheckfinds no intersection between committed files andFilesTouched, returningfalse. This orphans the checkpoint trailer added byPrepareCommitMsg— metadata never gets written toentire/checkpoints/v1.Root cause: Mismatch between
PrepareCommitMsg(unconditionally adds trailer for ACTIVE no-TTY sessions) andPostCommit(requires file overlap to condense).FilesTouchedis stale mid-turn because it's only updated viaSaveStep(Stop hook) orSaveTaskStep.Observed 3 times in a single session (
2335990a): checkpoints7a17709559b6,291f49fbcf7b, and720e4558e8e2all orphaned.Fix
When
committedTouchedFilesis empty for an ACTIVE session, returntrue(fail-open) instead offalse. This matches the existing fail-open behavior whenfilesTouchedBeforeis empty entirely (line 658-659).if len(committedTouchedFiles) == 0 { - return false + return isActive }Tradeoff
Stale ACTIVE sessions (agent killed without Stop hook) may now 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).
Alternatives considered
LastInteractionTime)Discussion points
Test plan
TestPostCommit_ActiveSession_CommitsFileNotInFilesTouched_Condenses— new test for the bugTestPostCommit_StaleActiveSession_CondensedAlongside— updated: stale session now condensed (was_NotCondensed)mise run fmt && mise run lint && mise run test:ci)