From e5a06f1063d952b64120ee65f5ea6f088ef7925a Mon Sep 17 00:00:00 2001 From: Jakob Heuser Date: Mon, 7 Sep 2026 23:24:44 -0700 Subject: [PATCH 1/3] fix(cli): one bad file costs one finding, not the whole Vale run MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A single markdown file with unparseable YAML front matter aborted the entire Vale invocation before any result was written, so `check` reported `results: []` for the whole run regardless of how many other files had findings — indistinguishable from a genuinely clean pass. runVale now retries: when Vale's own config-error payload attributes a failure to one of the run's target files (as opposed to a rule config it loaded through StylesPath, which always reports an absolute path), that file is excluded and reported as a per-file finding (ruleId: "vale-parse-error", severity: "error"), and the run continues over everything else. A failure that cannot be attributed to a single target file — a malformed rule, a timeout, a crash — still fails the run exactly as before. No YAML parser was added: Vale's own error object already names the file and the reason it could not be parsed, so the fix reuses that rather than re-deriving it with a second parser. --- .changeset/one-bad-file-zeroes-the-run.md | 16 + packages/cli/src/rules/vale/run.ts | 500 +++++++++++++------ packages/cli/test/mixed-engine-check.test.ts | 71 +++ packages/cli/test/vale-run.test.ts | 134 +++++ 4 files changed, 575 insertions(+), 146 deletions(-) create mode 100644 .changeset/one-bad-file-zeroes-the-run.md diff --git a/.changeset/one-bad-file-zeroes-the-run.md b/.changeset/one-bad-file-zeroes-the-run.md new file mode 100644 index 00000000..0e132690 --- /dev/null +++ b/.changeset/one-bad-file-zeroes-the-run.md @@ -0,0 +1,16 @@ +--- +"@taskless/cli": patch +--- + +`check` no longer loses every finding in a run because one file's front +matter could not be parsed. A Vale front-matter error used to abort the +entire Vale invocation before any result was written, so `results` came back +`[]` for the whole run regardless of how many other files had findings — and +`[]` was indistinguishable from a genuinely clean pass. + +`runVale` now retries around a file Vale's own error attributes to one of the +run's targets, excluding it and reporting it as a per-file finding +(`ruleId: "vale-parse-error"`, `severity: "error"`) instead of failing the +whole run. Every other file's findings are reported normally. A failure Vale +does not attribute to a single target file — a malformed rule, a timeout, a +crash — is unaffected and still fails the run exactly as before. diff --git a/packages/cli/src/rules/vale/run.ts b/packages/cli/src/rules/vale/run.ts index 3add0592..16c24c14 100644 --- a/packages/cli/src/rules/vale/run.ts +++ b/packages/cli/src/rules/vale/run.ts @@ -1,5 +1,6 @@ import { spawn } from "node:child_process"; -import { join } from "node:path"; +import { stat } from "node:fs/promises"; +import { isAbsolute, join, resolve as resolvePath } from "node:path"; import { StringDecoder } from "node:string_decoder"; import type { CheckResult } from "../../types/check"; @@ -20,7 +21,12 @@ import { skippedFilesNotice, TASKLESS_DIRECTORY, } from "./formats"; -import { asValeConfigError, toValeCheckResults, type ValeOutput } from "./map"; +import { + asValeConfigError, + toValeCheckResults, + type ValeConfigError, + type ValeOutput, +} from "./map"; /** * The Vale config a run reads, relative to the project root. @@ -116,7 +122,23 @@ function describeValeStderr(stderr: string): string { } const error = asValeConfigError(parsed); if (error === undefined) return stderr; + return formatValeConfigError(error, { withPath: true }); +} +/** + * Render a {@link ValeConfigError} as a sentence, shared by the whole-run + * failure message ({@link describeValeStderr}) and the per-file finding + * {@link parseErrorResult} builds for one excluded file. + * + * `withPath` exists because the two callers already say the file a different + * way: the whole-run message has nowhere else to put it, so it is appended + * here; a per-file finding already carries the file on `CheckResult.file`, and + * repeating it inside `message` would be the same fact twice. + */ +function formatValeConfigError( + error: ValeConfigError, + options: { withPath: boolean } +): string { const text = error.Text.split("\n") .map((line) => line.trim()) .filter((line) => line !== "") @@ -126,153 +148,133 @@ function describeValeStderr(stderr: string): string { // 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}` - }`; + const path = + !options.withPath || error.Path === undefined || error.Path === "" + ? "" + : ` in ${error.Path}`; + return `${code}${text}${path}`; } -export interface ValeRunOptions { - /** Project root. Vale runs here, so the config's relative paths resolve. */ - cwd: string; - /** Target paths, relative to `cwd`. Empty means Vale's own default set. */ - paths?: string[]; - /** Config path relative to `cwd`. Defaults to the assembled run config. */ - configPath?: string; - timeoutMs?: number; +/** + * The rule id a per-file parse failure is filed under. + * + * Not one of Vale's own checks — no style produced this finding, Vale never + * finished parsing the file well enough to run one — but `CheckResult.ruleId` + * has no slot for "no rule ran here, the file itself could not be read". + * Namespaced so it reads as Vale's own report rather than a style violation, + * and so a caller filtering by rule id can tell the two apart. + */ +const PARSE_ERROR_RULE_ID = "vale-parse-error"; + +/** + * One file Vale could not parse, reported as a finding rather than aborting + * the whole run. + * + * This is the fix for taskless/cli#300. Vale's own config-error payload + * already names the file and the reason it failed to parse — that is what + * {@link targetFileParseError} keys off of — so this only has to shape that + * same information into the scanner-agnostic {@link CheckResult}, the same way + * {@link toValeCheckResult} shapes an ordinary finding. `severity: "error"` + * is deliberate: a file that could not be checked at all is not a clean pass, + * and reporting it as anything softer would let it read as one. + */ +function parseErrorResult(file: string, error: ValeConfigError): CheckResult { + const line = Math.max(0, (error.Line ?? 1) - 1); + return { + source: "vale", + ruleId: PARSE_ERROR_RULE_ID, + severity: "error", + message: `Vale could not check this file: ${formatValeConfigError(error, { withPath: false })}`, + file, + range: { + start: { line, column: 0 }, + end: { line, column: 0 }, + }, + matchedText: "", + }; } /** - * Run Vale over `paths` using the assembled run config, and map what it reports. + * Whether a Vale config-error names one of THIS RUN's target files — as + * opposed to a rule file of ours, or a style Vale loaded through + * `StylesPath`. * - * `--no-exit` is what makes the exit code readable: without it Vale exits - * non-zero merely because it found something, which is indistinguishable from - * failing to run. With it, a non-zero exit means Vale itself failed. + * The two are told apart by nothing more than what Vale's own error already + * says, so this needs no YAML parser of its own to re-derive the distinction + * (see the "Verify Build Output In The Build, Not By Parsing It" reasoning in + * `STYLEGUIDE-CODE.md`, which extends to any generator or tool that has + * already answered a question a second parser would only re-guess at). Vale + * itself already parsed the file — that is why it is complaining — and its + * error object already carries exactly which file and why. * - * The config is assembled from each rule's own `.vale.ini` rather than read - * from one committed file. The per-rule configs remain the source of truth for - * scoping, so the matchers a user edits are exactly the matchers that execute — - * assembly concatenates them in a deterministic order and adds nothing. + * `assembleValeConfig` always writes `StylesPath` as an ABSOLUTE path (see + * `stylesPath` in `verify.ts`, and `valeHeader` in `assemble.ts`), so a + * problem Vale finds while loading a rule through that path reports an + * absolute `Path`. A target file, by contrast, is named on Vale's command + * line exactly as this module passed it — always relative to `cwd`, per + * `targets` below — so a problem reading a target file reports the relative + * path we asked Vale to check. Measured against the real binary: a bad + * `level:` in a rule file reports that rule's absolute path on disk; an + * unquoted colon in a document's front matter reports the relative path this + * module handed to Vale. + * + * The existence check is defensive, not load-bearing: if it is ever wrong for + * a real target file, the failure mode is "this file could not be excluded, + * the run reports the ordinary blocking failure" — never a bad exclusion. */ -export async function runVale( - options: ValeRunOptions -): Promise { - const { path: binary, tried } = findValeBinary(); - if (binary === undefined) { - return { - status: "unavailable", - blocking: false, - message: valeUnavailableMessage(tried), - }; +async function targetFileParseError( + error: ValeConfigError, + cwd: string +): Promise { + const path = error.Path; + if (path === undefined || path === "" || isAbsolute(path)) { + return undefined; } + if ( + path === TASKLESS_DIRECTORY || + path.startsWith(`${TASKLESS_DIRECTORY}/`) + ) { + return undefined; + } + try { + const stats = await stat(resolvePath(cwd, path)); + if (!stats.isFile()) return undefined; + } catch { + return undefined; + } + return path; +} - const configPath = options.configPath ?? ASSEMBLED_VALE_CONFIG; - const paths = options.paths ?? []; - const timeoutMs = options.timeoutMs ?? VALE_TIMEOUT_MS; - - // Vale needs somewhere to look. Given no input it prints its usage text and - // exits 0, which reaches the mapper as "not JSON" and reports the engine as - // failed on every run — so a whole-project `check`, which passes no paths at - // all, produced zero Vale findings and one spurious failure. ast-grep is the - // reason this is easy to miss: it takes its targets from the config and is - // content with none, so the two engines disagree about what "no paths" means. - // `cwd` is the project root, so `.` is the whole project. - // - // `isWholeProjectWalk` rather than `paths.length === 0`: `check .` arrives - // here with `paths = ["."]`, which a length test reads as a user-named path - // and so skips the `.taskless/` exclusion below. Vale reads hidden - // directories by default, so that route reported prose findings inside - // `.taskless/` on any `check .`, independently of the ast-grep fix in this - // change. Same defect, same signal, one line apart. - const wholeProject = isWholeProjectWalk(paths); - const targets = wholeProject ? ["."] : paths; - - // 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. - // - // `.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. - // - // 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. - // - // What git ignores, on a whole-project walk only — same terms as - // `.taskless/**` above, and for a sharper version of the same reason. Vale - // has no notion of a VCS: it walked into build output, vendored trees, and a - // git worktree at `worktrees//`, which is a complete second checkout, - // so every prose rule fired again against another branch's documents - // (taskless/cli#166). ast-grep needed no equivalent — its walker honors - // `.gitignore` already — which is precisely why the two engines disagreed - // about which files the project contains. Asked only when we chose `.` - // ourselves, so `check worktrees/probe` still checks what it was handed. - // - // Started together with the converter-dependent scan below, because neither - // answer feeds the other: the ignore list shapes the `--glob` argument, the - // scan shapes the skipped-files notice, and only the `.filter` further down - // ever brings the two together. Awaited in series they would charge every - // whole-project run a git subprocess and then a directory walk, back to - // back, before the Vale subprocess has even been spawned. - // - // The notice half of that pair is 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. - // - // The ignored paths are filtered back out for the same reason the notice - // exists at all: it must describe the run that happened. An `.adoc` inside - // `worktrees/` is not a file this run declined to convert, it is a file this - // run was never going to look at, and naming it would send the reader to - // investigate a directory the fix above deliberately excluded. - const [ignoredEntries, converterDependent] = await Promise.all([ - wholeProject ? listGitIgnoredEntries(options.cwd) : [], - findConverterDependentFiles(options.cwd, paths), - ]); - - const exclude = [ - ...(wholeProject - ? [ - `${TASKLESS_DIRECTORY}/**`, - ...gitIgnoredExclusionGlobs(ignoredEntries), - ] - : []), - ...converterExclusionGlobs(), - ]; - const globArgument = buildValeGlob(exclude); - const globFlags = globArgument === undefined ? [] : [globArgument]; - - const skipped = skippedFilesNotice( - converterDependent.filter((file) => !isGitIgnoredPath(file, ignoredEntries)) - ); - - // `--` separates flags from positional paths, so a path beginning with `-` - // is not read as a flag. - const argv = [ - "--config", - configPath, - "--output=JSON", - "--no-exit", - ...globFlags, - "--", - ...targets, - ]; +/** One Vale invocation's outcome, before the retry loop in {@link runVale} + * decides what to do about it. + * + * A narrower shape than {@link ValeRunOutcome}: `unavailable` cannot happen + * here (the caller already checked for a binary before ever attempting a run), + * and a `failed` attempt carries the parsed {@link ValeConfigError} when Vale's + * failure had that shape, so the retry loop can ask {@link targetFileParseError} + * whether this attempt can be narrowed and tried again — without re-parsing the + * message it also carries. + */ +type ValeAttempt = + | { status: "ok"; results: CheckResult[]; notice?: string } + | { status: "timeout"; message: string } + | { status: "failed"; message: string; configError?: ValeConfigError }; - return new Promise((resolve) => { +/** + * Spawn Vale once and map what it reports. Extracted from {@link runVale} so + * the retry loop there can call it again with a wider exclusion glob after + * dropping one file that could not be parsed. + */ +async function spawnVale( + binary: string, + argv: string[], + cwd: string, + timeoutMs: number, + skipped: string | undefined +): Promise { + return new Promise((settlePromise) => { const child = spawn(binary, argv, { - cwd: options.cwd, + cwd, stdio: ["ignore", "pipe", "pipe"], env: { ...process.env, PATH: buildPath() }, }); @@ -292,18 +294,17 @@ export async function runVale( let settled = false; /** Resolve once. A timeout kill also fires `close`, which must not win. */ - const settle = (outcome: ValeRunOutcome): void => { + const settle = (outcome: ValeAttempt): void => { if (settled) return; settled = true; clearTimeout(timer); - resolve(outcome); + settlePromise(outcome); }; const timer = setTimeout(() => { child.kill("SIGKILL"); settle({ status: "timeout", - blocking: true, message: `Vale exceeded ${String(timeoutMs)}ms and was terminated. The Vale engine reported a timeout; other engines were unaffected.`, }); }, timeoutMs); @@ -327,7 +328,6 @@ export async function runVale( // pass. `runAstGrepScan` rejects outright on the same event. settle({ status: "failed", - blocking: true, message: `Vale could not be executed at ${binary}: ${error.message}`, }); }); @@ -342,12 +342,18 @@ export async function runVale( // With --no-exit, a non-zero code is Vale failing, not Vale finding. if (code !== null && code !== 0) { const stderr = stderrChunks.join("").trim(); + let configError: ValeConfigError | undefined; + try { + configError = asValeConfigError(JSON.parse(stderr)); + } catch { + configError = undefined; + } settle({ status: "failed", - blocking: true, message: `Vale exited ${String(code)}${ stderr === "" ? "" : `: ${describeValeStderr(stderr)}` }`, + ...(configError === undefined ? {} : { configError }), }); return; } @@ -376,7 +382,7 @@ export async function runVale( // maps to [] below. This branch is for a Vale that says nothing at all // — cheap insurance against JSON.parse("") reporting a clean run as a // failure. - settle({ status: "ok", blocking: false, results: [], ...notice }); + settle({ status: "ok", results: [], ...notice }); return; } @@ -394,24 +400,22 @@ export async function runVale( if (configError !== undefined) { settle({ status: "failed", - blocking: true, message: `Vale rejected the configuration (${configError.Code}): ${configError.Text}${ configError.Path === undefined ? "" : ` in ${configError.Path}` }`, + configError, }); return; } settle({ status: "ok", - blocking: false, results: toValeCheckResults(parsed as ValeOutput), ...notice, }); } catch (error) { settle({ status: "failed", - blocking: true, message: `Vale produced output that is not JSON: ${ error instanceof Error ? error.message : String(error) }`, @@ -421,6 +425,210 @@ export async function runVale( }); } +export interface ValeRunOptions { + /** Project root. Vale runs here, so the config's relative paths resolve. */ + cwd: string; + /** Target paths, relative to `cwd`. Empty means Vale's own default set. */ + paths?: string[]; + /** Config path relative to `cwd`. Defaults to the assembled run config. */ + configPath?: string; + timeoutMs?: number; +} + +/** + * Run Vale over `paths` using the assembled run config, and map what it reports. + * + * `--no-exit` is what makes the exit code readable: without it Vale exits + * non-zero merely because it found something, which is indistinguishable from + * failing to run. With it, a non-zero exit means Vale itself failed. + * + * The config is assembled from each rule's own `.vale.ini` rather than read + * from one committed file. The per-rule configs remain the source of truth for + * scoping, so the matchers a user edits are exactly the matchers that execute — + * assembly concatenates them in a deterministic order and adds nothing. + */ +export async function runVale( + options: ValeRunOptions +): Promise { + const { path: binary, tried } = findValeBinary(); + if (binary === undefined) { + return { + status: "unavailable", + blocking: false, + message: valeUnavailableMessage(tried), + }; + } + + const configPath = options.configPath ?? ASSEMBLED_VALE_CONFIG; + const paths = options.paths ?? []; + const timeoutMs = options.timeoutMs ?? VALE_TIMEOUT_MS; + + // Vale needs somewhere to look. Given no input it prints its usage text and + // exits 0, which reaches the mapper as "not JSON" and reports the engine as + // failed on every run — so a whole-project `check`, which passes no paths at + // all, produced zero Vale findings and one spurious failure. ast-grep is the + // reason this is easy to miss: it takes its targets from the config and is + // content with none, so the two engines disagree about what "no paths" means. + // `cwd` is the project root, so `.` is the whole project. + // + // `isWholeProjectWalk` rather than `paths.length === 0`: `check .` arrives + // here with `paths = ["."]`, which a length test reads as a user-named path + // and so skips the `.taskless/` exclusion below. Vale reads hidden + // directories by default, so that route reported prose findings inside + // `.taskless/` on any `check .`, independently of the ast-grep fix in this + // change. Same defect, same signal, one line apart. + const wholeProject = isWholeProjectWalk(paths); + const targets = wholeProject ? ["."] : paths; + + // 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. + // + // `.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. + // + // 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. + // + // What git ignores, on a whole-project walk only — same terms as + // `.taskless/**` above, and for a sharper version of the same reason. Vale + // has no notion of a VCS: it walked into build output, vendored trees, and a + // git worktree at `worktrees//`, which is a complete second checkout, + // so every prose rule fired again against another branch's documents + // (taskless/cli#166). ast-grep needed no equivalent — its walker honors + // `.gitignore` already — which is precisely why the two engines disagreed + // about which files the project contains. Asked only when we chose `.` + // ourselves, so `check worktrees/probe` still checks what it was handed. + // + // Started together with the converter-dependent scan below, because neither + // answer feeds the other: the ignore list shapes the `--glob` argument, the + // scan shapes the skipped-files notice, and only the `.filter` further down + // ever brings the two together. Awaited in series they would charge every + // whole-project run a git subprocess and then a directory walk, back to + // back, before the Vale subprocess has even been spawned. + // + // The notice half of that pair is 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. + // + // The ignored paths are filtered back out for the same reason the notice + // exists at all: it must describe the run that happened. An `.adoc` inside + // `worktrees/` is not a file this run declined to convert, it is a file this + // run was never going to look at, and naming it would send the reader to + // investigate a directory the fix above deliberately excluded. + const [ignoredEntries, converterDependent] = await Promise.all([ + wholeProject ? listGitIgnoredEntries(options.cwd) : [], + findConverterDependentFiles(options.cwd, paths), + ]); + + const exclude = [ + ...(wholeProject + ? [ + `${TASKLESS_DIRECTORY}/**`, + ...gitIgnoredExclusionGlobs(ignoredEntries), + ] + : []), + ...converterExclusionGlobs(), + ]; + + const skipped = skippedFilesNotice( + converterDependent.filter((file) => !isGitIgnoredPath(file, ignoredEntries)) + ); + + // One bad target file must cost one finding, not the whole run + // (taskless/cli#300). A front-matter YAML error is Vale's own parse + // failure, not a rejected rule config, and it aborts the whole invocation + // before any result is written — exactly like the converter-dependent + // crash above, and for the same reason: nothing about it is scoped to the + // one file that triggered it. Unlike that case there is no format to + // preemptively exclude; which file is bad is only known once Vale says so. + // + // So this retries: on a failure Vale's own error object attributes to one + // of *our* target files (`targetFileParseError`), that file is added to the + // exclusion glob and the whole thing is asked again, with a finding + // recorded for the file that was dropped. A failure that cannot be + // attributed to a single target file — a bad rule, a timeout, a crash — is + // not this bug, and is reported exactly as before: blocking, with nothing + // to retry around. + // + // Bounded by construction rather than by a counter: every successful + // iteration excludes one target file that was not already excluded, and + // there are finitely many files to exclude. Re-reporting the same path + // twice in a row is the only way this could spin, and that path is refused + // rather than retried (see the `excludedTargets.has` check below). + const excludedTargets = new Set(); + const excludedFindings: CheckResult[] = []; + + for (;;) { + const globArgument = buildValeGlob([...exclude, ...excludedTargets]); + const globFlags = globArgument === undefined ? [] : [globArgument]; + + // `--` separates flags from positional paths, so a path beginning with + // `-` is not read as a flag. + const argv = [ + "--config", + configPath, + "--output=JSON", + "--no-exit", + ...globFlags, + "--", + ...targets, + ]; + + const attempt = await spawnVale( + binary, + argv, + options.cwd, + timeoutMs, + skipped + ); + + if (attempt.status === "ok") { + return { + status: "ok", + blocking: false, + results: [...excludedFindings, ...attempt.results], + ...(attempt.notice === undefined ? {} : { notice: attempt.notice }), + }; + } + + if (attempt.status === "timeout") { + return { status: "timeout", blocking: true, message: attempt.message }; + } + + const { configError } = attempt; + const candidate = + configError === undefined + ? undefined + : await targetFileParseError(configError, options.cwd); + + if (candidate === undefined || configError === undefined) { + return { status: "failed", blocking: true, message: attempt.message }; + } + if (excludedTargets.has(candidate)) { + return { status: "failed", blocking: true, message: attempt.message }; + } + + excludedTargets.add(candidate); + excludedFindings.push(parseErrorResult(candidate, configError)); + } +} + /** Absolute path of the committed Vale config for `cwd`. */ export function valeConfigPath(cwd: string): string { return join(cwd, ASSEMBLED_VALE_CONFIG); diff --git a/packages/cli/test/mixed-engine-check.test.ts b/packages/cli/test/mixed-engine-check.test.ts index 2304d5ec..48afe197 100644 --- a/packages/cli/test/mixed-engine-check.test.ts +++ b/packages/cli/test/mixed-engine-check.test.ts @@ -339,6 +339,77 @@ describe("check over a project with both engines", () => { }); }); + withVale("a target file check cannot parse (taskless/cli#300)", () => { + // The exact shape from the issue: one file whose front matter has an + // unquoted colon used to take every other file's findings down with it. + // `check content/blog --json` came back `{"results":[]}`, indistinguishable + // from a directory with nothing to say. + it("still reports every other file's findings, end to end", async () => { + const scaffold = await mkdtemp(join(tmpdir(), "taskless-parse-error-")); + try { + const init = await runCli(["init", "--no-interactive", "-d", scaffold]); + expect(init.exitCode).toBe(0); + + const rule = join(scaffold, ".taskless", "rules", "vale", "no-simply"); + await mkdir(rule, { recursive: true }); + await writeFile( + join(rule, "no-simply.yml"), + "extends: existence\nmessage: \"Avoid 'simply'\"\nlevel: warning\ntokens:\n - simply\n" + ); + await writeFile( + join(rule, ".vale.ini"), + "[*.md]\ntskl) rule = no-simply\nBasedOnStyles =\nno-simply.no-simply = YES\n" + ); + + const blog = join(scaffold, "content", "blog"); + await mkdir(blog, { recursive: true }); + await writeFile( + join(blog, "good-1.md"), + "---\ntitle: Good post one\n---\n\nJust simply do it.\n" + ); + await writeFile( + join(blog, "good-2.md"), + "---\ntitle: Good post two\n---\n\nJust simply do it, again.\n" + ); + await writeFile( + join(blog, "zzz-probe.md"), + "---\ndescription: this has a colon: right here so it cannot parse\n---\n\nSome content, simply written.\n" + ); + + const { stdout, exitCode } = await runCli([ + "check", + "content/blog", + "-d", + scaffold, + "--json", + ]); + const output = JSON.parse(stdout.trim()) as CheckOutput; + + // Before the fix: `{"success":false,"results":[],"failures":[…]}` — + // both good files' findings gone over one bad one. + expect(output.success).toBe(false); + expect(exitCode).toBe(1); + + const byFile = new Map(output.results.map((f) => [f.file, f])); + expect(byFile.get("content/blog/good-1.md")).toMatchObject({ + source: "vale", + ruleId: "no-simply", + }); + expect(byFile.get("content/blog/good-2.md")).toMatchObject({ + source: "vale", + ruleId: "no-simply", + }); + expect(byFile.get("content/blog/zzz-probe.md")).toMatchObject({ + source: "vale", + ruleId: "vale-parse-error", + severity: "error", + }); + } finally { + await rm(scaffold, { recursive: true, force: true }); + } + }); + }); + describe("whatever the host provides", () => { it("reports ast-grep findings regardless of Vale's availability", async () => { // Deliberately ungated, and deliberately not mocking the binary away: the diff --git a/packages/cli/test/vale-run.test.ts b/packages/cli/test/vale-run.test.ts index 2cc2fc08..028f80a4 100644 --- a/packages/cli/test/vale-run.test.ts +++ b/packages/cli/test/vale-run.test.ts @@ -271,6 +271,140 @@ withVale("runVale against the real binary", () => { if (outcome.status !== "timeout") return; expect(outcome.message).toContain("terminated"); }); + + describe("a target file Vale cannot parse (taskless/cli#300)", () => { + // An unquoted colon in a YAML value, exactly the shape from the issue: + // `description: this has a colon: right here` is not valid YAML, and + // Vale's front-matter parser aborts on it before any file in the run is + // linted. + const badFrontMatter = + "---\ndescription: this has a colon: right here\n---\n\nJust simply do it.\n"; + + it("keeps every other file's findings instead of zeroing the whole run", async () => { + const cwd = makeProject( + `${header}\n[*.md]\nno-simply.no-simply = YES\n`, + { "no-simply": existenceRule("simply", "Avoid 'simply'") }, + { + "good-1.md": "Just simply do it.\n", + "good-2.md": "Just simply do it, again.\n", + "bad.md": badFrontMatter, + } + ); + + const outcome = await runVale({ + cwd, + paths: ["good-1.md", "good-2.md", "bad.md"], + }); + + // Before the fix this was `{ status: "failed", results: undefined }` + // and the two good files' findings were gone. The run now completes: + // one bad file costs one finding, not the other two. + expect(outcome.status).toBe("ok"); + if (outcome.status !== "ok") return; + expect(outcome.blocking).toBe(false); + + const byFile = new Map(outcome.results.map((r) => [r.file, r])); + expect(byFile.get("good-1.md")).toMatchObject({ + ruleId: "no-simply", + file: "good-1.md", + }); + expect(byFile.get("good-2.md")).toMatchObject({ + ruleId: "no-simply", + file: "good-2.md", + }); + expect(byFile.get("bad.md")).toMatchObject({ + source: "vale", + ruleId: "vale-parse-error", + severity: "error", + file: "bad.md", + }); + expect(byFile.get("bad.md")?.message).toContain("E201"); + expect(outcome.results).toHaveLength(3); + }); + + it("distinguishes a run that found nothing from a run that could not read anything", async () => { + // The exact confusion from the issue: an empty `results` used to mean + // both "clean corpus" and "the run never got to look at anything". + const clean = makeProject( + `${header}\n[*.md]\nno-simply.no-simply = YES\n`, + { "no-simply": existenceRule("simply", "Avoid 'simply'") }, + { "doc.md": "Nothing objectionable here.\n" } + ); + const cleanOutcome = await runVale({ cwd: clean, paths: ["doc.md"] }); + expect(cleanOutcome).toMatchObject({ status: "ok", results: [] }); + + const unreadable = makeProject( + `${header}\n[*.md]\nno-simply.no-simply = YES\n`, + { "no-simply": existenceRule("simply", "Avoid 'simply'") }, + { "bad.md": badFrontMatter } + ); + const unreadableOutcome = await runVale({ + cwd: unreadable, + paths: ["bad.md"], + }); + expect(unreadableOutcome.status).toBe("ok"); + if (unreadableOutcome.status !== "ok") return; + // Not `[]`: a run that could read nothing must not look identical to a + // clean pass, which is the whole failure this issue is about. + expect(unreadableOutcome.results).not.toEqual([]); + expect(unreadableOutcome.results).toHaveLength(1); + expect(unreadableOutcome.results[0]).toMatchObject({ + ruleId: "vale-parse-error", + file: "bad.md", + }); + }); + + it("drops more than one bad file, one finding per file", async () => { + const cwd = makeProject( + `${header}\n[*.md]\nno-simply.no-simply = YES\n`, + { "no-simply": existenceRule("simply", "Avoid 'simply'") }, + { + "good.md": "Just simply do it.\n", + "bad-1.md": badFrontMatter, + "bad-2.md": badFrontMatter, + } + ); + + const outcome = await runVale({ + cwd, + paths: ["good.md", "bad-1.md", "bad-2.md"], + }); + + expect(outcome.status).toBe("ok"); + if (outcome.status !== "ok") return; + const parseErrors = outcome.results.filter( + (r) => r.ruleId === "vale-parse-error" + ); + expect(parseErrors.map((r) => r.file).toSorted()).toEqual([ + "bad-1.md", + "bad-2.md", + ]); + expect(outcome.results.find((r) => r.file === "good.md")).toMatchObject({ + ruleId: "no-simply", + }); + }); + + it("still blocks on a genuine rule-config error, without mistaking it for a target file", async () => { + // A malformed RULE, not a malformed document. Its path lives under + // `.taskless/rules/vale/`, not among the run's targets, so it must not + // be excluded and retried as though it were one of the user's files — + // that would spin forever trying to "drop" a file that is never in the + // target set at all. + const cwd = makeProject( + `${header}\n[*.md]\nbogus.bogus = YES\n`, + { + bogus: `extends: existence\nmessage: "test"\nlevel: catastrophe\ntokens:\n - simply\n`, + }, + { "doc.md": "Just simply do it.\n" } + ); + + const outcome = await runVale({ cwd, paths: ["doc.md"] }); + expect(outcome.status).toBe("failed"); + if (outcome.status !== "failed") return; + expect(outcome.blocking).toBe(true); + expect(outcome.message).toContain("bogus.yml"); + }); + }); }); describe("ValeRunOutcome.blocking", () => { From 785b1465e978545b0376ab713c4162926398408e Mon Sep 17 00:00:00 2001 From: Jakob Heuser Date: Mon, 7 Sep 2026 23:37:09 -0700 Subject: [PATCH 2/3] test(cli): pin Vale's config-error path shape as a vendor contract The #300 fix rests on a fact about VALE, not about this repository: a config error names a bad target file by its relative path as passed, and a bad rule file by an absolute path, because that one reaches Vale through StylesPath. `targetFileParseError` uses exactly that distinction to tell "one unreadable target, exclude it and retry" from "our own rule config is broken, stop". Nothing pinned it. The behaviour tests in `vale-run.test.ts` do run against the real binary and would fail if Vale changed, so this is not a coverage gap so much as a legibility one: they would report "expected ok to be failed" and leave someone to work backwards to the cause. This says which vendor assumption broke. It belongs in the vendor contract for a second reason. That file is what the upgrade procedure re-probes on every Vale bump, which is the moment this answer can change, and a bump is exactly when nobody is thinking about #300. Verified both directions against the pinned binary, and confirmed the assertion is real by inverting it and watching it fail. The failure direction stays safe either way: an unrecognised target error stops the run rather than excluding a rule config and continuing, so a change here degrades #300 back to its old behaviour rather than silently checking nothing. Refs #300 --- .../cli/test/vale-vendor-contract.test.ts | 49 ++++++++++++++++++- 1 file changed, 48 insertions(+), 1 deletion(-) diff --git a/packages/cli/test/vale-vendor-contract.test.ts b/packages/cli/test/vale-vendor-contract.test.ts index 165b75db..21b829bd 100644 --- a/packages/cli/test/vale-vendor-contract.test.ts +++ b/packages/cli/test/vale-vendor-contract.test.ts @@ -1,7 +1,7 @@ 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 { isAbsolute, join } from "node:path"; import { afterEach, describe, expect, it } from "vitest"; @@ -282,6 +282,53 @@ withVale("Vale vendor contract", () => { expect(parsed.Code).toBe("E201"); }); + it("reports a config error's Path relative for a target file, absolute for a rule", () => { + // Depended on by: `targetFileParseError` in run.ts, which uses exactly this + // distinction to tell "one unreadable TARGET file, exclude it and retry" + // from "our own rule config is broken, stop". That is the whole mechanism + // behind taskless/cli#300, and it rests on Vale's choice of path shape + // rather than on anything this repository controls. + // + // Pinned HERE, in the vendor contract, and not only through run.ts's + // behaviour tests, because of what each one says when it breaks. If Vale + // starts reporting target files absolutely, run.ts's tests fail with + // "expected ok to be failed" and someone has to work backwards to the + // cause. This one names it. The upgrade procedure re-probes this file on + // every bump, which is the moment the answer can change. + // + // The failure direction is the safe one either way: an unrecognised target + // error stops the run rather than excluding a rule config and continuing, + // so a change here degrades #300 back to its old behaviour rather than + // silently checking nothing. Loud, not silent, but still wrong. + + // A target file Vale cannot parse: unquoted colon in its front matter. + const targetCwd = project( + `${header}\n[*.md]\nrules.lvl = YES\n`, + { lvl: existence("simply") }, + { + "doc.md": + "---\ndescription: has a colon: right here\n---\n\nJust simply do it.\n", + } + ); + const targetError = JSON.parse( + runRaw(targetCwd, ["doc.md"], ["--no-exit"]).stderr + ) as { Path?: string }; + expect(targetError.Path).toBe("doc.md"); + expect(isAbsolute(targetError.Path ?? "")).toBe(false); + + // A rule file Vale cannot load, reached through StylesPath rather than + // named on the command line. + const ruleCwd = project( + `${header}\n[*.md]\nrules.bogus = YES\n`, + { bogus: existence("simply", "catastrophe") }, + { "doc.md": "Just simply do it.\n" } + ); + const ruleError = JSON.parse( + runRaw(ruleCwd, ["doc.md"], ["--no-exit"]).stderr + ) as { Path?: string }; + expect(isAbsolute(ruleError.Path ?? "")).toBe(true); + }); + describe("matcher semantics", () => { const rules = { "no-simply": existence("simply"), From 6a682d300cabf3a2dbd4c365fa1075a780971809 Mon Sep 17 00:00:00 2001 From: Jakob Heuser Date: Tue, 8 Sep 2026 09:40:13 -0700 Subject: [PATCH 3/3] fix(cli): narrow the target-file discriminator, drop double parse, reuse formatter MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses review feedback on #315: - targetFileParseError's `.taskless/`-prefix carve-out reintroduced the exact #300 bug on a path the carve-out itself was blind to: verifyValeRule points runVale explicitly at `.taskless/rules/vale//rule-tests`, which is an explicit path, not a whole-project walk, so the `.taskless/**` glob exclusion never applies there and Vale really does walk into it. A malformed fixture reported a relative Path starting with `.taskless/rules/vale/...`, which the carve-out misread as "not a target," so the retry never fired and verifyValeRule failed the whole rule instead of excluding the one bad fixture. isAbsolute alone is the correct, and now the only, discriminator — a rule/style path reached through StylesPath is always absolute (pinned in vale-vendor-contract.test.ts), so any relative Path is by construction one of the run's own targets, .taskless/ or not. - The non-zero-exit branch in spawnVale parsed stderr twice: once by hand to populate configError, once again inside describeValeStderr. Extracted parseValeConfigError so both call sites share one parse. - The "Vale rejected the configuration" branch (reached via a zero-exit stdout payload, defensive/non-live per its own comment) built its message by hand instead of reusing formatValeConfigError, so it skipped that helper's multi-line cleanup and code-dedup logic. Added a regression test in vale-verify.test.ts exercising verifyValeRule with a malformed fail-fixture, mutation-checked by reinstating the removed carve-out and confirming it fails with the pre-fix symptom ("expected a verification, got Vale failed"). --- packages/cli/src/rules/vale/run.ts | 85 +++++++++++++++++---------- packages/cli/test/vale-verify.test.ts | 42 +++++++++++++ 2 files changed, 95 insertions(+), 32 deletions(-) diff --git a/packages/cli/src/rules/vale/run.ts b/packages/cli/src/rules/vale/run.ts index 16c24c14..513ded57 100644 --- a/packages/cli/src/rules/vale/run.ts +++ b/packages/cli/src/rules/vale/run.ts @@ -99,6 +99,25 @@ export type ValeRunOutcome = | { status: "timeout"; blocking: true; message: string } | { status: "failed"; blocking: true; message: string }; +/** + * Parse Vale's stderr as its one-object config-error document, or `undefined` + * when it is not that shape. + * + * Split out from {@link describeValeStderr} so the non-zero-exit branch in + * {@link spawnVale} can parse `stderr` exactly once and use the result both to + * build the failure message and to populate `ValeAttempt.configError` — the + * value {@link targetFileParseError} reads to decide whether this failure can + * be narrowed to one target file and retried. Without this split, the same + * bytes were parsed twice: once here, once again inside `describeValeStderr`. + */ +function parseValeConfigError(stderr: string): ValeConfigError | undefined { + try { + return asValeConfigError(JSON.parse(stderr)); + } catch { + return undefined; + } +} + /** * Vale's stderr, rendered as a sentence instead of a JSON blob. * @@ -110,19 +129,17 @@ export type ValeRunOutcome = * 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. + * Takes the already-parsed error rather than re-parsing `stderr` itself — see + * {@link parseValeConfigError}. Anything that did not parse to 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; - return formatValeConfigError(error, { withPath: true }); +function describeValeStderr( + stderr: string, + configError: ValeConfigError | undefined +): string { + if (configError === undefined) return stderr; + return formatValeConfigError(configError, { withPath: true }); } /** @@ -213,10 +230,27 @@ function parseErrorResult(file: string, error: ValeConfigError): CheckResult { * absolute `Path`. A target file, by contrast, is named on Vale's command * line exactly as this module passed it — always relative to `cwd`, per * `targets` below — so a problem reading a target file reports the relative - * path we asked Vale to check. Measured against the real binary: a bad - * `level:` in a rule file reports that rule's absolute path on disk; an - * unquoted colon in a document's front matter reports the relative path this - * module handed to Vale. + * path we asked Vale to check. Measured against the real binary, and pinned as + * a vendor contract in `vale-vendor-contract.test.ts`: a bad `level:` in a rule + * file reports that rule's absolute path on disk; an unquoted colon in a + * document's front matter reports the relative path this module handed to + * Vale. + * + * `isAbsolute` is therefore the WHOLE discriminator, deliberately with no + * additional `.taskless/`-prefix carve-out. An earlier version of this + * function also rejected any relative path starting with `.taskless/`, on the + * theory that Taskless's own directory could not hold a legitimate target. + * That reasoning was wrong: `verifyValeRule` (`verify.ts`) points `runVale` + * explicitly at `.taskless/rules/vale//rule-tests`, and `check` accepts + * an explicit path under `.taskless/` and checks it (see + * `mixed-engine-check.test.ts`, "still checks an explicitly named path inside + * .taskless"). Neither call passes through the `.taskless/**` glob exclusion + * below — that exclusion applies ONLY on a whole-project walk. A malformed + * fixture under `rule-tests/` therefore reports a relative `Path` starting + * with `.taskless/rules/vale/...`, which the old carve-out misread as "not a + * target" — reintroducing the exact #300 failure on the one path meant to + * catch it: `verifyValeRule` returned one blocking failure for the whole rule + * instead of excluding just the bad fixture and reporting the rest. * * The existence check is defensive, not load-bearing: if it is ever wrong for * a real target file, the failure mode is "this file could not be excluded, @@ -230,12 +264,6 @@ async function targetFileParseError( if (path === undefined || path === "" || isAbsolute(path)) { return undefined; } - if ( - path === TASKLESS_DIRECTORY || - path.startsWith(`${TASKLESS_DIRECTORY}/`) - ) { - return undefined; - } try { const stats = await stat(resolvePath(cwd, path)); if (!stats.isFile()) return undefined; @@ -342,16 +370,11 @@ async function spawnVale( // With --no-exit, a non-zero code is Vale failing, not Vale finding. if (code !== null && code !== 0) { const stderr = stderrChunks.join("").trim(); - let configError: ValeConfigError | undefined; - try { - configError = asValeConfigError(JSON.parse(stderr)); - } catch { - configError = undefined; - } + const configError = parseValeConfigError(stderr); settle({ status: "failed", message: `Vale exited ${String(code)}${ - stderr === "" ? "" : `: ${describeValeStderr(stderr)}` + stderr === "" ? "" : `: ${describeValeStderr(stderr, configError)}` }`, ...(configError === undefined ? {} : { configError }), }); @@ -400,9 +423,7 @@ async function spawnVale( if (configError !== undefined) { settle({ status: "failed", - message: `Vale rejected the configuration (${configError.Code}): ${configError.Text}${ - configError.Path === undefined ? "" : ` in ${configError.Path}` - }`, + message: `Vale rejected the configuration: ${formatValeConfigError(configError, { withPath: true })}`, configError, }); return; diff --git a/packages/cli/test/vale-verify.test.ts b/packages/cli/test/vale-verify.test.ts index d9e9be30..f49245cd 100644 --- a/packages/cli/test/vale-verify.test.ts +++ b/packages/cli/test/vale-verify.test.ts @@ -363,6 +363,48 @@ withVale("verifyValeRule", () => { const result = verification(await verifyValeRule(cwd, "no-simply")); expect(result.passed).toBe(true); }); + + it("excludes a fixture with unparseable front matter instead of blocking every other fixture (taskless/cli#300)", async () => { + // `verifyValeRule` points `runVale` at `.taskless/rules/vale//.tests` + // directly — an explicit path, not a whole-project walk — so the + // `.taskless/**` glob exclusion in `runVale` never applies here and Vale + // really does walk into this directory. A malformed fixture therefore + // reports a config-error `Path` that starts with `.taskless/rules/vale/…`. + // `targetFileParseError` must still recognize that as a target file (no + // `.taskless/`-prefix carve-out) or this call path falls back to the + // pre-#300 behaviour: one bad fixture returns `{ outcome: { status: + // "failed" } }` for the WHOLE rule, and neither `a.md` nor `c.md` below is + // ever evaluated. + const cwd = makeProject( + { "no-simply": existence("simply") }, + { + "no-simply": { + fail: { + "a.md": "Just simply do it.\n", + "bad.md": + "---\ndescription: has a colon: right here\n---\n\nJust simply do it.\n", + }, + pass: { "c.md": "Nothing objectionable.\n" }, + }, + } + ); + + const result = verification(await verifyValeRule(cwd, "no-simply")); + + // The good fixtures are still evaluated normally: `a.md` fires, `c.md` + // stays clean. + expect(result.missingFailures).not.toContain( + ".taskless/rules/vale/no-simply/.tests/fail/a.md" + ); + expect(result.unexpectedFindings).toEqual([]); + // `bad.md` could not be parsed, so it never fires under its own rule id — + // it is reported as a missing failure rather than silently dropped, and + // rather than taking `a.md` and `c.md` down with it. + expect(result.missingFailures).toContain( + ".taskless/rules/vale/no-simply/.tests/fail/bad.md" + ); + expect(result.passed).toBe(false); + }); }); withVale("verifyValeRules", () => {