fix: preserve embedding api version suffixes#8736
Conversation
There was a problem hiding this comment.
Code Review
This pull request refactors the API base normalization logic for the OpenAI embedding source into a dedicated helper function _normalize_api_base and adds corresponding unit tests. The reviewer identified a potential AttributeError if embedding_api_base is explicitly configured as None or an empty string, and provided a code suggestion to safely fall back to the default URL using the or operator.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| api_base = _normalize_api_base( | ||
| provider_config.get("embedding_api_base", "https://api.openai.com/v1") | ||
| .strip() | ||
| .removesuffix("/") | ||
| .removesuffix("/embeddings") | ||
| ) |
There was a problem hiding this comment.
If embedding_api_base is explicitly configured as None or an empty string in the provider configuration, provider_config.get("embedding_api_base", "https://api.openai.com/v1") will return None or "". This will cause _normalize_api_base to raise an AttributeError (since None has no strip method) or fail to apply the default base URL.
Using or instead of the get method's default argument ensures that we fall back to the default OpenAI API base URL if the configured value is falsy (such as None or "").
| api_base = _normalize_api_base( | |
| provider_config.get("embedding_api_base", "https://api.openai.com/v1") | |
| .strip() | |
| .removesuffix("/") | |
| .removesuffix("/embeddings") | |
| ) | |
| api_base = _normalize_api_base( | |
| provider_config.get("embedding_api_base") or "https://api.openai.com/v1" | |
| ) |
There was a problem hiding this comment.
Hey - I've found 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="tests/test_openai_embedding_source.py" line_range="12-16" />
<code_context>
+ assert _normalize_api_base("https://example.test/v4") == "https://example.test/v4"
+
+
+def test_openai_embedding_api_base_adds_default_version():
+ assert _normalize_api_base("https://example.test/openai") == (
+ "https://example.test/openai/v1"
+ )
+ assert _normalize_api_base("https://example.test/v1/embeddings") == (
+ "https://example.test/v1"
+ )
</code_context>
<issue_to_address>
**suggestion:** Cover behavior for empty or whitespace-only bases and non-version `/embeddings` bases
The default-version behavior is only partially covered here. Please also add cases for:
- `"https://example.test/embeddings"` → `"https://example.test/v1"`, as described in the PR but not asserted.
- Empty / whitespace-only input (e.g., `""`, `" "`), which currently returns an empty string.
These will lock in the `/embeddings` trimming and fallback behavior and help prevent regressions.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| def test_openai_embedding_api_base_adds_default_version(): | ||
| assert _normalize_api_base("https://example.test/openai") == ( | ||
| "https://example.test/openai/v1" | ||
| ) | ||
| assert _normalize_api_base("https://example.test/v1/embeddings") == ( |
There was a problem hiding this comment.
suggestion: Cover behavior for empty or whitespace-only bases and non-version /embeddings bases
The default-version behavior is only partially covered here. Please also add cases for:
"https://example.test/embeddings"→"https://example.test/v1", as described in the PR but not asserted.- Empty / whitespace-only input (e.g.,
""," "), which currently returns an empty string.
These will lock in the /embeddings trimming and fallback behavior and help prevent regressions.
Summary
/vNsuffix when normalizing OpenAI-compatible embedding API bases/v1when a base URL has no version suffix/v3,/v4, unversioned bases, and/embeddingssuffix trimmingWhy
Fixes #8732. Providers such as Volcengine Ark can use embedding endpoints ending in
/v3. The old check only recognized/v1and/v4, sohttps://.../v3becamehttps://.../v3/v1and returned 404.Verified
python -m py_compile astrbot\core\provider\sources\openai_embedding_source.py tests\test_openai_embedding_source.py.\.venv\Scripts\python.exe -m ruff check astrbot\core\provider\sources\openai_embedding_source.py tests\test_openai_embedding_source.pygit diff --check/v3,/v4, unversioned base URLs, and/v1/embeddingsI also tried
.\.venv\Scripts\python.exe -m pytest tests\test_openai_embedding_source.py -qin a minimal local venv. Collection reached the sharedtests/conftest.pydependency chain and stopped on missing full AstrBot runtime dependencies; I did not install the whole application dependency set just for this small provider normalization check.Summary by Sourcery
Preserve and centralize normalization of OpenAI-compatible embedding API base URLs while ensuring a sensible default version is applied.
Bug Fixes:
Enhancements:
Tests: