Skip to content

feat: implement diff handling for tool calls and enhance error detail…#333

Merged
pikann merged 4 commits into
masterfrom
feature/implement-diff-handling-for-tool-calls
Jul 25, 2026
Merged

feat: implement diff handling for tool calls and enhance error detail…#333
pikann merged 4 commits into
masterfrom
feature/implement-diff-handling-for-tool-calls

Conversation

@pikann

@pikann pikann commented Jul 25, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Agent Edit/Write-style tool calls rendered as raw stringified JSON args instead of a readable diff. ToolFallback now renders a real line-by-line diff (reusing DiffLineRow from content-diff.tsx) whenever a tool-call part carries diff data, falling back to the existing JSON view otherwise.
  • eventsToThreadMessages now extracts diff content from two distinct event shapes, since edits can come from either backend path:
    • ACP-backed agents (Claude Code / Codex / Gemini CLI): ACPToolCallEvents that report edits as {type: "diff", path, old_text, new_text} content blocks.
    • Native OpenHands SDK agents: the file_editor tool's str_replace/create/insert commands, which report before/after content as old_content/new_content on the FileEditorObservation that follows the ActionEvent — a completely different shape with no diff-typed content block. Mirrors the SDK's own FileEditorObservation._has_meaningful_diff so view calls, errors, and no-op edits correctly fall back to the plain args view instead of showing an empty/misleading diff.
  • services/ai-agent: _wait_for_done_or_stop now returns the ConversationErrorEvent detail alongside (stopped, errored, shutdown) so run_conversation can persist it to agent_conversations.error_message on a terminal ERROR/STUCK status. Previously this detail was only logged, leaving error_message NULL for every conversation that failed this way.

Fixes #315

Test plan

  • bun run test in apps/web — 323 tests passing (incl. new coverage for both the ACP diff-block path and the native file_editor path: str_replace, create, view, and error cases)
  • pytest tests/test_executor.py in services/ai-agent — 29 tests passing (incl. new coverage for error_detail propagation on a terminal ERROR status)
  • tsc --noEmit / biome check on the changed frontend files — clean
  • Replayed the real event stream of a conversation that uses the native (non-ACP) agent path, pulled from a local dev DB — confirmed 22 of 42 file_editor tool calls (the actual edits) now produce a diff artifact, while view calls and no-ops correctly do not

🤖 Generated with Claude Code

@pullfrog pullfrog Bot 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.

✅ No new issues found.

Reviewed changes — two complementary improvements: frontend diff rendering for ACP and native file-editor tool calls, and backend error detail propagation so agent_conversations.error_message is populated on ERROR terminal status rather than only logged.

  • Render file diffs in tool-fallback cardsextractDiffBlocks parses ACP {type:"diff"} content blocks (tolerating both snake_case and camelCase), and extractFileEditorDiff handles native OpenHands file_editor observations. Tool-fallback shows diffs via DiffLineRow (exported from content-diff.tsx) through computeLineDiff computed inside useMemo. Diffs replace raw args when present.
  • Propagate ConversationErrorEvent detail_wait_for_done_or_stop returns a 4th error_detail tuple element, consumed by run_conversation and passed to update_conversation_status(error_message=...). Previously detail was only logged, leaving error_message NULL for every conversation that failed with ERROR/STUCK.

Pullfrog  | View workflow run | Using DeepSeek Pro (free via Pullfrog for OSS) (Kimi K2 not used — the program covers this model; add its provider key to run your pick) | 𝕏

Adding error_message to update_conversation_status's call signature
changed the recorded mock call shape, so the "paused" assertions no
longer matched and the "failed" test was left checking status only.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

@pullfrog pullfrog Bot 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.

✅ No new issues found.

Reviewed changes — test-only additions that validate the error_detail propagation added in the prior commit.

  • Unit test for _wait_for_done_or_stop error detail — new test_wait_for_done_or_stop_returns_error_detail_on_terminal_error creates a fake conversation in ERROR status and asserts the extracted detail matches the ConversationErrorEvent content. Existing tests updated to destructure the new 4-tuple.
  • E2E assertions for error_message propagationtest_agent_reply_is_persisted_through_real_sandbox and test_agent_tool_call_then_reply_persists_full_flow now assert error_message=None on pause. test_llm_failure_marks_conversation_failed asserts "fake upstream failure" in error_message, closing the gap where the prior commit added the feature but didn't validate it end-to-end.

Pullfrog  | View workflow run | Using DeepSeek Pro (free via Pullfrog for OSS) (Kimi K2 not used — the program covers this model; add its provider key to run your pick) | 𝕏

@pullfrog pullfrog Bot 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.

✅ No new issues found.

Reviewed changes — performance guard and UX improvement: collapse long unchanged diff runs into context markers and skip expensive diff computation for very large files.

  • Add collapseUnchangedContext to collapse long unchanged runs — new pure function in diff-utils.ts collapses stretches of unchanged lines beyond a configurable context window (default 3) into a single {type: "collapsed", count} marker, matching git diff's hunk convention. Both tool-fallback.tsx and content-diff.tsx use it so a one-line edit to a large file renders only the relevant context instead of every unchanged line.
  • Add MAX_DIFF_LINES_PER_SIDE = 2000 guard in tool-fallback.tsx — skips computeLineDiff entirely when either the old or new text exceeds 2000 lines, showing a "diff too large" i18n message. The O(n·m) DP matrix for a multi-thousand-line file would otherwise freeze the tab.
  • Add DiffCollapsedRow component and i18n keys — renders the collapsed-lines indicator using the new contentDiff.collapsedLines key across all 9 locales, plus contentDiff.diffTooLarge for the too-large guard.
  • Thorough collapseUnchangedContext unit tests — 6 test cases cover short untouched runs, long mid-file runs with context on both sides, leading-only context, trailing-only context, short gaps that merge without collapsing, and empty input.

Pullfrog  | View workflow run | Using DeepSeek Pro (free via Pullfrog for OSS) (Kimi K2 not used — the program covers this model; add its provider key to run your pick) | 𝕏

CI runs `vitest run`, which cannot bundle the Bun-native `bun:test`
built-in — it only worked locally under `bun test`, which shims it.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@pikann
pikann merged commit 551293e into master Jul 25, 2026
5 checks passed
@pikann
pikann deleted the feature/implement-diff-handling-for-tool-calls branch July 25, 2026 18:55
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.

[Bug] Agent edits render as raw JSON; no diff and no streaming

1 participant