From 332562321f509edc7be4e3d4d93f622a6b2b0d29 Mon Sep 17 00:00:00 2001 From: Thomas Connally Date: Sat, 13 Jun 2026 10:34:50 -0500 Subject: [PATCH 1/3] =?UTF-8?q?fix:=20address=206=20review=20findings=20?= =?UTF-8?q?=E2=80=94=20timeout,=20audit,=20cache,=20stderr,=20filter,=20co?= =?UTF-8?q?mments?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Add configurable timeout (30s default) to upstream tools/list calls. A hung upstream no longer blocks tool discovery from healthy ones. New config key: upstreamTimeoutMs (schema.ts). - Add 2s TTL cache on collectTools() to prevent redundant fan-out when multiple clients call tools/list simultaneously. - Serialize audit logger writes with a promise chain to prevent NDJSON line interleaving under concurrent HTTP requests. - Rate-limit upstream stderr logging to 10KB/min per upstream (configurable via stderrLogBytesPerMinute in server config). - Pre-compile micromatch glob patterns via matcher() in ToolFilter constructor instead of recompiling on every isAllowed() call. - Fix comment numbering in Shrinker.applyRules() (steps 6→8, 7→9). Closes #1, #2, #3, #4, #5, #6 --- src/config/schema.ts | 4 ++++ src/core/aggregator.ts | 18 +++++++++++++++++- src/core/filter.ts | 9 +++++++-- src/core/shrinker.ts | 4 ++-- src/observability/audit.ts | 8 +++++++- src/upstream/manager.ts | 14 +++++++++++++- 6 files changed, 50 insertions(+), 7 deletions(-) diff --git a/src/config/schema.ts b/src/config/schema.ts index 33e4c25..8d1a012 100644 --- a/src/config/schema.ts +++ b/src/config/schema.ts @@ -9,6 +9,8 @@ const stdioServerSchema = z.object({ maxRestarts: z.number().int().min(0).default(5), /** Initial backoff in ms, doubles up to 30s. */ restartBackoffMs: z.number().int().min(0).default(500), + /** Per-upstream stderr log budget in bytes per minute (0 = unlimited). */ + stderrLogBytesPerMinute: z.number().int().min(0).default(10_000), /** Per-server description-shrink override. */ shrink: z .object({ @@ -144,6 +146,8 @@ export const tooltrimConfigSchema = z.object({ observability: observabilitySchema, policy: policySchema, logLevel: z.enum(["trace", "debug", "info", "warn", "error", "fatal", "silent"]).default("info"), + /** Timeout in ms for upstream listTools/listResources/listPrompts calls (default 30s). */ + upstreamTimeoutMs: z.number().int().min(1000).default(30_000), }); export type StdioServerConfig = z.infer; diff --git a/src/core/aggregator.ts b/src/core/aggregator.ts index 7dfd4d5..9634903 100644 --- a/src/core/aggregator.ts +++ b/src/core/aggregator.ts @@ -54,6 +54,9 @@ export class Aggregator { private readonly promptRoute = new Map(); /** uri -> upstreamId for resources (URIs are unique upstream-side; we keep first wins on collision). */ private readonly resourceRoute = new Map(); + /** Short-lived cache for collectTools() to debounce redundant fan-out. */ + private toolsCache: { tools: unknown[]; ts: number } | null = null; + private readonly TOOLS_CACHE_TTL_MS = 2000; constructor(deps: AggregatorDeps) { this.deps = deps; @@ -210,12 +213,24 @@ export class Aggregator { * and return the merged list. */ async collectTools(): Promise { + // Return cached result if fresh enough to avoid redundant upstream fan-out. + if (this.toolsCache && Date.now() - this.toolsCache.ts < this.TOOLS_CACHE_TTL_MS) { + return this.toolsCache.tools; + } + this.toolRoute.clear(); const out: Record[] = []; + const timeoutMs = this.deps.cfg.upstreamTimeoutMs ?? 30_000; + for (const [id, conn] of this.deps.upstream.connections) { if (conn.status !== "connected" || !conn.capabilities?.tools) continue; try { - const result = await conn.client.listTools(); + const result = await Promise.race([ + conn.client.listTools(), + new Promise((_, reject) => + setTimeout(() => reject(new Error(`upstream \"${id}\" tools/list timed out after ${timeoutMs}ms`)), timeoutMs), + ), + ]); for (const t of result.tools ?? []) { const namespaced = this.namespace(id, t.name); if (!this.deps.filter.isAllowed(namespaced, "tool")) continue; @@ -249,6 +264,7 @@ export class Aggregator { this.log.warn({ id, err: errMsg(err) }, "upstream tools/list failed"); } } + this.toolsCache = { tools: out, ts: Date.now() }; return out; } diff --git a/src/core/filter.ts b/src/core/filter.ts index f3aea65..c5df741 100644 --- a/src/core/filter.ts +++ b/src/core/filter.ts @@ -26,6 +26,9 @@ export class ToolFilter { private readonly applyTools: boolean; private readonly applyResources: boolean; private readonly applyPrompts: boolean; + /** Pre-compiled matchers for batch glob evaluation. */ + private readonly allowMatcher: ((name: string) => boolean) | null; + private readonly denyMatcher: ((name: string) => boolean) | null; constructor(opts: FilterOptions) { this.allow = opts.allow; @@ -34,6 +37,8 @@ export class ToolFilter { this.applyTools = apply.tools ?? true; this.applyResources = apply.resources ?? true; this.applyPrompts = apply.prompts ?? true; + this.allowMatcher = this.allow.length > 0 ? micromatch.matcher(this.allow) : null; + this.denyMatcher = this.deny.length > 0 ? micromatch.matcher(this.deny) : null; } static fromConfig(cfg: TooltrimConfig): ToolFilter { @@ -49,10 +54,10 @@ export class ToolFilter { if (kind === "resource" && !this.applyResources) return true; if (kind === "prompt" && !this.applyPrompts) return true; - if (this.allow.length > 0 && !micromatch.isMatch(namespacedName, this.allow)) { + if (this.allowMatcher && !this.allowMatcher(namespacedName)) { return false; } - if (this.deny.length > 0 && micromatch.isMatch(namespacedName, this.deny)) { + if (this.denyMatcher && this.denyMatcher(namespacedName)) { return false; } return true; diff --git a/src/core/shrinker.ts b/src/core/shrinker.ts index 2b6efb0..aa853d8 100644 --- a/src/core/shrinker.ts +++ b/src/core/shrinker.ts @@ -180,10 +180,10 @@ export class Shrinker { .join(" "); s = s.replace(/\s+/g, " ").trim(); - // 6. capitalise first letter for readability + // 8. capitalise first letter for readability if (s.length > 0) s = s[0]!.toUpperCase() + s.slice(1); - // 7. truncate at the first sentence boundary past `maxChars` + // 9. truncate at the first sentence boundary past `maxChars` if (s.length > maxChars) { const sliceEnd = this.findSentenceEnd(s, maxChars); s = s.slice(0, sliceEnd).trimEnd(); diff --git a/src/observability/audit.ts b/src/observability/audit.ts index f70d62a..b6d0a6a 100644 --- a/src/observability/audit.ts +++ b/src/observability/audit.ts @@ -24,6 +24,8 @@ export class AuditLogger { private readonly enabled: boolean; private readonly filePath: string; private dirEnsured = false; + /** Serializes appendFile calls to prevent interleaved NDJSON lines. */ + private writeQueue = Promise.resolve(); constructor(enabled: boolean, filePath: string) { this.enabled = enabled; @@ -41,6 +43,10 @@ export class AuditLogger { this.dirEnsured = true; } const line = JSON.stringify({ ts: new Date().toISOString(), ...ev }); - await appendFile(this.filePath, line + "\n", "utf8"); + // Serialize writes to prevent interleaving under concurrent requests. + this.writeQueue = this.writeQueue.then(() => + appendFile(this.filePath, line + "\n", "utf8"), + ); + return this.writeQueue; } } diff --git a/src/upstream/manager.ts b/src/upstream/manager.ts index e279f12..a593539 100644 --- a/src/upstream/manager.ts +++ b/src/upstream/manager.ts @@ -161,8 +161,20 @@ export class UpstreamManager { cwd: cfg.cwd, stderr: "pipe", }); + let stderrBytesThisMinute = 0; + let stderrMinuteStart = Date.now(); + const stderrBudget = cfg.stderrLogBytesPerMinute ?? 10_000; + transport.stderr?.on("data", (chunk: Buffer) => { - this.log.debug({ id, chunk: chunk.toString("utf8").trim() }, "upstream stderr"); + const now = Date.now(); + if (now - stderrMinuteStart > 60_000) { + stderrBytesThisMinute = 0; + stderrMinuteStart = now; + } + if (stderrBytesThisMinute < stderrBudget) { + this.log.debug({ id, chunk: chunk.toString("utf8").trim() }, "upstream stderr"); + stderrBytesThisMinute += chunk.length; + } }); await client.connect(transport); } From 1d7792fd0e42f3d3c01539ca774a66ac31c1f6b1 Mon Sep 17 00:00:00 2001 From: perseus <51974392+tcconnally@users.noreply.github.com> Date: Mon, 15 Jun 2026 19:44:46 -0500 Subject: [PATCH 2/3] =?UTF-8?q?fix:=20address=20review=20findings=20?= =?UTF-8?q?=E2=80=94=20stderr=200=3Dunlimited,=20clearTimeout=20leak,=20sc?= =?UTF-8?q?hema=20comment=20accuracy?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- src/config/schema.ts | 2 +- src/core/aggregator.ts | 13 +++++++------ src/upstream/manager.ts | 2 +- 3 files changed, 9 insertions(+), 8 deletions(-) diff --git a/src/config/schema.ts b/src/config/schema.ts index 8d1a012..931e08b 100644 --- a/src/config/schema.ts +++ b/src/config/schema.ts @@ -146,7 +146,7 @@ export const tooltrimConfigSchema = z.object({ observability: observabilitySchema, policy: policySchema, logLevel: z.enum(["trace", "debug", "info", "warn", "error", "fatal", "silent"]).default("info"), - /** Timeout in ms for upstream listTools/listResources/listPrompts calls (default 30s). */ + /** Timeout in ms for upstream listTools calls (default 30s). */ upstreamTimeoutMs: z.number().int().min(1000).default(30_000), }); diff --git a/src/core/aggregator.ts b/src/core/aggregator.ts index 9634903..972b093 100644 --- a/src/core/aggregator.ts +++ b/src/core/aggregator.ts @@ -225,12 +225,12 @@ export class Aggregator { for (const [id, conn] of this.deps.upstream.connections) { if (conn.status !== "connected" || !conn.capabilities?.tools) continue; try { - const result = await Promise.race([ - conn.client.listTools(), - new Promise((_, reject) => - setTimeout(() => reject(new Error(`upstream \"${id}\" tools/list timed out after ${timeoutMs}ms`)), timeoutMs), - ), - ]); + let timer: ReturnType; + const timeout = new Promise((_, reject) => { + timer = setTimeout(() => reject(new Error(`upstream "${id}" tools/list timed out after ${timeoutMs}ms`)), timeoutMs); + }); + const result = await Promise.race([conn.client.listTools(), timeout]); + clearTimeout(timer); for (const t of result.tools ?? []) { const namespaced = this.namespace(id, t.name); if (!this.deps.filter.isAllowed(namespaced, "tool")) continue; @@ -261,6 +261,7 @@ export class Aggregator { }); } } catch (err) { + clearTimeout(timer); this.log.warn({ id, err: errMsg(err) }, "upstream tools/list failed"); } } diff --git a/src/upstream/manager.ts b/src/upstream/manager.ts index a593539..f824df0 100644 --- a/src/upstream/manager.ts +++ b/src/upstream/manager.ts @@ -171,7 +171,7 @@ export class UpstreamManager { stderrBytesThisMinute = 0; stderrMinuteStart = now; } - if (stderrBytesThisMinute < stderrBudget) { + if (stderrBudget === 0 || stderrBytesThisMinute < stderrBudget) { this.log.debug({ id, chunk: chunk.toString("utf8").trim() }, "upstream stderr"); stderrBytesThisMinute += chunk.length; } From 0cbdabf3defb26df2326a9117fc5cb59282ccde3 Mon Sep 17 00:00:00 2001 From: perseus <51974392+tcconnally@users.noreply.github.com> Date: Tue, 16 Jun 2026 22:41:05 +0000 Subject: [PATCH 3/3] fix: move timer declaration before try block, add micromatch type declaration - aggregator.ts: move `let timer` before `try` so it is in scope for the catch block's clearTimeout call - Add src/types/micromatch.d.ts to resolve tsc "Could not find declaration file for module 'micromatch'" (the @types/micromatch package is a stub) Addresses review feedback on PR #7. --- src/core/aggregator.ts | 2 +- src/types/micromatch.d.ts | 15 +++++++++++++++ 2 files changed, 16 insertions(+), 1 deletion(-) create mode 100644 src/types/micromatch.d.ts diff --git a/src/core/aggregator.ts b/src/core/aggregator.ts index 972b093..9a00e0b 100644 --- a/src/core/aggregator.ts +++ b/src/core/aggregator.ts @@ -224,8 +224,8 @@ export class Aggregator { for (const [id, conn] of this.deps.upstream.connections) { if (conn.status !== "connected" || !conn.capabilities?.tools) continue; + let timer: ReturnType | undefined; try { - let timer: ReturnType; const timeout = new Promise((_, reject) => { timer = setTimeout(() => reject(new Error(`upstream "${id}" tools/list timed out after ${timeoutMs}ms`)), timeoutMs); }); diff --git a/src/types/micromatch.d.ts b/src/types/micromatch.d.ts new file mode 100644 index 0000000..6c4deb9 --- /dev/null +++ b/src/types/micromatch.d.ts @@ -0,0 +1,15 @@ +declare module "micromatch" { + function micromatch( + patterns: string | string[], + options?: Record, + ): (str: string) => boolean; + + namespace micromatch { + function matcher( + patterns: string | string[], + options?: Record, + ): (str: string) => boolean; + } + + export = micromatch; +}