From ae335823839ffde98b024cfe547a56ed74f767b4 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sun, 6 Sep 2026 00:31:29 -0400 Subject: [PATCH] fix: let a URI suffix outrank a generic declared MIME (#2263) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Copilot review round 3. Precedence was `declared ?? inferred`, so a server serving SKILL.md as `text/plain` or `application/octet-stream` defeated the `.md` inference — and `ResourcePreviewPanel` already records that servers commonly do exactly that. The file was then not recognised as markdown, so its YAML frontmatter stayed in the viewer and the Frontmatter section disappeared, for a perfectly valid skill. A SPECIFIC declared type still wins, because a server that says `text/csv` knows its own resource. Only the generic types yield to the suffix, via a new `isGenericMime` that normalises parameters and casing the same way `isMarkdownMime` does. Both directions are covered: `text/plain` on a `.md` URI splits, `text/csv` on a `.md` URI does not. The round's other finding — that `DetailCard` is not a flex container, so the detail column has no definite height — is declined as a false positive, with evidence on the thread: Mantine's own Card.css sets `display: flex; flex-direction: column` on the card root, `DetailCard` still carries `h: "100%"`, and the `LongSkillDocument` story added last round asserts the exact behaviour claimed broken, in a real browser. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01R4441JdNLZerCVM7mDDSfB Signed-off-by: cliffhall --- .../SkillsScreen/SkillsScreen.test.tsx | 41 +++++++++++++++++++ .../screens/SkillsScreen/SkillsScreen.tsx | 14 ++++++- .../web/src/utils/inferMimeFromUri.test.ts | 25 ++++++++++- clients/web/src/utils/inferMimeFromUri.ts | 22 ++++++++++ 4 files changed, 99 insertions(+), 3 deletions(-) diff --git a/clients/web/src/components/screens/SkillsScreen/SkillsScreen.test.tsx b/clients/web/src/components/screens/SkillsScreen/SkillsScreen.test.tsx index 124c74e1..e947e3ba 100644 --- a/clients/web/src/components/screens/SkillsScreen/SkillsScreen.test.tsx +++ b/clients/web/src/components/screens/SkillsScreen/SkillsScreen.test.tsx @@ -1085,6 +1085,47 @@ describe("SkillsScreen", () => { ).not.toBeInTheDocument(); }); + it("lets a .md suffix outrank a generic declared MIME", async () => { + // Servers routinely serve SKILL.md as `text/plain`. Letting that outrank + // the suffix meant the file was not recognised as markdown, so its YAML + // stayed in the viewer and the Frontmatter section disappeared — for a + // perfectly valid skill. + const user = userEvent.setup(); + const onReadSkillFile = vi.fn(async () => ({ + text: "---\nname: data-analysis\n---\n\n# The body\n", + mimeType: "text/plain", + })); + renderWithMantine( + , + ); + await user.click(screen.getByText("data-analysis")); + const viewer = screen.getByTestId("skill-resource-viewer"); + await waitFor(() => expect(viewer).toHaveTextContent("The body")); + expect( + screen.getByRole("button", { name: /Frontmatter/ }), + ).toBeInTheDocument(); + expect(viewer).not.toHaveTextContent("name: data-analysis"); + }); + + it("keeps a SPECIFIC declared MIME over the suffix", async () => { + // The converse: a server that says `text/csv` for a `.md` URI knows its own + // resource, so the declaration wins and nothing is split. + const user = userEvent.setup(); + const onReadSkillFile = vi.fn(async () => ({ + text: "---\na,b\n---\n1,2\n", + mimeType: "text/csv", + })); + renderWithMantine( + , + ); + await user.click(screen.getByText("data-analysis")); + await waitFor(() => + expect( + screen.queryByRole("button", { name: /Frontmatter/ }), + ).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 diff --git a/clients/web/src/components/screens/SkillsScreen/SkillsScreen.tsx b/clients/web/src/components/screens/SkillsScreen/SkillsScreen.tsx index ff798be4..3a8125d4 100644 --- a/clients/web/src/components/screens/SkillsScreen/SkillsScreen.tsx +++ b/clients/web/src/components/screens/SkillsScreen/SkillsScreen.tsx @@ -43,6 +43,7 @@ import { import { splitSkillFile } from "../../../utils/splitSkillFile"; import { inferMimeFromUri, + isGenericMime, isMarkdownMime, } from "../../../utils/inferMimeFromUri"; import { tryDecodeBase64ToUtf8 } from "../../elements/ContentViewer/contentViewerUtils"; @@ -872,9 +873,18 @@ export function SkillsScreen({ skillUriIdentity(previewUri) === skillUriIdentity(selectedUri); const previewMime = useMemo(() => { + const declared = preview?.mimeType; + const inferred = + previewUri !== undefined ? inferMimeFromUri(previewUri) : undefined; + // A SPECIFIC declared type wins — the server knows its own resource. A + // *generic* one does not: servers routinely serve `SKILL.md` as + // `text/plain` or `application/octet-stream`, and letting that outrank a + // `.md` suffix meant a valid skill file was not recognised as markdown, so + // its YAML stayed in the viewer and the Frontmatter section disappeared. const stated = - preview?.mimeType ?? - (previewUri !== undefined ? inferMimeFromUri(previewUri) : undefined); + declared !== undefined && !isGenericMime(declared) + ? declared + : (inferred ?? declared); 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 diff --git a/clients/web/src/utils/inferMimeFromUri.test.ts b/clients/web/src/utils/inferMimeFromUri.test.ts index d9ee86f8..49fbcc02 100644 --- a/clients/web/src/utils/inferMimeFromUri.test.ts +++ b/clients/web/src/utils/inferMimeFromUri.test.ts @@ -1,5 +1,28 @@ import { describe, it, expect } from "vitest"; -import { inferMimeFromUri, isMarkdownMime } from "./inferMimeFromUri"; +import { + inferMimeFromUri, + isGenericMime, + isMarkdownMime, +} from "./inferMimeFromUri"; + +describe("isGenericMime", () => { + it("names the types a server sends when it does not really know", () => { + expect(isGenericMime("text/plain")).toBe(true); + expect(isGenericMime("application/octet-stream")).toBe(true); + // Normalised like `isMarkdownMime`, so parameters and casing still count. + expect(isGenericMime("text/plain; charset=utf-8")).toBe(true); + expect(isGenericMime("TEXT/PLAIN")).toBe(true); + }); + + it("does not treat a specific type as generic", () => { + // A server that says `text/markdown` or `text/csv` knows its own resource, + // and that declaration must outrank a URI suffix. + expect(isGenericMime("text/markdown")).toBe(false); + expect(isGenericMime("text/csv")).toBe(false); + expect(isGenericMime("application/json")).toBe(false); + expect(isGenericMime(undefined)).toBe(false); + }); +}); describe("inferMimeFromUri", () => { it("maps every suffix in the table", () => { diff --git a/clients/web/src/utils/inferMimeFromUri.ts b/clients/web/src/utils/inferMimeFromUri.ts index 76920ce2..c49dc797 100644 --- a/clients/web/src/utils/inferMimeFromUri.ts +++ b/clients/web/src/utils/inferMimeFromUri.ts @@ -37,6 +37,28 @@ export function inferMimeFromUri(uri: string): string | undefined { return undefined; } +/** + * Types a server sends when it does not really know, or did not bother. + * + * `ResourcePreviewPanel` already records the underlying observation — servers + * "commonly omit `mimeType` (or return a generic `text/plain` / + * `application/octet-stream`), so the URI suffix is the most reliable signal". + * Naming them lets a caller act on that: a declared type this generic is weaker + * evidence than a `.md` suffix, while a specific declared type still wins. + */ +const GENERIC_MIMES = new Set(["text/plain", "application/octet-stream"]); + +/** + * Whether a declared MIME is too generic to outrank a URI suffix. + * + * Normalised the same way `isMarkdownMime` is, so `text/plain; charset=utf-8` + * counts as generic too. + */ +export function isGenericMime(mime: string | undefined): boolean { + if (mime === undefined) return false; + return GENERIC_MIMES.has(mime.split(";")[0].trim().toLowerCase()); +} + /** * Whether an effective MIME type is Markdown — the only form that carries YAML * frontmatter worth splitting off (#2263).