fix: address Copilot review round 1 on #2251
- skills.ts: report a manifest that omits the skill's own SKILL.md (an empty list included), duplicate URIs, entries outside the skill root, and a missing size. `Conforms` was reachable for manifests that break invariants SEP-2640 states. - skills.ts: cross-check the declared byte length before hashing. A size that disagrees fails verification on its own — the digest is taken over the bytes the server served, so agreeing with it says nothing about whether the manifest describes them. - skills.ts: copy the view instead of slicing its backing store in `sha256Digest`. `SharedArrayBuffer.prototype.slice()` returns another SharedArrayBuffer, which `crypto.subtle.digest` rejects — the cast claimed to handle the exact input that would have thrown. No cast now. - skills.ts: state the one obligation NOT checked here — that an entry's frontmatter matches the fetched SKILL.md's. The digest cannot cover it, and closing it needs a YAML parser, so it is tracked on #2248. - managedSkillsState: cap the walk at LIST_MAX_PAGES. The repeated-cursor guard only catches a server stuck on one cursor; endlessly unique ones walked forever. Raises rather than truncating, like the salvage walk. - managedSkillsState: gate every write on a session generation, so a walk that resolves after a disconnect or destroy cannot repopulate a cleared store or deliver the previous session's skills into the next. - inspectorClient: send a cursor when it is `!== undefined`, not when it is truthy. An opaque cursor may be `""`, and dropping it re-requested page one — which the store then reported as a repeated-cursor failure. - SkillsScreen: key verdict invalidation on the manifest (URI + digests + sizes), not the URI alone. A Refresh that changed the manifest left a green badge attached to a digest nothing had checked. - SkillsScreen: epoch-guard every read continuation, so a fetch that resolves after the selection moved on cannot write into the new one. - SkillsScreen: bound "Verify all" to 4 concurrent reads. A conforming manifest may hold 512 files. - skillsSchemas: drop the guessed `resources/directory/read` result schema. Nothing calls it, so an unverified shape could sit wrong indefinitely; phase 3 adds it against the normative text. - skills-http.json: declare the extension bare. It advertised `directoryRead: true` with no handler, so Connection Info reported "Supported" for a method that answers -32601. - types.ts: restore the Tasks doc comment the Skills interface displaced. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018kfrH3uRNUM4JAk1reKtyY Signed-off-by: cliffhall <cliff@futurescale.com>
This commit is contained in:
cliffhall
parent
63623b754b
commit
da8cc166ce
4 changed files with 604 additions and 107 deletions
|
|
@ -5555,7 +5555,11 @@ export class InspectorClient extends InspectorClientEventTarget {
|
|||
const effectiveMeta = this.mergeMeta(metadata);
|
||||
const params: Record<string, unknown> = {
|
||||
...(effectiveMeta ? { _meta: effectiveMeta } : {}),
|
||||
...(cursor ? { cursor } : {}),
|
||||
// `!== undefined`, not truthiness: a cursor is opaque and the empty
|
||||
// string is a legal value. Dropping `""` would silently re-request page
|
||||
// one, which the store then reports as a repeated-cursor failure — a
|
||||
// conforming server made to look broken.
|
||||
...(cursor !== undefined ? { cursor } : {}),
|
||||
};
|
||||
const response = await this.invokeMcpClient(
|
||||
() =>
|
||||
|
|
|
|||
|
|
@ -22,6 +22,16 @@
|
|||
* is explicitly not a load and confers no standing, so none of the SEP's host
|
||||
* 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.
|
||||
*/
|
||||
|
||||
import type { ServerCapabilities } from "@modelcontextprotocol/client";
|
||||
|
|
@ -115,6 +125,10 @@ export type SkillIssueCode =
|
|||
| "name-path-mismatch"
|
||||
| "missing-digest"
|
||||
| "malformed-digest"
|
||||
| "missing-size"
|
||||
| "duplicate-resource"
|
||||
| "resource-outside-skill-root"
|
||||
| "manifest-missing-self"
|
||||
| "resource-limit-exceeded"
|
||||
| "size-limit-exceeded";
|
||||
|
||||
|
|
@ -206,7 +220,48 @@ export function checkSkillConformance(entry: SkillEntry): SkillIssue[] {
|
|||
});
|
||||
}
|
||||
|
||||
// A manifest is the *complete* file set, and the skill's own SKILL.md is one
|
||||
// of those files. An empty list, or one that omits the entry's own URI, is
|
||||
// therefore not "a skill with no extra files" — it is a manifest that cannot
|
||||
// be checked against what the skill actually is, and reporting `Conforms`
|
||||
// for it would be a wrong answer rather than a missing one.
|
||||
if (!entry.resources.some((resource) => resource.uri === entry.uri)) {
|
||||
issues.push({
|
||||
code: "manifest-missing-self",
|
||||
severity: "error",
|
||||
message: `Manifest does not list the skill's own entry file (${entry.uri}); a manifest must be the complete file set.`,
|
||||
});
|
||||
}
|
||||
|
||||
const seenUris = new Set<string>();
|
||||
// Relative references resolve against the skill root, so every manifest entry
|
||||
// must live under it. A URI outside that prefix is either a typo or a server
|
||||
// claiming integrity over a file that is not part of this skill. Left
|
||||
// `undefined` for a malformed entry URI — there is no root to measure
|
||||
// against, and `malformed-uri` already reports that.
|
||||
const root = entry.uri.endsWith(SKILL_FILE_SUFFIX)
|
||||
? `${entry.uri.slice(0, -SKILL_FILE_SUFFIX.length)}/`
|
||||
: undefined;
|
||||
|
||||
for (const resource of entry.resources) {
|
||||
if (seenUris.has(resource.uri)) {
|
||||
issues.push({
|
||||
code: "duplicate-resource",
|
||||
severity: "error",
|
||||
message:
|
||||
"Manifest lists this URI more than once; entries must be unique.",
|
||||
resourceUri: resource.uri,
|
||||
});
|
||||
}
|
||||
seenUris.add(resource.uri);
|
||||
if (root !== undefined && !resource.uri.startsWith(root)) {
|
||||
issues.push({
|
||||
code: "resource-outside-skill-root",
|
||||
severity: "error",
|
||||
message: `Manifest entry is outside the skill root "${root}".`,
|
||||
resourceUri: resource.uri,
|
||||
});
|
||||
}
|
||||
if (resource.digest === undefined) {
|
||||
issues.push({
|
||||
code: "missing-digest",
|
||||
|
|
@ -222,6 +277,18 @@ export function checkSkillConformance(entry: SkillEntry): SkillIssue[] {
|
|||
resourceUri: resource.uri,
|
||||
});
|
||||
}
|
||||
if (resource.size === undefined) {
|
||||
// A warning, not an error: an absent size costs the length cross-check in
|
||||
// `verifySkillResource` and silently understates the 16 MiB total, but
|
||||
// the digest still verifies the bytes.
|
||||
issues.push({
|
||||
code: "missing-size",
|
||||
severity: "warning",
|
||||
message:
|
||||
"Manifest entry declares no size, so it is excluded from the 16 MiB total and its length cannot be cross-checked.",
|
||||
resourceUri: resource.uri,
|
||||
});
|
||||
}
|
||||
}
|
||||
|
||||
return issues;
|
||||
|
|
@ -250,6 +317,10 @@ export interface SkillVerification {
|
|||
actualDigest?: string;
|
||||
/** The manifest's digest, echoed so a mismatch renders both halves. */
|
||||
expectedDigest?: string;
|
||||
/** The manifest's declared byte length, when it declared one. */
|
||||
expectedSize?: number;
|
||||
/** The fetched file's actual byte length, when it was measured. */
|
||||
actualSize?: number;
|
||||
/** Why the file could not be verified or fetched. */
|
||||
reason?: string;
|
||||
}
|
||||
|
|
@ -271,13 +342,16 @@ function toHex(bytes: Uint8Array): string {
|
|||
* context, so `subtle` is present there too.
|
||||
*/
|
||||
export async function sha256Digest(bytes: Uint8Array): Promise<string> {
|
||||
// `BufferSource` wants a plain ArrayBuffer; a Uint8Array over a SharedArrayBuffer
|
||||
// (or a view into a larger buffer) would otherwise hash the wrong range.
|
||||
const buffer = bytes.buffer.slice(
|
||||
bytes.byteOffset,
|
||||
bytes.byteOffset + bytes.byteLength,
|
||||
) as ArrayBuffer;
|
||||
const hash = await crypto.subtle.digest("SHA-256", buffer);
|
||||
// Copy the VIEW into a fresh typed array rather than slicing its backing
|
||||
// store. Two things depend on that: a `Uint8Array` can be a window into a
|
||||
// larger buffer, so hashing the buffer would digest neighbouring bytes; and
|
||||
// `SharedArrayBuffer.prototype.slice()` returns another `SharedArrayBuffer`,
|
||||
// which `crypto.subtle.digest` rejects — so slicing-and-casting would have
|
||||
// failed at runtime for the exact input a cast claimed to handle.
|
||||
// `new Uint8Array(view)` always allocates a plain `ArrayBuffer`, which is
|
||||
// also why no cast is needed here.
|
||||
const copy = new Uint8Array(bytes);
|
||||
const hash = await crypto.subtle.digest("SHA-256", copy.buffer);
|
||||
return `sha256:${toHex(new Uint8Array(hash))}`;
|
||||
}
|
||||
|
||||
|
|
@ -307,11 +381,30 @@ export function base64ToBytes(blob: string): Uint8Array {
|
|||
* says. `"unverifiable"` means the manifest advertised no digest (or advertised
|
||||
* a malformed one, already reported by {@link checkSkillConformance}); nothing
|
||||
* about the file itself is wrong, we simply have nothing to compare against.
|
||||
*
|
||||
* The declared `size` is cross-checked **before** the digest and fails
|
||||
* verification on its own. A length that disagrees with the manifest is a real
|
||||
* inconsistency even when the digest matches — the digest is taken over the
|
||||
* bytes the server served, so agreeing with it says nothing about whether the
|
||||
* manifest describes those bytes — and it is the cheaper check, so a
|
||||
* 16 MiB file that was never going to verify is not hashed first.
|
||||
*/
|
||||
export async function verifySkillResource(
|
||||
resource: SkillResource,
|
||||
bytes: Uint8Array,
|
||||
): Promise<SkillVerification> {
|
||||
const expectedSize = resource.size;
|
||||
if (expectedSize !== undefined && expectedSize !== bytes.byteLength) {
|
||||
return {
|
||||
status: "mismatch",
|
||||
expectedSize,
|
||||
actualSize: bytes.byteLength,
|
||||
...(resource.digest !== undefined
|
||||
? { expectedDigest: resource.digest }
|
||||
: {}),
|
||||
reason: `Manifest declares ${expectedSize} bytes but the fetched file is ${bytes.byteLength}.`,
|
||||
};
|
||||
}
|
||||
const expectedDigest = resource.digest;
|
||||
if (expectedDigest === undefined) {
|
||||
return {
|
||||
|
|
@ -332,5 +425,8 @@ export async function verifySkillResource(
|
|||
status: actualDigest === expectedDigest ? "verified" : "mismatch",
|
||||
actualDigest,
|
||||
expectedDigest,
|
||||
...(expectedSize !== undefined
|
||||
? { expectedSize, actualSize: bytes.byteLength }
|
||||
: {}),
|
||||
};
|
||||
}
|
||||
|
|
|
|||
|
|
@ -142,23 +142,13 @@ export const GetSkillResultSchema = z
|
|||
export type GetSkillResult = SkillEntry;
|
||||
|
||||
/**
|
||||
* `resources/directory/read` result — the direct (non-recursive) children of a
|
||||
* directory resource. Present for completeness of the wire surface; the
|
||||
* Inspector surfaces the `directoryRead` sub-flag today and calls the method in
|
||||
* phase 3.
|
||||
* ⚠️ **No `resources/directory/read` result schema here yet, on purpose.**
|
||||
*
|
||||
* 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.
|
||||
*/
|
||||
export const ReadResourceDirectoryResultSchema = z.looseObject({
|
||||
contents: z.array(
|
||||
z.looseObject({
|
||||
uri: z.string(),
|
||||
name: z.string().optional(),
|
||||
mimeType: z.string().optional(),
|
||||
size: z.number().optional(),
|
||||
}),
|
||||
),
|
||||
nextCursor: z.string().optional(),
|
||||
});
|
||||
|
||||
export type ReadResourceDirectoryResult = z.infer<
|
||||
typeof ReadResourceDirectoryResultSchema
|
||||
>;
|
||||
|
|
|
|||
|
|
@ -21,6 +21,7 @@
|
|||
|
||||
import type { InspectorClientProtocol } from "../inspectorClientProtocol.js";
|
||||
import type { SkillEntry } from "../skillsSchemas.js";
|
||||
import { LIST_MAX_PAGES } from "../listSalvage.js";
|
||||
import { isTerminalStatus } from "../types.js";
|
||||
import type { RequestMetadata } from "../types.js";
|
||||
import { TypedEventTarget } from "../typedEventTarget.js";
|
||||
|
|
@ -46,6 +47,16 @@ export interface ManagedSkillsStateEventMap {
|
|||
export const REPEATED_CURSOR_MESSAGE =
|
||||
"Server repeated a pagination cursor in skills/list; stopped to avoid an infinite walk.";
|
||||
|
||||
/**
|
||||
* Page cap for the `skills/list` walk. Mirrors `LIST_MAX_PAGES`, the bound the
|
||||
* SDK's aggregate verbs and the #1909 salvage re-walks already use, so the two
|
||||
* pagination paths in this repo cannot drift to different limits.
|
||||
*/
|
||||
export const SKILLS_MAX_PAGES = LIST_MAX_PAGES;
|
||||
|
||||
/** The error a walk raises when it hits {@link SKILLS_MAX_PAGES}. */
|
||||
export const SKILLS_PAGE_LIMIT_MESSAGE = `skills/list exceeded ${SKILLS_MAX_PAGES} pages without the server's pagination converging`;
|
||||
|
||||
export class ManagedSkillsState extends TypedEventTarget<ManagedSkillsStateEventMap> {
|
||||
private skills: SkillEntry[] = [];
|
||||
private pageCount = 0;
|
||||
|
|
@ -55,6 +66,9 @@ export class ManagedSkillsState extends TypedEventTarget<ManagedSkillsStateEvent
|
|||
// Overlap guard: a walk in flight makes a second one a no-op so a slow older
|
||||
// walk can't clobber a newer list via last-write-wins.
|
||||
private running = false;
|
||||
// Session counter, advanced by `reset` (disconnect) and by `destroy`. A walk
|
||||
// captures it and abandons its writes when it no longer matches.
|
||||
private generation = 0;
|
||||
|
||||
constructor(client: InspectorClientProtocol) {
|
||||
super();
|
||||
|
|
@ -106,6 +120,9 @@ export class ManagedSkillsState extends TypedEventTarget<ManagedSkillsStateEvent
|
|||
}
|
||||
|
||||
private reset(): void {
|
||||
// Advance the session first, so an in-flight walk's continuation sees a
|
||||
// stale generation and leaves the cleared state alone.
|
||||
this.generation += 1;
|
||||
this.skills = [];
|
||||
this.pageCount = 0;
|
||||
this.dispatchTypedEvent("skillsChange", this.getSkills());
|
||||
|
|
@ -136,6 +153,13 @@ export class ManagedSkillsState extends TypedEventTarget<ManagedSkillsStateEvent
|
|||
}
|
||||
if (this.running) return this.getSkills();
|
||||
this.running = true;
|
||||
// The session this walk belongs to. A disconnect, a `destroy()`, or a
|
||||
// reconnect on the same client advances it, and every write below is
|
||||
// gated on it still being current — otherwise a slow walk's continuation
|
||||
// repopulates a store that has already been cleared, or delivers the
|
||||
// previous session's skills into the next one.
|
||||
const generation = this.generation;
|
||||
const isCurrent = () => this.generation === generation;
|
||||
try {
|
||||
const collected: SkillEntry[] = [];
|
||||
const seen = new Set<string>();
|
||||
|
|
@ -143,9 +167,20 @@ export class ManagedSkillsState extends TypedEventTarget<ManagedSkillsStateEvent
|
|||
let pages = 0;
|
||||
for (;;) {
|
||||
const page = await client.listSkills(cursor, metadata);
|
||||
if (!isCurrent()) return this.getSkills();
|
||||
collected.push(...page.skills);
|
||||
pages += 1;
|
||||
if (page.nextCursor === undefined) break;
|
||||
// Two distinct runaway shapes, and the repeated-cursor guard only
|
||||
// catches one: a server stuck on a single cursor. A server that hands
|
||||
// back an endlessly *unique* cursor walks forever and grows
|
||||
// `collected` without bound, so the page cap is what stops it. The cap
|
||||
// raises rather than truncating, for the reason `listPaginationExceeded`
|
||||
// states: returning what we have would present a partial list as a
|
||||
// complete one.
|
||||
if (pages >= SKILLS_MAX_PAGES) {
|
||||
throw new Error(SKILLS_PAGE_LIMIT_MESSAGE);
|
||||
}
|
||||
if (seen.has(page.nextCursor)) {
|
||||
throw new Error(REPEATED_CURSOR_MESSAGE);
|
||||
}
|
||||
|
|
@ -156,7 +191,12 @@ export class ManagedSkillsState extends TypedEventTarget<ManagedSkillsStateEvent
|
|||
this.applyPages(collected, pages);
|
||||
return this.getSkills();
|
||||
} catch (err) {
|
||||
this.setError(err instanceof Error ? err : new Error(String(err)));
|
||||
// Still re-thrown even when the session moved on — the caller's
|
||||
// auth-recovery wrapper keys off the rejection — but the *state* is left
|
||||
// alone, so a dead session's failure cannot surface in the live one.
|
||||
if (isCurrent()) {
|
||||
this.setError(err instanceof Error ? err : new Error(String(err)));
|
||||
}
|
||||
throw err;
|
||||
} finally {
|
||||
this.running = false;
|
||||
|
|
@ -172,6 +212,7 @@ export class ManagedSkillsState extends TypedEventTarget<ManagedSkillsStateEvent
|
|||
|
||||
/** Unsubscribe from the client and drop the list; idempotent. */
|
||||
destroy(): void {
|
||||
this.generation += 1;
|
||||
this.unsubscribe?.();
|
||||
this.unsubscribe = null;
|
||||
this.skills = [];
|
||||
|
|
|
|||
Loading…
Reference in a new issue