Copilot review: the comments offered "a nullable number the user cleared" as a
way to get an explicit `null` into this field. Clearing the box reports
`undefined` — `toNumericValue("")` says so, and an existing test pins it — so
the only way `null` arrives is from parent state: a value received from the
server, restored from a deep link, or written by a caller for a nullable
schema. Say that in all three places instead.
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot review: a nullable number schema really does produce `null` for both
the raw value and the schema default, but the props declared only
`number | undefined` and the call site cast the null away. The runtime already
handled it — the initializer's `?? ""` exists for exactly that — so the types
were the part that was wrong, and a cast hiding it is how a future null bug
goes unseen by the compiler.
Widen both to `number | null | undefined` and drop the narrowing casts, keeping
the distinction the initializer encodes: `undefined` takes the default, `null`
is a value and renders as an empty box.
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot review: seeding with `value ?? defaultValue` also substitutes the
default when the raw value is an explicit `null` — a nullable number field the
user cleared, or one the server sent — where `resolveValue` previously kept the
null and the box rendered empty. A nullable schema with a non-null default
would have opened showing that default over a real value.
Test the `undefined` case explicitly, as `SchemaJsonField` already does, and
pin it: the new test fails with `'30'` where `''` belongs.
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot review: the code and test comments still said a defaulted array or
object argument *was* uneditable. It would be, but not through today's seeding
path — `collectSchemaDefaults` assigns `fieldSchema.default` itself, so the
substitution returns the same reference and the re-sync never fires. It needs a
structurally-equal but distinct value object, which parsed wire/deep-link
values and rebuilt nested-object defaults produce, and which these tests
construct. The number field has no such protection and did revert in the app.
State the boundary in all three places, and stop claiming the distinct-object
seed is how the Tools tab seeds the field, which it is not.
Signed-off-by: cliffhall <cliff@futurescale.com>
A field whose schema declares a `default` could not be edited. Unsendable
draft text reports `undefined` by design — an inspector must not report a value
it cannot send — and `resolveValue` turned that straight back into the default.
That is a real change from the previous value, so the draft/value re-sync
rewrote the box to the default and the keystroke was lost. Almost every edit
passes through an unsendable state, which left a defaulted array or object
argument uneditable in the JSON editor, and a defaulted number field unable to
be emptied.
Hand the two draft-holding fields the raw value and the schema default
separately, and compare only against the raw one. The default now seeds what
the field opens with, which is what a `default` means, rather than being
re-imposed while the user types. Every other field keeps resolving through
`resolveValue` unchanged.
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot review: seeding `{ b: null }` / `{ cfg: null }` meant the schema's
`default: null` was never resolved, so a regression that dropped null defaults
would have left both #1885 tests green while claiming to pin that path. Render
the harness unseeded instead, which is also how the reporter's form arrived.
Signed-off-by: cliffhall <cliff@futurescale.com>
Three issues report one defect: the schema form's JSON fallback re-escaping
the text being typed. #1853 saw it while typing an array, #1856 on the
Backspace that first makes a valid array invalid, and #1885 on a nullable
parameter whose default `null` had to be edited in place.
The mechanism — unparseable draft text stored back as the field's value, which
the next controlled render re-JSON.stringify'd — was removed by the draft/value
split in SchemaJsonField and the nullable-union collapse (#1928, #2007, #2020).
Those fixes were made for differently-framed issues, so nothing pinned the
flows these three reports actually describe.
Verified against the pre-fix commit: four of the five new tests fail there with
the reported corruption, including '"\"\"a"' for the caret-outside-the-quotes
case and the compounding backslash cascade for the typed array.
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot round 5 — three of four findings implemented.
- `cleanAuthorizationParams` validates the field as it comes off disk. Only the
web client's /api/servers route checked the shape; the CLI and TUI read
mcp.json directly, so `authorizationParams: "oops"` enumerated as the
character-indexed pairs 0=o, 1=o, … and `{ audience: 5 }` reached
URLSearchParams.set, which stringifies it — either way the Inspector sent
query parameters the user never wrote. Now validated in shared code so all
three clients enforce the documented string→string contract, warning on each
rejection. Follows `cleanRoots`, which guards the same class for `roots`.
- The form's description now states that these parameters are not sent while
Enterprise-managed authorization is on. The EMA leg authorizes against the
enterprise IdP — a different authorization server — and deliberately sends
none of them, so the copy was presenting a configured-but-inert value as
active.
- The /api/servers malformed-`oauth` warning listed an expected shape omitting
`onInsufficientScope`, which the validator accepts and the docs describe, so
it told users a valid config was malformed.
Ten new cases, all verified to fail against the pre-fix sources.
Declined: recording the authorize URL in the Network tab (repeat of round 2) —
that URL is reached by a top-level browser navigation rather than a tracked
fetch, so it has never appeared there for any authorization request; adding a
synthetic entry changes every OAuth flow and is a product decision for a
maintainer, not a change to make inside this PR.
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot round 4 (one suppressed finding, and a real bug).
`extractSecretsFromStored` rebuilt the stripped `oauth` object from an
allow-list of the non-secret fields. That list had gone stale: it never named
`onInsufficientScope`, so a server carrying both a client secret and a
non-default SEP-2350 policy silently lost the policy on save, falling back to
`reauthorize`. This predates #2018 — the PR only extended the same list with
`authorizationParams` — but the list is the defect, so fix the class rather
than add a third entry to it.
The stripped entry now subtracts the one secret instead of enumerating what to
keep, so any field added to `oauth` later is preserved by construction.
Regression test verified to fail against the allow-list version:
`expected { clientId: 'cid' } to deeply equal { clientId: 'cid', onInsufficientScope: 'throw' }`.
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot round 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>
