fix(cli): leave an already-partitioned rules tree alone

[thecodedrift]

What a user would have seen

A project already in the current rule-directory layout, but missing taskless.json, reads as schema version 0 — readRawManifest returns {version: 0} on ENOENT, so every migration runs. Migration 0004 then applies its rules/ → sg/rules/ move on top of the 0005 layout, burying every rule at .taskless/sg/rules/sg/<id>/<id>.yml. 0005 afterwards scaffolds fresh empty engine directories over the hole.

Nothing errors. check scans 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.md already requires both halves:

• Non-numeric version is treated as 0 — a version-less tree runs all migrations.
• Each migration is idempotent — a migration run against a directory where its changes already exist completes without errors and without duplicating files or content.

If both held, running every migration against an already-current tree would be a no-op. 0004 violates the second. This change makes the code conform to requirements that already exist, which is why no spec file changed.

The guard

rulesArePartitionedByEngine() in packages/cli/src/filesystem/migrations/0004-vale-engine.ts reads .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 the ENGINES constant — the same import 0005 already takes from src/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-0004 holds 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 rules move is ambiguous

rules is the one path in MOVES that means two different things: it is simultaneously the pre-0004 flat location and the root of the layout 0005 establishes. That is the same collision 0005's own assertRootIsFree comment already describes from the other side.

What I checked, and concluded needed no change:

Move source	Conclusion
rule-tests/	No later layout uses this name — 0005 puts tests at rules/<engine>/<id>/.tests/. Absent on a current tree, so the move is already a no-op.
sgconfig.yml	0005 gitignores a dot-prefixed .sgconfig.yml and removes the committed sg/sgconfig.yml. The undotted name does not exist on a current tree.
runtime-rules/, runtime-rule-tests/	Pre-0004 names only; nothing after 0004 writes them.

0005 itself is genuinely idempotent against a current tree and was left alone: sg/rules and friends are either absent or the empty .gitkeep scaffolds 0004 writes, so moveEngineRules/moveEngineTests move nothing; the scaffolded vale/.vale.ini carries no section, so splitValeConfig finds no blocks and yields no orphans; pruneEmpty plus scaffoldEngineDirectories converge 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 new describe("a current-layout project with no taskless.json"). Seeds rules/sg/no-eval/no-eval.yml plus the engine .gitkeeps, no taskless.json, and an app.ts containing eval(raw), then runs the real built CLI. It asserts check --json reports no-eval:app.ts and exits 1, and that verify --json still lists exactly sg/no-eval.

It sits deliberately outside the withVale gate — 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 0004 change 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

Command	Result
pnpm lint	clean
pnpm typecheck	1 successful, 0 errors
pnpm test	50 files, 604 tests passed, 0 failed
pnpm openspec validate --all --strict	23 passed, 0 failed

Nothing was failing on main beforehand.

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

[Copilot]

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 0004 to detect an already engine-partitioned .taskless/rules/ tree and skip the ambiguous rules/ → sg/rules/ move.
• Add regression tests covering “current layout + missing taskless.json” to ensure check still finds rules and verify reports 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.

[thecodedrift]

@claude /review

[claude]

Claude finished @thecodedrift's task in 4m 47s —— View job

---

Claude finished @thecodedrift's task

• Read diff and full context of 0004-vale-engine.ts, 0005-rule-directories.ts, rules/engines.ts, migrate.ts
• Correctness pass on the new rulesArePartitionedByEngine guard
• Cross-file check against 0005's assertRootIsFree and existing migration tests
• Reuse/simplification/altitude pass
• CLAUDE.md conventions check
• Test adequacy check
• Post inline comments and final summary

Overall assessment

The core fix is sound and well-reasoned: guarding 0004's rules/ → sg/rules/ move behind a check that the tree is already engine-partitioned is the right shape for the bug described, the regression test genuinely reproduces the failure (verified failing against unpatched source per the PR body), and the analysis of why the other four MOVES entries don't need the same guard checks out against the code (rule-tests/, sgconfig.yml, runtime-rules/, runtime-rule-tests/ are indeed pre-0004-only names). I traced 0001 → 0004 → 0005 end-to-end and confirmed the described failure mode (sg/rules/sg/<id>/<id>.yml) is exactly what happens without the guard.

One correctness concern worth resolving before merge (left inline on 0004-vale-engine.ts line ~129): rulesArePartitionedByEngine's entries.every(...) check is strict in a way that has a real false-negative — a single stray non-engine entry under an already-partitioned .taskless/rules/ (most plausibly a .DS_Store, which macOS creates automatically on Finder browsing regardless of .gitignore) flips the guard off and reproduces the exact rules-loss bug this PR fixes. The PR body acknowledges accepting risk on "mixed or partial" trees, but .DS_Store isn't the "tree nobody produces" case that reasoning describes — it's routine on macOS dev machines. Worth either filtering known noise files or explicitly confirming this tradeoff.

Secondary, lower-severity findings (also inline):

• The readdir catch swallows all errors, not just "missing path" — echoing the existing bot review comment, with an added detail: rename() doesn't need read permission on the source directory the way readdir() does, so an unreadable-but-renamable rules/ could be silently relocated rather than failing loudly.
• Minor reuse: the engine-name check duplicates the exported isKnownEngine(), and the directory-listing pattern duplicates subdirectories() (rules/engines.ts) / entriesOf() (0005-rule-directories.ts) — now three near-identical helpers.
• Altitude: this guard and 0005's assertRootIsFree are two independently-authored heuristics detecting the same rules/ layout collision from opposite directions, with no shared primitive between them.
• Test gap: the new regression tests only cover a clean current-layout tree, not the stray-entry case above.

Conventions: no clear violations of the repo's CLAUDE.md / STYLEGUIDE-CODE.md found — imports are correctly grouped/direct, no barrel exports, and the new tests consume the CLI's structured JSON output rather than parsing generated artifacts.

No security, performance, or clarity issues beyond what's noted above. The changeset accurately describes the fix and its scope.

[thecodedrift]

Re: @claude[bot] — "Claude finished @thecodedrift's task — Overall assessment: The core fix is sound… One correctness concern worth resolving before merge…"
#113 (comment)

All four findings are addressed in 6072031, each answered inline on its thread: the .DS_Store false negative (recognition now keys on "an engine directory present, and no loose *.yml at the root" rather than entries.every(...)), the over-broad readdir catch (narrowed to ENOENT/ENOTDIR), the isKnownEngine() reuse, and the test gap (a stray-entry case that was verified failing against the strict guard). The detectRulesLayout() extraction is declined here, but the two guards now read the same signal from opposite sides.

— AI Coding Agent

[thecodedrift]

@claude /review

[claude]

Claude finished @thecodedrift's task in 3m 28s —— View job

---

Review complete

• Read PR context, diff, and prior review threads (correctness concern, catch scope, reuse, altitude, test gap from the first /review — all marked addressed in 6072031)
• Read changed files (0004-vale-engine.ts, 0005-rule-directories.ts, migrate-round-trip.test.ts, changeset)
• Multi-angle pass: line-by-line diff scan, removed-behavior audit, cross-file interaction trace, reuse/simplification/efficiency, altitude + CLAUDE.md conventions, test adequacy
• Traced the highest-severity finding through both migrations myself to confirm before posting
• Posted inline comments; summary below

Overall assessment

The three false-negatives fixed in 6072031 (.DS_Store-style noise, the over-broad readdir catch, isKnownEngine reuse, the stray-entry test) are solid — I re-verified each against the current source and they hold. But tracing the remaining "genuinely mixed" branch turned up a correctness gap in the same class as the bug this PR fixes, which I posted inline on 0004-vale-engine.ts:128-156:

The guard's own doc comment says a tree with both an engine directory and a stray loose .yml "does migrate" the stray file, implying the rest is left alone — but the move is directory-granular, not file-granular. When skipRulesMove is false, movePreservingContent moves the entire rules/ directory as one unit. For a tree holding both .taskless/rules/sg/no-eval/no-eval.yml (already-correct current layout) and one stray .taskless/rules/some-old-rule.yml, the whole rules/ directory gets renamed wholesale to sg/rules/. Migration 0005's moveEngineRules then only relocates entries matching *.yml at the top level and skips directories via continue, so the buried sg/no-eval/ subtree is never re-inspected, and pruneEmpty won't remove sg/rules since it's still non-empty (the buried subtree keeps it occupied). Net effect: no-eval is permanently stranded at .taskless/sg/rules/sg/no-eval/no-eval.yml, invisible to check/verify — the exact silent-clean-pass failure this PR sets out to fix, just reachable via a stray file coexisting with an already-partitioned tree (plausible from a merge-conflict leftover, since .taskless/rules/ is committed) rather than a missing manifest alone. Neither new test combines an engine subdirectory with a stray .yml at the root, so this branch ships unverified. Full trace and a related (lower-severity) symlink variant are in the inline comment.

Everything else found was minor and non-blocking, most already implicitly acknowledged by the PR's own design notes:

• Diff hygiene: unrelated prettier reformatting (quote-style, line-wrap) mixed into migrate-round-trip.test.ts outside the new test cases — confirmed byte-identical content, purely cosmetic.
• Reuse: the "loose *.yml at rules/ root" signal is now implemented independently in both 0004's guard and 0005's assertRootIsFree, even though the doc comment says they read "the same signal" — nothing enforces that the two definitions stay in sync if this rule ever changes (e.g. to catch .yaml). Also a third private "readdir, treat missing dir as empty" helper alongside entriesOf/subdirectories.
• Simplification: the MOVES loop reconstructs "is this the rules move" from array shape (from.length === 1 && from[0] === "rules") each iteration rather than singling the one relevant entry out directly — flagged inline.
• Altitude: 0004 already imports from ../../rules/engines for isKnownEngine but hardcodes the literal "rules" instead of importing the same module's RULES_DIRECTORY (used by 0005) — harmless today since both are "rules", but it means the "no shared primitive between the two migrations" boundary isn't fully closed. Separately, the guard's correctness is implicitly tied to today's ENGINES list even though the migration is otherwise treated as frozen — worth a comment if that's meant literally.
• Conventions: no CLAUDE.md / STYLEGUIDE-CODE.md violations found — imports are direct and correctly grouped, no barrel exports, no unexported-type issues, and the new tests consume the CLI's own --json output rather than parsing a build artifact.
• A coincidental-name false positive (a directory literally named sg/vale/runtime for unrelated reasons) and the stricter EACCES/I-O-error propagation in the new catch (intentional per the docstring, but untested and stylistically inconsistent with assertNoDirectoryConflicts's blanket catch a few lines away) were also noted but are low-severity/edge-case.

No security issues found. Test coverage is otherwise good for the cases it does cover (verified via the PR body's fail-then-pass check against unpatched source).

[thecodedrift]

Re: @claude[bot] — "Claude finished @thecodedrift's task in 3m 28s — Review complete…"
#113 (comment)

The "genuinely mixed" correctness gap was real and is fixed in f6ca7fa: the guard is now inspectRulesLayout returning legacy/partitioned/mixed, and the mixed branch moves the loose *.yml files one at a time instead of falling through to the directory-granular move that buried sg/no-eval/. The symlinked-engine-directory variant is covered in the same predicate, and the MOVES shape-check simplification was taken. Declined as non-blocking: the shared detectRulesLayout primitive across 0004/0005 (a refactor of frozen migrations, worth its own change) and the RULES_DIRECTORY import (0004 is frozen against a literal layout by design). The prettier reformatting is pre-existing on this branch.

Verified: lint clean, typecheck clean, 606 tests across 50 files pass, openspec validate --all --strict 23/23. The new regression test fails against 6072031 with no-eval missing from the check output entirely.

— AI Coding Agent