Skip to content

Commit 34b9baa

Browse files
theCodeDriftclaude
andcommitted
fix(cli): give every ast-grep rule a .tests/ so one rule can't fail them all
Migration 0005 created a rule's `.tests/` only as a side effect of moving a test file into it, so an sg rule with no test at version 3 — or one whose filename lacked the `-YYYYMMDD-test.yml` suffix the migration attributes by — arrived at version 5 with no tests directory. Assembly named it in `testConfigs` anyway, and ast-grep 0.41.0 aborts the whole invocation on a `testDir` it cannot read (exit 6), which `--filter` does not scope away: `taskless test` on any rule died naming a rule the author never touched. 0005 now ensures `rules/sg/<id>/.tests/`, gitkeeped when empty so the repair survives a commit. `assembleSgConfig` separately omits a `testDir` that is not on disk, which rescues a project a nightly already stamped at version 5 — those never re-run the migration — and a rule directory made by hand. Neither turns a missing test into a pass; verify and test still report it. Fixes #150 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Cyga14bww8rmazH2XrF8ms
1 parent e788b11 commit 34b9baa

5 files changed

Lines changed: 213 additions & 18 deletions

File tree

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,9 @@
1+
---
2+
"@taskless/cli": patch
3+
---
4+
5+
Stop a rule with no tests from failing every other rule's ast-grep test run.
6+
7+
Migration `0005` created a rule's `.tests/` only as a side effect of moving a test file into it, so an ast-grep rule that had no test at version 3 — or one whose test file did not match the `<id>-YYYYMMDD-test.yml` shape the migration can attribute to a rule — arrived in the new layout with no tests directory at all. Assembly then named that directory as a `testConfigs` entry anyway, and ast-grep 0.41.0 treats a `testDir` it cannot read as fatal to the whole invocation rather than to the one rule: `taskless test` on _any_ rule died with `Cannot read rule directory .taskless/rules/sg/<other-id>/.tests` and exit 6, naming a rule the author had never touched. `--filter` does not scope that away, so there was no way to run one rule's tests around it.
8+
9+
`0005` now gives every `rules/sg/<id>/` a `.tests/`, holding a committed `.gitkeep` when it would otherwise be empty — git does not track empty directories, so without one the repair would not survive a commit and the failure would come back in CI. Assembly separately omits any `testDir` that is not on disk, which is what rescues a project a nightly already stamped at version 5: migrations short-circuit once the manifest is at the latest version, so those installs never re-run the amended `0005`, and the same state is reachable at any version by creating a rule directory by hand. Neither change turns a missing test into a pass — `verify` still reports "No test file found" and `test` still reports "Skipped: no test file found", both reading the rule directory rather than the generated config.

‎packages/cli/src/filesystem/migrations/0005-rule-directories.ts‎

Lines changed: 46 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -177,6 +177,15 @@ async function moveEngineTests(
177177
}
178178
}
179179

180+
/** Create `path` and drop a `.gitkeep` in it when it would otherwise be empty. */
181+
async function ensureTrackedDirectory(path: string): Promise<void> {
182+
await mkdir(path, { recursive: true });
183+
const entries = await entriesOf(path);
184+
if (entries.length === 0) {
185+
await writeFile(join(path, ".gitkeep"), "", "utf8");
186+
}
187+
}
188+
180189
/**
181190
* Create `rules/<engine>/` for every engine, tracked when empty.
182191
*
@@ -188,12 +197,42 @@ async function moveEngineTests(
188197
*/
189198
async function scaffoldEngineDirectories(directory: string): Promise<void> {
190199
for (const engine of ENGINES) {
191-
const path = join(directory, RULES_DIRECTORY, engine);
192-
await mkdir(path, { recursive: true });
193-
const entries = await entriesOf(path);
194-
if (entries.length === 0) {
195-
await writeFile(join(path, ".gitkeep"), "", "utf8");
196-
}
200+
await ensureTrackedDirectory(join(directory, RULES_DIRECTORY, engine));
201+
}
202+
}
203+
204+
/**
205+
* Give every ast-grep rule a `.tests/`, tracked when it holds nothing else.
206+
*
207+
* Up to here `.tests/` only ever appeared as a side effect of moving a test
208+
* *into* it, so a rule that had no test at version 3 — or one whose test file
209+
* did not match the `<id>-YYYYMMDD-test.yml` shape `moveEngineTests` can
210+
* attribute — arrived at version 5 with no tests directory at all.
211+
*
212+
* **That is not a cosmetic gap.** Assembly names every rule's `.tests/` as a
213+
* `testConfigs` entry, and ast-grep 0.41.0 aborts the entire invocation when
214+
* one of them is missing (`Cannot read rule directory ...`, exit 6) — which
215+
* `--filter` does not scope away. One rule with no tests therefore failed
216+
* `taskless test` for every *other* rule in the project, with an error naming a
217+
* rule the author had never touched. Measured against 0.41.0: an empty
218+
* `.tests/`, and one holding only a `.gitkeep`, are both accepted by `sg test`
219+
* and `sg scan`.
220+
*
221+
* The `.gitkeep` is what makes the repair survive a commit. Git does not track
222+
* empty directories, so a `.tests/` created here and left empty would never
223+
* reach CI or a fresh clone, and the failure would come back there. It is
224+
* committed rather than ignored — `.taskless/.gitignore` carries only the two
225+
* generated configs — and never reads as a test: `verify` counts a test file
226+
* only when it matches `<id>-*-test.yml`, so such a rule still reports "No test
227+
* file found" rather than quietly passing.
228+
*/
229+
async function ensureSgTestDirectories(directory: string): Promise<void> {
230+
const engineRoot = join(directory, RULES_DIRECTORY, "sg");
231+
for (const entry of await entriesOf(engineRoot)) {
232+
if (!entry.isDirectory()) continue;
233+
await ensureTrackedDirectory(
234+
join(engineRoot, entry.name, RULE_TESTS_DIRECTORY)
235+
);
197236
}
198237
}
199238

@@ -370,6 +409,7 @@ const migration: Migration = async (directory) => {
370409
await rm(join(directory, config), { force: true });
371410
}
372411

412+
await ensureSgTestDirectories(directory);
373413
await scaffoldEngineDirectories(directory);
374414
await ignoreGeneratedConfigs(directory);
375415

‎packages/cli/src/rules/assemble.ts‎

Lines changed: 32 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import { mkdir, readFile, writeFile } from "node:fs/promises";
1+
import { mkdir, readFile, stat, writeFile } from "node:fs/promises";
22
import { dirname, join, posix, relative, sep } from "node:path";
33

44
import {
@@ -41,6 +41,16 @@ function toPosix(path: string): string {
4141
return path.split(sep).join(posix.sep);
4242
}
4343

44+
/** Whether `path` is a directory on disk. */
45+
async function isDirectory(path: string): Promise<boolean> {
46+
try {
47+
const stats = await stat(path);
48+
return stats.isDirectory();
49+
} catch {
50+
return false;
51+
}
52+
}
53+
4454
/**
4555
* The header every assembled Vale config carries.
4656
*
@@ -137,6 +147,19 @@ export async function assembleValeConfig(
137147
*
138148
* `testConfigs` gets one entry per rule, because each rule keeps its tests
139149
* inside its own directory. Sorted with the rule ids, so the file is stable.
150+
*
151+
* A rule whose `.tests/` is not on disk is left out of `testConfigs`
152+
* entirely. **ast-grep 0.41.0 treats a missing `testDir` as fatal to the whole
153+
* invocation** — `Cannot read rule directory ...`, exit 6 — and `--filter` does
154+
* not scope that away, so emitting the entry regardless would let one rule fail
155+
* every other rule's test run, with an error naming a rule its author never
156+
* touched. Migration `0005` now creates the directory, but that does not make
157+
* this check redundant: `runMigrations` short-circuits once the manifest reads
158+
* version 5, so a project a nightly already stamped never re-runs the amended
159+
* migration, and a hand-made `mkdir .taskless/rules/sg/<id>/` reaches the same
160+
* state on any version. Nothing becomes a silent pass — `verify` still reports
161+
* "No test file found" and `test` still reports "Skipped: no test file found",
162+
* both of which read the rule directory rather than this config.
140163
*/
141164
export async function assembleSgConfig(
142165
cwd: string
@@ -145,9 +168,15 @@ export async function assembleSgConfig(
145168
if (ruleIds.length === 0) return undefined;
146169

147170
const rulesDirectory = tasklessRelative(RULES_DIRECTORY, "sg");
148-
const testDirectories = ruleIds.map((ruleId) =>
149-
toPosix(relative(join(cwd, ".taskless"), ruleTestsDirectory(cwd, "sg", ruleId)))
171+
const candidates = ruleIds.map((ruleId) =>
172+
ruleTestsDirectory(cwd, "sg", ruleId)
173+
);
174+
const present = await Promise.all(
175+
candidates.map((path) => isDirectory(path))
150176
);
177+
const testDirectories = candidates
178+
.filter((_, index) => present[index])
179+
.map((path) => toPosix(relative(join(cwd, ".taskless"), path)));
151180

152181
const contents = [
153182
"ruleDirs:",

‎packages/cli/test/assemble.test.ts‎

Lines changed: 67 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,12 +1,16 @@
1-
import { mkdir, mkdtemp, readFile, rm, writeFile } from "node:fs/promises";
1+
import {
2+
mkdir,
3+
mkdtemp,
4+
readFile,
5+
rm,
6+
stat,
7+
writeFile,
8+
} from "node:fs/promises";
29
import { tmpdir } from "node:os";
310
import { join } from "node:path";
411
import { afterEach, beforeEach, describe, expect, it } from "vitest";
512

6-
import {
7-
assembleSgConfig,
8-
assembleValeConfig,
9-
} from "../src/rules/assemble";
13+
import { assembleSgConfig, assembleValeConfig } from "../src/rules/assemble";
1014
import { ruleDirectory, ruleTestsDirectory } from "../src/rules/engines";
1115

1216
let cwd: string;
@@ -32,13 +36,23 @@ async function valeRule(id: string, config: string): Promise<void> {
3236

3337
/** Lay down an ast-grep rule with a test file. */
3438
async function sgRule(id: string): Promise<void> {
39+
await sgRuleWithoutTests(id);
40+
await mkdir(ruleTestsDirectory(cwd, "sg", id), { recursive: true });
41+
}
42+
43+
/**
44+
* Lay down an ast-grep rule that has no `.tests/` at all.
45+
*
46+
* The state a nightly-migrated project is in, and the state a hand-made
47+
* `mkdir .taskless/rules/sg/<id>/` reaches on any version.
48+
*/
49+
async function sgRuleWithoutTests(id: string): Promise<void> {
3550
const directory = ruleDirectory(cwd, "sg", id);
3651
await mkdir(directory, { recursive: true });
3752
await writeFile(
3853
join(directory, `${id}.yml`),
3954
`id: ${id}\nlanguage: TypeScript\nseverity: error\nmessage: x\nrule:\n pattern: eval($A)\n`
4055
);
41-
await mkdir(ruleTestsDirectory(cwd, "sg", id), { recursive: true });
4256
}
4357

4458
describe("Vale config assembly", () => {
@@ -150,4 +164,51 @@ describe("ast-grep config assembly", () => {
150164
it("writes nothing when there are no ast-grep rules", async () => {
151165
expect(await assembleSgConfig(cwd)).toBeUndefined();
152166
});
167+
168+
// ast-grep 0.41.0 aborts the whole invocation on a `testDir` it cannot read
169+
// (exit 6), and `--filter` does not scope that away — so a single rule with
170+
// no `.tests/` would fail every *other* rule's test run, naming a rule its
171+
// author never touched.
172+
it("omits a testDir whose directory is not on disk", async () => {
173+
await sgRule("has-tests");
174+
await sgRuleWithoutTests("no-tests");
175+
176+
const path = await assembleSgConfig(cwd);
177+
const contents = await readFile(join(cwd, path ?? ""), "utf8");
178+
179+
expect(contents).toContain("- testDir: rules/sg/has-tests/.tests");
180+
expect(contents).not.toContain("no-tests");
181+
});
182+
183+
it("emits no testDir that is missing from disk, for any rule", async () => {
184+
await sgRule("alpha");
185+
await sgRuleWithoutTests("beta");
186+
await sgRule("gamma");
187+
188+
const path = await assembleSgConfig(cwd);
189+
const contents = await readFile(join(cwd, path ?? ""), "utf8");
190+
191+
const prefix = " - testDir: ";
192+
const emitted = contents
193+
.split("\n")
194+
.filter((line) => line.startsWith(prefix))
195+
.map((line) => line.slice(prefix.length));
196+
expect(emitted.length).toBeGreaterThan(0);
197+
for (const directory of emitted) {
198+
const stats = await stat(join(cwd, ".taskless", directory));
199+
expect(stats.isDirectory(), `${directory} is a directory`).toBe(true);
200+
}
201+
});
202+
203+
// `testConfigs:` with no entries under it is accepted by ast-grep 0.41.0 —
204+
// measured for both `sg test` and `sg scan` — so a project where no rule has
205+
// tests yet still gets a config both commands can read.
206+
it("still emits a config when no rule has a tests directory", async () => {
207+
await sgRuleWithoutTests("only-rule");
208+
209+
const path = await assembleSgConfig(cwd);
210+
const contents = await readFile(join(cwd, path ?? ""), "utf8");
211+
212+
expect(contents).toBe("ruleDirs:\n - rules/sg\ntestConfigs:\n");
213+
});
153214
});

‎packages/cli/test/migrate-engine-layout.test.ts‎

Lines changed: 59 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ import { createHash } from "node:crypto";
22
import {
33
mkdir,
44
mkdtemp,
5+
readdir,
56
readFile,
67
rm,
78
stat,
@@ -314,6 +315,62 @@ describe("migrations 0004 + 0005 — one directory per rule", () => {
314315
).not.toContain("sgconfig.yml");
315316
});
316317

318+
// Assembly names every rule's `.tests/` as a `testConfigs` entry, and
319+
// ast-grep 0.41.0 aborts the whole invocation on one it cannot read (exit 6,
320+
// `Cannot read rule directory ...`) — which `--filter` does not scope away.
321+
// A migrated rule with no tests directory therefore failed `taskless test`
322+
// for every *other* rule in the project. The fixture above already produces
323+
// one: `no-eval`'s test file carries no timestamp, so 0005 cannot attribute
324+
// it and the rule arrives with nothing moved into it.
325+
it("gives every ast-grep rule a tests directory", async () => {
326+
await seedLegacyLayout();
327+
328+
await ensureTasklessDirectory(temporaryDirectory);
329+
330+
const sgRoot = join(tasklessDirectory, "rules", "sg");
331+
const entries = await readdir(sgRoot, { withFileTypes: true });
332+
const ruleIds = entries
333+
.filter((entry) => entry.isDirectory())
334+
.map((entry) => entry.name);
335+
expect(ruleIds).toContain("no-eval");
336+
337+
for (const ruleId of ruleIds) {
338+
const tests = join(sgRoot, ruleId, ".tests");
339+
expect(await exists(tests), `${ruleId} has a .tests/`).toBe(true);
340+
}
341+
342+
// Git does not track empty directories, so a `.tests/` created here and
343+
// left empty would never reach CI or a fresh clone and the failure would
344+
// come back there. The `.gitkeep` is committed, not gitignored —
345+
// `.taskless/.gitignore` carries only the two generated configs.
346+
expect(await exists(join(sgRoot, "no-eval", ".tests", ".gitkeep"))).toBe(
347+
true
348+
);
349+
const gitignore = await readFile(
350+
join(tasklessDirectory, ".gitignore"),
351+
"utf8"
352+
);
353+
expect(gitignore).not.toContain(".gitkeep");
354+
});
355+
356+
// A `.gitkeep` is not a test. `verify` counts a test file only when it
357+
// matches `<id>-*-test.yml`, so the repaired rule still reports honestly
358+
// rather than turning into a silent pass.
359+
it("does not gitkeep a tests directory that received a real test", async () => {
360+
await seedLegacyLayout();
361+
await writeTree(tasklessDirectory, {
362+
"rule-tests/no-var-20250101-test.yml": "id: no-var\nvalid:\n - foo()\n",
363+
"rules/no-var.yml":
364+
"id: no-var\nlanguage: typescript\nrule:\n pattern: var $A = $B\n",
365+
});
366+
367+
await ensureTasklessDirectory(temporaryDirectory);
368+
369+
const tests = join(tasklessDirectory, "rules", "sg", "no-var", ".tests");
370+
expect(await exists(join(tests, "no-var-20250101-test.yml"))).toBe(true);
371+
expect(await exists(join(tests, ".gitkeep"))).toBe(false);
372+
});
373+
317374
it("is idempotent — a second run changes nothing", async () => {
318375
await seedLegacyLayout();
319376
await ensureTasklessDirectory(temporaryDirectory);
@@ -322,9 +379,8 @@ describe("migrations 0004 + 0005 — one directory per rule", () => {
322379
);
323380

324381
// Re-run 0005 directly (runMigrations would short-circuit on version).
325-
const { default: migration } = await import(
326-
"../src/filesystem/migrations/0005-rule-directories"
327-
);
382+
const { default: migration } =
383+
await import("../src/filesystem/migrations/0005-rule-directories");
328384
await migration(tasklessDirectory);
329385

330386
expect(

0 commit comments

Comments
 (0)