fix(assistant): execute text tool calls#32
Conversation
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis PR enables iterative tool calling for both OpenAI Chat and Anthropic providers. Each provider now loops through tool execution, handles native tool calls, and falls back to parsing XML-like ChangesTool-calling loop and fallback
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #32 +/- ##
==========================================
+ Coverage 58.13% 59.97% +1.83%
==========================================
Files 165 168 +3
Lines 15973 16532 +559
==========================================
+ Hits 9286 9915 +629
+ Misses 5719 5611 -108
- Partials 968 1006 +38
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/assistant/openai_chat.go`:
- Around line 210-222: openAIChatToolMessages currently leaves callID empty when
len(events) > len(calls), producing invalid tool role messages; update the
function to fail fast by checking if index >= len(calls) and panic/return a
descriptive error (e.g., "mismatched counts: events vs calls") instead of using
an empty fallback, so that the invariant violation surfaces immediately;
reference openAIChatToolMessages and adjust callers
(appendOpenAIChatToolConversation / advanceOpenAIChatLoop) as needed to handle
the new behavior or propagate the error.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 65b2d4d8-58c8-4a76-afce-346f6e7b1de6
📒 Files selected for processing (13)
internal/assistant/anthropic.gointernal/assistant/anthropic_internal_test.gointernal/assistant/anthropic_tools_test.gointernal/assistant/client.gointernal/assistant/errors_extra.gointernal/assistant/openai_chat.gointernal/assistant/openai_responses.gointernal/assistant/provider_tool_calls_test.gointernal/assistant/text_tool_calls.gointernal/assistant/text_tool_calls_test.gointernal/assistant/tool_loop.gointernal/assistant/tool_loop_test.gointernal/assistant/tool_schema.go
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/assistant/provider_tool_calls_test.go (1)
186-188: ⚡ Quick winAssert successful tool execution, not just tool detection.
Line 186-188 currently verifies only event count/name. This can pass even when the read tool fails and returns an error payload. Please assert successful execution output in this fallback path too.
Proposed test assertion improvement
require.Len(t, result.ToolEvents, 1) assert.Equal(t, jsonReadToolName, result.ToolEvents[0].Name) + assert.Empty(t, result.ToolEvents[0].Error) + assert.Contains(t, result.ToolEvents[0].Result, "librecode") require.Len(t, requests, 2)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/assistant/provider_tool_calls_test.go` around lines 186 - 188, The test currently only checks result.ToolEvents length/name (result.ToolEvents, jsonReadToolName, requests) which can pass when the read tool failed; update the assertion to verify successful execution by asserting the tool event's output/response is the expected successful payload (e.g., inspect result.ToolEvents[0].Output or Result field contains the expected JSON/body and no error indicator) and/or assert the corresponding requests entry (requests[1]) contains the successful response content and status we expect; ensure you still keep the existing name/count asserts but add assertions that the tool returned the correct success value rather than an error payload.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@internal/assistant/provider_tool_calls_test.go`:
- Around line 186-188: The test currently only checks result.ToolEvents
length/name (result.ToolEvents, jsonReadToolName, requests) which can pass when
the read tool failed; update the assertion to verify successful execution by
asserting the tool event's output/response is the expected successful payload
(e.g., inspect result.ToolEvents[0].Output or Result field contains the expected
JSON/body and no error indicator) and/or assert the corresponding requests entry
(requests[1]) contains the successful response content and status we expect;
ensure you still keep the existing name/count asserts but add assertions that
the tool returned the correct success value rather than an error payload.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: abcf3acd-d8e2-493b-8e43-2b23d7e6813d
📒 Files selected for processing (8)
internal/assistant/anthropic.gointernal/assistant/anthropic_tools_test.gointernal/assistant/client.gointernal/assistant/openai_chat.gointernal/assistant/provider_tool_calls_test.gointernal/assistant/text_tool_calls.gointernal/assistant/text_tool_calls_test.gointernal/assistant/tool_loop_test.go



Summary
Validation