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
47 changes: 47 additions & 0 deletions packages/cli/src/ui/hooks/useGeminiStream.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -206,6 +206,7 @@ describe('useGeminiStream', () => {
let mockCancelAllToolCalls: Mock;
let mockMarkToolsAsSubmitted: Mock;
let mockBackgroundShellRegistry: { setNotificationCallback: Mock };
let mockWorkflowRunRegistry: { setCompletionCallback: Mock };
let mockMonitorRegistry: {
setNotificationCallback: Mock;
get: Mock;
Expand Down Expand Up @@ -242,6 +243,9 @@ describe('useGeminiStream', () => {
mockBackgroundShellRegistry = {
setNotificationCallback: vi.fn(),
};
mockWorkflowRunRegistry = {
setCompletionCallback: vi.fn(),
};
mockMonitorRegistry = {
setNotificationCallback: vi.fn(),
get: vi.fn().mockReturnValue({ status: 'running' }),
Expand Down Expand Up @@ -300,6 +304,7 @@ describe('useGeminiStream', () => {
})),
getBackgroundShellRegistry: vi.fn(() => mockBackgroundShellRegistry),
getMonitorRegistry: vi.fn(() => mockMonitorRegistry),
getWorkflowRunRegistry: vi.fn(() => mockWorkflowRunRegistry),
} as unknown as Config;
mockOnDebugMessage = vi.fn();
mockHandleSlashCommand = vi.fn().mockResolvedValue(false);
Expand Down Expand Up @@ -472,6 +477,48 @@ describe('useGeminiStream', () => {
});
});

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

await waitFor(() => {
expect(
mockWorkflowRunRegistry.setCompletionCallback,
).toHaveBeenCalledWith(expect.any(Function));
});
const callback = mockWorkflowRunRegistry.setCompletionCallback.mock
.calls[0][0] as (
displayText: string,
modelText: string,
meta: { todoWorkChainId?: string },
) => void;

act(() => {
callback(displayText, modelText, { todoWorkChainId: 'workflow-chain' });
});

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,
todoWorkChainId: 'workflow-chain',
}),
Comment on lines +513 to +517

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[Suggestion] The expect.objectContaining matcher in this new test asserts type and notificationDisplayText but not todoWorkChainId, even though the callback is invoked with { todoWorkChainId: 'workflow-chain' }. A mutation that drops todoWorkChainId: meta.todoWorkChainId from the queue push in useGeminiStream.ts would therefore survive this test green.

Failure scenario: if todoWorkChainId is silently dropped from the queue entry, the queue drain's contiguous-batch split (which groups notifications by todoWorkChainId) would merge completions from unrelated work chains into one batched sendMessageStream call and deliver no chain-routing metadata to the model loop — and this test would still pass.

Suggested change
expect.objectContaining({
type: SendMessageType.Notification,
notificationDisplayText: displayText,
}),
expect.objectContaining({
type: SendMessageType.Notification,
notificationDisplayText: displayText,
todoWorkChainId: 'workflow-chain',
}),
中文说明

[Suggestion] 这个新增测试里的 expect.objectContaining 匹配器断言了 typenotificationDisplayText,但没有断言 todoWorkChainId,尽管 callback 是以 { todoWorkChainId: 'workflow-chain' } 调用的。因此,一个把 useGeminiStream.ts 队列 push 中的 todoWorkChainId: meta.todoWorkChainId 删掉的 mutation 仍能让该测试通过。

失败场景:如果 todoWorkChainId 被静默地从队列条目中丢弃,队列 drain 的连续分批逻辑(按 todoWorkChainId 分组通知)会把来自不同 work chain 的 completion 合并进同一个批量 sendMessageStream 调用,并且不会把 chain 路由元数据传给模型 loop——而本测试仍会通过。

— qwen3.8-max-preview via Qwen Code /review

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

已修复:已补充 workflow completion 的 todoWorkChainId 路由断言。\n\n验证:cd packages/cli && npx vitest run src/ui/hooks/useGeminiStream.test.tsx(169 passed)。

);
});
});

it('forwards submitted prompt provenance only for UserQuery', async () => {
const { result, mockSendMessageStream } = renderTestHook();

Expand Down
18 changes: 18 additions & 0 deletions packages/cli/src/ui/hooks/useGeminiStream.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3978,6 +3978,24 @@ export const useGeminiStream = (
};
}, [config]);

// Register background workflow completions onto the shared queue. The
// registry keeps this separate from its terminal-bell subscriber.
useEffect(() => {
const registry = config.getWorkflowRunRegistry();
registry.setCompletionCallback((displayText, modelText, meta) => {
notificationQueueRef.current.push({
displayText,
modelText,
sendMessageType: SendMessageType.Notification,
todoWorkChainId: meta.todoWorkChainId,
});
setNotificationTrigger((n) => n + 1);
});
return () => {
registry.setCompletionCallback(undefined);
};
}, [config]);

// Register monitor notification callback onto the shared queue.
useEffect(() => {
const registry = config.getMonitorRegistry();
Expand Down
101 changes: 101 additions & 0 deletions packages/core/src/agents/runtime/workflow-runner.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
*/

import { beforeEach, describe, expect, it, vi } from 'vitest';
import { getEventListeners } from 'node:events';
import type { Config } from '../../config/config.js';
import { WorkflowRunRegistry } from '../workflow-run-registry.js';
import { AgentEventEmitter } from './agent-events.js';
Expand Down Expand Up @@ -91,6 +92,7 @@ describe('WorkflowRunner', () => {
signal: new AbortController().signal,
script: 'return await agent("work")',
args: undefined,
runInBackground: true,
});
await expect(productionHandle.completion).resolves.toMatchObject({
ok: true,
Expand Down Expand Up @@ -213,6 +215,103 @@ describe('WorkflowRunner', () => {
expect(logWorkflowRunMock).toHaveBeenCalledTimes(2);
});

it('keeps background runs alive after the caller turn ends', async () => {
const { config, registry } = configWithRegistry();
const observed = observeSettlement(registry);
const caller = new AbortController();
let resolveDispatch: ((value: string) => void) | undefined;
const handle = await WorkflowRunner.start({
config,
signal: caller.signal,
script: 'return await agent("work")',
args: undefined,
runInBackground: true,
dispatch: () =>
new Promise<string>((resolve) => {
resolveDispatch = resolve;
}),
});
await vi.waitFor(() => expect(resolveDispatch).toBeDefined());

caller.abort();
expect(observed.abortCount()).toBe(0);
expect(registry.get(handle.runId)?.status).toBe('running');

resolveDispatch?.('done');
await expect(handle.completion).resolves.toMatchObject({ ok: true });
expect(registry.get(handle.runId)?.status).toBe('completed');
expect(observed.terminalStatuses).toEqual(['completed']);
expect(observed.abortCount()).toBe(1);
});

it('rejects a concurrent resume while the original run is active', async () => {
const { config, registry } = configWithRegistry();
const runId = 'wf_1234abcd';
let resolveDispatch: ((value: string) => void) | undefined;
const original = await WorkflowRunner.start({
config,
signal: new AbortController().signal,
script: 'return await agent("original")',
args: undefined,
resumeFromRunId: runId,
runInBackground: true,
dispatch: () =>
new Promise<string>((resolve) => {
resolveDispatch = resolve;
}),
});
await vi.waitFor(() => expect(resolveDispatch).toBeDefined());
const replacementCaller = new AbortController();
const replacementDispatch = vi.fn(async () => 'replacement');

try {
await expect(
WorkflowRunner.start({
config,
signal: replacementCaller.signal,
script: 'return await agent("replacement")',
args: undefined,
resumeFromRunId: runId,
dispatch: replacementDispatch,
}),
).rejects.toThrow(/already active/);
expect(registry.getHandle(runId)).toBe(original);
expect(replacementDispatch).not.toHaveBeenCalled();
expect(getEventListeners(replacementCaller.signal, 'abort')).toHaveLength(
0,
);
} finally {
resolveDispatch?.('original');
await original.completion;
}

expect(registry.get(runId)?.result).toBe('original');
});

it('classifies a background failure after caller abort as failed', async () => {
const { config, registry } = configWithRegistry();
const caller = new AbortController();
let rejectDispatch: ((error: Error) => void) | undefined;
const handle = await WorkflowRunner.start({
config,
signal: caller.signal,
script: 'return await agent("work")',
args: undefined,
runInBackground: true,
dispatch: () =>
new Promise<string>((_resolve, reject) => {
rejectDispatch = reject;
}),
});
await vi.waitFor(() => expect(rejectDispatch).toBeDefined());

caller.abort();
rejectDispatch?.(new Error('background boom'));

await expect(handle.completion).resolves.toMatchObject({ ok: false });
expect(registry.get(handle.runId)?.status).toBe('failed');
});

it('routes registry cancellation through each live handle', async () => {
const cancelCases: Array<{
cancel: (registry: WorkflowRunRegistry, runId: string) => void;
Expand All @@ -234,6 +333,7 @@ describe('WorkflowRunner', () => {
signal: new AbortController().signal,
script: 'return await agent("work")',
args: undefined,
runInBackground: true,
dispatch: () =>
new Promise<string>((_resolve, reject) => {
rejectDispatch = reject;
Expand Down Expand Up @@ -272,6 +372,7 @@ describe('WorkflowRunner', () => {
signal: new AbortController().signal,
script: 'await new Promise(() => {})',
args: undefined,
runInBackground: true,
dispatch: async () => 'unused',
});

Expand Down
50 changes: 36 additions & 14 deletions packages/core/src/agents/runtime/workflow-runner.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,10 @@ import { randomBytes } from 'node:crypto';
import type { Config } from '../../config/config.js';
import { logWorkflowRun } from '../../telemetry/loggers.js';
import { WorkflowRunEvent } from '../../telemetry/types.js';
import { createChildAbortController } from '../../utils/abortController.js';
import {
createAbortController,
createChildAbortController,
} from '../../utils/abortController.js';
import {
type WorkflowRunRegistry,
type WorkflowTask,
Expand All @@ -35,6 +38,7 @@ export interface WorkflowRunnerOptions {
resumeFromRunId?: string;
dispatch?: WorkflowAgentDispatch;
onUpdate?: (entry: WorkflowTask) => void;
runInBackground?: boolean;
}

export type WorkflowRunSettlement =
Expand Down Expand Up @@ -64,6 +68,7 @@ export class WorkflowRunner {
options: WorkflowRunnerOptions,
): Promise<WorkflowRunHandle> {
const config = options.config;
const runInBackground = options.runInBackground === true;
const budget = WorkflowBudgetImpl.fromEnv();
const loaded =
options.scriptPath && options.script === undefined
Expand All @@ -83,7 +88,13 @@ export class WorkflowRunner {
const resumeReplay: JournalReplay | undefined = options.resumeFromRunId
? await journal?.load()
: undefined;
const controller = createChildAbortController(options.signal);
if (runInBackground && options.signal.aborted) {
throw new Error('Background workflow start was cancelled.');
}
const callerWasAbortedBeforeStart = options.signal.aborted;
const controller = runInBackground
? createAbortController()
Comment on lines +94 to +96

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[Suggestion] callerWasAbortedBeforeStart is dead logic — it can never be the deciding factor in the catch-block error-classification condition. Failure scenario: no runtime misbehavior, but a concrete maintenance cost — for background runs the pre-start guard (if (runInBackground && options.signal.aborted) throw) fires before this capture, so the variable is always false there; for foreground runs AbortSignal.aborted is monotonic, so whenever this variable is true the adjacent !runInBackground && options.signal.aborted disjunct is also true and subsumes it. A future maintainer changing the abort-classification logic must trace two async preflight operations plus the monotonicity invariant to discover the variable covers no case. Suggested fix (spans two locations — remove the variable here, and simplify the catch condition):

if (
  (!runInBackground && options.signal.aborted) ||
  entry?.status === 'cancelled'
) {
中文说明

callerWasAbortedBeforeStart 是死逻辑——它永远无法成为 catch 块错误分类条件中的决定因素。失败场景:没有运行时错误,但有具体的维护成本——对于后台运行,启动前的守卫(if (runInBackground && options.signal.aborted) throw)在这次捕获之前就会触发,所以该变量在那里恒为 false;对于前台运行,AbortSignal.aborted 是单调的,因此只要该变量为 true,相邻的 !runInBackground && options.signal.aborted 分支也必然为 true 并将其涵盖。未来修改 abort 分类逻辑的维护者必须追踪两个异步 preflight 操作加上单调性不变量,才能发现该变量不覆盖任何情况。建议修复(涉及两处——删除此处的变量,并简化 catch 条件):

if (
  (!runInBackground && options.signal.aborted) ||
  entry?.status === 'cancelled'
) {

— qwen3.8-max-preview via Qwen Code /review (v0.21.2)

: createChildAbortController(options.signal);
const registry = config.getWorkflowRunRegistry?.();
const dispatch =
options.dispatch ??
Expand All @@ -96,17 +107,24 @@ export class WorkflowRunner {
: undefined,
);
const orchestrator = new WorkflowOrchestrator(dispatch);
const entry = registry?.register({
runId,
meta: null,
status: 'running',
startTime: Date.now(),
outputFile: '',
abortController: controller,
tokenBudgetTotal: budget.total,
script,
scriptPath,
});
let entry: WorkflowTask | undefined;
try {
entry = registry?.register({
runId,
meta: null,
status: 'running',
startTime: Date.now(),
outputFile: '',
abortController: controller,
tokenBudgetTotal: budget.total,
script,
scriptPath,
isBackgrounded: runInBackground,
});
} catch (error) {
controller.abort();
throw error;
}
const emitUpdate = (): void => {
if (!entry || !options.onUpdate) return;
try {
Expand Down Expand Up @@ -172,7 +190,11 @@ export class WorkflowRunner {
const message = extractErrorMessage(error);
if (entry && details?.meta && !entry.meta) entry.meta = details.meta;
if (details?.logs) registry?.setRecentLogs(runId, details.logs);
if (options.signal.aborted) {
if (
callerWasAbortedBeforeStart ||
(!runInBackground && options.signal.aborted) ||
entry?.status === 'cancelled'
) {
registry?.cancel(runId, Date.now());
} else {
registry?.fail(runId, message, Date.now());
Expand Down
12 changes: 5 additions & 7 deletions packages/core/src/agents/tasks/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -37,13 +37,11 @@
* they have a separate lifecycle and their inclusion is deferred to a
* follow-up.
*
* `workflow` (P4b) is registered/observed via `WorkflowRunRegistry` and
* differs from the others in that the registry NEVER emits a
* `<task-notification>` envelope — `WorkflowTool` already returns its
* own llmContent + returnDisplay payload to the model on terminal, so
* a second envelope would duplicate the signal. The kind is widened
* here so the UI surfaces (pill / dialog / detail body) can switch on
* `entry.kind === 'workflow'`.
* `workflow` (P4b) is registered/observed via `WorkflowRunRegistry`.
* Foreground workflows return through their normal tool result; background
* workflows emit one terminal notification through a separate completion
* channel. The kind is widened here so the UI surfaces (pill / dialog /
* detail body) can switch on `entry.kind === 'workflow'`.
*/
export type TaskKind = 'agent' | 'shell' | 'monitor' | 'workflow';

Expand Down
Loading
Loading