diff --git a/clients/web/src/components/screens/SkillsScreen/SkillsScreen.test.tsx b/clients/web/src/components/screens/SkillsScreen/SkillsScreen.test.tsx index ca0be00a..1eb2e116 100644 --- a/clients/web/src/components/screens/SkillsScreen/SkillsScreen.test.tsx +++ b/clients/web/src/components/screens/SkillsScreen/SkillsScreen.test.tsx @@ -1263,6 +1263,53 @@ describe("SkillsScreen", () => { ); }); + it("expand-all covers sections that are not visible yet", async () => { + // `sectionIds` holds only what renders at this instant, and "expand all" + // used to write exactly that — so a section absent at the moment of the + // click (Frontmatter, while the read is still in flight; Resources, on a + // dynamic skill) was DROPPED from the open set, and arrived collapsed with + // the control offering to expand all over again (#2263). + const user = userEvent.setup(); + renderWithMantine(); + // Start on the dynamic skill, which renders no Resources section at all. + await user.click(screen.getByText("dynamic-report")); + // Settle the auto-read before touching the toggle, so the click lands on a + // known state rather than racing the section set. + await waitFor(() => + expect( + screen.getByRole("button", { name: /Collapse all|Expand all/ }), + ).toBeInTheDocument(), + ); + const toggle = () => + screen.getByRole("button", { name: /Collapse all|Expand all/ }); + if (toggle().getAttribute("aria-label") === "Collapse all") { + await user.click(toggle()); + } + await user.click(screen.getByRole("button", { name: "Expand all" })); + + // Switch to a static skill WITH findings, so the clean-entry collapse rule + // does not overlap with what this test is about. Scoped to the sidebar: + // with every section expanded, the skill's own name also appears in the + // detail pane's frontmatter block. + await user.click( + within(screen.getByTestId("skills-screen")).getAllByText("right-name")[0], + ); + await waitFor(() => + expect(screen.getByRole("button", { name: /Resources/ })).toHaveAttribute( + "aria-expanded", + "true", + ), + ); + expect(screen.getByRole("button", { name: /Frontmatter/ })).toHaveAttribute( + "aria-expanded", + "true", + ); + // And the control agrees that everything is open. + expect( + screen.getByRole("button", { name: "Collapse all" }), + ).toBeInTheDocument(); + }); + it("toggles every section at once from the header control", 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 b0bfa624..afd39c66 100644 --- a/clients/web/src/components/screens/SkillsScreen/SkillsScreen.tsx +++ b/clients/web/src/components/screens/SkillsScreen/SkillsScreen.tsx @@ -518,9 +518,14 @@ function shortDigest(digest: string | undefined): string { * The sidebar lists the skills the server enumerated; the detail pane shows the * entry's frontmatter, every conformance finding * (`checkSkillConformance`), and the resource manifest with a per-file - * verification verdict. Verification is on demand: SEP-2640 says a - * `resources/read` of a skill file is not a load and confers no standing, so - * the Inspector fetches only what the user asks it to. + * verification verdict. + * + * **Reading is not verification, and the two have different triggers** (#2263). + * The skill's own `SKILL.md` is read as soon as it is selected, so the file is + * simply on screen; SEP-2640 is explicit that a `resources/read` of a skill + * file is *not* a load and confers no standing, so that claims nothing on the + * user's behalf. Digest **verification** remains strictly on demand — it is + * what the buttons are for, and nothing is hashed until asked. */ export function SkillsScreen({ sessionKey, @@ -963,11 +968,20 @@ export function SkillsScreen({ // Which sections this skill actually renders — Frontmatter only exists when // the displayed file has any, so an "expand all" that named it unconditionally // would leave the toggle stuck reading "Expand" on a file without one. + // The sections rendered RIGHT NOW. A dynamic skill renders no Resources + // section, and Frontmatter is absent until the file has been read and turns + // out to have any — so naming those unconditionally would leave "expand all" + // permanently unsatisfied. + // + // This drives the toggle's *label* only. It must NOT be what "expand all" + // writes: `openSections` outlives any one skill, so replacing it with the + // currently-visible set silently drops the others. Pressing Expand all while + // the SKILL.md read was still in flight removed `frontmatter`, and the + // section then arrived collapsed with the control offering to expand all over + // again; switching away from a dynamic skill did the same to `resources`. const sectionIds = useMemo( () => [ "conformance", - // A dynamic skill renders no Resources section, so naming it here would - // leave "expand all" permanently unsatisfied. ...(isDynamic ? [] : ["resources"]), ...(previewParts?.frontmatter !== undefined ? ["frontmatter"] : []), "resource", @@ -1099,8 +1113,10 @@ export function SkillsScreen({ - setOpenSections(allSectionsOpen ? [] : [...sectionIds]) + setOpenSections(allSectionsOpen ? [] : [...ALL_SECTIONS]) } /> diff --git a/clients/web/src/hooks/useServerCommands.tsx b/clients/web/src/hooks/useServerCommands.tsx index 90457e78..748101c4 100644 --- a/clients/web/src/hooks/useServerCommands.tsx +++ b/clients/web/src/hooks/useServerCommands.tsx @@ -216,9 +216,10 @@ export interface ServerCommands { uri: string, ) => Promise>["result"]>; /** - * Read one skill file's contents (SEP-2640), for digest verification. Returns - * the single content block that answers the URI, narrowed to the two fields - * the digest is taken over. + * Read one skill file's contents (SEP-2640) — for display in the Skills + * screen's viewer, and for digest verification. Returns the single content + * block that answers the URI, narrowed to the two fields the digest is taken + * over. */ onReadSkillFile: (uri: string) => Promise; /** Re-fetch one skill entry through `skills/get` (SEP-2640). */ @@ -947,9 +948,13 @@ export function useServerCommands({ runCommandInBackground(() => resourcesPagination.onLoadMore(), "ambient"), [resourcesPagination, runCommandInBackground], ); - // Skill files are fetched on demand, never pre-fetched: SEP-2640 is explicit - // that a `resources/read` of a skill file is not a load and confers no - // standing, so the Inspector reads only what the user asks it to verify. + // Skill files are still never pre-fetched in bulk — nothing walks the + // manifest reading everything. What IS read without being asked is the single + // `SKILL.md` of whichever skill is selected, so the file is on screen (#2263); + // SEP-2640 is explicit that a `resources/read` of a skill file is not a load + // and confers no standing, so that claims nothing on the user's behalf. + // Digest verification remains strictly on demand. + // // Routed through `onReadResourceContents` so a skill read gets the same // auth-recovery retry every other read does. const onReadSkillFile = useCallback( diff --git a/clients/web/src/theme/Accordion.ts b/clients/web/src/theme/Accordion.ts index d783dc08..6816b2eb 100644 --- a/clients/web/src/theme/Accordion.ts +++ b/clients/web/src/theme/Accordion.ts @@ -24,14 +24,21 @@ export const ThemeAccordion = Accordion.extend({ }, // `skillSections` is `disclosure` plus a scrolling root (#2263). // - // The Skills pane holds sections whose content is a rendered document or a - // findings list, not a uniform row list, so they are sized to their content - // and never shrink. That removes the mid-content clipping a shrinking panel - // produced — the panel really was scrollable, but macOS overlay scrollbars - // are invisible until hover, so a Resources table cut off mid-alert read as - // broken rather than scrollable. Overflow moves up to the root, so in the - // rare case the sections genuinely exceed the pane it is the *stack* that - // scrolls, at a section boundary, instead of a panel slicing its own content. + // The Skills pane's metadata sections use `flex: 0 1 auto` with a `mih` + // floor: they give up space until they reach that floor and then scroll their + // own panels, which is what leaves the file viewer its share no matter how + // large a manifest or findings list gets. + // + // The scrolling root is the FALLBACK for when even the combined floors do not + // fit — with several sections open in a short window there is no arrangement + // that shows everything, and scrolling the stack at a section boundary beats + // crushing a panel below its floor. + // + // ⚠️ The floor is load-bearing, not decoration. Without it a section can be + // squeezed far below its content and slice it mid-line; with `flex-shrink: 0` + // instead, a 512-row manifest keeps its full height and pushes the viewer off + // the bottom of the pane. `SECTION_FLEX` in `SkillsScreen.tsx` records both + // failures — this variant only works in combination with it. styles: (_theme, props) => { if (props.variant === "skillSections") { return { diff --git a/clients/web/src/utils/inferMimeFromUri.test.ts b/clients/web/src/utils/inferMimeFromUri.test.ts index 49fbcc02..99ee0285 100644 --- a/clients/web/src/utils/inferMimeFromUri.test.ts +++ b/clients/web/src/utils/inferMimeFromUri.test.ts @@ -52,6 +52,25 @@ describe("inferMimeFromUri", () => { expect(inferMimeFromUri("https://x/a.md?v=2#top")).toBe("text/markdown"); }); + it("matches a percent-encoded suffix", () => { + // `reference%2Emd` names the same file as `reference.md`, and + // `skillUriIdentity` already treats those spellings as equivalent — so + // matching the raw string here disagreed with the rest of the app and left + // an encoded `.md` with no MIME, no markdown renderer and no split. + expect(inferMimeFromUri("skill://a/reference%2Emd")).toBe("text/markdown"); + expect(inferMimeFromUri("skill://a/report%2Epdf")).toBe("application/pdf"); + // A percent-encoded path segment separator is decoded too. + expect(inferMimeFromUri("skill://a/docs%2Fnotes.md")).toBe("text/markdown"); + }); + + 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. + expect(inferMimeFromUri("skill://a/bad%zz.md")).toBe("text/markdown"); + expect(inferMimeFromUri("skill://a/100%.md")).toBe("text/markdown"); + expect(inferMimeFromUri("skill://a/bad%zz.bin")).toBeUndefined(); + }); + it("returns undefined for an unrecognised suffix, so callers can default", () => { expect(inferMimeFromUri("skill://a/notes.bin")).toBeUndefined(); expect(inferMimeFromUri("skill://a/no-extension")).toBeUndefined(); diff --git a/clients/web/src/utils/inferMimeFromUri.ts b/clients/web/src/utils/inferMimeFromUri.ts index c49dc797..1aca33bb 100644 --- a/clients/web/src/utils/inferMimeFromUri.ts +++ b/clients/web/src/utils/inferMimeFromUri.ts @@ -30,13 +30,34 @@ const URI_SUFFIX_MIME: ReadonlyArray = [ */ export function inferMimeFromUri(uri: string): string | undefined { const path = uri.split("?")[0].split("#")[0]; - const lower = path.toLowerCase(); + const lower = decodePathSafely(path).toLowerCase(); for (const [suffix, mime] of URI_SUFFIX_MIME) { if (lower.endsWith(suffix)) return mime; } return undefined; } +/** + * Percent-decode a URI path for matching, falling back to the raw path. + * + * 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. + * + * `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. + */ +function decodePathSafely(path: string): string { + try { + return decodeURIComponent(path); + } catch { + return path; + } +} + /** * Types a server sends when it does not really know, or did not bother. * diff --git a/test-servers/src/skills.ts b/test-servers/src/skills.ts index 05cae596..c1d977e3 100644 --- a/test-servers/src/skills.ts +++ b/test-servers/src/skills.ts @@ -288,7 +288,9 @@ const FIXTURE_SKILLS: FixtureSkill[] = [ ]; /** Every servable `skill://` file, by URI. `dynamic` skills contribute their - * `SKILL.md` too, so the screen's "View SKILL.md" works there as well. */ + * `SKILL.md` too, so the Skills screen's resource viewer has something to show + * for them as well — it opens on the selected skill's own file, and a dynamic + * skill advertises no manifest but still serves that one. */ const FILES_BY_URI = new Map(); for (const skill of FIXTURE_SKILLS) { if (skill.files === "dynamic") {