Dev Merge #95
Dev Merge #95
Conversation
WalkthroughReorganized model-gating constants and updated OpenAI connector request composition: introduced O3/O4 model identifiers, changed feature gating to use derived model names, removed temperature/top_p from Responses API requests, added image/document size constants, and bumped core and sdk package versions. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant LLMService as LLM Service
participant OpenAI as OpenAI Connector
Client->>LLMService: send request (params, modelEntryName)
LLMService->>LLMService: derive modelName = stripPrefix(modelEntryName)
alt modelName in O3_AND_O4_MODELS
LLMService->>LLMService: skip temperature/top_p/presence/frequency/stop gating
else
LLMService->>LLMService: apply temperature/top_p/presence/frequency/stop
end
LLMService->>LLMService: determine json & system-message handling (using constants)
LLMService->>OpenAI: prepare and send request (body without temperature/top_p for Responses API)
OpenAI-->>LLMService: API response
LLMService-->>Client: return processed response
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 (
|
Fix/gpt 5 config
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/core/src/subsystems/LLMManager/LLM.service/connectors/openai/apiInterfaces/ResponsesApiInterface.ts (1)
846-852: Authorization bug: validateFileSize uses a hard-coded AccessCandidate ("temp")validateFileSize fetches file metadata using AccessCandidate.agent('temp'), which can fail authorization and leaks the intended access model. Pass the actual agentId (as done in ChatCompletions) to consistently authorize reads.
Apply this diff to pass agentId and fix all call sites:
@@ - private async validateFileSize(file: BinaryInput, maxSize: number, fileType: string): Promise<void> { - await file.ready(); - const fileInfo = await file.getJsonData(AccessCandidate.agent('temp')); + private async validateFileSize( + file: BinaryInput, + maxSize: number, + fileType: string, + agentId: string + ): Promise<void> { + await file.ready(); + const fileInfo = await file.getJsonData(AccessCandidate.agent(agentId)); if (fileInfo.size > maxSize) { throw new Error(`${fileType} file size (${fileInfo.size} bytes) exceeds maximum allowed size of ${maxSize} bytes`); } }@@ - await this.validateFileSize(file, MAX_IMAGE_SIZE, 'Image'); + await this.validateFileSize(file, MAX_IMAGE_SIZE, 'Image', agentId); @@ - await this.validateFileSize(file, MAX_DOCUMENT_SIZE, 'Document'); + await this.validateFileSize(file, MAX_DOCUMENT_SIZE, 'Document', agentId);Also applies to: 796-813, 819-841
packages/core/src/subsystems/LLMManager/LLM.service/connectors/openai/apiInterfaces/ChatCompletionsApiInterface.ts (1)
87-116: Bug: system-message gating uses modelEntryName instead of derived modelNameChecks against MODELS_WITHOUT_SYSTEM_MESSAGE_SUPPORT should use the derived modelName (without prefix). Using modelEntryName can miss matches (e.g., 'smythos/o1-mini'), causing invalid system messages to be sent.
Apply this refactor to define modelName once and use it consistently:
@@ - public async prepareRequestBody(params: TLLMPreparedParams): Promise<OpenAI.ChatCompletionCreateParams> { - let messages = await this.prepareMessages(params); + public async prepareRequestBody(params: TLLMPreparedParams): Promise<OpenAI.ChatCompletionCreateParams> { + let messages = await this.prepareMessages(params); + const modelName = params.modelEntryName?.replace(BUILT_IN_MODEL_PREFIX, ''); @@ - if (MODELS_WITHOUT_SYSTEM_MESSAGE_SUPPORT.includes(params.modelEntryName)) { + if (MODELS_WITHOUT_SYSTEM_MESSAGE_SUPPORT.includes(modelName)) { messages = this.convertSystemMessagesToUserMessages(messages); } @@ - const supportsSystemMessages = !MODELS_WITHOUT_SYSTEM_MESSAGE_SUPPORT.includes(params.modelEntryName); + const supportsSystemMessages = !MODELS_WITHOUT_SYSTEM_MESSAGE_SUPPORT.includes(modelName); @@ - const modelName = params.modelEntryName?.replace(BUILT_IN_MODEL_PREFIX, ''); + // modelName defined aboveAlso applies to: 131-135
♻️ Duplicate comments (1)
packages/core/src/subsystems/LLMManager/LLM.service/connectors/openai/apiInterfaces/ChatCompletionsApiInterface.ts (1)
453-459: Nice: file size validation uses the caller’s agent for authorizationThis mirrors the correct access control pattern. Please align ResponsesApiInterface the same way.
🧹 Nitpick comments (3)
packages/core/src/subsystems/LLMManager/LLM.service/connectors/openai/apiInterfaces/ResponsesApiInterface.ts (1)
285-287: Model name normalization should use BUILT_IN_MODEL_PREFIX (avoid hard-coded "smythos/")Using a literal 'smythos/' risks drift if the prefix changes. Prefer the shared BUILT_IN_MODEL_PREFIX constant for consistency with ChatCompletions.
@@ -import { SEARCH_TOOL_COSTS } from './constants'; +import { SEARCH_TOOL_COSTS } from './constants'; +import { BUILT_IN_MODEL_PREFIX } from '@sre/constants'; @@ - const modelName = context.modelEntryName?.replace('smythos/', ''); + const modelName = context.modelEntryName?.replace(BUILT_IN_MODEL_PREFIX, '');Also applies to: 5-16
packages/core/src/subsystems/LLMManager/LLM.service/connectors/openai/apiInterfaces/constants.ts (1)
1-2: Centralizing O3/O4 gating is good; consider resilient detection at call sitesThe explicit list is fine for now, but it risks drift as new O3/O4 variants appear. Prefer using O3_AND_O4_MODELS_PATTERN at call sites (or combining list + pattern) to future-proof gating logic.
packages/core/src/subsystems/LLMManager/LLM.service/connectors/openai/apiInterfaces/ChatCompletionsApiInterface.ts (1)
132-160: Optional: Make O3/O4 gating robust with the pattern to avoid driftCurrent checks rely solely on O3_AND_O4_MODELS membership. Consider augmenting with O3_AND_O4_MODELS_PATTERN to catch new variants (e.g., o3-mini, o4 family).
Example patch:
- const modelName = params.modelEntryName?.replace(BUILT_IN_MODEL_PREFIX, ''); - if (params?.temperature !== undefined && !O3_AND_O4_MODELS.includes(modelName)) { + const isO3OrO4 = O3_AND_O4_MODELS.includes(modelName) || O3_AND_O4_MODELS_PATTERN.test(modelName); + if (params?.temperature !== undefined && !isO3OrO4) { body.temperature = params.temperature; } @@ - if (params?.topP !== undefined && !O3_AND_O4_MODELS.includes(modelName)) { + if (params?.topP !== undefined && !isO3OrO4) { body.top_p = params.topP; } @@ - if (params?.frequencyPenalty !== undefined && !O3_AND_O4_MODELS.includes(modelName)) { + if (params?.frequencyPenalty !== undefined && !isO3OrO4) { body.frequency_penalty = params.frequencyPenalty; } @@ - if (params?.presencePenalty !== undefined && !O3_AND_O4_MODELS.includes(modelName)) { + if (params?.presencePenalty !== undefined && !isO3OrO4) { body.presence_penalty = params.presencePenalty; } @@ - if (params?.stopSequences?.length && !O3_AND_O4_MODELS.includes(modelName)) { + if (params?.stopSequences?.length && !isO3OrO4) { body.stop = params.stopSequences; }Note: This requires importing O3_AND_O4_MODELS_PATTERN from './constants'.
📜 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 (3)
packages/core/src/subsystems/LLMManager/LLM.service/connectors/openai/apiInterfaces/ChatCompletionsApiInterface.ts(2 hunks)packages/core/src/subsystems/LLMManager/LLM.service/connectors/openai/apiInterfaces/ResponsesApiInterface.ts(1 hunks)packages/core/src/subsystems/LLMManager/LLM.service/connectors/openai/apiInterfaces/constants.ts(1 hunks)
🔇 Additional comments (5)
packages/core/src/subsystems/LLMManager/LLM.service/connectors/openai/apiInterfaces/ResponsesApiInterface.ts (2)
7-11: Imports cleanup and constants usage look goodTrimming LLM.types imports and centralizing SEARCH_TOOL_COSTS import improves cohesion and reduces coupling. No issues.
545-554: Confirm sampling controls (temperature/top_p) in ResponsesApiInterfaceThe recent change removes any setting of
temperature/top_pwhen building the request body foropenai.responses.create. If the Responses endpoint still supports these sampling parameters for non-O3/O4 models, we should re-introduce gated handling (as done in ChatCompletions).Please:
- Verify in the OpenAI Responses API docs whether
temperatureandtop_pare accepted.- If they are supported, update
prepareRequestBodyin
packages/core/src/subsystems/LLMManager/LLM.service/connectors/openai/apiInterfaces/ResponsesApiInterface.ts
(around lines 543–554) to only set those fields for models outside the O3/O4 families.Example diff for reference:
+ import { O3_AND_O4_MODELS } from './constants'; + import { BUILT_IN_MODEL_PREFIX } from '@sre/constants'; @@ prepareRequestBody(params) { const body: OpenAI.Responses.ResponseCreateParams = { model: params.model as string, input, }; + // Sampling controls for models that support it (exclude O3/O4) + const modelName = params.modelEntryName?.replace(BUILT_IN_MODEL_PREFIX, ''); + if (params.temperature !== undefined && !O3_AND_O4_MODELS.includes(modelName)) { + (body as any).temperature = params.temperature; + } + if (params.topP !== undefined && !O3_AND_O4_MODELS.includes(modelName)) { + (body as any).top_p = params.topP; + }packages/core/src/subsystems/LLMManager/LLM.service/connectors/openai/apiInterfaces/constants.ts (1)
10-13: SEARCH_TOOL_COSTS: Simple, readable configThe prefix match strategy downstream will handle sub-variants like gpt-4o. Looks good.
packages/core/src/subsystems/LLMManager/LLM.service/connectors/openai/apiInterfaces/ChatCompletionsApiInterface.ts (2)
8-10: Good import reorg and prefix normalization sourceUsing BUILT_IN_MODEL_PREFIX from shared constants will keep derived model name logic consistent across interfaces.
83-120: Verified: only ChatCompletionsApiInterface.ts usesMODELS_WITHOUT_SYSTEM_MESSAGE_SUPPORT.includes(params.modelEntryName)
A repository-wide search returned only the two instances already in
packages/core/src/subsystems/LLMManager/LLM.service/connectors/openai/apiInterfaces/ChatCompletionsApiInterface.ts(lines 87 and 93). No other files use this gating, so no further changes are needed.
2 participants
Summary by CodeRabbit
Chores
Behavior changes