feat: Skills extension phase 3 — CLI, TUI, directory reads, frontmatter check
Closes #2248. Completes SEP-2640 support across all three clients, and closes the one obligation #2234 deliberately left open. - `checkSkillFrontmatterMatch` compares a served SKILL.md's own YAML frontmatter against the entry the listing advertised, field by field. No digest can cover this: a digest is taken over the bytes the server served, so it proves the file was not altered in transit and says nothing about whether the listing described it honestly. Needs a real YAML parser; `yaml` was already a root dependency, so this adds no package — but it does newly put it on core/'s import graph, hence the three bundler `external` lists. - `resources/directory/read`: result schemas defined against the normative text, `InspectorClient.readResourceDirectory`, a Directory section on the Skills screen, and `--method resources/directory/read` in the CLI. The client refuses the call locally when the server did not declare `directoryRead`, which is the SEP's MUST NOT. A child the directory lists but the entry does not is marked `not listed` rather than merged into the manifest — the SEP calls a directory result a live observation and forbids treating it as extending the manifest. - CLI: `skills/list`, `skills/get` and `resources/directory/read`, plus `--verify` — one NDJSON report per skill, a summary on stderr, exit 7 on a violation. Reads are sequential; a conforming manifest may declare 512 entries. - TUI: a Skills pane, shown only when the server declares the extension. Each row carries its conformance verdict as a glyph as well as a colour, since the pane is read over ssh and through script(1). Enter verifies. - `verifySkills` re-throws `AuthRecoveryRequiredError` rather than recording it per file: it says the session's authorization expired, so absorbing it would report N identical read failures and swallow the error the TUI and the web commands key off to reauthorize. Two open questions settled, both in code comments where they will be found: `skills/get` carries no caching attributes because SEP-2640 leaves the question open in as many words, so requiring them would fail a conforming server; and there is no `PagedSkillsState` because every consumer of this list is a whole-catalog verdict — computed over page one of three, "this server's skills conform" is not merely partial but wrong. The fixture grows two skills, both for checks that were otherwise undemonstrable: `lying-listing` (listing and file disagree, digest still verifies) and `stale-manifest` (serves a file its manifest does not declare). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ND46Ci64DC3tzBy9aG81Nc Signed-off-by: cliffhall <cliff@futurescale.com>
This commit is contained in:
cliffhall
parent
219a08e1b5
commit
9a8488681e
7 changed files with 4750 additions and 191 deletions
|
|
@ -145,9 +145,14 @@ import {
|
|||
} from "./modernTaskSchemas.js";
|
||||
import { buildClientExtensions } from "./extensions.js";
|
||||
import {
|
||||
DirectoryReadResultSchema,
|
||||
GetSkillResultSchema,
|
||||
ListSkillsResultSchema,
|
||||
ModernDirectoryReadResultSchema,
|
||||
ModernListSkillsResultSchema,
|
||||
RESOURCES_DIRECTORY_READ_METHOD,
|
||||
SKILLS_EXTENSION_KEY,
|
||||
type DirectoryReadResult,
|
||||
SKILLS_GET_METHOD,
|
||||
SKILLS_LIST_METHOD,
|
||||
type SkillEntry,
|
||||
|
|
@ -5648,6 +5653,77 @@ export class InspectorClient extends InspectorClientEventTarget {
|
|||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* One page of `resources/directory/read` (SEP-2640): the direct children of
|
||||
* a directory resource.
|
||||
*
|
||||
* **Gated on the server's own declaration, and the gate throws rather than
|
||||
* asks.** SEP-2640 is explicit that *"clients MUST NOT call
|
||||
* `resources/directory/read` against a server that has not declared
|
||||
* `directoryRead: true`"*, so this refuses locally instead of sending a call
|
||||
* the spec forbids and letting the server answer `-32601`. Refusing here also
|
||||
* keeps the Protocol tab honest: a request we were never allowed to make
|
||||
* should not appear in the exchange log as a server-side failure.
|
||||
*
|
||||
* Not recursive — the SEP says clients descend by calling the method again on
|
||||
* a child directory, so the walking is the caller's, not this method's.
|
||||
*/
|
||||
async readResourceDirectory(
|
||||
uri: string,
|
||||
cursor?: string,
|
||||
metadata?: RequestMetadata,
|
||||
): Promise<DirectoryReadResult> {
|
||||
if (!this.client) {
|
||||
throw new Error("Client is not connected");
|
||||
}
|
||||
const extension = this.getSkillsExtension();
|
||||
if (!extension?.directoryRead) {
|
||||
throw new Error(
|
||||
`Server did not declare directoryRead in ${SKILLS_EXTENSION_KEY}; ${RESOURCES_DIRECTORY_READ_METHOD} must not be called.`,
|
||||
);
|
||||
}
|
||||
const effectiveMeta = this.mergeMeta(metadata);
|
||||
const params: Record<string, unknown> = {
|
||||
uri,
|
||||
...(effectiveMeta ? { _meta: effectiveMeta } : {}),
|
||||
// `!== undefined` for the same reason `listSkills` uses it: a cursor is
|
||||
// opaque and `""` is a legal value, so truthiness would silently re-ask
|
||||
// for page one.
|
||||
...(cursor !== undefined ? { cursor } : {}),
|
||||
};
|
||||
// Era-aware for the same reason `skills/list` is — the method is
|
||||
// consumer-owned, so no SDK codec stamps or checks its envelope. The modern
|
||||
// variant requires only `resultType`; see the schema for why it stops
|
||||
// short of the caching attributes that `skills/list` requires.
|
||||
const resultSchema = this.isModernEra()
|
||||
? ModernDirectoryReadResultSchema
|
||||
: DirectoryReadResultSchema;
|
||||
try {
|
||||
return await this.invokeMcpClient(
|
||||
() =>
|
||||
this.client!.request(
|
||||
{ method: RESOURCES_DIRECTORY_READ_METHOD, params },
|
||||
resultSchema,
|
||||
this.getRequestOptions(this.progressTokenOf(metadata)),
|
||||
),
|
||||
{ method: RESOURCES_DIRECTORY_READ_METHOD },
|
||||
);
|
||||
} catch (err) {
|
||||
// Same attribution `getSkill` does, and for the same reason: there is no
|
||||
// managed store behind this method, so without this a rejected decode
|
||||
// would render in the Protocol tab as a clean success while the caller
|
||||
// showed an error. Only for a decode rejection — a request that never
|
||||
// produced a response would otherwise stamp an earlier exchange.
|
||||
if (isClientDecodeRejection(err)) {
|
||||
this.markResponseRejected(
|
||||
RESOURCES_DIRECTORY_READ_METHOD,
|
||||
err instanceof Error ? err.message : String(err),
|
||||
);
|
||||
}
|
||||
throw err;
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Get a prompt by name
|
||||
* @param name Prompt name
|
||||
|
|
|
|||
|
|
@ -42,6 +42,7 @@ import type { MalformedListItem } from "./listSalvage.js";
|
|||
import type { InspectorClientEventTarget } from "./inspectorClientEventTarget.js";
|
||||
import type { SkillEntry } from "./skillsSchemas.js";
|
||||
import type { SkillsExtensionSupport } from "./skills.js";
|
||||
import type { DirectoryReadResult } from "./skillsSchemas.js";
|
||||
import type { SamplingCreateMessage } from "./samplingCreateMessage.js";
|
||||
import type { ElicitationCreateMessage } from "./elicitationCreateMessage.js";
|
||||
|
||||
|
|
@ -122,6 +123,19 @@ export interface InspectorClientProtocol extends InspectorClientEventTarget {
|
|||
): Promise<{ skills: SkillEntry[]; nextCursor?: string }>;
|
||||
/** One skill entry by URI (`skills/get`). */
|
||||
getSkill(uri: string, metadata?: RequestMetadata): Promise<SkillEntry>;
|
||||
/**
|
||||
* One page of `resources/directory/read` — the direct children of a directory
|
||||
* resource (#2248). Optional on this interface, unlike the two methods above:
|
||||
* declaring the extension commits a server to `skills/list` and `skills/get`,
|
||||
* while this one is separately gated on `directoryRead`, so a caller has to
|
||||
* check for it anyway and the many test doubles that satisfy this interface
|
||||
* should not all have to grow a method most of them never reach.
|
||||
*/
|
||||
readResourceDirectory?(
|
||||
uri: string,
|
||||
cursor?: string,
|
||||
metadata?: RequestMetadata,
|
||||
): Promise<DirectoryReadResult>;
|
||||
|
||||
/**
|
||||
* Mark the response that most recently answered `method` as rejected by the
|
||||
|
|
|
|||
112
core/mcp/skillFile.ts
Normal file
112
core/mcp/skillFile.ts
Normal file
|
|
@ -0,0 +1,112 @@
|
|||
/**
|
||||
* The `SKILL.md` file format: splitting a served file into its YAML
|
||||
* frontmatter and body, and parsing that frontmatter into comparable JSON.
|
||||
*
|
||||
* Separate from `skills.ts` so the dependency runs one way. `skills.ts` owns the
|
||||
* *checks* and needs both halves of this module; this module knows nothing about
|
||||
* findings, severities, or `SkillEntry`, which is what lets it stay a pure text
|
||||
* transform with a single import.
|
||||
*
|
||||
* ⚠️ **This is the one place in `core/` that imports a YAML parser**, and it is
|
||||
* imported deliberately rather than hand-rolled. SEP-2640 requires a host to
|
||||
* *parse* a `SKILL.md`'s frontmatter and compare it field by field against the
|
||||
* entry's — see `checkSkillFrontmatterMatch` — and a regex approximation of YAML
|
||||
* would report a conforming server as broken the first time a description
|
||||
* carried a colon, a quoted string, or a multi-line block scalar. For a tool
|
||||
* whose entire output is "does this server conform", a checker that is itself
|
||||
* wrong is worse than no checker. `yaml` was already a repo-root **dependency**
|
||||
* (reached from `test-servers/src/load-config.ts`), so this adds no new package
|
||||
* to any manifest — but it does newly put `yaml` on `core/`'s runtime import
|
||||
* graph, which is why it joins the three bundler `external` lists in the same
|
||||
* change (see the dependency-placement rules in AGENTS.md).
|
||||
*/
|
||||
|
||||
import { parse as parseYaml } from "yaml";
|
||||
|
||||
export interface SkillFileParts {
|
||||
/**
|
||||
* The raw YAML between the fences, fences excluded — `undefined` when the
|
||||
* file has no frontmatter at all. Kept raw as well as parsed because the two
|
||||
* answer different questions: the parsed form is what the conformance check
|
||||
* compares, and the bytes are what a reader needs to see when the two
|
||||
* disagree.
|
||||
*/
|
||||
frontmatter?: string;
|
||||
/** Everything after the closing fence, or the whole file when there is none. */
|
||||
body: string;
|
||||
}
|
||||
|
||||
/**
|
||||
* Separate a leading YAML frontmatter fence from the rest of a skill file.
|
||||
*
|
||||
* The Skills screen renders the two halves in different places — the
|
||||
* frontmatter in its own collapsible section, the body in the file viewer — and
|
||||
* deriving both from **one** split is what stops them disagreeing: the section
|
||||
* can never show one file's frontmatter while the viewer shows another's, and a
|
||||
* file with no frontmatter cannot leave a stale section on screen.
|
||||
*
|
||||
* It also matters for rendering: the markdown renderer has no frontmatter
|
||||
* support, so an un-split `---\nname: …\n---` is read as a setext heading and
|
||||
* painted as a title above the document's real one.
|
||||
*
|
||||
* Two deliberate conservatisms, because this must never eat content:
|
||||
*
|
||||
* - Only a fence at the very **start** of the file counts. A `---` anywhere
|
||||
* else is a horizontal rule and is left in the body.
|
||||
* - A file that opens with `---` but never closes the fence is **not**
|
||||
* frontmatter; the whole file is returned as the body rather than being
|
||||
* truncated to nothing.
|
||||
*/
|
||||
export function splitSkillFile(text: string): SkillFileParts {
|
||||
if (!/^---[ \t]*\r?\n/.test(text)) return { body: text };
|
||||
const rest = text.slice(text.indexOf("\n") + 1);
|
||||
const close = rest.search(/^---[ \t]*\r?$/m);
|
||||
if (close === -1) return { body: text };
|
||||
const frontmatter = rest.slice(0, close).replace(/\r?\n$/, "");
|
||||
const after = rest.slice(close);
|
||||
const newline = after.indexOf("\n");
|
||||
if (newline === -1) return { frontmatter, body: "" };
|
||||
// Drop the blank line conventionally left between the fence and the body, so
|
||||
// the document does not open with dead space.
|
||||
return { frontmatter, body: after.slice(newline + 1).replace(/^\r?\n/, "") };
|
||||
}
|
||||
|
||||
/** Outcome of parsing a frontmatter block. Exactly one member is set. */
|
||||
export type ParsedFrontmatter =
|
||||
| { fields: Record<string, unknown> }
|
||||
| { error: string };
|
||||
|
||||
/**
|
||||
* Parse a frontmatter block into a field map.
|
||||
*
|
||||
* Two non-obvious decisions:
|
||||
*
|
||||
* - **YAML 1.2 core schema**, which is `yaml`'s default and is what the Agent
|
||||
* Skills format assumes. It resolves only JSON's own types, so a timestamp
|
||||
* stays the string the server wrote rather than becoming a `Date` — which
|
||||
* matters because the other side of the comparison arrived over JSON-RPC and
|
||||
* can only ever hold JSON types. Under YAML 1.1 the two would differ for a
|
||||
* date-shaped value that is in fact identical on the wire.
|
||||
* - **A non-mapping is an error, not an empty map.** `---\njust a string\n---`
|
||||
* parses successfully as the scalar `"just a string"`, and reporting that as
|
||||
* "no fields" would present a malformed file as one that merely omitted
|
||||
* everything. An *empty* block (`fields: {}`) is a different fact and is
|
||||
* reported as a successful parse of nothing.
|
||||
*/
|
||||
export function parseSkillFrontmatter(yamlText: string): ParsedFrontmatter {
|
||||
let parsed: unknown;
|
||||
try {
|
||||
parsed = parseYaml(yamlText);
|
||||
} catch (err) {
|
||||
return { error: err instanceof Error ? err.message : String(err) };
|
||||
}
|
||||
// `null` is what an empty (or comment-only) block parses to — a real, if
|
||||
// degenerate, mapping of no fields rather than a malformed one.
|
||||
if (parsed === null || parsed === undefined) return { fields: {} };
|
||||
if (typeof parsed !== "object" || Array.isArray(parsed)) {
|
||||
return {
|
||||
error: "Frontmatter is not a YAML mapping of fields.",
|
||||
};
|
||||
}
|
||||
return { fields: parsed as Record<string, unknown> };
|
||||
}
|
||||
|
|
@ -23,15 +23,15 @@
|
|||
* machinery (activation, per-skill consent, content-bound approval) is
|
||||
* implemented here. Surface and verify.
|
||||
*
|
||||
* ⚠️ **One SEP-2640 obligation is deliberately NOT checked here: that an entry's
|
||||
* `frontmatter` matches the fetched `SKILL.md`'s frontmatter field by field.**
|
||||
* The digest check does not cover it — a digest is taken over the bytes the
|
||||
* server served, so it proves the file was not tampered with in transit and
|
||||
* says nothing about whether the *listing* described that file honestly. A
|
||||
* server can therefore advertise one description, serve a different one, and
|
||||
* pass every check in this module. Closing it needs a YAML parser, which is a
|
||||
* new runtime dependency and a placement decision of its own, so it is tracked
|
||||
* on #2248 rather than half-done here.
|
||||
* **The frontmatter cross-check closes the gap the digest cannot** (#2248).
|
||||
* SEP-2640 requires that an entry's `frontmatter` match the fetched `SKILL.md`'s
|
||||
* frontmatter field by field, and no digest can establish that: a digest is
|
||||
* taken over the bytes the server served, so it proves the file was not altered
|
||||
* in transit and says nothing about whether the *listing* described that file
|
||||
* honestly. A server could advertise one description, serve another, and pass
|
||||
* every other check in this module. {@link checkSkillFrontmatterMatch} is the
|
||||
* check; it needs a real YAML parser, and `core/mcp/skillFile.ts` explains why
|
||||
* that dependency is imported rather than approximated.
|
||||
*/
|
||||
|
||||
import type { ServerCapabilities } from "@modelcontextprotocol/client";
|
||||
|
|
@ -42,6 +42,7 @@ import {
|
|||
type SkillResource,
|
||||
} from "./skillsSchemas.js";
|
||||
import { sha256Bytes } from "./sha256.js";
|
||||
import { parseSkillFrontmatter, splitSkillFile } from "./skillFile.js";
|
||||
|
||||
/** Maximum resource entries a single skill may declare (SEP-2640). */
|
||||
export const SKILL_MAX_RESOURCE_ENTRIES = 512;
|
||||
|
|
@ -243,7 +244,10 @@ export type SkillIssueCode =
|
|||
| "resource-outside-skill-root"
|
||||
| "manifest-missing-self"
|
||||
| "resource-limit-exceeded"
|
||||
| "size-limit-exceeded";
|
||||
| "size-limit-exceeded"
|
||||
| "frontmatter-absent"
|
||||
| "frontmatter-unparsable"
|
||||
| "frontmatter-mismatch";
|
||||
|
||||
/**
|
||||
* `error` marks a **MUST** of SEP-2640 that the server broke, so a manifest
|
||||
|
|
@ -646,6 +650,32 @@ export function base64ToBytes(blob: string): Uint8Array {
|
|||
return bytes;
|
||||
}
|
||||
|
||||
/**
|
||||
* The content a `resources/read` returned for one skill file. Either `text` (a
|
||||
* `TextResourceContents`) or `blob` (base64, a `BlobResourceContents`).
|
||||
*/
|
||||
export interface SkillFileContents {
|
||||
text?: string;
|
||||
blob?: string;
|
||||
mimeType?: string;
|
||||
}
|
||||
|
||||
/**
|
||||
* The raw bytes of a skill file, as fetched — the bytes its digest was taken
|
||||
* over.
|
||||
*
|
||||
* Throws for a result carrying neither `text` nor `blob`. That is a server bug,
|
||||
* and it must not be quietly treated as empty content: an empty `Uint8Array`
|
||||
* has a perfectly good SHA-256, so a silent fallback would report a *digest
|
||||
* mismatch* — a confident, wrong diagnosis — instead of "this response carried
|
||||
* no content at all". Callers surface the throw as a per-file read failure.
|
||||
*/
|
||||
export function skillFileBytes(contents: SkillFileContents): Uint8Array {
|
||||
if (typeof contents.text === "string") return textToBytes(contents.text);
|
||||
if (typeof contents.blob === "string") return base64ToBytes(contents.blob);
|
||||
throw new Error("resources/read returned neither text nor blob content.");
|
||||
}
|
||||
|
||||
/**
|
||||
* Verify one fetched skill file against its manifest entry.
|
||||
*
|
||||
|
|
@ -704,3 +734,105 @@ export async function verifySkillResource(
|
|||
: {}),
|
||||
};
|
||||
}
|
||||
|
||||
/**
|
||||
* Compare the fetched `SKILL.md`'s own frontmatter against the frontmatter the
|
||||
* entry advertised, field by field — the SEP-2640 obligation a digest cannot
|
||||
* discharge (#2248).
|
||||
*
|
||||
* SEP-2640: *"hosts MUST parse its YAML frontmatter and compare it
|
||||
* field-by-field against the entry's `frontmatter`. Any discrepancy MUST be
|
||||
* treated as a verification failure equivalent to a digest mismatch"*. So every
|
||||
* finding here is an `error`, matching what a digest mismatch reports — the
|
||||
* spec makes them equivalent and the report must not rank one below the other.
|
||||
*
|
||||
* **One finding per differing field, not one per file.** "Frontmatter does not
|
||||
* match" is unactionable for the server author who has to fix it; "listing says
|
||||
* `description: A`, file says `description: B`" is the whole diagnosis. The
|
||||
* union of both sides' keys is walked, so a field present on only one side is
|
||||
* reported as such rather than silently skipped.
|
||||
*
|
||||
* ⚠️ **Only call this with the bytes of the entry's own `SKILL.md`.** The check
|
||||
* is meaningless against a supporting file, which has no frontmatter to match,
|
||||
* and would report every one of them as `frontmatter-absent`. Callers select
|
||||
* the file; this function cannot tell which one it was handed.
|
||||
*
|
||||
* Values are compared as **canonical JSON**, so a frontmatter field holding a
|
||||
* nested mapping compares equal when the two sides agree on content and differ
|
||||
* only in key order — which is not a discrepancy in either JSON or YAML. Array
|
||||
* order *is* significant and is preserved, because a YAML sequence is ordered.
|
||||
*/
|
||||
export function checkSkillFrontmatterMatch(
|
||||
entry: SkillEntry,
|
||||
skillFileText: string,
|
||||
): SkillIssue[] {
|
||||
const { frontmatter } = splitSkillFile(skillFileText);
|
||||
if (frontmatter === undefined) {
|
||||
return [
|
||||
{
|
||||
code: "frontmatter-absent",
|
||||
severity: "error",
|
||||
message:
|
||||
"The served SKILL.md carries no YAML frontmatter block, so the listing's frontmatter cannot be the file's.",
|
||||
resourceUri: entry.uri,
|
||||
},
|
||||
];
|
||||
}
|
||||
const parsed = parseSkillFrontmatter(frontmatter);
|
||||
if ("error" in parsed) {
|
||||
return [
|
||||
{
|
||||
code: "frontmatter-unparsable",
|
||||
severity: "error",
|
||||
message: `The served SKILL.md's frontmatter is not valid YAML: ${parsed.error}`,
|
||||
resourceUri: entry.uri,
|
||||
},
|
||||
];
|
||||
}
|
||||
const issues: SkillIssue[] = [];
|
||||
// Sorted so the report is stable across runs — `Object.keys` order follows
|
||||
// insertion, which is the wire order on one side and the file order on the
|
||||
// other, and those need not agree even when the content does.
|
||||
const fields = [
|
||||
...new Set([
|
||||
...Object.keys(entry.frontmatter),
|
||||
...Object.keys(parsed.fields),
|
||||
]),
|
||||
].sort();
|
||||
for (const field of fields) {
|
||||
const listed = entry.frontmatter[field];
|
||||
const served = parsed.fields[field];
|
||||
// `undefined` is the only way "absent" reaches here: JSON has no undefined
|
||||
// value, and a YAML key written with an empty value parses to `null`, which
|
||||
// is a present field holding null and compares as one.
|
||||
if (listed === undefined) {
|
||||
issues.push({
|
||||
code: "frontmatter-mismatch",
|
||||
severity: "error",
|
||||
message: `The served SKILL.md declares "${field}" but the listing's frontmatter omits it.`,
|
||||
resourceUri: entry.uri,
|
||||
});
|
||||
continue;
|
||||
}
|
||||
if (served === undefined) {
|
||||
issues.push({
|
||||
code: "frontmatter-mismatch",
|
||||
severity: "error",
|
||||
message: `The listing declares "${field}" but the served SKILL.md omits it.`,
|
||||
resourceUri: entry.uri,
|
||||
});
|
||||
continue;
|
||||
}
|
||||
const listedJson = JSON.stringify(canonicalize(listed));
|
||||
const servedJson = JSON.stringify(canonicalize(served));
|
||||
if (listedJson !== servedJson) {
|
||||
issues.push({
|
||||
code: "frontmatter-mismatch",
|
||||
severity: "error",
|
||||
message: `Field "${field}" differs: the listing says ${listedJson} but the served SKILL.md says ${servedJson}.`,
|
||||
resourceUri: entry.uri,
|
||||
});
|
||||
}
|
||||
}
|
||||
return issues;
|
||||
}
|
||||
|
|
|
|||
|
|
@ -29,6 +29,7 @@
|
|||
*/
|
||||
|
||||
import { z } from "zod/v4";
|
||||
import { ResourceSchema } from "@modelcontextprotocol/core";
|
||||
|
||||
/** SEP-2133 extension identifier for the Skills extension (SEP-2640). */
|
||||
export const SKILLS_EXTENSION_KEY = "io.modelcontextprotocol/skills";
|
||||
|
|
@ -143,6 +144,22 @@ export const ModernListSkillsResultSchema = ListSkillsResultSchema.extend({
|
|||
/**
|
||||
* The `skills/get` result envelope: the entry wrapped under `skill`.
|
||||
*
|
||||
* ⚠️ **Not era-aware, and that is settled rather than pending (#2248).** The
|
||||
* obvious symmetry would be a modern variant requiring the caching attributes
|
||||
* the way {@link ModernListSkillsResultSchema} does. SEP-2640 forecloses it in
|
||||
* as many words, under `skills/get`: *"whether the result should also carry the
|
||||
* base protocol's caching attributes (`ttlMs` and `cacheScope`, per SEP-2549),
|
||||
* as `resources/read` results do, is **left open**"*.
|
||||
*
|
||||
* So there is no requirement to enforce, and inventing one would do real harm
|
||||
* rather than none: a server that reasonably reads "left open" as "not
|
||||
* required" would be reported as non-conforming by the tool whose job is to
|
||||
* tell it whether it conforms. `looseObject` means a server that *does* send
|
||||
* them still parses, which is the right handling for an attribute the spec
|
||||
* permits and does not mandate. Revisit only if a later revision closes the
|
||||
* question — and note that the same sentence is why `skills/get` carries no
|
||||
* `nextCursor` handling either: "a single entry is not a list".
|
||||
*
|
||||
* Required, not one of two accepted shapes. An earlier revision of this module
|
||||
* also accepted a bare entry at the top level, on the reading that the SEP
|
||||
* settled the entry but not its wrapper. It does settle the wrapper, and
|
||||
|
|
@ -165,13 +182,70 @@ export const GetSkillResultSchema = GetSkillEnvelopeSchema.transform(
|
|||
export type GetSkillResult = SkillEntry;
|
||||
|
||||
/**
|
||||
* ⚠️ **No `resources/directory/read` result schema here yet, on purpose.**
|
||||
* One `resources/directory/read` child: **the SDK's own `Resource`**, not a
|
||||
* shape restated here.
|
||||
*
|
||||
* The method name and the directory MIME type above are stated in SEP-2640;
|
||||
* the shape of the result it returns is not something this PR verified against
|
||||
* the normative text, and the Inspector does not call the method (phase 3,
|
||||
* #2248). Declaring a guessed schema would put an unverified claim in the one
|
||||
* module that is supposed to be the authority on the wire format — and one
|
||||
* nothing exercises, so it could be wrong indefinitely without failing
|
||||
* anything. Phase 3 adds it against the spec, alongside the call that uses it.
|
||||
* SEP-2640 defines the result as carrying "the same `Resource` objects that
|
||||
* `resources/list` returns, with the same `nextCursor` pagination contract", so
|
||||
* the schema that already decodes `resources/list` in this app is the literal
|
||||
* statement of that sentence — and one the SDK, not this module, keeps current.
|
||||
* Restating it would let the two drift, at which point a directory child and a
|
||||
* listed resource could disagree about what a `Resource` is while both claimed
|
||||
* to be one.
|
||||
*
|
||||
* ⚠️ It is **stricter than everything else in this module**, and that is the
|
||||
* deliberate exception rather than an oversight. `ResourceSchema` requires
|
||||
* `name` and strips unknown members, so one child missing `name` rejects the
|
||||
* whole page instead of being reported as a per-child finding — the opposite of
|
||||
* the posture the entry schemas above take. The reason the trade goes the other
|
||||
* way here is ownership: the skills entry types are consumer-owned and nothing
|
||||
* else validates them, so this module has to be the reporter; `Resource` is
|
||||
* base-protocol and already validated exactly this strictly on the
|
||||
* `resources/list` path, where {@link listSalvage} is the answer to a single bad
|
||||
* entry. Being *more* permissive here would mean a URI that fails as a listed
|
||||
* resource succeeds as a directory child, which is a worse inconsistency than
|
||||
* an all-or-nothing page.
|
||||
*/
|
||||
export const DirectoryChildSchema = ResourceSchema;
|
||||
|
||||
/**
|
||||
* `resources/directory/read` result on a **legacy** connection: the directory's
|
||||
* direct children plus the opaque cursor.
|
||||
*
|
||||
* `looseObject`, matching every other result schema here: a server that also
|
||||
* sends the caching attributes is not wrong for doing so, and a schema is not
|
||||
* the place to reject an extra member.
|
||||
*/
|
||||
export const DirectoryReadResultSchema = z.looseObject({
|
||||
resources: z.array(DirectoryChildSchema),
|
||||
nextCursor: z.string().optional(),
|
||||
});
|
||||
|
||||
export type DirectoryReadResult = z.infer<typeof DirectoryReadResultSchema>;
|
||||
|
||||
/**
|
||||
* `resources/directory/read` result on a **modern** (2026-07-28+) connection:
|
||||
* the page plus `resultType`, and deliberately **not** `ttlMs` / `cacheScope`.
|
||||
*
|
||||
* That asymmetry with {@link ModernListSkillsResultSchema} is the one judgement
|
||||
* call in this module, so it is written down rather than left to be re-derived:
|
||||
*
|
||||
* - For `skills/list` the SEP states the requirement outright — *"In protocol
|
||||
* versions 2026-07-28 and later, the result also carries … `ttlMs` and
|
||||
* `cacheScope`"* — so requiring them is quoting the spec.
|
||||
* - For `resources/directory/read` it states **nothing of the kind**, and its
|
||||
* one worked example of the result carries `resultType: "complete"` and no
|
||||
* caching attributes at all. Requiring them here would fail a server that
|
||||
* matched the SEP's own example, which is the failure direction this module
|
||||
* works hardest to avoid.
|
||||
*
|
||||
* `resultType` is required because it is the base protocol's, not this
|
||||
* extension's: SEP-2322 makes it a member of every modern result, the SEP's
|
||||
* example carries it, and `skills/*` being consumer-owned means the SDK codec
|
||||
* validates none of it — so if this schema does not, nothing does.
|
||||
*
|
||||
* ⚠️ Picked by `InspectorClient.readResourceDirectory` from the negotiated era.
|
||||
*/
|
||||
export const ModernDirectoryReadResultSchema = DirectoryReadResultSchema.extend(
|
||||
{ resultType: z.literal("complete") },
|
||||
);
|
||||
|
|
|
|||
221
core/mcp/skillsVerification.ts
Normal file
221
core/mcp/skillsVerification.ts
Normal file
|
|
@ -0,0 +1,221 @@
|
|||
/**
|
||||
* Fetch-and-verify: run every SEP-2640 check that needs the *bytes* of a skill's
|
||||
* files, over a whole set of entries (#2248).
|
||||
*
|
||||
* Separate from `skills.ts`, which is deliberately I/O-free —
|
||||
* `checkSkillConformance` reads a manifest, `verifySkillResource` compares bytes
|
||||
* it is handed, `checkSkillFrontmatterMatch` compares text it is handed, and
|
||||
* none of them knows how to obtain a file. This module is the part that does,
|
||||
* and keeping it in its own file is what lets the pure checks stay testable with
|
||||
* no client at all.
|
||||
*
|
||||
* It lives in `core/` because **two** clients drive it: the CLI's `--verify`
|
||||
* turns it into an NDJSON report with an exit code, and the TUI's Skills pane
|
||||
* runs it for one selected skill. Only the web screen does something different
|
||||
* — it fetches lazily, per click, because a browser user is reading one file at
|
||||
* a time rather than asking a yes/no question about a catalog. That difference
|
||||
* is about *when* to fetch, not *how* to check, so it does not belong here.
|
||||
*
|
||||
* ⚠️ Reads are **sequential, deliberately.** A conforming manifest may declare
|
||||
* 512 entries, and a parallel walk over one would open 512 `resources/read`
|
||||
* calls against a server whose whole purpose here is to be tested — the hazard
|
||||
* the web screen bounds with a concurrency limit. Sequential also makes the
|
||||
* report deterministic: entries come back in manifest order on every run, so a
|
||||
* CI diff of two reports shows what changed rather than what raced.
|
||||
*/
|
||||
|
||||
import { AuthRecoveryRequiredError } from "../auth/challenge.js";
|
||||
import type { InspectorClientProtocol } from "./inspectorClientProtocol.js";
|
||||
import type { RequestMetadata } from "./types.js";
|
||||
import {
|
||||
checkSkillConformance,
|
||||
checkSkillFrontmatterMatch,
|
||||
skillDisplayName,
|
||||
skillFileBytes,
|
||||
verifySkillResource,
|
||||
type SkillIssue,
|
||||
type SkillVerification,
|
||||
} from "./skills.js";
|
||||
import { DYNAMIC_RESOURCES, type SkillEntry } from "./skillsSchemas.js";
|
||||
|
||||
/** One manifest entry's outcome. `read-error` means the fetch itself failed. */
|
||||
export type SkillFileStatus = SkillVerification["status"] | "read-error";
|
||||
|
||||
export interface SkillFileReport {
|
||||
uri: string;
|
||||
status: SkillFileStatus;
|
||||
expectedDigest?: string;
|
||||
actualDigest?: string;
|
||||
expectedSize?: number;
|
||||
actualSize?: number;
|
||||
reason?: string;
|
||||
}
|
||||
|
||||
/** One skill's full verdict, and the unit of the NDJSON stream. */
|
||||
export interface SkillVerifyReport {
|
||||
uri: string;
|
||||
name: string;
|
||||
/** Structural findings against the entry as listed. */
|
||||
conformance: SkillIssue[];
|
||||
/**
|
||||
* Findings from comparing the served `SKILL.md`'s own frontmatter against the
|
||||
* listed one. Empty when the file could not be read — the read failure is
|
||||
* reported once, as a file result, rather than a second time as a phantom
|
||||
* frontmatter discrepancy.
|
||||
*/
|
||||
frontmatter: SkillIssue[];
|
||||
/** One entry per manifest file, in manifest order. Empty for `"dynamic"`. */
|
||||
files: SkillFileReport[];
|
||||
/**
|
||||
* False when anything the SEP makes a MUST was broken: an error-severity
|
||||
* finding, a digest or size mismatch, or a file that could not be read.
|
||||
*
|
||||
* A `warning` does **not** clear it — a `"dynamic"` manifest is legal, and a
|
||||
* report that failed CI for it would be telling server authors their
|
||||
* conforming skill is broken.
|
||||
*/
|
||||
ok: boolean;
|
||||
}
|
||||
|
||||
/** Result shape of one `resources/read`, narrowed to what a digest needs. */
|
||||
interface ReadContents {
|
||||
text?: string;
|
||||
blob?: string;
|
||||
mimeType?: string;
|
||||
}
|
||||
|
||||
/**
|
||||
* The first content block of a `resources/read` result.
|
||||
*
|
||||
* `contents[0]` rather than a search by URI: a server may legitimately answer
|
||||
* with a canonicalized spelling of the URI we asked for, and matching on the
|
||||
* string would reject it. A result with no blocks is a read failure and is
|
||||
* reported as one.
|
||||
*/
|
||||
function firstContents(result: unknown): ReadContents | undefined {
|
||||
const contents = (result as { contents?: unknown })?.contents;
|
||||
if (!Array.isArray(contents) || contents.length === 0) return undefined;
|
||||
const first: unknown = contents[0];
|
||||
if (typeof first !== "object" || first === null) return undefined;
|
||||
return first as ReadContents;
|
||||
}
|
||||
|
||||
/**
|
||||
* Verify every skill in `entries` against the connected server.
|
||||
*
|
||||
* Never throws for a single skill or a single file: a report that aborted on
|
||||
* the first unreadable file would hide every finding after it, and finding
|
||||
* everything wrong in one pass is the entire value of running this in CI.
|
||||
*
|
||||
* ⚠️ **`AuthRecoveryRequiredError` is the deliberate exception and is re-thrown.**
|
||||
* It is not a property of the file that happened to be in flight — it says the
|
||||
* session's authorization expired, so every remaining read would fail the same
|
||||
* way. Recording it per file would produce a report of N identical read
|
||||
* failures and, worse, would swallow the one error a caller keys off to start a
|
||||
* reauthorization: the TUI pane hands it to its recovery callback and the web
|
||||
* commands retry after it. Absorbed here, the user is simply told the files
|
||||
* could not be read, with no way offered to fix it.
|
||||
*/
|
||||
export async function verifySkills(
|
||||
client: InspectorClientProtocol,
|
||||
entries: readonly SkillEntry[],
|
||||
metadata?: RequestMetadata,
|
||||
): Promise<SkillVerifyReport[]> {
|
||||
const reports: SkillVerifyReport[] = [];
|
||||
for (const entry of entries) {
|
||||
// The entry's own SKILL.md, read once and used twice — for its digest and
|
||||
// for the frontmatter cross-check. Reading it twice would double the load
|
||||
// on the server and, worse, could compare a digest against one snapshot
|
||||
// and frontmatter against another.
|
||||
let entryText: string | undefined;
|
||||
const files: SkillFileReport[] = [];
|
||||
|
||||
const manifest =
|
||||
entry.resources === DYNAMIC_RESOURCES ? [] : entry.resources;
|
||||
for (const resource of manifest) {
|
||||
let contents: ReadContents | undefined;
|
||||
try {
|
||||
const invocation = await client.readResource(resource.uri, metadata);
|
||||
contents = firstContents(invocation.result);
|
||||
} catch (err) {
|
||||
if (err instanceof AuthRecoveryRequiredError) throw err;
|
||||
files.push({
|
||||
uri: resource.uri,
|
||||
status: "read-error",
|
||||
reason: err instanceof Error ? err.message : String(err),
|
||||
});
|
||||
continue;
|
||||
}
|
||||
if (!contents) {
|
||||
files.push({
|
||||
uri: resource.uri,
|
||||
status: "read-error",
|
||||
reason: "resources/read returned no content blocks.",
|
||||
});
|
||||
continue;
|
||||
}
|
||||
if (resource.uri === entry.uri && typeof contents.text === "string") {
|
||||
entryText = contents.text;
|
||||
}
|
||||
let bytes: Uint8Array;
|
||||
try {
|
||||
bytes = skillFileBytes(contents);
|
||||
} catch (err) {
|
||||
files.push({
|
||||
uri: resource.uri,
|
||||
status: "read-error",
|
||||
reason: err instanceof Error ? err.message : String(err),
|
||||
});
|
||||
continue;
|
||||
}
|
||||
const verification = await verifySkillResource(resource, bytes);
|
||||
files.push({ uri: resource.uri, ...verification });
|
||||
}
|
||||
|
||||
// A `"dynamic"` skill has no manifest, so the loop above read nothing —
|
||||
// but its SKILL.md is still served and still has to match the frontmatter
|
||||
// the listing advertised. That obligation is not waived by the file set
|
||||
// being unenumerable; only integrity is.
|
||||
if (entryText === undefined) {
|
||||
try {
|
||||
const invocation = await client.readResource(entry.uri, metadata);
|
||||
const contents = firstContents(invocation.result);
|
||||
if (typeof contents?.text === "string") entryText = contents.text;
|
||||
} catch (err) {
|
||||
// Left undefined: the frontmatter check is skipped below. When the
|
||||
// manifest listed this file the failure is already reported there, and
|
||||
// when it did not, `manifest-missing-self` is the finding that matters.
|
||||
// An expired authorization is not that case — see the note above.
|
||||
if (err instanceof AuthRecoveryRequiredError) throw err;
|
||||
}
|
||||
}
|
||||
|
||||
const conformance = checkSkillConformance(entry);
|
||||
const frontmatter =
|
||||
entryText === undefined
|
||||
? []
|
||||
: checkSkillFrontmatterMatch(entry, entryText);
|
||||
const hasError = [...conformance, ...frontmatter].some(
|
||||
(issue) => issue.severity === "error",
|
||||
);
|
||||
const fileFailed = files.some(
|
||||
(file) => file.status === "mismatch" || file.status === "read-error",
|
||||
);
|
||||
reports.push({
|
||||
uri: entry.uri,
|
||||
name: skillDisplayName(entry),
|
||||
conformance,
|
||||
frontmatter,
|
||||
files,
|
||||
ok: !hasError && !fileFailed,
|
||||
});
|
||||
}
|
||||
return reports;
|
||||
}
|
||||
|
||||
/** True when every skill in the report passed. */
|
||||
export function allSkillsVerified(
|
||||
reports: readonly SkillVerifyReport[],
|
||||
): boolean {
|
||||
return reports.every((report) => report.ok);
|
||||
}
|
||||
|
|
@ -16,6 +16,33 @@
|
|||
* reason the request is a plain `client.request`: the SDK has no high-level verb
|
||||
* for a consumer-owned extension method, so there is no cache-aware wrapper to
|
||||
* delegate to and no `cacheMode` to honor.
|
||||
*
|
||||
* ⚠️ **There is deliberately no `PagedSkillsState`, so the `paginatedLists`
|
||||
* server setting does not apply to Skills (#2248).** Tools, prompts and
|
||||
* resources each have a paged counterpart that setting switches them to; skills
|
||||
* does not, and the reason is not that the walk is cheap.
|
||||
*
|
||||
* It is that **every consumer of this list is a whole-catalog verdict.** The
|
||||
* Skills screen's conformance summary, and the CLI's `--verify` exit code, are
|
||||
* statements about the catalog: "this server's skills conform". Computed over
|
||||
* page one of three, that statement is *wrong* — it reports a clean catalog
|
||||
* while the tampered digest sits on page three, and reports it with exactly the
|
||||
* confidence of a real pass. Paging the other lists costs a reader some rows;
|
||||
* paging this one would make the tool's own output untrue. The setting exists
|
||||
* to let a user watch a server's pagination work, and this list's page count is
|
||||
* surfaced instead (`getPagination`, rendered by both the web screen and the
|
||||
* TUI pane), which serves that purpose without staking a verdict on a partial
|
||||
* read.
|
||||
*
|
||||
* The cost argument, which is the one #2248 asked about, points the same way
|
||||
* and is secondary: SEP-2640 makes a listing entry a *complete* manifest —
|
||||
* verbatim frontmatter and the full `resources` set with digests — precisely so
|
||||
* that "a host that pages through the listing has, in that one pass, everything
|
||||
* it needs … there is no second round-trip per skill". A full walk is the
|
||||
* access pattern the wire format was designed for. Revisit if a real server
|
||||
* turns up whose catalog makes the walk painful; the guards this walk already
|
||||
* carries (`SKILLS_MAX_PAGES`, the repeated-cursor check) are what bound it
|
||||
* until then.
|
||||
*/
|
||||
|
||||
import type { InspectorClientProtocol } from "../inspectorClientProtocol.js";
|
||||
|
|
|
|||
Loading…
Reference in a new issue