fix(inference): shim globalThis.fetch to proxy inference.local for cr…#4916
Open
Abhi190702 wants to merge 5 commits into
Open
fix(inference): shim globalThis.fetch to proxy inference.local for cr…#4916Abhi190702 wants to merge 5 commits into
Abhi190702 wants to merge 5 commits into
Conversation
Contributor
|
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:
📝 WalkthroughWalkthroughThis PR extends the http-proxy-fix preload to intercept fetch() calls to https://inference.local/* and route them through the existing FORWARD-mode http.request rewrite, adds proxy parsing and an idempotent fetch patch, updates start-script docs, aligns test environment vars, adds fetch regression tests, and makes the sync test conditional on Bash availability. ChangesProxy Fetch Shim and Alignment
Test Infrastructure Robustness
Sequence Diagram(s)sequenceDiagram
participant Client
participant FetchShim as Fetch Shim
participant HttpWrapper as http.request wrapper
participant Proxy
participant Destination as Real Host
Client->>FetchShim: fetch('https://inference.local/...')
FetchShim->>FetchShim: normalize URL, copy headers/body
FetchShim->>HttpWrapper: http.request(proxyHost:proxyPort, path=fullTargetURL)
HttpWrapper->>HttpWrapper: rewrite FORWARD-mode -> https.request(destHost, path)
HttpWrapper->>Proxy: CONNECT / tunneled request
Proxy->>Destination: tunnel to destination
Destination-->>Proxy: response stream
Proxy-->>HttpWrapper: proxied stream
HttpWrapper-->>FetchShim: IncomingMessage stream
FetchShim-->>Client: Response (stream/body)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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.
🧹 Nitpick comments (2)
scripts/nemoclaw-start.sh (1)
2058-2072: Queue sandbox entrypoint E2Es for this change.Because this script affects sandbox boot/runtime boundaries that unit tests miss, run:
gh workflow run nightly-e2e.yaml --ref <branch> -f jobs=sandbox-survival-e2e,sandbox-operations-e2e,cloud-e2e,openclaw-slack-pairing-e2eAs per coding guidelines,
scripts/nemoclaw-start.shchanges should be validated with the listed sandbox E2E suites.🤖 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 `@scripts/nemoclaw-start.sh` around lines 2058 - 2072, The change to scripts/nemoclaw-start.sh must be validated by queuing the specified sandbox E2E suites before merging: run the GitHub Actions workflow nightly-e2e.yaml against your branch and pass the jobs input set to sandbox-survival-e2e,sandbox-operations-e2e,cloud-e2e,openclaw-slack-pairing-e2e (e.g. using the gh CLI: gh workflow run nightly-e2e.yaml --ref <branch> -f jobs=sandbox-survival-e2e,sandbox-operations-e2e,cloud-e2e,openclaw-slack-pairing-e2e); ensure the workflow completes successfully for scripts/nemoclaw-start.sh changes and block merge until those E2E suites pass.Source: Coding guidelines
nemoclaw-blueprint/scripts/http-proxy-fix.js (1)
23-27: Run the proxy-chain E2Es for this L7 rewrite before merge.This path is explicitly marked for manual FORWARD-mode validation; run:
gh workflow run nightly-e2e.yaml --ref <branch> -f jobs=cloud-e2e,inference-routing-e2eAs per coding guidelines,
nemoclaw-blueprint/scripts/http-proxy-fix.jschanges require manual validation usingcloud-e2eandinference-routing-e2e.Also applies to: 246-260, 329-329
🤖 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-blueprint/scripts/http-proxy-fix.js` around lines 23 - 27, This change in nemoclaw-blueprint/scripts/http-proxy-fix.js touches a manual FORWARD-mode L7 rewrite that must be validated by running the cloud and inference routing end-to-end suites before merging: run the GitHub workflow nightly-e2e.yaml with ref set to your branch and pass -f jobs=cloud-e2e,inference-routing-e2e (gh workflow run nightly-e2e.yaml --ref <branch> -f jobs=cloud-e2e,inference-routing-e2e), exercise the https://inference.local/* fetch wrapper and confirm FORWARD-mode behavior, and repeat validation for the other affected regions noted (around the other changes at the same file ranges) before approving the PR.Source: Coding guidelines
🤖 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-blueprint/scripts/http-proxy-fix.js`:
- Around line 23-27: This change in nemoclaw-blueprint/scripts/http-proxy-fix.js
touches a manual FORWARD-mode L7 rewrite that must be validated by running the
cloud and inference routing end-to-end suites before merging: run the GitHub
workflow nightly-e2e.yaml with ref set to your branch and pass -f
jobs=cloud-e2e,inference-routing-e2e (gh workflow run nightly-e2e.yaml --ref
<branch> -f jobs=cloud-e2e,inference-routing-e2e), exercise the
https://inference.local/* fetch wrapper and confirm FORWARD-mode behavior, and
repeat validation for the other affected regions noted (around the other changes
at the same file ranges) before approving the PR.
In `@scripts/nemoclaw-start.sh`:
- Around line 2058-2072: The change to scripts/nemoclaw-start.sh must be
validated by queuing the specified sandbox E2E suites before merging: run the
GitHub Actions workflow nightly-e2e.yaml against your branch and pass the jobs
input set to
sandbox-survival-e2e,sandbox-operations-e2e,cloud-e2e,openclaw-slack-pairing-e2e
(e.g. using the gh CLI: gh workflow run nightly-e2e.yaml --ref <branch> -f
jobs=sandbox-survival-e2e,sandbox-operations-e2e,cloud-e2e,openclaw-slack-pairing-e2e);
ensure the workflow completes successfully for scripts/nemoclaw-start.sh changes
and block merge until those E2E suites pass.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 8bb8c76b-ef65-451b-8a27-18f945b49a7a
📒 Files selected for processing (6)
nemoclaw-blueprint/scripts/http-proxy-fix.jsscripts/nemoclaw-start.shtest/http-proxy-fix-e2e.test.tstest/http-proxy-fix-fetch.test.tstest/http-proxy-fix-rewrite.test.tstest/http-proxy-fix-sync.test.ts
💤 Files with no reviewable changes (2)
- test/http-proxy-fix-rewrite.test.ts
- test/http-proxy-fix-e2e.test.ts
wscurran
added
area: inference
Contributor
|
✨ Thanks for submitting this detailed PR about shimming globalThis.fetch to proxy inference.local for cron agentTurn jobs using a local inference provider. This proposes a way to fix the regression in the inference area. Related open issues: |
Author
|
Thanks for the review. On the two nitpicks from CodeRabbit — I don't have access to Let me know if there's |
Abhi190702
force-pushed
the
fix/4730-fetch-proxy-inference-local
branch
2 times, most recently
from
cc75ff1 to
6938c37
Compare
…on preflight Cron agentTurn jobs using a local Ollama provider were being skipped because the cron scheduler's provider preflight uses fetch() (undici) rather than http.request(), bypassing the OpenShell sandbox proxy that routes the virtual hostname inference.local to the local Ollama instance. The existing http-proxy-fix.js already patches http.request and https.request to honour proxy env variables. The preflight never goes through that path — it calls fetch() directly, which resolves inference.local via raw DNS, gets EAI_AGAIN, and marks the provider unreachable, causing the cron job to be skipped entirely. Extended http-proxy-fix.js to wrap globalThis.fetch for requests whose hostname is inference.local, routing them through the existing proxy-aware path. Every other hostname passes through the original fetch unchanged. The shim is: - Idempotent via __nemoclawFetchPatched guard - A no-op when typeof globalThis.fetch !== 'function' (Node < 18) - Scoped strictly to inference.local via URL hostname parsing - Non-destructive to the existing http.request/https.request patch - Free of hardcoded replacement addresses - Does not modify NO_PROXY or the managed inference.local routing design Also fixed in this commit: - Pre-existing Windows env case collision (HTTPS_PROXY vs https_proxy) in the existing proxy test suite causing false failures on Windows - Bash-dependent sync tests now skip locally when Bash/WSL is unavailable, but fail in CI to catch real regressions Verification: - npm test (proxy-focused suites) passed - npm run typecheck passed - node --check http-proxy-fix.js passed - git diff --check passed Fixes NVIDIA#4730 # Conflicts: # test/http-proxy-fix-sync.test.ts
Abhi190702
force-pushed
the
fix/4730-fetch-proxy-inference-local
branch
from
6938c37 to
564b9a3
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
nemoclaw-blueprint/scripts/http-proxy-fix.js (1)
169-178: 💤 Low valueFallback at line 177 could be more explicit.
The fallback
return res;would fail when passed to the Response constructor (which expects a Web stream), but this edge case is unlikely in Node 18+ whereReadable.toWebis available. Consider throwing an error or adding a comment explaining this is unreachable in the target environment.💡 Optional: Make the fallback more explicit
var stream = require('stream'); if (stream.Readable && typeof stream.Readable.toWeb === 'function') { return stream.Readable.toWeb(res); } - return res; + // Unreachable in Node 18+ (target environment); Response constructor requires Web stream + throw new Error('stream.Readable.toWeb unavailable; Node 18+ required');🤖 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-blueprint/scripts/http-proxy-fix.js` around lines 169 - 178, In responseBody, the fallback "return res" can produce an invalid value for the Response constructor; update the function (responseBody) to make this case explicit by either throwing a clear Error (e.g., "Readable.toWeb not available: cannot convert response stream") or by adding a comment asserting the branch is unreachable in our Node >=18 target and adding a defensive throw to fail fast; change the final fallback in responseBody to one of those options so callers never receive an unsupported value.
🤖 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-blueprint/scripts/http-proxy-fix.js`:
- Around line 169-178: In responseBody, the fallback "return res" can produce an
invalid value for the Response constructor; update the function (responseBody)
to make this case explicit by either throwing a clear Error (e.g.,
"Readable.toWeb not available: cannot convert response stream") or by adding a
comment asserting the branch is unreachable in our Node >=18 target and adding a
defensive throw to fail fast; change the final fallback in responseBody to one
of those options so callers never receive an unsupported value.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 27fc137d-6f15-4e9b-a8b9-9d495f322f41
📒 Files selected for processing (6)
nemoclaw-blueprint/scripts/http-proxy-fix.jsscripts/nemoclaw-start.shtest/http-proxy-fix-e2e.test.tstest/http-proxy-fix-fetch.test.tstest/http-proxy-fix-rewrite.test.tstest/http-proxy-fix-sync.test.ts
💤 Files with no reviewable changes (2)
- test/http-proxy-fix-rewrite.test.ts
- test/http-proxy-fix-e2e.test.ts
✅ Files skipped from review due to trivial changes (1)
- scripts/nemoclaw-start.sh
🚧 Files skipped from review as they are similar to previous changes (2)
- test/http-proxy-fix-sync.test.ts
- test/http-proxy-fix-fetch.test.ts
Signed-off-by: ABHIJEET RANJAN <abhijeet.r1907@gmail.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
4 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
This PR fixes a bug where cron
agentTurnjobs using a local inference provider (Ollama or vLLM, configured to map via the virtual hosthttps://inference.local/v1) are skipped.At execution time, the OpenClaw cron scheduler runs a "provider preflight" check using Node’s native
fetch()(backed byundiciin Node 18+). Sincefetch()bypasses the legacyhttp.requestnetwork stack, it bypasses the sandbox's existing proxy-rewrite logic, attempts raw DNS resolution for the virtual hostnameinference.local, and fails withgetaddrinfo EAI_AGAIN. By shimmingfetch()forinference.localrequests, the preflight check is correctly routed through the OpenShell sandbox proxy.Related Issue
Fixes #4730
Changes
http-proxy-fix.jsto ShimglobalThis.fetch:fetch(only on Node runtimes >= 18).inference.local(all other hosts bypass the wrapper).http.requestproxy rewrite.stream.Readable.toWeb(res)to preserve full streaming compatibility for downstream LLM consumers.__nemoclawFetchPatchedto remain completely idempotent when loaded multiple times.HTTPS_PROXYandhttps_proxyintest/http-proxy-fix-e2e.test.tsandtest/http-proxy-fix-rewrite.test.tsto prevent test failures on Windows hosts where the environment block is case-insensitive.test/http-proxy-fix-sync.test.tsif a usablebashshell is not available locally, preventing false failures on Windows machines while ensuring the tests still fail and block regressions in Linux-based CI environments.test/http-proxy-fix-fetch.test.tswith coverage for fetch rewrite, passthrough, idempotency, and Node < 18 fallback logic.Type of Change
Verification
npx prek run --all-filespasses (verified locally, excluding missing system-levelhadolinttool on Windows environment)npm testpasses (verified allhttp-proxy-fixsuites pass cleanly on Windows)npm run docsbuilds without warnings (doc changes only)Signed-off-by: ABHIJEET RANJAN abhijeet.r1907@gmail.com
Summary by CodeRabbit
Bug Fixes
Tests
Documentation