fix: support local vLLM inference routing#3013
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:
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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR extends the proxy bypass list in ChangesNO_PROXY expansion for local inference hosts
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 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 unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/nemoclaw-start.sh`:
- Around line 433-440: The NO_PROXY_EXTRA assignment currently uses
${NEMOCLAW_NO_PROXY_EXTRA-...} which discards the default hosts when
NEMOCLAW_NO_PROXY_EXTRA is set; change the logic so that when
NEMOCLAW_NO_PROXY_EXTRA is defined its value is appended to (not replacing) the
default hosts (host.openshell.internal, host.docker.internal), e.g. build
NO_PROXY_EXTRA from NEMOCLAW_NO_PROXY_EXTRA plus the defaults, then keep the
existing trimming (NO_PROXY_EXTRA="${NO_PROXY_EXTRA#,}" and
NO_PROXY_EXTRA="${NO_PROXY_EXTRA%,}") and the downstream use of NO_PROXY_EXTRA
when composing _NO_PROXY_VAL and the proxy variables.
🪄 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: 5d5f732a-bc05-4aa2-9941-67843b0ebcf4
📒 Files selected for processing (4)
scripts/nemoclaw-start.shsrc/lib/inference-config.test.tssrc/lib/inference-config.tstest/service-env.test.js
| NO_PROXY_EXTRA="${NEMOCLAW_NO_PROXY_EXTRA-host.openshell.internal,host.docker.internal}" | ||
| NO_PROXY_EXTRA="${NO_PROXY_EXTRA#,}" | ||
| NO_PROXY_EXTRA="${NO_PROXY_EXTRA%,}" | ||
| _PROXY_URL="http://${PROXY_HOST}:${PROXY_PORT}" | ||
| _NO_PROXY_VAL="localhost,127.0.0.1,::1,${PROXY_HOST}" | ||
| if [ -n "$NO_PROXY_EXTRA" ]; then | ||
| _NO_PROXY_VAL="${_NO_PROXY_VAL},${NO_PROXY_EXTRA}" | ||
| fi |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Confirm the current expansion in the reviewed file
rg -n 'NO_PROXY_EXTRA="\$\{NEMOCLAW_NO_PROXY_EXTRA-.*\}"' scripts/nemoclaw-start.sh
# 2) Reproduce the current expansion semantics safely (inline, not executing repo scripts)
bash -lc '
unset NEMOCLAW_NO_PROXY_EXTRA
NO_PROXY_EXTRA="${NEMOCLAW_NO_PROXY_EXTRA-host.openshell.internal,host.docker.internal}"
echo "unset => $NO_PROXY_EXTRA"
NEMOCLAW_NO_PROXY_EXTRA="172.17.0.1"
NO_PROXY_EXTRA="${NEMOCLAW_NO_PROXY_EXTRA-host.openshell.internal,host.docker.internal}"
echo "custom => $NO_PROXY_EXTRA"
NEMOCLAW_NO_PROXY_EXTRA=""
NO_PROXY_EXTRA="${NEMOCLAW_NO_PROXY_EXTRA-host.openshell.internal,host.docker.internal}"
echo "empty => <$NO_PROXY_EXTRA>"
'Repository: NVIDIA/NemoClaw
Length of output: 356
🏁 Script executed:
# Check file existence, shebang, and SPDX headers
head -5 scripts/nemoclaw-start.sh
# Check the actual code context around the reviewed lines
sed -n '430,445p' scripts/nemoclaw-start.shRepository: NVIDIA/NemoClaw
Length of output: 953
NEMOCLAW_NO_PROXY_EXTRA currently replaces defaults instead of appending additional hosts
Line 433 uses ${NEMOCLAW_NO_PROXY_EXTRA-...} which applies the default only when the variable is unset. When NEMOCLAW_NO_PROXY_EXTRA is set to any value (including custom hosts), the defaults host.openshell.internal and host.docker.internal are discarded. This contradicts the "EXTRA" naming convention, which implies appending additional hosts rather than replacing them.
💡 Suggested fix
-NO_PROXY_EXTRA="${NEMOCLAW_NO_PROXY_EXTRA-host.openshell.internal,host.docker.internal}"
-NO_PROXY_EXTRA="${NO_PROXY_EXTRA#,}"
-NO_PROXY_EXTRA="${NO_PROXY_EXTRA%,}"
+DEFAULT_NO_PROXY_EXTRA="host.openshell.internal,host.docker.internal"
+NO_PROXY_EXTRA="$DEFAULT_NO_PROXY_EXTRA"
+if [ "${NEMOCLAW_NO_PROXY_EXTRA+x}" = "x" ]; then
+ NO_PROXY_EXTRA="${NEMOCLAW_NO_PROXY_EXTRA#,}"
+ NO_PROXY_EXTRA="${NO_PROXY_EXTRA%,}"
+ if [ -n "$NO_PROXY_EXTRA" ]; then
+ NO_PROXY_EXTRA="${DEFAULT_NO_PROXY_EXTRA},${NO_PROXY_EXTRA}"
+ fi
+fi📝 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.
| NO_PROXY_EXTRA="${NEMOCLAW_NO_PROXY_EXTRA-host.openshell.internal,host.docker.internal}" | |
| NO_PROXY_EXTRA="${NO_PROXY_EXTRA#,}" | |
| NO_PROXY_EXTRA="${NO_PROXY_EXTRA%,}" | |
| _PROXY_URL="http://${PROXY_HOST}:${PROXY_PORT}" | |
| _NO_PROXY_VAL="localhost,127.0.0.1,::1,${PROXY_HOST}" | |
| if [ -n "$NO_PROXY_EXTRA" ]; then | |
| _NO_PROXY_VAL="${_NO_PROXY_VAL},${NO_PROXY_EXTRA}" | |
| fi | |
| DEFAULT_NO_PROXY_EXTRA="host.openshell.internal,host.docker.internal" | |
| NO_PROXY_EXTRA="$DEFAULT_NO_PROXY_EXTRA" | |
| if [ "${NEMOCLAW_NO_PROXY_EXTRA+x}" = "x" ]; then | |
| NO_PROXY_EXTRA="${NEMOCLAW_NO_PROXY_EXTRA#,}" | |
| NO_PROXY_EXTRA="${NO_PROXY_EXTRA%,}" | |
| if [ -n "$NO_PROXY_EXTRA" ]; then | |
| NO_PROXY_EXTRA="${DEFAULT_NO_PROXY_EXTRA},${NO_PROXY_EXTRA}" | |
| fi | |
| fi | |
| _PROXY_URL="http://${PROXY_HOST}:${PROXY_PORT}" | |
| _NO_PROXY_VAL="localhost,127.0.0.1,::1,${PROXY_HOST}" | |
| if [ -n "$NO_PROXY_EXTRA" ]; then | |
| _NO_PROXY_VAL="${_NO_PROXY_VAL},${NO_PROXY_EXTRA}" | |
| fi |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/nemoclaw-start.sh` around lines 433 - 440, The NO_PROXY_EXTRA
assignment currently uses ${NEMOCLAW_NO_PROXY_EXTRA-...} which discards the
default hosts when NEMOCLAW_NO_PROXY_EXTRA is set; change the logic so that when
NEMOCLAW_NO_PROXY_EXTRA is defined its value is appended to (not replacing) the
default hosts (host.openshell.internal, host.docker.internal), e.g. build
NO_PROXY_EXTRA from NEMOCLAW_NO_PROXY_EXTRA plus the defaults, then keep the
existing trimming (NO_PROXY_EXTRA="${NO_PROXY_EXTRA#,}" and
NO_PROXY_EXTRA="${NO_PROXY_EXTRA%,}") and the downstream use of NO_PROXY_EXTRA
when composing _NO_PROXY_VAL and the proxy variables.
ff368d5 to
7f95059
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
scripts/nemoclaw-start.sh (1)
1320-1350: Run sandbox E2Es for this entrypoint change before mergeThis file is the sandbox boot entrypoint; these proxy-path changes are high-impact and can be missed by unit tests.
As per coding guidelines,
scripts/nemoclaw-start.sh: “This file is a sandbox entrypoint script. Changes affect every sandbox boot and are invisible to unit tests… E2E test recommendation: sandbox-survival-e2e, sandbox-operations-e2e, cloud-e2e.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/nemoclaw-start.sh` around lines 1320 - 1350, This change to the sandbox entrypoint modifies proxy behavior (see PROXY_HOST, PROXY_PORT, NO_PROXY_EXTRA, _PROXY_URL, and _NO_PROXY_VAL in scripts/nemoclaw-start.sh) and must be validated with sandbox E2E tests before merging; run the recommended sandbox-survival-e2e, sandbox-operations-e2e and cloud-e2e against this branch/entrypoint to ensure HTTP_PROXY/HTTPS_PROXY/NO_PROXY handling (including the gateway IP and NO_PROXY_EXTRA host aliases like host.openshell.internal/host.docker.internal) does not break sandbox boot, DNS/inference.local resolution, or local inference runtimes (vLLM/Ollama) connectivity, and address any failures by adjusting NO_PROXY_EXTRA handling or proxy env assembly accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@scripts/nemoclaw-start.sh`:
- Around line 1320-1350: This change to the sandbox entrypoint modifies proxy
behavior (see PROXY_HOST, PROXY_PORT, NO_PROXY_EXTRA, _PROXY_URL, and
_NO_PROXY_VAL in scripts/nemoclaw-start.sh) and must be validated with sandbox
E2E tests before merging; run the recommended sandbox-survival-e2e,
sandbox-operations-e2e and cloud-e2e against this branch/entrypoint to ensure
HTTP_PROXY/HTTPS_PROXY/NO_PROXY handling (including the gateway IP and
NO_PROXY_EXTRA host aliases like host.openshell.internal/host.docker.internal)
does not break sandbox boot, DNS/inference.local resolution, or local inference
runtimes (vLLM/Ollama) connectivity, and address any failures by adjusting
NO_PROXY_EXTRA handling or proxy env assembly accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: d6c92bc1-d9c3-4f17-a345-8af7909c9667
📒 Files selected for processing (1)
scripts/nemoclaw-start.sh
|
✨ Thanks for submitting this PR that proposes a refactor of the sandbox to improve support for local vLLM inference routing. This change involves adding default no_proxy bypasses for host.openshell.internal and host.docker.internal, and using the vllm-local default model when building the OpenClaw primary model. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
scripts/nemoclaw-start.sh (1)
1330-1360: Run the sandbox E2E subset before merge for this entrypoint proxy change.This path executes on every sandbox boot, so unit tests can miss lifecycle regressions in real runtime environments.
As per coding guidelines:
scripts/nemoclaw-start.shis a sandbox entrypoint script and changes are “invisible to unit tests,” with recommended runs forsandbox-survival-e2e,sandbox-operations-e2e, andcloud-e2e.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/nemoclaw-start.sh` around lines 1330 - 1360, This change modifies the sandbox entrypoint proxy logic (variables PROXY_HOST, PROXY_PORT, NO_PROXY_EXTRA and the export of HTTP_PROXY/NO_PROXY in the start script), so before merging run the sandbox E2E suites to catch runtime lifecycle regressions: execute sandbox-survival-e2e, sandbox-operations-e2e, and cloud-e2e against the updated entrypoint to validate proxy behavior and inference.local resolution in real sandbox boots; if failures appear, revert or adjust the NO_PROXY_EXTRA/NO_PROXY_VAL logic and re-run until the E2E suites pass.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@scripts/nemoclaw-start.sh`:
- Around line 1330-1360: This change modifies the sandbox entrypoint proxy logic
(variables PROXY_HOST, PROXY_PORT, NO_PROXY_EXTRA and the export of
HTTP_PROXY/NO_PROXY in the start script), so before merging run the sandbox E2E
suites to catch runtime lifecycle regressions: execute sandbox-survival-e2e,
sandbox-operations-e2e, and cloud-e2e against the updated entrypoint to validate
proxy behavior and inference.local resolution in real sandbox boots; if failures
appear, revert or adjust the NO_PROXY_EXTRA/NO_PROXY_VAL logic and re-run until
the E2E suites pass.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 3a278dea-77ca-41f8-9da2-f5dd0309040f
📒 Files selected for processing (4)
scripts/nemoclaw-start.shsrc/lib/inference-config.test.tssrc/lib/inference-config.tstest/service-env.test.ts
e5f4db9 to
193d5ce
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/service-env.test.ts (1)
263-263: ⚡ Quick winExtract a shared NO_PROXY fixture constant to prevent drift.
The same
_NO_PROXY_VALliteral is duplicated across many wrappers. Centralizing it will keep expectations aligned and reduce future inconsistencies.Refactor sketch
+const DEFAULT_NO_PROXY_LITERAL = + "localhost,127.0.0.1,::1,${PROXY_HOST},host.openshell.internal,host.docker.internal"; // ... - '_NO_PROXY_VAL="localhost,127.0.0.1,::1,${PROXY_HOST},host.openshell.internal,host.docker.internal"', + `_NO_PROXY_VAL="${DEFAULT_NO_PROXY_LITERAL}"`,Also applies to: 303-303, 522-522, 633-633, 678-678, 726-726, 803-803, 847-847, 891-891, 931-931
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/service-env.test.ts` at line 263, Extract the repeated '_NO_PROXY_VAL="localhost,127.0.0.1,::1,${PROXY_HOST},host.openshell.internal,host.docker.internal"' literal into a single exported test constant (e.g., NO_PROXY_FIXTURE or _NO_PROXY_VAL) in test/service-env.test.ts and replace each duplicated string occurrence with that constant (references include the occurrences currently at lines ~263, 303, 522, 633, 678, 726, 803, 847, 891, 931 as reported). Ensure any test helpers or wrapper builders that currently inline the literal (search for the exact '_NO_PROXY_VAL=...' string) import or reference the new constant so all expectations use the shared fixture.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/service-env.test.ts`:
- Around line 497-506: The test for NEMOCLAW_NO_PROXY_EXTRA currently asserts
that providing extras removes default bypass hosts; update the expectation to
verify extras are appended instead of replacing defaults: in the test using
extractProxyVars({ NEMOCLAW_NO_PROXY_EXTRA: "172.17.0.1,vllm.local" }) and
inspecting vars.NO_PROXY split by ",", assert that it contains "10.200.0.1" and
also contains "172.17.0.1" and "vllm.local" and preserve (expect toContain) the
default "host.openshell.internal" rather than expecting it to be absent.
---
Nitpick comments:
In `@test/service-env.test.ts`:
- Line 263: Extract the repeated
'_NO_PROXY_VAL="localhost,127.0.0.1,::1,${PROXY_HOST},host.openshell.internal,host.docker.internal"'
literal into a single exported test constant (e.g., NO_PROXY_FIXTURE or
_NO_PROXY_VAL) in test/service-env.test.ts and replace each duplicated string
occurrence with that constant (references include the occurrences currently at
lines ~263, 303, 522, 633, 678, 726, 803, 847, 891, 931 as reported). Ensure any
test helpers or wrapper builders that currently inline the literal (search for
the exact '_NO_PROXY_VAL=...' string) import or reference the new constant so
all expectations use the shared fixture.
🪄 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: 534e2491-f31e-4621-be2e-d0b5f4cce539
📒 Files selected for processing (4)
scripts/nemoclaw-start.shsrc/lib/inference-config.test.tssrc/lib/inference-config.tstest/service-env.test.ts
✅ Files skipped from review due to trivial changes (1)
- scripts/nemoclaw-start.sh
🚧 Files skipped from review as they are similar to previous changes (1)
- src/lib/inference-config.ts
| it("NEMOCLAW_NO_PROXY_EXTRA appends custom bypass hosts", () => { | ||
| const vars = extractProxyVars({ | ||
| NEMOCLAW_NO_PROXY_EXTRA: "172.17.0.1,vllm.local", | ||
| }); | ||
| const noProxy = vars.NO_PROXY.split(","); | ||
| expect(noProxy).toContain("10.200.0.1"); | ||
| expect(noProxy).toContain("172.17.0.1"); | ||
| expect(noProxy).toContain("vllm.local"); | ||
| expect(noProxy).not.toContain("host.openshell.internal"); | ||
| }); |
There was a problem hiding this comment.
NEMOCLAW_NO_PROXY_EXTRA test asserts non-additive behavior.
Line 497 says extras are appended, but Line 505 expects a default local bypass host to disappear. That encodes override semantics and can hide regressions in local routing.
Suggested test expectation adjustment
it("NEMOCLAW_NO_PROXY_EXTRA appends custom bypass hosts", () => {
const vars = extractProxyVars({
NEMOCLAW_NO_PROXY_EXTRA: "172.17.0.1,vllm.local",
});
const noProxy = vars.NO_PROXY.split(",");
expect(noProxy).toContain("10.200.0.1");
+ expect(noProxy).toContain("host.openshell.internal");
+ expect(noProxy).toContain("host.docker.internal");
expect(noProxy).toContain("172.17.0.1");
expect(noProxy).toContain("vllm.local");
- expect(noProxy).not.toContain("host.openshell.internal");
});📝 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.
| it("NEMOCLAW_NO_PROXY_EXTRA appends custom bypass hosts", () => { | |
| const vars = extractProxyVars({ | |
| NEMOCLAW_NO_PROXY_EXTRA: "172.17.0.1,vllm.local", | |
| }); | |
| const noProxy = vars.NO_PROXY.split(","); | |
| expect(noProxy).toContain("10.200.0.1"); | |
| expect(noProxy).toContain("172.17.0.1"); | |
| expect(noProxy).toContain("vllm.local"); | |
| expect(noProxy).not.toContain("host.openshell.internal"); | |
| }); | |
| it("NEMOCLAW_NO_PROXY_EXTRA appends custom bypass hosts", () => { | |
| const vars = extractProxyVars({ | |
| NEMOCLAW_NO_PROXY_EXTRA: "172.17.0.1,vllm.local", | |
| }); | |
| const noProxy = vars.NO_PROXY.split(","); | |
| expect(noProxy).toContain("10.200.0.1"); | |
| expect(noProxy).toContain("host.openshell.internal"); | |
| expect(noProxy).toContain("host.docker.internal"); | |
| expect(noProxy).toContain("172.17.0.1"); | |
| expect(noProxy).toContain("vllm.local"); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/service-env.test.ts` around lines 497 - 506, The test for
NEMOCLAW_NO_PROXY_EXTRA currently asserts that providing extras removes default
bypass hosts; update the expectation to verify extras are appended instead of
replacing defaults: in the test using extractProxyVars({
NEMOCLAW_NO_PROXY_EXTRA: "172.17.0.1,vllm.local" }) and inspecting vars.NO_PROXY
split by ",", assert that it contains "10.200.0.1" and also contains
"172.17.0.1" and "vllm.local" and preserve (expect toContain) the default
"host.openshell.internal" rather than expecting it to be absent.
193d5ce to
3864d4c
Compare
Summary
lateststill routes sandbox requests forhost.openshell.internalthrough the policy proxy unless the host alias is inNO_PROXYhost.openshell.internalandhost.docker.internal, plusNEMOCLAW_NO_PROXY_EXTRAfor additional local inference bypass hostsvllm-localdefault model when building the OpenClaw primary model while preserving the dedicated local vLLM credential envRelated Issue
Supersession check
latest/v0.0.36already carries the larger DGX/vLLM onboarding work, so this branch was force-updated down to the one still-needed functional commitNO_PROXY=localhost,127.0.0.1,::1,10.200.0.1causesGET host.openshell.internal:8000/v1/modelsto be denied by policy, while the updated bypass list reaches vLLM successfullyTests
npm run build:clinpm test -- src/lib/inference-config.test.ts test/service-env.test.tsnpm run format:checkgit diff --check origin/main...HEADSummary by CodeRabbit
New Features
NEMOCLAW_NO_PROXY_EXTRAenvironment variable. Defaults to Docker and OpenShell internal hosts when unset.Tests