fix(review): address PR #1682 re-review (reuse concurrency + auth hardening)

- P1 (Greptile): a tool-use break-early turn released the warm connection
  (inUse=false) while conn.prompt() was still pending, letting the next turn
  launch a concurrent prompt on the same ACP session (protocol corruption).
  keepWarm now requires !sawToolCall, so a tool-use turn tears the connection
  down like the non-reuse path; only a clean stop turn (prompt fully resolved
  before finish) keeps it warm. + test.
- buildBridgeEnv: treat a whitespace-only auth var as absent (v.trim()), so a
  blank higher-preference token can't shadow a real lower-preference one and we
  never forward a useless blank token. + test.
- Auth-forwarding tests: clear ambient auth vars in beforeEach so a runner-env
  token can't shadow the case under test (CodeRabbit).
- Doc: clarify the allow-list never carries API keys by default; the single
  FUSION_CLAUDE_ACP_FORWARD_AUTH opt-in (default OFF) is the only exception.

348/348 pass, tsc clean.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

docs/solutions/integration-issues/acp-bridge-not-logged-in-thin-env-keychain-isolation.md

@@ -64,6 +64,8 @@ function buildBridgeEnv(supplied?: NodeJS.ProcessEnv): NodeJS.ProcessEnv {
 
 The critical additions over a naive `{HOME, PATH}` env are **`XDG_CONFIG_HOME`, `XDG_CACHE_HOME`, `USER`, `SHELL`, `LANG`**. With the full list, auth succeeds immediately.
 
+> The allow-list itself never carries API keys. The one exception is an **explicit operator opt-in**, `FUSION_CLAUDE_ACP_FORWARD_AUTH=1`, which forwards a single Claude auth token (`CLAUDE_CODE_OAUTH_TOKEN` > `ANTHROPIC_AUTH_TOKEN` > `ANTHROPIC_API_KEY`) for headless daemons that can't reach the login Keychain (gate R17). It is **OFF by default**, so the no-secrets posture above is the standing default — the opt-in only widens exposure when the operator deliberately enables it.
+
 **2. The Keychain finding (gate R17).** Claude Code stores its OAuth credentials in the macOS **login Keychain** as a generic-password item (service `"Claude Code-credentials"`), *not* a file (`~/.claude/.credentials.json` is an empty directory). A detached/headless process runs in a **different security session** and cannot read the login Keychain, so it fails regardless of env; a login-session process (interactive terminal, or an `fn` daemon launched from a login shell) can. This is codified as gate **R17**: the provider's runtime must have login-Keychain access. The driver also detects a not-logged-in turn and writes a best-effort cross-process signal (`fusion-acp-bridge-auth.json`) that `GET /providers/claude-cli/status` reads, so the dashboard can raise an auth-failure banner with a "Use `claude -p`" fallback.
 
 ## Why This Works

packages/pi-claude-cli/src/__tests__/acp-driver.test.ts

@@ -281,6 +281,30 @@ describe("connection reuse (item 1) — gated by FUSION_CLAUDE_ACP_REUSE", () =>
     expect(vi.mocked(spawn)).toHaveBeenCalledTimes(2); // turn 1 + the post-death cold restart
   });
 
+  it("does NOT keep the connection warm after a tool-use (break-early) turn — prompt() still pending", async () => {
+    process.env.FUSION_CLAUDE_ACP_REUSE = "1";
+    const reuseOpts = { ...OPTS, sessionId: "conv-tooluse" };
+
+    // Turn 1 (cold) breaks early on a pi-known tool — prompt() never resolves
+    // (the break happens mid-stream), so the connection must be torn down, not
+    // released warm, or turn 2 would launch a concurrent prompt on it.
+    const ctx1 = { messages: [{ role: "user", content: "hi" }, { role: "assistant", content: "hello" }] } as never;
+    scriptedUpdates = [
+      { sessionUpdate: "tool_call", toolCallId: "t1", _meta: { claudeCode: { toolName: "mcp__custom-tools__fn_task_list" } }, rawInput: {} },
+    ];
+    const s1 = streamViaAcp(MODEL, ctx1, reuseOpts) as unknown as { _events: Array<Record<string, unknown>> };
+    await flush();
+    expect(s1._events.find((e) => e.type === "done")!.reason).toBe("toolUse");
+    expect(vi.mocked(spawn)).toHaveBeenCalledTimes(1);
+
+    // Turn 2 must cold-spawn a fresh bridge (no warm reuse after a tool turn).
+    scriptedUpdates = [{ sessionUpdate: "agent_message_chunk", content: { type: "text", text: "after tool" } }];
+    const ctx2 = { messages: [...(ctx1 as unknown as { messages: unknown[] }).messages, { role: "user", content: "again" }] } as never;
+    streamViaAcp(MODEL, ctx2, reuseOpts);
+    await flush();
+    expect(vi.mocked(spawn)).toHaveBeenCalledTimes(2); // fresh spawn → no concurrent prompt on a warm conn
+  });
+
   it("cold-starts (no warm reuse) when the resume delta is empty (P1: no empty-prompt hang)", async () => {
     process.env.FUSION_CLAUDE_ACP_REUSE = "1";
     const reuseOpts = { ...OPTS, sessionId: "conv-empty" };
@@ -324,6 +348,14 @@ describe("buildBridgeEnv — R17 auth opt-in (item 3)", () => {
     authTok: process.env.ANTHROPIC_AUTH_TOKEN,
     key: process.env.ANTHROPIC_API_KEY,
   };
+  // Start each case from a clean slate so an ambient auth var in the runner's
+  // env can't shadow the token a test means to exercise (precedence is global).
+  beforeEach(() => {
+    delete process.env.FUSION_CLAUDE_ACP_FORWARD_AUTH;
+    delete process.env.CLAUDE_CODE_OAUTH_TOKEN;
+    delete process.env.ANTHROPIC_AUTH_TOKEN;
+    delete process.env.ANTHROPIC_API_KEY;
+  });
   afterEach(() => {
     for (const [k, v] of [
       ["FUSION_CLAUDE_ACP_FORWARD_AUTH", saved.flag],
@@ -370,6 +402,15 @@ describe("buildBridgeEnv — R17 auth opt-in (item 3)", () => {
     expect(env.ANTHROPIC_API_KEY).toBeUndefined();
   });
 
+  it("treats a whitespace-only higher-preference token as absent (no shadowing, no blank forward)", () => {
+    process.env.FUSION_CLAUDE_ACP_FORWARD_AUTH = "1";
+    process.env.CLAUDE_CODE_OAUTH_TOKEN = "   "; // blank → must be skipped
+    process.env.ANTHROPIC_API_KEY = "sk-real";
+    const env = buildBridgeEnv({ HOME: "/h", PATH: "/b" });
+    expect(env.CLAUDE_CODE_OAUTH_TOKEN).toBeUndefined();
+    expect(env.ANTHROPIC_API_KEY).toBe("sk-real"); // real lower-preference token wins
+  });
+
   it("reads the auth token from process.env, never a caller-supplied value (no token substitution)", () => {
     process.env.FUSION_CLAUDE_ACP_FORWARD_AUTH = "1";
     delete process.env.CLAUDE_CODE_OAUTH_TOKEN;

packages/pi-claude-cli/src/acp-driver.ts

@@ -148,7 +148,10 @@ export function buildBridgeEnv(supplied?: NodeJS.ProcessEnv): NodeJS.ProcessEnv
   if (process.env.FUSION_CLAUDE_ACP_FORWARD_AUTH === "1") {
     for (const key of BRIDGE_AUTH_ENV_KEYS) {
       const v = process.env[key];
-      if (typeof v === "string" && v.length > 0) {
+      // Treat a whitespace-only value as absent, so a blank higher-preference
+      // var doesn't shadow a real lower-preference token (and we never forward
+      // a useless blank token).
+      if (typeof v === "string" && v.trim().length > 0) {
         env[key] = v;
         break; // forward only the highest-preference token that's present
       }
@@ -290,8 +293,14 @@ export function streamViaAcp(
       if (inactivity) { clearTimeout(inactivity); inactivity = undefined; }
       if (onAbort && options.signal) options.signal.removeEventListener("abort", onAbort);
       const entry = cacheEntry;
+      // A tool-use turn breaks early while `conn.prompt()` is still pending (we
+      // never await it on break). Releasing the connection as warm here would
+      // let the next turn launch a SECOND concurrent prompt on the same ACP
+      // session — protocol corruption. So a tool-use turn always tears down,
+      // exactly like the non-reuse path; only a clean `stop` turn (prompt fully
+      // resolved before finish) keeps the connection warm.
       const keepWarm =
-        !destroy && entry !== undefined && reuseKey !== undefined &&
+        !destroy && !sawToolCall && entry !== undefined && reuseKey !== undefined &&
         acpSessionCache.get(reuseKey) === entry;
       if (keepWarm) {
         // Release the warm connection: drop this turn's handlers (so a late