fix(cli): reject MCP credential collisions before rebuild (#9401)

<!-- markdownlint-disable MD041 -->
## Summary

Sandbox rebuild now rejects an attached provider that supplies a
credential key reserved by managed MCP before changing MCP state. Forced
host-side recovery repeats the collision check before sandbox deletion.
The credential guide documents recovery that preserves the provider and
stored credential.

## Related Issue

Fixes #9388

## Changes

- Run the attached-provider credential collision check during ordinary
live-sandbox rebuild preparation.
- Repeat the collision check before forced host-side recovery deletes
the sandbox.
- Add regression tests for ordinary rebuild, forced recovery, and a
collision introduced after preflight.
- Verify collision failures do not detach providers or change policy,
agent adapter, or registry state.
- Document sandbox-scoped provider detach recovery and the destructive
scope of `credentials reset`.
- Record the completed documentation writer review: `docs-updated`, with
no actionable findings.

## 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] 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: The implementation
review covered credential custody, attachment ownership, fail-closed
metadata errors, rollback boundaries, and secret handling. Regression
tests verify collision failures do not mutate managed MCP state.
- [ ] Non-success, skipped, or missing CI check accepted by maintainer —
check name, approval link, and follow-up issue:

## 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 validate:pr` passed after refreshing `origin/main` 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 integration
test/mcp-destroy-lifecycle.test.ts
test/mcp-add-crash-consistency.test.ts
test/mcp-restart-policy-order.test.ts` — 62 tests passed.
- `npx vitest run --project cli
src/lib/actions/sandbox/mcp-bridge-provider.test.ts` — 18 tests passed.
  - `npm run test:changed` — 572 tests and 22 growth checks 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)
- [ ] 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)
- Documentation validation: `npm run docs:sync-agent-variants` and `npm
run docs` passed. Fern reported 0 errors and 2 warnings.

---
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>


<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit

* **Bug Fixes**
* Prevented sandbox rebuilds, restarts, and provider additions when
attached providers have credential-key conflicts.
* Prevented partial changes to providers, policies, adapters, or
sandboxes while preserving conflicting provider attachments.
* Added consistent collision checks across standard, host-side, and
recovery flows.
* Updated recovery guidance to detach the conflicting provider before
retrying; credential resets warn that stored credentials and provider
access are removed globally.

* **Documentation**
  * Documented credential-collision handling and recovery steps.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->

---------

Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
Signed-off-by: Senthil Ravichandran <senthilr@nvidia.com>
Co-authored-by: Senthil Ravichandran <senthilr@nvidia.com>

Author: apurvvkumaria

docs/security/credential-storage.mdx

@@ -121,6 +121,19 @@ Use this precedence to:
 
 Managed MCP is an exception: `$$nemoclaw <name> mcp add` always creates and attaches an OpenShell provider, and `--env KEY` supplies only the transient input value.
 For that credential boundary, refer to [About Managed MCP Servers](../manage-sandboxes/mcp-servers/about-managed-mcp-servers).
+Before an ordinary live-sandbox rebuild or forced host-side recovery changes managed MCP state, NemoClaw compares the credential keys for every provider attached to the sandbox.
+If another provider supplies a credential key that a managed MCP server reserves, rebuild stops before it changes the managed provider attachment, generated policy, or agent adapter.
+The collision check does not delete either provider or its stored credential value.
+Forced host-side recovery repeats the check before sandbox deletion.
+Detach only the conflicting provider from the affected sandbox:
+
+```bash
+openshell sandbox provider detach <sandbox-name> <provider-name>
+```
+
+This command keeps the provider and stored credential in OpenShell and does not change its attachments to other sandboxes.
+Rerun the original rebuild command.
+Do not run `$$nemoclaw credentials reset <PROVIDER_NAME>` unless you intend to detach that provider from every sandbox and delete its stored credential from OpenShell.
 
 When the host environment is empty, day-two operations such as `$$nemoclaw <name> rebuild` and remote-provider updates can reuse the credential already registered with the OpenShell gateway.
 Export the credential only when you want to create, replace, or rotate the stored provider value.

src/lib/actions/sandbox/mcp-bridge-add-restart.ts

@@ -31,7 +31,7 @@ import {
 } from "./mcp-bridge-policy";
 import {
   assertMcpProviderRecoverable,
-  assertNoAttachedProviderCredentialCollision,
+  assertNoAttachedProviderCredentialCollisions,
   attachProvider,
   deleteProvider,
   detachMissingProviderReference,
@@ -372,7 +372,7 @@ async function addMcpBridgeUnlocked(
     // Credential keys are sandbox-global. Prove this key is not already
     // supplied by a foreign attachment before opening its MCP route, then check
     // again after provider creation to close the intervening race.
-    assertNoAttachedProviderCredentialCollision(sandboxName, entry);
+    assertNoAttachedProviderCredentialCollisions(sandboxName, [entry]);
     // Loading the real protocol:mcp policy with --wait is the authoritative
     // running-supervisor capability check. Do it before any host credential is
     // created or updated so unsupported runtimes fail without that side effect.
@@ -407,7 +407,7 @@ async function addMcpBridgeUnlocked(
       // adapter mutations. A process death before this write fails closed.
       writeBridgeEntry(sandboxName, entry);
     }
-    assertNoAttachedProviderCredentialCollision(sandboxName, entry);
+    assertNoAttachedProviderCredentialCollisions(sandboxName, [entry]);
     if (providerResult.action === "updated" && previousCredentialRevision === undefined) {
       throw new McpBridgeError(
         `Could not retain the prior OpenShell credential revision for provider '${entry.providerName}'.`,

src/lib/actions/sandbox/mcp-bridge-input-targets.test.ts

@@ -182,7 +182,7 @@ replace(adapters, "registerAgentAdapter", () => {});
 replace(policy, "applyGeneratedPolicy", (_sandbox, _entry, target) => { admittedTarget = target; });
 replace(state, "ensureSandboxGatewaySelected", async () => {});
 replace(validation, "assertMcpCredentialBoundaryRuntimeVersion", () => {});
-replace(provider, "assertNoAttachedProviderCredentialCollision", () => {});
+replace(provider, "assertNoAttachedProviderCredentialCollisions", () => {});
 replace(provider, "inspectMcpProvider", () => ({
   credentialKeys: null, exists: false, id: null, resourceVersion: null, type: null,
 }));

src/lib/actions/sandbox/mcp-bridge-provider-inspection.ts

@@ -164,26 +164,29 @@ export function inspectMcpProviderAttachments(
   }
 }
 
-export function assertNoAttachedProviderCredentialCollision(
+export function assertNoAttachedProviderCredentialCollisions(
   sandboxName: string,
-  entry: McpBridgeEntry,
+  entries: readonly McpBridgeEntry[],
 ): void {
+  if (entries.length === 0) return;
   const inspection = inspectMcpProviderAttachments(sandboxName);
   if (!inspection.attachments) {
     throw new McpBridgeError(
       inspection.error ?? `Could not inspect providers attached to sandbox '${sandboxName}'.`,
     );
   }
-  const credentialKey = entry.env[0];
-  const collision = inspection.attachments.find(
-    (attachment) =>
-      attachment.credentialKeys.includes(credentialKey) &&
-      !(attachment.name === entry.providerName && attachment.providerId === entry.providerId),
-  );
-  if (collision) {
-    throw new McpBridgeError(
-      `Credential key '${credentialKey}' is already supplied by attached provider '${collision.name}' with ID '${collision.providerId ?? "missing"}'. Refusing to reserve the key for MCP before provider activation.`,
+  for (const entry of entries) {
+    const credentialKey = entry.env[0];
+    const collision = inspection.attachments.find(
+      (attachment) =>
+        attachment.credentialKeys.includes(credentialKey) &&
+        !(attachment.name === entry.providerName && attachment.providerId === entry.providerId),
     );
+    if (collision) {
+      throw new McpBridgeError(
+        `Credential key '${credentialKey}' is already supplied by attached provider '${collision.name}' with ID '${collision.providerId ?? "missing"}'. Refusing to continue managed MCP while this sandbox receives that key from another provider.`,
+      );
+    }
   }
 }
 

src/lib/actions/sandbox/mcp-bridge-provider.ts

@@ -8,7 +8,7 @@ export type {
 } from "./mcp-bridge-provider-inspection";
 export {
   assertMcpProviderRecoverable,
-  assertNoAttachedProviderCredentialCollision,
+  assertNoAttachedProviderCredentialCollisions,
   inspectMcpProvider,
   inspectMcpProviderAttachments,
   parseMcpProviderAttachmentNames,

src/lib/actions/sandbox/mcp-bridge-rebuild-exec-unavailable.ts

@@ -11,7 +11,10 @@ import {
   inspectExactMcpDestroyProvider,
 } from "./mcp-bridge-destroy-preflight";
 import { assertGeneratedPolicyExactReadOnly } from "./mcp-bridge-policy";
-import { preflightMcpEntryTargets } from "./mcp-bridge-provider";
+import {
+  assertNoAttachedProviderCredentialCollisions,
+  preflightMcpEntryTargets,
+} from "./mcp-bridge-provider";
 import {
   assertMcpDestroyNotPending,
   bridgeState,
@@ -162,6 +165,7 @@ async function inspectReadOnlyRecoveryState(
     providerByServer.set(entry.server, providerFingerprint(provider));
     targetsByServer.set(entry.server, targetFingerprint(target));
   }
+  assertNoAttachedProviderCredentialCollisions(sandboxName, entries);
   return { policyByServer, providerByServer, targetsByServer };
 }
 

src/lib/actions/sandbox/mcp-bridge-rebuild.ts

@@ -18,6 +18,7 @@ import {
 } from "./mcp-bridge-policy";
 import {
   assertMcpProviderRecoverable,
+  assertNoAttachedProviderCredentialCollisions,
   attachProvider,
   detachProvider,
   preflightMcpEntryTargets,
@@ -129,6 +130,7 @@ export async function prepareMcpBridgesForRebuild(
   for (const entry of entries) assertGeneratedPolicyMutationSafe(sandboxName, entry);
   assertMcpAdapterTeardownRuntimeCapabilities(sandboxName, sandbox, entries);
   for (const entry of entries) assertMcpProviderRecoverable(entry);
+  assertNoAttachedProviderCredentialCollisions(sandboxName, entries);
   const detached: McpBridgeEntry[] = [];
   const scrubbedAdapters: McpBridgeEntry[] = [];
   try {

src/lib/actions/sandbox/mcp-bridge-restart.ts

@@ -10,7 +10,7 @@ import { assertHermesMcpRuntimeIntent } from "./mcp-bridge-hermes-reconciliation
 import { applyGeneratedPolicy, assertGeneratedPolicyMutationSafe } from "./mcp-bridge-policy";
 import {
   assertMcpProviderRecoverable,
-  assertNoAttachedProviderCredentialCollision,
+  assertNoAttachedProviderCredentialCollisions,
   attachProvider,
   detachMissingProviderReference,
   type McpCredentialRevisionObservation,
@@ -117,6 +117,10 @@ async function restartMcpBridgeUnlocked(sandboxName: string, server?: string): P
   for (const entry of missingProviderEntries) {
     waitForDetachedMcpCredential(sandboxName, entry);
   }
+  // Reject a collision on any target before the first policy/provider/adapter
+  // mutation. The per-entry checks below still close races at each mutation
+  // edge without allowing a later target to fail after an earlier update.
+  assertNoAttachedProviderCredentialCollisions(sandboxName, targetEntries);
   for (const [name, storedEntry] of targets) {
     // Validated as a complete authenticated entry before gateway side effects.
     if (!storedEntry) continue;
@@ -125,7 +129,7 @@ async function restartMcpBridgeUnlocked(sandboxName: string, server?: string): P
     const adapterEnvValues = resolveCredentialEnv(envRefs);
     const target = resolvedTargetPins(resolvedByServer, entry);
     let previousCredentialRevision: McpCredentialRevisionObservation | undefined;
-    assertNoAttachedProviderCredentialCollision(sandboxName, entry);
+    assertNoAttachedProviderCredentialCollisions(sandboxName, [entry]);
     // Revalidate the actual running supervisor before rotating, recreating,
     // attaching, or re-registering an authenticated provider.
     applyGeneratedPolicy(sandboxName, entry, target);
@@ -152,7 +156,7 @@ async function restartMcpBridgeUnlocked(sandboxName: string, server?: string): P
       writeBridgeEntry(sandboxName, refreshedEntry);
       entry = refreshedEntry;
     }
-    assertNoAttachedProviderCredentialCollision(sandboxName, entry);
+    assertNoAttachedProviderCredentialCollisions(sandboxName, [entry]);
     if (providerResult.action === "updated" && previousCredentialRevision === undefined) {
       throw new McpBridgeError(
         `Could not retain the prior OpenShell credential revision for provider '${entry.providerName}'.`,
@@ -213,7 +217,13 @@ export async function restoreExistingMcpBridgeRuntime(
         `OpenShell provider '${entry.providerName}' is missing. Runtime restoration refuses to create or rotate credentials; run explicit MCP restart after exporting '${entry.env[0]}'.`,
       );
     }
-    assertNoAttachedProviderCredentialCollision(sandboxName, entry);
+  }
+  // Prove every restored entry is collision-free before the first mutation.
+  // The singleton check in the mutation loop still closes the race for each
+  // entry immediately before its policy and attachment are restored.
+  assertNoAttachedProviderCredentialCollisions(sandboxName, entries);
+  for (const entry of entries) {
+    assertNoAttachedProviderCredentialCollisions(sandboxName, [entry]);
     applyGeneratedPolicy(sandboxName, entry, resolvedTargetPins(resolvedByServer, entry));
     attachProvider(sandboxName, entry);
     waitForAttachedMcpCredential(sandboxName, entry);