fix: gate spam like delete, and keep one origin in the model

6 files changed, 267 insertions, 19 deletions

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 6da8e2f..e3bba3b 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -34,15 +34,16 @@ point at which they are stable.
   into that folder and records the one it came from in a `moved-from:` tag, so
   Restore and Undo put it back where it was. Before this the action only added
   the tag and left the file in place, so a message marked spam on one machine
-  did not appear in the folder on another.
+  did not appear in the folder on another. Like Delete, it is unavailable on a
+  reply row and on mail already in the trash.
 - **Empty Spam.** Moves every message in the account's spam folder to its trash
   in one act, scoped to the account selector like Empty trash, and asks nothing
   first: it is a move with an undo behind it, and a mutation that can be undone
   gets undo rather than a dialog.
-- **Find stranded spam.** Mail carrying the `spam` tag while sitting in no spam
-  folder, usually because another client or an older version tagged it and left
-  the file where it was, is listed by a menu entry so it can be selected and
-  moved. It is the spam counterpart to Find stranded deleted mail.
+- **Check for stranded spam.** Mail carrying the `spam` tag while sitting in
+  no spam folder, usually because another client or an older version tagged it
+  and left the file where it was, is listed by a menu entry so it can be
+  selected and moved. It is the spam counterpart to Find stranded deleted mail.
 - **A spam button on the message pane's bar**, beside Star and Archive. It is
   drawn with a bug rather than the theme's junk glyph, and falls back to
   `mail-mark-junk` on a theme that ships no bug.
diff --git a/README.md b/README.md
index 136076b..b21c445 100644
--- a/README.md
+++ b/README.md
@@ -779,9 +779,9 @@ Defaults, all rebindable through `[keys]`:
 
 Every action in this table carries a default binding, and every one appears in
 a menu. Six actions carry none, because a chord for them would be arbitrary:
-Find stranded spam, Empty trash, Delete permanently, Empty spam, Edit draft and
-Save message. They are reachable from the menus, and the shortcut reference
-prints them as unbound.
+Check for stranded spam, Empty trash, Delete permanently, Empty spam, Edit
+draft and Save message. They are reachable from the menus, and the shortcut
+reference prints them as unbound.
 **Help > Keyboard shortcuts** lists the current bindings, generated from the
 actions themselves, so it shows your overrides rather than these defaults.
 
diff --git a/src/config.cpp b/src/config.cpp
index 390848a..184c756 100644
--- a/src/config.cpp
+++ b/src/config.cpp
@@ -68,8 +68,8 @@ const QStringList kQueryGenerators = { QStringLiteral("unread"),
                                        QStringLiteral("spam") };
 
 /// The tag a generator matches, for the three filters that are a plain tag
-/// query. Empty for "sent", "drafts" and "trash", which compose from each
-/// account's folder instead and are handled separately.
+/// query. Empty for "sent", "drafts", "trash" and "spam", which compose from
+/// each account's folder instead and are handled separately.
 QString generatorTag(const QString &generator)
 {
     if (generator == QStringLiteral("unread"))
@@ -85,8 +85,9 @@ QString generatorTag(const QString &generator)
 /// user's own message back into the conversation it answers, and "drafts" is
 /// worse: a thread row stands for its first matched message, which for a draft
 /// reply is the message being replied TO, so the draft itself is unreachable.
-/// "trash" stays threaded, since a deleted message still belongs to its
-/// conversation. Closed set, and the one place the three views are decided.
+/// "trash" and "spam" stay threaded, since a deleted or spammed message still
+/// belongs to its conversation. Closed set, and the one place the three views
+/// are decided.
 bool generatorIsFlat(const QString &generator)
 {
     return generator == QStringLiteral("sent")
diff --git a/src/mainwindow.cpp b/src/mainwindow.cpp
index 476ff18..4b116ba 100644
--- a/src/mainwindow.cpp
+++ b/src/mainwindow.cpp
@@ -3931,6 +3931,15 @@ void MainWindow::refreshTrashActions()
                         && !m_replySelectionHidesDelete);
     }
 
+    // Mark spam follows Delete exactly: one reply cannot be moved out of its
+    // conversation, and the trash view does not afford it. Same two hides, ORed
+    // rather than fought over, so it reads the same flag instead of walking the
+    // selection a second time.
+    if (auto *spam = m_actions.value(QStringLiteral("spam"))) {
+        spam->setVisible((!haveSelection || !inTrash)
+                         && !m_replySelectionHidesDelete);
+    }
+
     // The mirror, which shipped beside it: Restore was added unconditionally
     // to both menus and so was offered on mail that was never deleted.
     if (auto *restore = m_actions.value(QStringLiteral("restore"))) {
@@ -6466,6 +6475,29 @@ QString MainWindow::originTagFor(const QString &dbRelativeFolder) const
     return QString(kOriginTagPrefix) + accountRelative;
 }
 
+QStringList MainWindow::originTagsToStrip(const QStringList &messageIds,
+                                          const QStringList &added) const
+{
+    const bool writingOrigin =
+        std::any_of(added.cbegin(), added.cend(), [](const QString &tag) {
+            return tag == kOriginTagPlaceholder()
+                   || tag.startsWith(QLatin1String(kOriginTagPrefix));
+        });
+    if (!writingOrigin)
+        return {};
+
+    QStringList stripped;
+    for (const QString &messageId : messageIds) {
+        for (const QString &tag : m_model->messageById(messageId).tags) {
+            if (tag.startsWith(QLatin1String(kOriginTagPrefix))
+                && !added.contains(tag) && !stripped.contains(tag)) {
+                stripped.append(tag);
+            }
+        }
+    }
+    return stripped;
+}
+
 void MainWindow::trashThreads(const QStringList &threadIds)
 {
     if (threadIds.isEmpty())
@@ -7331,6 +7363,11 @@ void MainWindow::sendMove(const QStringList &messageIds,
         if (tag != kOriginTagPlaceholder())
             displayRemove.append(tag);
     }
+    // The model's half of the worker's overwrite rule. This move is writing a
+    // new origin, so every `moved-from:` the model still holds is stale and
+    // goes with it; otherwise the confirmed update below appends the new tag
+    // beside the old one and restoreSelected() reads whichever comes first.
+    displayRemove += originTagsToStrip(messageIds, add);
     // A thread-scoped move already repainted its rows in
     // trashThreads() / untrashThreads(), synchronously, before
     // the worker was asked to resolve the threads at all. Repeating it here
@@ -7586,7 +7623,11 @@ void MainWindow::onMessagesMoved(const QMap<QString, QString> &originByMessageId
         };
 
         const QStringList resolvedAdd = resolve(pending.add);
-        const QStringList resolvedRemove = resolve(pending.remove);
+        QStringList resolvedRemove = resolve(pending.remove);
+        // The confirmed half of the same rule, and the one that matters when
+        // the optimistic update was skipped (a whole-thread move): the new
+        // origin replaces any other the model still holds.
+        resolvedRemove += originTagsToStrip(it.value(), resolvedAdd);
         sendMessageTagChange(it.value(), resolvedAdd, resolvedRemove,
                              pending.description);
 
diff --git a/src/mainwindow.h b/src/mainwindow.h
index a7fbf26..478de2d 100644
--- a/src/mainwindow.h
+++ b/src/mainwindow.h
@@ -1107,12 +1107,13 @@ private:
     SelectionKind selectionKind() const;
 
     /// Whether the selection holds a reply row, which is what hides Delete,
-    /// Restore and Archive (item 177).
+    /// Restore, Archive and Mark spam: all conversation-level acts, and a
+    /// single reply cannot be removed from its thread (item 177).
     ///
     /// Written by refreshScopedActionLabels() and read by
-    /// refreshTrashActions(), which runs after it and owns the same two
-    /// actions' visibility. A flag rather than a second walk over the
-    /// selection, so the two cannot answer differently.
+    /// refreshTrashActions(), which runs after it and owns the same actions'
+    /// visibility. A flag rather than a second walk over the selection, so the
+    /// two cannot answer differently.
     bool m_replySelectionHidesDelete = false;
 
     /// Hides Delete on mail already in the trash, and Restore on mail that
@@ -1393,6 +1394,21 @@ private:
     /// and a restore stripped a tag that had never been written.
     QString originTagFor(const QString &dbRelativeFolder) const;
 
+    /// The `moved-from:` tags these messages currently carry that a new origin
+    /// must replace, read from the MODEL.
+    ///
+    /// Mirrors the worker's overwrite rule (NotmuchWorker::applyTags): a
+    /// message carries exactly one origin, so writing a new one strips any
+    /// other. The worker does it against the database; this does it against
+    /// the optimistic model, where restoreSelected() reads the origin back and
+    /// a stale second tag would send the message to the wrong folder.
+    ///
+    /// Returns nothing unless `added` is writing a new origin, which is either
+    /// a resolved `moved-from:` tag or the unresolved placeholder the
+    /// optimistic update drops before painting.
+    QStringList originTagsToStrip(const QStringList &messageIds,
+                                  const QStringList &added) const;
+
     /// The account whose maildir contains `path`, or an invalid account when
     /// no configured maildir does.
     ///
diff --git a/tests/test_mainwindow.cpp b/tests/test_mainwindow.cpp
index dfce9c3..b9a8f0f 100644
--- a/tests/test_mainwindow.cpp
+++ b/tests/test_mainwindow.cpp
@@ -534,6 +534,12 @@ private slots:
     void theSpamCleanupQueryExcludesTheSpamFolder();
     void theSpamCleanupQueryWithoutASpamFolderIsJustTheTag();
 
+    // Item 187's final review. Mark spam is Delete's sibling, so it is hidden
+    // exactly where Delete is, and the model keeps one origin per message.
+    void spamIsAbsentOnAReplyRow();
+    void spamIsHiddenOnMailAlreadyInTheTrash();
+    void restoringAfterTwoMovesReturnsToTheLatestOrigin();
+
     // ComposeWindow, item 123. These need a window but no worker: the composer
     // never touches NotmuchWorker, it reads its context from the value struct
     // MainWindow hands it, so a Config written to a temporary INI is the whole
@@ -8496,7 +8502,8 @@ void TestMainWindow::theMessagePaneCarriesItsOwnActionBar()
     // this after seeing the first version, and the split is now by what the
     // action needs rather than by what it is about.
     const QStringList expected = { QStringLiteral("reply"),
-                                   QStringLiteral("forward") };
+                                   QStringLiteral("forward"),
+                                   QStringLiteral("spam") };
     for (const QString &name : expected) {
         auto *action = window.findChild<QAction *>(name);
         QVERIFY2(action, qPrintable(QStringLiteral("no action %1").arg(name)));
@@ -12249,7 +12256,9 @@ void TestMainWindow::emptySpamRewritesTheOriginToTheSpamFolder()
                                          "tag:\"moved-from:inbox\"")) == 1,
         15000);
 
-    window.findChild<QAction *>(QStringLiteral("empty_spam"))->trigger();
+    auto *emptySpam = window.findChild<QAction *>(QStringLiteral("empty_spam"));
+    QVERIFY2(emptySpam, "empty_spam does not exist");
+    emptySpam->trigger();
     QTRY_VERIFY_WITH_TIMEOUT(
         folderHasMessageFile(root + QStringLiteral("/acct/Trash/cur"), stem),
         15000);
@@ -17084,4 +17093,184 @@ void TestMainWindow::undoingAMarkReadRestoresOnlyWhatWasUnread()
              0);
 }
 
+void TestMainWindow::spamIsAbsentOnAReplyRow()
+{
+    // Item 187's final review. Mark spam is Delete's sibling, so it follows the
+    // same rule (item 177): a single reply cannot be moved out of its
+    // conversation, and moving only that one message is not a gesture this
+    // application offers. Absent on a reply, back on the conversation row.
+    const Config config;
+    MainWindow window(config);
+
+    auto *model = window.findChild<ThreadListModel *>();
+    auto *view = window.findChild<QTreeView *>();
+    QVERIFY(model && view);
+
+    ThreadSummary first = makeThread(QStringLiteral("t1"), {});
+    first.totalCount = 1;
+    ThreadSummary many = makeThread(QStringLiteral("t2"), {});
+    many.totalCount = 2;
+    model->appendBatch({ first, many });
+
+    MessageNode root;
+    root.messageId = QStringLiteral("m1");
+    root.threadId = QStringLiteral("t2");
+    root.depth = 0;
+    MessageNode reply;
+    reply.messageId = QStringLiteral("m2");
+    reply.threadId = QStringLiteral("t2");
+    reply.depth = 1;
+    model->setThreadMessages(QStringLiteral("t2"), { root, reply });
+
+    const QModelIndex thread = model->index(1, 0, QModelIndex());
+    view->expand(thread);
+    const QModelIndex replyRow = model->index(0, 0, thread);
+    view->selectionModel()->select(
+        replyRow, QItemSelectionModel::ClearAndSelect | QItemSelectionModel::Rows);
+    view->setCurrentIndex(replyRow);
+    QApplication::processEvents();
+
+    auto *spam = window.findChild<QAction *>(QStringLiteral("spam"));
+    QVERIFY(spam);
+    QVERIFY2(!spam->isVisible() || !spam->isEnabled(),
+             "Mark spam is offered on a reply: one reply cannot be moved out of "
+             "its conversation, the same rule Delete and Archive follow");
+
+    // And the mirror: on the conversation row it is back, so the hide is about
+    // what the row IS and not a stuck flag.
+    selectThreadRow(view, 1);
+    QApplication::processEvents();
+    QVERIFY2(spam->isVisible() && spam->isEnabled(),
+             "Mark spam stayed hidden on a conversation row");
+}
+
+void TestMainWindow::spamIsHiddenOnMailAlreadyInTheTrash()
+{
+    // The trash view does not afford Mark spam, exactly as it does not afford
+    // Delete: the message is already thrown away, and a move from the trash
+    // into the spam folder is not a gesture the view offers. Asked of the PATH,
+    // never the `deleted` tag, for the reason Delete is.
+    QTemporaryDir dir;
+    QVERIFY(dir.isValid());
+    const Config config = configWithTrash(dir);
+    MainWindow window(config);
+
+    auto *model = window.findChild<ThreadListModel *>();
+    QVERIFY(model);
+    auto *view = window.findChild<QTreeView *>();
+    QVERIFY(view);
+    auto *spam = window.findChild<QAction *>(QStringLiteral("spam"));
+    QVERIFY(spam);
+
+    model->appendBatch({
+        threadAtPath(QStringLiteral("t1"),
+                     QStringLiteral("acct/inbox/cur/1:2,S")),
+        threadAtPath(QStringLiteral("t2"),
+                     QStringLiteral("acct/trash/cur/2:2,S")),
+    });
+
+    view->setCurrentIndex(model->index(0, 0, {}));
+    QVERIFY2(spam->isVisible(),
+             "Mark spam is hidden on mail that is NOT in the trash, so this "
+             "test cannot tell the two cases apart");
+
+    view->setCurrentIndex(model->index(1, 0, {}));
+    QVERIFY2(!spam->isVisible(),
+             "Mark spam is still offered on mail already in the trash");
+
+    // A folder whose name STARTS with the trash folder's is a different folder,
+    // so the prefix must be compared with its separator here too.
+    model->appendBatch({ threadAtPath(QStringLiteral("t3"),
+                                      QStringLiteral("acct/trash-old/cur/3:2,S")) });
+    view->setCurrentIndex(model->index(2, 0, {}));
+    QVERIFY2(spam->isVisible(),
+             "Mark spam vanished on mail in acct/trash-old, which is not the "
+             "trash");
+}
+
+void TestMainWindow::restoringAfterTwoMovesReturnsToTheLatestOrigin()
+{
+    // Item 187's final review. The worker keeps exactly ONE `moved-from:` per
+    // message, overwriting the old one (NotmuchWorker::applyTags). The
+    // optimistic MODEL update did not: on a second move it added the new origin
+    // beside the old, and restoreSelected() reads the model and takes the FIRST
+    // `moved-from:` with a break(). So a message that travelled
+    // inbox -> Spam -> Trash restored to the INBOX rather than to the spam
+    // folder it actually came from, silently and with no way back.
+    WorkerBackedWindow backed;
+    QVERIFY(backed.fixture().addMessage(
+        QStringLiteral("acct/inbox"), QStringLiteral("twoorig@example.org"),
+        QStringLiteral("Two origins"), QStringLiteral("sender@example.org"),
+        QStringLiteral("Fri, 14 Aug 2026 10:00:00 +0200"),
+        QStringLiteral("Body text.")));
+    QVERIFY2(backed.build(QStringLiteral("acct"), QStringLiteral("acct"),
+                          QStringLiteral("Trash"), QStringLiteral("Spam")),
+             qPrintable(backed.error()));
+
+    MainWindow window(backed.config());
+    auto *model = window.findChild<ThreadListModel *>();
+    auto *view = window.findChild<ThreadListView *>();
+    auto *queryEdit =
+        window.findChild<QLineEdit *>(QStringLiteral("queryEdit"));
+    QVERIFY(model && view && queryEdit);
+
+    const QString root = backed.fixture().maildirPath();
+    const QString cfg = backed.fixture().configPath();
+    const QString stem = QStringLiteral("twoorig.example.org");
+
+    queryEdit->setText(QStringLiteral("tag:inbox"));
+    queryEdit->returnPressed();
+    QTRY_VERIFY_WITH_TIMEOUT(model->rowCount(QModelIndex()) == 1, 15000);
+
+    // inbox -> Spam, origin moved-from:inbox.
+    view->setCurrentIndex(model->index(0, 0, QModelIndex()));
+    window.findChild<QAction *>(QStringLiteral("spam"))->trigger();
+    QTRY_VERIFY_WITH_TIMEOUT(
+        folderHasMessageFile(root + QStringLiteral("/acct/Spam/cur"), stem),
+        15000);
+    QTRY_VERIFY_WITH_TIMEOUT(
+        notmuchCount(cfg, QStringLiteral("id:twoorig@example.org and "
+                                         "tag:\"moved-from:inbox\"")) == 1,
+        15000);
+
+    // Spam -> Trash, which overwrites the origin with moved-from:Spam. An
+    // `id:` query keeps the row in the model across the move, so the second
+    // Delete press can select it.
+    queryEdit->setText(QStringLiteral("id:twoorig@example.org"));
+    queryEdit->returnPressed();
+    QTRY_VERIFY_WITH_TIMEOUT(model->rowCount(QModelIndex()) == 1, 15000);
+    view->setCurrentIndex(model->index(0, 0, QModelIndex()));
+    window.findChild<QAction *>(QStringLiteral("delete"))->trigger();
+
+    QTRY_VERIFY_WITH_TIMEOUT(
+        folderHasMessageFile(root + QStringLiteral("/acct/Trash/cur"), stem),
+        15000);
+
+    // The DATABASE holds exactly one origin, and it names the spam folder.
+    // This is the worker's overwrite rule, already correct.
+    QTRY_VERIFY_WITH_TIMEOUT(
+        notmuchCount(cfg, QStringLiteral("id:twoorig@example.org and "
+                                         "tag:\"moved-from:Spam\"")) == 1,
+        15000);
+    QCOMPARE(notmuchCount(cfg, QStringLiteral("id:twoorig@example.org and "
+                                              "tag:\"moved-from:inbox\"")),
+             0);
+
+    // A second Delete press restores it. The MODEL restoreSelected() reads must
+    // also hold one origin; without the model-side overwrite it still held
+    // moved-from:inbox and sent the message home to the inbox.
+    view->setCurrentIndex(model->index(0, 0, QModelIndex()));
+    window.findChild<QAction *>(QStringLiteral("delete"))->trigger();
+
+    QTRY_VERIFY_WITH_TIMEOUT(
+        folderHasMessageFile(root + QStringLiteral("/acct/Spam/cur"), stem),
+        15000);
+    QVERIFY2(!folderHasMessageFile(root + QStringLiteral("/acct/inbox/cur"),
+                                   stem)
+                 && !folderHasMessageFile(root + QStringLiteral("/acct/inbox/new"),
+                                          stem),
+             "the restore went to the inbox instead of the spam folder the "
+             "message actually came from: the model held two origins");
+}
+
 #include "test_mainwindow.moc"