M4: Wire FilterEngine into live sync so matched rules actually fire #222

2026-07-29T23:51:18Z

KofTwentyTwo commented

2026-07-29 23:51:18 +00:00

(Migrated from github.com)

Summary

Closes #190. FilterEngine::evaluate was a real, well-tested match-and-act pipeline that was never called in production — rules could be created and validated but never fired against real synced mail. This PR wires it into the live inbound sync path.

Added apply_filter_actions(db, filter_engine, email) -> usize in crates/nunciod/src/sync.rs: evaluates email via FilterEngine::evaluate (already account-scoped) and routes every matched rule's actions to a real, persisted effect.
- MarkRead/MarkUnread apply immediately via DatabaseEngine::set_message_read.
- MoveTo/CopyTo/Flag/Unflag/Delete/ForwardTo/CallWebhook enqueue a real PendingRemoteMutation via OutboxManager::create_mutation + DatabaseEngine::save_pending_mutation, for the existing background outbox worker to eventually drain. This PR does not make the outbox worker perform the real IMAP/JMAP mutation or webhook HTTP call — that remains documented future work, exactly as the worker's own comment in main.rs already states.
- Every match appends a FilterExecutionLog entry via DatabaseEngine::save_filter_execution_log.
- A per-action DB write failure is logged via tracing::warn! and does not abort the rest of the sync or the remaining actions/rules, and is never counted in the returned "applied" count — no silent success on partial failure.
fetch_and_persist calls apply_filter_actions immediately after each db.save_email(&email).
FilterEngine is threaded through fetch_and_persist → sync_with_backend → run_account_sync / run_all_accounts_sync, and through the Mail gRPC service's Sync RPC (MailGrpcService gained a filter_engine: Arc<FilterEngine> field, mirroring how secrets/db are already shared).
main.rs clones the already-constructed filter_engine into the sync command consumer task and passes it to both run_all_accounts_sync/run_account_sync call sites.
Every existing sync.rs test call site now passes a FilterEngine (an empty_filter_engine() helper for tests that don't care about filtering).
No proto change — pure engine wiring, no client-visible API surface changes.

Reusable helper signature (for #193, bulk triage)

pub async fn apply_filter_actions(
    db: &nuncio_store::db::DatabaseEngine,
    filter_engine: &FilterEngine,
    email: &nuncio_core::model::Email,
) -> usize

Standalone in crates/nunciod/src/sync.rs, no dependency on the live inbound-sync call chain — takes only the primitives it needs, so #193's keyset-chunked bulk-triage scan can call it directly per message without re-deriving the evaluate-and-route logic.

New offline E2E tests (crates/nunciod/src/sync.rs)

All exercise sync_with_backend end-to-end with MockMailBackend — no live network:

sync_with_backend_fires_immediate_mark_read_action_on_match — a WHERE subject CONTAINS 'Urgent' ACTION MARK READ rule + a matching message: after sync, db.get_message(...) shows read == true, a FilterExecutionLog row exists, and list_pending_mutations stays empty (immediate action never touches the outbox).
sync_with_backend_enqueues_outbox_mutation_for_remote_action_on_match — a WHERE subject CONTAINS 'Archive Me' ACTION MOVE TO 'Archive' rule + a matching message: after sync, list_pending_mutations contains one pending row with mutation_type == "MOVE" and the Archive target in its payload, plus a matching FilterExecutionLog row; the message's read flag is untouched.
sync_with_backend_non_matching_message_produces_no_filter_side_effects — a non-matching message produces no read-flag change, no execution log, and no pending mutation (regression coverage for the negative case).

Gate evidence (local, Windows, rust-toolchain 1.97.1)

cargo fmt --all -- --check   -> clean, no diff
cargo check-all              -> clean, 0 clippy warnings, all targets
cargo test-all                -> 371 passed; 0 failed; 0 ignored (workspace-wide)

nunciod's own unit suite went from 100 to 103 tests (the 3 new sync.rs E2E cases above); every other crate's count is unchanged.

Decisions / notes for PO review

The outbox mutation_type tag vocabulary (MOVE, COPY, FLAG, UNFLAG, DELETE, FORWARD, WEBHOOK) is defined in a small remote_action_tag helper in sync.rs — nothing downstream currently parses it back into a RuleAction, so I picked short fixed tags rather than RuleAction::to_nsql()'s full NSQL text; happy to change if a different convention is preferred before #193/#194 start depending on it.
MailGrpcService gained a filter_engine field so the Sync RPC (used by spine_e2e_test.rs and other gRPC-level tests) also goes through the real filter pipeline, not just the sync.rs-level entry points named in the issue — this seemed necessary for the wiring to be genuinely complete end-to-end rather than only reachable from the background sync-command consumer.
Did not touch the outbox worker in main.rs; it still never marks a mutation "completed", per the issue's explicit scope boundary.

## Summary Closes #190. `FilterEngine::evaluate` was a real, well-tested match-and-act pipeline that was never called in production — rules could be created and validated but never fired against real synced mail. This PR wires it into the live inbound sync path. - Added `apply_filter_actions(db, filter_engine, email) -> usize` in `crates/nunciod/src/sync.rs`: evaluates `email` via `FilterEngine::evaluate` (already account-scoped) and routes every matched rule's actions to a real, persisted effect. - `MarkRead`/`MarkUnread` apply immediately via `DatabaseEngine::set_message_read`. - `MoveTo`/`CopyTo`/`Flag`/`Unflag`/`Delete`/`ForwardTo`/`CallWebhook` enqueue a real `PendingRemoteMutation` via `OutboxManager::create_mutation` + `DatabaseEngine::save_pending_mutation`, for the existing background outbox worker to eventually drain. This PR does **not** make the outbox worker perform the real IMAP/JMAP mutation or webhook HTTP call — that remains documented future work, exactly as the worker's own comment in `main.rs` already states. - Every match appends a `FilterExecutionLog` entry via `DatabaseEngine::save_filter_execution_log`. - A per-action DB write failure is logged via `tracing::warn!` and does not abort the rest of the sync or the remaining actions/rules, and is never counted in the returned "applied" count — no silent success on partial failure. - `fetch_and_persist` calls `apply_filter_actions` immediately after each `db.save_email(&email)`. - `FilterEngine` is threaded through `fetch_and_persist` → `sync_with_backend` → `run_account_sync` / `run_all_accounts_sync`, and through the `Mail` gRPC service's `Sync` RPC (`MailGrpcService` gained a `filter_engine: Arc<FilterEngine>` field, mirroring how `secrets`/`db` are already shared). - `main.rs` clones the already-constructed `filter_engine` into the sync command consumer task and passes it to both `run_all_accounts_sync`/`run_account_sync` call sites. - Every existing `sync.rs` test call site now passes a `FilterEngine` (an `empty_filter_engine()` helper for tests that don't care about filtering). - No proto change — pure engine wiring, no client-visible API surface changes. ## Reusable helper signature (for #193, bulk triage) ```rust pub async fn apply_filter_actions( db: &nuncio_store::db::DatabaseEngine, filter_engine: &FilterEngine, email: &nuncio_core::model::Email, ) -> usize ``` Standalone in `crates/nunciod/src/sync.rs`, no dependency on the live inbound-sync call chain — takes only the primitives it needs, so #193's keyset-chunked bulk-triage scan can call it directly per message without re-deriving the evaluate-and-route logic. ## New offline E2E tests (crates/nunciod/src/sync.rs) All exercise `sync_with_backend` end-to-end with `MockMailBackend` — no live network: - `sync_with_backend_fires_immediate_mark_read_action_on_match` — a `WHERE subject CONTAINS 'Urgent' ACTION MARK READ` rule + a matching message: after sync, `db.get_message(...)` shows `read == true`, a `FilterExecutionLog` row exists, and `list_pending_mutations` stays empty (immediate action never touches the outbox). - `sync_with_backend_enqueues_outbox_mutation_for_remote_action_on_match` — a `WHERE subject CONTAINS 'Archive Me' ACTION MOVE TO 'Archive'` rule + a matching message: after sync, `list_pending_mutations` contains one `pending` row with `mutation_type == "MOVE"` and the `Archive` target in its payload, plus a matching `FilterExecutionLog` row; the message's read flag is untouched. - `sync_with_backend_non_matching_message_produces_no_filter_side_effects` — a non-matching message produces no read-flag change, no execution log, and no pending mutation (regression coverage for the negative case). ## Gate evidence (local, Windows, rust-toolchain 1.97.1) ``` cargo fmt --all -- --check -> clean, no diff cargo check-all -> clean, 0 clippy warnings, all targets cargo test-all -> 371 passed; 0 failed; 0 ignored (workspace-wide) ``` nunciod's own unit suite went from 100 to 103 tests (the 3 new sync.rs E2E cases above); every other crate's count is unchanged. ## Decisions / notes for PO review - The outbox `mutation_type` tag vocabulary (`MOVE`, `COPY`, `FLAG`, `UNFLAG`, `DELETE`, `FORWARD`, `WEBHOOK`) is defined in a small `remote_action_tag` helper in `sync.rs` — nothing downstream currently parses it back into a `RuleAction`, so I picked short fixed tags rather than `RuleAction::to_nsql()`'s full NSQL text; happy to change if a different convention is preferred before #193/#194 start depending on it. - `MailGrpcService` gained a `filter_engine` field so the `Sync` RPC (used by `spine_e2e_test.rs` and other gRPC-level tests) also goes through the real filter pipeline, not just the sync.rs-level entry points named in the issue — this seemed necessary for the wiring to be genuinely complete end-to-end rather than only reachable from the background sync-command consumer. - Did not touch the outbox worker in `main.rs`; it still never marks a mutation "completed", per the issue's explicit scope boundary.

KofTwentyTwo commented

2026-07-30 00:02:51 +00:00

(Migrated from github.com)

Fixed the blocker: filters now fire exactly once per newly-arrived message.

Guard approach (option a): added `is_new_message(db, message_id)` in `sync.rs`, checked in `fetch_and_persist` before `db.save_email` runs (since `save_email` is `INSERT OR REPLACE` and would erase the "new vs. already-seen" distinction). It calls `db.get_message(id)` and returns true only on a genuine not-found; any other DB error is logged and conservatively treated as "not new" (never fires filters when existence can't be confirmed, to avoid duplicate remote mutations). apply_filter_actions is now only invoked when the message is genuinely new.

To support this without pulling sqlx into nunciod proper, added DatabaseError::is_not_found() to nuncio-store (mirrors the existing is_corrupt() helper) so nunciod can distinguish "not found" from other failures without a direct sqlx dependency.

New regression test: sync_with_backend_does_not_refire_filters_on_a_repeat_sync_of_the_same_message — syncs the same mock message twice against the same engine/db (two rules: an immediate MARK READ + a remote MOVE TO). After the first sync: read flag flips, 2 execution logs, 1 pending mutation. The message is then manually marked unread again (simulating a user un-reading it). After the second sync of the SAME message: read flag stays unread (no re-fire), still exactly 2 execution logs and 1 pending mutation (no new entries).

Also addressed the minor nit: the immediate-action match arm in apply_filter_actions is now exhaustive over the remote RuleAction variants instead of a _ => wildcard, so a future immediate-style action can't silently fall through to remote routing.

Documented the "fires once, on newly-arrived mail" design directly in apply_filter_actions's doc comment, explaining why sync's current non-incremental nature makes this guard necessary.

Gate (same branch, same commands):
```
cargo fmt --all -- --check -> clean
cargo check-all -> clean, 0 clippy warnings, all targets
cargo test-all -> all green, 0 ignored (nunciod unit suite: 104 tests, +1 for the new regression test)
```

Pushed as commit 766ca42 on story/m4-wire-filter-engine.

Fixed the blocker: filters now fire exactly once per newly-arrived message. **Guard approach (option a):** added \`is_new_message(db, message_id)\` in \`sync.rs\`, checked in \`fetch_and_persist\` **before** \`db.save_email\` runs (since \`save_email\` is \`INSERT OR REPLACE\` and would erase the "new vs. already-seen" distinction). It calls \`db.get_message(id)\` and returns true only on a genuine not-found; any other DB error is logged and conservatively treated as "not new" (never fires filters when existence can't be confirmed, to avoid duplicate remote mutations). `apply_filter_actions` is now only invoked when the message is genuinely new. To support this without pulling `sqlx` into `nunciod` proper, added `DatabaseError::is_not_found()` to `nuncio-store` (mirrors the existing `is_corrupt()` helper) so `nunciod` can distinguish "not found" from other failures without a direct `sqlx` dependency. **New regression test:** `sync_with_backend_does_not_refire_filters_on_a_repeat_sync_of_the_same_message` — syncs the same mock message twice against the same engine/db (two rules: an immediate `MARK READ` + a remote `MOVE TO`). After the first sync: read flag flips, 2 execution logs, 1 pending mutation. The message is then manually marked unread again (simulating a user un-reading it). After the second sync of the SAME message: read flag stays unread (no re-fire), still exactly 2 execution logs and 1 pending mutation (no new entries). Also addressed the minor nit: the immediate-action match arm in `apply_filter_actions` is now exhaustive over the remote `RuleAction` variants instead of a `_ =>` wildcard, so a future immediate-style action can't silently fall through to remote routing. Documented the "fires once, on newly-arrived mail" design directly in `apply_filter_actions`'s doc comment, explaining why sync's current non-incremental nature makes this guard necessary. **Gate (same branch, same commands):** \`\`\` cargo fmt --all -- --check -> clean cargo check-all -> clean, 0 clippy warnings, all targets cargo test-all -> all green, 0 ignored (nunciod unit suite: 104 tests, +1 for the new regression test) \`\`\` Pushed as commit 766ca42 on `story/m4-wire-filter-engine`.

KofTwentyTwo commented

2026-07-30 00:11:11 +00:00

(Migrated from github.com)

Merged `origin/dev` into this branch to pick up M3's real JMAP client and resolve the conflict.

Conflicted files: only `crates/nunciod/src/sync.rs` (both branches touched it — M3 rewired `build_mail_backend`'s JMAP arm to a real `JmapEngine::with_credentials` + rewrote the JMAP-backed sync tests to use wiremock stubs; this branch threaded `&FilterEngine` through the sync functions and added the filter E2E tests). No other files conflicted — `Cargo.lock`, `crates/nuncio-mail/{Cargo.toml,src/jmap.rs,tests/wiremock_jmap_test.rs}`, and `crates/nunciod/Cargo.toml` all merged cleanly (dev-only changes, nothing on this branch touched them).

Resolution: two conflict hunks, both inside test bodies:

`run_account_sync_persists_real_backend_messages_and_reports_success`: kept dev's wiremock-backed setup and its updated expected count (`assert_eq!(synced, 1)`, since the wiremock stub advertises 1 folder/1 message vs. the old static `JmapEngine`'s 3), combined with this branch's `filter_engine` parameter threaded into the `run_account_sync` call.
`run_all_accounts_sync_persists_across_multiple_accounts`: same pattern — kept dev's wiremock-server-per-account setup and updated count (`assert_eq!(total, 2)`), combined with this branch's `filter_engine` parameter.

`build_mail_backend` itself required no manual resolution — Git auto-merged it since the two branches touched disjoint regions (the JMAP arm from dev vs. `apply_filter_actions`/`is_new_message`/threaded `FilterEngine` params from this branch). Confirmed by inspection: the real `JmapEngine::with_credentials` arm is intact, and the fire-once-on-new-insert guard in `fetch_and_persist` is intact.

Gate on the merged result (same branch, same commands):
```
cargo fmt --all -- --check -> clean
cargo check-all -> clean, 0 clippy warnings, all targets
cargo test-all -> all green, 0 ignored
```
Notable counts: `nuncio-mail` unit suite 42 passed (up from M3's real-JMAP additions) + `wiremock_jmap_test.rs` 3 passed; `nunciod` unit suite 104 passed (unchanged from before the merge — the filter E2E tests and the JMAP wiremock changes live in different test functions and both pass together). No regressions from either side.

Pushed as merge commit `782cc47` on `story/m4-wire-filter-engine`. `gh pr view` now reports `mergeable: MERGEABLE` (no more conflict).

Merged \`origin/dev\` into this branch to pick up M3's real JMAP client and resolve the conflict. **Conflicted files:** only \`crates/nunciod/src/sync.rs\` (both branches touched it — M3 rewired \`build_mail_backend\`'s JMAP arm to a real \`JmapEngine::with_credentials\` + rewrote the JMAP-backed sync tests to use wiremock stubs; this branch threaded \`&FilterEngine\` through the sync functions and added the filter E2E tests). No other files conflicted — \`Cargo.lock\`, \`crates/nuncio-mail/{Cargo.toml,src/jmap.rs,tests/wiremock_jmap_test.rs}\`, and \`crates/nunciod/Cargo.toml\` all merged cleanly (dev-only changes, nothing on this branch touched them). **Resolution:** two conflict hunks, both inside test bodies: - \`run_account_sync_persists_real_backend_messages_and_reports_success\`: kept dev's wiremock-backed setup and its updated expected count (\`assert_eq!(synced, 1)\`, since the wiremock stub advertises 1 folder/1 message vs. the old static \`JmapEngine\`'s 3), combined with this branch's \`filter_engine\` parameter threaded into the \`run_account_sync\` call. - \`run_all_accounts_sync_persists_across_multiple_accounts\`: same pattern — kept dev's wiremock-server-per-account setup and updated count (\`assert_eq!(total, 2)\`), combined with this branch's \`filter_engine\` parameter. \`build_mail_backend\` itself required no manual resolution — Git auto-merged it since the two branches touched disjoint regions (the JMAP arm from dev vs. \`apply_filter_actions\`/\`is_new_message\`/threaded \`FilterEngine\` params from this branch). Confirmed by inspection: the real \`JmapEngine::with_credentials\` arm is intact, and the fire-once-on-new-insert guard in \`fetch_and_persist\` is intact. **Gate on the merged result (same branch, same commands):** \`\`\` cargo fmt --all -- --check -> clean cargo check-all -> clean, 0 clippy warnings, all targets cargo test-all -> all green, 0 ignored \`\`\` Notable counts: \`nuncio-mail\` unit suite 42 passed (up from M3's real-JMAP additions) + \`wiremock_jmap_test.rs\` 3 passed; \`nunciod\` unit suite 104 passed (unchanged from before the merge — the filter E2E tests and the JMAP wiremock changes live in different test functions and both pass together). No regressions from either side. Pushed as merge commit \`782cc47\` on \`story/m4-wire-filter-engine\`. \`gh pr view\` now reports \`mergeable: MERGEABLE\` (no more conflict).

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: KofTwentyTwo/nuncio#222