diff --git a/.changeset/idempotent-rules-partition.md b/.changeset/idempotent-rules-partition.md new file mode 100644 index 00000000..95349d75 --- /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 reads the shape of `.taskless/rules/` before moving it. A tree holding engine directories and no loose rule files is newer than the migration, not older, so it is left alone. A genuinely pre-`0004` tree of flat `rules/.yml` files still moves wholesale, as before. And a tree holding both — an already-partitioned layout with a stray `rules/.yml` beside it, as a merge-conflict leftover produces — migrates only the stray files: moving the directory to collect them would carry the partitioned rules down with it, and `0005` never brings them back, which is the same silent clean pass by another route. diff --git a/packages/cli/src/filesystem/migrations/0004-vale-engine.ts b/packages/cli/src/filesystem/migrations/0004-vale-engine.ts index 27873590..a24d83cd 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 { isKnownEngine } from "../../rules/engines"; /** * Default `sgconfig.yml` written when a project has none to move. `ruleDirs` @@ -87,19 +88,118 @@ const REQUIRED_DIRECTORIES: string[][] = (() => { })(); /** - * Trees moved by this migration, as [legacy path, engine-partitioned path] - * relative to `.taskless/`. Every move is content-preserving: runtime capture - * bytes determine their server-side reconciliation hashes, so a rewrite here - * would invalidate every signature. + * The `rules/` move, kept apart from `MOVES` because it is the only one whose + * source path means two different things and so the only one the migration + * has to reason about. Singling it out here beats re-deriving "is this the + * rules move" from the array's shape on every iteration below. + */ +const RULES_MOVE: [string[], string[]] = [["rules"], ["sg", "rules"]]; + +/** + * The remaining trees moved by this migration, as [legacy path, + * engine-partitioned path] relative to `.taskless/`. Every one of these names + * is pre-`0004`-only — no later layout uses `rule-tests/`, `sgconfig.yml`, + * `runtime-rules/`, or `runtime-rule-tests/` at the `.taskless/` root — so on + * a current tree they simply do not exist and their moves are no-ops. + * + * Every move is content-preserving: runtime capture bytes determine their + * server-side reconciliation hashes, so a rewrite here would invalidate every + * signature. */ const MOVES: Array<[string[], string[]]> = [ - [["rules"], ["sg", "rules"]], [["rule-tests"], ["sg", "rule-tests"]], [["sgconfig.yml"], ["sg", "sgconfig.yml"]], [["runtime-rules"], ["runtime", "rules"]], [["runtime-rule-tests"], ["runtime", "rule-tests"]], ]; +/** + * What layout is `.taskless/rules/` in right now? + * + * - `legacy` — pre-`0004`, or absent. The whole directory moves. + * - `partitioned` — already the layout `0005` establishes. Nothing moves. + * - `mixed` — both at once. Only the loose rule files move. + */ +type RulesLayout = "legacy" | "partitioned" | "mixed"; + +/** + * Read `.taskless/rules/` and say which layout it is in, plus the loose rule + * files found at its root. + * + * `rules/` is the one moved path 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 by what they hold: pre-`0004` holds loose + * rule *files* (`rules/.yml`), the current layout holds engine + * *directories*. Recognition keys on exactly those two signals, and the loose + * `*.yml` half is the same signal `0005`'s `assertRootIsFree` reads from the + * other side. + * + * Deliberately not "every entry is an engine directory": a single unrelated + * entry beside the real ones is ordinary (macOS writes `.DS_Store` the moment + * a folder is opened in Finder, `.gitignore` notwithstanding), and under a + * strict rule it would report `legacy` and move a live `rules/` tree wholesale + * — reproducing the exact bug this exists to prevent. A symlinked engine + * directory counts for the same reason: `Dirent.isDirectory()` is `false` for + * a symlink, and the cost of guessing wrong is asymmetric — over-recognising + * declines a move, under-recognising loses rules. + * + * **`mixed` is why this reports a layout rather than a boolean.** A tree that + * still holds pre-`0004` rule files beside engine directories has to migrate + * those files — leaving them strands rules the old layout owns, and `0005` + * refuses to run while they sit there. But the move that serves them is + * per-file: renaming the whole directory to satisfy one stray `.yml` carries + * every already-partitioned rule down with it, and `0005`'s `moveEngineRules` + * only relocates loose `*.yml` from `sg/rules/` — a directory entry is + * skipped, so the buried tree never comes back and `pruneEmpty` leaves it + * there. Same silent-clean-pass failure, reached from a merge-conflict + * leftover instead of a missing manifest. + */ +async function inspectRulesLayout( + root: string +): Promise<{ layout: RulesLayout; looseRuleFiles: string[] }> { + let entries; + try { + entries = await readdir(root, { withFileTypes: true }); + } catch (error) { + // No `rules/` at all — nothing to protect. Anything else (a permission + // problem, an I/O error) is unhealthy filesystem state, and this migration + // moves directories on the strength of what it reads here: swallowing it + // would relocate a tree whose contents were never actually inspected. + const code = (error as NodeJS.ErrnoException).code; + if (code === "ENOENT" || code === "ENOTDIR") { + return { layout: "legacy", looseRuleFiles: [] }; + } + throw error; + } + + const hasEngineDirectory = entries.some( + (entry) => + (entry.isDirectory() || entry.isSymbolicLink()) && + isKnownEngine(entry.name) + ); + const looseRuleFiles = entries + .filter((entry) => entry.isFile() && entry.name.endsWith(".yml")) + .map((entry) => entry.name); + + if (!hasEngineDirectory) return { layout: "legacy", looseRuleFiles }; + return { + layout: looseRuleFiles.length === 0 ? "partitioned" : "mixed", + looseRuleFiles, + }; +} + async function pathExists(path: string): Promise { try { await stat(path); @@ -286,6 +386,24 @@ async function ensureTrackedDirectory(path: string): Promise { const migration: Migration = async (directory) => { await assertNoDirectoryConflicts(directory); + // Only `rules/` needs this check; see `inspectRulesLayout` for why, and for + // why `mixed` moves files rather than the directory. + const [rulesFrom, rulesTo] = RULES_MOVE; + const rules = await inspectRulesLayout(join(directory, ...rulesFrom)); + if (rules.layout === "legacy") { + await movePreservingContent( + join(directory, ...rulesFrom), + join(directory, ...rulesTo) + ); + } else if (rules.layout === "mixed") { + for (const name of rules.looseRuleFiles) { + await movePreservingContent( + join(directory, ...rulesFrom, name), + join(directory, ...rulesTo, name) + ); + } + } + for (const [from, to] of MOVES) { await movePreservingContent( join(directory, ...from), diff --git a/packages/cli/test/migrate-round-trip.test.ts b/packages/cli/test/migrate-round-trip.test.ts index 7b75a6b3..cd45c8c6 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,120 @@ 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); + }); + + // A `.DS_Store` beside the engine directories is not a different layout — it + // is a macOS project someone opened in Finder. Recognising the tree only when + // *every* entry is an engine directory would move a live `rules/` wholesale + // on the strength of that file, which is the bug, not a corner of it. + it("still finds its rules with a stray entry beside the engine directories", 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": "", + "rules/.DS_Store": "Bud1", + }); + 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"); + expect(result.exitCode).toBe(1); + }); + + // A loose `*.yml` beside the engine directories is the one genuinely mixed + // shape: pre-0004 rule files and the current layout in the same tree. The + // stray file has to migrate — leaving it strands a rule the old layout owns — + // but the move is per-file, not the whole directory. Moving `rules/` + // wholesale to satisfy the stray would carry `sg/no-eval/` down with it, and + // 0005 only relocates loose `*.yml` from `sg/rules/`, so the buried rule + // never comes back: the exact bug, reached by a merge-conflict leftover + // instead of a missing manifest. + it("migrates a stray loose rule without burying the partitioned ones", 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": "", + "rules/some-old-rule.yml": + "id: some-old-rule\nlanguage: typescript\nseverity: error\nmessage: Avoid debugger.\nrule:\n pattern: debugger\n", + }); + await writeFile(join(cwd, "app.ts"), "eval(raw);\ndebugger;\n", "utf8"); + + const result = await runCli(["check", "-d", cwd, "--json"]); + + // Both survive: the already-partitioned rule stays put, the stray one is + // relocated into the layout rather than left where 0005 refuses to run. + expect( + parseJson(result.stdout).results.map( + (finding) => `${finding.ruleId}:${finding.file}` + ) + ).toEqual( + expect.arrayContaining(["no-eval:app.ts", "some-old-rule:app.ts"]) + ); + expect(result.exitCode).toBe(1); + + const verified = await runCli(["verify", "-d", cwd, "--json"]); + expect( + parseJson(verified.stdout) + .rules.map((rule) => `${rule.engine}/${rule.ruleId}`) + .toSorted() + ).toEqual(["sg/no-eval", "sg/some-old-rule"]); + }); + + 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"] + ); + }); +});