Repository navigation
Agent chat: rich tool rendering (diffs, structured command output) - #5967
lawrencecchen wants to merge 6 commits into
Conversation
file_change items now render unified-diff blocks (Claude Edit/Write/ MultiEdit/NotebookEdit inputs via a lightweight LCS line diff, Codex apply_patch envelopes parsed directly), with the file path, +/- counts, and new/deleted badges. command_execution rows get a prompt-styled command line, exit-code badge (Codex JSON envelope metadata or exit-code text fallback), duration, and the existing scrollable output block. Read-like dynamic_tool_call items show path + clamped preview instead of raw JSON; web_search rows show the query and extracted result links (http/https only, same anchor policy as the sanitized markdown). All parsing lives in pure functions (toolData.ts) with bun tests, plus server-render smoke tests for each row family. No new dependencies; the agentChatSurface chunk grows 28242 -> 48742 bytes. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
A live command_execution can carry partial output while still in_progress; the implicit exit-0 inference now requires the item to be completed. Also documents why the webview still parses the Codex shell JSON envelope (the daemon unwraps it except when the inner output is empty, and the protocol does not yet carry exit code/duration as structured fields). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Diff sources are cut at 200k chars before splitting and each FileDiff allocates at most 1000 DiffLine objects; +/- counts stay accurate over the full input and the row shows an 'N more lines not shown' note for the capped remainder. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Command output is only checked for the Codex shell JSON envelope when the session provider is codex (threaded as a value snapshot from the session header down to the command row), so another provider's stdout that happens to look like the envelope is never reinterpreted. Diffs whose source strings exceed the parse cap now carry sourceTruncated and the row renders an explicit 'change too large to diff fully' marker, so a tail-only change can no longer render as +0 -0 with no indication. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The textual exit-code regex now only runs on outputs flagged is_error, so success output that merely mentions an exit code keeps its green badge. Web-search result extraction iterates matchAll lazily with a 100k-char scan cap instead of materializing every embedded object. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The Codex envelope candidate is parsed only under 100k chars (the genuine daemon leak case is tiny), MultiEdit stops diffing once the line budget is spent and flags the diff truncated, and apply_patch counts lines past the per-file cap instead of allocating them. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit fab2c2e. Configure here.
| truncated = truncated || exceedsSourceCap(editOld, editNew); | ||
| editIndex += 1; | ||
| } | ||
| return lines.length > 0 ? [makeFileDiff(path, "edit", lines, truncated)] : []; |
There was a problem hiding this comment.
Wrong truncation message
Medium Severity
When a Claude MultiEdit hits the per-diff line budget, the parser sets sourceTruncated even though the source string was not cut at the character cap. The diff footer prefers that flag and shows the “change too large to diff fully” label instead of the line-count remainder message, which misstates why content is missing.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit fab2c2e. Configure here.
| return; | ||
| } | ||
| current.lines.push(line); | ||
| }; |
There was a problem hiding this comment.
Diff counts skip budget
Low Severity
When parseApplyPatch or MultiEdit processing hits the line budget, later diff lines or edits are skipped but never counted. Header +/− totals still come only from retained DiffLine entries, so large patches can under-report additions and removals while still showing a “more lines” footer.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit fab2c2e. Configure here.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fab2c2ed89
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const content = stringField(record, "content") ?? stringField(record, "new_source"); | ||
| if (content !== null) { | ||
| const sourceLines = splitLines(boundedSource(content)); | ||
| const lines: DiffLine[] = sourceLines.map((text) => ({ kind: "add", text })); |
There was a problem hiding this comment.
For Write/NotebookEdit inputs with many short lines (for example generated files or lockfiles under the 200k-character source cap), this maps the entire payload into DiffLine objects before makeFileDiff slices it to 1,000 rows. That puts the expensive allocation on the timeline render path and defeats the intended MAX_DIFF_LINES budget; count skipped lines while building only the first capped rows instead of materializing all lines first.
Useful? React with 👍 / 👎.
Greptile SummaryThis PR replaces the raw JSON dump rendering of tool timeline rows in
Confidence Score: 4/5Safe to merge; the two findings are display-only inaccuracies with no data-loss or security risk. The change is a significant rendering expansion with pure functions, good test coverage, and bounded allocations. The only notable issue is that NotebookEdit items are classified as op:create, causing a misleading new badge for operations that edit existing notebook cells. The MultiEdit loop can also temporarily allocate well beyond MAX_DIFF_LINES before makeFileDiff clamps the output, which is harmless in practice but worth tightening. Both are display/memory polish items on a well-structured, well-tested addition. webviews/src/agent-chat/react/toolData.ts — the NotebookEdit op classification and MultiEdit in-loop allocation. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[ConversationItem] --> B{item.type}
B -->|file_change| C[fileChangeDiffs]
B -->|command_execution| D[commandExecutionView]
B -->|web_search| E[webSearchView]
B -->|dynamic_tool_call| F{fileViewData?}
B -->|mcp_tool_call / unknown| G[GenericToolRow]
C --> C1{apply_patch marker?}
C1 -->|yes| C2[parseApplyPatch multi-file FileDiff list]
C1 -->|no| C3{old_string + new_string?}
C3 -->|yes| C4[computeLineDiff LCS single FileDiff]
C3 -->|no| C5{edits array?}
C5 -->|yes| C6[MultiEdit: per-edit LCS concatenated FileDiff]
C5 -->|no| C7[content / new_source all-added FileDiff]
C2 & C4 & C6 & C7 --> R1[FileChangeRow diff blocks + counts]
C1 --> C8{empty result?}
C8 -->|yes| G
D --> D1{provider == codex + small output?}
D1 -->|yes| D2[JSON envelope parse exit_code + duration]
D1 -->|no| D3[regex fallback is_error only]
D2 & D3 --> R2[CommandRow prompt + exit badge + output]
E --> R3[WebSearchRow query + result links]
F -->|yes| R4[FileViewRow path + clamped preview]
F -->|no| G
Reviews (1): Last reviewed commit: "Bound envelope parsing and multi-edit di..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const content = stringField(record, "content") ?? stringField(record, "new_source"); | ||
| if (content !== null) { | ||
| const sourceLines = splitLines(boundedSource(content)); | ||
| const lines: DiffLine[] = sourceLines.map((text) => ({ kind: "add", text })); | ||
| return [makeFileDiff(path, "create", lines, exceedsSourceCap(content))]; | ||
| } |
There was a problem hiding this comment.
op: "create" shows misleading "new" badge
NotebookEdit carries new_source (new cell body) without an old_source, so the code falls into the content ?? new_source branch and sets op: "create". This causes FileChangeRow to render a "new" badge for every NotebookEdit, even though the notebook file already exists and the operation is modifying a cell — not creating a new file. The all-added rendering is fine, but the op should be "edit" to suppress the badge. One approach is to distinguish: if the key was new_source (no old counterpart) produce op: "edit", reserving "create" for content-only Write items where there is genuinely no prior file.
| // Whole-loop work budget: once the line cap is reached, skip the | ||
| // remaining edits entirely instead of diffing them and slicing later. | ||
| if (lines.length >= MAX_DIFF_LINES) { | ||
| truncated = true; | ||
| break; | ||
| } | ||
| const editRecord = asRecord(edit); | ||
| const editOld = stringField(editRecord, "old_string"); | ||
| const editNew = stringField(editRecord, "new_string"); | ||
| if (editOld === null || editNew === null) { | ||
| continue; | ||
| } | ||
| if (editIndex > 0) { | ||
| lines.push({ kind: "hunk", text: "" }); | ||
| } | ||
| lines.push(...computeLineDiff(editOld, editNew)); | ||
| truncated = truncated || exceedsSourceCap(editOld, editNew); | ||
| editIndex += 1; | ||
| } | ||
| return lines.length > 0 ? [makeFileDiff(path, "edit", lines, truncated)] : []; |
There was a problem hiding this comment.
MAX_DIFF_LINES before the cap
The budget check at the top of the loop correctly aborts future edits once lines.length >= MAX_DIFF_LINES, but lines.push(...computeLineDiff(editOld, editNew)) for the current iteration is unbounded. In the worst case (many 1-char-per-line edits capped at MAX_DIFF_SOURCE_CHARS), a single computeLineDiff call can return up to ~200 k DiffLine objects when the LCS cell limit triggers del-all/add-all; all of them land in lines before makeFileDiff slices to 1000. The final output is always correct, but per-edit allocation can reach O(source_chars) temporarily. A cheap fix is to apply the same budget cap inside each per-edit push via lines.push(...computeLineDiff(editOld, editNew).slice(0, MAX_DIFF_LINES - lines.length)), breaking after if the cap was hit.
lawrencecchen
added
the
stale-revisit
This branch was successfully deployed
2 participants
Builds on #5736. Tool rows in the /agent-chat surface previously rendered every tool item as raw text (JSON input dump + output blob behind a disclosure). This gives each item family a structured renderer, with all parsing in pure functions.
file_change items render real unified-diff blocks: Claude Edit/MultiEdit old/new pairs go through a lightweight LCS line diff (common prefix/suffix trim, collapsed context runs, no Monaco or Pierre import), Write/NotebookEdit render as all-added creates, and Codex apply_patch envelopes are parsed directly from the +/- patch text (multi-file, Add/Update/Delete/Move). The file path is shown prominently with +/- counts and new/deleted badges; diffs clamp at 14 lines with expand.
command_execution items get a prompt-styled command line (Codex
["bash","-lc",...]argv unwrapped), an exit-code badge (green/red), duration when known, and the scrollable output block. Exit metadata comes from the Codex JSON envelope when it reaches the webview, or a prose fallback that only runs onis_erroroutput; implicit exit 0 is only inferred for completed items.dynamic_tool_call Read-like items show path + clamped preview instead of raw JSON. web_search items show the query and extracted result links (http/https only,
rel=noreferrer, same anchor policy as the sanitized markdown renderer; plain text fallback otherwise). mcp_tool_call and unrecognized shapes keep the generic expandable row.All render-path parsing is bounded: 200k-char source cap (flagged with an explicit "change too large to diff fully" marker so a tail-only change never looks unchanged), 1000-DiffLine cap per file with counted remainder, LCS cell limit, MultiEdit/apply_patch work budgets, 100k cap before envelope JSON.parse, lazy capped web-search scan. Envelope unwrapping is scoped to Codex sessions via a provider snapshot threaded from the session header, so other providers' stdout is never reinterpreted.
Logic lives in
toolData.ts(pure, no DOM) with bun tests for the diff computation, apply_patch parsing, exit-code/duration extraction, file-view and search parsing, and the bounding behavior, plus server-render smoke tests per row family. Components stay dumb and take item value snapshots; no raw useEffect; strings go through the agent-chat labels module (English-only by webview convention, matching the agent-session surface; autoreview's localization finding is rejected on that documented basis). Known protocol gap: the daemon strips Codex exit_code/duration except when the inner output is empty, so most real Codex rows show status color without a numeric badge until the protocol carries structured exit metadata.agentChatSurface.mjsgrows 28242 -> 50341 bytes (gzip 13.95 kB); no new dependencies. Gates:bun test(205 pass),bun run typecheck,bun run lint:ci, committed assets rebuilt via./scripts/build-webviews-app.sh.🤖 Generated with Claude Code
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Note
Cursor Bugbot is generating a summary for commit fab2c2e. Configure here.
Summary by cubic
Replaces raw tool output in /agent-chat with structured views: real unified diffs, command executions with exit status/duration, file previews, and web search links. Improves readability and safety with bounded parsing and provider‑scoped envelopes.
New Features
["bash","-lc", ...]argv.web_searchshows the query and extracted http/https links.toolData.tswith bun tests; components render typed data. No new dependencies.Bug Fixes
Written for commit fab2c2e. Summary will update on new commits.