From 28918209921a79335310cd4ff4e6f771173f9b2e Mon Sep 17 00:00:00 2001 From: Jakob Heuser Date: Mon, 12 Oct 2026 11:11:04 -0700 Subject: [PATCH] fix(test): notice when an sg rule's files:/ignores: globs reach no file An ast-grep fixture is a pathless snippet, so a rule's globs never apply to it. A rule scoped to a directory that does not exist passed `test` while `check` never ran it. `test` now asks ast-grep, through `--inspect entity` over the same walk `check` performs, whether the rule applies to any file, and attaches a notice when it applies to none. --- .changeset/sg-empty-scope-notice.md | 5 + packages/cli/src/rules/inspect.ts | 9 +- packages/cli/src/rules/sg-scope.ts | 161 ++++++++++++++++++ .../cli/test/verify-test-commands.test.ts | 99 +++++++++++ 4 files changed, 273 insertions(+), 1 deletion(-) create mode 100644 .changeset/sg-empty-scope-notice.md create mode 100644 packages/cli/src/rules/sg-scope.ts diff --git a/.changeset/sg-empty-scope-notice.md b/.changeset/sg-empty-scope-notice.md new file mode 100644 index 00000000..c06b70c6 --- /dev/null +++ b/.changeset/sg-empty-scope-notice.md @@ -0,0 +1,5 @@ +--- +"@taskless/cli": patch +--- + +`taskless test` now says when an ast-grep rule's `files:`/`ignores:` globs match no file in the project. A fixture snippet has no path, so the globs never apply to it, and a rule scoped to `lib/**` in a project that keeps its code in `src/` used to pass `test` while `check` never ran it. The rule still passes; the result carries a notice naming the globs, on the human output and in `notices` under `--json`. Rules scoped by `language:` alone are not checked, since a project with no code in that language is honestly clean. diff --git a/packages/cli/src/rules/inspect.ts b/packages/cli/src/rules/inspect.ts index cf779d52..01c94c7c 100644 --- a/packages/cli/src/rules/inspect.ts +++ b/packages/cli/src/rules/inspect.ts @@ -10,6 +10,7 @@ import { } from "./engines"; import { describeCoverageShortfall, type FixtureFinding } from "./fixtures"; import { collectSgFixtureFindings } from "./sg-fixture-findings"; +import { describeEmptySgScope } from "./sg-scope"; import { type EngineName } from "./layout"; import { assessCaptureDirectory, @@ -498,7 +499,13 @@ export async function testOneRule( errors, violations, ran: true, - notices: verification.notices, + // The fixtures above have no path, so nothing they proved says the + // rule's globs reach a real file. Asked here, on a rule that verified, + // and only ever as a notice; see `sg-scope.ts`. + notices: collectNotices([ + ...verification.notices, + await describeEmptySgScope(cwd, ruleId), + ]), // `sg test` still decides the verdict above and still cannot produce a // finding — the vendored binary has no `--json` and no output-format // flag. So unlike the other two engines, these are not read back out of diff --git a/packages/cli/src/rules/sg-scope.ts b/packages/cli/src/rules/sg-scope.ts new file mode 100644 index 00000000..00555c53 --- /dev/null +++ b/packages/cli/src/rules/sg-scope.ts @@ -0,0 +1,161 @@ +import { spawn } from "node:child_process"; +import { readFile } from "node:fs/promises"; +import process from "node:process"; +import { createInterface } from "node:readline"; + +import { parse } from "yaml"; + +import { ruleFilePath } from "./engines"; +import { buildPath, findSgBinary, sgWalkArgv } from "./scan"; + +/** + * Whether an ast-grep rule's `files:`/`ignores:` globs reach any file in the + * project. + * + * `test` cannot answer this from the fixtures. An sg fixture is a `valid:` or + * `invalid:` snippet with no path, so the globs never apply to it, and a rule + * whose `files:` names `lib/**` in a project that keeps its code in `src/` + * passes every test and is then never run by `check` (#486). A typo in a glob + * and a clean codebase look exactly the same from either command. + * + * ## Why ast-grep answers, not a glob library + * + * The question is what ast-grep's walker would hand this rule, which depends on + * its glob dialect, the `ignore` crate's `.gitignore` handling, and the walk + * flags `check` passes. Re-deriving that with a second matcher is a second + * opinion that can disagree with the first. `--inspect entity` reports it + * directly, per file, on stderr. Measured against the vendored 0.45.3: + * + * ``` + * sg: entity|file|src/app.ts: language=TypeScript,appliedRuleCount=0 + * ``` + * + * `appliedRuleCount` is the number of loaded rules whose language, `files:` and + * `ignores:` all admit the file. With `-r` exactly one rule is loaded, so any + * line above zero means the rule is in scope somewhere. + * + * `-r` rather than the assembled config for the reason + * `sg-fixture-findings.ts` gives: one malformed sibling rule aborts parsing of + * the assembled config, and the scope of THIS rule would then read as zero. + * With no project config, ast-grep resolves the globs against the cwd, which is + * the project root `check` scans from. `sgWalkArgv([])` is the whole-project + * walk `check` performs, so the files considered here are the files `check` + * considers. + */ + +/** One `--inspect entity` file line, as ast-grep 0.45.3 writes it. */ +const FILE_ENTITY = /^sg: entity\|file\|.*,appliedRuleCount=(\d+)$/; + +/** + * Which scoping keys the rule declares, or none when it declares neither. + * + * A rule with neither key is scoped only by `language:`, and reaching no file + * then means the project has no code in that language, which `check` reporting + * nothing is an honest account of. The defect is a glob that selects nothing, + * so only a rule that has a glob is measured, and only that rule pays for the + * walk. + */ +async function scopingKeys(ruleFile: string): Promise { + let rule: unknown; + try { + rule = parse(await readFile(ruleFile, "utf8")) as unknown; + } catch { + // An unreadable or malformed rule is `verify`'s to report. + return []; + } + if (typeof rule !== "object" || rule === null) return []; + return ["files", "ignores"].filter((key) => key in rule); +} + +/** + * Whether ast-grep applies the rule to at least one file in the project. + * + * Three answers, because a walk that did not finish is neither: `unknown` is + * what lets the caller stay silent rather than report an empty scope it never + * measured. + * + * Stops at the first file in scope: the answer is a yes/no, and the rest of the + * walk, including running the rule over every file, would only confirm it. + */ +function reachesAnyFile( + cwd: string, + ruleFile: string +): Promise<"reached" | "empty" | "unknown"> { + const sgBinary = findSgBinary(); + return new Promise((resolve) => { + const child = spawn( + sgBinary, + [ + "scan", + "-r", + ruleFile, + "--json=stream", + "--inspect", + "entity", + ...sgWalkArgv([]), + ], + { + cwd, + stdio: ["ignore", "pipe", "pipe"], + env: { ...process.env, PATH: buildPath() }, + } + ); + + let reached = false; + // Findings arrive on stdout and are not this question. Drained so a full + // pipe cannot block the child before it reaches the line we are waiting + // for. + child.stdout.resume(); + const rl = createInterface({ input: child.stderr }); + rl.on("line", (line) => { + const match = FILE_ENTITY.exec(line); + if (match !== null && Number(match[1]) > 0 && !reached) { + reached = true; + child.kill(); + } + }); + + child.on("error", () => { + resolve("unknown"); + }); + child.on("close", (code) => { + if (reached) { + resolve("reached"); + return; + } + // 0 is a clean scan and 1 is error-severity findings; anything else is a + // walk that did not finish, and "no file seen" from it is not evidence + // of an empty scope. + resolve(code === 0 || code === 1 ? "empty" : "unknown"); + }); + }); +} + +/** + * A notice when the rule's globs select no file in the project, or `undefined`. + * + * A notice rather than a failure. A glob can be narrow on purpose, written + * ahead of the code it targets, and nothing about the rule itself is invalid. + * What must not happen is the rule reading as tested and working while `check` + * never runs it, and a notice is printed beside the tick, which is where that + * reading forms. + * + * Silent when ast-grep could not finish the walk: saying the scope is empty on + * the strength of a failed scan would be the same false clean this exists to + * remove, in the other direction. + */ +export async function describeEmptySgScope( + cwd: string, + ruleId: string +): Promise { + const ruleFile = ruleFilePath(cwd, "sg", ruleId); + const keys = await scopingKeys(ruleFile); + if (keys.length === 0) return undefined; + if ((await reachesAnyFile(cwd, ruleFile)) !== "empty") return undefined; + const globs = keys.map((key) => `${key}:`).join("/"); + return ( + `${ruleId}'s ${globs} globs match no file in this project, so \`check\` ` + + `will never run it. The globs are matched against paths relative to the ` + + `project root; the fixtures passing does not exercise them.` + ); +} diff --git a/packages/cli/test/verify-test-commands.test.ts b/packages/cli/test/verify-test-commands.test.ts index b1a9c353..1879258b 100644 --- a/packages/cli/test/verify-test-commands.test.ts +++ b/packages/cli/test/verify-test-commands.test.ts @@ -601,3 +601,102 @@ describe("a rejection names the constraint it violated", () => { ); }); }); + +/** + * An sg rule with both fixture buckets populated, so `test` passes it, and + * `scope` written verbatim between `message:` and `rule:`. + */ +async function scopedSgRule(id: string, scope = "") { + const directory = join(cwd, ".taskless", "rules", "sg", id); + await mkdir(join(directory, ".tests"), { recursive: true }); + await writeFile( + join(directory, `${id}.yml`), + `id: ${id}\nlanguage: TypeScript\nseverity: error\nmessage: no log\n` + + `${scope}rule:\n pattern: console.log($$$)\n` + ); + await writeFile( + join(directory, ".tests", `${id}-test.yml`), + `id: ${id}\nvalid:\n - console.error(x);\ninvalid:\n - console.log(x);\n` + ); + return `.taskless/rules/sg/${id}`; +} + +/** The notices `test --json` attached to the one rule, asserting it passed. */ +async function noticesFor(path: string): Promise { + const result = await runCli(["test", path, "-d", cwd, "--json"]); + // A notice, never a failure: a narrow glob can be deliberate. + expect(result.exitCode).toBe(0); + const rule = (JSON.parse(result.stdout) as Report).rules[0]; + expect(rule?.ok).toBe(true); + return rule?.notices; +} + +// #486. An sg fixture has no path, so `files:` never applies to it, and a glob +// that selects nothing passed `test` while `check` never ran the rule. +// +// Every case here is one a glob matcher of our own would get wrong in at least +// one direction, which is why the scope is asked of ast-grep's walk instead. +describe("test says when an sg rule's globs reach no file", () => { + beforeEach(async () => { + await mkdir(join(cwd, "src"), { recursive: true }); + await writeFile(join(cwd, "src", "app.ts"), "console.log('hi');\n"); + }); + + it("notices a files: glob that matches nothing", async () => { + const path = await scopedSgRule("no-log", `files:\n - "lib/**/*.ts"\n`); + const notices = await noticesFor(path); + expect(notices).toHaveLength(1); + expect(notices?.[0]).toContain("no-log's files: globs match no file"); + }); + + it("is silent when the glob reaches a file", async () => { + const path = await scopedSgRule("no-log", `files:\n - "src/**/*.ts"\n`); + expect(await noticesFor(path)).toEqual([]); + }); + + // The glob matches a real file, but not one in the rule's language, so + // ast-grep never applies the rule to it. Matching the glob alone would call + // this in scope. + it("notices a glob that only reaches files in another language", async () => { + await writeFile(join(cwd, "src", "notes.md"), "console.log('hi')\n"); + const path = await scopedSgRule("no-log", `files:\n - "src/**/*.md"\n`); + expect(await noticesFor(path)).toHaveLength(1); + }); + + // An `ignores:` that swallows every file empties the scope as surely as a + // mistyped `files:`, and the notice has to name the key that did it. + it("notices an ignores: glob that excludes every file", async () => { + const path = await scopedSgRule("no-log", `ignores:\n - "**/*.ts"\n`); + const notices = await noticesFor(path); + expect(notices).toHaveLength(1); + expect(notices?.[0]).toContain("no-log's ignores: globs match no file"); + }); + + // `check` honours `.gitignore`, so a glob reaching only ignored files is a + // rule `check` never runs, however many files the glob matches on disk. + it("does not count a gitignored file as in scope", async () => { + await execFileAsync("git", ["init", "-q"], { cwd }); + await mkdir(join(cwd, "dist"), { recursive: true }); + await writeFile(join(cwd, "dist", "app.ts"), "console.log('hi');\n"); + await writeFile(join(cwd, ".gitignore"), "dist/\n"); + const path = await scopedSgRule("no-log", `files:\n - "dist/**/*.ts"\n`); + expect(await noticesFor(path)).toHaveLength(1); + }); + + // `check` never walks into `.taskless/`, so a file there must not count + // either, even though the glob and the language both admit it. + it("does not count the CLI's own directory as in scope", async () => { + await rm(join(cwd, "src"), { recursive: true }); + await writeFile(join(cwd, ".taskless", "probe.ts"), "console.log(1);\n"); + const path = await scopedSgRule("no-log", `files:\n - "**/*.ts"\n`); + expect(await noticesFor(path)).toHaveLength(1); + }); + + // No glob means the scope is the language alone, and a project with no code + // in it is honestly clean rather than mis-scoped. + it("is silent for a rule with no files: or ignores:", async () => { + await rm(join(cwd, "src"), { recursive: true }); + const path = await scopedSgRule("no-log"); + expect(await noticesFor(path)).toEqual([]); + }); +});