fix: charge the fallback SKILL.md read to the catalog byte budget; review round 1 (#2294)
- A "dynamic" skill's only read is the fallback SKILL.md fetch, which was not charged, so a catalog of dynamic skills never advanced catalogBytes and a configured byte budget bounded nothing. Charge it like a manifest read. - Add verifySkills cases for a configured byte budget and for dynamic skills. - Wire onSkillCatalogLimitChange into the ServerSettingsForm story. - Reword the new field descriptions, JSDoc and config table rows; rename the route test to cover every invalid value it sends. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01P6i1HBSdSS9kL4npZ2dDSe Signed-off-by: cliffhall <cliff@futurescale.com>
This commit is contained in:
cliffhall
parent
eb41185c37
commit
467c97307e
7 changed files with 102 additions and 9 deletions
|
|
@ -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<typeof ServerSettingsForm> = {
|
|||
onPaginatedListsChange: fn(),
|
||||
onAdvertisedExtensionChange: fn(),
|
||||
onMaxFetchRequestsChange: fn(),
|
||||
onSkillCatalogLimitChange: fn(),
|
||||
onProtocolEraChange: fn(),
|
||||
onModernLogLevelChange: fn(),
|
||||
onOAuthChange: fn(),
|
||||
|
|
|
|||
|
|
@ -871,7 +871,7 @@ export function ServerSettingsForm({
|
|||
<Stack gap="xs">
|
||||
<SkillCountInput
|
||||
label="Maximum Number of Skills"
|
||||
description="Most skills one skills verification run (the CLI's --verify, or the TUI's Skills pane) reads files from. Skills past the limit are still listed, reported as incomplete rather than checked."
|
||||
description="The maximum number of skills whose files are read in one verification run (the CLI's --verify, or the TUI's Skills pane). Skills past the limit are still listed, and reported as incomplete rather than checked."
|
||||
value={skillCatalogBudget.maxSkills}
|
||||
onChange={handleSkillCatalogLimitChange(
|
||||
"skillCatalogMaxSkills",
|
||||
|
|
@ -880,7 +880,7 @@ export function ServerSettingsForm({
|
|||
/>
|
||||
<SkillBytesInput
|
||||
label="Maximum Catalog Size"
|
||||
description="Most bytes one skills verification run reads across every skill. Reaching it stops the read, and the remaining skills are reported as incomplete."
|
||||
description="The maximum number of bytes read across all skills in one verification run. Once it is reached, no further skills are read, and the remaining ones are reported as incomplete."
|
||||
value={skillCatalogBudget.maxBytes}
|
||||
onChange={handleSkillCatalogLimitChange(
|
||||
"skillCatalogMaxBytes",
|
||||
|
|
|
|||
|
|
@ -1,6 +1,9 @@
|
|||
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 {
|
||||
DYNAMIC_RESOURCES,
|
||||
type SkillEntry,
|
||||
} from "@inspector/core/mcp/skillsSchemas.js";
|
||||
import {
|
||||
SKILL_MAX_CATALOG_SKILLS,
|
||||
sha256Digest,
|
||||
|
|
@ -834,6 +837,79 @@ describe("verifySkills (#2248)", () => {
|
|||
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<SkillEntry> => {
|
||||
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 = {
|
||||
|
|
|
|||
|
|
@ -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 });
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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;
|
||||
|
|
|
|||
|
|
@ -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.
|
||||
|
|
|
|||
Loading…
Reference in a new issue