Chatbot switching to v1 responses API#1709
Open
HeljaeRaeisaenen wants to merge 56 commits into
Open
Conversation
…ram for both thinking and non-thinking models in new api
…umber tracking from chatbot backend
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
services/headless-lms/models/src/chatbot_conversation_message_tool_calls.rs (1)
115-142:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
get_by_message_idstill returns at most one row.Multiple
insertcalls for the samechatbot_conversation_message_idare still possible (an LLM response can produce parallel tool calls), but this function usesfetch_optionalandOption<ChatbotConversationMessageToolCall>, which silently drops all but one row when several exist. Either rename to make the singular contract explicit (and addLIMIT 1), or returnVec<ChatbotConversationMessageToolCall>withfetch_all.Suggested fix (return all matches)
pub async fn get_by_message_id( conn: &mut PgConnection, msg_id: Uuid, -) -> ModelResult<Option<ChatbotConversationMessageToolCall>> { +) -> ModelResult<Vec<ChatbotConversationMessageToolCall>> { let res = sqlx::query_as!( ChatbotConversationMessageToolCall, r#" SELECT id, created_at, updated_at, deleted_at, chatbot_conversation_message_id, tool_name, tool_arguments, tool_call_id, tool_kind as "tool_kind: ToolKind", response_id FROM chatbot_conversation_message_tool_calls WHERE chatbot_conversation_message_id = $1 AND deleted_at IS NULL "#, msg_id ) - .fetch_optional(conn) + .fetch_all(conn) .await?; Ok(res) }🤖 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 `@services/headless-lms/models/src/chatbot_conversation_message_tool_calls.rs` around lines 115 - 142, The function get_by_message_id currently returns Option<ChatbotConversationMessageToolCall> and uses fetch_optional, which silently drops extra rows; change it to return ModelResult<Vec<ChatbotConversationMessageToolCall>> and replace fetch_optional(conn).await? with fetch_all(conn).await? so all rows for chatbot_conversation_message_id are returned; update the function signature and any callers expecting an Option to handle a Vec instead (alternatively, if you want a single row contract, explicitly add LIMIT 1 to the SQL and keep the Option return type).services/headless-lms/chatbot/src/llm_utils.rs (1)
405-435:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
tool_kindis still not set explicitly, and the error message is outdated.Two leftover issues from the previous review:
- Both
Ok(...)arms (lines 413–418, 423–428) use..Default::default(), which setstool_kindto the struct's default (ToolKind::Function). TheAzureAiSearchCallOutputbranch therefore writes an Azure AI Search output back to the DB withtool_kind = Function, diverging from the siblingFromimpl above and fromto_chatbot_conversation_message.- The fallback error string on line 431 still says "APIMessage" (old type name) and "type is not OutputItem::FunctionCallOutput", but the match now also handles
AzureAiSearchCallOutput.Suggested fix
impl TryFrom<APIOutputMessage> for ChatbotConversationMessageToolOutput { type Error = ChatbotError; fn try_from(value: APIOutputMessage) -> ChatbotResult<Self> { match value.message_type { OutputItem::FunctionCallOutput { call_id, output, response_id, } => Ok(ChatbotConversationMessageToolOutput { output, tool_call_id: call_id, response_id, + tool_kind: ToolKind::Function, ..Default::default() }), OutputItem::AzureAiSearchCallOutput { response_id, call_id, output, } => Ok(ChatbotConversationMessageToolOutput { output, tool_call_id: call_id, response_id, + tool_kind: ToolKind::AzureAiSearch, ..Default::default() }), _ => Err(chatbot_err!( Other, - "Can't convert APIMessage to ChatbotConversationMessageToolOutput: APIMessage type is not OutputItem::FunctionCallOutput" + "Can't convert APIOutputMessage to ChatbotConversationMessageToolOutput: expected a FunctionCallOutput or AzureAiSearchCallOutput variant" )), } } }🤖 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 `@services/headless-lms/chatbot/src/llm_utils.rs` around lines 405 - 435, In the TryFrom<APIOutputMessage> for ChatbotConversationMessageToolOutput implementation, explicitly set the tool_kind field instead of relying on ..Default::default(): set tool_kind = ToolKind::Function in the OutputItem::FunctionCallOutput arm and tool_kind = ToolKind::AzureAiSearch in the OutputItem::AzureAiSearchCallOutput arm (keep output, tool_call_id, response_id as before), and update the Err message to reference APIOutputMessage and list both unhandled variants (e.g., "Can't convert APIOutputMessage to ChatbotConversationMessageToolOutput: message type is not FunctionCallOutput or AzureAiSearchCallOutput") to reflect the current match arms.
🧹 Nitpick comments (3)
services/main-frontend/src/app/manage/course-instances/[id]/certificates/page.tsx (1)
111-120: ⚡ Quick winClarify the relationship between the
filefield andbodySerializerparameter.There's an apparent inconsistency in how files are handled:
- Line 111-112:
filesarray may contain bothoverlaySvgandbackgroundSvg(0-2 files after filtering)- Line 118: The
filefield only includesfirstFile(the first of potentially two files) or an empty placeholder- Line 120: The
bodySerializerreceives the fullfilesarrayIf the
bodySerializerperforms the actual serialization and uses thefilesparameter, then thefilefield in the body might exist only to satisfy type requirements. However, this creates confusion about which data structure is actually sent to the API.Additionally, the empty
Filefallback (new File([], "empty-certificate-upload")) suggests the API requires a non-empty array even when no files are uploaded, which may warrant a comment explaining this requirement.Could you verify:
- Does the API now require
fileto be an array type?- Does
bodySerializeroverride the body completely, or does it usebody.file?- Should the
filefield contain all files (matching thefilesparameter), or is the current single-file approach correct?Consider adding a brief comment explaining the relationship between these fields if the current behavior is intentional.
🤖 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 `@services/main-frontend/src/app/manage/course-instances/`[id]/certificates/page.tsx around lines 111 - 120, The code currently builds files and firstFile and passes files into bodySerializer (createCertificateConfigurationFormData) while the body.file only contains firstFile or an empty placeholder; verify whether the API expects an array under the file key and whether updateCertificateConfiguration's bodySerializer fully replaces the body or reads body.file; if the API expects all uploaded files, change the body.file to mirror the files array (or populate placeholders to match required length) and/or update createCertificateConfigurationFormData to only use body.file consistently; if the current behavior is intentional, add a short inline comment near files, firstFile, updateCertificateConfiguration and createCertificateConfigurationFormData explaining that bodySerializer will be used for actual multipart serialization and why body.file contains only a single placeholder to satisfy typing/API requirements.services/headless-lms/chatbot/src/message_suggestion.rs (1)
194-228: ⚡ Quick winDead branches in the cutoff loop:
Message::ToolOutputand_are unreachable.
conversation(lines 184–190) is pre-filtered toMessage::Textonly, so theMessage::ToolOutputbranch on lines 205–209 and the fallback_arm on lines 210–214 can never execute. TheToolOutputtoken estimation, the "skip this element" fallback, and the// todo: which tool call was itnote are all dead code that only adds noise.Either drop the unreachable arms (simplest) or — if the intent is to also account for tool-call/tool-output tokens in the budget and include them in the rendered context — extend the pre-filter to include those variants and adjust
create_msg_stringcallers accordingly. Right now the budget math silently ignores tool-call/output tokens entirely.Suggested simplification (drop unreachable arms)
let cutoff = conversation .iter() .enumerate() // we want to take messages starting from the newest (=last) .rev() .map_while(|(idx, el)| { - match el.message.to_owned() { - Message::Text(msg) => { - // add this message's tokens and extra 5 tokens for newline and - // tag to used_tokens - used_tokens += msg.used_tokens + 5;} - Message::ToolOutput(o) => { - // add the tokens needed for the tool info to used_tokens - let s = format!("Output:\n{}\n\n", o.output); // todo: which tool call was it - used_tokens += estimate_tokens(&s); - } - _ => { - // if there is no message or tool output, skip this element. - // putting in conv_len won't affect the truncation later as we take min. - return Some(conv_len); - } - } + let Message::Text(msg) = &el.message else { + // `conversation` is pre-filtered to Text only; this is unreachable. + return Some(conv_len); + }; + // +5 for the role tag and surrounding newlines added by create_msg_string + used_tokens += msg.used_tokens + 5; if used_tokens > token_budget { return None; } // include this element's index as a potential cutoff point Some(idx) })🤖 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 `@services/headless-lms/chatbot/src/message_suggestion.rs` around lines 194 - 228, The cutoff loop over conversation contains unreachable arms because conversation is pre-filtered to only Message::Text; either remove the dead branches (Message::ToolOutput and the fallback _) and their token-estimation/comment noise so used_tokens/token_budget math only handles Message::Text, or update the pre-filtering that builds conversation to include Message::ToolOutput (and any other variants you want accounted for) and then update create_msg_string callers to accept and render those variants; locate the cutoff logic around the variable conversation, the match on Message::{Text, ToolOutput, _}, the used_tokens and token_budget variables, and either delete the ToolOutput/_ arms or expand upstream filtering and downstream create_msg_string handling so token accounting includes tool outputs.services/headless-lms/chatbot/src/llm_utils.rs (1)
720-731: 💤 Low valueHard-coded
"gpt-5.2-chat"string special-cases a single model.This bypasses
model.model_typeandconfiguration.reasoning_effortfor one specific deployment name, which is fragile (typos, deployment-name changes, multiple gpt-5.2 variants all silently fall through to the default branch). If gpt-5.2-chat really needseffort = Mediumandsummary = Detailed, it would be more robust to either (a) drive this frommodel.model_type(e.g., a newModelTypevariant or a configurableforce_medium_effortflag onChatbotConfigurationModel), or (b) compare against a defined constant rather than an inline literal.🤖 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 `@services/headless-lms/chatbot/src/llm_utils.rs` around lines 720 - 731, The get_params_for_model function currently special-cases the literal "gpt-5.2-chat"; instead, change it to use a stable discriminator (either check model.model_type for a new ModelType::Gpt5_2 variant or add a boolean flag on ChatbotConfigurationModel such as force_medium_effort) or compare against a shared constant (e.g., GPT_5_2_CHAT_DEPLOYMENT) rather than an inline string; update the branch that returns LLMRequestParams::GPTThinking(ThinkingParams { reasoning: ... }) to read the decision from model.model_type or the new flag (or constant) and preserve respect for configuration.reasoning_effort when the special-case is not set.
🤖 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 `@services/headless-lms/chatbot/src/llm_utils.rs`:
- Around line 113-142: The Azure AI Search branch in the Message::ToolOutput
match deserializes tool_output.output with serde_json::from_str, causing
asymmetric behavior versus ToolKind::Function; update the AzureAiSearch arm in
the Message::ToolOutput match so it assigns output: tool_output.output (same as
the Function branch) and remove the from_str call, leaving any JSON
deserialization to downstream consumers (e.g., where AiSearchOutput is parsed in
azure_chatbot.rs).
In
`@services/main-frontend/src/app/manage/course-instances/`[id]/certificates/page.tsx:
- Line 118: The certificate file payload was changed to send an array (see the
expression file: [firstFile ?? new File([], "empty-certificate-upload")] in
page.tsx) which looks unrelated to the chatbot v1 responses API migration;
confirm intent by either (A) keeping this change in the current PR only if you
can point to the related API/migration task and update the PR description/tests
to document the certificate API change and its backward compatibility, or (B)
revert this line to the previous single-file shape and move the array-based
certificate payload into a dedicated PR that updates the certificate upload API
and its consumers (update page.tsx, any form handlers, and backend API contract
together). Ensure you reference page.tsx and the form handler that consumes
"firstFile" when making the change so all call sites remain consistent.
---
Duplicate comments:
In `@services/headless-lms/chatbot/src/llm_utils.rs`:
- Around line 405-435: In the TryFrom<APIOutputMessage> for
ChatbotConversationMessageToolOutput implementation, explicitly set the
tool_kind field instead of relying on ..Default::default(): set tool_kind =
ToolKind::Function in the OutputItem::FunctionCallOutput arm and tool_kind =
ToolKind::AzureAiSearch in the OutputItem::AzureAiSearchCallOutput arm (keep
output, tool_call_id, response_id as before), and update the Err message to
reference APIOutputMessage and list both unhandled variants (e.g., "Can't
convert APIOutputMessage to ChatbotConversationMessageToolOutput: message type
is not FunctionCallOutput or AzureAiSearchCallOutput") to reflect the current
match arms.
In `@services/headless-lms/models/src/chatbot_conversation_message_tool_calls.rs`:
- Around line 115-142: The function get_by_message_id currently returns
Option<ChatbotConversationMessageToolCall> and uses fetch_optional, which
silently drops extra rows; change it to return
ModelResult<Vec<ChatbotConversationMessageToolCall>> and replace
fetch_optional(conn).await? with fetch_all(conn).await? so all rows for
chatbot_conversation_message_id are returned; update the function signature and
any callers expecting an Option to handle a Vec instead (alternatively, if you
want a single row contract, explicitly add LIMIT 1 to the SQL and keep the
Option return type).
---
Nitpick comments:
In `@services/headless-lms/chatbot/src/llm_utils.rs`:
- Around line 720-731: The get_params_for_model function currently special-cases
the literal "gpt-5.2-chat"; instead, change it to use a stable discriminator
(either check model.model_type for a new ModelType::Gpt5_2 variant or add a
boolean flag on ChatbotConfigurationModel such as force_medium_effort) or
compare against a shared constant (e.g., GPT_5_2_CHAT_DEPLOYMENT) rather than an
inline string; update the branch that returns
LLMRequestParams::GPTThinking(ThinkingParams { reasoning: ... }) to read the
decision from model.model_type or the new flag (or constant) and preserve
respect for configuration.reasoning_effort when the special-case is not set.
In `@services/headless-lms/chatbot/src/message_suggestion.rs`:
- Around line 194-228: The cutoff loop over conversation contains unreachable
arms because conversation is pre-filtered to only Message::Text; either remove
the dead branches (Message::ToolOutput and the fallback _) and their
token-estimation/comment noise so used_tokens/token_budget math only handles
Message::Text, or update the pre-filtering that builds conversation to include
Message::ToolOutput (and any other variants you want accounted for) and then
update create_msg_string callers to accept and render those variants; locate the
cutoff logic around the variable conversation, the match on Message::{Text,
ToolOutput, _}, the used_tokens and token_budget variables, and either delete
the ToolOutput/_ arms or expand upstream filtering and downstream
create_msg_string handling so token accounting includes tool outputs.
In
`@services/main-frontend/src/app/manage/course-instances/`[id]/certificates/page.tsx:
- Around line 111-120: The code currently builds files and firstFile and passes
files into bodySerializer (createCertificateConfigurationFormData) while the
body.file only contains firstFile or an empty placeholder; verify whether the
API expects an array under the file key and whether
updateCertificateConfiguration's bodySerializer fully replaces the body or reads
body.file; if the API expects all uploaded files, change the body.file to mirror
the files array (or populate placeholders to match required length) and/or
update createCertificateConfigurationFormData to only use body.file
consistently; if the current behavior is intentional, add a short inline comment
near files, firstFile, updateCertificateConfiguration and
createCertificateConfigurationFormData explaining that bodySerializer will be
used for actual multipart serialization and why body.file contains only a single
placeholder to satisfy typing/API requirements.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: fb912fdb-9e0c-4625-83b1-274306b62cd8
📒 Files selected for processing (19)
services/cms/Dockerfile.production.slim.dockerfileservices/example-exercise/Dockerfile.production.slim.dockerfileservices/headless-lms/Dockerfile.production.slim.dockerfileservices/headless-lms/chatbot/src/azure_chatbot.rsservices/headless-lms/chatbot/src/chatbot_tools/azure_ai_search.rsservices/headless-lms/chatbot/src/cms_ai_suggestion.rsservices/headless-lms/chatbot/src/content_cleaner.rsservices/headless-lms/chatbot/src/llm_utils.rsservices/headless-lms/chatbot/src/message_suggestion.rsservices/headless-lms/models/.sqlx/query-78dd0f61529b6c663bf4238ab900e22b20389cc416449368fa5010c38eee5ff5.jsonservices/headless-lms/models/.sqlx/query-9178bbe940ca7098383e31554c19b99698ded2a88ece5e20be1dec616c4e5857.jsonservices/headless-lms/models/.sqlx/query-e5846a7aac2d0349da510598d023fc965c33926e6637fe469064987cc9de4760.jsonservices/headless-lms/models/.sqlx/query-fc016c003a9005b0dbc87337502807a65bd0abd4e35a9e32899bf7333aad8d46.jsonservices/headless-lms/models/src/chatbot_conversation_message_tool_calls.rsservices/main-frontend/src/app/manage/course-instances/[id]/certificates/page.tsxservices/main-frontend/src/components/course-material/chatbot/shared/ChatbotChatBody.tsxservices/main-frontend/src/utils/course-material/createChatbotTranscript.tsservices/quizzes/Dockerfile.production.slim.dockerfileservices/tmc/Dockerfile.production.slim.dockerfile
✅ Files skipped from review due to trivial changes (8)
- services/cms/Dockerfile.production.slim.dockerfile
- services/quizzes/Dockerfile.production.slim.dockerfile
- services/headless-lms/Dockerfile.production.slim.dockerfile
- services/tmc/Dockerfile.production.slim.dockerfile
- services/example-exercise/Dockerfile.production.slim.dockerfile
- services/headless-lms/models/.sqlx/query-9178bbe940ca7098383e31554c19b99698ded2a88ece5e20be1dec616c4e5857.json
- services/headless-lms/models/.sqlx/query-fc016c003a9005b0dbc87337502807a65bd0abd4e35a9e32899bf7333aad8d46.json
- services/headless-lms/models/.sqlx/query-e5846a7aac2d0349da510598d023fc965c33926e6637fe469064987cc9de4760.json
🚧 Files skipped from review as they are similar to previous changes (6)
- services/main-frontend/src/components/course-material/chatbot/shared/ChatbotChatBody.tsx
- services/main-frontend/src/utils/course-material/createChatbotTranscript.ts
- services/headless-lms/chatbot/src/cms_ai_suggestion.rs
- services/headless-lms/chatbot/src/chatbot_tools/azure_ai_search.rs
- services/headless-lms/chatbot/src/content_cleaner.rs
- services/headless-lms/chatbot/src/azure_chatbot.rs
nygrenh
requested changes
May 21, 2026
| @@ -559,11 +529,12 @@ impl Drop for RequestCancelledGuard { | |||
| } | |||
|
|
|||
| pub async fn make_request_and_stream<'a>( | |||
Member
There was a problem hiding this comment.
add doc comment explaining the return values
| } | ||
| Some(Result::Ok(line)) => { | ||
| if !line.starts_with("data: ") { | ||
| if line.starts_with("event: ") { |
Member
There was a problem hiding this comment.
CHeck later if (event_type, data_line) tuple would work here
| /// For saving output items that are not text messages or function calls, i.e. that | ||
| /// don't need further processing and are not streamed to the user. | ||
| /// Saves reasoning and Azure AI items. | ||
| pub async fn process_output_item( |
Member
There was a problem hiding this comment.
Check how many queries calling this in loop makes
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.
The chatbot now uses the v1 responses API. All models can use tools and course material search. Different kinds of reasoning models are supported, but not all. Models that definitely work include GPT 5.2, GPT 5.4, GPT 4o. Models from other providers can be supported in the future.
Indicating the LLM is performing reasoning can now be implemented in the future, as well as indicating tool use.
Summary by CodeRabbit
New Features
Improvements