fix(cli): leave an already-partitioned rules tree alone

.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/<id>/`; `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/<id>.yml` files still migrates as before.

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/<id>.yml`) *and* the root of the layout
+ * `0005` establishes (`rules/<engine>/<id>/`), 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/<id>/<id>.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<boolean> {
+  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<boolean> {
   try {
     await stat(path);
@@ -286,7 +326,16 @@ async function ensureTrackedDirectory(path: string): Promise<void> {
 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)

packages/cli/test/migrate-round-trip.test.ts

@@ -115,7 +115,7 @@ async function seedVersion4Project(): Promise<void> {
     "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<RuleReport>(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/<id>/`, `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<CheckOutput>(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<RuleReport>(verified.stdout);
+    expect(report.rules.map((rule) => `${rule.engine}/${rule.ruleId}`)).toEqual(
+      ["sg/no-eval"]
+    );
+  });
+});