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"); }