fix: address Copilot review round 19 on #2248

Four findings, all real.

- No catalog-level bound. The per-skill caps bound what ONE entry can
  cost; nothing bounded how many entries there are, and SEP-2640 puts no
  ceiling on a catalog — every entry costs at least one resources/read,
  so a large listing made `--verify` run indefinitely and transfer
  unboundedly. Adds SKILL_MAX_CATALOG_SKILLS / SKILL_MAX_CATALOG_BYTES.
  Entries past the budget are still reported, with their static findings
  and an `incomplete` reason, rather than dropped or failed: they were
  not checked, which is neither a pass nor a verdict against the server.

- `responseBytes` charged `text.length`, which counts UTF-16 code units.
  That undercharges non-ASCII by up to 3x, so a decoy block of emoji kept
  the counter under 16 MiB while the wire carried far more. Adds
  `utf8Length`, an allocation-free UTF-8 byte count — deliberately not
  TextEncoder, which would copy a payload the server chose the size of.

- `checkSkillNameCollisions` was O(N^2) in both work and output for a
  group of N: every entry filtered all N URIs and embedded the other
  N-1. Duplicate names are legal and a server controls N. Now names a
  bounded sample and counts the rest.

- `--verify` help documented exit 7 but not exit 8.

The three-tier bounding (per skill, per skill on the wire, per run) is
now a table in clients/cli/README.md, since the run bound is this tool's
limit rather than the spec's and should not read as a conformance rule.

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>
This commit is contained in:
cliffhall 2026-09-07 23:55:53 -04:00
commit 62fd7d1bd4
6 changed files with 264 additions and 15 deletions

View file

@ -381,6 +381,21 @@ wrong" and "this skill could not be fully checked" are different answers:
| `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. |
**The run is bounded, and says when a bound bit.** Three limits, all reported as
`incomplete` (`8`) rather than as a pass or a failure, because an entry that was
not read has not been cleared of anything:
| Bound | Limit | Why |
| --- | --- | --- |
| Per skill | 512 manifest entries / 16 MiB | SEP-2640's own interoperability limits. |
| Per skill, on the wire | 16 MiB actually served | The declared sizes are server-controlled; this one cannot be lied past. |
| Per run | 256 skills / 64 MiB | SEP-2640 bounds a skill and deliberately does not bound a *catalog*. Every entry costs at least one `resources/read`, so without this a large listing — hostile or merely big — is unbounded work against the tool inspecting it. |
The run bound is this tool's, not the spec's. A skill past it is still reported,
with its static conformance findings and an `incomplete` reason saying nothing
about its files was checked; verify it on its own with `--method skills/get
--uri <skill>` to get a verdict for it.
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

View file

@ -752,7 +752,7 @@ async function parseArgs(argv?: string[]): Promise<ParseResult> {
)
.option(
"--verify",
"Run the SEP-2640 conformance and digest checks over the skills returned, emit one JSON report per skill on stdout, and exit 7 if any fails. Use with --method skills/list or --method skills/get.",
"Run the SEP-2640 conformance and digest checks over the skills returned, emit one JSON report per skill on stdout, and exit 7 if any fails or 8 if any could not be fully checked within the read bounds. Use with --method skills/list or --method skills/get.",
)
.option(
"--connect-timeout <ms>",

View file

@ -1154,6 +1154,27 @@ describe("checkSkillNameCollisions (#2248)", () => {
expect(first?.message).toContain("skill://c/r/SKILL.md");
});
it("bounds a large collision group instead of transcribing it", () => {
// Duplicate names are legal and SEP-2640 puts no ceiling on a catalog, so
// naming every other member made both the work and the generated text
// O(N²) — a server controls N, which turns a legal listing into a denial
// of service against the tool sent to inspect it (Copilot).
const N = 500;
const collisions = checkSkillNameCollisions(
Array.from({ length: N }, (_, i) => at(`skill://s${i}/r/SKILL.md`, "r")),
);
expect(collisions.size).toBe(N);
const message = collisions.get("skill://s0/r/SKILL.md")?.message ?? "";
// Three named, the rest counted — enough to see what the collision IS and
// where to look, without a transcript of the catalog.
expect(message).toMatch(/and 496 more/);
expect(message).toContain("499 other skills in this listing also declare");
// The bound is on the message, so its length cannot grow with the catalog.
expect(message.length).toBeLessThan(400);
// Still never names itself.
expect(message).not.toContain("skill://s0/r/SKILL.md");
});
it("does not report the SAME skill listed twice as a collision", () => {
// A repeated entry is a different defect from two skills sharing a name,
// and calling it this one would be a wrong diagnosis rather than a missing

View file

@ -1,11 +1,15 @@
import { describe, it, expect, vi } from "vitest";
import type { InspectorClientProtocol } from "@inspector/core/mcp/inspectorClientProtocol.js";
import type { SkillEntry } from "@inspector/core/mcp/skillsSchemas.js";
import { sha256Digest } from "@inspector/core/mcp/skills.js";
import {
SKILL_MAX_CATALOG_SKILLS,
sha256Digest,
} from "@inspector/core/mcp/skills.js";
import { AuthRecoveryRequiredError } from "@inspector/core/auth/challenge.js";
import {
allSkillsVerified,
anySkillFailed,
utf8Length,
verifySkills,
} from "@inspector/core/mcp/skillsVerification.js";
@ -691,6 +695,101 @@ describe("verifySkills (#2248)", () => {
expect(report.incomplete).toMatch(/actually served/);
});
it("charges non-ASCII text in UTF-8 bytes, not UTF-16 units", async () => {
// `text.length` undercharged every non-ASCII payload by up to 3×, so a
// decoy block of emoji kept the counter under 16 MiB while the wire
// carried twice that, and the walk read on (Copilot). Each block below is
// 3 MiB of UTF-16 units and 12 MiB of UTF-8 bytes, so two cross the limit
// under correct accounting and six would be needed under the old one.
const emoji = "🙂".repeat(1.5 * 1024 * 1024); // 2 units each, 4 bytes each
const { skill } = await truncatable({ name: "emoji", count: 20 });
const readResource = vi.fn(async () => ({
result: {
contents: [{ uri: "skill://elsewhere/decoy.md", text: emoji }],
},
}));
const client = { readResource } as unknown as InspectorClientProtocol;
const [report] = await verifySkills(client, [skill]);
expect(readResource.mock.calls.length).toBe(3);
expect(report.incomplete).toMatch(/actually served/);
});
it("counts UTF-8 length exactly as TextEncoder does", async () => {
// The counter is hand-rolled to avoid allocating a copy of a payload a
// hostile server sized, so it is pinned against the reference encoder —
// including the surrogate cases that are the only reason it is not a
// one-liner.
const cases = [
"",
"plain ascii",
"café", // 2-byte
"日本語", // 3-byte
"🙂👍", // surrogate pairs, 4-byte
"a🙂b",
"\ud83d", // lone HIGH surrogate — U+FFFD, 3 bytes
"\udc4d", // lone LOW surrogate
"\ud83d\ud83d", // two highs in a row: neither pairs
"end\ud83d", // unpaired high at the very end
];
const encoder = new TextEncoder();
for (const value of cases) {
expect(utf8Length(value)).toBe(encoder.encode(value).byteLength);
}
});
it("stops reading once the run's catalog budget is spent", async () => {
// The per-skill caps bound what ONE entry costs; nothing bounded how many
// entries there are, and SEP-2640 puts no ceiling on a catalog — so a
// listing of a hundred thousand conforming skills made `--verify` run
// indefinitely (Copilot).
// Each skill carries its OWN SKILL.md, whose frontmatter matches its
// listing entry — otherwise every report is `failed` on a frontmatter
// mismatch and the budget is not what the test is measuring.
const enc = new TextEncoder();
const mdFor = (i: number) =>
`---\nname: s${i}\ndescription: A demo\n---\n\n# s${i}\n`;
const many = await Promise.all(
Array.from(
{ length: SKILL_MAX_CATALOG_SKILLS + 5 },
async (_, i): Promise<SkillEntry> => {
const bytes = enc.encode(mdFor(i));
return entry({
uri: `skill://s${i}/SKILL.md`,
frontmatter: { name: `s${i}`, description: "A demo" },
resources: [
{
uri: `skill://s${i}/SKILL.md`,
digest: await sha256Digest(bytes),
size: bytes.byteLength,
},
],
});
},
),
);
const readResource = vi.fn(async (uri: string) => ({
result: {
contents: [
{ uri, text: mdFor(Number(/s(\d+)/.exec(uri)?.[1] ?? "0")) },
],
},
}));
const client = { readResource } as unknown as InspectorClientProtocol;
const reports = await verifySkills(client, many);
// Every entry is still REPORTED — the static checks cost no I/O, so a
// skill past the budget is not silently dropped from the output.
expect(reports).toHaveLength(SKILL_MAX_CATALOG_SKILLS + 5);
expect(readResource.mock.calls.length).toBe(SKILL_MAX_CATALOG_SKILLS);
// …and the remainder says so, rather than passing or failing.
const past = reports.slice(SKILL_MAX_CATALOG_SKILLS);
for (const report of past) {
expect(report.outcome).toBe("incomplete");
expect(report.incomplete).toMatch(/catalog budget/);
expect(report.files).toHaveLength(0);
}
expect(reports[0].outcome).toBe("verified");
});
it("is not incomplete when the budget is crossed by the LAST entry", async () => {
// Crossing the line on the final row stopped nothing: every manifest entry
// was fetched and checked. Reporting "Stopped after 4 of 4" there both

View file

@ -54,6 +54,28 @@ export const SKILL_MAX_RESOURCE_ENTRIES = 512;
/** Maximum total size, in bytes, of a single skill's resources (16 MiB). */
export const SKILL_MAX_TOTAL_BYTES = 16 * 1024 * 1024;
/**
* Maximum skills one verification run will actually read from, and the byte
* ceiling across all of them.
*
* ⚠️ **Not SEP-2640 limits — they are this tool's own.** The SEP bounds a
* single skill and deliberately does not bound a catalog: `skills/list` may be
* arbitrarily long, and a page may hold arbitrarily many entries, so the
* cursor-walk's page cap constrains nothing here. Every entry costs at least
* one `resources/read`, so an unbounded catalog is unbounded work and
* unbounded transfer against the tool sent to inspect it — a `--verify` in CI
* that never returns (Copilot).
*
* Entries past either bound are reported as `incomplete` rather than dropped
* or failed: they were not checked, which is neither a pass nor a verdict
* against the server. A host wanting more is not wrong — these are safety
* limits, not conformance ones — which is why the reason names them.
*/
export const SKILL_MAX_CATALOG_SKILLS = 256;
/** @see {@link SKILL_MAX_CATALOG_SKILLS} — 64 MiB across the whole run. */
export const SKILL_MAX_CATALOG_BYTES = 64 * 1024 * 1024;
/** The suffix every skill URI ends with; the segment before it is the name. */
export const SKILL_FILE_SUFFIX = "/SKILL.md";
@ -971,6 +993,13 @@ export function checkSkillFrontmatterMatch(
return issues;
}
/**
* How many colliding URIs a `duplicate-name` message names before it counts the
* rest. Three is enough to show the shape of the collision; the count carries
* the scale.
*/
const COLLISION_SAMPLE = 3;
/**
* Findings that can only be computed over the **whole listing**, keyed by the
* entry they belong to (its normalized URI identity).
@ -1025,11 +1054,35 @@ export function checkSkillNameCollisions(
const uris = [...identities].sort();
for (const entry of group) {
const self = skillUriIdentity(entry.uri);
const others = uris.filter((uri) => uri !== self);
// ⚠️ A bounded SAMPLE, taken with an early exit — not
// `uris.filter(...)` and not the whole list in the message. Duplicate
// names are legal and SEP-2640 puts no ceiling on a catalog, so a group
// of N made both the work and the generated text O(N²): every one of N
// entries scanned all N URIs and embedded the other N−1 (Copilot). A
// server controls N, which turns a legal listing into a denial of
// service against the tool meant to inspect it.
const sample: string[] = [];
for (const uri of uris) {
if (uri === self) continue;
sample.push(uri);
if (sample.length === COLLISION_SAMPLE) break;
}
const unshown = identities.size - 1 - sample.length;
// Naming a few and counting the rest keeps the finding actionable — a
// reader needs to see that it IS a collision and where to look, not a
// transcript of the catalog.
const others =
unshown > 0
? `${sample.join(", ")}, and ${unshown} more`
: sample.join(", ");
const subject =
identities.size === 2
? "Another skill in this listing also declares"
: `${identities.size - 1} other skills in this listing also declare`;
issues.set(self, {
code: "duplicate-name",
severity: "warning",
message: `Another skill in this listing also declares the name "${name}" (${others.join(", ")}). This is legal — a consumer must tell them apart by their URIs rather than collapsing or preferring one.`,
message: `${subject} the name "${name}" (${others}). This is legal — a consumer must tell them apart by their URIs rather than collapsing or preferring one.`,
});
}
}

View file

@ -33,6 +33,8 @@ import {
checkSkillFrontmatterMatch,
checkSkillNameCollisions,
skillDisplayName,
SKILL_MAX_CATALOG_BYTES,
SKILL_MAX_CATALOG_SKILLS,
SKILL_MAX_RESOURCE_ENTRIES,
SKILL_MAX_TOTAL_BYTES,
skillFileBytes,
@ -219,6 +221,42 @@ function contentsFor(result: unknown, uri: string): ReadContents | undefined {
return undefined;
}
/**
* How many bytes a string occupies as UTF-8, without encoding a copy of it.
*
* `TextEncoder` would be the obvious answer and allocates a second buffer for
* a payload that may already be megabytes — and this is called on responses a
* hostile server chose the size of, which is the case the count exists to
* bound. Counting is O(n) and allocates nothing.
*
* A high surrogate is only worth 4 bytes when a low surrogate actually follows
* it. An unpaired one encodes as U+FFFD, which is 3 — the same as any other
* BMP character in that range, so it needs no special case beyond not
* consuming the next unit.
*/
export function utf8Length(value: string): number {
let total = 0;
for (let i = 0; i < value.length; i += 1) {
const code = value.charCodeAt(i);
if (code < 0x80) {
total += 1;
} else if (code < 0x800) {
total += 2;
} else if (code >= 0xd800 && code <= 0xdbff) {
const next = i + 1 < value.length ? value.charCodeAt(i + 1) : 0;
if (next >= 0xdc00 && next <= 0xdfff) {
total += 4;
i += 1;
} else {
total += 3;
}
} else {
total += 3;
}
}
return total;
}
/**
* What a `resources/read` response cost to receive, charged against the byte
* budget regardless of whether any of it is usable.
@ -230,12 +268,13 @@ function contentsFor(result: unknown, uri: string): ReadContents | undefined {
* or send an enormous blob that is not valid base64. Both leave the decode
* paths empty-handed while the bytes have already crossed the wire.
*
* The figure is an **approximation, and deliberately never an undercount by
* more than a small factor**: `text` is charged in UTF-16 code units (UTF-8 is
* between 1× and 3× that for the same string) and `blob` in base64 characters
* (roughly 4/3 of the bytes it decodes to, so an overcharge). Exactness is not
* the point — this is a safety limit, not an accounting figure, and the caller
* substitutes the exact decoded length whenever it has one.
* `text` is charged in **UTF-8 bytes**, the unit the limit is written in.
* Charging `text.length` instead — UTF-16 code units — undercharged every
* non-ASCII payload by up to 3×, so a decoy block of emoji or CJK kept the
* counter under 16 MiB while the wire carried far more, and the walk read on
* (Copilot). `blob` is charged in base64 characters, which is ~4/3 of what it
* decodes to: an OVERcharge, and deliberately left as one, since a blob that
* fails to decode has no byte count to be exact about.
*/
function responseBytes(result: unknown): number {
const contents = (result as { contents?: unknown })?.contents;
@ -244,7 +283,7 @@ function responseBytes(result: unknown): number {
for (const block of contents) {
if (typeof block !== "object" || block === null) continue;
const { text, blob } = block as { text?: unknown; blob?: unknown };
if (typeof text === "string") total += text.length;
if (typeof text === "string") total += utf8Length(text);
if (typeof blob === "string") total += blob.length;
}
return total;
@ -278,7 +317,21 @@ export async function verifySkills(
// reports no collision: there is no listing to collide within.
const collisions = checkSkillNameCollisions(entries);
const reports: SkillVerifyReport[] = [];
// ⚠️ Run-level budgets, on top of the per-skill ones below. The per-skill
// caps bound what ONE entry can cost; nothing bounded how many entries there
// are, and SEP-2640 puts no ceiling on a catalog — so a listing of a hundred
// thousand skills, each individually conforming, made `--verify` run
// indefinitely and transfer unboundedly (Copilot). See
// {@link SKILL_MAX_CATALOG_SKILLS}.
let walkedSkills = 0;
let catalogBytes = 0;
for (const entry of entries) {
// Static checks still run for every entry — they cost no I/O, so a skill
// past the budget is still reported on, just not read. What stops is the
// reading.
const withinBudget =
walkedSkills < SKILL_MAX_CATALOG_SKILLS &&
catalogBytes <= SKILL_MAX_CATALOG_BYTES;
// The entry's own SKILL.md, read once and used twice — for its digest and
// for the frontmatter cross-check. Reading it twice would double the load
// on the server and, worse, could compare a digest against one snapshot
@ -313,9 +366,10 @@ export async function verifySkills(
// 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);
let incomplete =
manifest.length < declared.length
const manifest = withinBudget ? boundedManifest(declared) : [];
let incomplete = !withinBudget
? `Not read: this run already reached its catalog budget of ${SKILL_MAX_CATALOG_SKILLS} skills / ${SKILL_MAX_CATALOG_BYTES} bytes. Nothing about this skill's files has been checked — verify it on its own with \`--method skills/get --uri\` to get a verdict.`
: 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
@ -436,7 +490,7 @@ export async function verifySkills(
// because a cap excluded it or because the byte budget broke the loop
// first, still gets the fallback: the frontmatter comparison is mandatory
// and must not be lost to a limit that exists to bound unrelated files.
if (!selfAttempted) {
if (withinBudget && !selfAttempted) {
// Recorded as a file result, not swallowed. Because a dynamic skill has
// no manifest rows, `files` would otherwise stay empty and its only static
// finding is a warning — so an unreadable SKILL.md returned `ok: true`
@ -485,6 +539,13 @@ export async function verifySkills(
}
}
// Charged after this entry's reads, so the skill that crosses the run
// budget is still fully reported rather than half-read. The NEXT one stops.
if (withinBudget) {
walkedSkills += 1;
catalogBytes += receivedBytes;
}
const entryText =
entryBytes === undefined ? undefined : bytesToText(entryBytes);