From 93126a86e139927d942256fb7b89a8a29fe761b3 Mon Sep 17 00:00:00 2001 From: Jakob Heuser Date: Wed, 23 Sep 2026 13:37:39 -0700 Subject: [PATCH 1/4] fix(migrate): make migration 9's directory rename the commit point Migration 9 renamed a rule's directory first and then chased the files inside it. The directory rename is what clears the collision, and the migration returns early when none is left, so a crash in between left a rule whose files and `id:` fields still carried the old name and that no re-run repaired. Every in-directory edit now happens under the old name and the directory rename runs last, as a single atomic commit. A rule interrupted before it still collides and is enumerated again; one interrupted after it is whole. Each inner step tolerates having already run, and file rewrites commit by renaming a temporary sibling. The fixture predicate also matched its own output, so a fixture named `-sg-...` by hand became `-sg-sg-...` on the first run. A name already at the target prefix now keeps it and only its `id:` follows. Migration 9 has never shipped (latest is 0.11.2, tagged 2026-09-19; the migration landed 2026-09-22), so the behaviour changes in place and no migration 10 is needed. --- .changeset/migration-9-atomicity.md | 15 ++ .../migrations/0009-unique-rule-ids.ts | 145 +++++++++++++++--- 2 files changed, 138 insertions(+), 22 deletions(-) create mode 100644 .changeset/migration-9-atomicity.md diff --git a/.changeset/migration-9-atomicity.md b/.changeset/migration-9-atomicity.md new file mode 100644 index 00000000..f46e932b --- /dev/null +++ b/.changeset/migration-9-atomicity.md @@ -0,0 +1,15 @@ +--- +"@taskless/cli": patch +--- + +Scaffold migration `9` — the one that renames a rule id held by more than one engine — now survives being interrupted, and no longer double-suffixes a fixture you had already named `-sg-…`. + +Migration 9 has not been in a released version, so nothing on disk anywhere was produced by the old behaviour and there is no repair step to run. `latest` is `0.11.2`, tagged 2026-09-19; the migration landed 2026-09-22. + +**It could not resume.** It renamed the rule directory first and then chased the files inside it, but renaming the directory is what resolves the collision, and the migration returns early when no collision is left. So a run that died in between — a `Ctrl-C`, a full disk, an editor holding a file open — left `sg/no-eval-sg/` containing `no-eval.yml` with `id: no-eval` and fixtures still under the `no-eval-` prefix. `verify` reported that as broken, and running `taskless init` again fixed nothing, because every later run found no collision and returned. + +The order is reversed: the rule file, its `id:` field, the `.tests/` fixtures and a Vale rule's `.vale.ini` are all rewritten under the old directory name, and the directory rename is the last thing to happen. A directory rename is a single atomic operation, so it is the moment a rule is done. A rule interrupted before it still collides and is picked up by the next run; a rule interrupted after it is already whole. Each inner step also tolerates having already run, and every file rewrite is committed by renaming a temporary sibling, so an interrupted write cannot truncate a rule file. + +One asymmetry can survive an interruption. Where `sg` and `vale` both hold an id, a complete run moves both and neither keeps the bare id. If a run is interrupted between the two halves, the half that finished keeps its suffix and the other keeps the bare id, because the collision it would have been renamed for is gone. The tree is collision-free and every rule is internally consistent; only the symmetry is lost. Rename it yourself if you want the pair to match. + +**A fixture already named `-sg-…` is no longer renamed again.** The predicate picking fixtures to rename matched every name it produced, so `no-eval-sg-basic-test.yml` in `sg/no-eval/.tests/` came out as `no-eval-sg-sg-basic-test.yml` on the first run. Such a fixture is already at the right prefix, so it now keeps its name and only its `id:` field follows — which still matters, since ast-grep attributes cases by the `id:` inside the file and a stale one reads as a rule that shipped no cases. diff --git a/packages/cli/src/filesystem/migrations/0009-unique-rule-ids.ts b/packages/cli/src/filesystem/migrations/0009-unique-rule-ids.ts index 932905d5..546c2a00 100644 --- a/packages/cli/src/filesystem/migrations/0009-unique-rule-ids.ts +++ b/packages/cli/src/filesystem/migrations/0009-unique-rule-ids.ts @@ -93,6 +93,45 @@ import type { Migration } from "../types"; * it. A migration that silently renames a user's rules is worse than one that * refuses. * + * ## An interrupted run is resumable, because the commit point is the rename + * + * Every edit a rule needs happens INSIDE the rule's old directory, and the + * directory rename is the last thing to run. That ordering is the whole + * atomicity story: `rename(2)` on a directory is a single atomic operation, so + * it is the point at which a rule is done, and nothing before it is observable + * as progress. + * + * It also makes the "is there a collision" gate a sound resume signal, which is + * why this migration still returns early on a collision-free tree. The commit + * is the operation that CLEARS the collision, so a rule that crashed before it + * still collides and is picked up again; a rule that crashed after it is + * already whole. The reverse ordering — rename the directory first, then chase + * its contents — clears the collision before the rule is consistent, and the + * next run's gate then reports nothing to do over a rule whose files and `id:` + * fields still carry the old name. + * + * Every step inside the directory is written to tolerate having already run: a + * file rename whose source is gone but whose target is there still has its + * `id:` rewritten, and a fixture already carrying the target prefix is never + * renamed a second time. File rewrites go to a temporary sibling and are + * committed with a rename, so a crash cannot leave a half-written rule file. + * + * Two windows remain, and neither leaves a tree a re-run cannot repair: + * + * - Between the first edit and the commit the directory name disagrees with + * the files inside it. `verify` reports that, and the next migration run + * finishes it. The scaffold version is written only after every migration + * returns, so the next `taskless` command runs this again by itself. + * - When a symmetric collision is interrupted between its two halves, the half + * that committed keeps its suffix and the half that never started keeps the + * bare id, because the collision it was named for is gone. The tree is + * collision-free and every rule is internally consistent; only the symmetry + * a complete run would have produced is lost. + * + * Durability past a power loss — between `rename(2)` returning and the + * directory entry reaching disk — is not addressed, and no user-space rename + * dance would address it. + * * Idempotent, and read-only when there is nothing to do. A project with no * collision is enumerated and nothing is written, so `git status` stays clean. */ @@ -109,6 +148,10 @@ const migration: Migration = async (directory) => { // describes it takes the PROJECT root, which is this directory's parent. const cwd = join(directory, ".."); const collisions = await findRuleIdCollisions(cwd); + // Sound as a resume signal, not merely as a "nothing to do" check: the + // directory rename that clears a collision is also the LAST thing each + // rename does, so a rule interrupted part-way still collides and is + // enumerated again here. See the atomicity section above. if (collisions.length === 0) return; const taken = await occupiedRuleIds(cwd); @@ -189,7 +232,16 @@ function freeRuleId( } } -/** Rename one rule and every reference to its id inside its own directory. */ +/** + * Rename one rule and every reference to its id inside its own directory. + * + * THE ORDER IS THE ATOMICITY. Everything inside the rule is rewritten under the + * OLD directory name first, and the directory rename runs last as the single + * atomic commit. Until it lands the rule still holds the colliding id, so a + * crash anywhere above leaves work the next run's collision scan finds. + * Renaming the directory first would clear the collision while the files inside + * still carried the old id, and no re-run would ever look again. + */ async function renameRule( cwd: string, engine: EngineName, @@ -198,22 +250,24 @@ async function renameRule( ): Promise { const fromPath = ruleDirectory(cwd, engine, from); const toPath = ruleDirectory(cwd, engine, to); - await rename(fromPath, toPath); - const lines = [` ${fromPath}`, ` -> ${toPath}`]; + const inside: string[] = []; if (engine === "sg") { - lines.push( - ...(await renameRuleFile(toPath, engine, from, to)), - ...(await renameSgFixtures(toPath, from, to)) + inside.push( + ...(await renameRuleFile(fromPath, engine, from, to)), + ...(await renameSgFixtures(fromPath, from, to)) ); } else if (engine === "vale") { - lines.push( - ...(await renameRuleFile(toPath, engine, from, to)), - ...(await rewriteValeConfig(toPath, from, to)) + inside.push( + ...(await renameRuleFile(fromPath, engine, from, to)), + ...(await rewriteValeConfig(fromPath, from, to)) ); } // No `runtime` branch: this is never called for one. See `NEVER_RENAMED`. - return lines; + await rename(fromPath, toPath); + // Reported directory-first even though it ran last: the report is read as + // "this rule moved, and here is what moved with it". + return [` ${fromPath}`, ` -> ${toPath}`, ...inside]; } /** @@ -223,9 +277,13 @@ async function renameRule( * The name comes from {@link ENGINE_LAYOUTS}, the table that decides it, so * the two engines this runs for stop being a second place that has to agree * with `layout.ts` about `${id}.yml`. Not `ruleFilePath`, which takes a `cwd` - * and a rule id and would resolve into the PRE-rename directory: by the time - * this is called the directory has already moved, and only the file inside it - * still carries the old name. + * and a rule id: this runs BEFORE the directory moves, so the path it would + * build is the one this rule is leaving rather than the one it is in. + * + * Resumable both ways round. A source that is gone with the target already in + * place is an earlier run that died between the rename and the `id:` rewrite, + * so the rewrite is completed rather than skipped — the old early return read + * that state as "no rule file" and left the id behind. */ async function renameRuleFile( ruleDirectoryPath: string, @@ -236,13 +294,18 @@ async function renameRuleFile( const fromName = ENGINE_LAYOUTS[engine].ruleFile(from); const toName = ENGINE_LAYOUTS[engine].ruleFile(to); const fromFile = join(ruleDirectoryPath, fromName); - if (!(await pathExists(fromFile))) return []; const toFile = join(ruleDirectoryPath, toName); - await rename(fromFile, toFile); - const rewritten = await rewriteIdField(toFile, from, to); - return [ - ` renamed ${fromName} -> ${toName}${rewritten ? " and its id: field" : ""}`, - ]; + if (await pathExists(fromFile)) { + await rename(fromFile, toFile); + const rewritten = await rewriteIdField(toFile, from, to); + return [ + ` renamed ${fromName} -> ${toName}${rewritten ? " and its id: field" : ""}`, + ]; + } + if (!(await pathExists(toFile))) return []; + return (await rewriteIdField(toFile, from, to)) + ? [` finished an interrupted rename: ${toName} id: field`] + : []; } /** @@ -256,6 +319,16 @@ async function renameRuleFile( * actually RUNS is keyed on the `id:` inside the file, so a renamed file * still carrying the old id is discovered, silently not counted, and the rule * reads as having shipped no cases. + * + * THE PREDICATE MUST NOT MATCH ITS OWN OUTPUT. `to` is always `-…`, so a + * plain `startsWith(`${from}-`)` accepts every name this loop produces. It cost + * a double suffix on the FIRST run for a fixture a human had already named + * `no-eval-sg-basic-test.yml`, which came back out as + * `no-eval-sg-sg-basic-test.yml`; and on a resumed run it would re-suffix every + * fixture the interrupted run had already moved. A name that already carries + * the target prefix is therefore never renamed — it is where it belongs either + * way — and only its `id:` follows, which is also what finishes a rename + * interrupted between the two. */ async function renameSgFixtures( ruleDirectoryPath: string, @@ -273,7 +346,14 @@ async function renameSgFixtures( } const lines: string[] = []; for (const entry of entries) { - if (!entry.startsWith(`${from}-`) || !entry.endsWith("-test.yml")) continue; + if (!entry.endsWith("-test.yml")) continue; + if (entry.startsWith(`${to}-`)) { + if (await rewriteIdField(join(testsPath, entry), from, to)) { + lines.push(` rewrote ${RULE_TESTS_DIRECTORY}/${entry} id: field`); + } + continue; + } + if (!entry.startsWith(`${from}-`)) continue; const renamed = `${to}-${entry.slice(from.length + 1)}`; await rename(join(testsPath, entry), join(testsPath, renamed)); const rewritten = await rewriteIdField(join(testsPath, renamed), from, to); @@ -306,7 +386,7 @@ async function rewriteValeConfig( } const rewritten = retargetValeConfig(source, from, to); if (rewritten === source) return []; - await writeFile(configPath, rewritten, "utf8"); + await writeFileAtomically(configPath, rewritten); return [` rewrote .vale.ini breadcrumb and ${from}.${from} assignment`]; } @@ -364,10 +444,31 @@ async function rewriteIdField( `$1$2${to}$2$3` ); if (rewritten === source) return false; - await writeFile(path, rewritten, "utf8"); + await writeFileAtomically(path, rewritten); return true; } +/** + * Write a file by writing a sibling and renaming it over the target, so a crash + * mid-write cannot leave a truncated rule file or fixture. + * + * The temporary name is DERIVED FROM THE TARGET rather than randomized, so a + * crash between the write and the rename leaves one predictable path that the + * next run overwrites and consumes: the target still holds its pre-edit bytes, + * so the next run rewrites it and reaches this same temporary again. A random + * suffix would strand a file in the rule directory instead. The name matches + * neither `.yml` nor `*-test.yml` nor `.vale.ini`, so nothing that scans + * the rule directory picks it up while it exists. + */ +async function writeFileAtomically( + path: string, + contents: string +): Promise { + const temporary = `${path}.tskl-0009.tmp`; + await writeFile(temporary, contents, "utf8"); + await rename(temporary, path); +} + /** A rule id is `[a-z0-9-]+`, but escaping keeps this honest if that widens. */ function escapeForRegExp(value: string): string { return value.replaceAll(/[$()*+.?[\\\]^{|}]/g, String.raw`\$&`); From aebe797e5227a18314a9eb210750b5c0935c5cb9 Mon Sep 17 00:00:00 2001 From: Jakob Heuser Date: Wed, 23 Sep 2026 13:37:42 -0700 Subject: [PATCH 2/4] test(migrate): resume migration 9 from a crash at three rename points A crash is injected at a named rename destination, not the Nth call, so each case says which step it interrupts. Two of the three, and the double-suffix case, fail against the previous ordering. --- packages/cli/test/rule-id-uniqueness.test.ts | 133 ++++++++++++++++++- 1 file changed, 131 insertions(+), 2 deletions(-) diff --git a/packages/cli/test/rule-id-uniqueness.test.ts b/packages/cli/test/rule-id-uniqueness.test.ts index a84d64d1..43352e6b 100644 --- a/packages/cli/test/rule-id-uniqueness.test.ts +++ b/packages/cli/test/rule-id-uniqueness.test.ts @@ -1,9 +1,9 @@ import { mkdir, mkdtemp, readFile, readdir, rm, stat } from "node:fs/promises"; import { writeFile } from "node:fs/promises"; import { tmpdir } from "node:os"; -import { join } from "node:path"; +import { basename, join } from "node:path"; -import { afterEach, beforeEach, describe, expect, it } from "vitest"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; import migration, { retargetValeConfig, @@ -26,6 +26,41 @@ import type { EngineName } from "../src/rules/layout"; */ let cwd: string; +/** + * Basename of a `rename` target the migration must die on, or `undefined` for + * a run that is allowed to finish. + * + * A crash is injected at a NAMED destination rather than at the Nth call, so a + * case says which step it interrupts and stays readable when the number of + * writes changes. Basename rather than full path because the pre-fix ordering + * renames the directory first, so the same step happens under a different + * parent there — matching the whole path would make these cases silently stop + * injecting anything against the code they exist to fail against. + */ +let crashAtRenameTo: string | undefined; + +vi.mock("node:fs/promises", async () => { + const actual = + await vi.importActual( + "node:fs/promises" + ); + return { + ...actual, + rename: async ( + from: Parameters[0], + to: Parameters[1] + ) => { + if ( + crashAtRenameTo !== undefined && + basename(to.toString()) === crashAtRenameTo + ) { + throw new Error(`simulated crash renaming to ${to.toString()}`); + } + return actual.rename(from, to); + }, + }; +}); + const SCOPED = (id: string): string => `[*.md]\ntskl) rule = ${id}\n${id}.${id} = YES\n`; @@ -80,6 +115,7 @@ async function runtimeRule(id: string): Promise { } beforeEach(async () => { + crashAtRenameTo = undefined; cwd = await mkdtemp(join(tmpdir(), "tskl-rule-id-")); await mkdir(join(cwd, ".taskless", "rules"), { recursive: true }); }); @@ -347,6 +383,99 @@ describe("migration 0009 renames a colliding project", () => { expect(await snapshot(join(cwd, ".taskless"))).toEqual(after); }); + // THE CENTREPIECE. A run that dies part-way must leave a tree the next run + // repairs. The first two crash points are unrecoverable against the previous + // ordering, which renamed the rule DIRECTORY first: that cleared the + // collision while the files inside still carried the old id, so the next run + // returned at the `collisions.length === 0` gate and nothing ever fixed it. + // Now the directory rename is the last step and therefore the commit point, + // so an interrupted rule still collides and is picked up again. + it.each([ + ["the sg rule file rename", "no-eval-sg.yml"], + ["the sg fixture rename", "no-eval-sg-20260101-test.yml"], + ["the sg directory commit", "no-eval-sg"], + ])( + "resumes to a correct end state after crashing at %s", + async (_step, target) => { + await sgRule("no-eval"); + await valeRule("no-eval"); + + crashAtRenameTo = target; + await expect(migration(join(cwd, ".taskless"))).rejects.toThrow( + "simulated crash" + ); + crashAtRenameTo = undefined; + + // The interrupted rule still holds the colliding id. That is the whole + // reason the next run looks at it again. + expect(await findRuleIdCollisions(cwd)).toHaveLength(1); + + await migration(join(cwd, ".taskless")); + + expect(await findRuleIdCollisions(cwd)).toEqual([]); + const directory = rulePath("sg", "no-eval-sg"); + expect(await exists(rulePath("sg", "no-eval"))).toBe(false); + expect( + await readFile(join(directory, "no-eval-sg.yml"), "utf8") + ).toContain("id: no-eval-sg"); + // Directory, fixture filename and the `id:` inside it all agree, which + // is the state `verify` demands and a half-migrated tree never reaches. + expect(await readdir(join(directory, ".tests"))).toEqual([ + "no-eval-sg-20260101-test.yml", + ]); + expect( + await readFile( + join(directory, ".tests", "no-eval-sg-20260101-test.yml"), + "utf8" + ) + ).toContain("id: no-eval-sg"); + // The vale half never started, so the resuming run renames it too. + const valeDirectory = rulePath("vale", "no-eval-vale"); + expect( + await readFile(join(valeDirectory, ".vale.ini"), "utf8") + ).toContain("no-eval-vale.no-eval-vale = YES"); + const verified = await verifyOneRule(cwd, { + engine: "vale", + ruleId: "no-eval-vale", + }); + expect(verified.ok).toBe(true); + } + ); + + // The output of the fixture rename used to match its own input predicate, so + // a fixture a human had named `-sg-…` BEFORE this ever ran came back out + // double-suffixed on the very first run. It is already at the target prefix, + // so it is left where it is and only its `id:` follows. + it("does not double-suffix a fixture already named -sg-...", async () => { + await sgRule("no-eval"); + await valeRule("no-eval"); + await writeFile( + join(rulePath("sg", "no-eval"), ".tests", "no-eval-sg-basic-test.yml"), + `id: no-eval\nvalid:\n - const a = 1;\n`, + "utf8" + ); + + await migration(join(cwd, ".taskless")); + + const tests = await readdir(join(rulePath("sg", "no-eval-sg"), ".tests")); + expect(tests.toSorted((a, b) => a.localeCompare(b))).toEqual([ + "no-eval-sg-20260101-test.yml", + "no-eval-sg-basic-test.yml", + ]); + // The `id:` still has to follow, or ast-grep attributes no case to it and + // the rule reads as having shipped none. + expect( + await readFile( + join( + rulePath("sg", "no-eval-sg"), + ".tests", + "no-eval-sg-basic-test.yml" + ), + "utf8" + ) + ).toContain("id: no-eval-sg"); + }); + it("is a no-op on a project with no rules tree at all", async () => { await rm(join(cwd, ".taskless", "rules"), { recursive: true }); await expect(migration(join(cwd, ".taskless"))).resolves.toBeUndefined(); From 4c1bcfcdd7b43ae879da6efbf25f5844c3b90545 Mon Sep 17 00:00:00 2001 From: Jakob Heuser Date: Wed, 23 Sep 2026 13:37:43 -0700 Subject: [PATCH 3/4] docs(openspec): require migration 9 to resume after an interrupted run Purely ADDED: 11 -> 12 requirements and 44 -> 47 scenarios in cli-taskless-bootstrap, with nothing dropped, verified by an archive dry-run diffing both lists before and after. --- .../.openspec.yaml | 2 + .../proposal.md | 74 +++++++++++++++++++ .../specs/cli-taskless-bootstrap/spec.md | 55 ++++++++++++++ .../2026-09-23-migration-9-atomicity/tasks.md | 23 ++++++ openspec/specs/cli-taskless-bootstrap/spec.md | 54 ++++++++++++++ 5 files changed, 208 insertions(+) create mode 100644 openspec/changes/archive/2026-09-23-migration-9-atomicity/.openspec.yaml create mode 100644 openspec/changes/archive/2026-09-23-migration-9-atomicity/proposal.md create mode 100644 openspec/changes/archive/2026-09-23-migration-9-atomicity/specs/cli-taskless-bootstrap/spec.md create mode 100644 openspec/changes/archive/2026-09-23-migration-9-atomicity/tasks.md diff --git a/openspec/changes/archive/2026-09-23-migration-9-atomicity/.openspec.yaml b/openspec/changes/archive/2026-09-23-migration-9-atomicity/.openspec.yaml new file mode 100644 index 00000000..265da3d9 --- /dev/null +++ b/openspec/changes/archive/2026-09-23-migration-9-atomicity/.openspec.yaml @@ -0,0 +1,2 @@ +schema: spec-driven +created: 2026-09-23 diff --git a/openspec/changes/archive/2026-09-23-migration-9-atomicity/proposal.md b/openspec/changes/archive/2026-09-23-migration-9-atomicity/proposal.md new file mode 100644 index 00000000..cc640b04 --- /dev/null +++ b/openspec/changes/archive/2026-09-23-migration-9-atomicity/proposal.md @@ -0,0 +1,74 @@ +## Why + +Migration `9` renames rule directories whose id is held by more than one +engine. Two defects were found while verifying that the nine scaffold +migrations are idempotent (taskless/cli#395). Idempotency holds — repeated +COMPLETE runs converge, measured by tree snapshot across a double run. What +does not hold is atomicity. + +**It cannot resume after an interrupted run.** The migration renamed the rule +DIRECTORY first and then chased the files inside it. A collision is defined as +one directory name appearing under two or more engines, so the directory +rename is the operation that CLEARS the collision — and the migration returns +early when there are none. A crash between the directory rename and the rest +therefore leaves `sg/no-eval-sg/` holding `no-eval.yml` with `id: no-eval` and +fixtures still under the `no-eval-` prefix, a tree `verify` reports as broken +and that no number of re-runs repairs, because every later run returns at the +collision gate having found nothing to do. + +**The fixture predicate matched its own output.** `entry.startsWith(`${from}-`)` +accepted every name the loop produced, since the replacement is `${from}-sg`. +Re-running could not trigger it — the directory rename throws `ENOENT`first — +but a fixture a human had named`no-eval-sg-basic-test.yml`BEFORE the +migration ran came out as`no-eval-sg-sg-basic-test.yml` on the first run. It +is also the step a resumed run repeats, so it stops being cosmetic the moment +the resume above exists. + +**Migration 9 has never shipped, so its behaviour is changed in place.** +`npm view @taskless/cli dist-tags` reports `latest: 0.11.2`, tagged +2026-09-19; the migration landed in `d41576f` on 2026-09-22, and +`git merge-base --is-ancestor d41576f v0.11.2` fails. No user has run it, so +there is no already-migrated tree in the wild and no migration `10` to write. + +## What Changes + +- **Ordering is the fix.** Every edit inside a rule now happens under the OLD + directory name, and the directory rename runs LAST as the single atomic + commit. A rule that crashes before it still holds the colliding id, so the + collision gate finds it again; a rule that crashes after it is already whole. + The gate becomes a sound resume signal rather than merely a no-op check. +- Each step inside the directory tolerates having already run: a rule file + whose source is gone but whose target is present still has its `id:` + rewritten, and file rewrites are committed by renaming a temporary sibling. +- The fixture predicate skips a name already carrying the target prefix and + rewrites only its `id:`, so it can no longer match what it produces. +- Tests: a crash injected at three named `rename` destinations, each resumed + and asserted to reach a consistent directory, fixture name and `id:` field; + and the pre-existing `-sg-` fixture name. Two of the three crash points and + the double-suffix case fail against the previous code. + +## Capabilities + +### New Capabilities + +None. `cli-taskless-bootstrap` gains one requirement. + +### Modified Capabilities + +None. "Migration 9 renames a rule id held by more than one engine" is +unchanged: it already requires every `.tests/-*-test.yml` to end at the new +prefix with its `id:` rewritten, which a fixture already at that prefix +satisfies without being renamed again. + +## Impact + +`packages/cli/src/filesystem/migrations/0009-unique-rule-ids.ts` and its test. +No public surface, no other command. The bump is `patch`: the migration has +never been in a released version, so no consumer crosses this boundary. + +## Delivery shape + +**Single PR.** One source file, one test file, one spec requirement — a diff +under 300 hand-written lines that is only reviewable together, since the +ordering change and the tests that prove it are the same argument. It is the +tip, so the change is archived here. diff --git a/openspec/changes/archive/2026-09-23-migration-9-atomicity/specs/cli-taskless-bootstrap/spec.md b/openspec/changes/archive/2026-09-23-migration-9-atomicity/specs/cli-taskless-bootstrap/spec.md new file mode 100644 index 00000000..b6233fae --- /dev/null +++ b/openspec/changes/archive/2026-09-23-migration-9-atomicity/specs/cli-taskless-bootstrap/spec.md @@ -0,0 +1,55 @@ +## ADDED Requirements + +### Requirement: Migration 9 resumes after an interrupted run + +Migration `9` SHALL be resumable: after a run that ends part-way through, for +any reason, a subsequent run SHALL bring the scaffold to the same end state a +single uninterrupted run would have reached. + +Every edit a rule needs SHALL be made inside the rule's existing directory, and +the directory rename SHALL be the LAST operation of that rule's rename. A +directory rename is a single atomic filesystem operation, so it is the point at +which a rule is done, and no earlier step SHALL be observable as progress. + +Because the directory rename is also the operation that clears the collision, +the collision scan SHALL remain a sound resume signal: a rule interrupted +before its commit still holds the colliding id and SHALL be enumerated again, +and a rule interrupted after its commit is already consistent. The migration +SHALL therefore still write nothing when no collision remains. + +Each step inside a rule's directory SHALL tolerate having already run: + +- a rule file whose old name is gone and whose new name is present SHALL still + have its `id:` field rewritten, rather than being treated as absent +- a fixture already carrying the target prefix SHALL NOT be renamed again, and + only its `id:` field SHALL follow +- a file rewrite SHALL be committed by renaming a temporary sibling over the + target, so an interrupted write SHALL NOT leave a truncated rule file + +The predicate selecting fixtures to rename SHALL NOT match the names it +produces. The target id is always the old id plus a suffix, so a predicate +keyed only on the old id accepts its own output and appends the suffix twice. + +#### Scenario: A run interrupted before a rule's directory rename is resumed + +- **WHEN** migration 9 fails after rewriting a colliding rule's file, `id:` + field or fixtures but before its directory is renamed +- **THEN** the rule SHALL still hold the colliding id +- **AND** a subsequent run SHALL enumerate it again and complete the rename +- **AND** the rule's directory name, rule file name, fixture names and every + `id:` field SHALL agree afterwards + +#### Scenario: A run interrupted after a rule's directory rename leaves that rule whole + +- **WHEN** migration 9 fails immediately after a rule's directory is renamed +- **THEN** that rule SHALL already carry its new id in its directory name, its + rule file, its fixtures and every `id:` field +- **AND** a subsequent run SHALL have nothing to do for it + +#### Scenario: A fixture already at the target prefix is not suffixed twice + +- **WHEN** a colliding `sg` rule's `.tests/` holds a fixture whose name already + begins with the target id, whether written by hand before the migration ran + or renamed by an interrupted run +- **THEN** the fixture SHALL keep its name +- **AND** its `id:` field SHALL be rewritten to the new id diff --git a/openspec/changes/archive/2026-09-23-migration-9-atomicity/tasks.md b/openspec/changes/archive/2026-09-23-migration-9-atomicity/tasks.md new file mode 100644 index 00000000..ece9377f --- /dev/null +++ b/openspec/changes/archive/2026-09-23-migration-9-atomicity/tasks.md @@ -0,0 +1,23 @@ +## 1. Implementation + +- [x] 1.1 Move every in-directory edit ahead of the directory rename, so the + rename is the commit point. +- [x] 1.2 Make `renameRuleFile` finish a rename interrupted between the file + rename and the `id:` rewrite. +- [x] 1.3 Make the fixture predicate skip a name already at the target prefix, + rewriting only its `id:`. +- [x] 1.4 Commit file rewrites by renaming a temporary sibling. + +## 2. Tests + +- [x] 2.1 Inject a crash at three named `rename` destinations, resume, and + assert directory, fixture name and `id:` all agree. +- [x] 2.2 Prove the crash-resume and double-suffix cases fail against the + previous source. +- [x] 2.3 Keep the double-run snapshot idempotency coverage passing. + +## 3. Spec + +- [x] 3.1 ADD the crash-resilience requirement to `cli-taskless-bootstrap`. +- [x] 3.2 Dry-run `openspec archive` and compare requirement and scenario + counts before and after. diff --git a/openspec/specs/cli-taskless-bootstrap/spec.md b/openspec/specs/cli-taskless-bootstrap/spec.md index 8628a572..e1ee0628 100644 --- a/openspec/specs/cli-taskless-bootstrap/spec.md +++ b/openspec/specs/cli-taskless-bootstrap/spec.md @@ -364,3 +364,57 @@ It SHALL write nothing when there is no collision. A project in that state SHALL - **WHEN** `.taskless/rules/` does not exist and migration 9 runs - **THEN** the migration SHALL succeed and write nothing + +### Requirement: Migration 9 resumes after an interrupted run + +Migration `9` SHALL be resumable: after a run that ends part-way through, for +any reason, a subsequent run SHALL bring the scaffold to the same end state a +single uninterrupted run would have reached. + +Every edit a rule needs SHALL be made inside the rule's existing directory, and +the directory rename SHALL be the LAST operation of that rule's rename. A +directory rename is a single atomic filesystem operation, so it is the point at +which a rule is done, and no earlier step SHALL be observable as progress. + +Because the directory rename is also the operation that clears the collision, +the collision scan SHALL remain a sound resume signal: a rule interrupted +before its commit still holds the colliding id and SHALL be enumerated again, +and a rule interrupted after its commit is already consistent. The migration +SHALL therefore still write nothing when no collision remains. + +Each step inside a rule's directory SHALL tolerate having already run: + +- a rule file whose old name is gone and whose new name is present SHALL still + have its `id:` field rewritten, rather than being treated as absent +- a fixture already carrying the target prefix SHALL NOT be renamed again, and + only its `id:` field SHALL follow +- a file rewrite SHALL be committed by renaming a temporary sibling over the + target, so an interrupted write SHALL NOT leave a truncated rule file + +The predicate selecting fixtures to rename SHALL NOT match the names it +produces. The target id is always the old id plus a suffix, so a predicate +keyed only on the old id accepts its own output and appends the suffix twice. + +#### Scenario: A run interrupted before a rule's directory rename is resumed + +- **WHEN** migration 9 fails after rewriting a colliding rule's file, `id:` + field or fixtures but before its directory is renamed +- **THEN** the rule SHALL still hold the colliding id +- **AND** a subsequent run SHALL enumerate it again and complete the rename +- **AND** the rule's directory name, rule file name, fixture names and every + `id:` field SHALL agree afterwards + +#### Scenario: A run interrupted after a rule's directory rename leaves that rule whole + +- **WHEN** migration 9 fails immediately after a rule's directory is renamed +- **THEN** that rule SHALL already carry its new id in its directory name, its + rule file, its fixtures and every `id:` field +- **AND** a subsequent run SHALL have nothing to do for it + +#### Scenario: A fixture already at the target prefix is not suffixed twice + +- **WHEN** a colliding `sg` rule's `.tests/` holds a fixture whose name already + begins with the target id, whether written by hand before the migration ran + or renamed by an interrupted run +- **THEN** the fixture SHALL keep its name +- **AND** its `id:` field SHALL be rewritten to the new id From e31a6ad3b20db90e546483b2ac4aa87c7b99c5f7 Mon Sep 17 00:00:00 2001 From: Jakob Heuser Date: Wed, 23 Sep 2026 14:06:35 -0700 Subject: [PATCH 4/4] test(migrate): reach the id:-commit window the crash harness could not MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The harness named a rename by its destination basename only. Putting `.yml` in place and committing a rewrite OF `.yml` are two renames with the same destination, and the first runs first, so the state between them — renamed, `id:` not yet written — was unreachable. That is exactly the state `renameRuleFile`'s resume branch exists to repair, and the same ambiguity hid `renameSgFixtures`' equivalent pair. Renames are now named by source, destination or both, which separates them: the commit's source is the `.tskl-0009.tmp` sibling. Two crash points added for those windows, plus a case that builds the same state by hand so the repair is covered independently of the temporary file's name. Verified non-vacuous: reverting either repair fails all four cases. --- packages/cli/test/rule-id-uniqueness.test.ts | 104 ++++++++++++++++--- 1 file changed, 88 insertions(+), 16 deletions(-) diff --git a/packages/cli/test/rule-id-uniqueness.test.ts b/packages/cli/test/rule-id-uniqueness.test.ts index 43352e6b..32bd0d52 100644 --- a/packages/cli/test/rule-id-uniqueness.test.ts +++ b/packages/cli/test/rule-id-uniqueness.test.ts @@ -27,17 +27,35 @@ import type { EngineName } from "../src/rules/layout"; let cwd: string; /** - * Basename of a `rename` target the migration must die on, or `undefined` for - * a run that is allowed to finish. + * The `rename` the migration must die on, named by the basename of its source, + * its destination, or both. `undefined` lets a run finish. * - * A crash is injected at a NAMED destination rather than at the Nth call, so a - * case says which step it interrupts and stays readable when the number of - * writes changes. Basename rather than full path because the pre-fix ordering - * renames the directory first, so the same step happens under a different + * A crash is injected at a NAMED rename rather than at the Nth call, so a case + * says which step it interrupts and stays readable when the number of writes + * changes. Basenames rather than whole paths because the pre-fix ordering + * renamed the directory first, so the same step happens under a different * parent there — matching the whole path would make these cases silently stop * injecting anything against the code they exist to fail against. + * + * BOTH ENDS ARE MATCHABLE BECAUSE THE DESTINATION ALONE IS AMBIGUOUS. Putting + * `.yml` in place and committing a rewrite OF `.yml` are two renames + * with the same destination, and the first always runs first — so a + * destination-only harness can never stop between them, which is exactly the + * state `renameRuleFile`'s resume branch exists to repair. Naming the source + * separates them: the commit's source is the `.tskl-0009.tmp` sibling, the file + * rename's source is `.yml`. */ -let crashAtRenameTo: string | undefined; +let crashAtRename: { from?: string; to?: string } | undefined; + +/** + * The suffix {@link writeFileAtomically} gives its temporary sibling. + * + * Duplicated from the migration on purpose rather than exported for the test: + * it is an implementation detail the migration is free to change, and a case + * naming it is asserting on the step it means to interrupt. If this ever stops + * matching, the affected cases fail by never injecting a crash, which is loud. + */ +const ATOMIC_WRITE_SUFFIX = ".tskl-0009.tmp"; vi.mock("node:fs/promises", async () => { const actual = @@ -50,11 +68,16 @@ vi.mock("node:fs/promises", async () => { from: Parameters[0], to: Parameters[1] ) => { + const wanted = crashAtRename; if ( - crashAtRenameTo !== undefined && - basename(to.toString()) === crashAtRenameTo + wanted !== undefined && + (wanted.from === undefined || + wanted.from === basename(from.toString())) && + (wanted.to === undefined || wanted.to === basename(to.toString())) ) { - throw new Error(`simulated crash renaming to ${to.toString()}`); + throw new Error( + `simulated crash renaming ${from.toString()} -> ${to.toString()}` + ); } return actual.rename(from, to); }, @@ -115,7 +138,7 @@ async function runtimeRule(id: string): Promise { } beforeEach(async () => { - crashAtRenameTo = undefined; + crashAtRename = undefined; cwd = await mkdtemp(join(tmpdir(), "tskl-rule-id-")); await mkdir(join(cwd, ".taskless", "rules"), { recursive: true }); }); @@ -391,20 +414,31 @@ describe("migration 0009 renames a colliding project", () => { // Now the directory rename is the last step and therefore the commit point, // so an interrupted rule still collides and is picked up again. it.each([ - ["the sg rule file rename", "no-eval-sg.yml"], - ["the sg fixture rename", "no-eval-sg-20260101-test.yml"], - ["the sg directory commit", "no-eval-sg"], + ["the sg rule file rename", { from: "no-eval.yml", to: "no-eval-sg.yml" }], + [ + "the sg rule file's id: commit", + { from: `no-eval-sg.yml${ATOMIC_WRITE_SUFFIX}` }, + ], + [ + "the sg fixture rename", + { from: "no-eval-20260101-test.yml", to: "no-eval-sg-20260101-test.yml" }, + ], + [ + "the sg fixture's id: commit", + { from: `no-eval-sg-20260101-test.yml${ATOMIC_WRITE_SUFFIX}` }, + ], + ["the sg directory commit", { to: "no-eval-sg" }], ])( "resumes to a correct end state after crashing at %s", async (_step, target) => { await sgRule("no-eval"); await valeRule("no-eval"); - crashAtRenameTo = target; + crashAtRename = target; await expect(migration(join(cwd, ".taskless"))).rejects.toThrow( "simulated crash" ); - crashAtRenameTo = undefined; + crashAtRename = undefined; // The interrupted rule still holds the colliding id. That is the whole // reason the next run looks at it again. @@ -442,6 +476,44 @@ describe("migration 0009 renames a colliding project", () => { } ); + // The same window, built by hand instead of by crashing into it. The harness + // proves the migration REACHES this state; this proves the repair works on + // one that arrived any other way — a run killed by SIGKILL, a container + // evicted mid-write — with no dependence on the temporary file's name. + it("finishes a rename left between the file move and its id: field", async () => { + const directory = join(cwd, ".taskless", "rules", "sg", "no-eval"); + await mkdir(join(directory, ".tests"), { recursive: true }); + // Renamed, `id:` not yet rewritten: what an interrupted run leaves. The + // old early return read a missing `no-eval.yml` as "no rule file" and left + // both stale ids behind. + await writeFile( + join(directory, "no-eval-sg.yml"), + `id: no-eval\nlanguage: TypeScript\nseverity: error\nmessage: no eval\nrule:\n pattern: eval($A)\n`, + "utf8" + ); + await writeFile( + join(directory, ".tests", "no-eval-sg-20260101-test.yml"), + `id: no-eval\nvalid:\n - const a = 1;\n`, + "utf8" + ); + await valeRule("no-eval"); + + await migration(join(cwd, ".taskless")); + + const moved = rulePath("sg", "no-eval-sg"); + expect(await exists(join(moved, "no-eval.yml"))).toBe(false); + expect(await readFile(join(moved, "no-eval-sg.yml"), "utf8")).toContain( + "id: no-eval-sg" + ); + expect( + await readFile( + join(moved, ".tests", "no-eval-sg-20260101-test.yml"), + "utf8" + ) + ).toContain("id: no-eval-sg"); + expect(await findRuleIdCollisions(cwd)).toEqual([]); + }); + // The output of the fixture rename used to match its own input predicate, so // a fixture a human had named `-sg-…` BEFORE this ever ran came back out // double-suffixed on the very first run. It is already at the target prefix,