From 4e676a0a54242be42a4f4a996a0025ce40f4e58a Mon Sep 17 00:00:00 2001 From: cliffhall Date: Fri, 4 Sep 2026 23:21:51 -0400 Subject: [PATCH] fix: address Copilot review round 6 on #2251 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) Claude-Session: https://claude.ai/code/session_018kfrH3uRNUM4JAk1reKtyY Signed-off-by: cliffhall --- .../SkillsScreen/SkillsScreen.test.tsx | 64 +++++++++++++++ .../screens/SkillsScreen/SkillsScreen.tsx | 80 ++++++++++++------- docs/test-servers.md | 8 +- test-servers/src/skills.ts | 20 +++-- 4 files changed, 135 insertions(+), 37 deletions(-) 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