fix: make a delivered file set define the rule directory - #236
Conversation
`writeDeliveredFileSet` wrote every file in a delivered set and removed nothing, so a file already on disk that the set did not mention survived the write. The schema calls `files` "every file the rule directory must contain", so the write now makes the directory match the set. One rule for all three callers (`rule create`, `rule improve`, and `check`'s repair) rather than a repair-only mode: a helper that merged for two and purged for the third would let `improve` leave behind exactly the stray `check` later has to remove. Deletion is bounded to `.taskless/rules/<engine>/<id>/` by construction (candidates are discovered by walking that directory, never derived from payload content) and then by `describeUnsafePath`, the same guard the delivered paths pass. Assess and purge are a unit, so a refused set leaves the directory exactly as it was, and writes precede deletions so an IO failure part-way cannot leave the rule missing pieces. `.tests/` is exempt and the comment says so: nothing there reaches an engine, and this CLI writes timestamped fixtures there that no delivered set will ever name. Fixes #233
|
@claude /review |
|
Claude finished @thecodedrift's task in 6m 0s —— View job Review completeReview mode: incremental — read 0 prior review thread(s) before reviewing. The
Overall this is a well-reasoned, well-documented change, and the core design (purge bounded by construction +
Nothing else stood out: the symlink/ |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Two gaps at the edges of the delivered-set purge, both found in review. The write loop followed symlinks while the purge deliberately did not. `mkdir` with `recursive` is satisfied by a link to an existing directory and `writeFile` follows a link to a file, so a planted `captures` or `check.ts` sent the blessed bytes outside `.taskless/rules/<engine>/<id>/` and left the rule directory without them - a rule that verifies as incomplete and never fires, reported as a successful delivery. A link inside the rule directory is now unlinked before the write, matching what the purge already does to the same entry; a rule directory that is itself a link is refused outright, since that one would also hand the purge whatever the link points at. A failed removal no longer abandons the pass. `rm` with `force` swallows only ENOENT, so a real failure on one stray left every later stray in place, and which ones survived depended on `readdir` order. Failures are collected, the pass continues, and the error names each entry it could not remove - loudly, because an unremoved stray is still a file an engine reads. Writes still come first, so a failed purge is a named leftover beside a complete rule, never a missing piece.
Both medium findings are fixed in #3 (write path follows symlinks) was an actual escape of the boundary, not only an asymmetry with the purge: #1 (partial purge on a mid-loop IO error) now collects failures and finishes the pass, then throws naming every entry it could not remove. Abandoning at the first failure made the surviving strays a function of Four new tests cover both (including a real — AI Coding Agent |
Review follow-up on #236, taking both low items as one change because the second is only worth doing as the first's mechanism. A purge failure became a normal, reachable outcome in the previous commit, and it landed in a catch that reported "could not be written". That is the opposite of true: the blessed bytes ARE on disk and only the cleanup is partial. A reader acting on it re-runs a repair that already succeeded, or concludes the rule is unrepaired and edits it by hand. The error string already said so, wrapped in a sentence contradicting it. `PurgeIncompleteError` carries the surviving entries, so the caller branches on a type rather than matching prose — which is the point: the two failures ask the reader for opposite things ("the rule is not there" versus "the rule is there and something stale is too"), and a call site that could only read the message would get it wrong the first time the wording changed. The repair notice now names what survived and says an engine still reads it. The second item was `DeliveryWrite.written`/`.removed`, computed and never consumed. Resolved by deleting the shape rather than plumbing it to a caller. Plumbing it would mean widening `writeRuleFile`'s return, which changes call sites in `commands/rules.ts` — a file PR #238 owns concurrently — for a value nothing needed once the failure path carries its own detail. The data now lives where it is used. Verified the new test bites by throwing a plain `Error` instead: it fails, and passes again when restored. Five unrelated tests timed out at the limit on one run and did not reproduce across four further runs, including three consecutive runs of the two suites this change touches.
1 participant
The defect
writeDeliveredFileSet(packages/cli/src/rules/deliver.ts) wrote every file in a delivered set and removed nothing, so a file already on disk that the set did not mention survived the write.That is unremarkable for
rule create, where the directory is normally new, and nearly so forrule improve. It is wrong for the repair path incheck: repair runs precisely because the directory's trustworthiness is in question — anunsafeverdict means the on-disk rule diverged from what the server blessed. Onlycheck.tsis signed and reconciled, so a stray file elsewhere in the rule directory (a leftover undercaptures/, say) is never reported by reconcile, was not replaced by the repair, and went on changing what the rule matched. The rule read as repaired and was not.The decision, and it is one decision for all three callers
The delivered set defines the rule directory's contents, for every caller. The published schema calls
files"Every file the rule directory must contain"; if that is the contract, writing must make the directory match the set rather than merge into it. A file the set does not name is not part of the rule.Repair is deliberately not special-cased. Either the set defines the directory or it does not. A helper that merged for
create/improveand purged for repair would meanimproveleaves behind exactly the stray thatchecklater has to remove, and "what is in this rule directory" would have two answers depending on which command wrote it last.Blast radius across the three callers, all via
writeRuleFile:rule create(commands/rules.ts)rule improve/ iterate (commands/rules.ts)repairWithheldRules,commands/check.ts)The single-content envelope is untouched — there is no set to be authoritative about, so it keeps overwriting one file. Pinned by a test.
How deletion is bounded
Scoped strictly to
.taskless/rules/<engine>/<id>/, twice over:describeUnsafePath, the same guard the delivered paths pass — the existing machinery, not a second path guard — so the rule for "written inside this directory" and the rule for "removed from it" cannot drift apart. A candidate it refuses is left alone.Two further properties:
writeDeliveredFileSettakes anassessDeliveryverdict, obtainable no other way, so a refused set never reaches the purge and leaves the directory exactly as it was. A half-purged directory is worse than a merged one.Details worth a reviewer's eye:
readdirdoes not follow them, so a link to a directory is listed as a file;rmunlinks the link and never touches its target. Tested.rmdir, not a recursiverm, becausermdirrefuses a non-empty directory: a directory still holding a delivered file, or the preserved.tests/, survives by the filesystem's answer rather than by bookkeeping. OnlyENOTEMPTY/EEXIST/ENOENTare swallowed.assessDelivery. On a case-insensitive filesystem a deliveredcaptures/Logs.ymlwritten into an existingcaptures/logs.ymlkeeps the on-disk spelling, and an exact compare would delete the file just written — leaving a rule with no capture, which verifies as incomplete and never fires. The cost is that a stale case-differing sibling is spared on a case-sensitive filesystem. That is the right way round to be wrong: sparing a stale file leaves the merge behavior this change narrows; deleting a live one makes the rule inert, which is the failure mode this codebase keeps hitting and does not trade for tidiness.What happens to
.tests/.tests/is preserved, and the code says so explicitly (aPRESERVED_SUBTREESconstant carrying the reasoning). Two reasons, the first being the one that matters:.tests/reaches an engine. The dot is what makes ast-grep skip the directory during rule discovery (documented onRULE_TESTS_DIRECTORY);strayModulesalready exempts it for the same reason, and runtime capture discovery skips it by name. A stale fixture cannot change what a rule matches, which is the entire harm the purge exists to prevent. A stale fixture fails a test run loudly, in front of someone already looking at that rule.writeRuleTestFilewrites<id>-<timestamp>-test.ymlon every single-content create and iterate, so fixtures accumulate locally and are absent from a later file-set delivery by construction. Purging.tests/would delete a rule's whole local test history the first time it was redelivered as a file set.Only the top-level
.tests/is exempt.RULE_TESTS_DIRECTORYis relative to the rule directory, so a nestedcaptures/.tests/is a file an engine reads, not a fixture directory, and it is purged like any other — otherwise the exemption is a hiding place. Tested.The repair notice names this rather than glossing it: it now says the rule directory holds exactly the delivered files apart from test fixtures under
.tests/.Cannot leave a rule inert
A complete set cannot be purged into an incomplete rule:
assessDeliveryalready requires the rule file, the engine config where there is one, and at least one capture where the engine uses them, and all of those are written before the purge runs. Two of the new tests assert the rule is still whole after the purge, not merely that the stray is gone.Tests, and the revert check
Extended both files the change touches:
packages/cli/test/deliver.test.ts— 7 new cases: stray capture removed; stray module andnotes.mdremoved with the emptiedlib/pruned whilecaptures/survives;.tests/fixtures kept; nestedcaptures/.tests/purged; symlink unlinked without touching its target; refused set leaves the directory untouched; single-content envelope does not purge.packages/cli/test/repair-integration.test.ts— end to end against the mock: a rule directory carrying a stray capture and a.tests/fixture, repaired from arestoreresponse, ends with the stray gone, the fixture kept,check.tsand the blessed capture in place, and the notice sayingwas restoredand naming.tests/.Revert check. With the purge call disabled (
writeDeliveredFileSetreturning without callingpurgeUndeliveredFiles) and the CLI rebuilt, 5 of the 8 new tests fail: all four positive deletion cases indeliver.test.tsand the repair-integration case. The three that still pass are the negative ones —.tests/kept, refused set leaves the directory untouched, single-content does not purge — which pin behavior that must not change, so passing against the old code is the correct result for them. Restored and re-run green.Checks
pnpm build,pnpm typecheck,pnpm lint,pnpm --filter @taskless/cli exec vitest run(70 files, 1148 tests, all passing), andpnpm cli checkagainst a fresh build.checkreports only the 4 pre-existingno-hedgingwarnings inroute.txtandonboard.txt, none in files this PR touches.Changeset added:
.changeset/delivered-set-is-the-directory.md(patch,@taskless/cli).Fixes #233