From 1f33ba93be90c7454182a7bdee6c636f4e10f359 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Fri, 4 Sep 2026 23:36:35 -0400 Subject: [PATCH] fix: address Copilot review round 7 on #2251 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - 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) Claude-Session: https://claude.ai/code/session_018kfrH3uRNUM4JAk1reKtyY Signed-off-by: cliffhall --- .../SkillsScreen/SkillsScreen.stories.tsx | 7 ++ .../SkillsScreen/SkillsScreen.test.tsx | 68 +++++++++++++++++++ .../screens/SkillsScreen/SkillsScreen.tsx | 53 ++++++++------- clients/web/src/test/core/mcp/skills.test.ts | 63 +++++++++++++++++ core/mcp/skills.ts | 47 ++++++++----- 5 files changed, 197 insertions(+), 41 deletions(-) diff --git a/clients/web/src/components/screens/SkillsScreen/SkillsScreen.stories.tsx b/clients/web/src/components/screens/SkillsScreen/SkillsScreen.stories.tsx index b1a97355..df786ba1 100644 --- a/clients/web/src/components/screens/SkillsScreen/SkillsScreen.stories.tsx +++ b/clients/web/src/components/screens/SkillsScreen/SkillsScreen.stories.tsx @@ -99,6 +99,13 @@ const meta: Meta = { ? { 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) => , }; diff --git a/clients/web/src/components/screens/SkillsScreen/SkillsScreen.test.tsx b/clients/web/src/components/screens/SkillsScreen/SkillsScreen.test.tsx index 2d4a074e..b5be49fc 100644 --- a/clients/web/src/components/screens/SkillsScreen/SkillsScreen.test.tsx +++ b/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( + , + ); + await user.click( + screen.getByRole("button", { name: /Fetch with skills\/get/ }), + ); + expect( + await screen.findByText("skills/get matches skills/list"), + ).toBeInTheDocument(); + + rerender( + , + ); + 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( + , + ); + 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(); diff --git a/clients/web/src/components/screens/SkillsScreen/SkillsScreen.tsx b/clients/web/src/components/screens/SkillsScreen/SkillsScreen.tsx index e4dbdc3b..3dfcd343 100644 --- a/clients/web/src/components/screens/SkillsScreen/SkillsScreen.tsx +++ b/clients/web/src/components/screens/SkillsScreen/SkillsScreen.tsx @@ -340,11 +340,19 @@ export function SkillsScreen({ const [fetchedEntry, setFetchedEntry] = useState({ 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(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; diff --git a/clients/web/src/test/core/mcp/skills.test.ts b/clients/web/src/test/core/mcp/skills.test.ts index 9787d3ba..5ea44b09 100644 --- a/clients/web/src/test/core/mcp/skills.test.ts +++ b/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"); diff --git a/core/mcp/skills.ts b/core/mcp/skills.ts index 2d9346a7..926c683a 100644 --- a/core/mcp/skills.ts +++ b/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) - .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); +} + +function sortKeys(value: Record): Record { + 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. */