fix(review): address PR #1681 round-2 comments
- 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>
This commit is contained in:
@@ -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}>
|
||||
|
||||
@@ -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 () => {
|
||||
|
||||
@@ -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() });
|
||||
|
||||
Reference in New Issue
Block a user