[auto-review] Reviewer technical failures return a terminal deny (no human fallback) — one non-compliant route blocks every tool call (0.2.1-rc.3, 55 events / 15 sessions)

[xunyyds]

Environment

• app: @deepseek-ai/dsh-desktop 0.2.1-rc.3, win32 x64
• plugin: @deepseek-ai/dsh-experimental-auto-review 0.2.1-rc.3 (bundled with the app; mounted via dsh.profile.bundles, confirmed active by persisted permission/preset {"preset":"auto"} events)
• approval policy: ask (the Auto preset's own value)
• evidence: 238 local session logs under ~/.dsh/sessions (session.v4.jsonl.zstd), 2026-10-16 → 10-25. A "genuine failure" = a tool/result whose text starts with Error: Auto review of tool … failed, which excludes sessions that merely quote the error while inspecting it
• related but distinct report: auto-review 的模型调用未传 sessionId，导致每次工具审查的费用无法归属到任何会话 #10381 (reviewer calls carry no sessionId, so review spend is unattributable) — same package, different defect

Summary

Auto review fails closed, which is right. But three of its failure paths are indistinguishable from a security denial from the user's point of view, and one of them is strictly harsher than a real denial:

1. A reviewer transport / timeout / quota / abort failure returns {kind:"deny"} before await next(), so the human approval flow is never offered. A model denial asks the user; a reviewer outage silently kills the call.
2. A reviewer that returns prose or tool-call markup instead of the two-member JSON object is treated as a fatal serialization error, with no fence stripping, no extraction, no retry, no format repair.
3. Because the reviewer reuses the session's own latest request/header route, one non-compliant route converts Auto mode into a total lockout — every tool call, including empty-argument read-only ones, is refused before its body runs.

Net effect: turning Auto on with such a route makes the session unusable, and nothing in the message tells the user how to recover.

Actual data (2026-10-16 → 10-25, rc.3)

55 genuine failure events across 15 sessions:

class	count	shape
A1. reviewer emitted tool-call markup	23	<tool_call ×10, DeepSeek ＜｜｜DSML｜｜ sentinel ×13
A2. reviewer emitted prose	13	CJK prose ×8 (Unexpected token '根', "根据之前的侦察，我已"...), Latin prose ×4 ("I'll start", "Let me che"), fenced ```toolCal ×1
B. reviewer request itself failed	15	TRANSPORT: Connection error ×4, QUOTA_EXCEEDED ×3 (HTTP 402 / provider rate-limit), aborted UNKNOWN: [object Object] ×3, sse transport error: terminated ×2, TIMEOUT ×1, other ×2
C. pending-tool schema lookup failed	4	auto-review: the pending native tool schema is missing or ambiguous

Per route (route = latest request/header before the refused call) — the discriminating fact:

route	A1	A2	B	C
qodercn/qfmodel	9	11	1	—
qodercn/dfmodel	14	1	—	—
fuck-gpt/gpt-6.1-sol	—	—	5	—
buddy/deepseek-v4.1-flash	—	—	1	4
minimax/MiniMax-M3.1-Flash(-Preview)	—	1	3	—
trae/deepseek-v4.1-flash	—	—	2	—
codearts / cline / raccoon (1 each)	—	—	3	—

All but one of the 36 protocol violations (A-class) come from the two qodercn routes on the same Qoder-shaped gateway; the other six routes produced zero A-class events, their failures being all transport/quota. So this is simultaneously a model-compliance problem and a harness robustness problem — and the harness is the part that turns a per-route 100% failure rate into a per-session 100% lockout.

Worst streaks, same session, minutes apart (UTC):

2026-10-21 15:25:36  mcp__windows__PowerShell  Unexpected token '根', "根据之前的侦察，我已"...
2026-10-21 15:28:04  mcp__windows__PowerShell  Unexpected token '<', "<tool_call"...
2026-10-21 15:28:57  pwsh                      Unexpected token '<', "<tool_call"...
2026-10-21 15:29:16  working_directory {}      Unexpected token '<', "<tool_call"...
2026-10-21 15:29:29  list_agents {}            Unexpected token '<', "<tool_call> "...

2026-10-25 07:11:51  /permission auto (risk dialog confirmed)
2026-10-25 07:11:57  pwsh            Unexpected token '<', "<｜｜DSML｜｜ "...
2026-10-25 07:11:59  list_agents {}  Unexpected token '<', "<｜｜DSML｜｜ "...
                     (24 s, 5 calls, all refused)
2026-10-25 07:12:15  /permission danger-full-access   (user escape)

working_directory {} and list_agents {} were called with empty arguments. Nothing about them can be a risk-policy judgement; the review step itself is what broke.

Note on the DSML shape: on qodercn/dfmodel the reviewer's text block literally begins with the DeepSeek sentinel <｜｜DSML｜｜ (U+FF5C doubled on both sides) — a third markup shape besides <tool_call> and prose. The review request declares no tools, so this is the model's chat template leaking its internal tool-call sentinel into content, not a real call.

Where in the code (0.2.1-rc.3 shipped bundle, packages/experimental/auto-review)

Line numbers refer to the unminified lib/index.js shipped in the desktop app.asar. The five defects below are unchanged from 0.2.1-rc.2 — the rc.2 → rc.3 bump fixed none of them.

1. Failure precedes delegation, so no human ever gets asked. In the prepended tools/pre-execute listener:

if (!review.ok) return failed(exec, review.error);   // kind: "deny" — returns BEFORE await next()
...
if (decision.decision === "deny" && ctx.approval.overrideOf(agent.session) === "never") return denied(...);
const downstream = await next();                     // human approval path only reached on a real decision

A reviewer denial can become askUser(...); a reviewer failure cannot. This asymmetry contradicts the README's framing ("被拒绝调用会请求用户审批") for the most common failure class (B, n=15).

2. Zero tolerance in the decision parser.

function parseDecision(text) { const value = JSON.parse(text); ... }

Raw JSON.parse on the reviewer's text: no ```json fence stripping, no first-balanced-{…} extraction, no single format-correction retry. readDecision additionally requires the last block be text and every preceding block be reasoning (lib/index.js:394), and rejects repeated members via topLevelMemberCount (:357). Each extra clause is another way for an otherwise-fine route to lose.

3. The reviewer route is the agent route. classifyRisk builds options from snapshot.provider / snapshot.model, i.e. the latest request/header config (:401), and the package exposes no reviewer-model override (the bundle exports only apply / inject / name; its cordis.patch.yml carries no config; searched the shipped bundle for any reviewer-model/auto-review config key: no match). Selecting Auto therefore couples the safety layer's reliability to the agent's chat-quality choice, with no way to pin a cheap strict JSON-following reviewer.

4. Class C is a pure harness bug, model-independent.

if (candidate === void 0 || schemas.length !== 1)
  throw new Error("auto-review: the pending native tool schema is missing or ambiguous");

Requiring exactly one schema match by name in the latest header breaks when the tool set churns: 3 consecutive pwsh refusals plus mcp__mobile__mobile_force_stop in one session (2026-10-20, buddy/deepseek-v4.1-flash) — i.e. MCP reconnect / tool-set drift invalidates review for tools that are plainly present and unambiguous.

5. Diagnostics stringify to [object Object] (3 of 15 class-B events):

const { code, message } = chunk.reason.failure;
throw new Error(`auto-review: reviewer ended with ${chunk.reason.kind} ${code}: ${message}`);

When failure.message is an object, the user-visible reason is aborted UNKNOWN: [object Object] — root cause unrecoverable from any log or card.

Expected / desired behaviour

1. Distinguish "reviewer unavailable" from "reviewer denied". Technical failures (transport, timeout, quota, abort, unparseable output) should escalate to the existing human ask path under the ask policy — the safest available fallback and the one users already understand — with the technical reason surfaced. Reserve hard failure for the never (in-process child) policy.
2. Make the parser forgiving before it rejects: strip code fences; extract the first balanced top-level {…}; tolerate leading/trailing prose; allow one bounded format-repair retry (temperature 0, "output only the JSON object"); accept the call only when the parsed object matches the closed protocol. Strictness belongs on the protocol, not on the byte position of the braces.
3. Decouple the reviewer route. Add config: reviewer provider/model (defaulting to the session route for compatibility), plus max retries and per-review timeout. A safety gate must be configurable independently of the agent's convenience model.
4. Fail open to human review on schema-lookup misses (class C), or resolve the pending schema from the live tool registry with the header only as a cache. schemas.length !== 1 should never be a terminal denial for a tool the harness is about to execute.
5. Normalize failure.message to a string and give reviewer failures a distinct structured error name/code (parallel to AUTO_REVIEW_DENIED), so they are greppable in session logs and distinguishable in the UI.
6. Ergonomics: after N consecutive reviewer failures (not denials) in one session, surface a one-line hint that Auto may be unavailable for this route and how to switch; today the model sees only Error: Auto review of tool "…" failed; its body was not executed: <JSON parse noise> and reliably misdiagnoses it as its own serialization bug (observed in at least three sessions on this machine).

Repro

1. Web/desktop profile with @deepseek-ai/dsh-experimental-auto-review mounted; session on a route that does not obey the bare-JSON contract (observed: qodercn/qfmodel, qodercn/dfmodel).
2. /permission auto (confirm the risk dialog).
3. Call any tool — including an argument-free read-only one such as list_agents or working_directory.
4. Every call is refused before its body with Auto review of tool "…" failed; its body was not executed: Unexpected token …, is not valid JSON, repeatedly, with no approval prompt and no self-healing.

Workaround in use (no code change)

Switch the session to /permission danger-full-access or workspace-write to bypass the reviewer; or move the agent route to one with zero A-class events (buddy/deepseek-v4.1-flash — though it still hits class C). This is a mitigation, not a fix: reliability of the safety layer currently depends on which chat model the user happened to pick.

Severity

auto-review (experimental), bug, reliability. Severity high: total session lockout, silent (no human override offered), and self-masking error text.

---

中文摘要（Chinese summary）

版本 @deepseek-ai/dsh-experimental-auto-review 0.2.1-rc.3 / dsh-desktop 0.2.1-rc.3 / win32 x64。

数据 本机 238 个会话日志、2026-10-16→10-25，真·审查故障 55 次 / 15 个会话（剔除仅引用错误文本的自我排查会话）：审查器输出工具调用标记 23 次（<tool_call> 10 + DeepSeek <｜｜DSML｜｜ 13）；输出散文 13 次（中文 8、英文 4、围栏标记 1）；审查请求本身失败 15 次（连接错误 4、配额 402/限流 3、aborted UNKNOWN: [object Object] 3、SSE 中断 2、超时 1、其他 2）；待审 schema 查找失败 4 次。

关键事实 36 次协议违规（A 类）里 35 次集中在 qoder 网关两条路由（qodercn/qfmodel 20 次、qodercn/dfmodel 15 次），其余路由 0 次格式违规。因此这既是模型合规问题，更是 harness 鲁棒性问题：是后者把「某条路由 100% 不合规」放大成了「整个会话 100% 不可用」。最严重的一段：/permission auto 后 24 秒内 5 次调用全被拦，其中 list_agents {} 是无参数只读调用——风险策略判断不可能涉及它，坏掉的是审查环节本身。

五个可定位缺陷（rc.2 → rc.3 原样保留，一个没修）

1. if (!review.ok) return failed(...) 发生在 await next() 之前：审查器技术故障返回终结性 deny，人工审批路径根本不触发。真·拒绝会 askUser，审查器故障反而更严厉，与 README「被拒绝调用会请求用户审批」矛盾（且这是占比最高的一类，n=15）。
2. parseDecision 直接 JSON.parse 原文：不剥 ```json 围栏、不抽取首个平衡 {}、不容许任何前后缀、不做一次格式纠正重试；`readDecision` 还要求「末块必须是 text、其余必须全是 reasoning」并用 `topLevelMemberCount` 拒绝重复成员。严格性应当作用在协议语义上，而不是花括号的位置上。
3. 审查器强制复用会话自身的 request/header 路由，包内不提供任何 reviewer-model 覆写（bundle 只导出 apply/inject/name，patch 无 config）：安全闸的可靠性被绑死在用户的聊天模型选择上。
4. schemas.length !== 1 即抛 pending native tool schema is missing or ambiguous——纯本地 bug，与模型无关（MCP 重连/工具集漂移即可触发，实测 pwsh 连续 3 次 + mcp__mobile__mobile_force_stop）。
5. failure.message 为对象时输出 [object Object]，15 次技术失败里 3 次，根因彻底丢失。

另：审查请求未声明任何 tools，qodercn/dfmodel 上模型把 DeepSeek 内部工具调用哨兵 <｜｜DSML｜｜（双全角竖线）当正文漏出，属 chat-template 泄漏而非真实调用。

期望：区分「审查器不可用」与「审查器拒绝」，前者在 ask 策略下升级给人工；解析加容错 + 有界重试；reviewer 路由/超时/重试次数可独立配置；schema 未命中改为回落到人工或从实时工具注册表解析；技术失败具备独立结构化错误标识并规范化 message；连续 N 次审查器故障时给一行「此路由可能不支持 Auto」的提示——当前模型只会看到 Unexpected token … is not valid JSON，并多次会话把它误判成自己的序列化 bug。

临时缓解（未改代码）：切 /permission danger-full-access 或 workspace-write 绕开审查器，或改用 0 次格式违规的路由（如 buddy/deepseek-v4.1-flash，但仍会撞上第 4 点）。

[PerryLink]

你说的这个行为在代码里是显式的，而且函数名就叫 failed，跟拒绝是分开的两条路——只是最后给了同一个 deny。

packages/experimental/auto-review/src/index.ts：

/** Materialize a reviewer failure as its own error rather than a denial. */
function failed(exec: ToolExecution, error: unknown): PreToolDecision {
  const message = error instanceof Error ? error.message : String(error)
  return {
    kind: 'deny',
    reason: `Auto review of tool "${exec.name}" failed; its body was not executed: ${message}`,
    info: { name: AUTO_REVIEW_FAILED_ERROR_NAME, code: AUTO_REVIEW_FAILED_CODE },
  }
}

注释写的是"as its own error rather than a denial"，但 kind 仍然是 'deny'，所以从权限层的角度看，评审器崩了和被评审为危险是同一个结果。区别只留在 info 里（AUTO_REVIEW_FAILED_ERROR_NAME），需要读事件才能分辨。

旁边就有现成的第三条路——askUser()：kind: 'ask'，把决定交回给人。所以修法不是新造语义，而是让 failed() 返回 askUser(exec, ...) 或者按配置二选一：技术上失败时，把这一条降级成需要人确认，而不是直接拒。

我倾向后者也做成可配，因为有些部署希望"评审不可用就等于拒绝"（fail-closed），这跟"评审不可用就问我"是策略差异，不该写死。默认值可以按你说的取 ask，因为它只影响单次调用，不会放宽任何边界。

顺带说一句你统计的口径：Error: Auto review of tool … failed 这个前缀就是 :713 那段拼出来的 reason，所以用它筛"真实失败"是对的，不会跟评审拒绝混起来（拒绝的文案是 Auto review rejected tool …）。