From 2420ea9c4bbac302741c33bed86030b96d7d057d Mon Sep 17 00:00:00 2001 From: TJ Baker <1617679+zaridan@users.noreply.github.com> Date: Sun, 2 Aug 2026 12:40:01 -0700 Subject: [PATCH 1/4] feat(mail): carry the provider's spam verdict through intake MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Gmail classifies every message before it reaches us, and we were throwing that verdict away. `history.list` is deliberately not label-filtered (filtering there races label application and would drop real mail), and the only label check downstream was the self-echo filter — so a message Google had already put in SPAM became an ordinary `active` conversation in the inbox. An operator who connects a Gmail mailbox reasonably expects Gmail's spam filtering to still apply. It did not. `RawInboundMessage.providerSpamVerdict` ('spam' | 'clean' | 'unknown') carries the transport's own conclusion across the provider boundary; `spamVerdictOf` derives it from Gmail's labels; ingest files a brand-new conversation as `spam` instead of `active` when it says so. Three properties are load-bearing and tested: - Nothing is dropped. A spam verdict changes one column. The message is parsed, stored, threaded and attachment-linked identically either way (inbound-ingestion.md §1 invariant #3), so a false positive is visible in the Spam folder and a reply reopens it. - An existing conversation is never re-filed. A reply that matched a valid reply token threads onto its target and leaves that target's status alone, however the provider classified it. Our token is the stronger signal, and an Actor's own placement is not overruled. - Only 'spam' is evidence. 'clean', 'unknown' and an absent field all mean active — the two are kept distinct on the wire because header scoring will need to tell "it said no" from "we don't know". Ordering note: the verdict is read only after the self-echo filter, so our own outbound reply that Gmail filed as junk is skipped entirely rather than filed as a spam conversation. IMAP is unaffected by construction — it opens INBOX, so a server-side Junk move means we never fetch the message. The asymmetry is now documented rather than accidental. specs/mail/spam-classification.md is the new spec. §3.1 and §4 are built here; header-derived signals, reclassification, and operator controls are specified but not built, and carry open decisions in its §7 ledger. Gates: tsc 0, biome 190 files clean, vitest 1757 pass / 0 fail. Co-Authored-By: Claude Opus 5 --- specs/api/agent-inbox-v1.md | 8 +- specs/mail/inbound-ingestion.md | 5 + specs/mail/spam-classification.md | 191 ++++++++++++++++++++++++++++++ src/mail/gmail-reconcile.test.ts | 102 ++++++++++++++++ src/mail/gmail-reconcile.ts | 36 +++++- src/mail/ingest.test.ts | 109 ++++++++++++++++- src/mail/ingest.ts | 38 +++++- src/providers/inbound-email.ts | 33 ++++++ src/providers/index.ts | 1 + src/store/conversations.ts | 20 +++- 10 files changed, 535 insertions(+), 8 deletions(-) create mode 100644 specs/mail/spam-classification.md diff --git a/specs/api/agent-inbox-v1.md b/specs/api/agent-inbox-v1.md index 8798b5c..23c2ad7 100644 --- a/specs/api/agent-inbox-v1.md +++ b/specs/api/agent-inbox-v1.md @@ -111,8 +111,12 @@ conversations `active`, and v1.0's `open` rows migrate to `active`. `pending` is statement that the conversation is parked awaiting something outside the inbox (a customer, a third party, a release); nothing sets it automatically in v1, and it still counts as open work (§3a). `closed` is resolved. `spam` is junk an Agent has thrown out -of the inbox; nothing classifies spam automatically in v1. Status pills in the UI: -Active = accent, Pending = warn, Closed = dim, Spam = critical. +of the inbox — **or, since 2026-08-02, junk a brand-new conversation was filed as at +intake when the transport's own classifier already called it spam** +([spam-classification.md](../mail/spam-classification.md) §4). That is the only automatic +source: nothing re-files an existing conversation, and no message is ever dropped rather +than filed. Status pills in the UI: Active = accent, Pending = warn, Closed = dim, +Spam = critical. **Snooze exception to "pending is never cleared automatically" (v1.1).** A snooze is a TIMED `pending` — `pending` plus a `snoozedUntil` timestamp (§4b) — and it is the diff --git a/specs/mail/inbound-ingestion.md b/specs/mail/inbound-ingestion.md index 8de2258..d529020 100644 --- a/specs/mail/inbound-ingestion.md +++ b/specs/mail/inbound-ingestion.md @@ -45,6 +45,11 @@ knows: - `providerMessageId` — the transport's own stable id for the message (for Gmail, the Gmail message id). This is the idempotency authority (§4), *not* the RFC `Message-ID`. - `receivedAt` — when the transport recorded delivery (not a header-parsed `Date`). +- `providerSpamVerdict` — optional; what the transport's own spam classifier already + concluded (`'spam' | 'clean' | 'unknown'`), carried through verbatim and never + re-derived here. It decides the status a *newly created* conversation is filed under + and nothing else — never whether the message is stored, which is invariant #3's to + answer, not a classifier's. See [spam-classification.md](./spam-classification.md). > **Correction.** The interface as first drafted returns a `NormalizedInboundEmail` > — headers and body already parsed, attachments already blob-referenced. That is wrong diff --git a/specs/mail/spam-classification.md b/specs/mail/spam-classification.md new file mode 100644 index 0000000..9cfeb68 --- /dev/null +++ b/specs/mail/spam-classification.md @@ -0,0 +1,191 @@ +# Spam classification + +**Status: PARTIALLY BUILT.** §3.1 and §4 are implemented and tested (the provider +verdict, Gmail's half of it, and the status decision at ingest). §3.2 (header-derived +signals), §5 (the reclassification loop) and §6 (operator controls) are specified but +**not built** — and §5.3 and §6 carry open decisions the maintainer has not made. See +§7 for the decision ledger. + +Companion to [inbound-ingestion.md](./inbound-ingestion.md), whose three invariants this +spec is subordinate to, and [threading.md](./threading.md), whose reply-token decision +always outranks anything here. Status vocabulary is +[agent-inbox-v1.md](../api/agent-inbox-v1.md) §3a. + +## 1. The problem this closes + +Before this spec, `spam` was a status an Actor set by hand and nothing else — as +agent-inbox-v1.md §3a put it: "nothing classifies spam automatically in v1." + +That was a deliberate v1 deferral, but it left a defect underneath it. The two intake +paths behaved differently, and neither behaviour was chosen: + +- **IMAP** opens `INBOX` only (`src/providers/adapters/imap/client.ts`). Whatever the + mail server filed as Junk was never fetched. Spam filtering by side effect. +- **Gmail** calls `history.list` with no `labelId` filter + (`src/providers/adapters/gmail/history.ts`) — correctly, because filtering there would + race the label application and drop real mail. But the only label check downstream was + the self-echo filter. **A message Google had already put in `SPAM` became an ordinary + `active` conversation in the inbox.** Google's verdict was computed, delivered to us, + and thrown away. + +An operator who connects a Gmail mailbox reasonably expects Gmail's spam filtering to +still apply. It did not. + +## 2. What this is not + +Helpthread does not build a spam classifier. Scoring message content is a large, +adversarial, permanently-maintained problem, and every mailbox we intake from is already +behind one. This spec is about **not discarding verdicts that already exist**, plus a small, +conservative set of header-derived signals for transports that supply no verdict. + +Three constraints bound everything below: + +1. **Nothing is ever dropped.** inbound-ingestion.md §1's third invariant is not + negotiable. A spam decision changes one column on one row; the message is parsed, + stored, threaded and attachment-linked identically either way. A false positive is + always visible in the Spam folder and always recoverable. +2. **Our own reply token always wins.** If `decideThreading` matched a valid token, the + message belongs to that conversation and the conversation's status is not this spec's + business (§4.2). +3. **An Actor's judgment is never silently overruled.** Automatic classification decides + where a *brand-new* conversation is filed. It never re-files a conversation a human + already placed. + +## 3. Signals + +### 3.1 The provider verdict (BUILT) + +`RawInboundMessage.providerSpamVerdict` (`src/providers/inbound-email.ts`) carries the +transport's own conclusion across the provider boundary. Three states: + +| Value | Meaning | +|---|---| +| `'spam'` | The provider affirmatively classified the message as junk. | +| `'clean'` | The provider classified it and did not call it junk. | +| `'unknown'` | No verdict available — the provider does not classify, or omitted it on this delivery. | + +Omitted entirely by a transport with no concept of a verdict; ingest reads an absent +field exactly as `'unknown'`. + +`'clean'` and `'unknown'` produce the same outcome today (§4.1) but are kept distinct on +the wire: "we asked and it said no" is evidence, "we have no idea" is not, and §3.2's +header signals will need to tell them apart — header scoring should defer to an explicit +`'clean'` and should not defer to silence. + +**Per transport:** + +- **Gmail** (built, `spamVerdictOf` in `src/mail/gmail-reconcile.ts`): the system `SPAM` + label ⇒ `'spam'`; any other non-empty label set ⇒ `'clean'`; an empty/absent `labelIds` + ⇒ `'unknown'`, because the history client documents that Gmail does not guarantee the + field is populated. This runs **after** the self-echo filter, so our own outbound reply + that Gmail happened to file as junk is skipped entirely rather than filed as a spam + conversation. +- **IMAP** (built by omission): the client opens `INBOX`, so a server-side Junk move + means the message is never fetched. The field is not supplied ⇒ `'unknown'`. This is + the correct verdict: we genuinely do not know, because we never saw the message at all. + Recorded here so the asymmetry with Gmail is documented rather than accidental. +- **A future forwarding-address transport**: whatever the receiving service reports. A + transport that reports nothing supplies nothing. + +### 3.2 Header-derived signals (NOT BUILT) + +For a `'unknown'` verdict on a message we did receive, a small conservative set of +RFC-defined headers, evaluated at ingest after `parseInboundEmail`: + +| Signal | Source | Weight | +|---|---|---| +| DMARC `fail` | `Authentication-Results` (RFC 7601), our own receiving hop only | Strong | +| SPF `fail` + DKIM `fail`, both | `Authentication-Results` | Strong | +| `Precedence: bulk` / `list` | RFC 2076 | Weak | +| `List-Unsubscribe` present **and** no prior conversation with this sender | RFC 8058 | Weak | +| `Auto-Submitted:` other than `no` | RFC 3834 | Weak — see below | + +Two rules make this safe: + +- **Only the receiving hop's `Authentication-Results` is trusted.** The header is + sender-forgeable; every hop before ours is attacker-controlled. A message with no + parseable `Authentication-Results` from our own hop contributes nothing, never a + failure. +- **One strong signal, or two weak ones, classifies as spam.** A single weak signal never + does. Legitimate mail is routinely `Precedence: bulk` (every receipt, every + notification), and a customer who genuinely writes from a mailing list address is not + junk. + +`Auto-Submitted` deserves a carve-out rather than a weight: an auto-reply is not spam, +it is a bounce or an out-of-office. Suppressing those is +inbound-ingestion.md §5's loop-guard problem, not this spec's, and the two must not be +conflated. Listed here only to record that it was considered and routed elsewhere. + +## 4. The decision (BUILT) + +### 4.1 New conversations + +At ingest, a brand-new conversation is created with `status = 'spam'` when the verdict is +`'spam'`, and `status = 'active'` otherwise (`'clean'`, `'unknown'`, absent). This is the +whole effect. `NewConversation.status` (`src/store/conversations.ts`) is narrowed to +exactly those two values: a conversation is never born `closed`, `pending`, or `deleted`. + +Both creation sites apply it — a genuine `new` threading decision, and the +deleted/not-found fallback where a valid token named a conversation that is gone. The +fallback case is flagged as an open decision (§7, D2). + +`conversation.created` and `conversation.message_received` fire for a spam conversation +exactly as for any other. A consumer that wants to ignore junk reads the status; events +are not silently withheld, because a webhook consumer that never learns a message +arrived cannot audit what we filed. + +### 4.2 Replies to existing conversations + +**No effect, ever.** A message that threads onto an existing conversation by valid reply +token leaves that conversation's status untouched, however the provider classified it. +Two reasons, either sufficient: our token is the stronger signal (we minted it, and it +proves we wrote to this address first), and the target's current status may be an Actor's +own deliberate placement. + +The existing reopen rule is unchanged and does the right thing here already: a reply to a +`spam` conversation reopens it to `active` (agent-inbox-v1.md §4a). A customer who +answers is a customer, whatever a classifier thought of the first message. + +## 5. Reclassification (NOT BUILT) + +### 5.1 The Actor's correction is the ground truth + +An Actor moving a conversation out of `spam` is the only correction signal that matters. +It is already recorded by `setStatus`. + +### 5.2 Sender allow-listing + +A sender an Actor has rescued from spam should not be re-classified on their next +message. The intended mechanism is per-mailbox sender state, not a global list. + +### 5.3 Feeding corrections back to the provider — OPEN + +Gmail's API can remove the `SPAM` label, which trains the operator's own filter. Doing so +means writing to the operator's mailbox in a way they did not directly ask for, on a +schedule they cannot see. **Not decided** (§7, D3). + +## 6. Operator controls (NOT BUILT) + +Whether automatic classification is on by default, and whether it can be turned off, is +**not decided** (§7, D1). The rest of the surface follows from that answer: a settings +toggle, and whether the Spam folder shows a "filed automatically" affordance distinct +from "an Actor filed this." + +## 7. Decision ledger + +Per the repo's provenance discipline, every normative choice above is marked with its +source. Anything marked **INFERRED** is the author's judgment and has not been approved. + +| # | Decision — in plain words | Source | +|---|---|---| +| — | Build the fix plus a spec for real auto-classification, rather than the fix alone | Maintainer, 2026-08-02: "b" (in reply to the (a) fix-only / (b) fix-plus-spec choice) | +| — | Spam is never auto-classified in v1 | Prior accepted spec, agent-inbox-v1.md §3a | +| D0 | Junk mail is stored and filed as `spam`, never dropped at intake | ⚠️ INFERRED — follows from inbound-ingestion.md §1's never-dropped invariant, but applying it to junk specifically is the author's reading | +| D1 | Whether automatic spam classification is on by default, and whether an operator can turn it off | ⚠️ INFERRED — unresolved; §6 is unbuildable until answered | +| D2 | A spam-verdict message whose reply token names a *deleted* conversation is filed as spam | ⚠️ INFERRED — the valid token argues it is a real customer; the deletion argues an Actor already discarded that thread. Uniform rule chosen for simplicity, not because the edge was decided | +| D3 | Whether an Actor's "not spam" correction is written back to the operator's Gmail | ⚠️ INFERRED — unresolved. Writing to the operator's mailbox unprompted is a one-way door of a kind the charter's data-ownership promise touches | +| D4 | Header scoring requires one strong or two weak signals; a lone weak signal never classifies | ⚠️ INFERRED — a conservative default, not a measured threshold | +| D5 | `Auto-Submitted` is a loop-guard concern, not a spam signal | ⚠️ INFERRED | + +**One-way door:** none in what is built. D3 would be one — it modifies state in the +operator's own mailbox, outside Helpthread's database. diff --git a/src/mail/gmail-reconcile.test.ts b/src/mail/gmail-reconcile.test.ts index 0112646..4621d29 100644 --- a/src/mail/gmail-reconcile.test.ts +++ b/src/mail/gmail-reconcile.test.ts @@ -657,6 +657,108 @@ describe('createGmailReconcileHandler', () => { }) }) + // --- The provider spam verdict (specs/mail/spam-classification.md §3) ------ + // Gmail has already classified every message it hands us. `history.list` + // is not label-filtered, so a SPAM-labeled message reaches this handler + // exactly like any other added message. It is INGESTED (never dropped — + // inbound-ingestion.md §1 invariant #3), carrying the verdict forward so + // ingest can file the conversation as `spam` instead of `active`. + + describe('provider spam verdict', () => { + it("a SPAM-labeled message is ingested, not dropped, and carries providerSpamVerdict: 'spam'", async () => { + const { store: watchStateStore, setCalls } = fakeWatchStateStore({ [MAILBOX_ID]: 'cursor-1' }) + const historyClient = fakeHistoryClient({ + listResult: { + kind: 'ok', + messages: taggedMsgs([['junk-1', ['SPAM']]]), + newHistoryId: 'cursor-2', + }, + rawMessages: { 'junk-1': { rawBytes: textBytes('raw'), receivedAt: new Date() } }, + }) + const ingest = vi.fn(async (raw: RawInboundMessage) => storedOutcome(raw)) + + const handler = createGmailReconcileHandler( + baseDeps({ watchStateStore, ingest, createHistoryClient: () => historyClient }), + ) + + const result = await handler(job()) + + expect(result).toEqual({ kind: 'ack' }) + // Never dropped: junk still gets a ledger row and a conversation. + expect(ingest).toHaveBeenCalledTimes(1) + const raw = ingest.mock.calls[0][0] as RawInboundMessage + expect(raw.providerMessageId).toBe('junk-1') + expect(raw.providerSpamVerdict).toBe('spam') + expect(setCalls).toEqual([{ mailboxId: MAILBOX_ID, historyId: 'cursor-2' }]) + }) + + it("an ordinary INBOX message carries providerSpamVerdict: 'clean' — an explicit not-spam, not an absent field", async () => { + const { store: watchStateStore } = fakeWatchStateStore({ [MAILBOX_ID]: 'cursor-1' }) + const historyClient = fakeHistoryClient({ + listResult: { + kind: 'ok', + messages: taggedMsgs([['customer-1', ['INBOX']]]), + newHistoryId: 'cursor-2', + }, + rawMessages: { 'customer-1': { rawBytes: textBytes('raw'), receivedAt: new Date() } }, + }) + const ingest = vi.fn(async (raw: RawInboundMessage) => storedOutcome(raw)) + + const handler = createGmailReconcileHandler( + baseDeps({ watchStateStore, ingest, createHistoryClient: () => historyClient }), + ) + + await handler(job()) + + expect((ingest.mock.calls[0][0] as RawInboundMessage).providerSpamVerdict).toBe('clean') + }) + + it("a message with no labelIds at all reports 'unknown' — Gmail omitted the field, so we assert nothing either way", async () => { + const { store: watchStateStore } = fakeWatchStateStore({ [MAILBOX_ID]: 'cursor-1' }) + const historyClient = fakeHistoryClient({ + listResult: { + kind: 'ok', + messages: taggedMsgs([['no-labels-1', []]]), + newHistoryId: 'cursor-2', + }, + rawMessages: { 'no-labels-1': { rawBytes: textBytes('raw'), receivedAt: new Date() } }, + }) + const ingest = vi.fn(async (raw: RawInboundMessage) => storedOutcome(raw)) + + const handler = createGmailReconcileHandler( + baseDeps({ watchStateStore, ingest, createHistoryClient: () => historyClient }), + ) + + await handler(job()) + + expect((ingest.mock.calls[0][0] as RawInboundMessage).providerSpamVerdict).toBe('unknown') + }) + + it('a SPAM-labeled message that is ALSO a self-echo is still skipped — the self-echo filter runs first', async () => { + const { store: watchStateStore, setCalls } = fakeWatchStateStore({ [MAILBOX_ID]: 'cursor-1' }) + const getRawMessage = vi.fn(async () => null) + const historyClient: GmailHistoryClient = { + listAddedMessageIds: async () => ({ + kind: 'ok', + messages: taggedMsgs([['echo-junk-1', ['SENT', 'SPAM']]]), + newHistoryId: 'cursor-2', + }), + getRawMessage, + } + const ingest = vi.fn(async (raw: RawInboundMessage) => storedOutcome(raw)) + + const handler = createGmailReconcileHandler( + baseDeps({ watchStateStore, ingest, createHistoryClient: () => historyClient }), + ) + + const result = await handler(job()) + + expect(result).toEqual({ kind: 'ack' }) + expect(ingest).not.toHaveBeenCalled() + expect(setCalls).toEqual([{ mailboxId: MAILBOX_ID, historyId: 'cursor-2' }]) + }) + }) + it('a 404-expired cursor pauses the mailbox, does not advance the cursor, and acks', async () => { const { store: mailboxStore, records } = fakeMailboxStore(activeMailbox()) const { store: watchStateStore, setCalls } = fakeWatchStateStore({ diff --git a/src/mail/gmail-reconcile.ts b/src/mail/gmail-reconcile.ts index 62853a5..ae22d2a 100644 --- a/src/mail/gmail-reconcile.ts +++ b/src/mail/gmail-reconcile.ts @@ -256,7 +256,12 @@ import type { GmailReconcileJob } from '../api/gmail-webhook.js' // compile time; the concrete `createGmailHistoryClient` is wired in at the // composition root and injected as `createHistoryClient` below. import type { GmailHistoryClient } from '../providers/adapters/gmail/index.js' -import type { BlobStore, RawInboundMessage, RawMessageContent } from '../providers/index.js' +import type { + BlobStore, + ProviderSpamVerdict, + RawInboundMessage, + RawMessageContent, +} from '../providers/index.js' import type { QueueHandlerResult, QueueMessage, QueueMessageHandler } from '../providers/queue.js' import type { GmailWatchStateStore } from '../store/gmail-watch-state.js' import type { MailboxStore } from '../store/mailboxes.js' @@ -584,6 +589,7 @@ async function reconcileOneMailbox( mailboxId, providerMessageId: messageId, receivedAt: fetched.receivedAt, + providerSpamVerdict: spamVerdictOf(labelIds), } outcomes.push(await ingest(raw)) } @@ -664,6 +670,34 @@ function isSelfEchoMessage(labelIds: string[]): boolean { return labelIds.includes('SENT') && !labelIds.includes('INBOX') } +/** + * Translate Gmail's own labels into the transport-agnostic + * {@link RawInboundMessage.providerSpamVerdict} (specs/mail/spam- + * classification.md §3.1). Gmail has already classified every message it + * hands us; `history.list` is deliberately NOT label-filtered (see + * `../providers/adapters/gmail/history.ts`), so a `SPAM`-labeled message + * arrives here exactly like any other added message and, before this, + * became an ordinary `active` conversation — Google's verdict silently + * discarded. + * + * The empty-`labelIds` case reports `'unknown'`, not `'clean'`: the history + * client documents that Gmail does not guarantee `labelIds` is populated on + * a history record, so an absent field is missing information, never an + * affirmative not-spam. This mirrors the fail-open discipline the self-echo + * filter already applies to the same field — an unlabeled message is + * ingested normally, into the ordinary inbox. + * + * Note the ordering at the call site: this runs only AFTER + * {@link isSelfEchoMessage} has already skipped the message, so a + * `SENT`+`SPAM` self-echo (our own reply that Gmail happened to file as + * junk) is never ingested at all rather than being filed as a spam + * conversation. + */ +function spamVerdictOf(labelIds: string[]): ProviderSpamVerdict { + if (labelIds.length === 0) return 'unknown' + return labelIds.includes('SPAM') ? 'spam' : 'clean' +} + /** * Build the `RawMessageContent` handed to `ingest` for one fetched raw * message: `inline` at or under `maxInlineRawBytes`, otherwise written to diff --git a/src/mail/ingest.test.ts b/src/mail/ingest.test.ts index dce48c1..0224185 100644 --- a/src/mail/ingest.test.ts +++ b/src/mail/ingest.test.ts @@ -1,7 +1,7 @@ import { afterEach, describe, expect, it, vi } from 'vitest' import { createPgliteDb, type Db, type Queryable } from '../db/client.js' import { migrate } from '../db/migrate.js' -import type { BlobStore, RawInboundMessage } from '../providers/index.js' +import type { BlobStore, ProviderSpamVerdict, RawInboundMessage } from '../providers/index.js' import { createThreadAttachmentStore } from '../store/attachments.js' import { createInboundDeliveryStore } from '../store/inbound-deliveries.js' import { @@ -50,12 +50,14 @@ function inboundDelivery( mailboxId: string, providerMessageId: string, bytes: Uint8Array, + providerSpamVerdict?: ProviderSpamVerdict, ): RawInboundMessage { return { content: { kind: 'inline', bytes }, mailboxId, providerMessageId, receivedAt: new Date('2026-07-13T12:00:00.000Z'), + ...(providerSpamVerdict === undefined ? {} : { providerSpamVerdict }), } } @@ -334,6 +336,111 @@ describe('ingestInboundMessage', () => { expect(await countRows(db, 'threads')).toBe(2) }) + // --- specs/mail/spam-classification.md §4: the provider's spam verdict + // decides the STATUS a newly-created conversation is filed under, and + // nothing else. It never drops a message, and it never touches a + // conversation that already exists. ------------------------------------- + + describe('provider spam verdict', () => { + async function statusOf(db: Db, conversationId: string): Promise { + const rows = await db.query<{ status: string }>( + 'SELECT status FROM conversations WHERE id = $1', + [conversationId], + ) + return rows[0].status + } + + it("a 'spam' verdict files the NEW conversation as spam — stored and threaded exactly as normal, just not in the inbox", async () => { + const { db, deps, mailboxId } = await freshDeps() + + const outcome = await ingestInboundMessage( + inboundDelivery(mailboxId, 'provider-junk-1', freshCustomerRaw(), 'spam'), + deps, + ) + + expect(outcome).toMatchObject({ kind: 'stored' }) + if (outcome.kind !== 'stored') throw new Error('unreachable') + expect(await statusOf(db, outcome.conversationId)).toBe('spam') + // Never dropped: the message itself is stored in full, so an Agent who + // opens the Spam folder sees the real thing and can reopen it. + expect(await countRows(db, 'conversations')).toBe(1) + expect(await countRows(db, 'threads')).toBe(1) + }) + + it("a 'clean' verdict files the new conversation as active", async () => { + const { db, deps, mailboxId } = await freshDeps() + + const outcome = await ingestInboundMessage( + inboundDelivery(mailboxId, 'provider-msg-1', freshCustomerRaw(), 'clean'), + deps, + ) + + if (outcome.kind !== 'stored') throw new Error('unreachable') + expect(await statusOf(db, outcome.conversationId)).toBe('active') + }) + + it("an 'unknown' verdict files the new conversation as active — absent evidence is never evidence of junk", async () => { + const { db, deps, mailboxId } = await freshDeps() + + const outcome = await ingestInboundMessage( + inboundDelivery(mailboxId, 'provider-msg-1', freshCustomerRaw(), 'unknown'), + deps, + ) + + if (outcome.kind !== 'stored') throw new Error('unreachable') + expect(await statusOf(db, outcome.conversationId)).toBe('active') + }) + + it('an omitted verdict (a provider that does not classify) files the new conversation as active', async () => { + const { db, deps, mailboxId } = await freshDeps() + + const outcome = await ingestInboundMessage( + inboundDelivery(mailboxId, 'provider-msg-1', freshCustomerRaw()), + deps, + ) + + if (outcome.kind !== 'stored') throw new Error('unreachable') + expect(await statusOf(db, outcome.conversationId)).toBe('active') + }) + + it("a 'spam'-verdict REPLY that threads onto an existing conversation never re-files it as spam — the token is the stronger signal, and an Agent's own judgment is never overridden", async () => { + const { db, deps, mailboxId } = await freshDeps() + + const first = await ingestInboundMessage( + inboundDelivery(mailboxId, 'provider-msg-1', freshCustomerRaw(), 'clean'), + deps, + ) + if (first.kind !== 'stored') throw new Error('unreachable') + + const replyToken = mintReplyMessageId( + { conversationId: first.conversationId, threadId: 'outbound-t1', mailDomain: MAIL_DOMAIN }, + keyring, + ) + const replyRaw = rawMessage( + { + From: 'customer@example.test', + To: 'support@example.test', + Subject: 'Re: Help with my order', + 'Message-ID': '', + 'In-Reply-To': replyToken, + }, + 'Still broken, please help.', + ) + + // Gmail called this reply junk. It still threads, and the existing + // conversation stays exactly where the Agent left it. + const second = await ingestInboundMessage( + inboundDelivery(mailboxId, 'provider-msg-2', replyRaw, 'spam'), + deps, + ) + + expect(second).toMatchObject({ kind: 'stored', conversationId: first.conversationId }) + expect(await statusOf(db, first.conversationId)).toBe('active') + expect(await countRows(db, 'conversations')).toBe(1) + expect(await countRows(db, 'threads')).toBe(2) + }) + }) + // --- HT-101 Stage 2b-i: mailbox_id is stamped once, at creation, never // re-stamped on append (purely additive — no other ingest behavior above // this line is touched by these two tests). ------------------------------ diff --git a/src/mail/ingest.ts b/src/mail/ingest.ts index 7b7d1eb..8811c9f 100644 --- a/src/mail/ingest.ts +++ b/src/mail/ingest.ts @@ -56,7 +56,12 @@ import { randomUUID } from 'node:crypto' import type { Db, Queryable } from '../db/client.js' -import type { BlobStore, RawInboundMessage, RawMessageContent } from '../providers/index.js' +import type { + BlobStore, + ProviderSpamVerdict, + RawInboundMessage, + RawMessageContent, +} from '../providers/index.js' import { insertThreadAttachmentsInTx, type NewThreadAttachment } from '../store/attachments.js' import { appendThreadInTx, createConversationInTx, type NewThread } from '../store/conversations.js' import { appendOutboxEventInTx } from '../store/event-outbox.js' @@ -439,6 +444,7 @@ async function processClaimedDelivery( parsed, attachmentRefs, delivery.attempts, + raw.providerSpamVerdict ?? 'unknown', ) logIngestEvent({ ...base, @@ -648,9 +654,10 @@ async function storeAndMarkDelivered( parsed: ParsedEmail, attachmentRefs: Omit[], claimedAttempts: number, + spamVerdict: ProviderSpamVerdict, ): Promise<{ conversationId: string; threadId: string; appendFallback?: AppendFallbackReason }> { return db.transaction(async (tx) => { - const written = await writeParsedEmail(tx, mailboxId, decision, parsed) + const written = await writeParsedEmail(tx, mailboxId, decision, parsed, spamVerdict) await insertThreadAttachmentsInTx( tx, attachmentRefs.map((ref) => ({ ...ref, threadId: written.threadId })), @@ -720,12 +727,36 @@ async function storeAndMarkDelivered( * already has whatever mailbox its own creation recorded, and a reply * threaded onto it must not overwrite that, even if this particular reply * happened to arrive at a different connected mailbox. + * + * ## The spam verdict (specs/mail/spam-classification.md §4) + * + * `spamVerdict` follows exactly the same shape, and for the same reason: it + * decides the STATUS of a brand-new conversation at both + * `createConversationInTx` call sites, and the `append` branch never reads + * it at all. Three properties are load-bearing: + * + * - **It never drops anything.** A `'spam'` verdict changes one column. The + * message is parsed, stored, threaded, and attachment-linked exactly as a + * clean one is (inbound-ingestion.md §1's third invariant), so an Actor + * who opens the Spam folder sees the real message and a reply reopens it + * to `active` (agent-inbox-v1.md §4a) — a false positive is always + * recoverable. + * - **It never re-files an existing conversation.** A reply that carried a + * valid reply token threads onto its target and leaves that target's + * status alone, however the provider classified this particular message. + * Our own token is the stronger signal, and an Actor who put a + * conversation somewhere must not be silently overruled by Google. + * - **Only `'spam'` is evidence.** `'clean'`, `'unknown'`, and an omitted + * field all mean `active` — see {@link RawInboundMessage.providerSpamVerdict} + * for why "we asked and it said no" and "we have no idea" are still kept + * distinct on the wire even though they agree here. */ async function writeParsedEmail( tx: Queryable, mailboxId: string, decision: ThreadingDecision, parsed: ParsedEmail, + spamVerdict: ProviderSpamVerdict, ): Promise<{ conversationId: string; threadId: string; appendFallback?: AppendFallbackReason }> { const firstMessage: NewThread = { direction: 'inbound', @@ -735,6 +766,7 @@ async function writeParsedEmail( bodyText: parsed.text, bodyHtml: parsed.html, } + const status = spamVerdict === 'spam' ? 'spam' : 'active' if (decision.kind === 'new') { const created = await createConversationInTx(tx, { @@ -742,6 +774,7 @@ async function writeParsedEmail( customerEmail: fromAddressOf(parsed), firstMessage, mailboxId, + status, }) await emitNewConversationEvents(tx, created.conversationId, created.threadId) return created @@ -769,6 +802,7 @@ async function writeParsedEmail( customerEmail: fromAddressOf(parsed), firstMessage, mailboxId, + status, }) await emitNewConversationEvents(tx, created.conversationId, created.threadId) return { ...created, appendFallback: appended.reason } diff --git a/src/providers/inbound-email.ts b/src/providers/inbound-email.ts index 0b6bc35..62b5a5d 100644 --- a/src/providers/inbound-email.ts +++ b/src/providers/inbound-email.ts @@ -103,8 +103,41 @@ export interface RawInboundMessage { /** When the provider recorded/delivered the message — not a header-parsed `Date`. */ receivedAt: Date + + /** + * What the TRANSPORT already concluded about this message being junk + * (specs/mail/spam-classification.md §3) — the provider's own verdict, + * carried through verbatim, never re-derived here. + * + * Every mail provider we intake from runs its own spam classifier before + * we ever see the message, and that verdict is both free and better than + * anything we would compute from headers alone. This field is how it + * survives the provider→pipeline seam instead of being discarded. + * + * Three states, deliberately not a boolean: + * + * - `'spam'` — the provider affirmatively classified it as junk (for + * Gmail, the system `SPAM` label on the message). + * - `'clean'` — the provider classified it and did NOT call it junk. + * - `'unknown'` — no verdict is available (the provider does not classify, + * or omitted the field on this delivery). Distinct from `'clean'` + * because "we asked and it said no" and "we have no idea" must not + * collapse: only the former is evidence. + * + * Omitted entirely by a provider that has no concept of a spam verdict; + * `src/mail/ingest.ts` reads an absent field exactly like `'unknown'`. + * + * A verdict NEVER causes a message to be dropped — inbound-ingestion.md + * §1's third invariant (at-least-once, never silently lost) is not + * negotiable, and a false positive that vanished would be unrecoverable. + * It only decides the STATUS a newly-created conversation is filed under. + */ + providerSpamVerdict?: ProviderSpamVerdict } +/** See {@link RawInboundMessage.providerSpamVerdict}. */ +export type ProviderSpamVerdict = 'spam' | 'clean' | 'unknown' + /** * Provider for turning one inbound-mail provider's webhook delivery into * the raw message(s) it contains. One implementation per provider (Gmail diff --git a/src/providers/index.ts b/src/providers/index.ts index 734eb3e..f7af974 100644 --- a/src/providers/index.ts +++ b/src/providers/index.ts @@ -8,6 +8,7 @@ export type { BlobStore } from './blob.js' export type { EmailSender, EmailSendResult, OutboundEmail } from './email-sender.js' export type { InboundEmailProvider, + ProviderSpamVerdict, RawInboundMessage, RawMessageContent, } from './inbound-email.js' diff --git a/src/store/conversations.ts b/src/store/conversations.ts index 098bb24..cb1d60b 100644 --- a/src/store/conversations.ts +++ b/src/store/conversations.ts @@ -257,6 +257,22 @@ export interface NewConversation { * (see {@link StoredConversation.mailboxId}). */ mailboxId?: string + + /** + * The status this conversation is CREATED at — `'active'` when omitted, + * which is what every caller but one wants and what the column already + * defaults to. + * + * The one caller that supplies it is `src/mail/ingest.ts`, passing + * `'spam'` when the transport's own classifier already called the message + * junk (`RawInboundMessage.providerSpamVerdict`, specs/mail/spam- + * classification.md §4). Deliberately narrowed to those two values: a + * conversation is never BORN `closed` (nothing has been resolved), + * `pending` (nobody has snoozed it), or `deleted` (it would be + * unreachable) — those are states an Actor moves a conversation into + * later, via `ConversationStore.setStatus`. + */ + status?: 'active' | 'spam' } /** A thread as read back from storage — camelCase, timestamps as `Date`. */ @@ -1132,8 +1148,8 @@ export async function createConversationInTx( input: NewConversation, ): Promise<{ conversationId: string; threadId: string }> { const [conversation] = await tx.query<{ id: string }>( - 'INSERT INTO conversations (subject, customer_email, mailbox_id) VALUES ($1, $2, $3) RETURNING id', - [input.subject, input.customerEmail, input.mailboxId ?? null], + 'INSERT INTO conversations (subject, customer_email, mailbox_id, status) VALUES ($1, $2, $3, $4) RETURNING id', + [input.subject, input.customerEmail, input.mailboxId ?? null, input.status ?? 'active'], ) const { threadId } = await insertThread(tx, conversation.id, input.firstMessage) return { conversationId: conversation.id, threadId } From f8cf328d19861c0eb4f8afaae07c221f827a51cb Mon Sep 17 00:00:00 2001 From: TJ Baker <1617679+zaridan@users.noreply.github.com> Date: Sun, 2 Aug 2026 13:06:41 -0700 Subject: [PATCH 2/4] fix(mail): address CodeRabbit review on the spam-verdict intake MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three findings, all real. 1. The self-echo-plus-SPAM regression test asserted that ingest was never called but not that getRawMessage was never called — so it did not actually verify the ordering claim it exists to protect (the self-echo filter runs before messages.get). Asserted now. 2. Vocabulary: the new spec and ingest's doc comments said "Actor" where the repo's checked-in vocabulary says "Agent" for human support staff. Aligned with the repo as it stands. (A rename of this vocabulary has been discussed but has not landed in the repo; this change follows what is actually written here rather than pre-empting it.) 3. The status contract read as self-contradictory across the two specs, and the ambiguity hid a real interaction nobody had written down: a reply that threads onto a conversation ALREADY filed as spam reopens it to active, even when the provider also called that reply junk. That is agent-inbox-v1.md §4a's pre-existing reopen rule composing with this spec's intake classification, and it is correct — the reply token proves we wrote to that address first — but it was nowhere stated. Verified against the code before documenting it: appendThreadInTx's reopen policy is `closed OR spam → active` on any genuinely-new inbound thread. §4.2 now carries the full four-row interaction table, both spam rows covered by new tests, and agent-inbox-v1.md §3a is tightened so "nothing re-files an existing conversation" can no longer be misread as denying the reopen rule. The practical consequence, now stated: a false positive self-heals the moment the sender replies, without anyone opening the Spam folder. Gates: tsc 0, biome 190 clean, src/mail 399 pass. Co-Authored-By: Claude Opus 5 --- specs/api/agent-inbox-v1.md | 21 ++++++++----- specs/mail/spam-classification.md | 51 +++++++++++++++++++++---------- src/mail/gmail-reconcile.test.ts | 4 +++ src/mail/ingest.test.ts | 38 +++++++++++++++++++++++ src/mail/ingest.ts | 4 +-- 5 files changed, 93 insertions(+), 25 deletions(-) diff --git a/specs/api/agent-inbox-v1.md b/specs/api/agent-inbox-v1.md index 23c2ad7..2cc13bb 100644 --- a/specs/api/agent-inbox-v1.md +++ b/specs/api/agent-inbox-v1.md @@ -107,16 +107,23 @@ interface AttachmentView { ``` **Status semantics (v1.1).** `active` is the working state — inbound mail creates -conversations `active`, and v1.0's `open` rows migrate to `active`. `pending` is an Agent +conversations `active` (with one exception, below), and v1.0's `open` rows migrate to +`active`. `pending` is an Agent statement that the conversation is parked awaiting something outside the inbox (a customer, a third party, a release); nothing sets it automatically in v1, and it still counts as open work (§3a). `closed` is resolved. `spam` is junk an Agent has thrown out -of the inbox — **or, since 2026-08-02, junk a brand-new conversation was filed as at -intake when the transport's own classifier already called it spam** -([spam-classification.md](../mail/spam-classification.md) §4). That is the only automatic -source: nothing re-files an existing conversation, and no message is ever dropped rather -than filed. Status pills in the UI: Active = accent, Pending = warn, Closed = dim, -Spam = critical. +of the inbox — **or, since 2026-08-02, a brand-new conversation filed as junk at intake +because the transport's own classifier already called it spam** +([spam-classification.md](../mail/spam-classification.md) §4). Status pills in the UI: +Active = accent, Pending = warn, Closed = dim, Spam = critical. + +That intake classification is narrow, and two boundaries matter. It applies **only when +the conversation is created** — it is never read for a message that threads onto an +existing conversation, so it can neither re-file nor rescue one. And it never decides +whether a message is *stored*: junk is filed under a different status, never dropped +(inbound-ingestion.md §1). The reopen rule below is unaffected by it and still governs +existing conversations — including one filed as `spam` at intake, which a reply reopens +to `active` like any other. See spam-classification.md §4.2 for the full interaction. **Snooze exception to "pending is never cleared automatically" (v1.1).** A snooze is a TIMED `pending` — `pending` plus a `snoozedUntil` timestamp (§4b) — and it is the diff --git a/specs/mail/spam-classification.md b/specs/mail/spam-classification.md index 9cfeb68..25d25fb 100644 --- a/specs/mail/spam-classification.md +++ b/specs/mail/spam-classification.md @@ -13,7 +13,7 @@ always outranks anything here. Status vocabulary is ## 1. The problem this closes -Before this spec, `spam` was a status an Actor set by hand and nothing else — as +Before this spec, `spam` was a status an Agent set by hand and nothing else — as agent-inbox-v1.md §3a put it: "nothing classifies spam automatically in v1." That was a deliberate v1 deferral, but it left a defect underneath it. The two intake @@ -47,7 +47,7 @@ Three constraints bound everything below: 2. **Our own reply token always wins.** If `decideThreading` matched a valid token, the message belongs to that conversation and the conversation's status is not this spec's business (§4.2). -3. **An Actor's judgment is never silently overruled.** Automatic classification decides +3. **An Agent's judgment is never silently overruled.** Automatic classification decides where a *brand-new* conversation is filed. It never re-files a conversation a human already placed. @@ -136,26 +136,45 @@ arrived cannot audit what we filed. ### 4.2 Replies to existing conversations -**No effect, ever.** A message that threads onto an existing conversation by valid reply -token leaves that conversation's status untouched, however the provider classified it. -Two reasons, either sufficient: our token is the stronger signal (we minted it, and it -proves we wrote to this address first), and the target's current status may be an Actor's -own deliberate placement. +**The verdict has no effect on an existing conversation, ever.** A message that threads +onto one by valid reply token contributes its thread and nothing else; the verdict is not +read. Two reasons, either sufficient: our token is the stronger signal (we minted it, and +it proves we wrote to this address first), and the target's current status may be an +Agent's own deliberate placement. -The existing reopen rule is unchanged and does the right thing here already: a reply to a -`spam` conversation reopens it to `active` (agent-inbox-v1.md §4a). A customer who -answers is a customer, whatever a classifier thought of the first message. +That is a statement about **this spec's** input only, and it must not be read as "the +status cannot change." It can, by a rule that predates this spec and is untouched by it: +agent-inbox-v1.md §4a's reopen, which moves a `closed` or `spam` conversation to `active` +on any genuinely-new inbound thread (`appendThreadInTx`, `src/store/conversations.ts`). + +Spelled out, because the composition is the part that is easy to get wrong: + +| Target's status | Verdict on the arriving reply | Resulting status | Decided by | +|---|---|---|---| +| `active` | `'spam'` | `active` — unchanged | This spec: verdict not read on append | +| `active` | `'clean'` / `'unknown'` | `active` — unchanged | This spec: verdict not read on append | +| `spam` | `'spam'` | **`active` — reopened** | §4a's reopen rule, not this spec | +| `spam` | `'clean'` / `'unknown'` | **`active` — reopened** | §4a's reopen rule, not this spec | + +The third row is the counter-intuitive one and it is deliberate: a message the provider +called junk, replying to a conversation we ourselves filed as junk, still reopens it. The +reply token proves we wrote to that address first, and a customer who answers is a +customer whatever a classifier thought of either message. Both `spam` rows are covered by +tests in `src/mail/ingest.test.ts`. + +The practical consequence for an operator: a false positive self-heals the moment the +sender replies, and never needs the Spam folder to be checked for it to do so. ## 5. Reclassification (NOT BUILT) -### 5.1 The Actor's correction is the ground truth +### 5.1 The Agent's correction is the ground truth -An Actor moving a conversation out of `spam` is the only correction signal that matters. +An Agent moving a conversation out of `spam` is the only correction signal that matters. It is already recorded by `setStatus`. ### 5.2 Sender allow-listing -A sender an Actor has rescued from spam should not be re-classified on their next +A sender an Agent has rescued from spam should not be re-classified on their next message. The intended mechanism is per-mailbox sender state, not a global list. ### 5.3 Feeding corrections back to the provider — OPEN @@ -169,7 +188,7 @@ schedule they cannot see. **Not decided** (§7, D3). Whether automatic classification is on by default, and whether it can be turned off, is **not decided** (§7, D1). The rest of the surface follows from that answer: a settings toggle, and whether the Spam folder shows a "filed automatically" affordance distinct -from "an Actor filed this." +from "an Agent filed this." ## 7. Decision ledger @@ -182,8 +201,8 @@ source. Anything marked **INFERRED** is the author's judgment and has not been a | — | Spam is never auto-classified in v1 | Prior accepted spec, agent-inbox-v1.md §3a | | D0 | Junk mail is stored and filed as `spam`, never dropped at intake | ⚠️ INFERRED — follows from inbound-ingestion.md §1's never-dropped invariant, but applying it to junk specifically is the author's reading | | D1 | Whether automatic spam classification is on by default, and whether an operator can turn it off | ⚠️ INFERRED — unresolved; §6 is unbuildable until answered | -| D2 | A spam-verdict message whose reply token names a *deleted* conversation is filed as spam | ⚠️ INFERRED — the valid token argues it is a real customer; the deletion argues an Actor already discarded that thread. Uniform rule chosen for simplicity, not because the edge was decided | -| D3 | Whether an Actor's "not spam" correction is written back to the operator's Gmail | ⚠️ INFERRED — unresolved. Writing to the operator's mailbox unprompted is a one-way door of a kind the charter's data-ownership promise touches | +| D2 | A spam-verdict message whose reply token names a *deleted* conversation is filed as spam | ⚠️ INFERRED — the valid token argues it is a real customer; the deletion argues an Agent already discarded that thread. Uniform rule chosen for simplicity, not because the edge was decided | +| D3 | Whether an Agent's "not spam" correction is written back to the operator's Gmail | ⚠️ INFERRED — unresolved. Writing to the operator's mailbox unprompted is a one-way door of a kind the charter's data-ownership promise touches | | D4 | Header scoring requires one strong or two weak signals; a lone weak signal never classifies | ⚠️ INFERRED — a conservative default, not a measured threshold | | D5 | `Auto-Submitted` is a loop-guard concern, not a spam signal | ⚠️ INFERRED | diff --git a/src/mail/gmail-reconcile.test.ts b/src/mail/gmail-reconcile.test.ts index 4621d29..3ac3a7d 100644 --- a/src/mail/gmail-reconcile.test.ts +++ b/src/mail/gmail-reconcile.test.ts @@ -755,6 +755,10 @@ describe('createGmailReconcileHandler', () => { expect(result).toEqual({ kind: 'ack' }) expect(ingest).not.toHaveBeenCalled() + // The ordering claim in spamVerdictOf's doc comment, asserted rather + // than described: the self-echo filter runs BEFORE messages.get, so a + // skipped echo costs no raw fetch even when it also carries SPAM. + expect(getRawMessage).not.toHaveBeenCalled() expect(setCalls).toEqual([{ mailboxId: MAILBOX_ID, historyId: 'cursor-2' }]) }) }) diff --git a/src/mail/ingest.test.ts b/src/mail/ingest.test.ts index 0224185..22a1d19 100644 --- a/src/mail/ingest.test.ts +++ b/src/mail/ingest.test.ts @@ -439,6 +439,44 @@ describe('ingestInboundMessage', () => { expect(await countRows(db, 'conversations')).toBe(1) expect(await countRows(db, 'threads')).toBe(2) }) + + it("a 'spam'-verdict reply onto an ALREADY-spam conversation still REOPENS it to active — the reopen rule (agent-inbox-v1.md §4a) outranks the verdict, because a customer who replies is a customer", async () => { + const { db, deps, mailboxId } = await freshDeps() + + // A first message the provider called junk: filed as spam. + const first = await ingestInboundMessage( + inboundDelivery(mailboxId, 'provider-junk-1', freshCustomerRaw(), 'spam'), + deps, + ) + if (first.kind !== 'stored') throw new Error('unreachable') + expect(await statusOf(db, first.conversationId)).toBe('spam') + + const replyToken = mintReplyMessageId( + { conversationId: first.conversationId, threadId: 'outbound-t1', mailDomain: MAIL_DOMAIN }, + keyring, + ) + const replyRaw = rawMessage( + { + From: 'customer@example.test', + To: 'support@example.test', + Subject: 'Re: Help with my order', + 'Message-ID': '', + 'In-Reply-To': replyToken, + }, + 'Still broken, please help.', + ) + + // The provider called THIS one junk too. It still reopens: the token + // proves we wrote to this address first, which outranks a classifier. + const second = await ingestInboundMessage( + inboundDelivery(mailboxId, 'provider-junk-2', replyRaw, 'spam'), + deps, + ) + + expect(second).toMatchObject({ kind: 'stored', conversationId: first.conversationId }) + expect(await statusOf(db, first.conversationId)).toBe('active') + expect(await countRows(db, 'conversations')).toBe(1) + }) }) // --- HT-101 Stage 2b-i: mailbox_id is stamped once, at creation, never diff --git a/src/mail/ingest.ts b/src/mail/ingest.ts index 8811c9f..eacb590 100644 --- a/src/mail/ingest.ts +++ b/src/mail/ingest.ts @@ -737,14 +737,14 @@ async function storeAndMarkDelivered( * * - **It never drops anything.** A `'spam'` verdict changes one column. The * message is parsed, stored, threaded, and attachment-linked exactly as a - * clean one is (inbound-ingestion.md §1's third invariant), so an Actor + * clean one is (inbound-ingestion.md §1's third invariant), so an Agent * who opens the Spam folder sees the real message and a reply reopens it * to `active` (agent-inbox-v1.md §4a) — a false positive is always * recoverable. * - **It never re-files an existing conversation.** A reply that carried a * valid reply token threads onto its target and leaves that target's * status alone, however the provider classified this particular message. - * Our own token is the stronger signal, and an Actor who put a + * Our own token is the stronger signal, and an Agent who put a * conversation somewhere must not be silently overruled by Google. * - **Only `'spam'` is evidence.** `'clean'`, `'unknown'`, and an omitted * field all mean `active` — see {@link RawInboundMessage.providerSpamVerdict} From 49598e0419153af857f94a4086e9e8f259b324bd Mon Sep 17 00:00:00 2001 From: TJ Baker <1617679+zaridan@users.noreply.github.com> Date: Sun, 2 Aug 2026 13:28:51 -0700 Subject: [PATCH 3/4] docs(mail): state the delta-window limit of the Gmail spam verdict MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit From an adversarial GPT-5.2 pass standing in for CodeRabbit, which hit its fair-usage limit and produced a green check with no review. Its headline finding — that the verdict is computed from messageAdded deltas only, so a later SPAM label is never seen — is wrong as stated: the history client also requests `labelAdded` and set-unions the deltas (src/providers/adapters/gmail/history.ts), so a message that arrives as INBOX and is classified SPAM moments later, within the same reconcile window, IS seen as spam. But the weaker version of it is real and was not written down anywhere: a SPAM label applied AFTER the window's history.list snapshot never reaches us. That message is already ingested as active, and the next reconcile deliberately ignores a labelsAdded record for an id it did not itself newly add — otherwise re-labeling any old message would manufacture an ingest. So it stays active. Not a regression (before this spec, every spam message stayed active), but it means the feature must not be described as "Gmail's spam filtering now applies." It applies to mail Gmail had classified by the time we read the history window — the common case, not all cases. §3.1 now says so, and points at §5 as where the remainder would be closed. Also renamed a test whose name overclaimed: it passes `[]`, and `[]` is exactly what the adapter normalizes an omitted labelIds to, so there is no separate undefined shape at this seam. The name now says that instead of implying an untested case. TRASH-labeled mail filing as active is real but pre-existing and out of scope here; tracked separately rather than folded in. Gates: tsc 0, biome 190 clean, src/mail 399 pass. Co-Authored-By: Claude Opus 5 --- specs/mail/spam-classification.md | 17 +++++++++++++++++ src/mail/gmail-reconcile.test.ts | 8 +++++++- 2 files changed, 24 insertions(+), 1 deletion(-) diff --git a/specs/mail/spam-classification.md b/specs/mail/spam-classification.md index 25d25fb..1245303 100644 --- a/specs/mail/spam-classification.md +++ b/specs/mail/spam-classification.md @@ -80,6 +80,23 @@ header signals will need to tell them apart — header scoring should defer to a field is populated. This runs **after** the self-echo filter, so our own outbound reply that Gmail happened to file as junk is skipped entirely rather than filed as a spam conversation. + + **This is best-effort, not complete, and the boundary is worth stating precisely.** + `history.list` is a delta stream, not a snapshot. The history client requests + `labelAdded` alongside `messageAdded` and set-unions the label deltas + (`src/providers/adapters/gmail/history.ts`), so a message that arrives labeled `INBOX` + and is classified `SPAM` moments later — **within the same reconcile window** — is + correctly seen as spam. What is *not* covered: a `SPAM` label applied **after** the + window's `history.list` snapshot. That message has already been ingested as `active`, + and the next reconcile deliberately ignores a `labelsAdded` record for an id it did not + itself newly add (otherwise any re-labeling of any old message would manufacture an + ingest). So it stays `active`. + + This is not a regression — before this spec, *every* spam message stayed `active` — but + it means the feature must not be described as "Gmail's spam filtering now applies." + It applies to mail Gmail had classified by the time we read the history window, which + is the common case and not all cases. Closing the remainder is §5's reclassification + problem, not something the intake path can solve. - **IMAP** (built by omission): the client opens `INBOX`, so a server-side Junk move means the message is never fetched. The field is not supplied ⇒ `'unknown'`. This is the correct verdict: we genuinely do not know, because we never saw the message at all. diff --git a/src/mail/gmail-reconcile.test.ts b/src/mail/gmail-reconcile.test.ts index 3ac3a7d..4e2be63 100644 --- a/src/mail/gmail-reconcile.test.ts +++ b/src/mail/gmail-reconcile.test.ts @@ -713,7 +713,13 @@ describe('createGmailReconcileHandler', () => { expect((ingest.mock.calls[0][0] as RawInboundMessage).providerSpamVerdict).toBe('clean') }) - it("a message with no labelIds at all reports 'unknown' — Gmail omitted the field, so we assert nothing either way", async () => { + // `[]` IS the omitted-field case at this seam: the history client + // normalizes an absent `labelIds` to `[]` before this handler ever sees + // it (`../providers/adapters/gmail/history.ts` — "Defaults to [] when + // Gmail's response omits labelIds"), so `ListedMessage.labelIds` is + // always a present array. There is no separate `undefined` shape to test + // here; that normalization is the adapter's own contract. + it("an empty label set reports 'unknown' — the shape an omitted labelIds is normalized to, so we assert nothing either way", async () => { const { store: watchStateStore } = fakeWatchStateStore({ [MAILBOX_ID]: 'cursor-1' }) const historyClient = fakeHistoryClient({ listResult: { From 29b7d82ee6cd7ae0fd988bcb65f08348291467ca Mon Sep 17 00:00:00 2001 From: TJ Baker <1617679+zaridan@users.noreply.github.com> Date: Sun, 2 Aug 2026 14:34:21 -0700 Subject: [PATCH 4/4] docs(mail): record the maintainer's answers on D1 and D3 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both open decisions in the spam-classification spec are now made, quoted rather than paraphrased. D1 — automatic classification is on by default with an off switch ("on by default with an off switch"). §6 records the three consequences that follow: off means the verdict is not read at all rather than read and ignored; the Spam folder must distinguish "filed automatically" from "an Agent filed this", which nothing currently stores; and the switch's grain is per-mailbox. That last one is still INFERRED and marked so — it was not part of the answer, it is the author's reading, and it is a two-way door. D3 — no writeback ("no writeback"). This is a boundary rather than a deferral, so §5.3 states it as one: Helpthread reads the operator's mailbox and does not write classification state back into it. A correction changes what Helpthread believes, never what Google believes. The honest consequence is recorded with it: a rescued sender can keep arriving with a 'spam' verdict, because Google's opinion never changes. Per-mailbox sender allow-listing (§5.2) is what absorbs that, which makes it load-bearing rather than optional once §5 is built. With D3 answered "no", the spec no longer contains a one-way door in either its built or its specified surface — the only candidate was writing into the operator's own mailbox, and that door is now closed rather than walked through. The remaining INFERRED rows are all two-way. No code change; gates re-run anyway: tsc 0, biome 190 clean, src/mail 399 pass. Co-Authored-By: Claude Opus 5 --- specs/mail/spam-classification.md | 57 +++++++++++++++++++++++-------- 1 file changed, 43 insertions(+), 14 deletions(-) diff --git a/specs/mail/spam-classification.md b/specs/mail/spam-classification.md index 1245303..2bdb958 100644 --- a/specs/mail/spam-classification.md +++ b/specs/mail/spam-classification.md @@ -3,8 +3,10 @@ **Status: PARTIALLY BUILT.** §3.1 and §4 are implemented and tested (the provider verdict, Gmail's half of it, and the status decision at ingest). §3.2 (header-derived signals), §5 (the reclassification loop) and §6 (operator controls) are specified but -**not built** — and §5.3 and §6 carry open decisions the maintainer has not made. See -§7 for the decision ledger. +**not built**. Every blocking decision is now made — §5.3 (no writeback to the operator's +mailbox) and §6 (on by default, with an off switch) were answered by the maintainer on +2026-08-02 and are quoted in §7's ledger. What remains unbuilt is implementation work, +not open questions. Companion to [inbound-ingestion.md](./inbound-ingestion.md), whose three invariants this spec is subordinate to, and [threading.md](./threading.md), whose reply-token decision @@ -194,18 +196,40 @@ It is already recorded by `setStatus`. A sender an Agent has rescued from spam should not be re-classified on their next message. The intended mechanism is per-mailbox sender state, not a global list. -### 5.3 Feeding corrections back to the provider — OPEN +### 5.3 Feeding corrections back to the provider — DECIDED: no -Gmail's API can remove the `SPAM` label, which trains the operator's own filter. Doing so -means writing to the operator's mailbox in a way they did not directly ask for, on a -schedule they cannot see. **Not decided** (§7, D3). +Gmail's API can remove the `SPAM` label, which would train the operator's own filter. +**Helpthread does not do this** (maintainer, 2026-08-02: "no writeback"). Corrections stay +inside Helpthread's own database. + +This is a boundary, not a deferral: Helpthread reads the operator's mailbox and does not +write classification state back into it. A correction made here changes what Helpthread +believes, never what the operator's mail provider believes. An operator who wants Gmail +retrained does it in Gmail, where they can see it. + +Consequence to be honest about: the same sender can keep arriving with a `'spam'` verdict +after being rescued, because Google's opinion is unchanged. §5.2's per-mailbox sender +state is what absorbs that, and it is why allow-listing is not optional once §5 is built. ## 6. Operator controls (NOT BUILT) -Whether automatic classification is on by default, and whether it can be turned off, is -**not decided** (§7, D1). The rest of the surface follows from that answer: a settings -toggle, and whether the Spam folder shows a "filed automatically" affordance distinct -from "an Agent filed this." +**On by default, with an off switch** (maintainer, 2026-08-02: "on by default with an off +switch"). Automatic classification applies to a newly connected mailbox with no setup, and +an operator who does not want it can turn it off. + +Three consequences follow, and none are built yet: + +- **The switch is per-mailbox, not global.** Mailboxes are connected independently and an + operator may trust one provider's filtering and not another's. (INFERRED — the grain was + not specified in the decision above; per-mailbox is the author's reading, and it is a + two-way door.) +- **Off means the verdict is not read at all**, not that it is read and ignored. A message + arriving at a mailbox with classification off is filed `active` regardless of its label, + exactly as before this spec existed. +- **The Spam folder distinguishes "filed automatically" from "an Agent filed this."** + Without that, an operator cannot audit what the classifier did, and cannot tell a + mistake of theirs from a mistake of ours. The storage for this does not exist yet — + `status` alone does not record who set it. ## 7. Decision ledger @@ -217,11 +241,16 @@ source. Anything marked **INFERRED** is the author's judgment and has not been a | — | Build the fix plus a spec for real auto-classification, rather than the fix alone | Maintainer, 2026-08-02: "b" (in reply to the (a) fix-only / (b) fix-plus-spec choice) | | — | Spam is never auto-classified in v1 | Prior accepted spec, agent-inbox-v1.md §3a | | D0 | Junk mail is stored and filed as `spam`, never dropped at intake | ⚠️ INFERRED — follows from inbound-ingestion.md §1's never-dropped invariant, but applying it to junk specifically is the author's reading | -| D1 | Whether automatic spam classification is on by default, and whether an operator can turn it off | ⚠️ INFERRED — unresolved; §6 is unbuildable until answered | +| D1 | Automatic spam classification is **on by default**, and an operator can **switch it off** | Maintainer, 2026-08-02: "on by default with an off switch" | +| D1a | That switch is **per-mailbox** rather than global | ⚠️ INFERRED — the grain was not part of D1's answer. A two-way door | | D2 | A spam-verdict message whose reply token names a *deleted* conversation is filed as spam | ⚠️ INFERRED — the valid token argues it is a real customer; the deletion argues an Agent already discarded that thread. Uniform rule chosen for simplicity, not because the edge was decided | -| D3 | Whether an Agent's "not spam" correction is written back to the operator's Gmail | ⚠️ INFERRED — unresolved. Writing to the operator's mailbox unprompted is a one-way door of a kind the charter's data-ownership promise touches | +| D3 | An Agent's "not spam" correction is **never** written back to the operator's Gmail | Maintainer, 2026-08-02: "no writeback" | | D4 | Header scoring requires one strong or two weak signals; a lone weak signal never classifies | ⚠️ INFERRED — a conservative default, not a measured threshold | | D5 | `Auto-Submitted` is a loop-guard concern, not a spam signal | ⚠️ INFERRED | -**One-way door:** none in what is built. D3 would be one — it modifies state in the -operator's own mailbox, outside Helpthread's database. +**One-way door:** none, in what is built or specified. The only candidate was writing +classification state back into the operator's own mailbox, and D3 closed that door rather +than walking through it — Helpthread reads the operator's mail and does not write its own +opinions into it. Every remaining INFERRED row above (D0, D1a, D2, D4, D5) is a two-way +door: each changes a default or a threshold, and reversing any of them costs one edit and +no migration.