Skip to content

fix(sweep): own the half-ended session shape - #2102

Open
Soph wants to merge 1 commit into
mainfrom
fix/sweep-half-ended-sessions
Open

fix(sweep): own the half-ended session shape#2102
Soph wants to merge 1 commit into
mainfrom
fix/sweep-half-ended-sessions

Conversation

@Soph

@Soph Soph commented Aug 22, 2026

Copy link
Copy Markdown
Collaborator

https://entire.io/gh/entireio/cli/trails/1126

Follow-up to #2029 (trail 1073), from re-reviewing it after the fixes landed.

Problem

A session state can carry EndedAt without PhaseEnded. State.IsEnded exists for exactly that shape, and its doc names it:

Both halves matter and neither implies the other in practice: Phase is what the state machine sets, EndedAt is what the finalizing write stamps, and a state file can carry one without the other (a legacy record, or a partial write). Callers that filter for active sessions must agree on this rule, so it lives here rather than being re-spelled at each site.

The sweep re-spelled it, and its two halves ended up disagreeing. isSweepableZombie correctly used IsEnded() to route non-ended zombies to finalizeExitedSessions, then three lines later re-tested Phase == PhaseEnded before nominating a condense.

For a half-ended state, both halves declined:

  • finalizeExitedSessions skips it, because OwnerExited() returns false once IsEnded().
  • the condense path skipped it on the phase test.

entire doctor does not reach it either — classifySession’s phase switch falls through to default: return nil. So the only thing that ever touched such a session was the 7-day stale purge, which deletes the uncondensed content rather than condensing it.

Fix

Three gates had to move together, and each is independently load-bearing — reverting any one alone fails the new end-to-end test (verified, see below):

Gate Change
isSweepableZombie Drop the Phase re-test. Reaching that line already means IsEnded(), so the test could only ever exclude the half-ended shape.
runSessionSweep Both loop guards keyed on Phase, so a nominated half-ended session was skipped before and after the fresh re-load.
IsCondensableEndedSession Same Phase test, so a session that survived both guards was still reported non-condensable.

Blast radius

IsCondensableEndedSession is shared with the PostCommit stale-session warning, which now also counts half-ended sessions holding uncondensed content. That is the same correction, not a side effect: they are stale and actionable by doctor for the same reason a PhaseEnded one is.

Deliberately not changed

doctor’s classifySession has the same phase-switch gap, so a half-ended session is still not listed by entire doctor. It is a separate command on a separate path, and the sweep now condenses these before doctor would have to. Happy to fold it in if you would rather have the two consistent.

Verification

TestRunSessionSweep_CondensesHalfEndedSession builds the shape end to end (Phase: PhaseIdle, EndedAt 48h old, StepCount: 2, shadow ref present) and asserts the sweep actually condensed it — not merely that the predicate stopped matching. Mutation-probed all three gates:

revert isSweepableZombie          -> FAIL "a half-ended session with uncondensed steps must nominate the sweep"
revert runSessionSweep guards     -> FAIL "the half-ended session must no longer hold uncondensed content"
revert IsCondensableEndedSession  -> FAIL "the sweep must have condensed the half-ended session, not just skipped it"

Plus a table case in TestIsSweepableZombie. mise run check clean (fmt, lint 0 issues, unit + integration + canary all green).


Note

Medium Risk
Changes which ended sessions the background sweep will condense, including a shared predicate used by PostCommit warnings. Wrong classification could condense a still-live session or keep dropping half-ended ones, but the change aligns with existing IsEnded semantics and is covered by a new e2e test.

Overview
The background session sweep now treats half-ended states (EndedAt stamped, Phase not ENDED) as ended zombies instead of leaving them unowned until the 7-day stale purge deletes uncondensed content.

isSweepableZombie, the condense loop in runSessionSweep, and IsCondensableEndedSession all key off IsEnded() rather than Phase == PhaseEnded, so nomination, re-load guards, and the condense gate agree. The PostCommit stale-session warning picks up the same shape.

Adds a table case and an end-to-end sweep test that asserts a half-ended session is actually condensed.

Reviewed by Cursor Bugbot for commit 422cb01. Configure here.

A state can carry EndedAt without PhaseEnded. State.IsEnded exists for
exactly that shape — its doc names it: "a state file can carry one
without the other (a legacy record, or a partial write). Callers that
filter for active sessions must agree on this rule, so it lives here
rather than being re-spelled at each site."

The sweep re-spelled it, and the two halves disagreed. isSweepableZombie
correctly used IsEnded to route non-ended zombies to finalizeExitedSessions,
then three lines later re-tested Phase == PhaseEnded before nominating a
condense. For a half-ended state both halves declined: finalizeExitedSessions
skips it because OwnerExited returns false once IsEnded, and the condense
path skipped it on the phase test. Nobody owned it. doctor does not reach it
either — classifySession's phase switch falls through to default — so the
only thing that ever touched such a session was the 7-day stale purge, which
deletes the uncondensed content instead of condensing it.

Three gates had to move together, and each is independently load-bearing
(reverting any one alone fails the new end-to-end test):

- isSweepableZombie: drop the Phase re-test. Reaching that line already
  means IsEnded, so the test could only ever exclude the half-ended shape.
- runSessionSweep: both loop guards keyed on Phase, so a nominated
  half-ended session was skipped before and after the fresh re-load.
- IsCondensableEndedSession: same Phase test, so a session that survived
  both guards was still reported as non-condensable.

IsCondensableEndedSession is shared with the PostCommit stale-session
warning, which now also counts half-ended sessions holding uncondensed
content. That is the same correction: they are stale and actionable by
doctor for the same reason a PhaseEnded one is.

Not changed here: doctor's classifySession has the same phase-switch gap,
so a half-ended session is still not listed by `entire doctor`. It is a
separate command on a separate path, and the sweep now condenses these
before doctor would have to.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Entire-Checkpoint: 01M0MKH7MXFF197VW1Q4SMZ7T7
Copilot AI lite review requested due to automatic review settings August 22, 2026 11:26
@Soph
Soph requested a review from a team as a code owner August 22, 2026 11:26

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes a logic gap in the background session sweep where “half-ended” session states (with EndedAt stamped but Phase not yet ENDED) were neither finalized nor condensed, leaving them to eventually be purged (and lose uncondensed content). It aligns the sweep’s nomination, loop guards, and the shared condensation predicate to consistently use State.IsEnded() semantics.

Changes:

  • Update strategy.IsCondensableEndedSession to key off IsEnded() instead of Phase == ENDED, so half-ended states remain eligible for condensation when appropriate.
  • Update the session sweep (isSweepableZombie and runSessionSweep) to avoid re-testing Phase after already determining IsEnded().
  • Add coverage for the half-ended shape via a new table test case and an end-to-end sweep test ensuring the session is actually condensed.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File Description
cmd/entire/cli/strategy/manual_commit_session.go Makes the shared “condensable ended session” predicate respect IsEnded() so condensation eligibility matches the canonical endedness rule.
cmd/entire/cli/session_sweep.go Ensures the sweep’s nomination and condense loop guards consistently treat half-ended sessions as ended zombies rather than skipping them.
cmd/entire/cli/session_sweep_test.go Adds explicit test coverage for half-ended sessions, including an end-to-end sweep test that asserts condensation occurs.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Comment @cursor review or bugbot run to trigger another review on this PR

Reviewed by Cursor Bugbot for commit 422cb01. Configure here.

// "a legacy record, or a partial write"), and re-testing Phase here would
// drop it: finalizeExitedSessions already skips it because OwnerExited is
// false once IsEnded, so a Phase test here would leave it owned by nobody.
if st.FullyCondensed || (st.StepCount <= 0 && !st.HasTaskContent()) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sweep can condense resumed sessions

High Severity

Replacing the PhaseEnded checks with IsEnded() also treats post-condense sessions as ended. CondenseSessionByID resets Phase to idle without clearing EndedAt or setting FullyCondensed, and TurnStart from idle does not clear EndedAt. A later resume that accumulates new steps still looks ended by age, so the sweep can condense a live session.

Additional Locations (2)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 422cb01. Configure here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants