fix: address Copilot review round 13 on #2248

**The byte budget trusted server-declared sizes.** 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. The 16 MiB safeguard protected against an honest
server only, which is no safeguard.

The walk now tracks bytes ACTUALLY received and stops issuing reads once the
real total crosses the cap, marking the report `incomplete` and so `ok:
false`. The declared prefilter is kept rather than replaced: it refuses to
schedule an obviously oversized set, and the received-byte counter stops one
that lied. The count happens after verifying the crossing file, so that file
is still reported rather than fetched and discarded.

⚠️ This bounds the total across responses, not any single one: a first
response larger than the cap is already in memory before it can be measured,
which would need a streaming read the client API does not expose. Stated in
the code rather than left to look closed.

**A modern `skills/get` did not require `resultType`.** The "left open"
question SEP-2640 states covers `ttlMs` / `cacheScope` and only those;
`resultType` is base-protocol per SEP-2322 and appears in the SEP's own
`skills/get` example. Requiring it of `resources/directory/read` but not here
was an inconsistency in the module rather than a distinction the spec draws.
Era-selected now, with the caching attributes still optional.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01ND46Ci64DC3tzBy9aG81Nc
Signed-off-by: cliffhall <cliff@futurescale.com>

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

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,
+    );
+  });
 });

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

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 },

core/mcp/skillsSchemas.ts

@@ -183,6 +183,21 @@ export const GetSkillEnvelopeSchema = z.looseObject({
  */
 export type GetSkillEnvelope = z.infer<typeof GetSkillEnvelopeSchema>;
 
+/**
+ * `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.
  *

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 —