Skip to content

fix: correct the Vale regex-engine claim, the fixture-bucket advice, and the .taskless/** advisory - #384

Merged
thecodedrift merged 4 commits into
mainfrom
docs/create-vale-rule-regex-and-fixtures
Sep 22, 2026
Merged

thecodedrift merged 4 commits into
mainfrom
docs/create-vale-rule-regex-and-fixtures

Conversation

@thecodedrift

@thecodedrift thecodedrift commented Sep 22, 2026

Copy link
Copy Markdown
Member

Four corrections, all to the same claim family: what Vale's regex engine can do, and where check does and does not look. One PR because they edit the same recipe, plus the spec that described the same behaviour.

The regex engine (#371)

Step 3 said tokens and swap "compile as Go RE2" and that "lookahead and lookbehind do not exist in RE2", telling an author to split a rule that one pattern expresses. Vale compiles with Go's regexp first and falls back to regexp2 when a pattern will not compile, so lookaround and backreferences are available.

Measured on the vendored binary, Vale 3.22.0, each firing on a fail/ fixture and quiet on pass/:

construct pattern result
backreference \b(\w+) \1\b ok: true
lookahead foo(?= bar) ok: true
lookbehind (?<=x )y ok: true

A negative control (the same backreference rule against a fixture with no repeated word) reported fail fixture did not fire, so the three passes are not a test that cannot fail.

The word-boundary and hyphen notes that shared that bullet list were re-measured before being kept: Github stays quiet inside GithubToken, click here inside Clicking here, and obviously fires inside obviously-named while obviously_stale is safe.

The step now says: Go regexp first, regexp2 fallback, lookaround and backreferences work but backtrack and are slower, so keep them off hot paths and prove them with a fixture.

The claim was scoped to tokens and swap, and only raw was measured

Review caught that the three measurements above all build a raw: list, while the step scopes its claim to tokens and swap — different keys, because those two get an implicit \b…\b wrapping that raw does not. Measuring the other two found the claim is not uniformly true:

construct raw tokens swap
lookahead foo(?= bar) fires fires fires
lookbehind (?<=x )y fires fires fires
backreference (\w+) \1 fires fires silent
lookahead foo(?=bar) fires silent silent

Two silent divergences, both now documented and pinned:

  • A backreference does nothing as a swap key. The identical pattern fires under tokens and raw. Nothing reaches stderr and the rule loads, so a swap rule built on \1 looks healthy and never fires at all. A literal swap key over the same document fires, which isolates the cause to the backreference rather than the scaffolding. A repeated-word check has to be an existence rule.
  • A trailing lookahead in tokens/swap must peek at a non-word character. The implicit \b is appended after the lookahead — the lookahead is zero-width, so the position is still where the match ended — putting the boundary between the match and the text peeked at. foo(?= bar) fires; foo(?=bar) can never match there, whatever the document says. raw is inserted verbatim and has no such limit.

Step 3 now carries both as caveats rather than a flat "they work".

The fixture bucket (#370)

The issue reported that check .taskless/rules/vale/<id>/.tests/fail --json can never return a finding, because check excludes .taskless/ unconditionally. It is not unconditional. run.ts applies the .taskless/** exclusion only on a whole-project walk, on the reasoning that an explicit path is a request; that has been true since August.

Measured on this branch:

  • check .taskless/rules/vale/no-em-dashes/.tests/fail --json returned three findings with the message text rendered. Same for no-hedging, three findings.
  • A whole-project check --json returned zero results under .taskless/.

The symptom in the issue reproduces from a different cause. Adding a [.taskless/**] matcher setting the rule to NO left test at ok: true and check on the same bucket at results: []. That is the exact pairing the issue describes.

So the recipe keeps the command and explains it instead of dropping it: which exclusion applies where, that the rendered message is the thing test cannot show you, and that an empty bucket means the rule's own matcher rather than the .vale.ini being broken. The troubleshooting list now opens with "read the finding first" rather than sending the author to the config.

The CLI half of #370 (test --verbose, or --include-fixtures on check) is deliberately not implemented here. It is a separate decision, and with the command above working it is a convenience rather than the only route.

The advisory said the same wrong thing

Found while writing the above: verify's own advisory carried the imprecision this PR removes from the recipe, and described the matcher as costing nothing.

Before:

no-simply/.vale.ini line 5: matcher [.taskless/**] is unnecessary: check excludes .taskless/ before Vale runs, so it acts only under a bare vale invocation.

After:

no-simply/.vale.ini line 5: matcher [.taskless/**] is unnecessary on a whole-project check, which excludes .taskless/ before Vale runs, and it silences the rule on a path you name, such as its own fixture bucket.

An author who followed the old text would read "acts only under a bare vale invocation" as permission to leave the matcher in place, and then be told their working check command could not work. It is still an advisory, since the matcher has a legitimate reading (keeping a bare vale quiet over fixtures that hold violations on purpose); it is no longer described as harmless. The docblock above adviseValeRuleConfig records why that changed. agent update repeated the claim in its 0.11.x ledger entry and is corrected to match, topic v9 to v10.

The spec said it too

cli-vale-rule-engine still carried the phrasing this PR removes from the recipe and the advisory: a [.taskless/**] matcher "acts only under a bare vale invocation", with a scenario requiring verify to "report that check already excludes that tree". The spec is the source of truth for this capability, so leaving it behind is how the next author reproduces #370 from the spec instead of the recipe.

The OpenSpec change vale-taskless-matcher-spec restates that requirement so both halves are normative — unnecessary on a whole-project check, AND silencing on a path named explicitly — and adds the check-notices assertion the other advisory scenarios already carry. No code, no behaviour change; the spec catching up to what shipped.

The MODIFIED block restates the requirement in full, since openspec archive replaces rather than patches. Verified by archiving and diffing: 37 scenarios before, 37 after, with the scenario titles identical and only the two intended lines changed. This PR is the tip, so the change is archived here.

Tests

  • vale-vendor-contract.test.ts gains six cases measuring lookaround and backreferences against the vendored binary, each asserted in both directions so a pattern that fails to compile cannot pass as a pattern that did not match. Three cover raw; three cover the tokens boundary interaction, lookaround under a swap key, and the backreference that swap silently ignores.
  • recipe-cross-references.test.ts pins the corrected recipe prose, including the two notes that survived the rewrite.
  • vale-config-schema.test.ts pinned the advisory verbatim and is updated to the new text.

pnpm typecheck, pnpm lint and pnpm --filter @taskless/cli test (1655 tests) all pass. The new advisory was also confirmed live, by reproducing the [.taskless/**] matcher on a throwaway rule and reading it back out of check --json.

Changeset

One file, patch, grown rather than duplicated. The recipes are embedded into the published bundle at build time and the advisory is user-facing output, so a consumer upgrading gets different text in both; pre-1.0, a change to shipped surface is patch.

Fixes #371
Fixes #370

@thecodedrift thecodedrift changed the title docs: correct the Vale recipe regex-engine and fixture-bucket claims (topic v13) fix: correct the Vale regex-engine claim, the fixture-bucket advice, and the .taskless/** advisory Sep 22, 2026
@thecodedrift

Copy link
Copy Markdown
Member Author

@claude /review

@claude

claude Bot commented Sep 22, 2026

Copy link
Copy Markdown

Claude finished @thecodedrift's task in 3m 50s —— View job


Review complete

  • Read .prior-review.json — empty (reviewThreads, reviews, and comments all contain nothing except the trigger comment itself)
  • Fetched PR diff, metadata, and changed files
  • Reviewed all 7 changed files for correctness, removed-behavior gaps, cross-file consistency, and reuse/simplification
  • Verified candidate findings directly against file contents before posting
  • Posted inline comments and this summary

Review mode: incremental — read 0 prior review thread(s) before reviewing. The prior-review snapshot contains no reviews, no review threads, and no comments beyond @claude /review itself, so this is the first substantive review of this PR — everything below is assessed fresh, nothing is being repeated or skipped as already-addressed.

Verified

  • The core claims all check out: run.ts's .taskless/** exclusion is gated on a whole-project walk (an explicit path bypasses it), the corrected advisory string in vale-config.ts matches its test verbatim, and the three new vendor-contract cases (backreference, lookahead, lookbehind) correctly assert both a firing and a non-firing fixture, so a pattern that fails to compile can't pass as a pattern that didn't match.
  • The claim that "the recipe's split-the-rule advice is lost" is false — it's explicitly retained (create-vale-rule.md:490-493) alongside the new performance guidance about regexp2 backtracking.
  • No hardcoded copies of the old advisory string remain anywhere outside this diff's own changes (checked inspect.ts, assemble.ts, verify.ts, both vale/verify.ts files).

New findings

  1. [New] The corrected .taskless/** claim survives, unfixed, in the live OpenSpec spec. openspec/specs/cli-vale-rule-engine/spec.md:236 still reads "declares a matcher under .taskless/\*\* (check excludes that tree before Vale runs, so the matcher acts only under a bare vale invocation)" — the exact imprecise phrasing this PR removes from the recipe and the advisory. The scenario at spec.md:288-291 ("verify SHALL accept the rule and report that check already excludes that tree") has the same gap: it doesn't distinguish whole-project vs. named-path behavior. This PR carries no spec delta, so the standing requirement — the actual source of truth for this capability — now disagrees with the recipe and the advisory it ships alongside. Not necessarily a blocker for this PR's stated scope, but worth a follow-up before someone reads the spec instead of the recipe and reproduces create-vale-rule: the recommended check <rule>/.tests/fail --json can never return a finding #370 from it. (Not left as an inline comment since the file isn't part of this diff and GitHub won't attach a line comment to an unchanged file.)
  2. [New] Untested claim scope — inline comment on create-vale-rule.md:476-487: the lookaround/backreference claim is explicitly scoped to tokens/swap, but both the prose's "Measured" evidence and the new vendor-contract tests only exercise raw, which (per this same file) doesn't get the automatic \b…\b wrapping that tokens does.
  3. [New] Minor duplication — inline comment on vale-vendor-contract.test.ts:92: rawPatternRule duplicates the existing existenceOver("raw", …) helper defined ~80 lines later.
  4. [New] Minor wording nit — inline comment on .changeset/vale-recipe-regex-and-fixtures.md:5: "matcher rather than the config" reads as a false contrast, since the matcher is itself config.
  5. [New, low confidence, not actionable] Two other locations still use the retired "Go RE2" framing this PR corrects elsewhere — packages/cli/src/rules/inspect.ts (~line 236, "Go RE2 requires a group name to be word characters only") and packages/cli/test/verify-test-commands.test.ts (~line 306, same phrasing). Both are about a narrower, still-accurate claim (named-capture-group syntax), not the lookaround-availability claim this PR fixes, and neither file is part of this diff, so this is a heads-up rather than a request to touch them here.
  6. [New, unverifiable] The new test comment at vale-vendor-contract.test.ts:753 states the recipe "said the opposite for twelve topic revisions" citing create-vale-rule: patterns are not RE2-only; backreferences and lookaround work on the vendored Vale #371. I have no access to per-file history in this review (git is unavailable here), so I can't confirm that count — worth the author double-checking it independently, since it'll read as a documented fact to future readers.

No correctness bugs found in the executable code paths (vale-config.ts, run.ts interaction, the test assertions themselves). All findings above are documentation-precision or minor-cleanup items.

Comment thread packages/cli/src/agent/create-vale-rule.md
Comment thread packages/cli/test/vale-vendor-contract.test.ts Outdated
Comment thread .changeset/vale-recipe-regex-and-fixtures.md Outdated
@thecodedrift
thecodedrift force-pushed the docs/create-vale-rule-regex-and-fixtures branch from 9efbca2 to fe6f12b Compare September 22, 2026 20:38
@thecodedrift

Copy link
Copy Markdown
Member Author

Re: @claude[bot] — "Claude finished @thecodedrift's task in 3m 50s —— Review complete…"
#384 (comment)

Read in full — four low findings, no high or medium. Fixed one: rawPatternRule was verified byte-identical to existenceOver("raw", …) for all three patterns and removed (4705f29). The remaining three (test scoping of the lookaround claim to tokens/swap, the spec-delta gap at cli-vale-rule-engine/spec.md:236, and the changeset's matcher-vs-config wording) are raised with the author as open questions, so their threads stay open.

— AI Coding Agent

… v13)

Step 3 said `tokens` and `swap` compile as Go RE2 and that lookahead and
lookbehind therefore do not exist, which sent authors off to split a rule
one pattern expresses. Vale compiles with Go `regexp` first and falls back
to `regexp2`, so lookaround and backreferences work. Measured on the
vendored binary and pinned in the vendor contract suite.

The test section's `check <id>/.tests/fail --json` was reported as a command
that can never return a finding. It does: `.taskless/` is excluded from the
whole-project walk only, and a path the user names is honored. The shape
that really empties it is the rule's own `[.taskless/**]` matcher setting
the rule to NO, which leaves `test` green and `check` silent, so the recipe
names that and the troubleshooting list now starts by reading the finding.
The advisory called the matcher unnecessary because `check` excludes
`.taskless/` before Vale runs, so it "acts only under a bare vale
invocation." The exclusion is applied on a whole-project walk only, since
an explicit path is a request, so the matcher does bite on
`check .taskless/rules/vale/<id>/.tests/fail` and empties the one command
that shows an author a rendered message while `test` stays green.

The advisory now names both halves, and the docblock records why it stopped
calling the matcher harmless. `create-vale-rule` and `update` repeated the
same claim in prose and are corrected with it.
The recipe scopes the lookaround/backreference claim to `tokens` and
`swap`, but every measurement behind it went through `raw`. Measuring the
other two against the vendored binary found two silent divergences:

- a backreference does nothing as a `swap` key, where the identical
  pattern fires under `tokens` and `raw`, with nothing on stderr
- the implicit `\b` on `tokens`/`swap` is appended after a trailing
  lookahead, so `foo(?=bar)` can never match there while `raw` fires

Step 3 now carries both as caveats instead of a flat "they work", and
the vendor-contract suite pins them.

Also corrects `cli-vale-rule-engine`, which still described a
`[.taskless/**]` matcher as acting "only under a bare vale invocation" —
the claim #370/#371 removed from the recipe and the advisory — and
states the imprecise topic span as the measured v1-through-v12.
@thecodedrift
thecodedrift force-pushed the docs/create-vale-rule-regex-and-fixtures branch from f073353 to f4d82cb Compare September 22, 2026 21:22
@thecodedrift
thecodedrift merged commit 3fc580c into main Sep 22, 2026
4 checks passed
@thecodedrift
thecodedrift deleted the docs/create-vale-rule-regex-and-fixtures branch September 22, 2026 21:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

1 participant