Rules: retire "All accounts" — per-account migration + scoping (#333 D1)

[CityDweller]

First of two PRs for the per-account Rules Manager (spec #334, approved). This is the data-touching half, deliberately isolated ahead of the window so the migration is an easy-to-review, easy-to-revert diff — per your call on #334. No window/UI changes here; that's PR B.

Migration — RuleService.LoadRules()

Implements D1 exactly as specced:

• Each unscoped rule (null AccountId) is duplicated into one rule per non-Graph account; Graph accounts get none (server rules replace client rules there, and an all-account client rule must not silently run on a Graph mailbox — D2).
• Copies get fresh Ids — reusing the source id across N copies would break selection and delete-by-id.
• Graph-only profile: the unscoped rule is dropped, with a per-rule log line (see release-note callout below).
• Idempotent by construction — the migration is "eliminate null AccountId", so the absence of any unscoped rule is the completion signal; a second run is a no-op. Runs in LoadRules() before any consumer sees the list, and persists once via the atomic SaveRules, so the file is fully migrated or untouched — never half-duplicated.
• Wired via an optional IAccountService on RuleService (supplied in App.xaml.cs; absent in unit tests that don't exercise migration, so existing call sites are unchanged).

RulesManagerViewModel

• "All accounts" removed from AccountOptions — a user can no longer create a new unscoped rule, which would reintroduce the null the migration just eliminated.
• New rules default to the first account.

ApplyRulesAsync's null-AccountId handling is intentionally left intact as defense in depth: the migration removes null from persisted data, but matching still treats a stray null as all-accounts rather than crashing.

⚠️ Release-notes callout required before the release that ships this

Per the spec, the Graph-only drop must be called out. It runs at startup the moment this merges (it's not behind the feature gate), so it needs to land in that version's notes. Suggested wording — tell me which version file to add it to, or I'll add it wherever you prefer:

Mail rules are now per-account. The "All accounts" rule option has been replaced by scoping each rule to a specific account. Existing "all accounts" rules are automatically copied to each of your standard (IMAP/SMTP) accounts. If you use only Microsoft 365 accounts, any old "all accounts" rules are removed, because those rules run inside QuickMail and Microsoft 365 accounts use server-side rules instead — recreate them per account, or as server rules, if needed.

Why this is safe

• IMAP users are untouched — their rules duplicate cleanly per account.
• The destructive path (drop) hits only a Microsoft-only profile carrying legacy all-account client rules — effectively nil, since Exchange only went default-on two days ago. Every drop is logged.

Tests

• +9 migration tests (RuleMigrationTests): duplication per non-Graph account with fresh ids, Graph excluded, Graph-only drop, already-scoped untouched, idempotency across two loads, disk persistence, the no-account-service path, and mixed scoped/unscoped.
• Updated two RulesManagerViewModelTests for the retired "All accounts" option.
• 92 rule-related tests green. Full suite 1577 passed; the lone failure is the known OpenClipboard Failed clipboard flake, which passes in isolation.

Next

PR B — the window: per-account selector, combined server+client list with "runs where" markers and group headings, the modeless conversion (your sign-off), the modeless editor, F6 ring, command palette, and App.xaml.cs DI wiring for IServerRuleService.

🤖 Generated with Claude Code

[kellylford]

Reviewed the migration end-to-end. Solid and well-isolated — the data-only scoping makes this easy to reason about and revert. Notes below; none are hard blockers.

Verified correct

• CloneForAccount copies every persisted MailRule field. The only two it omits — AccountDisplay, AccessibleName — are [JsonIgnore] display-only, correctly excluded. This is the main risk in a clone-based migration and it's clean.
• Fresh Id per copy (avoids the delete-by-id / selection collision).
• Idempotent by construction: keys off "any null AccountId remaining", persists once via the atomic SaveRules.
• IAccountService.LoadAccounts() and AccountModel.BackendKind / IsDefault / AccountLabel all exist as used.
• The two accessibility follow-up commits are sound, and the button row is correctly kept outside the IsEnabled=HasSelectedRule region so Close always works.

Relationship to main

• The non-Graph duplication + Graph-only drop design is the correct response to M365/Exchange going default-on in v0.8.35 (b26facb). The branch is based on top of that.
• Branch is behind main by exactly one commit — the docs-only #330 (90dda75). No code conflict; the "BEHIND" is cosmetic.

One risk worth a conscious decision (low probability, but this path deletes user data irreversibly)
The drop branch fires when targets.Count == 0. That can't distinguish a genuine Graph-only profile (intended drop) from a transient where LoadAccounts() returns empty for any reason. It only bites a profile that has rules but no/empty accounts.json — where rules are unusable anyway, and every drop is logged — so I'd call it acceptable. Flagging only because it's the sole path that removes rules.

Open items (from the PR body)

1. The Graph-only-drop release-notes callout is required, not optional — it runs at startup the moment this merges (not gated). Next release file is docs/release-notes-v0.8.36.md (already on main; csproj still says 0.8.35).
2. This is PR A; the per-account window is PR B (now up as #367, stacked on this branch).

Assessment: safe to approve once the release note lands.

[CityDweller]

Thanks — both addressed.

1. Release note (required). Added to docs/release-notes-v0.8.36.md, in the existing Rules Manager section: "All accounts" is retired; existing all-account rules are copied to each standard IMAP/SMTP account; Microsoft-365-only profiles have them removed (recreate per account; every removal logged); new rules pick an account.

2. The empty-accounts drop risk. Good catch — closed it. The migration now returns early when the account list is entirely empty, so the drop path only fires for a genuine Graph-only profile (accounts present, none non-Graph), never for a transient empty read (startup ordering, a locked/corrupt accounts.json). Test added (EmptyAccountList_SkipsMigration_KeepsUnscopedRule). This narrows the one data-deleting path to exactly the intended case.

The rest of your notes I took as confirmations (clone completeness, fresh ids, idempotency, the BEHIND being cosmetic). Migration tests 9/9; full suite green apart from the known clipboard flake.

Stack note for merge order: #364 → #367 → (editing, in progress). #367 is now gated behind a ServerRules flag (default off), so it's safe to merge whenever — the public sees nothing until the feature's complete.

[kellylford]

Both asks confirmed addressed — the early return on an entirely-empty account list is exactly the right narrowing, and EmptyAccountList_SkipsMigration_KeepsUnscopedRule pins it.

One thing had gone stale while this sat, so I fixed it on the branch rather than bouncing it back:

• Merged main in (a01f2d4). The branch had drifted from 1 behind to 5 behind — 0.8.36 shipped Jul 24 and the address-book work (Add the ability to search for mail to and from a contact from the address book #370–Contact mail results: close them and go back (#370 follow-up) #375) landed since. Clean merge, no code conflicts; the only textual overlap was in release-notes-v0.8.36.md.
• Moved the release note to 0.8.37 (2ff5c78). This is the important one: the note was in docs/release-notes-v0.8.36.md, but 0.8.36 was already released and main bumped to 0.8.37 in f2f83a82. As written, the Graph-only-removal warning would have shipped where no user would ever read it. It's now its own "Changed: mail rules are now per-account" section in docs/release-notes-v0.8.37.md — a heading rather than a bullet under the Rules Manager accessibility fixes, since a path that removes user rules deserves to be findable. Wording is yours, unchanged.

Verified locally on the merged tree: Release build clean, 1617/1617 tests pass (the OpenClipboard flake didn't reproduce).

Approving and merging. General note for the next stacked one: put the note in the next unreleased notes file and re-check it if the PR sits, since a release can ship underneath you.

[kellylford]

Approved. Release note is now in the right file, branch is current with main, CI green on all four checks (build, integration, CodeQL, Analyze), 1617/1617 locally on the merged tree.

[kellylford]

Approved, and all four checks are green on the merged head — but it can't actually merge yet, and it's my own fault:

The SubmitRequirements ruleset has require_last_push_approval: true. Because I pushed the last two commits (the main merge and the release-note move), my approval no longer satisfies it — GitHub won't count an approval from whoever pushed last. Repo auto-merge is off, so it can't queue either.

@CityDweller — cleanest unblock is you review and approve the two commits I added (a01f2d4 merge of main, 2ff5c78 moving your release note from the shipped 0.8.36 file to 0.8.37). That's the right outcome anyway: you should get eyes on a change made to your PR, and your approval satisfies the last-push rule. Once it's in, I'll merge with a merge commit so #367 stays a clean descendant of this branch and needs only a retarget rather than a rebase.

If you'd rather own the docs commit yourself, say so and I'll drop mine — no attachment to it.