fix(check): refuse a project behind the CLI instead of migrating it - #234
Conversation
|
@claude /review |
|
Claude finished @thecodedrift's task in 6m 39s —— View job Review completeReview mode: incremental — read 0 prior review thread(s) before reviewing.
FindingsPosted as inline comments (all
Could not post inline (GitHub rejects comments on lines outside the diff hunk — this file has no diff of its own, only lost callers): Everything else — the refusal message, the new Branch: |
03b2979 to
2175ed8
Compare
Review feedback on #234, and the first item is a regression this PR introduced. `check` used to reach the newer-than-this-CLI refusal through `ensureTasklessDirectory` → `runMigrations`. Dropping that call to stop the implicit migration dropped the check with it, and `pendingMigration` only ever asked "is the version BEHIND", so a scaffold ahead of this CLI satisfied `>=` and read as "nothing pending". Reproduced: a version-99 project made `check` report "No rules configured" for a layout it could not parse. That is the same silent answer this change exists to remove, reintroduced by the change itself. `requireCurrentSchema` now carries both directions, since it is the only precondition these commands have left. `--allow-version-mismatches` still works, and is tested, because a documented escape hatch that quietly stops working is worse than never having offered it. `verify` and `test` gain something too: the refusal now reaches their `--json` envelope with its code, where it previously escaped to the top-level handler as prose. Also from review: - Dropped a comment in `schemas/check.ts` describing the `migrated` field that this PR removed. It said `check` migrates before it can dispatch, which is now the opposite of true. The sibling file removed field and comment together; this one kept the comment. - Both refusal sites use the existing `writeJsonError` rather than hand-rolling it. The reviewer's point lands: the two copies had already drifted to different dead fallback codes, which is what duplication does before it does anything worse.
3b03d04 to
c1910b7
Compare
Review feedback on #234, and a second regression from this PR. Replacing the migrate-then-report call dropped its `onNotice` handler, so `ensureTasklessDirectory` fell back to writing every notice with `console.error`. On the one path that still writes — scaffolding a project with no `.taskless/` at all — that runs every migration from 0 and puts the file-by-file summary on stderr unconditionally, handing a machine consumer prose it cannot parse. Reproduced on a bare directory: eight lines of migration summary under `--json`, none of it parseable. Suppressed again, and tested both ways, because the point is not silence: without `--json` that summary is the only thing telling someone their working tree just changed.
All four findings taken. Two of them were regressions this PR introduced, and both are the failure mode the PR is about, so they were worth more than their buckets.
The dangling Separately: #230 merged while this was in flight, and its repair fixture writes a current-layout tree with no manifest, which this PR then refuses. Fixed by completing the fixture's scaffold (c1910b7) — the refusal is right, since nothing can tell a current tree with no manifest from a pre- 1140/1140 tests, typecheck, lint, — AI Coding Agent |
Raised by the generator team as N10, after `check` rewrote their working tree and `git add -A` swept the result into six unrelated pull requests. `check`, `verify` and `test` called `ensureTasklessDirectory`, so three commands whose job is to REPORT performed migration `0005`, which moves and deletes tracked files. Nothing on the human path said so, the diff landed in whatever commit came next, and in CI it ran on every checkout. The argument that settled it is theirs and is not about tidiness: a migration cannot be verified. Comparing findings before and after is impossible when asking the question performs the change, so a migration that silently dropped a rule is invisible to the one check that would catch it. So `pendingMigration` reads the version without touching anything, and `requireCurrentSchema` refuses and names `init`. Keyed on the `.taskless/` DIRECTORY rather than on the manifest file: a directory with no manifest reads as version 0, which is behind, and waving it through would report "no rules configured" for a project full of them — the silent answer this change exists to stop giving. A project with no `.taskless/` at all has nothing to migrate and is unaffected. `SCAFFOLD_MIGRATION_REQUIRED` is a new code rather than a reuse of `SCAFFOLD_VERSION_MISMATCH`. That one is a scaffold NEWER than the CLI, which asks the caller to upgrade the CLI; this asks them to migrate the project, and an agent has to tell them apart. Adding a code is not a breaking change to the contract. The `migrated` field moves to `init --json`, which is new. `init` is now the only command that migrates, so it is the only one that can report one, and a CI script still needs to know the tree was rewritten and what moved. Removed from the other three envelopes, where it can no longer occur; it was always optional and conditional. Two things fell out of the tests. Nine suites were leaning on the implicit migration without saying so — their fixtures are written in the pre-0004 layout — which is the same invisibility, showing up in our own suite. They migrate explicitly now, in setup, where it can be seen. `migrated-envelope.test.ts` was rewritten rather than repaired: its subject moved to `init`, and the inverse property (these commands leave the tree exactly as they found it) is now asserted directly. Also fixes a smaller loss on the way: `check`'s catch flattened every failure to SCAN_FAILED, discarding the code a `CLIError` already carried. "Migrate your project" and "the scan blew up" were arriving as the same answer.
Review feedback on #234, and the first item is a regression this PR introduced. `check` used to reach the newer-than-this-CLI refusal through `ensureTasklessDirectory` → `runMigrations`. Dropping that call to stop the implicit migration dropped the check with it, and `pendingMigration` only ever asked "is the version BEHIND", so a scaffold ahead of this CLI satisfied `>=` and read as "nothing pending". Reproduced: a version-99 project made `check` report "No rules configured" for a layout it could not parse. That is the same silent answer this change exists to remove, reintroduced by the change itself. `requireCurrentSchema` now carries both directions, since it is the only precondition these commands have left. `--allow-version-mismatches` still works, and is tested, because a documented escape hatch that quietly stops working is worse than never having offered it. `verify` and `test` gain something too: the refusal now reaches their `--json` envelope with its code, where it previously escaped to the top-level handler as prose. Also from review: - Dropped a comment in `schemas/check.ts` describing the `migrated` field that this PR removed. It said `check` migrates before it can dispatch, which is now the opposite of true. The sibling file removed field and comment together; this one kept the comment. - Both refusal sites use the existing `writeJsonError` rather than hand-rolling it. The reviewer's point lands: the two copies had already drifted to different dead fallback codes, which is what duplication does before it does anything worse.
#230 merged while this was in flight, and its integration fixture writes a current-layout tree with no `taskless.json`. A tree without a manifest reads as version 0, so `check` refuses it, and every repair notice the test asserts on went missing. The refusal is right and stays. Nothing can tell a current tree with no manifest from a pre-`0004` project by looking, and the safe answer to "I cannot confirm this is current" is to say so rather than to read it and report whatever falls out — which in that fixture's case was an empty envelope. So the fixture completes its scaffold, the way every other suite here now does.
Review feedback on #234, and a second regression from this PR. Replacing the migrate-then-report call dropped its `onNotice` handler, so `ensureTasklessDirectory` fell back to writing every notice with `console.error`. On the one path that still writes — scaffolding a project with no `.taskless/` at all — that runs every migration from 0 and puts the file-by-file summary on stderr unconditionally, handing a machine consumer prose it cannot parse. Reproduced on a bare directory: eight lines of migration summary under `--json`, none of it parseable. Suppressed again, and tested both ways, because the point is not silence: without `--json` that summary is the only thing telling someone their working tree just changed.
2f3eb66 to
55a952c
Compare
Review feedback on #234, and the first item is a regression this PR introduced. `check` used to reach the newer-than-this-CLI refusal through `ensureTasklessDirectory` → `runMigrations`. Dropping that call to stop the implicit migration dropped the check with it, and `pendingMigration` only ever asked "is the version BEHIND", so a scaffold ahead of this CLI satisfied `>=` and read as "nothing pending". Reproduced: a version-99 project made `check` report "No rules configured" for a layout it could not parse. That is the same silent answer this change exists to remove, reintroduced by the change itself. `requireCurrentSchema` now carries both directions, since it is the only precondition these commands have left. `--allow-version-mismatches` still works, and is tested, because a documented escape hatch that quietly stops working is worse than never having offered it. `verify` and `test` gain something too: the refusal now reaches their `--json` envelope with its code, where it previously escaped to the top-level handler as prose. Also from review: - Dropped a comment in `schemas/check.ts` describing the `migrated` field that this PR removed. It said `check` migrates before it can dispatch, which is now the opposite of true. The sibling file removed field and comment together; this one kept the comment. - Both refusal sites use the existing `writeJsonError` rather than hand-rolling it. The reviewer's point lands: the two copies had already drifted to different dead fallback codes, which is what duplication does before it does anything worse.
Stack (root → tip):
Raised by the generator team as N10, after
checkrewrote their working tree andgit add -Aswept the result into six unrelated pull requests.Stacked on #232, which adds
LATEST_SCHEMA_VERSIONthat this builds on.A reporting command was rewriting the repository
check,verifyandtestall calledensureTasklessDirectory, which runs migrations. Migration0005moves and deletes tracked files. So three commands whose entire job is to report changed the thing they were measuring — with nothing on the human path to say so, landing in whatever commit came next, and in CI running on every checkout.The argument that settled it is theirs, and it is not about tidiness: a migration cannot be verified. Comparing findings before and after is impossible when asking the question performs the change. If
0004had silently dropped a rule, the A/B that would catch it is unavailable by construction.What changes
pendingMigrationreads the recorded version without touching anything.requireCurrentSchemarefuses and namestaskless init.Keyed on the
.taskless/directory, not the manifest file. A directory with no manifest reads as version 0, which is behind — and waving it through would report "no rules configured" for a project full of them, since its tree is the pre-0004layout a current CLI finds nothing in. That is the silent answer this change exists to stop giving. A project with no.taskless/at all has nothing to migrate and is unaffected.SCAFFOLD_MIGRATION_REQUIREDis a new code, not a reuse ofSCAFFOLD_VERSION_MISMATCH. That one is a scaffold newer than the CLI and asks the caller to upgrade the CLI; this asks them to migrate the project. An agent has to tell them apart, and adding a code is not a breaking change to the contract.The
migratedfield followed the behaviourinit --jsonis new and carries it.initis now the only command that migrates, so it is the only one that can report one, and a CI script still needs to know the tree was rewritten and what moved.Removed from the
check/verify/testenvelopes, where it can no longer occur. Treated as non-breaking: it was always optional and conditional, so nothing that read it correctly breaks.What the tests said about us
Nine suites were leaning on the implicit migration without saying so. Their fixtures are written in the pre-
0004layout and were silently modernised mid-command. That is the same invisibility the change is about, showing up in our own suite — 64 tests failed on the first run. They now migrate explicitly in setup, through one shared helper, where it can be seen.migrated-envelope.test.tswas rewritten rather than repaired: its subject moved toinit, and the inverse property is now asserted directly — these commands leave the manifest at 3, the rule where it was, and create nothing where the migration would have put it.no-implicit-migration.test.tscovers the refusal itself: the tree is untouched, the message names the fix, the--jsonenvelope carries the code, a current project still runs, a project with no scaffold is unaffected, andinitstill migrates and reports what moved.One smaller loss fixed on the way
check's catch flattened every failure toSCAN_FAILED, discarding the code aCLIErroralready carried. "Migrate your project" and "the scan blew up" were arriving as the same answer.The trade
Every user meets a wall once after a CLI upgrade, where today they meet nothing. That is the visible version of the same event, against a silent rewrite of tracked files that is currently invisible until it shows up in someone's
git status. Same trade as #231, and the one the service makes for a client below its version floor.Verification
1101/1101 tests, typecheck, lint, and
pnpm cli checkclean.Built on top of #232
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