diff --git a/clients/web/src/components/screens/SkillsScreen/SkillsScreen.test.tsx b/clients/web/src/components/screens/SkillsScreen/SkillsScreen.test.tsx index 9ee2532c..1356b156 100644 --- a/clients/web/src/components/screens/SkillsScreen/SkillsScreen.test.tsx +++ b/clients/web/src/components/screens/SkillsScreen/SkillsScreen.test.tsx @@ -259,6 +259,82 @@ describe("SkillsScreen", () => { expect(await screen.findAllByText("plain string")).toHaveLength(2); }); + it("titles a size disagreement a size mismatch, not a digest one", async () => { + // `verifySkillResource` catches a size disagreement BEFORE hashing, so + // there is no `actualDigest` — labelling it "Digest mismatch" would render + // "actual undefined" and hide the real failure. + const user = userEvent.setup(); + renderWithMantine( + , + ); + await user.click(screen.getByText("data-analysis")); + await user.click(screen.getByRole("button", { name: /Verify all/ })); + expect(await screen.findByText("Size mismatch")).toBeInTheDocument(); + expect(screen.queryByText("Digest mismatch")).not.toBeInTheDocument(); + // The alert states both lengths; the manifest row also shows the declared + // one, hence `getAllByText`. + expect(screen.getAllByText(/9999 bytes/).length).toBeGreaterThan(0); + }); + + it("gives duplicated manifest URIs their own row and their own verdict", async () => { + // The conformance checker reports `duplicate-resource` rather than + // collapsing the rows, so the verdicts must not collapse either: the two + // entries declare different digests and only one of them is right. + const user = userEvent.setup(); + renderWithMantine( + , + ); + await user.click(screen.getByText("data-analysis")); + await user.click(screen.getByRole("button", { name: /Verify all/ })); + // One row verifies and the other does not — a shared key would have made + // both show whichever landed last. + expect(await screen.findByText("verified")).toBeInTheDocument(); + expect(screen.getByText("mismatch")).toBeInTheDocument(); + }); + + it("renders a base64 SKILL.md preview instead of a blank one", async () => { + // `onReadSkillFile` supports blob content, and verification reads it + // correctly; dropping it in the preview would paint an empty box for a + // file the screen had just checked. + const user = userEvent.setup(); + const onReadSkillFile = vi.fn().mockResolvedValue({ + blob: btoa("# from a blob\n"), + mimeType: "text/markdown", + }); + renderWithMantine( + , + ); + await user.click(screen.getByText("data-analysis")); + await user.click(screen.getByRole("button", { name: /View SKILL.md/ })); + const preview = await screen.findByTestId("skill-md-preview"); + expect(preview).toHaveTextContent("from a blob"); + }); + it("shows the SKILL.md preview on demand", async () => { const user = userEvent.setup(); renderWithMantine(); diff --git a/clients/web/src/components/screens/SkillsScreen/SkillsScreen.tsx b/clients/web/src/components/screens/SkillsScreen/SkillsScreen.tsx index 1f313cd0..faab7e7b 100644 --- a/clients/web/src/components/screens/SkillsScreen/SkillsScreen.tsx +++ b/clients/web/src/components/screens/SkillsScreen/SkillsScreen.tsx @@ -1,4 +1,4 @@ -import { useCallback, useMemo, useRef, useState } from "react"; +import { useCallback, useMemo, useState } from "react"; import { Alert, Badge, @@ -42,12 +42,37 @@ import { */ const VERIFY_CONCURRENCY = 4; -/** Per-file verification progress, keyed by the manifest entry's URI. */ +/** Per-row verification progress. */ type FileState = | { status: "pending" } | { status: "done"; verification: SkillVerification } | { status: "error"; message: string }; +/** + * Verification verdicts plus the manifest they belong to. Rows are keyed by + * their **index**, not their URI: the conformance checker deliberately tolerates + * a duplicated URI so it can report `duplicate-resource`, and a URI key would + * collide those two rows into one verdict. + */ +interface VerificationState { + /** + * The manifest these verdicts belong to, or `null` before anything has been + * verified. `useValueChange` deliberately does not fire on the first render, + * so `null` stands in for "the initial manifest, not yet adopted" — the first + * write claims it. Once set it is only ever replaced by an invalidation, so a + * stale continuation can never be mistaken for an initial one. + */ + key: string | null; + files: Record; +} + +/** The SKILL.md preview, plus the manifest it belongs to (`null` as above). */ +interface PreviewState { + key: string | null; + contents?: SkillFileContents; + message?: string; +} + export interface SkillsScreenProps { skills: SkillEntry[]; /** Pages the last `skills/list` walk took; shown so pagination is visible. */ @@ -239,15 +264,19 @@ export function SkillsScreen({ onReadSkillFile, }: SkillsScreenProps) { const { selectedSkillUri, search } = ui; - const [fileStates, setFileStates] = useState>({}); - const [preview, setPreview] = useState(null); - const [previewError, setPreviewError] = useState(null); - // Bumped every time the verdicts are invalidated. A read that was already in - // flight captures the value it started under and discards its result when - // this has moved on — otherwise a slow fetch for the previous selection (or - // the previous manifest) lands afterwards and writes a verdict for content - // nobody is looking at, or worse, one that was never checked. - const epoch = useRef(0); + // Both slices carry the manifest key they belong to, and every async + // continuation writes through a functional update that compares it. That is + // what discards a read still in flight when the selection changes or a + // Refresh replaces the manifest — without it, a slow fetch lands afterwards + // and writes a verdict for content nobody is looking at, or one that was + // never checked. Storing the key IN the state (rather than bumping a ref + // during render) keeps the `useValueChange` callback to `setState` calls + // only, which is the purity that hook documents and requires. + const [verification, setVerification] = useState({ + key: null, + files: {}, + }); + const [previewState, setPreviewState] = useState({ key: null }); const filtered = useMemo(() => { const needle = search.trim().toLowerCase(); @@ -295,43 +324,45 @@ export function SkillsScreen({ // Adjusted DURING RENDER via `useValueChange` rather than in an effect, so a // new selection (or a refreshed manifest) never paints a frame carrying the - // previous one's verification results. The epoch bump is a ref write, which - // is why it is done in the callback alongside the state resets rather than - // during the render body itself. - useValueChange(manifestKey, () => { - epoch.current += 1; - setFileStates({}); - setPreview(null); - setPreviewError(null); + // previous one's verification results. `setState` calls only — the hook + // replays this callback whenever React replays the render. + useValueChange(manifestKey, (next) => { + setVerification({ key: next, files: {} }); + setPreviewState({ key: next }); }); - const verifyFile = useCallback( - async (resource: SkillResource) => { - const started = epoch.current; - setFileStates((prev) => ({ - ...prev, - [resource.uri]: { status: "pending" }, - })); + const fileStates = verification.key === manifestKey ? verification.files : {}; + + /** + * Verify one manifest ROW. Keyed by row index, not by URI: the checker + * deliberately tolerates a duplicated URI so it can report + * `duplicate-resource`, and two rows sharing a key would share one verdict — + * verifying either would update both, and "Verify all" would race two + * different digest/size declarations into the same slot. + */ + const verifyRow = useCallback( + async (index: number, resource: SkillResource, key: string) => { + const write = (state: FileState) => + setVerification((prev) => { + // `null` is the un-adopted initial manifest; any other mismatch is a + // continuation from a manifest that has since been invalidated. + if (prev.key !== null && prev.key !== key) return prev; + const files = prev.key === key ? prev.files : {}; + return { key, files: { ...files, [index]: state } }; + }); + write({ status: "pending" }); try { const contents = await onReadSkillFile(resource.uri); - const verification = await verifySkillResource( + const result = await verifySkillResource( resource, skillFileBytes(contents), ); - if (epoch.current !== started) return; - setFileStates((prev) => ({ - ...prev, - [resource.uri]: { status: "done", verification }, - })); + write({ status: "done", verification: result }); } catch (err) { - if (epoch.current !== started) return; - setFileStates((prev) => ({ - ...prev, - [resource.uri]: { - status: "error", - message: err instanceof Error ? err.message : String(err), - }, - })); + write({ + status: "error", + message: err instanceof Error ? err.message : String(err), + }); } }, [onReadSkillFile], @@ -348,34 +379,42 @@ export function SkillsScreen({ // records them as per-row state), and this handler cannot be async, so the // settled promise is discarded explicitly at one place instead of per file. let next = 0; + const key = manifestKey; const worker = async (): Promise => { for (let i = next++; i < manifest.length; i = next++) { - await verifyFile(manifest[i]); + await verifyRow(i, manifest[i], key); } }; const workers = Math.min(VERIFY_CONCURRENCY, manifest.length); void Promise.all(Array.from({ length: workers }, () => worker())); - }, [manifest, verifyFile]); + }, [manifest, manifestKey, verifyRow]); const showSkillMd = useCallback(() => { if (!selected) return; - const started = epoch.current; + const key = manifestKey; // A click handler cannot await, and this chain terminates in its own - // `catch` that surfaces the message in the preview slot. Both arms are - // epoch-guarded: a read that resolves after the selection moved on would - // otherwise show one skill's SKILL.md under another's heading. + // `catch` that surfaces the message in the preview slot. Both arms compare + // the manifest key they started under: a read that resolves after the + // selection moved on would otherwise show one skill's SKILL.md under + // another's heading. void onReadSkillFile(selected.uri) .then((contents) => { - if (epoch.current !== started) return; - setPreview(contents); - setPreviewError(null); + setPreviewState((prev) => + prev.key !== null && prev.key !== key ? prev : { key, contents }, + ); }) .catch((err: unknown) => { - if (epoch.current !== started) return; - setPreview(null); - setPreviewError(err instanceof Error ? err.message : String(err)); + const message = err instanceof Error ? err.message : String(err); + setPreviewState((prev) => + prev.key !== null && prev.key !== key ? prev : { key, message }, + ); }); - }, [onReadSkillFile, selected]); + }, [manifestKey, onReadSkillFile, selected]); + + const preview = + previewState.key === manifestKey ? previewState.contents : undefined; + const previewError = + previewState.key === manifestKey ? previewState.message : undefined; const errorCount = issues.filter((i) => i.severity === "error").length; const warningCount = issues.length - errorCount; @@ -527,8 +566,8 @@ export function SkillsScreen({ - {manifest.map((resource) => { - const state = fileStates[resource.uri]; + {manifest.map((resource, index) => { + const state = fileStates[index]; const color = state?.status === "done" ? verificationColor(state.verification.status) @@ -536,7 +575,10 @@ export function SkillsScreen({ ? "red" : "gray"; return ( - + // Index-keyed for the same reason the verdicts are: + // a duplicated URI is a case this screen reports, so + // it must not also collide two rows into one. + {resource.uri} {resource.size ?? "—"} {shortDigest(resource.digest)} @@ -549,7 +591,9 @@ export function SkillsScreen({ // A click handler cannot await, and // `verifyFile` owns its own failures — it // records them as this row's state. - onClick={() => void verifyFile(resource)} + onClick={() => + void verifyRow(index, resource, manifestKey) + } > Verify @@ -561,35 +605,45 @@ export function SkillsScreen({ )} - {manifest.map((resource) => { - const state = fileStates[resource.uri]; + {manifest.map((resource, index) => { + const state = fileStates[index]; if (state?.status === "done") { - const { verification } = state; - if (verification.status === "mismatch") { - return ( - - - {resource.uri} - - expected {verification.expectedDigest} - - - actual {verification.actualDigest} - - - - ); - } - return null; + const result = state.verification; + if (result.status !== "mismatch") return null; + // A size disagreement is caught BEFORE hashing, so it has + // no `actualDigest` — titling it "Digest mismatch" and + // rendering "actual undefined" would hide the real failure. + const sizeFailure = result.actualDigest === undefined; + return ( + + + {resource.uri} + {sizeFailure ? ( + {result.reason} + ) : ( + <> + + expected {result.expectedDigest} + + + actual {result.actualDigest} + + + )} + + + ); } if (state?.status === "error") { return ( @@ -612,9 +666,24 @@ export function SkillsScreen({ {preview && ( SKILL.md + {/* `contents`, not a text `block`: a server may serve + SKILL.md as a base64 `blob`, and the block form would + substitute an empty string and paint a blank preview for + a file verification just read correctly. */} diff --git a/clients/web/src/test/core/mcp/inspectorClient-skills.test.ts b/clients/web/src/test/core/mcp/inspectorClient-skills.test.ts index d60b16db..1ad5250c 100644 --- a/clients/web/src/test/core/mcp/inspectorClient-skills.test.ts +++ b/clients/web/src/test/core/mcp/inspectorClient-skills.test.ts @@ -115,18 +115,20 @@ describe("InspectorClient skills methods (#2234)", () => { expect(request.mock.calls[0][0].params.uri).toBe("skill://demo/SKILL.md"); }); - it("normalizes the enveloped skills/get result to the entry", async () => { + it("unwraps the skills/get envelope to the entry", async () => { const client = makeClient(); stubRequest(client, { skill: ENTRY }); expect(await client.getSkill("skill://demo/SKILL.md")).toEqual(ENTRY); }); - it("normalizes the inline skills/get result to the entry", async () => { - // The SEP settles the entry shape but not the envelope; a server that - // returns the entry at the top level must not fail here. + it("rejects a skills/get result returned without its envelope", async () => { + // Normalizing it would let a non-conforming server through the one place + // that could have reported it. const client = makeClient(); stubRequest(client, ENTRY); - expect(await client.getSkill("skill://demo/SKILL.md")).toEqual(ENTRY); + await expect( + client.getSkill("skill://demo/SKILL.md"), + ).rejects.toBeDefined(); }); it("rejects a skills/list result that is not a skills page", async () => { diff --git a/clients/web/src/test/core/mcp/skills.test.ts b/clients/web/src/test/core/mcp/skills.test.ts index ace626b1..06e933d0 100644 --- a/clients/web/src/test/core/mcp/skills.test.ts +++ b/clients/web/src/test/core/mcp/skills.test.ts @@ -295,6 +295,39 @@ describe("checkSkillConformance", () => { } }); + it("reports a size that is not a non-negative integer byte length", () => { + for (const size of [-1, 1.5, Number.NaN, Number.MAX_SAFE_INTEGER + 2]) { + const issues = checkSkillConformance( + entry({ + resources: [ + { uri: "skill://demo/SKILL.md", digest: DIGEST, size: 20 }, + { uri: "skill://demo/ref.md", digest: DIGEST, size }, + ], + }), + ); + expect(issues.map((i) => i.code)).toEqual(["malformed-size"]); + expect(issues[0].severity).toBe("error"); + } + }); + + it("a negative size cannot pull the total back under the 16 MiB limit", () => { + // The reason `malformed-size` is an error and not just noise: summing a + // negative would hide a genuine `size-limit-exceeded`. + const issues = checkSkillConformance( + entry({ + resources: [ + { + uri: "skill://demo/SKILL.md", + digest: DIGEST, + size: SKILL_MAX_TOTAL_BYTES + 1, + }, + { uri: "skill://demo/ref.md", digest: DIGEST, size: -1000 }, + ], + }), + ); + expect(issues.map((i) => i.code)).toContain("size-limit-exceeded"); + }); + it("reports a manifest over the 512-entry limit", () => { const resources = [ { uri: "skill://demo/SKILL.md", digest: DIGEST, size: 20 }, @@ -350,6 +383,20 @@ describe("totalSkillBytes", () => { ]), ).toBe(15); }); + + it("excludes an unusable size rather than summing it", () => { + // An incomplete manifest may only ever *understate* the total, which is + // what keeps the limit check free of false positives. A negative or + // fractional value would break that. + expect( + totalSkillBytes([ + { uri: "a", size: 10 }, + { uri: "b", size: -100 }, + { uri: "c", size: 2.5 }, + { uri: "d", size: Number.NaN }, + ]), + ).toBe(10); + }); }); describe("byte helpers", () => { diff --git a/clients/web/src/test/core/mcp/skillsSchemas.test.ts b/clients/web/src/test/core/mcp/skillsSchemas.test.ts index e0a09226..724c98cc 100644 --- a/clients/web/src/test/core/mcp/skillsSchemas.test.ts +++ b/clients/web/src/test/core/mcp/skillsSchemas.test.ts @@ -7,7 +7,6 @@ import { SKILLS_GET_METHOD, SKILLS_LIST_METHOD, SkillEntrySchema, - normalizeGetSkillResult, } from "@inspector/core/mcp/skillsSchemas"; const ENTRY = { @@ -89,20 +88,24 @@ describe("ListSkillsResultSchema", () => { }); describe("GetSkillResultSchema", () => { - it("normalizes the enveloped form to the entry", () => { + it("unwraps the envelope to the entry", () => { expect(GetSkillResultSchema.parse({ skill: ENTRY })).toEqual(ENTRY); }); - it("normalizes the inline form to the entry", () => { - expect(GetSkillResultSchema.parse(ENTRY)).toEqual(ENTRY); - }); - - it("normalizeGetSkillResult accepts either shape directly", () => { - expect(normalizeGetSkillResult({ skill: ENTRY })).toEqual(ENTRY); - expect(normalizeGetSkillResult(ENTRY)).toEqual(ENTRY); + it("rejects an entry returned inline rather than normalizing it", () => { + // The envelope is required. Accepting the inline form would silently + // normalize a non-conforming response, which is the failure this + // extension's support exists to report. + expect(() => GetSkillResultSchema.parse(ENTRY)).toThrow(); }); it("rejects a result that is neither shape", () => { expect(() => GetSkillResultSchema.parse({ nothing: true })).toThrow(); }); + + it("rejects an envelope whose skill is not an entry", () => { + expect(() => + GetSkillResultSchema.parse({ skill: { frontmatter: {} } }), + ).toThrow(); + }); }); diff --git a/clients/web/src/test/core/mcp/state/managedSkillsState.test.ts b/clients/web/src/test/core/mcp/state/managedSkillsState.test.ts index 7a9ecfda..2cda1537 100644 --- a/clients/web/src/test/core/mcp/state/managedSkillsState.test.ts +++ b/clients/web/src/test/core/mcp/state/managedSkillsState.test.ts @@ -142,6 +142,52 @@ describe("ManagedSkillsState", () => { expect(state.getPagination()).toEqual({ pageCount: 0 }); }); + it("lets a reconnect load its skills while a stale walk is still hanging", async () => { + client.setStatus("connected"); + // The first walk never settles. A boolean overlap guard would stay set, + // so the reconnect's own load would no-op and never be retried — the + // reconnect would show an empty Skills tab forever. + client.listSkills.mockImplementationOnce(() => new Promise(() => {})); + void state.refresh().catch(() => {}); + await client.disconnect(); + + client.skillPages = [{ skills: [skill("fresh")] }]; + await client.connect(); + expect(state.getSkills().map((s) => s.frontmatter.name)).toEqual(["fresh"]); + }); + + it("a stale walk settling later cannot release the live session's guard", async () => { + client.setStatus("connected"); + let release: ((value: { skills: SkillEntry[] }) => void) | undefined; + client.listSkills.mockImplementationOnce( + async () => + new Promise((resolve) => { + release = resolve; + }), + ); + const stale = state.refresh(); + await client.disconnect(); + client.setStatus("connected"); + + // A live walk is now in flight under the new generation. + let releaseLive: ((value: { skills: SkillEntry[] }) => void) | undefined; + client.listSkills.mockImplementationOnce( + async () => + new Promise((resolve) => { + releaseLive = resolve; + }), + ); + const live = state.refresh(); + // The stale one settles first; its `finally` must not free the guard. + release?.({ skills: [] }); + await stale; + const blocked = await state.refresh(); + expect(blocked).toEqual([]); + releaseLive?.({ skills: [skill("live")] }); + await live; + expect(state.getSkills().map((s) => s.frontmatter.name)).toEqual(["live"]); + }); + it("does not surface a dead session's failure in the live one", async () => { client.setStatus("connected"); let fail: ((err: Error) => void) | undefined; diff --git a/core/mcp/inspectorClient.ts b/core/mcp/inspectorClient.ts index ca59952d..07a9cc6d 100644 --- a/core/mcp/inspectorClient.ts +++ b/core/mcp/inspectorClient.ts @@ -5577,9 +5577,10 @@ export class InspectorClient extends InspectorClientEventTarget { } /** - * One skill entry by URI (`skills/get`, SEP-2640). The SEP settles the entry - * shape but not the envelope around it, so the result is normalized through - * {@link normalizeGetSkillResult} rather than assuming one form. + * One skill entry by URI (`skills/get`, SEP-2640). The result envelope is + * required — `GetSkillResultSchema` unwraps `{ skill }` and rejects an entry + * returned inline, so a non-conforming shape fails here rather than being + * silently normalized past the conformance checks. */ async getSkill(uri: string, metadata?: RequestMetadata): Promise { if (!this.client) { @@ -5590,8 +5591,8 @@ export class InspectorClient extends InspectorClientEventTarget { uri, ...(effectiveMeta ? { _meta: effectiveMeta } : {}), }; - // `GetSkillResultSchema` normalizes both accepted envelopes to the entry, - // so there is nothing to unwrap here. + // `GetSkillResultSchema` unwraps the envelope, so there is nothing to + // unwrap here. return this.invokeMcpClient( () => this.client!.request( diff --git a/core/mcp/skills.ts b/core/mcp/skills.ts index 0a1aeead..f7c776d5 100644 --- a/core/mcp/skills.ts +++ b/core/mcp/skills.ts @@ -126,6 +126,7 @@ export type SkillIssueCode = | "missing-digest" | "malformed-digest" | "missing-size" + | "malformed-size" | "duplicate-resource" | "resource-outside-skill-root" | "manifest-missing-self" @@ -288,6 +289,13 @@ export function checkSkillConformance(entry: SkillEntry): SkillIssue[] { "Manifest entry declares no size, so it is excluded from the 16 MiB total and its length cannot be cross-checked.", resourceUri: resource.uri, }); + } else if (!isUsableSize(resource.size)) { + issues.push({ + code: "malformed-size", + severity: "error", + message: `Size ${resource.size} is not a non-negative integer byte length.`, + resourceUri: resource.uri, + }); } } @@ -295,13 +303,28 @@ export function checkSkillConformance(entry: SkillEntry): SkillIssue[] { } /** - * Sum of the manifest's declared `size` fields. An entry that omits `size` - * contributes nothing rather than failing the sum — the limit check is about - * catching a server that is demonstrably over, and an incomplete manifest can - * only ever understate the total, so this never produces a false positive. + * Whether a declared `size` is a usable byte length. SEP-2640 defines it as the + * raw byte count, so anything that is not a non-negative safe integer is + * nonsense — and a *negative* one is worse than nonsense, because summing it + * would pull the manifest total back under the 16 MiB limit and hide a + * violation. Reported as `malformed-size` and excluded from the sum. + */ +function isUsableSize(size: number | undefined): size is number { + return size !== undefined && Number.isSafeInteger(size) && size >= 0; +} + +/** + * Sum of the manifest's declared `size` fields. An entry that omits `size` — or + * declares an unusable one — contributes nothing rather than failing the sum: + * the limit check is about catching a server that is demonstrably over, and an + * incomplete manifest can only ever understate the total, so this never + * produces a false positive. */ export function totalSkillBytes(resources: readonly SkillResource[]): number { - return resources.reduce((sum, r) => sum + (r.size ?? 0), 0); + return resources.reduce( + (sum, r) => sum + (isUsableSize(r.size) ? r.size : 0), + 0, + ); } /** Outcome of comparing a fetched file against its advertised digest. */ diff --git a/core/mcp/skillsSchemas.ts b/core/mcp/skillsSchemas.ts index bf2d36a5..1441b3ea 100644 --- a/core/mcp/skillsSchemas.ts +++ b/core/mcp/skillsSchemas.ts @@ -105,39 +105,26 @@ export const ListSkillsResultSchema = z.looseObject({ export type ListSkillsResult = z.infer; /** - * The envelope form of a `skills/get` result: the entry wrapped under `skill`. + * The `skills/get` result envelope: the entry wrapped under `skill`. + * + * 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 + * accepting the inline form would silently normalize a non-conforming response + * — which is exactly the failure this extension's support exists to *report*. + * A server that returns the entry inline now fails the parse, loudly. */ const GetSkillEnvelopeSchema = z.looseObject({ skill: SkillEntrySchema }); /** - * Collapse either accepted `skills/get` shape to the entry it carries. + * `skills/get` result, unwrapped to the entry it carries. * - * Written as a parse rather than an `in` check because both accepted shapes are - * loose objects — they carry an index signature, so `"skill" in result` narrows - * nothing and would leave the extracted value `unknown`. Parsing the envelope - * is what proves its `skill` really is an entry, with no cast anywhere. + * The transform means every caller receives a `SkillEntry` and none of them + * reaches into the envelope; the strictness lives in the schema. */ -export function normalizeGetSkillResult(result: unknown): SkillEntry { - const enveloped = GetSkillEnvelopeSchema.safeParse(result); - return enveloped.success - ? enveloped.data.skill - : SkillEntrySchema.parse(result); -} - -/** - * `skills/get` result, normalized to the entry. - * - * ⚠️ The SEP settles the *entry* shape but not the envelope this result wraps it - * in, so both plausible forms are accepted: `{ skill: }` and the entry - * returned inline at the top level. Being permissive here costs nothing (the - * two are structurally distinguishable — an inline entry carries `uri` and - * `frontmatter`, the envelope carries neither) and spares a server author a - * failure whose cause is a spec ambiguity rather than their code. The transform - * means every caller receives the entry and none of them branches. - */ -export const GetSkillResultSchema = z - .union([GetSkillEnvelopeSchema, SkillEntrySchema]) - .transform(normalizeGetSkillResult); +export const GetSkillResultSchema = GetSkillEnvelopeSchema.transform( + (result) => result.skill, +); export type GetSkillResult = SkillEntry; diff --git a/core/mcp/state/managedSkillsState.ts b/core/mcp/state/managedSkillsState.ts index d45aa115..80866333 100644 --- a/core/mcp/state/managedSkillsState.ts +++ b/core/mcp/state/managedSkillsState.ts @@ -63,9 +63,9 @@ export class ManagedSkillsState extends TypedEventTarget void) | null = null; - // 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; + // Overlap guard, held as the generation whose walk is in flight (or `null`). + // See `refresh` for why this is not a boolean. + private runningGeneration: number | null = null; // 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; @@ -151,8 +151,6 @@ export class ManagedSkillsState extends TypedEventTarget this.generation === generation; + // The overlap guard is per SESSION, not a bare boolean. A boolean would + // stay set while a walk from a dead session was still awaiting the server, + // so the reconnect's own load would return here as a no-op and never be + // retried — permanently, if that stale request never settles. Keying it on + // the generation lets a new session start immediately, and the `finally` + // below only clears the guard it actually set. + if (this.runningGeneration === generation) return this.getSkills(); + this.runningGeneration = generation; try { const collected: SkillEntry[] = []; const seen = new Set(); @@ -199,7 +205,11 @@ export class ManagedSkillsState extends TypedEventTarget { (candidate) => `skill://${candidate.path}/SKILL.md` === uri, ); if (!skill) throw new Error(`Unknown skill uri: ${uri}`); - // The envelope form (`{ skill }`) is served deliberately: the SEP settles the - // entry shape but not this wrapper, and the Inspector accepts both — serving - // the wrapped one keeps that tolerance exercised. + // The envelope (`{ skill }`) is the conforming shape, and the only one the + // Inspector accepts — see `GetSkillResultSchema`. return { skill: toEntry(skill) }; }