Skip to content

fix(cli): prevent duplicate SessionStart systemMessage render #25827

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
jacob314 merged 2 commits into from
May 11, 2026
Merged

fix(cli): prevent duplicate SessionStart systemMessage render #25827

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
cb65b25
fix(cli): prevent duplicate SessionStart systemMessage render
dimssu Apr 22, 2026
c05ccbe
Merge branch 'main' into fix/issue-25655-sessionstart-duplicate-syste…
jacob314 May 11, 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
36 changes: 36 additions & 0 deletions packages/cli/src/ui/AppContainer.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1267,6 +1267,42 @@ describe('AppContainer State Management', () => {
});
});

describe('SessionStart Hook Rendering', () => {
it('does not render systemMessage directly (avoids duplicate with HookSystemMessage event)', async () => {
const mockAddItem = vi.fn();
mockedUseHistory.mockReturnValue({
history: [],
addItem: mockAddItem,
updateItem: vi.fn(),
clearItems: vi.fn(),
loadHistory: vi.fn(),
});

const fireSessionStartEvent = vi.fn().mockResolvedValue({
systemMessage: 'Hello from SessionStart hook',
getAdditionalContext: vi.fn(() => undefined),
});
vi.spyOn(mockConfig, 'getHookSystem').mockReturnValue({
fireSessionEndEvent: vi.fn().mockResolvedValue(undefined),
fireSessionStartEvent,
} as unknown as ReturnType<Config['getHookSystem']>);

const { unmount } = await act(async () => renderAppContainer());
await waitFor(() => expect(fireSessionStartEvent).toHaveBeenCalled());

// The direct-render path (the bug) would call addItem with the
// systemMessage text and no `source` field. The HookSystemMessage
// event-listener path (the correct one) always sets `source`.
const directRenderCall = mockAddItem.mock.calls.find(
([item]) =>
item?.text === 'Hello from SessionStart hook' && !item?.source,
);
expect(directRenderCall).toBeUndefined();

unmount();
});
});

describe('Token Counting from Session Stats', () => {
it('tracks token counts from session messages', async () => {
// Session stats are provided through the SessionStatsProvider context
Expand Down
16 changes: 0 additions & 16 deletions packages/cli/src/ui/AppContainer.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -497,16 +497,6 @@ export const AppContainer = (props: AppContainerProps) => {
?.fireSessionStartEvent(sessionStartSource);

if (result) {
if (result.systemMessage) {

@kamal2730 kamal2730 Apr 23, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Hey @dimssu, I think there might be a small issue with removing that block.

The outputFormat === 'json' check is specific to hook outputs, but result.systemMessage in AppContainer.tsx comes from a different flow (like SessionStartEvent) and isn’t necessarily tied to hook-based JSON output.

Because of that, relying only on the HookSystemMessage listener might miss some cases where systemMessage is set directly on result. The previous logic ensured those messages were added to history regardless of hooks.

This hook-based handling is here (which explains part of the behavior), and I can also see it being rendered after that:
hookEventHandler.ts (L462–L469)

So removing this could lead to some system messages not being shown. Maybe we should verify whether all systemMessage cases are indeed covered by the hook event before removing it?

@dimssu dimssu Apr 23, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good catch, you're right. I traced it: plain-text hook outputs go through convertPlainTextToHookOutput in hookRunner.ts, which does set systemMessage, but the emit in hookEventHandler.ts:463 is gated by outputFormat === 'json', so those never reach the listener. My patch would silently drop them.

Side note while I was digging: BeforeAgent/BeforeTool plain-text hooks already don't render their systemMessage anywhere (no direct-render path for those either), so the same latent gap exists for them today.

My thinking: drop the outputFormat === 'json' check in hookEventHandler so the event bus carries both formats. Then the AppContainer removal stays clean, and BeforeAgent/BeforeTool also start surfacing text-hook messages consistently — covers this bug without leaving the adjacent one. Let me know if that sounds reasonable or if you'd rather keep it narrower.

@jacob314 jacob314 Apr 23, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Please address this and then I will approve. Would also encourage you to get Gemini to write some tests to help verify we aren't missing cases.

@dimssu dimssu Apr 23, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks @jacob314. Just pushed the fix (force-pushed onto the same branch — head is now cb65b25ac).

On the approach: dropped the outputFormat === 'json' gate in hookEventHandler.ts so the HookSystemMessage event fires for both JSON and plain-text hooks. That way the AppContainer direct-render removal no longer regresses plain-text SessionStart hooks, and as a side effect plain-text BeforeAgent / BeforeTool hooks also start surfacing their messages (they silently dropped before).

On tests: added three cases in hookEventHandler.test.ts under a new systemMessage event emission block — JSON-format emission, text-format emission, and the no-systemMessage no-op. The existing AppContainer.test.tsx case that guards the direct-render path from coming back is still there. Also re-ran the PTY repro locally for both a JSON hook and a plain-text hook; both render exactly once now.

Happy to add more coverage if there's a specific case you'd like me to pin down.

historyManager.addItem(
{
type: MessageType.INFO,
text: result.systemMessage,
},
Date.now(),
);
}

const additionalContext = result.getAdditionalContext();
const geminiClient = config.getGeminiClient();
if (additionalContext && geminiClient) {
Expand Down Expand Up @@ -549,12 +539,6 @@ export const AppContainer = (props: AppContainerProps) => {
debugLogger.error('Error during cleanup:', e),
);
};
// Disable the dependencies check here. historyManager gets flagged
// but we don't want to react to changes to it because each new history
// item, including the ones from the start session hook will cause a
// re-render and an error when we try to reload config.
//
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [config, resumedSessionData]);

useEffect(
Expand Down
97 changes: 97 additions & 0 deletions packages/core/src/hooks/hookEventHandler.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ const mockCoreEvents = vi.hoisted(() => ({
emitFeedback: vi.fn(),
emitHookStart: vi.fn(),
emitHookEnd: vi.fn(),
emitHookSystemMessage: vi.fn(),
}));

vi.mock('../utils/debugLogger.js', () => ({
Expand Down Expand Up @@ -891,4 +892,100 @@ describe('HookEventHandler', () => {
);
});
});

describe('systemMessage event emission', () => {
const buildMocks = (
outputFormat: 'json' | 'text',
systemMessage: string,
) => {
const hookConfig: HookConfig = {
type: HookType.Command,
command: './hook.sh',
timeout: 30000,
};
const results: HookExecutionResult[] = [
{
success: true,
duration: 10,
hookConfig,
eventName: HookEventName.SessionStart,
output: { systemMessage },
outputFormat,
},
];
vi.mocked(mockHookPlanner.createExecutionPlan).mockReturnValue({
eventName: HookEventName.SessionStart,
hookConfigs: [hookConfig],
sequential: false,
});
vi.mocked(mockHookRunner.executeHooksParallel).mockResolvedValue(results);
vi.mocked(mockHookAggregator.aggregateResults).mockReturnValue({
success: true,
allOutputs: [],
errors: [],
totalDuration: 10,
});
};

it('emits HookSystemMessage for json-format hook output', async () => {
buildMocks('json', 'json banner');

await hookEventHandler.fireSessionStartEvent(SessionStartSource.Startup);

expect(mockCoreEvents.emitHookSystemMessage).toHaveBeenCalledTimes(1);
expect(mockCoreEvents.emitHookSystemMessage).toHaveBeenCalledWith(
expect.objectContaining({
eventName: HookEventName.SessionStart,
message: 'json banner',
}),
);
});

it('emits HookSystemMessage for text-format hook output', async () => {
buildMocks('text', 'plain-text banner');

await hookEventHandler.fireSessionStartEvent(SessionStartSource.Startup);

expect(mockCoreEvents.emitHookSystemMessage).toHaveBeenCalledTimes(1);
expect(mockCoreEvents.emitHookSystemMessage).toHaveBeenCalledWith(
expect.objectContaining({
eventName: HookEventName.SessionStart,
message: 'plain-text banner',
}),
);
});

it('does not emit when systemMessage is absent', async () => {
const hookConfig: HookConfig = {
type: HookType.Command,
command: './hook.sh',
timeout: 30000,
};
vi.mocked(mockHookPlanner.createExecutionPlan).mockReturnValue({
eventName: HookEventName.SessionStart,
hookConfigs: [hookConfig],
sequential: false,
});
vi.mocked(mockHookRunner.executeHooksParallel).mockResolvedValue([
{
success: true,
duration: 10,
hookConfig,
eventName: HookEventName.SessionStart,
output: {},
outputFormat: 'json',
},
]);
vi.mocked(mockHookAggregator.aggregateResults).mockReturnValue({
success: true,
allOutputs: [],
errors: [],
totalDuration: 10,
});

await hookEventHandler.fireSessionStartEvent(SessionStartSource.Startup);

expect(mockCoreEvents.emitHookSystemMessage).not.toHaveBeenCalled();
});
});
});
5 changes: 3 additions & 2 deletions packages/core/src/hooks/hookEventHandler.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -459,8 +459,9 @@ export class HookEventHandler {

logHookCall(this.context.config, hookCallEvent);

// Emit structured system message event for UI display
if (result.output?.systemMessage && result.outputFormat === 'json') {
// Emit structured system message event for UI display. Covers both
// 'json' and 'text' output formats so plain-text hook stdout also surfaces.
if (result.output?.systemMessage) {
coreEvents.emitHookSystemMessage({
hookName,
eventName,
Expand Down
Loading