Repository navigation
feat: add provider-neutral external knowledge sources - #1557
Merged
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds end-to-end external knowledge source support: a pluggable retrieval-source registry, internal RAG and listing endpoints, task-level ChangesExternal Knowledge Sources Feature
Sequence Diagram(s)sequenceDiagram
participant User
participant KnowledgeSourcePicker
participant ContextSelector
participant useChatStreamHandlers
participant InternalRetrieve as POST /internal/rag/retrieve
participant RetrievalSourceRegistry
participant ExternalProvider
User->>KnowledgeSourcePicker: select external KB or document
KnowledgeSourcePicker-->>ContextSelector: onSelect(ExternalKnowledgeContext)
ContextSelector-->>useChatStreamHandlers: selectedContexts updated
useChatStreamHandlers->>InternalRetrieve: contexts with external_knowledge_refs, user_id
InternalRetrieve->>RetrievalSourceRegistry: get(provider)
RetrievalSourceRegistry-->>InternalRetrieve: RetrievalSourceProvider
InternalRetrieve->>ExternalProvider: retrieve(query, refs, ctx)
ExternalProvider-->>InternalRetrieve: RetrievalSourceResult
InternalRetrieve-->>useChatStreamHandlers: records + source_summaries
useChatStreamHandlers-->>User: chat:done with retrieval_summary + SourceReferences footer
ChangesEstimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
backend/app/services/adapters/task_kinds/converters.py (1)
273-307: 🎯 Functional Correctness | 🟠 Major
convert_to_task_dict_optimizedomitsexternal_knowledge_refsThe non-optimized
convert_to_task_dictpopulatesexternal_knowledge_refsfromtask_crd.spec.externalKnowledgeRefs, but the optimized variant skips this entirely. Sincetask_crdis available in this function scope, the optimized path silently drops external knowledge bindings from the task detail response.model_selection = _get_model_selection_labels(task_crd) + external_knowledge_refs = [ + ref.model_dump(exclude_none=True) + for ref in (task_crd.spec.externalKnowledgeRefs or []) + ] + return { "id": task.id, @@ "preserve_executor": preserve_executor, + "external_knowledge_refs": external_knowledge_refs, }🤖 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/adapters/task_kinds/converters.py` around lines 273 - 307, The optimized task conversion path is missing external knowledge bindings, so update convert_to_task_dict_optimized to include external_knowledge_refs from task_crd.spec.externalKnowledgeRefs just like convert_to_task_dict does. Add the field in the returned task dictionary near the other task_crd.spec-derived values, keeping the optimized response shape aligned with the non-optimized converter.backend/app/api/endpoints/internal/rag.py (1)
226-272: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
exclude_if=is not supported by Pydantic 2.5.3 (declared version).
exclude_ifat the field level was introduced in Pydantic 2.12. On 2.5.3 it is treated as an unknownFieldkeyword rather than serialization control, so it won't be applied:source_type/source_id/source_uri/source_nameandsource_summarieswill be emitted asnullinstead of being omitted, and Pydantic may warn about extra schema kwargs. This silently breaks the intended "exclude whenNone" contract that downstream consumers rely on.The same pattern appears in
protected_mediation.py(lines 111-114) and should be addressed together. Either pin/upgrade Pydantic to >=2.12, or implement omission with a supported mechanism (e.g. a@model_serializer/model_dump(exclude_none=...)at the call site).Please verify the resolved Pydantic version in the repo lockfile before deciding.
#!/bin/bash # Confirm the declared/resolved pydantic version. rg -nP 'pydantic' backend/pyproject.toml backend/requirements*.txt 2>/dev/null fd -g 'poetry.lock' -g 'uv.lock' -g 'requirements*.txt' . --exec rg -nP -A1 '^name = "pydantic"|pydantic==' {} # Confirm exclude_if usages introduced rg -nP 'exclude_if' backend/appIn which Pydantic version was the field-level exclude_if parameter added, and how does Pydantic 2.5 handle unknown Field keyword arguments?🤖 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/api/endpoints/internal/rag.py` around lines 226 - 272, The field-level exclude_if usage in InternalRetrieveResponse and MixedRestrictedRetrieveResponse is not supported by the repo’s declared Pydantic version, so these optional fields will not be omitted when None. Fix the serialization contract by either upgrading Pydantic to a version that supports exclude_if and aligning the lockfile, or removing exclude_if and using a supported omission approach such as model_dump(exclude_none=True) or a serializer for these response models. Also update the matching exclude_if usage in protected_mediation.py so the behavior is consistent across the related response classes.frontend/src/features/tasks/components/chat/ContextSelector.tsx (1)
279-291: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRestore the popover test id.
The updated tests query
context-selector-popover, butPopoverContentno longer exposes thatdata-testid, so the new popover placement test will fail.Proposed fix
<PopoverContent + data-testid="context-selector-popover" className={cn(As per coding guidelines, preserve existing
data-testidattributes and add them to new interactive components.🤖 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 `@frontend/src/features/tasks/components/chat/ContextSelector.tsx` around lines 279 - 291, Restore the missing test hook on the popover content so the updated placement test can find it. Add back the existing data-testid on PopoverContent in ContextSelector, preserving the expected context-selector-popover identifier while keeping the current PopoverContent layout and behavior unchanged.Source: Coding guidelines
🧹 Nitpick comments (4)
packages/chat-core/src/task-state/TaskStateMachine.types.ts (1)
50-50: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
| stringwidens the union and discards the literal types.Because
stringsubsumes'rag_retrieval' | 'direct_injection', the union collapses tostring | null, so editors lose autocomplete and the compiler won't narrow on the known modes. If arbitrary strings are genuinely expected, use a branded alias (e.g.RetrievalMode | (string & {})) to keep literal hints; otherwise drop| string.🤖 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 `@packages/chat-core/src/task-state/TaskStateMachine.types.ts` at line 50, The TaskStateMachine.types definition for mode is widening the union and removing the benefit of the known literal modes. Update the mode type in TaskStateMachine.types so the known values in TaskStateMachineMode stay preserved for autocomplete and narrowing: either remove the plain string fallback if only fixed modes are allowed, or replace it with a non-widening pattern such as a branded string alias so rag_retrieval and direct_injection remain visible to the compiler.frontend/src/__tests__/features/knowledge/document/knowledge-document-page.test.tsx (1)
38-50: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a complementary non-mobile routing test.
This suite currently validates only the mobile branch. Add one test with
mockIsMobile = falseto assert desktop/external routing behavior and catch edge regressions.As per coding guidelines, "
**/*.{test,spec}.{ts,tsx,py}: Mock external services in tests, and keep tests independent and isolated while covering edge and error cases."🤖 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 `@frontend/src/__tests__/features/knowledge/document/knowledge-document-page.test.tsx` around lines 38 - 50, The responsive routing test suite only covers the mobile branch in KnowledgeDocumentPage. Add a complementary test in the KnowledgeDocumentPage responsive routing describe block that sets mockIsMobile = false and asserts the desktop/external routing outcome, using the existing KnowledgeDocumentPage render and test IDs to verify the desktop view appears while the mobile view does not. Keep the test isolated by resetting the mock state in the same pattern as the current mobile test.Source: Coding guidelines
backend/app/api/endpoints/internal/knowledge.py (1)
83-133: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valuePer-provider pagination makes
limit/total_returnedambiguous across multiple providers.
limit/offsetare forwarded unchanged to each provider, so whenexternal_knowledge_refsspan N providers the response can contain up toN * limititems, andtotal_returnedis just the returned count rather than a true total. Callers paging through results may get inconsistent windows. If multi-provider listing is expected, consider documenting that pagination is per-provider, or aggregate/cap server-side.🤖 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/api/endpoints/internal/knowledge.py` around lines 83 - 133, The pagination behavior in the knowledge listing flow is per-provider, but the current response shape in the internal knowledge endpoint makes `limit`, `offset`, and `total_returned` look global across all providers. Update the `list_documents` handling in `KnowledgeListDocumentsResponse` to either clearly document that pagination is applied independently per provider, or change the implementation to aggregate results server-side before applying pagination so `total_returned` reflects a true overall count. Keep the fix centered around the loop over `refs_by_provider` and the final response construction.chat_shell/chat_shell/tools/builtin/knowledge_listing.py (1)
606-615: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant manual bounds checks duplicate the Pydantic schema constraints.
KnowledgeListDocumentsInputalready enforcesge=0foroffsetandge=1, le=MAX_KB_LS_LIMITforlimit, so these branches are only reachable via direct_aruncalls bypassing schema validation. If kept for that path, fine; otherwise they're dead validation. Low priority.🤖 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 `@chat_shell/chat_shell/tools/builtin/knowledge_listing.py` around lines 606 - 615, The limit and offset validation in knowledge_listing.py duplicates the constraints already enforced by KnowledgeListDocumentsInput, so these branches are effectively redundant for normal tool usage. Update the _arun path to rely on the existing Pydantic schema validation, and only keep the manual checks if you explicitly need to guard direct internal calls; otherwise remove the duplicate limit/offset error returns from the knowledge list flow.
🤖 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 `@chat_shell/chat_shell/tools/builtin/knowledge_base.py`:
- Around line 1685-1731: The mixed restricted retrieval response is returning
the uncounted top-level retrieval summary, so citation counts stay stale even
when external results were cited. In this branch of the knowledge_base mixed
path, use the corrected summary produced by _format_rag_result /
external_payload instead of the original retrieval_summary when building the
final json.dumps payload. Keep the same keys in the response, but ensure the
top-level retrieval_summary matches the citation-counted version carried by
external_payload.
In `@chat_shell/chat_shell/tools/builtin/knowledge_listing.py`:
- Around line 733-793: The paging in _list_internal_documents is applied per
knowledge base and then concatenated, so the combined results from multiple KBs
do not represent a single coherent page. Update the knowledge listing flow
around _list_internal_documents so pagination is either applied after merging
all internal sources (and any external sources) into one ordered result set, or
explicitly documented as per-source paging; in either case, ensure the returned
total/limit metadata matches the actual paging semantics used by
KnowledgeListDocumentsTool.
- Around line 617-648: The external provider failure is being dropped in the
combined response path of knowledge_listing.py’s document aggregation flow.
Update the logic around _list_external_documents and the final json.dumps
payload so that when external_result contains an error and internal_documents is
non-empty, the error is still surfaced in warnings instead of being ignored;
keep the existing early return only for the no-internal-docs case and ensure
selected_sources/documents output still reflects the partial success.
In `@chat_shell/tests/test_knowledge_base_restricted_backend_mode.py`:
- Line 154: The non-leakage check in the restricted backend test is currently
ineffective because the mocked `_retrieve_with_strategy_from_all_kbs` data never
contains the forbidden text, so the assertion cannot detect a real leak. Update
the mock setup in `test_knowledge_base_restricted_backend_mode` so the
internal-side payload includes a sentinel raw chunk with the exact `"secret raw
content"` string, then keep the `raw_output` assertion to verify that this
internal content is not exposed.
In `@frontend/src/features/knowledge/knowledgeSourceViewRegistry.tsx`:
- Around line 48-50: listKnowledgeSourceViews() is returning the internal
snapshot array by reference, which lets callers mutate registry state directly.
Update the public API in knowledgeSourceViewRegistry so
listKnowledgeSourceViews() returns an immutable copy of snapshot instead of the
original array, and keep snapshot itself as the internal source of truth used by
the registry.
In `@frontend/src/features/tasks/components/chat/DingTalkKnowledgePicker.tsx`:
- Around line 466-488: The DingTalk tree item controls in
DingTalkKnowledgePicker are below the required mobile touch target size. Update
the expander button and the main node toggle button in the tree row so they meet
at least 44px by 44px, adjusting the button sizing/class names around the
ChevronRight expander and the onClick toggle button without changing the tree
behavior.
In `@frontend/src/features/tasks/components/chat/KnowledgeSourcePicker.tsx`:
- Around line 1591-1603: The document row buttons in KnowledgeSourcePicker are
only visually dimmed when supportsDocumentSelection is false, but they are still
interactive. Update the button rendering around the node rows (including the
later matching block) to use a real disabled button state when disabled is true,
and make sure the onClick guard matches that state; keep the existing
selected/disabled styling in sync so keyboard and screen-reader users see the
correct behavior.
In `@frontend/src/features/tasks/components/chat/SourceReferences.tsx`:
- Around line 135-158: The counts in SourceReferences are using a falsy fallback
that can overwrite a legitimate zero when source_statuses exists, causing wrong
ignoredCount and searchedCount values. In SourceReferences.tsx, update the
ignoredCount and searchedCount logic to prefer source status data when statuses
are present, and only fall back to summary?.ignored_source_ids /
summary?.searched_source_ids when detailed statuses are absent. Use the existing
statuses array and the source_statuses-derived branches in SourceReferences to
make the decision based on presence of status entries, not on filter(...).length
truthiness.
---
Outside diff comments:
In `@backend/app/api/endpoints/internal/rag.py`:
- Around line 226-272: The field-level exclude_if usage in
InternalRetrieveResponse and MixedRestrictedRetrieveResponse is not supported by
the repo’s declared Pydantic version, so these optional fields will not be
omitted when None. Fix the serialization contract by either upgrading Pydantic
to a version that supports exclude_if and aligning the lockfile, or removing
exclude_if and using a supported omission approach such as
model_dump(exclude_none=True) or a serializer for these response models. Also
update the matching exclude_if usage in protected_mediation.py so the behavior
is consistent across the related response classes.
In `@backend/app/services/adapters/task_kinds/converters.py`:
- Around line 273-307: The optimized task conversion path is missing external
knowledge bindings, so update convert_to_task_dict_optimized to include
external_knowledge_refs from task_crd.spec.externalKnowledgeRefs just like
convert_to_task_dict does. Add the field in the returned task dictionary near
the other task_crd.spec-derived values, keeping the optimized response shape
aligned with the non-optimized converter.
In `@frontend/src/features/tasks/components/chat/ContextSelector.tsx`:
- Around line 279-291: Restore the missing test hook on the popover content so
the updated placement test can find it. Add back the existing data-testid on
PopoverContent in ContextSelector, preserving the expected
context-selector-popover identifier while keeping the current PopoverContent
layout and behavior unchanged.
---
Nitpick comments:
In `@backend/app/api/endpoints/internal/knowledge.py`:
- Around line 83-133: The pagination behavior in the knowledge listing flow is
per-provider, but the current response shape in the internal knowledge endpoint
makes `limit`, `offset`, and `total_returned` look global across all providers.
Update the `list_documents` handling in `KnowledgeListDocumentsResponse` to
either clearly document that pagination is applied independently per provider,
or change the implementation to aggregate results server-side before applying
pagination so `total_returned` reflects a true overall count. Keep the fix
centered around the loop over `refs_by_provider` and the final response
construction.
In `@chat_shell/chat_shell/tools/builtin/knowledge_listing.py`:
- Around line 606-615: The limit and offset validation in knowledge_listing.py
duplicates the constraints already enforced by KnowledgeListDocumentsInput, so
these branches are effectively redundant for normal tool usage. Update the _arun
path to rely on the existing Pydantic schema validation, and only keep the
manual checks if you explicitly need to guard direct internal calls; otherwise
remove the duplicate limit/offset error returns from the knowledge list flow.
In
`@frontend/src/__tests__/features/knowledge/document/knowledge-document-page.test.tsx`:
- Around line 38-50: The responsive routing test suite only covers the mobile
branch in KnowledgeDocumentPage. Add a complementary test in the
KnowledgeDocumentPage responsive routing describe block that sets mockIsMobile =
false and asserts the desktop/external routing outcome, using the existing
KnowledgeDocumentPage render and test IDs to verify the desktop view appears
while the mobile view does not. Keep the test isolated by resetting the mock
state in the same pattern as the current mobile test.
In `@packages/chat-core/src/task-state/TaskStateMachine.types.ts`:
- Line 50: The TaskStateMachine.types definition for mode is widening the union
and removing the benefit of the known literal modes. Update the mode type in
TaskStateMachine.types so the known values in TaskStateMachineMode stay
preserved for autocomplete and narrowing: either remove the plain string
fallback if only fixed modes are allowed, or replace it with a non-widening
pattern such as a branded string alias so rag_retrieval and direct_injection
remain visible to the compiler.
🪄 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: 5f24b653-6004-453b-9957-a617518c295d
📒 Files selected for processing (96)
AGENTS.mdbackend/app/api/api.pybackend/app/api/endpoints/internal/__init__.pybackend/app/api/endpoints/internal/knowledge.pybackend/app/api/endpoints/internal/rag.pybackend/app/api/ws/events.pybackend/app/schemas/context_display.pybackend/app/schemas/external_knowledge.pybackend/app/schemas/kind.pybackend/app/schemas/shared_task.pybackend/app/schemas/subtask.pybackend/app/schemas/subtask_context.pybackend/app/schemas/task.pybackend/app/services/adapters/task_kinds/converters.pybackend/app/services/adapters/task_kinds/task_detail_helpers.pybackend/app/services/chat/external_knowledge_refs.pybackend/app/services/chat/preprocessing/contexts.pybackend/app/services/chat/trigger/unified.pybackend/app/services/execution/dispatcher.pybackend/app/services/knowledge/knowledge_service.pybackend/app/services/knowledge/protected_mediation.pybackend/app/services/knowledge/task_knowledge_base_service.pybackend/app/services/rag/sources/__init__.pybackend/app/services/rag/sources/models.pybackend/app/services/rag/sources/registry.pybackend/tests/api/endpoints/internal/test_rag_retrieve_endpoint.pybackend/tests/services/knowledge/test_namespace_knowledge_permissions.pybackend/tests/services/test_context_service.pybackend/tests/services/test_external_context_sync.pybackend/tests/services/test_task_detail_bot_lookup_cache.pybackend/tests/services/test_task_knowledge_base_sync.pychat_shell/chat_shell/services/context.pychat_shell/chat_shell/services/streaming/core.pychat_shell/chat_shell/tools/builtin/__init__.pychat_shell/chat_shell/tools/builtin/knowledge_base.pychat_shell/chat_shell/tools/builtin/knowledge_listing.pychat_shell/chat_shell/tools/events.pychat_shell/chat_shell/tools/knowledge_factory.pychat_shell/tests/test_kb_prompt_deduplication.pychat_shell/tests/test_knowledge_base_restricted_backend_mode.pychat_shell/tests/test_knowledge_listing.pychat_shell/tests/test_tool_events.pydocs/en/developer-guide/external-knowledge-sources.mddocs/zh/developer-guide/external-knowledge-sources.mdfrontend/src/__tests__/contexts/SocketContext.reconnect.test.tsxfrontend/src/__tests__/features/knowledge/document/knowledge-document-page.test.tsxfrontend/src/__tests__/features/knowledge/external-knowledge-pagination.test.tsfrontend/src/__tests__/features/knowledge/external-knowledge-source-registry.test.tsxfrontend/src/__tests__/features/tasks/components/chat/ChatContextInput.test.tsxfrontend/src/__tests__/features/tasks/components/chat/KnowledgeSourcePicker.external-knowledge.test.tsxfrontend/src/__tests__/features/tasks/components/chat/context-selector.test.tsxfrontend/src/__tests__/features/tasks/components/chat/useChatStreamHandlers.external-knowledge.test.tsxfrontend/src/__tests__/features/tasks/components/mobile-toggle-controls.test.tsxfrontend/src/apis/tasks.tsfrontend/src/app/shared/task/page.tsxfrontend/src/features/knowledge/document/components/KnowledgeDocumentPageDesktop.tsxfrontend/src/features/knowledge/document/components/KnowledgeSidebar/NavigationSection.tsxfrontend/src/features/knowledge/document/components/KnowledgeSidebar/index.tsxfrontend/src/features/knowledge/document/hooks/useKnowledgeSidebar.tsfrontend/src/features/knowledge/document/hooks/useKnowledgeUrlSync.tsfrontend/src/features/knowledge/externalKnowledgePagination.tsfrontend/src/features/knowledge/externalKnowledgeSourceRegistry.tsfrontend/src/features/knowledge/knowledgeSourceViewRegistry.tsxfrontend/src/features/tasks/components/MobileCorrectionModeToggle.tsxfrontend/src/features/tasks/components/chat/ChatArea.tsxfrontend/src/features/tasks/components/chat/ChatContextInput.tsxfrontend/src/features/tasks/components/chat/ContextBadge.tsxfrontend/src/features/tasks/components/chat/ContextSelector.tsxfrontend/src/features/tasks/components/chat/DingTalkDocContextSelector.tsxfrontend/src/features/tasks/components/chat/DingTalkKnowledgePicker.tsxfrontend/src/features/tasks/components/chat/KnowledgeSourcePicker.tsxfrontend/src/features/tasks/components/chat/SourceReferences.tsxfrontend/src/features/tasks/components/chat/useChatAreaState.tsfrontend/src/features/tasks/components/chat/useChatStreamHandlers.tsxfrontend/src/features/tasks/components/clarification/MobileClarificationToggle.tsxfrontend/src/features/tasks/components/input/ChatInputControls.tsxfrontend/src/features/tasks/components/input/MobileChatInputControls.tsxfrontend/src/features/tasks/components/message/ContextBadgeList.tsxfrontend/src/features/tasks/components/message/MessageBubble.tsxfrontend/src/features/tasks/components/mobile-switch-indicator.tsxfrontend/src/features/tasks/presentation/useMessagePresenter.tsfrontend/src/features/tasks/session/messageSyncer.tsxfrontend/src/i18n/locales/en/chat.jsonfrontend/src/i18n/locales/en/knowledge.jsonfrontend/src/i18n/locales/zh-CN/chat.jsonfrontend/src/i18n/locales/zh-CN/knowledge.jsonfrontend/src/types/api.tsfrontend/src/types/context.tsfrontend/src/types/external-knowledge.tsfrontend/src/types/socket.tspackages/chat-core/src/task-state/TaskStateMachine.types.tsshared/models/db/enums.pyshared/models/execution.pyshared/models/openai_converter.pyshared/prompts/knowledge_base.pyshared/tests/test_prompts.py
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
chat_shell/chat_shell/tools/builtin/knowledge_base.py (2)
898-1124: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy liftExtract this retrieval-summary/provenance subsystem out of
KnowledgeBaseTool.This PR adds another large, provider-neutral normalization/merging layer into an already 1800+ line tool that also owns transport, routing, mediation, formatting, and call accounting. Moving these helpers into a dedicated module will make the contracts easier to test and change safely.
As per coding guidelines, "Prefer high cohesion and low coupling; each module or class should have a single responsibility." and "If a file exceeds 1000 lines, split it into multiple sub-modules."
🤖 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 `@chat_shell/chat_shell/tools/builtin/knowledge_base.py` around lines 898 - 1124, The retrieval-summary and provenance normalization logic inside KnowledgeBaseTool is too large and should be moved into a dedicated module with a narrower responsibility. Extract the helper methods (_normalize_source_status, _build_internal_source_statuses, _build_retrieval_summary, _source_reference_status_key, and _with_citation_counts) into a separate retrieval/provenance utility, then update KnowledgeBaseTool to call that module instead of owning the merging and normalization directly. Keep the same behavior and symbols where possible so the existing retrieval flow still works, but reduce coupling by isolating these provider-neutral summary helpers.Source: Coding guidelines
696-704: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winTruncate the query before writing this info log.
This path logs the full user search text, so secrets pasted into a knowledge query will end up in normal service logs. The rest of this tool already uses a truncated pattern, so this should follow it too.
Suggested fix
logger.info( f"[KnowledgeBaseTool] Searching {len(self.knowledge_base_ids)} knowledge bases " - f"and {len(self.external_knowledge_refs)} external refs with query: {query}" + f"and {len(self.external_knowledge_refs)} external refs with query: {query[:50]}" + ( f", filtering by {len(effective_document_ids)} documents" if effective_document_ids else "" ) )Based on learnings, the established pattern is to log only truncated query text such as
query[:50].🤖 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 `@chat_shell/chat_shell/tools/builtin/knowledge_base.py` around lines 696 - 704, The info log in KnowledgeBaseTool is writing the full user query, so update the logger.info call in the search path to use the same truncated pattern as the rest of the tool, such as logging only query[:50]. Keep the existing context about knowledge base counts and document filtering, but replace the raw query reference with a shortened version so sensitive input is not emitted to normal service logs.Source: Learnings
🧹 Nitpick comments (1)
chat_shell/tests/test_knowledge_listing.py (1)
319-357: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the zero-document source contract in this failure test.
This path is where the tool should still keep failed external selections in
selected_sourceswithdocument_count == 0, but the test only checks warnings and counts. A regression that drops empty selected sources would still pass here.Suggested assertion
assert result["warnings"] == [ { "type": "external_listing_failed", "message": "Failed to list knowledge documents", "status_code": 503, } ] + assert any( + source["provider"] == "demo" + and source["source_id"] == "demo-kb-1" + and source["document_count"] == 0 + and source["documents"] == [] + for source in result["selected_sources"] + )As per coding guidelines, "Test edge cases and error conditions, and keep tests independent and isolated."
🤖 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 `@chat_shell/tests/test_knowledge_listing.py` around lines 319 - 357, The failure-path test for KnowledgeListDocumentsTool::_arun only asserts warnings and returned counts, so it can miss a regression where empty external sources are dropped. Update test_surfaces_external_listing_error_when_internal_documents_exist to also verify result["selected_sources"] retains the failed external selection with document_count equal to 0 (alongside the existing internal success), using the existing KnowledgeListDocumentsTool and _arun setup.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.
Outside diff comments:
In `@chat_shell/chat_shell/tools/builtin/knowledge_base.py`:
- Around line 898-1124: The retrieval-summary and provenance normalization logic
inside KnowledgeBaseTool is too large and should be moved into a dedicated
module with a narrower responsibility. Extract the helper methods
(_normalize_source_status, _build_internal_source_statuses,
_build_retrieval_summary, _source_reference_status_key, and
_with_citation_counts) into a separate retrieval/provenance utility, then update
KnowledgeBaseTool to call that module instead of owning the merging and
normalization directly. Keep the same behavior and symbols where possible so the
existing retrieval flow still works, but reduce coupling by isolating these
provider-neutral summary helpers.
- Around line 696-704: The info log in KnowledgeBaseTool is writing the full
user query, so update the logger.info call in the search path to use the same
truncated pattern as the rest of the tool, such as logging only query[:50]. Keep
the existing context about knowledge base counts and document filtering, but
replace the raw query reference with a shortened version so sensitive input is
not emitted to normal service logs.
---
Nitpick comments:
In `@chat_shell/tests/test_knowledge_listing.py`:
- Around line 319-357: The failure-path test for
KnowledgeListDocumentsTool::_arun only asserts warnings and returned counts, so
it can miss a regression where empty external sources are dropped. Update
test_surfaces_external_listing_error_when_internal_documents_exist to also
verify result["selected_sources"] retains the failed external selection with
document_count equal to 0 (alongside the existing internal success), using the
existing KnowledgeListDocumentsTool and _arun setup.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 94e2db92-7d26-4961-9f48-9ce41551fd5b
📒 Files selected for processing (19)
backend/app/api/endpoints/internal/knowledge.pybackend/app/services/adapters/task_kinds/converters.pybackend/tests/api/endpoints/internal/test_rag_retrieve_endpoint.pybackend/tests/services/adapters/test_task_detail_converter_project_id.pychat_shell/chat_shell/tools/builtin/knowledge_base.pychat_shell/chat_shell/tools/builtin/knowledge_listing.pychat_shell/tests/test_knowledge_base_restricted_backend_mode.pychat_shell/tests/test_knowledge_listing.pydocs/en/developer-guide/external-knowledge-sources.mddocs/zh/developer-guide/external-knowledge-sources.mdfrontend/src/__tests__/features/knowledge/document/knowledge-document-page.test.tsxfrontend/src/__tests__/features/knowledge/external-knowledge-source-registry.test.tsxfrontend/src/__tests__/features/knowledge/knowledge-source-view-registry.test.tsxfrontend/src/__tests__/features/tasks/components/chat/SourceReferences.test.tsxfrontend/src/features/knowledge/knowledgeSourceViewRegistry.tsxfrontend/src/features/tasks/components/chat/ContextSelector.tsxfrontend/src/features/tasks/components/chat/DingTalkKnowledgePicker.tsxfrontend/src/features/tasks/components/chat/KnowledgeSourcePicker.tsxfrontend/src/features/tasks/components/chat/SourceReferences.tsx
✅ Files skipped from review due to trivial changes (3)
- backend/app/services/adapters/task_kinds/converters.py
- docs/en/developer-guide/external-knowledge-sources.md
- docs/zh/developer-guide/external-knowledge-sources.md
🚧 Files skipped from review as they are similar to previous changes (9)
- chat_shell/tests/test_knowledge_base_restricted_backend_mode.py
- backend/app/api/endpoints/internal/knowledge.py
- frontend/src/features/tasks/components/chat/ContextSelector.tsx
- frontend/src/features/knowledge/knowledgeSourceViewRegistry.tsx
- backend/tests/api/endpoints/internal/test_rag_retrieve_endpoint.py
- frontend/src/features/tasks/components/chat/DingTalkKnowledgePicker.tsx
- chat_shell/chat_shell/tools/builtin/knowledge_listing.py
- frontend/src/features/tasks/components/chat/SourceReferences.tsx
- frontend/src/features/tasks/components/chat/KnowledgeSourcePicker.tsx
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (4)
chat_shell/tests/test_knowledge_listing.py (1)
321-360: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert that failed external sources still remain in
selected_sources.This case currently verifies the warning but not the core merged-listing contract. If provider failure drops the seeded external source, the tool will silently stop telling the model which source was selected but returned zero documents.
Suggested assertion
assert result["warnings"] == [ { "type": "external_listing_failed", "message": "Failed to list knowledge documents", "status_code": 503, } ] + assert any( + source["provider"] == "demo" + and source["source_id"] == "demo-kb-1" + and source["document_count"] == 0 + for source in result["selected_sources"] + )🤖 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 `@chat_shell/tests/test_knowledge_listing.py` around lines 321 - 360, The partial-listing test in KnowledgeListDocumentsTool should also verify that a failed external source is still preserved in selected_sources, not just reported in warnings. Update test_surfaces_external_listing_error_when_internal_documents_exist to assert the merged result includes the seeded external reference from external_knowledge_refs alongside the internal knowledge_base_ids source, so provider failure does not drop the selected source metadata.backend/tests/services/test_external_context_sync.py (1)
227-278: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAdd coverage for removing the last bound ref.
This test only exercises the branch where another entry remains.
remove_task_external_knowledge_ref()also has a separate persistence path that deletesexternalKnowledgeRefsentirely when the final binding is removed, and a regression there would leave stale task-level bindings behind for future messages.🤖 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/test_external_context_sync.py` around lines 227 - 278, The current test for remove_task_external_knowledge_ref only covers the case where one matching binding is removed and another externalKnowledgeRefs entry remains; add a test for the final-binding case. Extend backend/tests/services/test_external_context_sync.py to exercise remove_task_external_knowledge_ref with a TaskResource whose spec.externalKnowledgeRefs contains only the matching ref, then assert the function removes the entire externalKnowledgeRefs field via the persistence path in task_store.update_json and commits the db. Use the existing remove_task_external_knowledge_ref and task_store.update_json symbols to locate the behavior.frontend/src/features/tasks/components/group-chat/TaskKnowledgeBindingPanel.tsx (2)
126-148: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winFetch internal and external bindings in parallel.
These calls are independent, but the external request currently waits for the internal one to finish. That adds an extra round-trip to panel open time and is also why
loadingbecomesfalsebefore external loading is done.Suggested refactor
const fetchBindings = useCallback(async () => { setLoading(true) setError(null) setExternalError(null) - try { - const internalResponse = await taskKnowledgeBaseApi.getBoundKnowledgeBases(taskId) + const [internalResult, externalResult] = await Promise.allSettled([ + taskKnowledgeBaseApi.getBoundKnowledgeBases(taskId), + taskKnowledgeBaseApi.getBoundExternalKnowledgeRefs(taskId), + ]) + + try { + if (internalResult.status !== 'fulfilled') { + throw internalResult.reason + } + const internalResponse = internalResult.value setInternalKnowledgeBases(internalResponse.items) setMaxInternalLimit(internalResponse.max_limit) } catch (err) { console.error('Failed to fetch internal task knowledge bindings:', err) setError(t('knowledgeBinding.loadFailed')) - } finally { - setLoading(false) } try { - const externalResponse = await taskKnowledgeBaseApi.getBoundExternalKnowledgeRefs(taskId) + if (externalResult.status !== 'fulfilled') { + throw externalResult.reason + } + const externalResponse = externalResult.value setExternalRefs(externalResponse.items) } catch (err) { console.warn('Failed to fetch external task knowledge bindings:', err) setExternalRefs([]) setExternalError(t('knowledgeBinding.externalLoadFailed')) } + setLoading(false) }, [taskId, t])🤖 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 `@frontend/src/features/tasks/components/group-chat/TaskKnowledgeBindingPanel.tsx` around lines 126 - 148, The fetchBindings callback in TaskKnowledgeBindingPanel currently runs getBoundKnowledgeBases and getBoundExternalKnowledgeRefs sequentially, which delays panel load and clears loading too early. Refactor the fetch logic to start both requests together (for example by kicking off both API calls before awaiting either, and handling each result/error independently), while keeping the existing setError, setExternalError, setInternalKnowledgeBases, and setExternalRefs behavior intact.
84-87: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRename this component file to kebab-case.
The component name is fine, but the new file path
TaskKnowledgeBindingPanel.tsxbreaks the frontend file-naming rule. Renaming it now avoids spreading the non-standard import path further.As per coding guidelines, "Frontend component names must use PascalCase, and component files must use kebab-case naming."
🤖 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 `@frontend/src/features/tasks/components/group-chat/TaskKnowledgeBindingPanel.tsx` around lines 84 - 87, The component name TaskKnowledgeBindingPanel is fine, but the file name violates the frontend kebab-case file naming rule. Rename the component file to a kebab-case path and update any imports that reference the old filename so the new standard is used consistently without changing the PascalCase component export.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 `@chat_shell/tests/test_knowledge_listing.py`:
- Around line 399-409: The fixture in
test_provider_source_name_overrides_document_ref_seed is using the old
document-ref shape, so update the KnowledgeListDocumentsTool seed to the
normalized contract where name is the knowledge-source label and the document
label lives in target_name. Keep the assertions focused on
KnowledgeListDocumentsTool and its handling of external_knowledge_refs so the
test covers zero-document and error paths instead of only passing when the
provider response overwrites the bad seed.
In `@docs/en/user-guide/chat/managing-tasks.md`:
- Around line 30-41: The conversation definition is inconsistent with the
component table: the formula still includes Conversation History while the table
now uses Status. Update the formula near the Conversation Components section so
it matches the table, and make sure the fenced block is labeled as text in that
Markdown snippet to keep markdownlint satisfied. Use the existing Conversation
Components table and formula text as the symbols to locate the affected content.
In `@docs/zh/user-guide/chat/managing-tasks.md`:
- Around line 30-41: The “对话” definition is inconsistent between the formula and
the table: the formula still uses “对话历史” while the table uses “状态”, and the code
block should be marked as text to avoid markdownlint warnings. Update the
definition in managing-tasks.md so the formula matches the table’s fourth
component, and adjust the code fence in the same section to use an explicit text
language label; use the existing “对话 / 对话的组成要素 / 状态” symbols to keep the
terminology aligned.
- Around line 279-288: The troubleshooting section in the Chinese chat task
guide is missing the “code repository configuration error” cause that still has
a corresponding resolution step, so the Chinese and English versions are
inconsistent. Update the “可能原因” list in the affected chat task troubleshooting
section to match the English版内容, and keep the existing “验证代码仓库访问权限” step aligned
with that cause; use the surrounding “智能体不可用 / 系统资源受限” bullets to locate the
section.
In
`@frontend/src/features/tasks/components/group-chat/TaskKnowledgeBindingPanel.tsx`:
- Around line 300-307: The remove button in TaskKnowledgeBindingPanel is too
small for the mobile touch target requirement because its current sizing makes
it 32×32; update the Button styling for the item delete action in the
TaskKnowledgeBindingPanel render so it meets the 44×44 minimum, using the repo’s
preferred mobile sizing pattern such as h-11 and a minimum width of 44px while
preserving the existing icon button behavior and handlers like handleRemove and
the aria/testid attributes.
In `@frontend/src/i18n/locales/en/chat.json`:
- Around line 417-426: The mixed binding panel still uses
knowledge-base-specific wording for labels that now cover both internal and
external sources. Update the locale entries in the knowledgeBinding section so
the values referenced by title, empty, loadFailed, and removeFailed use
provider-neutral language, while keeping the existing externalKnowledge and
externalLoadFailed wording aligned with the combined view.
In `@frontend/src/i18n/locales/zh-CN/chat.json`:
- Around line 417-426: The knowledgeBinding strings in chat.json still use “知识库”
in generic labels, which is inconsistent when the panel manages both internal
and external sources; update the affected entries in knowledgeBinding
(especially title, empty, loadFailed, and removeFailed) to a more neutral
“知识/知识来源” wording, while keeping the existing externalKnowledge and
externalLoadFailed intent aligned with the new terminology.
---
Nitpick comments:
In `@backend/tests/services/test_external_context_sync.py`:
- Around line 227-278: The current test for remove_task_external_knowledge_ref
only covers the case where one matching binding is removed and another
externalKnowledgeRefs entry remains; add a test for the final-binding case.
Extend backend/tests/services/test_external_context_sync.py to exercise
remove_task_external_knowledge_ref with a TaskResource whose
spec.externalKnowledgeRefs contains only the matching ref, then assert the
function removes the entire externalKnowledgeRefs field via the persistence path
in task_store.update_json and commits the db. Use the existing
remove_task_external_knowledge_ref and task_store.update_json symbols to locate
the behavior.
In `@chat_shell/tests/test_knowledge_listing.py`:
- Around line 321-360: The partial-listing test in KnowledgeListDocumentsTool
should also verify that a failed external source is still preserved in
selected_sources, not just reported in warnings. Update
test_surfaces_external_listing_error_when_internal_documents_exist to assert the
merged result includes the seeded external reference from
external_knowledge_refs alongside the internal knowledge_base_ids source, so
provider failure does not drop the selected source metadata.
In
`@frontend/src/features/tasks/components/group-chat/TaskKnowledgeBindingPanel.tsx`:
- Around line 126-148: The fetchBindings callback in TaskKnowledgeBindingPanel
currently runs getBoundKnowledgeBases and getBoundExternalKnowledgeRefs
sequentially, which delays panel load and clears loading too early. Refactor the
fetch logic to start both requests together (for example by kicking off both API
calls before awaiting either, and handling each result/error independently),
while keeping the existing setError, setExternalError,
setInternalKnowledgeBases, and setExternalRefs behavior intact.
- Around line 84-87: The component name TaskKnowledgeBindingPanel is fine, but
the file name violates the frontend kebab-case file naming rule. Rename the
component file to a kebab-case path and update any imports that reference the
old filename so the new standard is used consistently without changing the
PascalCase component export.
🪄 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: 32248626-abff-416d-aa54-6b273a76ef09
📒 Files selected for processing (28)
backend/app/api/endpoints/adapter/task_knowledge_bases.pybackend/app/schemas/external_knowledge.pybackend/app/services/chat/external_knowledge_refs.pybackend/app/services/chat/preprocessing/contexts.pybackend/tests/services/test_external_context_sync.pychat_shell/chat_shell/tools/builtin/knowledge_listing.pychat_shell/tests/test_knowledge_listing.pydocs/en/developer-guide/external-knowledge-sources.mddocs/en/reference/yaml-specification.mddocs/en/user-guide/chat/managing-tasks.mddocs/zh/developer-guide/external-knowledge-sources.mddocs/zh/reference/yaml-specification.mddocs/zh/user-guide/chat/managing-tasks.mdfrontend/src/__tests__/features/knowledge/external-knowledge-source-registry.test.tsxfrontend/src/__tests__/features/tasks/components/group-chat/TaskKnowledgeBindingPanel.test.tsxfrontend/src/apis/task-knowledge-base.tsfrontend/src/features/knowledge/externalKnowledgeSourceRegistry.tsfrontend/src/features/tasks/components/chat/KnowledgeSourcePicker.tsxfrontend/src/features/tasks/components/chat/useChatStreamHandlers.tsxfrontend/src/features/tasks/components/group-chat/BindKnowledgeBaseDialog.tsxfrontend/src/features/tasks/components/group-chat/TaskKnowledgeBasePanel.tsxfrontend/src/features/tasks/components/group-chat/TaskKnowledgeBindingPanel.tsxfrontend/src/features/tasks/components/group-chat/TaskMembersPanel.tsxfrontend/src/features/tasks/components/group-chat/index.tsfrontend/src/i18n/locales/en/chat.jsonfrontend/src/i18n/locales/zh-CN/chat.jsonfrontend/src/types/context.tsfrontend/src/types/task-knowledge-base.ts
💤 Files with no reviewable changes (1)
- frontend/src/features/tasks/components/group-chat/TaskKnowledgeBasePanel.tsx
✅ Files skipped from review due to trivial changes (3)
- frontend/src/types/task-knowledge-base.ts
- frontend/src/features/tasks/components/group-chat/BindKnowledgeBaseDialog.tsx
- docs/en/developer-guide/external-knowledge-sources.md
🚧 Files skipped from review as they are similar to previous changes (7)
- backend/app/schemas/external_knowledge.py
- frontend/src/types/context.ts
- frontend/src/features/knowledge/externalKnowledgeSourceRegistry.ts
- chat_shell/chat_shell/tools/builtin/knowledge_listing.py
- frontend/src/features/tasks/components/chat/KnowledgeSourcePicker.tsx
- frontend/src/features/tasks/components/chat/useChatStreamHandlers.tsx
- backend/app/services/chat/preprocessing/contexts.py
Collaborator
|
LGTM |
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
frontend/src/features/tasks/components/group-chat/task-knowledge-binding-panel.tsx (2)
207-217: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDon't block external binding management on internal load failure.
fetchBindings()now loads internal and external bindings independently, but this early return hides successfully loadedexternalRefswhenever the internal request fails. In that partial-failure path, users can't view or remove external bindings even though that data is available. Render the internal error inline and keep the list visible instead.🤖 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 `@frontend/src/features/tasks/components/group-chat/task-knowledge-binding-panel.tsx` around lines 207 - 217, The error-only early return in task-knowledge-binding-panel’s binding render path is hiding successfully loaded external bindings when the internal fetch fails. Update the component logic around fetchBindings and the error state so the internal failure is shown inline while still rendering the externalRefs list and its controls, instead of returning the empty error panel from this branch.
212-214: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd stable
data-testidattributes to the new action buttons.Line 212 and Line 336 introduce new buttons, but unlike the add/remove controls they don't expose stable selectors for tests. As per coding guidelines, "Preserve existing
data-testidattributes, and add descriptivedata-testidattributes to new interactive elements such as buttons, inputs, links, and selects."Also applies to: 335-338
🤖 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 `@frontend/src/features/tasks/components/group-chat/task-knowledge-binding-panel.tsx` around lines 212 - 214, The new action buttons in TaskKnowledgeBindingPanel are missing stable test selectors, so add descriptive data-testid attributes to the retry button that calls fetchBindings and to the other new action button(s) around the add/remove controls. Keep existing data-testid values intact and update the Button elements in task-knowledge-binding-panel.tsx so tests can reliably target these interactive elements.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.
Outside diff comments:
In
`@frontend/src/features/tasks/components/group-chat/task-knowledge-binding-panel.tsx`:
- Around line 207-217: The error-only early return in
task-knowledge-binding-panel’s binding render path is hiding successfully loaded
external bindings when the internal fetch fails. Update the component logic
around fetchBindings and the error state so the internal failure is shown inline
while still rendering the externalRefs list and its controls, instead of
returning the empty error panel from this branch.
- Around line 212-214: The new action buttons in TaskKnowledgeBindingPanel are
missing stable test selectors, so add descriptive data-testid attributes to the
retry button that calls fetchBindings and to the other new action button(s)
around the add/remove controls. Keep existing data-testid values intact and
update the Button elements in task-knowledge-binding-panel.tsx so tests can
reliably target these interactive elements.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 960fe249-69e5-4eb4-aab2-ffb8fae22d8d
📒 Files selected for processing (10)
backend/tests/services/test_external_context_sync.pychat_shell/tests/test_knowledge_listing.pydocs/en/user-guide/chat/managing-tasks.mddocs/zh/user-guide/chat/managing-tasks.mdfrontend/src/__tests__/features/tasks/components/group-chat/task-knowledge-binding-panel.test.tsxfrontend/src/features/tasks/components/group-chat/TaskMembersPanel.tsxfrontend/src/features/tasks/components/group-chat/index.tsfrontend/src/features/tasks/components/group-chat/task-knowledge-binding-panel.tsxfrontend/src/i18n/locales/en/chat.jsonfrontend/src/i18n/locales/zh-CN/chat.json
✅ Files skipped from review due to trivial changes (2)
- frontend/src/i18n/locales/en/chat.json
- frontend/src/i18n/locales/zh-CN/chat.json
🚧 Files skipped from review as they are similar to previous changes (6)
- frontend/src/features/tasks/components/group-chat/index.ts
- frontend/src/features/tasks/components/group-chat/TaskMembersPanel.tsx
- docs/zh/user-guide/chat/managing-tasks.md
- chat_shell/tests/test_knowledge_listing.py
- backend/tests/services/test_external_context_sync.py
- docs/en/user-guide/chat/managing-tasks.md
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
Scope
Non-goals / Not included
Testing
cd backend && uv run pytest tests/services/test_context_service.pypnpm --dir frontend exec next buildpnpm --dir frontend test -- SocketContext.reconnectpnpm --filter wecode-ai-assistant run lintpnpm --filter wecode-ai-assistant exec tsc --noEmitpnpm --filter wecode-ai-assistant run test --passWithNoTestspnpm --filter @wegent/chat-core testpnpm --filter @wegent/chat-core buildAudit
git diff --checkpassed.Follow-up
Summary by CodeRabbit