From 6ea6980d064d1a27470b077c17adab286e4510b1 Mon Sep 17 00:00:00 2001 From: "Danilo M." Date: Tue, 25 Aug 2026 17:22:35 +0200 Subject: fix(hooks): keep inbox on mail that was sent AND received notmuch deduplicates by Message-ID, so mail the user sends to another of their own accounts is one message with two files: the sender's Sent copy and the recipient's Inbox copy. The carve-out matched on a file's path and tagged the message, so matching the Sent copy stripped inbox from the copy that had genuinely arrived and the mail was missing from the account that received it. sent_only() keeps a message only when EVERY file is inside a sent folder, which is what the docstring already claimed the predicate did. It is a loop because no query can express it: measured against a two-file message, `not path:` does not exclude it, and `count --output=files` reports every file of every matching message rather than the files that matched. Both read as if they worked, and are wrong for the same reason, that a notmuch term is a predicate over a message while this distinction is between its files. The root comes from database.mail_root rather than database.path, since this index is split and no message file is under the index directory. A test fixture with the index outside the mail root covers it; the ordinary layout cannot, because both keys return the same string there. Both mutations fail: all->any loses inbox on the self-addressed message, mail_root->path silently stops stripping anything. Verified read-only against the live index, tagging nothing: of 807 messages matching a sent path, 780 are still stripped and 27 are spared, every one of them two files with one in another account's inbox. No arrival is affected. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01HFuRPtzFrSxCQjFk6tq7gD --- assets/hooks/post-new | 125 ++++++++++++++++++++++++++++++++++++++---- assets/hooks/test_post_new.py | 92 +++++++++++++++++++++++++++++-- 2 files changed, 201 insertions(+), 16 deletions(-) diff --git a/assets/hooks/post-new b/assets/hooks/post-new index 428ec29..fca31a1 100755 --- a/assets/hooks/post-new +++ b/assets/hooks/post-new @@ -73,6 +73,109 @@ def log(message): print(f"post-new: {message}", file=sys.stderr) +def _quote(value): + """Escape a Message-ID for a double-quoted notmuch term.""" + return '"' + value.replace("\\", "\\\\").replace('"', '\\"') + '"' + + +def sent_only(query, folders): + """The ids matching `query` whose files are ALL inside `folders`. + + None on failure, so the caller leaves `tag:new` in place rather than + stripping on a half-read answer. + + **Every file has to be in a sent folder, not merely one of them.** notmuch + deduplicates by Message-ID, so mail the user sends to another of their own + accounts is ONE message with two files: the sender's Sent copy and the + recipient's Inbox copy. The old query matched on a path and the tag applied + to the message, so matching the Sent copy stripped `inbox` from the copy + that had genuinely arrived, and the mail was missing from the account that + received it (item 166). + + This cannot be expressed as a query, which is why it is a loop. Measured + against a two-file message: `not path:"Inbox/**"` does NOT exclude it, and + `count --output=files` on a path query reports every file of every matching + message rather than the files that matched. Both read as if they worked and + are wrong for the same reason, that a notmuch term is a predicate over a + MESSAGE and the distinction being drawn here is between its FILES. + """ + root = mail_root() + if root is None: + log("no mail root; leaving sent mail alone") + return None + + prefixes = [root / folder for folder in folders] + + matched = search(query, "messages") + if matched is None: + return None + + # ponytail: one `notmuch search` per matched message. N is the sent mail + # in tag:new, so an ordinary sync is a handful and a first-run reindex is + # the whole corpus. Batch by parsing --format=json once if that ever + # matters; it does not at this size, and the loop is the readable form. + ids = [] + for message_id in matched: + paths = search(f"id:{_quote(message_id)}", "files") + if paths is None: + return None + if all(any(_within(Path(path), prefix) for prefix in prefixes) + for path in paths): + ids.append(message_id) + return ids + + +def _within(path, prefix): + """Whether `path` is inside `prefix`, compared as paths. + + Not `startswith`: `/Sent-old/cur/1` starts with `/Sent` and is + a different folder. Same trap as the attachment-save path check in the + application. + """ + try: + path.relative_to(prefix) + except ValueError: + return False + return True + + +def mail_root(): + """The Maildir root, or None. + + `database.mail_root`, not `database.path`: notmuch can hold the Xapian + index somewhere else entirely, and this user's does. Under that layout + `database.path` is the INDEX directory and no message file is inside it. + """ + result = subprocess.run(["notmuch", "config", "get", "database.mail_root"], + capture_output=True, text=True) + if result.returncode != 0: + return None + value = result.stdout.strip() + return Path(value) if value else None + + +def search(query, output): + """`notmuch search --output=` as a list, or None on failure. + + An id comes back bare here, without the `id:` prefix, because + `--output=messages` prints `id:` and the prefix is stripped. + """ + result = subprocess.run( + ["notmuch", "search", f"--output={output}", "--", query], + capture_output=True, text=True) + if result.returncode != 0: + log(f"notmuch search failed: {result.stderr.strip()}") + return None + + values = [] + for line in result.stdout.splitlines(): + line = line.strip() + if not line: + continue + values.append(line[3:] if line.startswith("id:") else line) + return values + + def strip_inbox_from_sent(run): """Take `inbox` off mail the user SENT, and nothing else. @@ -105,20 +208,20 @@ def strip_inbox_from_sent(run): query = f"{SCOPE} and ({qtmaildirconf.sent_query(folders)})" - # Counted BEFORE the tag, because the tag is what makes the count zero. - # A `notmuch tag` that matches nothing SUCCEEDS, so the old log line said - # "applied" whether it stripped four messages or none, and item 164 is - # exactly the case where that distinction is the whole question: a draft - # kept `inbox` on a pass whose log claimed the carve-out had run. The - # count is the only thing that separates "the tag ran and something - # re-added inbox afterwards" from "the message was never in scope". - matched = count(query) - - if not run(["-inbox"], query): + ids = sent_only(query, folders) + if ids is None: return False + for message_id in ids: + if not run(["-inbox"], f"id:{_quote(message_id)}"): + return False + + # A `notmuch tag` matching nothing SUCCEEDS, so the old log line said + # "applied" whether it stripped four messages or none, and item 164 is + # exactly the case where that distinction is the whole question: a draft + # that kept `inbox` on a pass whose log claimed the carve-out had run. log(f"sent-folder carve-out applied over {len(folders)} folder(s), " - f"{matched} message(s)") + f"{len(ids)} message(s)") return True diff --git a/assets/hooks/test_post_new.py b/assets/hooks/test_post_new.py index a0228aa..07728a6 100755 --- a/assets/hooks/test_post_new.py +++ b/assets/hooks/test_post_new.py @@ -38,6 +38,16 @@ from pathlib import Path HOOK = Path(__file__).resolve().parent / "post-new" +def files(env, query): + """How many FILES notmuch holds for the messages matching a query, which + is not the message count: a message sent to another of the user's own + accounts is one message with two files.""" + result = subprocess.run( + ["notmuch", "search", "--output=files", "--", query], + env=env, capture_output=True, text=True, check=True) + return len([line for line in result.stdout.splitlines() if line]) + + def make_message(maildir, name, sender, subject): path = maildir / "new" / name path.write_text( @@ -202,7 +212,7 @@ def test_a_protected_removal_is_skipped_whole_and_the_run_continues(): assert count(env, "tag:new") == 0 -def setup_accounts(tmp, sent_config=True): +def setup_accounts(tmp, sent_config=True, split_index=False): """A maildir laid out as qtmaildir configures it: two accounts, each with an Inbox and a Sent folder, one message in each. @@ -232,10 +242,23 @@ def setup_accounts(tmp, sent_config=True): "you@example.org", "something else sent") config = Path(tmp) / "notmuch-config" - config.write_text( - f"[database]\npath={root}\n\n" - f"[new]\ntags=new;unread;inbox\n\n" - f"[user]\nname=Test\nprimary_email=you@example.org\n") + if split_index: + # The index somewhere else entirely, which is how the developer's own + # machine runs: `database.path` is then the INDEX directory and no + # message file is inside it. Anything reading that key as the mail + # root resolves every path wrongly, and the ordinary layout above + # cannot show it because both keys return the same string. + index = Path(tmp) / "index" + index.mkdir() + config.write_text( + f"[database]\npath={index}\nmail_root={root}\n\n" + f"[new]\ntags=new;unread;inbox\n\n" + f"[user]\nname=Test\nprimary_email=you@example.org\n") + else: + config.write_text( + f"[database]\npath={root}\n\n" + f"[new]\ntags=new;unread;inbox\n\n" + f"[user]\nname=Test\nprimary_email=you@example.org\n") env = dict(os.environ) env["NOTMUCH_CONFIG"] = str(config) @@ -284,6 +307,65 @@ def test_sent_mail_does_not_keep_the_inbox_tag(): assert count(env, 'tag:inbox and path:"acct-two/Inbox/**"') == 1 +def test_a_message_that_was_sent_AND_received_keeps_inbox(): + """Item 166. notmuch deduplicates by Message-ID, so mail the user sends + to their own other account is ONE message with TWO files: the sender's + Sent copy and the recipient's Inbox copy. + + The carve-out matches on a file's path but tags the MESSAGE, so matching + the Sent copy stripped `inbox` from the Inbox copy as well and the mail + vanished from the account that genuinely received it. The predicate has + to hold for every file, not for any file. + + The identical `name` is what makes this one message: make_message() + derives the Message-Id from it. + """ + with tempfile.TemporaryDirectory() as tmp: + env = setup_accounts(tmp) + root = Path(tmp) / "Mail" + make_message(root / "acct-one/Sent", "self-addressed", + "you@example.org", "to my other account") + make_message(root / "acct-two/Inbox", "self-addressed", + "you@example.org", "to my other account") + subprocess.run(["notmuch", "new"], env=env, capture_output=True, + check=True) + # One message, two files: the premise of the whole defect. + assert count(env, "id:self-addressed@example.org") == 1 + assert files(env, "id:self-addressed@example.org") == 2 + + write_rules(env, []) + result = subprocess.run([str(HOOK)], env=env, capture_output=True, + text=True) + assert result.returncode == 0, result.stderr + + # It arrived, so it keeps `inbox`... + assert count(env, "tag:inbox and id:self-addressed@example.org") == 1 + # ...and the ordinary sent message, whose only file is in a sent + # folder, still loses it. Without this half the fix could simply be + # "never strip anything". + assert count(env, "tag:inbox and id:sent-one@example.org") == 0 + + +def test_the_carve_out_works_with_the_index_split_from_the_mail(): + """notmuch can hold the Xapian index outside the Maildir, and this user's + does. `database.path` is then the index directory, so a carve-out reading + that key as the mail root builds prefixes no message file is under, finds + every file "outside" the sent folders, and silently stops stripping + anything. + + The ordinary fixture cannot catch it: with the index inside the mail root + both keys return the same string and either one passes. + """ + with tempfile.TemporaryDirectory() as tmp: + env = setup_accounts(tmp, split_index=True) + write_rules(env, []) + result = subprocess.run([str(HOOK)], env=env, capture_output=True, + text=True) + assert result.returncode == 0, result.stderr + assert count(env, 'tag:inbox and path:"acct-one/Sent/**"') == 0 + assert count(env, 'tag:inbox and path:"acct-one/Inbox/**"') == 1 + + def test_sent_mail_keeps_every_other_tag(): """Only `inbox` is stripped. `unread` in particular must survive: maildir.synchronize_flags is true, so removing it rewrites Maildir -- cgit v1.2.3