feat: support the Skills extension (SEP-2640) with digest verification

[cliffhall]

Closes #2234

Adds Inspector support for the Skills extension (SEP-2640, io.modelcontextprotocol/skills) — phases 1 and 2 of the issue's plan, which is everything its Acceptance list names. Phase 3 (CLI methods, TUI pane, resources/directory/read, paged mode) is filed as #2248.

The extension is a server-declared one, so it is read off the connecting server's capabilities.extensions and deliberately not added to ADVERTISABLE_EXTENSIONS — that registry is the catalog of extensions the Inspector advertises and the user toggles in Server Settings, and putting Skills there would produce a meaningless toggle. The precedent followed is core/mcp/appElicitation.ts, which reads the server side the same way.

No SDK change, and no raw-wire channel

skills/list, skills/get and resources/directory/read are consumer-owned extension methods that neither era codec defines, so the SDK's era gate skips them (Protocol._assertOutboundRequestInEra only fires for names a codec knows) and assertCapabilityForMethod falls through to a no-op. The entire client-side mechanism is an ordinary client.request(…, ResultSchema) with an explicit result schema.

That is why the raw-wire escape hatch modern tasks/* needs is not used here, and why the Skills tab — unlike Tasks — is not era-gated: a legacy-era server that declares the extension is serving it, so it gets the tab.

More than a viewer

SEP-2640 puts real obligations on whoever consumes a skill, and each one is a check a server author wants run against their implementation. core/mcp/skills.ts produces a structured finding list rather than a boolean.

The severity split is load-bearing, so it is stated rather than implied: error means a MUST was broken, so a skill reporting "0 errors" really is one the spec accepts; warning is everything the spec permits but a consumer still wants told about.

Finding	Severity	Why
dynamic-resources	warning	resources: "dynamic" is a legal form for generated content. Nothing is wrong with it — integrity simply cannot be verified at all, which is the case most easily buried
resource-limit-exceeded / size-limit-exceeded	warning	The 512-entry and 16 MiB bounds are interoperability limits: a server SHOULD NOT exceed them and a host MAY support more, so an oversized skill is less portable, not invalid
missing-name / missing-description	error	The two frontmatter fields the SEP requires
malformed-uri	error	Not a hierarchical URI ending in /SKILL.md with a path segment before it. The scheme is not constrained — skill:// is a SHOULD, and a domain-native scheme like github:// is explicitly allowed
name-path-mismatch	error	The one structural invariant the SEP states outright: the segment before /SKILL.md must equal frontmatter.name
manifest-missing-self	error	A manifest is the complete file set, so one omitting the skill's own SKILL.md — an empty list included — cannot be checked against the skill
duplicate-resource / resource-outside-skill-root	error	A repeated URI, or one that resolves outside the skill root (checked after normalization, so …/refunds/../other.md cannot slip through a prefix test)
missing-digest / malformed-digest	error	digest is required, and must be sha256: + 64 lowercase hex
missing-size	error	size is required, and an omitted one is how a server slips past the 16 MiB pre-fetch limit while looking clean

Digest verification is separate and on demand. verifySkillResource cross-checks the declared byte length first (cheap, and a length that disagrees is a real inconsistency even when the digest matches), then hashes with WebCrypto — falling back to core/mcp/sha256.ts where crypto.subtle is absent, which is any plain-HTTP LAN deployment. A mismatch is returned with both digests attached, never thrown, because showing it loudly is the whole value proposition and a throw would collapse it into a generic failure message.

Fetching is on demand for a spec reason too: SEP-2640 is explicit that a resources/read of a SKILL.md is not a load and confers no standing, so the Inspector reads only what the user asks it to verify. None of the SEP's host machinery — activation, per-skill consent, content-bound approval — is implemented. Surface and verify.

What changed

core/

• mcp/skillsSchemas.ts — the wire surface for the two methods this PR calls. Permissive where a non-conforming server should be reported rather than rejected at the parse (looseObject, digest typed as a plain string, so the conformance checks can name the problem); strict where a shape is settled — GetSkillResultSchema requires the { skill } envelope and rejects an entry returned inline, because normalizing that silently would let a non-conforming response past the one place that could have reported it. There is deliberately no resources/directory/read result schema: nothing calls that method yet, so an unverified shape could sit wrong indefinitely without failing anything. Phase 3 (Skills extension phase 3: CLI methods, TUI pane, resources/directory/read, and paginated mode #2248) adds it against the normative text, alongside the call.
• mcp/skills.ts — detection, the conformance checks, and digest verification. error means a stated SEP requirement was broken, so "0 errors" is an answer the spec would give; warning is reserved for what is legal yet leaves integrity unverifiable, which is "dynamic" and nothing else.
• mcp/sha256.ts — a dependency-free SHA-256 for when crypto.subtle is absent. Not an optimization: SubtleCrypto requires a secure context, and this app is documented as servable over plain HTTP on a LAN IP, where every verification would otherwise throw.
• mcp/inspectorClient.ts — getSkillsExtension(), listSkills(cursor), getSkill(uri). Both required methods are called by the UI, not merely available: the Skills tab pages through skills/list and fetches the selected entry with skills/get on demand.
• mcp/state/managedSkillsState.ts — walks every skills/list page, clears on disconnect, records the last failure as observable state. Deliberately not a ManagedListState subclass: that base is built around a top-level ServerCapabilities key to gate on and a list_changed notification to debounce, and Skills has neither. It also guards against a server that repeats a cursor, which would otherwise walk forever.
• react/useManagedSkills.ts — useStoreSnapshot, never useState + a subscribing effect.

clients/web/

• SkillsScreen — a conformance view: the frontmatter, every finding, and the resource manifest with a per-file verdict and both Verify all and per-row Verify. Selection changes drop the verdicts during render via useValueChange, so a newly selected skill never paints a frame carrying the previous one's results.
• The Skills tab, its lifted UI state, the OAuth-resume snapshot entry, and Connection Info's Skills Extension Options section — which exists because the generic "Server Extensions" list shows the identifier but not directoryRead, and that sub-flag is what a server author opens the modal to confirm. The two extension lists beside it also stopped being bold, comma-joined single lines: they are lists of items sitting directly under the capability checklists, so they now read like them.

test-servers/

• skills.ts + configs/skills-http.json — four skills over two skills/list pages, three of them deliberately non-conforming (a digest mismatch, a "dynamic" skill, a name/path disagreement). Without those the verification code is untestable. Works on either era, for the reason above.

Testing

npm run local:gate passes. New per-file coverage clears ≥90 on all four dimensions. Four rounds of Copilot review are folded in — see the PR comments for what each finding was and how it was answered, including the two that were deliberately declined and why.

Screenshots below.

Screenshots

Captured headlessly against the built prod bundle connected to test-servers/configs/skills-http.json.

The Skills tab. Four skills over two skills/list pages — the Protocol panel shows both calls, which is what proves the cursor walk ran. The sidebar badge counts each skill's static findings.

A conforming skill, verified. Both files — the skill's own SKILL.md and its reference.md — hash to the digests the manifest advertised, and both lengths agree with the declared sizes.

skills/get, the extension's second required method. Fetched on demand and compared against the entry skills/list advertised — the two describe the same skill, so a disagreement is a server bug only a side-by-side fetch can surface. The Protocol panel shows the call.

A digest mismatch. notes.md advertises a well-formed digest of bytes the server does not serve. The row badge flips to MISMATCH and the alert names the file with both digests — expected and actual — rather than a generic failure.

resources: "dynamic". No manifest, so there is nothing to verify and Verify all is disabled rather than a button that silently does nothing.

A name/path disagreement. Served from wrong-folder/ while claiming the name right-name — the one structural invariant SEP-2640 states outright.

Connection Info. "Server Extensions" lists the identifier; Skills Extension Options adds what a flat key list cannot show — the extension's sub-options, in the same ✓/✗ vocabulary as the capability columns. directoryRead is the only one SEP-2640 defines, and the fixture declares the extension bare: it serves no resources/directory/read, so reporting it as supported would have been a lie.

[Copilot]

🟡 Changes recommended

Critical and moderate issues remain in manifest validation, pagination safety, verification state, and advertised capabilities.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Adds SEP-2640 Skills extension support, including discovery, conformance reporting, digest verification, and web Inspector integration.

Changes:

• Adds Skills schemas, client APIs, pagination, validation, and digest verification.
• Introduces a Skills tab, capability reporting, and persisted UI state.
• Adds test-server fixtures, tests, stories, and documentation.

File summaries

File	Description
test-servers/src/skills.ts	Implements Skills fixtures and handlers.
test-servers/src/resolve-config.ts	Resolves Skills configuration.
test-servers/src/load-config.ts	Defines Skills configuration types.
test-servers/src/composable-test-server.ts	Advertises and wires Skills support.
test-servers/configs/skills-http.json	Adds the Skills showcase server.
docs/test-servers.md	Documents the Skills fixture.
core/react/useManagedSkills.ts	Exposes Skills state to React.
core/mcp/state/managedSkillsState.ts	Manages paginated Skills loading.
core/mcp/state/index.ts	Exports Skills state types.
core/mcp/skillsSchemas.ts	Defines Skills wire schemas.
core/mcp/skills.ts	Implements conformance and verification logic.
core/mcp/inspectorClientProtocol.ts	Adds Skills client contracts.
core/mcp/inspectorClient.ts	Implements Skills requests.
core/mcp/__tests__/fakeInspectorClient.ts	Extends the test client for Skills.
clients/web/src/utils/skillFileBytes.ts	Converts resource payloads into bytes.
clients/web/src/utils/skillFileBytes.test.ts	Tests resource decoding.
clients/web/src/utils/inspectorTabs.ts	Registers the Skills tab.
clients/web/src/utils/inspectorTabs.test.ts	Tests Skills tab registration.
clients/web/src/test/core/react/useManagedSkills.test.tsx	Tests the React Skills hook.
clients/web/src/test/core/mcp/state/managedSkillsState.test.ts	Tests managed Skills state.
clients/web/src/test/core/mcp/skillsSchemas.test.ts	Tests Skills schemas.
clients/web/src/test/core/mcp/skills.test.ts	Tests conformance and digest helpers.
clients/web/src/test/core/mcp/inspectorClient-skills.test.ts	Tests Skills client requests.
clients/web/src/lib/oauthResume.ts	Persists Skills UI state.
clients/web/src/lib/oauthResume.test.ts	Tests OAuth resume state.
clients/web/src/hooks/useTabUiState.ts	Adds lifted Skills tab state.
clients/web/src/hooks/useServerCommands.tsx	Adds Skills read and refresh commands.
clients/web/src/hooks/useServerCommands.test.tsx	Tests Skills commands.
clients/web/src/hooks/useInspectorStores.ts	Creates and exposes the Skills store.
clients/web/src/hooks/useInspectorStores.test.tsx	Tests Skills store integration.
clients/web/src/components/views/InspectorView/types.ts	Defines Skills panel properties.
clients/web/src/components/views/InspectorView/InspectorView.tsx	Gates and renders the Skills tab.
clients/web/src/components/views/InspectorView/InspectorView.test.tsx	Tests Skills tab visibility.
clients/web/src/components/views/InspectorView/InspectorView.stories.tsx	Adds Skills view story data.
clients/web/src/components/screens/SkillsScreen/SkillsScreen.tsx	Implements the Skills conformance UI.
clients/web/src/components/screens/SkillsScreen/SkillsScreen.test.tsx	Tests Skills interactions and verdicts.
clients/web/src/components/screens/SkillsScreen/SkillsScreen.stories.tsx	Adds Skills screen stories.
clients/web/src/components/screens/screenUiState.ts	Registers default Skills UI state.
clients/web/src/components/groups/ConnectionInfoContent/ConnectionInfoContent.tsx	Displays Skills capability details.
clients/web/src/components/groups/ConnectionInfoContent/ConnectionInfoContent.test.tsx	Tests Skills capability presentation.
clients/web/src/App.tsx	Connects Skills state and commands.
clients/web/README.md	Documents Skills automation attributes.

Review details

Suppressed comments (5)

clients/web/src/components/screens/SkillsScreen/SkillsScreen.tsx:321

• This completion is not tied to the selection that started it. If the user selects skill B before skill A's read resolves, the render-time reset runs first and this callback then repopulates B's pane with A's SKILL.md (the rejection path has the same race). Tag the request/result with the selected URI or discard it when it is no longer current.

    void onReadSkillFile(selected.uri)
      .then((contents) => {
        setPreview(contents);
        setPreviewError(null);
      })

clients/web/src/components/screens/SkillsScreen/SkillsScreen.tsx:288

• Fetching and hashing SKILL.md never performs the required field-by-field comparison between its YAML frontmatter and selected.frontmatter. A server can therefore advertise one description/metadata object, serve different frontmatter with a matching digest, and still be shown as Conforms/verified. Parse the fetched top-level SKILL.md and surface a verification failure for any discrepancy.

        const contents = await onReadSkillFile(resource.uri);
        const verification = await verifySkillResource(
          resource,
          skillFileBytes(contents),
        );

clients/web/src/components/screens/SkillsScreen/SkillsScreen.tsx:292

• A verification started for the old manifest can resolve after selection or list refresh has invalidated the state, and this continuation then writes the stale verdict back under the resource URI. Tag each request with the current selection/manifest revision and discard both success and failure completions when that revision has changed.

        const contents = await onReadSkillFile(resource.uri);
        const verification = await verifySkillResource(
          resource,
          skillFileBytes(contents),
        );
        setFileStates((prev) => ({
          ...prev,
          [resource.uri]: { status: "done", verification },
        }));

clients/web/src/components/views/InspectorView/types.ts:315

• The old “Tasks monitor” doc comment now attaches to SkillsPanelProps, while TasksPanelProps loses its description. Move that comment immediately above the Tasks interface so generated/editor documentation describes the correct API.

/** The Skills screen (SEP-2640): the enumerated skills and their verification. */

core/mcp/skills.ts:334

• Verification ignores the declared byte size. SEP-2640 requires a fetched file whose byteLength differs from resource.size to fail verification equivalently to a digest mismatch, so a server can currently advertise an incorrect size and still receive a verified verdict. Compare bytes.byteLength before hashing and surface expected/actual sizes.

• Files reviewed: 42/42 changed files
• Comments generated: 9
• Review effort level: Balanced

[cliffhall]

Review round 1 — all nine findings addressed (da8cc16)

Every one of these was right; eight are fixed and one is deferred with the reason stated in the source. Mirrored here at PR level because inline replies go hidden once the fix is pushed.

Manifest completeness — core/mcp/skills.ts

Fixed. Conforms really was reachable for manifests that break invariants the SEP states. Four new findings:

Code	Severity	Catches
manifest-missing-self	error	A manifest without the skill's own SKILL.md — resources: [] included, since an empty list is that case
duplicate-resource	error	The same URI listed twice
resource-outside-skill-root	error	An entry outside skill://<path>/, which relative references resolve against
missing-size	warning	No size, so the entry is excluded from the 16 MiB total and its length cannot be cross-checked

resource-outside-skill-root is skipped when the entry URI is itself malformed — there is no root to measure against and malformed-uri already reports that, so checking anyway would present one defect as many. missing-size is a warning rather than an error because the digest still verifies the bytes; what is lost is the cross-check below.

This changed the test fixtures: several of them omitted their own SKILL.md and were being asserted as clean, which was the bug in miniature.

Declared size ignored during verification

Fixed, and checked before hashing. Your reasoning is what makes it more than a nicety: the digest is taken over the bytes the server served, so agreeing with it says nothing about whether the manifest describes those bytes. verifySkillResource now returns mismatch with expectedSize / actualSize and a reason naming both, and a verified result echoes them too. Checking first also means a 16 MiB file that was never going to verify is not hashed.

sha256Digest and SharedArrayBuffer

Fixed, and you are right that the cast was worse than useless — SharedArrayBuffer.prototype.slice() returns another SharedArrayBuffer, so the function would have thrown for the exact input the comment claimed it handled. Now new Uint8Array(bytes), which always allocates a plain ArrayBuffer and copies only the view's range. The cast is gone entirely; the existing test that hashes a subarray into a larger buffer still pins the range behavior.

Unbounded pagination — managedSkillsState

Fixed with SKILLS_MAX_PAGES = LIST_MAX_PAGES, imported from listSalvage rather than re-declared, so the two pagination paths in this repo cannot drift. The repeated-cursor guard and the cap catch genuinely different shapes — one cursor forever versus an endlessly unique one — so both stay. The cap raises rather than truncating, for the reason listPaginationExceeded documents: returning what we have would present a partial list as a complete one. Test asserts the throw, the call count, and that nothing is committed.

Stale continuation after disconnect / destroy

Fixed with a session generation, advanced by reset() and destroy(). Every write is gated on it. The rejection is still re-thrown when the session has moved on — the caller's auth-recovery wrapper keys off it — but the state is left alone, so a dead session's failure cannot surface in the live one. Two tests: a resolve and a reject, each landing after a disconnect.

Empty-string cursor — inspectorClient.listSkills

Fixed: cursor !== undefined, not truthiness. The failure mode you describe is the nasty part — dropping "" re-requests page one, and the store then reports a repeated-cursor error, so a conforming server is made to look broken.

Stale verdicts across a manifest refresh — SkillsScreen

Fixed. Invalidation is keyed on a manifest signature (selected URI + each entry's URI, digest and size) rather than the URI alone, so a Refresh that changes the manifest drops the verdicts. Keyed as a primitive string because useValueChange compares with Object.is and a fresh array every render would loop. New test: verify, then re-render the same skill with a different digest, and assert the green badge is gone.

Verification / preview continuations racing the selection

Fixed with an epoch ref, bumped by the same invalidation. Both arms of the SKILL.md read and both arms of verifyFile discard their result when the epoch has moved on. Three tests hold a read open, switch skills, then resolve or reject it.

Verify all concurrency

Fixed: four workers pulling from a shared cursor, so a 512-entry manifest is 4 in-flight reads and each row still flips to checking… and then to its verdict as it lands, rather than all at once.

resources/directory/read — schema and fixture

Both retracted rather than corrected, which I think is the right call for a surface nothing calls yet.

• The schema is removed. You may well be right that it is resources and not contents — I did not verify either against the normative text, and that is the problem: an unexercised schema in the one module that is supposed to be the authority on the wire format could sit wrong indefinitely without failing anything. A comment now says so, and Skills extension phase 3: CLI methods, TUI pane, resources/directory/read, and paginated mode #2248 adds it alongside the call that uses it.
• The fixture declares the extension bare. It advertised directoryRead: true with no handler, so Connection Info reported "Supported" for a method that answers -32601. directoryRead stays available as a config option; skills-http.json just does not claim it. The Connection Info screenshot in the PR body is re-captured and now reads "Not supported" — which still demonstrates the row.

Tasks doc comment

Fixed — restored above TasksPanelProps.

Frontmatter field-by-field comparison — deferred, deliberately

The one I have not fixed, and your framing of it is correct: the digest cannot cover it, because a digest proves the file was not altered in transit and says nothing about whether the listing described that file honestly. A server can advertise one description, serve another, and pass every check here.

Closing it needs a YAML parser. There is no YAML dependency in this repo's runtime surface today, and adding one is a [Dependency placement] decision of its own rather than something to slip into this PR. So it is tracked on #2248, and — more importantly — the gap is now stated in core/mcp/skills.ts's module header, where someone reading Conforms will find it, instead of being silently absent.

---

npm run local:gate is green. New per-file coverage still clears ≥90 on all four dimensions (SkillsScreen.tsx 98.4 / 95.6 / 100 / 100).

[Copilot]

🟡 Changes recommended

Five unresolved moderate findings affect verification identity, size validation, schema compliance, and reconnect behavior.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (5)

Previously missed (1) — in code that hasn't changed since the last review.

core/mcp/skillsSchemas.ts:140

• The accepted SEP now specifies the skills/get result as an envelope containing skill; it is no longer ambiguous. Accepting an inline entry silently normalizes a non-conforming server response, which conflicts with this PR's conformance-checking purpose and the description's claim that both forms remain plausible. Require GetSkillEnvelopeSchema and update the inline-result test accordingly.

clients/web/src/components/screens/SkillsScreen/SkillsScreen.tsx:573

• verifySkillResource also returns mismatch for a size disagreement before hashing, with actualDigest unset and the explanation in reason. This branch labels that case “Digest mismatch” and renders actual undefined, hiding the actual size failure. Render the supplied reason for non-digest mismatches instead.

                          title="Digest mismatch"

clients/web/src/components/screens/SkillsScreen/SkillsScreen.tsx:618

• onReadSkillFile explicitly supports blob content, but the preview drops preview.blob and substitutes an empty string whenever text is absent. A server returning a base64 SKILL.md therefore shows a blank preview even though verification reads the correct bytes. Pass a text/blob contents object to ContentViewer so its resource-content path performs the decoding.

                  <ContentViewer
                    block={{ type: "text", text: preview.text ?? "" }}
                    mimeType={preview.mimeType ?? "text/markdown"}
                    copyable

clients/web/src/components/screens/SkillsScreen/SkillsScreen.tsx:305

• useValueChange runs this callback during render and explicitly requires setState-only purity; mutating epoch.current here escapes React. If concurrent React abandons this render, the ref increment remains and an in-flight verification for the still-committed selection is silently discarded. Keep the generation in React state and gate functional state updates by the manifest key instead of mutating a ref during render.

  useValueChange(manifestKey, () => {
    epoch.current += 1;
    setFileStates({});
    setPreview(null);
    setPreviewError(null);

core/mcp/state/managedSkillsState.ts:155

• A reconnect can permanently miss its skills load. If disconnect occurs while a walk is awaiting listSkills, reset() advances the generation but leaves running true; the reconnect-triggered refresh then returns here, and when the stale walk eventually clears running there is no retry (or it can stay blocked forever if that request hangs). Track the in-flight generation/token instead of one boolean so a new session may start, and ensure a stale finally cannot clear the new session's guard.

• Files reviewed: 42/42 changed files
• Comments generated: 3
• Review effort level: Balanced

[cliffhall]

Review round 2 — all eight findings addressed (bfeebd4)

Two of these caught bugs that round 1's own fixes introduced, which is the useful kind of second pass.

Duplicate URIs share one verdict, and one React key

Fixed. This one is on me from round 1: I added duplicate-resource so the screen reports a duplicated URI, then kept keying verdicts by URI — so the screen would report the duplicate while quietly collapsing the two rows it was reporting about. Verdicts and <Table.Tr> keys are now the manifest row index; the URI is still what the request is made with. New test declares the same URI twice with different digests and asserts one row reads verified and the other mismatch — a shared key made both show whichever landed last.

epoch.current += 1 inside useValueChange

Fixed, and you are right that it broke the hook's stated contract — which I quoted in the same file, so this was a self-inflicted one. The ref is gone. Both slices now carry the manifest key they belong to:

const [verification, setVerification] = useState<VerificationState>({ key: null, files: {} });

useValueChange is back to setState calls only, and every async continuation writes through a functional update comparing the key it started under. key: null covers the case that made a ref tempting — useValueChange deliberately does not fire on the first render, so the first write adopts the initial manifest; after that only an invalidation replaces the key, so a stale continuation can never be mistaken for an initial one.

Size mismatch rendered as "Digest mismatch — actual undefined"

Fixed, and again a defect round 1 created: I added the pre-hash size check and did not update the alert that reads its output. The alert now titles it Size mismatch and renders verification.reason when there is no actualDigest. Test asserts the title, the absence of "Digest mismatch", and the byte counts.

Preview drops blob

Fixed. ContentViewer now gets a contents object rather than a text block, so its resource-content path does the decoding. The mismatch you spotted is the sharp part: onReadSkillFile returns blob content and skillFileBytes verifies it correctly, so the screen would confirm a file's digest and then paint an empty preview of it. Test reads a base64 SKILL.md and asserts the text renders.

size accepted as an arbitrary number

Fixed with malformed-size (error) for anything that is not a non-negative safe integer, and totalSkillBytes now excludes those values. The negative case is why it is an error rather than cosmetic — summing it pulls the total back under 16 MiB and hides a real size-limit-exceeded. There is a test for exactly that: a manifest one byte over the limit plus a -1000 entry still reports the violation.

Reconnect blocked by a stale walk

Fixed, and this was the worst of the eight — a permanently empty Skills tab after a reconnect, and unrecoverable if the stale request never settled. The boolean is replaced by runningGeneration: number | null:

• a walk starts unless one is already in flight for the current generation, so a new session never waits on a dead one;
• the finally clears the guard only when it still holds the generation it set, so a stale walk settling later cannot release the live session's guard.

Two tests: a reconnect while the first walk hangs forever (asserts the fresh skills arrive), and a stale walk settling mid-flight of a live one (asserts the guard survives).

skills/get envelope

Tightened to the envelope only, taking your reading of the accepted SEP. The argument that decided it is the one you made: accepting the inline form silently normalizes a non-conforming response, which is precisely what this extension's support exists to report — and it is the same reasoning I accepted last round for the directory-read schema, so being permissive here would have been inconsistent. GetSkillResultSchema is now GetSkillEnvelopeSchema.transform(r => r.skill); an inline entry throws. Tests inverted accordingly, and normalizeGetSkillResult is gone since there is nothing left to normalize.

If that reading turns out to be wrong, this fails loudly the first time a real server returns the other shape — which is the failure mode I want over a permissive union that hides it.

#2248 and the PR description contradicted the schema removal

Fixed — a straight documentation error on my part, and thank you for catching that the follow-up issue was claiming a prerequisite round 1 had just deleted. Both are corrected:

• Skills extension phase 3: CLI methods, TUI pane, resources/directory/read, and paginated mode #2248 now says the result schema is its work, defined against the normative text alongside the call that uses it, and notes that skills-http.json declares the extension bare until the fixture actually serves the method. It also now records the frontmatter cross-check gap explicitly, rather than leaving it only in a source comment.
• The PR description no longer claims this module is "the whole wire surface". It is the wire surface for the two methods this PR calls, and it says why the third is deliberately absent.

---

npm run local:gate is green. SkillsScreen.tsx coverage is 99.2 / 96.4 / 100 / 100.

[Copilot]

🟡 Changes recommended

Critical URI-containment and multiple moderate correctness and concurrency issues remain unresolved.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (2)

clients/web/README.md:99

• This now names four screens, but the following sentence still says the smoke script “drives all three,” while the new Skills row correctly says Skills is not driven yet. Reword this paragraph to distinguish the four contracts from the three currently exercised by the smoke.

The Tools, Resources, Prompts and Skills screens each expose a `data-testid` plus a
small set of `data-*` attributes, so a headless driver can `waitForSelector` on
a deterministic signal rather than on visible copy. `scripts/smoke-web-tabs.mjs`
drives all three against `test-servers/configs/web-tabs-http.json` ([#2148](https://github.com/modelcontextprotocol/inspector/issues/2148)).

test-servers/src/skills.ts:257

• This extension does not need the private _requestHandlers escape hatch: unlike modern Tasks, the SDK supports consumer-owned methods through public setRequestHandler overloads when explicit schemas are supplied. Depending on an underscored SDK field can break this fixture on an SDK update and also bypasses request/result validation; register these methods through the public API instead.

• Files reviewed: 42/42 changed files
• Comments generated: 8
• Review effort level: Balanced

[cliffhall]

Review round 3 — all ten findings addressed (6a08c77)

URI containment missed .. traversal — core/mcp/skills.ts

Fixed, and this was the worst finding of the three rounds: skill://acme/billing/refunds/../other.md starts with the advertised root as a string and resolves outside it, so a manifest escaping its own skill was being reported as clean by the check written to catch exactly that.

A new normalizeSkillUri parses and normalizes before any comparison, and both the root and every entry go through it. It closes two neighbouring holes you named while I was in there:

• A relative string — demo/SKILL.md — fails to parse, so it is malformed-uri rather than something quietly treated as a skill path. skillNameFromUri reads off the normalized URI too, so a traversal segment cannot yield a name the resolved path does not carry.
• An opaque-path URI (skill:demo/../x.md, no authority) parses but is not normalized by the WHATWG parser — its .. survives verbatim — so it is rejected as well. Accepting it would have reopened the hole one layer down.

An unparseable manifest entry is now resource-outside-skill-root rather than skipped: nothing can establish that a non-URI is inside a root.

missing-description classified as a warning

Fixed — now an error. Your framing is the decisive part: a mandatory format violation showing as "0 errors" in the conformance summary is worse than not checking it, because it reads as an affirmative pass.

Decode rejections not attributed to the Protocol entry

Fixed, and thank you for pointing at managedListState.ts:317 — this store is deliberately not a subclass of that base, and the cost of that decision is exactly this: shared behavior has to be re-adopted by hand, and I missed one. skills/list now calls markResponseRejected under isClientDecodeRejection, so an invalid result stops rendering as a clean success in the Protocol tab. Two tests, including the negative one that guard exists for: a transport failure must not be attributed, because no response frame arrived and the last-answered id still points at an earlier successful call.

Two verifications of the same row

Fixed with a per-row attempt token. You are right that round 2's manifest key cannot see this: same row, same manifest, so the only thing distinguishing the two reads is which click started them. FileState now carries attempt, claimed synchronously before the read, and a write is dropped when a higher attempt already landed. Test starts two reads on one row, answers the second first with matching bytes and the first second with bytes that would verify as a mismatch, and asserts the row still reads verified.

Repeated "Verify all" stacks worker pools

Fixed. The button is disabled (and shows a loading state) until the batch settles, so the cap of four is a cap on total in-flight reads rather than per click.

Colliding keys on repeated findings

Fixed — the key now leads with the finding index. Three identical URIs produce two duplicate-resource findings with identical code and URI, and React was free to drop the extras. The failure mode is the pointed one: the view would hide findings in precisely the malformed input it exists to inspect. Test renders a manifest with the same URI three times and asserts both findings appear.

_requestHandlers escape hatch in the fixture

Fixed, and you are right that I over-generalized from modern-tasks.ts. The two cases are not the same: tasks/* are spec names the 2026 codec deleted, which is what forces the raw seam; skills/* are in neither codec, and setRequestHandler accepts a consumer-owned method given explicit schemas — which #2234's own analysis says, so I had the answer in the issue and did not use it.

skills/list and skills/get are now registered publicly with Zod param schemas, so params are validated on the way in and an SDK update cannot quietly break the fixture.

One use remains, and it is a wrap rather than a registration. resources/read has to answer skill:// URIs while leaving every other URI to the SDK's handler, and setRequestHandler replaces rather than chains — there is no public "extend this method" API. So the existing handler is captured and delegated to, and the module header now says that specifically instead of citing the tasks fixture. Verified end to end against the real fixture after the change: skills/list paginates, skills/get answers, and both files verify.

Unknown skills/get URI returned a generic failure

Fixed — ProtocolError(ProtocolErrorCode.InvalidParams, …), so it is a real -32602. Worth fixing precisely because this is a conformance fixture: it is meant to be non-conforming in three documented ways and correct everywhere else, and a stray generic failure quietly added a fourth.

README said "drives all three" while naming four

Fixed — the paragraph now distinguishes the three screens the smoke drives from the fourth that publishes the contract but is not smoked yet, and links #2234 for it.

---

npm run local:gate is green. The screenshots in the PR body are re-captured against the rebuilt bundle and the public-handler fixture.

[Copilot]

🟡 Changes recommended

Six moderate conformance and functionality issues remain unresolved.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (5)

Previously missed (2) — in code that hasn't changed since the last review.

core/mcp/skills.ts:422

• SubtleCrypto is unavailable in non-secure browser contexts, but the Inspector explicitly supports plain-HTTP LAN hosting (clients/web/README.md:382-386). At http://192.168…, this throws for every file and the UI misreports hashing as a read failure. Provide a browser-safe SHA-256 fallback or handle/document an HTTPS requirement explicitly.
  core/mcp/skillsSchemas.ts:103
• skills/list is cacheable in protocol revision 2026-07-28: the accepted SEP requires ttlMs and cacheScope on its result. Because this is a consumer-owned method, the SDK has no method-keyed wire schema for it, so this loose schema currently accepts a modern response that omits both fields. Add era-aware validation for the modern result envelope (while retaining the legacy shape) so the Inspector does not silently accept a non-conforming response.

clients/web/src/components/screens/SkillsScreen/SkillsScreen.tsx:345

• batchRunning is global rather than keyed to manifestKey. If the user changes skills while “Verify all” is running, the new skill remains loading/disabled until every old request settles; one hung old read blocks bulk verification indefinitely. Track the active batch by manifest key/generation and let stale finalizers clear only their own batch.

  useValueChange(manifestKey, (next) => {
    setVerification({ key: next, files: {} });
    setPreviewState({ key: next });
  });

core/mcp/skills.ts:332

• size is also a required field for every manifest entry, not an optional integrity hint. Reporting a missing value as only a warning lets a server evade the 16 MiB pre-fetch limit while the UI still reports zero conformance errors; this should be an error like other violated MUST requirements.
  core/mcp/skills.ts:314
• A digest is required on every manifest resource by SEP-2640, so classifying its absence as a warning makes an invalid entry show “0 errors.” This also conflicts with SkillIssueSeverity's contract that requirement violations are errors; keep the permissive wire schema, but report this finding as an error.

• Files reviewed: 42/42 changed files
• Comments generated: 3
• Review effort level: Balanced

[cliffhall]

Review round 4 — six fixed, two declined with reasons (1cb05ca)

skills/get had no production caller

Fixed, and this is the most important finding of the four rounds: I built the client method, tested it, described it in the PR — and nothing in the app called it. #2234's acceptance criterion was unmet and a server author's skills/get handler could not be exercised at all, which is most of the point of an inspector.

The detail pane now has Fetch with skills/get, and it does more than display the result: it compares the fetched entry field-by-field against the one skills/list advertised and reports whether they agree. Both describe the same skill, so a disagreement is a server bug that only a side-by-side fetch can surface — which makes the call worth making rather than a box to tick. Loading, error and result states are all rendered, all keyed to the manifest so a late answer cannot land under another skill. Screenshot added to the PR body; the Protocol panel in it shows the skills/get exchange.

crypto.subtle is unavailable over plain HTTP

Fixed, and thank you for chasing this into README.md#hosting-on-a-network — HOST=192.168.1.50 is a documented, supported deployment, and there crypto exists while crypto.subtle does not. Every verification would have thrown, and the UI would have reported "Could not read file" for files it had fetched perfectly well: a wrong diagnosis, not just a missing feature.

core/mcp/sha256.ts is a dependency-free FIPS 180-4 implementation used only when crypto.subtle is absent; WebCrypto is still preferred wherever it exists. Adding a dependency for ~60 lines would have meant a root declaration plus three bundler external lists, per [Dependency placement]. It is checked two ways, because "a hand-rolled hash" deserves more than a smoke test: the published FIPS vectors (empty, abc, the two-block case), and a differential check against WebCrypto across the padding boundaries (0/1/55/56/63/64/65/200/1000 bytes). There is also a test that stubs crypto down to no subtle — the exact shape a plain-HTTP page presents — and asserts the digest still comes back and matches.

normalizeSkillUri accepted any hierarchical URI

Fixed — the skill: scheme is now required. https://demo/SKILL.md was passing both the name check and the root check, so a manifest pointing anywhere on the web could be reported as conforming. That is the same class as the .. traversal from round 3, one layer up, and I should have closed it in the same change.

missing-digest and missing-size as warnings

Both promoted to errors. Your argument settles it and generalizes: SkillIssueSeverity promises that a violated MUST is an error, so classifying a required field's absence as a warning let an invalid manifest report 0 errors — an affirmative pass this checker must never give. The size case is worse than cosmetic, as you note: an omitted size is excluded from the sum, which is precisely how a server slips past the 16 MiB pre-fetch limit.

The severity doc now says what the split actually means: error = a stated requirement broken; warning = legal but leaves integrity unverifiable, which after this change is "dynamic" and nothing else.

batchRunning not keyed to the manifest

Fixed — the state holds the batch's manifest key, the button is disabled only for that manifest, and a finalizer clears only its own batch. Test selects a second skill while the first skill's reads hang forever and asserts its Verify all is live.

---

Declined, with reasons

ttlMs / cacheScope on a modern skills/list result (and in the fixture)

Not changed. This is the one place where acting would encode an unverified spec claim into a hard rejection.

#2234's own analysis records the SEP-2549 caching attributes as an open point; this review asserts the accepted SEP requires them on skills/list. I have not read the normative text, and neither had the source of either claim. The two errors are not symmetric:

• leaving the schema permissive means a server that omits them is accepted — and looseObject already passes both fields through untouched when a server does send them, so nothing is lost;
• tightening on a wrong reading means rejecting conforming responses, which fails servers that are doing nothing wrong.

That is the same reasoning that removed the guessed resources/directory/read schema in round 1, and being permissive here while being strict on the skills/get envelope in round 2 is not inconsistent: there, the review reported the shape as settled and the strictness only rejects a shape the fixture does not produce; here, the two available readings directly contradict each other.

So: ListSkillsResultSchema now states the open question and why it is not guessed, and #2248 owns settling it against the spec — with the note that the SDK is no help either way, since a consumer-owned method is absent from its cacheable-method registry and nothing stamps or validates these fields. Same reasoning for not stamping them in the fixture: I will not have the test server assert a requirement I cannot verify.

If you can point at the SEP line that settles it, I will make the change in this PR rather than the follow-up.

---

npm run local:gate is green. SkillsScreen.tsx coverage is 98.6 / 94.4 / 100 / 100. Screenshots re-captured against the rebuilt bundle, with the new skills/get shot added.

[Copilot]

🟡 Changes recommended

Multiple unresolved moderate issues affect SEP-2640 conformance, OAuth recovery, and advertised server capabilities.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (4)

Previously missed (3) — in code that hasn't changed since the last review.

core/mcp/skillsSchemas.ts:106

• The accepted SEP now requires modern-era skills/list results to carry the base list envelope (resultType, ttlMs, and cacheScope). Because this consumer-owned method bypasses the SDK codec, this permissive schema currently accepts an invalid modern response. Select an era-aware result schema here just as the existing modern list paths validate ModernResultEnvelopeSchema.
  core/mcp/skillsSchemas.ts:131
• The open question for skills/get is only whether it also carries ttlMs/cacheScope; the accepted modern response still has the required resultType: "complete". Since the SDK does not codec-check this extension method, requiring only { skill } accepts a malformed modern result (and the new fixture emits that malformed shape). Make this envelope era-aware and require resultType on modern connections.
  test-servers/src/composable-test-server.ts:844
• directoryRead: true is publicly accepted by the config and reaches this branch, but wireSkillsHandlers never registers resources/directory/read. Such a config therefore advertises support and then returns Method not found, exactly the false capability the comment says to avoid. Reject/remove this option until phase 3 or implement the handler before advertising it.

test-servers/src/skills.ts:223

• This response omits the modern list envelope even though the fixture is documented as working in either era. On a 2026-07-28 connection SEP-2640 requires resultType, ttlMs, and cacheScope; because this is a consumer-owned method, the SDK will not add them. Return the modern envelope when serving the modern era so this fixture is conforming.

• Files reviewed: 44/44 changed files
• Comments generated: 5
• Review effort level: Balanced

[Copilot]

🔵 Needs a closer look

Multiple moderate correctness, authentication, validation, and race-condition issues remain unresolved.

Review details

Suppressed comments (9)

Previously missed (2) — in code that hasn't changed since the last review.

clients/web/src/components/screens/SkillsScreen/SkillsScreen.tsx:468

• Repeated “View SKILL.md” clicks have the same last-completion-wins race: an older read that finishes last replaces the newer preview because selection/manifest keys cannot distinguish attempts within one manifest. Track an attempt token or disable the action while pending.

This issue also appears on line 484 of the same file.
core/mcp/skillsSchemas.ts:106

• This is no longer unsettled in the accepted SEP: for protocol version 2026-07-28 and later, every skills/list page additionally carries SEP-2549's ttlMs and cacheScope. Keeping one permissive schema accepts a modern response that omits or malforms required cache fields, contrary to this module's role as the wire validator. Add era-aware modern validation while retaining the legacy shape for older connections.

clients/web/src/components/screens/SkillsScreen/SkillsScreen.tsx:492

• Repeated “Fetch with skills/get” clicks for the same manifest are not ordered. If the older request resolves after a newer one, it overwrites the newer snapshot because both share the same key. Add a request-attempt token (as verifyRow does) or disable the action while its request is pending.

    void onGetSkill(selected.uri)
      .then((entry) => {
        // Compared field-by-field against what `skills/list` advertised. The
        // two describe the same skill, so a disagreement is a server bug that
        // only shows up when both are fetched.
        const agrees = JSON.stringify(entry) === JSON.stringify(selected);
        setFetchedEntry((prev) =>
          prev.key !== null && prev.key !== key ? prev : { key, entry, agrees },
        );

clients/web/src/components/screens/SkillsScreen/SkillsScreen.tsx:490

• JSON object member order is not semantically significant, and the SEP describes resources as a set, but JSON.stringify is sensitive to object-key and array order. A conforming skills/get response that serializes frontmatter keys—or the same resource set—in a different order will be shown as a server bug. Compare the entry structurally, with order-insensitive object fields and resource-set semantics.

        // Compared field-by-field against what `skills/list` advertised. The
        // two describe the same skill, so a disagreement is a server bug that
        // only shows up when both are fetched.
        const agrees = JSON.stringify(entry) === JSON.stringify(selected);
        setFetchedEntry((prev) =>

clients/web/src/hooks/useServerCommands.tsx:987

• This new server command bypasses the auth-recovery path that the hook requires every command to use. If skills/get returns an AuthRecoveryRequiredError, the Skills screen only shows a generic failure and neither applies stored credentials nor initiates reauthorization. Route the request through the same command-scoped resource recovery used by resources/read.

  const onGetSkill = useCallback(
    async (uri: string): Promise<SkillEntry> => {
      if (!inspectorClient) throw new Error("Client is not connected");
      return inspectorClient.getSkill(uri);
    },
    [inspectorClient],

core/mcp/skills.ts:127

• SEP-2640 only recommends skill://; it explicitly allows server-native schemes such as github://owner/repo/skills/refunds/SKILL.md and says no scheme is privileged. This check therefore marks a conforming custom-scheme entry as malformed-uri and also prevents its root/resource checks from running. Normalize any hierarchical absolute URI and enforce the /SKILL.md/parent-name constraints independently of the scheme.
  core/mcp/skills.ts:257
• The SEP defines these as interoperability limits: hosts MUST support up to them, but servers only SHOULD NOT exceed them and hosts MAY support larger skills. Reporting an over-limit entry as an error contradicts the stated invariant above that errors correspond to violated MUST requirements and makes a legal larger skill look non-conforming. These two limit findings should be warnings (or otherwise labeled as portability warnings), including the size-limit branch below.
  docs/test-servers.md:76
• The dynamic skill is legal under SEP-2640, so only two of these entries are non-conforming. Reword this as three edge cases (one unverifiable dynamic case and two conformance violations) to avoid documenting a supported wire form as invalid.
  test-servers/src/skills.ts:12
• resources: "dynamic" is explicitly a conforming SEP-2640 form for generated content; it is unverifiable, not non-conforming. Calling all three edge cases non-conforming makes this fixture's contract inaccurate. Distinguish the legal dynamic warning from the two actual violations.

• Files reviewed: 44/44 changed files
• Comments generated: 0 new
• Review effort level: Balanced

[Copilot]

🟡 Changes recommended

Moderate correctness issues remain in URI normalization, name validation, and modern skills/list schema enforcement.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (3)

Previously missed (1) — in code that hasn't changed since the last review.

core/mcp/skillsSchemas.ts:106

• The accepted SEP is no longer ambiguous here: for protocol version 2026-07-28 and later, skills/list must carry the base list envelope, including ttlMs and cacheScope (and resultType). Because this loose schema is used for every era, a modern server can omit those required fields and still appear successful in this conformance UI. Select an era-aware schema in InspectorClient.listSkills, combining the modern result envelope with this page schema while retaining the permissive legacy shape.

clients/web/src/test/core/mcp/inspectorClient-skills.test.ts:49

• This double cast bypasses the type system without the inline safety justification required for as unknown as. Since the test intentionally reaches private SDK state, document why this structural view is safe and why the public API cannot provide the same isolated setup.

    return client as unknown as SkillsInternals;

core/mcp/skills.ts:267

• This only rejects an absent/blank description, but the Agent Skills specification limits description to 1–1024 characters. A 1025-character value therefore receives no finding and the UI can incorrectly report “Conforms”; add a malformed-description error and boundary coverage.

• Files reviewed: 44/44 changed files
• Comments generated: 3
• Review effort level: Balanced

[Copilot]

🟡 Changes recommended

A critical cross-session verification-state flaw and two moderate conformance issues remain unresolved.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

• Files reviewed: 44/44 changed files
• Comments generated: 3
• Review effort level: Balanced

[cliffhall]

Fixed in 4d7dd18, and this was a genuinely subtle one — thank you. You're right that the screen stays mounted across a disconnect, so content cannot distinguish server A's entry from an identical-looking one on B. useInspectorStores now exposes a sessionNonce, bumped on both createStores and destroyStores so it never repeats across a reconnect, and App keys the screen on ${activeServerId}:${sessionNonce} — the server id alone would repeat on a reconnect to the same server, which is one of the crossings this is meant to prevent. Because the session is part of the same key the batch map uses, your second point (a retained entry leaving B's button disabled) falls out too; the test asserts both.

[cliffhall]

Fixed in 4d7dd18 — skillUriIdentity in both the selected lookup and the NavLink active check. This was the last raw-string URI comparison left after the previous round's cleanup, and the symptom you describe is the worst kind: the detail pane silently empties for a skill that is still right there, because the server re-spelled its URI. Test renders with a %53KILL.md selection against a canonically-spelled list and asserts the detail pane is still shown.

[cliffhall]

Still declined — eighth round for this one, and the reasoning is unchanged from round 5. The server half is done: the fixture stamps the full modern envelope (resultType / ttlMs / cacheScope), matching this repo's own ModernResultEnvelopeSchema. The client half stays permissive because making it strict means the Inspector rejects a modern server's response on a reading nobody in this thread has verified against the normative text — and the two mistakes are not symmetric: permissive accepts a server that omits the fields (and passes them through untouched when sent), while a wrong tightening fails servers that are doing nothing wrong. skillsSchemas.ts records the open question and why it is not guessed; #2248 owns settling it and names era-aware validation as the shape of the fix. The offer stands: quote the SEP line and I will make the change in this PR rather than the follow-up.

[cliffhall]

Reversing my decline — this is now implemented (11ac134). The latest round quoted the normative text, which is exactly what I said five rounds running I was waiting for: "In protocol versions 2026-07-28 and later, the result also carries … ttlMs and cacheScope." With the sentence in hand the asymmetry I was reasoning from no longer applies — it isn't a coin flip any more, so the tightening is right.

ModernListSkillsResultSchema is the page plus the base list envelope, with field shapes mirroring ModernResultEnvelopeSchema in listSalvage.ts (this repo's existing statement of a modern envelope, already applied to modern list results on the salvage path) so the two can't drift. InspectorClient.listSkills selects it from the negotiated era; the legacy schema stays permissive, since those are 2026-era attributes a legacy server has no business sending. Your point about why it matters is the one I'd underweighted: skills/* is consumer-owned, so it's absent from the SDK's cacheable-method registry and nothing else would ever catch it — a modern server answering { skills: [] } was reaching the conformance UI as a clean list.

Tests: modern rejects a page with no envelope, and rejects ttlMs: -1 / 0.5 / an unknown cacheScope; modern accepts a page carrying it; legacy still accepts a page without it. Plus two on InspectorClient proving the era actually selects the schema.

#2248 is narrowed accordingly — the skills/list half is settled here; what remains open there is only whether skills/get carries the same attributes, which the quoted sentence doesn't cover.

[Copilot]

🔵 Needs a closer look

Modern list results need era-aware validation, and description limits must count Unicode code points.

Review details

Suppressed comments (2)

Previously missed (1) — in code that hasn't changed since the last review.

core/mcp/skills.ts:297

• JavaScript's string.length counts UTF-16 code units, not Unicode characters. The Agent Skills limit is 1024 characters, and its reference validator uses Python len(description) (code points), so a valid description containing 600 non-BMP characters is incorrectly reported as malformed-description with length 1200. Count code points before applying and reporting the limit.

core/mcp/skillsSchemas.ts:116

• The accepted SEP now resolves the point this comment calls “unsettled”: its skills/list section states, “In protocol versions 2026-07-28 and later, the result also carries … ttlMs and cacheScope.” This schema therefore accepts a non-conforming modern response such as { skills: [] }, and the managed store/UI report it as a successful list. Please select an era-aware schema in listSkills so the modern path requires the base list-caching fields while the legacy path remains permissive.

• Files reviewed: 44/44 changed files
• Comments generated: 0 new
• Review effort level: Balanced

[cliffhall]

Round 14 — no new inline threads; both suppressed findings addressed (11ac134)

This round generated 0 new comments, so there was nothing new to reply to inline. Both suppressed items are done:

• ttlMs / cacheScope on modern skills/list — implemented, reversing five rounds of declining. The round quoted the normative sentence, which is precisely the condition I'd stated for making the change here rather than in the follow-up. Details on the existing thread.
• description length counted in UTF-16 code units — fixed. String.length counts code units, so 600 non-BMP characters measured as 1200 and a perfectly valid description was reported malformed-description. Now counted in code points, matching the Agent Skills reference validator's Python len(). Applied to the name length check too. This is a false positive against a conforming server, which is the failure direction this module works hardest to avoid — good catch.

#2248 is narrowed: the skills/list half of the caching-attribute question is settled here, and what remains open there is only whether skills/get carries the same attributes, which the quoted sentence doesn't cover.

On the gate

npm run local:gate is green on every stage this PR touches. The full run intermittently fails on one unrelated, untouched file timing out at the 5s default — AppRenderer.test.tsx and JsonObjectInput.stories.tsx have each done it, on a machine sitting at load average 45. Both pass in isolation, git diff origin/v2/main...HEAD --name-only matches neither, and the whole Storybook project (521 tests) passes on this branch when run on its own. Reporting it rather than quietly re-running until green.

[Copilot]

🔵 Needs a closer look

Multiple moderate issues remain in request ordering, protocol error reporting, and Skills UI semantics.

Review details

Suppressed comments (5)

Previously missed (3) — in code that hasn't changed since the last review.

clients/web/src/components/screens/SkillsScreen/SkillsScreen.tsx:532

• A newer preview request is not recorded until it settles, so if two clicks overlap and the older request resolves first, its contents are accepted and shown even though a newer request is already in flight. Claim the attempt immediately (and clear the previous preview) before starting onReadSkillFile, as verifyRow does, so every older continuation is rejected regardless of completion order.

This issue also appears on line 552 of the same file.
clients/web/src/components/screens/SkillsScreen/SkillsScreen.tsx:631

• SEP-2640 explicitly permits skills/list to return an empty or partial catalog and says an empty result must not be treated as proof that the server has no skills. Rendering “No skills” makes that false claim to the user; label this as “No skills listed” (or “skills/list returned no entries”) so unlisted skills remain possible via skills/get.
  core/mcp/inspectorClient.ts:5614
• A malformed skills/get envelope is rejected by GetSkillResultSchema, but this path never calls markResponseRejected. The button shows an error while the Protocol tab leaves the response looking like a clean success. Mirror the skills/list decode-rejection handling so schema failures are attributed to the skills/get exchange while the correlation window is current.

clients/web/src/components/screens/SkillsScreen/SkillsScreen.tsx:552

• This has the same ordering hole as the preview request: attempt 2 is only written after it settles, so attempt 1 can resolve first and temporarily publish a stale skills/get verdict while attempt 2 is pending. Write the new attempt into state before issuing the request so the older callback is stale immediately.

    void onGetSkill(selected.uri)

clients/web/src/components/screens/SkillsScreen/SkillsScreen.tsx:793

• Every per-resource control has the same accessible name, Verify, and the URI cell is not programmatically associated with the button. Screen-reader users navigating controls cannot tell which file each button verifies; include the resource URI in an aria-label while keeping the visible text concise.

                                  Verify

• Files reviewed: 44/44 changed files
• Comments generated: 0 new
• Review effort level: Balanced

[cliffhall]

Round 15 — no new inline threads; all five suppressed findings fixed (a561cd1)

Again 0 new comments, so nothing new to attach an inline reply to. All five suppressed items were real and unaddressed:

• Attempt claimed only on settle (preview and skills/get) — fixed. This was a genuine ordering hole: recording the attempt when the request resolves leaves a window where an older request that happens to answer first is still considered current and publishes while a newer one is in flight. verifyRow already claimed its row up front; these two didn't. Both now claim before the request goes out, which also clears the stale result rather than leaving the old file on screen during the new read. Tests answer the older request first and assert nothing is published.
• "No skills" — now "No skills listed". SEP-2640 lets a server return an empty or partial catalog and says an empty result is not proof it has none, so the old copy had the tool asserting something the protocol explicitly does not. The test asserts the new string and the absence of the old claim.
• Row buttons all named "Verify" — each now carries aria-label={Verify ${resource.uri}}. The URI cell is in the same row but not programmatically associated with the button, so a screen-reader user had no way to tell the controls apart. The row-verify tests now address the button by that accessible name, so the label is pinned by a test rather than only added.
• skills/get decode rejection not attributed — fixed with markResponseRejected, in the client rather than a store because skills/get has none (the screen calls it directly). Gated on isClientDecodeRejection, with the negative test that guard exists for: a transport failure must not be attributed, since no response frame arrived and the last-answered id still points at an earlier successful exchange.

npm run local:gate is green.

[Copilot]

🔵 Needs a closer look

Skills capability detection must reject invalid non-object extension declarations before exposing or invoking the extension.

Review details

Suppressed comments (3)

Previously missed (1) — in code that hasn't changed since the last review.

core/mcp/skills.ts:116

• A non-null primitive extension value (for example false or "skills") is currently treated as support, which exposes the Skills tab and sends skills/list even though SEP-2640 defines the declaration as an object. Reject non-object values here, matching the existing extension parsing in core/mcp/appElicitation.ts:52-55.

clients/web/src/components/screens/SkillsScreen/SkillsScreen.tsx:133

• This contract incorrectly says the two responses must agree and that a difference makes the server broken. SEP-2640 defines skills/get as a fresh point-in-time snapshot, so a conforming result may legitimately differ from an older listing; the implementation below already presents that case as an updated snapshot. Update this documentation to match the actual protocol semantics.

   * Re-fetch the selected entry through `skills/get` (SEP-2640). Distinct from
   * the entry `skills/list` already returned, and the point of exercising it is
   * that the two must agree: a server whose `skills/get` disagrees with its own
   * listing is broken in a way only a side-by-side fetch can show.

test-servers/src/skills.ts:383

• No automated real-transport test exercises these newly registered handlers: the Skills client tests stub client.request, and the screen tests mock callbacks. That leaves the integration-sensitive claims here—custom setRequestHandler, the private resources/read wrapper, pagination, and operation in both protocol eras—unguarded. Add a web integration test that connects to this fixture and calls skills/list, skills/get, and resources/read (including a modern-era connection).

• Files reviewed: 44/44 changed files
• Comments generated: 0 new
• Review effort level: Balanced

[cliffhall]

Round 16 — all three suppressed findings addressed (1ee9580)

No inline comments this round; the three suppressed findings were all valid and are all fixed.

1. getSkillsExtension accepted a non-object declaration. false or "skills" under capabilities.extensions["io.modelcontextprotocol/skills"] was read as support with no sub-options. SEP-2133 declares an extension as an object of sub-options, so a primitive is not a declaration — getSkillsExtension now returns undefined for one, matching how appElicitation.ts:52-55 already handles the same shape. The test that asserted the old behavior ("treats a non-object declaration as declared with no sub-options") is rewritten to assert the rejection.

2. The onGetSkill prop doc contradicted the implementation. It said the skills/get and skills/list entries "must agree" and that a difference means the server is broken. The code doesn't treat it that way — it renders the difference as a conformance finding, which is the whole point of the pane. Reworded to describe what it does.

3. No real-transport test exercised the fixture's handlers. Added clients/web/src/test/integration/mcp/inspectorClient-skills.test.ts, parameterized over legacy and modern. Everything else covering this feature stubs the seam it is about — the client unit tests replace client.request, the screen tests mock the callbacks, the store tests use a fake client — which left exactly the integration-sensitive claims unguarded: that a consumer-owned method can be served through the SDK's public setRequestHandler, that the fixture's resources/read wrapper answers skill:// while delegating everything else, that the cursor walk really pages, and that all of it holds on both eras (skills/* is in neither codec, which is why one fixture is expected to serve both legs). 14 tests, all passing.

One thing worth recording, since it changed what I could assert: I first wrote a standalone test reading resultType / ttlMs / cacheScope off the modern page. It can't work — listSkills narrows its return to { skills, nextCursor }, so those never reach the caller. The real assertion is that the modern listSkills resolves at all: it selects ModernListSkillsResultSchema from the negotiated era, and that schema rejects a page without the envelope, so a missing envelope surfaces as a rejection rather than as a missing property. The test now says so at the call.

npm run local:gate green.

[Copilot]

🔵 Needs a closer look

The broad protocol, cryptographic verification, state, and UI changes warrant final human review.

Review details

• Files reviewed: 45/45 changed files
• Comments generated: 0 new
• Review effort level: Balanced

[cliffhall]

Round 17 — clean

45/45 files reviewed, 0 new inline comments and no suppressed findings — the first fully clean round on this PR (round 16 was zero-inline but still carried three suppressed items, all since fixed in 1ee95809). Nothing to respond to and nothing to change.

The verdict is 🔵 Needs a closer look on the grounds that the protocol, digest-verification, state and UI surface is broad enough to warrant human review — that's a scope judgment about the change, not a finding against it.

Requesting one more round as the confirming pass, per the practice on this repo of not treating a single zero-comment round as the end.

[Copilot]

🟢 Approval recommended

The reviewed changes have no unresolved approval-blocking issues.

Review details

• Files reviewed: 45/45 changed files
• Comments generated: 0 new
• Review effort level: Balanced

[cliffhall]

Round 18 — 🟢 Approval recommended, second consecutive clean round

45/45 files reviewed, 0 new inline comments, nothing suppressed — the same as round 17, so this is the confirming pass and the review loop is finished.

Where it landed, across 18 rounds: every acceptance criterion on #2234 is met (phases 1 and 2), npm run local:gate is green, and the follow-up work that was deliberately deferred is tracked on #2248 — phase 3's CLI methods and TUI pane, resources/directory/read, and whether skills/get carries the same modern envelope attributes skills/list now requires (the SEP sentence quoted in round 14 covers only the list side).

The standing 🔵 from round 17 was a scope judgment — the protocol, digest-verification, state and UI surface is broad — not a finding, and round 18 resolves to approval on the same diff.