feat: Add multi‑channel chat support (Slack, Feishu) with UI and backend#15865
feat: Add multi‑channel chat support (Slack, Feishu) with UI and backend#15865zaviermeekz-cpu wants to merge 6 commits into
Conversation
- Add Channel abstract base class and registry - Implement Slack channel (Socket Mode) and Feishu channel (webhook) - Add ChatChannel database model - Add bootstrap reconcile loop for starting/stopping channels - Add REST API endpoints for channel management - Add frontend Chat Channels settings page - Update navigation and i18n Closes infiniflow#15844 This is a draft PR - backend integration requires testing in a working Docker environment.
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds channel message contracts and registry, a reconcile runtime with server hook, Feishu and Slack adapters, channel CRUD REST API, frontend service + settings UI and i18n, and service configuration template updates. ChangesChat channel management
Sequence DiagramsequenceDiagram
participant User as Slack/Feishu User
participant WebhookEndpoint as POST /channels/{id}/webhook
participant ChannelInstance as Running Channel
participant Dialog as Assistant/Dialog
participant ProviderAPI as Provider HTTP API
User->>WebhookEndpoint: Sends message to bot
WebhookEndpoint->>ChannelInstance: payload -> handle_webhook()
ChannelInstance->>ChannelInstance: parse IncomingMessage
ChannelInstance->>Dialog: _dispatch(incoming) with dialog_id
Dialog-->>ChannelInstance: completion chunks (answer, session_id)
ChannelInstance->>ChannelInstance: update session_id, build OutgoingMessage
ChannelInstance->>ProviderAPI: send reply via provider API
ProviderAPI-->>User: delivered reply
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
|
@KevinHuSh @xugangqiang This PR is ready for review. Thank you for your time and for maintaining such a great project! I’m happy to make any changes or improvements you suggest. Please let me know what you think. |
There was a problem hiding this comment.
Actionable comments posted: 4
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
api/apps/services/provider_api_service.py (1)
382-503:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd explicit logs for the newly introduced verification branches.
Line 382 onward introduces new control paths (VLLM base URL normalization effects, BaiduYiyan key coercion, OCR verification). Failures are logged, but branch decisions/success-path observability is still missing, which makes production troubleshooting harder.
As per coding guidelines, “
**/*.py: Add logging for new flows”.🤖 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 `@api/apps/services/provider_api_service.py` around lines 382 - 503, The new verification flow lacks informative info-level logs: add logging calls after base_url is set (via _normalize_provider_base_url) and when target_factory_name is chosen, log when factory_llms is built from FACTORY_LLM_INFOS or from model_info, and log BaiduYiyan API key coercion (showing masked/summary api_key_str) inside the BaiduYiyan branch; also add info/debug logs on each verification branch success/failure (embedding via EmbeddingModel.encode, chat via ChatModel.async_chat_streamly, rerank via RerankModel.similarity, ocr via OcrModel.check_available) indicating provider_name, llm['llm_name'], model_type and result so callers can observe which path passed or failed before returning. Ensure logs avoid printing secrets (mask api keys) and use existing logger variable (e.g., logging) and messages consistent with surrounding logs.Source: Coding guidelines
internal/entity/models/model.go (1)
516-523:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard optional
MaxTokensbefore dereferencing.
Model.MaxTokensis now nullable, but this branch still dereferences it unconditionally and then formats the pointer itself in the error message. Amax_tokensfilter against a model without that field will panic instead of returning a 4xx response.Suggested fix
case "max_tokens": if maxVal, ok := filterValue.(int); ok { - if *model.MaxTokens < maxVal { + if model.MaxTokens == nil { + matchFilter = false + resp.Code = 400 + resp.Message = "Model does not define max_tokens" + break + } + if *model.MaxTokens < maxVal { matchFilter = false resp.Code = 400 resp.Message = fmt.Sprintf("Model does not meet filter criteria: max_tokens (%d) < %d", - model.MaxTokens, maxVal) + *model.MaxTokens, maxVal) } }🤖 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 `@internal/entity/models/model.go` around lines 516 - 523, In the "max_tokens" filter branch, avoid unguarded dereference of the nullable Model.MaxTokens pointer: first assert filterValue is an int, then check if model.MaxTokens == nil and if so set matchFilter = false and resp.Code/resp.Message to a 4xx error indicating the model has no max_tokens (do not dereference); only when model.MaxTokens != nil perform the comparison using *model.MaxTokens and format the error message with the dereferenced value. Ensure you reference the "case \"max_tokens\"" branch, the model.MaxTokens field, matchFilter, resp.Code and resp.Message when applying this guard and updating messages.internal/entity/models/deepinfra.go (1)
843-867:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRestore credential validation in
ListModels.
CheckConnectionon Line 954 delegates to this method, but this implementation skipsAPIConfigCheckand never sends the Bearer token. That makes the health check unauthenticated: a public/modelsendpoint can incorrectly accept missing/invalid keys, and an authenticated one will fail even with a valid key.Suggested fix
func (d *DeepInfraModel) ListModels(apiConfig *APIConfig) ([]ListModelResponse, error) { + if err := d.baseModel.APIConfigCheck(apiConfig); err != nil { + return nil, err + } resolvedBaseURL, err := d.baseModel.GetBaseURL(apiConfig) if err != nil { return nil, err @@ - req, err := http.NewRequestWithContext(ctx, "GET", url, bytes.NewBuffer(jsonData)) + req, err := http.NewRequestWithContext(ctx, "GET", url, nil) if err != nil { return nil, fmt.Errorf("failed to create request: %w", err) } req.Header.Set("Content-Type", "application/json") + req.Header.Set("Authorization", fmt.Sprintf("Bearer %s", *apiConfig.ApiKey))🤖 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 `@internal/entity/models/deepinfra.go` around lines 843 - 867, The ListModels method in DeepInfraModel skips APIConfig validation and never sets the Authorization header, causing unauthenticated or failed health checks; restore the APIConfigCheck call (use the existing APIConfigCheck / d.baseModel.APIConfigCheck utility used elsewhere) at the start of ListModels to validate apiConfig and extract the token, and set the Bearer token on the http.Request (Authorization: Bearer <token>) before sending; also remove or avoid sending an unnecessary JSON body for the GET (use nil body) so the request matches the service expectation and CheckConnection (which calls ListModels) performs an authenticated health check.internal/entity/models/jina.go (1)
321-335:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAuthenticate
ListModelsthe same way as the other Jina calls.
CheckConnectionon Line 372 uses this method, butListModelsno longer validatesapiConfigand never sends the Bearer token. That disconnects verification from the supplied credentials, so a public/modelsendpoint can falsely pass bad keys and an authenticated one will fail even when chat/embedding requests succeed.Suggested fix
func (j *JinaModel) ListModels(apiConfig *APIConfig) ([]ListModelResponse, error) { + if err := j.baseModel.APIConfigCheck(apiConfig); err != nil { + return nil, err + } resolvedBaseURL, err := j.baseModel.GetBaseURL(apiConfig) if err != nil { return nil, err @@ req.Header.Set("Content-Type", "application/json") + req.Header.Set("Authorization", fmt.Sprintf("Bearer %s", *apiConfig.ApiKey))🤖 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 `@internal/entity/models/jina.go` around lines 321 - 335, The ListModels method (JinaModel.ListModels) currently skips validating apiConfig and does not set the Authorization header, unlike other Jina calls such as CheckConnection; update ListModels to validate the provided apiConfig the same way other methods do and add the Bearer token to the request headers (e.g., set "Authorization: Bearer <token>" from apiConfig) before sending the GET to j.baseModel.GetBaseURL(...) + j.baseModel.URLSuffix.Models so the call is authenticated consistently with chat/embedding requests.web/src/components/ui/toggle-list.tsx (1)
1-343:⚠️ Potential issue | 🟠 Major | ⚡ Quick winModifications to
src/components/ui/require explicit approval.This file is under
src/components/ui/, which is protected by coding guidelines. All modifications—including the newfooterprop, search filter changes, styling updates, and footer rendering logic—require explicit user approval before merge.As per coding guidelines, if
ToggleListdoesn't meet requirements, wrap or compose it in a new component outsidesrc/components/ui/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 `@web/src/components/ui/toggle-list.tsx` around lines 1 - 343, The PR modifies a protected UI file (ToggleList) under src/components/ui/ without explicit approval; either revert those edits or move the new behavior into a new component outside that folder and leave ToggleList untouched. Specifically, remove the newly added footer prop/logic (footer, footerClassName, footerNode rendering) and any search/filter styling changes in ToggleList, or create a composed wrapper component (e.g., NewToggleListWrapper) outside src/components/ui/ that imports the original ToggleList and implements the footer, API search (onSearch), and loadMore behaviors there; ensure references to ToggleList, footer/footerNode, onSearch, and filteredOptions are adjusted accordingly and get explicit approval before modifying files under src/components/ui/.Source: Coding guidelines
🟡 Minor comments (9)
internal/handler/document.go-1013-1016 (1)
1013-1016:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winNormalize
doc_idsquery tokens before calling the service.Raw
strings.Splitcan pass whitespace/empty IDs (e.g., trailing commas), which causes avoidable lookup mismatches downstream.Proposed fix
var docIDS []string if docIDsParam := c.Query("doc_ids"); docIDsParam != "" { - docIDS = strings.Split(docIDsParam, ",") + for _, id := range strings.Split(docIDsParam, ",") { + id = strings.TrimSpace(id) + if id != "" { + docIDS = append(docIDS, id) + } + } }🤖 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 `@internal/handler/document.go` around lines 1013 - 1016, The code reading the doc_ids query uses strings.Split directly into docIDS (variable) which can leave whitespace and empty tokens; update the parsing so that after c.Query("doc_ids") is non-empty you split, then for each token apply strings.TrimSpace and only append non-empty values to docIDS (thus removing tokens from trailing/leading commas and whitespace) before passing docIDS to the downstream service (refer to the docIDS variable, the c.Query("doc_ids") call, and the current strings.Split usage).rag/graphrag/utils.py-707-721 (1)
707-721:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winHandle non-positive
n_hopexplicitly.With
n_hop=0(or negative), this function still emits 1-hop paths. Add an early return to keep behavior consistent with the documented contract.💡 Suggested fix
def n_neighbor(graph: nx.Graph, node, n_hop: int = 2): + if n_hop <= 0: + return [] + """Enumerate paths of up to ``n_hop`` edges starting at ``node`` together🤖 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 `@rag/graphrag/utils.py` around lines 707 - 721, The n_neighbor function currently emits 1-hop paths even when n_hop is 0 or negative; add an explicit early return in n_neighbor to handle non-positive n_hop (e.g., if n_hop <= 0: return []), so the function honors the documented "up to n_hop edges" contract; locate the function n_neighbor in rag/graphrag/utils.py and add the check before any graph.edges/node traversal and counting logic.internal/entity/models/gitee.go-902-917 (1)
902-917:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winResolve model metadata lookup after final name normalization.
GetModelByNameOrAliasruns before the@OwnedBysuffix is applied. For provider-qualified models, this can attach incorrect (or empty) metadata in the returnedListModelResponse.Suggested fix
- for _, model := range modelList.Models { - modelName := model.ID - var modelResponse ListModelResponse - pm := GetProviderManager() - modelEntity := pm.GetModelByNameOrAlias(modelName) - if model.OwnedBy != "" { - modelName = model.ID + "@" + model.OwnedBy - } - modelResponse.Name = modelName + for _, model := range modelList.Models { + rawName := model.ID + modelName := rawName + if model.OwnedBy != "" { + modelName = model.ID + "@" + model.OwnedBy + } + modelResponse := ListModelResponse{Name: modelName} + pm := GetProviderManager() + var modelEntity *Model + if pm != nil { + modelEntity = pm.GetModelByNameOrAlias(modelName) + if modelEntity == nil && modelName != rawName { + modelEntity = pm.GetModelByNameOrAlias(rawName) + } + } if modelEntity != nil { modelResponse.Dimension = modelEntity.Dimension modelResponse.MaxTokens = modelEntity.MaxTokens modelResponse.ModelTypes = modelEntity.ModelTypes modelResponse.Thinking = modelEntity.Thinking } models = append(models, modelResponse) }🤖 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 `@internal/entity/models/gitee.go` around lines 902 - 917, The code calls GetProviderManager().GetModelByNameOrAlias using modelName before the provider-qualified suffix is appended, causing wrong metadata; update the logic in the loop so that the modelName is normalized first (append "@" + model.OwnedBy when model.OwnedBy != "") and then call GetModelByNameOrAlias with the final modelName to populate ListModelResponse fields (Dimension, MaxTokens, ModelTypes, Thinking) from modelEntity; adjust references in the loop around modelName, model.OwnedBy, GetProviderManager(), GetModelByNameOrAlias and ListModelResponse accordingly.web/src/locales/en.ts-1559-1561 (1)
1559-1561:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse sentence case for the new English locale strings.
Several new
en.tslabels are Title Case ("AI Studio Access Token","Chat Channels","Add Channel","Bot Token", etc.), which conflicts with the repo’s sentence-case rule foren.ts.Suggested copy updates
- paddleocrAccessToken: 'AI Studio Access Token', + paddleocrAccessToken: 'AI Studio access token', @@ - channels: 'Chat Channels', - chatChannels: 'Chat Channels', - addChannel: 'Add Channel', - editChannel: 'Edit Channel', + channels: 'Chat channels', + chatChannels: 'Chat channels', + addChannel: 'Add channel', + editChannel: 'Edit channel', @@ - bot_token: 'Bot Token', - app_token: 'App Token', + bot_token: 'Bot token', + app_token: 'App token', app_id: 'App ID', - app_secret: 'App Secret', - encrypt_key: 'Encryption Key', + app_secret: 'App secret', + encrypt_key: 'Encryption key',As per coding guidelines,
web/src/locales/en.tsshould use sentence case for English locale values.Also applies to: 1792-1807
🤖 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 `@web/src/locales/en.ts` around lines 1559 - 1561, The locale values use Title Case instead of the repo-standard sentence case; update the English strings for keys like paddleocrAccessToken, paddleocrAccessTokenMessage, paddleocrAccessTokenPlaceholder (and the other keys around lines ~1792-1807 such as Chat Channels/Add Channel/Bot Token) to sentence case (e.g., "AI Studio access token", "Access token for PaddleOCR API (optional)", "Your AI Studio token (optional)") so all en.ts values follow the sentence-case guideline.Source: Coding guidelines
api/channels/slack.py-37-50 (1)
37-50:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winValidate required
dialog_idfield.The
dialog_idis required for dispatching messages but defaults to an empty string (line 42). Add validation in__init__orstart:if not self._dialog_id: raise ValueError("SlackChannel requires non-empty dialog_id in config")🤖 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 `@api/channels/slack.py` around lines 37 - 50, The SlackChannel currently allows an empty dialog_id which is required for message dispatch; update the constructor (__init__) or the start method to validate self._dialog_id and raise a ValueError when it's falsy (e.g., if not self._dialog_id: raise ValueError("SlackChannel requires non-empty dialog_id in config")), so locate the dialog initialization in __init__ (self._dialog_id) and add this guard before using it in start or any other method that relies on it.api/channels/feishu.py-44-50 (1)
44-50:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winValidate required
dialog_idfield.The
dialog_idis required for dispatching messages but defaults to an empty string (line 50). If a channel is created without a validdialog_id, messages will fail silently or with obscure errors during dispatch.Add validation in
__init__orstart:if not self._dialog_id: raise ValueError("FeishuChannel requires non-empty dialog_id in config")🤖 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 `@api/channels/feishu.py` around lines 44 - 50, Validate that self._dialog_id is non-empty in the Feishu channel initialization (either in __init__ or in start) and raise a clear error if missing: check self._dialog_id (from config.get("dialog_id")) and if falsy raise ValueError("FeishuChannel requires non-empty dialog_id in config") so the class (e.g., FeishuChannel.__init__ or FeishuChannel.start) fails fast instead of allowing silent failures when dispatching.api/channels/slack.py-61-84 (1)
61-84:⚠️ Potential issue | 🟡 MinorSlack bot message filter should also ignore
app_id(not justbot_id)
api/channels/slack.pycurrently drops bot messages only whenbot_idis absent, but the repo’s Slack filtering logic (common/data_source/slack_connector.py) treats messages withbot_idorapp_idas bot-origin. Update the handler condition to match that behavior (optionally also consideringbot_profile).if event.get("type") == "message" and not (event.get("bot_id") or event.get("app_id") or event.get("bot_profile")): ...🤖 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 `@api/channels/slack.py` around lines 61 - 84, Update the message-filtering condition in the async handler _handle_event so it ignores messages originating from bots/apps the same way as common/data_source/slack_connector.py; replace the current check that only looks for bot_id with a predicate that skips messages when event has bot_id or app_id or bot_profile. Locate the block that constructs IncomingMessage and calls self._dispatch/send and modify the if guarding that block to use not (event.get("bot_id") or event.get("app_id") or event.get("bot_profile")) to prevent bot/app echoes from being processed.web/src/pages/user-setting/channels/index.tsx-148-149 (1)
148-149:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winLocalize credential labels/placeholders instead of hardcoded English.
Several user-facing strings bypass i18n and will render untranslated in non-English locales.
As per coding guidelines, "Use react-i18next for internationalization supporting 17 languages."
Also applies to: 155-160, 164-170, 178-179, 185-186, 193-194
🤖 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 `@web/src/pages/user-setting/channels/index.tsx` around lines 148 - 149, Replace hardcoded English UI strings (e.g., placeholder="dialog ID" and other input labels/placeholders around the same block) with react-i18next translations: import and call useTranslation() to get t and replace literal strings passed to placeholder/label props with t('key') entries (e.g., placeholder={t('channels.dialogId')}). Update all occurrences mentioned in the review (the placeholder/label props near the demonstrated placeholder and the other noted string sites) and add appropriate i18n keys in your translation resource files so the same keys are available in supported languages.Source: Coding guidelines
web/src/services/channel.ts-5-10 (1)
5-10:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winOnly set
Authorizationwhen a token exists.Sending
Authorization: ''is avoidable and can cause inconsistent auth behavior through proxies/gateways.Suggested fix
function authHeaders(): HeadersInit { + const token = authorizationUtil.getAuthorization(); return { 'Content-Type': 'application/json', - Authorization: authorizationUtil.getAuthorization() ?? '', + ...(token ? { Authorization: token } : {}), }; }🤖 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 `@web/src/services/channel.ts` around lines 5 - 10, The authHeaders function currently always sets an Authorization header (possibly to an empty string); change authHeaders so it only adds the Authorization header when authorizationUtil.getAuthorization() returns a non-empty value — call authorizationUtil.getAuthorization(), check for a truthy/non-empty token, and only include Authorization in the returned HeadersInit when present (leave Content-Type always set). Refer to the authHeaders function and authorizationUtil.getAuthorization() when making the change.
🧹 Nitpick comments (7)
test/unit_test/rag/svr/task_executor_refactor/test_chunk_post_processor.py (1)
198-263: ⚡ Quick winAdd required pytest priority markers to the newly added tests.
The new async test cases are missing
p1/p2/p3markers required by the repo testing convention.As per coding guidelines,
test/**/*.py: "Use pytest with priority markers (p1/p2/p3) for Python testing."🤖 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 `@test/unit_test/rag/svr/task_executor_refactor/test_chunk_post_processor.py` around lines 198 - 263, Each new async test (test_generate_metadata_empty_config_does_not_crash, test_generate_metadata_enum_none_accepted, test_generate_metadata_description_none_accepted, test_generate_metadata_built_in_with_enum_none) must include the repo's pytest priority marker; add the appropriate marker decorator (e.g., `@pytest.mark.p1`) above each test in addition to `@pytest.mark.asyncio` so they follow the p1/p2/p3 convention used for tests that exercise generate_metadata in chunk_post_processor; ensure the decorator is imported/available in the test module.Source: Coding guidelines
internal/entity/models/model.go (1)
394-401: Normalizealias2ModelIndexkeys to lowercase to matchGetModelByNameOrAlias(lines ~312-318, ~395-402).
GetModelByNameOrAliaslowercases the lookup key (strings.ToLower(modelName)), butalias2ModelIndexis built with rawmodel.Name/alias(no lowercasing on insertion). The currentconf/all_models.jsonhas no mixed-casename/aliasentries (mixed_case_entries=0), so it works today; normalizing keys during map construction makes the behavior robust if config ever changes.🤖 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 `@internal/entity/models/model.go` around lines 394 - 401, GetModelByNameOrAlias lowercases the incoming modelName but Alias2ModelIndex keys are stored as raw model.Name/alias, causing case-mismatch bugs; when building/populating ProviderManager.Alias2ModelIndex (the code that inserts into Alias2ModelIndex for each Model in ProviderManager.AllModels), normalize keys to lowercase (use strings.ToLower on model.Name and each alias) so lookups in GetModelByNameOrAlias succeed regardless of case; update the map-insertion logic that currently writes to Alias2ModelIndex to insert lowercase keys.web/src/pages/user-setting/setting-model/modal/verify-button/index.tsx (1)
70-88: 💤 Low valueRedundant error handling in outer catch block.
The outer
catchblock at lines 78-88 is likely unreachable.onHandleVerifyalready catches all errors internally (lines 49-68) and callsverifyCallbackin its own catch block (lines 62-65). UnlessonHandleVerifyitself throws during setup, the outer catch won't execute.Consider removing the outer try-catch or documenting why both layers are needed.
🤖 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 `@web/src/pages/user-setting/setting-model/modal/verify-button/index.tsx` around lines 70 - 88, The outer try-catch in handleVerify is redundant because onHandleVerify already catches errors and calls verifyCallback; remove the outer catch and only await onHandleVerify inside handleVerify while still setting initial state via setVerifyResult and wrapping the setIsVerifying(false) in finally to ensure the loading state is cleared; keep references to handleVerify, onHandleVerify, verifyCallback, setVerifyResult, and setIsVerifying so the caller flow and error reporting remain handled by onHandleVerify.api/channels/feishu.py (2)
106-136: ⚖️ Poor tradeoffNon-streaming dispatch may timeout on long-running queries.
The
_dispatchmethod usesstream: False(line 118), which waits for the complete answer before returning. For complex queries that require extensive RAG retrieval or long LLM generation, this can exceed HTTP or application timeouts, causing the webhook to fail.Consider enabling streaming and aggregating the response incrementally, or make streaming configurable per channel based on expected query complexity.
🤖 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 `@api/channels/feishu.py` around lines 106 - 136, The _dispatch method currently calls async_iframe_completion with "stream": False which can cause timeouts for long RAG/LLM runs; change to streaming by passing "stream": True to async_iframe_completion (or make a channel/tenant flag like self._enable_streaming) and iterate the yielded chunks to incrementally build answer_text, update session id from each chunk, and only return an OutgoingMessage after aggregation completes (or send partial messages if configured); modify _dispatch, the async_iframe_completion call, and the chunk handling logic to support both streaming and non-streaming modes while preserving session updates in self._sessions for incoming.sender_id.
86-90: ⚡ Quick winLog JSON decode errors instead of silent failure.
When the Feishu message content fails to parse (lines 86-90), the handler returns
{}silently. This makes debugging difficult when Feishu sends malformed payloads.Add logging:
except (json.JSONDecodeError, AttributeError) as e: logger.warning("FeishuChannel: failed to parse message content: %s", e) return {}🤖 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 `@api/channels/feishu.py` around lines 86 - 90, The exception handler around json.loads of msg.get("content", "{}") currently swallows JSONDecodeError/AttributeError; modify the except block in the Feishu message parsing (the code that builds content_json and text) to log the failure before returning: catch the exception as e and call logger.warning (or the module logger) with a descriptive message like "FeishuChannel: failed to parse message content" including the exception e, then return {}; update the except clause that currently does "except (json.JSONDecodeError, AttributeError): return {}" to "except (json.JSONDecodeError, AttributeError) as e: logger.warning(...); return {}".Source: Coding guidelines
api/apps/restful_apis/channel_api.py (2)
91-110: ⚖️ Poor tradeoffConsider Pydantic request validation for consistency.
As per coding guidelines, newer RESTful APIs under
api/apps/restful_apis/should use Pydantic for request validation. This endpoint uses manual validation (lines 98-110), which is functional but less maintainable and more error-prone than a declarative Pydantic schema.Consider defining a
CreateChannelRequestPydantic model with field validators (e.g.,channel: Literal["slack", "feishu"],name: str, etc.) to replace the manual checks.🤖 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 `@api/apps/restful_apis/channel_api.py` around lines 91 - 110, Replace the manual request validation in create_channel by defining a Pydantic model (e.g., CreateChannelRequest) with fields name: str, channel: Literal[...] (or constraining via Enum/typing) dialog_id: str and config: dict, add any validators for trimming/required checks, then parse/validate the request payload instead of using get_request_json and the manual checks; after validation, use the parsed model instance and keep the existing channel-type lookup against CHANNEL_REGISTRY and _CHANNEL_INFO and error responses via get_error_data_result for backward-compatible error handling.Source: Coding guidelines
40-50: ⚖️ Poor tradeoffConsider deriving channel schemas from the registry.
The hardcoded
_CHANNEL_INFOdictionary creates tight coupling and duplication. If a new channel type is registered, this dictionary must be manually updated in lockstep. More critically, the validation at line 109 checks bothCHANNEL_REGISTRYand_CHANNEL_INFO, allowing a mismatch where a channel type could be created via the API but not instantiable at runtime.Consider adding a class method to the
Channelbase class (e.g.,get_credential_schema()) that returns the required fields, and derive_CHANNEL_INFOdynamically fromCHANNEL_REGISTRY.🤖 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 `@api/apps/restful_apis/channel_api.py` around lines 40 - 50, The hardcoded _CHANNEL_INFO dict is duplicated with CHANNEL_REGISTRY and can drift; change the implementation to derive the credential schema from each registered Channel class by adding a class method on Channel (e.g., get_credential_schema()) that returns the list/dict of required fields, then replace _CHANNEL_INFO usage with a dynamic build that iterates CHANNEL_REGISTRY and calls ChannelClass.get_credential_schema() for each key, and update the API validation logic (the code that currently checks CHANNEL_REGISTRY and _CHANNEL_INFO) to only consult CHANNEL_REGISTRY and the new get_credential_schema() results so creation and instantiation remain consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 49e698d8-7e5c-446e-bd44-7c580aeb247b
📒 Files selected for processing (117)
VERSIONapi/apps/restful_apis/channel_api.pyapi/apps/services/provider_api_service.pyapi/channels/__init__.pyapi/channels/base.pyapi/channels/bootstrap.pyapi/channels/feishu.pyapi/channels/registry.pyapi/channels/slack.pyapi/db/db_models.pyapi/ragflow_server.pycommon/metadata_utils.pyconf/service_conf.yamlconf/service_conf.yaml.templateentrypoint.shinternal/dao/database.gointernal/engine/elasticsearch/chunk.gointernal/engine/elasticsearch/chunk_helpers_test.gointernal/engine/elasticsearch/chunk_test.gointernal/entity/models/302ai.gointernal/entity/models/aliyun.gointernal/entity/models/anthropic.gointernal/entity/models/astraflow.gointernal/entity/models/avian.gointernal/entity/models/azure_openai.gointernal/entity/models/baichuan.gointernal/entity/models/baidu.gointernal/entity/models/bedrock.gointernal/entity/models/builtin.gointernal/entity/models/cohere.gointernal/entity/models/cometapi.gointernal/entity/models/deepinfra.gointernal/entity/models/deepseek.gointernal/entity/models/dummy.gointernal/entity/models/fishaudio.gointernal/entity/models/futurmix.gointernal/entity/models/gitee.gointernal/entity/models/google.gointernal/entity/models/gpustack.gointernal/entity/models/groq.gointernal/entity/models/huaweicloud.gointernal/entity/models/huggingface.gointernal/entity/models/hunyuan.gointernal/entity/models/jiekouai.gointernal/entity/models/jina.gointernal/entity/models/lmstudio.gointernal/entity/models/localai.gointernal/entity/models/longcat.gointernal/entity/models/mineru.gointernal/entity/models/mineru_local.gointernal/entity/models/minimax.gointernal/entity/models/mistral.gointernal/entity/models/model.gointernal/entity/models/model_test.gointernal/entity/models/modelscope.gointernal/entity/models/moonshot.gointernal/entity/models/n1n.gointernal/entity/models/novita.gointernal/entity/models/nvidia.gointernal/entity/models/ollama.gointernal/entity/models/openai.gointernal/entity/models/openrouter.gointernal/entity/models/orcarouter.gointernal/entity/models/paddleocr.gointernal/entity/models/paddleocr_local.gointernal/entity/models/perplexity.gointernal/entity/models/ppio.gointernal/entity/models/qiniu.gointernal/entity/models/replicate.gointernal/entity/models/siliconflow.gointernal/entity/models/stepfun.gointernal/entity/models/togetherai.gointernal/entity/models/tokenhub.gointernal/entity/models/tokenpony.gointernal/entity/models/types.gointernal/entity/models/upstage.gointernal/entity/models/vllm.gointernal/entity/models/volcengine.gointernal/entity/models/voyage.gointernal/entity/models/xai.gointernal/entity/models/xiaomi.gointernal/entity/models/xinference.gointernal/entity/models/xunfei.gointernal/entity/models/zhipu-ai.gointernal/handler/auth.gointernal/handler/document.gointernal/handler/document_test.gointernal/handler/providers.gointernal/router/router.gointernal/service/heartbeat_sender.gointernal/service/model_service.gorag/flow/parser/parser.pyrag/graphrag/search.pyrag/graphrag/utils.pyrag/llm/chat_model.pyrag/prompts/generator.pyrag/utils/ob_conn.pytest/unit_test/rag/graphrag/test_graphrag_utils.pytest/unit_test/rag/svr/task_executor_refactor/conftest.pytest/unit_test/rag/svr/task_executor_refactor/test_chunk_post_processor.pytools/scripts/mysql_migration.pyweb/src/components/ui/toggle-list.tsxweb/src/locales/en.tsweb/src/locales/zh.tsweb/src/pages/agent/constant/index.tsxweb/src/pages/agent/hooks/use-get-begin-query.tsxweb/src/pages/user-setting/channels/index.tsxweb/src/pages/user-setting/setting-model/modal/provider-modal/components/add-custom-model-dialog.tsxweb/src/pages/user-setting/setting-model/modal/provider-modal/components/addable-toggle-list.tsxweb/src/pages/user-setting/setting-model/modal/provider-modal/components/use-custom-model-fields.tsxweb/src/pages/user-setting/setting-model/modal/provider-modal/field-config/provider-config-map.tsweb/src/pages/user-setting/setting-model/modal/provider-modal/hooks/use-list-models-picker.tsweb/src/pages/user-setting/setting-model/modal/provider-modal/index.tsxweb/src/pages/user-setting/setting-model/modal/verify-button/index.tsxweb/src/pages/user-setting/sidebar/index.tsxweb/src/routes.tsxweb/src/services/channel.ts
…token expiry, Go test compilation
@coderabbitai Thank you for the detailed review. The critical issues (credential nesting, Feishu token expiry, Go test compilation) were fixed in commit |
|
Tip For best results, initiate chat on the files or code changes.
The remaining open items from the review (Pydantic validation for Good luck with the human review — the PR is in good shape! 🐇✨ |
|
@zaviermeekz-cpu
|
|
@xugangqiang The PR is ready for review. I have resolved all conflicts and added unit tests. Live testing with a real Slack bot was partially successful (channel starts, logs confirm). Due to Slack API setup complexity, I could not complete the full message round‑trip, but the framework is fully implemented and follows the same pattern as Feishu. If possible, please help verify the final Slack integration. Thank you. |
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
api/ragflow_server.py (1)
36-36: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winChannel startup uses a stale API and creates conflicting runtimes.
Line 36 imports
reconcile_channels, butapi/channels/bootstrap.pyin this PR exposesrun_channels/start_channel_server; this can fail startup withImportError. Also, Lines 137-141 start a reconcile task while Lines 150-170 start a separate channel server thread, which risks duplicate channel loops when both paths are active. Keep one startup path (preferablystart_channel_server(stop_event)) and remove stale reconcile imports/hooks.Also applies to: 137-141, 150-170
🤖 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 `@api/ragflow_server.py` at line 36, The file still imports and invokes the stale reconcile_channels path; remove the import "reconcile_channels" and any startup hook that calls it (search for reconcile_channels and the task started at lines ~137-141) and instead use the single canonical channel startup path start_channel_server(stop_event) (or run_channels if code prefers) — keep only one of start_channel_server(stop_event) or run_channels, remove the alternate thread/task creation (the block around lines ~150-170) to avoid duplicate channel loops, and update the import to import start_channel_server (or run_channels) from api.channels.bootstrap so the module uses the current API. Ensure the chosen startup is passed the existing stop_event and no reconcile task remains.
🤖 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 `@api/channels/bootstrap.py`:
- Around line 153-156: The except block currently sets answer_text =
f"**ERROR**: {ex}", which leaks internal exception details to users; instead
keep LOGGER.exception("[%s:%s] completion failed", ch.channel_id, ch.account_id,
exc_info=True) (or rely on the existing LOGGER.exception call) and replace the
user-facing answer_text assignment (in the same except block) with a generic
message such as "**ERROR**: An internal error occurred while processing your
request. Please try again later." to avoid exposing sensitive/internal exception
content.
- Line 37: The bundled channels tuple _BUNDLED_CHANNELS is missing "slack", so
_register_channels() never imports Slack and its builder won't self-register;
add the string "slack" to the _BUNDLED_CHANNELS tuple and also update any other
channel-enumeration/list in the same module (the import/registration block that
mirrors _BUNDLED_CHANNELS) so Slack is imported during _register_channels() and
can register its builder.
- Around line 102-103: The code is calling DialogService.async_chat directly;
update this channel flow to use the unified LLMBundle from
api.db.services.llm_service instead. Replace imports of DialogService/async_chat
with an import of LLMBundle, instantiate or retrieve the appropriate LLMBundle
instance used elsewhere in the app, and route all LLM calls (the places
referencing DialogService.async_chat around the current block and the similar
section at lines ~148-152) through the LLMBundle API (e.g., the bundle's async
generate/chat method) while preserving the same input/output handling and using
get_uuid and existing context objects as before.
- Around line 226-230: The current loop that drops entries from the failed dict
permanently skips retries for unchanged configs (failed, desired, account_id,
failed[account_id]) — replace the binary failed mapping with a retry-aware
structure (e.g., store failure metadata like failure_hash, last_failed_at
timestamp and attempt_count instead of just fp) and update the loop that checks
desired vs failed to allow timed retries/backoff: when desired[account_id][2] ==
failed[account_id].failure_hash, consult last_failed_at and attempt_count to
decide whether to re-enqueue/retry the start (exponential or fixed backoff) and
only purge on config change or after success; ensure functions that set failures
(where failed[account_id] = fp currently) populate the metadata fields and
increment attempt_count so the bootstrap loop can honor retry schedules.
In `@api/db/db_models.py`:
- Around line 1218-1232: Remove the duplicate ChatChannel class and keep a
single authoritative definition for ChatChannel with dialog_id nullable
(default=None) and status defaulting to "1"; update the remaining ChatChannel
model (the one referenced by ChatChannelService.list_active()) so its field
types/defaults match the migration contract and ensure the migration/schema
entry for ChatChannel reflects dialog_id nullable and status default "1"; then
run/adjust migrations and tests to confirm the service filter status == "1" will
correctly find active channels.
---
Outside diff comments:
In `@api/ragflow_server.py`:
- Line 36: The file still imports and invokes the stale reconcile_channels path;
remove the import "reconcile_channels" and any startup hook that calls it
(search for reconcile_channels and the task started at lines ~137-141) and
instead use the single canonical channel startup path
start_channel_server(stop_event) (or run_channels if code prefers) — keep only
one of start_channel_server(stop_event) or run_channels, remove the alternate
thread/task creation (the block around lines ~150-170) to avoid duplicate
channel loops, and update the import to import start_channel_server (or
run_channels) from api.channels.bootstrap so the module uses the current API.
Ensure the chosen startup is passed the existing stop_event and no reconcile
task remains.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: df3ac963-abe1-4440-9ca1-42acf9458f5b
📒 Files selected for processing (4)
api/channels/bootstrap.pyapi/db/db_models.pyapi/ragflow_server.pyconf/service_conf.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- conf/service_conf.yaml
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
api/ragflow_server.py (1)
36-36: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winChannel startup uses a stale API and creates conflicting runtimes.
Line 36 imports
reconcile_channels, butapi/channels/bootstrap.pyin this PR exposesrun_channels/start_channel_server; this can fail startup withImportError. Also, Lines 137-141 start a reconcile task while Lines 150-170 start a separate channel server thread, which risks duplicate channel loops when both paths are active. Keep one startup path (preferablystart_channel_server(stop_event)) and remove stale reconcile imports/hooks.Also applies to: 137-141, 150-170
🤖 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 `@api/ragflow_server.py` at line 36, The file still imports and invokes the stale reconcile_channels path; remove the import "reconcile_channels" and any startup hook that calls it (search for reconcile_channels and the task started at lines ~137-141) and instead use the single canonical channel startup path start_channel_server(stop_event) (or run_channels if code prefers) — keep only one of start_channel_server(stop_event) or run_channels, remove the alternate thread/task creation (the block around lines ~150-170) to avoid duplicate channel loops, and update the import to import start_channel_server (or run_channels) from api.channels.bootstrap so the module uses the current API. Ensure the chosen startup is passed the existing stop_event and no reconcile task remains.
🤖 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 `@api/channels/bootstrap.py`:
- Around line 153-156: The except block currently sets answer_text =
f"**ERROR**: {ex}", which leaks internal exception details to users; instead
keep LOGGER.exception("[%s:%s] completion failed", ch.channel_id, ch.account_id,
exc_info=True) (or rely on the existing LOGGER.exception call) and replace the
user-facing answer_text assignment (in the same except block) with a generic
message such as "**ERROR**: An internal error occurred while processing your
request. Please try again later." to avoid exposing sensitive/internal exception
content.
- Line 37: The bundled channels tuple _BUNDLED_CHANNELS is missing "slack", so
_register_channels() never imports Slack and its builder won't self-register;
add the string "slack" to the _BUNDLED_CHANNELS tuple and also update any other
channel-enumeration/list in the same module (the import/registration block that
mirrors _BUNDLED_CHANNELS) so Slack is imported during _register_channels() and
can register its builder.
- Around line 102-103: The code is calling DialogService.async_chat directly;
update this channel flow to use the unified LLMBundle from
api.db.services.llm_service instead. Replace imports of DialogService/async_chat
with an import of LLMBundle, instantiate or retrieve the appropriate LLMBundle
instance used elsewhere in the app, and route all LLM calls (the places
referencing DialogService.async_chat around the current block and the similar
section at lines ~148-152) through the LLMBundle API (e.g., the bundle's async
generate/chat method) while preserving the same input/output handling and using
get_uuid and existing context objects as before.
- Around line 226-230: The current loop that drops entries from the failed dict
permanently skips retries for unchanged configs (failed, desired, account_id,
failed[account_id]) — replace the binary failed mapping with a retry-aware
structure (e.g., store failure metadata like failure_hash, last_failed_at
timestamp and attempt_count instead of just fp) and update the loop that checks
desired vs failed to allow timed retries/backoff: when desired[account_id][2] ==
failed[account_id].failure_hash, consult last_failed_at and attempt_count to
decide whether to re-enqueue/retry the start (exponential or fixed backoff) and
only purge on config change or after success; ensure functions that set failures
(where failed[account_id] = fp currently) populate the metadata fields and
increment attempt_count so the bootstrap loop can honor retry schedules.
In `@api/db/db_models.py`:
- Around line 1218-1232: Remove the duplicate ChatChannel class and keep a
single authoritative definition for ChatChannel with dialog_id nullable
(default=None) and status defaulting to "1"; update the remaining ChatChannel
model (the one referenced by ChatChannelService.list_active()) so its field
types/defaults match the migration contract and ensure the migration/schema
entry for ChatChannel reflects dialog_id nullable and status default "1"; then
run/adjust migrations and tests to confirm the service filter status == "1" will
correctly find active channels.
---
Outside diff comments:
In `@api/ragflow_server.py`:
- Line 36: The file still imports and invokes the stale reconcile_channels path;
remove the import "reconcile_channels" and any startup hook that calls it
(search for reconcile_channels and the task started at lines ~137-141) and
instead use the single canonical channel startup path
start_channel_server(stop_event) (or run_channels if code prefers) — keep only
one of start_channel_server(stop_event) or run_channels, remove the alternate
thread/task creation (the block around lines ~150-170) to avoid duplicate
channel loops, and update the import to import start_channel_server (or
run_channels) from api.channels.bootstrap so the module uses the current API.
Ensure the chosen startup is passed the existing stop_event and no reconcile
task remains.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: df3ac963-abe1-4440-9ca1-42acf9458f5b
📒 Files selected for processing (4)
api/channels/bootstrap.pyapi/db/db_models.pyapi/ragflow_server.pyconf/service_conf.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- conf/service_conf.yaml
🛑 Comments failed to post (5)
api/channels/bootstrap.py (4)
37-37: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
slackis missing from bundled channel registration.Line 37 omits
slack, but_register_channels()only imports modules listed in_BUNDLED_CHANNELS. If Slack isn’t imported elsewhere, its builder never self-registers and Slack channels won’t start.Also applies to: 43-54
🤖 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 `@api/channels/bootstrap.py` at line 37, The bundled channels tuple _BUNDLED_CHANNELS is missing "slack", so _register_channels() never imports Slack and its builder won't self-register; add the string "slack" to the _BUNDLED_CHANNELS tuple and also update any other channel-enumeration/list in the same module (the import/registration block that mirrors _BUNDLED_CHANNELS) so Slack is imported during _register_channels() and can register its builder.
102-103: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Use
LLMBundleas the unified LLM interface in this new channel flow.Line 102 routes generation through
DialogService.async_chatdirectly. This new LLM interaction path should be wired throughLLMBundlefor consistency with backend policy.
As per coding guidelines,**/*.py: Use LLMBundle from api.db.services.llm_service as the unified interface for LLM interactions.Also applies to: 148-152
🤖 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 `@api/channels/bootstrap.py` around lines 102 - 103, The code is calling DialogService.async_chat directly; update this channel flow to use the unified LLMBundle from api.db.services.llm_service instead. Replace imports of DialogService/async_chat with an import of LLMBundle, instantiate or retrieve the appropriate LLMBundle instance used elsewhere in the app, and route all LLM calls (the places referencing DialogService.async_chat around the current block and the similar section at lines ~148-152) through the LLMBundle API (e.g., the bundle's async generate/chat method) while preserving the same input/output handling and using get_uuid and existing context objects as before.Source: Coding guidelines
153-156: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Do not expose raw exception messages to end users.
Line 155 sends
exback to chat users ("**ERROR**: {ex}"), which can leak internal details or sensitive backend context. Return a generic failure message and keep detailed diagnostics in logs only.🤖 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 `@api/channels/bootstrap.py` around lines 153 - 156, The except block currently sets answer_text = f"**ERROR**: {ex}", which leaks internal exception details to users; instead keep LOGGER.exception("[%s:%s] completion failed", ch.channel_id, ch.account_id, exc_info=True) (or rely on the existing LOGGER.exception call) and replace the user-facing answer_text assignment (in the same except block) with a generic message such as "**ERROR**: An internal error occurred while processing your request. Please try again later." to avoid exposing sensitive/internal exception content.
226-230: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Failed channel starts are suppressed indefinitely for unchanged configs.
Once
failed[account_id] = fpis set, unchanged configs are skipped forever (Lines 234-235). A transient outage during startup can keep a bot down permanently until manual config edits. Add timed retry/backoff instead of permanent suppression.Also applies to: 233-237
🤖 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 `@api/channels/bootstrap.py` around lines 226 - 230, The current loop that drops entries from the failed dict permanently skips retries for unchanged configs (failed, desired, account_id, failed[account_id]) — replace the binary failed mapping with a retry-aware structure (e.g., store failure metadata like failure_hash, last_failed_at timestamp and attempt_count instead of just fp) and update the loop that checks desired vs failed to allow timed retries/backoff: when desired[account_id][2] == failed[account_id].failure_hash, consult last_failed_at and attempt_count to decide whether to re-enqueue/retry the start (exponential or fixed backoff) and only purge on config change or after success; ensure functions that set failures (where failed[account_id] = fp currently) populate the metadata fields and increment attempt_count so the bootstrap loop can honor retry schedules.api/db/db_models.py (1)
1218-1232: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Remove the duplicate
ChatChannelmodel; it breaks the runtime status/schema contract.Line 1218 introduces
ChatChannel, but anotherChatChannelalready exists at Line 1362. Python keeps only the later class, so the first model’sstatus="1"and nullabledialog_idcontract is effectively ignored. This conflicts withChatChannelService.list_active()(api/db/services/chat_channel_service.py, Line 55) which filtersstatus == "1", and can cause enabled channels to be skipped by reconciliation. Keep a singleChatChanneldefinition and alignstatus/dialog_idsemantics across model + migration (Line 1725) + service filter.Also applies to: 1362-1373, 1725-1725
🤖 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 `@api/db/db_models.py` around lines 1218 - 1232, Remove the duplicate ChatChannel class and keep a single authoritative definition for ChatChannel with dialog_id nullable (default=None) and status defaulting to "1"; update the remaining ChatChannel model (the one referenced by ChatChannelService.list_active()) so its field types/defaults match the migration contract and ensure the migration/schema entry for ChatChannel reflects dialog_id nullable and status default "1"; then run/adjust migrations and tests to confirm the service filter status == "1" will correctly find active channels.
What problem does this PR solve?
RAGFlow currently has the full plumbing for chat‑channel bots, but only Feishu/Lark is implemented and tested end‑to‑end. This PR adds the corresponding front‑end and back‑end code for Slack, Feishu (as reference), and a generic framework so other channels (Discord, Telegram, WhatsApp, etc.) can be added later.
Type of change
How has this been tested?
Channelabstract class, registry, bootstrap reconcile loop.slack_sdk).ChatChanneladded (Peewee)./api/v1/channels.Manual verification (limited due to Docker environment on Windows):
Checklist
Related Issue
Closes #15844
Additional context
This is a draft PR – the backend channel reconciler and webhook routes require testing in a full Docker environment. The main code is complete; the remaining work is integration and validation by maintainers with a working setup.