Skip to content

fix(sandbox): rewrite #2109 proxy fix as http.request wrapper - #2323

Closed
lcsmontiel wants to merge 4 commits into
NVIDIA:mainfrom
lcsmontiel:fix/2109-http-proxy-wrapper
Closed

fix(sandbox): rewrite #2109 proxy fix as http.request wrapper#2323
lcsmontiel wants to merge 4 commits into
NVIDIA:mainfrom
lcsmontiel:fix/2109-http-proxy-wrapper

Conversation

@lcsmontiel

@lcsmontiel lcsmontiel commented Apr 23, 2026

Copy link
Copy Markdown
Contributor

Summary

Why PR #2110's axios-proxy-fix.js doesn't fire

Three independent failure modes, any one of which breaks the preload:

  1. Delivery gap. nemoclaw-blueprint/scripts/ is excluded from stageOptimizedSandboxBuildContext() in src/lib/sandbox-build-context.ts. Only blueprint.yaml and policies/ are staged. axios-proxy-fix.js never reaches the Docker build context, so /opt/nemoclaw-blueprint/scripts/axios-proxy-fix.js does not exist at runtime. The -f check in nemoclaw-start.sh silently returns false; NODE_OPTIONS is never set.
  2. Can't fix delivery by extending the build context. Adding scripts/ to stageOptimizedSandboxBuildContext() cache-busts the COPY nemoclaw-blueprint/ Dockerfile layer, which re-runs npm ci inside the k3s Docker-in-Docker build, which hangs. The optimized build context is deliberately minimal.
  3. Module._load can't reach bundled ESM. Even assuming the file were present, the Module._load hook intercepts require('axios') by name. OpenClaw's dist/http-Bh-HtMAg.js inlines follow-redirects + proxy-from-env as ESM — no require() calls to patch. The Bot Connector reply path uses that bundled code.

All three reasons are why the original #2109 reproduction still fails on v0.0.22 despite the PR #2110 merge.

The fix

Wrap http.request() — the lowest common denominator every HTTP client bottoms out at. Detect FORWARD-mode requests (hostname == proxy IP AND path.startsWith('https://')) and rewrite them as https.request() against the real target. NODE_USE_ENV_PROXY's EnvHttpProxyAgent then handles the CONNECT tunnel correctly.

Works for:

  • axios (direct require or bundled)
  • follow-redirects (bundled ESM, no require)
  • proxy-from-env (bundled ESM, no require)
  • any future HTTP library that constructs FORWARD-mode requests manually

Delivery: inline heredoc, written at boot

  • nemoclaw-blueprint/scripts/http-proxy-fix.js — canonical source for review and tests.
  • scripts/nemoclaw-start.sh embeds the same JS inline via a HTTP_PROXY_FIX_EOF heredoc. At PID-1 boot, emit_sandbox_sourced_file writes it to /tmp/nemoclaw-http-proxy-fix.js with root:root 444 (same trust-boundary helper used for /tmp/nemoclaw-proxy-env.sh). Then export NODE_OPTIONS="... --require /tmp/nemoclaw-http-proxy-fix.js".
  • No change to sandbox-build-context.ts, no new COPY in Dockerfile, no runtime-deploy + restart dance. The fix is active on the first sandbox boot.
  • test/http-proxy-fix-sync.test.ts enforces byte-for-byte equality between the canonical JS file and the embedded heredoc. Edits to one force edits to the other or CI fails.
  • validate_tmp_permissions is extended with the new path on both the root and non-root boot paths. The fix JS is a trust-boundary file — tampering would let the sandbox user inject arbitrary code into every Node process via NODE_OPTIONS.
  • nemoclaw-blueprint/scripts/axios-proxy-fix.js is removed.
  • test/service-env.test.ts axios requests fail with ERR_BAD_RESPONSE inside NemoClaw sandbox — double proxy conflict with NODE_USE_ENV_PROXY #2109 regression tests updated to the new variable name (_PROXY_FIX_SCRIPT) and lose the file-existence fixture (the entrypoint now writes the file unconditionally when NODE_USE_ENV_PROXY=1). The [All platforms] Discord channel fails with 400 — Node.js EnvHttpProxyAgent uses forward proxy instead of CONNECT tunnel #1570 ws-proxy-fix tests are untouched aside from removing the dead _AXIOS_FIX_SCRIPT=... prop in their fixtures.

End-to-end validation

Verified 2026-04-23 on EC2 t3.large (ca-central-1), NemoClaw v0.0.22 + fix, OpenShell v0.0.29, Node 22.22.1.

Direct reproduction of #2109 (inside the sandbox)

sandbox@my-assistant:~$ echo $NODE_OPTIONS
--require /tmp/nemoclaw-http-proxy-fix.js

sandbox@my-assistant:~$ node -e "require('/usr/local/lib/node_modules/openclaw/node_modules/axios')
    .get('https://clawhub.ai', {timeout:10000})
    .then(r => console.log('PASS', r.status))
    .catch(e => console.log('FAIL', e.code))"
PASS 200

Full Teams round-trip

Teams client
  -> @SecurityBOT "test message"
  -> Azure Bot Service relay
  -> ALB (HTTPS:443)
  -> EC2 port forward (3978 -> sandbox:3978)
  -> OpenClaw Teams adapter (Bot Framework webhook)
  -> Agent -> LiteLLM proxy (localhost:4000) -> Bedrock (Claude Sonnet 4.6)
  -> Bot Connector POST to webchat.botframework.com  <- previously failed here
  -> Reply delivered to Teams client

OpenShell egress logs during the run:

[sandbox] [OCSF] NET:OPEN [INFO] ALLOWED inference.local:443
[sandbox] [INFO] routing proxy inference request (streaming) endpoint=http://172.17.0.1:4000/v1

Zero FORWARD rejected entries. All outbound HTTPS goes through CONNECT correctly.

Before/after

Without fix With fix
https.request() (Node core) PASS PASS
axios default FAIL (FORWARD rejected) PASS
axios with proxy: false PASS PASS
Bundled dist axios (follow-redirects) FAIL PASS
Bot Connector reply to Teams FAIL PASS
End-to-end Teams message FAIL (no reply) PASS

Related observation — PR #2296 / #1570

While tracing #2110's failure I noticed that scripts/nemoclaw-start.sh wires ws-proxy-fix.js (added by PR #2296, closes #1570) at the same /opt/nemoclaw-blueprint/scripts/ws-proxy-fix.js path that axios-proxy-fix.js used. Because the optimized sandbox build context still does not stage nemoclaw-blueprint/scripts/, that file has the same delivery gap — the -f check silently returns false and NODE_OPTIONS is never set. PR #2296's own test plan left the E2E checkbox unchecked:

- [ ] E2E: deploy sandbox with Discord channel, verify gateway connects

I did not touch ws-proxy-fix in this PR — different issue, different failure mode (the bug is inside EnvHttpProxyAgent's FORWARD-vs-CONNECT choice for Upgrade: websocket requests, which the http.request wrapper in this PR cannot safely handle — it would re-enter the same faulty agent logic). Raising this as a heads-up in case maintainers want a follow-up. Happy to open a separate issue or PR if useful.

Scope / non-goals

Test plan

Closes #2109.

Summary by CodeRabbit

  • New Features

    • Unified runtime proxy handling that transparently routes proxy-mode HTTPS requests to the correct direct HTTPS flow when the environment proxy flag is enabled.
  • Improvements

    • Startup now injects and validates a single, consistent proxy preload for both normal and sandboxed sessions; legacy library-specific preload removed.
  • Tests

    • Added tests to ensure the canonical proxy preload and its embedded startup copy remain synchronized.
  • Chores

    • Tightened entrypoint and temporary-file permission validation for the injected preload.

PR NVIDIA#2110's axios-only Module._load preload never fired at runtime:

  1. nemoclaw-blueprint/scripts/ is excluded from the optimized sandbox
     build context (src/lib/sandbox-build-context.ts), so
     axios-proxy-fix.js was not baked into the sandbox image.
  2. Adding scripts/ to the build context cache-busts the
     `COPY nemoclaw-blueprint/` Dockerfile layer and hangs npm ci in
     the k3s Docker-in-Docker build, so the delivery gap cannot be
     closed by expanding the context.
  3. Even if the file had reached the image, intercepting
     require('axios') via Module._load cannot patch follow-redirects +
     proxy-from-env bundled as ESM in OpenClaw's dist/http-Bh-HtMAg.js
     — there are no require() calls to intercept. The Bot Connector
     reply path uses the bundled code.

Replace with an http.request() wrapper — the lowest common denominator
every HTTP library bottoms out at. Detect FORWARD-mode requests
(hostname = proxy IP, path = full https:// URL) and rewrite them to
https.request() against the real target, letting NODE_USE_ENV_PROXY
handle the CONNECT tunnel correctly. Works for any HTTP client,
including bundled ESM that makes no require() calls.

Delivery:
  - nemoclaw-blueprint/scripts/http-proxy-fix.js — canonical source for
    review and tests.
  - scripts/nemoclaw-start.sh embeds the same JS inline via a heredoc,
    writes it to /tmp/nemoclaw-http-proxy-fix.js through
    emit_sandbox_sourced_file (root:root 444, symlink-safe), and loads
    it via NODE_OPTIONS=--require. No changes to sandbox-build-context.
  - test/http-proxy-fix-sync.test.ts enforces byte-for-byte equality
    between the heredoc and the canonical file, so future edits cannot
    silently diverge.
  - validate_tmp_permissions is invoked with the new path on both the
    root and non-root boot paths (the fix JS is a trust-boundary file
    — tampering would inject arbitrary code into every Node process
    via NODE_OPTIONS).

Because the content ships inside nemoclaw-start.sh rather than as a
separately-deployed file, the fix fires on the very first sandbox boot
with no post-onboard deploy + restart dance required.

Verified end-to-end on 2026-04-23: EC2 t3.large (ca-central-1),
NemoClaw v0.0.22 + OpenShell v0.0.29, Node 22.22.1. Direct
axios.get('https://clawhub.ai') returns 200 inside the sandbox; full
Teams -> ALB -> OpenClaw -> LiteLLM/Bedrock -> Bot Connector ->
Teams round-trip succeeds. No `FORWARD rejected` entries in OpenShell
network logs. Comparison table and reproduction steps posted in the
PR description.

Scope:
  - Fixes the NVIDIA#2109 regression class (axios / follow-redirects /
    proxy-from-env FORWARD-mode rewrites on NODE_USE_ENV_PROXY=1).
  - Does NOT fix NVIDIA#1570 (Discord WebSocket via the ws library). That
    bug sits at a different layer — EnvHttpProxyAgent's FORWARD-vs-
    CONNECT decision for Upgrade: websocket requests — and needs the
    agent-swap treatment that NVIDIA#2296 applies. The http.request wrapper
    in this PR cannot safely handle that case (it would re-enter the
    same faulty agent logic).
  - Does NOT modify sandbox-build-context.ts.

Removes the superseded nemoclaw-blueprint/scripts/axios-proxy-fix.js
and updates the existing regression tests in service-env.test.ts to
the new variable name (_PROXY_FIX_SCRIPT).

Closes NVIDIA#2109.
@copy-pr-bot

copy-pr-bot Bot commented Apr 23, 2026

Copy link
Copy Markdown

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@coderabbitai

coderabbitai Bot commented Apr 23, 2026

Copy link
Copy Markdown
Contributor

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: c13d94fa-37e7-4891-8e16-39a8d8942485

📥 Commits

Reviewing files that changed from the base of the PR and between 06d17d2 and 8c604f2.

📒 Files selected for processing (2)
  • nemoclaw-blueprint/scripts/http-proxy-fix.js
  • scripts/nemoclaw-start.sh
🚧 Files skipped from review as they are similar to previous changes (1)
  • scripts/nemoclaw-start.sh

📝 Walkthrough

Walkthrough

Removes an axios-specific preload and adds a new preload that monkey-patches Node's http.request to detect proxy FORWARD-mode HTTPS calls and replay them via https.request; updates startup script to embed/load the preload and updates tests to enforce sync and behavior.

Changes

Cohort / File(s) Summary
Proxy preload scripts
nemoclaw-blueprint/scripts/axios-proxy-fix.js, nemoclaw-blueprint/scripts/http-proxy-fix.js
Deleted the old axios-specific preload; added http-proxy-fix.js which, when NODE_USE_ENV_PROXY=1, detects requests sent to the proxy host with a full https:// path and rewrites them to use https.request targeting the original destination.
Startup script / preload embedding
scripts/nemoclaw-start.sh
Replaced axios-only NODE_OPTIONS=--require preload wiring with inline heredoc creation of /tmp/nemoclaw-http-proxy-fix.js, sets NODE_OPTIONS=--require accordingly, and tightens tmp-permissions validation to the specific preload file path for both root and non-root flows.
Sync & behavior tests
test/http-proxy-fix-sync.test.ts, test/service-env.test.ts
Added a test ensuring the heredoc-embedded preload matches the canonical http-proxy-fix.js, verifies NODE_OPTIONS --require wiring and tmp-permissions checks, updated regression tests to reference http-proxy-fix, and removed references to the legacy axios fix.

Sequence Diagram(s)

sequenceDiagram
  participant Client
  participant NodeRuntime as Node
  participant Env as EnvVars
  participant ProxyHost as Proxy
  participant Destination as RemoteHost

  Note over NodeRuntime,Env: On startup, preload (`http-proxy-fix.js`) is required if NODE_USE_ENV_PROXY=1
  Client->>NodeRuntime: call http.request(options)
  NodeRuntime->>NodeRuntime: patched http.request inspects options and Env.PROXY_HOST
  alt options.host matches Env.PROXY_HOST and options.path startsWith "https://"
    NodeRuntime->>NodeRuntime: parse target URL from options.path
    NodeRuntime->>Destination: call https.request(parsedTarget, options, cb)
    Destination-->>NodeRuntime: response
    NodeRuntime-->>Client: deliver response via callback
  else
    NodeRuntime->>ProxyHost: call original http.request(options)
    ProxyHost-->>NodeRuntime: response
    NodeRuntime-->>Client: deliver response
  end
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

Suggested labels

bug, OpenShell, fix

Suggested reviewers

  • ericksoa

Poem

🐰 I hop through code with nimble paws,

I swap the paths and mend the laws,
When proxies forward, I redirect,
So HTTPS finds its rightful trek,
🥕— a rabbit's patch, small applause.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely summarizes the primary change: replacing an axios-specific proxy fix with a generalized http.request wrapper mechanism.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands and usage tips.

@lcsmontiel
lcsmontiel marked this pull request as ready for review April 23, 2026 04:12

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 `@nemoclaw-blueprint/scripts/http-proxy-fix.js`:
- Around line 61-82: The code calls new URL(options.path) without handling URL
parsing errors which can throw for malformed values; wrap the new
URL(options.path) call in a try/catch (inside the if that checks
options.hostname === proxyHost and options.path.startsWith('https://')) and on
catch simply fall back to returning origRequest.apply(http, arguments) (or
otherwise return origRequest) so the process doesn't crash; ensure
callback/return behavior matches the existing branch and keep references to
options.path, proxyHost, new URL, and origRequest when locating the change.
🪄 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: Pro Plus

Run ID: df788072-7fbc-47dd-9a12-ce0dff26bbe0

📥 Commits

Reviewing files that changed from the base of the PR and between d9aced4 and b99b9c5.

📒 Files selected for processing (5)
  • nemoclaw-blueprint/scripts/axios-proxy-fix.js
  • nemoclaw-blueprint/scripts/http-proxy-fix.js
  • scripts/nemoclaw-start.sh
  • test/http-proxy-fix-sync.test.ts
  • test/service-env.test.ts
💤 Files with no reviewable changes (1)
  • nemoclaw-blueprint/scripts/axios-proxy-fix.js

Comment thread nemoclaw-blueprint/scripts/http-proxy-fix.js
@lcsmontiel

Copy link
Copy Markdown
Contributor Author

@BenediktSchackenberg @ericksoa You may want to take a look at this PR. Thank you.

Wrap the `new URL(options.path)` call in a try/catch so that a
malformed path value (which passes the `startsWith('https://')` check
but still fails URL parsing) falls back to the original http.request
instead of crashing the Node process.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 `@nemoclaw-blueprint/scripts/http-proxy-fix.js`:
- Around line 50-54: Change the unused catch binding name from e to _e wherever
it appears: update the try/catch blocks that parse proxyUrl (the catch
surrounding new URL(proxyUrl) that assigns proxyHost) and the other catch block
around lines 67-70 to use catch (_e) and also update the embedded heredoc in
scripts/nemoclaw-start.sh where the same catch binding is duplicated so both JS
(http-proxy-fix.js) and the shell-embedded JS use the prefixed _e name to
satisfy the unused-variable guideline.
- Around line 72-83: The current FORWARD-mode branch recreates the https.request
options object and drops important fields (signal, agent, auth, ca/cert/key,
rejectUnauthorized, etc.); instead, clone the original options and overwrite
only the proxy-specific fields so existing options are preserved: use
Object.assign({}, options, { method: options.method||'GET', hostname:
target.hostname, host: target.hostname, port: target.port||443, path:
target.pathname+target.search, protocol: 'https:' }) and pass that to
https.request (the call around https.request and variables options/target/method
must be updated) so AbortController, custom agents, TLS and auth fields remain
intact.
🪄 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: Pro Plus

Run ID: f000a9b2-7657-4507-834f-35cda948a2dc

📥 Commits

Reviewing files that changed from the base of the PR and between b99b9c5 and b3678c1.

📒 Files selected for processing (2)
  • nemoclaw-blueprint/scripts/http-proxy-fix.js
  • scripts/nemoclaw-start.sh

Comment thread nemoclaw-blueprint/scripts/http-proxy-fix.js
Comment thread nemoclaw-blueprint/scripts/http-proxy-fix.js Outdated
Address CodeRabbit review feedback on PR.

The FORWARD-mode branch previously constructed a fresh options object
with only {method, hostname, host, port, path, protocol, headers,
timeout}, silently dropping caller-supplied fields that can matter for
correctness:

  - signal — AbortController, used by modern axios/fetch for
    cancellation. Dropping it meant user-initiated aborts would not
    propagate to the rewritten https.request, leaving the request
    running after the caller thought it was cancelled.
  - TLS: ca, cert, key, passphrase, rejectUnauthorized — custom trust
    anchors or mTLS settings. Uncommon in the FORWARD path but not
    impossible.
  - auth — Basic-auth credentials for the target origin.
  - lookup, family, localAddress, maxHeaderSize, insecureHTTPParser —
    per-request network/parser tuning.

Switch to Object.assign({}, options, { ...proxy-routing-fields })
which clones the caller's options and overwrites only the fields we
explicitly need to change (method default, hostname/host/port/path/
protocol). Everything else is carried over verbatim.

Mirror the edit in the inline heredoc in scripts/nemoclaw-start.sh so
the canonical file and the embedded copy remain byte-identical; the
http-proxy-fix-sync test enforces this.
Address CodeRabbit review feedback (minor, PR NVIDIA#2323).

Both `catch (e)` bindings in http-proxy-fix.js are unused — one in the
proxy URL parse (falls through silently) and one in the FORWARD-path
new URL guard (returns the original request). Rename to `catch (_e)`
to satisfy the project's "unused variables must be prefixed with _"
convention.

Mirror the edit in the inline heredoc in scripts/nemoclaw-start.sh so
the canonical file and the embedded copy remain byte-identical; the
http-proxy-fix-sync test enforces this.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 `@scripts/nemoclaw-start.sh`:
- Around line 878-882: The proxy matcher currently only checks options.hostname
against proxyHost so requests that pass { host, port, path } slip through;
update the conditional that uses options.hostname (and the embedded heredoc
copy) to also accept options.host (e.g., check (options.hostname === proxyHost
|| options.host === proxyHost)) before testing
options.path.startsWith('https://') so host-only request options are rewritten
like hostname-based ones; ensure the same change is made in the corresponding
embedded heredoc to keep the two files byte-for-byte identical.
🪄 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: Pro Plus

Run ID: 1339cf9d-d779-4f6d-9abd-47b64cb94215

📥 Commits

Reviewing files that changed from the base of the PR and between b3678c1 and 06d17d2.

📒 Files selected for processing (2)
  • nemoclaw-blueprint/scripts/http-proxy-fix.js
  • scripts/nemoclaw-start.sh
🚧 Files skipped from review as they are similar to previous changes (1)
  • nemoclaw-blueprint/scripts/http-proxy-fix.js

Comment thread scripts/nemoclaw-start.sh
Comment on lines +878 to +882
if (
options.hostname === proxyHost &&
options.path &&
options.path.startsWith('https://')
) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== Node.js http.request() docs =="
curl -fsSL https://nodejs.org/api/http.html | rg -n -C2 'hostname|host'

echo
echo "== Current matcher in scripts/nemoclaw-start.sh =="
sed -n '874,905p' scripts/nemoclaw-start.sh

echo
echo "== Current proxy-fix tests =="
sed -n '1,220p' test/http-proxy-fix-sync.test.ts
rg -n -C2 'host|hostname|http\.request|https\.request' test

Repository: NVIDIA/NemoClaw

Length of output: 50372


🏁 Script executed:

# Read the actual matcher code
sed -n '874,905p' scripts/nemoclaw-start.sh

Repository: NVIDIA/NemoClaw

Length of output: 1174


🏁 Script executed:

# Check if canonical http-proxy-fix.js exists and read it
find . -name "http-proxy-fix.js" -o -name "http-proxy-fix*.js" | head -20

Repository: NVIDIA/NemoClaw

Length of output: 105


🏁 Script executed:

# Look for test cases using 'host' option in HTTP requests
rg -n "host:\s*['\"]" test/http-proxy-fix-sync.test.ts | head -20

Repository: NVIDIA/NemoClaw

Length of output: 41


🏁 Script executed:

# Check test coverage for request option shapes
rg -A5 "http\.request\(|https\.request\(" test/http-proxy-fix-sync.test.ts | grep -E "(host|hostname)" | head -30

Repository: NVIDIA/NemoClaw

Length of output: 41


🏁 Script executed:

# Read the canonical http-proxy-fix.js
cat ./nemoclaw-blueprint/scripts/http-proxy-fix.js

Repository: NVIDIA/NemoClaw

Length of output: 3556


🏁 Script executed:

# Search entire test file for any request/host option patterns
rg -n "options\s*=" test/http-proxy-fix-sync.test.ts -A2 -B1 | head -60

Repository: NVIDIA/NemoClaw

Length of output: 41


🏁 Script executed:

# Check if there are any http.request() calls with 'host' field in tests
rg "host:" test/ --type js --type ts | grep -v hostname | head -20

Repository: NVIDIA/NemoClaw

Length of output: 1977


🏁 Script executed:

# Search for test cases that specifically test http.request with host-only options
rg -n "host-only|host\s*:" test/http-proxy-fix-sync.test.ts -B3 -A3 | head -50

Repository: NVIDIA/NemoClaw

Length of output: 41


🏁 Script executed:

# Look at the entire http-proxy-fix test to understand coverage
wc -l test/http-proxy-fix-sync.test.ts && echo "---" && head -100 test/http-proxy-fix-sync.test.ts

Repository: NVIDIA/NemoClaw

Length of output: 3824


🏁 Script executed:

# Check if there are any integration or functional tests that might exercise host-only paths
fd -e "test.ts" -e "test.js" | xargs rg -l "http\.request|https\.request" | head -10

Repository: NVIDIA/NemoClaw

Length of output: 116


Handle host-only request options in the proxy-fix matcher.

The matcher only checks options.hostname, but http.request() also accepts host. Requests with only { host, port, path } will skip the rewrite and hit the original HTTPS FORWARD rejection.

Suggested fix
-    if (
-      options.hostname === proxyHost &&
-      options.path &&
-      options.path.startsWith('https://')
-    ) {
+    var requestHost = options.hostname;
+    if (!requestHost && typeof options.host === 'string') {
+      try {
+        requestHost = new URL('http://' + options.host).hostname;
+      } catch (e) {
+        requestHost = options.host;
+      }
+    }
+    if (
+      requestHost === proxyHost &&
+      typeof options.path === 'string' &&
+      options.path.startsWith('https://')
+    ) {

This edit must land in both nemoclaw-blueprint/scripts/http-proxy-fix.js and the embedded heredoc in scripts/nemoclaw-start.sh (the test at line 34–56 enforces byte-for-byte equality).

📝 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
if (
options.hostname === proxyHost &&
options.path &&
options.path.startsWith('https://')
) {
var requestHost = options.hostname;
if (!requestHost && typeof options.host === 'string') {
try {
requestHost = new URL('http://' + options.host).hostname;
} catch (e) {
requestHost = options.host;
}
}
if (
requestHost === proxyHost &&
typeof options.path === 'string' &&
options.path.startsWith('https://')
) {
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@scripts/nemoclaw-start.sh` around lines 878 - 882, The proxy matcher
currently only checks options.hostname against proxyHost so requests that pass {
host, port, path } slip through; update the conditional that uses
options.hostname (and the embedded heredoc copy) to also accept options.host
(e.g., check (options.hostname === proxyHost || options.host === proxyHost))
before testing options.path.startsWith('https://') so host-only request options
are rewritten like hostname-based ones; ensure the same change is made in the
corresponding embedded heredoc to keep the two files byte-for-byte identical.

@BenediktSchackenberg

Copy link
Copy Markdown
Contributor

Good analysis on why #2110 didn't land correctly — the three failure modes (build context exclusion, can't extend without breaking the build, Module._load missing bundled ESM) are all valid. The http.request() wrapper approach is the right level to fix this.

On the CodeRabbit feedback:

  • The try-catch around new URL(options.path) is worth adding — malformed URLs would crash any Node process that loads the preload
  • The unused catch bindings (catch (e)catch (_e)) is a style nit but easy to apply

ericksoa added a commit that referenced this pull request Apr 23, 2026
#2344)

## Summary

Signed replay of #2323 by @lcsmontiel — same changes, commits signed to
pass the org signature check.

- Replaces `axios-proxy-fix.js` with an `http.request()` wrapper that
catches the FORWARD-vs-CONNECT mismatch at the lowest common denominator
- Adds `try/catch` around `new URL(options.path)` to prevent process
crashes on malformed URLs
- Preserves caller-supplied options in the rewritten request
- Verified end-to-end on a real proxy-enabled sandbox (see #2323 for
full validation table)

## Original PR

All design rationale, failure analysis, and E2E validation are
documented in #2323. Credit to @lcsmontiel for the fix.

## Test plan

- [x] `npx vitest run --project cli test/http-proxy-fix-sync.test.ts` —
6/6 pass
- [x] `npx vitest run --project cli test/service-env.test.ts` — 41/41
pass
- [x] All pre-commit and pre-push hooks pass
- [ ] CI

Closes #2109.

🤖 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 HTTP request interception for improved proxy environment
variable handling, with enhanced support for Node.js 22.

* **Tests**
* Added new test suite validating proxy fix consistency and
configuration.
* Updated proxy environment variable tests to reflect new
implementation.

* **Chores**
  * Removed legacy proxy handling implementation.
  * Updated startup script to use enhanced proxy mechanism.

<!-- end of auto-generated comment: release notes by coderabbit.ai -->

---------

Co-authored-by: Lucas Montiel <lcsmontiel@gmail.com>
Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@ericksoa

Copy link
Copy Markdown
Contributor

@lcsmontiel Thank you for this excellent contribution! The root cause analysis was thorough — identifying all three independent failure modes of the original fix was impressive work, and the http.request wrapper approach is the right architectural call. The end-to-end validation on a real sandbox was also very much appreciated.

We replayed your commits (with the try/catch addition) into #2344 to pass the org signature check, and it's now merged to main. Full credit to you for the fix. 🙏

@ericksoa ericksoa closed this Apr 23, 2026
@wscurran wscurran added the bug-fix PR fixes a bug or regression label Jun 8, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug-fix PR fixes a bug or regression

Projects

None yet

4 participants