feat: add missing OpenAI/Claude/Gemini request fields#2971
Open
seefs001 wants to merge 3 commits intoQuantumNous:mainfrom
Open
feat: add missing OpenAI/Claude/Gemini request fields#2971seefs001 wants to merge 3 commits intoQuantumNous:mainfrom
seefs001 wants to merge 3 commits intoQuantumNous:mainfrom
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 WalkthroughThe PR adds a channel setting AllowIncludeObfuscation (DTO + frontend UI) and threads it through relay logic so stream_options.include_obfuscation is removed unless permitted; it also extends several DTOs (OpenAI, Gemini, Claude) with new fields/type adjustments and updates calls to RemoveDisabledFields to accept a pass-through flag. Changes
Sequence Diagram(s)sequenceDiagram
rect rgba(200,230,255,0.5)
participant User
participant Frontend
participant API_Server as "Server (Channels API)"
participant Relay
participant Upstream as "Provider API"
end
User->>Frontend: Toggle "Allow include obfuscation" and Save
Frontend->>API_Server: Save channel settings (includes allow_include_obfuscation)
API_Server-->>Frontend: 200 OK
Frontend->>User: UI confirms saved
Note over Relay,API_Server: At request time
API_Server->>Relay: Forward client request JSON + channel settings
Relay->>Relay: Call RemoveDisabledFields(jsonData, channelOtherSettings, passThroughEnabled)
alt passThroughEnabled == true
Relay->>Upstream: Send original JSON
else passThroughEnabled == false and AllowIncludeObfuscation == false
Relay->>Relay: Remove stream_options.include_obfuscation (and prune stream_options)
Relay->>Upstream: Send pruned JSON
end
Upstream-->>Relay: Provider response
Relay-->>API_Server: Return response
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
relay/common/override_test.go (1)
778-813: Add tests for the coreAllowIncludeObfuscationremoval logic.The new tests verify the pass-through skip paths, but there is no coverage for the actual
include_obfuscationremoval behaviour introduced inRemoveDisabledFields. The following scenarios are untested:
include_obfuscationis stripped whenAllowIncludeObfuscation = falseinclude_obfuscationis preserved whenAllowIncludeObfuscation = truestream_optionsis removed entirely wheninclude_obfuscationwas its only field (andAllowIncludeObfuscation = false)stream_optionsis kept (with its remaining fields) when other sibling fields (e.g.include_usage) co-exist🧪 Proposed additional test cases
+func TestRemoveDisabledFieldsRemovesIncludeObfuscationByDefault(t *testing.T) { + input := `{"stream_options":{"include_usage":true,"include_obfuscation":false}}` + settings := dto.ChannelOtherSettings{} // AllowIncludeObfuscation = false by default + + out, err := RemoveDisabledFields([]byte(input), settings, false) + if err != nil { + t.Fatalf("RemoveDisabledFields returned error: %v", err) + } + assertJSONEqual(t, `{"stream_options":{"include_usage":true}}`, string(out)) +} + +func TestRemoveDisabledFieldsRemovesStreamOptionsWhenOnlyIncludeObfuscation(t *testing.T) { + input := `{"model":"gpt-4","stream_options":{"include_obfuscation":false}}` + settings := dto.ChannelOtherSettings{} + + out, err := RemoveDisabledFields([]byte(input), settings, false) + if err != nil { + t.Fatalf("RemoveDisabledFields returned error: %v", err) + } + assertJSONEqual(t, `{"model":"gpt-4"}`, string(out)) +} + +func TestRemoveDisabledFieldsPreservesIncludeObfuscationWhenAllowed(t *testing.T) { + input := `{"stream_options":{"include_usage":true,"include_obfuscation":false}}` + settings := dto.ChannelOtherSettings{AllowIncludeObfuscation: true} + + out, err := RemoveDisabledFields([]byte(input), settings, false) + if err != nil { + t.Fatalf("RemoveDisabledFields returned error: %v", err) + } + assertJSONEqual(t, input, string(out)) +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@relay/common/override_test.go` around lines 778 - 813, Add unit tests covering RemoveDisabledFields's AllowIncludeObfuscation branch: write tests that call RemoveDisabledFields with dto.ChannelOtherSettings{AllowIncludeObfuscation: false} and payloads containing "stream_options":{"include_obfuscation":true} to assert include_obfuscation is removed and stream_options is removed if it becomes empty; another with AllowIncludeObfuscation: true to assert include_obfuscation is preserved; and a test where stream_options contains include_obfuscation plus another field (e.g. "include_usage") with AllowIncludeObfuscation:false to assert include_obfuscation is stripped but stream_options and the sibling field remain. Use function names like TestRemoveDisabledFields_StripsIncludeObfuscation_WhenNotAllowed, TestRemoveDisabledFields_PreservesIncludeObfuscation_WhenAllowed, and TestRemoveDisabledFields_RemovesEmptyStreamOptions to locate coverage for RemoveDisabledFields and dto.ChannelOtherSettings.relay/common/relay_info.go (1)
739-752: Redundant map re-assignment on line 748.
streamOptionsis the underlyingmap[string]interface{}obtained via type assertion fromdata["stream_options"]. Since Go maps are reference types,delete(streamOptions, "include_obfuscation")already mutates the map in-place, sodata["stream_options"]already reflects the deletion without any explicit re-assignment.♻️ Simplify the else branch
if len(streamOptions) == 0 { delete(data, "stream_options") - } else { - data["stream_options"] = streamOptions }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@relay/common/relay_info.go` around lines 739 - 752, The code redundantly reassigns streamOptions back into data after mutating it; because maps are reference types in Go, deleting a key via delete(streamOptions, "include_obfuscation") already updates data["stream_options"]. Remove the unnecessary else branch that sets data["stream_options"] = streamOptions and instead only delete data["stream_options"] when streamOptions becomes empty; keep the existing type-assertion and delete(streamOptions, ...) logic inside the AllowIncludeObfuscation check.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@relay/common/override_test.go`:
- Around line 778-813: Add unit tests covering RemoveDisabledFields's
AllowIncludeObfuscation branch: write tests that call RemoveDisabledFields with
dto.ChannelOtherSettings{AllowIncludeObfuscation: false} and payloads containing
"stream_options":{"include_obfuscation":true} to assert include_obfuscation is
removed and stream_options is removed if it becomes empty; another with
AllowIncludeObfuscation: true to assert include_obfuscation is preserved; and a
test where stream_options contains include_obfuscation plus another field (e.g.
"include_usage") with AllowIncludeObfuscation:false to assert
include_obfuscation is stripped but stream_options and the sibling field remain.
Use function names like
TestRemoveDisabledFields_StripsIncludeObfuscation_WhenNotAllowed,
TestRemoveDisabledFields_PreservesIncludeObfuscation_WhenAllowed, and
TestRemoveDisabledFields_RemovesEmptyStreamOptions to locate coverage for
RemoveDisabledFields and dto.ChannelOtherSettings.
In `@relay/common/relay_info.go`:
- Around line 739-752: The code redundantly reassigns streamOptions back into
data after mutating it; because maps are reference types in Go, deleting a key
via delete(streamOptions, "include_obfuscation") already updates
data["stream_options"]. Remove the unnecessary else branch that sets
data["stream_options"] = streamOptions and instead only delete
data["stream_options"] when streamOptions becomes empty; keep the existing
type-assertion and delete(streamOptions, ...) logic inside the
AllowIncludeObfuscation check.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Improvements
Tests