fix: address Copilot review round 9 on #2248

**Verification reads were unbounded.** 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 server advertising a million entries had the tool
perform a million sequential reads after the report already knew the manifest
was over. Capped at `SKILL_MAX_RESOURCE_ENTRIES`, with the finding still
reported so bounding the work does not silence the reason it was
unnecessary. `manifestListsSelf` is computed over the READ slice, so a
self-entry pushed past the cap still reaches the fallback: the frontmatter
comparison is mandatory and must not be lost to a limit that bounds other
files.

**A malformed skill URI still produced a directory root.** `skillUriIdentity`
falls back to the raw string, so `not a uri/SKILL.md` yielded the root `not a
uri` and enabled the Directory section — letting the UI build a request from
a URI the conformance checks had already rejected. The comment above it
claimed otherwise. `normalizeSkillUri` now, with the rule written down:
identity is for COMPARING two spellings, not for deciding a URI is
well-formed enough to build a request from.

The existing regression test passed against a weaker input that failed for
lacking the suffix and never reached the fallback; it is a table of three
shapes now.

**The test-server guide overstated its own coverage,** claiming four of the
five conformance scenarios map onto fixtures. Three do: there is no
size-mismatch fixture, because the fixture can override an advertised digest
and nothing else, so that path is covered by unit tests. Corrected, with what
adding one would take.

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/components/screens/SkillsScreen/SkillsScreen.test.tsx

@@ -2206,6 +2206,36 @@ describe("SkillsScreen directory browsing (#2248)", () => {
     );
   });
 
+  it.each([
+    ["no /SKILL.md suffix", "not-a-uri"],
+    // Ends with the suffix and so LOOKS addressable, but does not parse. The
+    // identity fallback returned the raw string here, producing the "root"
+    // `not a uri` and enabling a directory request built from a URI the
+    // conformance checks had already rejected (Copilot).
+    ["unparseable but suffixed", "not a uri/SKILL.md"],
+    ["relative, not a full URI", "demo/SKILL.md"],
+  ])(
+    "renders no Directory section for a malformed skill URI (%s)",
+    async (_label, uri) => {
+      const user = userEvent.setup();
+      const odd: SkillEntry = {
+        uri,
+        frontmatter: { name: "odd", description: "d" },
+        resources: [],
+      };
+      renderWithMantine(
+        <ControlledSkillsScreen
+          skills={[odd]}
+          onReadResourceDirectory={directoryReader({})}
+        />,
+      );
+      await user.click(screen.getByText("odd"));
+      expect(
+        screen.queryByRole("button", { name: /Directory/ }),
+      ).not.toBeInTheDocument();
+    },
+  );
+
   it("renders no Directory section for a skill whose URI is malformed", async () => {
     // There is no root to browse, and `malformed-uri` already reports it in
     // Conformance.

clients/web/src/components/screens/SkillsScreen/SkillsScreen.tsx

@@ -35,6 +35,7 @@ import {
   skillDisplayName,
   skillFileBytes,
   skillEntriesMatch,
+  normalizeSkillUri,
   skillUriIdentity,
   SKILL_FILE_SUFFIX,
   totalSkillBytes,
@@ -1129,8 +1130,15 @@ export function SkillsScreen({
    */
   const skillRoot = useMemo(() => {
     if (!selected) return undefined;
-    const normalized = skillUriIdentity(selected.uri);
-    return normalized.endsWith(SKILL_FILE_SUFFIX)
+    // `normalizeSkillUri`, NOT `skillUriIdentity`: the latter falls back to the
+    // raw string when parsing fails, so `not a uri/SKILL.md` yielded the "root"
+    // `not a uri` and enabled the Directory section — letting the UI send a
+    // directory request derived from a URI the conformance checks had already
+    // rejected as malformed (Copilot). Identity is the right tool for
+    // COMPARING two spellings; it is the wrong one for deciding that a URI is
+    // well-formed enough to build a request from.
+    const normalized = normalizeSkillUri(selected.uri);
+    return normalized !== undefined && normalized.endsWith(SKILL_FILE_SUFFIX)
       ? normalized.slice(0, -SKILL_FILE_SUFFIX.length)
       : undefined;
   }, [selected]);

clients/web/src/test/core/mcp/skillsVerification.test.ts

@@ -518,6 +518,71 @@ describe("verifySkills (#2248)", () => {
     expect(report.files[0].status).toBe("verified");
   });
 
+  it("bounds reads at the interoperability limit, not at the manifest length", async () => {
+    // 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 [report] = await verifySkills(client, [skill]);
+    expect(readResource).toHaveBeenCalledTimes(512);
+    expect(report.files).toHaveLength(512);
+    // …and the overage is still REPORTED, so bounding the reads does not
+    // silence the finding that made them unnecessary.
+    expect(report.conformance).toEqual(
+      expect.arrayContaining([
+        expect.objectContaining({ code: "resource-limit-exceeded" }),
+      ]),
+    );
+  });
+
+  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
+    // by a bloated manifest reaches the fallback read.
+    const skillMd = "---\nname: huge\ndescription: Served\n---\n\n# H\n";
+    const skill: SkillEntry = {
+      uri: "skill://huge/SKILL.md",
+      frontmatter: { name: "huge", description: "Listed" },
+      resources: [
+        ...Array.from({ length: 600 }, (_, i) => ({
+          uri: `skill://huge/f${i}.md`,
+          digest: `sha256:${"a".repeat(64)}`,
+          size: 1,
+        })),
+        // Beyond the 512 cap.
+        {
+          uri: "skill://huge/SKILL.md",
+          digest: `sha256:${"a".repeat(64)}`,
+          size: 1,
+        },
+      ],
+    };
+    const readResource = vi.fn(async (uri: string) => ({
+      result: {
+        contents: [{ uri, text: uri.endsWith("/SKILL.md") ? skillMd : "x" }],
+      },
+    }));
+    const client = { readResource } as unknown as InspectorClientProtocol;
+    const [report] = await verifySkills(client, [skill]);
+    expect(readResource).toHaveBeenCalledWith(
+      "skill://huge/SKILL.md",
+      undefined,
+    );
+    expect(report.frontmatter).toHaveLength(1);
+  });
+
   it("reports every skill it was given, in order", async () => {
     const a = await entry();
     const b = await entry({ uri: "skill://demo/SKILL.md" });

core/mcp/skillsVerification.ts

@@ -33,6 +33,7 @@ import {
   checkSkillFrontmatterMatch,
   checkSkillNameCollisions,
   skillDisplayName,
+  SKILL_MAX_RESOURCE_ENTRIES,
   skillFileBytes,
   skillUriIdentity,
   verifySkillResource,
@@ -68,7 +69,13 @@ export interface SkillVerifyReport {
    */
   frontmatter: SkillIssue[];
   /**
-   * One entry per manifest file, in manifest order.
+   * 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.
    *
    * ⚠️ **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
@@ -187,8 +194,16 @@ export async function verifySkills(
     let entryBytes: Uint8Array | undefined;
     const files: SkillFileReport[] = [];
 
-    const manifest =
+    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);
     const entryIdentity = skillUriIdentity(entry.uri);
     // Compared by NORMALIZED identity, like every other URI comparison here —
     // `checkSkillConformance` already accepts a manifest self-entry written in
@@ -244,7 +259,10 @@ export async function verifySkills(
     //
     // Gated on `manifestListsSelf` rather than on `entryBytes`, so a self-entry
     // the loop already tried and FAILED to read is not read a second time — its
-    // failure is recorded there.
+    // failure is recorded there. Note this is computed over the READ slice, so
+    // a self-entry pushed past the cap by a bloated manifest still reaches the
+    // fallback: the frontmatter comparison is mandatory and must not be lost to
+    // a limit that exists to bound unrelated files.
     if (!manifestListsSelf) {
       // Recorded as a file result, not swallowed. Because a dynamic skill has
       // no manifest rows, `files` would otherwise stay empty and its only static

docs/test-servers.md

@@ -145,14 +145,23 @@ They are the client-side obligations SEP-2640 makes testable from a hostile
 server, which is how the
 [`modelcontextprotocol/conformance`](https://github.com/modelcontextprotocol/conformance)
 harness grades a *client*: it stands up a server and watches what the client
-does. Four of its five skills scenarios map onto a fixture here — a digest
-mismatch (`tampered-notes`), a size mismatch, a frontmatter mismatch
-(`lying-listing`), and a read of a file the manifest does not list
-(`stale-manifest`). The fifth, **no-prefetch**, is a negative: it passes only if
-connecting and calling `skills/list` produces *no* `resources/read` at all. The
-Inspector satisfies it structurally — nothing is fetched until a user selects a
-skill or presses Verify, which is why every round trip on the Skills screen is a
-button rather than an effect.
+does. **Three** of its five skills scenarios map onto a fixture here — a digest
+mismatch (`tampered-notes`), a frontmatter mismatch (`lying-listing`), and a
+read of a file the manifest does not list (`stale-manifest`).
+
+The other two are covered, but not by this fixture, and the distinction is worth
+keeping honest:
+
+- **Size mismatch** has no fixture. `test-servers/src/skills.ts` can override an
+  advertised *digest* and nothing else, so the size path — which
+  `verifySkillResource` checks first, before hashing — is exercised by unit
+  tests rather than against a live server. Adding it would mean an
+  `advertisedSize` override beside the digest one.
+- **No-prefetch** is a negative and could not have a fixture: it passes only if
+  connecting and calling `skills/list` produces *no* `resources/read` at all.
+  The Inspector satisfies it structurally — nothing is fetched until a user
+  selects a skill or presses Verify, which is why every round trip on the Skills
+  screen is a button rather than an effect.
 
 ## Cancelling a call