fix(cli): leave an already-partitioned rules tree alone - #113
Open
thecodedrift wants to merge 1 commit into
Open
fix(cli): leave an already-partitioned rules tree alone#113thecodedrift wants to merge 1 commit into
thecodedrift wants to merge 1 commit into
Conversation
A `.taskless/` with no `taskless.json` reads as version 0, so every migration runs — including 0004's `rules/` -> `sg/rules/` move, which predates the layout such a tree is usually already in. Applied there it buried every rule at `sg/rules/sg/<id>/`, 0005 scaffolded empty engine directories over the gap, and `check` exited 0 on a clean report for a project it had stopped scanning. 0004 now recognizes an engine-partitioned `rules/` and skips that one move. Recognition is strict — every entry a directory named for an engine, at least one present — so a genuinely pre-0004 tree of flat `rules/<id>.yml` files still migrates. Fixes #109 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Jwc9FFroR3mTZ4hLiSkkX3
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes a migration-idempotency bug where a project already using the current .taskless/rules/<engine>/... layout but missing taskless.json was treated as version 0 and re-migrated, relocating rules into a non-scanned path and causing check to falsely report success.
Changes:
- Add a guard to migration
0004to detect an already engine-partitioned.taskless/rules/tree and skip the ambiguousrules/ → sg/rules/move. - Add regression tests covering “current layout + missing taskless.json” to ensure
checkstill finds rules andverifyreports the expected engine/rule IDs. - Add a changeset documenting the behavior change as a patch release.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| packages/cli/src/filesystem/migrations/0004-vale-engine.ts | Adds an engine-partition detection guard to keep 0004 idempotent on current-layout trees missing a manifest. |
| packages/cli/test/migrate-round-trip.test.ts | Adds regression coverage for the missing-manifest/current-layout case; minor formatting tweaks to existing assertions/fixtures. |
| .changeset/idempotent-rules-partition.md | Documents the migration fix as a patch changeset. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+131
to
+135
| try { | ||
| entries = await readdir(root, { withFileTypes: true }); | ||
| } catch { | ||
| return false; // No `rules/` at all — nothing to protect. | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What a user would have seen
A project already in the current rule-directory layout, but missing
taskless.json, reads as schema version 0 —readRawManifestreturns{version: 0}onENOENT, so every migration runs. Migration0004then applies itsrules/→sg/rules/move on top of the0005layout, burying every rule at.taskless/sg/rules/sg/<id>/<id>.yml.0005afterwards scaffolds fresh empty engine directories over the hole.Nothing errors.
checkscans a tree with no rules in it and prints{"success":true,"results":[]}with exit 0. The dangerous half is that this is indistinguishable from a project that passes: a manifest that was never committed, or was deleted, silently stops being checked.Why this is a bug, not a design choice
openspec/specs/cli-taskless-bootstrap/spec.mdalready requires both halves:If both held, running every migration against an already-current tree would be a no-op.
0004violates the second. This change makes the code conform to requirements that already exist, which is why no spec file changed.The guard
rulesArePartitionedByEngine()inpackages/cli/src/filesystem/migrations/0004-vale-engine.tsreads.taskless/rules/and returns true only when there is at least one entry and every entry is a directory named for an engine. It keys on theENGINESconstant — the same import0005already takes fromsrc/rules/engines.ts— so a fourth engine is recognized without editing this migration, which is the failure mode worth designing against: a hardcoded list would silently re-open this bug the day an engine is added.Recognition is strict on purpose. The two shapes are unambiguous — pre-
0004holds rule files (rules/<id>.yml), the current layout holds only engine directories — so a mixed or partial tree is not evidence of the new layout and still migrates. That keeps a genuinely old project moving forward at the cost of doing nothing clever with a tree nobody produces.Only the
rulesmove is ambiguousrulesis the one path inMOVESthat means two different things: it is simultaneously the pre-0004flat location and the root of the layout0005establishes. That is the same collision0005's ownassertRootIsFreecomment already describes from the other side.What I checked, and concluded needed no change:
rule-tests/0005puts tests atrules/<engine>/<id>/.tests/. Absent on a current tree, so the move is already a no-op.sgconfig.yml0005gitignores a dot-prefixed.sgconfig.ymland removes the committedsg/sgconfig.yml. The undotted name does not exist on a current tree.runtime-rules/,runtime-rule-tests/0004names only; nothing after0004writes them.0005itself is genuinely idempotent against a current tree and was left alone:sg/rulesand friends are either absent or the empty.gitkeepscaffolds0004writes, somoveEngineRules/moveEngineTestsmove nothing; the scaffoldedvale/.vale.inicarries no section, sosplitValeConfigfinds no blocks and yields no orphans;pruneEmptyplusscaffoldEngineDirectoriesconverge on the same shape. Only the one guard was added — no speculative rewriting of migrations that were not affected.Regression test
packages/cli/test/migrate-round-trip.test.ts, in a newdescribe("a current-layout project with no taskless.json"). Seedsrules/sg/no-eval/no-eval.ymlplus the engine.gitkeeps, notaskless.json, and anapp.tscontainingeval(raw), then runs the real built CLI. It assertscheck --jsonreportsno-eval:app.tsand exits 1, and thatverify --jsonstill lists exactlysg/no-eval.It sits deliberately outside the
withValegate — ast-grep alone is enough to see whether the rules survived, and this case must never regress silently on a machine without Vale.I verified it fails against unpatched source, not only that it passes: stashing the
0004change and rebuilding turns both cases red, and restoring it turns them green. A regression test that was never observed failing is only a test that the current code does what it does.Verification
pnpm lintpnpm typecheckpnpm testpnpm openspec validate --all --strictNothing was failing on
mainbeforehand.Residual, out of scope
There is still no "I scanned nothing" signal anywhere in the check pipeline. This guard closes the one path we know about, but any future tree that strands rules somewhere the scanner does not look would again report a clean pass with exit 0. That is the third option the issue floats, and it is a broader change than this fix — not addressed here.
Fixes #109