feat(config): allow provider path suffix overrides#2506
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new path_suffix configuration option for providers, allowing users to override or clear the default path segment (like /v1) inserted between the base_url and the API paths. The feedback highlights that path_suffix is missing CLI configuration support in ConfigToml (get_value, set_value, unset_value, and list_values), which would cause CLI-set values to be written as flat keys at the root of the TOML instead of being correctly deserialized into the provider's table.
| pub struct ProviderConfigToml { | ||
| pub api_key: Option<String>, | ||
| pub base_url: Option<String>, | ||
| pub path_suffix: Option<String>, |
There was a problem hiding this comment.
Missing CLI Config Support for path_suffix
Since path_suffix is added to ProviderConfigToml, it should be supported by the configuration CLI helper methods in ConfigToml (get_value, set_value, unset_value, and list_values).
Currently, if a user runs codewhale config set providers.openai.path_suffix "", it will bypass the structured providers.openai table and get written as a flat key "providers.openai.path_suffix" = "" in self.extras at the root of the TOML. When the config is loaded, this flat key will not be deserialized into self.providers.openai.path_suffix, rendering the CLI configuration ineffective.
Please add path_suffix support for the providers in crates/config/src/lib.rs. For example, for openai (and similarly for other providers):
In get_value:
"providers.openai.path_suffix" => self.providers.openai.path_suffix.clone(),In set_value:
"providers.openai.path_suffix" => self.providers.openai.path_suffix = Some(value.to_string()),In unset_value:
"providers.openai.path_suffix" => self.providers.openai.path_suffix = None,In list_values:
if let Some(v) = self.providers.openai.path_suffix.as_ref() {
out.insert("providers.openai.path_suffix".to_string(), v.clone());
}![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if let Some(path_suffix) = path_suffix { | ||
| let base = base_url.trim_end_matches('/'); | ||
| let suffix = path_suffix.trim().trim_matches('/'); | ||
| if suffix.is_empty() { | ||
| return format!("{base}/{path}"); | ||
| } | ||
| return format!("{base}/{suffix}/{path}"); | ||
| } | ||
| if path.starts_with("beta/") { |
There was a problem hiding this comment.
path_suffix is explicitly set, the function short-circuits before the beta/ prefix handling, so a DeepSeek user who adds path_suffix = "v1" would get https://api.deepseek.com/v1/beta/completions instead of the correct https://api.deepseek.com/beta/completions. The FIM (fim_completion) call site already routes through self.api_url("beta/completions"), making it subject to this bypass.
| if let Some(path_suffix) = path_suffix { | |
| let base = base_url.trim_end_matches('/'); | |
| let suffix = path_suffix.trim().trim_matches('/'); | |
| if suffix.is_empty() { | |
| return format!("{base}/{path}"); | |
| } | |
| return format!("{base}/{suffix}/{path}"); | |
| } | |
| if path.starts_with("beta/") { | |
| if path.starts_with("beta/") && path_suffix.is_none() { | |
| return format!("{}/{}", unversioned_base_url(base_url), path); | |
| } | |
| if let Some(path_suffix) = path_suffix { | |
| let base = base_url.trim_end_matches('/'); | |
| let suffix = path_suffix.trim().trim_matches('/'); | |
| if suffix.is_empty() { | |
| return format!("{base}/{path}"); | |
| } | |
| return format!("{base}/{suffix}/{path}"); | |
| } | |
| if path.starts_with("beta/") { |
| let path = path.trim_start_matches('/'); | ||
| if let Some(path_suffix) = path_suffix { | ||
| let base = base_url.trim_end_matches('/'); | ||
| let suffix = path_suffix.trim().trim_matches('/'); |
There was a problem hiding this comment.
api_path_suffix() already calls .trim().trim_matches('/') before returning the value, so the same normalization here is redundant. This is harmless but may surprise callers who pass raw values directly (as the new tests do), since the function behaves correctly for both pre-trimmed and raw inputs.
| let suffix = path_suffix.trim().trim_matches('/'); | |
| let suffix = path_suffix.trim_matches('/'); |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
|
Thanks @cyq1017. I reviewed this for the v0.8.50 harvest pass and am leaving it out for now, because the idea is useful but the config surface is not complete enough to ship safely. Concrete next steps that would make this a stronger follow-up PR:
This still looks like a good direction for OpenAI-compatible gateways once those edges are handled. |
|
Thanks @cyq1017. I’m closing this as superseded and credited rather than leaving a conflicted draft sitting open. The provider-local path suffix support is now present on \ through the smaller #2558 harvest/cherry-pick commits \ + , and the v0.9 stewardship branch adds the follow-up hardening in #2776 / \ so repo-local project config cannot override endpoint path routing. The shipped behavior keeps the feature narrow: \ only rewrites chat-completions URLs for OpenAI-compatible gateways; model listing and DeepSeek beta/FIM paths keep their built-in routing. Verified with test result: ok. 5 passed; 0 failed; 0 ignored; 0 measured; 4189 filtered out; finished in 0.00s and test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 92 filtered out; finished in 0.00s. Your #2506 draft helped shape the config/provider review trail, and the v0.9 changelog credits @cyq1017 and @hongqitai for that path-suffix follow-up work alongside the original #1874 report. Thank you. |
|
Correction: the close note above was garbled by shell quoting. This is the authoritative closure note. Thanks @cyq1017. I’m closing this as superseded and credited rather than leaving a conflicted draft sitting open. The provider-local path suffix support is now present on The shipped behavior keeps the feature narrow: Verified with: cargo test -p codewhale-tui --bin codewhale-tui --locked api_url_with_suffix -- --nocapture
cargo test -p codewhale-config --locked project_merge_denies_credentials_endpoints_and_provider_selection -- --nocaptureYour #2506 draft helped shape the config/provider review trail, and the v0.9 changelog credits @cyq1017 and @hongqitai for that path-suffix follow-up work alongside the original #1874 report. Thank you. |
2 participants
Problem
/chat/completionsbut reject the default/v1/chat/completionspath. Todaybase_urlcan change the host, but not that path segment.Change
path_suffixsupport to provider config./v1default when the field is omitted.path_suffix = ""for root-level OpenAI-compatible endpoints, and trim surrounding slashes for custom suffixes.Verification
cargo test -p codewhale-tui api_url --all-features --locked -- --nocapturecargo test -p codewhale-tui openai_provider_accepts_custom_model_and_base_url --all-features --locked -- --nocapturecargo check -p codewhale-tui --all-features --lockedcargo fmt --all -- --checkgit diff --check origin/main..HEADCloses #2089
Closes #1874
Greptile Summary
This PR adds an optional
path_suffixconfig key to provider tables, letting users override the path segment betweenbase_urland API paths. The default/v1behavior is preserved when the field is omitted, andpath_suffix = ""enables root-level OpenAI-compatible endpoints.api_url_with_path_suffixis introduced as the shared URL builder; the oldapi_urlfree function is kept only for tests.api_path_suffix()onConfigpre-normalises the value (trim + slash-strip) before it reaches the HTTP layer.Confidence Score: 4/5
Safe to merge for all documented use cases; the edge case of combining path_suffix with DeepSeek FIM may warrant a follow-up.
The feature works correctly for its primary target (OpenAI-compatible gateways). The one weak spot is that setting path_suffix on a DeepSeek provider silently bypasses the beta-path routing used by fim_completion, which could misdirect FIM calls to /v1/beta/completions instead of /beta/completions. This combination is unlikely in practice but there is no guard or documentation note against it.
crates/tui/src/client.rs — the beta path guard and the path_suffix branch ordering.
Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A["self.api_url(path)"] --> B["api_url_with_path_suffix(base_url, path_suffix, path)"] B --> C{path_suffix set?} C -- "Some(suffix)" --> D{suffix is empty?} D -- "Yes (path_suffix = '')" --> E["base/path\ne.g. https://gw.example/chat/completions"] D -- "No (path_suffix = 'v2')" --> F["base/suffix/path\ne.g. https://gw.example/v2/chat/completions"] C -- "None (default)" --> G{path starts with 'beta/'?} G -- Yes --> H["unversioned_base/path\ne.g. https://api.deepseek.com/beta/completions"] G -- No --> I{versioned base ends with 'beta'?} I -- Yes --> J["unversioned_base/v1/path"] I -- No --> K["versioned_base/path\ne.g. https://api.deepseek.com/v1/chat/completions"]Reviews (1): Last reviewed commit: "feat(config): allow provider path suffix..." | Re-trigger Greptile