From dc8510447fd1d0dbef8e90b5485dfb9b709faac0 Mon Sep 17 00:00:00 2001
From: gsxdsm <gsxdsm@users.noreply.github.com>
Date: Mon, 15 Jun 2026 12:43:08 -0700
Subject: [PATCH] fix(review): address PR #1681 round-2 comments
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

- CodeRabbit: spinner class `spin` -> `animate-spin` (matches the card's other
  Loader2 usages).
- CodeRabbit (major): tighten auth-failure detection so it only fires when the
  WHOLE turn is the short "Not logged in" message (<=80 chars), not when a long
  legitimate answer merely mentions the phrase — avoids false positives.
- CodeRabbit (major): expand the auth-signal test to assert the full invariant —
  set on a not-logged-in turn, clear (unlink) on a real response, and NOT flag a
  long answer that mentions the phrase.

pi-claude-cli acp-driver 5/5; typecheck clean.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
---
 .../app/components/ClaudeCliProviderCard.tsx  |  2 +-
 .../src/__tests__/acp-driver.test.ts          | 36 +++++++++++++------
 packages/pi-claude-cli/src/acp-driver.ts      | 14 +++++---
 3 files changed, 37 insertions(+), 15 deletions(-)

diff --git a/packages/dashboard/app/components/ClaudeCliProviderCard.tsx b/packages/dashboard/app/components/ClaudeCliProviderCard.tsx
index c8f0521e6a..3e0500a59f 100644
--- a/packages/dashboard/app/components/ClaudeCliProviderCard.tsx
+++ b/packages/dashboard/app/components/ClaudeCliProviderCard.tsx
@@ -260,7 +260,7 @@ export function ClaudeCliProviderCard({
             </p>
             <div className="onboarding-provider-card__actions">
               <button type="button" onClick={handleFallbackToDashP} disabled={busy !== null}>
-                {busy === "disabling" && <Loader2 className="spin" size={14} />}
+                {busy === "disabling" && <Loader2 className="animate-spin" size={14} />}
                 {t("setup.claudeCli.useDashP", "Use claude -p")}
               </button>
               <button type="button" onClick={handleTest} disabled={busy !== null}>
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 4ba8c09d55..77edc653bd 100644
--- a/packages/pi-claude-cli/src/__tests__/acp-driver.test.ts
+++ b/packages/pi-claude-cli/src/__tests__/acp-driver.test.ts
@@ -121,17 +121,33 @@ 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 () => {
+  it("R17 auth-signal: sets on a 'Not logged in' turn, clears on a real response, ignores long answers", async () => {
+    const run = async (text: string) => {
+      scriptedUpdates = [{ sessionUpdate: "agent_message_chunk", content: { type: "text", text } }];
+      streamViaAcp(MODEL, CTX, OPTS);
+      await flush();
+    };
+    const wroteAuthFailed = () =>
+      fsSpies.writeFileSync.mock.calls.some((c) => String(c[1]).includes('"authFailed":true'));
+
+    // Baseline: a real response leaves the signal cleared (lastAuthFailed=false).
+    await run("Here is a normal answer.");
     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<Record<string, unknown>> };
-    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");
+    fsSpies.unlinkSync.mockClear();
+
+    // 1. A turn that is ONLY the bridge's "Not logged in" message → signal written.
+    await run("Not logged in · Please run /login");
+    expect(wroteAuthFailed()).toBe(true);
+
+    // 2. A real response → signal cleared (unlink).
+    fsSpies.unlinkSync.mockClear();
+    await run("Sure — here's the result you asked for.");
+    expect(fsSpies.unlinkSync).toHaveBeenCalled();
+
+    // 3. A LONG legit answer that merely mentions the phrase → NOT flagged.
+    fsSpies.writeFileSync.mockClear();
+    await run(`If you are not logged in, the CLI prompts you to authenticate. ${"detail ".repeat(20)}`);
+    expect(wroteAuthFailed()).toBe(false);
   });
 
   it("ends with done even when the turn produces no content", async () => {
diff --git a/packages/pi-claude-cli/src/acp-driver.ts b/packages/pi-claude-cli/src/acp-driver.ts
index 2e1fe14cc8..1566e27a87 100644
--- a/packages/pi-claude-cli/src/acp-driver.ts
+++ b/packages/pi-claude-cli/src/acp-driver.ts
@@ -207,12 +207,18 @@ export function streamViaAcp(
       // 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 ?? [])
+      const trimmedText = (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);
+        .join("")
+        .trim();
+      // Only treat it as an auth failure when the WHOLE turn is essentially the
+      // bridge's short "Not logged in · Please run /login" message — not when a
+      // long, legitimate answer merely mentions the phrase (avoids false positives).
+      const isAuthFailure =
+        toolCount === 0 && trimmedText.length > 0 && trimmedText.length <= 80 && NOT_LOGGED_IN_RE.test(trimmedText);
+      if (isAuthFailure) recordBridgeAuthState(true);
+      else if (toolCount > 0 || trimmedText.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() });