From 52341b94e427cd2d8975916b7478ea53dd295f4b Mon Sep 17 00:00:00 2001 From: Jakob Heuser Date: Tue, 18 Aug 2026 21:19:35 -0700 Subject: [PATCH 1/3] 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 00000000..70cabe6a --- /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 27873590..95c79900 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 7b75a6b3..7e6b92f7 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"] + ); + }); +}); From dfbce921ce28a2571243680cb28ab75d2c96124c Mon Sep 17 00:00:00 2001 From: Jakob Heuser Date: Wed, 19 Aug 2026 11:57:02 -0700 Subject: [PATCH 2/3] fix(cli): recognise a partitioned rules tree by its rule files, not by every entry MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Requiring *every* entry under `.taskless/rules/` to be an engine directory turned a stray `.DS_Store` into evidence of the pre-0004 layout, moving a live rules tree wholesale — the exact failure the guard exists to prevent. Key on the two real signals instead: at least one engine directory, and no loose `*.yml` at the root, which is the same signal `0005`'s `assertRootIsFree` reads from the other side. Also narrow the `readdir` catch to ENOENT/ENOTDIR so an unreadable `rules/` fails loudly rather than being relocated uninspected, and reuse the exported `isKnownEngine` in place of a cast over `ENGINES`. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Jwc9FFroR3mTZ4hLiSkkX3 --- .../filesystem/migrations/0004-vale-engine.ts | 41 +++++++++++++------ packages/cli/test/migrate-round-trip.test.ts | 24 +++++++++++ 2 files changed, 52 insertions(+), 13 deletions(-) diff --git a/packages/cli/src/filesystem/migrations/0004-vale-engine.ts b/packages/cli/src/filesystem/migrations/0004-vale-engine.ts index 95c79900..d30f0d2e 100644 --- a/packages/cli/src/filesystem/migrations/0004-vale-engine.ts +++ b/packages/cli/src/filesystem/migrations/0004-vale-engine.ts @@ -13,7 +13,7 @@ import { dirname, join } from "node:path"; import type { Migration } from "../types"; import { CLIError } from "../../util/cli-error"; -import { ENGINES } from "../../rules/engines"; +import { isKnownEngine } from "../../rules/engines"; /** * Default `sgconfig.yml` written when a project has none to move. `ruleDirs` @@ -119,24 +119,39 @@ const MOVES: Array<[string[], string[]]> = [ * 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. + * 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 — at least one + * directory named for an engine, and no loose `*.yml` at the root — which 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 flip the guard off and move a live `rules/` tree + * wholesale — reproducing the exact bug this guard exists to prevent. A tree + * that still holds pre-`0004` rule files is genuinely mixed, and that one does + * migrate, because leaving it alone would strand rules the old layout owns. */ 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. + } 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 false; + throw error; } - if (entries.length === 0) return false; - return entries.every( - (entry) => - entry.isDirectory() && (ENGINES as readonly string[]).includes(entry.name) + if ( + !entries.some((entry) => entry.isDirectory() && isKnownEngine(entry.name)) + ) + return false; + return !entries.some( + (entry) => entry.isFile() && entry.name.endsWith(".yml") ); } diff --git a/packages/cli/test/migrate-round-trip.test.ts b/packages/cli/test/migrate-round-trip.test.ts index 7e6b92f7..c9898de6 100644 --- a/packages/cli/test/migrate-round-trip.test.ts +++ b/packages/cli/test/migrate-round-trip.test.ts @@ -265,6 +265,30 @@ describe("a current-layout project with no taskless.json", () => { 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); + }); + it("leaves the rule where the current layout puts it", async () => { await writeTree(tasklessDirectory, { "rules/sg/no-eval/no-eval.yml": From 56656426383e92add2103ed3ee5b02531cdbee49 Mon Sep 17 00:00:00 2001 From: Jakob Heuser Date: Wed, 19 Aug 2026 14:45:33 -0700 Subject: [PATCH 3/3] fix(cli): migrate a mixed rules tree file by file, not directory by directory MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A `.taskless/rules/` holding both engine directories and a stray loose `.yml` is genuinely mixed, and the guard already said that case "does migrate" — but the move it fell through to was directory-granular. Renaming `rules/` wholesale to collect one stray file carried every already-partitioned rule down with it, and `0005`'s `moveEngineRules` only relocates loose `*.yml` from `sg/rules/`: a directory entry is skipped, `pruneEmpty` finds the tree non-empty and leaves it, and the rules are stranded at `.taskless/sg/rules/sg//`. That is the same silent clean pass this branch set out to fix, reached from a merge-conflict leftover instead of a missing manifest. `rulesArePartitionedByEngine` becomes `inspectRulesLayout`, reporting `legacy` / `partitioned` / `mixed` plus the loose files it found. `mixed` now moves those files individually and leaves the engine directories where they are. A symlinked engine directory also counts as an engine directory, since `Dirent.isDirectory()` is false for a symlink and the cost of guessing wrong is asymmetric: over-recognising declines a move, under-recognising loses rules. The `rules/` move also moves out of `MOVES` into its own `RULES_MOVE`, so the loop no longer re-derives "is this the rules move" from the array's shape. Regression test added; it fails against the previous commit with `no-eval` missing from the check output entirely. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Jwc9FFroR3mTZ4hLiSkkX3 --- .changeset/idempotent-rules-partition.md | 2 +- .../filesystem/migrations/0004-vale-engine.ts | 118 +++++++++++++----- packages/cli/test/migrate-round-trip.test.ts | 40 ++++++ 3 files changed, 127 insertions(+), 33 deletions(-) diff --git a/.changeset/idempotent-rules-partition.md b/.changeset/idempotent-rules-partition.md index 70cabe6a..95349d75 100644 --- a/.changeset/idempotent-rules-partition.md +++ b/.changeset/idempotent-rules-partition.md @@ -6,4 +6,4 @@ Stop the engine-partition migration from relocating a rules tree that is already 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. +`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 d30f0d2e..a24d83cd 100644 --- a/packages/cli/src/filesystem/migrations/0004-vale-engine.ts +++ b/packages/cli/src/filesystem/migrations/0004-vale-engine.ts @@ -88,13 +88,25 @@ 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"]], @@ -102,11 +114,20 @@ const MOVES: Array<[string[], string[]]> = [ ]; /** - * Is `.taskless/rules/` already partitioned by engine — i.e. newer than this - * migration rather than older? + * What layout is `.taskless/rules/` in right now? * - * `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 + * - `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. * @@ -121,19 +142,33 @@ const MOVES: Array<[string[], string[]]> = [ * * 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 — at least one - * directory named for an engine, and no loose `*.yml` at the root — which is - * the same signal `0005`'s `assertRootIsFree` reads from the other side. + * *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 flip the guard off and move a live `rules/` tree - * wholesale — reproducing the exact bug this guard exists to prevent. A tree - * that still holds pre-`0004` rule files is genuinely mixed, and that one does - * migrate, because leaving it alone would strand rules the old layout owns. + * 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 rulesArePartitionedByEngine(root: string): Promise { +async function inspectRulesLayout( + root: string +): Promise<{ layout: RulesLayout; looseRuleFiles: string[] }> { let entries; try { entries = await readdir(root, { withFileTypes: true }); @@ -143,16 +178,26 @@ async function rulesArePartitionedByEngine(root: string): Promise { // 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 false; + if (code === "ENOENT" || code === "ENOTDIR") { + return { layout: "legacy", looseRuleFiles: [] }; + } throw error; } - if ( - !entries.some((entry) => entry.isDirectory() && isKnownEngine(entry.name)) - ) - return false; - return !entries.some( - (entry) => entry.isFile() && entry.name.endsWith(".yml") + + 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 { @@ -341,16 +386,25 @@ 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") - ); + // 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) { - 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 c9898de6..cd45c8c6 100644 --- a/packages/cli/test/migrate-round-trip.test.ts +++ b/packages/cli/test/migrate-round-trip.test.ts @@ -289,6 +289,46 @@ describe("a current-layout project with no taskless.json", () => { 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":