fix(safety): harden dedup planning and deletion consent

[salmonumbrella]

What changed

• Dedup scans now plan missing RFC822 Message-ID derivations without writing. Confirmed execution applies the exact content-bound plan atomically, rescans, and stops before merging if the actionable duplicate set changed.
• Duplicate discovery and fetch now use the same Message-ID rules across SQLite and PostgreSQL, including bare/bracketed legacy values and malformed byte edge cases.
• Remote deletion can be enabled durably in the invoking CLI config or for one command with the existing environment variable. A daemon's own config and environment never grant consent to a remote caller.
• Identifier comparison now states the existing invariant directly: email-shaped values compare case-insensitively; other identifier shapes preserve case.

Why

Dedup preview could write derived metadata before confirmation, outside the backup boundary. Remote deletion only had a one-command environment gate, which was awkward for headless clients and ambiguous across the CLI/daemon boundary. The identifier comparison rule was correct in code but easy for the next caller to bypass. This keeps preview read-only, makes confirmed writes fail closed, and keeps destructive consent explicit and client-owned.

Usage

Remote deletion remains disabled by default. To opt in durably:

[deletion]
remote_enabled = true

For one command:

MSGVAULT_ENABLE_REMOTE_DELETE=1 msgvault delete-staged

The dedup command and flags are unchanged; its confirmation now includes any planned Message-ID derivations.

Closes #312
Closes #314
Closes #315

[roborev-ci]

roborev: Combined Review (6c8e73d)

Verdict: Two medium-severity performance issues were identified; otherwise, reviewers found the changes sound.

Medium

• internal/dedup/dedup.go:1091 — Every dedup execution performs a second complete Scan, even when no RFC822 backfill is pending, doubling expensive archive-wide planning work. Skip the rescan and plan comparison unless metadata derivation was attempted.

• internal/store/dedup.go:171-190 — Duplicate discovery groups by a computed CASE/SUBSTR expression instead of the indexed rfc822_message_id column, potentially causing full expression scans and temporary grouping structures on large archives. Add a matching expression index or maintain an indexed canonical Message-ID column while preserving malformed-ID semantics.

---

Reviewers: 2 done | Synthesis: codex, 8s | Total: 15m2s

[roborev-ci]

roborev: Combined Review (7a28a45)

No issues found.

---

Reviewers: 2 done | Synthesis: codex | Total: 14m29s

[roborev-ci]

roborev: Combined Review (a246fc8)

Verdict: One medium-severity deduplication consistency issue remains.

Medium

• internal/store/dedup.go:949: Backfill normalization strips all leading/trailing < and > characters, while duplicate discovery preserves malformed Message-IDs such as <<foo>>. A missing-ID copy may be stored as foo instead of matching the existing message, causing duplicates to be missed. Use consistent structural canonicalization for backfill or reject malformed Message-IDs.

---

Reviewers: 2 done | Synthesis: codex, 7s | Total: 13m19s

[roborev-ci]

roborev: Combined Review (1c5f44f)

Verdict: One medium-severity issue found; otherwise, reviewers found no significant concerns.

Medium

• Unbounded memory use during backfill planning — internal/store/dedup.go:901-954
  The planner retains every derivable message item in memory, which can cause excessive memory usage or OOM for large archives. Consider spilling plans to durable temporary storage or processing validated batches while preserving the plan digest.

---

Reviewers: 2 done | Synthesis: codex, 7s | Total: 17m40s

[roborev-ci]

roborev: Combined Review (a0b01f9)

Verdict: One medium-severity deduplication issue requires attention; no other findings were reported.

Medium

• internal/dedup/dedup.go:995-1000, 1050-1055, 1100-1128 — With --content-hash, a duplicate group containing one stored Message-ID and one derivable missing-ID message can be reclassified from normalized-hash to message-id after backfill. Because actionablePlansEqual compares KeyType and Key, execution rejects the unchanged destructive operation, requiring the user to rerun deduplication. Compare post-backfill plans by survivor/loser IDs and remote deletion targets instead.

---

Reviewers: 2 done | Synthesis: codex, 7s | Total: 24m2s

[roborev-ci]

roborev: Combined Review (e2adbe3)

Verdict: No Medium, High, or Critical findings identified; code is clean at the required severity threshold.

---

Reviewers: 2 done | Synthesis: codex, 5s | Total: 11m51s

[wesm]

looking

[wesm]

Implemented the review follow-ups. Deduplication now stops and asks the user to rerun if the approved Message-ID work no longer matches the archive. It will not go on to hide messages when that update is stale or affects no rows.

Other changes:

• Malformed IDs in angle brackets no longer count as evidence that two messages are duplicates. Import threading keeps its old behavior, so an upgrade does not split existing conversations.
• When raw email source is available, its Message-ID must agree with the stored value. A missing, unreadable, malformed, or different header no longer counts as proof of a duplicate. A recoverable matching header still works even if the rest of the MIME message is malformed.
• I replaced the overloaded backfilled_count field with an explicit pending_backfill_count field. The CLI and daemon now reject incompatible versions instead of showing incorrect consent text.
• The command records whether it is a daemon subprocess at startup. A daemon crash can no longer make the subprocess inherit permission from the server config.
• Messages with missing IDs are read in bounded batches through one shared query. I also removed unused backfill code and split oversized functions and tests.
• PostgreSQL now reports a derived Message-ID containing a NUL (zero) byte as a failed candidate before approval. SQLite keeps its existing behavior because it can store that value.
• The PostgreSQL lock-order test now checks the actual database locks. This avoids a false timeout caused by the database driver waiting to deliver query rows.

I kept all derived-ID writes in one transaction. I also kept source parsing in the approval scan because the approval must match the exact source data. Per-message reads and unnecessary rescans are gone.

[roborev-ci]

roborev: Combined Review (ef4b6b2)

Verdict: One medium-severity issue remains; reviewers otherwise found no concrete security vulnerability.

Medium

• internal/dedup/dedup.go:818 — Legacy stored Message-IDs are trusted when raw MIME parsing fails or the raw message lacks a Message-ID because messageIDChecked remains false. This can incorrectly merge messages based on stale metadata and stage them for deletion. Treat missing, malformed, or unparseable raw Message-ID data as unverifiable and exclude the message, using recovery/header parsing where appropriate.

---

Reviewers: 2 done | Synthesis: codex, 11s | Total: 17m58s

[roborev-ci]

roborev: Combined Review (5d64e68)

Verdict: One medium-severity PostgreSQL backfill issue remains; otherwise, no concrete security weaknesses were found.

Medium

• NUL bytes in MIME Message-ID can abort PostgreSQL backfill — internal/store/dedup.go:1170-1175, internal/mime/message_id.go:28
  NUL bytes survive normalization and are counted as ready for backfill, but PostgreSQL TEXT values cannot contain NUL. Applying the plan aborts the entire transaction instead of classifying the candidate as failed. Reject NUL-containing derived IDs for PostgreSQL while preserving SQLite’s existing behavior.

---

Reviewers: 2 done | Synthesis: codex, 7s | Total: 20m27s

[roborev-ci]

roborev: Combined Review (80103cf)

Verdict: One medium-severity compatibility issue found; no high or critical issues identified.

Medium

• internal/importer/ingest.go:220 — Replacing legacy Message-ID normalization with strict normalization changes threading for malformed IDs such as <<legacy@example.test>>, potentially splitting existing conversations after re-import. Preserve legacy normalization for importer threading and use strict normalization only for deduplication/backfill validation.

---

Reviewers: 2 done | Synthesis: codex, 6s | Total: 21m45s

[wesm]

Follow-up on the latest automated review: importer thread normalization has not changed in this PR. internal/importer/ingest.go matches main, and the regression test confirms both legacy cases: <> falls back to the raw-message hash, while <<legacy@example.test>> still produces legacy@example.test. No additional code change is needed for that finding.