feat(servers): flatten mcp.json settings to top-level fields (#1358)

[cliffhall]

Summary

• Disk shape flattens: each server entry's Inspector-extension fields (headers, metadata, connectionTimeout, requestTimeout, oauth) live as direct keys alongside type / url, matching the .mcp.json shape Claude Code / Cursor / Cline write — hand-edited headers: { ... } from those tools' docs is now readable on first connect.
• headers on disk becomes a Record<string, string> (per Eliminate the mcp.json settings node and promote its fields to the top level #1358 decision 1); oauth becomes a nested { clientId?, clientSecret?, scopes? } object (decision 2). Inspector-only metadata / timeouts flatten as-is (decision 3).
• In-memory + wire shape unchanged: InspectorServerSettings keeps pair-array headers and flat oauth* fields because the form needs them in that shape for controlled-component editing. Conversion at the persistence boundary lives in serverList.ts (storedFieldsToInspectorSettings / inspectorSettingsToStoredFields).
• Wire envelope { id, config, settings } on PUT /api/servers/:id preserved per decision 5; the preserve/clear/apply semantics from feat(servers): persist per-server settings to mcp.json (#1352) #1353 are intact end-to-end.
• Hard cutover on legacy files per decision 4: normalizeMcpServers drops a pre-Eliminate the mcp.json settings node and promote its fields to the top level #1358 nested settings block on read with a one-line warn; persisted headers / metadata / timeouts / OAuth credentials are intentionally lost.

Closes #1358.

Approach

The implementation took Approach C from the design discussion: only the on-disk shape changes; the wire + memory + form shapes all stay as today. This achieves the issue's stated motivation (interop with hand-edited .mcp.json files compatible with Claude / Cursor / Cline ) with the smallest diff — the form / modal / App.tsx / InspectorClient / transport plumbing is all untouched. The PR description in #1358 originally hinted at also flattening the wire shape (Approach A), but the user-facing benefit is identical and Approach C avoids a stateful-form refactor.

Files changed

• core/mcp/types.ts — StoredMCPServer flattens; InspectorServerSettings unchanged.
• core/mcp/serverList.ts — new storedFieldsToInspectorSettings / inspectorSettingsToStoredFields helpers, plus updated mcpConfigToServerEntries / serverEntriesToMcpConfig.
• core/mcp/remote/node/server.ts — normalizeMcpServers drops legacy settings with warn; buildStoredEntry splats settings.* into top-level disk keys; smuggle guard widens to all six Inspector-extension keys; PUT preserve/clear paths read flat fields off existing.
• specification/v2_servers_file.md — example + per-server-settings section rewritten for the flat shape.
• Tests: serverList unit (26 cases, includes round-trip + legacy-drop + Claude-Code-fixture coverage), servers-route integration (40 cases, includes hard-cutover read + Claude-Code interop), useServers unit (two fixture updates).

Test plan

• `npm run validate` — 142 files / 1689 tests
• `npm run test:integration` — including 40 servers-route cases (POST/PUT/DELETE flat shape, legacy-drop, interop)
• `npm run test:storybook` — 69 files / 306 tests
• Coverage gate satisfied (`serverList.ts` 100% statements, `server.ts` 95%)

Acceptance criteria

• mcp.json files written by Inspector have no `settings` node
• Loading file with top-level `headers` works (wire `Authorization` lands on first connect)
• Legacy nested `settings` is dropped on read with a logged warn
• PUT body shape unchanged from feat(servers): persist per-server settings to mcp.json (#1352) #1353 (preserve/clear/apply semantics intact)
• ServerConfigModal vs ServerSettingsForm UI split unchanged
• Integration tests cover (a) top-level headers, (b) round-trip, (c) legacy drop, (d) settings-only PUT preserves config
• specification/v2_servers_file.md reflects the flat shape

🤖 Generated with Claude Code

[cliffhall]

@claude review

[claude]

Claude finished @cliffhall's task in 2m 43s —— View job

---

Code review — feat(servers): flatten mcp.json settings to top-level fields

• Read PR diff against origin/v2/main
• Review core/mcp/types.ts
• Review core/mcp/serverList.ts
• Review core/mcp/remote/node/server.ts
• Review specification/v2_servers_file.md
• Review test coverage
• Post review feedback

Overall

Clean execution of Approach C. The invariant "disk shape flattens; wire/memory/form unchanged" is held cleanly by localizing the conversion to two named helpers (storedFieldsToInspectorSettings / inspectorSettingsToStoredFields) at the persistence boundary, and the preserve/clear/apply PUT semantics from #1353 survive intact. Test coverage is thorough: round-trip, legacy-drop, Claude-Code .mcp.json interop, smuggle guard (POST + PUT with settings:null), empty-row drops, zero-suppression, concurrent writes. Nothing blocks merge — observations below are maintainability nits and a couple of soft-read defenses.

Observations

1. [Low · maintainability] Three parallel enumerations of the extension keys, with no compile-time coupling.

The five flat keys (headers, metadata, connectionTimeout, requestTimeout, oauth) are enumerated in:

• StoredInspectorFields type — core/mcp/serverList.ts:60
• INSPECTOR_FIELD_KEYS set — core/mcp/serverList.ts:165
• SMUGGLE_GUARDED_KEYS tuple — core/mcp/remote/node/server.ts:852 (also includes the legacy "settings")
• PUT-handler destructure — core/mcp/remote/node/server.ts:1165-1172

INSPECTOR_FIELD_KEYS is typed Set<keyof StoredInspectorFields> but the array literal won't statically fail if you forget a key when adding a new extension field — the type only constrains what you can put in, not that all members are present. SMUGGLE_GUARDED_KEYS is plain string-tuple and has no relation to the type at all.

Suggestion: either derive the two runtime enumerations from satisfies Record<keyof StoredInspectorFields, true> so a new key on the type forces updates downstream, or add a small exhaustive-check unit test (Object.keys<StoredInspectorFields> matches the set). Not urgent — today's five keys are stable — but next time someone adds proxy or auth, all three sites need to be updated and nothing will catch a miss.

2. [Low · defense] Read-path has no shape validation for Inspector-extension fields.

storedFieldsToInspectorSettings (core/mcp/serverList.ts:77-103) trusts the disk: a hand-edited headers: "oops" runs Object.entries("oops") producing [['0','o'], ['1','o'], ['2','p'], ['3','s']] which lands in the form as four garbage rows; oauth: 42 silently falls through .clientId as undefined. The write path has validateSettings, so we're asymmetric.

Not a bug (lenient-on-read is consistent with normalizeServerType), and the blast radius is "user hand-edits mcp.json into a malformed state," but a light type-guard here would turn malformed disk values into dropped fields + a log warn instead of garbage-in-the-form.

3. [Very low · nit] Warning visibility in dev without a logger.

normalizeMcpServers legacy-drop (core/mcp/remote/node/server.ts:819) and buildStoredEntry smuggle-guard (core/mcp/remote/node/server.ts:883) use fileLogger?.warn(...) only. Elsewhere in the file, handleWatcherError (line 378-385) falls back to console.warn when no logger is configured. Aligning the new warnings to the same fileLogger?.warn(...) || console.warn(...) pattern would surface these in dev (where the logger is often undefined) — useful since both warnings signal a potential client/user bug.

4. [Very low · doc rot] Stale reference in the spec.

specification/v2_servers_file.md:264 (CLI/TUI out-of-scope note) still reads "re-add a --header flag that writes to settings.headers". Context makes clear this means the in-memory InspectorServerSettings.headers, but after this PR's flattening, a future reader could easily misparse it as a disk shape. Worth a one-line tweak: e.g. "...writes to InspectorServerSettings.headers" or "the settings pair-array".

5. [Low · UX] Hard cutover on legacy files is irrecoverable by accident.

The PR description (and core/mcp/remote/node/server.ts:802-804) acknowledges that headers / metadata / timeouts / OAuth client secrets from pre-#1358 files are intentionally lost on first read. Decision 4 is fine and the blast radius (v2/main dogfooders between #1353 merge and now) is small — but clientSecret is the one field that's actually painful to re-enter, and the drop is one-shot.

Nice-to-have: normalizeMcpServers could copy the legacy settings block into an adjacent _legacy_settings field (or write mcp.json.legacy alongside the first post-#1358 save), so a user who notices the drop can recover by hand. Not blocking — decision 4 is explicit — but the cost is a few lines and saves the one user who comes back from vacation to find their OAuth creds gone.

Nits

• core/mcp/remote/node/server.ts:1157: const existing = current.mcpServers[originalId]!; — non-null assertion right after the in check. Swapping to const existing = current.mcpServers[originalId]; if (!existing) {...} would let TS narrow without the !. Style-only.
• core/mcp/serverList.ts:98-101: the three if (stored.oauth?.clientId) ... branches use truthiness, which intentionally drops empty strings. The comment in validateSettings (server.ts:962-964) explains this for the write path; a one-line comment here would make the read path just as self-explanatory.

Strengths

• StoredInspectorFields = Pick<StoredMCPServer, ...> (core/mcp/serverList.ts:60) as the shared type for both converter directions — no drift between read/write.
• Zero-suppression (connectionTimeout > 0, non-empty object/array checks) keeps file diffs minimal for unchanged entries — important for the "hand-edit and round-trip" promise.
• validateSettings picks-and-builds (server.ts:963-982) rather than spreading, so unknown stowaways can't reach disk via the envelope. The "smuggle via config" test (servers-route.test.ts:655-721) complements this on the other axis.
• The settings-only PUT preserves config test (servers-route.test.ts:549-581) and its mirror (updateServer preserves settings across config update in useServers.test.tsx:432-474) pin the full two-way patch semantics.
  · branch: feat/1358-flatten-mcp-json-settings

[cliffhall]

Thanks for the review. Five findings + two nits, all addressed in 3ce3420 except finding 5 which I'm leaving deliberate (rationale below).

1. Three parallel enumerations → single source via satisfies — addressed. INSPECTOR_FIELD_KEY_MAP in serverList.ts is now satisfies Record<keyof StoredInspectorFields, true>, so a new field on the type forces a compile error in the map. The runtime INSPECTOR_FIELD_KEYS set is derived from Object.keys of that map and exported; the server route's SMUGGLE_GUARDED_KEYS is now new Set([...INSPECTOR_FIELD_KEYS, "settings"]). Also added a stripInspectorFields(stored): MCPServerConfig helper that derives from the same source — the PUT route's preserve path now calls it instead of hand-destructuring. Three sites, one source of truth.

2. Read-path shape validation — addressed in normalizeMcpServers. Each Inspector-extension field gets a small type guard (isStringRecord / isKvArray / isOauthObject / isNonNegNumber); a malformed field is dropped individually with a logWarn instead of falling through to garbage in the form. Drops are field-scoped so one bad key (e.g. metadata: "oops") doesn't take out the rest of the entry. New integration test drops individually malformed Inspector-extension fields on read (read-path shape validation) covers a mixed-validity fixture. storedFieldsToInspectorSettings stays purely structural — validation lives at the server route boundary where the logger is, which keeps the converter usable from the client side too.

3. Warn visibility in dev — addressed. New logWarn(bindings, msg) helper at the top of createRemoteApp matches handleWatcherError's pattern: log to fileLogger if present, fall back to console.warn("[mcp.json]", msg, bindings). Both the legacy-drop and the smuggle guard now route through it, so the warning surfaces in npm run dev where the logger is often undefined.

4. Stale spec reference — addressed. The CLI/TUI out-of-scope line (v2_servers_file.md:264) now reads "re-add a --header flag that writes to the entry's top-level headers field on disk (post-#1358 flat shape)" — explicit about which shape it's talking about.

5. Hard-cutover sidecar for legacy settings — skipping, deliberate. Decision 4 in the issue is "hard cutover" and the user picked it knowing the blast radius. Writing mcp.json.legacy alongside the first post-#1358 save is a defensible nicety but it's a meaningful behavior change beyond what was specified (creates a new file on disk under the user's ~/.mcp-inspector/ dir, and now has its own questions: when to delete it, whether to surface a UI prompt, etc.). The clientSecret-loss point is real, but the user is one of the few people with personal data in v2/main's mcp.json today and they can manually migrate before merging. If they want a sidecar I'd open it as a small follow-up issue.

Nits — both addressed:

• current.mcpServers[originalId]! → explicit if (!existing) return c.json(...404) narrowing. The in check above makes the branch unreachable, but the explicit form is clearer for future refactors and drops the non-null assertion.
• Added a one-line comment on storedFieldsToInspectorSettings explaining that empty-string OAuth fields are intentionally dropped via truthiness — mirrors the write-side comment in validateSettings.

All checks green:

• npm run validate — 142 files / 1690 tests (was 1689; +1 from the new shape-validation test)
• npm run test:integration — servers-route 41/41 (was 40); legacy + Claude Code interop + new malformed-shape coverage
• npm run test:storybook — 69 files / 306 tests
• Coverage: serverList.ts 100% lines, server.ts 93.73%, well above the per-file gate

[cliffhall]

@claude review

[claude]

Claude finished @cliffhall's task in 2m 11s —— View job

---

Follow-up review — fix commit 3ce34206

• Diff the fix commit against origin/v2/main to see what changed since the last review
• Verify finding 1 fix: single source for Inspector-extension keys via satisfies
• Verify finding 2 fix: read-path shape validation in normalizeMcpServers
• Verify finding 3 fix: logWarn helper with console fallback
• Verify finding 4 fix: spec wording for CLI --header note
• Verify nit fixes: non-null assertion removal + OAuth truthiness comment
• Check for any regressions or new issues
• Post review feedback

Overall

All four addressed findings land cleanly, the deliberate skip on finding 5 (legacy-settings sidecar) is a reasonable read of decision 4, and both nits are resolved. The INSPECTOR_FIELD_KEY_MAP satisfies Record<keyof StoredInspectorFields, true> pattern is the right move — it makes a future proxy / auth / etc. field fail at compile time at serverList.ts:177-183 and the miss ripples through to the smuggle guard and preserve path for free. Ship it.

Verification of each fix

Finding 1 — single source of truth. serverList.ts:177-187 — INSPECTOR_FIELD_KEY_MAP is exhaustively typed via satisfies; the exported INSPECTOR_FIELD_KEYS set is derived from Object.keys(MAP). stripInspectorFields (serverList.ts:197-208) consumes it; mcpConfigToServerEntries (serverList.ts:226) consumes it; SMUGGLE_GUARDED_KEYS at server.ts:948-951 is literally new Set([...INSPECTOR_FIELD_KEYS, "settings"]); PUT preserve path at server.ts:1271-1272 uses stripInspectorFields instead of hand-destructuring. Four sites, one source — verified. Added proxy to StoredInspectorFields mentally and confirmed TS would flag the map at compile time.

Finding 2 — read-path shape validation. server.ts:824-852 defines isStringRecord / isKvArray / isOauthObject / isNonNegNumber; server.ts:877-917 applies each guard independently in normalizeMcpServers with a scoped logWarn. The integration test at servers-route.test.ts:796-836 confirms per-field independence on a mixed-validity fixture (headers survives, metadata/connectionTimeout/oauth drop; requestTimeout survives — exactly the invariant the fix promises). Note the guards go beyond just structural checks — isNonNegNumber rejects "30s" strings and negative numbers, which matches validateSettings's write-side symmetry. Verified.

Finding 3 — warn visibility. server.ts:802-808 logWarn matches handleWatcherError's fileLogger || console.warn pattern. Routed through at server.ts:866-869, 877-917 (five call sites), and 974-978. Dev runs without a configured logger will now see [mcp.json] warnings on stderr.

Finding 4 — spec doc. specification/v2_servers_file.md:264 now explicitly disambiguates "top-level headers field on disk (post-#1358 flat shape)" — no more ambiguity about which shape the CLI --header flag should write to.

Nit 1 — non-null assertion. server.ts:1249-1258 — the ! is gone, replaced with an explicit if (!existing) return 404. The in check at line 1237 makes the branch unreachable, but the explicit form both narrows cleanly and documents the contract. The 404 body matches the earlier one, so behavior is unchanged.

Nit 2 — OAuth truthiness comment. serverList.ts:98-101 now calls out that the truthiness gate drops empty-string OAuth fields to prevent the form from treating "" as configured. Mirrors the write-side comment.

New observations (all minor, none blocking)

A. [Very low · nit] Mutation of caller-provided input in normalizeMcpServers.

The pre-fix code did const { settings: _legacy, ...rest } = valObj (non-mutating); the post-fix code does delete valObj.settings / delete valObj.headers / etc. on the caller's object graph. This is safe today — normalizeMcpServers is only called on freshly-parsed JSON from parseStore at server.ts:1110 and 1121 and the caller doesn't retain another reference — but it's a small surprise in an otherwise pure-looking helper. Not worth refactoring for one call site, but worth a one-line comment like // Mutates valObj in place — safe because the only callers pass in freshly parsed JSON. would save a future reader a double-take.

B. [Very low · nit] Type-guard functions scoped inside createRemoteApp.

isStringRecord / isKvArray / isOauthObject / isNonNegNumber at server.ts:824-852 are pure and closed over nothing. They could move to module scope (or to serverList.ts alongside the converter) and be testable in isolation. Co-locating them with the only gate that uses them is also defensible; file it under "nice to have" only if the same guards end up needed elsewhere.

C. [Very low · nit] isOauthObject is lenient on extra keys.

It verifies clientId / clientSecret / scopes are string-or-undefined but allows unknown extra keys to pass through. Then storedFieldsToInspectorSettings only pulls the three known fields, so extras are silently dropped on round-trip. That's probably the right trade-off for forward-compat (future Inspector OAuth fields don't break reads on old builds), but worth noting that "lenient read" is intentional here — same pattern as normalizeServerType spreading through unknown config keys. No change suggested.

D. [Low · soft defense] isNonNegNumber accepts Infinity.

Infinity >= 0 is true, so connectionTimeout: Infinity in a hand-edited file would pass the guard. Practically harmless (the SDK would treat it as "effectively no timeout"), but if you want tight parity with sane integer inputs, adding Number.isFinite(v) to the guard costs one check. Very much optional.

Strengths

• The satisfies Record<keyof StoredInspectorFields, true> pattern is exactly the right tool for this problem — enforces exhaustiveness at the type level without the runtime cost of an exhaustive-check helper. This pattern is worth a # architecture note in AGENTS.md if similar field-lists appear elsewhere.
• Per-field drop granularity in normalizeMcpServers is the right UX — a single malformed key doesn't take out the whole entry, and the scoped warn tells the user exactly which field to fix. The integration test nails this invariant explicitly.
• stripInspectorFields + storedFieldsToInspectorSettings symmetry in the PUT preserve path is now textually obvious at server.ts:1271-1272 — both one-liners sourced from the same key set. Replaces the six-variable destructure that previously duplicated the key list inline.
• Acknowledging the skip on finding 5 explicitly (PR comment) is good process — decision 4 is the authoritative source and expanding beyond spec under a "review fix" commit would be a scope creep.
  · branch: feat/1358-flatten-mcp-json-settings

[cliffhall]

Thanks for the second-pass approval. Picked up the two of four new observations that warranted code changes — f94cd85:

A — in-place mutation comment: added a one-line comment on valObj in normalizeMcpServers calling out that the delete calls mutate the caller's object, with the safety rationale (only callers are readMcpConfig and the GET seed branch, both pass freshly-parsed JSON with no retained reference). Cheap, saves a double-take.

D — isNonNegNumber accepts Infinity: addressed. Guard is now typeof v === "number" && Number.isFinite(v) && v >= 0, which rejects Infinity and NaN alongside non-numbers. Matches validateSettings's write-side intent. The harm was minimal (the SDK treats Infinity as "no timeout") but the symmetry is worth a single extra check.

B — type guards at module scope: skipped. They're closing over nothing and would move cleanly, but a single call site doesn't justify the file split. If a future surface needs them, that's the moment to lift.

C — isOauthObject lenient on extra keys: skipped per your own note — this matches normalizeServerType's spread-unknown-keys forward-compat pattern. Future OAuth fields don't break reads on older builds; the form-side dropping of unknown extras keeps the in-memory shape clean.

Also took note of the suggestion to document the satisfies Record<keyof T, true> pattern in AGENTS.md — worth a follow-up issue if similar key-lists crop up elsewhere in v2. Not pulling that into this PR.

Validate still green: 142 files / 1690 tests / 306 storybook, coverage gate satisfied.

[cliffhall]

@claude review

[claude]

Claude finished @cliffhall's task in 1m 15s —— View job

---

Third-pass review — fix commit f94cd85c

• Diff f94cd85c against 3ce34206 to isolate third-round changes
• Verify observation A fix: in-place mutation comment on normalizeMcpServers
• Verify observation D fix: isNonNegNumber rejects Infinity/NaN
• Confirm skips on B (module-scope guards) and C (lenient OAuth extras) are documented
• Scan for any new issues or regressions introduced by the fix
• Post review feedback

Overall

f94cd85c is a 9-line, single-file patch confined to the two observations you picked up. Both land cleanly, the skip rationales for B and C are sound, and nothing regressed. Ship it.

Verification of each fix

Observation A — in-place mutation comment. core/mcp/remote/node/server.ts:865-868 now documents the delete valObj.* calls as a deliberate in-place mutation and names the safety precondition (freshly-parsed JSON with no retained caller reference). Reader-friendly and the comment sits directly above the first mutation — hard to miss.

Observation D — isNonNegNumber rejects Infinity/NaN. core/mcp/remote/node/server.ts:851-856 — guard is now typeof v === "number" && Number.isFinite(v) && v >= 0. Number.isFinite is correct here (not isFinite, which would coerce strings) and rejects both Infinity and NaN. The comment explicitly references validateSettings for the write-side symmetry claim — verified at server.ts:1074-1078 where the write path uses typeof s.connectionTimeout === "number" && s.connectionTimeout > 0 (technically tighter: write also excludes zero; read accepts zero-as-infinity, but that's the pre-existing read/write asymmetry for "timeout disabled", not something this patch introduced).

Skip on B (module-scope guards) / C (lenient OAuth extras). Both called out explicitly in your reply. The reasoning is self-consistent: B is a one-call-site lift that costs a file split for no concrete payoff; C mirrors normalizeServerType's "spread unknown keys" forward-compat pattern, which already exists in the same function, so aligning on it is better than introducing a second shape-tightness policy.

One observation on the new changes

[Very low · doc nit] The mutation comment mis-names one of the callers.

core/mcp/remote/node/server.ts:867 says "the only callers (readMcpConfig and the GET handler's seed branch)". Checking the actual callers:

• server.ts:1118 — readMcpConfig ✅
• server.ts:1129 — GET handler, but this is the non-seed branch. The seed branch (line 1124-1127) runs when raw === null, writes DEFAULT_SEED_CONFIG, and never calls normalizeMcpServers. The call at line 1129 is in the else path, after a file was successfully read and parsed.

So the comment's characterization of "seed branch" is inverted — the correct phrasing would be "the GET handler's read path" or "the non-seed branch of the GET handler." The safety rationale (both callers pass freshly-parsed JSON) is still accurate; it's just the branch label that's wrong. A future reader hunting down the seed branch to check for shared references would spend a minute or two before realizing there's no call there.

Fix this →

Strengths

• Surgical patch. 9 lines, one file, two load-bearing comments/checks. No scope creep — the temptation to also lift guards to module scope (observation B) or tighten OAuth extras (observation C) was correctly resisted.
• Number.isFinite over isFinite. The global isFinite coerces strings (isFinite("42") → true), which would have undercut the whole tightening. Using Number.isFinite keeps the guard a clean discriminator.
• Comments explain the why, not the what. The mutation comment names the safety precondition; the isNonNegNumber comment names the UX motivation (no propagation to the form where Infinity is meaningless). Both follow the CLAUDE.md guidance on non-obvious invariants.
• Follow-up suggestion captured for a separate issue. The satisfies Record<keyof T, true> pattern note for AGENTS.md is the right move — a documentation PR is its own piece of work and doesn't belong in a fix commit.

---

· branch: feat/1358-flatten-mcp-json-settings

[cliffhall]

Good catch — the seed branch returns DEFAULT_SEED_CONFIG directly and never calls normalizeMcpServers, so the comment had it inverted. Fixed in 3d9f15b: the comment now reads "the GET handler's file-present branch — the seed branch returns DEFAULT_SEED_CONFIG without normalizing", which both names the right caller and pre-empts a future reader's same hunt.

Only a comment change, no test impact.