From 52e8e1b5576c1058e02d9759db413aede942b159 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sun, 6 Sep 2026 01:40:33 -0400 Subject: [PATCH] fix: address Copilot review round 7 (#2263) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four findings — three inline, one suppressed. `verifyRow` opened the Conformance section, and every "Verify all" worker calls it once per row as it advances, so a batch begun on one skill kept calling it after the user had switched away. The keyed writes discard those results, but an open-state update is not keyed to a manifest, so obsolete work could reopen the section on the newly selected skill. The section is now opened at the two USER entry points — the row button and "Verify all" — through `openConformance`, and `verifyRow` carries a note saying why it must not do it itself. The skill description was an unbounded sibling of a zero-basis accordion in the pane's fixed header. SEP-2640 permits 1,024 characters and a non-conforming server can send more; in a narrow pane that wraps far enough to crowd the sections out — the same "server-controlled content sizes the layout" trap `viewerFlex` documents. Clamped to three lines, with the full text on a `title`. `inferMimeFromUri` matched against the whole URI minus query and fragment, so an authority ending in a mapped suffix was read as a filename: `https://documentation.md` returned Markdown despite a pathname of `/`. It now inspects `URL.pathname`, keeping the string fallback for URIs that cannot be parsed. And its decoding was too coarse. `decodeURIComponent` throws not only on a malformed escape but on a syntactically valid octet that is not valid UTF-8, so a single `%FF` anywhere in the path abandoned decoding for the whole string and `skill://a/%FF/reference%2Emd` lost its `%2E`. Each escape run is now decoded independently, and a run that is not valid UTF-8 falls back to decoding its ASCII octets one at a time — all suffix matching needs. Both behavioural fixes carry regression tests verified to fail with the defect restored. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01R4441JdNLZerCVM7mDDSfB Signed-off-by: cliffhall --- .../SkillsScreen/SkillsScreen.test.tsx | 56 +++++++++++++ .../screens/SkillsScreen/SkillsScreen.tsx | 83 ++++++++++++++----- .../web/src/utils/inferMimeFromUri.test.ts | 29 +++++++ clients/web/src/utils/inferMimeFromUri.ts | 61 ++++++++++---- 4 files changed, 191 insertions(+), 38 deletions(-) diff --git a/clients/web/src/components/screens/SkillsScreen/SkillsScreen.test.tsx b/clients/web/src/components/screens/SkillsScreen/SkillsScreen.test.tsx index 1eb2e116..b342701b 100644 --- a/clients/web/src/components/screens/SkillsScreen/SkillsScreen.test.tsx +++ b/clients/web/src/components/screens/SkillsScreen/SkillsScreen.test.tsx @@ -306,6 +306,62 @@ describe("SkillsScreen", () => { expect(badgeStyle(/1 mismatch\(es\)/)).toContain("red"); }); + it("a stale Verify all batch does not reopen Conformance on another skill", async () => { + // `verifyRow` is called once per row by every "Verify all" worker as it + // advances, so a batch begun on one skill keeps calling it after the user + // has moved on. The keyed writes discard those results, but an open-state + // update is not keyed to a manifest — so opening the section from inside + // `verifyRow` let obsolete work mutate the current pane (#2263). + const user = userEvent.setup(); + // Held open so the batch is still in flight when the selection changes. + const releases: (() => void)[] = []; + const onReadSkillFile = vi.fn( + () => + new Promise<{ text: string }>((resolve) => { + releases.push(() => resolve({ text: SELF_TEXT })); + }), + ); + // More rows than the concurrency cap, so workers keep pulling. + const manyRows: SkillEntry = { + ...CLEAN_SKILL, + uri: "skill://many/SKILL.md", + frontmatter: { name: "many", description: "Many rows" }, + resources: Array.from({ length: 10 }, (_, i) => ({ + uri: i === 0 ? "skill://many/SKILL.md" : `skill://many/f${i}.md`, + digest: SELF_DIGEST, + size: textToBytes(SELF_TEXT).byteLength, + })), + }; + renderWithMantine( + , + ); + await user.click(screen.getByText("many")); + await user.click(screen.getByRole("button", { name: /Verify all/ })); + + // Switch to a clean skill and collapse Conformance deliberately. + await user.click( + within(screen.getByTestId("skills-screen")).getAllByText( + "data-analysis", + )[0], + ); + await waitFor(() => + expect( + screen.getByRole("button", { name: /Conformance/ }), + ).toHaveAttribute("aria-expanded", "false"), + ); + + // Let the abandoned batch's workers advance. They must not reopen it. + for (const release of releases) release(); + await waitFor(() => expect(onReadSkillFile).toHaveBeenCalled()); + expect(screen.getByRole("button", { name: /Conformance/ })).toHaveAttribute( + "aria-expanded", + "false", + ); + }); + it("badges a warning-only entry yellow, not green", async () => { // Green reads as "nothing to see", which would hide the only signal the // section carries for an entry whose findings are all warnings (#2263). diff --git a/clients/web/src/components/screens/SkillsScreen/SkillsScreen.tsx b/clients/web/src/components/screens/SkillsScreen/SkillsScreen.tsx index afd39c66..dc2e22a3 100644 --- a/clients/web/src/components/screens/SkillsScreen/SkillsScreen.tsx +++ b/clients/web/src/components/screens/SkillsScreen/SkillsScreen.tsx @@ -273,6 +273,20 @@ const MonoCaption = Text.withProps({ variant: "monoCaption", }); +// The skill's description, clamped. +// +// It sits in the pane's FIXED header, beside an accordion whose basis is zero, +// so its height is subtracted from everything below it. SEP-2640 permits 1,024 +// characters and a non-conforming server can send more — in a narrow or short +// pane that wraps to enough lines to crowd the sections out entirely, which is +// the same "server-controlled content sizes the layout" trap `viewerFlex` +// documents. Three lines is enough to read a real description; the full text +// stays available through the native `title` tooltip the call site passes. +const SkillDescription = Text.withProps({ + size: "sm", + lineClamp: 3, +}); + const IssueStack = Stack.withProps({ gap: "xs", }); @@ -685,15 +699,33 @@ export function SkillsScreen({ * verifying either would update both, and "Verify all" would race two * different digest/size declarations into the same slot. */ + /** + * Reveal the Conformance section, which auto-collapses for an entry with no + * static findings — `tampered-notes` is exactly that: structurally clean, + * bytes wrong — so a verdict would otherwise land where nobody can see it. + * + * Called from the USER gestures that produce a verdict, never from the async + * work they start: an open-state update is not keyed to a manifest, so + * calling it from a continuation would let stale work mutate the current + * pane. + */ + const openConformance = useCallback(() => { + setOpenSections((prev) => + prev.includes("conformance") ? prev : [...prev, "conformance"], + ); + }, []); + const verifyRow = useCallback( async (index: number, resource: SkillResource, key: string) => { - // A mismatch is reported in the Conformance section, which auto-collapses - // for an entry with no *static* findings — and `tampered-notes` is - // exactly that: structurally clean, bytes wrong. Opening it here is what - // stops the verdict landing somewhere the user cannot see it. - setOpenSections((prev) => - prev.includes("conformance") ? prev : [...prev, "conformance"], - ); + // NOTE: opening the Conformance section deliberately does NOT happen + // here. `verifyRow` is called once per row by every "Verify all" worker + // as it advances, so a batch begun on one skill keeps calling it after + // the user has switched to another. The keyed writes above discard those + // continuations, but an open-state update is not keyed to a manifest and + // would have reopened Conformance on whatever skill is now selected. + // The section is opened at the two USER entry points instead — the row + // button and "Verify all" — where the gesture and the pane agree. + // // Claimed synchronously, so two verifications of this row are ordered // before either read starts. const attempt = (nextAttempt.current += 1); @@ -729,11 +761,11 @@ export function SkillsScreen({ ); const verifyAll = useCallback(() => { - // Same reason as `verifyRow`: the verdicts render in Conformance, which may - // be collapsed for a structurally clean entry. - setOpenSections((prev) => - prev.includes("conformance") ? prev : [...prev, "conformance"], - ); + // One of the two user entry points that opens Conformance — the verdicts + // render there, and it may be collapsed for a structurally clean entry. + // Done here rather than in `verifyRow` so a batch that outlives its own + // selection cannot reopen the section on a different skill. + openConformance(); // Bounded concurrency, not `Promise.all` over the whole manifest: a // conforming skill may declare 512 files, and firing 512 simultaneous // `resources/read` calls would bury the transport and the server for no @@ -768,7 +800,7 @@ export function SkillsScreen({ return next; }), ); - }, [manifest, manifestKey, verifyRow]); + }, [manifest, manifestKey, openConformance, verifyRow]); /** * Put one of the skill's files in the viewer. Driven both by the effect that @@ -840,10 +872,9 @@ export function SkillsScreen({ // The verdict renders inside the Conformance section, which auto-collapses // for a clean entry — and a clean entry is exactly the common case for this // button. Without this the answer would land in a collapsed section and the - // click would look like it did nothing. - setOpenSections((prev) => - prev.includes("conformance") ? prev : [...prev, "conformance"], - ); + // click would look like it did nothing. Called on the gesture, before the + // request goes out — see `openConformance`. + openConformance(); // Same shape as the SKILL.md read: a click handler cannot await, the chain // ends in its own `catch`, and both arms drop a result whose manifest has // been invalidated or whose click has been superseded. @@ -882,7 +913,7 @@ export function SkillsScreen({ message: err instanceof Error ? err.message : String(err), }); }); - }, [manifestKey, onGetSkill, selected]); + }, [manifestKey, onGetSkill, openConformance, selected]); const fetched = fetchedEntry.key === manifestKey ? fetchedEntry : undefined; // `invalid` outranks the snapshot comparison: an entry that breaks a @@ -1123,7 +1154,9 @@ export function SkillsScreen({ {selected.frontmatter.description && ( - {selected.frontmatter.description} + + {selected.frontmatter.description} + )} {/* Inline, not a `.withProps()` subcomponent: `Accordion` is a @@ -1442,14 +1475,18 @@ export function SkillsScreen({ aria-label={`Verify ${resource.uri}`} // A click handler cannot await, and // `verifyRow` owns its own failures — it - // records them as this row's state. - onClick={() => + // records them as this row's state. The + // section is opened HERE, on the gesture, + // not inside `verifyRow` — see + // `openConformance`. + onClick={() => { + openConformance(); void verifyRow( index, resource, manifestKey, - ) - } + ); + }} > Verify diff --git a/clients/web/src/utils/inferMimeFromUri.test.ts b/clients/web/src/utils/inferMimeFromUri.test.ts index 99ee0285..e23f8ae4 100644 --- a/clients/web/src/utils/inferMimeFromUri.test.ts +++ b/clients/web/src/utils/inferMimeFromUri.test.ts @@ -63,6 +63,35 @@ describe("inferMimeFromUri", () => { expect(inferMimeFromUri("skill://a/docs%2Fnotes.md")).toBe("text/markdown"); }); + it("inspects the PATH, not the whole URI", () => { + // An authority ending in a mapped suffix is not a filename: + // `https://documentation.md` has a pathname of `/` and no extension, and + // routing a root resource into the markdown renderer on that basis is + // wrong. + expect(inferMimeFromUri("https://documentation.md")).toBeUndefined(); + expect(inferMimeFromUri("https://documentation.md/")).toBeUndefined(); + // The same host WITH a real markdown path still resolves. + expect(inferMimeFromUri("https://documentation.md/a/readme.md")).toBe( + "text/markdown", + ); + // Non-special schemes SEP-2640 allows parse the same way. + expect(inferMimeFromUri("skill://data-analysis/SKILL.md")).toBe( + "text/markdown", + ); + }); + + it("decodes escape runs independently, so one bad octet cannot poison the path", () => { + // `%FF` is a syntactically valid triplet that is not valid UTF-8, so a + // single `decodeURIComponent` over the whole path throws and abandons the + // rest — leaving the `%2E` in a perfectly acceptable URI undecoded. + expect(inferMimeFromUri("skill://a/%FF/reference%2Emd")).toBe( + "text/markdown", + ); + expect(inferMimeFromUri("skill://a/%FF/report%2Epdf")).toBe( + "application/pdf", + ); + }); + it("falls back to the raw path on a malformed escape", () => { // `decodeURIComponent` throws on `%zz` or a lone `%`, and a server can send // either. A MIME guess is the wrong place to raise. diff --git a/clients/web/src/utils/inferMimeFromUri.ts b/clients/web/src/utils/inferMimeFromUri.ts index 1aca33bb..3d4e40eb 100644 --- a/clients/web/src/utils/inferMimeFromUri.ts +++ b/clients/web/src/utils/inferMimeFromUri.ts @@ -29,8 +29,7 @@ const URI_SUFFIX_MIME: ReadonlyArray = [ * unrecognised suffix so callers can fall through to their own default. */ export function inferMimeFromUri(uri: string): string | undefined { - const path = uri.split("?")[0].split("#")[0]; - const lower = decodePathSafely(path).toLowerCase(); + const lower = decodePercentTriplets(pathOf(uri)).toLowerCase(); for (const [suffix, mime] of URI_SUFFIX_MIME) { if (lower.endsWith(suffix)) return mime; } @@ -38,24 +37,56 @@ export function inferMimeFromUri(uri: string): string | undefined { } /** - * Percent-decode a URI path for matching, falling back to the raw path. + * The **path** component of a URI, not the whole string. + * + * Stripping only the query and fragment left the authority in, so a host that + * happened to end in a mapped suffix was read as a filename: + * `https://documentation.md` has a pathname of `/` and no extension at all, yet + * matched `.md` and routed a root resource into the markdown renderer. + * + * `new URL` handles this for every hierarchical URI including the non-special + * schemes SEP-2640 allows (`skill://a/SKILL.md` → `/SKILL.md`). It throws on + * anything it cannot parse — a bare relative name like `notes.md`, say — so + * that case keeps the old string handling. + */ +function pathOf(uri: string): string { + try { + return new URL(uri).pathname; + } catch { + return uri.split("?")[0].split("#")[0]; + } +} + +/** + * Percent-decode a path for matching, one escape run at a time. * * A URI may percent-encode unreserved characters, so `reference%2Emd` names the * same file as `reference.md` — and `skillUriIdentity` in `core/mcp/skills.ts` - * already treats those spellings as equivalent. Matching the raw string here - * disagreed with that: an encoded `.md` fell through to no MIME at all, so the - * markdown renderer never engaged and the frontmatter was never split. + * already treats those spellings as equivalent. * - * `decodeURIComponent` throws on a malformed escape (`%zz`, a lone `%`), which - * a server can certainly send; a MIME *guess* is the wrong place to raise, so a - * bad sequence simply falls back to matching the raw path. + * Decoding the path in one `decodeURIComponent` call was not enough. That + * throws on a malformed escape (`%zz`) *and* on a syntactically valid octet + * that is not valid UTF-8 (`%FF`), and a single such octet anywhere in the path + * then defeated decoding for the whole string — `skill://a/%FF/reference%2Emd` + * is a perfectly acceptable URI whose `%2E` would never be seen. + * + * So each run of escapes is decoded independently, and a run that cannot be + * decoded as UTF-8 falls back to decoding its **ASCII** octets individually. + * That is enough for suffix matching, where every character that matters is + * ASCII, and it leaves a byte it cannot interpret untouched rather than + * guessing. */ -function decodePathSafely(path: string): string { - try { - return decodeURIComponent(path); - } catch { - return path; - } +function decodePercentTriplets(path: string): string { + return path.replace(/(?:%[0-9A-Fa-f]{2})+/g, (run) => { + try { + return decodeURIComponent(run); + } catch { + return run.replace(/%([0-9A-Fa-f]{2})/g, (raw, hex: string) => { + const code = Number.parseInt(hex, 16); + return code < 0x80 ? String.fromCharCode(code) : raw; + }); + } + }); } /**