Copilot review (#2287), three findings:
1. **invalid-client recovery lost the provenance.** SDK v2 `auth()` answers
`invalid_client` / `unauthorized_client` with `invalidateCredentials("client")`
and an immediate retry; our clear drops the registration *and* its kind, so
the retry's URL-based client-ID save landed with nothing recorded and was
stored as `dcr`. Provenance now lives in `cimdClientMetadataUrl`, an
issuer-keyed marker on the issuer slot that records a property of the AS
rather than a credential — `clearClientInformation` deliberately leaves it
alone. `ensureCimdClientRegistration` writes it, and withdraws it when the AS
stops advertising CIMD, on every connect.
2. **Discovery became a hard network dependency.** Moving the existing-client
check after discovery meant a well-known outage could fail a reconnect the SDK
would have served from its persisted discovery state. Reuse
`provider.discoveryState()` first, and treat a discovery failure as "skip
pre-registration" rather than an error — this helper is an optimization over
what `auth()` does for itself.
3. **Dropped an unjustified double cast** in the test fetch helper; the async
signature is directly assignable to `typeof fetch`.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Lmj6X2epWpPE7ScdSKQmtA
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot review (#2287): the active-issuer fallback added last round could copy
issuer A's CIMD provenance onto a real dynamic registration for issuer B, since
RFC 7591 §3.2 permits B to mint the same opaque URL as its `client_id`.
Fix it at the source rather than in the resolver. `ensureCimdClientRegistration`
now runs discovery first and records the registration against the issuer it just
discovered, having confirmed *that* AS advertises
`client_id_metadata_document_supported`. Its "already registered?" check moves
after discovery and is keyed by that issuer — read ctx-less it resolved through
the active issuer and early-returned for every later one, which is what forced
the cross-issuer fallback in the first place.
`resolveSdkRegistrationKind` is therefore issuer-scoped again, with no fallback.
A second AS behind one resource now gets its own determination: CIMD when it
advertises CIMD, `dcr` otherwise — including when it mints the metadata URL as
its own `client_id`.
The cost is a discovery round trip per connect attempt rather than only the
first; noted at the call site.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Lmj6X2epWpPE7ScdSKQmtA
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot review (#2287): scoping the stored-CIMD check to the incoming issuer
alone mislabels the SDK's own CIMD write for a second authorization server.
SEP-2352 keys registrations per AS, so the first binding promotes the unkeyed
CIMD entry into issuer A's slot and clears the fallback;
`ensureCimdClientRegistration` then early-returns on its ctx-less read, and the
save under issuer B finds nothing recorded for B.
Check the issuer slot and then the server's active registration, so a second
issuer stays CIMD while a `client_id` the AS minted itself still falls through
to `dcr`. Covered by two tests driving a real `OAuthStorageBase` through the
A → B sequence, since the behaviour under test is how storage promotes and
clears slots rather than anything a mock would express.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Lmj6X2epWpPE7ScdSKQmtA
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot review (#2287): RFC 7591 §3.2 leaves a dynamically issued `client_id`
opaque, so `client_id === clientMetadataUrl` is not on its own proof that CIMD
ran — an authorization server could in principle mint that value from
`POST /register`.
Narrow the claim to a conjunction, whose decisive term is a registration we
recorded as CIMD ourselves rather than an inference about what the AS returned:
CIMD must be configured for this connection, the incoming `client_id` must be
exactly that metadata-document URL, and the registration already stored under
that id must be recorded as `cimd`. Everything else falls through to `dcr`.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Lmj6X2epWpPE7ScdSKQmtA
Signed-off-by: cliffhall <cliff@futurescale.com>
`BaseOAuthClientProvider.saveClientInformation` treated every save without an
explicit `registrationKind` as DCR. The SDK reaches it from three places in
`auth()` and only one is a dynamic registration, so the issuer-binding write
overwrote the `cimd` provenance our own pre-registration had just stored — and
Connection Info reported `Dynamic (DCR)` for a connection that never issued a
`POST /oauth/register`.
Recover the kind instead: a `client_id` equal to the configured
`clientMetadataUrl` is CIMD (which also covers the SDK's own CIMD write), and
otherwise a stored registration with the same `client_id` carries its recorded
kind forward. Matching on `client_id` keeps stale CIMD provenance from leaking
onto a later DCR registration for the same server.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Lmj6X2epWpPE7ScdSKQmtA
Signed-off-by: cliffhall <cliff@futurescale.com>
The Tools tab rendered every schema-portability finding, always open, above
the argument form. On a server whose schemas are broadly unportable that put
the same wall of text ahead of every tool's first input — and the findings
address the server author, not the caller trying to fill the form.
The section now opens collapsed behind the same disclosure heading the Skills
pane's Conformance section uses, with an `N error(s), M warning(s)` badge. The
expand choice is global rather than per tool (`useSchemaFindingsExpanded`,
stored as `inspector.schemaFindings.expanded`): this panel is reused across
selections, so a per-tool disclosure would re-collapse on every click and
reproduce the same scrolling. The badge stays visible while collapsed and the
tool-list severity icon is untouched, so nothing about a tool's standing is
hidden by the closed state.
Two neighbouring fixes on the same panel:
- `JsonEditor` frames Ace in a bordered `Paper`. Ace paints its background edge
to edge with nothing around it, so "Edit as JSON" replaced a column of
bordered inputs with one borderless slab.
- `ToolDetailPanel`'s body `ScrollArea` overrode the app-wide `type="scroll"`
default with `type="auto"`, parking a permanent bar down the side of any tool
whose form is taller than the panel. `offsetScrollbars` stays, so the form
does not shift sideways when the bar fades in.
`test-servers/configs/unportable-schemas-many-http.json` reproduces the
original complaint: the same four presets as `unportable-schemas-http.json`
carrying 26 findings rather than 5.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DU3QYnc95g6EM4vo3hYNet
Signed-off-by: cliffhall <cliff@futurescale.com>
Adds a third entry to `DEFAULT_SEED_CONFIG`, `example-server-default`, a
Streamable HTTP card pointing at the MCP org's own feature-reference server
(https://example-server.modelcontextprotocol.io/mcp). Until now every seeded
server was a local stdio process and every remote example in the docs was a
placeholder, so trying a remote connection meant finding or standing up a
server first.
The endpoint is protected by OAuth, but its authorization server advertises
Dynamic Client Registration and fronts a *mock* upstream IdP, so the whole
round trip completes with no account, no API key and no pre-registered client
— which is what makes it viable as a shipped default. Verified end to end
against a prod build: the card connects, negotiates MCP 2025-11-25 and lists
the reference server's tools.
The seed carries no `protocolEra`. That is not a default-by-omission: the
server answers the plain `initialize` handshake and rejects `server/discover`,
so it has no modern (SEP-2663) era to negotiate — `"auto"` would only buy a
failed probe and `"modern"` would fail the connection outright.
Docs updated alongside: the seeded-catalog block and per-client table in
`docs/mcp-server-configuration.md`, the Streamable HTTP example there (which
now leads with the live endpoint and keeps a placeholder to illustrate
`headers`), and the seed description in `clients/launcher/README.md`.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012x3rC3JXU17aW3uSwjPoi5
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot on #2281: the option's contract said an unregistered URI is ignored,
but `withDuplicateResources` runs on the assembled list, which also carries
whatever a resource template's `list` callback contributed — so a
template-listed URI was duplicated in contradiction of its own doc.
Broadened rather than restricted, because scoping it to `registeredResources`
would leave half the surface unreproducible: both kinds of entry land in the
same Resources sidebar list and collide on the same React key. The contract now
says so on `ServerConfig.duplicateResourceUris` and on the config-file mirror,
and an integration case covers a template-listed URI — it fails if the override
is narrowed to `state.registeredResources`.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018yVgAJz4wpkS4mkGk7DQnp
Signed-off-by: cliffhall <cliff@futurescale.com>
Ink writes styling as escape sequences inside the styled run, so an
accelerator underline splits the word it decorates: `<Text underline>I</Text>nfo`
reaches the frame buffer with escapes between `I` and `nfo`, and a plain
`toContain("Info")` fails against a component that is rendering correctly.
Chalk only emits color when it detects a TTY, so CI stays green and the six
failures land only on a developer whose shell exports FORCE_COLOR.
Add `__tests__/helpers/renderTui.tsx` — ink-testing-library's `render` with
`lastFrame()` and `frames` ANSI-stripped — and point all 16 renderer test
files at it. `frames` is a getter reading through to the live array, since
Ink appends to it as the component re-renders; the raw bytes stay reachable
as `stdout.lastFrame()`.
Pinning FORCE_COLOR=0 would turn the six green while hiding the class: any
future assertion on styled text has the same problem, and the suite would
stop covering the styled path at all.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01MFp2LVH7UJST5i8bDBJghw
Signed-off-by: cliffhall <cliff@futurescale.com>
The Resources sidebar keyed every row on its `uri` (and templates on
`uriTemplate`), but nothing in MCP makes those unique — a server may return
the same URI twice, and `ManagedListState.applyItems` replaces the list
wholesale, so a repeat is what the server actually sent. React logged
`Encountered two children with the same key` on every render, and a
filtered-out row survived reconciliation instead of unmounting.
This is the resources counterpart of #1957. The row identity is now the same
`listRowKey(id, sourceIndex)` the Tools sidebar has used since then, extracted
so the two formats cannot drift; `toolRowKey` delegates to it. Applied to all
three Resources lists and to `RootsTable`, whose user-maintained list can
collect a duplicate URI the same way.
Selection stays keyed on the URI, deliberately: unlike a duplicated tool name
(#2001), duplicate rows here denote the same resource — a `resources/read`
takes the URI — so highlighting both is correct. Only the React key needs to
tell them apart.
Adds `ServerConfig.duplicateResourceUris` and the
`duplicate-resource-uris-http.json` showcase config, the `resources/list`
mirror of `duplicateToolNames`, so the defect has a fixture to reproduce it —
appended rather than adjacent, for the reason #1957 documented.
Closes#2206
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018yVgAJz4wpkS4mkGk7DQnp
Signed-off-by: cliffhall <cliff@futurescale.com>
`declaresAnyFields` decides whether a tool is auto-invoked with `{}` or asked
about, and it counts a required name a schema never declares — so the #2224
check making such a union render nothing must not be read as "no arguments".
Nothing changed here; the invariant just had no test of its own.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PxYT4eKdnx1yRw7x4bW7Ts
Signed-off-by: cliffhall <cliff@futurescale.com>
`ManagedRequestorTasksState.refresh()` drives the legacy `tasks/list` walk
itself, and both of its cursor tests were truthiness checks — so the fixed
adapter was never reached. `while (cursor)` ended the walk on a `""`
`nextCursor`, leaving the user-visible task list on page one, and
`cursor ? [...this.tasks, ...page] : page` would have replaced the
accumulated list rather than extended it had the walk continued.
Both now test `=== undefined`. The other four paged/managed stores already
did (`pagedToolsState`, `pagedPromptsState`, `pagedResourcesState`,
`pagedResourceTemplatesState`, `pagedRequestorTasksState`,
`managedSkillsState`); this was the last walk on the truthiness side.
Covered by a new case in `managedRequestorTasksState.test.ts` that paginates
`"" → "c2" → end` and asserts the cursor each call received, so a walk that
terminated correctly but re-requested page one still fails. Reverting either
guard on its own fails exactly that case.
Reported by Copilot on #2277.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EN2UfrkQZvtrviMtqURRm4
Signed-off-by: cliffhall <cliff@futurescale.com>
An MCP cursor is an opaque string: the spec constrains neither its content
nor its length, so `""` is a `nextCursor` a server may legitimately return
and the client is required to send back verbatim.
`listPrompts`, `listResources`, `listResourceTemplates` and
`listRequestorTasks` built their request params with `cursor ? { cursor } : {}`,
a truthiness check that cannot distinguish "no cursor" from "the cursor is
the empty string". Against a server that paginates with `""` they dropped it
and re-requested page one — a list that silently stops at the first page, or
a walk that loops on it forever. Nothing surfaced an error, because the
request was well-formed; it just asked the wrong question. `listTools` (and
the later `listSkills`) already used `cursor !== undefined`; this brings the
other four into line.
`getReplayableParams` in `protocolReplay.ts` existed partly to mirror the
asymmetry — its `method === "tools/list" || params.cursor !== ""` special
case collapses to `typeof params.cursor === "string"` now that the adapters
agree, and its long comment is updated rather than left describing a state
that no longer exists.
Coverage:
- `inspectorClient-list-cursor.test.ts` asserts the outbound params of all
five adapters — `""` carried, an absent cursor sending no `cursor` key,
a non-empty cursor forwarded unchanged. `listTools` is the control.
- `test-servers/configs/empty-cursor-http.json` (`emptyStringCursor`) hands
out `""` as the cursor for page two, the shape no other fixture produces,
and `empty-cursor.test.ts` walks all three of its lists end to end.
- Removing the fix from any one adapter fails exactly its two cases.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EN2UfrkQZvtrviMtqURRm4
Signed-off-by: cliffhall <cliff@futurescale.com>
Adds `docs/cli-smoke-testing.md` — a copyable connect → list → call →
assert workflow for testing an arbitrary MCP server from a shell or a CI
job, plus index rows in the root README and a pointer from the CLI
README.
The gap this fills is a user-facing one. `clients/cli/README.md` is a
flag reference and `scripts/smoke-cli.mjs` is an internal end-to-end
check of the launcher → CLI path; neither tells someone how to compose
those flags into a check they run on every commit against their own
server.
Covers, in order: connecting over stdio/HTTP/SSE (including the `--`
separator, which splits the opposite way under `--cli`); `--format json`
and its `{result[, appInfo]}` envelope; asserting a tool exists with
`jq -e`; one representative `tools/call` and the `--tool-arg` vs
`--tool-args-json` coercion difference; the exit-code map and how to
branch on it; keeping OAuth non-interactive with `--stored-auth-only`
and an isolated `MCP_STORAGE_DIR`; and negative assertions — refusals,
a secret-shape scan of captured output, and `--strict` schema
portability. Ends with a complete script and a GitHub Actions job.
Every command and output shape in the guide was verified against the
bundled composable test servers over both stdio and HTTP rather than
transcribed from the reference, which corrected three claims that would
otherwise have shipped wrong:
- `--tool-arg zip=012` does not send `12`. `012` is not valid JSON, so
it is sent as the literal string. The real trap is `zip=10001`, which
does coerce to a number and is then rejected by a string schema.
- `if ! cmd; then status=$?` yields `0`, because `!` inverts the
pipeline's status — every failure class would look identical. The
guide uses `|| status=$?` and says why.
- The sample script does not report a failure class on its own; that is
the opt-in `case` block from §5.
The §8 script was run end to end against a live HTTP test server, and
exits 0/3/4/5/6 were each reproduced against a real server.
Closes#1886
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KsKC3HniVF9dCZtDVbYp5B
Signed-off-by: cliffhall <cliff@futurescale.com>
`isOfferable` in `core/json/rootUnion.ts` checked that a merged branch carries
at least one property and that every property value is readable, but never that
each name in `required` resolves to a property a form can render. A branch like
`{ properties: { kind }, required: ["kind", "payload"] }` was therefore offered
in the picker, rendered no control for `payload`, and had the submit-time check
report it missing permanently — the exact dead end the function's own comment
already reasons through for a `false`-schema required field, one case short.
Judge the merge, so a branch requiring only names the ROOT declares —
`anyOf: [{ required: ["email"] }, …]` — stays offerable as before. `hasOwn`
rather than `in`, since an argument legally named `constructor` would otherwise
resolve through the prototype and read as declared.
Declining every branch of a union leaves the web form with neither a picker nor
a field, which is a different dead end. `SchemaForm` now seeds the raw-JSON
editor (#2151) in that one case — a root union that resolved to no branches and
no properties — so the arguments stay expressible. Seeded rather than forced, so
the switch still works, and re-seeded during render (never in an effect) because
the form is reused across tools rather than remounted. A tool that genuinely
takes no arguments is untouched. The TUI has no such editor; there the union is
simply not offered, which is already better than a section that can never be
submitted.
Adds a `record_shipment_by` preset and wires it into the
`root-union-schemas-http.json` showcase so the shape is reproducible by hand,
with docs/test-servers.md covering it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PxYT4eKdnx1yRw7x4bW7Ts
Signed-off-by: cliffhall <cliff@futurescale.com>
`autoReadKey` cached the manifest key of the entry it had read for. When the
selected entry left `skills` — a refresh in flight, a disconnect — the render
emptied the preview but the ref kept the key. An identical entry reappearing
therefore matched the cache, the automatic read was skipped, and the viewer
stayed blank with no recovery short of reselecting. Clearing the guard while
no entry is selected is what lets the reappearance re-read.
Also corrects the guard's stated scope. It cannot survive a `ScreenStage`
remount — a remount mints a fresh ref — so the round-11 comment claiming it
did was wrong. That read is correct behavior rather than a gap: `previewState`
is local state and dies with the same unmount, so on return the viewer is
empty and the read is what refills it. Suppressing it would mean hoisting the
preview bytes above `ScreenStage` and showing bytes fetched under a connection
that may no longer exist.
The reappearance test is a real regression guard: restoring the defect fails
it while the older one-read-per-selection test still passes, so it is both
detecting and non-redundant. That older test remains a specification, for the
reason it already states, and now points at this one.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01R4441JdNLZerCVM7mDDSfB
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot review round 11: zero inline comments, two findings in the suppressed
block. Third round where the inline count alone would have ended the cycle.
The automatic SKILL.md read fired twice. I noted this possibility early and
dismissed it as harmless dev noise, which was wrong twice over: `ScreenStage`
remounts this screen while the selection persists, so it is not dev-only, and
this app is a protocol inspector — a phantom `resources/read` in the Protocol
panel that the user's own client would never send is the tool misreporting the
conversation, not merely a wasted round trip. An `autoReadKey` ref makes the
replay a no-op.
Panel scroll positions survived a skill change. `scrollTop` lives on the DOM
node rather than in React state, so switching between two long manifests showed
the new one part-way down with its first rows hidden, which reads as missing
data. The accordion is keyed by `manifestKey` so every panel gets a fresh scroll
container; `openSections` is controlled and survives.
⚠️ The test for the first fix is a SPECIFICATION, not a regression guard, and is
labelled as such in the file. It was written, then checked by removing the
guard — and it still passed. Three shapes were tried (a plain re-render, a
StrictMode wrapper, StrictMode with the selection present at mount) and none
reproduce the replay, because this environment does not double-invoke mount
effects. The fix stands on the reported reasoning; the test pins the contract of
one read per selection and must not be read as evidence the defect is fixed.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01R4441JdNLZerCVM7mDDSfB
Signed-off-by: cliffhall <cliff@futurescale.com>
Zero inline comments, three findings in the review's suppressed block — the
second round where the inline count alone would have ended the cycle. All three
are places where my own words did not match the code.
The Frontmatter section claimed to show "the raw YAML the server served, not a
re-serialised object". It did not: `ContentViewer`'s plain-text branch
pretty-prints anything that parses as JSON, and every JSON mapping is also valid
YAML — so `{"name":"a"}` came back re-serialised beneath a comment promising
verbatim bytes. For a conformance tool that is the one thing it must not do. It
now renders through `CodeHighlight` directly.
The green alert was titled "Conforms". Consolidating every verdict into this one
section means it can sit directly above a red digest mismatch, where an
unqualified "Conforms" contradicts the result on screen — the static checks
passing says nothing about the bytes. Retitled "No structural issues", which is
what it actually summarises. The contradiction was created by the consolidation
and the title was never revisited.
And the PR description said the shield stays on both integrity-checking
controls, while the per-row Verify rendered text only. The claim was right and
the code was not, so the row button now carries it too.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01R4441JdNLZerCVM7mDDSfB
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot review round 9, two findings.
The Skill Resource control shows the displayed file's name, and that control is
PINNED — it never scrolls — so a resource URI ending in a long breakable segment
grew the header. Fifth instance of the same class.
It took three attempts, and the mutation table is what exposed each one.
`lineClamp: 1` stopped the text wrapping but the row then wrapped the caption
onto its own line (+30.6px), because flexbox wraps before it shrinks. A
`nowrap` row improved that to +21.7px but Mantine's Accordion label wrapper
still would not let the caption shrink, so `miw: 0` had nothing to act on. A
hard `maw: "50%"` gives the ellipsis a definite width, and the control now grows
by 0px.
Worth recording that the FIRST version of that guard passed against the defect:
it asserted an absolute `height < 120px`, which an unclamped name satisfies
because it only wraps to a few lines. Comparing the control against its own
baseline — measured with a short name, then re-measured after selecting the
hostile one — is what makes it detect anything at all.
Second finding: `SkillDescription` carried `size: "sm"`, leaving typography at
the call site directly beneath a comment of mine asserting that these constants
carry layout only and that typography belongs in a `ThemeText` variant. Added a
`skillDescription` variant; only the clamp stays here.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01R4441JdNLZerCVM7mDDSfB
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot review round 8: the skill URI was still unbounded in the pane's fixed
header. It is the fourth instance of a pattern named in this PR's own round-7
reply — anything server-controlled and variable-height that sits beside the
zero-basis accordion sizes the layout — and it was directly above the
description clamped last round, which says something about naming a class not
being the same as sweeping for it.
So this audits the whole header rather than the reported element. All three
server-controlled strings are now bounded: the name truncates (already did),
the URI clamps to two lines, the description to three, and both clamps carry
the full value on a `title`. `HostileHeader` asserts the class is closed — a
long name, a 30-segment URI and a 400-word description, with the accordion
still taking more than half the card, every section non-zero, and the pane not
scrolling.
That new story then caught an accessibility defect this refactor introduced and
would otherwise have shipped: `scrollable-region-focusable`. Making each panel
its own scroll container means a panel whose content holds no focusable element
— the Conformance alerts, for instance — cannot be scrolled by keyboard at all.
Every `Accordion.Panel` now carries `tabIndex={0}`.
The geometric assertion lives in Storybook rather than the unit test, because a
CSS bound is not observable through happy-dom's `getComputedStyle`; the unit
test pins the contract that makes clamping safe instead — the full value stays
reachable on a `title`.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01R4441JdNLZerCVM7mDDSfB
Signed-off-by: cliffhall <cliff@futurescale.com>
Four findings — three inline, one suppressed.
`verifyRow` opened the Conformance section, and every "Verify all" worker calls
it once per row as it advances, so a batch begun on one skill kept calling it
after the user had switched away. The keyed writes discard those results, but an
open-state update is not keyed to a manifest, so obsolete work could reopen the
section on the newly selected skill. The section is now opened at the two USER
entry points — the row button and "Verify all" — through `openConformance`, and
`verifyRow` carries a note saying why it must not do it itself.
The skill description was an unbounded sibling of a zero-basis accordion in the
pane's fixed header. SEP-2640 permits 1,024 characters and a non-conforming
server can send more; in a narrow pane that wraps far enough to crowd the
sections out — the same "server-controlled content sizes the layout" trap
`viewerFlex` documents. Clamped to three lines, with the full text on a `title`.
`inferMimeFromUri` matched against the whole URI minus query and fragment, so an
authority ending in a mapped suffix was read as a filename:
`https://documentation.md` returned Markdown despite a pathname of `/`. It now
inspects `URL.pathname`, keeping the string fallback for URIs that cannot be
parsed.
And its decoding was too coarse. `decodeURIComponent` throws not only on a
malformed escape but on a syntactically valid octet that is not valid UTF-8, so
a single `%FF` anywhere in the path abandoned decoding for the whole string and
`skill://a/%FF/reference%2Emd` lost its `%2E`. Each escape run is now decoded
independently, and a run that is not valid UTF-8 falls back to decoding its
ASCII octets one at a time — all suffix matching needs.
Both behavioural fixes carry regression tests verified to fail with the defect
restored.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01R4441JdNLZerCVM7mDDSfB
Signed-off-by: cliffhall <cliff@futurescale.com>
Four findings — two inline, two in the review's suppressed block.
"Expand all" wrote `sectionIds` — the sections rendered at that instant — into
`openSections`, which outlives any one skill. Pressing it while the SKILL.md
read was still in flight dropped `frontmatter`, so the section arrived collapsed
with the control offering to expand all over again; a dynamic skill did the same
to `resources`. It now expands to the complete set, and `sectionIds` decides
only the toggle's label. The new test is verified to detect the defect: with
`sectionIds` restored it fails.
`inferMimeFromUri` matched the raw path, so a percent-encoded suffix such as
`reference%2Emd` produced no MIME at all — no markdown renderer and no
frontmatter split — even though `skillUriIdentity` already treats that spelling
as the same file. It now decodes defensively, falling back to the raw path on a
malformed escape rather than throwing inside what is only a guess.
Two comment fixes, both cases of a deliberate decision being contradicted by the
prose around it. `theme/Accordion.ts` still asserted "sized to content and never
shrink; overflow moves to the root" after round 5 moved the sections back to
`flex: 0 1 auto` with a floor — the exact stale rationale that makes a
load-bearing layout easy to regress. And the component doc, the
`onReadSkillFile` doc in `useServerCommands.tsx` and a `test-servers` fixture
comment all still said skill files are read only when the user asks to verify
them, one of them still naming the removed "View SKILL.md" button. All three now
separate reading (on selection, and not a load under SEP-2640) from verification
(strictly on demand).
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01R4441JdNLZerCVM7mDDSfB
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot review round 5, and it caught a fix that traded one bug for another.
Refusing to shrink (`0 0 auto`) was introduced to stop a squeezed section
slicing its content mid-line. It did that, and reintroduced the original defect
at scale: a CONFORMING manifest may declare 512 rows, and a section holding its
full intrinsic height pushes the file viewer off the bottom of the pane, so
reaching the file means scrolling past the manifest — precisely the "the file is
behind the manifest" problem this screen was refactored to end.
`0 1 auto` with the existing `OPEN_SECTION_MIN_HEIGHT` floor satisfies both: a
section gives up space until it reaches the floor, its panel scrolls internally
from there, and the viewer keeps the remainder. The floor is what stops the
shrink becoming the crush that sliced content — a panel at the floor scrolls
rather than clips. Both wrong directions are now recorded on the constant so
neither is re-derived.
`LongManifest` is the sibling story to `LongSkillDocument`, asserting that the
viewer's header stays within the card, that the Resources panel scrolls
internally, and that the pane does not scroll as one column. Verified to detect
the defect rather than merely pass: with `0 0 auto` restored it fails while the
other eight stories pass.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01R4441JdNLZerCVM7mDDSfB
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot review round 4. Both findings arrived in the review's *suppressed*
block with zero inline comments, so a round that reported "0 new comments" was
in fact carrying two real defects.
`useValueChange` deliberately does not fire on the first render, so the
clean-entry auto-collapse it drives never applied to a screen that MOUNTS with
a skill already selected — a restored `SkillsUiState` does exactly that, and
Conformance would sit expanded until some later selection change started
honouring the rule. The rule was written entirely in the change handler and the
initial state was never considered. `openSections` now seeds through
`initialOpenSections`, applying the same judgement up front while the
render-time synchronisation continues to handle every subsequent change.
The Resources panel still held `selected.resources === DYNAMIC_RESOURCES ? … :
…` inside a `{!isDynamic && …}` guard, and `isDynamic` is that exact
comparison, so the alert arm was unreachable. It is residue from moving the
dynamic banner into Conformance: the outer guard was added and the inner
ternary never removed. The manifest table now renders directly.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01R4441JdNLZerCVM7mDDSfB
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot review round 3. Precedence was `declared ?? inferred`, so a server
serving SKILL.md as `text/plain` or `application/octet-stream` defeated the
`.md` inference — and `ResourcePreviewPanel` already records that servers
commonly do exactly that. The file was then not recognised as markdown, so its
YAML frontmatter stayed in the viewer and the Frontmatter section disappeared,
for a perfectly valid skill.
A SPECIFIC declared type still wins, because a server that says `text/csv`
knows its own resource. Only the generic types yield to the suffix, via a new
`isGenericMime` that normalises parameters and casing the same way
`isMarkdownMime` does. Both directions are covered: `text/plain` on a `.md` URI
splits, `text/csv` on a `.md` URI does not.
The round's other finding — that `DetailCard` is not a flex container, so the
detail column has no definite height — is declined as a false positive, with
evidence on the thread: Mantine's own Card.css sets `display: flex;
flex-direction: column` on the card root, `DetailCard` still carries
`h: "100%"`, and the `LongSkillDocument` story added last round asserts the
exact behaviour claimed broken, in a real browser.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01R4441JdNLZerCVM7mDDSfB
Signed-off-by: cliffhall <cliff@futurescale.com>
Four findings, all real.
The `text/markdown` fallback applied to every manifest resource, but SEP-2640
expects a manifest to carry supporting scripts, examples and assets with types
of their own — an extensionless, untyped blob was being decoded and rendered as
markdown rather than falling back to binary. The fallback is now scoped to the
skill's own SKILL.md; anything else passes `undefined` and lets ContentViewer
apply its octet-stream/text default.
`mismatchCount` counts `status: "mismatch"`, which `verifySkillResource` uses
for both digest AND size failures, so the badge misreported a size-only failure
as a digest mismatch. Renamed to "N mismatch(es)" rather than filtering: a size
failure is a real verification failure and dropping it from the count would hide
it. The alerts still distinguish the two by title.
`isMarkdownMime` compared the raw string, so `text/markdown; charset=utf-8` and
any casing variant were rejected. That silently skipped the frontmatter split —
the fence stayed in the rendered document and the Frontmatter section vanished,
for a perfectly valid response. It now normalises to the base lowercase type.
The flex-basis regression this refactor exists to fix had no browser-level
guard: the unit tests only inspect accordion state, and the long test-server
fixture is never exercised by Storybook. `LongSkillDocument` is modelled on
`ResourcesScreen`'s `ManyResources` and asserts that every section keeps a
non-zero height, that sections tile without overlapping the header below, that
the viewer scrolls within its own panel, that the detail pane does not scroll as
one column, and that collapse-then-reopen restores identical geometry — the
sequence the original bug needed in order to show itself.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01R4441JdNLZerCVM7mDDSfB
Signed-off-by: cliffhall <cliff@futurescale.com>
Review on #2265 surfaced that the pane reported judgement from three sources
in three different places: static findings in Conformance, per-file digest
verification beside the manifest table, and the `skills/get` comparison in a
region of its own. All three now render in Conformance.
That also resolves the first Copilot finding structurally rather than with the
cap it suggested. The free-standing `skills/get` region had a content-sized
basis competing with an accordion whose basis was 0, so a large fetched entry
starved the accordion; moving the verdict inside deletes the competing region
entirely, and `FetchResultScroll` goes with it.
A red `N digest mismatch(es)` badge joins the errors/warnings badge, kept
separate because it is a runtime result — folding it into the error count would
make that number change meaning after a click. Conformance auto-collapses for an
entry with no findings and auto-opens whenever a verification, a fetch or a new
selection gives it something to say. Its badge is yellow for a warnings-only
entry; green read as "nothing to see".
A dynamic skill states its case once: the prose banner moves into Conformance
and the Resources section is dropped entirely, while the finding still counts
toward the warning total.
The second Copilot finding is fixed as suggested. Frontmatter splitting keyed on
the payload arm rather than the MIME, so a markdown blob kept its frontmatter
while a multi-document YAML file lost its first document. It now keys on an
effective MIME, with `inferMimeFromUri` extracted to `src/utils/` and shared
with `ResourcePreviewPanel` instead of duplicated.
Also: `Verify` gets its own table column so the buttons align rather than
tracking the verdict badge's width; the header actions take
`ListChangedIndicator`'s Refresh style and expand/collapse-all uses the shared
`ListToggle`; the `skills/get` copy no longer implies the entry defines the rule
it breaks; findings move below the JSON they annotate; and section spacing moves
into the `skillSections` accordion variant.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01R4441JdNLZerCVM7mDDSfB
Signed-off-by: cliffhall <cliff@futurescale.com>
`useProgressToasts` and `useTaskToasts` both track their live toast ids in a
`Set` held in a ref, and every toast's `onClose` closes over that Set object.
`ref.current` is one object for the component's lifetime, so the teardown's
`liveToastIds.clear()` emptied the very container the *next* effect run would
refill.
`notifications.hide` does not fire `onClose` synchronously — it plays the exit
transition first. By then the client swap has completed and the new session may
already have `show`n the same id (both hooks derive ids from data — the progress
token, the task id — so a reconnect replaying the same call reproduces them
exactly). The stale `onClose` then deleted the *new* session's entry, so the
next tick took the `show` branch instead of `update`, Mantine refused the
duplicate id, and the toast froze at its last progress before auto-closing.
Assign a fresh `Set` in the cleanup instead, so the outgoing session's callbacks
mutate only the outgoing session's Set. The teardown comments now say why the
swap is a swap and not a clear, since the distinction is invisible otherwise.
Each hook gains a regression test that fires the captured `onClose` by hand
after the new session has re-shown the id — no timers needed, since that call
*is* the late fire. Both fail against `clear()` and pass against the swap.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RFGzWfBjghUsUVGzyY43ne
Signed-off-by: cliffhall <cliff@futurescale.com>
`ReAuthBannerBar` set an unconditional `w: 420`. Because the banner is centered
with `left: 50%` plus a `translate(-50%, -50%)`, a viewport narrower than 420px
made it overflow *both* edges equally — clipping the close button on one side
and "Authorize again" on the other, which are the only two controls it has.
Losing both is a dead end rather than a cosmetic clip, since dismissing is not
equivalent to re-authorizing ("Authorize again" also clears the stale OAuth
state). It is reachable on a narrow desktop window too, not only a phone: the
element is `fixed`-positioned against the viewport.
The width is now capped rather than fixed — `maw={420}` with
`w="calc(100vw - 2rem)"` — so the banner shrinks with a 1rem gutter on each
side and the shadow and radius still read.
The same constant also carried `transform` and `zIndex` as flat CSS in
component-level `styles`. Mantine exposes neither as a style prop, but that
argues for the next tier in the repo's preference order — a theme variant — not
for inline `styles`. Both move to a `reauth` variant in `theme/Paper.ts`,
alongside the existing `code`, `contained` and `panel` variants.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LYcBw3Sftq5Yx4WpdzXttb
Signed-off-by: cliffhall <cliff@futurescale.com>
The pane was one scrolling column: SKILL.md sat below the manifest behind a
"View SKILL.md" button, the frontmatter pushed it further out of view, and
nothing could be put away.
It is now a flex column that does not scroll as a whole. Conformance,
Resources, Frontmatter and Skill Resource are four collapsible sections of one
`disclosure` accordion, and the file viewer claims the remaining height and
scrolls internally.
The viewer's flex-basis is 0, not auto, and that is load-bearing: `auto` makes
an item's basis its content height, and a rendered document is ~1,795px for the
data-analysis fixture. That basis joined the sum the container distributes,
over-constraining the column so the shrink factors crushed the siblings —
Conformance to a 2px panel, Resources to zero — with their contents spilling
over the headers below. Collapsing the viewer removed the giant basis, so the
symptom appeared on collapse-then-reopen. Open sections also carry a `mih`
floor, since the disclosure CSS sets `min-height: 0` on an active item.
Frontmatter now follows the displayed file rather than the selected skill.
Both halves come from one `splitSkillFile` call, so the section and the viewer
cannot disagree, and a file with no frontmatter renders no section at all.
Also: SKILL.md loads on selection (a `resources/read` is not a load under
SEP-2640), the manifest highlights on hover with each URI a button that swaps
the viewed file, both whole-skill actions moved to the pane header, and the
Conformance badge goes yellow for a warnings-only entry instead of green.
Flat CSS properties moved out of call sites into theme variants (ThemeText
gains four; a new ThemeTable takes the manifest font size).
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01R4441JdNLZerCVM7mDDSfB
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot review round 1.
- `newAttemptId` calls `globalThis.crypto.getRandomValues(...)` directly
rather than through a bound alias. CodeQL's `js/insecure-randomness`
browser model recognizes a secure RNG by that literal method call, so
the indirection would have left the `Math.random` last resort
classified as an unmitigated source.
- The no-crypto test arm now removes `globalThis.crypto` entirely
instead of hiding its two methods, matching the crypto-absence test in
`src/test/core/auth/utils.test.ts`. Hiding only the methods left the
global truthy, so the arm could not have caught a regression that read
`globalThis.crypto` without the optional guard. `withoutCryptoMember`
narrows to `randomUUID`, its only remaining caller.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Uw1s4LRBUzrFwLzPT4mAJn
Signed-off-by: cliffhall <cliff@futurescale.com>
`Record<string, unknown>` does not overlap `Crypto` (TS2352 under `tsc -b`).
`Partial<Pick<Crypto, …>>` says the same thing in one legal cast and keeps
`delete` operating on a known-optional property.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Uw1s4LRBUzrFwLzPT4mAJn
Signed-off-by: cliffhall <cliff@futurescale.com>
`clients/cli` resolved `@types/node` 24.13.1 against the root's 24.13.3.
Neither client declares it — #2196 consolidated it to the root — so the cli
copy arrived transitively via `@types/express` and was constrained by no
range of ours. `verify:dep-lockstep` reported OK and was right to: it
compares only packages that reach one `tsc` **program** from two installs,
and no one program sees both copies.
Fixes the instance and the class, which the issue notes are not exclusive.
**The class.** The guard grows a second tier alongside the program-derived
one, asking the weaker but broader question `AGENTS.md`'s "one version per
install-crossing dependency" rule actually states: does a package this repo
declares anywhere resolve to two versions across our installs at all? The
candidate set is every name in any install's `dependencies`/`devDependencies`
that more than one install holds a **top-level** copy of — 17 packages today.
Nested copies are excluded: one exists because some dependency asked for a
different version, so it is that dependency's range, not ours.
Neither tier subsumes the other. The program tier sees a copy no manifest
names (the `@modelcontextprotocol/sdk` case, arriving through another
package's `.d.ts`). The declared tier sees a transitive copy no program
loads (this issue), two *clients* disagreeing with no root copy involved
(`@types/react`, web against tui), and the peer shadows `AGENTS.md` calls out
as ungated — `eslint`, `typescript`, `vitest`.
Deny-by-default, with its own empty `TOLERATED_DECLARED_SKEW` and the same
within-a-major rule, now stated once in a shared `toleratesSkew` so the two
tiers cannot drift.
**The instance, and three others the new tier surfaced.** All four were
patch-level:
- `@types/node` — cli 24.13.1 vs root 24.13.3. Transitive, so it takes an
`overrides` entry in `clients/cli` (the repo's sanctioned lever for a
transitive pin), at the root's own `^24.12.4` rather than a frozen number.
- `@types/react` — web 19.2.18 vs tui 19.2.17.
- `react` — web 19.2.8 vs root/tui 19.2.7.
- `@vitejs/plugin-react` — web 6.0.5 vs root 6.0.2.
The last three are `npm update` in the stale install: it moves the lockfile
within the declared range without widening a range, which matters for the
root `react ^19.0.0` that `ink`'s externalized-bundle exemption depends on.
No declaration or range changed.
Tests: 4 end-to-end cases (the fixture gains manifest deps and a client-side
`solo` install, so the skew is real and no program holds both copies) and 13
helper cases.
Closes#2226
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01J86a6vMcA9csPtdsg7ZcCZ
Signed-off-by: cliffhall <cliff@futurescale.com>
Both findings from the v2.5.0 milestone-merge review (#2215).
1. `progressToastId` collided across distinct progress streams.
`ProgressToken` is `string | number`, so `String(token)` mapped the
numeric token 7 and the string token "7" onto one id — and because
notifications keyed by the same id are *replaced* rather than
stacked, two concurrent streams overwrote each other's toast. The
absent case was worse: it hardcoded the sentinel "default", which a
server is free to send as a genuine string token. The id now carries
the token's type (`progress-n:7` / `progress-s:7`) and gives the
no-token case a prefix of its own (`progress-none`) that no token
value can produce.
2. `newAttemptId`'s fallback now prefers `crypto.getRandomValues`.
`randomUUID` needs a secure context; `getRandomValues` does not and
exists in every browser that has `crypto` at all — so the exact
situation the fallback exists for (a `file://` page, a plain-HTTP
non-loopback host) still has a CSPRNG on hand. `Math.random` stays
as the last resort for a `crypto`-less global, and the "never a
security token" comment stays too. Retires CodeQL alert 72
(`js/insecure-randomness`) honestly rather than by dismissing it.
Tests: collision cases for 7 vs "7" and the absent token vs "default" /
"none", a distinctness sweep over the whole id space; and the attempt-id
fallback test split into a getRandomValues arm (asserting the CSPRNG is
used and `Math.random` is not) and a no-crypto-at-all arm.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Uw1s4LRBUzrFwLzPT4mAJn
Signed-off-by: cliffhall <cliff@futurescale.com>
Round 16 reported no inline comments and three suppressed findings, all
valid:
- `getSkillsExtension` treated a non-object extension value (`false`,
`"skills"`) as a declaration. SEP-2133 declares an extension as an
object of sub-options, so a primitive is not one — now rejected,
matching `appElicitation.ts`.
- The `onGetSkill` prop doc claimed the two responses "must agree" and
that a difference means a broken server, contradicting the
implementation, which reports a difference as a finding rather than a
fault. Reworded to what the code does.
- No real-transport test exercised the fixture's `skills/*` handlers.
Added `src/test/integration/mcp/inspectorClient-skills.test.ts`,
parameterized over both protocol eras: capability advertisement, the
paged `skills/list` walk (direct and through `ManagedSkillsState`),
`skills/get`, the `-32602` for an unknown URI, `resources/read` of a
skill file, and delegation of an ordinary resource through the
fixture's `resources/read` wrapper.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018kfrH3uRNUM4JAk1reKtyY
Signed-off-by: cliffhall <cliff@futurescale.com>
Round 15 generated no new inline comments; all five suppressed findings
were real and unaddressed.
- SkillsScreen: claim the attempt BEFORE issuing the request, for both the
SKILL.md preview and `skills/get`. Recording it only on settle left a
window where an older request that happened to resolve first was still
considered current and published while a newer one was in flight —
`verifyRow` already claimed its row up front, these two did not. Tests
answer the OLDER request first and assert nothing is published.
- SkillsScreen: "No skills listed", not "No skills". SEP-2640 lets a server
return an empty or partial catalog and says an empty result is not proof
it has none — an unlisted skill is still fetchable by URI — so the old
copy had the tool asserting something the protocol explicitly does not.
- SkillsScreen: each row's Verify button gets an `aria-label` carrying its
URI. Every row's visible text is "Verify" and the URI cell is not
programmatically associated with the button, so a screen-reader user had
no way to tell which file each control checked.
- inspectorClient: attribute a rejected `skills/get` envelope with
`markResponseRejected`, so the Protocol tab stops rendering it as a clean
success while the screen shows an error. In the client rather than a
store because `skills/get` has none. Gated on `isClientDecodeRejection`,
with a test that a transport failure is NOT attributed.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018kfrH3uRNUM4JAk1reKtyY
Signed-off-by: cliffhall <cliff@futurescale.com>
