Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
dimakis merged 3 commits into from
Jul 3, 2026
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d11a2c2
fix(chat): echo closeout prompts to frontend as user bubbles
dimakis Jul 2, 2026
5654fda
fix: address Centaur review — extract helper, add tests, debug log
dimakis Jul 2, 2026
577fc2b
fix: address Centaur review — robust test boundaries
dimakis Jul 2, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 43 additions & 0 deletions server/__tests__/chat.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -505,7 +505,7 @@
it('appends user_message to eventStore before runQueryLoop', () => {
const appendIdx = chatSource.indexOf("eventStore.append(options.resume, 'user_message'");
const queryLoopIdx = chatSource.indexOf('await runQueryLoop(');
expect(appendIdx).toBeGreaterThan(-1);

Check failure on line 508 in server/__tests__/chat.test.ts

View workflow job for this annotation

GitHub Actions / ci

server/__tests__/chat.test.ts > startChat stores user message for resumed sessions > appends user_message to eventStore before runQueryLoop 

AssertionError: expected -1 to be greater than -1
 ❯ server/__tests__/chat.test.ts:508:23
expect(queryLoopIdx).toBeGreaterThan(-1);
expect(appendIdx).toBeLessThan(queryLoopIdx);
});
Expand All @@ -521,7 +521,7 @@
expect(start).toBeGreaterThan(-1);
expect(end).toBeGreaterThan(start);
const region = chatSource.slice(start, end);
expect(region).toContain("eventStore.append(options.resume, 'user_message'");

Check failure on line 524 in server/__tests__/chat.test.ts

View workflow job for this annotation

GitHub Actions / ci

server/__tests__/chat.test.ts > startChat stores user message for resumed sessions > echoes user_message to transport and broadcasts to observers 

AssertionError: expected 'if (options.resume) {\n      const me…' to contain 'eventStore.append(options.resume, \'u…'

- Expected
+ Received

- eventStore.append(options.resume, 'user_message'
+ if (options.resume) {
+       const messageId =
+         options.clientMsgId || `umsg-${Date.now()}-${randomUUID().slice(0, 8)}-resume`;
+       storeAndEchoIfNew(
+         options.resume,
+         messageId,
+         fullPrompt,
+         clientId,
+         transport,
+         session.observers,
+       );
+     }
+
+     

 ❯ server/__tests__/chat.test.ts:524:20
expect(region).toContain('send(transport');
expect(region).toContain("type: 'user_message'");
expect(region).toContain('broadcastToObservers(session.observers');
Expand Down Expand Up @@ -824,3 +824,46 @@
expect(['contexgin', 'local', 'fallback']).toContain(session.agentDefinitionSource);
});
});

describe('closeout prompts echo to frontend', () => {

@dimakis dimakis Jul 2, 2026

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

🔵 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 dimakis Jul 2, 2026

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

🔵 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.

let chatSource: string;

beforeAll(async () => {
const { readFileSync } = await import('fs');
const { join } = await import('path');
chatSource = readFileSync(join(import.meta.dirname, '..', 'chat.ts'), 'utf-8');
});

it('echoCloseoutPrompt helper calls storeAndEchoIfNew', () => {

@dimakis dimakis Jul 2, 2026

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

🔵 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]

const fnStart = chatSource.indexOf('function echoCloseoutPrompt(');
expect(fnStart).toBeGreaterThan(-1);
const fnEnd = chatSource.indexOf('\n}', fnStart);
const fnBody = chatSource.slice(fnStart, fnEnd);
expect(fnBody).toContain('storeAndEchoIfNew(');
expect(fnBody).toContain("log.debug('skipping closeout echo");
});

it('auto-closeout calls echoCloseoutPrompt before inputQueue.push', () => {
const fnStart = chatSource.indexOf('function _closeoutSessionInner(');
expect(fnStart).toBeGreaterThan(-1);
const fnEnd = chatSource.indexOf('\n}', fnStart);

@dimakis dimakis Jul 2, 2026

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

🔵 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]

const fnBody = chatSource.slice(fnStart, fnEnd);
const echoIdx = fnBody.indexOf('echoCloseoutPrompt(');
const pushIdx = fnBody.indexOf('session.inputQueue.push(');
expect(echoIdx).toBeGreaterThan(-1);
expect(pushIdx).toBeGreaterThan(-1);
expect(echoIdx).toBeLessThan(pushIdx);
});

it('user-closeout calls echoCloseoutPrompt before inputQueue.push', () => {
const fnStart = chatSource.indexOf('export function closeSessionByUser(');
expect(fnStart).toBeGreaterThan(-1);
const fnEnd = chatSource.indexOf('\nexport function', fnStart + 1);
const fnBody = chatSource.slice(fnStart, fnEnd > -1 ? fnEnd : undefined);
const echoIdx = fnBody.indexOf('echoCloseoutPrompt(');
const pushIdx = fnBody.indexOf('session.inputQueue.push(');
expect(echoIdx).toBeGreaterThan(-1);
expect(pushIdx).toBeGreaterThan(-1);
expect(echoIdx).toBeLessThan(pushIdx);
});
});
25 changes: 23 additions & 2 deletions server/chat.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1355,6 +1355,27 @@ export function cleanupSessionWorktrees(
if (primary) session.worktreePaths.set('primary', primary);
}

/** Echo a closeout prompt to the frontend as a user bubble before injecting into the SDK. */
function echoCloseoutPrompt(
session: import('./session-registry.js').ManagedSession,
clientId: string,
prompt: string,
): void {
const messageId = `umsg-${Date.now()}-${randomUUID().slice(0, 8)}-closeout`;
if (session.sessionId) {
storeAndEchoIfNew(

@dimakis dimakis Jul 2, 2026

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

🔵 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.

session.sessionId,
messageId,
prompt,
clientId,

@dimakis dimakis Jul 2, 2026

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

🔵 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.

session.transport,
session.observers,
);
} else {
log.debug('skipping closeout echo — session not yet resolved', { clientId });
}
}

const CLOSEOUT_PROMPT = `This session is closing in 10 minutes due to inactivity.
Please perform session closeout:

Expand Down Expand Up @@ -1410,7 +1431,7 @@ function _closeoutSessionInner(clientId: string): void {

log.info('injecting closeout prompt', { clientId, wtId: session.wtId });

@dimakis dimakis Jul 2, 2026

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

🟡 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 dimakis Jul 2, 2026

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

🔵 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]

// Push the closeout prompt as an interrupt so the agent sees it immediately
echoCloseoutPrompt(session, clientId, CLOSEOUT_PROMPT);
session.inputQueue.push(makeUserMessage(CLOSEOUT_PROMPT, 'now'));

// The registry's CLOSEOUT_TIMEOUT_MS timer will abort the session after
Expand Down Expand Up @@ -1495,7 +1516,7 @@ export function closeSessionByUser(clientId: string): void {

log.info('user-initiated closeout', { clientId, wtId: session.wtId });

// Inject closeout prompt
echoCloseoutPrompt(session, clientId, USER_CLOSEOUT_PROMPT);
session.inputQueue.push(makeUserMessage(USER_CLOSEOUT_PROMPT, 'now'));

// Register abort listener to finalize with closed_by: 'user'
Expand Down
Loading