Skip to content

Commit 6ee1bfa

Browse files
cvericksoa
authored and
DemianHeyGen
committed
fix(cli): require argv arrays for runCapture (NVIDIA#2582)
## Summary This PR removes the shell-string overload from `runCapture()` and makes argv arrays the only supported input. It updates the remaining call sites to use explicit argv execution so shell parsing is always opt-in and visible in review. ## Changes - Remove the `execSync`-based shell-string path from `src/lib/runner.ts` and make `runCapture()` reject string inputs at runtime. - Convert `src/lib/preflight.ts` and `src/lib/onboard.ts` call sites from shell strings to argv arrays for command discovery, Docker inspection, DNS probing, `ps`, and `hostname -I`. - Update runner and preflight tests to cover argv-only behavior and spawn-based error redaction. ## Type of Change - [x] Code change (feature, bug fix, or refactor) - [ ] Code change with doc updates - [ ] Doc only (prose changes, no code sample modifications) - [ ] Doc only (includes code sample changes) ## Verification - [x] `npx prek run --all-files` passes - [x] `npm test` passes - [x] Tests added or updated for new or changed behavior - [x] No secrets, API keys, or credentials committed - [ ] Docs updated for user-facing behavior changes - [ ] `make docs` builds without warnings (doc changes only) - [ ] Doc pages follow the [style guide](https://github.com/NVIDIA/NemoClaw/blob/main/docs/CONTRIBUTING.md) (doc changes only) - [ ] New doc pages include SPDX header and frontmatter (new pages only) ## AI Disclosure - [x] AI-assisted — tool: pi coding agent --- Signed-off-by: Carlos Villela <cvillela@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Refactor** * Standardized command execution for host and container probes, improving reliability of gateway, IP/gateway discovery, disk/swap detection, and process/network checks for more consistent inspection results. * **Tests** * Updated tests to validate the new argument-array execution model, tighten error/regression guards, and ensure command output trimming and expected behavior. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Aaron Erickson <aerickson@nvidia.com>
1 parent cba8e92 commit 6ee1bfa

6 files changed

Lines changed: 199 additions & 166 deletions

File tree

‎src/lib/onboard.ts‎

Lines changed: 33 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -1816,7 +1816,15 @@ function destroyGateway() {
18161816
function getGatewayClusterContainerState(): string {
18171817
const containerName = getGatewayClusterContainerName();
18181818
const state = runCapture(
1819-
`docker inspect --type container --format '{{.State.Status}}{{if .State.Health}} {{.State.Health.Status}}{{end}}' ${shellQuote(containerName)} 2>/dev/null`,
1819+
[
1820+
"docker",
1821+
"inspect",
1822+
"--type",
1823+
"container",
1824+
"--format",
1825+
"{{.State.Status}}{{if .State.Health}} {{.State.Health.Status}}{{end}}",
1826+
containerName,
1827+
],
18201828
{ ignoreError: true },
18211829
)
18221830
.trim()
@@ -1848,6 +1856,22 @@ function getGatewayClusterContainerName(): string {
18481856
return `openshell-cluster-${GATEWAY_NAME}`;
18491857
}
18501858

1859+
function buildGatewayClusterExecArgv(script: string): string[] {
1860+
return ["docker", "exec", getGatewayClusterContainerName(), "sh", "-lc", script];
1861+
}
1862+
1863+
function hostCommandExists(commandName: string): boolean {
1864+
return !!runCapture(["sh", "-c", 'command -v "$1"', "--", commandName], {
1865+
ignoreError: true,
1866+
});
1867+
}
1868+
1869+
function captureProcessArgs(pid: number): string {
1870+
return runCapture(["ps", "-p", String(pid), "-o", "args="], {
1871+
ignoreError: true,
1872+
}).trim();
1873+
}
1874+
18511875
function getGatewayLocalEndpoint(): string {
18521876
return `https://127.0.0.1:${GATEWAY_PORT}`;
18531877
}
@@ -1910,13 +1934,11 @@ fi
19101934
}
19111935

19121936
function runGatewayClusterCapture(script: string, opts: RunnerOptions = {}) {
1913-
const containerName = getGatewayClusterContainerName();
1914-
return runCapture(`docker exec ${shellQuote(containerName)} sh -lc ${shellQuote(script)}`, opts);
1937+
return runCapture(buildGatewayClusterExecArgv(script), opts);
19151938
}
19161939

19171940
function runGatewayCluster(script: string, opts: RunnerOptions = {}) {
1918-
const containerName = getGatewayClusterContainerName();
1919-
return run(`docker exec ${shellQuote(containerName)} sh -lc ${shellQuote(script)}`, opts);
1941+
return run(buildGatewayClusterExecArgv(script), opts);
19201942
}
19211943

19221944
function listMissingGatewayBootstrapSecrets() {
@@ -2458,9 +2480,7 @@ async function preflight(): Promise<ReturnType<typeof nim.detectGpu>> {
24582480
// tunnels the user may have set up on the same port. (#1950)
24592481
if (port === DASHBOARD_PORT && portCheck.process === "ssh" && portCheck.pid) {
24602482
// Use `ps` to get the command line — works on Linux, macOS, and WSL.
2461-
const cmdline = runCapture(`ps -p ${portCheck.pid} -o args= 2>/dev/null`, {
2462-
ignoreError: true,
2463-
}).trim();
2483+
const cmdline = captureProcessArgs(portCheck.pid);
24642484
if (cmdline.includes("openshell")) {
24652485
console.log(
24662486
` Cleaning up orphaned SSH port-forward on port ${port} (PID ${portCheck.pid})...`,
@@ -3877,8 +3897,7 @@ async function setupNim(gpu: ReturnType<typeof nim.detectGpu>): Promise<{
38773897
let preferredInferenceApi: string | null = null;
38783898

38793899
// Detect local inference options
3880-
// "command -v" is a shell builtin — must go through bash.
3881-
const hasOllama = !!runCapture("command -v ollama", { ignoreError: true });
3900+
const hasOllama = hostCommandExists("ollama");
38823901
const ollamaRunning = !!runCapture(["curl", "-sf", `http://127.0.0.1:${OLLAMA_PORT}/api/tags`], {
38833902
ignoreError: true,
38843903
});
@@ -6219,12 +6238,11 @@ function getWslHostAddress(
62196238
return null;
62206239
}
62216240
const runCaptureFn = options.runCapture || runCapture;
6222-
const output = runCaptureFn("hostname -I 2>/dev/null", { ignoreError: true });
6223-
const candidates = String(output || "")
6241+
const output = runCaptureFn(["hostname", "-I"], { ignoreError: true });
6242+
return String(output || "")
62246243
.trim()
62256244
.split(/\s+/)
6226-
.filter(Boolean);
6227-
return candidates[0] || null;
6245+
.filter(Boolean)[0] || null;
62286246
}
62296247

62306248
function getDashboardAccessInfo(
@@ -6305,7 +6323,7 @@ function printDashboard(
63056323

63066324
const token = fetchGatewayAuthTokenFromSandbox(sandboxName);
63076325
const chatUiUrl = process.env.CHAT_UI_URL || `http://127.0.0.1:${CONTROL_UI_PORT}`;
6308-
const wslAddr = isWsl() ? (String(runCapture("hostname -I 2>/dev/null", { ignoreError: true }) || "").trim().split(/\s+/)[0] || null) : null;
6326+
const wslAddr = getWslHostAddress();
63096327
const chain = buildChain({ chatUiUrl, isWsl: isWsl(), wslHostAddress: wslAddr });
63106328

63116329
// Build access info inline — uses chain instead of re-deriving from env

‎src/lib/preflight.test.ts‎

Lines changed: 31 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -314,11 +314,11 @@ describe("assessHost", () => {
314314
readFileImpl: () => '{"default-cgroupns-mode":"private"}',
315315
commandExistsImpl: (name: string) =>
316316
name === "docker" || name === "apt-get" || name === "systemctl",
317-
runCaptureImpl: (command: string) => {
318-
if (command === "command -v apt-get") return "/usr/bin/apt-get";
319-
if (command === "command -v systemctl") return "/usr/bin/systemctl";
320-
if (command === "systemctl is-active docker") return "active";
321-
if (command === "systemctl is-enabled docker") return "enabled";
317+
runCaptureImpl: (command: readonly string[]) => {
318+
if (command.join(" ") === 'sh -c command -v "$1" -- apt-get') return "/usr/bin/apt-get";
319+
if (command.join(" ") === 'sh -c command -v "$1" -- systemctl') return "/usr/bin/systemctl";
320+
if (command.join(" ") === "systemctl is-active docker") return "active";
321+
if (command.join(" ") === "systemctl is-enabled docker") return "enabled";
322322
return "";
323323
},
324324
});
@@ -731,8 +731,7 @@ describe("probeContainerDns", () => {
731731
"Address: 104.16.26.35\n" +
732732
"Address: 104.16.27.35\n";
733733

734-
const BUSYBOX_FAILURE =
735-
";; connection timed out; no servers could be reached\n";
734+
const BUSYBOX_FAILURE = ";; connection timed out; no servers could be reached\n";
736735

737736
it("returns ok when busybox nslookup succeeds", () => {
738737
const result = probeContainerDns({ outputOverride: BUSYBOX_SUCCESS });
@@ -760,7 +759,7 @@ describe("probeContainerDns", () => {
760759
const pullError =
761760
"Unable to find image 'busybox:latest' locally\n" +
762761
"latest: Pulling from library/busybox\n" +
763-
"docker: Error response from daemon: Head \"https://registry-1.docker.io/v2/library/busybox/manifests/latest\": dial tcp: lookup registry-1.docker.io: no such host.\n";
762+
'docker: Error response from daemon: Head "https://registry-1.docker.io/v2/library/busybox/manifests/latest": dial tcp: lookup registry-1.docker.io: no such host.\n';
764763
const result = probeContainerDns({ outputOverride: pullError });
765764
expect(result.ok).toBe(false);
766765
expect(result.reason).toBe("image_pull_failed");
@@ -807,30 +806,35 @@ describe("probeContainerDns", () => {
807806
});
808807

809808
it("captures the spawned command for runCapture override", () => {
810-
const captured: string[] = [];
809+
const captured: string[][] = [];
811810
const result = probeContainerDns({
812811
runCaptureImpl: (command) => {
813-
captured.push(command);
812+
captured.push([...command]);
814813
return BUSYBOX_SUCCESS;
815814
},
816815
});
817816
expect(result.ok).toBe(true);
818817
expect(captured).toHaveLength(1);
819-
expect(captured[0]).toContain("docker run");
820-
expect(captured[0]).toContain("busybox");
821-
expect(captured[0]).toContain("registry.npmjs.org");
818+
// Probe shells out via `sh -c "<script> 2>&1"` so docker/busybox
819+
// stderr lands in stdout where the parser can see it.
820+
expect(captured[0].slice(0, 2)).toEqual(["sh", "-c"]);
821+
const script = captured[0][2];
822+
expect(script).toContain("docker run --rm");
823+
expect(script).toContain("busybox:latest");
824+
expect(script).toContain("registry.npmjs.org");
825+
expect(script).toContain("2>&1");
822826
});
823827

824828
it("allows the command to be overridden", () => {
825-
let seen = "";
829+
let seen: readonly string[] = [];
826830
probeContainerDns({
827-
command: "echo OVERRIDDEN",
831+
command: ["echo", "OVERRIDDEN"],
828832
runCaptureImpl: (command) => {
829833
seen = command;
830834
return "Name:\tregistry.npmjs.org\nAddress: 1.2.3.4\n";
831835
},
832836
});
833-
expect(seen).toBe("echo OVERRIDDEN");
837+
expect(seen).toEqual(["echo", "OVERRIDDEN"]);
834838
});
835839

836840
it("treats thrown runCapture errors as error reason", () => {
@@ -868,17 +872,20 @@ describe("probeContainerDns", () => {
868872
expect(seenOpts?.timeout).toBe(20_000);
869873
});
870874

871-
it("emits a plain `docker run ...` command (no shell-specific wrappers)", () => {
872-
let captured = "";
875+
it("does not depend on a host-side timeout/gtimeout binary", () => {
876+
// The probe bounds itself via Node's spawn-level timeout, not a
877+
// host `timeout`/`gtimeout` wrapper (the latter is missing on
878+
// macOS by default).
879+
let captured: readonly string[] = [];
873880
probeContainerDns({
874881
runCaptureImpl: (command) => {
875882
captured = command;
876883
return BUSYBOX_SUCCESS;
877884
},
878885
});
879-
expect(captured.startsWith("docker run")).toBe(true);
880-
expect(captured.startsWith("timeout ")).toBe(false);
881-
expect(captured.startsWith("gtimeout ")).toBe(false);
886+
const script = captured[2] ?? "";
887+
expect(script).not.toMatch(/^\s*timeout\b/);
888+
expect(script).not.toMatch(/^\s*gtimeout\b/);
882889
});
883890
});
884891

@@ -930,12 +937,12 @@ describe("getDockerBridgeGatewayIp", () => {
930937
});
931938

932939
it("uses the expected docker network inspect command shape", () => {
933-
let captured = "";
940+
let captured: readonly string[] = [];
934941
getDockerBridgeGatewayIp((cmd) => {
935942
captured = cmd;
936943
return "172.17.0.1";
937944
});
938-
expect(captured).toContain("docker network inspect bridge");
939-
expect(captured).toContain("Gateway");
945+
expect(captured.slice(0, 4)).toEqual(["docker", "network", "inspect", "bridge"]);
946+
expect(captured).toContain("{{range .IPAM.Config}}{{.Gateway}}{{end}}");
940947
});
941948
});

0 commit comments

Comments
 (0)