fix(macos): add inference.local to sandbox /etc/hosts for Ollama#268
Closed
mihai-chiorean wants to merge 3 commits intoNVIDIA:mainfrom
Closed
fix(macos): add inference.local to sandbox /etc/hosts for Ollama#268mihai-chiorean wants to merge 3 commits intoNVIDIA:mainfrom
mihai-chiorean wants to merge 3 commits intoNVIDIA:mainfrom
Conversation
Contributor
|
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:
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 (2)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughAdds a macOS-only post-setup step that connects to the OpenShell sandbox and ensures an Changes
Sequence Diagram(s)sequenceDiagram
participant Host as Host (macOS)
participant Onboard as Onboard (bin/lib/onboard.js)
participant Setup as Setup script (scripts/setup.sh)
participant OpenShell as OpenShell CLI
participant Sandbox as Sandbox (/etc/hosts, /etc/resolv.conf)
Host->>Onboard: start sandbox creation
Onboard->>Onboard: start dashboard port-forward
Onboard->>Onboard: rm -rf "${buildCtx}"
Onboard->>OpenShell: openshell connect/forward (darwin only) via Setup
OpenShell->>Sandbox: read /etc/resolv.conf (get first nameserver)
OpenShell->>Sandbox: if missing, append "IP inference.local" to /etc/hosts
Sandbox-->>OpenShell: success / already present / error (ignored)
OpenShell-->>Onboard: return result (warn on failure)
Onboard->>Host: continue sandbox registration/completion
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ 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}
Contributor
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 223-234: The macOS branch that runs when process.platform ===
"darwin" builds a command that blindly extracts an IP from /etc/resolv.conf and
appends it as "IP inference.local" to /etc/hosts, which can produce an invalid
hosts entry if no nameserver exists; update the logic around the run(...) call
that references sandboxName so it first validates the extracted IP (e.g., ensure
the grep/awk result matches an IPv4 pattern) and only appends the hosts line
when a valid IP is found, otherwise skip the append or use a safe fallback
(e.g., host.docker.internal) and log a warning; keep this check adjacent to the
existing run invocation so the command executed is conditional on the validated
IP.
In `@scripts/setup.sh`:
- Around line 202-208: The sandbox /etc/hosts append command can write a blank
IP if no nameserver is found; modify the sh -c script run by openshell sandbox
connect nemoclaw to capture the nameserver into a variable, test it is
non-empty, and only then echo "$IP inference.local" >> /etc/hosts (otherwise
skip or log a warning). Locate the one-liner passed to openshell (the grep
nameserver /etc/resolv.conf | head -1 | awk "{print \$2}" pipeline) and replace
it with a small shell snippet that sets IP=$(grep -m1 '^nameserver'
/etc/resolv.conf | awk '{print $2}'), checks [ -n "$IP" ], and appends only when
non-empty.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 687166ef-e5fe-4e45-babb-eb4e9ce6ab8d
📒 Files selected for processing (2)
bin/lib/onboard.jsscripts/setup.sh
mihai-chiorean
force-pushed
the
fix/macos-ollama-dns
branch
from
9934718 to
4e6d61c
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
bin/lib/onboard.js (1)
530-531: Remove duplicate build-context cleanup.
buildCtxis already removed earlier, so this second cleanup is redundant in the success path.Proposed simplification
- // Clean up build context - run(`rm -rf "${buildCtx}"`, { ignoreError: true });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/lib/onboard.js` around lines 530 - 531, The second cleanup call is redundant—remove the duplicate run(`rm -rf "${buildCtx}"`, { ignoreError: true }) so buildCtx is only removed once; locate the duplicate invocation of run that references buildCtx in onboard.js (the cleanup near line ~530) and delete that extra call, ensuring any remaining cleanup logic still runs in error/final paths.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@bin/lib/onboard.js`:
- Around line 530-531: The second cleanup call is redundant—remove the duplicate
run(`rm -rf "${buildCtx}"`, { ignoreError: true }) so buildCtx is only removed
once; locate the duplicate invocation of run that references buildCtx in
onboard.js (the cleanup near line ~530) and delete that extra call, ensuring any
remaining cleanup logic still runs in error/final paths.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3a37e338-63f6-4cc4-b2b5-2d462917b479
📒 Files selected for processing (2)
bin/lib/onboard.jsscripts/setup.sh
🚧 Files skipped from review as they are similar to previous changes (1)
- scripts/setup.sh
On macOS (Docker Desktop), the sandbox cannot resolve inference.local because there is no network namespace setup like on Linux. This adds inference.local -> host gateway IP to /etc/hosts inside the sandbox after creation, both in the onboard wizard and legacy setup.sh.
Addresses CodeRabbit review: if /etc/resolv.conf has no nameserver entry, the previous code would append an invalid hosts line with a blank IP. Now validates the extracted IP is non-empty before appending, otherwise logs a warning and skips.
mihai-chiorean
force-pushed
the
fix/macos-ollama-dns
branch
from
4e6d61c to
6d3c20b
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 1506-1507: The duplicate cleanup call is redundant: remove the
second invocation of run(`rm -rf "${buildCtx}"`, { ignoreError: true }) so the
build context is only removed once; locate the later occurrence of run(...) that
references buildCtx in onboard.js and delete that redundant call, leaving the
original cleanup (the earlier run(...) call around where buildCtx is first
removed) intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 23408131-90e3-4da8-b0e9-aa31efb0148d
📒 Files selected for processing (2)
bin/lib/onboard.jsscripts/setup.sh
🚧 Files skipped from review as they are similar to previous changes (1)
- scripts/setup.sh
Comment on lines
+1506
to
+1507
| // Clean up build context | ||
| run(`rm -rf "${buildCtx}"`, { ignoreError: true }); |
Contributor
There was a problem hiding this comment.
Remove duplicate build context cleanup.
The build context is already removed at line 1454 (run(\rm -rf "${buildCtx}"`, { ignoreError: true });`). This second cleanup is redundant—the directory no longer exists by this point.
🧹 Proposed fix
runOpenshell(["forward", "start", "--background", "18789", sandboxName], { ignoreError: true });
- // Clean up build context
- run(`rm -rf "${buildCtx}"`, { ignoreError: true });
-
// macOS: add inference.local to sandbox /etc/hosts so Ollama DNS resolves.📝 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
| // Clean up build context | |
| run(`rm -rf "${buildCtx}"`, { ignoreError: true }); | |
| runOpenshell(["forward", "start", "--background", "18789", sandboxName], { ignoreError: true }); | |
| // macOS: add inference.local to sandbox /etc/hosts so Ollama DNS resolves. |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bin/lib/onboard.js` around lines 1506 - 1507, The duplicate cleanup call is
redundant: remove the second invocation of run(`rm -rf "${buildCtx}"`, {
ignoreError: true }) so the build context is only removed once; locate the later
occurrence of run(...) that references buildCtx in onboard.js and delete that
redundant call, leaving the original cleanup (the earlier run(...) call around
where buildCtx is first removed) intact.
mafueee
pushed a commit
to mafueee/NemoClaw
that referenced
this pull request
Mar 28, 2026
…#326) * feat(sandbox): log connection attempts that bypass proxy path Add iptables LOG + REJECT rules inside the sandbox network namespace to detect and diagnose direct connection attempts that bypass the HTTP CONNECT proxy. This provides two improvements: 1. Fast-fail UX: applications get immediate ECONNREFUSED instead of a 30-second timeout when they bypass the proxy 2. Diagnostics: a /dev/kmsg monitor emits structured BYPASS_DETECT tracing events with destination, protocol, process identity, and actionable hints Both TCP and UDP bypass attempts are covered (UDP catches DNS bypass). The feature degrades gracefully if iptables or /dev/kmsg are unavailable. Closes NVIDIA#268 * chore: track .python-version to pin Python 3.13.12 for uv The sandbox base image runs Python 3.13. A stale venv on 3.12 causes all exec_python E2E tests to fail because cloudpickle bytecode is not compatible across minor versions. * fix(cluster): preserve hostGatewayIP across fast deploys The fast deploy's helm upgrade was missing the hostGatewayIP value that the bootstrap entrypoint injects into the HelmChart CR. This caused host.openshell.internal hostAliases to be lost from the gateway pod and sandbox pods after any fast deploy, breaking host gateway routing. Read the IP from the HelmChart CR and pass it through to helm upgrade. * wip: fix iptables path resolution, use dmesg for kmsg, add CAP_SYSLOG * fix(sandbox): restore NetworkNamespace Drop impl, remove dead kmsg code The Drop impl for NetworkNamespace was accidentally deleted during the bypass detection refactor, which would cause network namespaces and veth interfaces to leak on every sandbox shutdown. Also removes dead kmsg volume/mount code (bypass monitor uses dmesg instead of direct /dev/kmsg access) and removes an accidentally committed session transcript file.
Author
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
On macOS (Docker Desktop), the sandbox cannot resolve
inference.localbecause there is no network namespace setup like on Linux. Ollama local inference fails silently.Fix: After sandbox creation, on Darwin, exec into the sandbox and add
inference.localpointing to the host gateway IP from/etc/resolv.confto/etc/hosts. Applied to both:bin/lib/onboard.js(onboard wizard)scripts/setup.sh(legacy setup path)Idempotent — skips if entry already exists.
Partially addresses #260 (Ollama DNS item)
Summary by CodeRabbit
New Features
Chores