fix(email-triage): don't surface newsletters as approval cards when triage 529s#363
fix(email-triage): don't surface newsletters as approval cards when triage 529s#363nolanmak wants to merge 1 commit into
Conversation
When the triage model returned a transient 529 (surfacing as non-JSON text), process_email logged a status='error' action row and returned early — before the DecisionKind::Reply arm where the #217 is_human_sender gate lives. That row then (1) tripped has_open_action, permanently blocking the poll loop from re-triaging the email, and (2) was picked up by list_retryable_replies -> retry_once -> dispatch_reply, which force-drafted an empty body and posted an approval card with NO automated-sender gate. Result: newsletters/marketing (substack.com, patagonia, etc.) whose first triage hit a 529 leaked through as approval cards — every leaked card had an empty (length-0) draft body, the signature of the retry path. The retry queue is for drafting-stage failures (triage already decided reply, a real draft exists, create_draft/post_approval failed), not triage-stage failures. A triage-stage failure has run no gates and has no draft, so it must be re-triaged from scratch. Drop the log_action(Error) call in the parse-failure branch: with no action row the email stays unread/unprocessed and the next poll re-runs full triage + all gates — identical to how a network Err from reasoner.call already behaves. Adds triage_parse_failure_does_not_create_retryable_action, reproducing the production scenario (a substack.com sender whose triage 529s) through the real process_email + retry_once paths: asserts no action row, no retryable reply, no card, and a re-triage skip.
📝 WalkthroughWalkthrough
ChangesTriage parse failure retry handling
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 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 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/augmentagent-channel-email/src/channel.rs (1)
2477-2503: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the full no-state/no-Discord contract.
This test’s comments say “NO action row” and “no Discord posting,” but it only checks no open action and no approval-card posts. Add checks for zero action rows after pass 1 and zero flag notices so the regression can’t slip through via a non-open action or flag notice.
Suggested test hardening
assert!( !store.has_open_action("m-529").unwrap(), "a triage-stage failure must NOT create an action row" ); + let action_rows = store + .with_conn(|c| { + c.query_row( + "SELECT COUNT(*) FROM actions WHERE messageId = 'm-529'", + [], + |r| r.get::<_, i64>(0), + ) + }) + .unwrap(); + assert_eq!(action_rows, 0, "triage parse failure must not create any action row"); assert!(!store.is_email_complete("m-529").unwrap()); assert_eq!(broker.posts.lock().unwrap().len(), 0); + assert_eq!(broker.flag_posts.lock().unwrap().len(), 0); // --- Retry tick: with no errored row, nothing is retryable, so the leak // path (retry_once -> dispatch_reply, ungated) never runs. let retried = ch.retry_once().await.unwrap(); @@ let out2 = ch.poll_once().await.unwrap(); assert_eq!(out2.skipped, 1, "re-triage routes the automated sender to the skip gate"); assert_eq!(out2.awaiting_approval, 0); assert_eq!(broker.posts.lock().unwrap().len(), 0); + assert_eq!(broker.flag_posts.lock().unwrap().len(), 0); assert!(store.is_email_complete("m-529").unwrap());🤖 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 `@crates/augmentagent-channel-email/src/channel.rs` around lines 2477 - 2503, This test only asserts that there is no open action and no approval-card post, but it does not fully enforce the no-state/no-Discord contract. Strengthen the assertions around the existing retry and poll flow in channel.rs by checking that the store has zero action rows after pass 1 and that no flag notices were created or posted, using the same store/broker handles already exercised by ch.retry_once() and ch.poll_once().
🤖 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.
Nitpick comments:
In `@crates/augmentagent-channel-email/src/channel.rs`:
- Around line 2477-2503: This test only asserts that there is no open action and
no approval-card post, but it does not fully enforce the no-state/no-Discord
contract. Strengthen the assertions around the existing retry and poll flow in
channel.rs by checking that the store has zero action rows after pass 1 and that
no flag notices were created or posted, using the same store/broker handles
already exercised by ch.retry_once() and ch.poll_once().
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c474a128-11f3-4f36-99c6-74dd2bb34626
📒 Files selected for processing (1)
crates/augmentagent-channel-email/src/channel.rs
Symptom
Newsletter / marketing emails (Substack, Patagonia, etc.) were surfacing as Discord approval "draft" cards even though the
is_human_senderfilter (#217) is supposed to skip them. The filter itself works — it correctly skipped 8 GitHub notification emails the same day.Root cause
The leak only triggers when the first triage call hits a transient Claude
529 Overloaded(which surfaces as non-JSON text, soparse_decisionfails). On that path:process_emaillogged astatus='error'action row and returned before theDecisionKind::Replyarm — the only placeis_human_sender/is_event_blastrun.has_open_action(matchesstatus IN ('pending','error')), permanently blocking the poll loop from re-triaging the email.list_retryable_replies(WHERE status='error', no decision-kind filter) intoretry_once→dispatch_reply, which force-drafts an empty body and posts an approval card with no automated-sender gate.The retry queue was only ever meant for drafting-stage failures (triage already decided
reply, a real draft exists,create_draft/post_approvalfailed). A triage-stage failure has run no gates and has no draft, so treating it as a retryable reply is wrong for junk and humans.Evidence from the live DB: every leaked pending card had a draftBody length of 0 (no real draft ever generated), while every legitimate human reply had a non-empty draft. Logs showed
triage parse failed: 529 Overloadedfollowed ~one retry-interval later byapproval card posted.Fix
Drop the
log_action(Error)call in the triage parse-failure branch. With no action row written, the email stays unread/unprocessed and the next poll re-runs full triage + all gates — identical to how a networkErrfromreasoner.call(the?one line above) already behaves. Fixes both the junk leak and the empty-draft brokenness in one edit.Testing
triage_parse_failure_does_not_create_retryable_actionreproduces the exact production scenario (asubstack.comsender whose first triage 529s) through the realprocess_email+retry_oncepaths and asserts: no action row, nothing retryable, no approval card, and a re-triage skip.augmentagent-channel-emailsuite: 86 passed / 0 failed. Both existing retry tests stay green (the fix only touches the triage-stage branch, not the drafting-stage error paths). Release build clean.Deliberately out of scope (follow-ups)
pendingdlen=0 rows in the livedata.db(operational, not a code change).dispatch_replyitself (belt-and-suspenders; not needed for correctness once triage-stage failures stop entering the retry queue).Summary by CodeRabbit
Bug Fixes
Tests