[Feat] Integrate DCG with command auto-approval and denial UX - #1062
[Feat] Integrate DCG with command auto-approval and denial UX#1062navedmerchant wants to merge 2 commits into
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
e655ee2 to
28fb788
Compare
3e1b109 to
5f6e0a8
Compare
28fb788 to
f7ae3cb
Compare
5f6e0a8 to
dd00d03
Compare
dd00d03 to
e996b73
Compare
edelauna
left a comment
There was a problem hiding this comment.
Had some comments related to implementation.
| let dcgBlocked = false | ||
| if (providerState?.destructiveCommandGuardEnabled === true) { | ||
| const { ensureDcgInstalled, runDcg } = await import("../../services/destructive-command-guard") | ||
| if (!provider) { |
There was a problem hiding this comment.
Can this guard ever fire? provider is obtained at line 134 via providerRef.deref(), then used with optional chaining: provider?.getState(). So providerState?.destructiveCommandGuardEnabled === true is only true when provider is non-null — meaning we can only enter this block when provider is already guaranteed non-null. If provider is null, the outer if is false and we skip DCG entirely, silently bypassing it rather than throwing.
|
|
||
| const didApprove = await askApproval("command", canonicalCommand) | ||
| const provider = await task.providerRef.deref() | ||
| const providerState = await provider?.getState() |
There was a problem hiding this comment.
This moves getState() from post-approval to pre-approval, so it now runs for every command — including when DCG is disabled (the common case) and for commands the user declines. Is there a cheaper way to read just destructiveCommandGuardEnabled before deciding whether to invoke the full state?
| }) | ||
|
|
||
| expect(mockEnsureDcgInstalled).toHaveBeenCalledWith("/test/storage") | ||
| expect(mockRunDcg).toHaveBeenCalledWith("/test/storage/dcg", "echo test", "/test/workspace") |
There was a problem hiding this comment.
This confirms DCG was invoked correctly, but does it verify the allow result was acted upon? mockRunDcg defaults to { decision: "allow" }, so a mutation that ignores dcgResult.decision and always calls askApproval without isProtected would still pass both of these assertions.
| expect(screen.queryByText("chat:commandExecution.denied")).not.toBeInTheDocument() | ||
| expect(screen.getByTestId("command-execution")).toHaveAttribute("data-denied", "false") | ||
| }) | ||
| }) |
There was a problem hiding this comment.
The two cases cover "deny" and "approve", but not autoApprovalDecision: undefined — the normal state for commands awaiting user input. A mutation of === "deny" to !== "approve" in ChatRow.tsx would make those render as denied without being caught. Worth a third renderCommand(undefined) case?
| case "mistake_limit_reached": | ||
| return [null, null] // These will be handled by ErrorRow component | ||
| case "command": | ||
| if (message.autoApprovalDecision === "deny") { |
There was a problem hiding this comment.
When DCG is enabled, can autoApprovalDecision ever be "deny" here? checkAutoApproval short-circuits to approve before reaching getCommandDecision when DCG is on, so this branch looks like it only fires via the legacy denylist (DCG disabled). Is that the intent, or should DCG blocks also show this denied state?
Related GitHub Issue
Closes #1058
Completes #1049. Split from #1050.
Description
Completes the feature at the command approval boundary. DCG-approved commands can bypass the legacy allowlist only when command auto-approval and DCG are enabled. DCG-blocked commands are protected and require explicit approval. Disabling DCG preserves existing behavior. Localized reason/rule feedback and a dedicated denied-command chat state are included.
A user can still explicitly approve a DCG-blocked command; denying it does not execute it.
Stack
4 / 4 — base:
feat/dcg-settingThe complete stack tip is tree-identical to the original #1050 branch, so no original change was dropped.
Test Procedure
Result: 41 extension, 7 type-package, and 40 webview tests passed; type-checks, production builds, lint, pre-push checks, and i18n validation passed.
Checklist