fix(mail): file Gmail TRASH as spam instead of live support work - #182
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Review limit reached
Next review available in: 10 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
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 |
Adversarial review pass — adjudicatedSubstitution disclosed: CodeRabbit was not used. This PR's base is 6 findings — 1 real and fixed, 1 rejected, 4 confirmed non-defects.
Finding 1 — verified independently, then accepted with a recordI checked the mechanism rather than taking the reviewer's word: The overapproximation is not new (mark-as-junk-then-not-junk has always had it), but Accepted, not fixed in code ( This adds a second INFERRED row, so the verdict stays 🟡. D8 was never put to the maintainer as its own question. Gates re-run on |
…rits it The verdict table defines 'clean' as an affirmative judgment, but the Gmail implementation derives it from a bare absence of the SPAM label. Those are different claims, and the gap is invisible today only because §4.1 files 'clean' and 'unknown' identically. They collapse for Gmail because Gmail classifies every message it accepts, so absence of the junk marker IS the verdict. Record that as the specific licence it is, bind future transports to earning 'clean' the same way, and note that §3.2's header scoring is what makes the distinction load-bearing. No behaviour change. Decision D6 added to the §7 ledger.
A Gmail message carrying TRASH fell through spamVerdictOf to 'clean' and was created as an ordinary `active` conversation. Mail the operator's own mailbox had already thrown away — by a delete-on-arrival filter, or by hand before the reconcile window ran — became visible support work in the inbox. TRASH reaches the handler for the same reason SPAM does: history.list is an unfiltered delta stream and the client set-unions labelsAdded without ever REMOVING a label, so both ['TRASH'] and ['INBOX','TRASH'] arrive intact. spamVerdictOf now maps either label to 'spam'. This widens what that value asserts — from 'a classifier called this junk' to 'the source mailbox has already discarded this as unwanted' — so §3.1's definition widens with it rather than letting the field lie. Nothing downstream branches on which of the two produced the verdict; the only thing either may change is the status a brand-new conversation is born at. Nothing is dropped (inbound-ingestion.md §1 invariant #3): the message is parsed, stored, threaded and attachment-linked identically, sits readable in the Spam folder, and a reply reopens it to `active` (§4a) — so a message trashed in error self-heals. Not a regression from the spam-verdict work; before it, every ingested message became `active` regardless of labels. Decisions D7 and D7a added to the §7 ledger.
Adversarial review of the TRASH mapping surfaced a real one: the history client requests messageAdded and labelAdded, never labelRemoved, so labelIds is the set of labels a message has EVER carried in the window rather than its state now. Delete a just-arrived message, hit undo, and TRASH stays in the union — the conversation is filed spam while sitting in the operator's Inbox. The overapproximation is not new (mark-as-junk then not-junk has always had it), but TRASH extends it to a one-click action Gmail offers to undo, so the exposure is materially larger and should not go unrecorded. Accepted rather than fixed: reading labelRemoved would re-open the SENT/INBOX split-delta race the union was introduced to close, and the misfile is bounded and self-correcting — fully stored, readable in the Spam folder, and reopened to active by any reply. Decision D8 added to the §7 ledger, marked INFERRED.
D7a — a message carrying both SPAM and TRASH is filed as spam — was put as its own question and answered (maintainer decision, 2026-08-07), rather than being left to follow mechanically from D7. §7's closing sentence enumerated the remaining INFERRED rows but omitted D8, which this same section introduces.
9cfb957 to
3b2c802
Compare
|
@coderabbitai full review |
|
Adjudication — 2 findings, both declined with reasonCodeRabbit is rate limited ("Your included review limit is currently reached") and has never produced a review on this PR — the check reads Findings
Both findings come from a criterion in my own brief ("every added case must fail against the old implementation") that is right for regression tests and wrong for decision-pinning tests. Neither is a defect; no code changed in response. Invariants — all clean
Full gate re-run locally after the rebase, exit codes read directly: |
A message deleted and then restored inside one reconcile window stays filed as spam; the label union is not corrected by reading labelRemoved (maintainer decision, 2026-08-07). Reading labelRemoved would re-open the split-delta race the union was introduced to close. §7's remaining-INFERRED list returns to D0, D1a, D2, D4, D5.
1 participant
🟢 SAFE TO MERGE
Gates green on this head (typecheck 0, lint 0, 2018 tests / 106 files, exit 0), re-run after the rebase. No unanswered decisions — D7a and D8 were both put to the maintainer and answered on 2026-08-07. Codex (adversarial, in place of CodeRabbit — rate limited, no review ever produced on this PR): 2 findings, 0 real, both declined with reason.
Rebased onto the current
main, which now includes #193's comment sweep over the same three mail files. No conflicts; both sides verified present afterwards.What changed
spamVerdictOfmapped aTRASH-labeled Gmail message to'clean', soingestcreated it as an ordinaryactiveconversation. Mail the operator's own mailbox had already discarded — by a delete-on-arrival filter, or by hand before the reconcile window ran — became live support work.TRASHreaches the handler for the same reasonSPAMdoes:history.listis an unfiltered delta stream, and the history client set-unionslabelsAddedwithout ever removing a label. So both['TRASH']and['INBOX','TRASH']arrive intact.Three files: one condition in
src/mail/gmail-reconcile.ts(SPAMorTRASH⇒'spam') plus the doc comment explaining why an operator action and a classifier verdict are nonetheless collapsed;specs/mail/spam-classification.md(§3.1's widened definition, the'clean'honesty gap closed in prose, §7 gains D6/D7/D7a/D8); and four cases insrc/mail/gmail-reconcile.test.ts.Decision provenance
spam" from three options (separate signal filed asclosed/ file asspam/ leave as-is)No one-way doors. Nothing writes to the operator's mailbox (D3 is "no writeback"), no schema change, no public promise altered. Filing is reversible in the UI and self-heals on reply. D8 is a two-way door: reversing it costs one edit and no migration.
Invariants held
inbound-ingestion.md§1). ATRASHmessage is parsed, stored, threaded and attachment-linked exactly as before — one column differs.createConversationInTxsites; a reply threading onto an existing conversation never reads it (§4.2).SENT+TRASHmessage is skipped before the verdict is computed — our own deleted reply never becomes a spam conversation.INBOXonly, so a server-side delete means the message is never fetched; the field stays'unknown', which is the honest verdict.activeregardless of labels.Known limit, unchanged
A
TRASHlabel applied after the reconcile window'shistory.listsnapshot is not covered — that message is already ingested asactive, and the next reconcile deliberately ignoreslabelsAddedfor an id it did not itself newly add. Deleting a message you have already seen arrive in Helpthread does not retroactively file it. That is §5's reclassification problem, recorded in the spec rather than left as a surprise.