Added OpenAI compatible "input" field support in /v1/responses API#69
Added OpenAI compatible "input" field support in /v1/responses API#69
Conversation
|
Claude finished @olesho's task —— View job Code Review Complete ✅Reviewing PR #69: "Added OpenAI compatible 'input' field support in /v1/responses API" Todo List:
SummaryThis PR successfully adds OpenAI Responses API compatible input field support, enabling both string and conversation array formats. The implementation is comprehensive, covering backend validation, frontend message handling, and documentation updates. ✅ StrengthsCode Quality & Architecture
Implementation Quality
Security & Best Practices
|
WalkthroughThese changes add LiteLLM provider support alongside OpenAI, with per-model tier endpoints, enhanced input validation for conversation history, and system prompt configuration overrides integrated throughout the evaluation pipeline from frontend to backend. Changes
Sequence DiagramsequenceDiagram
participant UI as Frontend UI
participant Agent as EvaluationAgent
participant Config as LLMConfigurationManager
participant API as Backend API Server
UI->>Agent: initiate chat evaluation<br/>(input: string|array|object)
Agent->>Agent: extract nested model config<br/>(main/mini/nano/endpoint)
Agent->>Config: getConfiguration()
Config-->>Agent: return LLMConfig<br/>(with systemPrompt override)
Agent->>Agent: merge model config + override
Agent->>Agent: normalize input to unified shape<br/>(array→conversation, string→{message})
Agent->>API: POST /v1/responses<br/>(mergedInput, modelConfig)
API->>API: validate input format<br/>(string or OpenAI messages array)
API->>API: extract per-tier endpoints<br/>(main/mini/nano or fallback)
API->>API: create nested request<br/>(with provider+endpoint)
API-->>Agent: response
Agent->>Agent: attach tracing metadata<br/>(hasConversationHistory, hasSystemPrompt)
Agent-->>UI: return result
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 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]](proxy.php?url=https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
agent-server/nodejs/src/api-server.js (2)
574-582: Consider object property shorthand for consistency.The
providervariable is assigned and then used in the return object. While the code is correct, the static analysis tool suggests using ES6 object property shorthand for consistency with modern JavaScript conventions.Apply this diff to use property shorthand:
- const provider = defaults.provider || 'openai'; - return { - provider: provider, + provider: defaults.provider || 'openai', model: defaultModels[tier], api_key: process.env.OPENAI_API_KEY, // Add endpoint for litellm provider with env var fallback - endpoint: defaults.endpoint || (provider === 'litellm' ? process.env.LITELLM_ENDPOINT : undefined) + endpoint: defaults.endpoint || ((defaults.provider || 'openai') === 'litellm' ? process.env.LITELLM_ENDPOINT : undefined) };Alternatively, if you prefer to keep the variable for clarity, you can use shorthand like this:
const provider = defaults.provider || 'openai'; return { - provider: provider, + provider, model: defaultModels[tier], api_key: process.env.OPENAI_API_KEY, // Add endpoint for litellm provider with env var fallback endpoint: defaults.endpoint || (provider === 'litellm' ? process.env.LITELLM_ENDPOINT : undefined) };
523-559: Minor: JSDoc formatting style.The static analysis tool suggests removing hyphens before
@paramdescriptions (lines 527-529) to align with JSDoc conventions. This is a style preference and doesn't affect functionality.If you'd like to align with the linter rules, you can remove the hyphens:
/** * Extract model tier configuration from request * Handles both nested format { provider: "litellm", model: "qwen3:14b", api_key: "..." } * and flat format "qwen3:14b" - * @param {'main' | 'mini' | 'nano'} tier - Model tier - * @param {any} tierConfig - Configuration from request (can be object or string) - * @param {Object} defaults - Default configuration from config.yaml - * @returns {Object} Extracted configuration (without endpoint) + * @param {'main' | 'mini' | 'nano'} tier Model tier + * @param {any} tierConfig Configuration from request (can be object or string) + * @param {Object} defaults Default configuration from config.yaml + * @returns {Object} Extracted configuration (without endpoint) */The same applies to the JSDoc for
createDynamicRequestNestedat lines 655-656.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
agent-server/README.md(2 hunks)agent-server/nodejs/CLAUDE.md(2 hunks)agent-server/nodejs/README.md(1 hunks)agent-server/nodejs/src/api-server.js(6 hunks)front_end/panels/ai_chat/core/GraphHelpers.ts(2 hunks)front_end/panels/ai_chat/core/LLMConfigurationManager.ts(2 hunks)front_end/panels/ai_chat/evaluation/remote/EvaluationAgent.ts(6 hunks)front_end/panels/ai_chat/evaluation/remote/EvaluationProtocol.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
front_end/panels/ai_chat/evaluation/remote/EvaluationAgent.ts (4)
front_end/panels/ai_chat/core/AgentService.ts (1)
logger(378-389)front_end/panels/ai_chat/models/ChatTypes.ts (1)
ChatMessage(74-75)front_end/panels/ai_chat/ui/ChatView.ts (1)
m(1384-1406)front_end/panels/ai_chat/tracing/TracingConfig.ts (1)
withTracingContext(313-315)
front_end/panels/ai_chat/core/GraphHelpers.ts (1)
front_end/panels/ai_chat/core/LLMConfigurationManager.ts (1)
LLMConfigurationManager(44-433)
🪛 ESLint
front_end/panels/ai_chat/evaluation/remote/EvaluationAgent.ts
[error] 19-19: There should be at least one empty line between import groups
(import/order)
[error] 345-345: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
[error] 559-559: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
[error] 564-564: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
[error] 565-565: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
[error] 566-566: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
[error] 567-567: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
[error] 568-568: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
[error] 568-568: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
[error] 569-569: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
[error] 569-569: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
[error] 901-908: Unnecessary 'else' after 'return'.
(no-else-return)
[error] 925-925: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
[error] 928-928: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
[error] 953-953: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
[error] 1017-1017: Expected property shorthand.
(object-shorthand)
[error] 1018-1018: Expected property shorthand.
(object-shorthand)
[error] 1019-1019: Expected property shorthand.
(object-shorthand)
[error] 1020-1020: Expected property shorthand.
(object-shorthand)
[error] 1028-1028: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
[error] 1029-1029: Expected property shorthand.
(object-shorthand)
[error] 1030-1030: Expected property shorthand.
(object-shorthand)
[error] 1031-1031: Expected property shorthand.
(object-shorthand)
[error] 1032-1032: Expected property shorthand.
(object-shorthand)
[error] 1071-1071: Forbidden non-null assertion.
(@typescript-eslint/no-non-null-assertion)
[error] 1072-1072: Forbidden non-null assertion.
(@typescript-eslint/no-non-null-assertion)
agent-server/nodejs/src/api-server.js
[error] 527-527: There must be no hyphen before @param description.
(jsdoc/require-hyphen-before-param-description)
[error] 528-528: There must be no hyphen before @param description.
(jsdoc/require-hyphen-before-param-description)
[error] 529-529: There must be no hyphen before @param description.
(jsdoc/require-hyphen-before-param-description)
[error] 577-577: Expected property shorthand.
(object-shorthand)
[error] 655-655: There must be no hyphen before @param description.
(jsdoc/require-hyphen-before-param-description)
[error] 656-656: There must be no hyphen before @param description.
(jsdoc/require-hyphen-before-param-description)
front_end/panels/ai_chat/evaluation/remote/EvaluationProtocol.ts
[error] 92-92: Expected a comma.
(@stylistic/member-delimiter-style)
front_end/panels/ai_chat/core/GraphHelpers.ts
[error] 10-10: ./LLMConfigurationManager.js import should occur before import of ./Logger.js
(import/order)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: claude-review
🔇 Additional comments (10)
agent-server/nodejs/CLAUDE.md (3)
73-117: LGTM! Clear documentation for LiteLLM support.The request examples for OpenAI and LiteLLM providers are well-structured and clearly illustrate the endpoint configuration options. The fallback chain (per-tier endpoint → top-level endpoint → environment variable) is properly documented.
118-159: Well-documented conversation format with clear constraints.The Conversation State format documentation clearly explains the message structure, roles, and requirements. The constraints (max 100 messages, 10,000 characters each, at least one user message) are explicitly stated.
160-177: Response format documentation is clear and complete.The OpenAI-compatible response format with metadata is well-documented. The note about using clientId and tabId for screenshot capture provides important context for API consumers.
agent-server/README.md (1)
78-136: Comprehensive documentation for LiteLLM provider support.The documentation clearly explains both OpenAI and LiteLLM request formats, with explicit endpoint configuration guidance. The priority order (per-tier → top-level → environment variable) helps users understand the fallback behavior.
agent-server/nodejs/src/api-server.js (4)
344-396: Robust input validation for conversation arrays.The validation logic thoroughly checks the conversation array format:
- Type validation (string or array)
- Array length constraints (1-100 messages)
- Per-message validation (role, content, type, length)
- Required user message check
The error messages are clear and include context (message index), which aids debugging.
405-429: Improved logging with privacy-conscious formatting.The logging enhancements appropriately handle sensitive data:
- Endpoint information is marked as '[configured]' rather than exposed
- Long inputs are truncated
- Conversation arrays are summarized by message count
- Input type is explicitly logged for debugging
486-521: Proper endpoint configuration extraction with fallback chain.The endpoint extraction logic correctly implements the documented priority:
- Tier-specific endpoint (e.g.,
main_model.endpoint)- Top-level endpoint (
model.endpoint)- Undefined (falls back to environment variable later)
The use of
extractModelTierConfighelper promotes consistency across tiers.
659-701: Input format handling is well-implemented with clear logic.The method correctly handles both input formats:
- String format: Wraps in
{ message: input }for legacy compatibility- Array format: Passes through directly for conversation history
The comments referencing
EvaluationAgent.tsmethods provide helpful context for understanding how the formats are processed downstream.agent-server/nodejs/README.md (2)
390-420: Complete LiteLLM provider documentation with clear endpoint priority.The LiteLLM example and endpoint configuration priority are well-documented. The 4-level priority chain (per-tier → top-level → env var → default) provides clear guidance for users configuring custom endpoints.
421-466: Comprehensive conversation format documentation.The Conversation State format is thoroughly documented with:
- Example showing all message roles (system, user, assistant)
- Clear role descriptions and use cases
- Explicit constraints matching the validation logic
- Requirements for message structure
This provides users with complete information to use the conversation history feature.
| // If it's just a string (model name), use default provider | ||
| if (typeof tierConfig === 'string') { | ||
| return { | ||
| provider: defaults.provider || 'openai', | ||
| model: tierConfig, | ||
| api_key: process.env.OPENAI_API_KEY | ||
| }; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Verify API key selection for string format.
When tierConfig is a string (model name only), the code uses process.env.OPENAI_API_KEY even though the provider is determined from defaults.provider. If the default provider is not 'openai' (e.g., 'litellm'), this could result in using the wrong API key.
Run this script to check if there are scenarios where default provider is not 'openai':
🏁 Script executed:
#!/bin/bash
# Search for config.yaml files that might set a different default provider
rg -n -C3 'provider:.*(?!openai)' --type yaml
# Check if LITELLM_ENDPOINT or other provider env vars are used as defaults
rg -n 'provider.*litellm|groq|openrouter' --type js --type yamlLength of output: 750
🏁 Script executed:
#!/bin/bash
# Get the full extractModelTierConfig function
ast-grep --pattern $'function extractModelTierConfig($_) {
$$$
}'Length of output: 0
🏁 Script executed:
#!/bin/bash
# Look for where defaults object is initialized/set
rg -n -B5 -A10 'defaults\s*=.*provider' --type js
# Check what API key env vars are used for different providers
rg -n 'process\.env\.[A-Z_]*API|process\.env\.LITELLM|process\.env\.GROQ|process\.env\.OPENROUTER' --type js | head -20Length of output: 883
🏁 Script executed:
#!/bin/bash
# Check how defaults are passed to extractModelTierConfig
rg -n 'extractModelTierConfig' --type js -B2 -A2Length of output: 1427
🏁 Script executed:
#!/bin/bash
# Get the complete extractModelTierConfig function (lines 532-559)
sed -n '532,559p' agent-server/nodejs/src/api-server.jsLength of output: 851
🏁 Script executed:
#!/bin/bash
# Find where defaults object is created before calling extractModelTierConfig
rg -n -B20 'extractModelTierConfig.*main' agent-server/nodejs/src/api-server.js | head -50Length of output: 1204
🏁 Script executed:
#!/bin/bash
# Check if there's a mapping of provider to API key env var name
rg -n -C5 'LITELLM_API_KEY|GROQ_API_KEY|OPENROUTER_API_KEY' agent-server/nodejs/src/api-server.jsLength of output: 0
🏁 Script executed:
#!/bin/bash
# Find where this.configDefaults is set
rg -n 'this\.configDefaults\s*=' agent-server/nodejs/src/api-server.jsLength of output: 161
🏁 Script executed:
#!/bin/bash
# Find constructor or initialization of configDefaults
rg -n 'configDefaults' agent-server/nodejs/src/api-server.js -B2 -A2Length of output: 939
🏁 Script executed:
#!/bin/bash
# Check config.js to see what default providers can be
rg -n -A5 'model.*:' agent-server/nodejs/src/config.js | head -30Length of output: 205
🏁 Script executed:
#!/bin/bash
# Get the complete hardcoded defaults (lines 36-54)
sed -n '36,54p' agent-server/nodejs/src/api-server.jsLength of output: 511
🏁 Script executed:
#!/bin/bash
# Look for config.yaml file or template in the repo
fd -e yaml -e yml | grep -i config | head -10Length of output: 0
🏁 Script executed:
#!/bin/bash
# Check if createDefaultModelConfig uses provider from defaults
rg -n 'createDefaultModelConfig' agent-server/nodejs/src/api-server.js -A20 | head -30Length of output: 1301
🏁 Script executed:
#!/bin/bash
# Get the full createDefaultModelConfig function
rg -n 'createDefaultModelConfig' agent-server/nodejs/src/api-server.js -A15 | head -40Length of output: 1582
🏁 Script executed:
#!/bin/bash
# Check if there's any provider-to-api-key mapping logic anywhere
rg -n 'provider.*api_key|api_key.*provider' agent-server/nodejs/src/api-server.jsLength of output: 174
🏁 Script executed:
#!/bin/bash
# Get the full createDefaultModelConfig function implementation
sed -n '561,590p' agent-server/nodejs/src/api-server.jsLength of output: 1013
🏁 Script executed:
#!/bin/bash
# Check if config.yaml in repository can have a different provider set
find . -name "*.yaml" -o -name "*.yml" | xargs grep -l "provider:" 2>/dev/null | head -5Length of output: 0
The API key selection hardcodes OpenAI credentials even when using other providers.
The issue extends beyond line 553. Both extractModelTierConfig (line 553) and createDefaultModelConfig (line 579) hardcode process.env.OPENAI_API_KEY regardless of the provider. This is inconsistent with:
- The endpoint logic (line 581) which correctly checks
provider === 'litellm' - The supported providers (config.js: openai, litellm, groq, openrouter), each with their own API key env vars (LITELLM_API_KEY, GROQ_API_KEY, OPENROUTER_API_KEY)
When a non-OpenAI provider is configured via defaults.provider (e.g., from config.yaml), the code will use the wrong API key at runtime, causing authentication failures.
Required fix: Map provider to the corresponding API key environment variable:
- 'litellm' →
process.env.LITELLM_API_KEY - 'groq' →
process.env.GROQ_API_KEY - 'openrouter' →
process.env.OPENROUTER_API_KEY - 'openai' (default) →
process.env.OPENAI_API_KEY
| // This prevents duplication in the UI and LLM calls | ||
| const historyWithoutLastUser = conversationHistory.filter(msg => { | ||
| // Keep all model/assistant messages and all user messages except the last one | ||
| if (msg.entity === ChatMessageEntity.MODEL) { | ||
| return true; | ||
| } | ||
| // For user messages, only keep if it's not the last user message | ||
| if (msg.entity === ChatMessageEntity.USER) { | ||
| return msg.text !== userMessage; | ||
| } | ||
| return true; // Keep other message types | ||
| }); | ||
| state.messages = [...historyWithoutLastUser]; | ||
| logger.info('Loaded history without last user message', { | ||
| originalLength: conversationHistory.length, | ||
| filteredLength: historyWithoutLastUser.length | ||
| }); | ||
| } |
There was a problem hiding this comment.
Do not drop earlier duplicate user turns when trimming history
The current filter removes every ChatMessageEntity.USER whose text matches the latest user input. If a conversation contained the same prompt twice (e.g., “retry?”), both turns vanish from the history sent to AgentService, so the model no longer sees the earlier context. Please remove only the single most recent user item instead of filtering by value.
Apply this diff to keep all earlier duplicates:
- const historyWithoutLastUser = conversationHistory.filter(msg => {
- // Keep all model/assistant messages and all user messages except the last one
- if (msg.entity === ChatMessageEntity.MODEL) {
- return true;
- }
- // For user messages, only keep if it's not the last user message
- if (msg.entity === ChatMessageEntity.USER) {
- return msg.text !== userMessage;
- }
- return true; // Keep other message types
- });
+ let lastUserIndex = -1;
+ for (let i = conversationHistory.length - 1; i >= 0; i--) {
+ if (conversationHistory[i].entity === ChatMessageEntity.USER) {
+ lastUserIndex = i;
+ break;
+ }
+ }
+ const historyWithoutLastUser =
+ lastUserIndex === -1
+ ? conversationHistory
+ : conversationHistory.filter((_, index) => index !== lastUserIndex);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // This prevents duplication in the UI and LLM calls | |
| const historyWithoutLastUser = conversationHistory.filter(msg => { | |
| // Keep all model/assistant messages and all user messages except the last one | |
| if (msg.entity === ChatMessageEntity.MODEL) { | |
| return true; | |
| } | |
| // For user messages, only keep if it's not the last user message | |
| if (msg.entity === ChatMessageEntity.USER) { | |
| return msg.text !== userMessage; | |
| } | |
| return true; // Keep other message types | |
| }); | |
| state.messages = [...historyWithoutLastUser]; | |
| logger.info('Loaded history without last user message', { | |
| originalLength: conversationHistory.length, | |
| filteredLength: historyWithoutLastUser.length | |
| }); | |
| } | |
| // This prevents duplication in the UI and LLM calls | |
| let lastUserIndex = -1; | |
| for (let i = conversationHistory.length - 1; i >= 0; i--) { | |
| if (conversationHistory[i].entity === ChatMessageEntity.USER) { | |
| lastUserIndex = i; | |
| break; | |
| } | |
| } | |
| const historyWithoutLastUser = | |
| lastUserIndex === -1 | |
| ? conversationHistory | |
| : conversationHistory.filter((_, index) => index !== lastUserIndex); | |
| state.messages = [...historyWithoutLastUser]; | |
| logger.info('Loaded history without last user message', { | |
| originalLength: conversationHistory.length, | |
| filteredLength: historyWithoutLastUser.length | |
| }); | |
| } |
🤖 Prompt for AI Agents
In front_end/panels/ai_chat/evaluation/remote/EvaluationAgent.ts around lines
1049 to 1066, the current filter removes all user messages whose text equals the
latest input; change it to remove only the single most recent user message that
matches userMessage. Specifically, find the index of the last
ChatMessageEntity.USER whose text === userMessage (e.g., iterate from the end or
use lastIndex logic), create a new array that omits only that one index (keep
all other messages, including earlier duplicates), assign that array to
state.messages, and retain the existing logging of originalLength and
filteredLength.
2 participants
Changes to /v1/responses API
Summary by CodeRabbit
Release Notes
New Features
Documentation