Skip to content

Commit 72b9834

Browse files
theCodeDriftclaude
andcommitted
fix(cli): match Vale's extension routing case for case, as measured
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
1 parent 2e71df7 commit 72b9834

3 files changed

Lines changed: 85 additions & 14 deletions

File tree

‎packages/cli/src/rules/vale/formats.ts‎

Lines changed: 33 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -106,12 +106,24 @@ export const CONVERTER_DEPENDENT_EXTENSIONS: readonly string[] = [
106106
/**
107107
* The converter Vale would need for `path`, or `undefined` if it needs none.
108108
*
109-
* Extension comparison is lowercased: `README.RST` is a reStructuredText file
110-
* to Vale on a case-insensitive filesystem, and letting case decide would make
111-
* the crash reappear on exactly one platform.
109+
* Extension comparison is **case-sensitive, because Vale's own routing is**.
110+
* Measured against the pinned binary: `docs/guide.adoc` exits 2 with
111+
* `E100 [lintAdoc]`, while `docs/guide.ADOC` and `docs/guide.AdOc` are read as
112+
* plain text and exit 0 with findings — including when the uppercase spelling
113+
* names a lowercase file on a case-insensitive filesystem, since Vale routes on
114+
* the path string it was handed, not on the name the disk holds.
115+
*
116+
* This used to lowercase, on the assumption that a case-insensitive filesystem
117+
* would make `README.RST` reStructuredText to Vale. It does not, and the
118+
* assumption cost accuracy in the one place this function is read: a file Vale
119+
* had linted perfectly well was named in the skip notice as one it never
120+
* checked. Matching Vale exactly is what keeps the notice true, and it is the
121+
* same discipline as the tier table — measure the binary, do not reason about
122+
* it. `vale-vendor-contract.test.ts` pins the measurement, so a Vale that
123+
* becomes case-insensitive turns red here before it can crash a user's run.
112124
*/
113125
export function converterFor(path: string): string | undefined {
114-
return VALE_CONVERTER_BY_EXTENSION[extname(path).toLowerCase()];
126+
return VALE_CONVERTER_BY_EXTENSION[extname(path)];
115127
}
116128

117129
/**
@@ -173,6 +185,14 @@ const UNWALKED_DIRECTORIES = new Set([
173185
* into dot-directories, so a `.github/adr/0001.adoc` is skipped by Vale and
174186
* goes unnamed here. That under-reports a notice; it never suppresses a
175187
* finding, and it never lets the crash back in.
188+
*
189+
* Every match is re-checked through `converterFor` before it is kept, and that
190+
* is not belt-and-braces. Node's `glob` inherits the filesystem's own case
191+
* folding, so on macOS `**\/*.{adoc,…}` matches `docs/GUIDE.ADOC` — a file Vale
192+
* routes to its plain-text reader and lints normally. Left unfiltered, the
193+
* notice would claim a checked file was skipped, and it would claim it on
194+
* exactly one platform. `converterFor` is the one place that knows how Vale
195+
* routes, so the walk defers to it rather than trusting the pattern.
176196
*/
177197
export async function findConverterDependentFiles(
178198
cwd: string,
@@ -203,7 +223,7 @@ export async function findConverterDependentFiles(
203223
cwd,
204224
exclude: (entry) => UNWALKED_DIRECTORIES.has(basename(String(entry))),
205225
})) {
206-
found.add(match);
226+
if (converterFor(match) !== undefined) found.add(match);
207227
}
208228
} catch {
209229
// A target that is not a directory, an unreadable subtree, a platform
@@ -219,11 +239,15 @@ export async function findConverterDependentFiles(
219239
* The user-facing sentence for a set of skipped files, or `undefined` when
220240
* nothing was skipped.
221241
*
222-
* Phrased as "this build cannot parse", not "Vale does not support": Vale
242+
* Phrased as "not supported by this build", not "Vale does not support": Vale
223243
* supports every one of these formats, and telling a user otherwise sends them
224-
* to the wrong project's issue tracker. It names the converter because that is
225-
* the one thing they can act on — installing `asciidoctor` puts the files back
226-
* in scope with no change on our side.
244+
* to the wrong project's issue tracker.
245+
*
246+
* It names the converter as an *explanation*, never as an instruction. A format
247+
* that needs an external program is not supported here, full stop — so the
248+
* sentence must not read as "install `asciidoctor` and this will work", because
249+
* the exclusion is unconditional and does not consult the host. The action
250+
* offered is the one that actually works: scope the rule to a supported format.
227251
*/
228252
export function skippedFilesNotice(files: string[]): string | undefined {
229253
if (files.length === 0) return undefined;

‎packages/cli/test/vale-formats.test.ts‎

Lines changed: 30 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -98,11 +98,18 @@ describe("the format tier table", () => {
9898
}
9999
});
100100

101-
it("reads the extension case-insensitively", () => {
102-
// A case-insensitive filesystem hands Vale `README.RST` as
103-
// reStructuredText. Letting case decide would put the crash back on macOS
104-
// and Windows only.
105-
expect(converterFor("docs/README.RST")).toBe("rst2html");
101+
it("reads the extension case-sensitively, exactly as Vale routes it", () => {
102+
// Measured, not assumed: `doc.adoc` exits 2 with `E100 [lintAdoc]` while
103+
// `doc.ADOC` is read as plain text and exits 0 with findings — even when
104+
// the uppercase spelling names a lowercase file on a case-insensitive
105+
// filesystem, because Vale routes on the path string it was handed. This
106+
// function used to lowercase on the opposite assumption, which named a
107+
// file Vale had linted normally as one it never checked. The vendor
108+
// contract pins the binary's half of this.
109+
expect(converterFor("docs/README.rst")).toBe("rst2html");
110+
expect(converterFor("docs/README.RST")).toBeUndefined();
111+
expect(converterFor("guide.ADOC")).toBeUndefined();
112+
expect(converterFor("guide.AdOc")).toBeUndefined();
106113
});
107114

108115
it("treats an unmeasured extension as needing no converter", () => {
@@ -204,6 +211,24 @@ describe("finding converter-dependent files", () => {
204211
"d.adoc",
205212
]);
206213
});
214+
215+
it("does not name an uppercase-extension file Vale lints normally", async () => {
216+
// Node's `glob` folds case with the filesystem, so on macOS the pattern
217+
// matches `docs/GUIDE.ADOC` — but Vale routes that to its plain-text
218+
// reader and lints it. Naming it in the notice would tell the user a file
219+
// was skipped that was checked, on one platform only. The walk defers to
220+
// `converterFor`, which is the only thing that models Vale's routing.
221+
const cwd = makeProject({
222+
"docs/GUIDE.ADOC": "= T\n",
223+
"docs/real.adoc": "= T\n",
224+
});
225+
expect(await findConverterDependentFiles(cwd, [])).toEqual([
226+
"docs/real.adoc",
227+
]);
228+
expect(await findConverterDependentFiles(cwd, ["docs/GUIDE.ADOC"])).toEqual(
229+
[]
230+
);
231+
});
207232
});
208233

209234
withVale(

‎packages/cli/test/vale-vendor-contract.test.ts‎

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -625,6 +625,28 @@ withVale("Vale engine capabilities", () => {
625625
expect(output).toContain(`[${VALE_CONVERTER_CHECKERS[extension]!}]`);
626626
});
627627

628+
it("routes to a converter by EXACT-case extension", () => {
629+
// The half of the case question only the binary can answer, and it is not
630+
// the intuitive one: Vale routes on the extension exactly as spelled, so
631+
// an uppercase converter-dependent extension falls through to the
632+
// plain-text reader instead of crashing. `converterFor` matches this
633+
// exactly, which is why it no longer lowercases — a lowercasing lookup
634+
// named files in the skip notice that Vale had linted normally.
635+
//
636+
// Pinned rather than assumed because the direction matters in both ways.
637+
// If a future Vale becomes case-insensitive, `doc.ADOC` starts exiting
638+
// non-zero, this case goes red, and the lowercasing has to come back
639+
// before the crash reaches a user's run.
640+
for (const extension of VALE_CONVERTER_DEPENDENT_EXTENSIONS) {
641+
const upper = extension.toUpperCase();
642+
const name = `doc${upper}`;
643+
const cwd = anyExtension({ [name]: "We simply do it.\n" });
644+
const result = runRaw(cwd, [name], ["--no-exit"]);
645+
expect(result.status, `${upper} now needs a converter`).toBe(0);
646+
expect(`${result.stdout}${result.stderr}`).not.toContain("E100");
647+
}
648+
});
649+
628650
it("names a checker for every converter-dependent extension", () => {
629651
// Set-equality, so an extension added to one and not the other fails here
630652
// rather than throwing on an undefined tag inside the probe above.

0 commit comments

Comments
 (0)