Two suppressed review findings.
With an override configured, the wrapper cloned and parsed every
successful application/json response — every tools/call result and every
resource payload on the direct CLI/TUI transports — only to conclude it
was not a metadata document. It now tests the request URL for the RFC
8414 / OIDC well-known paths first, so ordinary traffic costs one string
test and is returned untouched.
The read path also trusted StoredMCPServer's `string` type for the two
override fields, which a hand-edited mcp.json does not honor: the CLI and
TUI read the file directly and then `.trim()` these, so a number would
crash the server load. Guarded with cleanEndpointOverride, mirroring
cleanAuthorizationParams.
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot round 2 (PR #2039) surfaced no new visible comments but six
suppressed ones. Four were actionable.
`clients/tui/src/App.tsx` — a stale-completion race in the new banner:
start disconnecting server A, switch to B, then let A's disconnect
reject, and A's failure appears under B. Rapid same-server retries had
the same shape, and the stale attempt is the one most likely to reject.
Claim a monotonic attempt token before awaiting and publish only if it
is still current and the server has not changed; `handleConnect` bumps
the same counter so a connect retires an in-flight disconnect. The
rejection is still owned when superseded — just not published. The
regression test drives a deferred rejection across a server switch, and
fails without the guard.
`tsconfig.lint.json` — the root type-aware block matches
`test-servers/src/**/*.tsx` but the parser project omitted that
extension, so such a file would have failed with "not found in any of
the provided project(s)" instead of being linted. Added, and AGENTS.md
now states the invariant: the include must stay a superset of the lint
globs.
`clients/web/server/{server,vite-hono-plugin}.ts` — the `open()`
rejection branch was untested, and duplicated. Both were fixed by
removing the duplication: `openBrowser` now lives beside
`printServerBanner` in `web-server-config.ts`, with one implementation
to test. Two copies of a one-line `.catch` is two chances for one to
drift back to a bare `open(url)`. The dev plugin is not booted in tests
— it returns early under VITEST by design — but it calls the same
tested helper.
`core/mcp/remote/node/server.ts` — `closeAbortedStream`'s rejecting-close
path, the entire reason the helper exists, had no test. Exported it (as
this file already does for `requestIdForSendWait` / `mcpParamHeadersOnly`)
and covered both legs, asserting no unhandledRejection fires.
Also corrects AGENTS.md, which said each client lists "both" tsconfig
projects — true for cli/tui/launcher, but web lists four leaf projects.
Signed-off-by: cliffhall <cliff@futurescale.com>
Two review findings on the fetch wrapper:
The content-type test was a substring match for "json", which also
matches `application/x-ndjson` — how the streamable-HTTP transport can
serve its long-lived server-push channel (`isLongLivedStreamResponse`).
Awaiting `.json()` on a clone of an unbounded stream never resolves, so
the MCP connection would hang for as long as an override was configured.
It now matches the media type exactly (`application/json` or a `+json`
suffix), with a regression test driven by a stream that never closes.
The overrides also leave `issuer` as discovery returned it, so pointing
one at an authorization server that advertises a different issuer is
rejected on callback by the RFC 9207 / SEP-2352 mix-up defense. That
check is deliberately not weakened; the limit is now stated on the
module, in the field description, and in the configuration guide.
Signed-off-by: cliffhall <cliff@futurescale.com>
Two review findings on the fetch wrapper:
Reading the body with `response.text()` meant every successful JSON
response was rebuilt while overrides were configured, even when it was
not a metadata document. That dropped native properties a synthesized
`Response` cannot carry (`url`, `redirected`, `type`) from responses the
wrapper has no business altering, and threw outright on a body-less
204/205, whose status Fetch forbids a body on — so an unrelated transport
response could fail. It now parses a clone and returns the caller's own
response unless a document is actually patched.
`new URL` also accepts embedded credentials while Fetch rejects a request
URL carrying them, so `https://user:pass@host/token` passed both the form
and the runtime and then failed deep in the OAuth exchange.
`oauthEndpointUrlError` now rejects a username or password.
Signed-off-by: cliffhall <cliff@futurescale.com>
The overrides are per-server configuration for the *resource* server's
authorization server, but the fetch wrapper that applies them is shared
with the enterprise-managed (EMA) flow, whose IdP OIDC discovery runs
through the same fetch and satisfies the metadata predicate. An
authorization override could therefore redirect the IdP login to the
resource AS, and a token override could send the IdP code or refresh
token there.
OAuthManager.getEndpointOverrides() now returns nothing when the server
is enterprise-managed — the same boundary redirectToExternalAuthorization
draws for the custom authorization parameters (#2018) — with a regression
test, the note surfaced on both form fields, and the rule documented.
Signed-off-by: cliffhall <cliff@futurescale.com>
A floated promise is invisible at review time — the call reads like an
awaited one minus four characters — and the unhandled rejection it
produces surfaces in a different test, in a different file, as a stack
pointing at SDK internals. In #1947 two un-held `callTool` calls made
`npm run ci` unpassable and took a full investigation to attribute; the
fix was two lines. `@typescript-eslint/no-floating-promises` catches the
class outright, so enable it at `error` everywhere.
The rule is type-aware and `tseslint.configs.recommended` is not, so each
scope's config gains a parser project. The four clients name their own
tsconfig projects (both of them — `src` and `__tests__`/`server` live in
different ones). The root scope has no tsconfig at all, so add
`tsconfig.lint.json`: a lint-only project covering `core/**`,
`test-servers/src/**`, and `vitest.shared.mts`, emitting nothing and
gating nothing. It sets `moduleResolution: bundler` deliberately — under
NodeNext `core/`'s extensionless imports fail to resolve, every import
degrades to `any`, and the rule silently stops seeing promises at all.
Measured surface, and the fixes:
- core (2) — `stream.close()` in two Hono `onAbort` listeners. Hono
invokes subscribers with a bare `subscriber()`, so a promise returned
from the listener is floated one layer up; `closeAbortedStream` owns
the rejection where it can actually be owned.
- tui (5) — `handleDisconnect` had no catch of its own and its only
caller is a key handler that cannot await, so a failing disconnect was
a live unhandled rejection; it now surfaces the message the way
`handleConnect` does. The rest are `void` at callers that genuinely
cannot await, over callees that already own their failures.
- web (92) — 83 are Storybook `expect(...)`, which is instrumented and
returns a promise, now awaited (`scrollAreaStoryAssertions` is async
for the same reason). Two tests were asserting against a provider call
they never awaited. `open(url)` in both web servers now warns instead
of escaping a synchronous callback.
- cli, launcher — already clean.
Type-aware linting costs time: `clients/web`'s `eslint .` goes from ~8s
to ~19s, inside `validate`. Narrowing the loaded projects is the lever if
that needs reducing later; dropping the rule is not.
Closes#1959
Signed-off-by: cliffhall <cliff@futurescale.com>
Both new #2006 regressions faked the events response with an object cast
`as unknown as Response`, which AGENTS.md rejects — it erases the check
that the mock actually satisfies the API the hook consumes.
Each now builds a real `ReadableStream` inside a real `Response`. `pull`
runs once per chunk the hook's reader consumes, so awaiting a promise
inside it gates delivery exactly the way the hand-rolled reader did,
while the mock stays type-checked against the Response contract. The
chunk counter replaces the read counter as the "the reader has consumed
the previous chunk" signal.
Both still fail against the pre-fix parser, verified.
Raised by Copilot on PR #2036.
Signed-off-by: cliffhall <cliff@futurescale.com>
Adds two optional per-server fields — `oauth.authorizationUrl` and
`oauth.tokenUrl` — that override the endpoints authorization-server
metadata discovery resolved, so a server can be pointed at a development
or staging authorization server without changing what it advertises.
SDK v2 routes both endpoints through the discovered metadata document and
neither through OAuthClientProvider, so the override is applied by
wrapping the client's base fetch and patching the metadata document in
flight (core/auth/endpointOverrides.ts). Wrapping the base fetch — rather
than only the auth fetch — also covers the discovery the SDK runs from
inside the transport on the 401/refresh path.
The fields round-trip through mcp.json, the /api/servers routes, the
runner (CLI/TUI), and the Server Settings → Authorization panel, where a
value that is not an absolute http(s) URL is flagged inline; at runtime
such a value is dropped with a single warning rather than failing the
connection.
Signed-off-by: cliffhall <cliff@futurescale.com>
The `/api/servers/events` reader in `useServers` searched for `\n\n` and
consumed exactly two characters as the separator. SSE permits CRLF line
endings, so a `event: change\r\n\r\n` frame stayed buffered forever:
`isSseDataFrame()` was never reached and no background refresh fired,
leaving the server list stale until a manual refresh or an LF-delimited
event arrived.
Frame boundaries are now located with a CRLF-aware matcher that reports
the separator's real length, and frame lines split on either terminator.
A bare CR is deliberately not accepted as a terminator: it cannot be
distinguished from the first half of a CRLF straddling a chunk boundary,
so a buffer ending in `…\r\n\r` would read as a completed frame and the
`\n` arriving next would open a bogus one.
Two regressions cover it, both verified failing against the old parser.
Each uses a referentially stable `fetchFn` and holds the frame behind a
promise released after the disk write — with an inline `fetchFn` the hook
re-subscribes every render and that re-subscription's own mount refresh
picks the edit up regardless, so the test would pass unfixed.
Closes#2006
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot review: the unit test covered the panel's rendering, but nothing
executed `mrtr_empty`, so the handler, its preset-registry branch, and the
showcase config entry could regress with CI green.
Adds a live modern-era test that loads `mrtr-showcase-http.json` through
`loadConfig`/`resolveConfig` — the same artifacts the README's manual
reproduction uses, so the config entry and the registry branch are covered
rather than just the fixture factory — answers the embedded elicitation, and
asserts the terminal result really is an empty `CallToolResult`: that is the
input the Tools panel must render as "completed, returned nothing".
Signed-off-by: cliffhall <cliff@futurescale.com>
The Tools screen mounts the Results panel only once a call has produced a
result, but the panel's zero-content branch rendered "No results yet" — the
same wording it would use before anything had run. An empty `content` array
with no `structuredContent` is a legal `CallToolResult`, so a call that had
plainly completed reported itself as a call that never happened.
That reads worst after a multi-round-trip (MRTR) sequence, which is how it was
reported: the user answers one or more embedded elicitations, watches the
Protocol tab group the exchange and mark it COMPLETE, and is then told there
are no results yet. Nothing was lost — the result was stored and rendered; the
panel simply had no wording for "completed, returned nothing".
The panel now renders an "Empty result" alert saying the call completed and
returned no content. This is the remaining half of the same gap #1908 closed
for `structuredContent`-only results.
test-servers: new `mrtr_empty` preset (`createMrtrEmptyResultTool`) — one
elicitation round, then a complete result with `content: []` and no
`structuredContent` — added to `mrtr-showcase-http.json` so the shape is
reproducible by hand. README documents it.
Signed-off-by: Cliff Hall <cliff@futurescale.com>
Signed-off-by: cliffhall <cliff@futurescale.com>
`ServerTasksCapability`'s `list`/`cancel` are objects, not booleans, so
the legacy-tasks fixture failed `tsc -b` in CI. A stale incremental
build cache hid it locally.
Signed-off-by: cliffhall <cliff@futurescale.com>
SEP-2663 moved task support from `capabilities.tasks` to the negotiated
`capabilities.extensions["io.modelcontextprotocol/tasks"]`, but the
Connection Info modal's capability grid tested only for the top-level
key. A modern server that advertised the extension in `server/discover`
therefore showed "Tasks ✗" while the very same extension id was listed
under "Server Extensions" two sections below.
Server capability rows now fall back to an extension id for the
capabilities the modern era expresses that way (tasks is the only one
today), gated on the negotiated era so a legacy connection is still
judged purely on its `initialize` capabilities. Keyed the same way
`InspectorClient.isTasksExtensionNegotiated()` gates the Tasks tab, so
the checkmark and the tab can't disagree.
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot review: the comments offered "a nullable number the user cleared" as a
way to get an explicit `null` into this field. Clearing the box reports
`undefined` — `toNumericValue("")` says so, and an existing test pins it — so
the only way `null` arrives is from parent state: a value received from the
server, restored from a deep link, or written by a caller for a nullable
schema. Say that in all three places instead.
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot review: a nullable number schema really does produce `null` for both
the raw value and the schema default, but the props declared only
`number | undefined` and the call site cast the null away. The runtime already
handled it — the initializer's `?? ""` exists for exactly that — so the types
were the part that was wrong, and a cast hiding it is how a future null bug
goes unseen by the compiler.
Widen both to `number | null | undefined` and drop the narrowing casts, keeping
the distinction the initializer encodes: `undefined` takes the default, `null`
is a value and renders as an empty box.
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot review: seeding with `value ?? defaultValue` also substitutes the
default when the raw value is an explicit `null` — a nullable number field the
user cleared, or one the server sent — where `resolveValue` previously kept the
null and the box rendered empty. A nullable schema with a non-null default
would have opened showing that default over a real value.
Test the `undefined` case explicitly, as `SchemaJsonField` already does, and
pin it: the new test fails with `'30'` where `''` belongs.
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot review: the code and test comments still said a defaulted array or
object argument *was* uneditable. It would be, but not through today's seeding
path — `collectSchemaDefaults` assigns `fieldSchema.default` itself, so the
substitution returns the same reference and the re-sync never fires. It needs a
structurally-equal but distinct value object, which parsed wire/deep-link
values and rebuilt nested-object defaults produce, and which these tests
construct. The number field has no such protection and did revert in the app.
State the boundary in all three places, and stop claiming the distinct-object
seed is how the Tools tab seeds the field, which it is not.
Signed-off-by: cliffhall <cliff@futurescale.com>
A field whose schema declares a `default` could not be edited. Unsendable
draft text reports `undefined` by design — an inspector must not report a value
it cannot send — and `resolveValue` turned that straight back into the default.
That is a real change from the previous value, so the draft/value re-sync
rewrote the box to the default and the keystroke was lost. Almost every edit
passes through an unsendable state, which left a defaulted array or object
argument uneditable in the JSON editor, and a defaulted number field unable to
be emptied.
Hand the two draft-holding fields the raw value and the schema default
separately, and compare only against the raw one. The default now seeds what
the field opens with, which is what a `default` means, rather than being
re-imposed while the user types. Every other field keeps resolving through
`resolveValue` unchanged.
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot review: seeding `{ b: null }` / `{ cfg: null }` meant the schema's
`default: null` was never resolved, so a regression that dropped null defaults
would have left both #1885 tests green while claiming to pin that path. Render
the harness unseeded instead, which is also how the reporter's form arrived.
Signed-off-by: cliffhall <cliff@futurescale.com>
Three issues report one defect: the schema form's JSON fallback re-escaping
the text being typed. #1853 saw it while typing an array, #1856 on the
Backspace that first makes a valid array invalid, and #1885 on a nullable
parameter whose default `null` had to be edited in place.
The mechanism — unparseable draft text stored back as the field's value, which
the next controlled render re-JSON.stringify'd — was removed by the draft/value
split in SchemaJsonField and the nullable-union collapse (#1928, #2007, #2020).
Those fixes were made for differently-framed issues, so nothing pinned the
flows these three reports actually describe.
Verified against the pre-fix commit: four of the five new tests fail there with
the reported corruption, including '"\"\"a"' for the caret-outside-the-quotes
case and the compounding backslash cascade for the typed array.
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot round 5 — three of four findings implemented.
- `cleanAuthorizationParams` validates the field as it comes off disk. Only the
web client's /api/servers route checked the shape; the CLI and TUI read
mcp.json directly, so `authorizationParams: "oops"` enumerated as the
character-indexed pairs 0=o, 1=o, … and `{ audience: 5 }` reached
URLSearchParams.set, which stringifies it — either way the Inspector sent
query parameters the user never wrote. Now validated in shared code so all
three clients enforce the documented string→string contract, warning on each
rejection. Follows `cleanRoots`, which guards the same class for `roots`.
- The form's description now states that these parameters are not sent while
Enterprise-managed authorization is on. The EMA leg authorizes against the
enterprise IdP — a different authorization server — and deliberately sends
none of them, so the copy was presenting a configured-but-inert value as
active.
- The /api/servers malformed-`oauth` warning listed an expected shape omitting
`onInsufficientScope`, which the validator accepts and the docs describe, so
it told users a valid config was malformed.
Ten new cases, all verified to fail against the pre-fix sources.
Declined: recording the authorize URL in the Network tab (repeat of round 2) —
that URL is reached by a top-level browser navigation rather than a tracked
fetch, so it has never appeared there for any authorization request; adding a
synthetic entry changes every OAuth flow and is a product decision for a
maintainer, not a change to make inside this PR.
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot round 4 (one suppressed finding, and a real bug).
`extractSecretsFromStored` rebuilt the stripped `oauth` object from an
allow-list of the non-secret fields. That list had gone stale: it never named
`onInsufficientScope`, so a server carrying both a client secret and a
non-default SEP-2350 policy silently lost the policy on save, falling back to
`reauthorize`. This predates #2018 — the PR only extended the same list with
`authorizationParams` — but the list is the defect, so fix the class rather
than add a third entry to it.
The stripped entry now subtracts the one secret instead of enumerating what to
keep, so any field added to `oauth` later is preserved by construction.
Regression test verified to fail against the allow-list version:
`expected { clientId: 'cid' } to deeply equal { clientId: 'cid', onInsufficientScope: 'throw' }`.
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot round 3 (both findings suppressed; both valid).
- The a11y fix in the previous commit added `align="flex-start"` to the row's
`Group`, giving it two static layout props and so tripping the .withProps()
rule in AGENTS.md — which applies to a single-use element too. Extracted as
`AuthorizationParamRow`. (The sibling `KeyValueRows` Group carries one static
prop and stays inline, correctly.)
- Added OAuthManager coverage for the config→provider bridge. The point is
sharp: the provider test proves the merge and the runner test proves the
options object, but nothing crossed the seam in `createOAuthProvider`, so
deleting the `authorizationParams:` line left the feature dead with every
test green. Verified by doing exactly that — both new cases now fail with
`expected null to be 'corp-idp'` — then restoring it. The second case also
pins the reserved-key drop through the manager path.
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot round 2 (suppressed comments). Five of the seven findings were the
accessibility of the new row list and one stray cast; all are addressed here.
- The reserved-key reason is now the key input's `error` **string** rather than
a bare boolean plus free-standing text. Mantine renders it in the input's own
error slot and wires the `aria-describedby` association, so a screen reader
learns *why* the field is invalid; a boolean only set `aria-invalid`. This
also deletes the duplicate `ReservedParamError` text and its Stack wrapper.
- Each control carries a per-row `aria-label` — the section heading and the
placeholders are not programmatically associated with the inputs, so the key
box, the value box, and a remove button announcing only "X" were
indistinguishable to assistive technology. The label names the row by its key,
falling back to "row N" while the key is still blank.
- Dropped an unnecessary `as unknown as Record<string, unknown>` in the
servers-route integration test: `MCPConfig.mcpServers` is already typed as
`Record<string, StoredMCPServer>`, so the entry's `oauth` reads directly.
Three tests cover the new behavior (accessible names, the blank-key row-number
fallback, and the aria-describedby association), each verified to fail against
the pre-fix component. No change to the reserved-key rule or the EMA boundary.
Signed-off-by: cliffhall <cliff@futurescale.com>
A field that cannot turn its text into a value reports `undefined` — which is
exactly what an empty field reports. That made the two indistinguishable to
every caller of `SchemaForm`, so invalid text in an optional field was
submittable and the argument simply arrived absent (#2020).
`SchemaForm` now tracks per-field draft validity and reports it through
`onValidityChange`, covering both fields that hold text the form cannot send:
the JSON editor (unparseable text) and the number input (a magnitude JS cannot
hold exactly, which is dropped rather than silently rounded). The number field
also states it on itself, as the JSON editor already did. A nested object form
reports through the same channel, and both the field-level and form-level
effects report "valid" on cleanup so a draft cannot outlive the field that held
it — a remount under a new `resetKey`, or an unmounted panel.
`ToolDetailPanel` gated Execute on `isExecuting` alone, so it also had no
required-argument check at all — unlike the App and elicitation panels. It now
uses `hasMissingRequiredFields` like they do. `InlineElicitationRequest` had
neither gate and gets both.
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot round 1: the new EMA regression test constructed its inner provider's
storage with `as unknown as OAuthStorage`, an unjustified double cast — the
rule in AGENTS.md is that such a cast must carry a justification or be replaced.
Replaced with the genuine article: `OAuthStorageBase` over an
`OAuthMemoryStore` and a no-op persist backend, so the stub type-checks against
the real interface the provider is handed and a future `OAuthStorage` contract
change cannot pass silently. Reusing `emaConfig.storage`, as suggested, would
not have addressed the objection — that value is itself a cast, and typed as
`EmaFlowConfig["storage"]` rather than `OAuthStorage`.
The test's assertion is unchanged and was re-verified to fail against pre-fix
code (`expected 'corp-idp' to be null`).
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot review round 1. With an ID Token row alongside the Access Token
row, both can render a "Decode JWT" toggle and a "Copy" control, and the
four buttons shared two accessible names — button-by-button screen-reader
navigation could not tell which token a control acted on. That collision
is introduced by this PR: before it there was only ever one token row.
- `CopyButton` takes an optional `label` naming what is being copied,
used for the accessible name only ("Copy ID Token"). The visible
tooltip stays "Copy", and the name keeps "Copy"/"Copied" as its first
word so it still contains the visible text (WCAG 2.5.3 Label in Name).
- `OAuthTokenField` qualifies its decode toggle the same way ("Decode JWT
for Access Token") and passes its row label down to `CopyButton`.
The visible UI is unchanged — this is accessible-name only.
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot round 10.
The scan no longer publishes its malformed entries into the tools/list report
at all. Round 9 only stopped it overwriting the aggregate's; the remaining
case is unsalvageable for a stronger reason than 'two different requests':
- a FAILED strict aggregate runs salvage, which writes the report itself;
- a SUCCESSFUL one means every entry parsed, so the rendered list contains no
malformed entries.
So by the time the scan finds something, either the aggregate already reported
it with indices matching the screen, or the aggregate succeeded and what the
scan found is provably not in the list being displayed. Publishing could only
ever name entries the user cannot see.
Nothing is lost: listToolsForScan still marks its own refused response, so the
finding surfaces on the Protocol entry for the exchange that carried it, where
it is true. The per-page accumulation and raw-entry offset existed only to feed
the removed publish, and go with it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RDXCSrnSBV7vZfxycG5Z6E
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot round 11. Two of the three are the enum-strip logic this PR added
misbehaving on degenerate input; the third is one more keyword for the
fail-closed rule.
1. An empty `enum` permits nothing at all, but the strip reported "unchanged"
(nothing was removed) and the collapse built a Select with no options — a
required field then had no interaction able to supply a value. It now
reaches the same conclusion as `[null]`: no selectable member, so decline.
The outcome is renamed `unselectable`, since "only-null" no longer describes
both routes to it.
2. Filtering `null` out of `enum` shortens it, which could make an ALREADY
mismatched `enumNames` come out the same length — so `enum: [null, "b"]`
with `enumNames: ["None"]` started labelling `b` as "None", turning labels
both renderers had correctly ignored into a confident mislabel. A list that
was not parallel before filtering is now dropped rather than kept.
3. `$ref` joins the opaque set. It is never resolved here and applies alongside
its siblings, so `{ type: "string", $ref: "#/$defs/intOnly" }` can be
unsatisfiable. The wrapper path already declined on it by virtue of the
annotation allowlist; this extends the same rule to branches, the
`type: [T, "null"]` path, and `admitsNull`.
No round 12 was requested: the review loop was stopped at the user's direction.
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot round 9.
The exclusion scan is a separate, uncached tools/list exchange, so its entries
need not be the ones rendered — the server can answer differently, and a
cache-served aggregate is a different list version. It was overwriting the
aggregate's report wholesale, replacing accurate indices with indices into a
list nobody is looking at. The aggregate now owns the report; the scan
publishes only when the aggregate left nothing, which is the cache-served case
that made it publish at all (round 2).
A conforming re-fetch no longer swallows the mark: the list can change between
the strict page and the lenient one, and a clean second response says nothing
about the first, which was refused. It is marked with the original decode
error when there is nothing per-item to name.
The scan's fallback re-fetch now goes through invokeMcpClient, like the strict
call beside it and like salvageList's walk, so an auth challenge on that
request can still be recovered instead of being swallowed by listAllTools's
best-effort scan catch.
Also fixes listTools dropping an empty-string cursor (truthiness rather than
!== undefined) — its own fallback leg already preserved it, so the two legs of
one scan page disagreed about which page they had fetched.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RDXCSrnSBV7vZfxycG5Z6E
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot round 10: one finding, the last position of the rule the previous two
rounds established. A *non-null* branch can carry its own `anyOf`, and the
hoist drops it — the branch's `anyOf` overwrites the wrapper's in the spread
and is then cleared, so `{ anyOf: [{ type: "string", anyOf: [{ const: "a" }] },
{ type: "null" }] }` widened into an unconstrained nullable string.
Rather than add a fourth inline condition, the four sites that ask "does this
schema compose something I cannot evaluate?" now share one predicate,
`hasUnevaluatedComposition` (an opaque applicator, or a nested `anyOf`):
- the surviving branch in the `anyOf` collapse (this round's finding),
- the null branch in `admitsNull`,
- the `type: [T, "null"]` collapse path,
- and, via `hasOpaqueApplicator`, the top of `admitsNull`.
Each of those was missed independently across rounds 8-10 for the same reason:
the guard existed but was spelled out inline at each site and drifted. Sharing
the predicate is the part that stops a fifth site from repeating it.
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot round 8.
The era contract validated with the wire schema but returned the raw input,
so the wire schema's normalization was discarded — both era schemas are
z.object, which strips unknown top-level fields as the strict codec does, so
salvaged tools could carry fields the strict path drops. The parsed output is
now what gets normalized and returned. This also simplifies the bridge: issues
come straight from the wire parse with their paths intact, so the custom-issue
forwarding (and its early describeIssue application) is gone.
The malformed-entry report is written by the aggregate walk's salvage, but in
paginated mode the tools/prompts/resources panels render the paged stores,
which neither write nor clear it — leaving a stale warning above a freshly
fetched page it does not describe, with indices into an aggregate the user is
no longer looking at. Those reports are now suppressed in paginated mode;
resource templates keep theirs, having no paged store. Suppressing rather than
adding per-page salvage: pointing at the wrong entry costs more than staying
quiet, and per-page salvage is a feature, not a display fix.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RDXCSrnSBV7vZfxycG5Z6E
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot round 9: four findings, all of them places where round 8's fail-closed
inversion was the right idea applied incompletely. The rule was only enforced
at the positions I happened to be editing; it now holds everywhere a schema is
inspected.
1. `admitsNull` returned early on an explicit `type`, ignoring a sibling
`anyOf` — but the two are conjunctive, so `{ type: ["string", "null"],
anyOf: [{ type: "string" }] }` admits no null at all. A sibling union now
withholds the claim.
2. A null-typed branch could be unsatisfiable via its own nested `anyOf`
(`{ type: "null", anyOf: [{ type: "string" }] }`). The branch check tested
`not`/`allOf`/`oneOf` but not `anyOf`; it now treats a nested union as
equally opaque.
3. The `type: [T, "null"]` collapse path had no applicator guard at all —
`collapsed` clears `anyOf` unconditionally and the renderers ignore the
rest, so `{ type: ["string", "null"], anyOf: [{ const: "a" }] }` was widened
into an unconstrained string field. That path now declines the same way the
`anyOf` path does.
4. The `admitsNull` docstring still advertised `oneOf` support that round 8
deliberately removed. Rewritten to state what is recognized, what is
declined and why, and the asymmetry that settles the direction.
The three code fixes share one helper (`hasOpaqueApplicator`) rather than
repeating the condition, so the next position that needs it cannot drift.
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot round 7.
The era contract was derived from the neutral ToolSchema plus one override,
which diverged from the real wire schemas both ways. Measured against SDK
2.0.0 on a real connection:
- modern accepts `inputSchema.properties: 5` and `required: [1]`; ours
rejected them, so a malformed sibling made salvage DROP a conforming tool
and report it as malformed — a false accusation from the screen whose job
is to say which entry is wrong.
- legacy rejects `outputSchema.properties: 5` / `required: [1]`; ours
accepted them, readmitting entries the strict path refused.
- modern rejects a non-string `inputSchema.$schema`, legacy accepts it;
ours accepted on both.
Each era is now expressed as its own wire schema, bridged to Tool by
`eraToolItemSchema`. The bridge exists because both eras accept values the
exported Tool type cannot describe (the SDK returns them typed as Tool
anyway), so the wire schema's output type is irrelevant and z.custom<Tool>
supplies the caller's type — which is what avoids an `as unknown as` here.
Forwarded issues keep their path so the report still names the field.
The full measured matrix is pinned table-driven in listSalvage-era.test.ts:
each row asserts the SDK's verdict AND that our contract agrees, so a
divergence that closes or widens fails loudly.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RDXCSrnSBV7vZfxycG5Z6E
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot round 8 opened the same conjunctive-semantics seam a third time, so
this stops patching cases and inverts the defaults instead. Both guards now
fail CLOSED: anything this module cannot evaluate sends the field to the JSON
editor rather than to a widget built from a partial reading.
1. The wrapper-conflict check enumerated the *validation* keywords and declined
when two of them disagreed — which meant any keyword nobody listed
(`oneOf`, `allOf`, `not`, `$ref`, a future vocabulary) was silently treated
as safe to drop. That whitelist fails open, and open means rendering a
widget for a constraint that is not the schema's. It is replaced by an
allowlist of *annotation* keywords: the collapse declines whenever the
wrapper carries anything beyond annotations and the union itself. Stricter
than the old equality test — a wrapper restating its branch's `type` now
declines too — which costs a dropdown on a rare redundant shape and buys a
rule with no gap in it.
2. `admitsNull` returned true for shapes it had not actually evaluated:
`not: { type: "null" }` rules null out, `allOf` can add a member that does,
`oneOf` requires *exactly one* branch to match (so a null branch does not
by itself mean null validates), and a branch like `{ type: "null",
const: "x" }` is unsatisfiable. All now answer false. Under-claiming
nullability costs a clear button; over-claiming lets the form emit a value
the schema rejects, so the asymmetry decides it.
Note the third finding — draft validity never reaching the submit controls —
is NOT fixed here, and it is worse than I described last round: `ToolDetailPanel`
gates Execute on `isExecuting` alone and never calls `hasMissingRequiredFields`,
so my earlier claim that required fields were already covered was wrong for the
Tools tab. Corrected on #2020, which is where the fix belongs.
Signed-off-by: cliffhall <cliff@futurescale.com>
