fix(permissions): escalate repeated destructive-command denials to manual approval - #13636

[Suggestion] R1-1: shouldFallback(blockedState) inherits the shared session cap, and hasReachedTotalCap sums totalBlock + totalUnavailable (denialTracking.ts:47-53). recordUnavailable is driven purely by classifier infrastructure failure (autoMode.ts:605-612 on decision.unavailable, set in classifier.ts:265 and :384), so a session with zero policy blocks and zero destructive commands can arm maxTotalDenials from outage traffic alone — and from that point the deterministic L5.2.5 guard's first denial of git reset --hard returns kind: 'fallback' instead of a hard error.

Concretely: a session suffers a classifier API outage, 20 recordUnavailable events accumulate and the user approves 9 outage prompts to keep working, so totalBlock stays 0 and nothing was ever blocked on policy. The model then issues its first git reset --hard. Before this change that is a hard error carrying the destructive reason and the anti-workaround guidance; after it, hasReachedTotalCap is already true, shouldFallback returns total_denial (which takes precedence over consecutive_block, denialTracking.ts:158-160), and the guard's verdict becomes an approvable prompt in exactly the prompt-fatigued session the deterministic guard exists to survive — with a banner that does not mention the command is destructive (see the comment on the message line below).

Three committed statements say the opposite: destructive-commands.ts:183-184 ("failures here are hard blocks regardless of classifier availability"), autoMode.ts:841-843 ("Regex-based hard blocks ... so API failures or classifier misjudgment cannot allow destructive git/IaC commands through"), and docs/design/auto-classifier-unavailable-fallback.md:70. Your Risk & Scope section does disclose the tradeoff for "3 consecutive blocks (or 20 session denials)", but the disclosed state is 20 denials; nothing argues for a cap armed by unavailability records, and the new test comments argue the sibling state instead.

Witness (A/B probe over the real compiled prepareAutoModeFallback -> evaluateAutoMode -> applyAutoModeDecision; BASE arm = this worktree's compiled autoMode.js with only the case 'blocked:destructive-command' block replaced by the verbatim merge-base blob git show 9cf2488f52:packages/core/src/permissions/autoMode.ts; arm proof grep -c "const blockedState = recordBlock(denialState, actionFingerprint);" = BASE 1 / PR 2):

BASE totals before the call {"unavailableEvents":20,"outagePromptsApproved":9,"totalBlock":0,"totalUnavailable":20}
BASE OUTCOME `git reset --hard` {"kind":"blocked","reason":"classifier_blocked"}
BASE what the model receives    {"errorMessage":"Blocked destructive git command: \"git reset --hard\". ... stop and ask the user for explicit approval. ..."}
PR   OUTCOME `git reset --hard` {"kind":"fallback","reason":"total_denial"}
PR   what the user receives     {"message":"Auto mode reached its session denial limit. Review this action manually."}

Either exempt this branch from the total_denial route and keep consecutive_block — the same-action anti-loop case the PR set out to fix — by honouring the escalation here only when fallback.reason === 'consecutive_block', or comparing blockedState.totalBlock alone against maxTotalDenials on this path; or, if the shared cap is intended for destructive denials too, update the three statements above and name the outage-armed case in Risk & Scope.

The caps are shared with the classifier path and must not move: AUTO_MODE_DENIAL_LIMITS = { maxConsecutiveBlock: 3, maxConsecutiveUnavailable: 2, maxTotalDenials: 20 } (denialTracking.ts:41-45), and hasReachedTotalCap deliberately sums both totals so "alternating denial modes cannot avoid fallback" (denialTracking.ts:19-21, :47-53) — a fix scoped to this branch must leave the classifier branch's totals semantics intact.

If you add the filter, please add the test that pins it: assert apply(destructive(), counters(0, 0, 0, 19), fingerprint) still returns kind: 'blocked' (19 unavailable records, zero policy blocks), then remove the filter and confirm that test goes red.

BASE 调用前的计数 {"unavailableEvents":20,"outagePromptsApproved":9,"totalBlock":0,"totalUnavailable":20}
BASE `git reset --hard` 的结果 {"kind":"blocked","reason":"classifier_blocked"}
BASE 模型收到的内容           {"errorMessage":"Blocked destructive git command: \"git reset --hard\". ... stop and ask the user for explicit approval. ..."}
PR   `git reset --hard` 的结果 {"kind":"fallback","reason":"total_denial"}
PR   用户看到的内容            {"message":"Auto mode reached its session denial limit. Review this action manually."}

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

Mechanism confirmed, but not fixing it in this pass — this one moves a permission boundary on a deterministic safety guard and needs a maintainer's call. Leaving it unresolved.

Verified at fa1c3a9fc7: hasReachedTotalCap sums both totals (denialTracking.ts:47-51), recordUnavailable increments totalUnavailable (denialTracking.ts:143), total_denial takes precedence over consecutive_block (denialTracking.ts:157-161), and the destructive branch runs shouldFallback over that shared state (autoMode.ts:590). So a session whose 20 records are all classifier-outage traffic does arm the cap at totalBlock === 0, and the first git reset --hard then returns kind: 'fallback' instead of the hard error. The three committed statements you quote are accurate as written: destructive-commands.ts:183-184 ("failures here are hard blocks regardless of classifier availability"), autoMode.ts:841-843, and docs/design/auto-classifier-unavailable-fallback.md:70.

Why it is not in fa1c3a9fc7 — the two fixes you offer both change a boundary, and they contradict what this PR already committed to:

1. Honouring the escalation only on consecutive_block requires deleting degrades to manual approval at the session total-denial cap (counters(0, 0, 19, 0) -> total_denial), a test this PR added on purpose and whose comment argues the opposite position ("Destructive denials previously counted towards the cap but could never trigger it"). That reverses a stated design decision rather than tightening an oversight.
2. Comparing blockedState.totalBlock alone against maxTotalDenials on this path gives the destructive branch different totals semantics from the classifier branch, against the explicit invariant at denialTracking.ts:19-21 that alternating denial modes cannot avoid fallback.

Either way the user-visible outcome flips — hard error carrying the destructive reason and the anti-workaround guidance, versus an approvable banner — so it also needs before/after terminal evidence on a real prompt, not just a unit test. That is not something to decide inside a Suggestion-level cleanup.

The decision for a maintainer is binary: (a) restrict the route to consecutive_block, then update the three statements and delete or repoint the total-cap test; or (b) keep the shared cap, then update the same three statements and name the outage-armed case explicitly in Risk & Scope, since "20 session denials" as currently disclosed does not tell a reviewer that unavailability traffic alone can arm it.

Nothing in fa1c3a9fc7 changes this behaviour — that commit is test-only (autoMode.test.ts, +28/-4).

[Suggestion] R1-3: This return falsifies a failure boundary stated in committed documentation that the diff does not update. docs/design/auto-classifier-unavailable-fallback.md:70, under ## Failure boundaries, reads "Explicit permission denies and deterministic destructive-command blocks remain errors."; docs/users/features/auto-mode.md:224 attributes the fallback to "3 consecutive policy blocks". After this change a deterministic destructive-command block returns kind: 'fallback', not an error, once consecutiveBlock reaches 3 or session totalBlock + totalUnavailable reaches 20.

The cost is that a maintainer or security reviewer deciding whether AUTO mode can ever surface git reset --hard / terraform destroy as an approval prompt reads that failure-boundary line and concludes it cannot. The next change written on that premise — a new consumer of AutoModeOutcome, a new transport, or a test asserting a hard-blocked command can never reach a confirmation dialog — is built against an invariant the implementation no longer has, and nothing in the doc points at the exception. The concrete states are the two caps, including a first destructive denial in a session already at 19 unrelated denials, which this diff's own counters(0, 0, 19, 0) test pins.

Witness (both docs read at the reviewed commit — the diff touches exactly two files, autoMode.ts and autoMode.test.ts, and neither doc is in it — plus a pristine pipeline probe showing a deterministic destructive block does not remain an error at the cap):

docs/design/auto-classifier-unavailable-fallback.md:70  "- Explicit permission denies and deterministic destructive-command blocks remain errors."
docs/users/features/auto-mode.md:224                    "After **3 consecutive policy blocks** the next tool call falls back to the standard manual-approval prompt."
P2 round1 preparedFallback={"fallback":false} via=blocked:destructive-command
 outcome.kind=fallback reason=consecutive_block
 message="Auto mode reached its consecutive denial limit on this action (Blocked destructive git command: \"git reset --hard\". ...). Review it manually."

Two corrections in your favour, both measured: :20 of the design doc is not falsified — the guard still runs and still hard-blocks below the caps — and auto-mode.md never documented the 20-denial cap (grep -n "20\b" there matches only :148, about environment entries), so :224 alone is the stale user-doc sentence. Scoping the design-doc bullet would close it, e.g. "Explicit permission denies remain errors. Deterministic destructive-command blocks remain errors until denial tracking reaches a cap; at the consecutive-block or session-total cap they fall back to manual approval like classifier blocks (see applyAutoModeDecision).", plus the same rule in docs/users/features/auto-mode.md's "Fallback to manual approval" section.

docs/design/auto-classifier-unavailable-fallback.md has no .zh-CN.md counterpart (measured: a glob over docs/design returns the single English file). That gap is pre-existing and this diff triggers nothing, but AGENTS.md requires linked English and Chinese versions kept synchronised in the same change when an existing design is updated — so fixing :70 by editing the design doc brings that rule into play. docs/users/features/ holds no .zh-CN.md files at all, so auto-mode.md is English-only by convention.

docs/design/auto-classifier-unavailable-fallback.md:70  "- Explicit permission denies and deterministic destructive-command blocks remain errors."
docs/users/features/auto-mode.md:224                    "After **3 consecutive policy blocks** the next tool call falls back to the standard manual-approval prompt."
P2 round1 preparedFallback={"fallback":false} via=blocked:destructive-command
 outcome.kind=fallback reason=consecutive_block
 message="Auto mode reached its consecutive denial limit on this action (Blocked destructive git command: \"git reset --hard\". ...). Review it manually."

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

Partially fixed in 2add596. The design-doc half is coupled to the ruling requested on the shouldFallback(blockedState) thread, so I am leaving this unresolved rather than pre-empting it.

Delivered — docs/users/features/auto-mode.md, "Fallback to manual approval": the bullet no longer attributes the cap to "3 consecutive policy blocks". It now reads "After 3 consecutive blocks — classifier policy blocks and deterministic destructive-command blocks both count — the call that reaches the third block falls back to the standard manual-approval prompt." Two deliberate differences from your suggested wording:

• It says the call that reaches the third block falls back, not "the next tool call". shouldFallback is evaluated on the post-increment state (autoMode.ts:589-592), so the old phrasing overstated the number of hard blocks by one. denialTracking.ts's header already states the same-call rule ("Calls that reach a consecutive threshold or the total denial cap also fall back on that same call"), and both cap tests in autoMode.test.ts assert it.
• The 20-denial cap stays undocumented in that file, matching your measurement that it never was documented there.

Not delivered — docs/design/auto-classifier-unavailable-fallback.md:70. Two reasons:

1. The correct replacement sentence is the open decision on the other thread. Under "restrict this branch to consecutive_block" the bullet must say destructive blocks fall back only at the consecutive cap; under "keep the shared cap" it must additionally name the outage-armed case. Writing either now pre-empts that ruling and would be rewritten by whichever commit implements it.
2. That file has no .zh-CN.md twin, and AGENTS.md's design-doc rule requires both versions "complete and synchronized in the same change, including when updating an existing design". So a one-line truth fix there carries a ~90-line translated file plus reciprocal language links — disproportionate next to a 16-line production change, and as you measured the gap is pre-existing rather than caused by this diff.

Both belong in the commit that implements the ruling.

One correction on the third statement you cite. autoMode.ts:841-843 ("Regex-based hard blocks ... so API failures or classifier misjudgment cannot allow destructive git/IaC commands through") is still true at this head and needs no edit. The escalation ends in a manual-approval confirmation, and decorateAutoModeFallbackConfirmation sets hideAlwaysAllow: true for every confirmation except ask_user_question (autoMode.ts:756-758). That flag is enforced rather than advisory: packages/cli/src/acp-integration/session/permissionUtils.ts:71-76 filters the visible options, packages/cli/src/serve/server/error-response.ts:203-221 answers a forged suppressed option with 400 invalid_option_id, and the TUI gates its always-allow buttons on the same flag (ToolConfirmationMessage.tsx:69-70, 291, 418). So there is no proceed_always* route by which a destructive command executes without its own human decision, and "cannot allow ... through" still holds. destructive-commands.ts:183-184 ("hard blocks regardless of classifier availability") is the statement the outage-armed cap actually contradicts; it moves with the same ruling as :70.

Partially delivered again in 43fef6f2b6 — a third instance of the same falsification class, in a file this review did not name.

docs/users/features/approval-mode.md:272 (Safety guardrails → Loop guard) read "after three consecutive policy blocks, the next call also falls back to manual approval". Both halves go stale for the same reasons you measured on auto-mode.md:224: deterministic destructive-command blocks now call recordBlock too, and shouldFallback is evaluated on the post-increment state (autoMode.ts:590), so it is the call that reaches the third block that falls back. It now mirrors the auto-mode.md bullet delivered in 2add596941, so the two user docs stop contradicting each other — approval-mode.md links to auto-mode.md eleven lines above.

Docs-only and ruling-neutral: under "restrict the route to consecutive_block" this sentence stays true as written, because recordBlock is shared by both block kinds and only the total-cap route would go away.

Still not delivered — docs/design/auto-classifier-unavailable-fallback.md:70. Re-verified at 43fef6f2b6, both blockers stand:

1. The correct replacement sentence is the open pick on the shouldFallback(blockedState) thread. Under "restrict to consecutive_block" the bullet must say destructive blocks fall back only at the consecutive cap; under "keep the shared cap" it must additionally name the outage-armed case. Anything ruling-neutral would be vaguer than either and gets rewritten by whichever commit implements it.
2. docs/design/README.md:36-42 — "When updating an existing single-language design, add the missing version and align the complete pair." That file has no .zh-CN.md twin (find docs -iname "*auto-classifier*" returns exactly one path), so a one-line truth fix carries a new translated design doc plus reciprocal language links.

No new ask is being posted: the binary decision already sits on the R1-1 thread and I am not pre-empting it.

Evidence — N/A for CLI/TUI/WebUI/wire surfaces: this delta changes documentation text only and touches no runtime path (production code is byte-identical to fd7d48aada; git diff fd7d48aada..43fef6f2b6 --stat = 1 file, docs/users/features/approval-mode.md). The observable artifact is the rendered bullet:

BEFORE  - **Loop guard**: after three consecutive policy blocks, the next call
          also falls back to manual approval so the agent isn't stuck cycling on
          a dead-end approach.
AFTER   - **Loop guard**: after three consecutive blocks — classifier policy blocks
          and deterministic destructive-command blocks both count — the call that
          reaches the third block also falls back to manual approval so the agent
          isn't stuck cycling on a dead-end approach.

Checks: npx prettier --check docs/users/features/approval-mode.md clean; npx vitest run src/permissions/autoMode.test.ts in packages/core = 107 passed / 107 (no test pins doc text, and none should — there is no behavior delta to mutate). Full pre-push review of 9cf2488f52...43fef6f2b6 (correctness + security dimensions) returned no must-fix findings.

Leaving this thread unresolved: the item you named is still open.