Skip to content

Commit d3d84fd

Browse files
theCodeDriftclaude
andcommitted
feat(cli): dispatch the three engines concurrently and merge their findings
Unit 3, 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. Vale's layout entry gains `executor: "vale-runner"`, replacing the `null` that recorded it as scaffolded but inert, and engine-dispatch.test.ts is updated to assert the new routing rather than the placeholder. 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 — which is precisely the "an unavailable engine must not abort the others" requirement. Using allSettled makes that 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. Exit code now has two independent causes. An error-severity finding is the ordinary one. An engine failure is the one that would be missed: a Vale that timed out or rejected its config produces no findings, so without it a broken engine exits 0 and reads exactly like a clean run. An unavailable engine stays advisory — an unsupported arch must not fail a check the other engines completed. Vale is not invoked when `.taskless/vale/rules/` is empty, per the spec. A scaffolded-but-empty engine directory is the state every `taskless init` leaves, and spawning a subprocess per check to confirm it found nothing is pure cost. Tests cover the mixed sg+vale corpus merging into one set, Vale absent while ast-grep still reports, an engine throwing without taking the others' results with it, and each exit-code cause on its own. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Jwc9FFroR3mTZ4hLiSkkX3
1 parent 2660127 commit d3d84fd

6 files changed

Lines changed: 476 additions & 41 deletions

File tree

‎openspec/changes/add-vale-rule-engine/tasks.md‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -13,9 +13,9 @@
1313

1414
## 2. Check orchestration
1515

16-
- [ ] 2.1 Dispatch to distinct executors by engine directory — ast-grep (`sg/`) → scanner, Vale (`vale/`) → runner, runtime (`runtime/rules/`) → harness
17-
- [ ] 2.2 Run engines concurrently, merge `CheckResult`s into one set, derive the exit code from merged severities, and keep an unavailable engine from aborting the others
18-
- [ ] 2.3 Tests: a mixed `sg`+`vale`+`runtime` corpus runs all executors and merges; with the `vale` binary absent, ast-grep results still return
16+
- [x] 2.1 Dispatch to distinct executors by engine directory — ast-grep (`sg/`) → scanner, Vale (`vale/`) → runner, runtime (`runtime/rules/`) → harness
17+
- [x] 2.2 Run engines concurrently, merge `CheckResult`s into one set, derive the exit code from merged severities, and keep an unavailable engine from aborting the others
18+
- [x] 2.3 Tests: a mixed `sg`+`vale`+`runtime` corpus runs all executors and merges; with the `vale` binary absent, ast-grep results still return
1919

2020
## 3. Engine-selection knowledge topic
2121

‎packages/cli/src/commands/check.ts‎

Lines changed: 32 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -2,13 +2,11 @@ import { resolve, join, isAbsolute, relative } from "node:path";
22
import { stat } from "node:fs/promises";
33
import { defineCommand } from "citty";
44

5-
import { runAstGrepScan } from "../rules/scan";
6-
import type { CheckResult } from "../types/check";
5+
import { deriveExitCode, runEngines } from "../rules/dispatch";
76
import { formatText } from "../util/format";
87
import { resolveSgConfigPath } from "../filesystem/sgconfig";
98
import { ensureTasklessDirectory } from "../filesystem/directory";
109
import {
11-
dedupeFindings,
1210
discoverAstGrepRuleSources,
1311
planEngineDispatch,
1412
} from "../rules/engines";
@@ -30,7 +28,6 @@ import {
3028
selectBlessedRuntimeRules,
3129
signRuntimeChecks,
3230
} from "../rules/runtime/run-set";
33-
import { executeRuntimeRules } from "../rules/runtime/harness";
3431

3532
async function pathExists(absolutePath: string): Promise<boolean> {
3633
try {
@@ -316,7 +313,7 @@ export const checkCommand = defineCommand({
316313
}
317314

318315
// Rules dispatch by the engine directory that contains them. This is also
319-
// the migration trigger: no config is generated on the check path any
316+
// the migration trigger: no config is generated on the check path any
320317
// more, so without this call an upgraded CLI would keep reading a stale
321318
// layout.
322319
//
@@ -363,22 +360,9 @@ export const checkCommand = defineCommand({
363360
}
364361

365362
try {
366-
const results: CheckResult[] = [];
367-
368-
// Static rules: always scan, no verification (inert data). Each
369-
// ast-grep source is scanned on its own — `sg/rules/` and, for an
370-
// unmigrated checkout, the legacy `.taskless/rules/` — and identical
371-
// findings from both are collapsed so a rule present in both layouts
372-
// is reported once.
373-
const staticResults: CheckResult[] = [];
374-
for (const source of astGrepSources) {
375-
const configPath = await resolveSgConfigPath(cwd, source);
376-
const scan = await runAstGrepScan(cwd, existingPaths, { configPath });
377-
staticResults.push(...scan.results);
378-
}
379-
results.push(...dedupeFindings(staticResults));
380-
381-
// Runtime rules: run only what the server validated (or forced).
363+
// Runtime rules are planned before dispatch, not during it: planning
364+
// consults auth and reconcile state, which is a decision about *what*
365+
// may run rather than part of running it.
382366
const plan = await planRuntime(cwd, runtimeRules, {
383367
anonymous: args.anonymous,
384368
dangerouslyRunScripts: Boolean(args["dangerously-run-scripts"]),
@@ -389,26 +373,42 @@ export const checkCommand = defineCommand({
389373
`Notice: runtime rule ${skipped.rule} was not run — ${skipped.reason}.`
390374
);
391375
}
392-
if (plan.execute.length > 0) {
393-
const runtimeResults = await executeRuntimeRules(cwd, plan.execute, {
394-
paths: existingPaths,
395-
timeoutMs: parseTimeoutMs(args.timeout),
396-
});
397-
results.push(...runtimeResults);
398-
}
376+
377+
// Every engine runs concurrently and merges into one result set. An
378+
// engine that cannot run reports a notice and the others still return.
379+
const resolvedSources = await Promise.all(
380+
astGrepSources.map(async (source) => ({
381+
source,
382+
configPath: await resolveSgConfigPath(cwd, source),
383+
}))
384+
);
385+
const dispatched = await runEngines({
386+
cwd,
387+
paths: existingPaths,
388+
astGrepSources: resolvedSources,
389+
runtimeRules: plan.execute,
390+
runtimeTimeoutMs: parseTimeoutMs(args.timeout),
391+
});
392+
const results = dispatched.results;
393+
394+
for (const notice of dispatched.notices) warn(`Notice: ${notice}`);
395+
for (const failure of dispatched.failures) warn(`Error: ${failure}`);
399396

400397
let errorCount = 0;
401398
let warningCount = 0;
402399
for (const result of results) {
403400
if (result.severity === "error") errorCount++;
404401
else if (result.severity === "warning") warningCount++;
405402
}
406-
const hasErrors = errorCount > 0;
407403
scanCounts = { errorCount, warningCount, findings: results.length };
408404

405+
// An engine failure fails the check even with no findings: a Vale that
406+
// timed out reports nothing, which would otherwise read as clean.
407+
const exitCode = deriveExitCode(dispatched);
408+
409409
if (args.json) {
410410
const output = checkOutputSchema.parse({
411-
success: !hasErrors,
411+
success: exitCode === 0,
412412
results,
413413
...(plan.skipped.length > 0 ? { skipped: plan.skipped } : {}),
414414
});
@@ -417,9 +417,8 @@ export const checkCommand = defineCommand({
417417
console.log(formatText(results));
418418
}
419419

420-
// Exit code: 1 if any errors, 0 otherwise
421-
if (hasErrors) {
422-
process.exitCode = 1;
420+
if (exitCode !== 0) {
421+
process.exitCode = exitCode;
423422
}
424423
} catch (error) {
425424
const message = `Error: ${error instanceof Error ? error.message : String(error)}`;

‎packages/cli/src/rules/dispatch.ts‎

Lines changed: 205 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,205 @@
1+
import { readdir } from "node:fs/promises";
2+
import { join } from "node:path";
3+
4+
import type { CheckResult } from "../types/check";
5+
import {
6+
dedupeFindings,
7+
ENGINE_LAYOUTS,
8+
type AstGrepRuleSource,
9+
type EngineName,
10+
} from "./engines";
11+
import { executeRuntimeRules } from "./runtime/harness";
12+
import type { RuntimeRule } from "./runtime/discover";
13+
import { runAstGrepScan } from "./scan";
14+
import { isValeFailure, runVale } from "./vale/run";
15+
16+
/**
17+
* Whether `.taskless/vale/rules/` holds anything to run.
18+
*
19+
* The spec is explicit that an empty rules directory means Vale is not invoked
20+
* at all. Worth an explicit check rather than letting Vale run and report
21+
* nothing: a scaffolded-but-empty engine directory is the common state after
22+
* `taskless init`, and spawning a subprocess per check to confirm it found
23+
* nothing is pure cost.
24+
*/
25+
export async function hasValeRules(cwd: string): Promise<boolean> {
26+
try {
27+
const entries = await readdir(
28+
join(cwd, ".taskless", ENGINE_LAYOUTS.vale.rulesDirectory)
29+
);
30+
return entries.some((entry) => entry.endsWith(".yml"));
31+
} catch {
32+
return false;
33+
}
34+
}
35+
36+
/** One engine's contribution to a check. */
37+
export interface EngineOutcome {
38+
engine: EngineName;
39+
results: CheckResult[];
40+
/**
41+
* Something the user should see that is not a finding — an engine that could
42+
* not run. Advisory: it does not affect the exit code.
43+
*/
44+
notice?: string;
45+
/**
46+
* The engine was present and failed. Unlike a notice this must reach the exit
47+
* code, or a broken engine reads as a clean run.
48+
*/
49+
failure?: string;
50+
}
51+
52+
export interface DispatchOptions {
53+
cwd: string;
54+
/** Target paths, already filtered to those that exist. */
55+
paths: string[];
56+
/** ast-grep sources, each with the config that scans it. */
57+
astGrepSources: Array<{ source: AstGrepRuleSource; configPath: string }>;
58+
/** Runtime rules that survived planning. Empty means the harness is skipped. */
59+
runtimeRules: RuntimeRule[];
60+
runtimeTimeoutMs?: number;
61+
valeTimeoutMs?: number;
62+
}
63+
64+
export interface DispatchResult {
65+
/** Every engine's findings, merged. */
66+
results: CheckResult[];
67+
/** Advisory messages: engines that could not run. */
68+
notices: string[];
69+
/** Failures that must fail the check even with no findings. */
70+
failures: string[];
71+
/** Per-engine detail, for callers that report engine by engine. */
72+
outcomes: EngineOutcome[];
73+
}
74+
75+
/**
76+
* ast-grep over every source, deduped.
77+
*
78+
* `sg/rules/` and the legacy `.taskless/rules/` are scanned separately, so a
79+
* rule present in both reports twice; the finding is its own identity, so
80+
* identical matches collapse.
81+
*/
82+
async function runAstGrepEngine(
83+
options: DispatchOptions
84+
): Promise<EngineOutcome> {
85+
const results: CheckResult[] = [];
86+
for (const { configPath } of options.astGrepSources) {
87+
const scan = await runAstGrepScan(options.cwd, options.paths, {
88+
configPath,
89+
});
90+
results.push(...scan.results);
91+
}
92+
return { engine: "sg", results: dedupeFindings(results) };
93+
}
94+
95+
/**
96+
* Vale, when it has rules to run.
97+
*
98+
* The three non-ok outcomes divide along the line `isValeFailure` draws: an
99+
* absent binary is a notice, because an unsupported arch is an ordinary state
100+
* and failing there would make `check` unrunnable on a machine where the other
101+
* engines work; a timeout or a crash is a failure, because Vale was present and
102+
* asked to work, and reporting that as a skip lets a broken rule file read as
103+
* "no Vale findings".
104+
*/
105+
async function runValeEngine(options: DispatchOptions): Promise<EngineOutcome> {
106+
if (!(await hasValeRules(options.cwd))) {
107+
return { engine: "vale", results: [] };
108+
}
109+
110+
const outcome = await runVale({
111+
cwd: options.cwd,
112+
paths: options.paths,
113+
timeoutMs: options.valeTimeoutMs,
114+
});
115+
116+
if (outcome.status === "ok") {
117+
return { engine: "vale", results: outcome.results };
118+
}
119+
return isValeFailure(outcome)
120+
? { engine: "vale", results: [], failure: outcome.message }
121+
: { engine: "vale", results: [], notice: outcome.message };
122+
}
123+
124+
/** The runtime harness, over rules that planning already cleared to run. */
125+
async function runRuntimeEngine(
126+
options: DispatchOptions
127+
): Promise<EngineOutcome> {
128+
if (options.runtimeRules.length === 0) {
129+
return { engine: "runtime", results: [] };
130+
}
131+
const results = await executeRuntimeRules(options.cwd, options.runtimeRules, {
132+
paths: options.paths,
133+
timeoutMs: options.runtimeTimeoutMs,
134+
});
135+
return { engine: "runtime", results };
136+
}
137+
138+
/**
139+
* Run every engine that has work, concurrently, and merge what they report.
140+
*
141+
* Concurrency is the point: the engines are independent subprocesses over the
142+
* same paths, and running them in sequence makes a check as slow as the sum of
143+
* its engines for no benefit.
144+
*
145+
* It also forces the isolation question. `allSettled`, not `all`: `all` rejects
146+
* on the first rejection and abandons the others, so one engine throwing would
147+
* discard results the rest had already produced — exactly the "an unavailable
148+
* engine must not abort the others" requirement, and the shape that makes it
149+
* true by construction rather than by everyone remembering to catch.
150+
*
151+
* A rejected engine becomes a failure rather than being swallowed. The engines
152+
* themselves report expected trouble as an outcome; a thrown error is something
153+
* unforeseen, and treating it as "no findings" would be the silent-disable
154+
* failure again.
155+
*/
156+
export async function runEngines(
157+
options: DispatchOptions
158+
): Promise<DispatchResult> {
159+
const engines: Array<[EngineName, Promise<EngineOutcome>]> = [
160+
["sg", runAstGrepEngine(options)],
161+
["vale", runValeEngine(options)],
162+
["runtime", runRuntimeEngine(options)],
163+
];
164+
165+
const settled = await Promise.allSettled(engines.map(([, task]) => task));
166+
167+
const outcomes: EngineOutcome[] = settled.map((entry, index) => {
168+
const engine = engines[index]?.[0] ?? "sg";
169+
if (entry.status === "fulfilled") return entry.value;
170+
const reason: unknown = entry.reason;
171+
return {
172+
engine,
173+
results: [],
174+
failure: `${engine} engine failed: ${
175+
reason instanceof Error ? reason.message : String(reason)
176+
}`,
177+
};
178+
});
179+
180+
return {
181+
results: outcomes.flatMap((outcome) => outcome.results),
182+
notices: outcomes
183+
.map((outcome) => outcome.notice)
184+
.filter((notice): notice is string => notice !== undefined),
185+
failures: outcomes
186+
.map((outcome) => outcome.failure)
187+
.filter((failure): failure is string => failure !== undefined),
188+
outcomes,
189+
};
190+
}
191+
192+
/**
193+
* The exit code for a completed check.
194+
*
195+
* Two independent reasons to fail, and both are needed. An error-severity
196+
* finding is the ordinary one. An engine failure is the one that is easy to
197+
* miss: a Vale that timed out or rejected its config produces no findings, so
198+
* without this a broken engine exits 0 and reads exactly like a clean run.
199+
*/
200+
export function deriveExitCode(result: DispatchResult): number {
201+
const hasErrorFinding = result.results.some(
202+
(finding) => finding.severity === "error"
203+
);
204+
return hasErrorFinding || result.failures.length > 0 ? 1 : 0;
205+
}

‎packages/cli/src/rules/engines.ts‎

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,11 @@ export const ENGINES = ["sg", "vale", "runtime"] as const;
1414
export type EngineName = (typeof ENGINES)[number];
1515

1616
/** How a rule reaches execution, or `null` when this CLI has no executor yet. */
17-
export type EngineExecutor = "ast-grep" | "runtime-harness" | null;
17+
export type EngineExecutor =
18+
| "ast-grep"
19+
| "vale-runner"
20+
| "runtime-harness"
21+
| null;
1822

1923
export interface EngineLayout {
2024
engine: EngineName;
@@ -40,8 +44,7 @@ export const ENGINE_LAYOUTS = {
4044
rulesDirectory: "vale/rules",
4145
ruleTestsDirectory: "vale/rule-tests",
4246
configFile: "vale/.vale.ini",
43-
// Scaffolded but inert: the Vale engine itself is a later change.
44-
executor: null,
47+
executor: "vale-runner",
4548
},
4649
runtime: {
4750
engine: "runtime",

‎packages/cli/test/engine-dispatch.test.ts‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -128,10 +128,11 @@ describe("engine dispatch by directory", () => {
128128
present: true,
129129
executor: "runtime-harness",
130130
});
131-
// Scaffolded, recognized, but nothing executes it yet.
131+
// Vale gained its executor with the Vale engine; before that this was
132+
// `null` because the directory was scaffolded but inert.
132133
expect(byEngine.get("vale")).toMatchObject({
133134
present: true,
134-
executor: null,
135+
executor: "vale-runner",
135136
});
136137
});
137138

0 commit comments

Comments
 (0)