feat(mail): carry the provider's spam verdict through intake - #180
Merged
Conversation
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 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughGmail labels now produce provider spam verdicts. Inbound ingestion uses these verdicts to set the status of new conversations while preserving existing conversation status and storing all messages. ChangesProvider Spam Verdict Ingestion
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Gmail
participant GmailReconcile
participant InboundIngest
participant ConversationStore
Gmail->>GmailReconcile: Message labels
GmailReconcile->>InboundIngest: RawInboundMessage with providerSpamVerdict
InboundIngest->>ConversationStore: Create new conversation with active or spam status
InboundIngest->>ConversationStore: Append message without changing existing status
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/mail/gmail-reconcile.test.ts (1)
737-759: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStrengthen the self-echo-and-spam test to assert
getRawMessagewas never called.This test spies on
getRawMessagebut only checks thatingestwas not called. Add an assertion thatgetRawMessagewas not called. This directly verifies the ordering claim in the module doc: the self-echo filter runs beforemessages.get.✅ Proposed test strengthening
expect(result).toEqual({ kind: 'ack' }) expect(ingest).not.toHaveBeenCalled() + expect(getRawMessage).not.toHaveBeenCalled() expect(setCalls).toEqual([{ mailboxId: MAILBOX_ID, historyId: 'cursor-2' }])🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/mail/gmail-reconcile.test.ts` around lines 737 - 759, Strengthen the test for the self-echo and SPAM message by asserting that the existing getRawMessage spy was never called, alongside the ingest assertion. Keep the test’s current result and watch-state expectations unchanged.src/mail/ingest.ts (1)
439-463: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider logging the spam verdict on stored outcomes.
raw.providerSpamVerdict ?? 'unknown'correctly defaults an absent verdict. ThelogIngestEventcall right after does not include the verdict or the resulting conversation status. Add it to make spam-verdict distribution and misclassifications observable without a database query.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/mail/ingest.ts` around lines 439 - 463, The stored-outcome log emitted after storeAndMarkDelivered should include the normalized spam verdict and resulting conversation status. Reuse raw.providerSpamVerdict ?? 'unknown' and the status returned by storeAndMarkDelivered when constructing the logIngestEvent payload, preserving existing fields and fallback behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@specs/mail/spam-classification.md`:
- Around line 16-17: Update the human support staff references in the
spam-classification specification, including the cited sections around the
status-setting and sender-rescue flows, from Actor to Agent. Reserve Assistant
for AI actors and ensure the terminology is consistent throughout the affected
prose.
- Around line 137-147: Clarify the status contract in
specs/mail/spam-classification.md lines 137-147 and specs/api/agent-inbox-v1.md
lines 114-119: provider verdicts must not alter an existing conversation’s
status, except that the normal reopen rule may change spam to active for valid
replies; newly created conversations must be active unless the provider verdict
is spam. Ensure both sections use consistent wording.
---
Nitpick comments:
In `@src/mail/gmail-reconcile.test.ts`:
- Around line 737-759: Strengthen the test for the self-echo and SPAM message by
asserting that the existing getRawMessage spy was never called, alongside the
ingest assertion. Keep the test’s current result and watch-state expectations
unchanged.
In `@src/mail/ingest.ts`:
- Around line 439-463: The stored-outcome log emitted after
storeAndMarkDelivered should include the normalized spam verdict and resulting
conversation status. Reuse raw.providerSpamVerdict ?? 'unknown' and the status
returned by storeAndMarkDelivered when constructing the logIngestEvent payload,
preserving existing fields and fallback behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 74ee2011-eae1-4af2-abc1-05ab75958cd7
📒 Files selected for processing (10)
specs/api/agent-inbox-v1.mdspecs/mail/inbound-ingestion.mdspecs/mail/spam-classification.mdsrc/mail/gmail-reconcile.test.tssrc/mail/gmail-reconcile.tssrc/mail/ingest.test.tssrc/mail/ingest.tssrc/providers/inbound-email.tssrc/providers/index.tssrc/store/conversations.ts
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 <noreply@anthropic.com>
Contributor
Author
|
@coderabbitai full review |
✅ Action performedFull review finished. Your included review limit is currently reached under our Fair Usage Limits Policy. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 39 minutes. |
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 <noreply@anthropic.com>
Contributor
Author
Review substitution: adversarial Codex-class pass in place of CodeRabbitCodeRabbit's check reads Adversarial pass: GPT-5.2 (via OpenRouter), prompted with this repo's sacred invariants (never drop mail; threading decided only by our own signed token; parse exactly once; an Agent's placement is never silently overruled) and directed at seven specific hunting grounds including Gmail label semantics, delta-stream ordering, and vacuous tests. 3 findings — 1 partly real (fixed), 1 real but out of scope (tracked), 1 wrong (name clarified).
Finding 1 is the valuable one, and it changed what this PR claims: the feature is now described as best-effort with a stated boundary, not as "Gmail's spam filtering now applies." Gates on |
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 <noreply@anthropic.com>
This was referenced Aug 2, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🟢 SAFE TO MERGE
Gates green. Both blocking decisions answered by the maintainer and quoted below. No one-way doors. CodeRabbit: 3 findings, 3 real, 3 fixed — then rate-limited, so an adversarial GPT-5.2 pass substituted for the later heads: 3 findings, 1 partly real (fixed), 1 out of scope (tracked), 1 wrong.
Decision provenance
specs/api/agent-inbox-v1.md§3aspam, never dropped at intakeAuto-Submittedis a loop-guard concern, not a spam signalD0, D1a, D2, D4 and D5 are conservative defaults, each cheap to overturn.
One-way door: none. 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 is a two-way door: each changes a default or a threshold, and reversing any costs one edit and no migration.
D3's honest consequence, recorded in §5.3: 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.The defect this closes
Gmail classifies every message before it reaches us, and the verdict was being discarded.
history.listis deliberately not label-filtered (src/providers/adapters/gmail/history.ts) — filtering there races label application and would drop real mail. But the only label check downstream was the self-echo filter (DRAFT, orSENTwithoutINBOX). A message Google had already put inSPAMcarries none of those, so it became an ordinaryactiveconversation in the inbox.An operator who connects a Gmail mailbox reasonably expects Gmail's spam filtering to still apply. It did not.
IMAP was unaffected by construction — the client opens
INBOX, so a server-side Junk move means the message is never fetched. That asymmetry between the two intake paths was accidental; it is now documented.What changes
RawInboundMessage.providerSpamVerdict('spam' | 'clean' | 'unknown') carries the transport's own conclusion across the provider boundary, never re-derived.spamVerdictOfmaps Gmail's labels onto it. Ingest files a brand-new conversation asspamrather thanactivewhen it says so.Three properties are load-bearing, each with tests:
'spam'verdict changes one column. The message is parsed, stored, threaded and attachment-linked identically either way (inbound-ingestion.md §1 invariant HT-7: mail-behavior acceptance fixtures #3).'spam'is evidence.'clean','unknown'and an absent field all meanactive.Ordering: the verdict is read only 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. Asserted, including that no raw fetch is wasted on it.
NewConversation.statusis narrowed to'active' | 'spam'— a conversation is never bornclosed,pending, ordeleted.The reopen interaction, spelled out
Review surfaced a real composition nobody had written down. §4.2 now carries it as a table; the counter-intuitive row is the third:
active'spam'active— unchangedspam'spam'active— reopenedA message the provider called junk, replying to a conversation we filed as junk, still reopens it — the reply token proves we wrote to that address first. Both
spamrows are covered by tests. The practical consequence: a false positive self-heals the moment the sender replies, with nobody opening the Spam folder.What this does NOT do
Stated because the feature must not be oversold.
history.listis a delta stream. ASPAMlabel applied after a reconcile window's snapshot never reaches us — that message is already ingested asactive, and the next reconcile deliberately ignores label deltas for ids it did not itself newly add. So it staysactive.Not a regression (before this, every spam message stayed
active), but this is not "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. Closing the remainder is §5's reclassification problem, which is specified and not built.Mail semantics
This touches the charter's conversation-integrity invariant, so, explicitly: no message's storage, parsing, or threading outcome changes. The threading decision is
decideThreading's alone and is not read by any code in this diff. The only behavioural delta is thestatuscolumn on rows this pipeline creates, plus one new optional field on the provider seam. Existing ingest and reconcile suites pass unchanged.Review — CodeRabbit substituted, disclosed
CodeRabbit did not review this PR. Its check reads
pass, but on the first head it reportedReview rate limited(citing the Fair Usage Limits Policy) and on subsequent headsReview skipped: incremental reviews are disabled. Green is not a review.It did produce one real review on the original head (
2420ea9) before hitting its limit — 3 findings, all 3 real, all 3 fixed inf8cf328: a test that asserted too little to prove its own ordering claim, anActor/Agentvocabulary slip, and the status-contract ambiguity that turned out to be hiding the reopen interaction documented above.For the later heads, per the repo's protocol, an adversarial GPT-5.2 pass (via OpenRouter) stood in — prompted with this repo's sacred invariants and directed at seven specific hunting grounds including Gmail label semantics, delta-stream ordering, and vacuous tests. 3 findings — 1 partly real (fixed), 1 real but out of scope (tracked), 1 wrong (name clarified). Full adjudication is in a PR comment; its most valuable finding is what produced the "What this does NOT do" section above.
The two commits after that pass (
49598e0,29b7d82) are prose plus one test-name string — zero executable logic — so every line of code that runs on this branch has been through an adversarial pass. Verifiable:git diff f8cf328..HEAD -- src/is a single renamed test title.Tests
src/mail/gmail-reconcile.test.ts— 4 new: SPAM carries'spam'; INBOX carries'clean'; an empty label set carries'unknown'; a SPAM+SENT self-echo is still skipped with no raw fetch.src/mail/ingest.test.ts— 6 new, against a real migrated database: each verdict's resulting status, an omitted field, a spam-verdict reply that must not re-file its target, and a spam-verdict reply onto an already-spam conversation that must still reopen it.Unrelated findings, tracked not folded in
src/store/gmail-watch-state.test.tsis flaky on cleanmain— 3 of 4 runs failed in an untouched checkout, in the lease block. A gate that fails half the time will eventually mask a real regression.TRASH-labeled mail files asactive. Real, pre-existing, and a normative decision in its own right.🤖 Generated with Claude Code