fix: persist agent and query-refusal chats to thread history via API#5080
Conversation
WorkspaceChat.new() was missing required fields in order to save the prompt/response correctly. Added `threadId` and `include: true`
shatfield4
left a comment
There was a problem hiding this comment.
Intended behavior here to not clutter the UI with API calls would be:
- make all API calls
include: falseto prevent them from showing in the UI - Have developers using the admin API persist context by using the
apiSessionId
There is a bug here in the apiChatHandler.js where chatSync and streamChat do not have the same behavior so be sure to resolve this so both functions set include: false always.
It looks like the main bug in here is that in recentChatHistory util function the query we do to get the context adds include: true which always returns nothing because we are filtering based on apiSessionId and include: true.
We need to change this so that when apiSessionId is provided to the recentChatHistory util, we remove the include: true from the query.
…persisted-to-thread-history-via-api
shatfield4
left a comment
There was a problem hiding this comment.
Code looks good here and I tested it thoroughly. A few things I found:
- We need to reject/throw and error if
userIdandsessionIdare in the same request (as discussed with Tim over slack) since there is no real use case for this - Noticed that
sessionIdis silently ignored in bothchatSyncandstreamChatthread endpoints
timothycarambat
left a comment
There was a problem hiding this comment.
Given the complexity and confusion around this issue in Slack, it would be the right move to build a test suite for this so that it is code-clear as to why things work the way they do and to also prevent regressions since the code-paths/logic for history loading via the API is confusing and sparsely documented.
If we have tests, we know exactly how the API chat system should work in all types of conditions and params.
| messageLimit = 20, | ||
| apiSessionId = null, | ||
| }) { | ||
| const where = { |
There was a problem hiding this comment.
This is a bad name, should just be something like conditions or something more clear imo
| // API chats using a sessionId are saved with include: false to keep them | ||
| // hidden from the UI. When fetching history for an API session, we omit | ||
| // the include filter so prior chats are still available for LLM context. | ||
| if (!apiSessionId) where.include = true; |
There was a problem hiding this comment.
Include include: true to keep compatibility with the existing code prior just to be sure we are covering cases we might not be aware of.
then if apiSessionId is not null we can do include: false.
Actually, to clear something up:
-
If a chat is sent with apiSessionId we hide it from the UI with include: false
-
If a chat is sent with same session id, even though it is false, we should pull the historical chats anyway so that chat history exists
-
What about people using sessionIDs right now where history is still true, but then upgrade. Their history is now reset for the same sessionID, correct?
In the above case, if that is correct, shouldnt we just omit/delete include totally from the query?
| if (userId && sessionId) { | ||
| response.status(400).json({ | ||
| id: uuidv4(), | ||
| type: "abort", | ||
| textResponse: null, | ||
| sources: [], | ||
| close: true, | ||
| error: | ||
| "Cannot pass both userId and sessionId. Use userId to chat as a user or sessionId for ephemeral sessions.", | ||
| }); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Make this a locally defined middleware check to the endpoint here to prevent code duplication since it is the same thing for both endpoints
Pull Request Type
Relevant Issues
resolves #5060
Description
API chats via
chatSyncandstreamChathad inconsistent behavior around UI visibility, thread association, and conversation history. This PR unifies both handlers so all code paths behave identically.Root causes:
chatSyncagent path was missingthreadIdanduser, so agent chats were saved as orphan records with no thread associationincludewas hardcoded inconsistently — some paths usedtrue, somefalse, some relied on the default (true). There was no unified logicrecentChatHistoryalways filteredinclude: true, which meant any chat saved withinclude: false(ephemeral session chats) was invisible to the LLM on subsequent calls — breaking conversation continuity forsessionId-based API usageChanges:
apiChatHandler.js— BothchatSyncandstreamChatnow deriveshouldIncludeInUI = !sessionIdat the top. EveryWorkspaceChats.newcall uses this value, and consistently passesthreadId,user, andapiSessionId.chats/index.js—recentChatHistoryno longer filters oninclude: truewhenapiSessionIdis present, so ephemeral session chats are still available as LLM context.Expected behavior with these changes:
UI Visibility
includesessionIdprovidedfalseuserIdprovided, nosessionIdtruetruetrueuser_idisnullso no user's frontend query matches itLLM Conversation History
sessionIdprovidedapi_session_idmatchuserIdprovided, nosessionIduser_id+thread_id+include: truethread_id+include: trueConsistency across handlers
chatSyncstreamChatshouldIncludeInUI,threadId✅,user✅shouldIncludeInUI,threadId✅,user✅shouldIncludeInUI,threadId✅,user✅shouldIncludeInUI,threadId✅,user✅shouldIncludeInUI,threadId✅,user✅shouldIncludeInUI,threadId✅,user✅shouldIncludeInUI,threadId✅,user✅shouldIncludeInUI,threadId✅,user✅Additional Information
stream.js) is unaffected — it does not go throughapiChatHandler.js/v1/workspace/:slug/chat) do not currently acceptuserIdin the request body —useris alwaysnull. Only thread-level endpoints supportuserId.Developer Validations
yarn lintfrom the root of the repo & committed changes