M4: Fix filter header-field and account-field evaluation bugs #191

New Issue

2026-07-28T14:04:30Z

KofTwentyTwo commented

2026-07-28 14:04:30 +00:00

(Migrated from github.com)

Context

Story for milestone M4. Read docs/STORY-WORKFLOW.md first — it defines the process, gates, and patterns; this issue defines only the specific work. Two confirmed, distinct correctness bugs live in CompiledFilter::eval_leaf (crates/nuncio-filter/src/engine.rs, lines 81-155), the function that evaluates a single NSQL WHERE-clause leaf condition against an Email:

Line 100-102: FilterField::Folder | FilterField::Account => Self::eval_string_op(&email.folder_id, ...). A rule-level ON ACCOUNT 'x' scope (handled separately and correctly by FilterRule::matches_account, crates/nuncio-filter/src/ast.rs line 281) is fine — but this is the account condition field used INSIDE a WHERE clause (e.g. WHERE account = 'acct-a'), and it silently compares against email.folder_id instead of email.account_id. A rule like WHERE account = 'acct-a' AND subject CONTAINS 'x' today matches based on which FOLDER the message is in, not which ACCOUNT it belongs to — a real logic bug a user would never suspect from the rule's own text.
Line 150-153: FilterField::Header(_name) => { /* Header evaluation fallback */ false }. This is not a real evaluation at all — it always returns false regardless of the actual header value, silently making any rule with a header[...] condition impossible to ever match. Root cause: nuncio_core::model::Email (crates/nuncio-core/src/model.rs) has NO field carrying parsed MIME headers at all — there is genuinely nothing to evaluate against yet, so a real per-header match is not achievable without a materially bigger change (adding a headers map to the domain model, populating it in MimeParserAdapter/the IMAP fetch path and the JMAP Email/get properties list, and persisting it in the store). That is out of scope for this correctness-fix story.

Goal

Fix the account condition field to read email.account_id (not email.folder_id). For the header[...] field, since real matching genuinely cannot be done honestly without a domain-model change that is out of scope here, make it fail loudly and honestly at rule-creation/validation time instead of silently evaluating to false at runtime — a user should get a clear "not yet supported" error when they try to create or validate a rule containing a header[...] condition, rather than a rule that persists successfully and then never, ever matches (a much worse failure mode: silent, permanent, unexplained). Do not implement fake header matching (e.g. matching against Subject/From as a stand-in) — that would be exactly the kind of fabricated behaviour the project's anti-theater rule forbids.

Files to touch / patterns to mirror

crates/nuncio-filter/src/engine.rs — CompiledFilter::eval_leaf (lines 81-155): split the FilterField::Folder | FilterField::Account combined match arm (line 100) into two separate arms; Folder keeps &email.folder_id, Account changes to &email.account_id. Update the FilterField::Header(_name) arm's comment (lines 150-153) to explain it is an unreachable defensive fallback (validation now rejects header conditions before a rule can ever reach evaluation), not a real feature.
crates/nuncio-filter/src/validator.rs — NsqlValidator::pass1_field_types (lines 70-108): currently FilterField::Header(_) is grouped with Subject/From/To/Body/Folder/Account in the arm accepting FilterValue::String/List (lines 86-98). Split it out into its own arm that always returns Err(ValidationError::FieldTypeMismatch(...)) with a message stating header-field conditions are not yet supported (message headers are not indexed in the current data model). This is checked by NsqlValidator::validate, which is already called from CreateRule/ValidateRule/PreviewRule in crates/nunciod/src/grpc.rs (FiltersGrpcService::create_rule/validate_rule/preview_rule) — so this single change makes ALL three RPCs (and the CLI commands backed by them) honestly reject a header[...] rule instead of silently accepting one that can never fire.
crates/nuncio-filter/src/engine.rs's #[cfg(test)] mod tests (bottom of file) — existing account-scoping tests (e.g. test_evaluate_scopes_rule_to_its_owning_account, lines 417-432) test the RULE-level ON ACCOUNT scope via FilterRule::matches_account, which is unaffected by this bug and already correct — do not confuse these with the new regression test needed for the account CONDITION field bug.

Implementation steps

In engine.rs, change the eval_leaf match: FilterField::Folder => Self::eval_string_op(&email.folder_id, ...) and FilterField::Account => Self::eval_string_op(&email.account_id, ...) as two distinct arms.
Add a regression test in engine.rs: build a rule via NsqlParser::parse_rule with NSQL like "WHERE account = 'acct-a' ACTION MARK READ", then assert it matches a message with account_id: "acct-a" regardless of folder_id, and does NOT match a message with account_id: "acct-b" even if that message happens to share the same folder_id as an acct-a message (this is the exact scenario the bug produced wrongly before the fix — prove the fix by constructing that specific "same folder, different account" collision).
In validator.rs, split FilterField::Header(_) out of pass1_field_types's combined string-type arm into its own arm returning Err(ValidationError::FieldTypeMismatch("header field conditions are not yet supported: message headers are not indexed".to_string())) (or similar wording).
Add a regression test in validator.rs: NsqlParser::parse_rule(..., "WHERE header['X-Spam-Score'] = '10' ACTION MARK READ") then NsqlValidator::validate(&rule, &ValidationOptions::default()) must return Err(ValidationError::FieldTypeMismatch(_)).
Update engine.rs's eval_leaf FilterField::Header(_name) => false arm's doc comment to state it is unreachable in practice (validation rejects the field before a rule persists) and is only a defensive fallback, not a real evaluation — do not delete the arm itself, since eval_leaf must remain exhaustive over FilterField.
Check crates/nunciod/src/grpc.rs's existing create_rule_rejects_invalid_nsql_and_persists_nothing/validate_rule_reports_honest_ok_and_error_verdicts_without_persisting/preview_rule_and_validate_rule_reject_invalid_nsql_syntax tests (grep for them) — none currently construct a header[...] NSQL string, so they should be unaffected, but re-run them to confirm.
Run cargo verify.

Acceptance criteria

WHERE account = '<id>' matches only messages of that account (email.account_id), never messages that merely share a folder_id.
Regression test proves the fix specifically distinguishes "same folder, different account" (the exact bug scenario).
NsqlValidator::validate rejects any rule containing a header[...] WHERE-clause condition with ValidationError::FieldTypeMismatch, so CreateRule/ValidateRule/PreviewRule (and their CLI equivalents) honestly refuse such a rule at creation/validation time instead of silently persisting a rule that can never match.
eval_leaf's FilterField::Header fallback comment is updated to describe it as an unreachable defensive branch, not a real feature — no fake header matching is introduced.
No proto change.
cargo verify green (fmt + clippy -D warnings all-targets + test-all), 0 ignored.

Definition of done & process

Follow docs/STORY-WORKFLOW.md: no fabricated data; comment convention (intent-only, no story/GH/phase refs in .rs/.proto); mock-only tests. Branch story/m4-filter-correctness off dev; PR to dev with the gate output pasted as evidence; the PO reviews before the issue is closed.

Dependencies / notes

No proto coordination needed; this is a self-contained engine/validator fix, independent of M4-S1/M4-S3/M4-S4 (can land before, after, or in parallel with any of them — it touches only nuncio-filter, not the sync path or the gRPC surface). Real per-header matching (adding a headers map to nuncio_core::model::Email, populating it from MIME parsing and JMAP properties, and persisting it) is real follow-up work worth filing as its own future story if header-based filtering becomes a real requirement — do not attempt it here. Size: M.

## Context Story for milestone M4. **Read [`docs/STORY-WORKFLOW.md`](https://github.com/KofTwentyTwo/nuncio/blob/dev/docs/STORY-WORKFLOW.md) first** — it defines the process, gates, and patterns; this issue defines only the specific work. Two confirmed, distinct correctness bugs live in `CompiledFilter::eval_leaf` (`crates/nuncio-filter/src/engine.rs`, lines 81-155), the function that evaluates a single NSQL `WHERE`-clause leaf condition against an `Email`: 1. Line 100-102: `FilterField::Folder | FilterField::Account => Self::eval_string_op(&email.folder_id, ...)`. A rule-level `ON ACCOUNT 'x'` scope (handled separately and correctly by `FilterRule::matches_account`, `crates/nuncio-filter/src/ast.rs` line 281) is fine — but this is the `account` *condition field* used INSIDE a `WHERE` clause (e.g. `WHERE account = 'acct-a'`), and it silently compares against `email.folder_id` instead of `email.account_id`. A rule like `WHERE account = 'acct-a' AND subject CONTAINS 'x'` today matches based on which FOLDER the message is in, not which ACCOUNT it belongs to — a real logic bug a user would never suspect from the rule's own text. 2. Line 150-153: `FilterField::Header(_name) => { /* Header evaluation fallback */ false }`. This is not a real evaluation at all — it always returns `false` regardless of the actual header value, silently making any rule with a `header[...]` condition impossible to ever match. Root cause: `nuncio_core::model::Email` (`crates/nuncio-core/src/model.rs`) has NO field carrying parsed MIME headers at all — there is genuinely nothing to evaluate against yet, so a real per-header match is not achievable without a materially bigger change (adding a headers map to the domain model, populating it in `MimeParserAdapter`/the IMAP fetch path and the JMAP `Email/get` properties list, and persisting it in the store). That is out of scope for this correctness-fix story. ## Goal Fix the `account` condition field to read `email.account_id` (not `email.folder_id`). For the `header[...]` field, since real matching genuinely cannot be done honestly without a domain-model change that is out of scope here, make it fail loudly and honestly at rule-creation/validation time instead of silently evaluating to `false` at runtime — a user should get a clear "not yet supported" error when they try to create or validate a rule containing a `header[...]` condition, rather than a rule that persists successfully and then never, ever matches (a much worse failure mode: silent, permanent, unexplained). Do not implement fake header matching (e.g. matching against `Subject`/`From` as a stand-in) — that would be exactly the kind of fabricated behaviour the project's anti-theater rule forbids. ## Files to touch / patterns to mirror - `crates/nuncio-filter/src/engine.rs` — `CompiledFilter::eval_leaf` (lines 81-155): split the `FilterField::Folder | FilterField::Account` combined match arm (line 100) into two separate arms; `Folder` keeps `&email.folder_id`, `Account` changes to `&email.account_id`. Update the `FilterField::Header(_name)` arm's comment (lines 150-153) to explain it is an unreachable defensive fallback (validation now rejects header conditions before a rule can ever reach evaluation), not a real feature. - `crates/nuncio-filter/src/validator.rs` — `NsqlValidator::pass1_field_types` (lines 70-108): currently `FilterField::Header(_)` is grouped with `Subject`/`From`/`To`/`Body`/`Folder`/`Account` in the arm accepting `FilterValue::String`/`List` (lines 86-98). Split it out into its own arm that always returns `Err(ValidationError::FieldTypeMismatch(...))` with a message stating header-field conditions are not yet supported (message headers are not indexed in the current data model). This is checked by `NsqlValidator::validate`, which is already called from `CreateRule`/`ValidateRule`/`PreviewRule` in `crates/nunciod/src/grpc.rs` (`FiltersGrpcService::create_rule`/`validate_rule`/`preview_rule`) — so this single change makes ALL three RPCs (and the CLI commands backed by them) honestly reject a `header[...]` rule instead of silently accepting one that can never fire. - `crates/nuncio-filter/src/engine.rs`'s `#[cfg(test)] mod tests` (bottom of file) — existing account-scoping tests (e.g. `test_evaluate_scopes_rule_to_its_owning_account`, lines 417-432) test the RULE-level `ON ACCOUNT` scope via `FilterRule::matches_account`, which is unaffected by this bug and already correct — do not confuse these with the new regression test needed for the `account` CONDITION field bug. ## Implementation steps 1. In `engine.rs`, change the `eval_leaf` match: `FilterField::Folder => Self::eval_string_op(&email.folder_id, ...)` and `FilterField::Account => Self::eval_string_op(&email.account_id, ...)` as two distinct arms. 2. Add a regression test in `engine.rs`: build a rule via `NsqlParser::parse_rule` with NSQL like `"WHERE account = 'acct-a' ACTION MARK READ"`, then assert it matches a message with `account_id: "acct-a"` regardless of `folder_id`, and does NOT match a message with `account_id: "acct-b"` even if that message happens to share the same `folder_id` as an `acct-a` message (this is the exact scenario the bug produced wrongly before the fix — prove the fix by constructing that specific "same folder, different account" collision). 3. In `validator.rs`, split `FilterField::Header(_)` out of `pass1_field_types`'s combined string-type arm into its own arm returning `Err(ValidationError::FieldTypeMismatch("header field conditions are not yet supported: message headers are not indexed".to_string()))` (or similar wording). 4. Add a regression test in `validator.rs`: `NsqlParser::parse_rule(..., "WHERE header['X-Spam-Score'] = '10' ACTION MARK READ")` then `NsqlValidator::validate(&rule, &ValidationOptions::default())` must return `Err(ValidationError::FieldTypeMismatch(_))`. 5. Update `engine.rs`'s `eval_leaf` `FilterField::Header(_name) => false` arm's doc comment to state it is unreachable in practice (validation rejects the field before a rule persists) and is only a defensive fallback, not a real evaluation — do not delete the arm itself, since `eval_leaf` must remain exhaustive over `FilterField`. 6. Check `crates/nunciod/src/grpc.rs`'s existing `create_rule_rejects_invalid_nsql_and_persists_nothing`/`validate_rule_reports_honest_ok_and_error_verdicts_without_persisting`/`preview_rule_and_validate_rule_reject_invalid_nsql_syntax` tests (grep for them) — none currently construct a `header[...]` NSQL string, so they should be unaffected, but re-run them to confirm. 7. Run `cargo verify`. ## Acceptance criteria - [ ] `WHERE account = '<id>'` matches only messages of that account (`email.account_id`), never messages that merely share a `folder_id`. - [ ] Regression test proves the fix specifically distinguishes "same folder, different account" (the exact bug scenario). - [ ] `NsqlValidator::validate` rejects any rule containing a `header[...]` `WHERE`-clause condition with `ValidationError::FieldTypeMismatch`, so `CreateRule`/`ValidateRule`/`PreviewRule` (and their CLI equivalents) honestly refuse such a rule at creation/validation time instead of silently persisting a rule that can never match. - [ ] `eval_leaf`'s `FilterField::Header` fallback comment is updated to describe it as an unreachable defensive branch, not a real feature — no fake header matching is introduced. - [ ] No proto change. - [ ] `cargo verify` green (fmt + clippy `-D warnings` all-targets + test-all), 0 ignored. ## Definition of done & process Follow [`docs/STORY-WORKFLOW.md`](https://github.com/KofTwentyTwo/nuncio/blob/dev/docs/STORY-WORKFLOW.md): no fabricated data; comment convention (intent-only, no story/GH/phase refs in `.rs`/`.proto`); mock-only tests. Branch `story/m4-filter-correctness` off `dev`; PR to `dev` with the gate output pasted as evidence; the PO reviews before the issue is closed. ## Dependencies / notes No proto coordination needed; this is a self-contained engine/validator fix, independent of `M4-S1`/`M4-S3`/`M4-S4` (can land before, after, or in parallel with any of them — it touches only `nuncio-filter`, not the sync path or the gRPC surface). Real per-header matching (adding a headers map to `nuncio_core::model::Email`, populating it from MIME parsing and JMAP properties, and persisting it) is real follow-up work worth filing as its own future story if header-based filtering becomes a real requirement — do not attempt it here. Size: M.

KofTwentyTwo commented

2026-07-30 00:19:39 +00:00

(Migrated from github.com)

Delivered via PR #223 (squash-merged to dev). account condition field now reads email.account_id (was folder_id) with a same-folder/different-account regression test; header[...] conditions are now honestly rejected at validation time (FieldTypeMismatch) across CreateRule/ValidateRule/PreviewRule instead of silently never matching. No fake header matching; real per-header support is deferred to a future story (needs a headers map on the domain model).

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: KofTwentyTwo/nuncio#191