From 59c5ccb779084162eb812311f8fdd9456154889a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E9=92=89=E8=90=81?= Date: Mon, 10 Aug 2026 16:33:30 +0800 Subject: [PATCH 01/10] fix(web-shell): surface loop detection turn errors --- .../web-shell-loop-detection-turn-error.md | 21 +++ packages/acp-bridge/src/bridge.test.ts | 114 ++++++++++++ packages/acp-bridge/src/bridge.ts | 34 +++- .../acp-integration/session/Session.test.ts | 164 ++++++++++++------ .../src/acp-integration/session/Session.ts | 73 +++++--- packages/sdk-typescript/src/daemon/events.ts | 1 + packages/sdk-typescript/src/daemon/types.ts | 2 + packages/web-shell/client/App.test.tsx | 24 +++ packages/web-shell/client/App.tsx | 4 +- .../adapters/transcriptToMessages.test.ts | 24 +++ .../client/adapters/transcriptToMessages.ts | 7 +- .../client/hooks/useMessages.test.ts | 10 ++ .../web-shell/client/hooks/useMessages.ts | 1 + packages/web-shell/client/i18n.tsx | 4 + 14 files changed, 399 insertions(+), 84 deletions(-) create mode 100644 docs/design/web-shell-loop-detection-turn-error.md diff --git a/docs/design/web-shell-loop-detection-turn-error.md b/docs/design/web-shell-loop-detection-turn-error.md new file mode 100644 index 00000000000..23269794dc3 --- /dev/null +++ b/docs/design/web-shell-loop-detection-turn-error.md @@ -0,0 +1,21 @@ +# Web Shell loop-detection turn errors + +## Problem + +ACP loop protection currently records unstarted tool calls as failures and then completes the prompt with `stopReason: end_turn`. Web Shell therefore presents the internal tool skip text as the only explanation and treats the turn as successful. + +## Design + +When a foreground ACP prompt is stopped by loop protection, preserve completed and skipped tool results as today, then reject that prompt with a structured ACP request error. The bridge publishes the existing `turn_error` terminal with `errorKind: loop_detected` and the detector's `loopType`. Cancellation continues to take precedence when it races the loop stop. + +Web Shell renders `loop_detected` from the structured kind, using localized plain language: the model repeated tool use or reached a safety limit, only the current turn stopped, and the user can continue with a more specific instruction. No client matches the internal English tool error. + +Skipped tools keep their existing failed terminal update and error details so they cannot remain pending and their display behavior does not change. The additional `turn_error` provides the user-facing explanation for the stopped turn. + +The session remains alive and the per-turn loop state is recreated for the next prompt. Cron and background-notification work keep their existing non-interactive handling. + +When Web Shell reloads a live session from paginated persisted history, the bridge appends the current in-memory `turn_error` to that replay. This keeps the terminal error visible across a page refresh without changing historical persistence. + +## Compatibility + +`turn_error` already terminates prompts and returns the UI to idle. Adding a known error kind and optional metadata is backward-compatible: older clients show the daemon message, while updated clients show localized guidance. diff --git a/packages/acp-bridge/src/bridge.test.ts b/packages/acp-bridge/src/bridge.test.ts index 5892db8d820..b6f2a877723 100644 --- a/packages/acp-bridge/src/bridge.test.ts +++ b/packages/acp-bridge/src/bridge.test.ts @@ -4084,6 +4084,65 @@ describe('createAcpSessionBridge', () => { await bridge.shutdown(); }); + it('keeps the current turn error when refreshing from persisted history', async () => { + const handle = makeChannel({ + promptImpl: () => { + throw new RequestError(-32603, 'Loop protection stopped this turn', { + code: 'LOOP_DETECTED', + errorKind: 'loop_detected', + loopType: 'turn_tool_call_cap', + }); + }, + extMethodImpl: (method, params) => { + if (method !== SERVE_STATUS_EXT_METHODS.sessionTranscript) { + throw new Error(`unexpected extMethod ${method}`); + } + return { + v: 1, + sessionId: params['sessionId'], + events: [], + hasMore: false, + }; + }, + }); + const bridge = makeBridge({ channelFactory: async () => handle.channel }); + const session = await bridge.spawnOrAttach({ workspaceCwd: WS_A }); + + await expect( + bridge.sendPrompt( + session.sessionId, + { + sessionId: session.sessionId, + prompt: [{ type: 'text', text: 'loop' }], + }, + undefined, + { promptId: 'prompt-loop' }, + ), + ).rejects.toThrow('Loop protection stopped this turn'); + + const refreshed = await bridge.loadSession({ + sessionId: session.sessionId, + workspaceCwd: WS_A, + clientId: session.clientId, + historyReplay: 'response', + historyPageSize: 100, + }); + + expect(refreshed.compactedReplay).toContainEqual( + expect.objectContaining({ + type: 'turn_error', + promptId: 'prompt-loop', + data: expect.objectContaining({ + code: 'LOOP_DETECTED', + errorKind: 'loop_detected', + loopType: 'turn_tool_call_cap', + }), + }), + ); + + await bridge.shutdown(); + }); + it('propagates partial and replayError from a bounded refresh', async () => { const handle = makeChannel({ loadSessionImpl: () => ({ @@ -8397,6 +8456,61 @@ describe('createAcpSessionBridge', () => { await bridge.shutdown(); }); + it('preserves structured loop detection details on turn_error', async () => { + const handle = makeChannel({ + promptImpl: () => { + throw new RequestError(-32603, 'Loop protection stopped this turn', { + code: 'LOOP_DETECTED', + errorKind: 'loop_detected', + loopType: 'turn_tool_call_cap', + }); + }, + }); + const bridge = makeBridge({ channelFactory: async () => handle.channel }); + const session = await bridge.spawnOrAttach({ workspaceCwd: WS_A }); + const abort = new AbortController(); + const iter = bridge.subscribeEvents(session.sessionId, { + signal: abort.signal, + }); + const turnError = (async () => { + for await (const event of iter) { + if (event.type === 'turn_error') return event; + } + throw new Error('turn_error was not published'); + })(); + + await expect( + bridge.sendPrompt( + session.sessionId, + { + sessionId: session.sessionId, + prompt: [{ type: 'text', text: 'loop' }], + }, + undefined, + { promptId: 'prompt-loop' }, + ), + ).rejects.toThrow('Loop protection stopped this turn'); + + await expect(turnError).resolves.toMatchObject({ + type: 'turn_error', + promptId: 'prompt-loop', + data: { + code: 'LOOP_DETECTED', + errorKind: 'loop_detected', + loopType: 'turn_tool_call_cap', + promptId: 'prompt-loop', + }, + }); + expect(bridge.getSessionSummary(session.sessionId).turnError).toEqual({ + message: 'Loop protection stopped this turn', + code: 'LOOP_DETECTED', + errorKind: 'loop_detected', + }); + + abort.abort(); + await bridge.shutdown(); + }); + it('echoes user_message_chunk to ALL session subscribers (cross-client sync)', async () => { // Cross-client sync fix: a prompt sent by client A must be visible // to every SSE subscriber of the same session — not just the diff --git a/packages/acp-bridge/src/bridge.ts b/packages/acp-bridge/src/bridge.ts index 833d1de2d7a..3b27d9066f0 100644 --- a/packages/acp-bridge/src/bridge.ts +++ b/packages/acp-bridge/src/bridge.ts @@ -1182,6 +1182,17 @@ function extractJsonRpcErrorDetail(data: unknown): string | undefined { return undefined; } +function extractJsonRpcErrorField( + err: unknown, + field: string, +): string | undefined { + if (typeof err !== 'object' || err === null) return undefined; + const data = (err as { data?: unknown }).data; + if (typeof data !== 'object' || data === null) return undefined; + const value = (data as Record)[field]; + return typeof value === 'string' && value.length > 0 ? value : undefined; +} + export function extractErrorCode(err: unknown): string | undefined { if (typeof err !== 'object' || err === null || !('code' in err)) return undefined; @@ -1208,8 +1219,13 @@ function broadcastTurnError( mutateTurnState: boolean, ): void { const message = extractErrorMessage(err); - const code = extractErrorCode(err); - const errorKind = classifyTurnErrorKind(message); + const structuredErrorKind = extractJsonRpcErrorField(err, 'errorKind'); + const errorKind = structuredErrorKind ?? classifyTurnErrorKind(message); + const code = + structuredErrorKind === 'loop_detected' + ? (extractJsonRpcErrorField(err, 'code') ?? extractErrorCode(err)) + : extractErrorCode(err); + const loopType = extractJsonRpcErrorField(err, 'loopType'); if (errorKind) { writeServeDebugLine( `turn_error classified session=${JSON.stringify(sessionId)} ` + @@ -1242,6 +1258,7 @@ function broadcastTurnError( message, ...(code ? { code } : {}), ...(errorKind ? { errorKind } : {}), + ...(loopType ? { loopType } : {}), ...(promptId ? { promptId } : {}), }, ...(originatorClientId ? { originatorClientId } : {}), @@ -4920,8 +4937,19 @@ export function createAcpSessionBridge(opts: BridgeOptions): AcpSessionBridge { !entry.promptActive && entry.events.lastEventId === lastEventId ) { + const replay = entry.events.snapshotReplay(); + const turnError = entry.turnError + ? [ + ...(replay?.compactedTurns ?? []), + ...(replay?.liveJournal ?? []), + ] + .reverse() + .find((event) => event.type === 'turn_error') + : undefined; return { - compactedReplay: page.events, + compactedReplay: turnError + ? [...page.events, turnError] + : page.events, liveJournal: [], lastEventId, ...(page.partial === true ? { partial: true as const } : {}), diff --git a/packages/cli/src/acp-integration/session/Session.test.ts b/packages/cli/src/acp-integration/session/Session.test.ts index 1e509b2d55b..5a227feebf4 100644 --- a/packages/cli/src/acp-integration/session/Session.test.ts +++ b/packages/cli/src/acp-integration/session/Session.test.ts @@ -6858,22 +6858,45 @@ describe('Session', () => { args: { file_path: `file_${index}.ts` }, })); functionCalls[101].id = 'read_0'; - mockChat.sendMessageStream = vi.fn().mockResolvedValueOnce( - createStreamWithChunks([ - { - type: core.StreamEventType.CHUNK, - value: { functionCalls }, - }, - ]), - ); + mockChat.sendMessageStream = vi + .fn() + .mockResolvedValueOnce( + createStreamWithChunks([ + { + type: core.StreamEventType.CHUNK, + value: { functionCalls }, + }, + ]), + ) + .mockResolvedValueOnce(createEmptyStream()); - await session.prompt({ - sessionId: 'test-session-id', - prompt: [{ type: 'text', text: 'read many files' }], + await expect( + session.prompt({ + sessionId: 'test-session-id', + prompt: [{ type: 'text', text: 'read many files' }], + }), + ).rejects.toMatchObject({ + data: { + code: 'LOOP_DETECTED', + errorKind: 'loop_detected', + loopType: core.LoopType.TURN_TOOL_CALL_CAP, + }, }); expect(mockChat.sendMessageStream).toHaveBeenCalledTimes(1); expect(mockToolRegistry.getTool).not.toHaveBeenCalled(); + const skippedUpdates = vi + .mocked(mockClient.sessionUpdate) + .mock.calls.map(([params]) => params.update) + .filter( + (update) => + update.sessionUpdate === 'tool_call_update' && + update.status === 'failed' && + JSON.stringify(update).includes( + 'Skipped because loop detection stopped the current turn', + ), + ); + expect(skippedUpdates).toHaveLength(101); expect(mockChat.addHistory).toHaveBeenCalledWith({ role: 'user', parts: expect.arrayContaining([ @@ -6910,6 +6933,14 @@ describe('Session', () => { 'Stopping ACP turn after 101 tool calls in one turn.', ), ); + + await expect( + session.prompt({ + sessionId: 'test-session-id', + prompt: [{ type: 'text', text: 'continue with a simpler step' }], + }), + ).resolves.toEqual({ stopReason: 'end_turn' }); + expect(mockChat.sendMessageStream).toHaveBeenCalledTimes(2); }); it('lets a productive turn continue past the default cap (adaptive)', async () => { @@ -8251,7 +8282,13 @@ describe('Session', () => { sessionId: 'test-session-id', prompt: [{ type: 'text', text: 'run the failing tool' }], }), - ).resolves.toEqual({ stopReason: 'end_turn' }); + ).rejects.toMatchObject({ + data: { + code: 'LOOP_DETECTED', + errorKind: 'loop_detected', + loopType: core.LoopType.REPEATED_TOOL_EXECUTION_FAILURE, + }, + }); expect(execute).toHaveBeenCalledTimes(9); expect(mockChat.sendMessageStream).toHaveBeenCalledTimes(3); @@ -8282,20 +8319,16 @@ describe('Session', () => { }), ]), }); - expect(mockClient.sessionUpdate).toHaveBeenCalledWith( - expect.objectContaining({ - sessionId: 'test-session-id', - update: expect.objectContaining({ - sessionUpdate: 'agent_message_chunk', - content: expect.objectContaining({ - type: 'text', - text: expect.stringContaining( - 'Automatic continuation stopped', - ), - }), - }), + expect( + vi.mocked(mockClient.sessionUpdate).mock.calls.some(([params]) => { + const update = params.update; + return ( + update.sessionUpdate === 'agent_message_chunk' && + update.content.type === 'text' && + update.content.text.includes('Automatic continuation stopped') + ); }), - ); + ).toBe(false); } finally { restoreGuardMode(); } @@ -8344,36 +8377,6 @@ describe('Session', () => { restoreGuardMode(); } }); - - it('returns cancelled when cancellation arrives while the stop message is emitted', async () => { - recreateSessionWithGuardMode('enforce'); - try { - installFailingTool(); - vi.mocked(mockClient.sessionUpdate).mockImplementation( - async ({ update }) => { - if ( - update.sessionUpdate === 'agent_message_chunk' && - update.content.type === 'text' && - update.content.text.includes('Automatic continuation stopped') - ) { - await session.cancelPendingPrompt(); - } - }, - ); - queueMatchingFailureStreak(); - - await expect( - session.prompt({ - sessionId: 'test-session-id', - prompt: [{ type: 'text', text: 'run the failing tool' }], - }), - ).resolves.toEqual({ stopReason: 'cancelled' }); - - expect(mockChat.sendMessageStream).toHaveBeenCalledTimes(3); - } finally { - restoreGuardMode(); - } - }); }); describe('shell heartbeat forwarding', () => { @@ -23979,6 +23982,57 @@ describe('Session', () => { await expect(prompt).resolves.toEqual({ stopReason: 'cancelled' }); }); + it('lets cancellation win while a loop-detected Stop continuation is preserved', async () => { + mockConfig.getMaxToolCallsPerTurn = vi.fn().mockReturnValue(1); + mockConfig.isMaxToolCallsPerTurnExplicit = vi.fn().mockReturnValue(true); + mockChat.sendMessageStream = vi + .fn() + .mockResolvedValueOnce(createEmptyStream()) + .mockResolvedValueOnce( + createStreamWithChunks([ + { + type: core.StreamEventType.CHUNK, + value: { + functionCalls: [ + { id: 'loop-1', name: 'read_file', args: { path: 'a' } }, + { id: 'loop-2', name: 'read_file', args: { path: 'b' } }, + ], + }, + }, + ]), + ); + const messageBus = { + request: vi.fn().mockResolvedValue({ + success: true, + output: { decision: 'block', reason: 'continue once' }, + }), + }; + mockConfig.getMessageBus = vi.fn().mockReturnValue(messageBus); + mockConfig.hasHooksForEvent = vi + .fn() + .mockImplementation((name: string) => name === 'Stop'); + let startDrain!: () => void; + const drainStarted = new Promise((resolve) => { + startDrain = resolve; + }); + let releaseDrain!: () => void; + const drainGate = new Promise((resolve) => { + releaseDrain = resolve; + }); + mockClient.extMethod = vi.fn(async () => { + startDrain(); + await drainGate; + return { messages: [] }; + }); + + const prompt = runGuardPrompt(); + await drainStarted; + await session.cancelPendingPrompt(); + releaseDrain(); + + await expect(prompt).resolves.toEqual({ stopReason: 'cancelled' }); + }); + it('runs exactly two continuations and emits replayable status', async () => { rebuildSessionWithGuard(); installPendingTodoTool(); diff --git a/packages/cli/src/acp-integration/session/Session.ts b/packages/cli/src/acp-integration/session/Session.ts index bc2ab1d8bd9..604d4f3ebdd 100644 --- a/packages/cli/src/acp-integration/session/Session.ts +++ b/packages/cli/src/acp-integration/session/Session.ts @@ -487,6 +487,7 @@ export type DaemonToolLoopState = { /** Highest repeat count of any single (tool, args) pair this turn. */ maxToolCallKeyRepeat: number; loopDetected: boolean; + loopType?: LoopType; repeatedToolFailureMode: RepeatedToolFailureGuardMode; repeatedToolFailureState: RepeatedToolFailureGuardState; }; @@ -499,6 +500,8 @@ const LOOP_DETECTED_SKIP_MESSAGE = 'Skipped because loop detection stopped the current turn before this tool call could run.'; const LOOP_DETECTED_CONTEXT_MESSAGE = 'System: this turn was terminated because the model exceeded tool-call safety limits. Try a different approach on the next turn.'; +const LOOP_DETECTED_TURN_ERROR_MESSAGE = + 'Tool-call loop protection stopped this turn. The session is still available; send a more specific instruction to continue.'; const TOOL_EXECUTION_CANCELLED_MESSAGE = 'Tool execution was cancelled.'; const TOOL_POST_EXECUTION_CANCELLED_MESSAGE = 'The tool had already completed; its output was discarded.'; @@ -604,6 +607,7 @@ function recordDaemonLoopDetected( ): true { if (!loopState.loopDetected) { loopState.loopDetected = true; + loopState.loopType = loopType; debugLogger.warn(message); try { logLoopDetected( @@ -621,6 +625,16 @@ function recordDaemonLoopDetected( return true; } +function createLoopDetectedTurnError( + loopState: DaemonToolLoopState, +): RequestError { + return new RequestError(-32603, LOOP_DETECTED_TURN_ERROR_MESSAGE, { + code: 'LOOP_DETECTED', + errorKind: 'loop_detected', + ...(loopState.loopType ? { loopType: loopState.loopType } : {}), + }); +} + function recordDaemonToolCalls( config: Config, promptId: string, @@ -3920,11 +3934,10 @@ export class Session implements SessionContext { ); nextMessage = nextAfterTools.message; if (nextAfterTools.stoppedByRepeatedToolFailure) { - return { - stopReason: getAbortAwareEndTurnStopReason( - pendingSend.signal, - ), - }; + if (pendingSend.signal.aborted) { + return { stopReason: 'cancelled' }; + } + throw createLoopDetectedTurnError(toolLoopState); } if (toolRun.loopDetected) { this.todoStopGuard.suspend(); @@ -3932,11 +3945,10 @@ export class Session implements SessionContext { toolRun, pendingSend.signal, ); - return { - stopReason: getAbortAwareEndTurnStopReason( - pendingSend.signal, - ), - }; + if (pendingSend.signal.aborted) { + return { stopReason: 'cancelled' }; + } + throw createLoopDetectedTurnError(toolLoopState); } } } @@ -4853,11 +4865,7 @@ export class Session implements SessionContext { options.onFullTurnModel, ), ); - if ( - toolRun.stopAfterPermissionCancel || - toolRun.loopDetected || - pendingSend.signal.aborted - ) { + if (toolRun.stopAfterPermissionCancel || pendingSend.signal.aborted) { this.todoStopGuard.suspend(); await this.#preserveStoppedToolRun(toolRun, pendingSend.signal); return { @@ -4868,6 +4876,20 @@ export class Session implements SessionContext { : {}), }; } + if (toolRun.loopDetected) { + this.todoStopGuard.suspend(); + await this.#preserveStoppedToolRun(toolRun, pendingSend.signal); + if (pendingSend.signal.aborted) { + return { + kind: 'terminal', + stopReason: 'cancelled', + ...(supersededAutomaticContinuation + ? { supersededAutomaticContinuation: true } + : {}), + }; + } + throw createLoopDetectedTurnError(toolLoopState); + } const nextAfterTools = await this.#buildNextMessageAfterToolRun( toolRun, pendingSend.signal, @@ -4876,6 +4898,18 @@ export class Session implements SessionContext { options.onFullTurnModel, ); nextMessage = nextAfterTools.message; + if (nextAfterTools.stoppedByRepeatedToolFailure) { + if (pendingSend.signal.aborted) { + return { + kind: 'terminal', + stopReason: 'cancelled', + ...(supersededAutomaticContinuation + ? { supersededAutomaticContinuation: true } + : {}), + }; + } + throw createLoopDetectedTurnError(toolLoopState); + } if (nextAfterTools.hadMidTurnUserInput) { nextGuardContinuation = undefined; continue; @@ -5405,15 +5439,6 @@ export class Session implements SessionContext { toolLoopState, { recordToQwenLogger: false }, ); - try { - await this.messageEmitter.emitAgentMessage( - REPEATED_TOOL_FAILURE_STOP_MESSAGE, - ); - } catch (error) { - debugLogger.warn( - `Failed to emit repeated tool failure stop message: ${this.#formatError(error)}`, - ); - } return { message: null, hadMidTurnUserInput, diff --git a/packages/sdk-typescript/src/daemon/events.ts b/packages/sdk-typescript/src/daemon/events.ts index 55423cf5d3d..a8b5e3db6f6 100644 --- a/packages/sdk-typescript/src/daemon/events.ts +++ b/packages/sdk-typescript/src/daemon/events.ts @@ -838,6 +838,7 @@ export interface DaemonTurnErrorData { message: string; code?: string; errorKind?: DaemonErrorKind | (string & {}); + loopType?: string; promptId?: string; [key: string]: unknown; } diff --git a/packages/sdk-typescript/src/daemon/types.ts b/packages/sdk-typescript/src/daemon/types.ts index 5032dfde388..b0a927765ff 100644 --- a/packages/sdk-typescript/src/daemon/types.ts +++ b/packages/sdk-typescript/src/daemon/types.ts @@ -1436,6 +1436,8 @@ export const DAEMON_ERROR_KINDS = [ 'writer_idle_timeout', // The model response stream ended before a complete turn could be read. 'model_stream_interrupted', + // Tool-call loop protection stopped the current turn. + 'loop_detected', ] as const; export type DaemonErrorKind = (typeof DAEMON_ERROR_KINDS)[number]; diff --git a/packages/web-shell/client/App.test.tsx b/packages/web-shell/client/App.test.tsx index 4cf52f5697e..7db6648ad98 100644 --- a/packages/web-shell/client/App.test.tsx +++ b/packages/web-shell/client/App.test.tsx @@ -10809,6 +10809,30 @@ describe('App session callbacks', () => { }); }); + it('asks for a new instruction instead of retrying a loop-detected turn', async () => { + const { container, rerender } = renderApp(); + await flush(); + + testState.prompt = 'repeat this'; + await clickSubmit(container); + + act(() => { + testState.blocks = [ + { + kind: 'error', + source: 'turn_error', + id: 'turn-error-loop', + errorKind: 'loop_detected', + text: 'internal fallback', + }, + ]; + rerender(); + }); + + expect(container.querySelector('[data-testid="retry"]')).toBeNull(); + expect(testState.latestChatEditorProps?.disabled).toBe(false); + }); + it('locks an image retry when its admission response is lost', async () => { const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}); const retrySend = deferred(); diff --git a/packages/web-shell/client/App.tsx b/packages/web-shell/client/App.tsx index b12cd27e7ea..7e0150f55d7 100644 --- a/packages/web-shell/client/App.tsx +++ b/packages/web-shell/client/App.tsx @@ -6053,7 +6053,9 @@ export function App({ const block = blocks[i]; if (block?.kind === 'user') break; if (block?.kind === 'error' && block.source === 'turn_error') { - retryableTurnErrorId = block.id; + if (block.errorKind !== 'loop_detected') { + retryableTurnErrorId = block.id; + } break; } if (block?.kind !== 'debug') break; diff --git a/packages/web-shell/client/adapters/transcriptToMessages.test.ts b/packages/web-shell/client/adapters/transcriptToMessages.test.ts index 83ee4addb67..fd60c467a84 100644 --- a/packages/web-shell/client/adapters/transcriptToMessages.test.ts +++ b/packages/web-shell/client/adapters/transcriptToMessages.test.ts @@ -2733,6 +2733,30 @@ describe('transcriptBlocksToDaemonMessages', () => { ]); }); + it('renders loop detection errors from a structured localized label', () => { + const messages = transcriptBlocksToDaemonMessages( + [ + { + id: 'err-loop', + kind: 'error' as const, + source: 'turn_error' as const, + errorKind: 'loop_detected' as const, + text: 'internal fallback', + clientReceivedAt: 1, + createdAt: 1, + updatedAt: 1, + }, + ], + { labels: { loopDetected: 'Localized loop guidance.' } }, + ); + + expect(messages[0]).toMatchObject({ + content: 'Localized loop guidance.', + retryable: false, + source: 'turn_error', + }); + }); + it('renders model stream interruption errors from structured errorKind labels', () => { const messages = transcriptBlocksToDaemonMessages( [ diff --git a/packages/web-shell/client/adapters/transcriptToMessages.ts b/packages/web-shell/client/adapters/transcriptToMessages.ts index 3b885dbadeb..26e09bd4db2 100644 --- a/packages/web-shell/client/adapters/transcriptToMessages.ts +++ b/packages/web-shell/client/adapters/transcriptToMessages.ts @@ -46,6 +46,7 @@ interface TranscriptMessageLabels { branchSuccess?: (name: string) => string; midTurnInserted?: (message: string) => string; modelStreamInterrupted?: string; + loopDetected?: string; } interface TranscriptMessageOptions { @@ -195,6 +196,9 @@ function getErrorDisplayText( block: DaemonStatusTranscriptBlock, labels?: TranscriptMessageLabels, ): string { + if (block.errorKind === 'loop_detected') { + return labels?.loopDetected ?? block.text; + } if ( block.errorKind === 'model_stream_interrupted' || // Older daemons emit this turn_error before they know about errorKind. @@ -775,7 +779,8 @@ export function transcriptBlocksToDaemonMessages( role: 'system', content: getErrorDisplayText(errorBlock, options.labels), variant: 'error', - retryable: errorBlock.source === 'turn_error', + retryable: + errorBlock.source === 'turn_error' && errorKind !== 'loop_detected', timestamp: blockTime, ...(errorBlock.source ? { source: errorBlock.source } : {}), ...getErrorMessageData(errorBlock.data, errorKind), diff --git a/packages/web-shell/client/hooks/useMessages.test.ts b/packages/web-shell/client/hooks/useMessages.test.ts index 6e03fb68ae6..806a97e21fe 100644 --- a/packages/web-shell/client/hooks/useMessages.test.ts +++ b/packages/web-shell/client/hooks/useMessages.test.ts @@ -77,12 +77,22 @@ describe('transcriptBlocksToLocalizedMessages', () => { DaemonStatusTranscriptBlock, 'clientReceivedAt' | 'createdAt' | 'updatedAt' >), + baseBlock({ + id: 'loop', + kind: 'error', + text: 'internal fallback', + errorKind: 'loop_detected', + } as Omit< + DaemonStatusTranscriptBlock, + 'clientReceivedAt' | 'createdAt' | 'updatedAt' + >), ]; expect(transcriptBlocksToLocalizedMessages(blocks, t)).toMatchObject([ { content: 'localized:request.cancelled' }, { content: 'branch.success:review' }, { content: 'localized:error.modelStreamInterrupted' }, + { content: 'localized:error.loopDetected' }, ]); }); }); diff --git a/packages/web-shell/client/hooks/useMessages.ts b/packages/web-shell/client/hooks/useMessages.ts index 64e07d95919..0c7dfd060f5 100644 --- a/packages/web-shell/client/hooks/useMessages.ts +++ b/packages/web-shell/client/hooks/useMessages.ts @@ -32,6 +32,7 @@ export function transcriptBlocksToLocalizedMessages( branchSuccess: (name) => t('branch.success', { name }), midTurnInserted: (message) => t('midTurn.inserted', { message }), modelStreamInterrupted: t('error.modelStreamInterrupted'), + loopDetected: t('error.loopDetected'), }, }); } diff --git a/packages/web-shell/client/i18n.tsx b/packages/web-shell/client/i18n.tsx index 7c7f1afb346..6d72d074a39 100644 --- a/packages/web-shell/client/i18n.tsx +++ b/packages/web-shell/client/i18n.tsx @@ -1440,6 +1440,8 @@ const EN: Messages = { 'error.unknown': 'Unknown error', 'error.modelStreamInterrupted': 'Model response stream was interrupted. Please retry.', + 'error.loopDetected': + 'The model got stuck while using tools or reached a safety limit, so this turn was stopped. Your session is still open—try a more specific instruction to continue.', 'shell.command': 'Shell Command', 'compact.enabled': 'Compact mode enabled', 'compact.disabled': 'Compact mode disabled', @@ -4200,6 +4202,8 @@ const ZH: Messages = { 'clear.blocked': '流式输出中无法清屏 — 先按 Esc 取消。', 'error.unknown': '未知错误', 'error.modelStreamInterrupted': '模型响应流已中断，请重试。', + 'error.loopDetected': + '模型在调用工具时反复尝试或达到了安全上限，因此系统停止了本轮操作。会话并未结束，你可以换一个更明确的指令继续。', 'shell.command': 'Shell 命令', 'compact.enabled': '紧凑模式已开启', 'compact.disabled': '紧凑模式已关闭', From c004d559a2ffddd18e8019fa5ecb527e9b040417 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E9=92=89=E8=90=81?= Date: Mon, 10 Aug 2026 10:14:50 +0000 Subject: [PATCH 02/10] fix(cli): expect loop-detected turn error in invalid-params stop test (#8853) --- .../src/acp-integration/session/Session.test.ts | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/packages/cli/src/acp-integration/session/Session.test.ts b/packages/cli/src/acp-integration/session/Session.test.ts index 5a227feebf4..112fe67f5ee 100644 --- a/packages/cli/src/acp-integration/session/Session.test.ts +++ b/packages/cli/src/acp-integration/session/Session.test.ts @@ -6612,9 +6612,17 @@ describe('Session', () => { ) .mockResolvedValueOnce(createEmptyStream()); - await session.prompt({ - sessionId: 'test-session-id', - prompt: [{ type: 'text', text: 'ask me before continuing' }], + await expect( + session.prompt({ + sessionId: 'test-session-id', + prompt: [{ type: 'text', text: 'ask me before continuing' }], + }), + ).rejects.toMatchObject({ + data: { + code: 'LOOP_DETECTED', + errorKind: 'loop_detected', + loopType: core.LoopType.INVALID_TOOL_PARAMS_STAGNATION, + }, }); expect(build).toHaveBeenCalledTimes(3); From 0fe2ef9291a37f8facc2cb0de8a4fec6314ad40e Mon Sep 17 00:00:00 2001 From: qwen-code-dev-bot Date: Tue, 11 Aug 2026 02:35:30 +0000 Subject: [PATCH 03/10] fix(cli): reject loop-detected stops only for foreground ACP turns Stop-hook continuations are shared with cron and background-notification turns, which must keep their pre-loop-error graceful end-turn handling; only the foreground prompt chain now rejects a loop-detected stop. Also folds in review feedback: drop the unreachable repeated-failure branch from the continuation, extract the cancellation-precedence helper, defer and freshness-guard the bounded-refresh turn-error replay, skip the phantom forward-failed compensation for structured turn errors, key the structured code gate on structuredness, share the retryability predicate in Web Shell, and keep the turn_complete error signal alive for loop-detected turns. --- packages/acp-bridge/src/bridge.test.ts | 35 +++-- packages/acp-bridge/src/bridge.ts | 44 ++++-- .../acp-integration/session/Session.test.ts | 145 ++++++++++++++++++ .../src/acp-integration/session/Session.ts | 69 +++++---- packages/web-shell/client/App.test.tsx | 37 +++++ packages/web-shell/client/App.tsx | 13 +- .../client/adapters/transcriptToMessages.ts | 11 +- 7 files changed, 295 insertions(+), 59 deletions(-) diff --git a/packages/acp-bridge/src/bridge.test.ts b/packages/acp-bridge/src/bridge.test.ts index b6f2a877723..1c3a1c34f93 100644 --- a/packages/acp-bridge/src/bridge.test.ts +++ b/packages/acp-bridge/src/bridge.test.ts @@ -4100,7 +4100,17 @@ describe('createAcpSessionBridge', () => { return { v: 1, sessionId: params['sessionId'], - events: [], + events: [ + { + v: 1, + type: 'session_update', + data: { + sessionUpdate: 'user_message_chunk', + content: { type: 'text', text: 'persisted turn content' }, + _meta: { 'qwen.session.recordId': 'record-loop-page' }, + }, + }, + ], hasMore: false, }; }, @@ -4128,17 +4138,20 @@ describe('createAcpSessionBridge', () => { historyPageSize: 100, }); - expect(refreshed.compactedReplay).toContainEqual( - expect.objectContaining({ - type: 'turn_error', - promptId: 'prompt-loop', - data: expect.objectContaining({ - code: 'LOOP_DETECTED', - errorKind: 'loop_detected', - loopType: 'turn_tool_call_cap', - }), + const compactedReplay = refreshed.compactedReplay ?? []; + expect(compactedReplay).toHaveLength(2); + expect(compactedReplay[0]).toMatchObject({ + type: 'session_update', + }); + expect(compactedReplay[compactedReplay.length - 1]).toMatchObject({ + type: 'turn_error', + promptId: 'prompt-loop', + data: expect.objectContaining({ + code: 'LOOP_DETECTED', + errorKind: 'loop_detected', + loopType: 'turn_tool_call_cap', }), - ); + }); await bridge.shutdown(); }); diff --git a/packages/acp-bridge/src/bridge.ts b/packages/acp-bridge/src/bridge.ts index 3b27d9066f0..9f423b4a471 100644 --- a/packages/acp-bridge/src/bridge.ts +++ b/packages/acp-bridge/src/bridge.ts @@ -1222,7 +1222,7 @@ function broadcastTurnError( const structuredErrorKind = extractJsonRpcErrorField(err, 'errorKind'); const errorKind = structuredErrorKind ?? classifyTurnErrorKind(message); const code = - structuredErrorKind === 'loop_detected' + structuredErrorKind !== undefined ? (extractJsonRpcErrorField(err, 'code') ?? extractErrorCode(err)) : extractErrorCode(err); const loopType = extractJsonRpcErrorField(err, 'loopType'); @@ -4937,19 +4937,27 @@ export function createAcpSessionBridge(opts: BridgeOptions): AcpSessionBridge { !entry.promptActive && entry.events.lastEventId === lastEventId ) { - const replay = entry.events.snapshotReplay(); - const turnError = entry.turnError - ? [ - ...(replay?.compactedTurns ?? []), - ...(replay?.liveJournal ?? []), - ] - .reverse() - .find((event) => event.type === 'turn_error') - : undefined; + let compactedReplay = page.events; + if (entry.turnError) { + const replay = entry.events.snapshotReplay(); + const journal = [ + ...(replay?.compactedTurns ?? []), + ...(replay?.liveJournal ?? []), + ]; + const turnError = [...journal] + .reverse() + .find((event) => event.type === 'turn_error'); + // Append only while the in-memory terminal is still the newest + // journaled event: automatic turns (cron/background + // notification) run without clearing entry.turnError, and + // re-appending the stale error after their newer content would + // misplace it in the refreshed transcript. + if (turnError && journal.at(-1)?.id === turnError.id) { + compactedReplay = [...page.events, turnError]; + } + } return { - compactedReplay: turnError - ? [...page.events, turnError] - : page.events, + compactedReplay, liveJournal: [], lastEventId, ...(page.partial === true ? { partial: true as const } : {}), @@ -6886,6 +6894,16 @@ export function createAcpSessionBridge(opts: BridgeOptions): AcpSessionBridge { ); return; } + if (extractJsonRpcErrorField(err, 'errorKind')) { + // Structured turn error (e.g. loop_detected): the + // forward succeeded and the daemon rejected the turn + // after running it. The formal turn_error terminal + // already ends the turn visibly; a phantom + // forward-failure line and prompt_cancelled broadcast + // would misreport it. + cancelPendingForSession(sessionId); + return; + } writeStderrLine( `sendPrompt: forward failed for session ${sessionId}: ${extractErrorMessage(err)}`, ); diff --git a/packages/cli/src/acp-integration/session/Session.test.ts b/packages/cli/src/acp-integration/session/Session.test.ts index 112fe67f5ee..8c49a509af9 100644 --- a/packages/cli/src/acp-integration/session/Session.test.ts +++ b/packages/cli/src/acp-integration/session/Session.test.ts @@ -8385,6 +8385,49 @@ describe('Session', () => { restoreGuardMode(); } }); + + it('returns cancelled when cancellation races the repeated-failure stop', async () => { + recreateSessionWithGuardMode('enforce'); + try { + const execute = installFailingTool(); + let enterRewriterWait!: () => void; + const rewriterWaitStarted = new Promise((resolve) => { + enterRewriterWait = resolve; + }); + let releaseRewriterWait!: () => void; + const rewriterWaitGate = new Promise((resolve) => { + releaseRewriterWait = resolve; + }); + session.messageRewriter = { + interceptUpdate: vi.fn().mockResolvedValue(undefined), + waitForPendingRewrites: vi.fn(async () => { + enterRewriterWait(); + await rewriterWaitGate; + }), + } as unknown as NonNullable; + queueMatchingFailureStreak(); + + const prompt = session.prompt({ + sessionId: 'test-session-id', + prompt: [{ type: 'text', text: 'run the failing tool' }], + }); + await rewriterWaitStarted; + await session.cancelPendingPrompt(); + releaseRewriterWait(); + + await expect(prompt).resolves.toEqual({ stopReason: 'cancelled' }); + expect(execute).toHaveBeenCalledTimes(9); + expect(logLoopDetectedSpy).toHaveBeenCalledWith( + mockConfig, + expect.objectContaining({ + loop_type: core.LoopType.REPEATED_TOOL_EXECUTION_FAILURE, + }), + { recordToQwenLogger: false }, + ); + } finally { + restoreGuardMode(); + } + }); }); describe('shell heartbeat forwarding', () => { @@ -29507,6 +29550,108 @@ describe('Session', () => { ).toBe(false); }); + it('keeps a cron turn graceful when its Stop continuation trips loop protection', async () => { + let fireCron!: (job: { + prompt: string; + cronExpr: string; + missed?: boolean; + }) => void; + const scheduler = { + hasPendingWork: true, + enableDurable: vi.fn().mockResolvedValue(undefined), + start: vi.fn( + ( + callback: (job: { + prompt: string; + cronExpr: string; + missed?: boolean; + }) => void, + ) => { + fireCron = callback; + }, + ), + stop: vi.fn(), + list: vi.fn().mockReturnValue([]), + getExitSummary: vi.fn().mockReturnValue(undefined), + }; + mockConfig.isCronEnabled = vi.fn().mockReturnValue(true); + mockConfig.getCronScheduler = vi.fn().mockReturnValue(scheduler); + rebuildSessionWithGuard(); + installPendingTodoTool(); + mockChat.sendMessageStream = vi + .fn() + .mockResolvedValueOnce(createEmptyStream()) + .mockResolvedValueOnce( + createStreamWithChunks([ + { + type: core.StreamEventType.CHUNK, + value: { + functionCalls: [ + { + id: 'cron-todo', + name: core.ToolNames.TODO_WRITE, + args: { todos: pendingTodos }, + }, + ], + }, + }, + ]), + ) + .mockResolvedValueOnce(createEmptyStream()) + .mockResolvedValueOnce( + createStreamWithChunks([ + { + type: core.StreamEventType.CHUNK, + value: { + functionCalls: [ + { + id: 'cron-loop-1', + name: 'read_file', + args: { path: 'a' }, + }, + { + id: 'cron-loop-2', + name: 'read_file', + args: { path: 'b' }, + }, + ], + }, + }, + ]), + ) + .mockResolvedValue(createEmptyStream()); + + await runGuardPrompt(); + // Explicit one-call cap: the cron turn's Stop-continuation batch of + // two calls trips the per-turn cap inside #runStopContinuation, the + // shared path cron and background-notification turns reach through + // #handleStopHookLoop. + mockConfig.getMaxToolCallsPerTurn = vi.fn().mockReturnValue(1); + mockConfig.isMaxToolCallsPerTurnExplicit = vi.fn().mockReturnValue(true); + fireCron({ prompt: 'scheduled work', cronExpr: '* * * * *' }); + + await vi.waitFor(() => { + expect(mockChat.sendMessageStream).toHaveBeenCalledTimes(4); + }); + expect(logLoopDetectedSpy).toHaveBeenCalledWith( + mockConfig, + expect.objectContaining({ + loop_type: core.LoopType.TURN_TOOL_CALL_CAP, + }), + {}, + ); + expect( + vi.mocked(mockClient.sessionUpdate).mock.calls.some(([params]) => { + const update = params.update; + return ( + update.sessionUpdate === 'agent_message_chunk' && + update.content.type === 'text' && + update.content.text.includes('[cron error]') + ); + }), + ).toBe(false); + }); + it('suspends an armed guard when a cron stream aborts', async () => { const scheduler = { hasPendingWork: true, diff --git a/packages/cli/src/acp-integration/session/Session.ts b/packages/cli/src/acp-integration/session/Session.ts index 604d4f3ebdd..7c27f399dad 100644 --- a/packages/cli/src/acp-integration/session/Session.ts +++ b/packages/cli/src/acp-integration/session/Session.ts @@ -635,6 +635,15 @@ function createLoopDetectedTurnError( }); } +// Cancellation takes precedence when it races a loop-detected stop. +function cancelledOrThrowLoopDetected( + signal: AbortSignal, + loopState: DaemonToolLoopState, +): 'cancelled' { + if (signal.aborted) return 'cancelled'; + throw createLoopDetectedTurnError(loopState); +} + function recordDaemonToolCalls( config: Config, promptId: string, @@ -3934,10 +3943,12 @@ export class Session implements SessionContext { ); nextMessage = nextAfterTools.message; if (nextAfterTools.stoppedByRepeatedToolFailure) { - if (pendingSend.signal.aborted) { - return { stopReason: 'cancelled' }; - } - throw createLoopDetectedTurnError(toolLoopState); + return { + stopReason: cancelledOrThrowLoopDetected( + pendingSend.signal, + toolLoopState, + ), + }; } if (toolRun.loopDetected) { this.todoStopGuard.suspend(); @@ -3945,10 +3956,12 @@ export class Session implements SessionContext { toolRun, pendingSend.signal, ); - if (pendingSend.signal.aborted) { - return { stopReason: 'cancelled' }; - } - throw createLoopDetectedTurnError(toolLoopState); + return { + stopReason: cancelledOrThrowLoopDetected( + pendingSend.signal, + toolLoopState, + ), + }; } } } @@ -3968,6 +3981,7 @@ export class Session implements SessionContext { true, fullTurnModelOverride, channelDeliveryCapture, + true, // rejectOnLoopDetected ); } finally { logConversationFinishedEvent( @@ -4007,6 +4021,7 @@ export class Session implements SessionContext { allowExternalHooks = true, modelOverride?: string, channelDeliveryCapture?: ChannelDeliveryCapture, + rejectOnLoopDetected = false, ): Promise<{ stopReason: PromptResponse['stopReason'] }> { const stopHookBlockingCap = this.config.getStopHookBlockingCap(); let stopHookIterationCount = 0; @@ -4072,6 +4087,7 @@ export class Session implements SessionContext { onFullTurnModel, getModelOverride: () => modelOverride, channelDeliveryCapture, + rejectOnLoopDetected, }, ); if (continuation.kind === 'terminal') { @@ -4171,6 +4187,7 @@ export class Session implements SessionContext { onFullTurnModel, getModelOverride: () => modelOverride, channelDeliveryCapture, + rejectOnLoopDetected, }, ); if (continuation.kind === 'terminal') { @@ -4296,6 +4313,7 @@ export class Session implements SessionContext { onFullTurnModel, getModelOverride: () => modelOverride, channelDeliveryCapture, + rejectOnLoopDetected, }, ); if (continuation.supersededAutomaticContinuation && externalReason) { @@ -4321,6 +4339,7 @@ export class Session implements SessionContext { onFullTurnModel?: (model: string) => boolean; getModelOverride?: () => string | undefined; channelDeliveryCapture?: ChannelDeliveryCapture; + rejectOnLoopDetected?: boolean; } = {}, ): Promise { let nextMessage: Content | null = { role: 'user', parts }; @@ -4879,16 +4898,18 @@ export class Session implements SessionContext { if (toolRun.loopDetected) { this.todoStopGuard.suspend(); await this.#preserveStoppedToolRun(toolRun, pendingSend.signal); - if (pendingSend.signal.aborted) { - return { - kind: 'terminal', - stopReason: 'cancelled', - ...(supersededAutomaticContinuation - ? { supersededAutomaticContinuation: true } - : {}), - }; - } - throw createLoopDetectedTurnError(toolLoopState); + return { + kind: 'terminal', + // Only the foreground chain rejects a loop-detected stop; cron + // and background-notification turns keep the graceful end-turn + // handling they had before loop stops became rejections. + stopReason: options.rejectOnLoopDetected + ? cancelledOrThrowLoopDetected(pendingSend.signal, toolLoopState) + : getAbortAwareEndTurnStopReason(pendingSend.signal), + ...(supersededAutomaticContinuation + ? { supersededAutomaticContinuation: true } + : {}), + }; } const nextAfterTools = await this.#buildNextMessageAfterToolRun( toolRun, @@ -4898,18 +4919,6 @@ export class Session implements SessionContext { options.onFullTurnModel, ); nextMessage = nextAfterTools.message; - if (nextAfterTools.stoppedByRepeatedToolFailure) { - if (pendingSend.signal.aborted) { - return { - kind: 'terminal', - stopReason: 'cancelled', - ...(supersededAutomaticContinuation - ? { supersededAutomaticContinuation: true } - : {}), - }; - } - throw createLoopDetectedTurnError(toolLoopState); - } if (nextAfterTools.hadMidTurnUserInput) { nextGuardContinuation = undefined; continue; diff --git a/packages/web-shell/client/App.test.tsx b/packages/web-shell/client/App.test.tsx index 7db6648ad98..13a4931c14f 100644 --- a/packages/web-shell/client/App.test.tsx +++ b/packages/web-shell/client/App.test.tsx @@ -10833,6 +10833,43 @@ describe('App session callbacks', () => { expect(testState.latestChatEditorProps?.disabled).toBe(false); }); + it('still reports a loop-detected turn error through turn_complete', async () => { + const onSessionChange = vi.fn(); + const { container, rerender } = renderApp({ onSessionChange }); + await flush(); + + testState.prompt = 'repeat this'; + await clickSubmit(container); + onSessionChange.mockClear(); + + act(() => { + testState.streamingState = 'responding'; + rerender({ onSessionChange }); + }); + act(() => { + testState.blocks = [ + { + kind: 'error', + source: 'turn_error', + id: 'turn-error-loop', + errorKind: 'loop_detected', + text: 'internal fallback', + }, + ]; + testState.streamingState = 'idle'; + rerender({ onSessionChange }); + }); + + expect(onSessionChange).toHaveBeenCalledWith({ + type: 'turn_complete', + sessionId: 'session-1', + error: expect.objectContaining({ + message: 'Turn error (block turn-error-loop)', + }), + }); + expect(container.querySelector('[data-testid="retry"]')).toBeNull(); + }); + it('locks an image retry when its admission response is lost', async () => { const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}); const retrySend = deferred(); diff --git a/packages/web-shell/client/App.tsx b/packages/web-shell/client/App.tsx index 7e0150f55d7..759aedb16ef 100644 --- a/packages/web-shell/client/App.tsx +++ b/packages/web-shell/client/App.tsx @@ -53,6 +53,7 @@ import { WEB_SHELL_SIDE_TASK_SOURCE_TYPE, } from './constants/sessions'; import { extractPendingPermission } from './adapters/transcriptAdapter'; +import { isRetryableTurnErrorKind } from './adapters/transcriptToMessages'; import { MessageList, type MessageListHandle } from './components/MessageList'; import { SubagentDetailsProvider } from './subagentDetailsContext'; import { MonitorDetailsProvider } from './monitorDetailsContext'; @@ -3708,6 +3709,7 @@ export function App({ composerSourceVersionRef.current, ); const retryableTurnErrorIdRef = useRef(null); + const lastTurnErrorIdRef = useRef(null); const retriedTurnErrorIdRef = useRef(null); const [showRetryHint, setShowRetryHint] = useState(false); const showRetryHintRef = useRef(showRetryHint); @@ -6048,18 +6050,21 @@ export function App({ }, [streamingState, sessionActions, reportError, pushToast, t]); useEffect(() => { + let turnErrorId: string | null = null; let retryableTurnErrorId: string | null = null; for (let i = blocks.length - 1; i >= 0; i--) { const block = blocks[i]; if (block?.kind === 'user') break; if (block?.kind === 'error' && block.source === 'turn_error') { - if (block.errorKind !== 'loop_detected') { + turnErrorId = block.id; + if (isRetryableTurnErrorKind(block.errorKind)) { retryableTurnErrorId = block.id; } break; } if (block?.kind !== 'debug') break; } + lastTurnErrorIdRef.current = turnErrorId; const canRetry = connected && retryableTurnErrorId !== null && @@ -6076,7 +6081,7 @@ export function App({ onStreamingStateChange?.(streamingState); }, [streamingState, onStreamingStateChange]); - // Reads retryableTurnErrorIdRef which is set by the blocks effect above. + // Reads lastTurnErrorIdRef which is set by the blocks effect above. // Declaration order matters: this effect must run after the blocks effect // so that within the same render, the ref is already updated before we read it. const prevStreamingForTurnCompleteRef = useRef(streamingState); @@ -6094,8 +6099,8 @@ export function App({ // a spurious turn_complete for the new session. if (!sessionId || sessionId !== streamingSessionIdRef.current) return; const turnError = - retryableTurnErrorIdRef.current != null - ? new Error(`Turn error (block ${retryableTurnErrorIdRef.current})`) + lastTurnErrorIdRef.current != null + ? new Error(`Turn error (block ${lastTurnErrorIdRef.current})`) : undefined; dispatchSessionChangeRef.current?.({ type: 'turn_complete', diff --git a/packages/web-shell/client/adapters/transcriptToMessages.ts b/packages/web-shell/client/adapters/transcriptToMessages.ts index 26e09bd4db2..9b522092ef1 100644 --- a/packages/web-shell/client/adapters/transcriptToMessages.ts +++ b/packages/web-shell/client/adapters/transcriptToMessages.ts @@ -192,6 +192,14 @@ function isUnrecognizedDaemonDebug( ); } +// Resubmitting a prompt the daemon stopped for loop protection tends to +// re-loop, so no retry affordance is offered for these turn errors. +export function isRetryableTurnErrorKind( + errorKind: string | undefined, +): boolean { + return errorKind !== 'loop_detected'; +} + function getErrorDisplayText( block: DaemonStatusTranscriptBlock, labels?: TranscriptMessageLabels, @@ -780,7 +788,8 @@ export function transcriptBlocksToDaemonMessages( content: getErrorDisplayText(errorBlock, options.labels), variant: 'error', retryable: - errorBlock.source === 'turn_error' && errorKind !== 'loop_detected', + errorBlock.source === 'turn_error' && + isRetryableTurnErrorKind(errorKind), timestamp: blockTime, ...(errorBlock.source ? { source: errorBlock.source } : {}), ...getErrorMessageData(errorBlock.data, errorKind), From 1d32541bb8e729c5dfb5a96f2e8843fac01e9f9b Mon Sep 17 00:00:00 2001 From: qwen-code-dev-bot Date: Tue, 11 Aug 2026 06:58:04 +0000 Subject: [PATCH 04/10] fix(acp-bridge): harden loop-detected turn errors per review feedback Address round-3 review findings: - The bounded-refresh append guard no longer compares the in-memory turn_error against the last journaled event of any kind. The published event is stored on the entry at broadcast time; any newer turn terminal clears it, and only turn-content events journaled after it block the append (queue/config bookkeeping no longer hides the error on refresh). The staleness check reads the in-flight journal via a new liveJournalSnapshot() accessor instead of flattening the replay window. - Channel turns keep the graceful end-turn handling like cron and background-notification turns, so their collected text is still delivered when loop protection stops them. - Loop-detected rejections drain the cron/notification queues in prompt()'s finally, preserving the pre-rejection drain invariant for queued automatic work. - Tests pin each behavior: queued-bookkeeping refresh append, stale-error no-append, prompt_cancelled absence, foreground stop-hook rejection, background-notification graceful default, and the user-facing message constant. --- .../web-shell-loop-detection-turn-error.md | 2 +- packages/acp-bridge/src/bridge.test.ts | 219 +++++++++++++ packages/acp-bridge/src/bridge.ts | 70 ++++- packages/acp-bridge/src/compactionEngine.ts | 20 +- packages/acp-bridge/src/eventBus.ts | 16 + .../acp-integration/session/Session.test.ts | 288 ++++++++++++++++++ .../src/acp-integration/session/Session.ts | 47 ++- 7 files changed, 630 insertions(+), 32 deletions(-) diff --git a/docs/design/web-shell-loop-detection-turn-error.md b/docs/design/web-shell-loop-detection-turn-error.md index 23269794dc3..86ac2446c56 100644 --- a/docs/design/web-shell-loop-detection-turn-error.md +++ b/docs/design/web-shell-loop-detection-turn-error.md @@ -12,7 +12,7 @@ Web Shell renders `loop_detected` from the structured kind, using localized plai Skipped tools keep their existing failed terminal update and error details so they cannot remain pending and their display behavior does not change. The additional `turn_error` provides the user-facing explanation for the stopped turn. -The session remains alive and the per-turn loop state is recreated for the next prompt. Cron and background-notification work keep their existing non-interactive handling. +The session remains alive and the per-turn loop state is recreated for the next prompt. Cron, background-notification, and channel-delivery turns keep their existing non-interactive handling: only interactive foreground prompts reject. A loop-detected rejection still drains the cron/notification queues, preserving the invariant that a loop-stopped turn never strands queued automatic work. When Web Shell reloads a live session from paginated persisted history, the bridge appends the current in-memory `turn_error` to that replay. This keeps the terminal error visible across a page refresh without changing historical persistence. diff --git a/packages/acp-bridge/src/bridge.test.ts b/packages/acp-bridge/src/bridge.test.ts index 1c3a1c34f93..f827a3b5f86 100644 --- a/packages/acp-bridge/src/bridge.test.ts +++ b/packages/acp-bridge/src/bridge.test.ts @@ -4156,6 +4156,220 @@ describe('createAcpSessionBridge', () => { await bridge.shutdown(); }); + it('keeps the turn error on refresh when queue bookkeeping lands after it', async () => { + // A queued-then-promoted prompt that trips loop protection publishes + // `pending_prompt_completed` AFTER its `turn_error` terminal (the + // queue-view bookkeeping from `result.finally`). That bookkeeping must + // not defeat the refresh-append of the terminal error. + let releaseFirst!: () => void; + const firstPrompt = new Promise<{ stopReason: 'end_turn' }>((resolve) => { + releaseFirst = () => resolve({ stopReason: 'end_turn' }); + }); + let promptCalls = 0; + const handle = makeChannel({ + promptImpl: () => { + promptCalls += 1; + if (promptCalls === 1) return firstPrompt; + throw new RequestError(-32603, 'Loop protection stopped this turn', { + code: 'LOOP_DETECTED', + errorKind: 'loop_detected', + loopType: 'turn_tool_call_cap', + }); + }, + extMethodImpl: (method, params) => { + if (method !== SERVE_STATUS_EXT_METHODS.sessionTranscript) { + throw new Error(`unexpected extMethod ${method}`); + } + return { + v: 1, + sessionId: params['sessionId'], + events: [ + { + v: 1, + type: 'session_update', + data: { + sessionUpdate: 'user_message_chunk', + content: { type: 'text', text: 'persisted turn content' }, + _meta: { 'qwen.session.recordId': 'record-loop-queued-page' }, + }, + }, + ], + hasMore: false, + }; + }, + }); + const bridge = makeBridge({ channelFactory: async () => handle.channel }); + const session = await bridge.spawnOrAttach({ workspaceCwd: WS_A }); + const abort = new AbortController(); + const iter = bridge.subscribeEvents(session.sessionId, { + signal: abort.signal, + }); + const seenTypes: string[] = []; + const collectUntilQueueBookkeeping = (async () => { + for await (const event of iter) { + seenTypes.push(event.type); + if ( + event.type === 'pending_prompt_completed' && + event.promptId === 'prompt-loop' + ) { + return; + } + } + throw new Error('pending_prompt_completed was not published'); + })(); + + const first = bridge.sendPrompt( + session.sessionId, + { + sessionId: session.sessionId, + prompt: [{ type: 'text', text: 'first' }], + }, + undefined, + { promptId: 'prompt-first' }, + ); + await vi.waitFor(() => { + expect(handle.agent.promptCalls).toHaveLength(1); + }); + + const second = bridge.sendPrompt( + session.sessionId, + { + sessionId: session.sessionId, + prompt: [{ type: 'text', text: 'loop' }], + }, + undefined, + { promptId: 'prompt-loop' }, + ); + await vi.waitFor(() => { + expect(bridge.getPendingPrompts(session.sessionId)).toHaveLength(2); + }); + + releaseFirst(); + await expect(first).resolves.toEqual({ stopReason: 'end_turn' }); + await expect(second).rejects.toThrow('Loop protection stopped this turn'); + await collectUntilQueueBookkeeping; + // The bookkeeping event lands AFTER the terminal — exactly the ordering + // that must not hide the error on refresh. + expect(seenTypes.indexOf('turn_error')).toBeGreaterThanOrEqual(0); + expect(seenTypes.indexOf('turn_error')).toBeLessThan( + seenTypes.lastIndexOf('pending_prompt_completed'), + ); + + const refreshed = await bridge.loadSession({ + sessionId: session.sessionId, + workspaceCwd: WS_A, + clientId: session.clientId, + historyReplay: 'response', + historyPageSize: 100, + }); + + const compactedReplay = refreshed.compactedReplay ?? []; + expect(compactedReplay).toHaveLength(2); + expect(compactedReplay[compactedReplay.length - 1]).toMatchObject({ + type: 'turn_error', + promptId: 'prompt-loop', + data: expect.objectContaining({ + code: 'LOOP_DETECTED', + errorKind: 'loop_detected', + loopType: 'turn_tool_call_cap', + }), + }); + + abort.abort(); + await bridge.shutdown(); + }); + + it('drops the stale turn error on refresh after newer automatic-turn content', async () => { + const handle = makeChannel({ + promptImpl: () => { + throw new RequestError(-32603, 'Loop protection stopped this turn', { + code: 'LOOP_DETECTED', + errorKind: 'loop_detected', + loopType: 'turn_tool_call_cap', + }); + }, + extMethodImpl: (method, params) => { + if (method !== SERVE_STATUS_EXT_METHODS.sessionTranscript) { + throw new Error(`unexpected extMethod ${method}`); + } + return { + v: 1, + sessionId: params['sessionId'], + events: [ + { + v: 1, + type: 'session_update', + data: { + sessionUpdate: 'user_message_chunk', + content: { type: 'text', text: 'persisted turn content' }, + _meta: { 'qwen.session.recordId': 'record-loop-stale-page' }, + }, + }, + ], + hasMore: false, + }; + }, + }); + const bridge = makeBridge({ channelFactory: async () => handle.channel }); + const session = await bridge.spawnOrAttach({ workspaceCwd: WS_A }); + + await expect( + bridge.sendPrompt( + session.sessionId, + { + sessionId: session.sessionId, + prompt: [{ type: 'text', text: 'loop' }], + }, + undefined, + { promptId: 'prompt-loop' }, + ), + ).rejects.toThrow('Loop protection stopped this turn'); + + // An automatic turn (cron/background notification) runs after the loop + // error without an interactive dispatch: its content is journaled via + // the ordinary session/update fan-in. + const abort = new AbortController(); + const iter = bridge.subscribeEvents(session.sessionId, { + signal: abort.signal, + }); + const sawAutomaticContent = (async () => { + for await (const event of iter) { + if ( + event.type === 'session_update' && + JSON.stringify(event.data).includes('automatic turn content') + ) { + return; + } + } + throw new Error('automatic turn content was not published'); + })(); + await handle.agentConnection.sessionUpdate({ + sessionId: session.sessionId, + update: { + sessionUpdate: 'agent_message_chunk', + content: { type: 'text', text: 'automatic turn content' }, + }, + }); + await sawAutomaticContent; + + const refreshed = await bridge.loadSession({ + sessionId: session.sessionId, + workspaceCwd: WS_A, + clientId: session.clientId, + historyReplay: 'response', + historyPageSize: 100, + }); + + const compactedReplay = refreshed.compactedReplay ?? []; + expect(compactedReplay).toHaveLength(1); + expect(compactedReplay.some((event) => event.type === 'turn_error')).toBe( + false, + ); + + abort.abort(); + await bridge.shutdown(); + }); + it('propagates partial and replayError from a bounded refresh', async () => { const handle = makeChannel({ loadSessionImpl: () => ({ @@ -8485,8 +8699,10 @@ describe('createAcpSessionBridge', () => { const iter = bridge.subscribeEvents(session.sessionId, { signal: abort.signal, }); + const emittedTypes: string[] = []; const turnError = (async () => { for await (const event of iter) { + emittedTypes.push(event.type); if (event.type === 'turn_error') return event; } throw new Error('turn_error was not published'); @@ -8514,6 +8730,9 @@ describe('createAcpSessionBridge', () => { promptId: 'prompt-loop', }, }); + // Structured rejections already ran on the daemon: the forward-failure + // phantom (`prompt_cancelled{forward_failed}`) must be suppressed. + expect(emittedTypes).not.toContain('prompt_cancelled'); expect(bridge.getSessionSummary(session.sessionId).turnError).toEqual({ message: 'Loop protection stopped this turn', code: 'LOOP_DETECTED', diff --git a/packages/acp-bridge/src/bridge.ts b/packages/acp-bridge/src/bridge.ts index 9f423b4a471..dfa17e9daba 100644 --- a/packages/acp-bridge/src/bridge.ts +++ b/packages/acp-bridge/src/bridge.ts @@ -702,6 +702,14 @@ interface SessionEntry { code?: string; errorKind?: string; }; + /** + * The journaled `turn_error` event behind `turnError`, when the failed + * turn published one. A bounded refresh replays it onto persisted + * history so the terminal survives a page refresh; any newer turn + * terminal clears it so a stale error is never re-appended. Not part of + * the session summary. + */ + turnErrorEvent?: BridgeEvent; retryAllowed: boolean; /** Prompt id whose `prompt_cancelled` event has already been broadcast. */ cancelBroadcastPromptId?: string; @@ -1131,7 +1139,7 @@ function broadcastTurnComplete( originatorClientId: string | undefined, ): void { try { - entry.events.publish({ + const published = entry.events.publish({ type: 'turn_complete', ...(promptId ? { promptId } : {}), data: { @@ -1141,6 +1149,8 @@ function broadcastTurnComplete( }, ...(originatorClientId ? { originatorClientId } : {}), }); + // A newer turn terminal supersedes any pending refresh-append error. + if (published !== undefined) entry.turnErrorEvent = undefined; } catch { /* bus may be closed during session teardown */ } @@ -1202,6 +1212,21 @@ export function extractErrorCode(err: unknown): string | undefined { return undefined; } +/** + * Event types that may be published after a turn terminal without adding + * turn content (prompt-queue bookkeeping and config changes). The bounded + * refresh-append guard skips these when deciding whether the in-memory + * `turn_error` is still the newest meaningful terminal; any other event + * type blocks the append. + */ +const REFRESH_APPEND_BOOKKEEPING_EVENT_TYPES = new Set([ + 'pending_prompt_added', + 'pending_prompt_completed', + 'prompt_cancelled', + 'model_switched', + 'approval_mode_changed', +]); + export function classifyTurnErrorKind( message: string, ): 'model_stream_interrupted' | undefined { @@ -1250,7 +1275,7 @@ function broadcastTurnError( }; } try { - entry.events.publish({ + const published = entry.events.publish({ type: 'turn_error', ...(promptId ? { promptId } : {}), data: { @@ -1263,6 +1288,15 @@ function broadcastTurnError( }, ...(originatorClientId ? { originatorClientId } : {}), }); + if (mutateTurnState) { + // Undefined when the bus dropped the publish (closed mid-teardown); + // the refresh-append guard then simply has nothing to replay. + entry.turnErrorEvent = published; + } else if (published !== undefined) { + // A queued prompt's terminal is a newer turn boundary: the prior + // in-memory error must no longer be replayed on refresh. + entry.turnErrorEvent = undefined; + } } catch { /* bus may be closed during session teardown */ } @@ -4938,22 +4972,25 @@ export function createAcpSessionBridge(opts: BridgeOptions): AcpSessionBridge { entry.events.lastEventId === lastEventId ) { let compactedReplay = page.events; - if (entry.turnError) { - const replay = entry.events.snapshotReplay(); - const journal = [ - ...(replay?.compactedTurns ?? []), - ...(replay?.liveJournal ?? []), - ]; - const turnError = [...journal] - .reverse() - .find((event) => event.type === 'turn_error'); - // Append only while the in-memory terminal is still the newest - // journaled event: automatic turns (cron/background + const turnErrorEvent = entry.turnErrorEvent; + if (turnErrorEvent) { + // Append only while no newer turn content was journaled after + // the in-memory terminal: automatic turns (cron/background // notification) run without clearing entry.turnError, and // re-appending the stale error after their newer content would - // misplace it in the refreshed transcript. - if (turnError && journal.at(-1)?.id === turnError.id) { - compactedReplay = [...page.events, turnError]; + // misplace it in the refreshed transcript. Bookkeeping events + // carry no turn content and must not defeat the append; a + // newer turn terminal clears turnErrorEvent at broadcast. The + // journal holds exactly the events published since the last + // turn boundary (the terminal itself folds into the replay + // window), so no history scan is needed. + const journal = entry.events.liveJournalSnapshot() ?? []; + const hasNewerTurnContent = journal.some( + (event) => + !REFRESH_APPEND_BOOKKEEPING_EVENT_TYPES.has(event.type), + ); + if (!hasNewerTurnContent) { + compactedReplay = [...page.events, turnErrorEvent]; } } return { @@ -6784,6 +6821,7 @@ export function createAcpSessionBridge(opts: BridgeOptions): AcpSessionBridge { entry.activePromptId = pendingEntry.promptId; delete entry.cancelBroadcastWithoutPrompt; delete entry.turnError; + delete entry.turnErrorEvent; activePromptCounter++; entry.sessionLastSeenAt = Date.now(); touchActivity(); diff --git a/packages/acp-bridge/src/compactionEngine.ts b/packages/acp-bridge/src/compactionEngine.ts index 857ca001c6e..d921f97091f 100644 --- a/packages/acp-bridge/src/compactionEngine.ts +++ b/packages/acp-bridge/src/compactionEngine.ts @@ -260,6 +260,20 @@ export class TurnBoundaryCompactionEngine implements CompactionEngine { this.makeHistoryTruncatedEvent(compactedTurns.length), ); } + return { + compactedTurns, + liveJournal: this.liveJournalSnapshot(), + lastEventId: this.lastEventId, + }; + } + + /** + * Snapshot of only the in-flight live journal — the events ingested + * since the last turn boundary (a boundary folds its turn into the + * replay window and resets the journal). Cheaper than `snapshot()`: + * no replay-window flatten. + */ + liveJournalSnapshot(): BridgeEvent[] { const liveJournal = this.liveJournal.map((entry) => isLiveJournalTextSegment(entry) ? mergeLiveJournalTextEvent( @@ -294,11 +308,7 @@ export class TurnBoundaryCompactionEngine implements CompactionEngine { }, }); } - return { - compactedTurns, - liveJournal, - lastEventId: this.lastEventId, - }; + return liveJournal; } seed(snapshot: { compactedTurns: BridgeEvent[]; lastEventId: number }): void { diff --git a/packages/acp-bridge/src/eventBus.ts b/packages/acp-bridge/src/eventBus.ts index 68d5233a92c..c83f305243e 100644 --- a/packages/acp-bridge/src/eventBus.ts +++ b/packages/acp-bridge/src/eventBus.ts @@ -44,6 +44,12 @@ export interface CompactionEngine { ingest(event: BridgeEvent, byteLength?: number): void; seedReplayEvents(events: BridgeEvent[]): void; snapshot(): SessionReplaySnapshot; + /** + * In-flight journal only — events ingested since the last turn + * boundary — without flattening the compacted replay window. Optional: + * consumers fall back to `snapshot()` semantics when absent. + */ + liveJournalSnapshot?(): BridgeEvent[]; close(): void; } @@ -396,6 +402,16 @@ export class EventBus { return snapshot; } + /** + * Events ingested since the last turn boundary (the boundary itself is + * folded into the replay window), without flattening that window. + * Undefined when no compaction engine is wired or it exposes no journal + * snapshot. + */ + liveJournalSnapshot(): BridgeEvent[] | undefined { + return this.compactionEngine?.liveJournalSnapshot?.(); + } + private markCompactionDegraded(err: unknown): void { if (this.compactionDegraded) return; this.compactionDegraded = true; diff --git a/packages/cli/src/acp-integration/session/Session.test.ts b/packages/cli/src/acp-integration/session/Session.test.ts index 8c49a509af9..1ab9c63253c 100644 --- a/packages/cli/src/acp-integration/session/Session.test.ts +++ b/packages/cli/src/acp-integration/session/Session.test.ts @@ -15,6 +15,7 @@ import * as path from 'node:path'; import { computeInitialTurnFromHistory, fireSessionPermissionDeniedForAutoMode, + LOOP_DETECTED_TURN_ERROR_MESSAGE, resolveExistingFile, resolveHomeLoopResolverRoots, Session, @@ -6884,6 +6885,7 @@ describe('Session', () => { prompt: [{ type: 'text', text: 'read many files' }], }), ).rejects.toMatchObject({ + message: LOOP_DETECTED_TURN_ERROR_MESSAGE, data: { code: 'LOOP_DETECTED', errorKind: 'loop_detected', @@ -6951,6 +6953,78 @@ describe('Session', () => { expect(mockChat.sendMessageStream).toHaveBeenCalledTimes(2); }); + it('drains cron work queued mid-turn when the turn rejects on loop protection', async () => { + mockConfig.getApprovalMode = vi.fn().mockReturnValue(ApprovalMode.YOLO); + mockConfig.getMaxToolCallsPerTurn = vi.fn().mockReturnValue(1); + mockConfig.isMaxToolCallsPerTurnExplicit = vi + .fn() + .mockReturnValue(true); + let fireCron!: (job: { prompt: string; cronExpr: string }) => void; + const scheduler = { + hasPendingWork: true, + enableDurable: vi.fn().mockResolvedValue(undefined), + start: vi.fn( + (callback: (job: { prompt: string; cronExpr: string }) => void) => { + fireCron = callback; + }, + ), + stop: vi.fn(), + list: vi.fn().mockReturnValue([]), + getExitSummary: vi.fn().mockReturnValue(undefined), + }; + mockConfig.isCronEnabled = vi.fn().mockReturnValue(true); + mockConfig.getCronScheduler = vi.fn().mockReturnValue(scheduler); + session.startCronScheduler(); + await vi.waitFor(() => expect(scheduler.start).toHaveBeenCalled()); + + // Gate the model stream so the cron fires while the foreground turn + // is still active; the turn then trips the explicit one-call cap and + // rejects. Loop-detected turns resolved end_turn before they became + // rejections (and drained), so the rejection path must drain too. + let releaseStream!: () => void; + const streamGate = new Promise((resolve) => { + releaseStream = resolve; + }); + async function* gatedCapTripStream() { + await streamGate; + yield { + type: core.StreamEventType.CHUNK, + value: { + functionCalls: [ + { id: 'cap-1', name: 'read_file', args: { path: 'a' } }, + { id: 'cap-2', name: 'read_file', args: { path: 'b' } }, + ], + }, + }; + } + mockChat.sendMessageStream = vi + .fn() + .mockResolvedValueOnce(gatedCapTripStream()) + .mockResolvedValue(createEmptyStream()); + + const prompt = session.prompt({ + sessionId: 'test-session-id', + prompt: [{ type: 'text', text: 'foreground work' }], + }); + await vi.waitFor(() => { + expect(mockChat.sendMessageStream).toHaveBeenCalledTimes(1); + }); + + fireCron({ prompt: 'scheduled work', cronExpr: '* * * * *' }); + const internals = session as unknown as { cronQueue: unknown[] }; + expect(internals.cronQueue).toHaveLength(1); + + releaseStream(); + await expect(prompt).rejects.toMatchObject({ + data: expect.objectContaining({ code: 'LOOP_DETECTED' }), + }); + + await vi.waitFor(() => { + expect(mockChat.sendMessageStream).toHaveBeenCalledTimes(2); + }); + expect(internals.cronQueue).toHaveLength(0); + }); + it('lets a productive turn continue past the default cap (adaptive)', async () => { mockConfig.getApprovalMode = vi.fn().mockReturnValue(ApprovalMode.YOLO); // Default cap (100) without an explicit setting: a turn of diverse @@ -11129,6 +11203,72 @@ describe('Session', () => { }); }); + it('keeps a channel turn graceful when loop protection stops it', async () => { + // Channel turns are non-interactive deliveries: like cron and + // background-notification turns they keep the graceful end-turn so + // the collected response text is still delivered instead of the + // prompt rejecting. + mockConfig.getApprovalMode = vi.fn().mockReturnValue(ApprovalMode.YOLO); + mockConfig.getMaxToolCallsPerTurn = vi.fn().mockReturnValue(1); + mockConfig.isMaxToolCallsPerTurnExplicit = vi + .fn() + .mockReturnValue(true); + mockChat.sendMessageStream = vi.fn().mockResolvedValueOnce( + createStreamWithChunks([ + { + type: core.StreamEventType.CHUNK, + value: { + functionCalls: [ + { + id: 'channel-loop-1', + name: 'read_file', + args: { file_path: 'a.ts' }, + }, + { + id: 'channel-loop-2', + name: 'read_file', + args: { file_path: 'b.ts' }, + }, + ], + }, + }, + ]), + ); + + await expect( + session.prompt({ + sessionId: 'test-session-id', + prompt: [{ type: 'text', text: 'channel work' }], + _meta: { + 'qwen.daemon.channelDelivery': { + deliveryId: 'prompt-loop-channel', + target: { + channelName: 'dingtalk', + type: 'user', + id: 'user-1', + }, + }, + }, + }), + ).resolves.toEqual({ stopReason: 'end_turn' }); + + expect(logLoopDetectedSpy).toHaveBeenCalledWith( + mockConfig, + expect.objectContaining({ + loop_type: core.LoopType.TURN_TOOL_CALL_CAP, + }), + {}, + ); + await vi.waitFor(() => { + expect(mockClient.extMethod).toHaveBeenCalledWith( + 'qwen/control/channel-delivery', + expect.objectContaining({ + deliveryId: 'prompt-loop-channel', + }), + ); + }); + }); + it('replaces the prompt candidate with a Stop-hook continuation final', async () => { const messageBus = { request: vi @@ -24084,6 +24224,55 @@ describe('Session', () => { await expect(prompt).resolves.toEqual({ stopReason: 'cancelled' }); }); + it('rejects a foreground turn whose Stop continuation trips loop protection', async () => { + // Pins rejectOnLoopDetected=true at the foreground #handleStopHookLoop + // call site: without it this turn would resolve end_turn. + mockConfig.getMaxToolCallsPerTurn = vi.fn().mockReturnValue(1); + mockConfig.isMaxToolCallsPerTurnExplicit = vi.fn().mockReturnValue(true); + mockChat.sendMessageStream = vi + .fn() + .mockResolvedValueOnce(createEmptyStream()) + .mockResolvedValueOnce( + createStreamWithChunks([ + { + type: core.StreamEventType.CHUNK, + value: { + functionCalls: [ + { id: 'loop-1', name: 'read_file', args: { path: 'a' } }, + { id: 'loop-2', name: 'read_file', args: { path: 'b' } }, + ], + }, + }, + ]), + ); + const messageBus = { + request: vi.fn().mockResolvedValue({ + success: true, + output: { decision: 'block', reason: 'continue once' }, + }), + }; + mockConfig.getMessageBus = vi.fn().mockReturnValue(messageBus); + mockConfig.hasHooksForEvent = vi + .fn() + .mockImplementation((name: string) => name === 'Stop'); + mockClient.extMethod = vi.fn(async () => ({ messages: [] })); + + await expect(runGuardPrompt()).rejects.toMatchObject({ + message: LOOP_DETECTED_TURN_ERROR_MESSAGE, + data: expect.objectContaining({ + code: 'LOOP_DETECTED', + loopType: core.LoopType.TURN_TOOL_CALL_CAP, + }), + }); + expect(logLoopDetectedSpy).toHaveBeenCalledWith( + mockConfig, + expect.objectContaining({ + loop_type: core.LoopType.TURN_TOOL_CALL_CAP, + }), + {}, + ); + }); + it('runs exactly two continuations and emits replayable status', async () => { rebuildSessionWithGuard(); installPendingTodoTool(); @@ -29652,6 +29841,105 @@ describe('Session', () => { ).toBe(false); }); + it('keeps a background-notification turn graceful when its Stop continuation trips loop protection', async () => { + rebuildSessionWithGuard(); + installPendingTodoTool(); + mockChat.sendMessageStream = vi + .fn() + .mockResolvedValueOnce(createEmptyStream()) + .mockResolvedValueOnce( + createStreamWithChunks([ + { + type: core.StreamEventType.CHUNK, + value: { + functionCalls: [ + { + id: 'notification-todo', + name: core.ToolNames.TODO_WRITE, + args: { todos: pendingTodos }, + }, + ], + }, + }, + ]), + ) + .mockResolvedValueOnce(createEmptyStream()) + .mockResolvedValueOnce( + createStreamWithChunks([ + { + type: core.StreamEventType.CHUNK, + value: { + functionCalls: [ + { + id: 'notification-loop-1', + name: 'read_file', + args: { path: 'a' }, + }, + { + id: 'notification-loop-2', + name: 'read_file', + args: { path: 'b' }, + }, + ], + }, + }, + ]), + ) + .mockResolvedValue(createEmptyStream()); + + await runGuardPrompt(); + // Explicit one-call cap: the notification turn's Stop-continuation + // batch of two calls trips the per-turn cap inside + // #runStopContinuation, pinning the graceful default at the + // background-notification #handleStopHookLoop call site. + mockConfig.getMaxToolCallsPerTurn = vi.fn().mockReturnValue(1); + mockConfig.isMaxToolCallsPerTurnExplicit = vi.fn().mockReturnValue(true); + const callback = + mockBackgroundTaskRegistry.setNotificationCallback.mock.calls.at( + -1, + )?.[0] as ( + displayText: string, + modelText: string, + meta: { agentId: string; status: string }, + ) => void; + + callback('background done', '', { + agentId: 'automatic-agent', + status: 'completed', + }); + + await vi.waitFor(() => { + expect(mockChat.sendMessageStream).toHaveBeenCalledTimes(4); + }); + expect(logLoopDetectedSpy).toHaveBeenCalledWith( + mockConfig, + expect.objectContaining({ + loop_type: core.LoopType.TURN_TOOL_CALL_CAP, + }), + {}, + ); + expect( + vi.mocked(mockClient.sessionUpdate).mock.calls.some(([params]) => { + const update = params.update; + return ( + update.sessionUpdate === 'agent_message_chunk' && + update.content.type === 'text' && + update.content.text.includes('[notification error]') + ); + }), + ).toBe(false); + await vi.waitFor(() => { + expect(mockClient.extNotification).toHaveBeenCalledWith( + '_qwencode/end_turn', + { + sessionId: 'test-session-id', + reason: 'end_turn', + source: 'background_notification', + }, + ); + }); + }); + it('suspends an armed guard when a cron stream aborts', async () => { const scheduler = { hasPendingWork: true, diff --git a/packages/cli/src/acp-integration/session/Session.ts b/packages/cli/src/acp-integration/session/Session.ts index 7c27f399dad..3f5a0a0545a 100644 --- a/packages/cli/src/acp-integration/session/Session.ts +++ b/packages/cli/src/acp-integration/session/Session.ts @@ -500,7 +500,7 @@ const LOOP_DETECTED_SKIP_MESSAGE = 'Skipped because loop detection stopped the current turn before this tool call could run.'; const LOOP_DETECTED_CONTEXT_MESSAGE = 'System: this turn was terminated because the model exceeded tool-call safety limits. Try a different approach on the next turn.'; -const LOOP_DETECTED_TURN_ERROR_MESSAGE = +export const LOOP_DETECTED_TURN_ERROR_MESSAGE = 'Tool-call loop protection stopped this turn. The session is still available; send a more specific instruction to continue.'; const TOOL_EXECUTION_CANCELLED_MESSAGE = 'Tool execution was cancelled.'; const TOOL_POST_EXECUTION_CANCELLED_MESSAGE = @@ -644,6 +644,16 @@ function cancelledOrThrowLoopDetected( throw createLoopDetectedTurnError(loopState); } +function isLoopDetectedTurnError(error: unknown): boolean { + if (!(error instanceof RequestError)) return false; + const data = error.data; + return ( + typeof data === 'object' && + data !== null && + (data as { code?: unknown }).code === 'LOOP_DETECTED' + ); +} + function recordDaemonToolCalls( config: Config, promptId: string, @@ -3061,6 +3071,7 @@ export class Session implements SessionContext { resolveCompletion = resolve; }); + let rejectedByLoopProtection = false; try { const result = await this.#executePrompt( params, @@ -3068,6 +3079,10 @@ export class Session implements SessionContext { channelDeliveryCapture, invocationContext, modelPrompt, + // Channel turns are non-interactive deliveries: like cron and + // background-notification turns they keep the graceful end-turn + // handling so the collected response text is still delivered. + channelDelivery === undefined, ); releasePendingSend(); // Drain any cron prompts that queued while the prompt was active @@ -3093,11 +3108,16 @@ export class Session implements SessionContext { errorKind: error.errorKind, }); } + rejectedByLoopProtection = isLoopDetectedTurnError(error); throw error; } finally { const stillOwnsPendingPrompt = this.pendingPrompt === pendingSend; releasePendingSend(); const shouldDrainAutomaticQueues = + // Loop-detected turns resolved end_turn (and drained) before loop + // stops became rejections; keep that invariant on the new path so + // queued cron/notification work is not stranded. + rejectedByLoopProtection || todoStopGuardPreparation.drainSupersededAutomaticQueues || this.todoStopGuardDrainAutomaticQueuesWhenIdle || this.todoStopGuard.blocksUnrelatedAutomaticTurns || @@ -3286,6 +3306,7 @@ export class Session implements SessionContext { channelDeliveryCapture?: ChannelDeliveryCapture, invocationContext?: InvocationContextV1, modelPrompt?: string, + rejectOnLoopDetected = false, ): Promise { const sessionId = this.config.getSessionId(); if ( @@ -3308,6 +3329,7 @@ export class Session implements SessionContext { pendingSend, channelDeliveryCapture, modelPrompt, + rejectOnLoopDetected, ), ), ); @@ -3318,6 +3340,7 @@ export class Session implements SessionContext { pendingSend: AbortController, channelDeliveryCapture?: ChannelDeliveryCapture, modelPrompt?: string, + rejectOnLoopDetected = false, ): Promise { return Storage.runWithRuntimeBaseDir( this.runtimeBaseDir, @@ -3944,10 +3967,12 @@ export class Session implements SessionContext { nextMessage = nextAfterTools.message; if (nextAfterTools.stoppedByRepeatedToolFailure) { return { - stopReason: cancelledOrThrowLoopDetected( - pendingSend.signal, - toolLoopState, - ), + stopReason: rejectOnLoopDetected + ? cancelledOrThrowLoopDetected( + pendingSend.signal, + toolLoopState, + ) + : getAbortAwareEndTurnStopReason(pendingSend.signal), }; } if (toolRun.loopDetected) { @@ -3957,10 +3982,12 @@ export class Session implements SessionContext { pendingSend.signal, ); return { - stopReason: cancelledOrThrowLoopDetected( - pendingSend.signal, - toolLoopState, - ), + stopReason: rejectOnLoopDetected + ? cancelledOrThrowLoopDetected( + pendingSend.signal, + toolLoopState, + ) + : getAbortAwareEndTurnStopReason(pendingSend.signal), }; } } @@ -3981,7 +4008,7 @@ export class Session implements SessionContext { true, fullTurnModelOverride, channelDeliveryCapture, - true, // rejectOnLoopDetected + rejectOnLoopDetected, ); } finally { logConversationFinishedEvent( From 3a338cee0613ec5f344a01100b23288e863ef789 Mon Sep 17 00:00:00 2001 From: qwen-code-dev-bot Date: Tue, 11 Aug 2026 14:04:55 +0000 Subject: [PATCH 05/10] fix(cli): keep channel-prompt turns graceful on loop-detected stops Channel tasks prompted through CHANNEL_PROMPT_META_KEY (DaemonChannelBridge / AcpBridge) carry no channelDelivery meta, so the loop-protection exemption keyed on channelDelivery missed them: a per-turn tool-call cap trip rejected the turn with LOOP_DETECTED, the channel bridge never emitted promptComplete, and the collected response text was lost as a failed task. Extend the graceful end-turn exemption to channel-prompt-meta turns, mirroring the repeated-failure guard forcing those turns already receive. --- .../acp-integration/session/Session.test.ts | 54 +++++++++++++++++++ .../src/acp-integration/session/Session.ts | 11 +++- 2 files changed, 63 insertions(+), 2 deletions(-) diff --git a/packages/cli/src/acp-integration/session/Session.test.ts b/packages/cli/src/acp-integration/session/Session.test.ts index 93c2d6826fb..5d1de79ce47 100644 --- a/packages/cli/src/acp-integration/session/Session.test.ts +++ b/packages/cli/src/acp-integration/session/Session.test.ts @@ -11269,6 +11269,60 @@ describe('Session', () => { }); }); + it('keeps a channel-prompt-meta turn graceful when loop protection stops it', async () => { + // DaemonChannelBridge/AcpBridge channel tasks prompt with + // CHANNEL_PROMPT_META_KEY and no channelDelivery meta; like the + // channelDelivery path above they must resolve end_turn so the + // bridge emits promptComplete with the collected response text + // instead of the rejection failing the non-interactive task. + mockConfig.getApprovalMode = vi.fn().mockReturnValue(ApprovalMode.YOLO); + mockConfig.getMaxToolCallsPerTurn = vi.fn().mockReturnValue(1); + mockConfig.isMaxToolCallsPerTurnExplicit = vi + .fn() + .mockReturnValue(true); + mockChat.sendMessageStream = vi.fn().mockResolvedValueOnce( + createStreamWithChunks([ + { + type: core.StreamEventType.CHUNK, + value: { + functionCalls: [ + { + id: 'channel-prompt-loop-1', + name: 'read_file', + args: { file_path: 'a.ts' }, + }, + { + id: 'channel-prompt-loop-2', + name: 'read_file', + args: { file_path: 'b.ts' }, + }, + ], + }, + }, + ]), + ); + + await expect( + session.prompt({ + sessionId: 'test-session-id', + prompt: [{ type: 'text', text: 'channel task' }], + _meta: { [CHANNEL_PROMPT_META_KEY]: true }, + }), + ).resolves.toEqual({ stopReason: 'end_turn' }); + + expect(logLoopDetectedSpy).toHaveBeenCalledWith( + mockConfig, + expect.objectContaining({ + loop_type: core.LoopType.TURN_TOOL_CALL_CAP, + }), + {}, + ); + expect(mockClient.extMethod).not.toHaveBeenCalledWith( + 'qwen/control/channel-delivery', + expect.anything(), + ); + }); + it('replaces the prompt candidate with a Stop-hook continuation final', async () => { const messageBus = { request: vi diff --git a/packages/cli/src/acp-integration/session/Session.ts b/packages/cli/src/acp-integration/session/Session.ts index 3f5a0a0545a..839c9e7f413 100644 --- a/packages/cli/src/acp-integration/session/Session.ts +++ b/packages/cli/src/acp-integration/session/Session.ts @@ -3064,6 +3064,10 @@ export class Session implements SessionContext { const channelDeliveryCapture = channelDelivery ? { finalText: '' } : undefined; + const channelPromptTurn = + (params as { _meta?: Record })._meta?.[ + CHANNEL_PROMPT_META_KEY + ] === true; // Track this prompt's completion for the next prompt to await let resolveCompletion!: () => void; @@ -3081,8 +3085,11 @@ export class Session implements SessionContext { modelPrompt, // Channel turns are non-interactive deliveries: like cron and // background-notification turns they keep the graceful end-turn - // handling so the collected response text is still delivered. - channelDelivery === undefined, + // handling so the collected response text is still delivered. Both + // channel mechanisms qualify — the channelDelivery meta and the + // CHANNEL_PROMPT_META_KEY turns sent by the channel bridges, which + // carry no channelDelivery capture. + channelDelivery === undefined && !channelPromptTurn, ); releasePendingSend(); // Drain any cron prompts that queued while the prompt was active From c862d3fbbedde207fccc7654f21ae565b970456d Mon Sep 17 00:00:00 2001 From: qwen-code-dev-bot Date: Tue, 11 Aug 2026 17:46:05 +0000 Subject: [PATCH 06/10] fix(acp-bridge): keep loop turn error on refresh after idle bookkeeping (#8853) --- packages/acp-bridge/src/bridge.test.ts | 161 +++++++++++++++++++++++++ packages/acp-bridge/src/bridge.ts | 16 ++- 2 files changed, 173 insertions(+), 4 deletions(-) diff --git a/packages/acp-bridge/src/bridge.test.ts b/packages/acp-bridge/src/bridge.test.ts index 50d54df3dcf..6608905d5ea 100644 --- a/packages/acp-bridge/src/bridge.test.ts +++ b/packages/acp-bridge/src/bridge.test.ts @@ -4280,6 +4280,167 @@ describe('createAcpSessionBridge', () => { await bridge.shutdown(); }); + it.each([ + 'model_switch_failed', + 'language_changed', + 'session_metadata_updated', + 'session_cwd_changed', + 'artifact_changed', + ] as const)( + 'keeps the turn error on refresh when %s bookkeeping lands after it', + async (bookkeepingType) => { + // Idle-reachable bookkeeping (a rejected model switch, language + // change, rename, cwd change, client artifact) carries no turn + // content and must not defeat the refresh-append of the terminal. + const promptImpl = () => { + throw new RequestError(-32603, 'Loop protection stopped this turn', { + code: 'LOOP_DETECTED', + errorKind: 'loop_detected', + loopType: 'turn_tool_call_cap', + }); + }; + const extMethodImpl = ( + method: string, + params: Record, + ): Record => { + if (method === SERVE_STATUS_EXT_METHODS.sessionTranscript) { + return { + v: 1, + sessionId: params['sessionId'], + events: [ + { + v: 1, + type: 'session_update', + data: { + sessionUpdate: 'user_message_chunk', + content: { type: 'text', text: 'persisted turn content' }, + _meta: { + 'qwen.session.recordId': + 'record-loop-idle-bookkeeping-page', + }, + }, + }, + ], + hasMore: false, + }; + } + if (method === SERVE_CONTROL_EXT_METHODS.sessionLanguage) { + return { language: 'zh-CN', outputLanguage: null, refreshed: false }; + } + if (method === SERVE_CONTROL_EXT_METHODS.sessionCd) { + return { previousCwd: WS_A, newCwd: WS_B, warnings: [] }; + } + return {}; + }; + let bridge: ReturnType; + if (bookkeepingType === 'model_switch_failed') { + const factory: ChannelFactory = async () => { + const { clientStream, agentStream } = createInMemoryChannel(); + const fakeAgent = new FakeAgent({ promptImpl, extMethodImpl }); + const augmented = new Proxy(fakeAgent, { + get(target, prop) { + if (prop === 'unstable_setSessionModel') { + return async () => { + throw new Error('agent denied'); + }; + } + // eslint-disable-next-line @typescript-eslint/no-explicit-any + return (target as any)[prop]; + }, + }); + new AgentSideConnection(() => augmented as Agent, agentStream); + return { + stream: clientStream, + exited: new Promise< + | { exitCode: number | null; signalCode: NodeJS.Signals | null } + | undefined + >(() => {}), + kill: async () => {}, + killSync: () => {}, + }; + }; + bridge = makeBridge({ channelFactory: factory }); + } else { + const handle = makeChannel({ promptImpl, extMethodImpl }); + bridge = makeBridge({ channelFactory: async () => handle.channel }); + } + const session = await bridge.spawnOrAttach({ workspaceCwd: WS_A }); + + await expect( + bridge.sendPrompt( + session.sessionId, + { + sessionId: session.sessionId, + prompt: [{ type: 'text', text: 'loop' }], + }, + undefined, + { promptId: 'prompt-loop' }, + ), + ).rejects.toThrow('Loop protection stopped this turn'); + + const abort = new AbortController(); + const iter = bridge.subscribeEvents(session.sessionId, { + signal: abort.signal, + }); + const collectUntilBookkeeping = (async () => { + for await (const event of iter) { + if (event.type === bookkeepingType) return; + } + throw new Error(`${bookkeepingType} was not published`); + })(); + + if (bookkeepingType === 'model_switch_failed') { + await expect( + bridge.setSessionModel(session.sessionId, { + sessionId: session.sessionId, + modelId: 'rejected-model', + }), + ).rejects.toThrow(); + } else if (bookkeepingType === 'language_changed') { + await bridge.setSessionLanguage(session.sessionId, { + language: 'zh-CN', + syncOutputLanguage: false, + }); + } else if (bookkeepingType === 'session_metadata_updated') { + await bridge.updateSessionMetadata(session.sessionId, { + displayName: 'Renamed after loop stop', + }); + } else if (bookkeepingType === 'session_cwd_changed') { + await bridge.changeSessionCwd(session.sessionId, { path: WS_B }); + } else { + await bridge.addSessionArtifact( + session.sessionId, + { title: 'Client link', url: 'https://example.com/client' }, + { clientId: session.clientId }, + ); + } + await collectUntilBookkeeping; + + const refreshed = await bridge.loadSession({ + sessionId: session.sessionId, + workspaceCwd: WS_A, + clientId: session.clientId, + historyReplay: 'response', + historyPageSize: 100, + }); + + const compactedReplay = refreshed.compactedReplay ?? []; + expect(compactedReplay).toHaveLength(2); + expect(compactedReplay[compactedReplay.length - 1]).toMatchObject({ + type: 'turn_error', + promptId: 'prompt-loop', + data: expect.objectContaining({ + code: 'LOOP_DETECTED', + errorKind: 'loop_detected', + loopType: 'turn_tool_call_cap', + }), + }); + + abort.abort(); + await bridge.shutdown(); + }, + ); + it('drops the stale turn error on refresh after newer automatic-turn content', async () => { const handle = makeChannel({ promptImpl: () => { diff --git a/packages/acp-bridge/src/bridge.ts b/packages/acp-bridge/src/bridge.ts index f951863d181..9522b49f965 100644 --- a/packages/acp-bridge/src/bridge.ts +++ b/packages/acp-bridge/src/bridge.ts @@ -1219,17 +1219,25 @@ export function extractErrorCode(err: unknown): string | undefined { /** * Event types that may be published after a turn terminal without adding - * turn content (prompt-queue bookkeeping and config changes). The bounded - * refresh-append guard skips these when deciding whether the in-memory - * `turn_error` is still the newest meaningful terminal; any other event - * type blocks the append. + * turn content (prompt-queue bookkeeping, config changes, and other + * idle-reachable session bookkeeping). The bounded refresh-append guard + * skips these when deciding whether the in-memory `turn_error` is still + * the newest meaningful terminal; any other event type blocks the append. + * `pending_prompt_started` is deliberately absent: it is published before + * admission clears `turnErrorEvent`, so blocking the append in that window + * keeps a stale error from trailing a turn that is already starting. */ const REFRESH_APPEND_BOOKKEEPING_EVENT_TYPES = new Set([ 'pending_prompt_added', 'pending_prompt_completed', 'prompt_cancelled', 'model_switched', + 'model_switch_failed', 'approval_mode_changed', + 'language_changed', + 'session_metadata_updated', + 'session_cwd_changed', + 'artifact_changed', ]); export function classifyTurnErrorKind( From 6efa3ca58c06e9c6ad0bfb8fbe1624afa2cfcf7a Mon Sep 17 00:00:00 2001 From: qwen-code-dev-bot Date: Thu, 13 Aug 2026 10:03:03 +0000 Subject: [PATCH 07/10] fix(acp-bridge): harden loop-protection turn state per review feedback (#8853) --- .../web-shell-loop-detection-turn-error.md | 2 +- packages/acp-bridge/src/bridge.test.ts | 554 +++++++++++++++++- packages/acp-bridge/src/bridge.ts | 77 ++- .../cli/src/acp-integration/acpAgent.test.ts | 48 ++ packages/cli/src/acp-integration/acpAgent.ts | 18 +- .../acp-integration/session/Session.test.ts | 123 ++++ .../src/acp-integration/session/Session.ts | 47 +- 7 files changed, 840 insertions(+), 29 deletions(-) diff --git a/docs/design/web-shell-loop-detection-turn-error.md b/docs/design/web-shell-loop-detection-turn-error.md index 86ac2446c56..43ba59f5f6d 100644 --- a/docs/design/web-shell-loop-detection-turn-error.md +++ b/docs/design/web-shell-loop-detection-turn-error.md @@ -12,7 +12,7 @@ Web Shell renders `loop_detected` from the structured kind, using localized plai Skipped tools keep their existing failed terminal update and error details so they cannot remain pending and their display behavior does not change. The additional `turn_error` provides the user-facing explanation for the stopped turn. -The session remains alive and the per-turn loop state is recreated for the next prompt. Cron, background-notification, and channel-delivery turns keep their existing non-interactive handling: only interactive foreground prompts reject. A loop-detected rejection still drains the cron/notification queues, preserving the invariant that a loop-stopped turn never strands queued automatic work. +The session remains alive and the per-turn loop state is recreated for the next prompt. Cron, background-notification, channel-delivery, and goal turns keep their existing non-interactive handling: only interactive foreground prompts reject. Goal turns bypass the bridge entirely, so rejecting one would settle it as failed and pause the goal without publishing any `turn_error`; they resolve `end_turn` like the other automatic turn types. A loop-detected rejection still drains the cron/notification queues, preserving the invariant that a loop-stopped turn never strands queued automatic work. When Web Shell reloads a live session from paginated persisted history, the bridge appends the current in-memory `turn_error` to that replay. This keeps the terminal error visible across a page refresh without changing historical persistence. diff --git a/packages/acp-bridge/src/bridge.test.ts b/packages/acp-bridge/src/bridge.test.ts index d46f1befe4c..5ab76999d62 100644 --- a/packages/acp-bridge/src/bridge.test.ts +++ b/packages/acp-bridge/src/bridge.test.ts @@ -4435,8 +4435,14 @@ describe('createAcpSessionBridge', () => { const compactedReplay = refreshed.compactedReplay ?? []; expect(compactedReplay).toHaveLength(2); + // Anchor the replay on the persisted page so the degenerate in-memory + // fallback (identical shape for a single-turn session) cannot satisfy + // the assertion. expect(compactedReplay[0]).toMatchObject({ type: 'session_update', + data: expect.objectContaining({ + content: { type: 'text', text: 'persisted turn content' }, + }), }); expect(compactedReplay[compactedReplay.length - 1]).toMatchObject({ type: 'turn_error', @@ -4560,6 +4566,12 @@ describe('createAcpSessionBridge', () => { const compactedReplay = refreshed.compactedReplay ?? []; expect(compactedReplay).toHaveLength(2); + expect(compactedReplay[0]).toMatchObject({ + type: 'session_update', + data: expect.objectContaining({ + content: { type: 'text', text: 'persisted turn content' }, + }), + }); expect(compactedReplay[compactedReplay.length - 1]).toMatchObject({ type: 'turn_error', promptId: 'prompt-loop', @@ -4580,6 +4592,14 @@ describe('createAcpSessionBridge', () => { 'session_metadata_updated', 'session_cwd_changed', 'artifact_changed', + 'settings_changed', + 'extensions_changed', + 'mcp_server_changed', + 'mcp_server_added', + 'mcp_server_removed', + 'approval_mode_changed', + 'model_switched', + 'prompt_cancelled', ] as const)( 'keeps the turn error on refresh when %s bookkeeping lands after it', async (bookkeepingType) => { @@ -4624,10 +4644,35 @@ describe('createAcpSessionBridge', () => { if (method === SERVE_CONTROL_EXT_METHODS.sessionCd) { return { previousCwd: WS_A, newCwd: WS_B, warnings: [] }; } + if (method === SERVE_CONTROL_EXT_METHODS.sessionApprovalMode) { + return { previous: 'default', current: ApprovalMode.PLAN }; + } + if (method === SERVE_CONTROL_EXT_METHODS.workspaceMcpRuntimeAdd) { + return { name: params['name'], toolCount: 1 }; + } + if (method === SERVE_CONTROL_EXT_METHODS.workspaceMcpRuntimeRemove) { + return { + name: params['name'], + removed: true, + wasShadowingSettings: false, + originatorClientId: '', + }; + } + if (method === SERVE_CONTROL_EXT_METHODS.workspaceMcpManage) { + return { + serverName: params['serverName'], + action: params['action'], + ok: true, + }; + } return {}; }; let bridge: ReturnType; - if (bookkeepingType === 'model_switch_failed') { + const isModelSwitchCase = + bookkeepingType === 'model_switch_failed' || + bookkeepingType === 'model_switched' || + bookkeepingType === 'settings_changed'; + if (isModelSwitchCase) { const factory: ChannelFactory = async () => { const { clientStream, agentStream } = createInMemoryChannel(); const fakeAgent = new FakeAgent({ promptImpl, extMethodImpl }); @@ -4635,7 +4680,10 @@ describe('createAcpSessionBridge', () => { get(target, prop) { if (prop === 'unstable_setSessionModel') { return async () => { - throw new Error('agent denied'); + if (bookkeepingType === 'model_switch_failed') { + throw new Error('agent denied'); + } + return {}; }; } // eslint-disable-next-line @typescript-eslint/no-explicit-any @@ -4690,6 +4738,35 @@ describe('createAcpSessionBridge', () => { modelId: 'rejected-model', }), ).rejects.toThrow(); + } else if ( + bookkeepingType === 'model_switched' || + bookkeepingType === 'settings_changed' + ) { + // The successful model-switch path publishes `model_switched` + // followed by a workspace-broadcast `settings_changed` with no + // `skipSessionId` — both land in every session's journal. + await bridge.setSessionModel(session.sessionId, { + sessionId: session.sessionId, + modelId: 'new-model', + }); + } else if (bookkeepingType === 'approval_mode_changed') { + await bridge.setSessionApprovalMode( + session.sessionId, + ApprovalMode.PLAN, + { persist: false }, + ); + } else if (bookkeepingType === 'extensions_changed') { + bridge.broadcastExtensionsChanged({ refreshed: 1, failed: 0 }); + } else if (bookkeepingType === 'mcp_server_added') { + await bridge.addRuntimeMcpServer('loop-mcp-server', { + command: 'loop-mcp', + }); + } else if (bookkeepingType === 'mcp_server_removed') { + await bridge.removeRuntimeMcpServer('loop-mcp-server'); + } else if (bookkeepingType === 'mcp_server_changed') { + await bridge.manageMcpServer('loop-mcp-server', 'enable', undefined); + } else if (bookkeepingType === 'prompt_cancelled') { + await bridge.cancelSession(session.sessionId); } else if (bookkeepingType === 'language_changed') { await bridge.setSessionLanguage(session.sessionId, { language: 'zh-CN', @@ -4720,6 +4797,15 @@ describe('createAcpSessionBridge', () => { const compactedReplay = refreshed.compactedReplay ?? []; expect(compactedReplay).toHaveLength(2); + // Anchor the replay on the persisted page: for a single-turn session + // the degenerate in-memory fallback has an identical shape, so only + // the persisted fixture's content proves the append path ran. + expect(compactedReplay[0]).toMatchObject({ + type: 'session_update', + data: expect.objectContaining({ + content: { type: 'text', text: 'persisted turn content' }, + }), + }); expect(compactedReplay[compactedReplay.length - 1]).toMatchObject({ type: 'turn_error', promptId: 'prompt-loop', @@ -4735,6 +4821,462 @@ describe('createAcpSessionBridge', () => { }, ); + it('keeps the turn error on refresh when an idle user-shell command streams output after it', async () => { + // User-shell activity publishes `user_shell_command`, `session_update` + // (shell output, `_meta.source: 'user-shell'`), and `user_shell_result` + // on the session bus while idle. The output is injected into the model + // conversation history, not the persisted transcript the refresh pages, + // so none of the three may defeat the append. + const shellSpy = vi + .spyOn(ShellExecutionService, 'execute') + .mockImplementation(async (_command, _cwd, onEvent) => { + onEvent({ type: 'data', chunk: 'hello\n' }); + return { + pid: 123, + result: Promise.resolve({ + rawOutput: Buffer.from('hello\n'), + output: 'hello\n', + exitCode: 0, + signal: null, + error: null, + aborted: false, + pid: 123, + executionMethod: 'none', + }), + }; + }); + try { + const handle = makeChannel({ + promptImpl: () => { + throw new RequestError(-32603, 'Loop protection stopped this turn', { + code: 'LOOP_DETECTED', + errorKind: 'loop_detected', + loopType: 'turn_tool_call_cap', + }); + }, + extMethodImpl: (method, params) => { + if (method !== SERVE_STATUS_EXT_METHODS.sessionTranscript) { + return {}; + } + return { + v: 1, + sessionId: params['sessionId'], + events: [ + { + v: 1, + type: 'session_update', + data: { + sessionUpdate: 'user_message_chunk', + content: { type: 'text', text: 'persisted turn content' }, + _meta: { 'qwen.session.recordId': 'record-loop-shell-page' }, + }, + }, + ], + hasMore: false, + }; + }, + }); + const bridge = makeBridge({ + sessionShellCommandEnabled: true, + channelFactory: async () => handle.channel, + }); + const session = await bridge.spawnOrAttach({ workspaceCwd: WS_A }); + + await expect( + bridge.sendPrompt( + session.sessionId, + { + sessionId: session.sessionId, + prompt: [{ type: 'text', text: 'loop' }], + }, + undefined, + { promptId: 'prompt-loop' }, + ), + ).rejects.toThrow('Loop protection stopped this turn'); + + await bridge.executeShellCommand( + session.sessionId, + 'echo hello', + undefined, + { clientId: session.clientId }, + ); + + const refreshed = await bridge.loadSession({ + sessionId: session.sessionId, + workspaceCwd: WS_A, + clientId: session.clientId, + historyReplay: 'response', + historyPageSize: 100, + }); + + const compactedReplay = refreshed.compactedReplay ?? []; + expect(compactedReplay).toHaveLength(2); + expect(compactedReplay[compactedReplay.length - 1]).toMatchObject({ + type: 'turn_error', + promptId: 'prompt-loop', + data: expect.objectContaining({ + code: 'LOOP_DETECTED', + errorKind: 'loop_detected', + loopType: 'turn_tool_call_cap', + }), + }); + + await bridge.shutdown(); + } finally { + shellSpy.mockRestore(); + } + }); + + it('keeps the turn error on refresh when a queued deadline terminal lands after it', async () => { + // A queued prompt's terminal publishes the event alone without mutating + // turn state; it must not erase the refresh-replay record of the active + // turn's failure either. The held cancel ack keeps the queued prompt's + // FIFO promotion blocked on the cancel-forward drain, so its deadline + // expires while it is still queued AFTER the loop terminal has landed. + let promptCalls = 0; + const heldTurn = deferred(); + let releaseCancel!: () => void; + const heldCancel = new Promise((resolve) => { + releaseCancel = resolve; + }); + const handle = makeChannel({ + promptImpl: () => { + promptCalls += 1; + if (promptCalls === 1) return heldTurn.promise; + return { stopReason: 'end_turn' }; + }, + cancelImpl: () => heldCancel, + extMethodImpl: (method, params) => { + if (method !== SERVE_STATUS_EXT_METHODS.sessionTranscript) { + return {}; + } + return { + v: 1, + sessionId: params['sessionId'], + events: [ + { + v: 1, + type: 'session_update', + data: { + sessionUpdate: 'user_message_chunk', + content: { type: 'text', text: 'persisted turn content' }, + _meta: { + 'qwen.session.recordId': 'record-loop-queued-deadline-page', + }, + }, + }, + ], + hasMore: false, + }; + }, + }); + const bridge = makeBridge({ channelFactory: async () => handle.channel }); + const session = await bridge.spawnOrAttach({ workspaceCwd: WS_A }); + const abort = new AbortController(); + const iter = bridge.subscribeEvents(session.sessionId, { + signal: abort.signal, + }); + const sawQueuedDeadlineTerminal = (async () => { + for await (const event of iter) { + if ( + event.type === 'turn_error' && + event.promptId === 'prompt-queued' && + (event.data as { code?: string }).code === 'prompt_deadline_exceeded' + ) { + return; + } + } + throw new Error('queued deadline terminal was not published'); + })(); + + const first = bridge.sendPrompt( + session.sessionId, + { + sessionId: session.sessionId, + prompt: [{ type: 'text', text: 'first' }], + }, + undefined, + { promptId: 'prompt-first' }, + ); + first.catch(() => {}); + await vi.waitFor(() => { + expect(handle.agent.promptCalls).toHaveLength(1); + }); + + const second = bridge.sendPrompt( + session.sessionId, + { + sessionId: session.sessionId, + prompt: [{ type: 'text', text: 'queued' }], + }, + undefined, + { promptId: 'prompt-queued', deadlineMs: 120 }, + ); + second.catch(() => {}); + await vi.waitFor(() => { + expect(bridge.getPendingPrompts(session.sessionId)).toHaveLength(2); + }); + + // Cancel the running turn, then reject it with the loop error. The held + // cancel ack blocks the queued prompt's promotion on the cancel-forward + // drain, so it is still queued when the deadline expires. + void bridge.cancelSession(session.sessionId).catch(() => {}); + await vi.waitFor(() => { + expect(handle.agent.cancelCalls).toHaveLength(1); + }); + heldTurn.reject( + new RequestError(-32603, 'Loop protection stopped this turn', { + code: 'LOOP_DETECTED', + errorKind: 'loop_detected', + loopType: 'turn_tool_call_cap', + }), + ); + await expect(first).rejects.toThrow('Loop protection stopped this turn'); + + await sawQueuedDeadlineTerminal; + + const refreshed = await bridge.loadSession({ + sessionId: session.sessionId, + workspaceCwd: WS_A, + clientId: session.clientId, + historyReplay: 'response', + historyPageSize: 100, + }); + + const compactedReplay = refreshed.compactedReplay ?? []; + expect( + compactedReplay.some( + (event) => + event.type === 'turn_error' && + (event.data as { errorKind?: string }).errorKind === 'loop_detected', + ), + ).toBe(true); + // The queued terminal must not overwrite the session-scoped summary + // either: it belongs to a prompt that never ran. + expect(bridge.getSessionSummary(session.sessionId).turnError).toMatchObject( + { code: 'LOOP_DETECTED' }, + ); + + abort.abort(); + releaseCancel(); + await bridge.shutdown(); + }); + + it('keeps the turn error on refresh when a queued prompt is removed after it', async () => { + // DAEMON-004 variant: removing a still-queued prompt publishes its + // `cancelled` terminal alone. Like the deadline variant it must not + // clear the active turn's refresh-replay record. + let promptCalls = 0; + const heldTurn = deferred(); + let releaseCancel!: () => void; + const heldCancel = new Promise((resolve) => { + releaseCancel = resolve; + }); + const handle = makeChannel({ + promptImpl: () => { + promptCalls += 1; + if (promptCalls === 1) return heldTurn.promise; + return { stopReason: 'end_turn' }; + }, + cancelImpl: () => heldCancel, + extMethodImpl: (method, params) => { + if (method !== SERVE_STATUS_EXT_METHODS.sessionTranscript) { + return {}; + } + return { + v: 1, + sessionId: params['sessionId'], + events: [ + { + v: 1, + type: 'session_update', + data: { + sessionUpdate: 'user_message_chunk', + content: { type: 'text', text: 'persisted turn content' }, + _meta: { + 'qwen.session.recordId': 'record-loop-queued-removed-page', + }, + }, + }, + ], + hasMore: false, + }; + }, + }); + const bridge = makeBridge({ channelFactory: async () => handle.channel }); + const session = await bridge.spawnOrAttach({ workspaceCwd: WS_A }); + const abort = new AbortController(); + const iter = bridge.subscribeEvents(session.sessionId, { + signal: abort.signal, + }); + const sawQueuedRemovedTerminal = (async () => { + for await (const event of iter) { + if ( + event.type === 'turn_complete' && + event.promptId === 'prompt-queued' && + (event.data as { stopReason?: string }).stopReason === 'cancelled' + ) { + return; + } + } + throw new Error('queued removed terminal was not published'); + })(); + + const first = bridge.sendPrompt( + session.sessionId, + { + sessionId: session.sessionId, + prompt: [{ type: 'text', text: 'first' }], + }, + undefined, + { promptId: 'prompt-first' }, + ); + first.catch(() => {}); + await vi.waitFor(() => { + expect(handle.agent.promptCalls).toHaveLength(1); + }); + + const second = bridge.sendPrompt( + session.sessionId, + { + sessionId: session.sessionId, + prompt: [{ type: 'text', text: 'queued' }], + }, + undefined, + { promptId: 'prompt-queued' }, + ); + second.catch(() => {}); + await vi.waitFor(() => { + expect(bridge.getPendingPrompts(session.sessionId)).toHaveLength(2); + }); + + void bridge.cancelSession(session.sessionId).catch(() => {}); + await vi.waitFor(() => { + expect(handle.agent.cancelCalls).toHaveLength(1); + }); + heldTurn.reject( + new RequestError(-32603, 'Loop protection stopped this turn', { + code: 'LOOP_DETECTED', + errorKind: 'loop_detected', + loopType: 'turn_tool_call_cap', + }), + ); + await expect(first).rejects.toThrow('Loop protection stopped this turn'); + + // The queued prompt is removed while its promotion is still blocked on + // the cancel-forward drain. + expect( + bridge.removePendingPrompt(session.sessionId, 'prompt-queued'), + ).toEqual({ removed: true }); + await sawQueuedRemovedTerminal; + + const refreshed = await bridge.loadSession({ + sessionId: session.sessionId, + workspaceCwd: WS_A, + clientId: session.clientId, + historyReplay: 'response', + historyPageSize: 100, + }); + + const compactedReplay = refreshed.compactedReplay ?? []; + expect( + compactedReplay.some( + (event) => + event.type === 'turn_error' && + (event.data as { errorKind?: string }).errorKind === 'loop_detected', + ), + ).toBe(true); + expect(bridge.getSessionSummary(session.sessionId).turnError).toMatchObject( + { code: 'LOOP_DETECTED' }, + ); + + abort.abort(); + releaseCancel(); + await bridge.shutdown(); + }); + + it('drops the turn error on refresh after a subsequent successful interactive turn', async () => { + // Loop reject, then a successful prompt, then refresh: the newer turn's + // terminal supersedes the pending append, so the stale loop error must + // not reappear after the newer turn's content. + let promptCalls = 0; + const handle = makeChannel({ + promptImpl: () => { + promptCalls += 1; + if (promptCalls === 1) { + throw new RequestError(-32603, 'Loop protection stopped this turn', { + code: 'LOOP_DETECTED', + errorKind: 'loop_detected', + loopType: 'turn_tool_call_cap', + }); + } + return { stopReason: 'end_turn' }; + }, + extMethodImpl: (method, params) => { + if (method !== SERVE_STATUS_EXT_METHODS.sessionTranscript) { + return {}; + } + return { + v: 1, + sessionId: params['sessionId'], + events: [ + { + v: 1, + type: 'session_update', + data: { + sessionUpdate: 'user_message_chunk', + content: { type: 'text', text: 'persisted turn content' }, + _meta: { 'qwen.session.recordId': 'record-loop-recovery-page' }, + }, + }, + ], + hasMore: false, + }; + }, + }); + const bridge = makeBridge({ channelFactory: async () => handle.channel }); + const session = await bridge.spawnOrAttach({ workspaceCwd: WS_A }); + + await expect( + bridge.sendPrompt( + session.sessionId, + { + sessionId: session.sessionId, + prompt: [{ type: 'text', text: 'loop' }], + }, + undefined, + { promptId: 'prompt-loop' }, + ), + ).rejects.toThrow('Loop protection stopped this turn'); + + await expect( + bridge.sendPrompt( + session.sessionId, + { + sessionId: session.sessionId, + prompt: [{ type: 'text', text: 'recovery' }], + }, + undefined, + { promptId: 'prompt-recovery' }, + ), + ).resolves.toEqual({ stopReason: 'end_turn' }); + + const refreshed = await bridge.loadSession({ + sessionId: session.sessionId, + workspaceCwd: WS_A, + clientId: session.clientId, + historyReplay: 'response', + historyPageSize: 100, + }); + + const compactedReplay = refreshed.compactedReplay ?? []; + expect(compactedReplay.some((event) => event.type === 'turn_error')).toBe( + false, + ); + + await bridge.shutdown(); + }); + it('drops the stale turn error on refresh after newer automatic-turn content', async () => { const handle = makeChannel({ promptImpl: () => { @@ -4817,6 +5359,14 @@ describe('createAcpSessionBridge', () => { }); const compactedReplay = refreshed.compactedReplay ?? []; + // The retained event must BE the persisted-page event — a stale branch + // that rebuilds the replay from anything else ships green without this. + expect(compactedReplay[0]).toMatchObject({ + type: 'session_update', + data: expect.objectContaining({ + content: { type: 'text', text: 'persisted turn content' }, + }), + }); expect(compactedReplay).toHaveLength(1); expect(compactedReplay.some((event) => event.type === 'turn_error')).toBe( false, diff --git a/packages/acp-bridge/src/bridge.ts b/packages/acp-bridge/src/bridge.ts index d6765badd60..85dd128dfd9 100644 --- a/packages/acp-bridge/src/bridge.ts +++ b/packages/acp-bridge/src/bridge.ts @@ -1525,6 +1525,7 @@ function broadcastTurnComplete( promptResult: { stopReason?: string; [k: string]: unknown }, promptId: string | undefined, originatorClientId: string | undefined, + mutateTurnState: boolean, ): void { try { const published = entry.events.publish({ @@ -1537,8 +1538,13 @@ function broadcastTurnComplete( }, ...(originatorClientId ? { originatorClientId } : {}), }); - // A newer turn terminal supersedes any pending refresh-append error. - if (published !== undefined) entry.turnErrorEvent = undefined; + // A newer turn terminal supersedes any pending refresh-append error — + // but only for a prompt that actually ran. A queued prompt's terminal + // (deadline expiry, queued removal) publishes the event alone without + // mutating turn state, so it must not erase the refresh-replay record + // of the active turn's failure either. + if (mutateTurnState && published !== undefined) + entry.turnErrorEvent = undefined; } catch { /* bus may be closed during session teardown */ } @@ -1609,6 +1615,16 @@ export function extractErrorCode(err: unknown): string | undefined { * `pending_prompt_started` is deliberately absent: it is published before * admission clears `turnErrorEvent`, so blocking the append in that window * keeps a stale error from trailing a turn that is already starting. + * + * Audit the full `broadcastWorkspaceEvent` vocabulary (and any other + * idle-reachable session-bus publish) before adding a new event type to + * the bus: every idle non-turn event belongs here, or an otherwise-idle + * activity — a model switch, an extension refresh, an MCP server change, + * a user-shell command — defeats the append and the loop terminal + * disappears from the refreshed transcript. `session_update` events are + * turn content except the user-shell output stream, which the guard + * skips via `isUserShellSessionUpdate` (its history goes to the model + * conversation, not the persisted transcript the refresh pages). */ const REFRESH_APPEND_BOOKKEEPING_EVENT_TYPES = new Set([ 'pending_prompt_added', @@ -1621,8 +1637,34 @@ const REFRESH_APPEND_BOOKKEEPING_EVENT_TYPES = new Set([ 'session_metadata_updated', 'session_cwd_changed', 'artifact_changed', + 'settings_changed', + 'extensions_changed', + 'mcp_server_changed', + 'mcp_server_added', + 'mcp_server_removed', + 'user_shell_command', + 'user_shell_result', ]); +/** + * User-shell output is published as `session_update` on the session bus + * while idle, but it carries no turn content for the persisted + * transcript (it is injected into the model conversation history + * instead), so it must not defeat the refresh-append of a pending + * terminal error the way a real turn's `session_update` does. + */ +function isUserShellSessionUpdate(event: BridgeEvent): boolean { + if (event.type !== 'session_update') return false; + const data = event.data; + if (!data || typeof data !== 'object' || Array.isArray(data)) return false; + const update = (data as Record)['update']; + if (!update || typeof update !== 'object' || Array.isArray(update)) + return false; + const meta = (update as Record)['_meta']; + if (!meta || typeof meta !== 'object' || Array.isArray(meta)) return false; + return (meta as Record)['source'] === 'user-shell'; +} + export function classifyTurnErrorKind( message: string, ): 'model_stream_interrupted' | undefined { @@ -1686,12 +1728,12 @@ function broadcastTurnError( }); if (mutateTurnState) { // Undefined when the bus dropped the publish (closed mid-teardown); - // the refresh-append guard then simply has nothing to replay. + // the refresh-append guard then simply has nothing to replay. A + // queued prompt's terminal (mutateTurnState=false) publishes the + // event alone and leaves the active turn's refresh-replay record + // untouched — the prompt never ran, so its terminal is not a newer + // turn boundary for the replay. entry.turnErrorEvent = published; - } else if (published !== undefined) { - // A queued prompt's terminal is a newer turn boundary: the prior - // in-memory error must no longer be replayed on refresh. - entry.turnErrorEvent = undefined; } } catch { /* bus may be closed during session teardown */ @@ -1735,6 +1777,14 @@ function publishPromptTerminal( } pendingEntry.terminalPublished = true; const originatorClientId = pendingEntry.originatorClientId; + // Only a running prompt's terminal belongs to the active turn. The + // `state === 'running'` gate (not `activePromptId`) is deliberate: on + // the normal settle path `settleActivePromptState` runs in + // `promptPromise.finally` BEFORE the terminal is published, so + // `activePromptId` is already cleared when a genuine active terminal + // lands here. Queued terminals publish their event alone and must + // neither set nor clear session-scoped turn state. + const mutateTurnState = pendingEntry.state === 'running'; if (terminal.kind === 'complete') { broadcastTurnComplete( entry, @@ -1742,6 +1792,7 @@ function publishPromptTerminal( terminal.result, pendingEntry.promptId, originatorClientId, + mutateTurnState, ); } else if (terminal.kind === 'cancelled') { broadcastTurnComplete( @@ -1750,6 +1801,7 @@ function publishPromptTerminal( { stopReason: 'cancelled' }, pendingEntry.promptId, originatorClientId, + mutateTurnState, ); } else { broadcastTurnError( @@ -1758,13 +1810,7 @@ function publishPromptTerminal( terminal.err, pendingEntry.promptId, originatorClientId, - // Only a running prompt's failure is the active turn's failure. The - // `state === 'running'` gate (not `activePromptId`) is deliberate: - // on the normal settle path `settleActivePromptState` runs in - // `promptPromise.finally` BEFORE the terminal is published, so - // `activePromptId` is already cleared when a genuine active failure - // lands here. - pendingEntry.state === 'running', + mutateTurnState, ); } } @@ -5549,7 +5595,8 @@ export function createAcpSessionBridge(opts: BridgeOptions): AcpSessionBridge { const journal = entry.events.liveJournalSnapshot() ?? []; const hasNewerTurnContent = journal.some( (event) => - !REFRESH_APPEND_BOOKKEEPING_EVENT_TYPES.has(event.type), + !REFRESH_APPEND_BOOKKEEPING_EVENT_TYPES.has(event.type) && + !isUserShellSessionUpdate(event), ); if (!hasNewerTurnContent) { compactedReplay = [...page.events, turnErrorEvent]; diff --git a/packages/cli/src/acp-integration/acpAgent.test.ts b/packages/cli/src/acp-integration/acpAgent.test.ts index 1f3d585cce5..3e93d310fcd 100644 --- a/packages/cli/src/acp-integration/acpAgent.test.ts +++ b/packages/cli/src/acp-integration/acpAgent.test.ts @@ -2147,6 +2147,7 @@ describe('QwenAgent MCP SSE/HTTP support', () => { 'qwen-code/private-parent-capability': 'must-not-propagate', 'qwen.daemon.modelPrompt': 'trusted model-only prompt', 'qwen.daemon.promptDisplayText': 'trusted display text', + 'qwen.channel.prompt': true, }, }); @@ -2157,6 +2158,7 @@ describe('QwenAgent MCP SSE/HTTP support', () => { _meta: { keep: true, 'qwen.daemon.promptDisplayText': 'trusted display text', + 'qwen.channel.prompt': true, }, }, invocation, @@ -2167,6 +2169,52 @@ describe('QwenAgent MCP SSE/HTTP support', () => { await agentPromise; }); + it('strips channel-prompt classification from untrusted callers', async () => { + // `qwen.channel.prompt` opts a turn out of loop-detected rejection and + // the repeated-failure guard. Only trusted parents (the channel bridges + // and the daemon bridge) may set it; an untrusted client marking its own + // prompt as a channel turn must not reach the session. A plain + // `qwen --acp` child has no expected capability and initializes + // untrusted. + await setupSessionMocks('untrusted-session'); + const agentPromise = runAcpAgent( + mockConfig, + makeSessionSettings(), + mockArgv, + ); + await vi.waitFor(() => expect(capturedAgentFactory).toBeDefined()); + const agent = capturedAgentFactory!({ + get closed() { + return mockConnectionState.promise; + }, + }) as AgentLike; + await agent.initialize({ clientCapabilities: {} }); + await agent.newSession({ cwd: '/tmp', mcpServers: [] }); + + await agent.prompt({ + sessionId: 'untrusted-session', + prompt: [{ type: 'text', text: 'hello' }], + _meta: { + keep: true, + 'qwen.channel.prompt': true, + }, + }); + + expect(lastSessionMock?.prompt).toHaveBeenCalledWith( + { + sessionId: 'untrusted-session', + prompt: [{ type: 'text', text: 'hello' }], + _meta: { keep: true }, + }, + undefined, + expect.any(AbortSignal), + undefined, + ); + + mockConnectionState.resolve(); + await agentPromise; + }); + it('closes managed writers before resource shutdown on connection EOF', async () => { const innerConfig = await setupSessionMocks('managed-session'); const order: string[] = []; diff --git a/packages/cli/src/acp-integration/acpAgent.ts b/packages/cli/src/acp-integration/acpAgent.ts index 2b1e7dfa465..e2a74e9789b 100644 --- a/packages/cli/src/acp-integration/acpAgent.ts +++ b/packages/cli/src/acp-integration/acpAgent.ts @@ -169,7 +169,10 @@ import { } from './authMethods.js'; import { AcpFileSystemService } from './service/filesystem.js'; import { ndJsonStream } from '@qwen-code/acp-bridge/ndJsonStream'; -import { ACP_EVENT_LOOP_STALL_RESTART_MS } from '@qwen-code/channel-base'; +import { + ACP_EVENT_LOOP_STALL_RESTART_MS, + CHANNEL_PROMPT_META_KEY, +} from '@qwen-code/channel-base'; import { Readable, Writable } from 'node:stream'; import { normalizeDisabledToolList } from '../config/normalizeDisabledTools.js'; import { pipeline } from 'node:stream/promises'; @@ -5484,10 +5487,12 @@ class QwenAgent implements Agent { const suppliedContext = meta[INVOCATION_CONTEXT_META_KEY]; const suppliedModelPrompt = meta[DAEMON_MODEL_PROMPT_META_KEY]; const suppliedPromptDisplayText = meta[DAEMON_PROMPT_DISPLAY_TEXT_META_KEY]; + const suppliedChannelPrompt = meta[CHANNEL_PROMPT_META_KEY]; delete meta[INVOCATION_CONTEXT_META_KEY]; delete meta[DAEMON_MODEL_PROMPT_META_KEY]; delete meta[PRIVATE_PARENT_CAPABILITY_META_KEY]; delete meta[DAEMON_PROMPT_DISPLAY_TEXT_META_KEY]; + delete meta[CHANNEL_PROMPT_META_KEY]; // The user-facing display projection is caller-controlled metadata; honor // it only for trusted parents (the daemon bridge re-injects the trusted // channel-worker value here). A plain delete would drop that re-injection. @@ -5497,6 +5502,17 @@ class QwenAgent implements Agent { ) { meta[DAEMON_PROMPT_DISPLAY_TEXT_META_KEY] = suppliedPromptDisplayText; } + // Channel classification is trusted-parent metadata: only the channel + // bridges and the daemon bridge hold the private parent capability. An + // untrusted caller must not be able to mark its own prompt as a channel + // turn — that opts the turn out of loop-detected rejection and the + // repeated-failure guard. + if ( + this.privateParentState === 'trusted' && + suppliedChannelPrompt === true + ) { + meta[CHANNEL_PROMPT_META_KEY] = true; + } if (Object.keys(meta).length > 0) { sanitizedParams._meta = meta; } else { diff --git a/packages/cli/src/acp-integration/session/Session.test.ts b/packages/cli/src/acp-integration/session/Session.test.ts index ba711b65488..b75d3d8dd87 100644 --- a/packages/cli/src/acp-integration/session/Session.test.ts +++ b/packages/cli/src/acp-integration/session/Session.test.ts @@ -8782,6 +8782,51 @@ describe('Session', () => { } }); + it('forces channel-delivery prompts off even when enforcement is configured', async () => { + // One server-side channel classification gates both the rejection + // and the guard mode: channelDelivery turns are non-interactive + // deliveries, so the repeated-failure guard must not stop them + // either — they keep running to their natural end. + recreateSessionWithGuardMode('enforce'); + try { + const execute = installFailingTool(); + queueMatchingFailureStreak(); + + await expect( + session.prompt({ + sessionId: 'test-session-id', + prompt: [{ type: 'text', text: 'channel delivery task' }], + _meta: { + 'qwen.daemon.channelDelivery': { + deliveryId: 'prompt-guard-off-delivery', + target: { + channelName: 'dingtalk', + type: 'user', + id: 'user-1', + }, + }, + }, + }), + ).resolves.toEqual({ stopReason: 'end_turn' }); + + expect(execute).toHaveBeenCalledTimes(9); + expect(mockChat.sendMessageStream).toHaveBeenCalledTimes(4); + expect(logRepeatedToolFailureGuardSpy).not.toHaveBeenCalled(); + // Drain the scheduled channel delivery before teardown so the + // unref'd delivery timer cannot fire after the mocks reset. + await vi.waitFor(() => { + expect(mockClient.extMethod).toHaveBeenCalledWith( + 'qwen/control/channel-delivery', + expect.objectContaining({ + deliveryId: 'prompt-guard-off-delivery', + }), + ); + }); + } finally { + restoreGuardMode(); + } + }); + it('forces channel-routed prompts off even when enforcement is configured', async () => { recreateSessionWithGuardMode('enforce'); try { @@ -15113,6 +15158,84 @@ describe('Session', () => { expect(mockGoalRuntime.finishTurn).not.toHaveBeenCalled(); }); + it('keeps a Goal turn graceful when loop protection stops it', async () => { + // Goal continuations are non-interactive and bypass the bridge: a + // rejection would settle the turn as failed and pause the goal + // with no turn_error ever published. They resolve end_turn like + // cron and channel turns, settling the iteration normally. + mockConfig.getApprovalMode = vi.fn().mockReturnValue(ApprovalMode.YOLO); + mockConfig.getMaxToolCallsPerTurn = vi.fn().mockReturnValue(1); + mockConfig.isMaxToolCallsPerTurnExplicit = vi + .fn() + .mockReturnValue(true); + const permit: core.GoalTurnPermit = { + goalId: 'goal-1', + revision: 1, + turnId: 'turn-loop-cap', + }; + const turnKey = 'goal-runtime:turn-loop-cap'; + mockGoalRuntime.getSnapshot.mockReturnValue({ + v: 2, + activity: 'running', + goal: { + goalId: 'goal-1', + revision: 1, + objective: 'check weather', + status: 'active', + evidenceCursor: { recordId: 'cursor-1' }, + turnCount: 0, + activeTimeMs: 0, + createdAt: 1234, + updatedAt: 1234, + }, + }); + mockGoalRuntime.permitForTurn.mockImplementation((key: string) => + key === turnKey ? permit : undefined, + ); + mockChat.sendMessageStream = vi.fn().mockResolvedValueOnce( + createStreamWithChunks([ + { + type: core.StreamEventType.CHUNK, + value: { + functionCalls: [ + { + id: 'goal-loop-1', + name: 'read_file', + args: { file_path: 'a.ts' }, + }, + { + id: 'goal-loop-2', + name: 'read_file', + args: { file_path: 'b.ts' }, + }, + ], + }, + }, + ]), + ); + + expect(boundGoalHost).toBeDefined(); + await boundGoalHost!.startGoalTurn({ + permit, + continuationContext: 'check weather', + }); + + await vi.waitFor(() => { + expect(logLoopDetectedSpy).toHaveBeenCalledWith( + mockConfig, + expect.objectContaining({ + loop_type: core.LoopType.TURN_TOOL_CALL_CAP, + }), + {}, + ); + }); + // Graceful end_turn settles the iteration; the goal is not paused. + await vi.waitFor(() => { + expect(mockGoalRuntime.finishTurn).toHaveBeenCalledWith(permit); + }); + expect(mockGoalRuntime.dispatch).not.toHaveBeenCalled(); + }); + it('pauses without counting a Goal turn cancelled before the model request', async () => { // `modelStarted` decides whether settlement records an iteration. // A user cancel still pauses the Goal before that point; releasing diff --git a/packages/cli/src/acp-integration/session/Session.ts b/packages/cli/src/acp-integration/session/Session.ts index 92b93cf9abf..2bab1c173c0 100644 --- a/packages/cli/src/acp-integration/session/Session.ts +++ b/packages/cli/src/acp-integration/session/Session.ts @@ -3669,6 +3669,11 @@ export class Session implements SessionContext { (params as { _meta?: Record })._meta?.[ CHANNEL_PROMPT_META_KEY ] === true; + // One server-side channel classification, consumed by both the + // rejection gate below and the guard-mode selection in + // #executePromptInner. The ACP boundary strips the channel-prompt key + // from untrusted callers, so both decisions see only trusted values. + const channelTurn = channelDelivery !== undefined || channelPromptTurn; // Track this prompt's completion for the next prompt to await let resolveCompletion!: () => void; @@ -3686,14 +3691,18 @@ export class Session implements SessionContext { channelDeliveryCapture, invocationContext, modelPrompt, - // Channel turns are non-interactive deliveries: like cron and - // background-notification turns they keep the graceful end-turn - // handling so the collected response text is still delivered. Both - // channel mechanisms qualify — the channelDelivery meta and the - // CHANNEL_PROMPT_META_KEY turns sent by the channel bridges, which - // carry no channelDelivery capture. - channelDelivery === undefined && !channelPromptTurn, + // Channel turns are non-interactive deliveries: like cron, + // background-notification, and goal turns they keep the graceful + // end-turn handling so the collected response text is still + // delivered. Both channel mechanisms qualify — the channelDelivery + // meta and the CHANNEL_PROMPT_META_KEY turns sent by the channel + // bridges, which carry no channelDelivery capture. Goal turns + // bypass the bridge entirely, so a rejection there would settle + // the turn as failed and pause the goal without any turn_error + // ever being published. + !channelTurn && goalTurn === undefined, goalTurn, + channelTurn, ); promptResult = result; releasePendingSend(); @@ -3927,6 +3936,7 @@ export class Session implements SessionContext { modelPrompt?: string, rejectOnLoopDetected = false, goalTurn?: AcpGoalTurn, + channelTurn = false, ): Promise { const sessionId = this.config.getSessionId(); if ( @@ -3952,6 +3962,7 @@ export class Session implements SessionContext { modelPrompt, rejectOnLoopDetected, goalTurn, + channelTurn, ), ), ); @@ -3967,6 +3978,7 @@ export class Session implements SessionContext { modelPrompt?: string, rejectOnLoopDetected = false, goalTurn?: AcpGoalTurn, + channelTurn = false, ): Promise { return Storage.runWithRuntimeBaseDir( this.runtimeBaseDir, @@ -4361,9 +4373,7 @@ export class Session implements SessionContext { let nextMessage: Content | null = { role: 'user', parts }; let turnCount = 0; const toolLoopState = createDaemonToolLoopState( - promptMetadata?.[CHANNEL_PROMPT_META_KEY] === true - ? 'off' - : this.repeatedToolFailureGuardMode, + channelTurn ? 'off' : this.repeatedToolFailureGuardMode, ); // conversation_finished must fire on every terminal path of the @@ -4640,6 +4650,7 @@ export class Session implements SessionContext { promptId, toolLoopState, onFullTurnModel, + rejectOnLoopDetected, ); nextMessage = nextAfterTools.message; if (nextAfterTools.stoppedByRepeatedToolFailure) { @@ -5630,6 +5641,7 @@ export class Session implements SessionContext { toolPromptId, toolLoopState, options.onFullTurnModel, + options.rejectOnLoopDetected ?? false, ); nextMessage = nextAfterTools.message; if (nextAfterTools.hadMidTurnUserInput) { @@ -6071,6 +6083,7 @@ export class Session implements SessionContext { promptId: string, toolLoopState: DaemonToolLoopState, onFullTurnModel?: (model: string) => boolean, + rejectOnLoopDetected = false, ): Promise { if (toolRun.loopDetected) { debugLogger.debug('Stopping ACP turn after daemon loop detection.'); @@ -6164,6 +6177,20 @@ export class Session implements SessionContext { toolLoopState, { recordToQwenLogger: false }, ); + if (!rejectOnLoopDetected) { + // Rejecting turns publish the structured turn_error as the + // user-visible explanation; graceful (non-interactive) stops have + // no replacement, so keep the transcript stop message for them. + try { + await this.messageEmitter.emitAgentMessage( + REPEATED_TOOL_FAILURE_STOP_MESSAGE, + ); + } catch (error) { + debugLogger.warn( + `Failed to emit repeated tool failure stop message: ${this.#formatError(error)}`, + ); + } + } return { message: null, hadMidTurnUserInput, From 91ba4193455f7b97694e3ce548dfe4f8a481e387 Mon Sep 17 00:00:00 2001 From: qwen-code-dev-bot Date: Thu, 13 Aug 2026 17:18:22 +0000 Subject: [PATCH 08/10] fix(acp): gate channel-turn classification and harden loop-error refresh replay (#8853) Close two trust-gate gaps in the channel-turn classification introduced for loop-detected turn errors: the `qwen.daemon.channelDelivery` sibling key was not gated like `qwen.channel.prompt` at the standalone ACP boundary, and on the daemon-hosted path a client-forged `qwen.channel.prompt` survived the serve route and bridge admission strip to be re-injected for trusted parents. Both keys are now stripped from untrusted callers and honored only from trusted context (private parent state, or the channel-worker prompt authorization validated by the daemon prompt route). Also harden the refresh-append guard for pending turn errors: the synthetic `history_truncated` journal marker and idle latest-wins `session_update` snapshots no longer defeat the append, and a queued terminal that folds newer turn content supersedes the stale error before the fold erases the evidence. Web Shell now derives turn_complete's error from the same backward walk as the retry decision. --- .../web-shell-loop-detection-turn-error.md | 2 +- packages/acp-bridge/src/bridge.test.ts | 474 +++++++++++++++++- packages/acp-bridge/src/bridge.ts | 78 ++- packages/acp-bridge/src/bridgeTypes.ts | 10 + .../channels/base/src/ChannelAgentBridge.ts | 4 +- .../base/src/DaemonChannelBridge.test.ts | 34 ++ .../channels/base/src/DaemonChannelBridge.ts | 8 +- .../cli/src/acp-integration/acpAgent.test.ts | 21 +- packages/cli/src/acp-integration/acpAgent.ts | 12 + .../acp-integration/session/Session.test.ts | 129 +++++ .../channel/channel-prompt-wire-key.test.ts | 13 + packages/cli/src/serve/routes/session.ts | 24 +- packages/cli/src/serve/server.test.ts | 60 ++- packages/web-shell/client/App.test.tsx | 46 ++ packages/web-shell/client/App.tsx | 15 +- 15 files changed, 885 insertions(+), 45 deletions(-) create mode 100644 packages/cli/src/commands/channel/channel-prompt-wire-key.test.ts diff --git a/docs/design/web-shell-loop-detection-turn-error.md b/docs/design/web-shell-loop-detection-turn-error.md index 43ba59f5f6d..576c4e2b6c3 100644 --- a/docs/design/web-shell-loop-detection-turn-error.md +++ b/docs/design/web-shell-loop-detection-turn-error.md @@ -14,7 +14,7 @@ Skipped tools keep their existing failed terminal update and error details so th The session remains alive and the per-turn loop state is recreated for the next prompt. Cron, background-notification, channel-delivery, and goal turns keep their existing non-interactive handling: only interactive foreground prompts reject. Goal turns bypass the bridge entirely, so rejecting one would settle it as failed and pause the goal without publishing any `turn_error`; they resolve `end_turn` like the other automatic turn types. A loop-detected rejection still drains the cron/notification queues, preserving the invariant that a loop-stopped turn never strands queued automatic work. -When Web Shell reloads a live session from paginated persisted history, the bridge appends the current in-memory `turn_error` to that replay. This keeps the terminal error visible across a page refresh without changing historical persistence. +When Web Shell reloads a live session from paginated persisted history, the bridge appends the current in-memory `turn_error` to that replay. This keeps the terminal error visible across a page refresh while the session remains idle; newer turn content — including automatic turns the rejection itself drains — supersedes it by design. ## Compatibility diff --git a/packages/acp-bridge/src/bridge.test.ts b/packages/acp-bridge/src/bridge.test.ts index 294498a90bd..5ba2b44cbdd 100644 --- a/packages/acp-bridge/src/bridge.test.ts +++ b/packages/acp-bridge/src/bridge.test.ts @@ -4950,6 +4950,15 @@ describe('createAcpSessionBridge', () => { const compactedReplay = refreshed.compactedReplay ?? []; expect(compactedReplay).toHaveLength(2); + // Anchor the replay on the persisted page so a regression of the + // bounded-append branch into the in-memory fallback cannot satisfy + // the assertions with an identically shaped replay. + expect(compactedReplay[0]).toMatchObject({ + type: 'session_update', + data: expect.objectContaining({ + content: { type: 'text', text: 'persisted turn content' }, + }), + }); expect(compactedReplay[compactedReplay.length - 1]).toMatchObject({ type: 'turn_error', promptId: 'prompt-loop', @@ -4966,6 +4975,237 @@ describe('createAcpSessionBridge', () => { } }); + it('keeps the turn error on refresh when journal truncation marks the idle tail', async () => { + // With a pinned small journal cap, idle user-shell output evicts its + // own older entries after the loop terminal; `liveJournalSnapshot` + // then unshifts the synthetic `history_truncated` marker. The marker + // is size accounting, never ingested turn content — it must not + // defeat the append the way real newer content does. + const shellSpy = vi + .spyOn(ShellExecutionService, 'execute') + .mockImplementation(async (_command, _cwd, onEvent) => { + onEvent({ type: 'data', chunk: 'first chunk\n' }); + onEvent({ type: 'data', chunk: 'second chunk\n' }); + return { + pid: 123, + result: Promise.resolve({ + rawOutput: Buffer.from('first chunk\nsecond chunk\n'), + output: 'first chunk\nsecond chunk\n', + exitCode: 0, + signal: null, + error: null, + aborted: false, + pid: 123, + executionMethod: 'none', + }), + }; + }); + try { + const handle = makeChannel({ + promptImpl: () => { + throw new RequestError(-32603, 'Loop protection stopped this turn', { + code: 'LOOP_DETECTED', + errorKind: 'loop_detected', + loopType: 'turn_tool_call_cap', + }); + }, + extMethodImpl: (method, params) => { + if (method !== SERVE_STATUS_EXT_METHODS.sessionTranscript) { + return {}; + } + return { + v: 1, + sessionId: params['sessionId'], + events: [ + { + v: 1, + type: 'session_update', + data: { + sessionUpdate: 'user_message_chunk', + content: { type: 'text', text: 'persisted turn content' }, + _meta: { + 'qwen.session.recordId': + 'record-loop-truncated-journal-page', + }, + }, + }, + ], + hasMore: false, + }; + }, + }); + const bridge = makeBridge({ + sessionShellCommandEnabled: true, + maxJournalEvents: 1, + channelFactory: async () => handle.channel, + }); + const session = await bridge.spawnOrAttach({ workspaceCwd: WS_A }); + + await expect( + bridge.sendPrompt( + session.sessionId, + { + sessionId: session.sessionId, + prompt: [{ type: 'text', text: 'loop' }], + }, + undefined, + { promptId: 'prompt-loop' }, + ), + ).rejects.toThrow('Loop protection stopped this turn'); + + await bridge.executeShellCommand( + session.sessionId, + 'echo truncated', + undefined, + { clientId: session.clientId }, + ); + + const refreshed = await bridge.loadSession({ + sessionId: session.sessionId, + workspaceCwd: WS_A, + clientId: session.clientId, + historyReplay: 'response', + historyPageSize: 100, + }); + + const compactedReplay = refreshed.compactedReplay ?? []; + expect(compactedReplay).toHaveLength(2); + expect(compactedReplay[0]).toMatchObject({ + type: 'session_update', + data: expect.objectContaining({ + content: { type: 'text', text: 'persisted turn content' }, + }), + }); + expect(compactedReplay[compactedReplay.length - 1]).toMatchObject({ + type: 'turn_error', + promptId: 'prompt-loop', + data: expect.objectContaining({ + code: 'LOOP_DETECTED', + errorKind: 'loop_detected', + loopType: 'turn_tool_call_cap', + }), + }); + + await bridge.shutdown(); + } finally { + shellSpy.mockRestore(); + } + }); + + it.each(['available_commands_update', 'current_mode_update'] as const)( + 'keeps the turn error on refresh when an idle %s session_update lands after it', + async (subtype) => { + // Latest-wins state snapshots fan out to idle sessions (a workspace + // skills/settings refresh, an approval-mode change). They carry no + // turn content for the persisted transcript and must not defeat the + // refresh-append of the terminal. + const handle = makeChannel({ + promptImpl: () => { + throw new RequestError(-32603, 'Loop protection stopped this turn', { + code: 'LOOP_DETECTED', + errorKind: 'loop_detected', + loopType: 'turn_tool_call_cap', + }); + }, + extMethodImpl: (method, params) => { + if (method !== SERVE_STATUS_EXT_METHODS.sessionTranscript) { + return {}; + } + return { + v: 1, + sessionId: params['sessionId'], + events: [ + { + v: 1, + type: 'session_update', + data: { + sessionUpdate: 'user_message_chunk', + content: { type: 'text', text: 'persisted turn content' }, + _meta: { + 'qwen.session.recordId': `record-loop-${subtype}-page`, + }, + }, + }, + ], + hasMore: false, + }; + }, + }); + const bridge = makeBridge({ channelFactory: async () => handle.channel }); + const session = await bridge.spawnOrAttach({ workspaceCwd: WS_A }); + + await expect( + bridge.sendPrompt( + session.sessionId, + { + sessionId: session.sessionId, + prompt: [{ type: 'text', text: 'loop' }], + }, + undefined, + { promptId: 'prompt-loop' }, + ), + ).rejects.toThrow('Loop protection stopped this turn'); + + const abort = new AbortController(); + const iter = bridge.subscribeEvents(session.sessionId, { + signal: abort.signal, + }); + const sawIdleUpdate = (async () => { + for await (const event of iter) { + if ( + event.type === 'session_update' && + (event.data as { update?: { sessionUpdate?: string } })?.update + ?.sessionUpdate === subtype + ) { + return; + } + } + throw new Error(`${subtype} was not published`); + })(); + await handle.agentConnection.sessionUpdate({ + sessionId: session.sessionId, + update: + subtype === 'available_commands_update' + ? { + sessionUpdate: subtype, + availableCommands: [], + _meta: { availableSkills: [] }, + } + : { sessionUpdate: subtype, currentModeId: 'plan' }, + }); + await sawIdleUpdate; + + const refreshed = await bridge.loadSession({ + sessionId: session.sessionId, + workspaceCwd: WS_A, + clientId: session.clientId, + historyReplay: 'response', + historyPageSize: 100, + }); + + const compactedReplay = refreshed.compactedReplay ?? []; + expect(compactedReplay).toHaveLength(2); + expect(compactedReplay[0]).toMatchObject({ + type: 'session_update', + data: expect.objectContaining({ + content: { type: 'text', text: 'persisted turn content' }, + }), + }); + expect(compactedReplay[compactedReplay.length - 1]).toMatchObject({ + type: 'turn_error', + promptId: 'prompt-loop', + data: expect.objectContaining({ + code: 'LOOP_DETECTED', + errorKind: 'loop_detected', + loopType: 'turn_tool_call_cap', + }), + }); + + abort.abort(); + await bridge.shutdown(); + }, + ); + it('keeps the turn error on refresh when a queued deadline terminal lands after it', async () => { // A queued prompt's terminal publishes the event alone without mutating // turn state; it must not erase the refresh-replay record of the active @@ -5015,13 +5255,16 @@ describe('createAcpSessionBridge', () => { const iter = bridge.subscribeEvents(session.sessionId, { signal: abort.signal, }); + const terminalOrder: string[] = []; const sawQueuedDeadlineTerminal = (async () => { for await (const event of iter) { + if (event.type !== 'turn_error') continue; + if (event.promptId === 'prompt-first') terminalOrder.push('loop'); if ( - event.type === 'turn_error' && event.promptId === 'prompt-queued' && (event.data as { code?: string }).code === 'prompt_deadline_exceeded' ) { + terminalOrder.push('deadline'); return; } } @@ -5073,6 +5316,14 @@ describe('createAcpSessionBridge', () => { await expect(first).rejects.toThrow('Loop protection stopped this turn'); await sawQueuedDeadlineTerminal; + // Pin the scenario's premise — the deadline expires while the prompt is + // still queued, AFTER the loop terminal has landed. The 120 ms budget + // can invert under load; without this the assertions below would stay + // green through the inverted ordering. + expect(terminalOrder.indexOf('loop')).toBeGreaterThanOrEqual(0); + expect(terminalOrder.indexOf('loop')).toBeLessThan( + terminalOrder.lastIndexOf('deadline'), + ); const refreshed = await bridge.loadSession({ sessionId: session.sessionId, @@ -5083,6 +5334,15 @@ describe('createAcpSessionBridge', () => { }); const compactedReplay = refreshed.compactedReplay ?? []; + // Anchor the replay on the persisted page so a regression of the + // bounded-append branch into the in-memory fallback cannot satisfy + // the assertions with an identically shaped replay. + expect(compactedReplay[0]).toMatchObject({ + type: 'session_update', + data: expect.objectContaining({ + content: { type: 'text', text: 'persisted turn content' }, + }), + }); expect( compactedReplay.some( (event) => @@ -5218,6 +5478,15 @@ describe('createAcpSessionBridge', () => { }); const compactedReplay = refreshed.compactedReplay ?? []; + // Anchor the replay on the persisted page so a regression of the + // bounded-append branch into the in-memory fallback cannot satisfy + // the assertions with an identically shaped replay. + expect(compactedReplay[0]).toMatchObject({ + type: 'session_update', + data: expect.objectContaining({ + content: { type: 'text', text: 'persisted turn content' }, + }), + }); expect( compactedReplay.some( (event) => @@ -5234,6 +5503,170 @@ describe('createAcpSessionBridge', () => { await bridge.shutdown(); }); + it('drops the stale turn error when a queued terminal folds newer automatic-turn content', async () => { + // A queued terminal is a turn boundary on the bus: ingesting it folds + // and resets the live journal. When newer turn content was journaled + // after the pending loop terminal, that content supersedes the stale + // error before the fold erases it — otherwise the refresh-append would + // re-place the stale loop error AFTER the newer automatic content, the + // exact misplacement the guard exists to prevent. + let promptCalls = 0; + const heldTurn = deferred(); + let releaseCancel!: () => void; + const heldCancel = new Promise((resolve) => { + releaseCancel = resolve; + }); + const handle = makeChannel({ + promptImpl: () => { + promptCalls += 1; + if (promptCalls === 1) return heldTurn.promise; + return { stopReason: 'end_turn' }; + }, + cancelImpl: () => heldCancel, + extMethodImpl: (method, params) => { + if (method !== SERVE_STATUS_EXT_METHODS.sessionTranscript) { + return {}; + } + return { + v: 1, + sessionId: params['sessionId'], + events: [ + { + v: 1, + type: 'session_update', + data: { + sessionUpdate: 'user_message_chunk', + content: { type: 'text', text: 'persisted turn content' }, + _meta: { + 'qwen.session.recordId': 'record-loop-queued-supersede-page', + }, + }, + }, + ], + hasMore: false, + }; + }, + }); + const bridge = makeBridge({ channelFactory: async () => handle.channel }); + const session = await bridge.spawnOrAttach({ workspaceCwd: WS_A }); + const abort = new AbortController(); + const iter = bridge.subscribeEvents(session.sessionId, { + signal: abort.signal, + }); + // One consumer for both waypoints: two loops over the same iterator + // would race for each event and starve one of the waits. + const sawContentThenQueuedTerminal = (async () => { + let sawContent = false; + for await (const event of iter) { + if ( + !sawContent && + event.type === 'session_update' && + JSON.stringify(event.data).includes('automatic turn content') + ) { + sawContent = true; + continue; + } + if ( + sawContent && + event.type === 'turn_complete' && + event.promptId === 'prompt-queued' && + (event.data as { stopReason?: string }).stopReason === 'cancelled' + ) { + return; + } + } + throw new Error('automatic content or queued terminal not published'); + })(); + + const first = bridge.sendPrompt( + session.sessionId, + { + sessionId: session.sessionId, + prompt: [{ type: 'text', text: 'first' }], + }, + undefined, + { promptId: 'prompt-first' }, + ); + first.catch(() => {}); + await vi.waitFor(() => { + expect(handle.agent.promptCalls).toHaveLength(1); + }); + + const second = bridge.sendPrompt( + session.sessionId, + { + sessionId: session.sessionId, + prompt: [{ type: 'text', text: 'queued' }], + }, + undefined, + { promptId: 'prompt-queued' }, + ); + second.catch(() => {}); + await vi.waitFor(() => { + expect(bridge.getPendingPrompts(session.sessionId)).toHaveLength(2); + }); + + void bridge.cancelSession(session.sessionId).catch(() => {}); + await vi.waitFor(() => { + expect(handle.agent.cancelCalls).toHaveLength(1); + }); + heldTurn.reject( + new RequestError(-32603, 'Loop protection stopped this turn', { + code: 'LOOP_DETECTED', + errorKind: 'loop_detected', + loopType: 'turn_tool_call_cap', + }), + ); + await expect(first).rejects.toThrow('Loop protection stopped this turn'); + + // An automatic turn journals content after the loop terminal; the + // queued terminal that follows folds it. + await handle.agentConnection.sessionUpdate({ + sessionId: session.sessionId, + update: { + sessionUpdate: 'agent_message_chunk', + content: { type: 'text', text: 'automatic turn content' }, + }, + }); + + expect( + bridge.removePendingPrompt(session.sessionId, 'prompt-queued'), + ).toEqual({ removed: true }); + await sawContentThenQueuedTerminal; + + const refreshed = await bridge.loadSession({ + sessionId: session.sessionId, + workspaceCwd: WS_A, + clientId: session.clientId, + historyReplay: 'response', + historyPageSize: 100, + }); + + const compactedReplay = refreshed.compactedReplay ?? []; + expect(compactedReplay[0]).toMatchObject({ + type: 'session_update', + data: expect.objectContaining({ + content: { type: 'text', text: 'persisted turn content' }, + }), + }); + expect( + compactedReplay.some( + (event) => + event.type === 'turn_error' && + (event.data as { errorKind?: string }).errorKind === 'loop_detected', + ), + ).toBe(false); + // The supersede only drops the refresh-replay record; the session + // summary still carries the active turn's failure. + expect(bridge.getSessionSummary(session.sessionId).turnError).toMatchObject( + { code: 'LOOP_DETECTED' }, + ); + + abort.abort(); + releaseCancel(); + await bridge.shutdown(); + }); + it('drops the turn error on refresh after a subsequent successful interactive turn', async () => { // Loop reject, then a successful prompt, then refresh: the newer turn's // terminal supersedes the pending append, so the stale loop error must @@ -10937,6 +11370,45 @@ describe('createAcpSessionBridge', () => { await bridge.shutdown(); }); + it('strips spoofed channel-prompt classification and injects only trusted context', async () => { + // `qwen.channel.prompt` opts a turn out of loop-detected rejection, + // so a forged key must not reach the child; only the authenticated + // channel-worker flag on the trusted context re-arms it. + const handle = makeChannel(); + const bridge = makeBridge({ channelFactory: async () => handle.channel }); + const session = await bridge.spawnOrAttach({ workspaceCwd: WS_A }); + + await bridge.sendPrompt( + session.sessionId, + { + sessionId: session.sessionId, + prompt: [{ type: 'text', text: 'forged channel turn' }], + _meta: { 'qwen.channel.prompt': true }, + } as PromptRequest, + undefined, + { promptId: 'prompt-forged' }, + ); + expect( + handle.agent.promptCalls[0]?._meta?.['qwen.channel.prompt'], + ).toBeUndefined(); + + await bridge.sendPrompt( + session.sessionId, + { + sessionId: session.sessionId, + prompt: [{ type: 'text', text: 'trusted channel turn' }], + _meta: { 'qwen.channel.prompt': true }, + } as PromptRequest, + undefined, + { promptId: 'prompt-trusted', channelPrompt: true }, + ); + expect(handle.agent.promptCalls[1]?._meta?.['qwen.channel.prompt']).toBe( + true, + ); + + await bridge.shutdown(); + }); + it('strips both spoofed retry and continue meta keys from one prompt', async () => { const handle = makeChannel(); const bridge = makeBridge({ channelFactory: async () => handle.channel }); diff --git a/packages/acp-bridge/src/bridge.ts b/packages/acp-bridge/src/bridge.ts index 97ca0bc7a1c..ef418b4e677 100644 --- a/packages/acp-bridge/src/bridge.ts +++ b/packages/acp-bridge/src/bridge.ts @@ -130,6 +130,7 @@ import { type ActiveWorkHeartbeatCapabilityV1, type ActiveWorkHoldCategory, type ActiveWorkSnapshotV1, + CHANNEL_PROMPT_META_KEY, CHANNEL_STARTUP_PROFILE_META_KEY, CHANNEL_STARTUP_PROFILE_VERSION, DAEMON_CHANNEL_DELIVERY_META_KEY, @@ -1081,8 +1082,9 @@ interface SessionEntry { * The journaled `turn_error` event behind `turnError`, when the failed * turn published one. A bounded refresh replays it onto persisted * history so the terminal survives a page refresh; any newer turn - * terminal clears it so a stale error is never re-appended. Not part of - * the session summary. + * terminal — or newer turn content about to be folded by a queued + * terminal boundary — clears it so a stale error is never re-appended + * after newer content. Not part of the session summary. */ turnErrorEvent?: BridgeEvent; retryAllowed: boolean; @@ -1626,9 +1628,12 @@ export function extractErrorCode(err: unknown): string | undefined { * activity — a model switch, an extension refresh, an MCP server change, * a user-shell command — defeats the append and the loop terminal * disappears from the refreshed transcript. `session_update` events are - * turn content except the user-shell output stream, which the guard - * skips via `isUserShellSessionUpdate` (its history goes to the model - * conversation, not the persisted transcript the refresh pages). + * turn content except the idle bookkeeping subtypes skipped via + * `isIdleBookkeepingSessionUpdate`: the user-shell output stream (its + * history goes to the model conversation, not the persisted transcript + * the refresh pages) and the latest-wins state snapshots + * (`available_commands_update`, `current_mode_update`) that settings and + * approval-mode refreshes fan out to idle sessions. */ const REFRESH_APPEND_BOOKKEEPING_EVENT_TYPES = new Set([ 'pending_prompt_added', @@ -1651,24 +1656,46 @@ const REFRESH_APPEND_BOOKKEEPING_EVENT_TYPES = new Set([ ]); /** - * User-shell output is published as `session_update` on the session bus - * while idle, but it carries no turn content for the persisted - * transcript (it is injected into the model conversation history - * instead), so it must not defeat the refresh-append of a pending - * terminal error the way a real turn's `session_update` does. + * `session_update` frames published while idle that carry no turn content + * for the persisted transcript, so they must not defeat the refresh-append + * of a pending terminal error the way a real turn's `session_update` does: + * the user-shell output stream (injected into the model conversation + * history instead of the transcript the refresh pages) and the + * latest-wins state snapshots (`available_commands_update` from a + * skills/settings refresh, the legacy dual-emit `current_mode_update`). */ -function isUserShellSessionUpdate(event: BridgeEvent): boolean { +function isIdleBookkeepingSessionUpdate(event: BridgeEvent): boolean { if (event.type !== 'session_update') return false; const data = event.data; if (!data || typeof data !== 'object' || Array.isArray(data)) return false; const update = (data as Record)['update']; if (!update || typeof update !== 'object' || Array.isArray(update)) return false; - const meta = (update as Record)['_meta']; + const updateRecord = update as Record; + const subtype = updateRecord['sessionUpdate']; + if ( + subtype === 'available_commands_update' || + subtype === 'current_mode_update' + ) { + return true; + } + const meta = updateRecord['_meta']; if (!meta || typeof meta !== 'object' || Array.isArray(meta)) return false; return (meta as Record)['source'] === 'user-shell'; } +/** + * Turn-content test behind the refresh-append guard: everything that is + * neither idle bookkeeping nor the synthetic journal-truncation marker + * (which `liveJournalSnapshot` unshifts without ever ingesting it as + * content) counts as newer turn content. + */ +function isRefreshAppendTurnContent(event: BridgeEvent): boolean { + if (event.type === 'history_truncated') return false; + if (REFRESH_APPEND_BOOKKEEPING_EVENT_TYPES.has(event.type)) return false; + return !isIdleBookkeepingSessionUpdate(event); +} + export function classifyTurnErrorKind( message: string, ): 'model_stream_interrupted' | undefined { @@ -1736,7 +1763,8 @@ function broadcastTurnError( // queued prompt's terminal (mutateTurnState=false) publishes the // event alone and leaves the active turn's refresh-replay record // untouched — the prompt never ran, so its terminal is not a newer - // turn boundary for the replay. + // turn boundary for the replay (but see `publishPromptTerminal`: a + // queued boundary that folds newer turn content supersedes it). entry.turnErrorEvent = published; } } catch { @@ -1789,6 +1817,18 @@ function publishPromptTerminal( // lands here. Queued terminals publish their event alone and must // neither set nor clear session-scoped turn state. const mutateTurnState = pendingEntry.state === 'running'; + if (!mutateTurnState && entry.turnErrorEvent) { + // A queued terminal is still a turn boundary on the bus: ingesting it + // folds and resets the live journal, erasing the guard's only evidence + // of newer turn content journaled since the pending error terminal. + // That content supersedes the stale error, so drop the refresh-replay + // record before the fold — otherwise the append would re-place the + // stale error AFTER the newer content. + const journal = entry.events.liveJournalSnapshot() ?? []; + if (journal.some(isRefreshAppendTurnContent)) { + entry.turnErrorEvent = undefined; + } + } if (terminal.kind === 'complete') { broadcastTurnComplete( entry, @@ -5689,9 +5729,7 @@ export function createAcpSessionBridge(opts: BridgeOptions): AcpSessionBridge { // window), so no history scan is needed. const journal = entry.events.liveJournalSnapshot() ?? []; const hasNewerTurnContent = journal.some( - (event) => - !REFRESH_APPEND_BOOKKEEPING_EVENT_TYPES.has(event.type) && - !isUserShellSessionUpdate(event), + isRefreshAppendTurnContent, ); if (!hasNewerTurnContent) { compactedReplay = [...page.events, turnErrorEvent]; @@ -7662,6 +7700,11 @@ export function createAcpSessionBridge(opts: BridgeOptions): AcpSessionBridge { delete meta[DAEMON_CHANNEL_DELIVERY_META_KEY]; delete meta[DAEMON_PROMPT_DISPLAY_TEXT_META_KEY]; delete meta[DAEMON_MODEL_PROMPT_META_KEY]; + // Channel classification is authenticated channel-worker + // metadata; the daemon prompt route validates the worker + // authorization and re-arms it through the trusted + // `channelPrompt` context flag below. + delete meta[CHANNEL_PROMPT_META_KEY]; if (isRetry) { meta[DAEMON_RETRY_META_KEY] = true; } @@ -7679,6 +7722,9 @@ export function createAcpSessionBridge(opts: BridgeOptions): AcpSessionBridge { if (modelPrompt !== undefined) { meta[DAEMON_MODEL_PROMPT_META_KEY] = modelPrompt; } + if (context?.channelPrompt === true) { + meta[CHANNEL_PROMPT_META_KEY] = true; + } meta[INVOCATION_CONTEXT_META_KEY] = invocationContext; if (Object.keys(meta).length > 0) { copy._meta = meta; diff --git a/packages/acp-bridge/src/bridgeTypes.ts b/packages/acp-bridge/src/bridgeTypes.ts index 0eec4891177..aa4c69ee5b0 100644 --- a/packages/acp-bridge/src/bridgeTypes.ts +++ b/packages/acp-bridge/src/bridgeTypes.ts @@ -687,6 +687,13 @@ export interface BridgeClientRequestContext { modelPrompt?: string; /** User-facing projection supplied by an authenticated channel worker. */ promptDisplayText?: string; + /** + * Trusted channel-turn classification injected by the daemon prompt route + * after validating the channel-worker prompt authorization. Never + * populated from caller-controlled ACP metadata: `sendPrompt` strips the + * wire key from untrusted callers and re-injects it only from this flag. + */ + channelPrompt?: boolean; /** Trusted Channel delivery correlation injected by the daemon prompt * route. Never populated from caller-controlled ACP metadata. */ channelDelivery?: { @@ -728,6 +735,9 @@ export function isValidTrustedModelPrompt(value: unknown): value is string { export const DAEMON_CHANNEL_DELIVERY_META_KEY = 'qwen.daemon.channelDelivery'; export const DAEMON_PROMPT_DISPLAY_TEXT_META_KEY = 'qwen.daemon.promptDisplayText'; +// Wire twin of channel-base's CHANNEL_PROMPT_META_KEY; the packages have no +// dependency path between them, so a cross-package test pins the value. +export const CHANNEL_PROMPT_META_KEY = 'qwen.channel.prompt'; /** * Returned from `recordHeartbeat`. `lastSeenAt` is the server-side diff --git a/packages/channels/base/src/ChannelAgentBridge.ts b/packages/channels/base/src/ChannelAgentBridge.ts index dfa33635cd4..01fff0cb201 100644 --- a/packages/channels/base/src/ChannelAgentBridge.ts +++ b/packages/channels/base/src/ChannelAgentBridge.ts @@ -7,7 +7,9 @@ export const CHANNEL_PROMPT_DISPLAY_TEXT_META_KEY = 'qwen.daemon.promptDisplayText'; export const CHANNEL_PROMPT_AUTHORIZATION_META_KEY = 'qwen.daemon.channelPromptAuthorization'; -// Client-supplied routing hint only; never use it as an authorization boundary. +// Channel-turn classification marker. Trusted-parent metadata: the daemon +// strips it from untrusted callers and honors it only when an authenticated +// channel worker (or a private-parent channel bridge) set it. export const CHANNEL_PROMPT_META_KEY = 'qwen.channel.prompt'; // Private-parent capability handshake with the spawned `qwen --acp` child // (packages/core/src/utils/invocation-context.ts owns the same constants). diff --git a/packages/channels/base/src/DaemonChannelBridge.test.ts b/packages/channels/base/src/DaemonChannelBridge.test.ts index d32e4e89e26..b390585664d 100644 --- a/packages/channels/base/src/DaemonChannelBridge.test.ts +++ b/packages/channels/base/src/DaemonChannelBridge.test.ts @@ -1976,6 +1976,40 @@ describe('DaemonChannelBridge', () => { bridge.stop(); }); + it('presents the prompt authorization even without a display text', async () => { + // The daemon validates the token for the channel-turn classification + // too; a channel prompt without display text still needs it to keep + // its classification (and the loop-rejection opt-out that rides it). + const events = new EventQueue(); + const session = createFakeSession(events); + const bridge = new DaemonChannelBridge({ + cwd: '/repo', + sessionFactory: vi.fn().mockResolvedValue(session), + promptAuthorization: 'worker-token', + }); + + await bridge.start(); + await bridge.newSession('/repo'); + + const promptPromise = bridge.prompt('session-1', 'hello'); + await waitFor(() => expect(session.prompt).toHaveBeenCalledOnce()); + expect(session.prompt).toHaveBeenCalledWith( + { + prompt: [{ type: 'text', text: 'hello' }], + _meta: { + [CHANNEL_PROMPT_META_KEY]: true, + [CHANNEL_PROMPT_AUTHORIZATION_META_KEY]: 'worker-token', + }, + }, + expect.any(AbortSignal), + ); + + events.push(turnCompleteEvent()); + await promptPromise; + events.close(); + bridge.stop(); + }); + it('aborts in-flight prompts when the bridge stops', async () => { const events = new EventQueue(); const session = createFakeSession(events); diff --git a/packages/channels/base/src/DaemonChannelBridge.ts b/packages/channels/base/src/DaemonChannelBridge.ts index b036b533f10..2441b915fed 100644 --- a/packages/channels/base/src/DaemonChannelBridge.ts +++ b/packages/channels/base/src/DaemonChannelBridge.ts @@ -417,10 +417,10 @@ export class DaemonChannelBridge }); } prompt.push({ type: 'text', text }); - const promptAuthorization = - options?.displayText !== undefined - ? this.options.promptAuthorization - : undefined; + // Always presented: the daemon validates it for the channel-turn + // classification as well as the display projection, and channel + // prompts without display text still need the classification. + const promptAuthorization = this.options.promptAuthorization; try { const result = await session.prompt( diff --git a/packages/cli/src/acp-integration/acpAgent.test.ts b/packages/cli/src/acp-integration/acpAgent.test.ts index 3e93d310fcd..a18944c1147 100644 --- a/packages/cli/src/acp-integration/acpAgent.test.ts +++ b/packages/cli/src/acp-integration/acpAgent.test.ts @@ -2148,6 +2148,10 @@ describe('QwenAgent MCP SSE/HTTP support', () => { 'qwen.daemon.modelPrompt': 'trusted model-only prompt', 'qwen.daemon.promptDisplayText': 'trusted display text', 'qwen.channel.prompt': true, + 'qwen.daemon.channelDelivery': { + deliveryId: 'delivery-trusted', + target: { channelName: 'dingtalk', type: 'user', id: 'user-1' }, + }, }, }); @@ -2159,6 +2163,10 @@ describe('QwenAgent MCP SSE/HTTP support', () => { keep: true, 'qwen.daemon.promptDisplayText': 'trusted display text', 'qwen.channel.prompt': true, + 'qwen.daemon.channelDelivery': { + deliveryId: 'delivery-trusted', + target: { channelName: 'dingtalk', type: 'user', id: 'user-1' }, + }, }, }, invocation, @@ -2169,11 +2177,12 @@ describe('QwenAgent MCP SSE/HTTP support', () => { await agentPromise; }); - it('strips channel-prompt classification from untrusted callers', async () => { - // `qwen.channel.prompt` opts a turn out of loop-detected rejection and + it('strips channel classification from untrusted callers', async () => { + // `qwen.channel.prompt` and `qwen.daemon.channelDelivery` both mark a + // turn as a channel turn, opting it out of loop-detected rejection and // the repeated-failure guard. Only trusted parents (the channel bridges - // and the daemon bridge) may set it; an untrusted client marking its own - // prompt as a channel turn must not reach the session. A plain + // and the daemon bridge) may set them; an untrusted client marking its + // own prompt as a channel turn must not reach the session. A plain // `qwen --acp` child has no expected capability and initializes // untrusted. await setupSessionMocks('untrusted-session'); @@ -2197,6 +2206,10 @@ describe('QwenAgent MCP SSE/HTTP support', () => { _meta: { keep: true, 'qwen.channel.prompt': true, + 'qwen.daemon.channelDelivery': { + deliveryId: 'delivery-forged', + target: { channelName: 'dingtalk', type: 'user', id: 'user-1' }, + }, }, }); diff --git a/packages/cli/src/acp-integration/acpAgent.ts b/packages/cli/src/acp-integration/acpAgent.ts index e2a74e9789b..ae3a504e0c7 100644 --- a/packages/cli/src/acp-integration/acpAgent.ts +++ b/packages/cli/src/acp-integration/acpAgent.ts @@ -337,6 +337,7 @@ import { CHANNEL_STARTUP_PROFILE_META_KEY, CHANNEL_STARTUP_PROFILE_VERSION, CLIENT_MCP_OVER_WS_CONFIG_FLAG, + DAEMON_CHANNEL_DELIVERY_META_KEY, DAEMON_MODEL_PROMPT_META_KEY, DAEMON_PROMPT_DISPLAY_TEXT_META_KEY, LOAD_REPLAY_BULK_MODE, @@ -5488,11 +5489,13 @@ class QwenAgent implements Agent { const suppliedModelPrompt = meta[DAEMON_MODEL_PROMPT_META_KEY]; const suppliedPromptDisplayText = meta[DAEMON_PROMPT_DISPLAY_TEXT_META_KEY]; const suppliedChannelPrompt = meta[CHANNEL_PROMPT_META_KEY]; + const suppliedChannelDelivery = meta[DAEMON_CHANNEL_DELIVERY_META_KEY]; delete meta[INVOCATION_CONTEXT_META_KEY]; delete meta[DAEMON_MODEL_PROMPT_META_KEY]; delete meta[PRIVATE_PARENT_CAPABILITY_META_KEY]; delete meta[DAEMON_PROMPT_DISPLAY_TEXT_META_KEY]; delete meta[CHANNEL_PROMPT_META_KEY]; + delete meta[DAEMON_CHANNEL_DELIVERY_META_KEY]; // The user-facing display projection is caller-controlled metadata; honor // it only for trusted parents (the daemon bridge re-injects the trusted // channel-worker value here). A plain delete would drop that re-injection. @@ -5513,6 +5516,15 @@ class QwenAgent implements Agent { ) { meta[CHANNEL_PROMPT_META_KEY] = true; } + // Channel delivery is the second input to the same classification; gate + // it identically so an untrusted caller cannot opt its own turn out of + // loop-detected rejection through the sibling key. + if ( + this.privateParentState === 'trusted' && + suppliedChannelDelivery !== undefined + ) { + meta[DAEMON_CHANNEL_DELIVERY_META_KEY] = suppliedChannelDelivery; + } if (Object.keys(meta).length > 0) { sanitizedParams._meta = meta; } else { diff --git a/packages/cli/src/acp-integration/session/Session.test.ts b/packages/cli/src/acp-integration/session/Session.test.ts index b75d3d8dd87..3b9c34bec1b 100644 --- a/packages/cli/src/acp-integration/session/Session.test.ts +++ b/packages/cli/src/acp-integration/session/Session.test.ts @@ -15236,6 +15236,135 @@ describe('Session', () => { expect(mockGoalRuntime.dispatch).not.toHaveBeenCalled(); }); + it('keeps a Goal turn graceful when the repeated-failure guard stops it', async () => { + // Goal turns keep the configured guard mode (they are not channel + // turns) but get rejectOnLoopDetected=false, so an enforce-mode + // failure streak stops them through the graceful branch: end_turn + // settlement plus the transcript stop message, never a rejection + // that would pause the goal without a published turn_error. + const guardModeEnv = 'QWEN_CODE_ACP_REPEATED_TOOL_FAILURE_GUARD'; + const previousGuardMode = process.env[guardModeEnv]; + process.env[guardModeEnv] = 'enforce'; + try { + session = new Session( + 'test-session-id', + mockConfig, + mockClient, + mockSettings, + ); + mockConfig.getApprovalMode = vi + .fn() + .mockReturnValue(ApprovalMode.YOLO); + mockConfig.getDisableAllHooks = vi.fn().mockReturnValue(true); + const execute = vi.fn().mockResolvedValue({ + llmContent: 'failed', + returnDisplay: 'failed', + error: { + message: 'execution failed', + type: core.ToolErrorType.EXECUTION_FAILED, + }, + }); + mockToolRegistry.getTool.mockReturnValue({ + name: 'failing_tool', + kind: core.Kind.Execute, + displayName: 'Failing Tool', + description: 'Fails during execution', + build: vi.fn().mockReturnValue({ + params: {}, + execute, + getDefaultPermission: vi.fn().mockResolvedValue('allow'), + getDescription: vi.fn().mockReturnValue('Failing Tool'), + toolLocations: vi.fn().mockReturnValue([]), + }), + canUpdateOutput: false, + isOutputMarkdown: true, + }); + const streamForBatch = (batch: number, count: number) => + createStreamWithChunks([ + { + type: core.StreamEventType.CHUNK, + value: { + functionCalls: Array.from({ length: count }, (_, index) => ({ + id: `goal_failure_${batch}_${index}`, + name: 'failing_tool', + args: { attempt: `${batch}_${index}` }, + })), + }, + }, + ]); + mockChat.sendMessageStream = vi + .fn() + .mockResolvedValueOnce(streamForBatch(1, 4)) + .mockResolvedValueOnce(streamForBatch(2, 4)) + .mockResolvedValueOnce(streamForBatch(3, 1)) + .mockResolvedValueOnce(createEmptyStream()); + + const permit: core.GoalTurnPermit = { + goalId: 'goal-1', + revision: 1, + turnId: 'turn-guard-stop', + }; + const turnKey = 'goal-runtime:turn-guard-stop'; + mockGoalRuntime.getSnapshot.mockReturnValue({ + v: 2, + activity: 'running', + goal: { + goalId: 'goal-1', + revision: 1, + objective: 'check weather', + status: 'active', + evidenceCursor: { recordId: 'cursor-1' }, + turnCount: 0, + activeTimeMs: 0, + createdAt: 1234, + updatedAt: 1234, + }, + }); + mockGoalRuntime.permitForTurn.mockImplementation((key: string) => + key === turnKey ? permit : undefined, + ); + + expect(boundGoalHost).toBeDefined(); + await boundGoalHost!.startGoalTurn({ + permit, + continuationContext: 'check weather', + }); + + await vi.waitFor(() => { + expect(logLoopDetectedSpy).toHaveBeenCalledWith( + mockConfig, + expect.objectContaining({ + loop_type: core.LoopType.REPEATED_TOOL_EXECUTION_FAILURE, + }), + { recordToQwenLogger: false }, + ); + }); + // Graceful end_turn settles the iteration; the goal is not paused. + await vi.waitFor(() => { + expect(mockGoalRuntime.finishTurn).toHaveBeenCalledWith(permit); + }); + expect(mockGoalRuntime.dispatch).not.toHaveBeenCalled(); + // The graceful stop keeps the user-visible stop message: it is + // the only explanation of a silently stopped autonomous turn. + expect( + vi.mocked(mockClient.sessionUpdate).mock.calls.some(([params]) => { + const update = params.update; + return ( + update.sessionUpdate === 'agent_message_chunk' && + update.content.type === 'text' && + update.content.text.includes('Automatic continuation stopped') + ); + }), + ).toBe(true); + } finally { + if (previousGuardMode === undefined) { + delete process.env[guardModeEnv]; + } else { + process.env[guardModeEnv] = previousGuardMode; + } + } + }); + it('pauses without counting a Goal turn cancelled before the model request', async () => { // `modelStarted` decides whether settlement records an iteration. // A user cancel still pauses the Goal before that point; releasing diff --git a/packages/cli/src/commands/channel/channel-prompt-wire-key.test.ts b/packages/cli/src/commands/channel/channel-prompt-wire-key.test.ts new file mode 100644 index 00000000000..bba61f35de4 --- /dev/null +++ b/packages/cli/src/commands/channel/channel-prompt-wire-key.test.ts @@ -0,0 +1,13 @@ +import { describe, expect, it } from 'vitest'; +import { CHANNEL_PROMPT_META_KEY } from '@qwen-code/channel-base'; +import { CHANNEL_PROMPT_META_KEY as BRIDGE_CHANNEL_PROMPT_META_KEY } from '@qwen-code/acp-bridge/bridgeTypes'; + +// The channel bridges write the channel-turn classification under the +// channel-base key and the daemon-side strip/re-injection reads it under +// the acp-bridge key; the packages have no dependency path between them, +// so pin the wire contract here where both packages are importable. +describe('channel prompt classification wire key', () => { + it('is identical across channel-base and acp-bridge', () => { + expect(CHANNEL_PROMPT_META_KEY).toBe(BRIDGE_CHANNEL_PROMPT_META_KEY); + }); +}); diff --git a/packages/cli/src/serve/routes/session.ts b/packages/cli/src/serve/routes/session.ts index 8ccd647dd80..0e3b8958f0e 100644 --- a/packages/cli/src/serve/routes/session.ts +++ b/packages/cli/src/serve/routes/session.ts @@ -31,7 +31,10 @@ import { type SessionArchiveState, } from '@qwen-code/qwen-code-core'; import type { SessionArtifactInput } from '@qwen-code/acp-bridge/sessionArtifacts'; -import { DAEMON_PROMPT_DISPLAY_TEXT_META_KEY } from '@qwen-code/acp-bridge/bridgeTypes'; +import { + CHANNEL_PROMPT_META_KEY, + DAEMON_PROMPT_DISPLAY_TEXT_META_KEY, +} from '@qwen-code/acp-bridge/bridgeTypes'; import { parseSessionSource } from '@qwen-code/acp-bridge'; import { isReservedLiveSessionSource, @@ -3367,23 +3370,31 @@ export function registerSessionRoutes( forwardedMeta?.[CHANNEL_WORKER_PROMPT_AUTHORIZATION_META_KEY]; const promptDisplayText = forwardedMeta?.[DAEMON_PROMPT_DISPLAY_TEXT_META_KEY]; + const channelPrompt = forwardedMeta?.[CHANNEL_PROMPT_META_KEY]; if (forwardedMeta) { delete forwardedMeta[CHANNEL_WORKER_PROMPT_AUTHORIZATION_META_KEY]; delete forwardedMeta[DAEMON_PROMPT_DISPLAY_TEXT_META_KEY]; + delete forwardedMeta[CHANNEL_PROMPT_META_KEY]; if (Object.keys(forwardedMeta).length > 0) { forwardedBody['_meta'] = forwardedMeta; } else { delete forwardedBody['_meta']; } } + const channelWorkerAuthorized = isChannelWorkerPromptAuthorized( + promptAuthorization, + runtime.workspaceCwd, + ); const trustedPromptDisplayText = - typeof promptDisplayText === 'string' && - isChannelWorkerPromptAuthorized( - promptAuthorization, - runtime.workspaceCwd, - ) + typeof promptDisplayText === 'string' && channelWorkerAuthorized ? promptDisplayText : undefined; + // Channel classification opts the turn out of loop-detected + // rejection, so it rides the same worker authorization as the + // display projection; a forged key from any other caller is dropped + // here and again at the bridge admission strip. + const trustedChannelPrompt = + channelWorkerAuthorized && channelPrompt === true; const lastEventId = ownerBridge.getSessionLastEventId(sessionId); // Epoch token paired with the cursor above: a client that seeds its @@ -3433,6 +3444,7 @@ export function registerSessionRoutes( ...(trustedPromptDisplayText !== undefined ? { promptDisplayText: trustedPromptDisplayText } : {}), + ...(trustedChannelPrompt ? { channelPrompt: true } : {}), ...(delivery !== undefined ? { channelDelivery: { diff --git a/packages/cli/src/serve/server.test.ts b/packages/cli/src/serve/server.test.ts index 1b241f6eb73..e6fb4a5fe6f 100644 --- a/packages/cli/src/serve/server.test.ts +++ b/packages/cli/src/serve/server.test.ts @@ -44,7 +44,10 @@ import { registerChannelWorkerPromptAuthorization, revokeChannelWorkerPromptAuthorization, } from './channel-worker-prompt-authorization.js'; -import { CHANNEL_PROMPT_DISPLAY_TEXT_META_KEY } from '@qwen-code/channel-base'; +import { + CHANNEL_PROMPT_DISPLAY_TEXT_META_KEY, + CHANNEL_PROMPT_META_KEY, +} from '@qwen-code/channel-base'; import { resolveWebShellDir, isDocumentNavigation, @@ -12531,6 +12534,61 @@ describe('createServeApp', () => { } }); + it('accepts channel-prompt classification only from the workspace worker', async () => { + // `qwen.channel.prompt` opts a turn out of loop-detected rejection; + // a forged key from an unauthorized caller must be dropped at the + // route (and again at the bridge admission strip), never reaching + // the trusted prompt context. + const bridge = fakeBridge(); + const app = createServeApp(baseOpts, undefined, { bridge }); + const workspace = realpathSync(process.cwd()); + const token = 'channel-worker-classification-token'; + registerChannelWorkerPromptAuthorization(token, workspace); + try { + const forged = await request(app) + .post('/session/session-A/prompt') + .set('Host', `127.0.0.1:${baseOpts.port}`) + .send({ + prompt: [{ type: 'text', text: 'hi' }], + _meta: { [CHANNEL_PROMPT_META_KEY]: true }, + }); + const forgedWithBadToken = await request(app) + .post('/session/session-A/prompt') + .set('Host', `127.0.0.1:${baseOpts.port}`) + .send({ + prompt: [{ type: 'text', text: 'hi' }], + _meta: { + [CHANNEL_WORKER_PROMPT_AUTHORIZATION_META_KEY]: 'forged', + [CHANNEL_PROMPT_META_KEY]: true, + }, + }); + const trusted = await request(app) + .post('/session/session-A/prompt') + .set('Host', `127.0.0.1:${baseOpts.port}`) + .send({ + prompt: [{ type: 'text', text: 'hi' }], + _meta: { + [CHANNEL_WORKER_PROMPT_AUTHORIZATION_META_KEY]: token, + [CHANNEL_PROMPT_META_KEY]: true, + }, + }); + + expect(forged.status).toBe(202); + expect(forgedWithBadToken.status).toBe(202); + expect(trusted.status).toBe(202); + expect(bridge.promptCalls[0]?.context?.channelPrompt).toBeUndefined(); + expect(bridge.promptCalls[1]?.context?.channelPrompt).toBeUndefined(); + expect(bridge.promptCalls[2]?.context?.channelPrompt).toBe(true); + for (const call of bridge.promptCalls) { + expect(call.req._meta ?? {}).not.toHaveProperty( + CHANNEL_PROMPT_META_KEY, + ); + } + } finally { + revokeChannelWorkerPromptAuthorization(token); + } + }); + it('validates delivery and forwards it only through trusted prompt context', async () => { const bridge = fakeBridge(); const channelDeliveryAuthorizations = diff --git a/packages/web-shell/client/App.test.tsx b/packages/web-shell/client/App.test.tsx index 2a84c675d02..0f960c9e3c5 100644 --- a/packages/web-shell/client/App.test.tsx +++ b/packages/web-shell/client/App.test.tsx @@ -12289,6 +12289,52 @@ describe('App session callbacks', () => { expect(container.querySelector('[data-testid="retry"]')).toBeNull(); }); + it('reports the turn error through turn_complete across a trailing background notification', async () => { + // turn_complete and the retry decision read the same backward walk, so + // a background-notification user block after the turn error must not + // hide the error from the host while the UI still offers retry. + const onSessionChange = vi.fn(); + const { container, rerender } = renderApp({ onSessionChange }); + await flush(); + + testState.prompt = 'interrupt this stream'; + await clickSubmit(container); + onSessionChange.mockClear(); + + act(() => { + testState.streamingState = 'responding'; + rerender({ onSessionChange }); + }); + act(() => { + testState.blocks = [ + { + kind: 'error', + source: 'turn_error', + id: 'turn-error-with-notification', + errorKind: 'model_stream_interrupted', + text: 'terminated', + }, + { + id: 'background-1', + kind: 'user', + text: 'Background task completed', + meta: { source: 'background_notification' }, + }, + ]; + testState.streamingState = 'idle'; + rerender({ onSessionChange }); + }); + + expect(onSessionChange).toHaveBeenCalledWith({ + type: 'turn_complete', + sessionId: 'session-1', + error: expect.objectContaining({ + message: 'Turn error (block turn-error-with-notification)', + }), + }); + expect(container.querySelector('[data-testid="retry"]')).not.toBeNull(); + }); + it.each([ ['a fresh prompt id', 'prompt-2'], ['a reused prompt id', 'prompt-1'], diff --git a/packages/web-shell/client/App.tsx b/packages/web-shell/client/App.tsx index 7514bdf1ee7..81b48ee3cf2 100644 --- a/packages/web-shell/client/App.tsx +++ b/packages/web-shell/client/App.tsx @@ -7092,16 +7092,6 @@ export function App({ ]); useEffect(() => { - let turnErrorId: string | null = null; - for (let i = blocks.length - 1; i >= 0; i--) { - const block = blocks[i]; - if (block?.kind === 'user') break; - if (block?.kind === 'error' && block.source === 'turn_error') { - turnErrorId = block.id; - break; - } - if (block?.kind !== 'debug') break; - } const lastTurnError = getRetryableTurnError(blocks); // Loop-detected turn errors still surface through turn_complete below, // but resubmitting a prompt the daemon stopped for loop protection @@ -7128,7 +7118,10 @@ export function App({ ) { retriedTurnErrorIdRef.current = retryableTurnError.id; } - lastTurnErrorIdRef.current = turnErrorId; + // Same walk as the retry decision above, so turn_complete and the + // retry affordance never disagree about whether the current turn has + // a turn error (e.g. across a trailing background notification). + lastTurnErrorIdRef.current = lastTurnError?.id ?? null; const canRetry = connected && retryableTurnError !== undefined && From a47e0b53fda36029476d4b6c609402e8d1f89ee5 Mon Sep 17 00:00:00 2001 From: qwen-code-dev-bot Date: Thu, 13 Aug 2026 22:11:44 +0000 Subject: [PATCH 09/10] fix(acp): close loop-protection bypasses in delivery classification and retry re-arm (#8853) --- .../web-shell-loop-detection-turn-error.md | 2 +- .../cli/src/acp-integration/acpAgent.test.ts | 14 +- packages/cli/src/acp-integration/acpAgent.ts | 6 +- .../acp-integration/session/Session.test.ts | 72 ++--- .../src/acp-integration/session/Session.ts | 26 +- packages/web-shell/client/App.test.tsx | 271 ++++++++++++++++++ packages/web-shell/client/App.tsx | 36 ++- 7 files changed, 359 insertions(+), 68 deletions(-) diff --git a/docs/design/web-shell-loop-detection-turn-error.md b/docs/design/web-shell-loop-detection-turn-error.md index 576c4e2b6c3..a7dc61d018f 100644 --- a/docs/design/web-shell-loop-detection-turn-error.md +++ b/docs/design/web-shell-loop-detection-turn-error.md @@ -12,7 +12,7 @@ Web Shell renders `loop_detected` from the structured kind, using localized plai Skipped tools keep their existing failed terminal update and error details so they cannot remain pending and their display behavior does not change. The additional `turn_error` provides the user-facing explanation for the stopped turn. -The session remains alive and the per-turn loop state is recreated for the next prompt. Cron, background-notification, channel-delivery, and goal turns keep their existing non-interactive handling: only interactive foreground prompts reject. Goal turns bypass the bridge entirely, so rejecting one would settle it as failed and pause the goal without publishing any `turn_error`; they resolve `end_turn` like the other automatic turn types. A loop-detected rejection still drains the cron/notification queues, preserving the invariant that a loop-stopped turn never strands queued automatic work. +The session remains alive and the per-turn loop state is recreated for the next prompt. Cron, background-notification, channel-classified, and goal turns keep their existing non-interactive handling: only interactive foreground prompts reject. Channel classification comes from the authenticated channel-prompt marker alone; the caller-requested delivery meta still schedules the delivery but keeps the foreground rejection, so it cannot opt a turn out of loop protection. Goal turns bypass the bridge entirely, so rejecting one would settle it as failed and pause the goal without publishing any `turn_error`; they resolve `end_turn` like the other automatic turn types. A loop-detected rejection still drains the cron/notification queues, preserving the invariant that a loop-stopped turn never strands queued automatic work. When Web Shell reloads a live session from paginated persisted history, the bridge appends the current in-memory `turn_error` to that replay. This keeps the terminal error visible across a page refresh while the session remains idle; newer turn content — including automatic turns the rejection itself drains — supersedes it by design. diff --git a/packages/cli/src/acp-integration/acpAgent.test.ts b/packages/cli/src/acp-integration/acpAgent.test.ts index a18944c1147..f1ee63d2ea7 100644 --- a/packages/cli/src/acp-integration/acpAgent.test.ts +++ b/packages/cli/src/acp-integration/acpAgent.test.ts @@ -2178,13 +2178,13 @@ describe('QwenAgent MCP SSE/HTTP support', () => { }); it('strips channel classification from untrusted callers', async () => { - // `qwen.channel.prompt` and `qwen.daemon.channelDelivery` both mark a - // turn as a channel turn, opting it out of loop-detected rejection and - // the repeated-failure guard. Only trusted parents (the channel bridges - // and the daemon bridge) may set them; an untrusted client marking its - // own prompt as a channel turn must not reach the session. A plain - // `qwen --acp` child has no expected capability and initializes - // untrusted. + // `qwen.channel.prompt` marks a turn as a channel turn, opting it out + // of loop-detected rejection and the repeated-failure guard, and + // `qwen.daemon.channelDelivery` schedules the response delivery. Only + // trusted parents (the channel bridges and the daemon bridge) may set + // them; an untrusted client marking its own prompt must not reach the + // session. A plain `qwen --acp` child has no expected capability and + // initializes untrusted. await setupSessionMocks('untrusted-session'); const agentPromise = runAcpAgent( mockConfig, diff --git a/packages/cli/src/acp-integration/acpAgent.ts b/packages/cli/src/acp-integration/acpAgent.ts index ae3a504e0c7..35be4f05293 100644 --- a/packages/cli/src/acp-integration/acpAgent.ts +++ b/packages/cli/src/acp-integration/acpAgent.ts @@ -5516,9 +5516,9 @@ class QwenAgent implements Agent { ) { meta[CHANNEL_PROMPT_META_KEY] = true; } - // Channel delivery is the second input to the same classification; gate - // it identically so an untrusted caller cannot opt its own turn out of - // loop-detected rejection through the sibling key. + // Channel delivery is a daemon-managed side effect (the prompt route + // injects it from the trusted context); an untrusted direct-ACP caller + // must not self-schedule its own response delivery through the key. if ( this.privateParentState === 'trusted' && suppliedChannelDelivery !== undefined diff --git a/packages/cli/src/acp-integration/session/Session.test.ts b/packages/cli/src/acp-integration/session/Session.test.ts index 3b9c34bec1b..fe046f17a22 100644 --- a/packages/cli/src/acp-integration/session/Session.test.ts +++ b/packages/cli/src/acp-integration/session/Session.test.ts @@ -8782,11 +8782,12 @@ describe('Session', () => { } }); - it('forces channel-delivery prompts off even when enforcement is configured', async () => { - // One server-side channel classification gates both the rejection - // and the guard mode: channelDelivery turns are non-interactive - // deliveries, so the repeated-failure guard must not stop them - // either — they keep running to their natural end. + it('keeps the configured guard for delivery-marked prompts', async () => { + // The delivery meta is a caller-requested side effect, not a + // channel classification: it schedules the delivery on end_turn + // but must not opt the turn out of the repeated-failure guard or + // loop-detected rejection, or any caller could bypass loop + // protection by marking its own prompt for delivery. recreateSessionWithGuardMode('enforce'); try { const execute = installFailingTool(); @@ -8807,21 +8808,19 @@ describe('Session', () => { }, }, }), - ).resolves.toEqual({ stopReason: 'end_turn' }); + ).rejects.toMatchObject({ + data: expect.objectContaining({ + code: 'LOOP_DETECTED', + loopType: core.LoopType.REPEATED_TOOL_EXECUTION_FAILURE, + }), + }); expect(execute).toHaveBeenCalledTimes(9); - expect(mockChat.sendMessageStream).toHaveBeenCalledTimes(4); - expect(logRepeatedToolFailureGuardSpy).not.toHaveBeenCalled(); - // Drain the scheduled channel delivery before teardown so the - // unref'd delivery timer cannot fire after the mocks reset. - await vi.waitFor(() => { - expect(mockClient.extMethod).toHaveBeenCalledWith( - 'qwen/control/channel-delivery', - expect.objectContaining({ - deliveryId: 'prompt-guard-off-delivery', - }), - ); - }); + expect(mockChat.sendMessageStream).toHaveBeenCalledTimes(3); + expect(mockClient.extMethod).not.toHaveBeenCalledWith( + 'qwen/control/channel-delivery', + expect.anything(), + ); } finally { restoreGuardMode(); } @@ -11818,11 +11817,11 @@ describe('Session', () => { }); }); - it('keeps a channel turn graceful when loop protection stops it', async () => { - // Channel turns are non-interactive deliveries: like cron and - // background-notification turns they keep the graceful end-turn so - // the collected response text is still delivered instead of the - // prompt rejecting. + it('rejects a delivery-marked turn when loop protection stops it', async () => { + // The delivery meta alone does not classify a turn as a channel + // turn: the loop-detected stop rejects like any foreground prompt + // instead of resolving end_turn, and the failed turn schedules no + // delivery. mockConfig.getApprovalMode = vi.fn().mockReturnValue(ApprovalMode.YOLO); mockConfig.getMaxToolCallsPerTurn = vi.fn().mockReturnValue(1); mockConfig.isMaxToolCallsPerTurnExplicit = vi @@ -11865,7 +11864,12 @@ describe('Session', () => { }, }, }), - ).resolves.toEqual({ stopReason: 'end_turn' }); + ).rejects.toMatchObject({ + data: expect.objectContaining({ + code: 'LOOP_DETECTED', + loopType: core.LoopType.TURN_TOOL_CALL_CAP, + }), + }); expect(logLoopDetectedSpy).toHaveBeenCalledWith( mockConfig, @@ -11874,22 +11878,18 @@ describe('Session', () => { }), {}, ); - await vi.waitFor(() => { - expect(mockClient.extMethod).toHaveBeenCalledWith( - 'qwen/control/channel-delivery', - expect.objectContaining({ - deliveryId: 'prompt-loop-channel', - }), - ); - }); + expect(mockClient.extMethod).not.toHaveBeenCalledWith( + 'qwen/control/channel-delivery', + expect.anything(), + ); }); it('keeps a channel-prompt-meta turn graceful when loop protection stops it', async () => { // DaemonChannelBridge/AcpBridge channel tasks prompt with - // CHANNEL_PROMPT_META_KEY and no channelDelivery meta; like the - // channelDelivery path above they must resolve end_turn so the - // bridge emits promptComplete with the collected response text - // instead of the rejection failing the non-interactive task. + // CHANNEL_PROMPT_META_KEY; the authenticated classification must + // resolve end_turn so the bridge emits promptComplete with the + // collected response text instead of the rejection failing the + // non-interactive task. mockConfig.getApprovalMode = vi.fn().mockReturnValue(ApprovalMode.YOLO); mockConfig.getMaxToolCallsPerTurn = vi.fn().mockReturnValue(1); mockConfig.isMaxToolCallsPerTurnExplicit = vi diff --git a/packages/cli/src/acp-integration/session/Session.ts b/packages/cli/src/acp-integration/session/Session.ts index 2bab1c173c0..841d76ad57f 100644 --- a/packages/cli/src/acp-integration/session/Session.ts +++ b/packages/cli/src/acp-integration/session/Session.ts @@ -3671,9 +3671,13 @@ export class Session implements SessionContext { ] === true; // One server-side channel classification, consumed by both the // rejection gate below and the guard-mode selection in - // #executePromptInner. The ACP boundary strips the channel-prompt key - // from untrusted callers, so both decisions see only trusted values. - const channelTurn = channelDelivery !== undefined || channelPromptTurn; + // #executePromptInner. Only the authenticated channel-prompt marker + // classifies a turn: the delivery meta is a caller-requested side + // effect (the response is still delivered on end_turn below), and + // letting it classify would let any caller opt its own turn out of + // loop-detected rejection and the repeated-failure guard. The ACP + // boundary strips the channel-prompt key from untrusted callers, so + // both decisions see only trusted values. // Track this prompt's completion for the next prompt to await let resolveCompletion!: () => void; @@ -3694,15 +3698,15 @@ export class Session implements SessionContext { // Channel turns are non-interactive deliveries: like cron, // background-notification, and goal turns they keep the graceful // end-turn handling so the collected response text is still - // delivered. Both channel mechanisms qualify — the channelDelivery - // meta and the CHANNEL_PROMPT_META_KEY turns sent by the channel - // bridges, which carry no channelDelivery capture. Goal turns - // bypass the bridge entirely, so a rejection there would settle - // the turn as failed and pause the goal without any turn_error - // ever being published. - !channelTurn && goalTurn === undefined, + // delivered. Only the authenticated CHANNEL_PROMPT_META_KEY turns + // sent by the channel bridges qualify; the delivery meta alone + // schedules the delivery but keeps the foreground rejection. Goal + // turns bypass the bridge entirely, so a rejection there would + // settle the turn as failed and pause the goal without any + // turn_error ever being published. + !channelPromptTurn && goalTurn === undefined, goalTurn, - channelTurn, + channelPromptTurn, ); promptResult = result; releasePendingSend(); diff --git a/packages/web-shell/client/App.test.tsx b/packages/web-shell/client/App.test.tsx index 0f960c9e3c5..ada74b33b54 100644 --- a/packages/web-shell/client/App.test.tsx +++ b/packages/web-shell/client/App.test.tsx @@ -12335,6 +12335,277 @@ describe('App session callbacks', () => { expect(container.querySelector('[data-testid="retry"]')).not.toBeNull(); }); + it('does not rearm a retry when the retried turn is loop-stopped', async () => { + // When the retried turn itself is stopped by loop protection, the + // catch path must not arm retry state on the loop error: Ctrl+Y + // calls handleRetry() directly even while the retry button is + // hidden, and resubmitting the stopped prompt tends to re-loop. + const retrySend = deferred(); + mockSessionActions.sendPrompt + .mockResolvedValueOnce(undefined) + .mockReturnValueOnce(retrySend.promise); + const { container, rerender } = renderApp(); + await flush(); + + testState.prompt = 'repeat this'; + await clickSubmit(container); + act(() => { + testState.blocks = [ + { + kind: 'error', + source: 'turn_error', + id: 'turn-error-1', + promptId: 'prompt-1', + }, + ]; + rerender(); + }); + expect(container.querySelector('[data-testid="retry"]')).not.toBeNull(); + + act(() => { + container + .querySelector('[data-testid="retry"]') + ?.click(); + }); + await vi.waitFor(() => { + expect(mockSessionActions.sendPrompt).toHaveBeenCalledTimes(2); + }); + const retryOptions = mockSessionActions.sendPrompt.mock.calls[1]?.[1]; + + // The loop turn_error lands before the rejection settles, so the + // catch walk already sees it when the re-arm runs. + act(() => { + testState.blocks = [ + { + kind: 'error', + source: 'turn_error', + id: 'turn-error-1', + promptId: 'prompt-1', + }, + { + kind: 'error', + source: 'turn_error', + id: 'turn-error-loop', + promptId: 'prompt-2', + errorKind: 'loop_detected', + text: 'internal fallback', + }, + ]; + rerender(); + }); + act(() => { + retryOptions?.onAdmissionStarted?.(); + retryOptions?.onAdmitted?.(); + }); + + await act(async () => { + retrySend.reject( + Object.assign(new Error('loop protection stopped the turn'), { + _daemonTurnError: true, + body: 'LOOP_DETECTED', + }), + ); + await Promise.resolve(); + }); + await flush(); + + expect(container.querySelector('[data-testid="retry"]')).toBeNull(); + await act(async () => { + window.dispatchEvent( + new KeyboardEvent('keydown', { key: 'y', ctrlKey: true }), + ); + await Promise.resolve(); + }); + expect(mockSessionActions.sendPrompt).toHaveBeenCalledTimes(2); + }); + + it('does not reoffer a loop-stopped retry to a later unrelated turn error', async () => { + // The rejection settles before the loop turn_error block commits + // (microtask vs transcript flush), so the catch walk still sees the + // original error. The stashed prompt must not survive the loop stop + // and be consumed by a later unrelated retryable turn error, which + // would resubmit the loop-stopped prompt misattributed to a turn + // the user never submitted. + const retrySend = deferred(); + mockSessionActions.sendPrompt + .mockResolvedValueOnce(undefined) + .mockReturnValueOnce(retrySend.promise) + .mockResolvedValueOnce(undefined); + const { container, rerender } = renderApp(); + await flush(); + + testState.prompt = 'repeat this'; + await clickSubmit(container); + act(() => { + testState.blocks = [ + { + kind: 'error', + source: 'turn_error', + id: 'turn-error-1', + promptId: 'prompt-1', + }, + ]; + rerender(); + }); + expect(container.querySelector('[data-testid="retry"]')).not.toBeNull(); + + act(() => { + container + .querySelector('[data-testid="retry"]') + ?.click(); + }); + await vi.waitFor(() => { + expect(mockSessionActions.sendPrompt).toHaveBeenCalledTimes(2); + }); + const retryOptions = mockSessionActions.sendPrompt.mock.calls[1]?.[1]; + act(() => { + retryOptions?.onAdmissionStarted?.(); + retryOptions?.onAdmitted?.(); + }); + + await act(async () => { + retrySend.reject( + Object.assign(new Error('loop protection stopped the turn'), { + _daemonTurnError: true, + body: 'LOOP_DETECTED', + }), + ); + await Promise.resolve(); + }); + await flush(); + + act(() => { + testState.blocks = [ + { + kind: 'error', + source: 'turn_error', + id: 'turn-error-1', + promptId: 'prompt-1', + }, + { + kind: 'error', + source: 'turn_error', + id: 'turn-error-loop', + promptId: 'prompt-2', + errorKind: 'loop_detected', + text: 'internal fallback', + }, + ]; + rerender(); + }); + await flush(); + expect(container.querySelector('[data-testid="retry"]')).toBeNull(); + + act(() => { + testState.blocks = [ + { + kind: 'error', + source: 'turn_error', + id: 'turn-error-1', + promptId: 'prompt-1', + }, + { + kind: 'error', + source: 'turn_error', + id: 'turn-error-loop', + promptId: 'prompt-2', + errorKind: 'loop_detected', + text: 'internal fallback', + }, + { + kind: 'error', + source: 'turn_error', + id: 'turn-error-3', + promptId: 'prompt-3', + }, + ]; + rerender(); + }); + await flush(); + + expect(container.querySelector('[data-testid="retry"]')).toBeNull(); + await act(async () => { + window.dispatchEvent( + new KeyboardEvent('keydown', { key: 'y', ctrlKey: true }), + ); + await Promise.resolve(); + }); + expect(mockSessionActions.sendPrompt).toHaveBeenCalledTimes(2); + }); + + it('does not report the previous turn error again when a retry settles without content', async () => { + // The retry turn settles while the transcript still ends with the + // original turn error (settle precedes the transcript flush); the + // turn_complete for that turn must not re-report the error the user + // already retried. + const onSessionChange = vi.fn(); + const retrySend = deferred(); + mockSessionActions.sendPrompt + .mockResolvedValueOnce(undefined) + .mockReturnValueOnce(retrySend.promise); + const { container, rerender } = renderApp({ onSessionChange }); + await flush(); + + testState.prompt = 'recover this stream'; + await clickSubmit(container); + onSessionChange.mockClear(); + + act(() => { + testState.streamingState = 'responding'; + rerender({ onSessionChange }); + }); + act(() => { + testState.blocks = [ + { + kind: 'error', + source: 'turn_error', + id: 'turn-error-1', + promptId: 'prompt-1', + }, + ]; + testState.streamingState = 'idle'; + rerender({ onSessionChange }); + }); + expect(onSessionChange).toHaveBeenCalledWith({ + type: 'turn_complete', + sessionId: 'session-1', + error: expect.objectContaining({ + message: 'Turn error (block turn-error-1)', + }), + }); + expect(container.querySelector('[data-testid="retry"]')).not.toBeNull(); + + act(() => { + container + .querySelector('[data-testid="retry"]') + ?.click(); + }); + await vi.waitFor(() => { + expect(mockSessionActions.sendPrompt).toHaveBeenCalledTimes(2); + }); + await act(async () => { + retrySend.resolve(); + await Promise.resolve(); + }); + await flush(); + + act(() => { + testState.streamingState = 'responding'; + rerender({ onSessionChange }); + }); + onSessionChange.mockClear(); + act(() => { + testState.streamingState = 'idle'; + rerender({ onSessionChange }); + }); + + expect(onSessionChange).toHaveBeenCalledWith({ + type: 'turn_complete', + sessionId: 'session-1', + error: undefined, + }); + }); + it.each([ ['a fresh prompt id', 'prompt-2'], ['a reused prompt id', 'prompt-1'], diff --git a/packages/web-shell/client/App.tsx b/packages/web-shell/client/App.tsx index 81b48ee3cf2..6529c402dd5 100644 --- a/packages/web-shell/client/App.tsx +++ b/packages/web-shell/client/App.tsx @@ -7120,8 +7120,14 @@ export function App({ } // Same walk as the retry decision above, so turn_complete and the // retry affordance never disagree about whether the current turn has - // a turn error (e.g. across a trailing background notification). - lastTurnErrorIdRef.current = lastTurnError?.id ?? null; + // a turn error (e.g. across a trailing background notification). An + // error the user already retried stays suppressed, mirroring the + // retry affordance; loop-detected errors are never retried, so they + // always surface. + lastTurnErrorIdRef.current = + lastTurnError && lastTurnError.id !== retriedTurnErrorIdRef.current + ? lastTurnError.id + : null; const canRetry = connected && retryableTurnError !== undefined && @@ -9903,17 +9909,27 @@ export function App({ }); } if (isDaemonTurnError(error)) { - failedTurnErrorRetryRef.current = { - errorId: retryErrorId, - text: retryText, - images: retryImages, - inputAnnotations: retryInputAnnotations, - owner: retryOwner, - }; + // A loop-detected rejection ends the retry lineage: the + // retried turn itself was stopped for loop protection, so + // the stashed prompt must not be re-offered — resubmitting + // it tends to re-loop. + if (error.body !== 'LOOP_DETECTED') { + failedTurnErrorRetryRef.current = { + errorId: retryErrorId, + text: retryText, + images: retryImages, + inputAnnotations: retryInputAnnotations, + owner: retryOwner, + }; + } const nextTurnError = getRetryableTurnError( store.getSnapshot().blocks, ); - if (nextTurnError) { + if ( + nextTurnError && + nextTurnError.kind === 'error' && + isRetryableTurnErrorKind(nextTurnError.errorKind) + ) { rearmFailedTurnErrorRetry( nextTurnError, store.getSnapshot().blocks, From c8fe62940f9d9c03e5034c3b6517034326fb7ff4 Mon Sep 17 00:00:00 2001 From: qwen-code-dev-bot Date: Fri, 14 Aug 2026 03:09:47 +0000 Subject: [PATCH 10/10] fix(acp): cover idle workspace fan-out events in the refresh-append allowlist (#8853) Co-authored-by: Qwen-Coder --- packages/acp-bridge/src/bridge.test.ts | 37 ++++++++++++++++++++++++++ packages/acp-bridge/src/bridge.ts | 21 +++++++++++++++ 2 files changed, 58 insertions(+) diff --git a/packages/acp-bridge/src/bridge.test.ts b/packages/acp-bridge/src/bridge.test.ts index 01d3f83a13b..15a528178f6 100644 --- a/packages/acp-bridge/src/bridge.test.ts +++ b/packages/acp-bridge/src/bridge.test.ts @@ -4641,6 +4641,22 @@ describe('createAcpSessionBridge', () => { 'approval_mode_changed', 'model_switched', 'prompt_cancelled', + 'tool_toggled', + 'workspace_initialized', + 'mcp_server_restarted', + 'mcp_server_restart_refused', + 'settings_reloaded', + 'trust_change_requested', + 'memory_changed', + 'agent_changed', + 'git_status_changed', + 'git_branch_changed', + 'github_setup_completed', + 'auth_device_flow_started', + 'auth_device_flow_throttled', + 'auth_device_flow_authorized', + 'auth_device_flow_failed', + 'auth_device_flow_cancelled', ] as const)( 'keeps the turn error on refresh when %s bookkeeping lands after it', async (bookkeepingType) => { @@ -4819,6 +4835,27 @@ describe('createAcpSessionBridge', () => { }); } else if (bookkeepingType === 'session_cwd_changed') { await bridge.changeSessionCwd(session.sessionId, { path: WS_B }); + } else if ( + bookkeepingType.startsWith('auth_device_flow_') || + bookkeepingType === 'tool_toggled' || + bookkeepingType === 'workspace_initialized' || + bookkeepingType === 'mcp_server_restarted' || + bookkeepingType === 'mcp_server_restart_refused' || + bookkeepingType === 'settings_reloaded' || + bookkeepingType === 'trust_change_requested' || + bookkeepingType === 'memory_changed' || + bookkeepingType === 'agent_changed' || + bookkeepingType === 'git_status_changed' || + bookkeepingType === 'git_branch_changed' || + bookkeepingType === 'github_setup_completed' + ) { + // The workspace service, git watcher, memory / agent CRUD, and the + // device-flow registry publish these through the workspace fan-out, + // not a session-scoped bridge method. + bridge.publishWorkspaceEvent({ + type: bookkeepingType, + data: { workspaceId: 'loop-bookkeeping-workspace' }, + }); } else { await bridge.addSessionArtifact( session.sessionId, diff --git a/packages/acp-bridge/src/bridge.ts b/packages/acp-bridge/src/bridge.ts index 9a5334f71f8..4ddaf905d15 100644 --- a/packages/acp-bridge/src/bridge.ts +++ b/packages/acp-bridge/src/bridge.ts @@ -1663,6 +1663,27 @@ const REFRESH_APPEND_BOOKKEEPING_EVENT_TYPES = new Set([ 'mcp_server_removed', 'user_shell_command', 'user_shell_result', + // Workspace-level fan-out (workspace service, git watcher, memory / + // agent CRUD, device-flow registry) reaches every session bus via + // `publishWorkspaceEvent` while idle. The `auth_device_flow_*` members + // mirror the closed `DeviceFlowEventEmission` union — audit that union + // when it grows. + 'tool_toggled', + 'workspace_initialized', + 'mcp_server_restarted', + 'mcp_server_restart_refused', + 'settings_reloaded', + 'trust_change_requested', + 'memory_changed', + 'agent_changed', + 'git_status_changed', + 'git_branch_changed', + 'github_setup_completed', + 'auth_device_flow_started', + 'auth_device_flow_throttled', + 'auth_device_flow_authorized', + 'auth_device_flow_failed', + 'auth_device_flow_cancelled', ]); /**