From fe5703419f2ac2a5e619b3d527530a71a9a9499e Mon Sep 17 00:00:00 2001 From: "Danilo M." Date: Thu, 13 Aug 2026 11:03:38 +0200 Subject: docs,rulequery: state the tag quoting rule rather than the test The draft compile() quoted every Is/IsNot term, which contradicted the same task's own assertion that a negated tag compiles to . The implementer resolved it in the direction the tests specify, and the resolution is right: notmuch reads tag:inbox and tag:"inbox" identically, counting 5322 either way against the live index, so quoting a tag would change the stored string without changing what it matches. That breaks the byte-for-byte round trip this type exists to guarantee. Restate the comment as the rule rather than as a note about what a test expects, correct the plan's draft so the remaining tasks do not inherit the contradiction, and warn the parser task that a quoted tag must not be read back as a quoting operator. --- docs/superpowers/plans/2026-08-13-rule-builder.md | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-) (limited to 'docs/superpowers/plans/2026-08-13-rule-builder.md') diff --git a/docs/superpowers/plans/2026-08-13-rule-builder.md b/docs/superpowers/plans/2026-08-13-rule-builder.md index fb39cca..4f65f7d 100644 --- a/docs/superpowers/plans/2026-08-13-rule-builder.md +++ b/docs/superpowers/plans/2026-08-13-rule-builder.md @@ -376,9 +376,20 @@ bool needsQuotes(const RuleTerm &term) { if (term.field == RuleTerm::Folder) return true; - if (term.op == RuleTerm::Is || term.op == RuleTerm::IsNot) + if (term.value.contains(QLatin1Char(' '))) return true; - return term.value.contains(QLatin1Char(' ')); + // Is/IsNot means an exact phrase, and only the free-text fields need + // quotes to express one. A tag or an attachment name is a single bare + // token to notmuch, which reads `tag:inbox` and `tag:"inbox"` identically + // (both count 5322 against the live index). Quoting them would therefore + // change the stored string without changing what it matches, and this + // type's whole contract is that an unedited rule compiles back byte for + // byte. + if (term.op == RuleTerm::Is || term.op == RuleTerm::IsNot) { + return term.field == RuleTerm::From || term.field == RuleTerm::To + || term.field == RuleTerm::Cc || term.field == RuleTerm::Subject; + } + return false; } QString compileTerm(const RuleTerm &term) @@ -793,6 +804,9 @@ bool parseTerm(const QString &token, RuleTerm *out) return !value.isEmpty(); } + // Tag and Attachment compile unquoted (see needsQuotes in Task 2), so + // their operator must not be inferred from the quoting: reading a quoted + // tag back as Is would compile it unquoted and change the stored string. if (field == RuleTerm::Attachment) out->op = RuleTerm::Has; else if (field == RuleTerm::Tag) -- cgit v1.2.3 From cff230c8e9dba7a95c8ee28932b7a299fc0d994a Mon Sep 17 00:00:00 2001 From: "Danilo M." Date: Thu, 13 Aug 2026 11:10:35 +0200 Subject: docs: correct the claim that a paren-bearing value is unrepresentable The spec listed from:(((( among the queries the parser must reject, and the plan's Task 6 asserted that rejection. Probing the built parser shows it accepts the query as a From row whose value is the literal text, and compiles it back byte for byte. That is correct behaviour, not a leak in the strictness rule. notmuch reads those parens as characters to search for rather than as grouping, so the query is meaningful and the row displaying it tells the truth. Rejecting it would buy nothing and would push a representable rule into text mode. The distinction the documents were missing: a parenthesis inside a VALUE is not a shape question at all, only a parenthesis in grouping position is. Restate both documents accordingly, and replace the assertion with a round-trip one, which is the property that actually matters here. --- docs/superpowers/plans/2026-08-13-rule-builder.md | 21 +++++++++++++++++---- .../specs/2026-08-13-rule-builder-design.md | 21 ++++++++++++++++----- 2 files changed, 33 insertions(+), 9 deletions(-) (limited to 'docs/superpowers/plans/2026-08-13-rule-builder.md') diff --git a/docs/superpowers/plans/2026-08-13-rule-builder.md b/docs/superpowers/plans/2026-08-13-rule-builder.md index 4f65f7d..96fddc6 100644 --- a/docs/superpowers/plans/2026-08-13-rule-builder.md +++ b/docs/superpowers/plans/2026-08-13-rule-builder.md @@ -1113,6 +1113,11 @@ void TestRuleQuery::anUnrepresentableQueryRejectsWhole() QStringLiteral("date:2026-01-01..2026-02-01"), // two-sided range QStringLiteral("from:a.example.org xor subject:x"), }; + // NOT in this list: `from:((((`. It parses, as a From row whose value is + // the literal text `((((`, and round-trips byte for byte. That is exactly + // what the query means to notmuch, which treats the parens as characters + // to search for rather than as grouping, so the row tells the truth and + // rejecting it would buy nothing. See the test below. for (const QString &query : unrepresentable) { const RuleQuery q = RuleQuery::parse(query); @@ -1126,11 +1131,19 @@ void TestRuleQuery::anUnrepresentableQueryRejectsWhole() void TestRuleQuery::aMalformedQueryIsRejectedNotDiagnosed() { - // notmuch accepts `from:((((` cleanly and matches nothing, so there is no - // failure to observe and a test asserting one fails against correct code. - // The assertion is on OUR rejection only. + // notmuch accepts `from:((((` cleanly and matches nothing: the parens are + // characters it searches for, not grouping. So there is no failure to + // observe, and a test asserting one fails against correct code. + // + // This parser accepts it too, as a From row whose value is that literal + // text, which is what the query actually means. What must hold is the + // round trip, not a rejection: displaying it as a row and compiling it + // back must not alter the stored string. const RuleQuery q = RuleQuery::parse(QStringLiteral("from:((((")); - QVERIFY(!q.parsed); + QVERIFY(q.parsed); + QCOMPARE(q.terms.size(), 1); + QCOMPARE(q.terms.at(0).value, QStringLiteral("((((")); + QCOMPARE(q.compile(), QStringLiteral("from:((((")); } ``` diff --git a/docs/superpowers/specs/2026-08-13-rule-builder-design.md b/docs/superpowers/specs/2026-08-13-rule-builder-design.md index c11ef47..a571506 100644 --- a/docs/superpowers/specs/2026-08-13-rule-builder-design.md +++ b/docs/superpowers/specs/2026-08-13-rule-builder-design.md @@ -221,7 +221,12 @@ an empty query and must open in the builder ready to receive a row. Everything else sets `parsed = false`: nested parens beyond that one shape, mixed `and`/`or` without parens, `xor`, an unrecognised prefix (`body:`, `mid:`, -`folder:`), a bare word with no prefix, a `path:` not ending in `/**`. +`folder:`), a bare word with no prefix, a `path:` not ending in `/**`, a +two-sided `date:` range, a trailing operator, an unterminated quote. + +A parenthesis **inside a value** is not a shape at all: `from:((((` is a From +term whose value happens to contain parens, and it parses and round-trips like +any other. Only a parenthesis in grouping position is a shape question. ### The parser is strict, and that is the safety property @@ -334,10 +339,16 @@ terms and tests exactly the same thing. **Rejection tests**, which carry the safety property. Queries that must set `parsed = false` and must not partially parse: nested `or` inside `or`, mixed -`and`/`or` without parens, `body:foo`, a bare word, a `path:` without `/**`, and -`from:((((`. That last asserts **our** rejection, never a provoked notmuch -failure: `CLAUDE.md` records twice that notmuch accepts it cleanly, and a test -expecting an error there fails against correct code. +`and`/`or` without parens, `body:foo`, a bare word, a `path:` without `/**`, a +two-sided `date:` range, and a trailing operator. + +**`from:((((` is not among them, and the reason is worth stating.** notmuch +treats those parens as characters to search for rather than as grouping, so the +query is meaningful, matches nothing, and reports no error. This parser accepts +it as a From row whose value is that literal text, which is what it means. The +assertion there is the **round trip**, never a rejection and never a provoked +notmuch failure: `CLAUDE.md` records twice that notmuch accepts it cleanly, and +a test expecting an error fails against correct code. **Compile tests** for every field and operator pair including both negations, and the parenthesisation rule at its boundary: `join == Any` with zero -- cgit v1.2.3