fix: address Copilot review round 11 on #2248

**Reads were bounded by entry count but not by total bytes.** A manifest can
sit at exactly 512 entries and declare a gigabyte each, so round 9's cap
still let a server dictate unbounded bandwidth and time after
`size-limit-exceeded` had already been reported. `boundedManifest` caps on
both interoperability limits now.

Sizes are summed exactly as `totalSkillBytes` sums them, so the bound and the
finding that reports the overage cannot disagree about the total. A read is
skipped only when the running total would CROSS the limit, so a conforming
skill — at most 16 MiB by definition — is never truncated; there is a test
for that, because a bound that protected against a hostile server by giving a
wrong answer about a good one would be a poor trade. The self-file fallback
still applies, so the mandatory frontmatter check survives either cap.

**The `vitest.shared.mts` pin rationale had gone stale.** It said `yaml` is
reached only through `test-servers/src`, which this PR made untrue by
importing it from `core/mcp/skillFile.ts`. That comment is what a future
dependency-placement change would read before removing or reclassifying the
pin, so it now names the `core/` path and the bundler `external` lists that
follow from it.

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/skillsVerification.test.ts

@@ -547,6 +547,61 @@ describe("verifySkills (#2248)", () => {
     );
   });
 
+  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);
+    expect(report.conformance).toEqual(
+      expect.arrayContaining([
+        expect.objectContaining({ code: "size-limit-exceeded" }),
+      ]),
+    );
+  });
+
+  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
+    // protection for a wrong answer about a good server.
+    const skill = await entry();
+    const { client, readResource } = clientServing({
+      "skill://demo/SKILL.md": SKILL_MD,
+      "skill://demo/ref.md": REF,
+    });
+    const [report] = await verifySkills(client, [skill]);
+    expect(readResource).toHaveBeenCalledTimes(2);
+    expect(report.files).toHaveLength(2);
+  });
+
   it("still reads the entry's own file when the cap would exclude it", async () => {
     // The frontmatter comparison is mandatory and must not be lost to a limit
     // that exists to bound *other* files — so a self-entry pushed past the cap

core/mcp/skillsVerification.ts

@@ -34,13 +34,18 @@ import {
   checkSkillNameCollisions,
   skillDisplayName,
   SKILL_MAX_RESOURCE_ENTRIES,
+  SKILL_MAX_TOTAL_BYTES,
   skillFileBytes,
   skillUriIdentity,
   verifySkillResource,
   type SkillIssue,
   type SkillVerification,
 } from "./skills.js";
-import { DYNAMIC_RESOURCES, type SkillEntry } from "./skillsSchemas.js";
+import {
+  DYNAMIC_RESOURCES,
+  type SkillEntry,
+  type SkillResource,
+} from "./skillsSchemas.js";
 
 /** One manifest entry's outcome. `read-error` means the fetch itself failed. */
 export type SkillFileStatus = SkillVerification["status"] | "read-error";
@@ -71,11 +76,12 @@ export interface SkillVerifyReport {
   /**
    * One entry per manifest file **read**, in manifest order.
    *
-   * ⚠️ Capped at `SKILL_MAX_RESOURCE_ENTRIES`. A manifest longer than that is
-   * already reported by the `resource-limit-exceeded` warning, and reading all
-   * of it would let a server dictate an unbounded number of round trips — so
-   * this can be SHORTER than the declared manifest, and a consumer must not
-   * read its length as the manifest's.
+   * ⚠️ Capped at `SKILL_MAX_RESOURCE_ENTRIES` entries **and**
+   * `SKILL_MAX_TOTAL_BYTES` of declared content. A manifest over either bound
+   * is already reported by `resource-limit-exceeded` / `size-limit-exceeded`,
+   * and reading it anyway would let a server dictate unbounded round trips or
+   * bandwidth — so this can be SHORTER than the declared manifest, and a
+   * consumer must not read its length as the manifest's.
    *
    * ⚠️ **Not necessarily empty for a `"dynamic"` skill.** Such a skill has no
    * manifest rows, but a failed read of its own `SKILL.md` — the file the
@@ -108,6 +114,33 @@ function reasonOf(err: unknown): string {
   return err instanceof Error ? err.message : String(err);
 }
 
+/**
+ * The prefix of a manifest that may be read: at most
+ * {@link SKILL_MAX_RESOURCE_ENTRIES} entries, and at most
+ * {@link SKILL_MAX_TOTAL_BYTES} of declared content.
+ */
+function boundedManifest(
+  declared: readonly SkillResource[],
+): readonly SkillResource[] {
+  const kept: SkillResource[] = [];
+  let bytes = 0;
+  for (const resource of declared) {
+    if (kept.length >= SKILL_MAX_RESOURCE_ENTRIES) break;
+    // Only a usable, non-negative size counts — matching `totalSkillBytes`, so
+    // the bound and the finding that reports the overage agree on the total.
+    const size =
+      typeof resource.size === "number" &&
+      Number.isSafeInteger(resource.size) &&
+      resource.size >= 0
+        ? resource.size
+        : 0;
+    if (bytes + size > SKILL_MAX_TOTAL_BYTES) break;
+    bytes += size;
+    kept.push(resource);
+  }
+  return kept;
+}
+
 /** Result shape of one `resources/read`, narrowed to what a digest needs. */
 interface ReadContents {
   text?: string;
@@ -196,14 +229,24 @@ export async function verifySkills(
 
     const declared =
       entry.resources === DYNAMIC_RESOURCES ? [] : entry.resources;
-    // ⚠️ **Bounded, because the manifest is server-controlled.** The 512-entry
-    // limit is CHECKED by `checkSkillConformance` — as a warning, since the SEP
-    // makes it an interoperability bound rather than a MUST — but checking it
-    // constrains nothing, so a hostile or broken server advertising a million
-    // entries had the tool perform a million sequential reads after the report
-    // already knew the manifest was over the limit (Copilot). The overage is
-    // reported by `resource-limit-exceeded`; reading it is what stops here.
-    const manifest = declared.slice(0, SKILL_MAX_RESOURCE_ENTRIES);
+    // ⚠️ **Bounded on BOTH interoperability limits, because the manifest is
+    // server-controlled.** `checkSkillConformance` reports when either is
+    // exceeded — as warnings, since the SEP makes them bounds rather than MUSTs
+    // — but checking constrains nothing, and this loop then downloads whatever
+    // was advertised anyway.
+    //
+    // The entry count alone is not enough: a manifest can sit at exactly 512
+    // entries and declare a gigabyte each, so bounding only the count still let
+    // a server dictate unbounded bandwidth and time (Copilot). Both overages
+    // are reported by `resource-limit-exceeded` / `size-limit-exceeded`; what
+    // stops here is *reading* them.
+    //
+    // Sizes are summed the way `totalSkillBytes` sums them — an entry
+    // declaring none contributes nothing, which cannot overstate the total and
+    // is bounded by the count cap regardless. A read is skipped only when the
+    // running total would CROSS the limit, so a conforming skill (≤ 16 MiB in
+    // total, by definition) is never truncated.
+    const manifest = boundedManifest(declared);
     const entryIdentity = skillUriIdentity(entry.uri);
     // Compared by NORMALIZED identity, like every other URI comparison here —
     // `checkSkillConformance` already accepts a manifest self-entry written in

vitest.shared.mts

@@ -65,9 +65,15 @@ export function vitestSharedPaths(clientDir: string) {
     // is a statement about the installed tree, not about the manifests — check
     // the tree before adding or removing an entry.
     //
-    // `express` and `yaml` are reached only through `test-servers/src` —
-    // express by the http/oauth servers, yaml by `load-config.ts` — which is
-    // root-owned code with no manifest of its own.
+    // `express` is reached only through `test-servers/src` (the http/oauth
+    // servers), which is root-owned code with no manifest of its own.
+    //
+    // `yaml` was too — `load-config.ts` — but is now also a `core/` runtime
+    // import: `core/mcp/skillFile.ts` parses a served SKILL.md's frontmatter
+    // for the SEP-2640 cross-check (#2248). That matters to anyone revisiting
+    // this pin: it is no longer removable by retiring a test-server path, and
+    // as a dependency `core/` imports it is additionally named in all three
+    // bundler `external` lists.
     //
     // Pointing these at `<client>/node_modules` is what broke when the MCP
     // packages moved to the root (#1970): express was never declared by a client