Bug sc 62423 fred service error missing toolcallid or #4
Conversation
…ervice-error-missing-toolcallid-or
WalkthroughThe changes in this pull request involve updates to the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Tool
participant System
User->>System: Request multiple tool calls (same name)
System->>Tool: Process tool call 1
Tool-->>System: Return result for tool call 1
System->>Tool: Process tool call 2
Tool-->>System: Return result for tool call 2
System-->>User: Return results for all tool calls
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
javascript/src/utils/stream.utils.ts (1)
Line range hint
158-169: Consider documenting the tool call handling strategy.The change to always return
falsefor tool calls represents an important architectural decision in how tool calls are processed. Consider adding a code comment explaining this design choice to help future maintainers understand why tool calls are handled differently from text and audio parts.} else { // we won't be able to reliably match tool calls // because there can be multiple tool calls with the same tool name + // Each tool call needs a unique index to prevent conflicts and ensure + // proper accumulation of arguments and IDs return false; }javascript/test/test-language-model.ts (1)
522-564: LGTM with a suggestion: Add assertion for unique toolCallIds.The test case effectively verifies handling of parallel tool calls with the same name but different arguments. This is crucial for the bug fix related to missing toolCallId.
Consider adding an assertion to verify that the toolCallIds are unique:
t.assert.equal(!!weatherCall1, true); t.assert.equal(!!weatherCall2, true); + t.assert.notEqual(weatherCall1?.toolCallId, weatherCall2?.toolCallId, + "Tool calls should have unique toolCallIds");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (3)
javascript/package.json(1 hunks)javascript/src/utils/stream.utils.ts(1 hunks)javascript/test/test-language-model.ts(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- javascript/package.json
🔇 Additional comments (3)
javascript/src/utils/stream.utils.ts (2)
167-167: LGTM! Safer handling of tool call indices.
The change to return false for tool calls is a good safety improvement. It prevents potential overwrites that could occur when multiple tool calls share the same name, ensuring each tool call gets a unique index.
Line range hint 102-109: Verify error handling for tool calls.
The error handling for missing toolCallId or toolName is well-implemented. Let's verify that this validation is sufficient for catching the reported bug.
✅ Verification successful
Let me analyze the validation of toolCallId and toolName across the codebase:
- In
stream.utils.ts, there's strict validation incomputeContent()that throws an error if either field is missing. - The schema in
schemas.gen.tsenforces both fields as required for tool calls and tool results. - The merging logic in
stream.utils.tscorrectly handles both fields during delta merging. - All language model adapters (OpenAI, Mistral, Google, Cohere, Anthropic) properly map these fields.
The error handling for tool calls is robust and correctly implemented. The validation in computeContent() ensures that both toolCallId and toolName are present before processing, which effectively prevents the reported bug. The schema validation and consistent handling across all adapters provide additional safeguards.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any other places where toolCallId or toolName might be used without validation
rg -g '*.ts' -g '*.js' 'toolCallId|toolName' -C 3
Length of output: 29713
javascript/test/test-language-model.ts (1)
387-387: LGTM: Comprehensive test input covering all schema requirements.
The test input provides good coverage of the complex schema by including all required fields with properly formatted data.
| @@ -164,10 +164,7 @@ export function guessDeltaIndex( | |||
| } else { | |||
There was a problem hiding this comment.
This else technically becomes unnecessary in this case
There was a problem hiding this comment.
i will make that change on the hoangvvo/llm-sdk and include on next release
What does this PR do?
Fix https://fireflies.sentry.io/issues/6052582757/?referrer=slack¬ification_uuid=e1c886ce-dfea-4803-a716-8305fe4f218e&alert_rule_id=15510789&alert_type=issue
Type of change
This pull request is a
Which introduces
How should this be manually tested?
xxx
What are the requirements to deploy to production?
Any background context you want to provide beyond Shortcut?
xxx
Screenshots (if appropriate)
Without this fix:
With this fix:
all tests pass:
![Uploading image.png…]()
Loom Video (if appropriate)
xxx
Any Security implications
xxx
Summary by CodeRabbit
New Features
@firefliesai/llm-sdkmodule to0.1.6.Bug Fixes
Tests