diff --git a/clients/web/src/components/groups/ServerSettingsForm/ServerSettingsForm.stories.tsx b/clients/web/src/components/groups/ServerSettingsForm/ServerSettingsForm.stories.tsx index c6e6fb69..245feb66 100644 --- a/clients/web/src/components/groups/ServerSettingsForm/ServerSettingsForm.stories.tsx +++ b/clients/web/src/components/groups/ServerSettingsForm/ServerSettingsForm.stories.tsx @@ -130,6 +130,12 @@ function InteractiveRender(args: ServerSettingsFormProps) { settings: { ...args.settings, maxFetchRequests: value }, }); }} + onSkillCatalogLimitChange={(field, value) => { + args.onSkillCatalogLimitChange(field, value); + updateArgs({ + settings: { ...args.settings, [field]: value }, + }); + }} onProtocolEraChange={(value) => { args.onProtocolEraChange(value); updateArgs({ @@ -178,6 +184,7 @@ const meta: Meta = { onPaginatedListsChange: fn(), onAdvertisedExtensionChange: fn(), onMaxFetchRequestsChange: fn(), + onSkillCatalogLimitChange: fn(), onProtocolEraChange: fn(), onModernLogLevelChange: fn(), onOAuthChange: fn(), diff --git a/clients/web/src/components/groups/ServerSettingsForm/ServerSettingsForm.tsx b/clients/web/src/components/groups/ServerSettingsForm/ServerSettingsForm.tsx index c9b802c2..2ad4c5ae 100644 --- a/clients/web/src/components/groups/ServerSettingsForm/ServerSettingsForm.tsx +++ b/clients/web/src/components/groups/ServerSettingsForm/ServerSettingsForm.tsx @@ -871,7 +871,7 @@ export function ServerSettingsForm({ { expect(reports[2].incomplete).toMatch(/budget of 2 skills \/ 67108864/); }); + it("honors a configured catalog BYTE budget (#2294)", async () => { + // Each skill's SKILL.md is ~50 bytes, so a 60-byte budget is spent by the + // second skill: the third is where reading stops. + const enc = new TextEncoder(); + const mdFor = (i: number) => + `---\nname: b${i}\ndescription: A demo\n---\n\n# b${i}\n`; + const skills = await Promise.all( + Array.from({ length: 4 }, async (_, i): Promise => { + const bytes = enc.encode(mdFor(i)); + return entry({ + uri: `skill://b${i}/SKILL.md`, + frontmatter: { name: `b${i}`, description: "A demo" }, + resources: [ + { + uri: `skill://b${i}/SKILL.md`, + digest: await sha256Digest(bytes), + size: bytes.byteLength, + }, + ], + }); + }), + ); + const readResource = vi.fn(async (uri: string) => ({ + result: { + contents: [ + { uri, text: mdFor(Number(/b(\d+)/.exec(uri)?.[1] ?? "0")) }, + ], + }, + })); + const client = { + readResource, + getServerSettings: () => ({ skillCatalogMaxBytes: 60 }), + } as unknown as InspectorClientProtocol; + const reports = await verifySkills(client, skills); + expect(readResource.mock.calls.length).toBe(2); + expect(reports.map((r) => r.outcome)).toEqual([ + "verified", + "verified", + "incomplete", + "incomplete", + ]); + expect(reports[2].incomplete).toMatch(/256 skills \/ 60 bytes/); + }); + + it("charges a dynamic skill's SKILL.md read to the byte budget", async () => { + // A `"dynamic"` skill has no manifest, so its only read is the fallback + // SKILL.md fetch. Uncharged, `catalogBytes` stayed 0 and a byte budget of + // 1 read every dynamic skill in the catalog (Copilot). + const mdFor = (i: number) => + `---\nname: d${i}\ndescription: A demo\n---\n\n# d${i}\n`; + const skills: SkillEntry[] = Array.from({ length: 3 }, (_, i) => ({ + uri: `skill://d${i}/SKILL.md`, + frontmatter: { name: `d${i}`, description: "A demo" }, + resources: DYNAMIC_RESOURCES, + })); + const readResource = vi.fn(async (uri: string) => ({ + result: { + contents: [ + { uri, text: mdFor(Number(/d(\d+)/.exec(uri)?.[1] ?? "0")) }, + ], + }, + })); + const client = { + readResource, + getServerSettings: () => ({ skillCatalogMaxBytes: 1 }), + } as unknown as InspectorClientProtocol; + const reports = await verifySkills(client, skills); + expect(readResource.mock.calls.length).toBe(1); + expect(reports[0].outcome).not.toBe("incomplete"); + expect(reports[1].outcome).toBe("incomplete"); + expect(reports[2].outcome).toBe("incomplete"); + }); + it("falls back to the default budget for an unusable configured limit", async () => { const { skill, client } = await truncatable({ name: "fb", count: 2 }); const withSettings = { diff --git a/clients/web/src/test/integration/mcp/remote/server-extra-coverage.test.ts b/clients/web/src/test/integration/mcp/remote/server-extra-coverage.test.ts index b30a3fbc..4ebd734b 100644 --- a/clients/web/src/test/integration/mcp/remote/server-extra-coverage.test.ts +++ b/clients/web/src/test/integration/mcp/remote/server-extra-coverage.test.ts @@ -832,7 +832,7 @@ describe("server.ts supplemental coverage", () => { expect((await res.json()).error).toMatch(/maxFetchRequests/); }); - it("rejects a non-positive skills catalog limit (#2294)", async () => { + it("rejects invalid skills catalog limits (#2294)", async () => { const skills = await postSettings({ ...base, skillCatalogMaxSkills: 0 }); expect((await skills.json()).error).toMatch(/skillCatalogMaxSkills/); const bytes = await postSettings({ ...base, skillCatalogMaxBytes: 1.5 }); diff --git a/core/mcp/skillsVerification.ts b/core/mcp/skillsVerification.ts index 91ca25a7..b3392230 100644 --- a/core/mcp/skillsVerification.ts +++ b/core/mcp/skillsVerification.ts @@ -504,6 +504,12 @@ export async function verifySkills( files.push({ uri: entry.uri, status: "read-error", reason }); try { const invocation = await client.readResource(entry.uri, metadata); + // ⚠️ Charged like a manifest read — raw response, or the decoded + // length when larger. Uncharged, a catalog of `"dynamic"` skills (whose + // ONLY read is this one) left `catalogBytes` at zero, so the run's byte + // budget bounded nothing for it and even a configured limit of 1 read + // every skill up to the count limit (Copilot, #2294). + let charged = responseBytes(invocation.result); const contents = contentsFor(invocation.result, entry.uri); if (!contents) { fail( @@ -512,10 +518,12 @@ export async function verifySkills( } else { try { entryBytes = skillFileBytes(contents); + charged = Math.max(charged, entryBytes.byteLength); } catch (err) { fail(reasonOf(err)); } } + receivedBytes += charged; // If the DECLARED manifest lists this file but the read bounds // excluded it, verify it here too. The fallback exists for the // frontmatter check, but reading a file and then not checking the diff --git a/core/mcp/types.ts b/core/mcp/types.ts index 651b85ee..4dcd79c6 100644 --- a/core/mcp/types.ts +++ b/core/mcp/types.ts @@ -172,14 +172,16 @@ export type StoredMCPServer = MCPServerConfig & { */ maxFetchRequests?: number; /** - * Most skills one skills verification run reads from (#2294). Omitted on - * disk when it equals `SKILL_MAX_CATALOG_SKILLS`. Must be a positive integer + * The maximum number of skills whose files are read in one skills + * verification run (#2294). Omitted on disk when it equals + * `SKILL_MAX_CATALOG_SKILLS`. Must be a positive integer * — there is no unlimited value, since the bound is what makes `--verify` * terminate. */ skillCatalogMaxSkills?: number; /** - * Byte ceiling across one skills verification run (#2294). Omitted on disk + * The maximum number of bytes read across all skills in one skills + * verification run (#2294). Omitted on disk * when it equals `SKILL_MAX_CATALOG_BYTES`. Positive integer, as above. */ skillCatalogMaxBytes?: number; diff --git a/docs/mcp-server-configuration.md b/docs/mcp-server-configuration.md index 89c91737..8165c250 100644 --- a/docs/mcp-server-configuration.md +++ b/docs/mcp-server-configuration.md @@ -188,8 +188,8 @@ These have no analog in the broader `mcp.json` ecosystem. Each is **omitted on w | `paginatedLists` | `false` | Fetch tools/resources/prompts one page at a time instead of auto-aggregating | | `advertisedExtensions` | — | Per-extension overrides for what the Inspector declares in `capabilities.extensions` | | `maxFetchRequests` | `1000` | Network-log retention for this server (`DEFAULT_MAX_FETCH_REQUESTS`); `0` means unlimited | -| `skillCatalogMaxSkills` | `256` | Most skills one skills verification run reads (`SKILL_MAX_CATALOG_SKILLS`) — the CLI's `--verify` and the TUI Skills pane. Positive integer; there is no unlimited value | -| `skillCatalogMaxBytes` | `67108864` | Most bytes one skills verification run reads (`SKILL_MAX_CATALOG_BYTES`, 64 MiB). Positive integer | +| `skillCatalogMaxSkills` | `256` | The maximum number of skills whose files are read in one verification run (`SKILL_MAX_CATALOG_SKILLS`) — the CLI's `--verify` and the TUI Skills pane. Positive integer; there is no unlimited value | +| `skillCatalogMaxBytes` | `67108864` | The maximum number of bytes read across all skills in one verification run (`SKILL_MAX_CATALOG_BYTES`, 64 MiB). Positive integer | | `oauth` | — | `{ clientId, clientSecret, scopes, requestRefreshToken, revokeOnClear, authorizationParams, authorizationUrl, tokenUrl, enterpriseManaged, onInsufficientScope }` | `metadata` is a JSON **object**, and its values may be any JSON — an object, an array, a number, a boolean, `null` — not only a string. The MCP spec does not restrict `_meta` value types in general, and the SDK models the field as a passthrough object, so the Inspector does not narrow it either ([#1910](https://github.com/modelcontextprotocol/inspector/issues/1910)). Edit it in Server Settings → Request Metadata, which is a JSON editor rather than the key/value rows `headers` and `env` use; text that is not a JSON object is flagged inline and not applied.