fix: preserve explicit rerank api paths#7165
fix: preserve explicit rerank api paths#7165whatevertogo wants to merge 3 commits intoAstrBotDevs:masterfrom
Conversation
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- In
rerank,response_datais assumed to be adict(calling.geton it) but only type-checked as a dict inside the error branch; consider validating the JSON type once up front or handling non-dict responses explicitly to avoidAttributeErrorin unexpected cases. - The base URL is stripped twice (
rstrip('/')in__init__andremovesuffix('/')in_resolve_rerank_endpoint); consolidating normalization in a single place (e.g., inside_resolve_rerank_endpoint) would simplify the logic and reduce the chance of subtle inconsistencies. - The info-level log
[vLLM Rerank] Using API URL: ...will be emitted on every provider initialization; consider downgrading this to debug or making it conditional to avoid noisy logs in production deployments.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `rerank`, `response_data` is assumed to be a `dict` (calling `.get` on it) but only type-checked as a dict inside the error branch; consider validating the JSON type once up front or handling non-dict responses explicitly to avoid `AttributeError` in unexpected cases.
- The base URL is stripped twice (`rstrip('/')` in `__init__` and `removesuffix('/')` in `_resolve_rerank_endpoint`); consolidating normalization in a single place (e.g., inside `_resolve_rerank_endpoint`) would simplify the logic and reduce the chance of subtle inconsistencies.
- The info-level log `[vLLM Rerank] Using API URL: ...` will be emitted on every provider initialization; consider downgrading this to debug or making it conditional to avoid noisy logs in production deployments.
## Individual Comments
### Comment 1
<location path="astrbot/core/provider/sources/vllm_rerank_source.py" line_range="17-29" />
<code_context>
)
class VLLMRerankProvider(RerankProvider):
+ @staticmethod
+ def _resolve_rerank_endpoint(base_url: str) -> str:
+ normalized = base_url.strip().removesuffix("/")
+ if normalized.endswith("/rerank"):
+ return normalized
+ if normalized.endswith("/v1"):
+ return f"{normalized}/rerank"
+
+ parsed = urlsplit(normalized if "://" in normalized else f"//{normalized}")
+ if not parsed.path or parsed.path == "/":
+ return f"{normalized}/v1/rerank"
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Normalize and validate schemeless base URLs more explicitly to avoid subtle endpoint issues.
Because `urlsplit(normalized if '://' in normalized else f'//{normalized}')` keeps schemeless inputs unchanged (e.g. `example.com` or `example.com/api`), `self.endpoint_url` can end up without a scheme, which aiohttp will reject and which behaves differently depending on whether a path is present. Please either enforce/add a default scheme (e.g. `http`) for host-only inputs or fail fast when `base_url` lacks a scheme so this configuration issue is explicit and easier to debug.
```suggestion
class VLLMRerankProvider(RerankProvider):
@staticmethod
def _resolve_rerank_endpoint(base_url: str) -> str:
"""
Normalize and resolve the rerank endpoint from a base URL.
Rules:
- If the base URL already ends with `/rerank`, use it as-is.
- If it ends with `/v1`, append `/rerank`.
- If it is host-only (with or without scheme), append `/v1/rerank`.
- If it has a path but no scheme (e.g. `example.com/api`), fail fast.
- For host-only schemeless URLs (e.g. `example.com`), default to `http://`.
"""
normalized = base_url.strip().removesuffix("/")
# Fast-path for already well-formed endpoints
if normalized.endswith("/rerank"):
return normalized
if normalized.endswith("/v1"):
return f"{normalized}/rerank"
has_scheme = "://" in normalized
# For schemeless inputs we still want to parse host vs path correctly
parsed = urlsplit(normalized if has_scheme else f"//{normalized}")
if not has_scheme:
# Schemeless with a non-root path is ambiguous: require explicit scheme
if parsed.path not in ("", "/"):
raise ValueError(
"VLLMRerankProvider base_url must include a URL scheme when a path is provided "
f"(got {base_url!r})"
)
# Host-only input like `example.com` or `example.com:8000`: default to http
normalized = f"http://{normalized}"
has_scheme = True
parsed = urlsplit(normalized)
# If no path or root path, assume `/v1/rerank` under the given base
if not parsed.path or parsed.path == "/":
return f"{normalized}/v1/rerank"
# Non-root path with scheme: use as-is (caller is responsible for correctness)
return normalized
```
</issue_to_address>
### Comment 2
<location path="astrbot/core/provider/sources/vllm_rerank_source.py" line_range="65-69" />
<code_context>
payload["top_n"] = top_n
assert self.client is not None
async with self.client.post(
- f"{self.base_url}/v1/rerank",
+ self.endpoint_url,
json=payload,
) as response:
+ response.raise_for_status()
response_data = await response.json()
+ if isinstance(response_data, dict) and "error" in response_data:
</code_context>
<issue_to_address>
**suggestion:** Reconsider the order of `raise_for_status` vs JSON parsing to preserve API error details.
Because `response.raise_for_status()` runs first, any non-2xx response will raise an `aiohttp` error before your `
Suggested implementation:
```python
async with self.client.post(
self.endpoint_url,
json=payload,
) as response:
# Parse the JSON body first so we can surface any structured API
# error payload before aiohttp raises on non-2xx status codes.
response_data = await response.json()
if isinstance(response_data, dict) and "error" in response_data:
```
```python
error = response_data["error"]
if isinstance(error, dict):
code = error.get("code", "unknown")
message = error.get("message", "Unknown rerank API error")
raise ValueError(f"Rerank API error {code}: {message}")
raise ValueError(f"Rerank API error: {error}")
# Only raise for HTTP status errors after we've inspected the body
# for API-specific error details.
response.raise_for_status()
results = response_data.get("results", [])
```
</issue_to_address>
### Comment 3
<location path="astrbot/core/provider/sources/vllm_rerank_source.py" line_range="69-78" />
<code_context>
+ self.endpoint_url,
json=payload,
) as response:
+ response.raise_for_status()
response_data = await response.json()
+ if isinstance(response_data, dict) and "error" in response_data:
+ error = response_data["error"]
+ if isinstance(error, dict):
+ code = error.get("code", "unknown")
+ message = error.get("message", "Unknown rerank API error")
+ raise ValueError(f"Rerank API error {code}: {message}")
+ raise ValueError(f"Rerank API error: {error}")
results = response_data.get("results", [])
if not results:
</code_context>
<issue_to_address>
**issue:** Guard against non-dict JSON responses before accessing `results`.
Currently you only special-case dicts with an `"error"` field, then always call `response_data.get("results", [])`. If the API ever returns a list or other non-dict JSON, this will raise `AttributeError`. Add an `isinstance(response_data, dict)` guard before accessing `"results"`, and raise a clear error if the schema is unexpected.
</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.
Pull request overview
Adjusts how AstrBot’s vllm_rerank provider resolves the rerank endpoint URL so user-supplied rerank API bases that already include a path (or a full endpoint) are preserved, while still supporting “host-only” bases via automatic suffixing.
Changes:
- Add endpoint resolution logic (
_resolve_rerank_endpoint) and use the resolved URL for requests inVLLMRerankProvider. - Improve error surfacing from the upstream rerank HTTP response.
- Update dashboard config hints and docs (ZH/EN) describing the rerank API base behavior.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
astrbot/core/provider/sources/vllm_rerank_source.py |
Adds URL resolution for rerank endpoint and uses it for POST requests; improves error handling. |
astrbot/core/config/default.py |
Updates config metadata hint for rerank_api_base. |
dashboard/src/i18n/locales/zh-CN/features/config-metadata.json |
Updates Chinese hint text for rerank_api_base. |
dashboard/src/i18n/locales/en-US/features/config-metadata.json |
Updates English hint text for rerank_api_base. |
dashboard/src/i18n/locales/ru-RU/features/config-metadata.json |
Updates Russian hint text for rerank_api_base. |
docs/zh/use/knowledge-base.md |
Updates Chinese knowledge-base doc to describe rerank API base URL behavior. |
docs/en/use/knowledge-base.md |
Updates English knowledge-base doc to describe rerank API base URL behavior. |
There was a problem hiding this comment.
Code Review
This pull request enhances the vLLM rerank provider by implementing a more flexible API base URL resolution logic, which automatically appends /v1/rerank to domains while preserving full paths. It also introduces comprehensive error handling for API responses and updates configuration hints and documentation in multiple languages. Feedback was provided to refactor the initialization logic in VLLMRerankProvider to eliminate redundant string operations and unnecessary instance variables.
This fixes the rerank API base URL handling for the
vllm_rerankprovider.fix #5699
Modifications / 改动点
Preserve explicit rerank paths instead of always appending
/v1/rerankAppend
/v1/rerankonly whenrerank_api_baseis just a host, and append/rerankonly when it already ends with/v1Surface upstream rerank API errors directly instead of falling back to a generic
no results returnedfailureUpdate config metadata hints and the Chinese/English knowledge-base docs to reflect the new behavior
This is NOT a breaking change. / 这不是一个破坏性变更。
Screenshots or Test Results / 运行截图或测试结果
Verification steps:
Local validation covered these cases for
VLLMRerankProvider._resolve_rerank_endpoint:http://127.0.0.1:8000 -> http://127.0.0.1:8000/v1/rerankhttp://127.0.0.1:8000/v1 -> http://127.0.0.1:8000/v1/rerankhttp://127.0.0.1:8000/v1/rerank -> http://127.0.0.1:8000/v1/rerankhttps://example.com/api/v3/rerank -> https://example.com/api/v3/rerankhttps://example.com/custom/path -> https://example.com/custom/pathChecklist / 检查清单
😊 If there are new features added in the PR, I have discussed it with the authors through issues/emails, etc.
/ 如果 PR 中有新加入的功能,已经通过 Issue / 邮件等方式和作者讨论过。
👀 My changes have been well-tested, and "Verification Steps" and "Screenshots" have been provided above.
/ 我的更改经过了良好的测试,并已在上方提供了“验证步骤”和“运行截图”。
🤓 I have ensured that no new dependencies are introduced, OR if new dependencies are introduced, they have been added to the appropriate locations in
requirements.txtandpyproject.toml./ 我确保没有引入新依赖库,或者引入了新依赖库的同时将其添加到
requirements.txt和pyproject.toml文件相应位置。😮 My changes do not introduce malicious code.
/ 我的更改没有引入恶意代码.
Summary by Sourcery
Improve vLLM rerank provider URL handling and error reporting while updating related configuration hints and docs.
Bug Fixes:
Enhancements:
Documentation:
rerank api base.