From daa37d08c5c053f7bb01f6b61db276fe5c562f61 Mon Sep 17 00:00:00 2001 From: gsxdsm Date: Mon, 15 Jun 2026 12:32:00 -0700 Subject: [PATCH] feat(acp): surface bridge auth failure in the UI with fallback / fix-auth (R17) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When the bridged `claude` can't authenticate (detached daemon / no keychain), the turn returns "Not logged in" instead of a real answer. Rather than silently relay that, detect it and let the user choose. - Driver: detect a "Not logged in"-only turn and write a cross-process signal (fusion-acp-bridge-auth.json); a real response clears it (acp-driver test). - Dashboard status: GET /providers/claude-cli/status reports acp.authFailed + authReason from the signal. - UI: the Claude CLI provider card shows an auth-failure banner with "Use claude -p" (sets experimentalFeatures.claudeCliAcp=false) and "I fixed auth — re-test", plus a fix hint (run `claude` to log in). - Enable resolution now recomputes each call with an operator force-override (FUSION_CLAUDE_ACP_FORCE), so the "Use -p" fallback takes effect on the next turn — no restart. claude-acp-enable tests updated. pi-claude-cli + engine tests green; dashboard typecheck clean. Co-Authored-By: Claude Opus 4.8 --- packages/dashboard/app/api/legacy.ts | 12 ++++ .../app/components/ClaudeCliProviderCard.tsx | 58 +++++++++++++++++++ .../src/routes/register-auth-routes.ts | 22 +++++++ .../src/__tests__/claude-acp-enable.test.ts | 16 ++--- packages/engine/src/claude-acp-enable.ts | 15 +++-- .../src/__tests__/acp-driver.test.ts | 17 +++++- packages/pi-claude-cli/src/acp-driver.ts | 43 +++++++++++++- 7 files changed, 167 insertions(+), 16 deletions(-) diff --git a/packages/dashboard/app/api/legacy.ts b/packages/dashboard/app/api/legacy.ts index 844f349497..1b28288ad4 100644 --- a/packages/dashboard/app/api/legacy.ts +++ b/packages/dashboard/app/api/legacy.ts @@ -1674,6 +1674,18 @@ export interface ClaudeCliStatus { reason?: string; } | null; ready: boolean; + /** Route A ACP transport state (Claude CLI via the claude-code-cli-acp bridge). */ + acp?: { + /** experimentalFeatures.claudeCliAcp (default ON). */ + enabled: boolean; + /** The acp-runtime plugin published a bundled bridge path. */ + bridgeAvailable: boolean; + /** Claude CLI is actually routing through the bridge (enabled + flag + bridge). */ + active: boolean; + /** The bridged `claude` returned "Not logged in" — needs fallback or re-auth (R17). */ + authFailed: boolean; + authReason?: string; + }; } export interface DroidCliStatus { diff --git a/packages/dashboard/app/components/ClaudeCliProviderCard.tsx b/packages/dashboard/app/components/ClaudeCliProviderCard.tsx index 53fa189b67..c8f0521e6a 100644 --- a/packages/dashboard/app/components/ClaudeCliProviderCard.tsx +++ b/packages/dashboard/app/components/ClaudeCliProviderCard.tsx @@ -4,6 +4,8 @@ import { Loader2 } from "lucide-react"; import { fetchClaudeCliStatus, setClaudeCliEnabled, + fetchGlobalSettings, + updateGlobalSettings, type ClaudeCliStatus, } from "../api"; import { ProviderIcon } from "./ProviderIcon"; @@ -122,6 +124,29 @@ export function ClaudeCliProviderCard({ [onToggled, refresh], ); + // R17 fallback: the bridge can't authenticate Claude. Turn the ACP transport + // off (experimentalFeatures.claudeCliAcp=false) so Claude CLI uses `claude -p`. + const handleFallbackToDashP = useCallback(async () => { + setBusy("disabling"); + setLastAction(null); + try { + const gs = await fetchGlobalSettings(); + await updateGlobalSettings({ + experimentalFeatures: { ...(gs.experimentalFeatures ?? {}), claudeCliAcp: false }, + }); + if (mountedRef.current) { + setLastAction({ kind: "disabled", restartRequired: false }); + } + await refresh(); + } catch (err) { + if (mountedRef.current) { + setLastAction({ kind: "error", message: err instanceof Error ? err.message : String(err) }); + } + } finally { + if (mountedRef.current) setBusy(null); + } + }, [refresh]); + const binaryAvailable = status?.binary.available ?? false; const currentlyEnabled = status?.enabled ?? authenticated; @@ -217,6 +242,39 @@ export function ClaudeCliProviderCard({ {description} + {status?.acp?.authFailed && ( +
+ + {t("setup.claudeCli.acpAuthFailedTitle", "Claude CLI bridge can't authenticate")} + +

+ {status.acp.authReason ?? + t( + "setup.claudeCli.acpAuthFailed", + "The ACP bridge reached a Claude session that isn't logged in. Fall back to `claude -p`, or fix authentication and re-test.", + )} +

+
+ + +
+

+ {t( + "setup.claudeCli.fixAuthHint", + "To fix: run `claude` in a terminal and complete login, then re-test.", + )} +

+
+ )}
{actions}
{lastAction && } diff --git a/packages/dashboard/src/routes/register-auth-routes.ts b/packages/dashboard/src/routes/register-auth-routes.ts index 908102c405..c2927774b8 100644 --- a/packages/dashboard/src/routes/register-auth-routes.ts +++ b/packages/dashboard/src/routes/register-auth-routes.ts @@ -1,4 +1,7 @@ import type { Request } from "express"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { existsSync, readFileSync } from "node:fs"; import { isGhAvailable, isGhAuthenticated } from "@fusion/core"; import { probeClaudeCli } from "../claude-cli-probe.js"; import { probeDroidCli } from "../droid-cli-probe.js"; @@ -600,6 +603,23 @@ export const registerAuthRoutes: ApiRouteRegistrar = (ctx) => { const acpBridgeAvailable = typeof process.env.FUSION_CLAUDE_ACP_BRIDGE === "string" && process.env.FUSION_CLAUDE_ACP_BRIDGE.length > 0; + // R17: the driver writes this signal when a turn comes back "Not logged in" + // (the bridged `claude` can't authenticate). Surface it so the UI can offer + // fall-back-to-`-p` or fix-auth. Path matches ACP_BRIDGE_AUTH_SIGNAL_PATH. + let acpAuthFailed = false; + let acpAuthReason: string | undefined; + try { + const signalPath = join(tmpdir(), "fusion-acp-bridge-auth.json"); + if (existsSync(signalPath)) { + const sig = JSON.parse(readFileSync(signalPath, "utf8")) as { authFailed?: boolean; reason?: string }; + if (sig?.authFailed) { + acpAuthFailed = true; + acpAuthReason = typeof sig.reason === "string" ? sig.reason : undefined; + } + } + } catch { + // best-effort; absence of the signal means no known auth failure + } res.json({ binary, @@ -609,6 +629,8 @@ export const registerAuthRoutes: ApiRouteRegistrar = (ctx) => { enabled: acpEnabled, bridgeAvailable: acpBridgeAvailable, active: enabled && acpEnabled && acpBridgeAvailable, + authFailed: acpAuthFailed, + authReason: acpAuthReason, }, // Convenience field: the provider card considers everything "ready" // when the binary is available, the user has enabled the toggle, diff --git a/packages/engine/src/__tests__/claude-acp-enable.test.ts b/packages/engine/src/__tests__/claude-acp-enable.test.ts index def192efb3..4d0f6bdbf0 100644 --- a/packages/engine/src/__tests__/claude-acp-enable.test.ts +++ b/packages/engine/src/__tests__/claude-acp-enable.test.ts @@ -16,22 +16,22 @@ describe("claudeAcpExperimentalEnabled — default ON", () => { }); describe("applyClaudeAcpEnable — translates the flag to FUSION_CLAUDE_ACP", () => { - it("sets FUSION_CLAUDE_ACP=1 when enabled (default ON) and env unset", () => { + it("sets FUSION_CLAUDE_ACP=1 when enabled (default ON)", () => { const env: NodeJS.ProcessEnv = {}; expect(applyClaudeAcpEnable({}, env)).toBe(true); expect(env.FUSION_CLAUDE_ACP).toBe("1"); }); - it("does NOT set the env when the flag is explicitly false", () => { - const env: NodeJS.ProcessEnv = {}; + it("sets FUSION_CLAUDE_ACP=0 when the flag is explicitly false (recomputed each call)", () => { + const env: NodeJS.ProcessEnv = { FUSION_CLAUDE_ACP: "1" }; // stale prior value expect(applyClaudeAcpEnable({ experimentalFeatures: { claudeCliAcp: false } }, env)).toBe(false); - expect(env.FUSION_CLAUDE_ACP).toBeUndefined(); + expect(env.FUSION_CLAUDE_ACP).toBe("0"); // flip takes effect — no latch on our own write }); - it("honors an explicit env override (operator/test wins over the flag)", () => { - const off: NodeJS.ProcessEnv = { FUSION_CLAUDE_ACP: "0" }; - expect(applyClaudeAcpEnable({}, off)).toBe(false); // flag default-on, but env says off + it("honors the operator force-override FUSION_CLAUDE_ACP_FORCE over the flag", () => { + const off: NodeJS.ProcessEnv = { FUSION_CLAUDE_ACP_FORCE: "0" }; + expect(applyClaudeAcpEnable({}, off)).toBe(false); // flag default-on, but forced off expect(off.FUSION_CLAUDE_ACP).toBe("0"); - const on: NodeJS.ProcessEnv = { FUSION_CLAUDE_ACP: "1" }; + const on: NodeJS.ProcessEnv = { FUSION_CLAUDE_ACP_FORCE: "1" }; expect(applyClaudeAcpEnable({ experimentalFeatures: { claudeCliAcp: false } }, on)).toBe(true); expect(on.FUSION_CLAUDE_ACP).toBe("1"); }); diff --git a/packages/engine/src/claude-acp-enable.ts b/packages/engine/src/claude-acp-enable.ts index 500702faca..e02fb7ab86 100644 --- a/packages/engine/src/claude-acp-enable.ts +++ b/packages/engine/src/claude-acp-enable.ts @@ -8,8 +8,10 @@ * `FUSION_CLAUDE_ACP_BRIDGE` on load — KTD10; absent → fail-closed to `-p`). * * The user-facing switch is `experimentalFeatures.claudeCliAcp`: ON unless the - * user explicitly sets it to `false`. An explicit `FUSION_CLAUDE_ACP` env value - * always wins (operator / test override) — see {@link applyClaudeAcpEnable}. + * user explicitly sets it to `false`. An operator force-override + * (`FUSION_CLAUDE_ACP_FORCE=0|1`) always wins. The decision is recomputed every + * call (each `createFnAgent`) so flipping the flag — e.g. the UI "use `claude -p`" + * fallback after an auth failure — takes effect on the next turn, no restart. */ /** True unless `experimentalFeatures.claudeCliAcp === false` (default ON). */ @@ -29,8 +31,11 @@ export function applyClaudeAcpEnable( globalSettings: Record | undefined, env: NodeJS.ProcessEnv = process.env, ): boolean { - if (typeof env.FUSION_CLAUDE_ACP === "string") return env.FUSION_CLAUDE_ACP === "1"; - const enabled = claudeAcpExperimentalEnabled(globalSettings); - if (enabled) env.FUSION_CLAUDE_ACP = "1"; + // Operator force-override (set in the launch environment), re-read every call + // so our own writes to FUSION_CLAUDE_ACP can't latch the decision. + const force = env.FUSION_CLAUDE_ACP_FORCE; + const enabled = + force === "1" ? true : force === "0" ? false : claudeAcpExperimentalEnabled(globalSettings); + env.FUSION_CLAUDE_ACP = enabled ? "1" : "0"; return enabled; } 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 83750a5f3e..4ba8c09d55 100644 --- a/packages/pi-claude-cli/src/__tests__/acp-driver.test.ts +++ b/packages/pi-claude-cli/src/__tests__/acp-driver.test.ts @@ -6,7 +6,9 @@ import { PassThrough } from "node:stream"; let scriptedUpdates: Array> = []; // Driver validates the bridge path with existsSync — make the fake path "exist". -vi.mock("node:fs", () => ({ existsSync: () => true })); +// writeFileSync/unlinkSync back the R17 auth-failure signal (spied). +const fsSpies = vi.hoisted(() => ({ writeFileSync: vi.fn(), unlinkSync: vi.fn() })); +vi.mock("node:fs", () => ({ existsSync: () => true, writeFileSync: fsSpies.writeFileSync, unlinkSync: fsSpies.unlinkSync })); vi.mock("node:child_process", () => ({ spawn: vi.fn(() => { @@ -119,6 +121,19 @@ describe("streamViaAcp — ACP→pi translation (U11)", () => { expect(done!.reason).toBe("toolUse"); }); + it("records the R17 auth-failure signal when the bridge turn is only 'Not logged in'", async () => { + fsSpies.writeFileSync.mockClear(); + scriptedUpdates = [ + { sessionUpdate: "agent_message_chunk", content: { type: "text", text: "Not logged in · Please run /login" } }, + ]; + const stream = streamViaAcp(MODEL, CTX, OPTS) as unknown as { _events: Array> }; + await flush(); + // signal file written with authFailed:true + const wrote = fsSpies.writeFileSync.mock.calls.find((c) => String(c[1]).includes("authFailed")); + expect(wrote).toBeTruthy(); + expect(String(wrote![1])).toContain("\"authFailed\":true"); + }); + it("ends with done even when the turn produces no content", async () => { scriptedUpdates = []; const stream = streamViaAcp(MODEL, CTX, OPTS) as unknown as { _events: Array> }; diff --git a/packages/pi-claude-cli/src/acp-driver.ts b/packages/pi-claude-cli/src/acp-driver.ts index f44051dd0f..7b8b9a929d 100644 --- a/packages/pi-claude-cli/src/acp-driver.ts +++ b/packages/pi-claude-cli/src/acp-driver.ts @@ -35,8 +35,9 @@ import { spawn, type ChildProcess } from "node:child_process"; import { Readable, Writable } from "node:stream"; -import { isAbsolute } from "node:path"; -import { existsSync } from "node:fs"; +import { isAbsolute, join } from "node:path"; +import { existsSync, writeFileSync, unlinkSync } from "node:fs"; +import { tmpdir } from "node:os"; import { ClientSideConnection, ndJsonStream, @@ -77,6 +78,35 @@ const MAX_CHUNK_CHARS = 64 * 1024; const MAX_TURN_CHARS = 5_000_000; const MAX_ID_CHARS = 256; +/** + * Cross-process signal for the dashboard: when the bridged `claude` can't + * authenticate (R17 — e.g. a detached daemon with no keychain), the turn comes + * back as "Not logged in · Please run /login" instead of a real answer. We + * record that here so `GET /providers/claude-cli/status` can surface it and the + * UI can prompt the user to fall back to `-p` or fix auth. A real response + * clears it. Best-effort; the path is recomputed identically dashboard-side. + */ +export const ACP_BRIDGE_AUTH_SIGNAL_PATH = join(tmpdir(), "fusion-acp-bridge-auth.json"); +const NOT_LOGGED_IN_RE = /not logged in|please run \/login/i; +let lastAuthFailed: boolean | undefined; + +function recordBridgeAuthState(failed: boolean, reason?: string): void { + if (lastAuthFailed === failed) return; // only write on transition + lastAuthFailed = failed; + try { + if (failed) { + writeFileSync( + ACP_BRIDGE_AUTH_SIGNAL_PATH, + JSON.stringify({ authFailed: true, at: new Date().toISOString(), reason: reason ?? "Claude in the ACP bridge is not logged in" }), + ); + } else { + unlinkSync(ACP_BRIDGE_AUTH_SIGNAL_PATH); + } + } catch { + /* best-effort signal — never let it affect the turn */ + } +} + /** * Bridge subprocess env allow-list. The bridged `claude` needs HOME (for * `~/.claude` auth/keychain, R17) and PATH; terminal vars improve rendering. @@ -173,6 +203,15 @@ export function streamViaAcp( // Downgrade a tool_use turn that surfaced zero pi tool calls → stop, so pi // doesn't try to dispatch non-existent tools (mirrors provider.ts:366-375). const toolCount = (bridge.getOutput().content ?? []).filter((c) => (c as { type?: string }).type === "toolCall").length; + // R17: a turn that is ONLY "Not logged in" (no tools, no real text) means + // the bridged `claude` can't authenticate — signal it for the UI. A real + // response (tools or non-trivial text) clears the signal. + const fullText = (bridge.getOutput().content ?? []) + .filter((c) => (c as { type?: string }).type === "text") + .map((c) => (c as { text?: string }).text ?? "") + .join(""); + if (toolCount === 0 && NOT_LOGGED_IN_RE.test(fullText)) recordBridgeAuthState(true); + else if (toolCount > 0 || fullText.trim().length > 0) recordBridgeAuthState(false); const effective: "stop" | "tool_use" = reason === "tool_use" && toolCount > 0 ? "tool_use" : "stop"; bridge.handleEvent({ type: "message_delta", delta: { stop_reason: effective === "tool_use" ? "tool_use" : "end_turn" } } as ClaudeApiEvent); stream.push({ type: "done", reason: effective === "tool_use" ? "toolUse" : "stop", message: bridge.getOutput() });