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 11. Two of the three are the enum-strip logic this PR added
misbehaving on degenerate input; the third is one more keyword for the
fail-closed rule.
1. An empty `enum` permits nothing at all, but the strip reported "unchanged"
(nothing was removed) and the collapse built a Select with no options — a
required field then had no interaction able to supply a value. It now
reaches the same conclusion as `[null]`: no selectable member, so decline.
The outcome is renamed `unselectable`, since "only-null" no longer describes
both routes to it.
2. Filtering `null` out of `enum` shortens it, which could make an ALREADY
mismatched `enumNames` come out the same length — so `enum: [null, "b"]`
with `enumNames: ["None"]` started labelling `b` as "None", turning labels
both renderers had correctly ignored into a confident mislabel. A list that
was not parallel before filtering is now dropped rather than kept.
3. `$ref` joins the opaque set. It is never resolved here and applies alongside
its siblings, so `{ type: "string", $ref: "#/$defs/intOnly" }` can be
unsatisfiable. The wrapper path already declined on it by virtue of the
annotation allowlist; this extends the same rule to branches, the
`type: [T, "null"]` path, and `admitsNull`.
No round 12 was requested: the review loop was stopped at the user's direction.
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 10: one finding, the last position of the rule the previous two
rounds established. A *non-null* branch can carry its own `anyOf`, and the
hoist drops it — the branch's `anyOf` overwrites the wrapper's in the spread
and is then cleared, so `{ anyOf: [{ type: "string", anyOf: [{ const: "a" }] },
{ type: "null" }] }` widened into an unconstrained nullable string.
Rather than add a fourth inline condition, the four sites that ask "does this
schema compose something I cannot evaluate?" now share one predicate,
`hasUnevaluatedComposition` (an opaque applicator, or a nested `anyOf`):
- the surviving branch in the `anyOf` collapse (this round's finding),
- the null branch in `admitsNull`,
- the `type: [T, "null"]` collapse path,
- and, via `hasOpaqueApplicator`, the top of `admitsNull`.
Each of those was missed independently across rounds 8-10 for the same reason:
the guard existed but was spelled out inline at each site and drifted. Sharing
the predicate is the part that stops a fifth site from repeating it.
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 9: four findings, all of them places where round 8's fail-closed
inversion was the right idea applied incompletely. The rule was only enforced
at the positions I happened to be editing; it now holds everywhere a schema is
inspected.
1. `admitsNull` returned early on an explicit `type`, ignoring a sibling
`anyOf` — but the two are conjunctive, so `{ type: ["string", "null"],
anyOf: [{ type: "string" }] }` admits no null at all. A sibling union now
withholds the claim.
2. A null-typed branch could be unsatisfiable via its own nested `anyOf`
(`{ type: "null", anyOf: [{ type: "string" }] }`). The branch check tested
`not`/`allOf`/`oneOf` but not `anyOf`; it now treats a nested union as
equally opaque.
3. The `type: [T, "null"]` collapse path had no applicator guard at all —
`collapsed` clears `anyOf` unconditionally and the renderers ignore the
rest, so `{ type: ["string", "null"], anyOf: [{ const: "a" }] }` was widened
into an unconstrained string field. That path now declines the same way the
`anyOf` path does.
4. The `admitsNull` docstring still advertised `oneOf` support that round 8
deliberately removed. Rewritten to state what is recognized, what is
declined and why, and the asymmetry that settles the direction.
The three code fixes share one helper (`hasOpaqueApplicator`) rather than
repeating the condition, so the next position that needs it cannot drift.
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 8 opened the same conjunctive-semantics seam a third time, so
this stops patching cases and inverts the defaults instead. Both guards now
fail CLOSED: anything this module cannot evaluate sends the field to the JSON
editor rather than to a widget built from a partial reading.
1. The wrapper-conflict check enumerated the *validation* keywords and declined
when two of them disagreed — which meant any keyword nobody listed
(`oneOf`, `allOf`, `not`, `$ref`, a future vocabulary) was silently treated
as safe to drop. That whitelist fails open, and open means rendering a
widget for a constraint that is not the schema's. It is replaced by an
allowlist of *annotation* keywords: the collapse declines whenever the
wrapper carries anything beyond annotations and the union itself. Stricter
than the old equality test — a wrapper restating its branch's `type` now
declines too — which costs a dropdown on a rare redundant shape and buys a
rule with no gap in it.
2. `admitsNull` returned true for shapes it had not actually evaluated:
`not: { type: "null" }` rules null out, `allOf` can add a member that does,
`oneOf` requires *exactly one* branch to match (so a null branch does not
by itself mean null validates), and a branch like `{ type: "null",
const: "x" }` is unsatisfiable. All now answer false. Under-claiming
nullability costs a clear button; over-claiming lets the form emit a value
the schema rejects, so the asymmetry decides it.
Note the third finding — draft validity never reaching the submit controls —
is NOT fixed here, and it is worse than I described last round: `ToolDetailPanel`
gates Execute on `isExecuting` alone and never calls `hasMissingRequiredFields`,
so my earlier claim that required fields were already covered was wrong for the
Tools tab. Corrected on #2020, which is where the fix belongs.
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>
Copilot round 7: both findings are the same principle applied one level
further — JSON Schema siblings are conjunctive, and this code was treating
them as alternatives.
1. The hoist is a spread, so a branch keyword *replaced* the wrapper's. That
is not a conjunction: a wrapper `enum: ["a"]` around a branch
`enum: ["a", "b"]` means `"a"`, but the spread produced `["a", "b"]` and
the dropdown would have offered — and submitted — a `"b"` the schema
rejects. `type`, bounds, and the object keywords fail the same way.
The collapse now declines when wrapper and branch carry different values
for the same validation keyword, and the field renders through the JSON
editor with its schema intact. Intersecting them properly is a much larger
surface, and a subtly wrong intersection would be worse than none.
Identical values are not a conflict, so the ordinary redundant-`type`
shape still collapses; metadata (`title`, `description`, `default`,
`enumNames`) never conflicts, since nothing validates against it.
2. `admitsNull` let a `{ type: "null" }` branch outvote an explicit
non-null top-level `type`. `{ type: "string", anyOf: [..., { type:
"null" }] }` rejects null — the value must satisfy the type *and* the
union — so an explicit `type` now decides instead of falling through to
the branch scan.
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot round 6: no visible comments, three suppressed. All valid.
1. JSON Schema keywords at one level are conjunctive, so a syntactic null does
not by itself mean the schema accepts null. `{ type: ["string", "null"],
enum: ["envio"] }` names "null" and still rejects it — yet the collapse
marked it `nullable: true`, giving the field a clear button that emits a
value the schema forbids, and `admitsNull` let required-field gating accept
it. Both now consult `nullExcludedBySiblings`.
The distinction that makes this safe: a *branch's* `enum` is not a sibling.
In `anyOf: [{ type: "string", enum: [...] }, { type: "null" }]` — the #1928
shape — the enum constrains that branch alone and the union still permits
null, so `nullable` is read off the ORIGINAL schema rather than the hoisted
merge. Reading the merge would have called the headline case non-nullable.
Flattening is unaffected either way; only the nullability claim is withheld.
2. The collapse newly routed a nullable array of numeric item enums into the
string-only `MultiSelect` path, which would submit `["1"]` for `[1]`. Gated
on `isStringEnum`, matching `toConstOptions`; `isStringEnum` is now a type
predicate so the narrowing is real rather than asserted.
3. Invalid JSON and an empty field both report `undefined`, so an optional
complex argument could be dropped from the submission while invalid text sat
visible in the box. The field now says so via `error`. Note a *required*
field was already gated — its value is `undefined`, which
`hasMissingRequiredFields` treats as missing — so this is specifically the
optional case. Disabling Execute/Open/Submit on an invalid optional draft
needs a validity channel out of `SchemaForm` into all four call sites, which
is wider than this fix; filed separately.
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot round 5: one inline comment, two suppressed. All three valid, and two
were defects from round 4's own fix.
1. Stripping the `null` sentinel from an `enum` left the parallel `enumNames`
untouched. Both renderers discard labels outright when the two lengths
disagree, so `{ enum: ["a", null], enumNames: ["Alpha", "None"] }` lost the
`Alpha` label entirely — a worse outcome than the sentinel it was fixing.
`enumNames` is now filtered by the same indices, and only when it was
positionally parallel to begin with (a mismatched list is already ignored
by both renderers, so re-indexing it would invent an alignment the server
never declared).
2. `enum: [null]` became `enum: undefined`, which widened a schema permitting
*only* `null` into a plain string field accepting arbitrary text — the form
would then invite values the schema forbids. Trading a cosmetic problem for
a correctness one, which was the wrong call. The collapse now declines
entirely for that case and the field renders through the JSON editor.
3. That JSON editor was still carrying #1928's original mechanism: it stored
unparseable text back as the *value*, which the next render
re-`JSON.stringify`d, so typing `[` gave `"["` then `"\"[\""`. Routing
nullable fields to real widgets removed the common way of landing there but
never fixed the editor — and #2007's fix deliberately sends object unions to
it, so a fallback nobody can type into is not a fallback. `SchemaJsonField`
now holds the draft text locally (the split `SchemaNumberInput` already
uses) and reports parsed JSON, or `undefined` while the text is mid-edit —
an inspector must not report a value it cannot send. Keyed by `resetKey`
like the number field, since it now holds state.
Also replaces an `as unknown as string[]` in the round-4 test, which violated
the repo's TypeScript rules and papered over `InspectorFormSchema.enum` being
unable to model a valid `["a", null]`. The fixture is wire data, so it now goes
through `toFormSchema` — the same wire→form narrowing boundary every production
call site uses.
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>
Copilot round 4: no new visible comments, two in the suppressed block. Both
valid.
1. The `type: [T, "null"]` encoding keeps its keywords at the top level, so a
nullable enum written that way carries the null *inside* the list:
`{ type: ["string", "null"], enum: ["envio", "recebimento", null] }`. The
collapse left it there, which broke both renderers in different ways — the
web dispatcher handed `null` to Mantine as option data, and the TUI's
all-strings check rejected the whole enum and fell back to a plain text
field. Neither is the dropdown the schema asks for.
The collapse now strips a `null` member whenever it flattens, since that
fact has already moved onto `nullable`. An enum of nothing but `null`
drops out entirely: it offers no value a dropdown could show, so the plain
widget serves the field better than an empty select. Covered in core and
end-to-end in both dispatchers.
2. The new integration test claimed in its own header to pin the registry and
config wiring, then called `createNullableFieldsTool()` directly — so a
misspelt preset case or a config naming a dead preset would have left it
green. A fair "this does not test what it says" catch. It now resolves the
checked-in `nullable-fields-http.json` through `loadConfig` →
`resolveConfig`, matching `duplicate-tool-names.test.ts`, and asserts the
resolved tool names before booting the server.
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>
Three findings (one inline, two in the suppressed block), all valid.
1. `toConstOptions` still stringified every non-null `const`, so
`items: { anyOf: [{ const: 1 }, { const: 2 }] }` submitted `["1"]` where
the schema says `[1]`. Same wrong-type-on-the-wire problem that keeps a
numeric `enum` off the select path, so it gets the same answer: only a
string `const` is selectable, anything else falls to the JSON editor
where the value keeps its type.
2. `hasMissingRequiredFields` derived null admission from the *renderer's*
collapse, which is narrower on purpose — it only flattens a two-member
union. So `anyOf: [string, number, null]` renders through the JSON
fallback (where `null` is typeable) yet was still marked missing,
disabling submit on a value the schema accepts. Null admission is a
validity question, not a rendering one, so it is now its own predicate:
`admitsNull` in core, recognizing `nullable`, `type: "null"`,
`type: [..., "null"]`, and a null branch anywhere in an `anyOf`/`oneOf`
of any size.
3. Nothing exercised the `record_shipment` preset — the web and TUI tests
build the `anyOf` shape by hand, which verifies the renderers but
*assumes* the premise the whole fix rests on: that Zod's `.nullish()`
emits `anyOf: [<branch>, { type: "null" }]` with the enum on the branch.
A Zod output change or a registry typo would leave every unit test green
while the documented reproduction server quietly stopped reproducing.
Added an integration test that boots the preset over HTTP, asserts the
wire shape, and runs it back through the collapse — following the
`duplicate-tool-names.test.ts` precedent, which exists for the same
reason (#1957).
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>
Closes#2007.
Two things, both surfaced while answering "does #1928's PR also fix #2007?".
**#2007, and why it belongs in this PR.** `z.array(z.union([z.object(…),
z.object(…)]))` gives an `items.anyOf` whose branches carry no top-level
`const`, so every MultiSelect option was `String(item.const ?? "")` — the
empty string, twice. Mantine *throws* on duplicate option values, greying
out the whole tool panel.
That is a pre-existing bug, but this branch makes it reachable from a new
direction: a *nullable* array of object unions previously had no top-level
`type` and fell to the JSON fallback, and now collapses to `type: "array"`
with `items.anyOf` — straight into the crash. Shipping the collapse without
this fix would widen an existing crash, so the two go together. Both shapes
are covered by tests.
`toConstOptions` now returns `null` unless every branch yields a distinct,
non-empty value, and the field falls through to the JSON editor — which is
the honest widget for a union of object shapes anyway. Applied to the
`string`/`oneOf` path as well, which had the identical `?? ""` and the same
duplicate-throw waiting on it.
**Copilot round 2.** Round 1's fix kept "an explicit `type` is
authoritative", so `{ type: "number", enum: [1, 2] }` still collapses with
its enum hoisted. The TUI's dispatcher tests `property.enum` *before* the
type switch, and `toSelectOptions` stringifies, so that reached a select
that submits `"1"` for `1`. The select path is now gated on `isStringEnum`
(exported from core for this), and a typed non-string enum falls through to
its typed field: the enum constraint is lost, the value's type is not,
which is the safer of the two losses. Pre-existing for a plain numeric
enum; the collapse just made the nullable form reachable too.
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>
Four findings, all valid:
1. Returning `T` was unsound. The collapse genuinely changes two fields —
`type` stops being an array, `anyOf` becomes `undefined` — so a caller
whose `T` typed either that way could keep treating it so afterwards.
The result is now `T | NormalizedNullableUnion<T>`, which states exactly
that: `Omit<T, "type" | "anyOf">` plus a `RenderableType` `type`, a
cleared `anyOf`, and `nullable`. The one assertion left is isolated in a
`collapsed()` helper, needed only because `Omit` stays deferred while `T`
is open.
2. A typeless `enum` does not imply strings. JSON Schema allows
`enum: [1, 2]`, and inferring `"string"` for it would hand numbers to a
`Select` declared `string[]`, and make the TUI submit `"1"` for `1`. Now
only an all-string enum earns the inference; anything else stays on the
JSON fallback. An explicit `type` is still authoritative.
3. `hasMissingRequiredFields` counted every null as missing, so clearing a
*required* nullable enum — newly possible via the clear button — would
disable submit on a value the schema permits. `required` constrains
presence, not content, so null now counts as present exactly when the
field's schema admits it.
4. `collectSchemaDefaults` read the raw union, so a default nested inside a
nullable object's branch was displayed by the form but never seeded into
the submitted values. It normalizes each property first, like the
renderer does.
Signed-off-by: cliffhall <cliff@futurescale.com>
Closes#2015, folded into #1928 rather than left as a follow-up.
The TUI's `schemaToForm` has the same defect the web `SchemaForm` had: its
local `JsonSchemaProperty` declares only `type?: string` — no `anyOf`, no
array-valued `type` — so a `.nullish()` argument has no top-level `type` or
`enum` for the dispatcher to read and falls to the `switch` default, losing
its select. Milder than the web bug (a plain text field, not a corrupting
JSON textarea), but the same class.
Fixing it in a second place is the argument for not having the logic in a
first place. So the collapse moves to `core/json/nullableUnion.ts` as
`normalizeNullableUnion`, generic over a minimal `NullableUnionSchema`
shape — the least a caller must expose to be *recognized* as a nullable
union — so neither client has to adopt the other's schema type. Both form
builders now share one copy and cannot drift on which schemas they render.
- `core/json/nullableUnion.ts` — the shared collapse, plus tests at
`clients/web/src/test/core/nullableUnion.test.ts` (the web suite is what
gates `core/`).
- `clients/web`: `schemaUtils.normalizeUnionType` is gone; `SchemaForm` and
`resolveRefsInMessage` call the core function directly.
- `clients/tui`: `schemaToForm` normalizes each property before dispatch,
and `JsonSchemaProperty` grows the `anyOf` / array-`type` fields the
encoding uses.
Verified by driving the built TUI under a pty against the
`nullable-fields-http.json` showcase server. Before, every argument of
`record_shipment` rendered as a plain text field; after, `direction` is a
select ("No value"), `quantity` an integer, `express` a boolean
("[Not set]").
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
* fix: load @napi-rs/keyring lazily so an unsupported platform degrades
`@napi-rs/keyring` ships one prebuilt binary per platform triple and
throws on import where it has none. On Android / Termux there is no
`@napi-rs/keyring-android-arm64`, so the static top-level import in
`core/auth/node/secret-store.ts` threw during module evaluation and the
Inspector exited at startup with "Cannot find native binding" — before
any of the keychain-unavailable handling in that same file could run.
Replaces the static import with a cached dynamic one. The outcome is
cached, not just the module, so a box without a binary doesn't
re-attempt (and re-throw) resolution on every secret operation —
`expectedSecretFields` means that would otherwise be once per server per
`GET /api/servers` — and so `set` can name the underlying cause in its
`KeychainUnavailableError`.
An unloadable package now folds into the same degradation contract as an
unreachable keychain: `get` returns null, `delete` and
`deleteAllForServer` no-op, and `set` throws `KeychainUnavailableError`
(not double-wrapped — the load failure is already typed by the time the
catch sees it). The class doc comment now enumerates all three ways
"unavailable" can arrive, since each escaped the contract at some point.
The shared `vi.mock` stub can't express this — it models a keyring that
loads — so the new tests build on `vi.resetModules()` + `vi.doMock` with
a throwing factory and re-import the module, covering the load failure,
each method under it, and the load-once caching.
Verified the dynamic import survives bundling in all three clients
(cli, tui, and the web runner) rather than being inlined back to a
static one.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XAR2Nr9kXbrywFNUVoTe9F
* fix: steer a missing native binding to a reinstall hint
`KeychainUnavailableError` gave the Linux libsecret / gnome-keyring
advice for every cause, including the one where the keychain is fine and
the @napi-rs/keyring platform package is what's missing — an unsupported
triple (#1905) or npm's optional-deps bug dropping it on a supported one
(npm/cli#4828, the Windows report on #1852). Detect the loader's "Cannot
find native binding" phrasing and point those users at a reinstall /
npx-cache clear instead.
Ported from the superseded #1943, which fixed this alongside the same
lazy-load change.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HDdo1rNRVQnVRRdsqSrvG3
* fix: name a bad keyring namespace instead of "is not a constructor"
The lazy import accepted whatever the resolution produced. The named
exports arrive through CJS interop, so a resolution that stopped
yielding them — a default-only export upstream, a bundler changing
interop, a platform where named-export detection fails — would hand us
`undefined`, and `new mod.AsyncEntry(...)` would throw a TypeError
inside the very try that implements graceful degradation.
Shape-check the resolved module and treat a bad shape as unavailable.
Scope, because it is narrower than it first appears: this does NOT
prevent a bad shape from emptying the secret list. `get` returns null
either way — that is its read-tolerance contract, and the TypeError
lands in the same catch a dead keychain does. What changes is the
diagnosis. Without the check the only signal is `set` reporting
"keyring.mod.AsyncEntry is not a constructor", which reads like an
Inspector bug; with it, `set` names the real problem once, at the load
boundary, in the same actionable 503 as every other unavailability.
Catching the silent-empty-list case itself would need a round-trip
against the unmocked package, which nothing in the suite does today.
The member access sits inside the try because reading a missing export
is not always a harmless undefined — a Proxy-backed namespace can throw,
as vitest's module mocks do — and letting that escape would reject the
cached promise, which is otherwise guaranteed to always resolve.
Of the four new tests only the cause-message one fails without the
guard; the rest pin the surrounding contract.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HDdo1rNRVQnVRRdsqSrvG3
* docs: correct the test header's count of unavailability entry points
The shape check added a fourth way "unavailable" can arrive, and the
enumeration in secret-store.ts was updated to match, but the test file's
header still said three — so it read as an exhaustive list that silently
omitted the case a whole describe below it covers.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HDdo1rNRVQnVRRdsqSrvG3
* fix: give the wrong-shape cause its own remediation hint
The shape check added a cause that fell through to the libsecret
advice — so a packaging mismatch told the user to install a keyring
daemon, which cannot help, and on Windows or macOS points somewhere that
does not exist. That is the same wrong-advice bug the native-binding
branch was added to fix, reintroduced by the guard itself.
Hint selection moves into `hintFor`, where libsecret is the explicit
fallback rather than the default, and the shape failure becomes a
distinct `KeyringModuleShapeError` matched by type — it is our own error,
so there is no reason to re-parse text we wrote (the native-binding
branch matches a string only because that message comes from napi-rs).
Also retitles the wrong-shape test: it claimed the guard avoids
"returning null secrets", but `get` returns null either way by the
read-tolerance contract. The title now says what actually differs —
`set` hard-fails — matching the correction already made in the comments.
Both raised by Copilot review.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HDdo1rNRVQnVRRdsqSrvG3
* fix: give both shape-check failure paths the packaging hint
The shape check can fail two ways — the members are absent, or reading
them throws — and only the first carried KeyringModuleShapeError. The
catch path returned the raw error, so a Proxy-backed namespace throwing
on access fell through to the libsecret advice: the same wrong-remedy
bug as the previous two commits, one branch further along.
Both paths now carry the type. KeyringModuleShapeError takes a detail
string plus a standard `cause`, so the throwing path reads "its exports
could not be read: <original>" and keeps the underlying failure legible
while still earning the packaging hint.
Three instances of one mistake in a row (a new cause added without
deciding which remediation it inherits) says the shape was wrong, not
just the branch: making libsecret an explicit fallback in `hintFor` did
not stop me adding a second exit that skipped it. The tests now pin the
invariant directly — one asserts both construction paths produce the
same hint — so the next cause that forgets is a failing test rather than
a reviewer's catch.
Raised by Copilot review.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HDdo1rNRVQnVRRdsqSrvG3
---------
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
* fix: construct AsyncEntry inside the try so keychain degradation engages
`KeyringSecretStore` built its `AsyncEntry` outside the `try` in `get`,
`set`, and `delete`. `AsyncEntry::new` performs the platform-store setup
(on Linux, the Secret Service connect with a keyutils fallback) and
throws when no backend is reachable, so the documented degradation
contract never engaged for a construction-time failure: the raw keyring
error escaped, 500ing every `GET /api/servers` on a box without a Secret
Service (the published container, which has no D-Bus session).
`expectedSecretFields` always includes the OAuth slot, so `rehydrateConfig`
constructs an entry for every server — the default seeded catalog was
enough to 500 the first list load, before any server was added. And
because the escaping error wasn't a `KeychainUnavailableError`, it
bypassed both the routes' 503 translation and the `migratePlaintextSecrets`
skip branch, yielding a generic 500 instead of the actionable message.
Moving construction inside the existing `try` restores the contract for
that failure mode: `get` returns null, `delete` no-ops, and `set` is the
only operation that throws — as `KeychainUnavailableError`, which is what
the 503 translation and the migration skip both match on.
The test stub's constructor could not fail, which is why the coverage
gate never saw the gap. Adds a `constructorThrows` hook alongside the
existing method-level `failures` flags and covers all three methods plus
`deleteAllForServer` under it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XAR2Nr9kXbrywFNUVoTe9F
* test: pin why the OAuth secret field is unconditional
`expectedSecretFields` always includes the OAuth slot, and #1848's report
frames that as why the 500 fired so broadly — which invites a follow-up
that skips the keychain read for servers with "no OAuth config". That
change would lose data.
`extractSecretsFromStored` deletes the `oauth` block outright when
`clientSecret` was its only property, so such a server carries NO marker
on disk that a secret exists — the keychain is the sole record, and the
unconditional slot is what finds it again. Gating the read on a
disk-visible `oauth` block would silently stop rehydrating exactly that
shape.
The existing tests do fail if the slot is made conditional (verified by
applying the change: 4 red), but only under names that explain nothing
about the consequence — `always lists the OAuth slot first` reads like a
tautology worth updating rather than a trap. Add a round-trip case that
states the consequence, so the next reader sees the field is load-bearing
rather than defensive.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01YAt8rqxysNbhYWLhoRm3fU
---------
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
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(auth): offer explicit re-authorize when callback state is lost (#1808)
A `/oauth/callback` that arrives with no recorded SEP-2352 discovery state
made the SDK throw `AuthorizationServerMismatchError` and the Inspector
dead-ended, surfacing the raw SDK text with no path forward.
The SDK folds two very different failures into that one error class, so
`core/auth/issuerBinding.ts` classifies them structurally: a recoverable
"lost authorization state" (the SDK puts prose, not an issuer URL, in the
`recordedIssuer` slot) versus a genuine cross-AS `issuer_mismatch`. Only
the first gets a recovery affordance.
Web: the callback failure now raises a dedicated banner ("Authorization
state was lost" / "Authorize again") whose action clears the stale OAuth
state for that server and starts a fresh authorization. A genuine
mismatch stays a red, non-dismissible security notification with both
issuers named and no one-click retry.
Node runners (CLI/TUI): the same classification rewrites the callback
rejection into the shared actionable copy instead of the SDK text.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016S7h83UKNQ7kvSiFDNksCt
* fix(auth): detect the SDK mismatch error via isInstance (#1808)
The callback-leg classifier gated on `err.mcpBrand`, but the SDK declares
that brand in a `static {}` block, so it lives on the constructor and is
never reachable from an instance. The predicate always returned false, so
`findIssuerBindingFailure()` never matched and the #1808 recovery banner
was inert in production — the callback dead-ended exactly as before.
Use the SDK's own `AuthorizationServerMismatchError.isInstance()`, which is
cross-copy safe by construction (it consults a `Symbol.for()`-keyed brand
set), with a `name` fallback for a serialization boundary. Correct the
module doc, which cited cross-bundle safety as the reason to avoid
`instanceof` — the very thing the SDK's branded `hasInstance` provides.
Every fixture now constructs a real `AuthorizationServerMismatchError`
instead of a hand-rolled object with an own `mcpBrand`, which is why a
green `npm run ci` hid this. Adds a test pinning the brand's placement so
the feature cannot silently go inert again.
Review follow-ups: drop the unused `isLostAuthorizationStateError`, remove
the duplicated banner title default, resolve the banner title/action label
where the banner is raised, keep the clear-failure notification open, and
bound remote-supplied issuers in user-facing copy.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016S7h83UKNQ7kvSiFDNksCt
---------
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
* 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
Cliff Hall
Sam Morrow