fix(mcp): probe wire-level credential resolution in mcp status and add (#6379)#6382
Conversation
|
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. |
|
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:
📝 WalkthroughWalkthroughThis PR adds a wire-level credential-resolution probe for MCP bridge servers, wires it into ChangesCredential resolution probe feature
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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: 3
🧹 Nitpick comments (5)
docs/deployment/set-up-mcp-bridge.mdx (2)
214-214: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReword for clarity: statement order implies a contradiction.
The sentence says
--probe"force[s] it for every server in the multi-server status form," then immediately says the baremcp status(multi-server) form "never" probes. Read literally in sequence, this looks contradictory even though the intent is "never probes by default." Consider clarifying that "never" refers to default behavior only.✏️ Suggested rewording
-Pass `--no-probe` to skip the probe, or `--probe` to force it for every server in the multi-server status form; the bare `mcp list` and `mcp status` forms never probe so they stay fast. +By default, the bare `mcp list` and `mcp status` forms never probe so they stay fast; pass `--probe` to force the probe for every server in the multi-server status form, or `--no-probe` to skip probing when a single server is named.🤖 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 `@docs/deployment/set-up-mcp-bridge.mdx` at line 214, Reword the probe description in the MCP bridge docs to remove the apparent contradiction: clarify that `--probe` forces probing for every server in the multi-server status form, while the bare `mcp list` and `mcp status` forms do not probe by default and therefore stay fast. Use the existing wording around the probe flags and the `mcp list`/`mcp status` forms to make the default behavior explicit.
210-216: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the
--probe/--no-probeconflict rejection and the success-case text output.This section documents
FAILEDandunknownverdicts and the--probe/--no-probeflags, but per the PR description, passing both flags together is rejected by the CLI, and the successful case has its own human-readable text (credential resolution: verified). Neither is mentioned here, so a reader trying--probe --no-probetogether or looking for the success-state wording won't find guidance.📝 Suggested addition
Pass `--no-probe` to skip the probe, or `--probe` to force it for every server in the multi-server status form; the bare `mcp list` and `mcp status` forms never probe so they stay fast. +Passing both `--probe` and `--no-probe` together is rejected. +A successful probe reports `credential resolution: verified` in text output and `ok: true` in JSON. An HTTP 2xx verdict proves end-to-end resolution; HTTP 400, 401, or 403 means the upstream rejected the request on the same path agents use; endpoint outages, policy denials, timeouts, and unreachable sandboxes report `ok: null` so an endpoint problem is never blamed on the host's credential rewrite.🤖 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 `@docs/deployment/set-up-mcp-bridge.mdx` around lines 210 - 216, Update the MCP bridge docs to mention the CLI behavior in this section: the `mcp status`/`mcp add` probe flags reject being used together, so describe that `--probe` and `--no-probe` are mutually exclusive and the command errors if both are passed. Also add the text-mode success wording for the probe result, using the same `provider.credentialResolution`/`credential resolution:` terminology already present, so readers can see that the successful human-readable output is `credential resolution: verified`.docs/reference/commands-nemohermes.mdx (1)
1278-1286: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing documentation for
--probe --no-probeconflict.Same gap as in the deployment guide: the flags table doesn't mention that combining
--probeand--no-probeis rejected. Since this table is the canonical flag reference, it's the most natural place for that detail.🤖 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 `@docs/reference/commands-nemohermes.mdx` around lines 1278 - 1286, Update the `mcp status` flag reference so it explicitly documents that `--probe` and `--no-probe` cannot be used together and that the command rejects this combination. Add this note in the flag table or nearby explanatory text for the `nemohermes my-assistant mcp status` section, keeping the wording aligned with the existing `--probe` and `--no-probe` descriptions.docs/reference/commands.mdx (1)
1600-1608: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing documentation for
--probe --no-probeconflict.Same gap as the nemohermes variant and the deployment guide: the flags table doesn't document that
--probeand--no-probeare mutually exclusive and rejected together. Recommend adding a row or note to keep this reference authoritative.🤖 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 `@docs/reference/commands.mdx` around lines 1600 - 1608, The command reference for mcp status is missing the mutual-exclusion rule for the probe flags, so update the documentation in the status command section to explicitly state that --probe and --no-probe cannot be used together and will be rejected; add this as a note or a dedicated row alongside the existing flag descriptions so the mcp status reference remains authoritative.src/lib/actions/sandbox/mcp-bridge-status-resolution.test.ts (1)
246-280: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPost-add tests fully no-op the commit path, only exercising probe wiring.
addRestart.addMcpBridge = async () => {};bypasses the real add/commit logic entirely. Since thegithubbridge is already pre-registered byharnessPrelude(lines 73-92), these tests never actually add a new server — they only verify thatreportAddCredentialResolutionruns against an already-existing entry after a no-op "add". This narrows test coverage to just the probe hookup and doesn't prove the real add flow (e.g., handling of a genuinely new entry, or ordering relative to the actual commit) behaves correctly with the probe.Consider mocking only the host-boundary calls inside the real
addMcpBridge(e.g., the OpenShell/process calls it makes) rather than stubbing the whole function, so the actual commit logic is still exercised alongside the probe.As per path instructions, "Flag copied production algorithms, broad mocks that bypass the behavior under test, and conditionals that make a test pass without exercising its claim."
🤖 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-resolution.test.ts` around lines 246 - 280, The post-add test is bypassing the real add/commit path by stubbing addRestart.addMcpBridge to a no-op, so it only checks probe wiring against an already pre-registered bridge. Update the test to exercise the actual addMcpBridge flow while mocking only the external host/process boundaries it depends on, so reportAddCredentialResolution is verified in the context of a real add of a new MCP server and the commit ordering remains covered.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.
Inline comments:
In `@src/lib/actions/sandbox/mcp-bridge-resolution-probe.test.ts`:
- Around line 92-100: The test in classifyCredentialResolutionProbe currently
treats HTTP 400 the same as 401/403, but 400 should remain indeterminate. Update
the loop in mcp-bridge-resolution-probe.test.ts so it only asserts failure for
the 401 and 403 cases, and add a separate assertion for 400 that expects an
indeterminate result from classifyCredentialResolutionProbe while still
verifying the returned httpStatus.
In `@src/lib/actions/sandbox/mcp-bridge-resolution-probe.ts`:
- Around line 149-156: In mcp-bridge-resolution-probe’s HTTP handling, do not
treat every 400 as an OpenShell credential-resolution failure; narrow the
failure classification in the probe result builder so only true
credential/auth-related responses map to ok: false. Update the logic around the
httpStatus check and the excerpt/detail construction to avoid labeling MCP
initialize or endpoint-validation 400s as OpenShell errors, and keep status/add
attribution tied to the actual source of failure.
- Around line 84-117: The probe body file path is predictable and derived from
entry.server, which can cause collisions and symlink/race issues when multiple
probes run or in the same sandbox. Update mcp-bridge-resolution-probe.ts to
create the response file with mktemp instead of hardcoding
/tmp/nemoclaw-mcp-credential-probe-... and keep using that generated path
through the curlArgs, quotedResponsePath, head, and cleanup steps in the probe
command builder. Make sure the unique temporary file is created before it is
referenced and removed afterward as part of the same flow.
---
Nitpick comments:
In `@docs/deployment/set-up-mcp-bridge.mdx`:
- Line 214: Reword the probe description in the MCP bridge docs to remove the
apparent contradiction: clarify that `--probe` forces probing for every server
in the multi-server status form, while the bare `mcp list` and `mcp status`
forms do not probe by default and therefore stay fast. Use the existing wording
around the probe flags and the `mcp list`/`mcp status` forms to make the default
behavior explicit.
- Around line 210-216: Update the MCP bridge docs to mention the CLI behavior in
this section: the `mcp status`/`mcp add` probe flags reject being used together,
so describe that `--probe` and `--no-probe` are mutually exclusive and the
command errors if both are passed. Also add the text-mode success wording for
the probe result, using the same `provider.credentialResolution`/`credential
resolution:` terminology already present, so readers can see that the successful
human-readable output is `credential resolution: verified`.
In `@docs/reference/commands-nemohermes.mdx`:
- Around line 1278-1286: Update the `mcp status` flag reference so it explicitly
documents that `--probe` and `--no-probe` cannot be used together and that the
command rejects this combination. Add this note in the flag table or nearby
explanatory text for the `nemohermes my-assistant mcp status` section, keeping
the wording aligned with the existing `--probe` and `--no-probe` descriptions.
In `@docs/reference/commands.mdx`:
- Around line 1600-1608: The command reference for mcp status is missing the
mutual-exclusion rule for the probe flags, so update the documentation in the
status command section to explicitly state that --probe and --no-probe cannot be
used together and will be rejected; add this as a note or a dedicated row
alongside the existing flag descriptions so the mcp status reference remains
authoritative.
In `@src/lib/actions/sandbox/mcp-bridge-status-resolution.test.ts`:
- Around line 246-280: The post-add test is bypassing the real add/commit path
by stubbing addRestart.addMcpBridge to a no-op, so it only checks probe wiring
against an already pre-registered bridge. Update the test to exercise the actual
addMcpBridge flow while mocking only the external host/process boundaries it
depends on, so reportAddCredentialResolution is verified in the context of a
real add of a new MCP server and the commit ordering remains covered.
🪄 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: af142c73-2fb7-47b1-8b77-2d965086ff35
📒 Files selected for processing (10)
docs/deployment/set-up-mcp-bridge.mdxdocs/reference/commands-nemohermes.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.test.tssrc/lib/actions/sandbox/mcp-bridge-resolution-probe.tssrc/lib/actions/sandbox/mcp-bridge-status-resolution.test.tssrc/lib/actions/sandbox/mcp-bridge-status.tssrc/lib/actions/sandbox/mcp-bridge.ts
|
All three review findings addressed in 503675e:
Round-3 CI is green (35 checks passing). |
cv
left a comment
cv
left a comment
There was a problem hiding this comment.
Security blocker: mcp status continues probing when the stored URL has failed current authenticated-endpoint validation. That can send an attached credential to a legacy, private, or HTTP endpoint. A 2xx, 401, or 403 response does not prove placeholder rewriting, and the response excerpt cannot reliably redact a persisted provider secret once its host env is absent. Gate probing on URL safety, use evidence that distinguishes resolved from unresolved credentials, and do not print untrusted authenticated response bodies.
|
@cv Thanks — all three points addressed in 7276fa2:
Docs updated to describe the differential semantics. Round-4 CI running. |
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 `@src/lib/actions/sandbox/mcp-bridge-resolution-probe.ts`:
- Around line 225-227: The equality check in mcpBridgeResolutionProbe should not
treat identical 5xx responses as a definite unresolved result. Update the
shared-response handling so identical auth-shaped rejections like 400/401/403
still return ok: false, but identical 5xx matches fall through to the
indeterminate path and return ok: null instead. Use the mcpBridgeResolutionProbe
logic that compares placeholder and control HTTP statuses, and add a test
covering the 500 case alongside the existing 400/401/403 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: 720e127b-9f98-4ade-8ee8-76f5cf07c8bd
📒 Files selected for processing (9)
docs/deployment/set-up-mcp-bridge.mdxdocs/reference/commands-nemohermes.mdxdocs/reference/commands.mdxsrc/lib/actions/sandbox/mcp-bridge-contracts.tssrc/lib/actions/sandbox/mcp-bridge-resolution-probe.test.tssrc/lib/actions/sandbox/mcp-bridge-resolution-probe.tssrc/lib/actions/sandbox/mcp-bridge-status-resolution.test.tssrc/lib/actions/sandbox/mcp-bridge-status.tssrc/lib/actions/sandbox/mcp-bridge.ts
✅ Files skipped from review due to trivial changes (1)
- docs/deployment/set-up-mcp-bridge.mdx
🚧 Files skipped from review as they are similar to previous changes (6)
- src/lib/actions/sandbox/mcp-bridge-contracts.ts
- src/lib/actions/sandbox/mcp-bridge-status-resolution.test.ts
- docs/reference/commands.mdx
- src/lib/actions/sandbox/mcp-bridge.ts
- docs/reference/commands-nemohermes.mdx
- src/lib/actions/sandbox/mcp-bridge-status.ts
cv
left a comment
cv
left a comment
There was a problem hiding this comment.
The follow-up resolves the unsafe persisted-URL probe and authenticated response-body leak. One core evidence blocker remains: identical resolved and bogus-control HTTP statuses are classified as definitive rewriting=false. A correctly rewritten but expired or revoked credential and the bogus control can both return 401 or 403; endpoint behavior can likewise produce identical 400, 404, or 5xx responses, so equality does not prove non-rewriting. Treat that outcome as inconclusive or use a probe with a response contract that actually distinguishes the credential values, and cover those cases. The exact-head CodeRabbit 5xx finding is the same issue.
TonyLuo-NV
force-pushed
the
fix/issue-6379-mcp-credential-resolution-probe
branch
from
7276fa2 to
010a290
Compare
|
Addressed the new CodeRabbit finding in 010a290: identical 5xx responses from both probes now classify as indeterminate ( Also rebased onto latest main (9fe4cb3): the round-4 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/lib/actions/sandbox/mcp-bridge-resolution-probe.ts (1)
104-164: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftKeep wire probing at the adapter boundary.
This action module owns curl command construction/runtime wrapping and calls
executeSandboxCommanddirectly. Move the wire probe runner/command-building boundary under adapters, leaving this module to orchestrate and classify. As per path instructions, “When updating MCP bridge status/probing, implement wire probing logic as an adapter boundary … orchestrate from actions, and keep classification logic pure where possible.”Also applies to: 263-273
🤖 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-resolution-probe.ts` around lines 104 - 164, The wire-probe logic is still being built and executed in this action module, which should only orchestrate and classify results. Move the curl command construction and runtime wrapping in quotedCurlCommand and buildCredentialResolutionProbeCommand into the adapter layer, and have this module call an adapter-provided probe runner instead of owning executeSandboxCommand-style behavior. Keep the action-level code focused on coordination, and preserve pure classification logic here with the adapter boundary handling the wire probing details.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 `@src/lib/actions/sandbox/mcp-bridge-resolution-probe.ts`:
- Around line 104-164: The wire-probe logic is still being built and executed in
this action module, which should only orchestrate and classify results. Move the
curl command construction and runtime wrapping in quotedCurlCommand and
buildCredentialResolutionProbeCommand into the adapter layer, and have this
module call an adapter-provided probe runner instead of owning
executeSandboxCommand-style behavior. Keep the action-level code focused on
coordination, and preserve pure classification logic here with the adapter
boundary handling the wire probing details.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: f7462223-e8bf-4402-ab28-4c8c330b4826
📒 Files selected for processing (10)
docs/deployment/set-up-mcp-bridge.mdxdocs/reference/commands-nemohermes.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.test.tssrc/lib/actions/sandbox/mcp-bridge-resolution-probe.tssrc/lib/actions/sandbox/mcp-bridge-status-resolution.test.tssrc/lib/actions/sandbox/mcp-bridge-status.tssrc/lib/actions/sandbox/mcp-bridge.ts
✅ Files skipped from review due to trivial changes (2)
- src/lib/actions/sandbox/mcp-bridge-render.ts
- docs/deployment/set-up-mcp-bridge.mdx
🚧 Files skipped from review as they are similar to previous changes (6)
- src/lib/actions/sandbox/mcp-bridge-contracts.ts
- src/lib/actions/sandbox/mcp-bridge-status.ts
- docs/reference/commands-nemohermes.mdx
- src/lib/actions/sandbox/mcp-bridge.ts
- docs/reference/commands.mdx
- src/lib/actions/sandbox/mcp-bridge-resolution-probe.test.ts
|
@cv Addressed in b962586 — the classifier no longer claims definitive non-rewriting from wire evidence:
On the alternative you mentioned — a response contract that truly distinguishes credential values needs either endpoint cooperation (auth echo) or OpenShell gateway introspection (e.g. the OCSF |
|
Note for the merge gate: the
|
cv
left a comment
cv
left a comment
There was a problem hiding this comment.
The identical-status false-negative is fixed, but the positive proof is still unsound. A placeholder 401 or 403 plus any different control status is currently reported as verified. On a broken gateway, the endpoint receives two different invalid literal bearer strings and may reject them differently, for example placeholder 401 and control 400, without rewriting either value. Reserve verified for placeholder 2xx with a rejected control; treat differing non-2xx results as inconclusive and add that regression case. For identical 400, include endpoint or request validation among the possible causes. The current acpx CI failure is inherited and fixed on main, but this proof issue remains PR-local.
NVIDIA#6379) Provider metadata checks (attached, credentialReady, adapter registered) cannot prove that OpenShell rewrites the openshell:resolve:env: placeholder on egress, so mcp status stayed all-green on hosts where the literal placeholder was sent to the MCP server and every agent request failed (NVIDIA/OpenShell#2161). mcp status <server> now sends one idempotent MCP initialize from inside the sandbox, wrapped in the adapter runtime so the generated protocol: mcp policy applies, carrying the placeholder authorization header exactly as agent traffic does. The verdict is reported as provider.credentialResolution in JSON and a credential resolution: line in text output, with a warning that names the OpenShell host defect on failure. mcp add runs the same probe after the add commits and warns without failing the durable transaction. --probe and --no-probe override the defaults; bare status and list never probe. Endpoint outages, policy denials, timeouts, and unreachable sandboxes classify as indeterminate so an endpoint problem is never blamed on the host's credential rewrite. Probe output is redacted before display. Signed-off-by: Tony Luo <xialuo@nvidia.com>
…A#6379) The test-conditional guardrail rejects new if statements in changed test files. Replace the stdout-fixture builder branches with spread expressions and the harness exit check with an expect assertion. Signed-off-by: Tony Luo <xialuo@nvidia.com>
…NVIDIA#6379) Per CodeRabbit review on the resolution probe: - HTTP 400 no longer classifies as a credential-resolution failure. A 400 can follow a correct rewrite when the endpoint rejects the initialize request itself, so blaming the OpenShell host would be a false accusation. It now reports ok: null with a detail naming both hypotheses (unresolved placeholder vs endpoint request validation) and recommending comparison against a known-good host. 401/403 remain hard failures. - The probe response body now lands in a mktemp path cleaned up via an EXIT trap instead of a predictable /tmp name derived from the server name, closing same-sandbox symlink races and parallel-probe clobbering. Signed-off-by: Tony Luo <xialuo@nvidia.com>
…gated (NVIDIA#6379) Address the security review on the wire-level probe: - Differential evidence: the probe now sends the MCP initialize twice — once with the placeholder header and once with a deliberately- unresolvable literal control bearer that no gateway rewrite can touch. Identical HTTP rejections for both requests indict the host's placeholder rewrite; an expired or revoked credential resolves on the wire and is rejected with a different status than the control, so it is reported as resolved-but-rejected instead of misblaming OpenShell. A 2xx for both means the endpoint does not enforce authentication and reports indeterminate. - No response bodies: curl writes to /dev/null and classification uses HTTP status and curl exit codes only, so untrusted authenticated endpoint output is never captured or printed and redaction no longer depends on the credential's host environment variable being present. - URL safety gate: the probe refuses to run when the persisted URL no longer satisfies the current authenticated-endpoint boundary, so a legacy, private-alias, or plain-HTTP endpoint is never sent a header the gateway could rewrite into a live credential. Signed-off-by: Tony Luo <xialuo@nvidia.com>
…A#6379) An endpoint that fails both the placeholder and control probes with the same 5xx only proves it failed identically twice, not that the placeholder went out unresolved. Identical auth-shaped 4xx rejections still indict the rewrite; identical 5xx now reports ok: null with the evidence. Signed-off-by: Tony Luo <xialuo@nvidia.com>
…VIDIA#6379) Identical HTTP statuses from the placeholder and control probes never prove non-rewriting: a correctly rewritten but expired or revoked credential and the bogus control can both draw the same 401/403, and an endpoint can reject or fail both probes the same way. The classifier no longer emits a definitive failure verdict from wire statuses; identical 4xx now reports ok: null with both hypotheses named, and the warning (status warnings array, add-tail stderr) tells the operator to verify the stored credential first and, only once it is confirmed valid, to treat the identical rejection as the host not rewriting placeholders. ok: false is reserved for future evidence sources that can actually prove non-rewriting. Signed-off-by: Tony Luo <xialuo@nvidia.com>
…trol (NVIDIA#6379) Differing non-2xx statuses prove nothing: a broken gateway forwards two different literal bearer strings and an endpoint may reject them differently (e.g. placeholder 401, control 400) without rewriting either. The verified verdict now requires a placeholder HTTP 2xx paired with a rejected control — the only outcome that proves a valid credential was on the wire. Differing rejections classify as inconclusive with the ambiguity named, and identical HTTP 400 adds endpoint request validation to the listed hypotheses. Regression tests cover the 401-vs-400 shape. Signed-off-by: Tony Luo <xialuo@nvidia.com>
TonyLuo-NV
force-pushed
the
fix/issue-6379-mcp-credential-resolution-probe
branch
from
198e01b to
b5fcfbe
Compare
|
@cv Fixed in dab4fdebb (branch also rebased onto main 6451f70 which carries the acpx repair):
|
cv
left a comment
cv
left a comment
There was a problem hiding this comment.
The classifier and detail text now handle differing non-2xx results correctly, but the user-visible credentialResolutionWarning path still misattributes identical 400 responses. For any identical 4xx it says that, if the stored credential is valid, the OpenShell host is not rewriting. A correctly rewritten credential and bogus control can both receive request-validation 400, and post-add emits only this warning rather than the safer detail. Make the identical-400 warning explicitly inconclusive and include endpoint/request validation as a hypothesis, then test the actual mcp status and post-add warning output rather than only the classifier detail.
…DIA#6379) The identical-4xx operator warning claimed that a confirmed-valid credential implies the host is not rewriting placeholders. That inference only holds for auth-shaped rejections: a rewritten valid credential and the bogus control can both draw a request-validation 400, so for identical 400 the warning now stays explicitly inconclusive, lists the request-validation hypothesis, and points at a known-good-host comparison instead. New tests assert the actual mcp status warnings array and post-add stderr output for the 400 shape, not just the classifier detail. Signed-off-by: Tony Luo <xialuo@nvidia.com>
|
cv
left a comment
cv
left a comment
There was a problem hiding this comment.
The identical-400 warning gap is resolved on this head. One security blocker remains unchanged: after sourcing /tmp/nemoclaw-proxy-env.sh, the probe still launches the adapter and curl children with OPENCLAW_GATEWAY_TOKEN in their environment. Unset that and any equivalent sensitive runtime variables immediately after sourcing, before the first child process, and cover the command-ordering invariant. The trusted advisor finding was produced on the immediately preceding head, but this commit does not touch probe command construction, so it still applies.
…VIDIA#6379) Sourcing /tmp/nemoclaw-proxy-env.sh can export OPENCLAW_GATEWAY_TOKEN, OPENCLAW_GATEWAY_URL/PORT, and the insecure-private-WS break-glass toggles alongside the proxy variables the probe needs. The probe script now unsets that sanitize set (the same one nemoclaw-start applies to un-managed openclaw children) immediately after sourcing and before the first child process, so neither the adapter runtime nor curl inherits gateway credentials. A test pins the source -> unset -> first-child ordering invariant for all three adapters. Signed-off-by: Tony Luo <xialuo@nvidia.com>
|
@cv Addressed in the latest commit: the probe script now unsets |
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
|
Maintainer review (manual — the automated PR Review Advisor skipped because this is a fork PR with no secrets access): Reviewed the wire-level credential-resolution probe for
Merge posture: Signed-off-by: Prekshi Vyas prekshiv@nvidia.com |
wscurran
added
area: cli
<!-- markdownlint-disable MD041 --> ## Summary Prepares the user-facing documentation for NemoClaw v0.0.76 and closes the release-prep documentation gate. It adds the release highlights, documents the arm64 Local NIM warning and expanded image cleanup behavior, and fixes agent-specific command headings in generated guides. ## Changes - Add the v0.0.76 release-notes section and move the shared-gateway route containment entry out of the v0.0.74 history where it was incorrectly placed. - Document the advisory Linux arm64 Local NIM manifest warning in the canonical platform matrix and local-inference guidance. - Document that `gc` scans both gateway-built and locally prebuilt sandbox image repositories. - Keep OpenClaw and Hermes session headings out of the generated Deep Agents command guide. - Add a focused variant regression test for the agent-specific session headings. ### Source summary | Merged sources | Documentation coverage | | --- | --- | | [#6414](#6414), [#6418](#6418), [#6416](#6416), [#6344](#6344) | v0.0.76 release notes and the Deep Agents quickstart/inference routes | | [#6340](#6340) | v0.0.76 release notes and existing Deep Agents observability guidance | | [#6338](#6338), [#6378](#6378), [#6297](#6297) | v0.0.76 release notes and existing inference/troubleshooting guidance | | [#6362](#6362) | v0.0.76 release notes and existing lifecycle, command, and credential guidance | | [#6330](#6330), [#6307](#6307), [#6008](#6008) | v0.0.76 release notes and existing security, troubleshooting, and command guidance | | [#6382](#6382) | v0.0.76 release notes and existing MCP/command guidance | | [#6326](#6326), [#5868](#5868), [#5539](#5539) | v0.0.76 release notes, platform matrix, inference options, and local-inference guidance | | [#6396](#6396), [#6390](#6390), [#6007](#6007) | v0.0.76 release notes and existing messaging guidance | | [#5388](#5388), [#6249](#6249), [#6303](#6303), [#6306](#6306) | v0.0.76 release notes and command/lifecycle guidance | ## 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: - [ ] 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: - [ ] Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue: ## Verification - [x] PR description includes the DCO sign-off declaration 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 --project integration test/generate-platform-docs.test.ts test/agent-variant-docs.test.ts test/sync-agent-variant-docs.test.ts` (3 files, 29 tests passed) - [ ] Applicable broad gate passed — `npm test` for broad runtime/test-harness changes; `npm run check` for repo-wide validation/coverage changes — command/result: - [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) — completed with 0 errors and 2 pre-existing Fern warnings - [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: Apurv Kumaria <akumaria@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Documentation** * Added v0.0.76 release notes content, and removed an older conflicting bullet from the surrounding release history. * Expanded Local NVIDIA NIM guidance across inference/provider docs, including an advisory for Linux arm64 DGX Spark/DGX Station hosts when a matching `linux/arm64` image manifest is unavailable. * Updated the command reference for correct session-section rendering and clarified `gc` image cleanup sources. * **Tests** * Added coverage ensuring Deep Agents omits sessions headings while Hermes includes them. * **CI** * Refreshed Local NVIDIA NIM provider notes used in the platform matrix. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
NVIDIA#6379) (NVIDIA#6382) <!-- markdownlint-disable MD041 --> ## Summary `mcp status` could report all-green provider metadata while OpenShell never rewrote the `openshell:resolve:env:` credential placeholder on egress, so every agent request failed with the literal placeholder as the bearer token and the managed MCP server was silently skipped. This PR adds a wire-level credential-resolution probe to `mcp status` and `mcp add` so that failure mode is loud and attributable. ## Related Issue Fixes the NemoClaw-side diagnostic gap of NVIDIA#6379 (keep that issue open until the upstream fix lands). Full chain for reviewers: - **Bug report (NemoClaw)**: NVIDIA#6379 — managed MCP unusable at runtime, `mcp status` stays all-green - **This PR (NemoClaw, diagnostic surface)**: wire-level differential credential-resolution probe in `mcp status` / `mcp add` - **Root-cause issue (upstream)**: NVIDIA/OpenShell#2161 — gateway never rewrites the `openshell:resolve:env:` placeholder on egress - **Root-cause fix (upstream)**: NVIDIA/OpenShell#2162 — fail closed when credential placeholders cannot be rewritten ## Changes - New `src/lib/actions/sandbox/mcp-bridge-resolution-probe.ts`: builds an in-sandbox MCP `initialize` probe (curl wrapped in the adapter runtime so the generated `protocol: mcp` policy attributes it to the adapter binary ancestry, same construction as the live E2E DNS-rebinding probe), classifies the outcome, and renders the operator warning. All output is redacted via `redactBridgeSecretsForDisplay`. - `statusMcpBridge` gains a `probeCredentialResolution` option; the verdict is surfaced as `provider.credentialResolution` (`ok: true | false | null`, `httpStatus`, `detail`) plus a warning naming the OpenShell host defect on failure. The probe shares the existing legacy-credential skip gate. - `mcp status <server>` probes by default; bare `mcp status` and `mcp list` never probe; `--probe` / `--no-probe` override both directions and combining them is rejected. - `mcp add` runs the probe once after the durable add commits and prints a loud warning on failure without failing the add (exit code stays 0; `--no-probe` skips). - Classification never blames the credential rewrite for endpoint problems: HTTP 2xx = resolved, 400/401/403 = resolution failure, 404/5xx/CONNECT-403/timeout/unreachable = indeterminate with detail. - Human-readable status renders a `credential resolution: verified / FAILED / unknown` line; help text documents the new flags. - Docs: `docs/deployment/set-up-mcp-bridge.mdx` (operate + troubleshooting) and `docs/reference/commands.mdx` (+ synced `commands-nemohermes.mdx`). ## Type of Change - [x] Code change with doc updates ## Quality Gates - [x] Tests added or updated for changed behavior - [x] Docs updated for user-facing behavior changes - [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: requesting maintainer review on this PR ## Verification - [x] PR description includes the DCO sign-off declaration 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: `npx vitest run --project cli src/lib/actions/sandbox/mcp-bridge-resolution-probe.test.ts src/lib/actions/sandbox/mcp-bridge-status-resolution.test.ts` → 22/22 passed; full `src/lib/actions/sandbox/` CLI lane → 1291 passed (3 unrelated pre-existing local-env failures: two 5s-timeout flakes that pass standalone, one host-python missing `yaml`); `npx vitest run --project integration test/mcp-add-crash-consistency.test.ts test/mcp-bridge-servers.test.ts test/mcp-openshell-workflow.test.ts test/mcp-provider-ownership.test.ts test/mcp-artifact-secret-scan.test.ts` → 41/41 passed; `npm run typecheck:cli` clean - [x] Quality Gates section completed with required justifications or waivers - [x] No secrets, API keys, or credentials committed - [x] `npm run docs` builds without warnings (doc changes only) Signed-off-by: Tony Luo <xialuo@nvidia.com> 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added optional wire-level credential-resolution probing for MCP `status` (runs by default when exactly one server is named). * Added `--probe` / `--no-probe` support for `add` and `status`, including a post-`add` verification that warns on failure without failing the command. * `status` now can display a “credential resolution” line with HTTP indicators. * **Bug Fixes** * Improved verdict semantics and safer probing/skipping rules (including “unknown” vs “FAILED” outcomes). * **Tests** * Added coverage for probe generation, classification, and CLI flag behavior. * **Documentation** * Updated `mcp`, Hermes/NemoClaw, and troubleshooting docs to reflect the new probe reporting and guidance. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Tony Luo <xialuo@nvidia.com> Signed-off-by: Carlos Villela <cvillela@nvidia.com> Co-authored-by: Carlos Villela <cvillela@nvidia.com>
<!-- markdownlint-disable MD041 --> ## Summary Prepares the user-facing documentation for NemoClaw v0.0.76 and closes the release-prep documentation gate. It adds the release highlights, documents the arm64 Local NIM warning and expanded image cleanup behavior, and fixes agent-specific command headings in generated guides. ## Changes - Add the v0.0.76 release-notes section and move the shared-gateway route containment entry out of the v0.0.74 history where it was incorrectly placed. - Document the advisory Linux arm64 Local NIM manifest warning in the canonical platform matrix and local-inference guidance. - Document that `gc` scans both gateway-built and locally prebuilt sandbox image repositories. - Keep OpenClaw and Hermes session headings out of the generated Deep Agents command guide. - Add a focused variant regression test for the agent-specific session headings. ### Source summary | Merged sources | Documentation coverage | | --- | --- | | [NVIDIA#6414](NVIDIA#6414), [NVIDIA#6418](NVIDIA#6418), [NVIDIA#6416](NVIDIA#6416), [NVIDIA#6344](NVIDIA#6344) | v0.0.76 release notes and the Deep Agents quickstart/inference routes | | [NVIDIA#6340](NVIDIA#6340) | v0.0.76 release notes and existing Deep Agents observability guidance | | [NVIDIA#6338](NVIDIA#6338), [NVIDIA#6378](NVIDIA#6378), [NVIDIA#6297](NVIDIA#6297) | v0.0.76 release notes and existing inference/troubleshooting guidance | | [NVIDIA#6362](NVIDIA#6362) | v0.0.76 release notes and existing lifecycle, command, and credential guidance | | [NVIDIA#6330](NVIDIA#6330), [NVIDIA#6307](NVIDIA#6307), [NVIDIA#6008](NVIDIA#6008) | v0.0.76 release notes and existing security, troubleshooting, and command guidance | | [NVIDIA#6382](NVIDIA#6382) | v0.0.76 release notes and existing MCP/command guidance | | [NVIDIA#6326](NVIDIA#6326), [NVIDIA#5868](NVIDIA#5868), [NVIDIA#5539](NVIDIA#5539) | v0.0.76 release notes, platform matrix, inference options, and local-inference guidance | | [NVIDIA#6396](NVIDIA#6396), [NVIDIA#6390](NVIDIA#6390), [NVIDIA#6007](NVIDIA#6007) | v0.0.76 release notes and existing messaging guidance | | [NVIDIA#5388](NVIDIA#5388), [NVIDIA#6249](NVIDIA#6249), [NVIDIA#6303](NVIDIA#6303), [NVIDIA#6306](NVIDIA#6306) | v0.0.76 release notes and command/lifecycle guidance | ## 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: - [ ] 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: - [ ] Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue: ## Verification - [x] PR description includes the DCO sign-off declaration 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 --project integration test/generate-platform-docs.test.ts test/agent-variant-docs.test.ts test/sync-agent-variant-docs.test.ts` (3 files, 29 tests passed) - [ ] Applicable broad gate passed — `npm test` for broad runtime/test-harness changes; `npm run check` for repo-wide validation/coverage changes — command/result: - [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) — completed with 0 errors and 2 pre-existing Fern warnings - [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: Apurv Kumaria <akumaria@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Documentation** * Added v0.0.76 release notes content, and removed an older conflicting bullet from the surrounding release history. * Expanded Local NVIDIA NIM guidance across inference/provider docs, including an advisory for Linux arm64 DGX Spark/DGX Station hosts when a matching `linux/arm64` image manifest is unavailable. * Updated the command reference for correct session-section rendering and clarified `gc` image cleanup sources. * **Tests** * Added coverage ensuring Deep Agents omits sessions headings while Hermes includes them. * **CI** * Refreshed Local NVIDIA NIM provider notes used in the platform matrix. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
4 participants
Summary
mcp statuscould report all-green provider metadata while OpenShell never rewrote theopenshell:resolve:env:credential placeholder on egress, so every agent request failed with the literal placeholder as the bearer token and the managed MCP server was silently skipped. This PR adds a wire-level credential-resolution probe tomcp statusandmcp addso that failure mode is loud and attributable.Related Issue
Fixes the NemoClaw-side diagnostic gap of #6379 (keep that issue open until the upstream fix lands).
Full chain for reviewers:
mcp statusstays all-greenmcp status/mcp addopenshell:resolve:env:placeholder on egressChanges
src/lib/actions/sandbox/mcp-bridge-resolution-probe.ts: builds an in-sandbox MCPinitializeprobe (curl wrapped in the adapter runtime so the generatedprotocol: mcppolicy attributes it to the adapter binary ancestry, same construction as the live E2E DNS-rebinding probe), classifies the outcome, and renders the operator warning. All output is redacted viaredactBridgeSecretsForDisplay.statusMcpBridgegains aprobeCredentialResolutionoption; the verdict is surfaced asprovider.credentialResolution(ok: true | false | null,httpStatus,detail) plus a warning naming the OpenShell host defect on failure. The probe shares the existing legacy-credential skip gate.mcp status <server>probes by default; baremcp statusandmcp listnever probe;--probe/--no-probeoverride both directions and combining them is rejected.mcp addruns the probe once after the durable add commits and prints a loud warning on failure without failing the add (exit code stays 0;--no-probeskips).credential resolution: verified / FAILED / unknownline; help text documents the new flags.docs/deployment/set-up-mcp-bridge.mdx(operate + troubleshooting) anddocs/reference/commands.mdx(+ syncedcommands-nemohermes.mdx).Type of Change
Quality Gates
Verification
Verifiedin GitHubpre-commit,commit-msg, andpre-pushhooks passed, ornpm run check:diffpassed when hooks were skipped or unavailablenpx vitest run --project cli src/lib/actions/sandbox/mcp-bridge-resolution-probe.test.ts src/lib/actions/sandbox/mcp-bridge-status-resolution.test.ts→ 22/22 passed; fullsrc/lib/actions/sandbox/CLI lane → 1291 passed (3 unrelated pre-existing local-env failures: two 5s-timeout flakes that pass standalone, one host-python missingyaml);npx vitest run --project integration test/mcp-add-crash-consistency.test.ts test/mcp-bridge-servers.test.ts test/mcp-openshell-workflow.test.ts test/mcp-provider-ownership.test.ts test/mcp-artifact-secret-scan.test.ts→ 41/41 passed;npm run typecheck:clicleannpm run docsbuilds without warnings (doc changes only)Signed-off-by: Tony Luo xialuo@nvidia.com
🤖 Generated with Claude Code
Summary by CodeRabbit
status(runs by default when exactly one server is named).--probe/--no-probesupport foraddandstatus, including a post-addverification that warns on failure without failing the command.statusnow can display a “credential resolution” line with HTTP indicators.mcp, Hermes/NemoClaw, and troubleshooting docs to reflect the new probe reporting and guidance.