feat(cli): verify Vale rules from per-rule fixture directories

[thecodedrift]

Stack (root → tip):

• feat(cli): share the platform-binary resolver, add the Vale binary spec #71
  • ➡️ feat(cli): verify Vale rules from per-rule fixture directories #93 (you are here)
    • feat(cli): dispatch the three engines concurrently and merge their findings #94
      • feat(cli): add the engine-selection topic and archive add-vale-rule-engine #95
        • test(cli): check a project both engines have work in, and fix what it found #100
          • docs(openspec): propose the agent command and the Vale authoring path #102

Unit 2 of add-vale-rule-engine. Stacked on #71, merging down.

Task 1.4 plus the verify half of 1.5: for each .taskless/vale/rule-tests/<rule>/, generate an isolating config, run Vale over its pass/ and fail/ fixtures, and assert every fail/ fires and every pass/ is clean.

The generated config is isolating, not merely minimal

Three details in it are load-bearing:

Detail	Why
StylesPath is absolute	The config is written to a temp dir, and Vale resolves StylesPath relative to the config file
BasedOnStyles = is empty	Otherwise a fixture could trip Vale.Spelling and be counted as the rule under test firing
Exactly one assignment, in one matcher	Precedence is positional, so assigning the key twice would lean on the semantics the scoping spec got wrong

A pass/ fixture's whole claim is "the rule under test does not fire here", and that claim is worthless if another rule's finding is what got counted. There's a test for exactly that: a pass/ fixture that trips a different rule still verifies clean.

The expected set comes from disk, not from Vale's output

A fixture that produces no finding is absent from the payload entirely. So a fail/ fixture that silently stopped matching is unobservable in the output alone — which is precisely the case the verifier exists to catch. Fixture lists are read from disk and compared against what fired.

Two states that could have shipped as false confidence

• A rule with no fixtures reports empty, not passed. An empty directory proves nothing; calling it a pass is how an unverified rule ships looking verified.
• An unavailable or failed Vale stops the pass rather than being recorded per rule. With no binary every rule reports no findings — indistinguishable from every rule being broken — and a wall of verification failures would send someone to debug their rules over a missing install.

Found while writing the fixtures

Vale's existence is case-sensitive by default: Simply does not match the token simply. This first read as a bug in the verifier rather than as fixtures that never matched, so it is now pinned in vale-vendor-contract.test.ts alongside the rest of Vale's observable behaviour.

Verification

pnpm --filter @taskless/cli test → 514 passed (14 new); lint, typecheck, prettier, and openspec validate --strict all clean.

Tasks 1.4 and 1.5 are now complete, which closes out section 1.

Refs OSS-21

[Copilot]

Pull request overview

Adds a fixture-driven verification harness for the Vale rule engine so each rule under .taskless/vale/rule-tests/<rule>/ can be validated in isolation (fail fixtures must fire; pass fixtures must be clean), with accompanying tests and a pinned “vendor contract” behavior check.

Changes:

• Implemented Vale rule fixture discovery and per-rule verification using an ephemeral, isolating .vale.ini.
• Added comprehensive tests for verify behavior (pass/fail, isolation, empty fixtures, missing binary).
• Documented completion of tasks 1.4/1.5 and pinned an additional observed Vale default (case-sensitive existence tokens).

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

File	Description
packages/cli/src/rules/vale/verify.ts	Adds fixture discovery + per-rule and all-rules verification logic using isolating generated config.
packages/cli/test/vale-verify.test.ts	Adds tests for verification behavior, including isolation and missing-binary handling.
packages/cli/test/vale-vendor-contract.test.ts	Pins additional Vale default behavior (existence tokens are case-sensitive by default).
openspec/changes/add-vale-rule-engine/tasks.md	Marks tasks 1.4/1.5 as complete and summarizes where coverage landed across units.

Suppressed comments (1)

packages/cli/src/rules/vale/verify.ts:105

• This catch { return [] } treats any readdir failure as "no tests". For errors like EACCES, that can incorrectly make verifyValeRules() report {status: "ok", rules: []} even though fixtures exist but are unreadable. Suggest only swallowing ENOENT/ENOTDIR and rethrowing other errors.

  } catch {
    return [];
  }

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[thecodedrift]

@claude /review

[claude]

Claude finished @thecodedrift's task in 3m 47s —— View job

---

Review of #93 — Vale rule verification (fixture-driven)

• Gather context (diff, run.ts, map.ts, engines.ts)
• Understand the request (review only)
• Review verify.ts for correctness
• Review tests for coverage/clarity
• Post inline comments
• Post final summary

Summary

Solid, well-documented unit. The isolating-config approach (absolute StylesPath, empty BasedOnStyles, single rules.<id> assignment) is correct and well-tested — the "pass fixture trips a different rule" isolation test in particular is exactly the right regression guard. Reading the expected set from disk rather than from Vale's output correctly handles the "silent fixture that stopped matching" case the PR calls out, and the case-sensitivity pin in vale-vendor-contract.test.ts is a good catch to preserve.

Two inline comments posted:

1. verify.ts:130 — The empty check only catches a rule with zero fixtures in both buckets. A rule with fail/ fixtures but no pass/ fixtures (or vice versa) is not empty and can report passed: true without ever proving the half of the claim that empty bucket represents — the same "unverified rule ships looking verified" failure mode the PR explicitly calls out for the fully-empty case, just less obvious.
2. verify.ts:64 — fixtureFiles's bare catch { return [] } treats any readdir failure (not just missing directory) as "no fixtures for this bucket." Since pass/ and fail/ are read independently, an unreadable pass/ bucket alongside a working fail/ bucket silently downgrades to "0 pass fixtures" rather than surfacing the error, and the rule can still report passed: true. This mirrors the pattern Copilot already flagged on discoverValeRuleTests (line 103) and is consistent with existing bare-catch conventions elsewhere in rules/ (engines.ts, runtime/discover.ts), but the consequence here is quieter under-verification rather than just "no rules found."

Also agree with Copilot's observation on verifyValeRule's return shape ({ unavailable: ValeRunOutcome } covers timeout/failed too, not just a missing binary) — the naming is misleading at call sites even though verifyValeRules unpacks it correctly today.

Minor, non-blocking:

• verifyValeRules verifies rules sequentially in a for loop, each spawning a Vale subprocess; this appears intentional (stop on the first unavailable/failed rather than recording partial results), so not flagging as a bug, just noting the tradeoff.
• No test exercises a rule with only pass/ fixtures and no fail/ fixtures (or vice versa) — would be good coverage for the gap in finding 1 above.

Nothing else stood out as incorrect; config generation, path normalization (toRelativePosix), and the unavailable-stops-the-pass behavior for verifyValeRules all match the PR description and are backed by tests.

[thecodedrift]

Re: @claude[bot] — "Review of #93 — Vale rule verification (fixture-driven) … Solid, well-documented unit."
#93 (comment)

Both inline findings were real and are fixed in d0d32b5.

One-sided fixture buckets — empty: boolean is replaced by fixtures: ValeFixtureCoverage ("both" | "pass-only" | "fail-only" | "none"), and only "both" can reach passed: true. A union rather than a second boolean, so "unwritten" and "half-written" stay distinguishable and cannot contradict each other. Tests now cover both one-sided cases, which you correctly noted nothing exercised.

The bare catch — fixtureFiles and discoverValeRuleTests now swallow only ENOENT/ENOTDIR and rethrow the rest. Your read of why this site is the consequential one is what justified fixing it rather than accepting the existing convention: the buckets are read independently, so a swallowed EACCES on pass/ left a rule looking populated rather than broken, and able to report passed: true with the pass side never checked.

Your agreement with Copilot on the { unavailable: ValeRunOutcome } return shape was also acted on — renamed to { outcome }, confirmed free by grepping for production callers first (there are none yet, only tests).

Two things you raised that I did not change, deliberately:

• Sequential verification in verifyValeRules. Agreed it is a tradeoff rather than a bug, and the stop-on-first-unavailable behaviour is the intended one — a wall of per-rule failures caused by one missing binary is exactly the outcome that guard exists to prevent.
• The other bare catches in rules/engines.ts and runtime/discover.ts. Left alone as out of scope. They are genuinely lower-consequence, since none has the independent-reads property that made fixtureFiles dangerous. A related one in filesystem/migrations/0004-vale-engine.ts:218 is arguably the worst of them, given its own comment explains that ENOTDIR is the case it needs to handle.

The spec was silent on an unpopulated bucket, which is what let code and spec drift here. Corrected one PR up in #95 (57e3eea) rather than on this branch — this file is git mv'd into the archive by #95, so an edit here would be dropped when that move replays during the rebase.

— AI Coding Agent

[thecodedrift]

small changes, mostly things that will make our code more resilient.

[thecodedrift]

This will probably get swept up in a simplify pass, but isMissingDirectory feels either like it should be a utility function more generally available or removed because it's adding a big layer of indirection.

I'm leaning towards the former, because I imagine SG needs this too

[thecodedrift]

I actually really appreciate this sanity test. Do we have one for ast-grep? If not, we should get another PR started (can stack on this or just off main) which adds this sanity test to make sure the binary's behavior didn't change.