feat(config, tui): Configurable path suffix for OpenAI-compat endpoints (#2089)#2508
feat(config, tui): Configurable path suffix for OpenAI-compat endpoints (#2089)#2508hongqitai wants to merge 6 commits into
Conversation
|
Thanks @hongqitai. I reviewed this during the v0.8.50 triage pass. This is a useful direction for #2089, but I am not harvesting it into #2504 yet because the current patch is broader than the release slice and has a couple of correctness gaps. Concrete blockers to address:
Once those are fixed, this could become the stronger follow-up compared with the smaller draft in #2506. |
There was a problem hiding this comment.
Code Review
This pull request introduces a new path_suffix configuration option across CLI arguments, TOML configuration files, environment variables, and the TUI client, allowing users to customize or override the default /v1 API version suffix appended to base URLs. Feedback on the changes highlights a bug where the custom path_suffix is ignored if the base URL already contains a version suffix (e.g., /v1 or /v4). To resolve this, it is recommended to strip any existing version suffix from the base URL when a custom path_suffix is explicitly provided, and to update the corresponding test assertions to verify this behavior.
3b9e328 to
ade16fd
Compare
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
65eca12 to
5286ac4
Compare
|
Fix all pronlems, and add suport for new changes |
|
Thanks @hongqitai for the update. I see the branch is now green, which is a good sign. I am keeping it out of the v0.8.52 release freeze because configurable OpenAI-compatible path suffixes are a broad provider/config/docs feature rather than a cleanup fix for the 0.8.51 fallout. This should get a focused review after 0.8.52 is published so we do not bury your work inside the emergency release lane. |
Summary
Closes #2089, Closes #1874
Add path_suffix into config, set it to change version:
Testing
cargo fmt --all -- --checkcargo clippy --workspace --all-targets --all-featurescargo test --workspace --all-featuresChecklist
Greptile Summary
This PR adds a configurable
path_suffixfield to all OpenAI-compatible provider configs, letting users override or suppress the automatically-appended/v1version segment in API base URLs. The feature is surfaced via TOML config, environment variables (one genericDEEPSEEK_PATH_SUFFIX/CODEWHALE_PATH_SUFFIXthat routes by active provider, plus per-provider vars likeOPENAI_PATH_SUFFIX), and a new--path-suffixCLI flag.versioned_base_urlandapi_urlare refactored to accept anOption<&str>path suffix; anormalize_versionhelper strips leading slashes so both"v2"and"/v2"are equivalent.ARCEE_PATH_SUFFIXis implemented in the config crate and wired up inpath_suffix_for, but is absent from thedocs/CONFIGURATION.mdenv-var listing, leaving Arcee users with no documentation for the feature.Confidence Score: 4/5
Safe to merge with minor documentation fixes; the core URL-building logic is well-tested and the config plumbing is mechanically sound across all providers.
The functional changes are thoroughly tested with unit tests covering empty suffix, non-empty suffix, with and without leading slash, and interaction with beta/versioned base URLs. Documentation has two gaps: missing ARCEE_PATH_SUFFIX entry and unclear description of path_suffix='' stripping behavior.
docs/CONFIGURATION.md needs ARCEE_PATH_SUFFIX added to the per-provider listing and a clearer description of the path_suffix='' stripping behaviour.
Important Files Changed
versioned_base_urlandapi_urlnow acceptOption<&str> path_suffix; anormalize_versionhelper strips leading slashes. All call sites updated and well-tested.path_suffixto all config structs with correct get/set/unset/list coverage for all providers.ARCEE_PATH_SUFFIXis wired up but missing from docs.path_suffixtoConfigandProviderConfig; newpath_suffix()method and env-var routing are consistent with existing patterns.DoctorApiTargetand diagnostic output updated;merge_configandmerge_provider_configcorrectly propagatepath_suffix.--path-suffixCLI flag, correctly routed viaDEEPSEEK_PATH_SUFFIXenv var to all providers.path_suffixbroadly but omitsARCEE_PATH_SUFFIXand understates the stripping behavior ofpath_suffix = "".self.path_suffix.as_deref(). Straightforward change.Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[User config / env var / CLI --path-suffix] --> B{Priority resolution} B --> |1st| C[CLI path_suffix flag] B --> |2nd| D[Provider-specific env var] B --> |3rd| E[Generic CODEWHALE_PATH_SUFFIX] B --> |4th| F[Provider config file] B --> |5th| G[Root deepseek config] C & D & E & F & G --> H[Resolved path_suffix] H --> I{api_url called} I --> |beta/ path| J[unversioned_base_url + path] I --> |other paths| K[normalize_version] K --> L{versioned_base_url} L --> |Some version| M[unversioned + / + version] L --> |None + /beta| N[unversioned + /v1] L --> |None + versioned| O[base_url as-is] L --> |None + bare| P[base_url + /v1] M & N & O & P --> Q[final URL]Reviews (5): Last reviewed commit: "Merge branch 'main' into feature/path-su..." | Re-trigger Greptile