feat: add base_url support for OpenAI provider in managed mode#98
Merged
avivhalfon merged 6 commits intomainfrom Mar 4, 2026
Merged
feat: add base_url support for OpenAI provider in managed mode#98avivhalfon merged 6 commits intomainfrom
avivhalfon merged 6 commits intomainfrom
Conversation
|
|
Contributor
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed everything up to 518fced in 7 seconds. Click for details.
- Reviewed
26lines of code in2files - Skipped
0files when reviewing. - Skipped posting
0draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
Workflow ID: wflow_4uUY6Y5FGLMBRAvD
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdded an optional Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). Comment |
Contributor
🔒 Container Vulnerability Scan (hub-migrations - amd64)Click to expand results |
Contributor
🔒 Container Vulnerability Scan (hub-migrations - arm64)Click to expand results |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/management/dto.rs (1)
57-61: Add round-trip coverage for the newbase_urlfield.
OpenAIProviderConfig.base_urlis additive, but this module currently has no DTO tests for it. A serialize/deserialize test here would prevent silent regressions in request/response handling.✅ Suggested test addition
+ #[test] + fn test_openai_provider_config_with_base_url_roundtrip() { + let config = OpenAIProviderConfig { + api_key: SecretObject::literal("k".to_string()), + organization_id: Some("org_123".to_string()), + base_url: Some("https://proxy.example.com/v1".to_string()), + }; + + let serialized = serde_json::to_value(&config).unwrap(); + let deserialized: OpenAIProviderConfig = serde_json::from_value(serialized).unwrap(); + assert_eq!(deserialized, config); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/management/dto.rs` around lines 57 - 61, Add a serde round-trip unit test named something like test_openai_provider_config_roundtrip that constructs an OpenAIProviderConfig with api_key, organization_id and base_url set (e.g., base_url = Some("https://example.com".into())), serializes it with serde_json::to_string, then deserializes with serde_json::from_str and asserts equality (or field equality) to verify base_url survives serialization; place the test alongside other DTO tests and use the OpenAIProviderConfig type and its SecretObject api_key to match existing test patterns.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/management/services/config_provider_service.rs`:
- Around line 136-138: The current insertion uses if let Some(base_url) =
c.base_url without trimming which lets whitespace-only values like Some(" ")
override defaults; update the check to trim and reject empty strings before
calling params.insert("base_url", ...). Concretely, for the block handling
c.base_url (the if let Some(base_url) = c.base_url {
params.insert("base_url"...) } section) trim the base_url and only insert when
the trimmed value is non-empty, and apply the identical trimmed/empty check to
the Azure branch that sets base_url so both code paths behave consistently.
---
Nitpick comments:
In `@src/management/dto.rs`:
- Around line 57-61: Add a serde round-trip unit test named something like
test_openai_provider_config_roundtrip that constructs an OpenAIProviderConfig
with api_key, organization_id and base_url set (e.g., base_url =
Some("https://example.com".into())), serializes it with serde_json::to_string,
then deserializes with serde_json::from_str and asserts equality (or field
equality) to verify base_url survives serialization; place the test alongside
other DTO tests and use the OpenAIProviderConfig type and its SecretObject
api_key to match existing test patterns.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
src/management/dto.rssrc/management/services/config_provider_service.rs
Contributor
🔒 Container Vulnerability Scan (hub - amd64)Click to expand results |
Contributor
🔒 Container Vulnerability Scan (hub - arm64)Click to expand results |
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/provider_api_tests.rs (1)
169-173: Add one positive-path assertion for custombase_url.The new feature path is best validated by setting
base_url: Some(...)in at least one OpenAI create/update success test and asserting it round-trips in the response/config.🧪 Minimal test coverage tweak
- config: ProviderConfig::OpenAI(OpenAIProviderConfig { + config: ProviderConfig::OpenAI(OpenAIProviderConfig { api_key: SecretObject::literal("test_openai_key".to_string()), organization_id: Some("test_org".to_string()), - base_url: None, + base_url: Some("https://proxy.openai.example/v1".to_string()), }),let updated_config = ProviderConfig::OpenAI(OpenAIProviderConfig { api_key: SecretObject::literal("updated_openai_key".to_string()), organization_id: Some("org_updated".to_string()), - base_url: None, + base_url: Some("https://proxy.openai.example/v1".to_string()), });Also applies to: 515-519
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/provider_api_tests.rs` around lines 169 - 173, Set and assert a non-default OpenAI base_url in the positive-path create/update provider tests: when constructing ProviderConfig::OpenAI with OpenAIProviderConfig set to api_key: SecretObject::literal(...), organization_id: Some(...), set base_url: Some("https://custom-openai.example") and then assert the returned provider/config round-trips that same base_url (e.g., response.config or returned OpenAIProviderConfig.base_url == Some("https://custom-openai.example")). Apply this to the create-success and update-success tests that use ProviderConfig::OpenAI (the occurrences around the shown block and the other occurrence at the referenced lines), ensuring both request payload and response assertions include the custom base_url.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/provider_api_tests.rs`:
- Around line 169-173: Set and assert a non-default OpenAI base_url in the
positive-path create/update provider tests: when constructing
ProviderConfig::OpenAI with OpenAIProviderConfig set to api_key:
SecretObject::literal(...), organization_id: Some(...), set base_url:
Some("https://custom-openai.example") and then assert the returned
provider/config round-trips that same base_url (e.g., response.config or
returned OpenAIProviderConfig.base_url ==
Some("https://custom-openai.example")). Apply this to the create-success and
update-success tests that use ProviderConfig::OpenAI (the occurrences around the
shown block and the other occurrence at the referenced lines), ensuring both
request payload and response assertions include the custom base_url.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
tests/model_definition_api_tests.rstests/pipelines_api_integration_tests.rstests/provider_api_tests.rs
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/management/services/config_provider_service.rs`:
- Around line 136-138: The long if-let expression with
c.base_url.map(...).filter(...) exceeds rustfmt width; break it into shorter
statements so cargo fmt passes — e.g., compute a trimmed BaseUrl in a separate
let binding using c.base_url.map(|s| s.trim().to_string()), then apply
.filter(|s| !s.is_empty()) or an if let Some(base_url) = ... on the next line,
and finally call params.insert("base_url".to_string(), base_url); this keeps the
logic (c.base_url, trim().to_string(), filter(|s| !s.is_empty()), params.insert)
intact while conforming to rustfmt line-length rules.
- Around line 144-146: The Azure branch should use the same trimming/filtering
pattern as the other branch: change the current if-let that consumes c.base_url
to use a non-consuming borrow and the identical map/trim/filter chain used
elsewhere (operate on c.base_url.as_ref() then map trim -> filter non-empty ->
clone/to_string), and then call params.insert("base_url".to_string(), base_url);
update the expression around c.base_url and the surrounding if let so formatting
and behavior match the other branch exactly.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/management/services/config_provider_service.rs
nina-kollman
reviewed
Mar 3, 2026
| } | ||
| if let Some(base_url) = c | ||
| .base_url | ||
| .map(|s| s.trim().to_string()) |
There was a problem hiding this comment.
Is the trimming really necessary ?
I cant see that we are doing it for any other param
Contributor
Author
There was a problem hiding this comment.
i found it anoying when testing and code rabit suggested
what do you think?
nina-kollman
reviewed
Mar 3, 2026
nina-kollman
left a comment
There was a problem hiding this comment.
Lets add test cases where the base_url is NOT none. The test should verify the correct value was chosen
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/management/services/config_provider_service.rs (1)
136-138:⚠️ Potential issue | 🟠 MajorPrevent empty/whitespace
base_urlfrom overriding OpenAI default endpoint.Raw insertion allows
Some("")/ whitespace-only values, which disables the fallback insrc/providers/openai/provider.rsand can cause runtime request failures.🛠️ Proposed fix
- if let Some(base_url) = c.base_url { - params.insert("base_url".to_string(), base_url); - } + if let Some(base_url) = c + .base_url + .map(|s| s.trim().to_string()) + .filter(|s| !s.is_empty()) + { + params.insert("base_url".to_string(), base_url); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/management/services/config_provider_service.rs` around lines 136 - 138, The current insertion of c.base_url into params allows Some("") or whitespace-only values to override the OpenAI default endpoint; update the check where you handle c.base_url so you trim the string and only call params.insert("base_url", ...) when c.base_url is Some and the trimmed value is non-empty (i.e., filter out empty/whitespace-only strings before inserting into params), referencing the existing c.base_url and params.insert call to locate the change.
🧹 Nitpick comments (1)
tests/provider_api_tests.rs (1)
883-919: Nice happy-path coverage for OpenAIbase_urlpersistence.Please add one negative-path case for invalid
base_url(e.g., whitespace-only) to lock expected validation/normalization behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/provider_api_tests.rs` around lines 883 - 919, Add a negative-path test alongside test_create_openai_provider_with_base_url: create a new async test (e.g., test_create_openai_provider_with_invalid_base_url) that builds a CreateProviderRequest using OpenAIProviderConfig with base_url set to a whitespace-only string (Some(" ".to_string())), POST it to "/api/v1/management/providers" using the same client setup, and assert the response status is axum::http::StatusCode::BAD_REQUEST (or the API’s validation error code) and that the response body contains a validation error mentioning "base_url" (or the service’s validation message) to lock the expected validation/normalization behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/management/services/config_provider_service.rs`:
- Around line 136-138: The current insertion of c.base_url into params allows
Some("") or whitespace-only values to override the OpenAI default endpoint;
update the check where you handle c.base_url so you trim the string and only
call params.insert("base_url", ...) when c.base_url is Some and the trimmed
value is non-empty (i.e., filter out empty/whitespace-only strings before
inserting into params), referencing the existing c.base_url and params.insert
call to locate the change.
---
Nitpick comments:
In `@tests/provider_api_tests.rs`:
- Around line 883-919: Add a negative-path test alongside
test_create_openai_provider_with_base_url: create a new async test (e.g.,
test_create_openai_provider_with_invalid_base_url) that builds a
CreateProviderRequest using OpenAIProviderConfig with base_url set to a
whitespace-only string (Some(" ".to_string())), POST it to
"/api/v1/management/providers" using the same client setup, and assert the
response status is axum::http::StatusCode::BAD_REQUEST (or the API’s validation
error code) and that the response body contains a validation error mentioning
"base_url" (or the service’s validation message) to lock the expected
validation/normalization behavior.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
src/management/services/config_provider_service.rstests/provider_api_tests.rs
nina-kollman
approved these changes
Mar 3, 2026
bf2bfa1 to
d669d15
Compare
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
3 participants
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.
Important
Add optional
base_urlsupport toOpenAIProviderConfigfor custom API URL configuration in managed mode.base_urlfield toOpenAIProviderConfigindto.rsfor custom OpenAI API URL support.transform_provider_dto()inconfig_provider_service.rsto includebase_urlin parameters if present.This description was created by
for 518fced. You can customize this summary. It will automatically update as commits are pushed.
Summary by CodeRabbit
New Features
Behavior
Tests