From f1a232ff1c6d1d0888ddd72b493ebfcfc24bc0e7 Mon Sep 17 00:00:00 2001 From: Jakob Heuser Date: Wed, 23 Sep 2026 12:16:14 -0700 Subject: [PATCH] fix(vale): reject a backreference in a swap key MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit No capture group survives a `swap` key. `NewSubstitution` compiles all of a rule's keys into one alternation, wrapping each in a capture group of its own because the index of the group that matched is how it recovers the replacement; any group the author wrote is rewritten to `(?:…)` by `convertCaptureGroups` first. Either way `\1` points at Vale's wrapper, the group still being matched, and matches nothing. Vale reports none of this: the rule loads, runs, writes nothing to stderr, and never fires. Measured against the vendored 3.22.0 binary, `(\w+) \1`, `(the) \1`, `the \1` and `(?:\w+) \1` are all silent as swap keys and all fire under `tokens` and `raw`. The detector is character-class aware because `[\1a]bc` was measured matching `abc`, where `\1` is an octal escape. Also corrects the mechanism the recipe and the vendor test gave for why lookaround and backreferences work at all. Vale compiles every pattern with regexp2 in RE2 mode unconditionally (internal/regex/regex.go:63); there is no Go-`regexp`-first path and no fallback. The conclusion was right, the explanation was not. `create-vale-rule` goes to v14, adds the `$1`-in-the-value asymmetry and the leading-lookbehind mirror of the trailing-lookahead limit, and the vendor contract now measures `consistency` and `conditional` so a widening is caught, not assumed. --- .changeset/vale-swap-backreference.md | 5 + .../.openspec.yaml | 2 + .../proposal.md | 71 +++++++ .../specs/cli-rule-validation/spec.md | 47 +++++ .../tasks.md | 40 ++++ openspec/specs/cli-rule-validation/spec.md | 46 +++++ packages/cli/src/agent/create-vale-rule.md | 76 +++++--- packages/cli/src/schemas/vale-rule.ts | 132 +++++++++++++ .../cli/test/vale-schema-contract.test.ts | 147 +++++++++++++++ .../cli/test/vale-vendor-contract.test.ts | 177 ++++++++++++++++-- 10 files changed, 705 insertions(+), 38 deletions(-) create mode 100644 .changeset/vale-swap-backreference.md create mode 100644 openspec/changes/archive/2026-09-23-vale-swap-backreference/.openspec.yaml create mode 100644 openspec/changes/archive/2026-09-23-vale-swap-backreference/proposal.md create mode 100644 openspec/changes/archive/2026-09-23-vale-swap-backreference/specs/cli-rule-validation/spec.md create mode 100644 openspec/changes/archive/2026-09-23-vale-swap-backreference/tasks.md diff --git a/.changeset/vale-swap-backreference.md b/.changeset/vale-swap-backreference.md new file mode 100644 index 00000000..d6b47744 --- /dev/null +++ b/.changeset/vale-swap-backreference.md @@ -0,0 +1,5 @@ +--- +"@taskless/cli": patch +--- + +`verify` now rejects a Vale `substitution` rule whose `swap` key carries a backreference. No capture group survives a swap key: Vale compiles a rule's keys into one alternation, wrapping each in a capture group of its own and rewriting the author's groups to non-capturing, so `\1` refers to Vale's wrapper and matches nothing. Vale reports none of this, so the rule loads, runs and silently never fires. The detector is character-class aware, since `\1` inside `[…]` is an octal escape and works. `$1` in the swap _value_ is unaffected and still works. The `create-vale-rule` topic goes to v14: the claim that Vale tries Go's `regexp` before falling back to `regexp2` is removed (there is no fallback; it compiles with `regexp2` unconditionally), the `swap` constraint is generalised, and the leading-lookbehind mirror of the trailing-lookahead limit is documented. diff --git a/openspec/changes/archive/2026-09-23-vale-swap-backreference/.openspec.yaml b/openspec/changes/archive/2026-09-23-vale-swap-backreference/.openspec.yaml new file mode 100644 index 00000000..265da3d9 --- /dev/null +++ b/openspec/changes/archive/2026-09-23-vale-swap-backreference/.openspec.yaml @@ -0,0 +1,2 @@ +schema: spec-driven +created: 2026-09-23 diff --git a/openspec/changes/archive/2026-09-23-vale-swap-backreference/proposal.md b/openspec/changes/archive/2026-09-23-vale-swap-backreference/proposal.md new file mode 100644 index 00000000..fdc3eb5e --- /dev/null +++ b/openspec/changes/archive/2026-09-23-vale-swap-backreference/proposal.md @@ -0,0 +1,71 @@ +## Why + +A backreference in a Vale `swap` key can never match, and Vale says nothing +about it. The rule loads, runs on every document, writes nothing to stderr, and +reports no finding. A silently dead rule is indistinguishable from a clean +project, so the author's own `fail/` fixture is the only thing that can catch +it — and by then they have already written the rule. + +The cause is not the regex engine. Vale compiles every pattern with `regexp2` +in RE2 compatibility mode, unconditionally (`internal/regex/regex.go:63`), so +backreferences work under `tokens`, under `raw`, in a `consistency` key and in a +`conditional` `first`. It is that **no capture group survives a `swap` key**: +`NewSubstitution` compiles all of a rule's keys into one alternation, wrapping +each key in a capture group of its own because the index of the group that +matched is how it recovers the replacement. Those wrappers must be numbered +1..n, so any group the author wrote is rewritten to `(?:…)` by +`convertCaptureGroups` first. Either way `\1` points at Vale's wrapper — the +group still being matched — and matches nothing. + +The standing spec covers the neighbouring case and not this one. "Behavior a +schema cannot express stays explicit" is scoped to a rule shape that _crashes_ +the binary; this shape does not crash it, and the justification a crash supplies +(one config for the whole run, no findings for any rule) is not available here. +That gap is what this change closes: it states when the schema may reject a +pattern the binary accepts, and what such a rejection owes the author. + +## What Changes + +- **`cli-rule-validation`** gains one requirement covering a rejection for a + pattern the binary accepts but can never honor, with the evidence bar and the + message obligations such a rejection carries. + +Code, already implemented on this branch: + +- The Vale style-layer schema rejects a `swap` key carrying a live + backreference. The detector is character-class aware, because inside `[…]` a + `\1` is an octal escape and the key works. +- `create-vale-rule` goes to topic v14: the two-engine fallback claim is + removed (it was never true), the `swap` constraint is generalised from "a + backreference does nothing" to "no capture group survives a swap key", the + `$1`-in-the-value asymmetry is documented, and the leading-lookbehind mirror + of the existing trailing-lookahead limit is added. +- `vale-vendor-contract.test.ts` pins the mechanism, the false-positive + candidates, and — newly — that `consistency` and `conditional` are NOT + affected, so a widening of the blast radius is caught rather than assumed. + +## Capabilities + +### New Capabilities + +None. + +### Modified Capabilities + +- `cli-rule-validation`: one ADDED requirement. No standing requirement is + restated, renamed or removed. + +## Impact + +One new schema rejection, `patch`. The migration cost was measured: across the +seven published Vale style packages plus this repository's own rules, 1,502 +`swap` keys in 161 rule files, **0** carry a live backreference. `check` is +unaffected (it does not run the style-layer schema over authored rules the way +`verify` does), and `test` already fails a rule whose `fail/` fixture never +fires. + +## Delivery shape + +**Single PR.** The spec, the schema change, the recipe bump and the tests are +one reviewable diff and are only correct together. It is the tip, so the change +is archived here. diff --git a/openspec/changes/archive/2026-09-23-vale-swap-backreference/specs/cli-rule-validation/spec.md b/openspec/changes/archive/2026-09-23-vale-swap-backreference/specs/cli-rule-validation/spec.md new file mode 100644 index 00000000..d1ccfd0e --- /dev/null +++ b/openspec/changes/archive/2026-09-23-vale-swap-backreference/specs/cli-rule-validation/spec.md @@ -0,0 +1,47 @@ +## ADDED Requirements + +### Requirement: The schema may reject a pattern the binary accepts only when the pattern can never fire + +The Vale rule schema SHALL reject a pattern that the vendored binary loads and runs without complaint, where that pattern cannot produce a finding on any document. Such a rejection SHALL be stated per rule in the style layer, and SHALL NOT be raised in the config layer. + +This is a different class from a rejection justified by blast radius. A rule shape that crashes the binary, or a field that draws an `E201`, takes down every other rule's findings for the whole run, and its rejection borrows that justification. A pattern that can never fire takes down nothing: it loads, runs, writes nothing to stderr, and reports nothing — which is exactly what a rule with nothing to say looks like. The author has no signal to act on, at the only moment they are looking, and there is no suppression mechanism anywhere in the CLI for them to reach for if the schema is wrong. + +Two obligations follow from that, and they pull against each other. + +The rejection SHALL be carried by a detector whose precision is established by measurement against the binary, in both directions: every rejected form SHALL have been measured silent, and every form the detector accepts that could plausibly have been rejected SHALL have been measured firing. Precision is load-bearing here in a way it is not for a blast-radius rejection, because an author facing a false positive cannot suppress it and cannot write the rule. + +The rejection's message SHALL carry its own justification rather than pointing at a broken run. It SHALL name the offending key or pattern, SHALL say that the pattern can never match and that the binary reports nothing, and SHALL name the construct to write instead. + +#### Scenario: A backreference in a swap key is rejected + +- **WHEN** a `substitution` rule's `swap` map carries a key containing a backreference outside a character class +- **THEN** `verify` SHALL reject the rule +- **AND** the message SHALL name the offending key +- **AND** the message SHALL say the key can never match and that Vale reports nothing +- **AND** the message SHALL name `existence` as the check to write instead + +#### Scenario: A pattern the binary honors is not rejected + +- **WHEN** a `swap` key contains `\1` inside a character class, where it is an octal escape rather than a backreference +- **THEN** `verify` SHALL accept the rule + +An escaped backslash before a digit, escaped parentheses, and a non-capturing group are accepted for the same reason: each was measured firing. + +#### Scenario: The rejection is scoped to the field that carries the defect + +- **WHEN** the same pattern appears in a field of another check that the binary does honor it in +- **THEN** `verify` SHALL accept the rule + +The defect belongs to how `substitution` compiles its keys, not to the regex engine, which is shared. Rejecting elsewhere would block a rule that works. + +#### Scenario: A widening of the defect fails the vendor contract + +- **WHEN** a Vale upgrade makes a neighbouring pattern field compile its patterns the same way +- **THEN** the vendor contract test SHALL fail +- **AND** the failure SHALL name the field whose treatment changed + +#### Scenario: A rejection the binary stops earning is removed, not kept + +- **WHEN** a Vale upgrade makes a rejected pattern fire +- **THEN** the vendor contract test asserting it silent SHALL fail +- **AND** the schema's rejection SHALL be removed rather than the test relaxed diff --git a/openspec/changes/archive/2026-09-23-vale-swap-backreference/tasks.md b/openspec/changes/archive/2026-09-23-vale-swap-backreference/tasks.md new file mode 100644 index 00000000..ed3099af --- /dev/null +++ b/openspec/changes/archive/2026-09-23-vale-swap-backreference/tasks.md @@ -0,0 +1,40 @@ +## 1. Spec + +- [x] 1.1 Confirm no standing requirement governs a rejection for a pattern the + binary accepts; "Behavior a schema cannot express stays explicit" is + scoped to a shape that crashes the binary. +- [x] 1.2 Add the requirement as an ADDED block, so nothing standing is + restated and nothing can be dropped by archive. +- [x] 1.3 Dry-run `openspec archive` and compare the scenario count in + `cli-rule-validation` before and after. + +## 2. Schema + +- [x] 2.1 Reject a `swap` key carrying a live backreference, in the style + layer, per rule. +- [x] 2.2 Make the detector character-class aware, so `[\1a]` is accepted. +- [x] 2.3 Scope the rejection to `substitution`, since the permissive checks + decode loosely and a `swap` map on one reaches the check. +- [x] 2.4 Write the message so it carries its own justification: the key can + never match, Vale reports nothing, the rule is silently dead — and say to + write an `existence` rule instead. + +## 3. Recipe + +- [x] 3.1 Remove the two-engine fallback claim; Vale compiles with `regexp2` + unconditionally. +- [x] 3.2 Generalise the `swap` constraint to "no capture group survives a swap + key", with all four measured forms. +- [x] 3.3 Document that `$1` in the swap value works, and that `\1` in a + character class works. +- [x] 3.4 Add the leading-lookbehind mirror of the trailing-lookahead limit. +- [x] 3.5 Bump the topic to v14. + +## 4. Tests + +- [x] 4.1 Detector, both directions: the four inert forms rejected, the + false-positive candidates accepted. +- [x] 4.2 Vendor contract: correct the mechanism comment, pin the no-capture- + group forms, the character class, the `$1` value, and the lookbehind. +- [x] 4.3 Vendor contract: measure `consistency` and `conditional` so a + widening of the blast radius is caught rather than assumed. diff --git a/openspec/specs/cli-rule-validation/spec.md b/openspec/specs/cli-rule-validation/spec.md index 30c55463..f1a7cfad 100644 --- a/openspec/specs/cli-rule-validation/spec.md +++ b/openspec/specs/cli-rule-validation/spec.md @@ -318,3 +318,49 @@ The write path SHALL NOT refuse. `check`'s repair path calls `writeRuleFile`, so - **WHEN** a rule is written whose id another engine already holds - **THEN** the rule SHALL be written - **AND** the caller SHALL receive a warning naming both directories + +### Requirement: The schema may reject a pattern the binary accepts only when the pattern can never fire + +The Vale rule schema SHALL reject a pattern that the vendored binary loads and runs without complaint, where that pattern cannot produce a finding on any document. Such a rejection SHALL be stated per rule in the style layer, and SHALL NOT be raised in the config layer. + +This is a different class from a rejection justified by blast radius. A rule shape that crashes the binary, or a field that draws an `E201`, takes down every other rule's findings for the whole run, and its rejection borrows that justification. A pattern that can never fire takes down nothing: it loads, runs, writes nothing to stderr, and reports nothing — which is exactly what a rule with nothing to say looks like. The author has no signal to act on, at the only moment they are looking, and there is no suppression mechanism anywhere in the CLI for them to reach for if the schema is wrong. + +Two obligations follow from that, and they pull against each other. + +The rejection SHALL be carried by a detector whose precision is established by measurement against the binary, in both directions: every rejected form SHALL have been measured silent, and every form the detector accepts that could plausibly have been rejected SHALL have been measured firing. Precision is load-bearing here in a way it is not for a blast-radius rejection, because an author facing a false positive cannot suppress it and cannot write the rule. + +The rejection's message SHALL carry its own justification rather than pointing at a broken run. It SHALL name the offending key or pattern, SHALL say that the pattern can never match and that the binary reports nothing, and SHALL name the construct to write instead. + +#### Scenario: A backreference in a swap key is rejected + +- **WHEN** a `substitution` rule's `swap` map carries a key containing a backreference outside a character class +- **THEN** `verify` SHALL reject the rule +- **AND** the message SHALL name the offending key +- **AND** the message SHALL say the key can never match and that Vale reports nothing +- **AND** the message SHALL name `existence` as the check to write instead + +#### Scenario: A pattern the binary honors is not rejected + +- **WHEN** a `swap` key contains `\1` inside a character class, where it is an octal escape rather than a backreference +- **THEN** `verify` SHALL accept the rule + +An escaped backslash before a digit, escaped parentheses, and a non-capturing group are accepted for the same reason: each was measured firing. + +#### Scenario: The rejection is scoped to the field that carries the defect + +- **WHEN** the same pattern appears in a field of another check that the binary does honor it in +- **THEN** `verify` SHALL accept the rule + +The defect belongs to how `substitution` compiles its keys, not to the regex engine, which is shared. Rejecting elsewhere would block a rule that works. + +#### Scenario: A widening of the defect fails the vendor contract + +- **WHEN** a Vale upgrade makes a neighbouring pattern field compile its patterns the same way +- **THEN** the vendor contract test SHALL fail +- **AND** the failure SHALL name the field whose treatment changed + +#### Scenario: A rejection the binary stops earning is removed, not kept + +- **WHEN** a Vale upgrade makes a rejected pattern fire +- **THEN** the vendor contract test asserting it silent SHALL fail +- **AND** the schema's rejection SHALL be removed rather than the test relaxed diff --git a/packages/cli/src/agent/create-vale-rule.md b/packages/cli/src/agent/create-vale-rule.md index 43c7b434..44e5b316 100644 --- a/packages/cli/src/agent/create-vale-rule.md +++ b/packages/cli/src/agent/create-vale-rule.md @@ -1,4 +1,4 @@ -# Topic: create-vale-rule (CLI v%(CLI_VERSION)s / topic v13) +# Topic: create-vale-rule (CLI v%(CLI_VERSION)s / topic v14) ## You are here This is `create-vale-rule`. It helps you write a Vale rule: a check over @@ -478,28 +478,52 @@ it. - `(?:…)`, `[…]`, `|`, `+`, `?` all work. - **Lookaround and backreferences work, and they are not free.** - Vale compiles a pattern with Go's own `regexp` first and falls - back to `regexp2` when that engine refuses it, so `(?=…)`, - `(?<=…)` and `\1` are all available even though Go's `regexp` has - none of them. Measured on Vale v%(VALE_VERSION)s with throwaway - rules, each firing on its `fail/` fixture and quiet on `pass/`: a - repeated-word `\b(\w+) \1\b`, a `foo(?= bar)` lookahead, and a - `(?<=x )y` lookbehind. The fallback engine backtracks and is the - slower of the two, so keep lookaround off a pattern that runs - over every file in the project, and prove any rule that uses one - with a fixture rather than trusting the syntax. "X but not when - followed by Y" is therefore writable as a single `substitution`, - but splitting it or narrowing with `scope` is still the cheaper - rule when either will do. - - Two measured limits sit on top of that, and both are silent: - - - **A backreference does nothing in `swap`.** The same - `(\w+) \1` that fires under `tokens` and under `raw` produces - no finding as a `swap` key, with nothing on stderr and the - rule loading cleanly, so the rule looks healthy and never - fires at all. A repeated-word check has to be an `existence` - rule; it cannot be a `substitution`. + Vale compiles every pattern with `regexp2` in RE2 compatibility + mode, so `(?=…)`, `(?<=…)` and `\1` are all available even + though Go's own `regexp` has none of them. (Older versions of + this topic said Vale tried Go's `regexp` first and fell back. + There is no fallback and never was; the conclusion was right and + the reason was not.) Measured on Vale v%(VALE_VERSION)s with + throwaway rules, each firing on its `fail/` fixture and quiet on + `pass/`: a repeated-word `\b(\w+) \1\b`, a `foo(?= bar)` + lookahead, and a `(?<=x )y` lookbehind. That engine backtracks + and is slower than a pattern without lookaround, so keep + lookaround off a rule that runs over every file in the project, + and prove any rule that uses one with a fixture rather than + trusting the syntax. "X but not when followed by Y" is therefore + writable as a single `substitution`, but splitting it or + narrowing with `scope` is still the cheaper rule when either + will do. + + Three measured limits sit on top of that, and all three are + silent: + + - **No capture group survives a `swap` key**, so a + backreference in one can never match. Vale compiles all of a + rule's swap keys into ONE alternation and wraps each key in a + capture group of its own, because the number of the group + that matched is how it knows which replacement to offer. Those + wrappers have to be numbered 1, 2, 3…, so any group you wrote + is rewritten to `(?:…)` first. Whichever way round, `\1` ends + up pointing at Vale's wrapper (the group still being matched) + and matches nothing. Measured silent, with nothing on + stderr and the rule loading cleanly: `(\w+) \1`, `(the) \1`, + `the \1` and `(?:\w+) \1`. **`%(TASKLESS_CLI)s verify` rejects a + `swap` key with a backreference**, because a rule that loads + and never fires is indistinguishable from a clean project. A + repeated-word check has to be an `existence` rule, where the + same pattern works under both `tokens` and `raw`. + + Two things this does NOT mean: + + - **`$1` in the swap VALUE works.** Vale expands it against + the original key, before the rewrite, so + `colour(s?): color$1` offers `colors` for `colours` and + `color` for `colour`. Only the key loses its groups. + - **`\1` inside a character class is fine**, because there it + is an octal escape rather than a backreference. `[\1a]bc` + matches `abc` and `verify` accepts it. + - **A trailing lookahead in `tokens` or `swap` has to peek at a non-word character.** The implicit `\b` is appended after the lookahead (the lookahead is zero-width, so the position is @@ -507,6 +531,12 @@ it. the match and the text peeked at. `foo(?= bar)` fires; `foo(?=bar)` can never match, whatever the document says. Use `raw` when the lookahead has to land on a word character. + - **A leading lookbehind has the same problem at the other + end.** The implicit `\b` goes on before it, so the boundary + is tested between what the lookbehind peeked at and the match. + `(?<=x )y` fires; `(?<=x)foo` is silent under `tokens` and + under `swap` on a document reading `xfoo`, and fires under + `raw`. Same fix: use `raw`. - **Word boundaries are applied for you, around the whole pattern.** Measured: `Github` does not fire inside `GithubToken`, and the multi-word `click here` does not fire inside `Clicking here`. diff --git a/packages/cli/src/schemas/vale-rule.ts b/packages/cli/src/schemas/vale-rule.ts index a9e66f01..44bdfbc2 100644 --- a/packages/cli/src/schemas/vale-rule.ts +++ b/packages/cli/src/schemas/vale-rule.ts @@ -617,6 +617,137 @@ function fatalShapeMessages( return fatal; } +/** + * A backreference in a `swap` key, which can never match. + * + * The first hard error in this layer for a pattern-quality problem the binary + * itself accepts. Every other rejection here is justified by whole-run blast + * radius — an `E201` suppresses every rule's findings, a panic ends the run — + * and their messages say so. This one cannot borrow that argument: Vale loads + * the rule, runs it, writes nothing to stderr, and reports nothing. The rule is + * silently dead, and a dead rule is indistinguishable from a clean project, so + * the author learns nothing at the only moment they are looking. + * + * ## Why no capture group survives a swap key + * + * Measured against Vale {@link PINNED_VALE_VERSION} and read off + * `internal/check/substitution.go` (`NewSubstitution`). Vale compiles all of a + * rule's swap keys into **one** alternation, wrapping each key in a capture + * group of its own so that the index of the group that matched says which key + * it was, and therefore which replacement to offer: + * + * ```go + * tokens += `(` + regexstr + `)|` + * ``` + * + * Those wrapper groups have to be numbered 1..n for that lookup to work, so + * any group the author wrote would shift the numbering. Vale removes them: + * + * ```go + * convertCaptureGroups(regexstr) // `(? `(?:` + * ``` + * + * The two halves together mean a `\1` in a swap key never refers to anything + * the author wrote, in either branch of Vale's own guard: + * + * - The key **has** a capturing group — `(\w+) \1` — so the rewrite runs and + * the group becomes `(?:\w+)`. `\1` is now Vale's wrapper. + * - The key has **no** capturing group — `the \1`, `(?:\w+) \1` — so the + * rewrite is skipped (the guard counts `(` against `(?` and `\(`) and `\1` + * is Vale's wrapper directly. + * + * Either way `\1` is a self-reference to the group still being matched, which + * is empty at that point and can never hold the text the author meant. Both + * forms were measured silent; the same patterns under `tokens` and under `raw` + * fire, which is what makes this a `swap`-only defect rather than a claim about + * Vale's regex engine. Vale compiles every pattern with `regexp2` in RE2 mode + * unconditionally (`internal/regex/regex.go:63`), so backreferences are + * available everywhere else. + * + * ## Why detection can be this literal + * + * The rewrite only ever replaces `(`, so it can neither create nor destroy a + * `\`. "Apply `convertCaptureGroups`, then look for a surviving + * backreference" therefore collapses to a single left-to-right scan for an + * unescaped `\<1-9>`, with no rewrite to perform. + * + * The scan tracks character classes because that is a measured difference, not + * defensive programming. Inside `[…]` a `\1` is an **octal** escape, not a + * backreference: `[\1a]bc` matches `abc` and was measured firing. A detector + * that skipped the class bookkeeping would reject a working rule, and there is + * no suppression mechanism anywhere in the CLI for the author to reach for. + * + * Also measured as accepted, and each covered by a test: `a\\1b` (an escaped + * backslash, then a literal `1`), `q\(1\)z` (escaped parentheses), and + * `(?:bull|ox)-like` (a non-capturing group and no backreference at all). + */ +function hasLiveBackreference(pattern: string): boolean { + let inClass = false; + for (let index = 0; index < pattern.length; index += 1) { + const character = pattern[index]; + if (character === "\\") { + const next = pattern[index + 1]; + if (!inClass && next !== undefined && next >= "1" && next <= "9") { + return true; + } + // Skip the escaped character, so `\\1` reads as a backslash then a `1`. + index += 1; + continue; + } + if (inClass) { + if (character === "]") { + inClass = false; + } + continue; + } + if (character === "[") { + inClass = true; + } + } + return false; +} + +function swapBackreferenceMessages( + rule: Record +): { path: PropertyKey[]; message: string }[] { + const { swap } = rule; + // Scoped to `substitution` deliberately, and the guard is not redundant with + // the field tables. `swap` is `substitution`'s field alone, so the strict + // union already rejects it on the other ten checks with an `E201` message — + // but `consistency` and `spelling` decode loosely (see `permissiveCheck`), + // so a `swap` map on either reaches here. Only `NewSubstitution` builds the + // alternation described above, so only there is the diagnosis below the true + // one, and claiming it elsewhere would be the too-strict direction on a + // guess. + if ( + rule.extends !== "substitution" || + typeof swap !== "object" || + swap === null || + Array.isArray(swap) + ) { + return []; + } + return Object.keys(swap) + .filter((key) => hasLiveBackreference(key)) + .map((key) => ({ + path: ["swap", key], + message: + `swap key ${JSON.stringify(key)} uses a backreference, and it can ` + + `never match. Vale compiles a rule's swap keys into one alternation, ` + + `wrapping each key in a capture group of its own and rewriting every ` + + `group you wrote to a non-capturing one, so ` + + String.raw`'\1'` + + ` refers to Vale's ` + + `wrapper rather than to anything in your pattern. Nothing is ` + + `reported: Vale ${PINNED_VALE_VERSION} loads the rule, writes nothing ` + + `to stderr, and the rule never fires on any document — so a silently ` + + `dead rule reads exactly like a clean project. Write it as an ` + + `'existence' rule instead, where a backreference works under both ` + + `'tokens' and 'raw'. ('$1' in the swap VALUE is a different thing and ` + + `does work; only the key is affected.)`, + })); +} + /** * The action names Vale 3.21.0 accepts at load. * @@ -791,6 +922,7 @@ const valeBodySchema = z const rule = context.value as Record; for (const { path, message } of [ ...fatalShapeMessages(rule), + ...swapBackreferenceMessages(rule), ...actionMessages(rule), ]) { context.issues.push({ code: "custom", input: rule, path, message }); diff --git a/packages/cli/test/vale-schema-contract.test.ts b/packages/cli/test/vale-schema-contract.test.ts index 22780317..bfe022f8 100644 --- a/packages/cli/test/vale-schema-contract.test.ts +++ b/packages/cli/test/vale-schema-contract.test.ts @@ -431,6 +431,153 @@ describe("advisories are said, not refused", () => { }); }); +/** + * The `swap` backreference rejection, both directions. + * + * The first hard error this layer raises for a pattern-quality problem Vale + * itself accepts, which makes the detector's precision load-bearing: there is + * no suppression mechanism anywhere in the CLI, so a false positive leaves an + * author with nothing to reach for. Every accepted case below was measured + * firing against the vendored binary, and the behaviour the rejection describes + * is pinned in `vale-vendor-contract.test.ts` ("a backreference is silently + * inert in a `swap` key"); this asks only that the schema says so, and only + * where it applies. + */ +const swapRule = (key: string): string => + `extends: substitution\nmessage: "x %s"\nlevel: error\nswap:\n '${key}': X\n`; + +const swapVerdict = (key: string) => + validateValeRule("demo", parseYaml(swapRule(key))); + +describe("a backreference in a swap key is rejected", () => { + // Every one of these was measured silent against Vale: the rule loads, Vale + // writes nothing to stderr, and no document ever produces a finding. + const inert = [ + // The capturing form. `convertCaptureGroups` rewrites `(\w+)` to + // `(?:\w+)`, so `\1` lands on Vale's own wrapper group. + String.raw`(\w+) \1`, + String.raw`(the) \1`, + // The two forms with no capture group at all, where Vale's rewrite is + // skipped and `\1` is the wrapper directly. Variant B of the detector -- + // "flag a backreference only when the key has a group to convert" -- would + // have missed both, which is why the detector does not look for the group. + String.raw`the \1`, + String.raw`(?:\w+) \1`, + ]; + + for (const key of inert) { + it(`rejects ${key}`, () => { + const { valid, errors } = swapVerdict(key); + expect(valid).toBe(false); + expect(errors).toHaveLength(1); + expect(errors[0]).toContain(JSON.stringify(key)); + // The message has to carry its own justification, because unlike every + // other rejection in this layer it cannot point at a broken run. + expect(errors[0]).toContain("never match"); + expect(errors[0]).toContain("existence"); + }); + } + + // Each of these was measured FIRING against the binary, so rejecting one + // would block a rule that works. + const accepted = [ + // Inside a character class `\1` is an octal escape, not a backreference. + // `[\1a]bc` was measured matching `abc`. + String.raw`[\1a]bc`, + // An escaped backslash, then a literal `1`. Measured matching `a\1b`. + String.raw`a\\1b`, + // Escaped parentheses: literal text, no group and no reference. Measured + // matching `q(1)z`. + String.raw`q\(1\)z`, + // A non-capturing group and no backreference at all. Measured matching + // `ox-like`. + String.raw`(?:bull|ox)-like`, + // The plain case, for contrast. + "utilize", + ]; + + for (const key of accepted) { + it(`accepts ${key}`, () => { + const { valid, errors } = swapVerdict(key); + expect(errors).toEqual([]); + expect(valid).toBe(true); + }); + } + + it("names every offending key when a rule has more than one", () => { + const { valid, errors } = validateValeRule( + "demo", + parseYaml( + 'extends: substitution\nmessage: "x %s"\nlevel: error\nswap:\n' + + " '(\\w+) \\1': X\n 'the \\2': Y\n utilize: use\n" + ) + ); + expect(valid).toBe(false); + expect(errors).toHaveLength(2); + }); + + it("leaves a swap on another check to the field table", () => { + // `swap` is `substitution`'s field alone, so the union rejects it on + // `existence` first and this check never adds a second sentence about a + // key that was never going to be read. + const { valid, errors } = validateValeRule( + "demo", + parseYaml( + 'extends: existence\nmessage: "x"\nlevel: error\nswap:\n' + + " '(\\w+) \\1': X\n" + ) + ); + expect(valid).toBe(false); + expect(errors).toHaveLength(1); + expect(errors[0]).toContain("not a field of the existence check"); + }); + + it("is silent on a swap map under a permissive check", () => { + // `consistency` decodes loosely, so a `swap` map on it reaches this check + // rather than being stopped by the field table. Vale ignores the field + // there, and the alternation this rejection describes is not built, so + // there is nothing true to say -- and saying it anyway would reject a + // rule the binary runs. + const { valid, errors } = validateValeRule( + "demo", + parseYaml( + 'extends: consistency\nmessage: "x"\nlevel: error\nswap:\n' + + " '(\\w+) \\1': X\n" + ) + ); + expect(errors).toEqual([]); + expect(valid).toBe(true); + }); + + it("reads the key case-insensitively, as Vale decodes it", () => { + const { valid, errors } = validateValeRule( + "demo", + parseYaml( + 'extends: substitution\nmessage: "x %s"\nlevel: error\nSwap:\n' + + " '(\\w+) \\1': X\n" + ) + ); + expect(valid).toBe(false); + expect(errors).toHaveLength(1); + }); + + it("says that `$1` in the swap VALUE is a different thing and works", () => { + // Measured: `colour(s?)` -> `color$1` offers `colors` on `colours`. Vale + // re-applies the ORIGINAL, unconverted key to the observed text to expand + // `$1` (`subMsg` in `internal/check/substitution.go`), which is why the + // value is untouched by the rewrite that breaks the key. + const { valid, errors } = validateValeRule( + "demo", + parseYaml( + 'extends: substitution\nmessage: "x %s"\nlevel: error\nswap:\n' + + " 'colour(s?)': 'color$1'\n" + ) + ); + expect(errors).toEqual([]); + expect(valid).toBe(true); + }); +}); + // --- Helpers ----------------------------------------------------------------- /** diff --git a/packages/cli/test/vale-vendor-contract.test.ts b/packages/cli/test/vale-vendor-contract.test.ts index bf727494..7f98862a 100644 --- a/packages/cli/test/vale-vendor-contract.test.ts +++ b/packages/cli/test/vale-vendor-contract.test.ts @@ -179,6 +179,14 @@ const existenceOver = ( const swapPatternRule = (pattern: string): string => `extends: substitution\nmessage: "%s -> %s"\nlevel: warning\nswap:\n '${pattern}': REPLACED\n`; +/** A `consistency` rule whose single `either` key is the pattern under test. */ +const consistencyEither = (key: string): string => + `extends: consistency\nmessage: "%s"\nlevel: warning\nnonword: false\neither:\n '${key}': 'zzzz'\n`; + +/** A `conditional` rule whose `first` is the pattern under test. */ +const conditionalFirst = (first: string): string => + `extends: conditional\nmessage: "%s"\nlevel: warning\nfirst: '${first}'\nsecond: 'zzzz'\n`; + /** A word budget of 8 over whatever `scope` names. */ const budget = (scope: string) => `extends: metric\nmessage: "%s words"\nlevel: error\nscope: '${scope}'\nformula: words\ncondition: "> 8"\n`; @@ -752,14 +760,18 @@ withVale("Vale vendor contract", () => { }); describe("lookaround and backreferences compile", () => { - // Vale tries Go's own `regexp` first and falls back to `regexp2` when a - // pattern will not compile, so constructs Go's engine has never had are - // still available. The recipe said the opposite from topic v1 (2026-08-13, - // the commit that introduced the claim) through v12, corrected here at v13 - // (taskless/cli#371), and sent authors off to split a rule that one - // pattern expresses, so the correction is pinned against the binary rather - // than restated in prose: if a future Vale drops the fallback, these go - // red and the recipe's step 3 is wrong again. + // Vale compiles EVERY pattern with `regexp2` in RE2 compatibility mode, + // unconditionally -- `regexp2.Compile(expr, regexp2.RE2)`, the whole body + // of `Compile` in `internal/regex/regex.go:63`. There is no Go-`regexp` + // path and nothing falls back to anything, so constructs Go's own engine + // has never had are simply available. The recipe said the opposite from + // topic v1 (2026-08-13, the commit that introduced the claim) through v12, + // and sent authors off to split a rule that one pattern expresses. v13 + // (taskless/cli#371) fixed the user-facing conclusion but explained it + // with a two-engine fallback that does not exist; v14 (taskless/cli#392) + // corrects the explanation. The conclusion is pinned against the binary + // rather than restated in prose: if a future Vale narrows the engine, + // these go red and the recipe's step 3 is wrong again. // // The recipe scopes the claim to `tokens` and `swap`, so those are // measured here too rather than inferred from `raw`. They do not behave @@ -835,13 +847,30 @@ withVale("Vale vendor contract", () => { expect(lines(behind, "Here is z y now.\n").lines).toEqual([]); }); - // ...but a backreference is SILENTLY inert in `swap`, where the identical - // pattern fires under both `tokens` and `raw`. Nothing is written to - // stderr and the rule loads, so a `swap` rule built on `\1` looks healthy - // and never fires at all. The literal control pins that the rule shape and - // the document are otherwise fine, so the silence is the backreference and - // not the scaffolding. This is why the recipe's step 3 can no longer say - // "backreferences work" for `tokens` and `swap` in one breath. + // ...but a backreference is SILENTLY inert in a `swap` key, where the + // identical pattern fires under both `tokens` and `raw`. Nothing is + // written to stderr and the rule loads, so a `swap` rule built on `\1` + // looks healthy and never fires at all. + // + // The cause is not the regex engine, which is the same `regexp2` here as + // everywhere else. It is that NO CAPTURE GROUP SURVIVES A SWAP KEY. + // `NewSubstitution` (`internal/check/substitution.go`) compiles all of a + // rule's keys into one alternation, wrapping each key in a group of its + // own so the index of the group that matched says which replacement to + // offer (`tokens += "(" + regexstr + ")|"`). Those wrappers must be + // numbered 1..n, so any group the author wrote is rewritten away first by + // `convertCaptureGroups`, whose pattern is `(? `(?:`. + // + // That is why the four cases below all behave the same despite differing + // in whether there is a group to convert: Vale's guard skips the rewrite + // when the key has no capturing `(`, and then `\1` is the wrapper + // directly. Either way `\1` is a self-reference to the group still being + // matched. + // + // Depended on by: the `swap` backreference REJECTION in + // `src/schemas/vale-rule.ts` (and its tests in + // `vale-schema-contract.test.ts`), which is a hard error. If Vale ever + // makes these work, this test goes red and that error must come out. it("silently ignores a backreference in a `swap` key", () => { const repeated = "A the the repeated word.\n"; @@ -863,6 +892,124 @@ withVale("Vale vendor contract", () => { ).toEqual(["the the"]); } }); + + // The half a "does the key have a capture group?" detector would miss. + // Neither of these has one, so Vale's rewrite never runs -- and both are + // just as inert, because the number they reference belongs to Vale's own + // wrapper. The schema's detector therefore looks for the backreference and + // never for the group. + it("ignores a `swap` backreference with no capture group to convert", () => { + const repeated = "A the the repeated word.\n"; + for (const pattern of [String.raw`the \1`, String.raw`(?:\w+) \1`]) { + const swapped = lines(swapPatternRule(pattern), repeated); + expect(swapped.lines, `swap key ${pattern}`).toEqual([]); + expect(swapped.stderr, `swap key ${pattern}`).toBe(""); + } + }); + + // Inside a character class `\1` is an OCTAL escape, not a backreference, + // so the key works and the schema must not reject it. This is the one + // case that makes the detector's character-class bookkeeping mandatory + // rather than polish: there is no suppression mechanism in the CLI, so a + // false positive here leaves an author with no recourse. + it("honors `\\1` inside a character class, where it is an octal escape", () => { + expect( + lines(swapPatternRule(String.raw`[\1a]bc`), "Here is abc now.\n") + .messages + ).toEqual(["REPLACED -> abc"]); + }); + + // The other forms the detector must not mistake for a backreference, each + // measured firing: an escaped backslash then a literal digit, and escaped + // parentheses. + it("honors keys that only look like backreferences", () => { + expect( + lines(swapPatternRule(String.raw`a\\1b`), "here a\\1b now.\n").messages + ).toEqual([String.raw`REPLACED -> a\1b`]); + expect( + lines(swapPatternRule(String.raw`q\(1\)z`), "here q(1)z now.\n") + .messages + ).toEqual(["REPLACED -> q(1)z"]); + }); + + // The asymmetry worth teaching beside the rejection: `$1` in the swap + // VALUE does work, because `subMsg` re-applies the ORIGINAL, unconverted + // key to the observed text to expand it. Only the key is affected by the + // rewrite, so "capture groups are useless in a substitution rule" would be + // the wrong lesson to take from the rejection. + it("expands `$1` in a `swap` value from the unconverted key", () => { + const rule = + 'extends: substitution\nmessage: "%s"\nlevel: warning\nswap:\n' + + " 'colour(s?)': 'color$1'\n"; + expect(lines(rule, "Many colours here.\n").messages).toEqual(["colors"]); + expect(lines(rule, "One colour here.\n").messages).toEqual(["color"]); + }); + + // The mirror of the trailing-lookahead case above, and the recipe + // documented only one direction until v14. A `tokens` entry is wrapped in + // `\b…\b`, and the LEADING `\b` lands BEFORE a leading lookbehind, so the + // boundary is tested between whatever the lookbehind peeked at and the + // match. With a word character there it is not a boundary and the pattern + // cannot match at all. `swap` wraps the same way and behaves the same; + // `raw` is verbatim and fires. + it("wraps a leading lookbehind in the implicit word boundaries", () => { + const glued = String.raw`(?<=x)foo`; + const document = "We wrote xfoo here.\n"; + expect( + lines(existenceOver("tokens", `'${glued}'`), document).lines + ).toEqual([]); + expect(lines(swapPatternRule(glued), document).lines).toEqual([]); + expect( + lines(existenceOver("raw", `'${glued}'`), document).messages + ).toEqual(["foo"]); + }); + + // The blast radius, measured rather than assumed. `swap` is the only + // pattern field with this defect: `consistency` wraps each `either` key in + // a NAMED group (`(?P…)`, `internal/check/consistency.go`) and + // regexp2 numbers named groups after unnamed ones, so the author's `\1` + // still points at the author's group; `conditional` compiles `first` and + // `second` directly with no wrapping at all. If a future Vale widens + // `convertCaptureGroups` to either, these go red and the schema's + // rejection has to widen with it. + // + // Each direction is asserted, because a pattern that fails to compile and + // a pattern that compiled and did not match are indistinguishable from a + // finding count alone. + it("honors a backreference in a `consistency` key", () => { + const both = "A the the repeat and a zzzz here.\n"; + const neither = "A the cat sat and a zzzz here.\n"; + + // The literal control establishes what "working" looks like: both forms + // present flags the minority spelling, one form alone says nothing. + expect(lines(consistencyEither("the the"), both).messages).toEqual([ + "zzzz", + ]); + expect(lines(consistencyEither("the the"), neither).lines).toEqual([]); + + // The backreference tracks it exactly, so it matched. + const backref = consistencyEither(String.raw`(\w+) \1`); + expect(lines(backref, both).messages).toEqual(["zzzz"]); + expect(lines(backref, neither).lines).toEqual([]); + }); + + it("honors a backreference in a `conditional` first", () => { + const repeated = "A the the repeated word.\n"; + const satisfied = "A the the repeated word and zzzz.\n"; + + expect(lines(conditionalFirst("the the"), repeated).messages).toEqual([ + "the the", + ]); + for (const first of [String.raw`(\w+) \1`, String.raw`(the) \1`]) { + expect( + lines(conditionalFirst(first), repeated).messages, + first + ).toEqual(["the the"]); + expect(lines(conditionalFirst(first), satisfied).lines, first).toEqual( + [] + ); + } + }); }); describe("`nonword` governs `tokens`, not `raw`", () => {