diff --git a/clients/web/src/components/screens/SkillsScreen/SkillsScreen.test.tsx b/clients/web/src/components/screens/SkillsScreen/SkillsScreen.test.tsx
index 1eb2e116..b342701b 100644
--- a/clients/web/src/components/screens/SkillsScreen/SkillsScreen.test.tsx
+++ b/clients/web/src/components/screens/SkillsScreen/SkillsScreen.test.tsx
@@ -306,6 +306,62 @@ describe("SkillsScreen", () => {
expect(badgeStyle(/1 mismatch\(es\)/)).toContain("red");
});
+ it("a stale Verify all batch does not reopen Conformance on another skill", async () => {
+ // `verifyRow` is called once per row by every "Verify all" worker as it
+ // advances, so a batch begun on one skill keeps calling it after the user
+ // has moved on. The keyed writes discard those results, but an open-state
+ // update is not keyed to a manifest — so opening the section from inside
+ // `verifyRow` let obsolete work mutate the current pane (#2263).
+ const user = userEvent.setup();
+ // Held open so the batch is still in flight when the selection changes.
+ const releases: (() => void)[] = [];
+ const onReadSkillFile = vi.fn(
+ () =>
+ new Promise<{ text: string }>((resolve) => {
+ releases.push(() => resolve({ text: SELF_TEXT }));
+ }),
+ );
+ // More rows than the concurrency cap, so workers keep pulling.
+ const manyRows: SkillEntry = {
+ ...CLEAN_SKILL,
+ uri: "skill://many/SKILL.md",
+ frontmatter: { name: "many", description: "Many rows" },
+ resources: Array.from({ length: 10 }, (_, i) => ({
+ uri: i === 0 ? "skill://many/SKILL.md" : `skill://many/f${i}.md`,
+ digest: SELF_DIGEST,
+ size: textToBytes(SELF_TEXT).byteLength,
+ })),
+ };
+ renderWithMantine(
+ ,
+ );
+ await user.click(screen.getByText("many"));
+ await user.click(screen.getByRole("button", { name: /Verify all/ }));
+
+ // Switch to a clean skill and collapse Conformance deliberately.
+ await user.click(
+ within(screen.getByTestId("skills-screen")).getAllByText(
+ "data-analysis",
+ )[0],
+ );
+ await waitFor(() =>
+ expect(
+ screen.getByRole("button", { name: /Conformance/ }),
+ ).toHaveAttribute("aria-expanded", "false"),
+ );
+
+ // Let the abandoned batch's workers advance. They must not reopen it.
+ for (const release of releases) release();
+ await waitFor(() => expect(onReadSkillFile).toHaveBeenCalled());
+ expect(screen.getByRole("button", { name: /Conformance/ })).toHaveAttribute(
+ "aria-expanded",
+ "false",
+ );
+ });
+
it("badges a warning-only entry yellow, not green", async () => {
// Green reads as "nothing to see", which would hide the only signal the
// section carries for an entry whose findings are all warnings (#2263).
diff --git a/clients/web/src/components/screens/SkillsScreen/SkillsScreen.tsx b/clients/web/src/components/screens/SkillsScreen/SkillsScreen.tsx
index afd39c66..dc2e22a3 100644
--- a/clients/web/src/components/screens/SkillsScreen/SkillsScreen.tsx
+++ b/clients/web/src/components/screens/SkillsScreen/SkillsScreen.tsx
@@ -273,6 +273,20 @@ const MonoCaption = Text.withProps({
variant: "monoCaption",
});
+// The skill's description, clamped.
+//
+// It sits in the pane's FIXED header, beside an accordion whose basis is zero,
+// so its height is subtracted from everything below it. SEP-2640 permits 1,024
+// characters and a non-conforming server can send more — in a narrow or short
+// pane that wraps to enough lines to crowd the sections out entirely, which is
+// the same "server-controlled content sizes the layout" trap `viewerFlex`
+// documents. Three lines is enough to read a real description; the full text
+// stays available through the native `title` tooltip the call site passes.
+const SkillDescription = Text.withProps({
+ size: "sm",
+ lineClamp: 3,
+});
+
const IssueStack = Stack.withProps({
gap: "xs",
});
@@ -685,15 +699,33 @@ export function SkillsScreen({
* verifying either would update both, and "Verify all" would race two
* different digest/size declarations into the same slot.
*/
+ /**
+ * Reveal the Conformance section, which auto-collapses for an entry with no
+ * static findings — `tampered-notes` is exactly that: structurally clean,
+ * bytes wrong — so a verdict would otherwise land where nobody can see it.
+ *
+ * Called from the USER gestures that produce a verdict, never from the async
+ * work they start: an open-state update is not keyed to a manifest, so
+ * calling it from a continuation would let stale work mutate the current
+ * pane.
+ */
+ const openConformance = useCallback(() => {
+ setOpenSections((prev) =>
+ prev.includes("conformance") ? prev : [...prev, "conformance"],
+ );
+ }, []);
+
const verifyRow = useCallback(
async (index: number, resource: SkillResource, key: string) => {
- // A mismatch is reported in the Conformance section, which auto-collapses
- // for an entry with no *static* findings — and `tampered-notes` is
- // exactly that: structurally clean, bytes wrong. Opening it here is what
- // stops the verdict landing somewhere the user cannot see it.
- setOpenSections((prev) =>
- prev.includes("conformance") ? prev : [...prev, "conformance"],
- );
+ // NOTE: opening the Conformance section deliberately does NOT happen
+ // here. `verifyRow` is called once per row by every "Verify all" worker
+ // as it advances, so a batch begun on one skill keeps calling it after
+ // the user has switched to another. The keyed writes above discard those
+ // continuations, but an open-state update is not keyed to a manifest and
+ // would have reopened Conformance on whatever skill is now selected.
+ // The section is opened at the two USER entry points instead — the row
+ // button and "Verify all" — where the gesture and the pane agree.
+ //
// Claimed synchronously, so two verifications of this row are ordered
// before either read starts.
const attempt = (nextAttempt.current += 1);
@@ -729,11 +761,11 @@ export function SkillsScreen({
);
const verifyAll = useCallback(() => {
- // Same reason as `verifyRow`: the verdicts render in Conformance, which may
- // be collapsed for a structurally clean entry.
- setOpenSections((prev) =>
- prev.includes("conformance") ? prev : [...prev, "conformance"],
- );
+ // One of the two user entry points that opens Conformance — the verdicts
+ // render there, and it may be collapsed for a structurally clean entry.
+ // Done here rather than in `verifyRow` so a batch that outlives its own
+ // selection cannot reopen the section on a different skill.
+ openConformance();
// Bounded concurrency, not `Promise.all` over the whole manifest: a
// conforming skill may declare 512 files, and firing 512 simultaneous
// `resources/read` calls would bury the transport and the server for no
@@ -768,7 +800,7 @@ export function SkillsScreen({
return next;
}),
);
- }, [manifest, manifestKey, verifyRow]);
+ }, [manifest, manifestKey, openConformance, verifyRow]);
/**
* Put one of the skill's files in the viewer. Driven both by the effect that
@@ -840,10 +872,9 @@ export function SkillsScreen({
// The verdict renders inside the Conformance section, which auto-collapses
// for a clean entry — and a clean entry is exactly the common case for this
// button. Without this the answer would land in a collapsed section and the
- // click would look like it did nothing.
- setOpenSections((prev) =>
- prev.includes("conformance") ? prev : [...prev, "conformance"],
- );
+ // click would look like it did nothing. Called on the gesture, before the
+ // request goes out — see `openConformance`.
+ openConformance();
// Same shape as the SKILL.md read: a click handler cannot await, the chain
// ends in its own `catch`, and both arms drop a result whose manifest has
// been invalidated or whose click has been superseded.
@@ -882,7 +913,7 @@ export function SkillsScreen({
message: err instanceof Error ? err.message : String(err),
});
});
- }, [manifestKey, onGetSkill, selected]);
+ }, [manifestKey, onGetSkill, openConformance, selected]);
const fetched = fetchedEntry.key === manifestKey ? fetchedEntry : undefined;
// `invalid` outranks the snapshot comparison: an entry that breaks a
@@ -1123,7 +1154,9 @@ export function SkillsScreen({
{selected.frontmatter.description && (
- {selected.frontmatter.description}
+
+ {selected.frontmatter.description}
+
)}
{/* Inline, not a `.withProps()` subcomponent: `Accordion` is a
@@ -1442,14 +1475,18 @@ export function SkillsScreen({
aria-label={`Verify ${resource.uri}`}
// A click handler cannot await, and
// `verifyRow` owns its own failures — it
- // records them as this row's state.
- onClick={() =>
+ // records them as this row's state. The
+ // section is opened HERE, on the gesture,
+ // not inside `verifyRow` — see
+ // `openConformance`.
+ onClick={() => {
+ openConformance();
void verifyRow(
index,
resource,
manifestKey,
- )
- }
+ );
+ }}
>
Verify
diff --git a/clients/web/src/utils/inferMimeFromUri.test.ts b/clients/web/src/utils/inferMimeFromUri.test.ts
index 99ee0285..e23f8ae4 100644
--- a/clients/web/src/utils/inferMimeFromUri.test.ts
+++ b/clients/web/src/utils/inferMimeFromUri.test.ts
@@ -63,6 +63,35 @@ describe("inferMimeFromUri", () => {
expect(inferMimeFromUri("skill://a/docs%2Fnotes.md")).toBe("text/markdown");
});
+ it("inspects the PATH, not the whole URI", () => {
+ // An authority ending in a mapped suffix is not a filename:
+ // `https://documentation.md` has a pathname of `/` and no extension, and
+ // routing a root resource into the markdown renderer on that basis is
+ // wrong.
+ expect(inferMimeFromUri("https://documentation.md")).toBeUndefined();
+ expect(inferMimeFromUri("https://documentation.md/")).toBeUndefined();
+ // The same host WITH a real markdown path still resolves.
+ expect(inferMimeFromUri("https://documentation.md/a/readme.md")).toBe(
+ "text/markdown",
+ );
+ // Non-special schemes SEP-2640 allows parse the same way.
+ expect(inferMimeFromUri("skill://data-analysis/SKILL.md")).toBe(
+ "text/markdown",
+ );
+ });
+
+ it("decodes escape runs independently, so one bad octet cannot poison the path", () => {
+ // `%FF` is a syntactically valid triplet that is not valid UTF-8, so a
+ // single `decodeURIComponent` over the whole path throws and abandons the
+ // rest — leaving the `%2E` in a perfectly acceptable URI undecoded.
+ expect(inferMimeFromUri("skill://a/%FF/reference%2Emd")).toBe(
+ "text/markdown",
+ );
+ expect(inferMimeFromUri("skill://a/%FF/report%2Epdf")).toBe(
+ "application/pdf",
+ );
+ });
+
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.
diff --git a/clients/web/src/utils/inferMimeFromUri.ts b/clients/web/src/utils/inferMimeFromUri.ts
index 1aca33bb..3d4e40eb 100644
--- a/clients/web/src/utils/inferMimeFromUri.ts
+++ b/clients/web/src/utils/inferMimeFromUri.ts
@@ -29,8 +29,7 @@ const URI_SUFFIX_MIME: ReadonlyArray = [
* unrecognised suffix so callers can fall through to their own default.
*/
export function inferMimeFromUri(uri: string): string | undefined {
- const path = uri.split("?")[0].split("#")[0];
- const lower = decodePathSafely(path).toLowerCase();
+ const lower = decodePercentTriplets(pathOf(uri)).toLowerCase();
for (const [suffix, mime] of URI_SUFFIX_MIME) {
if (lower.endsWith(suffix)) return mime;
}
@@ -38,24 +37,56 @@ export function inferMimeFromUri(uri: string): string | undefined {
}
/**
- * Percent-decode a URI path for matching, falling back to the raw path.
+ * The **path** component of a URI, not the whole string.
+ *
+ * Stripping only the query and fragment left the authority in, so a host that
+ * happened to end in a mapped suffix was read as a filename:
+ * `https://documentation.md` has a pathname of `/` and no extension at all, yet
+ * matched `.md` and routed a root resource into the markdown renderer.
+ *
+ * `new URL` handles this for every hierarchical URI including the non-special
+ * schemes SEP-2640 allows (`skill://a/SKILL.md` → `/SKILL.md`). It throws on
+ * anything it cannot parse — a bare relative name like `notes.md`, say — so
+ * that case keeps the old string handling.
+ */
+function pathOf(uri: string): string {
+ try {
+ return new URL(uri).pathname;
+ } catch {
+ return uri.split("?")[0].split("#")[0];
+ }
+}
+
+/**
+ * Percent-decode a path for matching, one escape run at a time.
*
* 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.
+ * already treats those spellings as equivalent.
*
- * `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.
+ * Decoding the path in one `decodeURIComponent` call was not enough. That
+ * throws on a malformed escape (`%zz`) *and* on a syntactically valid octet
+ * that is not valid UTF-8 (`%FF`), and a single such octet anywhere in the path
+ * then defeated decoding for the whole string — `skill://a/%FF/reference%2Emd`
+ * is a perfectly acceptable URI whose `%2E` would never be seen.
+ *
+ * So each run of escapes is decoded independently, and a run that cannot be
+ * decoded as UTF-8 falls back to decoding its **ASCII** octets individually.
+ * That is enough for suffix matching, where every character that matters is
+ * ASCII, and it leaves a byte it cannot interpret untouched rather than
+ * guessing.
*/
-function decodePathSafely(path: string): string {
- try {
- return decodeURIComponent(path);
- } catch {
- return path;
- }
+function decodePercentTriplets(path: string): string {
+ return path.replace(/(?:%[0-9A-Fa-f]{2})+/g, (run) => {
+ try {
+ return decodeURIComponent(run);
+ } catch {
+ return run.replace(/%([0-9A-Fa-f]{2})/g, (raw, hex: string) => {
+ const code = Number.parseInt(hex, 16);
+ return code < 0x80 ? String.fromCharCode(code) : raw;
+ });
+ }
+ });
}
/**