Merge pull request 'fix(canvas): render delegation responses as normal messages not error banners' (#171) from fix/issue-159-delegation-response-error into main

canvas/src/components/tabs/chat/AgentCommsPanel.tsx

@@ -513,7 +513,20 @@ function GroupedCommsView({
       />
       <div className="flex-1 overflow-y-auto p-3 space-y-2">
         {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 ? (
             <ErrorMessage key={msg.id} msg={msg} />
           ) : (
             <NormalMessage key={msg.id} msg={msg} />

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<unknown>>();
+const apiPostMock = vi.fn<(url: string, body?: unknown) => Promise<unknown>>();
 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(<AgentCommsPanel workspaceId="ws-self" />);
+
+    // 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(<AgentCommsPanel workspaceId="ws-self" />);
+
+    // 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 */ }));

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", () => {

canvas/src/components/tabs/chat/message-parser.ts

@@ -168,10 +168,10 @@ export function extractResponseText(body: Record<string, unknown>): string {
       if (rootTexts.length > 0) collected.push(rootTexts.join("\n"));
 
       // Task shape: {result: {artifacts: [{parts: [...]}]}}
-      const artifacts = result.artifacts as Array<Record<string, unknown>> | undefined;
+      const artifacts = result.artifacts as Array<Record<string, unknown> | 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, unknown>): 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 */ }