security: use argv for host-side helper calls#2476
Open
WilliamK112 wants to merge 1 commit intoNVIDIA:mainfrom
Open
security: use argv for host-side helper calls#2476WilliamK112 wants to merge 1 commit intoNVIDIA:mainfrom
WilliamK112 wants to merge 1 commit intoNVIDIA:mainfrom
Conversation
Contributor
📝 WalkthroughWalkthroughThis change replaces shell-string command invocations with argv-array calls across onboarding and preflight code, introduces a Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 3
🧹 Nitpick comments (1)
src/lib/onboard.ts (1)
2598-2609: Extract gateway-volume cleanup into a shared helper.This argv-based cleanup looks good, but the orphaned-container path later in the same file still carries a separate shell-pipeline implementation for the same volume removal. Keeping both versions around makes the security fix easy to miss the next time this logic changes.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/onboard.ts` around lines 2598 - 2609, Extract the duplicated Docker volume cleanup into a shared helper (e.g., create a function removeGatewayVolumes(gatewayName: string)) that uses runCapture(["docker","volume","ls","-q","--filter", `name=openshell-cluster-${gatewayName}`], {ignoreError:true}) to compute volumeIds, then calls run(["docker","volume","rm", ...volumeIds], {ignoreError:true, suppressOutput:true}) if any exist; replace the inline logic that uses runCapture/run at the volume cleanup site and the separate orphaned-container pipeline that performs the same removal to call removeGatewayVolumes(GATEWAY_NAME) instead so both paths share one implementation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/lib/preflight.ts`:
- Around line 715-724: The current grep call in existingFstabEntry uses a plain
substring search which can match commented lines or unrelated paths; update the
check so it only matches active (non-commented) fstab entries for /swapfile by
changing the command invoked by run([...]) to use an anchored regex (e.g., grep
-qE with a pattern like '^[[:space:]]*[^#].*/swapfile\b') so only uncommented
lines containing /swapfile are detected; keep the subsequent runCapture([...
"sudo", "tee", "-a", "/etc/fstab"]) logic unchanged so the append happens only
when no active entry exists.
In `@test/argv-callers.test.ts`:
- Line 1: Add the required SPDX header lines to the top of this test file (above
the existing import { afterEach, describe, expect, it, vi } from "vitest";
line): insert the exact comment lines "// SPDX-FileCopyrightText: Copyright (c)
2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved." and "//
SPDX-License-Identifier: Apache-2.0" as the first two lines of the file.
- Around line 3-123: This test uses CommonJS require/require.cache which breaks
ESM test runner—replace require.resolve/require/require.cache usage with ESM
dynamic imports and Vitest module-reset/mocking: use string module specifiers
(e.g. const preflightPath = "../dist/lib/preflight.js") and await
import(preflightPath) to load assessHost, probeContainerDns,
getDockerBridgeGatewayIp, and onboard.getGatewayClusterContainerState, call
vi.resetModules() (or vi.restoreAllMocks() + vi.resetModules()) in afterEach
instead of deleting require.cache, and replace manual cache injection for runner
(where you override runCapture) with vi.mock(runnerPath, async () => ({
...(await vi.importActual(runnerPath)), runCapture: vi.fn(() => "running
healthy\n") })) so the tests use Vitest’s ESM-aware mocking and module
lifecycle.
---
Nitpick comments:
In `@src/lib/onboard.ts`:
- Around line 2598-2609: Extract the duplicated Docker volume cleanup into a
shared helper (e.g., create a function removeGatewayVolumes(gatewayName:
string)) that uses runCapture(["docker","volume","ls","-q","--filter",
`name=openshell-cluster-${gatewayName}`], {ignoreError:true}) to compute
volumeIds, then calls run(["docker","volume","rm", ...volumeIds],
{ignoreError:true, suppressOutput:true}) if any exist; replace the inline logic
that uses runCapture/run at the volume cleanup site and the separate
orphaned-container pipeline that performs the same removal to call
removeGatewayVolumes(GATEWAY_NAME) instead so both paths share one
implementation.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 4cd66351-0ab1-47ac-9975-34f03d2f7bbc
📒 Files selected for processing (3)
src/lib/onboard.tssrc/lib/preflight.tstest/argv-callers.test.ts
Comment on lines
+715
to
+724
| const existingFstabEntry = | ||
| run(["grep", "-q", "/swapfile", "/etc/fstab"], { | ||
| ignoreError: true, | ||
| suppressOutput: true, | ||
| }).status === 0; | ||
| if (!existingFstabEntry) { | ||
| runCapture(["sudo", "tee", "-a", "/etc/fstab"], { | ||
| ignoreError: false, | ||
| input: "/swapfile none swap sw 0 0\n", | ||
| }); |
Contributor
There was a problem hiding this comment.
Match only active /swapfile entries in /etc/fstab.
Line 716 uses a plain substring grep, so a commented entry or an unrelated path containing /swapfile will suppress the append. The swap file still works for the current boot, but it will not be re-enabled after reboot.
Suggested fix
const existingFstabEntry =
- run(["grep", "-q", "/swapfile", "/etc/fstab"], {
+ run(["grep", "-qE", "^[[:space:]]*/swapfile[[:space:]]", "/etc/fstab"], {
ignoreError: true,
suppressOutput: true,
}).status === 0;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const existingFstabEntry = | |
| run(["grep", "-q", "/swapfile", "/etc/fstab"], { | |
| ignoreError: true, | |
| suppressOutput: true, | |
| }).status === 0; | |
| if (!existingFstabEntry) { | |
| runCapture(["sudo", "tee", "-a", "/etc/fstab"], { | |
| ignoreError: false, | |
| input: "/swapfile none swap sw 0 0\n", | |
| }); | |
| const existingFstabEntry = | |
| run(["grep", "-qE", "^[[:space:]]*/swapfile[[:space:]]", "/etc/fstab"], { | |
| ignoreError: true, | |
| suppressOutput: true, | |
| }).status === 0; | |
| if (!existingFstabEntry) { | |
| runCapture(["sudo", "tee", "-a", "/etc/fstab"], { | |
| ignoreError: false, | |
| input: "/swapfile none swap sw 0 0\n", | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/preflight.ts` around lines 715 - 724, The current grep call in
existingFstabEntry uses a plain substring search which can match commented lines
or unrelated paths; update the check so it only matches active (non-commented)
fstab entries for /swapfile by changing the command invoked by run([...]) to use
an anchored regex (e.g., grep -qE with a pattern like
'^[[:space:]]*[^#].*/swapfile\b') so only uncommented lines containing /swapfile
are detected; keep the subsequent runCapture([... "sudo", "tee", "-a",
"/etc/fstab"]) logic unchanged so the append happens only when no active entry
exists.
| @@ -0,0 +1,123 @@ | |||
| import { afterEach, describe, expect, it, vi } from "vitest"; | |||
Contributor
There was a problem hiding this comment.
Add the required SPDX header to this new test file.
This file is missing the repository-mandated license header. As per coding guidelines, **/*.{js,mjs,ts,tsx,sh,md}: Every source file must include an SPDX license header: // SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. and // SPDX-License-Identifier: Apache-2.0.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/argv-callers.test.ts` at line 1, Add the required SPDX header lines to
the top of this test file (above the existing import { afterEach, describe,
expect, it, vi } from "vitest"; line): insert the exact comment lines "//
SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All
rights reserved." and "// SPDX-License-Identifier: Apache-2.0" as the first two
lines of the file.
Comment on lines
+3
to
+123
| const preflightPath = require.resolve("../dist/lib/preflight.js"); | ||
| const onboardPath = require.resolve("../dist/lib/onboard.js"); | ||
| const runnerPath = require.resolve("../dist/lib/runner.js"); | ||
|
|
||
| afterEach(() => { | ||
| vi.restoreAllMocks(); | ||
| delete require.cache[preflightPath]; | ||
| delete require.cache[onboardPath]; | ||
| delete require.cache[runnerPath]; | ||
| }); | ||
|
|
||
| describe("argv callsites", () => { | ||
| it("assessHost uses argv commands for host probes", () => { | ||
| const seen: Array<string | readonly string[]> = []; | ||
| delete require.cache[preflightPath]; | ||
| const preflight = require(preflightPath); | ||
|
|
||
| const assessment = preflight.assessHost({ | ||
| platform: "linux", | ||
| env: {}, | ||
| release: "6.6.0", | ||
| procVersion: "Linux version 6.6.0", | ||
| readFileImpl: () => { | ||
| throw new Error("no daemon config"); | ||
| }, | ||
| runCaptureImpl: (command: string | readonly string[], options?: { ignoreError?: boolean }) => { | ||
| seen.push(command); | ||
| const key = Array.isArray(command) ? command.join(" ") : command; | ||
| if (key === "which docker") return "/usr/bin/docker\n"; | ||
| if (key === "which node") return "/usr/bin/node\n"; | ||
| if (key === "which openshell") return "/usr/bin/openshell\n"; | ||
| if (key === "which nvidia-smi") return ""; | ||
| if (key === "which apt-get") return "/usr/bin/apt-get\n"; | ||
| if (key === "which systemctl") return "/usr/bin/systemctl\n"; | ||
| if (key === "docker info --format {{json .}}") return '{"ServerVersion":"27.0.0","OperatingSystem":"Docker Engine"}'; | ||
| if (key === "systemctl is-active docker") return "active\n"; | ||
| if (key === "systemctl is-enabled docker") return "enabled\n"; | ||
| return options?.ignoreError ? "" : ""; | ||
| }, | ||
| }); | ||
|
|
||
| expect(assessment.dockerInstalled).toBe(true); | ||
| expect(assessment.nodeInstalled).toBe(true); | ||
| expect(assessment.openshellInstalled).toBe(true); | ||
| expect(assessment.packageManager).toBe("apt"); | ||
| expect(assessment.dockerReachable).toBe(true); | ||
| expect(seen).toContainEqual(["which", "docker"]); | ||
| expect(seen).toContainEqual(["docker", "info", "--format", "{{json .}}"]); | ||
| expect(seen).toContainEqual(["systemctl", "is-active", "docker"]); | ||
| expect(seen).toContainEqual(["systemctl", "is-enabled", "docker"]); | ||
| expect(seen.some((command) => typeof command === "string" && command.includes("command -v"))).toBe(false); | ||
| }); | ||
|
|
||
| it("probeContainerDns defaults to argv docker run", () => { | ||
| const seen: Array<string | readonly string[]> = []; | ||
| delete require.cache[preflightPath]; | ||
| const { probeContainerDns } = require(preflightPath); | ||
|
|
||
| const result = probeContainerDns({ | ||
| runCaptureImpl: (command: string | readonly string[], opts?: { ignoreError?: boolean; timeout?: number }) => { | ||
| seen.push(command); | ||
| expect(opts?.ignoreError).toBe(true); | ||
| expect(opts?.timeout).toBe(20_000); | ||
| return "Server:\t1.1.1.1\nName:\tregistry.npmjs.org\nAddress: 104.16.25.35\n"; | ||
| }, | ||
| }); | ||
|
|
||
| expect(result).toEqual({ ok: true }); | ||
| expect(seen).toEqual([ | ||
| ["docker", "run", "--rm", "--pull=missing", "busybox:latest", "nslookup", "registry.npmjs.org"], | ||
| ]); | ||
| }); | ||
|
|
||
| it("getDockerBridgeGatewayIp uses argv docker inspect", () => { | ||
| delete require.cache[preflightPath]; | ||
| const { getDockerBridgeGatewayIp } = require(preflightPath); | ||
| const seen: Array<string | readonly string[]> = []; | ||
|
|
||
| const gateway = getDockerBridgeGatewayIp((command: string | readonly string[]) => { | ||
| seen.push(command); | ||
| return "172.17.0.1fd00:abcd::1\n"; | ||
| }); | ||
|
|
||
| expect(gateway).toBe("172.17.0.1"); | ||
| expect(seen).toEqual([ | ||
| ["docker", "network", "inspect", "bridge", "--format", "{{range .IPAM.Config}}{{.Gateway}}{{end}}"], | ||
| ]); | ||
| }); | ||
|
|
||
| it("getGatewayClusterContainerState uses argv docker inspect", () => { | ||
| const actualRunner = require(runnerPath); | ||
| const runCapture = vi.fn(() => "running healthy\n"); | ||
| require.cache[runnerPath] = { | ||
| id: runnerPath, | ||
| filename: runnerPath, | ||
| loaded: true, | ||
| exports: { | ||
| ...actualRunner, | ||
| runCapture, | ||
| }, | ||
| } as any; | ||
|
|
||
| delete require.cache[onboardPath]; | ||
| const onboard = require(onboardPath); | ||
| const state = onboard.getGatewayClusterContainerState(); | ||
|
|
||
| expect(state).toBe("running healthy"); | ||
| expect(runCapture).toHaveBeenCalledWith( | ||
| [ | ||
| "docker", | ||
| "inspect", | ||
| "--type", | ||
| "container", | ||
| "--format", | ||
| "{{.State.Status}}{{if .State.Health}} {{.State.Health.Status}}{{end}}", | ||
| "openshell-cluster-nemoclaw", | ||
| ], | ||
| { ignoreError: true }, | ||
| ); | ||
| }); | ||
| }); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, let's check if the file exists and examine its contents
if [ -f "test/argv-callers.test.ts" ]; then
echo "=== File Found ==="
wc -l test/argv-callers.test.ts
echo ""
echo "=== First 50 lines ==="
head -50 test/argv-callers.test.ts
else
echo "File test/argv-callers.test.ts not found"
fiRepository: NVIDIA/NemoClaw
Length of output: 2326
🏁 Script executed:
#!/bin/bash
# Check the vitest configuration and ESM setup
echo "=== Checking vitest.config files ==="
find . -maxdepth 2 -name "vitest.config.*" -o -name "vite.config.*" | head -10
echo ""
echo "=== Checking package.json for module type ==="
if [ -f "package.json" ]; then
cat package.json | head -30
fiRepository: NVIDIA/NemoClaw
Length of output: 2016
🏁 Script executed:
#!/bin/bash
# Check if there's a tsconfig.json to understand module system
echo "=== Checking tsconfig.json ==="
if [ -f "tsconfig.json" ]; then
cat tsconfig.json | jq . 2>/dev/null || cat tsconfig.json
fiRepository: NVIDIA/NemoClaw
Length of output: 90
🏁 Script executed:
#!/bin/bash
# Check the vitest configuration
echo "=== vitest.config.ts ==="
cat vitest.config.ts
echo ""
echo "=== Check for 'type' field in package.json ==="
jq '.type' package.json 2>/dev/null || grep '"type"' package.json || echo "No 'type' field found"Repository: NVIDIA/NemoClaw
Length of output: 1879
🏁 Script executed:
#!/bin/bash
# Check if there are other test files using require pattern
echo "=== Other test files using require ==="
find test -name "*.test.ts" -o -name "*.test.js" | while read file; do
if grep -l "require(" "$file" 2>/dev/null; then
echo "File: $file"
fi
done
echo ""
echo "=== Check how many test files exist ==="
find test -name "*.test.ts" -o -name "*.test.js" | wc -lRepository: NVIDIA/NemoClaw
Length of output: 2166
Convert this suite to ESM module loading.
test/ directory requires ESM imports per coding guidelines, but this file uses CommonJS (require.resolve(), require(), require.cache). In an ESM setup, these globals are unavailable and this approach bypasses Vitest's vi.resetModules() flow. Use dynamic await import(...) with Vitest mocks instead.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/argv-callers.test.ts` around lines 3 - 123, This test uses CommonJS
require/require.cache which breaks ESM test runner—replace
require.resolve/require/require.cache usage with ESM dynamic imports and Vitest
module-reset/mocking: use string module specifiers (e.g. const preflightPath =
"../dist/lib/preflight.js") and await import(preflightPath) to load assessHost,
probeContainerDns, getDockerBridgeGatewayIp, and
onboard.getGatewayClusterContainerState, call vi.resetModules() (or
vi.restoreAllMocks() + vi.resetModules()) in afterEach instead of deleting
require.cache, and replace manual cache injection for runner (where you override
runCapture) with vi.mock(runnerPath, async () => ({ ...(await
vi.importActual(runnerPath)), runCapture: vi.fn(() => "running healthy\n") }))
so the tests use Vitest’s ESM-aware mocking and module lifecycle.
WilliamK112
force-pushed
the
security/argv-callers
branch
from
2f20e7d to
69dc6a9
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (3)
src/lib/preflight.ts (1)
715-725:⚠️ Potential issue | 🟡 MinorTighten the
/swapfilematch before skipping the append.Line 716 still uses a plain substring grep, so a commented entry or an unrelated path containing
/swapfilewill makeexistingFstabEntrylook true. In that case the swapfile works for the current boot, but it will not persist across reboot.Suggested fix
const existingFstabEntry = - run(["grep", "-q", "/swapfile", "/etc/fstab"], { + run(["grep", "-qE", "^[[:space:]]*/swapfile[[:space:]]", "/etc/fstab"], { ignoreError: true, suppressOutput: true, }).status === 0;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/preflight.ts` around lines 715 - 725, The current check using run(["grep", "-q", "/swapfile", "/etc/fstab"]) can be fooled by commented lines or other paths containing "/swapfile"; change the existence test (the existingFstabEntry assignment) to search for a non-commented fstab entry that lists /swapfile as the swap file (e.g., a regex like '^[[:space:]]*[^#].*\s+/swapfile(\s|$)' or equivalent) so only active fstab entries match, then keep the append logic using runCapture(["sudo","tee","-a","/etc/fstab"]) unchanged; update the grep invocation inside run(...) that sets existingFstabEntry (and any related variable) to use the stricter pattern.test/argv-callers.test.ts (2)
3-123:⚠️ Potential issue | 🟠 MajorConvert this suite from CommonJS loading/cache mutation to ESM + Vitest-native module control.
Lines 3-123 use
require.resolve,require, andrequire.cache, which violates thetest/ESM rule and makes module lifecycle handling brittle. Useawait import(...),vi.resetModules(), andvi.mock(...)instead.Refactor sketch (ESM-compatible)
-const preflightPath = require.resolve("../dist/lib/preflight.js"); -const onboardPath = require.resolve("../dist/lib/onboard.js"); -const runnerPath = require.resolve("../dist/lib/runner.js"); +const preflightPath = "../dist/lib/preflight.js"; +const onboardPath = "../dist/lib/onboard.js"; +const runnerPath = "../dist/lib/runner.js"; afterEach(() => { vi.restoreAllMocks(); - delete require.cache[preflightPath]; - delete require.cache[onboardPath]; - delete require.cache[runnerPath]; + vi.resetModules(); }); -it("assessHost uses argv commands for host probes", () => { +it("assessHost uses argv commands for host probes", async () => { const seen: Array<string | readonly string[]> = []; - delete require.cache[preflightPath]; - const preflight = require(preflightPath); + const preflight = await import(preflightPath); ... }); -it("probeContainerDns defaults to argv docker run", () => { +it("probeContainerDns defaults to argv docker run", async () => { const seen: Array<string | readonly string[]> = []; - delete require.cache[preflightPath]; - const { probeContainerDns } = require(preflightPath); + const { probeContainerDns } = await import(preflightPath); ... }); -it("getDockerBridgeGatewayIp uses argv docker inspect", () => { - delete require.cache[preflightPath]; - const { getDockerBridgeGatewayIp } = require(preflightPath); +it("getDockerBridgeGatewayIp uses argv docker inspect", async () => { + const { getDockerBridgeGatewayIp } = await import(preflightPath); ... }); -it("getGatewayClusterContainerState uses argv docker inspect", () => { - const actualRunner = require(runnerPath); +it("getGatewayClusterContainerState uses argv docker inspect", async () => { const runCapture = vi.fn(() => "running healthy\n"); - require.cache[runnerPath] = { ... } as any; + vi.mock(runnerPath, async () => { + const actualRunner = await vi.importActual<Record<string, unknown>>(runnerPath); + return { ...actualRunner, runCapture }; + }); - delete require.cache[onboardPath]; - const onboard = require(onboardPath); + const onboard = await import(onboardPath); const state = onboard.getGatewayClusterContainerState(); ... });#!/bin/bash # Verify CommonJS-only patterns are still present in this test file. rg -n -C2 'require\.resolve\(|\brequire\(|require\.cache' test/argv-callers.test.tsAs per coding guidelines,
test/**/*.{js,ts}: test/ directory must use ESM (import/export) for test filesandtest/**/*.test.{js,ts}: Root-level integration tests in test/ directory should use ESM imports and mock external dependencies without calling real NVIDIA APIs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/argv-callers.test.ts` around lines 3 - 123, The tests use CommonJS require/require.cache and require.resolve (preflightPath/onboardPath/runnerPath) which violates ESM test rules; replace all require.resolve/require/require.cache mutations with dynamic ESM imports (await import(...)) and use Vitest module helpers: call vi.resetModules() in afterEach, replace manual cache stubs with vi.mock(...) to override runner.runCapture when testing getGatewayClusterContainerState, and when importing preflight functions (assessHost, probeContainerDns, getDockerBridgeGatewayIp) import them via await import and pass mocked implementations (runCaptureImpl/readFileImpl) rather than mutating require.cache; ensure probeContainerDns and getDockerBridgeGatewayIp assertions still inspect the array commands and options, and remove any use of delete require.cache and require.resolve.
1-1:⚠️ Potential issue | 🟠 MajorAdd the required SPDX header to this test file.
Line 1 starts directly with imports; the mandatory SPDX copyright and license lines are missing.
Proposed fix
+// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 import { afterEach, describe, expect, it, vi } from "vitest";As per coding guidelines,
**/*.{js,mjs,ts,tsx,sh,md}: Every source file must include an SPDX license header: // SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. and // SPDX-License-Identifier: Apache-2.0.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/argv-callers.test.ts` at line 1, This file is missing the mandatory SPDX header; add the two required comment lines immediately above the first import statement (the line starting with "import { afterEach, describe, expect, it, vi }"): "// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved." and "// SPDX-License-Identifier: Apache-2.0", ensuring they are the very first lines in the file.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/lib/preflight.ts`:
- Around line 715-725: The current check using run(["grep", "-q", "/swapfile",
"/etc/fstab"]) can be fooled by commented lines or other paths containing
"/swapfile"; change the existence test (the existingFstabEntry assignment) to
search for a non-commented fstab entry that lists /swapfile as the swap file
(e.g., a regex like '^[[:space:]]*[^#].*\s+/swapfile(\s|$)' or equivalent) so
only active fstab entries match, then keep the append logic using
runCapture(["sudo","tee","-a","/etc/fstab"]) unchanged; update the grep
invocation inside run(...) that sets existingFstabEntry (and any related
variable) to use the stricter pattern.
In `@test/argv-callers.test.ts`:
- Around line 3-123: The tests use CommonJS require/require.cache and
require.resolve (preflightPath/onboardPath/runnerPath) which violates ESM test
rules; replace all require.resolve/require/require.cache mutations with dynamic
ESM imports (await import(...)) and use Vitest module helpers: call
vi.resetModules() in afterEach, replace manual cache stubs with vi.mock(...) to
override runner.runCapture when testing getGatewayClusterContainerState, and
when importing preflight functions (assessHost, probeContainerDns,
getDockerBridgeGatewayIp) import them via await import and pass mocked
implementations (runCaptureImpl/readFileImpl) rather than mutating
require.cache; ensure probeContainerDns and getDockerBridgeGatewayIp assertions
still inspect the array commands and options, and remove any use of delete
require.cache and require.resolve.
- Line 1: This file is missing the mandatory SPDX header; add the two required
comment lines immediately above the first import statement (the line starting
with "import { afterEach, describe, expect, it, vi }"): "//
SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All
rights reserved." and "// SPDX-License-Identifier: Apache-2.0", ensuring they
are the very first lines in the file.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 16eda3cd-80f5-4977-ad92-1b99f1e7516d
📒 Files selected for processing (3)
src/lib/onboard.tssrc/lib/preflight.tstest/argv-callers.test.ts
wscurran
added
security
Contributor
|
✨ Thanks for submitting this pull request that proposes a way to improve security by using argv for host-side helper calls, specifically converting shell-string helper calls to argv-style execution where the command does not actually need a shell. Related open issues: |
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
2 participants
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.
Summary
This is a small Phase 1 pass on #1889.
It converts shell-string helper calls to argv-style execution where the command does not actually need a shell, while intentionally leaving explicit shell-dependent cases in place, such as:
What changed
src/lib/preflight.tsto argv formsrc/lib/onboard.tsto argv formValidation
npm run build:clinpm test -- test/argv-callers.test.tsNotes
I also checked the broader test suite locally, but there are existing unrelated failures/noise on current main, so I kept validation scoped to the touched paths for this PR.
Summary by CodeRabbit
Bug Fixes
Tests