fix(mcp): fail unsuccessful verification - #11223
Conversation
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
|
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. |
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: trueComment |
|
🌿 Preview your docs: https://nvidia-preview-pr-11223.docs.buildwithfern.com/nemoclaw |
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall line coverage in commit 3970b65 in the TypeScript / code-coverage/cliThe overall line coverage in commit 3970b65 in the Show a line coverage summary of the most impacted files.
Updated |
|
PR Review Advisor finished for commit |
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
|
PR Review Advisor needs a maintainer decision for the current candidate All ordinary CI and exact managed-image checks passed for this candidate. The exact OpenClaw MCP discovery checks passed twice, including authenticated discovery and denied authentication. The operability and recovery specialist also completed and reported no defect. Eight Advisor specialists in workflow run 34264866154 did not produce their expected review artifacts:
The architecture, customer, verification, and reduction jobs reached the 900-second provider timeout. The documentation, security, migration, and delivery jobs exhausted provider retries with HTTP 429 responses. Their upload steps then had no files to publish. Per the PR follow-up policy, I kept the candidate unchanged and did not rerun the workflow. Maintainers: please decide whether to rerun the full PR Review Advisor workflow for this exact candidate or defer the incomplete Advisor evaluation. |
|
Maintainer decision for candidate The eight missing specialist artifacts are treated as optional review evidence for this candidate; they produced no blocking findings. This decision does not waive new findings or required-check failures that appear later. |
Outcome
MCP tool discovery now fails closed when either the sandbox command or its structured result reports failure. The result preserves the command status, classifies the failed stage, exits non-zero for automation, and keeps credentials and untrusted response text out of diagnostics.
Reason
Hermes can return command status 0 after a connection or authentication failure. Callers that trusted only that status could record an unusable managed MCP endpoint as verified.
Related issues
Fixes #10944
Part of #10942
Related to #6901, #7104, and #8697.
PR #11087 was inspected as prior evidence only. This candidate was implemented independently from current
mainand makes no change to that PR.Changes
commandStatus,failedStage, andfailureClass. The host checks a non-zero command status before parsing output, and a zero-status structured failure remains a failure.mcp status <server> --toolsexit non-zero unless discovery succeeds. Preflight skips, missing runtimes, malformed frames, structured failures, and non-zero sandbox commands therefore remain machine-detectable failures.tools/list, denied authentication with HTTP 401, redacted diagnostics, fixture credential restoration, and exact NemoClaw, Hermes, OpenShell, operating-system, and container-runtime evidence.initialize,notifications/initialized, and paginatedtools/list. Issue Add opt-in live MCP tool discovery to mcp status #6901 accepted this opt-in discovery contract and explicitly excludedtools/call; existing real Hermes adapter calls remain in the live scenario for the final-use requirement from Hermes MCP rebuild reports verified on OpenShell v0.0.101 but first provider-backed tool call returns HTTP 500 #8697.Verification
npx vitest run --project cli src/lib/actions/sandbox/mcp-bridge-tool-discovery.test.ts src/lib/actions/sandbox/mcp-tool-discovery-runtime.test.ts src/lib/actions/sandbox/mcp-bridge-status-resolution.test.ts src/lib/actions/sandbox/rebuild-flow-recovery.test.ts src/lib/actions/sandbox/rebuild-post-restore-phase.test.ts src/lib/actions/sandbox/rebuild-hermes-post-restore.test.ts— 157 passed.npx vitest run --project e2e-support test/e2e/support/mcp-bridge-tool-discovery.test.ts— 51 passed.npx vitest run --project integration test/mcp/mcp-tool-discovery-image-contract.test.ts test/inference/managed/managed-image-publication-workflow.test.ts— 52 passed.npm --prefix tools/mcp-tool-discovery-runtime test— passed.npm --prefix tools/mcp-tool-discovery-runtime run typecheck— passed.npm --prefix tools/mcp-tool-discovery-runtime run bundle:reviewed:check— passed.npm run typecheck:cli— passed.npm run test:changed— 2,074 passed and 15 skipped with the repository-pinned macOS Python test dependencies available.npm run docs— passed with 0 Fern errors.npm run checks:repository— passed.npm run e2e:assertions:check— passed without increasing the assertion budget.npx tsx scripts/checks/e2e-mock-parity.mts --base origin/main— passed.npm run validate:pr— pre-commit, commit-message, and pre-push validation passed.0689940before the review repairs. Each MCP pass exercised authenticated discovery, denied authentication, and the fixture-credential artifact scan.Review notes
The canonical sensitive-path policy at base
3e605bc55b4a660b03bf1e132bdd9725078e346cclassifies.github/workflows/managed-images.yaml,agents/hermes/Dockerfile,agents/langchain-deepagents-code/Dockerfile, andtools/mcp-tool-discovery-runtime/**as sensitive.The PR Review Advisor completed for candidate
0689940813e02c30571dd19dd64821a66dfb6f01. This candidate resolves its documentation drift, older discovery-protocol recovery, and exact Dockerfile instruction-hash findings. The architecture specialist recommended one writable protocol owner across the host, bundled sandbox runtime, Dockerfiles, and workflow. I did not apply that recommendation because those consumers cross host/sandbox and source/generated-image trust boundaries. The host cannot import the sandbox runtime, while Dockerfile and workflow literals are exact publication assertions. The reduction specialist found the independent boundary checks appropriate, and the contract tests continue to require exact agreement.I self-reviewed the complete
NVIDIA/NemoClawcandidate at3970b654138630b603bab7f772b0bdb8fb2ffb7f, including input trust, command construction, credential handling and redaction, process and structured-result classification, retry and cleanup behavior, lifecycle failure propagation, managed-image publication, the review repairs, and the conflict-free integration of currentmain. I found no unresolved candidate-owned blocker. This draft awaits evaluation of the current candidate.Signed-off-by: Apurv Kumaria akumaria@nvidia.com