fix(cli): one bad file costs one finding, not the whole Vale run

[thecodedrift]

Summary

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 from the retry 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 offending file and why it could not be parsed; the fix reuses that rather than re-deriving the same fact with a second parser (see STYLEGUIDE-CODE.md's "Verify Build Output In The Build, Not By Parsing It").

Before:

{"success":false,"results":[],"failures":["Vale exited 2: E201: yaml: mapping values are not allowed in this context in content/blog/zzz-probe.md"]}

After:

{"success":false,"results":[
  {"source":"vale","ruleId":"vale-parse-error","severity":"error","file":"content/blog/zzz-probe.md","message":"Vale could not check this file: E201: yaml: mapping values are not allowed in this context", ...},
  {"source":"vale","ruleId":"no-simply","severity":"warning","file":"content/blog/good-1.md", ...},
  {"source":"vale","ruleId":"no-simply","severity":"warning","file":"content/blog/good-2.md", ...}
]}

Test plan

• packages/cli/test/vale-run.test.ts — unit coverage against the real Vale binary: one bad file among good ones, two bad files, the "empty results vs unreadable run" distinction the issue is about, and a regression check that a genuine rule-config error still blocks (not misattributed to a target file).
• packages/cli/test/mixed-engine-check.test.ts — end-to-end test spawning the built CLI over a scaffolded project reproducing the issue's exact repro.
• Mutation-checked: reverted the fix (forced the retry candidate to always be undefined), confirmed the new tests fail with the pre-fix symptom, reverted back, confirmed green.
• pnpm typecheck, pnpm test (1350 tests), pnpm lint (includes pnpm cli check against the built CLI) all pass.
• Reproduced the issue's exact repro against the built CLI on a scratch directory, before and after.

Fixes #300

[thecodedrift]

Verified independently before review, since this is the largest change in the batch.

The discriminator holds for how the CLI actually invokes it. The absolute-vs-relative Path test is clever but load-bearing, so I checked what happens if a target ever arrives absolute: commands/check.ts:46 normalizes every incoming path to one relative to cwd and rejects anything escaping cwd, so runVale cannot be handed an absolute target through the CLI. The invariant the discriminator rests on is enforced one layer up, and your comment at the guard already says so.

Worth noting the failure direction too, because it is the right one: if the discriminator ever misjudged, the existence check and the .taskless/ guard make it fail toward blocking the whole run (loud, and the pre-fix behaviour) rather than toward excluding a rule config and continuing, which would silently run without a rule. That is the asymmetry that matters here, and it is on the safe side.

Mutation re-run independently: forcing the narrowing to never apply fails exactly the three tests meant to catch it, and reverting returns all 31 to green.

One thing left undone that I agree with: Option 3 (making the payload unambiguous when failures is non-empty) stays open. This fix converts the #300 class into an ok outcome with real findings so the ambiguity does not arise here, but it still exists for other blocking failures across all three engines. That is a broader design question and was right to leave.

— AI Coding Agent

[thecodedrift]

@claude /review

[claude]

Claude finished @thecodedrift's task in 6m 20s —— View job

---

Claude finished @thecodedrift's task ——

Todo

• Read .prior-review.json (prior top-level comments/threads)
• Fetch the PR diff (gh pr diff 315)
• Review packages/cli/src/rules/vale/run.ts retry/exclusion logic line by line
• Trace every caller of runVale / targetFileParseError for broken preconditions
• Check map.ts / formats.ts for interactions with the new code
• Review test coverage in vale-run.test.ts and mixed-engine-check.test.ts
• Post findings

Review mode: incremental — read 0 prior review thread(s) before reviewing. .prior-review.json contains no reviewThreads; it holds one top-level self-review comment from thecodedrift (independent verification of the absolute-vs-relative discriminator and mutation testing) and the /review trigger comment itself. That self-review doesn't raise any open findings to carry forward — it confirms the design holds and flags one already-acknowledged open item (Option 3, making the payload unambiguous when failures is non-empty), which the PR description also calls out as deliberately deferred. So there is nothing STILL OPEN to repeat; everything below is [New].

Summary

The core fix (retry-and-exclude in runVale, driven entirely off Vale's own error object with no second YAML parser) is sound, and the test coverage for the check path — one bad file, two bad files, the empty-results ambiguity, and the rule-config-error regression check — is thorough and well-targeted at the actual failure mode from #300.

One likely correctness gap and two small cleanups, posted inline:

1. targetFileParseError's .taskless/ exclusion likely reintroduces the same bug on a different call path (packages/cli/src/rules/vale/run.ts:233-238). verifyValeRule (vale/verify.ts:253) legitimately points runVale at .taskless/rules/vale/<ruleId>/rule-tests — isWholeProjectWalk is false for that explicit path, so the .taskless/** exclusion doesn't apply and Vale really does walk it. A malformed fixture there reports a relative Path starting with .taskless/, which this guard treats as "not a target file," so the retry never fires and runVale falls back to the pre-fix blocking failure. That surfaces in inspect.ts:409-418 as ok: false for the whole rule, with no per-fixture attribution — one bad fixture blocking evaluation of every other pass/fail fixture, which is the class of bug this PR exists to fix, just not on this path. Worth confirming whether the guard is needed at all given StylesPath errors are already caught by the isAbsolute check above it, or scoping it to whole-project walks only.

2. Duplicate stderr parsing (run.ts:344-357): the non-zero-exit branch does its own JSON.parse(stderr) + asValeConfigError to get configError, then immediately calls describeValeStderr(stderr) which parses the same string again internally. Minor waste, easy to consolidate.

3. Inconsistent message formatting (run.ts:400-406): the "Vale rejected the configuration" branch (reached when a config error arrives via a zero-exit stdout payload) builds its message by hand instead of the formatValeConfigError helper this PR extracted for exactly this type — so it skips the multi-line cleanup and code-dedup logic the other two call sites get. Marked as a defensive/non-live path in the surrounding comment, so low impact, but easy to make consistent now that the helper exists.

Nothing else stood out — the discriminator's absolute-vs-relative reasoning, the bounded-retry argument, and the excludedTargets.has guard against re-excluding the same path all check out against the code as written.

[thecodedrift]

Re: @claude — "Claude finished @thecodedrift's task in 6m 20s"

Addressed all three findings in 94ee60e:

1. [likely bug] the .taskless/ exclusion (run.ts:238) — confirmed reachable and reproduced it: verifyValeRule points runVale explicitly at .taskless/rules/vale/<ruleId>/rule-tests, which is an explicit path (not a whole-project walk), so the .taskless/** glob exclusion never applies and Vale really does walk into it. A malformed fixture there reported a relative Path starting with .taskless/rules/vale/..., which the carve-out misread as "not a target" — reintroducing One unparseable file zeroes findings for the whole check run #300 on the one path meant to catch it. Removed the carve-out entirely; isAbsolute(path) is sufficient on its own, since StylesPath is always written absolute (now pinned as a vendor contract). Added a regression test in vale-verify.test.ts, mutation-checked by reinstating the removed carve-out and confirming it fails with the exact pre-fix symptom.

2. [cleanup] hand-built message instead of formatValeConfigError (run.ts:406) — reused the helper. No test pinned the old wording.

3. [cleanup] duplicate stderr parse (run.ts:357) — extracted parseValeConfigError and parse once, threading the result into describeValeStderr. Mutation-checked that the shared value is load-bearing (forcing it to always return undefined fails all three One unparseable file zeroes findings for the whole check run #300 retry tests).

pnpm typecheck, pnpm test (1352 passing), and pnpm lint all green after the changes.