fix(rules): stop a rule with a spaced name from vanishing on save

A rule named "justeat orders" in the field labelled Name was written to
rules.json correctly and then dropped by every reader, because load()
required ^[a-z0-9][a-z0-9-]*$ and the save path validated nothing. The
rule stayed in the file, invisible in the dialog, never applied by the
post-new hook, and the next save from the dialog would have deleted it
outright.

The asymmetry was the defect, not the pattern. TagRules::validate() is
now the single predicate: the dialog refuses to save against it, and
load() uses it to repair rather than drop, so a rule that fails is
visible and fixable instead of silently discarded.

- The typed name is sanitised into an id when the field is committed,
  so the field shows what will reach the file. uniqueId() suffixes a
  collision, since sanitising is many-to-one and can manufacture the
  duplicate that load() then drops.
- An already-legal id is never rewritten, including one like "a---b"
  that sanitising would otherwise collapse. Rewriting valid ids would
  churn a file mailctl also reads.
- A bad id loads repaired, with the warning kept: what is on disk is
  not what the hook runs until the file is saved back.

Deliberately not mirrored into mailrules.py. The hook tags real mail
unattended every ten minutes, where silently renaming an id is worse
than dropping the rule; the file converges as soon as the dialog saves.
No format change, so no version bump and no two-repo commitment.

The load warning was not missing: it had been showing "1 rule could not
be read and was skipped" on every open, in the same font and colour as
the intro prose two lines above it, and read as more explanation. It is
now a red banner beside Save, with an icon and a dismiss button, and it
says the rules need attention rather than that they were skipped, which
is no longer true. Dismissal is per-appearance only; a persistent one
would re-hide the problem that went unnoticed for a session.

Both new dialog tests were confirmed to fail with the sanitiser
reverted, and the banner's styling, position and dismissal each fail
under mutation. 20 of 20 suites green, 34 tests in test_tagrules.

Closes item 83.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

1 files changed, 370 insertions, 6 deletions

diff --git a/tests/test_tagrules.cpp b/tests/test_tagrules.cpp
index ed3a06d..cc28711 100644
--- a/tests/test_tagrules.cpp
+++ b/tests/test_tagrules.cpp
@@ -38,6 +38,15 @@ private slots:
     void aRuleLoadsWithEveryField();
     void absentFieldsTakeTheirDefaults();
     void aMalformedRuleIsDroppedWithAWarning();
+    void aRuleWithABadIdLoadsForRepairRatherThanVanishing();
+    void aTypedNameIsSanitisedIntoAnId();
+    void aSanitisedNameThatCollidesGetsItsOwnId();
+    void savingIsRefusedWhenARuleWouldNotLoadBack();
+    void aRepairedIdSurvivesASaveAndReload();
+    void theWarningReadsAsAWarningAndSitsBesideSave();
+    void aDismissedWarningComesBackWhenThereIsSomethingNewToSay();
+    void aNameTypedWithSpacesIsSanitisedInTheField();
+    void aRuleAddedAndNamedInTheDialogSurvivesAReopen();
     void unknownFieldsSurviveASave();
     void stageOrderPutsAccountsFirst();
     void aQueryWithQuotesRoundTrips();
@@ -128,16 +137,19 @@ void TestTagRules::absentFieldsTakeTheirDefaults()
 
 void TestTagRules::aMalformedRuleIsDroppedWithAWarning()
 {
-    // One bad rule must not cost the others. Four separate defects, and the
-    // good rule sits first so a parser that stops at the first problem is
-    // caught by the count rather than by an empty list.
+    // One bad rule must not cost the others. The good rule sits first so a
+    // parser that stops at the first problem is caught by the count rather
+    // than by an empty list.
+    //
+    // A rule with nothing to run is still dropped: no query and no tags are
+    // both unrepairable without inventing the user's intent. A rule whose only
+    // fault is its ID is NOT dropped any more, see the next test.
     const QString path = writeRules(R"({
       "version": 1,
       "rules": [
         {"id": "good", "add": ["x"], "query": "from:a@example.com"},
         {"id": "no-query", "add": ["y"]},
-        {"id": "no-tags", "query": "from:b@example.com"},
-        {"id": "Bad Id", "add": ["z"], "query": "from:c@example.com"}
+        {"id": "no-tags", "query": "from:b@example.com"}
       ]
     })");
 
@@ -146,7 +158,164 @@ void TestTagRules::aMalformedRuleIsDroppedWithAWarning()
 
     QCOMPARE(rules.rules().size(), 1);
     QCOMPARE(rules.rules().first().id, QStringLiteral("good"));
-    QCOMPARE(rules.warnings().size(), 3);
+    QCOMPARE(rules.warnings().size(), 2);
+}
+
+void TestTagRules::aRuleWithABadIdLoadsForRepairRatherThanVanishing()
+{
+    // The defect this whole change exists for. A rule saved with a space in
+    // its id was written to the file correctly, dropped on every load, and so
+    // was invisible in the dialog while still occupying the file. The next
+    // save from the dialog would then have deleted it for good.
+    //
+    // It now loads, carrying its repaired id, so the dialog can show it and
+    // the user can fix it. The warning still fires: the file on disk is not
+    // what the hook will run until it is saved back.
+    const QString path = writeRules(R"({
+      "version": 1,
+      "rules": [
+        {"id": "justeat orders", "add": ["promo"],
+         "query": "from:no-reply@order.example.com"}
+      ]
+    })");
+
+    TagRules rules;
+    rules.load(path);
+
+    QCOMPARE(rules.rules().size(), 1);
+    QCOMPARE(rules.rules().first().id, QStringLiteral("justeat-orders"));
+    QCOMPARE(rules.rules().first().add, QStringList{ QStringLiteral("promo") });
+    QCOMPARE(rules.warnings().size(), 1);
+    QVERIFY(rules.warnings().first().contains(QStringLiteral("justeat orders")));
+}
+
+void TestTagRules::aTypedNameIsSanitisedIntoAnId()
+{
+    // Spaces, capitals and punctuation are what a person types into a field
+    // labelled "Name". Each case here is one the user is likely to produce,
+    // and every result has to satisfy ^[a-z0-9][a-z0-9-]*$ or the hook drops
+    // it.
+    QCOMPARE(TagRules::sanitiseId(QStringLiteral("justeat orders")),
+             QStringLiteral("justeat-orders"));
+    QCOMPARE(TagRules::sanitiseId(QStringLiteral("JustEat Orders")),
+             QStringLiteral("justeat-orders"));
+    QCOMPARE(TagRules::sanitiseId(QStringLiteral("Notify: PayPal!")),
+             QStringLiteral("notify-paypal"));
+    QCOMPARE(TagRules::sanitiseId(QStringLiteral("  spaced  out  ")),
+             QStringLiteral("spaced-out"));
+    QCOMPARE(TagRules::sanitiseId(QStringLiteral("-leading-dash")),
+             QStringLiteral("leading-dash"));
+
+    // A run of dashes is collapsed only when the name needed sanitising at
+    // all: "a---b" already satisfies the pattern and is left exactly as it is,
+    // because rewriting legal ids would churn the file mailctl also reads.
+    QCOMPARE(TagRules::sanitiseId(QStringLiteral("a---b")),
+             QStringLiteral("a---b"));
+    QCOMPARE(TagRules::sanitiseId(QStringLiteral("a - - b")),
+             QStringLiteral("a-b"));
+    QCOMPARE(TagRules::sanitiseId(QStringLiteral("mailing-list/SBo")),
+             QStringLiteral("mailing-list-sbo"));
+
+    // An id may not START with a dash or a digit-less symbol run, and a name
+    // made only of punctuation sanitises to nothing. Empty is not a legal id,
+    // so the caller has to supply a fallback rather than writing one out.
+    QCOMPARE(TagRules::sanitiseId(QStringLiteral("!!!")), QString());
+    QCOMPARE(TagRules::sanitiseId(QString()), QString());
+
+    // Already valid ids pass through untouched, or every load would rewrite
+    // the file and show mailctl a diff the user never made.
+    QCOMPARE(TagRules::sanitiseId(QStringLiteral("notify-github")),
+             QStringLiteral("notify-github"));
+}
+
+void TestTagRules::aSanitisedNameThatCollidesGetsItsOwnId()
+{
+    // Sanitising maps many names onto one id, so it can manufacture the exact
+    // duplicate that load() drops. "Justeat Orders" and "justeat orders" both
+    // reduce to justeat-orders; the second must not silently become the first.
+    const QStringList taken{ QStringLiteral("justeat-orders"),
+                             QStringLiteral("justeat-orders-2") };
+
+    QCOMPARE(TagRules::uniqueId(QStringLiteral("Justeat Orders"), taken),
+             QStringLiteral("justeat-orders-3"));
+
+    // No collision means no suffix.
+    QCOMPARE(TagRules::uniqueId(QStringLiteral("promo"), taken),
+             QStringLiteral("promo"));
+
+    // A name that sanitises to nothing still has to produce a legal id.
+    const QString fallback = TagRules::uniqueId(QStringLiteral("!!!"), taken);
+    QVERIFY(!fallback.isEmpty());
+    QVERIFY(TagRules::isValidId(fallback));
+}
+
+void TestTagRules::savingIsRefusedWhenARuleWouldNotLoadBack()
+{
+    // The asymmetry that caused the bug: save wrote anything, load validated.
+    // validate() is the one predicate both sides now use, so a rule that
+    // would not survive a reload is reported BEFORE it reaches the file.
+    TagRule good;
+    good.id = QStringLiteral("good");
+    good.query = QStringLiteral("from:a@example.com");
+    good.add = { QStringLiteral("x") };
+
+    TagRule noQuery;
+    noQuery.id = QStringLiteral("no-query");
+    noQuery.add = { QStringLiteral("y") };
+
+    TagRule noTags;
+    noTags.id = QStringLiteral("no-tags");
+    noTags.query = QStringLiteral("from:b@example.com");
+
+    TagRule badId;
+    badId.id = QStringLiteral("Bad Id");
+    badId.query = QStringLiteral("from:c@example.com");
+    badId.add = { QStringLiteral("z") };
+
+    QVERIFY(TagRules::validate({ good }).isEmpty());
+
+    const QStringList problems =
+        TagRules::validate({ good, noQuery, noTags, badId });
+    QCOMPARE(problems.size(), 3);
+    QVERIFY(problems.join(QChar(' ')).contains(QStringLiteral("no-query")));
+    QVERIFY(problems.join(QChar(' ')).contains(QStringLiteral("no-tags")));
+    QVERIFY(problems.join(QChar(' ')).contains(QStringLiteral("Bad Id")));
+
+    // A duplicate id survives a save and is then dropped on load, so it is a
+    // save-time problem too even though each rule is fine on its own.
+    TagRule twin = good;
+    QCOMPARE(TagRules::validate({ good, twin }).size(), 1);
+}
+
+void TestTagRules::aRepairedIdSurvivesASaveAndReload()
+{
+    // End to end, and the assertion that matters to the user: the rule they
+    // could not keep is still there after the round trip, with its tags, its
+    // query and its note intact.
+    const QString path = writeRules(R"({
+      "version": 1,
+      "rules": [
+        {"id": "justeat orders", "add": ["promo"], "stage": 50,
+         "note": "kept", "query": "from:no-reply@order.example.com"}
+      ]
+    })");
+
+    TagRules loaded;
+    loaded.load(path);
+    QCOMPARE(loaded.rules().size(), 1);
+    QVERIFY(loaded.save(path));
+
+    TagRules reread;
+    reread.load(path);
+    QCOMPARE(reread.rules().size(), 1);
+    QCOMPARE(reread.rules().first().id, QStringLiteral("justeat-orders"));
+    QCOMPARE(reread.rules().first().note, QStringLiteral("kept"));
+    QCOMPARE(reread.rules().first().query,
+             QStringLiteral("from:no-reply@order.example.com"));
+
+    // Repaired on the way in, so the second read has nothing left to complain
+    // about. A warning that never clears trains the user to ignore it.
+    QVERIFY(reread.warnings().isEmpty());
 }
 
 void TestTagRules::unknownFieldsSurviveASave()
@@ -560,6 +729,201 @@ void TestTagRules::leavingTextModeIsRefusedWhenTheQueryCannotBeShownAsRows()
              "a stale refusal must not outlive the query that caused it");
 }
 
+void TestTagRules::theWarningReadsAsAWarningAndSitsBesideSave()
+{
+    // The label was correct and unread: same font and colour as the intro
+    // prose two lines above it, so it looked like more explanation. The user
+    // opened this dialog repeatedly, with the warning showing every time,
+    // while hunting the rule it was telling them about.
+    //
+    // Asserted on the widget's own properties, not on a render. CLAUDE.md
+    // records why a pixel probe cannot carry this: counting lit pixels cannot
+    // tell one colour from another reliably, and viewport()->render() returns
+    // blank often enough that a probe reporting "no red anywhere" says more
+    // about the probe than the code.
+    QTemporaryDir configHome;
+    QVERIFY(configHome.isValid());
+    qputenv("XDG_CONFIG_HOME", configHome.path().toUtf8());
+    QVERIFY(QDir().mkpath(configHome.filePath(QStringLiteral("mailrules"))));
+
+    const QString stored = configHome.filePath(
+        QStringLiteral("mailrules/rules.json"));
+    QFile out(stored);
+    QVERIFY(out.open(QIODevice::WriteOnly));
+    // A rule that warns on load, so the label is populated by opening alone.
+    out.write(R"({
+      "version": 1,
+      "rules": [
+        {"id": "justeat orders", "query": "from:no-reply@order.example.com",
+         "add": ["promo"], "stage": 50, "enabled": true}
+      ]
+    })");
+    out.close();
+
+    TagRulesDialog dialog;
+
+    // The guard. Everything below asserts about a warning that is showing, and
+    // all of it would pass vacuously against a label that never appears.
+    QVERIFY2(!dialog.warningTextForTest().isEmpty(),
+             "a repaired rule must warn, or this test proves nothing");
+
+    QVERIFY2(dialog.warningTextForTest().contains(QStringLiteral("justeat")),
+             "the warning must name the rule it is about");
+
+    const QString style = dialog.warningStyleForTest();
+    QVERIFY2(style.contains(QStringLiteral("background-color")),
+             "a warning that is not filled reads as ordinary prose");
+    QVERIFY2(style.contains(QStringLiteral("bold")), "and it must be bold");
+
+    // Below the rule list, next to the button whose outcome it reports. The
+    // intro sits at the top, so comparing against it pins the move: this
+    // assertion fails if the label drifts back under the header.
+    QVERIFY2(dialog.warningIsBelowTheRuleListForTest(),
+             "the warning belongs beside Save, not under the intro text");
+
+    // Plain text, because the strings interpolate ids and queries read from
+    // the file. A query holding '<' would otherwise be swallowed as markup.
+    QCOMPARE(dialog.warningTextFormatForTest(), Qt::PlainText);
+}
+
+void TestTagRules::aDismissedWarningComesBackWhenThereIsSomethingNewToSay()
+{
+    // Dismissal is per-appearance. The warning most often says the file is not
+    // yet what the hook runs, so a persistent "do not show again" would rehide
+    // the exact problem that went unnoticed for a session. Closing it clears
+    // this one; the next thing worth saying shows it again.
+    QTemporaryDir configHome;
+    QVERIFY(configHome.isValid());
+    qputenv("XDG_CONFIG_HOME", configHome.path().toUtf8());
+    QVERIFY(QDir().mkpath(configHome.filePath(QStringLiteral("mailrules"))));
+
+    const QString stored = configHome.filePath(
+        QStringLiteral("mailrules/rules.json"));
+    QFile out(stored);
+    QVERIFY(out.open(QIODevice::WriteOnly));
+    out.write(R"({
+      "version": 1,
+      "rules": [
+        {"id": "justeat orders", "query": "from:no-reply@order.example.com",
+         "add": ["promo"], "stage": 50, "enabled": true}
+      ]
+    })");
+    out.close();
+
+    TagRulesDialog dialog;
+    QVERIFY2(!dialog.warningTextForTest().isEmpty(),
+             "the repaired rule must warn, or the dismissal proves nothing");
+
+    dialog.dismissWarningForTest();
+    QVERIFY2(dialog.warningTextForTest().isEmpty(),
+             "the X must actually clear the warning");
+
+    // Something new to say: a rule that cannot be saved. The dismissal must
+    // not have latched the banner shut.
+    dialog.setNameForTest(QStringLiteral("second"));
+    dialog.addRuleForTest();
+    dialog.setTagsForTest(QString());
+    dialog.setTextModeForTest(true);
+    dialog.setQueryTextForTest(QString());
+    dialog.saveForTest();
+
+    QVERIFY2(!dialog.warningTextForTest().isEmpty(),
+             "a refusal after a dismissal must still be shown");
+}
+
+void TestTagRules::aNameTypedWithSpacesIsSanitisedInTheField()
+{
+    // The field is labelled "Name", so a person types prose into it. What the
+    // field SHOWS after the edit is committed is the assertion: sanitising
+    // silently on save would leave the user looking at a name that is not the
+    // one being written.
+    QTemporaryDir configHome;
+    QVERIFY(configHome.isValid());
+    qputenv("XDG_CONFIG_HOME", configHome.path().toUtf8());
+    QVERIFY(QDir().mkpath(configHome.filePath(QStringLiteral("mailrules"))));
+
+    const QString stored = configHome.filePath(
+        QStringLiteral("mailrules/rules.json"));
+    QFile out(stored);
+    QVERIFY(out.open(QIODevice::WriteOnly));
+    out.write(R"({
+      "version": 1,
+      "rules": [
+        {"id": "vendor", "query": "from:vendor.example.org",
+         "add": ["vendor"], "stage": 50, "enabled": true}
+      ]
+    })");
+    out.close();
+
+    TagRulesDialog dialog;
+    dialog.setNameForTest(QStringLiteral("Justeat orders"));
+    QCOMPARE(dialog.nameLineForTest(), QStringLiteral("justeat-orders"));
+
+    dialog.saveForTest();
+
+    TagRules reloaded;
+    reloaded.load(stored);
+    QCOMPARE(reloaded.rules().size(), 1);
+    QCOMPARE(reloaded.rules().first().id, QStringLiteral("justeat-orders"));
+    QVERIFY2(reloaded.warnings().isEmpty(),
+             "a rule saved from the dialog must load back without complaint");
+}
+
+void TestTagRules::aRuleAddedAndNamedInTheDialogSurvivesAReopen()
+{
+    // The user's session, end to end: add a rule, name it in prose, fill in
+    // the query and tags, save, reopen. Before the fix the rule was written to
+    // the file with a space in its id and dropped by every reader, so the
+    // dialog came back without it and the file kept a rule nothing would run.
+    QTemporaryDir configHome;
+    QVERIFY(configHome.isValid());
+    qputenv("XDG_CONFIG_HOME", configHome.path().toUtf8());
+    QVERIFY(QDir().mkpath(configHome.filePath(QStringLiteral("mailrules"))));
+
+    const QString stored = configHome.filePath(
+        QStringLiteral("mailrules/rules.json"));
+    QFile out(stored);
+    QVERIFY(out.open(QIODevice::WriteOnly));
+    out.write(R"({
+      "version": 1,
+      "rules": [
+        {"id": "vendor", "query": "from:vendor.example.org",
+         "add": ["vendor"], "stage": 50, "enabled": true}
+      ]
+    })");
+    out.close();
+
+    {
+        TagRulesDialog dialog;
+        QCOMPARE(dialog.ruleCountForTest(), 1);
+
+        dialog.addRuleForTest();
+        QCOMPARE(dialog.ruleCountForTest(), 2);
+
+        dialog.setNameForTest(QStringLiteral("Justeat orders"));
+        dialog.setTextModeForTest(true);
+        dialog.setQueryTextForTest(
+            QStringLiteral("from:no-reply@order.example.com"));
+        dialog.setTagsForTest(QStringLiteral("promo, notify/justeat"));
+        dialog.saveForTest();
+
+        QVERIFY2(dialog.warningTextForTest().isEmpty(),
+                 "a complete rule must not be refused");
+    }
+
+    TagRules reloaded;
+    reloaded.load(stored);
+    QCOMPARE(reloaded.rules().size(), 2);
+
+    const TagRule added = reloaded.rules().at(1);
+    QCOMPARE(added.id, QStringLiteral("justeat-orders"));
+    QCOMPARE(added.query,
+             QStringLiteral("from:no-reply@order.example.com"));
+    QCOMPARE(added.add, (QStringList{ QStringLiteral("promo"),
+                                      QStringLiteral("notify/justeat") }));
+    QVERIFY(reloaded.warnings().isEmpty());
+}
+
 void TestTagRules::aFolderRowUsesTheDropdownAndKeepsItsSuffix()
 {
     // A path: without its suffix matches nothing and notmuch says nothing