[Feat] Integrate destructive command guard with auto-approval - #1050
Draft
navedmerchant wants to merge 13 commits into
Draft
[Feat] Integrate destructive command guard with auto-approval#1050navedmerchant wants to merge 13 commits into
navedmerchant wants to merge 13 commits into
Conversation
navedmerchant
requested review from
JamesRobert20,
edelauna,
hannesrudolph and
taltas
as code owners
July 29, 2026 06:17
Contributor
📝 WalkthroughWalkthroughAdds Destructive Command Guard support for command auto-approval, including persisted settings, secure binary installation, guarded execution, approval-state recording, denied-command rendering, tests, and localized UI text. ChangesDestructive Command Guard integration
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant SettingsView
participant webviewMessageHandler
participant DCGManager
participant ExecuteCommandTool
participant Task
participant ChatRow
SettingsView->>webviewMessageHandler: updateSettings(destructiveCommandGuardEnabled)
webviewMessageHandler->>DCGManager: ensureDcgInstalled(storageDir)
ExecuteCommandTool->>DCGManager: runDcg(binaryPath, command, cwd)
DCGManager-->>ExecuteCommandTool: allow or deny decision
ExecuteCommandTool->>Task: request approval with protection state
Task-->>ChatRow: autoApprovalDecision
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (8)
src/core/webview/ClineProvider.ts (1)
2443-2443: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse a shared DCG default constant.
Both state builders hard-code
false. Define one shared default alongside the setting schema and use it ingetState()andgetStateToPostToWebview()to keep default semantics consistent.Also applies to: 2676-2676
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/webview/ClineProvider.ts` at line 2443, Define a shared default constant for destructive command guard behavior alongside the setting schema, then replace the hard-coded false fallbacks in both getState() and getStateToPostToWebview() with that constant. Preserve the existing nullish-coalescing behavior.Source: Coding guidelines
packages/types/src/message.ts (1)
275-275: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd focused schema tests for
autoApprovalDecision.Cover both accepted values (
"approve"/"deny") and rejection of invalid values inpackages/types/src/__tests__/message.test.ts.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/types/src/message.ts` at line 275, Add focused tests in the message schema test suite for autoApprovalDecision, asserting that "approve" and "deny" are accepted while invalid values are rejected. Reuse the existing message schema test patterns and reference the autoApprovalDecision field directly.Source: Coding guidelines
webview-ui/src/components/chat/CommandExecution.tsx (1)
39-45: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueNew
isDeniedprop shadows the localisDeniedinhandleDenyPatternChange(Line 112).Harmless today, but the inner
const isDenied = deniedCommands.includes(pattern)now hides the prop of the same name. Renaming the local (e.g.isPatternDenied) removes the ambiguity.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@webview-ui/src/components/chat/CommandExecution.tsx` around lines 39 - 45, Rename the local isDenied variable inside handleDenyPatternChange to a distinct name such as isPatternDenied, and update its references there while preserving the isDenied prop used by CommandExecution.src/core/auto-approval/__tests__/dcg.spec.ts (1)
40-62: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider covering
alwaysAllowExecute: falsewith DCG enabled.The suite never asserts that DCG alone cannot auto-approve when the execute toggle is off — the most important negative boundary of this new branch.
♻️ Suggested additional case
it("does not auto-approve via DCG when execute auto-approval is off", async () => { const state = { ...baseState, alwaysAllowExecute: false } expect(await checkAutoApproval({ state, ask: "command", text: "echo safe" })).toEqual({ decision: "ask" }) })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/auto-approval/__tests__/dcg.spec.ts` around lines 40 - 62, Add a test alongside the existing checkAutoApproval cases that keeps destructiveCommandGuardEnabled enabled while setting baseState.alwaysAllowExecute to false, then verify a safe command returns decision "ask" rather than being auto-approved via DCG.src/core/tools/__tests__/executeCommandTool.spec.ts (1)
210-248: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd the allow-path and unavailable-binary cases.
Currently only the deny branch is exercised. Two cheap additions with real value: DCG enabled +
decision: "allow"should callaskApproval("command", "echo test")with no protection flag, andgetDcgBinaryPathreturningundefinedshould surface the enablement error viahandleErrorrather than silently executing.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/tools/__tests__/executeCommandTool.spec.ts` around lines 210 - 248, Extend the executeCommandTool tests around the existing DCG deny-case test to cover an enabled guard returning decision "allow", asserting askApproval is called with "command", "echo test", and no protection flag. Add an unavailable-binary case by making getDcgBinaryPath return undefined, and assert handleError receives the DCG enablement error without executing the command.src/services/destructive-command-guard/constants.ts (1)
9-40: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider deep-freezing archive metadata.
Readonly<Record<string, DcgArchiveInfo>>only prevents reassigning top-level keys; nested fields likesha256on eachDcgArchiveInforemain mutable at runtime. Since these values pin the trusted checksum used for supply-chain verification, marking the literalas const(and typing accordingly) would harden against accidental mutation elsewhere in the codebase.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/services/destructive-command-guard/constants.ts` around lines 9 - 40, Update DCG_ARCHIVES to use a deeply immutable literal by applying `as const` and adjusting its type so each archive entry’s `archive`, `binary`, and `sha256` fields cannot be reassigned. Preserve the existing platform entries and checksum values while ensuring nested metadata is read-only at compile time.src/services/destructive-command-guard/manager.ts (1)
51-160: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftAdd unit test coverage for the checksum/download/install security path.
manager.spec.tsonly exercises the pure platform-mapping helpers (getDcgArchiveInfo,isDcgSupportedPlatform,getDcgBinaryPath).downloadFile's trusted-domain/redirect/size enforcement,verifyChecksum,extractSingleBinary's layout validation, andinstallDcg's atomic swap are the actual security guarantees of this module and remain untested. These are mockable viahttps/fs/child_processmodule mocks.Based on learnings, "Prefer the narrowest test layer that proves behavior: focused tests first, integration tests for cross-module contracts, and end-to-end tests only for full workflow confidence."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/services/destructive-command-guard/manager.ts` around lines 51 - 160, Add focused unit tests in manager.spec.ts for downloadFile’s trusted-host checks, redirect limits, and download-size enforcement; verifyChecksum’s success and mismatch behavior; extractSingleBinary’s single-entry layout validation; and installDcg’s atomic swap behavior. Mock https, fs streams, and child_process.runProcess dependencies to exercise these paths without real network, archive, or process operations, while preserving the existing platform-helper tests.Source: Learnings
src/services/destructive-command-guard/runner.ts (1)
16-84: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftNo test coverage for
runDcg's branch logic.This function gates command auto-approval and has several distinct failure branches (timeout, output-size overflow, invalid JSON, unsupported schema version, decision/exit-code mismatch) that aren't covered by any test in this cohort (
manager.spec.tsonly tests unrelated helpers). Given this is on the safety-critical approval path, focused unit tests mockingchild_process.spawnwould materially increase confidence.Based on learnings, "For regressions, add the test at the lowest layer that would have failed; add an e2e test only when lower-level tests cannot represent the failure mode."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/services/destructive-command-guard/runner.ts` around lines 16 - 84, Add focused unit tests for runDcg by mocking child_process.spawn and simulating its stdout, stderr, close, and error events. Cover successful allow/deny decisions plus timeout, output-size overflow, invalid JSON, unsupported schema versions, decision/exit-code mismatches, and nonzero termination failures, asserting the resulting resolution or rejection messages.Source: Learnings
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/core/tools/ExecuteCommandTool.ts`:
- Around line 128-144: Replace the hardcoded missing-binary error in the
destructive-command guard block of ExecuteCommandTool with the localized
common.errors.destructiveCommandGuard.unavailable message. Add the unavailable
key to that error template in common.json for every locale, preserving the
existing destructive_command_guard_enable_failed template and using the
established localization lookup pattern.
In `@src/services/destructive-command-guard/manager.ts`:
- Around line 162-212: Prevent cross-process installation races in installDcg:
before the destructive fs.rm(finalDir) and fs.rename(stagingDir, finalDir)
replacement, re-check whether the verified binaryPath already exists and return
it while cleaning up staging, or implement an equivalent lightweight lock under
installRoot. Preserve checksum, extraction, and version verification, and ensure
concurrent windows never remove an already valid installation.
- Around line 145-160: Update extractSingleBinary to use a Windows-safe native,
PowerShell, or Node-based ZIP extraction path for .zip archives instead of
invoking runProcess("tar", ...). Preserve the existing tar listing and
extraction behavior for non-ZIP archives, and continue validating that the
archive contains only info.binary before extraction.
In `@webview-ui/src/components/chat/ChatRow.tsx`:
- Around line 294-301: Add stable React keys to both JSX elements returned by
the autoApprovalDecision "deny" branch in the surrounding message-rendering
function, resolving the react/jsx-key lint errors without changing the
denied-state content or styling.
In `@webview-ui/src/i18n/locales/de/settings.json`:
- Around line 331-334: Update the description in the destructiveCommandGuard
localization entry to replace “Zoos Befehlslisten” with the clearer German
wording “Die Befehlslisten von Zoo sind währenddessen deaktiviert.”
In `@webview-ui/src/i18n/locales/fr/settings.json`:
- Around line 332-335: Update the description in the destructiveCommandGuard
translation to replace the informal French pronouns “tu” and “ton” with the
formal “vous” and “votre”, preserving the existing meaning and wording
otherwise.
---
Nitpick comments:
In `@packages/types/src/message.ts`:
- Line 275: Add focused tests in the message schema test suite for
autoApprovalDecision, asserting that "approve" and "deny" are accepted while
invalid values are rejected. Reuse the existing message schema test patterns and
reference the autoApprovalDecision field directly.
In `@src/core/auto-approval/__tests__/dcg.spec.ts`:
- Around line 40-62: Add a test alongside the existing checkAutoApproval cases
that keeps destructiveCommandGuardEnabled enabled while setting
baseState.alwaysAllowExecute to false, then verify a safe command returns
decision "ask" rather than being auto-approved via DCG.
In `@src/core/tools/__tests__/executeCommandTool.spec.ts`:
- Around line 210-248: Extend the executeCommandTool tests around the existing
DCG deny-case test to cover an enabled guard returning decision "allow",
asserting askApproval is called with "command", "echo test", and no protection
flag. Add an unavailable-binary case by making getDcgBinaryPath return
undefined, and assert handleError receives the DCG enablement error without
executing the command.
In `@src/core/webview/ClineProvider.ts`:
- Line 2443: Define a shared default constant for destructive command guard
behavior alongside the setting schema, then replace the hard-coded false
fallbacks in both getState() and getStateToPostToWebview() with that constant.
Preserve the existing nullish-coalescing behavior.
In `@src/services/destructive-command-guard/constants.ts`:
- Around line 9-40: Update DCG_ARCHIVES to use a deeply immutable literal by
applying `as const` and adjusting its type so each archive entry’s `archive`,
`binary`, and `sha256` fields cannot be reassigned. Preserve the existing
platform entries and checksum values while ensuring nested metadata is read-only
at compile time.
In `@src/services/destructive-command-guard/manager.ts`:
- Around line 51-160: Add focused unit tests in manager.spec.ts for
downloadFile’s trusted-host checks, redirect limits, and download-size
enforcement; verifyChecksum’s success and mismatch behavior;
extractSingleBinary’s single-entry layout validation; and installDcg’s atomic
swap behavior. Mock https, fs streams, and child_process.runProcess dependencies
to exercise these paths without real network, archive, or process operations,
while preserving the existing platform-helper tests.
In `@src/services/destructive-command-guard/runner.ts`:
- Around line 16-84: Add focused unit tests for runDcg by mocking
child_process.spawn and simulating its stdout, stderr, close, and error events.
Cover successful allow/deny decisions plus timeout, output-size overflow,
invalid JSON, unsupported schema versions, decision/exit-code mismatches, and
nonzero termination failures, asserting the resulting resolution or rejection
messages.
In `@webview-ui/src/components/chat/CommandExecution.tsx`:
- Around line 39-45: Rename the local isDenied variable inside
handleDenyPatternChange to a distinct name such as isPatternDenied, and update
its references there while preserving the isDenied prop used by
CommandExecution.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 7668a3bc-553d-4f41-8d64-ff82c2563941
📒 Files selected for processing (96)
AGENTS.mdpackages/types/src/global-settings.tspackages/types/src/message.tspackages/types/src/vscode-extension-host.tssrc/core/auto-approval/__tests__/dcg.spec.tssrc/core/auto-approval/index.tssrc/core/task/Task.tssrc/core/tools/ExecuteCommandTool.tssrc/core/tools/__tests__/executeCommandTool.spec.tssrc/core/webview/ClineProvider.tssrc/core/webview/__tests__/ClineProvider.spec.tssrc/core/webview/webviewMessageHandler.tssrc/i18n/locales/ca/common.jsonsrc/i18n/locales/ca/tools.jsonsrc/i18n/locales/de/common.jsonsrc/i18n/locales/de/tools.jsonsrc/i18n/locales/en/common.jsonsrc/i18n/locales/en/tools.jsonsrc/i18n/locales/es/common.jsonsrc/i18n/locales/es/tools.jsonsrc/i18n/locales/fr/common.jsonsrc/i18n/locales/fr/tools.jsonsrc/i18n/locales/hi/common.jsonsrc/i18n/locales/hi/tools.jsonsrc/i18n/locales/id/common.jsonsrc/i18n/locales/id/tools.jsonsrc/i18n/locales/it/common.jsonsrc/i18n/locales/it/tools.jsonsrc/i18n/locales/ja/common.jsonsrc/i18n/locales/ja/tools.jsonsrc/i18n/locales/ko/common.jsonsrc/i18n/locales/ko/tools.jsonsrc/i18n/locales/nl/common.jsonsrc/i18n/locales/nl/tools.jsonsrc/i18n/locales/pl/common.jsonsrc/i18n/locales/pl/tools.jsonsrc/i18n/locales/pt-BR/common.jsonsrc/i18n/locales/pt-BR/tools.jsonsrc/i18n/locales/ru/common.jsonsrc/i18n/locales/ru/tools.jsonsrc/i18n/locales/tr/common.jsonsrc/i18n/locales/tr/tools.jsonsrc/i18n/locales/vi/common.jsonsrc/i18n/locales/vi/tools.jsonsrc/i18n/locales/zh-CN/common.jsonsrc/i18n/locales/zh-CN/tools.jsonsrc/i18n/locales/zh-TW/common.jsonsrc/i18n/locales/zh-TW/tools.jsonsrc/services/destructive-command-guard/__tests__/manager.spec.tssrc/services/destructive-command-guard/constants.tssrc/services/destructive-command-guard/index.tssrc/services/destructive-command-guard/manager.tssrc/services/destructive-command-guard/runner.tswebview-ui/src/components/chat/ChatRow.tsxwebview-ui/src/components/chat/CommandExecution.tsxwebview-ui/src/components/chat/__tests__/ChatRow.command-denied.spec.tsxwebview-ui/src/components/chat/__tests__/CommandExecution.spec.tsxwebview-ui/src/components/settings/AutoApproveSettings.tsxwebview-ui/src/components/settings/SettingsView.tsxwebview-ui/src/components/settings/__tests__/AutoApproveSettings.spec.tsxwebview-ui/src/i18n/locales/ca/chat.jsonwebview-ui/src/i18n/locales/ca/settings.jsonwebview-ui/src/i18n/locales/de/chat.jsonwebview-ui/src/i18n/locales/de/settings.jsonwebview-ui/src/i18n/locales/en/chat.jsonwebview-ui/src/i18n/locales/en/settings.jsonwebview-ui/src/i18n/locales/es/chat.jsonwebview-ui/src/i18n/locales/es/settings.jsonwebview-ui/src/i18n/locales/fr/chat.jsonwebview-ui/src/i18n/locales/fr/settings.jsonwebview-ui/src/i18n/locales/hi/chat.jsonwebview-ui/src/i18n/locales/hi/settings.jsonwebview-ui/src/i18n/locales/id/chat.jsonwebview-ui/src/i18n/locales/id/settings.jsonwebview-ui/src/i18n/locales/it/chat.jsonwebview-ui/src/i18n/locales/it/settings.jsonwebview-ui/src/i18n/locales/ja/chat.jsonwebview-ui/src/i18n/locales/ja/settings.jsonwebview-ui/src/i18n/locales/ko/chat.jsonwebview-ui/src/i18n/locales/ko/settings.jsonwebview-ui/src/i18n/locales/nl/chat.jsonwebview-ui/src/i18n/locales/nl/settings.jsonwebview-ui/src/i18n/locales/pl/chat.jsonwebview-ui/src/i18n/locales/pl/settings.jsonwebview-ui/src/i18n/locales/pt-BR/chat.jsonwebview-ui/src/i18n/locales/pt-BR/settings.jsonwebview-ui/src/i18n/locales/ru/chat.jsonwebview-ui/src/i18n/locales/ru/settings.jsonwebview-ui/src/i18n/locales/tr/chat.jsonwebview-ui/src/i18n/locales/tr/settings.jsonwebview-ui/src/i18n/locales/vi/chat.jsonwebview-ui/src/i18n/locales/vi/settings.jsonwebview-ui/src/i18n/locales/zh-CN/chat.jsonwebview-ui/src/i18n/locales/zh-CN/settings.jsonwebview-ui/src/i18n/locales/zh-TW/chat.jsonwebview-ui/src/i18n/locales/zh-TW/settings.json
navedmerchant
force-pushed
the
feat/autoapprovecommands
branch
from
July 29, 2026 06:34
d7b7b5b to
bce10d6
Compare
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/core/webview/webviewMessageHandler.ts (1)
577-577: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winWrap
casebodies in blocks to fixno-case-declarations.
const mcpHub = ...(577) andconst vsCodeLmModels = ...(1383) arelet/constdeclarations directly insidecaseclauses without a block, which both ESLint and Biome flag — the declaration is otherwise visible to siblingcaseclauses in the same switch.🐛 Proposed fix
- case "webviewDidLaunch": + case "webviewDidLaunch": { // Load custom modes first ... - provider.isViewLaunched = true - break + provider.isViewLaunched = true + break + }- case "requestVsCodeLmModels": - const vsCodeLmModels = await getVsCodeLmModels() - // TODO: Cache like we do for OpenRouter, etc? - await provider.postMessageToWebview({ type: "vsCodeLmModels", vsCodeLmModels }) - break + case "requestVsCodeLmModels": { + const vsCodeLmModels = await getVsCodeLmModels() + // TODO: Cache like we do for OpenRouter, etc? + await provider.postMessageToWebview({ type: "vsCodeLmModels", vsCodeLmModels }) + break + }As per coding guidelines, "Fix lint violations in new TypeScript code instead of adding suppressions."
Also applies to: 1382-1386
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/webview/webviewMessageHandler.ts` at line 577, Wrap the affected switch case bodies in explicit blocks to satisfy no-case-declarations. Update the case containing provider.getMcpHub() and the case containing the vsCodeLmModels declaration, keeping each case’s existing logic and control flow unchanged while scoping its const declarations to that case.Sources: Coding guidelines, Linters/SAST tools
src/core/webview/__tests__/ClineProvider.spec.ts (1)
847-847: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winReplace
as anywith the typed pattern already used above.Line 826 in this same file types the identical expression as
ReturnType<typeof vi.fn>; line 847 usesas anyinstead, which ESLint flags (no-explicit-any).🐛 Proposed fix
- const messageHandler = (mockWebviewView.webview.onDidReceiveMessage as any).mock.calls[0][0] + const messageHandler = (mockWebviewView.webview.onDidReceiveMessage as ReturnType<typeof vi.fn>).mock + .calls[0][0]As per coding guidelines, "Avoid
as any; use typed APIs, bracket notation for private members, or precise test doubles and type guards."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/webview/__tests__/ClineProvider.spec.ts` at line 847, Replace the `as any` cast in the `messageHandler` assignment with the existing `ReturnType<typeof vi.fn>` typed pattern used for the identical `onDidReceiveMessage` expression above, preserving the current test behavior and satisfying the no-explicit-any rule.Sources: Coding guidelines, Linters/SAST tools
♻️ Duplicate comments (1)
webview-ui/src/components/chat/ChatRow.tsx (1)
294-301: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winMissing
keyprops on the denied-command JSX array — previously flagged, still unresolved.ESLint still reports
react/jsx-keyon both elements returned from this branch.🐛 Proposed fix
if (message.autoApprovalDecision === "deny") { return [ - <OctagonX className="size-4 text-vscode-errorForeground" aria-label="Denied command icon" />, - <span className="font-bold text-vscode-errorForeground"> + <OctagonX + key="denied-icon" + className="size-4 text-vscode-errorForeground" + aria-label="Denied command icon" + />, + <span key="denied-title" className="font-bold text-vscode-errorForeground"> {t("chat:commandExecution.denied")} </span>, ] }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@webview-ui/src/components/chat/ChatRow.tsx` around lines 294 - 301, Add stable key props to both JSX elements returned by the denied branch of the message rendering logic, specifically the OctagonX icon and the denied-command span. Keep their existing content and styling unchanged while ensuring each array element has a unique key.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/services/destructive-command-guard/manager.ts`:
- Around line 84-95: Update the oversized-download handling in the response data
listener to destroy the piped output write stream when received exceeds
DCG_MAX_ARCHIVE_BYTES, alongside destroying the request. Preserve the existing
size-limit error and ensure the output stream cannot remain open after aborting.
---
Outside diff comments:
In `@src/core/webview/__tests__/ClineProvider.spec.ts`:
- Line 847: Replace the `as any` cast in the `messageHandler` assignment with
the existing `ReturnType<typeof vi.fn>` typed pattern used for the identical
`onDidReceiveMessage` expression above, preserving the current test behavior and
satisfying the no-explicit-any rule.
In `@src/core/webview/webviewMessageHandler.ts`:
- Line 577: Wrap the affected switch case bodies in explicit blocks to satisfy
no-case-declarations. Update the case containing provider.getMcpHub() and the
case containing the vsCodeLmModels declaration, keeping each case’s existing
logic and control flow unchanged while scoping its const declarations to that
case.
---
Duplicate comments:
In `@webview-ui/src/components/chat/ChatRow.tsx`:
- Around line 294-301: Add stable key props to both JSX elements returned by the
denied branch of the message rendering logic, specifically the OctagonX icon and
the denied-command span. Keep their existing content and styling unchanged while
ensuring each array element has a unique key.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ae067f4b-724a-4c0d-b9b7-6cd367315dfd
📒 Files selected for processing (96)
AGENTS.mdpackages/types/src/global-settings.tspackages/types/src/message.tspackages/types/src/vscode-extension-host.tssrc/core/auto-approval/__tests__/dcg.spec.tssrc/core/auto-approval/index.tssrc/core/task/Task.tssrc/core/tools/ExecuteCommandTool.tssrc/core/tools/__tests__/executeCommandTool.spec.tssrc/core/webview/ClineProvider.tssrc/core/webview/__tests__/ClineProvider.spec.tssrc/core/webview/webviewMessageHandler.tssrc/i18n/locales/ca/common.jsonsrc/i18n/locales/ca/tools.jsonsrc/i18n/locales/de/common.jsonsrc/i18n/locales/de/tools.jsonsrc/i18n/locales/en/common.jsonsrc/i18n/locales/en/tools.jsonsrc/i18n/locales/es/common.jsonsrc/i18n/locales/es/tools.jsonsrc/i18n/locales/fr/common.jsonsrc/i18n/locales/fr/tools.jsonsrc/i18n/locales/hi/common.jsonsrc/i18n/locales/hi/tools.jsonsrc/i18n/locales/id/common.jsonsrc/i18n/locales/id/tools.jsonsrc/i18n/locales/it/common.jsonsrc/i18n/locales/it/tools.jsonsrc/i18n/locales/ja/common.jsonsrc/i18n/locales/ja/tools.jsonsrc/i18n/locales/ko/common.jsonsrc/i18n/locales/ko/tools.jsonsrc/i18n/locales/nl/common.jsonsrc/i18n/locales/nl/tools.jsonsrc/i18n/locales/pl/common.jsonsrc/i18n/locales/pl/tools.jsonsrc/i18n/locales/pt-BR/common.jsonsrc/i18n/locales/pt-BR/tools.jsonsrc/i18n/locales/ru/common.jsonsrc/i18n/locales/ru/tools.jsonsrc/i18n/locales/tr/common.jsonsrc/i18n/locales/tr/tools.jsonsrc/i18n/locales/vi/common.jsonsrc/i18n/locales/vi/tools.jsonsrc/i18n/locales/zh-CN/common.jsonsrc/i18n/locales/zh-CN/tools.jsonsrc/i18n/locales/zh-TW/common.jsonsrc/i18n/locales/zh-TW/tools.jsonsrc/services/destructive-command-guard/__tests__/manager.spec.tssrc/services/destructive-command-guard/constants.tssrc/services/destructive-command-guard/index.tssrc/services/destructive-command-guard/manager.tssrc/services/destructive-command-guard/runner.tswebview-ui/src/components/chat/ChatRow.tsxwebview-ui/src/components/chat/CommandExecution.tsxwebview-ui/src/components/chat/__tests__/ChatRow.command-denied.spec.tsxwebview-ui/src/components/chat/__tests__/CommandExecution.spec.tsxwebview-ui/src/components/settings/AutoApproveSettings.tsxwebview-ui/src/components/settings/SettingsView.tsxwebview-ui/src/components/settings/__tests__/AutoApproveSettings.spec.tsxwebview-ui/src/i18n/locales/ca/chat.jsonwebview-ui/src/i18n/locales/ca/settings.jsonwebview-ui/src/i18n/locales/de/chat.jsonwebview-ui/src/i18n/locales/de/settings.jsonwebview-ui/src/i18n/locales/en/chat.jsonwebview-ui/src/i18n/locales/en/settings.jsonwebview-ui/src/i18n/locales/es/chat.jsonwebview-ui/src/i18n/locales/es/settings.jsonwebview-ui/src/i18n/locales/fr/chat.jsonwebview-ui/src/i18n/locales/fr/settings.jsonwebview-ui/src/i18n/locales/hi/chat.jsonwebview-ui/src/i18n/locales/hi/settings.jsonwebview-ui/src/i18n/locales/id/chat.jsonwebview-ui/src/i18n/locales/id/settings.jsonwebview-ui/src/i18n/locales/it/chat.jsonwebview-ui/src/i18n/locales/it/settings.jsonwebview-ui/src/i18n/locales/ja/chat.jsonwebview-ui/src/i18n/locales/ja/settings.jsonwebview-ui/src/i18n/locales/ko/chat.jsonwebview-ui/src/i18n/locales/ko/settings.jsonwebview-ui/src/i18n/locales/nl/chat.jsonwebview-ui/src/i18n/locales/nl/settings.jsonwebview-ui/src/i18n/locales/pl/chat.jsonwebview-ui/src/i18n/locales/pl/settings.jsonwebview-ui/src/i18n/locales/pt-BR/chat.jsonwebview-ui/src/i18n/locales/pt-BR/settings.jsonwebview-ui/src/i18n/locales/ru/chat.jsonwebview-ui/src/i18n/locales/ru/settings.jsonwebview-ui/src/i18n/locales/tr/chat.jsonwebview-ui/src/i18n/locales/tr/settings.jsonwebview-ui/src/i18n/locales/vi/chat.jsonwebview-ui/src/i18n/locales/vi/settings.jsonwebview-ui/src/i18n/locales/zh-CN/chat.jsonwebview-ui/src/i18n/locales/zh-CN/settings.jsonwebview-ui/src/i18n/locales/zh-TW/chat.jsonwebview-ui/src/i18n/locales/zh-TW/settings.json
🚧 Files skipped from review as they are similar to previous changes (83)
- src/i18n/locales/hi/common.json
- src/i18n/locales/ru/common.json
- webview-ui/src/i18n/locales/tr/chat.json
- src/i18n/locales/es/common.json
- src/i18n/locales/en/tools.json
- packages/types/src/global-settings.ts
- webview-ui/src/i18n/locales/ja/chat.json
- src/i18n/locales/pt-BR/tools.json
- src/i18n/locales/ja/tools.json
- webview-ui/src/i18n/locales/fr/chat.json
- src/i18n/locales/pl/tools.json
- src/i18n/locales/vi/tools.json
- webview-ui/src/i18n/locales/ru/settings.json
- webview-ui/src/components/chat/tests/ChatRow.command-denied.spec.tsx
- src/i18n/locales/ca/tools.json
- webview-ui/src/i18n/locales/vi/chat.json
- src/i18n/locales/id/tools.json
- src/i18n/locales/ko/tools.json
- webview-ui/src/i18n/locales/id/chat.json
- webview-ui/src/i18n/locales/fr/settings.json
- src/services/destructive-command-guard/tests/manager.spec.ts
- webview-ui/src/i18n/locales/hi/chat.json
- src/i18n/locales/es/tools.json
- src/i18n/locales/ru/tools.json
- src/i18n/locales/nl/tools.json
- webview-ui/src/components/settings/tests/AutoApproveSettings.spec.tsx
- src/i18n/locales/zh-TW/tools.json
- src/i18n/locales/it/common.json
- src/services/destructive-command-guard/index.ts
- packages/types/src/message.ts
- src/i18n/locales/de/tools.json
- src/i18n/locales/pl/common.json
- packages/types/src/vscode-extension-host.ts
- src/i18n/locales/tr/tools.json
- webview-ui/src/i18n/locales/pt-BR/chat.json
- src/i18n/locales/zh-CN/tools.json
- src/i18n/locales/it/tools.json
- src/i18n/locales/ca/common.json
- webview-ui/src/i18n/locales/en/chat.json
- webview-ui/src/i18n/locales/zh-CN/chat.json
- webview-ui/src/i18n/locales/ja/settings.json
- webview-ui/src/i18n/locales/ru/chat.json
- webview-ui/src/i18n/locales/ko/chat.json
- webview-ui/src/i18n/locales/it/chat.json
- webview-ui/src/i18n/locales/es/chat.json
- src/i18n/locales/nl/common.json
- webview-ui/src/i18n/locales/de/chat.json
- webview-ui/src/i18n/locales/nl/chat.json
- webview-ui/src/i18n/locales/ca/chat.json
- webview-ui/src/i18n/locales/ko/settings.json
- webview-ui/src/i18n/locales/pl/settings.json
- src/i18n/locales/pt-BR/common.json
- webview-ui/src/i18n/locales/es/settings.json
- src/i18n/locales/zh-TW/common.json
- webview-ui/src/i18n/locales/tr/settings.json
- src/i18n/locales/de/common.json
- src/core/auto-approval/tests/dcg.spec.ts
- webview-ui/src/i18n/locales/it/settings.json
- webview-ui/src/i18n/locales/vi/settings.json
- webview-ui/src/i18n/locales/nl/settings.json
- src/i18n/locales/zh-CN/common.json
- webview-ui/src/i18n/locales/pl/chat.json
- src/core/task/Task.ts
- webview-ui/src/components/settings/SettingsView.tsx
- webview-ui/src/components/chat/tests/CommandExecution.spec.tsx
- webview-ui/src/i18n/locales/zh-TW/settings.json
- webview-ui/src/i18n/locales/id/settings.json
- src/i18n/locales/ko/common.json
- webview-ui/src/components/chat/CommandExecution.tsx
- webview-ui/src/i18n/locales/pt-BR/settings.json
- src/i18n/locales/hi/tools.json
- webview-ui/src/i18n/locales/hi/settings.json
- src/i18n/locales/tr/common.json
- src/core/webview/ClineProvider.ts
- src/core/tools/ExecuteCommandTool.ts
- webview-ui/src/i18n/locales/de/settings.json
- src/i18n/locales/fr/tools.json
- webview-ui/src/components/settings/AutoApproveSettings.tsx
- src/services/destructive-command-guard/constants.ts
- src/i18n/locales/ja/common.json
- src/core/tools/tests/executeCommandTool.spec.ts
- src/core/auto-approval/index.ts
- webview-ui/src/i18n/locales/ca/settings.json
github-actions
Bot
added
the
awaiting-review
navedmerchant
marked this pull request as draft
July 29, 2026 06:58
github-actions
Bot
removed
the
awaiting-review
Contributor
|
Done — reviewed all 7 unresolved review threads and pushed 2478d1f. Fixed in this commit:
Already addressed on the branch head (verified, threads resolved):
Validation: full |
- Adopt a concurrently completed installation instead of removing it when another VS Code window finishes first, preventing transient ENOENT for consumers mid-swap. - Destroy the piped output stream when aborting an oversized download so the file descriptor is not leaked until GC. - Make the semble downloader test mock path-aware so the added mid-install re-check is modeled correctly.
zoomote
Bot
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Related GitHub Issue
Closes: #1049
Description
Integrates Destructive Command Guard (DCG) into command execution so modern models can run complex commands and generated scripts without unnecessary pauses while preserving an explicit approval boundary for dangerous operations.
Key implementation details:
destructiveCommandGuardEnabledsetting across shared types, extension state, settings submission, and provider state.Reviewers should pay particular attention to the safety boundary between DCG classification and
checkAutoApproval, plus binary lifecycle handling in the DCG manager.Test Procedure
Automated tests run from the package directories that declare Vitest:
cd src npx vitest run core/auto-approval/__tests__/dcg.spec.ts core/tools/__tests__/executeCommandTool.spec.ts core/webview/__tests__/ClineProvider.spec.ts services/destructive-command-guard/__tests__/manager.spec.tsResult: 4 test files passed, 141 tests passed.
cd webview-ui npx vitest run src/components/chat/__tests__/ChatRow.command-denied.spec.tsx src/components/chat/__tests__/CommandExecution.spec.tsx src/components/settings/__tests__/AutoApproveSettings.spec.tsxResult: 3 test files passed, 47 tests passed.
Full repository suite after the review follow-up commit (
pnpm testfrom the repo root): all tasks successful, 7223 tests passed, 38 skipped.Manual verification:
Pre-Submission Checklist
*.visual.tsxsnapshot inwebview-ui/. Seewebview-ui/AGENTS.md→ "When a UI change needs a snapshot".Visual Snapshots
Videos (interaction / animation only)
Not applicable; this change does not introduce animation or a motion-dependent flow.
Documentation Updates
Additional Notes
The DCG integration is opt-in. Existing command allowlist and denylist behavior remains available when DCG is disabled.
Review follow-up (commit 2478d1f): addressed the remaining review findings in the shared managed-binary services — the installer now adopts a valid installation completed by a concurrent VS Code window instead of removing it mid-swap, and the downloader destroys the piped output stream when aborting an oversized archive so the file descriptor is not leaked. Both fixes include regression tests.
Get in Touch
GitHub: @navedmerchant
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Tests