fix(servers): pass-3 review — patch-style PUT, toast on close-flush fail

Pass-3 review at #1353 (comment).

(b) `PUT /api/servers/:id` now treats both `config` and `settings` as
    optional patches:
      - field omitted → preserve the on-disk value
      - explicit body value → apply
      - settings: null → clear
    `updateServerSettings` stops reading `existing.config` from the
    in-memory `servers` snapshot, which closes the closure-capture
    hazard where a refresh between debounce-schedule and flush could
    silently revert a separate edit. The route is the single source of
    truth for transport config now. New tests cover settings-only PUT,
    rejected non-object config, and the empty-body no-op patch case.
    The previous "rejects a missing config" test is rewritten to
    document the new no-op-preserve semantics.

(c) Inline comment near the PUT rebuild loop documents that writing
    the full mcpServers map back is what gives the route its desirable
    self-healing property for malformed settings on *other* entries —
    deliberate side-effect, not an accident of normalization.

(d) `flushPendingSettings` surfaces failures via @mantine/notifications
    instead of swallowing them. The modal already closed by the time
    the flush PUT resolves, so a silent fail would leave the user
    thinking their last edits saved (especially painful for the OAuth
    client secret). The notifications wiring was already mounted in
    main.tsx.

Pass-3 (a) — settings form being effectively uncontrolled mid-edit — is
acknowledged in the reply: working as intended today; the proper fix is
internal modal state when file-watching (#1345) forces the issue.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Author: cliffhall

clients/web/src/App.tsx

@@ -1,5 +1,6 @@
 import { useCallback, useEffect, useMemo, useRef, useState } from "react";
 import { useComputedColorScheme, useMantineColorScheme } from "@mantine/core";
+import { notifications } from "@mantine/notifications";
 import type {
   InitializeResult,
   LoggingLevel,
@@ -801,7 +802,17 @@ function App() {
     const pending = pendingSettingsRef.current;
     if (!pending) return;
     pendingSettingsRef.current = null;
-    void updateServerSettings(pending.id, pending.settings);
+    // Fire-and-forget — but surface failures via toast. The modal closes
+    // immediately on user dismiss, so a silent fail-on-flush would leave
+    // the user thinking their last edits saved when they didn't (especially
+    // painful for the OAuth client secret).
+    updateServerSettings(pending.id, pending.settings).catch((err) => {
+      notifications.show({
+        title: `Failed to save settings for "${pending.id}"`,
+        message: err instanceof Error ? err.message : String(err),
+        color: "red",
+      });
+    });
   }, [updateServerSettings]);
 
   const onSettingsChange = useCallback(

clients/web/src/test/core/react/useServers.test.tsx

@@ -323,7 +323,7 @@ describe("useServers", () => {
     expect(stored.settings).toBeDefined();
   });
 
-  it("updateServerSettings throws when the target id does not exist", async () => {
+  it("updateServerSettings throws when the target id does not exist (server-side 404)", async () => {
     const { result } = renderHook(() =>
       useServers({ baseUrl: "http://test.local", fetchFn: h.fetchFn }),
     );

clients/web/src/test/integration/mcp/remote/servers-route.test.ts

@@ -344,13 +344,19 @@ describe("/api/servers routes", () => {
       expect(res.status).toBe(400);
     });
 
-    it("rejects a missing config", async () => {
+    it("accepts a body with neither config nor settings (no-op patch)", async () => {
+      // Both fields are now optional patches. An empty body is a degenerate
+      // but valid request — it preserves both config and settings on disk.
       const res = await fetch(`${h.baseUrl}/api/servers/alpha`, {
         method: "PUT",
         headers: { "Content-Type": "application/json" },
         body: JSON.stringify({ id: "alpha" }),
       });
-      expect(res.status).toBe(400);
+      expect(res.status).toBe(200);
+      expect(readConfig(h.configPath).mcpServers.alpha).toEqual({
+        type: "stdio",
+        command: "old",
+      });
     });
 
     it("rejects malformed JSON body", async () => {
@@ -541,6 +547,64 @@ describe("/api/servers routes", () => {
       expect(body.error).toMatch(/settings\.headers/);
     });
 
+    it("accepts a settings-only PUT and preserves config from disk", async () => {
+      writeFileSync(
+        h.configPath,
+        JSON.stringify({
+          mcpServers: {
+            theta: {
+              type: "streamable-http",
+              url: "https://x.test/mcp",
+            },
+          },
+        }),
+      );
+      const res = await fetch(`${h.baseUrl}/api/servers/theta`, {
+        method: "PUT",
+        headers: { "Content-Type": "application/json" },
+        body: JSON.stringify({
+          // No config — server should preserve the existing one inside its
+          // write lock and apply only the settings patch.
+          settings: {
+            headers: [{ key: "X-Tenant", value: "acme" }],
+            metadata: [],
+            connectionTimeout: 0,
+            requestTimeout: 0,
+          },
+        }),
+      });
+      expect(res.status).toBe(200);
+      const stored = readConfig(h.configPath).mcpServers.theta as {
+        type?: string;
+        url?: string;
+        settings?: { headers: { key: string; value: string }[] };
+      };
+      expect(stored.type).toBe("streamable-http");
+      expect(stored.url).toBe("https://x.test/mcp");
+      expect(stored.settings?.headers).toEqual([
+        { key: "X-Tenant", value: "acme" },
+      ]);
+    });
+
+    it("rejects a non-object config on PUT with 400", async () => {
+      writeFileSync(
+        h.configPath,
+        JSON.stringify({
+          mcpServers: {
+            iota: { type: "stdio", command: "node" },
+          },
+        }),
+      );
+      const res = await fetch(`${h.baseUrl}/api/servers/iota`, {
+        method: "PUT",
+        headers: { "Content-Type": "application/json" },
+        body: JSON.stringify({
+          config: "not-an-object",
+        }),
+      });
+      expect(res.status).toBe(400);
+    });
+
     it("rejects a settings array (not an object) with 400", async () => {
       const res = await fetch(`${h.baseUrl}/api/servers`, {
         method: "POST",

core/mcp/remote/node/server.ts

@@ -853,8 +853,18 @@ export function createRemoteApp(
     } catch {
       return c.json({ error: "Invalid JSON body" }, 400);
     }
-    if (!body.config || typeof body.config !== "object") {
-      return c.json({ error: "Missing or invalid config" }, 400);
+    // Config on PUT: optional. If the field is omitted, preserve the
+    // existing transport config from disk. This makes "patch only settings"
+    // a first-class shape — callers like updateServerSettings don't have to
+    // snapshot the current config off in-memory state (which could race
+    // against a concurrent refresh and silently revert a separate edit).
+    // A provided config must structurally be an object; we let
+    // `normalizeServerType` do the lenient type coercion downstream.
+    if (
+      body.config !== undefined &&
+      (body.config === null || typeof body.config !== "object")
+    ) {
+      return c.json({ error: "Invalid config" }, 400);
     }
     // Settings on PUT have three intents:
     //   - field omitted (`undefined`)  → preserve the existing settings node
@@ -890,9 +900,19 @@ export function createRemoteApp(
         if (newId !== originalId && newId in current.mcpServers) {
           return c.json({ error: `Server '${newId}' already exists` }, 409);
         }
-        // Rebuild preserving insertion order; replace the original key in place
-        // so the file diff stays minimal when not renaming.
+        // Rebuild preserving insertion order; replace the original key in
+        // place so the file diff stays minimal when not renaming. Writing
+        // the full map back also means normalize-on-read self-heals any
+        // malformed settings node on *other* servers in the file — a
+        // deliberate side-effect of using `readMcpConfig` + full rewrite
+        // here.
         const existing = current.mcpServers[originalId]!;
+        // Split the existing entry into its config (everything except
+        // settings) and its settings, then apply patch semantics from the
+        // body to each.
+        const { settings: _existingSettings, ...existingConfig } = existing;
+        const nextConfig =
+          body.config !== undefined ? body.config : existingConfig;
         let nextSettings: InspectorServerSettings | undefined;
         switch (settingsIntent.kind) {
           case "preserve":
@@ -908,10 +928,7 @@ export function createRemoteApp(
         const next: MCPConfig = { mcpServers: {} };
         for (const [key, val] of Object.entries(current.mcpServers)) {
           if (key === originalId) {
-            next.mcpServers[newId] = buildStoredEntry(
-              body.config,
-              nextSettings,
-            );
+            next.mcpServers[newId] = buildStoredEntry(nextConfig, nextSettings);
           } else {
             next.mcpServers[key] = val;
           }

core/react/useServers.ts

@@ -144,28 +144,27 @@ export function useServers(opts: UseServersOptions): UseServersResult {
 
   const updateServerSettings = useCallback(
     async (id: string, settings: InspectorServerSettings): Promise<void> => {
-      const existing = servers.find((s) => s.id === id);
-      if (!existing) {
-        throw new Error(`Server '${id}' not found`);
-      }
+      // Settings-only PUT — we deliberately omit `config` so the route
+      // preserves the on-disk transport config inside its write lock.
+      // Reading `existing.config` from in-memory `servers` here would pin a
+      // stale snapshot at scheduling time and could silently revert a
+      // separate concurrent edit (e.g. a future file-watcher refreshing
+      // `servers` between debounce schedule and flush). The server is the
+      // single source of truth for config.
       const res = await doFetch(
         `${base}/api/servers/${encodeURIComponent(id)}`,
         {
           method: "PUT",
           headers: buildHeaders(authToken, true),
-          body: JSON.stringify({
-            id,
-            config: existing.config,
-            settings,
-          }),
+          body: JSON.stringify({ id, settings }),
         },
       );
       if (!res.ok) {
         throw new Error(await readErrorMessage(res));
       }
       await refresh();
     },
-    [base, authToken, doFetch, refresh, servers],
+    [base, authToken, doFetch, refresh],
   );
 
   const removeServer = useCallback(