cli: programmatic ergonomics — --method initialize, --tool-args-json, --connect-timeout, env-var fixes

[cliffhall]

Closes #1573

Programmatic-ergonomics flags and env-var fixes for the CLI, re-implemented informed by PR #1510 (33fac3f) and adapted to the v2/main OAuth-wired cli.ts.

What changed

• --method initialize — connect-only probe; prints {serverInfo, protocolVersion, capabilities, instructions} from the cached InitializeResult.
• --tool-args-json '{"k":"v"}' — pass tool args verbatim as JSON (no string→number coercion). Mutually exclusive with --tool-arg; rejects combined use, malformed JSON, and non-object values.
• --connect-timeout <ms> — default 15000 for ad-hoc --server-url/target invocations (so a black-holed host fails fast), file-level timeout for catalog/config runs; 0 disables. Backed by the exported withConnectTimeout() helper.
• MCP_CATALOG_PATH is now ignored when an ad-hoc target is given (adHoc gate), so a shell exporting it can still run one-off ad-hoc invocations without the catalog/ad-hoc conflict.
• MCP_STORAGE_DIR is honored by core getStateFilePath (<MCP_STORAGE_DIR>/oauth.json), sitting below the per-file MCP_INSPECTOR_OAUTH_STATE_PATH override.

Tests & docs

• New clients/cli/__tests__/programmatic-ergonomics.test.ts — in-process runCli() coverage for each flag plus direct unit tests for withConnectTimeout branches.
• Added getStateFilePath resolution cases to the web storage integration suite (covers the new MCP_STORAGE_DIR branch).
• clients/cli/README.md documents the new flags and the env-var semantics.
• Per-file coverage gate stays ≥90 on all four dimensions; full npm run ci passes locally.

Stacking

First of the Wave 3 CLI stack (#1573 → #1574 → #1575). Branches off v2/main.

🤖 Generated with Claude Code

https://claude.ai/code/session_01BrihGWcrM9JGRyu41nzZYw

[cliffhall]

@claude review

[claude]

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

---

Review — PR #1651: CLI programmatic ergonomics

I read the full diff against origin/v2/main and traced the touched paths into core/. This is a clean, well-tested PR. The flag logic is sound, error paths are covered, and the tests exercise every branch (including the withConnectTimeout unit cases and env cleanup). A few notes below — one worth addressing before merge, the rest optional.

---

🟡 1. MCP_STORAGE_DIR is only honored by the OAuth backend, but the code comment claims it's app-wide

In core/auth/node/storage-node.ts the new doc comment says:

<MCP_STORAGE_DIR>/oauth.json — the storage-directory override honoured across the app, so setting it once points every persist backend at the same directory

That's not what the code does. The new branch reads MCP_STORAGE_DIR only inside getStateFilePath(). The other persist backends don't:

• core/client/config.ts:25 → getStoreFilePath(getDefaultStorageDir(), "client")
• getDefaultStorageDir() (core/storage/store-io.ts:22) resolves purely from HOME/USERPROFILE and never reads MCP_STORAGE_DIR.
• getDefaultMcpConfigPath() likewise ignores it.

So MCP_STORAGE_DIR moves oauth.json but leaves client.json and mcp.json in ~/.mcp-inspector — the opposite of "every persist backend at the same directory." The README wording is fine (it scopes the var to the OAuth backend), but the source comment overclaims.

Two ways to reconcile:

• Narrow the comment to match reality (OAuth backend only), or
• Make it genuinely app-wide by having getDefaultStorageDir() honor MCP_STORAGE_DIR (return process.env.MCP_STORAGE_DIR ?? path.join(homeDir, ".mcp-inspector", "storage")), then drop the special-case branch in storage-node.ts — the default path would pick it up. That better matches the var's name and the PR's stated intent.

Fix this →

---

🟢 2. Ad-hoc runs now pass a synthesized settings object instead of undefined (verified safe)

withConnectTimeout(undefined, DEFAULT_CONNECT_TIMEOUT_MS) builds a full InspectorServerSettings with defaulted fields for ad-hoc targets, where the code previously passed undefined. I checked this is behaviorally equivalent: requestTimeout: 0, taskTtl/maxFetchRequests at their DEFAULT_* constants, and connectionTimeout: 0 is treated as "no timeout" downstream (inspectorClient.ts:852-876). The headers: [] in the synthesized object can't clobber a --header flag, because passing --header makes selected.settings defined, which takes the if (settings) spread branch instead. No issue — noting it so a future reader knows the change was intentional.

---

🟢 3. --connect-timeout 0 correctly disables the timeout

options.connectTimeout ?? (adHoc ? DEFAULT_CONNECT_TIMEOUT_MS : undefined) uses ??, so an explicit 0 is preserved (not replaced by the default), and 0 maps to the untimed else branch at inspectorClient.ts:874. The --tool-args-json mutual-exclusion guard (Object.keys(toolArg).length > 0) also correctly tolerates the default empty {} from --tool-arg. Both good.

---

🟢 4. (Optional) --config + exported MCP_CATALOG_PATH still conflicts

The adHoc gate suppresses MCP_CATALOG_PATH for positional/--server-url/--transport targets, but a user with MCP_CATALOG_PATH exported who runs --cli --config foo.json still trips the catalog+config serverSourceConflict. This is pre-existing behavior (not a regression from this PR) and out of scope, but if the goal is "a shell exporting the var can still run one-off invocations," extending the gate to also ignore the env catalog when --config is explicit would round it out.

---

Verdict

Approve-worthy pending a decision on #1 (comment vs. behavior mismatch — the only thing I'd want resolved before merge). Everything else is optional polish. Nice work on the test coverage and the thorough inline rationale comments.
· branch v2/1573-cli-programmatic-ergonomics

[cliffhall]

Thanks for the thorough review. Response to each point:

🟡 1. MCP_STORAGE_DIR comment overclaims — applied (via option A, narrowed the comment). You're right that the branch only relocates oauth.json; client.json/mcp.json still resolve from getDefaultStorageDir(). Fixed in adf41d3 by narrowing the comment to state the var is scoped to the OAuth backend here.

I deliberately did not take option B (move the resolution into getDefaultStorageDir() and drop the branch). DEFAULT_STATE_PATH is a module-load-time constant (getStoreFilePath(getDefaultStorageDir(), "oauth")), so if the default resolved MCP_STORAGE_DIR, a runtime-set value would never be picked up — and that's exactly how the CLI stored-auth tests and the --wait-for-auth flow (PR #1655) set it. Reading the env var at call time inside getStateFilePath() is what makes that work. Making MCP_STORAGE_DIR genuinely app-wide (client.json + mcp.json too) is a broader change beyond #1573's scope and would need its own issue; the comment now reflects the actual OAuth-only scope, matching the README.

🟢 2 / 🟢 3. Agreed on both — the synthesized-settings equivalence (headers: [] can't clobber --header because that makes selected.settings defined and takes the spread branch) and --connect-timeout 0 preservation via ?? are intentional. Thanks for verifying downstream.

🟢 4. --config + exported MCP_CATALOG_PATH still conflicts — declined (out of scope). As you noted, this is pre-existing and not a regression from this PR. #1573's stated goal was specifically the ad-hoc (--server-url/target/--transport) case. Extending the adHoc gate to also suppress the env catalog when --config is explicit is reasonable but belongs in its own change; I'd rather not widen this PR's surface.

npm run ci is green after the comment fix.

[cliffhall]

@claude review

[claude]

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

---

Review — PR #1651: CLI programmatic ergonomics (re-review)

I re-read the diff against origin/v2/main after commit adf41d3a and traced the touched paths into core/. The one blocking item from my previous review (#1, the MCP_STORAGE_DIR comment overclaim) is resolved. No new issues found — this is ready to merge.

---

✅ Point #1 resolved (comment now matches behavior)

core/auth/node/storage-node.ts:22-32 now correctly scopes the doc comment to the OAuth backend:

this branch scopes MCP_STORAGE_DIR to the OAuth backend only; it does NOT relocate client.json / mcp.json …

It also documents the deliberate design choice not to take option B — resolving the env var at call time inside getStateFilePath() rather than in getDefaultStorageDir(), because DEFAULT_STATE_PATH is a module-load-time constant, so a runtime-set MCP_STORAGE_DIR (used by the stored-auth tests and the --wait-for-auth flow) would otherwise be missed. That rationale is sound and matches the README wording. Good call keeping the app-wide expansion out of #1573's scope.

Verified getStoreFilePath(process.env.MCP_STORAGE_DIR, "oauth") → <MCP_STORAGE_DIR>/oauth.json (store-io.ts:40-42), consistent with the comment, README, and the new integration test.

---

Spot checks on the rest (all confirmed clean)

• --method initialize — the four getters used (getServerInfo, getProtocolVersion, getCapabilities, getInstructions) all exist on InspectorClient (inspectorClient.ts:1541-1572). Any undefined field is simply omitted by JSON.stringify — fine for a probe.
• --tool-args-json mutual-exclusion guard — if (toolArg && Object.keys(toolArg).length > 0) correctly tolerates the default empty {} from --tool-arg while rejecting a real --tool-arg x=1 (covered by test). Null / array / malformed-JSON rejection all have tests.
• --connect-timeout — options.connectTimeout ?? (adHoc ? DEFAULT_CONNECT_TIMEOUT_MS : undefined) preserves an explicit 0 (no-timeout) via ??; the validator rejects negatives and NaN. Tests cover 5000 / 0 / -1 / non-numeric.
• adHoc gate — MCP_CATALOG_PATH is suppressed for positional / --server-url / --transport targets, with a test asserting a bogus env catalog is ignored on an ad-hoc run.
• Tests & coverage — the new programmatic-ergonomics.test.ts exercises every flag branch plus direct withConnectTimeout unit cases; the storage integration suite adds all four getStateFilePath resolution cases with proper env save/restore.

---

🟢 (Optional, non-blocking) minor edge — unchanged from before

Number("") and Number(" ") both evaluate to 0, so --connect-timeout "" would silently mean "no timeout" rather than erroring. Harmless in practice (nobody passes an empty timeout deliberately) and not worth a code change; noting only for completeness.

And point #4 from the prior round (--config + exported MCP_CATALOG_PATH still conflicts) remains reasonably deferred — pre-existing, out of #1573's ad-hoc scope.

---

Verdict

✅ Approve-worthy. The blocking comment/behavior mismatch is fixed, the flag logic is sound, and test coverage is thorough. Nice work.
· branch v2/1573-cli-programmatic-ergonomics

[cliffhall]

Thanks — glad the fix landed cleanly. Leaving the Number("") → 0 edge as-is per your note (an empty --connect-timeout value isn't a real usage, and treating it as no-timeout is harmless). No further changes on this PR.