fix: address Copilot review round 6 on #2251

Most of this round re-reported code the previous commit already changed
(it generated 0 new comments). Two findings were genuinely open.

- SkillsScreen: give "View SKILL.md" and "Fetch with skills/get" the same
  attempt ordering `verifyRow` got in round 3. Both were still
  last-completion-wins within one manifest: a double click whose older
  request settled last replaced the newer result. One shared `isStale`
  helper now covers all three actions — a result is dropped when its
  manifest was invalidated OR a later click already wrote.
- test-servers/skills.ts + docs: stop calling all three edge cases
  "non-conforming". `resources: "dynamic"` is a LEGAL wire form for
  generated content — it is unverifiable, not invalid, which is exactly
  why the checker reports it as a warning. The prose contradicted the
  code it was describing. Now: one legal-but-unverifiable case and two
  actual violations.

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

@@ -551,6 +551,70 @@ describe("SkillsScreen", () => {
     ).not.toBeDisabled();
   });
 
+  it("keeps the newest SKILL.md preview when two reads overlap", async () => {
+    // Same skill, same manifest — the key cannot order these, so without an
+    // attempt token the older read finishing last would replace the newer
+    // preview with stale content.
+    const user = userEvent.setup();
+    const resolvers: ((value: { text: string }) => void)[] = [];
+    const onReadSkillFile = vi.fn(
+      () =>
+        new Promise<{ text: string }>((resolve) => {
+          resolvers.push(resolve);
+        }),
+    );
+    renderWithMantine(
+      <ControlledSkillsScreen onReadSkillFile={onReadSkillFile} />,
+    );
+    await user.click(screen.getByText("data-analysis"));
+    const view = screen.getByRole("button", { name: /View SKILL.md/ });
+    await user.click(view);
+    await user.click(view);
+    expect(resolvers).toHaveLength(2);
+
+    resolvers[1]({ text: "# newest\n" });
+    expect(await screen.findByTestId("skill-md-preview")).toHaveTextContent(
+      "newest",
+    );
+    resolvers[0]({ text: "# stale\n" });
+    expect(screen.getByTestId("skill-md-preview")).not.toHaveTextContent(
+      "stale",
+    );
+  });
+
+  it("keeps the newest skills/get result when two fetches overlap", async () => {
+    const user = userEvent.setup();
+    const resolvers: ((value: SkillEntry) => void)[] = [];
+    const onGetSkill = vi.fn(
+      () =>
+        new Promise<SkillEntry>((resolve) => {
+          resolvers.push(resolve);
+        }),
+    );
+    renderWithMantine(<ControlledSkillsScreen onGetSkill={onGetSkill} />);
+    await user.click(screen.getByText("data-analysis"));
+    const fetchButton = screen.getByRole("button", {
+      name: /Fetch with skills\/get/,
+    });
+    await user.click(fetchButton);
+    await user.click(fetchButton);
+    expect(resolvers).toHaveLength(2);
+
+    // The newer fetch matches; the older one, landing last, would otherwise
+    // overwrite it with a "different snapshot" verdict.
+    resolvers[1](CLEAN_SKILL);
+    expect(
+      await screen.findByText("skills/get matches skills/list"),
+    ).toBeInTheDocument();
+    resolvers[0]({
+      ...CLEAN_SKILL,
+      frontmatter: { ...CLEAN_SKILL.frontmatter, description: "stale" },
+    });
+    expect(
+      screen.getByText("skills/get matches skills/list"),
+    ).toBeInTheDocument();
+  });
+
   it("shows the SKILL.md preview on demand", async () => {
     const user = userEvent.setup();
     renderWithMantine(<ControlledSkillsScreen />);

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

@@ -74,9 +74,15 @@ interface VerificationState {
   files: Record<number, FileState>;
 }
 
-/** The SKILL.md preview, plus the manifest it belongs to (`null` as above). */
+/**
+ * The SKILL.md preview, plus the manifest it belongs to (`null` as above) and
+ * the click that produced it. The manifest key cannot order two reads issued
+ * for the SAME manifest, so without `attempt` a double click whose older read
+ * finishes last would replace the newer preview.
+ */
 interface PreviewState {
   key: string | null;
+  attempt?: number;
   contents?: SkillFileContents;
   message?: string;
 }
@@ -88,6 +94,8 @@ interface PreviewState {
  */
 interface FetchedEntryState {
   key: string | null;
+  /** The click this result belongs to — see {@link PreviewState.attempt}. */
+  attempt?: number;
   entry?: SkillEntry;
   matches?: boolean;
   message?: string;
@@ -274,6 +282,21 @@ function verificationLabel(state: FileState | undefined): string {
   return state.verification.status;
 }
 
+/**
+ * Whether a settled request should be discarded: its manifest was invalidated
+ * (a different key), or a later click for the same manifest already wrote (a
+ * higher attempt). A `null` key is the un-adopted initial manifest, which the
+ * first write claims.
+ */
+function isStale(
+  held: { key: string | null; attempt?: number },
+  key: string,
+  attempt: number,
+): boolean {
+  if (held.key !== null && held.key !== key) return true;
+  return held.attempt !== undefined && held.attempt > attempt;
+}
+
 /** `sha256:abcd…wxyz`, so a long digest stays readable in a table cell. */
 function shortDigest(digest: string | undefined): string {
   if (!digest) return "—";
@@ -322,8 +345,11 @@ export function SkillsScreen({
   // button disabled until the previous skill's reads settled — indefinitely, if
   // one of them hangs.
   const [batchKey, setBatchKey] = useState<string | null>(null);
-  // Monotonic per-row attempt token. A ref because it is claimed inside an
-  // event handler, never during render.
+  // 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
+  // consumer compares it against its own slot. A ref because it is claimed
+  // inside an event handler, never during render.
   const nextAttempt = useRef(0);
 
   const filtered = useMemo(() => {
@@ -457,48 +483,46 @@ export function SkillsScreen({
   const showSkillMd = useCallback(() => {
     if (!selected) return;
     const key = manifestKey;
+    const attempt = (nextAttempt.current += 1);
     // A click handler cannot await, and this chain terminates in its own
-    // `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.
+    // `catch` that surfaces the message in the preview slot. Both arms go
+    // through `writePreview`, which drops a result whose manifest has been
+    // invalidated OR whose click has been superseded.
+    const writePreview = (next: Omit<PreviewState, "key" | "attempt">) =>
+      setPreviewState((prev) =>
+        isStale(prev, key, attempt) ? prev : { key, attempt, ...next },
+      );
     void onReadSkillFile(selected.uri)
-      .then((contents) => {
-        setPreviewState((prev) =>
-          prev.key !== null && prev.key !== key ? prev : { key, contents },
-        );
-      })
+      .then((contents) => writePreview({ contents }))
       .catch((err: unknown) => {
-        const message = err instanceof Error ? err.message : String(err);
-        setPreviewState((prev) =>
-          prev.key !== null && prev.key !== key ? prev : { key, message },
-        );
+        writePreview({
+          message: err instanceof Error ? err.message : String(err),
+        });
       });
   }, [manifestKey, onReadSkillFile, selected]);
 
   const fetchEntry = useCallback(() => {
     if (!selected) return;
     const key = manifestKey;
+    const attempt = (nextAttempt.current += 1);
     // Same shape as the SKILL.md read: a click handler cannot await, the chain
-    // ends in its own `catch`, and both arms compare the manifest key they
-    // started under so a late answer cannot land under another skill.
+    // ends in its own `catch`, and both arms drop a result whose manifest has
+    // been invalidated or whose click has been superseded.
+    const writeFetched = (next: Omit<FetchedEntryState, "key" | "attempt">) =>
+      setFetchedEntry((prev) =>
+        isStale(prev, key, attempt) ? prev : { key, attempt, ...next },
+      );
     void onGetSkill(selected.uri)
       .then((entry) => {
         // Compared semantically against what `skills/list` advertised — see
         // `skillEntriesMatch` for why a `JSON.stringify` comparison would
         // report key order and manifest order as differences.
-        const matches = skillEntriesMatch(entry, selected);
-        setFetchedEntry((prev) =>
-          prev.key !== null && prev.key !== key
-            ? prev
-            : { key, entry, matches },
-        );
+        writeFetched({ entry, matches: skillEntriesMatch(entry, selected) });
       })
       .catch((err: unknown) => {
-        const message = err instanceof Error ? err.message : String(err);
-        setFetchedEntry((prev) =>
-          prev.key !== null && prev.key !== key ? prev : { key, message },
-        );
+        writeFetched({
+          message: err instanceof Error ? err.message : String(err),
+        });
       });
   }, [manifestKey, onGetSkill, selected]);
 

docs/test-servers.md

@@ -80,14 +80,16 @@ It works on **either era**: `skills/list`, `skills/get` and
 era codec defines, so the SDK's era gate skips them entirely — which is why
 this fixture, unlike the tasks ones, needs no per-era variant.
 
-Three of the four skills are deliberately non-conforming, because the checks
-the Skills tab runs are untestable without them:
+Three of the four skills are deliberately awkward, because the checks the Skills
+tab runs are untestable without them. Only two are actual violations — the
+`"dynamic"` form is **conforming**, and is here because "legal but unverifiable"
+is the case most easily buried:
 
 | Skill | What it exercises |
 | --- | --- |
 | `data-analysis` | The clean case — **Verify all** reports `verified` for every file. |
 | `tampered-notes` | An advertised digest that does not match the bytes served, so verification reports a **digest mismatch** with both digests shown. |
-| `dynamic-report` | `resources: "dynamic"` — a generated file set, so integrity cannot be verified at all and the tab says so rather than staying silent. |
+| `dynamic-report` | `resources: "dynamic"` — a **legal** form for generated content. No manifest is advertised, so integrity cannot be verified at all; reported as a warning, not an error. |
 | `wrong-folder` | A URI path segment (`wrong-folder`) that disagrees with `frontmatter.name` (`right-name`), the one structural invariant SEP-2640 states outright. |
 
 Connection Info's **Skills Extension Options** section shows the `directoryRead`

test-servers/src/skills.ts

@@ -5,12 +5,20 @@
  * the `skill://` URIs those entries name, so an Inspector connected here can
  * exercise the whole flow: enumerate, fetch a file, and verify its digest.
  *
- * **The non-conforming skills are the point.** A fixture that only served a
- * clean skill would leave every verification and conformance path in the
- * Inspector untestable, so the set below deliberately includes one `"dynamic"`
- * skill, one whose advertised digest does not match the bytes served, and one
- * whose URI path segment disagrees with `frontmatter.name`. Each is the exact
- * shape one of the checks in `core/mcp/skills.ts` exists to catch.
+ * **The awkward skills are the point.** A fixture that only served a clean
+ * skill would leave every verification and conformance path in the Inspector
+ * untestable, so the set below deliberately includes three edge cases — each
+ * the exact shape one of the checks in `core/mcp/skills.ts` exists to catch:
+ *
+ *  - `dynamic-report` declares `resources: "dynamic"`. That is a **conforming**
+ *    wire form for generated content, not a violation; what it costs is that
+ *    integrity cannot be verified at all, which the Inspector reports as a
+ *    warning. It is here because "legal but unverifiable" is the case most
+ *    easily buried.
+ *  - `tampered-notes` advertises a digest that does not match the bytes it
+ *    serves — a genuine violation.
+ *  - `wrong-folder` has a URI path segment that disagrees with
+ *    `frontmatter.name` — the other genuine violation.
  *
  * `skills/list` and `skills/get` are registered through the **public**
  * `setRequestHandler`, which accepts a consumer-owned method name as long as