feat(cli): lift config-file headers/timeouts/OAuth into serverSettings (#1482)

[cliffhall]

Closes #1482.

Problem

When the CLI loaded a server from a --catalog/--config file it used the bare MCPServerConfig and dropped disk-level headers, timeouts, and OAuth — so CLI connections from a file silently ignored them. The TUI already lifted these into InspectorServerSettings via mcpConfigToServerEntries(); the CLI's resolveServerConfigs() path did not.

(This was the settings-lifting parity gap left open after #1499/#1347 normalized the --catalog/--config vocabulary.)

What changed

Core — core/mcp/node/servers.ts (new, shared by CLI + TUI)

• loadServerEntries() — resolves the catalog/config source (or ad-hoc target) into { config, settings }, lifting disk headers/timeouts/OAuth into InspectorServerSettings. Applies the default writable catalog, the --catalog/--config/ad-hoc conflict matrix, and seed-if-writable / error-if-read-only semantics. (This is the former loadTuiServers body, moved to core.)
• selectServerEntry() — single-server selection for the CLI (--server, or the only server, else an error listing names).
• headersToServerSettings() — moved here from the CLI/TUI duplicates.

CLI — clients/cli/src/cli.ts

• Resolves via loadServerEntries + selectServerEntry, so file settings now reach the connection.
• --header overrides the file's headers for that run while preserving the file's timeouts/OAuth.
• Drops the CLI-only conflict check and bare-config path (now shared).

TUI — clients/tui/src/tui-servers.ts

• Now a thin re-export of the shared core helpers (loadTuiServers → loadServerEntries). No behavior change; eliminates the duplication.

Tests

• core/.../servers.test.ts: settings lifting (headers/timeouts/OAuth), --header merge/override, stdio env/cwd overrides, catalog seed, read-only error, conflicts, ad-hoc, and selectServerEntry selection/error cases.
• CLI integration: a config-file header is sent on a tools/call over HTTP, and --header overrides the file's header.
• servers.ts coverage 97.8% lines / 100% functions.

Acceptance criteria (#1482)

• A CLI --catalog/--config invocation applies the file's headers, timeouts, and OAuth to the connection.
• CLI and TUI share one file→settings resolution path (no CLI-only path that drops settings).
• Integration test: a config file's custom header is applied on a CLI tools/call.

npm run validate passes end-to-end (web format/lint/build/coverage, CLI + TUI suites, launcher build, smoke:launcher/smoke:cli/smoke:tui/smoke:web).

🤖 Generated with Claude Code

[cliffhall]

@claude review

[claude]

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

---

Review: lift config-file settings into serverSettings (#1482)

• Read changed files & diff vs origin/v2/main
• Review core servers.ts (new shared helpers)
• Review CLI cli.ts changes
• Review TUI tui-servers.ts re-export
• Review tests & coverage
• Post review feedback

Overall this is a clean, well-scoped change. The duplicated CLI/TUI settings-lifting logic is consolidated into one shared core/mcp/node/servers.ts path, the CLI now actually applies disk headers/timeouts/OAuth, and the --header override semantics are sensible. The test coverage (unit + the HTTP integration test asserting the disk header is sent) is exactly what proves the fix. Nice work. A few notes below — none blocking.

---

1. Subtle fix worth calling out (good) — maxFetchRequests

The moved headersToServerSettings in core/mcp/node/servers.ts:46-55 now sets maxFetchRequests: DEFAULT_MAX_FETCH_REQUESTS, which the old CLI/TUI copies omitted even though InspectorServerSettings.maxFetchRequests is a required field (core/mcp/types.ts:445). So this consolidation also quietly corrects an incomplete object. 👍 — just flagging it so it's intentional, not accidental.

2. Behavioral change: --server + ad-hoc target now errors (minor)

Previously the CLI resolved via resolveLaunchServerConfigs(opts, "single"), where a positional/ad-hoc target with --server foo silently ignored --server and used the ad-hoc config. Now loadServerEntries returns { default: ... } for the ad-hoc branch (servers.ts:139-144), and selectServerEntry(entries, "foo") will throw:

Server 'foo' not found in config file. Available servers: default

Erroring on a contradictory combo (ad-hoc target and a named selector) is arguably more correct than silently ignoring it, but it's a behavior change that isn't mentioned in the PR description and the error text ("not found in config file") is a little misleading when there is no config file. Consider either a clearer message for that case or a note in the PR. Not blocking.

3. resolveLaunchServerConfigs is now dead production code

With both the CLI and TUI moved onto loadServerEntries, resolveLaunchServerConfigs (core/mcp/node/config.ts:423-428) is no longer referenced by any runner — only by config.test.ts. Its companion resolveServerConfigs is still used (by loadServerEntries multi-mode and getNamedServerConfigs), so this is just the thin launch wrapper. Fine to leave for now, but it's a candidate for removal in a follow-up so it doesn't linger as tested-but-unused. Fix this →

4. Duplicated stdio-override helper

applyStdioOverrides in servers.ts:58-71 is nearly identical to applyOverrides in config.ts:238-254. One small divergence: servers.ts trims cwd (overrides.cwd?.trim()) while config.ts does not (if (overrides.cwd) c.cwd = overrides.cwd). Not a bug for the file-source path, but two copies that can drift. Consider exporting one shared helper from config.ts and reusing it. Fix this →

5. Doc staleness (per AGENTS.md "keep docs up to date")

The CLI README update is good. But specification/v2_cli_tui_launcher.md and specification/v2_catalog_launch_config.md still describe the CLI resolving via resolveLaunchServerConfigs(..., "single") and loadTuiServers' body living in tui-servers.ts. After this move, the resolution path is loadServerEntries/selectServerEntry in core/mcp/node/servers.ts. Worth a small spec refresh so the design docs match (lines 91/104/233 and 119 in the launcher spec). Not blocking, but the repo guidelines explicitly call this out.

---

Things I checked that look correct

• Conflict matrix (--catalog+--config, --catalog/--config+ad-hoc) and seed-vs-error semantics are preserved via the shared serverSourceConflict / readServerListFile. ✅
• mergeSettings overrides only headers, preserving disk timeouts/OAuth — and the unit test at servers.test.ts:79-104 plus the integration override test prove it. ✅
• --header broadcast to all entries is harmless for the CLI (it selects exactly one via selectServerEntry) and documented as deliberate for the TUI. ✅
• selectServerEntry error messages match the previous loadServerFromConfig/resolveServerConfigs wording, so no UX regression on the multi-server / unknown-name paths. ✅
• The TUI re-export keeps loadTuiServers/headersToServerSettings/TuiServer names intact; existing tui-servers.test.ts still exercises them. ✅
  • branch v2/1482-cli-lift-config-settings

[cliffhall]

Thanks for the thorough review! Addressed all the actionable items in 58c32b8:

1. maxFetchRequests (informational) — Confirmed intentional. The consolidation deliberately corrects the previously-incomplete settings object; flagging it as such.

2. Misleading "not found in config file" error — Fixed. selectServerEntry's unknown-name error is now source-agnostic: Server 'foo' not found. Available servers: .... Added a doc comment explaining why (entries may be a single ad-hoc default, where "in config file" misleads). The "Multiple servers found in config file" message is kept verbatim since that path only occurs with a real file source.

3. Dead resolveLaunchServerConfigs — Removed. Both runners route through loadServerEntries now, so it was dead production code. Migrated its three default-catalog launch tests to compose the underlying primitives directly (resolveServerConfigs(withDefaultCatalogPath(opts), mode)), preserving identical coverage. Dropped the export from index.ts too.

4. Duplicated stdio-override helper — Deduped. Exported applyOverrides from config.ts (reconciling the divergence by adopting the cwd.trim() behavior) and reused it in servers.ts; removed the local applyStdioOverrides.

5. Spec staleness — Refreshed both specification/v2_cli_tui_launcher.md and specification/v2_catalog_launch_config.md: resolution path is now documented as loadServerEntries/selectServerEntry in core/mcp/node/servers.ts, the tracked gap G1 is marked resolved by #1482, and the already-stale withDefaultConfigPath references (renamed in #1499) are corrected to withDefaultCatalogPath.

npm run validate passes end-to-end (servers.ts 97.3% / config.ts 98.6% line coverage, CLI/TUI suites, all four smokes).

@claude review

[claude]

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

---

Re-review: changes in 58c32b8 (addressing prior review)

• Review error-message change in selectServerEntry
• Review applyOverrides dedup (cwd trim reconciliation)
• Review removal of resolveLaunchServerConfigs + migrated tests
• Review spec doc refresh
• Post review feedback

All five items from the previous review are addressed cleanly and correctly. Verified against origin/v2/main.

---

✅ 2 — Misleading error message

selectServerEntry now throws source-agnostic Server '${name}' not found. Available servers: … (servers.ts:147-149), and the doc comment (servers.ts:130-136) explains why ("config file" misleads when entries is a single ad-hoc default). The "Multiple servers found in config file" wording is correctly kept verbatim — that branch only fires for a real multi-entry file source, never for the ad-hoc { default } case. Good call.

✅ 3 — Dead resolveLaunchServerConfigs removed

Gone from config.ts and the index.ts export. Confirmed no remaining references in production code (grep shows only applyOverrides/resolveServerConfigs callers now). The three default-catalog launch tests were migrated to compose the primitives directly via a local resolveLaunch = (o, m) => resolveServerConfigs(withDefaultCatalogPath(o), m) helper (config.test.ts:251-254), preserving identical assertions (seed-on-first-run, empty-multi, single-entry). Coverage is equivalent — nice.

✅ 4 — applyStdioOverrides deduped

servers.ts now imports and reuses the exported applyOverrides from config.ts (servers.ts:5, :102); the local copy is gone. The divergence was reconciled by adopting the trim in the shared helper: if (overrides.cwd?.trim()) c.cwd = overrides.cwd.trim() (config.ts:250).

One thing worth flagging for the record (not a problem): this trim now applies to all applyOverrides call sites in config.ts (lines 363/387/405/443), not just the new file-source path — so ad-hoc and named-config resolution now also trim cwd. That's a harmless, arguably-correct behavior change (a whitespace-only --cwd is now ignored rather than set to whitespace), and it matches the trimming already done for catalogPath/configPath elsewhere. Just confirming it's intentional rather than an incidental side effect of the dedup.

✅ 5 — Spec refresh

Both specs are thoroughly updated: resolution path documented as loadServerEntries/selectServerEntry in core/mcp/node/servers.ts (launcher spec lines 91/104/119/231-234; catalog doc 323-335/526-530), G1 marked resolved by #1482 (catalog doc 95/321/462/473/494, changelog 510), and the stale withDefaultConfigPath references corrected to withDefaultCatalogPath. The TUI re-export and CLI single-config-via-loadServerEntries are reflected throughout.

✅ 1 — maxFetchRequests (informational)

Confirmed: the removed CLI-local headersToServerSettings omitted maxFetchRequests, while the core version (servers.ts:52) sets DEFAULT_MAX_FETCH_REQUESTS. The consolidation deliberately completes the previously-incomplete object. 👍

---

Verdict

LGTM. The refactor is tight, the migrated tests preserve coverage, and the docs now match the code. No new issues found. Nothing blocking.
• branch v2/1482-cli-lift-config-settings

[cliffhall]

Confirming the item-4 note: yes, the cwd.trim() now applying to all applyOverrides call sites is intentional. A whitespace-only --cwd being ignored rather than set to whitespace is the correct behavior, and it brings applyOverrides in line with the trimming already done for catalogPath/configPath. No incidental side effect.

Thanks for the careful re-review! 🙏