From 36df1fe30e4433e4d829ef9e97a34f98b043d9cf Mon Sep 17 00:00:00 2001 From: Molecule AI Fullstack Engineer Date: Mon, 18 May 2026 03:14:35 +0000 Subject: [PATCH] fix(both): surface actionable error_detail in canvas (#1420) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds error_detail to the live ACTIVITY_LOGGED WebSocket broadcast so the canvas can render an actionable error reason (e.g. oauth_org_not_allowed) instead of the opaque "Agent error (Exception) — see workspace logs for details." dead end. **Server (Go)** - logActivityExec now includes error_detail in the broadcast payload when set (omitted when nil, matching request_body/response_body pattern) - New tests: broadcast includes error_detail / omits when nil **Canvas (TypeScript)** - useChatSocket: error_detail extracted from ACTIVITY_LOGGED payload, passed to onSendError (preference: error_detail > summary > generic) - Old "Agent error (Exception) — see workspace logs for details." removed - New 8 tests covering error_detail/summary/generic/empty/wrong-workspace Co-Authored-By: Claude Opus 4.7 --- .../useChatSocket.errorDetail.test.ts | 204 ++++++++++++++++++ .../tabs/chat/hooks/useChatSocket.ts | 12 +- .../internal/handlers/activity.go | 7 + .../internal/handlers/activity_test.go | 87 ++++++++ 4 files changed, 307 insertions(+), 3 deletions(-) create mode 100644 canvas/src/components/tabs/chat/__tests__/useChatSocket.errorDetail.test.ts diff --git a/canvas/src/components/tabs/chat/__tests__/useChatSocket.errorDetail.test.ts b/canvas/src/components/tabs/chat/__tests__/useChatSocket.errorDetail.test.ts new file mode 100644 index 000000000..6599ae2c4 --- /dev/null +++ b/canvas/src/components/tabs/chat/__tests__/useChatSocket.errorDetail.test.ts @@ -0,0 +1,204 @@ +// @vitest-environment jsdom +/** + * Tests for actionable error rendering in useChatSocket (issue #1420). + * + * When a workspace agent returns an error on a canvas message/send, the canvas + * should surface the actionable error_detail (e.g. oauth_org_not_allowed) + * rather than the opaque "Agent error (Exception) — see workspace logs for details." + * fallback. Falls back to summary, then a generic hint. + */ +import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"; +import { renderHook, act } from "@testing-library/react"; +import React from "react"; +import { useChatSocket, type UseChatSocketCallbacks } from "../hooks/useChatSocket"; +import { emitSocketEvent, _resetSocketEventListenersForTests } from "@/store/socket-events"; +import type { WSMessage } from "@/store/socket"; + +// Silence React StrictMode double-invoke noise. +const WARN = console.warn; +beforeEach(() => { console.warn = () => {}; }); +afterEach(() => { console.warn = WARN; }); + +beforeEach(() => { + _resetSocketEventListenersForTests(); + vi.useFakeTimers(); + vi.setSystemTime(new Date("2026-05-18T10:00:00Z")); +}); +afterEach(() => { + vi.useRealTimers(); + _resetSocketEventListenersForTests(); +}); + +const WORKSPACE_ID = "00000000-0000-0000-0000-000000000001"; + +function makeActivityErrorEvent( + workspaceId: string, + overrides: Partial<{ + error_detail: string; + summary: string; + method: string; + status: string; + }> = {}, +): WSMessage { + const { + error_detail = "", + summary = "", + method = "message/send", + status = "error", + } = overrides; + return { + event: "ACTIVITY_LOGGED", + workspace_id: workspaceId, + timestamp: "2026-05-18T10:00:00Z", + payload: { + activity_type: "a2a_receive", + method, + status, + target_id: workspaceId, + duration_ms: 500, + summary, + ...(error_detail ? { error_detail } : {}), + } as Record, + }; +} + +describe("useChatSocket actionable error rendering", () => { + it("calls onSendError with error_detail when present in the payload", () => { + const onSendError = vi.fn(); + const callbacks: UseChatSocketCallbacks = { onSendError }; + renderHook(() => useChatSocket(WORKSPACE_ID, callbacks)); + + act(() => { + emitSocketEvent( + makeActivityErrorEvent(WORKSPACE_ID, { + error_detail: "403 Forbidden: oauth_org_not_allowed — Your organization has disabled Claude subscription access. Use an API key instead.", + }), + ); + }); + + expect(onSendError).toHaveBeenCalledTimes(1); + expect(onSendError.mock.calls[0][0]).toContain("oauth_org_not_allowed"); + expect(onSendError.mock.calls[0][0]).not.toContain("workspace logs"); + expect(onSendError.mock.calls[0][0]).not.toContain("Agent error (Exception)"); + }); + + it("falls back to summary when error_detail is absent", () => { + const onSendError = vi.fn(); + const callbacks: UseChatSocketCallbacks = { onSendError }; + renderHook(() => useChatSocket(WORKSPACE_ID, callbacks)); + + act(() => { + emitSocketEvent( + makeActivityErrorEvent(WORKSPACE_ID, { + summary: "A2A request to ws-agent failed: connection refused", + }), + ); + }); + + expect(onSendError).toHaveBeenCalledTimes(1); + expect(onSendError.mock.calls[0][0]).toBe("A2A request to ws-agent failed: connection refused"); + }); + + it("falls back to generic hint when neither error_detail nor summary is present", () => { + const onSendError = vi.fn(); + const callbacks: UseChatSocketCallbacks = { onSendError }; + renderHook(() => useChatSocket(WORKSPACE_ID, callbacks)); + + act(() => { + emitSocketEvent(makeActivityErrorEvent(WORKSPACE_ID, {})); + }); + + expect(onSendError).toHaveBeenCalledTimes(1); + expect(onSendError.mock.calls[0][0]).toContain("Agent error"); + // Should NOT be the old opaque phrase + expect(onSendError.mock.calls[0][0]).not.toContain("workspace logs"); + expect(onSendError.mock.calls[0][0]).not.toContain("Agent error (Exception)"); + }); + + it("does NOT call onSendError for other workspaces", () => { + const onSendError = vi.fn(); + const callbacks: UseChatSocketCallbacks = { onSendError }; + renderHook(() => useChatSocket(WORKSPACE_ID, callbacks)); + + act(() => { + emitSocketEvent( + makeActivityErrorEvent("00000000-0000-0000-0000-000000000099", { + error_detail: "some provider error", + }), + ); + }); + + expect(onSendError).not.toHaveBeenCalled(); + }); + + it("does NOT call onSendError for ok status", () => { + const onSendError = vi.fn(); + const callbacks: UseChatSocketCallbacks = { onSendError }; + renderHook(() => useChatSocket(WORKSPACE_ID, callbacks)); + + act(() => { + emitSocketEvent( + makeActivityErrorEvent(WORKSPACE_ID, { + status: "ok", + error_detail: "this should not appear", + }), + ); + }); + + expect(onSendError).not.toHaveBeenCalled(); + }); + + it("does NOT call onSendError when error_detail is an empty string", () => { + const onSendError = vi.fn(); + const callbacks: UseChatSocketCallbacks = { onSendError }; + renderHook(() => useChatSocket(WORKSPACE_ID, callbacks)); + + act(() => { + emitSocketEvent( + makeActivityErrorEvent(WORKSPACE_ID, { + error_detail: "", + summary: "", + }), + ); + }); + + // Empty strings are falsy — falls through to the generic hint + expect(onSendError).toHaveBeenCalledTimes(1); + expect(onSendError.mock.calls[0][0]).toContain("Agent error"); + expect(onSendError.mock.calls[0][0]).not.toContain("workspace logs"); + }); + + it("prefers error_detail over summary (error_detail is more actionable)", () => { + const onSendError = vi.fn(); + const callbacks: UseChatSocketCallbacks = { onSendError }; + renderHook(() => useChatSocket(WORKSPACE_ID, callbacks)); + + act(() => { + emitSocketEvent( + makeActivityErrorEvent(WORKSPACE_ID, { + error_detail: "403: api_key_expired", + summary: "A2A request failed", + }), + ); + }); + + expect(onSendError).toHaveBeenCalledTimes(1); + expect(onSendError.mock.calls[0][0]).toBe("403: api_key_expired"); + }); + + it("does NOT call onSendError when onSendError is undefined (no-op guard)", () => { + const callbacks: UseChatSocketCallbacks = { onAgentMessage: vi.fn() }; + expect(() => + renderHook(() => useChatSocket(WORKSPACE_ID, callbacks)), + ).not.toThrow(); + + act(() => { + emitSocketEvent( + makeActivityErrorEvent(WORKSPACE_ID, { + error_detail: "some error", + }), + ); + }); + // No error thrown even without onSendError + }); +}); diff --git a/canvas/src/components/tabs/chat/hooks/useChatSocket.ts b/canvas/src/components/tabs/chat/hooks/useChatSocket.ts index 15815e9a8..98b165c5d 100644 --- a/canvas/src/components/tabs/chat/hooks/useChatSocket.ts +++ b/canvas/src/components/tabs/chat/hooks/useChatSocket.ts @@ -53,6 +53,7 @@ export function useChatSocket( const targetId = (p.target_id as string) || ""; const durationMs = p.duration_ms as number | undefined; const summary = (p.summary as string) || ""; + const errorDetail = typeof p.error_detail === "string" ? p.error_detail : ""; let line = ""; if (type === "a2a_receive" && method === "message/send") { @@ -67,9 +68,14 @@ export function useChatSocket( const own = (targetId || msg.workspace_id) === workspaceId; if (own) { callbacksRef.current.onSendComplete?.(); - callbacksRef.current.onSendError?.( - "Agent error (Exception) — see workspace logs for details.", - ); + // Prefer the actionable error_detail from the workspace agent + // (e.g. "403 Forbidden: oauth_org_not_allowed ...") over the + // opaque generic. Fall back to a generic hint so the user + // always sees something actionable. Closes #1420. + const displayError = errorDetail + || summary + || "Agent error — please try again or check the agent's configuration."; + callbacksRef.current.onSendError?.(displayError); } } } else if (type === "a2a_send") { diff --git a/workspace-server/internal/handlers/activity.go b/workspace-server/internal/handlers/activity.go index 56dd7a1bb..fddcaea46 100644 --- a/workspace-server/internal/handlers/activity.go +++ b/workspace-server/internal/handlers/activity.go @@ -672,6 +672,13 @@ func logActivityExec(ctx context.Context, exec activityExecutor, broadcaster eve if len(params.ToolTrace) > 0 { payload["tool_trace"] = json.RawMessage(params.ToolTrace) } + // Include error_detail in the live broadcast so the canvas can surface + // an actionable error reason (e.g. oauth_org_not_allowed) instead of the + // opaque "Agent error (Exception)" fallback. The runtime's + // report_activity helper caps this at 4096 chars. + if params.ErrorDetail != nil { + payload["error_detail"] = *params.ErrorDetail + } // Include request/response bodies in the live broadcast so the // canvas's Agent Comms panel can render the actual task text // and reply text immediately, instead of falling back to the diff --git a/workspace-server/internal/handlers/activity_test.go b/workspace-server/internal/handlers/activity_test.go index ffb93d701..6386bc11d 100644 --- a/workspace-server/internal/handlers/activity_test.go +++ b/workspace-server/internal/handlers/activity_test.go @@ -934,6 +934,93 @@ func TestLogActivity_Broadcast_IncludesRequestAndResponseBodies(t *testing.T) { } } +// TestLogActivity_Broadcast_IncludesErrorDetail pins the fix for #1420: +// error_detail was stored in the DB but never included in the live +// ACTIVITY_LOGGED WebSocket broadcast, so the canvas could only show +// "Agent error (Exception) — see workspace logs for details." without +// surfacing the actionable error reason (e.g. oauth_org_not_allowed). +func TestLogActivity_Broadcast_IncludesErrorDetail(t *testing.T) { + mock := setupTestDB(t) + defer mock.ExpectationsWereMet() + + mock.ExpectExec("INSERT INTO activity_logs"). + WillReturnResult(sqlmock.NewResult(1, 1)) + + cb := &recordingBroadcaster{} + srcID := "ws-canvas" + tgtID := "ws-agent" + method := "message/send" + summary := "A2A request to ws-agent failed" + errorDetail := "403 Forbidden: oauth_org_not_allowed — Your organization has disabled Claude subscription access. Use an API key or ask your admin to enable access." + status := "error" + + LogActivity(context.Background(), cb, ActivityParams{ + WorkspaceID: srcID, + ActivityType: "a2a_receive", + SourceID: &srcID, + TargetID: &tgtID, + Method: &method, + Summary: &summary, + Status: status, + ErrorDetail: &errorDetail, + }) + + if len(cb.calls) != 1 { + t.Fatalf("expected 1 broadcast, got %d", len(cb.calls)) + } + payload := cb.calls[0].payload + if payload["activity_type"] != "a2a_receive" { + t.Errorf("activity_type = %v, want a2a_receive", payload["activity_type"]) + } + ed, ok := payload["error_detail"].(string) + if !ok { + t.Fatalf("error_detail missing from broadcast payload: got %#v", payload["error_detail"]) + } + if ed != errorDetail { + t.Errorf("error_detail = %q, want %q", ed, errorDetail) + } + if payload["status"] != status { + t.Errorf("status = %v, want %q", payload["status"], status) + } +} + +// TestLogActivity_Broadcast_OmitsNilErrorDetail verifies that when +// ErrorDetail is nil the broadcast does not include an empty error_detail key +// (matching the same omission pattern as request_body/response_body above). +func TestLogActivity_Broadcast_OmitsNilErrorDetail(t *testing.T) { + mock := setupTestDB(t) + defer mock.ExpectationsWereMet() + + mock.ExpectExec("INSERT INTO activity_logs"). + WillReturnResult(sqlmock.NewResult(1, 1)) + + cb := &recordingBroadcaster{} + srcID := "ws-canvas" + tgtID := "ws-agent" + method := "message/send" + summary := "A2A request succeeded" + status := "ok" + + LogActivity(context.Background(), cb, ActivityParams{ + WorkspaceID: srcID, + ActivityType: "a2a_receive", + SourceID: &srcID, + TargetID: &tgtID, + Method: &method, + Summary: &summary, + Status: status, + // ErrorDetail intentionally omitted (nil) + }) + + if len(cb.calls) != 1 { + t.Fatalf("expected 1 broadcast, got %d", len(cb.calls)) + } + payload := cb.calls[0].payload + if _, present := payload["error_detail"]; present { + t.Errorf("error_detail should be omitted when nil, got %v", payload["error_detail"]) + } +} + // TestLogActivityTx_DefersBroadcastUntilCommitHook pins the #149 // contract: LogActivityTx returns a commitHook that the caller MUST // invoke after tx.Commit(); the broadcast MUST NOT fire from inside -- 2.52.0