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 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/<id>.yml` files still moves wholesale, as before. And a tree holding both — an already-partitioned layout with a stray `rules/<id>.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.

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/<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 by what they hold: pre-`0004` holds loose
+ * rule *files* (`rules/<id>.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<boolean> {
   try {
     await stat(path);
@@ -286,6 +386,24 @@ async function ensureTrackedDirectory(path: string): Promise<void> {
 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),

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