From 65c49585d1bc180924ad39411779711d2dc3efe7 Mon Sep 17 00:00:00 2001 From: gsxdsm Date: Mon, 15 Jun 2026 14:52:58 -0700 Subject: [PATCH] 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 --- ...t-logged-in-thin-env-keychain-isolation.md | 2 + .../src/__tests__/acp-driver.test.ts | 41 +++++++++++++++++++ packages/pi-claude-cli/src/acp-driver.ts | 13 +++++- 3 files changed, 54 insertions(+), 2 deletions(-) diff --git a/docs/solutions/integration-issues/acp-bridge-not-logged-in-thin-env-keychain-isolation.md b/docs/solutions/integration-issues/acp-bridge-not-logged-in-thin-env-keychain-isolation.md index 475ccc457c..0f83b602fc 100644 --- a/docs/solutions/integration-issues/acp-bridge-not-logged-in-thin-env-keychain-isolation.md +++ b/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 diff --git a/packages/pi-claude-cli/src/__tests__/acp-driver.test.ts b/packages/pi-claude-cli/src/__tests__/acp-driver.test.ts index ee9e1a854d..8b2620e0e1 100644 --- a/packages/pi-claude-cli/src/__tests__/acp-driver.test.ts +++ b/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> }; + 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; diff --git a/packages/pi-claude-cli/src/acp-driver.ts b/packages/pi-claude-cli/src/acp-driver.ts index d034475dda..499b001a1d 100644 --- a/packages/pi-claude-cli/src/acp-driver.ts +++ b/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