feat(cli): dispatch the three engines concurrently and merge their findings

[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
    • ➡️ feat(cli): dispatch the three engines concurrently and merge their findings #94 (you are here)
      • 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
            • feat: one directory per rule, and path-addressed verify/test #103
              • docs(openspec): make every spec pass validate --all --strict #106

Unit 3 of add-vale-rule-engine. Stacked on #93, merging down. Tasks 2.1–2.3.

check sequenced ast-grep then runtime inline and had no Vale at all. That block moves to rules/dispatch.ts, gains Vale, and runs all three concurrently.

allSettled, not all

all rejects on the first rejection and abandons the rest — so one engine throwing would discard findings the others had already produced. That is precisely the "an unavailable engine must not abort the others" requirement, and allSettled makes it true by construction rather than by every future caller remembering to catch.

A rejected engine becomes a reported failure rather than being swallowed. The engines report expected trouble as an outcome, so a throw is something unforeseen — and treating it as "no findings" is the silent-disable failure again.

There's a test for exactly this: ast-grep is stubbed to reject, and Vale's findings still come back while the rejection surfaces as a failure.

Exit code now has two independent causes

Cause	Exit
Error-severity finding	1
Engine failure (timeout, crash, bad config)	1
Engine unavailable (no binary)	0 — advisory

The second is the one that would have been missed: a Vale that timed out produces no findings, so without it a broken engine exits 0 and reads exactly like a clean run. The third is deliberately not a failure — an unsupported arch must not fail a check the other engines completed.

Other changes

• Vale's layout entry gains executor: "vale-runner", replacing the null that recorded it as scaffolded-but-inert. engine-dispatch.test.ts is updated to assert the new routing rather than the placeholder — repointing the reader in the same unit that changes the behaviour.
• Vale is not invoked when .taskless/vale/rules/ is empty, per the spec. That's the state every taskless init leaves, and spawning a subprocess per check to confirm it found nothing is pure cost.

Verification

pnpm --filter @taskless/cli test → 524 passed (10 new); lint, typecheck, prettier, openspec validate --strict clean.

Section 2 is complete. Remaining: unit 4 — the engine-selection topic, its TOPICS entry, and the archive.

Refs OSS-21

[Copilot]

Pull request overview

This PR introduces a new orchestration layer for check that dispatches the sg (ast-grep), Vale, and runtime engines concurrently, merges their findings, and derives the exit code from both findings severity and engine failures (while treating “unavailable” engines as advisory).

Changes:

• Add rules/dispatch.ts to run all engines via Promise.allSettled, merging results and surfacing engine failures without discarding other engines’ findings.
• Wire check to use the new dispatcher and update engine layout metadata so Vale is executed via vale-runner.
• Add orchestration tests and update the OpenSpec tasks checklist for section 2.

Reviewed changes

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

Show a summary per file

File	Description
packages/cli/test/vale-orchestration.test.ts	Adds orchestration/exit-code tests for concurrent engine dispatch, Vale availability, and failure handling.
packages/cli/test/engine-dispatch.test.ts	Updates expectations/comments to reflect Vale now having an executor (vale-runner).
packages/cli/src/rules/engines.ts	Adds vale-runner executor and updates Vale engine layout to be executable.
packages/cli/src/rules/dispatch.ts	New shared dispatcher: concurrent engine runs, merged results, notices vs failures, exit code derivation.
packages/cli/src/commands/check.ts	Moves orchestration logic to runEngines() and uses deriveExitCode() for process exit + JSON success.
openspec/changes/add-vale-rule-engine/tasks.md	Marks orchestration tasks 2.1–2.3 as complete.

---

💡 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 53s —— View job

---

Review: dispatch the three engines concurrently and merge their findings

• Gather context (diff, changed files, CLAUDE.md conventions)
• Review packages/cli/src/rules/dispatch.ts
• Review packages/cli/src/commands/check.ts changes
• Review packages/cli/src/rules/engines.ts changes
• Review test files
• Post inline comments for concrete issues
• Post final summary

Overall assessment

The allSettled-based dispatcher in dispatch.ts is solid: engines are launched concurrently, a thrown engine becomes a failure instead of aborting the others, and deriveExitCode correctly treats an engine failure as exit 1 even with zero findings. The notice vs failure split for Vale's three non-ok outcomes (unavailable = advisory, timeout/failed = failure) is well reasoned and well tested in vale-orchestration.test.ts.

I found one bug I'd consider blocking, plus a few smaller issues, all posted inline:

1. packages/cli/src/commands/check.ts (~line 347) — Vale-only projects are never checked. The early-return "No rules configured" gate only inspects astGrepSources and runtimeRules:

if (astGrepSources.length === 0 && runtimeRules.length === 0) { ... return; }

It never asks whether Vale has rules. A project with only .taskless/vale/rules/*.yml populated (no ast-grep, no runtime rules) hits this branch, prints "No rules configured," and returns before runEngines/hasValeRules is ever called — so Vale silently never runs, despite this PR giving it a real executor. The comment right above this block (line ~331, "An engine directory this CLI has no executor for (vale) contributes nothing") is a leftover from before Vale had an executor and is now actively misleading. Existing check.test.ts cases only cover an empty project and an empty sg/rules/ dir, so there's no coverage for "Vale-only" that would have caught this.

2. packages/cli/src/rules/dispatch.ts:25-34 — hasValeRules swallows all readdir errors, not just "missing directory." An EACCES or other unexpected IO error is treated identically to "scaffolded but empty," so Vale gets silently skipped with no notice and no failure — exactly the "silent-disable" failure mode this PR's own isValeFailure docstring argues against elsewhere. (Also flagged by the Copilot review bot.) Suggest only swallowing ENOENT and letting other errors propagate so they surface as an engine failure via allSettled.

3. packages/cli/test/vale-orchestration.test.ts:201-226 — "keeps a thrown engine from discarding the others' results" isn't portable. Unlike the withVale-gated block above it, this describe runs unconditionally and calls runEngines over a real Vale config/rules without mocking findValeBinary, then asserts a source === "vale" result exists. On a machine/CI runner without the optional Vale binary, runVale returns unavailable and this assertion fails, even though the behavior under test (ast-grep's rejection not swallowing Vale's results) has nothing to do with Vale being installed. (Also flagged by the Copilot review bot — I independently arrived at the same conclusion tracing withVale's scope.) Suggest wrapping in withVale or mocking runVale to a deterministic ok outcome.

Nothing else stood out — the allSettled merge/failure logic, the EngineOutcome/DispatchResult shapes, and the engines.ts/engine-dispatch.test.ts executor wiring all look correct and consistent with the PR's stated design. Did not run build/lint/test locally; CI will report those.

[thecodedrift]

Re: @claude[bot] — "Review: dispatch the three engines concurrently and merge their findings … I found one bug I'd consider blocking, plus a few smaller issues"
#94 (comment)

You were right to call finding 1 blocking, and it is the most valuable thing any review turned up on this stack. Verified at check.ts:347 exactly as described: the gate asked only astGrepSources.length === 0 && runtimeRules.length === 0, so a project whose only rules live in .taskless/vale/rules/ returned "No rules configured" before runEngines was ever reached — Vale silently skipped, in the PR that gives Vale an executor. Your point that no check.test.ts case covered "Vale-only" is why nothing caught it.

Fixed in 4bb8c5d. The gate now asks hasValeRules(cwd) last in the && chain, so it is short-circuited away for any project that already has ast-grep or runtime rules and the ordinary path pays nothing. The stale comment above it — the one claiming Vale has no executor — is rewritten. There is a new regression test, and it was confirmed to fail before the fix (stash src/, rebuild, run: expected 'No rules configured…' not to contain 'No rules configured') rather than assumed to.

Findings 2 and 3 are fixed in d8602bf:

• hasValeRules swallowing every IO error now returns false only for ENOENT/ENOTDIR and lets the rest propagate, so allSettled turns it into an engine failure and a non-zero exit. Your parallel to isValeFailure's own docstring two files away is what made this worth fixing rather than accepting as convention.
• The non-portable test is now mocked rather than gated, per your and Copilot's preferred option, so the allSettled behaviour is exercised on every machine instead of skipped where Vale is absent. The assertion also moved off source === "vale" onto a distinctive ruleId from the mock — otherwise a spy that failed to intercept would still leave the test passing on a machine with the real binary.

One follow-up deliberately left out of scope, flagged so it is visible rather than lost: the new hasValeRules call sits outside the inner try that builds the SCAN_FAILED envelope, so an EACCES there exits non-zero and loud (the silent-disable concern is satisfied) but under --json prints a bare message rather than the error envelope. Tightening it means moving the try to also enclose planEngineDispatch, discoverAstGrepRuleSources, and discoverRuntimeRules, which are all currently outside it too.

Also note isValeFailure no longer exists — it became a blocking field on ValeRunOutcome, consumed here in c666973.

— AI Coding Agent

[thecodedrift]

just a small refactoring suggestion

[thecodedrift]

@claude /review

[claude]

Claude finished @thecodedrift's task in 2m 15s —— View job

---

Review: dispatch the three engines concurrently and merge their findings

• Gather context (diff, prior review threads, CLAUDE.md conventions)
• Re-review packages/cli/src/rules/dispatch.ts and packages/cli/src/commands/check.ts after the d8602bf/4bb8c5d/c666973 fixes
• Verify duplicate hasValeRules calls, --json output surface, and exit-code plumbing against the actual code
• Post inline comments for concrete issues
• Post final summary

Overall assessment

The three findings from the prior review round (check.ts's "Vale-only projects never checked" gate, hasValeRules swallowing all IO errors, the non-portable orchestration test) are all fixed and hold up under re-reading: the gate now asks hasValeRules(cwd) last in the && chain, hasValeRules only treats ENOENT/ENOTDIR as absence, and the orchestration tests mock runVale/findValeBinary deterministically instead of depending on a real binary. DispatchResult.exitCode replacing the standalone deriveExitCode helper (per your own follow-up comment) is a clean simplification — computed once, and four unit tests against a hand-built struct became coverage through runEngines itself for the same five paths.

Went a level deeper on this pass and found five new items, all posted inline. None are blocking; the first is the most worth a look:

1. --json output can't surface engine failures (check.ts:405, schemas/check.ts:34-41) — warn() no-ops under --json, and checkOutputSchema has no failures/notices field, so when an engine failure (not a finding) is why success: false, a JSON consumer sees {"success":false,"results":[]} with no explanation. skipped already exists on the schema for a similar advisory case, so a failures field following that pattern would close the gap.

2. hasValeRules is called twice per check (check.ts:358 and dispatch.ts:135) — the gate computes the answer to decide whether to short-circuit, then runValeEngine re-derives it from scratch a few lines of dataflow later. Also an asymmetry with the other two engines, whose "do I have work" is decided once upstream and threaded through DispatchOptions.

3. runAstGrepEngine scans its configs sequentially (dispatch.ts:106-117) — a project with both sg/rules/ and the legacy rules/ populated pays the sum of two subprocess spawns in serial, inside a PR about running scans concurrently instead of summing their latencies.

4. hasValeRules's ENOENT/ENOTDIR check duplicates isMissingDirectory in packages/cli/src/rules/vale/verify.ts:60-66 — same decision, independently written twice.

5. Confirmed for the record, not new: the hasValeRules gate call at check.ts:358 sits outside the inner try/catch that builds the SCAN_FAILED envelope, so a non-ENOENT error there (e.g. EACCES) escapes uncaught and bypasses --json. This is exactly the follow-up you already flagged as deliberately out of scope in the thread — verified accurate, left inline for whoever picks up the follow-up.

Nothing else stood out. Import conventions, exported-type reachability, and the allSettled/notice-vs-failure split all check out clean against CLAUDE.md and STYLEGUIDE-CODE.md. Did not run build/lint/test locally, per instructions — CI reports those.

[claude]

dispatched.failures has no way to reach --json output. warn() is a no-op whenever args.json is true (line 286-288), and checkOutputSchema (packages/cli/src/schemas/check.ts:34-41) only defines success/results/skipped — no field for failures or notices.

So when an engine failure (not a finding) is why the check fails — e.g. Vale times out or its config crashes, while ast-grep/runtime produce nothing error-severity — --json output is just {"success":false,"results":[]}. A script consuming that JSON has no way to distinguish "a rule fired" from "an engine crashed," even though dispatched.failures has the message right there.

Given skipped already exists on outputSchema as an advisory array for one engine's (runtime's) edge case, a failures/notices field following the same pattern would give --json consumers parity with what the text path already prints.

[claude]

This duplicates the hasValeRules call check.ts's noRuleFiles gate already made (check.ts:358) to decide whether to short-circuit — every check run against a project with Vale rules pays two readdirs of the same directory for the same answer.

It's also an asymmetry with the other two engines: astGrepConfigPaths and runtimeRules are both pre-resolved upstream and threaded through DispatchOptions (empty means skip), but Vale re-derives "do I have work" itself from cwd down here. Threading the gate's already-computed answer through DispatchOptions (or just not re-checking here) would remove both the duplicate I/O and the asymmetry.

[claude]

This loops astGrepConfigPaths with a sequential await per config, spawning each ast-grep subprocess one after another. A project mid-migration (sg/rules/ and the legacy rules/ both populated) pays the sum of both scans in serial — inside the PR whose stated point is running scans concurrently instead of paying the sum of their latencies. The two configs are independent, so Promise.all(options.astGrepConfigPaths.map((configPath) => runAstGrepScan(...))) would cut static-scan latency for that case, same as the three-engine dispatch does one level up.

[claude]

ABSENT_DIRECTORY_CODES/this ENOENT-or-ENOTDIR check re-implements isMissingDirectory in packages/cli/src/rules/vale/verify.ts:60-66 — same decision ("is this readdir failure just 'directory doesn't exist'"), independently written twice (a Set + inline compare here, a typed helper there). Worth sharing one implementation so the two call sites' definition of "absent" can't quietly drift apart.

[claude]

Confirming this for the record since it lines up with the follow-up you already flagged in the PR thread: this hasValeRules(cwd) call sits inside the outer try (opened at line 295) but before the inner try/catch (375/434) that builds the SCAN_FAILED envelope. hasValeRules deliberately rethrows anything that isn't ENOENT/ENOTDIR, so an EACCES on .taskless/vale/rules/ here isn't caught by this function — it propagates past the finally to the top-level handler in index.ts, which prints the raw error and ignores --json entirely. Not raising this as new (you already scoped it out), just confirming it's accurate in case the follow-up picks it up.