fix(onboard): route runCaptureEx env through buildRunnerEnv to inject NO_PROXY (#2616) - #3801
Conversation
… NO_PROXY (#2616) PR #2662 added withLocalNoProxy() and wired buildSubprocessEnv into the direct spawn sites in inference/ollama/proxy.ts (auth proxy, ollama pull), but missed two paths that still hit localhost: 1. runCaptureEx in src/lib/runner.ts was still spawning with raw { ...process.env, ...extraEnv }. validateOllamaModel calls runCaptureEx to probe http://localhost:11434/api/generate, which is the exact step the 2026-05-18 reopen reproduces failing with "HTTP 500 Internal Privoxy Error" on macOS+Privoxy. 2. Three spawn sites in inference/ollama/proxy.ts (the streamed pull curl at :488, the two unloadOllamaModels curls at :775 and :792) passed no `env:` option, defaulting to inheriting process.env with the user's http_proxy intact. Both routes now go through buildRunnerEnv / buildSubprocessEnv, which call withLocalNoProxy and inject NO_PROXY=localhost,127.0.0.1,host.docker.internal when any HTTP_PROXY is set. Adds a preflight warning (per the reverify criteria on the reopen) when the user's shell has HTTP_PROXY set without NO_PROXY=localhost — so users see a clear hint before they trip the trap with any tool that respects HTTP_PROXY. Extracted to src/lib/onboard/http-proxy-preflight.ts to stay within the onboard-entrypoint-budget. Acceptance (from the reopen comment): - export http_proxy=http://127.0.0.1:8118/ → nemoclaw onboard → Option 7 succeeds (no Privoxy 500). Covered by new regression test in test/runner.test.ts. - Preflight mentions http_proxy when set. New helper module. Closes #2616 Signed-off-by: Charan Jagwani <cjagwani@nvidia.com>
|
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)
📝 WalkthroughWalkthroughAdds a proxy-preflight utility and tests, warns during onboard preflight when a host HTTP proxy misses loopback, routes runner subprocess env construction through ChangesHTTP Proxy Loopback Fix
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add Comment |
cjagwani
added
bug
NV QA
![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
🤖 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 `@src/lib/onboard/http-proxy-preflight.ts`:
- Line 24: The current warn call logs the raw proxyEnv value (warn(`Detected
proxy: ${proxyEnv}`)), which can expose credentials; update the logging to
redact credentials by parsing proxyEnv (via the URL constructor) and log only
safe parts such as protocol, hostname and port (or replace username/password
with "[REDACTED]") instead of the full string; locate the warn call that
references proxyEnv in src/lib/onboard/http-proxy-preflight.ts and replace the
raw interpolation with the redacted/protocol-host-port representation.
🪄 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: 69219aa7-9d60-4008-a004-45ea4812682b
📒 Files selected for processing (5)
src/lib/inference/ollama/proxy.tssrc/lib/onboard.tssrc/lib/onboard/http-proxy-preflight.tssrc/lib/runner.tstest/runner.test.ts
E2E Advisor RecommendationRequired E2E: Dispatch hint: Auto-dispatched E2E: Full advisor summaryE2E Recommendation AdvisorBase: Required E2E
Optional E2E
New E2E recommendations
Dispatch hint
|
Selective E2E Results — ✅ All requested jobs passedRun: 26108909003
|
CodeRabbit flagged the preflight warning on #3801: it logged the raw HTTP_PROXY value, which leaks credentials when a user has http_proxy=http://user:password@proxy:3128 in their shell (common in corporate setups). Adds redactProxyCredentials() that: - Parses the URL and zeroes username/password if present - Falls back to a regex over the //user:pass@ userinfo segment for non-URL-parseable strings Plus a co-located test file covering: no proxy, NO_PROXY already bypasses loopback, basic-auth redaction (user:pass), username-only redaction, non-URL fallback, and the uppercase HTTP_PROXY variant. Signed-off-by: Charan Jagwani <cjagwani@nvidia.com>
|
Addressed in 7a4bbac. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
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 `@src/lib/onboard/http-proxy-preflight.ts`:
- Around line 21-22: The current check using noProxyEnv and the regex treats
either "localhost" OR "127.0.0.1" as sufficient; change it to require both
loopback entries before suppressing the warning: parse noProxyEnv (the variable
noProxyEnv) into comma-separated tokens, normalize whitespace/lowercase, and
only return false when both "localhost" and "127.0.0.1" are present in the token
set (instead of the existing /(localhost|127\.0\.0\.1)/ test); otherwise keep
returning true so the warning is shown.
🪄 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: c158523d-72c1-42af-8a5e-95d23052fbb5
📒 Files selected for processing (2)
src/lib/onboard/http-proxy-preflight.test.tssrc/lib/onboard/http-proxy-preflight.ts
Selective E2E Results — ✅ All requested jobs passedRun: 26109647508
|
…ort, function or class' Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
…press proxy preflight warning HTTP libraries (curl, Node fetch, Python requests) match the literal hostname against NO_PROXY. NO_PROXY=localhost alone still proxies 127.0.0.1 requests (and vice versa), so the preflight warning was falsely suppressed when only one entry was present. Switch the regex from OR to two separate checks AND'd together, and add unit tests for both partial-NO_PROXY cases. Addresses CodeRabbit major on #3801. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: Charan Jagwani <cjagwani@nvidia.com>
Selective E2E Results —
|
| Job | Result |
|---|---|
| cloud-onboard-e2e | |
| gpu-e2e | ⏭️ skipped |
Selective E2E Results — ✅ All requested jobs passedRun: 26131631298
|
Selective E2E Results — ✅ All requested jobs passedRun: 26132120219
|
wscurran
added
area: cli
4 participants
Summary
The 2026-05-18 regression on #2616 reproduces because PR #2662's fix (
withLocalNoProxy+buildSubprocessEnvwired intoinference/ollama/proxy.ts) missed two paths that still hitlocalhost:11434/11435:runCaptureExinsrc/lib/runner.ts:287was using raw{ ...process.env, ...extraEnv }, notbuildRunnerEnv(extraEnv)like its siblingrunCapture(line 234).validateOllamaModel→runCaptureEx→curl http://localhost:11434/api/generate→ inheritshttp_proxy=http://127.0.0.1:8118with noNO_PROXY→ Privoxy 500. This is the exact validation step in the reopen comment.inference/ollama/proxy.ts(streamed pull at:488,unloadOllamaModelsps + unload curls at:775/:792) passed noenv:option at all, defaulting to inheritingprocess.env.Both routes now go through
buildRunnerEnv/buildSubprocessEnv, which callwithLocalNoProxyto injectNO_PROXY=localhost,127.0.0.1,host.docker.internalwhen any HTTP_PROXY is set.Plus a preflight warning (per the reopen's reverify criteria) when the user's shell has HTTP_PROXY set without
NO_PROXY=localhost,127.0.0.1— so users see a clear hint before they trip the trap with any in-sandbox tool that respects HTTP_PROXY. Extracted tosrc/lib/onboard/http-proxy-preflight.tsto satisfyonboard-entrypoint-budget(onboard.ts is +1/-1 net).Acceptance criteria mapping (from the 2026-05-18 reopen comment)
export http_proxy=http://127.0.0.1:8118/→nemoclaw onboard→ Option 7 succeeds (no Privoxy 500)runner.ts:287swap tobuildRunnerEnv(extraEnv)fixes the validation curl; regression testtest/runner.test.ts#2616: runCaptureEx injects NO_PROXY=localhost,127.0.0.1 when http_proxy is setasserts the spawn envhttp_proxyif setsrc/lib/onboard/http-proxy-preflight.ts, called from preflight atonboard.tsafter the Docker checkTest plan
Results: 48/48 runner tests pass (1 new for #2616). 87/87 broader inference/Ollama tests still pass. Typecheck clean.
Notes for reviewers
onboard.tsnet delta is +1/-1 (the helper is one require call, replacing a blank line).onboard-entrypoint-budgetshould pass.src/lib/onboard/http-proxy-preflight.tsaccepts dependency injection (env,warn) so it stays unit-testable.runCapture(the non-Ex variant) already usesbuildRunnerEnv. This PR bringsrunCaptureExin line with it. The two are now consistent.{ ...process.env, ...opts.env }anti-pattern (onboard.ts:4245,adapters/openshell/client.ts:124/152/183). Not on the Ollama validation path, deliberately out of scope for this PR. Worth a follow-up sweep.spawnSyncand asserts the env handed to it.Closes #2616
Summary by CodeRabbit
New Features
Bug Fixes
Tests