A modern-era `subscriptions/listen` is a long-lived request: the first
message on its stream must be `notifications/subscriptions/acknowledged`,
and the JSON-RPC `result` for the listen id is reserved as the
graceful-closure marker. A server that answers the listen request with a
bare `result` is saying "acknowledged and closed in the same breath" — the
SDK settles the subscription `graceful` and rejects the pending `listen()`.
The Inspector read that rejection as an ordinary drop: eight re-lists over
roughly a minute, the badge flickering `Reconnecting…`, and a final bare
`Stream ended` that said nothing about why. Accepting the message was
never wrong — it is a well-formed graceful-closure response. The silence
was.
- `core/mcp/subscriptionAck.ts` owns the predicate. The SDK gives this
close no code of its own — it is the same `SdkError(ConnectionClosed)`
any pre-ack close carries — so the predicate reads the message, since
the alternative is refusing to retry a genuinely transient drop.
- The condition is deterministic, so the stream ends on the first
occurrence instead of spending the whole backoff run. Both routes are
covered: the one a subscribe/unsubscribe reaches, and the one a
reconnect reaches when an acknowledged stream drops and the server has
since started refusing.
- A `"never-acknowledged"` status of its own, not `"ended"` — `ended`
means an expected close. It surfaces as the thrown error's message, an
orange "Not acknowledged" badge, and a notice in the panel.
- `subscriptions-never-acknowledged-http.json` reproduces it. It
acknowledges the first listen that subscribes to a resource and refuses
every one after, which is what makes the badge reachable by hand.
Closes#2097
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 cross-branch agreement check ignored `const`, so two branches pinned to
`1` and `"1"` both stated no `type`, both matched the text `1`, and
the first one's typed constant was sent. Textually indistinguishable constants
of different types are ambiguous, and the raw string the user typed is the
honest answer.
Signed-off-by: cliffhall <cliff@futurescale.com>
- The seeding effect is keyed by the fixed values themselves as well as the
entity and branch. A tool refreshed in place keeps its `resetKey` and its
branch while its schema changes underneath, so a newly pinned field was
rendered read-only and never seeded — leaving a required one permanently
unsubmittable. The schema object's identity says nothing (callers rebuild it
every render), which is why the key is derived from what has to be seeded.
- branchLabel stringifies every primitive constant. `hasDiscriminator`
accepts distinct booleans and nulls, so a `true`/`false` union was
discriminated perfectly well and still labelled "Option 1"/"Option 2".
Signed-off-by: cliffhall <cliff@futurescale.com>
- A `const` may be an object or an array, with or without a `type`, and
`String({...})` is "[object Object]" — which no CLI argument can equal, so
the only value such a schema accepts never matched. Supplied text is parsed
and compared structurally through the resolver's own comparison (exported as
`sameJsonValue` so the two cannot disagree), for both branch selection and
the substitution.
- The TUI's one-option const control is never marked required: its option may
legitimately be the empty string, which ink-form's required gate can never
accept, so submission would not even reach `decodeFormValues`. The value is
fixed by the schema, and `missingRequiredFields` still validates the call.
Signed-off-by: cliffhall <cliff@futurescale.com>
- coercionProperties collapses each declaration before comparing types. A
nullable schema written as an `anyOf` states no top-level `type`, so
`number | null` and `boolean | null` both read as "no type" and counted
as agreeing — and `value=true` then came back as `NaN` through the first
branch's number.
- admitsNull reads an `enum` offering null, under the same sibling conditions
a `const: null` answers to. Without it `{ enum: [null] }` reported that it
rejects null, and a required read-only field spelled that way stayed
permanently invalid.
Signed-off-by: cliffhall <cliff@futurescale.com>
- convertToolParameters defines its result keys rather than assigning them, so
a discriminator legally named `__proto__` reaches the call instead of the
prototype setter. It also collapses a nullable declaration before dispatching,
so `type: ["number","null"]` coerces as a number rather than being sent
as text.
- typeNameOf sorts an array `type`: JSON Schema reads it as a SET, so
`["number","null"]` and `["null","number"]` are one declaration
and must not read as branches disagreeing.
- nullableUnion's non-asserting keyword set covers `$defs`, `$id`,
`$schema`, `$anchor`, `$dynamicAnchor` and `$vocabulary` — they
identify or declare rather than assert. `$ref` stays out: it applies
whatever it points at.
Signed-off-by: cliffhall <cliff@futurescale.com>
`anyOfAdmitsNull` sent every non-object member to `toBranch` and refused it,
so `{ const: null, anyOf: [true] }` read as non-nullable — a required field
seeded and rendered read-only as `null` and reported missing forever. `true`
is the unconstrained schema and admits every value, `null` among them;
`false` admits none and still does not.
Signed-off-by: cliffhall <cliff@futurescale.com>
- anyOfAdmitsNull recognizes a branch that constrains nothing (`{}`, or
annotations only): it admits every value, null among them, so a field pinned
to `const: null` beside such a union stays submittable. `admitsNull`
cannot say that on its own — an unconstrained schema tells it nothing either
way and it declines rather than guesses — but here the question is narrower.
- convertToolParameters sends a matched `const` as the schema's own typed
value: `kind=2` selects a branch pinned to `const: 2` and was then sent
`"2"`, which that same branch rejects. Only an exact match is
substituted; anything else is the user's input and is left alone.
Signed-off-by: cliffhall <cliff@futurescale.com>
JSON Schema's boolean form is legal, but only `true` is harmless: it
constrains nothing, while `false` admits no value at all — so a field
declared with it can never be filled, and a required one makes the whole
branch unsatisfiable. Offering it contradicted every other faithfulness check
in the resolver, so such a branch is declined.
Signed-off-by: cliffhall <cliff@futurescale.com>
- selectBranchIndex asks the supplied NAMES first and falls back to a lone
agreeing constant only when they settle nothing. A matching `const` does
not identify a branch while another candidate leaves that property unpinned:
`{ kind: "email", phone: "555" }` agrees with an email branch whose
`address` is missing while satisfying a phone branch outright.
- isOfferable requires every property VALUE to be readable — JSON Schema's
boolean form is fine, a `null` or an array is not a schema at all and the
web form dereferences one on the way to choosing a widget. Such a branch is
declined rather than handed on to crash a tool panel that would otherwise
have rendered.
Signed-off-by: cliffhall <cliff@futurescale.com>
A distinct `const` does not make `oneOf` alternatives mutually exclusive
unless the property carrying it is required: two branches pinning an OPTIONAL
`kind` both match `{}`, so arguments omitting it satisfy more than one
alternative — exactly what `oneOf` forbids. The property must now be required
by the root or by every branch.
Also makes `propertiesOf` total (an unreadable `properties` reads as empty,
with a separate predicate for the one caller that must tell them apart), which
removes a row of unreachable `?? {}` fallbacks, and covers the value/type
comparisons the const checks rest on.
Signed-off-by: cliffhall <cliff@futurescale.com>
- matchesConstants checks own-property presence before reading a supplied
value, so an absent argument legally named `constructor` no longer reads the
inherited one and rules out every branch pinning that name.
- anyOfAdmitsNull asks `admitsNull` about each composition-free branch instead
of testing `type` alone, so a branch spelling its nullability as
`{ const: null }` or `{ nullable: true }` is recognized. Refusing composed
branches is what bounds the recursion.
- Drop a double cast from the malformed-applicator fixture.
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>
- The name-based narrowing is extracted as `narrowBySuppliedNames` and used
by the CLI coercion as well as the form: an undiscriminated union whose
branches type a shared name differently was losing the coercion for every
argument, including the ones that identify the branch unambiguously. The
type-agreement fallback now also polls only the const-compatible branches.
- TUI missingRequiredFields accepts `null` only where the property admits it,
matching the web gate: branch fields are rendered optional, so a
`default: null` on a non-nullable required field reached the check and the
call went out with a value the schema rejects.
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>
- rootUnion: the added-names guard is now a shared `addsForbiddenNames` and
applies to the `allOf` fold as well as to union branches. A member adding a
name a restrictive root `additionalProperties` rejects would otherwise be
merged beside the keyword, where the name reads as allowed.
- selectBranchIndex tests `Object.hasOwn` before reading a pinned value: a
field legally named `constructor` resolved to the inherited one and ruled
out the very branch that pins it.
Signed-off-by: cliffhall <cliff@futurescale.com>
- rootUnion: keywords the two declarations state SEPARATELY can contradict too.
A merged `const` is now checked against its own `type` and `enum`, so
root `x: { type: "string" }` under branch `x: { const: 1 }` declines
instead of seeding an immutable 1 into a string field. An integer const still
satisfies a `number` type — the one direction JSON Schema widens.
- TUI missingRequiredFields tests `Object.hasOwn` first: an argument legally
named `constructor` resolved to the inherited one and read as supplied, and
the call went out without it.
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: schema values are compared through a key-sorted rendering, so two
discriminator constants written as {a,b} and {b,a} are recognized as the same
value rather than as mutually exclusive alternatives.
- selectBranchIndex: after the const and required passes, a name only ONE
alternative declares identifies that alternative; a name several declare is
ambiguous and says nothing.
- additionalProperties counts as restrictive only when its schema states an
assertion — an annotation-only `{ title: ... }` is the equivalent of
`true`, like `{}`.
- Drop the unjustified double casts from the ToolTestModal fixtures; the
schemas are assignable to Partial<Tool> as written.
Signed-off-by: cliffhall <cliff@futurescale.com>
- rootUnion: a `oneOf` demands that EXACTLY one alternative match, which
flattening cannot preserve, so it is offered only when a discriminator makes
the alternatives mutually exclusive; `anyOf` makes no such claim and is
unaffected. An empty `additionalProperties` schema is the equivalent of
`true` and no longer declines a union.
- selectBranchIndex: falls back to the branch whose own required fields the
values supply, so an undiscriminated union's values open the picker on the
shape they describe rather than on the first branch.
- admitsNull: honors a sibling `nullable: true` beside `const: null`.
- SchemaForm: a value is carried only where the incoming branch leaves the
root's declaration as it found it; anything it declares itself is reset,
since it may type the name differently from where the value was typed.
`hasOwn`/`fromEntries` throughout, so `constructor` is not read as a
supplied value and `__proto__` is not dropped into the prototype setter.
- TUI: the same prototype-safe construction on the way out of the form, and
the chosen branch's `required` list is checked at submit — rendering a
branch's fields optional makes the FORM satisfiable, not the call, so the
missing names are reported rather than a known-invalid call being sent.
Signed-off-by: cliffhall <cliff@futurescale.com>
- rootUnion: decline a union that adds fields under a restrictive root
`additionalProperties`. The keyword constrains what its SIBLING
`properties` does not name, so the original schema admits none of the
branch fields; flattening moves them beside the keyword, where they read as
allowed and the form would submit what the schema forbids.
- `default` joins the annotation set: it constrains nothing, so two
declarations suggesting different initial values no longer conflict.
- Property records are read with `Object.hasOwn` and built with
`fromEntries`/`defineProperty`. `properties` is a JSON record, so
`constructor` is a legal argument name that `in` reported as a collision
the root never made, and `__proto__` is one that assignment would drop
into the legacy prototype setter instead of keeping as a field.
- SchemaForm: a value survives a branch switch unless BOTH branches declare
the name. A root argument the outgoing branch merely specialized is still a
root argument in a branch that inherits it, so dropping it erased a valid
value.
Signed-off-by: cliffhall <cliff@futurescale.com>
- rootUnion: union members get the same flattenability test as allOf members,
so a branch whose nested `allOf: [false]` makes it unsatisfiable is no
longer offered as a callable alternative with that constraint erased.
- selectBranchIndex: weigh only the pinned constants the caller SUPPLIED. One
it did not supply is a value this identification exists to seed, so
requiring it meant a deep link naming `kind` alone matched no branch
whenever the branches also pinned, say, a `version`. A supplied constant
that disagrees is still evidence against.
- admitsNull: `const: null` is conjunctive with its siblings, not an
override — `{ type: "string", const: null }` and
`{ const: null, anyOf: [...] }` reject every value, so the claim is made
only where the type and a sibling union leave null on the table.
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>
The previous commit's coverage gate failed on `core/json/rootUnion.ts`
branches (89.81%). A property declared with JSON Schema's boolean form has no
keywords to merge, so the two sides agree only by being the same declaration —
now tested both ways, and the dead `baseProperty !== undefined` guard is gone
(`conflicts` is only reached for a name the base declares).
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: a name both the root and a branch declare is now MERGED rather
than replaced, so a root `minimum` survives a branch `maximum`; a branch
that contradicts the root on a property's `type` describes an unsatisfiable
value and is declined outright. Branch offerability also requires that
`properties` really is an object (a malformed `properties: null` threw)
and that the member's own `type` admits objects.
- rootUnion: `ownFields` becomes `declaredFields` — a branch commonly
specializes a root property, and consumers must render the specialization.
- TUI: the generated select and branch prefixes step aside from any name the
schema itself declares (JSON has no reserved namespace), decode filters them
by exact name, a branch's specialization renders in its own section, and a
`const` is rendered as a one-option select whose typed value is restored
from the schema on submit.
- convertToolParameters: identify the branch from the discriminator constants
the call supplies; when none identifies one, coerce only names every
declaring branch types the same way rather than picking an arbitrary branch.
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>
Copilot review round 1.
`refreshStoredAuthToken` consumes the same fallback and made a single SDK
discovery call, so preserving the path there — without the walk the other two
consumers got — would have regressed exactly the case the walk exists to keep:
a path-hosted MCP server whose authorization metadata is published at the
domain root.
It injects its own discovery function as a test seam, so the walk is now also
exposed as `discoverAuthorizationServerMetadataFromCandidates(candidates,
discover)`, which takes that function and reports *which* candidate answered —
the CLI needs the winning URL, since it is the base the token request is made
against, not just the metadata.
Also drops the `as unknown as typeof fetch` from the new test in favour of
`vi.fn<typeof fetch>()`.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013hzwzS8UvBsr4yZm7o7sev
Signed-off-by: cliffhall <cliff@futurescale.com>
When RFC 9728 protected-resource metadata names no authorization server, the
MCP server URL stands in as one. `getAuthorizationServerUrl` built that with
`new URL("/", serverUrl)`, which discards the path — exactly the part discovery
needs when the server is not hosted at the domain root. The SDK's
`buildDiscoveryUrls` derives the path-scoped well-known locations from that
pathname, so an origin-only value could only ever probe the domain root, which
404s for a server like `https://example.com/mod/minilesson/mcp.php`.
Keep the path (and strip query/fragment, which an RFC 8414 issuer never
carries). A straight swap would regress the case the origin-only value did
serve — a server that merely lives under a path while publishing its metadata
at the root — so the URL becomes the first of two candidates:
`getAuthorizationServerUrlCandidates` yields the path-scoped form then the bare
origin, and `discoverAuthorizationServerMetadataForServer` walks them, treating
a candidate's failure as non-fatal while another remains and rethrowing the
first error only when all of them fail.
`discoverScopes` and the CIMD pre-registration probe both go through the walk.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013hzwzS8UvBsr4yZm7o7sev
Signed-off-by: cliffhall <cliff@futurescale.com>
parseSSE declared currentEvent/currentData inside the read loop, so an SSE
frame whose data: line arrived in one chunk and whose terminating blank
line arrived in the next was discarded. buffer already carried a partial
LINE across reads; nothing carried a partial FRAME.
Because this is the web client's transport, the dropped frame is a
JSON-RPC response that never settles: the request hangs forever with no
rejection, no console message, and nothing in the Network tab. The
payload does not arrive corrupted, it does not arrive at all, which is
why it presents as a hang rather than an error.
It has not bitten us because Chromium and Firefox happen to deliver whole
frames at the payload sizes this transport currently sees. That is luck,
not a guarantee: chunk boundaries depend on payload size, TCP
segmentation, and any intermediary. The exposure grows with payload size,
so a large resources/read is the most likely first victim.
The fix hoists both out of the loop and adds an end-of-stream flush — a
server that closes without a final blank line has still delivered a
complete frame, and dropping it would lose the last message of every such
stream.
parseSSE is now exported, solely so the test can reach it. That is the
only way to hit this deterministically: driving a real transport cannot
steer where a chunk boundary lands, but a test that supplies the reader
can put it exactly on the terminator.
The test keeps the two PASSING cases (whole frame, split mid-payload)
alongside the three that failed, because they are what make the diagnosis
precise. This is not "SSE parsing is broken" — mid-payload splits are
handled correctly today, and only the frame-terminator boundary loses
data. Verified against the unfixed code: 3 of 5 fail, all 5 pass after.
Signed-off-by: cliffhall <cliff@futurescale.com>
#2114 landed on v2/main, which is the PR this one follows up. Two
conflicts, one of them silent.
The marked conflict was cosmetic: keep v2/main's comment on the
`provider.scope` read, which now notes the getter filters (#2068).
The real one git merged cleanly and would have left dead: #2114's
if (this.oauthConfig.requestRefreshToken === false && requestedScope) {
this.pendingAuthorizationScope = requestedScope;
}
sat directly above this branch's unconditional
`recordAuthorizationRequestScope`, which overwrites the same field a few
lines later. Dropping that gate is what #2117 is for, so it is removed
rather than kept as a no-op.
The two changes compose: the SDK adds `offline_access` only when
`clientMetadata.grant_types` includes `refresh_token`, which the provider
drops while the opt-out is on -- so reading the authorize URL yields the
filtered scope there, exactly what #2114's gated branch persisted.
#2114's third case pinned the gate and inverts here, as its issue
predicted. It now asserts the request is persisted even with the grant
on; that records no new claim, since the request equals what storage
already held.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013hzwzS8UvBsr4yZm7o7sev
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot review round 8 (its one suppressed finding; no new inline comments).
The `boolean-schema` suggestion for a bare `true` said the replacement
"narrows the schema deliberately". That holds in an ordinary property
position and inverts under a negating parent: `{"not": true}` rejects
every value, so swapping in `{"type": "object", …}` leaves the parent
rejecting only objects and every non-object then passes — the enclosing
contract is widened. Under `if` it redirects which branch applies rather
than tightening anything.
- Parent-aware advice under `not`: suggest `{}`, which is the exact
object form of `true` and is already exempt from `untyped-schema` in
that position, with the message saying why a concrete type would be
wrong there.
- Everywhere else, and in the `untyped-schema` suggestion too, describe
the replacement as a deliberate CHANGE of contract rather than a
narrowing. A suggestion has no business claiming an effect it cannot
guarantee across the positions it can print from.
Tests pin the `not`-specific advice and assert no bare-`true` suggestion
in any of the three positions contains "narrow". The CLI README's sample
report is re-captured from a real run.
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>
Copilot review round 7.
- Add `contentSchema` to the traversal. It holds the schema the decoded
string content must satisfy, so a bare `true` or a remote `$ref` under
it is a real finding all three clients were walking past. Also admit
the three content keywords to `CONSTRAINING_KEYWORDS` on the same
documented grounds as `format` — annotation-only by vocabulary, but
plainly an attempt to constrain, so reporting such a schema as
accepting anything would be noise. The comment now names that
category rather than singling `format` out.
- Make `handleError` async and await the envelope write. Round 6 made
the `--strict` report await its write and stopped one line short: the
envelope emitted for the very same throw still raced `process.exit()`,
so the machine-readable line a CI caller parses could be lost on a
pipe. The launcher's comment claiming the envelope was "well inside
the pipe buffer" was wrong reasoning and is replaced — fitting in the
buffer says nothing about whether the write was performed. Its call
site now awaits; the bin's `.catch(handleError)` needed no change.
- The CLI README called its sample block the "complete output" when the
error envelope follows it on every non-zero exit. Now scoped to "the
complete report", with the envelope named and linked.
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>
Copilot review round 6.
- Share one traversal between `walk` and `declaresAnyId`. The `$id` guard
added last round descended through EVERY object value, including
instance data — `examples: [{"$id": "x"}]`, a `default`, a `const` —
where `$id` is a payload key declaring no embedded resource. Reading
one there suppressed `remote-ref` for the whole document, turning last
round's false-positive fix into a blanket false negative. Both passes
now go through `forEachSubschema`, driven by the same keyword tables,
so they cannot drift about what counts as a schema position.
- Await the stderr writes. `writeSchemaLintReport` used a bare
`process.stderr.write`, but both CLI exit paths call `process.exit()`
as soon as the work returns, which discards anything still buffered on
a piped stderr — truncating or losing the multi-block `--strict`
report on exactly the redirected-output runs a CI caller uses. Added
`awaitableError` beside the existing `awaitableLog` and awaited it.
The test's fake defers its callback so a synchronous one cannot make
the assertion vacuous; two existing stderr stubs that never invoked
their callbacks were fixed in the same pass.
- Show the CLI README's sample report in full. It printed one error
block immediately followed by the summary, a shape the formatter
cannot produce — every finding gets a block. Captured from a real run.
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>
Copilot review round 5.
- The sidebar tooltip applied `hasError` to the TOTAL, so a tool with one
error and three warnings announced as "4 schema portability errors" —
a defect the previous round introduced while adding that wording, and
worse than the vague text it replaced. Both the tooltip and the
accessible label now carry the breakdown, formatted by a new shared
`summarizeToolFindings` so the fourth place to phrase a severity count
cannot drift from the other three.
- Suppress `remote-ref` when the document declares an `$id` anywhere.
`$id` establishes a base URI, so with a root `$id` of
`https://example.com/root` the ref `https://example.com/root#/$defs/x`
resolves to THIS document and needs no fetch — the finding and its
"inline the referenced schema" remediation would both be wrong.
Classifying correctly needs full RFC 3986 resolution against nested
embedded resources, which this module does not do, so it declines
rather than guessing. A missed finding is the acceptable direction.
- Run the `--strict` validations before every short-circuit return.
`servers/list`, `servers/show`, `--list-stored-auth` and
`--print-handoff` all return from `parseArgs` before any connect, so
the checks further down let `--strict --method servers/list` succeed
while silently ignoring a tools/list-only flag.
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>
Copilot review round 4.
- Add `SCHEMA_UNPORTABLE` to `codeForExit`. `CliExitCodeError` lets a
caller omit the envelope, so without the case an envelope-less throw
reported the generic `error` — the same silent mislabelling the rename
was meant to prevent, one layer down. Covered via the envelope-less
construction that exposed it.
- Keep a local `type` (and the preset's `required`) alongside the remote
`$ref` in the showcase config. The TUI's `schemaToForm` dispatches on
`type`, so a `$ref`-only property fell through to a string field that
`add`'s numeric handler then rejected — making the README's "every
showcase tool is runnable" claim false there. The `remote-ref` finding
is unaffected, since that rule reads `$ref` independently.
- Restrict `type-union` to well-formed unions: non-empty, unique, and
all recognized JSON Schema type names. Its message asserts the
construct is *legal* JSON Schema, which is false for `[]`, `[3]` or
`["bananas"]`, and the `anyOf` it suggested for those would be invalid
too. Malformed schemas stay the SDK parser's business.
- Drop `if`/`then`/`else` from the presence-only allowlist. They only
constrain as a pair, so `{"if": {"const": 1}}` accepts every value and
was evading `untyped-schema`; the pairing is now checked explicitly.
- Distinguish the web sidebar severities by SHAPE, not just colour — a
circle for an error, a triangle for a warning — and name the severity
in the tooltip. Colour alone left a colour-blind reader unable to tell
them apart.
- Fix the stale "across 2 tools" count in the CLI README's hint sample.
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>
Copilot review round 2.
- Remove `non-object-root` entirely. It could never fire: the SDK types
`inputSchema` with `type: literal("object")`, so a tool with any other
root fails `ListToolsResultSchema` and `salvageListItems` drops it
before any client sees it. Verified against a live server advertising
`inputSchema: {type: "array"}` — `listAllTools()` returns []. A rule
that cannot fire is worse than none, because the docs then claim a
check the tool does not perform; the condition is already reported
through the malformed-items surface. Pinned by an integration test so
a change in that behavior is a signal rather than a silent gap.
- Fix the bare-`false` suggestion. `properties: {a: false}` forbids the
property; deleting the entry *permits* it with any value under the
default `additionalProperties`, so the advice inverted the schema it
replaced. Now recommends `{"not": {}}` and says why deletion differs.
- Exempt `not`-parented schemas from `untyped-schema`, so the `{"not":
{}}` above does not trip this module's own rule — under negation an
always-accepting subschema means always-reject, making "accepts any
value" the exact opposite of what it does. Scoped to that position,
not inherited by its subtree.
- Say plainly, in the `true` and untyped suggestions, that they narrow
the schema deliberately rather than implying an equivalent rewrite.
`true` has no portable object-form equivalent — `{}` is the very shape
this lint flags.
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>
- Scope `non-object-root` to `inputSchema`. The pinned SDK types
`inputSchema` with `type: literal("object")` but `outputSchema` as a
bare `looseObject`, so a `{"type":"string"}` output is conforming; the
rule was claiming a requirement MCP does not make, at error severity.
- Decide `untyped-schema` from an allowlist of constraining keywords
rather than a denylist of annotations. JSON Schema ignores keywords it
does not recognize, so `{"vendorHint": true}` accepts every value and a
denylist passed it as constrained. `$defs`-only schemas now flag too.
- Walk `dependentSchemas` and draft-07 `dependencies`, with a test
pinning that the property-name-array form is not a schema position.
- Suggest `anyOf` branches for an array-form `type`, never "drop it from
`required`" — accepting `null` and permitting absence are independent
contracts, so the old suggestion silently changed the schema's meaning.
- Reject `--strict` alongside `--app-info`. That path returns NDJSON from
`runMethod` and never reaches the lint, so the flag was a no-op gate
that could never fail.
- Colour the web sidebar flag from the `--inspector-*` severity tokens,
matching the detail panel and avoiding the default yellow that failed
the a11y check.
- Add a live integration test for `rawToolSchemas`. It caught that an
`outputSchema` override on a preset returning no structured content
makes every call to that tool fail client-side validation — the
showcase config did exactly that, so the flagged tool could not be
run. Moved the bare `true` onto `get_temp`, documented the caveat, and
pinned both halves.
- Drop the double cast from the lint test fixture.
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>
