fix: correct the Vale regex-engine claim, the fixture-bucket advice, and the .taskless/** advisory - #384
Conversation
|
@claude /review |
|
Claude finished @thecodedrift's task in 3m 50s —— View job Review complete
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 Verified
New findings
No correctness bugs found in the executable code paths ( |
9efbca2 to
fe6f12b
Compare
Read in full — four — 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.
f073353 to
f4d82cb
Compare
Four corrections, all to the same claim family: what Vale's regex engine can do, and where
checkdoes 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
tokensandswap"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'sregexpfirst and falls back toregexp2when 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 onpass/:\b(\w+) \1\bok: truefoo(?= bar)ok: true(?<=x )yok: trueA 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:
Githubstays quiet insideGithubToken,click hereinsideClicking here, andobviouslyfires insideobviously-namedwhileobviously_staleis safe.The step now says: Go
regexpfirst,regexp2fallback, 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
tokensandswap, and onlyrawwas measuredReview caught that the three measurements above all build a
raw:list, while the step scopes its claim totokensandswap— different keys, because those two get an implicit\b…\bwrapping thatrawdoes not. Measuring the other two found the claim is not uniformly true:rawtokensswapfoo(?= bar)(?<=x )y(\w+) \1foo(?=bar)Two silent divergences, both now documented and pinned:
swapkey. The identical pattern fires undertokensandraw. Nothing reaches stderr and the rule loads, so aswaprule built on\1looks healthy and never fires at all. A literalswapkey over the same document fires, which isolates the cause to the backreference rather than the scaffolding. A repeated-word check has to be anexistencerule.tokens/swapmust peek at a non-word character. The implicit\bis 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.rawis 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 --jsoncan never return a finding, becausecheckexcludes.taskless/unconditionally. It is not unconditional.run.tsapplies 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 --jsonreturned three findings with the message text rendered. Same forno-hedging, three findings.check --jsonreturned zero results under.taskless/.The symptom in the issue reproduces from a different cause. Adding a
[.taskless/**]matcher setting the rule toNOlefttestatok: trueandcheckon the same bucket atresults: []. 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
testcannot show you, and that an empty bucket means the rule's own matcher rather than the.vale.inibeing 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-fixturesoncheck) 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:
After:
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
checkcommand could not work. It is still an advisory, since the matcher has a legitimate reading (keeping a barevalequiet over fixtures that hold violations on purpose); it is no longer described as harmless. The docblock aboveadviseValeRuleConfigrecords why that changed.agent updaterepeated 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-enginestill carried the phrasing this PR removes from the recipe and the advisory: a[.taskless/**]matcher "acts only under a barevaleinvocation", with a scenario requiringverifyto "report thatcheckalready 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-specrestates that requirement so both halves are normative — unnecessary on a whole-project check, AND silencing on a path named explicitly — and adds thecheck-notices assertion the other advisory scenarios already carry. No code, no behaviour change; the spec catching up to what shipped.The
MODIFIEDblock restates the requirement in full, sinceopenspec archivereplaces 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.tsgains 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 coverraw; three cover thetokensboundary interaction, lookaround under aswapkey, and the backreference thatswapsilently ignores.recipe-cross-references.test.tspins the corrected recipe prose, including the two notes that survived the rewrite.vale-config-schema.test.tspinned the advisory verbatim and is updated to the new text.pnpm typecheck,pnpm lintandpnpm --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 ofcheck --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 ispatch.Fixes #371
Fixes #370