fix: address Copilot review round 9 on #2251

- SkillsScreen: track "Verify all" batches in a MAP keyed by manifest, not
  one slot. A slot remembers only the most recent batch, so A running →
  start B → return to A left A's button enabled and a second worker pool
  could be started on top of A's first, doubling the concurrency cap. The
  round-7 test switched to B but never STARTED B's batch, which is exactly
  why it missed this; it now does, and fails against the old single slot.
- test-servers/skills.ts: pass `result` schemas to `setRequestHandler` for
  both custom methods, and type the three builders from them, so a shape
  change in `toEntry` / `listSkillsPage` fails `tsc` rather than shipping a
  fixture that claims to conform. Declared locally rather than imported
  from `core/mcp/skillsSchemas.ts`: a fixture validated against the
  client's own schema could never catch the client being wrong.

One correction to the review's premise on that second point: the SDK does
NOT require `{ params, result }`, and supplying `result` does not validate
the response. Its own doc on `RequestHandlerSchemas` says `result` is
optional and "no runtime validation is performed on the result" — it types
the handler's return value. The change is worth making for that
compile-time check; it does not do what was claimed.

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>

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(
       <ControlledSkillsScreen onReadSkillFile={onReadSkillFile} />,
     );
-    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 () => {

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

@@ -340,18 +340,20 @@ export function SkillsScreen({
   const [fetchedEntry, setFetchedEntry] = useState<FetchedEntryState>({
     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<ReadonlyMap<string, number>>(
+    () => 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;

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<string, unknown> {
+function toEntry(skill: FixtureSkill): z.infer<typeof SkillEntryShape> {
   return {
     uri: `skill://${skill.path}/SKILL.md`,
     frontmatter: skill.frontmatter,
@@ -254,7 +254,9 @@ function toEntry(skill: FixtureSkill): Record<string, unknown> {
 }
 
 /** One `skills/list` page starting at `cursor` (an index, as a string). */
-export function listSkillsPage(cursor?: string): Record<string, unknown> {
+export function listSkillsPage(
+  cursor?: string,
+): z.infer<typeof ListSkillsResultShape> {
   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<string, unknown> {
 }
 
 /** The `skills/get` result for one entry URI. */
-export function getSkillEntry(uri: string): Record<string, unknown> {
+export function getSkillEntry(
+  uri: string,
+): z.infer<typeof GetSkillResultShape> {
   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),
   );