fix: preserve Kandev MCP text results (#3067)

2026-08-26 22:17:38 +00:00
parent 898225214b
commit aa91bf26c7
8 changed files with 321 additions and 3 deletions
@@ -39,7 +39,11 @@ function kandevToolCall(opts: {
          input: opts.input,
          output:
            opts.resultJson !== undefined
-              ? [{ type: "text", text: JSON.stringify(opts.resultJson) }]
+              ? {
+                  _meta: null,
+                  content: [{ type: "text", text: JSON.stringify(opts.resultJson) }],
+                  structuredContent: null,
+                }
              : undefined,
        },
      },
@@ -97,6 +97,25 @@ describe("extractMcpResult", () => {
    ).toEqual({ version: 1, resolved_charts: [] });
  });

+  it("falls back to text content when structuredContent is null", () => {
+    expect(
+      extractMcpResult({
+        _meta: null,
+        content: [{ type: "text", text: '{"total":2,"workspaces":[{"id":"w1"}]}' }],
+        structuredContent: null,
+      }),
+    ).toEqual({ total: 2, workspaces: [{ id: "w1" }] });
+  });
+
+  it("falls back to text content when structured_content is null", () => {
+    expect(
+      extractMcpResult({
+        content: [{ type: "text", text: '{"total":1}' }],
+        structured_content: null,
+      }),
+    ).toEqual({ total: 1 });
+  });
+
  it("unwraps the raw result wrapper emitted by ACP clients", () => {
    expect(extractMcpResult({ result: '{"version":1,"resolved_charts":[]}' })).toEqual({
      version: 1,
@@ -69,6 +69,10 @@ function unwrapContentBlocks(blocks: unknown[]): unknown {
  return tryParseJson(joined);
 }

+function hasValue(value: unknown): boolean {
+  return value !== undefined && value !== null;
+}
+
 // extractMcpResult walks the various shapes the ACP/MCP transport can emit and
 // returns the structured payload. Returns null if the value is empty/missing.
 export function extractMcpResult(value: unknown): unknown {
@@ -89,8 +93,12 @@ export function extractMcpResult(value: unknown): unknown {
      structuredContent?: unknown;
      structured_content?: unknown;
    };
-    if (obj.structuredContent !== undefined) return obj.structuredContent;
-    if (obj.structured_content !== undefined) return obj.structured_content;
+    if (hasValue(obj.structuredContent)) {
+      return obj.structuredContent;
+    }
+    if (hasValue(obj.structured_content)) {
+      return obj.structured_content;
+    }
    if (Array.isArray(obj.content)) return unwrapContentBlocks(obj.content);
    if (typeof obj.output === "string") return tryParseJson(obj.output);
    if (
@@ -0,0 +1,72 @@
+---
+created: 2026-08-26
+status: complete
+requirements:
+  - REQ-UI-KANDEV-MCP-TOOL-RESULTS-001
+system_design:
+  - ../../specs/ui/system-design/kandev-mcp-tool-results.md
+legacy_specs: []
+---
+
+# Implementation Plan: Kandev MCP Tool Result Fallback
+
+## Overview
+
+This repair preserves JSON text results when an MCP response contains null
+structured content. One work order adds the regression first, corrects the
+shared parser, and keeps the existing result precedence.
+
+## Scope
+
+### In scope
+
+- Reproduce the persisted Codex ACP result envelope in focused tests.
+- Treat null structured content as absent.
+- Preserve non-null structured content and historic wrapper support.
+- Keep desktop and mobile result values equal through the shared parser.
+
+### Out of scope
+
+- Backend MCP or ACP changes.
+- New renderer types or presentation changes.
+- New mobile composition or Playwright scenarios.
+
+## Technical approach
+
+Add an exact null-envelope case to `extractMcpResult` coverage. Update the
+renderer fixture so it uses the persisted `CallToolResult` shape.
+
+Change `extractMcpResult` to accept only non-null structured values. If a
+structured value is null, continue to the existing `content` parser.
+
+## Tests
+
+- `AC-UI-KANDEV-MCP-TOOL-RESULTS-001.1`: Keep the existing structured-content
+  precedence test in `kandev/parse.test.ts`.
+- `AC-UI-KANDEV-MCP-TOOL-RESULTS-001.2`: Add a test with
+  `structuredContent: null` and JSON text content.
+- `AC-UI-KANDEV-MCP-TOOL-RESULTS-001.3`: Use the shared renderer fixture to
+  show the returned count from the exact persisted envelope.
+
+## E2E tests
+
+The existing `mcp-status.spec.ts` and `mobile-mcp-status.spec.ts` cover the
+shared transcript entry point on desktop and mobile. No new Playwright test is
+required because this repair changes only data normalization.
+
+The mock agent flattens MCP results before ACP delivery. Therefore, a focused
+parser fixture is the faithful test for the Codex null envelope.
+
+## Work orders
+
+- [x] [Task 01: Preserve text fallback](task-01-preserve-text-fallback.md) (done)
+
+## Verification results
+
+Task 01 completed. The focused parser and renderer suite passed 51 tests, and
+frontend typecheck passed.
+
+## Risks
+
+- A broad falsy-value check can change valid structured-result precedence.
+- An incomplete fixture can miss the real Codex `CallToolResult` shape again.
@@ -0,0 +1,86 @@
+---
+id: "01-preserve-text-fallback"
+title: "Preserve text fallback"
+status: done
+wave: 1
+depends_on: []
+plan: "plan.md"
+requirements:
+  - REQ-UI-KANDEV-MCP-TOOL-RESULTS-001
+acceptance_criteria:
+  - AC-UI-KANDEV-MCP-TOOL-RESULTS-001.1
+  - AC-UI-KANDEV-MCP-TOOL-RESULTS-001.2
+  - AC-UI-KANDEV-MCP-TOOL-RESULTS-001.3
+system_design:
+  - ../../specs/ui/system-design/kandev-mcp-tool-results.md
+---
+
+# Task 01: Preserve Text Fallback
+
+## Summary
+
+Add a regression for the persisted Codex MCP envelope. Then correct the shared
+parser so a null structured value does not hide JSON text content.
+
+## In scope
+
+- Add the failing null-envelope parser test before the production change.
+- Update the renderer fixture to match the persisted `CallToolResult` shape.
+- Make the minimum null-handling correction in `extractMcpResult`.
+- Preserve existing structured-content and historic-wrapper behavior.
+
+## Out of scope
+
+- Backend MCP, ACP adapter, or database changes.
+- Renderer layout or copy changes.
+- New Playwright tests.
+
+## Acceptance
+
+- The regression fails because the parser returns null before the correction.
+- The corrected parser returns the JSON text payload when `structuredContent`
+  is null.
+- All focused parser and native Kandev renderer tests pass after the change.
+
+## Verification
+
+```bash
+(cd apps && pnpm --filter @kandev/web test -- --run components/task/chat/messages/kandev/parse.test.ts components/task/chat/messages/kandev-tool-message.test.tsx)
+(cd apps/web && pnpm run typecheck)
+```
+
+## Files likely touched
+
+- `apps/web/components/task/chat/messages/kandev/parse.ts`
+- `apps/web/components/task/chat/messages/kandev/parse.test.ts`
+- `apps/web/components/task/chat/messages/kandev-tool-message.test.tsx`
+- `docs/plans/kandev-mcp-tool-result-fallback/plan.md`
+- `docs/plans/kandev-mcp-tool-result-fallback/task-01-preserve-text-fallback.md`
+
+## Dependencies
+
+None.
+
+## Risks
+
+- Preserve empty but valid structured objects. Treat only null and undefined
+  as absent.
+
+## Parallelism
+
+`sequential`
+
+## Inputs
+
+- `REQ-UI-KANDEV-MCP-TOOL-RESULTS-001` and its system design.
+- The persisted task-session message and ACP envelope from the diagnosis.
+- Existing parser and renderer tests.
+
+## Results
+
+RED: The real Codex envelope test failed because `extractMcpResult` returned
+null when `structuredContent` was null.
+
+GREEN: The parser now falls through to JSON text content only for null or
+undefined structured values. The focused suite passed 51 tests, and frontend
+typecheck passed.
@@ -72,6 +72,7 @@ contract that other capabilities can reuse.
 - [Reload Kandev when a tab is restored from a frozen browser snapshot](requirements/fix-duplicated-tab-stale-data.md)
 - [GitHub PR Review Actions](requirements/github-pr-review-actions.md)
 - [GitHub Saved-Query Default Views](requirements/github-saved-query-defaults.md)
+- [Kandev MCP Tool Results](requirements/kandev-mcp-tool-results.md)
 - [Auto-hide empty workflow steps](requirements/kanban-auto-hide-empty-columns.md)
 - [Repair last-prompt transcript pinning](requirements/last-prompt-pinning-regressions.md)
 - [Repair merge commit details](requirements/merge-commit-details.md)
@@ -161,6 +162,7 @@ contract that other capabilities can reuse.
 - [Task PR Automation Controls System Design Part 3](system-design/ci-pr-automation-03.md)
 - [Merge Queue Recovery Controls](system-design/ci-pr-merge-queue-recovery-controls.md)
 - [Entity Reference Composer](system-design/entity-reference-composer.md)
+- [Kandev MCP Tool Results](system-design/kandev-mcp-tool-results.md)
 - [Mobile Task Chrome](system-design/mobile-task-chrome.md)
 - [Persistent status motion](system-design/persistent-status-motion.md)
 - [Sidebar Task Focus](system-design/sidebar-task-focus.md)
@@ -0,0 +1,45 @@
+---
+status: active
+system: ui
+created: 2026-08-26
+owners:
+  - kandev
+---
+
+# Kandev MCP Tool Results Requirements
+
+## Overview
+
+Kandev shows native transcript rows for its built-in MCP tools. A completed
+result can contain structured content, text content, or both forms.
+
+The UI system owns this reusable presentation contract. The agent and platform
+systems continue to own tool execution, ACP normalization, and persistence.
+
+## Terminology
+
+- **Structured result:** A non-null object in `structuredContent` or
+  `structured_content`.
+- **Text fallback:** JSON text in the standard MCP `content` array.
+
+## Requirements
+
+### REQ-UI-KANDEV-MCP-TOOL-RESULTS-001: Reliable result display
+
+**Intent:** Users need each completed Kandev MCP row to show the result that
+the tool returned.
+
+#### Acceptance criteria
+
+- **AC-UI-KANDEV-MCP-TOOL-RESULTS-001.1:** When a completed result contains
+  non-null structured content, the transcript shall use that content.
+- **AC-UI-KANDEV-MCP-TOOL-RESULTS-001.2:** When structured content is null or
+  absent, the transcript shall parse an available JSON text fallback.
+- **AC-UI-KANDEV-MCP-TOOL-RESULTS-001.3:** Desktop and mobile transcripts shall
+  show the same result values from the shared parser.
+
+## Out of scope
+
+- Changes to MCP handlers, ACP normalization, or message persistence.
+- Portable rendering for non-Kandev MCP content.
+- Changes to transcript layout, navigation, scrolling, or touch behavior.
@@ -0,0 +1,82 @@
+---
+status: current
+system: ui
+requirements:
+  - REQ-UI-KANDEV-MCP-TOOL-RESULTS-001
+---
+
+# Kandev MCP Tool Results System Design
+
+## Purpose and boundaries
+
+This design defines how the web transcript selects a Kandev MCP result for
+native rendering. It does not change tool execution, transport, or storage.
+
+The ACP adapter removes provider wrappers and keeps the standard MCP
+`CallToolResult`. The UI then selects one usable payload from that result.
+
+## Requirement mapping
+
+| Requirement | Design section |
+| --- | --- |
+| `REQ-UI-KANDEV-MCP-TOOL-RESULTS-001` | [Result selection](#result-selection), [Responsive behavior](#responsive-behavior) |
+
+## Components and responsibilities
+
+- `normalizeCodexMCPToolResult` removes the Codex `{error, result}` wrapper.
+  It keeps the complete standard MCP result.
+- Persisted tool-message metadata stores that result at
+  `metadata.normalized.generic.output`.
+- `KandevToolMessage` sends the stored output to `extractMcpResult`.
+- `extractMcpResult` selects and parses the payload for all native Kandev tool
+  renderers.
+- Each renderer reads its fields and shows the native transcript row.
+
+## Result selection
+
+`extractMcpResult` uses this precedence:
+
+1. Use `structuredContent` when its value is not null.
+2. Use `structured_content` when its value is not null.
+3. Parse text blocks from `content`.
+4. Parse a string in `output`.
+5. Unwrap a recognized `result` wrapper.
+6. Keep a plain object unchanged.
+
+An explicit null structured value means that no structured result exists. It
+must not suppress a valid text fallback.
+
+## Control flow
+
+1. The agent completes a Kandev MCP tool call.
+2. The ACP adapter stores the standard result in normalized message metadata.
+3. The transcript passes the result to the shared parser.
+4. The parser applies the result-selection precedence.
+5. The registered renderer shows the parsed values.
+
+Live updates and replay use the same stored message shape and parser.
+
+## Failure and recovery
+
+Malformed JSON text remains plain text. A result with no usable payload keeps
+the existing renderer fallback and does not break adjacent transcript rows.
+
+The repair must preserve non-null structured results and historic wrapper
+support. These paths protect rich-output results and older persisted messages.
+
+## Persistence
+
+The repair adds no table or migration. Existing message metadata remains the
+only persisted source.
+
+## Responsive behavior
+
+Desktop and mobile chat use the same parser and renderer registry. This repair
+changes data selection only. It does not change composition or interaction.
+
+Existing desktop and mobile MCP transcript tests remain the nearest surface
+examples. Focused parser and renderer tests reproduce the provider envelope.
+
+## Related decisions
+
+- [Keep Agent Rich Output Host Native](../../../decisions/2026-08-14-kandev-native-agent-rich-output.md)