Dev Merge #92
Dev Merge #92
Conversation
WalkthroughThis PR bumps package versions and refactors OpenAI integration internals. It consolidates prompt token counting, overhauls Responses API streaming and tool handling, simplifies search tool cost constants, adjusts related types (adding callId), and updates unit tests accordingly. Public API adds processFunctionCallResults and modifies transformToolsConfig output and input shapes. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant ResponsesAPI
participant EventRouter as Event Handlers
participant ToolsData as Tools Store
Client->>ResponsesAPI: stream()
loop Stream events
ResponsesAPI->>EventRouter: route(event)
alt web_search events
EventRouter->>ToolsData: upsert web_search state (immutable)
else function_call delta/done
EventRouter->>ToolsData: update function args/results
else output text/item
EventRouter->>ToolsData: append output fragments
end
EventRouter-->>ResponsesAPI: finish status?
end
ResponsesAPI-->>Client: final output + tools data
sequenceDiagram
participant Caller
participant RespAPI as ResponsesAPI
participant Transformer as Tool/Message Transformer
Caller->>RespAPI: request(messages, toolsConfig, toolChoice)
RespAPI->>Transformer: applyToolMessageTransformation(messages)
RespAPI->>Transformer: transformToolsConfig(toolsConfig)
Transformer-->>RespAPI: normalized messages + tools (strict flag)
RespAPI->>RespAPI: validateToolChoice()
RespAPI-->>Caller: prepared request payload
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type 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: 1
🔭 Outside diff range comments (1)
packages/core/src/types/LLM.types.ts (1)
285-297: ToolData.callId — propagation verified; please add clarifying commentsQuick summary: ResponsesApiInterface already sets/preserves callId and uses it when emitting tool events, so propagation is implemented end-to-end. Please add a short comment clarifying semantics of id vs callId to avoid future confusion.
Files/locations to note
- packages/core/src/subsystems/LLMManager/LLM.service/connectors/openai/apiInterfaces/ResponsesApiInterface.ts
- process/stream handlers extract item.call_id and set/preserve tool.callId (e.g. creation at ~lines 381–396).
- Mapping/emits use callId when available:
- id: tool.callId || tool.id and callId: tool.callId (~lines 226–229)
- emitter.emit('tool_call_started', { id: callId, ... }) (~line 416)
- emitter.emit('tool_call_progress', { id: entry.callId || itemId, ... }) (~line 462)
- emitter.emit('tool_call_completed', { id: updatedEntry.callId || itemId, ... }) (~line 493)
- packages/core/src/types/LLM.types.ts
- ToolData includes callId?: string; add explicit comments describing the difference between id and callId.
- packages/core/src/subsystems/IO/Log.service/ (LogConnector.ts, ConsoleLog.class.ts)
- These APIs also accept a callId parameter for logging — confirm this uses the same convention or rename to avoid ambiguity.
Suggested minimal ToolData update (inline comment clarification)
export type ToolData = { index: number; id: string; // Chat Completions tool-call identifier (item.id) type: string; name: string; arguments: string | Record<string, any>; role: 'user' | 'tool' | 'assistant'; result?: string; function?: any; error?: string; // for Bedrock callId?: string; // OpenAI Responses API call_id (streaming mapping); prefer callId for Responses API flows };Category:
🧹 Nitpick comments (12)
packages/core/src/subsystems/LLMManager/LLM.service/connectors/openai/OpenAIConnector.class.ts (2)
86-90: Good consolidation of token counting; remove unused lastMessage.lastMessage is computed but not used after introducing computePromptTokens. Clean it up to avoid noise.
Apply this diff:
- const messages = _body?.messages || []; - const lastMessage = messages[messages.length - 1]; - const promptTokens = await this.computePromptTokens(messages, context); + const messages = _body?.messages || []; + const promptTokens = await this.computePromptTokens(messages, context);
143-147: Same here: remove unused lastMessage in streamRequest.Apply this diff:
- const messages = body?.messages || body?.input || []; - const lastMessage = messages[messages.length - 1]; - const promptTokens = await this.computePromptTokens(messages, context); + const messages = body?.messages || body?.input || []; + const promptTokens = await this.computePromptTokens(messages, context);packages/core/src/subsystems/LLMManager/LLM.service/connectors/openai/apiInterfaces/constants.ts (1)
10-13: Type the SEARCH_TOOL_COSTS map and freeze it; consider a safe default in consumers for unknown models.Untyped constants make it easier to accidentally assign non-numeric values. Typing and freezing improves safety. Also ensure consumer code handles unknown models gracefully (e.g., cost = 0 with a warning).
Apply this diff:
-export const SEARCH_TOOL_COSTS = { - 'gpt-4': 25 / 1000, - 'gpt-5': 10 / 1000, -}; +export const SEARCH_TOOL_COSTS: Readonly<Record<string, number>> = Object.freeze({ + 'gpt-4': 25 / 1000, + 'gpt-5': 10 / 1000, +});packages/core/src/subsystems/LLMManager/LLM.service/connectors/openai/apiInterfaces/ResponsesApiInterface.ts (9)
48-63: Consider typing the legacy 'started' event for consistencyYou already route the 'started' legacy event; adding a small interface (like the in_progress/searching/completed types) would keep handlers fully typed and avoid any casts.
149-206: Per-event try/catch is robust; capture non-stop finish reasons if presentCurrent logic always resolves finishReason to 'stop'. If the SDK emits distinct terminal reasons (e.g., cancelled/incomplete/error), consider extracting a more specific reason from part.response or the event type and forwarding it to the 'interrupted' emitter for better UX/telemetry.
298-300: Minor: avoid double-normalization of model namegetSearchToolCost already normalizes the smythos/ prefix. You can simplify calculateSearchToolUsage accordingly.
Apply this diff to simplify:
- const modelName = context.modelEntryName?.replace('smythos/', ''); - const cost = this.getSearchToolCost(modelName); + const cost = this.getSearchToolCost(context.modelEntryName);
376-430: Function call start handling is solid; consider initializing arguments explicitly as JSON when knownYou initialize arguments as '' until deltas arrive. That’s fine, but if initial arguments are provided on item add (some SDK events include them), prefer persisting the provided string intact to reduce intermediate states.
543-549: Enrich completion event mapping for better 'interrupted' semanticsCurrently all unknown/done-like events map to 'stop'. Consider distinguishing common terminal variants.
Apply this diff:
- private handleCompletionEvent(eventType: string): string { - if (eventType === EVENT_TYPES.RESPONSE_COMPLETED || eventType.includes('done')) { - return 'stop'; - } - return 'stop'; // Default finish reason - } + private handleCompletionEvent(eventType: string): string { + if (eventType === EVENT_TYPES.RESPONSE_COMPLETED || eventType.endsWith('.completed')) return 'stop'; + if (eventType.endsWith('.cancelled') || eventType.endsWith('.incomplete')) return 'cancelled'; + if (eventType.endsWith('.errored') || eventType.endsWith('.error')) return 'error'; + return 'stop'; + }
606-629: Clean up unreachable branch when applying tool_choicevalidateToolChoice returns true only for known shapes. The “pass-through with type assertion” branch is effectively unreachable and can be removed to reduce ambiguity.
Apply this diff:
- } else if (typeof toolChoice === 'object' && toolChoice !== null) { + } else if (typeof toolChoice === 'object' && toolChoice !== null) { // Handle object-based tool choices (specific function selection) if ('type' in toolChoice && toolChoice.type === 'function' && 'function' in toolChoice && 'name' in toolChoice.function) { // Transform Chat Completions specific function choice to Responses API format body.tool_choice = { type: 'function', name: toolChoice.function.name, }; - } else { - // For other object formats, pass through with type assertion - body.tool_choice = toolChoice as any; }
642-714: transformToolsConfig: propagate strict when provided and use safer defaults for parameters
- Preserve strict if present on the source (nested in function for Chat Completions or top-level for Responses tools) instead of hardcoding false.
- Use {} and [] rather than undefined for parameters.properties/required to avoid server-side schema validation issues.
Apply these diffs:
- return { + return { type: 'function' as const, name: undefined, description: undefined, parameters: { type: 'object', - properties: undefined, - required: undefined, + properties: {}, + required: [], }, - strict: false, + strict: false, } as OpenAI.Responses.Tool;- if (!funcTool.name || typeof funcTool.name !== 'string') { + if (!funcTool.name || typeof funcTool.name !== 'string') { return { type: 'function' as const, name: undefined, description: tool.description || '', - parameters: { type: 'object', properties: undefined, required: undefined }, - strict: false, + parameters: { type: 'object', properties: {}, required: [] }, + strict: Boolean((funcTool as any).strict), } as OpenAI.Responses.Tool; }- return { + return { type: 'function' as const, name: funcTool.name, description: funcTool.description || tool.description || '', parameters: funcTool.parameters || { type: 'object', properties: {}, required: [] }, - strict: false, + strict: Boolean((funcTool as any).strict), } as OpenAI.Responses.Tool;- if ('parameters' in tool) { + if ('parameters' in tool) { return { type: 'function' as const, name: tool.name, description: tool.description || '', parameters: tool.parameters || { type: 'object', properties: {}, required: [] }, - strict: false, + strict: typeof (tool as any).strict === 'boolean' ? (tool as any).strict : false, } as OpenAI.Responses.Tool; }
989-1030: Prefer deterministic fallback call_id for testability and reproducibilityUsing Date.now() produces non-deterministic artifacts. You can derive a stable fallback from loop indices.
Apply this diff:
- transformedMessages.push({ + transformedMessages.push({ type: 'function_call', name: toolCall.function.name || '', arguments: normalizedArgs, - call_id: toolCall.id || toolCall.call_id || `call_${Date.now()}_${index}`, // Ensure unique ID + call_id: toolCall.id || toolCall.call_id || `call_${i}_${index}`, // Ensure unique and deterministic });
309-312: Consider adding focused unit tests for the new event handlersapplyToolMessageTransformation is covered, but the streaming handlers (output_item.added, function_call_arguments.delta/done, output_item.done, and web search events) would benefit from unit tests to prevent regressions.
I can scaffold vitest tests that simulate these stream events and assert the emitted events and toolsData mutations. Want me to add those?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (8)
packages/core/package.json(1 hunks)packages/core/src/subsystems/LLMManager/LLM.service/connectors/openai/OpenAIConnector.class.ts(3 hunks)packages/core/src/subsystems/LLMManager/LLM.service/connectors/openai/apiInterfaces/ResponsesApiInterface.ts(10 hunks)packages/core/src/subsystems/LLMManager/LLM.service/connectors/openai/apiInterfaces/constants.ts(1 hunks)packages/core/src/subsystems/LLMManager/LLM.service/connectors/openai/types.ts(0 hunks)packages/core/src/types/LLM.types.ts(1 hunks)packages/core/tests/unit/openai/ResponsesApiInterface.unit.test.ts(2 hunks)packages/sdk/package.json(1 hunks)
💤 Files with no reviewable changes (1)
- packages/core/src/subsystems/LLMManager/LLM.service/connectors/openai/types.ts
🧰 Additional context used
🧬 Code Graph Analysis (2)
packages/core/src/subsystems/LLMManager/LLM.service/connectors/openai/OpenAIConnector.class.ts (4)
packages/core/src/subsystems/MemoryManager/LLMContext.ts (1)
messages(28-30)packages/core/src/helpers/Conversation.helper.ts (1)
context(70-72)packages/core/src/types/LLM.types.ts (1)
ILLMRequestContext(450-459)packages/core/src/subsystems/LLMManager/LLM.helper.ts (1)
LLMHelper(8-251)
packages/core/src/subsystems/LLMManager/LLM.service/connectors/openai/apiInterfaces/ResponsesApiInterface.ts (3)
packages/core/src/types/LLM.types.ts (3)
ToolData(286-297)TLLMParams(90-164)LLMModelInfo(12-12)packages/core/src/subsystems/LLMManager/LLM.service/connectors/openai/apiInterfaces/OpenAIApiInterface.ts (1)
ToolConfig(10-15)packages/core/src/subsystems/LLMManager/LLM.service/connectors/openai/apiInterfaces/constants.ts (1)
SEARCH_TOOL_COSTS(10-13)
🔇 Additional comments (10)
packages/core/package.json (1)
3-3: Version bump looks good.No concerns on this change.
packages/sdk/package.json (1)
3-3: SDK version bump looks good.No concerns on this change.
packages/core/tests/unit/openai/ResponsesApiInterface.unit.test.ts (2)
60-61: Test update for strict: false looks correctAsserting strict: false aligns with the new Responses Tool shape returned by transformToolsConfig.
770-771: Graceful handling of malformed tool config is well-coveredIncluding strict: false in the error-path expectation keeps the test aligned with the new default tool transformation.
packages/core/src/subsystems/LLMManager/LLM.service/connectors/openai/apiInterfaces/ResponsesApiInterface.ts (6)
313-353: New web search event handlers are clean and side-effect freeThese handlers use an immutable upsert and fail safely. Good separation of concerns.
1070-1080: Prefix-based cost resolution is simple and effectiveThis neatly accommodates variants like gpt-4o/gpt-5*. Looks good.
1086-1118: processFunctionCallResults adds a useful normalization layerNormalizing arguments and providing a consistent function payload will simplify downstream handling.
1145-1167: Immutable upsert for web search tool is cleanGood use of immutable updates and returning the index for chaining.
226-230: No downstream breakage found — using callId as emitted id looks safeQuick verification summary:
- I searched usages of ToolInfo / toolsData and callId across the repo.
- The OpenAI Responses stream handlers already track both item_id and call_id and preserve callId internally; extractToolCalls maps the final id to callId || id and also keeps callId on the object.
- Downstream consumers (packages/core/src/helpers/Conversation.helper.ts, packages/core/src/subsystems/LLMManager/LLM.inference.ts, connectors' transformToolMessageBlocks, and the SDK Chat/Agent forwarders) use tool.index, tool.arguments, tool.name, or explicitly honor callId when emitting progress/completion events — none require id to be the original item_id.
Files inspected (representative):
- packages/core/src/subsystems/LLMManager/LLM.service/connectors/openai/apiInterfaces/ResponsesApiInterface.ts (handles item_id / call_id, extractToolCalls)
- packages/core/src/helpers/Conversation.helper.ts (consumes toolsData; uses index to drive tool calls)
- packages/core/src/subsystems/LLMManager/LLM.inference.ts (uses toolsData for context/token accounting and passes to connectors)
- packages/sdk/src/LLM/Chat.class.ts and packages/sdk/src/Agent/Agent.class.ts (forward ToolInfo)
Conclusion: no code in this repository relies on id being the original item_id-only; keeping id: callId || id and exposing callId is consistent and safe.
31-46: Centralized event strings — verified (OpenAI SDK ^5.12.2, no other raw occurrences found)package.json shows openai: ^5.12.2, and a repo-wide search for raw response.* event strings returned only the EVENTS definition below; no other hard‑coded occurrences were found.
- packages/core/src/subsystems/LLMManager/LLM.service/connectors/openai/apiInterfaces/ResponsesApiInterface.ts — defines EVENT_TYPES (the only matches)
- packages/core/package.json — "openai": "^5.12.2"
No action required; EVENT_TYPES can stay as the single source of truth.
packages/core/src/subsystems/LLMManager/LLM.service/connectors/openai/OpenAIConnector.class.ts
Show resolved
Hide resolved
2 participants
Summary by CodeRabbit