fix(init): stop installing docs that describe a deleted layout - #232
Conversation
|
@claude /review |
|
Claude finished @thecodedrift's task in 5m 33s —— View job Review completeReview mode: incremental — read 0 prior review thread(s) before reviewing.
AssessmentThe core fix is sound: Two findings posted inline (both
Not inline-commentable, flagging here: the PR's own premise is "stop installing docs that describe a deleted layout," but the same class of drift now exists one layer up, in the project's living OpenSpec specs (not archived, and none of these paths are touched by this diff):
Since these specs are meant to be the durable source of truth (there's even an No other correctness, security, or cross-file issues found. The |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Raised by the generator team as N9, after running our migration on their repository. `.taskless/README.md` and the installed Taskless skill both named `rule-tests/`, a directory `0005` deletes, and the README described rules under `sg/rules/` and `vale/rules/` rather than the current `rules/<engine>/<id>/`. The skill line is the one that bites: it is a trigger description, the text an agent reads before it opens anything, so it taught agents to look in a directory the migration had removed. A MIGRATION IS NOT A SELF-HEAL FOR AN ALREADY-CURRENT PROJECT. `0001` writes the README on every run and says it "overwrites stale content from older versions". That is true while migrating and does nothing otherwise: `runMigrations` only runs migrations above the recorded version, so a project at 5 never ran `0001` again and kept its stale copy forever. Verified on this repository, which is at 5 and whose README still described the pre-`0004` tree after a full `init`. So `0006` rewrites it, and an existing project gets a correct description rather than only new installs. Spending a schema version on documentation is deliberate. The file is generated rather than authored and `0001` already overwrites it unconditionally, so no user content is at risk, and the alternative is a wrong description of the project's own directory that never corrects itself. The layout section is now DERIVED from the layout table rather than described beside it, which is the same move that fixed the seven stale comments: the words cannot disagree with the table because they are the table, and the next migration to relocate rules updates this text by changing the constants it already has to change. Two pieces of the same drift found on the way. `bootstrap.test.ts` asserted the fresh README CONTAINS `rule-tests`, so a passing test was holding the stale description in place, which is why running the suite never found it. And ten tests hardcoded the schema version while the version matrix listed prior versions literally, so each new migration silently stopped covering the version it had just made prior. Both now derive from an exported `LATEST_SCHEMA_VERSION`. `mixed-engine-check.test.ts` wrote its sample into `.taskless/README.md`, which `0006` then overwrote mid-check. Moved to a file no migration manages: the subject is the path exclusion, not that file.
Follow-up to the N9 fix, turning the lesson into a check rather than a thing to remember. A migration that moves, renames or deletes anything under `.taskless/` makes the files describing that directory wrong from that moment, and it is the only commit that knows. Every other mechanism runs later than the moment the fact changed, which is how `rule-tests/` stayed in the installed README and skill description for two releases after `0005` deleted it. The rule is written at the migration registry, where the next person to add one will be looking, with the three things that follow: update what describes the directory, refresh this repository's own `.taskless/` and commit it, and ask whether already-current projects need a version to reach them at all. The check is `installed-documentation.test.ts`, and it asks the question nothing was asking: is the artifact installed in THIS repository the one this build would write. Not whether a generated file matches its own generator, which is vacuous — whether the copy on disk was refreshed after the template changed, which is the step that was missed. It also names `rule-tests/` and `sg/rules/` directly, so a reintroduction is caught by what it says and not only by a whole-file comparison. Verified by reintroducing the stale sentence and confirming both assertions fail.
Review feedback on #232. Nothing exercised the migration on the scenario it exists for. The version matrix forward-migrates from every prior version but only asserts the resulting version number, and `installed-documentation.test.ts` compares this repository's README to `buildReadmeContent` computed directly — it never runs `0006`, so it would have passed with the migration deleted, as long as the file on disk happened to match. That is precisely the gap: the bug is an already-current v5 project whose README is frozen because `runMigrations` returns early, and no test seeded that. One does now — v5 manifest, stale README naming `rule-tests/`, then `ensureTasklessDirectory` — and it fails when `0006` is unregistered, which is how it was checked. Also converts the "a migration owns the prose" block to a section comment. It sat immediately above the `LATEST_SCHEMA_VERSION` docblock, so only the second attached for tooling and the first read as documentation of a constant it has nothing to say about. A note whose whole purpose is that the next migration author reads it should not be attached to the wrong declaration.
03b2979 to
2175ed8
Compare
Both findings taken in 2175ed8. The medium was the one that mattered, and the reasoning is the useful part: The orphaned docblock came from resolving a rebase conflict by keeping both sides. Converted to a Also useful that you checked the whole of Rebased onto current — AI Coding Agent |
1 participant
Stack (root → tip):
Raised by the generator team as N9, after they ran our migration on their own repository.
Two files we install still described the pre-
0004tree:.taskless/README.md— rules undersg/rules/andvale/rules/, plus arule-tests/directory0005deletes.skills/taskless/SKILL.md:6—(rules, rule-tests, rule-metadata).The skill line is the one that bites. It is the YAML
description, which is the trigger text an agent reads in its skill listing before it opens anything, so it taught agents to look in a directory the migration had removed.A migration is not a self-heal for an already-current project
0001writes the README on every run and its comment says it "overwrites stale content from older versions". That is true while migrating and does nothing otherwise:runMigrationsreturns early atmigrate.ts:300when the project is already atmaxVersion, so a project at 5 never runs0001again.Verified rather than assumed — on this repository, which is at version 5 and whose README still described the pre-
0004tree after a fullinit. Correcting the template alone would have fixed new installs and left every existing project stale forever.So
0006rewrites it. Spending a schema version on documentation is deliberate: the file is generated rather than authored,0001already overwrites it unconditionally, so no user content is at risk. The alternative is a wrong description of the project's own directory that never corrects itself. Confirmed working by migrating this repo 5 → 6.The layout section is now derived, not described
Built from
ENGINES,ENGINE_LAYOUTS,RULES_DIRECTORYandRULE_TESTS_DIRECTORY, so the words cannot disagree with the directories:Same move that fixed the seven stale comments. A future migration that relocates rules updates this text by changing constants it already has to change.
Two pieces of the same drift, found on the way
A passing test was holding the stale text in place.
bootstrap.test.tsasserted the freshly written README containsrule-tests— the directory the same migration deletes. That is why running the suite never found this. It now asserts the layout the migration leaves behind, per engine, and explicitly thatrule-testsis absent.Ten tests hardcoded the schema version, and the version matrix listed prior versions literally as
[0, 1, 2, 3, 4], so each new migration silently stopped covering the version it had just made prior. Both now derive from an exportedLATEST_SCHEMA_VERSION, itself computed from the migration map.One test that had to be decoupled
mixed-engine-check.test.tswrote its Vale sample into.taskless/README.md, and0006overwrote it mid-check. Moved to a file no migration manages — the subject is the path exclusion, not that file.Worth flagging that this is N10 in miniature: a migration running implicitly inside
checkrewrote a file out from under the command. That is a separate change and is next.Verification
1087/1087 tests, typecheck, lint, and
pnpm cli checkclean.Refs #227