fix(sandbox): add WebSocket CONNECT tunnel preload for Discord gateway - #2296
Merged
Conversation
#1570) Node.js EnvHttpProxyAgent sends forward proxy requests instead of CONNECT for HTTPS WebSocket upgrades, causing the OpenShell L7 proxy to reject Discord gateway connections with 400. Add a --require preload script that patches https.request() to detect Upgrade: websocket headers and inject a proper CONNECT tunnel agent. Non-WebSocket HTTPS requests pass through unchanged. Belt-and-suspenders: works regardless of upstream OpenClaw changes. If the caller provides a custom agent, the preload steps aside.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a preload patch that, when Changes
Sequence Diagram(s)sequenceDiagram
participant Proc as NemoClaw Process
participant Preload as ws-proxy-fix Preload
participant Https as Node https Module
participant Proxy as OpenShell Proxy
participant Discord as Discord Gateway
Proc->>Preload: start with --require (preload)
Preload->>Https: monkeypatch https.request()
Proc->>Https: https.request({ host: gateway.discord.gg, headers: Upgrade: websocket })
Https->>Preload: intercepted request
Preload->>Preload: detect Discord host + Upgrade
Preload->>Https: inject custom https.Agent (createConnection)
Https->>Proxy: HTTP CONNECT gateway.discord.gg:443 (via agent)
Proxy->>Https: 200 Connected (tunnel)
Https->>Https: tls.connect() over tunneled socket
Https->>Discord: WebSocket upgrade over TLS
Discord->>Proc: WebSocket connection established
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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 docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
nemoclaw-blueprint/scripts/ws-proxy-fix.ts (1)
96-117: Consider destroying the socket on error to prevent resource leaks.If the CONNECT request fails with an error event, the underlying socket may remain open. Adding explicit cleanup ensures the socket is destroyed in all error paths.
♻️ Proposed fix
connectReq.on( "connect", (_res: http.IncomingMessage, socket: net.Socket) => { if (_res.statusCode !== 200) { socket.destroy(); callback( new Error( `ws-proxy-fix: CONNECT ${targetHost}:${targetPort} via proxy failed (${_res.statusCode})`, ), ); return; } const tlsSocket = tls.connect({ socket, servername: (options.servername as string) || targetHost, }); callback(null, tlsSocket); }, ); - connectReq.on("error", (err: Error) => callback(err)); + connectReq.on("error", (err: Error) => { + connectReq.destroy(); + callback(err); + }); connectReq.end();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nemoclaw-blueprint/scripts/ws-proxy-fix.ts` around lines 96 - 117, The error handler for the proxy CONNECT request doesn't destroy the underlying socket, risking leaks; update the connectReq.on("error", (err) => ...) handler to check for and destroy the underlying socket (e.g., call connectReq.socket?.destroy() or use the 'socket' event to capture and destroy the socket) before invoking callback(err), so all error paths (including the existing non-200 branch that destroys socket) consistently clean up the socket associated with connectReq.
🤖 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/ws-proxy-fix.ts`:
- Around line 187-197: The host extraction when building the CONNECT agent can
include a port if opts.hostname is missing and opts.host contains "host:port"
(e.g., "gateway.discord.gg:443"), so change the host computation in the
WebSocket upgrade branch (inside isWsUpgrade handling) to strip any trailing
port from opts.host before passing to createTunnelAgent; specifically, derive
host by using opts.hostname || stripPort(opts.host) || "localhost" (handle IPv6
bracketed addresses correctly by removing a trailing :port only when not inside
[]), keep port parsing from opts.port as before, and then call
createTunnelAgent(host, port) and attach it to opts.agent.
In `@package.json`:
- Line 26: CI is skipping the package prepare hook (which runs "npm run
build:blueprint") because workflows call npm install/ci with --ignore-scripts
and only explicitly run "npm run build:cli", so
nemoclaw-blueprint/scripts/ws-proxy-fix.ts never gets compiled to
ws-proxy-fix.js; update CI workflows to either remove --ignore-scripts (allowing
the existing prepare script to run) or explicitly invoke "npm run
build:blueprint" in the job steps (in addition to "npm run build:cli") so
ws-proxy-fix.ts is compiled before tests run.
In `@test/service-env.test.ts`:
- Around line 809-852: The tests under the "ws-proxy-fix preload (issue `#1570`)"
suite fail in CI because the compiled preload script referenced by wsFixPath is
missing; fix by ensuring the build artifact exists before tests run (preferred):
add a pretest script that runs the build:blueprint step so vitest always has
ws-proxy-fix.js, or make the suite resilient by checking the file (use
node:fs.existsSync on the wsFixPath and conditionally use a skipIfMissing alias
for it/it.skip) so the tests gracefully skip when the artifact is absent.
---
Nitpick comments:
In `@nemoclaw-blueprint/scripts/ws-proxy-fix.ts`:
- Around line 96-117: The error handler for the proxy CONNECT request doesn't
destroy the underlying socket, risking leaks; update the connectReq.on("error",
(err) => ...) handler to check for and destroy the underlying socket (e.g., call
connectReq.socket?.destroy() or use the 'socket' event to capture and destroy
the socket) before invoking callback(err), so all error paths (including the
existing non-200 branch that destroys socket) consistently clean up the socket
associated with connectReq.
🪄 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: 05805cb5-942d-4850-8ad2-fa7648eaf52d
📒 Files selected for processing (7)
.gitignorenemoclaw-blueprint/scripts/ws-proxy-fix.tsnemoclaw-blueprint/tsconfig.jsonpackage.jsonscripts/nemoclaw-start.shtest/service-env.test.tstsconfig.cli.json
The ws-proxy-fix preload tests need the compiled .js file, but CI uses `npm install --ignore-scripts` which skips `prepare` and never runs `tsc -p nemoclaw-blueprint/tsconfig.json`.
This reverts commit 1a836bd.
The ws-proxy-fix preload tests need the compiled .js output but CI runs `npm run build:cli`, not the separate `build:blueprint`. Chain the blueprint tsc into build:cli so the existing CI step covers it.
The preload was monkey-patching https.request() globally for all WebSocket upgrades. Narrow it to *.discord.gg so non-Discord traffic is never touched.
Exercises the exact code path the ws library uses: https.request with Upgrade: websocket headers to gateway.discord.gg. Verifies the ws-proxy-fix preload issues a CONNECT tunnel that results in an HTTP 101 upgrade rather than a 400 from the L7 proxy.
Replace the minimal upgrade probe with a complete gateway handshake: connect via CONNECT tunnel, receive Hello (op 10), send Heartbeat (op 1), receive Heartbeat ACK (op 11), close cleanly. Uses raw WebSocket framing over https.request — the exact code path ws uses — with no external dependencies. Three separate assertions (M13c/d/e) verify each stage independently.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/e2e/test-messaging-providers.sh (1)
704-716: Optionally make the success oracle explicitly101to reduce ambiguity.The current pass condition matches any
UPGRADEtoken. Making itUPGRADE_101(and emitting status-checked output) would make this probe more self-describing and less brittle.Diff suggestion
req.on("upgrade", (res, socket) => { + if (res.statusCode !== 101) { + console.log("HTTP_" + res.statusCode); + socket.destroy(); + return; + } let buf = ""; socket.on("data", (chunk) => { buf += chunk.toString(); if (buf.length > 10) { - console.log("UPGRADE " + buf.slice(0, 200).replace(/[\x00-\x1f]+/g, " ")); + console.log("UPGRADE_101 " + buf.slice(0, 200).replace(/[\x00-\x1f]+/g, " ")); socket.destroy(); } }); setTimeout(() => { if (!socket.destroyed) { - console.log("UPGRADE (no data)"); + console.log("UPGRADE_101 (no data)"); socket.destroy(); } }, 5000); }); @@ -if echo "$dc_ws_tunnel" | grep -q "UPGRADE"; then +if echo "$dc_ws_tunnel" | grep -q "UPGRADE_101"; then pass "M13c: Discord gateway CONNECT tunnel succeeded (ws-proxy-fix `#1570`)"Also applies to: 731-733
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/test-messaging-providers.sh` around lines 704 - 716, The upgrade probe currently logs a generic "UPGRADE" token and matches any upgrade activity; update the req.on("upgrade", ...) handler to emit and match an explicit success marker like "UPGRADE_101" including the HTTP status check: when data is received in the socket handler (the buf accumulation and console.log call), include the actual status code (101) in the logged token (e.g., "UPGRADE_101") and ensure any external success-oracle that parses output looks for "UPGRADE_101" rather than "UPGRADE"; apply the same change to the second occurrence around the lines noted (the other socket/data handler at 731-733) so both probes are status-explicit and less ambiguous.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test/e2e/test-messaging-providers.sh`:
- Around line 704-716: The upgrade probe currently logs a generic "UPGRADE"
token and matches any upgrade activity; update the req.on("upgrade", ...)
handler to emit and match an explicit success marker like "UPGRADE_101"
including the HTTP status check: when data is received in the socket handler
(the buf accumulation and console.log call), include the actual status code
(101) in the logged token (e.g., "UPGRADE_101") and ensure any external
success-oracle that parses output looks for "UPGRADE_101" rather than "UPGRADE";
apply the same change to the second occurrence around the lines noted (the other
socket/data handler at 731-733) so both probes are status-explicit and less
ambiguous.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: deadaa67-3b75-403d-aeb1-5e085d0b012e
📒 Files selected for processing (1)
test/e2e/test-messaging-providers.sh
Contributor
There was a problem hiding this comment.
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 `@test/e2e/test-messaging-providers.sh`:
- Around line 771-807: The upgrade handler currently ignores Node's third
argument (head) and thus drops the initial buffered bytes that may contain the
Hello frame; modify the req.on("upgrade", ...) callback to accept the head
parameter (e.g., (_res, socket, head)) and prepend it into the stream before
processing by calling socket.unshift(head) or by feeding head into the frame
parser (the same path that uses pending, unmaskFrame, and socket.on("data")).
Ensure this happens before entering the socket.on("data") loop so unmaskFrame
can see the initial frame (affecting the Hello/heartbeat flow handled by msg.op
10/11, makeFrame, makeCloseFrame, etc.).
🪄 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: c462dae8-ac82-457c-b5b6-206c3997d0e1
📒 Files selected for processing (1)
test/e2e/test-messaging-providers.sh
- Destroy socket on CONNECT error to prevent resource leaks - Strip port from opts.host to avoid double-port in CONNECT path (e.g. gateway.discord.gg:443:443) - Handle upgrade event head buffer in e2e test to capture initial bytes that may contain the Hello frame
Contributor
Author
|
Addressed all CodeRabbit feedback:
Also scoped the Nightly e2e dispatched on this branch to validate the full CONNECT tunnel round-trip (M13c-e). |
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/e2e/test-messaging-providers.sh (1)
863-873: Consider adding explicitskipmarkers for M13d/M13e when M13c fails.When the WebSocket upgrade doesn't occur (e.g., HTTP_400, TIMEOUT), M13d and M13e silently produce no output. This leaves them absent from the test summary, which could be confusing when reviewing results. An explicit
skipwould improve traceability.💡 Suggested enhancement for explicit skip handling
if echo "$dc_ws_tunnel" | grep -q "HELLO op=10"; then pass "M13d: Received Discord Hello (opcode 10) with heartbeat interval" elif echo "$dc_ws_tunnel" | grep -q "UPGRADED"; then fail "M13d: Upgraded but never received Discord Hello" +else + # Upgrade didn't happen — M13c already reported the failure + skip "M13d: WebSocket upgrade did not complete" fi if echo "$dc_ws_tunnel" | grep -q "HEARTBEAT_ACK op=11"; then pass "M13e: Sent Heartbeat, received ACK (opcode 11) — full round-trip verified" elif echo "$dc_ws_tunnel" | grep -q "SENT_HEARTBEAT"; then fail "M13e: Sent Heartbeat but never received ACK" +else + skip "M13e: Heartbeat exchange did not occur" fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/test-messaging-providers.sh` around lines 863 - 873, Add explicit skip handling for tests M13d and M13e by checking the WebSocket upgrade/outcome in the dc_ws_tunnel output before the existing grep checks: if dc_ws_tunnel contains indicators like "HTTP_400", "TIMEOUT", or does not contain "UPGRADED" (i.e., the upgrade step failed/M13c failed), emit a skip marker for M13d and M13e instead of leaving them silent; otherwise run the existing checks that grep dc_ws_tunnel for "HELLO op=10", "HEARTBEAT_ACK op=11", and "SENT_HEARTBEAT" to pass/fail as currently implemented.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test/e2e/test-messaging-providers.sh`:
- Around line 863-873: Add explicit skip handling for tests M13d and M13e by
checking the WebSocket upgrade/outcome in the dc_ws_tunnel output before the
existing grep checks: if dc_ws_tunnel contains indicators like "HTTP_400",
"TIMEOUT", or does not contain "UPGRADED" (i.e., the upgrade step failed/M13c
failed), emit a skip marker for M13d and M13e instead of leaving them silent;
otherwise run the existing checks that grep dc_ws_tunnel for "HELLO op=10",
"HEARTBEAT_ACK op=11", and "SENT_HEARTBEAT" to pass/fail as currently
implemented.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: e8147d63-d3c5-49ef-b572-25f486495e82
📒 Files selected for processing (2)
nemoclaw-blueprint/scripts/ws-proxy-fix.tstest/e2e/test-messaging-providers.sh
🚧 Files skipped from review as they are similar to previous changes (1)
- nemoclaw-blueprint/scripts/ws-proxy-fix.ts
wscurran
added
Local Models
integration: discord
Contributor
The Dockerfile does a raw COPY of nemoclaw-blueprint/ — the compiled .js was gitignored so it never shipped to the sandbox image. Remove the .gitignore entry and track the compiled output. Add regression tests verifying: - opts.host with port (gateway.discord.gg:443) produces correct CONNECT path without double-port - non-Discord WebSocket upgrades are not intercepted
iamsh4
approved these changes
Apr 22, 2026
Contributor
There was a problem hiding this comment.
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/ws-proxy-fix.js`:
- Around line 79-89: The CONNECT listener currently ignores the third parameter
(head) which may contain initial TLS bytes; update the listener signature to
accept (res, socket, head) in the connectReq.on("connect", ...) callback, create
the TLS socket (tls.connect with { socket, servername: ... }) as before, and if
head && head.length > 0 push those bytes into the TLS stream (e.g., call
tlsSocket.unshift(head)) before invoking callback(null, tlsSocket) so the
tunneled data is preserved for the TLS handshake.
🪄 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: f619d3cf-d7f2-4633-b0e2-907160c2eee3
📒 Files selected for processing (2)
nemoclaw-blueprint/scripts/ws-proxy-fix.jstest/service-env.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- test/service-env.test.ts
The CONNECT event's third argument (head) may contain initial bytes from the tunnel. Unshift them back into the socket before calling tls.connect() so the TLS handshake doesn't drop the first record.
When the WebSocket upgrade doesn't complete, M13d and M13e were silently absent from the test summary. Add else branches that emit skip markers so all three tests always appear in results.
Contributor
Author
|
Fixed the M13d/M13e skip nitpick from CodeRabbit in cc0d6ef — both now emit explicit |
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
nemoclaw-blueprint/scripts/ws-proxy-fix.js (1)
72-99: Consider adding a timeout to the CONNECT request.If the proxy becomes unresponsive, the CONNECT request will hang indefinitely with no timeout. While the sandbox environment has a known proxy, adding a reasonable timeout would improve resilience.
♻️ Optional: Add timeout to CONNECT request
const connectReq = node_http_1.default.request({ host: proxyHost, port: proxyPort, method: "CONNECT", path: `${targetHost}:${targetPort}`, headers: { Host: `${targetHost}:${targetPort}` }, + timeout: 30000, // 30s timeout for CONNECT handshake }); + connectReq.on("timeout", () => { + connectReq.destroy(new Error(`ws-proxy-fix: CONNECT ${targetHost}:${targetPort} timed out`)); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nemoclaw-blueprint/scripts/ws-proxy-fix.js` around lines 72 - 99, The CONNECT request currently created via node_http_1.default.request (connectReq) can hang if the proxy is unresponsive; add a timeout on connectReq (e.g., set a small constant like 10_000 ms or use an injected option) and handle it by destroying the request/socket and invoking the existing callback with an Error; ensure you wire the timeout handler alongside the existing connectReq.on("error", ...) and clear the timeout/handlers once the "connect" event succeeds to avoid leaks (update logic around connectReq, connectReq.on("connect"), connectReq.on("error"), and connectReq.end()).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@nemoclaw-blueprint/scripts/ws-proxy-fix.js`:
- Around line 72-99: The CONNECT request currently created via
node_http_1.default.request (connectReq) can hang if the proxy is unresponsive;
add a timeout on connectReq (e.g., set a small constant like 10_000 ms or use an
injected option) and handle it by destroying the request/socket and invoking the
existing callback with an Error; ensure you wire the timeout handler alongside
the existing connectReq.on("error", ...) and clear the timeout/handlers once the
"connect" event succeeds to avoid leaks (update logic around connectReq,
connectReq.on("connect"), connectReq.on("error"), and connectReq.end()).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 42ea3b8c-9bf4-4f8d-bde1-96d5c9301c7e
📒 Files selected for processing (2)
nemoclaw-blueprint/scripts/ws-proxy-fix.jsnemoclaw-blueprint/scripts/ws-proxy-fix.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- nemoclaw-blueprint/scripts/ws-proxy-fix.ts
brandonpelfrey
approved these changes
Apr 22, 2026
cv
approved these changes
Apr 22, 2026
cv
left a comment
cv
left a comment
Collaborator
There was a problem hiding this comment.
Security pass looks good: the preload is scoped to Discord WebSocket upgrades, non-Discord HTTPS traffic is left alone, and the PR adds both unit coverage plus an end-to-end CONNECT/Hello/heartbeat regression path for #1570.
I also re-ran npm run build:cli, npx vitest run test/service-env.test.ts, and the maintainer gate checker locally; all passed.
lcsmontiel
added a commit
to lcsmontiel/NemoClaw
that referenced
this pull request
Apr 23, 2026
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.
8 tasks
5 tasks
Closed
2 tasks
5 tasks
wscurran
added
area: local-models
wscurran
added
NV QA
2 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
5 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
ws-proxy-fix.tspreload script that patcheshttps.request()to detect WebSocket upgrade requests and inject a CONNECT tunnel agent, fixing Discord gateway connections through the OpenShell L7 proxynemoclaw-start.sh(entrypoint + proxy-env.sh persistence for connect sessions)tsconfig.json,build:blueprintscript)Closes #1570
Details
Node.js 22's
EnvHttpProxyAgent(activated byNODE_USE_ENV_PROXY=1) sends forward proxy requests instead of CONNECT tunnels for HTTPS WebSocket upgrades. The OpenShell L7 proxy correctly rejects these with HTTP 400, breaking the Discord gateway connection.The preload intercepts
https.request()calls that contain anUpgrade: websocketheader and replaces the default agent with one that establishes a proper CONNECT tunnel through the proxy, then upgrades to TLS. Non-WebSocket HTTPS requests pass through unchanged.Belt-and-suspenders: if the caller (OpenClaw) already provides a custom agent, the preload steps aside — no double-tunnelling. Works regardless of any upstream OpenClaw fix.
Test plan
npx vitest run test/service-env.test.ts— 39 tests pass (5 new)npm run build:blueprint— clean TS compilationnpm run typecheck:cli— clean type-checknpx prek run --all-files— all relevant hooks pass (test-cli failures are pre-existing environmental)Summary by CodeRabbit
New Features
Bug Fixes
Chores
Tests