refactor(runtime): extract entrypoint preload modules#3109
Conversation
This reverts commit 4ebeae4.
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (16)
📝 WalkthroughWalkthroughThis PR extracts multiple inline Node.js preload scripts into external blueprint files, updates the Dockerfile to install those preloads into /usr/local/lib/nemoclaw/preloads/, rewires scripts/nemoclaw-start.sh to emit and load preloads at runtime via NODE_OPTIONS, and updates tests to reference the new preload sources. ChangesPreloads extraction, Dockerfile install, startup wiring & tests
Sequence Diagram(s)sequenceDiagram
participant Docker as Docker build
participant Image as Runtime Image (/usr/local/lib/nemoclaw/preloads)
participant Start as scripts/nemoclaw-start.sh
participant Node as Node process (NODE_OPTIONS preload)
participant Backend as application modules / network
Docker->>Image: COPY blueprint preload scripts + plugins
Image->>Start: preloads available at build-time path
Start->>Node: emit preload files to /tmp and set NODE_OPTIONS --require
Node->>Node: load preloads (guards, diagnostics, fixes) at require time
Node->>Backend: runtime behavior (HTTP intercepts, network guards, safety net)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
cjagwani
left a comment
There was a problem hiding this comment.
byte-equivalent extraction — verified for all 8 modules. emit_sandbox_sourced_file <"$_SOURCE" is a shell-level stdin copy, no parsing or substitution can drift. sync tests (http-proxy-fix-sync, slack-token-rewriter-sync) enforce equality. /usr/local/lib/nemoclaw/preloads/ install path is immutable + atomic mv to /tmp with 444 root:root.
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
prekshivyas
left a comment
There was a problem hiding this comment.
Pure code-organization move. Six runtime preload bodies (sandbox-safety-net, ciao-network-guard, slack-channel-guard, telegram-diagnostics, nemotron-inference-fix, seccomp-guard) extracted from scripts/nemoclaw-start.sh heredocs into standalone .js files under nemoclaw-blueprint/scripts/. Spot-verified each new module against the deleted heredoc body — byte-for-byte identical JS logic, no behavior change. The two pre-existing modules (http-proxy-fix.js, slack-token-rewriter.js) only got header comment updates documenting the new Dockerfile-copy path; bodies unchanged.
Architecture: Dockerfile now does COPY nemoclaw-blueprint/scripts/*.js /usr/local/lib/nemoclaw/preloads/ at image build with mode 644. Entrypoint reads via <\"$..._SOURCE\" redirect to write into /tmp for NODE_OPTIONS=--require, preserving the Landlock-readable staging path that OpenShell ≥0.0.36 requires for non-root sandbox processes. The ws-proxy-fix.js path also reorganized to /usr/local/lib/nemoclaw/preloads/ for consistency — declared via the Dockerfile diff and reflected in seccomp-guard.test.ts.
Security review: preloads remain root-owned mode 644, Landlock copy-to-/tmp mechanism preserved verbatim, NODE_OPTIONS=--require /tmp/... registration unchanged. No new attack surface — JS bodies move from one source-controlled location to another, both auditable and version-controlled. Byte-for-byte sync tests (http-proxy-fix-sync.test.ts, slack-token-rewriter-sync.test.ts) still pin canonical bytes; with the new architecture the entrypoint literally < redirects from the same canonical file, so byte equality is intrinsic.
Test adaptations are right-sized: startScriptHeredoc() helper falls back to reading the preload file when the heredoc marker isn't present, end-to-end behavior assertions unchanged. Each affected test gets a _..._SOURCE substitution to point at the local copy. local-slack-auth-test.sh swaps sed-extract-from-script for cp $GUARD_SOURCE $GUARD_JS — cleaner.
Minor nit (non-blocking): Dockerfile comment "Copy NODE_OPTIONS preload modules to a Landlock-accessible path" reads slightly imprecise — /usr/local/lib/nemoclaw/preloads/ is itself blocked by Landlock for non-root, which is exactly why the entrypoint stages copies under /tmp. Phrasing nit only.
CI: pr.yaml lightweight checks pass (commit-lint, dco, layer-boundary, check-hash, changes, get-pr-info). build-sandbox-images (validates the Dockerfile reorganization) and other rollup checks plus self-hosted run still in progress at approval time.
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Follow-up to NVIDIA#2344 (the http.request wrapper for NVIDIA#2109). PR NVIDIA#2344 covers axios / follow-redirects / proxy-from-env, all of which flow through Node's http.request and EnvHttpProxyAgent. This change covers an additional code path the Microsoft Teams adapter uses: native fetch() with a custom undici dispatcher. When OpenClaw's Teams adapter handles a channel @mention it calls the Microsoft Graph API via: fetch('https://graph.microsoft.com/v1.0/teams/.../messages/...', { dispatcher: customUndiciAgent }) The custom dispatcher routes the request through its own connection pool, bypassing the default EnvHttpProxyAgent — which is the only thing routing HTTPS through the OpenShell L7 proxy. Direct egress to graph.microsoft.com is blocked by the sandbox network namespace and surfaces as ECONNREFUSED. Channel @mentions silently fail; DMs work because the webhook payload carries the message body and no Graph call is needed. Wrap globalThis.fetch in the same preload that wraps http.request. When fetch() is called with a custom dispatcher and an HTTPS URL, strip the dispatcher and let EnvHttpProxyAgent handle the request. Non-HTTPS URLs and dispatcher-free calls pass through unchanged so intra-sandbox HTTP traffic and any non-proxy use of the dispatcher option keep working. Refinements over the originally proposed fix: - URL extraction handles all three fetch() input forms — string, URL object (.href), Request object (.url) — in that order. A naive url?.url check would miss URL instances (which have .href, not .url) and silently leave the dispatcher attached, defeating the fix on callers that pass URL instances. - One-shot console.warn so the strip is auditable in logs without spamming on every call. The flag is closure-scoped so a process restart re-arms it. - Defensive typeof origFetch === function guard so a Node runtime that ever ships without globalThis.fetch silently no-ops instead of throwing at preload time. After NVIDIA#3109 the preload is delivered as a standalone module copied from /usr/local/lib/nemoclaw/preloads/ at boot, so this PR only touches the canonical http-proxy-fix.js — no heredoc to keep in sync. http-proxy-fix-sync.test.ts already runs the entrypoint block end-to- end and reads the generated file; extended with explicit assertions for http.request and globalThis.fetch wrapper presence so a future deletion of either wrapper trips the test. Verified end-to-end on a real proxy-enabled sandbox: bot replies to Teams channel @mentions; DM regression check passes; non-Graph fetch() and fetch() without a custom dispatcher unaffected.
Summary
Extracts the Node.js runtime preload bodies from
scripts/nemoclaw-start.shinto standalone preload modules undernemoclaw-blueprint/scripts/. The entrypoint now copies those modules from a Landlock-readable install path into/tmpbefore registering them viaNODE_OPTIONS.Changes
nemoclaw-start.shto install preloads from files instead of inline heredocs./usr/local/lib/nemoclaw/preloads/in the sandbox image.Type of Change
Verification
npx prek run --all-filespassesnpm testpassesmake docsbuilds without warnings (doc changes only)Signed-off-by: Carlos Villela cvillela@nvidia.com
Summary by CodeRabbit
New Features
Bug Fixes
Chores