From 27f1b2f1bd165b7cbaa34a7c390146ae595588ed Mon Sep 17 00:00:00 2001 From: cliffhall Date: Mon, 7 Sep 2026 21:57:48 -0400 Subject: [PATCH] fix: address Copilot review round 12 on #2248 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit **The read bounds could produce a false `ok: true`.** Entries past the cap are never fetched, `fileFailed` only examines fetched rows, and `resource-limit-exceeded` is a WARNING — so a manifest whose 513th file was tampered with reported the skill verified. That trades a denial of service for a wrong answer, which is the failure this whole PR argues against. A truncated read is now recorded as `incomplete` and forces `ok: false`, reported separately so a consumer can tell "this skill is wrong" from "this skill was not fully checked". And a self entry excluded by the cap is now verified against its declared digest, not merely read for frontmatter — otherwise the skill's own SKILL.md was the one file nobody checked. **`entryKey` could crash the TUI.** It called `JSON.stringify` on the whole server-controlled entry, during render. `skillEntryKey` is decided by the same `jsonGraphError` guard that bounds the frontmatter comparison, with a coarse fallback that still separates two unrepresentable entries by URI — collapsing them would show one skill's verdict under another's name. **The tab bar wraps and `tabsHeight` was hard-coded to 1.** An OAuth-capable stdio server serving Skills needs ~107 columns, so it wraps at 132 as well as at 80, and every pane below was sized a row too tall. `tabBarRows` derives it from the same list `Tabs` renders, via a shared `visibleTabs(flags)` — the duplicated filter being how the two would drift apart again. Nine tests in a new `tabsConfig.test.ts`, including the 80-column regression. Two assert properties rather than instances: rows never decrease as the terminal narrows, and a tab wider than the row gets its own rather than looping. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01ND46Ci64DC3tzBy9aG81Nc Signed-off-by: cliffhall --- clients/tui/__tests__/tabsConfig.test.ts | 101 ++++++++++++++++++ clients/tui/src/App.tsx | 30 +++++- clients/tui/src/components/SkillsTab.tsx | 17 +-- clients/tui/src/components/Tabs.tsx | 23 ++-- clients/tui/src/components/tabsConfig.ts | 70 ++++++++++++ clients/web/src/test/core/mcp/skills.test.ts | 47 ++++++++ .../test/core/mcp/skillsVerification.test.ts | 62 +++++++++++ core/mcp/skills.ts | 24 +++++ core/mcp/skillsVerification.ts | 39 ++++++- 9 files changed, 380 insertions(+), 33 deletions(-) create mode 100644 clients/tui/__tests__/tabsConfig.test.ts diff --git a/clients/tui/__tests__/tabsConfig.test.ts b/clients/tui/__tests__/tabsConfig.test.ts new file mode 100644 index 00000000..2bdb41b5 --- /dev/null +++ b/clients/tui/__tests__/tabsConfig.test.ts @@ -0,0 +1,101 @@ +import { describe, it, expect } from "vitest"; +import { + tabBarRows, + tabs, + visibleTabs, + type TabType, +} from "../src/components/tabsConfig.js"; + +describe("tab accelerators", () => { + it("are unique and appear in their own label", () => { + const seen = new Set(); + for (const tab of tabs) { + expect(tab.label.toLowerCase()).toContain(tab.accelerator); + expect(seen.has(tab.accelerator)).toBe(false); + seen.add(tab.accelerator); + } + }); +}); + +describe("visibleTabs", () => { + it("drops every optional tab when nothing is supported", () => { + const ids = visibleTabs({ + showAuth: false, + showLogging: false, + showRequests: false, + showSkills: false, + }).map((t) => t.id); + expect(ids).not.toContain("auth"); + expect(ids).not.toContain("logging"); + expect(ids).not.toContain("requests"); + expect(ids).not.toContain("skills"); + // The unconditional ones remain. + expect(ids).toContain("info"); + expect(ids).toContain("tools"); + }); + + it("keeps each optional tab when its flag is set", () => { + const ids = visibleTabs({ + showAuth: true, + showLogging: true, + showRequests: true, + showSkills: true, + }).map((t) => t.id); + expect(ids).toEqual(tabs.map((t) => t.id)); + }); +}); + +describe("tabBarRows (#2248)", () => { + /** A stdio, OAuth-capable, Skills-serving server: the widest ordinary bar. */ + const stdioSkills = visibleTabs({ + showAuth: true, + showLogging: true, + showRequests: false, + showSkills: true, + }); + const counts: Partial> = { + resources: 0, + prompts: 0, + skills: 8, + tools: 1, + messages: 11, + logging: 3, + }; + + it("wraps that bar at 80 columns", () => { + // The regression this exists for: adding Skills pushed the bar past a + // default terminal, while `App` assumed one row and sized every pane below + // it one row too tall. + expect(tabBarRows(stdioSkills, counts, 80)).toBeGreaterThan(1); + }); + + it("needs only one row when the bar fits", () => { + expect(tabBarRows(stdioSkills, counts, 400)).toBe(1); + }); + + it("never reports fewer rows as the terminal narrows", () => { + // Monotonicity is the property that matters: a narrower terminal can only + // need the same number of rows or more, so a pane sized from this can + // never grow into the bar. + let previous = 1; + for (const width of [400, 200, 132, 100, 80, 60, 40, 20]) { + const rows = tabBarRows(stdioSkills, counts, width); + expect(rows).toBeGreaterThanOrEqual(previous); + previous = rows; + } + }); + + it("gives a tab wider than the row its own row rather than looping", () => { + expect(tabBarRows(stdioSkills, counts, 1)).toBe(stdioSkills.length); + }); + + it("counts the count suffixes, which are what tip it over", () => { + const withCounts = tabBarRows(stdioSkills, counts, 100); + const without = tabBarRows(stdioSkills, {}, 100); + expect(withCounts).toBeGreaterThanOrEqual(without); + }); + + it("returns one row for an empty bar", () => { + expect(tabBarRows([], {}, 80)).toBe(1); + }); +}); diff --git a/clients/tui/src/App.tsx b/clients/tui/src/App.tsx index e95a1f0e..2bdbe50e 100644 --- a/clients/tui/src/App.tsx +++ b/clients/tui/src/App.tsx @@ -81,6 +81,7 @@ import { InfoTab } from "./components/InfoTab.js"; import { AuthTab } from "./components/AuthTab.js"; import { ResourcesTab } from "./components/ResourcesTab.js"; import { PromptsTab } from "./components/PromptsTab.js"; +import { tabBarRows, visibleTabs } from "./components/tabsConfig.js"; import { SkillsTab } from "./components/SkillsTab.js"; import { ToolsTab } from "./components/ToolsTab.js"; import { NotificationsTab } from "./components/NotificationsTab.js"; @@ -1621,14 +1622,37 @@ function App({ // Calculate layout dimensions const headerHeight = 1; - const tabsHeight = 1; + const serverListWidth = Math.floor(dimensions.width * 0.3); + const contentWidth = dimensions.width - serverListWidth; + // Derived, not assumed. The bar wraps once the visible tabs exceed the + // terminal width — which a stdio server with Skills does at any ordinary + // width — and a hard-coded 1 sized every pane below it one row too tall, + // clipping the bottom of the TUI (Copilot). + const tabsHeight = tabBarRows( + visibleTabs({ + showAuth: !!( + selectedServer && + selectedServerConfig && + isOAuthCapableServerConfig(selectedServerConfig) + ), + showLogging: + !!selectedServer && + inspectorClients[selectedServer]?.getServerType() === "stdio", + showRequests: + !!selectedServer && + (inspectorClients[selectedServer]?.getServerType() === "sse" || + inspectorClients[selectedServer]?.getServerType() === + "streamable-http"), + showSkills: showSkillsTab, + }), + tabCounts, + contentWidth, + ); // Server details will be flexible - calculate remaining space for content const availableHeight = dimensions.height - headerHeight - tabsHeight; // Reserve space for server details (will grow as needed, but we'll use flexGrow) const serverDetailsMinHeight = 3; const contentHeight = availableHeight - serverDetailsMinHeight; - const serverListWidth = Math.floor(dimensions.width * 0.3); - const contentWidth = dimensions.width - serverListWidth; const getStatusColor = (status: string) => { switch (status) { diff --git a/clients/tui/src/components/SkillsTab.tsx b/clients/tui/src/components/SkillsTab.tsx index 7e72574c..b1bf292d 100644 --- a/clients/tui/src/components/SkillsTab.tsx +++ b/clients/tui/src/components/SkillsTab.tsx @@ -27,6 +27,7 @@ import { checkSkillConformance, checkSkillNameCollisions, skillDisplayName, + skillEntryKey, skillUriIdentity, type SkillIssue, } from "@inspector/core/mcp/skills.js"; @@ -87,18 +88,6 @@ const FILE_COLOR: Record = { "read-error": "red", }; -/** - * What a verification result is a result *about*: the whole entry, serialized. - * - * `JSON.stringify` is enough here — this compares an entry against a later copy - * of *itself* from the same server, so key order is stable and there is no need - * for the canonical form `skillEntriesMatch` uses to compare two independently - * produced entries. - */ -function entryKey(entry: SkillEntry): string { - return JSON.stringify(entry); -} - /** * The explanation printed under a failed file row. * @@ -185,7 +174,7 @@ export function SkillsTab({ void (async () => { try { const [result] = await verifySkills(inspectorClient, [skill]); - setReport({ key: entryKey(skill), result }); + setReport({ key: skillEntryKey(skill), result }); } catch (err) { if (err instanceof AuthRecoveryRequiredError) { onAuthRecoveryRequired?.(err); @@ -265,7 +254,7 @@ export function SkillsTab({ }; const issues = selectedSkill ? findingsFor(selectedSkill) : []; const activeReport = - selectedSkill && report?.key === entryKey(selectedSkill) + selectedSkill && report?.key === skillEntryKey(selectedSkill) ? report.result : null; const manifest = diff --git a/clients/tui/src/components/Tabs.tsx b/clients/tui/src/components/Tabs.tsx index 71e447dc..2a23d216 100644 --- a/clients/tui/src/components/Tabs.tsx +++ b/clients/tui/src/components/Tabs.tsx @@ -1,6 +1,6 @@ import React from "react"; import { Box, Text } from "ink"; -import { type TabType, tabs } from "./tabsConfig.js"; +import { type TabType, visibleTabs as visibleTabsFor } from "./tabsConfig.js"; /** * Split a tab label so the accelerator letter can be underlined wherever it @@ -59,19 +59,14 @@ export function Tabs({ showRequests = false, showSkills = false, }: TabsProps) { - let visibleTabs = tabs; - if (!showAuth) { - visibleTabs = visibleTabs.filter((tab) => tab.id !== "auth"); - } - if (!showLogging) { - visibleTabs = visibleTabs.filter((tab) => tab.id !== "logging"); - } - if (!showRequests) { - visibleTabs = visibleTabs.filter((tab) => tab.id !== "requests"); - } - if (!showSkills) { - visibleTabs = visibleTabs.filter((tab) => tab.id !== "skills"); - } + // Shared with `App`, which sizes the pane below this bar from the same list — + // see `tabBarRows`. + const visibleTabs = visibleTabsFor({ + showAuth, + showLogging, + showRequests, + showSkills, + }); return ( { + if (tab.id === "auth") return v.showAuth; + if (tab.id === "logging") return v.showLogging; + if (tab.id === "requests") return v.showRequests; + if (tab.id === "skills") return v.showSkills; + return true; + }); +} + +/** Rendered width of one tab: the 2-column marker, the label, and any count. */ +function tabWidth( + tab: { id: TabType; label: string }, + counts: Partial>, +): number { + const count = counts[tab.id]; + return ( + 2 + tab.label.length + (count === undefined ? 0 : ` (${count})`.length) + ); +} + +/** + * How many terminal rows the tab bar occupies at a given width. + * + * ⚠️ **Not always 1.** Adding Skills pushed a stdio server's bar past 100 + * columns, so it wraps at any ordinary terminal width — and `App` hard-coded + * `tabsHeight = 1`, sizing every content pane one row too tall and clipping the + * bottom of the TUI (Copilot). Deriving the height from the same list `Tabs` + * renders is what keeps the two in agreement as tabs are added. + * + * The bar is a `flexWrap="wrap"` row with one column of padding each side and + * no gaps, so greedy packing by rendered width matches what Ink lays out. + */ +export function tabBarRows( + visible: readonly { id: TabType; label: string }[], + counts: Partial>, + width: number, +): number { + const inner = Math.max(1, width - 2); + let rows = 1; + let used = 0; + for (const tab of visible) { + const w = tabWidth(tab, counts); + // A tab wider than the whole row still occupies one of its own rather than + // looping forever. + if (used > 0 && used + w > inner) { + rows += 1; + used = w; + } else { + used += w; + } + } + return rows; +} diff --git a/clients/web/src/test/core/mcp/skills.test.ts b/clients/web/src/test/core/mcp/skills.test.ts index f7e40eb3..ebcf1a11 100644 --- a/clients/web/src/test/core/mcp/skills.test.ts +++ b/clients/web/src/test/core/mcp/skills.test.ts @@ -9,6 +9,7 @@ import { checkSkillConformance, checkSkillFrontmatterMatch, checkSkillNameCollisions, + skillEntryKey, getSkillsExtension, isSkillsExtensionSupported, normalizeSkillUri, @@ -1201,3 +1202,49 @@ describe("checkSkillNameCollisions (#2248)", () => { ); }); }); + +describe("skillEntryKey (#2248)", () => { + const base: SkillEntry = { + uri: "skill://demo/SKILL.md", + frontmatter: { name: "demo", description: "A demo" }, + resources: [], + }; + + it("distinguishes two entries that differ anywhere", () => { + expect(skillEntryKey(base)).toBe(skillEntryKey({ ...base })); + expect(skillEntryKey(base)).not.toBe( + skillEntryKey({ + ...base, + frontmatter: { name: "demo", description: "changed" }, + }), + ); + }); + + it("survives frontmatter too deep to serialize", () => { + // `frontmatter` is unbounded server-controlled JSON, and this key is + // computed during render — so `JSON.stringify` let one catalog entry crash + // the pane that exists to report on it (Copilot). + let deep: unknown = "leaf"; + for (let i = 0; i < 60000; i += 1) deep = { a: deep }; + const hostile = { + ...base, + frontmatter: { name: "demo", deep }, + } as unknown as SkillEntry; + expect(() => skillEntryKey(hostile)).not.toThrow(); + expect(skillEntryKey(hostile)).toContain("unrepresentable"); + }); + + it("still separates two unrepresentable entries by identity", () => { + // The fallback is coarse, but it must not collapse distinct skills into + // one key — that would show a verdict under the wrong name. + let deep: unknown = "leaf"; + for (let i = 0; i < 60000; i += 1) deep = { a: deep }; + const a = { ...base, frontmatter: { deep } } as unknown as SkillEntry; + const b = { + ...base, + uri: "skill://other/SKILL.md", + frontmatter: { deep }, + } as unknown as SkillEntry; + expect(skillEntryKey(a)).not.toBe(skillEntryKey(b)); + }); +}); diff --git a/clients/web/src/test/core/mcp/skillsVerification.test.ts b/clients/web/src/test/core/mcp/skillsVerification.test.ts index 5f642447..44323c2b 100644 --- a/clients/web/src/test/core/mcp/skillsVerification.test.ts +++ b/clients/web/src/test/core/mcp/skillsVerification.test.ts @@ -538,6 +538,7 @@ describe("verifySkills (#2248)", () => { const [report] = await verifySkills(client, [skill]); expect(readResource).toHaveBeenCalledTimes(512); expect(report.files).toHaveLength(512); + expect(report.ok).toBe(false); // …and the overage is still REPORTED, so bounding the reads does not // silence the finding that made them unnecessary. expect(report.conformance).toEqual( @@ -581,6 +582,9 @@ describe("verifySkills (#2248)", () => { const [report] = await verifySkills(client, [skill]); // Two fit exactly; the third would cross, so it is never requested. expect(readResource).toHaveBeenCalledTimes(2); + // …and a verification that did not finish must not report success. + expect(report.incomplete).toMatch(/2 of 3 manifest entries/); + expect(report.ok).toBe(false); expect(report.conformance).toEqual( expect.arrayContaining([ expect.objectContaining({ code: "size-limit-exceeded" }), @@ -588,6 +592,61 @@ describe("verifySkills (#2248)", () => { ); }); + it("does not report success for a manifest it could not finish reading", async () => { + // The trade this bound must NOT make: entries past the cap are never + // fetched and `resource-limit-exceeded` is only a warning, so a manifest + // whose 513th file is tampered with returned `ok: true` and the CLI said + // the skill verified — a denial of service swapped for a false pass + // (Copilot). + const skill: SkillEntry = { + uri: "skill://many/SKILL.md", + frontmatter: { name: "many", description: "Over the entry limit" }, + resources: Array.from({ length: 600 }, (_, i) => ({ + uri: i === 0 ? "skill://many/SKILL.md" : `skill://many/f${i}.md`, + digest: `sha256:${"a".repeat(64)}`, + size: 1, + })), + }; + const readResource = vi.fn(async (uri: string) => ({ + result: { contents: [{ uri, text: "x" }] }, + })); + const client = { readResource } as unknown as InspectorClientProtocol; + const [report] = await verifySkills(client, [skill]); + expect(report.incomplete).toBeDefined(); + expect(report.ok).toBe(false); + }); + + it("verifies the entry's own file even when the cap excluded it", async () => { + // The fallback exists for the frontmatter check, but reading the file and + // then skipping the digest its manifest advertised would leave the skill's + // own SKILL.md the one file nobody verified (Copilot). + const skill: SkillEntry = { + uri: "skill://huge/SKILL.md", + frontmatter: { name: "huge", description: "Listed" }, + resources: [ + ...Array.from({ length: 600 }, (_, i) => ({ + uri: `skill://huge/f${i}.md`, + digest: `sha256:${"a".repeat(64)}`, + size: 1, + })), + { + uri: "skill://huge/SKILL.md", + digest: `sha256:${"b".repeat(64)}`, + size: 1, + }, + ], + }; + const readResource = vi.fn(async (uri: string) => ({ + result: { contents: [{ uri, text: "x" }] }, + })); + const client = { readResource } as unknown as InspectorClientProtocol; + const [report] = await verifySkills(client, [skill]); + const self = report.files.find((f) => f.uri === "skill://huge/SKILL.md"); + // A real verdict on the advertised digest, not merely a read. + expect(self?.status).toBe("mismatch"); + expect(self?.expectedDigest).toBe(`sha256:${"b".repeat(64)}`); + }); + it("does not truncate a conforming manifest", async () => { // A conforming skill totals at most 16 MiB by definition, so the bound // must never shorten one — otherwise it would trade a hostile-server @@ -600,6 +659,9 @@ describe("verifySkills (#2248)", () => { const [report] = await verifySkills(client, [skill]); expect(readResource).toHaveBeenCalledTimes(2); expect(report.files).toHaveLength(2); + // Nothing was skipped, so nothing is reported as incomplete. + expect(report.incomplete).toBeUndefined(); + expect(report.ok).toBe(true); }); it("still reads the entry's own file when the cap would exclude it", async () => { diff --git a/core/mcp/skills.ts b/core/mcp/skills.ts index 1565abfd..8ae718d5 100644 --- a/core/mcp/skills.ts +++ b/core/mcp/skills.ts @@ -513,6 +513,30 @@ export function totalSkillBytes(resources: readonly SkillResource[]): number { ); } +/** + * A stable key for "this exact entry", safe against a hostile listing. + * + * `JSON.stringify(entry)` is the obvious implementation and is the wrong one: + * `frontmatter` is unbounded server-controlled JSON, so a deep enough object + * throws `RangeError: Maximum call stack size exceeded` — and this is evaluated + * during render, so one catalog entry could crash the pane that exists to + * report on it (Copilot). + * + * The same guard that bounds the frontmatter comparison decides it here. When + * the entry is representable the key is its serialization, which is exact; + * when it is not, the key falls back to the entry's identity plus its manifest + * length. That fallback is deliberately coarse — such an entry already carries + * a `frontmatter-unparsable` error, so what matters is that it produces a + * usable key rather than a precise one. + */ +export function skillEntryKey(entry: SkillEntry): string { + if (jsonGraphError(entry) !== undefined) { + const count = Array.isArray(entry.resources) ? entry.resources.length : -1; + return `${skillUriIdentity(entry.uri)}#unrepresentable:${count}`; + } + return JSON.stringify(entry); +} + /** * Whether a `skills/get` entry describes the same skill as the `skills/list` * entry alongside it, compared **semantically** rather than byte-for-byte. diff --git a/core/mcp/skillsVerification.ts b/core/mcp/skillsVerification.ts index 666dd263..2ced0087 100644 --- a/core/mcp/skillsVerification.ts +++ b/core/mcp/skillsVerification.ts @@ -91,9 +91,24 @@ export interface SkillVerifyReport { * `files` mirrors the manifest one-for-one (Copilot). */ files: SkillFileReport[]; + /** + * Why the manifest could not be checked in full, or `undefined` when it was. + * + * Set when the read bounds truncated the manifest. It is reported separately + * from `ok` being false so a consumer can tell "this skill is wrong" from + * "this skill was not fully checked" — but it does make `ok` false, because + * the alternative is worse: entries past the cap are never fetched, and + * `resource-limit-exceeded` is only a WARNING, so a manifest whose 513th + * file was tampered with reported `ok: true` and the CLI said the skill + * verified (Copilot). A bound that turns a denial of service into a false + * pass has traded down. + */ + incomplete?: string; /** * False when anything the SEP makes a MUST was broken: an error-severity - * finding, a digest or size mismatch, or a file that could not be read. + * finding, a digest or size mismatch, or a file that could not be read — + * **or when {@link incomplete} is set**, since a verification that did not + * finish cannot report success. * * A `warning` does **not** clear it — a `"dynamic"` manifest is legal, and a * report that failed CI for it would be telling server authors their @@ -247,6 +262,10 @@ export async function verifySkills( // running total would CROSS the limit, so a conforming skill (≤ 16 MiB in // total, by definition) is never truncated. const manifest = boundedManifest(declared); + const incomplete = + manifest.length < declared.length + ? `Only ${manifest.length} of ${declared.length} manifest entries were read: the skill exceeds the ${SKILL_MAX_RESOURCE_ENTRIES}-entry / ${SKILL_MAX_TOTAL_BYTES}-byte interoperability limits, so the rest were not fetched and cannot be reported on.` + : undefined; const entryIdentity = skillUriIdentity(entry.uri); // Compared by NORMALIZED identity, like every other URI comparison here — // `checkSkillConformance` already accepts a manifest self-entry written in @@ -327,6 +346,20 @@ export async function verifySkills( fail(reasonOf(err)); } } + // If the DECLARED manifest lists this file but the read bounds + // excluded it, verify it here too. The fallback exists for the + // frontmatter check, but reading a file and then not checking the + // digest the manifest advertised for it would leave the entry's own + // SKILL.md the one file nobody verified (Copilot). + const declaredSelf = declared.find( + (resource) => skillUriIdentity(resource.uri) === entryIdentity, + ); + if (declaredSelf && entryBytes !== undefined) { + files.push({ + uri: entry.uri, + ...(await verifySkillResource(declaredSelf, entryBytes)), + }); + } } catch (err) { // An expired authorization is the one error that is not this file's // problem — see the note on the function. @@ -359,7 +392,9 @@ export async function verifySkills( conformance, frontmatter, files, - ok: !hasError && !fileFailed, + ...(incomplete ? { incomplete } : {}), + // An unfinished verification is not a passing one. + ok: !hasError && !fileFailed && incomplete === undefined, }); } return reports;