feat(acp): Protect against repeated tool execution failures - #8469
Conversation
Add a conservative prompt-local guard for repeated typed ACP tool execution failures, with shadow/warn/enforce rollout modes, privacy-safe telemetry, and coverage for the final execution outcome contract. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Review: repeated tool execution failure guardOverviewAdds a prompt-local, in-memory guard that detects the same Structure is good: The findings below are mostly about scope, operability, and rollout evidence quality, not about the reducer's logic. Blocking / should fix before leaving Draft1.
The design doc says cron/notification/background routes stay 2.
3. The guard is completely inert for ACP hosts that don't implement If the host doesn't implement the ext method, This is a correct fail-safe, but a seven-day shadow baseline that reads "no candidates" for those hosts is indistinguishable from "no repeated failures happen". Please state this dependency in the PR body and design doc, and consider counting 4. The repo documents operator env vars in 5. Unrecognized values silently resolve to switch (value?.trim().toLowerCase()) {
case 'off': case 'shadow': case 'warn': case 'enforce': return ...;
default: return 'shadow';
}
Medium6. Once a 7. ...(privacyRestricted ? {} : getCommonAttributes(config)),The stated goal is excluding session-scoped RUM and 8. Metric cardinality vs. the "low-cardinality" claim
9. More than one reminder per prompt is reachable The PR says "Warn mode injects one fixed corrective reminder". The reducer allows a streak to reset (e.g. one success) and rebuild to 10. Stop path returns a hardcoded Sibling terminal paths use Minor / nits
Test coverageThe reducer suite is strong: table-driven over modes, reset reasons, contract violations, and downgrade paths. The telemetry tests are better than average — the serialized-payload regex assertion against Gaps, in priority order:
Security & performanceNo injection or data-exposure concerns found. Guard text is fixed, never interpolated from tool output or error messages; telemetry excludes args, outputs, paths, raw messages, and MCP server names, and the tests assert it. Runtime cost is O(batch size) per tool batch on an already-async boundary — negligible. The only added I/O is the widened |
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
审计与修复已推送到
Shadow 在 最终本地验证：CLI 554 tests、Channels 91 tests、Core 128 tests， |
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
Review: repeated tool execution failure guard
The state machine itself is clean, well-isolated, and the test coverage (reducer table tests + 10 Session-level integration tests including cancellation races) is genuinely good. The conservative eligibility rules match the design doc, the off/channel/unreliable-host escape hatches all fail open, and the PROJECT_ENV_HARDCODED_EXCLUSIONS entry is the right call for an operator policy knob.
My concerns are concentrated in the telemetry design (which is the whole point of the shadow rollout) and in a few reachability / single-source-of-truth issues. Details inline; summarized here by priority:
Blocking-ish
- The SHA-256 prompt-id digest provides no real privacy but does break correlation.
promptIdis${sessionId}########${n}(see your own test assertingsha256('test-session-id########1')). Unsalted SHA-256 over that is trivially confirmable by anyone holding the session id —nis a small integer. Meanwhile the rawpromptIdis already emitted bylogToolCall(prompt_id: promptId) on the same pipeline, with common attributes carrying the session id. So nothing is actually hidden, and in exchange the terminal stop event loses its join to the session, the service version, and the rest of the prompt — exactly the data a staged rollout decision needs. See Session.ts:491. logLoopDetectednow branches onloop_typeinside a shared core API and drops bothQwenLoggerandgetCommonAttributesfor one loop type. That's hidden coupling inpackages/coredriven by one CLI feature. See loggers.ts.- Three of the four new
stoppedByRepeatedToolFailurebranches are unreachable —#runStopContinuation, the Stop-hook route and the background route all build their loop state with the default'off', and the reducer short-circuits on'off'. Dead + untestable code that reads as if those routes can be stopped. See Session.ts:4510.
Should fix
4. Eligibility is decided by elimination over ToolExecutionStatus, and the telemetry then hardcodes terminal_status: 'error' / execution_status: 'error'. Correct today; silently wrong the day the union gains a member.
5. CHANNEL_PROMPT_META_KEY is duplicated as a bare string literal across packages/channels/base and packages/cli, with no test tying them together — and a desync fails in the dangerous direction (channel prompts start enforcing).
Minor
6. Default shadow changes the drain request payload for every ACP host on day one, under a now-misleading todoStopGuardWatchQueuedPrompt name.
7. reset_reason attribution is order-dependent for mixed batches.
8. guardContext object smuggle + inconsistent executionErrorType gating between the two queueToolResultRecord sites.
Nothing here suggests the guard would misfire in shadow, so the rollout plan is sound. Items 1–3 are what I'd want settled before the shadow baseline starts collecting, since they affect whether that baseline is analyzable at all.
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
Review: the signal is right, the placement and the eligibility rule are not
The new signal — keying repetition on tool execution outcome rather than on the tool request — is genuinely missing today: LoopDetectionService only consumes ToolCallRequest / Content / Thought / Retry / Finished, so "same tool keeps failing for the same classified reason" is invisible to it. That part is worth having, and the reducer itself is good code: pure, well-tested, clear state machine.
Three concerns at the design level.
1. This belongs in packages/core, not in the ACP session layer
packages/core/src/services/loopDetectionService.ts is already the home for this concept: prompt-local lifecycle, LoopType emission, disableForSession, and an existing two-tier split (checkAlwaysOnSafeties always-on vs addAndCheckHeuristicLoops gated by model.skipLoopDetection) — which is essentially what shadow/warn/enforce re-implements from scratch.
Putting the guard in Session.ts means TUI (core/client.ts), headless (nonInteractiveCli.ts) and subagents (agents/runtime/agent-core.ts) get nothing. A tell: this PR has to add a REPEATED_TOOL_EXECUTION_FAILURE label to nonInteractiveCli.ts, but that branch is unreachable under the current architecture — it exists only to satisfy Record<LoopType, string>.
The natural anchor is CoreToolScheduler.onAllToolCallsComplete (coreToolScheduler.ts:5872) — the settled-batch boundary that already exists in core and is shared by useReactToolScheduler, nonInteractiveCli, agent-core and nonInteractiveToolExecutor. CompletedToolCall already carries request.name, tool (native vs DiscoveredMCPTool), response.errorType, response.executionStatus and status — exactly the fields RepeatedToolFailureObservation defines.
Note for anyone reaching for it: GeminiEventType.ToolCallResponse looks like an easier hook but is a dead enum member — nothing in core ever emits it.
Sketch:
- Move the reducer to
core/src/services/repeatedToolFailureReducer.ts(near-zero change; derive the observation fromCompletedToolCall). - Add
LoopDetectionService.recordSettledToolBatch(calls: readonly CompletedToolCall[]): boolean; clear it in the existingreset()so the prompt-local lifecycle comes for free; report through the existinglogLoopDetected+lastLoopType. - Call it where
onAllToolCallsCompletefires — one site, four consumers. - Wire it into
addAndCheckHeuristicLoops(gated bymodel.skipLoopDetection, defaulttrue= off), notcheckAlwaysOnSafeties. This makesQWEN_CODE_ACP_REPEATED_TOOL_FAILURE_GUARD, thePROJECT_ENV_HARDCODED_EXCLUSIONSentry and theshared-env-keys.tschange unnecessary. - ACP deliberately bypasses
CoreToolScheduler(see the comment atSession.ts:7039), so it keeps one feed call fromfinalizeRunToolResult. Session then only retains what is genuinely ACP-specific: history preservation on stop,emitAgentMessage,todoStopGuard.suspend()— roughly 15 lines instead of 285. - With the guard in core and gated by config, the
_metachannel marker can be dropped entirely (see the inline note onChannelAgentBridge.ts).
2. The eligibility rule gives near-zero recall and misses the dominant loop shape
The reducer requires every non-duplicate observation in a batch to share one key, and any single success resets the whole streak. So the most common expensive loop is undetectable by construction:
batch 1: run_shell("npm test") -> error
batch 2: read_file(x) ; edit(x) -> success, success <-- streak reset
batch 3: run_shell("npm test") -> error
batch 4: read_file(y) ; edit(y) -> success, success <-- reset again
...
What remains is "at least two consecutive batches containing nothing but the same failing tool" — and much of that is already covered by checkToolCallLoop (consecutive identical name+args) and the per-turn cap. The genuine increment is a thin slice: same tool, same ToolErrorType, different args, >= 8 times, >= 2 pure batches, zero successes. That is not enough surface to justify an enforce mode's false-positive budget.
The root cause is mixed, which exists to keep the reducer's "candidate" single-valued — an implementation convenience, not a domain requirement. Suggest replacing it with a per-key counter map that decays only on that key's own success. mixed then disappears and interleaved loops become detectable.
3. Shadow cannot produce the evidence enforce needs
Two independent gaps:
(a) Broken counterfactual. Shadow advances the virtual warned -> latched transition without ever injecting the reminder, so would_stop assumes "the model kept failing after seeing a correction" — a premise that never occurred. It is an upper bound on enforce's stop rate, not an estimate. The design doc acknowledges this (L399-401), but the rollout plan still treats the seven-day shadow baseline as the gate into warn.
(b) Telemetry cannot separate a loop from exploration. Dropping args from the key is a defensible tradeoff, but no field lets an analyst distinguish:
read_filefailing on the same path 8 times — definitely a loopread_filereturningFILE_NOT_FOUNDon 8 different guessed paths — plausibly legitimate exploration
Both emit an identical would_stop. After seven days you learn how often it would fire, but not whether firing was correct — and only the latter gates enforce. Suggest a bounded distinct_args_bucket ('1' | '2-3' | '4+') computed from a hash, which adds no argument content.
To be fair: shadow can validate the classifier invariants listed at doc L410+ (zero cancelled counted as eligible, zero not_started, etc.). It validates the classifier, not the intervention. The plan should say so explicitly and derive the enforce gate from warn-phase data.
What is already right
- Fail-open discipline is thorough: drain failure / timeout / missing
hasQueuedPromptall land onreliable: false->unreliable_input-> enforcement permanently disabled for the prompt. Verified all three in-repo answerers (bridgeClient.ts:1092, desktopqwen-agent.ts:459/653,channels/base/AcpBridge.ts:562) already returnhasQueuedPromptunconditionally, so shadow's extra request flag changes nothing in-repo. - Cancellation and queued-input precedence, plus full-batch settlement before stopping, are correct — and
Session.test.tscovers the "cancelled while emitting the stop message" race. - The guard's stop path sits inside the
try, so theconversation_finishedfinallyinvariant is preserved. - Keeping the legacy
todoStopGuardWatchQueuedPromptwire name with an explanatory comment is the right compatibility call.
CI: the one failing check is an unrelated flake — glob.test.ts > should allow path outside workspace times out globbing /tmp on the runner (1 failed | 19212 passed). Rerun.
Recommendation
Settle the placement question before polishing the implementation. Several inline findings below (the Todo-reminder regression is a real bug worth fixing regardless), but most of the surrounding machinery would disappear under the core-side design.
中文
结论：信号是对的，落点和判定规则不对
用工具执行结果（而非调用请求）做重复性判定，这个信号今天确实缺失 —— LoopDetectionService 只消费 ToolCallRequest/Content/Thought/Retry/Finished，看不到执行结果。这部分值得做，reducer 本身也写得好：纯函数、可测、状态机清晰。
方案层面三个问题。
1. 应该落在 packages/core，不是 ACP session 层
packages/core/src/services/loopDetectionService.ts 已经是这个概念的归属：prompt 局部生命周期、LoopType 上报、disableForSession，以及既有的双层分级（checkAlwaysOnSafeties 常开 vs addAndCheckHeuristicLoops 受 model.skipLoopDetection 门控）—— 后者基本就是 shadow/warn/enforce 重新实现了一遍的东西。
放在 Session.ts 意味着 TUI(core/client.ts)、headless(nonInteractiveCli.ts)、subagent(agents/runtime/agent-core.ts) 都拿不到。一个佐证：本 PR 不得不给 nonInteractiveCli.ts 加一条 REPEATED_TOOL_EXECUTION_FAILURE 文案，但那条分支在当前架构下永远不可达，纯粹是为了让 Record<LoopType, string> 过 typecheck。
真正的锚点是 CoreToolScheduler.onAllToolCallsComplete（coreToolScheduler.ts:5872）—— core 里已有的批次结算边界，被 useReactToolScheduler、nonInteractiveCli、agent-core、nonInteractiveToolExecutor 共用。CompletedToolCall 已经带了 request.name、tool（可判 native/mcp）、response.errorType、response.executionStatus、status，正好是 RepeatedToolFailureObservation 的全部字段。
提醒：GeminiEventType.ToolCallResponse 看起来是更简单的挂点，但它是死枚举 —— core 里没有任何地方发出它。
改动路径：
- reducer 搬到
core/src/services/repeatedToolFailureReducer.ts（几乎零改动，observation 改从CompletedToolCall派生）。 - 加
LoopDetectionService.recordSettledToolBatch(calls)；在既有reset()里一并清空，prompt 局部生命周期免费获得；命中走既有logLoopDetected+lastLoopType。 - 在
onAllToolCallsComplete触发处调用 —— 一处，覆盖四个消费方。 - 接入
addAndCheckHeuristicLoops（受model.skipLoopDetection门控，默认 true 即默认关闭），不进checkAlwaysOnSafeties。这样QWEN_CODE_ACP_REPEATED_TOOL_FAILURE_GUARD、PROJECT_ENV_HARDCODED_EXCLUSIONS条目和shared-env-keys.ts改动全部可以删掉。 - ACP 刻意绕过
CoreToolScheduler（见Session.ts:7039注释），所以保留一处自己的 feed（在finalizeRunToolResult里）。Session 只留真正 ACP 特有的：停止时历史保全、emitAgentMessage、todoStopGuard.suspend()—— 大约 15 行，而非 285 行。 - guard 落在 core 且由 config 门控后，
_metachannel 标记可以整个删掉（见ChannelAgentBridge.ts的行内 comment）。
2. 判定规则让召回接近于零，且漏掉最主要的循环形态
reducer 要求批次内所有非 duplicate observation 同 key，任何一次 success 都会 reset 整条 streak。所以最常见、最贵的那种循环按设计就检测不到：
batch 1: run_shell("npm test") -> error
batch 2: read_file(x) ; edit(x) -> success, success <-- streak 清零
batch 3: run_shell("npm test") -> error
batch 4: read_file(y) ; edit(y) -> success, success <-- 再次清零
...
剩下的「连续 >= 2 个批次里除了同一个失败工具什么都没有」，很大程度已被 checkToolCallLoop（连续相同 name+args）和每轮工具调用上限覆盖。真正的增量只剩很薄一层：同工具、同 ToolErrorType、不同 args、>= 8 次、>= 2 个纯净批次、零成功。这撑不起 enforce 的误判风险预算。
根因是 mixed —— 它是为了让 reducer 的 candidate 保持单值（实现便利），不是领域需求。建议换成按 key 计数的 Map，只在该 key 自己成功时清零；mixed 随之消失，交错型循环也能捕获。
3. shadow 产不出 enforce 需要的证据
两个独立缺口：
(a) 反事实断裂。 shadow 不注入 reminder，却照样推进虚拟 warned -> latched，所以 would_stop 假设了「模型看到提醒后仍不改」这个从未发生的前提。它是 enforce 真实停止率的上界，不是估计量。design doc L399-401 承认了这点，但 rollout 计划仍把七天 shadow 基线当成进入 warn 的门槛。
(b) 遥测无法区分循环与探索。 key 不含 args 是可接受的取舍，但没有任何字段能区分：
read_file对同一路径失败 8 次 —— 确定是循环read_file对 8 个不同猜测路径FILE_NOT_FOUND—— 可能是合理探索
两者产生完全相同的 would_stop。七天跑完你知道「会停多少次」，但不知道「停得对不对」—— 而后者才是 enforce 的准入条件。建议加一个有界的 distinct_args_bucket（'1' | '2-3' | '4+'，由 hash 计算），不引入任何参数内容。
公平地说：shadow 能验证 doc L410+ 列的分类器不变量（零 cancelled 被计入、零 not_started 等）。它验证的是分类器，不是干预行为。计划里应写明这一点，并把 enforce 的门槛建立在 warn 阶段数据上。
已经做对的部分
- fail-open 很完整：drain 失败/超时/缺
hasQueuedPrompt都落到reliable: false->unreliable_input-> 该 prompt 永久禁用强制停止。仓库内三个 answerer（bridgeClient.ts:1092、desktopqwen-agent.ts:459/653、channels/base/AcpBridge.ts:562）都无条件返回hasQueuedPrompt，所以 shadow 多带的请求标记对内部零影响。 - 取消与排队输入优先级、停止前的完整批次结算都正确，
Session.test.ts也覆盖了「emit stop message 期间收到取消」的竞态。 - guard 的 stop 返回在
try块内，conversation_finished的finally不变量没有被破坏。 - 保留 legacy 线名
todoStopGuardWatchQueuedPrompt并加注释，兼容性处理正确。
CI：唯一失败的 check 是无关 flake —— glob.test.ts > should allow path outside workspace 在 runner 上 glob 整个 /tmp 超时（1 failed | 19212 passed），rerun 即可。
建议
先定落点，再打磨实现。下面还有几条行内 comment（其中 Todo reminder 回归是真 bug，无论方案怎么定都该修），但大部分周边机制在 core 侧方案下会自然消失。
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
Review: the signal is right; the layer, the config axis, and the threshold are not
Re-reviewed at 8cb87ea5. Confirming first what this round fixed, because it changes my earlier assessment:
The recall objection from the previous round no longer applies. A complete batch containing only successes from other tools now returns { kind: 'none' } and preserves the streak (repeated-tool-failure-guard.ts L836-841 of the diff), with preserves a failure streak across successful batches from other tools covering it. So the dominant shape — run_shell fails -> read/edit succeed -> run_shell fails again — is detected now. That was my main design objection and it's resolved. Also fixed since the last round: the SHA-256 digest, the loop_type branch inside the shared core logger, the three unreachable stop branches, the order-dependent reset_reason, and the Todo-reminder drop on abort.
What I verified independently this round:
- The signal is genuinely missing today.
LoopDetectionServiceconsumes onlyToolCallRequest/Content/Thought/Retry/Finished— it sees requests, never outcomes.checkGlobalDuplicatekeys on name+args (threshold 6) and is gated off by default (config.ts:2318,skipLoopDetection ?? true); the invalid-params guard only coversnot_started. None of them can see "same tool, same classified execution error, fresh args." Worth having. - The three automatic-continuation routes really do pin
'off'— I checked eachcreateDaemonToolLoopState()call site. - Fail-open is thorough: drain timeout /
-32601/ missinghasQueuedPromptall land onreliable: false->unreliable_input-> enforcement disabled for the prompt. - The stop path returns inside the
try, so theconversation_finishedfinallyinvariant holds.
Four things I'd still settle, in priority order. Details inline.
1. Wrong layer (blocking for the design, not the code)
The guard lives only in ACP Session, so TUI, headless and subagents get nothing. The clearest tell is nonInteractiveCli.ts:175: a LoopType label headless can name but can never emit, present only to satisfy Record<LoopType, string>.
In fairness, moving it to core is not free: ACP deliberately bypasses CoreToolScheduler (see the comment at Session.ts:7196), so a core-side design still needs two feed points — onAllToolCallsComplete plus ACP's own finalizeRunToolResult. Same cost, three more consumers covered, and Session.ts keeps only what is genuinely ACP-specific (history preservation, emitAgentMessage, todoStopGuard.suspend()).
2. A new config axis where one already exists
model.skipLoopDetection + system-scope settings.json (/etc/qwen-code/settings.json, root-owned — storage-paths-lite.ts:48) is already the operator boundary for exactly this class of guard. The new env var has to be manually blacklisted in PROJECT_ENV_HARDCODED_EXCLUSIONS to stop a project .env self-promoting into enforce; a system-scope setting gets that trust boundary for free and deletes the shared-env-keys.ts change plus the docs table row.
3. The threshold undercuts the stated motivation
failureCount accumulates per-batch matching failures. In the common one-call-per-batch loop that is 8 batches to warn, 9 to stop — roughly 9-10 model round-trips. The PR's own motivation is that the existing high cap "wastes model rounds, tool latency, and tokens before doing so." Compare: global-duplicate 6, invalid-params 3.
4. Shadow can't answer the question that gates enforce
Two independent gaps, one acknowledged in the doc and one not:
(a) Shadow advances the virtual warned -> latched without ever injecting the reminder, so would_stop presumes a premise that never occurred. It is an upper bound on enforce's stop rate, not an estimate. Doc L399-401 says this, but the rollout still treats the 7-day shadow baseline as the gate into warn.
(b) Not currently acknowledged: because args are excluded from the key, read_file failing on the same path 8 times (a real loop) and read_file returning FILE_NOT_FOUND on 8 different guessed paths (plausible exploration) emit identical telemetry. After seven days you know how often it would fire, not whether firing was correct — and only the latter gates enforce.
Shadow can validate the classifier invariants at doc L410+ (zero cancelled counted, zero not_started, etc.). That's the classifier, not the intervention. I'd state that explicitly and derive the enforce gate from warn-phase data.
Suggested shape
Keep the reducer as-is — pure, well-tested, correctly built on the frozen executionStatus / executionErrorType contract rather than error-string matching. Change the surroundings: move it to core/src/services/, gate on skipLoopDetection, drop the env var and the _meta marker, and add a bounded distinct_args_bucket ('1' | '2-3' | '4+', from a hash — no argument content) so shadow can separate a loop from exploration.
中文
结论：信号对，但落点、配置轴和阈值都还需要再定
基于 8cb87ea5 复审。先确认这一轮修好的部分，因为它推翻了我上一轮的主要判断：
上一轮「召回接近于零」的意见已经不成立。 现在一个只包含其他工具成功的完整批次会返回 { kind: 'none' } 并保留 streak，对应测试 preserves a failure streak across successful batches from other tools。所以最主要的循环形态（run_shell 失败 -> read/edit 成功 -> run_shell 再失败）现在能检测到了。这是我上轮最主要的设计意见，已解决。同时修好的还有：SHA-256 摘要、core 共享 logger 里按 loop_type 分叉、三处不可达分支、reset_reason 顺序依赖、abort 分支吞掉 Todo reminder。
本轮我独立核实过：这个信号今天确实缺失（LoopDetectionService 只消费请求不消费结果；checkGlobalDuplicate 按 name+args 判重且默认关闭；invalid-params 只覆盖 not_started）；三个自动续跑路由确实都传 'off'；fail-open 完整；stop 返回在 try 内，conversation_finished 不变量没破。
还需要定的四件事：
1. 落点错层。 只在 ACP Session 生效，TUI/headless/subagent 都拿不到。最清楚的信号是 nonInteractiveCli.ts:175——一个 headless 能命名却永远发不出的 LoopType。公平地说，搬到 core 并非零成本：ACP 刻意绕过 CoreToolScheduler（Session.ts:7196 有注释），core 侧方案仍需两个 feed 点。代价相同，但多覆盖三类消费方。
2. 配置轴重复。 model.skipLoopDetection + system-scope settings.json（/etc/qwen-code/settings.json，root 拥有）已经是这类守卫的运维边界。新环境变量需要手工维护 PROJECT_ENV_HARDCODED_EXCLUSIONS 黑名单才能挡住项目 .env 自我提权；system-scope setting 免费获得同样的信任边界。
3. 阈值与动机相悖。 每批一次调用的常见循环需要 8 个批次才 warn、9 个才停，约 9-10 个模型往返。而 PR 的动机恰恰是「高上限兜底前浪费了轮次」。对比：全局重复 6，invalid-params 3。
4. shadow 产不出 enforce 需要的证据。 (a) shadow 从不注入提醒却推进虚拟 warned -> latched，would_stop 是上界不是估计量（doc L399 已承认，但 rollout 仍拿它当进 warn 的门槛）。(b) 尚未被承认的一点：键不含 args，导致「同一路径失败 8 次」（确定循环）与「8 个不同猜测路径 FILE_NOT_FOUND」（合理探索）产生完全相同的遥测。跑完七天你知道会停多少次，但不知道停得对不对——而后者才是 enforce 的准入条件。
建议形态：reducer 本身保留（纯函数、可测、建立在冻结契约而非错误文本匹配上）；改周边——搬进 core/src/services/、由 skipLoopDetection 门控、删掉环境变量与 _meta 标记、补一个有界的 distinct_args_bucket（'1' | '2-3' | '4+'，由 hash 计算，不含参数内容）。
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
|
Review disposition summary after merging current main in
Validation on the merged tree:
I am resolving the replied threads according to these dispositions. |
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
|
Thanks for the PR — the third in the series after #8176 and #8180, both merged. A few gate notes before code review:
Moving on to code review. 🔍 中文说明感谢贡献——这是 #8176 和 #8180（均已合并）之后的系列第三篇。进入代码审查前的几点门槛意见：
进入代码审查。🔍 — Qwen Code · qwen3.8-max Reviewed at |
Pull request was converted to draft
|
Thanks for the gate review. I am not cutting the drain reliability probe or rollout telemetry from this PR: the probe is the fail-open safety boundary for warn/enforce on incompatible ACP hosts, while the default-shadow telemetry is the evidence needed before either mode can be enabled. Removing both would preserve the reducer but break the staged-rollout safety loop. The branch has also passed roughly five review rounds, so I am deferring non-Critical scope churn. The PR is now Ready for maintainer review, but is not merge-ready: the local E2E plan and deterministic manual ACP fixture are pending pre-merge gates. Separate seven-day internal/public-cloud shadow baselines are post-merge rollout gates before staged warn/enforce promotion. |
|
Final review disposition for the current head
The only additional branch change warranted by the latest review is the readiness wording correction in |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline. Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally. Not explored to full depth (tool budget reached): chunk 3: could not run prettier --check live (prettier not installed in the worktree, no network install attempted); instead verified prettier's table invariants progr…; chunk 3: live prettier --check run not possible (no node_modules in the worktree, no global prettier); substituted the programmatic table-invariant verification above.; chunk 9: couldn't run the unit tests (dependencies aren't installed in the review worktree) — the static review only.; chunk 9: run repeated-tool-failure-guard.test.ts / typecheck — node_modules is not installed in this review worktree ( vitest unresolvable), so verification was sta…. Not reviewed: reverse audit — stopped before round 3 by the review time budget.
中文说明
已审查。 建议见行内评论。 未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。 未探索到全部深度（达到工具调用预算）：chunk 3：could not run prettier --check live (prettier not installed in the worktree, no network install attempted); instead verified prettier's table invariants progr…；chunk 3：live prettier --check run not possible (no node_modules in the worktree, no global prettier); substituted the programmatic table-invariant verification above.；chunk 9：couldn't run the unit tests (dependencies aren't installed in the review worktree) — the static review only.；chunk 9：run repeated-tool-failure-guard.test.ts / typecheck — node_modules is not installed in this review worktree ( vitest unresolvable), so verification was sta…。 未审查：反向审计——评审时间预算不足，未能开始第 3 轮。
— qwen3.8-max via Qwen Code /review (v0.21.8)
|
Qwen Code review timed out. Qwen review timed out after 21600 seconds (of the 360-minute budget). This run already used the maximum 360 minute timeout. See workflow logs. |
yiliang114
left a comment
yiliang114
left a comment
There was a problem hiding this comment.
LGTM from a correctness and security standpoint. Verified at head: eligibility is positive-only (terminal error + execution error + resolved identity + non-UNKNOWN frozen type), the 8-across-2-batches boundary is right (8 in one batch tracks but does not warn), candidate semantics match the spec including same-batch success-before-key-selection, and there is no cross-prompt or cross-session leakage — fresh loop state at all four creation sites, stop-hook/cron/notification/background routes forced off, aborted turns return before the reducer. Enforce stops only from warned on a later settled matching batch with every current-batch response preserved, todoStopGuard suspended until the next ordinary prompt, and cancellation winning in both the final-drain and stop-emit windows. The channel marker is strict === true and can only force off (both bridge implementations stamp it), the mode key sits in PROJECT_ENV_HARDCODED_EXCLUSIONS behind the case-folded predicate at all three project-env gates with home-scoped/process env still honored, the unreliable_input path fails open per prompt without leaking content, and telemetry carries only ids/enums/buckets. wenshao's ten review items are addressed or documented at head.
Two non-blocking recommendations before any enforce promotion — currently mutation-survivable and deferred under the Critical-only cutoff: pin the Session-level complete:false receipt path (dropping the ordinal-uniqueness conjunct keeps the suite green today) and the todoStopGuard.suspend() effect on the stop path. Also noted: the author-declared manual E2E gate is still pending pre-merge. CI green on this head. Nothing blocks from my side.
|
@qwen-code /triage |
|
Sandboxed verification: ✅ passed — merge-ready (agent verdict) - workflow run Ran the PR in an isolated, token-free container: A/B against the base build, mock-free harness assertions, targeted gates. Advisory evidence for human reviewers — not a review, an approval, or a CI check. Scripted assertions: 957 passed · 0 failed · 957 total 中文 — 判定：✅ 通过 · 可合入（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：957 通过 · 0 失败 · 957 总计 Verification reportPR 8469 — feat(acp): Protect against repeated tool execution failuresVerdict: 中文摘要
Central claim and A/B proofCentral claim: on the interactive ACP foreground route, Scenario (identical in both cells): a real
Witnesses: Secondary claims verified:
Mutation matrix (vacuity)Suite under mutation: 38 unit tests (
No survivors: every guard the PR introduces is pinned by a test that fails for the intended behavioral reason. The m0b positive control proves the suite can go red; m0 (which killed a different, smaller set) shows the two mutation points are independently load-bearing. Independent reducer harness ( Findings (non-blocking)
Not covered
MethodologyEnvironment: CI Evidence images
Harness scripts and raw logs are in the workflow run artifacts (7-day retention). — Qwen Code · sandboxed verification |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline. Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally. Not explored to full depth (tool budget reached): chunk 7: typecheck/unit-test run skipped — the review worktree has no node_modules and installing the full monorepo dependency tree was disproportionate to this review…; chunk 8: could not execute the test file — the review worktree has no node_modules and a monorepo install exceeded the tool budget; all assertions were verified by han…. Not reviewed: reverse audit — stopped before round 4 by the review time budget.
中文说明
已审查。 建议见行内评论。 未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。 未探索到全部深度（达到工具调用预算）：chunk 7：typecheck/unit-test run skipped — the review worktree has no node_modules and installing the full monorepo dependency tree was disproportionate to this review…；chunk 8：could not execute the test file — the review worktree has no node_modules and a monorepo install exceeded the tool budget; all assertions were verified by han…。 未审查：反向审计——评审时间预算不足，未能开始第 4 轮。
— qwen3.8-max via Qwen Code /review (v0.21.8)
|
Qwen Code review timed out. Qwen review timed out after 21600 seconds (of the 360-minute budget). This run already used the maximum 360 minute timeout. See workflow logs. |
Maintainer verification: real ACP stack, local buildI built this PR locally and drove it through a real ACP agent process ( Result: every behavioural claim in the PR reproduced. No defect found. I consider the ACP-side pre-merge gate satisfied — with the residual gaps listed at the bottom.
1. Declared checks, re-run
I widened the CLI leg from the four named files to the whole 2. Real ACP end-to-end harnessNot mocks — an actual process tree: The model is scripted as a pure function of conversation state (immune to the CLI's internal title/classifier/suggestion calls): while the guarded prompt is live it emits 4 Evidence per run is taken from three independent places: the model server's request log (what text actually reached the model), the ACP wire log (what the agent told the host), and the agent's own debug log written by the running process. 3. Scenario matrix — 16 runs
Reading of the load-bearing rows:
4. Turn-level detail, history preservation, and telemetry
5. One wire-level note for reviewersShadow is the default, and shadow asks for queued-prompt state. Confirmed on the wire: in
Not a blocker — worth stating explicitly in the rollout notes. 6. What this run does not cover
Recommendation: approve. The implementation behaves as described on a real ACP stack under 16 adversarial configurations, the default mode is provably inert, and every escape hatch (channel, legacy host, project env, queued input, cancellation, same-tool success) fails safe. 中文版本维护者验证：真实 ACP 栈 + 本地构建我在本地构建了这个 PR，并用真实的 ACP agent 进程（ 结论：PR 声明的每一条行为都在真实环境复现，没有发现缺陷。我认为 ACP 侧的合并前置门禁已经满足，遗留缺口列在最后。
1. 重跑 PR 声明的检查CLI 这条腿我从 4 个指定文件扩到了整个 2. 真实 ACP 端到端脚手架不是 mock，是真实进程树：harness ACP host 走 stdio JSON-RPC，负责 模型脚本是「对话状态的纯函数」（因此不受 CLI 内部的标题生成 / 分类器 / 建议模式调用干扰）：在被保护的 prompt 存活期间，每轮发出 4 个指向不存在路径的 每个场景的证据取自三个互相独立的来源：模型服务的请求日志（到底哪些文本进了模型）、ACP 线协议日志（agent 到底对 host 说了什么）、以及运行中 agent 自己写出的调试日志。 3. 场景矩阵 —— 16 次运行
关键行的解读：
4. 轮次细节、历史保留与遥测
5. 给评审的一条线协议提示Shadow 是默认模式，而 shadow 会请求排队 prompt 状态。线上实测：
不是阻塞项，但建议在发布说明里写清楚。 6. 本次未覆盖的部分
结论建议：可以合并。 在 16 组对抗性配置下，实现的行为与描述完全一致；默认模式可证明是惰性的；每一条逃生通道（channel、旧 host、项目 env、排队输入、取消、同工具成功）都是 fail-safe 的。 |
|
Released in v0.21.9. |
4 participants
What this PR does
This PR adds a conservative, prompt-local guard for repeated typed tool execution failures on the selected live interactive ACP foreground Session. It consumes the frozen execution outcome contract established by #8176 and #8180, counts only terminal execution failures with a resolved tool identity and structured execution error type, and requires eight matching failures across at least two complete model tool batches. The first release keeps one active failure candidate: successes from other resolved tools neither advance nor reset it, while a success from the candidate tool clears that tool before the remaining unique failure key is selected.
The guard supports operator-controlled
off,shadow,warn, andenforcemodes, withshadowas the default. Warn mode injects one fixed corrective reminder per candidate streak; enforce mode stops only after a later complete matching batch has settled and the candidate tool has not succeeded in between, preserves every current-batch function response in history, suspends automatic Todo continuation, and requires new user input before continuing. Channel-driven prompts are explicitly marked by both channel bridges and forced tooff; this client-supplied marker is a routing hint, not a trust or authorization boundary. Stop-hook/Todo automatic continuations, cron, notifications, and background routes also remain off.The default shadow mode asks the existing mid-turn drain extension for reliable queued-prompt state. Older or third-party ACP hosts that cannot provide that contract emit an
unreliable_inputdiagnostic, disable enforcement for the prompt, and remain fail-open. The rollout mode is documented as operator policy and cannot be injected by project.env, project.qwen/.env, or workspacesettings.env; invalid non-empty values warn and fall back to shadow.The change adds low-cardinality transition metrics and data-minimized OpenTelemetry diagnostics for staged rollout plus a distinct terminal loop reason. Diagnostic events reuse the raw ACP prompt ID already emitted by tool-call telemetry so authorized rollout analysis can join transitions to settled batches without a second identifier space, while metrics omit prompt identity, candidate ordinal, and execution error type. Tool arguments, outputs, paths, raw error messages, MCP server names, user IDs, and the private failure key remain excluded from guard-specific fields; the terminal loop event explicitly bypasses QwenLogger/RUM while retaining standard OpenTelemetry correlation. Deployment cohort and service version come from the OpenTelemetry Resource; each rollout environment must configure a stable dimension such as
deployment.environment, while the SDK always suppliesservice.version.Why it's needed
Repeated execution failures can keep an ACP foreground turn consuming tool calls and model rounds after the model has stopped making useful progress. The existing provider call-ID deduper, invalid-parameter guard, and total tool-call cap address different failure classes and either do not recognize semantic execution repetition or react too broadly.
Using #8180's final execution status and frozen execution error classification lets this guard exclude user cancellation, permission rejection, pre-execution validation, unknown outcomes, skipped siblings, and post-execution processing failures. The high threshold, two-batch requirement, reminder-before-stop sequence, prompt-local lifecycle, input and cancellation precedence, and shadow-first rollout keep false-positive risk bounded while providing evidence for independent internal and public-cloud rollout decisions.
Reviewer Test Plan
How to verify
Run
cd packages/cli && npx vitest run src/acp-integration/session/repeated-tool-failure-guard.test.ts src/acp-integration/session/Session.test.ts src/config/environment.test.ts src/config/shared-env-keys.test.ts. Confirm that shadow changes neither model continuation nor messages; failures remain detectable across successful calls from other tools, while same-tool success clears the candidate; warn injects one reminder for the candidate; enforce settles and preserves the post-reminder matching batch before stopping; cancellation and queued input win; unsupported hosts never enforce; channel-marked prompts remain off; the legacy drain payload remains unchanged in off mode; and project-controlled environment sources cannot select a rollout mode.Run
cd packages/channels/base && npx vitest run src/AcpBridge.test.ts src/DaemonChannelBridge.test.ts. Confirm that both channel prompt paths attach the private route marker without changing text, image, cancellation, or response collection behavior.Run
cd packages/core && npx vitest run src/telemetry/loggers.test.ts src/telemetry/metrics.test.ts. Confirm that metrics contain only bounded transition attributes, structured diagnostics retain standard prompt correlation while excluding sensitive tool fields, metric and log sink failures do not change control flow, and the terminal repeated-execution loop event is explicitly excluded from QwenLogger/RUM.Run
npm run build,npm run typecheck, andnpm run lint. The expected result is a clean build, typecheck, and lint.For manual ACP validation, use a deterministic typed failing tool and record the local plan and results under
.qwen/e2e-tests/. Verify concurrent batch settlement, permission cancellation, reconnect/restart and history replay, a fresh prompt after stop, unsupported-host behavior, channel exclusion, and shadow-mode non-interference. The local plan and manual fixture run are still pending and must be completed before merge. Ready for review means the implementation is ready for maintainer review; it does not mean merge-ready or claim completion of these pre-merge checks. After merge and deployment in shadow, collect separate seven-day internal and public-cloud baselines before beginning the staged warn/enforce rollout.Evidence (Before & After)
Before: a model can continue issuing fresh tool-call IDs for the same typed execution failure until another guard, the configured total cap, or user intervention ends the turn. Channel and older-host boundaries were not represented in the original guard rollout plan, and the guard metric included the execution error type as a label.
After: automated ACP coverage shows shadow mode observes without changing the turn, warn mode injects one fixed reminder for the candidate and continues, enforce mode settles and preserves the ninth matching failure, emits one fixed stop message, records
repeated_tool_execution_failure, and opens no fourth model stream. Enforce-configured channel prompts and unsupported hosts continue without a guard stop. The final local run passed 587 CLI tests, 91 channel tests, and 129 Core telemetry tests; full build, typecheck, lint, formatting, and diff checks also passed.Tested on
Environment (optional)
macOS local workspace, Node.js 26.0.0, npm workspace build, sandbox disabled for local test processes.
Risk & Scope
QWEN_CODE_ACP_REPEATED_TOOL_FAILURE_GUARDtooff,shadow,warn, orenforce; missing or invalid values resolve toshadow, and project-controlled environment sources cannot set it.Linked Issues
Related to #8176 and #8180.
中文说明
这个 PR 做了什么
这个 PR 为选中的实时交互式 ACP 前台 Session 增加了一个保守的、prompt 内局部生效的重复工具执行失败保护。它消费 #8176 和 #8180 建立的冻结执行结果契约，只统计具有已解析工具身份和结构化执行错误类型的终态执行失败，并要求至少跨两个完整模型工具批次累计八次相同失败。首版只保留一个活跃失败候选：其他已解析工具的成功既不推进也不清零该候选；候选工具自身成功时，会先清除该工具的失败，再选择剩余唯一失败键。
保护支持由运维控制的
off、shadow、warn和enforce模式，默认是shadow。Warn 模式对每个候选连续失败只注入一次固定纠偏提醒；Enforce 模式仅在后续完整匹配批次全部结束且候选工具期间没有成功后停止，保留当前批次的全部函数响应到历史，暂停自动 Todo 续跑，并要求新的用户输入后才能继续。两个 channel bridge 都会显式标记 channel 驱动的 prompt，Session 会把它们强制设为off；这个客户端提供的标记只是路由 hint，不是信任或授权边界。Stop-hook/Todo 自动续跑、cron、通知和后台路由也保持关闭。默认 Shadow 模式会通过现有的 mid-turn drain 扩展请求可靠的排队 prompt 状态。无法提供这个契约的旧版或第三方 ACP host 会记录一次
unreliable_input诊断、对当前 prompt 禁用强制停止，并保持 fail-open。发布模式已作为运维策略写入文档，项目.env、项目.qwen/.env和工作区settings.env都不能注入该值；非空无效值会产生告警并回退到 Shadow。本次变更增加了用于分阶段发布的低基数状态转换指标和数据最小化 OpenTelemetry 诊断，以及独立的终态循环原因。诊断事件复用工具调用遥测已经上报的原始 ACP prompt ID，使授权发布分析可以直接把状态转换关联到已结算批次；指标不包含 prompt 身份、候选序号或执行错误类型。保护专属字段仍不包含工具参数、输出、路径、原始错误消息、MCP 服务名、用户 ID 或私有失败键；终态循环事件会显式绕过 QwenLogger/RUM，同时保留标准 OpenTelemetry 关联字段。部署分组和服务版本来自 OpenTelemetry Resource；每个发布环境必须配置稳定的
deployment.environment等维度，SDK 则始终提供service.version。为什么需要
当模型已经不再取得有效进展时，重复执行失败仍可能让 ACP 前台轮次继续消耗工具调用和模型轮次。现有的 provider call-ID 去重、无效参数保护和总工具调用上限分别解决不同的失败类型，要么无法识别语义层的重复执行，要么粒度过宽。
使用 #8180 的最终执行状态和冻结执行错误分类后，本保护可以排除用户取消、权限拒绝、执行前校验、未知结果、被跳过的 sibling，以及执行后处理失败。较高阈值、双批次要求、先提醒后停止、prompt 局部生命周期、输入与取消优先级和 Shadow 优先发布共同限制误判风险，并为集团内和公有云分别作出发布决策提供证据。
Reviewer 测试计划
如何验证
运行
cd packages/cli && npx vitest run src/acp-integration/session/repeated-tool-failure-guard.test.ts src/acp-integration/session/Session.test.ts src/config/environment.test.ts src/config/shared-env-keys.test.ts。确认 Shadow 不改变模型续跑或消息；其他工具成功时失败仍可累计，而同工具成功会清除候选；Warn 对当前候选注入一次提醒；Enforce 等待提醒后的匹配批次全部结束并保留结果后才停止；取消和排队输入优先；不支持扩展的 host 永不强制停止；标记为 channel 的 prompt 保持关闭；Off 模式保留旧 drain 请求结构；项目控制的环境来源不能选择发布模式。运行
cd packages/channels/base && npx vitest run src/AcpBridge.test.ts src/DaemonChannelBridge.test.ts。确认两个 channel prompt 路径都会附带私有路由标记，且文本、图片、取消和响应收集行为不变。运行
cd packages/core && npx vitest run src/telemetry/loggers.test.ts src/telemetry/metrics.test.ts。确认指标只包含有界的状态转换属性，结构化诊断保留标准 prompt 关联但不包含敏感工具字段，指标及日志 sink 失败不会改变控制流，并且重复执行失败的终态循环事件会显式绕过 QwenLogger/RUM。运行
npm run build、npm run typecheck和npm run lint。预期结果是构建、类型检查和 lint 全部干净通过。手工 ACP 验证请使用确定性的结构化失败工具，并将本地计划和结果记录在
.qwen/e2e-tests/下。验证并发批次结算、权限取消、重连/重启和历史回放、停止后的新 prompt、不支持 host 的行为、channel 排除，以及 Shadow 模式不改变控制流。本地计划和手工 fixture 运行仍待完成，并且是合并前置条件。Ready for review 只表示实现可供 maintainer 评审，不代表已经 merge-ready，也不代表这些合并前检查已经完成。合并并以 Shadow 模式部署后，需分别收集集团内和公有云七天基线，之后才能开始分阶段 Warn/Enforce 发布。证据（变更前后）
变更前：模型可以持续使用新的 tool-call ID 发起同一种结构化执行失败，直到其他保护、配置的总上限或用户干预结束轮次。原始保护发布方案没有体现 channel 和旧 host 边界，保护指标也把执行错误类型作为标签。
变更后：ACP 自动化覆盖证明 Shadow 模式只观察而不改变轮次；Warn 模式会对当前候选注入一次固定提醒并继续；Enforce 模式会等待并保留第九次相同失败，发出一次固定停止消息，记录
repeated_tool_execution_failure，且不会开启第四个模型流。配置为 Enforce 的 channel prompt 和不支持扩展的 host 都会继续运行而不会被保护强制停止。最终本地运行通过了 587 个 CLI 测试、91 个 channel 测试和 129 个 Core 遥测测试；完整 build、typecheck、lint、格式和 diff 检查也全部通过。测试平台
环境（可选）
macOS 本地工作区，Node.js 26.0.0，npm workspace 构建，本地测试进程未启用 sandbox。
风险与范围
QWEN_CODE_ACP_REPEATED_TOOL_FAILURE_GUARD设置为off、shadow、warn或enforce；缺失或无效值会解析为shadow，项目控制的环境来源不能设置它。关联问题
关联 #8176 和 #8180。