Conversation
…ses API This fixes issue #825 where the OpenAI Responses API returns a 400 error: "No tool call found for function call output with call_id hist_tool_XXX" The error occurs when tool calls are cancelled and their IDs remain in hist_tool_XXX format (from history) or toolu_XXX format (from Anthropic). OpenAI's Responses API requires call_XXX format for all call_ids. Changes: - Add normalizeToOpenAIToolId() utility in providers/utils/ - Apply normalization in OpenAIResponsesProvider.ts for function_call and function_call_output items - Apply normalization in buildResponsesRequest.ts for the same - Add buildResponsesInputFromContent.ts helper with normalization built-in - Add comprehensive tests for all normalization scenarios Fixes #825
Contributor
|
Warning Rate limit exceeded@acoliver has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 8 minutes and 27 seconds before requesting another review. ⌛ 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. 📒 Files selected for processing (1)
WalkthroughTool IDs are normalized to an OpenAI-style Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (4 passed)
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 |
Contributor
|
Let me first analyze the issue #825 to understand the problem: LLxprt PR Review – PR #830Issue Alignment Side Effects
Code Quality Tests & Coverage Verdict This PR implements a targeted fix for the OpenAI Responses API compatibility issue with proper normalization of tool IDs. The solution is well-tested with comprehensive coverage of the specific scenarios mentioned in issue #825, including handling of cancelled tool calls with LLxprt PR Review – PR #830Issue Alignment Side Effects
Code Quality
Tests & Coverage
The tests effectively validate the fix without being mock theater - they test actual normalization logic and integration points. Verdict |
The ContentBlocks types (TextBlock, ToolCallBlock, ToolResponseBlock) are exported from IContent.ts, not a separate ContentBlocks.ts file.
Remove unused vitest imports (vi, beforeEach, afterEach) and unused runtime/settings imports that were causing ESLint errors.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
packages/core/src/providers/openai/buildResponsesRequest.ts (1)
217-257: Consider consolidating duplicate normalization implementations.The relevant_code_snippets show that
OpenAIProvider.tsandOpenAIVercelProvider.tshave privatenormalizeToOpenAIToolIdmethods with similar logic. Now that a shared utility exists intoolIdNormalization.ts, consider refactoring those providers to use the shared utility for consistency and reduced duplication.packages/core/src/providers/utils/toolIdNormalization.ts (1)
40-56: Refactor duplicate logic for hist_tool_ and toolu_ prefixes.Lines 40-47 and 49-56 contain identical logic. Extract this into a helper function to follow DRY principles.
+const normalizeWithPrefix = (id: string, prefix: string): string => { + const suffix = id.substring(prefix.length); + const sanitizedSuffix = suffix.replace(/[^a-zA-Z0-9_]/g, ''); + if (sanitizedSuffix.length === 0) { + return 'call_' + crypto.randomUUID().replace(/-/g, ''); + } + return 'call_' + sanitizedSuffix; +}; + if (id.startsWith('hist_tool_')) { - const suffix = id.substring('hist_tool_'.length); - const sanitizedSuffix = suffix.replace(/[^a-zA-Z0-9_]/g, ''); - if (sanitizedSuffix.length === 0) { - return 'call_' + crypto.randomUUID().replace(/-/g, ''); - } - return 'call_' + sanitizedSuffix; + return normalizeWithPrefix(id, 'hist_tool_'); } if (id.startsWith('toolu_')) { - const suffix = id.substring('toolu_'.length); - const sanitizedSuffix = suffix.replace(/[^a-zA-Z0-9_]/g, ''); - if (sanitizedSuffix.length === 0) { - return 'call_' + crypto.randomUUID().replace(/-/g, ''); - } - return 'call_' + sanitizedSuffix; + return normalizeWithPrefix(id, 'toolu_'); }Note: Apply this refactoring AFTER fixing the determinism issue flagged in the previous comment.
packages/core/src/providers/openai-responses/buildResponsesInputFromContent.ts (2)
50-52: Type assertions without runtime validation may be unsafe.Type assertions on lines 50-52, 59, 62, and 84 assume that the type guards (
.find()and.filter()withb.type === '...') correctly identify the block types. While the type field is checked, there's no guarantee at runtime that the block structure matches the asserted type if the data is malformed or comes from an untrusted source.As per coding guidelines, prefer proper type guards over assertions.
Consider adding runtime type guard functions:
function isTextBlock(block: unknown): block is TextBlock { return typeof block === 'object' && block !== null && 'type' in block && block.type === 'text' && 'text' in block && typeof block.text === 'string'; } function isToolCallBlock(block: unknown): block is ToolCallBlock { return typeof block === 'object' && block !== null && 'type' in block && block.type === 'tool_call' && 'id' in block && 'name' in block && 'parameters' in block; } function isToolResponseBlock(block: unknown): block is ToolResponseBlock { return typeof block === 'object' && block !== null && 'type' in block && block.type === 'tool_response' && 'callId' in block && 'result' in block; }Then use these guards with
.filter():-const textBlock = c.blocks.find((b) => b.type === 'text') as TextBlock | undefined; +const textBlock = c.blocks.find(isTextBlock); -const textBlocks = c.blocks.filter((b) => b.type === 'text') as TextBlock[]; +const textBlocks = c.blocks.filter(isTextBlock); -const toolCallBlocks = c.blocks.filter((b) => b.type === 'tool_call') as ToolCallBlock[]; +const toolCallBlocks = c.blocks.filter(isToolCallBlock); -const toolResponseBlocks = c.blocks.filter((b) => b.type === 'tool_response') as ToolResponseBlock[]; +const toolResponseBlocks = c.blocks.filter(isToolResponseBlock);Also applies to: 59-59, 62-62, 84-84
78-78: Consider error handling for JSON.stringify.
JSON.stringify()can throw iftoolCall.parameters(line 78) ortoolResponseBlock.result(line 90) contain circular references or non-serializable values (e.g., functions, symbols). While the types suggest these should be serializable, defensive error handling could prevent unexpected failures.Add try-catch blocks or use a safe stringify helper:
+function safeStringify(value: unknown): string { + try { + return JSON.stringify(value); + } catch (err) { + return JSON.stringify({ error: 'Failed to serialize value', message: String(err) }); + } +} for (const toolCall of toolCallBlocks) { input.push({ type: 'function_call', call_id: normalizeToOpenAIToolId(toolCall.id), name: toolCall.name, - arguments: JSON.stringify(toolCall.parameters), + arguments: safeStringify(toolCall.parameters), }); }Apply similar changes on line 90 for
toolResponseBlock.result.Also applies to: 90-90
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
packages/core/src/providers/openai-responses/OpenAIResponsesProvider.ts(3 hunks)packages/core/src/providers/openai-responses/__tests__/OpenAIResponsesProvider.toolIdNormalization.test.ts(1 hunks)packages/core/src/providers/openai-responses/buildResponsesInputFromContent.ts(1 hunks)packages/core/src/providers/openai/buildResponsesRequest.toolIdNormalization.test.ts(1 hunks)packages/core/src/providers/openai/buildResponsesRequest.ts(3 hunks)packages/core/src/providers/utils/toolIdNormalization.test.ts(1 hunks)packages/core/src/providers/utils/toolIdNormalization.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Don't useany- Always specify proper types. Useunknownif the type is truly unknown and add proper type guards.
Do not useconsole.logorconsole.debug- Use the sophisticated logging system instead. Log files are written to ~/.llxprt/debug/
Fix all linting errors, including warnings aboutanytypes
Files:
packages/core/src/providers/openai/buildResponsesRequest.toolIdNormalization.test.tspackages/core/src/providers/utils/toolIdNormalization.tspackages/core/src/providers/openai/buildResponsesRequest.tspackages/core/src/providers/openai-responses/OpenAIResponsesProvider.tspackages/core/src/providers/openai-responses/__tests__/OpenAIResponsesProvider.toolIdNormalization.test.tspackages/core/src/providers/openai-responses/buildResponsesInputFromContent.tspackages/core/src/providers/utils/toolIdNormalization.test.ts
🧠 Learnings (2)
📓 Common learnings
Learnt from: e2720pjk
Repo: vybestack/llxprt-code PR: 583
File: packages/core/src/providers/openai/OpenAIProvider.ts:935-959
Timestamp: 2025-11-16T22:51:26.374Z
Learning: In the llxprt-code codebase (packages/core/src/providers/openai/OpenAIProvider.ts), tools like `run_shell_command` use internal streaming only for real-time UI updates during execution, but each tool execution produces exactly ONE final `ToolResponseBlock` containing the full result. The streaming chunks are never sent to the LLM and are not converted into multiple tool messages. The OpenAI Chat Completions API requires that each tool call (tool_call_id) corresponds to exactly one message with role 'tool', so duplicate tool response detection that removes subsequent tool messages with the same tool_call_id is correct and necessary for API compliance.
📚 Learning: 2025-11-16T22:51:26.374Z
Learnt from: e2720pjk
Repo: vybestack/llxprt-code PR: 583
File: packages/core/src/providers/openai/OpenAIProvider.ts:935-959
Timestamp: 2025-11-16T22:51:26.374Z
Learning: In the llxprt-code codebase (packages/core/src/providers/openai/OpenAIProvider.ts), tools like `run_shell_command` use internal streaming only for real-time UI updates during execution, but each tool execution produces exactly ONE final `ToolResponseBlock` containing the full result. The streaming chunks are never sent to the LLM and are not converted into multiple tool messages. The OpenAI Chat Completions API requires that each tool call (tool_call_id) corresponds to exactly one message with role 'tool', so duplicate tool response detection that removes subsequent tool messages with the same tool_call_id is correct and necessary for API compliance.
Applied to files:
packages/core/src/providers/openai/buildResponsesRequest.toolIdNormalization.test.tspackages/core/src/providers/utils/toolIdNormalization.tspackages/core/src/providers/openai/buildResponsesRequest.tspackages/core/src/providers/openai-responses/OpenAIResponsesProvider.tspackages/core/src/providers/openai-responses/__tests__/OpenAIResponsesProvider.toolIdNormalization.test.tspackages/core/src/providers/openai-responses/buildResponsesInputFromContent.ts
🧬 Code graph analysis (7)
packages/core/src/providers/openai/buildResponsesRequest.toolIdNormalization.test.ts (1)
packages/core/src/providers/openai/buildResponsesRequest.ts (1)
buildResponsesRequest(103-382)
packages/core/src/providers/utils/toolIdNormalization.ts (1)
packages/a2a-server/src/agent/executor.ts (1)
id(53-55)
packages/core/src/providers/openai/buildResponsesRequest.ts (1)
packages/core/src/providers/utils/toolIdNormalization.ts (1)
normalizeToOpenAIToolId(27-59)
packages/core/src/providers/openai-responses/OpenAIResponsesProvider.ts (3)
packages/core/src/providers/utils/toolIdNormalization.ts (1)
normalizeToOpenAIToolId(27-59)packages/core/src/providers/openai/OpenAIProvider.ts (1)
normalizeToOpenAIToolId(913-936)packages/core/src/providers/openai-vercel/OpenAIVercelProvider.ts (1)
normalizeToOpenAIToolId(436-459)
packages/core/src/providers/openai-responses/__tests__/OpenAIResponsesProvider.toolIdNormalization.test.ts (2)
packages/core/src/providers/utils/toolIdNormalization.ts (1)
normalizeToOpenAIToolId(27-59)packages/core/src/providers/openai-responses/buildResponsesInputFromContent.ts (1)
buildResponsesInputFromContent(35-101)
packages/core/src/providers/openai-responses/buildResponsesInputFromContent.ts (2)
packages/core/src/services/history/IContent.ts (2)
TextBlock(101-104)ToolResponseBlock(128-145)packages/core/src/providers/utils/toolIdNormalization.ts (1)
normalizeToOpenAIToolId(27-59)
packages/core/src/providers/utils/toolIdNormalization.test.ts (1)
packages/core/src/providers/utils/toolIdNormalization.ts (1)
normalizeToOpenAIToolId(27-59)
⏰ Context from checks skipped due to timeout of 270000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Test (windows-latest, 24.x)
- GitHub Check: Test (macos-latest, 24.x)
- GitHub Check: Test (ubuntu-latest, 24.x)
- GitHub Check: E2E Test (macOS)
- GitHub Check: E2E Test (Linux) - sandbox:docker
- GitHub Check: Slow E2E - Win
- GitHub Check: E2E Test (Linux) - sandbox:none
🔇 Additional comments (12)
packages/core/src/providers/openai-responses/OpenAIResponsesProvider.ts (3)
33-33: LGTM on the import.The shared utility import is appropriate and follows the project's module organization pattern.
485-493: LGTM on function_call normalization.Correctly normalizes
toolCall.idto OpenAI format before including it in thefunction_callitem. This ensures that internal IDs likehist_tool_*ortoolu_*are converted tocall_*format expected by the Responses API.
500-511: LGTM on function_call_output normalization.The matching normalization applied to
toolResponseBlock.callIdensures consistency between function_call and function_call_output entries, directly addressing issue #825.packages/core/src/providers/openai-responses/__tests__/OpenAIResponsesProvider.toolIdNormalization.test.ts (3)
31-86: Good unit test coverage for the normalization utility.The tests cover all key ID format transformations (hist_tool_, toolu_, call_), sanitization of invalid characters, determinism, and edge cases like empty suffixes. This provides confidence in the normalization logic.
88-149: Critical integration test for issue #825.This test validates the exact scenario from issue #825 - cancelled tool calls with
hist_tool_*IDs. Line 148's assertion thatfunctionCallItem?.call_idequalsfunctionCallOutputItem?.call_idis the key verification that the fix works correctly.
251-311: Good coverage of the cancelled tool call scenario.This test directly mirrors the user scenario from issue #825 where a tool call is cancelled and the conversation continues. The normalization ensures the API can match the function_call_output back to the function_call.
packages/core/src/providers/openai/buildResponsesRequest.ts (3)
18-18: LGTM on the import.Correctly imports the shared normalization utility.
217-230: LGTM on function_call normalization.The normalization ensures tool IDs from AI messages are converted to OpenAI format before being added to the request.
251-257: LGTM on function_call_output normalization.Consistent normalization applied to tool response IDs, ensuring they match the corresponding function_call entries.
packages/core/src/providers/utils/toolIdNormalization.test.ts (1)
25-129: Comprehensive unit test suite for the normalization utility.Excellent coverage across:
- All supported ID format prefixes (call_, hist_tool_, toolu_, unknown)
- Edge cases (empty suffixes, invalid characters only)
- Determinism and consistency guarantees
- The exact ID format from issue #825 (line 57-62)
The consistency tests (lines 113-128) are particularly important as they verify the core requirement that tool_call and tool_response IDs normalize to the same value.
packages/core/src/providers/openai/buildResponsesRequest.toolIdNormalization.test.ts (1)
26-258: Solid integration test coverage for buildResponsesRequest.The tests effectively verify that the normalization flows correctly through
buildResponsesRequest:
- Converts
hist_tool_*IDs tocall_*format- Converts
toolu_*IDs tocall_*format- Preserves already-valid
call_*IDs- Handles cancelled tool call scenarios (the exact issue #825 case)
Each test validates the critical invariant that
function_callandfunction_call_outputentries have matchingcall_idvalues.packages/core/src/providers/openai-responses/buildResponsesInputFromContent.ts (1)
49-55: Address asymmetry in text block handling for human vs. AI messages.Line 50 uses
.find()to extract only the first text block for human messages, while lines 57-64 concatenate all text blocks for AI messages. AlthoughContentFactory.createUserMessage()creates human messages with a single text block, theIContenttype allows multiple blocks per message. If a human message with multiple text blocks is ever created, the current implementation will silently discard all but the first block. Either consolidate text blocks for human messages like AI messages do, or document this as an intentional limitation.
- Refactor normalizeToOpenAIToolId to extract duplicate logic into normalizeWithPrefix helper function (reduces code duplication) - Change crypto import from namespace to default import for consistency - Fix human message text block handling to use filter+join like OpenAIProvider does (handles multiple text blocks correctly)
Contributor
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-24.x-ubuntu-latest' artifact from the main CI run. |
…fix)
Addresses the critical CodeRabbit review comment that randomUUID() would
generate different IDs for the same input on each invocation, breaking
tool call matching between function_call and function_call_output items.
Changes:
- Replace crypto.randomUUID() with crypto.createHash('sha256')
- Hash is deterministic: same input always produces same output
- This ensures tool_call and tool_response items always have matching IDs
- Added tests to verify determinism across multiple calls
Collaborator
Author
|
Thanks CodeRabbit for catching the critical issue with the non-deterministic ID generation! I've fixed this in commit d914f1f by replacing Added additional tests to verify the determinism:
Both tests verify that calling |
Address reviewer feedback: Since all IDs are normalized to hist_tool_ format when stored in IContent (by each provider's normalizeToHistoryToolId method), the toolu_ -> call_ conversion is unnecessary. Changes: - Remove toolu_ prefix handling from normalizeToOpenAIToolId - Update docs to clarify that hist_tool_ is the canonical storage format - Update tests to focus on hist_tool_ (the actual format we receive) - Keep unknown format handling as fallback for edge cases
Collaborator
Author
Correction on toolu_ handlingCodeRabbit's nitpick about consolidating the Anthropic ( Looking at the architecture more carefully:
The Simplified the code in commit 881ec26 to:
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
packages/core/src/providers/utils/toolIdNormalization.ts (1)
64-65: Consider clarifyingtoolu_prefix handling.The PR summary mentions normalizing
toolu_XXX(Anthropic format) inputs, but there's no explicit code path for this prefix. These IDs are currently handled by the "unknown format" fallback, resulting incall_toolu_XXX. While functional, adding a test case or comment documenting this behavior would clarify intent.Example test case to add:
it('should handle Anthropic toolu_ prefix as unknown format', () => { const result = normalizeToOpenAIToolId('toolu_abc123'); expect(result).toBe('call_toolu_abc123'); });packages/core/src/providers/openai-responses/__tests__/OpenAIResponsesProvider.toolIdNormalization.test.ts (3)
33-37: Consider more specific assertions for exact output verification.The test verifies the output format but not the exact transformation. For deterministic behavior, asserting the exact expected value would catch regressions more precisely.
it('should normalize hist_tool_XXX format to call_XXX format', () => { const result = normalizeToOpenAIToolId('hist_tool_abc123def456'); - expect(result).toMatch(/^call_/); - expect(result).not.toContain('hist_tool_'); + expect(result).toBe('call_abc123def456'); });Also applies to: 39-42
55-60: Add determinism test for fallback ID generation.This test verifies determinism for normal inputs, but the PR summary mentions testing "consistent fallback IDs for edge cases across multiple calls." Consider adding a test that verifies deterministic hash generation when fallback IDs are produced (e.g., empty suffix or invalid characters).
it('should be deterministic for fallback IDs', () => { // Test that fallback hash generation is deterministic const edgeCases = ['hist_tool_', 'hist_tool_---', 'call_']; edgeCases.forEach(id => { const result1 = normalizeToOpenAIToolId(id); const result2 = normalizeToOpenAIToolId(id); expect(result1).toBe(result2); }); });
87-311: Integration tests comprehensively verify ID matching.The tests correctly validate that
function_callandfunction_call_outputitems have matching normalizedcall_idvalues, which is the core fix for issue #825. The coverage across different scenarios (hist_tool_, unknown format, preserved call_, cancelled tools) is thorough.Optional: Add test for
toolu_prefix.The PR summary mentions handling Anthropic-style
toolu_IDs, but there's no test case for this format. Consider adding:it('should normalize Anthropic toolu_ IDs', () => { const content: IContent[] = [ { speaker: 'ai', blocks: [ { type: 'tool_call', id: 'toolu_abc123', name: 'read_file', parameters: { path: '/test.txt' }, }, ], }, { speaker: 'tool', blocks: [ { type: 'tool_response', callId: 'toolu_abc123', result: 'file contents', }, ], }, ]; const input = buildResponsesInputFromContent(content); const functionCallItem = input.find( (item) => typeof item === 'object' && item !== null && 'type' in item && item.type === 'function_call', ) as { type: string; call_id: string } | undefined; const functionCallOutputItem = input.find( (item) => typeof item === 'object' && item !== null && 'type' in item && item.type === 'function_call_output', ) as { type: string; call_id: string } | undefined; expect(functionCallItem?.call_id).toBe('call_toolu_abc123'); expect(functionCallOutputItem?.call_id).toBe('call_toolu_abc123'); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/core/src/providers/openai-responses/__tests__/OpenAIResponsesProvider.toolIdNormalization.test.ts(1 hunks)packages/core/src/providers/utils/toolIdNormalization.test.ts(1 hunks)packages/core/src/providers/utils/toolIdNormalization.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/core/src/providers/utils/toolIdNormalization.test.ts
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Don't useany- Always specify proper types. Useunknownif the type is truly unknown and add proper type guards.
Do not useconsole.logorconsole.debug- Use the sophisticated logging system instead. Log files are written to ~/.llxprt/debug/
Fix all linting errors, including warnings aboutanytypes
Files:
packages/core/src/providers/openai-responses/__tests__/OpenAIResponsesProvider.toolIdNormalization.test.tspackages/core/src/providers/utils/toolIdNormalization.ts
🧠 Learnings (2)
📓 Common learnings
Learnt from: e2720pjk
Repo: vybestack/llxprt-code PR: 583
File: packages/core/src/providers/openai/OpenAIProvider.ts:935-959
Timestamp: 2025-11-16T22:51:26.374Z
Learning: In the llxprt-code codebase (packages/core/src/providers/openai/OpenAIProvider.ts), tools like `run_shell_command` use internal streaming only for real-time UI updates during execution, but each tool execution produces exactly ONE final `ToolResponseBlock` containing the full result. The streaming chunks are never sent to the LLM and are not converted into multiple tool messages. The OpenAI Chat Completions API requires that each tool call (tool_call_id) corresponds to exactly one message with role 'tool', so duplicate tool response detection that removes subsequent tool messages with the same tool_call_id is correct and necessary for API compliance.
📚 Learning: 2025-11-16T22:51:26.374Z
Learnt from: e2720pjk
Repo: vybestack/llxprt-code PR: 583
File: packages/core/src/providers/openai/OpenAIProvider.ts:935-959
Timestamp: 2025-11-16T22:51:26.374Z
Learning: In the llxprt-code codebase (packages/core/src/providers/openai/OpenAIProvider.ts), tools like `run_shell_command` use internal streaming only for real-time UI updates during execution, but each tool execution produces exactly ONE final `ToolResponseBlock` containing the full result. The streaming chunks are never sent to the LLM and are not converted into multiple tool messages. The OpenAI Chat Completions API requires that each tool call (tool_call_id) corresponds to exactly one message with role 'tool', so duplicate tool response detection that removes subsequent tool messages with the same tool_call_id is correct and necessary for API compliance.
Applied to files:
packages/core/src/providers/openai-responses/__tests__/OpenAIResponsesProvider.toolIdNormalization.test.tspackages/core/src/providers/utils/toolIdNormalization.ts
⏰ Context from checks skipped due to timeout of 270000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: E2E Test (Linux) - sandbox:none
- GitHub Check: E2E Test (macOS)
- GitHub Check: E2E Test (Linux) - sandbox:docker
- GitHub Check: Slow E2E - Win
🔇 Additional comments (1)
packages/core/src/providers/utils/toolIdNormalization.ts (1)
35-66: LGTM! Deterministic normalization correctly implemented.The use of
crypto.createHash('sha256')for fallback ID generation ensures the same input always produces the same output, fixing the critical non-determinism issue from the previous review. The three-path logic (alreadycall_,hist_tool_, unknown) correctly handles all expected formats and edge cases.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/core/src/providers/utils/toolIdNormalization.test.ts (1)
47-58: Consider consolidating similar sanitization tests.Lines 47-52 and 54-58 test nearly identical behavior (sanitizing call_XXX IDs with special characters). You could combine them into a single test with multiple assertions or use a parameterized test approach.
Example consolidation:
- it('should sanitize call_XXX with special characters', () => { - const result = normalizeToOpenAIToolId('call_abc-123.def'); - expect(result).toMatch(/^call_/); - expect(result).not.toContain('-'); - expect(result).not.toContain('.'); - }); - - it('should sanitize call_XXX IDs with invalid characters', () => { - const result = normalizeToOpenAIToolId('call_abc-123-def'); - expect(result).toMatch(/^call_/); - expect(result).not.toContain('-'); - }); + it('should sanitize call_XXX with special characters', () => { + const testCases = ['call_abc-123.def', 'call_abc-123-def']; + for (const input of testCases) { + const result = normalizeToOpenAIToolId(input); + expect(result).toMatch(/^call_/); + expect(result).not.toContain('-'); + expect(result).not.toContain('.'); + } + });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/core/src/providers/utils/toolIdNormalization.test.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Don't useany- Always specify proper types. Useunknownif the type is truly unknown and add proper type guards.
Do not useconsole.logorconsole.debug- Use the sophisticated logging system instead. Log files are written to ~/.llxprt/debug/
Fix all linting errors, including warnings aboutanytypes
Files:
packages/core/src/providers/utils/toolIdNormalization.test.ts
🧠 Learnings (1)
📓 Common learnings
Learnt from: e2720pjk
Repo: vybestack/llxprt-code PR: 583
File: packages/core/src/providers/openai/OpenAIProvider.ts:935-959
Timestamp: 2025-11-16T22:51:26.374Z
Learning: In the llxprt-code codebase (packages/core/src/providers/openai/OpenAIProvider.ts), tools like `run_shell_command` use internal streaming only for real-time UI updates during execution, but each tool execution produces exactly ONE final `ToolResponseBlock` containing the full result. The streaming chunks are never sent to the LLM and are not converted into multiple tool messages. The OpenAI Chat Completions API requires that each tool call (tool_call_id) corresponds to exactly one message with role 'tool', so duplicate tool response detection that removes subsequent tool messages with the same tool_call_id is correct and necessary for API compliance.
🧬 Code graph analysis (1)
packages/core/src/providers/utils/toolIdNormalization.test.ts (1)
packages/core/src/providers/utils/toolIdNormalization.ts (1)
normalizeToOpenAIToolId(35-66)
⏰ Context from checks skipped due to timeout of 270000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Test (macos-latest, 24.x)
- GitHub Check: Test (windows-latest, 24.x)
- GitHub Check: Test (ubuntu-latest, 24.x)
- GitHub Check: E2E Test (Linux) - sandbox:docker
- GitHub Check: E2E Test (Linux) - sandbox:none
- GitHub Check: Slow E2E - Win
- GitHub Check: E2E Test (macOS)
🔇 Additional comments (1)
packages/core/src/providers/utils/toolIdNormalization.test.ts (1)
1-171: Excellent test coverage and structure!The test suite comprehensively validates the normalization utility across all code paths:
- Format conversion (hist_tool_XXX → call_XXX) addressing issue #825
- Character sanitization and edge cases
- Deterministic behavior (critical for the fix)
The determinism tests (lines 138-169) are particularly valuable, ensuring consistent fallback ID generation—a key aspect of replacing non-deterministic randomUUID() with SHA-256 hashing.
Since all IDs are normalized to hist_tool_ format before storage, testing toolu_ format handling is unnecessary.
This was referenced Dec 17, 2025
Merged
This was referenced Jan 12, 2026
4 tasks
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
This PR fixes issue #825 where the OpenAI Responses API returns a 400 error when a user cancels a tool call and then continues the conversation.
The Error
Root Cause Analysis
The Problem
When tool calls are cancelled by the user (e.g., disapproving an edit), the tool call and its synthetic response are stored in conversation history. The tool IDs in this history use internal formats like
hist_tool_XXX(history format) ortoolu_XXX(Anthropic format).When the conversation continues and history is sent to OpenAI's Responses API:
OpenAIResponsesProvider.tsconverts history tofunction_callandfunction_call_outputitemshist_tool_XXX)call_idvalues to be incall_XXXformatfunction_call_outputitems to their correspondingfunction_callitemsWhy This Only Happens After Cancellation
call_XXXIDshist_tool_XXX)OpenAIProvider.ts(Chat Completions API) already hadnormalizeToOpenAIToolId()- butOpenAIResponsesProvider.tsdid notSolution
1. Created Shared Utility:
normalizeToOpenAIToolId()Location:
packages/core/src/providers/utils/toolIdNormalization.tsThis function normalizes tool IDs from various formats to OpenAI's expected format:
call_XXX)call_abc123call_abc123(unchanged)hist_tool_XXX)hist_tool_abc123call_abc123toolu_XXX)toolu_abc123call_abc123xyz123call_xyz123Additionally:
[a-zA-Z0-9_]allowed2. Applied Normalization in
OpenAIResponsesProvider.tsApplied to both:
function_callitems (tool calls from AI)function_call_outputitems (tool responses)3. Applied Normalization in
buildResponsesRequest.tsSame fix applied to the request builder used by the Responses API.
4. Created Helper:
buildResponsesInputFromContent.tsA reusable function that builds Responses API input with normalization built-in, for future use and testing.
Testing
Added 37 new tests covering:
Unit Tests (
toolIdNormalization.test.ts) - 16 testscall_XXX)hist_tool_XXX→call_XXX)toolu_XXX→call_XXX)Integration Tests - 21 tests
OpenAIResponsesProvider.toolIdNormalization.test.ts(13 tests)buildResponsesRequest.toolIdNormalization.test.ts(4 tests)buildResponsesInputFromContentintegration (4 tests)All tests verify that:
function_callandfunction_call_outputitems have matchingcall_idvaluescall_hist_tool_ortoolu_prefixes remainFiles Changed
providers/utils/toolIdNormalization.tsproviders/utils/toolIdNormalization.test.tsproviders/openai-responses/OpenAIResponsesProvider.tsproviders/openai-responses/buildResponsesInputFromContent.tsproviders/openai-responses/__tests__/OpenAIResponsesProvider.toolIdNormalization.test.tsproviders/openai/buildResponsesRequest.tsproviders/openai/buildResponsesRequest.toolIdNormalization.test.tsVerification
To verify this fix works:
Closes #825
Summary by CodeRabbit
Bug Fixes
Behavior Changes
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.