[Improve] Harden DCG binary installation - #1060
Conversation
📝 WalkthroughWalkthroughAdded the destructive-command-guard service. It selects and installs pinned platform binaries, validates downloads and archives, and runs the binary with restricted environment variables, output limits, timeout handling, and typed decision parsing. ChangesDestructive command guard service
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant ensureDcgInstalled
participant ManagedBinaryInstaller
participant runDcg
participant DCGBinary
Caller->>ensureDcgInstalled: request DCG binary
ensureDcgInstalled->>ManagedBinaryInstaller: install or reuse pinned binary
ManagedBinaryInstaller-->>Caller: binary path
Caller->>runDcg: command and working directory
runDcg->>DCGBinary: spawn with sanitized environment
DCGBinary-->>runDcg: bounded JSON decision
runDcg-->>Caller: typed allow or deny result
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
src/services/destructive-command-guard/__tests__/manager.spec.tsESLint skipped: missing config or dependency (missing-dependency). The ESLint configuration references a package that is not available in the sandbox. src/services/destructive-command-guard/__tests__/runner.spec.tsESLint skipped: the ESLint configuration for this file references a package that is not available in the sandbox. src/services/destructive-command-guard/constants.tsESLint skipped: the ESLint configuration for this file references a package that is not available in the sandbox.
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! |
5c460bd to
baade5f
Compare
github-actions
Bot
added
the
awaiting-review
edelauna
left a comment
edelauna
left a comment
There was a problem hiding this comment.
Looks good - just had 2 comments for increased test coverage.
github-actions
Bot
added
awaiting-author
baade5f to
e1a0c39
Compare
github-actions
Bot
added
awaiting-author
edelauna
left a comment
edelauna
left a comment
There was a problem hiding this comment.
😅 have a couple additional comments related to the logic used for the guard.
github-actions
Bot
added
awaiting-author
edelauna
left a comment
edelauna
left a comment
There was a problem hiding this comment.
Looks good - approving with some minor nits, which could also always be addressed later if we want.
github-actions
Bot
removed
the
awaiting-author
github-actions
Bot
removed
the
awaiting-review
zoomote
Bot
changed the title
|
Addressed all current unresolved review feedback in 5f3854a. DCG now preserves |
github-actions
Bot
added
the
awaiting-review
github-actions
Bot
added
awaiting-author
github-actions
Bot
removed
the
awaiting-author
|
Merging, base branch merged, code has not changed |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (4)
src/services/destructive-command-guard/__tests__/runner.spec.ts (1)
122-132: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a case for signal-terminated exits.
The tests cover exit code 2 but never a non-null
signal.runner.tsline 61 treatssignal || (code !== 0 && code !== 1)as a failure, so a signal-terminated run with code 0 must still reject. Add a case that emitsclosewith a signal to lock that branch.🧪 Proposed test
+ it("rejects runs terminated by a signal", async () => { + const child = createChild() + useChild(child) + + const result = runDcg("/dcg", "echo test", "/workspace") + child.stdout.write(JSON.stringify({ schema_version: 1, decision: "allow" })) + child.emit("close", 0, "SIGKILL") + + await expect(result).rejects.toThrow("DCG evaluation failed") + })🤖 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/__tests__/runner.spec.ts` around lines 122 - 132, Add a test alongside “rejects non-DCG exit statuses with stderr” that emits the child’s close event with a non-null signal and exit code 0, then assert the runDcg promise rejects. Reuse createChild, useChild, and the existing result setup to cover the signal failure branch in the runner.src/services/destructive-command-guard/__tests__/manager.spec.ts (3)
244-247: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace the silent early return with
skipIf.Line 247 returns when the current platform is supported. Vitest then reports a passing test with zero assertions on every supported platform. The ZIP test at line 332 already uses
it.skipIf. Use the same pattern here, and at line 263, so CI output states that the test was skipped.♻️ Proposed change
- it("warns when the current platform is unsupported", async () => { - const platformKey = `${process.platform}-${process.arch}` - const info = DCG_ARCHIVES[platformKey] - if (!info) return + const currentPlatformKey = `${process.platform}-${process.arch}` + + it.skipIf(!DCG_ARCHIVES[currentPlatformKey])("warns when the current platform is unsupported", async () => { + const platformKey = currentPlatformKey + const info = DCG_ARCHIVES[platformKey]!🤖 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/__tests__/manager.spec.ts` around lines 244 - 247, Replace the silent `if (!info) return` early return in the platform warning test with Vitest’s `it.skipIf` pattern, and apply the same change to the related test around `DCG_ARCHIVES` at line 263. Preserve the existing test bodies while ensuring supported platforms are reported as skipped rather than passing with zero assertions.
218-218: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument the remaining double assertions.
Lines 168, 200, 288, 292, and 310 explain each
as unknown ascast with a comment. Lines 218, 352, and 355 use the same casts without a comment. Add the same short explanation for consistency.As per coding guidelines: "Use double assertions only as a last resort and explain them with a comment."
Also applies to: 348-356
🤖 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/__tests__/manager.spec.ts` at line 218, Add concise explanatory comments beside the remaining double assertions in the test setup around mockSpawn and the lines also identified near 348–356. Match the existing comments used at lines 168, 200, 288, 292, and 310, without changing the assertions or test behavior.Source: Coding guidelines
302-308: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winHandle rejections in the async
setImmediatecallbacks.
writeFileruns inside an async callback that no one awaits. IfwriteFilerejects, the rejection is unhandled, the test hangs onfirstInstallation, and the real cause is hidden. The same pattern exists at lines 363-370. Catch the error and fail the child process instead.♻️ Proposed change
setImmediate(async () => { - if (executable === "tar") { - const stagingDir = args[args.indexOf("-C") + 1] - await writeFile(path.join(stagingDir, info.binary), "executable") - } - child.emit("close", 0) + try { + if (executable === "tar") { + const stagingDir = args[args.indexOf("-C") + 1] + await writeFile(path.join(stagingDir, info.binary), "executable") + } + child.emit("close", 0) + } catch (error) { + child.emit("error", error) + } })🤖 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/__tests__/manager.spec.ts` around lines 302 - 308, Handle promise rejections in both async setImmediate callbacks around the child process simulation, including the callback near the tar executable setup and the matching callback later in the test. Wrap writeFile and subsequent logic in try/catch, and on failure emit a nonzero child close/error result so the awaiting installation rejects instead of hanging.
🤖 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.
Nitpick comments:
In `@src/services/destructive-command-guard/__tests__/manager.spec.ts`:
- Around line 244-247: Replace the silent `if (!info) return` early return in
the platform warning test with Vitest’s `it.skipIf` pattern, and apply the same
change to the related test around `DCG_ARCHIVES` at line 263. Preserve the
existing test bodies while ensuring supported platforms are reported as skipped
rather than passing with zero assertions.
- Line 218: Add concise explanatory comments beside the remaining double
assertions in the test setup around mockSpawn and the lines also identified near
348–356. Match the existing comments used at lines 168, 200, 288, 292, and 310,
without changing the assertions or test behavior.
- Around line 302-308: Handle promise rejections in both async setImmediate
callbacks around the child process simulation, including the callback near the
tar executable setup and the matching callback later in the test. Wrap writeFile
and subsequent logic in try/catch, and on failure emit a nonzero child
close/error result so the awaiting installation rejects instead of hanging.
In `@src/services/destructive-command-guard/__tests__/runner.spec.ts`:
- Around line 122-132: Add a test alongside “rejects non-DCG exit statuses with
stderr” that emits the child’s close event with a non-null signal and exit code
0, then assert the runDcg promise rejects. Reuse createChild, useChild, and the
existing result setup to cover the signal failure branch in the runner.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b8ff35b7-5682-4638-aa3d-239408320348
📒 Files selected for processing (5)
src/services/destructive-command-guard/__tests__/manager.spec.tssrc/services/destructive-command-guard/__tests__/runner.spec.tssrc/services/destructive-command-guard/constants.tssrc/services/destructive-command-guard/manager.tssrc/services/destructive-command-guard/runner.ts
github-actions
Bot
added
the
awaiting-author
github-actions
Bot
added
awaiting-review
2 participants
What changed
Completes the DCG binary-service safeguards with macOS temporary-directory support, an explicit skipped ZIP-only test on non-Windows CI, and coverage for trusted cross-host GitHub release redirects.
Why this change was made
DCG must run with the minimum viable environment and install predictably across supported platforms and GitHub's release-asset redirect flow. Closes #1056.
Impact
DCG installation and execution are more reliable across platforms while keeping extension-host secrets out of the child process environment.
Related PRs
Summary by CodeRabbit
New Features
Tests