feat(cli): add MCP bridge for host-to-sandbox MCP server bridging#565
Open
charlesverdad wants to merge 9 commits intoNVIDIA:mainfrom
Open
feat(cli): add MCP bridge for host-to-sandbox MCP server bridging#565charlesverdad wants to merge 9 commits intoNVIDIA:mainfrom
charlesverdad wants to merge 9 commits intoNVIDIA:mainfrom
Conversation
Adds documentation for bridging stdio-based MCP servers from the host into a NemoClaw sandbox via HTTP proxying and OpenShell port forwarding. API keys stay on the host and never enter the sandbox. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
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:
📝 WalkthroughWalkthroughAdds MCP-bridge tooling and docs to run host stdio-based MCP servers via a host-side stdio→HTTP proxy, forward the proxy port into a NemoClaw sandbox with OpenShell, and register the forwarded endpoint with sandbox-side Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as Host CLI (nemoclaw mcp)
participant Proxy as Host Proxy (mcp-proxy)
participant MCP as MCP Subprocess
participant OpenShell as OpenShell
participant Sandbox as NemoClaw Sandbox
participant Mcporter as mcporter (in sandbox)
CLI->>Proxy: spawn proxy (--command, --env, --port)
Proxy->>MCP: spawn subprocess (stdio)
MCP-->>Proxy: emit/receive JSON-lines
Proxy-->>Sandbox: listen on 127.0.0.1:PORT (HTTP)
CLI->>OpenShell: openshell forward host:PORT -> sandbox:PORT
Sandbox->>Mcporter: connect to http://localhost:PORT
Mcporter-->>Sandbox: config add / verify
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 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.
Actionable comments posted: 2
🧹 Nitpick comments (6)
docs/deployment/set-up-mcp-bridge.md (6)
141-145: Rename "Related Topics" to "Next Steps".The page structure guidelines require a "Next Steps" section at the bottom that links to related pages. This section is titled "Related Topics" instead.
As per coding guidelines: "A 'Next Steps' section at the bottom links to related pages."
📝 Proposed fix
-## Related Topics +## Next Steps🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/deployment/set-up-mcp-bridge.md` around lines 141 - 145, Rename the bottom heading "Related Topics" to "Next Steps" in the document by replacing the heading text "Related Topics" with "Next Steps" and keep the existing bullet links intact (the list items "- [Set Up Telegram Bridge]...", "- [Commands]...", "- [Network Policies]...") so the section complies with the page structure guideline requiring a "Next Steps" section.
67-67: Split sentences onto separate lines.This line contains three sentences. The formatting rules require one sentence per line to make diffs more readable.
As per coding guidelines: "One sentence per line in source (makes diffs readable). Flag paragraphs where multiple sentences appear on the same line."
📝 Proposed fix
-Export the API key on the host. The bridge reads the variable from the host environment and passes it to the MCP server process. The key never enters the sandbox. +Export the API key on the host. +The bridge reads the variable from the host environment and passes it to the MCP server process. +The key never enters the sandbox.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/deployment/set-up-mcp-bridge.md` at line 67, The sentence "Export the API key on the host. The bridge reads the variable from the host environment and passes it to the MCP server process. The key never enters the sandbox." should be split so each sentence is on its own line; update the paragraph so it reads as three separate lines (one sentence per line) to follow the one-sentence-per-line guideline and improve diff readability.
61-61: Split sentences onto separate lines.This line contains two sentences. The formatting rules require one sentence per line to make diffs more readable.
As per coding guidelines: "One sentence per line in source (makes diffs readable). Flag paragraphs where multiple sentences appear on the same line."
📝 Proposed fix
-This persists across sandbox restarts. It is only lost if the sandbox is destroyed and recreated. +This persists across sandbox restarts. +It is only lost if the sandbox is destroyed and recreated.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/deployment/set-up-mcp-bridge.md` at line 61, The line "This persists across sandbox restarts. It is only lost if the sandbox is destroyed and recreated." contains two sentences on a single line; split them into two separate lines so each sentence sits on its own line (locate the sentence in docs/deployment/set-up-mcp-bridge.md and replace the single-line instance of "This persists across sandbox restarts. It is only lost if the sandbox is destroyed and recreated." with two lines: one for "This persists across sandbox restarts." and one for "It is only lost if the sandbox is destroyed and recreated.").
75-75: Split sentences onto separate lines.This line contains two sentences before the colon. While the colon correctly introduces the code block, the sentences should still be on separate lines for diff readability.
As per coding guidelines: "One sentence per line in source (makes diffs readable). Flag paragraphs where multiple sentences appear on the same line."
📝 Proposed fix
-Start the stdio→HTTP proxy on the host. The proxy spawns the MCP server as a subprocess and exposes it over HTTP on a local port: +Start the stdio→HTTP proxy on the host. +The proxy spawns the MCP server as a subprocess and exposes it over HTTP on a local port:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/deployment/set-up-mcp-bridge.md` at line 75, The sentence on the same line before the colon contains two sentences; split them into separate lines so each sentence is on its own line for diff readability—edit the markdown line that currently reads "Start the stdio→HTTP proxy on the host. The proxy spawns the MCP server as a subprocess and exposes it over HTTP on a local port:" so that "Start the stdio→HTTP proxy on the host." and "The proxy spawns the MCP server as a subprocess and exposes it over HTTP on a local port:" are each on their own lines.
130-130: Split sentences onto separate lines.This line contains three sentences. The formatting rules require one sentence per line to make diffs more readable.
As per coding guidelines: "One sentence per line in source (makes diffs readable). Flag paragraphs where multiple sentences appear on the same line."
📝 Proposed fix
-The proxy processes do not survive a host reboot. Restart them and the port forwards. The mcporter configuration inside the sandbox persists and does not need to be reconfigured. +The proxy processes do not survive a host reboot. +Restart them and the port forwards. +The mcporter configuration inside the sandbox persists and does not need to be reconfigured.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/deployment/set-up-mcp-bridge.md` at line 130, The single line contains three sentences and must be split so each sentence is on its own line; replace the current line "The proxy processes do not survive a host reboot. Restart them and the port forwards. The mcporter configuration inside the sandbox persists and does not need to be reconfigured." with three separate lines, one per sentence, preserving exact wording and punctuation to satisfy the "one sentence per line" guideline.
107-107: Split sentences onto separate lines.This line contains two sentences. The formatting rules require one sentence per line to make diffs more readable.
As per coding guidelines: "One sentence per line in source (makes diffs readable). Flag paragraphs where multiple sentences appear on the same line."
📝 Proposed fix
-If the tool list is returned, the bridge is working. The OpenClaw agent can now use the MCP tools through the `mcporter` skill. +If the tool list is returned, the bridge is working. +The OpenClaw agent can now use the MCP tools through the `mcporter` skill.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/deployment/set-up-mcp-bridge.md` at line 107, The line containing "If the tool list is returned, the bridge is working. The OpenClaw agent can now use the MCP tools through the `mcporter` skill." should be split into two separate lines so each sentence stands alone; replace the single-line paragraph with one line containing "If the tool list is returned, the bridge is working." and a second line containing "The OpenClaw agent can now use the MCP tools through the `mcporter` skill." preserving punctuation and markdown backticks.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/deployment/set-up-mcp-bridge.md`:
- Line 21: The H1 heading in the Markdown file does not match the frontmatter
title.page; update the top-level heading (the H1 line) to exactly match the
frontmatter value "Bridge MCP Servers into a NemoClaw Sandbox" so the H1 and
title.page are identical; locate the existing H1 "Bridge MCP Servers into a
Sandbox" and replace it with the full string from the frontmatter.
- Around line 31-41: The fenced ASCII diagram block lacks a language tag; change
the opening fence from ``` to ```text for the diagram in
docs/deployment/set-up-mcp-bridge.md so the block becomes a ```text fenced code
block (leave the diagram contents unchanged and keep the matching closing fence)
to satisfy Markdown linting.
---
Nitpick comments:
In `@docs/deployment/set-up-mcp-bridge.md`:
- Around line 141-145: Rename the bottom heading "Related Topics" to "Next
Steps" in the document by replacing the heading text "Related Topics" with "Next
Steps" and keep the existing bullet links intact (the list items "- [Set Up
Telegram Bridge]...", "- [Commands]...", "- [Network Policies]...") so the
section complies with the page structure guideline requiring a "Next Steps"
section.
- Line 67: The sentence "Export the API key on the host. The bridge reads the
variable from the host environment and passes it to the MCP server process. The
key never enters the sandbox." should be split so each sentence is on its own
line; update the paragraph so it reads as three separate lines (one sentence per
line) to follow the one-sentence-per-line guideline and improve diff
readability.
- Line 61: The line "This persists across sandbox restarts. It is only lost if
the sandbox is destroyed and recreated." contains two sentences on a single
line; split them into two separate lines so each sentence sits on its own line
(locate the sentence in docs/deployment/set-up-mcp-bridge.md and replace the
single-line instance of "This persists across sandbox restarts. It is only lost
if the sandbox is destroyed and recreated." with two lines: one for "This
persists across sandbox restarts." and one for "It is only lost if the sandbox
is destroyed and recreated.").
- Line 75: The sentence on the same line before the colon contains two
sentences; split them into separate lines so each sentence is on its own line
for diff readability—edit the markdown line that currently reads "Start the
stdio→HTTP proxy on the host. The proxy spawns the MCP server as a subprocess
and exposes it over HTTP on a local port:" so that "Start the stdio→HTTP proxy
on the host." and "The proxy spawns the MCP server as a subprocess and exposes
it over HTTP on a local port:" are each on their own lines.
- Line 130: The single line contains three sentences and must be split so each
sentence is on its own line; replace the current line "The proxy processes do
not survive a host reboot. Restart them and the port forwards. The mcporter
configuration inside the sandbox persists and does not need to be reconfigured."
with three separate lines, one per sentence, preserving exact wording and
punctuation to satisfy the "one sentence per line" guideline.
- Line 107: The line containing "If the tool list is returned, the bridge is
working. The OpenClaw agent can now use the MCP tools through the `mcporter`
skill." should be split into two separate lines so each sentence stands alone;
replace the single-line paragraph with one line containing "If the tool list is
returned, the bridge is working." and a second line containing "The OpenClaw
agent can now use the MCP tools through the `mcporter` skill." preserving
punctuation and markdown backticks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 588f9b1b-327d-4b95-b880-9652e52ee70c
📒 Files selected for processing (1)
docs/deployment/set-up-mcp-bridge.md
- Match H1 heading to frontmatter title.page value - Add language tag to fenced code block diagram Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Shows the proposed nemoclaw mcp add/import/list/remove/restart subcommands that would automate the manual bridge setup steps. Clearly marked as proposed/not yet implemented. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
docs/deployment/set-up-mcp-bridge.md (4)
29-29: Remove marketing adjective "lightweight".Line 29 uses "lightweight HTTP proxy", which reads as promotional rather than technical.
As per coding guidelines, "Superlatives and marketing language ("powerful," "robust," "seamless," "cutting-edge"). Say what it does, not how great it is."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/deployment/set-up-mcp-bridge.md` at line 29, Remove the marketing adjective "lightweight" from the sentence describing the MCP bridge; update the phrase "lightweight HTTP proxy" to a neutral technical description like "an HTTP proxy" when editing the line that explains how the MCP bridge runs a proxy on the host that wraps the stdio MCP server and is forwarded into the sandbox via OpenShell for mcporter to connect to.
29-29: Format CLI command names in prose as inline code.
mcporterappears in prose without inline code formatting (for example, Line 29, Line 92, Line 107).
As per coding guidelines, "CLI commands, file paths, flags, parameter names, and values must use inlinecodeformatting."Also applies to: 92-92, 107-107
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/deployment/set-up-mcp-bridge.md` at line 29, Summary: CLI command names in prose (notably "mcporter") must use inline code formatting. Fix: edit docs/deployment/set-up-mcp-bridge.md and replace plain-text occurrences of mcporter with inline code formatting (and do the same for any other CLI commands, file paths, flags, parameter names, or values in the same file); specifically update the prose instances mentioned (around the MCP bridge explanation and the other noted occurrences) so they use inline `code` formatting consistently per the style guide.
61-61: Split multi-sentence lines to one sentence per source line.Line 61, Line 67, Line 75, Line 107, and Line 130 each contain multiple sentences on a single source line.
As per coding guidelines, "One sentence per line in source (makes diffs readable)."Also applies to: 67-67, 75-75, 107-107, 130-130
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/deployment/set-up-mcp-bridge.md` at line 61, Split the multi-sentence lines so each sentence appears on its own source line: break the text at lines containing multiple sentences (the sentence that currently reads "This persists across sandbox restarts. It is only lost if the sandbox is destroyed and recreated." and the similar multi-sentence entries at the other occurrences) into separate lines — specifically update the sentences found on the lines flagged (61, 67, 75, 107, 130) so each sentence is on its own line to follow the "one sentence per line" guideline.
43-45: Add an introductory sentence after each H2 before lists/subsections.Line 43, Line 63, Line 109, Line 132, and Line 141 start sections without a short intro sentence.
As per coding guidelines, "Sections use H2 and H3, each starting with an introductory sentence."Also applies to: 63-65, 109-113, 132-134, 141-143
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/deployment/set-up-mcp-bridge.md` around lines 43 - 45, Add a brief introductory sentence immediately after each H2 heading before any lists or subsections (for example after the "Prerequisites" H2 and the other H2s referenced in the comment) describing the purpose or context of that section; update the markdown so each H2 is followed by one concise explanatory sentence (1–2 lines) that leads into the subsequent list or H3 subsections to satisfy the "Sections use H2 and H3, each starting with an introductory sentence" guideline.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/deployment/set-up-mcp-bridge.md`:
- Around line 141-145: Replace the bottom "Related Topics" section header and
its bullet list with a "Next Steps" section: change the header text "Related
Topics" to "Next Steps" and keep the existing link targets (the Telegram Bridge,
Commands, and Network Policies links) but rephrase link context if needed to fit
"Next Steps" wording; update the heading string and the three list items (the
lines containing "[Set Up Telegram Bridge]", "[Commands]", and "[Network
Policies]") so the section reads as "Next Steps" linking to those same pages.
---
Nitpick comments:
In `@docs/deployment/set-up-mcp-bridge.md`:
- Line 29: Remove the marketing adjective "lightweight" from the sentence
describing the MCP bridge; update the phrase "lightweight HTTP proxy" to a
neutral technical description like "an HTTP proxy" when editing the line that
explains how the MCP bridge runs a proxy on the host that wraps the stdio MCP
server and is forwarded into the sandbox via OpenShell for mcporter to connect
to.
- Line 29: Summary: CLI command names in prose (notably "mcporter") must use
inline code formatting. Fix: edit docs/deployment/set-up-mcp-bridge.md and
replace plain-text occurrences of mcporter with inline code formatting (and do
the same for any other CLI commands, file paths, flags, parameter names, or
values in the same file); specifically update the prose instances mentioned
(around the MCP bridge explanation and the other noted occurrences) so they use
inline `code` formatting consistently per the style guide.
- Line 61: Split the multi-sentence lines so each sentence appears on its own
source line: break the text at lines containing multiple sentences (the sentence
that currently reads "This persists across sandbox restarts. It is only lost if
the sandbox is destroyed and recreated." and the similar multi-sentence entries
at the other occurrences) into separate lines — specifically update the
sentences found on the lines flagged (61, 67, 75, 107, 130) so each sentence is
on its own line to follow the "one sentence per line" guideline.
- Around line 43-45: Add a brief introductory sentence immediately after each H2
heading before any lists or subsections (for example after the "Prerequisites"
H2 and the other H2s referenced in the comment) describing the purpose or
context of that section; update the markdown so each H2 is followed by one
concise explanatory sentence (1–2 lines) that leads into the subsequent list or
H3 subsections to satisfy the "Sections use H2 and H3, each starting with an
introductory sentence" guideline.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 22033421-c9be-48f5-8380-659846523506
📒 Files selected for processing (1)
docs/deployment/set-up-mcp-bridge.md
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
docs/deployment/set-up-mcp-bridge.md (1)
230-234:⚠️ Potential issue | 🟡 MinorRename the final section to
Next Steps.The page still ends with
Related Topicsinstead ofNext Steps.As per coding guidelines: "A "Next Steps" section at the bottom links to related pages."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/deployment/set-up-mcp-bridge.md` around lines 230 - 234, The "Related Topics" section header should be renamed to "Next Steps" to follow the documentation guideline; update the header text "Related Topics" in the markdown (the final section heading that currently precedes the list of links including "Set Up Telegram Bridge", "Commands", and "Network Policies") to read "Next Steps" while leaving the link items unchanged so the section still points to the same related pages.
🧹 Nitpick comments (4)
docs/deployment/set-up-mcp-bridge.md (4)
134-138: Use a MyST admonition instead of a blockquote note.Line 134 uses a blockquote + bold note pattern, but this page should use a MyST admonition (
:::{note}/:::{warning}) for callouts.As per coding guidelines: "Use MyST admonitions (:::{tip}, :::{note}, :::{warning}) for callouts, not bold text or blockquotes."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/deployment/set-up-mcp-bridge.md` around lines 134 - 138, Replace the blockquote + bold "Note" pattern that precedes the paragraph starting with "The following `nemoclaw mcp` subcommands are proposed but not yet implemented." with a MyST admonition: wrap that exact paragraph (preserve inline code and text) inside a :::{note} ... ::: block, ensuring there is a blank line before and after the admonition; if the content is a warning instead, use :::{warning} similarly. Keep the original wording and formatting (inline backticks) but swap the blockquote/bold markers for the MyST admonition delimiters.
43-48: Add introductory sentences at the start of these sections.Several H2/H3 sections begin directly with bullets, subheadings, or code blocks instead of an opening sentence (for example around Lines 43, 63, 109, and multiple
## Proposed CLIsubsections).As per coding guidelines: "Sections use H2 and H3, each starting with an introductory sentence."
Also applies to: 63-64, 109-111, 140-148, 156-160, 164-168, 172-176, 186-190, 194-198
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/deployment/set-up-mcp-bridge.md` around lines 43 - 48, Several headings start directly with bullets or code; add a concise introductory sentence to each H2/H3 that currently begins with a list or code block so the section has a one-line lead-in. Update the "## Prerequisites" heading and every "## Proposed CLI" and other H2/H3 mentioned (around lines referenced in the review: the sections beginning at ~43, ~63, ~109, and the groups around 140–198) by inserting a short descriptive sentence that summarizes the section’s purpose before the bullets or code blocks, ensuring each section begins with an introductory sentence as per the documentation guidelines.
149-155: Prefer present tense in descriptive prose.The phrasing around Lines 149-155 and Line 204 uses future tense (
would) for behavior descriptions. Rephrase in present tense to match docs voice rules.As per coding guidelines: "Present tense. Flag future tense ("will") in descriptions of current behavior."
Also applies to: 204-204
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/deployment/set-up-mcp-bridge.md` around lines 149 - 155, Replace the future-tense phrasing introduced by the header "This would:" and the numbered list that follows so the document reads in present tense; specifically change "This would:" to "This:" (or "This proxy:"), and rewrite each list item (e.g., "Start the stdio→HTTP proxy on the host with the named environment variables.", "Forward the port into the sandbox via `openshell forward`.", "Install mcporter in the sandbox if not already present.", "Register the server in the sandbox mcporter configuration.") to present-tense declarative statements, and make the same present-tense edit for the other occurrence referenced around line 204.
27-29: Use one sentence per source line consistently.Multiple lines contain more than one sentence on the same source line (for example Lines 27, 29, 61, 67, 130, 170, and 200).
As per coding guidelines: "One sentence per line in source (makes diffs readable)."
Also applies to: 61-61, 67-67, 130-130, 170-170, 200-200
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/deployment/set-up-mcp-bridge.md` around lines 27 - 29, Split any source lines that contain multiple sentences so each line has exactly one sentence; specifically edit the paragraphs containing "MCP servers typically run as stdio subprocesses..." and "The MCP bridge solves this by running a lightweight HTTP proxy..." as well as the other flagged lines (61, 67, 130, 170, 200) so that each sentence appears on its own line, preserving existing wording and punctuation and keeping the document's meaning unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/deployment/set-up-mcp-bridge.md`:
- Around line 6-8: Update the frontmatter arrays so product names use correct
casing: replace occurrences of "nemoclaw" with "NemoClaw" and "openclaw" /
"openshell" with "OpenClaw" / "OpenShell" inside the keywords and tags arrays
(the symbols to edit are the keywords: [...] and tags: [...] entries shown in
the diff); keep the rest of each string intact (e.g., "NemoClaw mcp bridge" if
part of a keyword) and ensure all three names consistently use the exact casing
NemoClaw, OpenClaw, and OpenShell.
---
Duplicate comments:
In `@docs/deployment/set-up-mcp-bridge.md`:
- Around line 230-234: The "Related Topics" section header should be renamed to
"Next Steps" to follow the documentation guideline; update the header text
"Related Topics" in the markdown (the final section heading that currently
precedes the list of links including "Set Up Telegram Bridge", "Commands", and
"Network Policies") to read "Next Steps" while leaving the link items unchanged
so the section still points to the same related pages.
---
Nitpick comments:
In `@docs/deployment/set-up-mcp-bridge.md`:
- Around line 134-138: Replace the blockquote + bold "Note" pattern that
precedes the paragraph starting with "The following `nemoclaw mcp` subcommands
are proposed but not yet implemented." with a MyST admonition: wrap that exact
paragraph (preserve inline code and text) inside a :::{note} ... ::: block,
ensuring there is a blank line before and after the admonition; if the content
is a warning instead, use :::{warning} similarly. Keep the original wording and
formatting (inline backticks) but swap the blockquote/bold markers for the MyST
admonition delimiters.
- Around line 43-48: Several headings start directly with bullets or code; add a
concise introductory sentence to each H2/H3 that currently begins with a list or
code block so the section has a one-line lead-in. Update the "## Prerequisites"
heading and every "## Proposed CLI" and other H2/H3 mentioned (around lines
referenced in the review: the sections beginning at ~43, ~63, ~109, and the
groups around 140–198) by inserting a short descriptive sentence that summarizes
the section’s purpose before the bullets or code blocks, ensuring each section
begins with an introductory sentence as per the documentation guidelines.
- Around line 149-155: Replace the future-tense phrasing introduced by the
header "This would:" and the numbered list that follows so the document reads in
present tense; specifically change "This would:" to "This:" (or "This proxy:"),
and rewrite each list item (e.g., "Start the stdio→HTTP proxy on the host with
the named environment variables.", "Forward the port into the sandbox via
`openshell forward`.", "Install mcporter in the sandbox if not already
present.", "Register the server in the sandbox mcporter configuration.") to
present-tense declarative statements, and make the same present-tense edit for
the other occurrence referenced around line 204.
- Around line 27-29: Split any source lines that contain multiple sentences so
each line has exactly one sentence; specifically edit the paragraphs containing
"MCP servers typically run as stdio subprocesses..." and "The MCP bridge solves
this by running a lightweight HTTP proxy..." as well as the other flagged lines
(61, 67, 130, 170, 200) so that each sentence appears on its own line,
preserving existing wording and punctuation and keeping the document's meaning
unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d54855c8-3f76-4ce2-b181-daf6748e0778
📒 Files selected for processing (1)
docs/deployment/set-up-mcp-bridge.md
charlesverdad
changed the title
charlesverdad
force-pushed
the
feat/mcp-bridge-design
branch
from
37c791c to
ae89ea5
Compare
Adds `nemoclaw <sandbox> mcp add|remove|list|restart` subcommands that bridge stdio-based MCP servers from the host into the sandbox via HTTP proxying and OpenShell port forwarding. API keys stay on the host and never enter the sandbox. Implementation: - scripts/mcp-proxy.js: stdio-to-HTTP proxy that spawns an MCP server subprocess and exposes it over JSON-RPC HTTP on 127.0.0.1 - bin/lib/mcp-bridge.js: proxy lifecycle, port management, mcporter bootstrap, and sandbox mcporter config registration - bin/nemoclaw.js: wire mcp as a sandbox-scoped action with sub-dispatch Closes NVIDIA#566 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
charlesverdad
force-pushed
the
feat/mcp-bridge-design
branch
from
ae89ea5 to
75d27c2
Compare
- Use correct product name casing in frontmatter (NemoClaw, OpenClaw, OpenShell) - Rename "Related Topics" to "Next Steps" per docs style guide Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (2)
docs/deployment/set-up-mcp-bridge.md (2)
27-31: Rewrite these sentences in active voice.This page still has passive constructions like “spawned by tools”, “If the tool list is returned”, and “does not need to be rewritten”. As per coding guidelines, "Active voice required. Flag passive constructions."
Also applies to: 114-116, 211-212
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/deployment/set-up-mcp-bridge.md` around lines 27 - 31, Rewrite the passive sentences into active voice: make the tools the subject (e.g., change “spawned by tools like Claude Code or Cursor” to “tools like Claude Code or Cursor spawn them”), change “The proxy port is forwarded into the sandbox via OpenShell, where mcporter connects to it” to an active form such as “OpenShell forwards the proxy port into the sandbox, and mcporter connects to it,” and similarly convert the passive phrases referenced at lines 114–116 and 211–212 so the actor is explicit (e.g., “If the tool list is returned” → “If the server returns the tool list”, “does not need to be rewritten” → “you do not need to rewrite it”).
214-214: Format the command in this heading as inline code.
nemoclaw startis a CLI command, so leavingnemoclawas lowercase prose here violates both the command-formatting rule and the product-casing rule. As per coding guidelines, "CLI commands, file paths, flags, parameter names, and values must use inlinecodeformatting" and "NemoClaw, OpenClaw, and OpenShell must use correct casing."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/deployment/set-up-mcp-bridge.md` at line 214, Update the heading "Integration with nemoclaw start" to format the CLI as inline code and correct product casing: change it to something like "Integration with NemoClaw `nemoclaw start`" so the product name uses the correct casing (NemoClaw) and the CLI command is wrapped in inline code (`nemoclaw start`).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@bin/lib/mcp-bridge.js`:
- Around line 100-111: The add function currently accepts the sandbox name
verbatim which later flows into PID/log file paths and into shell invocations
(e.g., the mcporter config add/remove calls around where name is used); validate
and sanitize the name early: trim whitespace, reject empty or names containing
whitespace or shell metacharacters by enforcing a strict allowlist (for example
only letters, digits, dot, underscore, hyphen), and return a clear error when
validation fails; additionally, where name is interpolated into any shell
command, ensure you use a safe escaping mechanism or avoid direct shell
interpolation by passing the name as an argument to controlled APIs.
- Around line 40-52: Fix port allocation by making getUsedPorts and
nextAvailablePort scan the global registry rather than a single sandbox: change
getUsedPorts to collect ports from all registry entries' mcp objects (across
sandboxes) and have nextAvailablePort iterate MCP_PORT_START..MCP_PORT_END
against that global set. In addition, in the add(...) code path that accepts
requestedPort, validate the requested port against the same global used set and
also verify the port is free on the host (e.g., attempt to bind or use an
isPortFree check) before accepting; if taken, reject/return an error or pick the
nextAvailablePort. Update any other occurrences (lines ~135-140) that call
nextAvailablePort or assume per-sandbox uniqueness to use the global check.
- Around line 56-69: The add/remove flows leave host state inconsistent when
sandbox registration or cleanup fails; update add() to perform sandbox-side
registration (ensureMcporter() and the mcporter "config add" call) before
starting the proxy/port-forward and do not mask errors with "|| true", and if
registration fails ensure you rollback any started proxy/forwards and delete
temporary files/registry entries; update remove() to run the sandbox cleanup
first and only mutate the local registry or stop host processes after cleanup
succeeds, and if cleanup fails ensure you stop any host-side forwarding/proxy
and preserve consistent state; reference the functions add(), remove(),
ensureMcporter(), and the mcporter config add/remove calls when making these
changes.
In `@docs/deployment/set-up-mcp-bridge.md`:
- Around line 45-49: Add a short lead-in sentence after each H2/H3 heading that
currently jumps straight into a list or code block (e.g., "Prerequisites", "Add
an MCP Server", "Manage the Bridge", "Security", and each "Proposed CLI"
subsection) to frame the content that follows; edit the markdown in
docs/deployment/set-up-mcp-bridge.md to insert a one-sentence introductory line
immediately below each heading (also update the other occurrences noted around
lines 67-69, 117-119, 149-209, 233-245) so every section begins with a brief
explanatory sentence before lists or code blocks.
- Around line 142-212: The docs currently state MCP subcommands are "not yet
implemented" while the code (bin/nemoclaw.js — see parseMcpArgs() and the mcp
routing at the sections handling lines around add|remove|list|restart, and
bin/lib/mcp-bridge.js) already implements add/remove/list/restart; update the
"Proposed CLI" section to clearly separate which MCP commands are shipped vs
future work: mark add/remove/list/restart as implemented (and note that
parseMcpArgs() only accepts --name, --command, --env, --port so --url and mcp
import are unsupported), and move or relabel the unsupported examples (--url,
import from claude/cursor/windsurf) into a "Planned" or "Future" subsection so
readers aren’t misled.
In `@scripts/mcp-proxy.js`:
- Around line 68-74: The current code creates childEnv by cloning process.env
and passes it to spawn (childEnv, spawn, cmd, cmdArgs), which leaks all host
environment variables; instead build childEnv only from the explicit allowed
list (the variables parsed from the --env flag or whatever whitelist is used),
copying each approved key from process.env if present, and do not spread
process.env. Replace the spread assignment const childEnv = { ...process.env }
with logic that initializes childEnv = {} and iterates over the approvedEnvNames
array (or the parsed --env values), adding childEnv[name] = process.env[name]
when defined, then pass that childEnv into spawn(cmd, cmdArgs, { env: childEnv,
stdio: [...] }). Ensure no other code paths later reassign childEnv to include
the full environment.
- Around line 156-170: The request handler created in server currently sets
Access-Control-Allow-Origin to "*" which enables DNS rebinding; update the
handler (inside the async (req, res) => { ... } block) to read
req.headers.origin and validate it against a whitelist containing allowed
origins (e.g., "http://localhost:PORT" and "http://127.0.0.1:PORT"); if the
Origin is missing or not on the whitelist, respond with 403 and do not set
Access-Control-Allow-Origin, otherwise set Access-Control-Allow-Origin to that
specific origin (not "*") and continue handling OPTIONS/POST as before; ensure
the preflight OPTIONS handling also respects this validation.
---
Nitpick comments:
In `@docs/deployment/set-up-mcp-bridge.md`:
- Around line 27-31: Rewrite the passive sentences into active voice: make the
tools the subject (e.g., change “spawned by tools like Claude Code or Cursor” to
“tools like Claude Code or Cursor spawn them”), change “The proxy port is
forwarded into the sandbox via OpenShell, where mcporter connects to it” to an
active form such as “OpenShell forwards the proxy port into the sandbox, and
mcporter connects to it,” and similarly convert the passive phrases referenced
at lines 114–116 and 211–212 so the actor is explicit (e.g., “If the tool list
is returned” → “If the server returns the tool list”, “does not need to be
rewritten” → “you do not need to rewrite it”).
- Line 214: Update the heading "Integration with nemoclaw start" to format the
CLI as inline code and correct product casing: change it to something like
"Integration with NemoClaw `nemoclaw start`" so the product name uses the
correct casing (NemoClaw) and the CLI command is wrapped in inline code
(`nemoclaw start`).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8d856f09-7b9c-4b48-b24f-cff2161cd7b6
📒 Files selected for processing (4)
bin/lib/mcp-bridge.jsbin/nemoclaw.jsdocs/deployment/set-up-mcp-bridge.mdscripts/mcp-proxy.js
Comment on lines
+56
to
+69
| function sshExec(sandboxName, command) { | ||
| const openshell = resolveOpenshell(); | ||
| if (!openshell) { | ||
| console.error(" openshell not found."); | ||
| process.exit(1); | ||
| } | ||
| const sshConfig = runCapture(`"${openshell}" sandbox ssh-config "${sandboxName}"`); | ||
| const confPath = `/tmp/nemoclaw-mcp-ssh-${sandboxName}.conf`; | ||
| fs.writeFileSync(confPath, sshConfig); | ||
| try { | ||
| return runCapture( | ||
| `ssh -T -F "${confPath}" -o ConnectTimeout=10 "openshell-${sandboxName}" '${command.replace(/'/g, "'\\''")}'`, | ||
| { ignoreError: true } | ||
| ); |
There was a problem hiding this comment.
Rollback host-side state when sandbox registration or cleanup fails.
add() starts the proxy and port forward before it knows the sandbox side is ready, then ensureMcporter() can fail or mcporter config add ... || true can suppress the registration error. remove() likewise suppresses sandbox cleanup failures and still mutates the registry. That leaves orphaned host processes or local state that no longer matches the sandbox.
Also applies to: 171-193, 226-233
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bin/lib/mcp-bridge.js` around lines 56 - 69, The add/remove flows leave host
state inconsistent when sandbox registration or cleanup fails; update add() to
perform sandbox-side registration (ensureMcporter() and the mcporter "config
add" call) before starting the proxy/port-forward and do not mask errors with
"|| true", and if registration fails ensure you rollback any started
proxy/forwards and delete temporary files/registry entries; update remove() to
run the sandbox cleanup first and only mutate the local registry or stop host
processes after cleanup succeeds, and if cleanup fails ensure you stop any
host-side forwarding/proxy and preserve consistent state; reference the
functions add(), remove(), ensureMcporter(), and the mcporter config add/remove
calls when making these changes.
Author
There was a problem hiding this comment.
Fixed in 8a71a90 — if ensureMcporter() fails, the proxy is killed, PID file removed, and port forward stopped.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
git show 8a71a90 -- bin/lib/mcp-bridge.js
Length of output: 11412
@charlesverdad, the rollback on ensureMcporter() failure is well-done — the proxy is killed, the PID file is removed, and the port forward is stopped. However, two of the three original concerns are still present in this commit:
-
mcporter config add ... || truestill suppresses registration errors (Lines 232–235).
Ifmcporter config addfails (e.g. mcporter rejects the entry), execution falls through, saves the registry entry, and returns success — leaving the proxy running with the sandbox side unregistered. The same rollback pattern added forensureMcporter()should apply here:const configOut = sshExec( sandboxName, `/sandbox/.local/node_modules/.bin/mcporter config add ${name} --url http://localhost:${port} --scope home 2>&1`, ); if (!configOut || /error/i.test(configOut)) { console.error(" mcporter config add failed. Rolling back..."); try { process.kill(proc.pid, "SIGTERM"); } catch {} try { fs.unlinkSync(pidFile(sandboxName, name)); } catch {} run(`openshell forward stop ${port} 2>/dev/null || true`, { ignoreError: true }); return; }
-
remove()still mutates the registry regardless of sandbox cleanup success (Lines 283–293).
mcporter config remove ... || truemasks failures, anddelete sandbox.mcp[serverName]runs unconditionally after. If the sandbox cleanup silently fails, the registry entry disappears but the server remains registered in mcporter inside the sandbox. Consider capturing the output and warning (or prompting--force) if sandbox cleanup appears to fail, before removing the registry entry.
Remove "How It Works" section from the MCP bridge how-to guide. Architecture details live in the PR description and issue NVIDIA#566. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
docs/deployment/set-up-mcp-bridge.md (1)
122-127:⚠️ Potential issue | 🟠 Major
Proposed CLIstill contradicts shipped behavior.At Lines 125-127, the page says
nemoclaw mcpsubcommands are not implemented, but this PR’s objective and implementation scope are formcp add|remove|list|restart. This will mislead readers.Also applies to: 129-192
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/deployment/set-up-mcp-bridge.md` around lines 122 - 127, The docs currently state that the `nemoclaw mcp` subcommands are "not yet implemented", which contradicts this PR that implements `mcp add|remove|list|restart`; update the "Proposed CLI" section and the repeated text (covering the block that currently claims unimplemented behavior) to reflect that `nemoclaw mcp` subcommands are implemented — remove or change the note, explicitly list the implemented subcommands (`mcp add`, `mcp remove`, `mcp list`, `mcp restart`), and adjust any instructions/examples in the same section (the repeated segment) so they describe the shipped behavior and usage rather than claiming they are pending.
🧹 Nitpick comments (2)
docs/deployment/set-up-mcp-bridge.md (2)
31-31: Use lowercasesandboxin the heading text.Line 31 uses “Sandbox” mid-title (
Install mcporter in the Sandbox). Per the word list, this should be lowercase unless it starts a sentence.As per coding guidelines, "sandbox (unless starting a sentence)."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/deployment/set-up-mcp-bridge.md` at line 31, Update the heading text "Install mcporter in the Sandbox" to use lowercase for "sandbox" so it reads "Install mcporter in the sandbox"; locate and edit the heading string in the docs/deployment set-up content (the header that currently says Install mcporter in the Sandbox) to match the style guideline "sandbox (unless starting a sentence)".
159-162: Use second person for reader-facing instructions.Lines 159-162 switch to third person (“The user confirms...”). This page otherwise addresses the reader directly; keep this phrasing in second person for consistency.
As per coding guidelines, "Second person ('you') when addressing the reader."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/deployment/set-up-mcp-bridge.md` around lines 159 - 162, Change the third-person sentence(s) that describe the confirmation step to second person to match the rest of the document: replace "The user confirms the variables are set, then the command proceeds as `mcp add`." (and any adjacent phrasing in the same paragraph that reads in third person) with a second-person instruction such as "You confirm the variables are set, then run the command `mcp add`." Keep the rest of the paragraph ("This reads the server definition by name..." and "It extracts the command and environment variable names only, never values.") intact so only the perspective of the confirmation sentence (the one referencing confirming variables and running `mcp add`) is updated to use "you"/imperative voice.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@docs/deployment/set-up-mcp-bridge.md`:
- Around line 122-127: The docs currently state that the `nemoclaw mcp`
subcommands are "not yet implemented", which contradicts this PR that implements
`mcp add|remove|list|restart`; update the "Proposed CLI" section and the
repeated text (covering the block that currently claims unimplemented behavior)
to reflect that `nemoclaw mcp` subcommands are implemented — remove or change
the note, explicitly list the implemented subcommands (`mcp add`, `mcp remove`,
`mcp list`, `mcp restart`), and adjust any instructions/examples in the same
section (the repeated segment) so they describe the shipped behavior and usage
rather than claiming they are pending.
---
Nitpick comments:
In `@docs/deployment/set-up-mcp-bridge.md`:
- Line 31: Update the heading text "Install mcporter in the Sandbox" to use
lowercase for "sandbox" so it reads "Install mcporter in the sandbox"; locate
and edit the heading string in the docs/deployment set-up content (the header
that currently says Install mcporter in the Sandbox) to match the style
guideline "sandbox (unless starting a sentence)".
- Around line 159-162: Change the third-person sentence(s) that describe the
confirmation step to second person to match the rest of the document: replace
"The user confirms the variables are set, then the command proceeds as `mcp
add`." (and any adjacent phrasing in the same paragraph that reads in third
person) with a second-person instruction such as "You confirm the variables are
set, then run the command `mcp add`." Keep the rest of the paragraph ("This
reads the server definition by name..." and "It extracts the command and
environment variable names only, never values.") intact so only the perspective
of the confirmation sentence (the one referencing confirming variables and
running `mcp add`) is updated to use "you"/imperative voice.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f650bb96-663f-4518-af3d-1aca9b132e50
📒 Files selected for processing (4)
bin/lib/mcp-bridge.jsbin/nemoclaw.jsdocs/deployment/set-up-mcp-bridge.mdscripts/mcp-proxy.js
🚧 Files skipped from review as they are similar to previous changes (3)
- bin/nemoclaw.js
- bin/lib/mcp-bridge.js
- scripts/mcp-proxy.js
- Validate bridge names against RFC pattern before use in file paths and shell commands (prevents injection) - Build minimal child env in mcp-proxy.js with only PATH, HOME, and explicitly named env vars (prevents leaking host secrets) - Add 10MB request body size limit to HTTP proxy - Make port allocation global across all sandboxes (prevents collisions) - Validate explicit --port against range and existing allocations - Rollback proxy and port forward if mcporter bootstrap fails - Split doc into implemented CLI commands vs future work sections Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Security fixes from review: Critical: - Validate command executable against path traversal and shell metacharacters - Strip process.env when spawning proxy — pass only PATH, HOME, and explicitly named --env vars (prevents leaking host secrets) - Add 10MB request body size limit on HTTP proxy High: - Remove CORS wildcard on localhost proxy (prevents drive-by browser CSRF) - Validate sandboxName at all entry points against VALID_NAME_RE (prevents path traversal in /tmp file paths and shell injection via SSH) - Add start-time to PID files and timeout to prevent recycled-PID kills Medium: - Add 120s timeout and 100-request cap on in-flight MCP proxy requests (prevents memory exhaustion from slow/dead child processes) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Author
|
Addressed the two remaining review comments in 382c61c:
|
…ce split Addresses CodeRabbit review: config.command.split(/\s+/) breaks on paths with spaces. The proxy now accepts --exe and --arg separately, so mcp-bridge.js can pass arguments without shell-style splitting. Also fixes mcporter config add suppressing errors — now checks output and rolls back proxy/forward if registration fails. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
charlesverdad
force-pushed
the
feat/mcp-bridge-design
branch
from
382c61c to
68a25d5
Compare
Author
|
Addressed the command whitespace splitting issue in 68a25d5:
|
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
1 participant
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
Adds
nemoclaw <sandbox> mcp add|remove|list|restartsubcommands that bridge stdio-based MCP servers from the host into the sandbox via HTTP proxying and OpenShell port forwarding. API keys stay on the host and never enter the sandbox.Closes #566.
Changes
scripts/mcp-proxy.js127.0.0.1bin/lib/mcp-bridge.jsbin/nemoclaw.jsmcpas a sandbox-scoped action withadd|remove|list|restartsub-dispatch, update help textdocs/deployment/set-up-mcp-bridge.mdArchitecture
Usage
Design properties
127.0.0.1only. Not reachable from the network.openshell forward./sandbox/.local/on first use (writable layer, persists across restarts)./tmp/nemoclaw-services-{name}/, sandbox-scoped commands, registry state in~/.nemoclaw/sandboxes.json.Test plan
npm test)nemoclaw <sandbox> mcp addwith a real MCP servernemoclaw <sandbox> mcp listshows running/stopped statusnemoclaw <sandbox> mcp removecleans up proxy and confignemoclaw <sandbox> mcp restartafter reboot🤖 Generated with Claude Code
Summary by CodeRabbit
nemoclaw <name> mcp {add|remove|list|restart}) to manage and monitor MCP bridges.