refactor(cli): make runner helpers argv-only by default - #2584
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughReplaces shell-interpolated command strings with structured argv arrays across runner APIs and call sites; adds explicit shell entry points ( Changes
Sequence Diagram(s)sequenceDiagram
participant Dev as Developer/CLI
participant Runner as Local Runner
participant LocalFS as Local FS / rsync/scp
participant Remote as Remote Host (ssh)
participant Docker as Docker Engine
Dev->>Runner: start deploy (argv array)
Runner->>LocalFS: run rsync/scp (argv)
LocalFS-->>Runner: transfer result
Runner->>Remote: ssh (argv) run remote setup/install
Remote->>Docker: manage volumes/services (argv docker ...)
Remote-->>Runner: exit status
Runner-->>Dev: report result
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 docstrings
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib/deploy.ts (1)
414-444:⚠️ Potential issue | 🔴 CriticalRemove
shellQuote()from rsync and scp remote targets.The
run()function usesspawnSync()without shell interpretation—arguments are passed directly to the executable as argv. WhenshellQuote()is applied to remote paths like${name}:${shellQuote(...)}, the single quotes become literal characters in the argument, causing rsync and scp to fail when trying to access paths with embedded quotes.Keep shell quoting only in ssh command strings (lines 413, 444, 464, 478, 502), where commands are executed by the remote shell.
Suggested fix
- `${name}:${shellQuote(`${remoteDir}/`)}`, + `${name}:${remoteDir}/`, @@ - run(["scp", "-q", ...sshArgs, envTmp, `${name}:${shellQuote(`${remoteDir}/.env`)}`]); + run(["scp", "-q", ...sshArgs, envTmp, `${name}:${remoteDir}/.env`]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/deploy.ts` around lines 414 - 444, The rsync/scp remote target arguments are being wrapped with shellQuote (e.g., `${name}:${shellQuote(`${remoteDir}/`)}` and `${name}:${shellQuote(`${remoteDir}/.env`)}`) even though run() invokes commands via spawnSync without a shell, which makes the single-quote characters literal and breaks rsync/scp; update the call sites that build remote targets (the rsync invocation and the scp invocation that uses envTmp) to pass unquoted remote target strings (combine name + ":" + remoteDir/remote file path directly) and leave shellQuote only where commands are sent to the remote shell via ssh (the ssh run() calls), ensuring run(["rsync", ... , `${name}:${remoteDir}/`]) and run(["scp", ... , `${name}:${remoteDir}/.env`]) style arguments instead of using shellQuote.
🤖 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/nemoclaw.ts`:
- Around line 173-182: The docker volume name filter is substring-based and may
match unrelated volumes; update removeGatewayClusterVolumes to compute the
prefix const prefix = `openshell-cluster-${NEMOCLAW_GATEWAY_NAME}`, then after
obtaining names from _runCapture only keep those that start with that prefix
(use startsWith) before calling run(["docker","volume","rm",...]). Keep the
existing _runCapture and run calls but add the prefix-based filter and an early
return when the filtered list is empty to avoid deleting non-prefixed volumes.
---
Outside diff comments:
In `@src/lib/deploy.ts`:
- Around line 414-444: The rsync/scp remote target arguments are being wrapped
with shellQuote (e.g., `${name}:${shellQuote(`${remoteDir}/`)}` and
`${name}:${shellQuote(`${remoteDir}/.env`)}`) even though run() invokes commands
via spawnSync without a shell, which makes the single-quote characters literal
and breaks rsync/scp; update the call sites that build remote targets (the rsync
invocation and the scp invocation that uses envTmp) to pass unquoted remote
target strings (combine name + ":" + remoteDir/remote file path directly) and
leave shellQuote only where commands are sent to the remote shell via ssh (the
ssh run() calls), ensuring run(["rsync", ... , `${name}:${remoteDir}/`]) and
run(["scp", ... , `${name}:${remoteDir}/.env`]) style arguments instead of using
shellQuote.
🪄 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: 02801d94-8f6a-4f98-bb4f-20ffcabdaf3b
📒 Files selected for processing (10)
src/lib/deploy.tssrc/lib/onboard.tssrc/lib/runner-argv.test.tssrc/lib/runner.tssrc/nemoclaw.tstest/cli.test.tstest/gateway-cleanup.test.tstest/gateway-liveness-probe.test.tstest/onboard-selection.test.tstest/runner.test.ts
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib/onboard.ts (1)
4605-4614:⚠️ Potential issue | 🟠 MajorWSL install-ollama path is inconsistent with the WSL-safe startup logic.
Line [4605] always binds Ollama to
0.0.0.0and the block always starts the auth proxy, but the regular Ollama path explicitly avoids that on WSL. This can break reachability on WSL installs and applies the wrong post-install flow.💡 Proposed fix
- runShell(`OLLAMA_HOST=0.0.0.0:${OLLAMA_PORT} ollama serve > /dev/null 2>&1 &`, { + const installOllamaEnv = isWsl() ? "" : `OLLAMA_HOST=0.0.0.0:${OLLAMA_PORT} `; + runShell(`${installOllamaEnv}ollama serve > /dev/null 2>&1 &`, { ignoreError: true, }); sleep(2); - if (!startOllamaAuthProxy()) { - process.exit(1); - } - console.log( - ` ✓ Using Ollama on localhost:${OLLAMA_PORT} (proxy on :${OLLAMA_PROXY_PORT})`, - ); + if (isWsl()) { + console.log(` ✓ Using Ollama on localhost:${OLLAMA_PORT}`); + } else { + if (!startOllamaAuthProxy()) { + process.exit(1); + } + console.log( + ` ✓ Using Ollama on localhost:${OLLAMA_PORT} (proxy on :${OLLAMA_PROXY_PORT})`, + ); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/onboard.ts` around lines 4605 - 4614, The code always binds Ollama to 0.0.0.0 and always calls startOllamaAuthProxy() which is inconsistent with the WSL-safe path; change the runShell invocation and proxy startup to be conditional on the WSL check used elsewhere (e.g., use the same isWSL or isWsl detection), so that when running on WSL you bind to localhost (127.0.0.1) and skip startOllamaAuthProxy(), otherwise bind to 0.0.0.0:${OLLAMA_PORT} and start the proxy; update the console message to reflect the correct host/port and only reference ${OLLAMA_PROXY_PORT} when the proxy was actually started.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/lib/onboard.ts`:
- Around line 4605-4614: The code always binds Ollama to 0.0.0.0 and always
calls startOllamaAuthProxy() which is inconsistent with the WSL-safe path;
change the runShell invocation and proxy startup to be conditional on the WSL
check used elsewhere (e.g., use the same isWSL or isWsl detection), so that when
running on WSL you bind to localhost (127.0.0.1) and skip
startOllamaAuthProxy(), otherwise bind to 0.0.0.0:${OLLAMA_PORT} and start the
proxy; update the console message to reflect the correct host/port and only
reference ${OLLAMA_PROXY_PORT} when the proxy was actually started.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 32b117d0-b088-4934-982f-e8bb4938f5ff
📒 Files selected for processing (1)
src/lib/onboard.ts
Collaborator
Automated PR review summaryReviewed PR #2584: refactor(cli): make runner helpers argv-only by default Recommendation
Installation and setup findings
What was validated
Failing tests and unresolved impact
Passing tests and why they matteredPassing test 1: Runner rejects implicit shell strings while explicit shell helpers still work
Passing test 2: Deploy argv migration resists metacharacter injection in exercised command construction
Passing test 3: Real OpenShell sandbox remained reachable for end-to-end review context
Bottom line
|
brandonpelfrey
approved these changes
Apr 28, 2026
DemianHeyGen
pushed a commit
to DemianHeyGen/NemoClaw
that referenced
this pull request
Apr 30, 2026
## Summary This PR makes `run()` and `runInteractive()` argv-only so shell parsing is no longer implicit in the default runner APIs. It adds explicit shell-only helpers for the few cases that still need `bash -c`, and migrates existing callers to either argv execution or those explicit shell boundaries. ## Changes - Make `src/lib/runner.ts` reject string input for `run()` and `runInteractive()`, and add explicit `runShell()` / `runInteractiveShell()` helpers. - Convert deploy, onboard, and CLI gateway-cleanup paths to argv execution where shell parsing is not required, including `ssh`, `scp`, `rsync`, `docker inspect`, `docker volume`, and `kill` calls. - Keep the true shell cases explicit in onboarding (backgrounded Ollama startup and `curl | sh`) and update runner/CLI/onboard tests to cover the new contracts. ## 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** * Hardened command execution by moving from shell-interpolated strings to structured argv-style invocations, and introduced explicit shell-only execution paths for commands that need shell features, improving reliability and security. * Safer Docker volume cleanup via explicit enumeration and bulk deletion to avoid fragile shell pipelines. * **Tests** * Updated and tightened tests to enforce argv usage, validate shell-only helpers, and reflect the new command-invocation patterns. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
This was referenced May 1, 2026
21 tasks
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
3 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 PR makes
run()andrunInteractive()argv-only so shell parsing is no longer implicit in the default runner APIs. It adds explicit shell-only helpers for the few cases that still needbash -c, and migrates existing callers to either argv execution or those explicit shell boundaries.Changes
src/lib/runner.tsreject string input forrun()andrunInteractive(), and add explicitrunShell()/runInteractiveShell()helpers.ssh,scp,rsync,docker inspect,docker volume, andkillcalls.curl | sh) and update runner/CLI/onboard tests to cover the new contracts.Type of Change
Verification
npx prek run --all-filespassesnpm testpassesmake docsbuilds without warnings (doc changes only)AI Disclosure
Signed-off-by: Carlos Villela cvillela@nvidia.com
Summary by CodeRabbit
Refactor
Tests