[theCodeDrift]

Problem

rule restore and rule rollback wrote their one rule in place (writeServedRule → writeDeliveredFileSet): delivered files first, then a purge of entries the set did not name. An IO error part-way (a full disk, EACCES on one file) left .taskless/rules/<engine>/<id>/ holding a mix of revisions while the command failed with INTERNAL_ERROR.

Measured on main with the new test: a rollback over a version whose fixture file is read-only overwrote the rule file with the new revision, then failed on the fixture with INTERNAL_ERROR: EACCES ... tests/fail/case/case.ts. The rule was left at the new revision with the old fixture.

rule create / rule improve stopped having this problem in #635, which left restore/rollback on the old path on purpose.

What changed

• recover.ts write() now calls prepareServedRule, then writePreparedRules(cwd, [prepared], …), the same staged path rule create uses. The rule is written in full under .taskless/.tmp-delivery-*, then renamed over its directory, with any version already there renamed aside first and renamed back if the move fails.
• Write failure → RULE_RESTORE_FAILED (new code in types/errors.ts and the recover-rule recipe's error table, topic v5). The message is the staged writer's, which names the errno and says .taskless/rules/ is as it was, or names where the previous version was kept if the rollback itself could not finish. This is a new code rather than RULE_RESTORE_MISMATCH because the two need different responses: a mismatch means the bytes were wrong and retrying will not help, while this one means the bytes were right and the command should be run again once the filesystem is fixed.
• A verified set that is still not a writable rule → RULE_RESTORE_MISMATCH (an unsafe fixture path, an incomplete set). Fixtures carry no signature, so such a set gets through verification and was only refused by prepareServedRule, as a plain Error, so it came out as INTERNAL_ERROR. RULE_UNSUPPORTED passes through unchanged.
• writeServedRule and writePreparedRule are deleted. No production code called them after this change. The tests that used writeServedRule as an entry point now go through a test-only helper (test/support/write-served-rule.ts) that does prepare + writePreparedRules, so those tests now exercise the path the commands actually use.

The PurgeIncompleteError branch: removed, nothing replaces it

That branch meant "the rule is on disk, but a stale entry beside it could not be removed", and returned success with a notice. Under rename-based delivery the old directory is moved aside as a whole and removed with the staging directory, so that state no longer exists. The served set either is the directory, or the directory is what it was before. If the purge ever did throw inside staging (it writes into a fresh directory, so it has nothing to purge), writePreparedRules would treat it like any other write error and roll back. A note on PurgeIncompleteError says so.

writeDeliveredFileSet's purge and PurgeIncompleteError themselves are kept. They are now reachable only through that function directly, and the two deliver.test.ts cases that pin the purge call it directly. Removing them, along with the symlink handling that only matters when writing over an existing directory, is a reasonable follow-up. I left it out to keep this PR to the write path.

Behaviour differences (same as #635 accepted for rule create)

• The rule directory is replaced whole. Entries the old directory held that the set does not name are gone, including ones whose names fail describeUnsafePath, which the in-place purge skipped.
• A regular file standing where the rule directory belongs is moved aside and replaced, instead of failing.
• The "N stale entries could not be removed" success notice is gone.
• A read-only file inside the old version no longer splits the rule. The old directory is moved aside as a unit, so the rollback succeeds.

Residual window (not atomic)

Each rename is atomic on one filesystem, but the pair of renames (old version aside, new version in) is not. A process killed outright (SIGKILL, power loss) between them runs no rollback. The rule can then be absent, with its previous version under .taskless/.tmp-delivery-*, and nothing moves it back automatically. Nothing is fsynced. This is documented on write() and in the new spec requirement.

Testing

• rule-recovery.test.ts, driven through the real rule restore --json / rule rollback --json against the stubbed v2 service:
  • a write that fails (.taskless/rules/sg/ made 0o555, so the move fails with a real EACCES): restore of an edited rule, restore of a missing rule, and rollback over the current version with a stray notes.md beside it. Each asserts RULE_RESTORE_FAILED, an EACCES message, exit code 1, .taskless/rules/ byte-identical, and no .tmp-delivery-* left behind. Skipped as root and on Windows.
  • rollback over a version with a read-only fixture file now succeeds, and the directory holds exactly the served files.
  • the rule directory is replaced whole, so stray entries do not survive.
  • a verified set with a tests/../../escape.ts fixture is RULE_RESTORE_MISMATCH, and nothing is written.
  • The old "still names the rule file when only the stale-entry cleanup failed" test is replaced. The state it checked no longer exists.
• Against main's src/, all four new IO tests fail: the three RULE_RESTORE_FAILED cases get INTERNAL_ERROR, or success for the edited-rule case, since the in-place write gets through a read-only engine directory, and the read-only-fixture case gets the half-write described above.
• OpenSpec: restore-rollback-rolls-back-io-failure, archived here. MODIFIED A delivered rule replaces its directory (drops the stale-entry sentence, adds one scenario) and Recovery commands report under --json (adds RULE_RESTORE_FAILED). Both keep their titles and restate every scenario. ADDED A restore or rollback that cannot be written leaves the rule as it was. Pre-archive check: cli-generated-rule-delivery 25 → 26 scenarios, cli-rule-recovery 27 → 32, none dropped.
• One patch changeset.
• pnpm typecheck, pnpm lint, pnpm --filter @taskless/cli test (3065 passed, 11 skipped) all pass.

Fixes #636

[theCodeDrift]

@claude /review

[github-actions]

Claude finished @theCodeDrift's task in 3m 39s —— View job

---

Review complete

Review mode: incremental — read 0 prior review thread(s) before reviewing. .prior-review.json contained no reviewThreads, reviews, or top-level comments beyond the trigger itself, so this is the first review pass on this PR — nothing to classify as already-addressed.

• Read .prior-review.json (empty — first review)
• Fetch PR diff and changed files (gh pr diff)
• Review recover.ts, files.ts, deliver.ts, errors.ts, cli-error.ts
• Review test changes (rule-recovery.test.ts, deliver.test.ts, support helper, and the three other test files that swapped import source)
• Review OpenSpec changes (archived change + synced specs)
• Post final review

Correctness

Traced the new failure path by hand against files.ts's writePreparedRules/commitStaged:

• Write failure (RULE_RESTORE_FAILED): recover.ts's write() now does prepareServedRule then writePreparedRules(cwd, [prepared], onWarning). A rename failure (e.g. the read-only sg/ engine directory in the new tests) propagates out of commitStaged with nothing committed, rollBack is a no-op (no step was committed, nothing created, nothing moved aside yet), and writePreparedRules wraps it with a message containing both the errno and "is as it was" — matching what expectUntouched in rule-recovery.test.ts asserts, and matching the new cli-rule-recovery spec requirement.
• Mismatch vs failed: prepareServedRule throws before any FS write, so routing its non-CLIError throws (invalid id, unsafe path, incomplete set) to RULE_RESTORE_MISMATCH, and routing writePreparedRules's non-CLIError throws to RULE_RESTORE_FAILED, is a clean split. Confirmed ServedFileSet is literally ServedRule["rules"][number] (api/v2.ts:224), so the write() parameter type change is cosmetic, not a behavior change.
• Read-only fixture file: since the whole rule directory is renamed aside as a unit rather than written into, a read-only file inside it never blocks the move (unlink/rename only cares about the containing directory's write permission on POSIX) — the new "rollback over a version holding a read-only file" test checks this correctly.
• Confirmed no production code still references the removed writeServedRule/writePreparedRule (grep came up empty), and the new RULE_RESTORE_FAILED code doesn't need any exhaustive switch update elsewhere — the JSON envelope path is generic over CLIErrorCode.

Minor, non-blocking observations

• recover.ts's second catch (around writePreparedRules) maps any non-CLIError throw to RULE_RESTORE_FAILED, whose message says "the rule is as it was; fix what message names, then run the command again." That bucket also catches assertRuleDirectoryIsNotALink's refusal (a symlink sitting where the rule directory belongs) — a real repo-state problem, not strictly an IO failure. The guidance ("fix what's named, then retry") still happens to be correct for that case, so this isn't wrong, just slightly broader than the comment above it ("An IO failure (a full disk, an unwritable directory)") suggests.
• rule-recovery.test.ts's new "rollback over a version holding a read-only file" test is gated on readOnlyDirectoriesRefuse (skipped on Windows/root), but the behavior it checks (the whole old directory is renamed aside, so the read-only file's permissions never matter to the write itself) doesn't depend on that flag for that rename — it's only relevant to whether removeStaging's later recursive rm of the staging directory can delete a read-only file, which does differ on Windows. Not incorrect, just worth knowing why the guard is there if it's ever questioned.

Tests / spec

• The new IO-failure tests (expectUntouched, rulesTree(), leftovers()) are well-targeted: they check byte-identical state, exit code, and no leftover .tmp-delivery-*, which is exactly what the new spec requirement promises.
• deliver.test.ts's writeInPlace() helper correctly isolates the remaining PurgeIncompleteError coverage to writeDeliveredFileSet called directly, consistent with the PR's statement that no command reaches that branch anymore.
• Archived OpenSpec change and the synced openspec/specs/* deltas match the implementation and tests described above; didn't find any scenario in the spec that the code doesn't satisfy, or vice versa.

No blocking issues found.

[theCodeDrift]

Re: @github-actions — "Claude finished @theCodeDrift's task in 3m 39s … No blocking issues found."
#653 (comment)

Thanks. On the two minor observations:

1. The RULE_RESTORE_FAILED catch is broader than its comment. Fixed in 98aa31d. The comment now names the symlinked-rule-directory refusal as well. As you noted, the code and its remedy ("fix what message names, then run again") are already right for that case, so I kept one code rather than splitting it.
2. The guard on the read-only-file test. Agreed, and left as it is. The guard is there because the test relies on chmod 0o444 actually refusing a write. On Windows chmod only toggles an attribute, and root ignores it, so in those environments the test would prove nothing about the old half-write.

— AI Coding Agent