Skip to content

fix(kb): resolve kb_id path parameter in dashboard retrieve endpoint - #9485

Open
JosephTian876 wants to merge 2 commits into
AstrBotDevs:masterfrom
JosephTian876:fix/kb-retrieve-kb-id
Open

JosephTian876 wants to merge 2 commits into
AstrBotDevs:masterfrom
JosephTian876:fix/kb-retrieve-kb-id

Conversation

@JosephTian876

@JosephTian876 JosephTian876 commented Jul 31, 2026 •

Copy link
Copy Markdown
Contributor

The knowledge base retrieval panel in the WebUI is completely unusable: every
search fails with 缺少参数 kb_names 或格式错误.

POST /api/v1/knowledge-bases/{kb_id}/retrieve identifies the knowledge base
with the kb_id path parameter, and the route already forwards it:

# astrbot/dashboard/api/knowledge_bases.py:295
@router.post("/knowledge-bases/{kb_id}/retrieve")
async def retrieve_knowledge_base(kb_id: str, payload: KnowledgeBaseRetrieveRequest, ...):
    body = payload.model_dump(exclude_none=True)
    return await _run(lambda: service.retrieve({"kb_id": kb_id, **body}), prefix="检索失败")

But KnowledgeBaseService.retrieve() only ever read kb_names, which this
route never supplies (KnowledgeBaseRetrieveRequest has no such field, and the
dashboard client sends only query / top_k / score_threshold). So kb_names
was structurally always None and the request was rejected before any lookup
happened — reproducible 100% of the time, for any knowledge base and any query.

The service's retrieve() was written for the legacy POST /api/kb/retrieve
endpoint, which does pass kb_names in the body. When the v1 REST route was
introduced in #8688, the kb_id → kb_names translation was never added.

Modifications / 改动点

astrbot/dashboard/services/knowledge_base_service.py — when kb_names is
absent, resolve the kb_id the route already passes into the kb_names the
manager expects:

if not kb_names:
    # The REST route identifies the knowledge base with the kb_id path
    # parameter, so resolve it into the kb_names the manager expects.
    kb_id = payload.get("kb_id")
    if kb_id:
        kb_helper = await kb_manager.get_kb(kb_id)
        if not kb_helper:
            raise KnowledgeBaseServiceError("知识库不存在")
        kb_names = [kb_helper.kb.kb_name]

Resolving through the name is safe because KnowledgeBaseManager.create_kb()
already enforces unique kb_name (知识库名称 '{kb_name}' 已存在).

An explicit kb_names still takes precedence, so the legacy
POST /api/kb/retrieve endpoint keeps working unchanged.

tests/unit/test_knowledge_base_service_contract.py — 4 regression tests:
resolution from kb_id, the route→service binding, the legacy kb_names path,
and the two error cases.

  • This is NOT a breaking change. / 这不是一个破坏性变更。

Screenshots or Test Results / 运行截图或测试结果

The new tests fail on master and pass with the fix (same test code both runs):

# master, with the new tests applied but WITHOUT the source fix
$ uv run pytest tests/unit/test_knowledge_base_service_contract.py -q
FAILED tests/unit/test_knowledge_base_service_contract.py::test_retrieve_resolves_kb_id_from_path_parameter
FAILED tests/unit/test_knowledge_base_service_contract.py::test_retrieve_route_passes_kb_id_to_service
FAILED tests/unit/test_knowledge_base_service_contract.py::test_retrieve_raises_when_kb_id_is_unknown
3 failed, 15 passed in 4.70s

# with the fix
$ uv run pytest tests/unit/test_knowledge_base_service_contract.py -q
18 passed, 1 warning in 3.91s

Surrounding suites, no new failures:

$ uv run pytest tests/unit/test_kb_manager_resilience.py tests/unit/test_kb_document_cleanup.py \
    tests/unit/test_kb_upload_atomicity.py tests/unit/test_dashboard_util.py \
    tests/test_fastapi_v1_dashboard.py tests/test_dashboard.py tests/test_kb_import.py -q
3 failed, 186 passed in 62.87s

The 3 failures are the pre-existing tests/test_dashboard.py::test_t2i_* cases;
they fail identically on a clean master checkout and are unrelated to this change.

$ uv run ruff format --check .
483 files already formatted
$ uv run ruff check .
All checks passed!

No API schema or OpenAPI definition changed, so no frontend client regeneration
is needed.


Checklist / 检查清单

  • 😊 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.txt and pyproject.toml.
    / 我确保没有引入新依赖库,或者引入了新依赖库的同时将其添加到 requirements.txt 和 pyproject.toml 文件相应位置。

  • 😮 My changes do not introduce malicious code.
    / 我的更改没有引入恶意代码。

Summary by Sourcery

Fix dashboard knowledge-base retrieval so requests targeting a knowledge base by ID complete successfully.

Bug Fixes:

  • Fix knowledge-base dashboard retrieval by resolving the REST route's kb_id into the knowledge-base name expected by the retrieval manager.
  • Return a clear error when the requested knowledge base does not exist while preserving legacy kb_names requests.

Tests:

  • Add regression coverage for path-parameter resolution, route-to-service forwarding, legacy name-based retrieval, and missing knowledge-base errors.

@dosubot dosubot Bot added size:XS This PR changes 0-9 lines, ignoring generated files. area:webui The bug / feature is about webui(dashboard) of astrbot. feature:knowledge-base The bug / feature is about knowledge base labels Jul 31, 2026
@dosubot

dosubot Bot commented Jul 31, 2026

Copy link
Copy Markdown

📄 Knowledge review

Dosu skipped reviewing this PR because your organization has used its 200 included credits for the month. Your usage will reset on 2026-08-01. To have Dosu review this PR before then, ask your organization admin to upgrade to a pro account.


Leave Feedback Ask Dosu about AstrBot Add Dosu to your team

@sourcery-ai sourcery-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey - I've left some high level feedback:

  • In KnowledgeBaseService.retrieve, when both kb_names and kb_id are provided, kb_names silently wins; consider either validating that they are consistent (same KB) or explicitly documenting and enforcing the precedence to avoid confusing, hard-to-debug situations.
  • The retrieval logic now depends on raw dict access to kb_id (payload.get("kb_id")); if this pattern grows, consider introducing a small typed request object or helper to centralize the kb_id → kb_names resolution and reduce the implicit contract on the data shape.
Prompt for AI Agents
Please address the comments from this code review:

## Overall Comments
- In `KnowledgeBaseService.retrieve`, when both `kb_names` and `kb_id` are provided, `kb_names` silently wins; consider either validating that they are consistent (same KB) or explicitly documenting and enforcing the precedence to avoid confusing, hard-to-debug situations.
- The retrieval logic now depends on raw `dict` access to `kb_id` (`payload.get("kb_id")`); if this pattern grows, consider introducing a small typed request object or helper to centralize the `kb_id` → `kb_names` resolution and reduce the implicit contract on the `data` shape.

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

@JosephTian876
JosephTian876 force-pushed the fix/kb-retrieve-kb-id branch from daef07a to 555ac39 Compare July 31, 2026 17:48
@dosubot dosubot Bot added size:S This PR changes 10-29 lines, ignoring generated files. and removed size:XS This PR changes 0-9 lines, ignoring generated files. labels Jul 31, 2026
@JosephTian876

Copy link
Copy Markdown
Contributor Author

Thanks for the review. Addressed the first point, and I'd like to push back on the second.

> when both kb_names and kb_id are provided, kb_names silently wins

Fair — the precedence was only stated in the PR description, not in the code. KnowledgeBaseRetrieveRequest declares no kb_names field, but it extends OpenModel (extra="allow"), so a client can smuggle one into the REST route's body and override the path parameter.

I've documented it in 6b9cb8a rather than rejecting the combination, because the two are not interchangeable: kb_names is a list and is how the legacy POST /api/kb/retrieve endpoint searches several bases at once, while kb_id addresses exactly one. Consistency validation would have to define what "consistent" means for {"kb_id": "a", "kb_names": ["a", "b"]}, and no client sends both today. retrieve() also had no docstring at all, which AGENTS.md asks for, so this fills that gap too.

> consider introducing a small typed request object or helper to centralize the kb_id → kb_names resolution

I'd rather not, as it would work against the repo's own guidelines:

  • AGENTS.md → No Unnecessary Helpers: "Inline-First Rule: If a logic block can be implemented directly within the main function without breaking overall readability, do not extract it into a new helper function", and a helper needs "High Reuse: The exact same logic is repeated across 3 or more different locations". This resolution happens in exactly one place.
  • Every sibling method in KnowledgeBaseService (create_kb, update_kb, delete_chunk, list_chunks, …) reads its input the same way via payload.get(...). A typed object here only would make this one method inconsistent with the rest of the service.

The suggestion is prefixed with "if this pattern grows" — agreed, and at that point the right fix is converting the whole service at once, not just this method. Happy to revisit if a maintainer prefers otherwise.

`POST /api/v1/knowledge-bases/{kb_id}/retrieve` identifies the knowledge
base with the `kb_id` path parameter, and the route already forwards it as
`service.retrieve({"kb_id": kb_id, **body})`. However `retrieve()` only ever
read `kb_names`, which that route never supplies, so every request from the
WebUI retrieval panel failed with "缺少参数 kb_names 或格式错误".

`kb_names` is still honoured when present, keeping the legacy
`POST /api/kb/retrieve` endpoint working unchanged.
The method had no docstring, so the precedence between `kb_names` and the
`kb_id` path parameter was only discoverable by reading the body. Spell out
that an explicit `kb_names` wins, which is what keeps the legacy multi-base
`POST /api/kb/retrieve` endpoint working.

@sourcery-ai sourcery-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sourcery assessment

Approved.

@JosephTian876

Copy link
Copy Markdown
Contributor Author

Rebased onto current master (dffe92277, v4.29.1). New head: 8839a8153e12905fbbea97dd7205fd365a71af88.

Bug still reproduces on master: POST /knowledge-bases/{kb_id}/retrieve still forwards {"kb_id": kb_id, **body}, while KnowledgeBaseService.retrieve() only reads kb_names, and KnowledgeBaseRetrieveRequest still exposes only query/top_k/threshold/rerank — so the request can never carry kb_names and always fails with "missing kb_names". The fix (resolve kb_id to a kb name via kb_manager.get_kb() when kb_names is absent) is therefore still required.

Conflicts: none. Both commits (the fix and the docstring) replayed cleanly; the service file had drifted on master but the touched region was untouched.

Verification:

  • pytest tests/unit/test_knowledge_base_service_contract.py -> 19 passed
  • All KB-related suites -> 64 passed
  • ruff format --check and ruff check clean on the changed file

CI has been triggered on the new head.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:webui The bug / feature is about webui(dashboard) of astrbot. feature:knowledge-base The bug / feature is about knowledge base size:S This PR changes 10-29 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant