Repository navigation
fix(knowledge): 优化知识库创建样式并修复偶发RAG配置不全问题 - #1385
Merged
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR introduces RAG configuration mode selection and retrieval configuration normalization for knowledge base creation. The backend now supports ChangesRAG Configuration Mode and Retrieval Config Normalization
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/app/services/knowledge/orchestrator.py (1)
1161-1169:⚠️ Potential issue | 🟠 Major | ⚡ Quick winForward
rag_config_modeintoKnowledgeBaseCreateto avoid silent mode drift.Line 1161 builds
KnowledgeBaseCreatewithoutrag_config_mode, so it falls back to schema default"auto"even when the caller passed"disabled". This can lose user intent in downstream persistence/consumers ofKnowledgeBaseCreate.💡 Suggested fix
data = KnowledgeBaseCreate( name=name, description=description, namespace=namespace, kb_type=kb_type, + rag_config_mode=rag_config_mode, retrieval_config=resolved_retrieval_config, summary_enabled=summary_enabled, summary_model_ref=resolved_summary_model_ref, )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/app/services/knowledge/orchestrator.py` around lines 1161 - 1169, The KnowledgeBaseCreate construction omits rag_config_mode so it falls back to the schema default and can drop a caller-provided value; update the KnowledgeBaseCreate(...) call in orchestrator.py (the block that creates the data variable) to pass rag_config_mode=rag_config_mode (or the correctly scoped variable holding the caller's value) so the created object preserves the caller's intent downstream.
🧹 Nitpick comments (2)
backend/tests/schemas/test_knowledge_response.py (1)
7-21: ⚡ Quick winAdd type hints and a short docstring to
make_kind.This helper currently misses required Python typing/docstring conventions.
Suggested fix
+from typing import Any, Optional + -def make_kind(retrieval_config): +def make_kind(retrieval_config: Optional[dict[str, Any]]) -> SimpleNamespace: + """Build a minimal Kind-like object for knowledge response tests.""" return SimpleNamespace(As per coding guidelines, “Type hints are required for all function parameters and return values” and “Use descriptive names and docstrings for all public functions and classes.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/tests/schemas/test_knowledge_response.py` around lines 7 - 21, The helper function make_kind is missing type hints and a docstring: add a concise docstring describing that it builds a SimpleNamespace representing a fake knowledge base "kind", annotate the parameter retrieval_config as Mapping[str, Any] (or dict[str, Any]) and the return type as SimpleNamespace, and ensure imports/types (from types import SimpleNamespace, from typing import Mapping, Any) are present or adjusted; keep the function body unchanged except for these signature and docstring additions.Source: Coding guidelines
backend/tests/services/knowledge/test_orchestrator.py (1)
174-332: ⚡ Quick winAdd one regression test for
create_knowledge_base(..., rag_config_mode=...)pass-through.These tests validate
_resolve_retrieval_configwell, but they won’t catch mode loss at theKnowledgeBaseCreate(...)construction path. Add one integration-style unit test asserting the created payload preserves the requested mode.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/tests/services/knowledge/test_orchestrator.py` around lines 174 - 332, Add a regression test that ensures create_knowledge_base preserves rag_config_mode through to the KnowledgeBaseCreate payload: in backend/tests/services/knowledge/test_orchestrator.py add a test that patches the orchestrator's KnowledgeBaseCreate (or the constructor/factory used when creating the KB) with patch.object(orchestrator, "KnowledgeBaseCreate") as mock_kb_create, calls orchestrator.create_knowledge_base(..., rag_config_mode="disabled") (providing minimal required args), and asserts mock_kb_create.assert_called_once() and that the call args include rag_config_mode="disabled" (or that the constructed payload dict passed contains the requested mode); this will catch mode-loss regressions at the KnowledgeBaseCreate construction path.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/app/schemas/knowledge.py`:
- Around line 314-316: The code assumes `config` is a dict before calling
`.get()`, which can throw on legacy non-dict `retrievalConfig`; update the guard
so you first check `isinstance(config, dict)` (and likewise ensure
`embedding_config` is a dict) before accessing `.get()`—e.g., if `not
isinstance(config, dict): return` (or skip/drop this config) and set
`embedding_config = config.get("embedding_config") or {}` only after that check;
keep the existing return behavior (return `config` when
`config.get("retriever_name")` and `embedding_config.get("model_name")` are
present).
In `@frontend/src/features/knowledge/document/components/KnowledgeBaseForm.tsx`:
- Around line 351-361: The remove guided question Button currently uses tailwind
classes "h-9 w-9" (36×36px) which is below the 44px touch target; update the
Button (the component that calls handleRemoveGuidedQuestion) to use at least
44px (for example change className to "h-11 w-11") and increase the inner X icon
size (e.g., change X className to "h-5 w-5") so the visible icon remains
appropriately scaled; keep the existing onClick, data-testid, and variant/size
props unchanged.
---
Outside diff comments:
In `@backend/app/services/knowledge/orchestrator.py`:
- Around line 1161-1169: The KnowledgeBaseCreate construction omits
rag_config_mode so it falls back to the schema default and can drop a
caller-provided value; update the KnowledgeBaseCreate(...) call in
orchestrator.py (the block that creates the data variable) to pass
rag_config_mode=rag_config_mode (or the correctly scoped variable holding the
caller's value) so the created object preserves the caller's intent downstream.
---
Nitpick comments:
In `@backend/tests/schemas/test_knowledge_response.py`:
- Around line 7-21: The helper function make_kind is missing type hints and a
docstring: add a concise docstring describing that it builds a SimpleNamespace
representing a fake knowledge base "kind", annotate the parameter
retrieval_config as Mapping[str, Any] (or dict[str, Any]) and the return type as
SimpleNamespace, and ensure imports/types (from types import SimpleNamespace,
from typing import Mapping, Any) are present or adjusted; keep the function body
unchanged except for these signature and docstring additions.
In `@backend/tests/services/knowledge/test_orchestrator.py`:
- Around line 174-332: Add a regression test that ensures create_knowledge_base
preserves rag_config_mode through to the KnowledgeBaseCreate payload: in
backend/tests/services/knowledge/test_orchestrator.py add a test that patches
the orchestrator's KnowledgeBaseCreate (or the constructor/factory used when
creating the KB) with patch.object(orchestrator, "KnowledgeBaseCreate") as
mock_kb_create, calls orchestrator.create_knowledge_base(...,
rag_config_mode="disabled") (providing minimal required args), and asserts
mock_kb_create.assert_called_once() and that the call args include
rag_config_mode="disabled" (or that the constructed payload dict passed contains
the requested mode); this will catch mode-loss regressions at the
KnowledgeBaseCreate construction path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b7481cef-db46-465f-8691-03b2ddf7df30
📒 Files selected for processing (26)
backend/app/api/endpoints/knowledge.pybackend/app/api/endpoints/rag.pybackend/app/schemas/kind.pybackend/app/schemas/knowledge.pybackend/app/services/knowledge/orchestrator.pybackend/app/services/rag/runtime_resolver.pybackend/tests/schemas/test_knowledge_response.pybackend/tests/services/knowledge/test_orchestrator.pydocker-compose.ymldocs/en/reference/yaml-specification.mddocs/en/user-guide/knowledge/configuring-retrievers.mddocs/en/user-guide/knowledge/knowledge-base-guide.mddocs/zh/reference/yaml-specification.mddocs/zh/user-guide/knowledge/configuring-retrievers.mddocs/zh/user-guide/knowledge/knowledge-base-guide.mdfrontend/src/features/knowledge/document/components/CreateKnowledgeBaseDialog.tsxfrontend/src/features/knowledge/document/components/EditKnowledgeBaseDialog.tsxfrontend/src/features/knowledge/document/components/KnowledgeBaseForm.tsxfrontend/src/features/knowledge/document/components/KnowledgeDialogs.tsxfrontend/src/features/knowledge/document/components/KnowledgeDocumentPageMobile.tsxfrontend/src/features/knowledge/document/components/RetrievalSettingsSection.tsxfrontend/src/features/knowledge/document/components/RetrievalTestDialog.tsxfrontend/src/features/knowledge/document/hooks/useKnowledgeBaseDialogs.tsfrontend/src/i18n/locales/en/knowledge.jsonfrontend/src/i18n/locales/zh-CN/knowledge.jsonfrontend/src/types/knowledge.ts
💤 Files with no reviewable changes (1)
- docker-compose.yml
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/tests/schemas/test_knowledge_response.py (1)
7-21:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd type hints to comply with coding guidelines.
The helper function is missing type hints for the parameter and return value. As per coding guidelines, type hints are required for all function parameters and return values.
📝 Proposed fix
+from typing import Any + -def make_kind(retrieval_config): +def make_kind(retrieval_config: Any) -> SimpleNamespace: + """Create a stub Kind object for testing with the given retrieval config.""" return SimpleNamespace(🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/tests/schemas/test_knowledge_response.py` around lines 7 - 21, Annotate the make_kind helper: add type hints for the parameter and return value by changing its signature to def make_kind(retrieval_config: Mapping[str, Any]) -> SimpleNamespace:, and ensure you import Any and Mapping from typing (and keep/import SimpleNamespace from types). This provides a concrete parameter type for retrieval_config and documents the returned SimpleNamespace.Source: Coding guidelines
🧹 Nitpick comments (1)
backend/scripts/fix_kb_retriever_and_reindex.py (1)
30-503: 🏗️ Heavy liftAlign this new script with the repo's public-function contract.
This file adds several public helpers, but the repo Python rules are only partially applied: none of the functions are wrapped with
@trace_sync(),main()is missing a docstring, and multiple helpers still omit parameter type hints (db,kb,document, etc.). Please bring the script up to the same typing and telemetry standard as the rest of the backend.As per coding guidelines, "Type hints are required for all function parameters and return values", "Use descriptive names and docstrings for all public functions and classes", and "Use OpenTelemetry decorators from
shared/telemetry/decorators.py:@trace_async()or@trace_sync()for entire functions,add_span_event()for events,set_span_attribute()for attributes".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/scripts/fix_kb_retriever_and_reindex.py` around lines 30 - 503, The new script adds public helpers that lack full type hints, docstrings, and telemetry; update the following symbols: get_db_session, get_knowledge_base, get_documents, ensure_retriever_config, reindex_document, process_knowledge_base, and main so they conform to repo conventions: add precise parameter and return type hints (e.g., db: Session, kb: Kind, document: KnowledgeDocument, return types like Session | None, Optional[dict[str, Any]], dict[str, int], int), add or expand docstrings for public functions (main must have a docstring), import and apply the OpenTelemetry decorator `@trace_sync`() from shared.telemetry.decorators to each public function, and insert lightweight span events/attributes via add_span_event() or set_span_attribute() at key steps (for example in ensure_retriever_config when a fix is applied and in reindex_document before enqueue and on exceptions) so telemetry captures success/failure; ensure imports for typing (Optional, Any), SQLAlchemy Session, and model classes are added as needed and run a quick type-check to confirm signatures compile.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/scripts/fix_kb_retriever_and_reindex.py`:
- Around line 232-241: The code currently treats missing retriever_name, missing
embedding_model_name, or missing owner user as skipped by returning {"success":
False, "skipped": True, ...}, which lets process_knowledge_base/main treat the
run as successful; change these hard precondition failures so they are not
marked as skips: in the function that currently returns for those branches (the
block using retriever_name, embedding_model_name, user, log_error, and
document.id), return a failure that is not a skip — e.g. {"success": False,
"skipped": False, "reason": reason, **details} or omit the "skipped" key — so
only get_rag_indexing_skip_reason() cases remain true skips and
process_knowledge_base will surface the run as failing. Ensure all three
branches (missing retriever_name/embedding_model_name and missing user) are
updated consistently.
- Around line 97-101: The loop that builds parsed_ids converts stripped_id with
int(stripped_id) and will raise a raw ValueError for bad input (e.g. "--kb-ids
1,foo"); update the parsing to validate and surface a clean argparse-style
error: in main() (the code that parses --kb-ids) wrap the int(...) conversion in
a try/except and on failure call the ArgumentParser.error(...) or raise
argparse.ArgumentTypeError with a clear message (including the offending token)
so the CLI prints a normal usage/validation error instead of a traceback; ensure
parsed_ids is only appended after successful conversion.
- Around line 306-324: The exception handler in reindex_document leaves the
shared SQLAlchemy session (db) in a failed transaction state; call db.rollback()
immediately when catching the Exception (before calling
mark_document_index_enqueue_failed and before returning) so the session is reset
for subsequent iterations. Concretely, in the except Exception as exc block in
reindex_document (file: backend/scripts/fix_kb_retriever_and_reindex.py) invoke
db.rollback() (or db.session.rollback() if db is a Session-like object) right
after logging the error, then proceed to call mark_document_index_enqueue_failed
and the subsequent logging/returns; ensure any secondary exceptions from
mark_document_index_enqueue_failed do not skip a prior rollback (i.e., perform
rollback first, then try mark_document_index_enqueue_failed). This fixes the
shared-session PendingRollbackError cascade from prepare_document_index_enqueue
and unexpected exceptions.
---
Outside diff comments:
In `@backend/tests/schemas/test_knowledge_response.py`:
- Around line 7-21: Annotate the make_kind helper: add type hints for the
parameter and return value by changing its signature to def
make_kind(retrieval_config: Mapping[str, Any]) -> SimpleNamespace:, and ensure
you import Any and Mapping from typing (and keep/import SimpleNamespace from
types). This provides a concrete parameter type for retrieval_config and
documents the returned SimpleNamespace.
---
Nitpick comments:
In `@backend/scripts/fix_kb_retriever_and_reindex.py`:
- Around line 30-503: The new script adds public helpers that lack full type
hints, docstrings, and telemetry; update the following symbols: get_db_session,
get_knowledge_base, get_documents, ensure_retriever_config, reindex_document,
process_knowledge_base, and main so they conform to repo conventions: add
precise parameter and return type hints (e.g., db: Session, kb: Kind, document:
KnowledgeDocument, return types like Session | None, Optional[dict[str, Any]],
dict[str, int], int), add or expand docstrings for public functions (main must
have a docstring), import and apply the OpenTelemetry decorator `@trace_sync`()
from shared.telemetry.decorators to each public function, and insert lightweight
span events/attributes via add_span_event() or set_span_attribute() at key steps
(for example in ensure_retriever_config when a fix is applied and in
reindex_document before enqueue and on exceptions) so telemetry captures
success/failure; ensure imports for typing (Optional, Any), SQLAlchemy Session,
and model classes are added as needed and run a quick type-check to confirm
signatures compile.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 958f45be-26ed-4541-828d-da0abfded31e
📒 Files selected for processing (3)
backend/app/schemas/knowledge.pybackend/scripts/fix_kb_retriever_and_reindex.pybackend/tests/schemas/test_knowledge_response.py
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/app/schemas/knowledge.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Changes
Documentation