From e5d24a3d876b638332f4f37aae6d82d0e3fd30b6 Mon Sep 17 00:00:00 2001 From: Jakob Heuser Date: Wed, 12 Aug 2026 19:31:41 -0700 Subject: [PATCH 1/3] test(cli): check a project both engines have work in, and fix what it found The unit suites each mock at the seam they test -- `vale-orchestration` stubs `runVale`, `vale-run` generates a config into a temp directory. Nothing ran 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. Two bugs lived in that gap, and both made Vale silently inert rather than loudly broken. **Vale never ran on a whole-project check.** Given no path arguments Vale prints its usage text and exits 0, which reaches the mapper as "output that is not JSON" and reports the engine failed. `taskless check` with no paths -- the ordinary invocation -- therefore produced zero Vale findings and one spurious engine failure, every time. ast-grep is why this was easy to miss: it takes targets from its config and is content with none, so the two engines disagree about what "no paths" means. `runVale` now defaults to `.`, the project root it already runs in. **The scaffolded `.vale.ini` could not resolve a rule.** Vale reads `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` -- the name `stripRulesPrefix` undoes and the shape `verify.ts` generates. `0004` wrote `StylesPath = rules`, making that directory a style containing no rules: every check resolved to nothing and Vale returned `{}`. The failure shape is the bad one -- `rule verify` passes, because verify generates its own correct config, and the rule then never fires in `check`. The existing migration test asserted `.vale.ini` exists. It never asked whether anything under it could resolve, which is how a config this wrong survived. The new scaffold test goes through `init` rather than importing the constant, and was confirmed to fail against the old value first. Also corrects `0001`'s README, which told users Vale was "inert; nothing runs it yet" -- true when written, false as of this stack. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Jwc9FFroR3mTZ4hLiSkkX3 --- .../src/filesystem/migrations/0001-init.ts | 2 +- .../filesystem/migrations/0004-vale-engine.ts | 15 +- packages/cli/src/rules/vale/run.ts | 11 +- .../.taskless/sg/rules/no-console-warn.yml | 6 + .../.taskless/sg/rules/no-eval.yml | 7 + .../.taskless/sg/sgconfig.yml | 4 + .../.taskless/taskless.json | 3 + .../.taskless/vale/.vale.ini | 7 + .../.taskless/vale/rules/no-obviously.yml | 5 + .../.taskless/vale/rules/no-simply.yml | 5 + .../fixtures/mixed-engines-project/README.md | 5 + .../fixtures/mixed-engines-project/sample.js | 4 + packages/cli/test/mixed-engine-check.test.ts | 254 ++++++++++++++++++ 13 files changed, 323 insertions(+), 5 deletions(-) create mode 100644 packages/cli/test/fixtures/mixed-engines-project/.taskless/sg/rules/no-console-warn.yml create mode 100644 packages/cli/test/fixtures/mixed-engines-project/.taskless/sg/rules/no-eval.yml create mode 100644 packages/cli/test/fixtures/mixed-engines-project/.taskless/sg/sgconfig.yml create mode 100644 packages/cli/test/fixtures/mixed-engines-project/.taskless/taskless.json create mode 100644 packages/cli/test/fixtures/mixed-engines-project/.taskless/vale/.vale.ini create mode 100644 packages/cli/test/fixtures/mixed-engines-project/.taskless/vale/rules/no-obviously.yml create mode 100644 packages/cli/test/fixtures/mixed-engines-project/.taskless/vale/rules/no-simply.yml create mode 100644 packages/cli/test/fixtures/mixed-engines-project/README.md create mode 100644 packages/cli/test/fixtures/mixed-engines-project/sample.js create mode 100644 packages/cli/test/mixed-engine-check.test.ts 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..cc4e52ec 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 = [ diff --git a/packages/cli/src/rules/vale/run.ts b/packages/cli/src/rules/vale/run.ts index 110d7d21..da402912 100644 --- a/packages/cli/src/rules/vale/run.ts +++ b/packages/cli/src/rules/vale/run.ts @@ -98,12 +98,21 @@ export async function runVale( // `--` separates flags from positional paths, so a path beginning with `-` // is not read as a flag. + // 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 targets = paths.length > 0 ? paths : ["."]; const argv = [ "--config", configPath, "--output=JSON", "--no-exit", - ...(paths.length > 0 ? ["--", ...paths] : []), + "--", + ...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..513141ee --- /dev/null +++ b/packages/cli/test/mixed-engine-check.test.ts @@ -0,0 +1,254 @@ +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("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); + }); + }); +}); From 01c4e54928fb01bdee0ae4688f0b47f0fe8907f7 Mon Sep 17 00:00:00 2001 From: Jakob Heuser Date: Wed, 12 Aug 2026 20:18:53 -0700 Subject: [PATCH 2/3] docs(cli): correct 0004's inert claim and pin the empty-paths contract Two comments that were true when written and are false as of this stack, both flagged in review. `0004`'s migration docstring still said `vale/` "stays inert (no engine reads it yet)" -- contradicting the corrected `VALE_CONFIG_CONTENT` comment twelve lines above it and the `0001` README in the same PR. The accurate distinction is quiet, not inert: `check` reads and runs the config, and the scaffold simply enables no rules yet. That difference is the reason the scaffolded config has to be one Vale can resolve rules under at all. `DispatchOptions.paths` never stated that empty means "the whole project", which is precisely the gap the Vale bug fell into: the engines disagree natively about empty input -- ast-grep takes targets from its config and is content with none, Vale prints usage and exits 0 -- so passing it straight through ran one correctly and reduced the other to a parse failure on every whole-project check. Documented on the field a future engine integrator reads, so the next one decides what empty means for its executor rather than assuming the caller narrowed it. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Jwc9FFroR3mTZ4hLiSkkX3 --- .../filesystem/migrations/0004-vale-engine.ts | 5 ++++- packages/cli/src/rules/dispatch.ts | 16 ++++++++++++++-- 2 files changed, 18 insertions(+), 3 deletions(-) diff --git a/packages/cli/src/filesystem/migrations/0004-vale-engine.ts b/packages/cli/src/filesystem/migrations/0004-vale-engine.ts index cc4e52ec..6d7baabf 100644 --- a/packages/cli/src/filesystem/migrations/0004-vale-engine.ts +++ b/packages/cli/src/filesystem/migrations/0004-vale-engine.ts @@ -256,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, }; } - From 63719f37f67716b6c22dd68fab23f7f8b83d12c5 Mon Sep 17 00:00:00 2001 From: Jakob Heuser Date: Wed, 12 Aug 2026 21:08:11 -0700 Subject: [PATCH 3/3] fix(cli): stop a whole-project check from linting .taskless Vale has no reason to know `.taskless/` is ours, and defaulting a pathless check to `.` walks straight into it. With a rule enabled, the first thing a user saw after authoring their first prose rule was findings in the committed `.vale.ini` and in the rule definition they had just written -- prose complaints about the machinery, pointing at files nobody wrote as prose. Section globs cannot fix this: `.taskless/README.md` matches `[*.md]` as readily as any document, so even a correctly scoped config lints it. `--glob=!.taskless/**` filters which files Vale walks without touching which rules apply to them -- measured, with a config scoped to `[*.md]` and a `.txt` file present, the user's scoping still decides what is checked and only the exclusion changes. 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 -- covered by its own test. Split out of #101 rather than left there. That issue bundled this with the question of whether to exclude build output and vendored trees, and those are not the same kind of problem: excluding `dist/` is a product default someone could reasonably disagree with, while Taskless linting its own metadata is wrong under every reading. #101 keeps the part that is genuinely a judgement call. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Jwc9FFroR3mTZ4hLiSkkX3 --- packages/cli/src/rules/vale/run.ts | 27 +++++++++-- packages/cli/test/mixed-engine-check.test.ts | 48 ++++++++++++++++++++ 2 files changed, 71 insertions(+), 4 deletions(-) diff --git a/packages/cli/src/rules/vale/run.ts b/packages/cli/src/rules/vale/run.ts index da402912..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,8 +99,6 @@ export async function runVale( const paths = options.paths ?? []; const timeoutMs = options.timeoutMs ?? VALE_TIMEOUT_MS; - // `--` separates flags from positional paths, so a path beginning with `-` - // is not read as a flag. // 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 @@ -105,12 +106,30 @@ export async function runVale( // 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 targets = paths.length > 0 ? paths : ["."]; + 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 = [ "--config", configPath, "--output=JSON", "--no-exit", + ...exclude, "--", ...targets, ]; diff --git a/packages/cli/test/mixed-engine-check.test.ts b/packages/cli/test/mixed-engine-check.test.ts index 513141ee..c60995f2 100644 --- a/packages/cli/test/mixed-engine-check.test.ts +++ b/packages/cli/test/mixed-engine-check.test.ts @@ -179,6 +179,54 @@ describe("check over a project with both engines", () => { }); }); + 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