Skip to content

fix(ui): distinguish failed tool results from successful ones - #729

Draft
kvnloo wants to merge 4 commits into
CopilotKit:mainfrom
kvnloo:fix/tool-result-truthfulness
Draft

kvnloo wants to merge 4 commits into
CopilotKit:mainfrom
kvnloo:fix/tool-result-truthfulness

Conversation

@kvnloo

@kvnloo kvnloo commented Oct 3, 2026 •

Copy link
Copy Markdown

Follow-up to #438.

Credit to guidovizoso for the original transcript diagnosis, and to kevin9327 for #570, which established the server-side invariant this UI can now rely on: a refusal is marked Refused., while a vendor/runtime failure is returned as The vendor reported an error: … or That tool could not be called….

I rechecked #438 against current main rather than restoring the old patch wholesale:

  • the old percent-decoding problem is gone;
  • failed server tools still render as an ordinary completed ToolLine;
  • refusals still collapse the action label to bare Blocked.

Change

  • classify the two current terminal-failure forms in tool-result.ts;
  • pass failed into the existing ToolLine failure state;
  • keep refusal and failure distinct;
  • retain the action label on refusal as Blocked: <label>.

Regression

app/tests/tool-result.test.ts now covers:

  • vendor error, raw + JSON encoded → failed;
  • runtime/deployment call failure → failed;
  • ordinary result → not failed;
  • policy refusal → not failed;
  • no result yet → not failed/running remains separate.

This does not change execution, policy, audit, or provider behavior; it only makes the transcript agree with the server outcome already recorded.

Credit / provenance

@charan-rathore charan-rathore left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed a00ef629ac34481ea13f5f50abfc1f3ab92226b5 against #438 and merged #570, and checked the current prefixes in server/src/plugins/tools.ts and the callback route in server/src/app.ts. Decoding the JSON string before matching the two terminal-failure forms is consistent with those producers. Keeping Refused. separate and leaving an absent result in the running state preserves the server's failure/refusal distinction without changing execution or policy.

Local bun run test on tool-result/tool-name passes 26 tests. I added two temporary review probes rendering the actual ToolLine with renderToStaticMarkup; all 28 tests pass, including the amber failure label, destructive Blocked: Searched Slack refusal label, normal label, and running class. bun run typecheck passes app/server/worker.

Two formatting failures are introduced here. On the four changed files, bunx biome check reports:

  • app/src/components/channels/tool-line.tsx: the longer refusal/failure ternary needs the formatter's multiline layout.
  • app/tests/tool-result.test.ts: the extra blank line before the new describe is rejected.

The same check also reports import ordering in chat-transcript.tsx, but I verified that failure already exists on base cb5dc32a44517622c6db4e527e61d3abb389b43c; I am not attributing it to this PR. The base check has one error and this head has three. Could you format the two newly introduced cases before merging?

It would also be useful to keep component-level coverage of the new Blocked: <action> label/failure presentation: the added tests currently exercise only toolResultFailed, so dropping the failed prop or reverting the refusal label would leave them green. My rendering checks did not exercise a live transcript in the browser, and I did not run the database-backed full suite. This is a comment rather than an approval of those untested paths.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants