diff --git a/packages/cli/src/filesystem/migrations/0001-init.ts b/packages/cli/src/filesystem/migrations/0001-init.ts index f1ac49df..fc886f14 100644 --- a/packages/cli/src/filesystem/migrations/0001-init.ts +++ b/packages/cli/src/filesystem/migrations/0001-init.ts @@ -36,7 +36,7 @@ Rules are partitioned by the engine that runs them. Each engine directory holds that tool's own native config, its \`rules/\`, and its \`rule-tests/\`: - \`sg/\` - ast-grep: \`sgconfig.yml\`, generated rules (managed by Taskless), and their pass/fail test cases -- \`vale/\` - Vale prose rules: \`.vale.ini\`. Scaffolded and inert; nothing runs it yet +- \`vale/\` - Vale prose rules: \`.vale.ini\`, \`rules/\`, and their pass/fail fixtures. Run by \`check\` alongside ast-grep - \`runtime/\` - Rules that execute a \`check.ts\`, each in its own \`rules//\` directory `; diff --git a/packages/cli/src/filesystem/migrations/0004-vale-engine.ts b/packages/cli/src/filesystem/migrations/0004-vale-engine.ts index 425a5594..6d7baabf 100644 --- a/packages/cli/src/filesystem/migrations/0004-vale-engine.ts +++ b/packages/cli/src/filesystem/migrations/0004-vale-engine.ts @@ -23,10 +23,19 @@ import { CLIError } from "../../util/cli-error"; const SG_CONFIG_CONTENT = `ruleDirs:\n - rules\ntestConfigs:\n - testDir: rule-tests\n`; /** - * Minimal, inert `.vale.ini`. Nothing executes Vale yet; this exists so the - * engine directory has its native config in the canonical place from day one. + * The scaffolded `.vale.ini`. + * + * `StylesPath` is the engine directory, NOT `rules/`. Vale treats StylesPath as + * a directory *of styles*, so a rule at `vale/rules/no-simply.yml` is the + * `no-simply` rule of the `rules` style, and its check is `rules.no-simply` — + * which is the name `stripRulesPrefix` in `vale/map.ts` exists to undo, and the + * shape `verify.ts` generates. Pointing StylesPath at `rules/` instead makes + * that same file a style directory with no rules in it: every check resolves to + * nothing, Vale reports `{}`, and a prose check passes clean with every rule + * silently disabled. Measured against the real binary, which is the only way + * this is visible — the layout is identical either way. */ -const VALE_CONFIG_CONTENT = `StylesPath = rules\nMinAlertLevel = suggestion\n\n[*]\n`; +const VALE_CONFIG_CONTENT = `StylesPath = .\nMinAlertLevel = suggestion\n\n[*]\n`; /** Directories that must exist after the migration, tracked when empty. */ const SCAFFOLD_DIRECTORIES = [ @@ -247,7 +256,10 @@ async function ensureTrackedDirectory(path: string): Promise { * * `rules/`, `rule-tests/`, and `sgconfig.yml` move under `sg/`; the runtime * tier moves to `runtime/rules/` and `runtime/rule-tests/`; `vale/` is - * scaffolded with its native config but stays inert (no engine reads it yet). + * scaffolded with its native config, which `check` reads and runs alongside + * ast-grep. The scaffold enables no rules, so it is quiet until a user adds + * one — quiet, not inert, and the difference is why `VALE_CONFIG_CONTENT` + * above has to be a config Vale can actually resolve rules under. * * The move edits no file contents. `sgconfig.yml`'s `ruleDirs: [rules]` is * relative to the config file, so it stays valid after the move with no path diff --git a/packages/cli/src/rules/dispatch.ts b/packages/cli/src/rules/dispatch.ts index ce9e49a5..e51d1060 100644 --- a/packages/cli/src/rules/dispatch.ts +++ b/packages/cli/src/rules/dispatch.ts @@ -58,7 +58,20 @@ export interface EngineOutcome { export interface DispatchOptions { cwd: string; - /** Target paths, already filtered to those that exist. */ + /** + * Target paths, already filtered to those that exist. + * + * **Empty means "the whole project", and each engine is responsible for + * expressing that in its own terms.** The engines do not agree on what an + * empty target list means natively: ast-grep takes its targets from the + * config and is content with none, while Vale given no input prints its usage + * text and exits 0. Passing the empty list straight through therefore ran + * ast-grep correctly and reduced Vale to a parse failure on every whole- + * project check — findings silently absent, engine reported as broken. + * + * A new engine must decide what empty means for its own executor rather than + * assuming the caller narrowed it. + */ paths: string[]; /** * One `--config` path per ast-grep rule source, already resolved. The source @@ -221,4 +234,3 @@ export async function runEngines( : 0, }; } - diff --git a/packages/cli/src/rules/vale/run.ts b/packages/cli/src/rules/vale/run.ts index 110d7d21..a252e5a8 100644 --- a/packages/cli/src/rules/vale/run.ts +++ b/packages/cli/src/rules/vale/run.ts @@ -8,8 +8,11 @@ import { buildPath } from "../scan"; import { findValeBinary, valeUnavailableMessage } from "./binary"; import { asValeConfigError, toValeCheckResults, type ValeOutput } from "./map"; +/** Taskless's own directory, as a project-relative path. */ +const TASKLESS_DIRECTORY = ".taskless"; + /** The committed Vale config, relative to the project root. */ -export const COMMITTED_VALE_CONFIG = `.taskless/${ENGINE_LAYOUTS.vale.configFile}`; +export const COMMITTED_VALE_CONFIG = `${TASKLESS_DIRECTORY}/${ENGINE_LAYOUTS.vale.configFile}`; /** * How long a single Vale invocation may run before it is killed. @@ -96,6 +99,29 @@ export async function runVale( const paths = options.paths ?? []; const timeoutMs = options.timeoutMs ?? VALE_TIMEOUT_MS; + // Vale needs somewhere to look. Given no input it prints its usage text and + // exits 0, which reaches the mapper as "not JSON" and reports the engine as + // failed on every run — so a whole-project `check`, which passes no paths at + // all, produced zero Vale findings and one spurious failure. ast-grep is the + // reason this is easy to miss: it takes its targets from the config and is + // content with none, so the two engines disagree about what "no paths" means. + // `cwd` is the project root, so `.` is the whole project. + const wholeProject = paths.length === 0; + const targets = wholeProject ? ["."] : paths; + + // Walking the whole project reaches `.taskless/` too, and Vale has no reason + // to know that directory is ours: with a rule enabled it reports findings in + // the committed `.vale.ini` and in the user's own rule definitions — prose + // complaints about the machinery, pointing at files nobody wrote as prose. + // Section globs do not help, since `.taskless/README.md` matches `[*.md]` as + // readily as any document. `--glob` filters which files are walked without + // touching which rules apply to them, so a user's scoping still decides that. + // + // Applied ONLY when we chose `.` ourselves. An explicit path is a request, + // and silently declining to check a file someone named would be worse than + // checking one they did not. + const exclude = wholeProject ? [`--glob=!${TASKLESS_DIRECTORY}/**`] : []; + // `--` separates flags from positional paths, so a path beginning with `-` // is not read as a flag. const argv = [ @@ -103,7 +129,9 @@ export async function runVale( configPath, "--output=JSON", "--no-exit", - ...(paths.length > 0 ? ["--", ...paths] : []), + ...exclude, + "--", + ...targets, ]; return new Promise((resolve) => { diff --git a/packages/cli/test/fixtures/mixed-engines-project/.taskless/sg/rules/no-console-warn.yml b/packages/cli/test/fixtures/mixed-engines-project/.taskless/sg/rules/no-console-warn.yml new file mode 100644 index 00000000..0df23b69 --- /dev/null +++ b/packages/cli/test/fixtures/mixed-engines-project/.taskless/sg/rules/no-console-warn.yml @@ -0,0 +1,6 @@ +id: no-console-warn +language: javascript +severity: warning +rule: + pattern: console.warn($$$) +message: Prefer a structured logger over console.warn() diff --git a/packages/cli/test/fixtures/mixed-engines-project/.taskless/sg/rules/no-eval.yml b/packages/cli/test/fixtures/mixed-engines-project/.taskless/sg/rules/no-eval.yml new file mode 100644 index 00000000..f7c355ba --- /dev/null +++ b/packages/cli/test/fixtures/mixed-engines-project/.taskless/sg/rules/no-eval.yml @@ -0,0 +1,7 @@ +id: no-eval +language: javascript +severity: error +rule: + pattern: eval($$$) +message: Avoid using eval() +note: eval() is unsafe. Use alternatives like Function() or JSON.parse(). diff --git a/packages/cli/test/fixtures/mixed-engines-project/.taskless/sg/sgconfig.yml b/packages/cli/test/fixtures/mixed-engines-project/.taskless/sg/sgconfig.yml new file mode 100644 index 00000000..098ecb2a --- /dev/null +++ b/packages/cli/test/fixtures/mixed-engines-project/.taskless/sg/sgconfig.yml @@ -0,0 +1,4 @@ +ruleDirs: + - rules +testConfigs: + - testDir: rule-tests diff --git a/packages/cli/test/fixtures/mixed-engines-project/.taskless/taskless.json b/packages/cli/test/fixtures/mixed-engines-project/.taskless/taskless.json new file mode 100644 index 00000000..f13e55ca --- /dev/null +++ b/packages/cli/test/fixtures/mixed-engines-project/.taskless/taskless.json @@ -0,0 +1,3 @@ +{ + "version": 4 +} diff --git a/packages/cli/test/fixtures/mixed-engines-project/.taskless/vale/.vale.ini b/packages/cli/test/fixtures/mixed-engines-project/.taskless/vale/.vale.ini new file mode 100644 index 00000000..0e265ab1 --- /dev/null +++ b/packages/cli/test/fixtures/mixed-engines-project/.taskless/vale/.vale.ini @@ -0,0 +1,7 @@ +StylesPath = . +MinAlertLevel = suggestion + +[*.md] +BasedOnStyles = +rules.no-simply = YES +rules.no-obviously = YES diff --git a/packages/cli/test/fixtures/mixed-engines-project/.taskless/vale/rules/no-obviously.yml b/packages/cli/test/fixtures/mixed-engines-project/.taskless/vale/rules/no-obviously.yml new file mode 100644 index 00000000..df9aec1d --- /dev/null +++ b/packages/cli/test/fixtures/mixed-engines-project/.taskless/vale/rules/no-obviously.yml @@ -0,0 +1,5 @@ +extends: existence +message: "Avoid 'obviously' — it tells the reader they should already know" +level: error +tokens: + - obviously diff --git a/packages/cli/test/fixtures/mixed-engines-project/.taskless/vale/rules/no-simply.yml b/packages/cli/test/fixtures/mixed-engines-project/.taskless/vale/rules/no-simply.yml new file mode 100644 index 00000000..0335a9a3 --- /dev/null +++ b/packages/cli/test/fixtures/mixed-engines-project/.taskless/vale/rules/no-simply.yml @@ -0,0 +1,5 @@ +extends: existence +message: "Avoid 'simply' — it hides the work from the reader" +level: warning +tokens: + - simply diff --git a/packages/cli/test/fixtures/mixed-engines-project/README.md b/packages/cli/test/fixtures/mixed-engines-project/README.md new file mode 100644 index 00000000..f44f1790 --- /dev/null +++ b/packages/cli/test/fixtures/mixed-engines-project/README.md @@ -0,0 +1,5 @@ +# Sample + +This document obviously trips one rule, and simply trips another. + +Nothing else here is objectionable. diff --git a/packages/cli/test/fixtures/mixed-engines-project/sample.js b/packages/cli/test/fixtures/mixed-engines-project/sample.js new file mode 100644 index 00000000..d305bc2c --- /dev/null +++ b/packages/cli/test/fixtures/mixed-engines-project/sample.js @@ -0,0 +1,4 @@ +// Trips the sg engine: one error-severity rule and one warning-severity rule. +const result = eval("2 + 2"); +console.warn("this is a warning"); +console.log("this is fine"); diff --git a/packages/cli/test/mixed-engine-check.test.ts b/packages/cli/test/mixed-engine-check.test.ts new file mode 100644 index 00000000..c60995f2 --- /dev/null +++ b/packages/cli/test/mixed-engine-check.test.ts @@ -0,0 +1,302 @@ +import { execFile } from "node:child_process"; +import { cp, mkdtemp, readFile, rm, writeFile } from "node:fs/promises"; +import { tmpdir } from "node:os"; +import { join, resolve } from "node:path"; +import { promisify } from "node:util"; + +import { afterEach, beforeEach, describe, expect, it } from "vitest"; + +import { findValeBinary } from "../src/rules/vale/binary"; + +const execFileAsync = promisify(execFile); +const binPath = resolve(import.meta.dirname, "../dist/index.js"); +const fixturesDirectory = resolve( + import.meta.dirname, + "fixtures/mixed-engines-project" +); + +/** + * End-to-end over a project both engines have work in. + * + * The unit suites mock at the seam they are testing — `vale-orchestration` + * stubs `runVale`, `vale-run` builds configs in a temp directory. Nothing + * exercised a committed project the way a user has one: both engine directories + * populated, both native configs on disk as authored, and the real binary + * resolved for each. That gap is not theoretical. The scaffolded `.vale.ini` + * carried a `StylesPath` under which no rule could resolve, so Vale reported + * `{}` and a check over prose rules passed clean — invisible to every test that + * generated its own config. + */ + +/** Run the built CLI, tolerating a non-zero exit. */ +async function runCli( + args: string[] +): Promise<{ stdout: string; stderr: string; exitCode: number }> { + try { + const { stdout, stderr } = await execFileAsync("node", [binPath, ...args]); + return { stdout, stderr, exitCode: 0 }; + } catch (error) { + const failure = error as { stdout: string; stderr: string; code: number }; + return { + stdout: failure.stdout ?? "", + stderr: failure.stderr ?? "", + exitCode: failure.code, + }; + } +} + +interface CheckFinding { + source: string; + ruleId: string; + severity: string; + file: string; +} + +interface CheckOutput { + success: boolean; + results: CheckFinding[]; +} + +/** Vale ships per-platform; an unsupported host has none. */ +const withVale = findValeBinary().path === undefined ? describe.skip : describe; + +describe("check over a project with both engines", () => { + let project: string; + + beforeEach(async () => { + project = await mkdtemp(join(tmpdir(), "taskless-mixed-")); + await cp(fixturesDirectory, project, { recursive: true }); + }); + + afterEach(async () => { + await rm(project, { recursive: true, force: true }); + }); + + withVale("with the Vale binary available", () => { + it("reports findings from both engines in one run", async () => { + const { stdout, exitCode } = await runCli([ + "check", + "-d", + project, + "--json", + ]); + const output = JSON.parse(stdout.trim()) as CheckOutput; + const sources = new Set(output.results.map((finding) => finding.source)); + + // The claim this file exists to make: one invocation, both engines. + expect(sources).toContain("ast-grep"); + expect(sources).toContain("vale"); + expect(exitCode).toBe(1); + expect(output.success).toBe(false); + }); + + it("attributes each finding to the engine and file it came from", async () => { + const { stdout } = await runCli(["check", "-d", project, "--json"]); + const output = JSON.parse(stdout.trim()) as CheckOutput; + const byRule = new Map(output.results.map((f) => [f.ruleId, f])); + + // Code rules see the code file, prose rules see the document. A merged + // result set makes crossing them easy to miss, so pin the pairing. + expect(byRule.get("no-eval")).toMatchObject({ + source: "ast-grep", + severity: "error", + file: "sample.js", + }); + expect(byRule.get("no-console-warn")).toMatchObject({ + source: "ast-grep", + severity: "warning", + file: "sample.js", + }); + expect(byRule.get("no-simply")).toMatchObject({ + source: "vale", + severity: "warning", + file: "README.md", + }); + expect(byRule.get("no-obviously")).toMatchObject({ + source: "vale", + severity: "error", + file: "README.md", + }); + }); + + it("strips Vale's styles prefix from the reported rule id", async () => { + // Vale reports `rules.no-simply`, named for the styles directory. A user + // authored `no-simply`, so that is what a finding has to say. + const { stdout } = await runCli(["check", "-d", project, "--json"]); + const output = JSON.parse(stdout.trim()) as CheckOutput; + const valeRules = output.results + .filter((finding) => finding.source === "vale") + .map((finding) => finding.ruleId); + + expect(valeRules.length).toBeGreaterThan(0); + for (const ruleId of valeRules) { + expect(ruleId).not.toContain("rules."); + } + }); + + it("names both engines in human output", async () => { + const { stdout } = await runCli(["check", "-d", project]); + expect(stdout).toContain("no-eval"); + expect(stdout).toContain("no-simply"); + expect(stdout).toContain("sample.js"); + expect(stdout).toContain("README.md"); + }); + + it("fails on a prose rule even when the code is clean", async () => { + // Vale alone must be able to fail a check. Otherwise a prose-only project + // reports success no matter what it says. + await rm(join(project, "sample.js")); + const { stdout, exitCode } = await runCli([ + "check", + "-d", + project, + "--json", + ]); + const output = JSON.parse(stdout.trim()) as CheckOutput; + + // `every` is vacuously true on an empty set, so assert presence first — + // otherwise a Vale that found nothing at all passes this test. + expect(output.results.length).toBeGreaterThan(0); + expect(output.results.every((f) => f.source === "vale")).toBe(true); + expect(exitCode).toBe(1); + }); + + it("still reports code findings when the prose is clean", async () => { + // The mirror of the case above, and the one that would hide a Vale that + // silently found nothing: ast-grep carries the run either way. + await rm(join(project, "README.md")); + const { stdout, exitCode } = await runCli([ + "check", + "-d", + project, + "--json", + ]); + const output = JSON.parse(stdout.trim()) as CheckOutput; + + expect(output.results.length).toBeGreaterThan(0); + expect(output.results.every((f) => f.source === "ast-grep")).toBe(true); + expect(exitCode).toBe(1); + }); + }); + + withVale("never lints Taskless's own directory", () => { + it("reports nothing under .taskless on a whole-project check", async () => { + // Vale has no reason to know `.taskless/` is ours, and a whole-project + // walk reaches it: with a rule enabled it reported findings in the + // committed `.vale.ini` and in the user's own rule definitions — prose + // complaints about the machinery. Section globs do not help, because + // `.taskless/README.md` matches `[*.md]` as readily as any document. + await writeFile( + join(project, ".taskless", "README.md"), + "This readme simply describes things, and obviously so.\n" + ); + + const { stdout } = await runCli(["check", "-d", project, "--json"]); + const output = JSON.parse(stdout.trim()) as CheckOutput; + + // Vale still ran — otherwise this passes for the wrong reason. + expect(output.results.some((f) => f.source === "vale")).toBe(true); + expect( + output.results.filter((f) => f.file.startsWith(".taskless")) + ).toEqual([]); + }); + + it("still checks an explicitly named path inside .taskless", async () => { + // The exclusion is ours, not the user's. Naming a path is a request, and + // silently declining to check a file someone asked for would be worse + // than checking one they did not. + await writeFile( + join(project, ".taskless", "README.md"), + "This readme simply describes things.\n" + ); + + const { stdout } = await runCli([ + "check", + "-d", + project, + "--json", + ".taskless/README.md", + ]); + const output = JSON.parse(stdout.trim()) as CheckOutput; + + expect( + output.results.some( + (f) => f.source === "vale" && f.file === ".taskless/README.md" + ) + ).toBe(true); + }); + }); + + withVale("the scaffolded config a real project starts from", () => { + it("resolves a rule dropped into the scaffolded vale directory", async () => { + // The guard for the bug this file found. `migrate-engine-layout` asserts + // `.vale.ini` EXISTS; it never asked whether a rule under it could + // resolve. It could not — `StylesPath` pointed at `rules/`, making that + // directory a style with no rules in it, so every check resolved to + // nothing and Vale returned `{}`. A user would author a rule, watch + // `rule verify` pass (verify generates its own config), and never see it + // fire in `check`. + // + // Deliberately goes through `init` rather than importing the constant, so + // it tests the config a user actually gets rather than one we assert + // about. + const scaffold = await mkdtemp(join(tmpdir(), "taskless-scaffold-")); + try { + const init = await runCli(["init", "--no-interactive", "-d", scaffold]); + expect(init.exitCode).toBe(0); + + const valeDirectory = join(scaffold, ".taskless", "vale"); + await writeFile( + join(valeDirectory, "rules", "no-simply.yml"), + "extends: existence\nmessage: \"Avoid 'simply'\"\nlevel: warning\ntokens:\n - simply\n" + ); + // Enable it the way a user would: one matcher in the committed config. + const config = await readFile(join(valeDirectory, ".vale.ini"), "utf8"); + await writeFile( + join(valeDirectory, ".vale.ini"), + `${config}\n[*.md]\nBasedOnStyles =\nrules.no-simply = YES\n` + ); + await writeFile(join(scaffold, "doc.md"), "Just simply do it.\n"); + + const { stdout } = await runCli(["check", "-d", scaffold, "--json"]); + const output = JSON.parse(stdout.trim()) as CheckOutput; + + expect( + output.results.some( + (finding) => + finding.source === "vale" && finding.ruleId === "no-simply" + ) + ).toBe(true); + } finally { + await rm(scaffold, { recursive: true, force: true }); + } + }); + }); + + describe("whatever the host provides", () => { + it("reports ast-grep findings regardless of Vale's availability", async () => { + // Deliberately ungated, and deliberately not mocking the binary away: the + // CLI runs as a subprocess here, so a `vi.spyOn` in this process would not + // reach it. What this pins is the property that holds on every host — + // ast-grep carries the run, and an absent Vale cannot take it down with + // it. On a machine with Vale this passes alongside prose findings; on one + // without, it passes with a notice instead. The exit code comes from the + // error-severity code rule either way. + const { stdout, exitCode } = await runCli([ + "check", + "-d", + project, + "--json", + ]); + const output = JSON.parse(stdout.trim()) as CheckOutput; + + expect( + output.results.some( + (finding) => + finding.source === "ast-grep" && finding.ruleId === "no-eval" + ) + ).toBe(true); + expect(exitCode).toBe(1); + }); + }); +});