Skip to content

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

Open
thecodedrift wants to merge 1 commit into
mainfrom
fix/migration-idempotent-on-current-layout
Open

fix(cli): leave an already-partitioned rules tree alone#113
thecodedrift wants to merge 1 commit into
mainfrom
fix/migration-idempotent-on-current-layout

Conversation

@thecodedrift

Copy link
Copy Markdown
Member

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

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
Copilot AI lite review requested due to automatic review settings August 19, 2026 06:17

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +131 to +135
try {
entries = await readdir(root, { withFileTypes: true });
} catch {
return false; // No `rules/` at all — nothing to protect.
}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Migrations relocate a current-layout project when taskless.json is missing, and check then reports success

2 participants