don't update LastInteraction when only git hooks were triggered#550
don't update LastInteraction when only git hooks were triggered#550
Conversation
Entire-Checkpoint: dd07617714d9
PR SummaryLow Risk Overview Tests are updated accordingly, including Written by Cursor Bugbot for commit ba20a2f. Configure here. |
There was a problem hiding this comment.
Pull request overview
This PR adjusts the session phase state machine so that git commit hooks no longer count as “user interaction” and therefore do not keep sessions alive by updating LastInteractionTime, enabling stale-session cleanup to work as intended.
Changes:
- Remove
ActionUpdateLastInteractionfromEventGitCommittransitions across IDLE/ACTIVE/ENDED phases. - Update unit tests to reflect the new transition action sets and updated expectations around
LastInteractionTime.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
cmd/entire/cli/session/phase.go |
Removes ActionUpdateLastInteraction from EventGitCommit transition results so commits don’t refresh session “activity”. |
cmd/entire/cli/session/phase_test.go |
Updates transition/apply-transition tests to match the new GitCommit behavior and expectations. |
| func TestApplyTransition_ReturnsHandlerError_ButSetsPhase(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| state := &State{Phase: PhaseActive} | ||
| handler := &mockActionHandler{returnErr: errors.New("condense failed")} | ||
| // Synthetic transition with [Condense, UpdateLastInteraction] actions. | ||
| result := TransitionResult{ | ||
| NewPhase: PhaseIdle, | ||
| Actions: []Action{ActionCondense, ActionUpdateLastInteraction}, | ||
| Actions: []Action{ActionCondense}, | ||
| } | ||
|
|
||
| err := ApplyTransition(context.Background(), state, result, handler) | ||
|
|
||
| require.Error(t, err) | ||
| assert.Contains(t, err.Error(), "condense failed") | ||
| // Phase must still be set even though handler failed. | ||
| assert.Equal(t, PhaseIdle, state.Phase) | ||
| // Common action must still run even though handler failed. | ||
| require.NotNil(t, state.LastInteractionTime, | ||
| "UpdateLastInteraction must run despite earlier handler error") | ||
| } |
There was a problem hiding this comment.
TestApplyTransition_ReturnsHandlerError_ButSetsPhase used to also assert that a common action (ActionUpdateLastInteraction) still runs even when a handler action errors (matching ApplyTransition's contract that common actions always apply). After this change, that behavior is no longer exercised by any test in this file. Add a focused test case where a handler action fails but ActionUpdateLastInteraction is still present later in the Actions list (e.g., []Action{ActionCondenseIfFilesTouched, ActionUpdateLastInteraction}), and assert LastInteractionTime was set despite the handler error.
I noticed a lot of sessions still sitting around locally. And what jumped out was that all of them were constantly updated. Turns out that the commit hooks kept looking at older sessions - which is fine - but also kept bumping
LastInteractionTimewhich made them never going to be cleaned up. Now a commit isn't really an interaction in a session, so let's not do this in this case.