From 5f41b166e6695b1023e798ef787961b8f5aec501 Mon Sep 17 00:00:00 2001 From: Nothing Chan Date: Wed, 8 Jul 2026 22:52:18 +0800 Subject: [PATCH] fix(cli): unblock /clear after task cancellation and surface the blocked reason (#5949) (#6499) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit /new (alias of /clear) silently did nothing when typed right after cancelling a request, for two stacked reasons: - hasBlockingBackgroundWork() gated on the registry's hasUnfinalizedTasks(), which counts cancelled-but-not-yet-finalized entries. That clause exists for the headless holdback loop (every task_started must pair with a task_notification), but /clear and session resume abort-and-reset the registry right after the gate — suppressing that very notification — so blocking on it only made the switch fail in the window between cancel and finalizeCancelled(). The gate now keys off a new BackgroundTaskRegistry.hasRunningTasks(), which counts only entries still actually executing; the headless holdback keeps using hasUnfinalizedTasks() unchanged. - When genuinely blocked, interactive mode showed only a transient debug line while non-interactive returned a proper error. Interactive now returns the same visible error message, so a blocked /clear no longer looks like a no-op. Co-authored-by: Claude Fable 5 --- .../cli/src/ui/commands/clearCommand.test.ts | 68 +++++++++++++++++-- packages/cli/src/ui/commands/clearCommand.ts | 16 ++--- .../cli/src/ui/hooks/useResumeCommand.test.ts | 12 ++-- .../src/ui/utils/backgroundWorkUtils.test.ts | 20 +++--- .../cli/src/ui/utils/backgroundWorkUtils.ts | 8 ++- .../core/src/agents/background-tasks.test.ts | 38 +++++++++++ packages/core/src/agents/background-tasks.ts | 20 ++++++ 7 files changed, 153 insertions(+), 29 deletions(-) diff --git a/packages/cli/src/ui/commands/clearCommand.test.ts b/packages/cli/src/ui/commands/clearCommand.test.ts index a89e3cd82c..57f69fbf1e 100644 --- a/packages/cli/src/ui/commands/clearCommand.test.ts +++ b/packages/cli/src/ui/commands/clearCommand.test.ts @@ -65,7 +65,7 @@ describe('clearCommand', () => { resetChat: mockResetChat, }) as unknown as GeminiClient, getBackgroundTaskRegistry: vi.fn().mockReturnValue({ - hasUnfinalizedTasks: vi.fn().mockReturnValue(false), + hasRunningTasks: vi.fn().mockReturnValue(false), reset: mockResetBackgroundTasks, abortAll: mockAbortBackgroundTasks, }), @@ -324,7 +324,7 @@ describe('clearCommand', () => { config: { getHookSystem: mockGetHookSystem, getBackgroundTaskRegistry: vi.fn().mockReturnValue({ - hasUnfinalizedTasks: vi.fn().mockReturnValue(false), + hasRunningTasks: vi.fn().mockReturnValue(false), reset: mockResetBackgroundTasks, abortAll: mockAbortBackgroundTasks, }), @@ -404,7 +404,65 @@ describe('clearCommand', () => { services: { config: { getBackgroundTaskRegistry: vi.fn().mockReturnValue({ - hasUnfinalizedTasks: vi.fn().mockReturnValue(true), + hasRunningTasks: vi.fn().mockReturnValue(true), + reset: vi.fn(), + }), + getBackgroundShellRegistry: vi.fn().mockReturnValue({ + getAll: vi.fn().mockReturnValue([]), + hasRunningEntries: vi.fn().mockReturnValue(false), + reset: vi.fn(), + }), + getMonitorRegistry: vi.fn().mockReturnValue({ + getRunning: vi.fn().mockReturnValue([]), + reset: vi.fn(), + }), + getWorkflowRunRegistry: vi.fn().mockReturnValue({ + hasRunningEntries: vi.fn().mockReturnValue(false), + reset: vi.fn(), + abortAll: vi.fn(), + }), + getHookSystem: mockGetHookSystem, + startNewSession: mockStartNewSession, + getGeminiClient: vi.fn().mockReturnValue({ + resetChat: mockResetChat, + } as unknown as GeminiClient), + getModel: vi.fn().mockReturnValue('test-model'), + getApprovalMode: vi.fn().mockReturnValue('default'), + getToolRegistry: vi.fn().mockReturnValue({ + getAllTools: vi.fn().mockReturnValue([]), + }), + getDebugLogger: vi.fn().mockReturnValue({ warn: vi.fn() }), + }, + }, + session: { + startNewSession: vi.fn(), + }, + }); + + const result = await clearCommand.action(blockedContext, ''); + + expect(result).toEqual({ + type: 'message', + messageType: 'error', + content: + "Stop the current session's running background tasks before starting a new session.", + }); + expect(mockStartNewSession).not.toHaveBeenCalled(); + expect(mockResetChat).not.toHaveBeenCalled(); + }); + + it('returns a visible error when blocked in interactive mode (#5949)', async () => { + // Interactive mode used to bail with only a transient debug line, + // so a blocked /new looked like the command silently did nothing. + if (!clearCommand.action) + throw new Error('clearCommand must have an action.'); + + const blockedContext = createMockCommandContext({ + executionMode: 'interactive', + services: { + config: { + getBackgroundTaskRegistry: vi.fn().mockReturnValue({ + hasRunningTasks: vi.fn().mockReturnValue(true), reset: vi.fn(), }), getBackgroundShellRegistry: vi.fn().mockReturnValue({ @@ -460,7 +518,7 @@ describe('clearCommand', () => { services: { config: { getBackgroundTaskRegistry: vi.fn().mockReturnValue({ - hasUnfinalizedTasks: vi.fn().mockReturnValue(false), + hasRunningTasks: vi.fn().mockReturnValue(false), reset: vi.fn(), }), getBackgroundShellRegistry: vi.fn().mockReturnValue({ @@ -516,7 +574,7 @@ describe('clearCommand', () => { services: { config: { getBackgroundTaskRegistry: vi.fn().mockReturnValue({ - hasUnfinalizedTasks: vi.fn().mockReturnValue(false), + hasRunningTasks: vi.fn().mockReturnValue(false), reset: vi.fn(), }), getBackgroundShellRegistry: vi.fn().mockReturnValue({ diff --git a/packages/cli/src/ui/commands/clearCommand.ts b/packages/cli/src/ui/commands/clearCommand.ts index cbbd4cbfc3..4dd3ecadf1 100644 --- a/packages/cli/src/ui/commands/clearCommand.ts +++ b/packages/cli/src/ui/commands/clearCommand.ts @@ -47,14 +47,14 @@ export const clearCommand: SlashCommand = { const content = "Stop the current session's running background tasks before starting a new session."; context.ui.setDebugMessage(content); - if (context.executionMode !== 'interactive') { - return { - type: 'message' as const, - messageType: 'error' as const, - content, - }; - } - return; + // Return the error in every mode. Interactive mode used to bail + // with only the transient debug line above, so a blocked /clear + // looked like the command silently did nothing (issue #5949). + return { + type: 'message' as const, + messageType: 'error' as const, + content, + }; } // Fire SessionEnd event (non-blocking to avoid UI lag) diff --git a/packages/cli/src/ui/hooks/useResumeCommand.test.ts b/packages/cli/src/ui/hooks/useResumeCommand.test.ts index 3f7c8be2a0..57908beb21 100644 --- a/packages/cli/src/ui/hooks/useResumeCommand.test.ts +++ b/packages/cli/src/ui/hooks/useResumeCommand.test.ts @@ -235,7 +235,7 @@ describe('useResumeCommand', () => { getGeminiClient: () => geminiClient, startNewSession: vi.fn(), getBackgroundTaskRegistry: () => ({ - hasUnfinalizedTasks: vi.fn().mockReturnValue(false), + hasRunningTasks: vi.fn().mockReturnValue(false), reset: vi.fn(), }), getBackgroundShellRegistry: () => ({ @@ -331,7 +331,7 @@ describe('useResumeCommand', () => { getGeminiClient: () => geminiClient, startNewSession: vi.fn(), getBackgroundTaskRegistry: () => ({ - hasUnfinalizedTasks: vi.fn().mockReturnValue(false), + hasRunningTasks: vi.fn().mockReturnValue(false), reset: vi.fn(), }), getBackgroundShellRegistry: () => ({ @@ -424,7 +424,7 @@ describe('useResumeCommand', () => { getGeminiClient: () => geminiClient, startNewSession: vi.fn(), getBackgroundTaskRegistry: () => ({ - hasUnfinalizedTasks: vi.fn().mockReturnValue(false), + hasRunningTasks: vi.fn().mockReturnValue(false), reset: vi.fn(), }), getBackgroundShellRegistry: () => ({ @@ -489,7 +489,7 @@ describe('useResumeCommand', () => { const config = { getBackgroundTaskRegistry: () => ({ - hasUnfinalizedTasks: vi.fn().mockReturnValue(true), + hasRunningTasks: vi.fn().mockReturnValue(true), reset: vi.fn(), }), getBackgroundShellRegistry: () => ({ @@ -554,7 +554,7 @@ describe('useResumeCommand', () => { const config = { getBackgroundTaskRegistry: () => ({ - hasUnfinalizedTasks: vi.fn().mockReturnValue(false), + hasRunningTasks: vi.fn().mockReturnValue(false), reset: vi.fn(), }), getBackgroundShellRegistry: () => ({ @@ -629,7 +629,7 @@ describe('useResumeCommand', () => { getGeminiClient: () => geminiClient, startNewSession: vi.fn(), getBackgroundTaskRegistry: () => ({ - hasUnfinalizedTasks: vi.fn().mockReturnValue(false), + hasRunningTasks: vi.fn().mockReturnValue(false), reset: vi.fn(), }), getBackgroundShellRegistry: () => ({ diff --git a/packages/cli/src/ui/utils/backgroundWorkUtils.test.ts b/packages/cli/src/ui/utils/backgroundWorkUtils.test.ts index df2b76d3a4..44e80c39ac 100644 --- a/packages/cli/src/ui/utils/backgroundWorkUtils.test.ts +++ b/packages/cli/src/ui/utils/backgroundWorkUtils.test.ts @@ -12,14 +12,14 @@ import { } from './backgroundWorkUtils.js'; function createMockConfig(overrides?: { - hasUnfinalizedTasks?: boolean; + hasRunningTasks?: boolean; runningMonitors?: unknown[]; hasRunningEntries?: boolean; hasRunningWorkflows?: boolean; }): Config { return { getBackgroundTaskRegistry: () => ({ - hasUnfinalizedTasks: () => overrides?.hasUnfinalizedTasks ?? false, + hasRunningTasks: () => overrides?.hasRunningTasks ?? false, reset: vi.fn(), }), getMonitorRegistry: () => ({ @@ -42,11 +42,13 @@ describe('hasBlockingBackgroundWork', () => { expect(hasBlockingBackgroundWork(createMockConfig())).toBe(false); }); - it('returns true when background tasks are unfinalized', () => { + // #5949: the gate keys off hasRunningTasks(), NOT hasUnfinalizedTasks() + // — a cancelled task whose finalize callback hasn't fired yet must not + // block /clear or session resume (both abort-and-reset right after the + // gate, suppressing the pending notification anyway). + it('returns true when background tasks are still running', () => { expect( - hasBlockingBackgroundWork( - createMockConfig({ hasUnfinalizedTasks: true }), - ), + hasBlockingBackgroundWork(createMockConfig({ hasRunningTasks: true })), ).toBe(true); }); @@ -75,10 +77,10 @@ describe('hasBlockingBackgroundWork', () => { ).toBe(true); }); - it('short-circuits: does not check monitors or shells when tasks are unfinalized', () => { + it('short-circuits: does not check monitors or shells when tasks are running', () => { const config = { getBackgroundTaskRegistry: () => ({ - hasUnfinalizedTasks: () => true, + hasRunningTasks: () => true, reset: vi.fn(), }), getMonitorRegistry: () => { @@ -95,7 +97,7 @@ describe('hasBlockingBackgroundWork', () => { it('short-circuits: does not check shells when monitors are running', () => { const config = { getBackgroundTaskRegistry: () => ({ - hasUnfinalizedTasks: () => false, + hasRunningTasks: () => false, reset: vi.fn(), }), getMonitorRegistry: () => ({ diff --git a/packages/cli/src/ui/utils/backgroundWorkUtils.ts b/packages/cli/src/ui/utils/backgroundWorkUtils.ts index 2df65ae500..2302ef53af 100644 --- a/packages/cli/src/ui/utils/backgroundWorkUtils.ts +++ b/packages/cli/src/ui/utils/backgroundWorkUtils.ts @@ -8,7 +8,13 @@ import type { Config } from '@qwen-code/qwen-code-core'; export function hasBlockingBackgroundWork(config: Config): boolean { return ( - config.getBackgroundTaskRegistry().hasUnfinalizedTasks() || + // hasRunningTasks, not hasUnfinalizedTasks: a cancelled task whose + // finalize callback hasn't fired yet must not block /clear or + // session resume — both abort-and-reset the registry right after + // this gate, suppressing the pending notification anyway. Gating on + // the unfinalized set made /new silently no-op when typed in the + // window between cancel and finalize (issue #5949). + config.getBackgroundTaskRegistry().hasRunningTasks() || config.getMonitorRegistry().getRunning().length > 0 || config.getBackgroundShellRegistry().hasRunningEntries() || // R7 (wenshao): the WorkflowRunRegistry is a 4th sibling that the diff --git a/packages/core/src/agents/background-tasks.test.ts b/packages/core/src/agents/background-tasks.test.ts index 982dc85c10..9e121a8864 100644 --- a/packages/core/src/agents/background-tasks.test.ts +++ b/packages/core/src/agents/background-tasks.test.ts @@ -873,6 +873,44 @@ describe('BackgroundTaskRegistry', () => { expect(registry.hasUnfinalizedTasks()).toBe(false); }); + it('hasRunningTasks ignores cancelled-but-not-notified entries (#5949)', () => { + // Session-switch gates (/clear, /resume) key off hasRunningTasks so a + // task the user just cancelled — aborted, only its terminal + // notification outstanding — cannot make the switch silently no-op. + // hasUnfinalizedTasks must still report it for the headless holdback. + registry.register({ + agentId: 'test-1', + description: 'test agent', + status: 'running', + startTime: Date.now(), + abortController: new AbortController(), + isBackgrounded: true, + outputFile: '/tmp/test.jsonl', + }); + expect(registry.hasRunningTasks()).toBe(true); + + registry.cancel('test-1'); + expect(registry.get('test-1')!.status).toBe('cancelled'); + expect(registry.hasRunningTasks()).toBe(false); + expect(registry.hasUnfinalizedTasks()).toBe(true); + + registry.finalizeCancelled('test-1', ''); + expect(registry.hasRunningTasks()).toBe(false); + }); + + it('hasRunningTasks ignores foreground entries', () => { + registry.register({ + agentId: 'fg-1', + description: 'foreground agent', + status: 'running', + startTime: Date.now(), + abortController: new AbortController(), + isBackgrounded: false, + outputFile: '/tmp/test.jsonl', + }); + expect(registry.hasRunningTasks()).toBe(false); + }); + it('hasUnfinalizedTasks clears once every entry has been notified', () => { registry.register({ agentId: 'a', diff --git a/packages/core/src/agents/background-tasks.ts b/packages/core/src/agents/background-tasks.ts index 7c6df2f017..9774fb6861 100644 --- a/packages/core/src/agents/background-tasks.ts +++ b/packages/core/src/agents/background-tasks.ts @@ -1008,6 +1008,26 @@ export class BackgroundTaskRegistry { return false; } + /** + * True while any background entry is still actually executing. Unlike + * `hasUnfinalizedTasks()`, a `cancelled`-but-not-yet-finalized entry + * does NOT count: its work has already been aborted and only the + * terminal task-notification is outstanding. Session-switch gates + * (/clear, /resume) key off this instead — they abort-and-reset the + * registry right after passing the gate, which suppresses that very + * notification, so blocking on it made the command silently no-op + * when the user cleared immediately after cancelling (issue #5949). + * Headless holdback loops must keep using `hasUnfinalizedTasks()` so + * every task_started still pairs with a task_notification. + */ + hasRunningTasks(): boolean { + for (const entry of this.agents.values()) { + if (!entry.isBackgrounded) continue; + if (entry.status === 'running') return true; + } + return false; + } + /** * Drops every in-memory entry without touching sidecar state. *