fix(sandbox): wrap fetch() to route HTTPS through EnvHttpProxyAgent - #2
Merged
Conversation
Follow-up to NVIDIA#2344 (the http.request rewrite). PR NVIDIA#2344 covers axios / follow-redirects / proxy-from-env. This change covers an additional code path the Microsoft Teams adapter uses: native fetch() with a custom undici dispatcher. When the OpenClaw Teams adapter handles a channel @mention it calls the Microsoft Graph API to fetch the full message body: fetch('https://graph.microsoft.com/v1.0/teams/.../messages/...', { dispatcher: customUndiciAgent }) The custom dispatcher routes the request through its own connection pool, bypassing Node's default EnvHttpProxyAgent — which is the only thing routing HTTPS through the OpenShell L7 proxy. Direct egress to graph.microsoft.com is blocked by the sandbox network namespace and surfaces as ECONNREFUSED. Channel @mentions silently fail; DMs work because their webhook payload carries the message body and no Graph call is needed. Wrap globalThis.fetch in the same preload that wraps http.request: when fetch() is called with a custom dispatcher and an HTTPS URL, strip the dispatcher and let EnvHttpProxyAgent handle the request. Non-HTTPS URLs and dispatcher-free calls pass through unchanged so direct intra-sandbox HTTP traffic and any non-proxy use of the dispatcher option keep working. Refinements over the originally proposed fix: - URL extraction handles all three fetch() input forms — string, URL object (.href), Request object (.url) — in that order. The naive `url?.url` check would miss URL objects (no .url property) and silently leave the dispatcher attached, defeating the fix on callers that pass URL instances. - One-shot console.warn so the strip is auditable in logs without spamming on every call (the Teams adapter polls the Graph API frequently under load — per-call logs would flood). The flag is closure-scoped so a process restart re-arms it. - Defensive `typeof origFetch === 'function'` guard so a Node runtime that ever ships without globalThis.fetch (or a future embedding context that strips it) silently no-ops instead of throwing. Mirrored in the inline heredoc in scripts/nemoclaw-start.sh; the http-proxy-fix-sync test enforces byte-for-byte equality between the canonical file and the heredoc, plus explicit fetch-wrapper presence assertions so a future "delete from both copies in lockstep" can't silently regress (byte-equality alone cannot catch that case). Verified end-to-end: bot now successfully replies to channel @mentions; DM regression check passes; non-Graph fetch() calls and fetch() calls without a custom dispatcher unaffected. Reproduction and rollout details posted in the PR description.
|
🚀 Docs preview ready! |
lcsmontiel
marked this pull request as ready for review
April 30, 2026 22:25
lcsmontiel
pushed a commit
that referenced
this pull request
May 19, 2026
…3456) (NVIDIA#3520) > **Draft for visibility.** Issue-autopilot Stages 4-5 of NVIDIA#3456. Will mark ready once batch self-review + CI complete. ## Summary Closes the two remaining output threads in NVIDIA#3456 after the core dead-loop fix already landed on `main` (via NVIDIA#3459, NVIDIA#3434, NVIDIA#3483). Full sub-bug mapping in the [NVIDIA#3456 status comment](NVIDIA#3456 (comment)). - **Sub-bug #3** — `nemoclaw <name> destroy --yes` recovery hint replaced with a registry-aware helper. - **Sub-bug NVIDIA#4** — `Destroyed gateway 'nemoclaw' skipped` self-contradictory wording replaced with `Gateway 'nemoclaw' already removed or unreachable`. ## Acceptance criteria mapping | Sub-bug | Resolution | Evidence | |---|---|---| | #1 dead loop | Already fixed on main (NVIDIA#3459) | out of scope | | #2 firewall diagnostic | Already fixed on main (NVIDIA#3459) | out of scope | | **#3** literal `<name>` placeholder | **This PR** | `src/lib/onboard/gpu-recovery.ts` + `onboard.ts:10387-10405` | | **NVIDIA#4** misleading "skipped" wording | **This PR** | `src/lib/actions/uninstall/run-plan.ts:210-228, 407-414` | | NVIDIA#5 uninstall residuals | Already fixed on main (NVIDIA#3483) | out of scope | ## Behavior matrix `gpuPassthroughRecoveryLines(names)`: | Input | Suggestion | |---|---| | `null` / `[]` | `nemoclaw uninstall && nemoclaw onboard --gpu` | | one sandbox | `nemoclaw <name> destroy --yes --cleanup-gateway && nemoclaw onboard --gpu` | | many sandboxes | each `destroy --yes`, only the last gets `--cleanup-gateway` | ## Test plan ``` npm run typecheck:cli npx vitest run src/lib/onboard/gpu-recovery.test.ts src/lib/actions/uninstall/run-plan.test.ts ``` 22 tests pass (6 new + 16 existing). ## Notes for reviewers - This is the work [NVIDIA#3464 attempted](NVIDIA#3464); that PR was closed without merging after CodeRabbit asked for the `<name>` placeholder to be forbidden in tests via negative assertion. This PR adopts that refinement. - `runOptional` extension is backwards-compatible — existing callers without `onSkip` get the original wording. Closes NVIDIA#3456 once merged. --------- Signed-off-by: Charan Jagwani <charjags100@gmail.com> Co-authored-by: Charan Jagwani <charjags100@gmail.com> Co-authored-by: Carlos Villela <cvillela@nvidia.com>
lcsmontiel
pushed a commit
that referenced
this pull request
May 28, 2026
…#3271) (NVIDIA#4020) ## Summary Adds `classifyGatewayFailure` and wires it into `showSandboxStatus`'s final fallback branch so `nemoclaw <name> status` prints a clearly-named failure layer header before the existing actionable hints. Closes the UX gap split out of NVIDIA#2666 / NVIDIA#3270. ## Related Issue Fixes NVIDIA#3271. Supersedes NVIDIA#3309 (kagura-agent), which implemented the same feature but missed the `docker ps -a` existence check that AC #2 explicitly requires (CodeRabbit major finding on that PR). ## Changes - `src/lib/actions/sandbox/gateway-failure-classifier.ts`: new module exposing `classifyGatewayFailure(sandboxName, { runners? })` with injectable runners (`dockerInfo`, `dockerIsRunning`, `dockerExists`, `portProbe`) plus `getLayerHeader(layer)`. - Layers: `docker_unreachable`, `container_missing` (new, distinct from `container_exited` per AC #2), `container_exited_port_conflict`, `container_exited`, `gateway_unreachable`. - Default runners go through `src/lib/adapters/docker` (`dockerInfo`, `dockerCapture`) to satisfy the docker-abstraction guard. - `src/lib/actions/sandbox/status.ts`: calls the classifier and prints the layer header before `printGatewayLifecycleHint` in the final fallback branch. ## Type of Change - [x] Code change (feature, bug fix, or refactor) - [ ] Code change with doc updates - [ ] Doc only (prose changes, no code sample modifications) - [ ] Doc only (includes code sample changes) ## Verification - [x] Unit tests in isolation: `npx vitest run test/gateway-failure-classifier.test.ts` → 8/8 pass (per-layer, including `container_missing` and short-circuit behavior). - [x] Subprocess test in isolation: `npx vitest run test/repro-2666-silent-list-status.test.ts` → 7/7 pass, including the new "`nemoclaw <name> status` prints the `container_exited_port_conflict` layer header (NVIDIA#3271)" test which spawns the real CLI against a fake docker stack + a real TCP listener holding the gateway port. - [x] `test/docker-abstraction-guard.test.ts` passes — no direct `execSync("docker …")` outside `src/lib/adapters/docker`. - [x] Tests added or updated for new or changed behavior - [x] No secrets, API keys, or credentials committed - [ ] Docs updated for user-facing behavior changes (status output is a UX polish, not a contract change) - [ ] `make docs` builds without warnings (no doc changes)⚠️ Committed with `--no-verify` (user-authorized): the pre-commit `Test (CLI)` hook (full vitest with v8 coverage) hits unrelated timeout flakes on this macOS workstation (Defender + Spotlight + iMessage indexer contention). The new tests in this PR pass cleanly in isolation. CI on Linux runners is the authoritative gate. ## Definition of Done (from NVIDIA#3271) - [x] `status` prints a clearly-named layer header in each classified state (5 layers, expanded from the original 4 to split `container_missing` from `container_exited`). - [x] Classifier has unit tests per layer. - [x] Repro subprocess test extended to assert the named layer for the container-stopped + foreign-port-holder scenario. 🤖 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** * Added smarter gateway failure diagnostics that identify unreachable Docker, missing or exited gateway containers, and port conflicts; includes clear failure headers. * **Bug Fixes** * Status command now shows the appropriate failure header before guidance and exits with a non-zero status when verification fails. * **Tests** * Added unit and end-to-end tests covering diagnostics, header ordering, and port-conflict scenarios. <!-- review_stack_entry_start --> [](https://app.coderabbit.ai/change-stack/NVIDIA/NemoClaw/pull/4020?utm_source=github_walkthrough&utm_medium=github&utm_campaign=change_stack) <!-- review_stack_entry_end --> <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Charan Jagwani <cjagwani@nvidia.com> Signed-off-by: Aaron Erickson <aerickson@nvidia.com> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Co-authored-by: Aaron Erickson <aerickson@nvidia.com>
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
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
msteams debounce flush failed: fetch failedfollowed byECONNREFUSEDagainstgraph.microsoft.comwithagent=custom.fetch()with a custom undicidispatcher. The custom dispatcher creates its own connection pool, bypassing Node's defaultEnvHttpProxyAgent— which is the only thing routing HTTPS through the OpenShell L7 proxy. Direct egress is blocked by the sandbox netns.http.requestrewrite for axios requests fail with ERR_BAD_RESPONSE inside NemoClaw sandbox — double proxy conflict with NODE_USE_ENV_PROXY NVIDIA/NemoClaw#2109). fix(sandbox): rewrite #2109 proxy fix as http.request wrapper (signed) NVIDIA/NemoClaw#2344 coversaxios/follow-redirects/proxy-from-env; this coversfetch() + custom dispatcher. Same preload file, same delivery path (via/usr/local/lib/nemoclaw/preloads/http-proxy-fix.jsafter the refactor(runtime): extract entrypoint preload modules NVIDIA/NemoClaw#3109 refactor).Reproduction
@mentionthe bot.DMs continue to work because the webhook payload carries the message body — no Graph API call is needed.
The fix
Add a
globalThis.fetchwrapper alongside the existinghttp.requestwrapper innemoclaw-blueprint/scripts/http-proxy-fix.js. Whenfetch()is called with a custom dispatcher and an HTTPS URL, strip the dispatcher and let the defaultEnvHttpProxyAgenthandle the request through the proxy.Non-HTTPS URLs and dispatcher-free
fetch()calls pass through unchanged — direct intra-sandbox HTTP traffic and any non-proxy use of the dispatcher option keep working.Design notes
fetch()input forms — string,URLobject (.href),Requestobject (.url) — in that order. A naiveurl?.urlwould missURLinstances (which have.href, not.url) and silently leave the dispatcher attached, defeating the fix.console.warnso the strip is auditable in logs without spamming on every call. The Teams adapter polls Graph frequently under load — per-call logs would flood. The warned flag is closure-scoped so a process restart re-arms it.typeof origFetch === 'function'guard so a Node runtime that ever ships withoutglobalThis.fetch(or a future embedding context that strips it) silently no-ops instead of throwing at preload time./usr/local/lib/nemoclaw/preloads/. This PR only touches the canonicalhttp-proxy-fix.js— no heredoc to keep in sync. The existinghttp-proxy-fix-sync.test.tsend-to-end test (which extracts the entrypoint block, runs it, and reads the generated/tmp/...file) is extended with explicit assertions that both thehttp.requestwrapper and theglobalThis.fetchwrapper are present in the generated preload, so a future accidental deletion of either trips CI.Why not patch undici / replace the dispatcher with a proxy-aware one?
Module._loadhook in fix(proxy): resolve axios + NODE_USE_ENV_PROXY double-proxy conflict NVIDIA/NemoClaw#2110 that fix(sandbox): rewrite #2109 proxy fix as http.request wrapper (signed) NVIDIA/NemoClaw#2344 superseded.EnvHttpProxyAgentroute is the smallest correct change.console.warn, so if a caller's dispatcher carried meaningful behavior other than direct routing, that's discoverable in logs.Relationship to NVIDIA#2344 / NVIDIA#2296 / NVIDIA#2109
http.request()(axios, follow-redirects, proxy-from-env)https.request()Upgrade: websocket (Discord)EnvHttpProxyAgentpicks FORWARD instead of CONNECT for WSws-proxy-fix.js)fetch()+ custom dispatcher (Teams Graph)EnvHttpProxyAgententirelySame root cause family (HTTP code path that doesn't respect
NODE_USE_ENV_PROXY), three different surface code paths, three orthogonal fixes.Test plan
npx vitest run --project cli test/http-proxy-fix-sync.test.ts— 1/1 pass (extracts entrypoint block, runs it, reads the generated preload; now asserts bothhttp.requestandglobalThis.fetchwrappers +delete newOpts.dispatcher)npx vitest run --project cli test/service-env.test.ts— passes (no regression in the persist block tests)npm run typecheck:cli— cleanshellcheck scripts/nemoclaw-start.sh— clean (unchanged in this PR)fetch()calls andfetch()calls without a custom dispatcher confirmed unaffected.Closes the channel-message gap left by NVIDIA#2344.