diff --git a/openspec/changes/add-vale-rule-engine/tasks.md b/openspec/changes/add-vale-rule-engine/tasks.md index 61004a1..ba36968 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 a2709cb..40b1962 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 { hasValeRules, 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. // @@ -330,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 @@ -347,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( @@ -363,22 +373,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 +386,23 @@ 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 astGrepConfigPaths = await Promise.all( + astGrepSources.map((source) => resolveSgConfigPath(cwd, source)) + ); + const dispatched = await runEngines({ + cwd, + paths: existingPaths, + astGrepConfigPaths, + 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 +410,16 @@ 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 }; + // 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({ - success: !hasErrors, + success: exitCode === 0, results, ...(plan.skipped.length > 0 ? { skipped: plan.skipped } : {}), }); @@ -417,9 +428,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 0000000..ce9e49a --- /dev/null +++ b/packages/cli/src/rules/dispatch.ts @@ -0,0 +1,224 @@ +import { readdir } from "node:fs/promises"; +import { join } from "node:path"; + +import type { CheckResult } from "../types/check"; +import { dedupeFindings, ENGINE_LAYOUTS, type EngineName } from "./engines"; +import { executeRuntimeRules } from "./runtime/harness"; +import type { RuntimeRule } from "./runtime/discover"; +import { runAstGrepScan } from "./scan"; +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"]); + +/** + * 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. + * + * 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 `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 { + const entries = await readdir( + join(cwd, ".taskless", ENGINE_LAYOUTS.vale.rulesDirectory) + ); + return entries.some((entry) => entry.endsWith(".yml")); + } catch (error) { + const code = (error as NodeJS.ErrnoException).code; + if (code !== undefined && ABSENT_DIRECTORY_CODES.has(code)) return false; + throw error; + } +} + +/** 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[]; + /** + * 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; + 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[]; + /** + * 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; +} + +/** + * 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.astGrepConfigPaths) { + 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 `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))) { + 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 outcome.blocking + ? { 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) + }`, + }; + }); + + const results = outcomes.flatMap((outcome) => outcome.results); + const failures = outcomes.flatMap((outcome) => outcome.failure ?? []); + + return { + results, + notices: outcomes.flatMap((outcome) => outcome.notice ?? []), + failures, + outcomes, + exitCode: + results.some((finding) => finding.severity === "error") || + failures.length > 0 + ? 1 + : 0, + }; +} + diff --git a/packages/cli/src/rules/engines.ts b/packages/cli/src/rules/engines.ts index 3723233..82ce211 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/check.test.ts b/packages/cli/test/check.test.ts index 9c1e8f5..827f004 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 }); diff --git a/packages/cli/test/engine-dispatch.test.ts b/packages/cli/test/engine-dispatch.test.ts index 717b82e..f06f044 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 0000000..e1a21f4 --- /dev/null +++ b/packages/cli/test/vale-orchestration.test.ts @@ -0,0 +1,297 @@ +import { + chmodSync, + 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 { hasValeRules, runEngines } from "../src/rules/dispatch"; +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(); + 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; + sgSeverity?: "warning" | "error"; +}) { + const valeRules = options?.valeRules ?? true; + const sgSeverity = options?.sgSeverity ?? "warning"; + 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: ${sgSeverity}`, + "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; +} + +/** 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 () => { + // 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); + }); + + 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("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 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 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("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); + }); +}); + +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"], + astGrepConfigPaths: sgConfigPaths, + 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"], + astGrepConfigPaths: sgConfigPaths, + 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"], + astGrepConfigPaths: sgConfigPaths, + 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(dispatched.exitCode).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") + ); + + // 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", + blocking: false, + 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, + paths: ["doc.md"], + astGrepConfigPaths: sgConfigPaths, + runtimeRules: [], + }); + + // Vale's findings survive the other engine's rejection... + 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(dispatched.exitCode).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"], + astGrepConfigPaths: sgConfigPaths, + runtimeRules: [], + }); + + expect(dispatched.failures).toHaveLength(1); + expect(dispatched.failures[0]).toContain("vale engine failed"); + expect(dispatched.exitCode).toBe(1); + } finally { + chmodSync(rules, 0o755); + } + } + ); +});