fix(cli): stop one AsciiDoc file from disabling every Vale rule - #157
fix(cli): stop one AsciiDoc file from disabling every Vale rule#157thecodedrift wants to merge 5 commits into
Conversation
|
@claude /review |
|
Claude finished @thecodedrift's task in 6m 33s —— View job I'll analyze this and get back to you. |
394a995 to
b6498f2
Compare
|
@claude /review The earlier review on this PR stopped before posting findings (the checklist above is unfinished), and the branch has changed since — it was rebased onto a corrected base and carries new commits. Please review the current head. |
|
Claude finished @thecodedrift's task in 3m 14s —— View job Review mode: incremental — read 1 prior review thread(s) before reviewing.
I read the full diff against Findings posted inline
Considered and not posted inline
Everything else — the single- |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
3ecd0cd to
b1eaaa7
Compare
Vale supports AsciiDoc, reStructuredText, XML/DITA and MDX but parses none of them itself — it shells out to asciidoctor, rst2html, dita or mdx2vast, and the @taskless/vale-* packages ship the binary with none of those alongside it. Without the converter Vale does not skip the file: it writes one E100 to stderr, nothing at all to stdout, and exits 2. The abort is Vale's own and it is not scoped to the offending file, so every finding from every other file in the run was destroyed before serialization. Measured against the example project, one .adoc took a check reporting five Vale findings across four files down to zero. runVale now excludes the converter-dependent extensions from Vale's walk and names the skipped files in a notice that says which converter would put them back in scope. The tiers live in one table in rules/vale/formats.ts, measured against the pinned binary rather than transcribed — which is how .asc, a third AsciiDoc spelling absent from the bug report, got covered. A per-extension test re-measures every entry against the real Vale, so a version bump that moves a format between tiers fails there instead of silently turning the engine off again. Two details are load-bearing: Vale honours exactly one --glob and keeps the last, so the .taskless/ and format exclusions must travel as one negated alternation; and Vale matches a --glob against the basename only when the pattern contains no `/`, so a bare *.adoc branch inside that alternation stops matching docs/guide.adoc and the crash survives one directory down. Vale's error output is also decoded rather than forwarded, so a failure reads as a sentence naming the missing program. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Cyga14bww8rmazH2XrF8ms
Two branches measured Vale 3.17.1's format tiers independently and landed
two tables that disagreed about six extensions. `VALE_FORMAT_TIERS` in
`rules/capabilities.ts` is now the only one: a flat extension -> tier record
where a converter-dependent row carries the program's own name in its tier
(`converter:asciidoctor`), so adding an extension is one line in one file.
`rules/vale/formats.ts` derives the exclusion list, the glob and the notice's
converter name from it and keeps no copy; the recipes render their format
lists from the same record.
`capabilities.ts` is the home rather than `formats.ts` because the tiers have
two consumers with incompatible constraints — the module that runs Vale, and
`prompts/recipes.ts`, which must stay free of every host capability or
`assert-prompts-graph` fails the build. It stays import-free.
Every row was re-probed against the shipped binary, each tier by the property
only that tier has, since ordinary prose fires in all three readable tiers:
- `.asc` and `.rest` are converter-dependent (exit 2, `E100 [lintAdoc]`
asciidoctor and `E100 [lintRST]` rst2html). Neither was in either table's
converter tier before this.
- `.tex` and `.rmd` are the plaintext fallback, not native: Vale lints
`% simply in a comment` and `simply <- 1` inside a ```{r} chunk.
- `.mkd` and `.mkdn` are the plaintext fallback too, found while re-measuring
the rest — both lint straight through a fenced code block, an HTML comment
and an indented block, where `.md`, `.markdown` and `.mdown` skip all three.
Calling them markup was the more expensive error of the two directions: it
would have promised a `scope:` that has nothing to act on.
- `.ditamap` was probed and is plaintext, so it is not in the converter tier
despite the `.dita` neighbour. `.xml` is "an XSLT transform", which is Vale's
own wording ("no XSLT transform provided").
- All 35 comment extensions and the six remaining markup ones re-confirmed
unchanged; `.mdown` was missing from the markup constant and is added.
Tests: the per-extension probing lives in `vale-vendor-contract.test.ts` only,
so a weaker fixture cannot overrule a stronger one — `vale-formats.test.ts`
lost its bare-prose tier loop, which could not discriminate, and keeps the
derivation and end-to-end cases. New there: a plaintext tier suite that asserts
each listed extension lints the construct a parser would have skipped, a
coverage case that every row of the table is reached by some probe, and a
plaintext-is-converter-free case in `formats.ts`.
MDX is now described as not supported *yet* in `create-vale-rule.txt` and
`route.txt` — Vale 3.18.0 parses it natively and a CLI update carrying that
Vale is expected to bring it, with no date promised. That release also adds a
Typst converter, so the table warns that a version bump invalidates every row
and names `.typ` as the known incoming case.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Cyga14bww8rmazH2XrF8ms
The reconciled table added `.asc` and `.rest` as converter-dependent, but VALE_CONVERTER_CHECKERS arrived from the branch below without them, so the per-extension probe had no tag to assert and the set-equality case failed. Measured against the shipped 3.17.1: `.asc` reports `[lintAdoc]` and `.rest` reports `[lintRST]`, matching their canonical spellings. Working as intended — the set-equality assertion exists so that an extension added to one list and not the other fails here rather than throwing on an undefined tag inside the probe. Also carries the `.xml` correction into the reconciled table, which is now the single source: it names `xsltproc and an XSLT stylesheet` rather than `an XSLT transform`, because the program alone does not make `.xml` lintable. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Cyga14bww8rmazH2XrF8ms
…ng a tool Maintainer decision: we do not offer support for any Vale format that parses through an external program. The skip notice used to end "Install it and put it on your PATH to have these files checked." That offered a path this build does not ship and does not test, and for `.xml` cannot deliver at all — an XSLT stylesheet is specific to the document, so no install makes it lintable. It also made behaviour host-dependent: macOS ships /usr/bin/xsltproc and typical Linux CI images do not, so the same repository checked differently depending on the machine. That split is what made this stack red in CI while green locally, which is the strongest argument for excluding unconditionally: a repository should check the same way everywhere. The exclusion mechanism is unchanged — it is what prevents the total-run crash. What changed is the promise. The programs are still named, as the reason rather than as a remedy, and the recipes now say plainly that installing them does not change the answer. The cost, accepted: a user who genuinely has asciidoctor installed loses `.adoc` checking that would have worked. Also reconciles the comment tier against Vale's own documentation, and sharpens the version-bump note now that the 3.18.0 picture is confirmed: `.mdx` gains a native parser and becomes supported, `.typ` gains one that shells out to typst2vast and so stays unsupported permanently, and MyST, Quarto and QDoc arrive needing no external program. All four require 3.18.0, so none is reachable from the pinned 3.17.1. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Cyga14bww8rmazH2XrF8ms
b6498f2 to
fdfc24f
Compare
Review asked whether the exclusion glob's case-sensitivity could let `GUIDE.ADOC` past the `--glob` and reproduce the whole-run crash. Measured against the pinned 3.17.1 binary, it cannot: Vale routes on the extension exactly as spelled. `doc.adoc` exits 2 with `E100 [lintAdoc]`; `doc.ADOC` and `doc.AdOc` are read as plain text and exit 0 with findings — including when the uppercase spelling names a lowercase file on a case-insensitive filesystem, since Vale routes on the path string it was handed. That leaves the asymmetry pointing the other way. `converterFor` lowercased on the assumption that a case-insensitive filesystem would make `README.RST` reStructuredText to Vale, so a file Vale had linted perfectly well could be named in the skip notice as one it never checked. Node's `glob` folds case with the filesystem, so the notice walk found `docs/GUIDE.ADOC` on macOS and not on Linux — a wrong notice on exactly one platform. `converterFor` now compares case-sensitively, and the walk re-checks every match through it rather than trusting the pattern. A new vendor-contract probe pins the binary's half: an uppercase converter-dependent extension must exit 0 with no `E100`, so a Vale that becomes case-insensitive turns red here before it can crash a user's run. Also corrects `skippedFilesNotice`'s doc comment, which still described the converter name as something to install — the message itself stopped saying that, and the tests already forbid it. No change to `capabilities.ts`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Cyga14bww8rmazH2XrF8ms
Placeholder for the review that landed as #issuecomment-5391681169; its three inline findings are answered and resolved on their threads. Acknowledging so a re-run does not resurface it. — AI Coding Agent |
All three inline findings are answered on their threads and resolved. Summary of the dispositions, since the two case-sensitivity findings converged on a premise that measurement contradicts: Findings 1 and 2 ( The asymmetry was still a bug, just on the other side. Finding 3 ( The — AI Coding Agent |
Stack (root → tip):
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."