Repository navigation
fix(conversations): Keep the root actor when later turns record execution - #2436
Merged
Merged
Conversation
…tion Co-Authored-By: David Cramer <david@sentry.io>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
3 screenshot changes — 3 changed · 0 added · 0 removed Review screenshots in Frameshift
|
dcramer
approved these changes
Oct 30, 2026
sentry-junior Bot
added a commit
that referenced
this pull request
Oct 30, 2026
Before #2436 the coalesce write could also replace one human Identity with a later human Identity. A multi-author root with a user Identity today is then no evidence against the first-author rule. Run the agreement check and the repair only on roots with one author User, and count the rest.
sentry-junior Bot
added a commit
that referenced
this pull request
Oct 30, 2026
Event Messages and bot Messages have no author Identity today. An event Turn is how a root got the event system Identity before #2436, so requiring an author on every user Message would skip the damaged rows. Count authors only on human user Messages, as isEventConversationMessage and the isBot flag define them.
This branch was successfully deployed
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
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.
Refs #2431 (bug #2: a web conversation's root actor gets replaced by
Local CLI).Root cause
SqlStore.recordExecutionbuilt the conversation from the arguments of the current write only.recordActivityalready started from the saved row and usedmergeActor, butrecordExecutiondid not. Two callers send it per-Turn values:turn-cursor.tssendssource: "local"for any non-web Turn on a web conversation, because web conversations use alocaldestination.sessionLogActordrops non-Slack actors, so it also sends no actor.task-execution/store.ts(recordExecutionMetadata) sends the actor and source from the Redis work state. If that state has expired (7 days) and a watch event or a child result arrives, the work state is created again with sourceeventand no actor. Slack conversations hit this path too.With no actor,
actorIdentityForConversationinvented a system identity from that per-Turn source (Local CLIorEvent). The upsert then wrote it withcoalesce(excluded.actor_identity_id, existing), so it replaced the real root actor. Thesourcecolumn itself is set-once, so only the actor changed.What was wrong with the
local-clifallbackThe fallback is not wrong in itself. Real CLI conversations never store a user actor, so a
Local CLIsystem identity for a conversation that started aslocalis correct. The bug is that the fallback ran against the source of the current Turn, not the conversation's saved origin. A web conversation is neverlocalin SQL, but an event Turn on it lookslocalat the write site.Fix
recordExecutionnow takes the actor and source from the saved conversation first, the same wayrecordActivitydoes:mergeActor(existing.actor, args.actor)andexisting.source ?? args.source. All three callers ofupsertConversation(createChild,recordActivity,recordExecution) now build from the saved row, so this one change covers both execution callers and both Slack and web roots.Why not make
actor_identity_idset-once in the upsert: today a conversation that a system started (for example by the scheduler) gets its root actor upgraded when the first person replies, throughrecordActivityandmergeActor. A set-once column would change that. That is a separate product decision. This fix keeps existing behavior and stops the incorrect overwrite.Not changed
sessionLogActorstill drops non-Slack actors. With this fix, a missing actor no longer causes an overwrite. Passing web actors through can be done later if a write needs them.readConversationParticipants(api/conversations/participants.ts) still includes system identities. After this fix, a system identity is the root actor only for conversations that a system started (scheduler, event, real CLI). Hiding those from the avatar stack is a display decision, and the dashboard already falls back to the root actor when the list is empty.Existing data
This does not repair rows that are already wrong. A possible backfill, not run anywhere: for root conversations with
source = 'web'whoseactor_identity_idpoints at thejunior/local-clisystem identity, setactor_identity_idto the identity of the earliest usermessageevent injunior_conversation_eventsfor that conversation. Slack roots overwritten with thejunior/eventidentity can be repaired the same way whensource = 'slack'. Both queries are idempotent and change only rows whose actor is a system identity that does not match their source.via David Cramer.
--
View Session [Sentry]