feat(chat): make the thread part of reply affinity, not just the channel - #1259
feat(chat): make the thread part of reply affinity, not just the channel#1259stevegeek wants to merge 5 commits into
Conversation
…ssages handleAgentOutboundMessage's doc comment and an inline comment both state "recipient defaults to the agent's creator when not explicitly specified", but no such fallback existed — the function went straight from a failed explicit-recipient resolution to a 400 "recipient is required" error. This broke every automatic reply-forwarding path that doesn't set Recipient/RecipientID, including the assistant-reply Stop hook (pkg/sciontool/hooks/handlers/hub.go) used by both the web dashboard Messages tab and the Telegram plugin: an agent's reply would be captured successfully but silently fail to deliver with a 400 from the Hub. Add the missing fallback: when no recipient is given at all, resolve the agent's creator (falling back to owner) via the store and use their user record, mirroring the existing explicit-recipient resolution logic. Added TestOutboundMessage_ImplicitRecipientDefaultsToCreator to cover it.
Per gemini-code-assist review on this PR: the implicit-recipient fallback silently ignored any error from store.GetUser(), including a real backend failure (DB down, etc.), and fell through to the generic "recipient is required" 400. That mislabels a server-side problem as a client mistake. Distinguish store.ErrNotFound (creator/owner record legitimately missing — correct to fall through) from any other error (write a 500 via writeErrorFromErr, matching how errors are handled elsewhere in this file).
Reverts 8b14a92 and 97aaa92 from this branch. The maintainer's response on GoogleCloudPlatform#1230 was that the explicit path is preferred - errors early build better habits in agent messaging - and that with native chat becoming prominent the intended direction is for a channel+thread to be a valid recipient, so that an agent posts to a chat space rather than to a person. A creator fallback makes that target more ambiguous. The commits stay in this branch's history because the work that follows was built on top of them and the diagnosis in them was sound. Only the behaviour is removed, so the net change against main is the thread routing alone.
Reply affinity records which channel a user last spoke from, so an agent's untagged reply can be routed back to chat - and then has nothing to attach to, because a message needs a thread to land in a conversation. The reply arrives beside the conversation it answers rather than in it, which to whoever is watching the thread is indistinguishable from no reply at all. This is independent of who the reply is addressed to. An agent that names its recipient explicitly still cannot get its answer into the conversation the human is reading, because naming a person does not name a place. webchat_conversation_context now stores last_thread_id beside last_channel, and the affinity block in handleAgentOutboundMessage restores both. The thread is filled in only when the caller named none: an agent that addressed a thread explicitly has already said where its reply should go. Both write paths record it - the native web spoke from msg.ThreadID, and broker inbound from req.Message.ThreadID - so a channel that carries threads keeps them, and one that does not records an empty thread rather than inheriting a stale one. Adding the column is idempotent on both backends: postgres takes ADD COLUMN IF NOT EXISTS, and SQLite, which has no such form, treats a duplicate-column error as the success case. Existing rows get an empty thread and behave exactly as they do today until the user next speaks.
There was a problem hiding this comment.
Code Review
This pull request enhances the reply affinity mechanism by tracking and restoring both the channel and the thread ID of the last conversation context. It updates the WebChatStore interface, modifies the SQLite and PostgreSQL schemas to include a last_thread_id column, adds database migrations, and updates the messaging handlers and tests to support thread-level routing. I have no feedback to provide as there are no review comments.
|
This is definitely related to a rethink of what "recipient-less" or "thread-first" messaging shape should look like. Messaging has been a strength for scion, but started as a basic agent-agent form. There is some balance to strike here between the simplicity of "send "hello" to foo" as a simple agent-agent message in the same project, vs the full rich-context of compound message types (group message, mentions etc) into threads where you may want to be able to introspect who is getting the message. The pattern to avoid for now - is to have what is analogous to a "broadcast storm" - which I've seen happen if agents start using the --broadcast flag (which I'm considering a setting to disable at hub/project scopes) I need to put some cycles and thought into a full refactor of the messaging contract - there is some balance to strike between explicit and implicit. Happy to raise this in a discussion if you have some thoughts |
|
See #1264 for the proposed plan |
Reply affinity records which channel a user last spoke from, so an agent's untagged reply can be routed back to chat. It then has nothing to attach to, because a message needs a thread to land in a conversation. The reply arrives beside the conversation it answers rather than in it — which, to whoever is watching the thread, is indistinguishable from no reply at all.
This is independent of who the reply is addressed to. An agent that names its recipient explicitly still cannot get its answer into the conversation the human is reading, because naming a person does not name a place.
Scope
Deliberately only the where. The branch starts from #1230's commits so that work is credited in the history, then reverts the recipient fallback per the discussion there — so the net change against
mainleaves the recipient question exactly as it is today. It adds no implicit behaviour that wasn't already there — affinity has always routed untagged replies; it just routed them to a channel and then dropped them beside the conversation.If the outcome of #1230 is that agents must always name a recipient explicitly, this still applies: naming a recipient does not put the reply in the thread.
Why this might be worth having
Chat delivers to an agent with
channelandthread_idin the envelope, and the agent can answer withscion message --channel … --thread-id …. That works, and it is the right explicit path.But it only works if every agent knows to do it. An agent that doesn't reply into the thread doesn't fail loudly — it looks like an agent ignoring you, which is a confusing failure to debug from the outside. Recording the thread means the conversation holds together whether or not the agent is aware of it, while an agent that wants to route its reply somewhere else still can.
The change
webchat_conversation_contextstoreslast_thread_idbesidelast_channel, and the affinity block inhandleAgentOutboundMessagerestores both.The thread is filled in only when the caller named none. An agent that addressed a thread explicitly has already said where its reply should go, and this does not overrule it.
Both write paths record it — the native web spoke from
msg.ThreadID, and broker inbound fromreq.Message.ThreadID— so a channel that carries threads keeps them, and one that does not records an empty thread rather than inheriting a stale one. There is a test for that last case specifically: routing a Telegram reply into a leftover web thread would be worse than not routing it at all.Migration
Idempotent on both backends, run from
Initas the existing migrations are:ALTER TABLE … ADD COLUMN IF NOT EXISTS.Existing rows get an empty thread and behave exactly as they do today until the user next speaks, at which point the route is recorded and replies start landing in the conversation.
Testing
go test ./pkg/hub/ -run 'TestOutboundMessage_ReplyAffinity|TestWebChatStore|TestAddConversationContext'— passes. The handler test wires a webchat store and a recording spoke and asserts the route the handler chose: an untagged reply gets the channel and thread restored, and a thread the caller named is not overwritten. Mutation-checked — deleting the thread restore, or letting it overwrite an explicit thread, both fail the suite. Covers: an absent row yields an empty route rather than an error; channel and thread returned together; a threadless channel yields no thread; the migration is idempotent across repeatedInit.RecordChannelinsert/upsert tests extended to assert the thread moves with the channel — a reply must not be put into the thread of a conversation the user has since left.go build ./...clean.maininpkg/hub—TestBypassAgents_404Before403Ordering,TestClassifyPath_ManagedPath,TestClassifyPath_ManagedLegacyGroves,TestFSValidatePath_ManagedOverlap,TestFSList_HomeDir,TestFSList_DefaultsToHome. All reproduce on a clean checkout and look environment-dependent (HOME and managed-path assumptions). fix(hub): implement implicit recipient fallback for agent outbound messages #1230 reports five of these; the bypass-ordering one fails deterministically here too.