fix: add local-inference policy preset for Ollama/vLLM host access (#693)#781
fix: add local-inference policy preset for Ollama/vLLM host access (#693)#781cluster2600 wants to merge 9 commits intoNVIDIA:mainfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughChanges add local inference provider support with auto-detection in onboarding, a new policy preset for local inference endpoints, secret redaction in command logging, refactored API key propagation via environment variables instead of inline exports, and updated tests validating the new preset. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~35 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/policies.test.js (1)
99-124: Good test coverage for the new preset.The tests thoroughly validate the key aspects of the
local-inferencepreset: host resolution, port configuration, binaries section, and policy extraction.Minor note: Line 115 references
#676in the comment, but this PR addresses issue#693. Consider updating the comment for accuracy.📝 Suggested fix
it("has a binaries section", () => { const content = policies.loadPreset("local-inference"); - assert.ok(content.includes("binaries:"), "must have binaries section (ref: `#676`)"); + assert.ok(content.includes("binaries:"), "must have binaries section (ref: `#693`)"); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/policies.test.js` around lines 99 - 124, Update the test assertion message that references the wrong issue number: locate the "has a binaries section" test in policies.test.js (the assertion checking content.includes("binaries:") whose message contains "(ref: `#676`)") and change the reference to the correct issue number "(ref: `#693`)". Ensure the assertion message string is updated exactly where "binaries:" check is performed so the test comment matches this PR.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/telegram-bridge.js`:
- Around line 105-114: Add an e2e test that verifies NVIDIA_API_KEY is
transmitted via SendEnv by invoking the same ssh spawn flow (the code path that
builds safeSessionId, cmd and calls spawn with env including NVIDIA_API_KEY and
option "-o SendEnv=NVIDIA_API_KEY") but have the remote command (the
openshell-${SANDBOX} target) echo/print the received NVIDIA_API_KEY value or run
a small script that returns it; the test should assert the returned value
matches the local API_KEY, failing if the env is empty or differs, and should
run the exact ssh argument pattern used by spawn to exercise SendEnv behavior.
---
Nitpick comments:
In `@test/policies.test.js`:
- Around line 99-124: Update the test assertion message that references the
wrong issue number: locate the "has a binaries section" test in policies.test.js
(the assertion checking content.includes("binaries:") whose message contains
"(ref: `#676`)") and change the reference to the correct issue number "(ref:
`#693`)". Ensure the assertion message string is updated exactly where "binaries:"
check is performed so the test comment matches this PR.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 73e5aa37-fcf8-420b-a793-b4abb1c71825
⛔ Files ignored due to path filters (1)
research/results.tsvis excluded by!**/*.tsv
📒 Files selected for processing (10)
bin/lib/onboard.jsbin/lib/runner.jsbin/nemoclaw.jsnemoclaw-blueprint/policies/presets/local-inference.yamlscripts/install.shscripts/telegram-bridge.jsscripts/walkthrough.shtest/policies.test.jstest/runner.test.jsuninstall.sh
|
Fixed the issue reference in the test assertion — was referencing #676 instead of #693. Cheers for catching that. Regarding the out-of-scope warning: this branch is stacked on top of the test fixes (#779) and secret redaction (#780) PRs. The local-inference preset changes are in the last commit. Happy to rebase once the base PRs land if that's preferred. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/policies.test.js (1)
12-14: Avoid hardcoding preset count separately from the canonical expected list.Line 14 duplicates the source of truth already maintained in the expected names list, which increases maintenance drift risk.
♻️ Suggested simplification
+ const expected = ["discord", "docker", "huggingface", "jira", "local-inference", "npm", "outlook", "pypi", "slack", "telegram"]; + it("returns all 10 presets", () => { const presets = policies.listPresets(); - assert.equal(presets.length, 10); + assert.equal(presets.length, expected.length); }); @@ it("returns expected preset names", () => { const names = policies.listPresets().map((p) => p.name).sort(); - const expected = ["discord", "docker", "huggingface", "jira", "local-inference", "npm", "outlook", "pypi", "slack", "telegram"]; assert.deepEqual(names, expected); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/policies.test.js` around lines 12 - 14, The test hardcodes the number 10 for preset count which duplicates the canonical expected list; change the assertion to derive the expected length from the existing expected names array (e.g., use expectedPresets.length or expectedNames.length) or assert equality against the expected list returned by policies.listPresets(); update the assertion in the test that calls policies.listPresets() so it compares lengths or contents to the single source-of-truth variable instead of the literal 10.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/policies.test.js`:
- Around line 107-123: The test currently uses string includes on
policies.loadPreset output which can falsely pass if host, ports, and binaries
appear in different policy blocks; update the test to parse the preset content
(e.g., using a YAML parser) and then locate the "local_inference" entry returned
by policies.extractPresetEntries or by parsing the YAML, then assert that within
that single local_inference object the host "host.openshell.internal", ports
11434 and 8000 (or the corresponding port fields), and a non-empty "binaries"
section all exist; use the existing functions policies.loadPreset and
policies.extractPresetEntries to obtain content/entries or parse the YAML and
perform property checks on the same policy object rather than independent string
matches.
---
Nitpick comments:
In `@test/policies.test.js`:
- Around line 12-14: The test hardcodes the number 10 for preset count which
duplicates the canonical expected list; change the assertion to derive the
expected length from the existing expected names array (e.g., use
expectedPresets.length or expectedNames.length) or assert equality against the
expected list returned by policies.listPresets(); update the assertion in the
test that calls policies.listPresets() so it compares lengths or contents to the
single source-of-truth variable instead of the literal 10.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: decd4a58-6aeb-425b-87bd-b9874186f028
📒 Files selected for processing (1)
test/policies.test.js
|
Heads up: there's a merge conflict from upstream test file changes (looks like the test suite was migrated to Vitest). Happy to rebase once the dust settles on the current merge queue. The core changes (local-inference preset YAML + onboard auto-suggestion) are conflict-free. |
cluster2600
force-pushed
the
fix/03-local-inference-preset
branch
from
a39537b to
ae8d91a
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
scripts/telegram-bridge.js (1)
146-146: Consider redacting stderr before including in response.While the command no longer embeds secrets,
stderrfrom the remote process could theoretically contain sensitive output. For defense-in-depth consistency withrunner.jschanges, consider importing and applyingredactSecrets:♻️ Optional: add redaction to stderr output
-const { shellQuote, validateName } = require("../bin/lib/runner"); +const { shellQuote, validateName, redactSecrets } = require("../bin/lib/runner");Then at line 146:
- resolve(`Agent exited with code ${code}. ${stderr.trim().slice(0, 500)}`); + resolve(`Agent exited with code ${code}. ${redactSecrets(stderr.trim().slice(0, 500))}`);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/telegram-bridge.js` at line 146, The response currently interpolates raw stderr into the resolved message in scripts/telegram-bridge.js; import and use the existing redactSecrets function to sanitize stderr before including it in the resolve call. Specifically, ensure redactSecrets is imported/required at top of the module and replace the direct use of stderr.trim().slice(0, 500) in the resolve(`Agent exited...`) expression with a redacted version (e.g., redactSecrets(stderr).trim().slice(0, 500)) so the message contains sanitized output while preserving the existing truncation and formatting.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@scripts/telegram-bridge.js`:
- Line 146: The response currently interpolates raw stderr into the resolved
message in scripts/telegram-bridge.js; import and use the existing redactSecrets
function to sanitize stderr before including it in the resolve call.
Specifically, ensure redactSecrets is imported/required at top of the module and
replace the direct use of stderr.trim().slice(0, 500) in the resolve(`Agent
exited...`) expression with a redacted version (e.g.,
redactSecrets(stderr).trim().slice(0, 500)) so the message contains sanitized
output while preserving the existing truncation and formatting.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5983e3ed-a22e-4432-97cc-d646ae2cda3e
⛔ Files ignored due to path filters (1)
research/results.tsvis excluded by!**/*.tsv
📒 Files selected for processing (10)
bin/lib/onboard.jsbin/lib/runner.jsbin/nemoclaw.jsnemoclaw-blueprint/policies/presets/local-inference.yamlscripts/install.shscripts/telegram-bridge.jsscripts/walkthrough.shtest/policies.test.jstest/runner.test.jsuninstall.sh
✅ Files skipped from review due to trivial changes (2)
- uninstall.sh
- nemoclaw-blueprint/policies/presets/local-inference.yaml
🚧 Files skipped from review as they are similar to previous changes (6)
- scripts/walkthrough.sh
- bin/nemoclaw.js
- scripts/install.sh
- bin/lib/onboard.js
- test/runner.test.js
- test/policies.test.js
|
@cluster2600 Thank you very much for all this information! Claude Code and i are going to have a field day with finally getting this setup running this evening. i will report back tomorrow with my findings. my usecase/system is complex but is probably very similar to anyone running a gaming desktop and a separate server for docker. in my case, ryzen + 6700xt desktop hosting models, and a fully kitted out 12c/64gb trashcan mac pro running proxmox running docker VMs for my production lab |
cluster2600
force-pushed
the
fix/03-local-inference-preset
branch
from
ae8d91a to
81e8949
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/lib/onboard.js`:
- Around line 2007-2011: The code currently reads
registry.getSandbox(sandboxName).provider (sandboxProvider) which is often null
because registration doesn't set provider; instead detect the gateway by falling
back to the environment-set GATEWAY_NAME used by setupInference. Change the
detection to read sandbox.provider || sandbox.env?.GATEWAY_NAME (or other place
you set GATEWAY_NAME) before comparing to "ollama-local" / "vllm-local" so
suggestions.push("local-inference") fires; reference registry.getSandbox, the
sandboxProvider variable, setupInference/GATEWAY_NAME, and the
suggestions.push("local-inference") call to locate and update the logic.
In `@scripts/install.sh`:
- Around line 424-428: The root install.sh is missing the npm global prefix
writability check used in scripts/install.sh (NPM_GLOBAL_PREFIX="$(npm config
get prefix 2>/dev/null)" || true and the subsequent [ -n "$NPM_GLOBAL_PREFIX" ]
&& [ ! -w "$NPM_GLOBAL_PREFIX" ] && [ "$(id -u)" -ne 0 ] check that sets
SUDO="sudo"), so update the root install.sh to compute NPM_GLOBAL_PREFIX and set
SUDO the same way, then use that SUDO variable for both npm link invocations in
the root script (ensure both npm link calls are prefixed with $SUDO when
required) so behavior matches the logic in scripts/install.sh.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ad84f2b1-9181-4de9-a15e-8e7701790b3a
⛔ Files ignored due to path filters (1)
research/results.tsvis excluded by!**/*.tsv
📒 Files selected for processing (9)
bin/lib/onboard.jsbin/lib/runner.jsbin/nemoclaw.jsnemoclaw-blueprint/policies/presets/local-inference.yamlscripts/install.shscripts/telegram-bridge.jsscripts/walkthrough.shtest/policies.test.jsuninstall.sh
✅ Files skipped from review due to trivial changes (2)
- scripts/telegram-bridge.js
- nemoclaw-blueprint/policies/presets/local-inference.yaml
🚧 Files skipped from review as they are similar to previous changes (5)
- scripts/walkthrough.sh
- uninstall.sh
- bin/lib/runner.js
- bin/nemoclaw.js
- test/policies.test.js
|
I was able to get up and running last night Community Tips: Getting Local Ollama Working with NemoClaw (PR #781)Posted by: basharder Setup: Ubuntu 24.04 Docker host + Fedora desktop (AMD RX 6700 XT) running Ollama Hey — wanted to share what I learned getting this working end-to-end after PR #781 merged the What PR #781 Fixes (and What It Doesn't)The PR correctly adds Step 1: Apply the local-inference preset during onboardingAt step 7/7 (Policy presets), type Step 2: The policy needs
|
| Log message | Fix |
|---|---|
| not in policy 'local_inference' | Policy not loaded — run openshell policy set --wait |
| binary '/usr/bin/curl' not allowed | Add curl/bash/node to binaries: in policy |
| endpoint has L7 rules; use CONNECT | Replace rules: with access: full |
| internal IP without allowed_ips | Add allowed_ips: [] to endpoint |
| resolves to internal address 172.17.0.1 | Fix DNS — patch Sandbox CRD and CoreDNS |
AMD GPU Notes (RX 6700 XT / RDNA2)
Ollama supports AMD via ROCm but the 6700 XT isn't auto-detected. Add to the ollama systemd override on your Fedora/Linux machine:
[Service]
Environment="OLLAMA_HOST=0.0.0.0"
Environment="HSA_OVERRIDE_GFX_VERSION=10.3.0"
With 12GB VRAM, gpt-oss:20b runs at roughly 19% GPU / 81% CPU. Despite the CPU spill, the MoE architecture (only 3.6B active params) keeps it surprisingly fast. llama3.1:8b fits entirely in 12GB and runs 100% GPU.
Hope this helps others with the same setup. Happy to answer questions.
|
Hey @cluster2600 — sorry about this, but #961 landing has introduced merge conflicts here as well. I know you already flagged this was coming back on March 24th and offered to rebase once the dust settled — and I realize this is the second rebase ask today on top of #782. I appreciate your patience with all the unplanned extra work, especially given the great community writeup from @basharder showing how valuable this preset is. When you get a chance, could you rebase on main and resolve the conflicts? The core changes (local-inference preset YAML + onboard auto-suggestion) should still be clean as you predicted. This one is also in the critical path for the CLI migration (#924). Thanks again, and sorry for the churn. |
cluster2600
force-pushed
the
fix/03-local-inference-preset
branch
from
d841ba1 to
84451a0
Compare
|
Rebased — no worries about the churn, happy to keep it tidy. The preset YAML and auto-suggestion came through clean as expected. |
- uninstall.sh: check parent directory writability instead of file writability when deciding whether sudo is needed for removal. The previous check followed symlinks, so a symlink to a user-writable target in a root-owned directory would attempt unprivileged rm and fail with EACCES. - scripts/install.sh: replace NODE_MGR-based sudo heuristic with a direct writability check on the npm global prefix. The old approach always used sudo for nodesource installs, which changes PATH and breaks environments where the prefix is already user-writable.
…in process args (NVIDIA#579, NVIDIA#664) Add redactSecrets() to runner.js with patterns matching env assignments, nvapi- prefixes, GitHub PATs, and Bearer tokens. Apply to error messages in run() and runInteractive() so failed commands never leak credentials. Fix setupSpark() to pass NVIDIA_API_KEY via env option instead of interpolating into the command string (visible in ps aux). Fix walkthrough.sh to use tmux -e for env propagation instead of embedding the key in the shell command. Fix telegram-bridge.js to use SSH SendEnv instead of command-line export. 9 new tests covering redaction patterns and regression guards.
…VIDIA#693) RFC1918 addresses were blocked by the egress proxy because no policy entry existed for host.openshell.internal. Add a local-inference preset that whitelists ports 11434 (Ollama) and 8000 (vLLM) with binary restrictions, and auto-suggest it during onboard when a local inference provider is selected.
Signed-off-by: Maxime Grenu <maxime.grenu@gmail.com>
Show the jq command for extracting the token from inside the sandbox and the openshell download path for host-side retrieval. Replaces the vague ~/.openclaw/ reference which was misleading since the config lives at /sandbox/.openclaw/ inside the container. Signed-off-by: Maxime Grenu <maxime.grenu@gmail.com>
cluster2600
force-pushed
the
fix/03-local-inference-preset
branch
from
84451a0 to
8792b94
Compare
research/results.tsv
Outdated
There was a problem hiding this comment.
Should this have been .gitignored?
There was a problem hiding this comment.
hello, it was .gitignored but still tracked.
There was a problem hiding this comment.
Good catch — yes, research/ was already in .gitignore but this file was tracked before the rule was added. Removed it from the index in the latest push.
research/ was already in .gitignore but results.tsv was still tracked. Signed-off-by: Maxime Grenu <maxime.grenu@gmail.com>
wscurran
added
Local Models
|
✨ Thanks for submitting this PR with a detailed summary, it proposes a fix to improve the functionality of NemoClaw, which could enhance the user experience. |
…ript modules (#1240) ## Summary - Extract ~210 lines of pure, side-effect-free functions from the 3,800-line `onboard.js` into **5 typed TypeScript modules** under `src/lib/`: - `gateway-state.ts` — gateway/sandbox state classification from openshell output - `validation.ts` — failure classification, API key validation, model ID checks - `url-utils.ts` — URL normalization, text compaction, env formatting - `build-context.ts` — Docker build context filtering, recovery hints - `dashboard.ts` — dashboard URL resolution and construction - Add **56 co-located unit tests** (`src/lib/*.test.ts`) for the extracted modules - Set up CLI TypeScript compilation: `tsconfig.src.json` compiles `src/` → `dist/` as CJS - `onboard.js` imports from compiled `dist/lib/` output — transparent to callers - Pre-commit hook updated to build TS and include `dist/lib/` in coverage These functions are **not touched by any #924 blocker PR** (#781, #782, #819, #672, #634, #890), so this extraction is safe to land immediately. ## Test plan - [x] 598 CLI tests pass (542 existing + 56 new) - [x] `tsc -p tsconfig.src.json` compiles cleanly - [x] `tsc -p tsconfig.cli.json` type-checks cleanly - [x] `tsc -p jsconfig.json` type-checks cleanly - [x] Coverage ratchet passes with `dist/lib/` included Closes #1237. Relates to #924 (shell consolidation). 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Improved sandbox-creation recovery hints and targeted remediation commands. * Smarter dashboard URL resolution and control-UI URL construction. * **Bug Fixes** * More accurate gateway and sandbox state detection. * Enhanced classification of validation/apply failures and safer model/key validation. * Better provider URL normalization and loopback handling. * **Tests** * Added comprehensive tests covering new utilities. * **Chores** * CLI now builds before CLI tests; CI/commit hooks updated to run the CLI build. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ript modules (#1240) ## Summary - Extract ~210 lines of pure, side-effect-free functions from the 3,800-line `onboard.js` into **5 typed TypeScript modules** under `src/lib/`: - `gateway-state.ts` — gateway/sandbox state classification from openshell output - `validation.ts` — failure classification, API key validation, model ID checks - `url-utils.ts` — URL normalization, text compaction, env formatting - `build-context.ts` — Docker build context filtering, recovery hints - `dashboard.ts` — dashboard URL resolution and construction - Add **56 co-located unit tests** (`src/lib/*.test.ts`) for the extracted modules - Set up CLI TypeScript compilation: `tsconfig.src.json` compiles `src/` → `dist/` as CJS - `onboard.js` imports from compiled `dist/lib/` output — transparent to callers - Pre-commit hook updated to build TS and include `dist/lib/` in coverage These functions are **not touched by any #924 blocker PR** (#781, #782, #819, #672, #634, #890), so this extraction is safe to land immediately. ## Test plan - [x] 598 CLI tests pass (542 existing + 56 new) - [x] `tsc -p tsconfig.src.json` compiles cleanly - [x] `tsc -p tsconfig.cli.json` type-checks cleanly - [x] `tsc -p jsconfig.json` type-checks cleanly - [x] Coverage ratchet passes with `dist/lib/` included Closes #1237. Relates to #924 (shell consolidation). 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Improved sandbox-creation recovery hints and targeted remediation commands. * Smarter dashboard URL resolution and control-UI URL construction. * **Bug Fixes** * More accurate gateway and sandbox state detection. * Enhanced classification of validation/apply failures and safer model/key validation. * Better provider URL normalization and loopback handling. * **Tests** * Added comprehensive tests covering new utilities. * **Chores** * CLI now builds before CLI tests; CI/commit hooks updated to run the CLI build. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
cv
left a comment
cv
left a comment
There was a problem hiding this comment.
Thanks for the policy preset work — the local-inference preset and auto-detection logic are solid.
However, this PR needs a rebase and scope reduction before it can land:
1. Drop the runner.js secret redaction. #1246 just merged with a more comprehensive implementation (stdio interception, URL credential redaction, partial reveal for debuggability). The redactSecrets() added here now conflicts with that work. Please rebase onto main and remove the runner.js changes — the redaction is already handled.
2. Consider splitting the remaining changes. The policy preset, the env var propagation fixes (sudo -E, SSH SendEnv, tmux -e), and the install.sh sudo logic are three independent concerns. The preset is the core fix for #693; the others could be separate PRs. Up to you whether to split or keep bundled, but the runner.js part must go.
3. CI is red (DCO + checks). A rebase onto current main should fix this.
The preset YAML, onboard auto-detection, and test additions all look good — just need the cleanup above.
4 participants
Summary
graph LR S[Sandbox] -->|local-inference preset| G[host.openshell.internal] G -->|:11434| O[Ollama] G -->|:8000| V[vLLM] style S fill:#bbf style O fill:#9f9 style V fill:#9f9New
local-inference.yamlpreset allowshost.openshell.internalon ports 11434 (Ollama) and 8000 (vLLM) with binaries restriction. Onboard auto-suggests this preset when a local provider is selected.4 new tests. Closes #693.
Test plan
Summary by CodeRabbit
New Features
Bug Fixes
Tests