diff --git a/canvas/src/components/tabs/chat/AgentCommsPanel.tsx b/canvas/src/components/tabs/chat/AgentCommsPanel.tsx index 074d96fc..cb49309b 100644 --- a/canvas/src/components/tabs/chat/AgentCommsPanel.tsx +++ b/canvas/src/components/tabs/chat/AgentCommsPanel.tsx @@ -513,7 +513,20 @@ function GroupedCommsView({ />
{visible.map((msg) => - msg.status === "error" ? ( + // Only render the error UI when there is NO usable response + // content. A "error" status from the platform means the HTTP + // transport layer had a problem — but the agent response text + // may have arrived and been stored in response_body.text. + // Delegation results set responseText via extractResponseText + // once that function learned to parse body.text, so checking + // !msg.responseText here correctly identifies "no actual reply + // was received" vs. "reply arrived but status=error". + // + // Without this guard, successful delegation results were + // rendered as error banners, PMs saw "restart" prompts and + // restarted working agents, and retry storms formed as the + // platform re-delivered the same completed work (issue #159). + msg.status === "error" && !msg.responseText ? ( ) : ( diff --git a/canvas/src/components/tabs/chat/__tests__/AgentCommsPanel.render.test.tsx b/canvas/src/components/tabs/chat/__tests__/AgentCommsPanel.render.test.tsx index 80b37982..b8032c33 100644 --- a/canvas/src/components/tabs/chat/__tests__/AgentCommsPanel.render.test.tsx +++ b/canvas/src/components/tabs/chat/__tests__/AgentCommsPanel.render.test.tsx @@ -4,9 +4,11 @@ import { render, screen, fireEvent, waitFor } from "@testing-library/react"; // API mock — tests can override per case via apiGetMock.mockImplementationOnce. const apiGetMock = vi.fn<(url: string) => Promise>(); +const apiPostMock = vi.fn<(url: string, body?: unknown) => Promise>(); vi.mock("@/lib/api", () => ({ api: { get: (url: string) => apiGetMock(url), + post: (url: string, body?: unknown) => apiPostMock(url, body), }, })); @@ -16,17 +18,23 @@ vi.mock("@/hooks/useSocketEvent", () => ({ useSocketEvent: () => {}, })); -// Canvas store — peer name resolution. -vi.mock("@/store/canvas", () => ({ - useCanvasStore: { - getState: () => ({ - nodes: [ - { id: "ws-self", data: { name: "Self" } }, - { id: "ws-peer", data: { name: "Peer Agent" } }, - ], - }), - }, -})); +// Canvas store — peer name resolution + ErrorMessage requires selectNode +// (Zustand hook usage). The mock must support BOTH: +// useCanvasStore.getState().nodes (plain object with getState) +// useCanvasStore((s) => s.selectNode) (Zustand hook with selector) +vi.mock("@/store/canvas", () => { + const state = { + nodes: [ + { id: "ws-self", data: { name: "Self" } }, + { id: "ws-peer", data: { name: "Peer Agent" } }, + ], + selectNode: vi.fn(), + }; + const hook = (selector?: (s: typeof state) => unknown) => + selector ? selector(state) : state; + hook.getState = () => state; + return { useCanvasStore: hook }; +}); // Toaster shim — AgentCommsPanel imports showToast. vi.mock("../../Toaster", () => ({ @@ -41,6 +49,8 @@ import { AgentCommsPanel } from "../AgentCommsPanel"; const scrollSpy = vi.fn<(opts?: ScrollIntoViewOptions | boolean) => void>(); beforeEach(() => { apiGetMock.mockReset(); + apiPostMock.mockReset(); + apiPostMock.mockResolvedValue({}); scrollSpy.mockReset(); Element.prototype.scrollIntoView = scrollSpy as unknown as Element["scrollIntoView"]; }); @@ -49,6 +59,81 @@ afterEach(() => { vi.clearAllMocks(); }); +// Regression test: when a delegation succeeds but the platform persisted +// status="error" (transport-layer HTTP failure, not agent failure), the +// canvas had the response text in msg.text but rendered ErrorMessage +// anyway, burying the real content in an "Underlying error" banner and +// prompting PMs to restart working agents (issue #159). +describe("AgentCommsPanel — error rendering guard (issue #159)", () => { + it("renders NormalMessage when status=error but msg.text is present (successful delegation)", async () => { + // Simulate a delegation result where status="error" (HTTP transport + // failed) but response_body.text carries the actual agent response. + // The correct behaviour: show the content as a normal inbound bubble, + // NOT an error banner. + apiGetMock.mockResolvedValueOnce([ + { + id: "act-1", + activity_type: "delegation", + method: "delegate_result", + source_id: "ws-self", + target_id: "ws-peer", + summary: "Delegation completed", + request_body: null, + // delegation.go stores response_body as {text: "...", delegation_id: "..."} + response_body: { + text: "PR #149: tier-check fails NO REVIEWS (author needs engineers/managers/ceo approval)", + delegation_id: "delg_01jx8q4n3k", + }, + status: "error", // transport-layer error, not agent failure + created_at: "2026-04-25T18:00:00Z", + }, + ]); + render(); + + // The response text should appear in a normal inbound bubble, NOT in + // an error banner. Specifically: no "Failed to deliver" or "returned + // an error" text should appear. + await waitFor(() => { + expect(screen.queryByText(/failed to deliver/i)).toBeNull(); + expect(screen.queryByText(/returned an error/i)).toBeNull(); + }); + // The actual content must be visible. + await waitFor(() => + expect( + screen.getByText(/tier-check fails NO REVIEWS/i), + ).toBeDefined(), + ); + }); + + it("renders ErrorMessage when status=error and msg.text is absent (true failure)", async () => { + // True delivery failure: no response body, no text. The error banner + // IS appropriate here. + apiGetMock.mockResolvedValueOnce([ + { + id: "act-1", + activity_type: "a2a_send", + source_id: "ws-self", + target_id: "ws-peer", + method: "message/send", + summary: "A2A send failed", + request_body: null, + response_body: null, + status: "error", + created_at: "2026-04-25T18:00:00Z", + }, + ]); + render(); + + // Error banner IS shown for true failures (no content). + // jsdom doesn't reliably match role="alert" in getByRole, so use + // getByText instead. + const errorBanner = await waitFor(() => + screen.getByText(/failed to deliver/i), + ); + expect(errorBanner).toBeDefined(); + }); +}); + describe("AgentCommsPanel — initial-state parity with ChatTab my-chat", () => { it("shows loading text while history fetch is in flight", () => { apiGetMock.mockReturnValueOnce(new Promise(() => { /* never resolves */ })); diff --git a/canvas/src/components/tabs/chat/__tests__/message-parser.test.ts b/canvas/src/components/tabs/chat/__tests__/message-parser.test.ts index 71befb4e..3a4748a7 100644 --- a/canvas/src/components/tabs/chat/__tests__/message-parser.test.ts +++ b/canvas/src/components/tabs/chat/__tests__/message-parser.test.ts @@ -209,6 +209,43 @@ describe("extractResponseText", () => { }; expect(extractResponseText(body)).toBe("Summary\nDetail block one\nDetail block two"); }); + + // Regression: delegation.go stores response_body as + // {"text": "...", "delegation_id": "..."} — no "result" wrapper. + // Without body.text handling, extractResponseText returns "" for + // delegate_result rows, causing the error UI to fire even when the + // delegation succeeded (issue #159). + it("extracts from body.text (delegation response_body shape)", () => { + const body = { + text: "PR #149: tier-check fails NO REVIEWS (author needs engineers/managers/ceo approval)", + delegation_id: "delg_01jx8q4n3k", + }; + expect(extractResponseText(body)).toBe( + "PR #149: tier-check fails NO REVIEWS (author needs engineers/managers/ceo approval)" + ); + }); + + it("prefers body.result over body.text when both present", () => { + const body = { + result: { parts: [{ kind: "text", text: "A2A result wins" }] }, + text: "Delegation text", + }; + // result path is checked first; A2A wins when both present. + expect(extractResponseText(body)).toBe("A2A result wins"); + }); + + it("returns empty string when body.text is empty string", () => { + expect(extractResponseText({ text: "" })).toBe(""); + }); + + it("extracts from body.response_preview (DELEGATION_COMPLETE WS event shape)", () => { + const body = { + response_preview: "PR #149: tier-check fails NO REVIEWS (author needs engineers/managers/ceo approval)", + }; + expect(extractResponseText(body)).toBe( + "PR #149: tier-check fails NO REVIEWS (author needs engineers/managers/ceo approval)" + ); + }); }); describe("extractTextsFromParts", () => { diff --git a/canvas/src/components/tabs/chat/message-parser.ts b/canvas/src/components/tabs/chat/message-parser.ts index 9ce8e502..cc1cf5e1 100644 --- a/canvas/src/components/tabs/chat/message-parser.ts +++ b/canvas/src/components/tabs/chat/message-parser.ts @@ -168,10 +168,10 @@ export function extractResponseText(body: Record): string { if (rootTexts.length > 0) collected.push(rootTexts.join("\n")); // Task shape: {result: {artifacts: [{parts: [...]}]}} - const artifacts = result.artifacts as Array> | undefined; + const artifacts = result.artifacts as Array | undefined>; if (artifacts) { for (const a of artifacts) { - const t = extractTextsFromParts(a.parts); + const t = extractTextsFromParts(a?.parts); if (t) collected.push(t); } } @@ -179,6 +179,20 @@ export function extractResponseText(body: Record): string { if (collected.length > 0) return collected.join("\n"); } + // Delegation results from delegation.go store response_body as + // {"text": "...", "delegation_id": "..."} — no "result" wrapper. + // Check this after the body.result path so A2A responses take + // precedence when both shapes are somehow present. + // Without this, responseText is always "" for delegate_result rows, + // causing the error UI to fire even when the delegation succeeded + // (issue #159). + if (typeof body.text === "string" && body.text) return body.text; + // DELEGATION_COMPLETE event (via canvas-events WS handler) stores + // response_body as {response_preview: "..."}. Handle this too. + if (typeof body.response_preview === "string" && body.response_preview) { + return body.response_preview; + } + // {task: "text"} — request body format, shouldn't be in response but handle it if (typeof body.task === "string") return body.task; } catch { /* ignore */ }