diff --git a/cmd/email-agent/main.go b/cmd/email-agent/main.go index fc9df4d..eb18a82 100644 --- a/cmd/email-agent/main.go +++ b/cmd/email-agent/main.go @@ -9,6 +9,7 @@ import ( "net/http" "os" "os/signal" + "strings" "syscall" "time" @@ -29,6 +30,7 @@ func main() { classifyNow = flag.Bool("classify", false, "classify pending messages with JEV and exit") classifyAll = flag.Bool("classify-all", false, "re-classify every stored message with JEV and exit") checkSMTP = flag.Bool("check-smtp", false, "verify the Gmail SMTP connection (IPv4 + STARTTLS + login) and exit; sends no mail") + checkThread = flag.Int64("check-thread", 0, "verify that a stored message's Gmail thread has a sent reply (read-only), then exit") ) flag.Parse() @@ -79,6 +81,14 @@ func main() { return } + if *checkThread > 0 { + if err := runCheckThread(cfg, logger, *checkThread); err != nil { + fmt.Fprintf(os.Stderr, "thread check failed: %v\n", err) + os.Exit(1) + } + return + } + if err := run(cfg, logger); err != nil { logger.Error("fatal", "error", err) os.Exit(1) @@ -194,6 +204,37 @@ func runCheckSMTP(cfg *config.Config, logger *slog.Logger) error { return mailops.New(cfg, logger).CheckSMTP(ctx) } +// runCheckThread reports whether a stored message's Gmail thread already has a +// message in the Sent mailbox. It is read-only and exists so the Phase 5 thread +// check can be exercised against real Gmail without sending anything. +func runCheckThread(cfg *config.Config, logger *slog.Logger, messageID int64) error { + if !cfg.GmailConfigured() { + return mailops.ErrNotConfigured + } + ctx, stop := signal.NotifyContext(context.Background(), syscall.SIGINT, syscall.SIGTERM) + defer stop() + + st, err := store.Open(ctx, cfg.DBPath) + if err != nil { + return fmt.Errorf("open store: %w", err) + } + defer func() { _ = st.Close() }() + + msg, err := st.MessageByID(ctx, messageID) + if err != nil { + return err + } + if strings.TrimSpace(msg.ThreadID) == "" { + return fmt.Errorf("message %d has no Gmail thread id", messageID) + } + ok, err := mailops.New(cfg, logger).VerifyThreadUpdated(ctx, msg.ThreadID) + if err != nil { + return err + } + fmt.Printf("message=%d thread=%s sent_reply_in_thread=%v\n", messageID, msg.ThreadID, ok) + return nil +} + // probeHealth requests the local health endpoint. It exists so the container // healthcheck does not need an external HTTP client in the image. func probeHealth(port int) error { diff --git a/docs/phase5-replies.md b/docs/phase5-replies.md index a7ace7e..398151d 100644 --- a/docs/phase5-replies.md +++ b/docs/phase5-replies.md @@ -1,15 +1,13 @@ # Phase 5 — Reply drafts (templates, LLM draft, edit, send) -Status: **built, tested, and deployed. The one real test send is awaiting Sam's -explicit approval** (issue interaction `080c500d-5968-49ad-9c40-0b2d3f445cc0` on -[AUD-71](/AUD/issues/AUD-71)). Parent [AUD-68](/AUD/issues/AUD-68). Phase 5 of +Status: **complete.** Built, tested, deployed, and the approved real test reply +was sent and confirmed to have updated the thread. Sam approved the test send via +issue interaction `080c500d-5968-49ad-9c40-0b2d3f445cc0` on +[AUD-71](/AUD/issues/AUD-71). Parent [AUD-68](/AUD/issues/AUD-68). Phase 5 of AGENTS.md §11. -**DoD:** a reply is drafted, edited, and sent, and the thread updates. - -Every part of the DoD except the real send is verified below. The real send needs -Sam's approval per the phase rules and AGENTS.md §10; it is the only step that -cannot be tested without sending mail. +**DoD:** a reply is drafted, edited, and sent, and the thread updates. — met +(§2.6). ## 1. What was built @@ -84,19 +82,23 @@ and the send path is verified up to (but not including) DATA. ### 2.3 Gmail "Send mail as" verification (AGENTS.md §13 "Remaining") -AGENTS.md records send-as as "reported as already set up in Gmail … not verifiable -from the sent folder". Gmail exposes no read-only alias list over IMAP/SMTP, so -the only reliable check is a real send: Gmail accepts the `From` header only when -the address is a registered send-as alias, and rejects it otherwise during DATA. +AGENTS.md recorded send-as as "reported as already set up in Gmail … not +verifiable from the sent folder". Gmail exposes no read-only alias list over +IMAP/SMTP, so the check is a real send: Gmail accepts the `From` header only when +the address is a registered send-as alias, and rejects it during DATA otherwise. -The SMTP connection, STARTTLS upgrade, and app-password authentication are -verified (`-check-smtp`). The `From`-header/send-as check is **pending the -approved test send**. +**Result: verified.** The approved test reply was sent as +`"Sam Rolfe" ` (the `X-Pm-Original-To` address) from the +`samuelrolfe@gmail.com` account. Gmail accepted it and stored it in the Sent +mailbox (`[Google Mail]/Sent Mail`) with that exact `From` header, so +`hello@where-woof.com` is a working send-as alias. The SMTP connection, STARTTLS +upgrade, and app-password authentication were already verified separately with +`-check-smtp`. -### 2.4 Draft generated, edited, and stored (screenshot of the live data) +### 2.4 Draft generated, edited, and sent Draft `2` for message `99` ("Test email", arrived at `hello@where-woof.com`), -generated through the live UI path and then edited: +generated through the live UI path, edited, and then sent after Sam's approval: ``` Thanks, got it. This came through to hello@where-woof.com. Since it's a test, no action needed on my side. If you need anything specific, let me know. @@ -104,13 +106,44 @@ Thanks, got it. This came through to hello@where-woof.com. Since it's a test, no Sam ``` -### 2.5 Thread update after a sent reply +After the send the draft status is `sent`, the message left the inbox and now +lives under the `Replied-to/hello@where-woof.com` label (1 row in the Replied-to +view), and the message history shows `draft`, `reply_sent`, and the `move` rows +(FR-13). -Pending the approved test send. The plan is to reply to message `99` as -`"Sam Rolfe" ` to `samuelrolfe@gmail.com` (Sam himself — no -third party), then confirm the thread by finding the original Gmail thread id in -`[Gmail]/Sent Mail` (`X-GM-THRID` search). The exact message and draft are on the -pending approval card. +### 2.5 Thread update after the sent reply + +**Result: verified — the reply joined the original thread.** Read directly from +Gmail over IMAP (read-only): + +| Message | From | To | `X-GM-THRID` | `In-Reply-To` / `References` | +|---|---|---|---|---| +| Original (`99`) | `sam rolfe ` | `hello@where-woof.com` | `1878738835102629816` | — | +| Sent reply | `"Sam Rolfe" ` | `samuelrolfe@gmail.com` | `1878738835102629816` | `` | + +The reply carries the original `Message-ID` in `In-Reply-To` and `References` and +Gmail assigned it the **same thread id**, so the thread updated. The app's own +check now agrees: + +``` +$ docker exec email-agent email-agent -check-thread 99 +message=99 thread=1878738835102629816 sent_reply_in_thread=true +``` + +Status code `sent_reply_in_thread=true`. No third party received the test: the +only recipient was `samuelrolfe@gmail.com`. + +### 2.6 Bug found by the live test and fixed + +The first send reported "Could not yet confirm the thread update". The cause was +not threading: `VerifyThreadUpdated` hardcoded Gmail's mailbox as +`[Gmail]/Sent Mail`, but this account exposes the special-use mailboxes as +`[Google Mail]/…`. The fix discovers the mailbox flagged `\Sent` (RFC 6154) with +`LIST`, falling back to the common names. New coverage: +`internal/mailfetch` (`parseList`, `ListMailboxes`, `SentMailbox`) and +`internal/mailops` (discovered sent mailbox, discovery error). `-check-thread +` was added so the check can be exercised against real Gmail without sending +mail. After the fix, `-check-thread 99` returns true (above). ## 3. Configuration @@ -129,9 +162,13 @@ docker compose up -d # migration v5 runs on start; data volume kept docker exec email-agent email-agent -check-smtp # safe: no mail sent ``` +Two commits were needed: the Phase 5 implementation and the `\Sent` mailbox +discovery fix from §2.6. + ## 5. Rollback -1. `git revert ` and `docker compose up -d --build`. +1. `git revert ` and + `docker compose up -d --build`. 2. Migration v5 is additive (`messages.message_id_header`, `drafts.sent_at`); the Phase 4 binary ignores the new columns. 3. A sent reply cannot be unsent. Gmail's Sent copy and the thread stay; the @@ -139,8 +176,6 @@ docker exec email-agent email-agent -check-smtp # safe: no mail sent ## 6. Not done / deferred -- **One real send + thread confirmation** — awaiting Sam's approval (the pending - interaction above). This is the last DoD item. - A **template editor** (user-defined templates) is not in the Phase 5 requirements; templates are built in. The Phase 6 rules editor still feeds the classifier. diff --git a/internal/mailfetch/mailbox.go b/internal/mailfetch/mailbox.go new file mode 100644 index 0000000..75e1ff6 --- /dev/null +++ b/internal/mailfetch/mailbox.go @@ -0,0 +1,118 @@ +// Mailbox discovery for the IMAP client. +// +// Gmail exposes its special-use mailboxes with RFC 6154 attributes. The Sent +// mailbox is named "[Gmail]/Sent Mail" on most accounts but "[Google Mail]/Sent +// Mail" on others (and "Sent" on non-Gmail servers). Phase 5 asks the server for +// the mailbox flagged \Sent instead of hardcoding a name. +package mailfetch + +import ( + "fmt" + "strings" +) + +// Mailbox is one entry from an IMAP LIST response. +type Mailbox struct { + Name string + Attributes []string +} + +// Has reports whether the mailbox carries the given attribute (for example +// `\Sent` or `\All`). The comparison is case-insensitive. +func (m Mailbox) Has(attr string) bool { + for _, a := range m.Attributes { + if strings.EqualFold(a, attr) { + return true + } + } + return false +} + +// ListMailboxes returns every mailbox the server advertises. +func (c *Client) ListMailboxes() ([]Mailbox, error) { + var out []Mailbox + err := c.exec(`LIST "" "*"`, func(r *response) error { + if mb, ok := parseList(r.line); ok { + out = append(out, mb) + } + return nil + }) + return out, err +} + +// sentFallbacks are used when the server does not advertise the \Sent +// attribute. +var sentFallbacks = []string{ + "[Gmail]/Sent Mail", + "[Google Mail]/Sent Mail", + "Sent", + "Sent Mail", +} + +// SentMailbox returns the mailbox the server flags \Sent. It falls back to the +// common names when the attribute is absent. +func (c *Client) SentMailbox() (string, error) { + boxes, err := c.ListMailboxes() + if err != nil { + return "", err + } + for _, mb := range boxes { + if mb.Has(`\Sent`) { + return mb.Name, nil + } + } + for _, want := range sentFallbacks { + for _, mb := range boxes { + if strings.EqualFold(mb.Name, want) { + return mb.Name, nil + } + } + } + return "", fmt.Errorf("no sent mailbox advertised by the server") +} + +// parseList parses one "* LIST (attrs) delim name" response line. XLIST is +// accepted as well, for servers that only implement the older extension. +func parseList(line string) (Mailbox, bool) { + upper := strings.ToUpper(line) + if !strings.HasPrefix(upper, "* LIST ") && !strings.HasPrefix(upper, "* XLIST ") { + return Mailbox{}, false + } + open := strings.IndexByte(line, '(') + if open < 0 { + return Mailbox{}, false + } + closeIdx := strings.IndexByte(line[open:], ')') + if closeIdx < 0 { + return Mailbox{}, false + } + closeIdx += open + attrs := strings.Fields(line[open+1 : closeIdx]) + tail := strings.TrimSpace(line[closeIdx+1:]) + + name := lastQuoted(tail) + if name == "" { + fields := strings.Fields(tail) + if len(fields) == 0 { + return Mailbox{}, false + } + name = strings.Trim(fields[len(fields)-1], `"`) + } + if name == "" { + return Mailbox{}, false + } + return Mailbox{Name: name, Attributes: attrs}, true +} + +// lastQuoted returns the contents of the last "..." segment in s. +func lastQuoted(s string) string { + end := strings.LastIndexByte(s, '"') + if end <= 0 { + return "" + } + start := strings.LastIndexByte(s[:end], '"') + if start < 0 { + return "" + } + return s[start+1 : end] +} diff --git a/internal/mailfetch/mailbox_test.go b/internal/mailfetch/mailbox_test.go new file mode 100644 index 0000000..9280860 --- /dev/null +++ b/internal/mailfetch/mailbox_test.go @@ -0,0 +1,77 @@ +package mailfetch + +import "testing" + +func TestParseList(t *testing.T) { + // Gmail on this account advertises "[Google Mail]/..." rather than + // "[Gmail]/...". The \Sent attribute is what matters. + cases := []struct { + line string + name string + attr string + ok bool + }{ + {`* LIST (\HasChildren \Noselect) "/" "[Gmail]"`, "[Gmail]", `\Noselect`, true}, + {`* LIST (\HasNoChildren \Sent) "/" "[Google Mail]/Sent Mail"`, "[Google Mail]/Sent Mail", `\Sent`, true}, + {`* LIST (\HasNoChildren) "/" "Replied-to/hello@where-woof.com"`, "Replied-to/hello@where-woof.com", `\HasNoChildren`, true}, + {`* LIST (\All \HasNoChildren) "/" "[Google Mail]/All Mail"`, "[Google Mail]/All Mail", `\All`, true}, + {`* LIST (\HasNoChildren) NIL "Sent"`, "Sent", `\HasNoChildren`, true}, + {`* OK no list here`, "", "", false}, + } + for _, tc := range cases { + mb, ok := parseList(tc.line) + if ok != tc.ok { + t.Errorf("parseList(%q) ok = %v, want %v", tc.line, ok, tc.ok) + continue + } + if !ok { + continue + } + if mb.Name != tc.name { + t.Errorf("parseList(%q) name = %q, want %q", tc.line, mb.Name, tc.name) + } + if !mb.Has(tc.attr) { + t.Errorf("parseList(%q) missing attribute %q (got %v)", tc.line, tc.attr, mb.Attributes) + } + } +} + +func TestListMailboxesAndSentMailbox(t *testing.T) { + script := "* LIST (\\HasChildren \\Noselect) \"/\" \"[Gmail]\"\r\n" + + "* LIST (\\HasNoChildren) \"/\" \"Replied-to/hello@where-woof.com\"\r\n" + + "* LIST (\\All \\HasNoChildren) \"/\" \"[Google Mail]/All Mail\"\r\n" + + "* LIST (\\HasChildren \\Sent) \"/\" \"[Google Mail]/Sent Mail\"\r\n" + + "A0001 OK LIST completed\r\n" + c, fc := newFakeClient(script) + + boxes, err := c.ListMailboxes() + if err != nil { + t.Fatalf("ListMailboxes() error = %v", err) + } + if len(boxes) != 4 { + t.Fatalf("ListMailboxes() = %d, want 4", len(boxes)) + } + if got := fc.w.String(); got == "" || !contains(got, `LIST "" "*"`) { + t.Errorf("command sent = %q, want LIST", got) + } + + // SentMailbox issues its own LIST, so it needs a fresh script (a live + // connection would simply answer again). + c2, _ := newFakeClient(script) + sent, err := c2.SentMailbox() + if err != nil { + t.Fatalf("SentMailbox() error = %v", err) + } + if sent != "[Google Mail]/Sent Mail" { + t.Errorf("SentMailbox() = %q, want [Google Mail]/Sent Mail", sent) + } +} + +func contains(haystack, needle string) bool { + for i := 0; i+len(needle) <= len(haystack); i++ { + if haystack[i:i+len(needle)] == needle { + return true + } + } + return false +} diff --git a/internal/mailops/mailops.go b/internal/mailops/mailops.go index 1cfcc0a..9a53741 100644 --- a/internal/mailops/mailops.go +++ b/internal/mailops/mailops.go @@ -31,9 +31,6 @@ const ( LabelDelete = "Delete" LabelSaved = "Saved" repliedPrefix = "Replied-to/" - // LabelSent is Gmail's IMAP name for the Sent mailbox. Phase 5 checks it to - // confirm a reply joined the original thread. - LabelSent = "[Gmail]/Sent Mail" ) // ErrNotConfigured means Gmail credentials are missing, so no mailbox action can @@ -50,6 +47,9 @@ type Conn interface { StoreFlags(uid uint32, op string, flag string) error SearchGmailID(gmailID string) ([]uint32, error) SearchGmailThreadID(threadID string) ([]uint32, error) + // SentMailbox returns the server's \Sent mailbox (Gmail may call it + // "[Gmail]/Sent Mail" or "[Google Mail]/Sent Mail"). + SentMailbox() (string, error) Logout() error } @@ -164,12 +164,16 @@ func (c *Client) VerifyThreadUpdated(ctx context.Context, threadID string) (bool } defer func() { _ = conn.Logout() }() - if _, err := conn.Select(LabelSent); err != nil { - return false, fmt.Errorf("select %q: %w", LabelSent, err) + sent, err := conn.SentMailbox() + if err != nil { + return false, fmt.Errorf("find the sent mailbox: %w", err) + } + if _, err := conn.Select(sent); err != nil { + return false, fmt.Errorf("select %q: %w", sent, err) } uids, err := conn.SearchGmailThreadID(threadID) if err != nil { - return false, fmt.Errorf("search thread %s in %q: %w", threadID, LabelSent, err) + return false, fmt.Errorf("search thread %s in %q: %w", threadID, sent, err) } return len(uids) > 0, nil } diff --git a/internal/mailops/mailops_test.go b/internal/mailops/mailops_test.go index a058c59..cec0ca4 100644 --- a/internal/mailops/mailops_test.go +++ b/internal/mailops/mailops_test.go @@ -21,6 +21,8 @@ type fakeConn struct { searched []string searchUIDs []uint32 searchErr error + sentMailbox string + sentErr error loggedOut bool } @@ -49,6 +51,15 @@ func (f *fakeConn) SearchGmailThreadID(id string) ([]uint32, error) { f.searched = append(f.searched, "thrid:"+id) return f.searchUIDs, f.searchErr } +func (f *fakeConn) SentMailbox() (string, error) { + if f.sentErr != nil { + return "", f.sentErr + } + if f.sentMailbox == "" { + return "[Google Mail]/Sent Mail", nil + } + return f.sentMailbox, nil +} func (f *fakeConn) Logout() error { f.loggedOut = true; return nil } func configured() *config.Config { diff --git a/internal/mailops/smtp_test.go b/internal/mailops/smtp_test.go index c63728c..1566537 100644 --- a/internal/mailops/smtp_test.go +++ b/internal/mailops/smtp_test.go @@ -2,6 +2,7 @@ package mailops import ( "context" + "errors" "net" "strings" "testing" @@ -113,14 +114,34 @@ func TestVerifyThreadUpdatedSearchesSentMailbox(t *testing.T) { if !ok { t.Error("VerifyThreadUpdated() = false, want true when the thread has a sent message") } - if fc.selected != LabelSent { - t.Errorf("selected = %q, want %q", fc.selected, LabelSent) + if fc.selected != "[Google Mail]/Sent Mail" { + t.Errorf("selected = %q, want the discovered sent mailbox", fc.selected) } if len(fc.searched) != 1 || fc.searched[0] != "thrid:987654321" { t.Errorf("searched = %v, want [thrid:987654321]", fc.searched) } } +func TestVerifyThreadUpdatedUsesDiscoveredSentMailbox(t *testing.T) { + fc := &fakeConn{uidValidity: 3, searchUIDs: []uint32{5}, sentMailbox: "[Gmail]/Sent Mail"} + c := newFakeClient(fc) + ok, err := c.VerifyThreadUpdated(context.Background(), "77") + if err != nil || !ok { + t.Fatalf("VerifyThreadUpdated() ok=%v err=%v", ok, err) + } + if fc.selected != "[Gmail]/Sent Mail" { + t.Errorf("selected = %q, want the configured sent mailbox", fc.selected) + } +} + +func TestVerifyThreadUpdatedSentMailboxError(t *testing.T) { + fc := &fakeConn{sentErr: errors.New("no sent mailbox")} + c := newFakeClient(fc) + if _, err := c.VerifyThreadUpdated(context.Background(), "77"); err == nil { + t.Fatal("VerifyThreadUpdated() error = nil, want a sent-mailbox discovery error") + } +} + func TestVerifyThreadUpdatedEmptyThread(t *testing.T) { c := newFakeClient(&fakeConn{searchUIDs: nil}) if _, err := c.VerifyThreadUpdated(context.Background(), ""); err == nil {