fix: let a URI suffix outrank a generic declared MIME (#2263)
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) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01R4441JdNLZerCVM7mDDSfB Signed-off-by: cliffhall <cliff@futurescale.com>
This commit is contained in:
parent
418448edc3
commit
ae33582383
4 changed files with 99 additions and 3 deletions
|
|
@ -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(
|
||||
<ControlledSkillsScreen onReadSkillFile={onReadSkillFile} />,
|
||||
);
|
||||
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(
|
||||
<ControlledSkillsScreen onReadSkillFile={onReadSkillFile} />,
|
||||
);
|
||||
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
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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", () => {
|
||||
|
|
|
|||
|
|
@ -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).
|
||||
|
|
|
|||
Loading…
Reference in a new issue