diff --git a/.changeset/vale-converter-formats.md b/.changeset/vale-converter-formats.md new file mode 100644 index 00000000..2725d48d --- /dev/null +++ b/.changeset/vale-converter-formats.md @@ -0,0 +1,71 @@ +--- +"@taskless/cli": patch +--- + +Stop one AsciiDoc file from disabling every Vale rule in the project. + +Vale supports AsciiDoc, reStructuredText, XML/DITA and MDX, but it parses none +of them by itself — it shells out to `asciidoctor`, `rst2html`, `dita` or +`mdx2vast`, and the `@taskless/vale-*` packages ship the binary with none of +those alongside it. On a host without the converter Vale does not skip the file: +it prints one `E100 [lintAdoc] Runtime error` on stderr, writes nothing at all to +stdout, and exits 2. The abort is Vale's own and it is not scoped to the file +that caused it, so every finding from every other file in the run was destroyed +before it was ever serialized. Measured against the example project, adding a +single `.adoc` took a check that reported five Vale findings across four files +down to zero — reported as a raw JSON blob among the results, and exiting 1 the +same way any ordinary failing check does. + +`runVale` now excludes the converter-dependent extensions from Vale's own walk, +so the rest of the project is checked normally and the skipped files are named +in a notice that says which converter would put them back in scope. The tiers +live in one table in `rules/capabilities.ts` — the same record the agent recipes +render their format lists from — measured against the pinned binary rather than +transcribed from documentation. That is how `.asc` and `.rest`, a third AsciiDoc +spelling and a second reStructuredText one that crash identically and were in +neither bug report, ended up covered. Measurement also corrected four +extensions that a documentation reading had put in the wrong tier: `.tex`, +`.rmd`, `.mkd` and `.mkdn` are all read as plain text by this Vale, not parsed, +so excluding them would have dropped files Vale lints perfectly well. A +per-extension test re-measures every row against the real Vale — each tier by +the property only that tier has, since ordinary prose fires in all of them — so +a version bump that moves a format between tiers fails there instead of silently +turning the engine off again. + +The recipes now say MDX is not supported _yet_, rather than unsupported: Vale +3.18.0 parses it natively and a CLI update carrying that Vale is expected to +bring it. The same release adds a Typst converter, which will move `.typ` out of +the plaintext tier, so the table carries a standing instruction to re-measure +every row on a version bump. + +Two details are load-bearing and were both wrong on the first attempt. Vale +honours exactly one `--glob` and keeps the last, so the `.taskless/` exclusion +and the format exclusions have to travel as one negated alternation or the first +is silently discarded. And Vale matches a `--glob` against the basename only +when the pattern contains no `/` — combined with `.taskless/**` the whole +expression goes path-wise, at which point a bare `*.adoc` stops matching +`docs/guide.adoc` and the crash survives one directory down from wherever it was +tested. Vale's error output is also decoded now rather than forwarded verbatim, +so a failure reads as a sentence naming the missing program instead of a +five-field JSON object. + +**These formats are now stated as unsupported rather than as needing a tool.** +The notice used to end "Install it and put it on your PATH to have these files +checked", which offered a path this build does not ship, does not test, and for +`.xml` cannot deliver — 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. The exclusion is unconditional +for that reason, and the programs are still named as the reason rather than as +a remedy. + +The comment tier was reconciled against Vale's own documentation at +docs.vale.sh/formats/code, which adds `.bsh`, `.csx`, `.pod`, `.py3` and `.sbt` +once measured. It also documents `.pyi`, `.qml` and `.scss` as comment-aware, +and on the pinned 3.17.1 a bare non-comment line in each of them lints — so they +stay in the plaintext tier. That divergence is the argument for probing rather +than transcribing: the docs describe the current Vale, this build pins an older +one, and copying the list would have shipped `.scss` as comment-aware and been +wrong. `.pod` is a reminder of how easily this is misread — it lints Perl +comments but not POD blocks, so probing it with `=head1` looks like no support +at all. diff --git a/packages/cli/src/agent/create-vale-rule.txt b/packages/cli/src/agent/create-vale-rule.txt index 76bcb10e..7df1ceda 100644 --- a/packages/cli/src/agent/create-vale-rule.txt +++ b/packages/cli/src/agent/create-vale-rule.txt @@ -314,11 +314,18 @@ it. parser, so the whole file is linted as prose: a rule matched to YAML flags key names and values, not just the comments. If that is not what the rule means, narrow the glob rather than accepting it. - - **cannot be read at all** — Vale supports these formats but shells - out to an external converter to parse them, and this CLI ships none - of those converters: + These land here despite reading like markup, so a `scope:` value + has nothing to act on in them: %(VALE_PLAINTEXT_FORMATS)s + - **not supported** — Vale parses these only by shelling out to an + external program, and this build does not support any format that + needs one: %(VALE_CONVERTER_FORMATS)s + Do not tell the user to install the program. Taskless excludes these + files from the run whatever is installed, so that a repository + checks the same way on every machine; `.xml` could not work anyway, + since an XSLT stylesheet is specific to the document. + **A single unreadable file fails the whole Vale pass.** Vale exits 2 with an `E100` runtime error and abandons the run — `--no-exit` does not suppress it — so every other Vale rule over every other file goes @@ -326,6 +333,13 @@ it. matcher that takes `check` down the first time the repo grows an `.mdx` file. Never put one of those extensions in a glob. + **MDX is not supported yet.** Vale v%(VALE_VERSION)s reads `.mdx` only + through `mdx2vast`, which this CLI does not ship. Vale 3.18.0 parses + MDX natively, and a CLI update carrying that Vale is expected to + bring MDX support with it. Until then, scope the rule to `.md` and + say that `.mdx` is not supported yet rather than offering a matcher + that covers it. + 5. **Write the fixtures.** Two directories inside the rule, both flat. Vale lints the whole fixture tree, so a document nested a level deeper would be linted and never checked against either bucket, which diff --git a/packages/cli/src/agent/route.txt b/packages/cli/src/agent/route.txt index 597e9785..4832a3a4 100644 --- a/packages/cli/src/agent/route.txt +++ b/packages/cli/src/agent/route.txt @@ -122,18 +122,29 @@ answered together. `.sql` and every unnamed extension included. There is no parser, so the file is linted as one block of prose, and a Vale rule scoped to YAML flags the code as readily as the comments. That is - rarely what was asked for — say so before writing it. - - *cannot be read at all* — Vale supports these formats but shells - out to an external converter to parse them, and this CLI ships - none of those converters: + rarely what was asked for — say so before writing it. These read + like markup and are not: %(VALE_PLAINTEXT_FORMATS)s + - *not supported* — Vale parses these only by shelling out to an + external program, and this build does not support any format that + needs one: %(VALE_CONVERTER_FORMATS)s + Installing the program does not change this. Taskless excludes + these files from the Vale run whatever is on the machine, so that + a repository checks the same way everywhere rather than depending + on what a given host happens to have available. + **One such file fails the entire Vale pass, not just that file.** Vale exits 2 with an `E100` runtime error, `--no-exit` does not suppress it, and every other Vale rule over every other file goes unreported. A matcher written as `[*.{md,mdx}]` is not a wider `[*.md]` — it is a broken one. + **MDX is not supported yet.** Vale 3.18.0 parses MDX natively and + a CLI update carrying that Vale is expected to bring it; on + v%(VALE_VERSION)s, `.mdx` needs `mdx2vast` and this CLI ships + none. Tell the user MDX is not supported yet and scope to `.md`. + **A language on neither list does not route to runtime by default.** Check `create-legacy-rule` first: the repo may already run a linter that speaks it, and that linter's own dialect is a local destination diff --git a/packages/cli/src/prompts/recipes.ts b/packages/cli/src/prompts/recipes.ts index 29506259..73dc8e60 100644 --- a/packages/cli/src/prompts/recipes.ts +++ b/packages/cli/src/prompts/recipes.ts @@ -15,6 +15,7 @@ import { valeCommentList, valeConverterList, valeMarkupList, + valePlaintextList, } from "../rules/capabilities"; // Agent recipe files embedded at build time via Vite import.meta.glob. @@ -164,6 +165,7 @@ export function buildVariables( VALE_VERSION, VALE_MARKUP_FORMATS: valeMarkupList(), VALE_COMMENT_FORMATS: valeCommentList(), + VALE_PLAINTEXT_FORMATS: valePlaintextList(), VALE_CONVERTER_FORMATS: valeConverterList(), PACKAGE_MANAGER_DLX: options.packageManagerDlx ?? PACKAGE_MANAGER_DLX_MARKER, diff --git a/packages/cli/src/rules/capabilities.ts b/packages/cli/src/rules/capabilities.ts index 6655c8e7..45079323 100644 --- a/packages/cli/src/rules/capabilities.ts +++ b/packages/cli/src/rules/capabilities.ts @@ -98,79 +98,209 @@ export const AST_GREP_LANGUAGES = [ */ export const VALE_VERSION = "3.17.1"; +/** + * Which tier Vale routes an extension to. + * + * `converter:` carries the program's own name in the tier, so one + * table row states both the tier and the thing a user would install. The name + * is the one Vale prints in its `E100` text, because that is the string a + * reader will search for. + */ +export type ValeFormatTier = + /** Parsed in-process: the document is prose, its own syntax is skipped. */ + | "markup" + /** Parsed in-process: comment text is linted, the code body is invisible. */ + | "comment" + /** No parser: the whole file is linted as one block of prose. */ + | "plaintext" + /** Vale shells out to a program the `@taskless/vale-*` packages do not ship. */ + | `converter:${string}`; + +/** The `ValeFormatTier` prefix that marks a converter-dependent format. */ +const CONVERTER_TIER_PREFIX = "converter:"; + +/** + * EVERY MEASURED VALE EXTENSION, AND ITS TIER. THE ONLY TABLE. + * + * Adding an extension is one line here; every list, glob, notice and test below + * derives from this record, so there is no second place to keep in step. Two + * branches once measured this independently and produced two tables that + * disagreed about six extensions — that is what this single record exists to + * make impossible. + * + * MEASURED, NOT DOCUMENTED, AND MEASURED BY A DISCRIMINATING PROBE. Ordinary + * prose fires in all three readable tiers, so it can never separate them. Each + * tier is pinned by the property only that tier has, in + * `test/vale-vendor-contract.test.ts`: + * + * - **markup** — a construct only a real parser skips yields ZERO (a fenced + * code block, an Org `#` line, an HTML comment). + * - **comment** — the token in a comment yields one finding and the same token + * on a bare non-comment line yields ZERO. A type that fires on both is the + * plaintext fallback wearing a code extension. + * - **plaintext** — a bare line yields a finding. Listed only where the tier is + * surprising: `.tex`, `.rmd`, `.mkd` and `.mkdn` all look like markup and are + * not. Everything unnamed lands here too, which is why this tier does not + * need to be exhaustive. + * - **converter** — a non-zero exit whose output carries `E100` and the + * program's name. + * + * Measured spellings, not families. `.mdown` is native Markdown and `.mkd` and + * `.mkdn` are not; `.asc` is a third AsciiDoc spelling that crashes exactly + * like `.adoc`; `.ditamap` is plaintext while `.dita` needs `dita`. Case is + * part of the key — `.r` and `.R` were both measured, `.PY` was measured and is + * not comment-aware. Add a row only after probing it; the contract test refuses + * to take one on faith. + * + * A VERSION BUMP INVALIDATES THIS TABLE — RE-MEASURE THE WHOLE OF IT. The tiers + * are a property of {@link VALE_VERSION}'s binary, and the dangerous direction + * is a format Vale *learns*: an extension missing from this table is read as + * plain text today, but the moment Vale routes it to a converter the same + * omission is a crash that takes down every Vale rule in the run. + * + * Vale 3.18.0 is the known incoming bump, and it moves rows in three different + * directions — which is why "re-measure" is not boilerplate here: + * + * - `.mdx` gains a native parser, so it moves `converter:mdx2vast` → `markup` + * and becomes supported. + * - `.typ` gains a parser that shells out to `typst2vast` + * (https://docs.vale.sh/formats/typst), so it moves `plaintext` → + * `converter:typst2vast`. That is the dangerous direction: today it is read + * as prose, and after the bump the same row would crash the run. It also + * stays unsupported permanently, since we do not support formats needing an + * external program. + * - MyST, Quarto and QDoc arrive with parsers needing no external program, so + * they become genuinely supportable and want `markup` rows once measured. + * + * All four are documented as requiring v3.18.0 or later, so none of them is + * reachable from {@link VALE_VERSION}. Re-probe every row on the bump. + */ +export const VALE_FORMAT_TIERS: Readonly> = { + // markup — parsed, the format's own constructs skipped + ".htm": "markup", + ".html": "markup", + ".markdown": "markup", + ".md": "markup", + ".mdown": "markup", + ".org": "markup", + ".xhtml": "markup", + // comment text only — the code body is invisible + ".c": "comment", + ".c++": "comment", + ".bsh": "comment", + ".cc": "comment", + ".clj": "comment", + ".cpp": "comment", + ".cs": "comment", + ".css": "comment", + ".csx": "comment", + ".cxx": "comment", + ".go": "comment", + ".h": "comment", + ".h++": "comment", + ".hpp": "comment", + ".hs": "comment", + ".java": "comment", + ".jl": "comment", + ".js": "comment", + ".jsx": "comment", + ".less": "comment", + ".lua": "comment", + ".php": "comment", + ".pl": "comment", + ".pm": "comment", + ".pod": "comment", + ".proto": "comment", + ".ps1": "comment", + ".py": "comment", + ".py3": "comment", + ".pyw": "comment", + ".r": "comment", + ".R": "comment", + ".rb": "comment", + ".rs": "comment", + ".sass": "comment", + ".sbt": "comment", + ".scala": "comment", + ".swift": "comment", + ".ts": "comment", + ".tsx": "comment", + // plaintext, and surprising about it — these look parsed and are not + ".mkd": "plaintext", + ".mkdn": "plaintext", + ".rmd": "plaintext", + ".tex": "plaintext", + ".typ": "plaintext", + // plaintext HERE, though Vale's own docs list them as comment-tier. The docs + // describe the CURRENT Vale; we pin 3.17.1. Measured on the pinned binary a + // bare non-comment line lints, which is the plaintext signature. Transcribing + // the docs would have shipped these as comment-tier and been wrong for this + // build — the case for probing rather than copying. + ".pyi": "plaintext", + ".qml": "plaintext", + ".scss": "plaintext", + // converter-dependent — Vale supports the format, we ship no converter + ".adoc": "converter:asciidoctor", + ".asc": "converter:asciidoctor", + ".asciidoc": "converter:asciidoctor", + ".dita": "converter:dita", + ".mdx": "converter:mdx2vast", + ".rest": "converter:rst2html", + ".rst": "converter:rst2html", + ".xml": "converter:xsltproc and an XSLT stylesheet", +}; + +/** Every extension in `tier`, in table order. */ +function extensionsInTier(tier: ValeFormatTier): string[] { + return Object.entries(VALE_FORMAT_TIERS) + .filter(([, entry]) => entry === tier) + .map(([extension]) => extension); +} + /** * Extensions Vale parses as markup: the whole document is prose, and the * format's own non-prose constructs are excluded. * - * MEASURED, NOT DOCUMENTED. Each entry was distinguished from the plaintext - * fallback by a construct only a real parser skips — a fenced code block for - * Markdown, a `#` line for Org, an HTML comment for the HTML family — because - * on ordinary prose a markup parse and a plaintext parse are indistinguishable. - * * The HTML entries carry a consequence worth stating to an author: prose * outside an element is not linted, so a bare sentence in a `.html` file yields * nothing. */ -export const VALE_MARKUP_EXTENSIONS = [ - ".htm", - ".html", - ".markdown", - ".md", - ".org", - ".xhtml", -] as const; +export const VALE_MARKUP_EXTENSIONS: readonly string[] = + extensionsInTier("markup"); /** * Extensions where Vale lints **comment text only** and ignores the code body. + */ +export const VALE_COMMENT_EXTENSIONS: readonly string[] = + extensionsInTier("comment"); + +/** + * Extensions measured into the plaintext fallback whose spelling suggests + * otherwise. * - * MEASURED BY THE NEGATIVE, which is the only test that separates this tier - * from the plaintext fallback: a token inside a comment yields a finding, and - * the same token on a bare non-comment line yields nothing. A file type that - * fires on both is plaintext, not comment-aware. + * Not exhaustive and not meant to be — every unnamed extension is plaintext + * too. These are the ones an author would reasonably assume were parsed, so + * they are worth naming in a recipe rather than leaving to "everything else". + */ +export const VALE_PLAINTEXT_EXTENSIONS: readonly string[] = + extensionsInTier("plaintext"); + +/** + * The converter each converter-dependent extension needs, keyed by extension. * - * Case-sensitive, and not closed over the obvious aliases. `.r` and `.R` are - * both here because both were measured; `.PY` was measured and is not - * comment-aware, and neither are `.hh`/`.hxx` despite `.h`/`.hpp` being. Add an - * entry only after probing it — the contract test below refuses to take one on - * faith. + * The lookup `rules/vale/formats.ts` uses to name a converter in the skip + * notice. Keys are lowercase because every measured converter format is; a + * caller comparing an extension off the filesystem must lowercase it first, or + * `README.RST` becomes a crash on case-insensitive platforms only. */ -export const VALE_COMMENT_EXTENSIONS = [ - ".c", - ".c++", - ".cc", - ".clj", - ".cpp", - ".cs", - ".css", - ".cxx", - ".go", - ".h", - ".h++", - ".hpp", - ".hs", - ".java", - ".jl", - ".js", - ".jsx", - ".less", - ".lua", - ".php", - ".pl", - ".pm", - ".proto", - ".ps1", - ".py", - ".pyw", - ".r", - ".R", - ".rb", - ".rs", - ".sass", - ".scala", - ".swift", - ".ts", - ".tsx", -] as const; +export const VALE_CONVERTER_BY_EXTENSION: Readonly> = + Object.fromEntries( + Object.entries(VALE_FORMAT_TIERS).flatMap(([extension, tier]) => + tier.startsWith(CONVERTER_TIER_PREFIX) + ? [[extension, tier.slice(CONVERTER_TIER_PREFIX.length)]] + : [] + ) + ); /** A format Vale supports upstream but cannot read without an external tool. */ export interface ValeConverterFormat { @@ -180,6 +310,22 @@ export interface ValeConverterFormat { converter: string; } +/** The converter-dependent extensions grouped by the program they need. */ +function groupByConverter(): ValeConverterFormat[] { + const groups = new Map(); + for (const [extension, converter] of Object.entries( + VALE_CONVERTER_BY_EXTENSION + )) { + const existing = groups.get(converter); + if (existing === undefined) groups.set(converter, [extension]); + else existing.push(extension); + } + return [...groups].map(([converter, extensions]) => ({ + converter, + extensions, + })); +} + /** * Formats that fail rather than lint, because Vale shells out to a converter * this CLI does not ship. @@ -195,18 +341,9 @@ export interface ValeConverterFormat { * caught by a rule's glob takes down the entire Vale pass, including every * other rule and every other file — so `[*.{md,mdx}]` is not a slightly wider * matcher than `[*.md]`, it is a broken one. - * - * VERSION-SENSITIVE. Vale 3.18.0 parses MDX natively, so a bump past it moves - * `.mdx` out of this list; that is a {@link VALE_VERSION} edit plus an entry - * removal here, and the contract test fails until both happen. */ -export const VALE_CONVERTER_DEPENDENT: readonly ValeConverterFormat[] = [ - { extensions: [".rst"], converter: "rst2html" }, - { extensions: [".adoc", ".asciidoc"], converter: "asciidoctor" }, - { extensions: [".xml"], converter: "xsltproc and an XSLT stylesheet" }, - { extensions: [".dita"], converter: "dita" }, - { extensions: [".mdx"], converter: "mdx2vast" }, -]; +export const VALE_CONVERTER_DEPENDENT: readonly ValeConverterFormat[] = + groupByConverter(); /** * Vale's checker tag per converter-dependent extension, from the `E100` text. @@ -226,8 +363,10 @@ export const VALE_CONVERTER_DEPENDENT: readonly ValeConverterFormat[] = [ */ export const VALE_CONVERTER_CHECKERS: Readonly> = { ".rst": "lintRST", + ".rest": "lintRST", ".adoc": "lintAdoc", ".asciidoc": "lintAdoc", + ".asc": "lintAdoc", ".xml": "lintXML", ".dita": "lintDITA", ".mdx": "lintMDX", @@ -235,7 +374,7 @@ export const VALE_CONVERTER_CHECKERS: Readonly> = { /** Every converter-dependent extension, flattened. */ export const VALE_CONVERTER_DEPENDENT_EXTENSIONS: readonly string[] = - VALE_CONVERTER_DEPENDENT.flatMap((format) => format.extensions); + Object.keys(VALE_CONVERTER_BY_EXTENSION); /** `Bash, C, Cpp, …` — the ast-grep language list as recipe prose. */ export function astGrepLanguageList(): string { @@ -252,6 +391,16 @@ export function valeCommentList(): string { return VALE_COMMENT_EXTENSIONS.join(", "); } +/** + * `.mkd, .mkdn, …` — the plaintext extensions worth naming, as recipe prose. + * + * Rendered rather than written into the recipe because the surprising cases are + * exactly the ones a hand-written list gets wrong. + */ +export function valePlaintextList(): string { + return VALE_PLAINTEXT_EXTENSIONS.join(", "); +} + /** * `.rst (needs rst2html), …` — Vale's converter-dependent formats as recipe * prose, each naming the tool whose absence is the actual failure. diff --git a/packages/cli/src/rules/vale/formats.ts b/packages/cli/src/rules/vale/formats.ts new file mode 100644 index 00000000..763fd764 --- /dev/null +++ b/packages/cli/src/rules/vale/formats.ts @@ -0,0 +1,278 @@ +import { glob } from "node:fs/promises"; +import { basename, extname } from "node:path"; + +import { + VALE_CONVERTER_BY_EXTENSION, + VALE_CONVERTER_DEPENDENT_EXTENSIONS, +} from "../capabilities"; + +/** + * Taskless's own directory, as a project-relative path. + * + * Lives here rather than in `run.ts` because both the exclusion glob and the + * notice walk have to agree on what "ours, not the user's prose" means, and two + * copies of that string is how they stop agreeing. + */ +export const TASKLESS_DIRECTORY = ".taskless"; + +/** + * Which markup formats this build of Vale can actually parse. + * + * Vale supports AsciiDoc, reStructuredText, XML/DITA and (before 3.18) MDX + * upstream — but not on its own. For those formats it shells out to an external + * program to convert the source into something it can lint, and the + * `@taskless/vale-*` platform packages ship the Vale binary as pure payload + * with none of those programs alongside it. On a host without the converter the + * conversion fails, and Vale does not degrade: it prints one + * `E100 [lintAdoc] Runtime error` object on stderr, writes **nothing** to + * stdout, and exits 2. The abort is Vale's, not ours, and it is not scoped to + * the offending file — every finding from every other file in the same run is + * lost inside Vale before it is ever serialized. + * + * So a single `.adoc` anywhere a rule's matcher reaches turned the whole Vale + * engine off. `check` reported the crash, but as a raw JSON blob among the + * findings, and the exit code looked the same as any other failing check — so + * in a repo that already had an ast-grep finding, "every Vale rule stopped + * running" was indistinguishable from a normal red check. + * + * ## The table lives in `rules/capabilities.ts` + * + * `VALE_FORMAT_TIERS` there is the single measured record of what Vale does + * with an extension, and everything in this module is derived from it — the + * exclusion list, the glob, and the converter named in the notice. Nothing here + * restates a tier, because a second copy is precisely how two independently + * measured tables came to disagree about six extensions. + * + * `capabilities.ts` is the home rather than this file because the same tiers + * are read by two consumers with incompatible constraints: this module, which + * runs Vale, and `src/prompts/recipes.ts`, which renders the tiers into the + * agent recipes and must stay free of every host capability (`node:fs` here + * would fail `assert-prompts-graph` at build time). Pure data satisfies both. + * + * ## Asserting known support rather than dodging known breakage + * + * The tiers are **measured against the pinned binary**, not read off Vale's + * documentation, and `vale-vendor-contract.test.ts` re-measures every entry + * against the real binary on every run, each tier by the property only that + * tier has. A format whose tier changes — or a converter Vale starts requiring + * for a format we currently call native — turns that test red before it can + * turn a user's check silently green. + * + * The reason the operative list is the *converter* tier rather than the native + * one deserves stating, because "allowlist what we know works" reads like it + * should be the other way round. Vale does not lint markup only. Anything it + * does not recognize as markup it reads as plain text or as source-code + * comments — `.py`, `.ts`, `.yml`, `.txt`, and files with no extension at all + * (`README`, `LICENSE`, `Makefile`) all get linted, and none of them can shell + * out. An allowlist expressed as "only hand Vale these extensions" would + * therefore have to enumerate every language Vale knows *and* would still drop + * every extensionless file — trading a loud crash for exactly the silent + * disabling this whole module exists to prevent, over a far larger set of + * files. + * + * The safe path and the unknown path are the same path here, which is what + * makes an exclusion honest rather than a denylist with a nice name: shelling + * out is a property of a short, closed set of markup formats, and an extension + * outside that set falls through to Vale's plain-text reader, which has no + * converter to be missing. + * + * **That property is pinned to the vendored binary, and a version bump is what + * breaks it.** "Unknown to us" is safe only while it also means "unknown to + * Vale": the moment Vale learns a format, it starts routing that extension to a + * parser, and if that parser shells out, an extension missing from the table is + * a crash rather than a plain-text read. Vale 3.18.0 is the live example — it + * adds Typst, which parses through `typst2vast`, so `.typ` stops being the + * plaintext read it is today and upgrading the `@taskless/vale-*` packages + * without re-measuring would reintroduce exactly this bug under a new + * extension. Re-measure the whole table on every bump; the per-extension cases + * in `vale-vendor-contract.test.ts` are how. + * + * Note what the exclusion policy buys here. Because a format needing an + * external program is never supported, a newly-converter-backed extension has + * one correct destination rather than a judgement call, and the answer does not + * depend on what happens to be installed on the machine running `check`. + */ + +/** + * Extensions Vale must never be handed, lowercase, leading dot, sorted. + * + * Derived from the tier table rather than written out again, so adding a + * measured entry there is the whole change. + */ +export const CONVERTER_DEPENDENT_EXTENSIONS: readonly string[] = [ + ...VALE_CONVERTER_DEPENDENT_EXTENSIONS, +].toSorted(); + +/** + * The converter Vale would need for `path`, or `undefined` if it needs none. + * + * Extension comparison is **case-sensitive, because Vale's own routing is**. + * Measured against the pinned binary: `docs/guide.adoc` exits 2 with + * `E100 [lintAdoc]`, while `docs/guide.ADOC` and `docs/guide.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, not on the name the disk holds. + * + * This used to lowercase, on the assumption that a case-insensitive filesystem + * would make `README.RST` reStructuredText to Vale. It does not, and the + * assumption cost accuracy in the one place this function is read: a file Vale + * had linted perfectly well was named in the skip notice as one it never + * checked. Matching Vale exactly is what keeps the notice true, and it is the + * same discipline as the tier table — measure the binary, do not reason about + * it. `vale-vendor-contract.test.ts` pins the measurement, so a Vale that + * becomes case-insensitive turns red here before it can crash a user's run. + */ +export function converterFor(path: string): string | undefined { + return VALE_CONVERTER_BY_EXTENSION[extname(path)]; +} + +/** + * Glob patterns, in Vale's dialect, that exclude the converter-dependent files. + * + * A globstar-prefixed `*.adoc` rather than a bare one, and this is not + * cosmetic. Vale matches a `--glob` against the file's **basename** when the + * pattern contains no `/`, and against its path when it does. Every pattern + * here is combined into one + * alternation with `.taskless/**`, which contains a `/` — so the whole + * expression is matched path-wise, and a bare `*.adoc` branch then stops + * matching `docs/guide.adoc`. Measured: that exact combination still crashed on + * a nested file while excluding the root-level one, which is the worst possible + * shape of bug — it looks fixed in the repository you tested it in. + */ +export function converterExclusionGlobs(): string[] { + return CONVERTER_DEPENDENT_EXTENSIONS.map((extension) => `**/*${extension}`); +} + +/** + * The single `--glob` expression for a run, or `undefined` when there is + * nothing to exclude. + * + * One expression because Vale accepts one `--glob` and the last one wins: + * passing two flags silently drops the first, so the exclusions have to be one + * negated alternation or they are not exclusions at all. + */ +export function buildValeGlob(patterns: string[]): string | undefined { + if (patterns.length === 0) return undefined; + return `--glob=!{${patterns.join(",")}}`; +} + +/** How many skipped paths a notice names before it summarizes the rest. */ +const NOTICE_SAMPLE_LIMIT = 5; + +/** Directories never worth walking to build a notice. */ +const UNWALKED_DIRECTORIES = new Set([ + "node_modules", + ".git", + TASKLESS_DIRECTORY, +]); + +/** + * Converter-dependent files inside the run's target set. + * + * This exists only so the skip can be *named*. The fix itself needs no file + * list — Vale's own walker does the excluding — but a fix whose entire user + * experience is "some findings are quietly not there" would be the bug again + * one layer down, so the notice has to say which files and which converter. + * + * Two things it deliberately does not do. It does not reconstruct Vale's walk: + * it asks a much narrower question (are there files with these eight + * extensions?) whose answer is a notice, never a finding, so being approximate + * costs a slightly vague message and nothing else. And it does not run when + * there is nothing to say — the common repository has no AsciiDoc at all, and + * `glob` over a pruned tree returning empty is the whole cost in that case. + * + * Known imprecision, and it is one-directional: Node's `glob` does not descend + * into dot-directories, so a `.github/adr/0001.adoc` is skipped by Vale and + * goes unnamed here. That under-reports a notice; it never suppresses a + * finding, and it never lets the crash back in. + * + * Every match is re-checked through `converterFor` before it is kept, and that + * is not belt-and-braces. Node's `glob` inherits the filesystem's own case + * folding, so on macOS `**\/*.{adoc,…}` matches `docs/GUIDE.ADOC` — a file Vale + * routes to its plain-text reader and lints normally. Left unfiltered, the + * notice would claim a checked file was skipped, and it would claim it on + * exactly one platform. `converterFor` is the one place that knows how Vale + * routes, so the walk defers to it rather than trusting the pattern. + */ +export async function findConverterDependentFiles( + cwd: string, + paths: string[] +): Promise { + const extensions = CONVERTER_DEPENDENT_EXTENSIONS.map((extension) => + extension.slice(1) + ).join(","); + + // An explicitly named file answers by its own name; only a directory needs + // walking. A whole-project run has one target, the project. + const named: string[] = []; + const roots: string[] = []; + if (paths.length === 0) { + roots.push("."); + } else { + for (const path of paths) { + if (converterFor(path) !== undefined) named.push(path); + roots.push(path); + } + } + + const found = new Set(named); + for (const root of roots) { + const prefix = root === "." || root === "" ? "" : `${root}/`; + try { + for await (const match of glob(`${prefix}**/*.{${extensions}}`, { + cwd, + exclude: (entry) => UNWALKED_DIRECTORIES.has(basename(String(entry))), + })) { + if (converterFor(match) !== undefined) found.add(match); + } + } catch { + // A target that is not a directory, an unreadable subtree, a platform + // where `glob` rejects the pattern: all of them mean "no notice", never + // "no fix". The exclusion has already been applied by the time this runs. + } + } + + return [...found].toSorted(); +} + +/** + * The user-facing sentence for a set of skipped files, or `undefined` when + * nothing was skipped. + * + * Phrased as "not supported by this build", not "Vale does not support": Vale + * supports every one of these formats, and telling a user otherwise sends them + * to the wrong project's issue tracker. + * + * It names the converter as an *explanation*, never as an instruction. A format + * that needs an external program is not supported here, full stop — so the + * sentence must not read as "install `asciidoctor` and this will work", because + * the exclusion is unconditional and does not consult the host. The action + * offered is the one that actually works: scope the rule to a supported format. + */ +export function skippedFilesNotice(files: string[]): string | undefined { + if (files.length === 0) return undefined; + + const converters = [ + ...new Set( + files.flatMap((file) => { + const converter = converterFor(file); + return converter === undefined ? [] : [converter]; + }) + ), + ].toSorted(); + + const sample = files.slice(0, NOTICE_SAMPLE_LIMIT); + const remainder = files.length - sample.length; + const listed = + remainder > 0 + ? `${sample.join(", ")} (and ${String(remainder)} more)` + : sample.join(", "); + + return ( + `Vale did not check ${String(files.length)} file(s): ${listed}. These ` + + `formats are not supported by this build — Vale parses them only through ` + + `an external program (${converters.join(", ")}), which this build does ` + + `not ship and does not check for. Scope the rule to a supported format; ` + + `every other file was checked normally.` + ); +} diff --git a/packages/cli/src/rules/vale/run.ts b/packages/cli/src/rules/vale/run.ts index 92090428..b1e1bdc2 100644 --- a/packages/cli/src/rules/vale/run.ts +++ b/packages/cli/src/rules/vale/run.ts @@ -8,11 +8,15 @@ import { ASSEMBLED_VALE_CONFIG } from "../engines"; import { buildPath } from "../scan"; import { isWholeProjectWalk } from "../walk-scope"; import { findValeBinary, valeUnavailableMessage } from "./binary"; +import { + buildValeGlob, + converterExclusionGlobs, + findConverterDependentFiles, + skippedFilesNotice, + TASKLESS_DIRECTORY, +} from "./formats"; import { asValeConfigError, toValeCheckResults, type ValeOutput } from "./map"; -/** Taskless's own directory, as a project-relative path. */ -const TASKLESS_DIRECTORY = ".taskless"; - /** * The Vale config a run reads, relative to the project root. * @@ -84,6 +88,44 @@ export type ValeRunOutcome = | { status: "timeout"; blocking: true; message: string } | { status: "failed"; blocking: true; message: string }; +/** + * Vale's stderr, rendered as a sentence instead of a JSON blob. + * + * Vale reports its own errors as a one-object JSON document on stderr — + * `{Line, Path, Text, Code, Span}` with `Text` carrying embedded newlines. Piped + * straight into a failure message that lands amid a check's findings, it reads + * as a stack trace: the actionable half (`asciidoctor not found`) is four lines + * into a structure whose other four fields say nothing. Same reasoning as + * decoding ast-grep's stderr rather than forwarding bytes — the message is the + * only thing the user has to act on. + * + * Anything that is not that shape is returned untouched. A best-effort decoder + * that swallows what it cannot read would be worse than none. + */ +function describeValeStderr(stderr: string): string { + let parsed: unknown; + try { + parsed = JSON.parse(stderr); + } catch { + return stderr; + } + const error = asValeConfigError(parsed); + if (error === undefined) return stderr; + + const text = error.Text.split("\n") + .map((line) => line.trim()) + .filter((line) => line !== "") + .join(" "); + // The code stays in the message, and is prepended only when `Text` does not + // already open with it. Vale is inconsistent about that — `E100` repeats the + // code in its text and `E201` does not — and the code is what a user searches + // for, so losing it while "improving" the message would be a downgrade. + const code = text.startsWith(error.Code) ? "" : `${error.Code}: `; + return `${code}${text}${ + error.Path === undefined || error.Path === "" ? "" : ` in ${error.Path}` + }`; +} + export interface ValeRunOptions { /** Project root. Vale runs here, so the config's relative paths resolve. */ cwd: string; @@ -139,18 +181,42 @@ export async function runVale( const wholeProject = isWholeProjectWalk(paths); const targets = wholeProject ? ["."] : paths; - // Walking the whole project reaches `.taskless/` too, and Vale has no reason - // to know that directory is ours: with a rule enabled it reports findings in - // the rule configs and in the user's own rule definitions — prose - // complaints about the machinery, pointing at files nobody wrote as prose. - // Section globs do not help, since `.taskless/README.md` matches `[*.md]` as - // readily as any document. `--glob` filters which files are walked without - // touching which rules apply to them, so a user's scoping still decides that. + // Two exclusions reach Vale, and they have to travel together because Vale + // accepts exactly one `--glob` and the last one wins — pass two flags and the + // first is silently discarded, which is how an exclusion becomes a no-op that + // still looks applied on the command line. // - // Applied ONLY when we chose `.` ourselves. An explicit path is a request, - // and silently declining to check a file someone named would be worse than + // `.taskless/**` keeps Vale out of our own directory. Walking the whole + // project reaches it too, and Vale has no reason to know that directory is + // ours: with a rule enabled it reports findings in the rule configs and in the + // user's own rule definitions — prose complaints about the machinery, pointing + // at files nobody wrote as prose. Section globs do not help, since + // `.taskless/README.md` matches `[*.md]` as readily as any document. Applied + // ONLY when we chose `.` ourselves: an explicit path is a request, and + // silently declining to check a file someone named would be worse than // checking one they did not. - const exclude = wholeProject ? [`--glob=!${TASKLESS_DIRECTORY}/**`] : []; + // + // The converter-dependent formats are excluded on **every** run, named path or + // not, and that asymmetry is deliberate. Handing Vale one `.adoc` on a host + // with no `asciidoctor` does not check that file badly — it aborts the entire + // Vale process before any result is written, taking every other file's + // findings with it. Honouring the request would cost the user the rest of + // their check, so the request is declined and reported instead. See + // `formats.ts` for the tier table this is derived from. + const exclude = [ + ...(wholeProject ? [`${TASKLESS_DIRECTORY}/**`] : []), + ...converterExclusionGlobs(), + ]; + const globArgument = buildValeGlob(exclude); + const globFlags = globArgument === undefined ? [] : [globArgument]; + + // Asked before the run rather than inferred from it. Vale never reports what + // its walker declined to open, so once the glob has done its job the skipped + // files are unrecoverable from the output — and a fix whose only visible + // effect is that some findings are quietly missing is the bug it replaced. + const skipped = skippedFilesNotice( + await findConverterDependentFiles(options.cwd, paths) + ); // `--` separates flags from positional paths, so a path beginning with `-` // is not read as a flag. @@ -159,7 +225,7 @@ export async function runVale( configPath, "--output=JSON", "--no-exit", - ...exclude, + ...globFlags, "--", ...targets, ]; @@ -239,7 +305,9 @@ export async function runVale( settle({ status: "failed", blocking: true, - message: `Vale exited ${String(code)}${stderr === "" ? "" : `: ${stderr}`}`, + message: `Vale exited ${String(code)}${ + stderr === "" ? "" : `: ${describeValeStderr(stderr)}` + }`, }); return; } @@ -249,10 +317,18 @@ export async function runVale( // Attached to every `ok` path so a diagnostic cannot be dropped by which // branch happened to produce the (empty) results. const diagnostic = stderrChunks.join("").trim(); + // Both advisories share one field, so they are joined rather than one + // overwriting the other: a project can perfectly well have a section-less + // rule assignment *and* an AsciiDoc file, and dropping either message + // would be a silent skip wearing the other's clothes. + const advisories = [ + ...(skipped === undefined ? [] : [skipped]), + ...(diagnostic === "" + ? [] + : [`Vale reported while running: ${diagnostic}`]), + ]; const notice = - diagnostic === "" - ? {} - : { notice: `Vale reported while running: ${diagnostic}` }; + advisories.length === 0 ? {} : { notice: advisories.join("\n") }; const stdout = stdoutChunks.join("").trim(); if (stdout === "") { diff --git a/packages/cli/test/prompts.test.ts b/packages/cli/test/prompts.test.ts index 78e74222..6b406827 100644 --- a/packages/cli/test/prompts.test.ts +++ b/packages/cli/test/prompts.test.ts @@ -144,6 +144,7 @@ describe("the CLI invocation variable", () => { "VALE_COMMENT_FORMATS", "VALE_CONVERTER_FORMATS", "VALE_MARKUP_FORMATS", + "VALE_PLAINTEXT_FORMATS", "VALE_VERSION", ]); // INPUT_SCHEMA stays conditional on the placeholder being present. diff --git a/packages/cli/test/recipe-cross-references.test.ts b/packages/cli/test/recipe-cross-references.test.ts index 0a4b4618..4319f727 100644 --- a/packages/cli/test/recipe-cross-references.test.ts +++ b/packages/cli/test/recipe-cross-references.test.ts @@ -9,6 +9,7 @@ import { valeCommentList, valeConverterList, valeMarkupList, + valePlaintextList, } from "../src/rules/capabilities"; import { buildInvocation } from "../src/util/invocation"; @@ -375,9 +376,14 @@ describe("recipes state engine reach from the pinned versions", () => { expect(route).toContain(valeMarkupList()); expect(route).toContain(valeCommentList()); expect(route).toContain(valeConverterList()); + expect(route).toContain(valePlaintextList()); // The consequence, not just the list. A recipe that names `.mdx` without // saying it takes the whole pass down has not conveyed the hazard. expect(route).toContain("E100"); + // MDX is a "not yet", not a "never" — Vale 3.18.0 parses it natively. An + // agent told only that it is unreadable would tell a user MDX is + // unsupported, full stop. + expect(route).toContain("MDX is not supported yet"); }); it("names ast-grep's languages in rendered create-sg-rule.txt", async () => { @@ -407,5 +413,10 @@ describe("recipes state engine reach from the pinned versions", () => { // a matcher. It may still cite it — as the counter-example it now is — so // this pins the warning rather than the absence of the string. expect(recipe).toContain("Never put one of those extensions in a glob."); + expect(recipe).toContain(valePlaintextList()); + expect(recipe).toContain("MDX is not supported yet"); + // No date. The bump is expected, not scheduled, and a recipe that implies + // otherwise is stale the moment it slips. + expect(recipe).not.toMatch(/\b20\d\d-\d\d\b/); }); }); diff --git a/packages/cli/test/vale-formats.test.ts b/packages/cli/test/vale-formats.test.ts new file mode 100644 index 00000000..355407b4 --- /dev/null +++ b/packages/cli/test/vale-formats.test.ts @@ -0,0 +1,357 @@ +import { spawnSync } from "node:child_process"; +import { mkdtempSync, mkdirSync, rmSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; + +import { afterEach, describe, expect, it } from "vitest"; + +import { VALE_FORMAT_TIERS } from "../src/rules/capabilities"; +import { findValeBinary } from "../src/rules/vale/binary"; +import { + buildValeGlob, + CONVERTER_DEPENDENT_EXTENSIONS, + converterExclusionGlobs, + converterFor, + findConverterDependentFiles, + skippedFilesNotice, +} from "../src/rules/vale/formats"; +import { runVale } from "../src/rules/vale/run"; + +/** + * The exclusion derived from the format tiers, and the run that uses it. + * + * WHAT VALE DOES WITH AN EXTENSION IS NOT ASSERTED HERE. `VALE_FORMAT_TIERS` + * lives in `src/rules/capabilities.ts` and every row of it is re-measured + * against the real binary in `vale-vendor-contract.test.ts` ("Vale engine + * capabilities"), each tier by the discriminating property only that tier has. + * This file asserts what our code does *given* those tiers, plus the end-to-end + * behaviour of a run that contains a converter-dependent file. Two files + * probing the same extension with different fixtures is how a weaker probe gets + * to overrule a stronger one, so the probing happens in exactly one of them. + */ + +const binary = findValeBinary().path; +const withVale = binary === undefined ? describe.skip : describe; + +const workspaces: string[] = []; +afterEach(() => { + for (const workspace of workspaces.splice(0)) { + rmSync(workspace, { recursive: true, force: true }); + } +}); + +const existenceRule = + "extends: existence\nmessage: \"Avoid 'simply'\"\nlevel: warning\ntokens:\n - simply\n"; + +/** + * A project whose single rule matches **every** file, which is the scoping that + * makes the crash reachable: Vale only routes a file to a parser when the + * configuration gives it a check to run, so a rule scoped `[*.md]` never asks + * for `asciidoctor` in the first place. + */ +function makeProject(documents: Record): string { + const cwd = mkdtempSync(join(tmpdir(), "vale-formats-")); + workspaces.push(cwd); + mkdirSync(join(cwd, ".taskless", "rules", "vale", "no-simply"), { + recursive: true, + }); + writeFileSync( + join(cwd, ".taskless", "rules", "vale", "no-simply", "no-simply.yml"), + existenceRule + ); + writeFileSync( + join(cwd, ".taskless", ".vale.ini"), + "StylesPath = rules/vale\nMinAlertLevel = suggestion\n\n[*]\nBasedOnStyles =\nno-simply.no-simply = YES\n" + ); + for (const [path, body] of Object.entries(documents)) { + const full = join(cwd, path); + mkdirSync(join(full, ".."), { recursive: true }); + writeFileSync(full, body); + } + return cwd; +} + +describe("the format tier table", () => { + it("derives the exclusion list from the tiers, with nothing hand-written", () => { + // One place, so the merge with `capabilities.ts` is mechanical: adding a + // measured entry to the table is the entire change, and no second list can + // fall behind it. + const expected = Object.entries(VALE_FORMAT_TIERS) + .filter(([, tier]) => tier.startsWith("converter:")) + .map(([extension]) => extension) + .toSorted(); + expect(expected.length).toBeGreaterThan(0); + expect([...CONVERTER_DEPENDENT_EXTENSIONS]).toEqual(expected); + // Every excluded format names the program a user would install. A skip the + // user cannot act on is only marginally better than a silent one. + for (const extension of CONVERTER_DEPENDENT_EXTENSIONS) { + expect(converterFor(`doc${extension}`)).toBeTruthy(); + } + }); + + it("covers all three AsciiDoc spellings", () => { + // `.asc` was not in the bug report and crashes identically. It is here + // because the tiers were measured rather than transcribed, and this case + // is what stops the next transcription from dropping it again. + for (const extension of [".adoc", ".asciidoc", ".asc"]) { + expect(converterFor(`guide${extension}`)).toBe("asciidoctor"); + } + }); + + it("reads the extension case-sensitively, exactly as Vale routes it", () => { + // Measured, not assumed: `doc.adoc` exits 2 with `E100 [lintAdoc]` while + // `doc.ADOC` is read as plain text and exits 0 with findings — even when + // the uppercase spelling names a lowercase file on a case-insensitive + // filesystem, because Vale routes on the path string it was handed. This + // function used to lowercase on the opposite assumption, which named a + // file Vale had linted normally as one it never checked. The vendor + // contract pins the binary's half of this. + expect(converterFor("docs/README.rst")).toBe("rst2html"); + expect(converterFor("docs/README.RST")).toBeUndefined(); + expect(converterFor("guide.ADOC")).toBeUndefined(); + expect(converterFor("guide.AdOc")).toBeUndefined(); + }); + + it("treats an unmeasured extension as needing no converter", () => { + // The safe path and the unknown path are the same path: Vale reads an + // extension it does not recognize as plain text, which cannot shell out. + expect(converterFor("script.py")).toBeUndefined(); + expect(converterFor("Makefile")).toBeUndefined(); + expect(converterFor("notes.md")).toBeUndefined(); + }); + + it("hands over the formats measured as plaintext, converter-free", () => { + // `.tex`, `.rmd`, `.mkd` and `.mkdn` all read as markup and are not — the + // first table to be written by hand put `.tex` and `.rmd` in the native + // tier. Being wrong about the tier is survivable; being wrong about needing + // a converter is not, because it excludes a file Vale would have linted + // perfectly well. This is that half of the claim. + for (const extension of [".tex", ".rmd", ".mkd", ".mkdn", ".typ"]) { + expect(converterFor(`doc${extension}`)).toBeUndefined(); + } + }); +}); + +describe("the exclusion glob", () => { + it("anchors every pattern with **/ so nested files are excluded too", () => { + // Vale matches a `--glob` against the basename when the pattern contains no + // `/`, and against the path when it does. Combined with `.taskless/**` the + // whole expression goes path-wise, and a bare `*.adoc` branch then stops + // matching `docs/guide.adoc` — excluding the file you tested and not the + // one in the next directory down. + for (const pattern of converterExclusionGlobs()) { + expect(pattern.startsWith("**/*.")).toBe(true); + } + }); + + it("emits one negated alternation, because Vale honours only one --glob", () => { + expect(buildValeGlob([".taskless/**", "**/*.adoc"])).toBe( + "--glob=!{.taskless/**,**/*.adoc}" + ); + }); + + it("emits no flag when there is nothing to exclude", () => { + expect(buildValeGlob([])).toBeUndefined(); + }); +}); + +describe("the skipped-files notice", () => { + it("is absent when nothing was skipped", () => { + expect(skippedFilesNotice([])).toBeUndefined(); + }); + + it("names the files and says the format is unsupported, not that a tool is missing", () => { + const notice = + skippedFilesNotice(["docs/guide.adoc", "spec/api.rst"]) ?? ""; + expect(notice).toContain("docs/guide.adoc"); + expect(notice).toContain("spec/api.rst"); + expect(notice).toContain("every other file was checked normally"); + // The programs are still named — they are the REASON, and a user reading + // "asciidoctor" understands what kind of gap this is. + expect(notice).toContain("asciidoctor"); + expect(notice).toContain("rst2html"); + // But the notice must not read as an offer. We do not support any format + // that needs an external program, so telling a user to install one promises + // a path that is untested, and for `.xml` impossible — an XSLT stylesheet is + // specific to the document. It would also make behaviour host-dependent: + // macOS ships /usr/bin/xsltproc and Linux CI images do not, so the same + // repository would check differently per machine. + expect(notice).toContain("not supported by this build"); + expect(notice).not.toMatch(/install/i); + expect(notice).not.toMatch(/\bPATH\b/); + }); + + it("summarizes rather than printing an unbounded file list", () => { + const files = Array.from({ length: 9 }, (_, index) => `d${index}.adoc`); + const notice = skippedFilesNotice(files) ?? ""; + expect(notice).toContain("9 file(s)"); + expect(notice).toContain("and 4 more"); + }); +}); + +describe("finding converter-dependent files", () => { + it("finds them at the root and nested, and ignores everything else", async () => { + const cwd = makeProject({ + "a.md": "simply\n", + "d.adoc": "= T\n", + "docs/deep/f.adoc": "= T\n", + "docs/g.rst": "T\n", + "node_modules/pkg/vendor.adoc": "= T\n", + }); + expect(await findConverterDependentFiles(cwd, [])).toEqual([ + "d.adoc", + "docs/deep/f.adoc", + "docs/g.rst", + ]); + }); + + it("answers an explicitly named file from its own name", async () => { + const cwd = makeProject({ "a.md": "simply\n", "d.adoc": "= T\n" }); + expect(await findConverterDependentFiles(cwd, ["a.md", "d.adoc"])).toEqual([ + "d.adoc", + ]); + }); + + it("does not name an uppercase-extension file Vale lints normally", async () => { + // Node's `glob` folds case with the filesystem, so on macOS the pattern + // matches `docs/GUIDE.ADOC` — but Vale routes that to its plain-text + // reader and lints it. Naming it in the notice would tell the user a file + // was skipped that was checked, on one platform only. The walk defers to + // `converterFor`, which is the only thing that models Vale's routing. + const cwd = makeProject({ + "docs/GUIDE.ADOC": "= T\n", + "docs/real.adoc": "= T\n", + }); + expect(await findConverterDependentFiles(cwd, [])).toEqual([ + "docs/real.adoc", + ]); + expect(await findConverterDependentFiles(cwd, ["docs/GUIDE.ADOC"])).toEqual( + [] + ); + }); +}); + +withVale( + "runVale against the real binary, with converter-dependent files", + () => { + it("still reports every Markdown finding when an AsciiDoc file is present", async () => { + // The bug, as one case. Vale aborts the whole process on the first `E100` + // and writes nothing at all to stdout, so before the exclusion these three + // findings did not arrive late or partially — they never existed. + const cwd = makeProject({ + "a.md": "Just simply do it.\n", + "b.md": "You can simply run it.\n", + "c.md": "You simply go.\n", + "d.adoc": "= Title\n\nJust simply do it.\n", + "docs/nested.adoc": "= Title\n\nsimply\n", + }); + + const outcome = await runVale({ + cwd, + configPath: join(".taskless", ".vale.ini"), + }); + + expect(outcome.status).toBe("ok"); + if (outcome.status !== "ok") return; + expect(outcome.results.map((result) => result.file).toSorted()).toEqual([ + "a.md", + "b.md", + "c.md", + ]); + }); + + it("says which files it skipped, rather than dropping them silently", async () => { + // Silence is the bug. A run that quietly checks less than it was asked to + // is indistinguishable from a clean one, which is exactly how the engine + // got disabled without anyone noticing. + const cwd = makeProject({ + "a.md": "Just simply do it.\n", + "docs/nested.adoc": "= Title\n\nsimply\n", + }); + + const outcome = await runVale({ + cwd, + configPath: join(".taskless", ".vale.ini"), + }); + + expect(outcome.status).toBe("ok"); + if (outcome.status !== "ok") return; + expect(outcome.notice).toContain("docs/nested.adoc"); + expect(outcome.notice).toContain("asciidoctor"); + }); + + it("declines a converter-dependent file even when named explicitly", async () => { + // The one place we override an explicit request. Honouring it does not + // check that file badly — it aborts the process, so the request would cost + // the user the rest of their check. + const cwd = makeProject({ + "a.md": "Just simply do it.\n", + "d.adoc": "= Title\n\nsimply\n", + }); + + const outcome = await runVale({ + cwd, + paths: ["a.md", "d.adoc"], + configPath: join(".taskless", ".vale.ini"), + }); + + expect(outcome.status).toBe("ok"); + if (outcome.status !== "ok") return; + expect(outcome.results.every((result) => result.file === "a.md")).toBe( + true + ); + expect(outcome.results.length).toBeGreaterThan(0); + expect(outcome.notice).toContain("d.adoc"); + }); + + it("keeps out of .taskless/ while excluding converter formats", () => { + // The two exclusions have to travel in one `--glob`, because Vale keeps + // only the last one. This is the case that catches a future edit that adds + // a second flag and silently drops the first. + const cwd = makeProject({ + "a.md": "Just simply do it.\n", + "d.adoc": "= Title\n\nsimply\n", + }); + // The rule's own fixture directory: prose about the machinery, which a + // whole-project run must not report as a user's finding. + mkdirSync( + join(cwd, ".taskless", "rules", "vale", "no-simply", ".tests", "fail"), + { recursive: true } + ); + writeFileSync( + join( + cwd, + ".taskless", + "rules", + "vale", + "no-simply", + ".tests", + "fail", + "hedged.md" + ), + "Just simply do it.\n" + ); + + const result = spawnSync( + binary as string, + [ + "--config", + join(".taskless", ".vale.ini"), + "--output=JSON", + "--no-exit", + buildValeGlob([ + ".taskless/**", + ...converterExclusionGlobs(), + ]) as string, + "--", + ".", + ], + { cwd, encoding: "utf8" } + ); + + expect(result.status).toBe(0); + expect(result.stdout).not.toContain(".taskless"); + expect(result.stdout).toContain("a.md"); + }); + } +); diff --git a/packages/cli/test/vale-orchestration.test.ts b/packages/cli/test/vale-orchestration.test.ts index 80e703aa..122112c2 100644 --- a/packages/cli/test/vale-orchestration.test.ts +++ b/packages/cli/test/vale-orchestration.test.ts @@ -446,3 +446,42 @@ describe("an engine failure under --json", () => { expect(output.notices).toBeUndefined(); }); }); + +withVale("a repository containing a converter-dependent file", () => { + it("still reports its Markdown findings, and says what it skipped", async () => { + // End to end, through the built CLI, because the failure this covers was + // whole-run: one `.adoc` aborted Vale before it serialized anything, so + // every Markdown finding in the project disappeared while `check` still + // exited non-zero for an unrelated ast-grep finding — a dead engine + // wearing a normal red check. + const cwd = makeMixedProject(); + // Widen the rule past `[*.md]`. Vale only routes a file to a parser when + // the configuration gives it a check to run, so the crash is unreachable + // while every matcher is Markdown-only. + writeFileSync( + join(cwd, ".taskless", "rules", "vale", "no-simply", ".vale.ini"), + "[*]\ntskl) rule = no-simply\nBasedOnStyles =\nno-simply.no-simply = YES\n" + ); + writeFileSync(join(cwd, "guide.adoc"), "= Guide\n\nJust simply do it.\n"); + mkdirSync(join(cwd, "docs"), { recursive: true }); + writeFileSync(join(cwd, "docs", "api.rst"), "API\n===\n\nsimply\n"); + + const { stdout } = await runCli(["check", "-d", cwd, "--json"]); + const output = parseJson(stdout); + + // The findings survive... + expect( + output.results.some( + (result) => (result as { source?: string }).source === "vale" + ) + ).toBe(true); + // ...and it is a skip, not an engine failure. + expect(output.failures).toBeUndefined(); + // ...and the skip is said out loud, naming both the file and the fix. + const notices = (output.notices ?? []).join("\n"); + expect(notices).toContain("guide.adoc"); + expect(notices).toContain("docs/api.rst"); + expect(notices).toContain("asciidoctor"); + expect(notices).toContain("rst2html"); + }); +}); diff --git a/packages/cli/test/vale-vendor-contract.test.ts b/packages/cli/test/vale-vendor-contract.test.ts index b9f88256..9079c2a8 100644 --- a/packages/cli/test/vale-vendor-contract.test.ts +++ b/packages/cli/test/vale-vendor-contract.test.ts @@ -11,11 +11,14 @@ import { VALE_CONVERTER_CHECKERS, VALE_CONVERTER_DEPENDENT, VALE_CONVERTER_DEPENDENT_EXTENSIONS, + VALE_FORMAT_TIERS, VALE_MARKUP_EXTENSIONS, + VALE_PLAINTEXT_EXTENSIONS, VALE_VERSION, valeCommentList, valeConverterList, valeMarkupList, + valePlaintextList, } from "../src/rules/capabilities"; /** @@ -382,18 +385,31 @@ withVale("Vale vendor contract", () => { * - **comment-only** is separated from plaintext by the NEGATIVE — the same * token on a bare non-comment line must yield nothing. A file type that fires * on both is the plaintext fallback wearing a code extension. + * - **plaintext** is the tier that needs no separating — a bare line fires — + * but the entries listed in it do: each names a construct a parser WOULD have + * skipped, and the probe asserts Vale lints it. That is the assertion that + * `.tex` and `.rmd` are not markup, and it is the one the first hand-written + * table got backwards. * - **converter-dependent** is separated from everything by failing. * * See taskless/cli#151. */ /** The comment syntax Vale must see through, per extension. */ +/** + * A correct comment for `extension`, which is the whole difficulty of this + * file: a wrong delimiter reads exactly like absent support. `.css` looks + * unsupported if fed `//` and is comment-aware with a block comment; `.pod` is + * linted as Perl, so a `#` comment fires while a `=head1` POD block does not. + */ function comment(extension: string): string { const HASH = new Set([ ".jl", ".pl", ".pm", ".ps1", + ".pod", ".py", + ".py3", ".pyw", ".r", ".R", @@ -441,6 +457,10 @@ withVale("Vale engine capabilities", () => { prose: "We simply do it.\n", skipped: "Fine.\n\n```\nsimply\n```\n", }, + ".mdown": { + prose: "We simply do it.\n", + skipped: "Fine.\n\n```\nsimply\n```\n", + }, ".org": { prose: "We simply do it.\n", skipped: "# simply\nFine.\n" }, ".htm": { prose: "

We simply do it.

\n", @@ -459,14 +479,32 @@ withVale("Vale engine capabilities", () => { it("reports the pinned version", () => { // VALE_VERSION is rendered beside the reach lists in route.txt and // create-vale-rule.txt, so it is the attribution for every claim below. - // It also gates one of them: Vale 3.18.0 parses MDX natively, and a bump - // past it must move `.mdx` out of VALE_CONVERTER_DEPENDENT. + // It also gates two of them, in opposite directions: Vale 3.18.0 parses MDX + // natively, so a bump past it moves `.mdx` from converter-dependent to + // markup — and the same release adds a Typst converter, so it moves `.typ` + // from plaintext to `converter:typst2vast`. A bump re-measures the whole + // table; these two are only the rows already known to move. const result = spawnSync(binary as string, ["--version"], { encoding: "utf8", }); expect(result.stdout.trim()).toBe(`vale version ${VALE_VERSION}`); }); + it("probes every row of VALE_FORMAT_TIERS", () => { + // The table is the claim and this is its coverage check: every extension in + // it is reached by one of the tier suites below, so a row cannot be added + // without being measured. Reconciling two independently written tables + // turned up six rows that disagreed, every one of them in a tier nothing + // probed. + const probed = [ + ...VALE_MARKUP_EXTENSIONS, + ...VALE_COMMENT_EXTENSIONS, + ...VALE_PLAINTEXT_EXTENSIONS, + ...VALE_CONVERTER_DEPENDENT.flatMap(({ extensions }) => extensions), + ].toSorted(); + expect(probed).toEqual(Object.keys(VALE_FORMAT_TIERS).toSorted()); + }); + it("covers every markup extension in VALE_MARKUP_EXTENSIONS", () => { // A fixture missing here would let an extension be added to the constant // without ever being probed, which is the drift the constant exists to @@ -502,6 +540,52 @@ withVale("Vale engine capabilities", () => { } ); + /** + * For each listed plaintext extension, the construct a parser for the format + * its spelling suggests would have skipped. + * + * Vale lints it, which is the whole finding: `.tex` is not TeX to Vale and + * `.rmd` is not R Markdown. The mirror image of the markup fixtures — same + * documents, opposite expectation. + */ + const PLAINTEXT_FIXTURES: Record = { + ".mkd": "Fine.\n\n```\nsimply\n```\n", + ".mkdn": "Fine.\n\n```\nsimply\n```\n", + ".rmd": "Fine.\n\n```{r}\nsimply <- 1\n```\n", + ".tex": "% simply in a comment\nFine.\n", + ".typ": "// simply\nFine.\n", + // Documented by Vale as comment-aware, measured as plaintext on the pinned + // binary — the construct here is the comment a parser would have skipped. + ".pyi": "# simply\nFine.\n", + ".qml": "// simply\nFine.\n", + ".scss": "// simply\nFine.\n", + }; + + it("covers every extension in VALE_PLAINTEXT_EXTENSIONS", () => { + expect(Object.keys(PLAINTEXT_FIXTURES).toSorted()).toEqual( + [...VALE_PLAINTEXT_EXTENSIONS].toSorted() + ); + }); + + it.each([...VALE_PLAINTEXT_EXTENSIONS])( + "reads %s as plaintext, lint-through-syntax and all", + (extension) => { + // A bare line fires: the tier of last resort has no syntax to hide behind. + expect( + findings(`bare${extension}`, "simply\n"), + `${extension} ignored a bare line — it has a parser, and is not plaintext` + ).toHaveLength(1); + // And the format's own non-prose syntax fires too, which is what makes it + // plaintext rather than markup. `.mkdn` and `.mkd` are the cautionary + // pair: they read as Markdown spellings and Vale lints straight through a + // fenced code block in both. + expect( + findings(`syntax${extension}`, PLAINTEXT_FIXTURES[extension]!), + `${extension} skipped its own syntax — it is parsed, and belongs in a markup or comment tier` + ).toHaveLength(1); + } + ); + it("lints an unrecognized extension as whole-file prose", () => { // The fallback, stated as an assertion because it is a routing hazard // rather than a convenience: `.yml` has no parser, so a Vale rule scoped to @@ -541,6 +625,28 @@ withVale("Vale engine capabilities", () => { expect(output).toContain(`[${VALE_CONVERTER_CHECKERS[extension]!}]`); }); + it("routes to a converter by EXACT-case extension", () => { + // The half of the case question only the binary can answer, and it is not + // the intuitive one: Vale routes on the extension exactly as spelled, so + // an uppercase converter-dependent extension falls through to the + // plain-text reader instead of crashing. `converterFor` matches this + // exactly, which is why it no longer lowercases — a lowercasing lookup + // named files in the skip notice that Vale had linted normally. + // + // Pinned rather than assumed because the direction matters in both ways. + // If a future Vale becomes case-insensitive, `doc.ADOC` starts exiting + // non-zero, this case goes red, and the lowercasing has to come back + // before the crash reaches a user's run. + for (const extension of VALE_CONVERTER_DEPENDENT_EXTENSIONS) { + const upper = extension.toUpperCase(); + const name = `doc${upper}`; + const cwd = anyExtension({ [name]: "We simply do it.\n" }); + const result = runRaw(cwd, [name], ["--no-exit"]); + expect(result.status, `${upper} now needs a converter`).toBe(0); + expect(`${result.stdout}${result.stderr}`).not.toContain("E100"); + } + }); + it("names a checker for every converter-dependent extension", () => { // Set-equality, so an extension added to one and not the other fails here // rather than throwing on an undefined tag inside the probe above. @@ -563,6 +669,9 @@ withVale("Vale engine capabilities", () => { it("renders each list as recipe prose with no gaps", () => { expect(valeMarkupList().split(", ")).toEqual([...VALE_MARKUP_EXTENSIONS]); expect(valeCommentList().split(", ")).toEqual([...VALE_COMMENT_EXTENSIONS]); + expect(valePlaintextList().split(", ")).toEqual([ + ...VALE_PLAINTEXT_EXTENSIONS, + ]); for (const { extensions, converter } of VALE_CONVERTER_DEPENDENT) { expect(valeConverterList()).toContain( `${extensions.join("/")} (needs ${converter})`