fix: address 6 review findings — timeout, audit, cache, stderr, filter, comments

[tcconnally]

Fixes all 6 issues from a deep-dive code review.

Changes

#	Issue	Fix
#2	No timeout on upstream tools/list	Promise.race with configurable timeout (30s default). New upstreamTimeoutMs config key.
#3	Audit logger NDJSON interleaving	Promise chain serializes appendFile calls.
#4	collectTools called on every tools/list	2s TTL cache prevents redundant upstream fan-out.
#5	Upstream stderr unbounded	Rate-limited to 10KB/min per upstream via stderrLogBytesPerMinute.
#6	micromatch compiled per-tool	Pre-compiled via micromatch.matcher() in constructor.
#1	Comment numbering in shrinker	Steps 6→8, 7→9.

Tests

8 files, 36 tests, all passing. No regressions. Backward-compatible — new config keys have sensible defaults.

[false200]

Thanks @tcconnally, reviewed the diff locally, all 36 existing tests pass. Left a few inline notes. Happy to merge once all the fixes are in place.

[false200]

Three small fixes requested (stderr 0=unlimited, clearTimeout after Promise.race, schema comment accuracy). Rest looks good, will merge once those are addressed. Thanks again!

[false200]

Bug: schema says stderrLogBytesPerMinute: 0 means unlimited, but when budget is 0 this condition is never true, so nothing gets logged.

So the suggested fix would be:
if (stderrBudget === 0 || stderrBytesThisMinute < stderrBudget) {
...
}

[false200]

Minor leak: if listTools() wins the race, this timer is never cleared.

Suggested pattern would be:
let timer: ReturnType;
const timeout = new Promise((_, reject) => {
timer = setTimeout(() => reject(new Error(...)), timeoutMs);
});
try {
const result = await Promise.race([conn.client.listTools(), timeout]);
clearTimeout(timer!);
...
} catch (err) {
clearTimeout(timer!);
...
}

[false200]

Comment mentions listResources/listPrompts, but the PR only wraps listTools() in aggregator.ts. Either extend the timeout to those calls too, or narrow this comment to tools/list for now.

[tcconnally]

All three review findings addressed:

1. stderr 0=unlimited — added stderrBudget === 0 || guard in manager.ts:174
2. clearTimeout leak — timer stored and cleared after Promise.race resolves + in catch block (aggregator.ts)
3. Schema comment — narrowed to just listTools in schema.ts:149

Ready for another look when you have a moment.

[false200]

timer is declared inside try, so it's out of scope in catch - tsc fails with:

• TS2454: Variable 'timer' is used before being assigned (line 233)
• TS2304: Cannot find name 'timer' (line 264)

Move let timer before the try block.
Suggested fix should look like:

let timer: ReturnType<typeof setTimeout> | undefined;
try {
  const timeout = new Promise<never>((_, 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]);
  if (timer !== undefined) clearTimeout(timer);
  timer = undefined;
  // ... process tools
} catch (err) {
  if (timer !== undefined) clearTimeout(timer);
  this.log.warn({ id, err: errMsg(err) }, "upstream tools/list failed");
}

[false200]

pnpm typecheck fails here - @types/micromatch types matcher() as taking string, not string[]:

Suggested changes:

this.allowMatcher = this.allow.length > 0 ? micromatch.matcher(this.allow) : null;
this.denyMatcher = this.deny.length > 0 ? micromatch.matcher(this.deny) : null;

to

this.allowMatcher = this.allow.length > 0 ? micromatch.matcher(this.allow as string | string[]) : null;
this.denyMatcher = this.deny.length > 0 ? micromatch.matcher(this.deny as string | string[]) : null;

[false200]

@tcconnally Thanks for the quick turnaround, stderr fix and schema comment look good.

One more thing on clearTimeout: timer is declared inside the try block so it's out of scope in catch, and tsc fails on this branch. Move let timer before the try.

Also pnpm typecheck fails on filter.ts micromatch.matcher() typings, may need a cast for string[].

Happy to merge once all the fixes are in place.

[tcconnally]

Fixed both remaining issues:

1. timer scope — moved let timer before try block so it's accessible in catch
2. micromatch types — added src/types/micromatch.d.ts declaration since the @types/micromatch package is a stub

tsc --noEmit passes clean on these two issues now.

[false200]

Thanks @tcconnally!
Approving and merging once CI is green.