Skip to content

fix: restore apply_patch diff stats/viewer and align tool card rendering#214

Open
hightman wants to merge 2 commits intoOpen-ACP:developfrom
hightman:fix/apply-patch-diffstats-compat
Open

fix: restore apply_patch diff stats/viewer and align tool card rendering#214
hightman wants to merge 2 commits intoOpen-ACP:developfrom
hightman:fix/apply-patch-diffstats-compat

Conversation

@hightman
Copy link
Copy Markdown
Contributor

@hightman hightman commented Apr 7, 2026

Summary

Improve apply_patch compatibility for OpenCode-style ACP payloads so Telegram tool cards can consistently show line-change stats and viewer links, including tool_update events that omit the tool name. Also render apply_patch with existing edit-style tool card semantics for simpler, backward-compatible UI behavior.

Related Issues

Type of Change

  • 🐛 Bug fix
  • ✨ New feature
  • 🔌 New adapter / plugin
  • 📝 Documentation
  • ♻️ Refactor (no functional change)
  • ⚡ Performance improvement
  • 🧪 Tests
  • 🔧 CI / Build / Config

Changes Made

  • Unified diff-stat extraction path in MessageTransformer and added a safe apply_patch compatibility branch for kind=other payloads.
  • Added robust parsing for OpenCode apply_patch rawOutput.metadata (files[].additions/deletions, optional top-level totals) without affecting non-apply_patch agents.
  • Extended extractFileInfo with an apply_patch-only early branch (including patchText/patch_text fallback) so viewer links are generated from rawOutput.metadata.files[].before/after.
  • Added handling for tool_update cases where name is missing by recognizing cached patch text input.
  • Updated display-spec behavior so apply_patch kind=other reuses edit-style card rendering while preserving patch target title extraction from patch text.
  • Added/updated tests covering diffStats compatibility, viewer-link extraction, malformed payload safety, and display-spec mapping.

Testing

  • Ran pnpm test — all tests pass
  • Tested manually with:
  • Added new tests for this change
  • No tests needed (explain why)

Test commands executed:

  • pnpm vitest run src/core/adapter-primitives/__tests__/display-spec-builder.test.ts src/core/__tests__/message-transformer-extended.test.ts src/__tests__/extract-file-info.test.ts
  • pnpm build

Platform Impact

  • Telegram
  • Discord
  • Slack
  • CLI
  • REST API
  • Web UI
  • All / Core

Screenshots / Recordings

Screenshots(original):
1

Screenshots(after patching):
2

Checklist

  • My code follows the project's TypeScript conventions (ESM, .js extensions)
  • I have updated relevant documentation (if needed)
  • I have added/updated tests (if applicable)
  • My changes don't introduce new warnings or errors
  • I have run pnpm build successfully
  • My changes are backward compatible with existing config and data formats
  • If changing plugin API / PluginContext: I have updated src/cli/plugin-template/

Copy link
Copy Markdown
Contributor

@0xmrpeter 0xmrpeter left a comment

Choose a reason for hiding this comment

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

Hey @hightman, thanks for putting this together! The overall approach is solid — the defensive parsing is consistent, the backward-compatibility guard works well, and the test coverage is genuinely good quality. A couple of things worth addressing before merge:


[Important] Dead code in buildTitle

The new isApplyPatchOther early-return block at the top of buildTitle correctly handles all apply_patch kind=other calls. However, there are two older nameLower === "apply_patch" blocks further down in the same function that are now unreachable for any apply_patch call going through buildToolSpec — since effectiveKind is already remapped to "edit" before buildTitle is called, the old blocks will never fire.

Leaving them in creates confusion about which path is authoritative. Would be great to clean those up as part of this PR.


[Important] Multi-file patch: viewer link only shows first file

parseApplyPatchRawOutput correctly sums additions/deletions across all files in metadata.files for diffStats — nice. But the function returns on the first valid file, so when apply_patch touches multiple files in one call, only a single viewer link is generated (for whichever file comes first in the array), which may not be the most relevant one.

A simple improvement: sort files by additions + deletions descending before the loop, so the viewer link points to the file with the most changes — consistent with what diffStats reflects. If a full fix isn't in scope right now, at minimum a comment acknowledging this limitation would help future maintainers.


[Minor] Detection logic is duplicated across 3 files

The apply_patch detection heuristic (kind === "other" + name match or patchText presence) is re-implemented inline in message-transformer.ts, extract-file-info.ts, and display-spec-builder.ts. Not a blocker, but if the heuristic ever needs updating it'll need to change in 3 places. Worth extracting to a shared utility in a follow-up.


[Minor] PR checklist

The - [ ] Ran pnpm test — all tests pass item is still unchecked. Totally fine if you only ran the affected test files, just worth confirming the full suite doesn't surface anything unexpected.


Overall this is close — the core logic is correct and the tests are well-structured. Addressing the dead code cleanup and the multi-file viewer gap (even just with a comment) would get this to approve.

hightman and others added 2 commits April 7, 2026 18:12
Unify apply_patch detection, harden diff/viewer extraction for tool_update payloads, prioritize highest-churn files for multi-file viewer links, and map apply_patch cards to edit-style rendering for consistent UI behavior.

Co-authored-by: Codex <noreply@openai.com>
Keep a single TodoWrite title formatter in buildTitle to reduce ambiguity while preserving existing compatibility behavior.

Co-authored-by: Codex <noreply@openai.com>
@hightman hightman force-pushed the fix/apply-patch-diffstats-compat branch from 47ac7e0 to edfa5f3 Compare April 7, 2026 10:16
@hightman
Copy link
Copy Markdown
Contributor Author

hightman commented Apr 7, 2026

All review feedback has now been addressed and the branch was force-pushed.

  • Removed duplicate/unreachable branches in buildTitle (including legacy apply_patch fallback paths and duplicated TodoWrite handling), keeping one authoritative path.
  • Improved multi-file apply_patch viewer targeting by selecting the file with the highest additions + deletions.
  • Extracted shared apply_patch detection into src/core/utils/apply-patch-detection.ts to remove duplicated heuristics across files.
  • Kept apply_patch tool cards rendered with edit-style semantics for UI compatibility.

Validation:

  • pnpm vitest run src/core/adapter-primitives/tests/display-spec-builder.test.ts src/core/tests/message-transformer-extended.test.ts src/tests/extract-file-info.test.ts
  • pnpm build

Latest commit: edfa5f3

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