feat: make the skills verification catalog budget configurable per server (#2294)
Adds a Skills section to Server Settings with Maximum Number of Skills and Maximum Catalog Size fields, defaulting to SKILL_MAX_CATALOG_SKILLS (256) and SKILL_MAX_CATALOG_BYTES (64 MiB). The values persist as skillCatalogMaxSkills / skillCatalogMaxBytes on the server's mcp.json entry (omitted at the default), are validated as positive integers by the /api/servers routes, and are read by verifySkills through getServerSettings(), so the CLI's --verify and the TUI Skills pane both honor them. 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:
parent
3e728a3b8c
commit
eb41185c37
5 changed files with 469 additions and 6 deletions
|
|
@ -65,6 +65,7 @@ import {
|
|||
stripInspectorFields,
|
||||
} from "../../serverList.js";
|
||||
import { toRecord } from "../../../json/jsonUtils.js";
|
||||
import { isSkillCatalogLimit } from "../../skills.js";
|
||||
import { resolveImportSource } from "../../import/resolveSource.js";
|
||||
import { RemoteSession } from "./remote-session.js";
|
||||
import { createRemoteAuthProvider } from "./tokenAuthProvider.js";
|
||||
|
|
@ -1681,6 +1682,19 @@ export function createRemoteApp(
|
|||
);
|
||||
delete valObj.maxFetchRequests;
|
||||
}
|
||||
// #2294 — positive integers only; `0` is not "unlimited" for these.
|
||||
for (const key of [
|
||||
"skillCatalogMaxSkills",
|
||||
"skillCatalogMaxBytes",
|
||||
] as const) {
|
||||
if (key in valObj && !isSkillCatalogLimit(valObj[key])) {
|
||||
logWarn(
|
||||
{ route: "/api/servers", id, droppedKey: key },
|
||||
`Dropping malformed \`${key}\` field — expected a positive integer.`,
|
||||
);
|
||||
delete valObj[key];
|
||||
}
|
||||
}
|
||||
if ("oauth" in valObj && !isOauthObject(valObj.oauth)) {
|
||||
logWarn(
|
||||
{ route: "/api/servers", id, droppedKey: "oauth" },
|
||||
|
|
@ -1953,6 +1967,18 @@ export function createRemoteApp(
|
|||
error: "settings.maxFetchRequests must be a non-negative number",
|
||||
};
|
||||
}
|
||||
// #2294 — optional on the wire; absent means the default budget.
|
||||
for (const key of [
|
||||
"skillCatalogMaxSkills",
|
||||
"skillCatalogMaxBytes",
|
||||
] as const) {
|
||||
if (obj[key] !== undefined && !isSkillCatalogLimit(obj[key])) {
|
||||
return {
|
||||
ok: false,
|
||||
error: `settings.${key} must be a positive integer`,
|
||||
};
|
||||
}
|
||||
}
|
||||
for (const optional of [
|
||||
"oauthClientId",
|
||||
"oauthClientSecret",
|
||||
|
|
@ -2099,6 +2125,13 @@ export function createRemoteApp(
|
|||
// writes no spurious `roots` field.
|
||||
roots: isRootArray(obj.roots) ? obj.roots : [],
|
||||
};
|
||||
// Validated above; absent stays absent (the default budget).
|
||||
if (isSkillCatalogLimit(obj.skillCatalogMaxSkills)) {
|
||||
value.skillCatalogMaxSkills = obj.skillCatalogMaxSkills;
|
||||
}
|
||||
if (isSkillCatalogLimit(obj.skillCatalogMaxBytes)) {
|
||||
value.skillCatalogMaxBytes = obj.skillCatalogMaxBytes;
|
||||
}
|
||||
if (typeof obj.oauthClientId === "string" && obj.oauthClientId !== "") {
|
||||
value.oauthClientId = obj.oauthClientId;
|
||||
}
|
||||
|
|
|
|||
|
|
@ -13,6 +13,11 @@ import {
|
|||
DEFAULT_TASK_TTL_MS,
|
||||
isModernLogLevel,
|
||||
} from "./types.js";
|
||||
import {
|
||||
isSkillCatalogLimit,
|
||||
SKILL_MAX_CATALOG_BYTES,
|
||||
SKILL_MAX_CATALOG_SKILLS,
|
||||
} from "./skills.js";
|
||||
import type { Root } from "@modelcontextprotocol/client";
|
||||
import type {
|
||||
InspectorServerSettings,
|
||||
|
|
@ -160,6 +165,8 @@ type StoredInspectorFields = Pick<
|
|||
| "paginatedLists"
|
||||
| "advertisedExtensions"
|
||||
| "maxFetchRequests"
|
||||
| "skillCatalogMaxSkills"
|
||||
| "skillCatalogMaxBytes"
|
||||
| "oauth"
|
||||
| "roots"
|
||||
>;
|
||||
|
|
@ -521,6 +528,8 @@ export function storedFieldsToInspectorSettings(
|
|||
stored.paginatedLists !== undefined ||
|
||||
stored.advertisedExtensions !== undefined ||
|
||||
stored.maxFetchRequests !== undefined ||
|
||||
stored.skillCatalogMaxSkills !== undefined ||
|
||||
stored.skillCatalogMaxBytes !== undefined ||
|
||||
stored.oauth !== undefined ||
|
||||
stored.roots !== undefined ||
|
||||
stored.protocolEra !== undefined ||
|
||||
|
|
@ -557,6 +566,16 @@ export function storedFieldsToInspectorSettings(
|
|||
// `[]`, which `inspectorSettingsToStoredFields` then omits on write.
|
||||
roots: stored.roots ?? [],
|
||||
};
|
||||
// Lifted only when usable (#2294). Unlike maxFetchRequests these stay
|
||||
// optional in memory — absent means the default, which
|
||||
// `resolveSkillCatalogBudget` supplies — and a hand-edited `0` or `-1` is
|
||||
// dropped here rather than carried into a form that would re-persist it.
|
||||
if (isSkillCatalogLimit(stored.skillCatalogMaxSkills)) {
|
||||
settings.skillCatalogMaxSkills = stored.skillCatalogMaxSkills;
|
||||
}
|
||||
if (isSkillCatalogLimit(stored.skillCatalogMaxBytes)) {
|
||||
settings.skillCatalogMaxBytes = stored.skillCatalogMaxBytes;
|
||||
}
|
||||
// Absent on disk reads back as the default era; the write side then omits the
|
||||
// default so a byte-stable round-trip never injects `protocolEra` into files
|
||||
// that never set it. An unknown literal from a hand-edited file is dropped
|
||||
|
|
@ -737,6 +756,22 @@ export function inspectorSettingsToStoredFields(
|
|||
out.maxFetchRequests = settings.maxFetchRequests;
|
||||
}
|
||||
|
||||
// Same omit-the-default rule (#2294). An unusable value writes nothing, so
|
||||
// it reads back as the default rather than being persisted and then
|
||||
// rejected on the next load.
|
||||
if (
|
||||
isSkillCatalogLimit(settings.skillCatalogMaxSkills) &&
|
||||
settings.skillCatalogMaxSkills !== SKILL_MAX_CATALOG_SKILLS
|
||||
) {
|
||||
out.skillCatalogMaxSkills = settings.skillCatalogMaxSkills;
|
||||
}
|
||||
if (
|
||||
isSkillCatalogLimit(settings.skillCatalogMaxBytes) &&
|
||||
settings.skillCatalogMaxBytes !== SKILL_MAX_CATALOG_BYTES
|
||||
) {
|
||||
out.skillCatalogMaxBytes = settings.skillCatalogMaxBytes;
|
||||
}
|
||||
|
||||
const oauthFields: NonNullable<StoredMCPServer["oauth"]> = {};
|
||||
if (settings.oauthClientId) oauthFields.clientId = settings.oauthClientId;
|
||||
if (settings.oauthClientSecret)
|
||||
|
|
@ -815,6 +850,8 @@ const INSPECTOR_FIELD_KEY_MAP = {
|
|||
paginatedLists: true,
|
||||
advertisedExtensions: true,
|
||||
maxFetchRequests: true,
|
||||
skillCatalogMaxSkills: true,
|
||||
skillCatalogMaxBytes: true,
|
||||
oauth: true,
|
||||
roots: true,
|
||||
} as const satisfies Record<keyof StoredInspectorFields, true>;
|
||||
|
|
|
|||
|
|
@ -76,6 +76,47 @@ 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;
|
||||
|
||||
/**
|
||||
* Whether `value` is usable as a per-server catalog budget override (#2294):
|
||||
* a positive safe integer.
|
||||
*
|
||||
* ⚠️ **`0` is deliberately not "unlimited"**, unlike `maxFetchRequests`. The
|
||||
* budget exists so a `--verify` against a huge catalog terminates; an
|
||||
* unlimited setting would reintroduce exactly the unbounded run it closed. A
|
||||
* user who wants more raises the number.
|
||||
*/
|
||||
export function isSkillCatalogLimit(value: unknown): value is number {
|
||||
return typeof value === "number" && Number.isSafeInteger(value) && value > 0;
|
||||
}
|
||||
|
||||
/** A verification run's catalog budget, resolved from per-server settings. */
|
||||
export interface SkillCatalogBudget {
|
||||
maxSkills: number;
|
||||
maxBytes: number;
|
||||
}
|
||||
|
||||
/**
|
||||
* The catalog budget for a run: the server's configured limits where they are
|
||||
* usable, else {@link SKILL_MAX_CATALOG_SKILLS} / {@link SKILL_MAX_CATALOG_BYTES}.
|
||||
*
|
||||
* A malformed value falls back to the default rather than throwing — settings
|
||||
* can arrive from a hand-edited `mcp.json`, and a verification that refused to
|
||||
* run over a typo would be a worse outcome than one that ran at the default.
|
||||
*/
|
||||
export function resolveSkillCatalogBudget(settings?: {
|
||||
skillCatalogMaxSkills?: number;
|
||||
skillCatalogMaxBytes?: number;
|
||||
}): SkillCatalogBudget {
|
||||
return {
|
||||
maxSkills: isSkillCatalogLimit(settings?.skillCatalogMaxSkills)
|
||||
? settings.skillCatalogMaxSkills
|
||||
: SKILL_MAX_CATALOG_SKILLS,
|
||||
maxBytes: isSkillCatalogLimit(settings?.skillCatalogMaxBytes)
|
||||
? settings.skillCatalogMaxBytes
|
||||
: SKILL_MAX_CATALOG_BYTES,
|
||||
};
|
||||
}
|
||||
|
||||
/** The suffix every skill URI ends with; the segment before it is the name. */
|
||||
export const SKILL_FILE_SUFFIX = "/SKILL.md";
|
||||
|
||||
|
|
|
|||
|
|
@ -32,9 +32,8 @@ import {
|
|||
checkSkillConformance,
|
||||
checkSkillFrontmatterMatch,
|
||||
checkSkillNameCollisions,
|
||||
resolveSkillCatalogBudget,
|
||||
skillDisplayName,
|
||||
SKILL_MAX_CATALOG_BYTES,
|
||||
SKILL_MAX_CATALOG_SKILLS,
|
||||
SKILL_MAX_RESOURCE_ENTRIES,
|
||||
SKILL_MAX_TOTAL_BYTES,
|
||||
skillFileBytes,
|
||||
|
|
@ -323,6 +322,13 @@ export async function verifySkills(
|
|||
// thousand skills, each individually conforming, made `--verify` run
|
||||
// indefinitely and transfer unboundedly (Copilot). See
|
||||
// {@link SKILL_MAX_CATALOG_SKILLS}.
|
||||
//
|
||||
// The limits are the server's configured ones when set (#2294). Read through
|
||||
// `getServerSettings()` rather than taken as a parameter, so every caller —
|
||||
// the CLI's `--verify` and the TUI pane — honors the setting without having
|
||||
// to remember to pass it. Optional-chained because a test double need not
|
||||
// implement the accessor.
|
||||
const budget = resolveSkillCatalogBudget(client.getServerSettings?.());
|
||||
let walkedSkills = 0;
|
||||
let catalogBytes = 0;
|
||||
for (const entry of entries) {
|
||||
|
|
@ -330,8 +336,7 @@ export async function verifySkills(
|
|||
// 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;
|
||||
walkedSkills < budget.maxSkills && catalogBytes <= budget.maxBytes;
|
||||
// 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
|
||||
|
|
@ -368,7 +373,7 @@ export async function verifySkills(
|
|||
// total, by definition) is never truncated.
|
||||
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.`
|
||||
? `Not read: this run already reached its catalog budget of ${budget.maxSkills} skills / ${budget.maxBytes} bytes (raise it in the server's Skills settings). 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;
|
||||
|
|
|
|||
|
|
@ -171,6 +171,18 @@ export type StoredMCPServer = MCPServerConfig & {
|
|||
* file diff minimal for servers that never tuned it. `0` means unlimited.
|
||||
*/
|
||||
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
|
||||
* — 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
|
||||
* when it equals `SKILL_MAX_CATALOG_BYTES`. Positive integer, as above.
|
||||
*/
|
||||
skillCatalogMaxBytes?: number;
|
||||
/**
|
||||
* Pre-configured OAuth client credentials for HTTP transports. Nested to
|
||||
* match Claude Code's `.mcp.json` shape; lifted into the flat `oauthClientId`
|
||||
|
|
@ -900,6 +912,17 @@ export interface InspectorServerSettings {
|
|||
* render; defaults to `DEFAULT_MAX_FETCH_REQUESTS`. `0` means unlimited.
|
||||
*/
|
||||
maxFetchRequests: number;
|
||||
/**
|
||||
* Catalog budget for skills verification (#2294): the most skills, and the
|
||||
* most bytes, one run reads. Optional rather than concrete like
|
||||
* `maxFetchRequests` — absent means `SKILL_MAX_CATALOG_SKILLS` /
|
||||
* `SKILL_MAX_CATALOG_BYTES`, resolved by `resolveSkillCatalogBudget`, and
|
||||
* the form renders those defaults itself. Read by `verifySkills` through
|
||||
* `getServerSettings()`, so the CLI's `--verify` and the TUI's Skills pane
|
||||
* both honor it.
|
||||
*/
|
||||
skillCatalogMaxSkills?: number;
|
||||
skillCatalogMaxBytes?: number;
|
||||
/**
|
||||
* Roots advertised to the server via the `roots` client capability. Each
|
||||
* root carries a required `uri` and an optional `name` (SDK `Root`). The
|
||||
|
|
|
|||
Loading…
Reference in a new issue