feat(cli): move the Vale engine to 3.18.0 and re-measure its format reach - #161
Conversation
|
@claude /review |
|
Claude finished @thecodedrift's task in 7m 24s —— View job Review completeReview mode: incremental — read 0 prior review thread(s) before reviewing.
Overall assessment: the mechanical parts of this PR are solid — the version pin is consistent everywhere it needs to be, the single-source-of-truth table ( What I did find is a cluster of stale doc comments in
None of these affect runtime behavior or test correctness — the tables and fixtures themselves are right — but they're exactly the kind of comment that outlives the code it described and misleads the next person who trusts prose over the table it's supposed to explain, in a file whose entire design premise is "don't trust prose, measure it." One process/convention note: One minor test-quality nit: All findings are posted as inline comments on the relevant lines. |
All nine findings addressed in 922e378, and every one of them held up — nothing here was a false positive or already fixed. Each inline thread has a reply with the specifics; the two things worth surfacing at this level: The MyST observation was a missing row, not just a stale sentence. The doc block predicted MyST would arrive as Re-probed rather than reasoned, on the pinned 3.18.0 binary, using each language's own comment syntax — a wrong delimiter is indistinguishable from absent support:
So the table as committed was right; the prose around it was the thing describing 3.17.1. On the changeset: agreed and folded — One thing I found and deliberately did not change: — AI Coding Agent |
922e378 to
a40adc6
Compare
2bc22ef to
72b9834
Compare
a40adc6 to
7066045
Compare
All six move from 3.17.1-20260810052605 to 3.18.0-20260824195610, the stamp published by release-vale.yml on the merge of the manifest PR. The six SHA256 digests in that manifest were verified against upstream's vale_3.18.0_checksums.txt before the merge, and all six packages carry the identical stamp, so no platform is out of step with another. This commit alone changes what the binary does without changing what the capability table says it does, which the table's own doc comment warns is the dangerous direction: `.typ` becomes converter-backed in 3.18.0 while the table still reads it as plaintext, so it would be handed to Vale and take the run down. The next commit re-measures every row. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Cyga14bww8rmazH2XrF8ms
Seven rows moved, in both directions, and each was probed rather than read
off the release notes.
.mdx converter:mdx2vast -> markup now parsed natively
.typ plaintext -> converter now needs typst2vast
.rmd plaintext -> markup
.qml plaintext -> comment
.scss plaintext -> comment
.qmd (new) -> markup
.qdoc (new) -> comment
`.typ` is the row that mattered. An extension missing from the table is read
as prose, which is harmless; the moment Vale routes it to a converter, the
same omission is an `E100` that takes down every Vale rule in the run. The
bump alone would have shipped that.
The recipes carried the inverse of both facts. They told an author MDX was
unsupported, which now costs them a format Vale reads fine, and they used
`[*.{md,mdx}]` as the worked example of a matcher that breaks `check` —
which is now a perfectly good matcher. The cautionary example is `.typ`, and
the recipe says why it changed, since the dangerous extension is whichever
one the rendered list names rather than the one an agent remembers.
Probing found one change the release notes do not mention: PHP comment
extraction requires a real `<?php` tag on 3.18.0, where 3.17.1 linted a bare
`//` comment without one. The fixture is corrected rather than the row.
Four rows that looked changed were the probe's fault, not Vale's — `.html`,
`.htm` and `.xhtml` need prose inside an element, and `.php` needs its open
tag. Feeding a format the wrong construct is indistinguishable from absent
support, which is why every tier is asserted by its own discriminating
property and the per-extension fixtures live in exactly one file.
Verified end to end: a project containing `doc.mdx` and `paper.typ` reports
the MDX finding and names only the Typst file as skipped.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Cyga14bww8rmazH2XrF8ms
…edicted
The format table's own header insists on "MEASURED, NOT DOCUMENTED", and the
bump left its prose describing a version we no longer pin: the block above
VALE_FORMAT_TIERS still framed 3.18.0 as "the known incoming bump", the
plaintext bullet still cited `.rmd`, the `.pyi` note still said "we pin
3.17.1", and VALE_CONVERTER_DEPENDENT still used `[*.{md,mdx}]` as the glob
that takes down the whole pass — the one place the `.typ` swap was missed.
The same `.rmd` staleness sat in two contract-test doc comments.
The forecast also predicted a MyST row and none was added. Probed on the
pinned binary, `.myst` skips a fenced code block (zero findings) where a bare
line lints, so it is markup; it is now a row with its own fixture.
The 3.18.0 changeset is folded into `.changeset/vale-converter-formats.md`
rather than shipping beside it, per "a later PR extends the existing file" —
and folding it caught two sentences there that the bump falsified.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Cyga14bww8rmazH2XrF8ms
7066045 to
dc2b88c
Compare
The most repeated finding in the merged-PR review threads: a comment that cites a version or a claim the code moved out from under it. On #161 alone, five threads — "we pin 3.17.1" left behind after VALE_VERSION went to 3.18.0, a comment naming .rmd as plaintext after the diff moved it to markup, a block still framing 3.18.0 as "the known incoming bump" after it shipped. Same shape on #155 and #157. The rule flags the construction rather than the stale number, which is the half a static check can see. Vale reads .ts in its comments-only tier, so it lints the comment text and never the code. Zero findings on packages/cli/src today, because #161 fixed them all. Reach verified separately by planting a forecast comment in a real source file: fixtures run under an isolating config, so a passing test does not prove the matcher glob reaches anything. Also ignores .taskless/ in eslint and prettier. A rule about source comments needs .ts fixtures, Vale decides its comments-only tier by extension, and those files belong to no tsconfig — so lint-staged failed the commit on "not found by the project service". Rule fixtures are inputs written to be flagged; verify and test are what keep them honest. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017cEN93Acyp4zBwP3oDnyy1
Stack (root → tip):
Moves the Vale engine to
3.18.0-20260824195610— the stamprelease-vale.ymlpublished when the manifest PR (#158) merged — and re-measures the format table against the new binary.Two commits, deliberately: the pin, then the re-measure. They cannot be split into separate PRs. The pin alone changes what the binary does without changing what the table says it does, and for
.typthat is the crashing direction (below).Seven rows moved, in both directions
.mdx.typ.rmd.qml.scss.qmd.qdoc.typis the row that mattered. An extension missing from the table is read as prose, which is harmless. The moment Vale routes it to a converter, that same omission is anE100that takes down every Vale rule in the run — the failure this table exists to prevent, arriving under an extension nobody was watching. That is what the table's doc comment warns about, and it is now a worked example rather than a hypothetical.The recipes carried the inverse of both facts
They told an author MDX was unsupported — which now costs them a format Vale reads fine — and they used
[*.{md,mdx}]as the worked example of a matcher that breakscheck. That matcher is now perfectly good. The cautionary example is[*.{md,typ}], and the recipe says why it changed, because the dangerous extension is whichever one the rendered list names, not the one an agent remembers.Measured, not read off the release notes
Every row was re-probed by its own discriminating property: markup by a construct only a parser skips, comment-aware by a bare non-comment line yielding zero, plaintext by that same line yielding a finding, converter-backed by the
E100exit and its checker tag.That caught one change the release notes do not mention: PHP comment extraction now requires a real
<?phptag, where 3.17.1 linted a bare//comment without one. The fixture is corrected rather than the row.It also produced four false positives that were the probe's fault, not Vale's —
.html,.htmand.xhtmlneed prose inside an element, and.phpneeds its open tag. Feeding a format the wrong construct is indistinguishable from absent support, which is exactly why each tier is asserted by its own discriminating property and the per-extension fixtures live in one file rather than two.End-to-end
A project containing
doc.mdxandpaper.typ:MDX linted under
[*.{html,md,mdx}]; only the Typst file skipped.Provenance
The six SHA256 digests in the manifest were verified against upstream's
vale_3.18.0_checksums.txtbefore #158 was merged, and all six packages carry the identical stamp — no platform out of step. This was also the first exercise of the OIDC trusted-publishing path since it moved to thenpm-autopublishenvironment; it succeeded.Refs #151
Built on top of #155
An agent reading
route.txtcould not tell whether a language was buildable locally or had to escalate toruntime, which needs a login. The reporter's case: two GitHub Actions YAML rules were routed toruntimebecause nothing said whether ast-grep handles YAML.It does. The pinned
@ast-grep/cli@0.41.0lists 26 languages includingYaml, and a realpr-workflow-no-branches-filterrule (language: Yaml) passesverifyandtest. Both Actions rules belong increate-sg-rulewith no login.Refs #151
Derived, not transcribed
There was no source of truth for either engine's reach.
$defs.Languagein the vendored schema is a bare string with no enum,verifynever validateslanguage, anddetect --jsonreports the repository's languages in a different vocabulary. The binaries were the only authority, androute.txtsaid nothing.Transcribing two lists into a
.txtwould drift on a version bump with nothing to catch it, and stale prose is worse than the current silence — silence at least makes the agent ask. So: pinned constants insrc/rules/capabilities.ts, substituted into recipes as%(KEY)s, with vendor-contract tests asserting the constants still match the binaries. That is the patternast-grep-vendor-contract.test.tsalready exists for — exact-pinned binary plus a test that fires on the bump.capabilities.tsis deliberately import-free:prompts/recipes.tsimports it, and theassert-prompts-graphvite plugin fails the build if that chunk reaches a host capability.Rejected alternative: extending
detect --jsonwith anenginesblock.cli-detect's spec framesdetectas an offline repo scan, engine reach is knowable at build time, and it would cost a third subprocess turn in a recipe already spending two.Vale's reach was measured, not read
Vale self-reports no format list, so every tier here comes from probing the shipped 3.17.1 binary — and the measurement corrected the starting assumptions in five places:
.cssis comment-tier, not plaintext. The first probe said otherwise because it fed CSS a//comment, which is not one..sassis comment-tier;.scssis not..Ris comment-aware,.PY/.JS/.TSare not;.cc/.cxx/.pyw/.pmare,.hh/.hxxare not..htm,.xhtml).--no-exitdoes not suppress the converter tier'sE100exit 2.Each tier is pinned by its discriminating property, because on ordinary prose all three readable tiers look identical: markup by a construct only a parser skips, comment-tier by the negative (a bare non-comment line must yield zero), converter-tier by non-zero exit plus
E100plus the converter's name. A test also asserts the fixture map covers the markup constant exactly, so an unprobed entry cannot be added to a constant.create-vale-rule.txtwas teaching the crashIts worked example globbed
[*.{md,mdx}]..mdxneedsmdx2vast, which this CLI does not ship, and one such file fails the entire Vale pass — so any rule following that example silently disabled every other Vale rule. Now[*.{md,markdown}], with the point stated plainly: a matcher like that is not a wider[*.md], it is a broken one. (The guard that makes this non-fatal is in the PR above this one.)A correction, included deliberately
The last commit walks back a claim this branch originally shipped. Both the recipe and the constants said a plausible
C++oryaml"passesverifyand fails inside the binary." Measured at 0.41.0,C++andcppare accepted aliases that resolve toCpp, so an off-list spelling is not reliably an error.The two failures that are real, neither caught locally: a name ast-grep does not recognize (
C#forCSharp) aborts config parsing, taking every other rule's report down with it; and a valid name for the wrong parser (TypeScriptover a.tsxtree) reports nothing and reads as a clean codebase. The advice to copy from the list is unchanged; the reason for it is now the accurate one. The vendor-contract assertions were already right — they pin the canonical list, not what the binary rejects — but their comment said otherwise.Also
route.txtnow states that a language on neither list does not route toruntimeby default:create-legacy-rulecomes first, since the repo's own linter may already speak it. And a new note separates reach (a property of the pinned version) from availability (a property of the host), which the existing "availability you can assert" bullet was conflating while giving the agent no way to assert it.OpenSpec delta on
cli-rule-routing— engine reach becomes a fourth thing the recipe is specified to state, with the normative core being that it is derived from the pinned versions rather than transcribed. Not archived; this is not the tip of the stack.Built on top of #157
One
.adoc,.rst,.mdx,.xml,.ditaor.ascanywhere a Vale rule's matcher reaches disabled every Vale rule in the repo. Measured: a run reporting 5 Vale findings across 5 files dropped to 0 with one.adocadded, andcheckstill exited on an unrelated ast-grep finding — so the disappearance read as a normal red check.Vale genuinely supports these formats upstream. The failure is a missing external converter in our distribution, not absent support.
Refs #151
Mechanism, established before designing the fix
Vale aborts the whole run; our code discards nothing. On the first
E100, stdout is 0 bytes and exit is 2 — the Markdown findings are destroyed inside Vale before serialization. That ruled out "tolerate the exit code and parse partial output," which was the obvious first guess.Two further measured facts shaped it: the trigger is the config, not the file (with
[*.{html,md}]an.adocis inert; with[*]it crashes), and per-file invocation would cost N subprocesses. A Vale config setting ([formats] adoc = md) would lint AsciiDoc with the Markdown parser and emit nonsense. So: exclude the converter-dependent files from the set handed to Vale, and name the skip rather than letting it be silent.Two things that looked correct and were not
--glob, last-wins. Two flags silently drop the first.--globmatches the basename only when the pattern has no/. Combined with the existing.taskless/**exclusion the expression goes path-wise, at which point a bare*.adocexcluded root-leveld.adocbut notdocs/e.adoc— a fix that looks right in whichever directory you happened to test it in. Hence**/*.adoc. Both are now pinned by tests.Assert known support, with one deliberate inversion
The operative list is the converter tier, not a native allowlist. Vale lints far more than markup — source comments, plain text, and extensionless files (
README,LICENSE,Makefile) — so a positive allowlist would have to enumerate every language Vale knows and would still drop every extensionless file, trading a loud crash for silent disabling across a much larger set. Unknown-to-us is safe because it is unknown-to-Vale.The assert-known-support property is carried instead by the table naming what was measured, the exclusion being derived from it so the two cannot drift, and a per-extension test that re-measures every row against the real binary.
The exclusion applies even to explicitly named paths, overriding the prior "an explicit path is a request" rule in
run.ts. Justified in-comment: honouring the request does not check that file badly, it costs the user the rest of their check.Vale's stderr JSON is now also decoded to a sentence (
E201: 'level' must be one of [...] in .../bogus.yml), preserving the error code — followingdecode-sg-stderr.md.Reconciliation with
capabilities.tsThis branch and #155 independently measured Vale's tiers and produced two tables. The second commit merges them into one record in
capabilities.ts, with the converter name riding in the tier ("converter:asciidoctor") so one row states both the tier and the program to install — adding an extension is one line in one file.formats.tsnow holds no data at all; everything derives, and a test asserts the derivation.Both tables were wrong about different things, which is a decent argument for having built them independently:
.asc,.restcapabilities.tsE100 [lintAdoc]/[lintRST].tex,.rmd%comment / an R chunk, where.mdskips a fence.mkd,.mkdn.mdowncapabilities.ts.ditamapThe
.mkd/.mkdnerror was the more expensive direction: calling them markup promises ascope:that has nothing to act on.A per-extension probe now covers every row (
probes every row of VALE_FORMAT_TIERSasserts set-equality between the table's keys and what the suites measure), so a row cannot be added without being measured. The old bare-prose tier loop was removed rather than kept — bare prose cannot discriminate any of the three readable tiers, and leaving it would let a weaker fixture overrule a stronger one.MDX and the next bump
MDX is described as not supported yet, with Vale 3.18.0 parsing it natively and a CLI update expected to carry it. No date promised — a test asserts the recipe contains no
20NN-NN.typst2vast)..typis plaintext today, so a bump without re-measuring walks straight back into this bug under a new extension. The table's doc comment carries that warning and names both known 3.18.0 moves (.typ→ converter,.mdx→ markup). Vale updates within the0.11.xlineage, separately from this fix.On textlint
There is no recorded comparison anywhere — zero mentions of textlint, proselint, LanguageTool or alex in the working tree, all history, all 40 archived changes. Vale appears already chosen, on gap-filling grounds plus one substantive criterion: its Tengo sandbox exposes only
text/math/fmt, so a Vale rule is inert data, which is what lets it ship static-tier with no login gate.The converter gap is a real dent in the "self-sufficient binary" reasoning that decision rested on — it anticipated engine-level unavailability, not a per-file input killing an available engine mid-run. But textlint's rules are JavaScript, which cannot be static-tier; they would need the runtime harness, signing, and a login. That is the load-bearing property and Vale wins on it. Not close to justifying a migration; worth recording in the spec.
OpenSpec: none
cli-vale-rule-enginealready requires that engine trouble is reported and does not abort the run. The crash violated that; the fix restores it. One caveat flagged rather than papered over: the spec says a rule's scope is expressed through its own matchers, and this narrows that above the user's config, including for explicitly named paths. If that should be recorded, the natural home is next to "Vale check executes against an assembled run config over the target paths."