feat(cli): add opt-in MCP tool discovery - #6904
Closed
ericksoa wants to merge 74 commits into
Closed
Conversation
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
ericksoa
added
area: cli
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds opt-in, names-only MCP tool discovery to ChangesMCP tool discovery
Gateway supervisor discovery
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant StatusMcpBridge
participant SandboxRuntime
participant MCPServer
CLI->>StatusMcpBridge: mcp status server --tools
StatusMcpBridge->>SandboxRuntime: launch discovery through adapter boundary
SandboxRuntime->>MCPServer: initialize and tools/list
MCPServer-->>SandboxRuntime: paginated tool names
SandboxRuntime-->>StatusMcpBridge: validated toolDiscovery result
StatusMcpBridge-->>CLI: render names-only status
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Contributor
|
🌿 Preview your docs: https://nvidia-preview-pr-6904.docs.buildwithfern.com/nemoclaw |
Contributor
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage in commit b7fd458 in the TypeScript / code-coverage/cliThe overall coverage in commit b7fd458 in the Show a code coverage summary of the most impacted files.
Updated |
Contributor
PR Review Advisor — InformationalAdvisor assessment: Informational / high confidence Model lanes
Nemotron output stays in workflow artifacts and does not change the assessment above. E2E guidanceAdvisory only. E2E / PR Gate selects and runs jobs independently. Recommended E2E: This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
src/lib/actions/sandbox/mcp-bridge-runtime-command.ts (1)
26-48: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueAdd an exhaustiveness guard to
wrapMcpRuntimeCommandto fail loudly on future adapter additions. The switch has nodefaultcase; its "always returns a string" guarantee depends solely onAgentMcpAdapterhaving exactly 3 literal members today. If a 4th adapter is ever added without updating this switch, the function returnsundefinedat runtime (unless the repo'stsconfigseparately enablesnoImplicitReturns, which is not part ofstrict: true), and its only current consumer joins that into a shell script where the missing command silently shifts which prior command's exit code gets captured — a silent-misclassification failure mode rather than a loud one.
src/lib/actions/sandbox/mcp-bridge-runtime-command.ts#L26-L48: add adefaultbranch that throws (e.g., anassertNever(adapter)helper) so an unhandled adapter fails immediately instead of returningundefined.src/lib/actions/sandbox/mcp-bridge-resolution-probe.ts#L194-L204: no code change needed once the guard above is added; this site is listed only to document the downstream impact of the missing guard.🛡️ Proposed defensive guard
+function assertNeverAdapter(adapter: never): never { + throw new Error(`Unhandled MCP adapter: ${String(adapter)}`); +} + export function wrapMcpRuntimeCommand( adapter: AgentMcpAdapter, command: readonly string[], ): string { const quotedCommand = command.map(shellQuote).join(" "); switch (adapter) { case "mcporter": { ... } case "deepagents-config": { ... } + default: + return assertNeverAdapter(adapter); } }🤖 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 `@src/lib/actions/sandbox/mcp-bridge-runtime-command.ts` around lines 26 - 48, Update wrapMcpRuntimeCommand in src/lib/actions/sandbox/mcp-bridge-runtime-command.ts:26-48 to add a default exhaustiveness guard that throws for any unsupported AgentMcpAdapter, using an assertNever-style helper if appropriate, so the function cannot return undefined. No code change is needed at src/lib/actions/sandbox/mcp-bridge-resolution-probe.ts:194-204; it documents the downstream impact and is corrected by the guard.src/lib/actions/sandbox/mcp-bridge-tool-discovery.test.ts (2)
30-30: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMissing local issue-reference suffix on the
describetitle.This suite backs issue
#6901's tool-discovery feature but thedescribetitle doesn't carry a(#6901)suffix.As per coding guidelines,
**/*.test.{js,ts}: "Use behavior-oriented test titles and place local issue references in a final(#1234)suffix."🤖 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 `@src/lib/actions/sandbox/mcp-bridge-tool-discovery.test.ts` at line 30, Update the describe title in the MCP tool discovery test suite to retain its behavior-oriented wording and append the local issue reference suffix “(`#6901`)” at the end.Source: Coding guidelines
30-48: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for
buildMcpToolDiscoveryCommand's null-returning branches.Only the successful build path is tested. The two fail-closed branches — no credential binding (
authorizationValuereturns falsy) and a URL that doesn't round-trip throughnormalizeMcpServerUrl— aren't exercised, even though they're part of the security-relevant "skip if unsafe" contract.🤖 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 `@src/lib/actions/sandbox/mcp-bridge-tool-discovery.test.ts` around lines 30 - 48, The tests for buildMcpToolDiscoveryCommand only cover successful command construction and omit its fail-closed behavior. Add cases asserting it returns null when authorizationValue produces a falsy credential binding and when the server URL fails to round-trip through normalizeMcpServerUrl, while preserving the existing success-path coverage.src/lib/actions/sandbox/mcp-bridge-status.ts (1)
243-277: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the duplicated readiness object shared by probe and discovery.
{ policyGatewayPresent: policyPresence, providerAttached: attached, providerCredentialReady }is built twice, once forprobeCredentialResolutionand once fordiscoverMcpTools. Hoisting it into a singleconst readiness = {...}keeps the two security gates from silently diverging if one call site is updated without the other.♻️ Proposed refactor
if (resolutionWarning) warnings.push(resolutionWarning); + const toolDiscoveryReadiness = { + policyGatewayPresent: policyPresence, + providerAttached: attached, + providerCredentialReady, + }; const toolDiscovery = options.discoverTools && entry ? unsafeCredentialMayBeAttached ? { ok: false, count: 0, tools: [], truncated: false, detail: "tool discovery skipped: the unsupported legacy credential may still be attached to fresh sandbox children", } - : discoverMcpTools(sandboxName, entry, support.adapter, { - policyGatewayPresent: policyPresence, - providerAttached: attached, - providerCredentialReady, - }) + : discoverMcpTools(sandboxName, entry, support.adapter, toolDiscoveryReadiness) : undefined;🤖 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 `@src/lib/actions/sandbox/mcp-bridge-status.ts` around lines 243 - 277, In the surrounding status function, create one shared const readiness object containing policyGatewayPresent, providerAttached, and providerCredentialReady before the credential resolution and tool discovery calls. Pass this readiness object to both probeCredentialResolution and discoverMcpTools, removing their duplicated inline objects while preserving the existing conditional behavior.
🤖 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.
Inline comments:
In `@tools/mcp-tool-discovery-runtime/install-reviewed-runtime.sh`:
- Around line 38-40: Add a shell shebang at the beginning of
install-reviewed-runtime.sh before the SPDX headers, and change the file mode to
executable (0755) so it can be invoked directly.
---
Nitpick comments:
In `@src/lib/actions/sandbox/mcp-bridge-runtime-command.ts`:
- Around line 26-48: Update wrapMcpRuntimeCommand in
src/lib/actions/sandbox/mcp-bridge-runtime-command.ts:26-48 to add a default
exhaustiveness guard that throws for any unsupported AgentMcpAdapter, using an
assertNever-style helper if appropriate, so the function cannot return
undefined. No code change is needed at
src/lib/actions/sandbox/mcp-bridge-resolution-probe.ts:194-204; it documents the
downstream impact and is corrected by the guard.
In `@src/lib/actions/sandbox/mcp-bridge-status.ts`:
- Around line 243-277: In the surrounding status function, create one shared
const readiness object containing policyGatewayPresent, providerAttached, and
providerCredentialReady before the credential resolution and tool discovery
calls. Pass this readiness object to both probeCredentialResolution and
discoverMcpTools, removing their duplicated inline objects while preserving the
existing conditional behavior.
In `@src/lib/actions/sandbox/mcp-bridge-tool-discovery.test.ts`:
- Line 30: Update the describe title in the MCP tool discovery test suite to
retain its behavior-oriented wording and append the local issue reference suffix
“(`#6901`)” at the end.
- Around line 30-48: The tests for buildMcpToolDiscoveryCommand only cover
successful command construction and omit its fail-closed behavior. Add cases
asserting it returns null when authorizationValue produces a falsy credential
binding and when the server URL fails to round-trip through
normalizeMcpServerUrl, while preserving the existing success-path coverage.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 69705462-7103-4530-9c46-dfafb1a75b09
⛔ Files ignored due to path filters (1)
tools/mcp-tool-discovery-runtime/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (28)
Dockerfileagents/hermes/Dockerfileagents/langchain-deepagents-code/Dockerfiledocs/manage-sandboxes/manage-mcp-servers.mdxdocs/reference/commands.mdxsrc/lib/actions/sandbox/mcp-bridge-contracts.tssrc/lib/actions/sandbox/mcp-bridge-render.tssrc/lib/actions/sandbox/mcp-bridge-resolution-probe.tssrc/lib/actions/sandbox/mcp-bridge-runtime-command.tssrc/lib/actions/sandbox/mcp-bridge-status-resolution.test.tssrc/lib/actions/sandbox/mcp-bridge-status.tssrc/lib/actions/sandbox/mcp-bridge-tool-discovery.test.tssrc/lib/actions/sandbox/mcp-bridge-tool-discovery.tssrc/lib/actions/sandbox/mcp-bridge.tssrc/lib/actions/sandbox/mcp-tool-discovery-runtime.test.tssrc/lib/cli/public-display-mcp.test.tssrc/lib/cli/public-display-mcp.tssrc/lib/sandbox/build-context.tstest/e2e/live/mcp-bridge-servers.tstest/e2e/live/mcp-bridge-tool-discovery.tstest/e2e/live/mcp-bridge.test.tstest/onboard.test.tstest/sandbox-build-context.test.tstools/mcp-tool-discovery-runtime/dependency-review.mdtools/mcp-tool-discovery-runtime/install-reviewed-runtime.shtools/mcp-tool-discovery-runtime/mcp-tool-discovery.tstools/mcp-tool-discovery-runtime/package.jsontools/mcp-tool-discovery-runtime/tool-discovery-core.ts
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@tools/mcp-tool-discovery-runtime/tsconfig.json`:
- Line 1: Add the repository’s standard SPDX license header as a JSONC comment
at the beginning of tools/mcp-tool-discovery-runtime/tsconfig.json, before the
opening object; leave the existing configuration unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: b0a7504a-709b-45ed-92c9-83ce4227b84a
⛔ Files ignored due to path filters (1)
tools/mcp-tool-discovery-runtime/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (16)
Dockerfileagents/hermes/Dockerfileagents/langchain-deepagents-code/Dockerfilesrc/lib/actions/sandbox/mcp-bridge-runtime-command.tssrc/lib/actions/sandbox/mcp-bridge-status.tssrc/lib/actions/sandbox/mcp-bridge-tool-discovery.test.tssrc/lib/actions/sandbox/mcp-tool-discovery-runtime.test.tssrc/lib/sandbox/build-context.tstest/sandbox-build-context.test.tstools/mcp-tool-discovery-runtime/dependency-review.mdtools/mcp-tool-discovery-runtime/install-reviewed-runtime.shtools/mcp-tool-discovery-runtime/mcp-tool-discovery.tstools/mcp-tool-discovery-runtime/package.jsontools/mcp-tool-discovery-runtime/tool-discovery-core.tstools/mcp-tool-discovery-runtime/tsconfig.jsontsconfig.cli.json
🚧 Files skipped from review as they are similar to previous changes (13)
- tools/mcp-tool-discovery-runtime/package.json
- tools/mcp-tool-discovery-runtime/dependency-review.md
- src/lib/sandbox/build-context.ts
- src/lib/actions/sandbox/mcp-bridge-runtime-command.ts
- test/sandbox-build-context.test.ts
- src/lib/actions/sandbox/mcp-bridge-status.ts
- src/lib/actions/sandbox/mcp-tool-discovery-runtime.test.ts
- src/lib/actions/sandbox/mcp-bridge-tool-discovery.test.ts
- agents/langchain-deepagents-code/Dockerfile
- agents/hermes/Dockerfile
- tools/mcp-tool-discovery-runtime/tool-discovery-core.ts
- tools/mcp-tool-discovery-runtime/mcp-tool-discovery.ts
- Dockerfile
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
test/langchain-deepagents-code-profile-build-gate.test.ts (2)
111-111: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd the issue reference to the test title.
Use
accepts %s as a reviewed source-gate ARG (#6901)to satisfy the repository’s test-title convention.
As per coding guidelines, local issue references must be a final(#1234)suffix.🤖 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 `@test/langchain-deepagents-code-profile-build-gate.test.ts` at line 111, Update the parameterized test title in the accepts reviewed source-gate ARG test to append the required final issue-reference suffix `(`#6901`)` after the existing `%s` placeholder.Source: Coding guidelines
108-115: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winExercise each reviewed ARG through the gate fixture. The current case only checks
check-dcode-profile-import-gate.shwithtoContain(reviewedArg), so it doesn’t tie each parameter value to the gate input. AppendARG ${reviewedArg}to a copied Dockerfile fixture and assert the gate succeeds from that input instead.🤖 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 `@test/langchain-deepagents-code-profile-build-gate.test.ts` around lines 108 - 115, Update the parameterized test around runGateWithFakeDocker so each reviewedArg is appended as an ARG declaration to a copied Dockerfile fixture, then run the gate against that modified fixture and assert success. Remove the direct checkPath toContain assertion, ensuring each parameter value is exercised through the gate input itself.Source: Path instructions
🤖 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 `@test/langchain-deepagents-code-profile-build-gate.test.ts`:
- Line 111: Update the parameterized test title in the accepts reviewed
source-gate ARG test to append the required final issue-reference suffix
`(`#6901`)` after the existing `%s` placeholder.
- Around line 108-115: Update the parameterized test around
runGateWithFakeDocker so each reviewedArg is appended as an ARG declaration to a
copied Dockerfile fixture, then run the gate against that modified fixture and
assert success. Remove the direct checkPath toContain assertion, ensuring each
parameter value is exercised through the gate input itself.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: aa658d5f-b013-4705-b6c9-4f219bd4bd3a
📒 Files selected for processing (2)
scripts/check-dcode-profile-import-gate.shtest/langchain-deepagents-code-profile-build-gate.test.ts
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
21 tasks
cv
added a commit
that referenced
this pull request
Jul 15, 2026
<!-- markdownlint-disable MD041 --> ## Summary Keep control-plane E2E authorization in progress so GitHub can advance the same coordination check from authorization to running and then to its terminal verdict. This fixes the lifecycle observed on #6904, where GitHub preserved the completed failure conclusion even after the controller changed the check title to `Running 9 E2E jobs`, causing the native required job to fail while E2E was still running. ## Changes - Leave an internal control-plane authorization checkpoint `in_progress`, validate that pending state during manual authorization, and restore it after a retryable controller failure. - Reject completed authorization checks instead of reinterpreting them; older builds require a fresh exact-diff revision and PR-CI run before authorization. - Fail closed after any child dispatch: request cancellation, publish a terminal reconciliation result, and require a fresh exact diff rather than risk duplicate credential-bearing execution. - Update controller and native-observer lifecycle coverage for the pending authorization-to-running-to-success sequence, plus the E2E operator guide for pending authorization and retry restoration. ## Type of Change - [ ] Code change (feature, bug fix, or refactor) - [x] Code change with doc updates - [ ] Doc only (prose changes, no code sample modifications) - [ ] Doc only (includes code sample changes) ## Quality Gates - [x] Tests added or updated for changed behavior - [ ] Existing tests cover changed behavior — justification: - [ ] Tests not applicable — justification: - [x] Docs updated for user-facing behavior changes - [ ] Docs not applicable — justification: - [x] Sensitive paths changed (security, policy, credentials, preflight, onboarding, inference, runner, sandbox, or messaging) - [ ] Sensitive-path review completed or maintainer-approved waiver recorded — reviewer/approval link/justification: Required before merge; this changes the trusted controller state machine around credential-bearing E2E authorization while preserving exact head/base, maintainer-role, and risk-plan validation. - [ ] Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue: ## Verification - [x] PR description includes a `Signed-off-by:` line and every commit appears as `Verified` in GitHub - [x] Normal `pre-commit`, `commit-msg`, and `pre-push` hooks passed, or `npm run check:diff` passed when hooks were skipped or unavailable - [x] Targeted behavior tests pass for the current change set, or tests are marked not applicable above — `npx vitest run test/pr-e2e-gate-fork-skip.test.ts test/pr-e2e-required.test.ts test/pr-e2e-gate-workflow.test.ts test/pr-e2e-gate-lifecycle.test.ts test/pr-e2e-gate.test.ts` (5 files, 108 tests passed); `npm run typecheck:cli`; `npm run source-shape:check`; `npm run test:projects:check`; `npm run test-size:check` - [ ] Applicable broad gate passed — not applicable; this is a focused controller/observer lifecycle correction covered by the targeted controller, native-observer, workflow, and lifecycle suites above - [ ] Quality Gates section completed with required justifications or waivers — pending sensitive-path review above - [x] No secrets, API keys, or credentials committed - [ ] `npm run docs` builds without warnings (doc changes only) — build passed with the same two hidden Fern warnings - [ ] Doc pages follow the [style guide](https://github.com/NVIDIA/NemoClaw/blob/main/docs/CONTRIBUTING.md) (doc changes only) — not applicable; only the E2E operator README changed - [ ] New doc pages include SPDX header and frontmatter (new pages only) --- Signed-off-by: Carlos Villela <cvillela@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added a dedicated pending authorization check state for credentialed E2E runs, including clearer workflow instructions and required validation inputs. * Added reconciliation behavior when dispatched E2E jobs can’t be cancelled or completed reliably. * **Bug Fixes** * Prevented authorization checks from being misinterpreted as retryable waiting states after failures. * Required fresh authorization when base/head revisions change. * Improved restoration and strict handling of intermediate authorization/observer states. * **Documentation** * Updated E2E gate guidance to reflect the in-progress authorization and reconciliation flows. * **Tests** * Expanded E2E coverage for fork-skip approval and authorization-close scenarios. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
ericksoa
marked this pull request as draft
July 15, 2026 16:43
|
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. |
cv
marked this pull request as ready for review
July 15, 2026 16:50
senthilr-nv
reviewed
Jul 25, 2026
Collaborator
There was a problem hiding this comment.
Superseded by the exact-head approval: #6904 (review)
senthilr-nv
approved these changes
Jul 25, 2026
senthilr-nv
left a comment
senthilr-nv
left a comment
Collaborator
There was a problem hiding this comment.
Exact-head approval — d8ea51513
APPROVED.
Product scope: approved. Issue #6901 and the accepted maintainer decision establish the trusted-configured-endpoint contract for this integration. The upstream OpenShell work remains defense in depth and is not a product-scope blocker.
Code and security: approved. I found no blocking implementation or security defect in this head. Focused local validation passed across CLI/runtime tests, integration and E2E-support tests, type checks, builds, the docs build, and the production dependency audit. The test that timed out in CI also passed locally (14/14).
The remaining items are author-owned merge gates, not blockers to this code-review approval:
- Let the active CLI rerun complete successfully.
- Dispatch and pass the protected exact-head E2E matrix for OpenClaw, Hermes, and Deep Agents.
- Rerun the Documentation Writer Review and update its receipt for
d8ea51513; the current receipt records1b22a5797and says no docs changed, while this head changes two documentation pages. - Resolve or dismiss stale
CHANGES_REQUESTEDreviews as appropriate after the current-head requirements are satisfied.
This approval does not waive required checks or documentation governance; the author will take care of those before merge.
apurvvkumaria
enabled auto-merge (squash)
July 25, 2026 16:11
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
cv
disabled auto-merge
July 26, 2026 01:50
Collaborator
|
Moving this PR out of v0.0.96, not closing it. #6901 establishes valid product scope and this remains its only closing implementation, but the current branch has 74 accumulated commits, no human security approval for exact head b7fd458, and no passing protected OpenClaw/Hermes/Deep Agents matrix because the exact-head run failed in generate-matrix. Safe continuation is a fresh linear port of only the accepted MCP-discovery changes onto current main, followed by current-architecture review, an exact docs receipt, fresh human security approval, and the protected three-adapter E2E. |
23 tasks
Contributor
Author
|
Superseded by #7591, which rebuilds this work on current main around the shared MCP bridge/status architecture. |
prekshivyas
added a commit
that referenced
this pull request
Jul 27, 2026
## Summary Adds an opt-in `nemoclaw <sandbox> mcp status <server> --tools` diagnostic that lists tool names through the configured MCP registration without invoking tools. This is a clean, linear replacement for the accumulated branch in #6904, rebuilt directly on current `main` around the accepted #6901 trust boundary and current three-adapter architecture. ## Related Issue Closes #6901 ## Changes - Add bounded MCP `initialize` and paginated `tools/list` discovery with strict time, request, response, page, tool, cursor, and name limits. - Reuse the existing credential provider, network policy, MCP registration, and adapter ancestry; do not add a host fallback or a new policy path. - Pin, lock, audit, and bundle one official MCP SDK runtime into the OpenClaw, Hermes, and Deep Agents images. - Add status, JSON, help, and user documentation for the opt-in diagnostic. - Add unit, image-contract, support, and protected-compatible live E2E coverage for all three adapters, pagination, credential rewriting, session/protocol metadata, best-effort cleanup behavior, and the invariant that discovery never sends `tools/call`. - The shared runtime is required because Hermes and Deep Agents cannot reuse OpenClaw's `mcporter`; separate adapter clients would duplicate a security-sensitive protocol path. `test/e2e/live/mcp-bridge.test.ts` and the runtime/image contract tests protect the shared implementation. ## Type of Change - [ ] Code change (feature, bug fix, or refactor) - [x] Code change with doc updates - [ ] Doc only (prose changes, no code sample modifications) - [ ] Doc only (includes code sample changes) ## Quality Gates - [x] Tests added or updated for changed behavior - [ ] Existing tests cover changed behavior — justification: - [ ] Tests not applicable — justification: - [x] Docs updated for user-facing behavior changes - [ ] Docs not applicable — justification: - [x] Sensitive paths changed (security, policy, credentials, preflight, onboarding, inference, runner, sandbox, or messaging) - [x] Sensitive-path review completed or maintainer-approved waiver recorded — reviewer/approval link/justification: Codex Desktop reviewed exact head `6688fe6fc3fed997fe89e7322429c1f5505fe5d9` across all nine repository security categories, including independent validation of the fail-closed evidence path, and found no issues. Exact-head advisors, CodeQL, secret scanning, CI, and protected E2E passed. This does not replace the fresh human security approval required before merge. Accepted trust contract: #6901 (comment) - [ ] Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue: ## Documentation Writer Review - [x] Documentation writer subagent reviewed the completed changes - Result: `docs-updated` - Evidence: `docs/manage-sandboxes/manage-mcp-servers.mdx`, `docs/reference/commands.mdx`, `test/e2e/README.md`, `tools/mcp-tool-discovery-runtime/dependency-review.md`; reviewed terminology, structure, voice, security bounds, and code-sample presentation against accepted #6901; `npm run docs` passed with 0 errors. - Agent: Codex Desktop <!-- docs-review-head-sha: 7a0a432 --> <!-- docs-review-agents-blob-sha: be20a09 --> ## DGX Station Hardware Evidence - [ ] Tested on DGX Station - Tested commit: - Station profile/scenario: - Result: - Supporting evidence: ## Verification - [x] PR description includes a `Signed-off-by:` line and every commit appears as `Verified` in GitHub - [x] Normal `pre-commit`, `commit-msg`, and `pre-push` hooks passed, or `npm run check:diff` passed when hooks were skipped or unavailable - [x] Targeted behavior tests pass for the current change set, or tests are marked not applicable above — command/result or justification: CLI-focused tests passed (41); compatibility classifier, real-CLI, risk reporter, base-gate, and workflow/uploader suites passed (81); the four unrelated support files that timed out only under whole-project local contention passed 101/101 on isolated rerun. E2E semantic phase coverage passed (125 tests across 82 files); shared fake-server integration passed (10/10). `npm run typecheck:cli`, `npm run check:diff`, runtime/project boundary checks, and `npm run docs` passed on exact head `6688fe6fc3fed997fe89e7322429c1f5505fe5d9`. - [x] Applicable broad gate passed — `npm test` for broad runtime/test-harness changes; `npm run check` for repo-wide validation/coverage changes — command/result: Exact-head PR CI passed static checks, build/typecheck, reviewed npm audit, installer integration, plugin tests, and all 8 CLI shards (https://github.com/NVIDIA/NemoClaw/actions/runs/30213491792); self-hosted CI passed both sandbox-image builds and all smoke/integration jobs (https://github.com/NVIDIA/NemoClaw/actions/runs/30213492138); protected E2E passed all 14 selected concrete jobs and controller evidence verification (https://github.com/NVIDIA/NemoClaw/actions/runs/30213967327; https://github.com/NVIDIA/NemoClaw/actions/runs/30213947255). - [x] Quality Gates section completed with required justifications or waivers - [x] No secrets, API keys, or credentials committed - [ ] `npm run docs` builds without warnings (doc changes only) — 0 errors; two pre-existing Fern warnings remain. - [x] Doc pages follow the [style guide](https://github.com/NVIDIA/NemoClaw/blob/main/docs/CONTRIBUTING.md) (doc changes only) - [ ] New doc pages include SPDX header and frontmatter (new pages only) --- Signed-off-by: Aaron Erickson <aerickson@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added `mcp status --tools` to retrieve a point-in-time list of a server’s advertised MCP tool names, with bounded results and truncation when limits are reached. * `mcp status` JSON now includes `toolDiscovery` (`ok`, `count`, `tools`, `truncated`, optional `detail`); `--tools` can run alongside `--probe/--no-probe` with consistent redaction. * **Documentation** * Updated command reference and MCP server management docs to explain discovery behavior, readiness/trust requirements, output structure, and rebuild notes for older images. * **Bug Fixes / Security** * Improved discovery validation and tightened failure handling to avoid malformed/unsafe inventories and reduce risk of sensitive data leakage. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Aaron Erickson <aerickson@nvidia.com> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Prekshi Vyas <prekshiv@nvidia.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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 opt-in live MCP tool-name discovery with
nemoclaw <sandbox> mcp status <server> --toolsfor managed OpenClaw, Hermes, and LangChain Deep Agents Code sandboxes. The implementation uses one locked official MCP SDK runtime in every agent image, preserves the existing plain status contract, and never invokes a discovered tool.Related Issue
Fixes #6901
Changes
--toolsfor one explicitly named managed MCP server, including text output and the additive JSON fieldtoolDiscovery: { ok, count, tools, truncated, detail? }.--toolsis used alone;--probe --toolsintentionally runs both live checks.initialize,notifications/initialized, and paginatedtools/listlifecycle with bounded total/per-request time, cumulative response bytes, pages, tools, cursor bytes, and tool-name bytes.tools/call.tools/callassertion.The shared runtime/adapter-ancestor abstraction is required because OpenShell policy is bound to the selected agent runtime and process ancestry. Three independent agent-specific clients would duplicate MCP lifecycle and safety behavior; invoking one locked client below the existing adapter ancestor preserves that policy contract and gives every supported agent the same result semantics.
src/lib/actions/sandbox/mcp-bridge-tool-discovery.test.ts,src/lib/actions/sandbox/mcp-tool-discovery-runtime.test.ts, andtest/e2e/live/mcp-bridge.test.tsprotect this boundary.Type of Change
Quality Gates
openshell:resolveplaceholder, network traffic is readiness-gated and bounded, untrusted output is strictly parsed/redacted, the image runtime is root-owned and non-writable, and tests prove that notools/callrequest is sent.Documentation Writer Review
docs-updateddocs/manage-sandboxes/manage-mcp-servers.mdx,docs/reference/commands.mdx, the reporter lifecycle fixture repair, and the current-main merge, againstdocs/CONTRIBUTING.mdandWRITING.md; no findings. The pages accurately document names-only authenticated discovery, its JSON contract, credential boundary, lifecycle, limits, failure behavior, and rebuild guidance. Focused reporter tests pass 16/16;npm run check:diffand pre-push CLI TypeScript pass on PR SHAb7fd45892.Verification
Signed-off-by:line and every commit appears asVerifiedin GitHubpre-commit,commit-msg, andpre-pushhooks passed, ornpm run check:diffpassed when hooks were skipped or unavailabletools/call.npm testfor broad runtime/test-harness changes;npm run checkfor repo-wide validation/coverage changes — pending required PR CI on the exact remote head; no non-success acceptance is requested.npm run docsbuilds without warnings (doc changes only) —npm run docs:strictpasses with 0 errors; Fern reports 2 existing hidden warnings.Signed-off-by: Aaron Erickson aerickson@nvidia.com
Signed-off-by: Prekshi Vyas prekshiv@nvidia.com
Summary by CodeRabbit
mcp status --toolsfor opt-in, names-only discovery of advertised MCP tool names.mcp statuscan now include atoolDiscoverysection with success flag, discovered tool count, and truncation state.--toolsso the default credential probe is skipped unless explicitly enabled.