fix(chat): echo closeout prompts to frontend as user bubbles

[dimakis]

Summary

• Both closeoutSession() (auto TTL expiry) and closeSessionByUser() (manual close) pushed the closeout prompt directly into the SDK inputQueue without calling storeAndEchoIfNew(), so the agent processed the closeout but no message bubble appeared in the chat UI
• Added the missing storeAndEchoIfNew() calls before the inputQueue.push() in both paths, matching the pattern used by sendToChat() and interruptChat()

Test plan

• Let a session TTL expire — verify the closeout prompt appears as a user bubble in the chat
• Manually close a session — verify the user-initiated closeout prompt appears as a user bubble
• Verify the agent still processes the closeout (commits, PRs, memory) as before
• Verify session rejoin replays the closeout message from the event store

🤖 Generated with Claude Code

[dimakis]

Centaur Review

Found 3 issue(s) (1 warning).

server/chat.ts

Clean, focused fix that correctly follows the existing storeAndEchoIfNew pattern. Main gap is missing test coverage for the new echo behavior in both closeout paths.

• 🟡 missing_tests (L1412): The new echo behavior in both _closeoutSessionInner and closeSessionByUser has no test coverage. Neither function has existing unit tests in server/__tests__/chat.test.ts. At minimum, a test should verify that storeAndEchoIfNew is called with the correct prompt text and that the echo precedes the inputQueue.push. [fixable]
• 🔵 style (L1412): The two added blocks (lines 1412-1423 and 1510-1521) are nearly identical — same message ID construction, same storeAndEchoIfNew call shape, differing only in the prompt constant. Consider extracting a small helper like echoCloseoutPrompt(session, clientId, prompt) to reduce duplication, consistent with how this codebase prefers short focused functions. [fixable]
• 🔵 unsafe_assumptions (L1415): Unlike sendToChat (line 1214) and interruptChat (line 1254), which fall back to direct send() + broadcastToObservers() when session.sessionId is null, these closeout blocks silently skip the echo entirely. This is safe in practice (sessions will have resolved long before closeout), but the inconsistency with the established pattern could confuse future readers. A debug log in the else branch would make the intent explicit. [fixable]

[dimakis]

🟡 missing_tests: The new echo behavior in both _closeoutSessionInner and closeSessionByUser has no test coverage. Neither function has existing unit tests in server/__tests__/chat.test.ts. At minimum, a test should verify that storeAndEchoIfNew is called with the correct prompt text and that the echo precedes the inputQueue.push. [fixable]

[dimakis]

🔵 style: The two added blocks (lines 1412-1423 and 1510-1521) are nearly identical — same message ID construction, same storeAndEchoIfNew call shape, differing only in the prompt constant. Consider extracting a small helper like echoCloseoutPrompt(session, clientId, prompt) to reduce duplication, consistent with how this codebase prefers short focused functions. [fixable]

[dimakis]

🔵 unsafe_assumptions: Unlike sendToChat (line 1214) and interruptChat (line 1254), which fall back to direct send() + broadcastToObservers() when session.sessionId is null, these closeout blocks silently skip the echo entirely. This is safe in practice (sessions will have resolved long before closeout), but the inconsistency with the established pattern could confuse future readers. A debug log in the else branch would make the intent explicit. [fixable]

[dimakis]

Centaur Review

Found 3 issue(s) (1 warning).

server/__tests__/chat.test.ts

Implementation is correct and follows established patterns. The || with indexOf in the test is a latent bug (works by accident today), and the \nregistry. boundary marker is fragile — both are low-risk but worth fixing.

• 🟡 bugs (L859): || is wrong with indexOf: -1 (not found) is truthy in JS, so if the first search fails the fallback never executes. Currently masked because 'function closeSessionByUser(' is a substring of 'export function closeSessionByUser(' so the first search always succeeds. Should use a -1 check: let fnStart = chatSource.indexOf('function closeSessionByUser('); if (fnStart === -1) fnStart = chatSource.indexOf('export function closeSessionByUser('); [fixable]
• 🔵 style (L861): Using '\nregistry.' as the function boundary marker is fragile and opaque compared to the existing convention in this file (e.g., lines 516-522 use two known code landmarks to bound a region). If any module-level registry. call is added, removed, or reordered, this test silently captures the wrong slice. Consider using the pattern chatSource.indexOf('\n}', fnStart) (column-0 closing brace) which works for top-level functions and is used in the _closeoutSessionInner test above. [fixable]
• 🔵 missing_tests (L837): The structural tests verify code ordering but don't cover the sessionId guard branch. A small structural assertion that the function body contains the else { log.debug('skipping closeout echo path for the auto-closeout and user-closeout callers would lock in the guard behavior. Alternatively, a unit test of echoCloseoutPrompt itself with a session where sessionId is undefined would verify the fallback path. [fixable]

[dimakis]

🟡 bugs: || is wrong with indexOf: -1 (not found) is truthy in JS, so if the first search fails the fallback never executes. Currently masked because 'function closeSessionByUser(' is a substring of 'export function closeSessionByUser(' so the first search always succeeds. Should use a -1 check: let fnStart = chatSource.indexOf('function closeSessionByUser('); if (fnStart === -1) fnStart = chatSource.indexOf('export function closeSessionByUser('); [fixable]

[dimakis]

🔵 style: Using '\nregistry.' as the function boundary marker is fragile and opaque compared to the existing convention in this file (e.g., lines 516-522 use two known code landmarks to bound a region). If any module-level registry. call is added, removed, or reordered, this test silently captures the wrong slice. Consider using the pattern chatSource.indexOf('\n}', fnStart) (column-0 closing brace) which works for top-level functions and is used in the _closeoutSessionInner test above. [fixable]

[dimakis]

🔵 missing_tests: The structural tests verify code ordering but don't cover the sessionId guard branch. A small structural assertion that the function body contains the else { log.debug('skipping closeout echo path for the auto-closeout and user-closeout callers would lock in the guard behavior. Alternatively, a unit test of echoCloseoutPrompt itself with a session where sessionId is undefined would verify the fallback path. [fixable]

[dimakis]

Centaur Review

Found 4 issue(s) (2 warning).

server/__tests__/chat.test.ts

Implementation is clean and follows existing patterns well; the main concerns are fragile source-text test boundaries (fnEnd logic) and unreachable dead code in the || fallback.

• 🟡 style (L859): The || fallback is dead code. chatSource.indexOf('function closeSessionByUser(') always finds the substring within export function closeSessionByUser( and returns a positive (truthy) number, so the right side of || never executes. Use a single search for 'export function closeSessionByUser(' instead. [fixable]
• 🟡 bugs (L861): The function boundary chatSource.indexOf('\nregistry.', fnStart) returns -1 because the only registry. at column 0 (line 1468) is before fnStart. slice(fnStart, -1) then captures nearly the entire remainder of the file, not just the closeSessionByUser function. The test still passes because the assertions happen to hold across this larger range, but it's testing a much wider scope than intended and would silently break if another echoCloseoutPrompt( appeared later in the file. Consider using chatSource.indexOf('\nexport function', fnStart + 1) or '\n}\n' as a more robust end delimiter. [fixable]
• 🔵 missing_tests (L828): The tests are source-text assertions (grepping the .ts file as a string) rather than behavioral tests that exercise the actual code paths. This means they verify structure but not runtime behavior — e.g., they can't catch issues like storeAndEchoIfNew being called with wrong arguments, the echo containing incorrect content, or the transport being closed. A behavioral test that mocks storeAndEchoIfNew (or the transport/event store) and calls closeSessionByUser/closeoutSession would provide stronger guarantees.

server/chat.ts

Implementation is clean and follows existing patterns well; the main concerns are fragile source-text test boundaries (fnEnd logic) and unreachable dead code in the || fallback.

• 🔵 unsafe_assumptions (L1366): In auto-closeout (_closeoutSessionInner), the session is detached, meaning transport.isOpen() is likely false. The send(transport, echo) inside storeAndEchoIfNew will be a no-op in that case. The echo is still stored in the event store and broadcast to observers, so it's not lost — but worth noting that auto-closeout echoes may not reach the original client if it has already disconnected.

[dimakis]

🟡 style: The || fallback is dead code. chatSource.indexOf('function closeSessionByUser(') always finds the substring within export function closeSessionByUser( and returns a positive (truthy) number, so the right side of || never executes. Use a single search for 'export function closeSessionByUser(' instead. [fixable]

[dimakis]

🟡 bugs: The function boundary chatSource.indexOf('\nregistry.', fnStart) returns -1 because the only registry. at column 0 (line 1468) is before fnStart. slice(fnStart, -1) then captures nearly the entire remainder of the file, not just the closeSessionByUser function. The test still passes because the assertions happen to hold across this larger range, but it's testing a much wider scope than intended and would silently break if another echoCloseoutPrompt( appeared later in the file. Consider using chatSource.indexOf('\nexport function', fnStart + 1) or '\n}\n' as a more robust end delimiter. [fixable]

[dimakis]

🔵 missing_tests: The tests are source-text assertions (grepping the .ts file as a string) rather than behavioral tests that exercise the actual code paths. This means they verify structure but not runtime behavior — e.g., they can't catch issues like storeAndEchoIfNew being called with wrong arguments, the echo containing incorrect content, or the transport being closed. A behavioral test that mocks storeAndEchoIfNew (or the transport/event store) and calls closeSessionByUser/closeoutSession would provide stronger guarantees.

[dimakis]

🔵 unsafe_assumptions: In auto-closeout (_closeoutSessionInner), the session is detached, meaning transport.isOpen() is likely false. The send(transport, echo) inside storeAndEchoIfNew will be a no-op in that case. The echo is still stored in the event store and broadcast to observers, so it's not lost — but worth noting that auto-closeout echoes may not reach the original client if it has already disconnected.

[dimakis]

Centaur Review

Found 3 issue(s).

server/__tests__/chat.test.ts

Clean, correct implementation that follows established patterns. echoCloseoutPrompt correctly guards against unresolved sessions, uses the proven storeAndEchoIfNew dedup/echo path, and both callsites maintain the right ordering (echo before inputQueue.push). The structural tests are consistent with existing conventions but would benefit from a companion behavioral test.

• 🔵 missing_tests (L828): All three tests are structural source-code string assertions (read chat.ts as text, indexOf/slice, check for substrings). They verify that certain tokens exist in the source but cannot catch parameter mismatches, wrong argument ordering in the storeAndEchoIfNew call, or regressions in runtime behavior. A unit test that constructs a minimal ManagedSession stub, calls echoCloseoutPrompt, and asserts that the transport received a user_message echo would cover these gaps. That said, this pattern is already established in this test file (lines 481–577), so it is consistent with project conventions.
• 🔵 style (L849): The _closeoutSessionInner test uses chatSource.indexOf('\n}', fnStart) to find the function boundary. This relies on all inner braces being indented (currently true). The closeSessionByUser test uses the more robust \nexport function boundary. Consider using a similar next-function-declaration boundary for _closeoutSessionInner for consistency (e.g., searching for \n// Wire closeout or \nexport function instead of \n}). [fixable]

server/chat.ts

Clean, correct implementation that follows established patterns. echoCloseoutPrompt correctly guards against unresolved sessions, uses the proven storeAndEchoIfNew dedup/echo path, and both callsites maintain the right ordering (echo before inputQueue.push). The structural tests are consistent with existing conventions but would benefit from a companion behavioral test.

• 🔵 unsafe_assumptions (L1370): When session.sessionId is falsy, echoCloseoutPrompt silently skips the echo (only logs). By contrast, sendToChat (line 1218) still echoes the message to the transport even without a resolved sessionId. In practice this is safe — a closeout fires after 10+ minutes of inactivity so the session should always be resolved by then — but the asymmetry is worth documenting. The current defensive skip is arguably the better choice to avoid orphaned event-store entries.

[dimakis]

🔵 missing_tests: All three tests are structural source-code string assertions (read chat.ts as text, indexOf/slice, check for substrings). They verify that certain tokens exist in the source but cannot catch parameter mismatches, wrong argument ordering in the storeAndEchoIfNew call, or regressions in runtime behavior. A unit test that constructs a minimal ManagedSession stub, calls echoCloseoutPrompt, and asserts that the transport received a user_message echo would cover these gaps. That said, this pattern is already established in this test file (lines 481–577), so it is consistent with project conventions.

[dimakis]

🔵 style: The _closeoutSessionInner test uses chatSource.indexOf('\n}', fnStart) to find the function boundary. This relies on all inner braces being indented (currently true). The closeSessionByUser test uses the more robust \nexport function boundary. Consider using a similar next-function-declaration boundary for _closeoutSessionInner for consistency (e.g., searching for \n// Wire closeout or \nexport function instead of \n}). [fixable]

[dimakis]

🔵 unsafe_assumptions: When session.sessionId is falsy, echoCloseoutPrompt silently skips the echo (only logs). By contrast, sendToChat (line 1218) still echoes the message to the transport even without a resolved sessionId. In practice this is safe — a closeout fires after 10+ minutes of inactivity so the session should always be resolved by then — but the asymmetry is worth documenting. The current defensive skip is arguably the better choice to avoid orphaned event-store entries.