Copilot round 5 — three of four findings implemented.
- `cleanAuthorizationParams` validates the field as it comes off disk. Only the
web client's /api/servers route checked the shape; the CLI and TUI read
mcp.json directly, so `authorizationParams: "oops"` enumerated as the
character-indexed pairs 0=o, 1=o, … and `{ audience: 5 }` reached
URLSearchParams.set, which stringifies it — either way the Inspector sent
query parameters the user never wrote. Now validated in shared code so all
three clients enforce the documented string→string contract, warning on each
rejection. Follows `cleanRoots`, which guards the same class for `roots`.
- The form's description now states that these parameters are not sent while
Enterprise-managed authorization is on. The EMA leg authorizes against the
enterprise IdP — a different authorization server — and deliberately sends
none of them, so the copy was presenting a configured-but-inert value as
active.
- The /api/servers malformed-`oauth` warning listed an expected shape omitting
`onInsufficientScope`, which the validator accepts and the docs describe, so
it told users a valid config was malformed.
Ten new cases, all verified to fail against the pre-fix sources.
Declined: recording the authorize URL in the Network tab (repeat of round 2) —
that URL is reached by a top-level browser navigation rather than a tracked
fetch, so it has never appeared there for any authorization request; adding a
synthetic entry changes every OAuth flow and is a product decision for a
maintainer, not a change to make inside this PR.
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot round 4 (one suppressed finding, and a real bug).
`extractSecretsFromStored` rebuilt the stripped `oauth` object from an
allow-list of the non-secret fields. That list had gone stale: it never named
`onInsufficientScope`, so a server carrying both a client secret and a
non-default SEP-2350 policy silently lost the policy on save, falling back to
`reauthorize`. This predates #2018 — the PR only extended the same list with
`authorizationParams` — but the list is the defect, so fix the class rather
than add a third entry to it.
The stripped entry now subtracts the one secret instead of enumerating what to
keep, so any field added to `oauth` later is preserved by construction.
Regression test verified to fail against the allow-list version:
`expected { clientId: 'cid' } to deeply equal { clientId: 'cid', onInsufficientScope: 'throw' }`.
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot round 3 (both findings suppressed; both valid).
- The a11y fix in the previous commit added `align="flex-start"` to the row's
`Group`, giving it two static layout props and so tripping the .withProps()
rule in AGENTS.md — which applies to a single-use element too. Extracted as
`AuthorizationParamRow`. (The sibling `KeyValueRows` Group carries one static
prop and stays inline, correctly.)
- Added OAuthManager coverage for the config→provider bridge. The point is
sharp: the provider test proves the merge and the runner test proves the
options object, but nothing crossed the seam in `createOAuthProvider`, so
deleting the `authorizationParams:` line left the feature dead with every
test green. Verified by doing exactly that — both new cases now fail with
`expected null to be 'corp-idp'` — then restoring it. The second case also
pins the reserved-key drop through the manager path.
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot round 2 (suppressed comments). Five of the seven findings were the
accessibility of the new row list and one stray cast; all are addressed here.
- The reserved-key reason is now the key input's `error` **string** rather than
a bare boolean plus free-standing text. Mantine renders it in the input's own
error slot and wires the `aria-describedby` association, so a screen reader
learns *why* the field is invalid; a boolean only set `aria-invalid`. This
also deletes the duplicate `ReservedParamError` text and its Stack wrapper.
- Each control carries a per-row `aria-label` — the section heading and the
placeholders are not programmatically associated with the inputs, so the key
box, the value box, and a remove button announcing only "X" were
indistinguishable to assistive technology. The label names the row by its key,
falling back to "row N" while the key is still blank.
- Dropped an unnecessary `as unknown as Record<string, unknown>` in the
servers-route integration test: `MCPConfig.mcpServers` is already typed as
`Record<string, StoredMCPServer>`, so the entry's `oauth` reads directly.
Three tests cover the new behavior (accessible names, the blank-key row-number
fallback, and the aria-describedby association), each verified to fail against
the pre-fix component. No change to the reserved-key rule or the EMA boundary.
Signed-off-by: cliffhall <cliff@futurescale.com>
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>
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>
