feat(cli): share the platform-binary resolver, add the Vale binary spec

[thecodedrift]

Stack (root → tip):

• ➡️ feat(cli): share the platform-binary resolver, add the Vale binary spec #71 (you are here)
  • feat(cli): verify Vale rules from per-rule fixture directories #93
    • 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

Unit 1 of add-vale-rule-engine — the bottom of a four-unit stack that merges down. This PR carries the change proposal plus unit 1's implementation.

Stack

Unit	Branch	Scope
1	openspec/add-vale-rule-engine ← this PR	Binary resolution, Vale runner, CheckResult mapping
2	…-2-verify	Fixture-based verify
3	…-3-orchestration	Concurrent multi-engine dispatch and merge
4	…-4-topic	Engine-selection topic, TOPICS entry, archive

Merging down, per the proposal: the Vale engine is only correct once check can dispatch to it — the runner without orchestration ships an engine that never executes, and orchestration without the runner dispatches to nothing. Units merge tip→root, then one protected merge of this branch to main.

Landed so far — task 1.1

findSgBinary() hard-coded ast-grep's packaging: the @ast-grep/cli prefix, the -gnu/-msvc suffixes, two bin spellings, and an ast-grep identity check. Vale needs the same search with different answers to all four, so the search moved to rules/platform-binary.ts and each engine supplies a spec.

The parameter that matters is toolchainSuffix. ast-grep publishes @ast-grep/cli-linux-x64-gnu; add-vale-binary-packages publishes @taskless/vale-linux-x64 with no libc suffix. Reusing ast-grep's naming for Vale would resolve nothing on Linux and surface as the ordinary "Vale is unavailable" message — a naming bug wearing the costume of a host that never installed it. test/platform-binary.test.ts pins the naming for both engines across every published platform and cross-checks the Vale names against the optionalDependencies actually declared, so a rename on either side fails there rather than at runtime.

The resolver returns {path, tried} rather than throwing, because the callers need different things from a miss. ast-grep is the only executor for sg rules, so findSgBinary() still throws; findValeBinary() returns undefined per D6b — a missing Vale binary makes one engine unavailable and must not abort the others. It caches the miss as well as the hit, since an absent Vale is the common case and each resolution spawns a subprocess per candidate.

ast-grep behaviour is unchanged: isAstGrepBinary and findSgBinary keep their signatures and their existing tests.

Still to come in this unit

• 1.2 rules/vale/run.ts — invoke Vale against the committed config with JSON output, bounded by a subprocess timeout
• 1.3 Vale JSON → CheckResult: rules. prefix stripping, severity normalization (suggestion → hint), range from Line/Span
• 1.5 Tests: mapping, committed-config scoping, missing-binary and timeout paths

Verification

pnpm --filter @taskless/cli test → 456 passed; typecheck, lint, prettier clean.

Refs OSS-21

[Copilot]

Pull request overview

Adds a new OpenSpec change (openspec/changes/add-vale-rule-engine/) that specifies how the CLI should support Vale as a second static-tier rules engine, including engine routing, concurrent execution/merge semantics, and an engine-selection help topic.

Changes:

• Introduces requirements for running Vale with committed config, mapping Vale JSON findings into CheckResult, and handling missing binary/timeout cases.
• Specifies per-rule scoping via .vale.ini matchers and fixture-based Vale rule verification (pass//fail).
• Adds proposal/design/tasks documentation and OpenSpec metadata for the change.

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
openspec/changes/add-vale-rule-engine/.openspec.yaml	Declares OpenSpec change metadata (schema + created date).
openspec/changes/add-vale-rule-engine/proposal.md	Summarizes motivation, scope, impact, and delivery shape for adding Vale + engine-selection topic.
openspec/changes/add-vale-rule-engine/design.md	Captures key design decisions (scoping, layout, binary resolution strategy, mapping contract).
openspec/changes/add-vale-rule-engine/tasks.md	Implementation task checklist for Vale engine execution, orchestration, and help topic registration/tests.
openspec/changes/add-vale-rule-engine/specs/cli-vale-rule-engine/spec.md	Requirements for Vale execution model, config usage, result mapping, timeout, missing-binary behavior, and verify fixtures.
openspec/changes/add-vale-rule-engine/specs/cli-rule-routing/spec.md	Requirements for the engine-selection knowledge topic and its constraints (distinct from route/trust tier).
openspec/changes/add-vale-rule-engine/specs/cli-rule-format/spec.md	Requirements for Vale style layout under .taskless/vale/rules/ and rules. prefix normalization.
openspec/changes/add-vale-rule-engine/specs/cli-help/spec.md	Requirements to register engine-selection in help + add cross-references from route/static.
openspec/changes/add-vale-rule-engine/specs/cli-check/spec.md	Requirements for concurrent per-engine execution and merged results semantics.

Suppressed comments (1)

openspec/changes/add-vale-rule-engine/specs/cli-check/spec.md:18

• This spec ends with a "## MODIFIED Requirements" heading but contains no modified requirements. Empty sections can confuse reviewers and can break tooling that expects content under each header.

## MODIFIED Requirements

---

💡 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 2m 1s —— View job

---

Review checklist

• Gather context (PR description, diff origin/main...HEAD, OpenSpec docs, existing Copilot review comments)
• Run 8 parallel review passes (reuse/duplication, removed-behavior audit, cross-file tracing, CLAUDE.md conventions, altitude, efficiency, simplification, line-by-line diff scan)
• Verify highest-severity findings against the actual source before posting
• Post inline comments on concrete issues
• Post overall assessment

Overall assessment

This is careful, well-documented work — the doc comments explaining why (toolchainSuffix, identity checks, the unavailable-vs-failed distinction) are genuinely good and made this review easier. One real bug and a couple of behavior regressions surfaced from the findSgBinary → resolvePlatformBinary generalization, worth fixing before this merges.

Bug (blocking): vale/map.ts — Vale's Line/Span are 1-based (the file's own doc comments say so), but CheckResult.range is documented and used elsewhere as 0-indexed (runtime/harness.ts:22-23, ast-grep's schema, and format.ts which unconditionally does +1 for display). toValeCheckResult writes Vale's values through unconverted, so every Vale finding will display one line and one column past its real location. The existing test in vale-map.test.ts currently pins the unconverted value, so this won't fail CI as written.

Real regressions from the platform-binary.ts extraction (scan.ts): the search order for ast-grep vs sg at the node_modules/.bin and PATH tiers is silently reversed from the original (which deliberately checked sg first — see the removed comment "matching the PATH search below"), the "put sg on your PATH" error hint lost its Windows .exe suffix, and the tried diagnostic list now shows the platform-package tier twice. None of this is caught by a test, since existing tests assert resolution succeeds/fails, not which candidate wins.

Design gap worth a second look: in vale/run.ts, child.on("error") treats a post-verification spawn failure as "unavailable" (a skip, per isValeFailure), even though findValeBinary() already proved the binary runs during resolution — so a binary that vanishes or loses permissions between resolution and execution is silently treated the same as "Vale isn't installed," rather than surfaced as the real problem it is. scan.ts's equivalent handler treats the analogous case as a hard failure instead.

Smaller correctness concern: vale/run.ts decodes stdout/stderr with chunk.toString() per Buffer chunk rather than a stateful decoder, which can corrupt a multi-byte UTF-8 character split across a chunk boundary — plausible given Vale lints free-form prose. Worst case this turns a clean run into a false "Vale produced output that is not JSON" failure.

Process note: the changeset (.changeset/vale-rule-engine.md) currently describes units 2-4 of the stack (orchestration, verify fixtures, help topic) that aren't in this PR's diff — the root CLAUDE.md's stacked-PR guidance asks for the changeset to grow incrementally with what's actually landed on each PR, not describe the end state up front.

Also flagged inline, lower priority: isValeFailure encodes severity as a function callers must remember to call rather than a field on ValeRunOutcome (currently unenforced since orchestration hasn't landed yet), and a documentation contradiction between map.ts and run.ts about whether Vale's config-error payload can arrive with exit 0.

Not flagged as blocking, but noted during review for awareness: vale/run.ts's timeout/kill/settle scaffolding duplicates the shape already built in runtime/invoke.ts (no shared helper); runAstGrepScan still has no timeout at all, unlike the new Vale runner; a few small dead/duplicated bits in platform-binary.ts (PlatformBinarySpec.label is unused, tried is computed twice via separate .map() calls in the hit/miss return paths); and findValeBinary's return type duplicates the already-exported PlatformBinaryResolution instead of reusing it. None of these affect correctness of this unit and are reasonable to leave for a follow-up.

Copilot's earlier review comments about the two tasks.md placeholders and a stale line-number reference in design.md are still open and worth a quick pass, but are cosmetic relative to the above.

[claude]

Bug: Vale's 1-based Line/Span are written into CheckResult.range unconverted, but range is documented elsewhere as 0-indexed.

packages/cli/src/rules/runtime/harness.ts:22-23 states the contract explicitly: `Finding` line/column are 1-indexed (harness contract); `CheckResult.range` is 0-indexed (ast-grep native — display and `--json` consumers add 1), and findingToCheckResult in that same file does Math.max(0, finding.line - 1) to honor it. ast-grep's own schema (src/generated/ast-grep-rule-schema.json:297-322) documents its native range as 0-based too.

toValeCheckResult here stores finding.Line and finding.Span directly — both of which this file's own doc comments say are 1-based (ValeFinding.Span: "[start, end] columns, 1-based", line 17). The comment on line 142 ("Span is 1-based and inclusive, which matches what ast-grep already emits for column") appears to be incorrect — ast-grep's column is 0-based, not 1-based.

Concretely, formatText (src/util/format.ts:12) does result.range.start.line + 1 unconditionally for every CheckResult regardless of source. A Vale finding reported on line 3 will display as line 4 (and the column will be off by one too).

packages/cli/test/vale-map.test.ts:58-71 currently pins the unconverted (1-based) values as the expected output, so this won't be caught by CI as written — the test encodes the same off-by-one the implementation has.

Suggest subtracting 1 from finding.Line, startColumn, and endColumn (clamped at 0) to match the runtime-rule mapper's pattern, and updating the test fixture accordingly.

[claude]

child.on("error", ...) here always resolves as status: "unavailable", which isValeFailure() treats as a skip that must not fail check (per its own doc comment: "unavailable is a skip ... not evidence of anything wrong").

But by this point findValeBinary() has already verified the binary by successfully running --version during resolution (isPlatformBinary, platform-binary.ts:104-115). An error event on the actual scan spawn means something changed between resolution and execution — the binary was deleted, permissions changed, or it was quarantined — which is a real, if rare, failure, not "Vale isn't installed here."

Contrast with the analogous case in scan.ts:161-174 (runAstGrepScan's child.on("error")), which treats the same "vanished after verification" scenario as a rejection (a hard failure), not a silent success-path skip. Here the equivalent condition is silently downgraded to "other engines still ran, nothing to report" — a genuine installation problem could go unnoticed by the user.

[claude]

binaryNames: ["ast-grep", "sg"] silently reverses the search priority that existed before this refactor.

The removed code (see the diff) explicitly tried alternative (sg) before binary (ast-grep) in both the node_modules/.bin and PATH tiers, with a comment calling this out as deliberate ("Both names, matching the PATH search below"). resolvePlatformBinary now just iterates spec.binaryNames in list order at every tier, so those same two tiers now try ast-grep first — the opposite of the original priority — and the platform-package tier now probes both names (2 require.resolve calls) where it previously only checked one.

Two smaller regressions from the same change, both below in findSgBinary:

• The "install a supported platform build, or put `sg` on your PATH" hint (line ~104) now hardcodes the literal string "sg". The removed code used `${alternative}`, which was "sg.exe" on Windows — that platform-correct suffix is now lost.
• resolvePlatformBinary's tried list (platform-binary.ts:163,167) now lists the platform-package tier twice (once per binaryNames entry) instead of once, so the "Looked in: ..." error message has a redundant duplicate entry.

None of this is exercised by a test — sg-binary.test.ts and the new platform-binary.test.ts assert that resolution succeeds/fails, not which of two present candidates wins, so the ordering flip wouldn't be caught by CI.

[claude]

chunk.toString() decodes each Buffer chunk independently rather than accumulating raw buffers (or using node:string_decoder's StringDecoder) and decoding once. If a multi-byte UTF-8 sequence straddles a chunk boundary — plausible here since Vale lints free-form prose that can contain curly quotes, em dashes, accented characters, etc. — each half gets replaced with U+FFFD independently instead of being reassembled correctly.

Best case this corrupts Message/Match text silently; worst case the corruption lands inside a JSON string's escaping and JSON.parse(stdout) throws, which is then reported as status: "failed", message: "Vale produced output that is not JSON" — a false failure for a Vale run that actually completed. runAstGrepScan (scan.ts:145) avoids this by reading child.stdout through node:readline's createInterface, which decodes safely.

[claude]

This changeset text describes work from units 2-4 of the stack that isn't in this PR's diff: concurrent multi-engine dispatch/merge in check (lines 7-8 — commands/check.ts isn't touched here, and tasks 2.1-2.3 are unchecked), fixture-based Vale rule verification (lines 12-13 — no rules/vale/verify.ts in this diff, task 1.4 unchecked), and the engine-selection help topic (lines 15-17 — no src/help/* changes here, tasks 3.1-3.7 unchecked).

Per the root CLAUDE.md's stacked-PR guidance ("One changeset, at the bottom of the stack, grown as the stack grows"): "Each PR extends the changeset with its own scope rather than the base describing the whole future change up front. A reviewer reading the changeset then sees only what has actually landed, and is not asked to evaluate a release note that promises more than the diff in front of them." As written, a reviewer of this PR alone sees a release note for three units of work that haven't landed yet. Worth trimming this changeset down to unit 1's scope (binary resolution, Vale runner, CheckResult mapping) and growing it in the follow-up PRs as the proposal itself describes.

[claude]

Minor API-design note: isValeFailure is a free function callers must remember to invoke rather than severity being encoded on ValeRunOutcome itself. Currently there are zero production callers (grep confirms only tests reference it — orchestration lands in task 2.2), so nothing exercises this yet, but it'd be worth considering a blocking: boolean field (or similar) on the type so a future author writing the natural-looking outcome.status !== "ok" check is visibly wrong at compile time rather than only wrong once wired up — that specific mistake would fail check on every host missing the Vale binary, which is exactly the regression D6b exists to prevent.

[claude]

Minor: this comment says the config-error payload is measured to go to stderr with exit 2 (so it "never reaches the mapper today"), but run.ts:173-174's comment says the opposite — "a malformed rule makes Vale emit a config error and still exit 0, so this is the only place the difference is detectable." These two comments describe contradictory measured behavior for the same scenario; worth reconciling so a future reader isn't left guessing which one is accurate.