fix: missing field & field control - #1950
Conversation
WalkthroughAdds three pass-through controls (service_tier, store, safety_identifier), extends DTOs for OpenAI and Claude with new fields, adds RemoveDisabledFields to strip disallowed JSON fields based on channel settings, invokes it in relay handlers before param overrides, and surfaces toggles in the web modal and i18n. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant UI as Web UI
participant Relay as Relay Handler
participant San as RemoveDisabledFields
participant Provider as Upstream Provider
UI->>Relay: Send request JSON (OpenAI/Claude)
Relay->>San: RemoveDisabledFields(json, channelOtherSettings)
alt removal error or invalid JSON
San-->>Relay: error
Relay-->>UI: ConvertRequestFailed
else sanitized JSON
San-->>Relay: sanitized JSON
Relay->>Relay: ApplyParamOverride(...)
Relay->>Provider: Forward sanitized request
Provider-->>Relay: Response/Stream
Relay-->>UI: Response/Stream
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
relay/compatible_handler.go (1)
138-148: Keep blocked fields removed after overridesWe currently strip
service_tier/store/safety_identifierbefore runningApplyParamOverride, but any override that sets these keys will add them back, defeating the new controls. Run the removal after overrides (or re-run it) so the final payload sent upstream always respects the toggles.[suggested fix]
- // remove disabled fields for OpenAI API - jsonData, err = relaycommon.RemoveDisabledFields(jsonData, info.ChannelOtherSettings) - if err != nil { - return types.NewError(err, types.ErrorCodeConvertRequestFailed, types.ErrOptionWithSkipRetry()) - } - // apply param override if len(info.ParamOverride) > 0 { jsonData, err = relaycommon.ApplyParamOverride(jsonData, info.ParamOverride) if err != nil { return types.NewError(err, types.ErrorCodeChannelParamOverrideInvalid, types.ErrOptionWithSkipRetry()) } } + // remove disabled fields for OpenAI API after overrides as well + jsonData, err = relaycommon.RemoveDisabledFields(jsonData, info.ChannelOtherSettings) + if err != nil { + return types.NewError(err, types.ErrorCodeConvertRequestFailed, types.ErrOptionWithSkipRetry()) + }relay/responses_handler.go (1)
60-70: Strip disabled fields after overrides tooSame ordering issue here: overrides can bring back
service_tier/store/safety_identifier. Move or repeat the removal afterApplyParamOverrideso the outgoing JSON always honors the disable toggles.[suggested fix]
- // remove disabled fields for OpenAI Responses API - jsonData, err = relaycommon.RemoveDisabledFields(jsonData, info.ChannelOtherSettings) - if err != nil { - return types.NewError(err, types.ErrorCodeConvertRequestFailed, types.ErrOptionWithSkipRetry()) - } - // apply param override if len(info.ParamOverride) > 0 { jsonData, err = relaycommon.ApplyParamOverride(jsonData, info.ParamOverride) if err != nil { return types.NewError(err, types.ErrorCodeChannelParamOverrideInvalid, types.ErrOptionWithSkipRetry()) } } + // remove disabled fields after overrides + jsonData, err = relaycommon.RemoveDisabledFields(jsonData, info.ChannelOtherSettings) + if err != nil { + return types.NewError(err, types.ErrorCodeConvertRequestFailed, types.ErrOptionWithSkipRetry()) + }relay/claude_handler.go (1)
115-125: Enforce field removal after param overridesFor Claude requests, overrides executed after this block can reintroduce the blocked keys. To make the “pass-through control” effective, call
RemoveDisabledFieldsafterApplyParamOverride(or re-run it there) so the final JSON respects channel settings.[suggested fix]
- // remove disabled fields for Claude API - jsonData, err = relaycommon.RemoveDisabledFields(jsonData, info.ChannelOtherSettings) - if err != nil { - return types.NewError(err, types.ErrorCodeConvertRequestFailed, types.ErrOptionWithSkipRetry()) - } - // apply param override if len(info.ParamOverride) > 0 { jsonData, err = relaycommon.ApplyParamOverride(jsonData, info.ParamOverride) if err != nil { return types.NewError(err, types.ErrorCodeChannelParamOverrideInvalid, types.ErrOptionWithSkipRetry()) } } + // remove disabled fields after overrides + jsonData, err = relaycommon.RemoveDisabledFields(jsonData, info.ChannelOtherSettings) + if err != nil { + return types.NewError(err, types.ErrorCodeConvertRequestFailed, types.ErrOptionWithSkipRetry()) + }
🧹 Nitpick comments (4)
dto/channel_settings.go (1)
23-25: Defaults align with backend sanitization; schema looks goodBooleans map cleanly to removal logic (service_tier/safety_identifier off by default, store allowed). No blockers. Consider pointers later only if you need to distinguish “unset” from explicit false.
web/src/components/table/channels/modals/EditChannelModal.jsx (2)
461-473: Parsing is fine; minor robustness nitReads merge cleanly with safe fallbacks. If you expect non‑boolean legacy values, consider Boolean(value) normalization to avoid truthy strings.
Also applies to: 478-481
917-943: Guard against stale settings when type changesWhen switching away from OpenAI/Claude/Enterprise, previously saved keys can linger in settings. Consider pruning unsupported keys on submit to keep storage minimal and avoid surprise behavior.
Example patch:
// type === 1 (OpenAI) 或 type === 14 (Claude) if (localInputs.type === 1 || localInputs.type === 14) { settings.allow_service_tier = localInputs.allow_service_tier === true; if (localInputs.type === 1) { settings.disable_store = localInputs.disable_store === true; settings.allow_safety_identifier = localInputs.allow_safety_identifier === true; } else { + delete settings.disable_store; + delete settings.allow_safety_identifier; } } else { + delete settings.allow_service_tier; + delete settings.disable_store; + delete settings.allow_safety_identifier; } + if (localInputs.type !== 20) { + delete settings.openrouter_enterprise; + }dto/openai_request.go (1)
60-71: LGTM! Field additions are well-structured.The new fields are properly typed with appropriate use of
json.RawMessagefor opaque data andomitemptytags. The comments clearly document privacy and filtering concerns forSafetyIdentifier,Store, and other sensitive fields.Consider standardizing documentation language (mixed English/Chinese comments) for improved maintainability, though this is a minor style preference.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
dto/channel_settings.go(1 hunks)dto/claude.go(1 hunks)dto/openai_request.go(2 hunks)relay/claude_handler.go(1 hunks)relay/common/relay_info.go(1 hunks)relay/compatible_handler.go(1 hunks)relay/responses_handler.go(1 hunks)web/src/components/table/channels/modals/EditChannelModal.jsx(6 hunks)web/src/i18n/locales/en.json(1 hunks)web/src/i18n/locales/fr.json(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
relay/responses_handler.go (3)
relay/common/relay_info.go (1)
RemoveDisabledFields(515-543)dto/channel_settings.go (1)
ChannelOtherSettings(19-26)types/error.go (3)
NewError(193-213)ErrorCodeConvertRequestFailed(61-61)ErrOptionWithSkipRetry(323-327)
relay/compatible_handler.go (3)
relay/common/relay_info.go (1)
RemoveDisabledFields(515-543)dto/channel_settings.go (1)
ChannelOtherSettings(19-26)types/error.go (3)
NewError(193-213)ErrorCodeConvertRequestFailed(61-61)ErrOptionWithSkipRetry(323-327)
relay/common/relay_info.go (2)
dto/channel_settings.go (1)
ChannelOtherSettings(19-26)common/json.go (2)
Unmarshal(8-10)Marshal(20-22)
relay/claude_handler.go (3)
relay/common/relay_info.go (1)
RemoveDisabledFields(515-543)dto/channel_settings.go (1)
ChannelOtherSettings(19-26)types/error.go (3)
NewError(193-213)ErrorCodeConvertRequestFailed(61-61)ErrOptionWithSkipRetry(323-327)
🔇 Additional comments (3)
dto/claude.go (1)
203-206: Appropriate pass-through fields for ClaudeRawMessage for mcp_servers/metadata avoids double-encoding; service_tier tag matches sanitizer key. LGTM.
web/src/components/table/channels/modals/EditChannelModal.jsx (2)
172-176: Good defaults for pass‑through controlsDefaults match backend policy (service_tier/safety_identifier off; store allowed).
954-958: Cleanup of temporary fields on submit is correctTop‑level UI flags are stripped before send; keeps payload lean.
| // 服务层级字段，用于指定 API 服务等级。允许透传可能导致实际计费高于预期，默认应过滤 | ||
| ServiceTier string `json:"service_tier,omitempty"` | ||
| Store json.RawMessage `json:"store,omitempty"` | ||
| PromptCacheKey json.RawMessage `json:"prompt_cache_key,omitempty"` | ||
| Stream bool `json:"stream,omitempty"` | ||
| Temperature float64 `json:"temperature,omitempty"` | ||
| Text json.RawMessage `json:"text,omitempty"` | ||
| ToolChoice json.RawMessage `json:"tool_choice,omitempty"` | ||
| Tools json.RawMessage `json:"tools,omitempty"` // 需要处理的参数很少，MCP 参数太多不确定，所以用 map | ||
| TopP float64 `json:"top_p,omitempty"` | ||
| Truncation string `json:"truncation,omitempty"` | ||
| User string `json:"user,omitempty"` | ||
| MaxToolCalls uint `json:"max_tool_calls,omitempty"` | ||
| Prompt json.RawMessage `json:"prompt,omitempty"` |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Verify the type inconsistency for PromptCacheKey.
The field additions look correct overall. However, PromptCacheKey is declared as json.RawMessage here (line 793), but as string in GeneralOpenAIRequest (line 68). This inconsistency may cause confusion.
Please confirm whether the different types are intentional (due to different API requirements) or if they should be aligned for consistency.
🏁 Script executed:
#!/bin/bash
# Description: Check usage patterns of PromptCacheKey across the codebase to understand if different types are required
# Search for PromptCacheKey field access and usage
rg -nP -C3 'PromptCacheKey'Length of output: 1234
🏁 Script executed:
#!/bin/bash
# Find and display the struct declaration around line 790
fd -t f dto/openai_request.go | xargs sed -n '750,830p'Length of output: 0
🏁 Script executed:
#!/bin/bash
# Display the struct declaration around line 790 in dto/openai_request.go
sed -n '740,830p' dto/openai_request.goLength of output: 3202
Align PromptCacheKey type in OpenAIResponsesRequest
In dto/openai_request.go, the OpenAIResponsesRequest struct declares
PromptCacheKey json.RawMessage `json:"prompt_cache_key,omitempty"`but in GeneralOpenAIRequest (line 68) it’s a string. Change the type here to string to ensure consistency with the sibling struct and the OpenAI API spec.
🤖 Prompt for AI Agents
In dto/openai_request.go around lines 790 to 803, the OpenAIResponsesRequest
struct declares PromptCacheKey as json.RawMessage but GeneralOpenAIRequest
defines it as string; update the PromptCacheKey field here to type string
(keeping the `json:"prompt_cache_key,omitempty"` tag) so both structs match the
OpenAI API spec and sibling struct; ensure any code that constructs or reads
this field treats it as a string and remove any now-unused imports if
applicable.
| {/* 字段透传控制 - OpenAI 渠道 */} | ||
| {inputs.type === 1 && ( | ||
| <> | ||
| <div className='mt-4 mb-2 text-sm font-medium text-gray-700'> | ||
| {t('字段透传控制')} | ||
| </div> | ||
|
|
||
| <Form.Switch | ||
| field='allow_service_tier' | ||
| label={t('允许 service_tier 透传')} | ||
| checkedText={t('开')} | ||
| uncheckedText={t('关')} | ||
| onChange={(value) => | ||
| handleChannelOtherSettingsChange('allow_service_tier', value) | ||
| } | ||
| extraText={t( | ||
| 'service_tier 字段用于指定服务层级，允许透传可能导致实际计费高于预期。默认关闭以避免额外费用', | ||
| )} | ||
| /> | ||
|
|
||
| <Form.Switch | ||
| field='disable_store' | ||
| label={t('禁用 store 透传')} | ||
| checkedText={t('开')} | ||
| uncheckedText={t('关')} | ||
| onChange={(value) => | ||
| handleChannelOtherSettingsChange('disable_store', value) | ||
| } | ||
| extraText={t( | ||
| 'store 字段用于授权 OpenAI 存储请求数据以评估和优化产品。默认关闭，开启后可能导致 Codex 无法正常使用', | ||
| )} | ||
| /> | ||
|
|
||
| <Form.Switch | ||
| field='allow_safety_identifier' | ||
| label={t('允许 safety_identifier 透传')} | ||
| checkedText={t('开')} | ||
| uncheckedText={t('关')} | ||
| onChange={(value) => | ||
| handleChannelOtherSettingsChange('allow_safety_identifier', value) | ||
| } | ||
| extraText={t( | ||
| 'safety_identifier 字段用于帮助 OpenAI 识别可能违反使用政策的应用程序用户。默认关闭以保护用户隐私', | ||
| )} | ||
| /> | ||
| </> | ||
| )} | ||
|
|
||
| {/* 字段透传控制 - Claude 渠道 */} | ||
| {(inputs.type === 14) && ( | ||
| <> | ||
| <div className='mt-4 mb-2 text-sm font-medium text-gray-700'> | ||
| {t('字段透传控制')} | ||
| </div> | ||
|
|
||
| <Form.Switch | ||
| field='allow_service_tier' | ||
| label={t('允许 service_tier 透传')} | ||
| checkedText={t('开')} | ||
| uncheckedText={t('关')} | ||
| onChange={(value) => | ||
| handleChannelOtherSettingsChange('allow_service_tier', value) | ||
| } | ||
| extraText={t( | ||
| 'service_tier 字段用于指定服务层级，允许透传可能导致实际计费高于预期。默认关闭以避免额外费用', | ||
| )} | ||
| /> | ||
| </> | ||
| )} |
There was a problem hiding this comment.
Don’t mix “other settings” into channelSettings; it pollutes the “setting” JSON
handleChannelOtherSettingsChange currently updates channelSettings, which is later serialized into the “setting” JSON when any channel setting flips. That leaks keys like allow_service_tier into the wrong blob. Stop mutating channelSettings here.
Apply this change to the handler:
- const handleChannelOtherSettingsChange = (key, value) => {
- // 更新内部状态
- setChannelSettings((prev) => ({ ...prev, [key]: value }));
+ const handleChannelOtherSettingsChange = (key, value) => {
+ // 仅更新表单与 inputs；不要写入 channelSettings（它用于 setting）
// 同步更新到表单字段
if (formApiRef.current) {
formApiRef.current.setValue(key, value);
}
// 同步更新inputs状态
setInputs((prev) => ({ ...prev, [key]: value }));
// 更新 settings JSON
let settings = {};
if (inputs.settings) {
try {
settings = JSON.parse(inputs.settings);
} catch (error) {
console.error('解析设置失败:', error);
}
}
settings[key] = value;
const settingsJson = JSON.stringify(settings);
handleInputChange('settings', settingsJson);
};Optionally track a separate channelOtherSettings state if you need local defaults.
…l failures in RemoveDisabledFields
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
relay/common/relay_info.go (1)
516-549: Best-effort error handling implemented as suggested in prior review.The implementation correctly returns the original
jsonDatawithnilerror on unmarshal/marshal failures, ensuring valid requests aren't rejected. The conditional field removal logic is correct and matches the documented defaults.Consider documenting that field removal is shallow (top-level keys only) if nested removal might be expected in the future:
// RemoveDisabledFields 从请求 JSON 数据中移除渠道设置中禁用的字段 +// 注意：仅处理顶层字段，不会递归删除嵌套字段 // service_tier: 服务层级字段，可能导致额外计费（OpenAI、Claude、Responses API 支持）
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
relay/common/relay_info.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
relay/common/relay_info.go (3)
dto/channel_settings.go (1)
ChannelOtherSettings(19-26)common/json.go (2)
Unmarshal(8-10)Marshal(20-22)common/sys_log.go (1)
SysError(16-19)
🔇 Additional comments (1)
relay/common/relay_info.go (1)
511-515: Well-documented field sanitization logic.The function documentation clearly explains the purpose, default behavior, and potential impacts of each field removal, which helps maintainers understand the trade-offs.
fix: missing field & field control
1 participant
Summary by CodeRabbit
New Features
Documentation
Localization