test(vale): make the Vale 3.21.0 upgrade land green - #339
Merged
Merged
Conversation
The bot moved the six platform pins; this is what the 3.21.0 binary changed underneath them, measured against 3.20.0 side by side. VALE_VERSION moves to 3.21.0 and the vocabulary is re-derived. The diff is additive: `in` joins `conditional`'s field table, and `doc(` joins the scope families. Nothing was removed. `in` names a View scope, and Views live under a directory the rule layout has no home for, so the corpus records it as a measured member every use of which fails at load. Four things the pin alone left broken, each read rather than guessed: - Findings carry a new `Suggestions` field. It mirrors a `replace` action's params and is `[]` otherwise; `toFix` keeps reading `Action` and the contract pins that the two agree. - A duplicated `[glob]` section, or a repeated key in one, now keeps its LAST assignment (upstream 1e4f6ed, `ValueWithShadows`). The contract asserted first-wins in both orders; it now asserts last-wins in both, and assemble.ts's docstring says so. - Two perf commits made the timeout fixtures ~20x faster: 152KB ran in ~45ms against a 100ms budget and the test reported `ok`. Re-measured (linear, ~2.8us per repetition); the fixtures grow to 6.7MB and 19MB to restore the headroom the old ones had. - `.ipynb` is a format now (`internal/lint/notebook.go`), so it left the plaintext fallback for `markup`: a Markdown cell as Markdown, a code cell as its kernel's comments, raw cells and outputs unread. New tier row, new fixture, and three pins on what the tier means. New contract cases for what a rule author can reach: every `doc(...)` shape (chained, negated, standalone container, the inert standalone leaf, the E201 for a bad selector, a metric over one section), a metric honoring an ordinary scope, BlockIgnores/TokenIgnores on HTML, and an unknown action name refused at load. The sequence negated-scope fix in the release notes did not reproduce on either binary through the CLI's path and is pinned at the current count with that said.
… verify Two schema gaps the 3.21.0 vocabulary opened, one in each direction. Too strict: `scope: doc(<selector>)` was rejected as an unknown operand. The generator records the family as a `doc(` prefix; the schema now knows the term ends in `)` with something between, and splits a chain on `&` the way Vale does (a port of `splitOutside`), so a selector carrying `&` inside quotes is one term rather than two fragments. What is between the parens is not parsed: Vale compiles every selector at load and reports a bad one as `E201 invalid selector in 'doc(...)'`, which `test` surfaces, the path a malformed regex already takes. Too lax: an unknown `action` name. Measured on 3.20.0 the rule loaded and the run died only when it fired, as an E100 with no path; 3.21.0 refuses it at load as an E201 naming the file, and one config serves the run, so a typo silences every Vale rule on every check. The five accepted names are transcribed from upstream `checkAction` and held to the binary by corpus rows. Corpus: five accepted `doc(...)` shapes, the unclosed and empty forms the schema rejects with the binary, and three recorded divergences where the schema accepts what Vale does not honor because judging it would mean parsing CSS against the document (a standalone leaf is inert; a bad selector is Vale's to report; `in` needs a View the rule layout cannot supply). Each mutation was run: the plain split fails the quoted-ampersand row, dropping the paren check fails the empty and unclosed rows, dropping the action check fails both rejected rows.
The changeset grows in place, for a rule author: what can now be written (`doc(...)` scopes, a metric that honors its scope, `.ipynb`, BlockIgnores on HTML) and what an existing rule will observe (a duplicated matcher keeps its last assignment, an unknown action fails at load, a scoped metric measures what it names, notebook JSON is no longer prose). Only what the vendor contract pins end to end is listed; `in` on `conditional` and TextFSM Views are not, since neither can be reached through the rule layout. `update.md` gains the 0.11.2 ledger entry with the same list and what to do about each, every item measured against the 3.20.0 and 3.21.0 binaries side by side. The sequence negated-scope fix in the release notes did not reproduce on either binary, and the entry says so the way the ast-grep root-metavariable case does. `create-vale-rule.md` adds `doc(...)` to the scope table with the one trap (a leaf on its own is inert; chain it) and notes that `metric` takes a scope. Both topic versions bump.
Member
Author
|
@claude /review |
|
Claude finished @thecodedrift's task in 5m 56s —— View job Review completeReview mode: incremental — read 0 prior review thread(s) before reviewing. Todo
SummaryThis is an unusually well-substantiated PR — every behavioral claim (last-wins ini merge,
One inline note posted (low severity / plausible, not confirmed): in No correctness bugs, removed-behavior regressions, or CLAUDE.md violations found elsewhere in the diff. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Member
Author
Read and acknowledged. The one inline note (action-value case sensitivity) is being measured against the binary with a corpus row; the thread will carry the result. — AI Coding Agent |
Review on #339: `ACTION_NAMES.includes(name)` compares the value verbatim, and the only casing row (`action/mixed-case-name-key`) used an already-invalid value, so it proved the key's case-insensitivity and nothing about the value's. Measured on 3.21.0 with a params shape that loads clean under `replace`: `Replace` and `REPLACE` both draw `E201 unknown action 'Replace'`. The schema was already the binary's answer; the new row `action/mixed-case-name-value` is what holds it there. Mutation run: folding the value to lowercase before the compare fails that row as "Vale rejected, schema accepted". The docstring says which half of the action map is folded and which is not.
thecodedrift
added a commit
that referenced
this pull request
Sep 16, 2026
Review on #339: `ACTION_NAMES.includes(name)` compares the value verbatim, and the only casing row (`action/mixed-case-name-key`) used an already-invalid value, so it proved the key's case-insensitivity and nothing about the value's. Measured on 3.21.0 with a params shape that loads clean under `replace`: `Replace` and `REPLACE` both draw `E201 unknown action 'Replace'`. The schema was already the binary's answer; the new row `action/mixed-case-name-value` is what holds it there. Mutation run: folding the value to lowercase before the compare fails that row as "Vale rejected, schema accepted". The docstring says which half of the action map is folded and which is not.
thecodedrift
added a commit
that referenced
this pull request
Sep 16, 2026
Review on #339: `ACTION_NAMES.includes(name)` compares the value verbatim, and the only casing row (`action/mixed-case-name-key`) used an already-invalid value, so it proved the key's case-insensitivity and nothing about the value's. Measured on 3.21.0 with a params shape that loads clean under `replace`: `Replace` and `REPLACE` both draw `E201 unknown action 'Replace'`. The schema was already the binary's answer; the new row `action/mixed-case-name-value` is what holds it there. Mutation run: folding the value to lowercase before the compare fails that row as "Vale rejected, schema accepted". The docstring says which half of the action map is folded and which is not.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stack (root → tip):
#336 moves the six
@taskless/vale-*pins to Vale 3.21.0. That commit alone does not land green:VALE_VERSIONstill says 3.20.0, the generated vocabulary was derived against 3.20.0, and four things the binary changed underneath the pins fail the existing suites. This PR is what makes the upgrade correct, measured against both binaries side by side, and it merges down intovendor/vale/upgradeso the upgrade reachesmainatomically.What the pin bump alone left broken
VALE_VERSION→ 3.21.0;vale-vocabulary.tsre-derived. The vocabulary diff is additive:injoinsconditional's fields,doc(joins the scope families. Nothing removed.Suggestionsfield (mirrors areplaceaction's params).toFixkeeps readingAction; the contract pins that the two agree.[glob]section or repeated key in a rule's.vale.ininow keeps its last assignment (upstream 1e4f6ed). The contract asserted first-wins; it now asserts last-wins in both orders, andassemble.tssays so..ipynbis a format now, so it left the plaintext fallback: new tier row, new fixture, three pins on what the tier means.Schema gaps the new vocabulary opened
scope: doc(<selector>)was rejected as an unknown operand.verifynow accepts the family, splitting a chain on&the way Vale does (a port ofsplitOutside) so a selector with&inside quotes is one term. The selector itself is left to Vale, which reports a bad one asE201at load — the path a malformed regex already takes.actionname. 3.20.0 loaded it and died on first match with a path-less E100; 3.21.0 refuses it at load as E201, and one config serves the run, so a typo silences every Vale rule on every check.verifynow rejects a name outsidereplace,remove,suggest,convert,edit.New contract pins (binary invoked directly)
Every
doc(...)shape (chained, negated, standalone container, the inert standalone leaf, E201 for a bad selector, ametricover one section); ametrichonoring an ordinary scope;BlockIgnores/TokenIgnoreson HTML; unknown action refused at load. Thesequencenegated-scope double-report fix from the release notes did not reproduce on either binary through the CLI's path; it is pinned at the current count with that said.Changeset and ledger
.changeset/vale-3-21-0.mdis grown in place (stillpatch).update.mdgains aMigrating to 0.11.2entry because existing rules change behavior: ametricwith ascopenow measures per block, a badactionfails the run at load, duplicated ini keys flip to last-wins, and[*.ipynb]rules stop linting notebook JSON.create-vale-rule.mddocuments thedoc(...)shapes and the inert-leaf trap.Stacked on #336 — merges down into
vendor/vale/upgrade.