diff --git a/api/_lib/aiErrors.test.ts b/api/_lib/aiErrors.test.ts new file mode 100644 index 00000000..60e4fc4e --- /dev/null +++ b/api/_lib/aiErrors.test.ts @@ -0,0 +1,35 @@ +import { describe, expect, it } from "vitest"; + +import { preStreamError } from "./aiErrors.js"; + +describe("preStreamError", () => { + it("maps each kind to its status and a code matching the kind", () => { + expect(preStreamError("method_not_allowed").status).toBe(405); + expect(preStreamError("disabled").status).toBe(503); + expect(preStreamError("server_config").status).toBe(500); + expect(preStreamError("bad_request").status).toBe(400); + expect(preStreamError("rate_limit").status).toBe(429); + expect(preStreamError("disabled").body.code).toBe("disabled"); + }); + + it("returns copy that explains the problem and what to try", () => { + expect(preStreamError("disabled").body.error).toMatch(/turned off/i); + expect(preStreamError("bad_request").body.error).toMatch(/new chat/i); + expect(preStreamError("rate_limit").body.error).toMatch(/wait a moment/i); + expect(preStreamError("server_config").body.error).toMatch(/try again later/i); + }); + + it("never leaks internal configuration detail in the server_config copy", () => { + expect(preStreamError("server_config").body.error).not.toMatch(/api[_ ]?key|OPENROUTER/i); + }); + + it("carries retryAfter structurally rather than in the copy", () => { + const rl = preStreamError("rate_limit", 30); + expect(rl.body.retryAfter).toBe(30); + expect(rl.body.error).not.toMatch(/30/); + }); + + it("omits retryAfter entirely when none is given", () => { + expect("retryAfter" in preStreamError("rate_limit").body).toBe(false); + }); +}); diff --git a/api/_lib/aiErrors.ts b/api/_lib/aiErrors.ts new file mode 100644 index 00000000..6bbf6707 --- /dev/null +++ b/api/_lib/aiErrors.ts @@ -0,0 +1,100 @@ +// User-facing copy for every way an AI turn can fail — the pre-stream +// rejections (bad request, disabled, rate limited) and the mid-stream upstream +// failures. Kept in one module so the wording stays consistent and can be +// improved without touching route logic. + +export interface AiErrorBody { + code: string; + error: string; + retryAfter?: number; +} + +export type PreStreamErrorKind = + | "method_not_allowed" + | "disabled" + | "server_config" + | "bad_request" + | "rate_limit"; + +const PRE_STREAM_ERRORS: Record = { + method_not_allowed: { + status: 405, + error: "This endpoint only accepts POST requests.", + }, + disabled: { + status: 503, + error: "The AI assistant is currently turned off. Please check back later.", + }, + // Deliberately vague: the operator-facing cause (a missing OPENROUTER_API_KEY) + // is logged server-side and must not be echoed to the browser. + server_config: { + status: 500, + error: + "The AI assistant is temporarily unavailable due to a server configuration issue. " + + "Please try again later.", + }, + bad_request: { + status: 400, + error: "Your message couldn't be processed. Try starting a new chat, then send it again.", + }, + rate_limit: { + status: 429, + error: "You're sending messages too quickly. Please wait a moment and try again.", + }, +}; + +/** + * Maps a pre-stream rejection to its HTTP status and client-facing body. + * `retryAfter` (seconds) is carried structurally rather than embedded in the + * copy — the client appends the precise countdown as "(retry in Xs)", so + * baking a number into the sentence would show it twice. + */ +export function preStreamError( + kind: PreStreamErrorKind, + retryAfter?: number, +): { status: number; body: AiErrorBody } { + const { status, error } = PRE_STREAM_ERRORS[kind]; + return { + status, + body: { code: kind, error, ...(retryAfter !== undefined ? { retryAfter } : {}) }, + }; +} + +// Reads the `retry-after` header (seconds) from an upstream error, tolerating +// both a Headers object and a plain record. +function retryAfterSeconds(err: unknown): number | undefined { + const headers = (err as { headers?: unknown }).headers; + let raw: string | null | undefined; + if (headers && typeof (headers as Headers).get === "function") { + raw = (headers as Headers).get("retry-after"); + } else if (headers && typeof headers === "object") { + raw = (headers as Record)["retry-after"]; + } + const n = Number(raw); + return Number.isFinite(n) && n > 0 ? n : undefined; +} + +// Maps an error thrown during the turn to a client-facing SSE error. Upstream +// rate limits and overloads get a clear, actionable message (and retry timing) +// instead of an opaque failure. +export function describeTurnError(err: unknown): { + code: string; + message: string; + retryAfter?: number; +} { + const status = (err as { status?: number }).status; + if (status === 429) { + return { + code: "rate_limit", + message: "The AI service is rate limited. Please wait a moment and try again.", + retryAfter: retryAfterSeconds(err), + }; + } + if (status === 502 || status === 503 || status === 529) { + return { + code: "overloaded", + message: "The AI service is temporarily overloaded. Please try again shortly.", + }; + } + return { code: "server_error", message: (err as Error).message || "The request failed." }; +} diff --git a/api/_lib/aiHandler.test.ts b/api/_lib/aiHandler.test.ts index 6187d674..1d0e9293 100644 --- a/api/_lib/aiHandler.test.ts +++ b/api/_lib/aiHandler.test.ts @@ -317,4 +317,33 @@ describe("runAiTurn", () => { ]); expect(handoff!.data.serverToolResults[0] as any).toMatchObject({ tool_call_id: "c1" }); }); + + it("announces each server tool with a status event before the turn completes", async () => { + const { sink, events } = collectingSink(); + const client = fakeClient([ + { + message: { + role: "assistant", + content: null, + tool_calls: [ + { + id: "s1", + type: "function", + function: { name: "read_skill_reference", arguments: '{"name":"patterns"}' }, + }, + ], + }, + finish_reason: "tool_calls", + }, + { message: { role: "assistant", content: "done" }, finish_reason: "stop" }, + ]); + await runAiTurn(req, { ...deps, client }, sink); + + // Server tools run entirely server-side, so without this event the client + // sees no activity at all while they execute. + const statusIdx = events.findIndex((e) => e.event === "status"); + expect(statusIdx).toBeGreaterThanOrEqual(0); + expect(events[statusIdx].data.label).toBe("Reading design references"); + expect(statusIdx).toBeLessThan(events.findIndex((e) => e.event === "done")); + }); }); diff --git a/api/_lib/aiHandler.ts b/api/_lib/aiHandler.ts index 5e41f9ae..a3657873 100644 --- a/api/_lib/aiHandler.ts +++ b/api/_lib/aiHandler.ts @@ -84,17 +84,18 @@ export async function runAiTurn( (o) => !SERVER_TOOL_NAMES.has(o.call.function.name), ); - const serverToolResults: OpenRouterMessage[] = [ - ...malformedResults, - ...serverOutcomes.map((o) => { - const tool = SERVER_TOOLS.find((t) => t.name === o.call.function.name)!; - return { - role: "tool" as const, - tool_call_id: o.call.id, - content: tool.execute(o.parsed.value), - }; - }), - ]; + const serverToolResults: OpenRouterMessage[] = [...malformedResults]; + for (const o of serverOutcomes) { + const tool = SERVER_TOOLS.find((t) => t.name === o.call.function.name)!; + // Announce before executing: a server-tool-only round trip emits no + // handoff, so this is the client's only signal that work is happening. + sink.send("status", { label: tool.progressLabel }); + serverToolResults.push({ + role: "tool", + tool_call_id: o.call.id, + content: tool.execute(o.parsed.value), + }); + } if (clientOutcomes.length > 0 || malformedClientToolCalls.length > 0) { sink.send("handoff", { diff --git a/api/_lib/serverTools.ts b/api/_lib/serverTools.ts index 3051d565..4f489627 100644 --- a/api/_lib/serverTools.ts +++ b/api/_lib/serverTools.ts @@ -4,6 +4,10 @@ export interface ServerTool { name: string; description: string; input_schema: Record; + // Present-progressive label shown in the assistant's live status line while + // this tool runs. Server tools produce no client-side activity chip, so this + // is the only progress the user sees for them. + progressLabel: string; execute(input: unknown): string; } @@ -30,6 +34,7 @@ const readSkillReference: ServerTool = { }, required: ["name"], }, + progressLabel: "Reading design references", execute(input) { const name = (input as { name?: string })?.name; if (!name || !VALID_REFERENCES.includes(name as SkillReferenceName)) { diff --git a/api/ai.ts b/api/ai.ts index 5cc97f51..63fa4672 100644 --- a/api/ai.ts +++ b/api/ai.ts @@ -1,6 +1,7 @@ import type { VercelRequest, VercelResponse } from "@vercel/node"; import z from "zod"; +import { describeTurnError, preStreamError, type PreStreamErrorKind } from "./_lib/aiErrors.js"; import { runAiTurn } from "./_lib/aiHandler.js"; import { bootstrapAiRoute } from "./_lib/aiRoute.js"; import type { OpenRouterLike } from "./_lib/openrouterClient.js"; @@ -8,6 +9,10 @@ import { createLimiter } from "./_lib/ratelimit.js"; import { AiRequestSchema } from "./_lib/schema.js"; import type { SseSink } from "./_lib/sse.js"; +// Re-exported so the route module stays the public entry point for error +// description (api/ai.test.ts imports it from here). +export { describeTurnError }; + function posIntEnv(value: string | undefined, fallback: number): number { const n = Number(value); return Number.isFinite(n) && n > 0 ? n : fallback; @@ -24,25 +29,28 @@ export async function handleAiRequest(args: { }): Promise { const { method, body, ip, env, client, sink, respondError } = args; - if (method !== "POST") return respondError(405, { error: "Method not allowed" }); - if ((env.AI_ENABLED ?? "true") === "false") { - return respondError(503, { error: "AI assistant is disabled" }); - } + // All client-facing failure copy comes from aiErrors so the wording stays + // consistent with the mid-stream errors described by describeTurnError. + const fail = (kind: PreStreamErrorKind, retryAfter?: number) => { + const { status, body: errorBody } = preStreamError(kind, retryAfter); + respondError(status, errorBody); + }; + + if (method !== "POST") return fail("method_not_allowed"); + if ((env.AI_ENABLED ?? "true") === "false") return fail("disabled"); if (!env.OPENROUTER_API_KEY) { console.error( "[api/ai] OPENROUTER_API_KEY is not set — returning 500. Set it in .env (dev) or the Vercel env (prod).", ); - return respondError(500, { error: "Server configuration error" }); + return fail("server_config"); } const { data, error } = z.safeParse(AiRequestSchema, body); - if (error) return respondError(400, { error: "Invalid request body" }); + if (error) return fail("bad_request"); const limiter = createLimiter(env); const rl = await limiter.limit(ip); - if (!rl.ok) { - return respondError(429, { error: "Rate limit exceeded", retryAfter: rl.retryAfterSeconds }); - } + if (!rl.ok) return fail("rate_limit", rl.retryAfterSeconds); try { await runAiTurn( @@ -62,45 +70,6 @@ export async function handleAiRequest(args: { } } -// Reads the `retry-after` header (seconds) from an upstream error, tolerating -// both a Headers object and a plain record. -function retryAfterSeconds(err: unknown): number | undefined { - const headers = (err as { headers?: unknown }).headers; - let raw: string | null | undefined; - if (headers && typeof (headers as Headers).get === "function") { - raw = (headers as Headers).get("retry-after"); - } else if (headers && typeof headers === "object") { - raw = (headers as Record)["retry-after"]; - } - const n = Number(raw); - return Number.isFinite(n) && n > 0 ? n : undefined; -} - -// Maps an error thrown during the turn to a client-facing SSE error. Upstream -// rate limits and overloads get a clear, actionable message (and retry timing) -// instead of an opaque failure. -export function describeTurnError(err: unknown): { - code: string; - message: string; - retryAfter?: number; -} { - const status = (err as { status?: number }).status; - if (status === 429) { - return { - code: "rate_limit", - message: "The AI service is rate limited. Please wait a moment and try again.", - retryAfter: retryAfterSeconds(err), - }; - } - if (status === 502 || status === 503 || status === 529) { - return { - code: "overloaded", - message: "The AI service is temporarily overloaded. Please try again shortly.", - }; - } - return { code: "server_error", message: (err as Error).message || "The request failed." }; -} - function clientIp(req: VercelRequest): string { const realIp = req.headers["x-real-ip"]; if (typeof realIp === "string" && realIp.trim()) return realIp.trim(); diff --git a/src/components/panels/assistant/AssistantMessage.tsx b/src/components/panels/assistant/AssistantMessage.tsx index a47e9519..d7429a8b 100644 --- a/src/components/panels/assistant/AssistantMessage.tsx +++ b/src/components/panels/assistant/AssistantMessage.tsx @@ -89,8 +89,10 @@ export function AssistantMessage({ {message.state === "error" && (
+ {/* The destructive styling and icon already signal failure; the + message itself is a complete, actionable sentence. */} - Failed — {message.errorText ?? "something went wrong"} + {message.errorText ?? "Something went wrong. Please try again."}
)} diff --git a/src/components/panels/assistant/MessageList.tsx b/src/components/panels/assistant/MessageList.tsx index 31da3bd3..07312779 100644 --- a/src/components/panels/assistant/MessageList.tsx +++ b/src/components/panels/assistant/MessageList.tsx @@ -1,7 +1,7 @@ import { Loader2 } from "lucide-react"; import { useEffect, useRef } from "react"; -import type { DisplayMessage } from "../../../services/assistant/store"; +import { type DisplayMessage, useAssistantStore } from "../../../services/assistant/store"; import type { LocalParseService } from "../../../services/localparse"; import { AssistantMessage } from "./AssistantMessage"; @@ -17,6 +17,7 @@ export function MessageList({ busy: boolean; localParseService: LocalParseService; }) { + const activity = useAssistantStore((s) => s.activity); const endRef = useRef(null); useEffect(() => { // Some environments' scrollIntoView returns a Promise; an effect must @@ -26,10 +27,14 @@ export function MessageList({ }, [messages, busy]); const last = messages.length > 0 ? messages[messages.length - 1] : undefined; - const working = + // A specific activity ("Editing document", "Reading design references") always + // wins; the generic fallback covers the model's own thinking/streaming time, + // during which no tool is running. + const fallback = last?.role === "assistant" && !last.text && last.toolActivity.length === 0 ? "Thinking…" : "Working…"; + const working = activity ?? fallback; return (
diff --git a/src/components/panels/assistant/ToolActivityChip.tsx b/src/components/panels/assistant/ToolActivityChip.tsx index 6a5b7598..42440a51 100644 --- a/src/components/panels/assistant/ToolActivityChip.tsx +++ b/src/components/panels/assistant/ToolActivityChip.tsx @@ -1,26 +1,35 @@ +import { Loader2 } from "lucide-react"; + import { cn } from "@/lib/utils"; +import type { ToolActivity } from "../../../services/assistant/store"; import { TOOL_DISPLAY } from "../../../services/assistant/tools"; -export function ToolActivityChip({ - activity, -}: { - activity: { name: string; summary: string; ok: boolean }; -}) { +export function ToolActivityChip({ activity }: { activity: ToolActivity }) { const display = TOOL_DISPLAY[activity.name]; const label = display?.label ?? activity.name; + const running = activity.status === "running"; + // Only an explicit "error" renders as a failure: chips persisted before + // `status` replaced the old `ok` flag have no status at all, and must not be + // mistaken for failures. + const failed = activity.status === "error"; return (
- {display?.icon ?? "•"} - {activity.summary} + {running ? ( + + ) : ( + {display?.icon ?? "•"} + )} + {/* While running there is no summary yet, so show what is being done. */} + {running ? label : activity.summary}
); } diff --git a/src/services/assistant/controller.test.ts b/src/services/assistant/controller.test.ts index 43705d78..fb6c8c40 100644 --- a/src/services/assistant/controller.test.ts +++ b/src/services/assistant/controller.test.ts @@ -26,7 +26,9 @@ const baseDeps = { ctx, getState: () => ({ schema: "", relationships: "", assertions: "", expected: "" }), onText: vi.fn(), - onToolActivity: vi.fn(), + onToolStart: vi.fn(), + onToolEnd: vi.fn(), + onStatus: vi.fn(), onArtifact: vi.fn(), }; @@ -285,7 +287,8 @@ describe("runAssistantTurn", () => { }); it("reports tool activity for a malformed client tool call without executing anything", async () => { - const onToolActivity = vi.fn(); + const onToolStart = vi.fn(); + const onToolEnd = vi.fn(); const registry = new ToolRegistry(); const execute = vi.fn(); registry.register({ @@ -342,15 +345,195 @@ describe("runAssistantTurn", () => { await runAssistantTurn([{ role: "user", content: "check it" }], { ...baseDeps, registry, - onToolActivity, + onToolStart, + onToolEnd, stream: stream as any, }); expect(execute).not.toHaveBeenCalled(); - expect(onToolActivity).toHaveBeenCalledWith({ + // Malformed calls still get a guaranteed start/end pair, same as executed ones. + expect(onToolStart).toHaveBeenCalledWith({ id: "c1", name: "run_check" }); + expect(onToolEnd).toHaveBeenCalledWith({ + id: "c1", name: "run_check", summary: "malformed arguments", ok: false, }); }); + + it("brackets a client tool with a start and an end sharing the call id", async () => { + const onToolStart = vi.fn(); + const onToolEnd = vi.fn(); + const registry = new ToolRegistry(); + registry.register({ + name: "run_check", + description: "d", + parameters: z.object({ resource: z.string() }), + execute: () => ({ ok: true }), + summarize: () => "allowed", + }); + + const stream = gen([ + { + event: "handoff", + data: { + assistantMessage: { + role: "assistant", + content: null, + tool_calls: [ + { + id: "t1", + type: "function", + function: { name: "run_check", arguments: '{"resource":"doc:x"}' }, + }, + ], + }, + serverToolResults: [], + clientToolCalls: [{ id: "t1", name: "run_check", input: { resource: "doc:x" } }], + }, + }, + ]); + + await runAssistantTurn([{ role: "user", content: "check" }], { + ...baseDeps, + registry, + maxRoundTrips: 1, + onToolStart, + onToolEnd, + stream: stream as any, + }); + + expect(onToolStart).toHaveBeenCalledWith({ id: "t1", name: "run_check" }); + expect(onToolEnd).toHaveBeenCalledWith({ + id: "t1", + name: "run_check", + summary: "allowed", + ok: true, + }); + }); + + it("yields to the event loop between starting a tool and executing it", async () => { + // Every client tool is synchronous. Without a yield, onToolStart and + // tool.execute run in one macrotask, so React batches the start and end + // store writes and the in-progress chip never paints. Assert a macrotask + // boundary actually separates them. + const events: string[] = []; + const registry = new ToolRegistry(); + registry.register({ + name: "run_check", + description: "d", + parameters: z.object({}), + execute: () => { + events.push("execute"); + return { ok: true }; + }, + }); + + const stream = gen([ + { + event: "handoff", + data: { + assistantMessage: { role: "assistant", content: null }, + serverToolResults: [], + clientToolCalls: [{ id: "t1", name: "run_check", input: {} }], + }, + }, + ]); + + await runAssistantTurn([{ role: "user", content: "check" }], { + ...baseDeps, + registry, + maxRoundTrips: 1, + onToolStart: () => { + events.push("start"); + // Queued before the controller's own yield, so it must run first — and + // it can only run at all if the controller gives up the macrotask. + setTimeout(() => events.push("macrotask"), 0); + }, + stream: stream as any, + }); + + // Without the yield, execute() runs in the same macrotask as the start + // callback and this reads ["start", "execute", "macrotask"]. + expect(events).toEqual(["start", "macrotask", "execute"]); + }); + + it("still ends the tool when it is unknown to the registry", async () => { + const onToolStart = vi.fn(); + const onToolEnd = vi.fn(); + const stream = gen([ + { + event: "handoff", + data: { + assistantMessage: { role: "assistant", content: null }, + serverToolResults: [], + clientToolCalls: [{ id: "z9", name: "nope", input: {} }], + }, + }, + ]); + + await runAssistantTurn([{ role: "user", content: "go" }], { + ...baseDeps, + maxRoundTrips: 1, + onToolStart, + onToolEnd, + stream: stream as any, + }); + + // A start with no matching end would leave a chip spinning forever. + expect(onToolStart).toHaveBeenCalledWith({ id: "z9", name: "nope" }); + expect(onToolEnd).toHaveBeenCalledWith({ + id: "z9", + name: "nope", + summary: "unknown tool", + ok: false, + }); + }); + + it("forwards a status event and clears it once text resumes", async () => { + const onStatus = vi.fn(); + const stream = gen([ + { event: "status", data: { label: "Reading design references" } }, + { event: "text", data: { delta: "ok" } }, + { + event: "done", + data: { assistantMessage: { role: "assistant", content: "ok" }, finish_reason: "stop" }, + }, + ]); + + await runAssistantTurn([{ role: "user", content: "hi" }], { + ...baseDeps, + onStatus, + stream: stream as any, + }); + + expect(onStatus).toHaveBeenNthCalledWith(1, "Reading design references"); + expect(onStatus).toHaveBeenNthCalledWith(2, null); + // A spurious extra clear (e.g. one at loop-end and another in a finally) + // would slip past the two positional assertions above without this. + expect(onStatus).toHaveBeenCalledTimes(2); + }); + + it("clears a stuck status label when the stream throws mid-iteration", async () => { + const onStatus = vi.fn(); + class NetworkError extends Error {} + const stream = () => + // eslint-disable-next-line require-yield -- intentionally throws after one yield + (async function* () { + yield { event: "status", data: { label: "Reading design references" } } as SseEvent; + throw new NetworkError("connection reset"); + })(); + + const result = await runAssistantTurn([{ role: "user", content: "hi" }], { + ...baseDeps, + onStatus, + stream: stream as any, + }); + + // The stream never resumed with text/done, so without a finally the + // status label set above would be left stuck in the UI forever. + expect(onStatus).toHaveBeenCalledWith(null); + expect(onStatus).toHaveBeenCalledTimes(2); + expect(result.error?.message).toMatch(/connection reset/i); + }); }); diff --git a/src/services/assistant/controller.ts b/src/services/assistant/controller.ts index bc11338b..9dfaefd6 100644 --- a/src/services/assistant/controller.ts +++ b/src/services/assistant/controller.ts @@ -28,7 +28,13 @@ export interface TurnDeps { getState: () => StateSnapshot; maxRoundTrips?: number; onText: (delta: string) => void; - onToolActivity: (a: { name: string; summary: string; ok: boolean }) => void; + // Tool activity is bracketed so the UI can show an in-progress chip for the + // duration of the call rather than only after it resolves. `id` is the tool + // call id and correlates the two. + onToolStart: (a: { id: string; name: string }) => void; + onToolEnd: (a: { id: string; name: string; summary: string; ok: boolean }) => void; + // Transient "what's happening now" label; null clears it. + onStatus: (label: string | null) => void; onArtifact: (artifact: DisplayArtifact) => void; } @@ -58,6 +64,14 @@ export async function runAssistantTurn( let handoff: Extract["data"] | null = null; let doneErr: TurnResult["error"] | undefined; let finished = false; + // A server-tool status label applies only until the model produces output + // again — tracked so it can be cleared exactly once. + let statusActive = false; + const clearStatus = () => { + if (!statusActive) return; + statusActive = false; + deps.onStatus(null); + }; paragraphBreaks.nextTrip(); try { @@ -67,18 +81,30 @@ export async function runAssistantTurn( tools: deps.registry.toWire(), }); - for await (const ev of stream) { - if (ev.event === "text") { - deps.onText(paragraphBreaks.apply(ev.data.delta)); - } else if (ev.event === "done") { - messages.push(ev.data.assistantMessage); - finished = true; - } else if (ev.event === "error") { - doneErr = { message: ev.data.message, retryAfter: ev.data.retryAfter }; - finished = true; - } else if (ev.event === "handoff") { - handoff = ev.data; + try { + for await (const ev of stream) { + if (ev.event === "text") { + clearStatus(); + deps.onText(paragraphBreaks.apply(ev.data.delta)); + } else if (ev.event === "status") { + statusActive = true; + deps.onStatus(ev.data.label); + } else if (ev.event === "done") { + messages.push(ev.data.assistantMessage); + finished = true; + } else if (ev.event === "error") { + doneErr = { message: ev.data.message, retryAfter: ev.data.retryAfter }; + finished = true; + } else if (ev.event === "handoff") { + handoff = ev.data; + } } + } finally { + // Runs on every exit from the loop above — normal completion, an + // AbortError from the user clicking Stop, or any other stream + // failure — so a server-tool status label is never left stuck in + // the UI when the stream doesn't resume with text/done. + clearStatus(); } } catch (err) { if ((err as Error).name === "AbortError") { @@ -101,7 +127,10 @@ export async function runAssistantTurn( messages.push(...handoff.serverToolResults); for (const call of handoff.malformedClientToolCalls ?? []) { - deps.onToolActivity({ name: call.name, summary: "malformed arguments", ok: false }); + // Never executed, but still bracketed so the UI has one uniform path + // from start to resolution for every call it displays. + deps.onToolStart({ id: call.id, name: call.name }); + deps.onToolEnd({ id: call.id, name: call.name, summary: "malformed arguments", ok: false }); } for (const call of handoff.clientToolCalls) { @@ -112,16 +141,30 @@ export async function runAssistantTurn( return { messages, error: { message: "Reached the step limit for this turn." } }; } +// Defers to the next macrotask so the browser can paint work queued by the +// callback that ran just before it. `setTimeout` rather than +// requestAnimationFrame: rAF fires *before* paint (so synchronous work in its +// callback still blocks the frame), and it doesn't exist under Node in tests. +function yieldToPaint(): Promise { + return new Promise((resolve) => setTimeout(resolve, 0)); +} + async function executeClientTool(call: ClientToolCall, deps: TurnDeps): Promise { + // Announced before any validation so every displayed call — including the + // ones rejected below — has a start that is guaranteed a matching end. + deps.onToolStart({ id: call.id, name: call.name }); + const end = (summary: string, ok: boolean) => + deps.onToolEnd({ id: call.id, name: call.name, summary, ok }); + const tool = deps.registry.get(call.name); if (!tool) { - deps.onToolActivity({ name: call.name, summary: "unknown tool", ok: false }); + end("unknown tool", false); return { role: "tool", tool_call_id: call.id, content: `Unknown tool "${call.name}".` }; } const parsed = tool.parameters.safeParse(call.input); if (!parsed.success) { - deps.onToolActivity({ name: call.name, summary: "invalid input", ok: false }); + end("invalid input", false); return { role: "tool", tool_call_id: call.id, @@ -130,6 +173,11 @@ async function executeClientTool(call: ClientToolCall, deps: TurnDeps): Promise< } try { + // Every client tool is synchronous, so without yielding here the start and + // end callbacks land in the same macrotask: React batches both store writes + // and paints once, with the chip already resolved. Handing the event loop a + // turn lets the in-progress chip and status label actually render. + await yieldToPaint(); const result = await tool.execute(parsed.data, deps.ctx); const ok = tool.isError ? !tool.isError(result) : (result as { ok?: boolean }).ok !== false; @@ -144,11 +192,7 @@ async function executeClientTool(call: ClientToolCall, deps: TurnDeps): Promise< ? omitKeys(result, tool.redactFromModel) : result; - deps.onToolActivity({ - name: call.name, - summary: tool.summarize ? tool.summarize(result, parsed.data) : defaultSummarize(result), - ok, - }); + end(tool.summarize ? tool.summarize(result, parsed.data) : defaultSummarize(result), ok); // OpenAI's tool-role messages have no dedicated failure flag (unlike // Anthropic's is_error on tool_result blocks) — prefix the content so a // failing tool call is unambiguous to the model even when the tool's own @@ -156,7 +200,7 @@ async function executeClientTool(call: ClientToolCall, deps: TurnDeps): Promise< const content = JSON.stringify(modelResult); return { role: "tool", tool_call_id: call.id, content: ok ? content : `Error: ${content}` }; } catch (err) { - deps.onToolActivity({ name: call.name, summary: "error", ok: false }); + end("error", false); return { role: "tool", tool_call_id: call.id, diff --git a/src/services/assistant/store.test.ts b/src/services/assistant/store.test.ts index 8e23ab4c..ebe4b386 100644 --- a/src/services/assistant/store.test.ts +++ b/src/services/assistant/store.test.ts @@ -36,4 +36,17 @@ describe("useAssistantStore", () => { useAssistantStore.getState().reset(); expect(useAssistantStore.getState().pendingPrompt).toBeNull(); }); + + it("tracks the transient activity label", () => { + useAssistantStore.getState().setActivity("Editing document"); + expect(useAssistantStore.getState().activity).toBe("Editing document"); + useAssistantStore.getState().setActivity(null); + expect(useAssistantStore.getState().activity).toBeNull(); + }); + + it("reset clears the activity label", () => { + useAssistantStore.getState().setActivity("Running check"); + useAssistantStore.getState().reset(); + expect(useAssistantStore.getState().activity).toBeNull(); + }); }); diff --git a/src/services/assistant/store.ts b/src/services/assistant/store.ts index f2bf4b0e..04cc2cdf 100644 --- a/src/services/assistant/store.ts +++ b/src/services/assistant/store.ts @@ -5,10 +5,15 @@ import type { ChatMessage, DisplayArtifact } from "./types"; export type AssistantStatus = "idle" | "streaming" | "executing_tools" | "error"; +export type ToolActivityStatus = "running" | "ok" | "error"; + export interface ToolActivity { + // The tool call id — correlates the in-progress chip with its completion. + id?: string; name: string; + // Empty while running; the tool's own summary once resolved. summary: string; - ok: boolean; + status: ToolActivityStatus; } export interface DisplayMessage { id: string; @@ -26,6 +31,9 @@ export interface AssistantState { display: DisplayMessage[]; status: AssistantStatus; error?: { message: string; retryAfter?: number }; + // What the assistant is doing right now ("Editing document"), shown in the + // live status line. Transient: never persisted, cleared on reset. + activity: string | null; // Bumped by reset() so an in-flight turn's async continuation can tell a // reset happened mid-turn and skip resurrecting stale results into the store. generation: number; @@ -36,6 +44,7 @@ export interface AssistantState { pendingPrompt: string | null; reset: () => void; setStatus: (s: AssistantStatus, error?: AssistantState["error"]) => void; + setActivity: (activity: string | null) => void; appendUser: (text: string) => void; setMessages: (m: ChatMessage[]) => void; setDisplay: (d: DisplayMessage[]) => void; @@ -52,6 +61,7 @@ export const useAssistantStore = create()( display: [], status: "idle", error: undefined, + activity: null, generation: 0, pendingPrompt: null, reset: () => @@ -60,10 +70,12 @@ export const useAssistantStore = create()( display: [], status: "idle", error: undefined, + activity: null, pendingPrompt: null, generation: s.generation + 1, })), setStatus: (status, error) => set({ status, error }), + setActivity: (activity) => set({ activity }), appendUser: (text) => set((s) => ({ messages: [...s.messages, { role: "user", content: text }], diff --git a/src/services/assistant/streamClient.test.ts b/src/services/assistant/streamClient.test.ts index 5114146f..0f341ed3 100644 --- a/src/services/assistant/streamClient.test.ts +++ b/src/services/assistant/streamClient.test.ts @@ -71,4 +71,23 @@ describe("streamAssistant", () => { events.push(e); expect(events.map((e) => e.event)).toEqual(["done"]); }); + + it("falls back to friendly copy when a non-200 carries no JSON body", async () => { + // A CDN/proxy 503 never reaches our route, so there is no server-composed + // `error` field to surface. + const fetchImpl = async () => new Response("gateway", { status: 503 }); + await expect(async () => { + for await (const _ of streamAssistant(req, fetchImpl as unknown as typeof fetch)) void _; + }).rejects.toThrow(/temporarily unavailable/i); + }); + + it("prefers the server-composed message over the fallback", async () => { + const fetchImpl = async () => + new Response(JSON.stringify({ code: "disabled", error: "The AI assistant is turned off." }), { + status: 503, + }); + await expect(async () => { + for await (const _ of streamAssistant(req, fetchImpl as unknown as typeof fetch)) void _; + }).rejects.toThrow(/turned off/i); + }); }); diff --git a/src/services/assistant/streamClient.ts b/src/services/assistant/streamClient.ts index 0696e6a2..7974daa3 100644 --- a/src/services/assistant/streamClient.ts +++ b/src/services/assistant/streamClient.ts @@ -17,6 +17,23 @@ export class RateLimitError extends Error { } } +// Non-200s that never reached our route — CDN/proxy 5xx, gateway timeouts — +// carry no server-composed copy, so map the bare status to something a user can +// act on rather than surfacing a naked status code. +function fallbackErrorMessage(status: number): string { + if (status === 429) { + return "You're sending messages too quickly. Please wait a moment and try again."; + } + if (status === 504) return "The AI service took too long to respond. Please try again."; + if (status >= 500) { + return "The AI service is temporarily unavailable. Please try again in a moment."; + } + if (status === 404) { + return "The AI service could not be reached. Please reload the page and try again."; + } + return `The request failed (${status}). Please try again.`; +} + export async function* streamAssistant( req: StreamRequest, fetchImpl: typeof fetch = fetch, @@ -35,9 +52,11 @@ export async function* streamAssistant( } catch { /* ignore */ } - if (res.status === 429) - throw new RateLimitError(payload.error ?? "Rate limit exceeded", payload.retryAfter); - throw new Error(payload.error ?? `Request failed (${res.status})`); + // Server-composed copy (api/_lib/aiErrors.ts) is authoritative; the + // fallback only covers responses that never reached our route. + const message = payload.error ?? fallbackErrorMessage(res.status); + if (res.status === 429) throw new RateLimitError(message, payload.retryAfter); + throw new Error(message); } const reader = res.body.getReader(); diff --git a/src/services/assistant/tools/checkWatches.ts b/src/services/assistant/tools/checkWatches.ts index dca42489..9b837944 100644 --- a/src/services/assistant/tools/checkWatches.ts +++ b/src/services/assistant/tools/checkWatches.ts @@ -49,6 +49,7 @@ export const listCheckWatchesTool: AssistantTool, ListChec }, icon: "📋", label: "List check watches", + progressLabel: "Reading check watches", }; interface AddCheckWatchResult { @@ -76,6 +77,7 @@ export const addCheckWatchTool: AssistantTool< summarize: (result) => `watch added ⟹ ${result.current_result}`, icon: "📌", label: "Add check watch", + progressLabel: "Adding check watch", }; interface UpdateCheckWatchResult { @@ -112,6 +114,7 @@ export const updateCheckWatchTool: AssistantTool< summarize: (result) => (result.ok ? "watch updated" : (result.error ?? "failed")), icon: "✏️", label: "Update check watch", + progressLabel: "Updating check watch", }; interface RemoveCheckWatchResult { @@ -133,4 +136,5 @@ export const removeCheckWatchTool: AssistantTool<{ watch_id: string }, RemoveChe summarize: (result) => (result.ok ? "watch removed" : (result.error ?? "failed")), icon: "🗑", label: "Remove check watch", + progressLabel: "Removing check watch", }; diff --git a/src/services/assistant/tools/editDocument.ts b/src/services/assistant/tools/editDocument.ts index ed974fde..5dea5e82 100644 --- a/src/services/assistant/tools/editDocument.ts +++ b/src/services/assistant/tools/editDocument.ts @@ -105,4 +105,5 @@ export const editDocumentTool: AssistantTool, Explai summarize: (result) => `check ⟹ ${result.result}`, icon: "🔎", label: "Explain check", + progressLabel: "Explaining check", }; diff --git a/src/services/assistant/tools/index.test.ts b/src/services/assistant/tools/index.test.ts index 5fb153e9..56793335 100644 --- a/src/services/assistant/tools/index.test.ts +++ b/src/services/assistant/tools/index.test.ts @@ -1,6 +1,6 @@ import { describe, expect, it } from "vitest"; -import { CLIENT_TOOL_NAMES, buildDefaultRegistry } from "./index"; +import { CLIENT_TOOL_NAMES, TOOL_DISPLAY, buildDefaultRegistry } from "./index"; describe("buildDefaultRegistry", () => { it("registers all nine client tools", () => { @@ -24,3 +24,17 @@ describe("buildDefaultRegistry", () => { expect(CLIENT_TOOL_NAMES).toContain("edit_document"); }); }); + +describe("TOOL_DISPLAY", () => { + it("exposes a present-progressive progress label for every client tool", () => { + for (const name of CLIENT_TOOL_NAMES) { + expect(TOOL_DISPLAY[name].progress).toMatch(/ing\b/); + } + }); + + it("keeps the imperative label distinct from the progress label", () => { + // The chip shows the imperative form; the status line shows the gerund. + expect(TOOL_DISPLAY.edit_document.label).toBe("Edit document"); + expect(TOOL_DISPLAY.edit_document.progress).toBe("Editing document"); + }); +}); diff --git a/src/services/assistant/tools/index.ts b/src/services/assistant/tools/index.ts index 4f4237f4..8351dad9 100644 --- a/src/services/assistant/tools/index.ts +++ b/src/services/assistant/tools/index.ts @@ -28,9 +28,17 @@ export const CLIENT_TOOL_NAMES = ALL_CLIENT_TOOLS.map((t) => t.name); // Chat UI display metadata (icon + tooltip label), declared per tool above — // consumed by ToolActivityChip so it needs no per-tool name checks either. -export const TOOL_DISPLAY: Record = Object.fromEntries( - ALL_CLIENT_TOOLS.map((t) => [t.name, { icon: t.icon ?? "•", label: t.label ?? t.name }]), -); +export const TOOL_DISPLAY: Record = + Object.fromEntries( + ALL_CLIENT_TOOLS.map((t) => [ + t.name, + { + icon: t.icon ?? "•", + label: t.label ?? t.name, + progress: t.progressLabel ?? t.label ?? t.name, + }, + ]), + ); export function buildDefaultRegistry(): ToolRegistry { const registry = new ToolRegistry(); diff --git a/src/services/assistant/tools/openTabToLine.ts b/src/services/assistant/tools/openTabToLine.ts index cc3a9069..b7f9a8e3 100644 --- a/src/services/assistant/tools/openTabToLine.ts +++ b/src/services/assistant/tools/openTabToLine.ts @@ -29,4 +29,5 @@ export const openTabToLineTool: AssistantTool> = { summarize: (_result, input) => `opened ${input.target}:${input.line}`, icon: "↪", label: "Open tab", + progressLabel: "Opening editor", }; diff --git a/src/services/assistant/tools/runCheck.ts b/src/services/assistant/tools/runCheck.ts index ddd05aeb..3fd4093f 100644 --- a/src/services/assistant/tools/runCheck.ts +++ b/src/services/assistant/tools/runCheck.ts @@ -24,4 +24,5 @@ export const runCheckTool: AssistantTool, CheckRunRe summarize: (result) => `check ⟹ ${result.result}`, icon: "🔍", label: "Run check", + progressLabel: "Running check", }; diff --git a/src/services/assistant/tools/runValidation.ts b/src/services/assistant/tools/runValidation.ts index a10eefa2..68391ac9 100644 --- a/src/services/assistant/tools/runValidation.ts +++ b/src/services/assistant/tools/runValidation.ts @@ -18,4 +18,5 @@ export const runValidationTool: AssistantTool, Valid summarize: (result) => (result.passed ? "validation passed" : "validation failed"), icon: "✅", label: "Run validation", + progressLabel: "Running validation", }; diff --git a/src/services/assistant/types.ts b/src/services/assistant/types.ts index f1481dc8..6bc715ed 100644 --- a/src/services/assistant/types.ts +++ b/src/services/assistant/types.ts @@ -60,6 +60,10 @@ export interface AssistantTool { // and the raw tool name. icon?: string; label?: string; + // Present-progressive form shown in the live status line while the tool runs + // ("Editing document"), as opposed to `label`'s imperative ("Edit document"). + // Falls back to `label`. + progressLabel?: string; } export interface WireTool { @@ -95,4 +99,10 @@ export type SseEvent = }; } | { event: "done"; data: { assistantMessage: AssistantMessage; finish_reason: string } } + | { + // Server-side progress, e.g. a server tool starting. Purely transient UI + // signal — never part of the conversation sent back to the model. + event: "status"; + data: { label: string }; + } | { event: "error"; data: { code?: string; message: string; retryAfter?: number } }; diff --git a/src/services/assistant/useAssistantController.ts b/src/services/assistant/useAssistantController.ts index 362fb7bf..febe5d74 100644 --- a/src/services/assistant/useAssistantController.ts +++ b/src/services/assistant/useAssistantController.ts @@ -11,7 +11,7 @@ import type { Services } from "../services"; import { runAssistantTurn, type StateSnapshot } from "./controller"; import { useAssistantStore } from "./store"; import { streamAssistant } from "./streamClient"; -import { buildDefaultRegistry } from "./tools"; +import { TOOL_DISPLAY, buildDefaultRegistry } from "./tools"; import type { HistoryRecorder, ToolContext } from "./types"; export function useAssistantController( @@ -104,6 +104,14 @@ export function useAssistantController( s.setDisplay(display); }; + // Same staleness guard as patchAssistant: a reset() mid-turn must not let + // this turn's late callbacks write a label into the fresh store. + const setActivity = (label: string | null) => { + const s = useAssistantStore.getState(); + if (s.generation !== myGeneration) return; + s.setActivity(label); + }; + try { const result = await runAssistantTurn(useAssistantStore.getState().messages, { stream: (req) => streamAssistant({ endpoint, ...req, signal: abort.signal }), @@ -112,7 +120,29 @@ export function useAssistantController( getState: readState, maxRoundTrips: 10, onText: (delta) => patchAssistant((m) => (m.text += delta)), - onToolActivity: (a) => patchAssistant((m) => m.toolActivity.push(a)), + onToolStart: ({ id, name }) => { + patchAssistant((m) => + m.toolActivity.push({ id, name, summary: "", status: "running" }), + ); + setActivity(TOOL_DISPLAY[name]?.progress ?? name); + }, + onToolEnd: ({ id, name, summary, ok }) => { + patchAssistant((m) => { + // Resolve the chip this call opened; matching on id keeps + // concurrent or repeated calls to the same tool distinct. + let matched = false; + m.toolActivity = m.toolActivity.map((a) => { + if (matched || a.id !== id || a.status !== "running") return a; + matched = true; + return { ...a, summary, status: ok ? ("ok" as const) : ("error" as const) }; + }); + if (!matched) { + m.toolActivity.push({ id, name, summary, status: ok ? "ok" : "error" }); + } + }); + setActivity(null); + }, + onStatus: (label) => setActivity(label), onArtifact: (artifact) => patchAssistant((m) => m.artifacts.push(artifact)), }); @@ -121,6 +151,7 @@ export function useAssistantController( if (result.aborted) { // The user clicked Stop — stop() already returned status to idle; // just finalize the message with whatever streamed in before the abort. + setActivity(null); patchAssistant((m) => { m.state = "done"; }); @@ -136,10 +167,12 @@ export function useAssistantController( m.state = result.error ? "error" : "done"; m.errorText = errText; }); + setActivity(null); useAssistantStore.getState().setMessages(result.messages); useAssistantStore.getState().setStatus(result.error ? "error" : "idle", result.error); } catch (err) { if (useAssistantStore.getState().generation !== myGeneration) return; + setActivity(null); const message = (err as Error).message; patchAssistant((m) => { m.state = "error"; @@ -153,6 +186,7 @@ export function useAssistantController( const stop = useCallback(() => { abortRef.current?.abort(); + useAssistantStore.getState().setActivity(null); useAssistantStore.getState().setStatus("idle"); }, []); diff --git a/src/tests/browser/assistant-leaves.test.tsx b/src/tests/browser/assistant-leaves.test.tsx index ded12b1c..712bf800 100644 --- a/src/tests/browser/assistant-leaves.test.tsx +++ b/src/tests/browser/assistant-leaves.test.tsx @@ -29,7 +29,9 @@ describe("assistant leaf components", () => { it("renders a tool chip summary", async () => { const screen = await render( - , + , ); await expect.element(screen.getByText(/allowed/)).toBeInTheDocument(); }); diff --git a/src/tests/browser/assistant-message.test.tsx b/src/tests/browser/assistant-message.test.tsx index a4e388a4..b010780b 100644 --- a/src/tests/browser/assistant-message.test.tsx +++ b/src/tests/browser/assistant-message.test.tsx @@ -20,7 +20,7 @@ function assistantMsg(over: Partial): DisplayMessage { }; } -const chip = (summary: string) => ({ name: "edit_document", summary, ok: true }); +const chip = (summary: string) => ({ name: "edit_document", summary, status: "ok" as const }); const diff = (target: string) => ({ kind: "diff" as const, target, before: "a", after: "b" }); describe("AssistantMessage collapse behavior", () => { @@ -69,7 +69,7 @@ describe("AssistantMessage collapse behavior", () => { })} />, ); - await expect.element(screen.getByText(/Failed/)).toBeInTheDocument(); + await expect.element(screen.getByText(/boom/)).toBeInTheDocument(); expect(screen.getByText(/Actions taken/).elements()).toHaveLength(0); }); });