Stop repeated invalid tool parameter loops in ACP

[yiliang114]

What this PR does

The ACP daemon now tracks tool-call progress across each daemon turn and stops the turn when the model repeatedly sends invalid parameters for the same tool instead of correcting them. It also adds a hard per-turn tool-call cap for ACP daemon execution and records loop-detection telemetry when either guard fires.

Why it's needed

A model can repeatedly request a tool such as ask_user_question with fresh call IDs but the same invalid argument shape. Because the IDs are fresh, duplicate call protection does not apply, and ACP daemon mode can keep sending validation errors back to the model until the session repeats the same failed request thousands of times. The daemon needs the same kind of always-on safety boundary that the CLI already has for tool-call loops.

Reviewer Test Plan

How to verify

Confirm that an ACP prompt stops after the third repeated invalid parameter validation error when the provider uses fresh tool-call IDs. Also confirm that existing duplicate provider call ID loop behavior, non-interactive loop messaging, and the core per-turn tool-call cap behavior still work.

Evidence (Before & After)

Before this change, the focused ACP regression test reproduced the bug by continuing to a fourth model send after three repeated invalid ask_user_question parameter errors. After this change, the same test stops at three sends and logs the loop guard. Additional focused regression tests passed for duplicate provider call IDs, non-interactive loop messaging, and the core turn tool-call cap.

Local verification passed: focused ACP session tests, focused non-interactive CLI loop detection test, focused core loop detection test, npm run build && npm run typecheck, npm run lint, and git diff --check.

Tested on

OS	Status
🍏 macOS	✅ tested
🪟 Windows	⚠️ not tested
🐧 Linux	⚠️ not tested

Environment (optional)

Local macOS worktree using the repository npm scripts.

Risk & Scope

• Main risk or tradeoff: ACP daemon turns now stop after three identical tool-parameter validation failures, so a model gets fewer chances to self-correct that exact invalid argument shape in one turn.
• Not validated / out of scope: Full end-to-end ACP client testing on Windows and Linux.
• Breaking changes / migration notes: None expected.

Linked Issues

Fixes #6075

[qwen-code-ci-bot]

Thanks for the PR!

Template looks good ✓ — all required headings are present and filled in, including the linked issue (#6075).

On direction: this is squarely aligned with the project. #6075 documents a real ACP-daemon correctness bug — a model can spin the daemon turn indefinitely by resending the same invalid tool parameters with fresh call IDs, bypassing the existing duplicate-call-id guard. The CLI already has always-on loop guards for similar scenarios, so parity in the daemon path is the right thing to ship. CHANGELOG has no direct prior reference, but the loop-detection area (LoopType, LoopDetectedEvent) is well-established infrastructure that this PR extends in a natural way.

On approach: the scope is tight and minimal. Five files, ~230 lines, all pointed at the stated goal:

• A per-turn DaemonToolLoopState scoped to the ACP turn entry points.
• Two guards: a hard 100-call cap per turn, and a 3-strikes threshold for repeated (toolName, errorMessage) validation failures.
• Reuse of existing LoopType / logLoopDetected / LoopDetectedEvent telemetry — no parallel infrastructure.
• A new LoopType.INVALID_TOOL_PARAMS_STAGNATION value wired into the existing non-interactive loop message formatter.

No drive-by refactors, no scope creep. I don't see a materially simpler path — the existing duplicate-call-id and turn-cap guards already cover adjacent cases, and this PR slots the missing piece in cleanly.

Moving on to code review. 🔍

中文说明

感谢 PR！

模板完整 ✓ — 所有必需章节都已填写，包括关联的 issue（#6075）。

方向：与项目完全对齐。#6075 记录了一个真实的 ACP daemon 正确性 bug —— 模型可以用新的 tool-call id 不断重发相同非法参数，绕过现有的 duplicate-call-id 防护，让 daemon turn 无限空转。CLI 在类似场景下已经有 always-on 的 loop guard，所以在 daemon 路径补齐对等能力是正确的方向。CHANGELOG 中没有直接相关的历史条目，但 loop-detection 基础设施（LoopType / LoopDetectedEvent / logLoopDetected）已经相当成熟，本 PR 是对它的自然扩展。

方案：范围紧凑且最小化。5 个文件、约 230 行，全部围绕目标：

• 作用域在 ACP turn 入口的 per-turn DaemonToolLoopState。
• 两个守卫：每 turn 100 次 tool call 硬上限，以及对重复 (toolName, errorMessage) 验证失败的 3 次阈值。
• 复用已有的 LoopType / logLoopDetected / LoopDetectedEvent telemetry，没有引入平行基础设施。
• 新增 LoopType.INVALID_TOOL_PARAMS_STAGNATION，并接入现有的 non-interactive loop message formatter。

没有顺手的重构，也没有范围蔓延。我没有看到明显更简单的路径 —— 现有的 duplicate-call-id 和 turn-cap 守卫已经覆盖了相邻场景，本 PR 干净地补上了缺失的一块。

进入代码审查。🔍

— Qwen Code · qwen3.7-max

[qwen-code-ci-bot]

Code Review

Independent baseline (before reading the diff): the bug is "daemon resends the same invalid tool params with fresh IDs, bypassing duplicate-call-id guard." The minimal fix I would have proposed is:

1. Per-turn state tracking (toolName, errorMessage) → count inside the ACP turn loop.
2. Stop the turn when the same key hits a small threshold (3).
3. Reuse the existing LoopType / LoopDetectedEvent telemetry pipeline.
4. Add a hard per-turn tool-call cap as a belt-and-braces backstop.

The PR matches this baseline almost exactly — same state shape, same threshold, same reuse, same cap. The one nuance I hadn't anticipated is that the invalid-params counter should only fire when tool.build(args) actually throws (not when the tool body throws after build succeeds). The PR gets this right via a toolBuildSucceeded flag in the catch block.

Findings:

• Reuse: clean. The PR extends the existing LoopType enum, reuses logLoopDetected / LoopDetectedEvent, and adds the new enum value to the nonInteractiveCli loop-message formatter so user-facing text stays consistent. No parallel infrastructure.
• Scope: tight. All edits serve the stated goal. No drive-by refactors, no speculative abstractions. The new DaemonToolLoopState and three helper functions are local to Session.ts where they're used, which is appropriate since the daemon turn loop is an ACP-specific concern (the core loopDetectionService handles different loop shapes — chanting, action-stagnation, alternating patterns — and is not the right home for this).
• Correctness: cap check is applied at batch entry (runTools), invalid-params check is applied per-call (runTool), both return loopDetected: true which propagates up and terminates the turn. createDaemonToolLoopState() is called fresh at each of the four ACP turn entry points, so state is per-turn as documented. The !activeToolAbortSignal.aborted guard on the invalid-params record is correct — we don't want to count abort-raced failures.
• One nit (non-blocking): recordDaemonToolCalls returns loopState?.loopDetected ?? false in the early-exit path. It's correct and I read it twice to be sure. A named local would be slightly easier on future readers, but not worth a change request.

No critical blockers. No AGENTS.md violations. Implementation reads like a maintainer wrote it.

Real-Scenario Testing

Honest caveat up front: this bug lives in the ACP daemon path — triggered only when a model sends repeated invalid tool parameters with fresh call IDs. It is not reachable from a normal interactive/non-interactive qwen prompt, and there's no deterministic way to induce it from the CLI without a mock ACP client + scripted model. So a full before/after tmux reproduction of the bug itself isn't feasible. What I can show:

• A focused unit regression test that reproduces the exact bug shape (three ask_user_question calls with questions as a JSON string, fresh ids ask_1/ask_2/ask_3) and asserts the turn stops at three.
• Build + typecheck + lint green on the PR.
• A tmux smoke test of the bundled PR vs the current main showing the bundle still starts, plus a grep confirming the new INVALID_TOOL_PARAMS_STAGNATION label lands in the PR bundle and is absent from main.

Focused regression test

$ cd packages/cli && npx vitest run src/acp-integration/session/Session.test.ts \
    -t "stops an ACP prompt after repeated invalid tool parameters"

 ✓ src/acp-integration/session/Session.test.ts (206 tests | 205 skipped) 13ms

 Test Files  1 passed (1)
      Tests  1 passed | 205 skipped (206)

The test mocks ask_user_question.build to throw Parameter "questions" must be an array., feeds three consecutive streams with fresh ids, and asserts build is called exactly three times, sendMessageStream is called exactly three times, and debugLogger.warn fires with "Stopping ACP turn after repeated tool parameter errors". Without the fix the fourth stream would also be consumed; with the fix the turn terminates at three.

Full related-suite runs

Session.test.ts              206 passed
nonInteractiveCli.test.ts     69 passed | 1 skipped
loopDetectionService.test.ts  72 passed

Build / typecheck / lint

npm run build      ✅  (all packages)
npm run typecheck  ✅  (cli, core, sdk, webui)
npm run lint       ✅  (eslint . --ext .ts,.tsx)
npm run bundle     ✅  (dist/cli.js + chunks)

Tmux before/after smoke (bundle starts, version surfaces)

=== BEFORE (main) ===
$ node dist/cli.js --version
0.19.3

=== AFTER (PR) ===
$ node dist/cli.js --version
0.19.3

Both bundles start and exit cleanly. No CLI surface regression.

Label presence in built artifact

INVALID_TOOL_PARAMS_STAGNATION in main/dist: (none — expected).
In PR/dist:

dist/chunks/chunk-HQRP3JM6.js: LoopType2["INVALID_TOOL_PARAMS_STAGNATION"] = "invalid_tool_params_stagnation";
dist/chunks/acpAgent-62QCZDVD.js: "invalid_tool_params_stagnation" /* INVALID_TOOL_PARAMS_STAGNATION */,
dist/chunks/chunk-LJ3IJEQ2.js: ["invalid_tool_params_stagnation" /* INVALID_TOOL_PARAMS_STAGNATION */]:
  "the model repeatedly sent invalid tool parameters without correcting them"

The new enum value is compiled into core, consumed by the ACP agent chunk, and surfaced in the non-interactive CLI formatter with the correct user-facing label — exactly the three integration points the diff modifies.

中文说明

代码审查

独立基线（读 diff 之前）：这个 bug 是「daemon 用新 id 不断重发相同非法参数，绕过 duplicate-call-id 守卫」。我会提出的最小修复是：

1. 在 ACP turn 循环内维护 (toolName, errorMessage) → count 的 per-turn 状态。
2. 同一 key 达到一个小阈值（3）就终止 turn。
3. 复用现有 LoopType / LoopDetectedEvent telemetry 管线。
4. 加一个 per-turn tool call 硬上限作为兜底。

PR 几乎就是这个方案 —— 同样的状态形态、同样的阈值、同样的复用、同样的上限。一个我没想到的细节是：invalid-params 计数器只应在 tool.build(args) 真正抛错时才累加（不是 tool body 在 build 成功后抛错）。PR 通过 catch 块里的 toolBuildSucceeded 标志正确处理了这一点。

结论：

• 复用：干净。PR 扩展了现有 LoopType 枚举，复用 logLoopDetected / LoopDetectedEvent，并把新枚举值加入 nonInteractiveCli 的 loop message formatter，保持用户可见文案一致。没有平行基础设施。
• 范围：紧凑。所有改动都服务于目标，没有顺手重构，没有投机抽象。新的 DaemonToolLoopState 和三个辅助函数都放在使用它们的 Session.ts 里 —— 这是合适的，因为 daemon turn 循环是 ACP 特有的问题（core 的 loopDetectionService 处理的是 chanting、action-stagnation、alternating patterns 等不同类型的 loop，不是它的合适归宿）。
• 正确性：cap 检查在 batch 入口（runTools），invalid-params 检查在每次调用（runTool），两者都返回 loopDetected: true 并向上传播终止 turn。四个 ACP turn 入口各自调用 createDaemonToolLoopState()，所以状态确实是 per-turn 的。invalid-params 记录上的 !activeToolAbortSignal.aborted 守卫也是对的 —— 不该把 abort 竞争导致的失败计入。
• 小问题（非阻塞）：recordDaemonToolCalls 在早退分支里写的是 loopState?.loopDetected ?? false。语义正确但我读了两遍才确认。未来读者可能更愿意看到一个命名局部变量，但不值得为此要求修改。

无 critical blocker。无 AGENTS.md 违规。代码读起来像维护者写的。

真实场景测试

先说实话：这个 bug 在 ACP daemon 路径 —— 只有模型用新 tool-call id 反复发送相同非法参数时才会触发。它无法通过普通 interactive/non-interactive qwen 提示触发，也没有确定性的方式在没有 mock ACP client + 脚本化模型的情况下从 CLI 诱导。所以完整 before/after tmux 复现 bug 本身不可行。我能展示的是：

• 一个聚焦的单元回归测试，复现了完全一致的 bug 形态（三次 ask_user_question 调用，questions 是 JSON 字符串，fresh id ask_1/ask_2/ask_3），断言 turn 在三次后停止。
• PR 上 build + typecheck + lint 全绿。
• 一个 tmux smoke test：bundled PR 对比当前 main 仍能正常启动，加上 grep 确认新 INVALID_TOOL_PARAMS_STAGNATION 标签进入了 PR bundle、main 里没有。

（测试输出见上文英文部分，已包含完整 capture-pane / vitest 输出。）

— Qwen Code · qwen3.7-max

[qwen-code-ci-bot]

Stepping back: this PR reads like a maintainer's fix for a bug they found in traces.

• The motivation is concrete — a specific failure mode observed in ACP daemon traces (ACP daemon can loop indefinitely on repeated invalid tool parameters #6075), not a hypothetical.
• The implementation matches what I would have proposed independently. The PR even got the subtle part right: only counting validation failures when tool.build(args) throws, not when the tool body throws after a successful build. That's the difference between "model sent bad args" and "tool execution failed," and the guard should only fire on the former.
• Scope is minimal. Five files, ~230 lines, zero drive-by changes. The new state struct, three helpers, and enum value are exactly what the goal needs.
• Reuse is clean — extends the existing LoopType / LoopDetectedEvent / logLoopDetected pipeline instead of inventing a parallel one. The new helpers stay local to Session.ts since the daemon turn loop is ACP-specific; the core loopDetectionService handles different loop shapes and isn't the right home.
• Verification is solid. The focused regression test reproduces the exact bug shape and asserts the turn stops at three. Full related suites pass (206 Session + 69 nonInteractiveCli + 72 loopDetectionService). Build, typecheck, lint, and bundle all green. The tmux before/after is a smoke test only — the bug itself lives in the ACP daemon path and isn't reproducible from a normal qwen prompt without a scripted ACP client, so that's the honest limit of real-scenario coverage here.

Approval guardrail check: gh pr view --json isCrossRepository,title → ok (cross-repo but title is not refactor-prefixed, so auto-approval is allowed).

Approving. ✅

中文说明

退一步看：这个 PR 读起来像是维护者修了一个自己在 trace 里发现的 bug。

• 动机具体 —— ACP daemon can loop indefinitely on repeated invalid tool parameters #6075 记录的是 ACP daemon trace 里观察到的真实失败模式，不是假设场景。
• 实现和我独立提出的方案一致。PR 还把那个微妙细节做对了：只在 tool.build(args) 抛错时计数，tool body 在 build 成功后抛错时不计。这是「模型传错参数」和「tool 执行失败」的区别，守卫只应在前者触发。
• 范围最小化。5 个文件、约 230 行、零顺手改动。新的 state 结构体、三个辅助函数、一个枚举值，正好是目标所需。
• 复用干净 —— 扩展现有 LoopType / LoopDetectedEvent / logLoopDetected 管线，没有发明平行基础设施。新辅助函数留在 Session.ts 本地，因为 daemon turn 循环是 ACP 特有的；core 的 loopDetectionService 处理不同形态的 loop，不是合适归宿。
• 验证扎实。聚焦回归测试复现了完全一致的 bug 形态并断言 turn 在三次后停止。相关完整测试套件全绿（Session 206 + nonInteractiveCli 69 + loopDetectionService 72）。build、typecheck、lint、bundle 全绿。tmux before/after 只是 smoke test —— bug 本身在 ACP daemon 路径，没有脚本化 ACP client 无法从普通 qwen 提示复现，这是真实场景覆盖的诚实上限。

Approval guardrail 检查：gh pr view --json isCrossRepository,title → ok（cross-repo 但 title 不以 refactor 开头，允许自动 approve）。

— Qwen Code · qwen3.7-max

[qwen-code-ci-bot]

LGTM, looks ready to ship. ✅

[DragonnZhang]

Review Summary — PR #6076: Stop repeated invalid tool parameter loops in ACP

Scope reviewed: Loop detection logic in Session.ts, error-message handling in errors.ts/fileUtils.ts/write-file.ts, CacheSafeParams history copying in forkedAgent.ts, autonomous loop sentinel infrastructure, AcpFileSystemService local-read-root configuration.

Overall Assessment

The core loop-detection fix is well-structured. The DaemonToolLoopState approach with a Map<string, number> keyed by toolName\0error.message and a hard tool-call cap is a clean, minimal solution to the reported bug. The toolBuildSucceeded flag correctly scopes the invalid-params guard to tool.build() failures only (not execution errors), which is the right design choice.

Existing inline comments already cover the key concerns:

1. Stop-hook bypass (Session.ts ~line 2063) — #handleStopHookLoop creates a fresh DaemonToolLoopState, so a stop-hook continuation can re-enter the prompt loop with a clean loop-guard slate. This is the most significant concern.

2. Error text bucketing (Session.ts ~line 282) — Using the exact error.message string as a map key may under-count when validators embed user-controlled values (e.g., file paths) in error messages, causing the guard to miss genuine loops.

3. Concurrent batch short-circuit (Session.ts ~line 3954) — In a concurrent batch, runBounded continues scheduling calls after loopDetected is set. The shouldSkipUnstarted callback is only wired to abort signals, not to the loop state.

Verified as correct:

• Threshold logic: The >= comparison with threshold 3 correctly stops at the 3rd repeated failure, matching the test expectations.
• Cap logic: recordDaemonToolCalls counts the full batch size upfront (conservative over-counting), which is acceptable for a safety cap.
• Telemetry: LoopType.INVALID_TOOL_PARAMS_STAGNATION is properly added to the enum and to LOOP_TYPE_LABELS with the correct always-on classification.
• Error message handling: The getErrorMessage enhancements (plain object support, cause unwrapping, 1000-char truncation) are well-implemented with thorough test coverage.
• CacheSafeParams history copying: copyHistoryContainers shallow-copies Content[] and parts[] containers while sharing part objects by reference — correct for mutation isolation without deep cloning large history.
• Test coverage: The focused regression test for the invalid-params loop is sound and verifies the fix.

PR scope note:

This PR is titled for the loop detection fix but bundles several additional changes (autonomous loop sentinels, AcpFileSystemService local-read-root configuration, subagent-result.ts tag stripping, error message formatting improvements, npx → cross-env script fix, serve fast-path bundle check in CI). These are well-structured but extend the PR's scope beyond what the title suggests. Consider splitting in the future for easier review and bisect.