diff --git a/clients/web/src/test/core/mcp/inspectorClient-skills.test.ts b/clients/web/src/test/core/mcp/inspectorClient-skills.test.ts index 8c2c1dcb..a254db94 100644 --- a/clients/web/src/test/core/mcp/inspectorClient-skills.test.ts +++ b/clients/web/src/test/core/mcp/inspectorClient-skills.test.ts @@ -388,6 +388,34 @@ describe("InspectorClient skills methods (#2234)", () => { }); }); + it("requires resultType on a modern skills/get", async () => { + // Base-protocol (SEP-2322) and present in SEP-2640's own example, unlike + // the caching attributes the SEP leaves open. + const client = makeClient(); + internals(client).protocolEra = "modern"; + stubRequest(client, { skill: ENTRY }); + await expect( + client.getSkill("skill://demo/SKILL.md"), + ).rejects.toBeDefined(); + }); + + it("accepts a modern skills/get without the caching attributes", async () => { + const client = makeClient(); + internals(client).protocolEra = "modern"; + stubRequest(client, { skill: ENTRY, resultType: "complete" }); + await expect(client.getSkill("skill://demo/SKILL.md")).resolves.toEqual( + ENTRY, + ); + }); + + it("accepts a legacy skills/get without resultType", async () => { + const client = makeClient(); + stubRequest(client, { skill: ENTRY }); + await expect(client.getSkill("skill://demo/SKILL.md")).resolves.toEqual( + ENTRY, + ); + }); + it("rejects a skills/list result that is not a skills page", async () => { // The explicit result schema is the whole client-side mechanism for a // consumer-owned extension method, so a nonconforming result must fail diff --git a/clients/web/src/test/core/mcp/skillsSchemas.test.ts b/clients/web/src/test/core/mcp/skillsSchemas.test.ts index 2bbe3407..53f991e6 100644 --- a/clients/web/src/test/core/mcp/skillsSchemas.test.ts +++ b/clients/web/src/test/core/mcp/skillsSchemas.test.ts @@ -5,7 +5,9 @@ import { ModernDirectoryReadResultSchema, RESOURCES_DIRECTORY_READ_METHOD, DYNAMIC_RESOURCES, + GetSkillEnvelopeSchema, GetSkillResultSchema, + ModernGetSkillEnvelopeSchema, ListSkillsResultSchema, ModernListSkillsResultSchema, SKILLS_EXTENSION_KEY, @@ -283,4 +285,28 @@ describe("GetSkillResultSchema caching attributes (#2248)", () => { }).success, ).toBe(true); }); + + it("requires resultType on the modern envelope, but still not the caching fields", () => { + // "Left open" covers `ttlMs` / `cacheScope` and only those. `resultType` is + // base-protocol (SEP-2322) and appears in SEP-2640's own `skills/get` + // example, so leaving it optional here while requiring it of + // `resources/directory/read` was an inconsistency in this module rather + // than a distinction the spec draws (Copilot). + expect( + ModernGetSkillEnvelopeSchema.safeParse({ skill: ENTRY }).success, + ).toBe(false); + expect( + ModernGetSkillEnvelopeSchema.safeParse({ + skill: ENTRY, + resultType: "complete", + }).success, + ).toBe(true); + }); + + it("keeps the legacy envelope permissive about resultType", () => { + // A 2026-era member a legacy server has no business sending. + expect(GetSkillEnvelopeSchema.safeParse({ skill: ENTRY }).success).toBe( + true, + ); + }); }); diff --git a/clients/web/src/test/core/mcp/skillsVerification.test.ts b/clients/web/src/test/core/mcp/skillsVerification.test.ts index 44323c2b..de12de11 100644 --- a/clients/web/src/test/core/mcp/skillsVerification.test.ts +++ b/clients/web/src/test/core/mcp/skillsVerification.test.ts @@ -647,6 +647,61 @@ describe("verifySkills (#2248)", () => { expect(self?.expectedDigest).toBe(`sha256:${"b".repeat(64)}`); }); + 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; + 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(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. + const big = "x".repeat(17 * 1024 * 1024); + const skill: SkillEntry = { + uri: "skill://liar/SKILL.md", + frontmatter: { name: "liar", description: "One enormous file" }, + resources: [ + { + uri: "skill://liar/SKILL.md", + digest: `sha256:${"a".repeat(64)}`, + size: 1, + }, + { + uri: "skill://liar/b.md", + digest: `sha256:${"a".repeat(64)}`, + size: 1, + }, + ], + }; + const readResource = vi.fn(async (uri: string) => ({ + result: { contents: [{ uri, text: big }] }, + })); + const client = { readResource } as unknown as InspectorClientProtocol; + const [report] = await verifySkills(client, [skill]); + expect(readResource).toHaveBeenCalledTimes(1); + expect(report.files).toHaveLength(1); + expect(report.files[0].status).toBe("mismatch"); + }); + 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 diff --git a/core/mcp/inspectorClient.ts b/core/mcp/inspectorClient.ts index be9bba98..cec72b49 100644 --- a/core/mcp/inspectorClient.ts +++ b/core/mcp/inspectorClient.ts @@ -148,6 +148,7 @@ import { DirectoryReadResultSchema, GetSkillEnvelopeSchema, ListSkillsResultSchema, + ModernGetSkillEnvelopeSchema, ModernDirectoryReadResultSchema, ModernListSkillsResultSchema, RESOURCES_DIRECTORY_READ_METHOD, @@ -5640,13 +5641,21 @@ export class InspectorClient extends InspectorClientEventTarget { uri, ...(effectiveMeta ? { _meta: effectiveMeta } : {}), }; - // The envelope is returned whole; `getSkill` is the one that unwraps. + // Era-aware for the same reason `skills/list` is: the method is + // consumer-owned, so no SDK codec stamps or checks its envelope. The modern + // variant requires `resultType` — a base-protocol member SEP-2322 puts on + // every modern result — and still not the caching attributes, which + // SEP-2640 leaves open. The envelope is returned whole; `getSkill` + // unwraps. + const resultSchema = this.isModernEra() + ? ModernGetSkillEnvelopeSchema + : GetSkillEnvelopeSchema; try { return await this.invokeMcpClient( () => this.client!.request( { method: SKILLS_GET_METHOD, params }, - GetSkillEnvelopeSchema, + resultSchema, this.getRequestOptions(this.progressTokenOf(metadata)), ), { method: SKILLS_GET_METHOD }, diff --git a/core/mcp/skillsSchemas.ts b/core/mcp/skillsSchemas.ts index 4a36ec87..79e76341 100644 --- a/core/mcp/skillsSchemas.ts +++ b/core/mcp/skillsSchemas.ts @@ -183,6 +183,21 @@ export const GetSkillEnvelopeSchema = z.looseObject({ */ export type GetSkillEnvelope = z.infer; +/** + * `skills/get` on a **modern** (2026-07-28+) connection: the envelope plus + * `resultType`, and deliberately still not the caching attributes. + * + * The "left open" quote above covers `ttlMs` / `cacheScope` and **only** those. + * `resultType` is a different thing: SEP-2322 makes it a member of every modern + * result, and SEP-2640's own `skills/get` example carries + * `"resultType": "complete"`. Leaving it optional here while requiring it of + * `resources/directory/read` was an inconsistency in this module rather than a + * distinction the spec draws (Copilot). + */ +export const ModernGetSkillEnvelopeSchema = GetSkillEnvelopeSchema.extend({ + resultType: z.literal("complete"), +}); + /** * `skills/get` result, unwrapped to the entry it carries. * diff --git a/core/mcp/skillsVerification.ts b/core/mcp/skillsVerification.ts index 2ced0087..370c4bc2 100644 --- a/core/mcp/skillsVerification.ts +++ b/core/mcp/skillsVerification.ts @@ -262,10 +262,19 @@ 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 = + let 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; + // ⚠️ Bytes ACTUALLY RECEIVED, which is the only budget a server cannot + // lie its way past. `boundedManifest` above works from DECLARED sizes, and + // those are server-controlled — a manifest advertising `size: 1` (or, since + // the wire schema deliberately accepts it for reporting, no size at all) + // sailed through the declared budget and then served arbitrarily large + // bodies, defeating the 16 MiB safeguard entirely (Copilot). The declared + // prefilter still earns its place by refusing to *schedule* an obviously + // oversized set; this is what stops one that lied. + let receivedBytes = 0; const entryIdentity = skillUriIdentity(entry.uri); // Compared by NORMALIZED identity, like every other URI comparison here — // `checkSkillConformance` already accepts a manifest self-entry written in @@ -311,6 +320,17 @@ export async function verifySkills( if (skillUriIdentity(resource.uri) === entryIdentity) entryBytes = bytes; const verification = await verifySkillResource(resource, bytes); files.push({ uri: resource.uri, ...verification }); + // Counted AFTER verifying this file, so the one that crosses the line is + // still reported rather than fetched and discarded. The next read is what + // stops. ⚠️ This bounds the total across responses, not the size of any + // single one: a first response larger than the cap is already in memory + // by the time it can be measured, which would need a streaming read to + // prevent and is not something this API exposes. + receivedBytes += bytes.byteLength; + if (receivedBytes > SKILL_MAX_TOTAL_BYTES) { + incomplete = `Stopped after ${files.length} of ${declared.length} manifest entries: the files actually served exceed the ${SKILL_MAX_TOTAL_BYTES}-byte interoperability limit, whatever sizes the manifest declared.`; + break; + } } // A `"dynamic"` skill has no manifest, so the loop above read nothing —