From 0ad1c1e075e25da28080109452eb3925b2b3ae7a Mon Sep 17 00:00:00 2001 From: Jakob Heuser Date: Mon, 10 Aug 2026 20:50:22 -0700 Subject: [PATCH 1/6] feat(cli): dispatch the three engines concurrently and merge their findings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) Claude-Session: https://claude.ai/code/session_01Jwc9FFroR3mTZ4hLiSkkX3 --- .../changes/add-vale-rule-engine/tasks.md | 6 +- packages/cli/src/commands/check.ts | 65 +++-- packages/cli/src/rules/dispatch.ts | 205 ++++++++++++++++ packages/cli/src/rules/engines.ts | 9 +- packages/cli/test/engine-dispatch.test.ts | 5 +- packages/cli/test/vale-orchestration.test.ts | 227 ++++++++++++++++++ 6 files changed, 476 insertions(+), 41 deletions(-) create mode 100644 packages/cli/src/rules/dispatch.ts create mode 100644 packages/cli/test/vale-orchestration.test.ts diff --git a/openspec/changes/add-vale-rule-engine/tasks.md b/openspec/changes/add-vale-rule-engine/tasks.md index 61004a1b..ba369682 100644 --- a/openspec/changes/add-vale-rule-engine/tasks.md +++ b/openspec/changes/add-vale-rule-engine/tasks.md @@ -13,9 +13,9 @@ ## 2. Check orchestration -- [ ] 2.1 Dispatch to distinct executors by engine directory — ast-grep (`sg/`) → scanner, Vale (`vale/`) → runner, runtime (`runtime/rules/`) → harness -- [ ] 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 -- [ ] 2.3 Tests: a mixed `sg`+`vale`+`runtime` corpus runs all executors and merges; with the `vale` binary absent, ast-grep results still return +- [x] 2.1 Dispatch to distinct executors by engine directory — ast-grep (`sg/`) → scanner, Vale (`vale/`) → runner, runtime (`runtime/rules/`) → harness +- [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 +- [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 ## 3. Engine-selection knowledge topic diff --git a/packages/cli/src/commands/check.ts b/packages/cli/src/commands/check.ts index a2709cba..4a6d2dbc 100644 --- a/packages/cli/src/commands/check.ts +++ b/packages/cli/src/commands/check.ts @@ -2,13 +2,11 @@ import { resolve, join, isAbsolute, relative } from "node:path"; import { stat } from "node:fs/promises"; import { defineCommand } from "citty"; -import { runAstGrepScan } from "../rules/scan"; -import type { CheckResult } from "../types/check"; +import { deriveExitCode, runEngines } from "../rules/dispatch"; import { formatText } from "../util/format"; import { resolveSgConfigPath } from "../filesystem/sgconfig"; import { ensureTasklessDirectory } from "../filesystem/directory"; import { - dedupeFindings, discoverAstGrepRuleSources, planEngineDispatch, } from "../rules/engines"; @@ -30,7 +28,6 @@ import { selectBlessedRuntimeRules, signRuntimeChecks, } from "../rules/runtime/run-set"; -import { executeRuntimeRules } from "../rules/runtime/harness"; async function pathExists(absolutePath: string): Promise { try { @@ -316,7 +313,7 @@ export const checkCommand = defineCommand({ } // Rules dispatch by the engine directory that contains them. This is also -// the migration trigger: no config is generated on the check path any + // the migration trigger: no config is generated on the check path any // more, so without this call an upgraded CLI would keep reading a stale // layout. // @@ -363,22 +360,9 @@ export const checkCommand = defineCommand({ } try { - const results: CheckResult[] = []; - - // Static rules: always scan, no verification (inert data). Each - // ast-grep source is scanned on its own — `sg/rules/` and, for an - // unmigrated checkout, the legacy `.taskless/rules/` — and identical - // findings from both are collapsed so a rule present in both layouts - // is reported once. - const staticResults: CheckResult[] = []; - for (const source of astGrepSources) { - const configPath = await resolveSgConfigPath(cwd, source); - const scan = await runAstGrepScan(cwd, existingPaths, { configPath }); - staticResults.push(...scan.results); - } - results.push(...dedupeFindings(staticResults)); - - // Runtime rules: run only what the server validated (or forced). + // Runtime rules are planned before dispatch, not during it: planning + // consults auth and reconcile state, which is a decision about *what* + // may run rather than part of running it. const plan = await planRuntime(cwd, runtimeRules, { anonymous: args.anonymous, dangerouslyRunScripts: Boolean(args["dangerously-run-scripts"]), @@ -389,13 +373,26 @@ export const checkCommand = defineCommand({ `Notice: runtime rule ${skipped.rule} was not run — ${skipped.reason}.` ); } - if (plan.execute.length > 0) { - const runtimeResults = await executeRuntimeRules(cwd, plan.execute, { - paths: existingPaths, - timeoutMs: parseTimeoutMs(args.timeout), - }); - results.push(...runtimeResults); - } + + // Every engine runs concurrently and merges into one result set. An + // engine that cannot run reports a notice and the others still return. + const resolvedSources = await Promise.all( + astGrepSources.map(async (source) => ({ + source, + configPath: await resolveSgConfigPath(cwd, source), + })) + ); + const dispatched = await runEngines({ + cwd, + paths: existingPaths, + astGrepSources: resolvedSources, + runtimeRules: plan.execute, + runtimeTimeoutMs: parseTimeoutMs(args.timeout), + }); + const results = dispatched.results; + + for (const notice of dispatched.notices) warn(`Notice: ${notice}`); + for (const failure of dispatched.failures) warn(`Error: ${failure}`); let errorCount = 0; let warningCount = 0; @@ -403,12 +400,15 @@ export const checkCommand = defineCommand({ if (result.severity === "error") errorCount++; else if (result.severity === "warning") warningCount++; } - const hasErrors = errorCount > 0; scanCounts = { errorCount, warningCount, findings: results.length }; + // An engine failure fails the check even with no findings: a Vale that + // timed out reports nothing, which would otherwise read as clean. + const exitCode = deriveExitCode(dispatched); + if (args.json) { const output = checkOutputSchema.parse({ - success: !hasErrors, + success: exitCode === 0, results, ...(plan.skipped.length > 0 ? { skipped: plan.skipped } : {}), }); @@ -417,9 +417,8 @@ export const checkCommand = defineCommand({ console.log(formatText(results)); } - // Exit code: 1 if any errors, 0 otherwise - if (hasErrors) { - process.exitCode = 1; + if (exitCode !== 0) { + process.exitCode = exitCode; } } catch (error) { const message = `Error: ${error instanceof Error ? error.message : String(error)}`; diff --git a/packages/cli/src/rules/dispatch.ts b/packages/cli/src/rules/dispatch.ts new file mode 100644 index 00000000..8484c259 --- /dev/null +++ b/packages/cli/src/rules/dispatch.ts @@ -0,0 +1,205 @@ +import { readdir } from "node:fs/promises"; +import { join } from "node:path"; + +import type { CheckResult } from "../types/check"; +import { + dedupeFindings, + ENGINE_LAYOUTS, + type AstGrepRuleSource, + type EngineName, +} from "./engines"; +import { executeRuntimeRules } from "./runtime/harness"; +import type { RuntimeRule } from "./runtime/discover"; +import { runAstGrepScan } from "./scan"; +import { isValeFailure, runVale } from "./vale/run"; + +/** + * Whether `.taskless/vale/rules/` holds anything to run. + * + * The spec is explicit that an empty rules directory means Vale is not invoked + * at all. Worth an explicit check rather than letting Vale run and report + * nothing: a scaffolded-but-empty engine directory is the common state after + * `taskless init`, and spawning a subprocess per check to confirm it found + * nothing is pure cost. + */ +export async function hasValeRules(cwd: string): Promise { + try { + const entries = await readdir( + join(cwd, ".taskless", ENGINE_LAYOUTS.vale.rulesDirectory) + ); + return entries.some((entry) => entry.endsWith(".yml")); + } catch { + return false; + } +} + +/** One engine's contribution to a check. */ +export interface EngineOutcome { + engine: EngineName; + results: CheckResult[]; + /** + * Something the user should see that is not a finding — an engine that could + * not run. Advisory: it does not affect the exit code. + */ + notice?: string; + /** + * The engine was present and failed. Unlike a notice this must reach the exit + * code, or a broken engine reads as a clean run. + */ + failure?: string; +} + +export interface DispatchOptions { + cwd: string; + /** Target paths, already filtered to those that exist. */ + paths: string[]; + /** ast-grep sources, each with the config that scans it. */ + astGrepSources: Array<{ source: AstGrepRuleSource; configPath: string }>; + /** Runtime rules that survived planning. Empty means the harness is skipped. */ + runtimeRules: RuntimeRule[]; + runtimeTimeoutMs?: number; + valeTimeoutMs?: number; +} + +export interface DispatchResult { + /** Every engine's findings, merged. */ + results: CheckResult[]; + /** Advisory messages: engines that could not run. */ + notices: string[]; + /** Failures that must fail the check even with no findings. */ + failures: string[]; + /** Per-engine detail, for callers that report engine by engine. */ + outcomes: EngineOutcome[]; +} + +/** + * ast-grep over every source, deduped. + * + * `sg/rules/` and the legacy `.taskless/rules/` are scanned separately, so a + * rule present in both reports twice; the finding is its own identity, so + * identical matches collapse. + */ +async function runAstGrepEngine( + options: DispatchOptions +): Promise { + const results: CheckResult[] = []; + for (const { configPath } of options.astGrepSources) { + const scan = await runAstGrepScan(options.cwd, options.paths, { + configPath, + }); + results.push(...scan.results); + } + return { engine: "sg", results: dedupeFindings(results) }; +} + +/** + * Vale, when it has rules to run. + * + * The three non-ok outcomes divide along the line `isValeFailure` draws: an + * absent binary is a notice, because an unsupported arch is an ordinary state + * and failing there would make `check` unrunnable on a machine where the other + * engines work; a timeout or a crash is a failure, because Vale was present and + * asked to work, and reporting that as a skip lets a broken rule file read as + * "no Vale findings". + */ +async function runValeEngine(options: DispatchOptions): Promise { + if (!(await hasValeRules(options.cwd))) { + return { engine: "vale", results: [] }; + } + + const outcome = await runVale({ + cwd: options.cwd, + paths: options.paths, + timeoutMs: options.valeTimeoutMs, + }); + + if (outcome.status === "ok") { + return { engine: "vale", results: outcome.results }; + } + return isValeFailure(outcome) + ? { engine: "vale", results: [], failure: outcome.message } + : { engine: "vale", results: [], notice: outcome.message }; +} + +/** The runtime harness, over rules that planning already cleared to run. */ +async function runRuntimeEngine( + options: DispatchOptions +): Promise { + if (options.runtimeRules.length === 0) { + return { engine: "runtime", results: [] }; + } + const results = await executeRuntimeRules(options.cwd, options.runtimeRules, { + paths: options.paths, + timeoutMs: options.runtimeTimeoutMs, + }); + return { engine: "runtime", results }; +} + +/** + * Run every engine that has work, concurrently, and merge what they report. + * + * Concurrency is the point: the engines are independent subprocesses over the + * same paths, and running them in sequence makes a check as slow as the sum of + * its engines for no benefit. + * + * It also forces the isolation question. `allSettled`, not `all`: `all` rejects + * on the first rejection and abandons the others, so one engine throwing would + * discard results the rest had already produced — exactly the "an unavailable + * engine must not abort the others" requirement, and the shape that makes it + * true by construction rather than by everyone remembering to catch. + * + * A rejected engine becomes a failure rather than being swallowed. The engines + * themselves report expected trouble as an outcome; a thrown error is something + * unforeseen, and treating it as "no findings" would be the silent-disable + * failure again. + */ +export async function runEngines( + options: DispatchOptions +): Promise { + const engines: Array<[EngineName, Promise]> = [ + ["sg", runAstGrepEngine(options)], + ["vale", runValeEngine(options)], + ["runtime", runRuntimeEngine(options)], + ]; + + const settled = await Promise.allSettled(engines.map(([, task]) => task)); + + const outcomes: EngineOutcome[] = settled.map((entry, index) => { + const engine = engines[index]?.[0] ?? "sg"; + if (entry.status === "fulfilled") return entry.value; + const reason: unknown = entry.reason; + return { + engine, + results: [], + failure: `${engine} engine failed: ${ + reason instanceof Error ? reason.message : String(reason) + }`, + }; + }); + + return { + results: outcomes.flatMap((outcome) => outcome.results), + notices: outcomes + .map((outcome) => outcome.notice) + .filter((notice): notice is string => notice !== undefined), + failures: outcomes + .map((outcome) => outcome.failure) + .filter((failure): failure is string => failure !== undefined), + outcomes, + }; +} + +/** + * The exit code for a completed check. + * + * Two independent reasons to fail, and both are needed. An error-severity + * finding is the ordinary one. An engine failure is the one that is easy to + * miss: a Vale that timed out or rejected its config produces no findings, so + * without this a broken engine exits 0 and reads exactly like a clean run. + */ +export function deriveExitCode(result: DispatchResult): number { + const hasErrorFinding = result.results.some( + (finding) => finding.severity === "error" + ); + return hasErrorFinding || result.failures.length > 0 ? 1 : 0; +} diff --git a/packages/cli/src/rules/engines.ts b/packages/cli/src/rules/engines.ts index 37232337..82ce211a 100644 --- a/packages/cli/src/rules/engines.ts +++ b/packages/cli/src/rules/engines.ts @@ -14,7 +14,11 @@ export const ENGINES = ["sg", "vale", "runtime"] as const; export type EngineName = (typeof ENGINES)[number]; /** How a rule reaches execution, or `null` when this CLI has no executor yet. */ -export type EngineExecutor = "ast-grep" | "runtime-harness" | null; +export type EngineExecutor = + | "ast-grep" + | "vale-runner" + | "runtime-harness" + | null; export interface EngineLayout { engine: EngineName; @@ -40,8 +44,7 @@ export const ENGINE_LAYOUTS = { rulesDirectory: "vale/rules", ruleTestsDirectory: "vale/rule-tests", configFile: "vale/.vale.ini", - // Scaffolded but inert: the Vale engine itself is a later change. - executor: null, + executor: "vale-runner", }, runtime: { engine: "runtime", diff --git a/packages/cli/test/engine-dispatch.test.ts b/packages/cli/test/engine-dispatch.test.ts index 717b82e8..f06f044e 100644 --- a/packages/cli/test/engine-dispatch.test.ts +++ b/packages/cli/test/engine-dispatch.test.ts @@ -128,10 +128,11 @@ describe("engine dispatch by directory", () => { present: true, executor: "runtime-harness", }); - // Scaffolded, recognized, but nothing executes it yet. + // Vale gained its executor with the Vale engine; before that this was + // `null` because the directory was scaffolded but inert. expect(byEngine.get("vale")).toMatchObject({ present: true, - executor: null, + executor: "vale-runner", }); }); diff --git a/packages/cli/test/vale-orchestration.test.ts b/packages/cli/test/vale-orchestration.test.ts new file mode 100644 index 00000000..7a7d13c5 --- /dev/null +++ b/packages/cli/test/vale-orchestration.test.ts @@ -0,0 +1,227 @@ +import { mkdirSync, mkdtempSync, rmSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; + +import { afterEach, describe, expect, it, vi } from "vitest"; + +import { + deriveExitCode, + hasValeRules, + runEngines, + type DispatchResult, +} from "../src/rules/dispatch"; +import { findValeBinary } from "../src/rules/vale/binary"; + +const withVale = findValeBinary().path === undefined ? describe.skip : describe; + +const workspaces: string[] = []; +afterEach(() => { + vi.restoreAllMocks(); + while (workspaces.length > 0) { + rmSync(workspaces.pop() as string, { recursive: true, force: true }); + } +}); + +/** A project with an sg rule, a vale rule, and a document tripping both. */ +function makeMixedProject(options?: { valeRules?: boolean }) { + const valeRules = options?.valeRules ?? true; + const cwd = mkdtempSync(join(tmpdir(), "vale-orch-")); + workspaces.push(cwd); + + mkdirSync(join(cwd, ".taskless", "sg", "rules"), { recursive: true }); + writeFileSync( + join(cwd, ".taskless", "sg", "rules", "no-eval.yml"), + [ + "id: no-eval", + "language: javascript", + "severity: warning", + "message: Avoid eval", + "rule:", + " pattern: eval($$$ARGS)", + "", + ].join("\n") + ); + writeFileSync( + join(cwd, ".taskless", "sg", "sgconfig.yml"), + "ruleDirs:\n - rules\n" + ); + + mkdirSync(join(cwd, ".taskless", "vale", "rules"), { recursive: true }); + if (valeRules) { + writeFileSync( + join(cwd, ".taskless", "vale", "rules", "no-simply.yml"), + `extends: existence\nmessage: "Avoid 'simply'"\nlevel: warning\ntokens:\n - simply\n` + ); + } + writeFileSync( + join(cwd, ".taskless", "vale", ".vale.ini"), + "StylesPath = .\nMinAlertLevel = suggestion\n\n[*.md]\nBasedOnStyles =\nrules.no-simply = YES\n" + ); + + writeFileSync(join(cwd, "app.js"), "eval('1 + 1');\n"); + writeFileSync(join(cwd, "doc.md"), "Just simply do it.\n"); + return cwd; +} + +const sgSources = (cwd: string) => [ + { + source: { + rulesDirectory: "sg/rules", + ruleTestsDirectory: "sg/rule-tests", + absoluteRulesDirectory: join(cwd, ".taskless", "sg", "rules"), + ruleIds: ["no-eval"], + legacy: false, + }, + configPath: ".taskless/sg/sgconfig.yml", + }, +]; + +describe("hasValeRules", () => { + it("is false for a scaffolded-but-empty rules directory", async () => { + // The common state after `taskless init`. Spawning Vale per check to + // confirm it found nothing is pure cost. + expect(await hasValeRules(makeMixedProject({ valeRules: false }))).toBe( + false + ); + }); + + it("is true once a rule file exists", async () => { + expect(await hasValeRules(makeMixedProject())).toBe(true); + }); +}); + +describe("deriveExitCode", () => { + const base: DispatchResult = { + results: [], + notices: [], + failures: [], + outcomes: [], + }; + + it("is 0 for a clean run", () => { + expect(deriveExitCode(base)).toBe(0); + }); + + it("is 0 when an engine is merely unavailable", () => { + // A notice is advisory. An unsupported arch must not fail a check the + // other engines completed. + expect(deriveExitCode({ ...base, notices: ["vale unavailable"] })).toBe(0); + }); + + it("is 1 for an engine failure even with no findings", () => { + // The case that is easy to miss: a Vale that timed out reports nothing, so + // without this a broken engine exits 0 and reads exactly like a clean run. + expect(deriveExitCode({ ...base, failures: ["vale timed out"] })).toBe(1); + }); + + it("ignores warning-severity findings", () => { + expect( + deriveExitCode({ + ...base, + results: [ + { + source: "vale", + ruleId: "r", + severity: "warning", + message: "m", + file: "a.md", + range: { + start: { line: 1, column: 1 }, + end: { line: 1, column: 2 }, + }, + matchedText: "x", + }, + ], + }) + ).toBe(0); + }); +}); + +withVale("runEngines over a mixed corpus", () => { + it("runs every executor and merges their findings into one set", async () => { + const cwd = makeMixedProject(); + const dispatched = await runEngines({ + cwd, + paths: ["app.js", "doc.md"], + astGrepSources: sgSources(cwd), + runtimeRules: [], + }); + + const sources = new Set(dispatched.results.map((result) => result.source)); + expect(sources).toContain("ast-grep"); + expect(sources).toContain("vale"); + expect(dispatched.failures).toEqual([]); + // One merged set, not per-engine buckets the caller has to reassemble. + expect(dispatched.results.length).toBeGreaterThanOrEqual(2); + }); + + it("does not invoke Vale when it has no rules", async () => { + const cwd = makeMixedProject({ valeRules: false }); + const dispatched = await runEngines({ + cwd, + paths: ["app.js", "doc.md"], + astGrepSources: sgSources(cwd), + runtimeRules: [], + }); + expect(dispatched.results.every((result) => result.source !== "vale")).toBe( + true + ); + expect(dispatched.notices).toEqual([]); + }); +}); + +describe("runEngines when Vale is unavailable", () => { + it("still returns ast-grep results, and notices rather than fails", async () => { + // The requirement in one test: `.taskless/vale/` has rules, the binary is + // absent, and the check still reports what ast-grep found. + const binary = await import("../src/rules/vale/binary"); + vi.spyOn(binary, "findValeBinary").mockReturnValue({ + path: undefined, + tried: ["@taskless/vale-darwin-arm64", "PATH"], + }); + + const cwd = makeMixedProject(); + const dispatched = await runEngines({ + cwd, + paths: ["app.js", "doc.md"], + astGrepSources: sgSources(cwd), + runtimeRules: [], + }); + + expect( + dispatched.results.some((result) => result.source === "ast-grep") + ).toBe(true); + expect(dispatched.notices).toHaveLength(1); + expect(dispatched.notices[0]).toContain("Vale binary not found"); + // A skip, not a failure: the exit code is unaffected. + expect(dispatched.failures).toEqual([]); + expect(deriveExitCode(dispatched)).toBe(0); + }); + + it("keeps a thrown engine from discarding the others' results", async () => { + // allSettled, not all: `all` rejects on the first rejection and abandons + // the rest, so one engine throwing would throw away findings the others + // had already produced. + const scan = await import("../src/rules/scan"); + vi.spyOn(scan, "runAstGrepScan").mockRejectedValue( + new Error("ast-grep exploded") + ); + + const cwd = makeMixedProject(); + const dispatched = await runEngines({ + cwd, + paths: ["doc.md"], + astGrepSources: sgSources(cwd), + runtimeRules: [], + }); + + // Vale's findings survive the other engine's rejection... + expect(dispatched.results.some((result) => result.source === "vale")).toBe( + true + ); + // ...and the thrown engine is reported as a failure rather than swallowed. + expect(dispatched.failures).toHaveLength(1); + expect(dispatched.failures[0]).toContain("ast-grep exploded"); + expect(deriveExitCode(dispatched)).toBe(1); + }); +}); From 378a1d7bde0d72cca9ebcedd6ca6a9d942553da7 Mon Sep 17 00:00:00 2001 From: Jakob Heuser Date: Tue, 11 Aug 2026 17:12:25 -0700 Subject: [PATCH 2/6] fix(cli): run Vale for a project whose only rules are Vale's The "no rules configured" gate asked ast-grep and the runtime harness and returned before `runEngines`, so a project with only `.taskless/vale/rules/` reported itself unconfigured and never dispatched the engine this stack just gave an executor. Ask Vale too, short-circuited so the ordinary project pays nothing extra. --- packages/cli/src/commands/check.ts | 23 ++++++++++++++++++----- packages/cli/test/check.test.ts | 24 ++++++++++++++++++++++++ 2 files changed, 42 insertions(+), 5 deletions(-) diff --git a/packages/cli/src/commands/check.ts b/packages/cli/src/commands/check.ts index 4a6d2dbc..00330f23 100644 --- a/packages/cli/src/commands/check.ts +++ b/packages/cli/src/commands/check.ts @@ -2,7 +2,7 @@ import { resolve, join, isAbsolute, relative } from "node:path"; import { stat } from "node:fs/promises"; import { defineCommand } from "citty"; -import { deriveExitCode, runEngines } from "../rules/dispatch"; +import { deriveExitCode, hasValeRules, runEngines } from "../rules/dispatch"; import { formatText } from "../util/format"; import { resolveSgConfigPath } from "../filesystem/sgconfig"; import { ensureTasklessDirectory } from "../filesystem/directory"; @@ -327,9 +327,11 @@ export const checkCommand = defineCommand({ const dispatch = await planEngineDispatch(cwd); // Static rules (trusted ast-grep YAML) always run; runtime rules - // (untrusted check.ts) are gated separately. An engine directory this CLI - // has no executor for (vale) contributes nothing, and a directory that is - // not a known engine is ignored rather than handed to someone's parser. + // (untrusted check.ts) are gated separately. Vale is discovered below, + // in the "anything to run?" gate — every known engine now has an + // executor, so none of them can be assumed to contribute nothing. A + // directory that is not a known engine is still ignored rather than + // handed to someone's parser. const astGrepSources = await discoverAstGrepRuleSources(cwd); // Both halves matter: `executor` alone is read from the static layout // table and is therefore always `runtime-harness`, so gating on it only @@ -344,7 +346,18 @@ export const checkCommand = defineCommand({ ? await discoverRuntimeRules(cwd) : []; - if (astGrepSources.length === 0 && runtimeRules.length === 0) { + // "No rules configured" has to mean *no engine* has any, not just these + // two: a project whose only rules live in `.taskless/vale/rules/` would + // otherwise return here and Vale would never be dispatched, which is a + // silent skip of the engine the user actually configured. Asked last and + // short-circuited, so the ordinary project with ast-grep or runtime rules + // pays nothing and `runEngines` still owns the decision to spawn Vale. + const noRuleFiles = + astGrepSources.length === 0 && + runtimeRules.length === 0 && + !(await hasValeRules(cwd)); + + if (noRuleFiles) { if (args.json) { console.log( JSON.stringify( diff --git a/packages/cli/test/check.test.ts b/packages/cli/test/check.test.ts index 9c1e8f5f..827f004c 100644 --- a/packages/cli/test/check.test.ts +++ b/packages/cli/test/check.test.ts @@ -76,6 +76,30 @@ describe("check", () => { expect(stdout).toContain("No rules configured"); }); + it("does not report a Vale-only project as having no rules", async () => { + // Vale rules alone are rules. Gating the "nothing to run" message on + // ast-grep and runtime rules only returned before the engines were ever + // dispatched, so a project whose rules are all Vale's ran nothing and read + // as unconfigured. Asserted on the message rather than on findings so the + // test does not depend on the optional Vale binary being installed. + await mkdir(join(temporaryDirectory, ".taskless", "vale", "rules"), { + recursive: true, + }); + await writeFile( + join(temporaryDirectory, ".taskless", "vale", "rules", "no-simply.yml"), + `extends: existence\nmessage: "Avoid 'simply'"\nlevel: warning\ntokens:\n - simply\n` + ); + await writeFile( + join(temporaryDirectory, ".taskless", "vale", ".vale.ini"), + "StylesPath = .\nMinAlertLevel = suggestion\n\n[*.md]\nBasedOnStyles =\nrules.no-simply = YES\n" + ); + await writeFile(join(temporaryDirectory, "doc.md"), "Just simply do it.\n"); + + const { stdout } = await runCli(["check", "-d", temporaryDirectory]); + + expect(stdout).not.toContain("No rules configured"); + }); + it("runs scanner and produces human output for rule matches", async () => { await cp(fixturesDirectory, temporaryDirectory, { recursive: true }); From 65b53be649413a6758ba8b3073f1bc947edf5730 Mon Sep 17 00:00:00 2001 From: Jakob Heuser Date: Tue, 11 Aug 2026 17:17:13 -0700 Subject: [PATCH 3/6] fix(cli): stop hasValeRules from reading an IO error as "no rules" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A blanket catch answered `false` for any readdir failure, so an unreadable `.taskless/vale/rules/` skipped Vale with no notice and no failure — the silent-disable the failure/notice split exists to prevent. Only ENOENT and ENOTDIR mean absence now; anything else propagates and `runEngines` reports it as an engine failure. Also makes the allSettled isolation test portable: it asserted a Vale result over a real run, so it only passed on a machine that happened to have the optional binary. Vale is mocked to a deterministic outcome instead, keeping the behavior under test exercised everywhere. --- packages/cli/src/rules/dispatch.ts | 15 +++- packages/cli/test/vale-orchestration.test.ts | 90 +++++++++++++++++++- 2 files changed, 99 insertions(+), 6 deletions(-) diff --git a/packages/cli/src/rules/dispatch.ts b/packages/cli/src/rules/dispatch.ts index 8484c259..e5fae9b1 100644 --- a/packages/cli/src/rules/dispatch.ts +++ b/packages/cli/src/rules/dispatch.ts @@ -13,6 +13,9 @@ import type { RuntimeRule } from "./runtime/discover"; import { runAstGrepScan } from "./scan"; import { isValeFailure, runVale } from "./vale/run"; +/** Errno values that mean "the directory is not there", and nothing worse. */ +const ABSENT_DIRECTORY_CODES = new Set(["ENOENT", "ENOTDIR"]); + /** * Whether `.taskless/vale/rules/` holds anything to run. * @@ -21,6 +24,12 @@ import { isValeFailure, runVale } from "./vale/run"; * nothing: a scaffolded-but-empty engine directory is the common state after * `taskless init`, and spawning a subprocess per check to confirm it found * nothing is pure cost. + * + * Only absence is swallowed. A blanket `catch` here would read an unreadable + * rules directory (`EACCES`, a bad mount) as "no rules" and skip Vale with no + * notice and no failure — the same silent-disable that {@link isValeFailure} + * exists to prevent one file over. Anything that is not absence propagates, so + * `runEngines` reports it as an engine failure rather than a clean run. */ export async function hasValeRules(cwd: string): Promise { try { @@ -28,8 +37,10 @@ export async function hasValeRules(cwd: string): Promise { join(cwd, ".taskless", ENGINE_LAYOUTS.vale.rulesDirectory) ); return entries.some((entry) => entry.endsWith(".yml")); - } catch { - return false; + } catch (error) { + const code = (error as NodeJS.ErrnoException).code; + if (code !== undefined && ABSENT_DIRECTORY_CODES.has(code)) return false; + throw error; } } diff --git a/packages/cli/test/vale-orchestration.test.ts b/packages/cli/test/vale-orchestration.test.ts index 7a7d13c5..0d2ea359 100644 --- a/packages/cli/test/vale-orchestration.test.ts +++ b/packages/cli/test/vale-orchestration.test.ts @@ -1,4 +1,10 @@ -import { mkdirSync, mkdtempSync, rmSync, writeFileSync } from "node:fs"; +import { + chmodSync, + mkdirSync, + mkdtempSync, + rmSync, + writeFileSync, +} from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; @@ -14,6 +20,14 @@ import { findValeBinary } from "../src/rules/vale/binary"; const withVale = findValeBinary().path === undefined ? describe.skip : describe; +/** + * Tests that need mode bits to actually deny a read. Windows does not honour + * them and root bypasses them, so the directory would stay readable and the + * test would assert nothing. + */ +const readableModes = + process.platform === "win32" || process.getuid?.() === 0 ? it.skip : it; + const workspaces: string[] = []; afterEach(() => { vi.restoreAllMocks(); @@ -88,6 +102,24 @@ describe("hasValeRules", () => { it("is true once a rule file exists", async () => { expect(await hasValeRules(makeMixedProject())).toBe(true); }); + + readableModes( + "propagates a rules directory that exists but cannot be read", + async () => { + // Only absence means "no rules". An unreadable directory answered + // `false` would skip Vale with no notice and no failure, which is the + // silent-disable the engine's failure/notice split exists to prevent. + const cwd = makeMixedProject(); + const rules = join(cwd, ".taskless", "vale", "rules"); + chmodSync(rules, 0o000); + try { + await expect(hasValeRules(cwd)).rejects.toThrow(/EACCES|EPERM/); + } finally { + // Restore before teardown, or the workspace cannot be removed. + chmodSync(rules, 0o755); + } + } + ); }); describe("deriveExitCode", () => { @@ -207,6 +239,30 @@ describe("runEngines when Vale is unavailable", () => { new Error("ast-grep exploded") ); + // Vale is mocked rather than run: what is under test is that one engine's + // rejection does not discard another's results, which has nothing to do + // with whether the optional Vale binary is installed. Left real, this + // asserted `source === "vale"` on every machine but only passed on the + // ones that happened to have the binary. + const run = await import("../src/rules/vale/run"); + vi.spyOn(run, "runVale").mockResolvedValue({ + status: "ok", + results: [ + { + source: "vale", + ruleId: "mocked-vale-rule", + severity: "warning", + message: "Avoid 'simply'", + file: "doc.md", + range: { + start: { line: 1, column: 6 }, + end: { line: 1, column: 12 }, + }, + matchedText: "simply", + }, + ], + }); + const cwd = makeMixedProject(); const dispatched = await runEngines({ cwd, @@ -216,12 +272,38 @@ describe("runEngines when Vale is unavailable", () => { }); // Vale's findings survive the other engine's rejection... - expect(dispatched.results.some((result) => result.source === "vale")).toBe( - true - ); + expect( + dispatched.results.some((result) => result.ruleId === "mocked-vale-rule") + ).toBe(true); // ...and the thrown engine is reported as a failure rather than swallowed. expect(dispatched.failures).toHaveLength(1); expect(dispatched.failures[0]).toContain("ast-grep exploded"); expect(deriveExitCode(dispatched)).toBe(1); }); + + readableModes( + "reports an unreadable Vale rules directory as an engine failure", + async () => { + // The other half of the rule above: the throw from discovery reaches + // `failures` and the exit code, instead of Vale quietly contributing + // nothing and the run reading as clean. + const cwd = makeMixedProject(); + const rules = join(cwd, ".taskless", "vale", "rules"); + chmodSync(rules, 0o000); + try { + const dispatched = await runEngines({ + cwd, + paths: ["app.js", "doc.md"], + astGrepSources: sgSources(cwd), + runtimeRules: [], + }); + + expect(dispatched.failures).toHaveLength(1); + expect(dispatched.failures[0]).toContain("vale engine failed"); + expect(deriveExitCode(dispatched)).toBe(1); + } finally { + chmodSync(rules, 0o755); + } + } + ); }); From 5e74df9b960f9be23903a0d10834e4f7647948c3 Mon Sep 17 00:00:00 2001 From: Jakob Heuser Date: Tue, 11 Aug 2026 17:40:15 -0700 Subject: [PATCH 4/6] ref(cli): read Vale's severity off the outcome instead of asking a helper `isValeFailure(outcome)` was a free function a caller had to remember to call; `ValeRunOutcome` now carries `blocking` as a literal-typed field per variant, so dispatch reads the engine's own account of how bad its trouble is. The mistake the helper invited -- writing the natural-looking `outcome.status !== "ok"` and failing `check` on every host missing the Vale binary -- is now a type error rather than a silent behaviour change. That is the point of the shape, beyond this one call site: every engine runs a binary and returns a self-describing outcome, so the next lint engine answers "is this fatal?" the same way and no dispatcher grows a per-engine special case. The migration had to land here rather than with the field: `dispatch.ts` does not exist on the branch that defines `ValeRunOutcome`, so the field only became reachable once the rebase brought it up. Caught by the literal typing on the way through: the mocked `ok` outcome in `vale-orchestration.test.ts` predated the field and failed to compile, which is the check working as intended. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Jwc9FFroR3mTZ4hLiSkkX3 --- packages/cli/src/rules/dispatch.ts | 18 ++++++++++++------ packages/cli/test/vale-orchestration.test.ts | 1 + 2 files changed, 13 insertions(+), 6 deletions(-) diff --git a/packages/cli/src/rules/dispatch.ts b/packages/cli/src/rules/dispatch.ts index e5fae9b1..50b17c53 100644 --- a/packages/cli/src/rules/dispatch.ts +++ b/packages/cli/src/rules/dispatch.ts @@ -11,7 +11,7 @@ import { import { executeRuntimeRules } from "./runtime/harness"; import type { RuntimeRule } from "./runtime/discover"; import { runAstGrepScan } from "./scan"; -import { isValeFailure, runVale } from "./vale/run"; +import { runVale } from "./vale/run"; /** Errno values that mean "the directory is not there", and nothing worse. */ const ABSENT_DIRECTORY_CODES = new Set(["ENOENT", "ENOTDIR"]); @@ -27,9 +27,10 @@ const ABSENT_DIRECTORY_CODES = new Set(["ENOENT", "ENOTDIR"]); * * Only absence is swallowed. A blanket `catch` here would read an unreadable * rules directory (`EACCES`, a bad mount) as "no rules" and skip Vale with no - * notice and no failure — the same silent-disable that {@link isValeFailure} - * exists to prevent one file over. Anything that is not absence propagates, so - * `runEngines` reports it as an engine failure rather than a clean run. + * notice and no failure — the same silent-disable that `ValeRunOutcome`'s + * `blocking` field exists to prevent one file over. Anything that is not + * absence propagates, so `runEngines` reports it as an engine failure rather + * than a clean run. */ export async function hasValeRules(cwd: string): Promise { try { @@ -106,12 +107,17 @@ async function runAstGrepEngine( /** * Vale, when it has rules to run. * - * The three non-ok outcomes divide along the line `isValeFailure` draws: an + * The three non-ok outcomes divide along the line `outcome.blocking` draws: an * absent binary is a notice, because an unsupported arch is an ordinary state * and failing there would make `check` unrunnable on a machine where the other * engines work; a timeout or a crash is a failure, because Vale was present and * asked to work, and reporting that as a skip lets a broken rule file read as * "no Vale findings". + * + * Reading the severity off the outcome rather than asking a helper is the point + * of that field: an engine reports how bad its own trouble is, and a caller + * cannot forget to ask. Every engine we add answers the same question the same + * way. */ async function runValeEngine(options: DispatchOptions): Promise { if (!(await hasValeRules(options.cwd))) { @@ -127,7 +133,7 @@ async function runValeEngine(options: DispatchOptions): Promise { if (outcome.status === "ok") { return { engine: "vale", results: outcome.results }; } - return isValeFailure(outcome) + return outcome.blocking ? { engine: "vale", results: [], failure: outcome.message } : { engine: "vale", results: [], notice: outcome.message }; } diff --git a/packages/cli/test/vale-orchestration.test.ts b/packages/cli/test/vale-orchestration.test.ts index 0d2ea359..1965ef0a 100644 --- a/packages/cli/test/vale-orchestration.test.ts +++ b/packages/cli/test/vale-orchestration.test.ts @@ -247,6 +247,7 @@ describe("runEngines when Vale is unavailable", () => { const run = await import("../src/rules/vale/run"); vi.spyOn(run, "runVale").mockResolvedValue({ status: "ok", + blocking: false, results: [ { source: "vale", From 1774645bdda943fd27c349c02be83b113a9365e7 Mon Sep 17 00:00:00 2001 From: Jakob Heuser Date: Wed, 12 Aug 2026 12:03:11 -0700 Subject: [PATCH 5/6] ref(cli): pass dispatch the ast-grep configs it actually runs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `DispatchOptions.astGrepSources` carried a full `AstGrepRuleSource` beside each resolved `configPath`, and nothing in dispatch ever read it — `runAstGrepEngine` destructures `configPath` and discards the rest. Narrow the field to the `string[]` of config paths dispatch uses. The caller loses a hop (mapping sources to `{ source, configPath }` pairs becomes resolving the paths), and the tests stop constructing a six-field rule source whose only load-bearing member was one string literal. Also collect notices and failures with `flatMap(... ?? [])` rather than `map().filter()` with a hand-written type predicate, matching the `results` line directly above them. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Jwc9FFroR3mTZ4hLiSkkX3 --- packages/cli/src/commands/check.ts | 9 +++----- packages/cli/src/rules/dispatch.ts | 24 ++++++++------------ packages/cli/test/vale-orchestration.test.ts | 24 ++++++-------------- 3 files changed, 19 insertions(+), 38 deletions(-) diff --git a/packages/cli/src/commands/check.ts b/packages/cli/src/commands/check.ts index 00330f23..1013a774 100644 --- a/packages/cli/src/commands/check.ts +++ b/packages/cli/src/commands/check.ts @@ -389,16 +389,13 @@ export const checkCommand = defineCommand({ // Every engine runs concurrently and merges into one result set. An // engine that cannot run reports a notice and the others still return. - const resolvedSources = await Promise.all( - astGrepSources.map(async (source) => ({ - source, - configPath: await resolveSgConfigPath(cwd, source), - })) + const astGrepConfigPaths = await Promise.all( + astGrepSources.map((source) => resolveSgConfigPath(cwd, source)) ); const dispatched = await runEngines({ cwd, paths: existingPaths, - astGrepSources: resolvedSources, + astGrepConfigPaths, runtimeRules: plan.execute, runtimeTimeoutMs: parseTimeoutMs(args.timeout), }); diff --git a/packages/cli/src/rules/dispatch.ts b/packages/cli/src/rules/dispatch.ts index 50b17c53..85e07007 100644 --- a/packages/cli/src/rules/dispatch.ts +++ b/packages/cli/src/rules/dispatch.ts @@ -2,12 +2,7 @@ import { readdir } from "node:fs/promises"; import { join } from "node:path"; import type { CheckResult } from "../types/check"; -import { - dedupeFindings, - ENGINE_LAYOUTS, - type AstGrepRuleSource, - type EngineName, -} from "./engines"; +import { dedupeFindings, ENGINE_LAYOUTS, type EngineName } from "./engines"; import { executeRuntimeRules } from "./runtime/harness"; import type { RuntimeRule } from "./runtime/discover"; import { runAstGrepScan } from "./scan"; @@ -65,8 +60,11 @@ export interface DispatchOptions { cwd: string; /** Target paths, already filtered to those that exist. */ paths: string[]; - /** ast-grep sources, each with the config that scans it. */ - astGrepSources: Array<{ source: AstGrepRuleSource; configPath: string }>; + /** + * One `--config` path per ast-grep rule source, already resolved. The source + * each was derived from is the caller's concern; dispatch only runs configs. + */ + astGrepConfigPaths: string[]; /** Runtime rules that survived planning. Empty means the harness is skipped. */ runtimeRules: RuntimeRule[]; runtimeTimeoutMs?: number; @@ -95,7 +93,7 @@ async function runAstGrepEngine( options: DispatchOptions ): Promise { const results: CheckResult[] = []; - for (const { configPath } of options.astGrepSources) { + for (const configPath of options.astGrepConfigPaths) { const scan = await runAstGrepScan(options.cwd, options.paths, { configPath, }); @@ -196,12 +194,8 @@ export async function runEngines( return { results: outcomes.flatMap((outcome) => outcome.results), - notices: outcomes - .map((outcome) => outcome.notice) - .filter((notice): notice is string => notice !== undefined), - failures: outcomes - .map((outcome) => outcome.failure) - .filter((failure): failure is string => failure !== undefined), + notices: outcomes.flatMap((outcome) => outcome.notice ?? []), + failures: outcomes.flatMap((outcome) => outcome.failure ?? []), outcomes, }; } diff --git a/packages/cli/test/vale-orchestration.test.ts b/packages/cli/test/vale-orchestration.test.ts index 1965ef0a..4ad17c33 100644 --- a/packages/cli/test/vale-orchestration.test.ts +++ b/packages/cli/test/vale-orchestration.test.ts @@ -77,18 +77,8 @@ function makeMixedProject(options?: { valeRules?: boolean }) { return cwd; } -const sgSources = (cwd: string) => [ - { - source: { - rulesDirectory: "sg/rules", - ruleTestsDirectory: "sg/rule-tests", - absoluteRulesDirectory: join(cwd, ".taskless", "sg", "rules"), - ruleIds: ["no-eval"], - legacy: false, - }, - configPath: ".taskless/sg/sgconfig.yml", - }, -]; +/** The committed config `makeMixedProject` writes, as `check` would resolve it. */ +const sgConfigPaths = [".taskless/sg/sgconfig.yml"]; describe("hasValeRules", () => { it("is false for a scaffolded-but-empty rules directory", async () => { @@ -175,7 +165,7 @@ withVale("runEngines over a mixed corpus", () => { const dispatched = await runEngines({ cwd, paths: ["app.js", "doc.md"], - astGrepSources: sgSources(cwd), + astGrepConfigPaths: sgConfigPaths, runtimeRules: [], }); @@ -192,7 +182,7 @@ withVale("runEngines over a mixed corpus", () => { const dispatched = await runEngines({ cwd, paths: ["app.js", "doc.md"], - astGrepSources: sgSources(cwd), + astGrepConfigPaths: sgConfigPaths, runtimeRules: [], }); expect(dispatched.results.every((result) => result.source !== "vale")).toBe( @@ -216,7 +206,7 @@ describe("runEngines when Vale is unavailable", () => { const dispatched = await runEngines({ cwd, paths: ["app.js", "doc.md"], - astGrepSources: sgSources(cwd), + astGrepConfigPaths: sgConfigPaths, runtimeRules: [], }); @@ -268,7 +258,7 @@ describe("runEngines when Vale is unavailable", () => { const dispatched = await runEngines({ cwd, paths: ["doc.md"], - astGrepSources: sgSources(cwd), + astGrepConfigPaths: sgConfigPaths, runtimeRules: [], }); @@ -295,7 +285,7 @@ describe("runEngines when Vale is unavailable", () => { const dispatched = await runEngines({ cwd, paths: ["app.js", "doc.md"], - astGrepSources: sgSources(cwd), + astGrepConfigPaths: sgConfigPaths, runtimeRules: [], }); From 5d8d080e73786cbb1659da972b8cc81e8d20481d Mon Sep 17 00:00:00 2001 From: Jakob Heuser Date: Wed, 12 Aug 2026 12:41:43 -0700 Subject: [PATCH 6/6] ref(cli): carry the exit code on DispatchResult instead of deriving it `deriveExitCode(dispatched)` was a pure function every caller had to remember to call, computing a value that is fixed the moment the engines settle and never changes afterwards. Nothing about it varied per caller, so it was derivation for a thing that only needs computing once. `runEngines` now computes it and `DispatchResult` carries `exitCode`. Beyond removing the call, this removes the possibility of two callers disagreeing about what counts as failure -- the rule that an engine failure fails the check even with zero findings now lives with the data rather than with whoever remembers to consult it. The tradeoff is that four unit tests exercised the helper directly on a hand-built `DispatchResult`. They are replaced by tests through `runEngines`, which is the path the exit code actually takes: warning findings exit 0, an error-severity finding exits 1, a clean run exits 0, an unavailable engine exits 0, and a thrown engine exits 1. The last two already ran through `runEngines` and only needed the field. To cover the error case the sg fixture severity became a parameter. Net one test fewer, covering the same five paths against real dispatch rather than a constructed struct. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Jwc9FFroR3mTZ4hLiSkkX3 --- packages/cli/src/commands/check.ts | 9 +- packages/cli/src/rules/dispatch.ts | 40 ++++---- packages/cli/test/vale-orchestration.test.ts | 97 ++++++++++---------- 3 files changed, 76 insertions(+), 70 deletions(-) diff --git a/packages/cli/src/commands/check.ts b/packages/cli/src/commands/check.ts index 1013a774..40b19627 100644 --- a/packages/cli/src/commands/check.ts +++ b/packages/cli/src/commands/check.ts @@ -2,7 +2,7 @@ import { resolve, join, isAbsolute, relative } from "node:path"; import { stat } from "node:fs/promises"; import { defineCommand } from "citty"; -import { deriveExitCode, hasValeRules, runEngines } from "../rules/dispatch"; +import { hasValeRules, runEngines } from "../rules/dispatch"; import { formatText } from "../util/format"; import { resolveSgConfigPath } from "../filesystem/sgconfig"; import { ensureTasklessDirectory } from "../filesystem/directory"; @@ -412,9 +412,10 @@ export const checkCommand = defineCommand({ } scanCounts = { errorCount, warningCount, findings: results.length }; - // An engine failure fails the check even with no findings: a Vale that - // timed out reports nothing, which would otherwise read as clean. - const exitCode = deriveExitCode(dispatched); + // Computed by `runEngines`, not here: the exit code is a fact about a + // completed dispatch, and an engine failure has to fail the check even + // with no findings. + const { exitCode } = dispatched; if (args.json) { const output = checkOutputSchema.parse({ diff --git a/packages/cli/src/rules/dispatch.ts b/packages/cli/src/rules/dispatch.ts index 85e07007..ce9e49a5 100644 --- a/packages/cli/src/rules/dispatch.ts +++ b/packages/cli/src/rules/dispatch.ts @@ -80,6 +80,20 @@ export interface DispatchResult { failures: string[]; /** Per-engine detail, for callers that report engine by engine. */ outcomes: EngineOutcome[]; + /** + * The process exit code this run implies. + * + * Two independent reasons to fail, and both are needed. An error-severity + * finding is the ordinary one. An engine failure is the one that is easy to + * miss: 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. + * + * Carried on the result rather than derived by each caller. It is a fact + * about a completed dispatch, fixed the moment the engines settle, so + * computing it once here removes the chance of two callers disagreeing about + * what counts as failure. + */ + exitCode: number; } /** @@ -192,25 +206,19 @@ export async function runEngines( }; }); + const results = outcomes.flatMap((outcome) => outcome.results); + const failures = outcomes.flatMap((outcome) => outcome.failure ?? []); + return { - results: outcomes.flatMap((outcome) => outcome.results), + results, notices: outcomes.flatMap((outcome) => outcome.notice ?? []), - failures: outcomes.flatMap((outcome) => outcome.failure ?? []), + failures, outcomes, + exitCode: + results.some((finding) => finding.severity === "error") || + failures.length > 0 + ? 1 + : 0, }; } -/** - * The exit code for a completed check. - * - * Two independent reasons to fail, and both are needed. An error-severity - * finding is the ordinary one. An engine failure is the one that is easy to - * miss: a Vale that timed out or rejected its config produces no findings, so - * without this a broken engine exits 0 and reads exactly like a clean run. - */ -export function deriveExitCode(result: DispatchResult): number { - const hasErrorFinding = result.results.some( - (finding) => finding.severity === "error" - ); - return hasErrorFinding || result.failures.length > 0 ? 1 : 0; -} diff --git a/packages/cli/test/vale-orchestration.test.ts b/packages/cli/test/vale-orchestration.test.ts index 4ad17c33..e1a21f46 100644 --- a/packages/cli/test/vale-orchestration.test.ts +++ b/packages/cli/test/vale-orchestration.test.ts @@ -10,12 +10,7 @@ import { join } from "node:path"; import { afterEach, describe, expect, it, vi } from "vitest"; -import { - deriveExitCode, - hasValeRules, - runEngines, - type DispatchResult, -} from "../src/rules/dispatch"; +import { hasValeRules, runEngines } from "../src/rules/dispatch"; import { findValeBinary } from "../src/rules/vale/binary"; const withVale = findValeBinary().path === undefined ? describe.skip : describe; @@ -37,8 +32,12 @@ afterEach(() => { }); /** A project with an sg rule, a vale rule, and a document tripping both. */ -function makeMixedProject(options?: { valeRules?: boolean }) { +function makeMixedProject(options?: { + valeRules?: boolean; + sgSeverity?: "warning" | "error"; +}) { const valeRules = options?.valeRules ?? true; + const sgSeverity = options?.sgSeverity ?? "warning"; const cwd = mkdtempSync(join(tmpdir(), "vale-orch-")); workspaces.push(cwd); @@ -48,7 +47,7 @@ function makeMixedProject(options?: { valeRules?: boolean }) { [ "id: no-eval", "language: javascript", - "severity: warning", + `severity: ${sgSeverity}`, "message: Avoid eval", "rule:", " pattern: eval($$$ARGS)", @@ -112,50 +111,48 @@ describe("hasValeRules", () => { ); }); -describe("deriveExitCode", () => { - const base: DispatchResult = { - results: [], - notices: [], - failures: [], - outcomes: [], - }; - - it("is 0 for a clean run", () => { - expect(deriveExitCode(base)).toBe(0); - }); +describe("exit code carried on the dispatch result", () => { + // Exercised through `runEngines` rather than against a pure helper, because + // the exit code is now a property of a completed dispatch. These use no Vale + // rules, so they run on every host regardless of the optional binary. - it("is 0 when an engine is merely unavailable", () => { - // A notice is advisory. An unsupported arch must not fail a check the - // other engines completed. - expect(deriveExitCode({ ...base, notices: ["vale unavailable"] })).toBe(0); + it("is 0 when every finding is a warning", async () => { + const cwd = makeMixedProject({ valeRules: false }); + const dispatched = await runEngines({ + cwd, + paths: ["app.js"], + astGrepConfigPaths: sgConfigPaths, + runtimeRules: [], + }); + expect(dispatched.results.length).toBeGreaterThan(0); + expect(dispatched.exitCode).toBe(0); }); - it("is 1 for an engine failure even with no findings", () => { - // The case that is easy to miss: a Vale that timed out reports nothing, so - // without this a broken engine exits 0 and reads exactly like a clean run. - expect(deriveExitCode({ ...base, failures: ["vale timed out"] })).toBe(1); + it("is 1 for an error-severity finding", async () => { + const cwd = makeMixedProject({ valeRules: false, sgSeverity: "error" }); + const dispatched = await runEngines({ + cwd, + paths: ["app.js"], + astGrepConfigPaths: sgConfigPaths, + runtimeRules: [], + }); + expect( + dispatched.results.some((finding) => finding.severity === "error") + ).toBe(true); + expect(dispatched.exitCode).toBe(1); }); - it("ignores warning-severity findings", () => { - expect( - deriveExitCode({ - ...base, - results: [ - { - source: "vale", - ruleId: "r", - severity: "warning", - message: "m", - file: "a.md", - range: { - start: { line: 1, column: 1 }, - end: { line: 1, column: 2 }, - }, - matchedText: "x", - }, - ], - }) - ).toBe(0); + it("is 0 for a clean run with nothing to report", async () => { + const cwd = makeMixedProject({ valeRules: false }); + const dispatched = await runEngines({ + cwd, + paths: ["doc.md"], // the sg rule is javascript-only, so nothing matches + astGrepConfigPaths: sgConfigPaths, + runtimeRules: [], + }); + expect(dispatched.results).toEqual([]); + expect(dispatched.failures).toEqual([]); + expect(dispatched.exitCode).toBe(0); }); }); @@ -217,7 +214,7 @@ describe("runEngines when Vale is unavailable", () => { expect(dispatched.notices[0]).toContain("Vale binary not found"); // A skip, not a failure: the exit code is unaffected. expect(dispatched.failures).toEqual([]); - expect(deriveExitCode(dispatched)).toBe(0); + expect(dispatched.exitCode).toBe(0); }); it("keeps a thrown engine from discarding the others' results", async () => { @@ -269,7 +266,7 @@ describe("runEngines when Vale is unavailable", () => { // ...and the thrown engine is reported as a failure rather than swallowed. expect(dispatched.failures).toHaveLength(1); expect(dispatched.failures[0]).toContain("ast-grep exploded"); - expect(deriveExitCode(dispatched)).toBe(1); + expect(dispatched.exitCode).toBe(1); }); readableModes( @@ -291,7 +288,7 @@ describe("runEngines when Vale is unavailable", () => { expect(dispatched.failures).toHaveLength(1); expect(dispatched.failures[0]).toContain("vale engine failed"); - expect(deriveExitCode(dispatched)).toBe(1); + expect(dispatched.exitCode).toBe(1); } finally { chmodSync(rules, 0o755); }