fix: address Copilot review round 2 (#2263)

Four findings, all real.

The `text/markdown` fallback applied to every manifest resource, but SEP-2640
expects a manifest to carry supporting scripts, examples and assets with types
of their own — an extensionless, untyped blob was being decoded and rendered as
markdown rather than falling back to binary. The fallback is now scoped to the
skill's own SKILL.md; anything else passes `undefined` and lets ContentViewer
apply its octet-stream/text default.

`mismatchCount` counts `status: "mismatch"`, which `verifySkillResource` uses
for both digest AND size failures, so the badge misreported a size-only failure
as a digest mismatch. Renamed to "N mismatch(es)" rather than filtering: a size
failure is a real verification failure and dropping it from the count would hide
it. The alerts still distinguish the two by title.

`isMarkdownMime` compared the raw string, so `text/markdown; charset=utf-8` and
any casing variant were rejected. That silently skipped the frontmatter split —
the fence stayed in the rendered document and the Frontmatter section vanished,
for a perfectly valid response. It now normalises to the base lowercase type.

The flex-basis regression this refactor exists to fix had no browser-level
guard: the unit tests only inspect accordion state, and the long test-server
fixture is never exercised by Storybook. `LongSkillDocument` is modelled on
`ResourcesScreen`'s `ManyResources` and asserts that every section keeps a
non-zero height, that sections tile without overlapping the header below, that
the viewer scrolls within its own panel, that the detail pane does not scroll as
one column, and that collapse-then-reopen restores identical geometry — the
sequence the original bug needed in order to show itself.

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.stories.tsx

@@ -162,3 +162,103 @@ export const DigestMismatch: Story = {
     ).toBeInTheDocument();
   },
 };
+
+// A SKILL.md long enough to overflow the viewer. Every other fixture here is a
+// line or two, which is precisely why the layout regression this screen was
+// refactored for could not be caught in a story: with short content the viewer
+// never scrolls, so a pane that scrolls as one column looks identical to one
+// that does not (#2263).
+const LONG_SKILL_MD = [
+  "---",
+  "name: data-analysis",
+  "description: Analyze a CSV and summarize its columns",
+  "---",
+  "",
+  "# Data analysis",
+  "",
+  ...Array.from(
+    { length: 40 },
+    (_, i) =>
+      `Paragraph ${i + 1}. Read the file as UTF-8 and sniff the delimiter from ` +
+      "the header line rather than assuming a comma, because a mis-sniffed " +
+      "delimiter yields a single column whose name is the entire header.\n",
+  ),
+].join("\n");
+
+/**
+ * The layout contract, asserted in a real browser.
+ *
+ * This is the regression the refactor exists to prevent, and it is only visible
+ * with content that overflows: the file viewer must scroll **inside its own
+ * panel** while its sibling sections keep usable height, rather than the whole
+ * pane scrolling as one column.
+ *
+ * It also pins the collapse-then-reopen case, which is how the original bug
+ * actually presented — the viewer's content-sized `flex-basis` crushed its
+ * siblings, so collapsing it laid out correctly and reopening it broke again.
+ */
+export const LongSkillDocument: Story = {
+  args: {
+    onReadSkillFile: fn(async () => ({
+      text: LONG_SKILL_MD,
+      mimeType: "text/markdown",
+    })),
+  },
+  play: async ({ canvasElement }) => {
+    const canvas = within(canvasElement);
+    await userEvent.click(canvas.getByText("data-analysis"));
+    const viewerControl = await canvas.findByRole("button", {
+      name: /Skill Resource/,
+    });
+
+    const sections = () => [
+      ...canvasElement.querySelectorAll(
+        ".disclosure-sections > .mantine-Accordion-item",
+      ),
+    ];
+    const geometry = () =>
+      sections().map((s) => Math.round(s.getBoundingClientRect().height));
+
+    // Every section keeps a usable height: none is crushed to nothing by the
+    // viewer's content, which is exactly what a content-sized basis did.
+    const before = geometry();
+    await expect(before.length).toBeGreaterThanOrEqual(3);
+    for (const height of before) {
+      await expect(height).toBeGreaterThan(0);
+    }
+
+    // Sections tile in document order — none overlaps the header below it,
+    // which is how the crushed layout showed up on screen.
+    const rects = sections().map((s) => s.getBoundingClientRect());
+    for (let i = 1; i < rects.length; i++) {
+      await expect(Math.round(rects[i].top)).toBeGreaterThanOrEqual(
+        Math.round(rects[i - 1].bottom) - 1,
+      );
+    }
+
+    // The viewer scrolls WITHIN its own panel rather than growing the pane.
+    const viewerPanel = viewerControl
+      .closest(".mantine-Accordion-item")
+      ?.querySelector(".mantine-Accordion-panel");
+    if (!(viewerPanel instanceof HTMLElement)) {
+      throw new Error("Skill Resource panel not found");
+    }
+    await expect(viewerPanel.scrollHeight).toBeGreaterThan(
+      viewerPanel.clientHeight,
+    );
+
+    // And the detail pane itself does not scroll as one column.
+    const detailCard = canvasElement.querySelectorAll(".mantine-Card-root")[1];
+    if (!(detailCard instanceof HTMLElement)) {
+      throw new Error("Detail card not found");
+    }
+    await expect(detailCard.scrollHeight).toBeLessThanOrEqual(
+      detailCard.clientHeight + 1,
+    );
+
+    // Collapse then reopen restores the same geometry.
+    await userEvent.click(viewerControl);
+    await userEvent.click(viewerControl);
+    await expect(geometry()).toEqual(before);
+  },
+};

clients/web/src/components/screens/SkillsScreen/SkillsScreen.test.tsx

@@ -261,7 +261,7 @@ describe("SkillsScreen", () => {
       "aria-expanded",
       "false",
     );
-    expect(screen.queryByText(/digest mismatch\(es\)/)).not.toBeInTheDocument();
+    expect(screen.queryByText(/mismatch\(es\)/)).not.toBeInTheDocument();
 
     await user.click(screen.getByRole("button", { name: /Verify all/ }));
     expect(await screen.findByText("Digest mismatch")).toBeInTheDocument();
@@ -272,7 +272,7 @@ describe("SkillsScreen", () => {
     expect(conformance.closest(".mantine-Accordion-item")).toContainElement(
       screen.getByText("Digest mismatch"),
     );
-    expect(badgeStyle(/1 digest mismatch\(es\)/)).toContain("red");
+    expect(badgeStyle(/1 mismatch\(es\)/)).toContain("red");
   });
 
   it("badges a warning-only entry yellow, not green", async () => {
@@ -1085,6 +1085,44 @@ describe("SkillsScreen", () => {
     ).not.toBeInTheDocument();
   });
 
+  it("does not treat an untyped supporting resource as markdown", async () => {
+    // SEP-2640 expects a manifest to carry supporting scripts, examples and
+    // assets with types of their own. A markdown fallback is right for the
+    // skill's OWN SKILL.md and wrong for the rest: an extensionless, untyped
+    // blob would be decoded and rendered as markdown rather than as binary.
+    const user = userEvent.setup();
+    const asset: SkillEntry = {
+      uri: "skill://assets/SKILL.md",
+      frontmatter: { name: "assets", description: "Has a typeless blob" },
+      resources: [
+        { uri: "skill://assets/SKILL.md", digest: SELF_DIGEST, size: 1 },
+        // No suffix and no mimeType — nothing says what this is.
+        { uri: "skill://assets/payload", digest: SELF_DIGEST, size: 1 },
+      ],
+    };
+    const onReadSkillFile = vi.fn(async (uri: string) =>
+      uri.endsWith("payload")
+        ? { blob: btoa("---\nnot: frontmatter\n---\n\nbinary-ish") }
+        : { text: SELF_TEXT, mimeType: "text/markdown" },
+    );
+    renderWithMantine(
+      <ControlledSkillsScreen
+        skills={[asset]}
+        onReadSkillFile={onReadSkillFile}
+      />,
+    );
+    await user.click(screen.getByText("assets"));
+    await user.click(
+      screen.getByRole("button", { name: "skill://assets/payload" }),
+    );
+    // Not split, so no Frontmatter section is invented for it...
+    await waitFor(() =>
+      expect(
+        screen.queryByRole("button", { name: /Frontmatter/ }),
+      ).not.toBeInTheDocument(),
+    );
+  });
+
   it("marks the row whose file the viewer is showing", async () => {
     const user = userEvent.setup();
     renderWithMantine(<ControlledSkillsScreen />);

clients/web/src/components/screens/SkillsScreen/SkillsScreen.tsx

@@ -864,15 +864,27 @@ export function SkillsScreen({
   // One effective MIME for the displayed file, inferred the same way the
   // Resources screen infers one: servers routinely omit `mimeType` or answer a
   // generic `text/plain`, and the URI suffix is the better signal.
-  const previewMime = useMemo(
-    () =>
+  // Whether the viewer is showing the skill's OWN SKILL.md. Compared by
+  // identity, like every other URI comparison here.
+  const showingSkillMd =
+    previewUri !== undefined &&
+    selectedUri !== undefined &&
+    skillUriIdentity(previewUri) === skillUriIdentity(selectedUri);
+
+  const previewMime = useMemo(() => {
+    const stated =
       preview?.mimeType ??
-      (previewUri !== undefined ? inferMimeFromUri(previewUri) : undefined) ??
-      // Skill files are markdown by construction under SEP-2640, so that is the
-      // right last resort here rather than octet-stream.
-      "text/markdown",
-    [preview, previewUri],
-  );
+      (previewUri !== undefined ? inferMimeFromUri(previewUri) : undefined);
+    if (stated !== undefined) return stated;
+    // Markdown is the right last resort for a skill's OWN `SKILL.md` — SEP-2640
+    // makes that file markdown by construction. It is the WRONG one for the
+    // rest of a manifest, which the SEP expects to carry supporting scripts,
+    // examples and assets with types of their own: an extensionless, untyped
+    // blob would be decoded and rendered as markdown rather than falling back
+    // to binary. `undefined` here hands the decision to `ContentViewer`, whose
+    // own default is octet-stream for a blob and a text heuristic for text.
+    return showingSkillMd ? "text/markdown" : undefined;
+  }, [preview, previewUri, showingSkillMd]);
 
   // The displayed file, split once into frontmatter and body. BOTH halves of
   // the pane read from this single split, which is what keeps them honest: the
@@ -924,7 +936,10 @@ export function SkillsScreen({
   //
   // Covers a size disagreement as well as a digest one: `verifySkillResource`
   // reports both as `mismatch`, the size check simply being the cheaper one
-  // that runs first.
+  // that runs first. The badge is therefore labelled "mismatch(es)" rather than
+  // "digest mismatch(es)" — counting a size failure under a digest label would
+  // misreport it, and dropping it from the count would hide a real failure.
+  // The alerts below still distinguish the two by title.
   const mismatchCount = Object.values(fileStates).filter(
     (state) =>
       state.status === "done" && state.verification.status === "mismatch",
@@ -1088,7 +1103,7 @@ export function SkillsScreen({
                         verified result before anything had been checked. */}
                     {mismatchCount > 0 && (
                       <CountBadge color="red">
-                        {mismatchCount} digest mismatch(es)
+                        {mismatchCount} mismatch(es)
                       </CountBadge>
                     )}
                   </InlineRow>

clients/web/src/utils/inferMimeFromUri.test.ts

@@ -43,6 +43,20 @@ describe("isMarkdownMime", () => {
     expect(isMarkdownMime("text/x-markdown")).toBe(true);
   });
 
+  it("accepts a MIME carrying parameters", () => {
+    // `text/markdown; charset=utf-8` is a perfectly ordinary response, and
+    // ContentViewer accepts it. Rejecting it here skipped the frontmatter
+    // split, so the fence stayed in the document AND the Frontmatter section
+    // vanished.
+    expect(isMarkdownMime("text/markdown; charset=utf-8")).toBe(true);
+    expect(isMarkdownMime("text/markdown;charset=UTF-8")).toBe(true);
+  });
+
+  it("is case-insensitive", () => {
+    expect(isMarkdownMime("TEXT/MARKDOWN")).toBe(true);
+    expect(isMarkdownMime("Text/X-Markdown; charset=utf-8")).toBe(true);
+  });
+
   it("rejects everything else, including undefined", () => {
     // The gate on frontmatter splitting: a YAML resource must not be split, or
     // a multi-document file loses its first document.

clients/web/src/utils/inferMimeFromUri.ts

@@ -37,8 +37,18 @@ export function inferMimeFromUri(uri: string): string | undefined {
   return undefined;
 }
 
-/** Whether an effective MIME type is Markdown, which is the only form that
- *  carries YAML frontmatter worth splitting off (#2263). */
+/**
+ * Whether an effective MIME type is Markdown — the only form that carries YAML
+ * frontmatter worth splitting off (#2263).
+ *
+ * Normalised before comparing, because a server may answer
+ * `text/markdown; charset=utf-8` or `TEXT/MARKDOWN`, and `ContentViewer`
+ * accepts both. Comparing the raw string rejected them, which skipped the
+ * split: the frontmatter stayed in the rendered document AND the Frontmatter
+ * section vanished, for a response that was perfectly valid.
+ */
 export function isMarkdownMime(mime: string | undefined): boolean {
-  return mime === "text/markdown" || mime === "text/x-markdown";
+  if (mime === undefined) return false;
+  const base = mime.split(";")[0].trim().toLowerCase();
+  return base === "text/markdown" || base === "text/x-markdown";
 }