Skip to content
Merged
Show file tree
Hide file tree
Changes from all 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
39 changes: 39 additions & 0 deletions docs/design/monitor-cancel-notification.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
# Explicit Monitor Cancellation Notifications

## Problem

`task_stop` already returns a synchronous tool result confirming that a monitor
was cancelled. The monitor registry also emits a terminal `cancelled`
notification, which clients record as a notification user message and submit as
a new model turn. A `running` event queued just before cancellation can cause the
same extra turn even if the terminal notification is suppressed.

## Design

- Cancel monitors silently when cancellation comes from `task_stop`; the tool
result remains the user- and model-visible confirmation.
- Keep the registry's default cancellation behavior unchanged for other callers.
- At drain time, discard queued `running` monitor notifications whose registry
entry is now explicitly `cancelled`. This check applies to the interactive
queue, the persistent stream-json queue, and the one-shot headless queue.
- Continue delivering natural `completed` and `failed` notifications, along with
terminal notifications emitted by non-`task_stop` cancellation paths.

ACP already rejects `running` monitor notifications, so silent explicit
cancellation is sufficient for that client.

Owner-routed monitor notifications stay inside an agent's input queue rather
than the user's conversation. They are outside this session-notification fix;
in the common tool-call path, any queued event is delivered alongside the
already-required `task_stop` tool result instead of creating a session turn.

## Verification

- `task_stop` cancels and aborts a monitor without invoking its notification
callback.
- Each client drops a queued `running` event after the monitor is explicitly
cancelled.
- Existing terminal-notification tests continue to demonstrate that natural
completion and failure are delivered.
- A real model-driven `monitor` then `task_stop` run produces no follow-up
notification turn.
61 changes: 61 additions & 0 deletions packages/cli/src/nonInteractive/session.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@ interface ConfigOverrides {
let mockMonitorRegistry: {
setNotificationCallback: ReturnType<typeof vi.fn>;
setRegisterCallback: ReturnType<typeof vi.fn>;
get: ReturnType<typeof vi.fn>;
abortAll: ReturnType<typeof vi.fn>;
};
let mockBackgroundShellRegistry: {
Expand Down Expand Up @@ -190,6 +191,7 @@ describe('runNonInteractiveStreamJson', () => {
mockMonitorRegistry = {
setNotificationCallback: vi.fn(),
setRegisterCallback: vi.fn(),
get: vi.fn().mockReturnValue({ status: 'running' }),
abortAll: vi.fn(),
};
mockBackgroundShellRegistry = {
Expand Down Expand Up @@ -838,6 +840,65 @@ describe('runNonInteractiveStreamJson', () => {
);
});

it('drops a queued running monitor event after cancellation', async () => {
const initRequest = createControlRequest('initialize');
const userMessage = createUserMessage('Start then stop a monitor');
let closeInput: (() => void) | undefined;
let monitorCallback:
| ((
displayText: string,
modelText: string,
meta: {
monitorId: string;
toolUseId?: string;
status: string;
},
) => void)
| undefined;
let monitorStatus = 'running';

mockMonitorRegistry.get.mockImplementation(() => ({
status: monitorStatus,
}));
mockMonitorRegistry.setNotificationCallback.mockImplementation((cb) => {
monitorCallback = cb;
});
runNonInteractiveMock.mockImplementationOnce(async () => {
monitorCallback?.(
'Monitor "logs" event #1: ready',
'<task-notification>running</task-notification>',
{
monitorId: 'mon_1',
toolUseId: 'tool_mon_1',
status: 'running',
},
);
monitorStatus = 'cancelled';
});

mockInputReader.read = async function* () {
yield initRequest;
yield userMessage;
await new Promise<void>((resolve) => {
closeInput = resolve;
});
};

const sessionPromise = runNonInteractiveStreamJson(config, '');
await vi.waitFor(() => {
expect(runNonInteractiveMock).toHaveBeenCalledTimes(1);
});
closeInput?.();
await sessionPromise;

expect(runNonInteractiveMock).toHaveBeenCalledTimes(1);
expect(mockOutputAdapter.emitUserMessage).not.toHaveBeenCalled();
expect(mockOutputAdapter.emitSystemMessage).not.toHaveBeenCalledWith(
'task_notification',
expect.anything(),
);
});

it('stops accepting new monitor events before EOF drain', async () => {
const initRequest = createControlRequest('initialize');
const userMessage = createUserMessage('Start a monitor');
Expand Down
13 changes: 13 additions & 0 deletions packages/cli/src/nonInteractive/session.ts
Original file line number Diff line number Diff line change
Expand Up @@ -598,6 +598,19 @@ class Session {
): Promise<void> {
await this.waitForInitialization();

batch = batch.filter((item) => {
if (item.sdkNotification.status !== 'running') {
return true;
}
return (
this.config.getMonitorRegistry().get(item.sdkNotification.task_id)
?.status !== 'cancelled'
);
});
if (batch.length === 0) {
return;
}

for (const item of batch) {
this.outputAdapter.emitUserMessage([{ text: item.displayText }]);
this.outputAdapter.emitSystemMessage(
Expand Down
89 changes: 89 additions & 0 deletions packages/cli/src/nonInteractiveCli.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3283,6 +3283,95 @@ describe('runNonInteractive', () => {
expect(userEnvelopes).toHaveLength(0);
});

it('drops a queued running monitor event after cancellation', async () => {
(mockConfig.getOutputFormat as Mock).mockReturnValue(
OutputFormat.STREAM_JSON,
);
(mockConfig.getIncludePartialMessages as Mock).mockReturnValue(false);
setupMetricsMock();

const writes: string[] = [];
processStdoutSpy.mockImplementation((chunk: string | Uint8Array) => {
if (typeof chunk === 'string') {
writes.push(chunk);
} else {
writes.push(Buffer.from(chunk).toString('utf8'));
}
return true;
});

const notificationXml =
'<task-notification>\n' +
'<task-id>mon_1</task-id>\n' +
'<kind>monitor</kind>\n' +
'<status>running</status>\n' +
'<summary>Monitor emitted event #1.</summary>\n' +
'<result>ready</result>\n' +
'</task-notification>';
let monitorStatus = 'running';
mockMonitorRegistry.get.mockImplementation(() => ({
status: monitorStatus,
}));
mockMonitorRegistry.setNotificationCallback.mockImplementation((cb) => {
if (!cb) return;
cb('Monitor "logs" event #1: ready', notificationXml, {
monitorId: 'mon_1',
toolUseId: 'tool_mon_1',
status: 'running',
eventCount: 1,
});
monitorStatus = 'cancelled';
});
mockGeminiClient.sendMessageStream.mockReturnValueOnce(
createStreamFromEvents([
{ type: GeminiEventType.Content, value: 'Monitor stopped.' },
{
type: GeminiEventType.Finished,
value: {
reason: undefined,
usageMetadata: { totalTokenCount: 2 },
},
},
]),
);

await runNonInteractive(
mockConfig,
mockSettings,
'Start then stop a monitor',
'prompt-monitor-cancel',
);

expect(mockGeminiClient.sendMessageStream).toHaveBeenCalledTimes(1);
const envelopes = writes
.join('')
.split('\n')
.filter((line) => line.trim().length > 0)
.map((line) => JSON.parse(line));
expect(
envelopes.some(
(env) =>
env.type === 'user' &&
Array.isArray(env.message?.content) &&
env.message.content.some(
(block: unknown) =>
typeof block === 'object' &&
block !== null &&
'text' in block &&
block.text === 'Monitor "logs" event #1: ready',
),
),
).toBe(false);
expect(
envelopes.some(
(env) =>
env.type === 'system' &&
env.subtype === 'task_notification' &&
env.data?.task_id === 'mon_1',
),
).toBe(false);
});

it('does not let late monitor output keep one-shot runs alive', async () => {
(mockConfig.getOutputFormat as Mock).mockReturnValue(
OutputFormat.STREAM_JSON,
Expand Down
18 changes: 16 additions & 2 deletions packages/cli/src/nonInteractiveCli.ts
Original file line number Diff line number Diff line change
Expand Up @@ -456,6 +456,7 @@ export async function runNonInteractive(
displayText: string;
modelText: string;
sendMessageType: SendMessageType;
monitorId?: string;
sdkNotification?: {
task_id: string;
tool_use_id?: string;
Expand All @@ -469,6 +470,13 @@ export async function runNonInteractive(
}
const localQueue: LocalQueueItem[] = [];
const sdkOnlyMonitorQueue: LocalQueueItem[] = [];
const isCancelledMonitorEvent = (item: LocalQueueItem) =>
Boolean(
item.monitorId &&
item.sdkNotification?.status === 'running' &&
config.getMonitorRegistry().get(item.monitorId)?.status ===
'cancelled',
);
const emitNotificationToSdk = (item: LocalQueueItem) => {
if (item.sendMessageType !== SendMessageType.Notification) return;
adapter.emitUserMessage([{ text: item.displayText }]);
Expand All @@ -478,7 +486,10 @@ export async function runNonInteractive(
};
const flushQueuedNotificationsToSdk = (queue: LocalQueueItem[]) => {
while (queue.length > 0) {
emitNotificationToSdk(queue.shift()!);
const item = queue.shift()!;
if (!isCancelledMonitorEvent(item)) {
emitNotificationToSdk(item);
}
}
};
let captureMonitorTurnsInLocalQueue = true;
Expand Down Expand Up @@ -935,6 +946,7 @@ export async function runNonInteractive(
displayText,
modelText,
sendMessageType: SendMessageType.Notification,
monitorId: meta.monitorId,
sdkNotification: {
task_id: meta.monitorId,
tool_use_id: meta.toolUseId,
Expand Down Expand Up @@ -1884,7 +1896,9 @@ export async function runNonInteractive(
splitIdx++;
}
}
const batch = localQueue.splice(0, splitIdx);
const batch = localQueue
.splice(0, splitIdx)
.filter((item) => !isCancelledMonitorEvent(item));

if (batch.length === 0) return;

Expand Down
47 changes: 44 additions & 3 deletions packages/cli/src/ui/hooks/useGeminiStream.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -203,6 +203,10 @@ describe('useGeminiStream', () => {
let mockCancelAllToolCalls: Mock;
let mockMarkToolsAsSubmitted: Mock;
let mockBackgroundShellRegistry: { setNotificationCallback: Mock };
let mockMonitorRegistry: {
setNotificationCallback: Mock;
get: Mock;
};
let handleAtCommandSpy: MockInstance;

beforeEach(() => {
Expand Down Expand Up @@ -235,6 +239,10 @@ describe('useGeminiStream', () => {
mockBackgroundShellRegistry = {
setNotificationCallback: vi.fn(),
};
mockMonitorRegistry = {
setNotificationCallback: vi.fn(),
get: vi.fn().mockReturnValue({ status: 'running' }),
};

mockConfig = {
apiKey: 'test-api-key',
Expand Down Expand Up @@ -288,9 +296,7 @@ describe('useGeminiStream', () => {
setNotificationCallback: vi.fn(),
})),
getBackgroundShellRegistry: vi.fn(() => mockBackgroundShellRegistry),
getMonitorRegistry: vi.fn(() => ({
setNotificationCallback: vi.fn(),
})),
getMonitorRegistry: vi.fn(() => mockMonitorRegistry),
} as unknown as Config;
mockOnDebugMessage = vi.fn();
mockHandleSlashCommand = vi.fn().mockResolvedValue(false);
Expand Down Expand Up @@ -6972,6 +6978,41 @@ describe('useGeminiStream', () => {
).toBeUndefined();
});

it('drops a queued running monitor event after cancellation', async () => {
let monitorStatus = 'running';
mockMonitorRegistry.get.mockImplementation(() => ({
status: monitorStatus,
}));
renderTestHook();

const callback = mockMonitorRegistry.setNotificationCallback.mock
.calls[0][0] as (
displayText: string,
modelText: string,
meta: {
monitorId: string;
status: string;
},
) => void;
mockSendMessageStream.mockClear();
mockAddItem.mockClear();

await act(async () => {
callback(
'Monitor "logs" event #1: ready',
'<task-notification>running</task-notification>',
{ monitorId: 'mon_1', status: 'running' },
);
monitorStatus = 'cancelled';
});

expect(mockSendMessageStream).not.toHaveBeenCalled();
expect(mockAddItem).not.toHaveBeenCalledWith(
expect.objectContaining({ type: 'notification' }),
expect.any(Number),
);
});

// Regression for #7156: progress setState calls issued from inside a
// background subagent's AsyncLocalStorage frame can batch with the
// notification trigger into one React commit, so the drain effect
Expand Down
Loading
Loading