Skip to content

fix(core): allow manual retry after auto mode blocks - #11025

Merged
qqqys merged 3 commits into
QwenLM:mainfrom
qqqys:fix/issue-11019-auto-denial-manual-retry
Sep 5, 2026
Merged

fix(core): allow manual retry after auto mode blocks#11025
qqqys merged 3 commits into
QwenLM:mainfrom
qqqys:fix/issue-11019-auto-denial-manual-retry

Conversation

@qqqys

@qqqys qqqys commented Sep 4, 2026

Copy link
Copy Markdown
Collaborator

What this PR does

AUTO mode now offers a one-time manual review path after a classifier policy block. Retrying the same tool call with unchanged arguments in the same working directory bypasses only the classifier and opens the existing confirmation UI; a changed tool, argument, or working directory is classified normally. Policy-block recovery offers only allow-once or cancel and keeps the session in AUTO, while classifier-unavailable recovery retains the existing recommendation to switch to Default. The call that reaches the consecutive-block or session-denial threshold also enters manual review immediately instead of requiring one extra attempt. Core scheduling, pending-call re-evaluation, and ACP sessions share the same state transition.

The denial result now tells the model how to request manual review without suggesting equivalent-path workarounds. Deterministic destructive-command guards remain authoritative and still run before any recovery fallback.

Why it's needed

Classifier false positives in AUTO mode were effectively unoverridable: the model was told not to retry, while no user confirmation was exposed. Denial thresholds also had an off-by-one behavior, so the third consecutive block and twentieth session denial still failed before a later call could fall back.

Reviewer Test Plan

How to verify

Trigger an AUTO classifier policy block, then repeat the same tool call. Confirm that the first call is denied, the exact retry opens a one-time manual confirmation without another classifier request, offers no mode-switch or persistent-approval choice, and executes only after approval. Change an argument or the working directory and confirm that classification runs again. Starting from two consecutive blocks or nineteen total denials, confirm that the next classifier block opens manual confirmation on that current call. Repeat the checks through an ACP session and through pending-call re-evaluation. Confirm that classifier-unavailable recovery still offers the recommended Default-mode switch, and that classifier-unavailable and unrelated safety fallbacks do not emit a policy-denial hook while a threshold reached by an actual policy block does.

Evidence (Before & After)

Before: an exact retry was classified and blocked again; the third consecutive block and twentieth total denial were denied, with fallback delayed until a later call.

After: only an exact tool/arguments/working-directory match receives one manual review opportunity, and threshold-reaching calls enter that review immediately. All executions remain gated on explicit approval.

Tested on

OS Status
🍏 macOS
🪟 Windows ⚠️
🐧 Linux ⚠️

Environment (optional)

Node.js 24.14.1. Verified with the full affected core and ACP test files, permission-option and TUI regression tests, repository build, lint, and typecheck. Tool execution and classifier responses were mocked; no destructive command was run.

Risk & Scope

Linked Issues

Partially addresses #11019 (classifier-block recovery and denial-threshold behavior only).

中文说明

本 PR 做了什么

AUTO 模式现在会在分类器策略拦截后提供一次人工复核路径。在相同工作目录中,以完全不变的参数重试同一工具调用时,只跳过分类器并打开现有确认界面;工具、参数或工作目录任一变化都会正常重新分类。策略拦截后的恢复只提供允许一次或取消,并保持 AUTO 模式;分类器不可用时则保留现有的切换到 Default 建议。达到连续拦截阈值或会话拒绝总量阈值的当前调用,也会立即进入人工复核,不再需要额外再调用一次。核心调度、等待调用重新评估和 ACP 会话共用同一套状态迁移。

拒绝结果现在会告诉模型如何请求人工复核,同时仍禁止通过等价路径绕过。确定性的破坏性命令保护仍具有最高优先级,并继续在任何恢复 fallback 之前执行。

为什么需要

AUTO 模式下的分类器误判此前实际上无法覆盖:模型被告知不要重试,但用户又看不到确认入口。拒绝阈值还存在 off-by-one 行为,第 3 次连续拦截和会话第 20 次拒绝仍会失败,只有之后的调用才能 fallback。

Reviewer Test Plan

如何验证

触发一次 AUTO 分类器策略拦截,然后重复相同工具调用。确认首次调用被拒绝,精确重试在不再次请求分类器的情况下打开一次性人工确认,不提供模式切换或永久批准选项,并且只有批准后才会执行。修改参数或工作目录,确认会重新分类。从已有 2 次连续拦截或 19 次总拒绝开始,确认下一次分类器拦截会在当前调用直接打开人工确认。通过 ACP 会话和等待调用重新评估分别重复检查。确认分类器不可用时仍提供推荐的 Default 模式切换选项;分类器不可用及无关的安全 fallback 不会触发策略拒绝 hook,而真实策略拦截达到阈值时仍会触发。

证据(修复前后)

修复前:精确重试仍会重新分类并再次被拦;第 3 次连续拦截和第 20 次总拒绝仍被拒绝,fallback 推迟到后续调用。

修复后:只有工具、参数和工作目录完全匹配的调用获得一次人工复核机会,达到阈值的当前调用也会立即进入复核。所有执行仍必须经过明确批准。

测试平台

OS 状态
🍏 macOS
🪟 Windows ⚠️
🐧 Linux ⚠️

环境(可选)

Node.js 24.14.1。已运行受影响 core 与 ACP 测试文件的完整用例、权限选项与 TUI 回归测试、仓库构建、lint 和 typecheck。工具执行与分类器响应均为 mock,未运行任何破坏性命令。

风险与范围

关联 Issue

部分处理 #11019(仅分类器拦截恢复与拒绝阈值行为)。

@qqqys

qqqys commented Sep 4, 2026

Copy link
Copy Markdown
Collaborator Author

E2E test report

Baseline

  • In AUTO mode, submit a classifier-eligible shell call that the classifier blocks.
  • Repeat the same tool name and arguments.
  • Expected on the baseline: the repeat is classified and blocked again instead of reaching manual approval.
  • Seed two consecutive blocks or nineteen total denials, then submit one more classifier-blocked call.
  • Expected on the baseline: the current call is denied; only a later call sees the fallback state.

Fixed behavior

  • The first policy block remains denied and includes exact-retry guidance.
  • Repeating the same tool call in the same working directory skips only the classifier and opens a one-time manual confirmation.
  • Changing an argument or the working directory runs the classifier again.
  • The call that reaches the consecutive or total denial threshold opens manual confirmation immediately.
  • Approving the prompt executes once and resets the recovery streak; cancelling never executes.
  • Core scheduling, pending-call re-evaluation, and ACP daemon flows behave the same way.

Evidence

  • Deterministic core state-machine tests cover exact matching, one-shot consumption, argument/cwd mismatch, and threshold boundaries.
  • Scheduler tests cover full blocked -> changed -> exact retry -> manual approval execution and pending-call re-evaluation.
  • ACP tests cover classifier bypass on exact retry and current-call threshold fallback.
  • No live destructive command is used; shell invocations are mocked and execution remains behind manual confirmation.

Results

  • Core permission and scheduler regression files: 525/525 passed.
  • Full ACP Session regression file: 813/813 passed.
  • Permission-option and TUI regressions after final UX review: 55/55 passed; exact-retry and classifier-unavailable ACP comparison: 2/2 passed.
  • Repository build, typecheck, changed-file lint, formatting, and diff checks passed on macOS with Node.js 24.14.1.

@qwen-code-ci-bot

qwen-code-ci-bot commented Sep 4, 2026

Copy link
Copy Markdown
Collaborator

Qwen Triage finishedview run. See the stage comments in this thread for the result.

Qwen Triage 已完成 —— 查看运行。结果见本线程中的各阶段评论。

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Thanks for the PR — this one is well scoped and the motivation is solid.

Template looks good ✓

Problem: observed, not theoretical. #11019 is a real production incident (three affirmative ask_user_question answers, three classifier_blocked refusals, ~10 minutes and zero statements executed), and the reporter traced it to file:line against main. The triage pass on that issue independently confirmed the whole chain at 80497a74d0e8: a block creates no permission request so there is nothing for a user to answer, the escape hatch arms a future call rather than the blocked one, and the denial guidance forbids the very retry that would arm it. So the loop is real and closed.

Direction: aligned, and specifically endorsed. The issue's own triage called out "a smaller half of Defect 1 worth separating" — letting a block arm the manual-approval dialog directly instead of requiring a fourth attempt — as the piece that breaks the loop "with a much smaller safety delta than any projection change" and "can move faster". This PR is exactly that half, and it correctly leaves the projection change (the trusted-source allowlist) and Defect 2 (session-rebuild approval mode) out. CHANGELOG: no direct reference to a retry-after-block mechanism, but the area is clearly relevant — the reference agent ships one-time prompts inside auto mode ("Added a one-time prompt in auto mode before the first file read outside the working directories"), so auto mode is not a hard no-prompt mode and a one-shot confirmation here is consistent with where the ecosystem landed.

Size: core paths are touched (packages/core/src/permissions/**, coreToolScheduler.ts, plus cross-package into packages/cli). Breakdown: 370 production lines, 697 test lines, 0 generated/schema. Under the 500-line maintainer-awareness threshold and under the 1000-line advisory, and it is a fix rather than a refactor, so no size gate applies. Noting for the record that the author has write access and is a listed area owner, so this is maintainer-authored.

Approach: the scope feels right, and I could not find a materially simpler path. Both halves are load-bearing — fixing the off-by-one alone still leaves a first block with no user-visible recovery, and the retry token alone still leaves thresholds needing a fourth attempt. Two things I particularly like: the fingerprint reuses the existing getToolCallRepeatKey sha256 helper (canonicalized key order, legacy-alias aware) rather than inventing a parallel digest, and decorateClassifierUnavailableConfirmation is kept as a thin wrapper so no out-of-diff caller breaks. Two-thirds of the diff is tests.

One question worth answering before merge, not a blocker: passing the real reason through the decorator also changes external-write fallbacks. Previously those were decorated with a hardcoded reason: 'classifier_unavailable', which made both the TUI and the ACP option list offer "Switch to Default Mode and allow once (recommended)". After this change they no longer do. That looks like the correct consequence of labelling the reason accurately — the switch wording only makes sense when the classifier is genuinely unavailable — but it is a user-visible behaviour change that the description does not mention. Intentional?

Risk: Stage 1e matched a high-risk path — packages/cli/src/acp-integration/** (Session.ts, permissionUtils.ts), which is the area correlated with post-merge reverts in this repo. That does not block anything, but it means full review depth and CI evidence before approval, and I have checked the ACP read sites outside the diff (filterAlwaysAllowOptions, buildPermissionRequestContent) — both still behave correctly for the new reasons. The change also sits on a deliberate security boundary hardened by #4538, so I have read the destructive-command ordering closely; findings in the next comment.

Moving on to code review. 🔍

中文说明

感谢贡献 —— 这个 PR 范围清晰,动机扎实。

模板完整 ✓

问题:已观测到的真实缺陷,不是理论加固。#11019 是一次真实的生产事故(用户三次通过 ask_user_question 明确同意,三次仍被 classifier_blocked 拒绝,约 10 分钟内零条语句执行),报告者已对照 main 定位到具体文件与行号。该 issue 的 triage 也在 80497a74d0e8 上独立确认了整条链路:block 不产生权限请求,用户无从应答;逃生通道只对后续调用生效,救不了当前这次;而拒绝指引又恰好禁止了那个能启动逃生通道的重试。所以这个闭环是真实存在的。

方向:一致,而且是被明确建议过的方向。该 issue 的 triage 专门指出"缺陷 1 中值得单独拆出的较小一半"——让 block 直接启动人工确认弹窗,而不是要求第四次尝试——认为它"能以远小于投影改动的安全代价"打破闭环,且"可以更快推进"。本 PR 正是这一半,并正确地把投影改动(可信来源白名单)和缺陷 2(会话重建后审批模式)排除在外。CHANGELOG:没有与"拦截后重试"机制直接对应的条目,但该区域明显相关——参考实现同样在 auto mode 内部提供一次性确认("Added a one-time prompt in auto mode before the first file read outside the working directories"),说明 auto mode 并非绝对无弹窗模式,这里的一次性人工复核与生态走向一致。

规模:触及核心路径(packages/core/src/permissions/**coreToolScheduler.ts,并跨包到 packages/cli)。明细:生产代码 370 行,测试 697 行,生成/schema 0 行。低于 500 行的维护者关注阈值,也低于 1000 行的大 PR 建议阈值;类型是 fix 而非 refactor,因此不触发任何规模门禁。同时记录:作者具有 write 权限且是登记的领域 owner,属于维护者提交的 PR。

方案:范围合理,我没有找到明显更简的路径。两半改动都是必需的——只修 off-by-one,首次拦截仍然没有用户可见的恢复入口;只加重试标记,达到阈值时仍需要第四次尝试。有两点做得好:指纹复用了已有的 getToolCallRepeatKey(sha256,键序规范化、兼容旧别名)而不是另写一套摘要;decorateClassifierUnavailableConfirmation 保留为薄封装,diff 之外的调用方不会受影响。整个 diff 有三分之二是测试。

一个建议在合并前回答的问题(非阻塞):把真实的 reason 传入 decorator,同时改变了 external-write 的回退行为。此前它被硬编码为 reason: 'classifier_unavailable',因此 TUI 和 ACP 选项列表都会提供"Switch to Default Mode and allow once (recommended)";改动后不再提供。这看起来是"如实标注 reason"的正确结果——切换模式的措辞只在分类器确实不可用时才成立——但它是一处用户可见的行为变化,PR 描述里没有提到。是有意的吗?

风险:Stage 1e 命中高风险路径 —— packages/cli/src/acp-integration/**Session.tspermissionUtils.ts),即本仓库中与合并后 revert 相关的区域。这不构成阻塞,但意味着需要完整深度的 review 和 CI 证据才能批准。我已核对 diff 之外的 ACP 读取点(filterAlwaysAllowOptionsbuildPermissionRequestContent),二者对新增 reason 的行为仍然正确。该改动还位于 #4538 加固过的安全边界上,因此我重点核对了破坏性命令的执行顺序;结论见下一条评论。

进入代码审查 🔍

Qwen Code · qwen3.8-max-2026-09-02

Reviewed at b7bc46de118a5d37b9c503bc418d8dd6acae4158 · re-run with @qwen-code /triage

@qwen-code-ci-bot

qwen-code-ci-bot commented Sep 4, 2026

Copy link
Copy Markdown
Collaborator

Code review

I wrote down my own approach before opening the diff: re-check shouldFallback after recordBlock inside applyAutoModeDecision to fix the off-by-one, and for the false-positive half, a single-use token bound to a digest of (tool, args, cwd) that only ever selects the existing confirmation path — never grants execution — with the dialog reduced to allow-once/cancel, plus a change to the denial guidance so the model is actually told the retry exists. The PR landed on the same design, including the part I would most have expected to be missed: the PermissionDenied hook.

One finding worth fixing before merge.

AUTO_MODE_DENIAL_GUIDANCE is appended in two places, and the new sentence is only true in one of them. It ships with formatClassifierBlockMessage (where recordBlock(denialState, actionFingerprint) arms the token, so "retry the same tool call" really does open a dialog) and it also ships with the blocked:destructive-command branch — which still calls recordBlock(denialState) with no fingerprint and returns {kind:'blocked'} unconditionally. So when the deterministic destructive-command guard fires, the model is now told "To request manual approval for this exact action, retry the same tool call without changing its arguments" and that retry can never produce a manual review. It is worse than a no-op: the old wording ("stop and ask the user for explicit approval") was replaced, so on that path the model loses an actionable instruction and gains a false one, on the most dangerous category of command. I confirmed the safety property still holds — L5.2.5 runs before the skipClassifierReason early return in evaluateAutoMode, so a destructive command stays hard-blocked even with an armed token or after thresholds accumulate, and no dialog is ever offered. This fails closed; it is a model-guidance defect that invites a retry loop and burns turns, not a bypass. Cheapest fix is to split the constant so the destructive branch keeps a variant without the retry sentence — arming the token there would be exactly wrong.

Verified correct (naming these so you know what was actually checked, not just what passed):

  • The destructive-command ordering. evaluateAutoMode runs L5.1/L5.2 fast-paths, then L5.2.5 isDestructiveCommand, then pmForcedAsk, then external_write, and only then honours skipClassifierReason. A recovery fallback therefore cannot launder a deterministic block into a confirmable dialog. The PR's claim here is accurate against the code.
  • The token cannot grant execution. prepareAutoModeFallback consumes it synchronously via consumePendingManualRetry before the classifier is skipped, and it only selects {via:'fallback'} — the call still lands in the existing awaiting-approval flow and needs an explicit approve outcome. Aborting after consumption burns the one shot, but the next block re-arms it, so it self-corrects rather than locking out.
  • Digest properties. getToolCallRepeatKey sha256s a canonicalized {cwd, toolParams} with sorted object keys, preserved array order, null-prototype handling for __proto__, and canonicalToolName alias resolution. Field reordering cannot evade a match, a legacy alias cannot launder one, and a changed cwd or argument produces a different digest so the call is classified normally. Reusing this helper instead of writing a new digest was the right call.
  • Audit trail preserved. shouldFirePermissionDeniedForAutoMode now accepts a fallback outcome whose reason is consecutive_block or total_denial, and adds !decision.unavailable, so a threshold reached by a genuine policy block still emits PermissionDenied as classifier_blocked while classifier-unavailable and safety_check/ask_rule fallbacks do not. Without that change, converting the threshold block into a fallback would have silently dropped the hook.
  • Recovery dialog really is allow-once/cancel. hideAlwaysAllow: true from the decorator, filterAlwaysAllowOptions gating the switch option on reason === 'classifier_unavailable', and offersSwitchToDefault in the TUI all agree; the session stays in AUTO. The classifier-unavailable path keeps its "Switching to Default Mode is recommended" wording, and decorateClassifierUnavailableConfirmation survives as a wrapper so no out-of-diff caller breaks.
  • No TUI layout regression. I checked the compact-mode height math specifically, since that is where this kind of change usually breaks. The new 2 + (offersSwitchToDefault ? 1 : 0) + (hidesAlwaysAllow ? 0 : 1) matches the compact renderedOptions construction exactly, and is strictly more accurate than the hardcoded 3 it replaces; the non-compact options.length + (offersSwitchToDefault ? 1 : 0) still matches the insertion site below it.
  • Consumer sweep outside the diff. All three autoModeFallback read sites behave correctly for the new reasons: filterAlwaysAllowOptions hides always-allow for any fallback, and buildPermissionRequestContent forwards the message to ACP clients. recordAllow/recordBlock/shouldFallback gained optional parameters, so existing callers such as the subagent path still compile. And createApprovalModeOverride gives a child config a fresh createDenialState(), so pendingManualRetryFingerprint does not leak into subagents.
  • All three runtime paths moved together — core scheduling, pending-call re-evaluation, and the ACP Session — which is the usual failure mode for this area and it was avoided.

One unmentioned behaviour change (question, not a blocker): labelling the decorator reason accurately means external_write fallbacks are no longer decorated as classifier_unavailable, so they stop offering "Switch to Default Mode and allow once (recommended)" in both the TUI and the ACP option list. That reads as a genuine latent-bug fix — the switch wording only makes sense when the classifier is unavailable — but it is user-visible and absent from the description, and there is no test pinning the new external-write option set.

sequenceDiagram
    participant P1 as Model
    participant P2 as CoreToolScheduler
    participant P3 as evaluateAutoMode
    participant P4 as denialTracking state
    participant P5 as Confirmation UI
    participant P6 as User

    P1->>P2: tool call A
    P2->>P4: prepareAutoModeFallback, digest A
    P4-->>P2: no armed retry
    P2->>P3: evaluateAutoMode
    P3-->>P2: classifier policy block
    P2->>P4: recordBlock with digest A
    Note over P4: token A armed, streak below limit
    P2-->>P1: denied, with exact retry guidance
    P1->>P2: retry identical call A
    P2->>P4: prepareAutoModeFallback, digest A
    P4-->>P2: classifier_blocked_retry, token consumed
    P2->>P3: evaluateAutoMode, classifier skipped
    P3-->>P2: fallback, no classifier request
    P2->>P5: dialog, allow once or cancel only
    P5->>P6: one time manual review
    P6-->>P2: approve once
    P2->>P4: recordFallbackApprove clears streak
    P2-->>P1: executed
Loading
Files changed (14 of 14 shown)
File What changed
packages/core/src/permissions/denialTracking.ts Adds the optional one-shot retry fingerprint to the denial state, threads it through record/allow/shouldFallback, and adds the consume helper.
packages/core/src/permissions/autoMode.ts The core of the change: fingerprint helper, prepare/consume wrapper, post-record threshold recheck that turns the threshold-reaching block into a fallback, new per-reason user messages, widened hook predicate, and the reworded denial guidance.
packages/core/src/permissions/index.ts Re-exports the three new symbols.
packages/core/src/tools/tools.ts Widens the fallback reason union from a single literal to the six reasons that can carry a message.
packages/core/src/core/coreToolScheduler.ts Both L5 sites compute the fingerprint, prepare the fallback, and pass the reason through the decorator instead of a bare message.
packages/cli/src/acp-integration/session/Session.ts Same transition mirrored for the ACP path, so daemon sessions behave identically.
packages/cli/src/acp-integration/session/permissionUtils.ts Offers the switch-to-Default option only for classifier-unavailable, keeping policy-block recovery in AUTO.
packages/cli/src/ui/components/messages/ToolConfirmationMessage.tsx Splits "show the warning banner" from "offer the mode switch" and recomputes the compact-mode option height from the real option set.
packages/core/src/permissions/denialTracking.test.ts Pins digest-only storage, exact-match routing, mismatch rejection, pre-confirmation consumption, survival across unrelated allowed work, and clearing on allow.
packages/core/src/permissions/autoMode.test.ts Pins both threshold boundaries falling back on the current call, fingerprint cwd sensitivity, decorator back-compat, and hook firing.
packages/core/src/core/coreToolScheduler.test.ts End-to-end blocked then changed then exact retry then manual approval, no execution without approval, pending re-evaluation parity, threshold block stays pending.
packages/cli/src/acp-integration/session/Session.test.ts ACP exact retry reaches permission request with no classifier call, and ACP threshold block reaches manual approval.
packages/cli/src/acp-integration/session/permissionUtils.test.ts Blocked retries stay in AUTO and hide persistent choices.
packages/cli/src/ui/components/messages/ToolConfirmationMessage.test.tsx Renders blocked-retry guidance without offering a mode switch.

Test evidence

Evidence carried here: the PR's own CI, read through the API. This is an unattended pull_request_target run, so nothing was built, executed, or checked out — no PR-derived code ran. There are no failing checks, so there is no log excerpt to quote. The honest headline is that the two checks that matter most for this diff had not reported yet at review time: Test (ubuntu-latest, Node 22.x) and Lint & Static (ubuntu-latest, Node 22.x) are both still in_progress, and Test (macos-latest) / Test (windows-latest) are skipped on this commit. So the unit suite covering the changed permission and scheduler files is not verified: still running. Per the workflow this run does not poll — the finalize job rewrites the table below once CI settles.

What has landed green is relevant but not decisive: Integration Tests (no-AK, No Sandbox), TUI parity snapshots (ink vs opentui), OpenTUI no-flicker gate, Security Checks, Secret scan (TruffleHog), Dependency CVE audit, both Desktop Shell builds, and the SDK Java matrix except Real daemon E2E / Java 11, which is still running — that one is worth watching, since this PR changes the ACP daemon's permission-option surface.

The author posted an E2E report claiming 525/525 core, 813/813 ACP Session, and 55/55 permission-option and TUI regressions, plus build, lint, typecheck and format on macOS with Node 24.14.1. That is the author's self-reported result on a single platform, not evidence this review re-ran or can confirm, and the tool execution and classifier responses in it were mocked.

Final CI results for b7bc46d (auto-updated by the triage finalize job after CI completed):

Check Conclusion
web-shell E2E Smoke (ubuntu-latest, Node 22.x) ❌ failure
Classify PR ✅ success
Dependency CVE audit ✅ success
Desktop Shell (ubuntu-22.04) ✅ success
Desktop Shell (windows-2022) ✅ success
Integration Tests (no-AK, No Sandbox) ✅ success
Lint & Static (ubuntu-latest, Node 22.x) ✅ success
macos-latest / Java 21 ✅ success
OpenTUI no-flicker gate ✅ success
Real daemon E2E / Java 11 ✅ success
Secret scan (TruffleHog) ✅ success
Test (ubuntu-latest, Node 22.x) ✅ success
TUI parity snapshots (ink vs opentui) ✅ success
ubuntu-latest / Java 11 ✅ success
ubuntu-latest / Java 17 ✅ success
ubuntu-latest / Java 21 ✅ success
windows-latest / Java 21 ✅ success

One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行(取最新一次运行),省略 skipped,失败项排在最前。

Sandboxed verification would settle this, and the author has write access so both lanes are available. @qwen-code /tmux — that the policy-block recovery dialog really renders allow-once and cancel only, with the warning banner and no "Switch to Default Mode" entry; that is a TUI surface, the option set is rebuilt by hand in two places, and the compact-mode height math I verified above is exactly the kind of thing only a real terminal capture proves. @qwen-code /verify — that an exact retry opens manual review without a classifier request while a changed argument or cwd is classified normally, and that the third consecutive block and twentieth total denial fall back on the current call; the suite would still pass with the threshold recheck reverted, since the off-by-one shows up as a behaviour difference rather than a crash. Neither lane has been triggered on this commit, so the central behavioural claim currently rests on the diff reading above plus the author's mocked single-platform report.

中文说明

本节证据:仅通过 API 读取该 PR 自身的 CI 结果。 这是无人值守的 pull_request_target 运行,因此没有构建、执行或 checkout 任何代码 —— 未运行任何来自 PR 的代码。没有失败的 check,所以没有日志摘录可引用。

代码审查:我在看 diff 之前先写下了自己的方案(在 recordBlock 之后于 applyAutoModeDecision 内重新检查 shouldFallback 修 off-by-one;用绑定"工具+参数+cwd"摘要的一次性标记来做误判恢复,该标记只选择既有确认路径、绝不授予执行;弹窗收敛为仅允许一次/取消;并修改拒绝指引让模型知道重试入口存在)。PR 的实现与之一致,包括我最担心会被漏掉的一点:PermissionDenied hook。

一处建议在合并前修复的问题AUTO_MODE_DENIAL_GUIDANCE 被两个分支共用,而新增的那句话只在其中一个分支成立。它既随 formatClassifierBlockMessage 下发(该路径通过 recordBlock(denialState, actionFingerprint) 装载标记,"重试同一调用"确实会弹出确认),也随 blocked:destructive-command 分支下发 —— 而后者仍然是不带指纹的 recordBlock(denialState),并无条件返回 {kind:'blocked'}。因此当确定性破坏性命令保护生效时,模型会被告知"如需人工批准,请原样重试同一工具调用",而这次重试永远不可能带来人工复核。这比无用更糟:原先的措辞("停止并向用户请求明确批准")被替换掉了,于是该路径上模型失去了一条可执行的指令、换来一条假的,而且发生在最危险的命令类别上。我确认安全属性依然成立 —— evaluateAutoMode 中 L5.2.5 在 skipClassifierReason 提前返回之前执行,所以破坏性命令即使带着已装载的标记、即使计数已达阈值,仍然是硬拦截,绝不会提供弹窗。这是 fail-closed 的:属于会诱导重试循环、浪费轮次的模型指引缺陷,而不是绕过。最省的修法是拆分该常量,让破坏性分支使用不含重试句子的版本 —— 在那里装载标记恰恰是错误的。

已核实正确的部分:标记无法授予执行(在跳过分类器前同步消费,仅选择 fallback 路径);摘要特性(sha256 规范化,字段重排无法规避匹配、旧别名无法洗白、cwd 或参数变化即重新分类);审计链路保留(阈值拦截转为 fallback 后 hook 仍以 classifier_blocked 触发,而分类器不可用与 safety_check/ask_rule 不触发);恢复弹窗确实只提供允许一次/取消且会话保持 AUTO;无 TUI 布局回归(compact 高度公式与 renderedOptions 构造精确对应,且比原先硬编码的 3 更准确);diff 之外的消费点全部核对(三处 autoModeFallback 读取点行为正确,新增参数均为可选因而不破坏既有调用方,子 agent 使用全新的 createDenialState() 因此新字段不会泄漏);三条运行时路径(核心调度、等待调用重评估、ACP Session)同步改动。

一处未在描述中提及的行为变化(是问题,不是阻塞):如实标注 decorator 的 reason 之后,external_write 回退不再被标为 classifier_unavailable,因此 TUI 和 ACP 选项列表都不再提供"Switch to Default Mode and allow once (recommended)"。这看起来是修掉了一个潜在的标注错误(该措辞只在分类器不可用时才成立),但它是用户可见的、描述里没有提,也没有测试固定新的 external-write 选项集合。

测试证据的诚实结论:对本次 diff 最关键的两个 check —— Test (ubuntu-latest, Node 22.x)Lint & Static (ubuntu-latest, Node 22.x) —— 在审查时仍为 in_progress,而 Test (macos-latest) / Test (windows-latest) 在该 commit 上为 skipped。因此覆盖所改权限与调度文件的单元测试未验证:仍在运行。按流程本次不做轮询,CI 落定后由 finalize 任务就地更新上表。已变绿的部分相关但不决定性;其中 Real daemon E2E / Java 11 仍在运行,值得留意,因为本 PR 改动了 ACP daemon 的权限选项面。

作者发布的 E2E 报告称 core 525/525、ACP Session 813/813、权限选项与 TUI 回归 55/55,以及 macOS + Node 24.14.1 上的构建、lint、typecheck、format 通过。这是作者在单一平台上的自述结果,不是本次 review 复跑或能够确认的证据,且其中的工具执行与分类器响应均为 mock。

沙箱验证可以定案,且作者具有 write 权限,两条通道均可用。@qwen-code /tmux —— 验证策略拦截恢复弹窗是否真的只渲染允许一次与取消、带警告横幅且不含"Switch to Default Mode";这是 TUI 界面,选项集合在两处手工重建,而我上面核实的 compact 高度数学正是只有真实终端截图才能证明的那类问题。@qwen-code /verify —— 验证原样重试是否在不发起分类器请求的情况下进入人工复核、而修改参数或 cwd 会重新分类,以及第 3 次连续拦截与第 20 次会话拒绝是否在当前这次调用即回退;把阈值重检改回原样测试仍会全绿,因为 off-by-one 只体现为行为差异而非崩溃。该 commit 上两条通道都尚未触发,因此核心行为主张目前仅由上述 diff 阅读与作者的 mock 单平台报告支撑。

Qwen Code · qwen3.8-max-2026-09-02

Reviewed at b7bc46de118a5d37b9c503bc418d8dd6acae4158 · re-run with @qwen-code /triage

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Confidence: 3/5 — the design is sound and I could not find a safety regression, but it ships one false instruction to the model on the destructive-command path, and the CI that would cover it has not reported.

Stepping back: this is a good PR. It fixes a closed loop that a real user hit — three explicit confirmations, three refusals, ten minutes, nothing executed — and it fixes the half of #11019 that the issue's own triage singled out as the one that "can move faster" with "a much smaller safety delta". I wrote my own design before reading the diff and the PR arrived at the same one, including the PermissionDenied hook subtlety I would have expected to be missed. The token is genuinely single-use, genuinely bound to a digest of tool + args + cwd, and genuinely incapable of executing anything on its own. Two thirds of the diff is tests, and they pin the right things — exact match, mismatch, pre-confirmation consumption, both threshold boundaries, the hook, ACP parity, the reduced option set. If I inherited this in six months I would thank the author, not curse them.

So why not approve. Three things, in descending order of weight.

The guidance defect is real and it is a regression, not a nit. AUTO_MODE_DENIAL_GUIDANCE now tells the model that retrying the exact call requests manual approval. That is true for a classifier policy block and false for a deterministic destructive-command block, which shares the same string but never arms a token and never reaches a dialog — L5.2.5 runs before the fallback is consulted, so it stays hard-blocked no matter how many times the model retries. Base said "stop and ask the user for explicit approval"; this diff replaces that with an instruction to retry. On the most dangerous category of command, the model is now invited into a loop that cannot terminate in a human decision. Nothing unsafe happens — it fails closed — but it burns turns and misinforms the model, and it is a two-line fix (split the constant; do not arm the token there).

The decisive CI has not landed. Test (ubuntu-latest, Node 22.x) and Lint & Static (ubuntu-latest, Node 22.x) were still in_progress at review time and macOS/Windows unit runs are skipped on this commit. The suite covering the changed permission and scheduler files is therefore unverified by anything except the author's own mocked, single-platform report. I have deliberately not attached the deferred-approval marker that this comment could otherwise carry, so the finalize job will not auto-approve the PR when CI goes green — a green suite would show the tests pass, not that the finding above is resolved.

This is a security boundary in a revert-correlated path, arriving in volume. The change rewrites how AUTO mode can be overridden, in the area #4538 specifically hardened, and it touches acp-integration, which this repo's revert history flags as high-risk. It is also one of ten open PRs from this author in three days, spanning core permissions, goal, channels, dingtalk, web-shell and ipc. Each deserves its own judgement on merit and this one earns a good deal of credit — but "broad, fast, and mostly fork-sourced" is exactly the shape where a bot approval is the wrong signature, and where the fork-refactor guardrail would have capped this at 3/5 had the title been typed differently.

Two smaller things for whoever picks this up: the external_write decoration change is a genuine latent-bug fix (those fallbacks were being labelled classifier_unavailable and offered a mode switch that made no sense for them), but it is user-visible, unmentioned in the description, and untested — worth one line in the description and one assertion. And @qwen-code /tmux plus @qwen-code /verify are both available here since the author has write access; the option-list rendering and the "exact retry skips the classifier" claim are the two things static review cannot settle.

⏸️ Deferring rather than approving or rejecting. I could not resolve an accountable maintainer deterministically to hand this to: QWEN_MAINTAINER_HANDLE is not set in this environment, the PR carries no area labels for the owner policy to match on, there is no prior human reviewer to fall back to, and the scripted resolver itself was blocked from executing by this run's permission rules. I am not going to guess a login. The author has write access and is a listed area owner, so the practical path is: address the destructive-command guidance split (or say why it is intentional), let the ubuntu unit and lint checks land, and then re-run @qwen-code /triage — with the finding resolved and CI green this is a straightforward approve.

中文说明

置信度:3/5 —— 设计是可靠的,我没有找到安全性回退,但它在破坏性命令路径上向模型下发了一条假指令,而能覆盖这一点的 CI 还没有出结果。

退一步看:这是一个好 PR。它修复了真实用户撞上的死循环 —— 三次明确同意、三次拒绝、十分钟、零条语句执行 —— 而且修的正是 #11019 自身 triage 挑出的"可以更快推进、安全代价远小于投影改动"的那一半。我在看 diff 前先写下了自己的方案,PR 与之一致,包括我本以为会被漏掉的 PermissionDenied hook 细节。恢复标记确实是一次性的、确实绑定"工具+参数+cwd"摘要、也确实本身无法执行任何东西。diff 的三分之二是测试,且固定了正确的点:精确匹配、不匹配、确认前消费、两个阈值边界、hook、ACP 一致性、收敛后的选项集合。如果半年后接手这份代码,我会感谢作者。

那么为什么不批准。三点,按权重递减。

指引缺陷是真实的回退,不是小毛病。 AUTO_MODE_DENIAL_GUIDANCE 现在告诉模型"原样重试即请求人工批准"。这对分类器策略拦截成立,对确定性破坏性命令拦截不成立 —— 两者共用同一字符串,但后者从不装载标记、也永远到不了弹窗:L5.2.5 在 fallback 被查询之前执行,因此无论模型重试多少次都仍是硬拦截。基线写的是"停止并向用户请求明确批准",本 diff 把它换成了"去重试"。于是在最危险的命令类别上,模型被引入一个不可能以人工决策收尾的循环。没有不安全的事发生 —— 它是 fail-closed 的 —— 但它浪费轮次并误导模型,而且修复只需两行(拆分该常量;不要在那里装载标记)。

决定性的 CI 还没落地。 审查时 Test (ubuntu-latest, Node 22.x)Lint & Static (ubuntu-latest, Node 22.x) 仍为 in_progress,且该 commit 上 macOS/Windows 单元测试为 skipped。因此覆盖所改权限与调度文件的测试套件,除作者自己在单一平台上以 mock 方式给出的报告之外,没有任何东西验证过。我刻意没有附上本条评论原本可以携带的那条延迟批准标记,所以 CI 变绿后 finalize 任务不会自动批准本 PR —— 全绿的套件只能证明测试通过,不能证明上面那条缺陷已被解决。

这是位于 revert 高相关路径上的安全边界,而且是批量到达的。 该改动重写了 AUTO 模式可被覆盖的方式,正处在 #4538 专门加固过的区域,并且触及本仓库 revert 历史标记为高风险的 acp-integration。它同时是该作者三天内十个 open PR 之一,横跨 core 权限、goal、channels、dingtalk、web-shell 与 ipc。每一个都应按自身价值独立判断,这一个也确实值得相当多的肯定 —— 但"范围广、速度快、多数来自 fork"恰恰是机器人批准最不该出现的形态;如果标题类型是 refactor,fork-refactor 护栏本来也会把它封顶在 3/5。

两个较小的点留给接手者:external_write 的 decoration 变化是一处真实的潜在 bug 修复(这类回退此前被标为 classifier_unavailable,并提供了一个对它毫无意义的模式切换选项),但它是用户可见的、描述里没提、也没有测试 —— 值得在描述里加一句、加一条断言。另外,作者具有 write 权限,因此 @qwen-code /tmux@qwen-code /verify 两条通道都可用;选项列表渲染与"原样重试跳过分类器"这两点正是静态 review 无法定案的。

⏸️ 选择转交,而不是批准或拒绝。 我无法确定性地解析出一位可负责的 maintainer 来接手:本环境未设置 QWEN_MAINTAINER_HANDLE,该 PR 没有任何可供 owner 策略匹配的领域标签,也没有可回退的既有 human reviewer,而脚本化解析器本身被本次运行的权限规则阻止执行。我不会去猜一个登录名。作者具有 write 权限且是登记的领域 owner,因此现实路径是:处理破坏性命令的指引拆分(或说明这是有意为之),等 ubuntu 单元测试与 lint 落地,然后重跑 @qwen-code /triage —— 在该缺陷解决且 CI 全绿的前提下,这会是一个直接的 approve。

Qwen Code · qwen3.8-max-2026-09-02

Reviewed at b7bc46de118a5d37b9c503bc418d8dd6acae4158 · re-run with @qwen-code /triage

@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.

Partially reviewed — gaps disclosed.

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): "agent reverse-audit (round 4)": none — but I did not open chunks 1-4 (other agents' territory); my cross-file reads of scheduler/Session/autoMode were limited to verifying chunk-5 interactions….

中文说明

仅完成部分审查,审查缺口已披露。

未审查(原文为英文):build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.

未探索到全部深度(达到工具调用预算):"agent reverse-audit (round 4)"none — but I did not open chunks 1-4 (other agents' territory); my cross-file reads of scheduler/Session/autoMode were limited to verifying chunk-5 interactions…

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

Comment thread packages/core/src/permissions/autoMode.ts
Comment thread packages/core/src/permissions/denialTracking.ts
Comment thread packages/core/src/permissions/denialTracking.ts Outdated
Comment thread packages/cli/src/ui/components/messages/ToolConfirmationMessage.tsx
Comment thread packages/core/src/permissions/autoMode.ts Outdated
Comment thread packages/core/src/permissions/autoMode.ts Outdated
Comment thread packages/cli/src/ui/components/messages/ToolConfirmationMessage.tsx Outdated
Comment thread packages/core/src/permissions/autoMode.ts
Comment thread packages/core/src/core/coreToolScheduler.test.ts
Comment thread packages/core/src/permissions/autoMode.ts

@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.

Partially reviewed — gaps disclosed. Suggestions are inline.

Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.

Deferred under the convergence posture (round 2, not a blocker) — recorded, not requested in this round:

  • packages/core/src/permissions/autoMode.ts:688 — [review] system prompt 'Denied Tool Calls' rule contradicts the new retry guidance while the doc comment claims they stay in sync
  • packages/core/src/permissions/denialTracking.ts:10 — [review] user docs (auto-mode.md, approval-mode.md) still describe the removed next-call fallback semantics and omit the exact-retry path
  • packages/core/src/permissions/denialTracking.ts:39 — [review] formatDenialStateLog omits pendingManualRetryFingerprint — no log can show whether the retry token is armed, replaced, or consumed
  • packages/core/src/permissions/denialTracking.ts:97 — [review] recordAllow's !clearsPendingRetry zero-counter token-clear term has no witnessing test (mutant survives 533 tests)
  • packages/core/src/core/coreToolScheduler.ts:6562 — [review] pending-path fallback debug line bypasses formatDenialStateLog and drops the totals — two log shapes for one state machine
中文说明

仅完成部分审查,审查缺口已披露。 建议见行内评论。

未审查(原文为英文):build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.

收敛姿态下延后(第 2 轮,非阻断)——已记录,本轮不要求修改:共 5 条(原文未翻译,列表见上方英文部分)。

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

Comment on lines 207 to +209
if (hasReachedTotalCap(state)) {
return createDenialState();
return {
...state,

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] R1-3: (fix-induced) the fix for R1-2/R1-3 in efe3f9d removed the fingerprint disarm from recordFallbackApprove — the right call for unrelated approvals, but it also means approving the exact blocked action through any other fallback path no longer clears the armed retry token, so the next exact issuance skips the classifier and is re-prompted as "previously blocked" for an action the user just approved. Concretely: a parallel batch issues two identical calls X1, X2 (same tool/args/cwd, same fingerprint); the classifier flaps — X2 is classified unavailable, X1 is policy-blocked and arms pendingManualRetryFingerprint=X; the user approves X2, and recordFallbackApprove (called from recordAutoModeFallbackResolution in coreToolScheduler.ts and mirrored in Session.ts) resets the counters but has no fingerprint parameter, so the token survives. When the model then follows the denial guidance and retries X exactly, the stale token matches, the classifier is skipped (0 calls), and the user sees "Auto mode previously blocked this exact action. Review it manually." for the action they just approved. It fails safe — one extra prompt, never a bypass, and it self-heals on re-approval — but the retry message is factually wrong and muddies the approval loop.

Witness:

probe (end-to-end through the real scheduler, unmodified PR code):
state-after-approve={"consecutiveBlock":0,"consecutiveUnavailable":0,"totalBlock":1,
 "totalUnavailable":1,"pendingManualRetryFingerprint":"48904b2c..."}
retry-awaiting fallback={"reason":"classifier_blocked_retry",
 "message":"Auto mode previously blocked this exact action. Review it manually."} classifierCalls=0
FIXED arm (optional actionFingerprint + exact-match disarm): state-after-approve has no token;
 retry-executed classifierCalls=1 — probe flipped both directions; all 43 denialTracking tests still pass.

Suggested fix — give recordFallbackApprove an optional fingerprint and disarm only on an exact match, then thread the approved call's fingerprint through from recordAutoModeFallbackResolution (store it alongside autoModeFallbackCallIds, or recompute from the stored args + config.getCwd()) and from the ACP Session outcome handler:

export function recordFallbackApprove(
  state: AutoModeDenialState,
  actionFingerprint?: string,
): AutoModeDenialState {
  const disarm =
    actionFingerprint !== undefined &&
    state.pendingManualRetryFingerprint === actionFingerprint
      ? { pendingManualRetryFingerprint: undefined }
      : {};
  if (hasReachedTotalCap(state)) {
    return {
      ...state,
      consecutiveBlock: 0,
      consecutiveUnavailable: 0,
      totalBlock: 0,
      totalUnavailable: 0,
      ...disarm,
    };
  }
  // ...same disarm applied to the other two branches
}

Note the disarm must be conditional on an exact fingerprint match, never unconditional: this PR's own tests pin preservation for unrelated approvals (it.each(['an unrelated fallback approval', recordFallbackApprove]) and 'preserves an unrelated retry when resetting the total denial cap' in denialTracking.test.ts). If you add the disarm, please also add a denialTracking.test.ts case asserting recordFallbackApprove(state, 'blocked-action') clears a pendingManualRetryFingerprint of 'blocked-action' while recordFallbackApprove(state, 'other-action') preserves it, and confirm removing the disarm branch makes that test fail.

中文说明

R1-3(由修复引入):efe3f9d968 对 R1-2/R1-3 的修复从 recordFallbackApprove 中移除了指纹解除逻辑——对无关批准而言是正确的,但这也意味着通过其他 fallback 路径批准恰好被拦截的那个动作时,不再清除已武装的重试令牌,于是下一次完全相同的调用会跳过分类器,并对用户刚刚批准过的动作再次弹出"此前已拦截"的提示。具体场景:并行批次发出两个完全相同的调用 X1、X2(相同工具/参数/工作目录,指纹相同);分类器抖动——X2 被判为不可分类,X1 被策略拦截并武装 pendingManualRetryFingerprint=X;用户批准 X2,recordFallbackApprove(由 coreToolScheduler.ts 中的 recordAutoModeFallbackResolution 调用,Session.ts 中有对应实现)重置计数器,但没有指纹参数,令牌因此残留。当模型按拒绝指引精确重试 X 时,残留令牌命中,分类器被跳过(0 次调用),用户会对刚刚批准过的动作看到"Auto mode previously blocked this exact action. Review it manually."。该问题失败时是安全的——只多一次提示、绝不绕过审批,且再次批准即可自愈——但重试提示文案与事实不符,会干扰审批流程。

建议修复:给 recordFallbackApprove 增加可选指纹参数,仅在完全匹配时解除令牌,并把被批准调用的指纹从 recordAutoModeFallbackResolution(与 autoModeFallbackCallIds 一并存储,或用已存参数 + config.getCwd() 重新计算)以及 ACP Session 的结果处理处传入。解除逻辑必须以指纹精确匹配为条件、绝不能无条件清除:本 PR 自身的测试已钉住无关批准的保留行为(denialTracking.test.ts 中的 it.each(['an unrelated fallback approval', recordFallbackApprove])'preserves an unrelated retry when resetting the total denial cap')。若添加解除逻辑,请补充 denialTracking.test.ts 用例:断言 recordFallbackApprove(state, 'blocked-action') 会清除值为 'blocked-action'pendingManualRetryFingerprint,而 recordFallbackApprove(state, 'other-action') 保留它,并验证移除解除分支会使该测试变红。

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

@qwen-code-dev-bot qwen-code-dev-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.

Reviewed at head 21f88f25.

  • Security spine verified line by line: the manual-retry token is a digest of tool+args+cwd (getAutoModeActionFingerprint over the existing repeat-key helper), armed only by a classifier policy block, checked before skipping the classifier, and consumed on both the allow and the reject turn — and the scheduler's fallback case never executes: it drops into the existing human confirmation flow, so the fallback relaxes classification, never approval. Destructive guidance keeps the old no-workaround wording and gains no retry suggestion; the threshold off-by-one fix routes the threshold-reaching call into the same human-gated path immediately; the hook emission excludes classifier-unavailable while the exhaustive never in the message formatter keeps the reason set closed.
  • The shared-merge detail on the settings side and the Session/pending-call re-evaluation paths mirror the same transition, per the pinning tests (197 new Session tests, 207 autoMode, 78 denialTracking).
  • One open thread (parallel-identical-calls re-prompting an action just approved through the sibling's dialog) is a duplicate-prompt UX edge in a classifier-flap scenario — the human gate holds throughout; Suggestion, worth the author's follow-up note, not a blocker.
  • CI facts: 24 checks pass; the single Test (ubuntu) failure is the main-residual #10988 source guard (acpAgent.ts:9200 still names runWithAcpRuntimeOutputDir directly on current main — identical bytes here and there, and I flagged it on #11033), not this PR's doing. The page's CHANGES_REQUESTED is ci-bot's round-1 ticket at the first head; its re-reviews since then at the current head were COMMENTED without new blockers — a @qwen-code /triage re-run clears the stale ticket.

@yiliang114 yiliang114 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.

Reviewed the retry-token mechanism at head 21f88f2: the lifecycle is closed and fail-safe — only classifier policy blocks arm the fingerprint token (destructive blocks deliberately don't, since their guard runs before the manual-review promise can be honored); the token is consumed before the fallback dialog is shown; all four recordAllow call sites clear it on exact-fingerprint match only; state is per-Config-instance memory with no cross-session persistence. The token can only ever downgrade a call to manual confirmation, never auto-approve it, so a stale token costs one extra prompt at worst — no bypass path. Concurrency-safe: identical parallel calls each snapshot independently and get their own dialogs; prior-round fixes (R1-1 destructive guidance split, consumption timing, reject-then-reclassify) are all present and test-pinned.

R1-3 (open Suggestion) verified as still standing but bounded: approving the exact blocked action through the unavailable-verdict fallback path no longer disarms the token, so the next identical issuance is re-prompted once — an extra prompt, not a gating or safety issue. Fine to land as a follow-up.

Note on CI: the red Test (ubuntu) is inherited from main — the acpAgent runtime-root-pinning chokepoint test flags a direct runWithAcpRuntimeOutputDir mention at acpAgent.ts:9200, and both that test and that line exist on main (b0a463c); this PR touches neither file.

@qqqys
qqqys added this pull request to the merge queue Sep 5, 2026
Merged via the queue into QwenLM:main with commit f6ba502 Sep 5, 2026
116 of 121 checks passed
@chiga0

chiga0 commented Sep 5, 2026

Copy link
Copy Markdown
Collaborator

Post-merge review of head 21f88f25 (merged during review; no approval event). Deep-tier pass over the AUTO-mode manual-retry path, executed locally on linux / node v24.20.0.

Verified in the code at this head

  • The retry token is a canonicalized sha256 over tool + args + cwd (getAutoModeActionFingerprint on the repeat-key helper): field reordering cannot evade it, and a changed argument or working directory is classified normally.
  • The deterministic destructive-command guard runs before the classifier (L5.2.5) and its decision branch never touches the fingerprint path — manual retry relaxes classification only, never the hard guards, and keeps the strict no-workaround guidance with no retry suggestion.
  • One-shot lifecycle: the fingerprint is armed only by a classifier policy block, consumePendingManualRetry runs before the confirmation is displayed, and recordAllow clears it on an exact match only; the threshold off-by-one is fixed — shouldFallback is evaluated on the post-increment state so the call that reaches a limit enters manual review immediately.
  • Policy-block recovery offers allow-once/cancel only: hideAlwaysAllow on every non-ask_user_question fallback confirmation, and the switch-to-Default option is reserved for the two unavailable reasons. Permission-denied hooks fire only for real policy blocks and threshold fallbacks, not for unavailable or unrelated safety fallbacks.
  • ACP Session and pending-call re-evaluation share the same transition functions.

Executed: core permissions suites 533/533 · cli suites 922 tests: 920 pass plus 2 failures that are host-environment artifacts — this daemon session exports QWEN_RUNTIME_DIR, which overrides Storage.setRuntimeBaseDir in the two runtime-dir pinning tests; both pass with the variable unset, and neither the tests nor the runtime-dir logic are in this PR's diff. One mutation probe (ignoring the fingerprint match in shouldFallback) is killed by the suites.

Cross-check: both existing approvals at this head match my read, including the one open bounded Suggestion (re-prompt once when the exact blocked action is approved through a sibling's unavailable-verdict fallback — an extra prompt, not a gating gap). The ubuntu CI red is the main-residual source-guard item both approvals attribute to main, not this diff. Not covered here: live classifier E2E (no environment), Windows/macOS behaviour (linux only).

No blockers found in what merged.

Reviewed with AI assistance.

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.

5 participants