This is a comment left during a code review.
Path: src/agents/pi-bundle-mcp-tools.ts
Line: 171-173

Comment:
**No timeout on server connect / tool listing**

`client.connect(transport)` and `listAllTools(client)` are awaited without any timeout. The `@modelcontextprotocol/sdk` `StdioClientTransport` does not impose a startup or capability-negotiation deadline on its own. If a bundle MCP server process hangs at startup (e.g., waits for stdin, never writes its capabilities message, or enters an infinite loop), `createBundleMcpToolRuntime` will never resolve, blocking every embedded Pi run and compaction that calls it.

The `catch` block on line 207 only fires if the promise *rejects*, not if it *stalls*. A stalling process will keep the Pi agent suspended indefinitely.

Consider wrapping both calls with a `Promise.race` against a short deadline (e.g., 10 s) and treating the timeout as a startup failure that triggers the existing `logWarn + disposeSession` path:

```ts
const CONNECT_TIMEOUT_MS = 10_000;

async function withTimeout<T>(promise: Promise<T>, ms: number, label: string): Promise<T> {
  let timer: ReturnType<typeof setTimeout>;
  const timeout = new Promise<never>((_, reject) => {
    timer = setTimeout(() => reject(new Error(`timed out after ${ms}ms: ${label}`)), ms);
  });
  try {
    return await Promise.race([promise, timeout]);
  } finally {
    clearTimeout(timer);
  }
}

// in the server loop:
await withTimeout(client.connect(transport), CONNECT_TIMEOUT_MS, `connect ${serverName}`);
const listedTools = await withTimeout(listAllTools(client), CONNECT_TIMEOUT_MS, `listTools ${serverName}`);
```

How can I resolve this? If you propose a fix, please make it concise.

---

This is a comment left during a code review.
Path: src/agents/pi-bundle-mcp-tools.ts
Line: 194-204

Comment:
**No timeout on tool call execution**

`client.callTool(...)` is awaited with no deadline. If a bundle MCP server becomes unresponsive after startup (e.g., the subprocess hangs mid-execution), the Pi agent will stall indefinitely waiting for the tool result. This is separate from the startup timeout on `connect`/`listTools`.

The MCP SDK does not add a per-call timeout by default. A timeout here is especially important because tool calls happen inside an active agent turn, and a hung tool call will suspend the entire conversation loop.

Consider applying the same `withTimeout` helper described in the `connect`/`listTools` comment above, using a longer deadline appropriate for tool execution (e.g., the same timeout that guards the overall Pi attempt, or a configurable per-tool limit).

How can I resolve this? If you propose a fix, please make it concise.

---

This is a comment left during a code review.
Path: src/agents/pi-bundle-mcp-tools.ts
Line: 164-169

Comment:
**`attachStderrLogging` called before the transport process is spawned**

`attachStderrLogging` reads `transport.stderr` at construction time (line 168), but the `StdioClientTransport` only spawns the child process — and therefore only populates `transport.stderr` — inside its `start()` method, which is invoked by `client.connect(transport)` on line 172.

At line 168 `transport.stderr` is `undefined`, so `attachStderrLogging` hits the early-return guard (`if (!stderr || ...)`) and returns `undefined` for every server. As a result `session.detachStderr` is always `undefined` and no stderr output from bundle MCP servers is ever logged.

Move the `attachStderrLogging` call to after `client.connect(transport)` and assign the result back to `session.detachStderr`:

```ts
await client.connect(transport);
session.detachStderr = attachStderrLogging(serverName, transport);
const listedTools = await listAllTools(client);
```

How can I resolve this? If you propose a fix, please make it concise.

---

This is a comment left during a code review.
Path: src/agents/mcp-stdio.ts
Line: 34-40

Comment:
**Non-string args silently dropped; inconsistent with `toStringRecord`**

`toStringArray` filters out any non-`string` element — including numbers — without a warning. For example an MCP config with `args: ["--port", 8080]` would silently become `["--port"]`, causing the server to launch without the port argument.

By contrast, `toStringRecord` (which normalises the `env` map) explicitly converts numbers and booleans to strings:

```ts
if (typeof entry === "number" || typeof entry === "boolean") {
  return [key, String(entry)] as const;
}
```

Either apply the same coercion to `toStringArray`, or emit a warning when non-string elements are encountered so the misconfiguration is visible in logs:

```ts
function toStringArray(value: unknown): string[] | undefined {
  if (!Array.isArray(value)) return undefined;
  const entries = value.map((entry) => {
    if (typeof entry === "string") return entry;
    if (typeof entry === "number" || typeof entry === "boolean") return String(entry);
    return null; // unrepresentable — will be dropped with a log below
  });
  const result = entries.filter((e): e is string => e !== null);
  return result;
}
```

How can I resolve this? If you propose a fix, please make it concise.