diff --git a/clients/web/src/components/screens/SkillsScreen/SkillsScreen.test.tsx b/clients/web/src/components/screens/SkillsScreen/SkillsScreen.test.tsx
index 19cdb382..2d4a074e 100644
--- a/clients/web/src/components/screens/SkillsScreen/SkillsScreen.test.tsx
+++ b/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(
+ ,
+ );
+ 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((resolve) => {
+ resolvers.push(resolve);
+ }),
+ );
+ renderWithMantine();
+ 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();
diff --git a/clients/web/src/components/screens/SkillsScreen/SkillsScreen.tsx b/clients/web/src/components/screens/SkillsScreen/SkillsScreen.tsx
index b083d5ea..e4dbdc3b 100644
--- a/clients/web/src/components/screens/SkillsScreen/SkillsScreen.tsx
+++ b/clients/web/src/components/screens/SkillsScreen/SkillsScreen.tsx
@@ -74,9 +74,15 @@ interface VerificationState {
files: Record;
}
-/** 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(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) =>
+ 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) =>
+ 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]);
diff --git a/docs/test-servers.md b/docs/test-servers.md
index 5d4118de..4e214f34 100644
--- a/docs/test-servers.md
+++ b/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`
diff --git a/test-servers/src/skills.ts b/test-servers/src/skills.ts
index 49cd3071..b04444b7 100644
--- a/test-servers/src/skills.ts
+++ b/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