modelcontextprotocol/inspector
Watch
1
0
Fork
You've already forked inspector
 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions 2
2,393 commits 1 branch 87 tags 30 MiB
Commit graph

2,393 commits

This branch
This branch
All branches
Author SHA1 Message Date
Cliff Hall
0742f78357
 		docs: replace the markdown bug template with GitHub issue forms (#1894) 
* docs: replace the markdown bug template with issue forms (#1844)

Issues are the only intake channel now that external pull requests are off
(#1820), so the chooser is where the contribution policy has to be stated —
GitHub gives a would-be contributor no explanation when the "New pull request"
button isn't there.

The one existing template was a legacy **markdown** template, whose fields are
prose headings a reporter can delete or ignore. Most reports still arrive
missing the client and the transport, which are the two facts that decide where
to look. Issue forms enforce them: this replaces `bug_report.md` with
`1-bug_report.yml`, where client (web/cli/tui/core), version line (v1/v2),
Inspector version, Node version, transport, MCP server, repro, expected, and
actual are all `validations.required`. It auto-labels `bug`, and its intro says
maintainers implement the fix, so a prompt plus screenshots beats a diff —
with a field for exactly that.

Adds `2-feature_request.yml` (auto-labels `enhancement`), which puts the
problem statement first and marks the solution optional, since the problem is
what survives when a specific solution turns out not to fit.

Neither form applies a `v1`/`v2` label: GitHub can't map a form answer to a
label, so the version line is a required dropdown and a maintainer still labels
at triage. v1 takes security fixes only, so routing that answer early is the
point.

`config.yml` disables blank issues and carries the contact links. **There is
deliberately no security *template*** — a template still opens a public issue,
which is precisely what a vulnerability report must not do. The redirect is a
contact link straight to the private advisory form (`/security/advisories/new`,
verified enabled on this repo) plus one to `SECURITY.md`, which covers the v1
line too. The remaining links deflect the recurring misfiled classes: the
specification repo and the TypeScript SDK for reports that aren't about this
tool, the docs site, the contribution policy, and `#inspector-dev`.

Also fixes three links the #1884 rename left dangling — `SECURITY.md` and the
PR template still pointed at `CONTRIBUTORS.md`, which no longer exists on this
branch. A policy nobody can open is the same as no policy.

Validated against GitHub's issue-forms schema (unique ids, allowed `type`s,
`markdown` blocks carrying no `id`/`validations`, `checkboxes` marking
`required` per option rather than under `validations`).

Claude-Session: https://claude.ai/code/session_01YAt8rqxysNbhYWLhoRm3fU

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>

* docs: address Copilot review on the issue forms (#1844)

- PR template said `v2` targets `main`; the v2 base branch is `v2/main`.
  Correct on its face, and the one line in that template a reader acts on.
- Scoped the Node floor on the bug form to v2, since the form serves both
  lines and v1's floor is older — reading `>= 22.19.0` as universal would
  make a legitimate v1 report look out of support.
- Both the CONTRIBUTING.md paragraph and the AGENTS.md section now say the
  chooser is served from the **default branch**, so neither claims a form
  added on `v2/main` is live before the milestone merge.

* docs: re-wrap the CONTRIBUTING chooser paragraph (#1844)

Round-2 Copilot nit: the previous wrap left a dangling "The" at end of
line, splitting a phrase mid-sentence in the raw markdown.

Claude-Session: https://claude.ai/code/session_01YAt8rqxysNbhYWLhoRm3fU

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>

* docs: clarify the protocol-era prompt on the bug form (#1844)

Round-3 Copilot nit. "the protocol era (legacy / modern) if you selected
one" reads as referring to a field on this form, which has none — the era is
selected on the *connection*, in the Inspector's server settings. Reworded to
"the protocol era you connected with (auto / legacy / modern)", which also
picks up `auto`, the default the previous list omitted.

Claude-Session: https://claude.ai/code/session_01YAt8rqxysNbhYWLhoRm3fU

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>

* docs: stop describing the feature form as sharing the bug form's fields (#1844)

Round-4 Copilot nits, and it was right three times over — I had described
"client, version line, transport" as the required set for *both* forms in
CONTRIBUTING.md, AGENTS.md, and the config.yml header. The feature form has
neither a version-line dropdown nor a transport field, and cannot have the
first: v1 takes security fixes only, so a feature request is v2 by
construction.

Each of the three now distinguishes the two forms rather than generalizing
from the bug form.

Claude-Session: https://claude.ai/code/session_01YAt8rqxysNbhYWLhoRm3fU

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>

* feat: declare v2 statically on the feature form (#1844)

Round-5 Copilot nit caught a real contradiction: the AGENTS.md paragraph said
no form can apply a version label, then asserted the feature form "is always
labeled v2". Both halves can't be true.

Resolved in the direction that makes the claim true rather than softer. A
form's `labels:` is static, which is only an obstacle when the value depends
on a reporter's answer — and for the feature form it doesn't: v1 takes security
fixes only, so a feature request is v2 by construction. So it now declares
`["enhancement", "v2"]` outright, which also satisfies AGENTS.md's
label-at-create-time rule without a triage step.

The bug form still can't, and shouldn't: its version line is genuinely the
reporter's to answer.

Claude-Session: https://claude.ai/code/session_01YAt8rqxysNbhYWLhoRm3fU

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
 2026-08-01 19:50:49 -04:00
Cliff Hall
812a115424
 		docs: add Incoming to both boards, adopt the v2 Priority field, and document the triage rubric (#1892) 
* docs: add Incoming board status, adopt Priority field, document triage rubric (#1891)

Board #28 had no state for "filed but not yet triaged" — Todo means
maintainer-approved, so parking unreviewed issues there falsely asserted
sign-off. Adds an Incoming status ahead of Todo.

The board's Priority single-select existed but was unused (0 of 264 cards)
and undocumented, with P0/P1/P2 options matching no vocabulary in the repo.
Re-optioned to Urgent/High/Medium/Low and documented alongside Status.

Completes the priority rubric: fills in the score bands (12+/9-11/6-8/<=5),
expands both axes into scored tables, and fixes the truncated signal-indicator
line. Severity alone tops out at High by design.

Also drops the "V2 Go Live" status reference — that option is not in the
field's option list.

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

* docs: correct the Done option id and harden the deleted-column guidance (#1891)

The Done Status option was deleted from board #28, blanking the Status of
all 247 cards that held it. Restored from a pre-change item-list snapshot;
verified all 247 are Done again and no other card was disturbed.

A recreated option never regains its old id, so Done is now 259d6aab rather
than 248a3910 — updated in the option table and the merge step.

The hazard callout recommended the web UI as the safe path, which is what
made this look safe. It is not: deleting an option blanks its cards in the
UI exactly as it does via the API, with no undo. Corrects that, adds a
snapshot-first rule, and documents the recovery recipe used here — including
the grouping check that proves the orphaned set is exactly the deleted
option's cards before re-applying.

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

* docs: add Incoming to the v1 board and document its recipes (#1891)

Board #11 had the same gap as #28 — no state for "filed but not yet
triaged" — so a v1 security-fix issue had nowhere to sit that didn't
assert maintainer approval. Adds Incoming there too.

AGENTS.md carried no v1 board recipes at all, only a link, so anyone
boarding a v1 issue had to discover the project and field ids by hand
and was liable to reach for #28's (which are rejected, but only after
the attempt). Adds a V1 board section with its ids, states that Incoming
is the default status for new items on both boards, and notes that
Priority is v2-only — #11 has no such field.

The Incoming option was added with all four existing option ids echoed
back, and a before/after item-list diff confirms all 74 cards kept their
Status. Three cards on #11 carry no Status; that predates this change.

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

* docs: distinguish the board Priority from the org-level issue field (#1891)

An issue page shows two fields named "Priority" and nothing syncs them.
Ours is the project field on board #28. The other is a GitHub issue field
(IFSS_kgDOAdAWeg) defined at the modelcontextprotocol org and shared by
every repo in it, alongside Effort/Start date/Target date. Same name, same
four option names, unrelated storage — #1891 sat at Urgent in Fields and
High on the board simultaneously.

Adds a table naming both, states that no pass-through exists in either
direction, and says to leave the org-level one alone: it is not repo-scoped,
so deleting it would strip Priority from every other org repo.

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

* docs: mark the org Priority field untrusted and cap it at a +1 signal (#1891)

Both boards are private (public: false, verified), so maintainer-assigned
Status and Priority are invisible to reporters — board priority is a
working queue, not a published commitment.

The org-level Fields -> Priority is the opposite: public on the issue page
and not part of triage. A value there is a reporter's preference, not an
assessment, so it is untrusted input. It now feeds the rubric as a sixth
signal bonus — flat +1 for Urgent or High, identical for both — and never
maps to a band. Urgent needs 12, so nothing a reporter types reaches it
alone; the issue must already sit at 11 on maintainer-assessed axes.

Supersedes the earlier "it is noise" framing: the signal is worth keeping,
it just cannot be allowed to decide the outcome. Also corrects the band
arithmetic (2-15 -> 2-16) now that a sixth bonus exists, and notes that
scores recorded on 2026-08-01 cite the old /15 denominator.

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

* docs: drop the /15 denominator caveat now the recorded scores are restated (#1891)

The 51 triage comments were rewritten from /15 to /16, so the note saying
they cite the old denominator no longer describes anything. No total moved:
the reporter-set signal only scores for Urgent or High, and the one scored
issue carrying an org-field value (#1826) had Low, which earns nothing.

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

* docs: describe the board-option hazard per field, not Status-only (#1891)

Copilot review on #1892 flagged three places that still read as
Status-only now that the section covers Priority too:

- the version-label bullet said a v2 issue needs "a board card with a
  Status"; it needs a Status and a Priority (v1 needs only a Status,
  since board #11 has no Priority field)
- the updateProjectV2Field hazard said a bad edit "orphans the Status
  of every card"; it orphans whichever field was edited
- the recovery recipe is genuinely Status-specific (reads .status,
  writes the Status field id), so say so and give the two
  substitutions for a Priority deletion

Verified `gh project item-list --format json` exposes both .status and
.priority before documenting the .priority swap.

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

---------

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
 2026-08-01 19:50:06 -04:00
Cliff Hall
8801d0f8bc
 		fix(scripts): await the pack:verify web child's exit before removing its work dir (#1895) 
* fix(scripts): await the pack:verify web child's exit before removing its work dir

`pack-and-verify.mjs` had the same kill-then-remove shape that made
`smoke:tui` flake with ENOTEMPTY in #1801: `verifyWeb()`'s
`finally { stop() }` only *delivered* SIGTERM, then the main flow
`rmSync`'d the work dir immediately, and `fail()` did the same back to
back.

Applies the #1814 treatment, factored into a shared
`scripts/lib/child-cleanup.mjs` so the two scripts can't drift:

- `stopChild()` — SIGTERM, await the child's `exit`/`close`, escalate to
  SIGKILL after a grace period, and ultimately proceed with a warning so
  a wedged child can never hang the script. `verifyWeb()`'s `stop()` now
  goes through it and is awaited, so the child is gone before the caller
  removes `work`.
- `removeSafe()` — `rmSync` that warns instead of throwing. Used on both
  success-path removals (a leftover temp dir must never fail a run that
  passed) and in `fail()`, where an ENOTEMPTY could otherwise bury the
  real diagnostic under an rmSync stack and skip the "tarball retained
  at ..." hint. `fail()` stays synchronous — it is called from sync
  contexts and ends in `process.exit()` — so it best-effort signals the
  child; `removeSafe()` is what makes that safe.

`smoke-tui.mjs` adopts `removeSafe()` for its `cleanup()`. Its `done()`
keeps its bespoke wait (documented inline): that one must keep waiting
for `close` *after* `exit` on a shorter re-armed deadline so the quoted
output is complete, which `stopChild()`'s first-of-exit/close contract
deliberately does not do.

Adds `scripts/lib/child-cleanup.test.mjs` (10 cases, `node --test` via
`npm run test:scripts`) covering the SIGTERM/SIGKILL escalation, the
`close`-only spawn-failure path, the never-hang give-up, late events
after giving up, and that `removeSafe` warns rather than throws.

`npm run ci` passes, and `npm run pack:verify` (not in the gate — needs
network) was run end to end and passes with no cleanup warnings.

Claude-Session: https://claude.ai/code/session_01YAt8rqxysNbhYWLhoRm3fU

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>

* fix(scripts): normalize stopChild's graceMs and correct removeSafe's JSDoc

Copilot review round 1 on #1895.

- `stopChild()` trusted `graceMs` as given, but call sites build it as
  `Number(process.env.X ?? 5000)` — a typo yields `NaN`, which
  `setTimeout` treats as `0`. That would fire SIGTERM and SIGKILL in the
  same tick and print "did not exit within NaNms", collapsing the whole
  escalation silently. New exported `normalizeGraceMs()` falls back to
  `DEFAULT_EXIT_GRACE_MS` for anything non-finite or non-positive, with
  two tests: the table of bad values, and an end-to-end one asserting a
  `NaN` grace period still leaves only SIGTERM sent a turn later.
- `removeSafe()`'s `@returns` said "true if the path was removed", which
  is inaccurate for a missing path (`force: true` makes it a no-op
  success). Reworded to say what it actually reports.

`npm run ci` passes.

Claude-Session: https://claude.ai/code/session_01YAt8rqxysNbhYWLhoRm3fU

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
 2026-08-01 19:50:00 -04:00
Cliff Hall
694d013f26
 		chore(deps): bring main's rc-series bumps to v2/main (hono 2.x, vite 8.1.5) (#1881) 
The 2.0.0-rc series shipped these dependency bumps to `main` only, so
`v2/main` was developing against older versions and would have met them
for the first time at the milestone merge.

- `@hono/node-server` ^1.19.14 -> ^2.0.12 (root, clients/web) — a major.
  v2.0.0's only breaking changes are dropping Node 18 (the repo already
  requires >=22.19.0) and removing the `@hono/node-server/vercel` adapter
  (unused here). The used surface — `serve()`, `serveStatic`, `ServerType`
  — is unchanged, so no source edits were needed.
- `vite` ^8.0.0 -> ^8.1.5 (root, clients/web) and ^8.0.16 -> ^8.1.5
  (clients/tui) — the security bumps from #1841.

`npm install` at the root (postinstall cascades into every client)
regenerated the three touched lockfiles. `npm run ci` passes.


Claude-Session: https://claude.ai/code/session_01YAt8rqxysNbhYWLhoRm3fU

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
 2026-07-31 23:03:34 -04:00
Cliff Hall
13e981e9dd
 		chore: restore repo files dropped by the v2 tree swap (#1871) 
* chore: restore repo files dropped by the v2 tree swap

A sweep of the pre-swap tree (ac3c1a12) against v2/main turned up 14 files the
tree swap dropped and that were never restored to either branch. Most are
correctly gone (v1 CI workflows, v1 sample-config, husky hook, two 0-byte
files, v1-specific prettier config). These four are worth having:

- .github/dependabot.yml — rewritten, not restored. v1's covered only
  github-actions at /; this covers github-actions plus the six npm manifests
  v2 actually has (root + four clients), monthly rather than weekly, grouped
  to one PR per ecosystem/directory, targeting v2/main, and labeled `v2` so
  the version-label rule holds for bot PRs too.
- .github/ISSUE_TEMPLATE/bug_report.md — rewritten. v1's asked for an
  "Inspector Version e.g. 0.16.5" and had no client or version field. Under
  the issues-only policy this template is the primary intake, so it now asks
  the routing questions first (v1 vs v2, which client) and points at the
  prompt-not-a-diff path.
- .gitattributes — restored byte-for-byte (package-lock.json
  linguist-generated=true). More useful in v2 than v1: there are now six
  lockfiles, and they dominate dependency-bump diffs.
- CODE_OF_CONDUCT.md — restored byte-for-byte, so GitHub surfaces it as a
  community standard. Left unformatted deliberately: it is verbatim upstream
  Contributor Covenant text, v1 listed it in .prettierignore for that reason,
  and no v2 format glob reaches root markdown.

CONTRIBUTING.md is deliberately not restored here — it is handled by renaming
CONTRIBUTORS.md to that name, which also lights up GitHub's contributing
banner without leaving two files to keep in sync.

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

* docs(dependabot): state the documented target-branch/security-update rule

Copilot review: the comment read as if `target-branch` had no bearing on
security updates. Per GitHub's Dependabot options reference, an entry whose
`target-branch` names a non-default branch is not applied to security
updates at all — so the schedule/labels/groups here shape version updates
only, and security PRs continue to be raised against the default branch.

Comment-only; the configured settings are unchanged.

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

* docs: correct the branch description and the labeling ask

Two suppressed Copilot comments, both correct:

- dependabot.yml called `main` "release-only". It isn't — README.md and
  AGENTS.md both describe it as the legacy v1 implementation taking security
  and bug fixes only. Reworded to match, keeping the point that matters
  (Dependabot reads config from the default branch).
- bug_report.md asked reporters to label the issue `v2`/`v1`, which most
  external reporters have no permission to do. It now asks only for the
  version checkbox and says a maintainer labels it at triage.

Comment/prose only; no configured Dependabot setting changed.

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

* docs(dependabot): describe main as release-only, not "legacy v1"

Reverts the branch description introduced in c7fbf350. That change was made to
match README.md:17 and AGENTS.md — but those are precisely the stale sentences
PR #1866 exists to correct. `main` has not been the legacy v1 implementation
since the go-live tree swap; it is the release branch holding the latest
released v2, and the deprecated v1 line lives on `v1/main`.

So the comment now describes the real model: v2/main is the develop branch,
main is release-only and receives milestone merges, v1/main is maintenance and
takes security fixes only.

The rest of c7fbf350 stands — the bug_report.md change (asking reporters for
the version checkbox rather than to apply a label they lack permission to set)
was a genuine catch.

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

---------

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
 2026-07-31 23:03:24 -04:00
Cliff Hall
ee707db52d
 		ci: restore the Claude Code workflow and .mcp.json, lost in the v2 tree swap (#1869) 
* ci: restore the Claude Code workflow, lost in the v2 tree swap

Closes #1850.

Restored byte-for-byte from ac3c1a12 (sha256
1c44f2d9d51c936adb40c9acefd4b44628fe7810360e2f8196034347f9d2602d,
3592 bytes). No content changes.

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

* ci: restore .mcp.json, which claude.yml's --mcp-config points at

The restored workflow runs with `--mcp-config .mcp.json` and
`--allowedTools "Bash,mcp__mcp-docs"`, so without this file the mcp-docs
server never registers and those tools do not resolve.

Lost in the same tree swap: .mcp.json survives only on v1/main, and is
absent from both main and v2/main. Restored byte-for-byte from 47f92841
(sha256 b0f0b8ea0d6c6e4bd9d19270db8c4ed13a951c6a7d8fa9a7b76d0d12ddd2491f,
123 bytes). No content changes.

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

---------

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
 2026-07-31 23:03:14 -04:00
Cliff Hall
ff99547309
 		docs: restore SECURITY.md on v2/main, so the next tree swap can't drop it (#1867) 
Closes #1864.

SECURITY.md was restored to main in #1843 but never added to v2/main.
Because v2/main is the develop branch merged into main at milestones, and
the two share no common git ancestor, the next milestone merge would
silently remove the security policy from the default branch a second time.

Copied byte-for-byte from main (sha256
bbc8aaa5a33d481f902c1b7ac260a4c9c0d41dac5bb2916607222dd205dda924,
2633 bytes). No content changes.


Claude-Session: https://claude.ai/code/session_01YAt8rqxysNbhYWLhoRm3fU

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
 2026-07-31 23:03:05 -04:00
Cliff Hall
7cc3c1276d
 		docs: unpin the branch-pinned CONTRIBUTORS.md links in the PR template (#1879) 
* docs: say "security fixes only" for v1 and unpin the PR-template links

The v1 maintenance policy was stated two different ways across the docs —
"security fixes only" in CONTRIBUTORS.md, "bug fixes" in README.md. The
#1819 PR triage told 120 contributors that v1 is security-fixes-only, so
that is the wording we keep.

Also make the PR template's two CONTRIBUTORS.md links relative instead of
pinned to blob/main/. `main` is release-only, so those resolved against
the last released tree rather than the branch the reader is on.

The remaining AGENTS.md occurrences are left alone deliberately: PR #1866
rewrites those exact lines and lands first.

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

* docs: drop the README change; #1866 owns that line

The "Repo status" callout edit here patched `(bug fixes only)` →
`(security fixes only)` on the callout as it exists on v2/main. But #1866
replaces that entire callout, because the surrounding sentence is false today:
it still says "The `main` branch is the legacy v1 implementation", when `main`
now holds the latest released v2.

So this edit both collides with #1866 on the same line and, taken on its own,
would preserve the false statement while correcting only the parenthetical.
#1866 already sets the v1 policy wording to "security fixes only" across
AGENTS.md and README (ee020ff2), so nothing is lost by dropping it here.

This PR is now exactly its intended half: the two branch-pinned
`blob/main/CONTRIBUTORS.md` links in .github/pull_request_template.md, made
relative. That file is untouched by #1866, so the two PRs no longer overlap on
any line.

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

---------

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
 2026-07-31 23:02:55 -04:00
Cliff Hall
379a8a0a3a
 		docs: rename CONTRIBUTORS.md to CONTRIBUTING.md so GitHub surfaces the policy (#1884) 
* docs: rename CONTRIBUTORS.md to CONTRIBUTING.md (#1883)

GitHub's contributing-guidelines banner — shown when someone clicks
"New issue" or opens a pull request — keys on the exact filename
CONTRIBUTING.md. Naming the file CONTRIBUTORS.md left that prompt dark
precisely where the "open an issue, not a PR" policy needs to land.

Renamed with `git mv` so history follows the file; contents are
unchanged (its heading already reads "Contributing to MCP Inspector").
Updated the one in-repo reference, in AGENTS.md. The two links in
.github/pull_request_template.md are deliberately untouched — PR #1879
makes them relative so they follow the rename.

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

* docs: drop the AGENTS.md link edit; #1866 owns that line

This PR edited the `See [CONTRIBUTORS.md](./CONTRIBUTORS.md)` link in
AGENTS.md, which is the exact line #1866 rewrites — guaranteeing a one-line
conflict when this merges after it.

Handing that line to its owner instead: #1866 now writes the link as
`CONTRIBUTING.md` directly, so this PR is a pure `git mv` and the two touch no
common line. No conflict to resolve at merge time.

The tradeoff is a short window between the two merges where AGENTS.md links to
a filename that doesn't exist yet. That's deliberate and bounded: #1866 is
first in the merge order and this is third.

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

---------

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
 2026-07-31 23:02:09 -04:00
Cliff Hall
2c24b82e8f
 		docs: converge README Test servers section with main's restructure (#1880) 
* docs: converge README Test servers section with main's restructure

Transplants main's restructured "Test servers" section (#1836) onto
v2/main — the "Serving the modern protocol era" and "Showcase configs"
headings, the config table, and the per-config subsections — replacing
the eight long paragraphs v2/main still carried.

Folds in v2/main's #1846 correction: main's copy still claims
Mcp-Param-* mirroring is skipped in the browser, which stopped being
true when the Inspector took over building the mirrored headers itself.
The blockquote callout keeps main's shape with the corrected wording.

Scoped to that section only — the whole file is deliberately NOT copied
from main, so #1866's repo-status callout and release-section edits (and
every other v2/main-only line) are untouched.

Shrinks the first v2/main -> main milestone merge from a whole-section
conflict to a single 3-line hunk.

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

* docs: point the #1846 reference at the issue, not a nonexistent PR URL

#1846 is the issue; the fix shipped as PR #1847. The callout's link used
/pull/1846, which 404s. Per Copilot review.

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

---------

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
 2026-07-31 23:01:19 -04:00
Cliff Hall
3ac34e4092
 		docs: post-swap branch model, PR policy, milestone rule, and Copilot review instructions (#1866) 
* docs: correct post-swap branch model, PR policy, and stale gate/tree entries

Part of the go-live phase-8 docs hygiene: everything that silently inverted
when `main` became v2.

Branch/release model — documented canonically in AGENTS.md and corrected
everywhere it was restated:
- AGENTS.md: replace the one-line "Base Branches" bullet with a table giving
  each branch its role (v2/main = develop, main = release, v1/main =
  maintenance), whether PRs target it, and which npm dist-tag it publishes.
- README.md: the "Repo status" callout had it backwards, claiming `main` was
  the legacy v1 implementation. Also note that a v2 release is cut from `main`
  after the milestone merge, not from v2/main.
- CONTRIBUTORS.md: the version table listed v2's base branch as `main`.

PR policy:
- Every PR must reference an issue, from anyone, no exceptions.
- PRs are opened by repo maintainers only — org write access is not
  authorization. Everyone else files a detailed issue.
- Branch names start with the target version segment (v2/…, v1/…).
- Issues and PRs carry exactly one of `v1`/`v2` at creation; default `v2`.
- Screenshots proving UI/TUI changes go in the gitignored pr-screenshots/.

Corrections to stale content found while in there:
- The TUI coverage bullet still described the Ink/App.tsx/hooks exclusion as
  interim pending #1501, which closed 2026-06-29; the gate now covers all of
  src/** with only tui-servers.ts excluded.
- The completion checklist said `npm run validate` was the gate and covered
  "e2e tests", contradicting the mandatory-pre-push-gate section (npm run ci).
- v1 PR base was given as `main`; it is `v1/main`.
- Project tree omitted core/client, docs/, and scripts/ (README: core/client,
  docs/). core/client is under the coverage gate and imported by both App.tsx
  trees.

Also adds the launcher and `npm run test:scripts` to the "run the tests" list
and a tiers overview, since neither was mentioned.

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

* docs: require a milestone on every new issue, defaulting to the current one

Adds the rule alongside the version-label rule, since they fail the same way:
an unmilestoned issue drops out of release planning as silently as an
unlabeled one drops out of version filtering.

- Set it at create time (`gh issue create --milestone <title> ...`).
- If the user didn't specify one, default to the current milestone — the open
  milestone with the nearest due date — rather than leaving it blank pending a
  decision. Includes the `gh api .../milestones` one-liner that identifies it.
- Milestones are release buckets, so pick by when the work ships, not by size;
  sub-issues normally inherit their parent's milestone.

Also widens the section's opening callout: an issue is not "created" until it
is labeled, milestoned, AND boarded with a Status — four distinct steps, not
three.

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

* docs: add .github/copilot-instructions.md and require mirroring into it

GitHub Copilot reads .github/copilot-instructions.md when reviewing a PR. The
repo had no such file, so Copilot reviewed without any of the conventions that
actually govern this codebase — the ban on `any` and on unjustified double
casts, the Mantine-first styling rules, the `.withProps()` extraction rule, the
lib/utils split, test placement, and the per-file >=90% four-dimension gate.

The new file is a review-focused distillation of AGENTS.md: the rules a
reviewer would cite against a diff, plus a short "what to prioritize" list that
puts correctness and security first (this backend spawns processes and proxies
outbound requests). Deliberately omitted: board recipes and IDs, milestone and
branch mechanics, release procedure, and the project tree — no reviewer cites
those, and copying them would double the maintenance surface.

AGENTS.md's "Keep documentation files up to date" section now requires
mirroring review-relevant changes into it in the same PR, states that AGENTS.md
stays the source of truth, and spells out what counts as review-relevant. There
is no generation step and nothing detects drift, so the rule is the only thing
keeping the two in sync.

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

* docs: address Copilot review — v1 policy wording, stale V2 Go Live, typo

- Align the v1 maintenance policy on "security and bug fixes only" in
  AGENTS.md and CONTRIBUTORS.md; both said "security fixes only" while the
  README callout, the branch table, and the board description said
  "security and bug fixes".
- Drop the stale "or V2 Go Live" instruction from the work-begins step —
  that column no longer exists on board #28 (verified via
  `gh project field-list 28`), so the option id it implied would be
  rejected. Restore the removed-column id list the same edit dropped, now
  including V2 Go Live, and re-date the table to today's verification.
- Fix "contains the the new version" typo.
- Strip a stray trailing blank line at the end of AGENTS.md.

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

* docs: restore `inspector/` as the Project Structure tree root

The tree is a filesystem layout — every child is a real directory
(`clients/`, `core/`, `test-servers/`). Labeling its root `v2/main/` named
a branch instead, which reads as a path that does not exist in a checkout
and diverged from the identical tree in README.md, which still says
`inspector/`. The branch model is documented in its own table under
Repository & Project Boards; the tree does not need to restate it.

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

* docs: settle the v1 policy wording as "security fixes only"

Reverses the direction the Copilot round took in 33cc0366. That commit
normalized the six v1-policy sites onto "security and bug fixes" because that
was the majority wording — but majority is the wrong tiebreak here. The policy
is set by what we told contributors: the #1819 backlog close note, posted to
120 closed PRs, says "v1 will receive security fixes only", and the five PRs
retained for 1.0.5 are all security fixes.

So the two sites that already said "security fixes only" were the correct ones,
and the four that said otherwise are now aligned to them:

- AGENTS.md: the `v1` label description, the branch-role table, and the v1
  board line.
- README.md: the "Repo status" callout.

CONTRIBUTORS.md and AGENTS.md's Project Status bullet already read correctly
and are unchanged.

This is the substance of #1813, which is why that issue's PR is now reduced to
its PR-template-link half — the wording work landed here to avoid two PRs
editing the same lines in opposite directions.

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

* docs: point the AGENTS.md policy link at CONTRIBUTING.md

This PR rewrites the paragraph holding the only in-repo link to the
contributing policy, so it owns that line. #1884 renames the file
CONTRIBUTORS.md -> CONTRIBUTING.md; having #1884 also edit this line would
guarantee a one-line conflict between the two.

Writing the new filename here instead makes #1884 a pure `git mv` with no
overlapping line, so neither PR conflicts with the other.

Between this merging and #1884 merging, the link points at a filename that
doesn't exist yet. That window is deliberate and bounded — this PR is first in
the merge order (#1821) and #1884 is third.

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

---------

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
 2026-07-31 23:00:24 -04:00
Sam Morrow
5d91d4acd0
 		fix(core): mirror SEP-2243 x-mcp-header args to Mcp-Param-* on tools/call (#1847) 
* fix(core): mirror SEP-2243 x-mcp-header args to Mcp-Param-* on tools/call (#1846)

A strict modern (2026-07-28) HTTP server (e.g. the GitHub MCP Server)
rejects a tools/call whose argument carries an `x-mcp-header` annotation
when the matching `Mcp-Param-*` request header is missing (-32020
"header mismatch"). The Inspector never sent that header.

The SDK only mirrors inside `client.callTool()` (and skips it entirely in
a browser environment), but the Inspector routes tools/call through
`client.request()` for manual MRTR driving (#1704), so the SDK never
mirrors for us — on any transport. On top of that, the remote (web)
transport dropped per-request headers before they reached the backend's
upstream fetch.

- core/json/xMcpHeader.ts: port the SDK's non-public `buildMcpParamHeaders`
  + value encoding (Base64 sentinel for non-ASCII/whitespace/empty values)
  and add `mcpParamHeadersForTool`.
- InspectorClient.attemptToolCall: on a modern connection, build the
  headers from the tool's declarations + call arguments and attach them to
  the tools/call request options. `Protocol.request` forwards `headers`
  to the transport and preserves them across MRTR retry legs, so the
  direct StreamableHTTP transport applies them. Fixes CLI/TUI.
- RemoteClientTransport.requestSend + /api/mcp/send + RemoteSendRequest:
  forward per-send headers and apply them to the backend's upstream
  transport.send. The browser can't set cross-origin headers, but the
  Node backend can — where the Inspector's real request is issued. Fixes web.

Verified end-to-end against the live GitHub MCP Server (modern HTTP).

Closes #1846

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 36dbeb09-12b0-4c19-af36-2573772a7dbd

* review: harden backend header allowlist, fix breadcrumbs, trim comments

Address Copilot review on #1847:
- server.ts: restrict client-supplied per-send headers to the `Mcp-Param-`
  prefix (string values only) via `mcpParamHeadersOnly` before applying them
  upstream, so the per-send channel can't inject arbitrary headers (e.g.
  Authorization). `settings.headers` remains the channel for configured
  upstream headers. Covered by new unit tests.
- Fix stale `#1810` breadcrumbs → `#1846` in the two new tests.
- Trim the verbose mirroring comments in inspectorClient/types/remote transport.
- Drop the redundant base64-on-the-wire integration test (encoding is unit-
  tested; wire delivery is proven by the verbatim test), remove two unneeded
  test casts, and cover the boolean `true` branch.

Not changed (declined with reason on the PR): buildMcpParamHeaders remains a
faithful port of the SDK's helper and does not re-validate values against the
declared JSON Schema type — arg types are enforced by schema validation on
both sides, and matching the SDK's conforming wire output is the module's goal.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 36dbeb09-12b0-4c19-af36-2573772a7dbd

* review: mirror Mcp-Param-* on the task-augmented tools/call too

Address the maintainer review on #1847:

- `callToolStream` (the "Run as task" path) built its own request options and
  never mirrored, so a task-augmented `tools/call` on an annotated tool was
  still rejected by a strict modern server with -32020. It now mirrors like the
  plain path.
- Extract the two blocks both `tools/call` entry points had duplicated —
  `convertStringToolArgs` (the string-arg coercion) and
  `applyMirroredParamHeaders` (the SEP-2243 mirroring) — so the paths can't
  drift again. That duplication is why the task path was missed.
- Regression test: `callToolStream` against the modern `get_weather` server
  asserts the POST carries `Mcp-Param-City` (verified failing without the fix).
- README: the modern-network showcase claimed mirroring is skipped in the
  browser so the web client gets -32020, and that CLI/TUI mirror. Both are now
  wrong — the Inspector mirrors itself, on every client. AGENTS: note that
  xMcpHeader.ts now also builds the wire headers.

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

* docs: correct the stale "browser omits Mcp-Param-*" copy

Both statements predate this PR and are now the opposite of true, per the
wire capture on #1847 (proxy-observed upstream headers, web leg included).

- ToolDetailPanel: the mirrored-headers note claimed the browser omits
  `Mcp-Param-*`. It now says the web client's headers are applied by the Node
  backend that issues the upstream request.
- createRemoteFetch: the header comment claimed an `x-mcp-header`-annotated
  tool is "uncallable from the web client against a strict modern server" —
  the exact wrong conclusion to reach while debugging a -32020. It now records
  that the Inspector builds the headers itself, and that they do NOT travel
  this `/api/fetch` proxy path: they ride the transport's per-send `headers`
  through `/api/mcp/send`.

Copy only — no test asserted the old note text (ToolDetailPanel.test.tsx
matches the section title only).

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

---------

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: cliffhall <cliff@futurescale.com>
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Copilot-Session: 36dbeb09-12b0-4c19-af36-2573772a7dbd
 2026-07-28 19:19:48 -04:00
Cliff Hall
4d30d1cd1e
 		docs: restore the missing docs/ targets and split the catalog-seeding wording (#1811) (#1812) 
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.

- `docs/mcp-server-configuration.md` — written fresh for v2 (the v1.5 file predates the `--catalog`/`--config` split)
- `docs/launcher-config-consolidation-plan.md` — restored from `v1.5/main`, path drift corrected, v2 notes appended
- `docs/images/tui-screenshot.png` — regenerated against the current TUI
- launcher README seeding wording — split into the two real behaviors (web seeds two sample servers; cli/tui seed empty)

Review across three rounds also corrected several claims verified against source: the `--` separator is inverted between web and cli, a cli target placed after a flag is silently discarded, `--server` is a no-op on web and an unknown-option error on tui, the `MCP_CATALOG_PATH` ad-hoc guard is cli-only, and the conflict matrix diverges on both `--header` and `--transport stdio`.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

https://claude.ai/code/session_01Txmv2qqv3yeKgRzoqXytzD
 2026-07-27 20:37:14 -04:00
Cliff Hall
de9d8a5024
 		fix: wait for the TUI to exit before removing its temp dir (#1801) (#1814) 
* fix: wait for the TUI to exit before removing its temp dir (#1801)

`done()` sent SIGTERM and then immediately `rmSync`'d the work dir, which
doubles as the TUI's HOME. Nothing waited for the child, so the Ink process
could still be writing into the dir while rmSync walked it — a file landing
after a directory was read but before it was removed surfaces as ENOTEMPTY
on macOS, failing `smoke:tui` after a successful render.

Removal now happens only once the child's `exit` event fires: SIGTERM, then
SIGKILL after SMOKE_TUI_EXIT_GRACE_MS (default 5s), then clean up regardless
after twice that. The crash-before-render path short-circuits via a
`childExited` flag set by the pre-registered exit handler. cleanup() also
warns instead of throwing, so a leftover temp dir can never fail a passing
smoke.

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

* fix: wait on close, not exit, so spawn failure resolves instantly (review #1814)

A failed spawn emits `error` + `close` and never `exit`, so the previous
`child.once("exit")` wait never fired on that path: done() sent SIGTERM to a
process that never existed and resolved only via the give-up timer — 10s at
the default grace, with a warning blaming a SIGTERM the child never received.
Waiting on `close` covers both normal termination and spawn failure, and
additionally guarantees the stdio pipes are drained.

The short-circuit gate moves with it, `childExited` → `childClosed`: keying it
on `exit` would have skipped the drain on the crash-before-render path, which
is exactly where the quoted output matters. Messages that quote that output are
now thunks rendered in finish(), after the drain, instead of being sliced at
call time. SIGTERM is now sent only to a child that is still running, since on
both the crash and spawn-failure paths there is nothing left to signal.

finish() also gets a `finished` guard so its single-entry invariant is local
rather than inferred from process.exit() being synchronous.

Measured, spawn failure (grace shortened to 500ms): 1.04s + spurious warning
before, 0.02s and silent after. Crash-before-render now prints the child's
stderr detail line that the pre-drain slice could truncate. Happy path 3x,
timeout path, and full `npm run smoke` + `npm run validate` all green.

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

* fix: bound the drain, and make the give-up warning true (review #1814 r2)

Three follow-ons from the second review pass:

The give-up warning still said the TUI "did not exit within Xms of SIGTERM",
but after switching the wait to `close` neither clause is reliably true — the
timer's most plausible trigger is now a child that has already exited, and on
the crash and spawn-failure paths no SIGTERM is sent at all. That is the same
defect this PR fixed one round ago (a warning naming a signal never sent), so
the deadline is now re-armed with a message describing what is actually being
awaited.

`close` is bounded by whoever holds the stdio pipes, which can outlive the
direct child: a descendant that inherited them keeps it pending. That is the
one real cost of `exit` → `close`, and it is moot only because the TUI spawns
nothing at boot — a reason that lives entirely outside this file. Once `exit`
has fired the deadline drops to DRAIN_MS, keeping the drain guarantee without
inheriting an unrelated process's lifetime. Measured against a grandchild
holding the pipes for 3s: 0.54s and an accurate warning, instead of 3s.

The quoted output is sliced from the tail rather than the head. The thunk added
last round exists to capture bytes that arrive late, which a head slice then
discards as soon as output exceeds 800 chars — and on the timeout path the last
frame is what diagnoses a stuck render, not the first. Verified: a 2KB stderr
crash now reports its final line. The sibling scripts keep `slice(0, 800)`;
changing them is a deliberate sweep, not a drive-by here.

Also corrected the cleanup() comment — the wait lives in done(), and it is for
`close`, not `exit`.

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

* docs: correct the cleanup() and DRAIN_MS comments; tail-slice cleanly (review #1814 r3)

The cleanup() comment claimed its warning "should not fire now that removal
waits for close" — true only of the normal path. Both give-up branches call
finish() without close, and those are precisely the states where something may
still hold the dir, so the warning is expected there. Same defect class as the
two warnings already fixed in this PR (a message asserting what the code no
longer guarantees), relocated into a comment.

The DRAIN_MS comment explained why the cap exists but not what it costs: on
that path the work dir is removed while a descendant holding HOME=work may
still be writing to it — #1801's race, re-entered deliberately. That is only
acceptable because cleanup() warns instead of throwing, so record the two as
load-bearing for each other where the constant is defined.

Tail-slicing an Ink stream can begin mid-CSI-sequence, leading a diagnostic
with an escape-code fragment that mangles the line after it. outputTail() drops
through the first newline when the output was actually truncated, and returns
short output untouched.

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

---------

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
 2026-07-27 20:25:27 -04:00
Cliff Hall
cb454990eb
 		docs: refresh V2 board Status option IDs in AGENTS.md (#1823) (#1825) 
* docs: refresh V2 board Status option IDs in AGENTS.md (#1823)

`V2 Go Live` (`b3a6966e`) was added to the Status field via the web UI on
2026-07-27 for the go-live phases (#1804) and was not in the table; the
`SDK V2 + New Spec` row it replaced in position (`1bbb6f57`) had already
been removed, so following the table failed with "option Id does not
belong to the field". Record the current five options, note that adding
the column via the web UI left the other four IDs untouched (evidence the
UI path is safe, unlike the GraphQL full-replace hazard documented below
it), and list every removed column's dead ID in one place.

Also add the missing recipe for moving a card that is already on the board:
the existing recipes only cover `item-add` for a new item, leaving no
copy-paste path for looking an existing card's item id up by issue number.

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

* docs: close the ID-rotation pointer over the new recipe (review #1825)

The hazard note's maintenance pointer lists the hardcoded option ids that
must be updated when they rotate, but the recipe added in the previous
commit hardcodes a third one (159c8a02) that the pointer did not name —
reproducing the staleness class this PR exists to fix. Add it.

Also fold in two adjacent fixes: the "In review" casing left behind two
lines from the "In progress" → "In Progress" fix (labels are matched
exactly when re-fetching ids by name), and a note that --limit must stay
above the board's item count, since exceeding it truncates silently and
surfaces as an opaque node-resolution error from item-edit.

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

* docs: move the --limit caveat inside the recipe fence (review #1825)

The section tells the reader to copy the recipes verbatim, so guidance that
lives only in the prose above the fence is lost by anyone who grabs the block
— which is the whole point of the block. The two recipes above it already
carry their context as in-block comments; match that.

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

---------

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
 2026-07-27 20:10:01 -04:00
Cliff Hall
b3be65b0b4
 		docs: add CONTRIBUTORS.md issues-only policy (closes #1517) (#1604) 
* docs: add CONTRIBUTORS.md issues-only policy (#1517)

Document the contribution policy: we accept issues, not pull requests.
Maintainers handle design and implementation through a prompt-driven
workflow. Contributors who have already built a change locally should
share the prompt they used rather than a diff.

Cross-reference the policy from AGENTS.md.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* docs: add PR template redirecting external authors to issues-only policy (#1517)

GitHub auto-populates pull_request_template.md into every new PR body,
so it steers would-be external PR authors to CONTRIBUTORS.md before they
submit a diff.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* docs: drop version-board details from CONTRIBUTORS.md (#1517)

The policy takes effect after v2/main merges into main, leaving a single
line of development. Tell contributors to open a well-formed issue rather
than routing them to per-version project boards.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* docs: add version-board/label table to CONTRIBUTORS.md (closes #1595)

Completes requirement #2 of #1517: a table mapping each active version to
its base branch, project board, and version label, plus a "Label by
version" note mirroring AGENTS.md.

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

* docs: drop v1.5 from CONTRIBUTORS.md, invite deeper contribution (#1517)

Review feedback on PR #1604:

- Remove the v1.5 row/bullet from the version table — v1.5 was an internal
  intermediate branch that external contributors never saw, so surfacing it
  in an outward-facing doc is noise. The table is now v1 / v2 only.
- Add a "Want to work on the Inspector with us?" section pointing at the MCP
  Contributor Discord (#inspector-dev) and the community calls, framing the
  issues-only rule as being about unsolicited patches rather than a closed
  door, with supervised work as the path toward maintainership.
- Link that section from "Why this policy exists", and add the same Discord
  pointer to the PR template.

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

* docs: drop project-board links from CONTRIBUTORS.md (#1517)

The org project boards aren't visible to anyone outside the organization, so
linking them in an outward-facing doc points contributors at a 404. The
version table is now base branch + label only, and the PR template names the
version labels instead of "the appropriate version board".

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

* docs: v1 takes security fixes only (#1517)

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

* docs: use post-runbook branch names (v1/main, main) (#1517)

This doc will be read after the branch runbook completes, when v1 lives on
v1/main and v2 is the default main. Update the version table, the label-by-
version line, and the PR template's label hint + CONTRIBUTORS.md links
accordingly.

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

---------

Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
 2026-07-27 19:04:17 -04:00
Cliff Hall
7784553674
 		fix(auth): explicit re-authorize when the OAuth callback state was lost (#1808) (#1810) 
* fix(auth): offer explicit re-authorize when callback state is lost (#1808)

A `/oauth/callback` that arrives with no recorded SEP-2352 discovery state
made the SDK throw `AuthorizationServerMismatchError` and the Inspector
dead-ended, surfacing the raw SDK text with no path forward.

The SDK folds two very different failures into that one error class, so
`core/auth/issuerBinding.ts` classifies them structurally: a recoverable
"lost authorization state" (the SDK puts prose, not an issuer URL, in the
`recordedIssuer` slot) versus a genuine cross-AS `issuer_mismatch`. Only
the first gets a recovery affordance.

Web: the callback failure now raises a dedicated banner ("Authorization
state was lost" / "Authorize again") whose action clears the stale OAuth
state for that server and starts a fresh authorization. A genuine
mismatch stays a red, non-dismissible security notification with both
issuers named and no one-click retry.

Node runners (CLI/TUI): the same classification rewrites the callback
rejection into the shared actionable copy instead of the SDK text.

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

* fix(auth): detect the SDK mismatch error via isInstance (#1808)

The callback-leg classifier gated on `err.mcpBrand`, but the SDK declares
that brand in a `static {}` block, so it lives on the constructor and is
never reachable from an instance. The predicate always returned false, so
`findIssuerBindingFailure()` never matched and the #1808 recovery banner
was inert in production — the callback dead-ended exactly as before.

Use the SDK's own `AuthorizationServerMismatchError.isInstance()`, which is
cross-copy safe by construction (it consults a `Symbol.for()`-keyed brand
set), with a `name` fallback for a serialization boundary. Correct the
module doc, which cited cross-bundle safety as the reason to avoid
`instanceof` — the very thing the SDK's branded `hasInstance` provides.

Every fixture now constructs a real `AuthorizationServerMismatchError`
instead of a hand-rolled object with an own `mcpBrand`, which is why a
green `npm run ci` hid this. Adds a test pinning the brand's placement so
the feature cannot silently go inert again.

Review follow-ups: drop the unused `isLostAuthorizationStateError`, remove
the duplicated banner title default, resolve the banner title/action label
where the banner is raised, keep the clear-failure notification open, and
bound remote-supplied issuers in user-facing copy.

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

---------

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
 2026-07-27 18:08:31 -04:00
Cliff Hall
01df9d81bb
 		fix(core): start OAuth on a modern/auto-era connect (#1805) (#1809) 
* fix(core): start OAuth on a modern/auto-era connect (#1805)

The SDK's `server/discover` negotiation probe runs before anything else on
`protocolEra: "auto" | "modern"`, and its classifier reports whatever the
transport threw as `SdkError(ERA_NEGOTIATION_FAILED)` with the real error moved
to `data.cause`. That buried both connect-time auth signals: the remote path's
`AuthRecoveryRequiredError` (matched with `instanceof`, and carrying the
authorization URL) and a direct transport's `AuthChallengeError`. An
OAuth-protected server that authorized fine on the legacy era instead produced a
dead-end "Version negotiation probe failed" — no redirect, and the connection
status flipped to `error` despite `isConnectAuthRecoveryError` existing to
prevent exactly that.

- `findNestedAuthError()` walks `cause` / `data.cause` for either typed error.
  Not gated on the SDK's error code or message, so it survives rewording.
- `connect()` unwraps the rejection at the earliest point, ahead of
  `withDirectAuthRecovery` (whose `isAuthChallengeError` check is shallow), the
  outer status guard, and every client's connect error handling — so web, CLI,
  and TUI are all fixed without a client-side change.
- On the direct path, enable `interceptAuthChallenges` for the probing eras even
  with no stored tokens. Without an authProvider the 401 reaches the SDK as a raw
  `SdkHttpError`, and the classifier ignores the HTTP status (it only looks for a
  JSON-RPC error body) — so pin mode rethrew it with the 401 discarded entirely,
  no status and not even a cause for the walk to find.

Verified against a live dual-era server that answers 401 on both `initialize`
and `server/discover`: `auto` and `modern` now reject with
`AuthRecoveryRequiredError` carrying a real authorization URL. Each half of the
fix is load-bearing — reverting either fails the new integration tests.

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

* docs(core): mark the era-probe challenge interception as a removable workaround

Per review on #1806: the `|| this.probesProtocolEra()` clause compensates for
an upstream SDK gap (modelcontextprotocol/typescript-sdk#2561, tracked as
#1807). Make its lifespan explicit so whoever picks up #1807 knows this block
is the thing to delete, rather than having to re-derive it.

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

* fix(core): resolve connect() when auth recovery is satisfied silently

Review findings on #1809:

- connect() built its handshake promise once, outside the runConnect closure
  withDirectAuthRecovery retries. On a "satisfied" outcome the recovery runs a
  complete nested connect() via reconnectAfterAuthRecovery(), so the retry leg
  re-awaited the already-rejected promise and rejected a connect() whose client
  was in fact connected. Short-circuit the retry when already connected, and
  skip the post-connect block the nested connect already ran (a duplicate
  `connect` event re-triggers every list-state manager's refresh).
- Document the accepted 403 side effect of enabling interceptAuthChallenges
  with no stored tokens, and why every `{ pin }` counts as probing.
- Replace the private-method double cast in the era-probe test with an
  observable assertion on the transport options, and split the non-auth
  failure case per era instead of a message alternation that passed either way.

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

* fix(core): key the connect retry leg off attempt, bound nested recoveries

Second-pass review findings on #1809:

- The satisfied-recovery short-circuit keyed off live connection status, so an
  onclose landing between the nested connect() and the retry leg fell through
  to the rejected handshake promise and reported the stale 401 as the reason
  the session died. withDirectAuthRecovery now passes `attempt` to its
  operation, so connect() identifies the retry leg structurally and reports the
  teardown when the recovered session is already gone.
- Bound nested silent recoveries. The `attempt >= 1` guard is per-call and a
  satisfied outcome recovers via a nested connect() with a fresh counter, so a
  server that keeps rejecting freshly refreshed tokens recursed without bound.
  Counted across the nesting boundary, capped at 3.
- Strengthen the regression test (server info populated by the nested connect,
  no duplicate statusChange) and add the two missing cases: a failed reconnect
  still rejects, and the recursion bound terminates.

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

* test(core): cover the recovery teardown branch; fix a duplicated case

Third-pass review findings on #1809, both about tests added in the previous
commit:

- The retry leg's teardown error had no coverage, so a regression to the old
  status fall-through would have stayed green. Covered deterministically: the
  nested connect dispatches `connect` after setting "connected", so closing the
  live transport from that listener flips the status before the retry leg runs.
  Asserts the error identity, which is the point of the change.
- "still rejects when the reconnect underneath the recovery fails" used a
  challenging transport for every connect, so it walked the same bounded chain
  as the recursion-bound test rather than covering a non-auth reconnect
  failure, and its comment claimed an "error" status the auth path never
  reaches. Now fails the reconnect with a plain error and asserts both.

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

---------

Co-authored-by: rinormaloku <rinormaloku37@gmail.com>
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
 2026-07-27 15:06:01 -04:00
Cliff Hall
d9e4f444a2
 		test(cli,tui): typecheck the __tests__ dirs (#1791) (#1799) 
* test(cli,tui): typecheck the __tests__ dirs (#1791)

The cli/tui `typecheck` scripts only checked `src` (the src-only tsconfig
excludes `**/*.test.*`), leaving the `__tests__` dirs — which do the most
`as` casting — with no tsc pass. Add a `tsconfig.test.json` per client that
includes `__tests__` with the test-exclusions dropped and the test-only path
aliases (`@modelcontextprotocol/inspector-test-server`, the `@inspector/core/*`
deep paths, express/vitest) resolving what vitest resolves, then fold
`tsc -p tsconfig.test.json` into each `typecheck`.

Fixes the 76 errors it surfaced:
- cli: a wrong `@modelcontextprotocol/inspector-core/...` module specifier
  (should be `@inspector/core/...`; only a type import so vitest never caught
  it), `r.headers` optional-access guards, and partial OAuth mocks replaced
  with typed `makeFakeCliOAuthClient()` / `makeFakeServerSettings()` factories
  instead of `as` casts. Adds `@types/express` (devDep) so the transitively
  aliased test-server source typechecks, mirroring clients/web.
- tui: rest-param signatures on the App OAuth spies (fixes the spread-arg and
  narrow-return errors), a widened callback param (`iss?`), a `makeFakeServer-
  Settings()` factory for the tuiOAuth tests, required OAuth-metadata/client
  fields in AuthTab, and a loosened `entry()` message param for the
  deliberately-malformed wire messages HistoryTab's defensive branches render.

Gate coverage only; no runtime behavior change.

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

* test(cli,tui): address review — real spy signatures, config typecheck, nits

Responds to the @claude review of #1799:

- App.test.tsx: type the OAuth spies against the real InspectorClient method
  signatures (`vi.fn<InspectorClient["…"]>()`) and forward `Parameters<…>`
  tuples from the FakeClient wrappers, so `toHaveBeenCalledWith(...)` on the
  six OAuth-lifecycle spies stays argument-checked under the new gate rather
  than accepting anything via `unknown[]`. `getOAuthState` regains its wide
  `OAuthConnectionState | undefined` return.
- Typecheck the client config files too: add `vitest.config.ts` / `tsup.config.ts`
  (and tui `dev.ts`) to each src `tsconfig.json` include — they were the one
  first-party surface still getting no tsc pass.
- Trim the redundant `src/**/*` from each `tsconfig.test.json` include (tsc
  pulls in the src the tests import; the src-only config already checks it).
- Rename the tui settings helper to `oauth-test-fakes.ts` to match cli's, and
  soften both factories' docstrings to stop naming a loader they don't mirror
  field-for-field.
- Fix a stale `@modelcontextprotocol/inspector-core/...` specifier in a
  createRemoteTransport JSDoc example (the same wrong package name the gate
  caught in the cli tests, surviving in a comment).

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

* test: address round-2 review — correct claims, tighten types, typecheck launcher

Responds to the round-2 @claude review of #1799:

- Correct the toHaveBeenCalledWith claim: vitest doesn't type-check
  `toHaveBeenCalledWith(...)` arguments against the mock's signature, so the
  App.test.tsx comment no longer claims it does — the typed spies buy checked
  implementations/return payloads (the stale-literal fix), not arg-checking.
- authenticate spy now uses the real `InspectorClient["authenticate"]`
  signature (returns a `URL`), so it no longer diverges from production; the
  one mockResolvedValue is updated to a URL.
- makeFakeCliOAuthClient members are typed against the real CliOAuthClient
  signatures (not bare vi.fn()), so a wrong-signature override is rejected.
- HistoryTab: revert the `entry()` `message: unknown` widening (it un-checked
  the ~15 well-formed fixtures) and instead scope a named `MALFORMED` cast to
  the two intentionally-malformed messages.
- Typecheck the launcher `__tests__` too (the same gate hole in the one client
  the PR hadn't touched): new clients/launcher/tsconfig.test.json + a typecheck
  script folded into validate. Completes the "every client's __tests__ is
  typechecked" invariant.
- Doc the new test-typecheck + config-file typecheck in AGENTS.md.

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

* test: address round-3 nits — launcher config typecheck, callTool typed, docs

Responds to the round-3 @claude review of #1799 (all non-blocking):

- launcher: add `vitest.config.ts` to tsconfig.test.json's include (it was in
  neither project — the build config's rootDir: ./src rejects it). Now
  typechecked.
- App.test.tsx: type `callTool` against `InspectorClient["callTool"]` (+ the
  Parameters<…> wrapper) so its `mockResolvedValue` payloads are checked, and
  give `clearOAuthTokens` an `async () => {}` impl so the mock actually returns
  the `Promise<void>` its type promises.
- AGENTS.md: correct the per-client alias-set description (cli's is widest,
  tui's is core+react only, launcher's has no paths) and note launcher's config
  file lives in its test project, not the src project.

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

* test: address round-4 — durable typecheck-coverage guard + launcher/doc nits

Responds to the round-4 @claude review of #1799 (all non-blocking):

- Add `verify:typecheck-coverage` (scripts/verify-typecheck-coverage.mjs), the
  typecheck-coverage analog of verify:format-coverage and the durable guard for
  the invariant this PR establishes: for each Node client it runs the tsconfig
  projects its `typecheck` names with `tsc --listFiles`, unions them, and fails
  on any tracked .ts/.tsx that lands in no project. This closes the class the
  review kept surfacing by hand (launcher/__tests__ in round 3,
  launcher/vitest.config.ts in round 4) — a new top-level file can no longer go
  untypechecked silently. Wired into `npm run ci` and the GitHub workflow.
- launcher: reorder `validate` to run `typecheck` before `build` (matches tui
  and the documented order), and make `typecheck` self-contained — it now runs
  both `tsconfig.json` and `tsconfig.test.json`, so running it standalone means
  the same thing it does in cli/tui.
- makeFakeCliOAuthClient: note on the comment that the typing covers the
  defaults, not `overrides` (a bare vi.fn() override is Mock<(...args)=>any>).
- Docs: README table + AGENTS.md updated for the new guard and the corrected
  launcher typecheck/validate ordering.

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

* test: address round-5 — wire-check the guard, --listFilesOnly, into validate

Responds to the round-5 @claude review of #1799:

- Finding 1 (the pre-merge one): the guard now ports verify:format-coverage's
  reachability checks — it asserts each client's `typecheck` is reachable from
  its own `validate` and that the root chain runs each client's `validate`, so
  it can't stay green while measuring a typecheck nothing invokes. Verified:
  stripping `npm run typecheck` from launcher's validate now fails the guard.
- Nit 3: switch `tsc --listFiles` → `tsc --listFilesOnly` (identical file list,
  ~4× faster: whole guard 23s → ~4s). Now cheap enough for the inner loop, so
  it moves into `validate` (right after verify:format-coverage) instead of a
  standalone ci step — matching its sibling. Removed the separate ci-chain entry
  and GitHub workflow step (validate covers it).
- Nit 4: `npx --no-install tsc` so a missing local tsc fails fast instead of
  reaching for the registry (matches verify-build-gate.mjs).
- Nit 5: corrected projectFiles' JSDoc to describe the filter that's actually
  there (drops repo-external + node_modules paths; core/ + test-servers/ stay
  but are harmless).
- Finding 2: fixed the 2-space misindent on the new root package.json line
  (prettier --write). Docs (README/AGENTS) updated for the validate placement.

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

* chore(scripts): address round-6 guard nits — dedupe wiring, robust diagnostics

Responds to the round-6 @claude review of #1799 (all guard-ergonomics nits):

- Nit 2: extract the shared reachability logic (`reachableScripts`,
  `rootRunsClientValidate`) into scripts/lib/npm-scripts.mjs and consume it from
  BOTH verify-format-coverage.mjs and verify-typecheck-coverage.mjs, so the
  "is this gate actually run?" wiring can't drift between them (same rationale
  as scripts/lib/prod-web-server.mjs).
- Nit 1: typecheckProjects now harvests `-p`/`--project` from every script
  reachable from `typecheck` (not the single string), so a delegating
  `typecheck` (`npm run typecheck:src && …`) is handled instead of failing with
  a false "names no -p project" diagnosis. Also accepts `--project`.
- Nit 3: on a tsc config error, echo the `error TS…` diagnostic (scanning both
  streams — tsc prints config errors to stdout) so a broken tsconfig is
  self-explaining rather than surfacing only as a misleading "file in no
  project" report.
- Nit 4: refreshed the CI workflow's stale "Validate" step name/comment (it now
  runs the coverage guards + validate:core + per-client typecheck).
- Nit 5: reworded the `.d.ts` carve-out comment to read as pre-emptive (there
  are none under cli/tui/launcher today).

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

* chore(scripts): round-7 — guard .mts/.cts too; drop dead export

Responds to the round-7 @claude review of #1799:

- Finding 1: verify-typecheck-coverage now requires a tsc pass for tracked
  `.mts`/`.cts` (not just `.ts`/`.tsx`), matching verify-format-coverage's
  extension set — `.mts` is already the shared-config idiom here, so a client
  `vitest.config.ts` → `.mts` rename can no longer drop out of both the tsconfig
  include and the guard's required set. No-op on today's tree (no client
  `.mts`/`.cts`); confirmed a tracked `.mts` with a type error is now caught.
  Ambient `.d.{ts,mts,cts}` stay excluded.
- Nit 2: drop the unused `export` on `rootReachedCommands` (internal to the lib;
  nothing imports it, and scripts/ isn't eslint-gated to catch dead exports).

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

* test: round-8 — single casts in HistoryTab, uniform wrappers, doc scope

Responds to the round-8 @claude review of #1799:

- Finding 1: reduce the `MALFORMED` helper and `entry()`'s return cast from
  `as unknown as` to a single `as` (both compile clean — the malformed shapes
  are comparable to the JSONRPCMessage union), and delete the false "only
  option" sentence. Fixes the AGENTS.md avoid-double-casts violation.
- Nit 3: write all seven FakeClient OAuth wrappers as
  `(...a: Parameters<InspectorClient["…"]>)` (the three zero-arg ones were
  hand-written `()` forwarders), so the block comment's "forward the same
  Parameters<…> tuple" is literally true and a future added parameter can't be
  silently dropped.
- Nit 2: update the five places that still described the guard as `.ts`/`.tsx`
  only to `.ts`/`.tsx`/`.mts`/`.cts` (script header + trackedSourceFiles JSDoc,
  AGENTS.md ×2, README.md).

Nits 4 (js config files out of the typecheck guard by construction) and the
footnoted callTool/ancestor-tsc items left as agreed.

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

* test: round-9 — drop entry()'s harmful cast; guards vouch for each other

Responds to the round-9 @claude review of #1799:

- Finding 1: remove `entry()`'s `as MessageEntry` return cast in
  HistoryTab.test.tsx. It was unnecessary (the spread of Partial<MessageEntry>
  over complete defaults is directly assignable) and harmful — it silenced type
  errors in the helper's OWN default fields (a `direction`/`method` typo now
  surfaces TS2820/TS2353). MALFORMED's single cast stays (load-bearing). This
  leaves MALFORMED as the file's only cast, matching what the comments describe.
- Nit 2: add `rootReachesScript` to scripts/lib/npm-scripts.mjs and have each
  coverage guard assert its SIBLING is still wired into the root `validate`. A
  guard can't detect being unrun itself, but the two now vouch for each other,
  so dropping either from `validate` is caught by the other (only deleting both
  slips through). Verified: removing verify:typecheck-coverage from validate
  now fails verify:format-coverage, and vice-versa.

Nits 3 (git-index visibility) and 4 (pre-existing as-never/as-unknown casts in
tests — out of scope, none added by this PR) left as agreed.

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

* test: round-10 — reject non-checking typecheck passes; pre/post hooks; docs

Responds to the round-10 @claude review of #1799:

- Finding 1: verify-typecheck-coverage now rejects a `-p <project>` whose own
  command carries `--noCheck` or `--listFilesOnly` — such a pass lists files
  without type-checking them, which previously satisfied both the wiring and
  file-coverage halves while checking nothing. (Splits each script on
  `&&`/`||`/`;` so a flag on one command can't taint another.) Verified a
  `typecheck` using `tsc … --noCheck` now fails with a clear message; `--noCheck`
  is a live idiom here (cli's test-servers:build), so this is a real copy-paste
  hazard.
- Nit 2: reachableScripts (shared lib) now also follows npm's implicit
  `pre<name>`/`post<name>` lifecycle hooks, so moving a gate into e.g.
  `prevalidate` is recognized instead of producing a false "not reachable"
  failure (cli already uses a `pretest` hook). Verified.
- Nit 3: AGENTS.md "Test placement" now states the Node clients (cli/tui/
  launcher) keep all tests in `__tests__/`, enforced by verify:typecheck-coverage
  (a co-located src test lands in no tsconfig project).
- Nit 4: noted on the factory comments that `Partial<…>` overrides admit an
  explicit `undefined` for a required field (exactOptionalPropertyTypes off).

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

* fix: round-11 — launcher build excludes tests; guard catches all noCheck vectors

Responds to the round-11 @claude review of #1799:

- Finding 1 (shipping bug): clients/launcher/tsconfig.json now excludes
  `**/*.test.{ts,tsx}` (matching cli/tui). launcher's `build` is an EMITTING
  tsc over src/**, and clients/launcher/build is in the root `files` allowlist,
  so a co-located `src/**/*.test.ts` would have been compiled into the published
  tarball. Excluding it also makes such a test land in no tsconfig project, so
  verify:typecheck-coverage catches it — making the AGENTS.md placement rule
  literally true for all three clients.
- Finding 2: verify-typecheck-coverage now catches both remaining ways a
  typecheck could check nothing: (a) case-insensitive `--noCheck`/`--nocheck`/
  `--listFilesOnly` on the command, and (b) `noCheck` set in the tsconfig itself
  (detected via `tsc --showConfig`, which surfaces it from the config or its
  extends chain). Verified both fail the guard.
- Nits 3 & 4: restructured into an explicit gate-integrity phase (wiring +
  neutered + config-noCheck + no-projects) that reports under its own accurate
  header and exits BEFORE the file-coverage pass — so an inert gate no longer
  prints file-remediation advice, nor buries the real cause under an
  "in no tsconfig project" line for every file that gate covered.

The one CIMD OAuth E2E integration test that flaked in the coverage run passes
32/32 in isolation and on re-run; unrelated to these script/config changes.

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

* fix: round-12 — exclude .mts/.cts tests from build; drop false integrity line

Responds to the round-12 @claude review of #1799:

- Finding 1 (tarball, again): the round-11 test exclude only covered
  `.ts`/`.tsx`, so a co-located `src/*.test.mts` still compiled into launcher's
  shipped `build/`. All three clients now exclude `**/*.test.*` (one glob,
  covering .ts/.tsx/.mts/.cts) — which is also exactly what AGENTS.md already
  claims. Verified a `src/probe.test.mts` is no longer emitted and now fails the
  guard.
- Finding 2: the gate-integrity phase no longer prints a false "names no
  `-p <project>`" line when config-`noCheck` disables projects — the condition
  now keys off `projects.length` (harvested), not `checking.length` (post-
  filter), so a client whose projects were all config-disabled shows only the
  accurate per-project lines.
- Nit 3: recorded the boundary on the phase comment — it does not detect
  shell-level failure suppression (`… || true`, `; exit 0`).
- Nit 5: the file-coverage remediation now leads with "for a co-located test,
  move it to `__tests__/`" (adding it to the src `include` would reintroduce the
  build emit) before the general add-to-include advice.

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

* chore(scripts): round-13 — implicit-tsconfig + derived clients; diagnostics

Responds to the round-13 @claude review of #1799:

- Finding 1: verify-typecheck-coverage now understands a `tsc` command with no
  `-p`/`-b` project flag — it resolves the implicit `./tsconfig.json` (tsc's own
  default), so the idiomatic `tsc --noEmit` form counts as coverage instead of
  producing false "in no tsconfig project" failures. Also harvests `-b`/`--build`
  project paths. Verified `tsc --noEmit && tsc --noEmit -p tsconfig.test.json`
  now reports OK.
- Finding 2: CLIENTS is no longer hardcoded — it's derived from the `cd
  clients/<x> && npm run validate` entries in the root `validate` chain (minus an
  explicit web exclusion), so a new Node client is auto-required, matching the
  repo-wide reach of its sibling verify-format-coverage. Added a
  guard-would-check-nothing bail if the derivation yields an empty set. Verified
  a new `clients/newclient` is picked up and its uncovered file flagged.
- Nit 3: the phase-2 remediation now leads with the general add-to-`include`
  advice and only appends the "move it to __tests__/" note when a failing path
  is actually a test file (was misdirecting plain files).
- Nit 4: AGENTS.md no longer says the test config includes "only" __tests__
  (launcher's also includes its vitest.config.ts).

(Two different web integration tests each flaked once under parallel load during
the coverage runs — both pass in isolation and on a clean re-run; timing-
sensitive and unrelated to these script/doc-only changes.)

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

* chore(scripts): round-14 — enumerate clients from disk (fail-closed)

Responds to the round-14 @claude review of #1799:

- Finding 1: replace the root-chain `CLIENTS` derivation (round 13) with a
  disk enumeration — every `clients/<name>` whose package.json has a `typecheck`
  script. The chain derivation was fail-open: a root-chain edit that kept
  running a client but didn't match the strict regex silently dropped it from
  the required set (112→53 files, still green). Disk is fail-closed — a client
  can't be silently dropped, a new client with a typecheck is auto-required, and
  web (no typecheck script; builds via `tsc -b`) is excluded on its own, so the
  explicit exclusion is gone. The existing `rootRunsClientValidate` wiring check
  still verifies the root chain runs each — now LOUD when it doesn't. Verified:
  `cd ./clients/tui && …` now hard-fails the integrity phase instead of silently
  shrinking coverage; `cd clients/tui && npm run build && npm run validate` stays
  gated. Removed the now-unused `clientsRunByRootValidate` from the shared lib.
- Nits 2 & 3: documented the two unreachable-today limitations on
  typecheckProjects — a solution-style `tsc -b` (empty `files` + `references`)
  would under-report, and the implicit-tsconfig fallback assumes no file operands.
- Nit 4: refreshed the file header to mention `-b`/`--build` + the implicit
  `./tsconfig.json` (only the JSDoc had been updated).

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

* chore(scripts): round-15 — enroll-or-exempt enumeration; normalize `cd ./`

Responds to the round-15 @claude review of #1799:

- Finding 1: the disk enumeration keyed on the presence of a script named
  `typecheck`, so renaming it silently dropped the client (112→53, still green)
  — contradicting the docstring. Now every `clients/*` with a readable
  package.json must EITHER declare a `typecheck` (→ enrolled) OR be in an
  explicit EXEMPT map (`clients/web`, reason: `tsc -b`); anything else is a hard
  gate-integrity failure. This is fail-closed on the rename axis too, and web's
  skip is a stated decision rather than an accident of which scripts it declares
  — so web gaining a `typecheck` no longer auto-enrolls it into the solution-`-b`
  false failures. Verified: renaming tui's `typecheck` now hard-fails; a `tsc -b`
  on web stays exempt.
- Nit 3: `rootRunsClientValidate` normalizes a leading `./`, so
  `cd ./clients/tui && npm run validate` (which genuinely runs it) is no longer a
  false "no longer runs" alarm.
- Nit 2: README/AGENTS/script-header prose no longer names a fixed
  cli/tui/launcher scope — they describe the disk-discovered "every clients/*
  with a typecheck script (web exempt)" rule.

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

* chore(scripts): round-16 — anchor wiring match; close enumeration axes

Responds to the round-16 @claude review of #1799:

- Finding 1 (only false-PASS): rootRunsClientValidate matched the client dir as
  a bare substring, so a prefix-sibling (`clients/tui` vs `clients/tui-next`)
  satisfied the wiring check for the shorter name — dropping a client from the
  root chain went green if a sibling was present. Now an anchored regex (escaped
  dir + optional leading `./` + a trailing boundary). Verified the sibling case
  now hard-fails.
- Nit 2: a `clients/*` dir holding tracked TS but no readable package.json is
  now a gate-integrity failure (was treated as "not a client"), so the
  "fail-closed on every axis" claim holds. Verified.
- Nit 3: readdirSync is wrapped, so a missing `clients/` dir yields the intended
  "found no clients/* dir" message rather than a raw ENOENT stack.
- Nit 4: EXEMPT's reason is now surfaced in the success line ("clients/web
  exempt: typechecks via tsc -b …"), so the exemption is visible at runtime, not
  only in source.

(Also moved `isRequiredSource` above nodeClients — nit 2's new tracked-TS check
calls trackedSourceFiles at module-init, which referenced it before init.)

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

* chore(scripts): round-17 — share quote-aware tokenizer between guards

Responds to the round-17 @claude review of #1799:

- Finding 1: the two coverage guards parsed shell strings differently —
  verify-format-coverage had a quote-aware tokenize(); verify-typecheck-coverage
  used a raw regex. So a quoted `-p "tsconfig.test.json"` (a form tsc accepts)
  made the typecheck guard report every file that project covers as uncovered,
  and a quoted `cd "clients/tui"` in the root chain tripped a false wiring
  failure. Moved tokenize() (now single+double quote) into the shared lib,
  imported it into both guards, rewrote typecheckProjects to walk tokens
  (`-p`/`--project`/`-b`/`--build` → next token), and strip quotes before the
  `cd` match in rootRunsClientValidate. Verified both quoted forms now pass.
- Nit 2: a stale EXEMPT key (a dir that no longer exists) was asserted in the
  success line unchecked. nodeClients now fails if an EXEMPT key isn't a real
  `clients/*` directory. Verified.
- Nit 3 (build/dist filter belt-and-braces) left as a recorded boundary.

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

* chore(scripts): round-18 — match tsc by basename; strip quotes on both halves

Responds to the round-18 @claude review of #1799:

- Finding 1 (regression from round 17): `tokens.includes("tsc")` didn't match a
  path-invoked binary (`node_modules/.bin/tsc`), which the previous `\btsc\b`
  regex did — so such a `typecheck` was falsely reported as naming no project.
  Now matches on the token basename (`isTsc`), keeping round-17's quoting fix
  while restoring the tolerance. Verified a `node_modules/.bin/tsc` typecheck
  reports OK.
- Nit 2: `rootRunsClientValidate` stripped quotes for the `cd` match but tested
  `npm run validate` against the unstripped string, so `npm run "validate"`
  false-failed. Both halves now test the stripped string.
- Nits 3 & 4: documented on typecheckProjects — the contrived `--noCheck false`
  (checking on) is still treated as disabling, and the `&&`/`||`/`;` split runs
  before tokenizing (a quoted operator inside an arg would split mid-token,
  unreachable for project paths).

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

* test: round-19 — typecheck test-servers' unimported bin; --prefix wiring form

Responds to the round-19 @claude review of #1799:

- Finding 1: `test-servers/src/server-composable.ts` (a `#!/usr/bin/env node`
  bin entry nothing imports) got no tsc pass — the barrel alias covers the other
  13 test-servers/src files transitively, but not this one, and its own build
  runs `tsc --noCheck`, so its errors were emitted into `test-servers/build`
  unchecked. Added it explicitly to `clients/cli/tsconfig.test.json`'s include
  (zero source changes; verified a bad line in it now fails cli's typecheck).
  Softened the AGENTS.md clause that implied the whole test-server source is
  transitively covered.
- Nit 2: `rootRunsClientValidate` now also recognizes the
  `npm --prefix <dir> run validate` invocation form (was `cd`-only).
- Nit 3 (stderr spy drops the completion-callback overload): left as-is —
  latent, not live (no exercised path awaits a stderr drain), and adding typed
  overload params would reintroduce the write-overload complexity the round-1
  vi.spyOn refactor removed.

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

* chore(scripts): round-20 — anchor validate tail; guard shared-source class

Responds to the round-20 @claude review of #1799:

- Finding 1 (false PASS): the wiring matcher's tail `/\brun validate\b/` also
  matched `run validate:fast` (`:` is a word boundary) — a DIFFERENT script that
  may skip typecheck. So the root chain could run a client's `validate:fast`
  while the guard kept measuring its `validate` (unrun), green. Anchored the
  tail to a boundary (`/\brun validate(?=$|[\s&;])/`). Verified `validate:fast`
  now hard-fails.
- Finding 2: durably close the test-servers/src class instead of point-fixing.
  Added a SHARED_ROOTS check — every tracked TS under `test-servers/src` + the
  root `vitest.shared.mts` (the `format:check:shared`/`lint:shared` surface) must
  land in the GLOBAL union of client projects. So a NEW unimported bin entry is
  caught (not just server-composable.ts, named in round 19). Verified a fresh
  `server-orphan-bin.ts` now fails. Zero source changes — the union already
  covers 15/15.
- Nit 4: the integrity message and rootRunsClientValidate docstring now mention
  the `npm --prefix` form too.

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

* chore(scripts): round-21 — deny-by-default non-client coverage

Responds to the round-21 @claude review of #1799 (findings 1 & 2, one fix):

- Replaced the `SHARED_ROOTS` allowlist with a deny-by-default non-client set:
  all tracked TS minus `clients/*` (per-client pass) minus a reasoned `core/`
  exemption (covered by web's `tsc -b`). This closes BOTH round-21 findings at
  once:
  - Finding 1 (stale allowlist fail-open): a rename of `test-servers/src` no
    longer silently drops it — its files reappear in the required "other" set
    and hard-fail (verified: `git mv test-servers/src test-servers/source` now
    fails instead of going 127→113 green). The subtractive `core/` exemption is
    fail-closed on staleness for the same reason.
  - Finding 2 (allowlist vs deny-by-default): a new top-level TS location is now
    auto-required (verified `tools/probe.ts` is flagged), matching the repo-wide
    reach of the sibling verify-format-coverage.
- Nit 3: added the shared-source clause to the README table row (AGENTS got it
  last round).

Header comment updated to describe the non-client coverage.

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

* chore(scripts): round-22 — narrow core/ exemption to what web's tsc -b covers

Responds to the round-22 @claude review of #1799:

- Finding 1: the `core/` exemption was whole-tree, but web's `tsc -b` only
  covers `core/**/*.ts` MINUS co-located `*.test.ts(x)` (its app project's
  include). So a `core/*.tsx`/`*.mts`/`*.cts` or a co-located `core` test got no
  tsc pass from anything. Narrowed the exemption to exactly that shape
  (`isExemptCoreFile`): a core `.ts` that isn't a co-located test. So a core
  `.tsx`/`.mts`/co-located test is now required in some project. Zero-fix today
  (0 such files); verified `core/mcp/probe-widget.tsx` and a co-located core
  `.test.ts` are now flagged.
- Nit 2: `EXEMPT_ROOTS` is now a Map (dir → reason) and its reason is surfaced
  in the success line alongside the client exemption.
- Nit 3: the non-client miss message is now the general "in no client's tsconfig
  project"; the `test-servers` remediation clause is conditional on an actual
  `test-servers/` path (tracked via `nonClientMisses`, not string-sniffing the
  composed message).
- Nit 4: success line says "(N clients + non-client)".

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

* chore(scripts): round-23 — core exemption mirrors both web projects

Responds to the round-23 @claude review of #1799:

- Finding 1: round 22's `isExemptCoreFile` mirrored only web's app project, but
  web's `tsc -b` also builds `tsconfig.test.json`, which includes
  `core/**/__tests__/**/*.{ts,tsx}`. So a `core/**/__tests__/*.test.ts` (a live
  layout — `core/mcp/__tests__/fakeInspectorClient.ts` already sits there) was
  flagged though web checks it. Now mirrors the UNION: under `__tests__/` →
  exempt `.ts`/`.tsx`; otherwise `.ts` minus co-located `*.test.ts(x)`. Verified
  a core `__tests__` test now passes while a core `.tsx` outside `__tests__`
  stays required.
- Finding 2: `EXEMPT_ROOTS` was a Map whose keys nothing read (round 22 replaced
  its filter with hardcoded `isExemptCoreFile`) — a phantom registry. Collapsed
  to a single `CORE_EXEMPT_REASON` const used in the OK line.
- Nit 3: README/AGENTS scope descriptions updated (core is no longer wholesale
  excluded; the non-client set is deny-by-default).

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

* chore(scripts): round-24 — enroll clients/web via tsconfig references

Responds to the round-24 @claude review of #1799:

- Finding 1: `clients/web` was whole-tree EXEMPT, but web's `tsc -b` only reaches
  what its four referenced projects include — a file at web's package root
  (e.g. `playwright.config.ts`) got no tsc pass and passed EVERY gate, fully
  silently. This was the last whole-tree exemption wider than the gate it
  defers to. Now web is ENROLLED: a client with no `typecheck` script but a
  `tsconfig.json` `references` array is measured through those reference
  projects (`tsc -p <ref> --listFilesOnly`), and is wired iff its `validate`
  runs `tsc -b` (`validateRunsTscBuild`). EXEMPT is now the empty escape hatch.
  Verified: a web-root `playwright.config.ts` with a type error is now flagged,
  and dropping `tsc -b` from web's build hard-fails the integrity phase.
  Zero-fix on the tree — the union of web's 4 projects covers all 625 tracked
  web files.
- Nit 2: assert `core/` exists (the OK line unconditionally states its
  exemption) — parity with the EXEMPT stale-key check.
- Nit 3: a `core/` miss now gets core-specific remediation (widen web's
  `tsconfig.app.json` include, not a cli/tui project).
- Nit 4: README/AGENTS scope descriptions corrected — web is enrolled (not
  exempt), and the exact core-exempt shape (app `*.ts` minus co-located tests +
  test `__tests__/**/*.{ts,tsx}`).

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

* chore(scripts): round-25 — drop redundant core exemption; guard tsc -b --noCheck

Responds to the round-25 @claude review of #1799:

- Finding 1: enrolling web (round 24) made the hand-maintained `core/` exemption
  both redundant and fail-open — web's measured `tsc -b` projects already cover
  core, and the mirror could go stale (narrowing web's include let an unimported
  core error pass). Deleted `isExemptCoreFile`, `CORE_EXEMPT_REASON`, the core
  presence assertion, and the exemptNote core clause. `core/` is now required
  like everything else and covered by measurement (127→894 files; a `core/*.tsx`
  web doesn't reach is now caught by measurement, not a hand-model). Kept the
  core-specific remediation clause.
- Finding 2: the reference (`tsc -b`) path skipped the non-inertness check — a
  `tsc -b --noCheck` build gated nothing while the guard stayed green. Turned
  `validateRunsTscBuild` into `tscBuildStatus` (ok/neutered/none); a neutered
  `tsc -b` now hard-fails, matching the typecheck-script path. Factored the
  disabling-flag predicate (`isDisablingFlag`) so both paths share it.
- Nit 3: `clientTsconfigReferences` now strips block comments too (every other
  tsconfig in the repo uses `/* */`).
- Nit 5: README/AGENTS wiring clause covers web's `tsc -b` (not just the
  `typecheck` script), and core is described as measured, not exempt.

Nit 4 resolves itself: EXEMPT is empty and the exemptNote/ternary now collapse
cleanly. Nit 6 (tsc -b bound to a specific tsconfig) left as recorded.

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

* chore(scripts): round-26 — expand solution configs wherever harvested

Responds to the round-26 @claude review of #1799:

- Finding 1: the reference (`tsc -b`) path was selected only by "no typecheck
  script", so adding `typecheck: "tsc -b"` to web (the natural uniformity move)
  flipped it to the typecheck-script path, where the harvested solution config
  lists nothing → 665 false failures. Fixed at the measurement layer:
  projectFiles now expands a solution config (`{"files":[], "references":[…]}`
  — lists nothing under --listFilesOnly) to its references and measures those
  recursively, so a `tsc -b` project is measured no matter which path harvests
  it. Generalized the references reader (tsconfigReferences(path)). Verified web
  with `typecheck: "tsc -b"` now passes. Dropped the stale "web … is out of
  scope" JSDoc clause.
- Nit 2: a `core/**/__tests__/**` miss now points at `tsconfig.test.json` (its
  actual owner), not `tsconfig.app.json`.
- Nit 4: AGENTS.md's one-line summary now mentions the non-client (`core/`,
  `test-servers/src`) coverage, matching the detailed bullet.

Nit 3 (EXEMPT empty, dormant stale-key check) left as the intended escape hatch.

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

* chore(scripts): round-27 — non-inertness follows references on both paths

Responds to the round-27 @claude review of #1799:

- Finding 1: round 26 made COVERAGE follow a solution config's references, but
  the non-inertness (`noCheck`) check on the typecheck-script path still only
  inspected the harvested solution (empty compilerOptions) — so a `noCheck` in
  a REFERENCED project was invisible there (caught only on the reference path).
  Factored a shared `resolveLeafProjects` (a solution → its real leaf projects,
  cached rawProjectFiles) used by BOTH coverage (`projectFiles`) and a shared
  `checkingLeaves` disable-check, so a `noCheck` in a referenced project is now
  caught on either path. Verified: `typecheck: "tsc -b"` + `noCheck` in
  tsconfig.app.json now hard-fails (was green). Caching also cut the guard from
  ~20s to ~11s.
- Nit 2: directory-form references (`{ "path": "./packages/a" }`) now resolve to
  `<dir>/tsconfig.json`, so a nested-solution dir reference is expanded.
- Nit 3: reworded the empty-listFiles comment (it's "solution config OR errored
  config", both of which the reference expansion handles).
- Nit 4: script header notes a `tsc -b` solution is reduced to its references on
  either enrollment path.

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

* chore(scripts): round-28 — dedup shared leaf in checkingLeaves

Responds to the round-28 @claude review of #1799 (which found no new gate hole):

- Finding 1: `checkingLeaves` dedup'd the `checking` array but ran
  projectDisablesChecking + integrity.push on every visit, so a leaf reachable
  from two harvested projects produced a duplicate `noCheck` line (and a
  redundant `tsc --showConfig`). Now tracks visited leaves in a Set and skips an
  already-seen leaf entirely. Verified a duplicate harvested project yields one
  line, not two.
- Nit 3 (record-completeness): noted in AGENTS that the one tier the guard
  structurally can't see — a per-file `// @ts-nocheck` — is owned by
  `@typescript-eslint/ban-ts-comment` across lint:core/lint:shared/each client.

Nit 2 (the ~11s perf claim isn't reproducible on a CI-class runner) and nit 4
(hybrid-solution / composite boundaries, both tsc-rejected → unreachable) left
as recorded.

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

* test(scripts): unit-test the typecheck-coverage guard's parsers

Responds to the round-29 @claude review of #1799 (no new gate hole; the finding
is that the guard itself has no tests):

- Made the guard's pure helpers importable: exported `typecheckProjects`,
  `tscBuildStatus`, `tsconfigReferences`, `clientTsconfigReferences`,
  `resolveLeafProjects`, `isTsc`, `isDisablingFlag`, `isRequiredSource`, and
  moved the module-init + phase execution behind an exported `main()` run only
  when the file is executed directly (importing it for tests no longer runs the
  guard). Behavior when run directly is unchanged (894 files).
- Added table-driven tests (node's built-in `node --test`; the root has no
  vitest harness by design) for the shared lib (`tokenize`, `reachableScripts`
  incl. pre/post hooks, `rootRunsClientValidate` across cd/--prefix/quoted/
  prefix-sibling/`validate:fast`, `rootReachesScript`) and the guard's parsers
  (`isRequiredSource`, `isTsc`, `isDisablingFlag`, `typecheckProjects`,
  `tscBuildStatus`) — one case per rule a review round found (r7/r10/r13/r15/
  r16/r17/r18/r19/r20/r25). 12 tests. Wired `test:scripts` into `validate`.
- Nit 2: a `scripts/*.ts` miss now gets a scripts-specific remediation (keep it
  `.mjs`, or give `scripts/` its own tsconfig project).
- Documented the guard's test suite in AGENTS.md.

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

* test(scripts): make test:scripts self-guarding; fill parser test gaps

Responds to the round-30 @claude review of #1799 (mutation testing killed 5/5;
findings are about the new test gate + suite completeness):

- Finding 1: `test:scripts` lacked the two properties the guard enforces for
  every tsc pass. verify-typecheck-coverage now asserts, in phase 1, that
  `test:scripts` is reachable from the root `validate` (via the already-imported
  rootReachesScript) AND that its tracked `scripts/**/*.{test,spec}.*` file set
  is non-empty and every file is matched by the command's glob (a new exported
  `globToRegExp`). So dropping it from validate, or renaming a test to
  `*.spec.mjs`/`*.test.mts` (which node --test silently skips), now hard-fails.
  Verified both.
- Nit 2: extracted a pure `parseTsconfigReferences(raw)` from tsconfigReferences
  and table-tested the JSONC tolerance (block/line comments, trailing comma, no
  references, malformed, no path) — the r17/r26 rules had no cover.
- Nit 3: added `-b`/`--build`/bare-`tsc -b` and explicit `--project` cases to
  typecheckProjects's test so its title matches its table.
- Nit 4: rewrote the rootRunsClientValidate test to put `cmd` in a script
  reached from validate, and added the reachability cases (an orphan cd-validate
  must NOT count; validate:tui indirection must) — the r5 property.
- Nit 5: added test:scripts to AGENTS's validate ordering + the README table.

15 guard tests now.

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

* chore(scripts): round-31 — glob harvest follows delegation; brace/? globs

Round-31 review findings, both false failures in the new `test:scripts`
self-guard:

1. The glob half read `rootPkg.scripts["test:scripts"]` literally while the
   wiring half two lines above used `reachableScripts`, so a delegating
   `test:scripts` (`npm run test:scripts:lib && npm run test:scripts:guard`)
   — which genuinely runs all 15 tests — reported every tracked test file as
   "not matched by the glob", advising a rename the files already satisfy.
   Now harvested across every script reachable FROM `test:scripts`, the same
   fix round 6 applied to the `typecheck` harvest; `pretest:scripts` counts
   for free.

2. `globToRegExp` escaped `{`/`}` as literals and left `?` as a regex
   quantifier, both divergent from `node --test`'s glob. `*.{test,spec}.mjs`
   is the natural widening here (the guard's own file scan already treats a
   `.spec.` file as a test) and false-failed; `?` silently meant "optional
   previous char" and threw outright when segment-leading. Adds `?` → `[^/]`
   and a brace-alternation pass, with an unbalanced brace falling back to a
   literal.

Nit 3: `resolveLeafProjects`'s two path-resolution rules (the r26
directory-form reference, and refs resolving against the referring config's
dir) extracted as `projectConfigFile`/`refToProject` so they are one-row
tables like the other parsers, rather than untestable behind a `tsc` spawn.

Nit 4: AGENTS.md + README document that the guard now enforces `test:scripts`
on three axes (wired, non-empty, glob-covered).

Verified: probes C and D exit 0; probe B (rename to `*.spec.mjs`) still
caught; `test:scripts` 18/18; `verify:typecheck-coverage` OK 894 files;
`verify:format-coverage` OK 919; `npm run ci` green.

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

* chore(scripts): round-32 — measure globs with node's own matcher

Round-32 review; both findings point at deleting code.

1. `globToRegExp` hand-modeled `node --test`'s glob and was still one form
   short — character classes (`*[.]test.mjs`, `*.test.[mc]js`) hit the escape
   set and became literals, so the guard false-failed on files node genuinely
   runs. Replaced the whole translator (and its brace balance pre-scan) with
   node's `path.matchesGlob` — 22.5.0+, and the repo floor is >=22.19.0. Same
   *measure, don't model* move round 25 made when it deleted `isExemptCoreFile`
   in favor of measuring web's projects; being exact for every form node
   accepts is the point, since node's matcher is the thing being modeled.

2. The round-31 glob harvest was inline in `main()`, so mutation-reverting it
   to the literal single-script read left the suite green — the commit's
   headline fix was its one untested change. Extracted as `testScriptGlobs`,
   with rows for the direct form, the delegating form (both child globs, not
   `npm`/`run`/`<name>`), a `pretest:scripts` hook, and an unreachable script
   contributing nothing.

Nit 3: `resolveLeafProjects`' JSDoc moved back onto it (the extraction left it
stranded above `projectConfigFile`, which had two stacked doc blocks).

Nit 4a: a `test:scripts` naming no path/glob harvested nothing, and `.some()`
over an empty list blamed every test file with unfollowable advice. Now one
explicit message naming the real problem.

Verified: probes for the direct, delegating, brace, `[.]` and `[mc]` forms all
exit 0 (the last two were false failures before); probe B (rename to
`*.spec.mjs`) still caught; bare `node --test` reports the new single message;
`test:scripts` 18/18; `verify:typecheck-coverage` OK 894; `npm run ci` green.

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

* chore(scripts): round-33 — scope the glob harvest to `node --test` segments

1. False pass. `testScriptGlobs` kept every non-flag token from every reachable
   script, so a glob belonging to a NEIGHBOURING command was attributed to the
   test runner — and since `.some()` needs only one glob to match, a broader
   one suppressed a real miss rather than adding an inert pattern. A
   `pretest:scripts` of `prettier --check "scripts/**/*.mjs"` (reachable via
   the pre-hook round 31 added) was enough: the probe-B rename to `*.spec.mjs`
   then passed while `node --test` silently ran 12 of 18 tests. Now scoped per
   command segment to the ones invoking `--test`, exactly as
   `typecheckProjects` gates on `tokens.some(isTsc)`. The JSDoc paragraph
   claiming a surplus token can't suppress a miss is corrected — it was false.

2. The gate-integrity block is extracted as `testScriptProblems(scripts,
   testFiles)`, pure and outside `main()`. The nit-4a branch added last round
   was the third consecutive commit whose newest branch sat outside the test
   seam and mutation-survived; extracting the block ends the pattern rather
   than pinning one more condition from the outside.

Verified. The finding-1 probe (prettier pre-hook + `*.spec.mjs` rename) now
exits 1 where it exited 0; the hook alone still exits 0. Mutation testing over
the whole helper — empty-harvest branch to `if (false)`, segment gate removed,
wiring axis removed, non-empty axis removed, segment split removed,
`reachableScripts` to `Object.keys`, `matchesTestGlob` to equality — kills
7/7, including the branch that survived the last three rounds.
`test:scripts` 21/21 · `verify:typecheck-coverage` OK 894 ·
`verify:format-coverage` OK 919 · `npm run ci` green.

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

* chore(scripts): round-34 — harvest only positional args; normalize `./`

Both findings are the axis round 33 fixed, one level in.

1. False pass. The segment gate stopped a neighbouring COMMAND's glob, but
   within a kept segment the filter was still "any non-flag token", so a
   glob-valued flag's VALUE was harvested as a positional arg.
   `--test-coverage-include "scripts/**/*.mjs"` is broader than the test glob
   and matches a renamed `*.spec.mjs`, so it vouched for the very file the
   runner skips — and it isn't hypothetical: `scripts/` sits outside the ≥90
   coverage gate, so that flag is the obvious next thing added here. Tokens
   preceded by a value-taking `--test-*` flag are now dropped.

2. False failure. `./scripts/**/*.test.mjs` — which `node --test` accepts, and
   the common npm-script idiom — matched nothing, because `git ls-files` emits
   no `./`. Both files were then blamed for a rename that never happened, the
   unfollowable-remediation shape fixed for the bare-runner case last round. A
   leading `./` is normalized off, as `rootRunsClientValidate` already does.

Nit 4: the gate-integrity footer's typecheck-wiring advice is now conditioned
on at least one issue being about the typecheck wiring — a `test:scripts`
failure carries its own per-line remediation, and appending `--noCheck`/`tsc
-b` advice under a renamed-test-file message sends the reader after the wrong
thing.

Verified. The coverage-include probe with the rename now exits 1 where it
exited 0, and exits 0 without the rename; the `./` probe exits 0 where it
exited 1; a `test:scripts`-only failure no longer prints the typecheck footer.
Mutation sweep 9/9 killed, including both new rules. `test:scripts` 22/22 ·
`verify:typecheck-coverage` OK 894 · `verify:format-coverage` OK 919 ·
`npm run ci` green.

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

* chore(scripts): round-35 — fourth axis: flags that narrow the run

1. False pass on a new axis. The three axes ask whether the tests are run,
   whether there are any, and whether the runner is told about each — none asks
   what it then DOES with them. `--test-shard 1/2` keeps every glob intact and
   executes 6 of 22 tests, exiting 0, with the guard green; `--test-only` runs
   2, as does a non-matching `--test-name-pattern`. This is `--noCheck` for the
   test runner, one domain over. Adds `NARROWING_TEST_FLAGS` and a fourth axis
   in `testScriptProblems`. Three of those flags were already in
   `VALUE_TAKING_TEST_FLAGS` — the guard knew them well enough to drop their
   values but not that they shrink the run. Both `--flag value` and
   `--flag=value` report as the bare flag name.

2. The footer conditioning added last round was itself inside `main()` — the
   pattern the same commit's comment documented, recurring in the commit that
   documented it. Extracted as `integrityAdvice(problems, nonTypecheck)`.

   Doing that fixes nit 3 in the same move: the old condition keyed off
   `testScriptIssues` specifically, so it covered one of three non-typecheck
   categories. A sibling-vouch failure ("the root validate no longer runs
   verify:format-coverage") and client-enrollment problems still got
   `--noCheck`/`tsc -b` advice about a typecheck pass they have nothing to do
   with. All three sets are now excluded.

Verified. `--test-shard 1/2`, `--test-only` and `--test-name-pattern zzz` each
now exit 1 naming the flag, while node runs 6/2/2 of 22. A sibling-vouch
failure no longer prints the typecheck footer. Mutation sweep 12/12 killed,
including all three new rules and the branch this commit replaces.
`test:scripts` 24/24 · `verify:typecheck-coverage` OK 894 ·
`verify:format-coverage` OK 919 · `npm run ci` green.

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

* chore(scripts): round-36 — discover test files by content, not name

1. False pass on the axis underneath the other four. The required test-file set
   was built with the same name pattern the `test:scripts` glob uses, so a
   rename escaping BOTH was invisible: the file left `testFiles` entirely, so
   nothing asked whether the runner was told about it, and the survivor
   satisfied the non-empty axis. A dot→hyphen slip
   (`npm-scripts.test.mjs` → `npm-scripts-test.mjs`) dropped 6 of 24 tests with
   every gate green — and the file that stops running is the one pinning the
   r16 prefix-sibling and r20 tail anchors, the series' two other false passes.

   Round 30 fixed "renamed to a form the glob misses" by broadening the name
   pattern; a fifth name pattern only moves the boundary again. `node --test`
   decides what a test is by whether it registers tests, so `isScriptsTestFile`
   reads that instead — the same measure-don't-model move as r25 and r32. It
   also covers what no name pattern can: a NEW test file added under a name the
   glob misses, never run and never noticed.

Nit 2: `enrollmentProblems` no longer suppresses the footer. "declares no
`typecheck` script …" IS about the typecheck pass and the footer's first clause
is its canonical fix; excluding it lost the advice for the r15 rename scenario.
The criterion is "is this about the typecheck pass at all", not "does it carry
per-line advice" — several typecheck problems carry their own and still want
the footer. JSDoc corrected accordingly.

Nit 3: the complementary-shard boundary (`1/2` + `2/2` across two reachable
scripts genuinely runs everything, and is flagged anyway) recorded on the
helper, alongside the guard's other documented boundaries.

Nit 4: the three `--test-coverage-{lines,branches,functions}` thresholds added
to VALUE_TAKING_TEST_FLAGS. Their values are numeric and so inert rather than
suppressing, but model completeness is cheaper than re-deriving that later.

Verified. The hyphen rename now exits 1 naming the file; the `.spec.mjs` rename
is still caught; an enrollment failure prints the footer again; the guard is
unchanged at 894 files OK (exactly the 2 tracked test files reference
`node:test`, none of the other 12 `scripts/` files do). Mutation sweep 14/14.
`test:scripts` 25/25 · `verify:format-coverage` OK 919 · `npm run ci` green.

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

* chore(scripts): round-37 — register-not-mention; guard the discovery read

Both findings were introduced by round 36's content predicate.

1. False failure. The predicate matched a bare `node:test` mention, so a shared
   helper importing the runner's `mock` API — registering no tests — was
   enrolled as a required test file and told to rename itself to `*.test.mjs`,
   which would only make node run an empty file. `scripts/lib/` is already the
   repo's shared-helper location, so this is the plausible next file there.
   Tightened to node's actual criterion: an import/require OF `node:test` AND a
   test registration.

2. Regression. `readFileSync` was passed as an argument, so it ran for every
   tracked `scripts/` file before the predicate's extension gate could reject
   it — a file in the index but not the worktree (`rm` without `git rm`) took
   the guard down with a raw ENOENT stack, aborting phase 1 before any real
   finding printed. Discovery did no reads at all before round 36. Extension is
   now gated first and the read wrapped: an unreadable tracked file warns and
   is skipped, the round-6 precedent where a failed `tsc` echoes its diagnostic
   instead of dying.

Verified: the mock-helper probe exits 0; the tracked-but-deleted probe exits 0
with a one-line skip warning instead of a stack; the dot→hyphen rename is still
caught. Mutation sweep over the predicate 3/3 (import form, registration,
extension). `test:scripts` 25/25 · `verify:typecheck-coverage` OK 894 ·
`verify:format-coverage` OK 919 · `npm run ci` green.

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

* chore(scripts): round-37 nit 3 — one definition of the JS-extension check

Finding 2's fix hoisted the extension gate into `main()` (it must run before
the read), leaving a second copy of the regex inside the predicate that reads
as protection but can never fire there. Extracted as `isJsFile` and used in
both positions, so the two can't drift.

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

---------

Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
 2026-07-27 12:05:35 -04:00
Cliff Hall
c2af547cc8
 		fix(core): register peer-request handlers before connect (#1797) (#1798) 
* fix(core): register peer-request handlers before connect (#1797)

The roots/sampling/elicitation capabilities are advertised on the SDK
`Client` at construction, so from the moment `connect()` sends
`notifications/initialized` the server may issue any of those requests.
The handlers, however, were registered after the handshake resolved and
after `fetchServerInfo()` — leaving a window in which the SDK answered
`-32601 Method not found`.

`server-filesystem` asks for `roots/list` the instant it is initialized
(that is how it learns its allowed directories) and lost the race, so it
silently fell back to its CLI-argument directories instead of the roots
configured in the Inspector. `server-everything` asks later and did not.

Extract the four peer-request handler blocks into
`registerPeerRequestHandlers()` and call it before `client.connect()`.
None of them depend on server capabilities — only on constructor-set
state. The notification handlers that do gate on `this.capabilities` stay
in `connect()`, after `initialize`.

Regression test drives the race deterministically with a fake transport
that delivers `roots/list` synchronously from inside the `send()` of
`notifications/initialized`; it fails on the pre-fix ordering.

Closes #1797

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

* review: round-1 fixes — pre-connect roots/list_changed, wider timing test (#1797)

Finding 1: `notifications/roots/list_changed` was the one handler in the
post-handshake block that gates on nothing but constructor state, so it
sat in the same race — a server emitting it right after `initialized` had
it silently dropped (no wire error; the symptom is a missed UI refresh).
Moved to a new `registerPeerNotificationHandlers()` called next to
`registerPeerRequestHandlers()` before `connect()`.

Finding 2: the timing test asserted one of the four moved handler blocks.
It now injects `tasks/list` and the `roots/list_changed` notification in
the same burst as `roots/list`, so moving any of the block back after
`connect()` fails the test. Sampling/elicitation stay out — they park a
pending request awaiting user input and have no reply to assert on.
Dropped the unused `sessionId`/`setProtocolVersion` transport fields.

Verified the added assertions are load-bearing: the notification
assertion fails against the previous commit, the `roots/list` one against
`origin/v2/main`.

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

* review: round-2 fixes — correct the roots/list_changed premise (#1797)

The round-1 doc comment claimed a server may emit
`notifications/roots/list_changed` at initialize. It can't, normally:
`RootsListChangedNotificationSchema` is a member of
`ClientNotificationSchema` and absent from `ServerNotificationSchema` —
the client sends it (setRoots does, at :4251). The inbound handler is
defensive coverage for a non-conformant server, and dispatches
`rootsChange` with our own already-known roots. Reworded the method doc
and the test's header so neither states the false premise; the move
itself stands (it gates on no server capability, so it belongs with the
request handlers).

Also from review:
- Dropped the vacuous `notificationRejected` flag and its inverted
  comment. The SDK does not throw for an unregistered notification, so
  the try/catch could never observe one — confirmed by bisect, where the
  pre-fix failure was always `rootsChanges`, never the flag. Removing the
  catch also stops it swallowing a genuine throw from the injected
  requests.
- Narrowed `replies` to responses (`"result" in message || "error" in
  message`) so it can't also collect client-originated requests.
- Annotated the unreachable `if (!this.client)` guard with a justified
  `v8 ignore` per the AGENTS.md dead-guard policy.

Re-verified both regression directions: the `roots/list` assertion fails
against `origin/v2/main`, the `rootsChange` one against `55e25b7c`.

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

* review: round-3 fix — drop stale comments inside the roots handler (#1797)

The four inline comments moved verbatim with the handler contradicted the
method doc reworded one round earlier: they said "server's roots" (they
are ours — it is a ClientNotification) and "we'll just dispatch an empty
array", which is not what `this.roots || []` does when roots are
configured. The new timing test asserts exactly that non-empty dispatch.
Replaced with one accurate line. Comment-only.

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

* fix(cli): advertise roots so roots/set is actually serviceable (#1797)

Round-4 review found the same -32601 symptom on a second path: the CLI
constructed its InspectorClient with no `roots` option, so
`capabilities.roots` was never advertised and no `roots/list` handler was
registered — yet `--method roots/set` calls `setRoots()`, which announces
`notifications/roots/list_changed` to the server. A server taking up that
invitation got "Method not found", after the CLI reported success.

The review's suggested fix — register `roots/list` unconditionally — is
not implementable: the SDK asserts the client capability inside
`setRequestHandler` ("Client does not support roots capability"), so it
throws during connect for any client built without the option, and
`registerCapabilities` refuses to run after connect. A client that omits
`roots` therefore cannot legally serve `roots/list` at all.

Fixed at the layer that can fix it: `cli.ts` seeds `roots: []`, matching
web (`App.tsx` always passes it). The core registration stays gated on
the constructor value, now with the SDK constraint documented so the next
reader doesn't retry the unconditional version.

`setRoots()`'s doc claimed it "will enable roots capability if it wasn't
already enabled" — it cannot, for the reasons above. Corrected, and
dropped the dead `if (this.roots === undefined) this.roots = []` the very
next line overwrote.

Tests: a CLI test drives `list_roots` over an HTTP test server (verified
load-bearing — without the seed it fails on the -32601 error text), and
the core timing test gains a case pinning that roots set after connect
are served with the current values, not the constructor's.

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

* fix(cli,tui): advertise the roots configured in mcp.json (#1797)

Round-5 review: seeding `roots: []` fixed the -32601 but not the outcome.
`server-filesystem` discards an empty roots answer (`validatedRootDirs
.length > 0`) and keeps its CLI-argument directories — which is the
symptom this PR set out to fix. The user's roots were already in hand two
lines away, on `serverSettings.roots` (lifted from mcp.json by
`mcpConfigToServerEntries`). Now passed through `cleanRoots`, the same
shared helper web uses, so both clients answer `roots/list` byte-identically
for the same config.

This matters most on the CLI because it is one-shot: `--method roots/set`
sets roots on a connection torn down a few statements later, so the config
file is the only durable way to give a run its roots.

Same gap in the TUI (`App.tsx` built its options with no `roots` key at
all), fixed the same way. The review suggested a follow-up issue for it;
folding it in here instead, since it is the identical line and splitting
it would leave a known-wrong client in tree.

Also from review: hoisted `waiters` up with the other fields, and
`injectRequest` now rejects after 1s naming the method and id rather than
hanging to the vitest timeout.

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

* review: round-6 fixes — document roots in the CLI README, harden cleanRoots (#1797)

- clients/cli/README.md enumerated the settings lifted from a catalog/config
  file without roots. Roots are the least discoverable member of that list —
  there is no flag, and `--method roots/set` dies with the connection — so
  the file is the only durable way to supply them. Said so.
- cleanRoots() trusted its input's shape. `Root[]` is a compile-time type over
  hand-editable mcp.json, and as of this PR all three clients feed it straight
  from disk, so `"roots": [{"name":"Work"}]` or `"roots": "file:///work"` threw
  at connect. Non-array bails to []; an entry without a string uri is dropped
  with a warning. Hardened in the one shared place rather than three call
  sites. Review suggested a follow-up issue; folded in for the same reason as
  the TUI fix last round.
- Test comment said the CLI passes a `roots: []` seed — true one commit ago.
- injectRequest clears its timeout on reply (an armed timer outliving a test is
  the #1760 teardown class), and setRoots' doc drops the "despite what this
  comment used to claim" archaeology.

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

* review: round-7 fix — cleanRoots also throws on a non-string name (#1797)

The round-6 hardening guarded `uri` and left `name` throwing one line
later: `r.name?.trim()` guards null/undefined, not "is a string", so
`"roots": [{"uri":"file:///a","name":42}]` still died at connect — the
same crash class, one field over, reachable from all three clients and
the web settings-save path. Reproduced against the shipped function body
before fixing.

A bad `name` now costs the name, not the root (it is optional, and the
uri is still usable), with a warning. The doc no longer overstates what
is validated.

Also from review: the CLI test comment said "`cli.ts` now seeds
`roots: []`" — true only of that ad-hoc test's path; and the
registration comment's `(:588)` line reference is now a durable name.

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

* fix(core): normalize roots in setRoots() too (#1797)

Round-8 review: this PR made cleanRoots the single normalizer for roots at
connect and at save, but setRoots — the third way roots enter the client —
did a bare `this.roots = [...roots]`. Web cleans at its call site; the CLI
does not, so `--method roots/set --roots-json '[{"name":"Work"}]'` stored a
root with no uri, advertised it in the roots/list reply, and echoed it back
as success. cleanRoots is idempotent, so calling it inside setRoots covers
the CLI without disturbing web's pre-clean, and all three entry points now
agree.

Review filed this as an optional follow-up; folding it in for the same
reason as the TUI and cleanRoots items — it is smaller than the issue
describing it.

Test asserts the malformed entry is dropped and the valid one survives;
verified load-bearing against the bare-spread version.

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

* review: round-9 fixes — normalize roots in the constructor too (#1797)

The round-8 commit claimed "all three entry points now agree"; review
pointed out the constructor is a fourth, and the only one still raw. All
three clients clean at their call site, so nothing was broken — but the
invariant was enforced by convention rather than by the class, at the very
entry point this whole thread started from. `this.roots` now runs through
cleanRoots, with a ternary so the `undefined`-vs-`[]` distinction that
gates capabilities.roots survives.

Also from review:
- The CLI roots/set test provoked a cleanRoots warning without suppressing
  it (AGENTS.md). Suppressed, and the spy now asserts the drop was
  reported rather than silent.
- `enables roots when previously undefined …` in coverage-backfill was
  named after behaviour round 5 established does not exist. Renamed, with
  a comment on what that client actually can and cannot do.
- setRoots dispatches a copy to rootsChange, as getRoots() already did, so
  a listener can't push into the list we advertise.
- run-method.test.ts's connectStdio now passes `roots: []` like cli.ts, so
  the helper matches the client shape production builds.

Verified load-bearing: the constructor test fails against the raw
assignment. validate / coverage / build-gate / smoke / storybook all green
(run as separate stages — full `npm run ci` kept being killed mid-coverage
by the environment).

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

* review: round-10 fixes — copy at both rootsChange dispatches (#1797)

Round-9 claimed setRoots' copy closed the "a listener can't push into the
list we advertise" hole. It closed one of two sites: my reply conflated
the notification handler's *dispatch* with the roots/list *request*
handler — the latter is serialized to the wire immediately and is fine,
the former hands listeners the internal array. Both copy now.

Also from review: dropped the `({}) as never` in the new constructor test
for the real factory the sibling tests use (this test never connects, so
it is never invoked), and the needless `async` on it.

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

* review: round-11 doc fixes; merge v2/main (#1797)

- cleanRoots' opening sentence still framed it as the settings-form
  normalizer, contradicting the paragraph below it that says every client
  feeds it from disk. It has four callers now; says so.
- Marked the two runtime guards `Keep:` with the reason. The `Root[]`
  parameter narrows both to `never`, so "the compiler says this can't
  happen" is the argument someone will use to delete the checks rounds 6-7
  installed. The reason now lives where the deletion would happen.
- Noted the constructor-normalization case in the timing test's header, so
  it is discoverable from a file otherwise about the initialized race.

Also merged origin/v2/main (#1796, loopback binding), which had advanced
by one commit and touched the smoke scripts.

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

* fix(core): clear queued peer requests when connect() fails (#1797)

Round-12 review found a second-order consequence of the handler move.
Before it, registration happened after every await in connect(), so the
moment a peer request could be queued the connect could no longer fail —
queue and failure path never overlapped. Registering before the handshake
puts both the handshake and the post-connect `setLoggingLevel` inside that
window.

The catch sets status to error and drops the transport but never cleared
pendingSamples/pendingElicitations; only disconnect() does, and connect()
reaches it solely on the connect-timeout path (not the default). So a
server that elicits at `initialized` and then fails the connect left a
live pending-request modal on a dead connection — web derives it from the
queue lengths with no status gate — and the elicitation's cancel() never
fired, hanging any awaitUrlElicitation waiter.

Extracted disconnect()'s clear-and-cancel into clearPendingPeerRequests()
and called it from the catch, dispatching the change events so the UI
drops the modal. CLI/TUI are unaffected either way (sample/elicit off).

Test note: the first version of this test passed with and without the fix
— its `requestedSchema` omitted `properties`, so the SDK rejected the
inbound params and nothing was ever queued. Fixed the fixture; it now
fails without the change with a queued ElicitationCreateMessage.

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

* fix(core): clear the peer-request queue on a mid-session crash too (#1797)

Round-13 review, two items.

The test asserted only `getPendingElicitations()`/`getPendingSamples()`,
which read the arrays directly — but the arrays are not what removes the
modal. `usePendingClientRequests` tracks its own state off the
pendingSamplesChange/pendingElicitationsChange events, so a regression that
cleared without dispatching would strand a live modal on a dead connection
and stay green. Both tests now assert the events fired with an empty
payload; verified by deleting just the dispatches, which fails them.

The connect-failure fix left the other way a connection ends uncovered:
a mid-session crash goes through the transport's `onclose`, which sets
status and fires `disconnect` but never touched the queues. Same two
hazards, and arguably more reachable. `clearAndAnnouncePendingPeerRequests`
wraps the clear plus its events, guarded on a non-empty queue so the paths
that overlap (a connect failure whose dropCachedTransport also fires
onclose) announce once; called from both. New test queues an elicitation,
fires onclose, and asserts the queue and the event — fails without the
change.

Review flagged the crash path as a follow-up; folding it in, as with the
earlier out-of-scope items — it is the same seam and three lines.

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

* review: round-14 fixes — make both teardown paths agree (#1797)

- The crash path's comment claimed "cleared before the `disconnect` event
  so a consumer sees an empty queue" while disconnect() cleared *after*
  its own dispatch — an invariant stated as a property and held at one of
  two sites, the same shape as the round-10 finding. Moved disconnect()'s
  clear above its status/disconnect block so both paths agree; its change
  events stay batched with the other teardown dispatches. Full gate green,
  so nothing depended on the old ordering.
- clearPendingPeerRequests' doc still named the `connect()` failure path as
  a caller; that path now goes through clearAndAnnouncePendingPeerRequests.
  Names the two real callers.
- The mid-session test waited on `await Promise.resolve()` — the
  microtask-counting pattern this file replaced with injectRequest two
  rounds ago, working today only because the enqueue happens exactly one
  tick deep. Waits on the client's own `newPendingElicitation` instead.
- The file header still called the constructor case "the last case"; it is
  third of five, and the two teardown cases went undescribed. Rewritten.
- Restored the missing blank line between two tests, and noted why the
  sampling-event assertion holds for a queue that never held anything.

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

* review: round-15 fixes — scope the parity claim to the arrays (#1797)

Round-14 made the two teardown paths agree on when the queue is cleared,
and the comment then claimed parity flatly. It holds for the arrays but
not the events: the crash path announces before `disconnect`, while
disconnect() batches its change events with the rest of its teardown, so
they land just after. Since the events are what drives the modal, the
distinction matters to the next reader. Scoped the comment rather than
un-batching — un-batching would trade a documented ordering for a real
behaviour change with no gain.

Same rule as round 14, one level down: check the other site, then check
the other axis.

Also gave the new `newPendingElicitation` wait the timeout that
`injectRequest` documents, so a regression that stops the enqueue fails at
the wait instead of hanging to the vitest timeout.

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

* test: pin disconnect()'s clear-before-announce ordering (#1797)

Round-14 moved clearPendingPeerRequests() above disconnect()'s status
block so a `disconnect` consumer sees an empty queue on both teardown
paths, and round 15 documented it — but nothing enforced it. The existing
`disconnect` listeners only count events, and the two teardown tests cover
the crash and connect-failure paths, neither of which goes through
disconnect(). So the one behavioural change in the stack was untested in
both directions, and the tidy that undoes it (regrouping the clear with
the batched change events) would have stayed green.

Test records the queue length from inside a `disconnect` listener; the
fixture's close() never fires onclose, so it exercises disconnect()'s own
ordering rather than the crash path. Verified against the pre-round-14
position: `expected 1 to be +0`.

Folded the newPendingElicitation wait into a shared helper now that two
tests use it.

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

* test: pin disconnect()'s announce as well as its clear (#1797)

Round-16 pinned the array half of disconnect()'s teardown and left the
events unpinned — and nothing else in the suite covers them, so deleting
disconnect()'s two change-event dispatches would have left a live modal
after a user-initiated disconnect with the suite green. That is the path
most likely to have a modal open when it runs. Asserted after the await
rather than inside the disconnect listener, since disconnect() batches the
events deliberately; verified by deleting the dispatches (`expected
undefined to be +0`).

Header rewritten by category rather than position: it has now gone stale
twice in three rounds because it counted tests ("the last two") and
enumerated paths that a new test falsified. It names the three teardown
paths and what distinguishes them instead.

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

* review: round-18 doc fixes — scope the three-path claim, re-attribute cancel() (#1797)

The header I rewrote last round claimed all three teardown paths clear
before announcing. True only if "announcing" means the `disconnect` event:
the connect-failure path emits none at all, and both it and the crash path
clear after their `statusChange`. Scoped to what's true. Same shape as
rounds 10/14/15 — a parity asserted in prose that holds on one of the axes
it could mean — which is now the fourth time, in a sentence I wrote.

ElicitationCreateMessage.cancel()'s doc still credited `disconnect()`
teardown; this PR made clearPendingPeerRequests() its only caller, serving
three paths. Re-attributed, and promoted the no-`onRemove` note to its own
paragraph with the consequence spelled out — it quietly became a
three-caller invariant during this PR, and "don't splice mid-iteration"
reads like a style note until you know it would skip every other entry and
leave those promises unsettled.

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

* fix(core): gate roots/list on what was advertised, not on this.roots (#1797)

Round-19 review found a latent wedge, reproduced before fixing: a client
constructed without `roots` that later calls setRoots() has this.roots
defined, so registerPeerRequestHandlers tried to register the handler on
every subsequent connect() — and the SDK throws "Client does not support
roots capability" from setRequestHandler when the capability was never
advertised. That throw lands before the handshake, so the client can never
reconnect. Worse than the -32601 this PR started from: the connection is
gone rather than answering one method badly.

Not a regression (the same gate lived after the handshake before) and
unreachable from the three shipped clients, which all pass roots. But
core/ is a library surface, and the failure is unrecoverable.

Gate now reads a construction-time `rootsCapabilityAdvertised` — the thing
the SDK actually asserts on — so this.roots stays free to change. setRoots'
doc says what does happen on such a client: the roots are stored and
readable, but no server can ask for them.

Also scoped the header's claim that the connect-failure path emits no
`disconnect`: true of connect()'s own catch, but on a real transport
dropCachedTransport's close() fires onclose first, which does dispatch one
— making the catch's own call the auth-recovery backstop.

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

* review: round-20 — derive the roots gate from the advertisement itself (#1797)

Round-19's fix restored the invariant but not the derivation:
rootsCapabilityAdvertised was computed from `options.roots` alongside the
capability block rather than from it, so the two encodings of "did we
advertise roots" could drift. Adding any term to the advertisement
condition would leave the flag true with `capabilities.roots` absent —
the same unrecoverable wedge, reintroduced silently and in the throwing
direction.

Now read off the built capability object after it is assembled, so
divergence is structurally impossible rather than merely absent. Verified
the reconnect test still fails against the old `this.roots` gate.

Also: named the capability-lifetime category in the test header (the new
test was none of the three it listed), moved that test below the teardown
group so the group stays contiguous, and rewrapped the over-long comment
line.

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

* fix(core): gate elicitation/create on what was advertised too (#1797)

Round-21 review asked round-20's question of the capability next door, and
there the two derivations already disagreed — no future edit needed.
`elicit` is typed `boolean | { form?, url? }`, so `{}` or
`{ form: false, url: false }` is valid, enables no mode, and advertises no
`capabilities.elicitation` — while the registration gated on `this.elicit`
being truthy. Reproduced before fixing:

  CONNECT ERROR: Client does not support elicitation capability
                 (required for elicitation/create)

Since #1797 moved registration before the handshake, that throw means the
client cannot connect at all. Same wedge as the roots one, one capability
over, and reachable through a documented option value rather than a future
edit.

Gated on a new elicitationCapabilityAdvertised, derived from the built
capability object exactly as the roots flag now is. Test constructs with
`{ form: false, url: false }` and asserts connect() resolves; verified
load-bearing.

Also made the four construction-time capability inputs (sample, elicit,
receiverTasks, advertisedExtensions) `readonly`. The round-19 bug was a
mutator making a construction-time fact stale, so a future setter for any
of these should be a compile error rather than a review catch.

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

* fix(core): derive tasks.requests and the remaining gates from the advertisement (#1797)

capabilities.tasks.requests declares which server→client requests we accept
as tasks, but was built from `receiverTasks` alone — so
`{ receiverTasks: true, elicit: false }` told the server it could send a
task-augmented elicitation/create while advertising no elicitation
capability and (since the last commit) registering no handler. A server
taking the invitation gets -32601: the advertise-then-refuse shape this PR
opened with, and the last place it survived. Now built from
capabilities.sampling / capabilities.elicitation, both decided just above,
with the key omitted entirely if neither applies.

Also made the remaining three registration gates read the built capability
object like the two from the last two rounds — sampling, tasks/*, and the
URL-elicitation completion handler. None could diverge today, but a mixed
convention means the next reader has to re-derive which gates are safe, and
adding one condition to an advertisement block is exactly how elicitation
broke. Five gates, one rule now.

Test asserts a receiverTasks+elicit:false client advertises tasks.requests
with sampling and without elicitation; verified load-bearing.

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

* review: round-23 — pin the omitted-requests arm, drop the count again (#1797)

The `requests` key is omitted when neither sampling nor elicitation is
advertised, but every `receiverTasks: true` construction in the tree left
one of them enabled, so that arm was unexercised — and it would not have
shown as a coverage failure, since one uncovered branch in a ~4900-line
file sits well inside the per-file gate. Second client in the same test
asserts `tasks` survives (list/cancel are still serviceable) while
`requests` is absent rather than empty; verified against `requests:
taskRequests`.

The header regained a numeral four commits after round 17 rewrote it by
category to stop exactly that, and it was already stale. Dropped, and named
the third category — advertise-only-what-you-serve, which is about the
capability object rather than a registration gate.

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

* fix(core): settle queued samples on teardown instead of discarding them (#1797)

Round-24 review. Round 13 recorded the samples-vs-elicitations asymmetry
as deliberate on two grounds: nothing internal awaits a sample, and the
transport is gone by then. The first still holds; the second is false on
one path, and it is a path this PR created. `connect()`'s catch drops the
transport only when `!transportHasAuthProvider`, so on the HTTP+OAuth path
— web's ordinary authenticated case — it is retained and the caller
reconnects over it. A sampling request queued during the handshake was
then discarded with the connection still live: no response frame ever
written, and the server waits forever for a reply to a request we
accepted. Accept-then-silence, which is worse for the peer than the
-32601 this PR opened with.

SamplingCreateMessage.cancel() mirrors the elicitation one — settles
(rejects, what the UI's decline path already sends and the SDK turns into
an error response) without calling onRemove, since the caller iterates the
queue and clears it. clearPendingPeerRequests loops it like the
elicitations.

Also corrected two comments that assumed the catch always tore the
transport down, and noted that the retention is gated on
transportHasAuthProvider alone — independently of
isConnectAuthRecoveryError, which gates only the status hold.

Test asserts settlement rather than a wire frame, since on the disconnect()
path the transport is closed before the settle; a later respond() throwing
"already resolved or rejected" is the proof. Verified load-bearing.

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

* fix(core): mark receiver-task payload promises handled (#1797)

Round-25 review. clearPendingPeerRequests iterates every entry in
pendingSamples, and two shapes live there: the plain server-request one,
whose reject reaches the SDK handler, and the task-augmented one, whose
reject reaches the receiver task's payloadPromise. That promise only gets
a consumer when the server polls tasks/result, so a task sitting in
input_required has no handler attached — and the settle added last commit
surfaced as an unhandled rejection.

Not hypothetical: the repo already worked around it test-side, with a
helper that reaches into private state to attach no-op catches, whose
comment describes this exact race. Fixed at the source instead — one
`void payloadPromise.catch(() => {})` at creation, which also covers the
pre-existing cancelReceiverTask path. Removed the now-redundant helper and
its two call sites. Verified: dropping the source line makes that suite
report two unhandled rejections.

Corrected SamplingCreateMessage.cancel()'s doc — its "no response frame is
ever written" rationale is true of the plain shape only; a task-augmented
sample already answered the wire with a CreateTaskResult, so settling it is
about not leaving the task in input_required limbo.

Nits: generalized the enqueue wait helper to both events so the sampling
wait gets the timeout the elicitation one has; moved the sampling teardown
test back into the contiguous teardown group; named the settle property in
the header; fixed the fixture doc that still claimed to record replies.

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

* review: round-26 — drop the comment that outlived its call (#1797)

The suppressReceiverPayloadRejections call went last commit; its two-line
comment stayed, describing a pre-attach that no longer happens and
crediting the test for handling a rejection the source now handles.
Removed. Also reworded the surviving .catch() in inspectorClient.test.ts:
it is still needed, but as a value capture for the assertion, not for
unhandled-rejection suppression.

Rewrapped the teardown paragraph in the test header — my round-25 edit
left a 104-char line, and two hand-fixes left it ragged, so it is reflowed
in one pass.

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

* review: round-27 — skip the roots/list_changed the SDK would refuse (#1797)

Review reported setRoots() emitting roots/list_changed on a client that
never advertised roots, inviting a re-fetch we'd answer -32601. Probed it:
the premise is wrong. The SDK asserts the notification capability too —
`notification()` rejects with "Client does not support roots list changed
notifications" and nothing reaches the wire. No spec violation and no
invitation; what actually happened is that setRoots provoked that
rejection and logged it as "Failed to send", implying a transient failure
where the truth is structural.

Kept the early return on that corrected basis: it stops us provoking a
rejection we know will come, and warns accurately instead. Doc and comment
say the SDK is the enforcer rather than claiming this guard is what keeps
the wire clean.

The test pins the end state (no invitation for roots we can't serve) and
passes either way — recorded as such rather than presented as a regression
guard, since it isn't one.

Also moved the advertise-category test below the two gate cases, matching
the order the header introduces them.

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

* review: round-28 — gate the notification on the predicate the SDK asserts (#1797)

The roots/list_changed guard read `capabilities.roots !== undefined`, but
the SDK asserts `capabilities.roots.listChanged` for the notification —
wider than the flag. Identical today (roots are only ever advertised as
`{ listChanged: true }`), but a conditional advertisement would un-gate it
silently, which is the round-21 shape again. Split into
rootsListChangedCapabilityAdvertised, mirroring
urlElicitationCapabilityAdvertised, which already reads its assertion's
sub-field; rootsCapabilityAdvertised keeps the roots/list registration,
where presence is what setRequestHandler checks.

Also moved the new test out of the gate pair it had split — it is a
runtime-emission case, not a registration-gate one — and extended the
header's converse-category paragraph to cover notifications, which indexes
it without inventing a category. Reflowed the setRoots doc paragraph in one
pass (111-char line from the last commit, plus a ragged orphan from an
earlier edit).

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

* review: round-29 — chase the corrected premise into clients/cli (#1797)

Round 27 corrected "setRoots announces roots/list_changed to a server that
then gets -32601" — the SDK refuses the notification from a client that
never declared it, so nothing reached the wire. That reword happened in
core/, but the same premise had been written into the two comments
justifying the CLI's roots seed, and they still asserted it. Both now state
the real pre-fix failure: a server asking for roots on its own got -32601,
and roots/set could not announce at all.

Also fixed the warn message under the round-28 gate: it named the roots
capability while the gate is on roots.listChanged — precisely the pair that
split was made to distinguish, so a client advertising `{}` would have been
told it advertised nothing.

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

* fix(core): reject in-flight raw-wire requests on a mid-session crash (#1797)

rejectPendingRawWireRequests had one caller — disconnect() — so the crash
path settled the inbound peer-request queue this PR added and left the
outbound raw-wire map alone. That map holds the modern tasks/* frames the
SDK's era gate refuses to route, so the SDK's chained _onclose doesn't know
about them: a Tasks-tab poll in flight when the server died waited out its
own 30s timeout and reported a timeout for what was a crash, on a
connection that had already dispatched `disconnect`.

Pre-existing — both the map and the crash path predate this PR. Fixing it
here because the invariant it violates is one this PR wrote down: "every
path that ends a connection has to clear that queue, announce it, and
settle each entry", in clearAndAnnouncePendingPeerRequests' doc and the
timing test's header. One channel was true of one path out of three.

Test starts a raw-wire request and fires onclose; without the call it
hangs to the vitest timeout, which is the symptom.

Checked the two siblings and left both: taskInputAbortControllers is
already unblocked via the elicitation cancel the crash path performs, and
activeToolCallAbortController rides an ordinary client.request the SDK
settles itself.

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

* review: round-31 test fixes — justify the cast, drop the no-op tick (#1797)

- The new test's `as unknown as` had no justification, which AGENTS.md
  explicitly prohibits. It is unavoidable — rawWireRequest is private and
  every public route is gated on isTasksExtensionNegotiated(), which needs
  a modern handshake the fixture doesn't perform — so that reasoning is now
  written down instead of left to be re-derived.
- `await Promise.resolve()` was a no-op: rawWireRequest registers its entry
  in the promise executor, synchronously, before any suspension. Dropped,
  which also makes the test pin that ordering rather than paper over it —
  and stops the third recurrence of the microtask-counting pattern this
  file's own helper doc argues against.
- Moved the test into the teardown group and extended the header's teardown
  paragraph to cover the outbound direction, which it framed entirely as
  inbound.

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

* review: round-32 — separate the outbound teardown claim from the inbound one (#1797)

My round-31 header edit spliced the outbound raw-wire passage into the
middle of the inbound claim, so "There are three: a failed connect(), a
mid-session transport close, and an explicit disconnect()" ended up reading
as scoping the outbound settle — which is two by design, since nothing
populates the map before the handshake. Round 31 established that scoping
so the code wouldn't imply an impossible state; the prose then un-scoped
it. Split into its own paragraph with its own scope clause, and reflowed
both in one pass rather than nudging breaks.

Also raced the raw-wire assertion, for the reason the file's own wait
helper documents: without the teardown rejection the test waited out the
request timeout and reported a bare vitest timeout. It now fails with
"raw-wire request not settled by teardown" — verified.

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

* review: round-33 — extract the timeout race, mark the failure-path promise handled (#1797)

Three copies of "reject after 1s so a regression fails where it happened"
had accumulated, each re-explaining itself. Extracted `withTimeout`, used
by the enqueue wait and the raw-wire assertion; injectRequest keeps its own
since its timeout also deletes the waiter entry.

On the failing path the raw-wire test left the production 30s timer armed,
so a regressed run got the intended failure plus a stray unhandled
rejection up to 30s later — the armed-timer class the file's own comment
names, on the failure path. `settled` is marked handled before the race.
Verified: with the fix removed the run fails with "raw-wire request not
settled by teardown" and no unhandled rejection.

Recorded why disconnect() drains the SDK's in-flight requests but rejects
the raw-wire map outright: the user asked to disconnect, and a Tasks poll
is not worth delaying that for. Pre-existing and deliberate, now written
down where the asymmetry is visible.

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

* review: round-34 — drop a no-op guard and the false mechanism it asserted (#1797)

Round 33 claimed the raw-wire test could leave `settled` rejecting
unhandled on a regression run, and round 33's fix encoded that as
`void settled.catch(() => {})` with a comment explaining it. Both were
wrong: `Promise.race` attaches a rejection handler to every input, so
`withTimeout(settled, …)` on the next line already marks it handled.
Verified with a standalone script — no unhandledRejection fires, and the
pre-fix version was equally safe.

Removed the line and the comment. The comment mattered more than the line:
it told the next reader that `Promise.race` does not mark its losers
handled, which is false and would cost them elsewhere.

Kept the true half by setting `timeout: 50` on that test's client, so a
regression leaves no 30s timer armed and fails with `Raw request
"tasks/get" timed out after 50 ms` — more self-describing than the raced
generic message. Verified.

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

* review: round-35 — the drain comment described a trade-off nobody made (#1797)

Last round's comment said raw-wire requests "get no share of the drain
above: the user asked to disconnect, and a Tasks poll is not worth delaying
that for" — framing a decision. Verified it isn't one: `safeDisconnectTimeout`
defaults to 0 and the only caller anywhere that passes a value is a single
test, so nothing is drained in production; and the drain polls the SDK's
`_responseHandlers`, which never holds raw-wire ids, so those were never
eligible regardless.

Reworded to what is true. The framing came from my own round-33 reply,
which described the drain as running on every disconnect — a comment
faithfully encoding a half-read mechanism, which is the same failure as the
`Promise.race` one it followed.

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

* review: round-36 — the race comment outlived the mechanism its own commit replaced (#1797)

`timeout: 50` made the request settle in ~50ms, so the 1s race can never
win and a regression fails on the assertion (`timed out after 50 ms` vs the
expected /Connection closed/) — which the commit message said, while the
comment beside it still credited the race. Reworded: the short timeout is
what fails fast and names itself, the race is the backstop for a promise
that never settles at all. Race kept; it covers the case where the
request's own timer is broken too.

Third comment in three rounds encoding a mechanism the same commit changed.

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

* review: round-37 — scope the timeout comment, align the bypass guards (#1797)

`timeout: 50` is the client-wide request timeout, not a bound on the
raw-wire request; it is inert in that test only because the test issues no
SDK request after connect. Noted, so someone adding one later knows it
would inherit a 50ms budget rather than reading the comment and looking
elsewhere.

The two installReceiverTaskResponseBypass guards read `this.receiverTasks`
while the five gates around them read the advertisement. Installing the
bypass mutates the Protocol's _requestHandlers map, so it is registration —
same as the setRequestHandler it follows. Equivalent today; aligned so the
convention is uniform rather than something the next reader has to
re-derive per site. The genuinely-runtime `receiverTasks` reads are left.

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

* review: round-38 — one predicate for the whole bypass mechanism (#1797)

Round 37 moved the bypass *install* guard to the advertisement and left the
three branches it controls on the raw option, splitting one immutable fact
across two sources. The dead direction is the harmful one: if the tasks
advertisement ever acquires a condition — the exact edit that produced the
elicitation wedge — then advertised goes false while receiverTasks stays
true, the bypass is not installed, and the handler still returns
`{ task }` into the SDK's validating wrapper, which rejects it. That is the
breakage the bypass exists to prevent, and before round 37 it could not
happen because install and branch read the same field.

All three now read tasksCapabilityAdvertised. `receiverTasks` is left where
it belongs: assigned once, and driving the advertisement.

Also noted that the wrapper's own check is redundant with its install guard
and kept deliberately, since it must agree with the handler branch.

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

* fix(core): don't carry receiver tasks into the next session (#1797)

Round-39 review found the last member of the enumeration rounds 12-14 and
31 built: receiverTaskRecords was cleared only by disconnect(). The crash
path doesn't clear it, the connect() failure catch doesn't, and connect()
never reset it — so a record created in one session survived into the next
connect() on the same instance, and `tasks/list` is answered from that map.
A new server would be told about a task it never created.

Reachable in web (the only client with receiverTasks): a task-augmented
sampling/createMessage arriving in the handshake window creates a record;
if that connect then fails with a 401, App.tsx retries connect() on the
same instance.

Cleared at the top of connect() rather than only on the crash path — the
auth-recovery retry means ending a session isn't the only way a new one
begins, and starting clean covers every route in. disconnect() keeps its
clear via the same extracted helper.

Pre-existing — the crash path never cleared these — but the handshake
window this PR opened is what lets a receiver task exist before connect
resolves, and this was the one member left outside the invariant the PR
documented. Test verified load-bearing.

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

* fix(core): reset all session-scoped state on connect, not just receiver tasks (#1797)

Round-40 review: receiverTaskRecords was one of five collections cleared
only by disconnect(). Two of the other four have real symptoms on the same
crash-then-reconnect path:

- subscribedResources — the modern subscribeToResource early-returns on a
  uri already in the set, so after a reconnect the user's Subscribe click
  silently sends nothing to the new server while the UI keeps listing it.
- cancelledTaskIds — a leftover id mislabels a *new* task sharing it as
  cancelled rather than failed, and server task ids are often short.

taskInputAbortControllers leaked un-aborted controllers.

modernLogLevel had the opposite asymmetry: disconnect() cleared it so it
couldn't leak (#1629), but nothing restored it, so a reconnect after an
explicit disconnect silently lost the user's configured level while a
reconnect after a crash kept a mid-session override. It is now recomputed
from serverSettings the way the constructor does, so both routes agree.

clearReceiverTasks became resetSessionState, called from the same place.

Tests cover the two symptomatic siblings and now assert receiver tasks
through the `tasks/list` handler — what a server actually sees, and the
symptom itself — which also removed the unjustified `as unknown as` the
previous commit added. The remaining cast is justified in place, since
cancelledTaskIds has no public reader.

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

* fix(web): re-seed the Logs control when the client re-seeds its own level (#1797)

Round-41 review caught a consequence of the last commit. `resetSessionState`
restores the client's modern log level from the server setting at connect;
`resetSessionScopedUiState` still blanked the web control to null on every
disconnect. On the one path that reconnects the *same* instance —
auth recovery — the control then read Off while every modern request carried
`_meta` logLevel "debug", the default. A tool whose job is showing what you
send should not disagree with itself about what it sent.

The control now re-seeds from the active server's settings exactly as
setupClientForServer does, instead of blanking.

Also from review:
- Added the tests the restore never had, covering both halves: a mid-session
  override does not survive a reconnect, and a disconnect no longer drops the
  configured level (the #1629 half, and the actual behaviour change).
- resetSessionState's doc claimed "every piece of state scoped to one
  connection", which invites assuming membership rather than checking it.
  Now states the rule: state a new session would misread; what only needs
  settling on the way out stays with the teardown paths.
- Header: session-scoping paragraph moved below the outbound one to match
  test order, and it now names all five members rather than three.

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

* refactor(core): one derivation for the modern log level (#1797)

Round-42 review: the configured level was computed independently in four
places — twice in InspectorClient (constructor, resetSessionState) and twice
in App.tsx (the seed and the re-seed). The client and the Logs control
agreed only because four sites happened to compute the same expression, and
any drift reproduces exactly the symptom the previous commit fixed: the
control reading one level while every modern request stamps another.

Extracted resolveModernLogLevel into core/mcp/types.ts beside
DEFAULT_MODERN_LOG_LEVEL; all four call sites use it, with the web mapping
undefined→null at its own boundary (display, not a second derivation).

Also from review:
- disconnect()'s comment still credited itself with preventing the #1629
  leak, which resetSessionState now prevents. It says what the line does
  now — read "not opted in" while disconnected — and notes the web control
  deliberately shows the configured level in that window.
- resetSessionState's doc claimed an un-aborted taskInputAbortControllers
  entry "leaks a paused poll loop". Both registration sites release in a
  finally, so nothing leaks permanently; the abort closes the window
  between a crash and the unwind. Softened, and it now has the test it
  lacked — the only member of the five without one.
- Noted in App.tsx that the re-seed depends on activeServerIdRef still
  holding the outgoing id (it syncs in a passive effect), so an eager clear
  would silently drop it to the default.

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

* fix(core): reset the subscription stream state with the set it derives from (#1797)

Round-43 review: resetSessionState cleared subscribedResources but left
modernStreamState, so a crash-then-reconnect on the same instance held an
empty set alongside `{ active: true }` — a combination the rest of the file
treats as impossible, since onModernSubscriptionClosed and
onModernReconnectFailed both compute `active` from `subscribedResources.size
> 0`, and the comment at :4712 asserts a terminal drop clears both. No
symptom today only because the UI mirror resets itself independently, in
another file — which is the arrangement this PR has spent its rounds
removing. Clearing it here also announces the reset; that set's every other
mutation dispatches.

Also from review:
- The web re-seed treated "no server found" as the default level rather
  than Off, because resolveModernLogLevel returns the default for absent
  *settings*. Reachable via a second disconnect after a crash, and via
  removing the active server. Now distinguishes the two.
- Converged the two injectRequest copies: withTimeout takes an optional
  onTimeout, so both drop their waiter on the timeout path. The newer copy
  had lost exactly the cleanup that was round 33's reason for leaving the
  original alone.

Test seeds the stream state a live subscription would have left; verified
load-bearing (the earlier version passed either way, since the fixture
never activates a stream).

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

* fix(web,core): correct the re-seed branch; share the subscription-stream reset (#1797)

Round-44 review. My round-43 fix traded one wrong branch for another: it
read `?.settings`, which is undefined both when no server matched *and*
when the server has no settings node — and the second is the common case
(a hand-written mcp.json entry never opened in Server Settings). Those
servers showed Debug while connected and Off after disconnect, which is
exactly the client/UI disagreement rounds 41-42 removed, with no
modernLogLevelChange event to reconcile it. Now branches on the server.

Worth noting no test would have caught it: App.tsx is a documented
exclusion from the coverage gate.

Also from review:
- resetSessionState cleared subscribedResources without announcing it,
  two lines below a comment arguing that every other mutation of that set
  dispatches. And it took two statements of a six-statement cluster
  disconnect() clears together — leaving modernReconnectAttempts (a new
  session's first drop would back off as if it were the old one's nth),
  modernListenGeneration (the bump that makes an in-flight re-listen bail),
  and a stale modernSubscription. One resetSubscriptionStream helper now
  serves both, announcing both axes; disconnect() keeps only the live
  stream's best-effort close. That also removed the file's one assignment
  to modernStreamState that bypassed its setter.
- The stream-state test pinned the value, not the dispatch — the half the
  UI actually tracks. Now asserts both; verified against a bare field
  assignment.
- onModernSubscriptionClosed's comment claimed the disconnect reset always
  clears the set; on a crash it doesn't until the next connect. Says so.

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

* review: round-45 — reattach the orphaned doc, order the announcements (#1797)

Extracting resetSubscriptionStream last round inserted it *and its doc*
between resetSessionState's doc and resetSessionState — so two doc blocks
stacked, the new helper took the nearest, and the 23-line block explaining
each member's symptom attached to nothing. Dead text where it sat, and the
most load-bearing comment in the batch. Moved, not reworded.

That is round 32's finding in production code, from the same cause:
inserting anything between a doc block and its method is a move that needs
checking, prose or code.

Also from review:
- resetSubscriptionStream announced the cleared set before updating the
  stream state derived from it, so a listener in between saw an empty set
  with an `active` stream — the combination the helper exists to prevent,
  exposed by its own dispatches. Both fields move first, then both are
  announced. The onModernSubscriptionClosed comment now says that window
  is never observable, which is newly true.
- The test header called the log level "cleared"; it is re-seeded, which is
  the #1629 half of the test twenty lines below. Says "reset", with the
  distinction spelled out, and the paragraph reflowed in one pass.

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

* fix(core): close the listen stream the connect-path reset drops (#1797)

Round-46 review: resetSubscriptionStream's doc claimed disconnect() is the
only caller with a stream worth closing, and the helper did a bare
`modernSubscription = null`. Not guaranteed on the connect path — an
`onerror` without an `onclose` leaves the transport up, connect() reuses
it, and the reset then drops the last reference to a stream still open on
the server. Nothing can close it afterwards: the `closed` handler bails on
the bumped generation, and `closed` never resolves for a live stream.
Before the extraction the surviving reference was eventually closed by
refreshModernSubscription as `previous`, so this was newly unclosable.

The helper now closes it itself, best-effort, after the dispatches so the
round-45 ordering is unaffected; disconnect() drops its two hand-rolled
lines.

Also from review:
- The session test pinned the subscription set's value but not its
  dispatch — the half the UI tracks, and the gap round 43 closed for the
  stream axis one line away. Both now asserted; verified load-bearing.
- resetSessionState's doc said disconnect() "clears all of this", which is
  true by duplication for two of the five members; it now says a sixth has
  to be added in both places. And "the teardown paths" handle the in-flight
  tool call only on the disconnect route — on a crash it is callTool's own
  finally. Named.

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

* review: round-47 — fix the doc's count, catch the closed-promise chain (#1797)

- resetSessionState's doc said "two members through the same helpers, the
  other two hand-rolled" for a body with five. It is two and three — and
  the member the enumeration dropped is the one that is *paired* rather
  than duplicated: modernLogLevel is re-derived here and blanked in
  disconnect(), deliberately. Both corrected.
- `subscription.closed.then(...)` had no rejection handler, unlike every
  other promise around that stream. Newly reachable: before the last
  commit a connect-path reset dropped the reference with `closed`
  permanently pending, so the chain never settled at all; closing the
  stream is what makes it settle. Added, with the Node-terminates reasoning
  the file already applies to payloadPromise.
- The listen-stream test exercised transport reuse without asserting it —
  the half that makes "still open on the server" true. Counts factory calls
  now, so the premise fails loudly if a future change recreates the
  transport rather than silently ceasing to apply.

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

* review: round-48 — scope the catch, guard the close, index the test (#1797)

- The .catch comment claimed the connect-path close made a *rejecting*
  `closed` reachable. It doesn't: closing resolves it. What that close newly
  reaches is the handler running at all, where the reference used to be
  dropped with `closed` pending forever. And chaining after `.then` also
  swallowed handler throws — silently abandoning a re-listen on the path
  whose job is recovering a dropped stream. Now the second argument to
  `.then`, scoped to the rejection, with the comment saying which half is
  which.
- `closing?.close()` reaches into third-party code from `disconnect()`'s
  straight-line teardown, none of which is inside a `try` — a synchronous
  throw would skip the raw-wire rejects, the task aborts, the receiver-task
  clear and nine dispatches, silently, since most callers catch a
  disconnect(). Best-effort now against both failure modes.
- Header indexes the listen-stream test: same category, but a release
  obligation rather than a misread one.
- Its comment described the generation guard, which the stub can't exercise;
  marked as production behaviour.

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

* review: round-49 — pin the best-effort close, unorphan the doc (#1797)

Round 48 made `resetSubscriptionStream`'s close best-effort against both a
rejected promise and a synchronous throw, but nothing exercised either arm:
both `modernSubscription` stubs in the tree resolve. On the `disconnect()`
path an escaping failure would silently skip the raw-wire rejects, the paused
task-input aborts, the tool-call abort and `clearReceiverTasks()` — the
findings rounds 13 and 30 closed, re-opened at once with a green suite. Two
table-driven cases now seed a `close()` that throws and one that rejects,
and assert teardown continued past it.

Also: the `.then` comment gave opposite verdicts on one mechanism (an
unhandled rejection is named unacceptable, then chosen for a handler throw) —
reworded to say the handler cannot throw today and why the shape is
defensive; and `refreshModernSubscription`'s doc block, orphaned two methods
up since before this branch, moved onto the method it describes.

Closes #1797

* review: round-50 — sweep the queues start-clean, unify the close (#1797)

`resetSessionState`'s doc excluded the peer-request queues and the raw-wire
map because "the teardown paths handle them" — true of the paths that run.
An `onerror` without an `onclose` runs none of them: it flips status to
"error" and leaves `baseTransport` cached, so a `connect()` on the same
instance reuses a live transport. That is the route the round-46 stream close
was built on, and it strands these two the same way — the web pending-request
modal is derived from the queue length with no status gate, so it outlives
the session, and answering it writes a response for the old request id onto
the new connection. Both are now swept start-clean beside the reset (both
helpers are idempotent, so the routes that already ran them are unaffected),
the end-clean clears stay put for the `disconnect`-event consumer, and both
docs say which is which.

Also folds the three subscription `close()` sites onto one
`closeSubscriptionBestEffort` helper, so all three absorb a synchronous throw
as well as a rejection — at `refreshModernSubscription`'s first site the
reference is already dropped, so a throw there both leaks a live stream and
aborts the refresh before its replacement is opened.

Closes #1797

* review: round-51 — pin the sweep's ordering, name the category (#1797)

The connect-start sweep must sit after `resetSessionState()`, and the two
calls read as independent. Cancelling a task-augmented peer request settles
it synchronously into the record callback, which ends in `upsertReceiverTask`
— a no-op only because `clearReceiverTasks()` just emptied the map. Hoisted,
it would emit a `notifications/tasks/status` for the outgoing session's task
onto the transport the connect is about to reuse. Stated at the call and
pinned by a test (which fails on the hoist).

Adding that third caller also made five docs stale, all describing the old
set by count or as "teardown" — the class round 23 diagnosed. All five now
name the category: every route out, plus the top of `connect()` as a backstop
for the one route in that settles nothing.

And the site where round 50's fold actually changed behaviour —
`refreshModernSubscription`'s first close, where the reference is already
dropped and a `.catch()` never caught a synchronous throw — is now tested
from the public surface (a re-list over a poisoned stream), table-driven over
both arms. Verified to fail on the pre-fold form.

Closes #1797

* review: round-52 — say what the sweep prevents, scope the close doc (#1797)

The sweep's comment named as its harm the exact write the sweep performs: on
this route the SDK's transport is still live, so cancelling a queued peer
request emits a response for the old id immediately — which is right, and is
the settle-don't-discard rule, at the best available moment. As written it
read as an argument against emitting the frame, inviting the clear-without-
settling regression rounds 24–25 removed. It now names the real hazard: a
*user*-authored answer landing arbitrarily later, past the re-handshake.

`closeSubscriptionBestEffort`'s "at each site" held at two of three — the
superseded-generation discard stored nothing and has nothing following it,
so its escaping failure would only reject an already-superseded
`subscribeToResource`. Scoped, still wrapped.

Also moves the `disconnect()`-side close pair next to its `connect()`-side
twin, and states which axis the file's ordering follows, since route and
category both describe it and cut differently.

Closes #1797

* review: round-53 — give each close site its real harm, pin it (#1797)

The doc grouped `resetSubscriptionStream`'s close with the "takes the
caller's remaining work with it" sites. It can't: the call is `void`-ed onto
an `async` helper, so even a synchronous `close()` throw becomes a dropped
rejection — `disconnect()`'s teardown runs regardless. The harm there is an
*unhandled* rejection, which this PR already established (the payloadPromise
fix) is the process-fatal class in Node — so the one site with the severe
failure mode was documented as having the mild one. Each site now carries its
own consequence, and the local comment no longer gives two reasons that
cancel each other.

The test had the same gap: both its assertions hold with the helper's
try/catch deleted. It now also asserts no unhandled rejection fired, which
does fail without the guard (verified on both arms); the teardown assertions
stay, since they still catch unwrapping the call to a bare `close()`.

Also names the two back-references the round-52 move left positional, and
softens the "on the wire until dropCachedTransport()" claim — that drop is
conditional and never runs on stdio.

Closes #1797

* review: round-54 — the mildest site isn't mild from every caller (#1797)

`refreshModernSubscription` has three callers, and the superseded-generation
discard's failure reaches all of them. From `unsubscribeFromResource` — which
deliberately keeps the removal when a re-listen fails — it surfaces "Failed
to unsubscribe" for an unsubscribe that actually stuck, which is worse than
the consequence the doc assigned to the site it called mildest. The re-listen
bullet had the mirror gap: on the reconnect caller the escape is absorbed
into the backoff run, not a set left with no stream. Both now name their
callers, and the generation-bump claim is softened to what it proves (a newer
refresh *started*).

Test: the `unhandledRejection` listener is process-wide, so the assertion now
filters to this fixture's own reason rather than failing on a rejection some
other test left pending; and it is registered inside the `try` whose `finally`
removes it, so a throw in the setup can't leak it into the rest of the file.
Re-verified the filtered form still fails on both arms with the helper's
try/catch removed.

Closes #1797

* fix(core): don't roll back a subscribe a newer refresh superseded (#1797)

`subscribeToResource`'s rollback assumes what its comment says — that the
server filter is unchanged — which is exactly what supersession falsifies.
The newer refresh built its filter from the set *including* this URI, so on
success the server honors it; rolling back leaves the set missing a URI the
live stream carries (the UI showing it unsubscribed while its
resources/updated keep arriving) and, if it was the only one, an empty set
with an active stream — the combination `resetSubscriptionStream` exists to
prevent, reached from the other end.

Reachable without any close() failure: a rejecting `listen()` propagates the
same way, which a user subscribing while the reconnect timer re-lists — i.e.
exactly when the server is flaky — can produce. The rollback is now gated on
the generation not having advanced past this call's own bump; the error still
reaches the caller either way. Test drives two overlapping subscribes with
the first `listen()` rejecting, and fails without the gate.

The doc bullet that called this the mild site is corrected with it: both
user-initiated callers report a failure that did not happen, in mirrored
ways. Also gives the two close-failure arms distinct reasons so the
unhandled-rejection filter can exclude the sibling arm as well as a foreign
rejection.

Closes #1797

* fix(core): reconcile the stream badge when a re-listen this call owned fails

The supersession gate left the both-fail case stuck: two overlapping
subscribes whose listens both reject leave the superseded URI in the set (its
rollback correctly skipped) while the state keeps the optimistic
"connecting" — no stream, no reconnect armed (neither onModernSubscriptionClosed
nor onModernReconnectFailed ran), so the badge never changes and no
resources/updated ever arrive. That is the caveat this file already wrote
down for the neighbouring case — the bump proves a newer refresh *started* —
applied to the gate itself.

Fixed in the branch that knows the answer: the call that still owns the
re-listen reconciles the state instead of only handling an emptied set,
reusing the "ended" badge onModernSubscriptionClosed already reports for
"stream gone, URIs still subscribed". unsubscribeFromResource gets the same
treatment — it rolls nothing back by design, but its failed re-listen left
the badge reading Listening over no stream.

Also: the gate's comment named one of the two things that bump the
generation (resetSubscriptionStream also does, from disconnect and the
start-clean reset) and cross-referenced the wrong invariant; both corrected,
and `<=` tightened to `===`, which is what the monotonic synchronous bump
actually guarantees.

Closes #1797

* fix(core): order the subscription writes, retry instead of stranding

Two sites emptied the subscription set and announced it before the stream
state caught up — the last-URI rollback in subscribeToResource's catch, and
(pre-existing) the ordinary last-URI unsubscribe, which dispatches a
round-trip before the re-listen sets INACTIVE. Both exposed an empty set with
an active stream: the pair resetSubscriptionStream orders its own writes to
prevent, and which three comments in this branch now cite. State before the
announce at both, asserted from the consumer's side.

And the reconcile added last round settled for an honest but dead badge: it
was the only route to "stream gone, URIs live" that had made no attempt at
all, over subscriptions the server may never have honored, recoverable only
by an Unsubscribe/Subscribe toggle (a fresh Subscribe early-returns). It now
hands them to the reconnect machinery, which already fits — base delay after
a user-initiated refresh, bails on a terminal status or an emptied set, and
lands on the same "ended" badge past the cap. That also removes the
provably-dead null check the reconcile carried.

Closes #1797

---------

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
 2026-07-27 11:32:23 -04:00
Cliff Hall
cea22d86b8
 		feat(web): enforce localhost-only binding; expand loopback origin allow-list (#1796) 
Closes #1795.

Enforce localhost-only binding in code (the "never 0.0.0.0" rule was only a spec note): refuse an all-interfaces HOST — across every spelling the OS binds as the wildcard (inet_aton decimal/octal/hex, IPv6, IPv4-mapped, zone-scoped, IDNA/Unicode) — unless DANGEROUSLY_BIND_ALL_INTERFACES=true (Docker's sanctioned opt-in). Fixes the reported stdio-connect 403 by expanding the default loopback origin allow-list to all three interchangeable forms.

Hardening surfaced across an extended review: closes an IDNA/fullwidth wildcard bypass, a blank-ALLOWED_ORIGINS fail-open, and the CLI/TUI OAuth-callback wildcard bind (loopback-only now, RFC 8252); canonicalizes the origin allow-list the way a browser does; derives frame-ancestors from the real embedder list (IPv6-literal exclusion verified in headless Chromium); strictPort on all three bind points; and preserves the CLI exit-code map + JSON envelope through the mcp-inspector --cli launcher path. Shared host normalization lives in core/node/hostUrl.ts. Docs + tests + smokes throughout.

🤖 Generated with [Claude Code](https://claude.com/claude-code)
 2026-07-26 22:15:10 -04:00
Bob Dickinson
33e88eac9f
 		CLI: catalog listing, auth flags, and OAuth browser UX (#1782) 
* feat(cli): catalog listing, auth flags, and OAuth browser UX

Add servers/list and servers/show, --relogin / --stored-auth-only, browser
OAuth navigation with OSC 8 links, and extract method handlers under
clients/cli/src/handlers. Tracks #1781.

Co-authored-by: Cursor <cursoragent@cursor.com>

* fix(cli): address #1782 review — relogin clear, stored-auth-only, docs

Clear both raw and normalized OAuth keys on --relogin; check the shared
store before bailing under --stored-auth-only mid-session; restore the
tests README link target; document servers/show redaction limits; keep
mcpi groundwork with TODO(#1432) markers.

Co-authored-by: Cursor <cursoragent@cursor.com>

* fix(cli): gate OAuth browser open and harden #1782 review follow-ups

Arm auto-open only for CLI-owned interactive OAuth, honor MCP_AUTO_OPEN_ENABLED,
move --relogin clear after validation, skip keychain on servers/list, and cover
secret redaction after rehydrate.

Co-authored-by: Cursor <cursoragent@cursor.com>

* fix(cli): silence disarmed OAuth URLs and round-3 review nits

Suppress authorize URL print when navigation is disarmed or
--stored-auth-only; document MCP_AUTO_OPEN_ENABLED; redact Cookie/auth
headers; keep servers/show unit tests off the OS keychain.

Co-authored-by: Cursor <cursoragent@cursor.com>

* test(cli): cover withArmedAutoOpen for the ≥90 branch gate

CI failed on cliOAuth.ts branch coverage after the auto-open arming
helper landed; exercise the armed path through runCliInteractiveOAuth.

Co-authored-by: Cursor <cursoragent@cursor.com>

* fix(cli): mid-RPC unauthorized recovery and clear round-4 nits

Mirror connect's UnauthorizedError path in withCliAuthRecoveryRetry, let
MCP_AUTO_OPEN_ENABLED=true force open off-TTY, mock open-url in the
in-process runner, reject --relogin on catalog methods, restore the
__tests__ README inventory, and narrow OAuth helpers to structural client types.

Co-authored-by: Cursor <cursoragent@cursor.com>

* fix(cli): reconnect after mid-RPC OAuth and finish round-5 nits

Reconnect before retrying after unauthorized recovery, guard non-OAuth
configs, decouple forceAutoOpen, move open-url mock to vitest setupFiles,
and reject --relogin with stored-auth short-circuits.

Co-authored-by: Cursor <cursoragent@cursor.com>

* fix(cli): fail fast on non-TTY interactive OAuth and round-6 nits

Skip interactive OAuth without a TTY unless MCP_AUTO_OPEN_ENABLED=true;
document --stored-auth-only for CI; redact settings.metadata; reject
--relogin for stdio; clarify AuthRecoveryRequired reconnect comments.

Co-authored-by: Cursor <cursoragent@cursor.com>

* fix(cli): admit interactive OAuth when stdin is a TTY (round-7)

Gate on stdin||stderr so `2>&1 | tee` still works; fold confirm/auto-open/
TTY into CliOAuthConnectOptions; rename authRequiredFailure; document
MCP_AUTO_OPEN_ENABLED=true as non-TTY admit.

Co-authored-by: Cursor <cursoragent@cursor.com>

* fix(cli): fail fast on step-up without stdin (round-8)

Decline [y/N] on readline EOF/close, require a stdin TTY for the default
step-up confirmer (force-open admit is not enough), document stderr-only
browser auto-open vs stdin||stderr flow admit, and drop the alias wrapper.

Co-authored-by: Cursor <cursoragent@cursor.com>

* fix(cli): allow piped step-up answers; bound silent pipes (round-9)

Drop the hard stdin-TTY gate that broke `echo y | …`. Keep EOF decline and
add a short non-TTY timeout so an open pipe that never writes cannot hang.
Clarify isTTY as admit-only; extract the readline mock factory.

Co-authored-by: Cursor <cursoragent@cursor.com>

* fix(cli): keep partial piped step-up answers (round-10)

Capture readline `line` before close so `printf y` without a newline is
not treated as decline; timeout is auth_required with a distinct message;
document the 5s bound and post-confirm callback wait; pin the TTY no-timeout arm.

Co-authored-by: Cursor <cursoragent@cursor.com>

* fix(cli): tighten step-up timeout messaging and TTY test (round-11)

Make the TTY no-timeout assertion use a settled flag (the Promise.race
check was vacuous), say "timed out" instead of "declined" on the pipe
bound, and document that piped answers need a newline or stdin close.

Co-authored-by: Cursor <cursoragent@cursor.com>

---------

Co-authored-by: Cursor <cursoragent@cursor.com>
 2026-07-26 16:55:25 -04:00
Cliff Hall
a7b2fdb2ef
 		test(web): settle real-timer Transitions so the coverage gate can't leak 'window is not defined' (#1786) (#1793) 
* test(web): settle real-timer Transitions so the coverage gate can't leak 'window is not defined' (#1786)

The web coverage gate was intermittently failing with an unhandled
`ReferenceError: window is not defined` thrown from a Mantine `Transition`
timer that fired after happy-dom tore down `window` (the #1760 class). The
three `ViewHeader.test.tsx` tests that use `renderWithMantineTransitions`
(real, env="default" timers) each drove their *primary* transition to
completion, but the concurrent *enter* on a sibling cell leaves no unmount to
`waitFor`, so its rAF→setTimeout chain could still be pending at teardown and
fire post-teardown, failing an unrelated later file's run.

Fix: after each such test, flush pending transition timers against the
still-mounted tree via `settleTransitions()` (a wait longer than the 300ms
header duration + rAF slack), so the settling setState targets a live tree and
nothing is left to fire after cleanup unmounts.

Also close the related nit: `renderWithMantine`/`renderWithMantineTransitions`
now accept a `colorScheme` option, so `ViewHeader.test.tsx` no longer
hand-rolls a bare `defaultColorScheme="dark"` MantineProvider (the #1760
anti-pattern). AGENTS.md documents both the option and the settle requirement.

Closes #1786

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

* test(web): address review — export settleTransitions, derive settle from HEADER_ANIM_MS, guard fake timers, correct comments

Review feedback on PR #1793:

1. Derive the settle window from the real animation duration: export
   `HEADER_ANIM_MS` from ViewHeader.tsx and compute
   `TRANSITION_SETTLE_MS = HEADER_ANIM_MS + 200` in the test, so bumping the
   header duration can't silently make the settle insufficient (the invariant
   is now structural, not a comment).

2. Move `settleTransitions` into the shared `renderWithMantine.tsx` helper
   (exported, optional `ms` arg) next to `renderWithMantineTransitions`, since
   AGENTS.md points every future caller at it — shared test scaffolding belongs
   in src/test/, not a single test file. AGENTS.md now references the helper.

3. Guard `settleTransitions` against `vi.useFakeTimers()`: it awaits a real
   setTimeout, so under fake timers it would hang to the test timeout. Throw a
   clear message instead.

4. Correct the diagnosis comments: the hazard isn't a Mantine timer surviving
   unmount (its effect cleanup cancels timers) — it's the React state update
   Mantine's inner rAF schedules outside `act`, resolving post-teardown. Also
   fix the stale setup.ts matchMedia comment: the reduce-motion stub does not
   disable transitions (Mantine defaults respectReducedMotion=false); env="test"
   is the actual #1760 protection.

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

* test(web): tighten the review-trail comments (round 2)

Comment-accuracy follow-ups from PR #1793 round 2 (test infra only, no
behavior change):

- DEFAULT_SETTLE_MS: drop the false "Mantine's default header transitions run
  ~300ms" attribution (300ms is ViewHeader's HEADER_ANIM_MS; Mantine's Transition
  default is 250ms). Justify 500ms generically — a typical few-hundred-ms
  transition plus rAF slack.
- settleTransitions doc: stop asserting a single escape mechanism as fact. State
  what's observed (a post-teardown dispatchSetState throwing from
  resolveUpdatePriority, which runs synchronously inside dispatchSetState), what's
  known (useTransition has an unmount cleanup), and that the exact escape route
  isn't pinned down — the act-wrapped drain fixes it regardless.
- setup.ts: happy-dom v20 does implement matchMedia, so reframe the block as a
  deliberate override (pinning prefers-reduced-motion: reduce) rather than a
  polyfill for a missing API.
- Export MantineRenderOptions so callers can name the options type.

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

* test(web): resolve the last comment-contradiction + tighten the matchMedia override (round 3)

Round-3 review of PR #1793 (test infra only):

- setup.ts finding 1: the inner `value:` comment still said "happy-dom doesn't
  implement it", contradicting the block comment two lines up (which now
  correctly states happy-dom v20 DOES implement matchMedia and this is an
  override). Dropped the false clause; kept the part that actually justifies the
  `as unknown as MediaQueryList` double cast (the stub omits most of the
  interface).
- setup.ts finding 5 (folded in rather than deferred): anchored the `matches`
  regex to `/prefers-reduced-motion:\s*reduce/` so the override no longer also
  answers `true` to `(prefers-reduced-motion: no-preference)`.
- renderWithMantine.tsx nit 4: the `env="test"` rationale sat on `makeWrapper`,
  which also builds the `env="default"` wrappers (opposite purpose). Moved it
  onto `renderWithMantine`, mirroring the `env="default"` rationale on
  `renderWithMantineTransitions`; `makeWrapper` keeps a one-liner.

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

* test(web): make renderWithMantineTransitions own the settle (round 4)

Round-4 review of PR #1793 — implements the note-2 hardening in-PR rather than
deferring it, plus note 1 and the nit:

- Note 2 (settle was a convention AGENTS.md had to police): the real-transitions
  helper now OWNS the settle. `renderWithMantineTransitions` arms an automatic
  post-test drain via a helper-registered `afterEach`, so a real-transitions
  test can no longer leak the #1760 class by forgetting to settle a concurrent
  enter. Verified empirically that the helper's `afterEach` runs BEFORE the
  global cleanup() in setup.ts (afterEach is LIFO; setup.ts's cleanup is a
  setupFile registered first) — the ordering the drain depends on. The three
  explicit `settleTransitions()` calls in ViewHeader.test.tsx are gone; each
  render passes `settleMs: HEADER_ANIM_MS + 200` instead, keeping the window
  duration-derived. The auto-settle skips under fake timers (and no
  transitions test uses them). `settleTransitions` stays exported for the rare
  manual mid-body case.

- Note 1 (settle formula was duration-only): the helper doc + the new
  `settleMs` option doc now say the window must exceed the longest JS chain —
  duration PLUS any enterDelay/exitDelay — plus rAF slack.

- Nit: ViewHeader.test.tsx now states why HEADER_ANIM_MS is exported (the
  settle window must track the animation it settles).

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

* test(web): pin + self-check the auto-settle hook ordering; fix fake-timer doc; keep longest settle (round 5)

Round-5 review of PR #1793:

- Note 2 (load-bearing hook ordering was unpinned/undefended): the
  auto-settle's afterEach must run before setup.ts's cleanup(). Added BOTH
  guards the review suggested. (1) Runtime self-check: renderWithMantineTransitions
  captures the render container; the afterEach throws a clear error if it's
  already detached (cleanup ran first). (2) Pinned `sequence.hooks: "stack"` on
  the unit project. While verifying, found the ordering actually holds in EVERY
  sequence.hooks mode (stack/list/parallel) because setupFile hooks are outer
  relative to the helper's import-registered hook — so the comments now state
  that true mechanism (the pin is defense-in-depth; the container.isConnected
  check is the real enforcement), rather than claiming it "depends on stack".
  Full 3533-test unit suite passes under "stack".

- Note 1 (fake-timer claim was backwards): AGENTS.md no longer says the leak
  "would return" under fake timers (fake clocks park Mantine's setTimeout/rAF
  and are discarded, not drained). The real hazard — the auto-settle silently
  no-ops, so real-clock work is unprotected — is now stated. The afterEach also
  console.warns on the fake-timer skip (was silent; the manual settleTransitions
  throws — this closes that asymmetry).

- Note 3 (arming was last-write-wins): armedSettleMs now keeps the longest
  window via Math.max, so a later short-animation render can't under-settle an
  earlier long one.

- Nit: documented `settleMs: 0` as a working opt-out for a test that drove every
  transition to completion itself (act flush, no wait).

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

* test(web): post-await self-check + correct the hook-ordering mechanism (round 6)

Round-6 review of PR #1793:

- Finding 1 (self-check blind in parallel): added the post-await
  container.isConnected re-check the review suggested (factored both checks into
  assertConnectedForSettle(container, "before"|"after")). While verifying it, I
  established the actual mechanism, which differs from what both prior comments
  and the review assumed: setup.ts's cleanup() is a *setupFile (outer)* hook, and
  Vitest runs inner afterEach hooks to completion before outer ones in EVERY
  sequence.hooks mode — so cleanup never races this settle. Verified: ViewHeader's
  real-transitions tests pass under stack/list/parallel with neither check
  tripping, and a same-suite `afterEach(cleanup)` probe (where the ordering CAN
  break) does trip them (pre-check). So sequence.hooks isn't load-bearing here;
  the comments in renderWithMantine.tsx, vite.config.ts, and AGENTS.md now say
  that. The "stack" pin stays as documentation of intent / defense-in-depth, and
  the two checks are the real guard against a future same-level-cleanup
  regression.

- Nit: documented why last-write-wins on armedContainer is fine (cleanup detaches
  every container at once, so the last is a faithful "cleanup ran" proxy).
- Nit: the fake-timer skip warning now interpolates the current test name so it's
  actionable at teardown.

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

* test(web): disarm the auto-settle on self-unmount; concurrency + message nits (round 7)

Round-7 review of PR #1793:

- Finding 1 (guard misdiagnoses a self-unmount): the detached-tree check fired
  on ANY detachment and blamed a broken cleanup ordering, but a test calling the
  unmount() returned by renderWithMantineTransitions also detaches the container
  (a common pattern here). Fixed at the source: the helper now wraps unmount() to
  disarm the auto-settle (clear armedSettleMs/armedContainer) — once the test has
  unmounted there's nothing to settle against a live tree. The guard message also
  now names the self-unmount cause. Verified: a probe that unmounts a
  real-transitions render no longer trips the guard; ViewHeader unaffected.

- Nit: documented that the module-level arming assumes sequential tests (not safe
  under test.concurrent/describe.concurrent).
- Nit: the fake-timer skip warning falls back to "(unknown test)" when
  currentTestName is undefined.

- Finding 3 (doc weight): trimmed the AGENTS.md bullet to the *rule* (use the
  transitions variant only for mid-flight assertions; pass a duration-derived
  settleMs; no fake timers; use the returned unmount()) and pointed at the
  helper's own comments for the full mechanism, which now carries it at more
  depth.

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

* test(web): downgrade (not disarm) the auto-settle on self-unmount; per-container liveness (round 8)

Round-8 review of PR #1793:

- Finding 1 (round-7 disarm was too blunt + self-contradictory): disarming on
  self-unmount removed the drain entirely for the mid-flight-unmount test that
  most needs #1760 protection, and its "useTransition cleanup applies" rationale
  contradicted this file's own settleTransitions doc (which says that cleanup
  can't be relied on to stop the rAF). Now the wrapped unmount() DOWNGRADES: the
  drain still runs (mechanism-independent — flushes queued rAF/setTimeout while
  window is alive; a setState on the unmounted tree is a React no-op), only the
  liveness assertion for that container is dropped.

- Finding 2 (disarm undid round-5 max-wins arming): arming is now per-container
  (a Set of live containers) instead of all-or-nothing. Unmounting one tree drops
  only its own container; a second still-mounted tree keeps its longer window and
  its liveness check. The isConnected assertions run over the still-live set.

Verified with probes: returned unmount() drains without tripping the guard; a
second still-mounted tree is unaffected when the first self-unmounts; a bare
mid-body cleanup() STILL trips the guard (it's live, not decoration); ViewHeader
unchanged. Comments/AGENTS.md updated so the self-unmount path and the
settleTransitions doc no longer contradict each other.

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

* test(web): make the auto-settle drain best-effort so a guard trip doesn't also leak (round 9)

Round-9 review of PR #1793 (merge-verdict; optional robustness note):

- Finding 1 (pre-settle throw skipped the drain): on the ordering regression the
  guard exists to catch, throwing before the drain meant the maintainer got BOTH
  the explanatory error AND an uncaught post-teardown `window is not defined` in
  an unrelated file. The afterEach now captures a liveness failure, always runs
  the act-wrapped drain (flushing queued rAF/setTimeout while window is alive),
  and throws afterward — so the explanatory error stands alone. Verified: a bare
  mid-body cleanup() still throws the guard error (now after draining); ViewHeader
  unchanged.

- Nits: AGENTS.md now names the reachable bare-detach path (a mid-body cleanup()
  on a still-armed tree) rather than "a bare RTL unmount"; the settleMs doc notes
  arming is per-test, so in a multi-render test the longest window wins (a `0` is
  superseded by a longer sibling render).

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

* test(web): fix the regressed fake-timer doc + guarantee the liveness error survives a drain throw (round 10)

Round-10 review of PR #1793 (merge-verdict):

- Finding 1 (AGENTS.md fake-timer clause regressed in the round-7 doc trim): it
  had reverted to "silently dropping the protection", which round 5 had corrected
  — "silently" is now false (the auto-settle console.warns on the fake-timer
  skip) and "dropping the protection" reinstates a walked-back framing (fake
  clocks park Mantine's timers on a discarded clock, so the no-op doesn't
  obviously reopen #1760). Reworded to match the helper: it warns, and the hazard
  is that real-clock work is then unprotected, so the test depends on which clock
  was installed at teardown.

- Finding 3 (nit): matched the console.warn string to that same corrected
  framing, so AGENTS.md and the warning agree.

- Finding 2 (nit): a drain-time throw could discard the captured liveness error
  (the substitution round 9 set out to prevent, one step further). Now the drain
  error is captured too and the liveness error is preferred, so the explanatory
  error always wins. Verified a bare mid-body cleanup() still throws the liveness
  error after the best-effort drain; lint clean (no throw-in-finally); ViewHeader
  unchanged.

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

* test(web): preserve the drain error as cause + route the guard message to the reachable cause (round 11)

Round-11 review of PR #1793 (merge-verdict; two optional polish items):

- Finding 1: when both a liveness error and a drain-time throw occur, the drain
  error was dropped entirely. It's now attached as the liveness error's `cause`
  (when the liveness error is an Error), so an act-surfaced component error from
  the settling render is preserved without changing which error fails the test.

- Finding 2: the guard's error message pointed the reader first at the
  `sequence.hooks: "stack"` pin, which the file itself documents as
  defense-in-depth and NOT the ordering guarantee. Reworded to name the actually-
  reachable cause first — cleanup moved to a same-level afterEach instead of the
  setup.ts setupFile (outer) hook — and demote the pin to a parenthetical, so a
  maintainer who trips the check isn't sent to investigate a non-causal setting.

Verified: a bare mid-body cleanup() throws the reworded message; eslint clean;
ViewHeader unchanged.

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

* test(web): guard message states observation before candidate causes; log the preserved drain error (round 12)

Round-12 review of PR #1793 (merge-verdict):

- Finding 1: the round-11 rewording had dropped the "Expected cause:" hedge and
  asserted setup.ts's cleanup() as the fact, but the check only observes that a
  still-live armed tree was detached — it can't identify what detached it.
  Reworded to state that observation first, then list candidate causes: (a) a
  bare mid-body cleanup() in the test (the reachable case today), or (b) cleanup
  moved off the setup.ts setupFile hook to a same-level afterEach. No single
  cause asserted as fact.

- Nits 2a/2b: when both a liveness error and a drain error fire, the drain error
  is now also console.error'd (in addition to being attached as `cause`), so it's
  readable regardless of whether the reporter renders `error.cause` and isn't
  silently dropped even if the liveness error somehow isn't an Error.

Verified: a bare mid-body cleanup() throws the reworded message; eslint clean;
ViewHeader unchanged.

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

* test(web): final comment/string polish on the auto-settle guard (round 13)

Round-13 review of PR #1793 (merge-verdict; string/comment polish only):

- Finding 1: the console.error text claimed the drain error was "reported as the
  liveness error's cause" unconditionally, but the cause is only attached when
  the liveness error is an Error — misleading on the very non-Error path the
  hoisted log exists to serve. Dropped that parenthetical so the log is true on
  both paths.
- Nit: trimmed the now-duplicated outer comment to just the priority rationale
  (the inner comment, which mentions the log + instanceof conditional, is the
  accurate one).
- Nit: guard message says "drained against a dead tree" (exact for both the
  before- and during-settle detach) instead of "drained nothing".
- Nit: the message now names renderWithMantine.tsx rather than "the ordering note
  here", so it's self-contained out of context.

eslint clean; ViewHeader unchanged; npm run ci green.

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

* test(web): correct the sequence.hooks default claim (it's "stack" in vitest 4, not "parallel") (round 14)

Round-14 review of PR #1793 (merge-verdict):

- Finding 1: verified against installed vitest 4.1.0 — `resolved.sequence.hooks
  ??= "stack"` (coverage chunk), so the runtime default is "stack"; the CLI
  help-text's "parallel" is stale in vitest itself. The vite.config.ts comment
  claimed the pin overrides a "parallel" default; corrected to say the pin is
  Vitest 4's own default made explicit (documents intent + guards a future
  default change), which is a truer reason to keep it than an override.

- Nit: tied the two additions together — the post-settle check's same-level race
  can only arise if someone changes sequence.hooks to "parallel" (same-level
  hooks run sequentially under "stack"/"list"), so the pin and the post-check now
  reference each other rather than sitting in opposed positions.

ViewHeader unchanged; npm run ci green.

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

* test(web): document the isolate:true dependency (empirically confirmed) + fix pre-check-blindness wording (round 15)

Round-15 review of PR #1793 (merge-verdict; comment-only):

- Finding 1: the auto-settle's module-level afterEach depends on isolate:true
  (the default) to re-register per test file; under isolate:false the module
  evaluates once per worker and the hook binds to only the first importing file,
  so other files arm with no consuming hook — the drain silently never runs and
  the state leaks across files. Reproduced it (vitest --no-isolate --pool=threads
  --no-file-parallelism over two real-transitions files leaves one file's arming
  unconsumed) and documented the assumption + the onTestFinished escape hatch on
  the arming-state comment.

- Nit: the "sequence.hooks is why the pre-check can't see the concurrent detach"
  clause was wrong — the pre-check is blind because it runs synchronously before
  the await (a property of where it sits, not the hook mode). Reworded to state
  that, and that the concurrent case *additionally* needs sequence.hooks:
  "parallel".

ViewHeader unchanged; npm run ci green.

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

---------

Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
 2026-07-26 15:49:33 -04:00
Cliff Hall
4defa32ac6
 		chore: close a batch of quality-gate & tidy gaps (#1785 #1787 #1789 #1790 #1767) (#1792) 
* chore: close a batch of quality-gate & tidy gaps (#1785 #1787 #1789 #1790 #1767)

Bundles five small, related tidy/gate items into one PR:

- #1785: delete the dead src/types/navigation.ts InspectorTab (0 importers;
  duplicated/drifted from utils/inspectorTabs InspectorTabId). src/types/
  now holds only ambient .d.ts stubs.
- #1787: add src/hooks/** to the web coverage `include` (it was silently
  outside the ≥90 gate) and add tests for the two untested hooks,
  useServerJsonImport and useImportClientConfig, bringing every hook to
  ≥90 on lines/statements/functions/branches.
- #1789: widen clients/{cli,tui,launcher} format/format:check to their
  top-level configs; add root format:shared / format:check:shared covering
  test-servers/src/**, vitest.shared.mts, and the repo-root eslint.config.js.
- #1790: pin prettier to an exact version (3.8.4, dropping the ^3.8.1
  caret) in all five package.json files so the format gate's verdict can't
  shift with an in-range patch bump; lockfiles synced.
- #1767: its root-file format+lint gap is closed here (format:shared +
  a new lint:shared via a second files block in the root eslint.config.js;
  web/server/ was already gated in #1788). Its larger cli/tui __tests__
  typecheck piece (76 type errors) is split out to #1791.

Prettier-normalized the newly-covered files (pure formatting). No runtime
behavior change. `npm run ci` green.

Closes #1785
Closes #1787
Closes #1789
Closes #1790
Closes #1767

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

* chore: address PR #1792 review findings

- #1789 gap: widen clients/web format to include .storybook and clients/
  launcher format to include scripts/; reformat the 3 .storybook files
  (were non-conforming under the pinned prettier 3.8.4). Audited all 885
  tracked source files — now fully prettier-clean.
- Export VALIDATE_DEBOUNCE_MS / COLLAPSE_DELAY_MS / HIGHLIGHT_DURATION_MS
  from useServerJsonImport and use them in the test instead of hardcoded
  300/1000/250 (so a tuned constant can't silently desync the test).
- Assert the symmetric renameErrors["alpha"] alongside ["beta"] so a
  one-directional collision-check regression fails.
- Comment the blank-rename test as exercising a UI-unreachable defensive
  branch (canImport gates submit).
- Un-stale the two specification/v2_ux_interfaces_plan.md references to the
  now-removed navigation.ts InspectorTab.

npm run ci green.

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

* docs: sync AGENTS.md format-surface description with the widened globs (#1792 review r2)

1ce3b64 added .storybook to clients/web's format glob and scripts/ to
clients/launcher's, but AGENTS.md:222 still described the pre-fix scope.
Bring it in line and note the non-recursive top-level glob is why nested
dirs are named explicitly. Docs only.

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

* chore: handle PR #1792 round-3 follow-ups in-place (format-coverage guard, uniform globs, theme gate)

Rather than defer these to tickets, fold them into this PR:

- Obs 2: widen every client + root format glob to a uniform
  *.{ts,tsx,mts,cts,js,jsx,mjs,cjs} so a future .mts/.cjs/.tsx top-level
  file can't silently slip the gate.
- Obs 1: add scripts/verify-format-coverage.mjs (wired into `validate`) —
  it parses the format:check globs out of every package.json, enumerates
  all tracked source files, and fails listing any not covered by a glob.
  The durable, standing version of #1789's one-shot audit; negative-tested.
- Obs 3: bring src/theme/** under the coverage gate (21/23 files were
  already ≥90 transitively; add Paper.test.tsx for the one laggard,
  Paper.ts). src/App.tsx (~4.5k lines, ~42% branches) is converted from a
  silent omission into an explicit, documented exclusion (comment on the
  coverage `include` + AGENTS.md) — gating it is a dedicated effort, not a
  whitelist tweak.

npm run ci green (the new guard reports 885/885 tracked source files gated).

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

* chore: address PR #1792 round-4 findings (guard hardening, docs, exclusion list)

- Harden verify-format-coverage: only harvest format:check globs from
  scripts transitively reachable from `validate` (via npm run refs), so an
  unwired/renamed format:check no longer counts as coverage. Negative-tested
  — renaming clients/web format:check to an unreferenced name now fails the
  guard (627 files flagged) instead of passing.
- Name src/main.tsx and src/index.ts (browser + bin bootstraps) alongside
  src/App.tsx in the coverage-include comment and AGENTS.md, so the
  documented exclusion list is complete rather than claiming App.tsx is the
  only one.
- Doc sync: document verify:format-coverage (AGENTS.md + README script table
  + scripts/ layout line), fix the `validate` ordering (verify:format-
  coverage now runs before validate:core) in AGENTS.md and README, and
  update the quoted globs to the uniform *.{ts,tsx,mts,cts,js,jsx,mjs,cjs} /
  test-servers *.{ts,tsx,mts,cts}.

npm run ci green.

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

* chore: address PR #1792 round-5 findings (airtight guard + doc/config sync)

- verify-format-coverage: assert the root `validate` chain invokes each
  client's `validate` (cd <dir> && npm run validate), closing the last
  reachability edge — a client dropped from the root chain now fails the
  guard even though its own globs still exist. Negative-tested.
- eslint.config.js: widen the shared `files` block to
  test-servers/src/**/*.{ts,tsx,mts,cts} to match the widened lint:shared
  glob, so a future .mts/.cts there is lint-gated, not just prettier-gated.
- AGENTS.md: scope the "uniform extension set" claim to client globs
  (core stays {ts,tsx}, shared {ts,tsx,mts,cts}), with verify:format-coverage
  as the backstop — removing the self-contradiction with the line below.

npm run ci green.

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

---------

Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
 2026-07-26 10:43:11 -04:00
Cliff Hall
166ed38e7f
 		chore(web): format-gate the ungated first-party TS/JS (server/, vite.config.ts, scripts/) (#1788) 
* chore(web): format-gate the ungated first-party TS/JS (#1778)

Prettier only covered core/ and each client's src/, leaving the web Node
backend (clients/web/server/**), web top-level configs (vite.config.ts,
tsup.runner.config.ts, eslint.config.js, ...), and the root build/verify
tooling (scripts/**/*.mjs) ungated — so their formatting was never checked
by validate/CI. eslint already reached these via `eslint .`, so this is a
prettier-only gap.

- clients/web format/format:check now cover src, server, and top-level
  *.{ts,js}, not just src.
- New root format:scripts / format:check:scripts for scripts/**/*.mjs,
  folded into the root format and into validate:core.
- Normalized the newly-covered files with prettier --write (pure
  formatting; no runtime change).

Closes #1778

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

* chore: future-proof the scripts format glob (#1778 review)

Widen format:scripts / format:check:scripts from "scripts/**/*.mjs" to
"scripts/**/*.{mjs,js,cjs,ts}" so a future non-.mjs file under scripts/
is auto-gated, matching the future-proof glob style clients/web now uses.
A brace glob is a single pattern, so prettier does not error on the
unmatched branches. Addresses observation #2 from the PR #1788 review.

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

* docs: sync AGENTS.md scripts glob after the widening (#1788 review r2)

d86d8f7 widened the scripts glob to {mjs,js,cjs,ts} and updated the
validate:core bullet, but the format bullet at AGENTS.md:217 still said
scripts/**/*.mjs. Bring it in line so the two mentions agree.

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

---------

Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
 2026-07-25 23:15:45 -04:00
Cliff Hall
a82a70f4f5
 		refactor(web): sharpen the src/lib vs src/utils rule and move misfiled modules (#1784) 
* refactor(web): sharpen src/lib vs src/utils rule and move misfiled modules (#1776)

Codify the empirical rule the web client already followed loosely — utils =
pure functions that compute; lib = things that instantiate, adapt, or touch
the environment — in AGENTS.md and clients/web/README.md, and move the modules
that sat on the wrong side of it.

Moves (source + co-located test):
- lib/sandbox-csp -> utils (pure CSP-string construction; type-only import)
- utils/browserTabVisibility -> lib (registers visibilitychange listeners)
- utils/oauthResume -> lib (reads/writes sessionStorage)
- utils/clearServerOAuthState -> lib (drives live InspectorClient / OAuthStorage)

Borderline audits (kept in place): pendingReauth (pure type declarations),
deepLink / correlateTransportErrors / mcpNetworkHeaders / oauthUx / oauthFlow /
oauthNetworkPhase (pure transforms / type-only core imports) stay in utils;
downloadFile stays whole in lib (dominant side is DOM side effects).

Also relocate the lone src/test/lib/downloadFile.test.ts straggler beside its
source at src/lib/downloadFile.test.ts, and document the test-placement rule
(side-by-side by default; src/test/ only for core/ tests, the integration
project, and shared scaffolding).

Pure move — no behavior change; no build/coverage config keys off the boundary.

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

* refactor(web): address review — fix stale paths, move customHeaders, one-way arrows (#1776)

Responds to the @claude review on PR #1784:

1. Stale path references (fixed): update the four in-repo pointers left at
   pre-move paths — specification/v2_auth_mid_session.md (oauthResume,
   browserTabVisibility → lib/), and the two load-bearing sandbox security
   comments (sandbox_proxy.html, sandbox-controller.ts → src/utils/sandbox-csp).

2. lib/types/customHeaders (moved): it's pure (types + pure transforms), the
   rule's clearest counterexample. Moved lib/types/customHeaders.{ts,test.ts} →
   utils/ (no external importers; only its own test) and removed the
   undocumented lib/types/ subdir. Documented that utils/ is the home for pure
   shared domain types + their constructors — no separate types/ bucket.

3. One-way arrows (item 3): declare OAuthResumeAuthKind in utils/pendingReauth
   and re-export it from lib/oauthResume, so the only cross-directory arrow now
   points lib → utils. Codified that invariant in AGENTS.md.

Wording nit: AGENTS.md + README now state diagnostic logging
(console.warn/error) doesn't count as a side effect for the utils rule.

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

* docs: address review round 2 — stale customHeaders refs, rule wording (#1776)

Doc-only follow-up to the round-2 @claude review on PR #1784 (no code changed):

1. Stale paths (same class as round-1 item 1): the customHeaders move left
   three references to the deleted lib/types/customHeaders.ts path in
   specification/ (v2_ux_interfaces_phase_0_4_inventory.md,
   v2_ux_interfaces_plan.md ×2) — updated to utils/customHeaders.ts.

2. customHeaders anchor: note in AGENTS.md that its shape is staged for
   ServerSettingsForm (per v2_ux_interfaces_plan.md), so its zero-importer
   state reads as intentional rather than rot; kept as the no-types/-bucket
   illustration and led the anchor list with load-bearing modules.

3. Type-only-import clause: split the conflated clause — a type-only import
   (pendingReauth) and re-exporting core's pure functions/constants
   (oauthUx/oauthFlow) are distinct reasons to stay in utils; what makes a
   module lib is wrapping core's stateful runtime. Fixed in both AGENTS.md
   and the web README.

Cosmetic: re-padded the v2_auth_mid_session.md table row to column width.

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

* docs: address review round 3 — scope the "no types/ bucket" claim (#1776)

Doc-only follow-up to the round-3 @claude review on PR #1784 (no code changed):

1. "no separate types/ bucket" overreached: clients/web/src/types/ does exist
   (navigation.ts + react-syntax-highlighter .d.ts ambient stubs wired via
   tsconfig.app.json paths). Scoped the claim in both AGENTS.md and the web
   README to "no types/ sub-bucket INSIDE lib/utils", and noted src/types/ is
   for ambient .d.ts stubs, not domain types.

2. README example list drifted from AGENTS.md's round-2 load-bearing anchors —
   realigned it (inspectorTabs/mcpNetworkHeaders, dropped the zero-importer
   customHeaders from the headline list).

3. Clarified v2_ux_interfaces_plan.md:185 so the v2 path reads as the copy's
   destination, not as the v1.5 source location.

Follow-up filed as #1785 (board Todo): src/types/navigation.ts InspectorTab is
dead and duplicates utils/inspectorTabs.ts — kept out of this pure-move PR.

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

* docs: address review round 4 — reconcile src/types/ claim with #1785 (#1776)

Doc-only follow-up to the round-4 @claude review on PR #1784 (no code changed):

The round-3 scope fix overshot: it asserted src/types/ holds "not domain types
at all", but the dead navigation.ts InspectorTab is a plain-.ts domain type
still living there, and specification/v2_ux_interfaces_plan.md:181-182 actively
prescribes placing InspectorTab there — so AGENTS.md and that spec doc gave
opposite guidance about the same directory.

- AGENTS.md + web README: softened to "not the place for new domain types"
  and added a parenthetical that the one lingering domain type (dead
  navigation.ts) is tracked for removal in #1785; until then a pure domain
  type belongs in utils/, not src/types/.
- v2_ux_interfaces_plan.md:181: marked the tab-enum placement guidance as
  superseded by #1785 (live type is InspectorTabId in utils/inspectorTabs.ts).

Self-heals on merge of #1785.

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

* docs: address review round 5 — fix garbled #1785 clause, restructure utils rule (#1776)

Doc-only follow-up to the round-5 @claude review on PR #1784 (no code changed):

1. Grammar (item 1): the round-4 #1785 clause read "superseding [#1785] will
   remove it" — a participle with no subject. Reworded to "the dead
   navigation.ts InspectorTab, which #1785 will remove".

2. Readability (item 2): the src/utils/ bullet had accreted into a single
   ~1,400-char blob across five review rounds. Promoted the carve-outs to
   scannable sub-bullets (Domain types / Diagnostic logging / Importing from
   core) and lifted the src/types/ note out to its own line, since it's a
   statement about a sibling directory, not about utils.

AGENTS.md only; the README's parallel sentence was already clean.

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

* docs+config: address review round 6 — correct the coverage-glob claim (#1776)

Responds to the round-6 @claude review on PR #1784:

1. "no build/coverage glob" was factually wrong (should-fix): the web coverage
   include in clients/web/vite.config.ts DOES enumerate src/lib/** and
   src/utils/**. The conclusion (a move between them is coverage-neutral)
   survives, but the reason was wrong. Reworded both AGENTS.md and the web
   README to say nothing *enforces* the boundary — no path alias, and the
   include lists both dirs so a move is coverage-neutral — plus the important
   whitelist caveat: a module outside components/lib/utils/server falls out of
   the ≥90 gate entirely.

2. Removed a pre-existing duplicate glob in vite.config.ts (src/lib/**/*.{ts,tsx}
   appeared twice, lines 161 and 163). Inert (vitest de-dupes) but it sat three
   lines from the config the doc now describes.

Nits: gave the README bullet the same round-5 sub-bullet restructure as
AGENTS.md, and made its #1785 reference a real markdown link (bare #N doesn't
autolink in rendered repo markdown).

npm run ci green (config change re-verified through coverage + smokes + Storybook).

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

---------

Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
 2026-07-25 22:24:30 -04:00
Bob Dickinson
d009fcca09
 		Fixed tab naming to correspond to the web client. (#1780) 2026-07-25 20:11:52 -04:00
Cliff Hall
07d6316fba
 		build(web): fail vite build on a browser-externalized Node built-in (#1769) (#1777) 
* build(web): fail vite build on a browser-externalized Node built-in (#1769)

Vite 8 (rolldown) externalizes a node:* / Node built-in that reaches the
browser graph with a warning and ships a `module.exports = {}` stub — the
build still succeeds, shipping a broken bundle. The runtime browser smoke
(smoke:web:browser, #1615) only catches the subset where that stub is *called*
at module init; a stub imported but never called ships silently and nothing
gates it.

Add a Vite build plugin that turns that specific browser-externalization
warning into a hard `vite build` error, moving detection upstream into
`npm run build` / `validate` and additionally catching the never-called case
the runtime smoke can't see.

- clients/web/server/browser-externalized-builtin-gate.ts: Vite-agnostic
  detection + error + per-build state machine (unit-tested to the ≥90 gate).
- vite.config.ts: thin Vite wiring. A throw inside rolldown's onLog is
  swallowed (the one hook where it doesn't abort — verified against vite@8.0.0),
  so the gate records the warning in onLog and re-throws in buildEnd. Scoped to
  `vite build` (apply: 'build'); the Node runner build (tsup) is unaffected.
- scripts/verify-build-gate.mjs + verify:build-gate (in npm run ci and the
  GitHub workflow): runs a real build with a node:fs probe and asserts it fails
  via the gate — the only check that catches the warning phrasing drifting in a
  future Vite bump and silently disabling the message-keyed gate.

There is no stable log `code` on the rolldown warning (only message + plugin),
so the gate keys off the documented message phrasing; the drift guard above is
what keeps that safe across Vite versions. smoke:web:browser stays as the
runtime backstop.

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

* build(web): harden the browser-externalized-builtin gate per review (#1769)

Review follow-ups on PR #1777:

- Per-build state: add gate.reset() called from the plugin's buildStart, so a
  `vite build --watch` rebuild doesn't inherit a prior build's recorded warning
  (the plugin instance is reused across rebuilds). Makes the docblock true.
- buildEnd(error) now only asserts on the success path — rolldown also calls
  buildEnd(error) when the build already failed for another reason, and throwing
  then would mask that real error with the #1769 message.
- verify-build-gate.mjs: drop the shell `rm -rf` cache-clear entirely (verified
  the gate fires reliably without it — the earlier "caching" symptom was the
  onLog-swallow behavior, not a stale cache); add a result.error spawn-failure
  check so a missing `npx` isn't misdiagnosed as "not via the gate"; add
  SIGINT/SIGTERM handlers so an interrupt during the multi-minute build still
  restores the mutated entry; add a progress log before the spawn.
- Match the double-quote style of the sibling server/ files.
- Docs: list browser-externalized-builtin-gate.ts in the AGENTS.md server/ block
  and the clients/web/README server/ list (with why a build helper lives under
  server/), and add verify-build-gate to the root README scripts/ line.

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

* build(web): round-2 review hardening for the #1769 build gate

- Scope the gate to the browser (`client`) environment via applyToEnvironment,
  so a future SSR/node environment built from this config isn't failed for a
  legitimate node:* import — browser-only intent is now structural.
- Report ALL externalized built-ins in one build, deduped, instead of only the
  first offender (N leaks no longer means N fix-and-rebuild cycles).
  browserExternalizedBuiltinError now takes string[]; gate uses a Set.
- verify-build-gate.mjs: pin Vite with `npx --no-install` so the drift check
  can never silently run a registry-fetched Vite instead of the repo-pinned one
  (the whole point is version fidelity); append the probe instead of prepending
  (ES imports hoist, and appending won't demote a future leading directive);
  clarify the signal-handler comment (handlers are a backstop — a blocked
  spawnSync tears down via the process group first).
- AGENTS.md: update both `ci` pipeline enumerations to include verify:build-gate
  and drop the now-false "Storybook is the only CI step left out" claim.

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

* build(web): round-3 review polish for the #1769 build gate

- Error message: name the transitive-dependency remedy (a resolve.alias to a
  browser shim) — "remove the import / gate behind the backend" doesn't apply
  when the node:* import lives in node_modules.
- isBrowserExternalizedBuiltinLog is now a `message is string` type guard, so
  recordLog drops its redundant `!== undefined` clause while still typing the
  Set.add() — a real one-liner, not a coverage-only removal.
- verify-build-gate.mjs: docblock now states WHY it builds the real config +
  entry (catches config-level regressions — plugin removed, onwarn suppression
  added — that a temp config would miss), so the minute-long build isn't later
  "optimized" into a temp entry and silently lose that coverage. Guard the
  entry read so a renamed main.tsx fails through fail() with an actionable
  message. On a botched restore, save the captured original to a sidecar .bak
  and point there instead of recommending `git checkout --`, which would
  discard uncommitted edits.
- AGENTS.md: fix the muddled CI-steps sentence (GitHub CI runs the whole chain
  as separate steps, Storybook last) and note the applyToEnvironment client-only
  scoping in the gate prose.

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

* build(web): round-4 review polish — sharper verify-build-gate diagnostics (#1769)

All in scripts/verify-build-gate.mjs's failure paths:

- Split the "build SUCCEEDED" failure into three diagnoses keyed off the
  captured output, so it names the right file: warning present but build passed
  → plugin not applying (removed from `plugins`, `applyToEnvironment` env-name
  mismatch, or an `onwarn` suppression); node:fs present but phrase absent →
  phrasing drift, update the constant; neither → probe never reached the graph.
  The old message hard-coded "phrasing drifted", which misdirects for the
  config-regression class this script exists to catch. Added applyToEnvironment
  to the docblock's config-regression list too.
- Bound the build with a 10-min spawnSync timeout (+ SIGKILL); a timeout sets
  result.error (ETIMEDOUT) and reports via the existing branch, instead of
  burning to the GitHub job's 360-min default with no output.
- Guard the probe write so a read-only/EACCES checkout fails through fail()
  with an actionable message instead of escaping as a raw stack.
- SIGTERM now exits 143 (was 130, which is SIGINT's code).

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

* build(web): round-5 review polish — self-consistent verify-build-gate (#1769)

All in scripts/verify-build-gate.mjs:

- Drift guard: assert the gate source file still contains the mirrored
  KNOWN_PHRASE literal, failing fast if the two drift. Without it, a stale
  KNOWN_PHRASE after a phrasing update would make the three-way diagnosis
  misreport a plugin-not-applying regression as a phrasing drift — the exact
  misdirection the diagnosis exists to avoid.
- Make the .bak safety net reachable on the path that needs it most: restoreEntry
  now catches its own write failure and routes to a shared saveBackupAndFail
  helper, instead of letting the write escape the finally as a raw stack (which
  skipped the .bak fallback and left the probe injected in the entry).
- Add the fourth "plugin isn't applying" cause to the diagnosis: a future Vite
  emitting the warning before the client env's buildStart reset clears it (same
  symptom as the other three).
- Flag once, on the failing-check path, that clients/web/dist holds a probe
  build so a local debugger doesn't serve it unaware.

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

* build(web): round-6 review polish — verify-build-gate literal contracts (#1769)

All in scripts/verify-build-gate.mjs; one theme — the literals the script
asserts on can each go stale independently of what they mirror:

- Success key: assert on the gate's distinctive thrown-error prefix
  "Build failed (#1769)", not a bare "#1769". The issue number also appears in
  vite.config.ts comments, so a code frame from an unrelated build error there
  could otherwise make the check report OK with the gate dead — the one
  direction it must never get wrong (a false pass hiding a dead gate).
- Drift guard now anchors on the BROWSER_EXTERNALIZED_BUILTIN_PHRASE assignment
  (regex) instead of a whole-file substring match, so the old wording lingering
  in a comment can't mask a changed constant; and its readFileSync of the gate
  module is guarded so a moved file fails through fail() actionably.
- Extract PROBE_MODULE ("node:fs") used to build both the PROBE and the
  diagnosis check + messages, so changing the probe module can't silently
  misroute a phrasing-drift failure into "probe never reached the graph".
- Comment the load-bearing diagnosis order (phrase before module-name).

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

* build(web): round-7 review polish — drift-guard both mirrored literals (#1769)

- Extract ERROR_PREFIX ("Build failed (#1769)") and drift-guard it against the
  gate module too, and use it as the success key. Previously only KNOWN_PHRASE
  was guarded; the prefix is mirrored across the same TS/.mjs boundary for the
  same reason, so rewording the gate's thrown error would make a correctly-firing
  gate report "the build broke for another reason" (benign direction, but the
  same misdirection the other guards close).
- Write the restore-failure .bak under the OS temp dir, not next to the entry:
  a sidecar inside src/ is untracked and a `git add -A` (likely while recovering)
  would sweep it into a commit.
- Branch-(b) remediation now says update BOTH the gate constant AND KNOWN_PHRASE,
  so a phrasing-drift fix lands in one pass instead of tripping the drift guard
  on the next run.
- Note next to BROWSER_EXTERNALIZED_BUILTIN_PHRASE that it must stay a single
  string literal (the script's drift guard anchors a regex on the assignment).

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

* build(web): round-8 review polish — safe recovery path + structural onLog order (#1769)

- saveBackupAndFail no longer claims a backup it may not have written: it tracks
  whether the write succeeded and only points the user at the .bak in that case
  (otherwise it says the probe is still injected and to restore from VCS). Uses
  mkdtempSync for a unique dir instead of a fixed tmp filename, so a
  pre-existing/foreign-owned /tmp file can't make the recovery message point at
  someone else's content.
- Guard the restore-verification readFileSync too, routing a failure through
  saveBackupAndFail so the whole restore path is uniformly safe (a raw throw
  there would skip the backup net).
- vite.config.ts: enforce: 'pre' on the gate plugin so it runs ahead of the
  normal-plugin group in the onLog chain — a plugin whose onLog returns false
  filters the log for later plugins, so trailing the array would let a future
  log-filtering plugin silently blind the gate. Makes "the gate sees every log"
  structural rather than array-position-dependent.

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

* build(web): round-9 review polish — prove the gate fired on the probe (#1769)

The success key confirmed the #1769 gate fired, but not that OUR probe tripped
it. If the repo already leaked a Node built-in, the gate would fire on that and
the script would report OK even if the probe was tree-shaken or entryPath went
stale — branch (c)'s failure silently inverted into a pass. Add an assertion
that PROBE_MODULE appears among the gate's embedded offender warnings. Unreachable
inside `npm run ci` (validate's build:web fails first on a pre-existing leak),
but this script is a documented standalone command — run exactly when debugging
a leak, the case where this mattered.

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

* build(web): round-10 review polish — roll back a partial probe write (#1769)

The probe writeFileSync uses the default 'w' flag (truncates at open), so a
mid-write ENOSPC/EIO could leave clients/web/src/main.tsx empty or half-written
— and its catch called fail() directly, the one path on this script that could
destroy the entry rather than merely dirty it (every other read/write is already
routed into the recovery net). Call restoreEntry() in that catch to roll the
partial write back (falling through to the .bak net if the restore itself can't
write), and correct the comment that claimed a write failure happens "before any
mutation". Also widen the enforce:'pre' comment to note it orders ahead of Vite's
core plugins too (harmless — the gate defines no resolve/load/transform).

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

* build(web): round-11 review polish — don't let the rollback swallow the write error (#1769)

Round 10's restoreEntry() in the probe-write catch swallowed the primary error:
restoreEntry() on its own write failure calls saveBackupAndFail → fail → exit,
so the "could not write the probe" diagnosis never ran when the rollback also
failed — which is exactly the read-only-checkout / ENOSPC case (both writes fail
at open). On a read-only checkout nothing was ever written, yet the developer
was told the restore failed and pointed at a /tmp backup of an untouched file.

Roll back inline instead: attempt the restore without exiting, track whether it
succeeded, then always report the PRIMARY write error, appending "restore from
version control" only when the rollback didn't take. Also pin the
"Build failed (#1769)" error prefix as contiguous in a single string fragment
(a comment symmetric with the phrase pin), since verify-build-gate.mjs
drift-guards on it as ERROR_PREFIX.

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

* build(web): round-12 review polish — don't let the pin comment defeat its own guard (#1769)

The round-11 pin comment quoted "Build failed (#1769)" verbatim, so the prefix
appeared twice in the gate module and verify-build-gate.mjs's whole-file
includes(ERROR_PREFIX) drift guard passed on the comment alone — rewording the
thrown error would have kept the guard green while a correctly-firing gate
reported "the build broke for another reason." Reworded the comment to not repeat
the full lead (matching how the phrase pin avoids repeating its phrase); the
literal now occurs exactly once, restoring the guard's meaning.

Also make the probe-write recovery message factual instead of inferred: read the
entry back and only warn "restore from version control" when it actually differs
from the original. On a read-only checkout both writes fail at open, so the file
is untouched and the old hedge needlessly implied damage.

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

---------

Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
 2026-07-25 19:45:11 -04:00
Cliff Hall
28016ea0c8
 		fix(web): degrade the connected header to the catalog name for an empty reported serverInfo name (#1774) (#1775) 
* fix(web): degrade the connected header to the catalog name for an empty reported serverInfo name (#1774)

A server that reports serverInfo with an empty name string (an
empty-but-present name) left ViewHeader rendering a nameless title. App's
`??` fallback only fires when the whole serverInfo object is absent
(the #1772 modern-omitted case), so a reported `{ name: "" }` slipped
through with `serverInfo.name === ""`.

Resolve the header's display name in InspectorView: fall back to the
active server's catalog name when the reported name is blank. This is a
display-only fallback — the Connection Info modal stays faithful to the
raw report because it reads App's untouched `initializeResult` +
`serverInfoReported`, so `serverInfoReported` keeps meaning "the server
sent a serverInfo object" (a reported-but-empty name still reads as "—"
in the modal, and any reported version stays intact — driving the flag
off name emptiness would wrongly mask the version).

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

* fix(web): degrade a whitespace-only reported server name; strengthen tests + cross-reference (#1774)

Review follow-ups on PR #1775:

- resolveHeaderServerInfo now guards with `serverInfo.name.trim()`, so a
  whitespace-only reported name ("   ") — the same non-conforming class as
  an empty string — also degrades to the catalog name instead of rendering a
  blank-looking header title.
- Strengthen the no-catalog-match test: assert the connected header chrome
  (Disconnect control) actually renders inside role="banner", making it a real
  regression guard rather than a vacuous absent-string check.
- Add a whitespace-only header-fallback test.
- Add a back-reference in App's initializeResult memo comment pointing down to
  InspectorView's resolveHeaderServerInfo, closing the loop between the two
  fallbacks.

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

* fix(web): trim whitespace-only reported name/version in the Connection Info modal too (#1774)

Second-review follow-up on PR #1775. The header now degrades a whitespace-only
reported name; the modal had the same gap — `serverInfo.name || "—"` treats
"   " as truthy and renders a visually blank row, the exact outcome the `||`
exists to prevent. Trim both the name and version rows before the fallback so a
blank-but-present value reads as "—". Uses optional chaining (`?.trim()`) to
preserve the prior tolerance of a non-conforming runtime-absent field (the type
is non-null, but a server can omit it — covered by the existing "version
missing" test). Stays faithful: the fallback is still "—", never a borrowed
catalog name. Applied the same `?.` hardening to the header's
resolveHeaderServerInfo guard.

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

* test(web): pin the runtime-absent-name tolerance in header + modal (#1774)

Third-review follow-up on PR #1775. Commit 2f508ffa added `serverInfo.name?.trim()`
in both ConnectionInfoContent and resolveHeaderServerInfo, but only the version
`?.` short-circuit was exercised (by the pre-existing "version missing" test) —
the symmetric name `?.` was uncovered. Add two tests passing a serverInfo that
omits `name` (`{ version: "1.0.0" } as never`): the modal Name row reads "—",
and the header degrades to the catalog name. Pins the documented tolerance and
makes the two new branches explicit rather than relying on gate headroom.

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

---------

Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
 2026-07-25 14:41:42 -04:00
Cliff Hall
820d38c438
 		fix(web): render the connected header when a modern server omits serverInfo (#1773) 
A modern-era server/discover makes serverInfo optional (SHOULD, not MUST); a conforming modern server that omits it left initializeResult undefined, collapsing the entire connected header + tab bar so the server was uninspectable. Gate initializeResult on connectionStatus alone with a catalog-name fallback, and thread a serverInfoReported flag so the Connection Info modal reports the omission truthfully instead of presenting the inferred name as server-sent. Closes #1772.
 2026-07-25 11:40:31 -04:00
Cliff Hall
da37692485
 		refactor(web): sweep AGENTS.md TypeScript + Mantine rule violations (#1771) 
Codebase-wide sweep bringing the web client into strict compliance with the AGENTS.md TypeScript and Mantine/React conventions: ~170 inline 2+-static-prop elements extracted to named .withProps() constants across ~77 files, the AGENTS.md rule wording tightened to match (static-prop boundary + Accordion/Transition/data-*/Box exemptions), a real ScreenStage Box->Flex layout regression fixed (direction: column) with a Storybook regression guard, and an App.tsx double-cast tightened. Closes #1762.
 2026-07-25 11:38:49 -04:00
Cliff Hall
8aba3122c5
 		refactor(web): consolidate the three GET /api/config hooks into one (#1770)
Some checks failed
Run install, format, lint, build, and test on every push / build (push) Has been cancelled
Details
Run install, format, lint, build, and test on every push / publish (push) Has been cancelled
Details
Run install, format, lint, build, and test on every push / publish-github-container-registry (push) Has been cancelled
Details
v2-alpha-15 
* refactor(web): consolidate the three GET /api/config hooks into one (#1643)

useSandboxUrl, useServerListWritable, and useInspectorVersion each fetched
the same static /api/config payload separately and read one field from it —
three requests on load for one payload. Replace them with a single
useInitialConfig hook that fetches once and exposes { version, sandboxUrl,
writable, loading } with the same tolerant parsing/defaults (writable
defaults true; version/sandboxUrl undefined when absent; unmount-safe).

- Add core/react/useInitialConfig.ts; wire App.tsx to it.
- Remove the three former single-field hooks and their tests.
- Fold the fetch-branch matrix into one useInitialConfig test.

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

* test(web): address review nits on useInitialConfig tests (#1643)

- "keeps writable true for any non-false value" now sends real falsy-but-
  not-false values a nonconforming backend could actually emit (null / 0 /
  "no") instead of `writable: undefined`, which was indistinguishable from an
  absent key and duplicated the omitted-field test.
- Soften the post-unmount comment to say the test exercises the isCancelled()
  guard branches for coverage rather than implying it can detect their removal
  (React 18 dropped the setState-after-unmount warning).

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

* docs(web): fix contradicting unmount-guard comment; it.each writable cases (#1643)

Second-review follow-ups:
- core/react/useInitialConfig.ts: correct the isCancelled() rationale — React
  18 no longer warns on setState-after-unmount; the guard exists to stop a
  stale payload (after unmount or a baseUrl/authToken re-run) overwriting
  current state. The old wording contradicted the corrected test comment.
- useInitialConfig.test.tsx: convert the writable-non-false loop to it.each so
  each value (null / 0 / "no") reports independently.

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

* test(web): use vitest %j specifier in it.each title; tidy hook docblock (#1643)

Third-review follow-ups (both title/comment-only):
- useInitialConfig.test.tsx: `%p` is a Jest specifier vitest doesn't support,
  so all three cases rendered the literal `%p`. Switch to `%j` — titles now
  render null / 0 / "no" distinctly (verified: `writable=null`, `writable=0`,
  `writable="no"`).
- useInitialConfig.ts: extend the class docblock to say a response is dropped
  after unmount OR a re-fetch, matching the corrected inline guard comment.

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

---------

Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
 2026-07-24 22:12:19 -04:00
Cliff Hall
742e82515a
 		ci: add headless-browser boot smoke for the web client (#1615) (#1768)
Some checks failed
Run install, format, lint, build, and test on every push / build (push) Has been cancelled
Details
Run install, format, lint, build, and test on every push / publish (push) Has been cancelled
Details
Run install, format, lint, build, and test on every push / publish-github-container-registry (push) Has been cancelled
Details
v2-alpha-14 
* ci: add headless-browser boot smoke for the web client (#1615)

smoke:web only asserts GET / serves the SPA HTML with the injected token;
it never runs the React app, so a Node built-in reaching the browser
bundle (which crashes the app to a blank page at runtime, e.g. #1612)
slips through. Add smoke:web:browser, which boots the same prod --web
server and runs the bundle in headless Chromium (Playwright, already a
clients/web devDependency), asserting the app renders its first frame
(the "Add Servers" control) with no uncaught page errors — in particular
no `Module "node:*" has been externalized`. This catches "Node code
reached the browser bundle" as a class, not one import at a time.

Wire it into the root smoke chain (and thus npm run ci / GitHub CI), and
move the Playwright install/cache steps ahead of the smoke step so the
new smoke reuses the cache. Update AGENTS.md and README.

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

* ci: address review on the headless-browser boot smoke

- Don't fail on benign console.error (CDN font loads, React warnings) — the
  #1612 class arrives as an uncaught pageerror. console errors are now
  diagnostics, hard-failing only when they carry the externalization
  signature.
- Broaden the externalization regex to non-prefixed built-ins ("fs" as well
  as "node:fs") and label the signature on both the pageerror and console
  channels.
- Rename the port override to SMOKE_WEB_BROWSER_PORT so it doesn't collide
  with smoke:web's SMOKE_WEB_PORT.
- Add a settle window (networkidle + 500ms) so a late-evaluating lazy chunk
  that throws is caught, and race the render against launcher death so a
  mid-load crash reports the real cause instead of a render timeout.
- Print an actionable hint when Playwright/chromium can't launch (missing
  --with-deps system libs).
- Extract the shared spawn/readiness boilerplate into
  scripts/lib/prod-web-server.mjs so smoke-web and smoke-web-browser can't
  drift.

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

* ci: fix playwright resolution in the browser smoke (CI failure)

The bare `import("playwright")` resolved relative to the script's own
directory (scripts/), not the cwd — so `cd clients/web` in the npm script
never made it resolvable. It only passed locally because an ancestor
node_modules happened to carry playwright; CI has none, so it failed with
"Cannot find package 'playwright'". Resolve it explicitly from
clients/web via createRequire, independent of cwd and ancestor dirs.

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

* ci: address second review on the prod-web-server helper

- waitForReady now records the last non-ok status and includes it in the
  timeout message, so a server that boots but persistently answers 500
  (broken dist / injection failure) reports "server did not start … (last
  response: HTTP 500)" instead of a bare timeout that points at the wrong
  thing. Keeps polling on non-ok (a warming server may answer 503).
- Add a child 'error' handler so a spawn-level failure surfaces as the
  smoke's FAILED line instead of an uncaught raw stack; readiness and the
  child-exit race both report it.
- Drop the unused isExited/exitCode/child exports from the helper API.

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

* ci: reframe browser smoke around uncaught pageerror; fix stop() edge

Third-review findings.

1. The `Module "…" has been externalized` string can't fire against a Vite 8
   *prod* bundle — verified end-to-end against the installed vite@8.0.0: a
   node: import reaching the browser graph is a BUILD-time warning, and the
   shipped stub is a silent `module.exports = {}`. The real #1612 manifestation
   is the first *call* into that empty stub throwing a TypeError at module init
   (confirmed: injecting a used `node:fs` import fails the smoke with
   "readFileSync is not a function"; an unused one ships {} and is harmless).
   So the signature machinery was dead against the bundle it runs on — dropped
   it. The smoke now asserts exactly what it verifies: the prod bundle boots and
   paints with no uncaught page error. Reworded the script header, the
   AGENTS.md bullet, and the README row to match. Console errors stay
   diagnostic-only.

2. The helper's child 'error' handler set `exited = true`, but Node emits
   'error' for a failed kill/send too (child still alive) — so `stop()`
   (guarded on `exited`) would skip the SIGTERM and orphan the launcher on the
   port. Track the error separately (`childError`); `bootFailed()` = exited ||
   childError, while `stop()` still SIGTERMs a live child.

Docs: fixed the stale AGENTS.md claim that `cd clients/web` makes
`import("playwright")` resolve (createRequire does, cwd-independently; the cd is
only for `npx playwright install`), and noted the shared
scripts/lib/prod-web-server.mjs helper on both smoke bullets.

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

* ci: also hard-fail the async half of the crash class (fourth review)

page.on("pageerror") fires for uncaught SYNCHRONOUS exceptions only. The
async twin of the #1612 shape — the same empty-stub TypeError reached
through an await/.then(), or a failed dynamic import (this app lazy-loads
chunks) — lands on the console channel as `Uncaught (in promise) …` /
`Failed to fetch dynamically imported module`, which were diagnostic-only,
so the smoke would have passed. Hard-fail console errors matching
/^Uncaught\b|Failed to fetch dynamically imported module/; every other
console error stays a printed non-fatal note. The prefixes are unambiguous
(a font/CDN miss reads "Failed to load resource: net::ERR_…"; React
warnings never start with "Uncaught"), so this can't reintroduce the flake
the earlier round fixed. Verified: an unhandled rejection at module init
now fails the smoke where it previously wouldn't.

Also prod-scope the AGENTS.md note that the "externalized" string isn't a
runtime message (under `npm run dev` Vite's stub is a Proxy that
console.warns it), and document both the sync and async failure signals.

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

* docs: reword smoke success line + README to two-channel wording (fifth review)

After the async hard-failure signature, the assertion spans both the
pageerror (sync) and console (unhandled rejection / failed dynamic import)
channels. The OK console line and the README smoke row still said "page
error"; match the two-channel wording already in the AGENTS.md bullet.

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

* ci: correct the playwright-package-resolution hint (sixth review)

The catch around requireFromWeb("playwright") advised `playwright install
--with-deps chromium`, but that installs browser binaries, not the npm
package — and the npm script already ran `playwright install` a step
earlier. A failure there means the package isn't resolvable from
clients/web, so the actionable fix is `npm install` at the repo root (the
postinstall cascade installs clients/web's devDependencies). The
chromium.launch() catch keeps its --with-deps advice (correct for the
missing-system-libraries case).

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

---------

Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
 2026-07-24 19:11:46 -04:00
Cliff Hall
373f346e5f
 		chore: close quality-gate coverage gaps (#1689) (#1766)
Some checks failed
Run install, format, lint, build, and test on every push / build (push) Has been cancelled
Details
Run install, format, lint, build, and test on every push / publish (push) Has been cancelled
Details
Run install, format, lint, build, and test on every push / publish-github-container-registry (push) Has been cancelled
Details
v2-alpha-13 
Closes #1689. Typecheck cli/tui src, add a core/ format+lint gate, and gate core/json + core/client coverage. See PR #1766 for the full review history.
 2026-07-24 17:20:12 -04:00
Cliff Hall
d76c4d5379
 		Merge pull request #1764 from modelcontextprotocol/v2/fix-1699-flaky-viewheader-animation 
test: de-flake ViewHeader connect/disconnect animation assertion (#1699)
 2026-07-24 16:17:45 -04:00
Cliff Hall
93cd0041a0
 		test: cover EMA interactive leg 1 end-to-end (#1693) (#1765) 
* test: cover EMA interactive leg 1 end-to-end (#1693)

The EMA e2e suite only drove silent EMA (seeded IdP session + legs 2–3);
the interactive leg-1 authorization-code exchange — the exact path that
broke in #1688 (a dropped RFC 9207 iss) — had no e2e coverage.

- Extend the mock IdP (ema-mock-servers.ts) with an interactive
  authorization endpoint (GET /authorize auto-approves and redirects with
  code + state + RFC 9207 iss) and an authorization_code branch on
  POST /token (client-cred + PKCE S256 check → id_token). The IdP metadata
  already advertises authorization_response_iss_parameter_supported, so the
  SDK enforces iss on the exchange.
- Add two leg-1 e2e tests: the full authorize → callback(iss) → id_token →
  resource-token → connect ladder, and the RFC 9207 guard that a missing
  callback iss rejects the exchange before any token is stored.

Part 1 of #1693 (composable test-server AS advertising + emitting iss, plus
standard-OAuth iss coverage) already landed in #1725.

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

* test: apply review nits — simplify iss regex, dedupe redirect provider

Address non-blocking review nits on #1693:
- Drop the redundant `Issuer mismatch|` alternation from the guard test's
  matcher; `/issuer/i` already matches (case-insensitive).
- Export `createStaticRedirectUrlProvider` from the shared oauth fixtures
  helper and reuse it in the EMA e2e, removing the local duplicate.

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

---------

Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
 2026-07-24 13:11:51 -04:00
cliffhall
444701c59b test: de-flake ViewHeader connect/disconnect animation assertion (#1699) 
The server name and Disconnect button are removed by two independent
Mantine Transition exits. The test waitFor'd the server-name removal, then
did a synchronous queryByRole check for the button — on a slow runner the
button's exit lags the name's, so it was still in the DOM at the check.
Wrap the button "removed" assertion in its own waitFor.

Part 2 of #1699 (TUI OAuth step-up) was already fixed in PR #1742 via
POLL_TRIES=100.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5
 2026-07-24 12:27:24 -04:00
Cliff Hall
40aaf47705
 		test: deterministic remote-server teardown to kill #1667 afterEach hang (#1763) 
* test: deterministic remote-server teardown to kill #1667 afterEach hang

The remote integration tests close their Hono HTTP server in afterEach with
`server.close(cb)`, which waits for all keep-alive sockets to go idle before
firing the callback. undici's global-fetch connection pool holds sockets open,
and under the full parallel/instrumented `npm run coverage` load they may not
go idle before the 30s hook timeout — intermittently failing the dead-transport
tests in remote-auth-branches.test.ts. The crash-on-startup / dead-session
tests are the worst case: they never disconnect, so the session and its dead
subprocess transport linger when the server closes.

Force the sockets closed with `server.closeAllConnections()` alongside close()
so the callback fires deterministically regardless of parallel load, in both
remote-auth-branches.test.ts and its sibling connect-crash.test.ts.

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

* server: unref the abandoned-session sweep timer

The transport-onclose path arms a 30s setTimeout to GC a session whose client
never opens the events stream. That sweep is best-effort cleanup, not work the
process must stay alive for — unref it so a pending timer can't keep the event
loop (or a test worker) alive for 30s after everything else has finished.

Also narrow closeAllConnections() behind an `in` guard in the two remote
teardown helpers: ServerType is Server | Http2Server and the method is
http1-only, so the direct call failed tsc.

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

* test: extract shared closeHarnessServer teardown helper

Address review feedback on #1763: the deterministic close()+closeAllConnections
teardown (and its multi-line rationale) was duplicated across
remote-auth-branches.test.ts and connect-crash.test.ts. Extract it to a shared
closeHarnessServer(server) helper so the logic and its comment live in one
place.

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

---------

Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
 2026-07-24 12:20:54 -04:00
Cliff Hall
2439ef9c3c
 		web: disable Mantine transitions in tests to kill post-teardown timer flake (#1760) (#1761)
Some checks failed
Run install, format, lint, build, and test on every push / build (push) Has been cancelled
Details
Run install, format, lint, build, and test on every push / publish (push) Has been cancelled
Details
Run install, format, lint, build, and test on every push / publish-github-container-registry (push) Has been cancelled
Details
v2-alpha-12 
* web: disable Mantine transitions in tests to kill post-teardown timer flake (#1760)

A Mantine `Transition`/`Modal` open/close `setTimeout` can fire after happy-dom
tears down `window` at the end of a run, throwing an uncaught
`ReferenceError: window is not defined` that fails the whole `coverage` job even
when every assertion passed (seen on PR #1759's CI, originating in
ServerRemoveConfirmModal.test.tsx).

Set `env="test"` on the shared `renderWithMantine` MantineProvider so Mantine
renders transitions synchronously (no timer). This removes the leak class for
every test that renders through the shared wrapper (Modals, Menus, Popovers, …).
Also apply it to ViewHeader's local forced-dark provider.

The handful of tests that assert mid-flight transition state (ViewHeader's
`data-anim="out"` crossfade checks, #1450) genuinely need real transitions, so
add an opt-in `renderWithMantineTransitions` wrapper (env="default") and route
those three tests through it — each already drives the transition to completion
(waitFor/findBy) so it doesn't reintroduce the leak. Tests that assert only the
entered state stay on the default (leak-free) wrapper.

Full `npm run ci` green; unit run reports zero unhandled errors.

Closes #1760

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

* docs: note the renderWithMantine env=test invariant in AGENTS.md (#1760)

Per review feedback: document that tests must render through
`renderWithMantine` (env="test", transition-free) rather than a hand-rolled
`MantineProvider`, and reserve `renderWithMantineTransitions` for asserting
mid-flight animation state (driving the transition to completion). Keeps future
tests from quietly reintroducing the post-teardown timer leak class.

Refs #1760

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

---------

Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
 2026-07-24 10:58:23 -04:00
Cliff Hall
0679b9c5ec
 		web: de-flake fetchTracking redaction test — exclude volatile id from leak checks (#1757) (#1759) 
The secret-redaction tests assert the serialized tracked entry does not contain
a secret value (e.g. `JSON.stringify(tracked[0]).not.toContain("shh")`).
Redaction works, but the entry carries a random request id
(`${timestamp}-${Math.random().toString(36)…}`) whose base36 tail can
coincidentally contain a short secret substring (`…sw7kshhf0` contains "shh"),
producing spurious failures that pass on re-run.

Add a `serializedWithoutId(entry)` helper that serializes the entry with `id`
overwritten to `undefined` (JSON.stringify drops undefined-valued keys), and
route all nine whole-entry `.not.toContain(secret)` leak checks through it. The
id is generated independently of any secret and can never legitimately carry
one, so excluding it keeps every deterministic field under the leak check while
removing the only source of nondeterminism. No production change.

Closes #1757


Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5

Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
 2026-07-24 10:06:38 -04:00
Cliff Hall
99ef731746
 		Refactor / justify codebase-wide 'as unknown as' casts (#1690) (#1756) 
* core: refactor serverList.ts object↔Record casts into typed helper (#1690)

Remove the four pre-existing `as unknown as Record<string, unknown>` /
`as unknown as MCPServerConfig` casts in core/mcp/serverList.ts:

- stripInspectorFields: rewrite as clone + `delete` over INSPECTOR_FIELD_KEYS.
  Every Inspector-extension key is optional on StoredMCPServer, so the result
  is still (a subtype of) MCPServerConfig with no cast.
- oauth-strip in extractSecretsFromStored: `oauth` is optional, so a plain
  `delete stripped.oauth` is legal — drop the cast.
- mcpConfigToServerEntries: route the one genuinely-needed widening through a
  single documented `toRecord(value: object)` helper. Taking `object` makes the
  single `as Record<string, unknown>` legal (no TS2352), so the double cast
  disappears from the call site.

Net: zero `as unknown as` in serverList.ts; the one remaining structural cast
lives inside the audited toRecord helper. Behavior-neutral — existing coverage
(79 serverList tests) unchanged; full `npm run ci` passes.

Closes #1690

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

* core+web+test-servers: eliminate/justify remaining `as unknown as` casts (#1690)

Codebase-wide follow-on to the serverList refactor: audit every `as unknown
as` double cast outside tests and either remove it or ensure it carries a
justifying comment (the review bar is "no *unjustified* double cast").

Removed (widened via a shared helper or narrowed to a single cast):
- Promote the serverList `toRecord(value: object)` widening helper to
  core/json/jsonUtils.ts and reuse it at core/mcp/node/config.ts (normalize
  loop), core/mcp/remote/node/server.ts (pino level dispatch), and
  clients/web/src/App.tsx (window global lookup).
- inspectorClient.rawWireRequest: type the frame as `JSONRPCRequest` and
  narrow only `params` with a single structural cast (the SDK's `_meta`-typed
  params is what blocked direct assignment).
- toolOutputValidation: SDK `outputSchema` → `JsonSchemaType` is a single cast.
- test-server-fixtures: the optional `task` handle is reachable with a single
  cast (target field is optional).

Justified in place (genuinely unavoidable — private SDK internals, partial
interface stubs, third-party/nominal bridges): the inspectorClient private-map
accesses, resourceContext + tokenAuthProvider partial `OAuthClientProvider`
stubs, the ext-apps peer bridge, modern/legacy `_requestHandlers` bypasses,
and the happy-dom `matchMedia` test mock.

Behavior-neutral. Full `npm run ci` passes (validate, ≥90 coverage gate,
smoke, Storybook).

Refs #1690

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

* address @claude review nits: consistent cast justifications + drop reflow noise (#1690)

- test-servers/src/modern-tasks.ts: add an inline justification above the
  `_requestHandlers` double cast, mirroring the sibling in
  composable-test-server.ts (was leaning on the function JSDoc only).
- Annotate the partial-`AppBridge` mock double casts in the story fixtures
  (AppsScreen ×2, AppRenderer, InspectorView) so no non-test-file double cast
  is unjustified.
- inspectorClient.ts: revert unrelated Prettier reflow hunks (protocolVersion,
  task-content array, timeout-reject wrap, reconnect guard) so the diff is the
  single rawWireRequest cast change only.

Behavior-neutral (comment/format-only). `npm run ci` green apart from a known
flaky redaction test whose assertion collided with a random request id.

Refs #1690

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

* actually revert the reflow noise this time (#1690)

The reflow reverts in cc187ea were silently undone by a `prettier --write` I
ran on the same file right after, so the four Prettier-only hunks from 6a3afa4a
were still in the diff (caught in re-review). Revert them for real now — and
without re-running Prettier on the file:

- core/mcp/inspectorClient.ts: restore the protocolVersion line, task-content
  array, timeout-reject call, and reconnect guard to their origin/v2/main form,
  so the file's diff is only the single rawWireRequest cast change.
- test-servers/src/modern-tasks.ts: restore the ModernTaskStatus union to its
  one-line form (the leading-`|` reflow was also Prettier churn), leaving only
  the new `_requestHandlers` justification comment.

Formatting-only; `npm run validate` green.

Refs #1690

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

---------

Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
 2026-07-24 09:34:32 -04:00
Cliff Hall
e788bcbaca
 		web: pin Connection Info modal header while body scrolls (#1754) (#1755) 
Same fix as #1698, applied to the Connection Info modal, which had the same
pattern: a custom header Group (title + close) inside a Stack in the modal body,
using the default `<Modal>` with no `scrollAreaComponent`. With a long server
`instructions` section (e.g. the everything server) the details overflow the
viewport and the whole modal scrolled, taking the header out of view.

Switch to the compound `Modal.Root`/`Content`/`Header`/`Body` API, move the
header into `Modal.Header` (sticky by design) and set
`scrollAreaComponent={ScrollArea.Autosize}` so only the body scrolls. The
fade-down transition is supplied app-wide by `ThemeModalRoot` (added in #1753),
so no `transitionProps` literal is needed here.

Closes #1754


Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5

Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
 2026-07-23 22:08:04 -04:00
Cliff Hall
092165476e
 		web: pin Server/Client Settings modal header while body scrolls (#1753) 
* web: pin Server/Client Settings modal header while body scrolls (#1698)

Expanding enough accordion sections grew the Server Settings modal past the
viewport, so the whole modal scrolled and carried the header (title + close)
out of view. The header was a custom Group inside the modal body, and the modal
used no `scrollAreaComponent`, so overflow was not confined to the body.

Switch both the Server Settings and Client Settings modals from the default
`<Modal>` (which always wraps children in `Modal.Body`) to the compound
`Modal.Root`/`Modal.Content`/`Modal.Header`/`Modal.Body` API, moving the
toggle/title/close row into `Modal.Header` (sticky by design) and setting
`scrollAreaComponent={ScrollArea.Autosize}` so only the body scrolls.
`Modal.Root` keeps the close-on-escape / close-on-click-outside / focus-trap /
scroll-lock / portal defaults; `transitionProps` restores the fade-down
animation the `<Modal>` wrapper otherwise supplies. Client Settings does not yet
have enough content to scroll but is kept consistent per the issue.

Closes #1698

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

* docs(pr-screenshots): Server Settings modal sticky-header proof (#1698)

Two frames from the ServerSettingsModal story (all sections expanded, short
viewport): header pinned at the top of the scroll and again scrolled down,
showing it stays put while only the body scrolls.

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

* web: move Modal.Root fade-down transition into the theme (#1698 review)

Addresses the review note on PR #1753: the `transitionProps={{ transition:
"fade-down", duration: 200 }}` literal was duplicated in both settings modals to
restore the animation `<Modal>` defaults to but `Modal.Root` doesn't inherit.
Move it to a `ThemeModalRoot` (`Modal.Root.extend`) registered under the
`ModalRoot` theme key, mirroring the existing `ThemeScrollAreaAutosize`
precedent. Both call sites drop the literal; any future compound `Modal.Root`
now animates consistently with every plain `<Modal>` from one source of truth.

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

---------

Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
 2026-07-23 19:57:03 -04:00
Cliff Hall
c2b866110d
 		tui: detect array-of-enum on items.enum alone (#1752) 
* tui: detect array-of-enum on items.enum alone (#1751)

The TUI schemaToForm array-of-enum branch was nested under an outer
`if (property.enum)` guard, so a standard array-of-enums schema —
`{ type: "array", items: { enum: [...] } }` with no top-level `enum` — never
reached it and fell through to a plain string input. The web SchemaForm already
guards on `items?.enum` alone; this aligns the TUI with it.

Restructure the enum handling so the array-of-enums case keys on
`property.type === "array" && property.items?.enum` independently of a top-level
`enum`, falling back to the single-select `property.enum` branch otherwise. The
enumNames labeling from #1691 applies unchanged.

Tests: the array-of-enum cases now use the standard `items.enum`-only shape
(dropping the redundant top-level `enum` the old nesting required), plus a case
asserting a redundant top-level `enum` still works.

Closes #1751

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

* docs(pr-screenshots): TUI array-of-enum before/after proof shots (#1751)

Before/after of the TUI tool form for an array-of-enums field with no top-level
`enum`: before the fix it renders as a plain text field (raw "m"); with the fix
it renders as a select resolving the enumNames title ("Medium").

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

---------

Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
 2026-07-23 19:12:45 -04:00
Cliff Hall
8c41dcd383
 		web+tui: honor enumNames in enum forms (#1750) 
* web+tui: honor enumNames in enum forms (#1691)

Legacy titled enums (`enum` + non-standard `enumNames`) rendered their raw
wire values instead of the human-readable titles in both the web SchemaForm
and the TUI schemaToForm builder, so a user picking "pet-1" had no idea it
meant "Cats".

Web (SchemaForm.tsx): extract a `toEnumData` helper that zips `enum` with
`enumNames` into `{ value, label }[]` when the lengths match, falling back to
bare values otherwise. Use it in both the string-enum and array multi-select
branches (the latter previously inlined the same zip).

TUI (schemaToForm.ts): add `enumNames` / `items.enumNames` to the property
interface and a `toSelectOptions` helper used by the single-select and
array-of-enum branches, labeling by `enumNames[i]` when length-matched and
falling back to `String(val)`.

Both guard on the arrays being the same length: a wrong-length zip would
mislabel options, which is worse than showing raw values. Raw values are
always submitted; only the display label changes.

Closes #1691

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

* docs(pr-screenshots): TUI enumNames before/after proof shots (#1691)

Before/after captures of the TUI tool-test form (schemaToForm -> ink-form)
against a legacy titled enum: with the fix the field resolves pet-1 -> "Cats";
before, it showed the raw "pet-1".

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

---------

Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
 2026-07-23 18:21:53 -04:00
Cliff Hall
402d7b204d
 		Server Settings: disambiguate EMA OAuth credential fields (#1692) (#1749) 
* Server Settings: disambiguate EMA OAuth credential fields (#1692)

Under enterprise-managed authorization the OAuth Settings "Client ID" /
"Client Secret" fields hold the *resource authorization server* credentials
(EMA leg 3 — the Test Client from the xaa.dev resource registration), not the
app/IdP pair configured in Client Settings. The unqualified labels sat under a
toggle naming the enterprise IdP, pulling readers toward the wrong pair.

- When enterpriseManaged is on, relabel to "Resource AS Client ID" /
  "Resource AS Client Secret" and add a description naming the resource
  authorization server and pointing the app pair back to Client Settings.
- Document the Scopes delimiter: space-separated (RFC 6749), with a warning
  that commas produce one invalid token, plus a placeholder example.

Adds tests for both label states and the scopes hint, plus an
EnterpriseManagedOAuth story.

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

* pr-screenshots: OAuth EMA field disambiguation proof shots (#1692)

Add before/after screenshots of the Server Settings → OAuth Settings fields
(EMA off = plain Client ID/Secret; EMA on = Resource AS Client ID/Secret) plus
the new space-separated Scopes hint, with a README section documenting them.

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

* Address review: genericize EMA copy, reuse enterpriseManaged const (#1692)

- Drop the hardcoded xaa.dev staging host from the resource-AS field
  description (it's a general EMA concept, not env-specific); phrase it as
  "the resource authorization server's registered client credential (EMA leg
  3)". Also trims the copy's overlap with the checkbox description.
- Reuse the existing `enterpriseManaged` const for the EMA Checkbox's `checked`
  prop instead of recomputing `settings.enterpriseManaged ?? false`.
- Update the test assertion and pr-screenshots README/EMA-on screenshot to
  match the new copy.

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

* Client Settings: mirror EMA disambiguation (IdP-qualified labels) (#1692)

The Server Settings OAuth fields now label the leg-3 resource-AS pair
explicitly; make Client Settings parallel so each modal names its pair and
points at where the other lives.

- Relabel the EMA credential fields to "IdP Client ID" / "IdP Client Secret"
  (parallel to Server Settings' "Resource AS Client ID/Secret"), so the label
  alone disambiguates without relying on the description.
- Add the reciprocal cross-reference to each description: these are the
  enterprise IdP pair (EMA legs 1–2), not the per-server resource authorization
  server credentials, which go in Server Settings → OAuth Settings.
- Update ClientSettingsForm/ClientSettingsModal tests for the new labels; add a
  pr-screenshots section + Client Settings screenshot.

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

---------

Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
 2026-07-23 16:51:24 -04:00
Cliff Hall
c51d6aabd2
 		Extensions UI: own Advertised Extensions section + 'Client Advertised Extensions' heading (#1748) 
* Settings: move Advertised Extensions to its own accordion section (#1747)

The Advertised Extensions control (added in #1739) lived inside the Options
accordion panel, wedged between "Fetch Lists One Page at a Time" and "Network
Log Size". Promote it to its own top-level "Advertised Extensions" accordion
section (after Options), so the distinct concept — which extensions the
Inspector advertises to this server in its client capabilities — reads as its
own collapsible section.

- ServerSettingsForm: add "extensions" to ServerSettingsSection; move the
  checkbox group into a new Accordion.Item value="extensions". Its description
  moves up into the panel; no behavior change (still folds into
  settings.advertisedExtensions via onAdvertisedExtensionChange).
- ServerSettingsModal: add "extensions" to allSectionsFor so expand/collapse-all
  covers it.
- Tests: the toggle now lives under the new (collapsed-by-default) section, so
  the form tests render it expanded and the modal tests open it first.

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

* Connection Info: rename "Advertised Extensions" heading to "Client Advertised Extensions"

Disambiguates the client-side column from the adjacent "Server Extensions"
column and from the (separate) Server Settings "Advertised Extensions" section.
Label-only change; the value still renders clientCapabilities.extensions.

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

* docs: refresh Connection Info screenshot for the renamed heading

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

* docs: refresh #1747 proof screenshots (fresh capture)

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

* Settings: add AdvertisedExtensions Storybook story (review)

Address the #1748 review's optional note: the new Advertised Extensions
accordion section had no story expanding it, so it wasn't documented in the
visual catalog (the default story expands only ['options']). Add an
AdvertisedExtensions story with expandedSections: ['extensions'] and a
per-server override, showing the default-on and toggled-off checkbox states.

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

---------

Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
 2026-07-23 12:10:13 -04:00
Cliff Hall
e2c75da5ce
 		Extensions: Connection Info both eras + io.modelcontextprotocol/ui advertisement (Phase 3 of #1633) (#1744) 
* Extensions: Connection Info both eras + io.modelcontextprotocol/ui advertisement (Phase 3 of #1633)

Completes the Extensions surfacing and advertises the MCP Apps UI extension.

core/mcp/extensions.ts:
- Add UI_EXTENSION_KEY ("io.modelcontextprotocol/ui") and MCP_APP_MIME_TYPE
  ("text/html;profile=mcp-app") — mirrors of ext-apps' EXTENSION_ID /
  RESOURCE_MIME_TYPE (hardcoded: the constants live on the package's /server
  subpath or an extensionless re-export that doesn't resolve under NodeNext).
- Registry entries gain an optional `advertisement` value; the builder stamps
  it (default {}). Add the `ui` entry, default-advertised with
  `{ mimeTypes: ["text/html;profile=mcp-app"] }` — the Inspector always renders
  App tools. No ext-apps cast needed: the extensions value type is `object`.

ConnectionInfoContent:
- Source server extensions from the negotiated `capabilities.extensions`
  (getServerCapabilities, era-transparent) instead of the modern-only
  discoverResult, so a LEGACY server advertising extensions renders them too.
- Add a two-column "Server Extensions" / "Advertised Extensions" section (the
  Inspector's own advertised map), mirroring the server/client capability
  columns. Drop the now-redundant Extensions row from the modern Discovery
  section (Supported versions stays).

Tests: extensions.test.ts covers the ui entry + advertisement value and the
per-extension enable/disable; ConnectionInfoContent.test.tsx covers legacy
server extensions (no discovery), the advertised-extensions column, and the
em-dash empty states. npm run ci green (the one InspectorView localStorage
failure is a known flake, passes on re-run).

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

* Extensions: address Phase 3 review (mime drift-guard, story, exact em-dashes)

- extensions-mimetype.test.ts (integration/node): assert MCP_APP_MIME_TYPE
  equals ext-apps' real RESOURCE_MIME_TYPE (imported) and that the client
  advertises it — a genuine drift guard, replacing the tautological literal
  check in the unit test (which now just asserts the type).
- ConnectionInfoContent Modern story: carry the tasks extension on
  initializeResult.capabilities too, so the Server Extensions row demonstrates
  it (the row now reads negotiated server capabilities, not discoverResult).
- Tighten the em-dash assertions from ">= N" to exact counts (3/3/2) now that
  the two extension sections em-dash by default.

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

* Extensions: drift-guard UI_EXTENSION_KEY against ext-apps EXTENSION_ID (review)

Address the Phase 3 re-review nit: the extension id was the other half of the
same drift risk (a server keys its Apps lookup on it). Import ext-apps'
EXTENSION_ID from the /server subpath in the node integration test (fine there;
the browser build is what avoids /server) and assert UI_EXTENSION_KEY === it.

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

* Extensions: clone the registry advertisement so builds never alias it (review)

Address the Phase 3 re-review's optional defensive nit: buildClientExtensions
returned the same object reference as the ADVERTISABLE_EXTENSIONS entry for the
ui advertisement, so an in-place mutation of a stamped value (e.g.
extensions[ui].mimeTypes) could corrupt the registry for later connections.
structuredClone the advertisement on stamp; add a test asserting a mutated
build doesn't leak into the next one. Nothing mutates it today — purely
defensive.

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

* Extensions: add #1740 Connection Info proof screenshots

End-to-end smoke test in the web client against the legacy
advertised-extensions-http.json server: the Connection Info modal shows the new
era-transparent Server Extensions / Advertised Extensions section, with
Advertised Extensions listing io.modelcontextprotocol/tasks + io.modelcontextprotocol/ui
(the new ui advertisement), and only ui after toggling Tasks off and reconnecting.

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

---------

Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
 2026-07-23 10:11:36 -04:00
Cliff Hall
fe295396af
 		Extensions: advertised-extensions toggle in Server Settings (Phase 2 of #1633) (#1743) 
* Extensions: advertised-extensions toggle in Server Settings (Phase 2 of #1633)

Adds the debugging knob from #1633: users choose which extensions the
Inspector advertises to a server, and a server may register different tools
in response. Builds on Phase 1's registry + advertisedExtensions option (#1738).

Settings + persistence (core):
- InspectorServerSettings.advertisedExtensions + StoredMCPServer field, wired
  through serverList's read/write converters and INSPECTOR_FIELD_KEY_MAP
  (omit-when-empty, byte-stable round-trip).
- validateSettings (remote PUT route): isBooleanRecord guard + carry a
  non-empty map; unknown/malformed rejected.
- App.tsx threads savedSettings.advertisedExtensions into the client options.

UI:
- ServerSettingsForm renders an "Advertised Extensions" group of checkboxes
  driven by ADVERTISABLE_EXTENSIONS; a per-server override wins over the
  registry default. Modal folds a toggle into settings.advertisedExtensions.

Test server + proof:
- composable-test-server extensionGatedTools: a tool registered disabled and
  enabled on `initialized` iff the client declared the mapped extension
  (getClientCapabilities). Legacy stateful leg. Threaded through
  load-config/resolve-config; advertised-extensions-http.json showcase + README.
- Integration test connects legacy with/without the tasks extension advertised
  and asserts the gated tool appears/disappears in tools/list — the acceptance
  criterion, proven end to end, not asserted.

Tests: serverList round-trip (non-empty/absent/empty), ServerSettingsForm
toggle rows, ServerSettingsModal fold, validateSettings accept/reject, and the
integration gating test. npm run ci green (per-file ≥90 gate held).

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

* Extensions: add #1739 proof screenshots (advertised-extensions toggle)

End-to-end smoke test in the web client against the legacy
advertised-extensions-http.json test server (port 3220): connecting with the
Tasks extension advertised (default) lists echo + get_weather, unchecking the
Advertised Extensions → Tasks toggle and reconnecting lists only echo — the
server's tool registration changing on the client-declared extension.

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

* Extensions: reconverge advertisedExtensions to no-override on default (review)

Address Phase 2 review observation #1: toggling an extension back to its
registry default now deletes the override key (and drops the whole map to
undefined when it empties), so the on-disk `advertisedExtensions` reconverges
to the byte-stable "no override" state instead of persisting a redundant
`{ key: <default> }`. Matches the omit-when-default policy the other settings
use. Only a value that differs from the default is persisted.

Adds a modal test asserting the reconvergence (re-checking the sole disabling
override drops advertisedExtensions to undefined).

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

---------

Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
 2026-07-22 16:23:43 -04:00
Cliff Hall
2f80958d3f
 		Extensions: first-class advertised-extensions map (Phase 1 of #1633) (#1742) 
* Extensions: first-class advertised-extensions map (Phase 1 of #1633)

Collapse the two ad-hoc `capabilities.extensions` spreads in the
InspectorClient constructor (EMA, gated on enterpriseManaged; the modern
Tasks extension, always-on) into a single builder that is the source of
truth for what the client advertises.

- core/mcp/extensions.ts: `ADVERTISABLE_EXTENSIONS` registry (id + label +
  default-on flag), the `EMA_EXTENSION_KEY` constant, and
  `buildClientExtensions()`. The registry is the shared list the Server
  Settings toggle UI (Phase 2, #1739) will drive; EMA stays a conditional
  built-in (auth-mode driven, not a free toggle).
- InspectorClientOptions.advertisedExtensions: per-extension overrides
  (present key wins over the registry default) so a future UI can toggle
  advertised extensions as a debugging knob.
- Behavior preserved: tasks defaults to advertised, so the map is non-empty
  and rides every modern request envelope; EMA advertised iff enterpriseManaged.

The raw modern-task envelope re-stamp is left untouched (it fires only on
the raw tasks channel, which requires the extension anyway).

Tests: extensions.test.ts covers the registry and every builder branch
(defaults, EMA on/off, override enable/disable, unknown key) at 100%.
The tasks-era and EMA integration suites still assert the same wire
advertisement. `advertisedExtensions` is plumbed but not yet wired to the
form (that lands in Phase 2).

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

* Extensions: address Phase 1 review (EMA-override test + envelope comment)

- extensions.test.ts: add an EMA-specific assertion that an override for
  EMA_EXTENSION_KEY cannot advertise it (auth-mode only) — documents intent
  and guards against EMA being added to ADVERTISABLE_EXTENSIONS by mistake.
- inspectorClient.ts: rewrite the withModernTaskEnvelope tasks-stamp comment
  so it stays accurate once Phase 2 lets a user disable general tasks
  advertisement — the force-stamp is load-bearing (the raw tasks/* channel
  requires the extension), not a redundant re-add.

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

* TUI: de-flake step-up OAuth frame assertions (raise poll ceiling)

The step-up authorize tests intermittently timed out on the final
"Step-up authorization succeeded" frame. Root cause is a test-infra
timing budget, not product code: waitForFrame/waitUntil polled only 25×
(25ms tick ≈ 625ms), and the step-up flow adds an extra async OAuth-runner
hop plus React commits that can exceed that under CI load with v8 coverage.

Hoist the poll count into a named POLL_TRIES = 100 (~2.5s ceiling). A poll
exits the instant its predicate is true, so a higher ceiling never slows a
passing assertion — it only widens the margin for the slow path. Verified
5× green (3 plain + 2 under --coverage), including the previously-flaky
step-up cases.

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

* TUI: prettier-format the wrapped waitForFrame signature

The POLL_TRIES change pushed waitForFrame's signature past the print width;
Prettier wraps it. (Root has no aggregate format script — format is
per-client, so this was missed locally.)

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

---------

Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
 2026-07-22 15:25:45 -04:00