fix(p0): Stop hook transitions to COMPLETED on explicit signal (CL-3)#24
Conversation
…CL-3) Closes the P0 #3 issue from the 2026-05-08 CTO audit. PRD §CL-3 requires the Stop hook to transition a session to COMPLETED. The previous implementation only armed the idle timer; sessions never reached the COMPLETED state through any code path. Now `_handle_stop` checks the payload for explicit-completion signals: - `final: true` - `session_ended: true` - `reason: "session_complete"` or `"completed"` When any is present, transitions ACTIVE/etc → COMPLETED (idempotent when already COMPLETED). Without a signal, preserves the original behavior: arms the idle timer so tick_idle_check eventually flips to WAITING. bridge/tests/test_stop_to_completed.py — 8 tests covering each completion signal, the no-signal idle-timer fallback (with the WAITING transition fired by tick_idle_check), unrelated reasons, idempotence on already-COMPLETED, and unknown-session no-op. Total: 167 passing (was 159; adds 8). Refs: CTO audit doc PR #22, kanban t_26404be3. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Code Review
This pull request implements the transition of sessions to a COMPLETED state when explicit completion signals (such as 'final' or 'session_ended') are received in Stop events, as per PRD §CL-3. It also adds a comprehensive test suite to verify these transitions and ensure backward compatibility for idle-timer logic. Feedback was provided regarding a potential memory leak in the HookListener, as session IDs are currently added to the _last_stop_time dictionary even for terminal states without being cleaned up; a code suggestion was provided to pop these entries when a session is finalized.
| # Always record the stop time so tick_idle_check can fire if no | ||
| # subsequent productive event arrives. | ||
| listener._last_stop_time[session.session_id] = time.time() | ||
|
|
||
| # Inspect payload for explicit-completion signals. | ||
| is_final = bool( | ||
| payload.get("final") | ||
| or payload.get("session_ended") | ||
| or payload.get("reason") == "session_complete" | ||
| or payload.get("reason") == "completed" | ||
| ) | ||
| if is_final and session.status != SessionStatus.COMPLETED: | ||
| return [listener._set_status(session, SessionStatus.COMPLETED, note="stop_final")] | ||
| return [] |
There was a problem hiding this comment.
The current implementation always records the stop time in _last_stop_time, even when the session is transitioning to COMPLETED. Since COMPLETED is a terminal state and tick_idle_check only processes ACTIVE sessions, these entries will persist indefinitely in the dictionary, leading to a memory leak in the HookListener instance over time.
Additionally, the Session model has a completed_at field (see bridge/session_state.py) that should ideally be populated when a session is completed to maintain data consistency.
I suggest moving the idle timer arming logic into the else block and ensuring the timer is cleared if the session is final.
| # Always record the stop time so tick_idle_check can fire if no | |
| # subsequent productive event arrives. | |
| listener._last_stop_time[session.session_id] = time.time() | |
| # Inspect payload for explicit-completion signals. | |
| is_final = bool( | |
| payload.get("final") | |
| or payload.get("session_ended") | |
| or payload.get("reason") == "session_complete" | |
| or payload.get("reason") == "completed" | |
| ) | |
| if is_final and session.status != SessionStatus.COMPLETED: | |
| return [listener._set_status(session, SessionStatus.COMPLETED, note="stop_final")] | |
| return [] | |
| # Inspect payload for explicit-completion signals. | |
| is_final = bool( | |
| payload.get("final") | |
| or payload.get("session_ended") | |
| or payload.get("reason") == "session_complete" | |
| or payload.get("reason") == "completed" | |
| ) | |
| if is_final: | |
| # Terminal state: ensure the idle timer is disarmed to prevent memory leaks. | |
| listener._last_stop_time.pop(session.session_id, None) | |
| if session.status != SessionStatus.COMPLETED: | |
| return [listener._set_status(session, SessionStatus.COMPLETED, note="stop_final")] | |
| else: | |
| # Non-terminal stop: arm the idle timer so tick_idle_check can flip to WAITING. | |
| listener._last_stop_time[session.session_id] = time.time() | |
| return [] |
Closes P0 #3 from the 2026-05-08 CTO audit. Stop with
final/session_ended/reason=session_complete/reason=completed→ COMPLETED. Without those signals, preserves existing arm-idle-timer behavior. 8 new tests; 167 total.Refs: PR #22 (audit), kanban t_26404be3.