diff --git a/.changeset/rule-id-uniqueness.md b/.changeset/rule-id-uniqueness.md new file mode 100644 index 00000000..f4e329cf --- /dev/null +++ b/.changeset/rule-id-uniqueness.md @@ -0,0 +1,15 @@ +--- +"@taskless/cli": patch +--- + +Two rules can no longer share an id across engines. `verify` fails a rule whose id is also a directory name under another engine, and a new scaffold migration (`9`) renames the ones that already exist. + +`.taskless/rules/sg/no-eval/` beside `.taskless/rules/vale/no-eval/` was silent: `check`'s human output prints `error[no-eval]` with no engine, so a collision shows two identical lines, and every id-addressed command had two answers to choose between. + +**Your rule ids may change on upgrade, and `check` output changes with them.** The first `taskless init` after upgrading renames the colliding `sg` and `vale` copies to `-` — `sg/no-eval` becomes `sg/no-eval-sg`, `vale/no-eval` becomes `vale/no-eval-vale`. Where both move, neither keeps the bare id, so nobody has to work out which of their two rules kept the name. If `-` is already taken it uses the next free `--2`, `-3`, … and never overwrites an existing rule. + +**A `runtime` rule is never renamed** and keeps the bare id, so a collision between `runtime` and another engine moves only the other one. Runtime rules are the signed and blessed tier, and this keeps the upgrade clear of that machinery. Nothing is left colliding either way, because one engine can only hold one directory per id. + +Everything the rename touches is inside the rule's own directory: the directory name, the rule file, its `id:` field, an sg rule's `.tests/` fixtures and their `id:` fields, and a Vale rule's `.vale.ini` breadcrumb and both segments of its `.` assignment. Every rename is printed — old path, new path, and each file rewritten — as is any runtime rule that kept its id, so you can see exactly what moved before committing it. Update any CI config, baseline file or suppression comment that names an old id — including `check --rule `, which errors with `RULE_NOT_FOUND` rather than reporting zero findings when the id it names has been renamed out from under it. + +`.taskless/rule-metadata/.yml` is left where it is rather than following either rule, since a symmetric rename gives it no owner. In practice there is nothing there: this CLI has never written a sidecar, because the service does not return the metadata block they are written from. diff --git a/.taskless/taskless.json b/.taskless/taskless.json index b00f7c3b..693fa8d5 100644 --- a/.taskless/taskless.json +++ b/.taskless/taskless.json @@ -1,5 +1,5 @@ { - "version": 8, + "version": 9, "install": { "targets": { ".taskless": { diff --git a/openspec/changes/archive/2026-09-22-rule-id-uniqueness/proposal.md b/openspec/changes/archive/2026-09-22-rule-id-uniqueness/proposal.md new file mode 100644 index 00000000..0193a15b --- /dev/null +++ b/openspec/changes/archive/2026-09-22-rule-id-uniqueness/proposal.md @@ -0,0 +1,47 @@ +## Why + +Nothing keeps `.taskless/rules/sg/no-eval/` and `.taskless/rules/vale/no-eval/` from both existing. `isValidRuleId` is `/^[a-z0-9][a-z0-9-]*$/`, with no engine component and no cross-engine check; the write path resolves one engine and `mkdir`s inside it; the read path asks `listRuleIds` per engine and never diffs the answers. `findRuleEngines` already returns every engine holding an id, and its docblock already says the uniqueness the old code assumed was never true of the ids this CLI accepts. + +The visible cost is that human `check` output cannot tell the two apart: `util/format.ts` prints `severity[ruleId]` with no engine, so a collision shows two identical `error[no-eval]` lines. The JSON envelope is fine — every result carries `source` — so a machine consumer keying on `(source, ruleId)` is correct and one keying on `ruleId` alone silently merges two rules. + +The metadata-sidecar clobber that #387 leads with is LATENT rather than live, and that is what makes an automatic rename safe. `writeRuleMetaFiles` keys `.taskless/rule-metadata/{id}.yml` on the id alone, so two colliding rules would share one file — but nothing writes one. The sidecar comes from the `meta` block of a rule status response, the service does not populate it, and both call sites are documented as dead in practice; `rule meta` reports `RULE_META_UNAVAILABLE` saying so, and `.taskless/rule-metadata/` does not exist in this repository. There is no metadata for a rename to destroy. + +## What Changes + +- **`verify` fails a rule whose id is held by more than one engine**, naming every holding directory and the shared metadata sidecar, and telling the user to rename one. The check is **per rule**, not only project-wide: `verify` runs on a single rule as well as on the tree, and verifying the rule an author just wrote is the moment the collision is cheapest to fix. The project-wide form falls out of running it for each rule. `test` inherits it, because `test` runs `verify` first. +- **The failure is not a `RULE_CONSTRAINTS` entry.** Every constraint is declared for one `engine` and published per engine in the conformance corpus, because a constraint says what this CLI requires of a rule for that engine beyond what the engine itself requires. This requires nothing of the rule: the file is valid, and what is wrong is that a sibling tree holds the same directory name. Giving it an engine would mean inventing an engine-agnostic constraint kind for one entry, or filing three near-identical ones and telling a generator that ast-grep has a house rule about Vale. It is reported in `errors` with no `violations` attribution, the same way every other non-engine finding already is. +- **The wording matches `rules delete`.** `rules delete` refuses the same state with `RULE_ID_AMBIGUOUS` and "Rule … is held by N engines, so there is no single rule to delete: ". The opening clause is shared verbatim and only the consequence differs, so the two surfaces cannot come to describe one condition as two. +- **`writeRuleFile` warns and still writes.** `check`'s repair path calls it, so a refusal would brick repair for both colliding rules — strictly worse than the silence it would replace. The failure belongs in `verify`, which is what the warning points at. +- **Migration `9` renames the colliding `sg` and `vale` copies to `-`** on upgrade. Where both move, neither keeps the bare id, because any precedence rule between them would be arbitrary and would leave a user working out which of their two rules kept the name. +- **A `runtime` copy is never renamed** and keeps the bare id. Runtime rules are the signed and blessed tier, and leaving them untouched keeps the migration clear of that machinery rather than reasoning about it. It costs nothing: within one engine the filesystem already guarantees one directory per id, so moving the other copies resolves the collision either way. Measured, a rename would in fact have been safe — `signRuleFile` hashes the CONTENT of `check.ts` and never its path — so this is a precaution, not a correctness fix. `LATEST_SCHEMA_VERSION` becomes 9 and this repository's own `.taskless/taskless.json` is bumped with it. +- **It renames rather than refuses, because refusing walls `init`.** `check` and `verify` send a stale scaffold to `init` with `SCAFFOLD_MIGRATION_REQUIRED`, and `init` is what runs migrations — so a throwing migration makes the CLI's own instruction the thing that fails, with a multi-file hand edit as the only way out. Renaming resolves it at the one moment the CLI has both the user's attention and full knowledge of the layout. +- **The rename reaches nothing outside the rule's own directory**, which is what makes it safe to do automatically. Measured against this tree: `sg` carries the id in the directory, `.yml`, its `id:` field, and every `.tests/-*-test.yml` plus each fixture's own `id:`; `vale` in the directory, `.yml`, and both the `tskl) rule` breadcrumb and both segments of `.` in `.vale.ini` (both, because `StylesPath` points at `rules/vale`, so the directory is the style and `.yml` the check); `runtime` nowhere, since it is never renamed. Nothing outside names a rule id: `taskless.json` records versions, and the runtime reconcile join is by content signature, so a moved-but-unchanged rule still resolves. +- **It never clobbers, and it reports everything.** A taken `-` takes the first free `--N` from 2, where free means held by no engine, so clearing one collision cannot create another. Every rename is printed — old path, new path, each file rewritten — because a migration that silently renames a user's rules is worse than one that refuses. +- **The metadata sidecar is left in place.** The rename is symmetric, so `rule-metadata/.yml` has no owner to follow and moving it to either side would be a guess. It is left, reported, and orphaned, which costs nothing: the service does not populate the `meta` block a sidecar is written from, so this CLI has never written one and `.taskless/rule-metadata/` does not exist in this repository. `rule meta` says exactly that when asked, and the `status.meta` branch that would write one is documented as dead in practice. + +Deliberately out of scope, per taskless/cli#387: whether the metadata sidecar should gain an engine segment, and whether `check --rule ` should stop selecting both engines. The filter over-selects rather than mis-selects, its findings carry `source`, and once `verify` refuses the collision the case stops arising. + +## Delivery shape + +**Single PR.** The `verify` failure and the migration are one behavior seen from two moments, and shipping either alone is wrong in a way the other fixes: the check without the migration only ever fires for rules written after it, while a project that already collides keeps sharing a sidecar; the migration without the check walls off existing projects for a condition nothing else reports. The diff is small enough to review whole, and the spec, implementation and archive land together. + +## Capabilities + +### New Capabilities + +None. + +### Modified Capabilities + +- `cli-rule-validation`: a new requirement for the per-rule uniqueness refusal; "Rules are validated and tested by path, not by id" restated so its cross-engine scenario says what addressing still guarantees and what `verify` now reports. +- `cli-taskless-bootstrap`: a new requirement for migration 9, the rename it performs per engine, and what it refuses to touch. + +## Impact + +- `packages/cli/src/rules/id-uniqueness.ts` (new): the collision finders and the shared wording. +- `packages/cli/src/rules/inspect.ts`: `verifyOneRule` applies the check around the engine layers; the `sg` branch of `testOneRule` reaches it through the same helper rather than a second `verifySgRule` call. +- `packages/cli/src/rules/files.ts`: `writeRuleFile` warns after the write. +- `packages/cli/src/filesystem/migrations/0009-unique-rule-ids.ts` (new), registered in `migrate.ts`; `LATEST_SCHEMA_VERSION` becomes 9 and `.taskless/taskless.json` is migrated and committed. +- `packages/cli/src/filesystem/manifest.ts` (new): the manifest shape and its two accessors, extracted from `migrate.ts` so reading the manifest no longer loads the migration registry. `install/state.ts`, `rules/reconcile-marker.ts`, `commands/info.ts`, `commands/init.ts`, `commands/onboard.ts` and `test/migrate-install.test.ts` import it directly; nothing re-exports from `migrate.ts`. This is what lets `0009` reach `reconcile-marker` for `pathExists` at all: while the two halves shared a module, that import closed a loop through the runner and left `migrations["9"]` undefined. +- Tests: `packages/cli/test/rule-id-uniqueness.test.ts`. +- `.changeset/rule-id-uniqueness.md`. diff --git a/openspec/changes/archive/2026-09-22-rule-id-uniqueness/specs/cli-rule-validation/spec.md b/openspec/changes/archive/2026-09-22-rule-id-uniqueness/specs/cli-rule-validation/spec.md new file mode 100644 index 00000000..cacf915a --- /dev/null +++ b/openspec/changes/archive/2026-09-22-rule-id-uniqueness/specs/cli-rule-validation/spec.md @@ -0,0 +1,74 @@ +## ADDED Requirements + +### Requirement: Verify refuses a rule id held by more than one engine + +`verify` SHALL fail a rule whose id is also a directory name under another engine, and the failure SHALL name every engine directory holding the id and the `.taskless/rule-metadata/.yml` sidecar they share, and SHALL say to rename one of them. `test` SHALL inherit the refusal, because `test` runs `verify` first. + +The check SHALL be made per rule, not only over the whole project. `verify` is invoked on a single rule as well as on the tree, and verifying the rule an author has just written is the moment the collision is cheapest to fix; a project-wide pass that only diffs the per-engine id lists reports nothing at exactly that moment. The project-wide form follows from running the per-rule check for each rule. + +The condition SHALL be described the way `rules delete` already describes it, which refuses the same state under `RULE_ID_AMBIGUOUS`. One condition described in two vocabularies is how a reader comes to believe it is two conditions. + +The failure SHALL NOT be attributed to a published `RULE_CONSTRAINTS` entry. Every constraint is declared for one engine and published per engine, because a constraint states what this CLI requires of a rule for that engine beyond what the engine itself requires. A collision requires nothing of the rule: the file is valid, and what is wrong is that a sibling tree holds the same directory name. + +The write path SHALL NOT refuse. `check`'s repair path calls `writeRuleFile`, so refusing there would leave both colliding rules unrepairable, which is worse than the silence it would replace. A warning after the write is the write path's whole contribution. + +#### Scenario: A colliding pair fails from either side + +- **WHEN** `no-eval` exists under two engines and `verify` runs against either one +- **THEN** the rule SHALL fail +- **AND** the failure SHALL name both engine directories + +#### Scenario: Verifying one rule catches a collision with another engine + +- **WHEN** `verify` runs against a single rule whose id is also held by another engine +- **THEN** it SHALL report the collision without being run over the whole tree + +#### Scenario: A tree with no collision passes + +- **WHEN** every rule id in the project is held by exactly one engine +- **THEN** `verify` SHALL report no collision for any rule + +#### Scenario: The collision carries no constraint id + +- **WHEN** `verify --json` reports a collision +- **THEN** the message SHALL appear in `errors` +- **AND** no violation SHALL be reported for it + +#### Scenario: Writing a colliding rule warns rather than refusing + +- **WHEN** a rule is written whose id another engine already holds +- **THEN** the rule SHALL be written +- **AND** the caller SHALL receive a warning naming both directories + +## MODIFIED Requirements + +### Requirement: Rules are validated and tested by path, not by id + +The CLI SHALL provide `verify ` and `test `. Both SHALL accept a path to a rule's canonical location or to any directory above it, and SHALL resolve the owning engine from the path's position under `.taskless/rules//` rather than by parsing the file. + +An id does not name one thing. The same id can exist under `sg` and under `vale`, so an id-addressed command has to either guess or report an ambiguity; a path has neither problem. Resolving the engine from position — never from content — is the same rule dispatch follows, so a rule cannot be validated by one engine and executed by another. + +Addressing a rule by path is what removes the ambiguity from the COMMAND. It does not make the project's layout correct: the two rules still share one metadata sidecar, and `verify` reports that as a failure of each rule. The two are separate answers to separate questions, and neither replaces the other. + +#### Scenario: A rule path resolves to its engine + +- **WHEN** `verify .taskless/rules/vale/no-simply` is run +- **THEN** the CLI SHALL validate it as a Vale rule + +#### Scenario: The same id under two engines is not ambiguous + +- **WHEN** `no-simply` exists under both `rules/sg/` and `rules/vale/` +- **THEN** each is addressed by its own path +- **AND** neither command SHALL require the user to disambiguate +- **AND** each rule SHALL still be reported as failing verification, because the id is held by two engines + +#### Scenario: A directory means everything beneath it + +- **WHEN** `verify .taskless/` is run +- **THEN** every rule beneath it SHALL be validated, each against its own engine +- **AND** the command SHALL report per-rule results rather than a single pass or fail + +#### Scenario: A path outside any engine's rules directory is rejected + +- **WHEN** a path resolves to no engine +- **THEN** the CLI SHALL exit non-zero naming the path, rather than guessing an engine diff --git a/openspec/changes/archive/2026-09-22-rule-id-uniqueness/specs/cli-taskless-bootstrap/spec.md b/openspec/changes/archive/2026-09-22-rule-id-uniqueness/specs/cli-taskless-bootstrap/spec.md new file mode 100644 index 00000000..207915ad --- /dev/null +++ b/openspec/changes/archive/2026-09-22-rule-id-uniqueness/specs/cli-taskless-bootstrap/spec.md @@ -0,0 +1,96 @@ +## ADDED Requirements + +### Requirement: Migration 9 renames a rule id held by more than one engine + +Migration `9` SHALL read `.taskless/rules/` and, for every rule id that is a directory name under more than one engine, SHALL rename the `sg` and `vale` copies to `-`. + +A `runtime` copy SHALL NEVER be renamed, and SHALL keep the bare id. Runtime rules are the tier whose artifacts are signed and blessed, and leaving them untouched keeps the migration clear of that machinery rather than reasoning about it. It costs nothing, because within one engine the filesystem already guarantees one directory per id, so moving the other copies resolves the collision either way. + +Where two engines that DO move both hold an id, neither SHALL keep it. Any precedence rule between them would be arbitrary, and a symmetric rename means no user has to work out which of their two rules silently kept the name. + +It SHALL rename rather than refuse. A throwing migration walls `init`, which is the command `SCAFFOLD_MIGRATION_REQUIRED` sends a stale scaffold to, so a refusal leaves the CLI's own instruction failing and a multi-file hand edit as the only way out. + +The rename SHALL carry every reference to the id inside the rule's own directory, and SHALL reach nothing outside it: + +| Engine | What the rename SHALL move | +| --------- | --------------------------------------------------------------------------------------------------------------- | +| `sg` | the directory, `.yml`, its `id:` field, every `.tests/-*-test.yml`, and each fixture's own `id:` field | +| `vale` | the directory, `.yml`, and in `.vale.ini` both the `tskl) rule` breadcrumb and both segments of `.` | +| `runtime` | the directory only | + +Both Vale segments move because `StylesPath` points at `rules/vale`, so the rule directory is the style and `.yml` is the check inside it. + +It SHALL NOT clobber. When `-` is already in use the migration SHALL take the first free `--N` counting from 2, and a name SHALL count as free only when NO engine holds it, so resolving one collision cannot create another. Every name it chooses SHALL satisfy the rule id contract. + +It SHALL NOT move or delete `.taskless/rule-metadata/.yml`. The rename is symmetric, so the sidecar has no owner to follow and moving it to either side would be a guess. + +It SHALL print every rename: the old path, the new path, and each file rewritten inside it. A migration that silently renames a user's rules is worse than one that refuses. It SHALL also name any `runtime` copy that kept its id, so a reader of a three-engine collision is not left wondering why one of the three did not move. + +It SHALL write nothing when there is no collision. A project in that state SHALL be read and left exactly as it is, so a second run touches nothing and the working tree stays clean. A project with no `rules/` tree SHALL be left as it is. + +#### Scenario: Colliding sg and vale copies are renamed symmetrically + +- **WHEN** `no-eval` exists under both `sg` and `vale` and migration 9 runs +- **THEN** `rules/sg/no-eval` SHALL become `rules/sg/no-eval-sg` +- **AND** `rules/vale/no-eval` SHALL become `rules/vale/no-eval-vale` +- **AND** no engine SHALL still hold the bare id + +#### Scenario: A colliding runtime rule keeps its id + +- **WHEN** `no-eval` exists under both `sg` and `runtime` and migration 9 runs +- **THEN** `rules/sg/no-eval` SHALL become `rules/sg/no-eval-sg` +- **AND** `rules/runtime/no-eval` SHALL be left byte for byte as it was +- **AND** no id SHALL be held by more than one engine afterwards + +#### Scenario: Only the sg and vale copies move when all three collide + +- **WHEN** `no-eval` exists under `sg`, `vale` and `runtime` and migration 9 runs +- **THEN** the `sg` and `vale` copies SHALL be renamed +- **AND** `rules/runtime/no-eval` SHALL keep the bare id +- **AND** the report SHALL say that the runtime copy kept its id + +#### Scenario: An sg rule's file, id field and fixtures follow it + +- **WHEN** migration 9 renames a colliding `sg` rule +- **THEN** `.yml` SHALL become `.yml` with its `id:` field rewritten +- **AND** every `.tests/-*-test.yml` SHALL be renamed to the new prefix with its own `id:` field rewritten + +#### Scenario: A Vale rule's style file and both config segments follow it + +- **WHEN** migration 9 renames a colliding `vale` rule +- **THEN** `.yml` SHALL become `.yml` +- **AND** the `.vale.ini` breadcrumb SHALL name the new id +- **AND** the `.` assignment SHALL become `.` + +#### Scenario: A taken target name takes the next free suffix + +- **WHEN** `-` is already held by some engine +- **THEN** the migration SHALL rename to the first free `--N` counting from 2 +- **AND** the existing rule of that name SHALL NOT be modified + +#### Scenario: The metadata sidecar is left in place + +- **WHEN** `.taskless/rule-metadata/.yml` exists for a colliding id and migration 9 runs +- **THEN** the sidecar SHALL be left exactly as it is +- **AND** the report SHALL say it was left behind + +#### Scenario: Every rename is reported + +- **WHEN** migration 9 renames anything +- **THEN** it SHALL print each old path, each new path, and each file it rewrote + +#### Scenario: The renamed project verifies and checks clean + +- **WHEN** migration 9 has renamed a colliding project +- **THEN** `verify` SHALL report no collision +- **AND** each renamed rule SHALL still run and report findings under its new id + +#### Scenario: Migration 9 is idempotent + +- **WHEN** migration 9 runs a second time over a project it has already renamed, or over one with no collision +- **THEN** it SHALL write nothing + +#### Scenario: A project with no rules tree is left alone + +- **WHEN** `.taskless/rules/` does not exist and migration 9 runs +- **THEN** the migration SHALL succeed and write nothing diff --git a/openspec/changes/archive/2026-09-22-rule-id-uniqueness/tasks.md b/openspec/changes/archive/2026-09-22-rule-id-uniqueness/tasks.md new file mode 100644 index 00000000..e1a4f3c8 --- /dev/null +++ b/openspec/changes/archive/2026-09-22-rule-id-uniqueness/tasks.md @@ -0,0 +1,25 @@ +## 1. The check + +- [x] 1.1 Add `rules/id-uniqueness.ts`: `findRuleIdCollision` for one id (over `findRuleEngines`), `findRuleIdCollisions` for the tree (one pass over the per-engine id lists), `metadataSidecarPath`, and `describeRuleIdCollision` carrying `rules delete`'s opening clause verbatim. +- [x] 1.2 `rules/inspect.ts`: split `verifyOneRule` into the engine layers plus a uniqueness wrapper; list the collision first among the errors; attribute no constraint. Reach it from the `sg` branch of `testOneRule` without a second `verifySgRule` call. +- [x] 1.3 `rules/files.ts`: `writeRuleFile` warns after the write when another engine holds the id, and still returns the path. + +## 2. The migration + +- [x] 2.1 Verify the rename is safe: confirm the metadata sidecar is never written by this CLI, and enumerate every reference to a rule id per engine, proving each lives inside the rule's own directory. +- [x] 2.2 Add `filesystem/migrations/0009-unique-rule-ids.ts`: rename every colliding copy to `-`, carrying the rule file, its `id:`, sg fixtures and their `id:`, and the Vale breadcrumb and both `.` segments. A `runtime` copy is exempt and keeps the bare id, named in the report so the omission is not silent. Next free `-N` suffix when the target is taken, sidecar left in place, every rename printed. Register `"9"` in `migrate.ts`. +- [x] 2.3 Break the cycle at its cause: extract the manifest from `migrate.ts` into `filesystem/manifest.ts`, which imports nothing from the runner, and repoint all six manifest importers directly. `0009` then reaches `rules/reconcile-marker` for `pathExists` with no loop. +- [x] 2.4 Prove it end to end on a real v8 scaffold: `check` → `init` → `verify` → `check`, including that both renamed rules still fire under their new ids. +- [x] 2.5 Migrate this repository's own `.taskless/` with `pnpm build && pnpm cli init` and commit the rewritten manifest. + +## 3. Tests + +- [x] 3.1 `test/rule-id-uniqueness.test.ts`: a colliding pair fails `verify` from either side naming both paths; a single rule verified alone catches the collision; a non-colliding tree passes and reports no collisions; the collision carries no `violations`. +- [x] 3.2 The migration renames symmetrically; an sg rule's file, `id:` and fixtures follow; a Vale rule's style file and both config segments follow; a runtime rule is never renamed, for sg+runtime, vale+runtime and all three; a taken target takes the next free suffix without touching the existing rule; the sidecar is left in place; no collision is left behind; two runs write nothing on a clean project and nothing after a rename; a missing rules tree is a no-op. Plus unit cases for `retargetValeConfig`. +- [x] 3.3 `writeRuleFile` still writes through a collision and warns, and does not warn without one. + +## 4. Release + +- [x] 4.1 `.changeset/rule-id-uniqueness.md`, `patch`, saying what a user holding an existing collision must do. +- [x] 4.2 `test/migration-registry.test.ts`: every registered version applies when the graph is entered at a migration module. Validated by reintroducing the cycle and confirming the test fails — two earlier forms of it passed against the same broken tree. +- [x] 4.3 `pnpm typecheck`, `pnpm lint`, `pnpm --filter @taskless/cli test`. diff --git a/openspec/specs/cli-rule-validation/spec.md b/openspec/specs/cli-rule-validation/spec.md index 5cd76ba9..30c55463 100644 --- a/openspec/specs/cli-rule-validation/spec.md +++ b/openspec/specs/cli-rule-validation/spec.md @@ -12,6 +12,8 @@ The CLI SHALL provide `verify ` and `test `. Both SHALL accept a pat An id does not name one thing. The same id can exist under `sg` and under `vale`, so an id-addressed command has to either guess or report an ambiguity; a path has neither problem. Resolving the engine from position — never from content — is the same rule dispatch follows, so a rule cannot be validated by one engine and executed by another. +Addressing a rule by path is what removes the ambiguity from the COMMAND. It does not make the project's layout correct: the two rules still share one metadata sidecar, and `verify` reports that as a failure of each rule. The two are separate answers to separate questions, and neither replaces the other. + #### Scenario: A rule path resolves to its engine - **WHEN** `verify .taskless/rules/vale/no-simply` is run @@ -22,6 +24,7 @@ An id does not name one thing. The same id can exist under `sg` and under `vale` - **WHEN** `no-simply` exists under both `rules/sg/` and `rules/vale/` - **THEN** each is addressed by its own path - **AND** neither command SHALL require the user to disambiguate +- **AND** each rule SHALL still be reported as failing verification, because the id is held by two engines #### Scenario: A directory means everything beneath it @@ -275,3 +278,43 @@ error message is not a breaking change. - **WHEN** `verify --json` rejects a Vale rule whose config assigns a key naming another rule - **THEN** the rule's result SHALL carry a violation with `constraintId` `vale-config-own-key-only` - **AND** the violation's message SHALL also appear in `errors` + +### Requirement: Verify refuses a rule id held by more than one engine + +`verify` SHALL fail a rule whose id is also a directory name under another engine, and the failure SHALL name every engine directory holding the id and the `.taskless/rule-metadata/.yml` sidecar they share, and SHALL say to rename one of them. `test` SHALL inherit the refusal, because `test` runs `verify` first. + +The check SHALL be made per rule, not only over the whole project. `verify` is invoked on a single rule as well as on the tree, and verifying the rule an author has just written is the moment the collision is cheapest to fix; a project-wide pass that only diffs the per-engine id lists reports nothing at exactly that moment. The project-wide form follows from running the per-rule check for each rule. + +The condition SHALL be described the way `rules delete` already describes it, which refuses the same state under `RULE_ID_AMBIGUOUS`. One condition described in two vocabularies is how a reader comes to believe it is two conditions. + +The failure SHALL NOT be attributed to a published `RULE_CONSTRAINTS` entry. Every constraint is declared for one engine and published per engine, because a constraint states what this CLI requires of a rule for that engine beyond what the engine itself requires. A collision requires nothing of the rule: the file is valid, and what is wrong is that a sibling tree holds the same directory name. + +The write path SHALL NOT refuse. `check`'s repair path calls `writeRuleFile`, so refusing there would leave both colliding rules unrepairable, which is worse than the silence it would replace. A warning after the write is the write path's whole contribution. + +#### Scenario: A colliding pair fails from either side + +- **WHEN** `no-eval` exists under two engines and `verify` runs against either one +- **THEN** the rule SHALL fail +- **AND** the failure SHALL name both engine directories + +#### Scenario: Verifying one rule catches a collision with another engine + +- **WHEN** `verify` runs against a single rule whose id is also held by another engine +- **THEN** it SHALL report the collision without being run over the whole tree + +#### Scenario: A tree with no collision passes + +- **WHEN** every rule id in the project is held by exactly one engine +- **THEN** `verify` SHALL report no collision for any rule + +#### Scenario: The collision carries no constraint id + +- **WHEN** `verify --json` reports a collision +- **THEN** the message SHALL appear in `errors` +- **AND** no violation SHALL be reported for it + +#### Scenario: Writing a colliding rule warns rather than refusing + +- **WHEN** a rule is written whose id another engine already holds +- **THEN** the rule SHALL be written +- **AND** the caller SHALL receive a warning naming both directories diff --git a/openspec/specs/cli-taskless-bootstrap/spec.md b/openspec/specs/cli-taskless-bootstrap/spec.md index 1ebf9708..8628a572 100644 --- a/openspec/specs/cli-taskless-bootstrap/spec.md +++ b/openspec/specs/cli-taskless-bootstrap/spec.md @@ -269,3 +269,98 @@ The migration exists because the `create-vale-rule` recipe wrote `BasedOnStyles - **WHEN** migration 8 runs twice over the same scaffold - **THEN** the second run SHALL change nothing + +### Requirement: Migration 9 renames a rule id held by more than one engine + +Migration `9` SHALL read `.taskless/rules/` and, for every rule id that is a directory name under more than one engine, SHALL rename the `sg` and `vale` copies to `-`. + +A `runtime` copy SHALL NEVER be renamed, and SHALL keep the bare id. Runtime rules are the tier whose artifacts are signed and blessed, and leaving them untouched keeps the migration clear of that machinery rather than reasoning about it. It costs nothing, because within one engine the filesystem already guarantees one directory per id, so moving the other copies resolves the collision either way. + +Where two engines that DO move both hold an id, neither SHALL keep it. Any precedence rule between them would be arbitrary, and a symmetric rename means no user has to work out which of their two rules silently kept the name. + +It SHALL rename rather than refuse. A throwing migration walls `init`, which is the command `SCAFFOLD_MIGRATION_REQUIRED` sends a stale scaffold to, so a refusal leaves the CLI's own instruction failing and a multi-file hand edit as the only way out. + +The rename SHALL carry every reference to the id inside the rule's own directory, and SHALL reach nothing outside it: + +| Engine | What the rename SHALL move | +| --------- | --------------------------------------------------------------------------------------------------------------- | +| `sg` | the directory, `.yml`, its `id:` field, every `.tests/-*-test.yml`, and each fixture's own `id:` field | +| `vale` | the directory, `.yml`, and in `.vale.ini` both the `tskl) rule` breadcrumb and both segments of `.` | +| `runtime` | the directory only | + +Both Vale segments move because `StylesPath` points at `rules/vale`, so the rule directory is the style and `.yml` is the check inside it. + +It SHALL NOT clobber. When `-` is already in use the migration SHALL take the first free `--N` counting from 2, and a name SHALL count as free only when NO engine holds it, so resolving one collision cannot create another. Every name it chooses SHALL satisfy the rule id contract. + +It SHALL NOT move or delete `.taskless/rule-metadata/.yml`. The rename is symmetric, so the sidecar has no owner to follow and moving it to either side would be a guess. + +It SHALL print every rename: the old path, the new path, and each file rewritten inside it. A migration that silently renames a user's rules is worse than one that refuses. It SHALL also name any `runtime` copy that kept its id, so a reader of a three-engine collision is not left wondering why one of the three did not move. + +It SHALL write nothing when there is no collision. A project in that state SHALL be read and left exactly as it is, so a second run touches nothing and the working tree stays clean. A project with no `rules/` tree SHALL be left as it is. + +#### Scenario: Colliding sg and vale copies are renamed symmetrically + +- **WHEN** `no-eval` exists under both `sg` and `vale` and migration 9 runs +- **THEN** `rules/sg/no-eval` SHALL become `rules/sg/no-eval-sg` +- **AND** `rules/vale/no-eval` SHALL become `rules/vale/no-eval-vale` +- **AND** no engine SHALL still hold the bare id + +#### Scenario: A colliding runtime rule keeps its id + +- **WHEN** `no-eval` exists under both `sg` and `runtime` and migration 9 runs +- **THEN** `rules/sg/no-eval` SHALL become `rules/sg/no-eval-sg` +- **AND** `rules/runtime/no-eval` SHALL be left byte for byte as it was +- **AND** no id SHALL be held by more than one engine afterwards + +#### Scenario: Only the sg and vale copies move when all three collide + +- **WHEN** `no-eval` exists under `sg`, `vale` and `runtime` and migration 9 runs +- **THEN** the `sg` and `vale` copies SHALL be renamed +- **AND** `rules/runtime/no-eval` SHALL keep the bare id +- **AND** the report SHALL say that the runtime copy kept its id + +#### Scenario: An sg rule's file, id field and fixtures follow it + +- **WHEN** migration 9 renames a colliding `sg` rule +- **THEN** `.yml` SHALL become `.yml` with its `id:` field rewritten +- **AND** every `.tests/-*-test.yml` SHALL be renamed to the new prefix with its own `id:` field rewritten + +#### Scenario: A Vale rule's style file and both config segments follow it + +- **WHEN** migration 9 renames a colliding `vale` rule +- **THEN** `.yml` SHALL become `.yml` +- **AND** the `.vale.ini` breadcrumb SHALL name the new id +- **AND** the `.` assignment SHALL become `.` + +#### Scenario: A taken target name takes the next free suffix + +- **WHEN** `-` is already held by some engine +- **THEN** the migration SHALL rename to the first free `--N` counting from 2 +- **AND** the existing rule of that name SHALL NOT be modified + +#### Scenario: The metadata sidecar is left in place + +- **WHEN** `.taskless/rule-metadata/.yml` exists for a colliding id and migration 9 runs +- **THEN** the sidecar SHALL be left exactly as it is +- **AND** the report SHALL say it was left behind + +#### Scenario: Every rename is reported + +- **WHEN** migration 9 renames anything +- **THEN** it SHALL print each old path, each new path, and each file it rewrote + +#### Scenario: The renamed project verifies and checks clean + +- **WHEN** migration 9 has renamed a colliding project +- **THEN** `verify` SHALL report no collision +- **AND** each renamed rule SHALL still run and report findings under its new id + +#### Scenario: Migration 9 is idempotent + +- **WHEN** migration 9 runs a second time over a project it has already renamed, or over one with no collision +- **THEN** it SHALL write nothing + +#### Scenario: A project with no rules tree is left alone + +- **WHEN** `.taskless/rules/` does not exist and migration 9 runs +- **THEN** the migration SHALL succeed and write nothing diff --git a/packages/cli/src/commands/info.ts b/packages/cli/src/commands/info.ts index 86b94cf9..b47233cb 100644 --- a/packages/cli/src/commands/info.ts +++ b/packages/cli/src/commands/info.ts @@ -8,7 +8,7 @@ import { fetchWhoami } from "../auth/whoami"; import { outputSchema as infoOutputSchema } from "../schemas/info"; import { makeErrorEnvelope } from "../types/errors"; import { resolveRepositoryContext } from "../util/git-remote"; -import { readManifest } from "../filesystem/migrate"; +import { readManifest } from "../filesystem/manifest"; import { reconciliationStart } from "../rules/reconcile-marker"; import { TASKLESS_DIRECTORY } from "../rules/vale/formats"; diff --git a/packages/cli/src/commands/init.ts b/packages/cli/src/commands/init.ts index 9cc1ae23..26943266 100644 --- a/packages/cli/src/commands/init.ts +++ b/packages/cli/src/commands/init.ts @@ -33,7 +33,7 @@ import { reconciliationStart, stampNewProjectRules, } from "../rules/reconcile-marker"; -import { readManifest } from "../filesystem/migrate"; +import { readManifest } from "../filesystem/manifest"; import type { MigrationReport } from "../filesystem/migrate"; import { TASKLESS_DIRECTORY } from "../rules/vale/formats"; import { CLIError } from "../util/cli-error"; diff --git a/packages/cli/src/commands/onboard.ts b/packages/cli/src/commands/onboard.ts index 308de480..d537016e 100644 --- a/packages/cli/src/commands/onboard.ts +++ b/packages/cli/src/commands/onboard.ts @@ -4,7 +4,7 @@ import process from "node:process"; import { defineCommand } from "citty"; import { ensureTasklessDirectory } from "../filesystem/directory"; -import { readManifest, writeManifest } from "../filesystem/migrate"; +import { readManifest, writeManifest } from "../filesystem/manifest"; import { getRecipe } from "../prompts/recipes"; import { withSurveyInvite } from "../survey/invite"; import { getTelemetry } from "../telemetry"; diff --git a/packages/cli/src/filesystem/manifest.ts b/packages/cli/src/filesystem/manifest.ts new file mode 100644 index 00000000..f78347bb --- /dev/null +++ b/packages/cli/src/filesystem/manifest.ts @@ -0,0 +1,225 @@ +import { readFile, writeFile } from "node:fs/promises"; +import { join } from "node:path"; + +import { CLIError } from "../util/cli-error"; +import { buildInvocation } from "../util/invocation"; + +/** + * The `.taskless/taskless.json` manifest: its shape, and reading and writing it. + * + * SPLIT OUT OF `migrate.ts` SO THE MANIFEST CAN BE READ WITHOUT LOADING EVERY + * MIGRATION. The two halves were one module, so "read the manifest" pulled in + * the migration registry, and a migration importing anything that reads the + * manifest closed a loop through the runner. That is not hypothetical: `0009` + * reached `rules/reconcile-marker` for `pathExists`, reconcile-marker reads the + * manifest, and the cycle left `migrations["9"]` holding `undefined` on any + * graph entered through `rules/files.ts` — surfacing as + * `TypeError: migrate is not a function` in the middle of a rule write, and + * only there, because every other entry point happened to evaluate the modules + * in a working order. + * + * THIS MODULE MUST NOT IMPORT `migrate.ts`. That is the whole property it + * exists to hold: the manifest is data plus two accessors, and nothing about + * reading it needs to know that migrations exist. + */ + +export interface TasklessInstallTarget { + skills?: string[]; + commands?: string[]; + /** + * Install mode for this target: `canonical` (full content) or `reference` + * (stubs delegating to the canonical store). Absent in manifests written + * before this field existed; consumers treat a missing value as canonical. + */ + mode?: "canonical" | "reference"; +} + +export interface TasklessInstallManifest { + cliVersion?: string; + targets?: Record; + onboarded?: boolean; +} + +/** + * What the project's rules were last reconciled against. + * + * Separate from `install` on purpose, because the two answer different + * questions and drift apart. `install` records how the scaffold got here; + * `rules` records what the rules are valid against. Conflating them is what + * made `install.cliVersion` a bad candidate for this: a skills refresh moves + * it without anyone having read a rule. + * + * Every field here advances ONLY on a completed reconciliation, never on an + * upgrade. If a CLI bump silently rewrote `engines.sg` to the newly vendored + * version, the field would always report "current" and the divergence it + * exists to expose would be invisible. + */ +export interface TasklessRulesManifest { + /** CLI version whose ledger entries have all been walked and acted on. */ + reconciledTo?: string; + /** + * Engine versions the rules were authored and last reconciled against. + * + * Engine version is what determines whether matching semantics moved under + * a rule, so recording it is what lets a later differential ask a concrete + * question instead of reconstructing one. + */ + engines?: { + sg?: string; + vale?: string; + }; +} + +export interface TasklessManifest { + version: number; + install?: TasklessInstallManifest; + rules?: TasklessRulesManifest; +} + +const MANIFEST_FILE = "taskless.json"; + +/** + * The refusal a manifest that exists but cannot be read produces. + * + * Named separately because the remedy is the interesting part. It must NOT + * say "run `init`": `init` re-runs every migration and then rewrites the + * manifest from what it managed to parse, which for an unreadable file is + * nothing. Measured on a manifest whose first line reads `"version": 6` and + * whose second is a leftover `<<<<<<< HEAD`: the file came back as + * `{"version": 6}` with `install.onboarded` and the whole `rules` block gone. + */ +function unreadableManifest(path: string, reason: string): CLIError { + return new CLIError( + `${path} could not be read: ${reason}.\n\n` + + `This is not a schema version mismatch, so migrating will not help: ` + + `\`${buildInvocation()} init\` refuses here too, rather than rewriting the file ` + + `with only the part it can parse. A leftover merge conflict, a truncated write ` + + `or a partial editor save are the usual causes.\n\n` + + `Repair the JSON by hand, or delete the file to rebuild the scaffold from scratch.`, + "SCAFFOLD_MANIFEST_UNREADABLE" + ); +} + +/** + * Read the manifest file, returning the full parsed record plus the normalized + * version. Unknown top-level fields are preserved so callers can round-trip + * them on write. + * + * ABSENT AND UNREADABLE ARE DIFFERENT STATES, and collapsing them was the bug + * in taskless/cli#278. An absent manifest is an ordinary fresh project and + * reads as version 0. A manifest that is present and unparseable read as + * version 0 too, which `requireCurrentSchema` then reported as fact: a file + * declaring `"version": 6` produced "This project's .taskless/ is at schema + * version 0". The number was invented, and acting on it destroyed the file. + * + * So the second case throws. Every caller that could rewrite the manifest + * reaches it first, which is what makes the refusal a guard rather than a + * better message. + */ +export async function readRawManifest( + directory: string +): Promise<{ version: number; raw: Record }> { + const path = join(directory, MANIFEST_FILE); + let content: string; + try { + content = await readFile(path, "utf8"); + } catch (error) { + if ( + error && + typeof error === "object" && + "code" in error && + (error as NodeJS.ErrnoException).code === "ENOENT" + ) { + return { version: 0, raw: {} }; + } + throw error; + } + + let parsed: unknown; + try { + parsed = JSON.parse(content); + } catch (error) { + throw unreadableManifest( + path, + error instanceof Error ? error.message : String(error) + ); + } + + // Any non-object (`null`, an array, a primitive) is valid JSON that is not a + // manifest. Reading `.version` off `null` would throw a bare TypeError, and + // treating it as version 0 has the same consequence as an unparseable file: + // the next write replaces whatever is there. + if (!isPlainObject(parsed)) { + throw unreadableManifest(path, "its top-level value is not a JSON object"); + } + + // A missing or non-numeric `version` on an otherwise readable object is NOT + // this failure. The rest of the object survives a migration untouched, since + // every write merges over `raw`, so migrating from 0 loses nothing. + const version = Number(parsed.version); + return { + version: Number.isFinite(version) ? version : 0, + raw: parsed, + }; +} + +export async function writeRawManifest( + directory: string, + raw: Record +): Promise { + await writeFile( + join(directory, MANIFEST_FILE), + JSON.stringify(raw, null, 2) + "\n", + "utf8" + ); +} + +/** + * Read the full manifest, returning the typed shape. Unknown fields are + * discarded by this API — if you need round-trip preservation, use + * {@link readManifest} below and pass its `raw` object back through + * {@link writeManifest}. + */ +export async function readManifest( + directory: string +): Promise<{ manifest: TasklessManifest; raw: Record }> { + const { version, raw } = await readRawManifest(directory); + const install = raw.install as TasklessInstallManifest | undefined; + const rules = raw.rules as TasklessRulesManifest | undefined; + return { + manifest: { + version, + install: isPlainObject(install) ? install : undefined, + rules: isPlainObject(rules) ? rules : undefined, + }, + raw, + }; +} + +/** + * Write the manifest, merging the provided fields over any existing unknown + * top-level fields stored in `raw`. Callers typically pass the `raw` object + * returned by {@link readManifest} to preserve forward-compatible state. + */ +export async function writeManifest( + directory: string, + manifest: TasklessManifest, + raw: Record = {} +): Promise { + const merged: Record = { ...raw, version: manifest.version }; + if (manifest.install === undefined) { + delete merged.install; + } else { + merged.install = manifest.install; + } + if (manifest.rules === undefined) { + delete merged.rules; + } else { + merged.rules = manifest.rules; + } + await writeRawManifest(directory, merged); +} + +function isPlainObject(value: unknown): value is Record { + return typeof value === "object" && value !== null && !Array.isArray(value); +} diff --git a/packages/cli/src/filesystem/migrate.ts b/packages/cli/src/filesystem/migrate.ts index 7dc6d409..908b56a5 100644 --- a/packages/cli/src/filesystem/migrate.ts +++ b/packages/cli/src/filesystem/migrate.ts @@ -1,10 +1,10 @@ -import { readFile, writeFile } from "node:fs/promises"; import { join } from "node:path"; import process from "node:process"; import { CLIError } from "../util/cli-error"; import { buildInvocation } from "../util/invocation"; import { pathExists } from "../rules/reconcile-marker"; +import { readRawManifest, writeRawManifest } from "./manifest"; import type { Migrations } from "./types"; import { diffSnapshots, snapshotPaths, type TreeChanges } from "./snapshot"; import init from "./migrations/0001-init"; @@ -15,61 +15,7 @@ import ruleDirectories from "./migrations/0005-rule-directories"; import refreshReadme from "./migrations/0006-refresh-readme"; import ignoreScratchFiles from "./migrations/0007-ignore-scratch-files"; import dropBasedOnStyles from "./migrations/0008-drop-based-on-styles"; - -export interface TasklessInstallTarget { - skills?: string[]; - commands?: string[]; - /** - * Install mode for this target: `canonical` (full content) or `reference` - * (stubs delegating to the canonical store). Absent in manifests written - * before this field existed; consumers treat a missing value as canonical. - */ - mode?: "canonical" | "reference"; -} - -export interface TasklessInstallManifest { - cliVersion?: string; - targets?: Record; - onboarded?: boolean; -} - -/** - * What the project's rules were last reconciled against. - * - * Separate from `install` on purpose, because the two answer different - * questions and drift apart. `install` records how the scaffold got here; - * `rules` records what the rules are valid against. Conflating them is what - * made `install.cliVersion` a bad candidate for this: a skills refresh moves - * it without anyone having read a rule. - * - * Every field here advances ONLY on a completed reconciliation, never on an - * upgrade. If a CLI bump silently rewrote `engines.sg` to the newly vendored - * version, the field would always report "current" and the divergence it - * exists to expose would be invisible. - */ -export interface TasklessRulesManifest { - /** CLI version whose ledger entries have all been walked and acted on. */ - reconciledTo?: string; - /** - * Engine versions the rules were authored and last reconciled against. - * - * Engine version is what determines whether matching semantics moved under - * a rule, so recording it is what lets a later differential ask a concrete - * question instead of reconstructing one. - */ - engines?: { - sg?: string; - vale?: string; - }; -} - -export interface TasklessManifest { - version: number; - install?: TasklessInstallManifest; - rules?: TasklessRulesManifest; -} - -const MANIFEST_FILE = "taskless.json"; +import uniqueRuleIds from "./migrations/0009-unique-rule-ids"; const migrations: Migrations = { "1": init, @@ -80,6 +26,7 @@ const migrations: Migrations = { "6": refreshReadme, "7": ignoreScratchFiles, "8": dropBasedOnStyles, + "9": uniqueRuleIds, }; /** Global flag that downgrades a too-new scaffold from an error to a skip. */ @@ -106,152 +53,6 @@ function sortedMigrations( .toSorted(([a], [b]) => a - b); } -/** - * The refusal a manifest that exists but cannot be read produces. - * - * Named separately because the remedy is the interesting part. It must NOT - * say "run `init`": `init` re-runs every migration and then rewrites the - * manifest from what it managed to parse, which for an unreadable file is - * nothing. Measured on a manifest whose first line reads `"version": 6` and - * whose second is a leftover `<<<<<<< HEAD`: the file came back as - * `{"version": 6}` with `install.onboarded` and the whole `rules` block gone. - */ -function unreadableManifest(path: string, reason: string): CLIError { - return new CLIError( - `${path} could not be read: ${reason}.\n\n` + - `This is not a schema version mismatch, so migrating will not help: ` + - `\`${buildInvocation()} init\` refuses here too, rather than rewriting the file ` + - `with only the part it can parse. A leftover merge conflict, a truncated write ` + - `or a partial editor save are the usual causes.\n\n` + - `Repair the JSON by hand, or delete the file to rebuild the scaffold from scratch.`, - "SCAFFOLD_MANIFEST_UNREADABLE" - ); -} - -/** - * Read the manifest file, returning the full parsed record plus the normalized - * version. Unknown top-level fields are preserved so callers can round-trip - * them on write. - * - * ABSENT AND UNREADABLE ARE DIFFERENT STATES, and collapsing them was the bug - * in taskless/cli#278. An absent manifest is an ordinary fresh project and - * reads as version 0. A manifest that is present and unparseable read as - * version 0 too, which `requireCurrentSchema` then reported as fact: a file - * declaring `"version": 6` produced "This project's .taskless/ is at schema - * version 0". The number was invented, and acting on it destroyed the file. - * - * So the second case throws. Every caller that could rewrite the manifest - * reaches it first, which is what makes the refusal a guard rather than a - * better message. - */ -async function readRawManifest( - directory: string -): Promise<{ version: number; raw: Record }> { - const path = join(directory, MANIFEST_FILE); - let content: string; - try { - content = await readFile(path, "utf8"); - } catch (error) { - if ( - error && - typeof error === "object" && - "code" in error && - (error as NodeJS.ErrnoException).code === "ENOENT" - ) { - return { version: 0, raw: {} }; - } - throw error; - } - - let parsed: unknown; - try { - parsed = JSON.parse(content); - } catch (error) { - throw unreadableManifest( - path, - error instanceof Error ? error.message : String(error) - ); - } - - // Any non-object (`null`, an array, a primitive) is valid JSON that is not a - // manifest. Reading `.version` off `null` would throw a bare TypeError, and - // treating it as version 0 has the same consequence as an unparseable file: - // the next write replaces whatever is there. - if (!isPlainObject(parsed)) { - throw unreadableManifest(path, "its top-level value is not a JSON object"); - } - - // A missing or non-numeric `version` on an otherwise readable object is NOT - // this failure. The rest of the object survives a migration untouched, since - // every write merges over `raw`, so migrating from 0 loses nothing. - const version = Number(parsed.version); - return { - version: Number.isFinite(version) ? version : 0, - raw: parsed, - }; -} - -async function writeRawManifest( - directory: string, - raw: Record -): Promise { - await writeFile( - join(directory, MANIFEST_FILE), - JSON.stringify(raw, null, 2) + "\n", - "utf8" - ); -} - -/** - * Read the full manifest, returning the typed shape. Unknown fields are - * discarded by this API — if you need round-trip preservation, use - * {@link readManifest} below and pass its `raw` object back through - * {@link writeManifest}. - */ -export async function readManifest( - directory: string -): Promise<{ manifest: TasklessManifest; raw: Record }> { - const { version, raw } = await readRawManifest(directory); - const install = raw.install as TasklessInstallManifest | undefined; - const rules = raw.rules as TasklessRulesManifest | undefined; - return { - manifest: { - version, - install: isPlainObject(install) ? install : undefined, - rules: isPlainObject(rules) ? rules : undefined, - }, - raw, - }; -} - -/** - * Write the manifest, merging the provided fields over any existing unknown - * top-level fields stored in `raw`. Callers typically pass the `raw` object - * returned by {@link readManifest} to preserve forward-compatible state. - */ -export async function writeManifest( - directory: string, - manifest: TasklessManifest, - raw: Record = {} -): Promise { - const merged: Record = { ...raw, version: manifest.version }; - if (manifest.install === undefined) { - delete merged.install; - } else { - merged.install = manifest.install; - } - if (manifest.rules === undefined) { - delete merged.rules; - } else { - merged.rules = manifest.rules; - } - await writeRawManifest(directory, merged); -} - -function isPlainObject(value: unknown): value is Record { - return typeof value === "object" && value !== null && !Array.isArray(value); -} - /** * What one migration run changed, in a form a caller can print or hand to a * machine consumer. diff --git a/packages/cli/src/filesystem/migrations/0009-unique-rule-ids.ts b/packages/cli/src/filesystem/migrations/0009-unique-rule-ids.ts new file mode 100644 index 00000000..932905d5 --- /dev/null +++ b/packages/cli/src/filesystem/migrations/0009-unique-rule-ids.ts @@ -0,0 +1,376 @@ +import { readdir, readFile, rename, writeFile } from "node:fs/promises"; +import { join } from "node:path"; + +import { + findRuleIdCollisions, + metadataSidecarPath, +} from "../../rules/id-uniqueness"; +import { listRuleIds, ruleDirectory } from "../../rules/engines"; +// Safe to reach for now that the manifest lives in `filesystem/manifest.ts`. +// `reconcile-marker` reads the manifest, and while that meant importing +// `migrate.ts` — the module holding the migration registry — this import +// closed a loop that left `migrations["9"]` undefined. The manifest no longer +// knows migrations exist, so the path stops here. +import { pathExists } from "../../rules/reconcile-marker"; +import { + ENGINE_LAYOUTS, + ENGINES, + RULE_TESTS_DIRECTORY, + type EngineName, +} from "../../rules/layout"; +import { isValidRuleId } from "../../rules/validate-id"; +import type { Migration } from "../types"; + +/** + * Give every rule id held by more than one engine a name of its own. + * + * `.taskless/rules/sg/no-eval/` and `.taskless/rules/vale/no-eval/` could both + * exist, because `isValidRuleId` is `/^[a-z0-9][a-z0-9-]*$/` with no engine + * component and no cross-engine check. `verify` now refuses that state per + * rule, which is the guard for a collision created after this runs — by hand, + * or by a merge. This is what clears the ones that are already there. + * + * ## Why it renames rather than refuses + * + * A throwing migration walls `init`, and `init` is the command every other + * refusal points at: `check` and `verify` send a stale scaffold there with + * `SCAFFOLD_MIGRATION_REQUIRED`. Refusing therefore leaves the user in a state + * where the CLI's own instruction is the thing that fails, and the way out is + * a multi-file hand edit. Renaming resolves it at the one moment the CLI has + * the user's attention and full knowledge of the layout. + * + * It is safe to do here because a rename reaches nothing outside the rule's + * own directory. Measured against this tree: + * + * | Engine | What carries the id | + * | --------- | -------------------------------------------------------------------------- | + * | `sg` | directory, `.yml`, its `id:` field, `.tests/-*-test.yml` and each file's `id:` | + * | `vale` | directory, `.yml`, and in `.vale.ini` the `tskl) rule` breadcrumb and the `.` assignment | + * | `runtime` | nothing — a runtime rule is never renamed; see below | + * + * Both Vale segments move because `StylesPath` points at `rules/vale`, so the + * rule directory is the style and `.yml` is the check inside it. Nothing + * outside `.taskless/rules///` names a rule id: `taskless.json` + * records versions rather than rules, and the runtime reconcile join is by + * content signature, so a moved-but-unchanged rule still resolves. + * + * ## Runtime rules are never renamed + * + * A `runtime` copy keeps the bare id, and only `sg` and `vale` copies are + * moved. Runtime rules are the tier whose artifacts are signed and blessed, + * and leaving them untouched keeps this migration clear of that machinery + * entirely rather than reasoning about it. Measured, a rename would in fact be + * safe — `signRuleFile` hashes the CONTENT of `check.ts` and never its path, + * and the reconcile join is by signature, so a moved-but-unchanged rule still + * resolves — so this is a precaution rather than a correctness fix. It costs + * nothing: the result is collision-free either way. + * + * ## Among the engines that do move, the rename is symmetric + * + * When `sg` and `vale` both hold an id, both move; neither keeps it. Any + * precedence rule between them would be arbitrary, and a symmetric rename + * means nobody has to work out which of their two rules silently kept the + * name. `check` output moves with it, which is why the changeset says to + * expect it. + * + * The result is collision-free in every case, because within one engine the + * filesystem already guarantees one directory per id. `sg` + `runtime` leaves + * `sg/-sg` beside `runtime/`; all three leaves `sg/-sg`, + * `vale/-vale` and `runtime/`. + * + * ## What it will not do + * + * It never clobbers. A target already in use takes the next free + * `--2`, `-3`, … and a name is only free when NO engine holds it, + * so clearing one collision cannot create another. It never touches + * `.taskless/rule-metadata/.yml`: the rename is symmetric, so the sidecar + * has no natural owner and moving it to either side would be a guess. It is + * left in place, reported, and orphaned — which costs nothing, because the + * service does not populate the `meta` block a sidecar is written from, so + * this CLI has never written one (`rule meta` says so when asked). + * + * Every rename is printed: old path, new path, and each file rewritten inside + * it. A migration that silently renames a user's rules is worse than one that + * refuses. + * + * Idempotent, and read-only when there is nothing to do. A project with no + * collision is enumerated and nothing is written, so `git status` stays clean. + */ +/** + * The engine whose rules keep their id whatever else holds it. + * + * Named rather than inlined so the carve-out is one fact in one place: the + * loop, the docblock table and the report all mean the same thing by it. + */ +const NEVER_RENAMED: EngineName = "runtime"; + +const migration: Migration = async (directory) => { + // The collision is a fact about `.taskless/rules/`, and every helper that + // describes it takes the PROJECT root, which is this directory's parent. + const cwd = join(directory, ".."); + const collisions = await findRuleIdCollisions(cwd); + if (collisions.length === 0) return; + + const taken = await occupiedRuleIds(cwd); + const lines: string[] = []; + for (const collision of collisions) { + for (const engine of collision.engines) { + if (engine === NEVER_RENAMED) { + // Said out loud rather than left as a silent omission: a reader + // looking at a three-engine collision must not be left wondering why + // one of the three did not move. Its id stays in `taken`, so nothing + // else can be renamed onto it. + lines.push( + ` ${ruleDirectory(cwd, engine, collision.ruleId)}`, + ` kept its id (runtime rules are never renamed)` + ); + continue; + } + const to = freeRuleId(collision.ruleId, engine, taken); + taken.add(to); + lines.push(...(await renameRule(cwd, engine, collision.ruleId, to))); + } + const sidecar = metadataSidecarPath(cwd, collision.ruleId); + if (await pathExists(sidecar)) { + lines.push( + ` ! ${sidecar} left in place: the rename is symmetric, so the sidecar has no owner to follow.` + ); + } + } + console.error( + [ + `Migration 9 renamed ${String(collisions.length)} rule id(s) held by more than one engine:`, + ...lines, + ].join("\n") + ); +}; + +/** + * Every rule id in use, across every engine. + * + * Collected once up front rather than re-read per candidate, so a name chosen + * for one half of a collision is unavailable to the other half in the same + * run. Without that, `sg/x` and `vale/x` could both be offered the same free + * name and the second rename would clobber the first. + */ +async function occupiedRuleIds(cwd: string): Promise> { + const ids = new Set(); + for (const engine of ENGINES) { + for (const id of await listRuleIds(cwd, engine)) ids.add(id); + } + return ids; +} + +/** + * `-`, or the first free `--N` when that is taken. + * + * Free means held by NO engine, not merely by this one: a name that resolves + * one collision by creating another has resolved nothing. The suffix is a + * plain ascending integer from 2, so the choice is reproducible and a reader + * of the printed report can see why it landed where it did. + */ +function freeRuleId( + ruleId: string, + engine: EngineName, + taken: Set +): string { + const base = `${ruleId}-${engine}`; + // `` already matched `/^[a-z0-9][a-z0-9-]*$/` and every engine name is + // lowercase letters, so the result cannot fail the id contract. Asserted + // rather than assumed, because the one thing worse than a refusal here is a + // rename to a name the rest of the CLI will not accept. + if (!isValidRuleId(base)) { + throw new Error(`Migration 9 would rename "${ruleId}" to an invalid id`); + } + if (!taken.has(base)) return base; + for (let suffix = 2; ; suffix++) { + const candidate = `${base}-${String(suffix)}`; + if (!taken.has(candidate)) return candidate; + } +} + +/** Rename one rule and every reference to its id inside its own directory. */ +async function renameRule( + cwd: string, + engine: EngineName, + from: string, + to: string +): Promise { + const fromPath = ruleDirectory(cwd, engine, from); + const toPath = ruleDirectory(cwd, engine, to); + await rename(fromPath, toPath); + const lines = [` ${fromPath}`, ` -> ${toPath}`]; + + if (engine === "sg") { + lines.push( + ...(await renameRuleFile(toPath, engine, from, to)), + ...(await renameSgFixtures(toPath, from, to)) + ); + } else if (engine === "vale") { + lines.push( + ...(await renameRuleFile(toPath, engine, from, to)), + ...(await rewriteValeConfig(toPath, from, to)) + ); + } + // No `runtime` branch: this is never called for one. See `NEVER_RENAMED`. + return lines; +} + +/** + * The rule file named after `from` becomes the one named after `to`, and its + * own `id:` follows. + * + * The name comes from {@link ENGINE_LAYOUTS}, the table that decides it, so + * the two engines this runs for stop being a second place that has to agree + * with `layout.ts` about `${id}.yml`. Not `ruleFilePath`, which takes a `cwd` + * and a rule id and would resolve into the PRE-rename directory: by the time + * this is called the directory has already moved, and only the file inside it + * still carries the old name. + */ +async function renameRuleFile( + ruleDirectoryPath: string, + engine: EngineName, + from: string, + to: string +): Promise { + const fromName = ENGINE_LAYOUTS[engine].ruleFile(from); + const toName = ENGINE_LAYOUTS[engine].ruleFile(to); + const fromFile = join(ruleDirectoryPath, fromName); + if (!(await pathExists(fromFile))) return []; + const toFile = join(ruleDirectoryPath, toName); + await rename(fromFile, toFile); + const rewritten = await rewriteIdField(toFile, from, to); + return [ + ` renamed ${fromName} -> ${toName}${rewritten ? " and its id: field" : ""}`, + ]; +} + +/** + * `.tests/-*-test.yml` becomes `.tests/-*-test.yml`, each file's + * `id:` with it. + * + * BOTH halves are load-bearing, and missing either leaves a rule that looks + * tested and is not. `discoverRuleTestFiles` claims a file for a rule by the + * `-` filename prefix, so a file left under the old prefix stops being + * found at all and the rule fails `sg-test-file-required`. What ast-grep + * actually RUNS is keyed on the `id:` inside the file, so a renamed file + * still carrying the old id is discovered, silently not counted, and the rule + * reads as having shipped no cases. + */ +async function renameSgFixtures( + ruleDirectoryPath: string, + from: string, + to: string +): Promise { + const testsPath = join(ruleDirectoryPath, RULE_TESTS_DIRECTORY); + let entries: string[]; + try { + entries = await readdir(testsPath); + } catch { + // No fixtures. `verify` reports that as `sg-test-file-required`; it is not + // this migration's business. + return []; + } + const lines: string[] = []; + for (const entry of entries) { + if (!entry.startsWith(`${from}-`) || !entry.endsWith("-test.yml")) continue; + const renamed = `${to}-${entry.slice(from.length + 1)}`; + await rename(join(testsPath, entry), join(testsPath, renamed)); + const rewritten = await rewriteIdField(join(testsPath, renamed), from, to); + lines.push( + ` renamed ${RULE_TESTS_DIRECTORY}/${entry} -> ${RULE_TESTS_DIRECTORY}/${renamed}${rewritten ? " and its id: field" : ""}` + ); + } + return lines; +} + +/** + * The breadcrumb and the `.` assignment in a Vale rule's config. + * + * Both segments of the assignment move: `StylesPath` points at `rules/vale`, + * so the rule directory is the style and `.yml` is the check inside it. + */ +async function rewriteValeConfig( + ruleDirectoryPath: string, + from: string, + to: string +): Promise { + const configPath = join(ruleDirectoryPath, ".vale.ini"); + let source: string; + try { + source = await readFile(configPath, "utf8"); + } catch { + // A rule with no config declares no scope. `verify` reports that; there is + // nothing here to rewrite. + return []; + } + const rewritten = retargetValeConfig(source, from, to); + if (rewritten === source) return []; + await writeFile(configPath, rewritten, "utf8"); + return [` rewrote .vale.ini breadcrumb and ${from}.${from} assignment`]; +} + +/** A `.vale.ini` retargeted from one rule id to another. Exported for tests. */ +export function retargetValeConfig( + source: string, + from: string, + to: string +): string { + const quoted = escapeForRegExp(from); + return source + .replaceAll( + new RegExp( + String.raw`^([ \t]*tskl\) rule[ \t]*=[ \t]*)${quoted}([ \t]*)$`, + "gm" + ), + `$1${to}$2` + ) + .replaceAll( + new RegExp(String.raw`^([ \t]*)${quoted}\.${quoted}([ \t]*=)`, "gm"), + `$1${to}.${to}$2` + ); +} + +/** + * Rewrite a YAML document's top-level `id:` when, and only when, it currently + * reads as `from`. + * + * Anchored on the key at the start of a line, the way `0008` anchors its + * deletion, so every other byte survives: a rule file is the author's own + * text, and a parse-and-re-serialize would reflow it. A file whose `id:` is + * something else is left alone rather than corrected — that is the + * `sg-id-matches-directory` defect, `verify` already names it, and quietly + * fixing it here would hide a rule that was never what its directory claimed. + * + * Returns whether anything changed, so the printed report does not claim an + * edit it did not make. + */ +async function rewriteIdField( + path: string, + from: string, + to: string +): Promise { + let source: string; + try { + source = await readFile(path, "utf8"); + } catch { + return false; + } + const rewritten = source.replaceAll( + new RegExp( + String.raw`^(id:[ \t]*)(['"]?)${escapeForRegExp(from)}\2([ \t]*)$`, + "gm" + ), + `$1$2${to}$2$3` + ); + if (rewritten === source) return false; + await writeFile(path, rewritten, "utf8"); + return true; +} + +/** A rule id is `[a-z0-9-]+`, but escaping keeps this honest if that widens. */ +function escapeForRegExp(value: string): string { + return value.replaceAll(/[$()*+.?[\\\]^{|}]/g, String.raw`\$&`); +} + +export default migration; diff --git a/packages/cli/src/install/state.ts b/packages/cli/src/install/state.ts index e97bd04d..68f25e9a 100644 --- a/packages/cli/src/install/state.ts +++ b/packages/cli/src/install/state.ts @@ -3,8 +3,8 @@ import { join } from "node:path"; import type { TasklessInstallManifest, TasklessInstallTarget, -} from "../filesystem/migrate"; -import { readManifest, writeManifest } from "../filesystem/migrate"; +} from "../filesystem/manifest"; +import { readManifest, writeManifest } from "../filesystem/manifest"; const TASKLESS_DIR = ".taskless"; diff --git a/packages/cli/src/rules/files.ts b/packages/cli/src/rules/files.ts index 246e0c4f..69379f97 100644 --- a/packages/cli/src/rules/files.ts +++ b/packages/cli/src/rules/files.ts @@ -14,6 +14,7 @@ import { findRuleEngines, } from "./engines"; import type { EngineName } from "./layout"; +import { describeRuleIdCollision, findRuleIdCollision } from "./id-uniqueness"; import { isValidRuleId } from "./validate-id"; import { assessDelivery, @@ -125,6 +126,7 @@ export async function writeRuleFile( ) { onWarning?.(`Rule "${rule.id}" ${missingFixtures}.`); } + await warnOnIdCollision(cwd, rule.id, onWarning); // The rule file, so the caller's contract ("where did this rule land") // is unchanged whichever envelope delivered it. return ruleFilePath(cwd, engine, rule.id); @@ -154,9 +156,35 @@ export async function writeRuleFile( await mkdir(ruleDirectory(cwd, engine, rule.id), { recursive: true }); const filePath = ruleFilePath(cwd, engine, rule.id); await writeFile(filePath, stringify(rule.content, { lineWidth: 0 }), "utf8"); + await warnOnIdCollision(cwd, rule.id, onWarning); return filePath; } +/** + * Say so when the rule just written shares its id with another engine's. + * + * A WARNING, never a refusal, and that is the whole design. `check`'s repair + * path calls {@link writeRuleFile}, so refusing here would brick repair for + * both colliding rules — strictly worse than the silence it replaces. The + * failure belongs in `verify`, which is what the message points at. + * + * After the write, like the fixtures warning above it: this is an observation + * about a rule that is now on disk, and warning first would read as a reason + * it was refused. + */ +async function warnOnIdCollision( + cwd: string, + ruleId: string, + onWarning?: (message: string) => void +): Promise { + if (onWarning === undefined) return; + const collision = await findRuleIdCollision(cwd, ruleId); + if (collision === undefined) return; + onWarning( + `${describeRuleIdCollision(cwd, collision)} \`verify\` fails both until one is renamed.` + ); +} + /** * Whether a rule's `content` is something a rule file can be written from. * diff --git a/packages/cli/src/rules/id-uniqueness.ts b/packages/cli/src/rules/id-uniqueness.ts new file mode 100644 index 00000000..06ee328a --- /dev/null +++ b/packages/cli/src/rules/id-uniqueness.ts @@ -0,0 +1,113 @@ +import { join } from "node:path"; + +import { findRuleEngines, listRuleIds, ruleDirectory } from "./engines"; +import { ENGINES, type EngineName } from "./layout"; + +/** + * One rule id held by more than one engine. + * + * A rule id is a directory name under `.taskless/rules//`, and nothing + * in the id contract makes it unique across the three sibling trees: + * `isValidRuleId` is `/^[a-z0-9][a-z0-9-]*$/`, with no engine component and no + * cross-engine check. So `.taskless/rules/sg/no-eval/` and + * `.taskless/rules/vale/no-eval/` can both exist, and until this existed + * nothing said so. + */ +export interface RuleIdCollision { + ruleId: string; + /** Every engine holding the id, in {@link ENGINES} order. Always ≥ 2. */ + engines: EngineName[]; + /** Each engine's directory for the id, in the same order. */ + paths: string[]; +} + +function collisionFrom( + cwd: string, + ruleId: string, + engines: EngineName[] +): RuleIdCollision | undefined { + if (engines.length < 2) return undefined; + return { + ruleId, + engines, + paths: engines.map((engine) => ruleDirectory(cwd, engine, ruleId)), + }; +} + +/** + * Whether this one id is held by more than one engine. + * + * Asked per rule rather than only over the whole tree, because verifying the + * rule an author just wrote is the moment a collision is cheapest to fix. A + * whole-project pass that only diffs the engine id lists would say nothing at + * exactly that moment. + */ +export async function findRuleIdCollision( + cwd: string, + ruleId: string +): Promise { + return collisionFrom(cwd, ruleId, await findRuleEngines(cwd, ruleId)); +} + +/** + * Every collision in the project, in id order. + * + * Built from the per-engine id lists rather than by re-asking + * {@link findRuleEngines} for each id, so the tree is enumerated once. The + * answer is the same one {@link findRuleIdCollision} gives for each id. + */ +export async function findRuleIdCollisions( + cwd: string +): Promise { + const holders = new Map(); + for (const engine of ENGINES) { + for (const ruleId of await listRuleIds(cwd, engine)) { + const existing = holders.get(ruleId); + if (existing === undefined) { + holders.set(ruleId, [engine]); + } else { + existing.push(engine); + } + } + } + const collisions: RuleIdCollision[] = []; + for (const ruleId of [...holders.keys()].toSorted((a, b) => + a.localeCompare(b) + )) { + const collision = collisionFrom(cwd, ruleId, holders.get(ruleId) ?? []); + if (collision !== undefined) collisions.push(collision); + } + return collisions; +} + +/** The sidecar both colliding rules write to and read from. */ +export function metadataSidecarPath(cwd: string, ruleId: string): string { + return join(cwd, ".taskless", "rule-metadata", `${ruleId}.yml`); +} + +/** + * The condition, worded the way `rules delete` already words it. + * + * `rules delete` refuses the same state with "Rule … is held by N engines, so + * there is no single rule to delete: ". Two surfaces describing one + * condition in two vocabularies is how a user comes to believe they are two + * conditions, so the opening clause is shared verbatim and only the + * consequence differs. + * + * The sidecar is named because it is the damage. `writeRuleMetaFiles` keys + * `.taskless/rule-metadata/{id}.yml` on the id alone, so the two rules share + * one file: the second `rule create` or `rule improve` overwrites the first's + * metadata silently, and `deleteRuleFiles` removes it for whichever rule goes + * first. That happens whether or not anyone runs `check`. + */ +export function describeRuleIdCollision( + cwd: string, + collision: RuleIdCollision +): string { + return ( + `Rule "${collision.ruleId}" is held by ${String(collision.engines.length)} engines, ` + + `so its id does not name one rule: ${collision.paths.join(", ")}. ` + + `They share one metadata sidecar at ${metadataSidecarPath(cwd, collision.ruleId)}, ` + + `so whichever was written last owns it.` + ); +} diff --git a/packages/cli/src/rules/inspect.ts b/packages/cli/src/rules/inspect.ts index d0996b6a..823ecced 100644 --- a/packages/cli/src/rules/inspect.ts +++ b/packages/cli/src/rules/inspect.ts @@ -25,6 +25,7 @@ import { validateValeRuleConfig } from "../schemas/vale-config"; import { validateValeRule } from "../schemas/vale-rule"; import { verifyRule, type VerifyResult } from "./verify"; import { violate, type RuleViolation } from "./constraints"; +import { describeRuleIdCollision, findRuleIdCollision } from "./id-uniqueness"; import { verifyValeRule } from "./vale/verify"; import type { ResolvedRule } from "./resolve-path"; @@ -191,6 +192,54 @@ async function verifySgRule( * `verify` and `test` are separate commands. */ export async function verifyOneRule( + cwd: string, + rule: ResolvedRule +): Promise { + return withIdCollision( + cwd, + rule.ruleId, + await verifyRuleComponents(cwd, rule) + ); +} + +/** + * Fail a verdict whose rule id is held by more than one engine. + * + * Applied AFTER the engine's own layers rather than instead of them, so the + * author still hears everything that is wrong with the rule in front of them. + * The collision is listed first because it is the only one of the failures + * that is about the project rather than about the file, and the only one whose + * remedy is a rename. + * + * Not a {@link RULE_CONSTRAINTS} entry, deliberately. Every constraint is + * declared for one `engine` and published per engine in the conformance + * corpus, because a constraint answers "what does this CLI require of a rule + * for THIS engine beyond what the engine itself requires". This requires + * nothing of the rule: the file is valid, and what is wrong is that a sibling + * tree holds the same directory name. Giving it an engine would mean either + * inventing an engine-agnostic constraint kind for a single entry, or filing + * three near-identical ones and telling a generator that ast-grep has a house + * rule about Vale. + */ +async function withIdCollision( + cwd: string, + ruleId: string, + verification: RuleVerification +): Promise { + const collision = await findRuleIdCollision(cwd, ruleId); + if (collision === undefined) return verification; + return { + ...verification, + ok: false, + errors: [ + `${describeRuleIdCollision(cwd, collision)} Rename one of them.`, + ...verification.errors, + ], + }; +} + +/** {@link verifyOneRule} minus the cross-engine uniqueness check. */ +async function verifyRuleComponents( cwd: string, { engine, ruleId }: ResolvedRule ): Promise { @@ -359,7 +408,11 @@ export async function testOneRule( // One call covers both halves. The verdict is still consulted first and // still short-circuits, so the ordering above is unchanged — the tests // simply already ran alongside the layers that decide it. - const { verification, result } = await verifySgRule(cwd, ruleId); + const { verification: verdict, result } = await verifySgRule(cwd, ruleId); + // `test` runs `verify` first, and the uniqueness check is part of `verify`. + // Reached through the same helper rather than by a second `verifySgRule` + // call, which would spawn `sg test` twice for one answer. + const verification = await withIdCollision(cwd, ruleId, verdict); if (!verification.ok) { return { ...verification, ran: false }; } diff --git a/packages/cli/src/rules/reconcile-marker.ts b/packages/cli/src/rules/reconcile-marker.ts index bc36d638..be387443 100644 --- a/packages/cli/src/rules/reconcile-marker.ts +++ b/packages/cli/src/rules/reconcile-marker.ts @@ -2,7 +2,7 @@ import { access } from "node:fs/promises"; import { join } from "node:path"; import { AST_GREP_VERSION, VALE_VERSION } from "./capabilities"; -import { readManifest, writeManifest } from "../filesystem/migrate"; +import { readManifest, writeManifest } from "../filesystem/manifest"; import { TASKLESS_DIRECTORY } from "./vale/formats"; import { CLIError } from "../util/cli-error"; import { getCliVersion } from "../wizard/intro"; diff --git a/packages/cli/test/check-rule-filter.test.ts b/packages/cli/test/check-rule-filter.test.ts index 875be200..4a4fbb69 100644 --- a/packages/cli/test/check-rule-filter.test.ts +++ b/packages/cli/test/check-rule-filter.test.ts @@ -319,6 +319,25 @@ describe("check --rule", () => { // `rules delete` refuses an ambiguous id because deleting the wrong rule // is irreversible. Measuring is not, and an unfiltered `check` would have // run both, so both run and `source` tells them apart. + + // MIGRATE FIRST, THEN BUILD THE COLLISION. Migration 9 + // (`0009-unique-rule-ids`) renames every id held by more than one engine, + // so a collision seeded into the fixture before it runs is renamed to + // `no-eval-sg`/`no-eval-vale` and `--rule no-eval` then names no rule at + // all — `check` exits `RULE_NOT_FOUND` and this test dies in `triples()` + // reading `.map` of an undefined `results`. `runCli` migrates on every + // invocation via `migrateFixture`, so the migration has to happen here, + // before the second engine's copy exists. + // + // That is not a trick to keep the old wording alive: it is the only way a + // project can hold this state now. The migration clears the collisions + // already on disk, and what remains is one created AFTER it ran — by + // hand, or by a merge landing a same-id rule under another engine — which + // is exactly the case the per-rule check in `verify` exists to catch. + // `check` still has to measure both, and `rules/rule-filter.ts` says so. + // Do not "simplify" this back into the `beforeEach`. + await migrateFixture(["-d", project]); + const valeRule = join(project, ".taskless/rules/vale/no-eval"); await mkdir(valeRule, { recursive: true }); await writeFile( diff --git a/packages/cli/test/migrate-install.test.ts b/packages/cli/test/migrate-install.test.ts index 45bf72de..5750904f 100644 --- a/packages/cli/test/migrate-install.test.ts +++ b/packages/cli/test/migrate-install.test.ts @@ -4,11 +4,8 @@ import { tmpdir } from "node:os"; import { describe, expect, it, beforeEach, afterEach } from "vitest"; import { ensureTasklessDirectory } from "../src/filesystem/directory"; -import { - readManifest, - writeManifest, - LATEST_SCHEMA_VERSION, -} from "../src/filesystem/migrate"; +import { readManifest, writeManifest } from "../src/filesystem/manifest"; +import { LATEST_SCHEMA_VERSION } from "../src/filesystem/migrate"; describe("install-state migrations", () => { let temporaryDirectory: string; diff --git a/packages/cli/test/migration-registry.test.ts b/packages/cli/test/migration-registry.test.ts new file mode 100644 index 00000000..7386154e --- /dev/null +++ b/packages/cli/test/migration-registry.test.ts @@ -0,0 +1,88 @@ +// THE IMPORT ORDER IN THIS FILE IS THE TEST. Do not reorder, and do not let a +// formatter group these differently — see the docblock below. +import { mkdir, mkdtemp, rm } from "node:fs/promises"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; + +import { afterEach, beforeEach, describe, expect, it } from "vitest"; + +// Entered FIRST, before the runner. A MIGRATION MODULE ITSELF is the entry +// that broke the registry: reached before `migrate.ts`, its own default export +// is still unassigned when the runner (pulled in behind it) builds the record, +// so the registry captures `undefined`. `rule-id-uniqueness.test.ts` imports +// `0009` on its first line for its unit cases, which is the only reason the +// original cycle was ever observed. +import "../src/filesystem/migrations/0009-unique-rule-ids"; +// The path a rule write takes to the runner, via `ensureTasklessDirectory`. +import "../src/rules/files"; +import { + LATEST_SCHEMA_VERSION, + runMigrations, +} from "../src/filesystem/migrate"; + +/** + * Every version in the migration registry resolves to a function when the + * graph is entered through a rule write. + * + * THIS ASSERTION WAS SILENTLY FALSE, and nothing reported it. Migration `0009` + * imported `pathExists` from `rules/reconcile-marker`, which read the manifest + * from `filesystem/migrate.ts` — the module holding the registry. The cycle + * left `migrations["9"]` holding `undefined`, and the only symptom was + * `TypeError: migrate is not a function` thrown from the middle of a rule + * write. The manifest now lives in `filesystem/manifest.ts` and knows nothing + * about migrations, so the loop is gone; this is what keeps it gone. + * + * STATIC IMPORTS, IN THIS ORDER, AND THAT IS NOT INCIDENTAL. Two earlier + * versions of this test were measured against a deliberately reintroduced + * cycle and BOTH PASSED, which is the only reason this one is trusted: + * + * 1. `vi.resetModules()` with dynamic `import()`, to exercise several entry + * orders from one file. Vite's SSR module transform resolves a dynamic + * re-import differently from the hoisted static graph, so the broken order + * was never reproduced at all. + * 2. Static imports, but entered through `rules/files.ts`. Not enough: by then + * `migrate.ts` is reached before any migration module, and it builds the + * record from fully evaluated imports. + * + * What reproduces it is entering at a MIGRATION MODULE first, which is what + * `rule-id-uniqueness.test.ts` happens to do on its first line. A test for a + * cycle has to be entered the way the cycle was, and "the suite is green" is + * not evidence that it would be. + * + * Proven by RUNNING the registry rather than inspecting its shape: + * `runMigrations` reports every version it applied, and a version bound to + * `undefined` throws on call rather than reaching the `applied` list. That also + * keeps the registry unexported, since exporting internals to make an assertion + * possible is the shape of a check in the wrong place. + */ +let cwd: string; + +beforeEach(async () => { + cwd = await mkdtemp(join(tmpdir(), "tskl-registry-")); + // `runMigrations` takes an existing `.taskless/`; creating it is + // `ensureTasklessDirectory`'s job, and going through that would enter the + // graph from one more fixed place rather than the one under test. + await mkdir(join(cwd, ".taskless"), { recursive: true }); +}); + +afterEach(async () => { + await rm(cwd, { recursive: true, force: true }); +}); + +describe("the migration registry", () => { + it("applies every registered version when entered through a rule write", async () => { + const report = await runMigrations(join(cwd, ".taskless"), { + onNotice: () => { + /* silence the scaffold notice */ + }, + }); + + expect(report).toBeDefined(); + expect(report?.to).toBe(LATEST_SCHEMA_VERSION); + // Every version from 1 to the latest ran. A registry entry bound to + // `undefined` throws when called, so it cannot appear here. + expect(report?.applied).toEqual( + Array.from({ length: LATEST_SCHEMA_VERSION }, (_, index) => index + 1) + ); + }); +}); diff --git a/packages/cli/test/rule-id-uniqueness.test.ts b/packages/cli/test/rule-id-uniqueness.test.ts new file mode 100644 index 00000000..a84d64d1 --- /dev/null +++ b/packages/cli/test/rule-id-uniqueness.test.ts @@ -0,0 +1,449 @@ +import { mkdir, mkdtemp, readFile, readdir, rm, stat } from "node:fs/promises"; +import { writeFile } from "node:fs/promises"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; + +import { afterEach, beforeEach, describe, expect, it } from "vitest"; + +import migration, { + retargetValeConfig, +} from "../src/filesystem/migrations/0009-unique-rule-ids"; +import { writeRuleFile } from "../src/rules/files"; +import { verifyOneRule } from "../src/rules/inspect"; +import { findRuleIdCollisions } from "../src/rules/id-uniqueness"; +import type { EngineName } from "../src/rules/layout"; + +/** + * A rule id is a directory name under `.taskless/rules//`, and nothing + * in the id contract makes it unique across the three sibling trees. These + * cases pin the two places that now say so: `verify`, per rule, and migration + * `0009`, once per project on upgrade. + * + * Every case uses `vale` and `runtime` rules. Both verify from the files alone, + * so nothing here depends on an engine binary being installed — and the + * uniqueness check is engine-agnostic by construction, so the pair chosen + * proves the same thing an `sg`/`vale` pair would. + */ +let cwd: string; + +const SCOPED = (id: string): string => + `[*.md]\ntskl) rule = ${id}\n${id}.${id} = YES\n`; + +async function valeRule(id: string): Promise { + const directory = join(cwd, ".taskless", "rules", "vale", id); + await mkdir(directory, { recursive: true }); + await writeFile( + join(directory, `${id}.yml`), + `extends: existence\nmessage: "Avoid %s"\nlevel: warning\ntokens:\n - simply\n`, + "utf8" + ); + await writeFile(join(directory, ".vale.ini"), SCOPED(id), "utf8"); + return directory; +} + +/** `.taskless/rules//`, for asserting on where a rule landed. */ +function rulePath(engine: EngineName, id: string): string { + return join(cwd, ".taskless", "rules", engine, id); +} + +async function exists(path: string): Promise { + try { + await stat(path); + return true; + } catch { + return false; + } +} + +/** An sg rule, with the `id:` field and one fixture the rename has to follow. */ +async function sgRule(id: string): Promise { + const directory = join(cwd, ".taskless", "rules", "sg", id); + await mkdir(join(directory, ".tests"), { recursive: true }); + await writeFile( + join(directory, `${id}.yml`), + `id: ${id}\nlanguage: TypeScript\nseverity: error\nmessage: no eval\nrule:\n pattern: eval($A)\n`, + "utf8" + ); + await writeFile( + join(directory, ".tests", `${id}-20260101-test.yml`), + `id: ${id}\nvalid:\n - const a = 1;\ninvalid:\n - eval(x);\n`, + "utf8" + ); + return directory; +} + +async function runtimeRule(id: string): Promise { + const directory = join(cwd, ".taskless", "rules", "runtime", id); + await mkdir(directory, { recursive: true }); + await writeFile(join(directory, "check.ts"), "export default () => [];\n"); + return directory; +} + +beforeEach(async () => { + cwd = await mkdtemp(join(tmpdir(), "tskl-rule-id-")); + await mkdir(join(cwd, ".taskless", "rules"), { recursive: true }); +}); + +afterEach(async () => { + await rm(cwd, { recursive: true, force: true }); +}); + +describe("verify refuses a rule id held by more than one engine", () => { + it("fails both rules of a colliding pair, naming both paths", async () => { + const valePath = await valeRule("no-eval"); + const runtimePath = await runtimeRule("no-eval"); + + for (const engine of ["vale", "runtime"] as const) { + const result = await verifyOneRule(cwd, { engine, ruleId: "no-eval" }); + expect(result.ok).toBe(false); + const joined = result.errors.join(" "); + expect(joined).toContain(valePath); + expect(joined).toContain(runtimePath); + expect(joined).toContain("is held by 2 engines"); + } + }); + + // The whole reason the check is per rule rather than only project-wide: an + // author verifying the rule they just wrote is the moment the collision is + // cheapest to fix, and a pass over the two id lists says nothing then. + it("catches the collision when a single rule is verified", async () => { + await valeRule("no-eval"); + await runtimeRule("no-eval"); + await valeRule("no-simply"); + + const result = await verifyOneRule(cwd, { + engine: "vale", + ruleId: "no-eval", + }); + expect(result.errors[0]).toContain("does not name one rule"); + }); + + it("passes a tree where every id is held by one engine", async () => { + await valeRule("no-simply"); + await runtimeRule("env-keys-declared"); + + const vale = await verifyOneRule(cwd, { + engine: "vale", + ruleId: "no-simply", + }); + expect(vale.ok).toBe(true); + expect(vale.errors).toEqual([]); + expect(await findRuleIdCollisions(cwd)).toEqual([]); + }); + + // The collision is not attributed to a published constraint: every entry in + // RULE_CONSTRAINTS is declared for one engine, and this one is about the + // project's layout rather than about any engine's rule. + it("reports the collision without attributing it to a constraint", async () => { + await valeRule("no-eval"); + await runtimeRule("no-eval"); + + const result = await verifyOneRule(cwd, { + engine: "vale", + ruleId: "no-eval", + }); + expect(result.violations).toEqual([]); + }); +}); + +describe("migration 0009 renames a colliding project", () => { + // Symmetric between the engines that move: neither `sg` nor `vale` keeps the + // bare id, because any precedence rule between them would be arbitrary and + // would leave a user working out which of their two rules kept the name. + it("renames both sg and vale copies to -", async () => { + await sgRule("no-eval"); + await valeRule("no-eval"); + + await migration(join(cwd, ".taskless")); + + expect(await exists(rulePath("sg", "no-eval"))).toBe(false); + expect(await exists(rulePath("vale", "no-eval"))).toBe(false); + expect(await exists(rulePath("sg", "no-eval-sg"))).toBe(true); + expect(await exists(rulePath("vale", "no-eval-vale"))).toBe(true); + }); + + it("moves an sg rule's file, its id: field, and its fixtures", async () => { + await sgRule("no-eval"); + await valeRule("no-eval"); + + await migration(join(cwd, ".taskless")); + + const directory = rulePath("sg", "no-eval-sg"); + expect(await readFile(join(directory, "no-eval-sg.yml"), "utf8")).toContain( + "id: no-eval-sg" + ); + // BOTH halves matter: the filename prefix is how `discoverRuleTestFiles` + // claims a fixture for a rule, and the `id:` inside is what ast-grep + // attributes cases by. Miss either and the rule reads as untested. + const fixture = join(directory, ".tests", "no-eval-sg-20260101-test.yml"); + expect(await exists(fixture)).toBe(true); + expect(await readFile(fixture, "utf8")).toContain("id: no-eval-sg"); + }); + + it("moves a Vale rule's style file and both segments of its config", async () => { + await sgRule("no-eval"); + await valeRule("no-eval"); + + await migration(join(cwd, ".taskless")); + + const directory = rulePath("vale", "no-eval-vale"); + expect(await exists(join(directory, "no-eval-vale.yml"))).toBe(true); + const config = await readFile(join(directory, ".vale.ini"), "utf8"); + expect(config).toContain("tskl) rule = no-eval-vale"); + // The style directory AND the style file basename both moved, because + // StylesPath points at rules/vale. + expect(config).toContain("no-eval-vale.no-eval-vale = YES"); + expect(config).not.toContain("no-eval.no-eval"); + }); + + // Runtime rules are the signed tier. Leaving them alone keeps the migration + // clear of that machinery entirely — and costs nothing, because within one + // engine the filesystem already guarantees one directory per id, so moving + // the other copy is enough to resolve the collision. + it("never renames a runtime rule, moving only the sg copy", async () => { + await sgRule("no-eval"); + const runtimeDirectory = await runtimeRule("no-eval"); + const before = await snapshot(runtimeDirectory); + + await migration(join(cwd, ".taskless")); + + // Byte-identical: same paths, same sizes, same mtimes. + expect(await snapshot(runtimeDirectory)).toEqual(before); + expect(await exists(rulePath("runtime", "no-eval"))).toBe(true); + expect(await exists(rulePath("runtime", "no-eval-runtime"))).toBe(false); + expect(await exists(rulePath("sg", "no-eval"))).toBe(false); + expect(await exists(rulePath("sg", "no-eval-sg"))).toBe(true); + expect(await findRuleIdCollisions(cwd)).toEqual([]); + }); + + it("never renames a runtime rule when Vale is the other holder", async () => { + await valeRule("no-eval"); + await runtimeRule("no-eval"); + + await migration(join(cwd, ".taskless")); + + expect(await exists(rulePath("runtime", "no-eval"))).toBe(true); + expect(await exists(rulePath("vale", "no-eval-vale"))).toBe(true); + const verified = await verifyOneRule(cwd, { + engine: "vale", + ruleId: "no-eval-vale", + }); + expect(verified.ok).toBe(true); + expect(await findRuleIdCollisions(cwd)).toEqual([]); + }); + + it("moves sg and vale and leaves runtime alone when all three collide", async () => { + await sgRule("no-eval"); + await valeRule("no-eval"); + const runtimeDirectory = await runtimeRule("no-eval"); + const before = await snapshot(runtimeDirectory); + + await migration(join(cwd, ".taskless")); + + expect(await exists(rulePath("sg", "no-eval-sg"))).toBe(true); + expect(await exists(rulePath("vale", "no-eval-vale"))).toBe(true); + expect(await exists(rulePath("runtime", "no-eval"))).toBe(true); + expect(await snapshot(runtimeDirectory)).toEqual(before); + expect(await findRuleIdCollisions(cwd)).toEqual([]); + + // Every surviving rule still verifies. The runtime one keeps the bare id + // and is no longer in collision with anything. + for (const rule of [ + { engine: "vale", ruleId: "no-eval-vale" }, + { engine: "runtime", ruleId: "no-eval" }, + ] as const) { + const result = await verifyOneRule(cwd, rule); + expect( + result.errors.filter((error) => error.includes("is held by")), + `${rule.engine}/${rule.ruleId}` + ).toEqual([]); + } + }); + + // Never clobbers. `-` taken means the next free ascending + // suffix, and free means held by NO engine, so clearing one collision + // cannot create another. + it("takes the next free suffix when - is taken", async () => { + await sgRule("no-eval"); + await valeRule("no-eval"); + await sgRule("no-eval-sg"); + + await migration(join(cwd, ".taskless")); + + // The pre-existing `no-eval-sg` is untouched and keeps its own id. + expect( + await readFile( + join(rulePath("sg", "no-eval-sg"), "no-eval-sg.yml"), + "utf8" + ) + ).toContain("id: no-eval-sg"); + expect(await exists(rulePath("sg", "no-eval-sg-2"))).toBe(true); + expect( + await readFile( + join(rulePath("sg", "no-eval-sg-2"), "no-eval-sg-2.yml"), + "utf8" + ) + ).toContain("id: no-eval-sg-2"); + }); + + it("leaves the metadata sidecar in place rather than guessing an owner", async () => { + await sgRule("no-eval"); + await valeRule("no-eval"); + const sidecar = join(cwd, ".taskless", "rule-metadata", "no-eval.yml"); + await mkdir(join(cwd, ".taskless", "rule-metadata"), { recursive: true }); + await writeFile(sidecar, "title: something\n", "utf8"); + + await migration(join(cwd, ".taskless")); + + expect(await readFile(sidecar, "utf8")).toBe("title: something\n"); + }); + + // The rewritten Vale config has to still describe a rule Vale would enable: + // both segments of `.` moved, and a half-renamed assignment verifies + // as a rule that is present and off. + it("leaves the renamed Vale rule verifying clean", async () => { + await sgRule("no-eval"); + await valeRule("no-eval"); + + await migration(join(cwd, ".taskless")); + + const result = await verifyOneRule(cwd, { + engine: "vale", + ruleId: "no-eval-vale", + }); + expect(result.errors).toEqual([]); + expect(result.ok).toBe(true); + }); + + it("leaves no collision behind", async () => { + await sgRule("no-eval"); + await valeRule("no-eval"); + await runtimeRule("no-eval"); + + await migration(join(cwd, ".taskless")); + + expect(await findRuleIdCollisions(cwd)).toEqual([]); + }); + + it("is a no-op on a project with no collision, writing nothing", async () => { + await valeRule("no-simply"); + await runtimeRule("env-keys-declared"); + const before = await snapshot(join(cwd, ".taskless")); + + await expect(migration(join(cwd, ".taskless"))).resolves.toBeUndefined(); + await expect(migration(join(cwd, ".taskless"))).resolves.toBeUndefined(); + + expect(await snapshot(join(cwd, ".taskless"))).toEqual(before); + }); + + it("is idempotent: a second run after a rename changes nothing", async () => { + await sgRule("no-eval"); + await valeRule("no-eval"); + await migration(join(cwd, ".taskless")); + const after = await snapshot(join(cwd, ".taskless")); + + await migration(join(cwd, ".taskless")); + + expect(await snapshot(join(cwd, ".taskless"))).toEqual(after); + }); + + it("is a no-op on a project with no rules tree at all", async () => { + await rm(join(cwd, ".taskless", "rules"), { recursive: true }); + await expect(migration(join(cwd, ".taskless"))).resolves.toBeUndefined(); + }); +}); + +describe("retargetValeConfig", () => { + it("moves the breadcrumb and both assignment segments, leaving other bytes", () => { + const source = + "# no-eval is mentioned in this comment\n" + + "[*.md]\n" + + "tskl) rule = no-eval\n" + + "no-eval.no-eval = YES\n" + + "\n" + + "[CHANGELOG.md]\n" + + "tskl) rule = no-eval\n" + + "no-eval.no-eval = NO\n"; + + expect(retargetValeConfig(source, "no-eval", "no-eval-vale")).toBe( + "# no-eval is mentioned in this comment\n" + + "[*.md]\n" + + "tskl) rule = no-eval-vale\n" + + "no-eval-vale.no-eval-vale = YES\n" + + "\n" + + "[CHANGELOG.md]\n" + + "tskl) rule = no-eval-vale\n" + + "no-eval-vale.no-eval-vale = NO\n" + ); + }); + + it("leaves a config naming a different rule alone", () => { + const source = "[*.md]\ntskl) rule = other\nother.other = YES\n"; + expect(retargetValeConfig(source, "no-eval", "no-eval-vale")).toBe(source); + }); +}); + +describe("writeRuleFile keeps working through a collision", () => { + // `check`'s repair path calls `writeRuleFile`. A refusal here would brick + // repair for BOTH colliding rules, which is worse than the silence it would + // replace, so the write succeeds and only warns. + it("writes the rule and warns instead of refusing", async () => { + await valeRule("no-eval"); + const warnings: string[] = []; + + const written = await writeRuleFile( + cwd, + { + id: "no-eval", + engine: "sg", + content: { + id: "no-eval", + language: "TypeScript", + severity: "error", + message: "no eval", + rule: { pattern: "eval($A)" }, + }, + } as Parameters[1], + (message) => warnings.push(message) + ); + + expect(await readFile(written, "utf8")).toContain("no-eval"); + expect(warnings.join(" ")).toContain("is held by 2 engines"); + }); + + it("does not warn when the id is held by one engine", async () => { + const warnings: string[] = []; + await writeRuleFile( + cwd, + { + id: "no-debugger", + engine: "sg", + content: { + id: "no-debugger", + language: "TypeScript", + severity: "error", + message: "no debugger", + rule: { pattern: "debugger" }, + }, + } as Parameters[1], + (message) => warnings.push(message) + ); + expect(warnings).toEqual([]); + }); +}); + +/** Every file under `directory`, with its size and mtime, for an idempotency check. */ +async function snapshot(directory: string): Promise { + const entries = await readdir(directory, { + recursive: true, + withFileTypes: true, + }); + const lines: string[] = []; + for (const entry of entries) { + if (entry.isDirectory()) continue; + const path = join(entry.parentPath, entry.name); + const stats = await stat(path); + lines.push(`${path} ${String(stats.size)} ${stats.mtimeMs.toString()}`); + } + return lines.toSorted((a, b) => a.localeCompare(b)); +}