From 1d512e55ee8fc1000c89bc9d85411ef1bcebbff3 Mon Sep 17 00:00:00 2001 From: Jakob Heuser Date: Tue, 18 Aug 2026 21:19:35 -0700 Subject: [PATCH] fix(cli): leave an already-partitioned rules tree alone MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A `.taskless/` with no `taskless.json` reads as version 0, so every migration runs — including 0004's `rules/` -> `sg/rules/` move, which predates the layout such a tree is usually already in. Applied there it buried every rule at `sg/rules/sg//`, 0005 scaffolded empty engine directories over the gap, and `check` exited 0 on a clean report for a project it had stopped scanning. 0004 now recognizes an engine-partitioned `rules/` and skips that one move. Recognition is strict — every entry a directory named for an engine, at least one present — so a genuinely pre-0004 tree of flat `rules/.yml` files still migrates. Fixes #109 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Jwc9FFroR3mTZ4hLiSkkX3 --- .changeset/idempotent-rules-partition.md | 9 +++ .../filesystem/migrations/0004-vale-engine.ts | 49 +++++++++++++ packages/cli/test/migrate-round-trip.test.ts | 70 +++++++++++++++++-- 3 files changed, 121 insertions(+), 7 deletions(-) create mode 100644 .changeset/idempotent-rules-partition.md diff --git a/.changeset/idempotent-rules-partition.md b/.changeset/idempotent-rules-partition.md new file mode 100644 index 0000000..70cabe6 --- /dev/null +++ b/.changeset/idempotent-rules-partition.md @@ -0,0 +1,9 @@ +--- +"@taskless/cli": patch +--- + +Stop the engine-partition migration from relocating a rules tree that is already partitioned. + +A `.taskless/` with no `taskless.json` — a manifest that was never committed, or was deleted — reads as version 0, so every migration runs against it. Migration `0004` then applied its `rules/` → `sg/rules/` move to a tree already in the current layout, burying every rule at `.taskless/sg/rules/sg//`; `0005` scaffolded fresh empty engine directories over the gap. Nothing errored. `check` scanned a tree with no rules in it and exited 0 on a clean report, so a project that had silently stopped being checked was indistinguishable from one that passes. + +`0004` now recognizes an already-partitioned `.taskless/rules/` — every entry an engine directory, no loose rule files — and leaves it alone, because a tree in that shape is newer than the migration, not older. Recognition is strict, so a genuinely pre-`0004` project with flat `rules/.yml` files still migrates as before. diff --git a/packages/cli/src/filesystem/migrations/0004-vale-engine.ts b/packages/cli/src/filesystem/migrations/0004-vale-engine.ts index 2787359..95c7990 100644 --- a/packages/cli/src/filesystem/migrations/0004-vale-engine.ts +++ b/packages/cli/src/filesystem/migrations/0004-vale-engine.ts @@ -13,6 +13,7 @@ import { dirname, join } from "node:path"; import type { Migration } from "../types"; import { CLIError } from "../../util/cli-error"; +import { ENGINES } from "../../rules/engines"; /** * Default `sgconfig.yml` written when a project has none to move. `ruleDirs` @@ -100,6 +101,45 @@ const MOVES: Array<[string[], string[]]> = [ [["runtime-rule-tests"], ["runtime", "rule-tests"]], ]; +/** + * Is `.taskless/rules/` already partitioned by engine — i.e. newer than this + * migration rather than older? + * + * `rules/` is the one path in `MOVES` that means two different things. It is + * the pre-`0004` flat location (`rules/.yml`) *and* the root of the layout + * `0005` establishes (`rules///`), so the same move that upgrades + * an old tree wrecks a current one. + * + * That collision is reachable, because a `.taskless/` with no `taskless.json` + * reads as version 0 and runs **every** migration — a project whose manifest + * was never committed, or was deleted, arrives here in the 0005 shape. Moving + * `rules/` then produces `.taskless/sg/rules/sg//.yml`; `0005` + * afterwards scaffolds fresh empty engine directories over the hole, and + * `check` scans a tree with no rules in it and exits 0 on a clean report. The + * failure is not an error the user can see — it is a project that quietly + * stopped being checked. + * + * The two shapes are distinguishable with certainty: pre-`0004` holds rule + * *files*, the current layout holds only engine *directories*. Recognition is + * therefore strict — every entry must be a directory named for an engine, and + * there must be at least one. A mixed or partial tree is not evidence of the + * new layout, so it still migrates, which keeps a genuinely old project moving + * forward at the cost of doing nothing clever with a tree nobody produces. + */ +async function rulesArePartitionedByEngine(root: string): Promise { + let entries; + try { + entries = await readdir(root, { withFileTypes: true }); + } catch { + return false; // No `rules/` at all — nothing to protect. + } + if (entries.length === 0) return false; + return entries.every( + (entry) => + entry.isDirectory() && (ENGINES as readonly string[]).includes(entry.name) + ); +} + async function pathExists(path: string): Promise { try { await stat(path); @@ -286,7 +326,16 @@ async function ensureTrackedDirectory(path: string): Promise { const migration: Migration = async (directory) => { await assertNoDirectoryConflicts(directory); + // Only `rules/` needs this check. The other four sources — `rule-tests/`, + // `sgconfig.yml`, `runtime-rules/`, `runtime-rule-tests/` — are names no + // later layout uses, so on a current tree they simply do not exist and their + // moves are already no-ops. + const skipRulesMove = await rulesArePartitionedByEngine( + join(directory, "rules") + ); + for (const [from, to] of MOVES) { + if (skipRulesMove && from.length === 1 && from[0] === "rules") continue; await movePreservingContent( join(directory, ...from), join(directory, ...to) diff --git a/packages/cli/test/migrate-round-trip.test.ts b/packages/cli/test/migrate-round-trip.test.ts index 7b75a6b..7e6b92f 100644 --- a/packages/cli/test/migrate-round-trip.test.ts +++ b/packages/cli/test/migrate-round-trip.test.ts @@ -115,7 +115,7 @@ async function seedVersion4Project(): Promise { "vale/.vale.ini": "StylesPath = rules\nMinAlertLevel = suggestion\n\n[*.md]\ntskl) rule = no-simply\nBasedOnStyles =\nrules.no-simply = YES\n", "vale/rules/no-simply.yml": - 'extends: existence\nmessage: "Avoid \'%s\'"\nlevel: warning\nignorecase: true\ntokens:\n - simply\n', + "extends: existence\nmessage: \"Avoid '%s'\"\nlevel: warning\nignorecase: true\ntokens:\n - simply\n", "vale/rule-tests/no-simply/fail/bad.md": "You simply do it.\n", "vale/rule-tests/no-simply/pass/ok.md": "You do it.\n", @@ -168,11 +168,9 @@ withVale("a version-4 project upgraded through 0005", () => { const result = await runCli(["verify", "-d", cwd, "--json"]); const report = parseJson(result.stdout); // Engine order follows the `ENGINES` declaration, not the alphabet. - expect(report.rules.map((rule) => `${rule.engine}/${rule.ruleId}`)).toEqual([ - "sg/no-eval", - "vale/no-simply", - "runtime/no-eval-runtime", - ]); + expect(report.rules.map((rule) => `${rule.engine}/${rule.ruleId}`)).toEqual( + ["sg/no-eval", "vale/no-simply", "runtime/no-eval-runtime"] + ); expect(report.rules.flatMap((rule) => rule.errors)).toEqual([]); expect(result.exitCode).toBe(0); }); @@ -201,7 +199,12 @@ withVale("a version-4 project upgraded through 0005", () => { await runCli(["check", "-d", cwd, "--json"]); - const moved = join(tasklessDirectory, "rules", "runtime", "no-eval-runtime"); + const moved = join( + tasklessDirectory, + "rules", + "runtime", + "no-eval-runtime" + ); expect(await sha256(join(moved, "captures", "capture.yml"))).toBe( before.capture ); @@ -226,3 +229,56 @@ withVale("a version-4 project upgraded through 0005", () => { ).toBe(first); }); }); + +/** + * A project already in the current layout whose `taskless.json` is missing. + * + * `readRawManifest` reports version 0 for an absent manifest, so every + * migration runs — including `0004`, whose `rules/` → `sg/rules/` move + * predates the layout this tree is already in. Unguarded it buries the rules + * at `sg/rules/sg//`, `0005` scaffolds empty engine directories over the + * gap, and `check` reports a clean pass on a project it no longer scans. + * + * Deliberately outside `withVale`: ast-grep alone is enough to see whether the + * rules survived, and this is the case that must never regress silently. + */ +describe("a current-layout project with no taskless.json", () => { + it("still finds its rules instead of reporting a clean pass", async () => { + await writeTree(tasklessDirectory, { + "rules/sg/no-eval/no-eval.yml": + "id: no-eval\nlanguage: typescript\nseverity: error\nmessage: Avoid eval.\nrule:\n pattern: eval($A)\n", + "rules/vale/.gitkeep": "", + "rules/runtime/.gitkeep": "", + }); + await writeFile(join(cwd, "app.ts"), "eval(raw);\n", "utf8"); + + const result = await runCli(["check", "-d", cwd, "--json"]); + + expect( + parseJson(result.stdout).results.map( + (finding) => `${finding.ruleId}:${finding.file}` + ) + ).toContain("no-eval:app.ts"); + // An error-severity match is exit 1. Exit 0 here is the bug: a project + // whose rules were moved out from under it looks indistinguishable from a + // project that passes. + expect(result.exitCode).toBe(1); + }); + + it("leaves the rule where the current layout puts it", async () => { + await writeTree(tasklessDirectory, { + "rules/sg/no-eval/no-eval.yml": + "id: no-eval\nlanguage: typescript\nseverity: error\nmessage: Avoid eval.\nrule:\n pattern: eval($A)\n", + "rules/vale/.gitkeep": "", + "rules/runtime/.gitkeep": "", + }); + + await runCli(["check", "-d", cwd, "--json"]); + + const verified = await runCli(["verify", "-d", cwd, "--json"]); + const report = parseJson(verified.stdout); + expect(report.rules.map((rule) => `${rule.engine}/${rule.ruleId}`)).toEqual( + ["sg/no-eval"] + ); + }); +});