fix(hub): implement implicit recipient fallback for agent outbound messages - #1230
fix(hub): implement implicit recipient fallback for agent outbound messages#1230iJuanPablo wants to merge 2 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.
There was a problem hiding this comment.
Code Review
This pull request introduces fallback logic in handleAgentOutboundMessage to automatically resolve the recipient to the agent's creator (or owner) when no recipient is specified, accompanied by a new unit test to verify this behavior. The review feedback correctly points out that silently ignoring errors from s.store.GetUser can mask transient database issues as validation errors (400 Bad Request) instead of returning an internal server error (500), and suggests properly handling these errors.
| if u, err := s.store.GetUser(ctx, creatorID); err == nil { | ||
| recipientID = u.ID | ||
| name := u.DisplayName | ||
| if name == "" { | ||
| name = u.Email | ||
| } | ||
| recipient = "user:" + name | ||
| } |
There was a problem hiding this comment.
Silently ignoring all errors from s.store.GetUser can lead to misleading behavior. If a transient database error occurs (e.g., connection timeout), the function will silently fail to resolve the recipient and proceed to return a 400 Bad Request ("recipient is required") instead of a 500 Internal Server Error.
We should check if the error is a real database error (not store.ErrNotFound) and return a proper server error to the client.
u, err := s.store.GetUser(ctx, creatorID)
if err != nil {
if !errors.Is(err, store.ErrNotFound) {
writeErrorFromErr(w, err, "Failed to resolve agent creator")
return
}
} else {
recipientID = u.ID
name := u.DisplayName
if name == "" {
name = u.Email
}
recipient = "user:" + name
}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).
|
Thanks so much for your contribution - there are a couple reasons this may not be best way forward.
If there is a specific creator recipient - this makes a channel target a little more ambiguous - WDYT? which chat integration are you using the most? |
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.
|
I've been trying to work through this issue too, and understand the desire for it not to be implicit. I worked with Claude to try to solve this differently and have a possible proposal in #1259: rather than defaulting the recipient, it records the thread alongside the channel in the existing reply-affinity context, so an agent's untagged reply lands back in the conversation it answers, ie addressing the where rather than the who. |
|
See the proposed plan at #1264 |
Fixes #1229
Summary
handleAgentOutboundMessage's doc comment and an inline comment bothpromise that outbound messages default their recipient to the agent's
creator when none is explicitly specified, but that fallback was never
implemented — the function returned a 400
"recipient is required"immediately whenever both
recipientandrecipient_idwere empty.This silently broke every automatic reply-forwarding path, most notably the
assistant-reply Stop hook (
pkg/sciontool/hooks/handlers/hub.go) used byboth the web dashboard's Messages tab and the Telegram plugin: an agent's
final reply is captured by the hook and forwarded via
SendOutboundMessage(), which never setsRecipient/RecipientID, so theHub rejects it with 400 and the reply never reaches the user.
Fix
When no recipient is given at all, resolve
agent.CreatedBy(falling backto
agent.OwnerIDfor agents created by automation with no creator) viastore.GetUser, mirroring the existing explicit-recipient resolution logicjust above it (email lookup / display-name search /
"user:<name>"formatting).
Testing
TestOutboundMessage_ImplicitRecipientDefaultsToCreator— createsan agent with
CreatedByset and no recipient in the request, asserts a2xx response instead of the previous 400.
go build ./pkg/hub/...— clean.go test ./pkg/hub/... -run TestOutboundMessage— all pass, including thenew test and the existing
TestOutboundMessage_UnknownRecipient(explicitbad recipient still correctly rejected).
go test ./pkg/hub/...— same 5 pre-existing failures on unpatchedmain(TestClassifyPath_ManagedPath,TestClassifyPath_ManagedLegacyGroves,TestFSValidatePath_ManagedOverlap,TestFSList_HomeDir,TestFSList_DefaultsToHome— unrelatedfs_safety/filesystem tests,verified to fail identically with this change stashed out).