Skip to content

fix: dedupe tool call display by toolCallId and sanitize titles#138

Merged
thepagent merged 2 commits intoopenabdev:mainfrom
marvin-69-jpg:fix/dedupe-tool-call-display
Apr 11, 2026
Merged

fix: dedupe tool call display by toolCallId and sanitize titles#138
thepagent merged 2 commits intoopenabdev:mainfrom
marvin-69-jpg:fix/dedupe-tool-call-display

Conversation

@marvin-69-jpg
Copy link
Copy Markdown
Contributor

Summary

Two independent rendering bugs in the streaming tool-call display, both visible in any Discord thread that runs more than a couple of bash tools:

  1. Duplicate placeholder lines. claude-agent-acp emits the first tool_call event before the input fields are streamed in, so the title falls back to "Terminal" / "Edit" / etc. A later tool_call_update carries the refined real title. The current handler pushes a new line on every event without deduping, leaving orphaned placeholders that never get updated:

    🔧 `Terminal`...
    ❌
    🔧 `cd /home/node/work && git status`...
    ✅
    

    The ToolDone arm tries to find the entry to update via tool_lines.iter_mut().rev().find(|l| l.contains(&title)), which works for the refined entries but never matches the orphaned Terminal placeholders.

  2. Inline-code spans break on multi-line commands. The title is rendered as format!("🔧 {title}...") — single backticks. Discord's single-backtick inline code only spans one line. Any heredoc, multi-line bash, or &&-chained command split across lines breaks the inline code span at the first \n, and the rest of the command spills out as garbled raw text. Visually it looks like the command was truncated.

Fix

  • Carry the ACP toolCallId through AcpEvent::ToolStart / ToolDone so the renderer can pin updates to the same entry across the streaming tool_calltool_call_updatetool_call_update(status=completed) lifecycle.
  • Replace tool_lines: Vec<String> with Vec<ToolEntry> (id, title, state). ToolStart updates the slot in place if the id is already present; ToolDone finds by id (not by substring match against an unreliable title) and preserves the existing title when the Done event omits one — which it routinely does in claude-agent-acp updates.
  • Add a sanitize_title helper that flattens \n to " ; " and rewrites embedded backticks so they can't break the surrounding inline-code span.
  • Each tool now renders via ToolEntry::render(), which picks the icon from a ToolState enum (Running / Completed / Failed) — no more brittle substring lookups.

Repro (before this PR)

In a Discord thread connected to a claude preset bot, ask:

@bot 在 /tmp/work 跑這幾個指令:
1. pwd
2. git status
3. ls -la && echo done

Result: every Bash invocation leaves a 🔧 \Terminal`...line in the message that never updates to the real command. Multi-line&&` commands also render with the second line falling out of the inline-code span.

After this PR

Each tool call appears as a single line that evolves in place:

🔧 `pwd`...
✅ `pwd`
🔧 `git status`...
✅ `git status`
🔧 `ls -la && echo done`...
✅ `ls -la && echo done`

Multi-line commands collapse to one line: cmd1 ; cmd2 ; cmd3 — still inside the inline-code span, no markdown breakage.

Files changed

  • src/acp/protocol.rsAcpEvent::ToolStart / ToolDone carry an id; classify_notification extracts toolCallId from the update payload.
  • src/discord.rs — new ToolEntry / ToolState types; sanitize_title helper; tool_lines becomes Vec<ToolEntry>; ToolStart/ToolDone match arms dedupe by id.

+108 / -15. No new dependencies.

Compatibility

  • ACP protocol: toolCallId is already part of every tool_call and tool_call_update payload (it's how claude-agent-acp itself correlates the events internally — see acp-agent.js lines 1501-1556 in v0.25.0). We just weren't reading it.
  • Backwards compatible with any ACP backend that emits tool_call without a toolCallId (we fall back to an empty string id, in which case all calls without ids share one slot — the same effective behavior as today, just on the empty-id path).
  • No public API change. All new types are private to discord.rs.

Testing

Running in production for the past two days against a multi-user Discord channel doing heavy git/gh PR workflows including many multi-line bash commands. No regressions seen; the orphan-line and multi-line-cmd issues are gone.

Note: this PR is intentionally scoped to the tool-call display layer. It does not touch the streaming chunking / message-splitting code (that's #135's territory) or the thinking/agent_thought_chunk handling. A follow-up will surface thinking content as a blockquote on top of the message; that's a separate concern.

claude-agent-acp emits multiple events for the same tool invocation as
the input fields stream in: a `tool_call` with a placeholder title
("Terminal" / "Edit" / etc.) followed by one or more `tool_call_update`
events that refine the title to the real command. The current handler
pushes a new line on every event without deduping, so the message ends
up with orphaned placeholder lines that never get updated:

    🔧 `Terminal`...
    ❌
    🔧 `cd /home/node/work && git status`...
    ✅

It also passes the raw title through inline-code formatting without
flattening newlines or escaping embedded backticks. Discord's
single-backtick inline-code spans render on a single line only, so
multi-line shell commands (heredocs, &&-chained commands written across
lines) appear truncated mid-render with the inline-code span breaking
on the first newline.

Fix:

* Carry the ACP toolCallId through AcpEvent::ToolStart / ToolDone so the
  renderer can pin updates to the same entry.
* Replace `tool_lines: Vec<String>` with `Vec<ToolEntry>` (id, title,
  state). ToolStart updates the slot in place if the id is already
  present; ToolDone preserves the existing title when the Done event
  omits one (which it routinely does in claude-agent-acp updates).
* Add a `sanitize_title` helper that flattens \n to " ; " and rewrites
  embedded backticks so they can't break the surrounding inline-code
  span.
* Each tool now renders via ToolEntry::render() which picks the icon
  from the state — no more brittle substring-based line lookups against
  the placeholder title.

Tested in production against a multi-user Discord channel running heavy
git/gh workflows with multi-line bash commands.
Copy link
Copy Markdown
Contributor

@masami-agent masami-agent left a comment

Choose a reason for hiding this comment

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

Solid fix, well-documented, and production-tested 👍

The toolCallId-based dedup is the right approach — substring matching on title was always fragile, especially with claude-agent-acp's placeholder → refined title lifecycle. The ToolEntry / ToolState design is clean and the sanitize_title helper handles the multi-line command rendering issue nicely.

A few minor notes (non-blocking):

  • ToolEntry and ToolState are marked pub but only used within discord.rs — could be pub(crate) or private. Not a blocker.
  • Backward compat with empty toolCallId is handled correctly (all share one slot = same behavior as before).

LGTM, good to merge.

@chaodu-agent
Copy link
Copy Markdown
Collaborator

LGTM — clean fix, well-structured, and production-tested. Approving and merging.

One minor note for a follow-up: ToolState and ToolEntry (and their fields) are marked pub but only used within discord.rs. Can be made private in a future cleanup. Non-blocking.

Thanks for the contribution! 🙏

Copy link
Copy Markdown
Collaborator

@thepagent thepagent left a comment

Choose a reason for hiding this comment

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

LGTM — visibility fix applied, squashing and merging.

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.

4 participants