Fix agent tool call execution diagnostics#353
Merged
Conversation
📝 WalkthroughWalkthroughThis PR enhances observability and tool resolution across the LLM service pipeline. It unifies tool-name lookup across built-in, external MCP, and proxy registries; adds MiniMax endpoint detection; and introduces structured logging with rich metadata throughout tool-call parsing, execution, and tracking workflows. ChangesTool Observability and Resolution Pipeline
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ 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)
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.
🧹 Nitpick comments (2)
TelegramSearchBot.LLM/Service/AI/LLM/OpenAIResponsesService.cs (1)
549-552: ⚡ Quick winConsider aligning tool indicator display behavior with the main execution path.
The resume path now yields each tool indicator immediately after that tool executes (incremental feedback), while the main path batches all tool indicators and yields them together after all tools complete (lines 338-339). This creates a user-visible inconsistency between the two execution paths.
The resume path's incremental feedback approach provides better progress visibility. Consider refactoring the main path to match this behavior for consistency.
♻️ Align main path to yield tool indicators incrementally
In the main execution path (around lines 254-339), move the tool indicator generation and yielding inside the loop to match the resume path behavior:
foreach (var funcCall in completedFuncCalls) { // ... normalization and history updates ... var argsDict = OpenAIService.DeserializeToolArgumentsForDisplay(argsJson); - toolIndicators.Append(McpToolHelper.FormatToolCallDisplay(name, argsDict)); // Execute tool _logger.LogInformation(...); string toolResultString; try { // ... tool execution ... } catch (Exception ex) { // ... error handling ... } + var toolIndicator = McpToolHelper.FormatToolCallDisplay(name, argsDict); + currentMessageContentBuilder.Append(toolIndicator); + yield return currentMessageContentBuilder.ToString(); + // Add function call output to history inputItems.Add(ResponseItem.CreateFunctionCallOutputItem(callId, toolResultString)); } -currentMessageContentBuilder.Append(toolIndicators.ToString()); -yield return currentMessageContentBuilder.ToString();This would provide consistent incremental feedback in both execution paths.
🤖 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 `@TelegramSearchBot.LLM/Service/AI/LLM/OpenAIResponsesService.cs` around lines 549 - 552, The main execution path currently batches tool-indicator output, causing inconsistency with the resume path which appends and yields each indicator immediately; update the main tool-processing loop that builds tool indicators (the code that currently calls McpToolHelper.FormatToolCallDisplay and appends to newContentBuilder/fullContentBuilder in batch) to instead create the toolIndicator, append it to newContentBuilder and fullContentBuilder and yield newContentBuilder.ToString() inside the per-tool loop (the same pattern used in the resume path) so both paths produce incremental feedback consistently.TelegramSearchBot.LLM/Service/AI/LLM/OpenAIService.cs (1)
1061-1066: ⚡ Quick winConsider truncating the final string instead of taking 80 tool names.
The current code uses
.Take(80)which takes up to 80 tool names before joining them with commas. If tool names are long, this could produce log lines exceeding thousands of characters. Consider truncating the final joined string to a character limit instead.♻️ Proposed fix to limit log line length
_logger.LogInformation( "{ServiceName}: Starting native tool call cycle. Model={Model}, ToolCount={ToolCount}, ToolNames={ToolNames}", ServiceName, modelName, nativeTools.Count, - string.Join(",", nativeTools.Select(t => t.FunctionName).Take(80))); + SanitizeAndTruncateArguments(string.Join(",", nativeTools.Select(t => t.FunctionName)), 200));🤖 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 `@TelegramSearchBot.LLM/Service/AI/LLM/OpenAIService.cs` around lines 1061 - 1066, The log builds ToolNames by joining nativeTools.Select(t => t.FunctionName).Take(80) which limits count but not character length; change the code around the _logger.LogInformation call in OpenAIService to first build the joinedNames string (e.g., string.Join(",", nativeTools.Select(t => t.FunctionName))) and then truncate it to a safe max character length (e.g., 200–500 chars) with an ellipsis when longer before passing it as the ToolNames argument; ensure you still use ServiceName, modelName and nativeTools.Count as before and only replace the ToolNames expression with the truncated string.
🤖 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 `@TelegramSearchBot.LLM/Service/AI/LLM/OpenAIResponsesService.cs`:
- Around line 549-552: The main execution path currently batches tool-indicator
output, causing inconsistency with the resume path which appends and yields each
indicator immediately; update the main tool-processing loop that builds tool
indicators (the code that currently calls McpToolHelper.FormatToolCallDisplay
and appends to newContentBuilder/fullContentBuilder in batch) to instead create
the toolIndicator, append it to newContentBuilder and fullContentBuilder and
yield newContentBuilder.ToString() inside the per-tool loop (the same pattern
used in the resume path) so both paths produce incremental feedback
consistently.
In `@TelegramSearchBot.LLM/Service/AI/LLM/OpenAIService.cs`:
- Around line 1061-1066: The log builds ToolNames by joining
nativeTools.Select(t => t.FunctionName).Take(80) which limits count but not
character length; change the code around the _logger.LogInformation call in
OpenAIService to first build the joinedNames string (e.g., string.Join(",",
nativeTools.Select(t => t.FunctionName))) and then truncate it to a safe max
character length (e.g., 200–500 chars) with an ellipsis when longer before
passing it as the ToolNames argument; ensure you still use ServiceName,
modelName and nativeTools.Count as before and only replace the ToolNames
expression with the truncated string.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0822b8bc-2884-4434-9edf-2dccd09f517f
📒 Files selected for processing (7)
TelegramSearchBot.LLM.Test/Service/AI/LLM/McpToolHelperProxyTests.csTelegramSearchBot.LLM.Test/Service/AI/LLM/OpenAIToolCallNormalizationTests.csTelegramSearchBot.LLM/Service/AI/LLM/McpToolHelper.csTelegramSearchBot.LLM/Service/AI/LLM/OpenAIResponsesService.csTelegramSearchBot.LLM/Service/AI/LLM/OpenAIService.csTelegramSearchBot.LLMAgent/Service/AgentLoopService.csTelegramSearchBot/Service/AI/LLM/ChunkPollingService.cs
Contributor
PR Check ReportSummary
Test Results
Code Quality
Test Artifacts
LinksThis report is auto-generated by GitHub Actions |
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 by CodeRabbit
New Features
Tests
Improvements