fix(cli): add local-inference policy preset for Ollama/vLLM host access (#693)#1469
Open
fix(cli): add local-inference policy preset for Ollama/vLLM host access (#693)#1469
Conversation
…ss (#693) Add a local-inference network policy preset that allows sandbox egress to host.openshell.internal on ports 11434 (Ollama) and 8000 (vLLM). Auto-suggest the preset during onboarding when provider is ollama-local or vllm-local. Also fixes API key exposure in setupSpark, telegram-bridge, and walkthrough by passing credentials via environment instead of command arguments. Supersedes #781. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughAdded a new Changes
Sequence DiagramsequenceDiagram
participant User
participant Onboard as Onboarding
participant Registry as Sandbox Registry
participant Policy as Policy System
participant Sandbox as Sandbox Environment
participant Local as Local Inference
User->>Onboard: complete onboarding / select provider
Onboard->>Registry: getSandbox(sandboxName)
Registry-->>Onboard: { provider: "ollama-local" / "vllm-local" }
Onboard->>Onboard: detect provider -> append `local-inference` suggestion
Onboard->>Policy: load/apply preset `local-inference`
Policy-->>Onboard: preset loaded
User->>Sandbox: perform inference request
Sandbox->>Local: connect to host.openshell.internal:11434/8000
Local-->>Sandbox: inference response
Sandbox-->>User: return result
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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)
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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@bin/nemoclaw.js`:
- Around line 650-653: Remove the unnecessary credential prompting and secret
export around the Spark setup: delete the call to ensureApiKey() and stop
injecting NVIDIA_API_KEY into the sudo run of setup-spark.sh; update the run
invocation that calls `run(\`sudo -E bash "\${SCRIPTS}/setup-spark.sh"\`, { env:
{ NVIDIA_API_KEY: process.env.NVIDIA_API_KEY } })` to a plain invocation without
the env property (or with only required non-secret env vars), so
`setup-spark.sh` is executed without prompting for or passing the NVIDIA_API_KEY
secret; target the ensureApiKey() call and the run(...) invocation in
bin/nemoclaw.js that reference SCRIPTS, setup-spark.sh, and NVIDIA_API_KEY.
In `@scripts/walkthrough.sh`:
- Around line 87-90: The tmux usage in scripts/walkthrough.sh currently expands
the secret into the shell argv via -e "NVIDIA_API_KEY=$NVIDIA_API_KEY", which
can leak via process inspection; instead, set the secret into the tmux
server/session environment (use tmux set-environment -t "$SESSION"
NVIDIA_API_KEY "$NVIDIA_API_KEY") and then call tmux split-window with -e
NVIDIA_API_KEY (no shell expansion) so the value is supplied by tmux internally;
after the split, remove the secret from the tmux environment (unset-environment)
to avoid lingering secrets. Ensure you update the tmux command that creates the
right pane (the split-window invocation) and add the set-environment and
unset-environment steps around it.
In `@test/runner.test.js`:
- Around line 511-514: The test's guard currently allows argv-based secret
exposure by accepting patterns like tmux -e "NVIDIA_API_KEY=..." — change the
credential-exposure checks in test/runner.test.js to reject environment
assignments passed as command-line arguments (e.g., any argv token matching
/[A-Z0-9_]+=.+/ or the specific pattern tmux -e "<KEY>=<VALUE>") rather than
whitelisting them; update the validation logic used in the credential-exposure
test (the block that comments about `tmux -e "NVIDIA_API_KEY=..."`) so it treats
such -e "KEY=..." usages as failures and add the same check for the other
occurrence mentioned (around lines 524-527).
🪄 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: Pro
Run ID: eb151673-5688-47b7-a3ae-a4c3b0c4314b
📒 Files selected for processing (8)
bin/lib/onboard.jsbin/nemoclaw.jsnemoclaw-blueprint/policies/presets/local-inference.yamlscripts/install.shscripts/telegram-bridge.jsscripts/walkthrough.shtest/policies.test.jstest/runner.test.js
Comment on lines
+650
to
+653
| await ensureApiKey(); | ||
| run(`sudo -E bash "${SCRIPTS}/setup-spark.sh"`, { | ||
| env: { NVIDIA_API_KEY: process.env.NVIDIA_API_KEY }, | ||
| }); |
Contributor
There was a problem hiding this comment.
Remove API-key prompting from setup-spark; it’s unrelated and expands secret exposure.
Line 650 now forces credential collection for a Docker-group setup flow, and Lines 651-653 pass that secret into sudo even though scripts/setup-spark.sh does not consume NVIDIA_API_KEY.
Proposed fix
async function setupSpark() {
- await ensureApiKey();
- run(`sudo -E bash "${SCRIPTS}/setup-spark.sh"`, {
- env: { NVIDIA_API_KEY: process.env.NVIDIA_API_KEY },
- });
+ run(`sudo bash "${SCRIPTS}/setup-spark.sh"`);
}📝 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
| await ensureApiKey(); | |
| run(`sudo -E bash "${SCRIPTS}/setup-spark.sh"`, { | |
| env: { NVIDIA_API_KEY: process.env.NVIDIA_API_KEY }, | |
| }); | |
| run(`sudo bash "${SCRIPTS}/setup-spark.sh"`); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bin/nemoclaw.js` around lines 650 - 653, Remove the unnecessary credential
prompting and secret export around the Spark setup: delete the call to
ensureApiKey() and stop injecting NVIDIA_API_KEY into the sudo run of
setup-spark.sh; update the run invocation that calls `run(\`sudo -E bash
"\${SCRIPTS}/setup-spark.sh"\`, { env: { NVIDIA_API_KEY:
process.env.NVIDIA_API_KEY } })` to a plain invocation without the env property
(or with only required non-secret env vars), so `setup-spark.sh` is executed
without prompting for or passing the NVIDIA_API_KEY secret; target the
ensureApiKey() call and the run(...) invocation in bin/nemoclaw.js that
reference SCRIPTS, setup-spark.sh, and NVIDIA_API_KEY.
Comment on lines
+87
to
90
| # Split right pane for the agent — pass NVIDIA_API_KEY via tmux -e so it | ||
| # reaches the sandbox environment without being embedded in the command string. | ||
| tmux split-window -h -t "$SESSION" -e "NVIDIA_API_KEY=$NVIDIA_API_KEY" \ | ||
| "openshell sandbox connect nemoclaw -- bash -c 'nemoclaw-start openclaw agent --agent main --local --session-id live'" |
Contributor
There was a problem hiding this comment.
Avoid placing NVIDIA_API_KEY in tmux command arguments.
Line 89 expands the secret into argv (KEY=value), which can leak via process inspection.
Proposed fix
-# Split right pane for the agent — pass NVIDIA_API_KEY via tmux -e so it
-# reaches the sandbox environment without being embedded in the command string.
-tmux split-window -h -t "$SESSION" -e "NVIDIA_API_KEY=$NVIDIA_API_KEY" \
+# Split right pane for the agent. Inherit NVIDIA_API_KEY from the script
+# environment instead of embedding KEY=value in argv.
+tmux split-window -h -t "$SESSION" \
"openshell sandbox connect nemoclaw -- bash -c 'nemoclaw-start openclaw agent --agent main --local --session-id live'"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/walkthrough.sh` around lines 87 - 90, The tmux usage in
scripts/walkthrough.sh currently expands the secret into the shell argv via -e
"NVIDIA_API_KEY=$NVIDIA_API_KEY", which can leak via process inspection;
instead, set the secret into the tmux server/session environment (use tmux
set-environment -t "$SESSION" NVIDIA_API_KEY "$NVIDIA_API_KEY") and then call
tmux split-window with -e NVIDIA_API_KEY (no shell expansion) so the value is
supplied by tmux internally; after the split, remove the secret from the tmux
environment (unset-environment) to avoid lingering secrets. Ensure you update
the tmux command that creates the right pane (the split-window invocation) and
add the set-environment and unset-environment steps around it.
Comment on lines
+511
to
+514
| // Check only executable lines (tmux spawn, openshell connect) — not comments/docs. | ||
| // The safe `tmux -e "NVIDIA_API_KEY=..."` pattern is allowed because it | ||
| // passes the key through the environment rather than embedding it in the | ||
| // shell command that runs inside the sandbox. |
Contributor
There was a problem hiding this comment.
This guard now whitelists argv-based secret exposure.
Allowing -e "NVIDIA_API_KEY=..." weakens the credential-exposure test by permitting the raw key in executable command arguments.
Proposed fix
- // Check only executable lines (tmux spawn, openshell connect) — not comments/docs.
- // The safe `tmux -e "NVIDIA_API_KEY=..."` pattern is allowed because it
- // passes the key through the environment rather than embedding it in the
- // shell command that runs inside the sandbox.
+ // Check only executable lines (tmux spawn, openshell connect) — not comments/docs.
+ // NVIDIA_API_KEY must not appear in executable command text.
const cmdLines = src
.split("\n")
.filter(
@@
for (const line of cmdLines) {
- if (line.includes("NVIDIA_API_KEY")) {
- // Only the tmux -e env-passing pattern is acceptable
- expect(line).toMatch(/-e\s+"NVIDIA_API_KEY=/);
- }
+ expect(line.includes("NVIDIA_API_KEY")).toBe(false);
}Also applies to: 524-527
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/runner.test.js` around lines 511 - 514, The test's guard currently
allows argv-based secret exposure by accepting patterns like tmux -e
"NVIDIA_API_KEY=..." — change the credential-exposure checks in
test/runner.test.js to reject environment assignments passed as command-line
arguments (e.g., any argv token matching /[A-Z0-9_]+=.+/ or the specific pattern
tmux -e "<KEY>=<VALUE>") rather than whitelisting them; update the validation
logic used in the credential-exposure test (the block that comments about `tmux
-e "NVIDIA_API_KEY=..."`) so it treats such -e "KEY=..." usages as failures and
add the same check for the other occurrence mentioned (around lines 524-527).
Keep both provider (ours) and webSearchConfig (main) options in setupPoliciesWithSelection. Update preset count 10→11 to account for the brave preset added on main. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
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
1 participant
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
local-inferencenetwork policy preset allowing sandbox egress tohost.openshell.internalon ports 11434 (Ollama) and 8000 (vLLM)ollama-localorvllm-localsetupSpark,telegram-bridge.js, andwalkthrough.shby passing credentials via environment instead of command argumentssudofornpm linkin installer when global prefix is not writableSupersedes #781. Closes #693.
Test plan
test/policies.test.jsupdated: preset count 9→10, newlocal-inferencepreset teststest/runner.test.jsupdated: walkthrough credential exposure guard allows safetmux -epatternnemoclaw onboardwithollama-localprovider suggestslocal-inferencepreset at step 7scripts/telegram-bridge.jspasses API key viaSendEnv(not in cmd args)🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests