Refa: align chat and search restful APIs#14229
Merged
JinHai-CN merged 9 commits intoinfiniflow:mainfrom Apr 22, 2026
Merged
Conversation
dosubot
Bot
added
the
size:L
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:
📝 WalkthroughWalkthroughMoved chat/session IDs from URL to request bodies, grouped chat/audio endpoints under /chat/, changed session update HTTP method PUT→PATCH, added SSE search-completion endpoint, added default completion dialog and session-creation logic, and updated frontend, SDK, tests, and docs to match new contracts. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API as Chat API
participant Auth
participant DB as Conversation DB
participant LLM as LLM Service
Client->>API: POST /chat/completions { messages, chat_id?, session_id? }
API->>Auth: verify ownership if chat_id present
Auth-->>API: OK / error
alt chat_id provided
API->>DB: load chat/dialog config
opt session_id provided
API->>DB: load session (verify dialog_id == chat_id)
DB-->>API: session or not found
end
alt session missing
API->>DB: create session for completion
DB-->>API: new session_id
end
API->>LLM: call with dialog context (conv + messages)
else chat_id absent
API->>LLM: call with default completion dialog (no conv persistence)
end
LLM-->>API: stream tokens / final answer
API-->>Client: stream/return structured answer (include chat_id only if provided)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
![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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
web/src/pages/next-search/hooks.ts (1)
330-373:⚠️ Potential issue | 🟠 MajorMissing
askUrlandsharedIdin dependency array.The
sendQuestioncallback usesaskUrl(line 343) andsharedId(line 338) but neither is included in the dependency array (lines 362-372). This can lead to stale closures where the callback uses outdated URL or ID values.🐛 Proposed fix to add missing dependencies
[ send, testChunk, kbIds, fetchRelatedQuestions, setPagination, pagination.pageSize, tenantId, searchId, related_search, + askUrl, + sharedId, ],🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/pages/next-search/hooks.ts` around lines 330 - 373, The sendQuestion useCallback closes over askUrl and sharedId but they are missing from its dependency array, which can cause stale values; update the dependency array for the sendQuestion callback (defined as sendQuestion) to include askUrl and sharedId so the hook re-creates when those values change, ensuring the checks that reference sharedId and the call to send(askUrl, ...) use current values.web/src/pages/next-chats/hooks/use-send-single-message.ts (1)
69-102:⚠️ Potential issue | 🟠 MajorMissing
chatIdin dependency array.The
sendMessagecallback useschatId(line 72) but it's not included in the dependency array (lines 94-101). This can cause stale closure issues wheresendMessageuses an outdatedchatIdvalue after navigation.🐛 Proposed fix to add missing dependency
[ derivedMessages, conversationId, + chatId, removeLatestMessage, setValue, send, controller, ],🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/pages/next-chats/hooks/use-send-single-message.ts` around lines 69 - 102, The useSendSingleMessage hook's sendMessage callback closes over chatId but the dependency array for the useCallback (the array currently listing derivedMessages, conversationId, removeLatestMessage, setValue, send, controller) is missing chatId; update the dependency array for the sendMessage useCallback to include chatId so the callback is recreated when chatId changes (look for the sendMessage function inside useSendSingleMessage and add chatId to its dependency list).api/apps/restful_apis/chat_api.py (1)
1069-1075:⚠️ Potential issue | 🟠 MajorPersist the session even when a model override is used.
is_embedded = bool(chat_model_id)suppressesConversationService.update_by_id(...)for any request that setsllm_id. That means existing or auto-created sessions stream a reply but never save the updatedmessages/reference, so the turn disappears on reload.💡 Suggested fix
- is_embedded = bool(chat_model_id) + should_persist_session = conv is not None stream_mode = req.pop("stream", True) @@ - if conv is not None and not is_embedded: + if should_persist_session: ConversationService.update_by_id(conv.id, conv.to_dict()) @@ - if conv is not None and not is_embedded: + if should_persist_session: ConversationService.update_by_id(conv.id, conv.to_dict())Also applies to: 1090-1092, 1107-1109
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/apps/restful_apis/chat_api.py` around lines 1069 - 1075, The current check sets is_embedded = bool(chat_model_id) which prevents ConversationService.update_by_id(...) from running when a request provides a model override (chat_model_id), causing messages/reference not to be persisted; change the logic so that overriding dia.llm_id/dia.llm_setting does not skip persistence—either set is_embedded = False when a model override is present or ensure ConversationService.update_by_id(...) is called unconditionally after handling dia.llm_id and dia.llm_setting; apply the same fix to the other occurrences near the blocks referenced (around lines that touch is_embedded, chat_model_id, and ConversationService.update_by_id) so sessions are always updated regardless of model override.
🧹 Nitpick comments (2)
web/src/hooks/logic-hooks.ts (1)
303-320: Consider retaining minimal error logging for production debugging.The error handling changes silently swallow all errors without logging. While this simplifies the code, it may make diagnosing issues in production more difficult. Consider adding structured logging (e.g.,
console.warnor a logging service) for unexpected errors while still swallowing them to prevent UI crashes.♻️ Optional: Add minimal error logging
- } catch { + } catch (e) { + // Log parse errors in development for debugging + if (process.env.NODE_ENV === 'development') { + console.debug('SSE parse error:', e); + } // Swallow parse errors silently }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/hooks/logic-hooks.ts` around lines 303 - 320, The catch blocks in the streaming logic currently swallow errors silently; update them to emit minimal structured logs so production debugging is possible: in the inner JSON/parse catch (the one that currently comments "// Swallow parse errors silently") add a console.warn or call to your logging utility that includes a short message and the caught error; in the outer catch that swallows fetch errors (the one after setDoneValue(body, true); resetAnswer();) log unexpected errors similarly but retain the existing AbortError handling (i.e., continue to break on DOMException with name 'AbortError' and only log other errors). Reference the surrounding helpers setDoneValue and resetAnswer so the logging is placed before those calls where appropriate.api/apps/restful_apis/chat_api.py (1)
145-158: Add logs around implicit session creation.This introduces a new write path that creates sessions outside the explicit session API, but it emits no trace with the source
chat_id, createdsession_id, oruser_id. That will make session-id regressions and unexpected auto-created conversations much harder to diagnose. As per coding guidelines,**/*.py: Add logging for new flows`.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/apps/restful_apis/chat_api.py` around lines 145 - 158, The _create_session_for_completion flow creates conversations implicitly but lacks audit logs; update _create_session_for_completion to log an info/debug entry before and after creation including chat_id (dialog), the generated session id (from get_uuid()/conv["id"]), and user_id, and on failure log an error with those identifiers and the resulting error path; place logs around ConversationService.save and after ConversationService.get_by_id returns to record success (session_id, chat_id, user_id) and on not-ok raise log the failure with same identifiers to aid tracing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@api/apps/restful_apis/chat_api.py`:
- Around line 1057-1059: The code clears chat_model_id which causes an explicit
llm_id request field to be ignored for chat-less completions; instead, preserve
or apply the requested llm_id when chat_id is omitted: in the branch where you
call _build_default_completion_dialog() and set dia.llm_setting, do not
unconditionally reset chat_model_id to "", and if an llm_id was provided on the
request, assign that value to chat_model_id (or propagate it into
dia.llm_setting) so one-off completions can target the specified model; locate
usage of _build_default_completion_dialog(), dia.llm_setting, chat_model_id,
llm_id and chat_id to implement this change.
- Around line 128-142: The default dialog builder
_build_default_completion_dialog currently uses the RAG-aware
_DEFAULT_PROMPT_CONFIG which injects KB/system prompts even for the plain
/chat/completions (no chat_id) path; change the logic so that when creating the
completion-only dialog (i.e., kb_ids is empty or in the code path handling
requests with no chat_id) you assign a non-RAG prompt_config (a plain
conversation system prompt) instead of _DEFAULT_PROMPT_CONFIG—update the
_build_default_completion_dialog or the caller that constructs dialogs for the
no-chat_id flow to select an alternative prompt_config (or a shallow copy
without {knowledge}/“not found” wording) so completion requests are not biased
toward retrieval-style responses.
In `@api/apps/restful_apis/search_api.py`:
- Around line 176-215: Add structured logging for the new completion flow:
ensure a module-level logger (logger = logging.getLogger(__name__)) exists, then
instrument the completion handler and inner stream generator to log key events —
entry to completion (log search_id and current_user.id), authorization failure
(when SearchService.accessible4deletion returns False), missing search_app (when
get_detail returns None), missing kb_ids, and before starting the async_ask
loop; inside the stream() except block log the full exception with
logger.exception or logger.error(..., exc_info=True) when async_ask yields
errors and also log completion of the stream (success path) — reference function
names completion, stream, async_ask and service SearchService/get_detail to
locate where to add these logs.
- Around line 203-207: The except block currently yields str(ex) directly to
clients (see the except Exception as ex handler in search_api.py), which can
leak internal details; change it to return a generic error payload (e.g., code
500 and a non-sensitive "Internal server error" message and sanitized
data.answer) while logging the full exception server-side using logger.exception
or similar inside the same except block so the stack trace is preserved for
debugging; update the yield in that handler to use the generic message and keep
logging of ex separate and detailed.
---
Outside diff comments:
In `@api/apps/restful_apis/chat_api.py`:
- Around line 1069-1075: The current check sets is_embedded =
bool(chat_model_id) which prevents ConversationService.update_by_id(...) from
running when a request provides a model override (chat_model_id), causing
messages/reference not to be persisted; change the logic so that overriding
dia.llm_id/dia.llm_setting does not skip persistence—either set is_embedded =
False when a model override is present or ensure
ConversationService.update_by_id(...) is called unconditionally after handling
dia.llm_id and dia.llm_setting; apply the same fix to the other occurrences near
the blocks referenced (around lines that touch is_embedded, chat_model_id, and
ConversationService.update_by_id) so sessions are always updated regardless of
model override.
In `@web/src/pages/next-chats/hooks/use-send-single-message.ts`:
- Around line 69-102: The useSendSingleMessage hook's sendMessage callback
closes over chatId but the dependency array for the useCallback (the array
currently listing derivedMessages, conversationId, removeLatestMessage,
setValue, send, controller) is missing chatId; update the dependency array for
the sendMessage useCallback to include chatId so the callback is recreated when
chatId changes (look for the sendMessage function inside useSendSingleMessage
and add chatId to its dependency list).
In `@web/src/pages/next-search/hooks.ts`:
- Around line 330-373: The sendQuestion useCallback closes over askUrl and
sharedId but they are missing from its dependency array, which can cause stale
values; update the dependency array for the sendQuestion callback (defined as
sendQuestion) to include askUrl and sharedId so the hook re-creates when those
values change, ensuring the checks that reference sharedId and the call to
send(askUrl, ...) use current values.
---
Nitpick comments:
In `@api/apps/restful_apis/chat_api.py`:
- Around line 145-158: The _create_session_for_completion flow creates
conversations implicitly but lacks audit logs; update
_create_session_for_completion to log an info/debug entry before and after
creation including chat_id (dialog), the generated session id (from
get_uuid()/conv["id"]), and user_id, and on failure log an error with those
identifiers and the resulting error path; place logs around
ConversationService.save and after ConversationService.get_by_id returns to
record success (session_id, chat_id, user_id) and on not-ok raise log the
failure with same identifiers to aid tracing.
In `@web/src/hooks/logic-hooks.ts`:
- Around line 303-320: The catch blocks in the streaming logic currently swallow
errors silently; update them to emit minimal structured logs so production
debugging is possible: in the inner JSON/parse catch (the one that currently
comments "// Swallow parse errors silently") add a console.warn or call to your
logging utility that includes a short message and the caught error; in the outer
catch that swallows fetch errors (the one after setDoneValue(body, true);
resetAnswer();) log unexpected errors similarly but retain the existing
AbortError handling (i.e., continue to break on DOMException with name
'AbortError' and only log other errors). Reference the surrounding helpers
setDoneValue and resetAnswer so the logging is placed before those calls where
appropriate.
🪄 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: c13cf4bf-c498-49a6-8b63-1e4e55b09716
📒 Files selected for processing (11)
api/apps/restful_apis/chat_api.pyapi/apps/restful_apis/search_api.pydocs/references/http_api_reference.mdtest/testcases/test_http_api/common.pytest/testcases/test_http_api/test_session_management/test_chat_completions.pyweb/src/hooks/logic-hooks.tsweb/src/pages/next-chats/hooks/use-send-chat-message.tsweb/src/pages/next-chats/hooks/use-send-single-message.tsweb/src/pages/next-search/hooks.tsweb/src/services/next-chat-service.tsweb/src/utils/api.ts
Comment on lines
+176
to
+215
| @manager.route("/searches/<search_id>/completion", methods=["POST"]) # noqa: F821 | ||
| @login_required | ||
| @validate_request("question") | ||
| async def completion(search_id): | ||
| if not SearchService.accessible4deletion(search_id, current_user.id): | ||
| return get_json_result( | ||
| data=False, | ||
| message="No authorization.", | ||
| code=RetCode.AUTHENTICATION_ERROR, | ||
| ) | ||
|
|
||
| req = await get_request_json() | ||
| uid = current_user.id | ||
| search_app = SearchService.get_detail(search_id) | ||
| if not search_app: | ||
| return get_data_error_result(message=f"Cannot find search {search_id}") | ||
|
|
||
| search_config = search_app.get("search_config", {}) | ||
| kb_ids = search_config.get("kb_ids") or req.get("kb_ids") or [] | ||
| if not kb_ids: | ||
| return get_data_error_result(message="`kb_ids` is required.") | ||
|
|
||
| async def stream(): | ||
| nonlocal req, uid, kb_ids, search_config | ||
| try: | ||
| async for ans in async_ask(req["question"], kb_ids, uid, search_config=search_config): | ||
| yield "data:" + json.dumps({"code": 0, "message": "", "data": ans}, ensure_ascii=False) + "\n\n" | ||
| except Exception as ex: | ||
| yield "data:" + json.dumps( | ||
| {"code": 500, "message": str(ex), "data": {"answer": "**ERROR**: " + str(ex), "reference": []}}, | ||
| ensure_ascii=False, | ||
| ) + "\n\n" | ||
| yield "data:" + json.dumps({"code": 0, "message": "", "data": True}, ensure_ascii=False) + "\n\n" | ||
|
|
||
| resp = Response(stream(), mimetype="text/event-stream") | ||
| resp.headers.add_header("Cache-control", "no-cache") | ||
| resp.headers.add_header("Connection", "keep-alive") | ||
| resp.headers.add_header("X-Accel-Buffering", "no") | ||
| resp.headers.add_header("Content-Type", "text/event-stream; charset=utf-8") | ||
| return resp |
Contributor
There was a problem hiding this comment.
Add logging for the new completion flow.
The new endpoint lacks logging for request handling and error conditions. As per coding guidelines, new flows should include logging for observability and debugging.
🔧 Proposed fix to add logging
+import logging
+
+logger = logging.getLogger(__name__)
+
`@manager.route`("/searches/<search_id>/completion", methods=["POST"]) # noqa: F821
`@login_required`
`@validate_request`("question")
async def completion(search_id):
+ logger.info(f"Search completion request for search_id={search_id}")
if not SearchService.accessible4deletion(search_id, current_user.id):
+ logger.warning(f"Unauthorized access attempt to search_id={search_id} by user={current_user.id}")
return get_json_result(
data=False,
message="No authorization.",
code=RetCode.AUTHENTICATION_ERROR,
)As per coding guidelines: **/*.py: Add logging for new flows.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@api/apps/restful_apis/search_api.py` around lines 176 - 215, Add structured
logging for the new completion flow: ensure a module-level logger (logger =
logging.getLogger(__name__)) exists, then instrument the completion handler and
inner stream generator to log key events — entry to completion (log search_id
and current_user.id), authorization failure (when
SearchService.accessible4deletion returns False), missing search_app (when
get_detail returns None), missing kb_ids, and before starting the async_ask
loop; inside the stream() except block log the full exception with
logger.exception or logger.error(..., exc_info=True) when async_ask yields
errors and also log completion of the stream (success path) — reference function
names completion, stream, async_ask and service SearchService/get_detail to
locate where to add these logs.
Comment on lines
+203
to
+207
| except Exception as ex: | ||
| yield "data:" + json.dumps( | ||
| {"code": 500, "message": str(ex), "data": {"answer": "**ERROR**: " + str(ex), "reference": []}}, | ||
| ensure_ascii=False, | ||
| ) + "\n\n" |
Contributor
There was a problem hiding this comment.
Avoid exposing raw exception details to clients.
The error handler yields str(ex) directly to the client, which may leak internal implementation details or sensitive information. Consider using a generic error message while logging the full exception server-side.
🛡️ Proposed fix to sanitize error response
except Exception as ex:
+ logger.exception(f"Error in search completion for search_id={search_id}")
yield "data:" + json.dumps(
- {"code": 500, "message": str(ex), "data": {"answer": "**ERROR**: " + str(ex), "reference": []}},
+ {"code": 500, "message": "An internal error occurred.", "data": {"answer": "**ERROR**: An internal error occurred.", "reference": []}},
ensure_ascii=False,
) + "\n\n"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@api/apps/restful_apis/search_api.py` around lines 203 - 207, The except block
currently yields str(ex) directly to clients (see the except Exception as ex
handler in search_api.py), which can leak internal details; change it to return
a generic error payload (e.g., code 500 and a non-sensitive "Internal server
error" message and sanitized data.answer) while logging the full exception
server-side using logger.exception or similar inside the same except block so
the stack trace is preserved for debugging; update the yield in that handler to
use the generic message and keep logging of ex separate and detailed.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
api/apps/restful_apis/chat_api.py (1)
927-943:⚠️ Potential issue | 🟠 MajorAlways clean up the temp audio file on non-stream failures.
Once
temp_audio_pathis created, any exception fromuploaded.save(...), model resolution, orasr_mdl.transcription(...)leaks the file on disk. This endpoint can accumulate orphan temp files under normal error conditions.💡 Suggested fix
fd, temp_audio_path = tempfile.mkstemp(suffix=suffix) os.close(fd) - await uploaded.save(temp_audio_path) - - try: - default_asr_model_config = get_tenant_default_model_by_type(current_user.id, LLMType.SPEECH2TEXT) - except Exception as e: - return get_data_error_result(message=str(e)) + try: + await uploaded.save(temp_audio_path) + default_asr_model_config = get_tenant_default_model_by_type( + current_user.id, LLMType.SPEECH2TEXT + ) + except Exception as e: + try: + os.remove(temp_audio_path) + except OSError: + logging.exception("Failed to clean up temp audio file") + return get_data_error_result(message=str(e)) asr_mdl = LLMBundle(current_user.id, default_asr_model_config) if not stream_mode: - text = asr_mdl.transcription(temp_audio_path) - try: - os.remove(temp_audio_path) - except Exception as e: - logging.error(f"Failed to remove temp audio file: {str(e)}") - return get_json_result(data={"text": text}) + try: + text = asr_mdl.transcription(temp_audio_path) + return get_json_result(data={"text": text}) + finally: + try: + os.remove(temp_audio_path) + except OSError: + logging.exception("Failed to clean up temp audio file")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/apps/restful_apis/chat_api.py` around lines 927 - 943, The temp audio file created at fd, temp_audio_path must be removed on all code paths; wrap the save, model resolution (get_tenant_default_model_by_type) and transcription (LLMBundle(...).transcription) in a try/finally so that os.remove(temp_audio_path) is executed in the finally block (and log failures to remove). Specifically, move the creation of LLMBundle and the call to asr_mdl.transcription(temp_audio_path) inside the try, and ensure any early returns (e.g., after catching exceptions from get_tenant_default_model_by_type) remove the temp file before returning; keep the existing non-stream behavior but guarantee cleanup on exceptions from uploaded.save, model resolution, or transcription.
🧹 Nitpick comments (1)
api/apps/restful_apis/chat_api.py (1)
154-167: Add logs around the auto-created completion session path.This helper introduces a new persistence flow, but it is completely silent. A small info/debug log with
chat_id, generatedsession_id, and failure reasons would make rollout/debugging much easier.As per coding guidelines, "
**/*.py: Add logging for new flows`."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/apps/restful_apis/chat_api.py` around lines 154 - 167, The _create_session_for_completion helper currently creates and persists a session silently; add logging around this flow by acquiring the module logger (logging.getLogger(__name__)) and log an INFO (or DEBUG) entry after ConversationService.save with chat_id and the generated session id (conv["id"]) and any relevant metadata (e.g., prologue or user_id), and when ConversationService.get_by_id returns not ok, log an ERROR that includes chat_id, session id, and the failure reason or exception details before raising LookupError; reference the _create_session_for_completion function and the ConversationService.save / ConversationService.get_by_id calls to locate where to insert these logs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@api/apps/restful_apis/chat_api.py`:
- Around line 1061-1063: Currently a new session is created and persisted
immediately via _create_session_for_completion (producing session_id) before
validating llm_id and before a successful model response; change this so you
only instantiate an in-memory session object (do not save) when branching to
_create_session_for_completion, validate llm_id and attempt the first model
call, and only call the persistence path that writes the "New session" record
(the code that currently uses conv.id / session_id) after the model response
succeeds; apply the same change to the other occurrences around the blocks
referenced (the similar calls at the same pattern near the later branches),
ensuring any user_id derivation (req.get("user_id", current_user.id)) is
preserved but persistence is deferred until success.
- Around line 1055-1064: When an existing session is loaded via
ConversationService.get_by_id, do not unconditionally overwrite conv.message
with req["messages"]; instead preserve the authoritative stored history and only
incorporate new turns from the request. Update the code around
ConversationService.get_by_id / conv.message so that when session_id is present
you: (a) if req["messages"] appears incremental (e.g., its last item(s) are not
in conv.message) append only the new messages to conv.message, or (b) if
req["messages"] appears to be a full history, first verify it shares the same
prefix as conv.message and only replace/extend when it is strictly longer and
consistent. For new sessions created via _create_session_for_completion keep the
existing behavior of setting conv.message from req["messages"]. Ensure
session_id, chat_id and conv.id references remain unchanged.
In `@docs/guides/chat/set_chat_variables.md`:
- Around line 73-89: Add a note to the /chat/completions example explaining that
the API returns a session_id on the first successful call and that callers must
capture and include that session_id in subsequent /chat/completions requests to
maintain multi-turn continuity (otherwise each call with only chat_id will start
a new session); reference the response field name session_id and the request
endpoint /chat/completions so readers know where to read and reuse it.
---
Outside diff comments:
In `@api/apps/restful_apis/chat_api.py`:
- Around line 927-943: The temp audio file created at fd, temp_audio_path must
be removed on all code paths; wrap the save, model resolution
(get_tenant_default_model_by_type) and transcription
(LLMBundle(...).transcription) in a try/finally so that
os.remove(temp_audio_path) is executed in the finally block (and log failures to
remove). Specifically, move the creation of LLMBundle and the call to
asr_mdl.transcription(temp_audio_path) inside the try, and ensure any early
returns (e.g., after catching exceptions from get_tenant_default_model_by_type)
remove the temp file before returning; keep the existing non-stream behavior but
guarantee cleanup on exceptions from uploaded.save, model resolution, or
transcription.
---
Nitpick comments:
In `@api/apps/restful_apis/chat_api.py`:
- Around line 154-167: The _create_session_for_completion helper currently
creates and persists a session silently; add logging around this flow by
acquiring the module logger (logging.getLogger(__name__)) and log an INFO (or
DEBUG) entry after ConversationService.save with chat_id and the generated
session id (conv["id"]) and any relevant metadata (e.g., prologue or user_id),
and when ConversationService.get_by_id returns not ok, log an ERROR that
includes chat_id, session id, and the failure reason or exception details before
raising LookupError; reference the _create_session_for_completion function and
the ConversationService.save / ConversationService.get_by_id calls to locate
where to insert these logs.
🪄 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: 5cd34e99-3b0b-4285-8feb-17b743deae06
📒 Files selected for processing (2)
api/apps/restful_apis/chat_api.pydocs/guides/chat/set_chat_variables.md
Comment on lines
+1055
to
+1064
| if session_id: | ||
| e, conv = ConversationService.get_by_id(session_id) | ||
| if not e: | ||
| return get_data_error_result(message="Session not found!") | ||
| if conv.dialog_id != chat_id: | ||
| return get_data_error_result(message="Session does not belong to this chat!") | ||
| else: | ||
| conv = _create_session_for_completion(chat_id, dia, req.get("user_id", current_user.id)) | ||
| session_id = conv.id | ||
| conv.message = deepcopy(req["messages"]) |
Contributor
There was a problem hiding this comment.
Keep the stored session history authoritative when session_id is present.
This loads the existing Conversation and then immediately replaces conv.message with req["messages"]. Any client that sends only the new turn—or sends a trimmed history—will silently erase persisted context and desync the saved references from the message list.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@api/apps/restful_apis/chat_api.py` around lines 1055 - 1064, When an existing
session is loaded via ConversationService.get_by_id, do not unconditionally
overwrite conv.message with req["messages"]; instead preserve the authoritative
stored history and only incorporate new turns from the request. Update the code
around ConversationService.get_by_id / conv.message so that when session_id is
present you: (a) if req["messages"] appears incremental (e.g., its last item(s)
are not in conv.message) append only the new messages to conv.message, or (b) if
req["messages"] appears to be a full history, first verify it shares the same
prefix as conv.message and only replace/extend when it is strictly longer and
consistent. For new sessions created via _create_session_for_completion keep the
existing behavior of setting conv.message from req["messages"]. Ensure
session_id, chat_id and conv.id references remain unchanged.
Comment on lines
+1061
to
+1063
| else: | ||
| conv = _create_session_for_completion(chat_id, dia, req.get("user_id", current_user.id)) | ||
| session_id = conv.id |
Contributor
There was a problem hiding this comment.
Don't persist a new session before the completion request is known-good.
A brand-new session is saved before llm_id validation and before the first successful model response is written back. Invalid model overrides or downstream chat failures will therefore leave behind empty "New session" records.
Also applies to: 1077-1079, 1098-1099, 1116-1117
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@api/apps/restful_apis/chat_api.py` around lines 1061 - 1063, Currently a new
session is created and persisted immediately via _create_session_for_completion
(producing session_id) before validating llm_id and before a successful model
response; change this so you only instantiate an in-memory session object (do
not save) when branching to _create_session_for_completion, validate llm_id and
attempt the first model call, and only call the persistence path that writes the
"New session" record (the code that currently uses conv.id / session_id) after
the model response succeeds; apply the same change to the other occurrences
around the blocks referenced (the similar calls at the same pattern near the
later branches), ensuring any user_id derivation (req.get("user_id",
current_user.id)) is preserved but persistence is deferred until success.
Comment on lines
73
to
89
| ```json {9} | ||
| curl --request POST \ | ||
| --url http://{address}/api/v1/chats/{chat_id}/completions \ | ||
| --url http://{address}/api/v1/chat/completions \ | ||
| --header 'Content-Type: application/json' \ | ||
| --header 'Authorization: Bearer <YOUR_API_KEY>' \ | ||
| --data-binary ' | ||
| { | ||
| "question": "xxxxxxxxx", | ||
| "chat_id": "{chat_id}", | ||
| "stream": true, | ||
| "messages": [ | ||
| { | ||
| "role": "user", | ||
| "content": "xxxxxxxxx" | ||
| } | ||
| ], | ||
| "style":"hilarious" | ||
| }' |
Contributor
There was a problem hiding this comment.
Document the session_id follow-up step here.
This example only sends chat_id, which means each call starts a brand-new session under the new /chat/completions flow. Please add a note that callers need to capture the returned session_id and send it on subsequent requests if they want multi-turn continuity.
💡 Suggested doc tweak
```json {9}
curl --request POST \
--url http://{address}/api/v1/chat/completions \
--header 'Content-Type: application/json' \
--header 'Authorization: Bearer <YOUR_API_KEY>' \
--data-binary '
{
"chat_id": "{chat_id}",
"stream": true,
"messages": [
{
"role": "user",
"content": "xxxxxxxxx"
}
],
"style":"hilarious"
}'+The first successful response returns a session_id. Reuse that session_id in later
+/chat/completions calls to continue the same conversation; otherwise a new session is created.
</details>
<!-- suggestion_start -->
<details>
<summary>📝 Committable suggestion</summary>
> ‼️ **IMPORTANT**
> Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
```suggestion
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/guides/chat/set_chat_variables.md` around lines 73 - 89, Add a note to
the /chat/completions example explaining that the API returns a session_id on
the first successful call and that callers must capture and include that
session_id in subsequent /chat/completions requests to maintain multi-turn
continuity (otherwise each call with only chat_id will start a new session);
reference the response field name session_id and the request endpoint
/chat/completions so readers know where to read and reuse it.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
api/apps/restful_apis/chat_api.py (3)
927-957:⚠️ Potential issue | 🟠 MajorAlways clean up
temp_audio_pathon every exit path.The file is only removed on the happy path for non-streaming requests and inside
event_stream()for streaming ones. If model lookup,LLMBundle(...), ortranscription(...)raises before those points, the temp file is leaked. Wrap the outer flow in atry/finallyso cleanup is guaranteed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/apps/restful_apis/chat_api.py` around lines 927 - 957, The temp file created at temp_audio_path must be removed on every exit path; wrap the outer flow (after tempfile.mkstemp and uploaded.save) in a try/finally that always attempts to delete temp_audio_path (use os.path.exists before os.remove to make deletion idempotent), so any exceptions from get_tenant_default_model_by_type, LLMBundle, transcription, or other early failures won't leak the file; keep the existing cleanup inside event_stream but make it tolerant of the file already being removed (i.e., check existence before removing) and reference temp_audio_path, get_tenant_default_model_by_type, LLMBundle, transcription, and event_stream when making the change.
1022-1029:⚠️ Potential issue | 🟠 MajorReject normalized histories that do not end with a user turn.
This filter can leave
msgempty or assistant-terminated, andasync_chat()asserts that the last message is from the user. Right now those requests turn into a 500 instead of a 4xx validation error.Suggested guard
for m in req["messages"]: if m["role"] == "system": continue if m["role"] == "assistant" and not msg: continue msg.append(m) + if not msg or msg[-1].get("role") != "user": + return get_data_error_result(message="`messages` must end with a user message.")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/apps/restful_apis/chat_api.py` around lines 1022 - 1029, After filtering normalized history into msg, add a validation that rejects requests where msg is empty or the last message's role is not "user" (msg[-1]["role"] != "user") and return a 4xx validation error instead of letting async_chat() hit its assertion; update the code around the msg construction and message_id extraction to check msg and the final role and raise/return a BadRequest/HTTP 400 with a clear message (e.g., "conversation must end with a user turn") so invalid histories are rejected early.
1083-1117:⚠️ Potential issue | 🟠 MajorPersist the session even when
llm_idis overridden.
is_embedded = bool(chat_model_id)makes both persistence paths skipConversationService.update_by_id(...)whenever the caller explicitly chooses a model. In that case the assistant reply/reference never lands in the session. Persistence should depend onconv is not None, not on whetherllm_idwas supplied.Suggested fix
- is_embedded = bool(chat_model_id) + should_persist_session = conv is not None stream_mode = req.pop("stream", True) @@ - if conv is not None and not is_embedded: + if should_persist_session: ConversationService.update_by_id(conv.id, conv.to_dict()) @@ - if conv is not None and not is_embedded: + if should_persist_session: ConversationService.update_by_id(conv.id, conv.to_dict())🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/apps/restful_apis/chat_api.py` around lines 1083 - 1117, The conversation persistence currently skips ConversationService.update_by_id when is_embedded = bool(chat_model_id) is true, causing sessions to not be saved if the caller supplied a model; change the update condition so persistence depends only on conv being present. In both places where ConversationService.update_by_id(conv.id, conv.to_dict()) is invoked (inside async def stream() after the async for loop and after the non-stream async for loop), remove the "and not is_embedded" check and call the update whenever conv is not None, keeping the existing variables chat_model_id, is_embedded, conv, and ConversationService references intact.
♻️ Duplicate comments (2)
api/apps/restful_apis/chat_api.py (2)
1055-1064:⚠️ Potential issue | 🟠 MajorDon't overwrite persisted session history with the request payload.
When
session_idis present, loading the storedConversationand then replacingconv.messagewithreq["messages"]lets trimmed or incremental clients erase authoritative history. Append only the new turns, or replace only after confirming the request is a consistent prefix extension.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/apps/restful_apis/chat_api.py` around lines 1055 - 1064, The code currently replaces persisted conversation history by setting conv.message = deepcopy(req["messages"]); instead, when session_id is provided, load the stored Conversation via ConversationService.get_by_id and either (a) verify that req["messages"] is a consistent prefix extension of conv.message (e.g., req_messages startswith stored messages) before replacing, or (b) append only the new turns from req["messages"] onto conv.message (compute the delta and extend conv.message) so authoritative history is preserved; update the logic around ConversationService.get_by_id, _create_session_for_completion, and the conv.message assignment to perform the prefix check or append rather than an unconditional overwrite.
154-167:⚠️ Potential issue | 🟠 MajorDelay persistence of completion-created sessions until the first answer succeeds.
This helper writes the
"New session"row immediately, but/chat/completionsnow calls it beforellm_idvalidation and before any model output is produced. Invalid overrides or downstream chat failures will leave orphaned empty sessions behind.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/apps/restful_apis/chat_api.py` around lines 154 - 167, The helper _create_session_for_completion currently persists a "New session" row immediately via ConversationService.save/get_by_id, causing orphaned sessions if llm_id validation or model output fails; change _create_session_for_completion to build and return the conversation object (with id from get_uuid(), dialog_id, name, initial message content from dialog.prompt_config, user_id, reference) without calling ConversationService.save/get_by_id, and update the caller of _create_session_for_completion (the /chat/completions flow) to call ConversationService.save (and then verify with ConversationService.get_by_id if needed) only after the LLM override validation and only after the first assistant response is successfully produced and persisted.
🧹 Nitpick comments (1)
api/apps/restful_apis/chat_api.py (1)
1017-1119: Add logs around the new completion flow branches.This endpoint now has chat-less completions, optional session bootstrap, model overrides, and two persistence paths, but only the exception path is logged. Please add debug/info logging for branch selection and session creation/persistence so this new flow is operable in production.
As per coding guidelines, "
**/*.py: Add logging for new flows".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/apps/restful_apis/chat_api.py` around lines 1017 - 1119, Add informational/debug logging to the new completion flow in session_completion: log when entering chat-less vs chat-backed flow (presence of chat_id), when session_id is provided vs created (reference _ensure_owned_chat, _create_session_for_completion), when model override is validated/applied (TenantLLMService.get_api_key and dia.llm_id assignment), which streaming mode was chosen (stream_mode/is_embedded), and when persistence happens (calls to ConversationService.update_by_id and when conv is modified/initialized). Place logs at key branch points: after evaluating chat_id/session_id, after creating or loading conv, after setting dia.llm_setting/llm_id, before starting streaming vs sync path, and right before each ConversationService.update_by_id to record session id and persistence path; use appropriate logging levels (info/debug) and include identifying values like chat_id, session_id, dia.id/tenant_id and chat_model_id.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@api/apps/restful_apis/chat_api.py`:
- Around line 979-982: The route and handler are misspelled as "recommandation";
rename the route path and function to use "recommendation" consistently: change
`@manager.route`("/chat/recommandation", ...) to
`@manager.route`("/chat/recommendation", ...) and rename the async function
recommandation() to recommendation(), updating any decorators above it
(login_required, `@validate_request`("question")) to remain attached; also search
for and update any internal references, imports, tests, or URL usage that call
recommandation to the new recommendation symbol so the router and callers remain
in sync.
---
Outside diff comments:
In `@api/apps/restful_apis/chat_api.py`:
- Around line 927-957: The temp file created at temp_audio_path must be removed
on every exit path; wrap the outer flow (after tempfile.mkstemp and
uploaded.save) in a try/finally that always attempts to delete temp_audio_path
(use os.path.exists before os.remove to make deletion idempotent), so any
exceptions from get_tenant_default_model_by_type, LLMBundle, transcription, or
other early failures won't leak the file; keep the existing cleanup inside
event_stream but make it tolerant of the file already being removed (i.e., check
existence before removing) and reference temp_audio_path,
get_tenant_default_model_by_type, LLMBundle, transcription, and event_stream
when making the change.
- Around line 1022-1029: After filtering normalized history into msg, add a
validation that rejects requests where msg is empty or the last message's role
is not "user" (msg[-1]["role"] != "user") and return a 4xx validation error
instead of letting async_chat() hit its assertion; update the code around the
msg construction and message_id extraction to check msg and the final role and
raise/return a BadRequest/HTTP 400 with a clear message (e.g., "conversation
must end with a user turn") so invalid histories are rejected early.
- Around line 1083-1117: The conversation persistence currently skips
ConversationService.update_by_id when is_embedded = bool(chat_model_id) is true,
causing sessions to not be saved if the caller supplied a model; change the
update condition so persistence depends only on conv being present. In both
places where ConversationService.update_by_id(conv.id, conv.to_dict()) is
invoked (inside async def stream() after the async for loop and after the
non-stream async for loop), remove the "and not is_embedded" check and call the
update whenever conv is not None, keeping the existing variables chat_model_id,
is_embedded, conv, and ConversationService references intact.
---
Duplicate comments:
In `@api/apps/restful_apis/chat_api.py`:
- Around line 1055-1064: The code currently replaces persisted conversation
history by setting conv.message = deepcopy(req["messages"]); instead, when
session_id is provided, load the stored Conversation via
ConversationService.get_by_id and either (a) verify that req["messages"] is a
consistent prefix extension of conv.message (e.g., req_messages startswith
stored messages) before replacing, or (b) append only the new turns from
req["messages"] onto conv.message (compute the delta and extend conv.message) so
authoritative history is preserved; update the logic around
ConversationService.get_by_id, _create_session_for_completion, and the
conv.message assignment to perform the prefix check or append rather than an
unconditional overwrite.
- Around line 154-167: The helper _create_session_for_completion currently
persists a "New session" row immediately via ConversationService.save/get_by_id,
causing orphaned sessions if llm_id validation or model output fails; change
_create_session_for_completion to build and return the conversation object (with
id from get_uuid(), dialog_id, name, initial message content from
dialog.prompt_config, user_id, reference) without calling
ConversationService.save/get_by_id, and update the caller of
_create_session_for_completion (the /chat/completions flow) to call
ConversationService.save (and then verify with ConversationService.get_by_id if
needed) only after the LLM override validation and only after the first
assistant response is successfully produced and persisted.
---
Nitpick comments:
In `@api/apps/restful_apis/chat_api.py`:
- Around line 1017-1119: Add informational/debug logging to the new completion
flow in session_completion: log when entering chat-less vs chat-backed flow
(presence of chat_id), when session_id is provided vs created (reference
_ensure_owned_chat, _create_session_for_completion), when model override is
validated/applied (TenantLLMService.get_api_key and dia.llm_id assignment),
which streaming mode was chosen (stream_mode/is_embedded), and when persistence
happens (calls to ConversationService.update_by_id and when conv is
modified/initialized). Place logs at key branch points: after evaluating
chat_id/session_id, after creating or loading conv, after setting
dia.llm_setting/llm_id, before starting streaming vs sync path, and right before
each ConversationService.update_by_id to record session id and persistence path;
use appropriate logging levels (info/debug) and include identifying values like
chat_id, session_id, dia.id/tenant_id and chat_model_id.
🪄 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: d8d4f1a3-094d-44eb-9bf0-5aced8936bf5
📒 Files selected for processing (1)
api/apps/restful_apis/chat_api.py
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #14229 +/- ##
==========================================
- Coverage 96.66% 95.97% -0.70%
==========================================
Files 10 10
Lines 690 695 +5
Branches 108 111 +3
==========================================
Hits 667 667
- Misses 8 11 +3
- Partials 15 17 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
api/apps/restful_apis/chat_api.py (1)
154-167: Add logging for session creation flow.Per coding guidelines, new flows should include logging. Consider adding a log statement when a session is created for completion to aid debugging and observability.
def _create_session_for_completion(chat_id, dialog, user_id): + logging.info("Creating session for completion: chat_id=%s, user_id=%s", chat_id, user_id) conv = { "id": get_uuid(), "dialog_id": chat_id,As per coding guidelines: "
**/*.py: Add logging for new flows"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/apps/restful_apis/chat_api.py` around lines 154 - 167, Add logging to the _create_session_for_completion flow: call the module logger (e.g., logger = logging.getLogger(__name__)) and emit an info/debug log after creating the conv dict and before/after ConversationService.save to record the new session id (get_uuid()), dialog_id and user_id, and emit an error log with the session id and returned error context if ConversationService.get_by_id returns not ok (or raise after logging). Also log the assistant prologue content in debug to aid observability. Ensure logs reference _create_session_for_completion, ConversationService.save and ConversationService.get_by_id so they are easy to trace.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@api/apps/restful_apis/chat_api.py`:
- Around line 154-167: Add logging to the _create_session_for_completion flow:
call the module logger (e.g., logger = logging.getLogger(__name__)) and emit an
info/debug log after creating the conv dict and before/after
ConversationService.save to record the new session id (get_uuid()), dialog_id
and user_id, and emit an error log with the session id and returned error
context if ConversationService.get_by_id returns not ok (or raise after
logging). Also log the assistant prologue content in debug to aid observability.
Ensure logs reference _create_session_for_completion, ConversationService.save
and ConversationService.get_by_id so they are easy to trace.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d5d97635-d871-40b0-9855-bb959a509bef
📒 Files selected for processing (4)
api/apps/restful_apis/chat_api.pysdk/python/ragflow_sdk/modules/session.pyweb/src/pages/next-chats/hooks/use-send-single-message.tsweb/src/pages/next-search/hooks.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- web/src/pages/next-chats/hooks/use-send-single-message.ts
1 task
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
2 participants
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.
What problem does this PR solve?
Refactor /api/v1/chats to be more RESTful.
Type of change