fix(embedding): remove forced /v1 suffix for embeddings#6589
fix(embedding): remove forced /v1 suffix for embeddings#6589ccsang wants to merge 3 commits intoAstrBotDevs:devfrom
Conversation
…Devs#6558 Add embedding_api_base_mode config (auto|force_v1|as_is) to default.py, modify openai_embedding_source.py to use resolve_openai_compatible_base_url() helper with mode parameter, add 7 test cases in tests/test_openai_embedding_source.py. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request resolves an issue where AstrBot's embedding adapter rigidly appended Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request introduces a flexible way to handle the /v1 suffix for embedding provider API base URLs by adding a new embedding_api_base_mode configuration. The implementation correctly centralizes the URL resolution logic into a new utility function and adds comprehensive tests. My review includes a suggestion to refactor some duplicated code in the new utility for better maintainability, and a fix for an incorrect assertion in one of the new tests to ensure the test suite is accurate.
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- Consider adding explicit runtime validation or normalization for
embedding_api_base_modevalues (e.g., mapping unknown strings toautowith a warning) so that typos in configuration don’t silently change behavior. - The logic for
force_v1andautoinresolve_openai_compatible_base_urlis very similar; you could simplify by factoring out the shared/v1-ensuring behavior into a common path and only branching where behavior truly differs.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider adding explicit runtime validation or normalization for `embedding_api_base_mode` values (e.g., mapping unknown strings to `auto` with a warning) so that typos in configuration don’t silently change behavior.
- The logic for `force_v1` and `auto` in `resolve_openai_compatible_base_url` is very similar; you could simplify by factoring out the shared `/v1`-ensuring behavior into a common path and only branching where behavior truly differs.
## Individual Comments
### Comment 1
<location path="astrbot/core/provider/utils.py" line_range="26-27" />
<code_context>
+ if not api_base:
+ return default_base
+
+ if mode == "as_is":
+ return api_base.removesuffix("/")
+
+ if mode == "force_v1":
</code_context>
<issue_to_address>
**issue (bug_risk):** "as_is" mode still normalizes the URL by stripping the trailing slash
This currently contradicts the docstring promise of returning the URL “as-is”. Please either (a) update the docstring to clarify that trailing slashes are removed, or (b) return `api_base` unchanged in `as_is` (skipping `removesuffix('/')`) so behavior matches the documentation and caller expectations.
</issue_to_address>
### Comment 2
<location path="astrbot/core/config/default.py" line_range="1941-1945" />
<code_context>
"description": "API Base URL",
"type": "string",
},
+ "embedding_api_base_mode": {
+ "description": "API Base URL Mode",
+ "type": "string",
+ "hint": "控制 API Base URL 的 /v1 后缀处理方式。auto: 自动判断(默认),如果 URL 不以 /v1 结尾则添加。force_v1: 强制添加 /v1 后缀。as_is: 保持原样不做修改。",
+ },
"volcengine_cluster": {
</code_context>
<issue_to_address>
**suggestion:** Consider constraining `embedding_api_base_mode` to a fixed set of allowed values
The schema currently treats `embedding_api_base_mode` as a free-form string, but the implementation only supports `"auto" | "force_v1" | "as_is"`. If the config system supports enums (or equivalent), restricting this field to those values would better match runtime behavior and avoid misconfiguration.
```suggestion
"embedding_api_base_mode": {
"description": "API Base URL Mode",
"type": "string",
"enum": ["auto", "force_v1", "as_is"],
"hint": "控制 API Base URL 的 /v1 后缀处理方式。auto: 自动判断(默认),如果 URL 不以 /v1 结尾则添加。force_v1: 强制添加 /v1 后缀。as_is: 保持原样不做修改。",
},
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fa22ca83f1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
- Fix as_is mode to return URL unchanged (including trailing slashes) - Add enum constraint to embedding_api_base_mode config - Update docstring to clarify as_is behavior
- Fix test assertion to expect trailing slash from OpenAI client - Simplify code by merging identical force_v1 and auto logic - Add documentation for as_is mode limitation (no /embeddings path) - Update config hint to clarify as_is mode usage
71850da to
daf0fcc
Compare
|
#6669 fixed |
Fixes #6558
Problem
AstrBot's embedding adapter unconditionally appends /v1 to all API base URLs.
Solution
Add embedding_api_base_mode config option (auto|force_v1|as_is).
Testing
Co-Authored-By: Claude Sonnet 4.6 noreply@anthropic.com
Summary by Sourcery
Make OpenAI-compatible embedding providers use a shared helper to resolve the API base URL and support configurable handling of the /v1 suffix.
New Features:
Bug Fixes:
Enhancements:
Tests: