fix: keep one Message-ID across a draft's revisions

Every autosave called MessageBuilder::build(), which generated a fresh
Message-ID unconditionally, so each revision of a draft was a different
message rather than a new version of one. The entry called this invisible
while the file is replaced correctly, and that turned out to be wrong:
mbsync uploads each revision to the drafts folder before the next save
removes the local file, so the server keeps one message per revision and
syncs them all back down. Measured on real mail as four independent
messages for a single reply, all four carrying a ,U= infix, threading
into the conversation and putting a draft tag on a Sent row. Deleting a
local file does not retract an uploaded one, which is why the local
cleanup, which is correct, could never fix it.

The user chose a stable id while drafting, discarded at send: the sent
copy is a different item from the draft, and the draft is deleted once
the message goes out, which the code already did.

Three links, none of which existed. OutgoingMessage::messageId is the
field, where empty means generate, so the send path is unchanged by
construction rather than by remembering to clear it.
MessageBuilder::build() uses a supplied id when there is one.
ComposeWindow::m_draftMessageId holds the identity between revisions,
assigned from built.messageId so the first save adopts the id GMime just
generated, and ComposeContext::draftMessageId carries it across a reopen,
read in forDraft() from ParsedMessage::messageId, which the parser
already provided and nothing had ever used.

Five tests, because the property spans three objects and a test at any
one of them passes while another link is broken. Two are the safety
constraint rather than the feature: a field defaulting to a fixed value
would satisfy the reuse test and make two sent messages share an id,
which is far worse than the defect this fixes.

One comment is corrected rather than left: the autosave's dirty check
justified comparing the message rather than the built bytes with "GMime
is given a fresh Date and Message-ID on every build". Half of that is no
longer true. The Date still is, so the conclusion stands.

Revisions already on the server are not touched by this; the four found
on real mail were deleted by hand.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Jq9gXquUo9W4KXDagJXMmn

3 files changed, 184 insertions, 0 deletions

diff --git a/tests/test_composewindow.cpp b/tests/test_composewindow.cpp
index d95d55d..49bb390 100644
--- a/tests/test_composewindow.cpp
+++ b/tests/test_composewindow.cpp
@@ -66,6 +66,8 @@ private slots:
     void theMenuBarReachesEveryComposerAction();
     void saveDraftWritesAndReports();
     void aSavedDraftIsFlaggedSeen();
+    void everyRevisionOfADraftKeepsOneMessageId();
+    void aSentMessageDoesNotInheritTheDraftsMessageId();
     void aForwardCarriesTheOriginalHtmlAndStripsRemoteContent();
     void anHtmlForwardPreviewsTheOriginalInsteadOfQuotingIt();
     void theMenusReuseTheToolbarActions();
@@ -752,6 +754,97 @@ void TestComposeWindow::saveDraftWritesAndReports()
     QVERIFY2(!window.isWindowModified(), "a manual save must clear the marker");
 }
 
+/// Item 165. Two saves of one draft must produce ONE message, not two.
+///
+/// Every save used to mint a fresh Message-ID, and mbsync uploads each
+/// revision to the drafts folder before the next save removes the local file,
+/// so the server kept one message per revision: measured on the user's own
+/// mail as four independent messages for a single reply, which then threaded
+/// into the conversation and put a `draft` tag on a Sent row. Deleting a local
+/// file does not retract an uploaded one, so the local cleanup, which is
+/// correct, could never fix this.
+///
+/// Asserted on the FILE's header rather than on a notmuch tag, for the reason
+/// the test below gives: the bytes are what the code here controls.
+void TestComposeWindow::everyRevisionOfADraftKeepsOneMessageId()
+{
+    const Config config = configWithDrafts();
+
+    ComposeContext context;
+    context.kind = ComposeContext::Kind::New;
+    context.accountKey = QStringLiteral("work");
+
+    ComposeWindow window(context, config, m_dir->path());
+    auto *body = window.findChild<QPlainTextEdit *>(QStringLiteral("body"));
+    QVERIFY(body);
+    auto *save = window.findChild<QAction *>(QStringLiteral("compose_save"));
+    QVERIFY(save);
+
+    const auto messageIdIn = [](const QString &path) {
+        QFile file(path);
+        if (!file.open(QIODevice::ReadOnly))
+            return QString();
+        const QString text = QString::fromUtf8(file.readAll());
+        for (const QString &line : text.split(QLatin1Char('\n'))) {
+            if (line.startsWith(QStringLiteral("Message-Id:"), Qt::CaseInsensitive))
+                return line.section(QLatin1Char(':'), 1).trimmed();
+            if (line.trimmed().isEmpty())
+                break;  // end of headers
+        }
+        return QString();
+    };
+
+    QSignalSpy saved(&window, &ComposeWindow::draftSaved);
+
+    body->setPlainText(QStringLiteral("First revision."));
+    save->trigger();
+    QCOMPARE(saved.size(), 1);
+    const QString firstPath = saved.at(0).at(0).toString();
+    const QString firstId = messageIdIn(firstPath);
+    QVERIFY2(!firstId.isEmpty(), "the first revision carries no Message-ID");
+
+    // A REAL change, or the dirty check short-circuits and no second file is
+    // written at all, which would pass this test while proving nothing.
+    body->setPlainText(QStringLiteral("Second revision, genuinely different."));
+    save->trigger();
+    QCOMPARE(saved.size(), 2);
+    const QString secondPath = saved.at(1).at(0).toString();
+    QVERIFY2(secondPath != firstPath,
+             "the second save wrote no new file, so the ids cannot be compared");
+
+    QCOMPARE(messageIdIn(secondPath), firstId);
+}
+
+/// The constraint that makes item 165 safe, and the one a future edit is most
+/// likely to break: the SEND path must mint its own id.
+///
+/// The user's decision was a stable id while drafting, DISCARDED at send, so
+/// the sent copy is a different item from the draft. Two sent messages sharing
+/// a Message-ID would be far worse than the defect this fixed, and a
+/// Message-ID reaches the server and every recipient, so it is not a local
+/// matter. currentMessage() leaves the field empty and the send path passes it
+/// straight to build(); this pins that.
+void TestComposeWindow::aSentMessageDoesNotInheritTheDraftsMessageId()
+{
+    const Config config = configWithDrafts();
+
+    ComposeContext context;
+    context.kind = ComposeContext::Kind::Draft;
+    context.accountKey = QStringLiteral("work");
+    context.draftMessageId = QStringLiteral("the-draft-id@example.org");
+
+    ComposeWindow window(context, config, m_dir->path());
+    auto *body = window.findChild<QPlainTextEdit *>(QStringLiteral("body"));
+    QVERIFY(body);
+    body->setPlainText(QStringLiteral("About to send."));
+
+    // The message the SEND path builds from, which is what decides the id.
+    const OutgoingMessage outgoing = window.currentMessage();
+    QVERIFY2(outgoing.messageId.isEmpty(),
+             "the send path carried the draft's Message-ID, so the sent copy "
+             "would share an id with a message already on the server");
+}
+
 /// A draft is authored by the user, so it is SEEN by definition and must never
 /// be tagged `unread`.
 ///
diff --git a/tests/test_mainwindow.cpp b/tests/test_mainwindow.cpp
index 5817ac4..f20701e 100644
--- a/tests/test_mainwindow.cpp
+++ b/tests/test_mainwindow.cpp
@@ -518,6 +518,7 @@ private slots:
     void doubleClickingADraftOpensTheComposer();
     void aResumedDraftReplacesItsFileRatherThanAddingOne();
     void aResumedDraftKeepsItsBlindRecipients();
+    void aResumedDraftKeepsTheMessageIdItWasSavedUnder();
     void aDraftRenamedByASyncStillReopensAndReplacesItsFile();
     void theComposerSplitsItsToolbarByScope();
     void ccAndBccHideBehindADisclosure();
@@ -14313,6 +14314,53 @@ void TestMainWindow::aDraftRenamedByASyncStillReopensAndReplacesItsFile()
              "into two files and both would reach the server");
 }
 
+/// Item 165, the third link in the chain and the one a composer-only test
+/// cannot reach: reopening a draft must keep the identity the FILE already
+/// has, or the next autosave starts a second one and the server ends up with
+/// two messages for one draft after all.
+void TestMainWindow::aResumedDraftKeepsTheMessageIdItWasSavedUnder()
+{
+    ComposeFixture fixture;
+    QVERIFY(fixture.build());
+
+    OutgoingMessage message;
+    message.accountKey = QStringLiteral("acct");
+    message.to = { QStringLiteral("someone@example.org") };
+    message.subject = QStringLiteral("Resumed");
+    message.markdownBody = QStringLiteral("Body.");
+    message.messageId = QStringLiteral("already-saved-under@example.org");
+
+    const QString folder = fixture.mailRoot() + QStringLiteral("/acct/Drafts");
+    const QString path = writeDraftFile(folder, message,
+                                        fixture.config().account(
+                                            QStringLiteral("acct")));
+    QVERIFY(!path.isEmpty());
+
+    const ComposeContext context =
+        ComposeContextBuilder::forDraft(fixture.config(), path);
+    QCOMPARE(context.draftMessageId,
+             QStringLiteral("already-saved-under@example.org"));
+
+    // And it reaches the next revision, which is the property that matters:
+    // the context carrying it is only half the chain.
+    ComposeWindow window(context, fixture.config(), fixture.mailRoot());
+    auto *body = window.findChild<QPlainTextEdit *>(QStringLiteral("body"));
+    QVERIFY(body);
+    body->setPlainText(QStringLiteral("Edited after reopening."));
+
+    QSignalSpy saved(&window, &ComposeWindow::draftSaved);
+    auto *save = window.findChild<QAction *>(QStringLiteral("compose_save"));
+    QVERIFY(save);
+    save->trigger();
+    QCOMPARE(saved.size(), 1);
+
+    QFile written(saved.first().first().toString());
+    QVERIFY(written.open(QIODevice::ReadOnly));
+    const QString text = QString::fromUtf8(written.readAll());
+    QVERIFY2(text.contains(QStringLiteral("<already-saved-under@example.org>")),
+             "the revision written after a reopen carries a different id");
+}
+
 void TestMainWindow::aResumedDraftKeepsItsBlindRecipients()
 {
     // MessageBuilder writes Bcc into the draft file deliberately, and says
diff --git a/tests/test_messagebuilder.cpp b/tests/test_messagebuilder.cpp
index 2ea14f0..56cf6a9 100644
--- a/tests/test_messagebuilder.cpp
+++ b/tests/test_messagebuilder.cpp
@@ -56,6 +56,8 @@ private slots:
     void aDirectoryAttachmentFailsRatherThanHangingTheProcess();
     void anUnparseableRecipientFailsRatherThanVanishing();
     void everyMessageCarriesADateAndMessageId();
+    void aSuppliedMessageIdIsUsedRatherThanAFreshOne();
+    void twoBuildsWithNoSuppliedIdStillDiffer();
     void aForwardSendsOnePartChosenByTheHtmlToggle();
     void recipientsAppearInTheirOwnHeaders();
     void anAccountWithNoAddressFailsRatherThanBuildingHeaderlessMail();
@@ -419,6 +421,47 @@ void TestMessageBuilder::everyMessageCarriesADateAndMessageId()
     QVERIFY(!r.messageId.isEmpty());
 }
 
+/// Item 165. A draft keeps ONE identity across its revisions, so an autosave
+/// replaces the message it wrote last time rather than adding another. Without
+/// this every save minted a new Message-ID, and mbsync uploaded each revision
+/// to the drafts folder before the next save removed the local file: measured
+/// on real mail as four distinct messages on the server for one reply.
+void TestMessageBuilder::aSuppliedMessageIdIsUsedRatherThanAFreshOne()
+{
+    OutgoingMessage message = baseMessage();
+    message.messageId = QStringLiteral("kept-across-revisions@example.org");
+
+    const MessageBuilder::Result r = MessageBuilder::build(message, m_account);
+    QVERIFY2(r.ok(), qPrintable(r.error));
+
+    QCOMPARE(r.messageId, QStringLiteral("kept-across-revisions@example.org"));
+    QVERIFY2(QString::fromUtf8(r.bytes).contains(
+                 QStringLiteral("<kept-across-revisions@example.org>")),
+             "the supplied id did not reach the headers");
+
+    // Twice, because the point is that a SECOND save keeps it. A test building
+    // once cannot tell a reused id from a freshly generated one.
+    const MessageBuilder::Result again = MessageBuilder::build(message, m_account);
+    QVERIFY2(again.ok(), qPrintable(again.error));
+    QCOMPARE(again.messageId, r.messageId);
+}
+
+/// The other half, and the constraint that makes the change safe: the SEND
+/// path supplies no id, and two sent messages must never share one. A field
+/// that defaulted to some fixed value would pass the test above and break
+/// this.
+void TestMessageBuilder::twoBuildsWithNoSuppliedIdStillDiffer()
+{
+    const MessageBuilder::Result first = MessageBuilder::build(baseMessage(), m_account);
+    const MessageBuilder::Result second = MessageBuilder::build(baseMessage(), m_account);
+    QVERIFY2(first.ok() && second.ok(), "a build failed");
+
+    QVERIFY(!first.messageId.isEmpty());
+    QVERIFY(!second.messageId.isEmpty());
+    QVERIFY2(first.messageId != second.messageId,
+             "two messages built with no supplied id share a Message-ID");
+}
+
 /// Bcc must be PRESENT in the bytes. The documented send command is `msmtp -t`,
 /// which reads its recipients FROM the headers and strips Bcc itself before
 /// transmission. Removing it here would mean blind recipients never receive the