fix(channel-test): responses fallback + better upstream error messages - #2501
fix(channel-test): responses fallback + better upstream error messages#2501FlowerRealm wants to merge 5 commits into
Conversation
- Parse upstream errors with detail/object fallback to avoid empty UI messages\n- Auto fallback from chat.completions to responses when messages unsupported\n- Add unit tests for error parsing and relay error handler
WalkthroughSwitches channel testing and many UI/defaults from the chat/completions path to the responses path; expands OpenAIResponses request construction and relay format routing; relaxes Responses request validation (allows nil Input); improves error-message fallbacks and nil-body handling; adds unit tests across these behaviors. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Controller as ChannelTestController
participant Upstream as UpstreamAPI
Note over Controller: buildTestRequest(model, endpointType, requestPath)
Client->>Controller: POST /testChannel (model, requestPath=/v1/chat/completions)
Controller->>Upstream: POST /v1/chat/completions (chat-format test payload)
alt Upstream returns 200
Upstream-->>Controller: 200 OK
Controller-->>Client: success (report)
else Upstream returns 4xx with unsupported-parameter or response-only error
Upstream-->>Controller: 4xx error
Controller->>Controller: preserve originTestModel
Controller->>Upstream: POST /v1/responses (OpenAIResponsesRequest, structured input, max_output_tokens)
alt Upstream returns 200
Upstream-->>Controller: 200 OK
Controller-->>Client: success (report)
else Upstream returns error
Upstream-->>Controller: error
Controller-->>Client: error (wrapped)
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
- Allow /v1/responses requests without input (spec allows model-only)\n- In channel test auto-detect, use /v1/responses for response-only models\n- Add unit test for responses request validation
![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
🧹 Nitpick comments (1)
relay/helper/valid_request_test.go (1)
15-15: Consider saving and restoring the global variable for test isolation.Modifying
constant.MaxRequestBodyMBwithout restoration could cause test interference if multiple tests in this package run in parallel and also modify this global variable.🔎 Proposed fix for test isolation
func TestGetAndValidateResponsesRequest_AllowsMissingInput(t *testing.T) { + // Save original value and restore after test + origMaxRequestBodyMB := constant.MaxRequestBodyMB + defer func() { constant.MaxRequestBodyMB = origMaxRequestBodyMB }() + constant.MaxRequestBodyMB = 20
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
controller/channel-test.gorelay/helper/valid_request.gorelay/helper/valid_request_test.go
💤 Files with no reviewable changes (1)
- relay/helper/valid_request.go
🚧 Files skipped from review as they are similar to previous changes (1)
- controller/channel-test.go
🧰 Additional context used
🧬 Code graph analysis (1)
relay/helper/valid_request_test.go (4)
constant/env.go (1)
MaxRequestBodyMB(12-12)dto/request_common.go (1)
Request(8-12)relay/helper/valid_request.go (1)
GetAndValidateResponsesRequest(113-123)dto/openai_request.go (1)
Input(901-905)
🔇 Additional comments (2)
relay/helper/valid_request_test.go (2)
1-13: LGTM! Clean test setup.The imports are appropriate and well-organized for a Gin-based unit test.
14-41: Well-structured test that validates the spec-compliant behavior.The test clearly demonstrates that
GetAndValidateResponsesRequestnow accepts model-only requests (without the Input field), which aligns with the PR objective to support responses-only upstreams.
|
@coderabbitai generate docstrings |
✅ Actions performedInitiated docstring generation; will generate only if new commits exist. |
|
Note Docstrings generation - SUCCESS |
Docstrings generation was requested by @FlowerRealm. * #2501 (comment) The following files were modified: * `controller/channel-test.go` * `relay/helper/valid_request.go` * `service/error.go`
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 (1)
web/src/hooks/playground/useApiRequest.jsx (1)
244-266: Potential issue: empty output leaves message in LOADING state.If the API returns a valid response but with empty
output_text/output, theif (outputText)check on line 244 will skip the message update, leaving the last message stuck inMESSAGE_STATUS.LOADING. This could confuse users.Consider handling empty-but-valid responses:
🔎 Suggested fix
- if (outputText) { + // Process response even if output is empty - still need to complete the message + { const processed = processThinkTags(outputText, ''); setMessage((prevMessage) => { const newMessages = [...prevMessage]; const lastMessage = newMessages[newMessages.length - 1]; if (lastMessage?.status === MESSAGE_STATUS.LOADING) { const autoCollapseState = applyAutoCollapseLogic( lastMessage, true, ); newMessages[newMessages.length - 1] = { ...lastMessage, - content: processed.content, + content: processed.content || '', reasoningContent: processed.reasoningContent, status: MESSAGE_STATUS.COMPLETE, ...autoCollapseState, }; } return newMessages; }); }
🧹 Nitpick comments (2)
docs/openapi/relay.json (1)
2514-2881: Consider removing unused endpoint schemas.The removal of POST
/v1/chat/completionsand POST/v1/completionsendpoints aligns with the PR's goal to consolidate on/v1/responses. However, the associated schemas (ChatCompletionRequest,ChatCompletionResponse,ChatCompletionStreamResponse,CompletionRequest,CompletionResponse) are still defined in the components section.If these schemas are no longer referenced elsewhere in the API specification, consider removing them to keep the documentation clean and avoid confusion. If they're retained for backward compatibility documentation or are referenced by other parts of the system, this can be safely ignored.
</review_comment_end>
web/src/helpers/api.js (1)
257-259: Consider adding a comment or minimal logging for the swallowed error.The empty catch block intentionally ignores logout failures during OAuth state preparation, which is reasonable since the logout is best-effort. However, silent error swallowing can make debugging harder.
🔎 Suggested improvement
try { await API.get('/api/user/logout', { skipErrorHandler: true }); - } catch (err) {} + } catch { + // Logout failure is non-critical during OAuth state preparation + }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
bin/time_test.shcommon/endpoint_defaults.gocontroller/channel-test.gocontroller/playground.godocs/openapi/relay.jsonmiddleware/distributor.gorelay/constant/relay_mode.gorouter/relay-router.goweb/src/components/table/channels/modals/EditChannelModal.jsxweb/src/components/table/channels/modals/ModelTestModal.jsxweb/src/components/table/models/modals/EditModelModal.jsxweb/src/components/table/models/modals/EditPrefillGroupModal.jsxweb/src/constants/common.constant.jsweb/src/constants/playground.constants.jsweb/src/helpers/api.jsweb/src/hooks/playground/useApiRequest.jsxweb/src/i18n/locales/en.jsonweb/src/i18n/locales/fr.jsonweb/src/i18n/locales/ja.jsonweb/src/i18n/locales/ru.jsonweb/src/i18n/locales/vi.jsonweb/src/i18n/locales/zh.json
💤 Files with no reviewable changes (1)
- web/src/constants/common.constant.js
✅ Files skipped from review due to trivial changes (1)
- web/src/components/table/channels/modals/EditChannelModal.jsx
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-27T02:15:25.448Z
Learnt from: AAEE86
Repo: QuantumNous/new-api PR: 1658
File: web/src/components/table/channels/modals/EditChannelModal.jsx:555-569
Timestamp: 2025-08-27T02:15:25.448Z
Learning: In EditChannelModal.jsx, the applyModelMapping function transforms the models list by replacing original model names (mapping values) with display names (mapping keys). The database stores this transformed list containing mapped keys. On channel load, data.models contains these mapped display names, making the initialization filter if (data.models.includes(key)) correct.
Applied to files:
web/src/components/table/channels/modals/ModelTestModal.jsxweb/src/components/table/models/modals/EditModelModal.jsx
📚 Learning: 2025-08-27T02:15:25.448Z
Learnt from: AAEE86
Repo: QuantumNous/new-api PR: 1658
File: web/src/components/table/channels/modals/EditChannelModal.jsx:555-569
Timestamp: 2025-08-27T02:15:25.448Z
Learning: In EditChannelModal.jsx, the database stores mapped keys (display names) in the models field after applying model mapping transformations. When loading a channel, data.models contains the mapped keys, not the original model names. The filtering logic if (data.models.includes(key)) in the initialization is correct.
Applied to files:
web/src/components/table/channels/modals/ModelTestModal.jsx
🧬 Code graph analysis (5)
controller/channel-test.go (6)
types/relay_format.go (1)
RelayFormatOpenAIResponses(9-9)dto/request_common.go (1)
Request(8-12)constant/endpoint_type.go (4)
EndpointTypeOpenAIResponse(7-7)EndpointTypeOpenAI(6-6)EndpointTypeAnthropic(8-8)EndpointTypeGemini(9-9)dto/openai_request.go (2)
OpenAIResponsesRequest(792-817)Input(901-905)model/model_meta.go (1)
Model(23-44)dto/embedding.go (1)
EmbeddingRequest(22-33)
router/relay-router.go (1)
controller/playground.go (1)
Playground(16-55)
middleware/distributor.go (1)
dto/request_common.go (1)
Request(8-12)
web/src/hooks/playground/useApiRequest.jsx (3)
web/src/constants/playground.constants.js (4)
API_ENDPOINTS(77-81)API_ENDPOINTS(77-81)DEBUG_TABS(70-74)DEBUG_TABS(70-74)web/src/constants/common.constant.js (2)
API_ENDPOINTS(26-38)API_ENDPOINTS(26-38)web/src/hooks/playground/usePlaygroundState.js (1)
sseSourceRef(110-110)
common/endpoint_defaults.go (1)
constant/endpoint_type.go (1)
EndpointTypeOpenAI(6-6)
🔇 Additional comments (33)
web/src/i18n/locales/vi.json (1)
2057-2057: LGTM! Localization updated to reflect new endpoint.The URL example in the Vietnamese translation has been correctly updated from
/v1/chat/completionsto/v1/responses, consistent with the PR's migration to the Responses API. The translation quality is accurate and maintains consistency with other translations in the file.bin/time_test.sh (2)
18-18: Endpoint migration looks good.The switch to
/v1/responsesaligns with the PR's objective to migrate channel testing to the Responses API.
21-21: Verify the stream setting.The payload structure correctly migrates to the Responses API format (
inputarray,max_output_tokens). However, the PR description states "Make the Responses test request use stream=true," but this line sets"stream": false.Is
stream=falseintentional for this timing script, or should it be aligned with the PR's stated objective?web/src/components/table/channels/modals/ModelTestModal.jsx (1)
65-79: OpenAI endpoint label correctly aligned to /v1/responsesThe updated label
OpenAI (/v1/responses)matches the new default OpenAI path used across the PR and keeps the endpoint selector consistent with backend behavior. No issues from a UI or logic perspective.common/endpoint_defaults.go (1)
11-27: Default OpenAI endpoint switched to /v1/responses consistentlyUpdating
EndpointTypeOpenAIto use"/v1/responses"(and fixing the inline JSON example) aligns backend defaults with the new Responses-based flow and keeps it consistent withEndpointTypeOpenAIResponse. No functional or compatibility concerns apparent in this file.web/src/i18n/locales/en.json (1)
1834-1834: English example URL updated to Responses endpointThe placeholder now shows
https://api.openai.com/v1/responses, which matches the new default OpenAI text endpoint and keeps UX guidance accurate.web/src/components/table/models/modals/EditPrefillGroupModal.jsx (1)
42-49: Endpoint template and placeholder now match /v1/responses
ENDPOINT_TEMPLATE.openaiand the JSON editor placeholder both use"/v1/responses"withPOST, which is consistent with the updated backend defaults and other UI text. This keeps quick-fill and docs aligned.Also applies to: 246-248
web/src/i18n/locales/ja.json (1)
1746-1746: Japanese placeholder URL aligned with /v1/responsesThe Japanese translation now uses
https://api.openai.com/v1/responsesas the example URL, matching the new canonical endpoint and remaining consistent with other locales.web/src/i18n/locales/zh.json (1)
1822-1822: LGTM - URL example updated to reflect responses endpoint migration.The translation string correctly updates the example URL from the legacy
/v1/chat/completionsendpoint to the new/v1/responsesendpoint, consistent with the PR's objective of migrating to the Responses API.web/src/i18n/locales/fr.json (1)
1844-1844: LGTM - French translation updated consistently with endpoint migration.The French localization string correctly mirrors the URL update from
/v1/chat/completionsto/v1/responses, maintaining consistency with other locale files and the PR's migration objectives.web/src/i18n/locales/ru.json (1)
1855-1855: LGTM!The Russian translation placeholder correctly reflects the updated endpoint path from
/v1/chat/completionsto/v1/responses, consistent with the broader API endpoint migration across the codebase.relay/constant/relay_mode.go (1)
57-59: LGTM!The path detection for
RelayModeResponsesis correctly prioritized beforechat/completions. Both/v1/responsesand/pg/responsesprefixes are properly handled, and there's no conflict with other paths since prefix matching is used consistently.router/relay-router.go (2)
61-61: LGTM!The playground route correctly updated from
/pg/chat/completionsto/pg/responses, aligning with thecontroller.Playgroundimplementation that now usesRelayFormatOpenAIResponses.
84-88: Routes/v1/chat/completionsand/v1/completionsare removed but migration path is undocumented.The POST routes for
/chat/completionsand/completionshave been intentionally removed from the router, confirming the breaking change. However, the relay mode detection logic inrelay/constant/relay_mode.gostill contains path checks for these routes, and multiple channel adaptors (moonshot, baidu_v2, cloudflare, minimax, volcengine) continue to supportRelayModeChatCompletionsmode. This incomplete removal leaves orphaned code and provides no guidance for clients migrating from these endpoints. Either complete the refactor by removing the orphaned detection logic, or document the replacement endpoint strategy.middleware/distributor.go (2)
83-84: LGTM!The path check correctly updated to match the new playground route
/pg/responses. The logic for handling playground requests with group validation remains intact.
293-294: LGTM!The playground path detection in
getModelRequestis consistent with the earlier check inDistribute(), correctly handling the/pg/responsesendpoint for model extraction.controller/playground.go (2)
34-37: LGTM!The fallback logic for
usingGroupis correct—first attemptingContextKeyUsingGroup, then falling back toContextKeyUserGroupif empty. This ensures a valid group is always available for the temporary token.
47-54: LGTM!The temporary token construction and relay invocation are correct:
- Token name clearly identifies the playground context with the group
RelayFormatOpenAIResponsesaligns with the new endpoint strategycontroller/channel-test.go (4)
78-78: LGTM!The default request path correctly updated to
/v1/responsesto align with the new OpenAIResponses pathway.
140-141: Consider using consistent relay format for OpenAI endpoint type.When
endpointTypeisEndpointTypeOpenAI, the code setsrelayFormattoRelayFormatOpenAIResponses. This is consistent with the PR's migration strategy, but the originalEndpointTypeOpenAIsemantically implies the legacy chat completions format. If this is intentional to force all "openai" endpoint tests through the responses pathway, consider documenting this behavior.
418-425: LGTM!The
OpenAIResponsesRequestconstruction for bothEndpointTypeOpenAIResponseandEndpointTypeOpenAIis correct:
- Uses structured
Inputwith proper JSON format- Sets reasonable
MaxOutputTokensandStreamvalues for testing
392-467: LGTM!The
buildTestRequestfunction correctly handles the newrequestPathparameter:
- Endpoint-specific branches correctly return appropriate request types
- Auto-detection falls back to
OpenAIResponsesRequestfor the default path- Embedding and image generation paths are properly handled
web/src/components/table/models/modals/EditModelModal.jsx (2)
45-45: LGTM!The
ENDPOINT_TEMPLATEcorrectly updated to reflect the new/v1/responsespath for the OpenAI endpoint type.
451-451: LGTM!The JSON placeholder correctly demonstrates the new endpoint format, providing users with an accurate example.
web/src/constants/playground.constants.js (1)
78-78: LGTM!The API endpoint constant correctly updated from
CHAT_COMPLETIONStoRESPONSES, matching the backend route changes. Verification confirms no lingering references to the old constant in JavaScript code, and the backend maintains the old endpoint path for backward compatibility.web/src/helpers/api.js (3)
117-135: LGTM - Clean content transformation helper.The
toResponsesContentfunction correctly transforms chat completions content format to the Responses API format (text→input_text,image_url→input_image). The null checks and filtering are appropriate.One minor observation: if
item.textisundefined, line 125 will produce{ type: 'input_text', text: '' }- this may be intentional for graceful degradation, but verify this is the desired behavior for malformed input.
140-143: LGTM - Payload restructured for Responses API.The mapping correctly transforms messages into the
inputarray format expected by the Responses API, applying content normalization viatoResponsesContent.
148-162: LGTM - Parameter mapping correctly updated.The change from
inputs[param]toinputs[key]on line 156 is correct: the value is read using the original key (e.g.,max_tokens) frominputs, then assigned to the payload using the mapped parameter name (e.g.,max_output_tokens). This properly handles the Responses API naming convention.web/src/hooks/playground/useApiRequest.jsx (5)
188-195: LGTM - Endpoint correctly updated to Responses API.The non-stream request now uses
API_ENDPOINTS.RESPONSES(which is/pg/responsesper the constants), aligning with the PR's migration to the Responses API.
231-242: LGTM - Robust output extraction with appropriate fallbacks.The extraction logic correctly handles both:
- Direct
output_textstring (preferred path)- Structured
outputarray with nested content itemsThe
flatMap→filter→map→joinchain is well-structured. The fallback to empty string ensures graceful degradation for unexpected response shapes.
325-417: LGTM - Well-structured unified SSE event handler.The consolidated
handleStreamEventfunction improves maintainability by centralizing event processing. Key observations:
- Legacy
[DONE]fallback is preserved for backward compatibility- Event-type-specific handling is clean and readable
- Error events (
response.failed,response.incomplete) properly close the stream and update UIMinor note: The
response.output_text.deltahandler silently returns ifdata.deltais not a string (lines 362-367). This is acceptable defensive coding but could mask malformed events during debugging.
419-434: LGTM - Comprehensive SSE event registration.The explicit list of
responseEventTypesclearly documents the expected SSE contract from the Responses API. Registering each type individually with the same handler, plus a genericmessagefallback, ensures robust event capture.
504-504: LGTM - Dependency array correctly reflects actual usage.The
handleSSEcallback's dependencies are correctly listed. Unlike the summary's mention of droppingapplyAutoCollapseLogic, this is correct becausehandleSSEdoesn't call it directly—it's used internally bystreamMessageUpdateandcompleteMessage, which are properly included.
1 participant
Problem:
Fix:
Tests:
Summary by CodeRabbit
New Features
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.