From 5a1f0efb5a2049067828fefe5ec2248e178b42a4 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Mon, 7 Sep 2026 22:58:19 -0400 Subject: [PATCH] fix: address Copilot review round 16 on #2248 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit **A hole my own round-13 change opened.** The byte budget added a `break`, and `manifestListsSelf` described the bounded slice rather than the rows the walk reached — so breaking before a later self-entry left the flag true, suppressed the fallback, and skipped the mandatory frontmatter check entirely. It is `selfAttempted` now, set inside the loop, and marked before the read: a row the walk reached but could not read has still been attempted, and its failure belongs to the loop rather than to a second fetch. **A preview could overwrite a verification's own bytes.** The digest verdict on screen was computed from the verification's fetch; replacing only the text let the frontmatter findings describe different bytes, recreating the mixed-fetch verdict that state exists to prevent. `entryTextVerified` marks which read produced it, and a preview no longer wins over a verification. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01ND46Ci64DC3tzBy9aG81Nc Signed-off-by: cliffhall --- .../SkillsScreen/SkillsScreen.test.tsx | 44 ++++++++++++++++++ .../screens/SkillsScreen/SkillsScreen.tsx | 42 ++++++++++++----- .../test/core/mcp/skillsVerification.test.ts | 45 +++++++++++++++++++ core/mcp/skillsVerification.ts | 33 +++++++++----- 4 files changed, 141 insertions(+), 23 deletions(-) diff --git a/clients/web/src/components/screens/SkillsScreen/SkillsScreen.test.tsx b/clients/web/src/components/screens/SkillsScreen/SkillsScreen.test.tsx index 2da4c578..ee4b74d3 100644 --- a/clients/web/src/components/screens/SkillsScreen/SkillsScreen.test.tsx +++ b/clients/web/src/components/screens/SkillsScreen/SkillsScreen.test.tsx @@ -2529,6 +2529,50 @@ describe("SkillsScreen frontmatter cross-check (#2248)", () => { ); }); + it("does not let a later preview overwrite a verification's own bytes", async () => { + // The digest verdict on screen was computed from the verification's fetch; + // replacing only the text would let the frontmatter findings describe + // different bytes, recreating the mixed-fetch verdict this state exists to + // prevent (Copilot). + const user = userEvent.setup(); + let served = skillMdFor({ ...CLEAN_FM, description: "As verified" }); + const onReadSkillFile = vi.fn(async (uri: string) => { + if (uri === "skill://data-analysis/reference.md") { + return { text: REF_TEXT }; + } + return { text: served, mimeType: "text/markdown" }; + }); + renderWithMantine( + , + ); + await user.click(screen.getByText("data-analysis")); + await user.click( + screen.getByRole("button", { + name: "Verify skill://data-analysis/SKILL.md", + }), + ); + await waitFor(() => + expect( + within(screen.getByTestId("skill-frontmatter-issues")).getByText( + /As verified/, + ), + ).toBeInTheDocument(), + ); + + // The server changes, and the reader re-opens the file in the viewer. The + // verification's text must survive, since its digest verdict still shows. + served = skillMdFor({ ...CLEAN_FM, description: "Changed after" }); + await user.click( + screen.getByRole("button", { name: "skill://data-analysis/SKILL.md" }), + ); + await waitFor(() => expect(onReadSkillFile).toHaveBeenCalledTimes(3)); + expect( + within(screen.getByTestId("skill-frontmatter-issues")).getByText( + /As verified/, + ), + ).toBeInTheDocument(); + }); + it("keeps a frontmatter finding when the reader opens another file", async () => { // The check ran off whatever the viewer was showing, so opening a // supporting file made `showingSkillMd` false and silently dropped the diff --git a/clients/web/src/components/screens/SkillsScreen/SkillsScreen.tsx b/clients/web/src/components/screens/SkillsScreen/SkillsScreen.tsx index d2370005..922514df 100644 --- a/clients/web/src/components/screens/SkillsScreen/SkillsScreen.tsx +++ b/clients/web/src/components/screens/SkillsScreen/SkillsScreen.tsx @@ -105,6 +105,18 @@ interface VerificationState { * has not clicked Verify should still get the check. */ entryText?: string; + /** + * True when {@link entryText} came from a **verification** rather than the + * preview read. + * + * A later preview of the same `SKILL.md` must not overwrite text a + * verification produced: the digest verdict on screen was computed from that + * fetch, and replacing only the text would let the frontmatter findings + * describe different bytes — recreating the mixed-fetch verdict this state + * exists to prevent (Copilot). A verification always wins, since it brings a + * matching digest verdict with it. + */ + entryTextVerified?: boolean; } /** @@ -963,9 +975,12 @@ export function SkillsScreen({ // anything not named here is dropped — which silently discarded the // verified `SKILL.md` text the frontmatter check depends on. ...(entryText !== undefined - ? { entryText } + ? { entryText, entryTextVerified: true } : prev.key === key && prev.entryText !== undefined - ? { entryText: prev.entryText } + ? { + entryText: prev.entryText, + entryTextVerified: prev.entryTextVerified, + } : {}), }; }); @@ -1076,16 +1091,19 @@ export function SkillsScreen({ } catch { return; // neither text nor blob; the viewer reports it } - setVerification((prev) => - prev.key !== null && prev.key !== key - ? prev - : { - ...prev, - key, - files: prev.key === key ? prev.files : {}, - entryText: text, - }, - ); + setVerification((prev) => { + if (prev.key !== null && prev.key !== key) return prev; + const sameKey = prev.key === key; + // Never over a verification's own text — see + // `entryTextVerified`. + if (sameKey && prev.entryTextVerified) return prev; + return { + ...prev, + key, + files: sameKey ? prev.files : {}, + entryText: text, + }; + }); } }) .catch((err: unknown) => { diff --git a/clients/web/src/test/core/mcp/skillsVerification.test.ts b/clients/web/src/test/core/mcp/skillsVerification.test.ts index a67a4ea1..3b46d24f 100644 --- a/clients/web/src/test/core/mcp/skillsVerification.test.ts +++ b/clients/web/src/test/core/mcp/skillsVerification.test.ts @@ -718,6 +718,51 @@ describe("verifySkills (#2248)", () => { ); }); + it("runs the fallback when the byte budget broke the loop before the self row", async () => { + // `manifestListsSelf` described the bounded SLICE, not the rows reached — + // so a break on the byte budget left it true, suppressed the fallback, and + // skipped the mandatory frontmatter check entirely (Copilot). + const md = "---\nname: late\ndescription: Served\n---\n\n# late\n"; + const big = "z".repeat(9 * 1024 * 1024); + const enc = new TextEncoder(); + const skill: SkillEntry = { + uri: "skill://late/SKILL.md", + frontmatter: { name: "late", description: "Listed" }, + resources: [ + // Two oversized files cross the budget before the self row is reached. + { + uri: "skill://late/a.md", + digest: await sha256Digest(enc.encode(big)), + size: 1, + }, + { + uri: "skill://late/b.md", + digest: await sha256Digest(enc.encode(big)), + size: 1, + }, + { + uri: "skill://late/SKILL.md", + digest: await sha256Digest(enc.encode(md)), + size: enc.encode(md).byteLength, + }, + ], + }; + const readResource = vi.fn(async (uri: string) => ({ + result: { + contents: [{ uri, text: uri.endsWith("/SKILL.md") ? md : big }], + }, + })); + const client = { readResource } as unknown as InspectorClientProtocol; + const [report] = await verifySkills(client, [skill]); + // The self file was still fetched, and its frontmatter still compared. + expect(readResource).toHaveBeenCalledWith( + "skill://late/SKILL.md", + undefined, + ); + expect(report.frontmatter).toHaveLength(1); + expect(report.frontmatter[0].code).toBe("frontmatter-mismatch"); + }); + it("does not truncate a conforming manifest", async () => { // A conforming skill totals at most 16 MiB by definition, so the bound // must never shorten one — otherwise it would trade a hostile-server diff --git a/core/mcp/skillsVerification.ts b/core/mcp/skillsVerification.ts index 62219783..efd7520a 100644 --- a/core/mcp/skillsVerification.ts +++ b/core/mcp/skillsVerification.ts @@ -294,14 +294,25 @@ export async function verifySkills( // oversized set; this is what stops one that lied. let receivedBytes = 0; const entryIdentity = skillUriIdentity(entry.uri); - // Compared by NORMALIZED identity, like every other URI comparison here — + // ⚠️ Whether the self entry was **actually reached**, not merely whether + // the bounded slice contains it. The byte budget can break the loop before + // a later self-entry, and a flag describing the slice stayed true — which + // suppressed the fallback and skipped the mandatory frontmatter check + // entirely (Copilot). Set inside the loop, so it can only be true of a row + // the walk got to. + // + // Compared by NORMALIZED identity, like every other URI comparison here: // `checkSkillConformance` already accepts a manifest self-entry written in // an equivalent form, so a raw string test would disagree with it and read // the same file a second time. - const manifestListsSelf = manifest.some( - (resource) => skillUriIdentity(resource.uri) === entryIdentity, - ); + let selfAttempted = false; for (const resource of manifest) { + if (skillUriIdentity(resource.uri) === entryIdentity) { + // Marked before the read, not after: a row the walk reached but could + // not read has still been attempted, and its failure is recorded here + // rather than re-attempted by the fallback. + selfAttempted = true; + } let contents: ReadContents | undefined; try { const invocation = await client.readResource(resource.uri, metadata); @@ -357,13 +368,13 @@ export async function verifySkills( // being unenumerable; only integrity is. The same applies to a skill whose // manifest omits its own file. // - // Gated on `manifestListsSelf` rather than on `entryBytes`, so a self-entry - // the loop already tried and FAILED to read is not read a second time — its - // failure is recorded there. Note this is computed over the READ slice, so - // a self-entry pushed past the cap by a bloated manifest still reaches the - // fallback: the frontmatter comparison is mandatory and must not be lost to - // a limit that exists to bound unrelated files. - if (!manifestListsSelf) { + // Gated on `selfAttempted` rather than on `entryBytes`, so a self-entry the + // loop already tried and FAILED to read is not read a second time — its + // failure is recorded there. A self-entry the walk never reached, whether + // because a cap excluded it or because the byte budget broke the loop + // first, still gets the fallback: the frontmatter comparison is mandatory + // and must not be lost to a limit that exists to bound unrelated files. + if (!selfAttempted) { // Recorded as a file result, not swallowed. Because a dynamic skill has // no manifest rows, `files` would otherwise stay empty and its only static // finding is a warning — so an unreadable SKILL.md returned `ok: true`