diff --git a/cmd/entire/cli/hooks.go b/cmd/entire/cli/hooks.go index 005ae8c5c..1faa6bccb 100644 --- a/cmd/entire/cli/hooks.go +++ b/cmd/entire/cli/hooks.go @@ -284,7 +284,9 @@ func handleSessionStartCommon() error { if state, loadErr := strategy.LoadSessionState(input.SessionID); loadErr != nil { fmt.Fprintf(os.Stderr, "Warning: failed to load session state on start: %v\n", loadErr) } else if state != nil { - strategy.TransitionAndLog(state, session.EventSessionStart, session.TransitionContext{}) + if transErr := strategy.TransitionAndLog(state, session.EventSessionStart, session.TransitionContext{}, session.NoOpActionHandler{}); transErr != nil { + fmt.Fprintf(os.Stderr, "Warning: session start transition failed: %v\n", transErr) + } if saveErr := strategy.SaveSessionState(state); saveErr != nil { fmt.Fprintf(os.Stderr, "Warning: failed to update session state on start: %v\n", saveErr) } diff --git a/cmd/entire/cli/hooks_claudecode_handlers.go b/cmd/entire/cli/hooks_claudecode_handlers.go index b2fade179..c72ba5364 100644 --- a/cmd/entire/cli/hooks_claudecode_handlers.go +++ b/cmd/entire/cli/hooks_claudecode_handlers.go @@ -748,7 +748,9 @@ func transitionSessionTurnEnd(sessionID string) { if turnState == nil { return } - strategy.TransitionAndLog(turnState, session.EventTurnEnd, session.TransitionContext{}) + if err := strategy.TransitionAndLog(turnState, session.EventTurnEnd, session.TransitionContext{}, session.NoOpActionHandler{}); err != nil { + fmt.Fprintf(os.Stderr, "Warning: turn-end transition failed: %v\n", err) + } // Always dispatch to strategy for turn-end handling. The strategy reads // work items from state (e.g. TurnCheckpointIDs), not the action list. @@ -774,7 +776,9 @@ func markSessionEnded(sessionID string) error { return nil // No state file, nothing to update } - strategy.TransitionAndLog(state, session.EventSessionStop, session.TransitionContext{}) + if transErr := strategy.TransitionAndLog(state, session.EventSessionStop, session.TransitionContext{}, session.NoOpActionHandler{}); transErr != nil { + fmt.Fprintf(os.Stderr, "Warning: session stop transition failed: %v\n", transErr) + } now := time.Now() state.EndedAt = &now diff --git a/cmd/entire/cli/session/phase.go b/cmd/entire/cli/session/phase.go index 6b5b9fc46..500fb076e 100644 --- a/cmd/entire/cli/session/phase.go +++ b/cmd/entire/cli/session/phase.go @@ -249,31 +249,76 @@ func transitionFromEnded(event Event, ctx TransitionContext) TransitionResult { } } -// ApplyCommonActions applies the common (non-strategy-specific) actions from a -// TransitionResult to the given State. It updates Phase, LastInteractionTime, -// and EndedAt as indicated by the transition. -// -// Returns the subset of actions that require strategy-specific handling -// (e.g., ActionCondense, ActionWarnStaleSession). -// The caller is responsible for dispatching those. -func ApplyCommonActions(state *State, result TransitionResult) []Action { +// ActionHandler defines strategy-specific side effects for state transitions. +// The compiler enforces that every strategy-specific action has a handler. +type ActionHandler interface { + HandleCondense(state *State) error + HandleCondenseIfFilesTouched(state *State) error + HandleDiscardIfNoFiles(state *State) error + HandleWarnStaleSession(state *State) error +} + +// NoOpActionHandler is a default ActionHandler where all methods are no-ops. +// Embed this in handler structs to only override the methods you need. +type NoOpActionHandler struct{} + +func (NoOpActionHandler) HandleCondense(_ *State) error { return nil } +func (NoOpActionHandler) HandleCondenseIfFilesTouched(_ *State) error { return nil } +func (NoOpActionHandler) HandleDiscardIfNoFiles(_ *State) error { return nil } +func (NoOpActionHandler) HandleWarnStaleSession(_ *State) error { return nil } + +// ApplyTransition applies a TransitionResult to state: sets the new phase +// unconditionally, then executes all actions. Common actions +// (UpdateLastInteraction, ClearEndedAt) always run regardless of handler +// errors so that bookkeeping fields stay consistent with the new phase. +// Strategy-specific handler actions stop on the first error; subsequent +// handler actions are skipped but common actions continue. Returns the +// first handler error, or nil. +func ApplyTransition(state *State, result TransitionResult, handler ActionHandler) error { state.Phase = result.NewPhase - var remaining []Action + var handlerErr error for _, action := range result.Actions { switch action { + // Common actions: always applied, even after a handler error. case ActionUpdateLastInteraction: now := time.Now() state.LastInteractionTime = &now case ActionClearEndedAt: state.EndedAt = nil - case ActionCondense, ActionCondenseIfFilesTouched, ActionDiscardIfNoFiles, - ActionWarnStaleSession: - // Strategy-specific actions — pass through to caller. - remaining = append(remaining, action) + + // Strategy-specific actions: skip remaining after the first handler error. + case ActionCondense: + if handlerErr == nil { + if err := handler.HandleCondense(state); err != nil { + handlerErr = fmt.Errorf("%s: %w", action, err) + } + } + case ActionCondenseIfFilesTouched: + if handlerErr == nil { + if err := handler.HandleCondenseIfFilesTouched(state); err != nil { + handlerErr = fmt.Errorf("%s: %w", action, err) + } + } + case ActionDiscardIfNoFiles: + if handlerErr == nil { + if err := handler.HandleDiscardIfNoFiles(state); err != nil { + handlerErr = fmt.Errorf("%s: %w", action, err) + } + } + case ActionWarnStaleSession: + if handlerErr == nil { + if err := handler.HandleWarnStaleSession(state); err != nil { + handlerErr = fmt.Errorf("%s: %w", action, err) + } + } + default: + if handlerErr == nil { + handlerErr = fmt.Errorf("unhandled action: %s", action) + } } } - return remaining + return handlerErr } // MermaidDiagram generates a Mermaid state diagram from the transition table. diff --git a/cmd/entire/cli/session/phase_test.go b/cmd/entire/cli/session/phase_test.go index 541308497..181d0bac8 100644 --- a/cmd/entire/cli/session/phase_test.go +++ b/cmd/entire/cli/session/phase_test.go @@ -1,6 +1,7 @@ package session import ( + "errors" "testing" "time" @@ -360,179 +361,215 @@ func TestTransition_all_phase_event_combinations_are_defined(t *testing.T) { } } -func TestApplyCommonActions_SetsPhase(t *testing.T) { +func TestMermaidDiagram(t *testing.T) { t.Parallel() - state := &State{Phase: PhaseIdle} - result := TransitionResult{ - NewPhase: PhaseActive, - Actions: []Action{ActionUpdateLastInteraction}, - } + diagram := MermaidDiagram() - remaining := ApplyCommonActions(state, result) + // Verify the diagram contains expected state declarations. + assert.Contains(t, diagram, "stateDiagram-v2") + assert.Contains(t, diagram, "IDLE") + assert.Contains(t, diagram, "ACTIVE") + assert.Contains(t, diagram, "ENDED") + assert.NotContains(t, diagram, "ACTIVE_COMMITTED") - assert.Equal(t, PhaseActive, state.Phase) - assert.Empty(t, remaining, "UpdateLastInteraction should be consumed") + // Verify key transitions are present. + assert.Contains(t, diagram, "idle --> active") + assert.Contains(t, diagram, "active --> active") // ACTIVE+GitCommit stays ACTIVE + assert.Contains(t, diagram, "ended --> idle") + assert.Contains(t, diagram, "ended --> active") + + // Verify actions appear in labels. + assert.Contains(t, diagram, "Condense") + assert.Contains(t, diagram, "ClearEndedAt") + assert.Contains(t, diagram, "WarnStaleSession") + assert.NotContains(t, diagram, "MigrateShadowBranch") +} + +// mockActionHandler records which handler methods were called. +type mockActionHandler struct { + condenseCalled bool + condenseIfFilesTouchedCalled bool + discardIfNoFilesCalled bool + warnStaleSessionCalled bool + returnErr error +} + +func (m *mockActionHandler) HandleCondense(_ *State) error { + m.condenseCalled = true + return m.returnErr +} + +func (m *mockActionHandler) HandleCondenseIfFilesTouched(_ *State) error { + m.condenseIfFilesTouchedCalled = true + return m.returnErr +} + +func (m *mockActionHandler) HandleDiscardIfNoFiles(_ *State) error { + m.discardIfNoFilesCalled = true + return m.returnErr +} + +func (m *mockActionHandler) HandleWarnStaleSession(_ *State) error { + m.warnStaleSessionCalled = true + return m.returnErr } -func TestApplyCommonActions_UpdatesLastInteractionTime(t *testing.T) { +func TestApplyTransition_SetsPhaseAndHandlesCommonActions(t *testing.T) { t.Parallel() state := &State{Phase: PhaseIdle} - before := time.Now() - + handler := &mockActionHandler{} result := TransitionResult{ NewPhase: PhaseActive, Actions: []Action{ActionUpdateLastInteraction}, } - _ = ApplyCommonActions(state, result) + err := ApplyTransition(state, result, handler) + require.NoError(t, err) + assert.Equal(t, PhaseActive, state.Phase) require.NotNil(t, state.LastInteractionTime) - assert.False(t, state.LastInteractionTime.Before(before), - "LastInteractionTime should be >= test start time") + assert.False(t, handler.condenseCalled) } -func TestApplyCommonActions_ClearsEndedAt(t *testing.T) { +func TestApplyTransition_CallsHandlerForCondense(t *testing.T) { t.Parallel() - endedAt := time.Now().Add(-time.Hour) - state := &State{ - Phase: PhaseEnded, - EndedAt: &endedAt, - } - + state := &State{Phase: PhaseActive} + handler := &mockActionHandler{} result := TransitionResult{ NewPhase: PhaseIdle, - Actions: []Action{ActionClearEndedAt}, + Actions: []Action{ActionCondense, ActionUpdateLastInteraction}, } - remaining := ApplyCommonActions(state, result) + err := ApplyTransition(state, result, handler) - assert.Nil(t, state.EndedAt, "EndedAt should be cleared") + require.NoError(t, err) + assert.True(t, handler.condenseCalled) assert.Equal(t, PhaseIdle, state.Phase) - assert.Empty(t, remaining, "ClearEndedAt should be consumed") + require.NotNil(t, state.LastInteractionTime) } -func TestApplyCommonActions_PassesThroughStrategyActions(t *testing.T) { +func TestApplyTransition_CallsHandlerForCondenseIfFilesTouched(t *testing.T) { t.Parallel() - state := &State{Phase: PhaseActive} + state := &State{Phase: PhaseEnded} + handler := &mockActionHandler{} result := TransitionResult{ - NewPhase: PhaseIdle, - Actions: []Action{ActionCondense, ActionUpdateLastInteraction}, + NewPhase: PhaseEnded, + Actions: []Action{ActionCondenseIfFilesTouched, ActionUpdateLastInteraction}, } - remaining := ApplyCommonActions(state, result) + err := ApplyTransition(state, result, handler) - assert.Equal(t, []Action{ActionCondense}, remaining, - "ActionCondense should be passed through to caller") - assert.Equal(t, PhaseIdle, state.Phase) - require.NotNil(t, state.LastInteractionTime) + require.NoError(t, err) + assert.True(t, handler.condenseIfFilesTouchedCalled) } -func TestApplyCommonActions_MultipleStrategyActions(t *testing.T) { +func TestApplyTransition_CallsHandlerForDiscardIfNoFiles(t *testing.T) { t.Parallel() - state := &State{Phase: PhaseActive} + state := &State{Phase: PhaseEnded} + handler := &mockActionHandler{} result := TransitionResult{ - NewPhase: PhaseActive, - Actions: []Action{ActionCondense, ActionUpdateLastInteraction}, + NewPhase: PhaseEnded, + Actions: []Action{ActionDiscardIfNoFiles, ActionUpdateLastInteraction}, } - remaining := ApplyCommonActions(state, result) + err := ApplyTransition(state, result, handler) - assert.Equal(t, []Action{ActionCondense}, remaining) - assert.Equal(t, PhaseActive, state.Phase) + require.NoError(t, err) + assert.True(t, handler.discardIfNoFilesCalled) } -func TestApplyCommonActions_WarnStaleSessionPassedThrough(t *testing.T) { +func TestApplyTransition_CallsHandlerForWarnStaleSession(t *testing.T) { t.Parallel() state := &State{Phase: PhaseActive} + handler := &mockActionHandler{} result := TransitionResult{ NewPhase: PhaseActive, Actions: []Action{ActionWarnStaleSession}, } - remaining := ApplyCommonActions(state, result) + err := ApplyTransition(state, result, handler) - assert.Equal(t, []Action{ActionWarnStaleSession}, remaining) + require.NoError(t, err) + assert.True(t, handler.warnStaleSessionCalled) } -func TestApplyCommonActions_NoActions(t *testing.T) { +func TestApplyTransition_ClearsEndedAt(t *testing.T) { t.Parallel() - state := &State{Phase: PhaseIdle} + endedAt := time.Now().Add(-time.Hour) + state := &State{Phase: PhaseEnded, EndedAt: &endedAt} + handler := &mockActionHandler{} result := TransitionResult{ NewPhase: PhaseIdle, - Actions: nil, + Actions: []Action{ActionClearEndedAt}, } - remaining := ApplyCommonActions(state, result) + err := ApplyTransition(state, result, handler) - assert.Nil(t, remaining) - assert.Equal(t, PhaseIdle, state.Phase) + require.NoError(t, err) + assert.Nil(t, state.EndedAt) } -func TestApplyCommonActions_EndedToActiveTransition(t *testing.T) { +func TestApplyTransition_ReturnsHandlerError_ButRunsCommonActions(t *testing.T) { t.Parallel() - endedAt := time.Now().Add(-time.Hour) - state := &State{ - Phase: PhaseEnded, - EndedAt: &endedAt, + 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}, } - // Simulate ENDED → ACTIVE transition (EventTurnStart) - result := Transition(PhaseEnded, EventTurnStart, TransitionContext{}) - remaining := ApplyCommonActions(state, result) + err := ApplyTransition(state, result, handler) - assert.Equal(t, PhaseActive, state.Phase) - assert.Nil(t, state.EndedAt, "EndedAt should be cleared on re-entry") - require.NotNil(t, state.LastInteractionTime) - assert.Empty(t, remaining, "all actions should be consumed") + require.Error(t, err) + assert.Contains(t, err.Error(), "condense 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") } -func TestApplyCommonActions_EndedToIdleOnSessionStart(t *testing.T) { +func TestApplyTransition_StopsOnFirstHandlerError(t *testing.T) { t.Parallel() - endedAt := time.Now().Add(-time.Hour) - state := &State{ - Phase: PhaseEnded, - EndedAt: &endedAt, + state := &State{Phase: PhaseActive} + handler := &mockActionHandler{returnErr: errors.New("condense failed")} + result := TransitionResult{ + NewPhase: PhaseIdle, + Actions: []Action{ActionCondense, ActionWarnStaleSession}, } - // Simulate ENDED → IDLE transition (EventSessionStart re-entry) - result := Transition(PhaseEnded, EventSessionStart, TransitionContext{}) - remaining := ApplyCommonActions(state, result) + err := ApplyTransition(state, result, handler) - assert.Equal(t, PhaseIdle, state.Phase) - assert.Nil(t, state.EndedAt, "EndedAt should be cleared on session re-entry") - assert.Empty(t, remaining, "only ClearEndedAt, which is consumed") + require.Error(t, err) + assert.True(t, handler.condenseCalled) + assert.False(t, handler.warnStaleSessionCalled, "should stop on first error") } -func TestMermaidDiagram(t *testing.T) { +func TestApplyTransition_ClearEndedAtRunsDespiteHandlerError(t *testing.T) { t.Parallel() - diagram := MermaidDiagram() - - // Verify the diagram contains expected state declarations. - assert.Contains(t, diagram, "stateDiagram-v2") - assert.Contains(t, diagram, "IDLE") - assert.Contains(t, diagram, "ACTIVE") - assert.Contains(t, diagram, "ENDED") - assert.NotContains(t, diagram, "ACTIVE_COMMITTED") + endedAt := time.Now().Add(-time.Hour) + state := &State{Phase: PhaseEnded, EndedAt: &endedAt} + handler := &mockActionHandler{returnErr: errors.New("condense failed")} + // Synthetic action list to test the mechanism: no real transition produces + // this exact ordering, but it verifies that ClearEndedAt (common) always + // runs even when a preceding handler action fails. + result := TransitionResult{ + NewPhase: PhaseEnded, + Actions: []Action{ActionCondenseIfFilesTouched, ActionClearEndedAt}, + } - // Verify key transitions are present. - assert.Contains(t, diagram, "idle --> active") - assert.Contains(t, diagram, "active --> active") // ACTIVE+GitCommit stays ACTIVE - assert.Contains(t, diagram, "ended --> idle") - assert.Contains(t, diagram, "ended --> active") + err := ApplyTransition(state, result, handler) - // Verify actions appear in labels. - assert.Contains(t, diagram, "Condense") - assert.Contains(t, diagram, "ClearEndedAt") - assert.Contains(t, diagram, "WarnStaleSession") - assert.NotContains(t, diagram, "MigrateShadowBranch") + require.Error(t, err) + assert.Nil(t, state.EndedAt, "ClearEndedAt must run despite earlier handler error") } diff --git a/cmd/entire/cli/strategy/manual_commit_hooks.go b/cmd/entire/cli/strategy/manual_commit_hooks.go index a2bc4dcf8..0ee6a3287 100644 --- a/cmd/entire/cli/strategy/manual_commit_hooks.go +++ b/cmd/entire/cli/strategy/manual_commit_hooks.go @@ -462,9 +462,73 @@ func (s *ManualCommitStrategy) handleAmendCommitMsg(logCtx context.Context, comm // // Shadow branches are only deleted when ALL sessions sharing the branch are non-active // and were condensed during this PostCommit. + +// postCommitActionHandler implements session.ActionHandler for PostCommit. +// Each session in the loop gets its own handler with per-session context. +// Handler methods use the *State parameter from ApplyTransition (same pointer +// as the state being transitioned) rather than capturing state separately. +type postCommitActionHandler struct { + s *ManualCommitStrategy + logCtx context.Context + repo *git.Repository + checkpointID id.CheckpointID + head *plumbing.Reference + commit *object.Commit + newHead string + shadowBranchName string + shadowBranchesToDelete map[string]struct{} + committedFileSet map[string]struct{} + hasNew bool + + // Output: set by handler methods, read by caller after TransitionAndLog. + condensed bool +} + +func (h *postCommitActionHandler) HandleCondense(state *session.State) error { + // For ACTIVE sessions, any commit during the turn is session-related. + // For IDLE/ENDED sessions (e.g., carry-forward), also require that the + // committed files overlap with the session's remaining files AND have + // matching content — otherwise an unrelated commit (or a commit with + // completely replaced content) would incorrectly get this session's checkpoint. + shouldCondense := h.hasNew + if shouldCondense && !state.Phase.IsActive() { + shouldCondense = filesOverlapWithContent(h.repo, h.shadowBranchName, h.commit, state.FilesTouched) + } + if shouldCondense { + h.condensed = h.s.condenseAndUpdateState(h.logCtx, h.repo, h.checkpointID, state, h.head, h.shadowBranchName, h.shadowBranchesToDelete, h.committedFileSet) + } else { + h.s.updateBaseCommitIfChanged(h.logCtx, state, h.newHead) + } + return nil +} + +func (h *postCommitActionHandler) HandleCondenseIfFilesTouched(state *session.State) error { + if len(state.FilesTouched) > 0 && h.hasNew { + h.condensed = h.s.condenseAndUpdateState(h.logCtx, h.repo, h.checkpointID, state, h.head, h.shadowBranchName, h.shadowBranchesToDelete, h.committedFileSet) + } else { + h.s.updateBaseCommitIfChanged(h.logCtx, state, h.newHead) + } + return nil +} + +func (h *postCommitActionHandler) HandleDiscardIfNoFiles(state *session.State) error { + if len(state.FilesTouched) == 0 { + logging.Debug(h.logCtx, "post-commit: skipping empty ended session (no files to condense)", + slog.String("session_id", state.SessionID), + ) + } + h.s.updateBaseCommitIfChanged(h.logCtx, state, h.newHead) + return nil +} + +func (h *postCommitActionHandler) HandleWarnStaleSession(_ *session.State) error { + // Not produced by EventGitCommit; no-op for exhaustiveness. + return nil +} + // During rebase/cherry-pick/revert operations, phase transitions are skipped entirely. // -//nolint:unparam,maintidx // error return required by interface but hooks must return nil; maintidx: complex but already well-structured +//nolint:unparam // error return required by interface but hooks must return nil func (s *ManualCommitStrategy) PostCommit() error { logCtx := logging.WithComponent(context.Background(), "checkpoint") @@ -559,11 +623,8 @@ func (s *ManualCommitStrategy) PostCommit() error { } transitionCtx.HasFilesTouched = len(state.FilesTouched) > 0 - // Run the state machine transition - remaining := TransitionAndLog(state, session.EventGitCommit, transitionCtx) - - // Save FilesTouched BEFORE the action loop — condensation clears it, - // but we need the original list for carry-forward computation. + // Save FilesTouched BEFORE TransitionAndLog — the handler's condensation + // clears it, but we need the original list for carry-forward computation. // For mid-session commits (ACTIVE, no shadow branch), state.FilesTouched may be empty // because no SaveChanges/Stop has been called yet. Extract files from transcript. filesTouchedBefore := make([]string, len(state.FilesTouched)) @@ -580,55 +641,22 @@ func (s *ManualCommitStrategy) PostCommit() error { slog.Any("files", filesTouchedBefore), ) - condensed := false - - // Dispatch strategy-specific actions. - // Each branch handles its own BaseCommit update so there is no - // fallthrough conditional at the end. On condensation failure, - // BaseCommit is intentionally NOT updated to preserve access to - // the shadow branch (which is named after the old BaseCommit). - for _, action := range remaining { - switch action { - case session.ActionCondense: - // For ACTIVE sessions, any commit during the turn is session-related. - // For IDLE/ENDED sessions (e.g., carry-forward), also require that the - // committed files overlap with the session's remaining files AND have - // matching content — otherwise an unrelated commit (or a commit with - // completely replaced content) would incorrectly get this session's checkpoint. - shouldCondense := hasNew - if shouldCondense && !state.Phase.IsActive() { - shouldCondense = filesOverlapWithContent(repo, shadowBranchName, commit, state.FilesTouched) - } - if shouldCondense { - condensed = s.condenseAndUpdateState(logCtx, repo, checkpointID, state, head, shadowBranchName, shadowBranchesToDelete, committedFileSet) - // condenseAndUpdateState updates BaseCommit on success. - // On failure, BaseCommit is preserved so the shadow branch remains accessible. - } else { - // No new content or unrelated commit — just update BaseCommit - s.updateBaseCommitIfChanged(logCtx, state, newHead) - } - case session.ActionCondenseIfFilesTouched: - // The state machine already gates this action on HasFilesTouched, - // but hasNew is an additional content-level check (transcript has - // new content beyond what was previously condensed). - if len(state.FilesTouched) > 0 && hasNew { - condensed = s.condenseAndUpdateState(logCtx, repo, checkpointID, state, head, shadowBranchName, shadowBranchesToDelete, committedFileSet) - // On failure, BaseCommit is preserved (same as ActionCondense). - } else { - s.updateBaseCommitIfChanged(logCtx, state, newHead) - } - case session.ActionDiscardIfNoFiles: - if len(state.FilesTouched) == 0 { - logging.Debug(logCtx, "post-commit: skipping empty ended session (no files to condense)", - slog.String("session_id", state.SessionID), - ) - } - s.updateBaseCommitIfChanged(logCtx, state, newHead) - case session.ActionClearEndedAt, session.ActionUpdateLastInteraction: - // Handled by session.ApplyCommonActions above - case session.ActionWarnStaleSession: - // Not produced by EventGitCommit; listed for switch exhaustiveness - } + // Run the state machine transition with handler for strategy-specific actions. + handler := &postCommitActionHandler{ + s: s, + logCtx: logCtx, + repo: repo, + checkpointID: checkpointID, + head: head, + commit: commit, + newHead: newHead, + shadowBranchName: shadowBranchName, + shadowBranchesToDelete: shadowBranchesToDelete, + committedFileSet: committedFileSet, + hasNew: hasNew, + } + if err := TransitionAndLog(state, session.EventGitCommit, transitionCtx, handler); err != nil { + fmt.Fprintf(os.Stderr, "[entire] Warning: post-commit action handler error: %v\n", err) } // Record checkpoint ID for ACTIVE sessions so HandleTurnEnd can finalize @@ -636,7 +664,7 @@ func (s *ManualCommitStrategy) PostCommit() error { // NOTE: This check runs AFTER TransitionAndLog updated the phase. It relies on // ACTIVE + GitCommit → ACTIVE (phase stays ACTIVE). If that state machine // transition ever changed, this guard would silently stop recording IDs. - if condensed && state.Phase.IsActive() { + if handler.condensed && state.Phase.IsActive() { state.TurnCheckpointIDs = append(state.TurnCheckpointIDs, checkpointID.String()) } @@ -645,7 +673,7 @@ func (s *ManualCommitStrategy) PostCommit() error { // commit across two `git commit` invocations, each gets a 1:1 checkpoint. // Uses content-aware comparison: if user did `git add -p` and committed // partial changes, the file still has remaining agent changes to carry forward. - if condensed { + if handler.condensed { remainingFiles := filesWithRemainingAgentChanges(repo, shadowBranchName, commit, filesTouchedBefore, committedFileSet) logging.Debug(logCtx, "post-commit: carry-forward decision (content-aware)", slog.String("session_id", state.SessionID), @@ -666,7 +694,7 @@ func (s *ManualCommitStrategy) PostCommit() error { // Only preserve shadow branch for active sessions that were NOT condensed. // Condensed sessions already have their data on entire/checkpoints/v1. - if state.Phase.IsActive() && !condensed { + if state.Phase.IsActive() && !handler.condensed { uncondensedActiveOnBranch[shadowBranchName] = true } } @@ -1255,8 +1283,10 @@ func (s *ManualCommitStrategy) InitializeSession(sessionID string, agentType age } if state != nil && state.BaseCommit != "" { - // Session is fully initialized — apply phase transition for TurnStart - TransitionAndLog(state, session.EventTurnStart, session.TransitionContext{}) + // Session is fully initialized — apply phase transition for TurnStart. + if transErr := TransitionAndLog(state, session.EventTurnStart, session.TransitionContext{}, session.NoOpActionHandler{}); transErr != nil { + fmt.Fprintf(os.Stderr, "[entire] Warning: turn start transition failed: %v\n", transErr) + } // Generate a new TurnID for each turn (correlates carry-forward checkpoints) turnID, err := id.Generate() @@ -1314,8 +1344,10 @@ func (s *ManualCommitStrategy) InitializeSession(sessionID string, agentType age return fmt.Errorf("failed to initialize session: %w", err) } - // Apply phase transition: new session starts as ACTIVE - TransitionAndLog(state, session.EventTurnStart, session.TransitionContext{}) + // Apply phase transition: new session starts as ACTIVE. + if transErr := TransitionAndLog(state, session.EventTurnStart, session.TransitionContext{}, session.NoOpActionHandler{}); transErr != nil { + fmt.Fprintf(os.Stderr, "[entire] Warning: turn start transition failed: %v\n", transErr) + } // Calculate attribution for pre-prompt edits // This captures any user edits made before the first prompt diff --git a/cmd/entire/cli/strategy/phase_postcommit_test.go b/cmd/entire/cli/strategy/phase_postcommit_test.go index d321ca34f..db46f61fe 100644 --- a/cmd/entire/cli/strategy/phase_postcommit_test.go +++ b/cmd/entire/cli/strategy/phase_postcommit_test.go @@ -639,11 +639,10 @@ func TestTurnEnd_Active_NoActions(t *testing.T) { // ACTIVE + TurnEnd → IDLE with no strategy-specific actions result := session.Transition(state.Phase, session.EventTurnEnd, session.TransitionContext{}) - remaining := session.ApplyCommonActions(state, result) - // Verify no strategy-specific actions for ACTIVE → IDLE - assert.Empty(t, remaining, - "ACTIVE + TurnEnd should not emit strategy-specific actions") + // Apply transition with no-op handler (no strategy actions for ACTIVE → IDLE) + err = session.ApplyTransition(state, result, session.NoOpActionHandler{}) + require.NoError(t, err) // Call HandleTurnEnd — should be a no-op (no TurnCheckpointIDs) err = s.HandleTurnEnd(state) diff --git a/cmd/entire/cli/strategy/session_state.go b/cmd/entire/cli/strategy/session_state.go index 9d9a3b18b..86412a264 100644 --- a/cmd/entire/cli/strategy/session_state.go +++ b/cmd/entire/cli/strategy/session_state.go @@ -167,16 +167,26 @@ func FindMostRecentSession() string { return "" } -// TransitionAndLog runs a session phase transition, applies common actions, logs the -// transition, and returns any remaining strategy-specific actions. +// TransitionAndLog runs a session phase transition, applies actions via the +// handler, and logs the transition. Returns the first handler error from +// ApplyTransition (if any) so callers can surface it. The error is also +// logged internally for diagnostics. // This is the single entry point for all state machine transitions to ensure // consistent logging of phase changes. -func TransitionAndLog(state *SessionState, event session.Event, ctx session.TransitionContext) []session.Action { +func TransitionAndLog(state *SessionState, event session.Event, ctx session.TransitionContext, handler session.ActionHandler) error { oldPhase := state.Phase result := session.Transition(oldPhase, event, ctx) - remaining := session.ApplyCommonActions(state, result) - logCtx := logging.WithComponent(context.Background(), "session") + + handlerErr := session.ApplyTransition(state, result, handler) + if handlerErr != nil { + logging.Error(logCtx, "action handler error during transition", + slog.String("session_id", state.SessionID), + slog.String("event", event.String()), + slog.Any("error", handlerErr), + ) + } + if result.NewPhase != oldPhase { logging.Info(logCtx, "phase transition", slog.String("session_id", state.SessionID), @@ -192,7 +202,10 @@ func TransitionAndLog(state *SessionState, event session.Event, ctx session.Tran ) } - return remaining + if handlerErr != nil { + return fmt.Errorf("transition %s: %w", event, handlerErr) + } + return nil } // ClearSessionState removes the session state file for the given session ID. diff --git a/cmd/entire/cli/strategy/session_state_test.go b/cmd/entire/cli/strategy/session_state_test.go index 0275977e9..5c598fa97 100644 --- a/cmd/entire/cli/strategy/session_state_test.go +++ b/cmd/entire/cli/strategy/session_state_test.go @@ -1,10 +1,12 @@ package strategy import ( + "errors" "os" "testing" "time" + "github.com/entireio/cli/cmd/entire/cli/session" "github.com/go-git/go-git/v5" ) @@ -380,3 +382,36 @@ func TestFindMostRecentSession_FallsBackWhenNoWorktreeMatch(t *testing.T) { t.Logf("cleanup warning: %v", err) } } + +// errorActionHandler returns an error from HandleCondense to test +// that TransitionAndLog propagates handler errors while still applying the phase transition. +type errorActionHandler struct { + session.NoOpActionHandler +} + +func (errorActionHandler) HandleCondense(_ *session.State) error { + return errors.New("test condense error") +} + +// TestTransitionAndLog_ReturnsHandlerError verifies that TransitionAndLog +// applies the phase transition even when the handler returns an error, +// and propagates that error to the caller. +func TestTransitionAndLog_ReturnsHandlerError(t *testing.T) { + t.Parallel() + + state := &SessionState{ + SessionID: "test-error-handler", + Phase: session.PhaseIdle, + } + + // IDLE + GitCommit → IDLE with ActionCondense. + // The handler will fail on ActionCondense, but the phase should still be IDLE. + err := TransitionAndLog(state, session.EventGitCommit, session.TransitionContext{}, &errorActionHandler{}) + + if state.Phase != session.PhaseIdle { + t.Errorf("Phase = %q, want %q (should transition despite handler error)", state.Phase, session.PhaseIdle) + } + if err == nil { + t.Error("TransitionAndLog() should return handler error") + } +} diff --git a/cmd/entire/cli/strategy/strategy.go b/cmd/entire/cli/strategy/strategy.go index 7866a74fc..9f1a8fa1b 100644 --- a/cmd/entire/cli/strategy/strategy.go +++ b/cmd/entire/cli/strategy/strategy.go @@ -464,7 +464,7 @@ type PrePushHandler interface { type TurnEndHandler interface { // HandleTurnEnd performs strategy-specific cleanup at the end of a turn. // Work items are read from state (e.g. TurnCheckpointIDs), not from the - // action list. The state has already been updated by ApplyCommonActions; + // action list. The state has already been updated by ApplyTransition; // the caller saves it after this method returns. HandleTurnEnd(state *session.State) error }