feat(cli): add migration 0004 for the engine-partitioned .taskless layout - #79
Conversation
There was a problem hiding this comment.
Pull request overview
Adds schema migration 0004 to relocate .taskless/ into an engine-partitioned directory layout (sg/, runtime/, scaffolded vale/), bumps the scaffold schema to v4, and introduces explicit version-mismatch gating (with an override flag). This is the stack root PR that carries the OpenSpec change docs for the broader multi-PR effort.
Changes:
- Add migration
0004to mechanically move legacy rule trees under.taskless/sg/and.taskless/runtime/, scaffold.taskless/vale/, and ensure empty engine dirs are tracked via.gitkeep. - Update migration runner to register schema v4 and throw a
SCAFFOLD_VERSION_MISMATCHerror when the on-disk schema is newer than the CLI (unless--allow-version-mismatchesis set). - Update/extend tests for schema v4 + new layout, and add the OpenSpec change (proposal/design/specs/tasks).
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| packages/cli/test/onboard.test.ts | Updates expected scaffold schema version to 4. |
| packages/cli/test/migrate-install.test.ts | Updates expected schema version to 4 and expands the version matrix. |
| packages/cli/test/migrate-engine-layout.test.ts | New test suite validating 0004’s byte-preserving moves, gitignore anchoring, scaffolding, idempotency, and version gating override. |
| packages/cli/test/init-no-interactive.test.ts | Updates expected scaffold schema version to 4. |
| packages/cli/test/bootstrap.test.ts | Updates directory assertions to match sg/ engine layout. |
| packages/cli/src/types/errors.ts | Adds SCAFFOLD_VERSION_MISMATCH error code. |
| packages/cli/src/index.ts | Adds global --allow-version-mismatches flag definition. |
| packages/cli/src/filesystem/migrations/0004-vale-engine.ts | New migration implementing the engine-partitioned .taskless/ layout and gitignore anchoring. |
| packages/cli/src/filesystem/migrations/0001-init.ts | Switches gitignore entry to anchored /sgconfig.yml and documents the rationale. |
| packages/cli/src/filesystem/migrate.ts | Registers migration v4 and enforces “scaffold newer than CLI” gating with override support. |
| packages/cli/src/filesystem/directory.ts | Threads allowVersionMismatches option through to runMigrations. |
| openspec/changes/partition-rules-by-engine/tasks.md | New task breakdown for the multi-PR change. |
| openspec/changes/partition-rules-by-engine/specs/cli-runtime-rule-execution/spec.md | New/updated spec describing runtime rule location/semantics in the partitioned layout. |
| openspec/changes/partition-rules-by-engine/specs/cli-rule-format/spec.md | New spec for engine-partitioned on-disk rule format and migration requirements. |
| openspec/changes/partition-rules-by-engine/specs/cli-check/spec.md | New spec for check reading committed sg/sgconfig.yml and engine-based dispatch. |
| openspec/changes/partition-rules-by-engine/proposal.md | New proposal capturing motivation, delivery shape (merge-down stack), and impact. |
| openspec/changes/partition-rules-by-engine/design.md | New design doc detailing layout decisions, risks, and migration plan. |
| openspec/changes/partition-rules-by-engine/.openspec.yaml | New OpenSpec metadata for the change directory. |
`movePreservingContent` documented a bail-out for a file at the *destination* but stat'd the source, so a directory source with a file destination recursed and then failed part-way through the migration. Found in review by Copilot on #79. The guard now requires both sides to be directories, and the migration checks every engine directory up front and refuses with SCAFFOLD_CONFLICT naming the path. Bailing out of a single move was not enough on its own — scaffolding still ran `mkdir` over the same file and died with a bare EEXIST, after earlier moves had already landed. Validating first keeps `.taskless/` in a state the user can still reason about.
The one finding in this review was real and is fixed in 959e636 (replied on the thread). The pre-flight conflict check it led to also covers the scaffolding step, which failed the same way for the same reason. One note for anyone reading CI here: — AI Coding Agent |
|
@claude /review |
|
Claude finished @thecodedrift's task in 5m 30s —— View job Review complete
Overall assessmentThis is a well-scoped migration with good invariants: moves are content-preserving (verified by hash comparisons in tests), idempotent, and the version-mismatch gate has clear semantics with test coverage for both the throw and override paths. The One concrete issue, posted inline:
Other notes (not blocking):
Per this repo's Branch: |
`movePreservingContent` documented a bail-out for a file at the *destination* but stat'd the source, so a directory source with a file destination recursed and then failed part-way through the migration. Found in review by Copilot on #79. The guard now requires both sides to be directories, and the migration checks every engine directory up front and refuses with SCAFFOLD_CONFLICT naming the path. Bailing out of a single move was not enough on its own — scaffolding still ran `mkdir` over the same file and died with a bare EEXIST, after earlier moves had already landed. Validating first keeps `.taskless/` in a state the user can still reason about.
c6b03d0 to
c3c1c13
Compare
The guard only checked the leaf scaffold paths, so a file at an engine root (`.taskless/runtime`) slipped past it: `stat` throws ENOTDIR rather than ENOENT when an ancestor is a file, and `pathExists` treated any error as "nothing there." The migration then ran and died with a bare EEXIST part-way through — exactly the failure the pre-flight exists to prevent. It now checks every ancestor as well as the leaves, shallow first, so the root is named and the unreachable paths below it are not reported twice. Found in review by Claude on #79; the added test reproduces the original EEXIST.
The concrete issue was real and is fixed in this branch (replied on the thread). — AI Coding Agent |
No actionable findings in this pass beyond the one already addressed. Since your earlier review of this PR, the branch also picked up the fix for the reader paths (migration — AI Coding Agent |
Split out of add-vale-rule-engine, which spanned 44 tasks across ten groups — more than one PR should carry, and the archive gate requires a change to land complete in a single PR. This half is the part that carries the risk: the relayout, migration 0004, directory dispatch, ingest defaulting, legacy-path tolerance, and running ast-grep over the committed config. It adds no engine and changes nothing a user observes, so a difference in check output after it lands has exactly one candidate explanation. Vale itself and the engine-selection topic stay in add-vale-rule-engine, which needs this layout to exist first. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CDv57zHq7abms3RReSQw6q
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CDv57zHq7abms3RReSQw6q
Move `.taskless/rules/`, `rule-tests/`, and `sgconfig.yml` under `sg/`, and the runtime tier to `runtime/rules/` and `runtime/rule-tests/`. Every move is content-preserving: runtime capture bytes determine their server-side reconciliation hashes, and `sgconfig.yml`'s `ruleDirs: [rules]` is relative to the config file, so nothing needs rewriting. Scaffold an inert `vale/` engine directory with its native `.vale.ini`, and `.gitkeep` every otherwise-empty scaffolded directory. `runMigrations` now throws when `taskless.json`'s version exceeds the highest migration this CLI knows, instructing the user to upgrade, unless the global `--allow-version-mismatches` flag is passed — previously it returned silently and let an older CLI half-read a newer scaffold. This is task group 1 of `partition-rules-by-engine`. The readers (`check`, `verify`, runtime discovery) are updated in groups 2-4; until then 20 tests in check/verify/runtime-check fail because the rules moved out from under them. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Group 1 alone leaves 20 tests failing, so no unit of this change is independently shippable: the stack merges down and reaches main as one commit. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
An unanchored gitignore pattern matches at any depth, so the `sgconfig.yml` line migration 0001 wrote also matches `.taskless/sg/sgconfig.yml` — the committed config this layout makes the source of truth would never be tracked. 0001 now writes `/sgconfig.yml`, and 0004 rewrites the unanchored line in existing checkouts, collapsing it with the anchored form when both are present. Belongs to task group 1; kept as its own commit since group 1 was already reviewed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`movePreservingContent` documented a bail-out for a file at the *destination* but stat'd the source, so a directory source with a file destination recursed and then failed part-way through the migration. Found in review by Copilot on #79. The guard now requires both sides to be directories, and the migration checks every engine directory up front and refuses with SCAFFOLD_CONFLICT naming the path. Bailing out of a single move was not enough on its own — scaffolding still ran `mkdir` over the same file and died with a bare EEXIST, after earlier moves had already landed. Validating first keeps `.taskless/` in a state the user can still reason about.
Lives at the base of the stack: Require Changeset runs only on PRs based on `main` and reads the PR's own diff, so on a merge-down stack the bottom PR is where it has to be. Every branch above inherits it, and it describes the whole change since that is what reaches `main` in one merge.
Migration `0004` relocated the trees but left every reader pointing at the flat pre-migration paths, so this PR on its own found no rules: an empty scan reports success, so the symptom was "no findings" rather than an error, across 20 tests. Repoints the scan config, rule read/write/delete, `verify`, the `detect` probe, and runtime discovery at `sg/` and `runtime/`, via one small `layout` module so the paths are stated once. `check` also has to migrate BEFORE it discovers. It read the rules directory first and only migrated later inside `generateSgConfig`, so a project still on the flat layout saw an empty `sg/rules/`, printed "No rules configured", and returned before the migration that would have populated it ever ran. This was invisible before `0004` because the path did not move. Only an existing `.taskless/` is migrated, so `check` in a project without one still says so rather than scaffolding one as a side effect. The suite is green on this branch alone: 396 passing.
The guard only checked the leaf scaffold paths, so a file at an engine root (`.taskless/runtime`) slipped past it: `stat` throws ENOTDIR rather than ENOENT when an ancestor is a file, and `pathExists` treated any error as "nothing there." The migration then ran and died with a bare EEXIST part-way through — exactly the failure the pre-flight exists to prevent. It now checks every ancestor as well as the leaves, shallow first, so the root is named and the unreachable paths below it are not reported twice. Found in review by Claude on #79; the added test reproduces the original EEXIST.
1e17924 to
b4bcabe
Compare
`movePreservingContent` documented a bail-out for a file at the *destination* but stat'd the source, so a directory source with a file destination recursed and then failed part-way through the migration. Found in review by Copilot on #79. The guard now requires both sides to be directories, and the migration checks every engine directory up front and refuses with SCAFFOLD_CONFLICT naming the path. Bailing out of a single move was not enough on its own — scaffolding still ran `mkdir` over the same file and died with a bare EEXIST, after earlier moves had already landed. Validating first keeps `.taskless/` in a state the user can still reason about.
Stack (root → tip):
Migration
0004moves.taskless/to an engine-partitioned layout: ast-grep rules go tosg/rules/andsg/rule-tests/, the runtime tree toruntime/rules/andruntime/rule-tests/, andvale/is scaffolded but inert until the Vale engine lands. Files move without their bytes being touched, so runtime rule signatures survive the migration.Also anchors the
sgconfig.ymlgitignore pattern to.taskless/. The pattern was unanchored, so it would match a same-named file anywhere in the tree.This PR carries the OpenSpec change docs for the whole effort, including the spec deltas the later PRs implement.
This is the bottom of a merge-down stack — the units are only correct together, since this migration relocates rules that nothing yet knows how to find. Nothing here reaches
mainon its own: the PRs above merge down into this branch, and the accumulated result is what merges once. The change directory stays unarchived here by design; the archive gate skips non-tip PRs and the tip archives it.Refs OSS-24