Step 1 of phase 2 of the App.tsx decomposition, landed first and alone
because it is the seam the remaining clusters are extracted onto.
App.tsx kept six "latest value" refs so long-lived callbacks could read a
current value without listing it as a dependency and being re-created on
every change: `activeServerNameRef`, `activeServerIdRef`, `serversRef`,
`inspectorClientRef`, `pendingStepUpRef` and `pendingReauthRef`, plus the
three passive effects that synced them. `useSessionRef` replaces all nine
with one hook returning one stable ref onto a `SessionSnapshot`.
This is not a store: nothing renders off it, nothing subscribes to it, and
mutating it never schedules a render. It is not the prop-to-state sync
AGENTS.md forbids either — that rule is about `setState` in an effect, and
a ref write produces no render at all. It is exactly the pattern the file
already used, written once instead of six times.
What it buys the later steps is a *stable* dependency: a hook taking
`sessionRef` does not drag the other clusters into its argument list, so
`useOAuthRecovery`, `useConnectionLifecycle` and `useServerCommands` can
be extracted one PR at a time.
Behavior is unchanged:
- The snapshot is seeded from the first render's values, matching what
each `useRef(value)` seeded.
- `activeServerName` keeps its one non-uniform rule — it holds the last
server that *was* active and is never cleared, because a transport
crash clears `activeServerId` before the failure toast is raised. It is
derived from `servers`/`activeServerId` rather than taking the
`activeServer` memo, which is the same value by definition and is what
lets the hook be called ahead of that memo.
- The two pending-* syncs already ran ahead of the only effect that reads
them, so merging them changes no effect's view. The other four synced
below every effect that reads them, so hoisting that sync changes none
either.
- The sync effect is deliberately un-gated rather than carrying a
dependency list: re-assigning unchanged values costs nothing and
removes a place for a newly-added field to be forgotten.
`sessionRef` is declared ahead of its first reader because a hook return
is not provably stable to `react-hooks/exhaustive-deps` — consumers must
list it, and a later declaration would put every one of them in the
temporal dead zone. Listing a ref that never changes identity re-runs
nothing.
The inline `pendingStepUp` state shape is named `PendingStepUp` and moved
beside `StepUpSource` in `utils/stepUp.ts`, so the snapshot can name it
without importing from the component that owns the state.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01MZdpUwhHf943FoprcLVzEm
Signed-off-by: cliffhall <cliff@futurescale.com>
- Move REPLAYABLE_PROTOCOL_METHODS / isReplayableProtocolMethod out of
components/groups/protocolUtils.ts into utils/replayableProtocolMethods.ts.
It is a pure predicate over a string set, so by the project's own lib-vs-utils
rule it never belonged under components/ — and keeping it there was what made
lib/protocolReplay.ts import 'up' into the component graph. Both consumers now
import from utils/; the predicate keeps its own test.
- Give replayProtocolRequest an exported ReplayClient = Pick<InspectorClient, ...>
naming the nine methods it calls, so a caller (and the test) can satisfy the
real contract instead of casting a stub into a whole client.
- Rebuild the protocolReplay fixtures as plain typed MessageEntry values. The
double cast had been hiding a real defect: MessageEntry.timestamp is a Date
and the fixture passed a number, so the assertion confirmed a shape
messagesToLogEntries cannot produce.
- Replace the window casts in authToken.test.ts with Reflect.set /
Reflect.deleteProperty. No 'as unknown as' remains in either test.
- Drop the stale 'floats top-right' sentence on ReAuthBannerBar, which moved
verbatim and arrived contradicting the paragraph below it.
Declined: re-homing ReAuthBannerBar's transform/zIndex as a theme variant.
'styles' is the Styles API rather than an inline style, Mantine exposes neither
property as a prop, and a styling change is out of scope for a phase whose
safety argument is that nothing was edited.
Signed-off-by: cliffhall <cliff@futurescale.com>
Phase 0 of #2126. Lines 239-668 of clients/web/src/App.tsx were already at
module scope — helpers, formatters, constants, and the toast subcomponents,
none of which close over component state — so the whole block moves verbatim
into files. App.tsx drops from 5,303 to 4,892 lines with no behavior change.
Placement follows the lib-vs-utils rule in AGENTS.md: pure computation to
utils/, anything touching the environment to lib/.
EMPTY_SETTINGS and the two loose toast ids deliberately do NOT land in a new
top-level src/constants.ts, which the issue suggested: the web coverage
include is a whitelist naming components/hooks/theme/lib/utils/server, so a
module there would have fallen out of the >=90 gate silently. They go under
utils/ instead.
ReAuthBannerBar is in the block but not in the issue's table — it was added
after the issue was written (#2108). It moves to components/groups/ReAuthBanner/,
beside the banner it wraps.
Every new module is at 100% on all four coverage dimensions. Two v8 ignores,
each justified in place: getAuthToken's typeof-window guard (a happy-dom
inherent path) and replayProtocolRequest's provably-dead default case.
Signed-off-by: cliffhall <cliff@futurescale.com>
- `const` constrains a property that is PRESENT; it neither requires the
property nor acts as a default. It is now supplied automatically only where
the schema also requires the field — in `collectSchemaDefaults`,
`applySchemaConstants` and the TUI's initial values alike — so an optional
`dryRun: { const: true }` no longer turns a valid `{}` call into one that
asks the server to do something. A value that IS present is still corrected
to the constant, whatever supplied it, and the field stays read-only.
- Offerability is judged on the base+member merge rather than the raw member,
so `anyOf: [{ required: ["email"] }, { required: ["phone"] }]` over
root-declared properties is offered. It was declined, which left the gate
checking the base alone and accepting `{}` — which that schema rejects.
- The TUI's branch sections keep the schema's own `required` for the seeding
decision while still rendering every branch field non-required, which is what
keeps a static form submittable.
Signed-off-by: cliffhall <cliff@futurescale.com>
A schema may require a property it never declares: `{ type: "object",
required: ["token"] }` is legal and the tool plainly takes an argument.
declaresAnyFields counted only `properties`, so an App tool shaped that way —
including nested under a composition — was reported input-free and
auto-invoked with `{}`.
Signed-off-by: cliffhall <cliff@futurescale.com>
- The seeding effect re-applies constants as well as filling absences: an
in-place schema change can move a `const` the user cannot edit, and the
read-only field then displayed the new value while `values` still held the
old one — which is what would be submitted. Ordinary defaults are still only
ever added, so an edited field keeps what the user put there.
- hasMissingIn checks own-property presence: a required argument legally named
`constructor` resolved to the inherited one and read as supplied, enabling
a submit the schema rejects.
- The TUI keeps the declared name as a branch field's fallback title. The
prefix is an internal field NAME, and buildFields labels from its map key —
so the showcase displayed `__b0__address` where the schema says
`address`.
Signed-off-by: cliffhall <cliff@futurescale.com>
The App deep link seeded defaults and spread its `appArgs` over them, which
replaces a nested object wholesale: `{ config: { kind: "sms" } }` discarded
the nested SMS branch's own defaults, which the form then displayed while the
submitted arguments omitted them. New seedSchemaValues merges the two per
level, picking each nested branch from the nested values, and the supplied
value still wins wherever the two meet.
Signed-off-by: cliffhall <cliff@futurescale.com>
collectSchemaDefaults recursed only when a nested object declared `properties`
directly, so one keeping its fields on a composition branch was skipped — while
SchemaForm rendered that branch and displayed its read-only discriminator. The
constant was shown and never submitted, and the server rejected the call. The
recursion now gates on declaresAnyFields.
Signed-off-by: cliffhall <cliff@futurescale.com>
- rootUnion reads every composition keyword through `membersOf` and every
`required` through `requiredOf`. These schemas describe the wire and each
member arrives as `unknown`, so `anyOf: {}` or `required: "x"` really
can arrive — and reading them as lists threw, taking all three clients down
rather than declining one malformed tool. Both web and TUI required-field
checks are guarded the same way.
- admitsNull evaluates a sibling `anyOf` beside a `const: null` instead of
refusing outright: the union decides in both directions, and
`{ const: null, anyOf: [{ type: "null" }] }` admits null as plainly as an
all-string union rejects it. Refusing it left a read-only required field
seeded with the only value it accepts and submit permanently disabled.
Signed-off-by: cliffhall <cliff@futurescale.com>
hasMissingRequiredFields treated every empty string as missing, so a required
field the schema pins to `""` — rendered read-only, and seeded with the only
value it accepts — left Execute and Open App permanently disabled. It now
matches the TUI check: an empty string counts as supplied when the property's
own `const` pins it there, and an ordinary blank required string still does
not.
Signed-off-by: cliffhall <cliff@futurescale.com>
- selectBranchIndex no longer stops when several branches share the supplied
constant. A branch is dropped only when a supplied value CONTRADICTS one of
its constants; the required-field and exclusive-name passes then run over the
remaining candidates, so { version: 1, phone: "555" } resolves to the SMS
branch instead of falling back to the first.
- hasMissingRequiredFields counts only the branches the values could be making
(new branchAcceptsValues): { kind: "sms", address: "x" } supplies
everything the EMAIL branch requires while contradicting its discriminator,
and would otherwise enable a submit the server refuses.
- convertToolParameters ignores a malformed property declaration when branches
agree on a type, rather than storing a non-schema as the coercion schema.
Signed-off-by: cliffhall <cliff@futurescale.com>
- rootUnion: a merged declaration pairing a `const` with an assertion this
module cannot evaluate (`minimum`, `pattern`, …) is declined. Proving the
conjunction safe is the requirement, not disproving it — `minimum: 10`
beside `const: 1` is as unsatisfiable as a type mismatch.
- declaresAnyFields counts a `$ref`: its shape is unknown rather than empty,
so `anyOf: [{ $ref }, { $ref }]` no longer reports an App tool as
input-free and auto-invokes it with `{}`.
- SchemaForm keys its draft-holding fields by the active branch as well, so a
half-typed `-` or an unparsed JSON draft cannot survive a switch into a
same-named field of the incoming branch — with both parent values
`undefined`, nothing else tells the field the entity changed.
Signed-off-by: cliffhall <cliff@futurescale.com>
collectSchemaDefaults assigned by name, so a legal field called `__proto__`
went to the legacy prototype setter instead of the result. A required one
pinned by `const` would then render read-only and be seeded with nothing,
leaving submit permanently disabled. Seeded through defineProperty now, the
way the other schema-record paths already build their maps.
Signed-off-by: cliffhall <cliff@futurescale.com>
applySchemaConstants only corrected top-level pinned fields, but the deep
link's overlay replaces a nested object wholesale rather than merging into it —
so `{ config: { kind: "sms" } }` slipped past a nested read-only
`kind: { const: "email" }` the form was displaying. It now recurses into
object-valued properties, resolving each one's own root union the same way.
Signed-off-by: cliffhall <cliff@futurescale.com>
- selectBranchIndex compares a discriminator structurally: a `const` may be
an object or array, and deep-link arguments arrive as freshly parsed
instances that could never be reference-equal to the schema's own.
- New applySchemaConstants, applied by InspectorView AFTER the deep link's
`appArgs` are spread over the seeded defaults. The form renders a pinned
field read-only, so a link disagreeing with one would otherwise auto-open an
App with a hidden value contradicting the shape on screen.
Signed-off-by: cliffhall <cliff@futurescale.com>
- rootUnion: an `allOf` member is foldable only when it states nothing beyond
what the merge applies (`type`/`properties`/`required` plus annotations),
so a nested `anyOf`, a `not`, or an `additionalProperties` can no longer
be erased along with the keyword; members are checked against the
ACCUMULATED merge, so two of them contradicting each other is caught even
when neither contradicts the root.
- admitsNull: `const: null` admits null, so a required field pinned to it is
no longer seeded with the one value it accepts and then reported missing.
- collectSchemaDefaults takes the values a caller is about to overlay, and
seeds the branch THEY identify — the App deep link passes its `appArgs`, so
branch 0's defaults can no longer sit invisibly under branch 1's arguments.
- The branch inference moves into core as `selectBranchIndex`, shared by the
seeding and the form's picker so the two cannot disagree.
Signed-off-by: cliffhall <cliff@futurescale.com>
- rootUnion: a branch restating a constraint the root states differently is
declined (keywords at one level are conjunctive and no intersection is
computed here), while annotations may disagree freely. An `allOf` with a
member it cannot fold in — JSON Schema's boolean form, or a $ref whose
constraints are unknown rather than absent — is left intact instead of
merged-and-stripped, which would turn an unsatisfiable schema into a
fillable form.
- rootUnion: `declaresAnyFields` counts the raw composition members, so
`hasInputFields` reports fields for a union the resolver declines and an
App tool is never auto-invoked with empty arguments.
- SchemaForm: the picker opens on the branch the supplied values identify
(an App deep link overlays its args, so they can name another branch), and
a branch switch no longer carries a value the outgoing branch declared —
the branches may type the same name differently.
- TUI: a base property some branch redeclares is offered in EVERY branch's
section under that branch's inherited or specialized declaration, so a
branch that does not redeclare it can still supply a required root
argument.
Signed-off-by: cliffhall <cliff@futurescale.com>
- rootUnion: decline a schema carrying BOTH oneOf and anyOf rather than
reading one and silently dropping the other's constraints.
- SchemaForm: handle `const` before every type dispatch and for every
constant type, so a numeric/boolean constant, or a schema carrying both
`const` and `enum`, can no longer offer a forbidden value.
- collectSchemaDefaults / TUI initialValue: `const` outranks `default`,
which is an annotation a schema may set to something its const rejects.
- convertToolParameters: resolve from the first branch's merged schema, so a
branch's specialization of a root-declared property is what types the arg.
- TUI: ink-form keys values by field name across the whole form, so two
branches' `kind` were one field and the later section's initial value
decided what the earlier submitted. Branch fields now render under prefixed
names behind a Variant select, and decodeFormValues translates back on
submit, dropping every branch but the chosen one.
Signed-off-by: cliffhall <cliff@futurescale.com>
A tool's `inputSchema` may carry composition keywords at its root — the
2026-07-28 revision makes this explicit — but both form builders enumerate
the root's `properties` and nothing else. A schema keeping its fields on
`allOf`/`oneOf`/`anyOf` branches therefore rendered no controls at all: no
branch picker, no fields, not even the raw-JSON fallback a union-typed
*property* falls back to, so the tool could only be called with empty
arguments.
Adds `core/json/rootUnion.ts`, one flattening shared by all three clients so
they cannot drift on which schemas they can render:
- `allOf` is merged unconditionally (conjunctive — there is no choice to
present), a root `oneOf`/`anyOf` is returned as branches.
- Web `SchemaForm` renders a Variant picker above the fields and prunes the
outgoing branch's values on a switch, since they are no longer visible and
would describe a shape the call is not making.
- TUI `schemaToForm` gives each branch its own section with optional fields —
ink-form is static, and only one alternative applies to a call.
- `convertToolParameters` looks through the branches, so a CLI `--tool-arg`
is typed by the schema that declares it rather than sent as a string.
Required-field gating asks whether *any* branch is satisfied, which is
selection-independent and never blocks arguments the schema accepts. A
`const` is seeded like a `default` and rendered read-only: it admits exactly
one value, so anything else the user could type produces a rejected call.
Adds a `root-union-schemas-http.json` showcase server.
Closes#2123
Signed-off-by: cliffhall <cliff@futurescale.com>
Review catch: `'unsafe-eval'` *is* a valid CSP source expression, so saying
the filter drops entries that "don't parse as a source expression" was both
wrong and a rerun of the misconception this PR set out to fix — and it
contradicted the NOTE two paragraphs down.
The doc now names the accepted subset (`scheme://host[:port][/path]`, a bare
or wildcard host, a scheme such as `data:`, and `*`) and says explicitly that
a CSP keyword is a valid source expression these origin-list fields simply
cannot express. The now-duplicated half of the third paragraph is trimmed.
Comment-only; the warning string and tests are unchanged.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UL9retfvAXRgvi6EWk4SY4
Signed-off-by: cliffhall <cliff@futurescale.com>
`approveCspSources` warned "dropping unsafe CSP source", which states a
security verdict the check never makes: `SAFE_CSP_SOURCE` screens each
entry for injection safety only, and deliberately does not bound how
broad a grant is (`resourceDomains: ["*"]` passes).
The four fields are origin lists, so the real cause of a drop is almost
always that the entry isn't an origin — a CSP keyword such as
`'unsafe-eval'` has no field in the contract to be requested through.
Reading it as a safety verdict is what sent #2012 chasing an XSS
regression that wasn't one.
The message now names the field the entry came from (so an app declaring
several lists can tell which one was bad) and states the shape expected,
with no claim about the value's safety.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UL9retfvAXRgvi6EWk4SY4
Signed-off-by: cliffhall <cliff@futurescale.com>
uiMeta returned undefined for both "this bag declares no ui" and "its ui
is unusable", so a content item declaring `ui: null` fell through and was
granted whatever a BROADER carrier asked for — wrong precedence, wrong
direction to fail in. It now distinguishes the two: no `ui` key means
keep looking, a declared-but-malformed one decides for "no hints".
Also pin the full three-way precedence with a case carrying all three
carriers at once (no prior case would have caught the first two operands
being swapped), and state why a permission's object value need not be
empty: `{}` is the spec's extension point, so rejecting a populated one
would drop grants from servers written against a newer spec.
Signed-off-by: cliffhall <cliff@futurescale.com>
approveSandboxPermissions screens untrusted `_meta`, so it takes
`unknown` and decides the shape itself rather than claiming its input is
already an McpUiResourcePermissions — which had forced its own tests to
cast malformed fixtures through `as unknown as`.
Also stop honoring `true` as a permission marker. The spec defines every
permission as an object (`camera: {}`), so `true` was an undocumented
shorthand, and honoring it means a server typo turns an iframe permission
on. Only the object marker grants now; arrays are rejected too.
Signed-off-by: cliffhall <cliff@futurescale.com>
The sandbox proxy's buildAllowAttribute() tests each permission key for
truthiness, so an untrusted `_meta.ui.permissions` bag could stand a
non-boolean in for a grant — `{ camera: "false" }`, a plausible way for a
server to mean "off", reads as on. Nothing screened it, because before
the `_meta.ui` fix no permissions reached the proxy at all.
Add approveSandboxPermissions alongside approveCspSources: keep only the
four keys the proxy knows, and only where the value is the spec's empty-
object marker or `true`; drop anything else (warning on a malformed
value, silently on an explicit `false`) and return undefined when nothing
survives. Both the notification and the hostCapabilities echo now carry
the approved set, so the app sees what was granted.
Also document what the listing lookup can and cannot see under
`paginatedLists`.
Signed-off-by: cliffhall <cliff@futurescale.com>
`StrictJsonValue` removed `undefined`, but the type cannot express the rest of
the gap: TypeScript has one `number`, and JSON has no encoding for `NaN` or
`±Infinity`, so `JSON.stringify` rewrites them to `null`.
This is reachable from ordinary input, not a careless caller.
`JSON.parse('{"a":1e400}')` returns `Infinity` on overflow — so text a user
types parses cleanly, satisfies the type, and reaches the server as `null`
while the editor still shows `1e400`.
`isSerializableJson` in `core/json` is the runtime half of `StrictJsonValue`,
applied at both boundaries that turn text into metadata:
- the editor's draft parser, which now reports "Numbers must be finite" inline
and withholds the value rather than emitting one it cannot send;
- the CLI's `--metadata` / `--tool-metadata` parser, which throws. Silently
sending `null` would be wrong, and so would falling back to the literal
string — neither is what was asked for.
Tests cover the validator directly (including that `JSON.stringify` really does
write `null`, which is the reason it matters), the editor parser, and the CLI
flag.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0177mPHAdECD18nLLCTwR5rh
Signed-off-by: cliffhall <cliff@futurescale.com>
- `JsonValue` admits `undefined`, so `RequestMetadata` accepted values that do
not survive serialization: `JSON.stringify` drops an `undefined` object
member and rewrites an `undefined` array element to `null`. The type
contradicted the one promise this payload makes. Added `StrictJsonValue` /
`StrictJsonObject` and used them for the metadata path.
Deliberately not a change to the repo-wide `JsonValue`: for tool *arguments*
`undefined` legitimately means "not supplied". The narrowing is sound
wherever it lands here, since every one of these values comes from
`JSON.parse`, which cannot produce `undefined`. The CLI's `parseKeyValuePair`
needed the same tightening — caught by `clients/cli`'s typecheck, which the
web `tsc -b` does not cover.
- Nothing asserted the JSON worker's annotations, and the wrapper's own "Not
valid JSON" message comes from our parser rather than from Ace — so a broken
worker URL would have left every test passing while the gutter diagnostics
silently disappeared. Added a Storybook play function that waits for a real
`session.getAnnotations()` error entry, and verified it fails when the worker
module URL is pointed at a bad path.
- `aria-describedby` referenced only the error, so a supplied `description` was
never announced and would have been dropped entirely once the draft went
invalid. Both ids are referenced now, with the description surviving the
error.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0177mPHAdECD18nLLCTwR5rh
Signed-off-by: cliffhall <cliff@futurescale.com>
`react-refresh/only-export-components` rejects a non-component export from a
component module, so `parseObjectDraft` could not live beside
`JsonObjectInput`. It is a pure transform, which puts it in `utils` under the
`lib`/`utils` rule anyway.
Also render the metadata hint's inline literals through `<Code>` rather than
markdown backticks, which a Mantine `Text` shows verbatim.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0177mPHAdECD18nLLCTwR5rh
Signed-off-by: cliffhall <cliff@futurescale.com>
Addresses Copilot's round-19 review on #2035 -- a divergence introduced by
my own round-14 guard rather than by the original code.
The SDK validated a value inside `encodeValue`, i.e. only after looking one
up by a DECLARED name. The restored guard checked the caller's whole map, so
an unrelated entry failed a template that never mentions it. Measured on
9caac43d:
tryExpandUriTemplate("x://{id}", { id: "7", stale: "a".repeat(1_000_001) })
=> error: The value for "stale" exceeds the 1000000-character limit
RFC 6570 ignores an extra variable, so this refused a perfectly valid read.
`valueLengthError` now takes the names to consider and both callers pass the
template's declared set.
Copilot's second half is worth taking on its own: strict expansion parsed
the template THREE times -- inside `templateError`, again for the name list,
again to expand -- and the template is server-controlled and may be 1 MB.
It is now parsed once and reused, with validation split into
`sdkTemplateError` (the constructor's unclosed-brace and length/count
limits) and `partsError` (this module's own grammar), so `templateError` and
strict expansion share both halves rather than duplicating either.
npm run ci passes.
Signed-off-by: cliffhall <cliff@futurescale.com>
Addresses Copilot's round-18 review on #2035. Two suppressed comments, both
holes in my own recent fixes rather than in the original code.
1. Round 16's grouping keyed off the length-announcing lead byte alone, so
it accepted sequences RFC 3629 forbids -- and the comment claiming they
fell back to per-triplet counting was simply wrong. Measured on 05bbfe50:
x{+v:1} "%C0%80x" => "x%C0%80" overlong
"%E0%80%80x" => "x%E0%80%80" overlong
"%ED%A0%80x" => "x%ED%A0%80" UTF-16 surrogate
"%F4%90%80%80x" => "x%F4%90%80%80" past U+10FFFF
The lead-byte ranges are now the well-formed ones -- C2-DF, E0 only with
A0-BF, ED only with 80-9F, F0 only with 90-BF, F4 only with 80-8F -- so
each of the above falls back to one triplet, which is what the fallback
was always documented to do. The legal boundary cases (%E0%A0%80,
%ED%9F%BF, %F4%8F%BF%BF) are tested alongside, so tightening the ranges
cannot quietly start splitting valid characters.
2. The preview never ran the read's template validation. It expands
expression by expression, so `x://{a}}/{b}` with both fields filled
previewed `x://1}/2` -- a URI whose stray `}` makes every read refuse.
Validation moved into an exported `templateError` that both callers
consult, so they cannot drift. It subsumes the narrower rule round 12
added here ("leave the invalid part standing"), which still let the REST
of such a template expand: the refusal is per template, not per
expression, so the preview now shows the template verbatim -- literals
unencoded included, since half-normalizing an unreadable template invents
a string the server never published. Two existing tests asserted the
weaker behavior and now assert this one.
npm run ci passes.
Signed-off-by: cliffhall <cliff@futurescale.com>
Addresses Copilot's round-16 review on #2035. Two suppressed comments, both
real, and both refinements of fixes from the previous two rounds rather than
defects in the original code.
1. Round 11 settled on "a pct-encoded triplet counts as one character", but
a multi-octet character spans several triplets -- `%C3%A9` is one `é`.
Measured on a3dd8563:
x{+v:1} v = "%C3%A9x" => "x%C3" a lone lead byte, decoding to nothing
`prefixUnits` now recognizes a well-formed pct-encoded UTF-8 sequence from
its lead byte (RFC 3629: C2-DF two octets, E0-EF three, F0-F4 four;
continuations 80-BF), longest alternative first so a four-octet sequence
is not read as a two-octet one followed by loose triplets.
A malformed sequence -- a lead byte with too few continuations, or a stray
continuation -- matches no sequence alternative and falls back to
per-triplet counting. That is the conservative direction: it counts as
MORE units, so truncation keeps less rather than emitting octets that
were never in the value.
2. Round 14's per-value ceiling only covered the read. The panel calls
`previewUriTemplate` on every keystroke, and that path expands expression
by expression, so it reached the allocation-heavy encoders with the same
oversized value -- submission refused while the UI froze on the paste
anyway. Both paths now share one exported `valueLengthError`, checked
before any encoding happens.
npm run ci passes.
Signed-off-by: cliffhall <cliff@futurescale.com>
Addresses Copilot's round-13 review on #2035. Two suppressed comments, both
real, both in the required-group work from round 7 rather than in the recent
preview rework.
1. `{a,a}` is ONE requirement named twice, but `requiredGroups` stored it as
a two-name group, so everything downstream read it as "either of these
will do". The TUI was left with a form it could not submit: its
`length === 1` test marked `a` optional while ResourceTestModal's guard
still refused a blank. The web panel offered the useless hint
"Any one of: a, a". Names are now deduplicated within the expression --
expansion still emits both occurrences (`x://{a,a}` -> `x://1,1`), which
RFC 6570 requires.
2. The per-field hint used `find`, so a variable in several shared groups
advertised only the first. With `{a,b}{b,c}{a,c}`, filling `b` satisfies
the first two and every visible hint then looks met while Read Resource
stays disabled on the unmet `{a,c}` -- a requirement with nothing on
screen pointing at it.
Fields now hint every shared group they sit in. That alone is not enough,
because no per-field hint can say WHICH group is still outstanding, so
the form states it directly: "Still needed: a or c", built from the same
`unmetRequiredGroups` the submit gate uses, so the message and the button
cannot disagree. `hasRequiredValues` is no longer called here -- the
panel derives both from the one list.
npm run ci passes.
Signed-off-by: cliffhall <cliff@futurescale.com>
Addresses Copilot's round-12 review on #2035. Both halves are regressions
from my own round-10 rework, which replaced "expand a rewritten template
through the lenient expandUriTemplate" with a part-by-part assembly and so
lost two properties that implementation had for free. Reproduced on
c6fb6271:
previewUriTemplate("x://{v}", { v: "\ud800" }) => throws URIError
previewUriTemplate("x://{a,}", { a: "1" }) => "x://1"
The first is the worse one: the preview runs during render, so an escaping
throw unmounts the whole panel rather than disabling its button -- and an
unpaired surrogate is real input, since `encodeURIComponent` raises URIError
on one and a paste can deliver it. The panel already reports that case
properly through `tryExpandUriTemplate`; the display path now degrades to
the raw template instead of taking the panel down with it.
The second promised a URI the user could never send: `{a,}` is an invalid
template whose submission is refused, but `a` survives into the
expression's varspecs, so filling it previewed `x://1`. An invalid
expression is now left standing as written, which is what the rest of the
panel already says about it.
Tests for both, plus one asserting the valid expressions around an invalid
one still preview.
npm run ci passes.
Signed-off-by: cliffhall <cliff@futurescale.com>
#1961 keyed the sidebar's React children by source position, but everything
downstream still identified a tool by `tool.name`. With a `tools/list` that
repeats a name, both copies highlighted at once, the click guard made the
other copy a no-op, and `tools.find((t) => t.name === …)` always resolved to
the first — so a later copy rendered but could never be opened.
Thread the row key through selection instead:
- `utils/toolUtils.ts` now owns `toolRowKey` (moved out of ToolControls) and
adds `findToolByRowKey`, which compares computed keys rather than parsing
one so the two can't drift.
- `ToolControls`: `selectedName` → `selectedKey`; the highlight and the click
guard compare keys, and `onSelectTool` emits the row key.
- `ToolsScreen`: `ToolsUiState.selectedToolName` → `selectedToolKey`, with
both lookups going through `findToolByRowKey`. Renamed through
`screenUiState.ts` and `App.tsx`'s `onToolsUiChange`.
`onCallTool` still sends `selectedTool.name` — the duplicated name is the
genuine wire identity; only the UI identity needed to be unique.
Also prefixes the excluded (SEP-2243) rows' keys: they render into the same
`Stack` as the list rows but index within their own list, so an excluded tool
sharing a name and index with a listed one collided in the key space.
Closes#2001
Signed-off-by: cliffhall <cliff@futurescale.com>
Addresses Copilot's round-10 review on #2035. One inline comment, one
suppressed; both real.
1. Literals were copied through verbatim, but RFC 6570 3.1 requires them
pct-encoded on expansion -- the conformance case `café/{var}` expands to
`caf%C3%A9/value`. Measured on ef3f5e74 it produced `café/value`, raw
UTF-8 in the path; since this module replaced the SDK's expander for both
clients, nothing else was going to encode it. (The pinned SDK does not
either -- it returns `café/value` too -- so this is a gap we inherited
rather than introduced.)
`encodeLiteral` keeps reserved delimiters (a literal's `/`, `?` and `#`
are structure) and existing pct-triplets, and encodes the rest. A BRACE
passes through: it can only reach a literal from a malformed template,
where an unclosed `{` becomes trailing text, and `%7B` would leave a
string resembling neither the template nor anything the server can match.
This is also what removed the preview's placeholder machinery. No token
can survive a round trip now that literals are encoded, so the preview
assembles part by part instead -- `encodeLiteral` for literal runs,
`expandTemplateExpression` for filled expressions, `part.source` for the
rest. Both halves still come from the shared expander, so the preview
cannot promise a URI that submitting would not send, and the question of
what token could never collide with a server-supplied template is gone.
2. On the varname set I took the structural point rather than the
conclusion. `VARCHAR` is RFC-exact again; `-` and `~` moved into a
separately named `TOLERATED_VARCHAR` tier, and a name needing it parses
with `conforming: false`, surfaced on `TemplateVariable`.
So the grammar in the code says what the RFC says and the tolerance is
explicit and separately tested -- but the Inspector still expands
`{default-graph-uri}`, because real servers publish hyphenated names and
the SDK's matcher round-trips them. Refusing to read a resource that
demonstrably works is a worse failure for a debugging tool than expanding
a name the conformance suite would reject. A caller wanting RFC-exact
behavior refuses on the flag rather than re-deriving the grammar.
npm run ci passes.
Signed-off-by: cliffhall <cliff@futurescale.com>
Addresses Copilot's round-8 review on #2035 (one inline comment, one
suppressed). Both real.
1. RFC 6570 distinguishes an UNDEFINED variable, which omits its whole
expression, from one defined as the empty string, which expands. The
expander was calling `definedValues` on the way in, collapsing the two --
so `x{?q}` with `{ q: "" }` gave `x` instead of `x?q=`, and there was no
way for any caller, `readResourceFromTemplate` included, to request
either that or `x;q`. Key presence is now honored, including §3.2.7's
rule that the matrix operator drops the `=` for an empty value (`x{;q}`
-> `x;q`, while `?`/`&` keep it).
Dropping blanks is a FORM concern, so it moved to the two form
boundaries: the web panel seeds every declared variable with "" and the
TUI's ink-form returns "" for every untouched field, so neither can tell
"never touched" from "deliberately empty" -- but that is a fact about a
text input, not about the template, and it has no business deciding what
a template means for every other caller.
Six unit tests and one integration test encoded the old collapse and now
assert the corrected meaning. The prototype-collision cases get stronger
rather than weaker: a blank `{?toString}` expands to `?toString=` -- its
OWN empty value -- where the original bug expanded the inherited
function's body.
2. The expansion error is now `role="alert"`. It can appear while typing (a
pasted value that cannot be encoded), and plain text is not announced, so
a screen-reader user met a disabled Read Resource with no stated reason.
npm run ci passes.
Signed-off-by: cliffhall <cliff@futurescale.com>
Ports the two behaviors PR #2033 (the parallel attempt at #1919, now closed
as a duplicate) got right and this branch did not.
1. An expression declaring no variable is a malformed template, not one
with a member to skip. RFC 6570 requires at least one varspec per
expression and admits no empty member, so `{}`, `{,}`, `{a,}`, `{?}`
and `{*}` (an explode modifier is not a name) now make the template
invalid. Skipping them was the more dangerous reading: `x://{}`
expanded to `x://` while the form rendered no inputs, so its
"everything required is filled" check was vacuously true and it
submitted a URI that is not the template the server published.
2. The read is withheld when expansion fails. `expandUriTemplate`'s
raw-template fallback exists for the preview, which runs during render
-- but the submit path used it too, so an invalid template was read
with its braces intact and the server answered with a confusing "not
found" for a defect that is not the user's. `tryExpandUriTemplate`
returns the URI or the reason as a value the caller cannot mistake for
one; the panel gates Read Resource on it and prints the reason.
This also covers a value that cannot be encoded -- an unpaired
surrogate has no UTF-8 encoding, so `encodeURIComponent` throws
`URIError` on it, and a text input can hold one via paste.
`requiredGroups` now skips an expression naming no variable: an empty
group can never be satisfied, so it would gate the form a second time on
a condition nothing can meet, and the "any one of" hint built from it
would name no fields. The accurate reason is the malformed template,
which the expansion gate reports.
npm run ci passes.
Signed-off-by: cliffhall <cliff@futurescale.com>
Addresses Copilot's round-6 review on #2035.
`toString`, `constructor`, `valueOf` and `__proto__` are all valid RFC
6570 variable names -- `varname` allows ALPHA / DIGIT / `_` / pct-encoded
-- and every lookup here was a bare `values[name]`, which finds
`Object.prototype`'s member for each of them. Measured: `({})["toString"]
!== undefined` is true with typeof "function", and `({}["constructor"] ??
"").length` is 1.
So a *blank* `{?toString}` field read as supplied and expanded a function
body into the URI instead of omitting the expression, and
`hasRequiredValues([["constructor"]], {})` returned true because
`Object.length` is 1 -- a required group satisfied by nothing.
All reads now go through an own-property `readValue`, and the web
preview's filled-check uses `Object.hasOwn`. `expandExpression` carries
the value through its filter rather than re-reading it afterwards, so the
"is it defined" test and the read cannot disagree and nothing needs a
non-null assertion.
The remaining six findings were documentation drifting behind the code,
which matters here because the comments are the argument for why this
module exists:
- The web helper still described the SDK's second-`?`-to-`&` rewrite as
something the preview relies on. That was removed last round.
- Three places still named the CLI as a consumer. It has no template form
and passes an already-expanded `--uri` to `readResource`.
- The PR description still described the superseded architecture
(expansion delegated to the SDK), i.e. the opposite of what now ships.
Signed-off-by: cliffhall <cliff@futurescale.com>
Addresses Copilot's round-5 review on #2035 — eight suppressed comments,
all of them real.
The consequential one overturns behavior I copied from the SDK and
defended in two earlier rounds. RFC 6570 expands each expression
independently; it carries no cross-expression state. The SDK tracks
whether a query expression has already emitted and rewrites a later
`{?two}`'s `?` to `&`. That looks friendlier and is wrong, and its own
matcher says so: for `x{?one}{?two}` it expands to `x?one=1&two=2`, and
`match("x?one=1&two=2")` on that same template returns **null**, while
`match("x?one=1?two=2")` returns both variables. So the rewrite emitted a
URI the advertised template cannot match — #1919 one level up. A server
that wants a continuation advertises `{?one}{&two}`, which expands to `&`
and matches; producing that shape is the server's choice to declare, not
ours to infer.
Also:
- `{id:abc}` was silently treated as `{id}`, and a test codified it. RFC
6570's max-length is `%x31-39 0*3DIGIT`, so `{id:}`, `{id:0}`,
`{id:abc}` and `{id:10000}` are invalid templates rather than templates
with an ignorable modifier — and the SDK's constructor accepts them all,
so nothing else rejects them. Strict expansion now throws; lenient
returns the raw template. Discovery stays lenient so the panel still
renders rather than going blank.
- The TUI could submit `{a,b}` completely blank while the web panel
blocked the same request: ink-form has no way to express "any one of
these", so its members are left optional and the modal needs its own
group check. Added, with tests — it was also below the coverage gate
without them (functions 76.92%).
- Added the integration test the config never had: it resolves the
checked-in config and drives all four reads over a real transport, so a
misspelt preset fails there rather than only when someone runs the repro
by hand.
- "All three clients" was false. The CLI has no template form and its
`resources/read` passes the already-expanded `--uri` straight to
`readResource`, so nothing here runs for it. Corrected in the README,
AGENTS.md, and the module comment, and dropped a docblock still
describing the SDK-delegation path removed last round.
Signed-off-by: cliffhall <cliff@futurescale.com>
Addresses Copilot's round-3 review on #2035. It again reported "no new
comments" while carrying two suppressed ones; both reproduced.
1. Requiredness could not live on a variable at all. Deduplication kept the
first occurrence's group, so in `x{?a}{?b}{a,b}` both names ended up
required with singleton groups and the form demanded both -- while the
SDK expands that template with only `a` to "x?a=11". Widening the stored
group would not have been enough either: in `{a,b}{a,c}`, filling `b` and
`c` satisfies both expressions, which no per-variable flag can express.
So requiredness is now returned per expression by `requiredGroups`, and
`hasRequiredValues` asks that each group be satisfied by any one of its
names. `TemplateVariable.groupNames` is gone rather than left as a field
that quietly means something narrower than it reads; `required` remains,
documented as driving the "Optional" marker and nothing else.
2. `encodeURI` is not the allow-reserved encoder `+` and `#` call for. It
escapes `[` and `]`, which are reserved and must survive, and it escapes
`%`, so an already-encoded value is double-encoded. Measured:
"[::1]" -> "%5B::1%5D" and "%2F" -> "%252F". Both corrupt the URI rather
than merely over-escaping it -- an IPv6 literal or a pre-encoded path
reaches the server altered, which is the same class of defect #1919 is
about.
Added an RFC 6570 3.2.1 encoder that preserves reserved characters and
existing pct-triplets, splitting on `%XX` so a lone `%` is still encoded
to `%25`, and matching with the `u` flag so an astral character is
encoded whole. `+` and `#` expressions are now taken over even for a
single name, so both expansion paths agree on what those operators mean.
Signed-off-by: cliffhall <cliff@futurescale.com>
Addresses Copilot's round-2 review on #2035. It reported "no new comments"
but carried three *suppressed* ones; all three reproduced against the
pinned SDK.
1. The `;` path-parameter operator is absent from the SDK's operator list,
so `{;id}` parsed as a variable literally named ";id" and expanded to
nothing. Added the operator and its named expansion (`;a=1;b=2`).
2. An RFC 6570 prefix modifier was folded into the variable name: `{id:3}`
yielded a variable called "id:3" and expanded to nothing. Varspecs are
now parsed properly and the value truncated before encoding. Truncation
is by code point, since `String.prototype.slice` counts UTF-16 units and
would split an astral character into a lone surrogate.
For both of these the wrong URI is the lesser problem: a form has to
*name* the variables it asks the user to fill, so the panel was
rendering fields labelled `;id` and `id:3` that nobody could use.
3. Requiredness was applied per variable, but it is a property of the
*expression*: RFC 6570 drops undefined names from a multi-name
expression, so `{a,b}` with only `a` filled expands to `a`'s value --
the SDK does this too. The panel was refusing input the expander would
have accepted. `hasRequiredValues` now encodes "any one name in a
required group suffices", and such a field reads "Any one of: a, b"
rather than falsely claiming each is mandatory.
Two structural consequences, each pinned by a test:
- Takeover is now per TEMPLATE rather than per expression. Splicing
corrected fragments into a template the SDK re-expands would leave its
cross-expression `?`-to-`&` rewrite blind to the fragments already
resolved.
- Expansion is split into a strict variant that throws and a lenient one
that returns the raw template. `readResourceFromTemplate` wraps the
thrown error with the template name -- three pre-existing integration
tests assert that -- while the form must not throw on a server-supplied
template, since that would take out the panel on render. The strict
variant constructs the SDK template unconditionally, because that
construction is what validates syntax: otherwise `x://{;a}{b,c` would
take the own-expansion path and its unclosed tail would pass as literal
text with nothing objecting.
Signed-off-by: cliffhall <cliff@futurescale.com>
Addresses Copilot's review on #2035. All three findings reproduced against
the pinned SDK before acting.
1. `#` was misclassified as required. Measured: `x://a{#frag}` with no
`frag` expands to exactly `x://a`, a well-formed URI naming a real
resource -- unlike `{+path}` (`x://a/`) or a simple `{userId}`
(`file:///users//profile`), which leave an empty path segment. Moved `#`
into the omittable set; the required cases now assert what the URI
*becomes* when blank, so the rule is checked rather than asserted.
2. Delegating to the SDK did not actually give RFC 6570 expansion.
`UriTemplate.expandPart` takes an early `names.length > 1` branch that
raw-joins values, skipping both `encodeValue` and the operator prefix:
`x://{a,b}` with `a = "foo/bar"` expands to `x://foo/bar,q` -- the very
unencoded-slash defect this PR is about -- and `x://a{/p,q}` to
`x://ax y,z`. Only `?`/`&` are correct, being dispatched earlier.
Fixing that in the web client alone would have left the TUI and CLI
wrong, since `readResourceFromTemplate` expands through the same class.
So parse/classify/expand now live in `core/mcp/uriTemplate.ts` and both
call sites use it. The correction is surgical: a multi-name non-query
expression is expanded here and spliced in as literal text before the
SDK sees the template (safe -- both encoders escape `{`/`}`), while
every single-name and query expression still goes through the SDK
untouched. The preview applies the same correction, so it cannot
promise a URI that submitting would not send.
3. The showcase promised a blank `{?topic}` read that did not work.
`UriTemplate.match()` compiles `{?topic}` to a *required*
`\?topic=([^&]+)`, so `match("foobar://events")` returns null and the
read 404s. A real server exposes the unfiltered collection as its own
resource; the showcase now registers `foobar://events` so the
documented step resolves. Verified end to end through the CLI.
Signed-off-by: cliffhall <cliff@futurescale.com>
The Resources screen discovered and substituted template variables with a
bare `/\{(\w+)\}/g` regex, which is wrong two ways: it cannot see an
expression carrying an operator, so `foobar://events{?topic}` rendered no
input at all; and it splices the raw value in, so a `/`, `?`, `#`, `%`,
space or non-ASCII character in a simple `{topic}` landed unencoded and
changed the URI's structure -- `foo/bar` produced an extra path segment
that a conforming matcher rejects with `-32602 Resource not found`.
Delegate expansion to the SDK's `UriTemplate`, the same implementation
`InspectorClient.readResourceFromTemplate` (and so the TUI) already
expands through, so the two clients cannot disagree about what a template
means. The new `utils/uriTemplate` supplies only what that class does
not: which variables to render an input for, which of them a read cannot
proceed without, and a partially-expanded preview.
Required-ness follows the operator. Under `?`, `&`, `.` or `/` the whole
expression is omitted when the variable is undefined, so those fields are
marked Optional and reading with them blank is a legitimate request for
the unfiltered resource; under `""`, `+` or `#` the variable sits mid-URI,
so it stays required. Blank fields are dropped before expanding so an
untouched optional field reads as undefined rather than as the empty
string, which would expand to a valueless `?topic=`.
Adds the `rfc6570_templates` preset and a `rfc6570-templates-http.json`
showcase server serving the two templates from the issue.
Closes#1919
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>
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>
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>
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>
