fix: address Copilot review round 6 (#2263)

Four findings — two inline, two in the review's suppressed block.

"Expand all" wrote `sectionIds` — the sections rendered at that instant — into
`openSections`, which outlives any one skill. Pressing it while the SKILL.md
read was still in flight dropped `frontmatter`, so the section arrived collapsed
with the control offering to expand all over again; a dynamic skill did the same
to `resources`. It now expands to the complete set, and `sectionIds` decides
only the toggle's label. The new test is verified to detect the defect: with
`sectionIds` restored it fails.

`inferMimeFromUri` matched the raw path, so a percent-encoded suffix such as
`reference%2Emd` produced no MIME at all — no markdown renderer and no
frontmatter split — even though `skillUriIdentity` already treats that spelling
as the same file. It now decodes defensively, falling back to the raw path on a
malformed escape rather than throwing inside what is only a guess.

Two comment fixes, both cases of a deliberate decision being contradicted by the
prose around it. `theme/Accordion.ts` still asserted "sized to content and never
shrink; overflow moves to the root" after round 5 moved the sections back to
`flex: 0 1 auto` with a floor — the exact stale rationale that makes a
load-bearing layout easy to regress. And the component doc, the
`onReadSkillFile` doc in `useServerCommands.tsx` and a `test-servers` fixture
comment all still said skill files are read only when the user asks to verify
them, one of them still naming the removed "View SKILL.md" button. All three now
separate reading (on selection, and not a load under SEP-2640) from verification
(strictly on demand).

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01R4441JdNLZerCVM7mDDSfB
Signed-off-by: cliffhall <cliff@futurescale.com>

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(<ControlledSkillsScreen />);
+    // 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(<ControlledSkillsScreen />);

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({
                 <ListToggle
                   variant="subtle"
                   compact={!allSectionsOpen}
+                  // Expands to the COMPLETE section set, not the visible one —
+                  // see `sectionIds` for what writing the visible set dropped.
                   onToggle={() =>
-                    setOpenSections(allSectionsOpen ? [] : [...sectionIds])
+                    setOpenSections(allSectionsOpen ? [] : [...ALL_SECTIONS])
                   }
                 />
               </InlineRow>

clients/web/src/hooks/useServerCommands.tsx

@@ -216,9 +216,10 @@ export interface ServerCommands {
     uri: string,
   ) => Promise<Awaited<ReturnType<InspectorClient["readResource"]>>["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<SkillFileContents>;
   /** 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(

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 {

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();

clients/web/src/utils/inferMimeFromUri.ts

@@ -30,13 +30,34 @@ const URI_SUFFIX_MIME: ReadonlyArray<readonly [string, string]> = [
  */
 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.
  *

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<string, FixtureFile>();
 for (const skill of FIXTURE_SKILLS) {
   if (skill.files === "dynamic") {