Skip to content

docs: restore the missing docs/ targets and split the catalog-seeding wording - #1812

Merged
cliffhall merged 4 commits into
v2/mainfrom
docs/1811-readme-links
Jul 28, 2026
Merged

docs: restore the missing docs/ targets and split the catalog-seeding wording#1812
cliffhall merged 4 commits into
v2/mainfrom
docs/1811-readme-links

Conversation

@cliffhall

Copy link
Copy Markdown
Member

Closes #1811

Four README links pointed at files that don't exist on v2/main, and the launcher README described catalog seeding as one behavior when it is two.

Root cause

v1.5/main is not an ancestor of v2/main. The clients were merged across but the repo-root docs/ folder was not, so the READMEs arrived still referencing it. docs/ here held only mcp-app-review.md, added later and independently.

What's in the PR

Target Approach
docs/mcp-server-configuration.md Written fresh for v2, not restored
docs/launcher-config-consolidation-plan.md Restored from v1.5/main, path drift corrected
docs/images/tui-screenshot.png Regenerated against the current TUI
launcher README seeding wording Split into the two real behaviors

Why the config doc is new rather than restored. The v1.5 file documents only --config, predating the --catalog / --config split (writable-and-seeded vs. read-only-and-errors), MCP_CATALOG_PATH, and the mutual-exclusivity rules. Restoring it verbatim would have shipped a model that no longer exists. This was the most load-bearing of the four links: both the cli and tui READMEs delegate their entire "MCP server (which server to connect to)" section to it, so that flag surface had no in-repo documentation at all for two of the three clients.

The plan doc kept its substance — single process, no spawn(), shared processor in core/mcp/node/config.ts. Fixed launcher/clients/launcher/ and the stale per-client-binary reference, and appended a v2 note on the non-workspace layout and the catalog/config split.

The screenshot was regenerated rather than restored, since the v1.5 image shows an older UI. Captured from a real pty (Ink needs raw mode) replayed through a terminal emulator, so it keeps its colors, against a small three-server catalog rather than a wall of test servers.

The seeding fix

The launcher README said "created/seeded if missing" in both its --web and --cli/--tui tables. It's two behaviors:

  • Web seeds DEFAULT_SEED_CONFIG (core/mcp/serverList.ts) — filesystem-server-default + everything-server-default.
  • CLI / TUI seed an empty { "mcpServers": {} } (seedEmptyCatalog in core/mcp/node/config.ts).

Verified by running each surface against a throwaway HOME. Both tables now state which, with a pointer between them.

This wasn't hypothetical: the undifferentiated wording produced a wrong claim in the modelcontextprotocol.io Inspector docs (modelcontextprotocol/modelcontextprotocol#3143, tracked by #1803), caught in review. Corrected there too.

Verification

  • npm run validate passes.
  • No broken relative links remain across README.md, AGENTS.md, clients/*/README.md, and docs/*.md (checked by resolving every relative link target).
  • Root README now indexes all three docs/ guides instead of just the App-review one.

🤖 Generated with Claude Code

https://claude.ai/code/session_01JVu2ZxsKXHM7BL9vd1mzwK

Closes #1811

Four README links pointed at files that never came across when the
v1.5 clients were merged into v2/main. v1.5/main is not an ancestor of
v2/main, so the clients arrived still referencing a docs/ folder that
stayed behind.

- docs/mcp-server-configuration.md: written fresh for v2 rather than
  restored. The v1.5 file predates the --catalog / --config split,
  MCP_CATALOG_PATH, and the mutual-exclusivity rules, so restoring it
  would have documented a model that no longer exists. This is the page
  both the cli and tui READMEs delegate their whole server-options
  section to, so it was the most load-bearing of the four.
- docs/launcher-config-consolidation-plan.md: restored from v1.5/main.
  Its substance still holds; corrected the launcher/ -> clients/launcher/
  path drift and the per-client-binary reference, and appended a v2 note
  covering the non-workspace layout and the catalog/config split.
- docs/images/tui-screenshot.png: regenerated against the current TUI
  rather than restoring the v1.5 image, which showed an older UI.

The launcher README described a missing writable catalog as
"created/seeded if missing" in both its --web and --cli/--tui tables,
reading as one behavior when it is two: web seeds DEFAULT_SEED_CONFIG
(filesystem + everything), while cli/tui seed an empty mcpServers
object. Both tables now say which, with a pointer between them. That
ambiguity had already produced a wrong claim in the
modelcontextprotocol.io docs.

Verified no broken relative links remain across README.md, AGENTS.md,
clients/*/README.md, and docs/*.md.
@cliffhall cliffhall added the v2 Issues and PRs for v2 label Jul 27, 2026
@cliffhall

Copy link
Copy Markdown
Member Author

@claude review

@claude

claude Bot commented Jul 27, 2026

Copy link
Copy Markdown
Contributor

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


Review

  • Gather context (diff, base branch)
  • Verify the four link targets now resolve
  • Verify the seeding claims against the source
  • Review docs/mcp-server-configuration.md against the CLI/web/TUI code
  • Review docs/launcher-config-consolidation-plan.md for path drift
  • Post review

Good PR — the seeding split is the load-bearing fix and I verified it exactly against the source. One real correctness bug in the new config doc, plus two over-generalizations. Details below.


🔴 The -- separator section is backwards for the CLI

docs/mcp-server-configuration.md:67-75 says both web and cli "forward everything after it to the target command as its own arguments." That's true for web, and inverted for the CLI:

// clients/cli/src/cli.ts:536-546
const dashDashIndex = scriptArgs.indexOf("--");
if (dashDashIndex >= 0) {
  targetArgs = scriptArgs.slice(0, dashDashIndex);   // BEFORE  -- = the server target
  optionArgs = scriptArgs.slice(dashDashIndex + 1);  // AFTER   -- = the Inspector's own options
}

vs. web, which matches the doc (clients/web/server/run-web.ts:61-66 + :108target = [...args, ...postArgs]).

The existing CLI test names it outright: clients/cli/__tests__/methods.test.ts:48"treats args before a -- separator as the server target".

So the doc's example is actively wrong under --cli:

mcp-inspector --cli node build/index.js -- --config /etc/myserver.conf --verbose

Here --config /etc/myserver.conf is consumed as the Inspector's read-only-session flag — precisely the outcome the next line claims the separator prevents. (Under the default --web mode the same line behaves as documented, which makes the mistake easy to miss.) The section needs to split web and cli, since their -- semantics are opposite. Fix this →


🟡 MCP_CATALOG_PATH ad-hoc guard is CLI-only

docs/mcp-server-configuration.md:91 states it unqualified, directly under a table headed "The shared flags":

MCP_CATALOG_PATH is honored only when no ad-hoc target is given … so a shell that exports it can still run one-off ad-hoc invocations

That guard exists only in the CLI (clients/cli/src/cli.ts:829-836, const envCatalog = adHoc ? undefined : process.env.MCP_CATALOG_PATH). Web and TUI read it unconditionally:

  • clients/web/server/run-web.ts:123opts.catalog ?? process.env.MCP_CATALOG_PATH
  • clients/tui/tui.tsx:92options.catalog?.trim() || process.env.MCP_CATALOG_PATH

so with it exported, mcp-inspector --tui node build/index.js trips serverSourceConflict (--catalog cannot be combined with an ad-hoc server URL/command) and the web equivalent process.exit(1)s. The cli README (clients/cli/README.md:85) scopes the same sentence correctly — the new shared doc drops the scope. Fix this →

🟡 "All three clients reject the combination identically"

docs/mcp-server-configuration.md:12 credits serverSourceConflict for all three. Web doesn't call it — it has its own inline matrix at clients/web/server/run-web.ts:113-127, which additionally rejects --header alongside --catalog/--config. That divergence is deliberate and documented on the shared helper:

// core/mcp/node/config.ts:290-296
// It deliberately omits web's `--header` + `--catalog`/`--config` rejection:
// unlike web, the CLI/TUI merge `--header` into per-server settings …

Line 88's flag-table note ("On web, requires an ad-hoc HTTP/SSE server") already captures it, so line 12 contradicts the same document a few rows down. "identically" → "the same source-selection rules" plus a pointer to the --header divergence.


🔵 Minor

  • docs/mcp-server-configuration.md:139,144taskTtl and maxFetchRequests show a default, but both have concrete ones: DEFAULT_TASK_TTL_MS = 60000 and DEFAULT_MAX_FETCH_REQUESTS = 1000 (core/mcp/types.ts:522,531). This matters because line 131 says each field is "omitted on write when it equals its default" — with in the column, a reader can't tell what triggers omission for those two.
  • docs/mcp-server-configuration.md:167 — the per-client seed table is accurate, but all three surfaces share the same default path (getDefaultMcpConfigPath()~/.mcp-inspector/mcp.json, core/storage/store-io.ts:31-34). So a user who runs --cli first and then --web gets an empty list in the UI, and vice versa — seeding is once-per-file, not once-per-surface. One sentence under the table would close the obvious follow-up question.
  • docs/mcp-server-configuration.md:169 — the -- row says TUI "no". The TUI passes argv straight to Commander, which treats a bare -- as end-of-options and still forwards the remainder as operands into targetArgs — so the practical behavior is probably "works, via Commander's default" rather than "unsupported". I couldn't run it to confirm (deps aren't installed in this checkout); worth a quick check before shipping "no".

Verified as correct ✅

  • Seeding, the headline fix. DEFAULT_SEED_CONFIG (core/mcp/serverList.ts:711-724) is exactly the two entries quoted, /tmp scope included; seedEmptyCatalog writes exactly { "mcpServers": {} } (core/mcp/node/config.ts:90-100). The web seed is correctly gated on writable, so --config never seeds (core/mcp/remote/node/server.ts:1892-1896) — the launcher README's "same seed-if-missing behavior" for --catalog <path> holds.
  • All relative links resolve. Checked every ](./…)/](../…) target across README.md, docs/*.md, and all four client READMEs — zero broken. The screenshot resolves from clients/tui/README.md:5 and is a reasonable 180K.
  • Field reference is accurateprotocolEra default "legacy" (types.ts:550), modernLogLevel default "debug" (:568), autoRefreshOnListChanged/paginatedLists default false (serverList.ts:248-249), the oauth block's five keys (types.ts:161-169), and roots as { uri, name? } (:177) all match.
  • --server is web+cli only — the TUI's option list has no such flag, and web warns "no effect on the web UI yet" (run-web.ts:157-160), exactly as the table says.
  • The restored plan doc reads clean; clients/launcher/ and the "single mcp-inspector bin" correction are right, and the v2 notes section is a good call rather than silently rewriting a v1.5 document.

The -- bug is the one I'd hold the merge for — a copy-pasteable example that does the opposite of what the prose promises is worse than the missing link was.
· branch docs/1811-readme-links

… defaults

Review of #1812 caught a real inversion: the `--` separator section described
web's semantics as if they were shared with the CLI, but the two are opposite.
Web (and, via Commander's default end-of-options handling, the TUI) append
everything *after* `--` to the target command; the CLI treats everything
*before* `--` as the target and everything *after* as the Inspector's own
options. The doc's copy-pasteable example therefore did the opposite of what
its prose promised under `--cli`. Split into the two forms with a correct
example for each, and note that the CLI has no way to pass a leading-dash
argument to a stdio server on the command line.

Also:

- Scope the `MCP_CATALOG_PATH` ad-hoc guard to the CLI. Web and TUI read the
  env var unconditionally, so an ad-hoc invocation with it exported trips the
  catalog/ad-hoc conflict there.
- Reword "all three clients reject the combination identically": they share the
  source-selection rules, but web uses its own inline matrix and is stricter,
  additionally rejecting `--header` with `--catalog`/`--config`.
- Fill in the real defaults for `taskTtl` (60000) and `maxFetchRequests` (1000),
  which the surrounding text needs in order to explain omit-on-default writes.
- Note that seeding is once per *file*, not per client: all three surfaces
  default to the same path, so whichever runs first decides the contents.
- Correct the per-client `--` row, which claimed the TUI has no support.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Txmv2qqv3yeKgRzoqXytzD
@cliffhall

Copy link
Copy Markdown
Member Author

Thanks — the -- finding is a real bug and you were right to hold the merge for it. I verified all six against the source and took all six. Pushed in 177739e.

🔴 The -- separator — confirmed, and worse than described

Confirmed at clients/cli/src/cli.ts:536-546 (before -- = target, after = Inspector options) vs. clients/web/server/run-web.ts:61-66,108 (target = [...args, ...postArgs]). The example was actively harmful under --cli: --config /etc/myserver.conf gets consumed as the read-only-session flag and then rejected as a catalog/ad-hoc conflict — the exact outcome the next sentence claimed the separator prevents.

The section now splits into the two forms with a correct example for each. I also added the consequence you did not mention, which I think is the more useful half: because the CLI spends -- on its own options, and its no-separator path takes only leading non-dash tokens as the target (allowUnknownOption silently swallows the rest), there is currently no way to pass a leading-dash argument to a stdio server on the --cli command line at all. The doc now says so and points at the catalog entry’s args as the workaround.

🔵 TUI -- — you were right to flag it, and it does work

You could not run it; I did. Reproduced the TUI’s exact Commander setup (clients/tui/tui.tsx:20-73 — plain [target...] variadic, no explicit split):

args= ["node","build/index.js","--config","/etc/x.conf","--verbose"]
opts= {}

So --config is not consumed and the remainder is appended to the target — behaviorally identical to web, via Commander’s default end-of-options handling. The per-client row now reads yes — after -- → target (Commander default) rather than no, with the mechanism noted so nobody "fixes" it later by adding an explicit split.

🟡 MCP_CATALOG_PATH — confirmed

The guard is CLI-only (cli.ts:829-836); run-web.ts:123 and tui.tsx:92 read it unconditionally. Scoped the sentence to the CLI and stated the web/TUI failure mode with the actual error string.

🟡 "identically" — confirmed

Web has its own inline matrix (run-web.ts:132-147) and is stricter, additionally rejecting --header with --catalog/--config — the divergence the shared helper documents on itself at core/mcp/node/config.ts:290-296. Reworded to "the same source-selection rules" with the --header divergence called out inline, so line 12 no longer contradicts the flag table further down.

🔵 Defaults and the shared path — both taken

  • taskTtl60000, maxFetchRequests1000, each naming its constant. You are right that was worse than merely incomplete: the paragraph directly above promises omit-on-write-when-equal-to-default, which is unreadable without the value.
  • Added the once-per-file sentence. All three surfaces share getDefaultMcpConfigPath(), so --cli first then --web yields an empty UI list — the obvious follow-up question, now answered in place.

npm run validate passes; the change is markdown-only.

@cliffhall

Copy link
Copy Markdown
Member Author

@claude review

@claude

claude Bot commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

Claude finished @cliffhall's task in 4m 54s —— View job


Review

  • Gather context (diff since the last review, base branch)
  • Re-verify the six items fixed in 177739e1
  • Fresh pass over the full docs/mcp-server-configuration.md
  • Fresh pass over the plan doc + README changes
  • Post review

All six prior findings land correctly — I re-verified each against source rather than taking the commit message for it, including installing commander@13.1 in a scratch dir to reproduce the TUI -- claim independently. Nothing blocking left. Four smaller accuracy items below, one of which is a copy-pasteable example that errors.


🟡 The two --cli examples under "Ad-hoc servers" don't run

docs/mcp-server-configuration.md:59-65:

mcp-inspector --cli node build/index.js
mcp-inspector --cli --server-url https://api.example.com/mcp --transport http

Both fail before connecting — --method is required on every non-short-circuit CLI path (clients/cli/src/cli.ts:816-820, "Method is required. Use --method to specify the method to invoke."; only --list-stored-auth / --print-handoff bypass it). The -- example at :82 gets this right (-- --method tools/list), which makes the omission here look deliberate rather than a slip.

Either append --method tools/list to both, or drop --cli and show them as bare mcp-inspector … (web), where they do run as written. Given the section's job is "here's how you name a server ad-hoc", the second is arguably cleaner — the ad-hoc grammar is what's being demonstrated, not the CLI. Fix this →

🟡 --server on web is a complete no-op, not "yes"

Two rows overstate it:

  • :93 — "--server <name> | Select one named server from the file | Web and CLI only."
  • :178 — per-client row: web "yes (currently warns — the UI lists every server)"

On web the flag never does anything, on any path:

  • catalog / config branches — warns and is discarded (clients/web/server/run-web.ts:159-163, 168-172).
  • ad-hoc branchserverName: opts.server is passed into resolveServerConfigs(…, "single"), but that parameter is only read when a file source exists (core/mcp/node/config.ts:355, 368); the ad-hoc branch has none by construction (the conflict matrix already rejected catalog/config + ad-hoc), so it falls to buildConfigFromOptions(options) and the flag is dropped. The map key comes from deriveSeedServerId(config) (run-web.ts:47-56, 227), not from --server.

So web behaves exactly like the TUI here — every server in the file is listed — and "Web and CLI only" invites the reader to conclude the opposite. --server selects only under --cli; web accepts-and-warns. Worth saying that plainly in the Notes cell, since the per-client table is two screens further down. Fix this →


🔵 Minor

  • :179 / :178 — TUI --server is a hard error, not a silent ignore. The row reads "no — all servers are listed", which sounds like the flag is tolerated. The TUI defines no --server and sets no allowUnknownOption (clients/tui/tui.tsx:20-73), so mcp-inspector --tui --server foo exits with error: unknown option '--server'. One clause ("rejected as an unknown option") saves a confused reader.
  • :71-77 — the "Web and TUI behave the same" pairing hides an asymmetry worth one sentence. They agree on where post--- tokens go, but not on whether you need --. Web sets allowUnknownOption() + allowExcessArguments(), so an unrecognized dash flag already falls through into the target — I confirmed against commander@13: node build/index.js --verbose --flag=1args = ["node","build/index.js","--verbose","--flag=1"], opts = {}. The TUI has neither, so the same line is a parse error. Net: on web -- is needed only for flags the Inspector defines (which is what the prose says); on the TUI it's needed for any dash flag. That's the practical rule a TUI user hits first.
  • clients/cli/README.md:97 and clients/tui/README.md:32 still say --catalog is "seeded if missing" — the exact undifferentiated phrasing this PR exists to kill, on the two surfaces that seed empty. They're outside the diff and they do link to the new doc, so it's not wrong, just unfinished: seeded empty if missing is a one-word fix on each and closes the wording out repo-wide.

Verified as correct ✅

The six fixes, re-checked at source:

  1. -- splitclients/cli/src/cli.ts:534-546 (before → targetArgs, after → optionArgs) vs. clients/web/server/run-web.ts:59-66, 106 (target = [...args, ...postArgs]). The two forms at :71-85 now match reality, and the launcher forwards -- untouched (parseLauncherArgv only strips a contiguous mode-flag prefix), so both examples work end-to-end through the mcp-inspector bin.
  2. The "no leading-dash arg to a stdio server under --cli" consequence is real and is the more useful half, as you say: -- is spent on Inspector options, and the no-separator path takes only leading non-dash tokens (while (!scriptArgs[i].startsWith("-"))), so there is no third route. Pointing at the catalog entry's args is the right workaround.
  3. TUI -- — independently reproduced. Rebuilt the TUI's exact commander setup under commander@13.1.0: ["node","build/index.js","--","--config","/etc/x.conf","--verbose"]args = ["node","build/index.js","--config","/etc/x.conf","--verbose"], opts = {}. --config is not consumed; the remainder is appended to the target. yes — after \--` → target (Commander default)` is right, and noting the mechanism is the right call.
  4. MCP_CATALOG_PATH — CLI-only guard at cli.ts:829-836; run-web.ts:123 and tui.tsx:92 read it unconditionally. The quoted error string matches serverSourceConflict verbatim.
  5. "same source-selection rules" — web's inline matrix (run-web.ts:132-147) is a superset, adding the two --header rejections; core/mcp/node/config.ts:290-296 documents that omission on itself. Line 12 no longer contradicts the --header note at :98.
  6. DefaultsDEFAULT_TASK_TTL_MS = 60000 (types.ts:522), DEFAULT_MAX_FETCH_REQUESTS = 1000 (:531, 0 = unlimited per :692). Also spot-checked the rest of the field table: DEFAULT_PROTOCOL_ERA = "legacy" (:550), DEFAULT_MODERN_LOG_LEVEL = "debug" (:568), the oauth five-key block (:161-169), roots: Root[] as { uri, name? } (:177).

Plus:

  • The once-per-file paragraph (:53) is exactly right and well-placed. All three surfaces resolve getDefaultMcpConfigPath()~/.mcp-inspector/mcp.json (core/storage/store-io.ts:30-34); web seeds DEFAULT_SEED_CONFIG inside the write lock only when the file is still absent (core/mcp/remote/node/server.ts:1906-1913) and returns as-is otherwise, and the read-only branch (:1893-1897) never seeds. The --cli-then---web empty-list outcome follows.
  • Seeding splitDEFAULT_SEED_CONFIG (core/mcp/serverList.ts:711) is the two quoted entries; EMPTY_CATALOG_CONTENT / seedEmptyCatalog (core/mcp/node/config.ts:89-99) writes exactly { "mcpServers": {} }, gated on writable at :105-112. The launcher README's "same seed-if-missing behavior" for an explicit --catalog holds — the web seed branch keys off writable, not off the path being the default.
  • Links — resolved every relative target across README.md, AGENTS.md, all four client READMEs, and docs/*.md: zero broken.
  • Screenshot — current tab set (Info · Auth · Resources · Prompts · Tools · Protocol · Network), colors intact, three-server catalog. Console is legitimately absent, not truncated: Tabs.tsx:57-59 filters it on showLogging. 178K is fine.
  • Plan docclients/launcher/ and the single-bin correction are right, and the appended "v2 notes" section (non-workspace layout + the catalog/config split) is the honest way to age a v1.5 document rather than silently rewriting it.
    · branch docs/1811-readme-links

…g repo-wide

Second review pass on #1812. All markdown.

- The two ad-hoc examples used `--cli` but omitted `--method`, so neither ran.
  Dropped `--cli` (the section demonstrates the ad-hoc grammar, not the CLI)
  and documented the CLI's two extra rules separately. Verified against the
  built launcher: `--method` is required, and the target must come *first* —
  `--cli --method tools/list node build/index.js` silently discards the target
  and falls back to the user's catalog rather than erroring, which is the more
  dangerous half and is now called out with a ✅/❌ pair.
- `--server` was described as "Web and CLI only". It selects only under `--cli`:
  web warns and discards it in the catalog/config branches, and in the ad-hoc
  branch `serverName` is only read when a file source exists, so it is dropped
  there too. The TUI does not define the flag at all and rejects it as an
  unknown option. Both the flag table and the per-client table now say so.
- Noted the web/TUI `--` asymmetry: they agree on where post-`--` tokens go,
  but web's `allowUnknownOption()` + `allowExcessArguments()` mean a separator
  is only needed for flags the Inspector defines, whereas the TUI has neither
  and needs `--` for any dash argument meant for the server.
- Fixed the undifferentiated "seeded if missing" wording still left in the cli
  and tui READMEs — the exact phrasing this PR exists to remove, on the two
  surfaces that seed empty.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Txmv2qqv3yeKgRzoqXytzD
@cliffhall

Copy link
Copy Markdown
Member Author

Took all four. Pushed in ae1e4c4. npm run validate passes; still markdown-only.

🟡 The ad-hoc examples — right, and the CLI failure mode is worse than "requires --method"

Confirmed at clients/cli/src/cli.ts:816-820. I took your second suggestion and dropped --cli from both — the section is demonstrating the ad-hoc grammar, so the CLI's method requirement was incidental noise there.

But writing the CLI note sent me to check the obvious next form, and it does not work either. Run against the built launcher:

$ mcp-inspector --cli --method tools/list node test-servers/build/test-server-stdio.js
{"error":{"code":"auth_required","message":"Multiple servers found in config file. Please specify one with --server. Available servers: NightRiderHttp, everything-server-http, …"}}

The target must come first. The no-separator path takes only the leading run of non-dash tokens (cli.ts:543-548), so a target placed after any flag is not merely ignored — it is silently discarded and the Inspector falls back to your catalog. It exits 0 and connects to a server, just not the one you named. The target-first form is fine:

$ mcp-inspector --cli node test-servers/build/test-server-stdio.js --method tools/list
{"tools":[{"name":"echo", …

So the section now carries both rules with a ✅/❌ pair, and notes that --tui has no ordering constraint. A silent wrong-server connection seemed worth more than a footnote.

🟡 --server on web — confirmed a full no-op

Traced both paths you named. Catalog and config branches warn and discard (run-web.ts:159-163, 168-172); the ad-hoc branch passes serverName (:189) but resolveServerConfigs only reads it when a file source exists (core/mcp/node/config.ts:355), which the conflict matrix has already guaranteed it does not. "Web and CLI only" did invite exactly the wrong inference. Both rows now say selection happens only under --cli, with web's accept-and-warn stated in the Notes cell rather than two screens away.

🔵 TUI --server — folded into the same rows

clients/tui/tui.tsx:20-73 defines no --server and sets no allowUnknownOption, so it is error: unknown option '--server'. The per-client cell says that verbatim now; "no — all servers are listed" did read as tolerated.

🔵 The web/TUI -- asymmetry — good catch, added

This is the better framing and I had flattened it. Web sets allowUnknownOption() + allowExcessArguments() (run-web.ts:72-73), the TUI sets neither. So the practical rule is: on web -- is needed only for flags the Inspector defines; on the TUI it is needed for any dash flag. Added as its own paragraph after the shared "where post--- tokens go" point, since a TUI user hits the difference first.

🔵 cli/tui READMEs — fixed, and one more instance

Fair — outside the diff but squarely the thing this PR exists to remove. Both now read seeded **empty** if missing. Grepping for it also turned up clients/tui/README.md:22, a code-comment # writable catalog (seeded if missing) you had not flagged; fixed too. clients/web/README.md and the launcher tables were already differentiated, so the wording is now consistent repo-wide.

@cliffhall

Copy link
Copy Markdown
Member Author

@claude review

@claude

claude Bot commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

Claude finished @cliffhall's task in 4m 37s —— View job


Review

  • Gather context (diff since the last review, base branch)
  • Re-verify the four fixes in ae1e4c4d against source
  • Independently reproduce the commander claims (web + TUI, commander@13)
  • Fresh pass over the full doc set + link check
  • Post review

All four prior findings land. I re-derived the argv claims rather than trusting the commit message — installed commander@13 in a scratch dir and replayed both parsers' exact option sets. Nothing blocking. Three precision items left, all minor.


🔵 A second conflict-matrix divergence, and on this axis web is looser

docs/mcp-server-configuration.md:12 now says all three apply "the same source-selection rules" with web "deliberately stricter on one axis" (--header). There's a second axis, and it runs the other way — --transport stdio:

  • CLI/TUI: hasAdHocServerOptions counts any --transport (core/mcp/node/config.ts:257-263Boolean(options.transport)), so hasAdHoc is true.
  • Web: run-web.ts:127-130(!!opts.transport && opts.transport !== "stdio"), so --transport stdio is not ad-hoc.

Net: mcp-inspector --cli --catalog c.json --transport stdio --method tools/list is rejected with --catalog cannot be combined with an ad-hoc server URL/command, while the web equivalent is accepted and the flag quietly ignored. The flag table's --transport note ("Ad-hoc targets only", :108) reads as universal but is only enforced on two of three surfaces. Degenerate combination, but the doc goes out of its way to enumerate the divergences — this is the one it misses. Fix this →

🔵 On web, --server warns on two of three paths — the ad-hoc path is silent

:107 ("Web accepts it, warns, and ignores it") and :192 ("accepted but a no-op — warns, lists every server") both promise a warning. The console.warn exists only in the catalog and config branches (run-web.ts:158, :168). The ad-hoc branch passes serverName: opts.server into resolveServerConfigs (:189) where it's unreachable — no file source, so config.ts:355 never reads it — and prints nothing. So mcp-inspector --server foo node build/index.js drops the flag with zero feedback, which is the case where a reader is most likely to have expected it to do something. "warns (except with an ad-hoc target, where it's dropped silently)" covers it.

🔵 :67 — the --method requirement isn't an argv-splitting consequence

"Under --cli two extra rules apply, both consequences of how it splits argv". The target-first rule is; --method is a plain validation several hundred lines later (clients/cli/src/cli.ts:816-820), unrelated to the split. Small, but the sentence invites the reader to look for a connection that isn't there.


Verified as correct ✅

Re-derived, not taken on trust:

  • The ad-hoc examples now run. :61,64 dropped --cli, so both are default-web invocations. Reproduced web's parser (allowExcessArguments() + allowUnknownOption(), run-web.ts:72-73) under commander@13: ["node","build/index.js"]args verbatim, opts empty. ✓
  • The CLI target-first rule and its silent-fallback consequence. cli.ts:534-548: with no --, while (!scriptArgs[i].startsWith("-")) takes only the leading non-dash run; targetArgs is the only source of target (:843) — program.args is never read anywhere in cli.ts, so the form really does discard the target rather than recovering it through the [target...] variadic. Falls through to withDefaultCatalogPath → the default catalog. The ✅/❌ pair is the right way to show this, and calling out that it exits 0 against the wrong server is the part that would actually bite someone.
  • --tui has no ordering rule. Rebuilt the TUI's exact option set (tui.tsx:20-73) under commander@13: --cwd /srv node build/index.js and node build/index.js --cwd /srv both → args = ["node","build/index.js"], opts = {cwd:"/srv"}. ✓
  • The web/TUI -- asymmetry (:91) — the sharpest addition in this round. Confirmed both halves, including the ordering hazard I went looking for and didn't find: commander preserves position for unknown options rather than appending them, so node srv.js --dev --verbose x.txtargs = ["node","srv.js","--verbose","x.txt"] with --dev consumed. The target's argument order survives, which is what makes "on web -- is only needed for a flag the Inspector does define" safe advice. The TUI half: node build/index.js --verboseerror: unknown option '--verbose'; with --["node","build/index.js","--config","/etc/m.conf","--verbose"], opts empty. ✓
  • --server per-surface. CLI genuinely selects — selectServerEntry (core/mcp/node/servers.ts:150-172) looks the name up and throws Server '<name>' not found otherwise. TUI: no such option and no allowUnknownOption, reproduced error: unknown option '--server'. ✓
  • serverSourceConflict really is shared by cli+tui — both route through loadServerEntries (core/mcp/node/servers.ts:89-101), the only caller. Web's inline matrix (run-web.ts:132-147) is the parallel implementation. ✓
  • Launcher passes -- through. parseLauncherArgv (clients/launcher/src/parse-launcher-argv.ts:20-25) consumes only a contiguous mode-flag prefix, so every mcp-inspector … example in the doc reaches the client's parser intact — which is what makes the two opposite -- behaviors observable through the one bin.
  • Seeding wording is now consistent repo-wide. Grepped every .md: the only remaining bare "seed-if-missing" phrasings are docs/mcp-server-configuration.md:197 and clients/launcher/README.md:58, both in the CRUD paragraph where the contrast is with error-if-missing, not with web — correct in context. clients/tui/README.md:22, clients/cli/README.md:97, and clients/tui/README.md:32 now all say empty.
  • Links. Re-resolved every relative target across README.md, AGENTS.md, docs/*.md, and all four client READMEs — 9 files, zero broken.
  • Plan doc. Still accurate against the tree: clients/launcher/src/index.tsbuild/index.js is the bin, it dynamic-imports and calls in-process (no spawn), and runWeb/runCli/runTui all exist with the documented shape. The "v2 notes" append is the honest way to age a v1.5 doc.

The remaining three are all one-clause edits. None of them is a copy-pasteable command that misbehaves, so I would not hold the merge for any of them.
· branch docs/1811-readme-links

…r warning

Third review pass on #1812. All markdown, all one-clause precision edits.

- Added the second conflict-matrix divergence. The doc enumerated only the
  `--header` axis, where web is stricter; there is a second where web is
  looser: cli/tui count any `--transport` as an ad-hoc marker
  (`hasAdHocServerOptions`), while web exempts `--transport stdio`. So
  `--catalog c.json --transport stdio` errors on cli/tui and is silently
  accepted on web. Noted alongside the `--header` divergence and on the
  `--transport` row, whose "Ad-hoc targets only" read as universal.
- Scoped the web `--server` warning. It fires only in the catalog and config
  branches; with an ad-hoc target the flag is dropped with no output at all,
  which is where a reader is most likely to have expected it to work.
- Dropped the "both consequences of how it splits argv" framing: the
  target-first rule is, but `--method` is a plain validation unrelated to the
  split, and the claim invited the reader to look for a link that isn't there.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Txmv2qqv3yeKgRzoqXytzD
@cliffhall

Copy link
Copy Markdown
Member Author

Took all three. Pushed in 2803866. npm run validate passes; markdown-only. This closes the review out on my side — three rounds, and the remaining findings were one-clause precision edits rather than defects.

🔵 The --transport stdio divergence — good catch

Confirmed: hasAdHocServerOptions is a bare Boolean(options.transport) (core/mcp/node/config.ts:257-263), while web reads (!!opts.transport && opts.transport !== "stdio") (run-web.ts:127-130). You are right that this was the one the doc had gone out of its way to enumerate and then missed — and it runs the opposite direction from --header, which makes "web is stricter on one axis" actively misleading rather than just incomplete.

Rewrote the paragraph as two bullets, one per axis, each labelled with which way it leans, and annotated the --transport row so "Ad-hoc targets only" no longer reads as universally enforced.

🔵 The web --server warning is two-of-three paths

Confirmed — console.warn exists only at run-web.ts:158 and :168, and the ad-hoc branch prints nothing. Agreed on why it matters: an ad-hoc invocation is exactly where someone would expect --server to pick something, and it is the one path with no feedback at all. Both rows now distinguish warns-with-a-file-source from silent-with-an-ad-hoc-one.

🔵 --method is not an argv-splitting consequence

Right, and it was my sentence to begin with — I bundled the two rules under one causal claim for symmetry that does not exist. --method is a plain validation at cli.ts:816-820. Dropped the framing; the two rules now just stand as two rules.


Thanks for the three passes. Worth recording what they actually caught, since none of it was in the original scope of "four dead links":

  • Round 1: the -- separator section documented web semantics as if the CLI shared them, when the two are inverted — a copy-pasteable example that did the opposite of its prose.
  • Round 2: the ad-hoc examples did not run, and chasing the fix surfaced that a CLI target placed after a flag is silently discarded and falls back to the user's catalog — exits 0 against the wrong server.
  • Round 3: a second conflict-matrix divergence pointing the opposite way from the one documented.

Each round the verification was re-derived from source rather than taken from the commit message, including independently reproducing the commander behavior — that is what made the pushed claims worth trusting. Ready to merge.

@cliffhall
cliffhall merged commit 4d30d1c into v2/main Jul 28, 2026
3 checks passed
@cliffhall
cliffhall deleted the docs/1811-readme-links branch July 28, 2026 00:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

v2 Issues and PRs for v2

Projects

None yet

Development

Successfully merging this pull request may close these issues.

docs: fix four broken README links and the ambiguous catalog-seeding wording

1 participant