From 02b006031efc5f8d5cc62092099db67e96282092 Mon Sep 17 00:00:00 2001 From: diegosouzapw Date: Thu, 18 Jun 2026 17:49:02 -0300 Subject: [PATCH] fix(compression): bound mcpAccessibility maxTextChars on the live read path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit smartFilterText reserves 300 chars for the truncation tail, so a maxTextChars at/below that makes headSize <= 0 and the whole tool result is replaced by the notice (total data loss). Two gaps let a bad value reach the engine: the DB normalizer floored maxTextChars only at > 0, and the live MCP-server read path (readMcpAccessibilityConfig) did a raw {...DEFAULT, ...parsed} spread with no bounding at all. Centralize the floors in a shared clampMcpAccessibilityConfig (engine layer) used by both the DB normalizer and the server read path; values in (0, 600) fall back to the default. Export the tail-reserve constant (300) and use it in smartFilterText so the engine and the bounds stay in sync. The F5.3 headSize >= 0 clamp stays as a second layer. Defense-in-depth today (the config isn't writable via the settings API yet — making it configurable is a tracked follow-up in the compression-completeness work); this closes the data-loss path for when it lands. Part of the compression "100% functional" program (audit follow-up). --- open-sse/mcp-server/server.ts | 7 +-- .../engines/mcpAccessibility/constants.ts | 37 ++++++++++++++ .../engines/mcpAccessibility/index.ts | 17 ++++--- open-sse/services/compression/types.ts | 5 +- src/lib/db/compression.ts | 31 ++---------- .../mcpAccessibility-config-bounds.test.ts | 48 +++++++++++++++++++ 6 files changed, 109 insertions(+), 36 deletions(-) create mode 100644 tests/unit/compression/mcpAccessibility-config-bounds.test.ts diff --git a/open-sse/mcp-server/server.ts b/open-sse/mcp-server/server.ts index 0418d0c544..d10454167d 100644 --- a/open-sse/mcp-server/server.ts +++ b/open-sse/mcp-server/server.ts @@ -88,6 +88,7 @@ import { compressMcpRegistryMetadata } from "./descriptionCompressor.ts"; import { smartFilterText } from "../services/compression/engines/mcpAccessibility/index.ts"; import { DEFAULT_MCP_ACCESSIBILITY_CONFIG, + clampMcpAccessibilityConfig, type McpAccessibilityConfig, } from "../services/compression/engines/mcpAccessibility/constants.ts"; import { getDbInstance } from "../../src/lib/db/core.ts"; @@ -137,9 +138,9 @@ function readMcpAccessibilityConfig(): McpAccessibilityConfig { .prepare("SELECT value FROM key_value WHERE namespace = ? AND key = ?") .get("compression", "mcpAccessibility") as { value?: string } | undefined; if (!row?.value) return { ...DEFAULT_MCP_ACCESSIBILITY_CONFIG }; - const parsed = JSON.parse(row.value); - if (!parsed || typeof parsed !== "object") return { ...DEFAULT_MCP_ACCESSIBILITY_CONFIG }; - return { ...DEFAULT_MCP_ACCESSIBILITY_CONFIG, ...parsed }; + // clampMcpAccessibilityConfig bounds every field (and folds in the non-object guard), so a + // persisted out-of-range maxTextChars can't make smartFilterText truncate the whole text. + return clampMcpAccessibilityConfig(JSON.parse(row.value)); } catch { return { ...DEFAULT_MCP_ACCESSIBILITY_CONFIG }; } diff --git a/open-sse/services/compression/engines/mcpAccessibility/constants.ts b/open-sse/services/compression/engines/mcpAccessibility/constants.ts index 89f57bca6f..3ed49d26c1 100644 --- a/open-sse/services/compression/engines/mcpAccessibility/constants.ts +++ b/open-sse/services/compression/engines/mcpAccessibility/constants.ts @@ -24,3 +24,40 @@ export const DEFAULT_MCP_ACCESSIBILITY_CONFIG: McpAccessibilityConfig = { collapseKeepTail: MCP_ACCESSIBILITY_DEFAULTS.collapseKeepTail, minLengthToProcess: MCP_ACCESSIBILITY_DEFAULTS.minLengthToProcess, }; + +/** + * Chars `smartFilterText` reserves for the truncation tail/notice (`maxTextChars - this` is the + * head kept). A `maxTextChars` at or below this leaves no head, so the whole tool result would be + * replaced by the notice. The engine and the config bounds must agree on this number. + */ +export const MCP_ACCESSIBILITY_TAIL_RESERVE = 300; + +/** + * Minimum sane `maxTextChars`: below this the truncated head is too small to be useful (or empty). + * Values in `(0, MIN)` are treated as misconfiguration and fall back to the default. + */ +export const MCP_ACCESSIBILITY_MIN_MAX_TEXT_CHARS = MCP_ACCESSIBILITY_TAIL_RESERVE * 2; + +function boundedInt(value: unknown, min: number, fallback: number): number { + return typeof value === "number" && Number.isFinite(value) && value >= min + ? Math.floor(value) + : fallback; +} + +/** + * Bound a raw/persisted mcpAccessibility config into a safe, fully-populated config. Centralizes + * the numeric floors so both the DB normalizer and the live MCP-server read path agree (a small + * `maxTextChars` would otherwise make `smartFilterText` truncate the whole text away). + */ +export function clampMcpAccessibilityConfig(raw: unknown): McpAccessibilityConfig { + const record = (raw && typeof raw === "object" ? raw : {}) as Record; + const d = DEFAULT_MCP_ACCESSIBILITY_CONFIG; + return { + enabled: record["enabled"] !== false, + maxTextChars: boundedInt(record["maxTextChars"], MCP_ACCESSIBILITY_MIN_MAX_TEXT_CHARS, d.maxTextChars), + collapseThreshold: boundedInt(record["collapseThreshold"], 1, d.collapseThreshold), + collapseKeepHead: boundedInt(record["collapseKeepHead"], 0, d.collapseKeepHead), + collapseKeepTail: boundedInt(record["collapseKeepTail"], 0, d.collapseKeepTail), + minLengthToProcess: boundedInt(record["minLengthToProcess"], 1, d.minLengthToProcess), + }; +} diff --git a/open-sse/services/compression/engines/mcpAccessibility/index.ts b/open-sse/services/compression/engines/mcpAccessibility/index.ts index caf709d4ec..47ede74949 100644 --- a/open-sse/services/compression/engines/mcpAccessibility/index.ts +++ b/open-sse/services/compression/engines/mcpAccessibility/index.ts @@ -1,5 +1,5 @@ import { collapseRepeated } from "./collapseRepeated.ts"; -import type { McpAccessibilityConfig } from "./constants.ts"; +import { MCP_ACCESSIBILITY_TAIL_RESERVE, type McpAccessibilityConfig } from "./constants.ts"; const NOISE_PATTERNS: RegExp[] = [/^\s*-\s*generic:?\s*$/gm, /^\s*-\s*text:\s*""\s*$/gm]; @@ -19,10 +19,11 @@ export function smartFilterText(text: string, config: McpAccessibilityConfig): s ); if (out.length > config.maxTextChars) { - // Clamp to >=0: a maxTextChars below the 300-char tail reservation would make headSize - // negative, and slice(0, negative) counts from the END — silently keeping a wrong, - // oversized fragment instead of the intended head. - const headSize = Math.max(0, config.maxTextChars - 300); + // Clamp to >=0: a maxTextChars below the tail reservation would make headSize negative, and + // slice(0, negative) counts from the END — silently keeping a wrong, oversized fragment + // instead of the intended head. (clampMcpAccessibilityConfig keeps maxTextChars sane, but + // smartFilterText is also called with raw configs in tests, so the clamp stays here too.) + const headSize = Math.max(0, config.maxTextChars - MCP_ACCESSIBILITY_TAIL_RESERVE); const head = out.slice(0, headSize); // Measure omitted against the FILTERED text (out), not the raw input (text), which may // have shrunk via noise removal / collapse above. @@ -35,4 +36,8 @@ export function smartFilterText(text: string, config: McpAccessibilityConfig): s } export type { McpAccessibilityConfig } from "./constants.ts"; -export { DEFAULT_MCP_ACCESSIBILITY_CONFIG } from "./constants.ts"; +export { + DEFAULT_MCP_ACCESSIBILITY_CONFIG, + clampMcpAccessibilityConfig, + MCP_ACCESSIBILITY_TAIL_RESERVE, +} from "./constants.ts"; diff --git a/open-sse/services/compression/types.ts b/open-sse/services/compression/types.ts index e3eebad8bc..d51668d5ba 100644 --- a/open-sse/services/compression/types.ts +++ b/open-sse/services/compression/types.ts @@ -338,4 +338,7 @@ export const DEFAULT_ULTRA_CONFIG: UltraConfig = { }; export type { McpAccessibilityConfig } from "./engines/mcpAccessibility/constants.ts"; -export { DEFAULT_MCP_ACCESSIBILITY_CONFIG } from "./engines/mcpAccessibility/constants.ts"; +export { + DEFAULT_MCP_ACCESSIBILITY_CONFIG, + clampMcpAccessibilityConfig, +} from "./engines/mcpAccessibility/constants.ts"; diff --git a/src/lib/db/compression.ts b/src/lib/db/compression.ts index 8e19fb9bc6..8dc0a30aa9 100644 --- a/src/lib/db/compression.ts +++ b/src/lib/db/compression.ts @@ -11,6 +11,7 @@ import { DEFAULT_MCP_ACCESSIBILITY_CONFIG, DEFAULT_RTK_CONFIG, DEFAULT_ULTRA_CONFIG, + clampMcpAccessibilityConfig, type AggressiveConfig, type CavemanConfig, type CavemanOutputModeConfig, @@ -509,32 +510,10 @@ export function getDefaultRtkConfig(): RtkConfig { } function normalizeMcpAccessibilityConfig(value: unknown): McpAccessibilityConfig { - const record = toRecord(value); - return { - ...DEFAULT_MCP_ACCESSIBILITY_CONFIG, - ...record, - enabled: record.enabled !== false, - maxTextChars: - typeof record.maxTextChars === "number" && record.maxTextChars > 0 - ? Math.floor(record.maxTextChars) - : DEFAULT_MCP_ACCESSIBILITY_CONFIG.maxTextChars, - collapseThreshold: - typeof record.collapseThreshold === "number" && record.collapseThreshold > 0 - ? Math.floor(record.collapseThreshold) - : DEFAULT_MCP_ACCESSIBILITY_CONFIG.collapseThreshold, - collapseKeepHead: - typeof record.collapseKeepHead === "number" && record.collapseKeepHead >= 0 - ? Math.floor(record.collapseKeepHead) - : DEFAULT_MCP_ACCESSIBILITY_CONFIG.collapseKeepHead, - collapseKeepTail: - typeof record.collapseKeepTail === "number" && record.collapseKeepTail >= 0 - ? Math.floor(record.collapseKeepTail) - : DEFAULT_MCP_ACCESSIBILITY_CONFIG.collapseKeepTail, - minLengthToProcess: - typeof record.minLengthToProcess === "number" && record.minLengthToProcess > 0 - ? Math.floor(record.minLengthToProcess) - : DEFAULT_MCP_ACCESSIBILITY_CONFIG.minLengthToProcess, - }; + // clampMcpAccessibilityConfig (engine layer) owns the numeric floors so the DB normalizer and + // the live MCP-server read path agree — in particular it floors maxTextChars to a sane minimum + // (a value below the tail reservation would make smartFilterText truncate the whole text away). + return clampMcpAccessibilityConfig(value); } export async function getMcpAccessibilityConfig(): Promise { diff --git a/tests/unit/compression/mcpAccessibility-config-bounds.test.ts b/tests/unit/compression/mcpAccessibility-config-bounds.test.ts new file mode 100644 index 0000000000..a13436aa08 --- /dev/null +++ b/tests/unit/compression/mcpAccessibility-config-bounds.test.ts @@ -0,0 +1,48 @@ +import { test } from "node:test"; +import assert from "node:assert/strict"; +import { + smartFilterText, + clampMcpAccessibilityConfig, +} from "../../../open-sse/services/compression/engines/mcpAccessibility/index.ts"; +import { DEFAULT_MCP_ACCESSIBILITY_CONFIG } from "../../../open-sse/services/compression/engines/mcpAccessibility/constants.ts"; + +// smartFilterText reserves 300 chars for the truncation tail/notice, so any maxTextChars below +// that leaves headSize <= 0 and the whole tool result is replaced by the notice (total data +// loss). The DB normalizer floored maxTextChars only at > 0, and the production read path in +// server.ts bypassed bounding entirely. clampMcpAccessibilityConfig is the shared guard. +test("clamps maxTextChars below the tail reserve to the default", () => { + for (const bad of [1, 50, 300, 599]) { + assert.equal( + clampMcpAccessibilityConfig({ maxTextChars: bad }).maxTextChars, + DEFAULT_MCP_ACCESSIBILITY_CONFIG.maxTextChars, + `maxTextChars=${bad} must fall back to default` + ); + } +}); + +test("keeps a sane maxTextChars (>= 600)", () => { + assert.equal(clampMcpAccessibilityConfig({ maxTextChars: 600 }).maxTextChars, 600); + assert.equal(clampMcpAccessibilityConfig({ maxTextChars: 1000 }).maxTextChars, 1000); + assert.equal(clampMcpAccessibilityConfig({ maxTextChars: 1234.9 }).maxTextChars, 1234); +}); + +test("bounds the other numeric fields and honors enabled", () => { + const c = clampMcpAccessibilityConfig({ + collapseThreshold: -5, + minLengthToProcess: 0, + collapseKeepHead: -1, + enabled: false, + }); + assert.equal(c.collapseThreshold, DEFAULT_MCP_ACCESSIBILITY_CONFIG.collapseThreshold); + assert.equal(c.minLengthToProcess, DEFAULT_MCP_ACCESSIBILITY_CONFIG.minLengthToProcess); + assert.equal(c.collapseKeepHead, DEFAULT_MCP_ACCESSIBILITY_CONFIG.collapseKeepHead); + assert.equal(c.enabled, false); +}); + +test("a clamped config never lets smartFilterText truncate the whole text away", () => { + // The previously-dangerous stored value: maxTextChars=50 → clamps to default, so a + // 1000-char tool result is NOT replaced wholesale by the truncation notice. + const cfg = clampMcpAccessibilityConfig({ maxTextChars: 50, minLengthToProcess: 1 }); + const out = smartFilterText("A".repeat(1000), cfg); + assert.ok(out.includes("A".repeat(500)), "content preserved (not nuked by a tiny maxTextChars)"); +});