Addresses Copilot's round-7 review on #2035. Two suppressed comments, both
real; each reproduced against this branch before acting.
1. `parseVarSpec` was a "strip the first `*`, trim, split on `:`" pass, so
it accepted shapes RFC 6570's varspec production does not admit --
measured on the previous commit:
x://{*id} name "id" => "x://abcdef" explode is a TRAILING modifier
x://{id*:3} name "id" => "x://abc" the two modifiers are exclusive
x://{ id } name "id" => "x://abcdef" whitespace is not a varchar
x://{a b} name "a b" => "x://1" ditto -- and it emits into the URI
`{id*:3}` is the worst of them: it silently truncated the value through a
combination the grammar forbids. Parsing is now one anchored regex --
a varname, then at most one trailing `*` or `:1-9999`.
The name set is `varchar` widened by `-` and `~`, deliberately. A name is
the one thing expansion emits into the URI WITHOUT encoding it (`;`, `?`
and `&` write `name=value`), so the rule it has to satisfy is "needs no
encoding" -- RFC 3986 unreserved plus pct-encoded, of which the RFC's own
varchar is a subset. Strict varchar would refuse `{user-id}`, which real
servers publish and which the SDK's matcher round-trips; refusing to read
a resource that demonstrably works is a worse outcome for a debugging
tool than a name one character outside the production. Everything now
rejected is a shape we would otherwise have to guess at.
2. The TUI built its "Missing required template variable(s): ..." list with
its own bare `values[name]` filter while gating on `hasRequiredValues`,
which reads own properties only. For a variable legitimately named
`constructor` the two disagreed: `({})["constructor"]` is `Object`, whose
`.length` is 1, so the list came out empty and the error named no field
at all while the submit stayed blocked. Both now come from one exported
`unmetRequiredGroups`, with `hasRequiredValues` defined in terms of it.
Also renames a test whose title still said multi-name query expressions were
left "to the SDK, which handles them" -- expansion has been owned here since
935113d1; the behavior it asserts is naming, not delegation.
npm run ci passes.
Signed-off-by: cliffhall <cliff@futurescale.com>
Ports the two behaviors PR #2033 (the parallel attempt at #1919, now closed
as a duplicate) got right and this branch did not.
1. An expression declaring no variable is a malformed template, not one
with a member to skip. RFC 6570 requires at least one varspec per
expression and admits no empty member, so `{}`, `{,}`, `{a,}`, `{?}`
and `{*}` (an explode modifier is not a name) now make the template
invalid. Skipping them was the more dangerous reading: `x://{}`
expanded to `x://` while the form rendered no inputs, so its
"everything required is filled" check was vacuously true and it
submitted a URI that is not the template the server published.
2. The read is withheld when expansion fails. `expandUriTemplate`'s
raw-template fallback exists for the preview, which runs during render
-- but the submit path used it too, so an invalid template was read
with its braces intact and the server answered with a confusing "not
found" for a defect that is not the user's. `tryExpandUriTemplate`
returns the URI or the reason as a value the caller cannot mistake for
one; the panel gates Read Resource on it and prints the reason.
This also covers a value that cannot be encoded -- an unpaired
surrogate has no UTF-8 encoding, so `encodeURIComponent` throws
`URIError` on it, and a text input can hold one via paste.
`requiredGroups` now skips an expression naming no variable: an empty
group can never be satisfied, so it would gate the form a second time on
a condition nothing can meet, and the "any one of" hint built from it
would name no fields. The accurate reason is the malformed template,
which the expansion gate reports.
npm run ci passes.
Signed-off-by: cliffhall <cliff@futurescale.com>
Two suppressed review findings.
With an override configured, the wrapper cloned and parsed every
successful application/json response — every tools/call result and every
resource payload on the direct CLI/TUI transports — only to conclude it
was not a metadata document. It now tests the request URL for the RFC
8414 / OIDC well-known paths first, so ordinary traffic costs one string
test and is returned untouched.
The read path also trusted StoredMCPServer's `string` type for the two
override fields, which a hand-edited mcp.json does not honor: the CLI and
TUI read the file directly and then `.trim()` these, so a number would
crash the server load. Guarded with cleanEndpointOverride, mirroring
cleanAuthorizationParams.
Signed-off-by: cliffhall <cliff@futurescale.com>
Addresses Copilot's round-6 review on #2035.
`toString`, `constructor`, `valueOf` and `__proto__` are all valid RFC
6570 variable names -- `varname` allows ALPHA / DIGIT / `_` / pct-encoded
-- and every lookup here was a bare `values[name]`, which finds
`Object.prototype`'s member for each of them. Measured: `({})["toString"]
!== undefined` is true with typeof "function", and `({}["constructor"] ??
"").length` is 1.
So a *blank* `{?toString}` field read as supplied and expanded a function
body into the URI instead of omitting the expression, and
`hasRequiredValues([["constructor"]], {})` returned true because
`Object.length` is 1 -- a required group satisfied by nothing.
All reads now go through an own-property `readValue`, and the web
preview's filled-check uses `Object.hasOwn`. `expandExpression` carries
the value through its filter rather than re-reading it afterwards, so the
"is it defined" test and the read cannot disagree and nothing needs a
non-null assertion.
The remaining six findings were documentation drifting behind the code,
which matters here because the comments are the argument for why this
module exists:
- The web helper still described the SDK's second-`?`-to-`&` rewrite as
something the preview relies on. That was removed last round.
- Three places still named the CLI as a consumer. It has no template form
and passes an already-expanded `--uri` to `readResource`.
- The PR description still described the superseded architecture
(expansion delegated to the SDK), i.e. the opposite of what now ships.
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot round 2 (PR #2039) surfaced no new visible comments but six
suppressed ones. Four were actionable.
`clients/tui/src/App.tsx` — a stale-completion race in the new banner:
start disconnecting server A, switch to B, then let A's disconnect
reject, and A's failure appears under B. Rapid same-server retries had
the same shape, and the stale attempt is the one most likely to reject.
Claim a monotonic attempt token before awaiting and publish only if it
is still current and the server has not changed; `handleConnect` bumps
the same counter so a connect retires an in-flight disconnect. The
rejection is still owned when superseded — just not published. The
regression test drives a deferred rejection across a server switch, and
fails without the guard.
`tsconfig.lint.json` — the root type-aware block matches
`test-servers/src/**/*.tsx` but the parser project omitted that
extension, so such a file would have failed with "not found in any of
the provided project(s)" instead of being linted. Added, and AGENTS.md
now states the invariant: the include must stay a superset of the lint
globs.
`clients/web/server/{server,vite-hono-plugin}.ts` — the `open()`
rejection branch was untested, and duplicated. Both were fixed by
removing the duplication: `openBrowser` now lives beside
`printServerBanner` in `web-server-config.ts`, with one implementation
to test. Two copies of a one-line `.catch` is two chances for one to
drift back to a bare `open(url)`. The dev plugin is not booted in tests
— it returns early under VITEST by design — but it calls the same
tested helper.
`core/mcp/remote/node/server.ts` — `closeAbortedStream`'s rejecting-close
path, the entire reason the helper exists, had no test. Exported it (as
this file already does for `requestIdForSendWait` / `mcpParamHeadersOnly`)
and covered both legs, asserting no unhandledRejection fires.
Also corrects AGENTS.md, which said each client lists "both" tsconfig
projects — true for cli/tui/launcher, but web lists four leaf projects.
Signed-off-by: cliffhall <cliff@futurescale.com>
Addresses Copilot's round-5 review on #2035 — eight suppressed comments,
all of them real.
The consequential one overturns behavior I copied from the SDK and
defended in two earlier rounds. RFC 6570 expands each expression
independently; it carries no cross-expression state. The SDK tracks
whether a query expression has already emitted and rewrites a later
`{?two}`'s `?` to `&`. That looks friendlier and is wrong, and its own
matcher says so: for `x{?one}{?two}` it expands to `x?one=1&two=2`, and
`match("x?one=1&two=2")` on that same template returns **null**, while
`match("x?one=1?two=2")` returns both variables. So the rewrite emitted a
URI the advertised template cannot match — #1919 one level up. A server
that wants a continuation advertises `{?one}{&two}`, which expands to `&`
and matches; producing that shape is the server's choice to declare, not
ours to infer.
Also:
- `{id:abc}` was silently treated as `{id}`, and a test codified it. RFC
6570's max-length is `%x31-39 0*3DIGIT`, so `{id:}`, `{id:0}`,
`{id:abc}` and `{id:10000}` are invalid templates rather than templates
with an ignorable modifier — and the SDK's constructor accepts them all,
so nothing else rejects them. Strict expansion now throws; lenient
returns the raw template. Discovery stays lenient so the panel still
renders rather than going blank.
- The TUI could submit `{a,b}` completely blank while the web panel
blocked the same request: ink-form has no way to express "any one of
these", so its members are left optional and the modal needs its own
group check. Added, with tests — it was also below the coverage gate
without them (functions 76.92%).
- Added the integration test the config never had: it resolves the
checked-in config and drives all four reads over a real transport, so a
misspelt preset fails there rather than only when someone runs the repro
by hand.
- "All three clients" was false. The CLI has no template form and its
`resources/read` passes the already-expanded `--uri` straight to
`readResource`, so nothing here runs for it. Corrected in the README,
AGENTS.md, and the module comment, and dropped a docblock still
describing the SDK-delegation path removed last round.
Signed-off-by: cliffhall <cliff@futurescale.com>
The overrides are per-server configuration for the *resource* server's
authorization server, but the fetch wrapper that applies them is shared
with the enterprise-managed (EMA) flow, whose IdP OIDC discovery runs
through the same fetch and satisfies the metadata predicate. An
authorization override could therefore redirect the IdP login to the
resource AS, and a token override could send the IdP code or refresh
token there.
OAuthManager.getEndpointOverrides() now returns nothing when the server
is enterprise-managed — the same boundary redirectToExternalAuthorization
draws for the custom authorization parameters (#2018) — with a regression
test, the note surfaced on both form fields, and the rule documented.
Signed-off-by: cliffhall <cliff@futurescale.com>
Addresses Copilot's round-4 review on #2035 — again reported as "no new
comments" while carrying three suppressed ones. All three reproduced.
1. The TUI form still named its fields from the SDK. `uriTemplateToForm`
read `UriTemplate.variableNames`, which mangles a name: `{;id}` yields
";id" and `{id:3}` yields "id:3". The form therefore submitted
`{ ";id": "7" }` while the shared expander looked up `id`, found
nothing, and dropped the expression — the value vanished silently.
This is the finding that mattered most, because it falsified the claim
this change is built on. Moving the expander into core only makes the
clients agree if each client's FORM derives its names from the same
parser: a form submits under the names it rendered. The TUI now reads
`templateVariables`, and the SDK template is constructed only to
validate, preserving the existing malformed-template diagnostic.
Its `required` flag comes from `requiredGroups`, and only a variable
that is the sole member of a non-omittable expression is marked: ink-
form cannot express "any one of these", so marking every member of
`{a,b}` required would refuse input the expander accepts.
2. `encodeURIComponent` is not RFC 3986's unreserved set — it leaves the
sub-delims !'()* bare, which RFC 6570 requires encoded for every
operator except + and #.
Fixing that settled a design question left open in the previous round.
With two encoders, the SAME value encoded differently depending on
whether its expression happened to carry a modifier, since only the
modifier pushed it onto our path. So delegation is gone entirely: one
expander, one set of rules. The SDK's `UriTemplate` is still
constructed, but only to validate a template.
3. The "Any one of: a, b" hint could contradict the disabled submit
button. A name can sit in a singleton required group AND a shared one
(`x://{a}/{a,b}`), where the singleton demands that exact field. The
hint is suppressed in that case.
Signed-off-by: cliffhall <cliff@futurescale.com>
A floated promise is invisible at review time — the call reads like an
awaited one minus four characters — and the unhandled rejection it
produces surfaces in a different test, in a different file, as a stack
pointing at SDK internals. In #1947 two un-held `callTool` calls made
`npm run ci` unpassable and took a full investigation to attribute; the
fix was two lines. `@typescript-eslint/no-floating-promises` catches the
class outright, so enable it at `error` everywhere.
The rule is type-aware and `tseslint.configs.recommended` is not, so each
scope's config gains a parser project. The four clients name their own
tsconfig projects (both of them — `src` and `__tests__`/`server` live in
different ones). The root scope has no tsconfig at all, so add
`tsconfig.lint.json`: a lint-only project covering `core/**`,
`test-servers/src/**`, and `vitest.shared.mts`, emitting nothing and
gating nothing. It sets `moduleResolution: bundler` deliberately — under
NodeNext `core/`'s extensionless imports fail to resolve, every import
degrades to `any`, and the rule silently stops seeing promises at all.
Measured surface, and the fixes:
- core (2) — `stream.close()` in two Hono `onAbort` listeners. Hono
invokes subscribers with a bare `subscriber()`, so a promise returned
from the listener is floated one layer up; `closeAbortedStream` owns
the rejection where it can actually be owned.
- tui (5) — `handleDisconnect` had no catch of its own and its only
caller is a key handler that cannot await, so a failing disconnect was
a live unhandled rejection; it now surfaces the message the way
`handleConnect` does. The rest are `void` at callers that genuinely
cannot await, over callees that already own their failures.
- web (92) — 83 are Storybook `expect(...)`, which is instrumented and
returns a promise, now awaited (`scrollAreaStoryAssertions` is async
for the same reason). Two tests were asserting against a provider call
they never awaited. `open(url)` in both web servers now warns instead
of escaping a synchronous callback.
- cli, launcher — already clean.
Type-aware linting costs time: `clients/web`'s `eslint .` goes from ~8s
to ~19s, inside `validate`. Narrowing the loaded projects is the lever if
that needs reducing later; dropping the rule is not.
Closes#1959
Signed-off-by: cliffhall <cliff@futurescale.com>
Adds two optional per-server fields — `oauth.authorizationUrl` and
`oauth.tokenUrl` — that override the endpoints authorization-server
metadata discovery resolved, so a server can be pointed at a development
or staging authorization server without changing what it advertises.
SDK v2 routes both endpoints through the discovered metadata document and
neither through OAuthClientProvider, so the override is applied by
wrapping the client's base fetch and patching the metadata document in
flight (core/auth/endpointOverrides.ts). Wrapping the base fetch — rather
than only the auth fetch — also covers the discovery the SDK runs from
inside the transport on the 401/refresh path.
The fields round-trip through mcp.json, the /api/servers routes, the
runner (CLI/TUI), and the Server Settings → Authorization panel, where a
value that is not an absolute http(s) URL is flagged inline; at runtime
such a value is dropped with a single warning rather than failing the
connection.
Signed-off-by: cliffhall <cliff@futurescale.com>
Addresses Copilot's round-3 review on #2035. It again reported "no new
comments" while carrying two suppressed ones; both reproduced.
1. Requiredness could not live on a variable at all. Deduplication kept the
first occurrence's group, so in `x{?a}{?b}{a,b}` both names ended up
required with singleton groups and the form demanded both -- while the
SDK expands that template with only `a` to "x?a=11". Widening the stored
group would not have been enough either: in `{a,b}{a,c}`, filling `b` and
`c` satisfies both expressions, which no per-variable flag can express.
So requiredness is now returned per expression by `requiredGroups`, and
`hasRequiredValues` asks that each group be satisfied by any one of its
names. `TemplateVariable.groupNames` is gone rather than left as a field
that quietly means something narrower than it reads; `required` remains,
documented as driving the "Optional" marker and nothing else.
2. `encodeURI` is not the allow-reserved encoder `+` and `#` call for. It
escapes `[` and `]`, which are reserved and must survive, and it escapes
`%`, so an already-encoded value is double-encoded. Measured:
"[::1]" -> "%5B::1%5D" and "%2F" -> "%252F". Both corrupt the URI rather
than merely over-escaping it -- an IPv6 literal or a pre-encoded path
reaches the server altered, which is the same class of defect #1919 is
about.
Added an RFC 6570 3.2.1 encoder that preserves reserved characters and
existing pct-triplets, splitting on `%XX` so a lone `%` is still encoded
to `%25`, and matching with the `u` flag so an astral character is
encoded whole. `+` and `#` expressions are now taken over even for a
single name, so both expansion paths agree on what those operators mean.
Signed-off-by: cliffhall <cliff@futurescale.com>
Addresses Copilot's round-2 review on #2035. It reported "no new comments"
but carried three *suppressed* ones; all three reproduced against the
pinned SDK.
1. The `;` path-parameter operator is absent from the SDK's operator list,
so `{;id}` parsed as a variable literally named ";id" and expanded to
nothing. Added the operator and its named expansion (`;a=1;b=2`).
2. An RFC 6570 prefix modifier was folded into the variable name: `{id:3}`
yielded a variable called "id:3" and expanded to nothing. Varspecs are
now parsed properly and the value truncated before encoding. Truncation
is by code point, since `String.prototype.slice` counts UTF-16 units and
would split an astral character into a lone surrogate.
For both of these the wrong URI is the lesser problem: a form has to
*name* the variables it asks the user to fill, so the panel was
rendering fields labelled `;id` and `id:3` that nobody could use.
3. Requiredness was applied per variable, but it is a property of the
*expression*: RFC 6570 drops undefined names from a multi-name
expression, so `{a,b}` with only `a` filled expands to `a`'s value --
the SDK does this too. The panel was refusing input the expander would
have accepted. `hasRequiredValues` now encodes "any one name in a
required group suffices", and such a field reads "Any one of: a, b"
rather than falsely claiming each is mandatory.
Two structural consequences, each pinned by a test:
- Takeover is now per TEMPLATE rather than per expression. Splicing
corrected fragments into a template the SDK re-expands would leave its
cross-expression `?`-to-`&` rewrite blind to the fragments already
resolved.
- Expansion is split into a strict variant that throws and a lenient one
that returns the raw template. `readResourceFromTemplate` wraps the
thrown error with the template name -- three pre-existing integration
tests assert that -- while the form must not throw on a server-supplied
template, since that would take out the panel on render. The strict
variant constructs the SDK template unconditionally, because that
construction is what validates syntax: otherwise `x://{;a}{b,c` would
take the own-expansion path and its unclosed tail would pass as literal
text with nothing objecting.
Signed-off-by: cliffhall <cliff@futurescale.com>
Addresses Copilot's review on #2035. All three findings reproduced against
the pinned SDK before acting.
1. `#` was misclassified as required. Measured: `x://a{#frag}` with no
`frag` expands to exactly `x://a`, a well-formed URI naming a real
resource -- unlike `{+path}` (`x://a/`) or a simple `{userId}`
(`file:///users//profile`), which leave an empty path segment. Moved `#`
into the omittable set; the required cases now assert what the URI
*becomes* when blank, so the rule is checked rather than asserted.
2. Delegating to the SDK did not actually give RFC 6570 expansion.
`UriTemplate.expandPart` takes an early `names.length > 1` branch that
raw-joins values, skipping both `encodeValue` and the operator prefix:
`x://{a,b}` with `a = "foo/bar"` expands to `x://foo/bar,q` -- the very
unencoded-slash defect this PR is about -- and `x://a{/p,q}` to
`x://ax y,z`. Only `?`/`&` are correct, being dispatched earlier.
Fixing that in the web client alone would have left the TUI and CLI
wrong, since `readResourceFromTemplate` expands through the same class.
So parse/classify/expand now live in `core/mcp/uriTemplate.ts` and both
call sites use it. The correction is surgical: a multi-name non-query
expression is expanded here and spliced in as literal text before the
SDK sees the template (safe -- both encoders escape `{`/`}`), while
every single-name and query expression still goes through the SDK
untouched. The preview applies the same correction, so it cannot
promise a URI that submitting would not send.
3. The showcase promised a blank `{?topic}` read that did not work.
`UriTemplate.match()` compiles `{?topic}` to a *required*
`\?topic=([^&]+)`, so `match("foobar://events")` returns null and the
read 404s. A real server exposes the unfiltered collection as its own
resource; the showcase now registers `foobar://events` so the
documented step resolves. Verified end to end through the CLI.
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot round 5 — three of four findings implemented.
- `cleanAuthorizationParams` validates the field as it comes off disk. Only the
web client's /api/servers route checked the shape; the CLI and TUI read
mcp.json directly, so `authorizationParams: "oops"` enumerated as the
character-indexed pairs 0=o, 1=o, … and `{ audience: 5 }` reached
URLSearchParams.set, which stringifies it — either way the Inspector sent
query parameters the user never wrote. Now validated in shared code so all
three clients enforce the documented string→string contract, warning on each
rejection. Follows `cleanRoots`, which guards the same class for `roots`.
- The form's description now states that these parameters are not sent while
Enterprise-managed authorization is on. The EMA leg authorizes against the
enterprise IdP — a different authorization server — and deliberately sends
none of them, so the copy was presenting a configured-but-inert value as
active.
- The /api/servers malformed-`oauth` warning listed an expected shape omitting
`onInsufficientScope`, which the validator accepts and the docs describe, so
it told users a valid config was malformed.
Ten new cases, all verified to fail against the pre-fix sources.
Declined: recording the authorize URL in the Network tab (repeat of round 2) —
that URL is reached by a top-level browser navigation rather than a tracked
fetch, so it has never appeared there for any authorization request; adding a
synthetic entry changes every OAuth flow and is a product decision for a
maintainer, not a change to make inside this PR.
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot round 4 (one suppressed finding, and a real bug).
`extractSecretsFromStored` rebuilt the stripped `oauth` object from an
allow-list of the non-secret fields. That list had gone stale: it never named
`onInsufficientScope`, so a server carrying both a client secret and a
non-default SEP-2350 policy silently lost the policy on save, falling back to
`reauthorize`. This predates #2018 — the PR only extended the same list with
`authorizationParams` — but the list is the defect, so fix the class rather
than add a third entry to it.
The stripped entry now subtracts the one secret instead of enumerating what to
keep, so any field added to `oauth` later is preserved by construction.
Regression test verified to fail against the allow-list version:
`expected { clientId: 'cid' } to deeply equal { clientId: 'cid', onInsufficientScope: 'throw' }`.
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot round 10.
The scan no longer publishes its malformed entries into the tools/list report
at all. Round 9 only stopped it overwriting the aggregate's; the remaining
case is unsalvageable for a stronger reason than 'two different requests':
- a FAILED strict aggregate runs salvage, which writes the report itself;
- a SUCCESSFUL one means every entry parsed, so the rendered list contains no
malformed entries.
So by the time the scan finds something, either the aggregate already reported
it with indices matching the screen, or the aggregate succeeded and what the
scan found is provably not in the list being displayed. Publishing could only
ever name entries the user cannot see.
Nothing is lost: listToolsForScan still marks its own refused response, so the
finding surfaces on the Protocol entry for the exchange that carried it, where
it is true. The per-page accumulation and raw-entry offset existed only to feed
the removed publish, and go with it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RDXCSrnSBV7vZfxycG5Z6E
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot round 9.
The exclusion scan is a separate, uncached tools/list exchange, so its entries
need not be the ones rendered — the server can answer differently, and a
cache-served aggregate is a different list version. It was overwriting the
aggregate's report wholesale, replacing accurate indices with indices into a
list nobody is looking at. The aggregate now owns the report; the scan
publishes only when the aggregate left nothing, which is the cache-served case
that made it publish at all (round 2).
A conforming re-fetch no longer swallows the mark: the list can change between
the strict page and the lenient one, and a clean second response says nothing
about the first, which was refused. It is marked with the original decode
error when there is nothing per-item to name.
The scan's fallback re-fetch now goes through invokeMcpClient, like the strict
call beside it and like salvageList's walk, so an auth challenge on that
request can still be recovered instead of being swallowed by listAllTools's
best-effort scan catch.
Also fixes listTools dropping an empty-string cursor (truthiness rather than
!== undefined) — its own fallback leg already preserved it, so the two legs of
one scan page disagreed about which page they had fetched.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RDXCSrnSBV7vZfxycG5Z6E
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot round 8.
The era contract validated with the wire schema but returned the raw input,
so the wire schema's normalization was discarded — both era schemas are
z.object, which strips unknown top-level fields as the strict codec does, so
salvaged tools could carry fields the strict path drops. The parsed output is
now what gets normalized and returned. This also simplifies the bridge: issues
come straight from the wire parse with their paths intact, so the custom-issue
forwarding (and its early describeIssue application) is gone.
The malformed-entry report is written by the aggregate walk's salvage, but in
paginated mode the tools/prompts/resources panels render the paged stores,
which neither write nor clear it — leaving a stale warning above a freshly
fetched page it does not describe, with indices into an aggregate the user is
no longer looking at. Those reports are now suppressed in paginated mode;
resource templates keep theirs, having no paged store. Suppressing rather than
adding per-page salvage: pointing at the wrong entry costs more than staying
quiet, and per-page salvage is a feature, not a display fix.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RDXCSrnSBV7vZfxycG5Z6E
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot round 7.
The era contract was derived from the neutral ToolSchema plus one override,
which diverged from the real wire schemas both ways. Measured against SDK
2.0.0 on a real connection:
- modern accepts `inputSchema.properties: 5` and `required: [1]`; ours
rejected them, so a malformed sibling made salvage DROP a conforming tool
and report it as malformed — a false accusation from the screen whose job
is to say which entry is wrong.
- legacy rejects `outputSchema.properties: 5` / `required: [1]`; ours
accepted them, readmitting entries the strict path refused.
- modern rejects a non-string `inputSchema.$schema`, legacy accepts it;
ours accepted on both.
Each era is now expressed as its own wire schema, bridged to Tool by
`eraToolItemSchema`. The bridge exists because both eras accept values the
exported Tool type cannot describe (the SDK returns them typed as Tool
anyway), so the wire schema's output type is irrelevant and z.custom<Tool>
supplies the caller's type — which is what avoids an `as unknown as` here.
Forwarded issues keep their path so the report still names the field.
The full measured matrix is pinned table-driven in listSalvage-era.test.ts:
each row asserts the SDK's verdict AND that our contract agrees, so a
divergence that closes or widens fails loudly.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RDXCSrnSBV7vZfxycG5Z6E
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot round 6.
Bound both fallback walks with LIST_MAX_PAGES, which is now also passed to
the SDK client as `listMaxPages` so the strict aggregate and its fallback
share one value by construction. A server emitting endlessly UNIQUE cursors
never trips the repeated-cursor guard, so the salvage re-walk and the
SEP-2243 exclusion scan would page and accumulate without bound. Hitting the
cap aborts rather than returning a truncated list as a complete one.
Mark the exclusion scan's refused response on every fallback-failure path.
`salvageList` can rethrow bare because ManagedListState marks from the error
it receives; the scan has no such backstop — listAllTools catches and logs a
failing scan by design — so an unmarked rethrow left a response the client
demonstrably refused rendering as a clean exchange.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RDXCSrnSBV7vZfxycG5Z6E
Signed-off-by: cliffhall <cliff@futurescale.com>
Real deployments gate IdP selection, prompt behavior, or audience on a query
parameter the OAuth/OIDC core specs never standardized (Keycloak's
`kc_idp_hint`, OIDC's `login_hint` / `prompt` / `acr_values`, Auth0's
`audience`), and none of them could be driven from the Inspector.
The Inspector never builds the authorize URL — the SDK's `auth()` does, and it
exposes no hook for extra parameters — so the merge happens at the one seam
that sees the finished URL: `BaseOAuthClientProvider.redirectToAuthorization`.
It lives in `core/`, so web, CLI, and TUI all inherit it.
- `core/auth/authorizationParams.ts`: the reserved-key list (`client_id`,
`code_challenge`, `code_challenge_method`, `redirect_uri`, `resource`,
`response_type`, `scope`, `state`), the key validator the form renders
inline, and the merge itself. Reserved keys are dropped with a warning
rather than silently overriding PKCE / the state binding / RFC 8707;
blank-key rows are skipped; the caller's URL is never mutated.
- Applied to the authorization request only — nothing wires it into the token
request.
- Persisted as `oauth.authorizationParams` (a string record) on the per-server
config beside `scopes`, edited as key/value rows in Server Settings ->
Authorization ("Additional authorization parameters"), with an inline error
per offending row plus a section-level warning.
- The enterprise-managed (EMA) leg redirects to a *different* authorization
server (the enterprise IdP), so it goes through the explicit
`redirectToExternalAuthorization` entry point, which skips the merge — a
`kc_idp_hint` or `audience` meant for the MCP server's AS must never be
appended to the IdP's authorize URL.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RDXCSrnSBV7vZfxycG5Z6E
Signed-off-by: cliffhall <cliff@futurescale.com>
Both findings are the same shape: the fallback accepting what the strict path
would not.
- `ModernResultEnvelopeSchema` typed `ttlMs` as any number, where the codec
requires a non-negative integer. `ttlMs: -1` (or `0.5`) plus one bad entry
would salvage, reporting the entry and hiding the envelope violation. Tightened
to `z.int().min(0)`.
- The item schema was the SDK's era-NEUTRAL `ToolSchema`, and the negotiated
era's wire schema is stricter in ways the SDK does not export. Verified
against SDK 2.0.0 on a real connection: on legacy, a tool with
`outputSchema: { type: "array" }` is refused by the strict aggregate but
accepted by the neutral schema; on modern, `execution` is STRIPPED by the
strict path but kept by the neutral one. Either way a salvaged list could show
— and let the user call — a tool the connection's era says isn't valid.
`toolItemSchemaForEra` closes both. Replicating rules that live in the SDK's
internals is a real cost; `listSalvage-era.test.ts` is what makes it safe to
pay, pinning each rule to the strict path's OBSERVED behavior so a divergence
that closes or widens fails loudly instead of rotting.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HDdo1rNRVQnVRRdsqSrvG3
Signed-off-by: cliffhall <cliff@futurescale.com>
Two findings, both about the fallback accepting more than it should.
- The lenient page schema validated pagination and nothing else, so a response
carrying a malformed entry AND an unrelated top-level violation salvaged: the
entry was reported and the other violation vanished with the strict error that
caught it. The page schema is now DERIVED from the method's real result schema
with only its item array relaxed (`lenientListPageSchema`), so every other
field — `_meta`, `nextCursor`, anything a future revision adds — is still
validated. The modern era envelope the raw-wire path bypasses (`resultType`,
`ttlMs`, `cacheScope`) is checked explicitly for the same reason.
Salvage is also narrowed to `InvalidResult`: `UnsupportedResultType` means the
codec doesn't recognize what KIND of frame arrived, so it isn't a list with a
bad entry and reporting an item-level reason would hide a protocol failure.
Marking the Protocol entry still applies to both, hence two predicates.
- On every rethrow path the re-walk had already answered the method, so the
caller's own `markResponseRejected` (in `ManagedListState.refresh`) landed on
the lenient page that succeeded and left the refused response clean. The
correlation is restored before rethrowing — deleted when nothing had answered
before, so the outer mark is a no-op rather than a lie.
`nextCursorOf` / `rawItemsOf` narrow the page's fields instead of casting: the
runtime-keyed `extend()` loses the shape, and asserting it back would be exactly
the unjustified cast the last round removed.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HDdo1rNRVQnVRRdsqSrvG3
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot review: the option's doc still promised the level is set "if server
supports logging" — after the era gate that is exactly the case which now
silently does nothing, since a modern server can advertise `logging` and still
ignore the option.
Document it as legacy-only and point modern callers at the per-request
`io.modelcontextprotocol/logLevel` opt-in (`modernLogLevel` /
`setModernLogLevel()`).
Signed-off-by: cliffhall <cliff@futurescale.com>
The visible review was clean; all five findings were in the suppressed block,
two of them real defects.
- The exclusion scan's aggregate offset was wrong twice over: `excluded.length`
counts only invalid-header tools, and on page one it also folded in the
current page's valid count. A reported index therefore pointed at a position
that doesn't exist — worst for an unlabeled entry, whose index is all the user
has. Replaced with a raw-entry offset advanced by each page's valid + malformed
counts.
- The scan's own refused response was never marked. It is a distinct exchange
from the aggregate's, and the lenient re-fetch moves the method correlation
off it, so its Protocol entry rendered as a clean success the client had in
fact refused. Marked with the captured id, as `salvageList` already does.
- Three double casts in the test fixture are gone: `ListPage` now models raw
wire data (`items: unknown`, `nextCursor?: unknown`), which is what a server
built to emit spec-forbidden shapes actually serves. Declaring the validated
type and casting malformed values through it asserted the opposite of what
these tests exist to check. The cursor map narrows to `string` at the one
place a cursor addresses a page.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HDdo1rNRVQnVRRdsqSrvG3
Signed-off-by: cliffhall <cliff@futurescale.com>
All four findings taken (one visible, three in the suppressed block), plus the
`excludedTools` sibling leak the round-1 reply had deferred:
- `rawItemsOf` read a missing or non-array list member as an empty page, so a
top-level violation (`{ tools: "invalid" }`) on a later page was discarded
and the call returned a silently truncated list. It now returns `undefined`
for "not a list at all", and the walk rethrows the original strict error.
- A failure of the re-walk ITSELF (a malformed `nextCursor`, a connection lost
mid-walk) escaped in place of the original error, whose response is the one
`rejectedResponseId` was captured for. The walk is now wrapped: the original
surfaces, the secondary is logged so it stays diagnosable.
- The exclusion scan's lenient fallback discarded its malformed entries despite
the comment claiming they were reported. Observable when the aggregate is
served from cache: the salvage never runs, so nothing else reports them. The
scan now returns them and `refreshExcludedTools` reports them — only ever
adding, so a clean scan can't clear what the salvage path just wrote.
- An empty-string cursor is a valid cursor, and `cursor ? {cursor} : {}` dropped
it, re-requesting page one and duplicating its entries until the
repeated-cursor guard fired. Both walks now test `!== undefined`.
- `excludedTools` had the same session leak fixed for `malformedListItems` last
round: cleared only on an explicit `disconnect()`, so the `onerror`-with-no-
`onclose` route carried the previous server's set into the next connection.
Cleared on the start-clean path too — safe because every `listAllTools`
recomputes it, so this can only drop a stale set.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HDdo1rNRVQnVRRdsqSrvG3
Signed-off-by: cliffhall <cliff@futurescale.com>
`logging/setLevel` is a legacy-era method, but connect() called it on any
server advertising the `logging` capability, era regardless. The modern wire
era rejects it outright, and because the call sits in the post-handshake
sequence that rejection failed the *connection* — so the CLI (the one caller
passing `initialLoggingLevel`, hardcoded to "debug") could not run any method
at all against a modern server that advertised logging.
Gate it on `protocolEra !== "modern"`. Modern has no session-scoped level to
set: the equivalent is the per-request `io.modelcontextprotocol/logLevel`
`_meta` opt-in, already driven by `modernLogLevel` from the server settings.
So modern skips the call rather than translating it. Legacy is unchanged.
The guard lives next to the capability check in inspectorClient.ts rather
than in the CLI, so any future caller passing `initialLoggingLevel` is
covered too.
Signed-off-by: cliffhall <cliff@futurescale.com>
Three real defects it caught, plus the tests it asked for:
- The strict `tools/list` aggregate is filtered by the SDK for invalid
`x-mcp-header` annotations (SEP-2243), but the salvage path was not: one
ordinary malformed tool would trip the fallback and readmit every
invalid-header tool beside it. Salvaged tools now go through the same scan.
- `markResponseRejected` resolves "the last response received for this method",
and the re-walk answers that same method — so the mark landed on a salvage
page that succeeded while the actually-invalid response rendered clean. The
correlation id is now captured before the re-walk.
- The report was cleared only on an explicit `disconnect()`. The route out that
tears down nothing (`onerror` with no `onclose`) carried it into the next
connection, so it is now cleared on the start-clean `resetSessionState()` path
as well.
Fixing the first exposed two more, both found by the new test:
- The modern (2026-07-28) codec validates a result against the spec schema
before the caller's, so a lenient `client.request` is rejected exactly as the
strict one was — there was no salvage on the modern leg at all. It now goes
through the raw-wire channel that exists for the modern `tasks/*` methods.
`withModernEnvelope` is split out of `withModernTaskEnvelope` so a list
request doesn't claim the tasks extension the user may have disabled.
- A malformed tool also blanked the SEP-2243 excluded set, leaving a tool that
vanished for an invalid header with no explanation — #1909's own complaint one
level down. The scan walk now tolerates a malformed sibling.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HDdo1rNRVQnVRRdsqSrvG3
Signed-off-by: cliffhall <cliff@futurescale.com>
The SDK validates a list result as a whole, so a single non-conforming entry
rejected the entire response and the panel showed "Couldn't load resources"
with zero items — the exact opposite of what a debugging tool should do with a
server that is 90% right. Reported against a PHP server whose empty
`annotations` object reaches the wire as `[]` rather than `{}`.
`listAll*` now falls back to a lenient re-walk when — and only when — the SDK
rejects a result it received (`InvalidResult` / `UnsupportedResultType`), keeps
the entries that validate, and reports the rest. A conforming server takes the
strict, cache-aware path exactly as before.
Salvaging is not accepting: the dropped entries are named with their reason in
a warning above the list, and the Protocol entry is still marked rejected, so
the wire truth survives. When every entry validates, the original error is
rethrown rather than swallowed — the rejection was about something the
per-item pass cannot explain.
`isClientDecodeRejection` moves to the new module and is now shared with
managedListState, which had the only copy.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HDdo1rNRVQnVRRdsqSrvG3
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot review round 4: every await before the `connect` dispatch is a window
for a `disconnect()` or a transport close to overtake the connect, and the
listen round-trip widened it. Announcing anyway restarts every managed list
refresh against a session being torn down or already dead.
The dispatch is now gated on `status === "connected" && !disconnecting` — the
second because an explicit teardown claims ownership before it awaits
`client.close()`, leaving the status untouched until that block finishes.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HDdo1rNRVQnVRRdsqSrvG3
Copilot review round 3 (no new code findings, three stale-comment ones):
- `openModernListenStreamOnConnect`'s generation guard was justified by a
`connect` listener racing it — which the previous commit made impossible by
dispatching `connect` after this call. The guard is still right, for reasons
that survive the reorder: `statusChange` has already fired, and a concurrent
`subscribeToResource` or `disconnect()` (whose `resetSubscriptionStream` bumps
the generation) can supersede it.
- `reconcileModernStreamStateAfterFailedRefresh`'s JSDoc still described its
branches as "nothing subscribed" vs "URIs live"; they are the empty vs
non-empty *filter* now, which differ exactly on the listChanged-only path.
- The superseded-failure test carried the same stale `connect` rationale.
Comments only; no behavior change.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HDdo1rNRVQnVRRdsqSrvG3
Copilot review round 2: opening the stream after `dispatchTypedEvent("connect")`
left a real gap. The managed list states start their initial `refresh()` from
that event, so `tools/list` could go out ahead of `subscriptions/listen` — and a
list the server changed in that window would notify nobody, leaving the UI
stale with no way to notice.
The handler registration, the initial logging level, and the stream open now all
run before the `connect` dispatch, so no consumer can act on the connection
until the notification channel is established (or its retry armed). Costs one
listen round-trip on a modern connect.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HDdo1rNRVQnVRRdsqSrvG3
Copilot review: `openModernListenStreamOnConnect`'s catch reconciled
unconditionally, unlike the subscribe/unsubscribe call sites. The `connect`
event is dispatched before the stream opens, so a listener subscribing to a
resource can start and acknowledge a newer refresh while this one is still
awaiting its `listen()` — and reconciling anyway would arm a reconnect that
tears down a healthy stream. Apply the same generation test.
Also consolidates the tests' private-member access onto the file's existing
`internals()` helper (one justified double cast, hoisted and documented) and
makes the failure stub bump the generation the way the real method does.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HDdo1rNRVQnVRRdsqSrvG3
On the modern (2026-07-28) era every server→client notification rides the
`subscriptions/listen` stream, but the Inspector only ever opened that stream
from a resource-subscribe click. A tools-only server advertising
`tools.listChanged` therefore had no path to an open stream, so
`notifications/tools/list_changed` could never arrive — even though
`buildSubscriptionFilter()` already modelled the opt-in.
The trigger now matches the filter it builds: the stream opens (and stays open,
and reconnects) whenever the built filter carries anything — subscribed URIs or
an enabled list-change opt-in the server advertises. That mirrors the SDK's own
`ClientOptions.listChanged` auto-open, which opens on a non-empty config ∩
capability intersection. `connect()` opens it once the handlers are registered;
a failure there is handed to the reconnect machinery rather than failing the
connect.
`ResourceSubscriptionStreamState.active` deliberately keeps its narrower
meaning — "at least one URI is subscribed" — because it drives the Subscriptions
section's badge, which has nothing to report for a listChanged-only stream. That
also preserves the invariant the rest of the file is written against: an empty
subscribed set is never announced alongside an `active` stream.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HDdo1rNRVQnVRRdsqSrvG3
Copilot review follow-ups:
- `applyProtocolVersion` took the value straight from an unvalidated JSON
body and went to `RegExp.test`, which coerces — `123` and `true` both
matched the token pattern and reached `Transport.setProtocolVersion` with
the wrong type. It now takes `unknown` and checks `typeof === "string"`
first, with cases for the coercible values.
- Replace `as unknown as Transport` in the new session fixtures with a
`makeTransport()` helper returning a structurally valid `Transport`, so an
interface change stays visible to the tests.
- Drop `as unknown as typeof fetch` in the new transport test —
`vi.fn<typeof fetch>()` is already the right type.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0119LaPQ3v4NyBK3ZTdM9j84
The web client's SDK Client owns the `initialize` handshake, so the
`setProtocolVersion()` call it makes after initialize lands on
RemoteClientTransport — a pass-through to the Node backend, not the real
HTTP transport. The backend's StreamableHTTPClientTransport therefore never
learned the negotiated version and stamped no `Mcp-Protocol-Version` on any
request after initialize, starting with `notifications/initialized`. A
stateful server that requires the header rejects that notification with
HTTP 400.
RemoteClientTransport now records the version and rides it on every
`/api/mcp/send`; the backend applies it to its transport before the send, so
that request and everything the transport issues afterwards (the standalone
SSE GET, the session DELETE) carry the header. The version is validated
against a token pattern on the backend so a client can't push arbitrary
bytes into an upstream header.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0119LaPQ3v4NyBK3ZTdM9j84
Two fixes from Copilot's review of #1953.
useManagedListError re-synced React state from the `state` prop inside a
useEffect, which renders one frame carrying the PREVIOUS store's error
after `state` changes (switching servers) before the effect corrects it —
the pattern AGENTS.md forbids. useSyncExternalStore reads the snapshot
during render, so a store swap lands in the same frame, and it also
closes the window where an error recorded between render and subscribe
was missed. The snapshot returns the stored Error instance (or null), so
it is referentially stable as the API requires.
The `markResponseRejected` correlation maps were never cleared. Ids of
requests that never got a response — a timeout, a dropped connection —
are the only entries trackResponse can't remove, so they accumulated
across reconnects. Cleared on the start-clean connect path, per the
convention documented on resetSessionState (one route out, `onerror`
with no `onclose`, tears down nothing).
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SW1p8E2uiyyLx4RKwrwSrt
markResponseRejected was called for ANY refresh failure. The id it
recovers is "the last response received for this method", which is the
failing exchange only when a response actually arrived and was refused
while decoding. A transport drop, a timeout, or an abort produces no
response frame at all — the last-answered id then still points at an
EARLIER successful call, so marking it stamped "Rejected by the
Inspector" onto an exchange that worked. That is the same class of lie
this issue exists to remove.
A server-sent JSON-RPC error is excluded for a different reason: the id
would be right, but the failure is the server's and its entry already
renders as an error from the error frame.
Gated on SdkErrorCode.InvalidResult / UnsupportedResultType — decisions
the client made about a frame in hand. The failure is still recorded as
list state either way; only the Protocol attribution is gated.
Caught in Copilot's review of #1953.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SW1p8E2uiyyLx4RKwrwSrt
A list load that failed — a transport error, or a result the SDK's era
codec rejects as invalid — reported nothing. The connect-time refresh is
fired with no caller to await it (`void this.refresh()`), so its rejection
became an unhandled rejection: the header said "Connected", the panel
rendered an empty list indistinguishable from a server that has none, and
the failed exchange showed in the Protocol panel as a clean success.
Two surfaces now tell the truth:
- ManagedListState records the failure as observable state (`getError` +
an `errorChange` event) and still re-throws, so App's auth-recovery
wrapper keeps working. The four `useManaged*` hooks expose it via a
shared `useManagedListError`, and the Tools/Prompts/Resources sidebars
render a `ListLoadError` alert with the reason and a Retry.
- A response the client refused is attributed back to its Protocol entry
(`markResponseRejected` → `responseRejected` → `MessageEntry.clientError`),
which now renders an Error status and a "Rejected by the Inspector"
alert rather than the green resultType badge alone. The SDK gives no
request id with a decode failure, so the id is recovered by correlation;
see the comment on `markResponseRejected` for why that is exact.
Also stops `listAllTools` from dropping a failing excluded-tools walk on
the floor — it stays non-fatal but is logged.
Closes#1953
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SW1p8E2uiyyLx4RKwrwSrt
Firefox does not hand a streaming `fetch()` response to JS until the
first *body* byte arrives; Chromium resolves the promise as soon as the
headers do. Both SSE endpoints flushed headers immediately and then
stayed silent until there was something to report, which deadlocked
`/api/mcp/events`: `RemoteClientTransport.openEventStream()` awaits that
fetch *before* the MCP client sends `initialize`, so no `initialize` ->
no event to report -> no body byte -> the fetch never resolves. The web
UI hung on "Connecting..." forever with nothing in the UI, the console,
or the node logs.
Write an inert `:` SSE comment the instant each stream opens. Conforming
parsers ignore a comment line, so this unblocks the read without
inventing a wire event and needs no client-side protocol change.
`X-Content-Type-Options: nosniff` was also proposed on the issue but does
not fix it -- verified against Firefox 153, the fetch still never
resolves with the header set and no body byte. The delay is not MIME
sniffing.
The priming write is deliberately ordered *after* each handler registers
its consumer (the session event consumer; the file-watch subscriber plus
`ensureWatcher()`). Callers treat the arrival of the stream's first bytes
as proof they are subscribed, so priming first would hand out that proof
across an `await` and drop an edit made in the gap.
`/api/servers/events` gets the same treatment: its fetch was equally
stuck on Firefox until the first real change event, which for an unedited
`mcp.json` never comes. Its client-side reader counted any `\n\n` as a
change, so the inert frame would have fired a spurious background
re-fetch on every connection -- it now skips frames carrying no
`event:`/`data:` field. The transport's own `parseSSE` already drops
comment frames correctly and needed no change.
Verified end to end in real Firefox against a live streamable-HTTP test
server: before, stuck on "Connecting..." past 25s; after, initialize ->
notifications/initialized -> tools/list, connected.
Closes#1858
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01YAt8rqxysNbhYWLhoRm3fU
* chore(deps): migrate to eslint 10 to clear the brace-expansion DoS advisory (#1838)
eslint 9 pulled `minimatch@3` → `brace-expansion@1.1.16`, which carries
GHSA-mh99-v99m-4gvg (high, DoS via unbounded expansion length). It appeared in
all five lockfiles — the root plus each of the four clients, none of which is an
npm workspace, so all five had to move together.
Deferred from the pre-2.0.0 sweep because it is dev-scope, not attacker-
controlled, and a major bump across five packages was the wrong thing to attempt
immediately before an irreversible publish. This is that work.
## What moved
- `eslint` `^9.39.4/5` → `^10.8.0` and `@eslint/js` → `^10.0.1`, in all five
manifests.
- `eslint-plugin-react-hooks` `^7.0.1` → `^7.1.1` in web and tui. Required, not
cosmetic: 7.0.1's peer range stops at `^9.0.0`, so installing it alongside
eslint 10 fails `ERESOLVE`.
- Everything else already declared eslint 10 support and stayed put:
`typescript-eslint@^8.56.1` (`^8.57 || ^9 || ^10`),
`eslint-plugin-react-refresh@^0.5.2` (`^9 || ^10`),
`eslint-plugin-storybook@^10.2.19` (`>=8`).
## Config migration
None was needed. All five configs were already flat (`eslint.config.js` with
`defineConfig`/`globalIgnores` from `eslint/config`), which is the format v10
keeps and the only one it still supports. The root config's `core/` and shared-
surface blocks — including the `_`-prefix `argsIgnorePattern` /
`varsIgnorePattern` / `caughtErrorsIgnorePattern` — are unchanged and still gate
`lint:core` and `lint:shared`.
No rule was disabled, downgraded, or scoped away to make the new eslint pass.
v10 surfaced 21 real violations across three rules; all 21 are fixed in the code.
### `preserve-caught-error` (new in `eslint:recommended`) — 15 sites
A `throw new Error(msg)` inside a `catch` silently drops the original error.
Each now passes `{ cause }`, so the underlying failure survives into the chain.
Messages are unchanged, so nothing that asserts on them moves. 12 in `core/`,
2 in `clients/cli`, 1 in `test-servers/src`.
### `no-useless-assignment` (new in `eslint:recommended`) — 2 sites
`clients/cli/src/cli.ts`'s `optionArgs` and `test-servers/src/test-server-oauth`
's `valid` were initialized and then unconditionally overwritten on every path.
Both are now declared without the dead initializer; TypeScript's definite-
assignment analysis covers the remaining paths.
### `react-hooks/set-state-in-effect` — 8 sites (web)
These were false negatives the plugin fixed in 7.1.0, not a rule we newly opted
into: `set-state-in-effect` has been `error` in `flat/recommended` since 7.0.1,
and the web config has always extended it. Every one was the same anti-pattern —
resetting or re-syncing local state from a prop inside a `useEffect`, which
paints the stale value for one frame and then renders again to correct it.
All eight now use a new `useValueChange(value, onChange)` hook
(`clients/web/src/hooks/useValueChange.ts`), which is React's documented
"adjusting state during render": compare against the previous render's value
with `Object.is` and call back *during* render, so React discards the
in-progress output before it reaches the DOM.
- `MrtrConversation`, `ProtocolEntry`, `TaskCard` — mirror `isListExpanded`.
- `NetworkEntry` — same mirror, plus the "Reveal in Network" force-open. The
reveal's rAF scroll stays an effect (real external-system work); only the
`setIsExpanded(true)` moved. `isExpanded` is now seeded from
`isListExpanded || revealed`, since a render-time sync fires on change and so
cannot cover an entry that mounts already revealed.
- `PromptArgumentsForm`, `ResourceTemplatePanel` — reset on template/prompt
switch.
- `ServerConfigModal` — reset on open. Keyed on `opened ? initial : undefined`,
which collapses "opened flipped" and "initial changed while open" into one
value; `initial` is already memoized, so it cannot thrash.
`useValueChange` has its own test file, and `ServerConfigModal` gains an
open→edit→close→reopen test (the close leg was previously untested, which is
what left the new guard uncovered).
## Advisory delta
`npm audit`, per package, before → after:
| Package | before | after |
| ------------------- | -------------------------- | -------------------------- |
| root | 9 (1 low, 6 mod, 2 high) | 8 (1 low, 6 mod, 1 high) |
| `clients/web` | 19 (3 low, 6 mod, 6 high, 4 crit) | 17 (3 low, 6 mod, 4 high, 4 crit) |
| `clients/cli` | 6 (2 low, 4 high) | 4 (2 low, 2 high) |
| `clients/tui` | 4 (1 low, 3 high) | 2 (1 low, 1 high) |
| `clients/launcher` | 3 (3 high) | 1 (1 high) |
`brace-expansion` is gone from all five — `eslint@10` resolves `minimatch@10` →
`brace-expansion@5.0.9`. Dropping `@eslint/eslintrc` (removed in v10) also took
`js-yaml` with it, clearing GHSA-52cp-r559-cp3m (high) from all four clients as
a bonus. The remainder are unrelated and tracked separately.
`npm run ci` passes end to end — validate, the per-file ≥90 coverage gate, the
build gate, all five smokes (including `smoke:tui`, run locally on a real TTY),
and 462 Storybook tests.
Also documents the effect rule in AGENTS.md and mirrors it into
`.github/copilot-instructions.md`, since it is now something a reviewer cites.
Claude-Session: https://claude.ai/code/session_01YAt8rqxysNbhYWLhoRm3fU
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
* docs: state useValueChange's referential-stability requirement (#1838)
Copilot review on #1900: the hook compares with `Object.is`, so an object or
array literal rebuilt in the component body compares unequal every render.
Because the callback is what updates state, that is not merely a wasted render —
it is an infinite loop.
Every current caller passes a primitive (`boolean`, `string`) or an already-
memoized value, so nothing changes behaviorally. But the hazard was implicit,
and the next caller is the one it would bite.
Documented on the hook itself, and one clause added to the AGENTS.md rule with
its mirror in `.github/copilot-instructions.md`. No runtime guard: the
requirement is the same one a `useEffect` dependency array carries, and here the
failure is loud and immediate rather than silent.
`npm run ci` passes.
Claude-Session: https://claude.ai/code/session_01YAt8rqxysNbhYWLhoRm3fU
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
* fix(cli): stop asserting the --tool-args-json catch binding is an Error (#1838)
Copilot review round 2 on #1900: the `--tool-args-json` parse handler formatted
its message with `(e as Error).message`. A `catch` binding is `unknown`, so that
cast is an assertion rather than a check — a thrown non-Error would render
`--tool-args-json is not valid JSON: undefined`, which points the user at
nothing.
`JSON.parse` does only throw `SyntaxError` today, so this is not a live bug. But
the cast is the kind AGENTS.md asks us not to leave unjustified, the same
handler was already being touched here to add `{ cause: e }`, and every other
site in this PR uses the guarded form. Now it does too.
`npm run ci` passes.
Claude-Session: https://claude.ai/code/session_01YAt8rqxysNbhYWLhoRm3fU
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
* docs: require useValueChange's onChange to be render-pure (#1838)
Copilot review round 3 on #1900: `onChange` is called during render, so it is
bound by render-phase purity — and the web client mounts under `StrictMode`,
which deliberately double-renders in development. A callback doing anything
external would run an unpredictable number of times, and concurrent React can
abandon an in-progress render entirely.
Every current caller already passes a callback that only calls `setState`, so
nothing changes. But "runs during render" is the whole point of the hook and the
constraint that follows from it was left implicit.
Stated on the hook, with the same clause in the AGENTS.md rule and its
`.github/copilot-instructions.md` mirror. All three now point at `NetworkEntry`
as the worked example of the split: the reveal's force-open is a state update
and lives in `useValueChange`, while its `requestAnimationFrame` scroll stays a
`useEffect`.
`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>
* 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
* 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>
* 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>
cliffhall
Sam Morrow