diff --git a/ci/e2e-assertion-budget.json b/ci/e2e-assertion-budget.json index c45189e4179..01ce6fbf7d0 100644 --- a/ci/e2e-assertion-budget.json +++ b/ci/e2e-assertion-budget.json @@ -15,14 +15,14 @@ "testFileCount": 78, "liveFileCount": 189, "direct": { - "expectCalls": 1417, - "matcherAssertions": 1396, + "expectCalls": 1412, + "matcherAssertions": 1391, "nodeAssertions": 103, - "namedAssertionHelpers": 470, + "namedAssertionHelpers": 468, "failCalls": 0, "throwGuards": 71, - "objectFieldAssertions": 186, - "assertionPoints": 2226, + "objectFieldAssertions": 176, + "assertionPoints": 2209, "generatedProbeBlocks": 96, "generatedProbeConditions": 250 }, @@ -82,7 +82,7 @@ "test/e2e/live/managed-image-activation-e2e.test.ts": [0,0,13,30,0], "test/e2e/live/managed-image-multiarch-startup.test.ts": [1,1,1,7,0], "test/e2e/live/managed-image-protected-runtime.test.ts": [0,0,42,79,15], - "test/e2e/live/mcp-bridge.test.ts": [48,164,194,499,46], + "test/e2e/live/mcp-bridge.test.ts": [43,147,194,499,46], "test/e2e/live/messaging-compatible-endpoint.test.ts": [13,19,13,28,2], "test/e2e/live/messaging-providers.test.ts": [0,11,6,51,12], "test/e2e/live/model-router-provider-routed-inference.test.ts": [1,2,1,2,0], diff --git a/docs/deployment/set-up-mcp-bridge.mdx b/docs/deployment/set-up-mcp-bridge.mdx index 37af4ef1d53..0455b3b447b 100644 --- a/docs/deployment/set-up-mcp-bridge.mdx +++ b/docs/deployment/set-up-mcp-bridge.mdx @@ -103,6 +103,8 @@ OpenShell withholds the credential before the binding is active and outside the Credential keys must still be unique across providers attached to the same sandbox. NemoClaw rejects managed MCP credential-key reuse and requires a unique least-privilege token and environment key for each server. +It also rejects credential-bound definitions for the same normalized endpoint URL when their credential bindings differ, because OpenShell cannot choose between credentials for otherwise indistinguishable requests. +Entries that intentionally share one credential binding are not ambiguous; otherwise, keep one managed definition for that URL or use distinct endpoint URLs. A managed MCP provider created by an earlier NemoClaw build can have the profile-less `generic` type and cannot satisfy this binding contract. NemoClaw refuses to restart or rebuild that registration. @@ -123,7 +125,9 @@ This does not expose the raw credential to the sandbox before the request is aut ## Agent Adapters -OpenClaw writes the native `openclaw.json` `mcp.servers` map in the sandbox. Older managed Mcporter entries are accepted only by the explicit `mcp migrate` path. +OpenClaw writes the native `openclaw.json` `mcp.servers` map in the sandbox. +Use `mcp migrate --apply` to convert older managed Mcporter entries to native configuration. +To discard an entry instead, follow [Remove a Server](manage-mcp-servers#remove-a-server); removal requires proven registry ownership. Hermes writes managed HTTP entries under `/sandbox/.hermes/config.yaml`. @@ -158,7 +162,8 @@ The command carries no MCP traffic or raw service credential, and its payload co -OpenClaw stores MCP servers in the native `openclaw.json` `mcp.servers` map. Run `nemoclaw mcp migrate` before the first mutation when an older sandbox still contains a managed Mcporter entry. The migration preview calls out that materializing the native entry activates behavior OpenClaw previously did not load. +Before converting an older managed Mcporter entry, run `nemoclaw mcp migrate` to preview the change. +The preview explains that creating the native entry activates behavior OpenClaw previously did not load. diff --git a/docs/manage-sandboxes/add-mcp-server.mdx b/docs/manage-sandboxes/add-mcp-server.mdx index 7539af90bc3..33f5da16957 100644 --- a/docs/manage-sandboxes/add-mcp-server.mdx +++ b/docs/manage-sandboxes/add-mcp-server.mdx @@ -24,7 +24,10 @@ A legacy image stops with migration guidance before it changes a live provider, Hermes stores MCP servers in its native `config.yaml`. Registration and inspection preserve the stable credential handle from the readiness check in the OpenShell resolver placeholder without comparing it with host-side desired state. -OpenClaw stores MCP servers in `openclaw.json` under `mcp.servers`. Existing Mcporter entries require the explicit migration described in [Manage MCP Servers](manage-mcp-servers#migrate-an-existing-registration). +OpenClaw stores MCP servers in `openclaw.json` under `mcp.servers`. +To continue using or updating an existing Mcporter entry, follow [Migrate an Existing Registration](manage-mcp-servers#migrate-an-existing-registration). +To remove an entry instead, `$$nemoclaw my-sandbox mcp remove ` supports direct removal when its committed registry row proves ownership and the removal preconditions are met. +Refer to [Remove a Server](manage-mcp-servers#remove-a-server). An interrupted add is retried from the current agent and OpenShell sources. A deterministic provider name lets the retry recognize a partially created provider without a host-side transaction marker. The retry resumes only when the native entry, generated policy, provider identity and credential key, and sandbox attachment form an exact prefix of the requested add; conflicting or ambiguous resources remain unchanged and stop the command. @@ -168,6 +171,8 @@ Use a dedicated service name such as `MY_SERVICE_MCP_TOKEN`. NemoClaw requires exactly one `--env` bearer credential per server. Use a distinct environment variable for each managed server in the same sandbox because credential keys must be unique across attached providers. NemoClaw binds each dedicated provider to its generated MCP endpoint, so OpenShell resolves the placeholder only for that endpoint's host, port, and path. +One normalized endpoint URL can have only one credential-bound managed server definition in a sandbox. +If two definitions need different credentials, retain one definition for that URL or configure distinct endpoint URLs so OpenShell can select one credential unambiguously. If this server uses the profile-less `generic` provider from an earlier NemoClaw build, remove it and add it again with the credential exported. That provider cannot be bound to an endpoint, so restart and rebuild fail closed until you recreate the registration. diff --git a/docs/manage-sandboxes/manage-mcp-servers.mdx b/docs/manage-sandboxes/manage-mcp-servers.mdx index 38edd3e61c9..077e7c15dd1 100644 --- a/docs/manage-sandboxes/manage-mcp-servers.mdx +++ b/docs/manage-sandboxes/manage-mcp-servers.mdx @@ -43,7 +43,7 @@ An existing valid provider can remain ready when a host variable is unset becaus ## Migrate an Existing Registration -Preview legacy Mcporter or Deep Agents projection entries before the first mutation with the cutover release: +To keep using legacy Mcporter or Deep Agents projection entries, preview their conversion to native configuration: ```bash $$nemoclaw my-sandbox mcp migrate @@ -199,6 +199,20 @@ Removing the server deletes the agent-native entry, removes its generated key fr ## Remove a Server +For a legacy registration, removal requires a committed registry row that proves ownership of the named entry. +Removal also requires every other committed legacy row to have an observed legacy entry with the same server name. +If a registry-only row remains, NemoClaw preserves all sources and refuses removal. +Run `$$nemoclaw my-sandbox mcp migrate --apply` when you intend to convert those registrations to native configuration, or explicitly resolve the registry-only rows first. +When these conditions pass, you do not need to migrate the named entry before removing it. +NemoClaw verifies that the named legacy entry is absent before retiring its ownership row and cleaning up policy and provider attachments. +Ownership rows for remaining legacy entries are preserved so you can remove or migrate them later. +If the registry write fails after the agent entry is removed, rerun the same removal command. +The retry verifies that the entry is still absent and compares any remaining policy's recorded registration and provider identity with the committed ownership row before retiring that row. +Provider detachment still requires an exact match with the live provider identity. +NemoClaw preserves unproven entries and refuses removal when the same server exists in both legacy and native configuration. + +Remove the named registration: + ```bash $$nemoclaw my-sandbox mcp remove github ``` diff --git a/docs/reference/commands.mdx b/docs/reference/commands.mdx index b0662f23b13..0f97b92ffb9 100644 --- a/docs/reference/commands.mdx +++ b/docs/reference/commands.mdx @@ -2443,6 +2443,8 @@ $$nemoclaw my-assistant mcp list [--json] Add an MCP Streamable HTTP server to a sandbox. Pass `--url` for the MCP endpoint and the required single `--env KEY` bearer credential for the sandbox-side MCP client. Pass repeatable `--deny-tool ` options to block matching `tools/call` requests at the OpenShell proxy. The generated deny rules match tool names only and do not inspect arguments. Pass a repeatable `--trusted-private-host ` option to admit an exact RFC1918, CGNAT, or IPv6 unique local destination for the current command. The declaration must equal the normalized host from `--url`. For managed MCP, use a DNS hostname for an IPv6 unique local address because NemoClaw has not qualified direct IPv6-literal MCP URLs. You can supply exact hosts through `NEMOCLAW_TRUSTED_PRIVATE_HOSTS` instead, and NemoClaw combines the variable with command options. NemoClaw records the resulting exact trust intent and address pins, so restart, rebuild, and restore do not depend on the ambient environment. NemoClaw registers that credential in an OpenShell provider, installs a generated OpenShell `protocol: mcp` policy for the target endpoint, attaches the provider to the running sandbox, and writes only an OpenShell resolver placeholder for the recorded key into the agent configuration. Inline `--env KEY=VALUE` is rejected because it would expose the value in NemoClaw process arguments. Load the variable from a secret manager or masked prompt, export it without recording the value in shell history, and pass only `--env KEY`. All endpoints must use HTTPS. The full URL and path are persisted and displayed, so URLs cannot contain userinfo, query strings, fragments, known secret-shaped path material, percent-escaped or glob-style paths, or port zero. Server names must start with a letter and contain at most 64 letters, digits, hyphens, or underscores, and endpoint hostnames must use canonical lowercase DNS labels. NemoClaw rejects invalid names and endpoints before it writes lifecycle state or changes OpenShell resources. NemoClaw generates a narrow `protocol: mcp` policy for the destination, literal path, adapter binaries, pinned addresses, explicit MCP methods, and a 131,072-byte request-body limit. OpenShell `0.0.116` evaluates that policy before replacing the attached provider placeholder in the allowed request header. NemoClaw imports the endpointless `nemoclaw-mcp-v1` profile and binds the dedicated provider to that endpoint with `credential_binding.provider`. OpenShell withholds the credential before the binding is active and outside the bound host, port, and path. The sandbox client connects directly through OpenShell's existing egress path, and NemoClaw does not run a host-side MCP data-plane bridge, proxy, relay, or listener. After the add commits, NemoClaw freshly verifies the exact generated policy, expected provider attachment, recorded provider ID, `nemoclaw-mcp-v1` type, valid resource version, and exactly one credential key matching the recorded key. If those readiness checks pass, it sends a differential pair of wire-level MCP `initialize` requests from inside the sandbox — one with the placeholder header and one with an unresolvable control bearer — to verify that OpenShell resolves the credential on egress; otherwise it reports an inconclusive `probe skipped` result and sends no request. Neither outcome fails the committed add, and `--no-probe` skips this check. For full setup details, see [Add an MCP Server](../manage-sandboxes/mcp-servers/add-an-mcp-server). +Credential-bound managed definitions may share a normalized endpoint only when they intentionally share one credential binding. NemoClaw rejects the same endpoint with different credential bindings; use a distinct endpoint URL when different credentials are required. + The generated policy is also binary-scoped: the selected agent's adapter binaries are allowed, while an interactive `/usr/bin/curl` request to the same endpoint is denied. Use `mcp status ` to inspect policy readiness and run the supported bounded in-sandbox reachability check. The command verifies credential resolution only when it reports a verified verdict; other probe outcomes are inconclusive. @@ -2572,7 +2574,11 @@ Remove an MCP server from a sandbox. NemoClaw first removes the named agent-native entry so the agent can no longer use it, then removes the deterministic policy key and detaches the exact live provider when the policy binding, provider identity, type, and credential key agree. The workspace provider and its stored credential are deliberately preserved because current OpenShell provider deletion is name-based. The command reports the retained provider name so an operator can inspect or remove it separately after confirming it is unused. -The command fails closed when it cannot safely identify the source entry. `--force` permits removal of a modified same-name agent entry, but it does not widen policy or provider authority and never deletes a provider. Legacy Mcporter or `.nemoclaw-mcp.json` entries must be converted with `mcp migrate --apply` before ordinary removal. +The command fails closed when it cannot safely identify the source entry. `--force` permits removal of a modified same-name agent entry, but it does not widen policy or provider authority and never deletes a provider. A named legacy Mcporter or `.nemoclaw-mcp.json` entry can be removed directly only when its committed NemoClaw registry row proves ownership. An unproven legacy entry is preserved; use `mcp migrate --apply` when you intend to convert it to native state. + +Legacy removal also requires every other committed legacy row to have an observed legacy entry with the same server name. +If a registry-only row remains, NemoClaw preserves all sources and refuses removal. +Run `$$nemoclaw my-assistant mcp migrate --apply` when you intend to convert those registrations to native configuration, or explicitly resolve the registry-only rows first. ```bash $$nemoclaw my-assistant mcp remove github [--force] diff --git a/docs/reference/troubleshoot-mcp-servers.mdx b/docs/reference/troubleshoot-mcp-servers.mdx index 82108cbd7d0..2562f3de662 100644 --- a/docs/reference/troubleshoot-mcp-servers.mdx +++ b/docs/reference/troubleshoot-mcp-servers.mdx @@ -90,7 +90,7 @@ NemoClaw disables the native agent entry first and cleans only live resources wh ## Agent MCP Capability Is Missing If add or restart reports that the OpenClaw native writer, Hermes transaction helper, or Deep Agents native MCP capability v3 is unavailable, rebuild the sandbox onto a current image before retrying. -When an older OpenClaw or Deep Agents sandbox has a legacy MCP file, preview `mcp migrate`, then use `mcp migrate --apply` to convert it. A native definition that conflicts with the legacy definition stops migration before mutation. +When an older OpenClaw or Deep Agents sandbox has a legacy MCP file, preview `mcp migrate`, then use `mcp migrate --apply` when you intend to convert it. To delete one named legacy entry instead, run `mcp remove `; NemoClaw removes it only when a matching committed registry row proves ownership and otherwise preserves it. A native definition that conflicts with the legacy definition stops migration before mutation. Migration carries the legacy `.deepagents/.nemoclaw-mcp.json` entries through a normal rebuild into the native `/sandbox/.deepagents/.mcp.json` file. diff --git a/src/lib/actions/sandbox/mcp-bridge-adapter-teardown.test.ts b/src/lib/actions/sandbox/mcp-bridge-adapter-teardown.test.ts index cff020bdb6b..ecc503a694a 100644 --- a/src/lib/actions/sandbox/mcp-bridge-adapter-teardown.test.ts +++ b/src/lib/actions/sandbox/mcp-bridge-adapter-teardown.test.ts @@ -18,6 +18,9 @@ const mocks = vi.hoisted(() => ({ getSandboxOrThrow: vi.fn(), inspectExactMcpDestroyProvider: vi.fn(), inspectMcpProvider: vi.fn(), + assertMcpProviderRecoverable: vi.fn(), + preflightMcpEntryTargets: vi.fn(), + detachProvider: vi.fn(), observeMcpCredentialRevision: vi.fn(), removeGeneratedPolicy: vi.fn(), registerAgentAdapterAtCurrentCredentialRevision: vi.fn(), @@ -35,13 +38,13 @@ vi.mock("./mcp-bridge-provider-readiness", () => ({ observeMcpCredentialRevision: mocks.observeMcpCredentialRevision, })); vi.mock("./mcp-bridge-provider", () => ({ - assertMcpProviderRecoverable: vi.fn(), + assertMcpProviderRecoverable: mocks.assertMcpProviderRecoverable, assertNoProviderCredentialCollisions: vi.fn(), assertNoRegisteredProviderCredentialCollisions: vi.fn(), - detachProvider: vi.fn(), + detachProvider: mocks.detachProvider, getMcpProviderInspectionRuntimeSelection: mocks.getMcpProviderInspectionRuntimeSelection, inspectMcpProvider: mocks.inspectMcpProvider, - preflightMcpEntryTargets: vi.fn(), + preflightMcpEntryTargets: mocks.preflightMcpEntryTargets, waitForDetachedMcpCredential: vi.fn(), })); vi.mock("./mcp-bridge-destroy-preflight", () => ({ @@ -66,7 +69,8 @@ vi.mock("./mcp-bridge-restart", () => ({ vi.mock("./mcp-bridge-runtime-capabilities", () => ({ assertMcpAdapterTeardownRuntimeCapabilities: vi.fn(), })); -vi.mock("./mcp-bridge-state", () => ({ +vi.mock("./mcp-bridge-state", async (importOriginal) => ({ + ...(await importOriginal()), ensureSandboxGatewaySelected: mocks.ensureSandboxGatewaySelected, getBridgeAdapter: mocks.getBridgeAdapter, getSandboxAgent: mocks.getSandboxAgent, @@ -78,7 +82,10 @@ vi.mock("./mcp-bridge-validation", () => ({ })); import { scrubManagedMcpAdapterOrThrow } from "./mcp-bridge-adapter-teardown"; -import { prepareMcpBridgesForRebuild } from "./mcp-bridge-rebuild"; +import { + prepareMcpBridgesForAbsentSandboxRebuild, + prepareMcpBridgesForRebuild, +} from "./mcp-bridge-rebuild"; const sandbox = { agent: "hermes" } as SandboxEntry; const runtimeSelection = { gatewayName: "nemoclaw-8091", workspace: "default" } as const; @@ -95,6 +102,7 @@ const entry: McpSourceEntry = { describe("MCP adapter teardown rollback", () => { beforeEach(() => { + vi.clearAllMocks(); mocks.ensureSandboxGatewaySelected.mockReset().mockResolvedValue(undefined); mocks.getMcpProviderInspectionRuntimeSelection.mockReset().mockReturnValue(runtimeSelection); mocks.getBridgeAdapter.mockReset().mockReturnValue("hermes-config"); @@ -120,6 +128,57 @@ describe("MCP adapter teardown rollback", () => { mocks.unregisterAgentAdapter.mockReset().mockReturnValue("removed"); }); + it.each([ + ["present sandbox", prepareMcpBridgesForRebuild], + ["absent sandbox", prepareMcpBridgesForAbsentSandboxRebuild], + ] as const)( + "rejects conflicting credentials before rebuild preparation with %s", + async (_name, prepare) => { + const conflicting = { + ...entry, + server: "github-alias", + env: ["GITHUB_ALIAS_TOKEN"], + providerName: "alpha-mcp-github-alias", + }; + + await expect(prepare("alpha", [entry, conflicting])).rejects.toThrow( + /cannot safely choose between credentials for an indistinguishable endpoint/, + ); + + expect(mocks.preflightMcpEntryTargets).not.toHaveBeenCalled(); + expect(mocks.ensureSandboxGatewaySelected).not.toHaveBeenCalled(); + expect(mocks.assertMcpProviderRecoverable).not.toHaveBeenCalled(); + expect(mocks.inspectMcpProvider).not.toHaveBeenCalled(); + expect(mocks.inspectExactMcpDestroyProvider).not.toHaveBeenCalled(); + expect(mocks.unregisterAgentAdapter).not.toHaveBeenCalled(); + expect(mocks.removeGeneratedPolicy).not.toHaveBeenCalled(); + expect(mocks.detachProvider).not.toHaveBeenCalled(); + }, + ); + + it("preserves distinct credential endpoints in an absent-sandbox rebuild handoff", async () => { + const distinct = { + ...entry, + server: "other", + url: "https://other.example.test/mcp/", + env: ["OTHER_TOKEN"], + providerName: "alpha-mcp-other", + }; + + await expect( + prepareMcpBridgesForAbsentSandboxRebuild("alpha", [entry, distinct]), + ).resolves.toMatchObject({ + entries: [entry, distinct], + detachedProviderEntries: [], + scrubbedAdapterEntries: [], + }); + expect(mocks.preflightMcpEntryTargets).toHaveBeenCalledWith([entry, distinct]); + expect(mocks.assertMcpProviderRecoverable).toHaveBeenCalledTimes(2); + expect(mocks.unregisterAgentAdapter).not.toHaveBeenCalled(); + expect(mocks.removeGeneratedPolicy).not.toHaveBeenCalled(); + expect(mocks.detachProvider).not.toHaveBeenCalled(); + }); + it("restores the fresh revision observed after a later rebuild step fails (#10155)", async () => { mocks.observeMcpCredentialRevision .mockReset() diff --git a/src/lib/actions/sandbox/mcp-bridge-add-restart.ts b/src/lib/actions/sandbox/mcp-bridge-add-restart.ts index 341f4178ff8..f46f664ca0a 100644 --- a/src/lib/actions/sandbox/mcp-bridge-add-restart.ts +++ b/src/lib/actions/sandbox/mcp-bridge-add-restart.ts @@ -51,6 +51,7 @@ import { waitForDetachedMcpCredential, } from "./mcp-bridge-provider"; import { + assertNoAmbiguousMcpCredentialTarget, assertNoDerivedResourceCollision, ensureSandboxGatewaySelected, getBridgeAdapter, @@ -488,6 +489,12 @@ async function addMcpBridgeUnlocked( : undefined; const adapterEnvValues = resolveCredentialEnv(options.env); const policyName = buildMcpBridgePolicyName(options.server); + assertNoAmbiguousMcpCredentialTarget( + observed.bridges, + options.server, + normalizedUrl, + providerName, + ); assertNoDerivedResourceCollision(observed.bridges, options.server, providerName, policyName); let requestedEntry: McpSourceEntry = { server: options.server, diff --git a/src/lib/actions/sandbox/mcp-bridge-migration.test.ts b/src/lib/actions/sandbox/mcp-bridge-migration.test.ts index 8d3b293e8cf..39a5c950c4f 100644 --- a/src/lib/actions/sandbox/mcp-bridge-migration.test.ts +++ b/src/lib/actions/sandbox/mcp-bridge-migration.test.ts @@ -2,6 +2,7 @@ // SPDX-License-Identifier: Apache-2.0 import { beforeEach, describe, expect, it, vi } from "vitest"; +import type { McpProviderInspection } from "./mcp-bridge-provider"; const entry = { server: "github", @@ -24,6 +25,8 @@ const mocks = vi.hoisted(() => ({ getAdapter: vi.fn(), updateSandbox: vi.fn(), inspectLegacy: vi.fn(), + inspectSource: vi.fn(), + inspectPolicyOnly: vi.fn(), inspectSources: vi.fn(), joinEntries: vi.fn((_sandbox: unknown, entries: unknown) => entries), removeLegacy: vi.fn(), @@ -31,8 +34,20 @@ const mocks = vi.hoisted(() => ({ reloadOpenClaw: vi.fn(), unregister: vi.fn(), selectGateway: vi.fn(), + assertTeardown: vi.fn(), + removePolicy: vi.fn(), + inspectProvider: vi.fn(async (): Promise => ({ + exists: false, + id: null, + resourceVersion: null, + type: null, + credentialKeys: null, + })), + detachProvider: vi.fn(), + waitForDetached: vi.fn(), preflightTargets: vi.fn().mockResolvedValue(new Map([["github", { addresses: ["8.8.8.8"] }]])), readConfig: vi.fn(), + writeConfig: vi.fn(), })); vi.mock("../../state/mcp-lifecycle-lock", () => ({ @@ -44,13 +59,19 @@ vi.mock("../../state/registry", () => ({ })); vi.mock("../../state/config-io", () => ({ readConfigFile: mocks.readConfig, + writeConfigFile: mocks.writeConfig, +})); +vi.mock("../../state/registry/lock", () => ({ + withLock: (operation: () => unknown) => operation(), })); vi.mock("./mcp-bridge-adapters", () => ({ + assertAgentMcpTeardownRuntimeCapability: mocks.assertTeardown, registerAgentAdapter: mocks.register, reloadOpenClawGatewayAfterMcpMutation: mocks.reloadOpenClaw, unregisterAgentAdapter: mocks.unregister, })); -vi.mock("./mcp-bridge-provider", () => ({ +vi.mock("./mcp-bridge-provider", async (importOriginal) => ({ + ...(await importOriginal()), assertMcpProviderRecoverable: mocks.assertProviderRecoverable, getMcpProviderInspectionRuntimeSelection: () => ({ gatewayName: "nemoclaw", @@ -58,9 +79,15 @@ vi.mock("./mcp-bridge-provider", () => ({ }), providerAttached: () => true, preflightMcpEntryTargets: mocks.preflightTargets, + inspectMcpProvider: mocks.inspectProvider, + detachProvider: mocks.detachProvider, + waitForDetachedMcpCredential: mocks.waitForDetached, })); -vi.mock("./mcp-bridge-source", () => ({ +vi.mock("./mcp-bridge-source", async (importOriginal) => ({ + ...(await importOriginal()), inspectLegacyBridgeState: mocks.inspectLegacy, + inspectSourceBridgeState: mocks.inspectSource, + inspectPolicyOnlyMcpEntry: mocks.inspectPolicyOnly, inspectAgentMcpSources: mocks.inspectSources, removeLegacyAgentMcpEntry: mocks.removeLegacy, joinMcpEntriesToOpenShell: mocks.joinEntries, @@ -68,6 +95,7 @@ vi.mock("./mcp-bridge-source", () => ({ vi.mock("./mcp-bridge-policy", async (importOriginal) => ({ ...(await importOriginal()), getPolicyPresence: mocks.getPolicyPresence, + removeGeneratedPolicy: mocks.removePolicy, })); vi.mock("../../policy", () => ({ getPresetContentGatewayState: mocks.getPolicyState, @@ -76,9 +104,14 @@ vi.mock("./mcp-bridge-state", () => ({ ensureSandboxGatewaySelected: mocks.selectGateway, getSandboxAgent: mocks.getAgent, getBridgeAdapter: mocks.getAdapter, + getSandboxOrThrow: mocks.getSandbox, +})); +vi.mock("../../onboard/experimental/portable-agent-lifecycle", () => ({ + assertHermesPortableCommandUnavailable: vi.fn(), })); import { migrateMcpBridges } from "./mcp-bridge-migration"; +import { removeMcpBridge } from "./mcp-bridge-remove"; describe("explicit MCP migration", () => { beforeEach(() => { @@ -92,6 +125,10 @@ describe("explicit MCP migration", () => { mocks.getAdapter.mockReturnValue("openclaw-config"); mocks.updateSandbox.mockReturnValue(true); mocks.readConfig.mockReturnValue({}); + mocks.inspectPolicyOnly.mockResolvedValue(undefined); + mocks.writeConfig.mockImplementation((_path: string, document: unknown) => { + mocks.readConfig.mockReturnValue(structuredClone(document)); + }); mocks.getPolicyPresence.mockResolvedValue(true); mocks.getPolicyState.mockResolvedValue("match"); mocks.joinEntries.mockImplementation((_sandbox: unknown, entries: unknown) => entries); @@ -99,6 +136,10 @@ describe("explicit MCP migration", () => { bridges: { github: entry }, sources: { native: {}, legacy: { github: entry } }, }); + mocks.inspectSource.mockReturnValue({ + bridges: { github: entry }, + sources: { native: {}, legacy: { github: entry } }, + }); mocks.inspectSources.mockReturnValue({ native: { github: { ...entry, source: "native" } }, legacy: { github: entry }, @@ -129,6 +170,14 @@ describe("explicit MCP migration", () => { }); it("materializes native config, verifies it, then retires legacy state", async () => { + const registry = + await vi.importActual("../../state/registry"); + const sibling = { name: "beta", mcp: { bridges: { github: entry } } }; + mocks.readConfig.mockReturnValue({ + sandboxes: { alpha: { name: "alpha", mcp: { bridges: { github: entry } } }, beta: sibling }, + }); + expect(registry.updateSandbox("alpha", { model: "new-model" })).toBe(true); + expect(mocks.readConfig().sandboxes.alpha.mcp.bridges.github).toEqual(entry); await expect(migrateMcpBridges("alpha", { apply: true })).resolves.toMatchObject({ applied: true, }); @@ -139,7 +188,8 @@ describe("explicit MCP migration", () => { expect(mocks.reloadOpenClaw.mock.invocationCallOrder[0]).toBeLessThan( mocks.removeLegacy.mock.invocationCallOrder[0], ); - expect(mocks.updateSandbox).toHaveBeenCalledWith("alpha", {}); + expect(mocks.readConfig().sandboxes.alpha).toEqual({ name: "alpha", model: "new-model" }); + expect(mocks.readConfig().sandboxes.beta).toEqual(sibling); }); it("validates restrictive live policy before the first native write", async () => { @@ -152,6 +202,22 @@ describe("explicit MCP migration", () => { expect(mocks.removeLegacy).not.toHaveBeenCalled(); }); + it("preserves changed ownership instead of retiring a different migration snapshot", async () => { + const original = { bridges: { github: entry } }; + const replacement = { bridges: { github: { ...entry, providerId: "replacement" } } }; + mocks.readConfig.mockReturnValue({ sandboxes: { alpha: { name: "alpha", mcp: original } } }); + mocks.removeLegacy.mockImplementationOnce(async () => { + mocks.readConfig.mockReturnValue({ + sandboxes: { alpha: { name: "alpha", mcp: replacement } }, + }); + }); + await expect(migrateMcpBridges("alpha", { apply: true })).rejects.toThrow( + "Legacy MCP ownership changed during migration", + ); + expect(mocks.writeConfig).not.toHaveBeenCalled(); + expect(mocks.readConfig().sandboxes.alpha.mcp).toEqual(replacement); + }); + it("retains legacy state when OpenClaw activation fails", async () => { mocks.reloadOpenClaw.mockImplementationOnce(() => { throw new Error("activation failed"); @@ -176,6 +242,9 @@ describe("explicit MCP migration", () => { mcpCapability: { support: "bridge", adapter: deepEntry.adapter }, }); mocks.getAdapter.mockReturnValue(deepEntry.adapter); + mocks.readConfig.mockReturnValue({ + sandboxes: { alpha: { name: "alpha", mcp: { bridges: { github: deepEntry } } } }, + }); mocks.inspectLegacy.mockReturnValue({ bridges: { github: deepEntry }, sources: { native: {}, legacy: { github: deepEntry } }, @@ -194,10 +263,10 @@ describe("explicit MCP migration", () => { expect(rebuildSandbox).toHaveBeenCalledWith("alpha"); expect(mocks.removeLegacy).toHaveBeenCalledWith( expect.objectContaining({ agent: deepEntry.agent }), - deepEntry, + { ...deepEntry, source: "legacy-registry", denyTools: [] }, expect.any(Object), ); - expect(mocks.updateSandbox).toHaveBeenCalledWith("alpha", {}); + expect(mocks.readConfig().sandboxes.alpha).toEqual({ name: "alpha" }); }); it("does not report Deep Agents migration success when legacy cleanup fails", async () => { @@ -308,6 +377,389 @@ describe("explicit MCP migration", () => { expect(mocks.register).not.toHaveBeenCalled(); }); + it("rejects ambiguous credential targets before native registration or legacy cleanup", async () => { + const secondEntry = { + ...entry, + server: "gitlab", + env: ["GITLAB_TOKEN"], + providerName: "alpha-mcp-gitlab", + providerId: "gitlab-provider-id", + policyName: "mcp-bridge-gitlab", + }; + mocks.inspectLegacy.mockReturnValue({ + bridges: { github: entry, gitlab: secondEntry }, + sources: { native: {}, legacy: { github: entry, gitlab: secondEntry } }, + }); + + await expect(migrateMcpBridges("alpha", { apply: true })).rejects.toThrow( + /cannot safely choose between credentials for an indistinguishable endpoint/, + ); + expect(mocks.register).not.toHaveBeenCalled(); + expect(mocks.removeLegacy).not.toHaveBeenCalled(); + }); + + it("preserves survivor ownership for migration or a second direct removal", async () => { + const secondEntry = { + ...entry, + server: "gitlab", + env: ["GITLAB_TOKEN"], + providerName: "alpha-mcp-gitlab", + providerId: "gitlab-provider-id", + policyName: "mcp-bridge-gitlab", + }; + const legacy = { github: entry, gitlab: secondEntry }; + mocks.inspectSource.mockReturnValue({ + bridges: {}, + sources: { native: {}, legacy }, + }); + const retained = { name: "alpha", agent: "openclaw", gatewayName: "nemoclaw" }; + const otherSandbox = { name: "beta", mcp: { bridges: { github: entry } } }; + mocks.readConfig.mockReturnValue({ + defaultSandbox: "alpha", + sandboxes: { alpha: { ...retained, mcp: { bridges: legacy } }, beta: otherSandbox }, + }); + mocks.inspectSources.mockResolvedValueOnce({ native: {}, legacy: { github: entry } }); + mocks.inspectProvider.mockResolvedValueOnce({ + exists: true, + id: secondEntry.providerId, + resourceVersion: 1, + type: "generic", + credentialKeys: secondEntry.env, + }); + mocks.detachProvider.mockResolvedValueOnce("detached"); + + await expect(removeMcpBridge("alpha", "gitlab")).resolves.toBeUndefined(); + expect(mocks.removeLegacy).toHaveBeenCalledExactlyOnceWith( + { name: "alpha", agent: "openclaw" }, + secondEntry, + { gatewayName: "nemoclaw", workspace: "default" }, + ); + expect(mocks.unregister).not.toHaveBeenCalled(); + expect(mocks.readConfig()).toEqual({ + defaultSandbox: "alpha", + sandboxes: { + alpha: { ...retained, mcp: { bridges: { github: entry } } }, + beta: otherSandbox, + }, + }); + expect(mocks.updateSandbox).not.toHaveBeenCalled(); + + const runtimeSelection = { gatewayName: "nemoclaw", workspace: "default" }; + const committedEntry = { ...secondEntry, source: "legacy-registry", denyTools: [] }; + expect(mocks.removePolicy).toHaveBeenCalledExactlyOnceWith("alpha", committedEntry, { + runtimeSelection, + }); + expect(mocks.detachProvider).toHaveBeenCalledExactlyOnceWith("alpha", committedEntry, { + allowLegacyGeneric: true, + runtimeSelection, + }); + expect(mocks.waitForDetached).toHaveBeenCalledExactlyOnceWith( + "alpha", + committedEntry, + runtimeSelection, + ); + + mocks.inspectLegacy.mockReturnValue({ + bridges: { github: entry }, + sources: { native: {}, legacy: { github: entry } }, + }); + await expect(migrateMcpBridges("alpha")).resolves.toMatchObject({ + items: [{ server: "github", action: "migrate" }], + }); + mocks.inspectSource.mockReturnValue({ + bridges: {}, + sources: { native: {}, legacy: { github: entry } }, + }); + mocks.inspectSources.mockResolvedValueOnce({ native: {}, legacy: {} }); + await expect(removeMcpBridge("alpha", "github")).resolves.toBeUndefined(); + expect(mocks.readConfig()).toEqual({ + defaultSandbox: "alpha", + sandboxes: { alpha: retained, beta: otherSandbox }, + }); + expect(mocks.removeLegacy).toHaveBeenCalledTimes(2); + expect(mocks.removePolicy).toHaveBeenLastCalledWith( + "alpha", + { + ...entry, + source: "legacy-registry", + denyTools: [], + }, + { runtimeSelection }, + ); + }); + + it.each([ + [ + "a committed row changes", + () => + mocks.inspectSources.mockImplementationOnce(async () => { + mocks.readConfig.mockReturnValue({ + sandboxes: { + alpha: { mcp: { bridges: { github: { ...entry, providerId: "replacement" } } } }, + }, + }); + return { native: {}, legacy: {} }; + }), + ], + [ + "still present", + () => mocks.inspectSources.mockResolvedValueOnce({ native: {}, legacy: { github: entry } }), + ], + [ + "reappears as a native registration", + () => mocks.inspectSources.mockResolvedValueOnce({ native: { github: entry }, legacy: {} }), + ], + [ + "inspection fails", + () => mocks.inspectSources.mockRejectedValueOnce(new Error("inspection unavailable")), + ], + [ + "ownership changes", + () => + mocks.inspectSources.mockImplementationOnce(async () => { + mocks.readConfig.mockReturnValue({ sandboxes: { alpha: {} } }); + return { native: {}, legacy: {} }; + }), + ], + ] as const)( + "preserves cleanup state when post-removal verification %s", + async (_outcome, arrangeInspection) => { + const document = { sandboxes: { alpha: { mcp: { bridges: { github: entry } } } } }; + mocks.readConfig.mockReturnValue(document); + arrangeInspection(); + await expect(removeMcpBridge("alpha", "github")).rejects.toThrow( + /remains in agent configuration|inspection unavailable|ownership changed/, + ); + expect(mocks.writeConfig).not.toHaveBeenCalled(); + expect(mocks.updateSandbox).not.toHaveBeenCalled(); + expect(mocks.removePolicy).not.toHaveBeenCalled(); + expect(mocks.detachProvider).not.toHaveBeenCalled(); + }, + ); + + it("removes a native server without changing an unrelated legacy registration", async () => { + const nativeEntry = { + ...entry, + server: "gitlab", + source: "native" as const, + env: ["GITLAB_TOKEN"], + providerName: "alpha-mcp-gitlab", + providerId: "gitlab-provider-id", + policyName: "mcp-bridge-gitlab", + }; + const legacy = { github: entry }; + mocks.inspectSource.mockReturnValue({ + bridges: { gitlab: nativeEntry }, + sources: { native: { gitlab: nativeEntry }, legacy }, + }); + mocks.unregister.mockResolvedValueOnce("removed"); + + await expect(removeMcpBridge("alpha", "gitlab")).resolves.toBeUndefined(); + const runtimeSelection = { gatewayName: "nemoclaw", workspace: "default" }; + expect(mocks.unregister).toHaveBeenCalledExactlyOnceWith( + "alpha", + "openclaw-config", + nativeEntry, + runtimeSelection, + expect.objectContaining({ teardown: true, force: false }), + ); + expect(mocks.removePolicy).toHaveBeenCalledExactlyOnceWith("alpha", nativeEntry, { + runtimeSelection, + }); + expect(mocks.removeLegacy).not.toHaveBeenCalled(); + expect(mocks.updateSandbox).not.toHaveBeenCalled(); + expect(legacy).toEqual({ github: entry }); + }); + + it("finishes owned cleanup when a registry write fails after source removal", async () => { + mocks.readConfig.mockReturnValue({ + sandboxes: { alpha: { mcp: { bridges: { github: entry } } } }, + }); + mocks.inspectSources.mockResolvedValue({ native: {}, legacy: {} }); + mocks.removeLegacy.mockImplementationOnce(async () => { + mocks.inspectSource.mockReturnValue({ bridges: {}, sources: { native: {}, legacy: {} } }); + }); + mocks.writeConfig.mockImplementationOnce(() => { + throw new Error("registry disk failure"); + }); + await expect(removeMcpBridge("alpha", "github")).rejects.toThrow("registry disk failure"); + expect(mocks.removePolicy).not.toHaveBeenCalled(); + expect(mocks.detachProvider).not.toHaveBeenCalled(); + mocks.inspectPolicyOnly.mockResolvedValueOnce({ ...entry, source: "policy" }); + mocks.inspectProvider.mockResolvedValue({ + exists: true, + id: entry.providerId, + resourceVersion: 1, + type: "generic", + credentialKeys: entry.env, + }); + mocks.detachProvider.mockResolvedValueOnce("detached"); + await expect(removeMcpBridge("alpha", "github")).resolves.toBeUndefined(); + expect(mocks.readConfig()).toEqual({ sandboxes: { alpha: {} } }); + expect(mocks.removeLegacy).toHaveBeenCalledOnce(); + expect(mocks.removePolicy).toHaveBeenCalledOnce(); + expect(mocks.detachProvider).toHaveBeenCalledOnce(); + expect(mocks.waitForDetached).toHaveBeenCalledOnce(); + expect(mocks.unregister).not.toHaveBeenCalled(); + }); + + it("removes an owned legacy entry after an unrelated real registry update", async () => { + const registry = + await vi.importActual("../../state/registry"); + mocks.readConfig.mockReturnValue({ + sandboxes: { alpha: { name: "alpha", mcp: { bridges: { github: entry } } } }, + }); + expect(registry.updateSandbox("alpha", { agentVersion: "new-version" })).toBe(true); + mocks.inspectSources.mockResolvedValueOnce({ native: {}, legacy: {} }); + await expect(removeMcpBridge("alpha", "github")).resolves.toBeUndefined(); + expect(mocks.readConfig().sandboxes.alpha).toEqual({ + name: "alpha", + agentVersion: "new-version", + }); + expect(mocks.removeLegacy).toHaveBeenCalledOnce(); + expect(mocks.removePolicy).toHaveBeenCalledOnce(); + }); + + it("preserves a registry-only row when the policy's recorded provider identity changed", async () => { + mocks.readConfig.mockReturnValue({ + sandboxes: { alpha: { mcp: { bridges: { github: entry } } } }, + }); + mocks.inspectSource.mockReturnValue({ bridges: {}, sources: { native: {}, legacy: {} } }); + mocks.inspectPolicyOnly.mockResolvedValueOnce({ + ...entry, + source: "policy", + providerId: "replacement", + }); + await expect(removeMcpBridge("alpha", "github")).rejects.toThrow("no longer matches"); + expect(mocks.writeConfig).not.toHaveBeenCalled(); + expect(mocks.removePolicy).not.toHaveBeenCalled(); + expect(mocks.detachProvider).not.toHaveBeenCalled(); + }); + + it("preserves a registry-only row when the live provider identity changed", async () => { + mocks.readConfig.mockReturnValue({ + sandboxes: { alpha: { mcp: { bridges: { github: entry } } } }, + }); + mocks.inspectSource.mockReturnValue({ bridges: {}, sources: { native: {}, legacy: {} } }); + mocks.inspectProvider.mockResolvedValueOnce({ + exists: true, + id: "replacement", + resourceVersion: 1, + type: "generic", + credentialKeys: entry.env, + }); + + await expect(removeMcpBridge("alpha", "github")).rejects.toThrow("provider identity changed"); + expect(mocks.writeConfig).not.toHaveBeenCalled(); + expect(mocks.removePolicy).not.toHaveBeenCalled(); + expect(mocks.detachProvider).not.toHaveBeenCalled(); + }); + + it("rejects legacy removal before mutation when another registry-only row remains", async () => { + const registryOnlyEntry = { + ...entry, + server: "gitlab", + url: "https://gitlab.example.test/mcp", + env: ["GITLAB_TOKEN"], + providerName: "alpha-mcp-gitlab", + providerId: "gitlab-provider-id", + policyName: "mcp-bridge-gitlab", + }; + mocks.inspectSource.mockReturnValue({ + bridges: {}, + sources: { native: {}, legacy: { github: entry } }, + }); + mocks.readConfig.mockReturnValue({ + sandboxes: { alpha: { mcp: { bridges: { github: entry, gitlab: registryOnlyEntry } } } }, + }); + + await expect(removeMcpBridge("alpha", "github")).rejects.toThrow( + /registry-only legacy registration.*gitlab.*No source was changed/, + ); + expect(mocks.selectGateway).not.toHaveBeenCalled(); + expect(mocks.removeLegacy).not.toHaveBeenCalled(); + expect(mocks.updateSandbox).not.toHaveBeenCalled(); + expect(mocks.unregister).not.toHaveBeenCalled(); + }); + + it("preserves a named legacy entry without matching registry ownership", async () => { + mocks.readConfig.mockReturnValue({}); + + await expect(removeMcpBridge("alpha", "github")).rejects.toThrow( + /cannot be proven as registry-owned and was preserved/, + ); + expect(mocks.removeLegacy).not.toHaveBeenCalled(); + expect(mocks.unregister).not.toHaveBeenCalled(); + }); + + it("preserves legacy source and ownership when teardown capability fails", async () => { + mocks.readConfig.mockReturnValue({ + sandboxes: { alpha: { mcp: { bridges: { github: entry } } } }, + }); + mocks.assertTeardown.mockRejectedValueOnce(new Error("teardown capability unavailable")); + await expect(removeMcpBridge("alpha", "github")).rejects.toThrow( + "teardown capability unavailable", + ); + expect(mocks.removeLegacy).not.toHaveBeenCalled(); + expect(mocks.updateSandbox).not.toHaveBeenCalled(); + expect(mocks.unregister).not.toHaveBeenCalled(); + expect(mocks.removePolicy).not.toHaveBeenCalled(); + expect(mocks.detachProvider).not.toHaveBeenCalled(); + }); + + it("preserves dual legacy and native registrations before any removal", async () => { + mocks.inspectSource.mockReturnValue({ + bridges: { github: entry }, + sources: { legacy: { github: entry }, native: { github: { ...entry, source: "native" } } }, + }); + mocks.readConfig.mockReturnValue({ + sandboxes: { alpha: { mcp: { bridges: { github: entry } } } }, + }); + await expect(removeMcpBridge("alpha", "github")).rejects.toThrow(/both legacy and native/); + expect(mocks.removeLegacy).not.toHaveBeenCalled(); + expect(mocks.updateSandbox).not.toHaveBeenCalled(); + expect(mocks.unregister).not.toHaveBeenCalled(); + expect(mocks.removePolicy).not.toHaveBeenCalled(); + expect(mocks.detachProvider).not.toHaveBeenCalled(); + }); + + it("enriches native ownership before rejecting a differently credentialed alias", async () => { + const nativeEntry = { + server: "gitlab", + agent: "openclaw", + adapter: "openclaw-config" as const, + url: entry.url, + env: ["GITLAB_TOKEN"], + policyName: "mcp-bridge-gitlab", + source: "native" as const, + }; + mocks.inspectLegacy.mockReturnValue({ + bridges: { github: entry }, + sources: { native: { gitlab: nativeEntry }, legacy: { github: entry } }, + }); + mocks.joinEntries.mockImplementation((_sandbox: unknown, entries: unknown) => { + const sourceEntries = entries as Record; + return sourceEntries.gitlab + ? { + gitlab: { + ...sourceEntries.gitlab, + providerName: "alpha-mcp-gitlab", + providerId: "gitlab-provider-id", + }, + } + : entries; + }); + + await expect(migrateMcpBridges("alpha")).rejects.toThrow( + /cannot safely choose between credentials for an indistinguishable endpoint/, + ); + await expect(migrateMcpBridges("alpha", { apply: true })).rejects.toThrow( + /cannot safely choose between credentials for an indistinguishable endpoint/, + ); + expect(mocks.register).not.toHaveBeenCalled(); + expect(mocks.removeLegacy).not.toHaveBeenCalled(); + }); + it("previews a valid committed registry-only row for explicit migration", async () => { mocks.inspectLegacy.mockReturnValue({ bridges: {}, diff --git a/src/lib/actions/sandbox/mcp-bridge-migration.ts b/src/lib/actions/sandbox/mcp-bridge-migration.ts index b7b37565757..58f8ac96f30 100644 --- a/src/lib/actions/sandbox/mcp-bridge-migration.ts +++ b/src/lib/actions/sandbox/mcp-bridge-migration.ts @@ -3,21 +3,21 @@ import { isDeepStrictEqual } from "node:util"; -import { readConfigFile } from "../../state/config-io"; +import { findAmbiguousMcpCredentialTarget } from "../../domain/mcp-credential-target"; import { withMcpLifecycleLock } from "../../state/mcp-lifecycle-lock"; import * as registry from "../../state/registry"; import * as policies from "../../policy"; -import { REGISTRY_FILE } from "../../state/registry/persistence"; +import { + readLegacyMcpRegistryProjection, + retireLegacyMcpRegistryProjection, +} from "../../state/registry/legacy-mcp"; +import { readCommittedLegacyRegistryEntries, sameMcpRegistration } from "./mcp-bridge-source"; import { registerAgentAdapter, reloadOpenClawGatewayAfterMcpMutation, unregisterAgentAdapter, } from "./mcp-bridge-adapters"; -import { - buildMcpBridgePolicyYaml, - buildMcpBridgePolicyName, - getPolicyPresence, -} from "./mcp-bridge-policy"; +import { buildMcpBridgePolicyYaml, getPolicyPresence } from "./mcp-bridge-policy"; import type { McpSourceEntry } from "./mcp-bridge-contracts"; import { McpBridgeError } from "./mcp-bridge-contracts"; import { @@ -37,7 +37,7 @@ import { getBridgeAdapter, getSandboxAgent, } from "./mcp-bridge-state"; -import { normalizeMcpDenyTools, validateSandboxName } from "./mcp-bridge-validation"; +import { validateSandboxName } from "./mcp-bridge-validation"; export type McpMigrationItem = { server: string; @@ -61,123 +61,6 @@ export type McpMigrationPlan = { applied: boolean; }; -function sameRegistration(left: McpSourceEntry, right: McpSourceEntry): boolean { - return ( - left.server === right.server && left.url === right.url && isDeepStrictEqual(left.env, right.env) - ); -} - -function isObjectRecord(value: unknown): value is Record { - return typeof value === "object" && value !== null && !Array.isArray(value); -} - -function readCommittedLegacyRegistryEntries( - sandboxName: string, - currentAgent: string, - currentAdapter: McpSourceEntry["adapter"], -): Record { - const document = readConfigFile(REGISTRY_FILE, {}); - if (!isObjectRecord(document) || !isObjectRecord(document.sandboxes)) return {}; - const rawSandbox = document.sandboxes[sandboxName]; - if (!isObjectRecord(rawSandbox) || !isObjectRecord(rawSandbox.mcp)) return {}; - const rawState = rawSandbox.mcp; - if (rawState.destroyPreparedAt || rawState.destroyPendingAt) { - throw new McpBridgeError( - `Legacy MCP registry state for '${sandboxName}' contains an incomplete destroy transaction. No source was changed.`, - 2, - ); - } - if (!isObjectRecord(rawState.bridges)) return {}; - const entries: Record = {}; - for (const [server, raw] of Object.entries(rawState.bridges)) { - if (!/^[A-Za-z][A-Za-z0-9_-]{0,63}$/u.test(server) || !isObjectRecord(raw)) { - throw new McpBridgeError( - `Legacy MCP registry server '${server}' is not a valid committed registration. No source was changed.`, - 2, - ); - } - if (raw.addState !== undefined) { - throw new McpBridgeError( - `Legacy MCP registry server '${server}' contains an incomplete add transaction. No source was changed.`, - 2, - ); - } - const agent = typeof raw.agent === "string" && raw.agent ? raw.agent : "openclaw"; - const recordedAdapter = - typeof raw.adapter === "string" && raw.adapter ? raw.adapter : currentAdapter; - const adapter = recordedAdapter === "mcporter" ? "openclaw-config" : recordedAdapter; - if (agent !== currentAgent || adapter !== currentAdapter) { - throw new McpBridgeError( - `Legacy MCP registry server '${server}' targets ${agent}/${String(adapter)} instead of the current ${currentAgent}/${String(currentAdapter)} runtime. No source was changed.`, - 2, - ); - } - if ( - typeof raw.url !== "string" || - raw.url.length > 4096 || - !Array.isArray(raw.env) || - raw.env.length !== 1 || - typeof raw.env[0] !== "string" || - !/^[A-Za-z_][A-Za-z0-9_]{0,127}$/u.test(raw.env[0]) - ) { - throw new McpBridgeError( - `Legacy MCP registry server '${server}' is not a valid committed registration. No source was changed.`, - 2, - ); - } - let url: URL; - try { - url = new URL(raw.url); - } catch { - throw new McpBridgeError( - `Legacy MCP registry server '${server}' has an invalid URL. No source was changed.`, - 2, - ); - } - if (url.protocol !== "https:" || url.username || url.password) { - throw new McpBridgeError( - `Legacy MCP registry server '${server}' has an unsupported URL. No source was changed.`, - 2, - ); - } - const requestedDenyTools = raw.pendingDenyTools ?? raw.denyTools ?? []; - if ( - !Array.isArray(requestedDenyTools) || - requestedDenyTools.some((tool) => typeof tool !== "string") - ) { - throw new McpBridgeError( - `Legacy MCP registry server '${server}' has invalid denied-tool intent. No source was changed.`, - 2, - ); - } - const denyTools = normalizeMcpDenyTools(requestedDenyTools as string[]); - const allowedIps = Array.isArray(raw.allowedIps) - ? raw.allowedIps.filter((address): address is string => typeof address === "string") - : undefined; - entries[server] = { - server, - agent, - adapter, - url: url.toString(), - env: [raw.env[0]], - denyTools, - ...(allowedIps?.length ? { allowedIps } : {}), - ...(typeof raw.trustedPrivateHost === "string" && raw.trustedPrivateHost - ? { trustedPrivateHost: raw.trustedPrivateHost } - : {}), - ...(typeof raw.providerName === "string" && raw.providerName - ? { providerName: raw.providerName } - : {}), - ...(typeof raw.providerId === "string" && raw.providerId - ? { providerId: raw.providerId } - : {}), - policyName: buildMcpBridgePolicyName(server), - source: "legacy-registry", - }; - } - return entries; -} - async function preflightMigrationOpenShellState( sandboxName: string, entries: readonly McpSourceEntry[], @@ -236,10 +119,12 @@ export async function migrateMcpBridges( const observed = await inspectLegacyBridgeState(sandbox, runtimeSelection); const agent = getSandboxAgent(sandbox); const adapter = getBridgeAdapter(agent); + const legacyProjection = readLegacyMcpRegistryProjection(sandboxName); const committedRegistryEntries = readCommittedLegacyRegistryEntries( sandboxName, agent.name, adapter, + legacyProjection, ); const rawRegistryEntries = await joinMcpEntriesToOpenShell( sandbox, @@ -263,7 +148,7 @@ export async function migrateMcpBridges( } for (const [server, registryEntry] of Object.entries(rawRegistryEntries)) { const agentLegacy = observed.bridges[server]; - if (agentLegacy && !sameRegistration(agentLegacy, registryEntry)) { + if (agentLegacy && !sameMcpRegistration(agentLegacy, registryEntry)) { throw new McpBridgeError( `Legacy agent and registry MCP definitions conflict for '${server}'. No source was changed.`, 2, @@ -274,9 +159,25 @@ export async function migrateMcpBridges( const entries = Object.values(legacyEntries).sort((left, right) => left.server.localeCompare(right.server), ); + const nativeEntries = await joinMcpEntriesToOpenShell( + sandbox, + observed.sources.native, + runtimeSelection, + "inspect native MCP migration conflict state", + ); + const ambiguousTarget = findAmbiguousMcpCredentialTarget([ + ...entries, + ...Object.values(nativeEntries), + ]); + if (ambiguousTarget) { + throw new McpBridgeError( + `MCP servers '${ambiguousTarget.entry.server}' and '${ambiguousTarget.conflict.server}' target the same URL with different credential bindings. OpenShell cannot safely choose between credentials for an indistinguishable endpoint. Remove one owned legacy registration with \`nemoclaw ${sandboxName} mcp remove \`, then rerun migration. No source was changed.`, + 2, + ); + } const conflicts = entries.filter((entry) => { const native = observed.sources.native[entry.server]; - return native && !sameRegistration(native, entry); + return native && !sameMcpRegistration(native, entry); }); if (conflicts.length > 0) { throw new McpBridgeError( @@ -345,7 +246,7 @@ export async function migrateMcpBridges( } native = (await inspectAgentMcpSources(rebuilt, rebuiltRuntimeSelection)).native; const missing = entries.filter( - (entry) => !native[entry.server] || !sameRegistration(native[entry.server], entry), + (entry) => !native[entry.server] || !sameMcpRegistration(native[entry.server], entry), ); if (missing.length > 0) { throw new McpBridgeError( @@ -371,7 +272,7 @@ export async function migrateMcpBridges( } throw error; } - registry.updateSandbox(sandboxName, {}); + retireLegacyMcpRegistryProjection(sandboxName, legacyProjection); return { sandbox: sandboxName, items, applied: true }; } @@ -395,7 +296,7 @@ export async function migrateMcpBridges( const current = (await inspectAgentMcpSources(sandbox, runtimeSelection)).native[ entry.server ]; - if (!current || !sameRegistration(current, entry)) { + if (!current || !sameMcpRegistration(current, entry)) { throw new McpBridgeError( `Native MCP verification failed after migrating '${entry.server}'.`, ); @@ -408,9 +309,7 @@ export async function migrateMcpBridges( await removeLegacyAgentMcpEntry(sandbox, entry, runtimeSelection); } } - // Force a normal non-MCP registry serialization so legacy MCP fields are - // omitted immediately after the explicit migration succeeds. - registry.updateSandbox(sandboxName, {}); + retireLegacyMcpRegistryProjection(sandboxName, legacyProjection); return { sandbox: sandboxName, items, applied: true }; } catch (error) { if (!cleanupStarted) { diff --git a/src/lib/actions/sandbox/mcp-bridge-rebuild.ts b/src/lib/actions/sandbox/mcp-bridge-rebuild.ts index 81019f1874f..bb5b05d45c9 100644 --- a/src/lib/actions/sandbox/mcp-bridge-rebuild.ts +++ b/src/lib/actions/sandbox/mcp-bridge-rebuild.ts @@ -32,7 +32,11 @@ import { } from "./mcp-bridge-provider"; import { restoreExistingMcpBridgeRuntime } from "./mcp-bridge-restart"; import { assertMcpAdapterTeardownRuntimeCapabilities } from "./mcp-bridge-runtime-capabilities"; -import { ensureSandboxGatewaySelected, getSandboxOrThrow } from "./mcp-bridge-state"; +import { + assertNoAmbiguousMcpCredentialTargets, + ensureSandboxGatewaySelected, + getSandboxOrThrow, +} from "./mcp-bridge-state"; import { assertAuthenticatedBridgeEntry, validateSandboxName } from "./mcp-bridge-validation"; export interface McpRebuildPreparation { @@ -100,6 +104,7 @@ async function getCompleteMcpRebuildEntries( const currentSandbox = getSandboxOrThrow(sandboxName); let runtimeSelection = options.runtimeSelection; const entries = sourceEntries.map(cloneMcpSourceEntry); + assertNoAmbiguousMcpCredentialTargets(entries); if (entries.length > 0) { runtimeSelection ??= getMcpProviderInspectionRuntimeSelection(currentSandbox); } diff --git a/src/lib/actions/sandbox/mcp-bridge-remove.ts b/src/lib/actions/sandbox/mcp-bridge-remove.ts index 42f98e8ed96..2298d94fb4b 100644 --- a/src/lib/actions/sandbox/mcp-bridge-remove.ts +++ b/src/lib/actions/sandbox/mcp-bridge-remove.ts @@ -3,12 +3,17 @@ import { assertHermesPortableCommandUnavailable } from "../../onboard/experimental/portable-agent-lifecycle"; import { withMcpLifecycleLock } from "../../state/mcp-lifecycle-lock"; +import { + readLegacyMcpRegistryProjection, + removeLegacyMcpRegistryEntry, +} from "../../state/registry/legacy-mcp"; import { assertAgentMcpTeardownRuntimeCapability, unregisterAgentAdapter, } from "./mcp-bridge-adapters"; import { isAgentMcpAdapter, McpBridgeError, type McpSourceEntry } from "./mcp-bridge-contracts"; import { removeGeneratedPolicy } from "./mcp-bridge-policy"; +import { readCommittedLegacyRegistryEntries, sameMcpRegistration } from "./mcp-bridge-source"; import { detachProvider, getMcpProviderInspectionRuntimeSelection, @@ -22,7 +27,12 @@ import { getSandboxAgent, getSandboxOrThrow, } from "./mcp-bridge-state"; -import { inspectPolicyOnlyMcpEntry, inspectSourceBridgeState } from "./mcp-bridge-source"; +import { + inspectPolicyOnlyMcpEntry, + inspectAgentMcpSources, + inspectSourceBridgeState, + removeLegacyAgentMcpEntry, +} from "./mcp-bridge-source"; import { resolvePersistedCredentialEnvForRedaction, validateMcpServerName, @@ -41,14 +51,66 @@ export async function removeMcpBridge( const sandbox = getSandboxOrThrow(sandboxName); const runtimeSelection = getMcpProviderInspectionRuntimeSelection(sandbox); const observed = await inspectSourceBridgeState(sandbox, runtimeSelection); - if (Object.keys(observed.sources.legacy).length > 0) { - throw new McpBridgeError( - `Legacy MCP agent configuration requires explicit migration. Run \`nemoclaw ${sandboxName} mcp migrate\` first.`, - 2, + const agent = getSandboxAgent(sandbox); + let entry: McpSourceEntry | undefined; + let removedLegacySource = false; + const legacyEntry = observed.sources.legacy[server]; + if (legacyEntry) { + const legacyProjection = readLegacyMcpRegistryProjection(sandboxName); + const committedEntries = readCommittedLegacyRegistryEntries( + sandboxName, + agent.name, + getBridgeAdapter(agent), + legacyProjection, + ); + const committedEntry = committedEntries[server]; + if ( + !legacyProjection || + !committedEntry || + !sameMcpRegistration(legacyEntry, committedEntry) + ) { + throw new McpBridgeError( + `Legacy MCP server '${server}' cannot be proven as registry-owned and was preserved. No source was changed.`, + 2, + ); + } + if (observed.sources.native[server]) { + throw new McpBridgeError( + `MCP server '${server}' exists in both legacy and native configuration. Migrate or explicitly resolve the duplicate before removal. No source was changed.`, + 2, + ); + } + const registryOnlyServers = Object.keys(committedEntries).filter( + (candidateServer) => + candidateServer !== server && observed.sources.legacy[candidateServer] === undefined, ); + if (registryOnlyServers.length > 0) { + throw new McpBridgeError( + `Legacy MCP server '${server}' cannot be removed while registry-only legacy registration(s) '${registryOnlyServers.join( + "', '", + )}' remain. Migrate or explicitly resolve them first. No source was changed.`, + 2, + ); + } + await ensureSandboxGatewaySelected(sandboxName, runtimeSelection); + const legacyAdapter = isAgentMcpAdapter(committedEntry.adapter) + ? committedEntry.adapter + : getBridgeAdapter(agent); + await assertAgentMcpTeardownRuntimeCapability(sandboxName, legacyAdapter, runtimeSelection); + await removeLegacyAgentMcpEntry(sandbox, legacyEntry, runtimeSelection); + const remaining = await inspectAgentMcpSources(sandbox, runtimeSelection); + if (remaining.legacy[server] || remaining.native[server]) { + throw new McpBridgeError( + `MCP server '${server}' remains in agent configuration. Registry ownership, policy, and provider state were preserved.`, + 2, + ); + } + removeLegacyMcpRegistryEntry(sandboxName, server, legacyProjection); + entry = committedEntry; + removedLegacySource = true; + } else { + entry = observed.bridges[server]; } - const agent = getSandboxAgent(sandbox); - let entry: McpSourceEntry | undefined = observed.bridges[server]; if (!entry && agent.mcpCapability.adapter) { entry = (await inspectPolicyOnlyMcpEntry( @@ -59,6 +121,61 @@ export async function removeMcpBridge( runtimeSelection, )) ?? undefined; } + // A failed registry write may leave ownership after the agent entry is gone. + // Reconcile that exact row on retry, without treating it as runtime state. + if (!legacyEntry && !observed.sources.native[server]) { + const legacyProjection = readLegacyMcpRegistryProjection(sandboxName); + const committedEntry = legacyProjection + ? readCommittedLegacyRegistryEntries( + sandboxName, + agent.name, + getBridgeAdapter(agent), + legacyProjection, + )[server] + : undefined; + if (legacyProjection && committedEntry) { + if ( + entry && + (!sameMcpRegistration(entry, committedEntry) || + entry.providerName !== committedEntry.providerName || + entry.providerId !== committedEntry.providerId) + ) { + throw new McpBridgeError( + `Legacy MCP server '${server}' no longer matches its live policy and provider. Ownership and resources were preserved.`, + 2, + ); + } + const provider = await inspectMcpProvider(committedEntry.providerName, runtimeSelection); + if (provider.exists === null) { + throw new McpBridgeError( + `Legacy MCP server '${server}' provider identity could not be inspected. Ownership and resources were preserved.`, + 2, + ); + } + if (provider.exists && provider.id !== committedEntry.providerId) { + throw new McpBridgeError( + `Legacy MCP server '${server}' provider identity changed. Ownership and resources were preserved.`, + 2, + ); + } + await ensureSandboxGatewaySelected(sandboxName, runtimeSelection); + await assertAgentMcpTeardownRuntimeCapability( + sandboxName, + getBridgeAdapter(agent), + runtimeSelection, + ); + const remaining = await inspectAgentMcpSources(sandbox, runtimeSelection); + if (remaining.legacy[server] || remaining.native[server]) { + throw new McpBridgeError( + `MCP server '${server}' reappeared before cleanup. Ownership and resources were preserved.`, + 2, + ); + } + removeLegacyMcpRegistryEntry(sandboxName, server, legacyProjection); + entry = committedEntry; + removedLegacySource = true; + } + } if (!entry) { if (!options.force) { throw new McpBridgeError( @@ -74,12 +191,16 @@ export async function removeMcpBridge( ? entry.adapter : getBridgeAdapter(getSandboxAgent(sandbox)); - await assertAgentMcpTeardownRuntimeCapability(sandboxName, adapter, runtimeSelection); - const removal = await unregisterAgentAdapter(sandboxName, adapter, entry, runtimeSelection, { - force: options.force === true, - envValues: resolvePersistedCredentialEnvForRedaction(entry.env), - teardown: true, - }); + if (!removedLegacySource) { + await assertAgentMcpTeardownRuntimeCapability(sandboxName, adapter, runtimeSelection); + } + const removal = removedLegacySource + ? "removed" + : await unregisterAgentAdapter(sandboxName, adapter, entry, runtimeSelection, { + force: options.force === true, + envValues: resolvePersistedCredentialEnvForRedaction(entry.env), + teardown: true, + }); if (removal === "unowned" && !options.force) { throw new McpBridgeError( `The native MCP server '${server}' changed before removal. Rerun against the current agent configuration.`, diff --git a/src/lib/actions/sandbox/mcp-bridge-restart-recovery.test.ts b/src/lib/actions/sandbox/mcp-bridge-restart-recovery.test.ts index 3c217a5d39b..ea310bc4873 100644 --- a/src/lib/actions/sandbox/mcp-bridge-restart-recovery.test.ts +++ b/src/lib/actions/sandbox/mcp-bridge-restart-recovery.test.ts @@ -58,7 +58,8 @@ vi.mock("./mcp-bridge-runtime-capabilities", () => ({ assertMcpAdapterMutationRuntimeCapabilities: vi.fn(), assertMcpAdapterTeardownRuntimeCapabilities: vi.fn(), })); -vi.mock("./mcp-bridge-state", () => ({ +vi.mock("./mcp-bridge-state", async (importOriginal) => ({ + ...(await importOriginal()), ensureSandboxGatewaySelected: vi.fn(), getBridgeAdapter: mocks.getBridgeAdapter, getSandboxAgent: mocks.getSandboxAgent, @@ -229,4 +230,23 @@ describe("OpenClaw MCP partial-mutation recovery", () => { expect(mocks.reload).toHaveBeenCalledOnce(); expect(mocks.reload).toHaveBeenCalledWith("alpha", ["openclaw-config", "openclaw-config"]); }); + + it("rejects an ambiguous recovery batch before target preflight or mutation", async () => { + const ambiguousEntries = [ + entries[0], + { + ...entries[1], + url: entries[0].url, + }, + ]; + + await expect(restoreExistingMcpBridgeRuntime("alpha", ambiguousEntries)).rejects.toThrow( + /cannot safely choose between credentials for an indistinguishable endpoint/, + ); + + expect(mocks.preflightTargets).not.toHaveBeenCalled(); + expect(mocks.register).not.toHaveBeenCalled(); + expect(mocks.refresh).not.toHaveBeenCalled(); + expect(mocks.reload).not.toHaveBeenCalled(); + }); }); diff --git a/src/lib/actions/sandbox/mcp-bridge-restart.ts b/src/lib/actions/sandbox/mcp-bridge-restart.ts index 6c8aaeead33..f4230f6e618 100644 --- a/src/lib/actions/sandbox/mcp-bridge-restart.ts +++ b/src/lib/actions/sandbox/mcp-bridge-restart.ts @@ -32,6 +32,7 @@ import { assertMcpAdapterTeardownRuntimeCapabilities, } from "./mcp-bridge-runtime-capabilities"; import { + assertNoAmbiguousMcpCredentialTargets, ensureSandboxGatewaySelected, getBridgeAdapter, getSandboxAgent, @@ -98,6 +99,7 @@ async function restartMcpBridgeUnlocked(sandboxName: string, server?: string): P const agent = getSandboxAgent(sandbox); const adapter = getBridgeAdapter(agent); const bridges = observed.bridges; + assertNoAmbiguousMcpCredentialTargets(Object.values(bridges)); const targets = server ? [[server, bridges[server]] as const] : Object.entries(bridges); if (targets.length === 0) { console.log(` No MCP servers for sandbox '${sandboxName}'.`); @@ -197,6 +199,7 @@ export async function restoreExistingMcpBridgeRuntime( ): Promise { if (entries.length === 0) return; for (const entry of entries) assertAuthenticatedBridgeEntry(entry); + assertNoAmbiguousMcpCredentialTargets(entries); const resolvedByServer = await preflightMcpEntryTargets(entries); if (options.lifecyclePhase !== "teardown-rollback") { assertMcpCredentialBoundaryRuntimeVersion(); diff --git a/src/lib/actions/sandbox/mcp-bridge-source.ts b/src/lib/actions/sandbox/mcp-bridge-source.ts index 3fc5966c27c..e2c920ba9ac 100644 --- a/src/lib/actions/sandbox/mcp-bridge-source.ts +++ b/src/lib/actions/sandbox/mcp-bridge-source.ts @@ -2,6 +2,8 @@ // SPDX-License-Identifier: Apache-2.0 import path from "node:path"; +import { isDeepStrictEqual } from "node:util"; +import { readLegacyMcpRegistryProjection } from "../../state/registry/legacy-mcp"; import YAML from "yaml"; @@ -22,7 +24,130 @@ import { import { executeSandboxCommand } from "./process-recovery"; import { quoteMcpBridgeShellArg } from "./mcp-bridge-runtime-command"; import { redactBridgeFailureForDisplay } from "./mcp-bridge-output"; -import { buildMcpBridgeProviderName } from "./mcp-bridge-validation"; +import { buildMcpBridgeProviderName, normalizeMcpDenyTools } from "./mcp-bridge-validation"; + +export function sameMcpRegistration(left: McpSourceEntry, right: McpSourceEntry): boolean { + return ( + left.server === right.server && left.url === right.url && isDeepStrictEqual(left.env, right.env) + ); +} + +export function assertNoLegacyMcpSources( + sandboxName: string, + legacySources: Readonly>, + operation: string, +): void { + const legacyNames = Object.keys(legacySources).sort(); + if (legacyNames.length === 0) return; + throw new McpBridgeError( + `Legacy MCP agent configuration requires explicit migration for '${legacyNames.join(", ")}'. Run \`nemoclaw ${sandboxName} mcp migrate\` to preview it before ${operation}.`, + 2, + ); +} + +export function readCommittedLegacyRegistryEntries( + sandboxName: string, + currentAgent: string, + currentAdapter: McpSourceEntry["adapter"], + rawState = readLegacyMcpRegistryProjection(sandboxName), +): Record { + if (!rawState) return {}; + if (rawState.destroyPreparedAt || rawState.destroyPendingAt) { + throw new McpBridgeError( + `Legacy MCP registry state for '${sandboxName}' contains an incomplete destroy transaction. No source was changed.`, + 2, + ); + } + if (!isObjectRecord(rawState.bridges)) return {}; + const entries: Record = {}; + for (const [server, raw] of Object.entries(rawState.bridges)) { + if (!/^[A-Za-z][A-Za-z0-9_-]{0,63}$/u.test(server) || !isObjectRecord(raw)) { + throw new McpBridgeError( + `Legacy MCP registry server '${server}' is not a valid committed registration. No source was changed.`, + 2, + ); + } + if (raw.addState !== undefined) { + throw new McpBridgeError( + `Legacy MCP registry server '${server}' contains an incomplete add transaction. No source was changed.`, + 2, + ); + } + const agent = typeof raw.agent === "string" && raw.agent ? raw.agent : "openclaw"; + const recordedAdapter = + typeof raw.adapter === "string" && raw.adapter ? raw.adapter : currentAdapter; + const adapter = recordedAdapter === "mcporter" ? "openclaw-config" : recordedAdapter; + if (agent !== currentAgent || adapter !== currentAdapter) { + throw new McpBridgeError( + `Legacy MCP registry server '${server}' targets ${agent}/${String(adapter)} instead of the current ${currentAgent}/${String(currentAdapter)} runtime. No source was changed.`, + 2, + ); + } + if ( + typeof raw.url !== "string" || + raw.url.length > 4096 || + !Array.isArray(raw.env) || + raw.env.length !== 1 || + typeof raw.env[0] !== "string" || + !/^[A-Za-z_][A-Za-z0-9_]{0,127}$/u.test(raw.env[0]) + ) { + throw new McpBridgeError( + `Legacy MCP registry server '${server}' is not a valid committed registration. No source was changed.`, + 2, + ); + } + let url: URL; + try { + url = new URL(raw.url); + } catch { + throw new McpBridgeError( + `Legacy MCP registry server '${server}' has an invalid URL. No source was changed.`, + 2, + ); + } + if (url.protocol !== "https:" || url.username || url.password) { + throw new McpBridgeError( + `Legacy MCP registry server '${server}' has an unsupported URL. No source was changed.`, + 2, + ); + } + const requestedDenyTools = raw.pendingDenyTools ?? raw.denyTools ?? []; + if ( + !Array.isArray(requestedDenyTools) || + requestedDenyTools.some((tool) => typeof tool !== "string") + ) { + throw new McpBridgeError( + `Legacy MCP registry server '${server}' has invalid denied-tool intent. No source was changed.`, + 2, + ); + } + const denyTools = normalizeMcpDenyTools(requestedDenyTools as string[]); + const allowedIps = Array.isArray(raw.allowedIps) + ? raw.allowedIps.filter((address): address is string => typeof address === "string") + : undefined; + entries[server] = { + server, + agent, + adapter, + url: url.toString(), + env: [raw.env[0]], + denyTools, + ...(allowedIps?.length ? { allowedIps } : {}), + ...(typeof raw.trustedPrivateHost === "string" && raw.trustedPrivateHost + ? { trustedPrivateHost: raw.trustedPrivateHost } + : {}), + ...(typeof raw.providerName === "string" && raw.providerName + ? { providerName: raw.providerName } + : {}), + ...(typeof raw.providerId === "string" && raw.providerId + ? { providerId: raw.providerId } + : {}), + policyName: buildMcpBridgePolicyName(server), + source: "legacy-registry", + }; + } + return entries; +} export interface AgentMcpSourceSnapshot { native: Record; diff --git a/src/lib/actions/sandbox/mcp-bridge-state.test.ts b/src/lib/actions/sandbox/mcp-bridge-state.test.ts new file mode 100644 index 00000000000..a8044c28bfc --- /dev/null +++ b/src/lib/actions/sandbox/mcp-bridge-state.test.ts @@ -0,0 +1,48 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +import { describe, expect, it } from "vitest"; + +import type { McpSourceEntry } from "./mcp-bridge-contracts"; +import { assertNoAmbiguousMcpCredentialTarget } from "./mcp-bridge-state"; + +const existing: McpSourceEntry = { + server: "alpha", + agent: "openclaw", + adapter: "openclaw-config", + url: "https://api.githubcopilot.com/mcp/", + env: ["AAA_TOK"], + providerName: "sandbox-mcp-alpha", + policyName: "mcp-bridge-alpha", +}; + +describe("managed MCP credential target ambiguity", () => { + it("rejects a second credential binding for the same endpoint (#11895)", () => { + expect(() => + assertNoAmbiguousMcpCredentialTarget( + { alpha: existing }, + "beta", + existing.url, + "sandbox-mcp-beta", + ), + ).toThrow(/cannot safely choose between credentials for an indistinguishable endpoint/); + }); + + it("allows separately credentialed servers at distinct endpoints (#11895)", () => { + expect(() => + assertNoAmbiguousMcpCredentialTarget( + { alpha: existing }, + "beta", + "https://mcp.example.com/api/", + "sandbox-mcp-beta", + ), + ).not.toThrow(); + }); + + it("allows unauthenticated aliases because they do not compete for credentials (#11895)", () => { + const anonymous = { ...existing, env: [], providerName: undefined }; + expect(() => + assertNoAmbiguousMcpCredentialTarget({ alpha: anonymous }, "beta", anonymous.url, undefined), + ).not.toThrow(); + }); +}); diff --git a/src/lib/actions/sandbox/mcp-bridge-state.ts b/src/lib/actions/sandbox/mcp-bridge-state.ts index f84244170bc..ec3d4c8e90d 100644 --- a/src/lib/actions/sandbox/mcp-bridge-state.ts +++ b/src/lib/actions/sandbox/mcp-bridge-state.ts @@ -8,6 +8,10 @@ import { replaceOpenShellRuntimeSelectionEnv, } from "../../gateway-runtime-action"; import type { SandboxEntry } from "../../state/registry"; +import { + findAmbiguousMcpCredentialTarget, + type McpCredentialTarget, +} from "../../domain/mcp-credential-target"; import type { McpSourceEntry } from "./mcp-bridge-contracts"; import * as registry from "../../state/registry"; import { getSandboxTargetGatewayName } from "./gateway-target"; @@ -94,6 +98,27 @@ export function assertNoDerivedResourceCollision( } } +/** Reject credential-bound aliases whose requests have no distinct network selector. */ +export function assertNoAmbiguousMcpCredentialTargets( + entries: readonly McpCredentialTarget[], +): void { + const ambiguous = findAmbiguousMcpCredentialTarget(entries); + if (!ambiguous) return; + throw new McpBridgeError( + `MCP servers '${ambiguous.entry.server}' and '${ambiguous.conflict.server}' target the same URL with different credential bindings. OpenShell cannot safely choose between credentials for an indistinguishable endpoint. Use one managed server definition for this URL or a distinct endpoint.`, + 2, + ); +} + +export function assertNoAmbiguousMcpCredentialTarget( + bridges: Readonly>, + server: string, + url: string, + providerName: string | undefined, +): void { + assertNoAmbiguousMcpCredentialTargets([...Object.values(bridges), { server, url, providerName }]); +} + export async function ensureSandboxGatewaySelected( sandboxName: string, runtimeSelection: OpenShellRuntimeSelection, diff --git a/src/lib/actions/sandbox/mcp-bridge-status-resolution.test.ts b/src/lib/actions/sandbox/mcp-bridge-status-resolution.test.ts index 13c616d875a..c7cf8fcab27 100644 --- a/src/lib/actions/sandbox/mcp-bridge-status-resolution.test.ts +++ b/src/lib/actions/sandbox/mcp-bridge-status-resolution.test.ts @@ -405,6 +405,7 @@ describeConcurrentProbeSuite("MCP status wire-level credential-resolution probe" writeHarnessResult(JSON.stringify({ attachmentInspections: providerAttachmentInspectionCount, attached: statuses.map((status) => status.provider.attached), + supported: statuses.map((status) => status.support.supported), })); `, ); @@ -412,6 +413,7 @@ describeConcurrentProbeSuite("MCP status wire-level credential-resolution probe" expect(JSON.parse(stdout)).toEqual({ attachmentInspections: 1, attached: [true, true], + supported: [true, true], }); }); diff --git a/src/lib/actions/sandbox/rebuild-flow-lifecycle.test.ts b/src/lib/actions/sandbox/rebuild-flow-lifecycle.test.ts index ac62c2b7744..1230cda9085 100644 --- a/src/lib/actions/sandbox/rebuild-flow-lifecycle.test.ts +++ b/src/lib/actions/sandbox/rebuild-flow-lifecycle.test.ts @@ -768,6 +768,29 @@ describe("rebuildSandbox flow: lifecycle", () => { expect(harness.onboardSpy).toHaveBeenCalledOnce(); }); + it("stops a legacy-only MCP rebuild before teardown or sandbox deletion", async () => { + const harness = createRebuildFlowHarness({ + mcpLegacySources: [ + { + server: "github", + agent: "openclaw", + adapter: "mcporter-config", + url: "https://mcp.example.test/mcp", + env: ["GITHUB_TOKEN"], + }, + ], + }); + + await expect( + harness.rebuildSandbox("alpha", ["--yes"], { throwOnError: true }), + ).rejects.toThrow("Legacy MCP agent configuration requires explicit migration for 'github'"); + + expect(harness.prepareMcpBridgesForRebuildSpy).not.toHaveBeenCalled(); + expect(harness.backupSandboxStateSpy).not.toHaveBeenCalled(); + expect(harness.onboardSpy).not.toHaveBeenCalled(); + expectNoSandboxDelete(harness.runOpenshellSpy); + }); + it("keeps the journaled row when replacement creation fails (#7734)", async () => { const harness = createRebuildFlowHarness({ onboard: () => { diff --git a/src/lib/actions/sandbox/rebuild-mcp-phase.ts b/src/lib/actions/sandbox/rebuild-mcp-phase.ts index 40a50e7a8cd..d38ddba2373 100644 --- a/src/lib/actions/sandbox/rebuild-mcp-phase.ts +++ b/src/lib/actions/sandbox/rebuild-mcp-phase.ts @@ -17,7 +17,11 @@ import type { RebuildBail } from "./rebuild-credential-preflight"; import { type RebuildSandboxEntry, resolveSandboxGatewayName } from "./rebuild-flow-helpers"; import type { McpProviderInspectionRuntimeSelection } from "./mcp-bridge-provider"; import { getMcpProviderInspectionRuntimeSelection } from "./mcp-bridge-provider"; -import { inspectAgentMcpSources, joinMcpEntriesToOpenShell } from "./mcp-bridge-source"; +import { + assertNoLegacyMcpSources, + inspectAgentMcpSources, + joinMcpEntriesToOpenShell, +} from "./mcp-bridge-source"; import type { McpSourceEntry } from "./mcp-bridge-contracts"; export type McpRebuildPreparation = Awaited>; @@ -36,6 +40,7 @@ export async function observeMcpStateForRebuild( workspace: OPENSHELL_DEFAULT_WORKSPACE, }; const sources = await inspectAgentMcpSources(sandbox, sourceRuntime); + assertNoLegacyMcpSources(sandbox.name, sources.legacy, "rebuilding"); if (Object.keys(sources.native).length === 0) return { entries: [] }; const selectedRuntime = runtimeSelection ?? getMcpProviderInspectionRuntimeSelection(sandbox); const entries = Object.values( diff --git a/src/lib/domain/mcp-credential-target.test.ts b/src/lib/domain/mcp-credential-target.test.ts new file mode 100644 index 00000000000..edccb19fba0 --- /dev/null +++ b/src/lib/domain/mcp-credential-target.test.ts @@ -0,0 +1,55 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +import { describe, expect, it } from "vitest"; + +import { findAmbiguousMcpCredentialTarget } from "./mcp-credential-target"; + +describe("managed MCP credential targets", () => { + it("finds distinct credential providers bound to the same endpoint", () => { + expect( + findAmbiguousMcpCredentialTarget([ + { server: "alpha", url: "https://mcp.example.test/", providerName: "alpha-provider" }, + { server: "beta", url: "https://mcp.example.test/", providerName: "beta-provider" }, + ]), + ).toMatchObject({ entry: { server: "beta" }, conflict: { server: "alpha" } }); + }); + + it("treats query-only URL variants as the same enforced endpoint", () => { + expect( + findAmbiguousMcpCredentialTarget([ + { + server: "alpha", + url: "https://mcp.example.test/mcp?tenant=alpha", + providerName: "alpha-provider", + }, + { + server: "beta", + url: "https://mcp.example.test/mcp?tenant=beta", + providerName: "beta-provider", + }, + ]), + ).toMatchObject({ entry: { server: "beta" }, conflict: { server: "alpha" } }); + }); + + it("allows distinct endpoints and unauthenticated aliases", () => { + expect( + findAmbiguousMcpCredentialTarget([ + { server: "alpha", url: "https://alpha.example.test/", providerName: "alpha-provider" }, + { server: "beta", url: "https://beta.example.test/", providerName: "beta-provider" }, + ]), + ).toBeNull(); + expect( + findAmbiguousMcpCredentialTarget([ + { server: "alpha", url: "https://mcp.example.test/" }, + { server: "beta", url: "https://mcp.example.test/" }, + ]), + ).toBeNull(); + expect( + findAmbiguousMcpCredentialTarget([ + { server: "alpha", url: "https://mcp.example.test/" }, + { server: "beta", url: "https://mcp.example.test/", providerName: "beta-provider" }, + ]), + ).toBeNull(); + }); +}); diff --git a/src/lib/domain/mcp-credential-target.ts b/src/lib/domain/mcp-credential-target.ts new file mode 100644 index 00000000000..7be8f84cc7d --- /dev/null +++ b/src/lib/domain/mcp-credential-target.ts @@ -0,0 +1,45 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +export type McpCredentialTarget = { + server: string; + url: string; + providerName?: string; +}; + +export type AmbiguousMcpCredentialTarget = { + entry: McpCredentialTarget; + conflict: McpCredentialTarget; +}; + +function credentialPolicyEndpointKey(rawUrl: string): string { + try { + const url = new URL(rawUrl); + const port = url.port || (url.protocol === "https:" ? "443" : "80"); + return JSON.stringify([url.hostname, port, url.pathname || "/"]); + } catch { + // Invalid legacy values still retain the previous exact-string comparison. + return rawUrl; + } +} + +/** Find credential-bound definitions that cannot be distinguished by endpoint. */ +export function findAmbiguousMcpCredentialTarget( + entries: readonly McpCredentialTarget[], +): AmbiguousMcpCredentialTarget | null { + for (const [index, entry] of entries.entries()) { + const endpointKey = credentialPolicyEndpointKey(entry.url); + const conflict = entries + .slice(0, index) + .find( + (candidate) => + candidate.server !== entry.server && + credentialPolicyEndpointKey(candidate.url) === endpointKey && + candidate.providerName !== undefined && + entry.providerName !== undefined && + candidate.providerName !== entry.providerName, + ); + if (conflict) return { entry, conflict }; + } + return null; +} diff --git a/src/lib/sandbox/version.test.ts b/src/lib/sandbox/version.test.ts index a7b43c13581..4c726cddbf5 100644 --- a/src/lib/sandbox/version.test.ts +++ b/src/lib/sandbox/version.test.ts @@ -1,7 +1,7 @@ // SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. // SPDX-License-Identifier: Apache-2.0 -import { mkdirSync, mkdtempSync, rmSync, writeFileSync } from "node:fs"; +import { mkdirSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; import { dirname, join } from "node:path"; import { afterAll, afterEach, beforeEach, describe, expect, it, vi } from "vitest"; @@ -161,6 +161,37 @@ describe("checkAgentVersion", async () => { expect(updated?.agentVersion).toBe("2026.5.27"); }); + it.each(["test-sb", "other-sb"])( + "preserves %s legacy ownership while reporting the live version", + async (legacySandbox) => { + const document = { + sandboxes: { + "test-sb": { name: "test-sb", agent: "openclaw" }, + [legacySandbox]: { + name: legacySandbox, + agent: "openclaw", + mcp: { bridges: { github: { providerId: "owned" } } }, + }, + }, + defaultSandbox: "test-sb", + }; + writeFileSync(TEST_REGISTRY_FILE, JSON.stringify(document)); + runSsh.mockResolvedValue({ + kind: "completed", + exitCode: 0, + stdout: "OpenClaw 2026.5.27", + stderr: "", + }); + expect(await checkAgentVersion("test-sb")).toMatchObject({ + sandboxVersion: "2026.5.27", + detectionMethod: "ssh-exec", + verificationFailed: false, + }); + expect(JSON.parse(readFileSync(TEST_REGISTRY_FILE, "utf8"))).toMatchObject(document); + expect(registry.getSandbox("test-sb")?.agentVersion).toBe("2026.5.27"); + }, + ); + it("probes the sandbox's own recorded gateway, not OpenShell's ambient selection (#7429)", async () => { // A sandbox onboarded under a non-default NEMOCLAW_GATEWAY_PORT is // registered against `nemoclaw-`. `openshell sandbox get` / diff --git a/src/lib/sandbox/version.ts b/src/lib/sandbox/version.ts index 6f0f38b5ad5..bb9ce93d25b 100644 --- a/src/lib/sandbox/version.ts +++ b/src/lib/sandbox/version.ts @@ -205,7 +205,7 @@ export async function checkAgentVersion( // Slow path: SSH exec into sandbox const probed = await probeAgentVersion(sandboxName, probeGatewayName); if (probed && sb) { - // Cache for future fast-path lookups + // Registry persistence preserves any disk-only legacy ownership evidence. registry.updateSandbox(sandboxName, { agentVersion: probed }); } diff --git a/src/lib/state/registry-normalization.test.ts b/src/lib/state/registry-normalization.test.ts index 1e337076a9e..9b3078c9788 100644 --- a/src/lib/state/registry-normalization.test.ts +++ b/src/lib/state/registry-normalization.test.ts @@ -115,6 +115,47 @@ describe("sandbox registry normalization", () => { expect(persisted.sandboxes?.alpha).not.toHaveProperty("cuaRuntimeReadiness"); }); + it.each(["alpha", "beta"])( + "preserves legacy ownership across an unrelated update to %s until explicit retirement", + async (updatedSandbox) => { + const legacy = { bridges: { github: { providerId: "owned-provider" } } }; + const { home, registry } = await loadRegistryDocument({ + sandboxes: { alpha: { name: "alpha", mcp: legacy }, beta: { name: "beta" } }, + }); + const { readLegacyMcpRegistryProjection, removeLegacyMcpRegistryEntry } = + await import("./registry/legacy-mcp"); + + expect(registry.updateSandbox(updatedSandbox, { model: "replacement" })).toBe(true); + expect(readLegacyMcpRegistryProjection("alpha")).toEqual(legacy); + expect(registry.getSandbox("alpha")).not.toHaveProperty("mcp"); + expect(registry.getSandbox(updatedSandbox)?.model).toBe("replacement"); + const staleRuntimeSnapshot = registry.load(); + removeLegacyMcpRegistryEntry("alpha", "github", legacy); + registry.save(staleRuntimeSnapshot); + registry.updateSandbox(updatedSandbox, { agentVersion: "new-version" }); + expect(readLegacyMcpRegistryProjection("alpha")).toBeUndefined(); + const persisted = JSON.parse( + fs.readFileSync(path.join(home, ".nemoclaw", "sandboxes.json"), "utf8"), + ); + expect(persisted.sandboxes.alpha).not.toHaveProperty("mcp"); + expect(persisted.sandboxes[updatedSandbox].agentVersion).toBe("new-version"); + }, + ); + + it("preserves only persisted legacy ownership, not a caller-supplied replacement", async () => { + const legacy = { bridges: { github: { providerId: "owned-provider" } } }; + const { registry } = await loadRegistryDocument({ + sandboxes: { alpha: { name: "alpha", mcp: legacy }, beta: { name: "beta" } }, + }); + const { readLegacyMcpRegistryProjection } = await import("./registry/legacy-mcp"); + const updates = { model: "replacement", mcp: { bridges: {} } }; + registry.updateSandbox("alpha", updates); + registry.updateSandbox("beta", updates); + expect(readLegacyMcpRegistryProjection("alpha")).toEqual(legacy); + expect(readLegacyMcpRegistryProjection("beta")).toBeUndefined(); + expect(registry.getSandbox("alpha")).not.toHaveProperty("mcp"); + }); + it("preserves a stale pointer for diagnostics but repairs it on registration", async () => { const registry = await loadRegistryWith({ mismatched: { name: "different" } }, "mismatched"); diff --git a/src/lib/state/registry/legacy-mcp.ts b/src/lib/state/registry/legacy-mcp.ts new file mode 100644 index 00000000000..d6875dc52e6 --- /dev/null +++ b/src/lib/state/registry/legacy-mcp.ts @@ -0,0 +1,88 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +import { isDeepStrictEqual } from "node:util"; +import { readConfigFile, writeConfigFile } from "../config-io"; +import { withLock } from "./lock"; +import { REGISTRY_FILE } from "./persistence"; + +function isObjectRecord(value: unknown): value is Record { + return typeof value === "object" && value !== null && !Array.isArray(value); +} + +/** Read deprecated ownership evidence without admitting it to runtime registry state. */ +export function readLegacyMcpRegistryProjection( + sandboxName: string, +): Record | undefined { + const document = readConfigFile(REGISTRY_FILE, {}); + if (!isObjectRecord(document) || !isObjectRecord(document.sandboxes)) return undefined; + const sandbox = document.sandboxes[sandboxName]; + return isObjectRecord(sandbox) && isObjectRecord(sandbox.mcp) ? sandbox.mcp : undefined; +} + +/** Retire exactly the ownership snapshot whose migration completed successfully. */ +export function retireLegacyMcpRegistryProjection( + sandboxName: string, + expectedProjection: Record | undefined, +): void { + withLock(() => { + const document = readConfigFile(REGISTRY_FILE, {}); + const sandbox = + isObjectRecord(document) && isObjectRecord(document.sandboxes) + ? document.sandboxes[sandboxName] + : undefined; + const current = isObjectRecord(sandbox) ? sandbox.mcp : undefined; + if (!isDeepStrictEqual(current, expectedProjection)) { + throw new Error("Legacy MCP ownership changed during migration; registry was preserved."); + } + if ( + current === undefined || + !isObjectRecord(document) || + !isObjectRecord(document.sandboxes) || + !isObjectRecord(sandbox) + ) + return; + const nextSandbox = { ...sandbox }; + delete nextSandbox.mcp; + writeConfigFile(REGISTRY_FILE, { + ...document, + sandboxes: { ...document.sandboxes, [sandboxName]: nextSandbox }, + }); + }); +} + +/** Retire only the removed entry's unchanged ownership proof, preserving survivors. */ +export function removeLegacyMcpRegistryEntry( + sandboxName: string, + server: string, + expectedProjection: Record, +): void { + withLock(() => { + const document = readConfigFile(REGISTRY_FILE, {}); + if (!isObjectRecord(document) || !isObjectRecord(document.sandboxes)) { + throw new Error("Legacy MCP ownership changed during removal; registry was preserved."); + } + const sandbox = document.sandboxes[sandboxName]; + if ( + !isObjectRecord(sandbox) || + !isObjectRecord(sandbox.mcp) || + !isDeepStrictEqual(sandbox.mcp, expectedProjection) || + !isObjectRecord(sandbox.mcp.bridges) || + !Object.hasOwn(sandbox.mcp.bridges, server) + ) { + throw new Error("Legacy MCP ownership changed during removal; registry was preserved."); + } + const bridges = { ...sandbox.mcp.bridges }; + delete bridges[server]; + const nextSandbox = { ...sandbox }; + if (Object.keys(bridges).length > 0) { + nextSandbox.mcp = { ...sandbox.mcp, bridges }; + } else { + delete nextSandbox.mcp; + } + writeConfigFile(REGISTRY_FILE, { + ...document, + sandboxes: { ...document.sandboxes, [sandboxName]: nextSandbox }, + }); + }); +} diff --git a/src/lib/state/registry/persistence.ts b/src/lib/state/registry/persistence.ts index 9fe7d3825e9..a4efa15b2da 100644 --- a/src/lib/state/registry/persistence.ts +++ b/src/lib/state/registry/persistence.ts @@ -105,7 +105,20 @@ export function load(): SandboxRegistry { } export function save(data: SandboxRegistry): void { - writeConfigFile(REGISTRY_FILE, serializeRegistryForDisk(data)); + const serialized = serializeRegistryForDisk(data); + const previous = readConfigFile(REGISTRY_FILE, {}); + // Legacy MCP ownership is disk-only compatibility evidence, not runtime + // authority. Ordinary writes must retain it until an explicit migration or + // verified removal retires it. Never accept a caller-supplied replacement. + if (isObjectRecord(previous) && isObjectRecord(previous.sandboxes)) { + for (const [name, entry] of Object.entries(serialized.sandboxes)) { + const prior = previous.sandboxes[name]; + if (isObjectRecord(prior) && Object.hasOwn(prior, "mcp")) { + Object.assign(entry, { mcp: prior.mcp }); + } + } + } + writeConfigFile(REGISTRY_FILE, serialized); } function normalizeRegistry(value: unknown): SandboxRegistry { diff --git a/test/e2e/live/mcp-bridge-reliability.ts b/test/e2e/live/mcp-bridge-reliability.ts index 6242b18b12a..5a2cd1443a3 100644 --- a/test/e2e/live/mcp-bridge-reliability.ts +++ b/test/e2e/live/mcp-bridge-reliability.ts @@ -1,16 +1,34 @@ // SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. // SPDX-License-Identifier: Apache-2.0 -import { buildHermesMcpStatusCommand } from "../../../src/lib/actions/sandbox/mcp-bridge-adapter-status"; +import { expect } from "vitest"; +import { MCP_MUTATION_TIMEOUT_MS, type McpAdapter } from "./mcp-bridge-cleanup.ts"; +import { applyMcpHostPolicyEdit } from "./mcp-bridge-sandbox.ts"; +import { + buildHermesMcpStatusCommand, + buildOpenClawMcpInspectCommand, +} from "../../../src/lib/actions/sandbox/mcp-bridge-adapter-status"; import { buildMcpCredentialRevisionObservationCommand } from "../../../src/lib/actions/sandbox/mcp-bridge-provider"; import type { McpAttachedCredentialRevision } from "../../../src/lib/actions/sandbox/mcp-bridge-provider-readiness"; -import type { McpSourceEntry } from "../../../src/lib/actions/sandbox/mcp-bridge-contracts"; +import type { + McpBridgeStatus, + McpSourceEntry, +} from "../../../src/lib/actions/sandbox/mcp-bridge-contracts"; +import { + buildMcpBridgePolicyKey, + buildMcpBridgePolicyName, +} from "../../../src/lib/actions/sandbox/mcp-bridge-policy-render"; +import { buildMcpBridgeProviderName } from "../../../src/lib/actions/sandbox/mcp-bridge-validation"; import { shellQuote } from "../../../src/lib/core/shell-quote"; import { buildAvailabilityProbeEnv } from "../fixtures/availability-env.ts"; import type { ArtifactSink } from "../fixtures/artifacts.ts"; import { assertExitZero, resultText } from "../fixtures/clients/command.ts"; import type { HostCliClient } from "../fixtures/clients/host.ts"; -import { type SandboxClient, trustedSandboxShellScript } from "../fixtures/clients/sandbox.ts"; +import { + type SandboxClient, + sandboxAccessEnv, + trustedSandboxShellScript, +} from "../fixtures/clients/sandbox.ts"; import { MCP_BRIDGE_TEST_CREDENTIALS } from "../fixtures/mcp-bridge-credentials.ts"; import type { ShellProbeResult } from "../fixtures/shell-probe.ts"; import { runBoundedRetry, type RetryEvidence } from "../../../tools/e2e/retry-evidence.mts"; @@ -23,6 +41,77 @@ import { import { MCP_PROVIDER_REWRITE_PROBE_SOURCE } from "./mcp-provider-rewrite-probe.ts"; import { FAKE_MCP_STATUS_RESULT_TOKEN } from "./mcp-bridge-servers.ts"; +export async function addBridgeAndReadStatus( + host: HostCliClient, + sandbox: SandboxClient, + options: { + sandboxName: string; + mcpUrl: string; + expectedAdapter: McpAdapter; + artifactPrefix: string; + credential?: string; + serverName?: string; + credentialEnvName?: string; + applyHostPolicyEdit?: boolean; + }, +): Promise { + await (options.applyHostPolicyEdit === false + ? Promise.resolve() + : applyMcpHostPolicyEdit(sandbox, options)); + const credential = options.credential ?? MCP_BRIDGE_TEST_CREDENTIALS.host; + const serverName = options.serverName ?? "fake"; + const credentialEnvName = options.credentialEnvName ?? "FAKE_MCP_SECRET"; + const addArgs = [ + options.sandboxName, + "mcp", + "add", + serverName, + "--url", + options.mcpUrl, + "--env", + credentialEnvName, + "--deny-tool", + MCP_BRIDGE_DENIED_TOOL_SELECTOR, + ]; + const add = await host.nemoclaw(addArgs, { + artifactName: `${options.artifactPrefix}-mcp-add-fake-server`, + env: { + ...buildAvailabilityProbeEnv(), + [credentialEnvName]: credential, + }, + redactionValues: [credential], + timeoutMs: MCP_MUTATION_TIMEOUT_MS[options.expectedAdapter], + }); + assertExitZero(add, `${options.artifactPrefix} mcp add fake server`); + const status = await host.nemoclaw([options.sandboxName, "mcp", "status", serverName, "--json"], { + artifactName: `${options.artifactPrefix}-mcp-status-json`, + env: { + ...buildAvailabilityProbeEnv(), + [credentialEnvName]: credential, + }, + redactionValues: [credential], + timeoutMs: 60_000, + }); + assertExitZero(status, `${options.artifactPrefix} mcp status --json`); + const statusJson = JSON.parse(status.stdout) as McpBridgeStatus; + expect(statusJson.support).toMatchObject({ + adapter: options.expectedAdapter, + }); + expect(statusJson).toMatchObject({ + server: serverName, + url: options.mcpUrl, + env: { names: [credentialEnvName], ready: true }, + provider: { present: true, state: "configured", attached: true }, + policy: { name: `mcp-bridge-${serverName}`, present: true, state: "configured" }, + adapter: { registered: true }, + }); + expect(statusJson.warnings).toEqual([]); + expect(status.stdout).not.toContain(credential); + expect(statusJson.provider.name).toBe(`${options.sandboxName}-mcp-${serverName}`); + + return statusJson.provider.name!; +} + const ANSI_ESCAPE = /\u001b\[[0-9;]*m/gu; const HERMES_GATEWAY_DRAINING_RETRIES = 3; const HERMES_GATEWAY_DRAINING_RETRY_DELAY_MS = 5_000; @@ -57,11 +146,62 @@ export const DEEPAGENTS_MCP_DENIED_TOOL_PROBE = { toolName: `fake_${MCP_BRIDGE_DENIED_TOOL_NAME}`, }; +/** Capture every mutation surface after an ambiguous OpenClaw credential alias is rejected. */ +export async function captureRejectedOpenClawCredentialAliasState( + host: HostCliClient, + sandbox: SandboxClient, + options: { sandboxName: string; mcpUrl: string }, +) { + const server = "credential-alias"; + const providerName = buildMcpBridgeProviderName(options.sandboxName, server); + const entry: McpSourceEntry = { + server, + agent: "openclaw", + adapter: "openclaw-config", + url: options.mcpUrl, + env: ["ALIAS_MCP_SECRET"], + providerName, + policyName: buildMcpBridgePolicyName(server), + }; + const commandOptions = { env: sandboxAccessEnv(), timeoutMs: 60_000 }; + const source = await host.nemoclaw([options.sandboxName, "mcp", "list", "--json"], { + ...commandOptions, + artifactName: "mcp-negative-ambiguous-credential-alias-list", + }); + const providers = await host.command( + host.openshellCommandPath, + ["sandbox", "provider", "list", options.sandboxName], + { + ...commandOptions, + artifactName: "mcp-negative-ambiguous-credential-alias-provider-list", + }, + ); + const policy = await sandbox.openshell(["policy", "get", "--full", options.sandboxName], { + ...commandOptions, + artifactName: "mcp-negative-ambiguous-credential-alias-policy", + }); + const adapter = await sandbox.execShell( + options.sandboxName, + trustedSandboxShellScript(["set -eu", buildOpenClawMcpInspectCommand(entry, true)].join("\n")), + { ...commandOptions, artifactName: "mcp-negative-ambiguous-credential-alias-adapter" }, + ); + const bridges = JSON.parse(source.stdout) as { bridges: McpBridgeStatus[] }; + return { + adapterAbsent: adapter.exitCode === 0 && /(?:^|\n)absent(?:\n|$)/u.test(resultText(adapter)), + policyAbsent: + policy.exitCode === 0 && !resultText(policy).includes(buildMcpBridgePolicyKey(server)), + providerAbsent: providers.exitCode === 0 && !resultText(providers).includes(providerName), + sourceAbsent: + source.exitCode === 0 && !bridges.bridges.some((candidate) => candidate.server === server), + }; +} + export async function runDeniedMcpToolCall( host: HostCliClient, options: { agent: "openclaw" | "hermes" | "langchain-deepagents-code"; artifactName: string; + credentialEnvName?: string; deniedTool?: string; mcpUrl?: string; sandbox: SandboxClient; @@ -115,10 +255,11 @@ export async function runDeniedMcpToolCall( payload, ]; const deniedToolName = options.deniedTool ?? MCP_BRIDGE_DENIED_TOOL_NAME; + const credentialEnvName = options.credentialEnvName ?? "FAKE_MCP_SECRET"; const command = options.agent === "openclaw" ? [ - `nemoclaw-start node - ${shellQuote(targetUrl)} tools/call deny FAKE_MCP_SECRET ${shellQuote(deniedToolName)} <<'NEMOCLAW_MCP_DENIED_TOOL_PROBE'`, + `nemoclaw-start node - ${shellQuote(targetUrl)} tools/call deny ${shellQuote(credentialEnvName)} ${shellQuote(deniedToolName)} <<'NEMOCLAW_MCP_DENIED_TOOL_PROBE'`, MCP_PROVIDER_REWRITE_PROBE_SOURCE, "NEMOCLAW_MCP_DENIED_TOOL_PROBE", ].join("\n") diff --git a/test/e2e/live/mcp-bridge.test.ts b/test/e2e/live/mcp-bridge.test.ts index a7a96ffe469..9d38d0e477b 100644 --- a/test/e2e/live/mcp-bridge.test.ts +++ b/test/e2e/live/mcp-bridge.test.ts @@ -10,10 +10,7 @@ import { buildOpenClawMcpInspectCommand, } from "../../../src/lib/actions/sandbox/mcp-bridge-adapter-status"; import { shellQuote } from "../../../src/lib/core/shell-quote"; -import type { - McpSourceEntry, - McpBridgeStatus, -} from "../../../src/lib/actions/sandbox/mcp-bridge-contracts"; +import type { McpSourceEntry } from "../../../src/lib/actions/sandbox/mcp-bridge-contracts"; import { execTimeout, testTimeout } from "../../helpers/timeouts.ts"; import type { ArtifactSink } from "../fixtures/artifacts.ts"; import { buildAvailabilityProbeEnv } from "../fixtures/availability-env.ts"; @@ -62,6 +59,8 @@ import { DEEPAGENTS_MCP_DENIED_TOOL_PROBE, HERMES_MCP_ENV_LOAD_COMMANDS, HERMES_MCP_DENIED_TOOL_PROBE, + captureRejectedOpenClawCredentialAliasState, + addBridgeAndReadStatus, readConcurrentMcpStatusAndConfirmHermesRegistration, MCP_BRIDGE_DENIED_TOOL_NAME, MCP_BRIDGE_DENIED_TOOL_SELECTOR, @@ -74,7 +73,6 @@ import { retryHermesGatewayDraining, } from "./mcp-bridge-reliability.ts"; import { - applyMcpHostPolicyEdit, buildMcpDnsRebindingProbeScript, expectExitNonZero, hostAddressForSandbox, @@ -104,7 +102,6 @@ const OPENCLAW_SANDBOX_NAME = process.env.NEMOCLAW_SANDBOX_NAME ?? "e2e-mcp-brid const HERMES_SANDBOX_NAME = process.env.NEMOCLAW_MCP_HERMES_SANDBOX_NAME ?? "e2e-mcp-hermes"; const DEEPAGENTS_SANDBOX_NAME = process.env.NEMOCLAW_MCP_DEEPAGENTS_SANDBOX_NAME ?? "e2e-mcp-dcode"; const SERVER_NAME = "fake"; -const SERVER_POLICY_KEY = "mcp_bridge_fake"; const CONCURRENT_SERVER_NAME = "concurrent"; const HOST_SECRET = MCP_BRIDGE_TEST_CREDENTIALS.host; const ROTATED_HOST_SECRET = MCP_BRIDGE_TEST_CREDENTIALS.rotatedHost; @@ -197,75 +194,6 @@ async function assertSecretAbsentFromSandbox( }); expectExitZero(result, "host MCP secret must not appear in sandbox files"); } -async function addBridgeAndReadStatus( - host: HostCliClient, - sandbox: SandboxClient, - options: { - sandboxName: string; - mcpUrl: string; - expectedAdapter: McpAdapter; - artifactPrefix: string; - credential?: string; - applyHostPolicyEdit?: boolean; - }, -): Promise { - await (options.applyHostPolicyEdit === false - ? Promise.resolve() - : applyMcpHostPolicyEdit(sandbox, options)); - const credential = options.credential ?? HOST_SECRET; - const addArgs = [ - options.sandboxName, - "mcp", - "add", - SERVER_NAME, - "--url", - options.mcpUrl, - "--env", - "FAKE_MCP_SECRET", - "--deny-tool", - MCP_BRIDGE_DENIED_TOOL_SELECTOR, - ]; - const add = await host.nemoclaw(addArgs, { - artifactName: `${options.artifactPrefix}-mcp-add-fake-server`, - env: { - ...buildAvailabilityProbeEnv(), - FAKE_MCP_SECRET: credential, - }, - redactionValues: [credential], - timeoutMs: MCP_MUTATION_TIMEOUT_MS[options.expectedAdapter], - }); - expectExitZero(add, `${options.artifactPrefix} mcp add fake server`); - const status = await host.nemoclaw( - [options.sandboxName, "mcp", "status", SERVER_NAME, "--json"], - { - artifactName: `${options.artifactPrefix}-mcp-status-json`, - env: { - ...buildAvailabilityProbeEnv(), - FAKE_MCP_SECRET: credential, - }, - redactionValues: [credential], - timeoutMs: 60_000, - }, - ); - expectExitZero(status, `${options.artifactPrefix} mcp status --json`); - const statusJson = JSON.parse(status.stdout) as McpBridgeStatus; - expect(statusJson.support).toMatchObject({ - supported: true, - adapter: options.expectedAdapter, - }); - expect(statusJson).toMatchObject({ - url: options.mcpUrl, - env: { names: ["FAKE_MCP_SECRET"], ready: true }, - provider: { present: true, state: "configured", attached: true }, - policy: { present: true, state: "configured" }, - adapter: { registered: true }, - }); - expect(statusJson.warnings).toEqual([]); - expect(status.stdout).not.toContain(credential); - expect(statusJson.provider.name).toBe(`${options.sandboxName}-mcp-${SERVER_NAME}`); - - return statusJson.provider.name!; -} async function assertConcurrentAddSerialized( host: HostCliClient, @@ -410,20 +338,24 @@ async function assertBridgeInfrastructure( const policyText = resultText(policy); expect(policyText).toMatch( new RegExp( - `${SERVER_POLICY_KEY}[\\s\\S]*protocol: mcp[\\s\\S]*method: tools/list[\\s\\S]*method: tools/call`, + "mcp_bridge_fake[\\s\\S]*protocol: mcp[\\s\\S]*method: tools/list[\\s\\S]*method: tools/call", ), ); expect(policyText).not.toContain("FAKE_MCP_SECRET"); expect(policyText).toContain(`tool: ${MCP_BRIDGE_DENIED_TOOL_SELECTOR}`); expect(policyText).toContain(new URL(options.mcpUrl).hostname); - const provider = await host.command("openshell", ["provider", "get", options.providerName], { - artifactName: `${options.artifactPrefix}-openshell-provider-get-mcp`, - env: buildAvailabilityProbeEnv(), - timeoutMs: 60_000, - }); + const provider = await host.command( + host.openshellCommandPath, + ["provider", "get", options.providerName], + { + artifactName: `${options.artifactPrefix}-openshell-provider-get-mcp`, + env: buildAvailabilityProbeEnv(), + timeoutMs: 60_000, + }, + ); expectExitZero(provider, `${options.artifactPrefix} openshell provider get mcp provider`); expect(resultText(provider)).toContain("FAKE_MCP_SECRET"); - expect(resultText(provider)).not.toContain(HOST_SECRET); + expect(resultText(provider)).not.toContain(MCP_BRIDGE_TEST_CREDENTIALS.host); } async function removeBridgeAndAssertEmpty( @@ -575,11 +507,17 @@ async function assertRealAdapterToolCall( resultToken: string; artifactName: string; expectedSecret?: string; + credentialEnvName?: string; + serverName?: string; + otherEndpoint?: Awaited>; deniedTool?: string; mcpUrl?: string; }, ): Promise { const before = fakeMcp.requests.filter((request) => request.rpcMethod === "tools/call").length; + const otherCallsBefore = options.otherEndpoint?.requests.filter( + (request) => request.rpcMethod === "tools/call", + ).length; const prompt = `Call the fake MCP tool exactly once with challenge ${TOOL_CHALLENGE} and return its result verbatim.`; const hermesPayload = JSON.stringify({ model: COMPATIBLE_MODEL, @@ -639,18 +577,25 @@ async function assertRealAdapterToolCall( const calls = fakeMcp.requests.filter((request) => request.rpcMethod === "tools/call"); const details = ["rpcId", "rpcToolName", "responseStatus", "responseHasResult"]; expect(calls, JSON.stringify(calls.slice(before), details)).toHaveLength(before + 1); - expect(calls.at(-1)).toMatchObject({ - auth: `Bearer ${options.expectedSecret ?? HOST_SECRET}`, - path: "/mcp", - }); - expect(calls.at(-1)?.auth).not.toContain("openshell:resolve:env"); + // Check this invocation so later rotation remains valid, without printing secrets. + expect( + calls + .slice(before) + .every( + (request) => + request.auth === `Bearer ${options.expectedSecret ?? HOST_SECRET}` && + request.path === "/mcp", + ) && + options.otherEndpoint?.requests.filter((request) => request.rpcMethod === "tools/call") + .length === otherCallsBefore, + ).toBe(true); const denied = options.deniedTool ? await runDeniedMcpToolCall(host, { ...options, artifactName: `${options.artifactName}-denied-${options.deniedTool}`, requests: fakeMcp.requests, sandbox, - serverName: SERVER_NAME, + serverName: options.serverName ?? SERVER_NAME, }) : null; expect(denied ? denied.result.timedOut || denied.result.exitCode === null : false).toBe(false); @@ -764,15 +709,13 @@ test( scope: mcpBridgeE2eScope, server: SERVER_NAME, }); + const openClawToolSearch = { query: "fake echo", toolNames: ["fake__fake_echo"] }; const compatibleMock = await startCompatibleMock({ apiKey: COMPATIBLE_KEY, model: COMPATIBLE_MODEL, toolChallenge: TOOL_CHALLENGE, toolResultToken: MCP_RESULT, - openClawToolSearch: { - query: "fake echo", - toolNames: ["fake__fake_echo"], - }, + openClawToolSearch, }); cleanup.add("stop MCP bridge compatible endpoint mock", () => compatibleMock.close()); const fakeMcp = await startFakeMcpHttpsServer(MCP_SERVER_OPTIONS); @@ -875,6 +818,79 @@ test( ...bridge, expectedAdapter: "openclaw-config", }); + await expectMcpCliFailure( + host, + OPENCLAW_SANDBOX_NAME, + ["add", "credential-alias", "--url", mcpUrl, "--env", "ALIAS_MCP_SECRET"], + /cannot safely choose between credentials for an indistinguishable endpoint/, + "mcp-negative-ambiguous-credential-alias", + { + ...buildAvailabilityProbeEnv(), + ALIAS_MCP_SECRET: HOST_SECRET, + }, + ); + const aliasState = await captureRejectedOpenClawCredentialAliasState(host, sandbox, bridge); + expect(aliasState.sourceAbsent).toBe(true); + expect(aliasState.providerAbsent).toBe(true); + expect(aliasState.policyAbsent).toBe(true); + expect(aliasState.adapterAbsent).toBe(true); + // Distinguishable endpoints must coexist, not merely pass preflight. Exercise + // both native adapters while both credentials and policy entries are present. + const distinctMcp = await startFakeMcpHttpsServer({ + ...MCP_SERVER_OPTIONS, + secret: ROTATED_HOST_SECRET, + }); + cleanup.add("stop distinct MCP HTTPS server", () => distinctMcp.close()); + const distinctTunnel = await startPublicMcpHttpsTunnel({ + cleanup, + label: "distinct credential MCP HTTPS server", + progress, + server: distinctMcp, + }); + cleanup.add("remove distinct credential bridge", () => + cleanupMcpBridge(host, OPENCLAW_SANDBOX_NAME, "distinct", "openclaw-config"), + ); + await addBridgeAndReadStatus(host, sandbox, { + ...bridge, + mcpUrl: distinctTunnel.url, + expectedAdapter: "openclaw-config", + serverName: "distinct", + credentialEnvName: "DISTINCT_MCP_SECRET", + credential: ROTATED_HOST_SECRET, + artifactPrefix: "openclaw-distinct", + applyHostPolicyEdit: false, + }); + await assertRealAdapterToolCall(host, sandbox, fakeMcp, { + ...bridge, + resultToken: MCP_RESULT, + artifactName: "openclaw-dual-original-tool-call", + otherEndpoint: distinctMcp, + deniedTool: MCP_BRIDGE_DENIED_TOOL_NAME, + }); + openClawToolSearch.query = "distinct fake echo"; + openClawToolSearch.toolNames = ["distinct__fake_echo"]; + try { + await restartBridgeWithoutHostSecret(host, OPENCLAW_SANDBOX_NAME, "openclaw"); + await assertRealAdapterToolCall(host, sandbox, distinctMcp, { + ...bridge, + resultToken: MCP_RESULT, + expectedSecret: ROTATED_HOST_SECRET, + otherEndpoint: fakeMcp, + serverName: "distinct", + mcpUrl: distinctTunnel.url, + credentialEnvName: "DISTINCT_MCP_SECRET", + deniedTool: MCP_BRIDGE_DENIED_TOOL_NAME, + artifactName: "openclaw-dual-distinct-tool-call", + }); + } finally { + openClawToolSearch.query = "fake echo"; + openClawToolSearch.toolNames = ["fake__fake_echo"]; + } + await cleanupMcpBridge(host, OPENCLAW_SANDBOX_NAME, "distinct", "openclaw-config"); + await assertBridgeInfrastructure(host, sandbox, { + ...bridge, + providerName, + }); await assertAuthenticatedMcpToolDiscovery(host, fakeMcp, { artifacts, sandboxName: OPENCLAW_SANDBOX_NAME, @@ -883,27 +899,6 @@ test( hostSecret: HOST_SECRET, progress, }); - await assertBridgeInfrastructure(host, sandbox, { - ...bridge, - providerName, - }); - await expectMcpCliFailure( - host, - OPENCLAW_SANDBOX_NAME, - ["add", SERVER_NAME, "--url", mcpUrl, "--env", "FAKE_MCP_SECRET"], - /already exists/, - "mcp-negative-duplicate-server", - { - ...buildAvailabilityProbeEnv(), - FAKE_MCP_SECRET: HOST_SECRET, - }, - ); - - expect(fakeMcp.requests.some((request) => request.auth === `Bearer ${HOST_SECRET}`)).toBe(true); - expect( - fakeMcp.requests.every((request) => !request.auth.includes("openshell:resolve:env")), - ).toBe(true); - await artifacts.writeText("mcp-provider-rewrite-proof.cjs", MCP_PROVIDER_REWRITE_PROBE_SOURCE); const runNodeMcpProbe = runMcpProviderRewriteProbe.bind(null, sandbox, OPENCLAW_SANDBOX_NAME); @@ -944,10 +939,6 @@ test( jsonrpc: "2.0", method: "tools/list", }); - expect( - fakeMcp.requests.every((request) => !request.auth.includes("openshell:resolve:env")), - ).toBe(true); - const deniedMethodRequestCount = fakeMcp.requests.filter( (request) => request.rpcMethod === "admin/delete", ).length; @@ -1031,12 +1022,6 @@ test( ]); progress.phase("exercise lifecycle and confirm OpenClaw bridge removal"); - await assertRealAdapterToolCall(host, sandbox, fakeMcp, { - ...bridge, - resultToken: MCP_RESULT, - artifactName: "openclaw-real-mcp-tool-call-initial", - deniedTool: MCP_BRIDGE_DENIED_TOOL_NAME, - }); const updateProof = await runOpenClawDeniedToolUpdateProof( host, sandbox, @@ -1047,13 +1032,6 @@ test( expect(updateProof.commandsSucceeded).toBe(true); expect(updateProof.after).toBe(updateProof.before + 1); expect(updateProof.lastCall).toMatchObject({ auth: `Bearer ${HOST_SECRET}` }); - await restartBridgeWithoutHostSecret(host, OPENCLAW_SANDBOX_NAME, "openclaw"); - await assertRealAdapterToolCall(host, sandbox, fakeMcp, { - ...bridge, - resultToken: MCP_RESULT, - artifactName: "openclaw-real-mcp-tool-call-after-restart", - deniedTool: MCP_BRIDGE_DENIED_TOOL_NAME, - }); await replaceBridgeCredentialConservatively( host, sandbox, diff --git a/test/e2e/mock-parity.json b/test/e2e/mock-parity.json index eeb0ea90646..0aecb3f7a85 100644 --- a/test/e2e/mock-parity.json +++ b/test/e2e/mock-parity.json @@ -610,6 +610,7 @@ "test/e2e/live/openshell-exact-main-runtime-contracts.ts" ], "fast": [ + "src/lib/actions/sandbox/mcp-bridge-state.test.ts", "src/lib/actions/sandbox/mcp-bridge-policy.test.ts", "src/lib/actions/sandbox/mcp-bridge-adapter-registration.test.ts", "src/lib/actions/sandbox/mcp-bridge-adapter-teardown.test.ts", diff --git a/test/e2e/support/mcp-bridge-reliability.test.ts b/test/e2e/support/mcp-bridge-reliability.test.ts index 317658455bf..2df70018663 100644 --- a/test/e2e/support/mcp-bridge-reliability.test.ts +++ b/test/e2e/support/mcp-bridge-reliability.test.ts @@ -7,6 +7,7 @@ import path from "node:path"; import { describe, expect, it, vi } from "vitest"; +import { buildMcpBridgePolicyKey } from "../../../src/lib/actions/sandbox/mcp-bridge-policy-render"; import { ArtifactSink } from "../fixtures/artifacts.ts"; import { HostCliClient } from "../fixtures/clients/host.ts"; import type { SandboxClient } from "../fixtures/clients/sandbox.ts"; @@ -14,6 +15,8 @@ import { MCP_BRIDGE_TEST_CREDENTIALS } from "../fixtures/mcp-bridge-credentials. import { startTestProgress } from "../fixtures/progress.ts"; import { ShellProbe } from "../fixtures/shell-probe.ts"; import { + captureRejectedOpenClawCredentialAliasState, + addBridgeAndReadStatus, confirmHermesMcpRegistrationAfterRestartSettlement, isHermesMcpAddPostProbeNotReady, isHermesMcpStatusAwaitingRestartSettlement, @@ -30,6 +33,116 @@ import { const HTTP_STATUS_MARKER = "NEMOCLAW_HERMES_MCP_HTTP_STATUS="; +describe("real MCP add/status helper", () => { + it.each([ + ["fake", "FAKE_MCP_SECRET", MCP_BRIDGE_TEST_CREDENTIALS.host], + ["distinct", "DISTINCT_MCP_SECRET", MCP_BRIDGE_TEST_CREDENTIALS.rotatedHost], + ])( + "keeps %s endpoint and credential identity in both real CLI calls", + async (server, envName, secret) => { + const url = `https://${server}.example.test/mcp`; + const providerName = `alpha-mcp-${server}`; + const nemoclaw = vi.fn().mockResolvedValue({ + exitCode: 0, + timedOut: false, + stderr: "", + stdout: JSON.stringify({ + support: { supported: true, adapter: "openclaw-config" }, + server, + url, + env: { names: [envName], ready: true }, + provider: { name: providerName, present: true, state: "configured", attached: true }, + policy: { name: `mcp-bridge-${server}`, present: true, state: "configured" }, + adapter: { registered: true }, + warnings: [], + }), + }); + expect( + await addBridgeAndReadStatus( + { nemoclaw } as unknown as HostCliClient, + {} as SandboxClient, + { + sandboxName: "alpha", + mcpUrl: url, + expectedAdapter: "openclaw-config", + artifactPrefix: server, + serverName: server, + credentialEnvName: envName, + credential: secret, + applyHostPolicyEdit: false, + }, + ), + ).toBe(providerName); + expect(nemoclaw.mock.calls[0]?.[0]).toEqual([ + "alpha", + "mcp", + "add", + server, + "--url", + url, + "--env", + envName, + "--deny-tool", + "fake_s*", + ]); + expect(nemoclaw.mock.calls[1]?.[0]).toEqual(["alpha", "mcp", "status", server, "--json"]); + expect(nemoclaw.mock.calls[0]?.[1].env[envName] === secret).toBe(true); + expect(nemoclaw.mock.calls[1]?.[1].env[envName] === secret).toBe(true); + expect(nemoclaw.mock.calls[0]?.[1].redactionValues.includes(secret)).toBe(true); + expect(nemoclaw.mock.calls[1]?.[1].redactionValues.includes(secret)).toBe(true); + }, + ); +}); + +describe("rejected OpenClaw credential alias state", () => { + it.each([false, true])( + "observes all mutation surfaces on the selected gateway (residual policy: %s)", + async (residualPolicy) => { + vi.stubEnv("OPENSHELL_GATEWAY", "non-default-mcp-gateway"); + try { + const ok = { exitCode: 0, timedOut: false, stdout: "", stderr: "" }; + const nemoclaw = vi.fn().mockResolvedValue({ ...ok, stdout: '{"bridges":[]}' }); + const command = vi.fn().mockResolvedValue(ok); + const openshell = vi.fn().mockImplementation(async (_args, options) => { + expect(options.env.OPENSHELL_GATEWAY).toBe("non-default-mcp-gateway"); + return { + ...ok, + stdout: residualPolicy ? buildMcpBridgePolicyKey("credential-alias") : "", + }; + }); + const execShell = vi.fn().mockResolvedValue({ ...ok, stdout: "absent\n" }); + const result = await captureRejectedOpenClawCredentialAliasState( + { + nemoclaw, + command, + openshellCommandPath: "/fixture/openshell", + } as unknown as HostCliClient, + { openshell, execShell } as unknown as SandboxClient, + { sandboxName: "alpha", mcpUrl: "https://example.test/mcp" }, + ); + + expect(result).toEqual({ + adapterAbsent: true, + policyAbsent: !residualPolicy, + providerAbsent: true, + sourceAbsent: true, + }); + expect(openshell).toHaveBeenCalledWith( + ["policy", "get", "--full", "alpha"], + expect.objectContaining({ + env: expect.objectContaining({ OPENSHELL_GATEWAY: "non-default-mcp-gateway" }), + }), + ); + expect(nemoclaw.mock.calls[0]?.[1].env.OPENSHELL_GATEWAY).toBe("non-default-mcp-gateway"); + expect(command.mock.calls[0]?.[2].env.OPENSHELL_GATEWAY).toBe("non-default-mcp-gateway"); + expect(execShell.mock.calls[0]?.[2].env.OPENSHELL_GATEWAY).toBe("non-default-mcp-gateway"); + } finally { + vi.unstubAllEnvs(); + } + }, + ); +}); + describe("OpenClaw denied-tool target", () => { it.each([undefined, "", "relative/path"])( "rejects an invalid URL %j before host or sandbox work", diff --git a/test/helpers/rebuild-flow-generic-harness.ts b/test/helpers/rebuild-flow-generic-harness.ts index 41d4fe3b722..0e0ce11eef9 100644 --- a/test/helpers/rebuild-flow-generic-harness.ts +++ b/test/helpers/rebuild-flow-generic-harness.ts @@ -1048,9 +1048,15 @@ export function createRebuildFlowHarness(overrides: RebuildFlowOverrides = {}): const nativeMcpSources = Object.fromEntries( mcpSourceEntries.map((entry) => [String(entry.server), structuredClone(entry)]), ); + const legacyMcpSources = Object.fromEntries( + (overrides.mcpLegacySources ?? []).map((entry) => [ + String(entry.server), + structuredClone(entry), + ]), + ); vi.spyOn(mcpBridgeSource, "inspectAgentMcpSources").mockReturnValue({ native: nativeMcpSources, - legacy: {}, + legacy: legacyMcpSources, }); vi.spyOn(mcpBridgeSource, "joinMcpEntriesToOpenShell").mockReturnValue(nativeMcpSources); const defaultMcpPreparation = ( diff --git a/test/helpers/rebuild-flow-test-support.ts b/test/helpers/rebuild-flow-test-support.ts index 3ea9cdde7a4..b64fc966d85 100644 --- a/test/helpers/rebuild-flow-test-support.ts +++ b/test/helpers/rebuild-flow-test-support.ts @@ -129,6 +129,7 @@ export type RebuildFlowOverrides = { revalidateBeforeDelete?: () => Promise; assertDeleteEdgeUnchanged?: () => void; }; + mcpLegacySources?: Array>; runOpenshell?: (args: string[]) => | { status: number;