feat(cli,tui): skills catalog budget flags (--skill-catalog-max-skills/--skill-catalog-max-bytes)

[cliffhall]

Closes #2420

What

Web exposes the per-server skills catalog budget (skillCatalogMaxSkills / skillCatalogMaxBytes) in Server Settings, consumed by resolveSkillCatalogBudget (core/mcp/skills.ts). The CLI and TUI had no way to set it.

• New flags on CLI and TUI: --skill-catalog-max-skills <n> and --skill-catalog-max-bytes <n>.
• Validation: a shared core coerce, skillCatalogLimitParser(flag) in core/mcp/node/config.ts. It accepts only a run of decimal digits that denotes a positive safe integer, the same values isSkillCatalogLimit keeps when it reads mcp.json. 0, -1, 1.5, 1e3, 0x10 and non-numeric input are rejected before connecting, with an error that names the flag.
• Plumbing: the flags flow through ServerLoadOptions → mergeSettings in loadServerEntries, exactly as --protocol-era does. They override the file's values, or seed default settings for an ad-hoc target, and the rest of the file's settings are preserved. Like --header and --protocol-era, they apply to every loaded server. verifySkills already reads the budget through client.getServerSettings(), so no further wiring is needed.
• Where the budget has an effect: it bounds a verification run that covers several skills, which today means the CLI's --verify. The CLI therefore rejects the flags without --verify, the same pairing rule as --require-digests. The TUI Skills pane verifies one skill per gesture, so neither the TUI flags nor the existing mcp.json values change what the pane does today. The TUI README and --help say so, and making the pane verify the whole catalog is tracked separately in TUI Skills pane verifies one skill at a time, so the skills catalog budget never applies there #2590. The TUI flags are kept for parity with web's per-server setting and with the file field the TUI already accepts, and they apply once the pane verifies several skills.
• Server config file: the file entry was already honored. serverList.ts lifts skillCatalogMaxSkills / skillCatalogMaxBytes from mcp.json into the settings, and both clients pass those settings to InspectorClient. A new test asserts the file values reach the resolved settings and are overridden by the flags.
• Docs: clients/cli/README.md (options table), clients/tui/README.md and the flag table in docs/mcp-server-configuration.md.

The issue's "longer-term matrix test" is out of scope here, as agreed.

Tests

• config.test.ts: the parser accepts and rejects the listed values.
• servers.test.ts: the flags override file values and preserve other settings, one flag at a time keeps the other file value, and an ad-hoc target gets default settings.
• clients/cli/__tests__/programmatic-ergonomics.test.ts: an end-to-end --verify against the skills fixture. Without the flag no report is incomplete; with --skill-catalog-max-skills 1 later skills are reported incomplete, which proves the flag reaches the budget. Invalid values are rejected before connecting, and each flag is rejected without --verify.
• clients/tui/__tests__/tui-entry-flags.test.ts: runTui registers both flags and forwards them, parsed, to the server loader. The loader is mocked to capture its options, because tui.tsx is outside the TUI coverage include.

Per-file coverage of the changed core files is above 90 on all four dimensions (config.ts 99.3/100/98.7/97.8, servers.ts 97.8/100/97.8/97.2 lines/functions/statements/branches).

No screenshot: the TUI change adds flags only and nothing changes on screen.

Gate

npm run local:gate could not run to completion in this sandbox. The machine-wide gate lease was 15–20 gates deep, and two leased attempts timed out at the lease's 45-minute cap. These stages were therefore run directly (proxy env unset), outside the lease, and all are green except as noted:

• ✅ local:validate, verify:skills:cli, coverage:cli, coverage:tui, coverage:launcher, verify:build-gate, verify:bundle-externals, smoke:launcher, smoke:cli, smoke:tui
• ⚠️ coverage:web: 8796/8797 pass. The single failure is environmental and unrelated to this diff: secret-store-selection.test.ts > isOnMountPoint > is false when the path itself can't be resolved. In this sandbox the repo checkout itself (/Users/cliffhall/Projects/mcp-inspector-v2) is a mount point, so walking bad\0path up to the cwd correctly reports "mounted". Sibling PRs hit the same failure.
• Review-round follow-ups (error message, TUI docs, TUI entry test, the CLI --verify pairing) were re-checked with each client's validate: CLI 451 tests and TUI 444 tests green, plus verify:typecheck-coverage. Run with the proxy env unset, since the CLI token-revocation tests fail under the sandbox proxy.
• ⏭️ Left to CI because they bind fixed ports and need the lease: smoke:web, smoke:web:chromium, smoke:web:tabs, smoke:web:firefox, local:storybook. This PR touches no web code.

🤖 Generated with Claude Code

[Copilot]

Copilot review overview

🟢 Approval recommended

The implementation satisfies #2420 with appropriate plumbing and tests; only minor error-message precision remains.

Review effort: Balanced
Findings: 1

Open (1)

• Document decimal-digit and safe-integer input constraints · New

What changed in this PR

Adds CLI and TUI skills-catalog budget flags, closing #2420 and aligning both clients with Web settings.

Changes:

• Adds shared validation and settings plumbing.
• Exposes both flags in CLI and TUI.
• Adds documentation and unit/end-to-end coverage.

File	Description
docs/​mcp-server-configuration.md	Documents shared budget flags.
core/​mcp/​node/​servers.ts	Merges flag overrides into server settings.
core/​mcp/​node/​index.ts	Exports the shared parser.
core/​mcp/​node/​config.ts	Validates budget arguments.
clients/​web/​src/​test/​core/​mcp/​node/​servers.test.ts	Tests settings merging.
clients/​web/​src/​test/​core/​mcp/​node/​config.test.ts	Tests accepted and rejected values.
clients/​tui/​tui.tsx	Adds and forwards TUI flags.
clients/​tui/​README.md	Documents TUI usage.
clients/​cli/​src/​cli.ts	Adds and forwards CLI flags.
clients/​cli/​README.md	Documents CLI usage.
clients/​cli/​__tests__/​programmatic-ergonomics.test.ts	Verifies end-to-end CLI behavior.

---

💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.

[cliffhall]

Copilot round 1 (1 low-severity finding, "Approval recommended"):

• Budget-flag error message did not name the decimal-digit / safe-integer constraints: fixed in 58a58a9 (replied inline). No suppressed comments.

Requesting round 2.

[Copilot]

Copilot review overview

🔵 Needs a closer look

The TUI flags are currently ineffective because each verification starts a new budget run containing only one skill.

Review effort: Balanced
Findings: None

Resolved since last review (1)

• Document decimal-digit and safe-integer input constraints

Previously missed (2)

In code that hasn't changed since last review

TUI verification flags are ineffective for per-skill runs

clients/​tui/​tui.tsx:60

These options cannot affect the TUI as currently implemented. SkillsTab calls verifySkills(inspectorClient, [skill]) for one selected skill at a time (clients/tui/src/components/SkillsTab.tsx:190), while the catalog budget is checked before a skill and charged only after that skill finishes (core/mcp/skillsVerification.ts:352-353, 569-574). Because both accepted limits are positive, the sole skill is always read and there is no subsequent skill to truncate, so both new TUI flags are no-ops. Make TUI verification catalog-wide (or otherwise preserve a budget across per-skill runs), or do not expose these TUI flags until they can take effect.

Missing TUI entrypoint test for forwarding new flags

clients/​tui/​tui.tsx:121

Add a TUI entrypoint test that passes both new flags and verifies the parsed values reach the loaded server/client settings. The TUI coverage configuration includes only src/** (clients/tui/vitest.config.ts:34), and the existing tui-servers.test.ts calls the shared loader directly, so neither these Commander registrations nor the forwarding at lines 120-121 are exercised; the current green TUI coverage result cannot detect a misspelt option or dropped forwarding field.

[cliffhall]

Copilot round 2 ("Needs a closer look", 0 inline; 2 "previously missed" findings in the body, so they have no thread and are answered here):

• TUI verification flags are ineffective for per-skill runs (clients/tui/tui.tsx). Correct: SkillsTab calls verifySkills(client, [skill]) one skill per gesture, and the catalog budget is a run-level bound, so neither the new flags nor the existing mcp.json values change the pane today. Partly fixed, partly declined. Fixed in 93cf3ed: the TUI README and --help no longer claim the pane reads within the budget; they say what it actually bounds and point at the CLI --verify. Declined: making the pane verify the whole catalog is a new capability beyond Skills catalog budget setting has no CLI or TUI equivalent of the Web per-server setting #2420, so it is filed as TUI Skills pane verifies one skill at a time, so the skills catalog budget never applies there #2590 (Todo / Low, v2.10.0). The TUI flags stay, for parity with the web per-server setting and the file field the TUI already accepts, and they take effect without further wiring once the pane verifies several skills.
• Missing TUI entrypoint test for forwarding the new flags. Fixed in 783e3e9: clients/tui/__tests__/tui-entry-flags.test.ts mocks the loader to capture the options runTui builds, and asserts both flags arrive parsed (and are unset when absent). TUI validate passes (444 tests).

Requesting round 3.

[Copilot]

Copilot review overview

🔵 Needs a closer look

The TUI flags are currently ineffective, while the CLI silently accepts them where they have no effect.

Review effort: Balanced
Findings: None

Previously missed (2)

In code that hasn't changed since last review

Require --verify when using verification budget flags

clients/​cli/​src/​cli.ts:862

The CLI accepts this option for methods where it is guaranteed to do nothing, such as --method tools/list: these settings are consumed only by verifySkills, which runs only when --verify is set. Since the option is documented as a --verify budget, silently accepting it elsewhere can make automation appear bounded when it is not. Add the same early pairing validation used for --strict, --verify, and --require-digests, requiring --verify for both budget flags, with a regression test.

TUI skill budget flags have no effect

clients/​tui/​tui.tsx:54

These TUI flags have no observable effect today. SkillsTab always calls verifySkills(inspectorClient, [skill]), and because every accepted limit is positive and the budget is charged only after that skill, neither limit can stop a read. This therefore does not provide the TUI equivalent requested by #2420 and contradicts the PR description's claim that the Skills pane honors the flags. Implement a multi-skill verification path that consumes this budget, or defer the TUI flags until #2590 is resolved.

[cliffhall]

Copilot round 3 ("Needs a closer look", 0 inline; 2 "previously missed" findings in the body, answered here):

• Require --verify when using the budget flags (clients/cli/src/cli.ts). Fixed in f8b51d0: each flag without --verify is now rejected, the same rule as --require-digests, with a regression test and a README note. CLI validate: 451 tests green.
• TUI skill budget flags have no effect. This repeats round 2's finding, already answered: correct in substance, and the rest of the change (a multi-skill TUI verification path) is declined as beyond Skills catalog budget setting has no CLI or TUI equivalent of the Web per-server setting #2420 and tracked in TUI Skills pane verifies one skill at a time, so the skills catalog budget never applies there #2590. The one new point was right: the PR description still said the Skills pane honors the flags. The body is corrected; the TUI README and --help were corrected in 93cf3ed. The flags stay for parity with web's per-server setting and the file field the TUI already accepts.

Requesting round 4.

[Copilot]

Copilot review overview

🟡 Changes recommended

The CLI permits inert flags with skills/get, and several descriptions incorrectly present the byte threshold as a hard ceiling.

Review effort: Balanced
Findings: 1

Open (1)

• Reject inert budget flags for skills/get · New

Previously missed (4)

In code that hasn't changed since last review

Clarify README byte budget behavior

clients/​cli/​README.md:129

The README repeats the hard-ceiling claim, but the byte budget is charged only after a skill is fully read, so a run can exceed this value by the current skill before later skills are skipped. Document it as the cumulative threshold after which no additional skill is read.

Document byte budget as a post-read cumulative threshold

clients/​cli/​src/​cli.ts:868

“The most bytes one run reads” promises a hard ceiling, but verifySkills charges this budget after finishing each skill. For example, the existing 60-byte-budget test reads two roughly 50-byte skills before stopping later entries (skillsVerification.test.ts:845-886). Describe this as the cumulative threshold after which no additional skill is read, rather than a maximum byte count.

Describe byte setting as a cumulative threshold

clients/​tui/​tui.tsx:61

This also describes the byte setting as the most bytes a run reads, although the current verifier completes the skill that crosses the threshold and only skips subsequent skills. Reword it as a cumulative threshold so users do not rely on it as a hard byte ceiling.

Clarify TUI verification budget limitations

docs/​mcp-server-configuration.md:129

This shared table presents the flags as an effective TUI verification budget, while the PR establishes that the TUI only verifies one skill and the limits currently have no effect. The same page also still says at line 194 that skillCatalogMaxSkills applies to “the TUI Skills pane.” Add the TUI caveat here and correct that field description so the configuration reference does not contradict the TUI README and #2590.

[cliffhall]

Declined. --method skills/get --verify is a valid --verify run, and the budget there behaves exactly as the same mcp.json setting does: it is read and applied, it just cannot truncate a one-entry run. The --verify pairing exists to reject a flag on a run where no budget is ever read. Narrowing further, to skills/list only, is extra hardening beyond #2420, so I am leaving it out of this PR.

[cliffhall]

Copilot round 4 ("Changes recommended": 1 inline + 4 "previously missed" in the body). All declined; no code changed this round, so the review loop stops here (pr-flow 7c: a round holding only declined findings).

• Reject inert budget flags for skills/get (inline). Declined, with the reply in the thread: skills/get --verify reads and applies the budget like the file setting does; rejecting it there is hardening beyond Skills catalog budget setting has no CLI or TUI equivalent of the Web per-server setting #2420.
• Byte budget described as a hard ceiling (CLI README, CLI --help, TUI --help). Declined as wording polish. The flag text matches the existing skillCatalogMaxBytes row in docs/mcp-server-configuration.md ("the maximum number of bytes read across all skills"), which predates this PR. The charge-after-read semantics belong to verifySkills (Make the skills --verify catalog budget configurable #2294), and any doc correction should change that shared row and all its echoes at once, not only the new flags.
• Clarify TUI verification limits in docs/mcp-server-configuration.md. Declined as polish: the TUI README and --help already state the limitation (93cf3ed), and TUI Skills pane verifies one skill at a time, so the skills catalog budget never applies there #2590 tracks the docs row together with the behavior.

Copilot loop exit: stop, round held only declined findings.