fix: enhance tool call handling with toolCallID and improve logging#11
Merged
fix: enhance tool call handling with toolCallID and improve logging#11
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 38 minutes and 57 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe changes introduce tool-call ID propagation throughout the system—from agent handler interfaces to WebSocket events to the frontend store—enabling reliable correlation of tool invocations with their completions. Additionally, tool construction is refactored to depend on model instances passed per agent creation, and session loading is enhanced to reconstruct tool activity from stored entries. Changes
Sequence DiagramsequenceDiagram
actor User
participant Agent
participant Runner
participant Handler as Handler<br/>(ACP/Web/TUI)
participant Frontend as Frontend<br/>Store
User->>Agent: Execute with tools
Agent->>Agent: Create tool call<br/>(ID: "call-123")
Agent->>Runner: Tool call event
Runner->>Handler: OnToolCall(name, args, "call-123")
Handler->>Handler: Store & emit event<br/>with tool_call_id
Handler->>Frontend: WebSocket: tool_call event<br/>(tool_call_id: "call-123")
Frontend->>Frontend: addToolCall(name, args, "call-123")<br/>Create ToolCall with ID
Agent->>Runner: Tool execution completes
Runner->>Handler: OnToolResult(name, output, "call-123", err)
Handler->>Handler: Record result<br/>with ID
Handler->>Frontend: WebSocket: tool_result event<br/>(tool_call_id: "call-123")
Frontend->>Frontend: resolveToolCall(name, output, "call-123", err)<br/>Match by ID → Update ToolCall
Frontend->>User: Update UI with result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/handler/acp.go (1)
46-61:⚠️ Potential issue | 🟠 MajorACP still matches results to “most recent tool” instead of the new correlation ID.
Both methods ignore
toolCallIDand rely on a singleactiveToolCall. If two tool invocations overlap, the laterOnToolCalloverwrites that field andOnToolResultcan update the wrong ACP call, so this transport misses the bug fix the PR is adding. Keep a runner-ID → ACP-ID mapping and resolve results through that map.Suggested direction
type ACPHandler struct { conn *acp.AgentSideConnection sessionID acp.SessionId toolCallCounter atomic.Int64 mu sync.Mutex - // activeToolCall tracks the ToolCallId for the most recently started tool. - activeToolCall acp.ToolCallId + // activeToolCall tracks the ToolCallId for the most recently started tool. + activeToolCall acp.ToolCallId + toolCallsByRunnerID map[string]acp.ToolCallId } @@ func NewACPHandler(conn *acp.AgentSideConnection, sessionID acp.SessionId) *ACPHandler { return &ACPHandler{ conn: conn, sessionID: sessionID, + toolCallsByRunnerID: make(map[string]acp.ToolCallId), } } @@ -func (h *ACPHandler) OnToolCall(name, args, _ string) { +func (h *ACPHandler) OnToolCall(name, args, toolCallID string) { id := h.nextToolCallID() h.mu.Lock() h.activeToolCall = id + if toolCallID != "" { + h.toolCallsByRunnerID[toolCallID] = id + } h.mu.Unlock() @@ -func (h *ACPHandler) OnToolResult(name, output, _ string, err error) { +func (h *ACPHandler) OnToolResult(name, output, toolCallID string, err error) { h.mu.Lock() id := h.activeToolCall + if toolCallID != "" { + if mapped, ok := h.toolCallsByRunnerID[toolCallID]; ok { + id = mapped + delete(h.toolCallsByRunnerID, toolCallID) + } + } h.mu.Unlock()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/handler/acp.go` around lines 46 - 61, OnToolCall and OnToolResult currently ignore the incoming tool/runner identifier and use the single mutable field activeToolCall, which causes races when calls overlap; change this by adding a map (e.g., runnerToACP map[string]string) protected by h.mu, store the generated ACP call ID (from nextToolCallID / acp.StartToolCall) keyed by the passed-in tool/runner ID in OnToolCall (use the third parameter instead of discarding it), and in OnToolResult look up the ACP ID via that same runner ID to make the SessionUpdate for the corresponding acp.EndToolCall (or error update); ensure you delete the mapping after finalizing the result and keep locking around accesses to runnerToACP and activeToolCall for consistency, falling back to the previous behavior only if the runner ID is absent.
🧹 Nitpick comments (1)
AGENTS.md (1)
61-61: Narrow this lint exclusion to avoid weakening the mandatory lint policy.“Exclude the
script/directory from the linter” is very broad and conflicts with the stricter “make lintmust pass” guidance elsewhere in this file. Please scope this to specific files/rules (or generated outputs only) and document the exact config path where the exception is enforced.Suggested wording
-- Exclude the script/ directory from the linter — it contains code generation scripts that may not follow all conventions. +- Keep `make lint` mandatory. If exceptions are needed for `script/`, scope them to specific files/rules in linter config and document the rationale (avoid broad directory-wide exclusion).Based on learnings: “Lint is mandatory:
make lintmust pass before any PR.”🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@AGENTS.md` at line 61, Narrow the broad lint exclusion for the script/ directory by limiting it to specific generated outputs or known rule exceptions (e.g., /* generated */ files, script/build/*.js) instead of excluding the whole directory; update AGENTS.md to list the exact files/globs and the linter config path where the exception is declared (e.g., in .eslintrc.js overrides or .eslintignore entry) and confirm that the global requirement “make lint must pass” remains enforced for all other files. Ensure you reference the exact config key/path (e.g., "overrides" in .eslintrc.js or the .eslintignore glob) and add one-line rationale in AGENTS.md describing why that specific exclusion is necessary.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/command/web.go`:
- Around line 118-120: buildAllTools currently closes over the outer rec so
subagents keep writing to the old session after handleSwitchModel rotates
s.recorder; fix by making SubagentDeps obtain the recorder lazily or by updating
rec before createAgent runs. Specifically, either change how
env.NewSubagentTool/SubagentDeps is constructed so its Recorder field is a
function/closure (e.g., RecorderFunc) that returns the current s.recorder at
call time, or ensure handleSwitchModel updates the outer rec variable that
buildAllTools captures prior to calling createAgent; update references to rec in
buildAllTools, SubagentDeps, and env.NewSubagentTool accordingly.
In `@internal/config/log.go`:
- Around line 32-36: The stdlib logger redirection is only set when the file
open succeeds, which allows diagnostics to go to stderr on failures; after
initializing appLogger (the configured zap logger instance) always call
log.SetOutput(appLogger.Writer()) and retain log.SetFlags(log.LstdFlags) so the
standard library logger is routed to the configured logger instead of stderr;
remove any fallback to os.Stderr or conditional log.SetOutput(f) branches and
ensure any file-open error paths still initialize appLogger and then perform
log.SetOutput(appLogger.Writer()).
In `@internal/runner/runner.go`:
- Around line 207-210: The current loop over the map variable pending (keyed by
tc.Index) relies on ranging a Go map which yields randomized order; to preserve
the model's emitted order, collect the map keys (tc.Index), sort them
numerically, then iterate the sorted keys and for each index fetch p :=
pending[idx] and call h.OnToolCall(p.name, p.args.String(), p.id) and
rec.RecordToolCall(...) if rec != nil; update the block around pending,
h.OnToolCall, rec.RecordToolCall to use this sorted-key iteration so tool calls
are emitted in the original tc.Index order.
In `@web/src/stores/chat.ts`:
- Around line 354-364: The reconstructed ToolCall object (tc) is missing the
toolCallID so timeline items lose their correlation; when creating tc in the
block that builds ToolCall (the const tc in this diff), include a toolCallID
property set from e.tool_call_id (or undefined/null when absent) so the object
retains the original correlation key, and continue to add it to pendingToolCalls
(pendingToolCalls.set(e.tool_call_id, tc)) as before; update references to
ToolCall consumers if they expect the toolCallID field.
- Around line 386-389: The current loop only marks ID-backed entries in
pendingToolCalls as done, leaving legacy/no-ID tool calls still marked
'running'; update the session-reconstruction cleanup to also iterate over the
main toolCalls collection (e.g., toolCalls or toolCalls.values()) and set any
toolCall.tc.status === 'running' to 'done' (and/or where result is missing) so
all unresolved running tool calls—both ID-backed and legacy—are finalized;
reference pendingToolCalls, toolCalls, and the tc.status field when making the
change.
- Around line 174-179: The agentDone cleanup only sets running -> done for
top-level tool entries in timeline.value (loop in agentDone); extend this to
also traverse and finalize any nested child tool calls (the children arrays on
tool items) so no child remains 'running'. Implement a recursive helper or
stack-based traversal referenced from agentDone that visits each tool item and
its children (e.g., inspect item.data.children) and sets any data.status ===
'running' to 'done' for every nested tool node.
---
Outside diff comments:
In `@internal/handler/acp.go`:
- Around line 46-61: OnToolCall and OnToolResult currently ignore the incoming
tool/runner identifier and use the single mutable field activeToolCall, which
causes races when calls overlap; change this by adding a map (e.g., runnerToACP
map[string]string) protected by h.mu, store the generated ACP call ID (from
nextToolCallID / acp.StartToolCall) keyed by the passed-in tool/runner ID in
OnToolCall (use the third parameter instead of discarding it), and in
OnToolResult look up the ACP ID via that same runner ID to make the
SessionUpdate for the corresponding acp.EndToolCall (or error update); ensure
you delete the mapping after finalizing the result and keep locking around
accesses to runnerToACP and activeToolCall for consistency, falling back to the
previous behavior only if the runner ID is absent.
---
Nitpick comments:
In `@AGENTS.md`:
- Line 61: Narrow the broad lint exclusion for the script/ directory by limiting
it to specific generated outputs or known rule exceptions (e.g., /* generated */
files, script/build/*.js) instead of excluding the whole directory; update
AGENTS.md to list the exact files/globs and the linter config path where the
exception is declared (e.g., in .eslintrc.js overrides or .eslintignore entry)
and confirm that the global requirement “make lint must pass” remains enforced
for all other files. Ensure you reference the exact config key/path (e.g.,
"overrides" in .eslintrc.js or the .eslintignore glob) and add one-line
rationale in AGENTS.md describing why that specific exclusion is necessary.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 947dd21f-3d9d-4e3e-8cf0-df5ae641f251
📒 Files selected for processing (12)
AGENTS.mdinternal/command/web.gointernal/config/log.gointernal/handler/acp.gointernal/handler/handler.gointernal/handler/tui.gointernal/handler/web.gointernal/runner/runner.gointernal/web/server.goweb/src/App.vueweb/src/stores/chat.tsweb/src/types/api.ts
Comment on lines
118
to
120
| env.NewSubagentTool(&tools.SubagentDeps{ | ||
| ChatModel: chatModel, | ||
| ChatModel: cm, | ||
| Recorder: rec, |
There was a problem hiding this comment.
Subagent tools still capture the previous session recorder.
buildAllTools closes over the outer rec, and handleSwitchModel in internal/web/server.go rebuilds the agent before rotating s.recorder. After a model change, the main turn records to the new session, but subagent events from this agent can still append to the old session file. Please either update the outer rec before createAgent runs or make SubagentDeps resolve the current recorder lazily.
Also applies to: 203-204
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/command/web.go` around lines 118 - 120, buildAllTools currently
closes over the outer rec so subagents keep writing to the old session after
handleSwitchModel rotates s.recorder; fix by making SubagentDeps obtain the
recorder lazily or by updating rec before createAgent runs. Specifically, either
change how env.NewSubagentTool/SubagentDeps is constructed so its Recorder field
is a function/closure (e.g., RecorderFunc) that returns the current s.recorder
at call time, or ensure handleSwitchModel updates the outer rec variable that
buildAllTools captures prior to calling createAgent; update references to rec in
buildAllTools, SubagentDeps, and env.NewSubagentTool accordingly.
Comment on lines
+354
to
+364
| const tc: ToolCall = { | ||
| id: genId('tc'), | ||
| name: e.name, | ||
| args: e.args || '', | ||
| status: 'running', | ||
| timestamp: e.timestamp ? new Date(e.timestamp).getTime() : Date.now(), | ||
| } | ||
| timeline.value.push({ kind: 'tool', data: tc, seq: nextSeqId() }) | ||
| if (e.tool_call_id) { | ||
| pendingToolCalls.set(e.tool_call_id, tc) | ||
| } |
There was a problem hiding this comment.
Persist toolCallID on reconstructed tool calls.
Line 354-360 builds ToolCall without toolCallID, so loaded timeline items lose the same correlation key used elsewhere in this store.
Proposed fix
const tc: ToolCall = {
id: genId('tc'),
+ toolCallID: e.tool_call_id,
name: e.name,
args: e.args || '',
status: 'running',
timestamp: e.timestamp ? new Date(e.timestamp).getTime() : Date.now(),
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const tc: ToolCall = { | |
| id: genId('tc'), | |
| name: e.name, | |
| args: e.args || '', | |
| status: 'running', | |
| timestamp: e.timestamp ? new Date(e.timestamp).getTime() : Date.now(), | |
| } | |
| timeline.value.push({ kind: 'tool', data: tc, seq: nextSeqId() }) | |
| if (e.tool_call_id) { | |
| pendingToolCalls.set(e.tool_call_id, tc) | |
| } | |
| const tc: ToolCall = { | |
| id: genId('tc'), | |
| toolCallID: e.tool_call_id, | |
| name: e.name, | |
| args: e.args || '', | |
| status: 'running', | |
| timestamp: e.timestamp ? new Date(e.timestamp).getTime() : Date.now(), | |
| } | |
| timeline.value.push({ kind: 'tool', data: tc, seq: nextSeqId() }) | |
| if (e.tool_call_id) { | |
| pendingToolCalls.set(e.tool_call_id, tc) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/src/stores/chat.ts` around lines 354 - 364, The reconstructed ToolCall
object (tc) is missing the toolCallID so timeline items lose their correlation;
when creating tc in the block that builds ToolCall (the const tc in this diff),
include a toolCallID property set from e.tool_call_id (or undefined/null when
absent) so the object retains the original correlation key, and continue to add
it to pendingToolCalls (pendingToolCalls.set(e.tool_call_id, tc)) as before;
update references to ToolCall consumers if they expect the toolCallID field.
This was referenced Apr 16, 2026
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Enhance the handling of tool calls by introducing a
toolCallIDfor better tracking and logging. This change improves the logging mechanism to ensure all logs are directed to a specified debug log file.Related Issues / Tickets
Type of Change
Changes Made
Added
toolCallIDparameter toOnToolCallandOnToolResultmethods.Updated logging to redirect output to a debug log file.
Modified relevant interfaces and data structures to accommodate the new
toolCallID.Testing
Screenshots / Recordings
Checklist
Additional Notes
Summary by CodeRabbit
New Features
Bug Fixes
Chores