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, 100 insertions, 8 deletions

diff --git a/src/tagrules.cpp b/src/tagrules.cpp
index 39cc529..0049278 100644
--- a/src/tagrules.cpp
+++ b/src/tagrules.cpp
@@ -58,8 +58,96 @@ QStringList stringsOf(const QJsonValue &value)
     return out;
 }
 
+/// The id pattern, in one place. mailrules.py enforces the same one; see
+/// "Changing the shared rule format" in CLAUDE.md before touching it.
+const QRegularExpression &idPattern()
+{
+    static const QRegularExpression pattern(
+        QStringLiteral("^[a-z0-9][a-z0-9-]*$"));
+    return pattern;
+}
+
 } // namespace
 
+bool TagRules::isValidId(const QString &id)
+{
+    return idPattern().match(id).hasMatch();
+}
+
+QString TagRules::sanitiseId(const QString &name)
+{
+    // Untouched when already legal: sanitising every id on load would rewrite
+    // a good file and show mailctl a diff the user never made.
+    if (isValidId(name))
+        return name;
+
+    QString out;
+    out.reserve(name.size());
+    for (const QChar ch : name.toLower()) {
+        if ((ch >= QLatin1Char('a') && ch <= QLatin1Char('z'))
+            || (ch >= QLatin1Char('0') && ch <= QLatin1Char('9'))) {
+            out.append(ch);
+        } else if (!out.isEmpty() && !out.endsWith(QLatin1Char('-'))) {
+            // One dash per run of anything else, so "Notify: PayPal!" does not
+            // become "notify--paypal-".
+            out.append(QLatin1Char('-'));
+        }
+    }
+    while (out.endsWith(QLatin1Char('-')))
+        out.chop(1);
+
+    // Empty rather than a manufactured id. The caller knows what the rule is
+    // and can name it; this function inventing "rule-1" would hide the fact
+    // that nothing of the typed name survived.
+    return out;
+}
+
+QString TagRules::uniqueId(const QString &name, const QStringList &taken)
+{
+    QString base = sanitiseId(name);
+    if (base.isEmpty())
+        base = QStringLiteral("rule");
+    if (!taken.contains(base))
+        return base;
+
+    // Starts at 2: the unsuffixed id is the first, so "-2" reads as the second
+    // rule of that name rather than as an index.
+    for (int suffix = 2;; ++suffix) {
+        const QString candidate =
+            base + QLatin1Char('-') + QString::number(suffix);
+        if (!taken.contains(candidate))
+            return candidate;
+    }
+}
+
+QStringList TagRules::validate(const QList<TagRule> &rules)
+{
+    QStringList problems;
+    QStringList seen;
+    for (const TagRule &rule : rules) {
+        const QString where =
+            rule.id.isEmpty() ? QObject::tr("(unnamed)") : rule.id;
+
+        if (!isValidId(rule.id)) {
+            problems.append(
+                QObject::tr("'%1': the name must be lowercase letters, digits "
+                            "and dashes").arg(where));
+        } else if (seen.contains(rule.id)) {
+            problems.append(
+                QObject::tr("'%1': another rule already has this name")
+                    .arg(where));
+        }
+        seen.append(rule.id);
+
+        if (rule.query.trimmed().isEmpty())
+            problems.append(QObject::tr("'%1': no query").arg(where));
+        if (rule.add.isEmpty() && rule.remove.isEmpty())
+            problems.append(QObject::tr("'%1': adds and removes nothing")
+                                .arg(where));
+    }
+    return problems;
+}
+
 QString TagRules::defaultPath()
 {
     // Not QStandardPaths::ConfigLocation: that appends the organization and
@@ -121,9 +209,12 @@ void TagRules::load(const QString &path)
     }
 
     // An id is a handle: a UI selects on it and a diff tracks it.
-    static const QRegularExpression idPattern(
-        QStringLiteral("^[a-z0-9][a-z0-9-]*$"));
-
+    //
+    // A bad id is REPAIRED rather than dropped. Dropping it made the rule
+    // invisible in the dialog while it still occupied the file, so the next
+    // save deleted it outright: the user wrote a rule, saw it vanish, wrote it
+    // again, and lost it again. The warning still fires, because what is on
+    // disk is not what the hook runs until the file is saved back.
     QStringList seen;
     const QJsonArray array = root.value(QStringLiteral("rules")).toArray();
     for (int index = 0; index < array.size(); ++index) {
@@ -132,12 +223,13 @@ void TagRules::load(const QString &path)
 
         TagRule rule;
         rule.id = object.value(QStringLiteral("id")).toString();
-        if (!idPattern.match(rule.id).hasMatch()) {
+        if (!isValidId(rule.id)) {
+            const QString repaired = uniqueId(rule.id, seen);
             m_warnings.append(
-                QObject::tr("%1: id '%2' is missing or not lowercase letters, "
-                            "digits and dashes; dropped")
-                    .arg(where, rule.id));
-            continue;
+                QObject::tr("%1: name '%2' is not lowercase letters, digits "
+                            "and dashes; loaded as '%3'. Save to keep it.")
+                    .arg(where, rule.id, repaired));
+            rule.id = repaired;
         }
 
         if (seen.contains(rule.id)) {