Add persisted webchat conversations and extract initial API client package#487
Open
Add persisted webchat conversations and extract initial API client package#487
Conversation
Contributor
WalkthroughAdds a new typed api-client package, OpenAPI/typegen task, webchat conversation persistence and CRUD endpoints, schema/type updates, DB migration, LLM streaming and hook streaming changes, and related event types and client APIs. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
🚥 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. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
src/conversation/webchat.rs (1)
283-298: Don't synthesize required fields on row decode failures.Falling back to
""orUtc::now()for required columns turns bad rows or schema drift into valid-looking API payloads, which is harder to detect than surfacing the decode failure.As per coding guidelines:
src/**/*.rs: Don't silently discard errors. Nolet _ =on Results. Handle them, log them, or propagate them.Also applies to: 301-320
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/conversation/webchat.rs` around lines 283 - 298, Change row_to_conversation to propagate decoding errors instead of synthesizing defaults: make its signature return Result<WebChatConversation, sqlx::Error> (or appropriate error type) and replace all unwrap_or/_or_else uses with try_get(...) ? so any missing/invalid columns bubble up. Specifically update id, agent_id, title, title_source, archived (use try_get::<i64,_>("archived")? == 1), created_at and updated_at to use the ? operator on try_get, and then adjust callers of row_to_conversation to handle the Result; apply the same pattern to the similar decode function(s) around lines 301-320.packages/api-client/src/client.ts (1)
43-48: Don't forceContent-Typeon bodyless requests.
buildHeaders()unconditionally addsContent-Type: application/jsoneven for GET/DELETE calls. This triggers CORS preflight requests for cross-origin consumers—an easy miss when testing same-origin.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/api-client/src/client.ts` around lines 43 - 48, The buildHeaders function currently always sets "Content-Type": "application/json", which forces CORS preflights for bodyless requests; change buildHeaders (and all its call sites) to accept an explicit flag or infer from context (e.g., add an optional parameter like includeContentType or bodyProvided) and only add the Content-Type header when that flag is true (or when a body is present), while still conditionally adding Authorization using authToken and merging extra headers; update callers to pass the flag for POST/PUT/PATCH requests and omit it for GET/DELETE so GET/DELETE won't include Content-Type.src/api/webchat.rs (1)
51-57: Clamplimitin the handler instead of trusting downstream code.The route advertises a max of 500, but this forwards
query.limitstraight to the store. Enforcing the bound at the API boundary keeps the contract explicit and avoids a single request asking for an arbitrarily large page.Also applies to: 235-237
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/webchat.rs` around lines 51 - 57, Clamp the incoming WebChatConversationsQuery.limit at the API boundary instead of passing it straight to the store: where the handler reads the WebChatConversationsQuery (the function that receives query and calls the store with query.limit), replace use of query.limit with a clamped value (e.g. let limit = query.limit.clamp(1, 500) or std::cmp::min(query.limit, 500)) and pass that to the store; do the same for the other handler that uses the same query type (the similar use at the block referenced around lines 235-237). Ensure you still fall back to default_conversation_limit when query.limit is missing/zero if that behavior is desired.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/api-client/src/client.ts`:
- Around line 182-190: The helper listMemories(agentId: string, limit = 12, sort
= "recent") omits the required offset parameter from the request contract;
update the function signature (listMemories) to accept an offset (e.g., offset =
0 or offset?: number), include offset in the URLSearchParams alongside agent_id,
limit, and sort, and ensure the value is stringified (String(offset)) before
calling request<MemoriesListResponse>(`/agents/memories?...`) so the outgoing
request matches the published schema.
In `@src/api/webchat.rs`:
- Around line 126-128: The send handler currently uses
conversation_store(...).ensure(&request.agent_id, &request.session_id), which
recreates deleted conversations and lets clients reuse raw session IDs as
InboundMessage.conversation_id; change the logic in the /webchat/send flow (the
handler that calls conversation_store and ensure, and where
InboundMessage.conversation_id is set) to either (A) require the conversation to
already exist and return an error if missing, or (B) canonicalize/mint a
server-side conversation ID (matching create()’s portal:chat:{agent_id}:...
naming) before persisting the InboundMessage; stop calling store.ensure(...) for
incoming sends so deletion remains durable (also apply the same change to the
related block around the other occurrence at lines 142-154).
In `@src/conversation/webchat.rs`:
- Around line 186-199: Check ownership before removing message history: inside
the same transaction use webchat_conversations to verify the row exists and is
owned by agent_id (e.g., SELECT id FROM webchat_conversations WHERE agent_id = ?
AND id = ? FOR UPDATE or run the DELETE on webchat_conversations first and
inspect its affected_rows) before executing the DELETE FROM
conversation_messages for session_id. Concretely, use sqlx to query
webchat_conversations with agent_id and session_id (or run the DELETE on
webchat_conversations and check result.rows_affected()), return an
error/rollback if 0 rows affected, and only then proceed to delete from
conversation_messages and commit the tx (symbols: webchat_conversations,
conversation_messages, agent_id, session_id, tx, result).
- Around line 228-276: The backfill currently scans all legacy portal sessions
and assigns them to the caller's agent; update backfill_from_messages to
restrict the initial SELECT to only legacy channel_ids that belong to the
current agent (so we don't backfill other agents' sessions or do a full-table
scan). Concretely, change the query in backfill_from_messages that selects
DISTINCT channel_id from conversation_messages to include an agent filter—either
add "AND agent_id = ?" and bind(agent_id) if conversation_messages has an
agent_id column, or change the LIKE to bind a pattern that includes the agent id
(e.g. "channel_id LIKE 'portal:chat:{agent_id}:%'")—then proceed as before to
check existing via get and insert into webchat_conversations; this ensures
generate_title/default_title logic only runs for the caller's own legacy
sessions.
---
Nitpick comments:
In `@packages/api-client/src/client.ts`:
- Around line 43-48: The buildHeaders function currently always sets
"Content-Type": "application/json", which forces CORS preflights for bodyless
requests; change buildHeaders (and all its call sites) to accept an explicit
flag or infer from context (e.g., add an optional parameter like
includeContentType or bodyProvided) and only add the Content-Type header when
that flag is true (or when a body is present), while still conditionally adding
Authorization using authToken and merging extra headers; update callers to pass
the flag for POST/PUT/PATCH requests and omit it for GET/DELETE so GET/DELETE
won't include Content-Type.
In `@src/api/webchat.rs`:
- Around line 51-57: Clamp the incoming WebChatConversationsQuery.limit at the
API boundary instead of passing it straight to the store: where the handler
reads the WebChatConversationsQuery (the function that receives query and calls
the store with query.limit), replace use of query.limit with a clamped value
(e.g. let limit = query.limit.clamp(1, 500) or std::cmp::min(query.limit, 500))
and pass that to the store; do the same for the other handler that uses the same
query type (the similar use at the block referenced around lines 235-237).
Ensure you still fall back to default_conversation_limit when query.limit is
missing/zero if that behavior is desired.
In `@src/conversation/webchat.rs`:
- Around line 283-298: Change row_to_conversation to propagate decoding errors
instead of synthesizing defaults: make its signature return
Result<WebChatConversation, sqlx::Error> (or appropriate error type) and replace
all unwrap_or/_or_else uses with try_get(...) ? so any missing/invalid columns
bubble up. Specifically update id, agent_id, title, title_source, archived (use
try_get::<i64,_>("archived")? == 1), created_at and updated_at to use the ?
operator on try_get, and then adjust callers of row_to_conversation to handle
the Result; apply the same pattern to the similar decode function(s) around
lines 301-320.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 18735a7f-7689-4d74-aa2d-46a90ca7c563
⛔ Files ignored due to path filters (1)
packages/api-client/package.jsonis excluded by!**/*.json
📒 Files selected for processing (14)
docs/design-docs/api-client-package-followup.mdinterface/src/api/schema.d.tsinterface/src/api/types.tsjustfilemigrations/20260324000001_webchat_conversations.sqlpackages/api-client/src/client.tspackages/api-client/src/events.tspackages/api-client/src/index.tspackages/api-client/src/schema.d.tspackages/api-client/src/types.tssrc/api/server.rssrc/api/webchat.rssrc/conversation.rssrc/conversation/webchat.rs
Comment on lines
+182
to
+190
| listMemories(agentId: string, limit = 12, sort = "recent") { | ||
| const params = new URLSearchParams({ | ||
| agent_id: agentId, | ||
| limit: String(limit), | ||
| sort, | ||
| }); | ||
| return request<MemoriesListResponse>( | ||
| `/agents/memories?${params.toString()}`, | ||
| ); |
Contributor
There was a problem hiding this comment.
Include offset in listMemories().
The package re-exports interface/src/api/schema.d.ts, where list_memories still requires offset. This helper omits it, so callers can send a request that doesn't match the published contract.
Suggested fix
listMemories(agentId: string, limit = 12, sort = "recent") {
const params = new URLSearchParams({
agent_id: agentId,
limit: String(limit),
+ offset: "0",
sort,
});
return request<MemoriesListResponse>(
`/agents/memories?${params.toString()}`,
);📝 Committable suggestion
‼️ 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.
Suggested change
| listMemories(agentId: string, limit = 12, sort = "recent") { | |
| const params = new URLSearchParams({ | |
| agent_id: agentId, | |
| limit: String(limit), | |
| sort, | |
| }); | |
| return request<MemoriesListResponse>( | |
| `/agents/memories?${params.toString()}`, | |
| ); | |
| listMemories(agentId: string, limit = 12, sort = "recent") { | |
| const params = new URLSearchParams({ | |
| agent_id: agentId, | |
| limit: String(limit), | |
| offset: "0", | |
| sort, | |
| }); | |
| return request<MemoriesListResponse>( | |
| `/agents/memories?${params.toString()}`, | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/api-client/src/client.ts` around lines 182 - 190, The helper
listMemories(agentId: string, limit = 12, sort = "recent") omits the required
offset parameter from the request contract; update the function signature
(listMemories) to accept an offset (e.g., offset = 0 or offset?: number),
include offset in the URLSearchParams alongside agent_id, limit, and sort, and
ensure the value is stringified (String(offset)) before calling
request<MemoriesListResponse>(`/agents/memories?...`) so the outgoing request
matches the published schema.
Comment on lines
+126
to
+128
| let store = conversation_store(&state, &request.agent_id)?; | ||
| store | ||
| .ensure(&request.agent_id, &request.session_id) |
Contributor
There was a problem hiding this comment.
/webchat/send currently makes deletes reversible.
ensure(&request.agent_id, &request.session_id) recreates any missing row, and the same raw session_id is then reused as InboundMessage.conversation_id. That means a client can delete a conversation and resurrect it with one more send, while also bypassing the portal:chat:{agent_id}:... naming that create() uses for server-issued IDs. If deletion is meant to be durable, send should operate on an existing webchat conversation or canonicalize/mint the ID server-side before persisting it.
Also applies to: 142-154
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/api/webchat.rs` around lines 126 - 128, The send handler currently uses
conversation_store(...).ensure(&request.agent_id, &request.session_id), which
recreates deleted conversations and lets clients reuse raw session IDs as
InboundMessage.conversation_id; change the logic in the /webchat/send flow (the
handler that calls conversation_store and ensure, and where
InboundMessage.conversation_id is set) to either (A) require the conversation to
already exist and return an error if missing, or (B) canonicalize/mint a
server-side conversation ID (matching create()’s portal:chat:{agent_id}:...
naming) before persisting the InboundMessage; stop calling store.ensure(...) for
incoming sends so deletion remains durable (also apply the same change to the
related block around the other occurrence at lines 142-154).
Comment on lines
+186
to
+199
| sqlx::query("DELETE FROM conversation_messages WHERE channel_id = ?") | ||
| .bind(session_id) | ||
| .execute(&mut *tx) | ||
| .await | ||
| .map_err(|error| anyhow::anyhow!(error))?; | ||
|
|
||
| let result = sqlx::query("DELETE FROM webchat_conversations WHERE agent_id = ? AND id = ?") | ||
| .bind(agent_id) | ||
| .bind(session_id) | ||
| .execute(&mut *tx) | ||
| .await | ||
| .map_err(|error| anyhow::anyhow!(error))?; | ||
|
|
||
| tx.commit().await.map_err(|error| anyhow::anyhow!(error))?; |
Contributor
There was a problem hiding this comment.
Authorize the delete before removing message history.
This transaction deletes from conversation_messages using only session_id, then checks ownership in webchat_conversations. If the second delete affects 0 rows, the commit still goes through and the history for that channel is already gone.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/conversation/webchat.rs` around lines 186 - 199, Check ownership before
removing message history: inside the same transaction use webchat_conversations
to verify the row exists and is owned by agent_id (e.g., SELECT id FROM
webchat_conversations WHERE agent_id = ? AND id = ? FOR UPDATE or run the DELETE
on webchat_conversations first and inspect its affected_rows) before executing
the DELETE FROM conversation_messages for session_id. Concretely, use sqlx to
query webchat_conversations with agent_id and session_id (or run the DELETE on
webchat_conversations and check result.rows_affected()), return an
error/rollback if 0 rows affected, and only then proceed to delete from
conversation_messages and commit the tx (symbols: webchat_conversations,
conversation_messages, agent_id, session_id, tx, result).
Comment on lines
+228
to
+276
| async fn backfill_from_messages(&self, agent_id: &str) -> crate::error::Result<()> { | ||
| let rows = sqlx::query( | ||
| "SELECT DISTINCT channel_id FROM conversation_messages WHERE channel_id LIKE 'portal:chat:%'", | ||
| ) | ||
| .fetch_all(&self.pool) | ||
| .await | ||
| .map_err(|error| anyhow::anyhow!(error))?; | ||
|
|
||
| for row in rows { | ||
| let channel_id: String = row.try_get("channel_id").unwrap_or_default(); | ||
| if channel_id.is_empty() { | ||
| continue; | ||
| } | ||
|
|
||
| let existing = self.get(agent_id, &channel_id).await?; | ||
| if existing.is_some() { | ||
| continue; | ||
| } | ||
|
|
||
| let title = sqlx::query( | ||
| "SELECT content FROM conversation_messages \ | ||
| WHERE channel_id = ? AND role = 'user' \ | ||
| ORDER BY created_at ASC LIMIT 1", | ||
| ) | ||
| .bind(&channel_id) | ||
| .fetch_optional(&self.pool) | ||
| .await | ||
| .map_err(|error| anyhow::anyhow!(error))? | ||
| .and_then(|title_row| title_row.try_get::<String, _>("content").ok()) | ||
| .map(|content| generate_title(&content)) | ||
| .unwrap_or_else(default_title); | ||
|
|
||
| let title_source = if title == default_title() { | ||
| "system" | ||
| } else { | ||
| "user" | ||
| }; | ||
|
|
||
| sqlx::query( | ||
| "INSERT INTO webchat_conversations (id, agent_id, title, title_source) VALUES (?, ?, ?, ?) \ | ||
| ON CONFLICT(id) DO NOTHING", | ||
| ) | ||
| .bind(&channel_id) | ||
| .bind(agent_id) | ||
| .bind(&title) | ||
| .bind(title_source) | ||
| .execute(&self.pool) | ||
| .await | ||
| .map_err(|error| anyhow::anyhow!(error))?; |
Contributor
There was a problem hiding this comment.
Backfill only the current agent's legacy sessions.
SELECT DISTINCT channel_id ... LIKE 'portal:chat:%' pulls every legacy portal session, then inserts missing rows with the caller's agent_id. The first agent to hit list() can backfill another agent's conversation into its own namespace and expose the wrong preview/title. It also makes every list request do a full-table backfill pass.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/conversation/webchat.rs` around lines 228 - 276, The backfill currently
scans all legacy portal sessions and assigns them to the caller's agent; update
backfill_from_messages to restrict the initial SELECT to only legacy channel_ids
that belong to the current agent (so we don't backfill other agents' sessions or
do a full-table scan). Concretely, change the query in backfill_from_messages
that selects DISTINCT channel_id from conversation_messages to include an agent
filter—either add "AND agent_id = ?" and bind(agent_id) if conversation_messages
has an agent_id column, or change the LIKE to bind a pattern that includes the
agent id (e.g. "channel_id LIKE 'portal:chat:{agent_id}:%'")—then proceed as
before to check existing via get and insert into webchat_conversations; this
ensures generate_title/default_title logic only runs for the caller's own legacy
sessions.
![tembo[bot]](https://avatars.githubusercontent.com/in/1223475?s=60&v=4){kind=small}
| let store = conversation_store(&state, &request.agent_id)?; | ||
| store | ||
| .ensure(&request.agent_id, &request.session_id) | ||
| .await |
Contributor
There was a problem hiding this comment.
This makes /webchat/send return 500 if the conversation persistence write fails, even though the endpoint is meant to be fire-and-forget message injection. Consider making these writes best-effort (warn + continue) so transient SQLite issues don't block message delivery.
Suggested change
| .await | |
| let store = conversation_store(&state, &request.agent_id)?; | |
| if let Err(error) = store.ensure(&request.agent_id, &request.session_id).await { | |
| tracing::warn!(%error, session_id = %request.session_id, "failed to ensure webchat conversation"); | |
| } | |
| if let Err(error) = store | |
| .maybe_set_generated_title(&request.agent_id, &request.session_id, &request.message) | |
| .await | |
| { | |
| tracing::warn!(%error, session_id = %request.session_id, "failed to update generated webchat title"); | |
| } |
| limit: i64, | ||
| ) -> crate::error::Result<Vec<WebChatConversationSummary>> { | ||
| self.backfill_from_messages(agent_id).await?; | ||
|
|
Contributor
There was a problem hiding this comment.
list() currently calls backfill_from_messages() every time. If this endpoint is hit frequently, this can get expensive (distinct scan + per-conversation queries). Might be worth guarding it (one-time flag) or doing the backfill in a single INSERT ... SELECT query / migration.
|
|
||
| for row in rows { | ||
| let channel_id: String = row.try_get("channel_id").unwrap_or_default(); | ||
| if channel_id.is_empty() { |
Contributor
There was a problem hiding this comment.
unwrap_or_default() here will silently turn an unexpected row shape into an empty channel_id, which then just gets skipped. I'd rather surface the error so backfill failures are visible.
Suggested change
| if channel_id.is_empty() { | |
| let channel_id: String = row | |
| .try_get("channel_id") | |
| .map_err(|error| anyhow::anyhow!(error))?; |
| function buildHeaders(extra?: HeadersInit): HeadersInit { | ||
| return { | ||
| "Content-Type": "application/json", | ||
| ...(authToken ? { Authorization: `Bearer ${authToken}` } : {}), |
Contributor
There was a problem hiding this comment.
Minor browser ergonomics: always sending Content-Type: application/json (even on GET) can trigger an extra CORS preflight and adds a header that's not needed. Also, surfacing the error body makes debugging much easier.
Suggested change
| ...(authToken ? { Authorization: `Bearer ${authToken}` } : {}), | |
| function buildHeaders(extra?: HeadersInit, hasBody = false): Headers { | |
| const headers = new Headers(extra); | |
| if (authToken) { | |
| headers.set("Authorization", `Bearer ${authToken}`); | |
| } | |
| if (hasBody && !headers.has("Content-Type")) { | |
| headers.set("Content-Type", "application/json"); | |
| } | |
| return headers; | |
| } | |
| async function request<T>(path: string, init?: RequestInit): Promise<T> { | |
| const response = await fetch(`${getApiBase()}${path}`, { | |
| ...init, | |
| headers: buildHeaders(init?.headers, Boolean(init?.body)), | |
| }); | |
| if (!response.ok) { | |
| const errorText = await response.text().catch(() => ""); | |
| throw new Error( | |
| `Spacebot API error: ${response.status}${errorText ? ` - ${errorText}` : ""}`, | |
| ); | |
| } | |
| return response.json() as Promise<T>; | |
| } |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
src/llm/model.rs (2)
976-983: Consider flushing pending tool calls at stream end for resilience.Unlike
stream_openai_chat_request(line 1462), this implementation doesn't flushpending_tool_callsat the end of the stream. While the Responses API sends explicitoutput_item.doneevents, a stream interruption could silently lose accumulated tool call arguments.Additionally, Line 976 has a redundant
.to_string()sinceprovider_labelis already aStringfrom line 880.Suggested changes
- let provider_label = provider_label.to_string(); + let provider_label = provider_label; let stream = async_stream::stream! { let mut stream = response.bytes_stream(); let mut block_buffer = String::new(); let mut raw_text = String::new(); let mut sse_text = String::new(); let mut saw_data_event = false; let mut pending_tool_calls: std::collections::HashMap<String, OpenAiStreamingToolCall> = std::collections::HashMap::new();And after line 1083 (after processing remaining buffer), add a flush similar to
stream_openai_chat_request:// Flush any remaining pending tool calls (defensive against incomplete streams) for (_item_id, tool_call) in std::mem::take(&mut pending_tool_calls) { if tool_call.name.trim().is_empty() { continue; } if let Ok(arguments) = parse_streamed_tool_arguments(&tool_call.name, &tool_call.arguments) { yield Ok(RawStreamingChoice::ToolCall(RawStreamingToolCall { id: tool_call.id.clone(), internal_call_id: tool_call.internal_call_id, call_id: None, name: tool_call.name, arguments, signature: None, additional_params: None, })); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/llm/model.rs` around lines 976 - 983, The stream handler in stream_responses (the block initializing pending_tool_calls and variables) should flush any remaining pending_tool_calls at the end of the stream to avoid losing tool-call arguments on interrupted streams and to match stream_openai_chat_request's behavior; after the final buffer-processing section (after the code that consumes block_buffer/sse_text/raw_text), iterate over std::mem::take(&mut pending_tool_calls) and for each non-empty OpenAiStreamingToolCall call attempt to parse_streamed_tool_arguments and yield a RawStreamingToolCall item as done in stream_openai_chat_request; also remove the redundant provider_label.to_string() (since provider_label is already a String) to avoid an unnecessary clone.
4232-4310: Good test coverage for basic streaming events.The test validates text deltas, tool call name deltas, and argument deltas. Consider adding test cases for
OutputItemDonewithFunctionCall(which emits the finalToolCall) and reasoning events for more complete coverage.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/llm/model.rs` around lines 4232 - 4310, Add tests to exercise the finalization path and reasoning deltas: create additional OpenAiResponsesStreamingCompletionChunk cases that use OutputItemDone with OpenAiResponsesOutput::FunctionCall (ensure item_id and call_id match pending entries) and assert process_openai_responses_stream_event returns a RawStreamingChoice::ToolCall (final tool call) for that id; also add a case for a reasoning-style delta (e.g., an output text/reasoning chunk) and assert it is emitted as the expected RawStreamingChoice::Message or reasoning variant. Target the same test function or new tests that call process_openai_responses_stream_event and reference the concrete symbols OpenAiResponsesStreamingCompletionChunk, OpenAiResponsesOutput::FunctionCall, OutputItemDone, and RawStreamingChoice to locate the code paths to exercise.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/hooks/spacebot.rs`:
- Around line 474-482: The max-turns guard executes too late because
current_max_turns is incremented after the check, allowing two extra iterations;
fix by making the check reflect the post-increment value (either move the
current_max_turns += 1 above the if or change the condition to trigger when
current_max_turns >= max_turns) and raise PromptError::MaxTurnsError (including
max_turns, chat_history, prompt) when the new count would exceed the limit;
update the block around current_max_turns, max_turns, and
PromptError::MaxTurnsError accordingly so streaming enforces the same limit as
non-streaming.
- Around line 1097-1152: on_tool_call_delta is emitting incremental
ProcessEvent::TextDelta from reply content before the reply undergoes the same
secret/leak validation performed in on_tool_call/on_tool_result; this can leak
secrets token-by-token. Change on_tool_call_delta (and reuse the same reply
tracking state in reply_tool_delta_state) to either (A) run the identical
validation routine used by on_tool_result/on_tool_call on the accumulated
content (use Self::extract_partial_reply_content output) and only send the
TextDelta when that validation passes, or (B) buffer deltas locally (keep
updating state.raw_args and state.emitted_content) and do not call
self.event_tx.send(ProcessEvent::TextDelta{...}) until the final
on_tool_call/on_tool_result indicates the reply is validated; reference
functions/fields: on_tool_call_delta, on_tool_call, on_tool_result,
Self::extract_partial_reply_content, reply_tool_delta_state, state.raw_args,
state.emitted_content, and event_tx when making the change.
- Around line 515-535: The code currently forwards every
StreamedAssistantContent::Text chunk to SpacebotHook::on_text_delta immediately,
which can leak unvalidated/malformed tool output; buffer the incoming text
deltas inside the streaming loop instead of calling on_text_delta directly, and
only emit/forward them (call on_text_delta and/or send ProcessEvent::TextDelta)
after prompt_once_streaming() completes validation (and handle_agent_result()
checks) — if validation fails or a retry occurs, drop or sanitize the buffered
deltas so they are never sent; locate the streaming loop handling
StreamedAssistantContent::Text, the on_text_delta calls, and the
prompt_once_streaming()/handle_agent_result() paths to implement this
deferred-buffer-and-emit behavior.
- Around line 464-466: Reset the stream-local state (did_call_tool and
last_text_response) at the start of each outer iteration where current_max_turns
is (re)initialized so they don't carry over between attempts; specifically, move
or add reinitialization of did_call_tool = false and last_text_response.clear()
into the same loop/block that sets current_max_turns (the outer iteration that
drives turn attempts) and ensure the same change is applied to the other places
where current_max_turns is reinitialized later in the function so each attempt
starts with fresh values for did_call_tool and last_text_response.
---
Nitpick comments:
In `@src/llm/model.rs`:
- Around line 976-983: The stream handler in stream_responses (the block
initializing pending_tool_calls and variables) should flush any remaining
pending_tool_calls at the end of the stream to avoid losing tool-call arguments
on interrupted streams and to match stream_openai_chat_request's behavior; after
the final buffer-processing section (after the code that consumes
block_buffer/sse_text/raw_text), iterate over std::mem::take(&mut
pending_tool_calls) and for each non-empty OpenAiStreamingToolCall call attempt
to parse_streamed_tool_arguments and yield a RawStreamingToolCall item as done
in stream_openai_chat_request; also remove the redundant
provider_label.to_string() (since provider_label is already a String) to avoid
an unnecessary clone.
- Around line 4232-4310: Add tests to exercise the finalization path and
reasoning deltas: create additional OpenAiResponsesStreamingCompletionChunk
cases that use OutputItemDone with OpenAiResponsesOutput::FunctionCall (ensure
item_id and call_id match pending entries) and assert
process_openai_responses_stream_event returns a RawStreamingChoice::ToolCall
(final tool call) for that id; also add a case for a reasoning-style delta
(e.g., an output text/reasoning chunk) and assert it is emitted as the expected
RawStreamingChoice::Message or reasoning variant. Target the same test function
or new tests that call process_openai_responses_stream_event and reference the
concrete symbols OpenAiResponsesStreamingCompletionChunk,
OpenAiResponsesOutput::FunctionCall, OutputItemDone, and RawStreamingChoice to
locate the code paths to exercise.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5d17fb0b-0c5a-4060-b9aa-f543d3d0b051
📒 Files selected for processing (3)
src/agent/channel.rssrc/hooks/spacebot.rssrc/llm/model.rs
Comment on lines
+464
to
+466
| let mut current_max_turns = 0usize; | ||
| let mut last_text_response = String::new(); | ||
| let mut did_call_tool = false; |
Contributor
There was a problem hiding this comment.
Reset stream-local state inside each outer iteration.
did_call_tool and last_text_response currently survive across attempts, and later text/reasoning chunks can flip did_call_tool back to false after a tool call has already happened. That lets a later iteration return stale text instead of continuing from the tool results, or spin until MaxTurnsError if a post-tool completion comes back empty.
🐛 Suggested fix
- let mut current_max_turns = 0usize;
- let mut last_text_response = String::new();
- let mut did_call_tool = false;
+ let mut current_max_turns = 0usize;
loop {
+ let mut last_text_response = String::new();
+ let mut did_call_tool = false;
let current_prompt = chat_history
.last()
.cloned()
.expect("chat history should always include current prompt");
@@
StreamedAssistantContent::Text(text) => {
if !is_text_response {
last_text_response.clear();
is_text_response = true;
}
@@
- did_call_tool = false;
}
@@
- StreamedAssistantContent::Reasoning(_)
- | StreamedAssistantContent::ReasoningDelta { .. } => {
- did_call_tool = false;
- }
+ StreamedAssistantContent::Reasoning(_)
+ | StreamedAssistantContent::ReasoningDelta { .. } => {}
}
}Also applies to: 517-537, 652-655, 686-695
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/hooks/spacebot.rs` around lines 464 - 466, Reset the stream-local state
(did_call_tool and last_text_response) at the start of each outer iteration
where current_max_turns is (re)initialized so they don't carry over between
attempts; specifically, move or add reinitialization of did_call_tool = false
and last_text_response.clear() into the same loop/block that sets
current_max_turns (the outer iteration that drives turn attempts) and ensure the
same change is applied to the other places where current_max_turns is
reinitialized later in the function so each attempt starts with fresh values for
did_call_tool and last_text_response.
Comment on lines
+474
to
+482
| if current_max_turns > max_turns + 1 { | ||
| return Err(PromptError::MaxTurnsError { | ||
| max_turns, | ||
| chat_history: Box::new(chat_history), | ||
| prompt: Box::new(prompt.to_string().into()), | ||
| }); | ||
| } | ||
|
|
||
| current_max_turns += 1; |
Contributor
There was a problem hiding this comment.
max_turns = 5 still executes seven streaming cycles here.
The guard runs before current_max_turns += 1, so the loop only errors once it has already allowed two extra iterations. That makes the streaming path materially looser than the non-streaming path and widens runaway tool loops.
🐛 Suggested fix
- if current_max_turns > max_turns + 1 {
+ current_max_turns += 1;
+ if current_max_turns > max_turns {
return Err(PromptError::MaxTurnsError {
max_turns,
chat_history: Box::new(chat_history),
prompt: Box::new(prompt.to_string().into()),
});
}
-
- current_max_turns += 1;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/hooks/spacebot.rs` around lines 474 - 482, The max-turns guard executes
too late because current_max_turns is incremented after the check, allowing two
extra iterations; fix by making the check reflect the post-increment value
(either move the current_max_turns += 1 above the if or change the condition to
trigger when current_max_turns >= max_turns) and raise
PromptError::MaxTurnsError (including max_turns, chat_history, prompt) when the
new count would exceed the limit; update the block around current_max_turns,
max_turns, and PromptError::MaxTurnsError accordingly so streaming enforces the
same limit as non-streaming.
Comment on lines
+515
to
+535
| while let Some(content) = stream.next().await { | ||
| match content.map_err(PromptError::CompletionError)? { | ||
| StreamedAssistantContent::Text(text) => { | ||
| if !is_text_response { | ||
| last_text_response.clear(); | ||
| is_text_response = true; | ||
| } | ||
| last_text_response.push_str(&text.text); | ||
| if let HookAction::Terminate { reason } = | ||
| <SpacebotHook as PromptHook<M>>::on_text_delta( | ||
| self, | ||
| &text.text, | ||
| &last_text_response, | ||
| ) | ||
| .await | ||
| { | ||
| return Err(PromptError::PromptCancelled { | ||
| chat_history: Box::new(chat_history), | ||
| reason, | ||
| }); | ||
| } |
Contributor
There was a problem hiding this comment.
Don't publish raw text deltas before the turn is validated.
This forwards every streamed text chunk to on_text_delta() immediately, but the channel only decides whether that text is safe/usable after prompt_once_streaming() returns. In the tool-syntax recovery path in src/agent/channel.rs, malformed [reply] ... output will already have reached ProcessEvent::TextDelta consumers before the retry kicks in; the same race applies to secret-bearing plaintext that handle_agent_result() would later block. TextDelta is already consumed by the API/SSE layer, so this is outbound, not just telemetry.
As per coding guidelines, "Scan tool output and user input via SpacebotHook.on_tool_result() for leak detection (API keys, tokens, PEM keys). Block exfiltration before outbound HTTP."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/hooks/spacebot.rs` around lines 515 - 535, The code currently forwards
every StreamedAssistantContent::Text chunk to SpacebotHook::on_text_delta
immediately, which can leak unvalidated/malformed tool output; buffer the
incoming text deltas inside the streaming loop instead of calling on_text_delta
directly, and only emit/forward them (call on_text_delta and/or send
ProcessEvent::TextDelta) after prompt_once_streaming() completes validation (and
handle_agent_result() checks) — if validation fails or a retry occurs, drop or
sanitize the buffered deltas so they are never sent; locate the streaming loop
handling StreamedAssistantContent::Text, the on_text_delta calls, and the
prompt_once_streaming()/handle_agent_result() paths to implement this
deferred-buffer-and-emit behavior.
Comment on lines
+1097
to
+1152
| async fn on_tool_call_delta( | ||
| &self, | ||
| _tool_call_id: &str, | ||
| internal_call_id: &str, | ||
| tool_name: Option<&str>, | ||
| tool_call_delta: &str, | ||
| ) -> HookAction { | ||
| if self.process_type != ProcessType::Channel { | ||
| return HookAction::Continue; | ||
| } | ||
|
|
||
| let Some(channel_id) = self.channel_id.clone() else { | ||
| return HookAction::Continue; | ||
| }; | ||
|
|
||
| let mut guard = match self.reply_tool_delta_state.lock() { | ||
| Ok(guard) => guard, | ||
| Err(_) => return HookAction::Continue, | ||
| }; | ||
|
|
||
| let state = guard | ||
| .entry(internal_call_id.to_string()) | ||
| .or_insert_with(ReplyToolDeltaState::default); | ||
|
|
||
| if let Some(tool_name) = tool_name { | ||
| state.tool_name = Some(tool_name.to_string()); | ||
| } | ||
|
|
||
| if state.tool_name.as_deref() != Some("reply") { | ||
| return HookAction::Continue; | ||
| } | ||
|
|
||
| state.raw_args.push_str(tool_call_delta); | ||
| let Some(content) = Self::extract_partial_reply_content(&state.raw_args) else { | ||
| return HookAction::Continue; | ||
| }; | ||
|
|
||
| if !content.starts_with(&state.emitted_content) { | ||
| return HookAction::Continue; | ||
| } | ||
|
|
||
| let delta = &content[state.emitted_content.len()..]; | ||
| if delta.is_empty() { | ||
| return HookAction::Continue; | ||
| } | ||
|
|
||
| state.emitted_content = content.clone(); | ||
| self.event_tx | ||
| .send(ProcessEvent::TextDelta { | ||
| agent_id: self.agent_id.clone(), | ||
| process_id: self.process_id.clone(), | ||
| channel_id: Some(channel_id), | ||
| text_delta: delta.to_string(), | ||
| aggregated_text: content, | ||
| }) | ||
| .ok(); |
Contributor
There was a problem hiding this comment.
Validate reply content before emitting streamed deltas.
on_tool_call_delta() publishes partial reply.content as ProcessEvent::TextDelta before on_tool_call() / on_tool_result() run the existing reply secret checks. Because those events are forwarded by the API/SSE layer, a blocked reply can still leak to clients token-by-token. Either gate these deltas behind the same validation or keep them private until the full reply has passed validation.
As per coding guidelines, "Scan tool output and user input via SpacebotHook.on_tool_result() for leak detection (API keys, tokens, PEM keys). Block exfiltration before outbound HTTP."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/hooks/spacebot.rs` around lines 1097 - 1152, on_tool_call_delta is
emitting incremental ProcessEvent::TextDelta from reply content before the reply
undergoes the same secret/leak validation performed in
on_tool_call/on_tool_result; this can leak secrets token-by-token. Change
on_tool_call_delta (and reuse the same reply tracking state in
reply_tool_delta_state) to either (A) run the identical validation routine used
by on_tool_result/on_tool_call on the accumulated content (use
Self::extract_partial_reply_content output) and only send the TextDelta when
that validation passes, or (B) buffer deltas locally (keep updating
state.raw_args and state.emitted_content) and do not call
self.event_tx.send(ProcessEvent::TextDelta{...}) until the final
on_tool_call/on_tool_result indicates the reply is validated; reference
functions/fields: on_tool_call_delta, on_tool_call, on_tool_result,
Self::extract_partial_reply_content, reply_tool_delta_state, state.raw_args,
state.emitted_content, and event_tx when making the change.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/design-docs/conversation-settings.md`:
- Around line 41-51: Add explicit language identifiers to all fenced code blocks
to satisfy markdownlint MD040: annotate the "Agent Config (TOML)" block with
```toml or ```text, the "enum MemoryMode" block with ```rust, and similarly tag
other blocks with appropriate languages (e.g., ```typescript, ```json, ```sql)
so each fenced block in the document (including the blocks shown in the comment
and the other ranges you listed) begins with a language token after the opening
backticks.
- Around line 970-971: Change the sentence that claims "webchat" has no external
API consumers to a cautious phrasing like "assumed low external coupling" and,
when renaming the internal "webchat" identifier and related endpoints, preserve
backwards compatibility by adding alias endpoints and deprecation notices so
existing `@spacebot/api-client` consumers continue to work; update any
documentation text that asserts no external dependency and add a note to
maintain compatibility/aliasing for the rename.
🪄 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: 36a01c74-4bcd-46ed-ad74-3b5697e4da94
⛔ Files ignored due to path filters (5)
desktop/src-tauri/Cargo.lockis excluded by!**/*.lock,!**/*.lockdesktop/src-tauri/Cargo.tomlis excluded by!**/*.tomldesktop/src-tauri/gen/schemas/acl-manifests.jsonis excluded by!**/gen/**,!**/*.json,!**/gen/**desktop/src-tauri/gen/schemas/desktop-schema.jsonis excluded by!**/gen/**,!**/*.json,!**/gen/**desktop/src-tauri/gen/schemas/macOS-schema.jsonis excluded by!**/gen/**,!**/*.json,!**/gen/**
📒 Files selected for processing (1)
docs/design-docs/conversation-settings.md
Comment on lines
+41
to
+51
| ``` | ||
| Agent Config (TOML) | ||
| ├── routing: { channel, branch, worker, compactor, cortex } | ||
| │ ├── task_overrides: { "coding" → model } | ||
| │ └── fallbacks: { model → [fallback_models] } | ||
| ├── memory_persistence: { enabled, message_interval } | ||
| ├── working_memory: { enabled, context_token_budget, ... } | ||
| ├── cortex: { knowledge_synthesis (change-driven), maintenance, ... } | ||
| ├── channel_config: { listen_only_mode, save_attachments } | ||
| └── bindings[]: { channel, require_mention, ... } | ||
| ``` |
Contributor
There was a problem hiding this comment.
Add language identifiers to fenced code blocks (markdownlint MD040).
Multiple fenced blocks are missing a language tag, which will keep lint warnings noisy. Please annotate each block (e.g., text, rust, typescript, json, toml, sql).
Suggested pattern
-```
+```text
Agent Config (TOML)
...
-```
+```-```
+```rust
enum MemoryMode {
Full,
Ambient,
Off,
}
-```
+```Also applies to: 104-111, 254-275, 560-588, 769-773, 776-781, 785-795, 801-803, 807-826, 834-839, 852-860, 870-872, 903-912
🧰 Tools
🪛 markdownlint-cli2 (0.22.0)
[warning] 41-41: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/design-docs/conversation-settings.md` around lines 41 - 51, Add explicit
language identifiers to all fenced code blocks to satisfy markdownlint MD040:
annotate the "Agent Config (TOML)" block with ```toml or ```text, the "enum
MemoryMode" block with ```rust, and similarly tag other blocks with appropriate
languages (e.g., ```typescript, ```json, ```sql) so each fenced block in the
document (including the blocks shown in the comment and the other ranges you
listed) begins with a language token after the opening backticks.
Comment on lines
+151
to
+189
| #### `worker_context: WorkerContextMode` | ||
|
|
||
| ```rust | ||
| struct WorkerContextMode { | ||
| history: WorkerHistoryMode, // What conversation context the worker sees | ||
| memory: WorkerMemoryMode, // What memory context the worker gets | ||
| } | ||
|
|
||
| enum WorkerHistoryMode { | ||
| None, // No conversation history (current default) | ||
| Summary, // LLM-generated summary of recent conversation context | ||
| Recent(u32), // Last N messages from the parent conversation, injected into system prompt | ||
| Full, // Full conversation history clone (branch-style) | ||
| } | ||
|
|
||
| enum WorkerMemoryMode { | ||
| None, // No memory context (current default) | ||
| Ambient, // Knowledge synthesis + working memory injected into system prompt (read-only) | ||
| Tools, // Ambient context + memory_recall tool (can search but not write) | ||
| Full, // Ambient context + full memory tools (recall, save, delete) — branch-level access | ||
| } | ||
| ``` | ||
|
|
||
| This is the most important new axis. Today, the gap between "worker" and "branch" is a cliff — branches see everything, workers see nothing. Worker context settings turn this into a spectrum. | ||
|
|
||
| **`WorkerHistoryMode` options:** | ||
|
|
||
| - **None** — Current behavior. Worker sees only the task description. Channel must front-load all context into the task string. Cheapest, most isolated, fastest to start. | ||
| - **Summary** — Before spawning the worker, the channel generates a brief summary of the relevant conversation context and prepends it to the worker's system prompt. Cost: one extra LLM call at spawn time (can use a cheap model). Benefit: worker understands why the task exists without seeing the full transcript. | ||
| - **Recent(N)** — Last N conversation messages injected into the worker's system prompt as context (similar to how cortex chat injects channel context). No extra LLM call. Worker sees recent exchanges but not the full history. Good middle ground. | ||
| - **Full** — Worker receives a clone of the full channel history, same as a branch. Most expensive in tokens, but gives the worker complete conversational context. | ||
|
|
||
| **`WorkerMemoryMode` options:** | ||
|
|
||
| - **None** — Current behavior. No memory context, no memory tools. Worker is a pure executor. | ||
| - **Ambient** — Knowledge synthesis + working memory injected into the worker's system prompt. Worker has ambient awareness of what the agent knows — identity, facts, preferences, decisions, recent activity — but can't search or write. Read-only, no extra tools, minimal cost. | ||
| - **Tools** — Ambient context + `memory_recall` tool. Worker can actively search the agent's memory when it needs context. Cannot save new memories (that's still a branch concern). This is the "informed executor" mode. | ||
| - **Full** — Ambient context + full memory tools (recall, save, delete). Worker operates at branch-level memory access. Use sparingly — this blurs the worker/branch boundary. | ||
|
|
Contributor
There was a problem hiding this comment.
Worker context design currently contradicts the worker/branch boundary.
This section proposes injecting conversation history and memory access into workers (Summary, Recent, Full, Tools, Full), which collapses the existing contract that workers are task-isolated and context belongs to branches. Please either (a) scope these modes to branches, or (b) explicitly document this as a deliberate architecture change with rationale and migration risks.
Based on learnings: “Do not give workers channel context; workers receive only a fresh prompt and task description.”
Also applies to: 192-249
Comment on lines
+970
to
+971
| This rename is safe to do in a single commit. The "webchat" name is internal — no external API consumers depend on it (the Spacedrive interface uses the `@spacebot/api-client` package which abstracts the endpoints). | ||
|
|
Contributor
There was a problem hiding this comment.
The “no external API consumers” claim is unsafe for this PR trajectory.
Line 970 states the webchat naming is internal and has no external consumers, but this PR line of work introduces/extracts @spacebot/api-client for external consumption. Please reword this to “assumed low external coupling” and keep compatibility/alias endpoints during rename.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/design-docs/conversation-settings.md` around lines 970 - 971, Change the
sentence that claims "webchat" has no external API consumers to a cautious
phrasing like "assumed low external coupling" and, when renaming the internal
"webchat" identifier and related endpoints, preserve backwards compatibility by
adding alias endpoints and deprecation notices so existing `@spacebot/api-client`
consumers continue to work; update any documentation text that asserts no
external dependency and add a note to maintain compatibility/aliasing for the
rename.
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
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
webchat_conversationsmigration plus backfill logic so existing portal chat history can appear in the new conversation model@spacebot/api-clientpackage with fetch helpers, SSE event types, and shared API type exports for external consumersValidation
cargo test --workspace --quiet(fails in existing memory search tests because the embedding modelonnx/model.onnxcould not be retrieved; 752 tests passed, 4 failed)Note
Automated Summary (commit 05bbc07)
Adds comprehensive webchat conversation persistence with a SQLite-backed store. The new
WebChatConversationStoresupports CRUD operations, automatic title generation from first messages, archival state management, and summaries with message counts and previews. The TypeScript@spacebot/api-clientpackage provides a reusable client boundary with typed fetch helpers, SSE event streams, and re-exported schema types. A SQL migration handles backfilling existing portal chat sessions into the new conversation model. New API endpoints support listing conversations (with optional archival filtering), creating, updating (title/archived state), and deleting conversations by ID.Written by Tembo for commit 05bbc07. This will update automatically on new commits.