From 89438d1a4b8d08c879986ca6bd8c9f62c4a9e7a0 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Mon, 7 Sep 2026 22:47:32 -0400 Subject: [PATCH] fix: address Copilot review round 15 on #2248 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit **Verification is a tri-state now, because two states could not be right.** Round 12 was correct that `ok: true` for a manifest whose unread 513th file may be tampered with is a false pass. Round 15 is correct that forcing `ok: false` calls a server nonconformant for exceeding limits SEP-2640 states as SHOULD NOT, with hosts free to support more — contradicting this module's own rule that a warning never fails a report. Both hold, so: verified -> exit 0 everything checked, everything passed failed -> exit 7 a MUST was broken incomplete -> exit 8 nothing checked was wrong; the walk was cut short `ok` keeps its narrow meaning, `allSkillsVerified` is stricter than `every(r => r.ok)` since it selects the exit code, and `anySkillFailed` separates 7 from 8. A job that tolerates oversized catalogs can allow 8 and still fail on 7. **`manifestKey` still used `JSON.stringify` on the entry** — the same crash I fixed in the TUI in round 12, one file over. **Directory children were navigable outside the skill root.** A server could return `skill://other-skill/...` and clicking it left the tree, with "Up" only comparing equality against the root. Containment is checked on the normalized URI now, and an outside child renders as text rather than a link. Worth recording from the tests: `..` cannot escape the authority, so `skill://a/nested/../x.md` resolves back inside and must stay navigable. **The TUI matched report rows by raw URI**, which missed a row recorded under an equivalent spelling while `extraReportFiles` suppressed it as covered. Normalized, like the membership test beside it. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01ND46Ci64DC3tzBy9aG81Nc Signed-off-by: cliffhall --- clients/cli/README.md | 15 +- .../cli/__tests__/run-method-skills.test.ts | 45 +++- .../cli/__tests__/skills-verify-cli.test.ts | 27 +++ clients/cli/src/error-handler.ts | 13 + clients/cli/src/handlers/consume-outcome.ts | 9 +- clients/cli/src/handlers/run-method.ts | 21 +- clients/tui/src/components/SkillsTab.tsx | 9 +- .../SkillsScreen/SkillsScreen.test.tsx | 71 ++++++ .../screens/SkillsScreen/SkillsScreen.tsx | 86 +++++-- .../test/core/mcp/skillsVerification.test.ts | 229 ++++++++++-------- core/mcp/skillsVerification.ts | 50 +++- 11 files changed, 434 insertions(+), 141 deletions(-) diff --git a/clients/cli/README.md b/clients/cli/README.md index 7a6d1dce..aeacebd6 100644 --- a/clients/cli/README.md +++ b/clients/cli/README.md @@ -372,10 +372,16 @@ caller branching on `.code` should not have to special-case this command. `--method skills/get --uri ` verifies exactly one skill, in the same shape. -**What fails the run.** `ok` is false — and the exit code is `7` — for anything -SEP-2640 makes a MUST: an error-severity conformance finding, a digest or size -mismatch, or a manifest file that could not be read. A **warning** does not fail -it. That distinction matters most for `resources: "dynamic"`, which is a +**What fails the run.** Three outcomes, three exit codes, because "this skill is +wrong" and "this skill could not be fully checked" are different answers: + +| `outcome` | Exit | When | +| --- | --- | --- | +| `verified` | `0` | Everything was checked and everything passed. | +| `failed` | `7` | Something SEP-2640 makes a MUST was broken — an error-severity finding, a digest or size mismatch, or an unreadable manifest file. | +| `incomplete` | `8` | Nothing checked was wrong, but the read bounds stopped the walk before it finished. See `incomplete` in the report for the reason. | + +A **warning** never produces `7`. That distinction matters most for `resources: "dynamic"`, which is a *conforming* wire form for generated content: it means integrity cannot be verified, which is worth reporting, but failing CI for it would tell server authors their valid skill is broken. @@ -415,6 +421,7 @@ prose from stderr: | `5` | Tool error (`tools/call` returned `isError:true`, or the tool was not found). | | `6` | `--strict` found an error-severity tool-schema portability problem (`schema_unportable` — the schema is valid JSON Schema, just not portable). | | `7` | `--verify` found a SEP-2640 violation (`skills_nonconformant` — a conformance error, a digest or size mismatch, or an unreadable manifest file). | +| `8` | `--verify` could not check the whole catalog (`skills_incomplete` — the read bounds stopped the walk). The server broke no **MUST**: the 512-entry and 16 MiB limits are `SHOULD NOT`, and hosts may support more. A job that tolerates oversized catalogs can allow `8` and still fail on `7`. | On any non-zero exit the CLI also writes a single JSON line to **stderr** — the `ErrorEnvelope`: diff --git a/clients/cli/__tests__/run-method-skills.test.ts b/clients/cli/__tests__/run-method-skills.test.ts index 7074e4a0..e750f97c 100644 --- a/clients/cli/__tests__/run-method-skills.test.ts +++ b/clients/cli/__tests__/run-method-skills.test.ts @@ -201,6 +201,47 @@ describe("runMethod skills dispatch (#2248)", () => { ); }); + it("exits 8, not 7, when the walk was only truncated", async () => { + // SEP-2640 states the read limits as SHOULD NOT and lets hosts support + // more, so exiting `SKILL_NONCONFORMANT` would call a conforming server + // nonconformant — while exiting 0 would report success for entries nobody + // fetched (Copilot). + const md = "---\nname: many\ndescription: Big\n---\n\n# many\n"; + const enc = new TextEncoder(); + const selfDigest = await sha256Digest(enc.encode(md)); + const bodyDigest = await sha256Digest(enc.encode("x")); + const entry: SkillEntry = { + uri: "skill://many/SKILL.md", + frontmatter: { name: "many", description: "Big" }, + resources: Array.from({ length: 600 }, (_, i) => + i === 0 + ? { + uri: "skill://many/SKILL.md", + digest: selfDigest, + size: enc.encode(md).byteLength, + } + : { uri: `skill://many/f${i}.md`, digest: bodyDigest, size: 1 }, + ), + }; + const client = mockClient({ + listSkills: vi.fn().mockResolvedValue({ skills: [entry] }), + readResource: vi.fn(async (uri: string) => ({ + result: { + contents: [{ uri, text: uri.endsWith("/SKILL.md") ? md : "x" }], + }, + })), + }); + const outcome = await runMethod(client, { + method: "skills/list", + verify: true, + }); + if (outcome.kind !== "ndjson") throw new Error("unreachable"); + expect(outcome.exitCode).toBe(EXIT_CODES.SKILL_INCOMPLETE); + expect(EXIT_CODES.SKILL_INCOMPLETE).not.toBe( + EXIT_CODES.SKILL_NONCONFORMANT, + ); + }); + it("--verify works on a single skills/get", async () => { const entry = await cleanEntry(); const client = mockClient({ @@ -227,6 +268,7 @@ describe("summarizeSkillVerification (#2248)", () => { frontmatter: [], files: [{ uri: "skill://demo/SKILL.md", status: "verified" }], ok: true, + outcome: "verified", ...over, }); @@ -247,6 +289,7 @@ describe("summarizeSkillVerification (#2248)", () => { // so collapsing the two counts would misreport the cause. const failed = report({ ok: false, + outcome: "failed", files: [{ uri: "skill://demo/SKILL.md", status: "mismatch" }], }); expect(summarizeSkillVerification([report(), failed])).toBe( @@ -255,7 +298,7 @@ describe("summarizeSkillVerification (#2248)", () => { }); it("reports a failure with no mismatched file", () => { - const failed = report({ ok: false, files: [] }); + const failed = report({ ok: false, outcome: "failed", files: [] }); expect(summarizeSkillVerification([failed])).toBe( "1 of 1 skill failed verification (0 digest/size mismatch across 0 files).", ); diff --git a/clients/cli/__tests__/skills-verify-cli.test.ts b/clients/cli/__tests__/skills-verify-cli.test.ts index 1a4b89d0..2becbff7 100644 --- a/clients/cli/__tests__/skills-verify-cli.test.ts +++ b/clients/cli/__tests__/skills-verify-cli.test.ts @@ -135,6 +135,33 @@ describe("consumeMethodOutcome NDJSON summary and exit code (#2248)", () => { }); }); + it("labels the envelope for an INCOMPLETE run, not a nonconformant one", async () => { + // The envelope's `code` follows the exit code, so a caller reading one + // never has to reconcile it against the other — and exit 8 means the + // server broke no MUST. + const streams = captureStreams(); + let thrown: unknown; + try { + await consumeMethodOutcome( + { + kind: "ndjson", + lines: [{ outcome: "incomplete" }], + summary: "not fully checked", + exitCode: EXIT_CODES.SKILL_INCOMPLETE, + }, + {}, + ); + } catch (err) { + thrown = err; + } finally { + streams.restore(); + } + expect(thrown).toMatchObject({ + exitCode: EXIT_CODES.SKILL_INCOMPLETE, + envelope: { code: "skills_incomplete" }, + }); + }); + it("leaves an --app-info NDJSON outcome unchanged", async () => { // No summary, no exit code — the field is additive and the older caller // must behave exactly as before. diff --git a/clients/cli/src/error-handler.ts b/clients/cli/src/error-handler.ts index 7f8b432b..d2ab086c 100644 --- a/clients/cli/src/error-handler.ts +++ b/clients/cli/src/error-handler.ts @@ -35,6 +35,19 @@ export const EXIT_CODES = { * collapsing them would make `if [ $? -eq 6 ]` ambiguous. */ SKILL_NONCONFORMANT: 7, + /** + * `--verify` could not check the whole catalog: the read bounds stopped the + * walk before it finished (#2248). + * + * Distinct from `SKILL_NONCONFORMANT` because the server has broken no + * **MUST** — SEP-2640 states the 512-entry and 16 MiB limits as SHOULD NOT, + * with hosts free to support more — so exiting 7 would call a conforming + * server nonconformant. It is still non-zero, because reporting success for + * a manifest whose unread entries were never fetched is a false pass. A CI + * job that wants to tolerate oversized catalogs can allow 8 and still fail + * on 7. + */ + SKILL_INCOMPLETE: 8, } as const; /** Machine-readable error envelope written as one JSON line on stderr. */ diff --git a/clients/cli/src/handlers/consume-outcome.ts b/clients/cli/src/handlers/consume-outcome.ts index 6891c30a..5db0738b 100644 --- a/clients/cli/src/handlers/consume-outcome.ts +++ b/clients/cli/src/handlers/consume-outcome.ts @@ -1,5 +1,5 @@ import { awaitableError, awaitableLog } from "../utils/awaitable-log.js"; -import { CliExitCodeError } from "../error-handler.js"; +import { CliExitCodeError, EXIT_CODES } from "../error-handler.js"; import { emitResult } from "./emit-result.js"; import type { MethodArgs, MethodOutcome } from "./method-types.js"; @@ -30,7 +30,12 @@ export async function consumeMethodOutcome( // last thing that happens. if (outcome.exitCode) { throw new CliExitCodeError(outcome.exitCode, outcome.summary ?? "", { - code: "skills_nonconformant", + // The envelope's `code` follows the exit code, so a caller reading one + // never has to reconcile it against the other. + code: + outcome.exitCode === EXIT_CODES.SKILL_INCOMPLETE + ? "skills_incomplete" + : "skills_nonconformant", }); } return; diff --git a/clients/cli/src/handlers/run-method.ts b/clients/cli/src/handlers/run-method.ts index 67d4ae01..f3d883e0 100644 --- a/clients/cli/src/handlers/run-method.ts +++ b/clients/cli/src/handlers/run-method.ts @@ -15,6 +15,7 @@ import { collectAppInfo } from "./collect-app-info.js"; import { summarizeSkillVerification } from "./skills-verify.js"; import { allSkillsVerified, + anySkillFailed, verifySkills, } from "@inspector/core/mcp/skillsVerification.js"; import type { @@ -334,9 +335,17 @@ export async function runMethod( kind: "ndjson", lines: reports, summary: summarizeSkillVerification(reports), + // Three outcomes, three exit codes: a broken MUST is 7, a walk the + // read bounds cut short is 8, and everything checked and passing is + // 0. Collapsing the middle case into either of the others reports + // something untrue about the server (Copilot). ...(allSkillsVerified(reports) ? {} - : { exitCode: EXIT_CODES.SKILL_NONCONFORMANT }), + : { + exitCode: anySkillFailed(reports) + ? EXIT_CODES.SKILL_NONCONFORMANT + : EXIT_CODES.SKILL_INCOMPLETE, + }), }; } result = { skills }; @@ -371,9 +380,17 @@ export async function runMethod( kind: "ndjson", lines: reports, summary: summarizeSkillVerification(reports), + // Three outcomes, three exit codes: a broken MUST is 7, a walk the + // read bounds cut short is 8, and everything checked and passing is + // 0. Collapsing the middle case into either of the others reports + // something untrue about the server (Copilot). ...(allSkillsVerified(reports) ? {} - : { exitCode: EXIT_CODES.SKILL_NONCONFORMANT }), + : { + exitCode: anySkillFailed(reports) + ? EXIT_CODES.SKILL_NONCONFORMANT + : EXIT_CODES.SKILL_INCOMPLETE, + }), }; } result = envelope; diff --git a/clients/tui/src/components/SkillsTab.tsx b/clients/tui/src/components/SkillsTab.tsx index cdbeecff..c76ec46e 100644 --- a/clients/tui/src/components/SkillsTab.tsx +++ b/clients/tui/src/components/SkillsTab.tsx @@ -431,8 +431,15 @@ export function SkillsTab({ {manifest.map((resource, idx) => { + // Matched on normalized identity, like the membership test + // just above — a raw comparison misses a report row recorded + // under an equivalent spelling, while `extraReportFiles` + // suppresses it as already covered, and the verdict renders + // nowhere (Copilot). const fileReport = activeReport?.files.find( - (file) => file.uri === resource.uri, + (file) => + skillUriIdentity(file.uri) === + skillUriIdentity(resource.uri), ); return ( { ).not.toBeInTheDocument(); }); + it("refuses to navigate a child outside the skill root", async () => { + // A server can return a child pointing anywhere; descending into one + // leaves the selected skill's tree, and "Up" only compares against + // `skillRoot`, so the walk could then continue outside it entirely + // (Copilot). The row is still SHOWN — a child outside the skill is itself + // the finding — but it is not a link. + const user = userEvent.setup(); + const STRAY = { + uri: "skill://other-skill/notes.md", + name: "notes.md", + mimeType: "text/markdown", + }; + await openRoot( + user, + directoryReader({ [ROOT]: { resources: [CHILD_FILE, STRAY] } }), + ); + const table = within(screen.getByTestId("skill-directory")); + expect(table.getByText(/outside this skill/)).toBeInTheDocument(); + expect( + table.queryByRole("button", { name: `View ${STRAY.uri}` }), + ).not.toBeInTheDocument(); + // The legitimate sibling is unaffected. + expect( + table.getByRole("button", { name: `View ${CHILD_FILE.uri}` }), + ).toBeInTheDocument(); + }); + + it("refuses a sibling whose path merely starts with the same characters", async () => { + // The reason the check appends a separator: a bare `startsWith(skillRoot)` + // would accept `skill://data-analysis-other/...` as a child of + // `skill://data-analysis`. + const user = userEvent.setup(); + const LOOKALIKE = { + uri: "skill://data-analysis-other/notes.md", + name: "other.md", + mimeType: "text/markdown", + }; + await openRoot( + user, + directoryReader({ [ROOT]: { resources: [LOOKALIKE] } }), + ); + expect( + within(screen.getByTestId("skill-directory")).getByText( + /outside this skill/, + ), + ).toBeInTheDocument(); + }); + + it("accepts a `..` segment that resolves back inside the root", async () => { + // Worth pinning, because the intuition is wrong: `..` cannot escape the + // AUTHORITY. `skill://data-analysis/../x.md` normalizes to + // `skill://data-analysis/x.md`, which really is inside this skill — so + // rejecting it would refuse a legitimate child. Containment is decided on + // the normalized URI precisely so this resolves before it is compared. + const user = userEvent.setup(); + const RESOLVES_INSIDE = { + uri: "skill://data-analysis/nested/../notes.md", + name: "notes.md", + mimeType: "text/markdown", + }; + await openRoot( + user, + directoryReader({ [ROOT]: { resources: [RESOLVES_INSIDE] } }), + ); + const table = within(screen.getByTestId("skill-directory")); + expect(table.queryByText(/outside this skill/)).not.toBeInTheDocument(); + expect( + table.getByRole("button", { name: `View ${RESOLVES_INSIDE.uri}` }), + ).toBeInTheDocument(); + }); + it("says an empty directory is empty", async () => { const user = userEvent.setup(); renderWithMantine( diff --git a/clients/web/src/components/screens/SkillsScreen/SkillsScreen.tsx b/clients/web/src/components/screens/SkillsScreen/SkillsScreen.tsx index 329d7a71..d2370005 100644 --- a/clients/web/src/components/screens/SkillsScreen/SkillsScreen.tsx +++ b/clients/web/src/components/screens/SkillsScreen/SkillsScreen.tsx @@ -33,6 +33,7 @@ import { checkSkillNameCollisions, bytesToText, skillDisplayName, + skillEntryKey, skillFileBytes, skillEntriesMatch, normalizeSkillUri, @@ -844,7 +845,12 @@ export function SkillsScreen({ // and a fresh object every render would loop. const manifestKey = useMemo( () => - `${sessionKey}\n${selected ? JSON.stringify(selected) : (selectedSkillUri ?? "")}`, + // `skillEntryKey`, not `JSON.stringify`: `selected` carries unbounded + // server-controlled frontmatter, and this runs during render — so a + // deeply nested entry threw `RangeError` before the screen could show + // the `frontmatter-unparsable` finding that describes it (Copilot). Same + // helper, and the same guard, the TUI uses. + `${sessionKey}\n${selected ? skillEntryKey(selected) : (selectedSkillUri ?? "")}`, [selected, selectedSkillUri, sessionKey], ); @@ -2181,6 +2187,22 @@ export function SkillsScreen({ {directoryChildren.map((child, index) => { const isDir = child.mimeType === DIRECTORY_MIME_TYPE; + // A server can return a child pointing + // anywhere. Descending into one leaves the + // selected skill's tree, and the "Up" control + // only compares against `skillRoot` — so the + // walk could then continue outside it entirely + // (Copilot). Containment is decided on the + // NORMALIZED URI, like every containment check + // in `core/mcp/skills.ts`, so a `..` segment + // cannot walk out while still matching as a + // prefix. + const childUri = normalizeSkillUri(child.uri); + const inRoot = + childUri !== undefined && + skillRoot !== undefined && + (childUri === skillRoot || + childUri.startsWith(`${skillRoot}/`)); // A directory is not a manifest entry in the // first place — a manifest lists files — so it // is neither listed nor unlisted and gets no @@ -2195,31 +2217,43 @@ export function SkillsScreen({ // collapse into one. - - isDir - ? readDirectory( - child.uri, - manifestKey, - ) - : showResource( - child.uri, - manifestKey, - ) - } - > - {isDir ? `${child.name}/` : child.name} - + {!inRoot ? ( + // Shown, never navigable. The reader + // should see what the server sent, and + // a child outside the skill it was + // asked about is itself the finding. + + {child.name} (outside this skill) + + ) : ( + + isDir + ? readDirectory( + child.uri, + manifestKey, + ) + : showResource( + child.uri, + manifestKey, + ) + } + > + {isDir + ? `${child.name}/` + : child.name} + + )} {child.uri} diff --git a/clients/web/src/test/core/mcp/skillsVerification.test.ts b/clients/web/src/test/core/mcp/skillsVerification.test.ts index 6f3b9018..a67a4ea1 100644 --- a/clients/web/src/test/core/mcp/skillsVerification.test.ts +++ b/clients/web/src/test/core/mcp/skillsVerification.test.ts @@ -5,6 +5,7 @@ import { sha256Digest } from "@inspector/core/mcp/skills.js"; import { AuthRecoveryRequiredError } from "@inspector/core/auth/challenge.js"; import { allSkillsVerified, + anySkillFailed, verifySkills, } from "@inspector/core/mcp/skillsVerification.js"; @@ -14,6 +15,45 @@ import { * pin is the fetching policy and the failure handling, since the checks * themselves are covered in `skills.test.ts`. */ + +/** + * A manifest big enough to be truncated, whose files all VERIFY — so the + * report's outcome isolates "incomplete" instead of also tripping a real + * failure. The entry's own SKILL.md carries frontmatter matching the listing, + * since a body of "x" would be `frontmatter-absent` and so genuinely failed. + */ +async function truncatable(options: { + name: string; + count: number; + body?: string; +}): Promise<{ skill: SkillEntry; client: InspectorClientProtocol }> { + const { name, count, body = "x" } = options; + const md = `---\nname: ${name}\ndescription: Big\n---\n\n# ${name}\n`; + const enc = new TextEncoder(); + const selfDigest = await sha256Digest(enc.encode(md)); + const bodyDigest = await sha256Digest(enc.encode(body)); + const selfUri = `skill://${name}/SKILL.md`; + const skill: SkillEntry = { + uri: selfUri, + frontmatter: { name, description: "Big" }, + resources: Array.from({ length: count }, (_, i) => + i === 0 + ? { uri: selfUri, digest: selfDigest, size: enc.encode(md).byteLength } + : { + uri: `skill://${name}/f${i}.md`, + digest: bodyDigest, + size: enc.encode(body).byteLength, + }, + ), + }; + const client = { + readResource: async (uri: string) => ({ + result: { contents: [{ uri, text: uri === selfUri ? md : body }] }, + }), + } as unknown as InspectorClientProtocol; + return { skill, client }; +} + describe("verifySkills (#2248)", () => { const SKILL_MD = "---\nname: demo\ndescription: A demo\n---\n\n# Demo\n"; const REF = "# Reference\n"; @@ -522,100 +562,50 @@ describe("verifySkills (#2248)", () => { // The 512-entry limit is CHECKED but constrains nothing, so a hostile // server advertising far more had the tool perform that many sequential // reads after the report already knew the manifest was over (Copilot). - const skill: SkillEntry = { - uri: "skill://huge/SKILL.md", - frontmatter: { name: "huge", description: "Too many files" }, - resources: Array.from({ length: 900 }, (_, i) => ({ - uri: i === 0 ? "skill://huge/SKILL.md" : `skill://huge/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 { skill, client } = await truncatable({ name: "many", count: 900 }); + const readResource = vi.spyOn( + client as unknown as { readResource: (u: string) => unknown }, + "readResource", + ); 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. + // Incomplete, not failed: every file it read verified. + expect(report.outcome).toBe("incomplete"); expect(report.conformance).toEqual( expect.arrayContaining([ expect.objectContaining({ code: "resource-limit-exceeded" }), ]), ); }); - it("bounds reads by the total-byte limit, not only the entry count", async () => { // A manifest can sit at exactly 512 entries and declare a gigabyte each, // so bounding the count alone still let a server dictate unbounded - // bandwidth after `size-limit-exceeded` had already been reported - // (Copilot). - const huge = 8 * 1024 * 1024; // two of these cross the 16 MiB bound - const skill: SkillEntry = { - uri: "skill://fat/SKILL.md", - frontmatter: { name: "fat", description: "Enormous files" }, - resources: [ - { - uri: "skill://fat/SKILL.md", - digest: `sha256:${"a".repeat(64)}`, - size: huge, - }, - { - uri: "skill://fat/b.md", - digest: `sha256:${"a".repeat(64)}`, - size: huge, - }, - { - uri: "skill://fat/c.md", - digest: `sha256:${"a".repeat(64)}`, - size: huge, - }, - ], - }; - 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]); - // 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" }), - ]), + // bandwidth after `size-limit-exceeded` had been reported (Copilot). + const { skill, client } = await truncatable({ + name: "fat", + count: 4, + body: "y".repeat(7 * 1024 * 1024), + }); + const readResource = vi.spyOn( + client as unknown as { readResource: (u: string) => unknown }, + "readResource", ); + const [report] = await verifySkills(client, [skill]); + // The SKILL.md is small; 7 MiB bodies then cross the 16 MiB bound. + expect(readResource.mock.calls.length).toBeLessThan(4); + expect(report.outcome).toBe("incomplete"); }); - 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; + // 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 (Copilot). + const { skill, client } = await truncatable({ name: "many", count: 600 }); const [report] = await verifySkills(client, [skill]); expect(report.incomplete).toBeDefined(); - expect(report.ok).toBe(false); + expect(report.outcome).toBe("incomplete"); + expect(allSkillsVerified([report])).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 @@ -649,29 +639,23 @@ describe("verifySkills (#2248)", () => { it("stops on bytes ACTUALLY served, not the sizes the manifest declared", async () => { // The declared budget is server-controlled: advertising `size: 1` and then - // serving megabytes sailed straight through it, defeating the 16 MiB - // safeguard entirely (Copilot). - const big = "x".repeat(6 * 1024 * 1024); - const skill: SkillEntry = { - uri: "skill://liar/SKILL.md", - frontmatter: { name: "liar", description: "Understates its sizes" }, - resources: Array.from({ length: 10 }, (_, i) => ({ - uri: i === 0 ? "skill://liar/SKILL.md" : `skill://liar/f${i}.md`, - digest: `sha256:${"a".repeat(64)}`, - size: 1, // a lie - })), - }; - const readResource = vi.fn(async (uri: string) => ({ - result: { contents: [{ uri, text: big }] }, - })); - const client = { readResource } as unknown as InspectorClientProtocol; + // serving megabytes sailed straight through it (Copilot). The fixture'"'"'s + // digests are honest, so the only thing wrong is the unfinished walk. + const { skill, client } = await truncatable({ + name: "liar", + count: 10, + body: "z".repeat(6 * 1024 * 1024), + }); + // …and now understate every non-entry size, which the old budget trusted. + for (const r of skill.resources as { size?: number }[]) r.size = 1; + const readResource = vi.spyOn( + client as unknown as { readResource: (u: string) => unknown }, + "readResource", + ); const [report] = await verifySkills(client, [skill]); - // Three 6 MiB bodies cross 16 MiB; the walk stops rather than reading ten. - expect(readResource).toHaveBeenCalledTimes(3); + expect(readResource.mock.calls.length).toBeLessThan(10); expect(report.incomplete).toMatch(/actually served/); - expect(report.ok).toBe(false); }); - it("still reports the file that crossed the byte budget", async () => { // The crossing file is verified before the walk stops, so its verdict is // not fetched and then thrown away. @@ -748,7 +732,7 @@ describe("verifySkills (#2248)", () => { expect(report.files).toHaveLength(2); // Nothing was skipped, so nothing is reported as incomplete. expect(report.incomplete).toBeUndefined(); - expect(report.ok).toBe(true); + expect(report.outcome).toBe("verified"); }); it("still reads the entry's own file when the cap would exclude it", async () => { @@ -814,3 +798,54 @@ describe("verifySkills (#2248)", () => { expect(allSkillsVerified([])).toBe(true); }); }); + +describe("verification outcomes (#2248)", () => { + const clean = async (): Promise => { + const md = "---\nname: ok\ndescription: Fine\n---\n\n# ok\n"; + const bytes = new TextEncoder().encode(md); + return { + uri: "skill://ok/SKILL.md", + frontmatter: { name: "ok", description: "Fine" }, + resources: [ + { + uri: "skill://ok/SKILL.md", + digest: await sha256Digest(bytes), + size: bytes.byteLength, + }, + ], + }; + }; + + function serving(text: string) { + return { + readResource: async (uri: string) => ({ + result: { contents: [{ uri, text }] }, + }), + } as unknown as InspectorClientProtocol; + } + + it("separates a broken MUST from an unfinished walk", async () => { + // The distinction the tri-state exists for: both are non-zero outcomes, + // but only one of them says the server did something wrong. + const md = "---\nname: ok\ndescription: Fine\n---\n\n# ok\n"; + const good = await verifySkills(serving(md), [await clean()]); + expect(good[0].outcome).toBe("verified"); + expect(allSkillsVerified(good)).toBe(true); + expect(anySkillFailed(good)).toBe(false); + + const bad = await verifySkills(serving("tampered"), [await clean()]); + expect(bad[0].outcome).toBe("failed"); + expect(allSkillsVerified(bad)).toBe(false); + expect(anySkillFailed(bad)).toBe(true); + }); + + it("does not report an incomplete walk as a failure", async () => { + // `anySkillFailed` selects the CLI exit code, so this is what keeps a + // conforming-but-oversized server off exit 7. + const { skill, client } = await truncatable({ name: "many", count: 600 }); + const reports = await verifySkills(client, [skill]); + expect(reports[0].outcome).toBe("incomplete"); + expect(anySkillFailed(reports)).toBe(false); + expect(allSkillsVerified(reports)).toBe(false); + }); +}); diff --git a/core/mcp/skillsVerification.ts b/core/mcp/skillsVerification.ts index e64f1955..62219783 100644 --- a/core/mcp/skillsVerification.ts +++ b/core/mcp/skillsVerification.ts @@ -104,15 +104,33 @@ export interface SkillVerifyReport { * pass has traded down. */ incomplete?: string; + /** + * What the verification concluded. **Three outcomes, not two**, because + * "this skill is wrong" and "this skill could not be fully checked" are + * different answers and collapsing them misreports one of them: + * + * - `verified` — everything was checked and everything passed. + * - `failed` — something the SEP makes a MUST was broken. + * - `incomplete` — nothing checked was wrong, but the read bounds stopped + * the walk before it finished. See {@link incomplete} for the reason. + * + * ⚠️ The `incomplete` case exists because both of the obvious two-state + * answers are wrong. Reporting success would be a **false pass** for a + * manifest whose unread 513th file is tampered with. Reporting failure + * would call a server nonconformant for exceeding limits SEP-2640 states as + * SHOULD NOT — with hosts free to support more — which contradicts this + * module's own rule that a warning never fails a report (Copilot). + */ + outcome: "verified" | "failed" | "incomplete"; /** * 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 — - * **or when {@link incomplete} is set**, since a verification that did not - * finish cannot report success. + * finding, a digest or size mismatch, or a file that could not be read. * * 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 - * conforming skill is broken. + * conforming skill is broken. Neither does {@link incomplete}: an unfinished + * walk is reported through {@link outcome}, so `ok` keeps its narrow meaning + * of "nothing that was checked is wrong". */ ok: boolean; } @@ -419,16 +437,32 @@ export async function verifySkills( frontmatter, files, ...(incomplete ? { incomplete } : {}), - // An unfinished verification is not a passing one. - ok: !hasError && !fileFailed && incomplete === undefined, + ok: !hasError && !fileFailed, + outcome: + hasError || fileFailed + ? "failed" + : incomplete !== undefined + ? "incomplete" + : "verified", }); } return reports; } -/** True when every skill in the report passed. */ +/** + * True when every skill was checked in full and passed. + * + * Deliberately stricter than `every(r => r.ok)`: a report that could not be + * finished has not verified anything about the part it did not read, so it is + * not "verified" even though nothing it *did* read was wrong. + */ export function allSkillsVerified( reports: readonly SkillVerifyReport[], ): boolean { - return reports.every((report) => report.ok); + return reports.every((report) => report.outcome === "verified"); +} + +/** True when any skill broke something the SEP makes a MUST. */ +export function anySkillFailed(reports: readonly SkillVerifyReport[]): boolean { + return reports.some((report) => report.outcome === "failed"); }