A field that cannot turn its text into a value reports `undefined` — which is
exactly what an empty field reports. That made the two indistinguishable to
every caller of `SchemaForm`, so invalid text in an optional field was
submittable and the argument simply arrived absent (#2020).
`SchemaForm` now tracks per-field draft validity and reports it through
`onValidityChange`, covering both fields that hold text the form cannot send:
the JSON editor (unparseable text) and the number input (a magnitude JS cannot
hold exactly, which is dropped rather than silently rounded). The number field
also states it on itself, as the JSON editor already did. A nested object form
reports through the same channel, and both the field-level and form-level
effects report "valid" on cleanup so a draft cannot outlive the field that held
it — a remount under a new `resetKey`, or an unmounted panel.
`ToolDetailPanel` gated Execute on `isExecuting` alone, so it also had no
required-argument check at all — unlike the App and elicitation panels. It now
uses `hasMissingRequiredFields` like they do. `InlineElicitationRequest` had
neither gate and gets both.
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot round 1: the new EMA regression test constructed its inner provider's
storage with `as unknown as OAuthStorage`, an unjustified double cast — the
rule in AGENTS.md is that such a cast must carry a justification or be replaced.
Replaced with the genuine article: `OAuthStorageBase` over an
`OAuthMemoryStore` and a no-op persist backend, so the stub type-checks against
the real interface the provider is handed and a future `OAuthStorage` contract
change cannot pass silently. Reusing `emaConfig.storage`, as suggested, would
not have addressed the objection — that value is itself a cast, and typed as
`EmaFlowConfig["storage"]` rather than `OAuthStorage`.
The test's assertion is unchanged and was re-verified to fail against pre-fix
code (`expected 'corp-idp' to be null`).
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot review round 1. With an ID Token row alongside the Access Token
row, both can render a "Decode JWT" toggle and a "Copy" control, and the
four buttons shared two accessible names — button-by-button screen-reader
navigation could not tell which token a control acted on. That collision
is introduced by this PR: before it there was only ever one token row.
- `CopyButton` takes an optional `label` naming what is being copied,
used for the accessible name only ("Copy ID Token"). The visible
tooltip stays "Copy", and the name keeps "Copy"/"Copied" as its first
word so it still contains the visible text (WCAG 2.5.3 Label in Name).
- `OAuthTokenField` qualifies its decode toggle the same way ("Decode JWT
for Access Token") and passes its row label down to `CopyButton`.
The visible UI is unchanged — this is accessible-name only.
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot round 10.
The scan no longer publishes its malformed entries into the tools/list report
at all. Round 9 only stopped it overwriting the aggregate's; the remaining
case is unsalvageable for a stronger reason than 'two different requests':
- a FAILED strict aggregate runs salvage, which writes the report itself;
- a SUCCESSFUL one means every entry parsed, so the rendered list contains no
malformed entries.
So by the time the scan finds something, either the aggregate already reported
it with indices matching the screen, or the aggregate succeeded and what the
scan found is provably not in the list being displayed. Publishing could only
ever name entries the user cannot see.
Nothing is lost: listToolsForScan still marks its own refused response, so the
finding surfaces on the Protocol entry for the exchange that carried it, where
it is true. The per-page accumulation and raw-entry offset existed only to feed
the removed publish, and go with it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RDXCSrnSBV7vZfxycG5Z6E
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot round 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>
When an authorization server returns an `id_token` alongside the access
token, the Inspector already persisted it and never showed it. Wire it
through to Connection Info as a viewer.
- Generalize `OAuthAccessTokenField` into `OAuthTokenField` (label + token),
so the ID Token row reuses the existing copy control and "Decode JWT"
toggle rather than duplicating them. Access-token behavior is unchanged.
- `oauthDetailsFromConnectionState` now surfaces `tokens.id_token` as
`idToken`, omitted entirely when absent so no empty row renders.
- Docs: update the two `specification/` references to the renamed field.
This is deliberately a viewer, not a flow: no OIDC login mode, no signature
verification, and the `id_token` is never treated as a credential. The EMA
path is untouched. `id_token` is already in `maskSecrets`'
`JSON_SENSITIVE_KEYS`, so it stays masked in the Network tab — no gap.
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>
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>
A tool argument that is optional AND explicitly nullable — Zod's
`.nullish()` / `.nullable().optional()` — compiles to
`anyOf: [{ type: "string", enum: [...] }, { type: "null" }]`. Every branch
of the tool-call form's field dispatcher tests a single `type` string, so
such a field matched nothing and fell through to the raw-JSON fallback,
whose onChange stores the unparseable in-progress text back into state and
re-`JSON.stringify`s it on the next render — compounding a fresh layer of
escaping on every keystroke.
`normalizeUnionType` already existed for the FastMCP `string|null` shape,
but it only copied the surviving branch's `type` and dropped everything
else, so the `enum` never reached the dispatcher; and nothing called it on
a tool input schema — only elicitation requests, via
`resolveRefsInMessage`. So:
- Rewrite `normalizeUnionType` to hoist the non-null branch's own keywords
(`enum`, `items`, `properties`, `minimum`, …) onto the flattened schema,
as v1.x did. This also collapses nine near-identical blocks into two and
picks up the `object`/`array` cases the `type: [X, "null"]` form was
missing.
- Normalize each field in `SchemaForm.renderField`, so tool, elicitation,
and app forms all get it rather than just elicitation.
- Give a nullable enum `Select` a clear affordance, so a value can be set
back to `null`; a non-nullable enum keeps none, since it would reject it.
Adds a `record_shipment` preset and `nullable-fields-http.json` showcase
config serving all four nullable scalar shapes, for manual verification.
Signed-off-by: cliffhall <cliff@futurescale.com>
- The `retriedDynamic` latch test never made the retry fail: a real `listen(0)`
always succeeds, so it produced one EADDRINUSE and passed with the latch
deleted. Mock `node:http` so every listen fails, asserting exactly two listen
attempts and a settled result. Verified by mutation: removing the latch now
hangs the test instead of passing it.
- The migration guide's Docker notes still described a dynamic sandbox port and
told readers to pin a custom one, contradicting the new fixed 6275 two
sections later.
- Narrowed "or any container" in the troubleshooting entry, which overstated the
scope and contradicted the PR's own scope note. It now names the IPv4-forwarder
case and explicitly excludes the official image and a plain loopback -p.
Signed-off-by: cliffhall <cliff@futurescale.com>
Two defaults that each break the Inspector inside a container.
#1951 — the web UI never loads in a VS Code dev container. `HOST` defaulted
to the NAME `localhost`, and `server.listen(port, host)` resolves a name
through `dns.lookup` and binds the SINGLE address it gets back. Since Node 17
the default result order is `verbatim`, so on a glibc Linux host — whose
/etc/hosts maps localhost to both 127.0.0.1 and ::1 — that is `::1`. The
server bound IPv6 loopback only and refused every IPv4 client, including the
dev container port forwarder. It presents as an endless spinner rather than a
connection error because the forwarder accepts on the host side before failing
to connect inward. Default to the ADDRESS 127.0.0.1 instead; an explicit
HOST=localhost is still honored as typed.
#2008 — the Apps tab is unreachable in any container. The MCP Apps sandbox
listened on an OS-assigned port, so it changed every run and could never be
named in a devcontainer `forwardPorts`, a `docker run -p`, or a tunnel. Default
it to a fixed 6275, mirroring CLIENT_PORT's fixed 6274, and retry once on an
OS-assigned port (loudly) if it is taken — the collision a dynamic default
never had.
Verified in the reporter's exact image
(mcr.microsoft.com/devcontainers/typescript-node:4-24-trixie): both listeners
come up on 127.0.0.1 and answer 200 on 6274 and 6275/sandbox.
Also EXPOSE 6275 and set MCP_SANDBOX_PORT in the image, and document both
defaults in the web README, the migration guide, and the root README's Docker
section.
Signed-off-by: cliffhall <cliff@futurescale.com>
Review was right, and I had assumed the handle type rather than checking it.
Probed both under happy-dom:
setTimeout -> typeof "object", constructor Timeout
requestAnimationFrame -> typeof "object", constructor Immediate
So `typeof id === "number"` in the clearTimeout wrapper never matched, and an
explicitly-cleared timer was never untracked. Both `Set<number>` annotations and
`const id: number` were fictions TypeScript could not catch, since the DOM lib
DECLARES number while happy-dom RETURNS an object.
The effect was benign — the teardown sweep re-clears them and clearTimeout is
idempotent — but the sharper problem is that the test asserting this passed for
the wrong reason. "clearTimeout doesn't throw" holds whether or not tracking
works, so it proved nothing and hid the broken guard.
Hold handles as `unknown`, route cancellation through two small helpers that
carry the one justified cast each, and drop the numeric guards. The test now
asserts the net's own bookkeeping through an exported pendingTimerCount(), and
is mutation-checked: restoring the numeric guard fails it (1 failed | 6 passed).
That is the second test in this PR that passed without proving its claim. Both
had the same tell — the assertion would have held with the feature 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>
Review feedback on #1987: `waitForDeadTransport` polled `/api/mcp/send`,
which answers `transport_error` from two branches — the `isTransportDead()`
short-circuit and the `catch` around a rejected `send()`. Only the first
implies the transport is marked dead, so the stop condition could not tell
them apart from the response alone.
That was safe in practice: `StdioClientTransport.send` rejects only when
`_process` is undefined, and the transport clears `_process` and calls
`onclose` in one synchronous block, so the `catch` branch is not observable
before `markTransportDead()` has run (confirmed empirically — 0/25 probes
returned early). But the safety rested on an SDK-internal ordering this test
neither states nor controls, and if it changed the send test would silently
cover the `catch` branch instead of the short-circuit it names — a regression
with no failing run attached to it.
Poll `/api/mcp/auth-state` instead. For a session connected without an
authState there is no auth provider, so `setAuthState` always throws and
`transport_error` there has exactly one source: the dead-transport
short-circuit. The route never awaits a transport response, so it cannot
hang either. Same cost, exact condition, no dependence on SDK internals.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WLYmVPVLdeYpE6aMTUsCks
Review found the net still had the exact hole it was written to close.
The afterEach is synchronous, so a queued rAF callback cannot run until it
returns — at which point it registers a fresh setTimeout AFTER the sweep has
already drained. On a file's last test that timer survives environment
teardown. That is the rAF race described in the previous commit message, left
open by the fix for it.
Track animation frames too, and cancel them BEFORE sweeping timers. Order is
the fix: JS is single-threaded and nothing between the two loops yields, so no
frame callback can run in between. Mutation-checked — without the cancel pass
the new ordering test fails (1 failed | 6 passed); with it, 7 pass.
Also from review:
- renderWithMantine.tsx still documented the disproven env="test" behavior. It
is the file contributors are told to use, so leaving it would have preserved
the wrong belief exactly where it does most damage. That was the third copy of
this claim in the repo; all three now corrected.
- Dropped an unjustified `as unknown as` in the test helper, which broke the
repo's own double-cast rule. Inference keeps the handle type clearTimeout
accepts.
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>
connectDeadSession spawned a real node subprocess whose only job is to die,
then slept a flat 300ms for the death to propagate. That budget has to cover a
cold spawn(), node's startup, its exit, the parent observing the close, and
onclose marking the session dead — comfortable on an idle box, not under CI
contention or a machine running several agents.
When the budget was missed the session was still live, so isTransportDead() was
false and /api/mcp/send fell through to `await responseWait` for a tools/list
reply the dead child can never send. Nothing bounds that wait, so the test hung
to the project's full 30s ceiling. The timeout IS the symptom of losing the
race, which is why it never presented as a clean assertion failure.
Poll the observable condition instead. Two design points that are load-bearing:
- The probe is a NOTIFICATION (method, no id). requestIdForSendWait returns
undefined for it, so the route never awaits a response and cannot hang.
Probing with a request would reproduce the exact bug being fixed.
- Not /api/mcp/events, which looks like the natural "transport died" signal:
opening that stream on a dead transport calls sessions.delete(sessionId),
so the send under test would then answer 404 instead of transport_error.
Both terminal branches of the route answer transport_error — the
isTransportDead() short-circuit and the catch around a send() that threw on the
dead pipe — and either proves the subsequent real send cannot reach the hanging
path, so both are valid stop conditions.
Verified by reproducing the bug deterministically: with the wait reduced to 0
(the budget always missed) the test times out at 30s; with the poll it passes
10/10. Fixing the helper rather than the test also covers its second call site,
the auth-state test, which sits on the same race and had simply not lost it yet.
connect-crash.test.ts:147 is deliberately left alone. #1985 proposed giving it
the same treatment, but readSseEvents stops at transport_error, which arrives
whenever the child dies — so its 200ms sleep cannot hang, and changing it would
add risk for no gain.
Closes#1985
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>
A window.setTimeout that outlives its test file fires after happy-dom disposes
that file's window; React's dispatchSetState then throws an uncaught
ReferenceError: window is not defined. Vitest attributes it to whichever file
was running, so a single leak fails the ENTIRE run with every test passing —
landing on an innocent file under a step named "Enforce per-file coverage gate."
Seen twice in CI, both on docs-only branches.
Not a Mantine bug, and not a badly-written test. Both hooks do clear on unmount
(useTransition's clearAllTimeouts, useLockScroll's effect cleanup). It is a rAF
race: clearAllTimeouts cancels the *pending* rAF, but the transition schedules
rAF -> rAF -> setTimeout, so if the inner callback is already in flight when the
unmount lands, cancelAnimationFrame is a no-op and that callback then schedules
a timer after cleanup has run. Nothing owns it. Needs a loaded machine, which is
why it only ever appeared in CI.
Track every timer and clear whatever is still outstanding after cleanup(). The
ordering is load-bearing: legitimate unmount cleanups get their turn first, so
only true leaks are dropped. Fake timers pass through untracked, which is
correct — a fake timer cannot outlive the environment.
Also corrects the claim, in AGENTS.md and in setup.ts's own comment, that
env="test" prevents this. It does not: env is read only by Transition.mjs at its
render branch, while useTransition runs before that check (hooks cannot be
conditional) and still schedules real timers. Measured — opening a <Modal>
through renderWithMantine schedules three 200ms timers. Believing that guarantee
is why this went unexamined.
The regression test is mutation-verified: with the net disabled it fails, with
it restored it passes. It deliberately does not assert that Mantine schedules no
timers — it does, and that is normal; the contract is that none survive.
Closes#1984
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>
