[theCodeDrift]

Stack (root → tip):

• fix(rules): check every generated rule before writing the first #632
  • ➡️ fix(rules): roll back a rule delivery that fails mid-write #635 (you are here)

Problem

#632 makes rule create / rule improve check every rule a request produced before writing the first. It leaves out one case, and says so in the deliverRevisions doc comment and the cli-generated-rule-delivery spec: an IO failure part-way through the writes (a full disk, EACCES on the second rule). In that case rules 1..N-1 stay on disk while the command fails and reports no rules. Under --json the failure was also INTERNAL_ERROR.

What changed

• New writePreparedRules in rules/files.ts, used by deliverRevisions:
  1. Stage. Every rule is written in full into .taskless/.tmp-delivery-*/ (mkdtemp inside .taskless/, so it is on the same filesystem as .taskless/rules/ and rename is never cross-device). It goes through the same writeDeliveredFileSet as before, with the staging dir standing in for the project root. A full disk or unwritable file stops here, before anything under .taskless/rules/ has moved.
  2. Commit. For each rule, any existing directory is renamed aside into the staging dir, then the staged directory is renamed onto the rule path. If a rename fails, the moves already made are undone in reverse order: the new directory is removed, the old one renamed back, and any parent directories the commit created are rmdired. Then the staging dir is removed.
• A write-phase failure now surfaces as CLIError RULE_GENERATION_FAILED, naming the rule and the errno, and saying .taskless/rules/ is as it was.
• If the rollback itself cannot finish, the staging dir is kept, because it holds the only copy of a replaced version, and the error names where each one is.
• A symlinked rule directory is still refused (assertRuleDirectoryIsNotALink, now exported from deliver.ts). That check now runs before anything is staged, rather than part-way through the writes.
• Fixture and id-collision warnings are computed after the commit, against the real tree, as writePreparedRule did.
• rule restore / rule rollback (writeServedRule, one rule each) are unchanged. Their PurgeIncompleteError handling in recover.ts depends on the in-place write, and moving them over is a separate decision.

Residual window (not atomic)

Each rename is atomic on one filesystem, but the sequence of renames is not. If the process is killed outright (SIGKILL, power loss) between two renames, no rollback runs. .taskless/rules/ can then hold some rules at their new version and the rest at their old one, and one rule can be absent for the instant between its two renames. Every replaced version is still on disk under .taskless/.tmp-delivery-*, but nothing restores it automatically. Nothing is fsynced, so after a power loss the filesystem decides the order the renames reached the disk. All of this is written down in the writePreparedRules and deliverRevisions doc comments and in the spec requirement.

Testing

• rule-create-multi-rule.test.ts has two new cases, both driven through the real rule create --json. In each, the second rule is a Vale rule whose engine directory .taskless/rules/vale/ is read-only (0o555), so moving it into place fails with a real EACCES:
  • the first rule replaces an older version already on disk, with a stray fixture beside it: the old directory comes back byte-identical
  • the first rule is new: its directory does not exist afterwards
  • both cases assert RULE_GENERATION_FAILED, that .taskless/rules/ is unchanged, and that no .tmp-delivery-* is left behind. They are skipped as root or on Windows, where a read-only directory does not refuse the write.
• Against fix(rules): check every generated rule before writing the first #632's code, both new tests fail (expected 'INTERNAL_ERROR' to be 'RULE_GENERATION_FAILED').
• pnpm typecheck, pnpm lint, pnpm --filter @taskless/cli test (2972 passed) all pass.
• OpenSpec: a MODIFIED delta on "Every rule of a request is checked before any is written" restates the requirement in full and adds two scenarios. The pre-archive scenario check went from 23 to 25 with none dropped, and the change is archived here.
• The fix(rules): check every generated rule before writing the first #632 changeset was extended rather than adding a second one (still patch).
• git merge-tree --write-tree origin/refactor/626-explicit-schema-policy HEAD: clean, no conflicts with refactor(schema): explicit scaffold state and per-command migrate/refuse policy #634.

Merge order

Stacked on #632; land after it.

Refs #623

Built on top of #632

A rule-generation request can produce several rules. deliverRevisions verified every rule's signatures up front, but the other pre-write checks (rule id, known engine, and assessDelivery: unsafe paths, case-folded duplicates, incomplete sets) ran inside the write loop. So when rule 2 was refused, rule 1 was already in .taskless/rules/ while the command failed and reported no rules. Under --json those refusals also surfaced as INTERNAL_ERROR.

What changed

• writeServedRule is split into prepareServedRule (every check, no filesystem writes) and writePreparedRule (ensureTasklessDirectory, the write, the missing-fixtures and id-collision warnings). writeServedRule stays as prepare + write, so rule restore / rollback behave exactly as before, error types included.
• deliverRevisions now verifies and prepares every rule before writing the first. A prepare refusal that is a plain Error is wrapped as RULE_GENERATION_FAILED there (not at the source, so restore's reporting is unchanged); RULE_UNSUPPORTED keeps its code. Per-rule notices are held and released in request order, so output ordering is unchanged.
• The doc comment on deliverRevisions now states what it guarantees, and names the remaining gap: an IO failure part-way through the writes (out of scope, per the issue).
• OpenSpec: an ADDED requirement in cli-generated-rule-delivery, archived in this PR (21 scenarios before, 23 after).
• Patch changeset.

Testing

• New packages/cli/test/rule-create-multi-rule.test.ts drives the real rule create --json against the v2 stub with two rules: second rule with an unknown engine, an unsafe ../ path, or an incomplete set. Each asserts the error code and that .taskless/rules/ (pre-seeded with an older version of rule 1) is byte-identical afterwards. A fourth case checks both rules are written when both pass. The three refusal cases fail against the previous generate.ts.
• pnpm typecheck, pnpm lint, pnpm --filter @taskless/cli test (2970 passed).

Fixes #623

[theCodeDrift]

@claude /review

[github-actions]

Claude finished @theCodeDrift's task in 4m 24s —— View job

---

Review complete

Review mode: incremental — read 0 prior review thread(s) before reviewing. .prior-review.json held no threads, reviews, or top-level comments beyond the trigger itself, so this is the first review of this PR; the whole diff was assessed.

• Read .prior-review.json
• Fetch PR diff and metadata
• Review packages/cli/src/rules/files.ts (staging/commit/rollback logic)
• Review packages/cli/src/rules/deliver.ts and generate.ts
• Review test coverage in rule-create-multi-rule.test.ts
• Review OpenSpec changes
• Post final review

Summary: I traced writePreparedRules/commitStaged/rollBack/removeCreatedParents in packages/cli/src/rules/files.ts step by step, including the interaction between previous (an existing version moved aside) and createdParent (a fresh parent mkdir created). The two are mutually exclusive for a given step — createdParent is only set when the engine directory didn't exist, which means the rule couldn't have had a previous version either — so the rollback ordering (rm new dir → removeCreatedParents → rename old dir back) never tries to rename into a parent it just deleted. For rules sharing a newly-created engine directory within the same batch, forward-order creation and reverse-order rollback correctly leave createdParent cleanup until the directory is actually empty of siblings. I also checked that staging stays on the same filesystem as .taskless/rules/ (both under .taskless/), that the symlink guard runs before any staging happens, and that warnings/collision checks run against the real tree post-commit as intended. I did not find a correctness or security bug in the staging/commit/rollback logic, deliver.ts's now-exported assertRuleDirectoryIsNotALink, or the generate.ts wiring (including the new ready.length === 0 early return, which just avoids creating/removing a pointless empty staging directory).

Nothing new to report as a defect. Two very minor, non-blocking observations, neither of which I think needs a change:

• commitStaged/rollBack are correct for every path the new tests exercise, but the specific sub-case of a rule that replaces an existing version failing on the second (into-place) rename — rather than the first (move-aside) rename — isn't exercised by either new test; both drive the failure through a brand-new engine directory, so previous is always undefined on the failing step. The logic traces out fine by hand, but a test covering that path would pin it down.
• In writePreparedRules (files.ts:224-225), if removeStaging fails after a successful commit and the caller passed no onWarning, the leftover-staging-directory warning is silently dropped. This matches the existing optional-onWarning convention elsewhere in the file (e.g. writePreparedRule's fixture/collision warnings), and the only real caller (generate.ts) always supplies a callback, so it's not a behavior change worth blocking on.

The implementation matches what the writePreparedRules/deliverRevisions doc comments and the OpenSpec delta promise, the two new --json tests genuinely exercise the rollback path with a real EACCES (and fail against #632 as claimed), and the spec/changeset/archive text is consistent with the code.

[theCodeDrift]

Re: @github-actions — "Claude finished @theCodeDrift's task in 4m 24s…"
#635 (comment)

Thanks. Leaving both as they are. (1) To fail the into-place rename of a rule that is replacing an older version, the move-aside rename has to succeed in the same directory just before it. A real-filesystem failure cannot separate the two (they need the same permission on the same parent), and doing it through a mocked fs would test the mock rather than the rollback. The undo for that step is the previous branch that the overwrite test already exercises on rule 1. (2) A dropped warning when onWarning is absent follows the convention used everywhere else in the file, and the only caller always passes one.

— AI Coding Agent