fix(servers): address PR review on flatten (#1359)

- Single-source the Inspector-extension key list with `satisfies`
  (finding 1). `INSPECTOR_FIELD_KEY_MAP satisfies Record<keyof
  StoredInspectorFields, true>` in serverList.ts means a new field on
  the type forces a compile error here. The set is exported and
  re-used by the server route's smuggle guard, so adding `proxy` or
  similar in the future updates all three sites in lockstep. The PUT
  preserve path now uses a new `stripInspectorFields` helper that
  derives from the same source instead of hand-destructuring.
- Add read-path shape validation in `normalizeMcpServers` (finding 2):
  `headers: "oops"` / `metadata: 42` / `oauth: []` / non-numeric
  timeouts are dropped individually with a warn, so a hand-edited
  malformed file can't put garbage rows into the form. Mirrors the
  write-side `validateSettings` symmetry. New integration test covers
  each malformed shape with a partial fixture so a single bad key
  doesn't take out the whole entry.
- Align new warns to the `fileLogger || console.warn` fallback pattern
  (finding 3) via a small `logWarn` helper, so the legacy-drop and
  smuggle warns are visible in `npm run dev` where the logger is
  often unconfigured.
- Spec doc tweak: the CLI/TUI out-of-scope note's "writes to
  `settings.headers`" reference now reads "writes to the entry's
  top-level `headers` field on disk (post-#1358 flat shape)" (finding
  4). Avoids confusion for future readers.
- Nits: replace `current.mcpServers[originalId]!` with explicit
  narrowing; add a comment on the OAuth truthiness drop in
  `storedFieldsToInspectorSettings` mirroring the write-side comment.

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

Author: cliffhall

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

@@ -793,6 +793,49 @@ describe("/api/servers routes", () => {
       expect(stored).not.toHaveProperty("headers");
     });
 
+    it("drops individually malformed Inspector-extension fields on read (read-path shape validation)", async () => {
+      // A hand-edited mcp.json with a mix of valid and malformed
+      // Inspector-extension fields. `normalizeMcpServers` drops each bad
+      // field independently with a warn — so one wrong key doesn't take
+      // out the whole entry, and garbage values can't reach the form via
+      // the disk → memory converter.
+      writeFileSync(
+        h.configPath,
+        JSON.stringify({
+          mcpServers: {
+            bad: {
+              type: "streamable-http",
+              url: "https://x.test/mcp",
+              // Good: should round-trip
+              headers: { "X-Keep": "yes" },
+              // Bad: string instead of pair-array → drop
+              metadata: "oops",
+              // Bad: non-number timeout → drop
+              connectionTimeout: "30s",
+              // Good: valid number
+              requestTimeout: 60000,
+              // Bad: array instead of object → drop
+              oauth: ["not", "an", "object"],
+            },
+          },
+        }),
+      );
+
+      const getRes = await fetch(`${h.baseUrl}/api/servers`);
+      expect(getRes.status).toBe(200);
+      const getBody = (await getRes.json()) as {
+        mcpServers: Record<string, Record<string, unknown>>;
+      };
+      const fetched = getBody.mcpServers.bad!;
+      // Good fields survive.
+      expect(fetched.headers).toEqual({ "X-Keep": "yes" });
+      expect(fetched.requestTimeout).toBe(60000);
+      // Bad fields are dropped.
+      expect(fetched).not.toHaveProperty("metadata");
+      expect(fetched).not.toHaveProperty("connectionTimeout");
+      expect(fetched).not.toHaveProperty("oauth");
+    });
+
     it("loads a hand-edited file with top-level Claude Code-style `headers` (interop with `.mcp.json`)", async () => {
       // A user pastes a server entry copied from the Claude Code docs:
       // top-level `headers: { ... }`, no settings wrapper. GET should

core/mcp/remote/node/server.ts

@@ -33,9 +33,11 @@ import type {
 } from "../../types.js";
 import {
   DEFAULT_SEED_CONFIG,
+  INSPECTOR_FIELD_KEYS,
   inspectorSettingsToStoredFields,
   normalizeServerType,
   storedFieldsToInspectorSettings,
+  stripInspectorFields,
 } from "../../serverList.js";
 import { RemoteSession } from "./remote-session.js";
 import { createTokenAuthProvider } from "./tokenAuthProvider.js";
@@ -792,40 +794,128 @@ export function createRemoteApp(
 
   // --- /api/servers (server list backed by mcp.json) ---
 
+  // Match handleWatcherError's pattern: log to the configured logger if
+  // present, fall back to console.warn for visibility in `npm run dev`
+  // where no logger is wired up. Both gates below feed user-visible
+  // signals (legacy on-disk shape, client bug smuggling fields), so a
+  // silent drop is worse than a console line.
+  const logWarn = (bindings: Record<string, unknown>, msg: string): void => {
+    if (fileLogger) {
+      fileLogger.warn(bindings, msg);
+    } else {
+      console.warn("[mcp.json]", msg, bindings);
+    }
+  };
+
   // Defensive normalize so editor-edited files with type:"http" or missing
   // type round-trip into the canonical form on every read. Inspector-
   // extension fields (headers / metadata / connectionTimeout / requestTimeout
-  // / oauth) sit at the top level of each entry post-#1358; `normalizeServerType`
-  // spreads them through unchanged. Legacy `settings` nodes written by the
-  // pre-#1358 build (one #1352 release on v2/main that never shipped stable)
-  // are dropped with a warn — those persisted headers / metadata / timeouts
-  // / OAuth credentials are intentionally lost on first read (hard cutover
-  // per #1358 decision 4). Users re-enter via the form or hand-edit into the
+  // / oauth) sit at the top level of each entry post-#1358; this function
+  // is the read-side gate — it validates each field's shape and drops
+  // anything malformed with a logged warn, so a hand-edited file with
+  // `headers: "oops"` can't put garbage rows into the form.
+  //
+  // Legacy `settings` nodes written by the pre-#1358 build (one #1352
+  // release on v2/main that never shipped stable) are dropped with a
+  // warn — those persisted headers / metadata / timeouts / OAuth
+  // credentials are intentionally lost on first read (hard cutover per
+  // #1358 decision 4). Users re-enter via the form or hand-edit into the
   // flat shape.
+  const isStringRecord = (v: unknown): v is Record<string, string> => {
+    if (v === null || typeof v !== "object" || Array.isArray(v)) return false;
+    for (const val of Object.values(v as Record<string, unknown>)) {
+      if (typeof val !== "string") return false;
+    }
+    return true;
+  };
+  const isKvArray = (v: unknown): v is { key: string; value: string }[] => {
+    if (!Array.isArray(v)) return false;
+    return v.every(
+      (e) =>
+        e !== null &&
+        typeof e === "object" &&
+        typeof (e as Record<string, unknown>).key === "string" &&
+        typeof (e as Record<string, unknown>).value === "string",
+    );
+  };
+  const isOauthObject = (
+    v: unknown,
+  ): v is { clientId?: string; clientSecret?: string; scopes?: string } => {
+    if (v === null || typeof v !== "object" || Array.isArray(v)) return false;
+    const o = v as Record<string, unknown>;
+    for (const k of ["clientId", "clientSecret", "scopes"] as const) {
+      if (o[k] !== undefined && typeof o[k] !== "string") return false;
+    }
+    return true;
+  };
+  const isNonNegNumber = (v: unknown): v is number =>
+    typeof v === "number" && v >= 0;
+
   const normalizeMcpServers = (
     raw: unknown,
   ): Record<string, StoredMCPServer> => {
     if (!raw || typeof raw !== "object") return {};
     const out: Record<string, StoredMCPServer> = {};
     for (const [id, val] of Object.entries(raw as Record<string, unknown>)) {
       if (!val || typeof val !== "object") continue;
-      // Strip a legacy nested `settings` node before normalizeServerType
-      // would spread it through. The keys that LIVE flat on disk
-      // (`headers` / `metadata` / `oauth` / ...) are not touched here —
-      // they pass through verbatim and the disk → memory converter
-      // (`storedFieldsToInspectorSettings`) is the read-side gate.
       const valObj = val as Record<string, unknown>;
+
+      // Strip a legacy nested `settings` node before normalizeServerType
+      // would spread it through. Hard cutover per decision 4.
       if ("settings" in valObj) {
-        fileLogger?.warn(
+        logWarn(
           { route: "/api/servers", id, droppedKey: "settings" },
           "Dropping legacy `settings` node from mcp.json entry — fields now live at the top level. Re-enter via the settings form or hand-edit the file into the flat shape.",
         );
-        const { settings: _legacy, ...rest } = valObj;
-        out[id] = normalizeServerType(
-          rest as Record<string, unknown> & { type?: string },
-        ) as StoredMCPServer;
-        continue;
+        delete valObj.settings;
       }
+
+      // Per-field shape validation on the Inspector-extension keys. The
+      // write path (validateSettings) is symmetric — same checks. Drop
+      // bad shapes individually so a single malformed key doesn't take
+      // out the rest of the entry; log so the user can fix the file.
+      if ("headers" in valObj && !isStringRecord(valObj.headers)) {
+        logWarn(
+          { route: "/api/servers", id, droppedKey: "headers" },
+          "Dropping malformed `headers` field — expected `Record<string, string>`.",
+        );
+        delete valObj.headers;
+      }
+      if ("metadata" in valObj && !isKvArray(valObj.metadata)) {
+        logWarn(
+          { route: "/api/servers", id, droppedKey: "metadata" },
+          "Dropping malformed `metadata` field — expected `Array<{ key: string, value: string }>`.",
+        );
+        delete valObj.metadata;
+      }
+      if (
+        "connectionTimeout" in valObj &&
+        !isNonNegNumber(valObj.connectionTimeout)
+      ) {
+        logWarn(
+          { route: "/api/servers", id, droppedKey: "connectionTimeout" },
+          "Dropping malformed `connectionTimeout` field — expected non-negative number.",
+        );
+        delete valObj.connectionTimeout;
+      }
+      if (
+        "requestTimeout" in valObj &&
+        !isNonNegNumber(valObj.requestTimeout)
+      ) {
+        logWarn(
+          { route: "/api/servers", id, droppedKey: "requestTimeout" },
+          "Dropping malformed `requestTimeout` field — expected non-negative number.",
+        );
+        delete valObj.requestTimeout;
+      }
+      if ("oauth" in valObj && !isOauthObject(valObj.oauth)) {
+        logWarn(
+          { route: "/api/servers", id, droppedKey: "oauth" },
+          "Dropping malformed `oauth` field — expected `{ clientId?, clientSecret?, scopes? }`.",
+        );
+        delete valObj.oauth;
+      }
+
       out[id] = normalizeServerType(
         valObj as Record<string, unknown> & { type?: string },
       ) as StoredMCPServer;
@@ -849,14 +939,16 @@ export function createRemoteApp(
   // `id` is threaded through purely so the warning correlates to a specific
   // entry; the route ultimately reaches here via POST or PUT and both know
   // the target id at call time.
-  const SMUGGLE_GUARDED_KEYS = [
+  //
+  // The set of guarded keys is the source-of-truth `INSPECTOR_FIELD_KEYS`
+  // plus the legacy `"settings"` wrapper key. Adding a new Inspector-
+  // extension field to `StoredMCPServer` propagates through the
+  // `satisfies` check in `serverList.ts` and updates this guard
+  // automatically; nothing to remember to update here.
+  const SMUGGLE_GUARDED_KEYS: ReadonlySet<string> = new Set<string>([
+    ...INSPECTOR_FIELD_KEYS,
     "settings",
-    "headers",
-    "metadata",
-    "connectionTimeout",
-    "requestTimeout",
-    "oauth",
-  ] as const;
+  ]);
   const buildStoredEntry = (
     id: string,
     config: unknown,
@@ -873,14 +965,14 @@ export function createRemoteApp(
     const smuggled: string[] = [];
     const configOnly: Record<string, unknown> = {};
     for (const [k, v] of Object.entries(configObj)) {
-      if ((SMUGGLE_GUARDED_KEYS as readonly string[]).includes(k)) {
+      if (SMUGGLE_GUARDED_KEYS.has(k)) {
         smuggled.push(k);
         continue;
       }
       configOnly[k] = v;
     }
     if (smuggled.length > 0) {
-      fileLogger?.warn(
+      logWarn(
         { route: "/api/servers", id, smuggledKeys: smuggled },
         "Stripping Inspector-extension keys from request body's `config` — those must travel through the top-level `settings` field, not nested inside `config`.",
       );
@@ -1154,29 +1246,30 @@ export function createRemoteApp(
         // malformed settings node on *other* servers in the file — a
         // deliberate side-effect of using `readMcpConfig` + full rewrite
         // here.
-        const existing = current.mcpServers[originalId]!;
+        const existing = current.mcpServers[originalId];
+        if (!existing) {
+          // The `in` check above guarantees this branch is unreachable;
+          // narrowing without the non-null assertion keeps TS happy and
+          // makes the contract explicit for future refactors.
+          return c.json(
+            { error: `Server '${originalId}' not found` },
+            404,
+          );
+        }
         // Split the existing entry into its SDK-only config (no Inspector-
         // extension fields) and its lifted settings, then apply patch
         // semantics from the body to each. The flat-on-disk Inspector
         // fields are sliced off `existing` so the preserve-on-omit `config`
         // path doesn't accidentally carry them through as raw disk keys —
         // they need to flow through `buildStoredEntry` so empty `settings`
         // intents can clear them.
-        const {
-          headers: existingHeaders,
-          metadata: existingMetadata,
-          connectionTimeout: existingCT,
-          requestTimeout: existingRT,
-          oauth: existingOauth,
-          ...existingConfig
-        } = existing;
-        const existingSettings = storedFieldsToInspectorSettings({
-          headers: existingHeaders,
-          metadata: existingMetadata,
-          connectionTimeout: existingCT,
-          requestTimeout: existingRT,
-          oauth: existingOauth,
-        });
+        //
+        // `stripInspectorFields` + `storedFieldsToInspectorSettings` both
+        // derive from the same `INSPECTOR_FIELD_KEYS` set, so adding a new
+        // Inspector-extension field to `StoredMCPServer` doesn't silently
+        // leak through this preserve path.
+        const existingConfig = stripInspectorFields(existing);
+        const existingSettings = storedFieldsToInspectorSettings(existing);
         const nextConfig =
           body.config !== undefined ? body.config : existingConfig;
         let nextSettings: InspectorServerSettings | undefined;

core/mcp/serverList.ts

@@ -95,6 +95,10 @@ export function storedFieldsToInspectorSettings(
     connectionTimeout: stored.connectionTimeout ?? 0,
     requestTimeout: stored.requestTimeout ?? 0,
   };
+  // Truthiness drops empty-string OAuth fields — mirrors the write-side
+  // coercion in `validateSettings` (server.ts) so a round-trip can't
+  // accidentally surface `oauthClientId: ""` to the form, where the
+  // OAuth manager would misread it as "configured."
   if (stored.oauth?.clientId) settings.oauthClientId = stored.oauth.clientId;
   if (stored.oauth?.clientSecret)
     settings.oauthClientSecret = stored.oauth.clientSecret;
@@ -157,18 +161,51 @@ export function inspectorSettingsToStoredFields(
 }
 
 /**
- * Set of known Inspector-extension keys that live at the top level of a
- * `StoredMCPServer`. Used by the disk → memory converter to slice the
- * extension fields off the entry so what remains is a pure SDK
- * `MCPServerConfig`.
+ * Source of truth for the set of Inspector-extension keys that live at the
+ * top level of a `StoredMCPServer`. Enumerated through a map keyed by
+ * `keyof StoredInspectorFields` with a `satisfies` constraint so any new
+ * field added to the type forces a compile error here — the disk → memory
+ * converter slice, the server-side smuggle guard, and the PUT preserve
+ * path all derive from this single source.
+ *
+ * Don't replace this with a hand-typed string array — `satisfies
+ * Record<keyof StoredInspectorFields, true>` is what gives us the
+ * exhaustive check. `as const` plus the `satisfies` clause yields a
+ * narrow tuple-of-literals type that downstream consumers can use as
+ * `(keyof StoredInspectorFields)[]`.
  */
-const INSPECTOR_FIELD_KEYS = new Set<keyof StoredInspectorFields>([
-  "headers",
-  "metadata",
-  "connectionTimeout",
-  "requestTimeout",
-  "oauth",
-]);
+const INSPECTOR_FIELD_KEY_MAP = {
+  headers: true,
+  metadata: true,
+  connectionTimeout: true,
+  requestTimeout: true,
+  oauth: true,
+} as const satisfies Record<keyof StoredInspectorFields, true>;
+
+export const INSPECTOR_FIELD_KEYS = new Set(
+  Object.keys(INSPECTOR_FIELD_KEY_MAP) as (keyof StoredInspectorFields)[],
+);
+
+/**
+ * Strip the Inspector-extension fields off a `StoredMCPServer` so the
+ * remainder is the pure SDK config shape the PUT route's preserve path
+ * needs. Source-of-truth driven via `INSPECTOR_FIELD_KEYS` so adding a
+ * new extension field doesn't silently leak through this slice — the
+ * `satisfies` constraint above forces the map update, which propagates
+ * here.
+ */
+export function stripInspectorFields(
+  stored: StoredMCPServer,
+): MCPServerConfig {
+  const out: Record<string, unknown> = {};
+  for (const [k, v] of Object.entries(
+    stored as unknown as Record<string, unknown>,
+  )) {
+    if (INSPECTOR_FIELD_KEYS.has(k as keyof StoredInspectorFields)) continue;
+    out[k] = v;
+  }
+  return out as unknown as MCPServerConfig;
+}
 
 /**
  * Convert the on-disk `MCPConfig` into the `ServerEntry[]` the Servers screen

specification/v2_servers_file.md

@@ -261,4 +261,4 @@ Each server entry may carry these Inspector-extension fields at the top level:
 - File watching for hot reload of external edits.
 - Per-server tags / folders / groups.
 - Export current list as JSON.
-- CLI/TUI: switch their default `--config` to `getDefaultMcpConfigPath()` when no `--config` flag is given. Touch when those clients are wired up to v2. While porting, re-add a `--header` flag that writes to `settings.headers` rather than to `MCPServerConfig`.
+- CLI/TUI: switch their default `--config` to `getDefaultMcpConfigPath()` when no `--config` flag is given. Touch when those clients are wired up to v2. While porting, re-add a `--header` flag that writes to the entry's top-level `headers` field on disk (post-#1358 flat shape) rather than to `MCPServerConfig`.