fix(onboard): bypass host proxy for managed inference hostname - #4897
Merged
Conversation
Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
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 (2)
📝 WalkthroughWalkthroughThis PR expands the NO_PROXY/no_proxy allowlist injected into subprocess environments to include additional local hostnames (notably ChangesNO_PROXY Allowlist Expansion for Local Inference
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 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 |
laitingsheng
added
area: sandbox
Contributor
E2E Advisor RecommendationRequired E2E: Dispatch hint: Auto-dispatched E2E: Full advisor summaryE2E Recommendation AdvisorBase: Required E2E
Optional E2E
New E2E recommendations
Dispatch hint
|
Contributor
E2E Scenario Advisor RecommendationRequired scenario E2E: Dispatch required scenario E2E:
Full scenario advisor summaryE2E Scenario AdvisorBase: Required scenario E2E
Optional scenario E2E
Relevant changed files
|
Contributor
PR Review AdvisorFindings: 0 needs attention, 1 worth checking, 0 nice ideas Review findings🛠️ Needs attention
🔎 Worth checking
🌱 Nice ideas
Consider writing more tests for
This is an automated advisory review. A human maintainer must make the final merge decision. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
nemoclaw/src/lib/subprocess-env.ts (1)
63-66: ⚡ Quick winMinor formatting inconsistency with the CLI mirror.
The CLI version (
src/lib/subprocess-env.tsline 63) chains.split(",").map((s) => s.trim()).filter(Boolean)on a single line, while this plugin version spreads it across lines 63-66. Per the mirror-file note (line 17-18), these files should stay in sync.Consider aligning the formatting to match the CLI version for consistency.
♻️ Align with CLI formatting
- const parts = current - .split(",") - .map((s) => s.trim()) - .filter(Boolean); + const parts = current.split(",").map((s) => s.trim()).filter(Boolean);🤖 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 `@nemoclaw/src/lib/subprocess-env.ts` around lines 63 - 66, The file has a formatting mismatch for the expression that builds parts; change the multi-line chain in the const parts assignment to the single-line chained form used in the CLI mirror: replace the current newline-separated ".split(",").map((s) => s.trim()).filter(Boolean)" sequence with a single-line chain on the same statement where const parts = current.split(",").map((s) => s.trim()).filter(Boolean); so the expression in subprocess-env.ts (the const parts assignment) matches the CLI formatting.
🤖 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 `@nemoclaw/src/lib/subprocess-env.ts`:
- Around line 63-66: The file has a formatting mismatch for the expression that
builds parts; change the multi-line chain in the const parts assignment to the
single-line chained form used in the CLI mirror: replace the current
newline-separated ".split(",").map((s) => s.trim()).filter(Boolean)" sequence
with a single-line chain on the same statement where const parts =
current.split(",").map((s) => s.trim()).filter(Boolean); so the expression in
subprocess-env.ts (the const parts assignment) matches the CLI formatting.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: be3efee2-52b7-4b58-b8dc-ec91bc808e39
📒 Files selected for processing (6)
nemoclaw/src/lib/subprocess-env.tssrc/lib/onboard/http-proxy-preflight.test.tssrc/lib/onboard/http-proxy-preflight.tssrc/lib/subprocess-env.test.tssrc/lib/subprocess-env.tstest/credential-exposure.test.ts
Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
laitingsheng
added
the
area: e2e
Contributor
Selective E2E Results — ✅ All requested jobs passedRun: 27067948583
|
Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
Contributor
|
🌿 Preview your docs: https://nvidia-preview-pr-4897.docs.buildwithfern.com/nemoclaw |
Contributor
Selective E2E Results — ✅ All requested jobs passedRun: 27069185612
|
cv
approved these changes
Jun 6, 2026
Contributor
Selective E2E Results — ✅ All requested jobs passedRun: 27069621748
|
Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
Contributor
Selective E2E Results — ✅ All requested jobs passedRun: 27071742298
|
Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/lib/subprocess-env.test.ts (2)
110-119: ⚡ Quick winConsider checking both
NO_PROXYandno_proxyfor consistency.The implementation in
withLocalNoProxymodifies bothNO_PROXYandno_proxy(Context snippet 1), and earlier tests (lines 96-101, 103-108) verify both variables. For consistency and completeness, consider adding an assertion forno_proxyas well.♻️ Proposed enhancement for test completeness
it("does not inject a broad .local suffix or arbitrary *.local hostnames", () => { const env: Record<string, string> = { HTTP_PROXY: "http://127.0.0.1:8118" }; withLocalNoProxy(env); const parts = (env.NO_PROXY ?? "").split(","); expect(parts).not.toContain(".local"); expect(parts).not.toContain("*.local"); expect(parts).not.toContain("evil.local"); expect(parts).not.toContain("attacker.local"); expect(parts.filter((p) => p.endsWith(".local"))).toEqual(["inference.local"]); + const noParts = (env.no_proxy ?? "").split(","); + expect(noParts.filter((p) => p.endsWith(".local"))).toEqual(["inference.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 `@src/lib/subprocess-env.test.ts` around lines 110 - 119, The test only asserts against env.NO_PROXY but the helper withLocalNoProxy updates both NO_PROXY and no_proxy; update this test to also inspect env.no_proxy the same way (split on commas and assert it does not contain ".local", "*.local", "evil.local", "attacker.local" and that the only .local suffix present is "inference.local") so both NO_PROXY and no_proxy are validated consistently alongside the existing checks in withLocalNoProxy.
121-133: ⚡ Quick winConsider checking both
NO_PROXYandno_proxyfor consistency.The test sets both
NO_PROXYandno_proxyin the input (lines 124-125), but only verifiesNO_PROXYin the assertions. SincewithLocalNoProxymodifies both variables and earlier tests verify both, consider adding assertions forno_proxyas well.♻️ Proposed enhancement for test completeness
it("preserves a caller-provided .local entry without expanding the bypass", () => { const env: Record<string, string> = { HTTP_PROXY: "http://127.0.0.1:8118", NO_PROXY: "trusted.local", no_proxy: "trusted.local", }; withLocalNoProxy(env); const parts = (env.NO_PROXY ?? "").split(","); expect(parts).toContain("trusted.local"); expect(parts).toContain("inference.local"); expect(parts).not.toContain(".local"); expect(parts).not.toContain("*.local"); + const noParts = (env.no_proxy ?? "").split(","); + expect(noParts).toContain("trusted.local"); + expect(noParts).toContain("inference.local"); + expect(noParts).not.toContain(".local"); + expect(noParts).not.toContain("*.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 `@src/lib/subprocess-env.test.ts` around lines 121 - 133, The test currently only asserts against env.NO_PROXY even though the input sets both NO_PROXY and no_proxy and withLocalNoProxy updates both; update the test for completeness by also reading and splitting env.no_proxy (e.g., const lowerParts = (env.no_proxy ?? "").split(",")) and add the same expectations used for parts (expect(lowerParts).toContain("trusted.local"); expect(lowerParts).toContain("inference.local"); expect(lowerParts).not.toContain(".local"); expect(lowerParts).not.toContain("*.local");) so both NO_PROXY and no_proxy are validated after calling withLocalNoProxy.
🤖 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 `@src/lib/subprocess-env.test.ts`:
- Around line 110-119: The test only asserts against env.NO_PROXY but the helper
withLocalNoProxy updates both NO_PROXY and no_proxy; update this test to also
inspect env.no_proxy the same way (split on commas and assert it does not
contain ".local", "*.local", "evil.local", "attacker.local" and that the only
.local suffix present is "inference.local") so both NO_PROXY and no_proxy are
validated consistently alongside the existing checks in withLocalNoProxy.
- Around line 121-133: The test currently only asserts against env.NO_PROXY even
though the input sets both NO_PROXY and no_proxy and withLocalNoProxy updates
both; update the test for completeness by also reading and splitting
env.no_proxy (e.g., const lowerParts = (env.no_proxy ?? "").split(",")) and add
the same expectations used for parts
(expect(lowerParts).toContain("trusted.local");
expect(lowerParts).toContain("inference.local");
expect(lowerParts).not.toContain(".local");
expect(lowerParts).not.toContain("*.local");) so both NO_PROXY and no_proxy are
validated after calling withLocalNoProxy.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 3c817a82-e866-46e7-82db-0543064aaebd
📒 Files selected for processing (3)
nemoclaw/src/lib/subprocess-env.tssrc/lib/subprocess-env.test.tssrc/lib/subprocess-env.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- nemoclaw/src/lib/subprocess-env.ts
- src/lib/subprocess-env.ts
Contributor
Selective E2E Results — ✅ All requested jobs passedRun: 27072262172
|
Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
Contributor
Selective E2E Results — ✅ All requested jobs passedRun: 27082239620
|
Contributor
Selective E2E Results — ✅ All requested jobs passedRun: 27082427593
|
12 tasks
miyoungc
added a commit
that referenced
this pull request
Jun 8, 2026
## Summary - Add the v0.0.61 release notes from the GitHub dev announcement. - Document managed vLLM recovery after host reboot and Slack denied-mention feedback. - Refresh generated `nemoclaw-user-*` skills from the source docs. ## Source summary - #4983 -> `docs/about/release-notes.mdx`: Added the v0.0.61 release summary from the dev announcement and linked behavior groups to deeper docs. - #4904 -> `docs/inference/use-local-inference.mdx`: Documented that managed vLLM restarts the `nemoclaw-vllm` container after host reboot during recovery. - #4933 -> `docs/manage-sandboxes/messaging-channels.mdx`: Documented Slack sender feedback for denied channel `@mention` events. - #4879, #4915, #4935, #4759, #4164, #4888, #4897, #4944, #4959 -> `.agents/skills/`: Refreshed generated user skills from the current source docs for release prep. ## Verification - `python3 scripts/docs-to-skills.py docs/ .agents/skills/ --prefix nemoclaw-user --doc-platform fern-mdx` - `npm run docs` (passed outside the tool sandbox after `tsx` IPC pipe creation was blocked in the sandbox) - `npm run build:cli` (refreshed local `dist/` for the pre-push TypeScript hook) - Commit and pre-push hooks passed, including docs-to-skills verification, markdownlint, gitleaks, skills YAML tests, and CLI TypeScript. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Documentation** * Updated sandbox security documentation with file descriptor limits. * Changed default inference model for DGX Station profile. * Enhanced agent policy and backup/restore documentation. * Improved command reference examples with clearer formatting. * Clarified Slack messaging denial notice behavior. * Added automatic vLLM container recovery during host reboot. * Updated release notes for v0.0.61. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
6 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
Follow-up to PR #4331 (
fix(onboard): forward host proxy env into sandbox). #4331 started forwarding the hostHTTP_PROXY/HTTPS_PROXY/NO_PROXYintoopenshell sandbox create -- env ...so OpenShell-internal traffic could traverse the host proxy when one is configured. The forwardedNO_PROXYseed list did not yet include the OpenShell-managed inference hostname (inference.local) or the rootless container-host alias (host.containers.internal). When a hostHTTP_PROXYis active on macOS + Colima, OpenShell's L7 proxy chainsinference.localthrough the host proxy — which cannot reach an OpenShell-internal hostname — and streaming chat completions stall until the 120 s idle timeout fires (#4846).This PR seeds
inference.localandhost.containers.internalinto thewithLocalNoProxy()augmentation so the regression introduced by #4331's host-proxy forwarding stops short of the managed inference hostname.Related Issue
Fixes #4846 (regression source: #4331).
Changes
src/lib/subprocess-env.tsandnemoclaw/src/lib/subprocess-env.ts(mirrored): appendinference.localandhost.containers.internalto the seededNO_PROXYhost list inwithLocalNoProxy(); JSDoc documents the boundary (host-side subprocesses +openshell sandbox create -- env ...) and the removal condition.src/lib/onboard/http-proxy-preflight.ts: warning copy now names the managed inference hostname and includes it in the suggestedexport NO_PROXY=line for tools running outside the sandbox. Suppression now also requiresinference.localso users with the previous loopback-only guidance still see the new export advice.docs/reference/troubleshooting.mdx: document the full set of host names NemoClaw adds toNO_PROXYwhen a host HTTP proxy is detected.src/lib/subprocess-env.test.tsandsrc/lib/onboard/http-proxy-preflight.test.ts, plustest/host-proxy-inference-local-e2e.test.ts(curl-driven E2E provinginference.localreaches a local listener directly whenHTTP_PROXYis set).subprocess-env.test.tsalso adds negative tests proving arbitrary*.local/.localhostnames are not added to the bypass and that a caller-provided.localentry does not expand the bypass surface. CLI / plugin sync test intest/credential-exposure.test.tsupdated to match the new host list.Scope and boundaries
*.localsuffix. Deliberately narrow: only exactinference.localis seeded. The reported failure path is the single OpenShell-managed inference hostname; widening to any*.localwould change the bypass surface beyond the reported repro without evidence of other affected hostnames. A negative test (subprocess-env.test.ts) pins this behaviour.NO_PROXYis unchanged.scripts/nemoclaw-start.shcontinues to setNO_PROXY=localhost,127.0.0.1,::1,${PROXY_HOST}inside the sandbox and intentionally does not includeinference.localthere — OpenShell's L7 proxy resolves that hostname internally, and bypassing it would force a direct DNS lookup that does not resolve from inside the container. This PR does not touch that runtime layer.withLocalNoProxy()runs throughappendHostProxyEnvArgs()intoopenshell sandbox create -- env .... TheNO_PROXYentry is consumed by OpenShell at sandbox-create time when it decides whether to chain its L7 proxy through the hostHTTP_PROXYfor a given hostname. Withinference.localinNO_PROXYat sandbox-create time, OpenShell stops tunneling that hostname through the host proxy (the reportedProxy connection error: Broken pipe (os error 32)path).test/host-proxy-inference-local-e2e.test.tsexercises the env-construction primitive end-to-end via a realcurlspawn driven bybuildSubprocessEnv()against a local listener bound toinference.local. The full macOS + Colima sandbox path requires a macOS runner and is left to scenario E2E (ubuntu-repo-cloud-openclawcovers the closest cloud-onboard cross-section).Type of Change
Verification
npx prek run --all-filespassesnpm testpassesnpm run docsbuilds without warnings (doc changes only)Summary by CodeRabbit
Bug Fixes
Documentation
Tests
Signed-off-by: Tinson Lai tinsonl@nvidia.com