Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
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 packages/cli/src/ui/hooks/useGeminiStream.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -154,6 +154,7 @@ describe('useGeminiStream', () => {
let mockScheduleToolCalls: Mock;
let mockCancelAllToolCalls: Mock;
let mockMarkToolsAsSubmitted: Mock;
let mockBackgroundShellRegistry: { setNotificationCallback: Mock };
let handleAtCommandSpy: MockInstance;

beforeEach(() => {
Expand Down Expand Up @@ -182,6 +183,9 @@ describe('useGeminiStream', () => {
vertexai: false,
authType: AuthType.USE_GEMINI,
};
mockBackgroundShellRegistry = {
setNotificationCallback: vi.fn(),
};

mockConfig = {
apiKey: 'test-api-key',
Expand Down Expand Up @@ -230,6 +234,7 @@ describe('useGeminiStream', () => {
getBackgroundTaskRegistry: vi.fn(() => ({
setNotificationCallback: vi.fn(),
})),
getBackgroundShellRegistry: vi.fn(() => mockBackgroundShellRegistry),
getMonitorRegistry: vi.fn(() => ({
setNotificationCallback: vi.fn(),
})),
Expand Down Expand Up @@ -356,6 +361,44 @@ describe('useGeminiStream', () => {
};
};

it('queues background shell terminal notifications for the model loop', async () => {
const { mockSendMessageStream } = renderTestHook();
const displayText = 'Background shell "npm test" completed.';
const modelText =
'<task-notification>\n<kind>shell</kind>\n<status>completed</status>\n</task-notification>';

await waitFor(() => {
expect(
mockBackgroundShellRegistry.setNotificationCallback,
).toHaveBeenCalledWith(expect.any(Function));
});

const callback = mockBackgroundShellRegistry.setNotificationCallback.mock
.calls[0][0] as (displayText: string, modelText: string) => void;

act(() => {
callback(displayText, modelText);
});

await waitFor(() => {
expect(mockAddItem).toHaveBeenCalledWith(
{ type: 'notification', text: displayText },
expect.any(Number),
);
});
await waitFor(() => {
expect(mockSendMessageStream).toHaveBeenCalledWith(
modelText,
expect.any(AbortSignal),
expect.any(String),
expect.objectContaining({
type: SendMessageType.Notification,
notificationDisplayText: displayText,
}),
);
});
});

it('should not submit tool responses if not all tool calls are completed', () => {
const toolCalls: TrackedToolCall[] = [
{
Expand Down
16 changes: 16 additions & 0 deletions packages/cli/src/ui/hooks/useGeminiStream.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2485,6 +2485,22 @@ export const useGeminiStream = (
};
}, [config]);

// Register background shell terminal notification callback onto the shared queue.
useEffect(() => {
const registry = config.getBackgroundShellRegistry();
registry.setNotificationCallback((displayText, modelText) => {
notificationQueueRef.current.push({
displayText,
modelText,
sendMessageType: SendMessageType.Notification,
});
setNotificationTrigger((n) => n + 1);
});
return () => {
registry.setNotificationCallback(undefined);
};
}, [config]);

// Register monitor notification callback onto the shared queue.
useEffect(() => {
const registry = config.getMonitorRegistry();
Expand Down
161 changes: 160 additions & 1 deletion packages/core/src/services/backgroundShellRegistry.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,34 @@
* SPDX-License-Identifier: Apache-2.0
*/

import { describe, expect, it, vi } from 'vitest';
import { mkdtempSync, rmSync, writeFileSync } from 'node:fs';
import { tmpdir } from 'node:os';
import { join } from 'node:path';
import { afterEach, describe, expect, it, vi } from 'vitest';
import {
BackgroundShellRegistry,
MAX_NOTIFICATION_OUTPUT_TAIL_BYTES,
MAX_RETAINED_TERMINAL_SHELLS,
type ShellTaskRegistration,
} from './backgroundShellRegistry.js';

let tmpDirs: string[] = [];

afterEach(() => {
for (const dir of tmpDirs) {
rmSync(dir, { recursive: true, force: true });
}
tmpDirs = [];
});

function makeOutputFile(content: string): string {
const dir = mkdtempSync(join(tmpdir(), 'qwen-shell-notification-'));
tmpDirs.push(dir);
const file = join(dir, 'shell.output');
writeFileSync(file, content);
return file;
}

function makeEntry(
overrides: Partial<ShellTaskRegistration> = {},
): ShellTaskRegistration {
Expand Down Expand Up @@ -187,6 +208,144 @@ describe('BackgroundShellRegistry', () => {
});
Comment thread
DragonnZhang marked this conversation as resolved.
});

describe('notifications', () => {
it('emits one task-notification when a shell completes', () => {
Comment thread
DragonnZhang marked this conversation as resolved.
const reg = new BackgroundShellRegistry();
const callback = vi.fn();
const outputPath = makeOutputFile('first line\nfinal result\n');
reg.setNotificationCallback(callback);
reg.register(
makeEntry({
shellId: 'a',
command: 'npm test',
cwd: '/repo',
outputPath,
pid: 1234,
}),
);

reg.complete('a', 0, 2000);

expect(callback).toHaveBeenCalledTimes(1);
const [displayText, modelText, meta] = callback.mock.calls[0];
expect(displayText).toBe('Background shell "npm test" completed.');
expect(modelText).toContain('<task-notification>');
expect(modelText).toContain('<task-id>a</task-id>');
expect(modelText).toContain('<kind>shell</kind>');
expect(modelText).toContain('<status>completed</status>');
expect(modelText).toContain('<command>npm test</command>');
expect(modelText).toContain('<cwd>/repo</cwd>');
expect(modelText).toContain('<pid>1234</pid>');
expect(modelText).toContain('<exit-code>0</exit-code>');
expect(modelText).toContain(
'<output-tail truncated="false">first line\nfinal result</output-tail>',
);
expect(modelText).toContain(`<output-file>${outputPath}</output-file>`);
expect(meta).toEqual({
shellId: 'a',
status: 'completed',
exitCode: 0,
});
});

it('escapes XML and strips display control characters on failure', () => {
const reg = new BackgroundShellRegistry();
const callback = vi.fn();
reg.setNotificationCallback(callback);
reg.register(
makeEntry({
shellId: 'a&b',
command: 'echo "<script>"',
cwd: '/repo&work',
outputPath: '/tmp/out&err.log',
}),
);

reg.fail('a&b', 'bad <thing>\x1B[31m', 2000);

const [displayText, modelText] = callback.mock.calls[0];
expect(displayText).toBe('Background shell "echo "<script>"" failed.');
expect(modelText).toContain('<task-id>a&amp;b</task-id>');
expect(modelText).toContain(
'<command>echo &quot;&lt;script&gt;&quot;</command>',
);
expect(modelText).toContain('<cwd>/repo&amp;work</cwd>');
expect(modelText).toContain('<result>bad &lt;thing&gt;[31m</result>');
expect(modelText).toContain(
'<output-file>/tmp/out&amp;err.log</output-file>',
);
});

it('limits output-tail to the retained byte budget', () => {
const reg = new BackgroundShellRegistry();
const callback = vi.fn();
const outputPath = makeOutputFile(
'prefix-' +
'a'.repeat(MAX_NOTIFICATION_OUTPUT_TAIL_BYTES) +
'\nlast line\n',
);
reg.setNotificationCallback(callback);
reg.register(makeEntry({ shellId: 'a', outputPath }));

reg.complete('a', 0, 2000);

const [, modelText] = callback.mock.calls[0];
expect(modelText).toContain('<output-tail truncated="true">');
expect(modelText).toContain('last line</output-tail>');
expect(modelText).not.toContain('prefix-');
});

it('does not emit more than once for late terminal transitions', () => {
Comment thread
DragonnZhang marked this conversation as resolved.
const reg = new BackgroundShellRegistry();
const callback = vi.fn();
reg.setNotificationCallback(callback);
reg.register(makeEntry({ shellId: 'a' }));

reg.complete('a', 0, 2000);
reg.fail('a', 'late failure', 3000);
reg.cancel('a', 4000);

expect(callback).toHaveBeenCalledTimes(1);
});

it('waits until cancel() to notify after requestCancel()', () => {
const reg = new BackgroundShellRegistry();
const callback = vi.fn();
reg.setNotificationCallback(callback);
reg.register(makeEntry({ shellId: 'a' }));

reg.requestCancel('a');

expect(callback).not.toHaveBeenCalled();

reg.cancel('a', 2000);

expect(callback).toHaveBeenCalledTimes(1);
const [displayText, modelText, meta] = callback.mock.calls[0];
expect(displayText).toBe('Background shell "sleep 60" was cancelled.');
expect(modelText).toContain('<status>cancelled</status>');
expect(meta).toEqual({
shellId: 'a',
status: 'cancelled',
exitCode: undefined,
});
});

it('does not emit notifications from abortAll shutdown cleanup', () => {
const reg = new BackgroundShellRegistry();
const callback = vi.fn();
reg.setNotificationCallback(callback);
reg.register(makeEntry({ shellId: 'a' }));
reg.register(makeEntry({ shellId: 'b' }));

reg.abortAll();

expect(callback).not.toHaveBeenCalled();
expect(reg.get('a')!.notified).toBe(false);
expect(reg.get('b')!.notified).toBe(false);
});
});

describe('requestCancel', () => {
it('aborts the signal but leaves status running and endTime undefined', () => {
const reg = new BackgroundShellRegistry();
Expand Down
Loading
Loading