fix(inference): shim globalThis.fetch to proxy inference.local for cr… - #4916
fix(inference): shim globalThis.fetch to proxy inference.local for cr…#4916Abhi190702 wants to merge 20 commits into
Conversation
|
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}
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
|
✨ 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: |
|
Thanks for the review. On the two nitpicks from CodeRabbit — I don't have access to Let me know if there's |
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
6938c37 to
564b9a3
Compare
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>
…iance Signed-off-by: ABHIJEET RANJAN <abhijeet.r1907@gmail.com>
Manual PR Review Advisor resultThis PR Review Advisor analysis was run manually via Run: https://github.com/NVIDIA/NemoClaw/actions/runs/28206392670 PR Review AdvisorThe scoped inference.local fetch shim is directionally appropriate, but the security-sensitive preload needs bounded body handling, harder idempotence, and stronger regression proof for the actual cron-preflight workaround. Required before merge
Resolve or justify before merge
In-scope improvements
Test follow-ups to resolve or justify
What looks good
|
Signed-off-by: ABHIJEET RANJAN <abhijeet.r1907@gmail.com>
|
Thanks for the detailed advisor review. I pushed a follow-up hardening pass for the Latest follow-up commit: What this follow-up is trying to solveThe original fix added a narrow The advisor review called out a few important hardening points around this preload path:
This follow-up addresses those points without expanding the PR into unrelated CLI tests, package changes, workflow changes, source-shape budget changes, or a larger E2E framework. Files changed in this follow-upThe final hardening diff is limited to: No package files, workflow files, source-shape budget files, or unrelated CLI tests were changed. 1. Bounded request-body handlingPreviously, non-GET/HEAD That created two risks:
I added an explicit bounded policy for New body policyThe shim now uses a documented 1 MiB maximum body size for The new behavior is:
Content-Length validationThe
This is meant to keep the preload from becoming an unbounded buffering path while still supporting the small JSON-style provider/preflight requests this route is meant for. 2. Hardened fetch patch idempotenceThe previous idempotence check relied on this public mutable flag: globalThis.__nemoclawFetchPatchedThe advisor pointed out that a stale or colliding value there could silently prevent the shim from installing, leaving I changed the idempotence check so the shim verifies the actual installed fetch function instead. The wrapper is marked with: __nemoclawInferenceLocalProxyFix === trueThe preload now checks whether ResultThis means:
3. Source-boundary and removal-condition documentationI expanded the NemoClaw#4730 comment block in The comment now documents: Invalid stateOpenClaw cron/provider preflight can call native That can trigger a raw DNS lookup and fail with: because Source boundaryThis file is the sandbox preload transport boundary. It is loaded at sandbox boot through the preload mechanism and is the controlled layer where this repo can keep Source-fix constraintThe cron/provider preflight path is version-coupled / external to this localized preload fix. I did not try to rewrite broader cron/provider logic in this PR because that would expand the scope significantly beyond the current transport bug. Regression proofThe regression tests prove the transport boundary behavior controlled by this preload: Removal conditionThe comment now states that this shim should be removed once OpenClaw cron/provider preflight uses the sandbox proxy-aware provider route directly, or once an upgraded OpenClaw version no longer uses raw native fetch for 4. Regression coverage addedI expanded The fetch test suite now covers: Routing / scope
Idempotence
Body safety
Header boundaryThe fetch path now has regression coverage for proxy-hop header stripping. The test verifies that proxy-hop headers are not leaked to the final target request, including: It also verifies that target-intent headers are preserved, including: URL preservationThe fetch rewrite now has explicit coverage for preserving: for a URL like: 5. Cron/provider-preflight regression noteI looked for a small stable cron/provider-preflight entry point that could be invoked directly in this repo without pulling in a much broader E2E surface. I could not find one that looked appropriate for a focused regression in this PR. Because of that, I intentionally kept the proof at the boundary this preload controls: This proves the transport behavior needed for the provider-preflight workaround while avoiding a large new cron/E2E harness in this patch. The source-boundary comment now also documents why this localized behavior exists and when it should be removed. Advisor finding mapping
Local verificationI ran the focused and related checks locally. Result: Related proxy tests: Source-shape: Diff check: Hadolint: Hadolint also passed in the local static run. Scope intentionally not changedI intentionally did not change:
The follow-up is limited to hardening the |
E2E Advisor RecommendationRequired E2E: Dispatch hint: Full advisor summaryE2E Recommendation AdvisorBase: Required E2E
Optional E2E
New E2E recommendations
Dispatch hint
|
|
Closing as superseded by #5129, which merged the fix for the same linked issue (#4730). I rechecked this PR's exact head and current
There is therefore no remaining linked behavior for this broader global fetch shim to land. Keeping it would add unnecessary long-lived transport semantics, including request-body buffering and global wrapper/idempotence behavior, for a bug already fixed at the narrower source boundary. Thank you for the investigation and implementation work here. If a different non-cron |
5 participants
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