Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 46 additions & 0 deletions packages/cli/src/nonInteractiveCli.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2527,6 +2527,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 = {
Expand Down
5 changes: 4 additions & 1 deletion packages/cli/src/nonInteractiveCli.ts
Original file line number Diff line number Diff line change
Expand Up @@ -198,6 +198,8 @@ const LOOP_TYPE_LABELS: Record<LoopType, string> = {
'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 {
Expand All @@ -212,7 +214,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;
Comment thread
yiliang114 marked this conversation as resolved.
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).'
Expand Down
13 changes: 13 additions & 0 deletions packages/core/src/agents/runtime/agent-core.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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)) {
Comment on lines +1250 to +1253

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[Suggestion] R3-2: The AgentCore (headless/sub-agent) half of this guard's runtime wiring has no test — every wiring test targets the LlmClient path in client.test.ts. This is the still-open sibling of R2-2 ("neither runtime wiring site of this guard is exercised by any test"), whose client.ts half was fixed in 8a8240c; the agent-core half remains.

Deleting this block — or a refactor changing toolCallResult.results so error parts no longer flow — ships fully green: nothing under packages/core/src/agents/** references recordToolErrorBatch or REPEATED_TOOL_ERROR. The #10887 dead-end burn (repeated failing calls with varied args) would then continue unbounded inside Task-tool sub-agents — the runtime most prone to unsupervised token spend — and CI would never notice.

Witness:

mutant run (recordToolErrorBatch block deleted from agent-core.ts):
agent-headless.test.ts: Tests 68 passed (68) — ships green
grep packages/core/src/agents/** for recordToolErrorBatch|REPEATED_TOOL_ERROR -> exactly one hit: agent-core.ts:1253 itself

Fix direction: add a test in agent-headless.test.ts modelled on the existing 'should stop consecutive identical tool calls with fresh ids' test (~line 2172): a tool whose invocation returns a functionResponse carrying { error: 'fatal: not a git repository' } for 3 rounds with varied args per round; expect AgentTerminateMode.LOOP_DETECTED with loopType repeated_tool_error. The mocked tool result must place the failure at functionResponse.response['error'] (extractToolErrors skips non-string payloads), not in llmContent. The new test is its own witness — it goes red (terminates MAX_TURNS instead of LOOP_DETECTED) if this wiring is removed; please confirm by deleting the block and watching it fail.

中文说明

守卫运行时接线的 AgentCore(无头/子 agent)一半没有任何测试——所有接线测试都针对 client.test.ts 中的 LlmClient 路径。这是 R2-2(「守卫的两个运行时接线站点均无测试」)仍未关闭的一半:client.ts 一半已在 8a8240c 修复,agent-core 一半仍缺。

删除这个块——或某个使错误 part 不再流入的 toolCallResult.results 重构——都能全绿通过:packages/core/src/agents/** 下没有任何代码引用 recordToolErrorBatchREPEATED_TOOL_ERROR#10887 的死循环烧 token(变体参数的重复失败调用)将在 Task 工具子 agent 内部无界继续——那是最容易无监督消耗 token 的运行时——而 CI 永远不会察觉。

见证(见英文段):删除 agent-core.ts 中 recordToolErrorBatch 块的变异体下,agent-headless.test.ts 68 个用例全部通过。

修复方向:在 agent-headless.test.ts 中仿照现有 'should stop consecutive identical tool calls with fresh ids'(约 2172 行)新增用例:某工具的调用返回携带 { error: 'fatal: not a git repository' }functionResponse,连续 3 轮、每轮参数各异;断言以 loopType repeated_tool_error 终止于 AgentTerminateMode.LOOP_DETECTED。mock 的工具结果必须把失败放在 functionResponse.response['error']extractToolErrors 跳过非字符串载荷),而不是 llmContent。新测试自身即见证——移除该接线后应变红(以 MAX_TURNS 而非 LOOP_DETECTED 终止);请删除该块确认其失败。

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

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Verified at head d136ac9: grep -rn 'recordToolErrorBatch\|REPEATED_TOOL_ERROR' packages/core/src/agents/ returns exactly one hit — agent-core.ts:1253 itself — so the AgentCore/headless wiring is indeed unexercised, while the client.ts half got its wiring test (client.test.ts:8445ff, from 8a8240c). The modelled test exists at agent-headless.test.ts:2172.

Deferred, not declined: this closeout round has a scope fuse (PR additions 1559 > 1500 → no code changes, replies only), so the suggested wiring test cannot land in this pass. Leaving the thread unresolved to keep the signal visible; the proposed shape (3 rounds × varied args, error at functionResponse.response['error'], expect LOOP_DETECTED/repeated_tool_error) is the right one for the follow-up.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Confirmed. grep -rn "recordToolErrorBatch\|REPEATED_TOOL_ERROR" packages/core/src/agents/ returns exactly one hit — agent-core.ts:1253, the production line itself. Test files referencing recordToolErrorBatch are core/client.test.ts and services/loopDetectionService.test.ts only; nothing under agents/**. So deleting the block at agent-core.ts:1244-1256 ships green, and the headless/sub-agent runtime — the one most exposed to unsupervised spend — keeps the #10887 dead end unbounded.

Test to add (not added this round): packages/core/src/agents/agent-headless.test.ts, sibling of 'should stop consecutive identical tool calls with fresh ids' (~:2172). Tool whose invocation returns a functionResponse with response: { error: 'fatal: not a git repository' } for 3 rounds, args varied per round; assert AgentTerminateMode.LOOP_DETECTED with loopType repeated_tool_error. The failure must sit at functionResponse.response.errorextractToolErrors skips non-string payloads (loopDetectionService.ts:780-781).

PR is at +1562, over this pass's 1500-line cap, so no code changes this round. Leaving unresolved.

Comment thread
yiliang114 marked this conversation as resolved.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[Suggestion] R3-2: The AgentCore half of this guard's runtime wiring is still exercised by no test — carried from round 3 and re-derived independently three times this round. A grep over packages/core/src/agents/** for recordToolErrorBatch, REPEATED_TOOL_ERROR and repeated_tool_error returns exactly one hit, this line itself; the diff adds tests only for client.ts and for the service in isolation. The call sits behind if (terminateMode !== AgentTerminateMode.LOOP_DETECTED) immediately after a for loop that breaks with terminateMode already set, and its input is a flatMap over toolCallResult.results, so any reorder of that branch, an earlier break/return added to the round, or processFunctionCalls ceasing to return every executed result silently deletes the guard from the subagent runtime with the whole suite green. The cost lands on the runtime nobody is watching: a subagent that hits the reported dead end keeps re-running it to its round or time budget, burning tokens with no user in the loop and no repeated_tool_error attribution in the subagent journal. Equally unpinned is the batch shape — moving this feed inside the per-result loop directly above would make one parallel batch of three identical failures trip the threshold before the model has seen any error, and no runtime-level test would notice. The harness already exists: agent-headless.test.ts pins the sibling result-aware guard from issue 9450 at :2245, :2333 and :2414.

Witness:

witness: not run — grep over packages/core/src/agents for recordToolErrorBatch /
REPEATED_TOOL_ERROR / repeated_tool_error returns 1 hit (agent-core.ts:1253, the source
line); no test file matches. The mutation-survival claim is read from the call graph.

Add two tests to agent-headless.test.ts in the issue-9450 harness: one driving three consecutive rounds whose tool results carry a byte-identical functionResponse.response.error and asserting scope.getTerminateMode() is AgentTerminateMode.LOOP_DETECTED with the finish event's loopType equal to 'repeated_tool_error'; and one driving a single round carrying three sibling calls with the same error, asserting no halt, then two more such rounds, asserting the halt.

Two premises: the guard is deliberately always-on, unlike the heuristic tier in the same file — !this.runtimeContext.getSkipLoopDetection() && gates only loopDetector.addAndCheckHeuristicLoops(event) (agent-core.ts:941-942) and recordToolErrorBatch returns early only on this.loopDetected / this.disabledForSession (loopDetectionService.ts:623-627) — so a test that sets getSkipLoopDetection() true and still expects the halt is asserting intended behaviour, not a bug; and the duplicate-provider-call path breaks out before this guard runs (agent-core.ts:1225), so a fixture reusing the duplicate-call shape from agent-headless.test.ts:2169 never reaches recordToolErrorBatch and would assert a halt for the wrong reason. The acceptance criterion is that deleting this block turns the first test red on both the terminate mode and the loopType assertion, and moving the feed into the per-result loop turns the second red on round 1 instead of round 3, while a varying-error negative control stays green — please apply both mutations and confirm.

中文说明

该守卫运行时接线的 AgentCore 一半仍然没有任何测试覆盖——本条承接第 3 轮,并在本轮被独立地重新发现三次。在 packages/core/src/agents/** 中检索 recordToolErrorBatchREPEATED_TOOL_ERRORrepeated_tool_error,只有一处命中,即本行自身;本 diff 只为 client.ts 与该服务的孤立单元测试新增了用例。该调用位于 if (terminateMode !== AgentTerminateMode.LOOP_DETECTED) 之后、紧随一个会以已置位的 terminateMode 执行 breakfor 循环,其输入是对 toolCallResult.resultsflatMap;因此该分支的任何重排、轮次中提前加入的 break/return、或 processFunctionCalls 不再返回每个已执行结果,都会在全套测试保持绿色的情况下悄悄把守卫从子 agent 运行时中删除。代价落在无人观察的运行时上:命中所述死循环的子 agent 会一直重跑到其轮次或时间预算,持续消耗 token,既无用户介入,子 agent 日志中也没有 repeated_tool_error 归因。批次形态同样未被钉住——把这个喂入移进紧邻上方的逐结果循环,会让一次含三个相同失败的并行批次在模型看到任何错误之前就触发阈值,而运行时层面的测试不会察觉。测试骨架已存在:agent-headless.test.ts 在 :2245、:2333、:2414 钉住了 issue 9450 的同类结果感知守卫。

见证见上方英文段(未执行:检索仅 1 处命中即源码行本身,无测试文件匹配;变异存活结论来自调用图阅读)。

请在 agent-headless.test.ts 的 issue-9450 骨架中新增两个用例:其一,驱动连续三轮、工具结果携带字节一致的 functionResponse.response.error,断言 scope.getTerminateMode()AgentTerminateMode.LOOP_DETECTED 且结束事件的 loopType 等于 'repeated_tool_error';其二,驱动一轮携带三个同类调用(同一错误),断言不终止,再驱动两轮同样的批次,断言终止。

两个前提:与同文件中的启发式层级不同,该守卫是刻意「始终开启」的——!this.runtimeContext.getSkipLoopDetection() && 只约束 loopDetector.addAndCheckHeuristicLoops(event)(agent-core.ts:941-942),而 recordToolErrorBatch 仅在 this.loopDetected / this.disabledForSession 时提前返回(loopDetectionService.ts:623-627)——因此把 getSkipLoopDetection() 设为 true 仍期望终止的测试,断言的是预期行为而非缺陷;另外重复的 provider 调用路径会在该守卫运行前 break(agent-core.ts:1225),所以复用 agent-headless.test.ts:2169 的重复调用形态的夹具永远到不了 recordToolErrorBatch,会因错误的原因断言终止。验收标准是:删除该代码块后第一个用例在终止模式与 loopType 两个断言上都变红;把喂入移进逐结果循环后第二个用例在第 1 轮而非第 3 轮变红;而变化错误的反向对照保持绿。请分别施加这两种变异并确认。

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

terminateMode = AgentTerminateMode.LOOP_DETECTED;
}
}
if (terminateMode === AgentTerminateMode.LOOP_DETECTED) {
break;
}
Expand Down
89 changes: 89 additions & 0 deletions packages/core/src/core/client.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8478,6 +8478,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) => {
Expand Down
49 changes: 32 additions & 17 deletions packages/core/src/core/client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3940,6 +3940,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' ||
Expand All @@ -3950,29 +3952,42 @@ 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(
undefined,
'loop detected',
)) {
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);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[Critical] R6-1: [certifies-falsely] [new-surface] This halt returns the turn before the round's tool results are written to chat history, so the model's functionCall turn is left unpaired and the next send's orphan repair tells the model that results which really arrived were lost to a crash.

turn.run(model, requestToSend, signal) at client.ts:4087 is the only place this round's content enters history (llm-chat.ts:2997-2998, "Add user content to history ONCE before any attempts"), and the shared halt block below this feed ends in return turn. So on the round the guard trips, history ends with model[functionCall] and no answering user[functionResponse].

Nothing compensates on the interactive path. The TUI already called markToolsAsSubmitted before submitting (use-llm-stream.ts:5479), and its LoopDetected handler only opens a dialog (:2450-2477) — unlike every other non-submitting branch, which writes the pairing explicitly for exactly this reason (:5377-5381: "unless the responses are written now the model's function calls stay unanswered and the next /goal resume sends a history with an unpaired call"; also :5406, :5485, :5635, :5663). The dialog's disable arm then tells the user "Loop detection has been disabled for this session. Please try your request again.", and that next send runs repairOrphanedToolUseTurns (llm-chat.ts:3011-3014), synthesizing Tool execution result was not recorded — likely interrupted by network failure, abort, or process exit. Treat as failure and retry if needed. (llm-chat.ts:1526-1528). The model is therefore told that a call which did execute — and whose result the user watched land on screen — was lost to a crash and should be retried, at the precise moment the user has disabled every guard (disableForSession is honored by the always-on tier too, loopDetectionService.ts:625-628). The dead-end retry this halt exists to stop becomes unbounded, and the real result is absent from context.

This is newly reachable rather than merely pre-existing: at the merge base the only detector that could halt at this pre-send point was recordToolResultByCallId, which needs a requestByCallId entry populated only for STATEFUL_READ_TOOLS (task_list), so an ordinary run_shell_command error round could not reach it. One caveat in fairness — the invariant violation itself is not brand new: a frozen task_list board tripping GLOBAL_TOOL_CALL_DUPLICATE reached the same return turn at base. This diff widens it from one stateful-read polling path to every tool-error round. The agent-runtime arm is a different harm: agent-core.ts:1253-1255 breaks before currentMessages = toolCallResult.messages at :1260, but that run terminates, so there is no next send and no fabricated repair — there the cost is dropped productive work. Headless also exits 1 on the halt, so the falsified-history harm needs a session that survives it: the interactive TUI, or a --resume of the persisted transcript.

Witness:

probe — the PR's own client.test.ts harness, with mockTurnRunFn delegating to the REAL
LlmChat.sendMessageStream (so the history push and the per-send orphan repair are the
product's own code, not a model), real LoopDetectionService, args varied per round.
Comparator validated: rounds 0-2 show turnRunCalled=true AND the real functionResponse
landing in history, so it can report a difference.

PR arm (unmodified):
  PROBE_ROUND 3 turnRunCalled=false chatSendCount=3 events=["loop_detected"]
  PROBE_HISTORY after_round_3 len=7
    [...,{"role":"model","parts":["functionCall(run_shell_command#fail-2)"]}]  <- ends UNPAIRED
  PROBE_FUNCTION_RESPONSES_IN_HISTORY [..., {"id":"fail-2","name":"run_shell_command",
    "response":{"error":"Tool execution result was not recorded — likely interrupted by
    network failure, abort, or process exit. Treat as failure and retry if needed."}}]

Mutant arm (loopHalt = false in place of recordToolErrorBatch — models the merge base):
  PROBE_HALT round=-1 loopType=undefined
  PROBE_FUNCTION_RESPONSES_IN_HISTORY [fail-0..fail-4 all =
    {"error":"fatal: not a git repository (or any of the parent directories): .git"}]
    <- no fabricated entry for any executed round

base-axis read at merge base 101b003f93: recordToolErrorBatch and REPEATED_TOOL_ERROR
have zero grep hits; requestByCallId is populated only under
`if (event.value.callId && stateful)`.

Fix direction: on the pre-send loopHalt path, pair the unanswered calls with the real results instead of dropping them — this.getChat().addHistory({ role: 'user', parts: toolResultParts }) before return turn, mirroring use-llm-stream.ts:5406 — and in agent-core.ts assign currentMessages = toolCallResult.messages (or append the finalized parts to the run's history) before the break at :1257. If the intent is that the model must not see round 3's error, write a truthful synthetic response for those callIds rather than leaving the pair dangling, so the repair's "not recorded / interrupted by network failure, abort, or process exit" text is never injected for a call that executed.

The pairing write must fire only when turn.run is skipped: llm-chat.ts:2997-2998 pushes the user content once before any attempts, so a write on a path that also sends would duplicate the round's functionResponses — the hazard restoreStrippedRetryEntries gates on the push counter for at client.ts:3113-3143.

Please extend the new halt test in client.test.ts (helper runFailingToolTurns, :8489-8546) with a history assertion — after the LoopDetected event, client.getChat().getHistory() contains a role: 'user' entry whose functionResponse.id === 'fail-2', equivalently repairOrphanedToolUseTurns(history) returns injected: [] — then remove the history write and confirm that test goes red. Both tests as added stay green either way today, which is the gap.

中文说明

R6-1:[certifies-falsely] [new-surface] 这个终止路径在本轮工具结果写入对话历史之前就 return turn 了,导致模型的 functionCall 轮次没有配对;下一次发送时的 orphan 修复会告诉模型,那些确实已经返回的结果「因崩溃/中断而丢失,请重试」。

client.ts:4087turn.run(model, requestToSend, signal) 是本轮内容进入历史的唯一入口(llm-chat.ts:2997-2998,注释为「Add user content to history ONCE before any attempts」),而这个喂入点下方的共享终止块以 return turn 结束。因此守卫触发的那一轮,历史会以 model[functionCall] 结尾,没有配对的 user[functionResponse]

交互路径上没有任何补偿:TUI 在提交前已经调用了 markToolsAsSubmitteduse-llm-stream.ts:5479),而它的 LoopDetected 处理只弹一个对话框(:2450-2477)——与其他所有「不提交」分支不同,那些分支都显式写入了配对,理由正是这个(:5377-5381:「unless the responses are written now the model's function calls stay unanswered and the next /goal resume sends a history with an unpaired call」;另见 :5406:5485:5635:5663)。对话框的「禁用」分支接着告诉用户「Loop detection has been disabled for this session. Please try your request again.」,而下一次发送会执行 repairOrphanedToolUseTurnsllm-chat.ts:3011-3014),合成出「Tool execution result was not recorded — likely interrupted by network failure, abort, or process exit. Treat as failure and retry if needed.」(llm-chat.ts:1526-1528)。于是模型被告知:一个确实执行过(而且用户已在屏幕上看到结果)的调用因崩溃丢失、应当重试——而此时用户刚好禁用了所有守卫(disableForSession 对 always-on 层同样生效,loopDetectionService.ts:625-628)。这个终止本要阻止的死循环重试变成无界的,真实结果也不在上下文里。

这是新引入的可达路径,不只是既有问题:在 merge base 上,唯一能在该「发送前」位置终止的检测是 recordToolResultByCallId,它需要 requestByCallId 中存在条目,而该映射只为 STATEFUL_READ_TOOLStask_list)填充,所以普通的 run_shell_command 错误轮根本到不了这里。公平起见也要说明一点:这个不变式被破坏本身并非全新——base 上一个冻结的 task_list 看板触发 GLOBAL_TOOL_CALL_DUPLICATE 也会走到同一个 return turn;本 diff 把它从「一条 stateful-read 轮询路径」扩大到「每一个工具错误轮」。agent 运行时那半边是另一种损害:agent-core.ts:1253-1255:1260currentMessages = toolCallResult.messages 之前 break,但那次 run 随即终止,不存在「下一次发送」,也就没有伪造修复——那里的代价是被丢弃的已完成工作。headless 在终止时直接 exit 1,所以「历史被伪造」这一损害需要一个在终止后仍存活的会话:交互式 TUI,或对已持久化 transcript 执行 --resume

修复方向:在发送前的 loopHalt 路径上,用真实结果去配对那些未被应答的调用,而不是直接丢弃——在 return turn 之前执行 this.getChat().addHistory({ role: 'user', parts: toolResultParts })(参照 use-llm-stream.ts:5406);在 agent-core.ts 中,于 :1257 的 break 之前赋值 currentMessages = toolCallResult.messages(或把 finalize 后的 parts 追加到该 run 的历史)。如果设计意图就是「不让模型看到第 3 轮的错误」,也请为这些 callId 写入如实的合成响应,而不是让配对悬空,从而避免把「not recorded / interrupted by network failure, abort, or process exit」这段文字注入到一个确实执行过的调用上。

约束:llm-chat.ts:2997-2998 是「任何 attempt 之前只写入一次用户内容」,因此配对写入必须只在 turn.run 被跳过时发生;在同时也会发送的路径上写入会重复本轮的 functionResponse——这正是 client.ts:3113-3143restoreStrippedRetryEntries 用 push 计数器去防范的问题。

请在 client.test.ts 新增的终止用例(辅助函数 runFailingToolTurns:8489-8546)里补一条历史断言:LoopDetected 事件之后,client.getChat().getHistory() 中存在一个 role: 'user' 条目,其 functionResponse.id === 'fail-2'(等价地,repairOrphanedToolUseTurns(history) 返回 injected: []);然后移除该历史写入,确认这个测试变红。今天这两个新增用例无论有没有该写入都是绿的,这正是缺口所在。

— qwen3.8-max-2026-09-02 via Qwen Code /review (v0.23.0)

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Verdict: REAL — traced end to end at head 6439e0921d. Not fixed this round (scope note at the bottom); leaving this thread unresolved.

Trigger path I read:

  • client.ts:3971loopHalt = this.loopDetector.recordToolErrorBatch(toolResultParts), inside the SendMessageType.ToolResult branch; toolResultParts is every functionResponse part of that message, collected in the loop above it (the same loop that feeds recordToolResultByCallId at :3957).
  • The shared halt block is client.ts:3973-3991 and ends in return turn; at :3990.
  • requestToSend only enters history inside turn.run(model, requestToSend, signal) at client.ts:4087llm-chat.ts:2997-2998 (// Add user content to history ONCE before any attempts. / this.history.push(userContent);). Returning at :3990 skips it, so history ends on model[functionCall] with no answering user[functionResponse].
  • Nothing on the interactive path compensates. use-llm-stream.ts:2476-2481 (handleLoopDetectedEvent) only sets loopDetectionConfirmationRequest; handleLoopDetectionConfirmation (:2450-2473) just addItems text, and its disable arm prints "Loop detection has been disabled for this session. Please try your request again." Contrast the cancelled-batch branch at use-llm-stream.ts:5380-5382, which calls llmClient.addHistory({ role: 'user', parts: responsesToSend }) for exactly this reason — see the comment at :5373-5379 ("unless the responses are written now the model's function calls stay unanswered and the next /goal resume sends a history with an unpaired call"). markToolsAsSubmitted already ran at :5479, so those callIds are never re-submitted.
  • The next send then runs the inline repair at llm-chat.ts:3011-3014, injecting ORPHAN_TOOL_USE_REPAIR_REASON (llm-chat.ts:1526-1528): "Tool execution result was not recorded — likely interrupted by network failure, abort, or process exit. Treat as failure and retry if needed." So the model is told results that did arrive were lost to a crash, and is invited to retry the very calls the guard just halted on — with loop detection now disabled for the session if the user picked that arm.
  • Same shape in the subagent runtime: agent-core.ts:1250-1253 feeds the round, sets LOOP_DETECTED at :1254, and breaks at :1258 — before currentMessages = toolCallResult.messages at :1260.

One correction to the [new-surface] tag, because it changes who owns the fix: the return turn before the history push is not new. origin/main client.ts:3953-3975 returns from the identical place for the recordToolResultByCallId (#9450) detection. What this PR changes is the reach — #9450 only fires for stateful-read tools (task_list), while recordToolErrorBatch is always-on (loopDetectionService.ts:623-627 checks only loopDetected and disabledForSession; it is not behind model.skipLoopDetection) and fires on any tool error repeating three rounds. A narrow pre-existing edge becomes a routine path, so fixing it here is right.

Minimal fix (not applied): write the pairing before returning — in the halt block at client.ts:3973-3991, push this round's parts into history before return turn, e.g. this.getChat().addHistory(createUserContent(toolResultParts)) (getChat().addHistory is the receiver already used at client.ts:670-671, createUserContent is imported at :16), and mirror it in the agent-core.ts:1253 twin by assigning currentMessages = toolCallResult.messages before the break. Two production files plus tests. Whoever implements it should also decide whether the pre-send microcompactHistoryBeforeSend call at the end of the same branch needs to see that content, since the write now happens without a send.

Why it is not applied this round: the PR is at +1562/−45, over the 1500-addition scope fuse this closeout sweep runs under, and it is in round 17 with findings still being minted. This needs an owner decision — lift the fuse for a targeted fix, or split the guard out. A reply alone does not close a real Critical, so the thread stays unresolved.

}
Comment thread
yiliang114 marked this conversation as resolved.
if (loopHalt) {
for (const goalEvent of await finalizeInterruptedGoalTurn(
undefined,
'loop detected',
)) {
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');
Expand Down
15 changes: 13 additions & 2 deletions packages/core/src/core/coreToolScheduler.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1035,6 +1035,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 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
* 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,
Expand All @@ -1045,7 +1056,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: [
Expand Down Expand Up @@ -1755,7 +1766,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,
)
Expand Down
Loading
Loading