From b4e5919bb44398c97107c9fea3af6519be974403 Mon Sep 17 00:00:00 2001 From: Asim Aslam Date: Fri, 2 Oct 2026 10:37:06 +0100 Subject: [PATCH] Keep mail replies out of the IMAP conversation bridge --- agent/ask.go | 13 ------------- agent/mail/mail.go | 23 +++++++++-------------- inbox/imapbridge.go | 10 ++++++++++ inbox/imapbridge_test.go | 30 ++++++++++++++++++++++++++++++ service/mail/checkin_imap_test.go | 19 +++++++++++++++++++ service/mail/client.go | 17 ++++++++++++----- service/mail/outbox.go | 4 ++-- 7 files changed, 82 insertions(+), 34 deletions(-) diff --git a/agent/ask.go b/agent/ask.go index 3eb649fc4..3386607ef 100644 --- a/agent/ask.go +++ b/agent/ask.go @@ -469,19 +469,6 @@ func History(account, threadID string, max int) []QueryMessage { return out } -// Sent records the id a client's own protocol gave an answer, so a reply to it -// finds this conversation. Mail's Message-ID; nothing for a client without one. -func Sent(accountID, threadID, ref string) { - if threadID == "" || strings.TrimSpace(ref) == "" { - return - } - for _, m := range thread.Messages(accountID, threadID, 1) { - if m.Role == thread.RoleAgent { - thread.SetRef(accountID, m.ID, ref) - } - } -} - func threadID(th *thread.Thread) string { if th == nil { return "" diff --git a/agent/mail/mail.go b/agent/mail/mail.go index bd1453d81..280cdc7a2 100644 --- a/agent/mail/mail.go +++ b/agent/mail/mail.go @@ -11,6 +11,8 @@ import ( "strings" "time" + "github.com/google/uuid" + "mu/agent" "mu/internal/app" "mu/internal/event" @@ -231,10 +233,9 @@ func answerMail(m mail.InboundMail) { // the account has to be able to see that it happened. via := agent.Via{From: m.From} - // The conversation the answer will be recorded on, filled in once the run - // has happened. deliver closes over it so it can note the id the reply went - // out under, which is what the next message in the thread looks for. - var threadRef string + // Reserve the mail identity before recording the answer. The independent + // Inbox consumer can then project delivery without adding another turn. + answerRef := "<" + uuid.NewString() + ".reply@" + domain + ">" record := func(prompt, answer string, err error) string { return agent.Record(agent.Recorded{ @@ -284,15 +285,9 @@ func answerMail(m mail.InboundMail) { // every message from somebody writing to their own agent. to, cc := replyTo(m) plain = introduction(m.Owner, m, from) + plain - sent, err := mail.SendReplyAll(m.Owner, name, from, to, cc, subject, - plain, app.RenderString(plain), m.MessageID, m.References) - // The id the answer went out under, so the reply to *it* finds this - // turn. Recorded even when delivery failed below, because a message - // that reached the far side and then errored still gets answered. - // Against the conversation, which is what the next message looks - // in — the workflow record is how this answer was produced, not what - // was said. - agent.Sent(m.Owner, threadRef, sent) + _, err := mail.SendReplyAll(m.Owner, name, from, to, cc, subject, + plain, app.RenderString(plain), m.MessageID, m.References, answerRef) + if err != nil { app.Log("mail", "agent %s could not reply to %s: %v", name, m.From, err) // Recorded as an error against the run, because a reply that @@ -371,12 +366,12 @@ func answerMail(m mail.InboundMail) { As: from, Ref: m.InReplyTo + " " + m.References, MessageRef: m.MessageID, + AnswerRef: answerRef, From: m.From, FromName: m.FromName, Via: via, }) answer := res.Text - threadRef = res.Thread if err != nil { app.Log("mail", "agent %s failed on mail from %s: %v", name, m.From, err) deliver(res.Flow, prompt, "I could not answer that one. Try again, or ask a different way.") diff --git a/inbox/imapbridge.go b/inbox/imapbridge.go index f2877b82d..577416100 100644 --- a/inbox/imapbridge.go +++ b/inbox/imapbridge.go @@ -118,6 +118,16 @@ func asMessages(accountID string, t thread.Thread, domain string) []*mail.Messag } } + // Mail answers carry their sending address in From (AskRequest.As). + // They belong to the mail transport even before delivery is indexed; + // projecting them here exposes a second .conversation sender to IMAP. + if m.Role == thread.RoleAgent && m.Workflow != "" && strings.HasSuffix(strings.ToLower(m.From), "@"+strings.ToLower(domain)) { + if strings.HasPrefix(m.Ref, "<") && strings.HasSuffix(m.Ref, ">") { + prev = m.Ref + } + continue + } + one := &mail.Message{ Bridged: true, ID: id, diff --git a/inbox/imapbridge_test.go b/inbox/imapbridge_test.go index 5f19644cd..fff387ab1 100644 --- a/inbox/imapbridge_test.go +++ b/inbox/imapbridge_test.go @@ -35,3 +35,33 @@ func TestCheckinBridgeSkipsNativeMailAndKeepsReferences(t *testing.T) { t.Fatalf("bad bridge: %+v", got) } } + +func TestCheckinMailAnswerIsNotBridgedBeforeOrAfterDelivery(t *testing.T) { + const owner = "checkin-mail-answer" + defer thread.Forget(owner) + th := thread.Open(owner, thread.WebClient, "checkin:mail-answer") + from := "agent@" + mail.ConfiguredDomain() + ref := "" + answer := thread.Message{Account: owner, Thread: th.ID, Role: thread.RoleAgent, Text: "One answer", From: from, Ref: ref, Workflow: "mail-run"} + id := thread.Add(answer) + if got := Bridge(owner); len(got) != 0 { + t.Fatalf("mail answer bridged before delivery: %+v", got) + } + if err := mail.SendMessageTo(mail.Delivery{FromID: from, ToID: owner, Subject: "Re: Daily Checkin", Body: "One answer", MessageID: ref}); err != nil { + t.Fatal(err) + } + // The independent arrival consumer may run before the mail responder returns. + if added := thread.Add(thread.Message{Account: owner, Thread: th.ID, Role: thread.RolePerson, Text: "One answer", From: from, Ref: ref, Workflow: "mail-run"}); added != id { + t.Fatal("delivery recorded a second conversation turn") + } + if got := Bridge(owner); len(got) != 0 { + t.Fatalf("mail answer bridged after delivery: %+v", got) + } + // Old answers whose delivery raced the late reference update are suppressed too. + thread.Add(thread.Message{Account: owner, Thread: th.ID, Role: thread.RoleAgent, Text: "Older mail answer", From: from, Workflow: "older-mail-run"}) + thread.Add(thread.Message{Account: owner, Thread: th.ID, Role: thread.RoleAgent, Text: "One answer"}) + got := Bridge(owner) + if len(got) != 1 || got[0].Body != "One answer" || got[0].InReplyTo != ref { + t.Fatalf("lost genuine web answer or mail references: %+v", got) + } +} diff --git a/service/mail/checkin_imap_test.go b/service/mail/checkin_imap_test.go index aff5eeae1..98ed75065 100644 --- a/service/mail/checkin_imap_test.go +++ b/service/mail/checkin_imap_test.go @@ -46,3 +46,22 @@ func TestLocalSubmissionKeepsClientMessageID(t *testing.T) { t.Fatalf("client identity lost: %+v", stored) } } + +func TestMailReplyUsesReservedIdentity(t *testing.T) { + const ref = "" + body, id := buildExternalTo("Micro", "agent@example.test", "", "person@outside.test", nil, "Re: Checkin", "Answer", "

Answer

", "", "", ref) + if id != ref || !strings.Contains(string(body), "Message-ID: "+ref+"\r\n") { + t.Fatal("outbound reply replaced reserved identity") + } + const owner = "reserved-reply-owner" + auth.SetAccountForTest(&auth.Account{ID: owner, Approved: true}) + defer auth.RemoveAccountForTest(owner) + got, err := SendReplyAll(owner, "Micro", "agent@"+ConfiguredDomain(), owner, nil, "Re: Checkin", "Answer", "

Answer

", "", "", ref) + if err != nil { + t.Fatal(err) + } + stored := FindMessageByMessageID(ref) + if got != ref || stored == nil || stored.ToID != owner { + t.Fatalf("local reply replaced reserved identity: %q %+v", got, stored) + } +} diff --git a/service/mail/client.go b/service/mail/client.go index 9059e8f38..5eae0f08c 100644 --- a/service/mail/client.go +++ b/service/mail/client.go @@ -191,7 +191,7 @@ func buildExternal(displayName, from, replyTo, to, subject, bodyPlain, bodyHTML return buildExternalTo(displayName, from, replyTo, to, nil, subject, bodyPlain, bodyHTML, replyToMsgID, references) } -func buildExternalTo(displayName, from, replyTo, to string, cc []string, subject, bodyPlain, bodyHTML string, replyToMsgID, references string) ([]byte, string) { +func buildExternalTo(displayName, from, replyTo, to string, cc []string, subject, bodyPlain, bodyHTML string, replyToMsgID, references string, identity ...string) ([]byte, string) { // Extract username from email for Message-ID username := from if strings.Contains(from, "@") { @@ -200,6 +200,9 @@ func buildExternalTo(displayName, from, replyTo, to string, cc []string, subject // Generate unique Message-ID for threading messageID := fmt.Sprintf("<%d.%s@%s>", time.Now().UnixNano(), username, ConfiguredDomain()) + if len(identity) > 0 && identity[0] != "" && !strings.ContainsAny(identity[0], "\r\n") { + messageID = identity[0] + } // Generate boundary for multipart boundary := fmt.Sprintf("----=_Part_%d", time.Now().UnixNano()) @@ -499,7 +502,10 @@ func DKIMStatus() (enabled bool, domain, selector string) { // and it asks per recipient: a thread with one local person and one outside is // both, which is the case a single branch at the call site would get wrong. func SendReplyAll(fromID, displayName, from, to string, cc []string, subject, bodyPlain, bodyHTML, - inReplyTo, references string) (string, error) { + inReplyTo, references, messageID string) (string, error) { + if strings.ContainsAny(messageID, "\r\n") { + return "", fmt.Errorf("invalid Message-ID") + } var outside []string var here []string @@ -522,15 +528,16 @@ func SendReplyAll(fromID, displayName, from, to string, cc []string, subject, bo // this instance are still owed their copy — the relay being down is not // their problem, and returning early meant one bad address on a thread // silenced the answer for everybody on it. - var messageID string var relayErr error if len(outside) > 0 { id, err := queueReply(fromID, displayName, from, outside[0], outside[1:], subject, - bodyPlain, bodyHTML, inReplyTo, references) + bodyPlain, bodyHTML, inReplyTo, references, messageID) if err != nil { relayErr = err } - messageID = id + if id != "" { + messageID = id + } } // And everybody here, delivered rather than relayed. Each on their own, diff --git a/service/mail/outbox.go b/service/mail/outbox.go index a4d6b3973..f178620b6 100644 --- a/service/mail/outbox.go +++ b/service/mail/outbox.go @@ -39,8 +39,8 @@ type queuedMail struct { LastError string `json:"last_error,omitempty"` } -func queueReply(owner, display, from, to string, cc []string, subject, plain, html, parent, refs string) (string, error) { - message, id := buildExternalTo(display, from, "", to, cc, subject, plain, html, parent, refs) +func queueReply(owner, display, from, to string, cc []string, subject, plain, html, parent, refs string, identity ...string) (string, error) { + message, id := buildExternalTo(display, from, "", to, cc, subject, plain, html, parent, refs, identity...) return enqueueMail(owner, queuedMail{From: from, Subject: subject, MessageID: id, Message: signExternal(message), Recipients: append([]string{to}, cc...)}) }