Merge pull request #2119 from modelcontextprotocol/v2/fix/2096-stdio-env-cwd-revert

fix(web): carry a landed stdio env/cwd onto the config it connects with

clients/web/src/App.test.tsx

@@ -3283,6 +3283,94 @@ describe("App paginated list pagination toggle (#1721)", () => {
     expect(fetchLogOptions?.maxFetchRequests).toBe(42);
   });
 
+  it("carries a landed stdio env/cwd onto the config it connects with (#2096)", async () => {
+    // The sibling case above covers the client *options*, which are derived
+    // from the settings. `env` and `cwd` are the two fields the modal edits as
+    // settings while the transport reads them off the **config** — and the
+    // config came straight off the same frozen `servers` entry. So a save that
+    // landed while list reads were failing kept spawning the child process with
+    // the pre-save environment, while the modal (re-seeded from the tracker
+    // since #2089) showed the new one. Credentials live here, so the symptom is
+    // the inspected server failing to authorize rather than a lost edit.
+    const user = userEvent.setup();
+    renderWithMantine(<App />);
+
+    const draftOptions = vi.mocked(useSettingsDraft).mock.calls.at(-1)?.[0];
+    if (!draftOptions) throw new Error("useSettingsDraft was never called");
+    await act(async () => {
+      await draftOptions.onPersist("A", {
+        ...settingsWithRoots([]),
+        env: [{ key: "FOO", value: "after" }],
+        cwd: "/tmp/after",
+      });
+    });
+
+    // `servers` still reports `{ type: "stdio", command: "node" }` with no
+    // settings node at all — what a failed reload leaves behind.
+    await user.click(screen.getByText("connect"));
+    await waitFor(() => expect(clientInstances).toHaveLength(1));
+    expect(vi.mocked(McpIndex.InspectorClient).mock.calls.at(-1)?.[0]).toEqual({
+      type: "stdio",
+      command: "node",
+      env: { FOO: "after" },
+      cwd: "/tmp/after",
+    });
+  });
+
+  it("clears a stdio env/cwd the last write removed (#2096)", async () => {
+    // Clearing has to travel too, and it is the direction a naive merge gets
+    // wrong: an empty env list and a blank cwd mean "remove the field", which
+    // is how the modal deletes a value. Spreading the settings over the config
+    // would leave the stale one in place instead.
+    const user = userEvent.setup();
+    const previousUseServers = vi.mocked(useServers).getMockImplementation();
+    try {
+      vi.mocked(useServers).mockReturnValue({
+        servers: [
+          {
+            ...SERVER_A,
+            config: {
+              type: "stdio",
+              command: "node",
+              env: { FOO: "before" },
+              cwd: "/tmp/before",
+            },
+          } as ServerEntry,
+        ],
+        loading: false,
+        error: undefined,
+        refresh: vi.fn().mockResolvedValue(undefined),
+        addServer: vi.fn(),
+        updateServer: vi.fn(),
+        updateServerSettings: updateServerSettingsSpy,
+        removeServer: vi.fn(),
+        reorderServers: vi.fn(),
+        importSource: vi.fn().mockResolvedValue({ servers: {} }),
+      });
+      renderWithMantine(<App />);
+
+      const draftOptions = vi.mocked(useSettingsDraft).mock.calls.at(-1)?.[0];
+      if (!draftOptions) throw new Error("useSettingsDraft was never called");
+      await act(async () => {
+        await draftOptions.onPersist("A", {
+          ...settingsWithRoots([]),
+          env: [],
+          cwd: "",
+        });
+      });
+
+      await user.click(screen.getByText("connect"));
+      await waitFor(() => expect(clientInstances).toHaveLength(1));
+      expect(
+        vi.mocked(McpIndex.InspectorClient).mock.calls.at(-1)?.[0],
+      ).toEqual({ type: "stdio", command: "node" });
+    } finally {
+      if (previousUseServers) {
+        vi.mocked(useServers).mockImplementation(previousUseServers);
+      }
+    }
+  });
+
   it("lets a successful list read supersede a concrete rollback override (#2089)", async () => {
     // A rollback sets the override to a value rather than clearing it, so the
     // effect that drops it cannot key on the persisted boolean alone: if a

clients/web/src/App.tsx

@@ -70,6 +70,7 @@ import { ManagedResourceTemplatesState } from "@inspector/core/mcp/state/managed
 import { ManagedRequestorTasksState } from "@inspector/core/mcp/state/managedRequestorTasksState.js";
 import { ResourceSubscriptionsState } from "@inspector/core/mcp/state/resourceSubscriptionsState.js";
 import {
+  applyStdioSettingsToConfig,
   cleanRoots,
   oauthAuthorizationParamsFromSettings,
   oauthEndpointOverridesFromSettings,
@@ -2656,7 +2657,19 @@ function App() {
               ...(activeCimdUrl && { clientMetadataUrl: activeCimdUrl }),
             }
           : undefined;
-      const client = new InspectorClient(server.config, {
+      // The stdio `env` / `cwd` are edited as *settings* but stored on — and
+      // read by the transport from — *config*, so the tracker's account of the
+      // former has to be carried onto the latter. `server.config` comes off the
+      // same frozen `servers` entry `server.settings` does, so without this a
+      // save that landed while list reads were failing spawns the child process
+      // with the pre-save environment while the modal, re-seeded from the
+      // tracker, shows the new one (#2096). Same mapping the PUT route applies
+      // when persisting it, from the same helper.
+      const effectiveConfig = applyStdioSettingsToConfig(
+        server.config,
+        savedSettings,
+      );
+      const client = new InspectorClient(effectiveConfig, {
         environment,
         // The Tasks tab needs the receiver-task pipeline; the
         // requestor-task list comes from the client's task store.

clients/web/src/test/core/mcp/serverList.test.ts

@@ -12,12 +12,18 @@ import {
   mergeSecretsIntoStored,
   normalizeServerType,
   normalizeStoredMetadata,
+  applyStdioSettingsToConfig,
+  stdioConfigFieldsFromSettings,
   oauthAuthorizationParamsFromSettings,
   oauthEndpointOverridesFromSettings,
   serverEntriesToMcpConfig,
   serializeMcpConfig,
   storedFieldsToInspectorSettings,
 } from "@inspector/core/mcp/serverList.js";
+import type {
+  InspectorServerSettings,
+  MCPServerConfig,
+} from "@inspector/core/mcp/types.js";
 import {
   SECRET_FIELD_OAUTH_CLIENT_SECRET,
   envSecretField,
@@ -1833,3 +1839,122 @@ describe("cleanAuthorizationParams", () => {
     ]);
   });
 });
+
+describe("stdio env/cwd settings → config mapping (#2096)", () => {
+  // `env` and `cwd` are edited as settings but read by the transport off the
+  // config, so both sides of the wire have to derive one from the other. This
+  // is the single mapping they share — the PUT write-through persists it, the
+  // web client applies it when constructing an InspectorClient.
+  const settings = (
+    over: Partial<InspectorServerSettings> = {},
+  ): InspectorServerSettings => ({
+    headers: [],
+    env: [],
+    metadata: {},
+    connectionTimeout: 0,
+    requestTimeout: 0,
+    taskTtl: 60000,
+    maxFetchRequests: 1000,
+    roots: [],
+    ...over,
+  });
+
+  describe("stdioConfigFieldsFromSettings", () => {
+    it("collapses the pair rows into a record and trims the cwd", () => {
+      expect(
+        stdioConfigFieldsFromSettings(
+          settings({
+            env: [
+              { key: "FOO", value: "after" },
+              { key: "BAR", value: "" },
+            ],
+            cwd: "  /tmp/after  ",
+          }),
+        ),
+      ).toEqual({ env: { FOO: "after", BAR: "" }, cwd: "/tmp/after" });
+    });
+
+    it("reports an empty list and a blank cwd as unset, i.e. cleared", () => {
+      // The modal deletes a value by emptying it, so "no rows" cannot mean
+      // "leave it alone" — it has to remove the field.
+      expect(
+        stdioConfigFieldsFromSettings(settings({ env: [], cwd: "   " })),
+      ).toEqual({ env: undefined, cwd: undefined });
+      // A row the user started and left blank is dropped, so it does not
+      // materialize an env consisting of one nameless variable.
+      expect(
+        stdioConfigFieldsFromSettings(
+          settings({ env: [{ key: "  ", value: "x" }] }),
+        ).env,
+      ).toBeUndefined();
+    });
+  });
+
+  describe("applyStdioSettingsToConfig", () => {
+    const stdio: MCPServerConfig = {
+      type: "stdio",
+      command: "node",
+      args: ["server.js"],
+      env: { FOO: "before" },
+      cwd: "/tmp/before",
+    };
+
+    it("overwrites env/cwd and leaves the rest of the config alone", () => {
+      expect(
+        applyStdioSettingsToConfig(
+          stdio,
+          settings({
+            env: [{ key: "FOO", value: "after" }],
+            cwd: "/tmp/after",
+          }),
+        ),
+      ).toEqual({
+        type: "stdio",
+        command: "node",
+        args: ["server.js"],
+        env: { FOO: "after" },
+        cwd: "/tmp/after",
+      });
+    });
+
+    it("removes the fields the settings cleared", () => {
+      expect(applyStdioSettingsToConfig(stdio, settings())).toEqual({
+        type: "stdio",
+        command: "node",
+        args: ["server.js"],
+      });
+    });
+
+    it("does not mutate the config it was given", () => {
+      const before = { ...stdio };
+      applyStdioSettingsToConfig(stdio, settings());
+      expect(stdio).toEqual(before);
+    });
+
+    it("treats a typeless entry as stdio, matching the on-disk default", () => {
+      // An `mcp.json` entry written without a `type` key is stdio (the Claude
+      // Desktop convention), and the modal edits its env like any other.
+      const applied = applyStdioSettingsToConfig(
+        { command: "node" } as MCPServerConfig,
+        settings({ env: [{ key: "FOO", value: "after" }] }),
+      );
+      expect(applied).toEqual({ command: "node", env: { FOO: "after" } });
+    });
+
+    it("returns a non-stdio config untouched", () => {
+      // An HTTP server carries neither field, matching the modal's stdio-only
+      // UI — applying an empty mirror to it must not invent one.
+      const http: MCPServerConfig = {
+        type: "streamable-http",
+        url: "https://mcp.example.com/mcp",
+      };
+      expect(applyStdioSettingsToConfig(http, settings())).toBe(http);
+    });
+
+    it("returns the config untouched when there are no settings", () => {
+      // A server with no settings node has nothing to apply; treating that as
+      // an empty mirror would clear an env the file does hold.
+      expect(applyStdioSettingsToConfig(stdio, undefined)).toBe(stdio);
+    });
+  });
+});

core/mcp/remote/node/server.ts

@@ -51,7 +51,6 @@ import type {
 } from "../../types.js";
 import {
   DEFAULT_SEED_CONFIG,
-  envPairsToRecord,
   expectedSecretFields,
   extractSecretsFromStored,
   INSPECTOR_FIELD_KEYS,
@@ -59,6 +58,7 @@ import {
   isProtocolEra,
   mergeSecretsIntoStored,
   normalizeServerType,
+  stdioConfigFieldsFromSettings,
   storedFieldsToInspectorSettings,
   stripInspectorFields,
 } from "../../serverList.js";
@@ -1662,16 +1662,18 @@ export function createRemoteApp(
   ): void => {
     if (!(entry.type === "stdio" || entry.type === undefined)) return;
     const stdio = entry as StdioServerConfig & StoredMCPServer;
+    // Shared with the web client's connect-time application of the same
+    // mapping (#2096), so the environment a save persists and the one the
+    // child process is spawned with cannot be derived differently.
+    const { env, cwd } = stdioConfigFieldsFromSettings(settings);
     if (provided.env) {
-      const env = envPairsToRecord(settings.env);
-      if (Object.keys(env).length > 0) {
+      if (env) {
         stdio.env = env;
       } else {
         delete stdio.env;
       }
     }
     if (provided.cwd) {
-      const cwd = settings.cwd?.trim();
       if (cwd) {
         stdio.cwd = cwd;
       } else {

core/mcp/serverList.ts

@@ -197,6 +197,62 @@ export function envPairsToRecord(
   return out;
 }
 
+/**
+ * The stdio `env` / `cwd` **config** fields implied by a settings mirror.
+ * `undefined` means "the field is not set" — i.e. remove it — which is how a
+ * user clears a value through the Server Settings modal (empty env list, blank
+ * cwd).
+ *
+ * `env` and `cwd` are the two fields the modal edits as *settings* while the
+ * transport reads them off *config* (see `storedFieldsToInspectorSettings`), so
+ * every consumer that has settings and needs a config has to perform this same
+ * mapping. It lives here, once, because the two consumers are on opposite sides
+ * of the wire — the `/api/servers` PUT write-through persists it, and the web
+ * client applies it when constructing an `InspectorClient` (#2096) — and a
+ * disagreement between them is invisible until a spawned child process gets an
+ * environment the file does not describe.
+ */
+export function stdioConfigFieldsFromSettings(
+  settings: InspectorServerSettings,
+): { env: Record<string, string> | undefined; cwd: string | undefined } {
+  const env = envPairsToRecord(settings.env);
+  const cwd = settings.cwd?.trim();
+  return {
+    env: Object.keys(env).length > 0 ? env : undefined,
+    cwd: cwd ? cwd : undefined,
+  };
+}
+
+/**
+ * A copy of `config` with the stdio `env` / `cwd` taken from `settings`.
+ *
+ * Used where a caller holds a settings value it trusts more than the config it
+ * was read alongside — the web client's connect path, which resolves settings
+ * through the last-persisted-write tracker while `config` still comes off a
+ * `servers` entry that stops advancing once a list read fails (#2089). Without
+ * this the two disagree and the child process is spawned with the *pre-save*
+ * environment even though the save reached disk (#2096).
+ *
+ * Non-stdio configs are returned untouched — they carry neither field, matching
+ * the modal's stdio-only UI. Absent settings likewise: a server with no settings
+ * node has nothing to apply, and treating that as an empty mirror would clear a
+ * config `env` the file does hold.
+ */
+export function applyStdioSettingsToConfig(
+  config: MCPServerConfig,
+  settings: InspectorServerSettings | undefined,
+): MCPServerConfig {
+  if (!settings) return config;
+  if (!(config.type === "stdio" || config.type === undefined)) return config;
+  const { env, cwd } = stdioConfigFieldsFromSettings(settings);
+  const next: StdioServerConfig = { ...(config as StdioServerConfig) };
+  if (env) next.env = env;
+  else delete next.env;
+  if (cwd) next.cwd = cwd;
+  else delete next.cwd;
+  return next;
+}
+
 /**
  * Read a server's `metadata` off disk into the in-memory `RequestMetadata`
  * object, accepting both the current object shape and the pre-#1910