fix: address Copilot review round 7 on #2251

- skills.ts: stop sorting `resources` arrays recursively. Only the entry's
  OWN manifest is a set; `frontmatter` is verbatim arbitrary JSON, so a
  custom `frontmatter.metadata.resources` array was being sorted too and
  two genuinely different frontmatters compared equal. The manifest sort is
  now done once, at the top level, and every other array keeps its order.
- SkillsScreen: invalidate on the whole selected entry, not just its
  manifest. The `skills/get` comparison covers `frontmatter`, so a refresh
  that changed only a description left a stale "matches skills/list"
  verdict on screen — computed against an entry that no longer existed.
  Re-verifying after a metadata-only refresh is the cheap direction to be
  wrong in; showing a match against a different entry is not.
- SkillsScreen: give each "Verify all" invocation a token, so a finalizer
  can only ever clear its own batch rather than any batch with a matching
  manifest key.
- SkillsScreen.stories: supply `onGetSkill`. Every story rendered the
  "Fetch with skills/get" button, and clicking it threw.

On the batch token: the A → B → A sequence the review describes is not
actually reachable, because the button is disabled whenever the in-flight
batch's key matches the selection — returning to A finds it disabled, not
clickable. The token is defence in depth for the same property, and the
test asserts what is really guaranteed rather than the unreachable path.

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.stories.tsx

@@ -99,6 +99,13 @@ const meta: Meta<typeof SkillsScreen> = {
         ? { text: REF_TEXT }
         : { text: SELF_TEXT, mimeType: "text/markdown" },
     ),
+    // Echoes back the entry `skills/list` advertised, so "Fetch with
+    // skills/get" demonstrates the matching case rather than throwing.
+    onGetSkill: fn(async (uri: string) => {
+      const found = sampleSkills.find((skill) => skill.uri === uri);
+      if (!found) throw new Error(`Unknown skill uri: ${uri}`);
+      return found;
+    }),
   },
   render: (args) => <StatefulSkillsScreen {...args} />,
 };

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

@@ -615,6 +615,74 @@ describe("SkillsScreen", () => {
     ).toBeInTheDocument();
   });
 
+  it("drops the skills/get verdict when a refresh changes only metadata", async () => {
+    // The manifest is untouched, so a manifest-only invalidation key would
+    // leave "matches" on screen even though it was computed against the
+    // previous entry — and that comparison covers `frontmatter` too.
+    const user = userEvent.setup();
+    const onGetSkill = vi.fn().mockResolvedValue(CLEAN_SKILL);
+    const { rerender } = renderWithMantine(
+      <SkillsScreen
+        {...baseProps}
+        skills={[CLEAN_SKILL]}
+        onGetSkill={onGetSkill}
+        ui={{ ...EMPTY_SKILLS_UI, selectedSkillUri: CLEAN_SKILL.uri }}
+      />,
+    );
+    await user.click(
+      screen.getByRole("button", { name: /Fetch with skills\/get/ }),
+    );
+    expect(
+      await screen.findByText("skills/get matches skills/list"),
+    ).toBeInTheDocument();
+
+    rerender(
+      <SkillsScreen
+        {...baseProps}
+        skills={[
+          {
+            ...CLEAN_SKILL,
+            frontmatter: {
+              ...CLEAN_SKILL.frontmatter,
+              description: "reworded since the fetch",
+            },
+          },
+        ]}
+        onGetSkill={onGetSkill}
+        ui={{ ...EMPTY_SKILLS_UI, selectedSkillUri: CLEAN_SKILL.uri }}
+      />,
+    );
+    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.)
+    const user = userEvent.setup();
+    const onReadSkillFile = vi.fn(
+      () => new Promise<{ text: string }>(() => {}),
+    );
+    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();
+
+    // B is free to run its own batch...
+    await user.click(screen.getByText("tampered"));
+    expect(
+      screen.getByRole("button", { name: /Verify all/ }),
+    ).not.toBeDisabled();
+
+    // ...and returning to A finds its batch still in flight.
+    await user.click(screen.getByText("data-analysis"));
+    expect(screen.getByRole("button", { name: /Verify all/ })).toBeDisabled();
+  });
+
   it("shows the SKILL.md preview on demand", async () => {
     const user = userEvent.setup();
     renderWithMantine(<ControlledSkillsScreen />);

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

@@ -340,11 +340,19 @@ export function SkillsScreen({
   const [fetchedEntry, setFetchedEntry] = useState<FetchedEntryState>({
     key: null,
   });
-  // The manifest whose "Verify all" batch is in flight, or `null`. Keyed rather
-  // than a bare boolean: a global flag would leave a NEWLY selected skill's
-  // button disabled until the previous skill's reads settled — indefinitely, if
-  // one of them hangs.
-  const [batchKey, setBatchKey] = useState<string | null>(null);
+  // The "Verify all" batch in flight, as the manifest it belongs to plus a
+  // token unique to that invocation, or `null`.
+  //
+  // 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,
+  );
   // 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
   // counter rather than three because it only has to be *increasing*, and each
@@ -380,20 +388,18 @@ export function SkillsScreen({
     [selected],
   );
 
-  // What every verdict on screen is a verdict *about*: the selected skill AND
-  // the manifest it advertised. Keying invalidation on the URI alone would
-  // leave a green `verified` badge attached to a digest the Refresh replaced,
-  // so the UI would vouch for content it has never checked. A primitive string
-  // rather than the manifest object, because `useValueChange` compares with
-  // `Object.is` and a fresh array every render would loop.
+  // What every result on screen is a result *about*: the selected skill entry,
+  // in full. Keying on the URI alone would leave a green `verified` badge
+  // attached to a digest a Refresh replaced; keying on the manifest alone would
+  // leave a stale "skills/get matches skills/list" verdict after a
+  // metadata-only change, since that comparison covers `frontmatter` too.
+  // Re-verifying after a metadata-only refresh is the cheap direction to be
+  // wrong in; showing a match that was computed against a different entry is
+  // not. A primitive string, because `useValueChange` compares with `Object.is`
+  // and a fresh object every render would loop.
   const manifestKey = useMemo(
-    () =>
-      [
-        selectedSkillUri ?? "",
-        selected?.resources === DYNAMIC_RESOURCES ? "dynamic" : "",
-        ...manifest.map((r) => `${r.uri}|${r.digest ?? ""}|${r.size ?? ""}`),
-      ].join("\n"),
-    [manifest, selected, selectedSkillUri],
+    () => (selected ? JSON.stringify(selected) : (selectedSkillUri ?? "")),
+    [selected, selectedSkillUri],
   );
 
   // Adjusted DURING RENDER via `useValueChange` rather than in an effect, so a
@@ -469,14 +475,15 @@ export function SkillsScreen({
       }
     };
     const workers = Math.min(VERIFY_CONCURRENCY, manifest.length);
-    setBatchKey(key);
+    const token = (nextAttempt.current += 1);
+    setBatch({ 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 batch: a stale finalizer must not free a button the
-      // user has since re-armed on another skill.
-      () => setBatchKey((prev) => (prev === key ? null : prev)),
+      // 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)),
     );
   }, [manifest, manifestKey, verifyRow]);
 
@@ -527,7 +534,7 @@ export function SkillsScreen({
   }, [manifestKey, onGetSkill, selected]);
 
   const fetched = fetchedEntry.key === manifestKey ? fetchedEntry : undefined;
-  const batchRunning = batchKey === manifestKey;
+  const batchRunning = batch?.key === manifestKey;
 
   const preview =
     previewState.key === manifestKey ? previewState.contents : undefined;

clients/web/src/test/core/mcp/skills.test.ts

@@ -10,6 +10,7 @@ import {
   getSkillsExtension,
   isSkillsExtensionSupported,
   normalizeSkillUri,
+  skillEntriesMatch,
   sha256Digest,
   skillDisplayName,
   skillNameFromUri,
@@ -172,6 +173,68 @@ describe("normalizeSkillUri", () => {
   });
 });
 
+describe("skillEntriesMatch", () => {
+  const base = (): SkillEntry => ({
+    uri: "skill://demo/SKILL.md",
+    frontmatter: { name: "demo", description: "d" },
+    resources: [
+      { uri: "skill://demo/SKILL.md", digest: DIGEST, size: 20 },
+      { uri: "skill://demo/ref.md", digest: DIGEST, size: 10 },
+    ],
+  });
+
+  it("ignores object key order", () => {
+    const reordered = {
+      resources: base().resources,
+      uri: base().uri,
+      frontmatter: { description: "d", name: "demo" },
+    };
+    expect(skillEntriesMatch(base(), reordered)).toBe(true);
+  });
+
+  it("ignores manifest order, because the manifest is a set", () => {
+    const reversed = {
+      ...base(),
+      resources: [...(base().resources as object[])].reverse(),
+    } as SkillEntry;
+    expect(skillEntriesMatch(base(), reversed)).toBe(true);
+  });
+
+  it("does NOT reorder an array nested in frontmatter", () => {
+    // `frontmatter` is verbatim arbitrary JSON from the skill author, so a
+    // custom `resources` array inside it is an ordinary list. A recursive
+    // sort would make these two genuinely different entries compare equal.
+    const withNested = (order: string[]): SkillEntry => ({
+      ...base(),
+      frontmatter: {
+        ...base().frontmatter,
+        metadata: { resources: order.map((uri) => ({ uri })) },
+      },
+    });
+    expect(
+      skillEntriesMatch(withNested(["a", "b"]), withNested(["b", "a"])),
+    ).toBe(false);
+    expect(
+      skillEntriesMatch(withNested(["a", "b"]), withNested(["a", "b"])),
+    ).toBe(true);
+  });
+
+  it("still sees a real difference", () => {
+    expect(
+      skillEntriesMatch(base(), {
+        ...base(),
+        frontmatter: { name: "demo", description: "changed" },
+      }),
+    ).toBe(false);
+  });
+
+  it("compares the dynamic form without sorting it", () => {
+    const dynamic = { ...base(), resources: "dynamic" } as SkillEntry;
+    expect(skillEntriesMatch(dynamic, { ...dynamic })).toBe(true);
+    expect(skillEntriesMatch(dynamic, base())).toBe(false);
+  });
+});
+
 describe("skillDisplayName", () => {
   it("prefers the declared frontmatter name", () => {
     expect(skillDisplayName(entry())).toBe("demo");

core/mcp/skills.ts

@@ -411,31 +411,42 @@ export function totalSkillBytes(resources: readonly SkillResource[]): number {
  * presents it as "the snapshot moved" and leaves the judgement to the reader.
  */
 export function skillEntriesMatch(a: SkillEntry, b: SkillEntry): boolean {
-  return canonicalJson(a) === canonicalJson(b);
+  return canonicalEntry(a) === canonicalEntry(b);
 }
 
-/** Deterministic JSON: object keys sorted recursively, and a skill's manifest
- *  sorted by URI so its enumeration order carries no meaning. */
-function canonicalJson(value: unknown): string {
-  return JSON.stringify(canonicalize(value));
+/**
+ * Deterministic JSON for one entry: object keys sorted recursively, and **the
+ * entry's own manifest** — nothing else — sorted by URI.
+ *
+ * The manifest sort is deliberately not recursive. `frontmatter` is verbatim
+ * arbitrary JSON from the skill author, so a custom `frontmatter.metadata.
+ * resources` array would be caught by a recursive rule and two genuinely
+ * different frontmatters would compare equal. Only `SkillEntry.resources` is
+ * a set; every other array keeps its order.
+ */
+function canonicalEntry(entry: SkillEntry): string {
+  const { resources, ...rest } = entry;
+  const manifest = Array.isArray(resources)
+    ? [...resources]
+        .sort((x, y) => String(x?.uri).localeCompare(String(y?.uri)))
+        .map(canonicalize)
+    : resources;
+  return JSON.stringify({ ...sortKeys(rest), resources: manifest });
 }
 
+/** Object keys sorted recursively; array ORDER is preserved throughout. */
 function canonicalize(value: unknown): unknown {
   if (Array.isArray(value)) return value.map(canonicalize);
   if (value === null || typeof value !== "object") return value;
-  const entries = Object.entries(value as Record<string, unknown>)
-    .map(([key, member]): [string, unknown] => {
-      // The manifest is a set; only its contents are meaningful.
-      if (key === "resources" && Array.isArray(member)) {
-        const sorted = [...(member as SkillResource[])].sort((x, y) =>
-          String(x?.uri).localeCompare(String(y?.uri)),
-        );
-        return [key, sorted.map(canonicalize)];
-      }
-      return [key, canonicalize(member)];
-    })
-    .sort(([x], [y]) => x.localeCompare(y));
-  return Object.fromEntries(entries);
+  return sortKeys(value as Record<string, unknown>);
+}
+
+function sortKeys(value: Record<string, unknown>): Record<string, unknown> {
+  return Object.fromEntries(
+    Object.entries(value)
+      .map(([key, member]): [string, unknown] => [key, canonicalize(member)])
+      .sort(([x], [y]) => x.localeCompare(y)),
+  );
 }
 
 /** Outcome of comparing a fetched file against its advertised digest. */