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>
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>
`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>
`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>
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>
- SkillsScreen: put the SESSION in the invalidation key. This screen stays
mounted across a disconnect, so content alone cannot tell server A's entry
from an identical-looking one on server B — A's in-flight verification
could land afterwards and report `verified` for a file never read from B,
and a retained batch entry could leave B's Verify all disabled.
`useInspectorStores` now exposes a `sessionNonce`, bumped on both create
and destroy so it never repeats across a reconnect, and App keys on
`${activeServerId}:${sessionNonce}` — the server id alone would repeat,
which is one of the crossings this exists to prevent.
- SkillsScreen: match the selection by `skillUriIdentity`, in both the
lookup and the NavLink active check. A refresh that canonicalizes
`skill://demo/%53KILL.md` names the same skill, and the detail pane was
emptying out because the server changed its spelling — the last raw-string
URI comparison left after round 12.
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>
`ServerImportJsonModal` > "guards against a live edit made before the
debounce re-validates" opened its window on the real clock: it pasted
invalid JSON and clicked Add Server, relying on less than
VALIDATE_DEBOUNCE_MS (300ms) of wall time elapsing in between. When more
did, the debounce landed first, `canAdd` went false, and the click hit a
disabled button — a no-op that sets no submit error, so the
`findByText(/Fix the validation errors/)` timed out. The first assertion
(`onAddServer` not called) still passed, which is why the failure read as
mysterious rather than as a disabled button.
Run the test on fake timers end to end instead. The first validation is
landed explicitly with `advanceTimersByTimeAsync(VALIDATE_DEBOUNCE_MS)`
— imported from the hook, so the test cannot drift from the value it
depends on — and after the second paste the timers are simply not
advanced. The pending re-validation therefore cannot land at all, the
window stays open by construction, and the final assertion becomes a
synchronous `getByText`.
Verified with a throwaway probe holding both shapes side by side with a
deterministic 400ms stall injected between the paste and the click: the
old shape fails exactly as reported, the new one passes.
No timeout was widened and no production code changed.
Closes#2250
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VQgwZ1kGzhkdkMJ81JVg42
Signed-off-by: cliffhall <cliff@futurescale.com>
All three findings were the same underlying inconsistency: URI comparisons
on raw strings, in a flow that elsewhere deliberately treats normalized
equivalents as the same resource. One `skillUriIdentity` helper now backs
every comparison, so they cannot disagree.
- skills.ts: `manifest-missing-self` compares identities, so a manifest
listing the RFC-equivalent `skill://demo/%53KILL.md` is recognized as the
entry's own file — it is fetchable as that file, so reporting it missing
was the tool disagreeing with itself.
- skills.ts: `skillEntriesMatch` normalizes the entry URI and every manifest
URI before comparing, so a server that canonicalizes an escape between the
listing and the fetch is not reported as a changed snapshot.
- SkillsScreen: the `wrongUri` check uses the same identity, so a
canonicalizing server is not accused of answering for a different skill —
the read path already accepts exactly that equivalence.
- skills.ts: apply the Agent Skills name grammar to the RAW value. Trimming
first let `" demo "` pass, and whitespace is not in the grammar — so an
entry could report "Conforms" with a name that can never equal its URI
path segment. The trimmed copy now only distinguishes absent from invalid.
- skills.ts: add `malformed-description` for a description above the
1024-character Agent Skills limit, with boundary coverage.
- inspectorClient-skills.test: justify the `as unknown as` per AGENTS.md —
both fields are `private` with no public setter, the asserted shape is
exactly what the class declares, and the alternative is a live connection.
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>
- skills.ts: validate `frontmatter.name` against the Agent Skills format
(1–64 lowercase alphanumerics and hyphens, no leading, trailing or
consecutive hyphen). Checking only for non-emptiness let `Bad Name` read
as "Conforms" — and the name is not decorative: it must equal the URI
path segment, so a name that cannot appear in a URI is a contradiction
the entry cannot satisfy. Suppressed when the name is absent entirely,
which `missing-name` already reports.
- skills.ts: detect duplicates on the NORMALIZED identity. Containment and
the read that fetches the bytes both treat normalized-equivalents as one
resource, so `skill://demo/SKILL.md` and `skill://demo/x/../SKILL.md`
were passing as two distinct files while naming one. The finding still
reports the raw URI, and two different unparseable URIs stay distinct.
- SkillsScreen: run `checkSkillConformance` on the fetched `skills/get`
entry and treat an error — or an entry answering for a different URI —
as invalid (red), not as "a different snapshot" (yellow). A fresh
point-in-time read excuses a CHANGE; it does not excuse a violation, and
answering with another skill is never a refresh of the one requested.
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>
- SkillsScreen: track "Verify all" batches in a MAP keyed by manifest, not
one slot. A slot remembers only the most recent batch, so A running →
start B → return to A left A's button enabled and a second worker pool
could be started on top of A's first, doubling the concurrency cap. The
round-7 test switched to B but never STARTED B's batch, which is exactly
why it missed this; it now does, and fails against the old single slot.
- test-servers/skills.ts: pass `result` schemas to `setRequestHandler` for
both custom methods, and type the three builders from them, so a shape
change in `toEntry` / `listSkillsPage` fails `tsc` rather than shipping a
fixture that claims to conform. Declared locally rather than imported
from `core/mcp/skillsSchemas.ts`: a fixture validated against the
client's own schema could never catch the client being wrong.
One correction to the review's premise on that second point: the SDK does
NOT require `{ params, result }`, and supplying `result` does not validate
the response. Its own doc on `RequestHandlerSchemas` says `result` is
optional and "no runtime validation is performed on the result" — it types
the handler's return value. The change is worth making for that
compile-time check; it does not do what was claimed.
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>
- skills.ts: stop sorting `resources` arrays recursively. Only the entry's
OWN manifest is a set; `frontmatter` is verbatim arbitrary JSON, so a
custom `frontmatter.metadata.resources` array was being sorted too and
two genuinely different frontmatters compared equal. The manifest sort is
now done once, at the top level, and every other array keeps its order.
- SkillsScreen: invalidate on the whole selected entry, not just its
manifest. The `skills/get` comparison covers `frontmatter`, so a refresh
that changed only a description left a stale "matches skills/list"
verdict on screen — computed against an entry that no longer existed.
Re-verifying after a metadata-only refresh is the cheap direction to be
wrong in; showing a match against a different entry is not.
- SkillsScreen: give each "Verify all" invocation a token, so a finalizer
can only ever clear its own batch rather than any batch with a matching
manifest key.
- SkillsScreen.stories: supply `onGetSkill`. Every story rendered the
"Fetch with skills/get" button, and clicking it threw.
On the batch token: the A → B → A sequence the review describes is not
actually reachable, because the button is disabled whenever the in-flight
batch's key matches the selection — returning to A finds it disabled, not
clickable. The token is defence in depth for the same property, and the
test asserts what is really guaranteed rather than the unreachable path.
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>
Most of this round re-reported code the previous commit already changed
(it generated 0 new comments). Two findings were genuinely open.
- SkillsScreen: give "View SKILL.md" and "Fetch with skills/get" the same
attempt ordering `verifyRow` got in round 3. Both were still
last-completion-wins within one manifest: a double click whose older
request settled last replaced the newer result. One shared `isStale`
helper now covers all three actions — a result is dropped when its
manifest was invalidated OR a later click already wrote.
- test-servers/skills.ts + docs: stop calling all three edge cases
"non-conforming". `resources: "dynamic"` is a LEGAL wire form for
generated content — it is unverifiable, not invalid, which is exactly
why the checker reports it as a warning. The prose contradicted the
code it was describing. Now: one legal-but-unverifiable case and two
actual violations.
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>
Two of these reverse round 4, which asked for the opposite. Where the
readings conflict I took the permissive one: a wrong tightening rejects
conforming servers, a wrong loosening only fails to report something.
- skills.ts: stop requiring the `skill:` scheme. SEP-2640 says a server
SHOULD use `skill://` and explicitly allows a domain-native scheme, so
round 4's requirement handed a conforming `github://…` skill a false
`malformed-uri` and skipped its name and root checks. Containment is
scheme-independent — it compares against THIS entry's own root — so the
traversal fix that requirement came packaged with is unaffected.
- skills.ts: the 512-entry and 16 MiB limits are warnings. Both are
SHOULD NOTs for a server and MAYs for a host, so calling them errors
contradicted the "error = a MUST was broken" contract round 4
established and told authors a permitted skill was invalid.
- skills.ts: `skillEntriesMatch` compares canonically — keys sorted,
manifest sorted by URI. `JSON.stringify` treated key order and manifest
order as differences, though neither carries meaning.
- SkillsScreen: a differing `skills/get` is "a different snapshot"
(yellow), not a disagreement (red). The SEP defines it as a fresh
point-in-time read, so a skill that changed since the listing
legitimately differs.
- useServerCommands: route `onGetSkill` through the command-scoped OAuth
recovery like every other server command. An expired authorization
rendered an error and stopped there — no reauthorization, no retry.
- test-servers/skills.ts: stamp the modern base result envelope
(`resultType` / `ttlMs` / `cacheScope`) on every skills result. The SDK
stamps it only for methods in its own codec, and `skills/*` are
consumer-owned, so a 2026-era connection was receiving results without
it. Unconditional rather than per era: the modern leg builds a server
per request, so there is no era to branch on at registration time, and
on legacy they are three members no codec inspects.
- composable-test-server: remove the `skills.directoryRead` option. It was
publicly settable and advertised a method nothing serves, producing the
exact false capability the fixture exists to help catch. `skills` is now
a plain boolean; the flag returns in phase 3 (#2248) with its handler.
Still not changed: whether a modern `skills/list` RESULT SCHEMA must
require that envelope. Stamping it server-side is free; requiring it
client-side would reject servers on a reading nobody has checked against
the normative text. #2248 owns settling it.
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>
Three things the Skills work got wrong in that modal, all found by review
of the PR screenshots.
- The "Skills Extension" section repeated `io.modelcontextprotocol/skills`
as its value — the identifier the "Server Extensions" section two rows
above already lists, so the section added nothing. What a flat key list
*cannot* show is the extension's sub-options, which is the fact a server
author opens this modal to check. Renamed "Skills Extension Options" and
rendered as a ✓/✗ row for `directoryRead`, in the same vocabulary as the
capability columns, so it reads as the same kind of claim.
- The extension sections' contents were bold (`ValueText`, the value half
of a label/value pair) while sitting directly beneath the capability
checklists, which are plain. They are lists of items, not values, so
they now use the same `Text` the checklist rows do.
- Those lists were comma-joined into one line, which wraps mid-identifier
in a half-width column. One row per identifier.
The `skills-directory-read` tests now assert `data-supported` rather than
the copy: "Not supported" contains "Supported", so a text assertion passed
for either answer.
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>
- SkillsScreen: actually call `skills/get`. The client method and its
tests existed but no production caller invoked it, so #2234's acceptance
criterion ("skills/get retrieves a single entry") was unmet and a server
author's required handler could not be exercised. The detail pane now
fetches the selected URI on demand and reports whether the fetched entry
AGREES with the one skills/list advertised — both describe the same
skill, so a disagreement is a server bug only a side-by-side fetch shows.
- core/mcp/sha256.ts: a dependency-free SHA-256, used when `crypto.subtle`
is absent. `SubtleCrypto` needs a secure context, and this app is
documented as servable over plain HTTP on a LAN IP — where every skill
verification threw and the UI reported a read failure for files it had
fetched fine. Checked against the FIPS 180-4 vectors and differentially
against WebCrypto.
- skills.ts: require the `skill:` scheme in `normalizeSkillUri`. Checking
only that a URI was hierarchical let `https://demo/SKILL.md` pass the
name and root checks — a manifest pointing anywhere on the web, reported
as conforming.
- skills.ts: `missing-digest` and `missing-size` are errors. Both are
required fields, and an omitted `size` is what lets a server slip past
the 16 MiB pre-fetch limit while the UI reports zero errors. `warning`
is now reserved for what is legal yet unverifiable — `"dynamic"`.
- SkillsScreen: key the "Verify all" batch guard to the manifest. A global
flag left a newly selected skill's button disabled until the previous
skill's reads settled — forever, if one hung.
Not changed, deliberately: whether a modern-era `skills/list` result MUST
carry the SEP-2549 caching attributes. #2234's analysis records it as open
and the review asserts the opposite; neither reading was checked against
the normative text. Leaving the schema permissive accepts a server that
omits them, while tightening on a wrong reading would reject conforming
responses — the more expensive direction. `skillsSchemas.ts` states this
and #2248 settles it.
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>
- skills.ts: decide root containment on NORMALIZED URIs. A raw prefix
check passed `skill://acme/billing/refunds/../other.md`, which starts
with the advertised root but resolves outside it — a traversal the
conformance report was reporting as clean.
- skills.ts: `normalizeSkillUri` also rejects a relative string, so
`demo/SKILL.md` is `malformed-uri` rather than a skill path, and an
opaque-path URI (`skill:demo/..`), which the parser leaves un-normalized
and on which containment cannot be decided.
- skills.ts: `missing-description` is an error. SEP-2640 requires it, so
an absent one must not read as "0 errors" in the conformance summary.
- managedSkillsState: call `markResponseRejected` for a decode rejection,
as every managed list does. An invalid `skills/list` result was showing
in the Protocol tab as a clean success.
- SkillsScreen: per-row attempt token. The manifest key cannot tell two
verifications of the SAME row apart, so a double click (or a row button
pressed during "Verify all") let an older read finish last and overwrite
the newer verdict.
- SkillsScreen: disable "Verify all" while a batch runs. The concurrency
cap is per invocation, so repeated clicks stacked pools — 4, then 8,
then 12.
- SkillsScreen: include the finding index in each issue alert's key. Three
identical URIs produce two `duplicate-resource` findings with the same
code and URI, and React was free to drop the extras — hiding findings in
exactly the malformed input this view exists to inspect.
- test-servers/skills.ts: register `skills/list` and `skills/get` through
the PUBLIC `setRequestHandler` with explicit param schemas. The private
`_requestHandlers` map is now reached only to wrap `resources/read`,
which has to chain onto the SDK's handler rather than replace it — the
one thing the public API cannot express, and the comment now says so
instead of citing the tasks fixture.
- test-servers/skills.ts: raise `-32602` for an unknown `skills/get` URI.
A plain Error mapped to a generic server failure, making the fixture
non-conforming outside its three documented bad cases.
- clients/web/README.md: the paragraph named four contracts while still
saying the smoke "drives all three".
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>
- SkillsScreen: key verdicts and React elements by manifest ROW INDEX,
not URI. The checker deliberately tolerates a duplicated URI so it can
report `duplicate-resource`; a URI key collided those rows into one
verdict, so verifying either updated both and "Verify all" raced two
different digest/size declarations into the same slot.
- SkillsScreen: hold the invalidation generation in React state keyed by
the manifest, not a ref bumped during render. `useValueChange` runs its
callback during render and requires setState-only purity — an abandoned
render left the ref incremented and silently discarded a live
verification.
- SkillsScreen: title a size disagreement "Size mismatch" and render its
reason. `verifySkillResource` catches it before hashing, so the alert
was showing "actual undefined" under "Digest mismatch".
- SkillsScreen: pass `contents` to ContentViewer so a base64 SKILL.md
renders. The text-block form substituted "" and painted a blank preview
for a file verification had just read correctly.
- skills.ts: add `malformed-size` for a size that is not a non-negative
safe integer, and exclude such values from the 16 MiB total. A negative
one could pull the sum back under the limit and hide a violation.
- managedSkillsState: make the overlap guard per-session instead of a
boolean. A disconnect during an in-flight walk left it set, so the
reconnect's own load no-oped and was never retried — permanently, if
the stale request never settled. A stale `finally` can no longer clear
the live session's guard either.
- skillsSchemas: require the `{ skill }` envelope for `skills/get`. The
accepted SEP settles it, and normalizing an inline entry would let a
non-conforming response past the one place that could report it.
- #2248 and the PR description: corrected — they said
`ReadResourceDirectoryResultSchema` was already declared, which round 1
removed. #2248 now owns defining it, and records the frontmatter
cross-check gap too.
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>
- skills.ts: report a manifest that omits the skill's own SKILL.md
(an empty list included), duplicate URIs, entries outside the skill
root, and a missing size. `Conforms` was reachable for manifests that
break invariants SEP-2640 states.
- skills.ts: cross-check the declared byte length before hashing. A size
that disagrees fails verification on its own — the digest is taken over
the bytes the server served, so agreeing with it says nothing about
whether the manifest describes them.
- skills.ts: copy the view instead of slicing its backing store in
`sha256Digest`. `SharedArrayBuffer.prototype.slice()` returns another
SharedArrayBuffer, which `crypto.subtle.digest` rejects — the cast
claimed to handle the exact input that would have thrown. No cast now.
- skills.ts: state the one obligation NOT checked here — that an entry's
frontmatter matches the fetched SKILL.md's. The digest cannot cover it,
and closing it needs a YAML parser, so it is tracked on #2248.
- managedSkillsState: cap the walk at LIST_MAX_PAGES. The repeated-cursor
guard only catches a server stuck on one cursor; endlessly unique ones
walked forever. Raises rather than truncating, like the salvage walk.
- managedSkillsState: gate every write on a session generation, so a walk
that resolves after a disconnect or destroy cannot repopulate a cleared
store or deliver the previous session's skills into the next.
- inspectorClient: send a cursor when it is `!== undefined`, not when it
is truthy. An opaque cursor may be `""`, and dropping it re-requested
page one — which the store then reported as a repeated-cursor failure.
- SkillsScreen: key verdict invalidation on the manifest (URI + digests +
sizes), not the URI alone. A Refresh that changed the manifest left a
green badge attached to a digest nothing had checked.
- SkillsScreen: epoch-guard every read continuation, so a fetch that
resolves after the selection moved on cannot write into the new one.
- SkillsScreen: bound "Verify all" to 4 concurrent reads. A conforming
manifest may hold 512 files.
- skillsSchemas: drop the guessed `resources/directory/read` result
schema. Nothing calls it, so an unverified shape could sit wrong
indefinitely; phase 3 adds it against the normative text.
- skills-http.json: declare the extension bare. It advertised
`directoryRead: true` with no handler, so Connection Info reported
"Supported" for a method that answers -32601.
- types.ts: restore the Tasks doc comment the Skills interface displaced.
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>
Adds detection, enumeration, and conformance checking for the Skills
extension (`io.modelcontextprotocol/skills`) — phases 1 and 2 of #2234,
which is everything its Acceptance list names.
Skills is a *server*-declared extension, read off the connecting server's
`capabilities.extensions`, so it is deliberately absent from
`ADVERTISABLE_EXTENSIONS` — that registry is what the Inspector advertises
and the user toggles, and an entry there would be a meaningless toggle.
`skills/list` and `skills/get` are consumer-owned extension methods that
neither era codec defines, so the SDK's era gate skips them and they go out
as ordinary `client.request` calls with explicit result schemas. The
raw-wire channel modern `tasks/*` needs is not used, and the Skills tab is
not era-gated: a legacy-era server that declares the extension is serving it.
The checks are the point, not the list view. `core/mcp/skills.ts` reports
each obligation SEP-2640 states — the name/path invariant, the digest
format, the 512-entry and 16 MiB limits, and `resources: "dynamic"`, which
means integrity cannot be verified at all. Digest verification hashes the
fetched bytes with WebCrypto and returns a mismatch with both digests
attached rather than throwing, because showing a mismatch loudly is the
whole value proposition.
Files are fetched on demand: SEP-2640 is explicit that a `resources/read`
of a `SKILL.md` is not a load and confers no standing, so none of the SEP's
host machinery is implemented.
The `skills-http` fixture serves four skills over two pages, three of them
deliberately non-conforming — without those the verification code is
untestable.
Phase 3 (CLI, TUI, `resources/directory/read`, paged mode) is #2248.
Closes#2234
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>
Copilot review round 14. Two code fixes and a documentation sweep.
- `buildRevocationRequest` ran outside `revokeToken`'s best-effort try.
`encodeURIComponent` throws on a lone UTF-16 surrogate — valid JSON, so it
can reach there from a persisted client id or secret — and every caller has
already cleared its local state by then, so a rejection instead of a `failed`
outcome would break the documented guarantee. Construction is inside the try
now, and the catch reports `params.endpoint` rather than a `url` that may not
exist on that path.
- The fixture's Basic decode turned a malformed percent escape into an Express
500, reporting a bad credential as a server fault rather than as the
`invalid_client` 401 the endpoint means. Decoding failure is failed
authentication.
- Ten places still said revocation happens *before* the clear, which the
reorder in the previous commit made false: the settings-form description, the
`--no-revoke` help text, both READMEs, `docs/mcp-server-configuration.md`,
three JSDoc blocks, and the PR body. Swept, and the ordering is stated where
it is load-bearing rather than asserted in passing. Also merged the two
adjacent JSDoc blocks on `InspectorClient.clearOAuthTokens`.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0177mPHAdECD18nLLCTwR5rh
Signed-off-by: cliffhall <cliff@futurescale.com>
Clearing the Inspector's OAuth state deleted the local copy of the tokens
and stopped there. The access token — and the refresh token, which is
long-lived by design — stayed valid at the authorization server until they
expired on their own, so a day of connect/disconnect iteration left it
holding grants for sessions that ended hours ago. RFC 7009 §1 describes
exactly that case, and nothing in the repo constructed a revocation
request, so a server author implementing `revocation_endpoint` had no way
to exercise it with the tool that exists to exercise this.
core/auth/revocation.ts builds and sends the request; the three clear paths
call it, so the behavior is written once — web's "Clear OAuth state and
disconnect" (both the active and stored-only paths), the TUI's Auth tab,
and the CLI's --relogin. It runs BEFORE the local clear, because the token,
the client credentials and the discovered `revocation_endpoint` all live in
the store the clear is about to empty. The request names the refresh token
when there is one: §2.1 asks the AS to also invalidate the access tokens
issued under the same grant, so one request covers both halves.
Best-effort by construction. Every path returns a TokenRevocationOutcome
rather than throwing: no advertised endpoint means nothing is sent and that
authorization server behaves exactly as before, and a network error, a
non-2xx, a 5s timeout or an unreadable store is reported and nothing more.
Forgetting the tokens is what the user asked for. `lost_authorization_state`
recovery deliberately skips it — it clears a half-finished flow to retry it,
and an authorization that never completed has no grant to revoke.
Client authentication resolves the preregistered slot first, mirroring
BaseOAuthClientProvider.clientInformation; reading only the dynamic slot
would send no authentication for a server configured with `oauth.clientId`.
The Basic credential is byte-identical to the SDK's applyBasicAuth so this
request and the token request cannot present the same secret differently.
Opt out per server with `oauth.revokeOnClear` (default on, only `false`
written to disk) or per CLI run with --no-revoke. Read at clear time rather
than connect time, so toggling needs no reconnect. Turning it off is a
testing affordance: a client that walks away still holding live tokens is a
case a server author may want to reproduce.
test-servers now advertises `revocation_endpoint` and serves POST
/oauth/revoke, faithful on §2.2 (an unknown token is a success) and §2.1
(revoking a refresh token invalidates the access tokens under the same
grant), with oauth-revocation-http.json / oauth-no-revocation-http.json as
showcase configs.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0177mPHAdECD18nLLCTwR5rh
Signed-off-by: cliffhall <cliff@futurescale.com>
Three findings, all valid on verification.
`onToggleConnection` and `onDisconnect` do not own every rejection, so a
bare `void` on them turns an ordinary Connect/Disconnect click into a
global unhandled rejection with nothing shown to the user. The toggle has
three escape paths outside its own try/catch — awaiting
`initialConfigSettledRef`, constructing the client via
`setupClientForServer`, and the already-connected disconnect branch,
whose `try/finally` runs `finalizeExplicitDisconnect()` and then lets a
failed transport close propagate — and `onDisconnect` is that same
`try/finally` on its own. Both are now terminated with a `.catch` that
toasts the failure.
The blanket comment claiming all seven wrappers own their failures was
wrong. The five command handlers (`onCallTool`, `onGetPrompt`,
`onReadResource`, `onCancelTask`, `onOpenApp`) do each end in a `catch`,
so `void` stays correct for them; the comment now splits the two groups
and says which is which.
Adds the two tests the review asked for, covering the rejected-close path
through both the toggle and the explicit Disconnect. Each fails against
the previous `void` code, so they pin the fix rather than merely passing.
The `InspectorView` double grows a `disconnect` button, since the
standalone `onDisconnect` was otherwise unreachable from it.
Also restores the `PropOverrides` doc example, which the scripted
regrouping had rewritten inside its own JSDoc into a triple-nested shape
that would not type-check if copied.
Signed-off-by: cliffhall <cliff@futurescale.com>
The panel tests stopped at the disabled button, which leaves the wire claim
unverified: a screen that flagged the draft and still fired its callback
would have satisfied every one of them.
`ToolsScreen` and `AppsScreen` now assert `onCallTool` / `onOpenApp` are not
called for a refused draft, and ARE called once the value is rewritten with
the declared type — so the draft is demonstrably what blocks the dispatch
rather than an inert screen.
The plain and "Run as task" paths need no separate case: the panel has one
Execute button whose disabled state does not depend on the toggle, and the
callTool/callToolStream split happens later in App.tsx, downstream of the
callback a refused draft never reaches.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0177mPHAdECD18nLLCTwR5rh
Signed-off-by: cliffhall <cliff@futurescale.com>
`<InspectorView>` took ~130 flat props, which made ~170 of App.tsx's JSX
lines a prop wall rather than a component tree. It now takes 12 domain
props and the call site is 14 lines.
Two conventions keep the change reviewable. No field is renamed — every
field inside a bundle carries the exact name it had as a flat prop, so
this is a regrouping rather than a rename. And the bundles stop at
`InspectorView`: it destructures each one back into the same locals its
body already used and passes the same individual props down, so nothing
below the view knows they exist.
Three props have no single screen and are placed deliberately:
`onCompleteArgument` / `completionsSupported` are read by both the
Prompts and Resources screens, so they sit in `connection` rather than
being duplicated; `malformedListItems` is one array three screens filter
for their own entries, so it sits in `shell`; `erroredServerId` /
`connectedServerId` are connection outcomes the Servers screen renders,
so they sit in `connection`.
Every multi-line closure declared inline in the JSX is now a named
`useCallback` above the return, including `onServerReorder`'s `.catch`
and the seven `void`-discarding wrappers, which carry their
`no-floating-promises` justification once rather than at each call site.
The `InspectorView` double in App.test.tsx is now typed with the real
`InspectorViewProps` instead of a hand-written structural mirror, so it
cannot drift from the component it stands in for. Stories keep one
constant per bundle, since Storybook merges args only at the top level.
No behavior change.
Signed-off-by: cliffhall <cliff@futurescale.com>
The Tools and Apps tabs' "Edit as JSON" editor sent its draft through the
string-to-declared-type conversion `callTool` applies for the widget form, so
`{"count":"01"}` against a numeric schema left as `{"count":1}` — the editor
showed one payload and the wire carried another.
Both JSON-authoring surfaces now refuse such a draft and name the value to
rewrite. Edit-and-replay already did; this makes the Tools and Apps forms
agree with it, which is the consistency #2171's acceptance asks for.
`coercedArgumentNames` and `coercedArgumentsError` move into core/json so the
two surfaces share one predicate and one sentence — the check runs the real
conversion and compares rather than restating its rules, so it cannot drift
from what would actually be sent. Enforcement is opt-in
(`SchemaFormProps.enforceToolArgumentTypes`): an elicitation renders through
the same form and its values are never converted, so enforcing there would
refuse a draft for a reason that is not true of it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0177mPHAdECD18nLLCTwR5rh
Signed-off-by: cliffhall <cliff@futurescale.com>
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>
Copilot's thirteenth pass, two comments, both real.
- `1e-400` has a nonzero mantissa and parses to `0`. The digit comparison skips
it for being exponent-form and it is not negative zero, so nothing saw a
document that says one number and sends none of it. Caught now by pairing
`Number(literal) === 0` with a nonzero digit in the mantissa, which leaves a
literal written as zero (`0e-400`) and a subnormal that still parses to
something (`1e-320`) alone.
- The `resource` content-block branch rendered every embedded resource in a
plain code block, ignoring `resource.mimeType`, so a prompt message carrying
JSON did not inherit this migration even though the text-block branch did. An
embedded *text* resource declaring JSON now goes through the same dispatch.
Narrowed to JSON, as the Network tab's bodies are: routing every declared type
through would also put an embedded `text/html` resource into the sandboxed
frame and a `text/csv` one into a table. The Resources screen already renders
resource contents that way, so doing it here too is defensible — but it is a
change to what a tool result *is*, and belongs to whoever decides that.
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's twelfth pass. The suppressed comment is right and is fixed here; the
inline one is real too, but needs a core API change and is filed as #2171.
`-0` is represented exactly, so the digit comparison had nothing to say about
it — but `JSON.stringify` writes it as `0`, so the loss happens on the way
*out* rather than on the way in. The editor showed `-0` and the wire carried
`0`, and the viewer rewrote a received `-0` while pretty-printing.
`hasImpreciseIntegerLiteral` becomes `findUnsendableNumberLiteral`, checking
`Object.is(Number(literal), -0)` for every numeric spelling — `-0`, `-0.0`,
`-0e1` — before the integer-only digit check. Returning the offending literal
rather than a boolean lets the message name it, which is more use than either
of the two rules behind it and matches how the duplicate-key message reads.
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's eleventh pass: three comments, one defect in three places.
`JSON.parse` accepts duplicate member names and keeps the last without
complaint, so `{"role":"user","role":"admin"}` becomes `{"role":"admin"}` —
a document that renders, and submits, as *less* than it says. In the read-only
viewer that is the Inspector hiding what the server sent; in a draft it is the
editor showing more than the wire will carry. Nothing downstream could catch
it, because by the time `JSON.parse` returns the evidence is gone.
New `findDuplicateObjectKey` scans the source, scoped per object so
`{"a":{"a":1}}` and `[{"a":1},{"a":2}]` are fine, and compares names by decoded
value so `"a"` and `"a"` collide as they should. It shares its
escape-handling with the number-literal scanner, which now reads strings
through the same helper rather than skipping them inline.
Wired into all four JSON-draft parsers and into the viewer's faithfulness
check, so the same document is refused, or shown unindented, everywhere.
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's tenth pass generated no new inline comments. Of its two suppressed
ones, this fixes the first and declines the second.
`ContentViewer` pretty-prints by `JSON.parse` then `JSON.stringify`, which is a
round trip through JavaScript's number type and is not lossless. A server
sending `{"id":9007199254740993}` was shown `…992`, and one sending
`{"limit":1e400}` was shown `null`. Neither is a formatting difference: both
are the viewer telling the reader something untrue about the payload it
received, which for an inspector is the worst kind of bug. It predates this PR
— `formatJson` has always round-tripped — but this PR is where the read-only
JSON path was rebuilt, and the same two predicates the draft parsers use answer
it exactly.
Both formatters now display the payload as it arrived when reindenting would
alter a value: unindented, but honest. The copy button already copied the
original text either way, so this makes the screen agree with it.
Not changed: a literal whose *spelling* is normalized but whose value is not.
`0.10000000000000001` and `0.1` are the same double, as are `1e2` and `100`, so
those still format. Rejecting them would mean refusing any literal not already
in JS's shortest form — `1.50`, `1e2`, `0.5000` — which blocks drafts that send
exactly what they mean. The integer guard stays because there the digits name a
value the double is not.
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>
