Identities, Collections, and Deduplication

[jesserobbins]

Summary

Implements the identity, collection, and deduplication system from #278.

New commands

• list-identities — auto-discover sent-from addresses across accounts using three signals (From header, OAuth, config); prints likely identities for configuring the [identity].addresses list.
• collections — manage named source groups (create, list, show, add, remove, delete); a default All collection is seeded automatically and tracks every source.
• deduplicate — find and merge duplicates across sources for the same account.

deduplicate behavior

• Groups by RFC822 Message-ID; optional --content-hash also groups by normalized raw MIME.
• Picks a survivor (honoring --prefer <source-types> and identity-based sent-copy preference), unions labels, soft-deletes the pruned copies.
• Read-only by default (--dry-run). --apply required to write.
• Reversible: every run writes a per-source manifest; --undo <batch-id> restores hidden rows. --undo is repeatable to undo multiple batches.
• Atomic snapshot before merge via SQLite VACUUM INTO (skip with --no-backup).
• --delete-dups-from-source-server additionally stages pruned copies for remote deletion (destructive, opt-in; remote sources only).

Query layer

• SourceIDs filter propagated through DuckDB and SQLite query paths, plus soft-delete exclusion everywhere.
• TUI account selector lists collections and filters results to the collection's sources.

Closes #278.

[roborev-ci]

roborev: Combined Review (e1e5be4)

I’ll consolidate the reports into one PR-ready comment, keeping only medium-or-higher findings and merging anything duplicated.
Medium-severity issues remain around DuckDB soft-delete filtering, default collection initialization, and TOML escaping.

Medium

• internal/query/duckdb.go:682
  DuckDB aggregate/stat/filter query builders do not add deleted_at IS NULL, while SQLite paths now do. Until the cache is rebuilt, soft-deleted duplicates can still appear in DuckDB-backed aggregates, stats, and filtered views.
  Fix: Add the same soft-delete predicate to DuckDB buildWhereClause, buildFilterConditions, GetTotalStats, and search condition paths that read message rows.

• internal/store/collections.go:57
  The default "All" collection is only created when EnsureDefaultCollection is called, but this change does not wire it into schema/store initialization or source creation. The CLI documents "All" as automatic, but normal installs can have no default collection, and newly created sources will not join it unless callers remember to invoke this method.
  Fix: Call EnsureDefaultCollection during schema initialization and ensure source creation/upsert paths add new sources to "All".

• cmd/msgvault/cmd/list_identities.go:112
writeIdentitiesTOML manually escapes only double quotes before printing archive-derived email addresses into a TOML config snippet. A crafted From: address containing a newline, backslash escape, or TOML syntax could break out of the string and inject additional config when pasted.
  Fix: Generate TOML with a TOML encoder, or use a correct TOML string escaping routine that handles quotes, backslashes, control characters, CR/LF, and non-printable bytes. Consider validating identity candidates as syntactically valid email addresses before including them in paste-ready config output.

---

Synthesized from 3 reviews (agents: codex, gemini | types: default, security)

[roborev-ci]

roborev: Combined Review (3080a0f)

I’ll merge these into one PR-review comment, deduplicating overlap and dropping the low-severity item as requested.
Summary verdict: High and Medium issues remain; the PR is not ready to merge until side effects, scoping, compile, and manifest path handling are fixed.

High

• internal/dedup/dedup.go:184: Engine.Scan mutates the database before confirmation and during --dry-run by backfilling rfc822_message_id. It also performs the backfill globally through CountMessagesWithoutRFC822ID / BackfillRFC822IDs, even when deduplication is scoped to a specific account or collection.

  Fix: Make scan side-effect-free for dry runs and pre-confirmation flows, or move the backfill into the confirmed execution path. Scope the backfill/count to AccountSourceIDs.

• internal/dedup/dedup.go in sanitizeAccount: Invalid rune literal ' @' will fail compilation with invalid character literal (more than one character).

  Fix: Change case r == ' @' || r == '.': to case r == '@' || r == '.':.

Medium

• cmd/msgvault/cmd/deduplicate.go:112: A resolved collection with zero sources is treated the same as no --account, so deduplicate --account <empty-collection> deduplicates every source independently instead of doing nothing or erroring.

  Fix: Track whether --account was explicitly provided and return an error when a resolved collection/source scope expands to zero source IDs.

• internal/query/source_filter.go:14: Empty SourceIDs falls through as “no filter,” so an explicitly empty collection passed into query/list/stat paths can show all accounts instead of no results.

  Fix: Distinguish nil from explicitly empty multi-source filters and add a 1=0 condition for explicitly empty scopes.

• internal/dedup/dedup.go:615 and cmd/msgvault/cmd/deduplicate.go:203: batchID can include a raw source identifier in per-source dedup mode, and that value is later embedded into manifest.ID and used in filepath.Join(mgr.PendingDir(), manifest.ID+".json"). Source identifiers are user-controlled and may contain path separators, allowing manifest writes outside the intended pending directory or into unintended nested paths when --delete-dups-from-source-server is used.

  Fix: Sanitize the full manifest ID before using it as a filename, or construct manifest filenames only from a restricted character set. Also verify the cleaned path remains under mgr.PendingDir() after joining.

---

Synthesized from 3 reviews (agents: codex, gemini | types: default, security)

[roborev-ci]

roborev: Combined Review (b2aa98c)

Summary verdict: Medium-risk empty-scope and collection workflow issues remain; no Critical or High findings were reported.

Medium

• internal/dedup/dedup.go:188
Engine.Scan passes an empty AccountSourceIDs slice through to store duplicate queries as unscoped, so the documented empty/default config can group duplicates across all sources instead of strict per-source dedup. This can merge messages from different accounts for non-CLI callers.
  Fix: Reject empty AccountSourceIDs in Scan, or implement true per-source grouping when empty; reserve cross-source grouping for explicit source scopes.

• cmd/msgvault/cmd/list_identities.go:50
  When --account resolves to a collection with zero sources, scope.SourceIDs() expands to an empty slice and ListLikelyIdentities(scopeIDs...) treats that as unscoped, returning identities from every source instead of none. Empty collections can occur after collections remove.
  Fix: Detect len(scopeIDs) == 0 after resolving an account/collection and return no candidates or a clear “zero sources” error; alternatively update the store API to distinguish empty scoped lists from unscoped calls.

• cmd/msgvault/cmd/collections.go
runCollectionsCreate and runCollectionsAdd only update collection associations and exit. The spec/implementation plan says creating a collection or adding accounts should automatically trigger deduplication.
  Fix: Invoke the dedup engine scoped to the collection’s sources after successful create/add operations.

---

Synthesized from 3 reviews (agents: codex, gemini | types: default, security)

[jesserobbins]

Fixed (5 total):

• --account zero-source guard in deduplicate
• --account zero-source guard in list-identities
• batchID path traversal sanitization
• Empty SourceIDs nil-vs-empty distinction in query filter
• Missing deleted_at IS NULL in DuckDB GetGmailIDsByFilter

Disagree with the following:

• Invalid rune ' @' — code was already correct
• Scan backfill mutates during dry-run — idempotent enrichment, required for
  results
• Scan empty AccountSourceIDs for non-CLI callers — speculative, CLI handles
  correctly
• Collections create/add should auto-trigger dedup — dedup is a separate
  explicit operation
• EnsureDefaultCollection not wired in — already called in InitSchema
• TOML escaping — already uses proper toml.NewEncoder

[roborev-ci]

roborev: Combined Review (52f1cd4)

Deduplicated verdict: PR has 2 Medium findings around deduplication scope and unintended mutation; no High or Critical findings.

Medium

• internal/dedup/dedup.go:204: Engine.Scan backfills rfc822_message_id before honoring dry-run behavior, and the backfill runs across the whole database even for scoped --account scans. This means msgvault deduplicate --dry-run or account-scoped scans can still mutate unrelated rows before confirmation or backup.

  Fix: Make the backfill respect DryRun, and pass AccountSourceIDs through CountMessagesWithoutRFC822ID / BackfillRFC822IDs so only scoped sources are touched.

• internal/dedup/dedup.go:233: The engine contract says empty AccountSourceIDs means per-source deduplication, but Scan passes an empty source list to store methods, where it means “all sources.” A direct engine caller using the default config can therefore merge duplicates across accounts, contradicting the documented safety rule.

  Fix: Require a non-empty scope for engine scans, or implement true per-source grouping when AccountSourceIDs is empty.

---

Synthesized from 3 reviews (agents: codex, gemini | types: default, security)

[jesserobbins]

roborev: Combined Review (52f1cd4)

Deduplicated verdict: PR has 2 Medium findings around deduplication scope and unintended mutation; no High or Critical findings.

Medium

• internal/dedup/dedup.go:204: Engine.Scan backfills rfc822_message_id before honoring dry-run behavior, and the backfill runs across the whole database even for scoped --account scans. This means msgvault deduplicate --dry-run or account-scoped scans can still mutate unrelated rows before confirmation or backup.
  Fix: Make the backfill respect DryRun, and pass AccountSourceIDs through CountMessagesWithoutRFC822ID / BackfillRFC822IDs so only scoped sources are touched.

Dry run, in my opinion, shouldn't do anything that writes or mutates... including backup. It should clearly disclose what it would have done, and what it can't do because of a potential mutate.

• internal/dedup/dedup.go:233: The engine contract says empty AccountSourceIDs means per-source deduplication, but Scan passes an empty source list to store methods, where it means “all sources.” A direct engine caller using the default config can therefore merge duplicates across accounts, contradicting the documented safety rule.
  Fix: Require a non-empty scope for engine scans, or implement true per-source grouping when AccountSourceIDs is empty.

Okay, I agree with that.

[roborev-ci]

roborev: Combined Review (b5c3b86)

Verdict: One medium-severity issue should be fixed before merging; no critical or high findings were reported.

Medium

• internal/config/config.go:132: IdentityAddressSet can insert blank or whitespace-only addresses into the identity set. That can make duplicate candidates with an empty FromEmail appear to match a configured identity, causing unknown or blank senders to influence survivor selection incorrectly.
  • Fix: Trim addresses first, skip empty values, then lowercase and insert them.

---

Synthesized from 3 reviews (agents: codex, gemini | types: default, security)

[roborev-ci]

roborev: Combined Review (5086043)

I’ll consolidate the review outputs into one PR-ready comment, keeping only medium-or-higher findings and merging overlaps without inventing missing line numbers.
Verdict: Changes need fixes before merge: one build-breaking issue and several medium-risk dedup/MIME behavior problems remain.

Critical

None.

High

• internal/dedup/dedup.go, in SanitizeFilenameComponent
  • case r == ' @' || r == '.': appears to use an invalid multi-character rune literal, which will break the Go build.
  • Fix: Change ' @' to '@'.

Medium

• internal/dedup/dedup.go:215

  • Scan backfills rfc822_message_id during non-dry-run mode before the CLI confirmation prompt, so aborting deduplication can still leave database changes behind.
  • Fix: Keep Scan read-only, prompt before backfill, or parse raw MIME in memory for reporting.

• internal/dedup/dedup.go:209

  • Dry-run skips RFC822 backfill entirely, so it can report fewer duplicate groups than the subsequent real run will merge.
  • Fix: Make dry-run compute candidate Message-IDs from raw MIME without persisting them.

• internal/dedup/dedup.go, in normalizeRawMIME

  • Header boundary detection checks for \r\n\r\n before \n\n; if LF-only headers contain CRLF delimiters later in the body, the body can be misclassified as headers.
  • Fix: Find both delimiter indices and use the smallest non-negative match.

• internal/dedup/normalize_test.go and internal/store/dedup_test.go

  • Several raw MIME test literals appear to use broken line endings like rn instead of \r\n, plus obfuscated email addresses such as alice @example.com, which can break or mask MIME parsing behavior.
  • Fix: Restore proper MIME line endings and valid email addresses in test fixtures.

---

Synthesized from 3 reviews (agents: codex, gemini | types: default, security)

[roborev-ci]

roborev: Combined Review (ef35ac0)

Medium: UndoDedup does not fully reverse merge side effects.

Medium

• internal/store/dedup.go:383 - UndoDedup only clears deleted_at and delete_batch_id. It does not undo labels copied to the survivor or raw MIME backfilled during MergeDuplicates, so undo can leave altered query/filter behavior behind.
  • Suggested fix: record per-batch merge side effects and reverse them during undo, or make the undo behavior explicitly limited to restoring hidden duplicate rows.

---

Synthesized from 3 reviews (agents: codex, gemini | types: default, security)

[roborev-ci]

roborev: Combined Review (19caff5)

Deduplication feature has two Medium issues that should be addressed before merge.

Medium

• [internal/dedup/dedup.go:204] Engine.Scan mutates the database during a normal scan by backfilling rfc822_message_id before CLI confirmation and before backup creation. If the user declines the prompt, the database has still changed, and the backup cannot restore the true pre-scan state.

  • Fix: Keep Scan read-only; move the backfill into the confirmed execution path after backup creation, or make it a separate confirmed preflight/backfill step.

• [internal/dedup/dedup.go:259] The content-hash fallback excludes messages already found in an RFC822 duplicate group, which misses transitive duplicates where two copies share a Message-ID and a third has a rewritten or missing Message-ID but identical normalized MIME.

  • Fix: Include primary-pass survivors in the content-hash pass and merge overlapping duplicate groups, for example with a union-find over message IDs.

---

Synthesized from 3 reviews (agents: codex, gemini | types: default, security)

[roborev-ci]

roborev: Combined Review (4c5ff50)

One medium-severity issue remains: survivor selection can ignore archived_at when only one candidate has it.

Medium

• internal/dedup/dedup.go:775 - isBetter only compares ArchivedAt when both timestamps are non-zero. If the current survivor has a zero ArchivedAt and a later candidate has a real timestamp, the function falls through to lower ID instead of applying the documented “earlier archived_at” tiebreaker. This can make survivor choice depend on row insertion order when only one side has archived_at.

  Suggested fix: Define explicit ordering for zero timestamps, such as treating non-zero ArchivedAt as better than zero before comparing actual timestamp values.

---

Synthesized from 3 reviews (agents: codex, gemini | types: default, security)

[wesm]

Haven't forgotten about this, will look at this soon!

[wesm]

via Codex

Thanks for putting this together. The direction is useful, especially the reversible local dedup flow, survivor selection, batch IDs, and the idea of default/user-defined collections. Before this goes further, I think we should align on the long-term data model and scope semantics, because several parts of this branch seem to encode a different model than I think msgvault should use.

My current understanding of the model we want is:

• Account: one ingest source/archive. Examples: one Gmail sync source, one IMAP source, one mbox import, one Apple Mail import, one iMessage import.
• Collection: a named grouping of accounts/sources. Examples: All, work, personal, old laptop imports.
• All: the default collection containing every account/source.
• Dedup within an account should only dedup duplicate rows inside that one ingest source.
• Dedup across accounts should require an explicit collection boundary.

That model is attractive because it gives us a crisp safety boundary: we never dedup across independent archives unless the user has deliberately grouped them into a collection.

Concerns and questions:

1. Terminology/model mismatch

   The spec and package comments sometimes describe an account as a logical multi-source mailbox. For example, internal/dedup/dedup.go says dedup runs against a "single logical account" and that an account may include Gmail plus mbox for the same mailbox. The spec also says the same address can appear in multiple accounts, which points the other way: each import is an account/source and collections group them.

   I think we should decide this explicitly before merging. My preference is account = one ingest source, collection = grouping. If we agree, the docs, comments, CLI help, and config names should consistently use that model.

2. --account is overloaded with collection behavior

   ResolveAccount checks collections first, then sources, and deduplicate --account uses that resolver. So --account work may target a collection named work, not an account/source. That makes CLI behavior surprising and introduces name-shadowing risk if a collection and account share the same identifier/display name.

   Suggestion: keep --account restricted to one account/source, and add an explicit --collection flag for collection-level operations. If a command wants a generic scope, model that internally as a Scope, but expose precise flags at the CLI boundary.

3. The spec mentions --collection, but the implementation routes through --account

   The spec says msgvault deduplicate --collection personal, but the CLI only has --account, with collection resolution hidden inside it. I would rather make collection dedup explicit:

   • msgvault deduplicate --account <account>: one source/account only
   • msgvault deduplicate --collection <collection>: all accounts in that collection
   • msgvault deduplicate: each account/source independently

4. Collection creation can accidentally accept collections as members

   collections create --accounts ... calls ResolveAccount, which means a collection name can expand to source IDs. That effectively allows nested collection references even though the schema only stores source IDs. If nested collections are not a supported concept, this should reject collection references and require actual account/source identifiers or IDs.

5. Identity configuration is global, but the spec says identities are tied to accounts/collections

   The config adds a global [identity] addresses = [...], while the spec says identities are tied to accounts and a collection identity is the union of member account identities. Global identity addresses may be useful as a temporary input, but they are a different model. This matters for sent-copy detection: an address that is "me" in one account/collection may not be safe to apply globally across all scopes.

   We may want either:

   • stored per-account identity records, with collection identity derived from members, or
   • an explicit statement that identity is currently global and account-scoped identities are future work.

6. Dedup-hidden messages are not hidden from every search surface

   Most SQLite/DuckDB query paths now filter messages.deleted_at IS NULL, but vector/hybrid search still appears to filter only deleted_from_source_at IS NULL:

   • internal/vector/sqlitevec/backend.go live-hit filtering
   • internal/vector/sqlitevec/backend.go filteredMessageIDs
   • internal/vector/sqlitevec/fused.go fused search CTE

   Since dedup uses messages.deleted_at as a local hide flag, vector/hybrid results can still surface pruned duplicates. If the contract is "hidden from all query paths," vector/hybrid needs the same filter or a shared live-message predicate.

7. Collection scoping is only partially propagated through msgvault

   Query structs now have SourceIDs, which is a good start, but several user-facing paths still resolve only a single source/account:

   • local search --account uses GetSourceByIdentifier
   • vector/hybrid search --account uses GetSourceByIdentifier
   • text/vector/TUI/API/MCP paths appear to remain mostly single-SourceID oriented
   • some stats/sub-aggregate label/account counts still only special-case SourceID, not SourceIDs

   We should decide whether collections are only a dedup/admin concept in this PR, or whether they are intended to be first-class query scopes. If first-class, we probably need a common scope resolver and tests across search, stats, TUI, API, MCP, and vector paths.

8. Cache/index invalidation needs a clearer policy

   Dedup tells the user to run build-cache --full-rebuild, and the Parquet export excludes deleted_at rows. That handles one cache if the user follows the instruction. But FTS and vector indexes are separate surfaces. Filtering can be enough, but then every retrieval path must consistently apply deleted_at IS NULL; otherwise the index can retain hidden messages and leak them in results.

   I think we should document one of these policies:

   • dedup mutates only SQLite and every query/index path must filter local-hidden rows, or
   • dedup also schedules/rebuilds affected derived indexes/caches, or
   • dedup requires explicit rebuild commands and refuses/flags stale derived surfaces.

9. Schema ownership is split

   Collection tables are created by ensureCollectionSchema instead of the main schema files. That may be pragmatic for this branch, but msgvault has been moving toward dialect-aware schema ownership. If collections are permanent core concepts, they probably belong in the canonical schema/migration path rather than being hidden behind lazy creation.

10. Undo is not a full rollback

UndoDedup restores hidden duplicate rows but intentionally does not undo label unioning or raw MIME backfill. That is probably acceptable, but the user-facing language should be very precise: undo restores pruned rows and cancels pending manifests where possible; it does not restore the exact pre-run database state. This is especially important if agents/users rely on --undo as a safety guarantee.

11. Remote deletion manifest semantics need a second look under collection scopes

The implementation has a good safety rule: only stage remote deletion when loser and survivor share the same source_id. That avoids deleting from Gmail just because a local backup had a copy. But once collections are in play, reporting, manifest naming, and Account labeling need to remain source/account-specific rather than collection-specific. It would be good to add explicit tests for collection-scoped dedup with remote deletion enabled.

12. Spec includes broader behavior that is not implemented here

The spec discusses import-time dedup (--into), automatic dedup when creating/adding to collections, collection export to an account, and identity-derived inbound/outbound classification. This PR implements some foundational pieces, but not all of those flows. That is fine if intentional, but the PR description/spec should distinguish what is implemented now from the longer-term proposal.

Recommended architecture direction:

• Keep the core store model simple: sources/accounts are ingest units; collections are many-to-many groupings of source IDs.
• Introduce an explicit internal scope type, something like Scope{Kind: Account|Collection|All, SourceIDs: []int64, DisplayName: string}.
• Make CLI flags precise: --account for one source/account, --collection for collections. Avoid letting one flag silently resolve to either.
• Centralize live-message filtering so deleted_from_source_at and local dedup deleted_at semantics are not reimplemented inconsistently across SQLite, DuckDB, vector, FTS, API, MCP, and TUI paths.
• Treat identity as a scoped concept, or explicitly call out that global identity config is interim.

I think this branch is useful, but I would like us to settle these boundaries before we bake in the naming/API shape. The main thing I want to avoid is agents and humans reading "account" differently across commands, docs, and package APIs.

[jesserobbins]

This is good feedback. Let me take another pass at clarifying the spec first.

[wesm]

Let me know if I can assist, this will be good to get designed correctly so it can provide a good foundation for different types of msgvault frontend environments

[jesserobbins]

@wesm I did a pretty deep review on the feedback, worked through a couple of the other design elements in #278. I am going to update this branch with those changes. Let me know if you have further feedback.

[roborev-ci]

roborev: Combined Review (a4a71e3)

Medium-risk issues remain around query scoping, fresh imports, and live-message filtering.

Medium

• internal/query/sqlite.go:1432
MergeFilterIntoQuery converts only filter.SourceID into merged.AccountIDs and ignores filter.SourceIDs, so collection-scoped filters can be dropped when merged into search queries, widening results to all accounts.
  Fix: Copy filter.SourceIDs into merged.AccountIDs when present, preserving the same precedence as appendSourceFilter.

• cmd/msgvault/cmd/import_imessage.go:123
openStoreAndInit now errors when the database file does not already exist. Since this helper is used by import-imessage, a fresh import can no longer initialize the database as it did before.
  Fix: Remove the pre-existence check from the shared helper, or split it so only read-only commands require an existing database.

• internal/query/sqlite.go:189
  Several updated read paths add deleted_at IS NULL but still only apply deleted_from_source_at IS NULL behind HideDeletedFromSource, which violates the new live-message contract and lets source-deleted rows remain visible in normal queries/caches.
  Fix: Use the shared live-message predicate consistently, including both deleted_at IS NULL and deleted_from_source_at IS NULL, across search, aggregates, cache export, and dedup candidate queries.

---

Synthesized from 3 reviews (agents: codex, gemini | types: default, security)

[jesserobbins]

Changed my mind. Am closing this branch and will submit a fresh pr in the morning.

[jesserobbins]

Superseded by #304 — same scope, flat 5-commit history, design docs and worked-out alignment notes stripped out for review/merge clarity. Leaving this PR open as the historical record (the design alignment review trail and iterative refinement live here).