From 61bbce2faba8481f76db59bb59fec5944e94a756 Mon Sep 17 00:00:00 2001 From: Jakob Heuser Date: Sat, 7 Nov 2026 21:05:05 -0800 Subject: [PATCH 1/2] fix(rules): roll back a rule delivery that fails mid-write Stage every rule of a request under .taskless/ and move each into place with rename, moving any older version aside first. An IO failure while staging or moving undoes the moves already made, so .taskless/rules/ is left as it was and the command fails with RULE_GENERATION_FAILED. Refs #623 --- .../multi-rule-delivery-checks-every-rule.md | 2 + .../.openspec.yaml | 2 + .../proposal.md | 26 ++ .../specs/cli-generated-rule-delivery/spec.md | 46 ++++ .../tasks.md | 5 + packages/cli/src/rules/deliver.ts | 4 +- packages/cli/src/rules/files.ts | 244 +++++++++++++++++- packages/cli/src/rules/generate.ts | 44 +++- .../cli/test/rule-create-multi-rule.test.ts | 112 +++++++- 9 files changed, 472 insertions(+), 13 deletions(-) create mode 100644 openspec/changes/multi-rule-delivery-rolls-back-io-failure/.openspec.yaml create mode 100644 openspec/changes/multi-rule-delivery-rolls-back-io-failure/proposal.md create mode 100644 openspec/changes/multi-rule-delivery-rolls-back-io-failure/specs/cli-generated-rule-delivery/spec.md create mode 100644 openspec/changes/multi-rule-delivery-rolls-back-io-failure/tasks.md diff --git a/.changeset/multi-rule-delivery-checks-every-rule.md b/.changeset/multi-rule-delivery-checks-every-rule.md index e87c226c..efd1a47c 100644 --- a/.changeset/multi-rule-delivery-checks-every-rule.md +++ b/.changeset/multi-rule-delivery-checks-every-rule.md @@ -3,3 +3,5 @@ --- `rule create` and `rule improve` no longer leave part of a request on disk when one of its rules is refused. Every rule a request produced is now checked (its engine, its paths, and whether its file set is complete) before the first is written, so a refusal leaves `.taskless/rules/` as it was. Under `--json`, these refusals now report `RULE_GENERATION_FAILED` or `RULE_UNSUPPORTED` instead of `INTERNAL_ERROR`. + +An IO failure while the rules are being written (a full disk, a directory the CLI cannot write to) no longer leaves the earlier rules on disk either: every rule is staged under `.taskless/` first and moved into place afterwards, and a failure part-way moves back any rule already replaced, so `.taskless/rules/` is left as it was and the command reports `RULE_GENERATION_FAILED`. diff --git a/openspec/changes/multi-rule-delivery-rolls-back-io-failure/.openspec.yaml b/openspec/changes/multi-rule-delivery-rolls-back-io-failure/.openspec.yaml new file mode 100644 index 00000000..e582ee4a --- /dev/null +++ b/openspec/changes/multi-rule-delivery-rolls-back-io-failure/.openspec.yaml @@ -0,0 +1,2 @@ +schema: spec-driven +created: 2026-11-07 diff --git a/openspec/changes/multi-rule-delivery-rolls-back-io-failure/proposal.md b/openspec/changes/multi-rule-delivery-rolls-back-io-failure/proposal.md new file mode 100644 index 00000000..d1f84b86 --- /dev/null +++ b/openspec/changes/multi-rule-delivery-rolls-back-io-failure/proposal.md @@ -0,0 +1,26 @@ +## Why + +`multi-rule-delivery-checks-every-rule` made a request's rules all or nothing against every check that runs before a write, and named what it left out: an IO failure part-way through the writes themselves (a full disk, `EACCES` on the second rule) still left the earlier rules written while the command failed and reported no rules (#623). Under `--json` that failure also surfaced as `INTERNAL_ERROR`. + +## What Changes + +- `rule create` and `rule improve` stage every rule of a request in full in a temporary directory under `.taskless/`, then move each into `.taskless/rules/` with `rename`, moving any version already there aside first. +- An IO failure while staging or moving rolls back the rules already moved: each new directory is removed, each replaced version is renamed back, and directories the move created are removed. `.taskless/rules/` is left as it was, the staging directory is removed, and the command fails with `RULE_GENERATION_FAILED`. +- If the rollback itself cannot finish, the staging directory is kept and the error names where each replaced version is. +- What is not atomic is stated, not hidden: each rename is atomic on one filesystem, but a sequence of them is not, so a process killed outright between two renames can leave a mix of new and old rules, with the replaced versions kept under `.taskless/.tmp-delivery-*`. +- `rule restore` and `rule rollback` deliver one rule and behave as before. + +## Capabilities + +### New Capabilities + +None. + +### Modified Capabilities + +- `cli-generated-rule-delivery`: the multi-rule guarantee now covers an IO failure during the writes, with the residual crash window named. + +## Impact + +- `packages/cli/src/rules/files.ts` (new `writePreparedRules`), `packages/cli/src/rules/generate.ts` (`deliverRevisions`), and `packages/cli/src/rules/deliver.ts` (exports `assertRuleDirectoryIsNotALink`). +- Delivery shape: **single PR, stacked on #632** (`multi-rule-delivery-checks-every-rule`). It extends that change's requirement and changeset, and lands after it. diff --git a/openspec/changes/multi-rule-delivery-rolls-back-io-failure/specs/cli-generated-rule-delivery/spec.md b/openspec/changes/multi-rule-delivery-rolls-back-io-failure/specs/cli-generated-rule-delivery/spec.md new file mode 100644 index 00000000..ef290376 --- /dev/null +++ b/openspec/changes/multi-rule-delivery-rolls-back-io-failure/specs/cli-generated-rule-delivery/spec.md @@ -0,0 +1,46 @@ +## MODIFIED Requirements + +### Requirement: Every rule of a request is checked before any is written + +When a generation request produced more than one rule, the CLI SHALL verify every rule's +signatures, and check every rule's id, engine, and file set as a writable rule, before writing +any of them. A request in which any rule is refused SHALL leave `.taskless/rules/` as it was and +exit with an error. A refusal from these checks SHALL carry an error code: `RULE_UNSUPPORTED` for +an engine this CLI does not know, and `RULE_GENERATION_FAILED` otherwise. + +The writes SHALL be all or nothing as well. The CLI SHALL stage every rule in full under +`.taskless/`, on the same filesystem as `.taskless/rules/`, before moving any into place. When an +IO failure occurs while staging or moving, the CLI SHALL undo every move already made, restoring +any version of a rule it replaced, SHALL remove the staging directory, and SHALL exit with +`RULE_GENERATION_FAILED`, leaving `.taskless/rules/` as it was. When the undo itself cannot +finish, the CLI SHALL keep the staging directory and SHALL name where each replaced version is. + +A process killed outright between two moves is outside this guarantee: each move is atomic, the +sequence of them is not, and no rollback runs. The replaced versions SHALL then still be on disk +in the staging directory. + +#### Scenario: A later rule with an unknown engine writes nothing + +- **WHEN** a request produced two rules and the second declares an engine the CLI does not know +- **THEN** the CLI SHALL exit with `RULE_UNSUPPORTED` +- **AND** `.taskless/rules/` SHALL be unchanged, including the first rule's directory + +#### Scenario: A later rule with an unsafe path writes nothing + +- **WHEN** a request produced two rules and the second delivers a path containing `..` +- **THEN** the CLI SHALL exit with `RULE_GENERATION_FAILED`, naming the second rule +- **AND** `.taskless/rules/` SHALL be unchanged + +#### Scenario: A write that fails on a later rule restores the earlier one + +- **WHEN** a request produced two rules, the first replaces an older version already on disk, and moving the second into place fails with an IO error +- **THEN** the CLI SHALL exit with `RULE_GENERATION_FAILED`, naming the second rule +- **AND** `.taskless/rules/` SHALL be byte-identical to how it started, including every file of the older version +- **AND** no staging directory SHALL remain under `.taskless/` + +#### Scenario: A write that fails on a later rule removes a new earlier one + +- **WHEN** a request produced two rules, the first is new, and moving the second into place fails with an IO error +- **THEN** the CLI SHALL exit with `RULE_GENERATION_FAILED` +- **AND** the first rule's directory SHALL NOT exist +- **AND** no staging directory SHALL remain under `.taskless/` diff --git a/openspec/changes/multi-rule-delivery-rolls-back-io-failure/tasks.md b/openspec/changes/multi-rule-delivery-rolls-back-io-failure/tasks.md new file mode 100644 index 00000000..61cb2dd8 --- /dev/null +++ b/openspec/changes/multi-rule-delivery-rolls-back-io-failure/tasks.md @@ -0,0 +1,5 @@ +## 1. Delivery + +- [x] 1.1 Add `writePreparedRules`, which stages every rule under `.taskless/` and moves each into place, rolling back on failure +- [x] 1.2 Write a request's rules through it in `deliverRevisions`, reporting a write failure as `RULE_GENERATION_FAILED` +- [x] 1.3 Test a two-rule request whose second write fails, with the first rule both new and replacing an older version diff --git a/packages/cli/src/rules/deliver.ts b/packages/cli/src/rules/deliver.ts index 41d3f129..90b00869 100644 --- a/packages/cli/src/rules/deliver.ts +++ b/packages/cli/src/rules/deliver.ts @@ -306,7 +306,9 @@ function describeErrno(error: unknown): string { * describes, and silently deleting it would orphan whatever a developer had * pointed it at. */ -async function assertRuleDirectoryIsNotALink(directory: string): Promise { +export async function assertRuleDirectoryIsNotALink( + directory: string +): Promise { let entry; try { entry = await lstat(directory); diff --git a/packages/cli/src/rules/files.ts b/packages/cli/src/rules/files.ts index 8be66aed..5952df86 100644 --- a/packages/cli/src/rules/files.ts +++ b/packages/cli/src/rules/files.ts @@ -1,5 +1,5 @@ -import { mkdir, readFile, rm } from "node:fs/promises"; -import { join } from "node:path"; +import { mkdir, mkdtemp, readFile, rename, rm, rmdir } from "node:fs/promises"; +import { dirname, join } from "node:path"; import { parse } from "yaml"; @@ -12,11 +12,14 @@ import { isKnownEngine, LEGACY_RULE_TESTS_DIRECTORY, RULE_TESTS_DIRECTORY, + TASKLESS_DIRECTORY, type EngineName, } from "./layout"; import { describeRuleIdCollision, findRuleIdCollision } from "./id-uniqueness"; +import { isMissingDirectory } from "./errno"; import { isValidRuleId } from "./validate-id"; import { + assertRuleDirectoryIsNotALink, assessDelivery, describeMissingFixtures, writeDeliveredFileSet, @@ -116,6 +119,243 @@ export async function writePreparedRule( return ruleFilePath(cwd, engine, id); } +/** + * Where a multi-rule delivery is assembled, inside `.taskless/` so that every + * rename out of it stays on one filesystem: a rename across devices is a copy, + * which is neither atomic nor guaranteed to succeed. + */ +const STAGING_PREFIX = ".tmp-delivery-"; + +/** One rule's move into `.taskless/rules/`, recorded so it can be undone. */ +interface CommitStep { + id: string; + /** The rule directory being replaced or created. */ + target: string; + /** Where the version already at `target` was moved, when there was one. */ + previous?: string; + /** The topmost directory `mkdir` created above `target`, when it made one. */ + createdParent?: string; + /** Set once the staged rule is at `target`. */ + committed: boolean; +} + +/** + * Write several rules {@link prepareServedRule} accepted, so that an IO + * failure part-way leaves `.taskless/rules/` as it was. `onWarning` names the + * rule a warning is about by its index in `rules`, and omits the index for a + * warning about the delivery as a whole. + * + * Two phases: + * + * 1. **Stage.** Every rule is written in full into a fresh directory under + * `.taskless/`, by the same {@link writeDeliveredFileSet} a single rule + * goes through, so a full disk or an unwritable file stops here, before + * anything under `.taskless/rules/` has moved. + * 2. **Commit.** Each rule is moved into place with `rename`: the version + * already there (if any) is renamed aside into the staging directory, then + * the staged directory is renamed onto the rule's path. If any rename + * fails, the rules already moved are undone in reverse order: the new + * directory is removed and the old one renamed back, and directories the + * commit created are removed again. + * + * WHAT IS AND IS NOT ATOMIC. Each rename is atomic, on one filesystem; the + * sequence of them is not. A process killed outright (SIGKILL, a power loss) + * between two renames leaves `.taskless/rules/` holding some rules at their + * new version and the rest at their old one, possibly with one rule absent + * for the instant between its two renames. Every replaced version is then + * still on disk, under `.taskless/.tmp-delivery-*`, but nothing restores it + * automatically. No renames are fsynced, so after a power loss the order in + * which they reached the disk is up to the filesystem. Rollback can itself + * fail (a permission changed underneath it, say), and then the staging + * directory is KEPT rather than removed and the error names what is where, so + * the replaced version is never deleted while it is the only copy. + * + * The rule directory being a symlink is refused before anything is staged, + * as {@link writeDeliveredFileSet} refuses it: renaming the link aside would + * otherwise orphan whatever a developer had pointed it at. + */ +export async function writePreparedRules( + cwd: string, + rules: readonly PreparedRule[], + onWarning?: (message: string, index?: number) => void +): Promise { + await ensureTasklessDirectory(cwd); + const staging = await mkdtemp(join(cwd, TASKLESS_DIRECTORY, STAGING_PREFIX)); + // The rule being staged, then the steps of the commit: between them, the + // rule a failure happened on. + let current: string | undefined; + const steps: CommitStep[] = []; + try { + for (const rule of rules) { + current = rule.id; + await assertRuleDirectoryIsNotALink( + ruleDirectory(cwd, rule.engine, rule.id) + ); + // The staging directory stands in for the project root, so the staged + // copy lands at the same relative path it will occupy for real. + await writeDeliveredFileSet( + staging, + rule.engine, + rule.id, + rule.assessment + ); + } + current = undefined; + await commitStaged(cwd, staging, rules, steps); + } catch (error) { + const reason = error instanceof Error ? error.message : String(error); + if (error instanceof RollbackIncompleteError) { + // The staging directory holds the only copy of a replaced version. + throw new Error( + `${reason} The staging directory ${staging} was kept, so nothing ` + + `in it is lost; move what it names back, then delete it.` + ); + } + const unremoved = await removeStaging(staging); + const failed = current ?? steps.at(-1)?.id; + throw new Error( + `${failed === undefined ? "The rules" : `Rule "${failed}"`} could ` + + `not be written (${reason}). Every rule already moved into place was ` + + `rolled back, so .taskless/rules/ is as it was.` + + (unremoved === undefined ? "" : ` ${unremoved}`) + ); + } + + const unremoved = await removeStaging(staging); + if (unremoved !== undefined) onWarning?.(unremoved); + + // After the commit, and asked of the real tree, as `writePreparedRule` asks + // them: these are observations about rules that are now on disk. + if (onWarning === undefined) return; + for (const [index, { id, assessment }] of rules.entries()) { + const warn = (message: string): void => { + onWarning(message, index); + }; + const missingFixtures = describeMissingFixtures(assessment.files); + if (missingFixtures !== undefined) { + warn(`Rule "${id}" ${missingFixtures}.`); + } + await warnOnIdCollision(cwd, id, warn); + } +} + +/** + * Move every staged rule into `.taskless/rules/`, recording each move in + * `steps`, and undo the ones already made if any move fails. Throws the + * failure once the undo is complete, or {@link RollbackIncompleteError} when + * the undo itself could not finish. + */ +async function commitStaged( + cwd: string, + staging: string, + rules: readonly PreparedRule[], + steps: CommitStep[] +): Promise { + const replaced = join(staging, "replaced"); + await mkdir(replaced); + try { + for (const [index, rule] of rules.entries()) { + const step: CommitStep = { + id: rule.id, + target: ruleDirectory(cwd, rule.engine, rule.id), + committed: false, + }; + // Recorded before the first rename, so a failure on either rename of + // this rule is undone along with every rule before it. + steps.push(step); + const previous = join(replaced, String(index)); + try { + await rename(step.target, previous); + step.previous = previous; + } catch (error) { + // Nothing there yet is the ordinary case for a new rule. + if (!isMissingDirectory(error)) throw error; + } + step.createdParent = await mkdir(dirname(step.target), { + recursive: true, + }); + await rename(ruleDirectory(staging, rule.engine, rule.id), step.target); + step.committed = true; + } + } catch (error) { + const stranded = await rollBack(steps); + if (stranded.length > 0) { + const reason = error instanceof Error ? error.message : String(error); + throw new RollbackIncompleteError(reason, stranded); + } + throw error; + } +} + +/** + * Undo {@link commitStaged}'s steps, last first, and return a description of + * each one that could not be undone. Every step is attempted even after one + * fails, so as much as possible is put back. + */ +async function rollBack(steps: readonly CommitStep[]): Promise { + const stranded: string[] = []; + for (const step of steps.toReversed()) { + try { + if (step.committed) { + await rm(step.target, { recursive: true, force: true }); + } + if (step.createdParent !== undefined) { + await removeCreatedParents(step.target, step.createdParent); + } + if (step.previous !== undefined) { + await rename(step.previous, step.target); + } + } catch (error) { + const code = (error as NodeJS.ErrnoException).code ?? String(error); + stranded.push( + step.previous === undefined + ? `rule "${step.id}" may still be at ${step.target} (${code})` + : `the previous version of rule "${step.id}" is at ${step.previous}, not ${step.target} (${code})` + ); + } + } + return stranded; +} + +/** + * Remove the directories `mkdir` created for a rule, deepest first, stopping + * at the topmost one it created. `rmdir` rather than a recursive `rm`, so a + * directory something else has since written into is refused, never emptied. + */ +async function removeCreatedParents( + target: string, + createdParent: string +): Promise { + for (let directory = dirname(target); ; directory = dirname(directory)) { + await rmdir(directory); + if (directory === createdParent || dirname(directory) === directory) { + return; + } + } +} + +/** Remove the staging directory, returning a warning when it could not be. */ +async function removeStaging(staging: string): Promise { + try { + await rm(staging, { recursive: true, force: true }); + return undefined; + } catch (error) { + const code = (error as NodeJS.ErrnoException).code ?? String(error); + return `The staging directory ${staging} could not be removed (${code}); it holds nothing still in use and can be deleted.`; + } +} + +/** A failed commit whose undo could not put everything back. */ +class RollbackIncompleteError extends Error { + constructor(reason: string, stranded: readonly string[]) { + super( + `The rules could not be written (${reason}), and rolling back the ` + + `rules already moved into place did not finish: ${stranded.join("; ")}.` + ); + this.name = "RollbackIncompleteError"; + } +} + /** * Check, then write, one rule served by the v2 API: * {@link prepareServedRule} followed by {@link writePreparedRule}. For a diff --git a/packages/cli/src/rules/generate.ts b/packages/cli/src/rules/generate.ts index 2417d789..92ea7d85 100644 --- a/packages/cli/src/rules/generate.ts +++ b/packages/cli/src/rules/generate.ts @@ -12,9 +12,10 @@ import { CLIError } from "../util/cli-error"; import { getCliPrefix } from "../util/package-manager"; import { prepareServedRule, - writePreparedRule, + writePreparedRules, type PreparedRule, } from "./files"; +import { ruleFilePath } from "./engines"; import { verifyServedRule } from "./verify-delivery"; /** @@ -232,8 +233,18 @@ export interface Delivered { * of those leaves the tree exactly as it was, rather than holding half a * delivery the command then reports as nothing. * - * What this does not cover is an IO failure part-way through the writes - * themselves (a full disk, say): the rules written before it stay written. + * The writes are then all or nothing too, through + * {@link writePreparedRules}: every rule is staged in full under `.taskless/` + * before any is moved into `.taskless/rules/`, and an IO failure while + * staging or moving (a full disk, an `EACCES` on the second rule) rolls back + * the rules already moved, so the command fails with + * `RULE_GENERATION_FAILED` and `.taskless/rules/` holds what it held before. + * + * What remains is a process killed outright between two renames, which no + * rollback runs for: some rules are then at their new version and the rest + * at their old, with every replaced version kept under + * `.taskless/.tmp-delivery-*`. A rollback that itself fails keeps that + * directory too, and the error says where each replaced version is. */ export async function deliverRevisions( cwd: string, @@ -285,16 +296,37 @@ export async function deliverRevisions( } const delivered: Delivered = { rules: [], files: [], notices: [] }; + if (ready.length === 0) return delivered; + + // Write warnings arrive tagged with their rule, and join that rule's + // notices; one about the delivery as a whole goes after every rule's. + const deliveryNotices: string[] = []; + try { + await writePreparedRules( + cwd, + ready.map(({ prepared }) => prepared), + (message, index) => { + const rule = index === undefined ? undefined : ready[index]; + (rule?.notices ?? deliveryNotices).push(message); + } + ); + } catch (error) { + if (error instanceof CLIError) throw error; + throw new CLIError( + error instanceof Error ? error.message : String(error), + "RULE_GENERATION_FAILED" + ); + } + for (const { served, prepared, notices } of ready) { + const ruleFile = ruleFilePath(cwd, prepared.engine, prepared.id); delivered.notices.push(...notices); - const ruleFile = await writePreparedRule(cwd, prepared, (message) => { - delivered.notices.push(message); - }); delivered.rules.push(prepared.id); delivered.files.push(ruleFile); const planNotice = notRunOnPlanNotice(served, prepared.engine, ruleFile); if (planNotice !== undefined) delivered.notices.push(planNotice); } + delivered.notices.push(...deliveryNotices); return delivered; } diff --git a/packages/cli/test/rule-create-multi-rule.test.ts b/packages/cli/test/rule-create-multi-rule.test.ts index 7d345a14..7fe8df6e 100644 --- a/packages/cli/test/rule-create-multi-rule.test.ts +++ b/packages/cli/test/rule-create-multi-rule.test.ts @@ -1,5 +1,6 @@ import { execFileSync } from "node:child_process"; import { + chmod, mkdir, mkdtemp, readdir, @@ -23,7 +24,11 @@ import { runCommand } from "citty"; import { ruleCommand } from "../src/commands/rules"; import { LATEST_SCHEMA_VERSION } from "../src/filesystem/migrate"; -import { ruleDirectory, ruleFilePath } from "../src/rules/engines"; +import { + engineRulesDirectory, + ruleDirectory, + ruleFilePath, +} from "../src/rules/engines"; import { servedBody, stubV2Server, type StubRule } from "./support/v2-server"; /** @@ -50,16 +55,39 @@ const goodRule: StubRule = { const EXISTING = "id: first-good-rule\nlanguage: TypeScript\nrule:\n pattern: bar\n"; -/** Serve a request that produced `goodRule`, then `second`. */ -async function serve(second: StubRule): Promise { +/** Serve a request that produced `first` (`goodRule` unless named), then `second`. */ +async function serve(second: StubRule, first = goodRule): Promise { stubV2Server({ produced: [ - { rule: goodRule, body: await servedBody(goodRule, "rev-1") }, + { rule: first, body: await servedBody(first, "rev-1") }, { rule: second, body: await servedBody(second, "rev-1") }, ], }); } +/** A complete Vale rule, so its write lands in a different engine directory. */ +const valeRule: StubRule = { + id: "second-rule", + engine: "vale", + files: [ + { path: "tests/fail/case/case.md", content: "Some text.\n" }, + { + path: "second-rule.yml", + content: + "extends: existence\nmessage: no\nlevel: error\ntokens:\n - text\n", + }, + { path: ".vale.ini", content: "[*.md]\nBasedOnStyles = Taskless\n" }, + ], +}; + +/** + * Whether a read-only directory actually refuses this process. It does not + * for root, which CI containers sometimes are, nor on Windows, where `chmod` + * only toggles the read-only attribute on files. + */ +const readOnlyDirectoriesRefuse = + process.platform !== "win32" && process.getuid?.() !== 0; + describe("rule create: a request with several rules", () => { let cwd: string; let logSpy: MockInstance<(...data: unknown[]) => void>; @@ -102,6 +130,9 @@ describe("rule create: a request with several rules", () => { }); afterEach(async () => { + // A test may leave an engine directory read-only; hand it back so the + // temporary tree can be removed. + await chmod(engineRulesDirectory(cwd, "vale"), 0o755).catch(() => {}); vi.restoreAllMocks(); vi.unstubAllGlobals(); delete process.env.TASKLESS_TOKEN; @@ -238,4 +269,77 @@ describe("rule create: a request with several rules", () => { second.files[1]?.content ); }); + + /** + * An IO failure on the second rule's write, after both rules passed every + * check: `.taskless/rules/vale/` is read-only, so the Vale rule cannot be + * moved into place, and the `sg` rule committed before it must be undone. + */ + async function failSecondWrite(): Promise { + await mkdir(engineRulesDirectory(cwd, "vale"), { recursive: true }); + await chmod(engineRulesDirectory(cwd, "vale"), 0o555); + } + + /** Any staging directory a delivery left inside `.taskless/`. */ + async function leftovers(): Promise { + const entries = await readdir(join(cwd, ".taskless")); + return entries.filter((name) => name.startsWith(".tmp-delivery-")); + } + + it.skipIf(!readOnlyDirectoriesRefuse)( + "restores an overwritten rule when a later write fails", + async () => { + // A stray beside the seeded rule, so the restore is shown to bring back + // the whole old directory and not only the file being replaced. + const stray = join( + ruleDirectory(cwd, "sg", "first-good-rule"), + "tests", + "pass", + "old", + "case.ts" + ); + await mkdir(join(stray, ".."), { recursive: true }); + await writeFile(stray, "bar();\n"); + const before = await seededTree(); + await serve(valeRule); + await failSecondWrite(); + + await expect(create()).rejects.toThrow(); + const { code, message } = envelope(); + expect(code).toBe("RULE_GENERATION_FAILED"); + expect(message).toContain('Rule "second-rule" could not be written'); + expect(message).toContain("EACCES"); + expect(await rulesTree()).toEqual(before); + expect(await leftovers()).toEqual([]); + } + ); + + it.skipIf(!readOnlyDirectoriesRefuse)( + "removes a new rule when a later write fails", + async () => { + const before = await seededTree(); + const newRule: StubRule = { + id: "brand-new-rule", + engine: "sg", + files: [ + { path: "tests/fail/case/case.ts", content: "foo();\n" }, + { + path: "brand-new-rule.yml", + content: + "id: brand-new-rule\nlanguage: TypeScript\nrule:\n pattern: foo\n", + }, + ], + }; + await serve(valeRule, newRule); + await failSecondWrite(); + + await expect(create()).rejects.toThrow(); + expect(envelope().code).toBe("RULE_GENERATION_FAILED"); + expect(await rulesTree()).toEqual(before); + await expect( + readdir(ruleDirectory(cwd, "sg", "brand-new-rule")) + ).rejects.toThrow(); + expect(await leftovers()).toEqual([]); + } + ); }); From efecc58e1a92aa61fe0ff0f02a183b0f29fca0b6 Mon Sep 17 00:00:00 2001 From: Jakob Heuser Date: Sat, 7 Nov 2026 21:05:24 -0800 Subject: [PATCH 2/2] spec(rules): archive multi-rule-delivery-rolls-back-io-failure --- .../.openspec.yaml | 0 .../proposal.md | 0 .../specs/cli-generated-rule-delivery/spec.md | 0 .../tasks.md | 0 .../specs/cli-generated-rule-delivery/spec.md | 25 ++++++++++++++++++- 5 files changed, 24 insertions(+), 1 deletion(-) rename openspec/changes/{multi-rule-delivery-rolls-back-io-failure => archive/2026-11-07-multi-rule-delivery-rolls-back-io-failure}/.openspec.yaml (100%) rename openspec/changes/{multi-rule-delivery-rolls-back-io-failure => archive/2026-11-07-multi-rule-delivery-rolls-back-io-failure}/proposal.md (100%) rename openspec/changes/{multi-rule-delivery-rolls-back-io-failure => archive/2026-11-07-multi-rule-delivery-rolls-back-io-failure}/specs/cli-generated-rule-delivery/spec.md (100%) rename openspec/changes/{multi-rule-delivery-rolls-back-io-failure => archive/2026-11-07-multi-rule-delivery-rolls-back-io-failure}/tasks.md (100%) diff --git a/openspec/changes/multi-rule-delivery-rolls-back-io-failure/.openspec.yaml b/openspec/changes/archive/2026-11-07-multi-rule-delivery-rolls-back-io-failure/.openspec.yaml similarity index 100% rename from openspec/changes/multi-rule-delivery-rolls-back-io-failure/.openspec.yaml rename to openspec/changes/archive/2026-11-07-multi-rule-delivery-rolls-back-io-failure/.openspec.yaml diff --git a/openspec/changes/multi-rule-delivery-rolls-back-io-failure/proposal.md b/openspec/changes/archive/2026-11-07-multi-rule-delivery-rolls-back-io-failure/proposal.md similarity index 100% rename from openspec/changes/multi-rule-delivery-rolls-back-io-failure/proposal.md rename to openspec/changes/archive/2026-11-07-multi-rule-delivery-rolls-back-io-failure/proposal.md diff --git a/openspec/changes/multi-rule-delivery-rolls-back-io-failure/specs/cli-generated-rule-delivery/spec.md b/openspec/changes/archive/2026-11-07-multi-rule-delivery-rolls-back-io-failure/specs/cli-generated-rule-delivery/spec.md similarity index 100% rename from openspec/changes/multi-rule-delivery-rolls-back-io-failure/specs/cli-generated-rule-delivery/spec.md rename to openspec/changes/archive/2026-11-07-multi-rule-delivery-rolls-back-io-failure/specs/cli-generated-rule-delivery/spec.md diff --git a/openspec/changes/multi-rule-delivery-rolls-back-io-failure/tasks.md b/openspec/changes/archive/2026-11-07-multi-rule-delivery-rolls-back-io-failure/tasks.md similarity index 100% rename from openspec/changes/multi-rule-delivery-rolls-back-io-failure/tasks.md rename to openspec/changes/archive/2026-11-07-multi-rule-delivery-rolls-back-io-failure/tasks.md diff --git a/openspec/specs/cli-generated-rule-delivery/spec.md b/openspec/specs/cli-generated-rule-delivery/spec.md index 20dd1887..ef05c61a 100644 --- a/openspec/specs/cli-generated-rule-delivery/spec.md +++ b/openspec/specs/cli-generated-rule-delivery/spec.md @@ -185,7 +185,16 @@ any of them. A request in which any rule is refused SHALL leave `.taskless/rules exit with an error. A refusal from these checks SHALL carry an error code: `RULE_UNSUPPORTED` for an engine this CLI does not know, and `RULE_GENERATION_FAILED` otherwise. -An IO failure while writing, after every rule passed, is outside this guarantee. +The writes SHALL be all or nothing as well. The CLI SHALL stage every rule in full under +`.taskless/`, on the same filesystem as `.taskless/rules/`, before moving any into place. When an +IO failure occurs while staging or moving, the CLI SHALL undo every move already made, restoring +any version of a rule it replaced, SHALL remove the staging directory, and SHALL exit with +`RULE_GENERATION_FAILED`, leaving `.taskless/rules/` as it was. When the undo itself cannot +finish, the CLI SHALL keep the staging directory and SHALL name where each replaced version is. + +A process killed outright between two moves is outside this guarantee: each move is atomic, the +sequence of them is not, and no rollback runs. The replaced versions SHALL then still be on disk +in the staging directory. #### Scenario: A later rule with an unknown engine writes nothing @@ -198,3 +207,17 @@ An IO failure while writing, after every rule passed, is outside this guarantee. - **WHEN** a request produced two rules and the second delivers a path containing `..` - **THEN** the CLI SHALL exit with `RULE_GENERATION_FAILED`, naming the second rule - **AND** `.taskless/rules/` SHALL be unchanged + +#### Scenario: A write that fails on a later rule restores the earlier one + +- **WHEN** a request produced two rules, the first replaces an older version already on disk, and moving the second into place fails with an IO error +- **THEN** the CLI SHALL exit with `RULE_GENERATION_FAILED`, naming the second rule +- **AND** `.taskless/rules/` SHALL be byte-identical to how it started, including every file of the older version +- **AND** no staging directory SHALL remain under `.taskless/` + +#### Scenario: A write that fails on a later rule removes a new earlier one + +- **WHEN** a request produced two rules, the first is new, and moving the second into place fails with an IO error +- **THEN** the CLI SHALL exit with `RULE_GENERATION_FAILED` +- **AND** the first rule's directory SHALL NOT exist +- **AND** no staging directory SHALL remain under `.taskless/`