Blanket auto-deny for unapproved commands (hands-free mode) - #1760
Conversation
# Conflicts: # apps/vscode-e2e/src/suite/restart-persistence.test.ts
getSystemPrompt read provider state twice: once for the MCP gate and again after the hub wait. When the caller threaded no state, the two reads could observe different snapshots. Hoist the fallback resolution to the top of the call so the prompt and the tool guidance share one snapshot on the unthreaded path. Type the test harness getSystemPrompt signature with ProviderState and ModelInfo instead of unknown, and align the affected test title and comments with the single-read behavior.
The provider state read can resolve to nothing even while the provider reference stays alive. Add a test for that case so the system prompt call keeps receiving undefined disabledTools instead of failing.
Request construction re-read model metadata twice after the streaming turn's bounded fetch; thread the captured snapshot through attemptApiRequest so prompt assembly, context sizing, and tool arrays agree on a single view, resolving the fallback only when no snapshot was supplied. A cancellation that lands during a request's waits now stops the request before any tool array, abort controller, or provider call is issued for it.
The empty-response retry test now asserts that the retry iteration reaches attemptApiRequest with its own incremented attempt count (second call, retryAttempt 1), instead of only checking the resulting conversation history.
…prompt build getSystemPrompt no longer falls back to re-reading provider state; the provider-state snapshot parameter is now required. An explicit undefined declares that the caller's own read came back empty because the provider was already gone, and the prompt then resolves from defaults. The prompt and the request's runtime tool array now resolve from a single snapshot by construction rather than by caller convention. Behavior is unchanged on all reachable paths. Task.spec.ts grows from 128 to 129 tests to cover the required-parameter contract.
The bounded metadata waits in Task.safeEnsureModelFetched and the system prompt preview cleared their timer but left the handler-side promise waiting on the model-catalog fetch. The ApiHandler contract now threads an optional AbortSignal through ensureModelFetched(): RouterProvider settles the waiter with a rejection when the signal aborts, so an abandoned or cancelled caller detaches instead of parking a promise on the shared fetch (which keeps running for other waiters and still populates the cache, by design). The task aborts its waiter both when the 5s bound expires and when cancelCurrentRequest runs (cancel and dispose paths); the preview aborts at its bound and on completion. The task-lifecycle doc's table padding was also reconciled with the PR base: the remaining diff there is now only prettier's column re-padding, which the repo's own pre-commit formatter enforces.
…napshot Mutation-diff gate kills (PR Zoo-Code-Org#1505): - zoo-gateway: signal-aware ensureModelFetched tests for the fetch-wins and fetch-rejects branches (block/CallExpression NoCoverage), an addEventListener spy pinning the { once: true } options, and paired add/remove listener assertions pinning the abort event name on both detach sites (StringLiteral mutants). - Task: ownership-guard tests for metadataFetchAbortController (clear on own completion, leave a replaced controller in place). - generateSystemPrompt: signal-capture tests pinning the timeout-bound and finally-block controller.abort() detaches (CallExpression mutants). CodeRabbit: thread the request model-info snapshot into buildCleanConversationHistory so preserveReasoning resolves from the same per-request snapshot as the prompt and tool arrays, plus regression tests. No Stryker-disable directives were needed; all 14 mutants are killed behaviorally.
github-actions
Bot
removed
the
awaiting-author
github-actions
Bot
added
the
awaiting-author
…amp boundaries fe30882 pushed the PR diff's extension-package executable line count to 503, tripping the mutation gate's MAX_CHANGED_LINES=500 hard failure (Stryker never ran). CI mutation testing is advisory for Survived/NoCoverage mutants, so the regression is the scope cap itself, not surviving mutants. - stryker-diff: exclude src/scripts/merge-lcov.mjs via the existing excludedPaths convention (CI build/coverage tooling, same class as src/esbuild.mjs; covered by src/scripts/__tests__/merge-lcov.spec.mjs), bringing the extension scope back to 497 lines; pin the exclusion in the packageForPath manifest test. - merge-lcov.spec: pin parseBranchCount boundaries (-1 clamps, 0 stays, positive passes through, union prefers the positive lane) and keep non-integer BRDA rejection pinned.
github-actions
Bot
added
awaiting-author
edelauna
left a comment
edelauna
left a comment
There was a problem hiding this comment.
Had 2 comments - feel free to ping me in discord to re-review
| } | ||
|
|
||
| // Require explicit user approval for dangerous patterns | ||
| if (containsDangerousSubstitution(command)) { |
There was a problem hiding this comment.
Under blanket deny this allowlist check is the only gate, and the execute-time and queue re-checks call the same checkAutoApproval. Should containsDangerousSubstitution (or parseCommand) also cover $(...) and backticks inside double quotes and unquoted heredocs, and |&? With allowlist cat,echo, echo "$(rm x)" and echo a |& rm x both come back auto_approve from getCommandDecisionDetailed. The PR body says shell expansions are auto-denied. Is this meant to be covered here or in a follow-up?
There was a problem hiding this comment.
This is definitely something for a follow-up and should not be covered in this pr, as the behaviour is pre-existing on main, the pr is at a critical size and the underlying issue is much larger than what you have described.
The underlying issue is not only being able to correctly separate the individual commands, it is also that the mechanism that checks a single command allows, to put it mildly, surprising behaviour. The current mechanism is a string-match of the first part of the command, all arbitrary extensions of the command are allowed. No matter what is on the allowlist currently, there is almost always a way to create unintended destructive behaviour with it.
Simple examples:
- allowing "rm logs/logfile.txt" also allows "rm logs/logfile.txt AGENTS.md plans/*"
- allowing "git push" also allows "git push --force"
etc.
I have not had the time to write a proper issue for that, but this is definitely on my agenda to tackle next, together with the ability to create allowlists per project, in addition to globally (technically you can already create a project-level allowlist but it is ignored for execution. It is not ignored when you open the allowlist setting, then it is loaded and merged with the global list, which is also surprising behaviour).
There was a problem hiding this comment.
You were right though that the pr body was misleading, I just changed that.
| }), | ||
| ) | ||
| return | ||
| } else if (recheck.decision !== "approve") { |
There was a problem hiding this comment.
Is there a test where blanket deny stays engaged on both state reads, the command is allowlisted, and the re-check returns approve? The tests I found for this re-check all end in a deny or a non-approval. A change that makes this branch also reject approve looks like it would pass them. That would stop hands-free mode from running any allowlisted command.
There was a problem hiding this comment.
uh, good catch, added the test in commit 9f901f5
github-actions
Bot
added
awaiting-author
edelauna
left a comment
edelauna
left a comment
There was a problem hiding this comment.
Looks good - one last request, can you add a snapshot of the setting menu with this new setting?
I thought I already did this in commit 984ddfc or do you mean something else? |
edelauna
left a comment
edelauna
left a comment
There was a problem hiding this comment.
Thanks for this contribution.
|
@CodeRabbit approve |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
✅ Action performedComments resolved and changes approved. |
github-actions
Bot
added
awaiting-maintainer
…7-process-queued-messages-on-terminal-finish Union with Zoo-Code-Org#1760 (blanket auto-deny for unapproved commands), which overlaps the queued-message approval surface: - queuedResponseForAsk restores upstream's claim-path semantics for command/use_mcp_server/tool asks (a queued message may answer them via the policy-gated claim path), while keeping this branch's failure-gate exclusion (api_req_failed, auto_approval_max_req_reached) so typed text is never destroyed by a retry prompt that aborts the task. - The drain path keeps this branch's invariant: a raw conversational submission never posts into an approval ask (it cannot be an approval and would deny-with-feedback); approval conversion stays exclusive to the policy-gated claim path (Zoo-Code-Org#1760's blanket-deny latch and drain-site re-check run unchanged). - The in-ask auto-claim computes the resolution BEFORE claiming: this branch's failure-gate exclusions resolve to undefined, and the claim-then-resolve order leaked the claim and stalled later asks. - ask()/askImpl carries upstream's autoApprovalContext and autoDenyDetail plumbing; both askApproval copies destructure and handle both queuedMessageId and autoDenyDetail. - handleQueuedAskResponse tolerates an uninitialized queuedFeedbackRows (Object.create-based tests) for the registered-row handback. - Spec unions: executeCommandTool keeps both mock sets; upstream's ask-auto-deny/presentAssistantMessage-auto-deny fixtures gain sayUserFeedbackAndAckQueued / boolean-return stubs matching the merged Task API; this branch's superseded refusal pins are rewritten to the union semantics (failure gates keep refusing, approval asks go through the policy-gated claim path). Full core suite green: 80 files, 1475 passed / 5 skipped / 0 failed. check-types and eslint clean.
3 participants
Closes: #1569
Adds an opt-in blanket auto-deny for terminal commands: a new
alwaysDenyUnapprovedCommandssetting, default off. When enabled — and command auto-approval is already on — every command that is not explicitly allowlisted is denied automatically instead of prompting, and the model is told exactly why in a form it can act on, so hands-free sessions keep moving without silently widening what can run.What changes
formatResponse.toolAutoDenied): the reason, the offending sub-command, the Destructive Command Guard rule id when applicable, a note that the chain was rejected in its entirety and nothing in it executed, and a suggestion to re-run the remaining parts as separate approved commands. It never suggests asking the user.ExtensionState, SettingsView checkbox (Auto-approve > Execute, visible in both guard modes) through the buffered-edit/Save flow, genericupdateSettingspersistence, settings import/export. Strings in all 18 webview locales; non-English carry English placeholders, per this repo's convention for fresh keys.Merge safety
autoApprovalEnabled+alwaysAllowExecute); with it off, the only behavior change is the denylist payload noted above.getCommandDecisionnow delegates togetCommandDecisionDetailedwith identical decision logic; the change adds attribution detail for denial messages, not new decision rules.9f901f59f(21, including platform unit tests on ubuntu and windows,e2e-mock,mutation-diff, the visual suites, and translation parity).Scope note: hardening the allowlist parser itself (shell expansions and chaining forms the current matcher does not flag) is not part of this PR — follow-up territory.
User-facing docs live in Zoo-Code-Docs; no docs changes are included in this PR.