diff --git a/clients/web/src/components/screens/SkillsScreen/SkillsScreen.test.tsx b/clients/web/src/components/screens/SkillsScreen/SkillsScreen.test.tsx index b5be49fc..739ae968 100644 --- a/clients/web/src/components/screens/SkillsScreen/SkillsScreen.test.tsx +++ b/clients/web/src/components/screens/SkillsScreen/SkillsScreen.test.tsx @@ -655,12 +655,12 @@ describe("SkillsScreen", () => { expect(screen.queryByTestId("skills-get-result")).not.toBeInTheDocument(); }); - it("keeps Verify all disabled on returning to a skill whose batch is still running", async () => { - // This is what actually blocks a second batch for one manifest: the button - // is disabled whenever the in-flight batch's key matches the selection, so - // A → B → A comes back to a disabled button rather than a second pool. - // (`batch` also carries a per-invocation token, so a finalizer can only - // ever clear its own batch — belt and braces for the same property.) + it("keeps Verify all disabled per skill while batches on other skills run", async () => { + // A → *start B's batch too* → back to A. That middle step is the one that + // matters: with a single slot instead of a map, starting B's batch + // overwrote A's, so A's button read as free and a second pool of workers + // could be started on top of A's first — doubling the concurrency cap the + // button exists to hold. const user = userEvent.setup(); const onReadSkillFile = vi.fn( () => new Promise<{ text: string }>(() => {}), @@ -668,19 +668,21 @@ describe("SkillsScreen", () => { renderWithMantine( , ); - await user.click(screen.getByText("data-analysis")); - await user.click(screen.getByRole("button", { name: /Verify all/ })); - expect(screen.getByRole("button", { name: /Verify all/ })).toBeDisabled(); + const verifyAll = () => screen.getByRole("button", { name: /Verify all/ }); - // B is free to run its own batch... + await user.click(screen.getByText("data-analysis")); + await user.click(verifyAll()); + expect(verifyAll()).toBeDisabled(); + + // B is free to run its own batch, and does. await user.click(screen.getByText("tampered")); - expect( - screen.getByRole("button", { name: /Verify all/ }), - ).not.toBeDisabled(); + expect(verifyAll()).not.toBeDisabled(); + await user.click(verifyAll()); + expect(verifyAll()).toBeDisabled(); - // ...and returning to A finds its batch still in flight. + // Returning to A still finds A's own batch in flight. await user.click(screen.getByText("data-analysis")); - expect(screen.getByRole("button", { name: /Verify all/ })).toBeDisabled(); + expect(verifyAll()).toBeDisabled(); }); it("shows the SKILL.md preview on demand", async () => { diff --git a/clients/web/src/components/screens/SkillsScreen/SkillsScreen.tsx b/clients/web/src/components/screens/SkillsScreen/SkillsScreen.tsx index 3dfcd343..bd7b09e7 100644 --- a/clients/web/src/components/screens/SkillsScreen/SkillsScreen.tsx +++ b/clients/web/src/components/screens/SkillsScreen/SkillsScreen.tsx @@ -340,18 +340,20 @@ export function SkillsScreen({ const [fetchedEntry, setFetchedEntry] = useState({ key: null, }); - // The "Verify all" batch in flight, as the manifest it belongs to plus a - // token unique to that invocation, or `null`. + // Every "Verify all" batch in flight, keyed by the manifest it belongs to. // - // The key alone would be a bare boolean's problem one level up: a global flag - // leaves a NEWLY selected skill's button disabled until the previous skill's - // reads settle (indefinitely, if one hangs), and a key-only guard lets two - // batches for the SAME manifest clear each other — start on A, switch to B, - // return to A and start again, and the first A batch's finalizer sees a - // matching key and frees the button while the second is still running, - // re-opening the concurrency cap it exists to hold. - const [batch, setBatch] = useState<{ key: string; token: number } | null>( - null, + // A **map**, not one slot, and the reason is a bug a single slot really had: + // batches on different skills genuinely overlap, so a slot remembers only the + // most recent one. Start A, switch to B and start B, return to A — the slot + // now says B, A's button reads as free, and clicking it starts a SECOND pool + // of workers for A on top of the first, doubling the concurrency cap. Keyed + // by manifest, A stays disabled for exactly as long as A's batch runs. + // + // The value is the invocation's token, so a finalizer deletes only its own + // entry; and a per-manifest entry is what keeps a hung batch on one skill + // from disabling every other skill's button. + const [batches, setBatches] = useState>( + () => new Map(), ); // Monotonic attempt token, shared by every on-demand action here: a manifest // row's verification, the SKILL.md preview, and the `skills/get` fetch. One @@ -476,14 +478,21 @@ export function SkillsScreen({ }; const workers = Math.min(VERIFY_CONCURRENCY, manifest.length); const token = (nextAttempt.current += 1); - setBatch({ key, token }); + setBatches((prev) => new Map(prev).set(key, token)); // The concurrency cap is per invocation, so without the button being // disabled below, a second click would start a second pool of four and a // third would make it twelve — the flood the cap exists to prevent. void Promise.all(Array.from({ length: workers }, () => worker())).finally( - // Clears only ITS OWN invocation: matched on the token, not the key, so - // an earlier batch settling cannot free a button a later one is holding. - () => setBatch((prev) => (prev?.token === token ? null : prev)), + // Clears only ITS OWN invocation: matched on the token as well as the + // key, so an earlier batch settling cannot free a button a later one is + // holding. + () => + setBatches((prev) => { + if (prev.get(key) !== token) return prev; + const next = new Map(prev); + next.delete(key); + return next; + }), ); }, [manifest, manifestKey, verifyRow]); @@ -534,7 +543,7 @@ export function SkillsScreen({ }, [manifestKey, onGetSkill, selected]); const fetched = fetchedEntry.key === manifestKey ? fetchedEntry : undefined; - const batchRunning = batch?.key === manifestKey; + const batchRunning = batches.has(manifestKey); const preview = previewState.key === manifestKey ? previewState.contents : undefined; diff --git a/test-servers/src/skills.ts b/test-servers/src/skills.ts index ace50d87..18d2edca 100644 --- a/test-servers/src/skills.ts +++ b/test-servers/src/skills.ts @@ -238,7 +238,7 @@ for (const skill of FIXTURE_SKILLS) { } /** The wire entry for one fixture skill. */ -function toEntry(skill: FixtureSkill): Record { +function toEntry(skill: FixtureSkill): z.infer { return { uri: `skill://${skill.path}/SKILL.md`, frontmatter: skill.frontmatter, @@ -254,7 +254,9 @@ function toEntry(skill: FixtureSkill): Record { } /** One `skills/list` page starting at `cursor` (an index, as a string). */ -export function listSkillsPage(cursor?: string): Record { +export function listSkillsPage( + cursor?: string, +): z.infer { const start = cursor ? Number.parseInt(cursor, 10) : 0; // A cursor the fixture never issued is answered as an empty final page // rather than an error: the Inspector's walk should terminate, and a thrown @@ -270,7 +272,9 @@ export function listSkillsPage(cursor?: string): Record { } /** The `skills/get` result for one entry URI. */ -export function getSkillEntry(uri: string): Record { +export function getSkillEntry( + uri: string, +): z.infer { const skill = FIXTURE_SKILLS.find( (candidate) => `skill://${candidate.path}/SKILL.md` === uri, ); @@ -320,6 +324,51 @@ interface UriRequest { const ListSkillsParamsSchema = z.object({ cursor: z.string().optional() }); const GetSkillParamsSchema = z.object({ uri: z.string() }); +/** + * Result schemas for the two custom methods. + * + * ⚠️ The SDK does **not** runtime-validate a handler's result — its own doc on + * `RequestHandlerSchemas` says `result` is optional and "no runtime validation + * is performed on the result". So these do not make the fixture's output + * checked at the server boundary; what they buy is that the handler's return + * type is inferred from them, so a shape change in `toEntry` or + * `listSkillsPage` fails `tsc` instead of silently shipping a fixture that + * claims to be conforming. That is the whole benefit, and it is worth having + * for a fixture whose job is to be wrong only in documented ways. + * + * Deliberately declared here rather than imported from + * `core/mcp/skillsSchemas.ts`: a fixture that validated itself against the + * client's own schema could never catch the client being wrong. + */ +const ModernEnvelopeShape = { + resultType: z.literal("complete"), + ttlMs: z.int().min(0), + cacheScope: z.enum(["public", "private"]), +}; + +const SkillResourceShape = z.object({ + uri: z.string(), + digest: z.string(), + size: z.number(), +}); + +const SkillEntryShape = z.object({ + uri: z.string(), + frontmatter: z.object({ name: z.string(), description: z.string() }), + resources: z.union([z.literal("dynamic"), z.array(SkillResourceShape)]), +}); + +const ListSkillsResultShape = z.object({ + ...ModernEnvelopeShape, + skills: z.array(SkillEntryShape), + nextCursor: z.string().optional(), +}); + +const GetSkillResultShape = z.object({ + ...ModernEnvelopeShape, + skill: SkillEntryShape, +}); + /** * Wire `skills/list`, `skills/get` and the `skill://` half of `resources/read` * onto an `McpServer`. @@ -329,13 +378,13 @@ export function wireSkillsHandlers(mcpServer: McpServer): void { lowLevel.setRequestHandler( "skills/list", - { params: ListSkillsParamsSchema }, + { params: ListSkillsParamsSchema, result: ListSkillsResultShape }, async (params) => listSkillsPage(params.cursor), ); lowLevel.setRequestHandler( "skills/get", - { params: GetSkillParamsSchema }, + { params: GetSkillParamsSchema, result: GetSkillResultShape }, async (params) => getSkillEntry(params.uri), );