Repository navigation
fix(mcp): inject yes flag for npx servers - #5670
Conversation
|
Important Review skippedNo new commits to review since the last review. ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdds a build-time OpenClaw patch that prepends ChangesOpenClaw MCP npx normalization patch
Estimated code review effort: 4 (Complex) | ~50 minutes Sequence Diagram(s)sequenceDiagram
actor DockerBuild as Docker Build
participant PatchScript as patch-openclaw-mcp-npx.mts
participant OpenClawDist as OpenClaw dist
participant MCP as MCP stdio transport
DockerBuild->>PatchScript: Run patch against OpenClaw dist
PatchScript->>OpenClawDist: Scan and rewrite bundled JavaScript
PatchScript->>OpenClawDist: Inject npx normalization and timeout helpers
OpenClawDist-->>PatchScript: Write patched files
PatchScript-->>DockerBuild: Return patch status
MCP->>MCP: Normalize npx args with -y
MCP-->>MCP: Report contextual redacted timeout message
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
2f5970d to
1ba644b
Compare
npx-backed MCP servers can block on a cold package resolution prompt before the MCP initialize handshake completes. When that happens, the prompt consumes the stdio channel that should carry JSON-RPC and OpenClaw reports a generic startup timeout. Patch the bundled OpenClaw MCP stdio transport during the image build so npx commands receive -y unless -y or --yes is already present. The patch leaves non-npx commands untouched, fails closed on unexpected source shapes, and rewrites timeout diagnostics to include the server name, timeout, redacted command context, and an npx-specific remediation hint. Add focused tests that execute the patched output and verify idempotency, non-npx behavior, --yes handling, redaction, and build-context staging. Fixes NVIDIA#5120 Signed-off-by: Abhijeet Ranjan <abhijeet.r1907@gmail.com>
1ba644b to
a28d568
Compare
wscurran
added
area: integrations
|
✨ Thanks for the proposed fix that adds a build-time compatibility patch for OpenClaw's MCP StdioClientTransport to automatically prepend the -y flag when npx is detected. This proposes a way to prevent npx from blocking the MCP stdio initialize handshake with an interactive install prompt while preserving existing MCP server configs. Related open issues: |
|
Thanks for taking a look. I kept the PR focused on the Happy to adjust the approach if you’d prefer this handled differently from the build-time compatibility patch. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
Dockerfile (1)
517-525: 🩺 Stability & Availability | 🔵 TrivialRun the Dockerfile-specific E2E set before merge.
This patch is build-layer behavior, so validate via real image build/runtime jobs (
cloud-e2e,sandbox-survival-e2e,hermes-e2e,rebuild-openclaw-e2e,openclaw-tui-chat-correlation-e2e) using the providedgh workflow run nightly-e2e.yaml ...command.As per path instructions,
Dockerfilechanges are only testable with a real container build and include that exact E2E recommendation set.🤖 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 `@Dockerfile` around lines 517 - 525, The Dockerfile changes involving the OpenClaw MCP npx patch require validation through a real container build and runtime. Before merging this PR, run the Dockerfile-specific E2E test jobs to ensure the patch works correctly: cloud-e2e, sandbox-survival-e2e, hermes-e2e, rebuild-openclaw-e2e, and openclaw-tui-chat-correlation-e2e. Execute these jobs using the gh workflow run nightly-e2e.yaml command to verify the build-layer behavior and patch application are functioning as expected.Source: Path instructions
scripts/patch-openclaw-mcp-npx.mts (1)
236-251: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDead fallback branches that would emit invalid JS if ever reached.
buildTimeoutReplacementonly ever returns a string starting withnemoClawMcpTimeoutMessage(or throws, so thestartsWith("nemoClawMcpTimeoutMessage(")checks on lines 240 and 248 are always true and theelsereturns on lines 241 and 249 are unreachable. Worse, that dead code is itself broken:${TIMEOUT_HINT}is interpolated into a double-quoted JS string literal, andTIMEOUT_HINTcontains"-y", so the produced output would have unescaped quotes and fail to parse. Recommend dropping the dead branches to avoid a future foot-gun.♻️ Simplify the .concat / + replacements
text = text.replace( /"MCP server connection timed out after "\.concat\(([^,]+),\s*"ms"\)/g, - (_match: string, expression: string, offset: number) => { - const replacement = buildTimeoutReplacement(source, expression, offset); - if (replacement.startsWith("nemoClawMcpTimeoutMessage(")) return replacement; - return `"MCP server connection timed out after ".concat(${expression}, "ms. ${TIMEOUT_HINT}")`; - }, + (_match: string, expression: string, offset: number) => + buildTimeoutReplacement(source, expression, offset), ); text = text.replace( /"MCP server connection timed out after "\s*\+\s*([^+]+?)\s*\+\s*"ms"/g, - (_match: string, expression: string, offset: number) => { - const replacement = buildTimeoutReplacement(source, expression, offset); - if (replacement.startsWith("nemoClawMcpTimeoutMessage(")) return replacement; - return `"MCP server connection timed out after " + ${expression.trim()} + "ms. ${TIMEOUT_HINT}"`; - }, + (_match: string, expression: string, offset: number) => + buildTimeoutReplacement(source, expression, offset), );🤖 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/patch-openclaw-mcp-npx.mts` around lines 236 - 251, The two replacement handlers in the text.replace calls are checking if the buildTimeoutReplacement result starts with "nemoClawMcpTimeoutMessage(" and only returning it if true, with fallback returns that are unreachable dead code. Since buildTimeoutReplacement only ever returns a string starting with "nemoClawMcpTimeoutMessage(" or throws an error, remove the if-startsWith checks from both handlers (the one handling the .concat pattern and the one handling the + operator pattern) and simply return the replacement directly from buildTimeoutReplacement in each case.
🤖 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 `@Dockerfile`:
- Around line 517-525: The Dockerfile changes involving the OpenClaw MCP npx
patch require validation through a real container build and runtime. Before
merging this PR, run the Dockerfile-specific E2E test jobs to ensure the patch
works correctly: cloud-e2e, sandbox-survival-e2e, hermes-e2e,
rebuild-openclaw-e2e, and openclaw-tui-chat-correlation-e2e. Execute these jobs
using the gh workflow run nightly-e2e.yaml command to verify the build-layer
behavior and patch application are functioning as expected.
In `@scripts/patch-openclaw-mcp-npx.mts`:
- Around line 236-251: The two replacement handlers in the text.replace calls
are checking if the buildTimeoutReplacement result starts with
"nemoClawMcpTimeoutMessage(" and only returning it if true, with fallback
returns that are unreachable dead code. Since buildTimeoutReplacement only ever
returns a string starting with "nemoClawMcpTimeoutMessage(" or throws an error,
remove the if-startsWith checks from both handlers (the one handling the .concat
pattern and the one handling the + operator pattern) and simply return the
replacement directly from buildTimeoutReplacement in each case.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: c80c05bf-f89a-4c9e-b3a2-7dd4f38c030f
📒 Files selected for processing (5)
Dockerfilescripts/patch-openclaw-mcp-npx.mtssrc/lib/sandbox/build-context.tstest/openclaw-mcp-npx-patch.test.tstest/sandbox-build-context.test.ts
|
This PR is currently conflicted, and commit |
# Conflicts: # Dockerfile # src/lib/sandbox/build-context.ts # test/sandbox-build-context.test.ts
E2E Target Results — ❌ Some jobs failedRun: 29103669399
|
Patch OpenClaw StdioClientTransport bundles even when they omit the timeout diagnostic. Timeout-message rewrites still fail closed when that diagnostic is present but unknown. Add regression coverage for transport-only bundles. Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
|
@coderabbitai review |
✅ Action performedReview finished.
|
E2E Target Results — ❌ Some jobs failedRun: 29105481092
|
E2E Target Results — ✅ All selected jobs passedRun: 29106517570
|
|
Exact-head follow-up at |
## Summary Release-prep documentation for **v0.0.80**. Adds the `## v0.0.80` section to `docs/about/release-notes.mdx` summarizing user-facing changes since v0.0.79, each bullet linking to the relevant deeper page. Produced via `nemoclaw-contributor-update-docs` (pre-tag path): scanned `v0.0.79..HEAD`, applied the docs skip list (no violations), and confirmed the 8 commits that already shipped in-PR docs are complete. No new pages needed. ## Source summary - #6507 -> `docs/about/release-notes.mdx`: Hermes v0.18 + Slack Block Kit (rich rendering, digest-pinned base image). - #6584 / #6616 -> `docs/about/release-notes.mdx`: host-local OpenRouter runtime attribution adapter (port `11437`, `NEMOCLAW_OPENROUTER_RUNTIME_ADAPTER_PORT`) and native Deep Agents `openrouter` provider. - #6210 / #6292 -> `docs/about/release-notes.mdx`: host corporate proxy CA import into sandbox trust (`NEMOCLAW_CORPORATE_CA_BUNDLE`, `NEMOCLAW_CORPORATE_CA_IMPORT`). - #6624 / #6623 / #6656 -> `docs/about/release-notes.mdx`: release-matched base-image selection, surfaced cluster-image build diagnostics, preserved Nemotron profile registration. - #6629 / #6637 -> `docs/about/release-notes.mdx`: bare `connect` default-sandbox behavior and route-probe hardening. - #6634 / #6626 / #6596 / #5569 / #6610 / #6655 -> `docs/about/release-notes.mdx`: onboarding/recovery preservation, stale-gateway-PID fix, installer backup message, vLLM label on managed platforms. - #6578 / #5670 -> `docs/about/release-notes.mdx`: automatic Hermes light terminal skin and non-interactive `npx` MCP server startup. ## Verification `npm run docs`: 0 errors, all internal links resolve (2 pre-existing hidden-page warnings). `_build/` variants for OpenClaw, Hermes, and Deep Agents all regenerate with the v0.0.80 section. Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com> 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Documentation** * Added release notes for v0.0.80. * Documented Hermes upgrades, including Slack Block Kit rendering. * Added details on OpenRouter traffic routing and attribution headers. * Documented improved proxy certificate handling and sandbox reliability. * Highlighted enhanced connection defaults, route-probing safeguards, onboarding recovery, and terminal/MCP startup behavior. * Added references to relevant user-guide documentation. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
## Summary Fixes NVIDIA#5120. This PR hardens NemoClaw's bundled OpenClaw runtime for MCP stdio servers launched through `npx`. It adds a focused build-time compatibility patch for OpenClaw's MCP `StdioClientTransport` construction. When an MCP server command resolves to `npx`, `npx.cmd`, or a path-like equivalent, the patch automatically prepends `-y` unless the server args already include `-y` or `--yes`. This prevents `npx` from blocking the MCP stdio initialize handshake with an interactive install prompt while preserving existing MCP server configs. --- ## Why On first use or with a cold npm cache, `npx` may prompt for package-install confirmation before running the MCP server package. For MCP stdio transports, that prompt can block the same stdio channel OpenClaw expects to use for the JSON-RPC `initialize` handshake. As a result, the MCP server never replies to `initialize`, and OpenClaw eventually reports a startup timeout. The observed failure mode is: ```text MCP server connection timed out after 30000ms ``` Adding `-y` is the smallest targeted fix because it: * keeps existing user MCP configs valid * avoids increasing the global startup timeout * avoids preinstalling arbitrary MCP packages * avoids changing non-`npx` MCP servers * preserves user-provided args * directly removes the interactive `npx` prompt from the stdio startup path --- ## What changed ### MCP `npx` argument normalization Added `scripts/patch-openclaw-mcp-npx.js`. The patch normalizes MCP server args only when the command resolves to `npx`. Behavior: * `npx` gets `-y` prepended when missing * `npx.cmd` is supported for Windows-compatible command resolution * path-like commands ending in `npx` or `npx.cmd` are supported * existing `-y` is preserved * existing `--yes` is preserved * non-`npx` commands are left unchanged * args are not duplicated on repeated patch runs Example: ```json { "command": "npx", "args": [ "@modelcontextprotocol/server-filesystem", "/sandbox/.openclaw/workspace", "/tmp" ] } ``` is started as: ```text npx -y @modelcontextprotocol/server-filesystem /sandbox/.openclaw/workspace /tmp ``` --- ### Fail-closed patching The patch intentionally fails loudly if the expected bundled OpenClaw MCP transport shape is not found. It also validates that the timeout diagnostic site includes the server-name and command context needed for the improved error message. This avoids silently shipping an incomplete patch if the upstream bundled OpenClaw output changes. --- ### Improved timeout diagnostics MCP startup timeout diagnostics now include: * MCP server name * timeout duration in ms * redacted command context * an `npx`-specific hint when applicable The command context is redacted to avoid leaking sensitive runtime details while still making the failure actionable. --- ### Docker build integration The Dockerfile now runs the patch after the bundled OpenClaw files exist and before later image setup/use. The sandbox build-context staging was updated to include the patch script because the Dockerfile copies and executes it during image construction. --- ### Regression coverage Added `test/openclaw-mcp-npx-patch.test.ts`. The tests cover: * `npx` receives `-y` when missing * `npx.cmd` receives `-y` when missing * path-like `npx` commands are supported * existing `-y` is not duplicated * existing `--yes` is not duplicated * non-`npx` commands are unchanged * patch output remains idempotent * timeout diagnostics include the required context * redaction behavior is preserved * failure behavior is fail-closed * actual patched fixture output executes in a VM and verifies the rewritten behavior --- ## What this PR intentionally does not change This PR does not: * increase the MCP startup timeout * preinstall MCP packages globally * modify MCP server configuration format * change non-`npx` MCP server behavior * alter sandbox permissions or MCP tool authorization * make real npm/network calls in tests * patch unrelated OpenClaw runtime behavior --- ## Validation Passed: ```text npm test -- --run test/openclaw-mcp-npx-patch.test.ts npm run typecheck npm_config_script_shell=/bin/bash npm run build:cli node --check scripts/patch-openclaw-mcp-npx.js git diff --check ``` Also passed targeted staging assertions for the sandbox build-context/onboarding test changes. --- ## Notes A full Docker image build was not run in this local environment. The patch is intentionally fail-closed, so if the bundled OpenClaw MCP transport output changes shape, the Docker build should fail loudly instead of silently producing an incomplete runtime patch. Signed-off-by: Abhijeet Ranjan <abhijeet.r1907@gmail.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * MCP servers launched via npx now automatically start in non-interactive mode (`-y/--yes` injection). * Improved MCP timeout errors now include server/command context and redacted sensitive CLI arguments. * **Infrastructure** * Docker image builds now run an OpenClaw MCP stdio patch during build to normalize npx behavior. * Sandbox staging now includes the new MCP patch script. * **Tests** * Added a dedicated test suite covering npx detection/normalization, injected transport changes, idempotency, end-to-end CLI behavior, and failure when expected targets are missing. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Abhijeet Ranjan <abhijeet.r1907@gmail.com> Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com> Co-authored-by: Prekshi Vyas <prekshiv@nvidia.com>
## Summary Release-prep documentation for **v0.0.80**. Adds the `## v0.0.80` section to `docs/about/release-notes.mdx` summarizing user-facing changes since v0.0.79, each bullet linking to the relevant deeper page. Produced via `nemoclaw-contributor-update-docs` (pre-tag path): scanned `v0.0.79..HEAD`, applied the docs skip list (no violations), and confirmed the 8 commits that already shipped in-PR docs are complete. No new pages needed. ## Source summary - NVIDIA#6507 -> `docs/about/release-notes.mdx`: Hermes v0.18 + Slack Block Kit (rich rendering, digest-pinned base image). - NVIDIA#6584 / NVIDIA#6616 -> `docs/about/release-notes.mdx`: host-local OpenRouter runtime attribution adapter (port `11437`, `NEMOCLAW_OPENROUTER_RUNTIME_ADAPTER_PORT`) and native Deep Agents `openrouter` provider. - NVIDIA#6210 / NVIDIA#6292 -> `docs/about/release-notes.mdx`: host corporate proxy CA import into sandbox trust (`NEMOCLAW_CORPORATE_CA_BUNDLE`, `NEMOCLAW_CORPORATE_CA_IMPORT`). - NVIDIA#6624 / NVIDIA#6623 / NVIDIA#6656 -> `docs/about/release-notes.mdx`: release-matched base-image selection, surfaced cluster-image build diagnostics, preserved Nemotron profile registration. - NVIDIA#6629 / NVIDIA#6637 -> `docs/about/release-notes.mdx`: bare `connect` default-sandbox behavior and route-probe hardening. - NVIDIA#6634 / NVIDIA#6626 / NVIDIA#6596 / NVIDIA#5569 / NVIDIA#6610 / NVIDIA#6655 -> `docs/about/release-notes.mdx`: onboarding/recovery preservation, stale-gateway-PID fix, installer backup message, vLLM label on managed platforms. - NVIDIA#6578 / NVIDIA#5670 -> `docs/about/release-notes.mdx`: automatic Hermes light terminal skin and non-interactive `npx` MCP server startup. ## Verification `npm run docs`: 0 errors, all internal links resolve (2 pre-existing hidden-page warnings). `_build/` variants for OpenClaw, Hermes, and Deep Agents all regenerate with the v0.0.80 section. Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com> 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Documentation** * Added release notes for v0.0.80. * Documented Hermes upgrades, including Slack Block Kit rendering. * Added details on OpenRouter traffic routing and attribution headers. * Documented improved proxy certificate handling and sandbox reliability. * Highlighted enhanced connection defaults, route-probing safeguards, onboarding recovery, and terminal/MCP startup behavior. * Added references to relevant user-guide documentation. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
4 participants
Summary
Fixes #5120.
This PR hardens NemoClaw's bundled OpenClaw runtime for MCP stdio servers launched through
npx.It adds a focused build-time compatibility patch for OpenClaw's MCP
StdioClientTransportconstruction. When an MCP server command resolves tonpx,npx.cmd, or a path-like equivalent, the patch automatically prepends-yunless the server args already include-yor--yes.This prevents
npxfrom blocking the MCP stdio initialize handshake with an interactive install prompt while preserving existing MCP server configs.Why
On first use or with a cold npm cache,
npxmay prompt for package-install confirmation before running the MCP server package.For MCP stdio transports, that prompt can block the same stdio channel OpenClaw expects to use for the JSON-RPC
initializehandshake. As a result, the MCP server never replies toinitialize, and OpenClaw eventually reports a startup timeout.The observed failure mode is:
Adding
-yis the smallest targeted fix because it:npxMCP serversnpxprompt from the stdio startup pathWhat changed
MCP
npxargument normalizationAdded
scripts/patch-openclaw-mcp-npx.js.The patch normalizes MCP server args only when the command resolves to
npx.Behavior:
npxgets-yprepended when missingnpx.cmdis supported for Windows-compatible command resolutionnpxornpx.cmdare supported-yis preserved--yesis preservednpxcommands are left unchangedExample:
{ "command": "npx", "args": [ "@modelcontextprotocol/server-filesystem", "/sandbox/.openclaw/workspace", "/tmp" ] }is started as:
Fail-closed patching
The patch intentionally fails loudly if the expected bundled OpenClaw MCP transport shape is not found.
It also validates that the timeout diagnostic site includes the server-name and command context needed for the improved error message.
This avoids silently shipping an incomplete patch if the upstream bundled OpenClaw output changes.
Improved timeout diagnostics
MCP startup timeout diagnostics now include:
npx-specific hint when applicableThe command context is redacted to avoid leaking sensitive runtime details while still making the failure actionable.
Docker build integration
The Dockerfile now runs the patch after the bundled OpenClaw files exist and before later image setup/use.
The sandbox build-context staging was updated to include the patch script because the Dockerfile copies and executes it during image construction.
Regression coverage
Added
test/openclaw-mcp-npx-patch.test.ts.The tests cover:
npxreceives-ywhen missingnpx.cmdreceives-ywhen missingnpxcommands are supported-yis not duplicated--yesis not duplicatednpxcommands are unchangedWhat this PR intentionally does not change
This PR does not:
npxMCP server behaviorValidation
Passed:
Also passed targeted staging assertions for the sandbox build-context/onboarding test changes.
Notes
A full Docker image build was not run in this local environment. The patch is intentionally fail-closed, so if the bundled OpenClaw MCP transport output changes shape, the Docker build should fail loudly instead of silently producing an incomplete runtime patch.
Signed-off-by: Abhijeet Ranjan abhijeet.r1907@gmail.com
Summary by CodeRabbit
New Features
-y/--yesinjection).Infrastructure
Tests