From a577e6e2bf54085a149217fb3422ad31f0d32061 Mon Sep 17 00:00:00 2001 From: "jinjing.zzj" Date: Thu, 3 Sep 2026 19:45:13 +0800 Subject: [PATCH 01/10] fix(core): halt turns on repeated identical tool errors (issue #10887) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Production sessions burned 5-14M tokens in dead-end loops: the model kept re-running failing operations with varied arguments (every (tool, args) pair unique) while the same error returned on every call — e.g. exit 128 / permission denied on every attempt, 83% of 153 calls erroring. No existing detector inspects tool results (except the task_list fingerprinting), so the identical error class never accumulated: argument-based repetition never triggers on varied args, interleaved successful reads reset the stagnation detectors, and 153 calls stayed far below the 1000-call backstop. Add an always-on error-signature guard to LoopDetectionService: recordToolResult (and recordToolResultByCallId, including unpaired callIds) now fingerprints the `functionResponse.response.error` payload of failed results and halts the turn via the existing LoopDetected path after 3 consecutive error results carry the same signature. Successful results neither advance nor reset the streak (interleaved reads must not mask a dead end); a different error signature restarts it. Oversized error messages reuse the existing persistence-stub normalization so identical underlying errors fingerprint identically. Out of scope: the per-session token budget (issue suggestion 2) is a larger product decision, not part of this fix. Co-authored-by: Qwen-Coder Patrol-Run: qwen-issue-patrol/jmtlf4og3fn --- packages/cli/src/nonInteractiveCli.ts | 5 +- .../src/services/loopDetectionService.test.ts | 138 ++++++++++++++++++ .../core/src/services/loopDetectionService.ts | 104 ++++++++++++- packages/core/src/telemetry/types.ts | 2 + 4 files changed, 246 insertions(+), 3 deletions(-) diff --git a/packages/cli/src/nonInteractiveCli.ts b/packages/cli/src/nonInteractiveCli.ts index f5adcaffb13..59abe798373 100644 --- a/packages/cli/src/nonInteractiveCli.ts +++ b/packages/cli/src/nonInteractiveCli.ts @@ -194,6 +194,8 @@ const LOOP_TYPE_LABELS: Record = { 'the model repeatedly sent invalid tool parameters without correcting them', [LoopType.REPEATED_TOOL_EXECUTION_FAILURE]: 'the same tool execution failure continued after a corrective reminder', + [LoopType.REPEATED_TOOL_ERROR]: + 'the model kept receiving the same tool error without making progress', }; function formatLoopDetectedMessage(loopType: LoopType | undefined): string { @@ -208,7 +210,8 @@ function formatLoopDetectedMessage(loopType: LoopType | undefined): string { loopType === LoopType.SHELL_COMMAND_STAGNATION || loopType === LoopType.GLOBAL_TOOL_CALL_DUPLICATE || loopType === LoopType.INVALID_TOOL_PARAMS_STAGNATION || - loopType === LoopType.REPEATED_TOOL_EXECUTION_FAILURE; + loopType === LoopType.REPEATED_TOOL_EXECUTION_FAILURE || + loopType === LoopType.REPEATED_TOOL_ERROR; const hint = loopType === LoopType.TURN_TOOL_CALL_CAP ? ' A per-turn tool-call cap was reached. The default is adaptive (allows up to 1000 diverse calls, halting only on repeated calls); an explicitly set `model.maxToolCallsPerTurn` is a hard cap. If the model was repeating the same call, investigate the repetition; otherwise unset the value to use the adaptive default, or raise it (set 0 to disable).' diff --git a/packages/core/src/services/loopDetectionService.test.ts b/packages/core/src/services/loopDetectionService.test.ts index c1ffc192343..8fcf8ecb8d3 100644 --- a/packages/core/src/services/loopDetectionService.test.ts +++ b/packages/core/src/services/loopDetectionService.test.ts @@ -3293,4 +3293,142 @@ ${boardState} expect(fired).toBe(false); }); }); + + describe('Repeated tool-error detection (issue #10887)', () => { + // Dead-end sessions kept burning millions of tokens re-running failing + // operations: every (tool, args) pair unique (the model varies the + // retry), interleaved successful reads between failures, and the same + // error returning on every call. No argument-based repetition signal + // ever accumulates, so only the repeated error payload is evidence. + // Mirrored from loopDetectionService.ts. + const REPEATED_TOOL_ERROR_THRESHOLD = 3; + + const errorResult = (errorMessage: string, callId = 'call-err'): Part[] => [ + { + functionResponse: { + id: callId, + name: 'run_shell_command', + response: { error: errorMessage }, + }, + }, + ]; + + const successResult = (output: string, callId = 'call-ok'): Part[] => [ + { + functionResponse: { + id: callId, + name: 'read_file', + response: { output }, + }, + }, + ]; + + it('halts when the same error keeps returning across distinct calls with interleaved successes', () => { + const gitError = + 'fatal: not a git repository (or any of the parent directories): .git'; + let fired = false; + for (let i = 0; i < 10 && !fired; i++) { + // A successful read between failing calls must not mask the streak + // (interleaved reads are what let the reported loops slip past the + // stagnation detectors). + expect( + service.recordToolResult( + { name: 'read_file', args: { file_path: `f${i}.ts` } }, + successResult(`content ${i}`, `ok-${i}`), + ), + ).toBe(false); + fired = service.recordToolResult( + { + name: 'run_shell_command', + args: { command: `git remote -v attempt-${i}` }, + }, + errorResult(gitError, `err-${i}`), + ); + if (i < REPEATED_TOOL_ERROR_THRESHOLD - 1) { + expect(fired).toBe(false); + } + } + expect(fired).toBe(true); + expect(service.getLastLoopType()).toBe(LoopType.REPEATED_TOOL_ERROR); + }); + + it('fires from recordToolResultByCallId for calls this service never streamed', () => { + // client.ts records every functionResponse by callId, including calls + // the service never paired at request time. + const permDenied = 'bash: /usr/bin/foo: Permission denied'; + for (let i = 0; i < REPEATED_TOOL_ERROR_THRESHOLD - 1; i++) { + expect( + service.recordToolResultByCallId( + `unknown-${i}`, + errorResult(permDenied, `unknown-${i}`), + ), + ).toBe(false); + } + expect( + service.recordToolResultByCallId( + `unknown-${REPEATED_TOOL_ERROR_THRESHOLD}`, + errorResult(permDenied, `unknown-${REPEATED_TOOL_ERROR_THRESHOLD}`), + ), + ).toBe(true); + expect(service.getLastLoopType()).toBe(LoopType.REPEATED_TOOL_ERROR); + }); + + it('does not halt below the threshold and restarts the streak on a different error', () => { + const errA = 'fatal: not a git repository'; + const errB = 'npm ERR! code E404'; + const call = (command: string) => ({ + name: 'run_shell_command', + args: { command }, + }); + expect(service.recordToolResult(call('a1'), errorResult(errA))).toBe( + false, + ); + expect(service.recordToolResult(call('a2'), errorResult(errA))).toBe( + false, + ); + // A different error signature restarts the streak... + expect(service.recordToolResult(call('b1'), errorResult(errB))).toBe( + false, + ); + expect(service.recordToolResult(call('a3'), errorResult(errA))).toBe( + false, + ); + expect(service.recordToolResult(call('a4'), errorResult(errA))).toBe( + false, + ); + expect(service.getLastLoopType()).toBeNull(); + }); + + it('clears the error streak on reset()', () => { + const err = 'fatal: not a git repository'; + const call = (command: string) => ({ + name: 'run_shell_command', + args: { command }, + }); + service.recordToolResult(call('a1'), errorResult(err)); + service.recordToolResult(call('a2'), errorResult(err)); + service.reset('fresh-prompt'); + expect(service.recordToolResult(call('a3'), errorResult(err))).toBe( + false, + ); + expect(service.recordToolResult(call('a4'), errorResult(err))).toBe( + false, + ); + expect(service.getLastLoopType()).toBeNull(); + }); + + it('honors an explicit in-session disable', () => { + service.disableForSession(); + const err = 'fatal: not a git repository'; + for (let i = 0; i <= REPEATED_TOOL_ERROR_THRESHOLD; i++) { + expect( + service.recordToolResult( + { name: 'run_shell_command', args: { command: `c${i}` } }, + errorResult(err), + ), + ).toBe(false); + } + expect(service.getLastLoopType()).toBeNull(); + }); + }); }); diff --git a/packages/core/src/services/loopDetectionService.ts b/packages/core/src/services/loopDetectionService.ts index 6c4cd6ab74a..9a42e6fbda8 100644 --- a/packages/core/src/services/loopDetectionService.ts +++ b/packages/core/src/services/loopDetectionService.ts @@ -39,6 +39,20 @@ export { getToolCallRepeatKey }; // so the client breaks the loop before the server rejects the whole // conversation with a 400 (issue #5019). const TOOL_CALL_LOOP_THRESHOLD = 5; +// Consecutive tool results carrying the same error signature tolerated +// before the always-on error-repetition guard halts the turn (issue +// #10887). Dead-end sessions kept re-running failing operations with varied +// arguments — every (tool, args) pair unique, so no argument-based +// repetition signal ever accumulated — while the same error kept returning +// (e.g. exit 128 / permission denied on every call). The streak is keyed by +// a fingerprint of the error payload and counts across arbitrary calls: +// successful results in between neither advance nor reset it (interleaved +// reads are exactly what let the reported loops slip past the stagnation +// detectors), and a different error signature restarts it. Deliberately low: +// a byte-identical error repeating across calls is never a productive +// signal — a corrected approach that still fails identically is exactly the +// dead end to surface to the user instead of burning tokens on. +const REPEATED_TOOL_ERROR_THRESHOLD = 3; const CONTENT_LOOP_THRESHOLD = 10; const CONTENT_CHUNK_SIZE = 50; // Cap for the debug-log excerpt of a fired chanting region (~one period, @@ -377,6 +391,14 @@ export class LoopDetectionService { { fingerprint: string; count: number } >(); + // Consecutive identical tool-error signatures (issue #10887): fingerprint + // of the most recent error payload and how many consecutive error results + // carried it. Successful results neither advance nor reset the streak (an + // interleaved read must not mask a dead end); a different error signature + // restarts it at one. + private toolErrorStreakSignature: string | null = null; + private toolErrorStreakCount = 0; + // callId → request pairing so results can be matched to their calls when // the runtime only has the response (populated on ToolCallRequest events, // consumed by recordToolResultByCallId). @@ -440,6 +462,12 @@ export class LoopDetectionService { ): boolean { if (this.loopDetected) return true; if (this.disabledForSession) return false; + // Repeated tool-error detection (issue #10887): applies to every tool, + // ahead of the stateful-read carve-out — the dead-end signal is the + // repeated error payload, not which call produced it. + if (this.checkRepeatedToolError(responseParts)) { + return true; + } if (!this.isStatefulReadTool(toolCall.name)) return false; const resultText = LoopDetectionService.extractResultText(responseParts); @@ -523,14 +551,20 @@ export class LoopDetectionService { * Variant of recordToolResult for runtimes that only have the response: * the request is resolved through the callId pairing populated on * ToolCallRequest events. Unknown callIds (e.g. client-initiated calls - * that never streamed through this service) are ignored. + * that never streamed through this service) skip the request-dependent + * guards but still feed the error-repetition guard, which works from the + * result alone (issue #10887). */ recordToolResultByCallId( callId: string, responseParts: readonly Part[], ): boolean { const request = this.requestByCallId.get(callId); - if (!request) return false; + if (!request) { + if (this.loopDetected) return true; + if (this.disabledForSession) return false; + return this.checkRepeatedToolError(responseParts); + } this.requestByCallId.delete(callId); return this.recordToolResult( { name: request.name, args: request.args }, @@ -567,6 +601,70 @@ export class LoopDetectionService { return chunks.length > 0 ? chunks.join('\n') : null; } + /** + * Extracts the error payload of a failed tool result, or null when the + * parts carry none. Failed calls surface their failure as a + * `functionResponse.response.error` string across every runtime (scheduler + * error responses, cancellations, timeouts). Oversized error messages + * arrive as persistence stubs whose envelope embeds a per-call unique + * path, so each value is reduced to its stable payload first + * (stripPersistenceEnvelope) — identical underlying errors fingerprint + * identically no matter where they were persisted. + */ + private static extractToolErrorText( + responseParts: readonly Part[], + ): string | null { + const errors: string[] = []; + for (const part of responseParts) { + const response = part.functionResponse?.response; + if (!response) continue; + const error = response['error']; + if (typeof error === 'string' && error.trim().length > 0) { + errors.push(stripPersistenceEnvelope(error)); + } + } + return errors.length > 0 ? errors.join('\n') : null; + } + + /** + * Repeated tool-error detection (issue #10887): halts the turn when the + * same error signature returns on REPEATED_TOOL_ERROR_THRESHOLD consecutive + * error results. Result-aware and tool-agnostic: dead-end loops vary their + * (tool, args) on every retry, so argument-based repetition never + * accumulates — the repeated error payload is the evidence. Always-on like + * the consecutive-identical-call guard (a byte-identical error repeating + * across calls is never productive, and the gated heuristics ship disabled + * by default in the CLI). + * + * @returns true when the streak trips the threshold (loopDetected is set); + * callers halt the turn exactly as for an event-detected loop. + */ + private checkRepeatedToolError(responseParts: readonly Part[]): boolean { + const errorText = LoopDetectionService.extractToolErrorText(responseParts); + if (errorText === null) { + // A successful result is neither evidence of the dead end nor a reset: + // interleaved reads between failing calls must not mask the streak. + return false; + } + const signature = createHash('sha256').update(errorText).digest('hex'); + if (this.toolErrorStreakSignature === signature) { + this.toolErrorStreakCount++; + } else { + this.toolErrorStreakSignature = signature; + this.toolErrorStreakCount = 1; + } + if (this.toolErrorStreakCount < REPEATED_TOOL_ERROR_THRESHOLD) { + return false; + } + this.lastLoopType = LoopType.REPEATED_TOOL_ERROR; + logLoopDetected( + this.config, + new LoopDetectedEvent(LoopType.REPEATED_TOOL_ERROR, this.promptId), + ); + this.loopDetected = true; + return true; + } + private getToolCallKey(toolCall: { name: string; args: object }): string { return getToolCallRepeatKey(toolCall.name, toolCall.args); } @@ -1677,6 +1775,8 @@ export class LoopDetectionService { this.capMaxKeyRepeat = 0; this.statefulRepeatState.clear(); this.statefulConsecutiveResults.clear(); + this.toolErrorStreakSignature = null; + this.toolErrorStreakCount = 0; this.requestByCallId.clear(); } diff --git a/packages/core/src/telemetry/types.ts b/packages/core/src/telemetry/types.ts index 2215e369f3b..517cea047ba 100644 --- a/packages/core/src/telemetry/types.ts +++ b/packages/core/src/telemetry/types.ts @@ -489,6 +489,8 @@ export enum LoopType { INVALID_TOOL_PARAMS_STAGNATION = 'invalid_tool_params_stagnation', /** The same tool execution failure continued after a corrective reminder. */ REPEATED_TOOL_EXECUTION_FAILURE = 'repeated_tool_execution_failure', + /** Consecutive tool results returned the same error signature (issue #10887). */ + REPEATED_TOOL_ERROR = 'repeated_tool_error', } export class LoopDetectedEvent implements BaseTelemetryEvent { From e9624460779319d147cfafb0008dbd277f1ec7fa Mon Sep 17 00:00:00 2001 From: "jinjing.zzj" Date: Fri, 4 Sep 2026 06:47:37 +0800 Subject: [PATCH 02/10] fix(core): count error-loop guard rounds, not parallel-batch siblings Review feedback on #10916 (qqqys; qwen-code-ci-bot R1-1/R1-2/R1-5): - Record the issue-#10887 error-repetition guard once per assistant round via recordToolErrorBatch: client.ts and the agent runtime now feed every result of an executed batch in one call, and sibling calls collapse into at most one streak advance per distinct error signature. A single denied/cancelled/timed-out parallel batch no longer halts the turn as a "loop" before the model has seen any of the errors; the same error returning on consecutive rounds still trips the threshold. - Normalize run_shell_command exit-failure blocks before hashing: the per-call Command:/Directory:/Process Group PGID: lines made every retry of the same failure fingerprint uniquely, so the guard never fired on the exact incident shape of issue #10887 (repeated git exit-128 failures with varied arguments). - Exclude synthetic non-failure payloads from the fingerprint: session-recovery orphan repairs (>=3 dangling calls on --resume otherwise halted the resumed turn before any model round) and user cancellations. - Update recordToolResult/recordToolResultByCallId docstrings and the checkAlwaysOnSafeties pairing rationale for the batch-level guard. Co-authored-by: Qwen-Coder Patrol-Run: qwen-pr-closeout/jmtlz4ubdgq --- .../core/src/agents/runtime/agent-core.ts | 13 ++ packages/core/src/core/client.ts | 43 ++-- .../src/services/loopDetectionService.test.ts | 186 ++++++++++----- .../core/src/services/loopDetectionService.ts | 219 ++++++++++++------ 4 files changed, 322 insertions(+), 139 deletions(-) diff --git a/packages/core/src/agents/runtime/agent-core.ts b/packages/core/src/agents/runtime/agent-core.ts index 3aa9d45ce29..192648724b0 100644 --- a/packages/core/src/agents/runtime/agent-core.ts +++ b/packages/core/src/agents/runtime/agent-core.ts @@ -1241,6 +1241,19 @@ export class AgentCore { break; } } + if (terminateMode !== AgentTerminateMode.LOOP_DETECTED) { + // Error-repetition guard (issue #10887): one batch-level + // recording per round — the sibling calls of this round are ONE + // round of evidence, not sequential retries, so a single + // denied/cancelled batch cannot trip the guard before the model + // has seen any of the errors. + const roundResultParts = toolCallResult.results.flatMap( + (toolResult) => toolResult.responseParts, + ); + if (loopDetector.recordToolErrorBatch(roundResultParts)) { + terminateMode = AgentTerminateMode.LOOP_DETECTED; + } + } if (terminateMode === AgentTerminateMode.LOOP_DETECTED) { break; } diff --git a/packages/core/src/core/client.ts b/packages/core/src/core/client.ts index 2630a262b14..3c5b6d8eef8 100644 --- a/packages/core/src/core/client.ts +++ b/packages/core/src/core/client.ts @@ -3863,6 +3863,8 @@ export class LlmClient { // loop (issue #9450). A detection here (the result-aware global // duplicate count) halts the turn exactly like the event-loop // guards below. + let loopHalt = false; + const toolResultParts: Part[] = []; for (const part of requestToSend) { if ( typeof part !== 'object' || @@ -3873,27 +3875,40 @@ export class LlmClient { } const functionResponseId = (part as Part).functionResponse?.id; if (!functionResponseId) continue; + toolResultParts.push(part as Part); if ( this.loopDetector.recordToolResultByCallId(functionResponseId, [ part as Part, ]) ) { - for (const goalEvent of await finalizeInterruptedGoalTurn()) { - yield goalEvent; - } - const loopType = this.loopDetector.getLastLoopType(); - yield { - type: LlmEventType.LoopDetected, - ...(loopType && { value: { loopType } }), - }; - await arenaAgentClient?.reportError('Loop detected'); - this.lastApiCompletionTimestamp = Date.now(); - endCurrentInteraction('error', 'loop detected', 'loop_detected'); - this.cancelPendingMemoryPrefetch('no_safe_delivery_point'); - this.fireLoopDetectedStopFailure(loopType); - return turn; + loopHalt = true; + break; } } + // Error-repetition guard (issue #10887): one batch-level recording + // per ToolResult message, so the sibling calls of this round count + // as ONE round of evidence, not as sequential retries — a single + // denied/cancelled batch must not trip the guard before the model + // has seen any of the errors. + if (!loopHalt) { + loopHalt = this.loopDetector.recordToolErrorBatch(toolResultParts); + } + if (loopHalt) { + for (const goalEvent of await finalizeInterruptedGoalTurn()) { + yield goalEvent; + } + const loopType = this.loopDetector.getLastLoopType(); + yield { + type: LlmEventType.LoopDetected, + ...(loopType && { value: { loopType } }), + }; + await arenaAgentClient?.reportError('Loop detected'); + this.lastApiCompletionTimestamp = Date.now(); + endCurrentInteraction('error', 'loop detected', 'loop_detected'); + this.cancelPendingMemoryPrefetch('no_safe_delivery_point'); + this.fireLoopDetectedStopFailure(loopType); + return turn; + } const toolResultMemory = await this.consumeManagedAutoMemoryRecall('tool_result'); if (toolResultMemory?.prompt) { diff --git a/packages/core/src/services/loopDetectionService.test.ts b/packages/core/src/services/loopDetectionService.test.ts index 8fcf8ecb8d3..3959c50de19 100644 --- a/packages/core/src/services/loopDetectionService.test.ts +++ b/packages/core/src/services/loopDetectionService.test.ts @@ -20,6 +20,7 @@ import { LlmEventType } from '../core/turn.js'; import * as loggers from '../telemetry/loggers.js'; import { LoopType } from '../telemetry/types.js'; import type { DebugLogger } from '../utils/debugLogger.js'; +import { ORPHAN_TOOL_USE_REPAIR_REASON } from '../core/llm-chat.js'; import { FULL_OUTPUT_DIGEST_LABEL } from '../tools/truncation.js'; import { DEFAULT_MAX_TOOL_CALLS_PER_TURN, @@ -3300,6 +3301,9 @@ ${boardState} // retry), interleaved successful reads between failures, and the same // error returning on every call. No argument-based repetition signal // ever accumulates, so only the repeated error payload is evidence. + // The guard counts model ROUNDS: runtimes feed every result of a batch + // through one recordToolErrorBatch call, so sibling calls of one + // parallel batch collapse into a single piece of evidence. // Mirrored from loopDetectionService.ts. const REPEATED_TOOL_ERROR_THRESHOLD = 3; @@ -3323,27 +3327,18 @@ ${boardState} }, ]; - it('halts when the same error keeps returning across distinct calls with interleaved successes', () => { + it('halts when the same error keeps returning across rounds with interleaved successes', () => { const gitError = 'fatal: not a git repository (or any of the parent directories): .git'; let fired = false; for (let i = 0; i < 10 && !fired; i++) { - // A successful read between failing calls must not mask the streak - // (interleaved reads are what let the reported loops slip past the - // stagnation detectors). - expect( - service.recordToolResult( - { name: 'read_file', args: { file_path: `f${i}.ts` } }, - successResult(`content ${i}`, `ok-${i}`), - ), - ).toBe(false); - fired = service.recordToolResult( - { - name: 'run_shell_command', - args: { command: `git remote -v attempt-${i}` }, - }, - errorResult(gitError, `err-${i}`), - ); + // One batch per round: a successful read between failing calls must + // not mask the streak (interleaved reads are what let the reported + // loops slip past the stagnation detectors). + fired = service.recordToolErrorBatch([ + ...successResult(`content ${i}`, `ok-${i}`), + ...errorResult(gitError, `err-${i}`), + ]); if (i < REPEATED_TOOL_ERROR_THRESHOLD - 1) { expect(fired).toBe(false); } @@ -3352,23 +3347,58 @@ ${boardState} expect(service.getLastLoopType()).toBe(LoopType.REPEATED_TOOL_ERROR); }); - it('fires from recordToolResultByCallId for calls this service never streamed', () => { + it('counts sibling calls of one parallel batch as ONE round, not as retries', () => { + // A single assistant turn emitting N parallel calls that all hit the + // same fixed error (permission deny, user cancel, shared timeout) + // must not trip the guard: the model emitted all of them from one + // state and has not seen any of the errors yet. + const denied = 'Tool "run_shell_command" is denied.'; + expect( + service.recordToolErrorBatch([ + ...errorResult(denied, 'sib-1'), + ...errorResult(denied, 'sib-2'), + ...errorResult(denied, 'sib-3'), + ...errorResult(denied, 'sib-4'), + ]), + ).toBe(false); + expect(service.getLastLoopType()).toBeNull(); + // The same error returning on the NEXT rounds is the repeat. + expect(service.recordToolErrorBatch(errorResult(denied, 'round-2'))).toBe( + false, + ); + expect(service.recordToolErrorBatch(errorResult(denied, 'round-3'))).toBe( + true, + ); + expect(service.getLastLoopType()).toBe(LoopType.REPEATED_TOOL_ERROR); + }); + + it('still fires for unknown-callId errors fed through the round batch', () => { // client.ts records every functionResponse by callId, including calls - // the service never paired at request time. + // the service never paired at request time; those skip the + // request-dependent guards but their errors still reach the + // error-repetition guard through recordToolErrorBatch. const permDenied = 'bash: /usr/bin/foo: Permission denied'; - for (let i = 0; i < REPEATED_TOOL_ERROR_THRESHOLD - 1; i++) { + for (let i = 1; i < REPEATED_TOOL_ERROR_THRESHOLD; i++) { expect( service.recordToolResultByCallId( `unknown-${i}`, errorResult(permDenied, `unknown-${i}`), ), ).toBe(false); + expect( + service.recordToolErrorBatch(errorResult(permDenied, `unknown-${i}`)), + ).toBe(false); } expect( service.recordToolResultByCallId( `unknown-${REPEATED_TOOL_ERROR_THRESHOLD}`, errorResult(permDenied, `unknown-${REPEATED_TOOL_ERROR_THRESHOLD}`), ), + ).toBe(false); + expect( + service.recordToolErrorBatch( + errorResult(permDenied, `unknown-${REPEATED_TOOL_ERROR_THRESHOLD}`), + ), ).toBe(true); expect(service.getLastLoopType()).toBe(LoopType.REPEATED_TOOL_ERROR); }); @@ -3376,44 +3406,22 @@ ${boardState} it('does not halt below the threshold and restarts the streak on a different error', () => { const errA = 'fatal: not a git repository'; const errB = 'npm ERR! code E404'; - const call = (command: string) => ({ - name: 'run_shell_command', - args: { command }, - }); - expect(service.recordToolResult(call('a1'), errorResult(errA))).toBe( - false, - ); - expect(service.recordToolResult(call('a2'), errorResult(errA))).toBe( - false, - ); + expect(service.recordToolErrorBatch(errorResult(errA))).toBe(false); + expect(service.recordToolErrorBatch(errorResult(errA))).toBe(false); // A different error signature restarts the streak... - expect(service.recordToolResult(call('b1'), errorResult(errB))).toBe( - false, - ); - expect(service.recordToolResult(call('a3'), errorResult(errA))).toBe( - false, - ); - expect(service.recordToolResult(call('a4'), errorResult(errA))).toBe( - false, - ); + expect(service.recordToolErrorBatch(errorResult(errB))).toBe(false); + expect(service.recordToolErrorBatch(errorResult(errA))).toBe(false); + expect(service.recordToolErrorBatch(errorResult(errA))).toBe(false); expect(service.getLastLoopType()).toBeNull(); }); it('clears the error streak on reset()', () => { const err = 'fatal: not a git repository'; - const call = (command: string) => ({ - name: 'run_shell_command', - args: { command }, - }); - service.recordToolResult(call('a1'), errorResult(err)); - service.recordToolResult(call('a2'), errorResult(err)); + service.recordToolErrorBatch(errorResult(err)); + service.recordToolErrorBatch(errorResult(err)); service.reset('fresh-prompt'); - expect(service.recordToolResult(call('a3'), errorResult(err))).toBe( - false, - ); - expect(service.recordToolResult(call('a4'), errorResult(err))).toBe( - false, - ); + expect(service.recordToolErrorBatch(errorResult(err))).toBe(false); + expect(service.recordToolErrorBatch(errorResult(err))).toBe(false); expect(service.getLastLoopType()).toBeNull(); }); @@ -3421,14 +3429,84 @@ ${boardState} service.disableForSession(); const err = 'fatal: not a git repository'; for (let i = 0; i <= REPEATED_TOOL_ERROR_THRESHOLD; i++) { + expect(service.recordToolErrorBatch(errorResult(err))).toBe(false); + } + expect(service.getLastLoopType()).toBeNull(); + }); + + it('fires on repeated shell exit failures despite per-call volatile lines', () => { + // run_shell_command embeds the command, the directory, and a fresh + // process-group id in every failure block (shell.ts); the guard must + // key on the stable failure core, not the volatile lines — issue + // #10887 surfaced on repeated git exit-128 failures with varied + // arguments. + const shellBlock = (attempt: number) => + [ + `Command: git remote show origin attempt-${attempt}`, + `Directory: /work/dir-${attempt}`, + 'Output: fatal: unable to access', + 'Error: (none)', + 'Exit Code: 128', + 'Signal: (none)', + `Process Group PGID: ${10000 + attempt}`, + ].join('\n'); + for (let i = 0; i < REPEATED_TOOL_ERROR_THRESHOLD - 1; i++) { expect( - service.recordToolResult( - { name: 'run_shell_command', args: { command: `c${i}` } }, - errorResult(err), + service.recordToolErrorBatch( + errorResult(shellBlock(i), `shell-${i}`), ), ).toBe(false); } + expect( + service.recordToolErrorBatch( + errorResult( + shellBlock(REPEATED_TOOL_ERROR_THRESHOLD), + `shell-${REPEATED_TOOL_ERROR_THRESHOLD}`, + ), + ), + ).toBe(true); + expect(service.getLastLoopType()).toBe(LoopType.REPEATED_TOOL_ERROR); + }); + + it('ignores synthetic session-recovery and cancellation payloads', () => { + // Session recovery feeds one byte-identical orphan-repair result per + // dangling call through client.ts on --resume; three or more dangling + // calls must not halt the resumed turn before any model round. User + // cancellations are the user's action, not tool failures. + const orphanBatch: Part[] = [ + ...errorResult(ORPHAN_TOOL_USE_REPAIR_REASON, 'orphan-1'), + ...errorResult(ORPHAN_TOOL_USE_REPAIR_REASON, 'orphan-2'), + ...errorResult(ORPHAN_TOOL_USE_REPAIR_REASON, 'orphan-3'), + ]; + for (let round = 0; round <= REPEATED_TOOL_ERROR_THRESHOLD; round++) { + expect(service.recordToolErrorBatch(orphanBatch)).toBe(false); + } + const cancelled = + '[Operation Cancelled] Reason: Tool call cancelled by user.'; + for (let round = 0; round <= REPEATED_TOOL_ERROR_THRESHOLD; round++) { + expect( + service.recordToolErrorBatch([ + ...errorResult(cancelled, 'cancelled-1'), + ...errorResult(cancelled, 'cancelled-2'), + ...errorResult(cancelled, 'cancelled-3'), + ]), + ).toBe(false); + } expect(service.getLastLoopType()).toBeNull(); + // A real error riding alongside synthetic payloads still accumulates. + const real = 'fatal: not a git repository'; + let fired = false; + for ( + let round = 0; + round < REPEATED_TOOL_ERROR_THRESHOLD && !fired; + round++ + ) { + fired = service.recordToolErrorBatch([ + ...errorResult(ORPHAN_TOOL_USE_REPAIR_REASON, `orphan-r${round}`), + ...errorResult(real, `real-${round}`), + ]); + } + expect(fired).toBe(true); }); }); }); diff --git a/packages/core/src/services/loopDetectionService.ts b/packages/core/src/services/loopDetectionService.ts index 9a42e6fbda8..63a5bb10846 100644 --- a/packages/core/src/services/loopDetectionService.ts +++ b/packages/core/src/services/loopDetectionService.ts @@ -19,6 +19,7 @@ import { LoopType, } from '../telemetry/types.js'; import type { Config } from '../config/config.js'; +import { ORPHAN_TOOL_USE_REPAIR_REASON } from '../core/llm-chat.js'; import { getToolCallRepeatKey } from '../tools/tool-call-repeat-key.js'; import { FULL_OUTPUT_DIGEST_LABEL, @@ -39,20 +40,27 @@ export { getToolCallRepeatKey }; // so the client breaks the loop before the server rejects the whole // conversation with a 400 (issue #5019). const TOOL_CALL_LOOP_THRESHOLD = 5; -// Consecutive tool results carrying the same error signature tolerated -// before the always-on error-repetition guard halts the turn (issue -// #10887). Dead-end sessions kept re-running failing operations with varied -// arguments — every (tool, args) pair unique, so no argument-based -// repetition signal ever accumulated — while the same error kept returning -// (e.g. exit 128 / permission denied on every call). The streak is keyed by -// a fingerprint of the error payload and counts across arbitrary calls: +// Consecutive rounds carrying the same error signature tolerated before the +// always-on error-repetition guard halts the turn (issue #10887). Dead-end +// sessions kept re-running failing operations with varied arguments — every +// (tool, args) pair unique, so no argument-based repetition signal ever +// accumulated — while the same error kept returning (e.g. exit 128 / +// permission denied on every call). The streak is keyed by a fingerprint of +// the error payload and counts model ROUNDS, not individual calls: sibling +// calls of one parallel batch collapse into a single piece of evidence (a +// denied batch is one event the model has not yet seen, not N retries), // successful results in between neither advance nor reset it (interleaved // reads are exactly what let the reported loops slip past the stagnation // detectors), and a different error signature restarts it. Deliberately low: -// a byte-identical error repeating across calls is never a productive +// a byte-identical error repeating across rounds is never a productive // signal — a corrected approach that still fails identically is exactly the // dead end to surface to the user instead of burning tokens on. const REPEATED_TOOL_ERROR_THRESHOLD = 3; + +// Producer prefix of user-cancellation error payloads +// (coreToolScheduler.ts createCancelledResponse / the scheduler's +// auxiliary-cancel path): `[Operation Cancelled] Reason: `. +const CANCELLED_TOOL_ERROR_PREFIX = '[Operation Cancelled] Reason:'; const CONTENT_LOOP_THRESHOLD = 10; const CONTENT_CHUNK_SIZE = 50; // Cap for the debug-log excerpt of a fired chanting region (~one period, @@ -392,10 +400,11 @@ export class LoopDetectionService { >(); // Consecutive identical tool-error signatures (issue #10887): fingerprint - // of the most recent error payload and how many consecutive error results - // carried it. Successful results neither advance nor reset the streak (an - // interleaved read must not mask a dead end); a different error signature - // restarts it at one. + // of the most recent error payload and how many consecutive rounds carried + // it. A round advances the streak at most once per distinct signature + // (sibling calls of one parallel batch collapse); successful results + // neither advance nor reset the streak (an interleaved read must not mask + // a dead end); a different error signature restarts it at one. private toolErrorStreakSignature: string | null = null; private toolErrorStreakCount = 0; @@ -452,6 +461,10 @@ export class LoopDetectionService { * and before the model is re-prompted with the result. Runtime paths that * only hold the response (no name/args) can use recordToolResultByCallId. * + * The tool-agnostic error-repetition guard (issue #10887) is NOT fed + * here: it counts model rounds, so runtimes record it once per executed + * batch through recordToolErrorBatch. + * * Returns true when the recorded result itself trips a detector (the * result-aware global-duplicate count); callers must then halt the turn * the same way they do for an event-detected loop. @@ -462,12 +475,6 @@ export class LoopDetectionService { ): boolean { if (this.loopDetected) return true; if (this.disabledForSession) return false; - // Repeated tool-error detection (issue #10887): applies to every tool, - // ahead of the stateful-read carve-out — the dead-end signal is the - // repeated error payload, not which call produced it. - if (this.checkRepeatedToolError(responseParts)) { - return true; - } if (!this.isStatefulReadTool(toolCall.name)) return false; const resultText = LoopDetectionService.extractResultText(responseParts); @@ -551,20 +558,17 @@ export class LoopDetectionService { * Variant of recordToolResult for runtimes that only have the response: * the request is resolved through the callId pairing populated on * ToolCallRequest events. Unknown callIds (e.g. client-initiated calls - * that never streamed through this service) skip the request-dependent - * guards but still feed the error-repetition guard, which works from the - * result alone (issue #10887). + * that never streamed through this service) are ignored here — the + * request-dependent guards need name/args — but they still feed the + * error-repetition guard, which works from the result alone, through the + * round-level recordToolErrorBatch call on the same parts (issue #10887). */ recordToolResultByCallId( callId: string, responseParts: readonly Part[], ): boolean { const request = this.requestByCallId.get(callId); - if (!request) { - if (this.loopDetected) return true; - if (this.disabledForSession) return false; - return this.checkRepeatedToolError(responseParts); - } + if (!request) return false; this.requestByCallId.delete(callId); return this.recordToolResult( { name: request.name, args: request.args }, @@ -572,6 +576,26 @@ export class LoopDetectionService { ); } + /** + * Records one batch (one assistant round) of tool results for the + * error-repetition guard (issue #10887). Runtimes feed EVERY executed + * result of the round in a single call — client.ts once per ToolResult + * message, the agent runtime once per executed batch — so sibling calls + * of one parallel batch count as ONE round of evidence, not as + * sequential retries: a single denied/cancelled/timed-out batch must not + * trip the guard before the model has seen any of the errors. Call once + * per round, after the per-result recording (recordToolResult / + * recordToolResultByCallId). + * + * @returns true when the streak trips the threshold (loopDetected is + * set); callers halt the turn exactly as for an event-detected loop. + */ + recordToolErrorBatch(responseParts: readonly Part[]): boolean { + if (this.loopDetected) return true; + if (this.disabledForSession) return false; + return this.checkRepeatedToolError(responseParts); + } + private isStatefulReadTool(toolName: string): boolean { return STATEFUL_READ_TOOLS.has(toolName); } @@ -602,67 +626,116 @@ export class LoopDetectionService { } /** - * Extracts the error payload of a failed tool result, or null when the - * parts carry none. Failed calls surface their failure as a - * `functionResponse.response.error` string across every runtime (scheduler - * error responses, cancellations, timeouts). Oversized error messages - * arrive as persistence stubs whose envelope embeds a per-call unique - * path, so each value is reduced to its stable payload first - * (stripPersistenceEnvelope) — identical underlying errors fingerprint - * identically no matter where they were persisted. + * Synthetic payloads that must not feed the error-repetition guard: they + * are not real tool failures. Session recovery builds one byte-identical + * orphan-repair result per dangling call after a crash — three or more of + * them would otherwise trip the threshold on the resumed turn before any + * model round — and a cancellation records the user's action, not a + * failure the model can correct. + */ + private static isSyntheticToolError(error: string): boolean { + return ( + error === ORPHAN_TOOL_USE_REPAIR_REASON || + error.startsWith(CANCELLED_TOOL_ERROR_PREFIX) + ); + } + + /** + * run_shell_command exit failures embed per-call volatile lines — the + * command itself (a dead-end loop varies it by definition) and the + * spawned process-group id (fresh on every spawn) — so hashing them would + * fingerprint every retry of the same failure uniquely and the streak + * would never accumulate; issue #10887 surfaced on exactly this shape + * (repeated git exit-128 failures with varied arguments). Reduce + * shell-shaped blocks to their stable failure core (Output/Error/Exit + * Code/Signal); other text passes through unchanged. + */ + private static stripShellBlockVolatiles(text: string): string { + if (!text.includes('Process Group PGID:')) return text; + return text + .split('\n') + .filter((line) => !/^(Command|Directory|Process Group PGID): /.test(line)) + .join('\n'); + } + + /** + * Extracts the per-result error payloads of failed tool results (empty + * when the parts carry none). Failed calls surface their failure as a + * `functionResponse.response.error` string across every runtime + * (scheduler error responses, timeouts). Synthetic non-failure payloads + * (orphan repairs, user cancellations) are skipped; oversized errors + * arrive as persistence envelopes whose per-call unique paths are reduced + * to a stable payload first (stripPersistenceEnvelope); shell failure + * blocks drop their per-call volatile lines — identical underlying errors + * fingerprint identically no matter how they were produced (issue + * #10887). */ - private static extractToolErrorText( + private static extractToolErrorTexts( responseParts: readonly Part[], - ): string | null { + ): string[] { const errors: string[] = []; for (const part of responseParts) { const response = part.functionResponse?.response; if (!response) continue; const error = response['error']; - if (typeof error === 'string' && error.trim().length > 0) { - errors.push(stripPersistenceEnvelope(error)); - } + if (typeof error !== 'string' || error.trim().length === 0) continue; + if (LoopDetectionService.isSyntheticToolError(error)) continue; + errors.push( + LoopDetectionService.stripShellBlockVolatiles( + stripPersistenceEnvelope(error), + ), + ); } - return errors.length > 0 ? errors.join('\n') : null; + return errors; } /** * Repeated tool-error detection (issue #10887): halts the turn when the - * same error signature returns on REPEATED_TOOL_ERROR_THRESHOLD consecutive - * error results. Result-aware and tool-agnostic: dead-end loops vary their - * (tool, args) on every retry, so argument-based repetition never - * accumulates — the repeated error payload is the evidence. Always-on like - * the consecutive-identical-call guard (a byte-identical error repeating - * across calls is never productive, and the gated heuristics ship disabled - * by default in the CLI). + * same error signature returns on REPEATED_TOOL_ERROR_THRESHOLD + * consecutive rounds. Result-aware and tool-agnostic: dead-end loops vary + * their (tool, args) on every retry, so argument-based repetition never + * accumulates — the repeated error payload is the evidence. Always-on + * like the consecutive-identical-call guard (a byte-identical error + * repeating across rounds is never productive, and the gated heuristics + * ship disabled by default in the CLI). + * + * Batch counting: responseParts carries every result of ONE round. + * Sibling calls collapse — the streak advances at most once per distinct + * signature per round, in first-occurrence order — because N + * simultaneous calls are emitted from one model state and are not N + * retries; the repeat is the same signature returning on the NEXT round. + * Successful results are neither evidence of the dead end nor a reset: + * interleaved reads between failing calls must not mask the streak. * * @returns true when the streak trips the threshold (loopDetected is set); * callers halt the turn exactly as for an event-detected loop. */ private checkRepeatedToolError(responseParts: readonly Part[]): boolean { - const errorText = LoopDetectionService.extractToolErrorText(responseParts); - if (errorText === null) { - // A successful result is neither evidence of the dead end nor a reset: - // interleaved reads between failing calls must not mask the streak. - return false; - } - const signature = createHash('sha256').update(errorText).digest('hex'); - if (this.toolErrorStreakSignature === signature) { - this.toolErrorStreakCount++; - } else { - this.toolErrorStreakSignature = signature; - this.toolErrorStreakCount = 1; - } - if (this.toolErrorStreakCount < REPEATED_TOOL_ERROR_THRESHOLD) { - return false; + const errorTexts = + LoopDetectionService.extractToolErrorTexts(responseParts); + const seen = new Set(); + for (const errorText of errorTexts) { + const signature = createHash('sha256').update(errorText).digest('hex'); + // Collapse sibling calls of this round into one piece of evidence. + if (seen.has(signature)) continue; + seen.add(signature); + if (this.toolErrorStreakSignature === signature) { + this.toolErrorStreakCount++; + } else { + this.toolErrorStreakSignature = signature; + this.toolErrorStreakCount = 1; + } + if (this.toolErrorStreakCount >= REPEATED_TOOL_ERROR_THRESHOLD) { + this.lastLoopType = LoopType.REPEATED_TOOL_ERROR; + logLoopDetected( + this.config, + new LoopDetectedEvent(LoopType.REPEATED_TOOL_ERROR, this.promptId), + ); + this.loopDetected = true; + return true; + } } - this.lastLoopType = LoopType.REPEATED_TOOL_ERROR; - logLoopDetected( - this.config, - new LoopDetectedEvent(LoopType.REPEATED_TOOL_ERROR, this.promptId), - ); - this.loopDetected = true; - return true; + return false; } private getToolCallKey(toolCall: { name: string; args: object }): string { @@ -858,8 +931,12 @@ export class LoopDetectionService { const stateful = this.isStatefulReadTool(event.value.name); // Pair requests with their later results (recordToolResultByCallId). - // Only stateful read tools participate: recordToolResult rejects every - // other tool, so tracking them would just accumulate full args objects + // Only stateful read tools participate: they are the only requests + // whose results feed a request-dependent guard (recordToolResult's + // result-aware counting needs name/args). The tool-agnostic + // error-repetition guard works from the result alone and is fed at + // round level through recordToolErrorBatch — unknown callIds included — + // so pairing any other tool would just accumulate full args objects // (write_file args can carry whole file contents) until eviction. if (event.value.callId && stateful) { this.requestByCallId.set(event.value.callId, { From eb846eb5dea9c426d062720f8e4b167f11b52e66 Mon Sep 17 00:00:00 2001 From: "jinjing.zzj" Date: Fri, 4 Sep 2026 06:49:00 +0800 Subject: [PATCH 03/10] fix(core): fingerprint batch-budget truncation envelopes by content digest MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review feedback on #10916 (qwen-code-ci-bot R1-3/R1-6): Oversized errors passing through the batch-budget finalizer reached the error-repetition guard wrapped in a `Tool output truncated. Persisted tool-output artifact: /.txt` envelope the guard did not recognize, so the per-call artifact path and the allocation-dependent head/tail preview made identical underlying errors fingerprint uniquely per call — the streak reset on every result and the guard never fired for exactly the largest errors. fitText now embeds the sha256 of the full pre-truncation text, labeled like the issue-#9450 stub digest line (FULL_OUTPUT_DIGEST_LABEL), and the loop guards recognize the envelope's leading prefix, reducing it to that digest. Recognition keeps the existing startsWith discipline. Regression tests: the same oversized error driven through the real finalizeToolResponses over three rounds with distinct callIds still trips the guard (and the envelope carries the digest line), and persisted-stub-shaped response.error payloads with identical digests but unique envelope paths halt on the third round. Co-authored-by: Qwen-Coder Patrol-Run: qwen-pr-closeout/jmtlz4ubdgq --- .../src/services/loopDetectionService.test.ts | 110 +++++++++++++++++- .../core/src/services/loopDetectionService.ts | 15 ++- .../core/src/tools/tool-response-finalizer.ts | 14 ++- 3 files changed, 131 insertions(+), 8 deletions(-) diff --git a/packages/core/src/services/loopDetectionService.test.ts b/packages/core/src/services/loopDetectionService.test.ts index 3959c50de19..59c195b491c 100644 --- a/packages/core/src/services/loopDetectionService.test.ts +++ b/packages/core/src/services/loopDetectionService.test.ts @@ -21,7 +21,14 @@ import * as loggers from '../telemetry/loggers.js'; import { LoopType } from '../telemetry/types.js'; import type { DebugLogger } from '../utils/debugLogger.js'; import { ORPHAN_TOOL_USE_REPAIR_REASON } from '../core/llm-chat.js'; -import { FULL_OUTPUT_DIGEST_LABEL } from '../tools/truncation.js'; +import { + FULL_OUTPUT_DIGEST_LABEL, + persistAndTruncateToolResult, +} from '../tools/truncation.js'; +import { + finalizeToolResponses, + type ToolResponseBudgetEntry, +} from '../tools/tool-response-finalizer.js'; import { DEFAULT_MAX_TOOL_CALLS_PER_TURN, LoopDetectionService, @@ -32,6 +39,17 @@ vi.mock('../telemetry/loggers.js', () => ({ logLoopDetectionDisabled: vi.fn(), })); +// Only persistAndTruncateToolResult is mocked (the finalizer regression +// test below drives it); every other export stays real. +vi.mock('../tools/truncation.js', async (importOriginal) => { + const actual = + await importOriginal(); + return { + ...actual, + persistAndTruncateToolResult: vi.fn(), + }; +}); + const TOOL_CALL_LOOP_THRESHOLD = 5; const CONTENT_LOOP_THRESHOLD = 10; const CONTENT_CHUNK_SIZE = 50; @@ -3508,5 +3526,95 @@ ${boardState} } expect(fired).toBe(true); }); + + it('halts on persisted oversized-error stubs with per-call unique paths', () => { + // The scheduler persists oversized errors before createErrorResponse + // (persistAndTruncateToolResult); the stub envelope embeds a + // per-call unique path, so the guard must key on the producer digest. + const digest = createHash('sha256') + .update('huge error content') + .digest('hex'); + const persistedStub = (callId: string): string => ` +Output too large (42 KB). Full output saved to: /tool-results/${callId}.txt +${FULL_OUTPUT_DIGEST_LABEL}${digest} +Note: this file may be cleaned up after 24 hours. + +Preview (up to 2000 chars): +huge error content +`; + for (let i = 0; i < REPEATED_TOOL_ERROR_THRESHOLD - 1; i++) { + expect( + service.recordToolErrorBatch( + errorResult(persistedStub(`call-${i}`), `call-${i}`), + ), + ).toBe(false); + } + expect( + service.recordToolErrorBatch( + errorResult(persistedStub('call-final'), 'call-final'), + ), + ).toBe(true); + expect(service.getLastLoopType()).toBe(LoopType.REPEATED_TOOL_ERROR); + }); + + it('halts on identical oversized errors truncated by the batch-budget finalizer', async () => { + // Oversized errors pass through finalizeToolResponses before reaching + // the guard; its truncation envelope embeds a per-call unique artifact + // path, so fitText embeds the sha256 of the full text and the guard + // must key on it — otherwise the largest errors fingerprint uniquely + // per call and the guard never fires. + vi.mocked(persistAndTruncateToolResult).mockImplementation( + async (callId, _toolName, content) => ({ + content, + outputFile: `/tool-results/${callId}.txt`, + bytesWritten: Buffer.byteLength(content), + }), + ); + const bigError = 'src/module.ts(1,5): error TS2345: boom. '.repeat(1000); + const finalizerConfig = { + getToolOutputBatchBudget: () => 4000, + } as unknown as Config; + const runRound = async (round: number): Promise => { + const entries: ToolResponseBudgetEntry[] = Array.from( + { length: 4 }, + (_, k) => ({ + callId: `call-r${round}-k${k}`, + toolName: 'run_shell_command', + responseParts: [ + { + functionResponse: { + id: `call-r${round}-k${k}`, + name: 'run_shell_command', + response: { error: bigError }, + }, + }, + ], + }), + ); + const finalized = await finalizeToolResponses( + finalizerConfig, + entries, + undefined, + false, + ); + return finalized.flatMap((entry) => entry.responseParts); + }; + let fired = false; + for ( + let round = 0; + round < REPEATED_TOOL_ERROR_THRESHOLD && !fired; + round++ + ) { + const parts = await runRound(round); + // The envelope carries the digest line; identical underlying errors + // must fingerprint identically despite per-call unique paths. + const error = parts[0]?.functionResponse?.response?.['error']; + expect(typeof error).toBe('string'); + expect(error as string).toContain(FULL_OUTPUT_DIGEST_LABEL); + fired = service.recordToolErrorBatch(parts); + } + expect(fired).toBe(true); + expect(service.getLastLoopType()).toBe(LoopType.REPEATED_TOOL_ERROR); + }); }); }); diff --git a/packages/core/src/services/loopDetectionService.ts b/packages/core/src/services/loopDetectionService.ts index 63a5bb10846..ad5837831aa 100644 --- a/packages/core/src/services/loopDetectionService.ts +++ b/packages/core/src/services/loopDetectionService.ts @@ -213,16 +213,19 @@ export function shouldHaltOnTurnToolCallCap( return isExplicitCap || totalCalls > hardCap || stuck; } -// Producer shapes of the oversized-result stubs (see tools/truncation.ts). -// Recognition is anchored on these LEADING prefixes: results like task_list -// embed peer-authored text verbatim, and that text can quote stub markers — -// honoring a marker found mid-string would let quoted content collapse or -// vary the fingerprint, so only shapes that START with a producer prefix are -// treated as stubs (issue #9450). +// Producer shapes of the oversized-result stubs (see tools/truncation.ts) +// and of the batch-budget finalizer's truncation envelope (see +// tools/tool-response-finalizer.ts fitText). Recognition is anchored on +// these LEADING prefixes: results like task_list embed peer-authored text +// verbatim, and that text can quote stub markers — honoring a marker found +// mid-string would let quoted content collapse or vary the fingerprint, so +// only shapes that START with a producer prefix are treated as stubs +// (issue #9450). const STUB_PRODUCER_PREFIXES: readonly string[] = [ '', 'Output too large (', TOOL_OUTPUT_TRUNCATED_PREFIX, + 'Tool output truncated.', ]; const STUB_PREVIEW_MARKER = `Preview (up to ${PREVIEW_SIZE_CHARS} chars):`; diff --git a/packages/core/src/tools/tool-response-finalizer.ts b/packages/core/src/tools/tool-response-finalizer.ts index 9909ca6658f..1bd59bc05d5 100644 --- a/packages/core/src/tools/tool-response-finalizer.ts +++ b/packages/core/src/tools/tool-response-finalizer.ts @@ -4,6 +4,7 @@ * SPDX-License-Identifier: Apache-2.0 */ +import { createHash } from 'node:crypto'; import type { Part } from '@google/genai'; import type { Config } from '../config/config.js'; import type { ToolArtifact } from './tools.js'; @@ -16,6 +17,7 @@ import { type ToolResultBoundaryStage, } from './tool-result-boundary-diagnostics.js'; import { + FULL_OUTPUT_DIGEST_LABEL, normalizeToolResultCallId, persistAndTruncateToolResult, } from './truncation.js'; @@ -215,7 +217,7 @@ function fitText( if (text.length <= maxChars) return text; if (maxChars <= 0) return ''; - const header = + const artifactHeader = persistedOutputFiles && persistedOutputFiles.length > 0 ? persistedOutputFiles.length === 1 ? `Tool output truncated. Persisted tool-output artifact: ${persistedOutputFiles[0]}` @@ -223,6 +225,16 @@ function fitText( .map((file) => `- ${file}`) .join('\n')}` : 'Tool output truncated.'; + // sha256 of the FULL pre-truncation text, labeled exactly like the + // single-result stub's digest line (FULL_OUTPUT_DIGEST_LABEL, + // truncation.ts): the header embeds a per-call unique artifact path and + // the head/tail preview below depends on this slot's allocated budget, so + // consumers that fingerprint results (services/loopDetectionService.ts) + // read this digest instead of hashing the envelope — identical underlying + // output fingerprints identically no matter which call it was persisted + // for (issue #10887). + const digest = createHash('sha256').update(text).digest('hex'); + const header = `${artifactHeader}\n${FULL_OUTPUT_DIGEST_LABEL}${digest}`; if (header.length >= maxChars) { return sliceStartWithoutBrokenSurrogate(header, maxChars); } From 75c8fe0d1de51cbde629278a74bda82863959c32 Mon Sep 17 00:00:00 2001 From: "jinjing.zzj" Date: Fri, 4 Sep 2026 13:06:43 +0800 Subject: [PATCH 04/10] fix(core): fingerprint repeated tool errors on producer-owned identities The error-repetition fingerprint was still derived by consumer-side heuristic stripping of rendered producer text, and that surface is unbounded: multi-line command continuation lines, the long-run advisory's per-run elapsed seconds, keep='both' envelopes gluing the payload onto the marker line, fitText re-hashing a pre-stubbed envelope's per-call path, and MCP errors embedding the varied function-call JSON all fingerprinted identical repeated failures uniquely per retry so the streak never accumulated. Close the class where the fields are known: - shell.ts embeds an anchored FULL_OUTPUT_DIGEST_LABEL sha256 of the stable failure core (Output/Error/Exit Code/Signal) into failure blocks; the guard prefers an anchored digest over hashing the block. - fitText reuses an inner producer digest when re-truncating an already-stubbed result instead of hashing the envelope path. - stripPersistenceEnvelope starts the reduced payload on its own line so the volatile-line filter sees the payload's first line anchored. - MCP tool errors fingerprint on the tool name plus the stable server payload after ` with response: `; the model-facing message is unchanged (only the fingerprint derivation is). - Pin that a fully successful round between failing rounds neither advances nor resets the streak. Co-authored-by: Qwen-Coder Patrol-Run: qwen-pr-closeout/jmtme4yqxhl --- .../src/services/loopDetectionService.test.ts | 270 ++++++++++++++++++ .../core/src/services/loopDetectionService.ts | 90 +++++- packages/core/src/tools/shell.test.ts | 76 +++++ packages/core/src/tools/shell.ts | 47 ++- .../core/src/tools/tool-response-finalizer.ts | 40 ++- 5 files changed, 504 insertions(+), 19 deletions(-) diff --git a/packages/core/src/services/loopDetectionService.test.ts b/packages/core/src/services/loopDetectionService.test.ts index 59c195b491c..4581392e5a1 100644 --- a/packages/core/src/services/loopDetectionService.test.ts +++ b/packages/core/src/services/loopDetectionService.test.ts @@ -24,6 +24,7 @@ import { ORPHAN_TOOL_USE_REPAIR_REASON } from '../core/llm-chat.js'; import { FULL_OUTPUT_DIGEST_LABEL, persistAndTruncateToolResult, + TOOL_OUTPUT_TRUNCATED_PREFIX, } from '../tools/truncation.js'; import { finalizeToolResponses, @@ -3616,5 +3617,274 @@ huge error content expect(fired).toBe(true); expect(service.getLastLoopType()).toBe(LoopType.REPEATED_TOOL_ERROR); }); + + // shell.ts embeds an anchored sha256 of the stable failure core + // (Output/Error/Exit Code/Signal) into every failure block; the tests + // below rebuild that producer shape and drive it through the guard. + const SHELL_FAILURE_CORE = [ + 'Output: fatal: unable to access', + 'Error: (none)', + 'Exit Code: 128', + 'Signal: (none)', + ].join('\n'); + const shellFailureCoreDigest = createHash('sha256') + .update(SHELL_FAILURE_CORE) + .digest('hex'); + const shellFailureBlock = ( + attempt: number, + options: { digest?: boolean; multiLineCommand?: boolean } = {}, + ): string => { + const { digest = true, multiLineCommand = false } = options; + return [ + multiLineCommand + ? `Command: git remote show origin &&\n echo attempt-${attempt}` + : `Command: git remote show origin attempt-${attempt}`, + `Directory: /work/dir-${attempt}`, + ...SHELL_FAILURE_CORE.split('\n'), + `Process Group PGID: ${10000 + attempt}`, + ...(digest + ? [`${FULL_OUTPUT_DIGEST_LABEL}${shellFailureCoreDigest}`] + : []), + ].join('\n'); + }; + + it('fires on repeated failures with varied multi-line commands via the producer digest', () => { + // A varied multi-line command puts continuation lines into the block + // that the line-anchored volatile filter cannot strip; the guard must + // prefer the producer-embedded stable-core digest instead of hashing + // the rendered block, or every retry fingerprints uniquely. + for (let i = 0; i < REPEATED_TOOL_ERROR_THRESHOLD - 1; i++) { + expect( + service.recordToolErrorBatch( + errorResult( + shellFailureBlock(i, { multiLineCommand: true }), + `ml-${i}`, + ), + ), + ).toBe(false); + } + expect( + service.recordToolErrorBatch( + errorResult( + shellFailureBlock(REPEATED_TOOL_ERROR_THRESHOLD, { + multiLineCommand: true, + }), + 'ml-final', + ), + ), + ).toBe(true); + expect(service.getLastLoopType()).toBe(LoopType.REPEATED_TOOL_ERROR); + }); + + it('fires despite a varied long-run advisory appended after the shell block', () => { + // shell.ts appends `Note: this foreground command ran for Ns.` with + // per-run elapsed seconds after the block; the digest covers the + // stable core only, so long builds failing identically accumulate. + for (let i = 0; i < REPEATED_TOOL_ERROR_THRESHOLD - 1; i++) { + expect( + service.recordToolErrorBatch( + errorResult( + `${shellFailureBlock(i)}\n\nNote: this foreground command ran for ${ + 61 + i + }s. Consider running it with is_background: true.`, + `lr-${i}`, + ), + ), + ).toBe(false); + } + expect( + service.recordToolErrorBatch( + errorResult( + `${shellFailureBlock(REPEATED_TOOL_ERROR_THRESHOLD)}\n\nNote: this foreground command ran for 67s. Consider running it with is_background: true.`, + 'lr-final', + ), + ), + ).toBe(true); + expect(service.getLastLoopType()).toBe(LoopType.REPEATED_TOOL_ERROR); + }); + + it('fires on a pre-stubbed oversized error re-truncated by the batch-budget finalizer', async () => { + // The scheduler gate persists oversized errors into a buildStub + // envelope BEFORE the finalizer (per-call unique path + stable + // full-content digest); batch-budget pressure then re-truncates that + // envelope. fitText must reuse the inner producer digest — + // recomputing it over the envelope would hash the per-call unique + // path and fingerprint identical repeated errors uniquely per call. + vi.mocked(persistAndTruncateToolResult).mockResolvedValue({ + content: '', + bytesWritten: 0, + }); + const hugeError = 'src/module.ts(1,5): error TS2345: boom. '.repeat(1200); + const innerDigest = createHash('sha256').update(hugeError).digest('hex'); + // The buildStub envelope shape (truncation.ts), embedding the + // per-call unique path and the stable digest of the full content. + const preStubbedError = (callId: string): string => ` +Output too large (42 KB). Full output saved to: /tool-results/${callId}.txt +${FULL_OUTPUT_DIGEST_LABEL}${innerDigest} +Note: this file may be cleaned up after 24 hours. +To read the complete output, use the read_file tool with the absolute file path above. + +Preview (up to 2000 chars): +${hugeError.slice(0, 2000)} +`; + const finalizerConfig = { + getToolOutputBatchBudget: () => 4000, + } as unknown as Config; + const runRound = async (round: number): Promise => { + const entries: ToolResponseBudgetEntry[] = Array.from( + { length: 4 }, + (_, k) => ({ + callId: `call-s${round}-k${k}`, + toolName: 'run_shell_command', + responseParts: [ + { + functionResponse: { + id: `call-s${round}-k${k}`, + name: 'run_shell_command', + response: { error: preStubbedError(`call-s${round}-k${k}`) }, + }, + }, + ], + }), + ); + const finalized = await finalizeToolResponses( + finalizerConfig, + entries, + undefined, + false, + ); + return finalized.flatMap((entry) => entry.responseParts); + }; + let fired = false; + for ( + let round = 0; + round < REPEATED_TOOL_ERROR_THRESHOLD && !fired; + round++ + ) { + const parts = await runRound(round); + const error = parts[0]?.functionResponse?.response?.['error']; + expect(typeof error).toBe('string'); + // Batch-budget pressure re-truncated the pre-stubbed envelope. + expect(error as string).toContain('Tool output truncated.'); + fired = service.recordToolErrorBatch(parts); + } + expect(fired).toBe(true); + expect(service.getLastLoopType()).toBe(LoopType.REPEATED_TOOL_ERROR); + }); + + it('fires on keep-both truncated shell envelopes with varied commands', () => { + // truncateAndSaveToFile keep='both' preserves the head (the Command + // line) and the tail. With the producer digest in the tail the guard + // keys on it; legacy blocks without a digest rely on the payload + // reduction keeping the payload's first line line-anchored so the + // volatile filter strips the varied Command line. + const envelopeFor = (attempt: number, withDigest: boolean): string => { + const tail = [ + ...SHELL_FAILURE_CORE.split('\n'), + `Process Group PGID: ${10000 + attempt}`, + ...(withDigest + ? [`${FULL_OUTPUT_DIGEST_LABEL}${shellFailureCoreDigest}`] + : []), + ].join('\n'); + return `${TOOL_OUTPUT_TRUNCATED_PREFIX}. +The full output has been saved to: /tmp/shell-${attempt}/find.output +To read the complete output, use the read_file tool with the absolute file path above. +The truncated output below shows the beginning and end of the content. The marker '... [CONTENT TRUNCATED] ...' indicates where content was removed. + +Truncated part of the output: +Command: find /nonexistent-${attempt} -name core +Directory: /work + +--- +... [CONTENT TRUNCATED] ... +--- + +${tail}`; + }; + for (let i = 0; i < REPEATED_TOOL_ERROR_THRESHOLD - 1; i++) { + expect( + service.recordToolErrorBatch( + errorResult(envelopeFor(i, true), `kb-${i}`), + ), + ).toBe(false); + } + expect( + service.recordToolErrorBatch( + errorResult( + envelopeFor(REPEATED_TOOL_ERROR_THRESHOLD, true), + 'kb-final', + ), + ), + ).toBe(true); + expect(service.getLastLoopType()).toBe(LoopType.REPEATED_TOOL_ERROR); + + // Legacy block, no producer digest: the single-line varied command + // must be stripped by the volatile filter once the payload starts on + // its own line. + const legacyService = new LoopDetectionService(makeConfig()); + for (let i = 0; i < REPEATED_TOOL_ERROR_THRESHOLD - 1; i++) { + expect( + legacyService.recordToolErrorBatch( + errorResult(envelopeFor(i, false), `kbl-${i}`), + ), + ).toBe(false); + } + expect( + legacyService.recordToolErrorBatch( + errorResult( + envelopeFor(REPEATED_TOOL_ERROR_THRESHOLD, false), + 'kbl-final', + ), + ), + ).toBe(true); + expect(legacyService.getLastLoopType()).toBe( + LoopType.REPEATED_TOOL_ERROR, + ); + }); + + it('fires on repeated MCP errors with varied function-call JSON but identical server payloads', () => { + // buildMcpToolError (mcp-tool.ts) embeds the full function-call JSON + // — including the args a dead-end loop varies on every retry — + // before the stable server payload. Hashing the message verbatim + // would fingerprint every retry uniquely (identical-args MCP errors + // are already caught by the consecutive-identical-call guard), so + // the fingerprint must key on the server payload. + const serverPayload = + '{"errorCode":"TABLE_NOT_FOUND","message":"table orders missing"}'; + const mcpError = (attempt: number) => + `MCP tool 'dw_query' reported tool error for function call: ${JSON.stringify( + { + name: 'dw_query', + args: { sql: `SELECT * FROM t${attempt}`, timeout: attempt * 1000 }, + }, + )} with response: ${serverPayload}`; + for (let i = 0; i < REPEATED_TOOL_ERROR_THRESHOLD - 1; i++) { + expect( + service.recordToolErrorBatch(errorResult(mcpError(i), `mcp-${i}`)), + ).toBe(false); + } + expect( + service.recordToolErrorBatch( + errorResult(mcpError(REPEATED_TOOL_ERROR_THRESHOLD + 6), 'mcp-final'), + ), + ).toBe(true); + expect(service.getLastLoopType()).toBe(LoopType.REPEATED_TOOL_ERROR); + }); + + it('keeps the streak alive across a fully successful round', () => { + // Successful results neither advance nor reset the streak — and that + // must hold across rounds too, not only within a batch: a session + // alternating a fully successful round and a failing round (the + // #10887 interleaved-reads shape stretched across rounds) still + // reaches the threshold on the third failing round. + const err = 'fatal: not a git repository'; + expect(service.recordToolErrorBatch(errorResult(err))).toBe(false); + expect(service.recordToolErrorBatch(successResult('all good'))).toBe( + false, + ); + expect(service.recordToolErrorBatch(errorResult(err))).toBe(false); + expect(service.recordToolErrorBatch(errorResult(err))).toBe(true); + expect(service.getLastLoopType()).toBe(LoopType.REPEATED_TOOL_ERROR); + }); }); }); diff --git a/packages/core/src/services/loopDetectionService.ts b/packages/core/src/services/loopDetectionService.ts index ad5837831aa..4728de58288 100644 --- a/packages/core/src/services/loopDetectionService.ts +++ b/packages/core/src/services/loopDetectionService.ts @@ -61,6 +61,16 @@ const REPEATED_TOOL_ERROR_THRESHOLD = 3; // (coreToolScheduler.ts createCancelledResponse / the scheduler's // auxiliary-cancel path): `[Operation Cancelled] Reason: `. const CANCELLED_TOOL_ERROR_PREFIX = '[Operation Cancelled] Reason:'; + +// Producer shape of MCP tool errors (mcp-tool.ts buildMcpToolError): +// `MCP tool '' reported tool error for function call: +// with response: `. +// The function-call JSON embeds the args a dead-end loop varies on every +// retry; only the server payload after the last ` with response: ` +// separator is the stable failure evidence. +const MCP_TOOL_ERROR_PREFIX = "MCP tool '"; +const MCP_TOOL_ERROR_CALL_MARKER = "' reported tool error for function call: "; +const MCP_TOOL_ERROR_RESPONSE_MARKER = ' with response: '; const CONTENT_LOOP_THRESHOLD = 10; const CONTENT_CHUNK_SIZE = 50; // Cap for the debug-log excerpt of a fired chanting region (~one period, @@ -286,7 +296,15 @@ function stripPersistenceEnvelope(value: string): string { if (payloadStart !== -1) { const payload = value.slice(payloadStart + marker.length); const closeTag = payload.indexOf(''); - return `payload:${closeTag === -1 ? payload : payload.slice(0, closeTag)}`; + // The newline puts the payload on its own line: the marker can + // consume the producer's line break (STUB_TRUNCATED_PART_MARKER + // carries it), and gluing the payload onto the marker line would + // shield the payload's first line — a shell block's `Command: ` + // line after keep='both' truncation — from the line-anchored + // volatile-line filter below (issue #10887). + return `payload:\n${ + closeTag === -1 ? payload : payload.slice(0, closeTag) + }`; } } return `raw:${value}`; @@ -661,17 +679,69 @@ export class LoopDetectionService { .join('\n'); } + /** + * Reduces one error payload to its stable fingerprint text. Shell + * failure blocks carry a producer-embedded sha256 of the stable failure + * core (shell.ts anchors it as a FULL_OUTPUT_DIGEST_LABEL line): prefer + * it over hashing the rendered block, whose per-call volatile text + * (multi-line command continuation lines, the long-run advisory's + * elapsed seconds) the line-anchored volatile filter cannot fully + * enumerate — the producer knows the stable fields, so the producer + * owns the identity (issue #10887). Stub envelopes reduce through + * stripPersistenceEnvelope (their digest or path-free payload); other + * text falls back to the volatile-line filter. + */ + private static normalizeToolErrorText(error: string): string { + const mcpNormalized = LoopDetectionService.normalizeMcpToolError(error); + if (mcpNormalized !== null) return mcpNormalized; + const enveloped = stripPersistenceEnvelope(error); + if (enveloped.includes('Process Group PGID:')) { + const digest = extractAnchoredStubDigest(enveloped); + if (digest !== null) { + return `sha256:${digest}`; + } + } + return LoopDetectionService.stripShellBlockVolatiles(enveloped); + } + + /** + * Reduces an MCP tool error to its stable fingerprint text: the tool + * name plus the server payload after the last ` with response: ` + * separator. buildMcpToolError (mcp-tool.ts) embeds the full + * function-call JSON — including the args a dead-end loop varies on + * every retry — before that separator, so hashing the message verbatim + * fingerprints every retry uniquely and the streak never accumulates; + * identical-args MCP errors are already caught by the + * consecutive-identical-call guard, so this is the only shape where the + * error-repetition guard adds anything for MCP. Only the fingerprint + * derivation changes — the model-facing message is untouched. Returns + * null for non-MCP payloads. + */ + private static normalizeMcpToolError(error: string): string | null { + if (!error.startsWith(MCP_TOOL_ERROR_PREFIX)) return null; + const callMarker = error.indexOf(MCP_TOOL_ERROR_CALL_MARKER); + if (callMarker === -1) return null; + const responseMarker = error.lastIndexOf(MCP_TOOL_ERROR_RESPONSE_MARKER); + if (responseMarker <= callMarker) return null; + const serverToolName = error.slice( + MCP_TOOL_ERROR_PREFIX.length, + callMarker, + ); + const serverPayload = error.slice( + responseMarker + MCP_TOOL_ERROR_RESPONSE_MARKER.length, + ); + return `MCP tool '${serverToolName}' error response: ${serverPayload}`; + } + /** * Extracts the per-result error payloads of failed tool results (empty * when the parts carry none). Failed calls surface their failure as a * `functionResponse.response.error` string across every runtime * (scheduler error responses, timeouts). Synthetic non-failure payloads - * (orphan repairs, user cancellations) are skipped; oversized errors - * arrive as persistence envelopes whose per-call unique paths are reduced - * to a stable payload first (stripPersistenceEnvelope); shell failure - * blocks drop their per-call volatile lines — identical underlying errors - * fingerprint identically no matter how they were produced (issue - * #10887). + * (orphan repairs, user cancellations) are skipped; each remaining error + * is reduced to its stable fingerprint text (normalizeToolErrorText) — + * identical underlying errors fingerprint identically no matter how they + * were produced (issue #10887). */ private static extractToolErrorTexts( responseParts: readonly Part[], @@ -683,11 +753,7 @@ export class LoopDetectionService { const error = response['error']; if (typeof error !== 'string' || error.trim().length === 0) continue; if (LoopDetectionService.isSyntheticToolError(error)) continue; - errors.push( - LoopDetectionService.stripShellBlockVolatiles( - stripPersistenceEnvelope(error), - ), - ); + errors.push(LoopDetectionService.normalizeToolErrorText(error)); } return errors; } diff --git a/packages/core/src/tools/shell.test.ts b/packages/core/src/tools/shell.test.ts index 84d48bc714f..d998e36d8b6 100644 --- a/packages/core/src/tools/shell.test.ts +++ b/packages/core/src/tools/shell.test.ts @@ -97,6 +97,11 @@ function getCommandParameterDescription(shellTool: ShellTool): string { .properties.command.description; } +// Sentinel digest returned by the mocked crypto.createHash. Must be 64 +// lowercase hex chars — the shape the loop guards' line-anchored digest +// scanner admits (services/loopDetectionService.ts). +const FAKE_BLOCK_DIGEST = 'a'.repeat(64); + describe('ShellTool', () => { let shellTool: ShellTool; let mockConfig: Config; @@ -113,6 +118,10 @@ describe('ShellTool', () => { check: ReturnType; recordWrite: ReturnType; }; + // Captures the input of the most recent mocked crypto.createHash chain + // (the failure-block digest, issue #10887) so tests can pin exactly + // which fields are hashed. + let lastCreateHashInput = ''; beforeEach(() => { vi.clearAllMocks(); @@ -227,6 +236,19 @@ describe('ShellTool', () => { (vi.mocked(crypto.randomBytes) as Mock).mockReturnValue( Buffer.from('abcdef', 'hex'), ); + // The failure-block digest (issue #10887) runs through the auto-mocked + // createHash: return a fixed 64-hex sentinel digest and capture the + // hashed input so tests can pin the digested fields. + lastCreateHashInput = ''; + vi.mocked(crypto.createHash).mockImplementation( + () => + ({ + update: (data: string) => { + lastCreateHashInput = data; + return { digest: () => FAKE_BLOCK_DIGEST }; + }, + }) as unknown as ReturnType, + ); shellTool = new ShellTool(mockConfig); @@ -3720,6 +3742,60 @@ describe('ShellTool', () => { expect(result.error?.type).toBe(ToolErrorType.SHELL_EXECUTE_ERROR); }); + it('embeds the stable failure-core digest in error blocks for the loop guards', async () => { + // The error-repetition guard keys on the producer-embedded sha256 of + // the stable failure core (Output/Error/Exit Code/Signal) so varied + // retries of the same failure fingerprint identically (issue #10887). + const invocation = shellTool.build({ + command: 'find missing-directory', + is_background: false, + }); + const promise = invocation.execute(mockAbortSignal); + resolveShellExecution({ + output: 'find: missing-directory: No such file or directory', + exitCode: 1, + error: null, + }); + + const result = await promise; + + expect(result.error?.type).toBe(ToolErrorType.SHELL_EXECUTE_ERROR); + expect(result.llmContent).toContain( + `Full output sha256: ${FAKE_BLOCK_DIGEST}`, + ); + expect(result.error?.message).toContain( + `Full output sha256: ${FAKE_BLOCK_DIGEST}`, + ); + // The digest must cover exactly the stable failure core — not the + // per-call volatile Command/Directory/PGID lines. + expect(lastCreateHashInput).toBe( + [ + 'Output: find: missing-directory: No such file or directory', + 'Error: (none)', + 'Exit Code: 1', + 'Signal: (none)', + ].join('\n'), + ); + }); + + it('keeps successful shell blocks digest-free', async () => { + const invocation = shellTool.build({ + command: 'echo ok', + is_background: false, + }); + const promise = invocation.execute(mockAbortSignal); + resolveShellExecution({ + output: 'ok', + exitCode: 0, + error: null, + }); + + const result = await promise; + + expect(result.error).toBeUndefined(); + expect(result.llmContent).not.toContain('Full output sha256:'); + }); + it('does not exempt exit 1 from a mixed compound command', async () => { const invocation = shellTool.build({ command: 'false && ps aux | grep pattern', diff --git a/packages/core/src/tools/shell.ts b/packages/core/src/tools/shell.ts index e5f87045061..1f57ceba130 100644 --- a/packages/core/src/tools/shell.ts +++ b/packages/core/src/tools/shell.ts @@ -30,7 +30,7 @@ import { ToolConfirmationOutcome, } from './tools.js'; import { getErrorMessage, isNodeError } from '../utils/errors.js'; -import { truncateToolOutput } from './truncation.js'; +import { FULL_OUTPUT_DIGEST_LABEL, truncateToolOutput } from './truncation.js'; import { CommitAttributionService, type StagedFileInfo, @@ -2822,15 +2822,52 @@ export class ShellToolInvocation extends BaseToolInvocation< ? result.error.message.replace(commandToExecute, this.params.command) : '(none)'; - llmContent = [ - `Command: ${this.params.command}`, - `Directory: ${this.params.directory || '(root)'}`, + // The repeatable evidence of a failure: identical for every retry + // of the same dead end, unlike the command/directory/PGID lines. + const stableFailureCoreLines = [ `Output: ${result.output || '(empty)'}`, `Error: ${finalError}`, // Use the cleaned error string. `Exit Code: ${result.exitCode ?? '(none)'}`, `Signal: ${result.signal ?? '(none)'}`, + ]; + const blockLines = [ + `Command: ${this.params.command}`, + `Directory: ${this.params.directory || '(root)'}`, + ...stableFailureCoreLines, `Process Group PGID: ${result.pid ?? '(none)'}`, - ].join('\n'); + ]; + // Failures embed a producer-owned stable identity for the loop + // guards (issue #10887): a sha256 of the stable failure core + // (Output/Error/Exit Code/Signal) anchored as a + // FULL_OUTPUT_DIGEST_LABEL line. The block's remaining lines are + // per-call volatile — the command itself (a dead-end loop varies it + // by definition; multi-line commands put continuation lines the + // consumer's line filter cannot enumerate), the directory, a fresh + // process-group id — and metadata appended after the block (the + // long-run advisory's per-run elapsed seconds) varies too, so + // hashing the rendered block fingerprints every retry uniquely and + // the error-repetition streak never accumulates. The digest covers + // exactly the failure evidence, so identical failures fingerprint + // identically no matter how the retry is varied. Only failures + // become model-facing `response.error` payloads (see the error + // object built below) and only they feed the error-repetition + // guard, so successes keep their block shape unchanged. The same + // digest line shape is consumed by the stateful-read guard's stub + // reduction (extractAnchoredStubDigest), and truncateToolOutput's + // keep='both' tail retention preserves it through truncation. + if ( + isSignalTermination(result.signal) || + isShellExitError(this.params.command, result.exitCode) + ) { + blockLines.push( + FULL_OUTPUT_DIGEST_LABEL + + crypto + .createHash('sha256') + .update(stableFailureCoreLines.join('\n')) + .digest('hex'), + ); + } + llmContent = blockLines.join('\n'); // (Long-run advisory append happens AFTER `truncateToolOutput` // below — see the explanation there for why post-truncation.) diff --git a/packages/core/src/tools/tool-response-finalizer.ts b/packages/core/src/tools/tool-response-finalizer.ts index 1bd59bc05d5..db57493fc03 100644 --- a/packages/core/src/tools/tool-response-finalizer.ts +++ b/packages/core/src/tools/tool-response-finalizer.ts @@ -209,6 +209,37 @@ function sliceEndWithoutBrokenSurrogate(text: string, length: number): string { return text.slice(start); } +/** + * Reads a line-anchored FULL_OUTPUT_DIGEST_LABEL digest embedded in `text` + * (the shape truncation.ts buildStub and shell.ts failure blocks produce): + * the label must START a line and be followed by exactly 64 hex chars + * ending the line. Re-truncating an already-stubbed result under + * batch-budget pressure must reuse that inner producer digest — recomputing + * the sha256 over the envelope would hash the per-call unique artifact path + * and fingerprint identical repeated errors uniquely per call, silently + * disabling the error-repetition guard for exactly the largest errors + * (issue #10887). + */ +function extractAnchoredFullDigest(text: string): string | null { + let searchFrom = 0; + for (;;) { + const index = text.indexOf(FULL_OUTPUT_DIGEST_LABEL, searchFrom); + if (index === -1) return null; + if (index === 0 || text[index - 1] === '\n') { + const digestStart = index + FULL_OUTPUT_DIGEST_LABEL.length; + const digest = text.slice(digestStart, digestStart + 64); + const terminator = text[digestStart + 64]; + if ( + /^[0-9a-f]{64}$/.test(digest) && + (terminator === undefined || terminator === '\n' || terminator === '\r') + ) { + return digest; + } + } + searchFrom = index + FULL_OUTPUT_DIGEST_LABEL.length; + } +} + function fitText( text: string, maxChars: number, @@ -232,8 +263,13 @@ function fitText( // consumers that fingerprint results (services/loopDetectionService.ts) // read this digest instead of hashing the envelope — identical underlying // output fingerprints identically no matter which call it was persisted - // for (issue #10887). - const digest = createHash('sha256').update(text).digest('hex'); + // for (issue #10887). When the input already carries a producer digest + // (an oversized error pre-stubbed by the scheduler gate, a shell failure + // block), reuse it: hashing the envelope here would fold the per-call + // unique path into the fingerprint (see extractAnchoredFullDigest). + const digest = + extractAnchoredFullDigest(text) ?? + createHash('sha256').update(text).digest('hex'); const header = `${artifactHeader}\n${FULL_OUTPUT_DIGEST_LABEL}${digest}`; if (header.length >= maxChars) { return sliceStartWithoutBrokenSurrogate(header, maxChars); From 22ed4502d5251546c9fb7c8d60707704f2b4ff0b Mon Sep 17 00:00:00 2001 From: "jinjing.zzj" Date: Fri, 4 Sep 2026 13:07:00 +0800 Subject: [PATCH 05/10] test(cli): pin repeated-tool-error membership in the always-on hint list The error-repetition guard never consults skipLoopDetection (neither runtime wiring gates it), so a headless halt must print the always-on hint, not the no-op setting as an escape hatch. Mirrors the existing hint-classification pins; removing REPEATED_TOOL_ERROR from the always-on condition turns this test red. Co-authored-by: Qwen-Coder Patrol-Run: qwen-pr-closeout/jmtme4yqxhl --- packages/cli/src/nonInteractiveCli.test.ts | 46 ++++++++++++++++++++++ 1 file changed, 46 insertions(+) diff --git a/packages/cli/src/nonInteractiveCli.test.ts b/packages/cli/src/nonInteractiveCli.test.ts index e71f9f839e6..878fa50aca5 100644 --- a/packages/cli/src/nonInteractiveCli.test.ts +++ b/packages/cli/src/nonInteractiveCli.test.ts @@ -2234,6 +2234,52 @@ describe('runNonInteractive', () => { ); }); + it('shows the always-on hint (not the skipLoopDetection escape) for a repeated-tool-error halt', async () => { + setupMetricsMock(); + const toolCallEvent: ServerLlmStreamEvent = { + type: LlmEventType.ToolCallRequest, + value: { + callId: 'tool-1', + name: 'run_shell_command', + args: { command: 'git remote show origin' }, + isClientInitiated: false, + prompt_id: 'prompt-id-repeated-tool-error', + }, + }; + const events: ServerLlmStreamEvent[] = [ + toolCallEvent, + { + type: LlmEventType.LoopDetected, + value: { loopType: LoopType.REPEATED_TOOL_ERROR }, + }, + ]; + mockLlmClient.sendMessageStream.mockReturnValue( + createStreamFromEvents(events), + ); + + const exitCode = await runNonInteractive( + mockConfig, + mockSettings, + 'Repeat a tool', + 'prompt-id-repeated-tool-error', + ); + + expect(exitCode).toBe(1); + // The error-repetition guard is always-on and never consults + // skipLoopDetection (neither runtime wiring gates it), so the headless + // message must not suggest the no-op setting as an escape hatch. + expect(processStderrSpy).toHaveBeenCalledWith( + expect.stringContaining( + 'always-on guard and cannot be disabled via `model.skipLoopDetection`', + ), + ); + expect(processStderrSpy).not.toHaveBeenCalledWith( + expect.stringContaining( + 'Set the `model.skipLoopDetection` setting to true', + ), + ); + }); + it('shows the skipLoopDetection escape hint for a heuristic loop type', async () => { setupMetricsMock(); const toolCallEvent: ServerLlmStreamEvent = { From 97c9fe78350a7cab68c142366cd61560342fe695 Mon Sep 17 00:00:00 2001 From: root Date: Fri, 4 Sep 2026 17:26:34 +0800 Subject: [PATCH 06/10] fix(core): carry repeated-error evidence on the LoopDetected event When the repeated-tool-error guard (issue #10887) halts a turn, the computed sha256 fingerprint of the repeated error payload was discarded at the fire site: LoopDetectedEvent carried only loop_type + prompt_id, so oncall pages arrived with no evidence of what kept failing. LoopDetectedEvent now carries optional error_signature (the fingerprint) and error_excerpt (raw payload truncated for telemetry, following the KittySequenceOverflowEvent truncation convention), populated only on the REPEATED_TOOL_ERROR fire path. The OTel log attributes pick the fields up through the existing event spread, and the qwen-logger RUM event includes them conditionally so other loop types keep their payload shape. Co-authored-by: Qwen-Coder Patrol-Run: qwen-pr-closeout/jmtmourmvi4 --- .../core/src/services/loopDetectionService.ts | 38 ++++++++++++------- .../src/telemetry/qwen-logger/qwen-logger.ts | 8 ++++ packages/core/src/telemetry/types.ts | 24 +++++++++++- 3 files changed, 55 insertions(+), 15 deletions(-) diff --git a/packages/core/src/services/loopDetectionService.ts b/packages/core/src/services/loopDetectionService.ts index 4728de58288..02e5117c142 100644 --- a/packages/core/src/services/loopDetectionService.ts +++ b/packages/core/src/services/loopDetectionService.ts @@ -441,10 +441,11 @@ export class LoopDetectionService { // Short excerpt of the repeated region captured when the chanting // detector fires, for debug logging only. Deliberately NOT part of the - // LoopDetected event payload: the event contract stays loop_type-only and - // the excerpt rides the debug log instead, so a headless reasoning-channel - // halt (empty stdout, label-only stderr) leaves an artifact that tells a - // true repetition from a misfire. + // LoopDetected event payload — the chanting excerpt rides the debug log + // instead, so a headless reasoning-channel halt (empty stdout, label-only + // stderr) leaves an artifact that tells a true repetition from a misfire. + // (REPEATED_TOOL_ERROR is the exception: its guard carries the error + // signature + excerpt on the event itself, see checkRepeatedToolError.) private lastChantExcerpt = ''; constructor(config: Config) { @@ -741,19 +742,24 @@ export class LoopDetectionService { * (orphan repairs, user cancellations) are skipped; each remaining error * is reduced to its stable fingerprint text (normalizeToolErrorText) — * identical underlying errors fingerprint identically no matter how they - * were produced (issue #10887). + * were produced (issue #10887). The raw payload is retained alongside for + * the telemetry excerpt: the signature identifies the failure, the raw + * text tells oncall what it is. */ - private static extractToolErrorTexts( + private static extractToolErrors( responseParts: readonly Part[], - ): string[] { - const errors: string[] = []; + ): Array<{ raw: string; normalized: string }> { + const errors: Array<{ raw: string; normalized: string }> = []; for (const part of responseParts) { const response = part.functionResponse?.response; if (!response) continue; const error = response['error']; if (typeof error !== 'string' || error.trim().length === 0) continue; if (LoopDetectionService.isSyntheticToolError(error)) continue; - errors.push(LoopDetectionService.normalizeToolErrorText(error)); + errors.push({ + raw: error, + normalized: LoopDetectionService.normalizeToolErrorText(error), + }); } return errors; } @@ -780,11 +786,10 @@ export class LoopDetectionService { * callers halt the turn exactly as for an event-detected loop. */ private checkRepeatedToolError(responseParts: readonly Part[]): boolean { - const errorTexts = - LoopDetectionService.extractToolErrorTexts(responseParts); + const errors = LoopDetectionService.extractToolErrors(responseParts); const seen = new Set(); - for (const errorText of errorTexts) { - const signature = createHash('sha256').update(errorText).digest('hex'); + for (const { raw, normalized } of errors) { + const signature = createHash('sha256').update(normalized).digest('hex'); // Collapse sibling calls of this round into one piece of evidence. if (seen.has(signature)) continue; seen.add(signature); @@ -796,9 +801,14 @@ export class LoopDetectionService { } if (this.toolErrorStreakCount >= REPEATED_TOOL_ERROR_THRESHOLD) { this.lastLoopType = LoopType.REPEATED_TOOL_ERROR; + // Carry the failure evidence: the signature identifies the repeated + // payload and the (truncated) raw excerpt tells oncall what it is. logLoopDetected( this.config, - new LoopDetectedEvent(LoopType.REPEATED_TOOL_ERROR, this.promptId), + new LoopDetectedEvent(LoopType.REPEATED_TOOL_ERROR, this.promptId, { + errorSignature: signature, + errorExcerpt: raw, + }), ); this.loopDetected = true; return true; diff --git a/packages/core/src/telemetry/qwen-logger/qwen-logger.ts b/packages/core/src/telemetry/qwen-logger/qwen-logger.ts index d89edc9f82c..c2ca5ec55e1 100644 --- a/packages/core/src/telemetry/qwen-logger/qwen-logger.ts +++ b/packages/core/src/telemetry/qwen-logger/qwen-logger.ts @@ -739,6 +739,14 @@ export class QwenLogger { properties: { prompt_id: event.prompt_id, error_type: event.loop_type, + // Repeated-tool-error evidence (issue #10887): only present when the + // REPEATED_TOOL_ERROR guard fired. + ...(event.error_signature !== undefined && { + error_signature: event.error_signature, + }), + ...(event.error_excerpt !== undefined && { + error_excerpt: event.error_excerpt, + }), }, }); diff --git a/packages/core/src/telemetry/types.ts b/packages/core/src/telemetry/types.ts index 517cea047ba..f4dbc92fc10 100644 --- a/packages/core/src/telemetry/types.ts +++ b/packages/core/src/telemetry/types.ts @@ -498,12 +498,34 @@ export class LoopDetectedEvent implements BaseTelemetryEvent { 'event.timestamp': string; loop_type: LoopType; prompt_id: string; + /** + * sha256 fingerprint of the repeated tool-error payload that tripped the + * guard. REPEATED_TOOL_ERROR only (issue #10887): pages arrive with the + * identity of the failing payload instead of a bare loop type. + */ + error_signature?: string; + /** + * Leading excerpt of the repeated tool-error payload, truncated for + * telemetry. REPEATED_TOOL_ERROR only (issue #10887). + */ + error_excerpt?: string; - constructor(loop_type: LoopType, prompt_id: string) { + constructor( + loop_type: LoopType, + prompt_id: string, + details?: { errorSignature?: string; errorExcerpt?: string }, + ) { this['event.name'] = 'loop_detected'; this['event.timestamp'] = new Date().toISOString(); this.loop_type = loop_type; this.prompt_id = prompt_id; + if (details?.errorSignature !== undefined) { + this.error_signature = details.errorSignature; + } + if (details?.errorExcerpt !== undefined) { + // Truncate for telemetry (avoid shipping full tool payloads). + this.error_excerpt = details.errorExcerpt.slice(0, 200); + } } } From 23937e806392371b0ee7c1853a0017f67c8fc3f5 Mon Sep 17 00:00:00 2001 From: root Date: Fri, 4 Sep 2026 17:26:51 +0800 Subject: [PATCH 07/10] refactor(core): export producer-owned CANCELLED_TOOL_ERROR_PREFIX coreToolScheduler.ts produced the `[Operation Cancelled] Reason: ` cancellation payload inline at two sites (createCancelledResponse and the auxiliary-cancel path), while loopDetectionService.ts re-declared the prefix literal to recognize it. Export one producer-owned constant from coreToolScheduler.ts and consume it at both producer sites and in the loop-detection guard, following the ORPHAN_TOOL_USE_REPAIR_REASON producer-owns-the-shape pattern. The produced payload text is unchanged. Co-authored-by: Qwen-Coder Patrol-Run: qwen-pr-closeout/jmtmourmvi4 --- packages/core/src/core/coreToolScheduler.ts | 15 +++++++++++++-- .../core/src/services/loopDetectionService.ts | 8 ++++---- 2 files changed, 17 insertions(+), 6 deletions(-) diff --git a/packages/core/src/core/coreToolScheduler.ts b/packages/core/src/core/coreToolScheduler.ts index 2af601ce4f3..2a894a2f633 100644 --- a/packages/core/src/core/coreToolScheduler.ts +++ b/packages/core/src/core/coreToolScheduler.ts @@ -951,6 +951,17 @@ const createErrorResponse = ( ...(artifacts && artifacts.length > 0 ? { artifacts } : {}), }); +/** + * Prefix of the model-visible error payload this scheduler produces for + * cancelled tool calls (see createCancelledResponse and the auxiliary-cancel + * path in `handleCancelToolCalls`). Exported so consumers that must + * recognize cancellation payloads — the loop-detection error-repetition + * guard (services/loopDetectionService.ts) — match the producer-owned + * constant instead of re-declaring the literal (producer-owns-the-shape + * pattern, cf. ORPHAN_TOOL_USE_REPAIR_REASON in llm-chat.ts). + */ +export const CANCELLED_TOOL_ERROR_PREFIX = '[Operation Cancelled] Reason:'; + const createCancelledResponse = ( request: ToolCallRequestInfo, reason: string, @@ -961,7 +972,7 @@ const createCancelledResponse = ( persistedOutputFiles?: string[], visionBridgeNotice?: string, ): CoreToolCallResponseInfo => { - const errorMessage = `[Operation Cancelled] Reason: ${reason}`; + const errorMessage = `${CANCELLED_TOOL_ERROR_PREFIX} ${reason}`; return { callId: request.callId, responseParts: [ @@ -1669,7 +1680,7 @@ export class CoreToolScheduler { const preservedResultDisplay = this.compactResultDisplayForInteractiveHistory(resultDisplay); - const errorMessage = `[Operation Cancelled] Reason: ${auxiliaryData}`; + const errorMessage = `${CANCELLED_TOOL_ERROR_PREFIX} ${auxiliaryData}`; const response: CoreToolCallResponseInfo = isToolCallResponseInfo( auxiliaryData, ) diff --git a/packages/core/src/services/loopDetectionService.ts b/packages/core/src/services/loopDetectionService.ts index 02e5117c142..d1d7c8ebf43 100644 --- a/packages/core/src/services/loopDetectionService.ts +++ b/packages/core/src/services/loopDetectionService.ts @@ -20,6 +20,7 @@ import { } from '../telemetry/types.js'; import type { Config } from '../config/config.js'; import { ORPHAN_TOOL_USE_REPAIR_REASON } from '../core/llm-chat.js'; +import { CANCELLED_TOOL_ERROR_PREFIX } from '../core/coreToolScheduler.js'; import { getToolCallRepeatKey } from '../tools/tool-call-repeat-key.js'; import { FULL_OUTPUT_DIGEST_LABEL, @@ -57,10 +58,9 @@ const TOOL_CALL_LOOP_THRESHOLD = 5; // dead end to surface to the user instead of burning tokens on. const REPEATED_TOOL_ERROR_THRESHOLD = 3; -// Producer prefix of user-cancellation error payloads -// (coreToolScheduler.ts createCancelledResponse / the scheduler's -// auxiliary-cancel path): `[Operation Cancelled] Reason: `. -const CANCELLED_TOOL_ERROR_PREFIX = '[Operation Cancelled] Reason:'; +// Producer prefix of user-cancellation error payloads: the producer-owned +// constant CANCELLED_TOOL_ERROR_PREFIX (coreToolScheduler.ts) anchors the +// `[Operation Cancelled] Reason: ` shape. // Producer shape of MCP tool errors (mcp-tool.ts buildMcpToolError): // `MCP tool '' reported tool error for function call: From 8a8240c4cfb1fc9c689219c398b1c2c3a41ee7b4 Mon Sep 17 00:00:00 2001 From: root Date: Fri, 4 Sep 2026 17:27:07 +0800 Subject: [PATCH 08/10] test(core): pin the client.ts wiring of the repeated-tool-error guard MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The ToolResult wiring of the error-repetition guard (issue #10887) had no coverage outside loopDetectionService.test.ts: an edit dropping loopDetector.recordToolErrorBatch from client.ts would pass CI. Drive real sendMessageStream rounds whose tool fails with the same error on every round (varied args — the reported dead-end shape, so no argument-based repetition signal can accumulate) and assert the turn halts with LoopDetected/repeated_tool_error, plus a changed-error control that keeps the turn alive. Reverting the wiring fails the halt test. Co-authored-by: Qwen-Coder Patrol-Run: qwen-pr-closeout/jmtmourmvi4 --- packages/core/src/core/client.test.ts | 89 +++++++++++++++++++++++++++ 1 file changed, 89 insertions(+) diff --git a/packages/core/src/core/client.test.ts b/packages/core/src/core/client.test.ts index d4f42044e72..90c65a70122 100644 --- a/packages/core/src/core/client.test.ts +++ b/packages/core/src/core/client.test.ts @@ -8439,6 +8439,95 @@ hello ).toBe('consecutive_identical_tool_calls'); }); + // Drives sendMessageStream with ToolResult messages carrying failed tool + // results, exercising the runtime wiring of the error-repetition guard + // (issue #10887): every ToolResult round must reach + // loopDetector.recordToolErrorBatch, and a detection must halt the + // turn with a LoopDetected event. The tool varies its args on every + // round — the reported dead-end shape — so no argument-based repetition + // signal can accumulate; only the batch feed sees the repeated error. + // Reverting the recordToolErrorBatch wiring in client.ts leaves the turn + // running and fails the halt test below. + async function runFailingToolTurns( + errorFor: (round: number) => string, + maxRounds = 5, + ) { + const promptId = 'prompt-repeated-tool-error'; + const allEvents: Array<{ type: string; value?: unknown }> = []; + for (let round = 0; round <= maxRounds; round++) { + mockTurnRunFn.mockReturnValueOnce( + (async function* () { + yield { + type: LlmEventType.ToolCallRequest, + value: { + callId: `fail-${round}`, + name: 'run_shell_command', + args: { command: `attempt-${round}` }, + isClientInitiated: false, + prompt_id: promptId, + }, + }; + })(), + ); + const contents = + round === 0 + ? [{ text: 'do the work' }] + : [ + { + functionResponse: { + id: `fail-${round - 1}`, + name: 'run_shell_command', + response: { error: errorFor(round - 1) }, + }, + }, + ]; + const events = await fromAsync( + client.sendMessageStream( + contents as never, + new AbortController().signal, + promptId, + { + type: + round === 0 + ? SendMessageType.UserQuery + : SendMessageType.ToolResult, + }, + ), + ); + allEvents.push(...(events as Array<{ type: string; value?: unknown }>)); + if ( + allEvents.some((e) => e.type === LlmEventType.LoopDetected) || + !events.some((e) => e.type === LlmEventType.ToolCallRequest) + ) { + return allEvents; + } + } + return allEvents; + } + + it('halts the interactive turn when ToolResult rounds keep returning the same error (#10887)', async () => { + const events = await runFailingToolTurns( + () => + 'fatal: not a git repository (or any of the parent directories): .git', + ); + const loopEvent = events.find( + (e) => e.type === LlmEventType.LoopDetected, + ); + expect(loopEvent).toBeDefined(); + expect( + (loopEvent?.value as { loopType?: string } | undefined)?.loopType, + ).toBe('repeated_tool_error'); + }); + + it('keeps the interactive turn alive while ToolResult errors keep changing (#10887)', async () => { + const events = await runFailingToolTurns( + (round) => `fatal: attempt ${round} failed in a new way`, + ); + expect(events.some((e) => e.type === LlmEventType.LoopDetected)).toBe( + false, + ); + }); + it('should halt via the always-on turn cap before the skipLoopDetection gate', async () => { let abortHandlerInvoked = false; mockMemoryManager.recall.mockImplementation((_root, _query, opts) => { From 3b1ce40cd2244505e892b9f8baa8d5dd1ab221bf Mon Sep 17 00:00:00 2001 From: "jinjing.zzj" Date: Fri, 4 Sep 2026 22:05:43 +0800 Subject: [PATCH 09/10] fix(core): unify repeated-tool-error fingerprints across truncation forms MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two stability gaps in the #10887 error-repetition guard: 1. buildStub hashed the FULL block, so shell failures in the (scheduler persistence gate, shell in-tool threshold] size band — whose blocks carry per-call volatile Command:/PGID lines — got a unique envelope digest per retry. The producer-anchored failure-core digest (the block's last line) never reached the stub, so the streak reset every round and REPEATED_TOOL_ERROR never fired. buildStub now reuses an anchored producer digest when present; content without one keeps the full-content hash (#9450 invariant). 2. The same failure fingerprinted differently by truncation form: raw blocks reduced to sha256: while enveloped forms (buildStub / keep='both' / fitText) reduced to sha256: with the identical digest. The varying Command: line and sibling sizes move one failure core across those thresholds between rounds, so the streak reset exactly at each form flip. Raw shell blocks now reduce to the same sha256: namespace (shared literal, PGID gate unchanged; non-shell stub fingerprints and the stateful-read path are untouched). Adds regression tests (each red-proofed by reverting its fix): the real persistAndTruncateToolResult stubbing a 29k-band block with varied Command/PGID per round must accumulate the streak and fire; alternating raw/keep-both forms of one failure must fire on round 3; buildStub digest reuse and the #9450 full-content-hash fallback are pinned. Co-authored-by: Qwen-Coder Patrol-Run: qwen-pr-closeout/jmtmxfessij --- .../src/services/loopDetectionService.test.ts | 116 ++++++++++++++++++ .../core/src/services/loopDetectionService.ts | 28 ++++- packages/core/src/tools/truncation.test.ts | 78 ++++++++++++ packages/core/src/tools/truncation.ts | 46 ++++++- 4 files changed, 264 insertions(+), 4 deletions(-) diff --git a/packages/core/src/services/loopDetectionService.test.ts b/packages/core/src/services/loopDetectionService.test.ts index 4581392e5a1..1af89986db3 100644 --- a/packages/core/src/services/loopDetectionService.test.ts +++ b/packages/core/src/services/loopDetectionService.test.ts @@ -6,6 +6,9 @@ import { beforeEach, describe, expect, it, vi } from 'vitest'; import { createHash } from 'node:crypto'; +import { mkdtempSync, rmSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import * as path from 'node:path'; import type { Part } from '@google/genai'; import type { Config } from '../config/config.js'; import type { @@ -3842,6 +3845,119 @@ ${tail}`; ); }); + it('fires when the same shell failure alternates raw and keep-both truncation forms', () => { + // The varying Command: line moves an identical failure core across + // the shell's in-tool truncation threshold between rounds: short + // command -> raw block, long command -> keep='both' envelope. Both + // forms carry the same producer failure-core digest and must + // fingerprint identically — distinct signature namespaces per form + // would reset the streak exactly on the round where the form flips + // and the guard would never fire (issue #10887). + const keepBothEnvelopeFor = (attempt: number): string => { + const tail = [ + ...SHELL_FAILURE_CORE.split('\n'), + `Process Group PGID: ${10000 + attempt}`, + `${FULL_OUTPUT_DIGEST_LABEL}${shellFailureCoreDigest}`, + ].join('\n'); + return `${TOOL_OUTPUT_TRUNCATED_PREFIX}. +The full output has been saved to: /tmp/shell-${attempt}/find.output +To read the complete output, use the read_file tool with the absolute file path above. +The truncated output below shows the beginning and end of the content. The marker '... [CONTENT TRUNCATED] ...' indicates where content was removed. + +Truncated part of the output: +Command: find /nonexistent-${attempt} -name core +Directory: /work + +--- +... [CONTENT TRUNCATED] ... +--- + +${tail}`; + }; + expect( + service.recordToolErrorBatch( + errorResult(shellFailureBlock(0), 'alt-raw-0'), + ), + ).toBe(false); + expect( + service.recordToolErrorBatch( + errorResult(keepBothEnvelopeFor(1), 'alt-envelope-1'), + ), + ).toBe(false); + expect( + service.recordToolErrorBatch( + errorResult(shellFailureBlock(2), 'alt-raw-2'), + ), + ).toBe(true); + expect(service.getLastLoopType()).toBe(LoopType.REPEATED_TOOL_ERROR); + }); + + it('fires on repeated scheduler-gate stubs of one shell failure with varied Command/PGID per round', async () => { + // A failure block in the (scheduler persistence gate, shell in-tool + // threshold] size band skips in-tool truncation and is persisted by + // the gate via the REAL persistAndTruncateToolResult -> buildStub. + // buildStub must reuse the producer-anchored failure-core digest: + // with the per-call volatile Command:/PGID lines varied per round, + // hashing the full block would fingerprint every retry uniquely and + // REPEATED_TOOL_ERROR would never fire (issue #10887). + const actualTruncation = await vi.importActual< + typeof import('../tools/truncation.js') + >('../tools/truncation.js'); + // Pad Output so the block sits in the band: above the scheduler's + // 28k persistence gate, below the shell's own 30k threshold. + const paddedCore = [ + `Output: ${'error TS2345: argument not assignable. '.repeat(730)}`, + 'Error: Exit code 1', + 'Exit Code: 1', + 'Signal: (none)', + ].join('\n'); + const coreDigest = createHash('sha256').update(paddedCore).digest('hex'); + const blockFor = (attempt: number): string => + [ + `Command: npm run build -- --filter pkg-${attempt}`, + `Directory: /work/dir-${attempt}`, + paddedCore, + `Process Group PGID: ${10000 + attempt}`, + `${FULL_OUTPUT_DIGEST_LABEL}${coreDigest}`, + ].join('\n'); + expect(blockFor(0).length).toBeGreaterThan(28_000); + expect(blockFor(0).length).toBeLessThanOrEqual(30_000); + const toolResultsDir = mkdtempSync( + path.join(tmpdir(), 'qc-pr10916-gate-'), + ); + const gateConfig = { + getToolResultBytesWritten: () => 0, + trackToolResultBytes: vi.fn(), + storage: { getToolResultsDir: () => toolResultsDir }, + } as unknown as Config; + try { + const firedPerRound: boolean[] = []; + for (let round = 0; round < REPEATED_TOOL_ERROR_THRESHOLD; round++) { + const persisted = await actualTruncation.persistAndTruncateToolResult( + `call-band-${round}`, + 'run_shell_command', + blockFor(round), + gateConfig, + ); + // The gate really stubbed the oversized block. + expect(persisted.content.startsWith('')).toBe(true); + expect(persisted.content.length).toBeLessThan(blockFor(round).length); + firedPerRound.push( + service.recordToolErrorBatch( + errorResult(persisted.content, `band-${round}`), + ), + ); + } + expect(firedPerRound).toEqual([ + ...Array(REPEATED_TOOL_ERROR_THRESHOLD - 1).fill(false), + true, + ]); + expect(service.getLastLoopType()).toBe(LoopType.REPEATED_TOOL_ERROR); + } finally { + rmSync(toolResultsDir, { recursive: true, force: true }); + } + }); + it('fires on repeated MCP errors with varied function-call JSON but identical server payloads', () => { // buildMcpToolError (mcp-tool.ts) embeds the full function-call JSON // — including the args a dead-end loop varies on every retry — diff --git a/packages/core/src/services/loopDetectionService.ts b/packages/core/src/services/loopDetectionService.ts index d1d7c8ebf43..582954521c0 100644 --- a/packages/core/src/services/loopDetectionService.ts +++ b/packages/core/src/services/loopDetectionService.ts @@ -241,6 +241,14 @@ const STUB_PRODUCER_PREFIXES: readonly string[] = [ const STUB_PREVIEW_MARKER = `Preview (up to ${PREVIEW_SIZE_CHARS} chars):`; const STUB_TRUNCATED_PART_MARKER = 'Truncated part of the output:\n'; +/** + * Prefix of the reduction every digest-carrying stub shape maps to. Shared + * literal between stripPersistenceEnvelope and normalizeToolErrorText so + * the raw/enveloped shapes of one shell failure cannot drift into two + * signature namespaces again (issue #10887). + */ +const PERSISTED_STUB_DIGEST_PREFIX = 'sha256:'; + /** * Reads the sha256 digest a stub producer embedded for the FULL * pre-truncation output: the label must START its line and be followed by @@ -289,7 +297,7 @@ function stripPersistenceEnvelope(value: string): string { } const digest = extractAnchoredStubDigest(value); if (digest !== null) { - return `sha256:${digest}`; + return `${PERSISTED_STUB_DIGEST_PREFIX}${digest}`; } for (const marker of [STUB_PREVIEW_MARKER, STUB_TRUNCATED_PART_MARKER]) { const payloadStart = value.indexOf(marker); @@ -691,6 +699,22 @@ export class LoopDetectionService { * owns the identity (issue #10887). Stub envelopes reduce through * stripPersistenceEnvelope (their digest or path-free payload); other * text falls back to the volatile-line filter. + * + * One signature namespace for every digest-carrying shape: the identical + * failure reaches this guard raw (under the scheduler persistence gate), + * in a buildStub envelope (the (gate, shell-threshold] size band), in a + * keep='both' truncation envelope, or in the batch-budget finalizer's + * fitText envelope — the varying Command: line and varying sibling sizes + * move the same failure core across those thresholds between rounds. + * Every shape retains the producer's line-anchored failure-core digest + * in its raw text (the raw block's last line, the stub envelope's digest + * line, keep='both' tail / fitText header retention), so the raw block + * must reduce to the SAME prefix the stub reduction emits — distinct + * prefixes per shape would reset the streak exactly on the round where + * the truncation form flips (issue #10887). The PGID gate keeps this + * scoped to shell-shaped payloads: non-shell stubs carry no PGID and + * keep their stripPersistenceEnvelope fingerprint unchanged (that + * reduction is shared with the stateful-read path, issue #9450). */ private static normalizeToolErrorText(error: string): string { const mcpNormalized = LoopDetectionService.normalizeMcpToolError(error); @@ -699,7 +723,7 @@ export class LoopDetectionService { if (enveloped.includes('Process Group PGID:')) { const digest = extractAnchoredStubDigest(enveloped); if (digest !== null) { - return `sha256:${digest}`; + return `${PERSISTED_STUB_DIGEST_PREFIX}${digest}`; } } return LoopDetectionService.stripShellBlockVolatiles(enveloped); diff --git a/packages/core/src/tools/truncation.test.ts b/packages/core/src/tools/truncation.test.ts index 78dd5b605fb..6c10eda163d 100644 --- a/packages/core/src/tools/truncation.test.ts +++ b/packages/core/src/tools/truncation.test.ts @@ -5,6 +5,7 @@ */ import { describe, it, expect, vi, beforeEach } from 'vitest'; +import { createHash } from 'node:crypto'; import type { Part } from '@google/genai'; import { truncateAndSaveToFile, @@ -12,6 +13,7 @@ import { truncateLlmContent, TOOL_OUTPUT_TRUNCATED_PREFIX, persistAndTruncateToolResult, + FULL_OUTPUT_DIGEST_LABEL, } from './truncation.js'; import * as fs from 'node:fs/promises'; import * as path from 'node:path'; @@ -507,6 +509,82 @@ describe('persistAndTruncateToolResult', () => { Buffer.byteLength(content), ); }); + + it('reuses the producer-anchored failure-core digest instead of hashing the volatile full block', async () => { + // shell.ts failure blocks anchor a sha256 of the stable failure core + // (Output/Error/Exit Code/Signal) as a FULL_OUTPUT_DIGEST_LABEL line; + // the Command:/Directory:/PGID lines are per-call volatile. buildStub + // must reuse the anchored core digest: hashing the full block would + // fold the volatile lines into the envelope digest, fingerprinting + // every retry of the identical failure uniquely — the error-repetition + // guard would then never fire for failures in the (scheduler + // persistence gate, shell in-tool threshold] size band (issue #10887). + vi.mocked(fs.mkdir).mockResolvedValue(undefined); + vi.mocked(atomicWriteFile).mockResolvedValue(undefined); + const config = { + getToolResultBytesWritten: () => 0, + trackToolResultBytes: vi.fn(), + storage: { getToolResultsDir: () => '/primary' }, + } as unknown as Config; + const stableCore = [ + 'Output: fatal: unable to access', + 'Error: (none)', + 'Exit Code: 128', + 'Signal: (none)', + ].join('\n'); + const coreDigest = createHash('sha256').update(stableCore).digest('hex'); + const blockFor = (attempt: number): string => + [ + `Command: git remote show origin attempt-${attempt}`, + `Directory: /work/dir-${attempt}`, + stableCore, + `Process Group PGID: ${10000 + attempt}`, + `${FULL_OUTPUT_DIGEST_LABEL}${coreDigest}`, + ].join('\n'); + + const envelopeDigests = new Set(); + for (let attempt = 0; attempt < 3; attempt++) { + const block = blockFor(attempt); + // Sanity: the per-call volatile lines really do change the full hash. + expect(createHash('sha256').update(block).digest('hex')).not.toBe( + coreDigest, + ); + const result = await persistAndTruncateToolResult( + `call-${attempt}`, + 'run_shell_command', + block, + config, + ); + const match = result.content.match( + new RegExp(`${FULL_OUTPUT_DIGEST_LABEL}([0-9a-f]{64})`), + ); + expect(match?.[1]).toBe(coreDigest); + envelopeDigests.add(match?.[1] ?? ''); + } + expect(envelopeDigests.size).toBe(1); + }); + + it('keeps the full-content digest when the content carries no anchored producer digest', async () => { + // #9450 invariant: the preview only covers the first PREVIEW_SIZE_CHARS + // chars, so without a producer digest the envelope digest must stay + // sensitive to mutations anywhere in the full content. + vi.mocked(fs.mkdir).mockResolvedValue(undefined); + vi.mocked(atomicWriteFile).mockResolvedValue(undefined); + const config = { + getToolResultBytesWritten: () => 0, + trackToolResultBytes: vi.fn(), + storage: { getToolResultsDir: () => '/primary' }, + } as unknown as Config; + const content = 'x'.repeat(10_000); + const result = await persistAndTruncateToolResult( + 'call-nodigest', + 'run_shell_command', + content, + config, + ); + const fullHash = createHash('sha256').update(content).digest('hex'); + expect(result.content).toContain(`${FULL_OUTPUT_DIGEST_LABEL}${fullHash}`); + }); }); describe('truncateToolOutput', () => { diff --git a/packages/core/src/tools/truncation.ts b/packages/core/src/tools/truncation.ts index 1057f4ab1ab..2947b2b8769 100644 --- a/packages/core/src/tools/truncation.ts +++ b/packages/core/src/tools/truncation.ts @@ -426,6 +426,35 @@ export function normalizeToolResultCallId(callId: string): string | undefined { : safeCallId; } +/** + * Reads a line-anchored FULL_OUTPUT_DIGEST_LABEL digest embedded in `text` + * (the shape shell.ts failure blocks produce: a sha256 of the stable + * failure core anchored as the block's last line): the label must START a + * line and be followed by exactly 64 hex chars ending the line. A mid-line + * mention of the label (quoted content) never matches. Mirrors the + * consumer-side extractors in services/loopDetectionService.ts and + * tools/tool-response-finalizer.ts. + */ +function extractAnchoredFullDigest(text: string): string | null { + let searchFrom = 0; + for (;;) { + const index = text.indexOf(FULL_OUTPUT_DIGEST_LABEL, searchFrom); + if (index === -1) return null; + if (index === 0 || text[index - 1] === '\n') { + const digestStart = index + FULL_OUTPUT_DIGEST_LABEL.length; + const digest = text.slice(digestStart, digestStart + 64); + const terminator = text[digestStart + 64]; + if ( + /^[0-9a-f]{64}$/.test(digest) && + (terminator === undefined || terminator === '\n' || terminator === '\r') + ) { + return digest; + } + } + searchFrom = index + FULL_OUTPUT_DIGEST_LABEL.length; + } +} + export async function persistAndTruncateToolResult( callId: string, toolName: string, @@ -528,8 +557,21 @@ function buildStub( // sha256 of the FULL pre-truncation output (see FULL_OUTPUT_DIGEST_LABEL): // the envelope's per-call unique path would otherwise fingerprint uniquely // every poll, silently disabling every result-aware loop guard for exactly - // the largest results (issue #9450). - const fullDigest = crypto.createHash('sha256').update(content).digest('hex'); + // the largest results (issue #9450). When the content already carries a + // producer-anchored digest, reuse it: shell.ts failure blocks anchor a + // sha256 of the stable failure core (Output/Error/Exit Code/Signal), and + // hashing the full block here instead would fold the per-call volatile + // Command:/PGID lines into the envelope digest — every retry of the + // identical failure would then fingerprint uniquely and the + // error-repetition streak would never accumulate for failures in the + // (scheduler persistence gate, shell in-tool threshold] size band + // (issue #10887). Content without an anchored digest keeps the + // full-content hash: the preview only covers the first PREVIEW_SIZE_CHARS + // chars, so the digest must stay sensitive to mutations anywhere in the + // full content (issue #9450). + const fullDigest = + extractAnchoredFullDigest(content) ?? + crypto.createHash('sha256').update(content).digest('hex'); if (isFilePath) { return ` From d136ac99d8ce7339ffdd056b72658f8a86dcfaea Mon Sep 17 00:00:00 2001 From: "jinjing.zzj" Date: Fri, 4 Sep 2026 22:06:43 +0800 Subject: [PATCH 10/10] fix(core): correct cancel-prefix JSDoc and surrogate-safe error_excerpt - CANCELLED_TOOL_ERROR_PREFIX's JSDoc named a nonexistent second producer site (handleCancelToolCalls); the real auxiliary-cancel producer is the 'cancelled' case of setStatusInternal. Point auditors at the real site so a wording change there cannot silently break the cancellation exclusion in the error-repetition guard. - LoopDetectedEvent.error_excerpt truncated via plain slice(0, 200) could split an astral character (emoji/CJK-extension text in tool output) straddling the cut, leaving an unpaired high surrogate that strict UTF-8/JSON telemetry consumers reject. Drop a trailing lone high surrogate after the slice; regression-tested (red-proofed). Co-authored-by: Qwen-Coder Patrol-Run: qwen-pr-closeout/jmtmxfessij --- packages/core/src/core/coreToolScheduler.ts | 2 +- packages/core/src/telemetry/loggers.test.ts | 29 +++++++++++++++++++++ packages/core/src/telemetry/types.ts | 12 +++++++-- 3 files changed, 40 insertions(+), 3 deletions(-) diff --git a/packages/core/src/core/coreToolScheduler.ts b/packages/core/src/core/coreToolScheduler.ts index 2a894a2f633..b279b260ae9 100644 --- a/packages/core/src/core/coreToolScheduler.ts +++ b/packages/core/src/core/coreToolScheduler.ts @@ -954,7 +954,7 @@ const createErrorResponse = ( /** * Prefix of the model-visible error payload this scheduler produces for * cancelled tool calls (see createCancelledResponse and the auxiliary-cancel - * path in `handleCancelToolCalls`). Exported so consumers that must + * path in the `'cancelled'` case of `setStatusInternal`). Exported so consumers that must * recognize cancellation payloads — the loop-detection error-repetition * guard (services/loopDetectionService.ts) — match the producer-owned * constant instead of re-declaring the literal (producer-owns-the-shape diff --git a/packages/core/src/telemetry/loggers.test.ts b/packages/core/src/telemetry/loggers.test.ts index cd801295def..84f51851ad2 100644 --- a/packages/core/src/telemetry/loggers.test.ts +++ b/packages/core/src/telemetry/loggers.test.ts @@ -561,6 +561,35 @@ describe('loggers', () => { }); }); + describe('LoopDetectedEvent error_excerpt truncation', () => { + it('truncates error_excerpt to 200 chars', () => { + const event = new LoopDetectedEvent(LoopType.REPEATED_TOOL_ERROR, 'p', { + errorSignature: 'sig', + errorExcerpt: 'a'.repeat(250), + }); + expect(event.error_excerpt).toBe('a'.repeat(200)); + }); + + it('does not end error_excerpt on a lone high surrogate', () => { + // The astral character straddles code-unit indices 199/200: a plain + // slice(0, 200) would leave an unpaired high surrogate that strict + // UTF-8/JSON consumers reject (issue #10887 telemetry fields). + const event = new LoopDetectedEvent(LoopType.REPEATED_TOOL_ERROR, 'p', { + errorSignature: 'sig', + errorExcerpt: 'a'.repeat(199) + '🙂', + }); + expect(event.error_excerpt).toBe('a'.repeat(199)); + }); + + it('keeps a complete astral character that fits the 200-char cut', () => { + const event = new LoopDetectedEvent(LoopType.REPEATED_TOOL_ERROR, 'p', { + errorSignature: 'sig', + errorExcerpt: 'a'.repeat(198) + '🙂', + }); + expect(event.error_excerpt).toBe('a'.repeat(198) + '🙂'); + }); + }); + describe('logUserPrompt', () => { const mockConfig = { getSessionId: () => 'test-session-id', diff --git a/packages/core/src/telemetry/types.ts b/packages/core/src/telemetry/types.ts index f4dbc92fc10..ac591ba0744 100644 --- a/packages/core/src/telemetry/types.ts +++ b/packages/core/src/telemetry/types.ts @@ -523,8 +523,16 @@ export class LoopDetectedEvent implements BaseTelemetryEvent { this.error_signature = details.errorSignature; } if (details?.errorExcerpt !== undefined) { - // Truncate for telemetry (avoid shipping full tool payloads). - this.error_excerpt = details.errorExcerpt.slice(0, 200); + // Truncate for telemetry (avoid shipping full tool payloads). Drop a + // trailing lone high surrogate: the slice can split an astral + // character (emoji/CJK-extension text in tool output) straddling the + // 200-char cut, and strict UTF-8/JSON consumers reject an unpaired + // surrogate (same hazard fitText handles via its surrogate-safe + // slices, tools/tool-response-finalizer.ts). + const cut = details.errorExcerpt.slice(0, 200); + this.error_excerpt = /[\uD800-\uDBFF]$/.test(cut) + ? cut.slice(0, -1) + : cut; } } }