Skip to content

fix(core): halt repeated shell inspection variants - #5944

Merged
yiliang114 merged 9 commits into
QwenLM:mainfrom
yiliang114:codex/fix-4695-tool-loop
Jun 28, 2026
Merged

fix(core): halt repeated shell inspection variants#5944
yiliang114 merged 9 commits into
QwenLM:mainfrom
yiliang114:codex/fix-4695-tool-loop

Conversation

@yiliang114

@yiliang114 yiliang114 commented Jun 27, 2026

Copy link
Copy Markdown
Collaborator

What this PR does

Adds an always-on loop guard for repeated shell-based git overview inspection variants. When the model keeps calling run_shell_command with semantically similar read-only repository overview commands such as git status, overview-style git diff, or git ls-files, Qwen Code now halts the run with shell_command_stagnation before it can churn until the per-turn hard cap.

The guard intentionally fails open for broader shell activity: file-specific diffs such as git diff -- src/file.ts and git diff src/file.ts are not bucketed, compound commands that stage or commit work are not bucketed, and a non-inspection tool call resets the streak. This keeps the always-on detector narrow enough for a core loop-detection service while still covering the issue's repeated shell inspection pattern.

Why it's needed

Issue #4695 reports a long-context DeepSeek-compatible session where the model repeatedly called shell tools while varying the command text, so the exact repeated-call guard did not catch the loop. The default model.skipLoopDetection setting can skip heuristic stagnation detection, leaving only the exact-call guard and the 100-tool hard cap. This change adds targeted always-on protection for the observed git inspection variant loop.

Reviewer Test Plan

How to verify

Ask an OpenAI-compatible endpoint to repeatedly return run_shell_command calls that vary between overview git status, git diff, and git ls-files inspections. The run should execute the first seven tool results, then halt on the eighth request with shell_command_stagnation; it should not continue until --max-tool-calls or the 100-call hard cap.

Also verify the negative cases: a legitimate sequence of file-specific git diff -- <path> review commands should not halt, compound shell commands that write to the repo should not be bucketed as read-only inspection, and any non-inspection tool call should reset the shell-inspection streak.

Evidence (Before & After)

Before: the regression test failed because the eighth similar git inspection variant did not trip any always-on guard.

After: the regression tests pass, including the false-positive cases added during review, and a tmux-driven E2E run against the bundled CLI halted with Loop detection halted the run (shell_command_stagnation: the model repeated similar shell inspection commands without making progress). This is an always-on guard and cannot be disabled via \model.skipLoopDetection`.`

Local verification after the final pushed commit:

  • cd packages/core && npx vitest run src/services/loopDetectionService.test.ts passed: 72 tests.
  • cd packages/cli && npx vitest run src/nonInteractiveCli.test.ts passed: 57 passed, 1 skipped.
  • npm run lint passed.
  • npm run typecheck passed.
  • npm run build passed; existing vscode companion curly warnings and frontend bundle-size/browserslist warnings were present, with exit code 0.
  • npm run bundle passed.
  • tmux E2E passed: real bundled dist/cli.js, local fake OpenAI-compatible endpoint, 8 chat completion requests served, 7 tool uses emitted before halt, 7 tool results executed before halt, final loop type shell_command_stagnation.

Tested on

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

Environment (optional)

Node.js local workspace build using dist/cli.js; tmux E2E used a temporary git repository under /private/tmp and a local fake OpenAI-compatible streaming endpoint to deterministically return the repeated tool-call sequence.

Risk & Scope

  • Main risk or tradeoff: Any always-on guard in LoopDetectionService affects all runs, so this is intentionally scoped to overview-style run_shell_command git inspections only. It excludes file-specific diffs, write-bearing compound chains, and interrupted streaks.
  • Not validated / out of scope: Windows and Linux local runs were not performed; the tmux E2E used an OpenAI-compatible fake endpoint rather than a live DeepSeek endpoint so the tool-call sequence is deterministic.
  • Breaking changes / migration notes: None.

Linked Issues

Fixes #4695

中文说明

这个 PR 做了什么

新增一个 always-on 循环保护,用于拦截重复的 shell git 概览检查变体。当模型持续调用 run_shell_command,并在 git status、概览类 git diffgit ls-files 这类语义相近的只读仓库概览命令之间变换时,Qwen Code 现在会在达到 per-turn 硬上限前以 shell_command_stagnation 中止运行。

这个保护刻意 fail open:git diff -- src/file.ts 这类文件级 review 命令不入桶,包含 git add / git commit 等写操作的复合 shell 命令不入桶,夹杂非检查工具调用时也会重置 streak。这样可以在覆盖 issue 里的重复 shell 检查模式时,尽量降低 core loop detector 的误伤面。

为什么需要

#4695 报告了一个长上下文 DeepSeek-compatible 会话:模型不断调用 shell 工具,但会变换命令文本,因此完全相同工具调用保护无法命中。默认 model.skipLoopDetection 可能让启发式 stagnation 检测不运行,实际只剩完全相同调用保护和 100 次工具调用硬上限。这个改动为已观察到的 git 检查变体循环增加了一个有针对性的 always-on 保护。

Reviewer Test Plan

How to verify

让一个 OpenAI-compatible endpoint 连续返回在概览类 git statusgit diffgit ls-files 之间变化的 run_shell_command 调用。运行应执行前七个工具结果,并在第八次请求时以 shell_command_stagnation 中止;不应继续跑到 --max-tool-calls 或 100 次硬上限。

同时验证负向场景:连续的文件级 git diff -- <path> review 命令不应中止,包含写入仓库行为的复合 shell 命令不应被当作只读检查入桶,非检查工具调用应重置 shell-inspection streak。

Evidence (Before & After)

Before:新增回归测试失败,因为第八个相似 git 检查变体没有触发任何 always-on guard。

After:回归测试通过;复核时补充的误伤测试也通过;tmux 驱动的真实 bundle CLI E2E 运行以 Loop detection halted the run (shell_command_stagnation: the model repeated similar shell inspection commands without making progress). This is an always-on guard and cannot be disabled via \model.skipLoopDetection`.` 中止。

最终推送 commit 后的本地验证:

  • cd packages/core && npx vitest run src/services/loopDetectionService.test.ts 通过:72 个测试。
  • cd packages/cli && npx vitest run src/nonInteractiveCli.test.ts 通过:57 个通过,1 个跳过。
  • npm run lint 通过。
  • npm run typecheck 通过。
  • npm run build 通过;存在既有 vscode companion curly warnings 和前端 bundle-size/browserslist warnings,退出码为 0。
  • npm run bundle 通过。
  • tmux E2E 通过:使用真实打包后的 dist/cli.js、本地 fake OpenAI-compatible endpoint,服务端收到 8 次 chat completion 请求,中止前发出 7 次 tool use,真实执行 7 个 tool result,最终 loop type 为 shell_command_stagnation

Tested on

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

Environment (optional)

Node.js 本地 workspace build,使用 dist/cli.js;tmux E2E 在 /private/tmp 下创建临时 git 仓库,并使用本地 fake OpenAI-compatible streaming endpoint 稳定返回重复工具调用序列。

Risk & Scope

  • Main risk or tradeoff:LoopDetectionService 的 always-on guard 会影响所有运行,所以这次刻意只覆盖 run_shell_command 里的概览类 git inspection。文件级 diff(包括 git diff -- pathgit diff path)、带写操作的复合命令、被非检查工具打断的 streak 都被排除。
  • Not validated / out of scope:未在 Windows 和 Linux 本地运行;tmux E2E 使用 OpenAI-compatible fake endpoint,而不是 live DeepSeek endpoint,以便稳定产生工具调用序列。
  • Breaking changes / migration notes:无。

Linked Issues

Fixes #4695

@yiliang114

Copy link
Copy Markdown
Collaborator Author

Qwen Code #4695 tmux E2E report

  • Result: PASS
  • CLI: /Users/jinjing/projects/projj/github.com/QwenLM/qwen-code/.worktrees/fix-4695-tool-loop/dist/cli.js
  • Project cwd: /private/tmp/qwen-4695-tmux-project
  • Fake OpenAI-compatible endpoint: http://127.0.0.1:31995/v1
  • Child exit code: 1
  • Chat completion requests served: 8
  • Tool uses emitted before halt: 7
  • Tool results executed before halt: 7
  • Loop type: shell_command_stagnation
  • Stdout: /private/tmp/qwen-4695-tmux-report/stdout.json
  • Stderr: /private/tmp/qwen-4695-tmux-report/stderr.txt
  • Summary: /private/tmp/qwen-4695-tmux-report/summary.json

Served command sequence

  1. git status --short
  2. git status --short && git diff --stat
  3. git diff --name-only HEAD
  4. git status --porcelain=v1
  5. git diff --stat HEAD
  6. git -C . status --short
  7. git --no-pager diff --stat
  8. git ls-files --modified

Halt message

Loop detection halted the run (shell_command_stagnation: the model repeated similar shell inspection commands without making progress). This is an always-on guard and cannot be disabled via `model.skipLoopDetection`.

@wenshao wenshao left a comment

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.

The JSDoc on checkAlwaysOnSafeties (line 253) still says "Enforces two guards" but the method now contains three (consecutive-identical, shell command stagnation, per-turn cap). Consider updating the doc comment to reflect the new guard.

— qwen3.7-max via Qwen Code /review

Comment thread packages/core/src/services/loopDetectionService.test.ts
Comment thread packages/core/src/services/loopDetectionService.ts
Address review on QwenLM#5944: the checkAlwaysOnSafeties JSDoc still said it
enforces two guards after the shell inspection-command stagnation guard
was added, so correct it to three.

Add a regression test proving a non-inspection tool call resets the
shell-stagnation streak to zero, which is the guard's main false-positive
defense and was previously untested.
isGitInspectionCommand matched if any segment of a shell chain was a git
status/diff/ls-files, so a productive chain like
`git add . && git status && git commit` was classified as read-only
inspection. Eight such commands would falsely trip the always-on
shell-stagnation guard.

Require every segment of the chain to be a read-only git inspection
before bucketing. Mixed chains that also write fail open (non-inspection),
the safe direction for an always-on halt. The QwenLM#4695 loop is still caught:
its compound case is status && diff, both read-only.

@wenshao wenshao left a comment

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] Two inline comments at the checkAlwaysOnSafeties call site in packages/core/src/core/client.ts still say "two guards" (lines 2146-2148 and 2178). The JSDoc in loopDetectionService.ts:253 was correctly updated to say "three guards", but client.ts was missed. An oncall engineer reading the dispatch point won't know the shell-stagnation guard exists.

Fix: update both comments to mention all three guards: "consecutive-identical tool-call guard, shell inspection-command stagnation guard, and per-turn tool-call cap".

Also, the new guard's !this.disabledForSession gate (line 301) lacks a dedicated test — the consecutive-identical guard has an analogous disableForSession test at test line 204. Consider adding one to catch regressions that drop the gate.

— qwen3.7-max via Qwen Code /review

Comment thread packages/core/src/services/loopDetectionService.ts Outdated
@yiliang114

Copy link
Copy Markdown
Collaborator Author

Updated review and tmux E2E report

After a second pass focused on the always-on LoopDetectionService risk surface, I narrowed the guard from broad shell git inspection matching to overview-only git inspection streaks. The branch now explicitly covers these false-positive boundaries:

  • File-specific review commands such as git diff -- src/file.ts do not trip shell_command_stagnation.
  • Compound shell commands that also write to the repository, such as git add ... && git status ... && git commit ..., are not bucketed as read-only inspection.
  • A non-inspection tool call resets the shell-inspection streak.

Final local verification after the pushed commit:

  • cd packages/core && npx vitest run src/services/loopDetectionService.test.ts passed: 66 tests.
  • cd packages/cli && npx vitest run src/nonInteractiveCli.test.ts passed: 57 passed, 1 skipped.
  • npm run lint passed.
  • npm run typecheck passed.
  • npm run build passed with exit code 0; existing vscode companion curly warnings and frontend bundle-size/browserslist warnings were present.
  • npm run bundle passed.
  • tmux E2E passed against the bundled CLI.

Qwen Code #4695 tmux E2E report

  • Result: PASS
  • CLI: /Users/jinjing/projects/projj/github.com/QwenLM/qwen-code/.worktrees/fix-4695-tool-loop/dist/cli.js
  • Project cwd: /private/tmp/qwen-4695-tmux-project
  • Fake OpenAI-compatible endpoint: http://127.0.0.1:31995/v1
  • Child exit code: 1
  • Chat completion requests served: 8
  • Tool uses emitted before halt: 7
  • Tool results executed before halt: 7
  • Loop type: shell_command_stagnation
  • Stdout: /private/tmp/qwen-4695-tmux-report/stdout.json
  • Stderr: /private/tmp/qwen-4695-tmux-report/stderr.txt
  • Summary: /private/tmp/qwen-4695-tmux-report/summary.json

Served command sequence

  1. git status --short
  2. git status --short && git diff --stat
  3. git diff --name-only HEAD
  4. git status --porcelain=v1
  5. git diff --stat HEAD
  6. git -C . status --short
  7. git --no-pager diff --stat
  8. git ls-files --modified

Halt message

Loop detection halted the run (shell_command_stagnation: the model repeated similar shell inspection commands without making progress). This is an always-on guard and cannot be disabled via `model.skipLoopDetection`.

@yiliang114

Copy link
Copy Markdown
Collaborator Author

Follow-up architecture/product review

I re-tested the previous reproduction case on the latest PR head and then did another product-surface pass on the always-on detector.

Result for the original case: it no longer reproduces as an unbounded loop. With the bundled CLI, the repeated overview inspection sequence halted on request 8 with shell_command_stagnation; 7 tool calls completed before the guard fired.

During the product review, I found one more false-positive boundary: git diff src/file.ts is also a legitimate file-specific review command, not just git diff -- src/file.ts. Because this guard is always-on and lives in the central LoopDetectionService, ambiguous git diff arguments should fail open rather than halt user work. I pushed a follow-up commit that treats git diff as overview-only only when its positional arguments are options or clear revisions/ranges; ordinary pathspec-like arguments are treated as progress.

Fresh verification after the follow-up commit:

  • cd packages/core && npx vitest run src/services/loopDetectionService.test.ts passed: 67 tests.
  • cd packages/cli && npx vitest run src/nonInteractiveCli.test.ts passed: 57 passed, 1 skipped.
  • npx prettier --check packages/core/src/services/loopDetectionService.ts packages/core/src/services/loopDetectionService.test.ts packages/core/src/core/client.ts packages/cli/src/nonInteractiveCli.ts packages/core/src/telemetry/types.ts passed.
  • npm run lint passed.
  • npm run typecheck passed.
  • npm run build passed with exit code 0; existing vscode companion curly warnings and frontend bundle-size/browserslist warnings were present.
  • npm run bundle passed.
  • tmux E2E passed against the bundled CLI: 8 chat completion requests served, 7 tool uses emitted before halt, 7 tool results executed before halt, final loop type shell_command_stagnation.

@yiliang114

Copy link
Copy Markdown
Collaborator Author

Additional guard-boundary tests

Added a test-only follow-up for the shell-stagnation guard. No production logic changed in this commit.

New coverage:

  • Retry replay resets the shell-inspection streak, so a retried stream does not double-count into shell_command_stagnation.
  • In-session loop-detection disable still suppresses shell inspection stagnation, matching the existing always-on identical-call behavior.
  • Shell chains that include non-git work such as git status && npm test are not bucketed as stagnant git overview inspection.

Fresh checks after the commit:

  • cd packages/core && npx vitest run src/services/loopDetectionService.test.ts passed: 70 tests.
  • npx prettier --check packages/core/src/services/loopDetectionService.test.ts passed.
  • npm run lint passed.

@wenshao

wenshao commented Jun 28, 2026

Copy link
Copy Markdown
Collaborator

Real-binary E2E — the guard works ✅, but the current head fails to build ❌ (1-line test fix)

I drove the actual qwen binary against a fake OpenAI endpoint that forces run_shell_command tool calls cycling through git-inspection variants, and observed the real run halt. The feature behaves exactly as described. However, the current head d8ab43fe1 does not compile — the newest (test-only) commit introduced a TypeScript error that breaks tsc --build, which is why the Test (ubuntu-latest) leg is red. Both findings below.

⚠️ Head moved during review: I first verified c2c73a777; the head is now d8ab43fe1 ("test(core): cover shell stagnation guard boundaries"). That commit changes only the test file (no production change — loopDetectionService.ts is byte-identical between the two), so the behavioral results hold for the new head; but the new test does not type-check.

1) Behavior: verified ✅ (real binary, head d8ab43fe1 production code)

Each row is a real qwen -p --approval-mode=yolo run against the fake endpoint; "executed" = role:'tool' results actually run before the halt.

# Scenario requests tool results executed halted? loop type
S1 8 varying git-overview commands (git status / overview git diff / git ls-files) 8 7 ✅ halt on 8th shell_command_stagnation
S2 same, but model.skipLoopDetection: true 8 7 ✅ still halts shell_command_stagnation (always-on)
S3 file-specific git diff -- file-N.txt ×14 19 18 ❌ no halt (fails open)
S4 git-inspection interrupted by echo … (non-inspection) 19 18 ❌ no halt (streak resets)

The halt message (printed by the real CLI) is exactly:

Loop detection halted the run (shell_command_stagnation: the model repeated similar
shell inspection commands without making progress). This is an always-on guard and
cannot be disabled via `model.skipLoopDetection`.

Mutation A/B (proves it's this PR): reverting loopDetectionService.ts to base + rebuilding core (grep checkShellCommandStagnation dist → 0) → S1 no longer halts: 18 executions, runs to the cap — i.e. exactly the #4695 unbounded loop. Restored → halts at 8 again. S2 confirms the guard runs in checkAlwaysOnSafeties before the skipLoopDetection gate.

2) Build: BROKEN on the current head ❌ (1-line fix)

npm run typecheck (and the CI build's tsc --build) fail with a single error from the newest commit's test:

src/services/loopDetectionService.test.ts(335,11): error TS2353:
  Object literal may only specify known properties, and 'value' does not
  exist in type 'ServerGeminiRetryEvent'.

The test constructs a Retry event as { type: GeminiEventType.Retry, value: {} }, but ServerGeminiRetryEvent (core/turn.ts:72) has no value field (only type / retryInfo? / isContinuation?). vitest/esbuild transpile-only, so the unit suite passes — but tsc --build (run by the core build during npm ci) rejects it, failing Test (ubuntu-latest) at Install dependencies. I reproduced it locally (core build exit 1, identical error).

Confirmed minimal fix: delete the value: {} line (leaving { type: GeminiEventType.Retry }) → npm run typecheck then exits 0 with no other errors. (Applied locally only to confirm; reverted — not committed.)

Verdict

  • The fix itself is correct and thoroughly verified — always-on halt at the 8th git-inspection variant, undisablable via skipLoopDetection, with the documented fail-open cases all holding.
  • Not mergeable as-is: the head fails tsc --build. One-line fix needed (drop value: {} at loopDetectionService.test.ts:335), then re-run CI.
  • BLOCKED here is partly the real red build (above) and partly REVIEW_REQUIRED. macOS (Darwin, Node 22); production behavior verified, the build break also reproduced locally.
🇨🇳 中文版本

真实二进制 E2E —— 守卫功能正常 ✅,但当前 head 编译失败 ❌(一行测试修复)

我用真实 qwen 二进制对接一个强制 run_shell_command 在 git 检查变体间循环的伪 OpenAI 端点,观察真实运行中止。功能行为与描述完全一致。但当前 head d8ab43fe1 无法编译——最新的(纯测试)commit 引入了一个 TypeScript 错误,破坏了 tsc --build,这正是 Test (ubuntu-latest) 变红的原因。两点结论如下。

⚠️ review 期间 head 移动了: 我最初验证的是 c2c73a777;现在 head 是 d8ab43fe1("test(core): cover shell stagnation guard boundaries")。该 commit 只改测试文件(生产代码无变化——loopDetectionService.ts 两者逐字节相同),所以行为结论对新 head 仍成立;但新测试无法通过类型检查。

1) 行为:已验证 ✅(真实二进制,head d8ab43fe1 生产代码)

每行都是真实 qwen -p --approval-mode=yolo 对接伪端点的运行;"executed" = 中止前真实执行的 role:'tool' 结果数。

# 场景 请求数 执行的工具结果 是否中止 loop type
S1 8 个变化的 git 概览命令(git status / 概览 git diff / git ls-files 8 7 ✅ 第 8 次中止 shell_command_stagnation
S2 同上,但 model.skipLoopDetection: true 8 7 ✅ 仍然中止 shell_command_stagnation(always-on)
S3 文件级 git diff -- file-N.txt ×14 19 18 ❌ 不中止(fail open)
S4 git 检查被 echo …(非检查命令)打断 19 18 ❌ 不中止(streak 重置)

真实 CLI 打印的中止信息正是:

Loop detection halted the run (shell_command_stagnation: the model repeated similar
shell inspection commands without making progress). This is an always-on guard and
cannot be disabled via `model.skipLoopDetection`.

变异 A/B(证明来自本 PR):loopDetectionService.ts revert 到 base 并重建 core(grep checkShellCommandStagnation dist → 0)→ S1 不再中止:执行 18 次,一直跑到上限——正是 #4695 的无界循环。还原后再次在第 8 次中止。S2 证明该守卫在 checkAlwaysOnSafeties 中、在 skipLoopDetection之前运行。

2) 构建:当前 head 已损坏 ❌(一行修复)

npm run typecheck(以及 CI 构建的 tsc --build)因最新 commit 的测试报单个错误:

src/services/loopDetectionService.test.ts(335,11): error TS2353:
  Object literal may only specify known properties, and 'value' does not
  exist in type 'ServerGeminiRetryEvent'.

该测试把 Retry 事件构造成 { type: GeminiEventType.Retry, value: {} },但 ServerGeminiRetryEventcore/turn.ts:72)没有 value 字段(只有 type / retryInfo? / isContinuation?)。vitest/esbuild 只转译不做类型检查,所以单测能过——但 core 构建(npm ci 时)跑的 tsc --build 会拒绝它,使 Test (ubuntu-latest)Install dependencies 步骤失败。我本地复现了(core build 退出码 1,错误一致)。

已确认的最小修复: 删掉 value: {} 这一行(保留 { type: GeminiEventType.Retry })→ npm run typecheck 退出码变 0,无其他错误。(仅本地应用以确认,已 revert,未提交。)

结论

  • 修复本身正确且已充分验证 —— 在第 8 个 git 检查变体处 always-on 中止,无法通过 skipLoopDetection 关闭,文档化的 fail-open 场景全部成立。
  • 当前状态不可合并: head 过不了 tsc --build。需一行修复(删 loopDetectionService.test.ts:335value: {}),然后重跑 CI。
  • 这里的 BLOCKED 一部分是上面真实的红构建,一部分是 REVIEW_REQUIRED。环境 macOS(Darwin, Node 22);生产行为已验证,构建破坏也已本地复现。

wenshao
wenshao previously approved these changes Jun 28, 2026
@yiliang114
yiliang114 marked this pull request as ready for review June 28, 2026 11:00
@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Thanks for the PR!

Template looks good ✓ — all required sections present with bilingual content.

On direction: this directly addresses the reported issue #4695 where a model could loop on varied git inspection commands while evading the exact-match consecutive-identical guard. Adding a narrower always-on guard for the specific observed pattern (overview-style git status/diff/ls-files) is the right call. The area is relevant — loop detection is a core safety service, and the gap the PR fills is well-documented in the issue.

On approach: the scope feels right. The guard is intentionally narrow — only overview-style git commands, excluding file-specific diffs, write-bearing compound chains, and interrupted streaks. Threshold of 8 matches the existing action-stagnation guard, which gives legitimate branch-review flows enough headroom. The 5-file diff is focused: one new enum value, the guard logic, wiring into the always-on tier, and thorough tests. No unrelated changes.

Moving on to code review. 🔍

中文说明

感谢贡献!

模板完整 ✓ — 所有必要章节齐全,包含中英双语。

方向:直接修复 #4695 报告的问题——模型通过变换 git 检查命令文本绕过完全相同调用保护。为观察到的特定模式(概览类 git status/diff/ls-files)增加更窄的 always-on 保护是正确做法。循环检测是核心安全服务,这个缺口在 issue 中有充分记录。

方案:范围合理。保护刻意只覆盖概览类 git 命令,排除文件级 diff、写操作复合链和被中断的 streak。阈值 8 与已有的 action-stagnation 保护一致,给合法的分支 review 流程留足空间。5 个文件的 diff 聚焦:一个新枚举值、保护逻辑、接入 always-on 层、以及充分的测试。没有无关改动。

进入代码审查 🔍

Qwen Code · qwen3.7-max

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Code Review

Read the diff against my independent proposal (add a counter for consecutive overview-style git shell calls, reset on non-inspection tools, halt at threshold). The PR's implementation matches or exceeds this — the getShellInspectionKey → single bucket key 'run_shell_command:git-inspection' design is clean and the isGitOverviewInspectionCommand classifier correctly fails open for compound commands, file-specific diffs, and non-git shell calls.

No critical issues found. Two minor observations (not blockers):

  • The streak comparison uses lastShellInspectionKey === key which means any git overview maps to the same bucket. This is the correct behavior for the reported pattern (model varies between status/diff/ls-files) but means the guard counts category repetitions, not individual command repetitions. The naming could be slightly more explicit about this, but it's clear from the tests.
  • The regex split /&&|\|\||[;&|\n]/ doesn't handle pipes (git status | head), which means piped git overviews won't be classified. This is the safe direction (fail open) — a real loop of piped commands would still be caught by the consecutive-identical guard.

Reuse Check

The guard integrates cleanly into the existing LoopDetectionService always-on tier — no new abstractions, no duplicated logic. resetToolCallCount() is correctly extended to clear the new state. ✅

Unit Tests

  • loopDetectionService.test.ts: 72/72 pass (including 12 new tests for the shell stagnation guard)
  • npm run typecheck: clean
  • npm run lint: clean (no errors; pre-existing vscode companion warnings only)
  • npm run build && npm run bundle: clean

E2E: Before/After with Fake OpenAI Endpoint

Tested with a local fake OpenAI-compatible endpoint that deterministically returns 10 varying git inspection run_shell_command tool calls followed by a stop response. YOLO mode, non-interactive -p prompt.

Before (installed qwen v0.19.1 — no shell_command_stagnation guard)

=== BEFORE: installed qwen v0.19.1 (no shell_command_stagnation guard) ===
Warning: QWEN_HOME points to "/home/github-runner/actions-runner-4/_work/_temp/qwen-home" but no settings.json was found there.
Done inspecting.
=== Before test completed ===

Ran all 10 tool calls without halting. The fake endpoint eventually returned a stop response. Without the guard, a real model could continue this loop until the 100-tool hard cap.

After (this PR's bundled build — with shell_command_stagnation guard)

=== AFTER: PR build (with shell_command_stagnation guard) ===
Warning: QWEN_HOME points to "/home/github-runner/actions-runner-4/_work/_temp/qwen-home" but no settings.json was found there.
Loop detection halted the run (shell_command_stagnation: the model repeated similar shell inspection commands without making progress). This is an always-on guard and cannot be disabled via `model.skipLoopDetection`.
=== After test completed (exit code: 0) ===

Halted at the 8th consecutive git inspection tool call with the expected shell_command_stagnation message. The always-on indicator is present. ✅

中文说明

代码审查

将 diff 与我的独立方案(为连续概览类 git shell 调用添加计数器,非检查工具重置,达到阈值中止)对比。PR 的实现匹配或优于该方案——getShellInspectionKey → 单一桶键 'run_shell_command:git-inspection' 的设计简洁,isGitOverviewInspectionCommand 分类器对复合命令、文件级 diff、非 git shell 调用正确 fail open。

未发现关键问题。

复用检查

保护干净地集成到现有的 LoopDetectionService always-on 层——无新抽象、无重复逻辑。resetToolCallCount() 正确扩展清除新状态。✅

单元测试

  • loopDetectionService.test.ts72/72 通过(含 12 个 shell stagnation 新增测试)
  • npm run typecheck通过
  • npm run lint通过
  • npm run build && npm run bundle通过

E2E:Before/After 使用 Fake OpenAI 端点

使用本地 fake OpenAI-compatible 端点,确定性返回 10 个变换的 git 检查 run_shell_command 工具调用,之后返回 stop 响应。YOLO 模式,非交互 -p 提示。

Before(安装的 qwen v0.19.1): 执行全部 10 个工具调用未中止。没有 guard,真实模型可能循环到 100 次硬上限。

After(此 PR 构建): 在第 8 个连续 git 检查工具调用时以 shell_command_stagnation 中止。✅

Qwen Code · qwen3.7-max

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Reflection

Stepping back, this PR does exactly what it sets out to do: close a specific gap in the always-on loop detection where a model could cycle through semantically similar but textually different git inspection commands without triggering any guard.

The implementation matches my independent proposal. The single-bucket key approach ('run_shell_command:git-inspection' for all overview variants) is the right abstraction — it catches the pattern (repeated repo inspection) rather than the text (specific command), which is exactly what the reported issue needed. The classifier is conservative: it requires every segment of a compound command to be a git overview, excludes file-specific diffs, and resets on any non-inspection tool call. This makes false positives very unlikely for an always-on guard.

The test suite is thorough — 12 new tests covering the positive case, streak reset on non-inspection tools, retry reset, session disable, compound commands with writes, non-git shell chains, file-specific diffs with and without --, and newline-separated variants. The before/after E2E confirmed the guard fires at exactly the 8th consecutive inspection, matching the threshold.

Every change in the diff is necessary: the new LoopType enum value, the guard logic in LoopDetectionService, the wiring in checkAlwaysOnSafeties, the label/hint in nonInteractiveCli.ts, and the tests. No scope creep, no drive-by refactors.

This is a clean, well-scoped fix with solid test coverage. Ready to ship. ✅

中文说明

反思

退一步看,这个 PR 精确完成了它的目标:填补 always-on 循环检测中的一个特定缺口——模型可以在语义相似但文本不同的 git 检查命令之间循环而不触发任何保护。

实现与我的独立方案一致。单一桶键方法(所有概览变体共享 'run_shell_command:git-inspection')是正确的抽象——它捕获模式(重复仓库检查)而非文本(具体命令),正是 issue 所需要的。分类器保守:要求复合命令的每个段都是 git 概览,排除文件级 diff,在非检查工具调用时重置。这使得 always-on 保护的误报可能性很低。

测试套件充分——12 个新测试覆盖正向场景、非检查工具重置 streak、retry 重置、session disable、写操作复合命令、非 git shell 链、有/无 -- 的文件级 diff、换行分隔变体。Before/after E2E 确认保护在第 8 个连续检查时精确触发,与阈值一致。

diff 中的每个改动都是必要的:新 LoopType 枚举值、LoopDetectionService 中的保护逻辑、checkAlwaysOnSafeties 中的接入、nonInteractiveCli.ts 中的标签/提示、以及测试。无范围蔓延、无顺手重构。

干净、范围合理的修复,测试覆盖扎实。可以合入。✅

Qwen Code · qwen3.7-max

@qwen-code-ci-bot qwen-code-ci-bot left a comment

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.

LGTM, looks ready to ship. ✅

const isAlwaysOn =
loopType === LoopType.TURN_TOOL_CALL_CAP ||
loopType === LoopType.CONSECUTIVE_IDENTICAL_TOOL_CALLS ||
loopType === LoopType.SHELL_COMMAND_STAGNATION ||

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] SHELL_COMMAND_STAGNATION is correctly added to isAlwaysOn here, but nonInteractiveCli.test.ts has no test verifying the user-facing message for this new type. Existing always-on types (CONSECUTIVE_IDENTICAL_TOOL_CALLS, TURN_TOOL_CALL_CAP) have analogous tests asserting that the "always-on guard" hint appears and the skipLoopDetection escape hatch does NOT. Without a parallel test, a future refactor could accidentally remove the new type from isAlwaysOn and no test would catch the regression.

Consider adding a test analogous to the existing always-on assertions:

it('shows the always-on hint for shell command stagnation loop type', async () => {
  // ... setup ...
  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'),
  );
});

— qwen3.7-max via Qwen Code /review

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.

I do not think this is a critical correctness issue for the current PR. The runtime behavior is already wired correctly: SHELL_COMMAND_STAGNATION is included in the always-on branch, so the headless message does not suggest model.skipLoopDetection as an escape hatch for this loop type.

This is a reasonable regression-test hardening suggestion, but it is not blocking for the fix. We can follow up with a small dedicated nonInteractiveCli.test.ts assertion that locks the user-facing message for SHELL_COMMAND_STAGNATION.

// stages, commits, runs another tool, or inspects file-specific diffs is
// making progress, so it must not share the stagnation bucket and trip a
// false halt. Failing open is the safe direction for an always-on guard.
const segments = command

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] isGitOverviewInspectionCommand requires every segment to match ^git..., so cd /dir && git status --short escapes the bucket entirely — the cd segment doesn't match the regex and every() returns false. This is the most common agent shell pattern (navigating to a package directory before inspecting). Consider stripping navigation prefixes before the every() check:

const NAVIGATION_PREFIXES = /^(?:cd|pushd|popd)\s+\S+\s*$/;
return segments.every((segment) =>
  NAVIGATION_PREFIXES.test(segment) || isGitInspectionSegment(segment)
);

Alternatively, if this is intentional (cd = progress), a one-line comment documenting the decision would help future maintainers.

— qwen3.7-max via Qwen Code /review

name: string;
args: object;
}): string | null {
if (toolCall.name !== 'run_shell_command') {

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] Two untested defensive code paths in the new guard:

  1. getShellInspectionKey's typeof command !== 'string' guard — no test exercises run_shell_command with missing, undefined, or non-string command args. If a future refactor changes this check, it could throw on malformed args instead of gracefully degrading.

  2. No test verifying that the shell inspection streak persists across Finished events — which it intentionally does (consistent with the consecutive-identical guard), but this behavior is untested. A future change adding streak reset to the Finished handler would silently break the guard for multi-round-trip inspection runs.

Both are quick additions that would protect against regressions.

— qwen3.7-max via Qwen Code /review

@@ -238,9 +251,10 @@ export class LoopDetectionService {

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] The JSDoc here was correctly updated to say "three guards", but two other inline comments still say "both": (1) the Retry branch comment ~10 lines below ("double-count against both always-on guards"), and (2) the comment above isAlwaysOn in nonInteractiveCli.ts:129 ("both run before the skipLoopDetection gate"). Consider updating all three locations in the same commit to keep them consistent.

— qwen3.7-max via Qwen Code /review

@yiliang114

Copy link
Copy Markdown
Collaborator Author

@qwen-code /triage

@yiliang114

Copy link
Copy Markdown
Collaborator Author

@qwen-code /triage

@yiliang114
yiliang114 enabled auto-merge June 28, 2026 12:56
@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Thanks for the PR!

Template looks good ✓

On direction: This is a real user-facing bug — #4695 describes DeepSeek collapsing into repeated git status/git diff overview loops that bypass the exact-match guard by varying command text. Loop detection is a core safety feature with ongoing investment in the CHANGELOG (#5234 per-turn cap, #5564 fail-on-loop, #5934 truncated-write loops). An always-on guard targeting this specific pattern is squarely in scope.

On approach: The scope is tight — 5 files, ~100 lines of logic, ~300 lines of tests. The guard intentionally fails open on every ambiguity: file-specific git diff -- src/file.ts and git diff src/file.ts are excluded, compound commands with write operations are excluded, and non-inspection tool calls reset the streak. The classification logic (every segment of a &&/;/| chain must be a git overview command) is the right default for an always-on guard — false negatives are recoverable, false positives halt user work. No concerns here.

Moving on to code review. 🔍

中文说明

感谢贡献!

模板完整 ✓

方向:这是一个真实的用户侧 bug——#4695 描述了 DeepSeek 在 git status/git diff 概览命令之间变换文本,绕过了完全相同调用保护。循环检测是核心安全特性,CHANGELOG 中有持续投入(#5234 per-turn 上限、#5564 loop 即失败、#5934 截断写入循环)。针对这个特定模式的 always-on 保护完全在范围内。

方案:范围紧凑——5 个文件,约 100 行逻辑,约 300 行测试。保护刻意在所有模糊场景 fail open:文件级 git diff -- src/file.tsgit diff src/file.ts 不入桶,带写操作的复合命令不入桶,非检查工具调用重置 streak。分类逻辑(&&/;/| 链的每一段都必须是 git 概览命令)对 always-on 保护来说是正确的默认值——漏报可以恢复,误报会中断用户工作。没有顾虑。

进入代码审查 🔍

Qwen Code · qwen3.7-max

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Code Review

Read the diff against my independent proposal (add an always-on streak counter for run_shell_command git overviews in LoopDetectionService). The PR's approach matches and exceeds it — the git command classification is more thorough than I'd have written, with proper handling of -- pathspec separators, git -C/--no-pager prefixes, revision tokens (HEAD~3, main..feat, SHA hashes), and compound shell chain segmentation.

No correctness bugs, no security issues, no regressions. The implementation follows project conventions — it extends the existing LoopDetectionService pattern (streak counter + threshold, same as action_stagnation and consecutive_identical_tool_calls), adds the new LoopType enum value in the right place, and wires the label into nonInteractiveCli.ts consistently. The ~300 lines of tests cover all the false-positive boundaries that matter for an always-on guard.

Reuse check: the classification logic is new but appropriately so — no existing utility in the repo classifies git commands by semantic intent. The streak-tracking pattern reuses the existing service architecture.

Tests

Unit tests (from worktree, PR branch checked out):

  • loopDetectionService.test.ts: 72 passed
  • nonInteractiveCli.test.ts: 57 passed, 1 skipped
  • npm run lint: passed
  • npm run typecheck: passed
  • npm run build && npm run bundle: passed

Guard Verification (built dist/cli.js)

Ran the LoopDetectionService directly from the compiled bundle against the exact command sequence from issue #4695:

=== Test 1: Overview git inspection (should halt at #8) ===
  [1] "git status --short" -> halted=false
  [2] "git status --short && git diff --stat" -> halted=false
  [3] "git diff --name-only HEAD" -> halted=false
  [4] "git status --porcelain=v1" -> halted=false
  [5] "git diff --stat HEAD" -> halted=false
  [6] "git -C . status --short" -> halted=false
  [7] "git --no-pager diff --stat" -> halted=false
  [8] "git ls-files --modified" -> halted=true type=shell_command_stagnation

=== Test 2: File-specific git diff (should NOT halt) ===
  9x file-specific diff -> any halted: false OK

=== Test 3: Streak reset on non-inspection call ===
  7 inspections + read_file + 7 more -> tripped: false OK

=== Test 4: Compound write commands (should NOT halt) ===
  10x add+status+commit -> any halted: false OK

All four scenarios behave correctly: the guard trips at exactly the 8th overview inspection, file-specific diffs are excluded, streaks reset on non-inspection calls, and compound write commands don't false-positive.

tmux Real-Scenario Test (bundled CLI)

Ran the bundled dist/cli.js against a test git repo with a normal "what files changed?" prompt to verify no false positives on legitimate git inspection:

$ node dist/cli.js -p 'What files changed in this repo? Show me the diff.' --max-session-turns 3 --yolo -o text

Warning: running headless with --yolo / approval-mode=yolo and no sandbox...
One tracked file changed, plus four untracked files:

**Modified:** `file1.ts`
```diff
-hello
+changed

Untracked (new): capture.txt, file3.ts, run.log, run2.log

No commits yet — the only commit is 099311a init. The change to file1.ts is unstaged.


CLI completed normally — the guard did not fire on a legitimate ~3 tool call git inspection sequence. No false positives.

<details>
<summary>中文说明</summary>

## 代码审查

将 diff 与我的独立方案(在 `LoopDetectionService` 中为 `run_shell_command` git 概览命令添加 always-on streak 计数器)对比。PR 的方案匹配并超越了我的设计——git 命令分类比我写的更完善,正确处理了 `--` pathspec 分隔符、`git -C`/`--no-pager` 前缀、revision token(`HEAD~3`、`main..feat`、SHA hash),以及复合 shell 链的分段处理。

没有正确性 bug、没有安全问题、没有回归。实现遵循项目规范——扩展现有的 `LoopDetectionService` 模式(streak 计数器 + 阈值,与 `action_stagnation` 和 `consecutive_identical_tool_calls` 相同),在正确位置添加新的 `LoopType` 枚举值,并在 `nonInteractiveCli.ts` 中一致地接入 label。约 300 行测试覆盖了 always-on 保护需要的所有误伤边界。

## 测试

- `loopDetectionService.test.ts`:**72 通过** ✅
- `nonInteractiveCli.test.ts`:**57 通过,1 跳过** ✅
- `npm run lint`:**通过** ✅
- `npm run typecheck`:**通过** ✅
- `npm run build && npm run bundle`:**通过** ✅

## 保护验证(编译后的 `dist/cli.js`)

直接从编译后的 bundle 运行 `LoopDetectionService`,使用 #4695 中的命令序列:

- 8 个概览 git 检查 → 第 8 个以 `shell_command_stagnation` 中止 ✅
- 9 个文件级 `git diff -- src/file.ts` → 不中止 ✅
- 7 个检查 + read_file + 7 个检查 → streak 重置,不中止 ✅
- 10 个 add+status+commit 复合命令 → 不中止 ✅

## tmux 真实场景测试(打包后的 CLI)

使用正常 "what files changed?" 提示词在测试 git 仓库上运行打包后的 CLI,验证合法 git 检查无误伤:CLI 正常完成,保护未在约 3 次工具调用的合法检查序列上触发。

</details>

— *Qwen Code · qwen3.7-max*

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Stepping back: this PR does exactly what a good loop-detection fix should — it identifies a specific, observed failure mode (models varying git overview command text to evade the exact-match guard), adds the narrowest always-on guard that catches it, and exhaustively tests the false-positive boundaries.

The implementation quality is high. The isGitOverviewInspectionCommand classifier is the standout piece — it segments compound shell chains and requires every segment to be a git overview command, which means a git add && git status && git commit chain is correctly excluded. The isOverviewGitDiff method handles the tricky case of git diff src/file.ts (no -- separator) by treating non-flag, non-revision positional args as file paths. This is the right fail-open direction for an always-on guard.

The diff is minimal — every change serves the stated goal, no drive-by refactors or scope creep. The ~300 lines of new tests are justified for an always-on circuit breaker that affects every run.

Verified end-to-end: the guard trips at command 8 on the exact sequence from #4695, doesn't false-positive on file-specific diffs or compound write commands, and the bundled CLI operates normally on legitimate git inspection workflows.

Approving. ✅

中文说明

退一步看:这个 PR 做了一个好的循环检测修复应该做的事——识别一个具体的、已观察到的失败模式(模型变换 git 概览命令文本以绕过完全相同调用保护),添加了能捕获它的最窄 always-on 保护,并详尽测试了误伤边界。

实现质量高。isGitOverviewInspectionCommand 分类器是亮点——它分割复合 shell 链并要求每一段都是 git 概览命令,这意味着 git add && git status && git commit 链被正确排除。isOverviewGitDiff 方法处理了 git diff src/file.ts(无 -- 分隔符)的棘手情况,将非 flag、非 revision 的位置参数视为文件路径。对于 always-on 保护来说,这是正确的 fail-open 方向。

Diff 最小化——每一处改动都服务于目标,没有顺手重构或范围蔓延。~300 行新测试对于影响每次运行的 always-on 断路器来说是合理的。

端到端验证通过:保护在 #4695 的命令序列的第 8 个命令处触发,对文件级 diff 或复合写入命令无误伤,打包后的 CLI 在合法 git 检查工作流上正常运行。

批准合并。✅

Qwen Code · qwen3.7-max

@qwen-code-ci-bot qwen-code-ci-bot left a comment

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.

LGTM, looks ready to ship. ✅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Tool-call loop: deepseek-v4-pro collapses into repeated identical tool_call inside its working context window, no client-side circuit breaker

3 participants