From dd0caced7968096ab1ad1d507ce03483d0b3d238 Mon Sep 17 00:00:00 2001 From: Jakob Heuser Date: Tue, 22 Sep 2026 10:46:38 -0700 Subject: [PATCH 1/6] feat(verify): refuse a rule id held by more than one engine `.taskless/rules/sg/no-eval/` and `.taskless/rules/vale/no-eval/` could both exist, and the collision was silent. The two share one `.taskless/rule-metadata/no-eval.yml`, because the sidecar is keyed by id alone, so the second `rule create` overwrites the first's metadata and deleting either takes the shared file with it. `verify` now fails such a rule, per rule rather than only project-wide, since verifying the rule an author just wrote is when the collision is cheapest to fix. `writeRuleFile` only warns, because `check`'s repair path calls it. Migration 9 detects and refuses on upgrade, never renaming: nothing can tell which rule should keep the id. --- .changeset/rule-id-uniqueness.md | 11 + .taskless/taskless.json | 2 +- .../2026-09-22-rule-id-uniqueness/proposal.md | 40 +++ .../specs/cli-rule-validation/spec.md | 74 ++++++ .../specs/cli-taskless-bootstrap/spec.md | 39 +++ .../2026-09-22-rule-id-uniqueness/tasks.md | 22 ++ openspec/specs/cli-rule-validation/spec.md | 43 ++++ openspec/specs/cli-taskless-bootstrap/spec.md | 38 +++ packages/cli/src/filesystem/migrate.ts | 2 + .../migrations/0009-unique-rule-ids.ts | 67 +++++ packages/cli/src/rules/files.ts | 28 +++ packages/cli/src/rules/id-uniqueness.ts | 113 +++++++++ packages/cli/src/rules/inspect.ts | 55 +++- packages/cli/test/rule-id-uniqueness.test.ts | 236 ++++++++++++++++++ 14 files changed, 768 insertions(+), 2 deletions(-) create mode 100644 .changeset/rule-id-uniqueness.md create mode 100644 openspec/changes/archive/2026-09-22-rule-id-uniqueness/proposal.md create mode 100644 openspec/changes/archive/2026-09-22-rule-id-uniqueness/specs/cli-rule-validation/spec.md create mode 100644 openspec/changes/archive/2026-09-22-rule-id-uniqueness/specs/cli-taskless-bootstrap/spec.md create mode 100644 openspec/changes/archive/2026-09-22-rule-id-uniqueness/tasks.md create mode 100644 packages/cli/src/filesystem/migrations/0009-unique-rule-ids.ts create mode 100644 packages/cli/src/rules/id-uniqueness.ts create mode 100644 packages/cli/test/rule-id-uniqueness.test.ts diff --git a/.changeset/rule-id-uniqueness.md b/.changeset/rule-id-uniqueness.md new file mode 100644 index 00000000..17a60874 --- /dev/null +++ b/.changeset/rule-id-uniqueness.md @@ -0,0 +1,11 @@ +--- +"@taskless/cli": patch +--- + +`verify` now fails a rule whose id is also a directory name under another engine, and a new scaffold migration (`9`) refuses to migrate a project that already holds one. + +Two rules with the same id under two engines — `.taskless/rules/sg/no-eval/` beside `.taskless/rules/vale/no-eval/` — share one `.taskless/rule-metadata/no-eval.yml`, because the sidecar is keyed by id alone. The second `rule create` or `rule improve` overwrites the first's metadata silently, `rule meta ` returns the wrong rule's, and deleting either one takes the shared sidecar with it. None of that needed anyone to run `check`. + +**If your project already has a collision**, the first `taskless init` after upgrading will refuse, naming both rule directories. Rename one of them — the directory, the rule file inside it, the rule's own `id:` field where its engine has one, and the `rule-metadata/.yml` sidecar — then re-run `init`. Nothing is renamed for you on purpose: nothing in the CLI can tell which of the two rules should keep the id, and the sidecar, the rule's `.tests/` fixtures and the server-side id all reference the old name, so an automatic rename would break the references of whichever rule it moved. + +`writeRuleFile` still writes through a collision and only warns, so `check`'s repair path is unaffected. 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..c067da7b --- /dev/null +++ b/openspec/changes/archive/2026-09-22-rule-id-uniqueness/proposal.md @@ -0,0 +1,40 @@ +## 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 damage is not hypothetical and does not need anyone to run `check`. `writeRuleMetaFiles` keys `.taskless/rule-metadata/{id}.yml` on the id alone, so two colliding rules share one sidecar: the second `rule create` or `rule improve` overwrites the first's metadata silently, `rule meta ` returns the wrong rule's, and `deleteRuleFiles` removes the shared file for whichever rule is deleted first, leaving the survivor without one. Human `check` output cannot tell the two apart either, because `util/format.ts` prints `severity[ruleId]` with no engine. The JSON envelope is fine: every result carries `source`. + +## 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` detects and refuses, and never renames.** Nothing there can tell which rule should keep the id, and a rename is not local: the sidecar, the rule's `.tests/` fixtures and the server-side id all reference the old name, so an automatic rename would pick one at random and break the references of whichever it moved. It is read-only in every case and idempotent. `LATEST_SCHEMA_VERSION` becomes 9 and this repository's own `.taskless/taskless.json` is bumped with it. +- **The refusal composes with `SCAFFOLD_MIGRATION_REQUIRED` rather than looping with it.** `check` and `verify` refuse a stale scaffold by naming `init`, and `init` is what runs migrations. A refusal that only reported the collision would send the user straight back to `init`, which would refuse again. It names the rename to perform _before_ re-running `init`, and reports `RULE_ID_AMBIGUOUS` so an agent that has learned what to do with `rules delete`'s refusal has learned what to do with this one. + +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. + +## 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. +- 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..e8980cef --- /dev/null +++ b/openspec/changes/archive/2026-09-22-rule-id-uniqueness/specs/cli-taskless-bootstrap/spec.md @@ -0,0 +1,39 @@ +## ADDED Requirements + +### Requirement: Migration 9 refuses a project whose rule id is held by more than one engine + +Migration `9` SHALL read `.taskless/rules/` and SHALL refuse the migration when any rule id is a directory name under more than one engine, naming every directory holding the id and the `.taskless/rule-metadata/.yml` sidecar they share. It SHALL report the same error code `rules delete` reports for the same condition, `RULE_ID_AMBIGUOUS`. + +It SHALL NOT rename anything. Nothing available to a migration can tell which of the two rules should keep the id, and a rename is not local: the sidecar, the rule's `.tests/` fixtures and the server-side id all reference the old name, so an automatic rename would pick one at random and break the references of whichever it moved. + +The refusal SHALL state the rename to perform BEFORE `init` is re-run. `check` and `verify` refuse a scaffold behind the current version with `SCAFFOLD_MIGRATION_REQUIRED`, which names `init`, and `init` is what runs migrations; a refusal that only reported the collision would return the user to `init` and be refused again. + +The migration SHALL write nothing in any case. A project with no collision 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: A colliding project is refused by name + +- **WHEN** `no-eval` exists under two engines and migration 9 runs +- **THEN** the migration SHALL throw +- **AND** the message SHALL name both rule directories and the shared metadata sidecar +- **AND** the error code SHALL be `RULE_ID_AMBIGUOUS` + +#### Scenario: The refusal names the rename to perform before init + +- **WHEN** migration 9 refuses a colliding project +- **THEN** the message SHALL say to rename one of the directories before re-running `init` +- **AND** it SHALL say that nothing is renamed automatically + +#### Scenario: Nothing is renamed + +- **WHEN** migration 9 refuses a colliding project +- **THEN** both rule directories SHALL remain where they were + +#### Scenario: Migration 9 is idempotent + +- **WHEN** migration 9 runs twice over a project with no collision +- **THEN** neither run SHALL write anything + +#### 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..c44c2766 --- /dev/null +++ b/openspec/changes/archive/2026-09-22-rule-id-uniqueness/tasks.md @@ -0,0 +1,22 @@ +## 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 Add `filesystem/migrations/0009-unique-rule-ids.ts`: read-only, throws a `CLIError` coded `RULE_ID_AMBIGUOUS` naming every holding directory, the shared sidecar, and the rename to perform before re-running `init`. Register `"9"` in `migrate.ts`. +- [x] 2.2 Trace the refusal against `SCAFFOLD_MIGRATION_REQUIRED` and confirm the two messages form a path out rather than a loop. +- [x] 2.3 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 refuses a colliding project naming both directories, says to rename before re-running `init`, is a no-op and writes nothing on a clean project across two runs, and is a no-op with no rules tree. +- [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 `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..158b968b 100644 --- a/openspec/specs/cli-taskless-bootstrap/spec.md +++ b/openspec/specs/cli-taskless-bootstrap/spec.md @@ -269,3 +269,41 @@ 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 refuses a project whose rule id is held by more than one engine + +Migration `9` SHALL read `.taskless/rules/` and SHALL refuse the migration when any rule id is a directory name under more than one engine, naming every directory holding the id and the `.taskless/rule-metadata/.yml` sidecar they share. It SHALL report the same error code `rules delete` reports for the same condition, `RULE_ID_AMBIGUOUS`. + +It SHALL NOT rename anything. Nothing available to a migration can tell which of the two rules should keep the id, and a rename is not local: the sidecar, the rule's `.tests/` fixtures and the server-side id all reference the old name, so an automatic rename would pick one at random and break the references of whichever it moved. + +The refusal SHALL state the rename to perform BEFORE `init` is re-run. `check` and `verify` refuse a scaffold behind the current version with `SCAFFOLD_MIGRATION_REQUIRED`, which names `init`, and `init` is what runs migrations; a refusal that only reported the collision would return the user to `init` and be refused again. + +The migration SHALL write nothing in any case. A project with no collision 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: A colliding project is refused by name + +- **WHEN** `no-eval` exists under two engines and migration 9 runs +- **THEN** the migration SHALL throw +- **AND** the message SHALL name both rule directories and the shared metadata sidecar +- **AND** the error code SHALL be `RULE_ID_AMBIGUOUS` + +#### Scenario: The refusal names the rename to perform before init + +- **WHEN** migration 9 refuses a colliding project +- **THEN** the message SHALL say to rename one of the directories before re-running `init` +- **AND** it SHALL say that nothing is renamed automatically + +#### Scenario: Nothing is renamed + +- **WHEN** migration 9 refuses a colliding project +- **THEN** both rule directories SHALL remain where they were + +#### Scenario: Migration 9 is idempotent + +- **WHEN** migration 9 runs twice over a project with no collision +- **THEN** neither run SHALL write anything + +#### 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/filesystem/migrate.ts b/packages/cli/src/filesystem/migrate.ts index 7dc6d409..4922c0b7 100644 --- a/packages/cli/src/filesystem/migrate.ts +++ b/packages/cli/src/filesystem/migrate.ts @@ -15,6 +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"; +import uniqueRuleIds from "./migrations/0009-unique-rule-ids"; export interface TasklessInstallTarget { skills?: string[]; @@ -80,6 +81,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. */ 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..96b49c18 --- /dev/null +++ b/packages/cli/src/filesystem/migrations/0009-unique-rule-ids.ts @@ -0,0 +1,67 @@ +import { join } from "node:path"; + +import { + describeRuleIdCollision, + findRuleIdCollisions, + metadataSidecarPath, + type RuleIdCollision, +} from "../../rules/id-uniqueness"; +import { CLIError } from "../../util/cli-error"; +import { buildInvocation } from "../../util/invocation"; +import type { Migration } from "../types"; + +/** + * Refuse a project where one rule id is held by more than one engine. + * + * `verify` now fails such a rule, but `verify` only reaches rules someone + * runs it on. A project that already holds `sg/no-eval` beside `vale/no-eval` + * would keep sharing one `rule-metadata/no-eval.yml` until someone happened to + * look, and the sidecar is overwritten by whichever rule is written last, with + * nothing reporting it. The upgrade is the one moment every existing project + * passes through, so this is where they are all checked. + * + * DETECTS AND REFUSES. IT MUST NEVER RENAME. Nothing here can tell which of + * the two rules should keep the id, and a rename is not local: the + * `rule-metadata/{id}.yml` sidecar, the rule's `.tests/` fixtures and the + * server-side id all reference the old name. An automatic rename would pick + * one at random and break the references of whichever it moved. + * + * The message has to do more work than a migration's usually does, because of + * where the user is standing when they read it. `check` and `verify` refuse a + * stale scaffold with `SCAFFOLD_MIGRATION_REQUIRED`, which says to run `init`; + * `init` is what runs this migration. So a refusal that only said "there is a + * collision" would send the user back to `init`, which would refuse again. It + * names the rename to perform BEFORE re-running `init`, which is what turns + * the pair of messages into a path out rather than a loop. + * + * Idempotent, and read-only in every case. A project with no collision is + * enumerated and nothing is written, so a second run touches nothing and + * `git status` stays clean. + */ +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; + throw new CLIError( + collisions.map((collision) => refusal(cwd, collision)).join("\n\n"), + // The same code `rules delete` reports for the same condition. An agent + // that has learned what to do with one has learned what to do with both. + "RULE_ID_AMBIGUOUS" + ); +}; + +/** One collision, and the rename that clears it. */ +function refusal(cwd: string, collision: RuleIdCollision): string { + return ( + `${describeRuleIdCollision(cwd, collision)}\n\n` + + `Rename one of those directories before re-running ` + + `\`${buildInvocation()} init\`, and rename with it: the rule file inside ` + + `it, the rule's own \`id:\` field where its engine has one, and the ` + + `sidecar at ${metadataSidecarPath(cwd, collision.ruleId)}. Nothing here ` + + `can tell which rule should keep the id, so nothing is renamed for you.` + ); +} + +export default migration; 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/test/rule-id-uniqueness.test.ts b/packages/cli/test/rule-id-uniqueness.test.ts new file mode 100644 index 00000000..4aa21d0a --- /dev/null +++ b/packages/cli/test/rule-id-uniqueness.test.ts @@ -0,0 +1,236 @@ +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 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 { CLIError } from "../src/util/cli-error"; + +/** + * 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; +} + +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 refuses a colliding project", () => { + it("throws naming both directories and the rename to perform", async () => { + const valePath = await valeRule("no-eval"); + const runtimePath = await runtimeRule("no-eval"); + + const error = await migration(join(cwd, ".taskless")).then( + () => {}, + (error_: unknown) => error_ + ); + expect(error).toBeInstanceOf(CLIError); + const message = (error as CLIError).message; + expect(message).toContain(valePath); + expect(message).toContain(runtimePath); + expect(message).toContain("rule-metadata"); + expect((error as CLIError).code).toBe("RULE_ID_AMBIGUOUS"); + + // Detects, never renames: nothing here can tell which rule should keep the + // id, and the sidecar, the `.tests/` fixtures and the server-side id all + // reference the old name. + const valeStats = await stat(valePath); + const runtimeStats = await stat(runtimePath); + expect(valeStats.isDirectory()).toBe(true); + expect(runtimeStats.isDirectory()).toBe(true); + }); + + // The migration runs on `init`, and `check`/`verify` send a stale scaffold + // to `init`. If the refusal did not say what to do BEFORE re-running `init`, + // the two messages would form a loop. + it("tells the user to rename before re-running init", async () => { + await valeRule("no-eval"); + await runtimeRule("no-eval"); + + const error = (await migration(join(cwd, ".taskless")).catch( + (error_: unknown) => error_ + )) as CLIError; + expect(error.message).toMatch(/Rename one of those directories before/); + expect(error.message).toContain("init"); + expect(error.message).toContain("nothing is renamed for you"); + }); + + 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 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("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)); +} From 6e6d02851ff953d7aaf1f46ed52d527bc26ce545 Mon Sep 17 00:00:00 2001 From: Jakob Heuser Date: Tue, 22 Sep 2026 11:03:40 -0700 Subject: [PATCH 2/6] feat(migrate): rename colliding rule ids instead of refusing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Migration 9 refused a project holding one id under two engines. Refusing walls `init`, which is the command `SCAFFOLD_MIGRATION_REQUIRED` sends a stale scaffold to, so the CLI's own instruction became the thing that failed and a multi-file hand edit was the only way out. It now renames every colliding copy to `-`, symmetrically, so no engine keeps the bare id and nobody has to work out which of their two rules kept the name. A taken target takes the next free `-N`. The rename carries the rule file, its `id:`, sg fixtures and their `id:`, and the Vale breadcrumb and both `.` segments — all inside the rule's own directory. Every rename is printed. Safe to automate because the metadata sidecar is never written: the service does not return the `meta` block it comes from, and `rule meta` reports RULE_META_UNAVAILABLE saying so. It is left in place, unowned. The `verify` refusal stays: it is the guard for a collision created after the migration runs, by hand or by a merge. --- .changeset/rule-id-uniqueness.md | 10 +- .../2026-09-22-rule-id-uniqueness/proposal.md | 15 +- .../specs/cli-taskless-bootstrap/spec.md | 83 +++- .../2026-09-22-rule-id-uniqueness/tasks.md | 10 +- openspec/specs/cli-taskless-bootstrap/spec.md | 83 +++- .../migrations/0009-unique-rule-ids.ts | 379 ++++++++++++++++-- packages/cli/test/rule-id-uniqueness.test.ts | 229 +++++++++-- 7 files changed, 679 insertions(+), 130 deletions(-) diff --git a/.changeset/rule-id-uniqueness.md b/.changeset/rule-id-uniqueness.md index 17a60874..193cf470 100644 --- a/.changeset/rule-id-uniqueness.md +++ b/.changeset/rule-id-uniqueness.md @@ -2,10 +2,12 @@ "@taskless/cli": patch --- -`verify` now fails a rule whose id is also a directory name under another engine, and a new scaffold migration (`9`) refuses to migrate a project that already holds one. +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. -Two rules with the same id under two engines — `.taskless/rules/sg/no-eval/` beside `.taskless/rules/vale/no-eval/` — share one `.taskless/rule-metadata/no-eval.yml`, because the sidecar is keyed by id alone. The second `rule create` or `rule improve` overwrites the first's metadata silently, `rule meta ` returns the wrong rule's, and deleting either one takes the shared sidecar with it. None of that needed anyone to run `check`. +`.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. -**If your project already has a collision**, the first `taskless init` after upgrading will refuse, naming both rule directories. Rename one of them — the directory, the rule file inside it, the rule's own `id:` field where its engine has one, and the `rule-metadata/.yml` sidecar — then re-run `init`. Nothing is renamed for you on purpose: nothing in the CLI can tell which of the two rules should keep the id, and the sidecar, the rule's `.tests/` fixtures and the server-side id all reference the old name, so an automatic rename would break the references of whichever rule it moved. +**Your rule ids may change on upgrade, and `check` output changes with them.** The first `taskless init` after upgrading renames every colliding copy to `-` — `sg/no-eval` becomes `sg/no-eval-sg`, `vale/no-eval` becomes `vale/no-eval-vale`. The rename is symmetric on purpose: no engine keeps the bare id, so nobody has to work out which of their two rules silently kept the name. If `-` is already taken it uses the next free `--2`, `-3`, … and never overwrites an existing rule. -`writeRuleFile` still writes through a collision and only warns, so `check`'s repair path is unaffected. +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 — so you can see exactly what moved before committing it. Update any CI config, baseline file or suppression comment that names an old id. + +`.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/openspec/changes/archive/2026-09-22-rule-id-uniqueness/proposal.md b/openspec/changes/archive/2026-09-22-rule-id-uniqueness/proposal.md index c067da7b..6b07f2a4 100644 --- a/openspec/changes/archive/2026-09-22-rule-id-uniqueness/proposal.md +++ b/openspec/changes/archive/2026-09-22-rule-id-uniqueness/proposal.md @@ -2,7 +2,9 @@ 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 damage is not hypothetical and does not need anyone to run `check`. `writeRuleMetaFiles` keys `.taskless/rule-metadata/{id}.yml` on the id alone, so two colliding rules share one sidecar: the second `rule create` or `rule improve` overwrites the first's metadata silently, `rule meta ` returns the wrong rule's, and `deleteRuleFiles` removes the shared file for whichever rule is deleted first, leaving the survivor without one. Human `check` output cannot tell the two apart either, because `util/format.ts` prints `severity[ruleId]` with no engine. The JSON envelope is fine: every result carries `source`. +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 @@ -10,8 +12,11 @@ The damage is not hypothetical and does not need anyone to run `check`. `writeRu - **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` detects and refuses, and never renames.** Nothing there can tell which rule should keep the id, and a rename is not local: the sidecar, the rule's `.tests/` fixtures and the server-side id all reference the old name, so an automatic rename would pick one at random and break the references of whichever it moved. It is read-only in every case and idempotent. `LATEST_SCHEMA_VERSION` becomes 9 and this repository's own `.taskless/taskless.json` is bumped with it. -- **The refusal composes with `SCAFFOLD_MIGRATION_REQUIRED` rather than looping with it.** `check` and `verify` refuse a stale scaffold by naming `init`, and `init` is what runs migrations. A refusal that only reported the collision would send the user straight back to `init`, which would refuse again. It names the rename to perform _before_ re-running `init`, and reports `RULE_ID_AMBIGUOUS` so an agent that has learned what to do with `rules delete`'s refusal has learned what to do with this one. +- **Migration `9` renames every colliding copy to `-`** on upgrade, symmetrically: no engine keeps the bare id, because any precedence rule would be arbitrary and would leave a user working out which of their two rules silently kept the name. `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` in the directory alone, since `check.ts` is a fixed name and a capture's `id:` and `metadata.taskless.name` are its own. 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. @@ -28,13 +33,13 @@ 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. +- `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/migrations/0009-unique-rule-ids.ts` (new), registered in `migrate.ts`; `LATEST_SCHEMA_VERSION` becomes 9 and `.taskless/taskless.json` is migrated and committed. Its imports stay on filesystem primitives and the layout table: `rules/reconcile-marker` imports the migration runner back, and that cycle leaves `migrations["9"]` undefined on any graph entered through `rules/files.ts`. - 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-taskless-bootstrap/spec.md b/openspec/changes/archive/2026-09-22-rule-id-uniqueness/specs/cli-taskless-bootstrap/spec.md index e8980cef..2b923672 100644 --- 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 @@ -1,37 +1,82 @@ ## ADDED Requirements -### Requirement: Migration 9 refuses a project whose rule id is held by more than one engine +### Requirement: Migration 9 renames a rule id held by more than one engine -Migration `9` SHALL read `.taskless/rules/` and SHALL refuse the migration when any rule id is a directory name under more than one engine, naming every directory holding the id and the `.taskless/rule-metadata/.yml` sidecar they share. It SHALL report the same error code `rules delete` reports for the same condition, `RULE_ID_AMBIGUOUS`. +Migration `9` SHALL read `.taskless/rules/` and, for every rule id that is a directory name under more than one engine, SHALL rename EVERY holding copy to `-`. No engine SHALL keep the bare id. Any precedence rule 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 NOT rename anything. Nothing available to a migration can tell which of the two rules should keep the id, and a rename is not local: the sidecar, the rule's `.tests/` fixtures and the server-side id all reference the old name, so an automatic rename would pick one at random and break the references of whichever it moved. +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 refusal SHALL state the rename to perform BEFORE `init` is re-run. `check` and `verify` refuse a scaffold behind the current version with `SCAFFOLD_MIGRATION_REQUIRED`, which names `init`, and `init` is what runs migrations; a refusal that only reported the collision would return the user to `init` and be refused again. +The rename SHALL carry every reference to the id inside the rule's own directory, and SHALL reach nothing outside it: -The migration SHALL write nothing in any case. A project with no collision 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. +| 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 | -#### Scenario: A colliding project is refused by name +Both Vale segments move because `StylesPath` points at `rules/vale`, so the rule directory is the style and `.yml` is the check inside it. A runtime rule carries the id in its directory alone: `check.ts` is a fixed name, and a capture file's `id:` and `metadata.taskless.name` identify the capture rather than the rule. -- **WHEN** `no-eval` exists under two engines and migration 9 runs -- **THEN** the migration SHALL throw -- **AND** the message SHALL name both rule directories and the shared metadata sidecar -- **AND** the error code SHALL be `RULE_ID_AMBIGUOUS` +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. -#### Scenario: The refusal names the rename to perform before init +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. -- **WHEN** migration 9 refuses a colliding project -- **THEN** the message SHALL say to rename one of the directories before re-running `init` -- **AND** it SHALL say that nothing is renamed automatically +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. -#### Scenario: Nothing is renamed +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. -- **WHEN** migration 9 refuses a colliding project -- **THEN** both rule directories SHALL remain where they were +#### Scenario: Every colliding copy is 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: 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 runtime rule is renamed by directory alone + +- **WHEN** migration 9 renames a colliding `runtime` rule +- **THEN** the directory SHALL be renamed +- **AND** `check.ts` and the capture files SHALL be left as they are + +#### 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 twice over a project with no collision -- **THEN** neither run SHALL write anything +- **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 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 index c44c2766..5b10f4d4 100644 --- a/openspec/changes/archive/2026-09-22-rule-id-uniqueness/tasks.md +++ b/openspec/changes/archive/2026-09-22-rule-id-uniqueness/tasks.md @@ -6,14 +6,16 @@ ## 2. The migration -- [x] 2.1 Add `filesystem/migrations/0009-unique-rule-ids.ts`: read-only, throws a `CLIError` coded `RULE_ID_AMBIGUOUS` naming every holding directory, the shared sidecar, and the rename to perform before re-running `init`. Register `"9"` in `migrate.ts`. -- [x] 2.2 Trace the refusal against `SCAFFOLD_MIGRATION_REQUIRED` and confirm the two messages form a path out rather than a loop. -- [x] 2.3 Migrate this repository's own `.taskless/` with `pnpm build && pnpm cli init` and commit the rewritten manifest. +- [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. Next free `-N` suffix when the target is taken, sidecar left in place, every rename printed. Register `"9"` in `migrate.ts`. +- [x] 2.3 Keep the migration's imports to filesystem primitives and the layout table: `rules/reconcile-marker` imports the migration runner back, and the cycle left `migrations["9"]` undefined on any graph entered through `rules/files.ts`. +- [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 refuses a colliding project naming both directories, says to rename before re-running `init`, is a no-op and writes nothing on a clean project across two runs, and is a no-op with no rules tree. +- [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 moves by directory alone; 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 diff --git a/openspec/specs/cli-taskless-bootstrap/spec.md b/openspec/specs/cli-taskless-bootstrap/spec.md index 158b968b..59b39ea0 100644 --- a/openspec/specs/cli-taskless-bootstrap/spec.md +++ b/openspec/specs/cli-taskless-bootstrap/spec.md @@ -270,38 +270,83 @@ 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 refuses a project whose rule id is held by more than one engine +### Requirement: Migration 9 renames a rule id held by more than one engine -Migration `9` SHALL read `.taskless/rules/` and SHALL refuse the migration when any rule id is a directory name under more than one engine, naming every directory holding the id and the `.taskless/rule-metadata/.yml` sidecar they share. It SHALL report the same error code `rules delete` reports for the same condition, `RULE_ID_AMBIGUOUS`. +Migration `9` SHALL read `.taskless/rules/` and, for every rule id that is a directory name under more than one engine, SHALL rename EVERY holding copy to `-`. No engine SHALL keep the bare id. Any precedence rule 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 NOT rename anything. Nothing available to a migration can tell which of the two rules should keep the id, and a rename is not local: the sidecar, the rule's `.tests/` fixtures and the server-side id all reference the old name, so an automatic rename would pick one at random and break the references of whichever it moved. +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 refusal SHALL state the rename to perform BEFORE `init` is re-run. `check` and `verify` refuse a scaffold behind the current version with `SCAFFOLD_MIGRATION_REQUIRED`, which names `init`, and `init` is what runs migrations; a refusal that only reported the collision would return the user to `init` and be refused again. +The rename SHALL carry every reference to the id inside the rule's own directory, and SHALL reach nothing outside it: -The migration SHALL write nothing in any case. A project with no collision 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. +| 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 | -#### Scenario: A colliding project is refused by name +Both Vale segments move because `StylesPath` points at `rules/vale`, so the rule directory is the style and `.yml` is the check inside it. A runtime rule carries the id in its directory alone: `check.ts` is a fixed name, and a capture file's `id:` and `metadata.taskless.name` identify the capture rather than the rule. -- **WHEN** `no-eval` exists under two engines and migration 9 runs -- **THEN** the migration SHALL throw -- **AND** the message SHALL name both rule directories and the shared metadata sidecar -- **AND** the error code SHALL be `RULE_ID_AMBIGUOUS` +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. -#### Scenario: The refusal names the rename to perform before init +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. -- **WHEN** migration 9 refuses a colliding project -- **THEN** the message SHALL say to rename one of the directories before re-running `init` -- **AND** it SHALL say that nothing is renamed automatically +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. -#### Scenario: Nothing is renamed +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. -- **WHEN** migration 9 refuses a colliding project -- **THEN** both rule directories SHALL remain where they were +#### Scenario: Every colliding copy is 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: 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 runtime rule is renamed by directory alone + +- **WHEN** migration 9 renames a colliding `runtime` rule +- **THEN** the directory SHALL be renamed +- **AND** `check.ts` and the capture files SHALL be left as they are + +#### 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 twice over a project with no collision -- **THEN** neither run SHALL write anything +- **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 diff --git a/packages/cli/src/filesystem/migrations/0009-unique-rule-ids.ts b/packages/cli/src/filesystem/migrations/0009-unique-rule-ids.ts index 96b49c18..cc2e4523 100644 --- a/packages/cli/src/filesystem/migrations/0009-unique-rule-ids.ts +++ b/packages/cli/src/filesystem/migrations/0009-unique-rule-ids.ts @@ -1,42 +1,76 @@ +import { readdir, readFile, rename, stat, writeFile } from "node:fs/promises"; import { join } from "node:path"; import { - describeRuleIdCollision, findRuleIdCollisions, metadataSidecarPath, - type RuleIdCollision, } from "../../rules/id-uniqueness"; -import { CLIError } from "../../util/cli-error"; -import { buildInvocation } from "../../util/invocation"; +import { ruleDirectory } from "../../rules/engines"; +import { + ENGINES, + RULE_TESTS_DIRECTORY, + type EngineName, +} from "../../rules/layout"; +import { isValidRuleId } from "../../rules/validate-id"; import type { Migration } from "../types"; /** - * Refuse a project where one rule id is held by more than one engine. - * - * `verify` now fails such a rule, but `verify` only reaches rules someone - * runs it on. A project that already holds `sg/no-eval` beside `vale/no-eval` - * would keep sharing one `rule-metadata/no-eval.yml` until someone happened to - * look, and the sidecar is overwritten by whichever rule is written last, with - * nothing reporting it. The upgrade is the one moment every existing project - * passes through, so this is where they are all checked. - * - * DETECTS AND REFUSES. IT MUST NEVER RENAME. Nothing here can tell which of - * the two rules should keep the id, and a rename is not local: the - * `rule-metadata/{id}.yml` sidecar, the rule's `.tests/` fixtures and the - * server-side id all reference the old name. An automatic rename would pick - * one at random and break the references of whichever it moved. - * - * The message has to do more work than a migration's usually does, because of - * where the user is standing when they read it. `check` and `verify` refuse a - * stale scaffold with `SCAFFOLD_MIGRATION_REQUIRED`, which says to run `init`; - * `init` is what runs this migration. So a refusal that only said "there is a - * collision" would send the user back to `init`, which would refuse again. It - * names the rename to perform BEFORE re-running `init`, which is what turns - * the pair of messages into a path out rather than a loop. - * - * Idempotent, and read-only in every case. A project with no collision is - * enumerated and nothing is written, so a second run touches nothing and - * `git status` stays clean. + * 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` | directory only — `check.ts` is a fixed name, and a capture's `id:` and `metadata.taskless.name` are its own, not the rule's | + * + * 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. + * + * ## The rename is symmetric + * + * Every colliding copy becomes `-`; no engine keeps the bare id. + * Any precedence rule 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. + * + * ## 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. */ const migration: Migration = async (directory) => { // The collision is a fact about `.taskless/rules/`, and every helper that @@ -44,24 +78,281 @@ const migration: Migration = async (directory) => { const cwd = join(directory, ".."); const collisions = await findRuleIdCollisions(cwd); if (collisions.length === 0) return; - throw new CLIError( - collisions.map((collision) => refusal(cwd, collision)).join("\n\n"), - // The same code `rules delete` reports for the same condition. An agent - // that has learned what to do with one has learned what to do with both. - "RULE_ID_AMBIGUOUS" + + const taken = await occupiedRuleIds(cwd); + const lines: string[] = []; + for (const collision of collisions) { + for (const engine of collision.engines) { + 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") ); }; -/** One collision, and the rename that clears it. */ -function refusal(cwd: string, collision: RuleIdCollision): string { - return ( - `${describeRuleIdCollision(cwd, collision)}\n\n` + - `Rename one of those directories before re-running ` + - `\`${buildInvocation()} init\`, and rename with it: the rule file inside ` + - `it, the rule's own \`id:\` field where its engine has one, and the ` + - `sidecar at ${metadataSidecarPath(cwd, collision.ruleId)}. Nothing here ` + - `can tell which rule should keep the id, so nothing is renamed for you.` +/** + * 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 listEngineRuleIds(cwd, engine)) ids.add(id); + } + return ids; +} + +async function listEngineRuleIds( + cwd: string, + engine: EngineName +): Promise { + try { + const entries = await readdir(join(cwd, ".taskless", "rules", engine), { + withFileTypes: true, + }); + return entries + .filter((entry) => entry.isDirectory()) + .map((entry) => entry.name); + } catch { + return []; + } +} + +/** + * `-`, 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, from, to)), + ...(await renameSgFixtures(toPath, from, to)) + ); + } else if (engine === "vale") { + lines.push( + ...(await renameRuleFile(toPath, from, to)), + ...(await rewriteValeConfig(toPath, from, to)) + ); + } + // `runtime` carries the id in the directory name alone: `check.ts` is a + // fixed name, and a capture file's `id:` and `metadata.taskless.name` are + // the capture's own identifiers, not the rule's. + return lines; +} + +/** `.yml` becomes `.yml`, and its own `id:` follows. */ +async function renameRuleFile( + ruleDirectoryPath: string, + from: string, + to: string +): Promise { + const fromFile = join(ruleDirectoryPath, `${from}.yml`); + if (!(await pathExists(fromFile))) return []; + const toFile = join(ruleDirectoryPath, `${to}.yml`); + await rename(fromFile, toFile); + const rewritten = await rewriteIdField(toFile, from, to); + return [ + ` renamed ${from}.yml -> ${to}.yml${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`\$&`); +} + +/** + * Whether `path` exists. + * + * Local rather than `rules/reconcile-marker`'s copy, and that is load-bearing: + * `reconcile-marker` imports `readManifest`/`writeManifest` from `migrate.ts`, + * which imports this migration, and the cycle left `migrations["9"]` holding + * `undefined` whenever the graph was entered through `rules/files.ts` — + * "migrate is not a function", in the middle of a rule write. A migration + * should reach for filesystem primitives and the layout table, nothing that + * can import the runner back. + */ +async function pathExists(path: string): Promise { + try { + await stat(path); + return true; + } catch { + return false; + } } export default migration; diff --git a/packages/cli/test/rule-id-uniqueness.test.ts b/packages/cli/test/rule-id-uniqueness.test.ts index 4aa21d0a..64807df1 100644 --- a/packages/cli/test/rule-id-uniqueness.test.ts +++ b/packages/cli/test/rule-id-uniqueness.test.ts @@ -5,11 +5,13 @@ import { join } from "node:path"; import { afterEach, beforeEach, describe, expect, it } from "vitest"; -import migration from "../src/filesystem/migrations/0009-unique-rule-ids"; +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 { CLIError } from "../src/util/cli-error"; +import type { EngineName } from "../src/rules/layout"; /** * A rule id is a directory name under `.taskless/rules//`, and nothing @@ -39,6 +41,37 @@ async function valeRule(id: string): Promise { 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 }); @@ -113,44 +146,129 @@ describe("verify refuses a rule id held by more than one engine", () => { }); }); -describe("migration 0009 refuses a colliding project", () => { - it("throws naming both directories and the rename to perform", async () => { - const valePath = await valeRule("no-eval"); - const runtimePath = await runtimeRule("no-eval"); +describe("migration 0009 renames a colliding project", () => { + // Symmetric: neither engine keeps the bare id, because any precedence rule + // would be arbitrary and would leave a user working out which of their two + // rules silently kept the name. + it("renames every colliding copy to -", async () => { + await sgRule("no-eval"); + await valeRule("no-eval"); - const error = await migration(join(cwd, ".taskless")).then( - () => {}, - (error_: unknown) => error_ + 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" ); - expect(error).toBeInstanceOf(CLIError); - const message = (error as CLIError).message; - expect(message).toContain(valePath); - expect(message).toContain(runtimePath); - expect(message).toContain("rule-metadata"); - expect((error as CLIError).code).toBe("RULE_ID_AMBIGUOUS"); - - // Detects, never renames: nothing here can tell which rule should keep the - // id, and the sidecar, the `.tests/` fixtures and the server-side id all - // reference the old name. - const valeStats = await stat(valePath); - const runtimeStats = await stat(runtimePath); - expect(valeStats.isDirectory()).toBe(true); - expect(runtimeStats.isDirectory()).toBe(true); - }); - - // The migration runs on `init`, and `check`/`verify` send a stale scaffold - // to `init`. If the refusal did not say what to do BEFORE re-running `init`, - // the two messages would form a loop. - it("tells the user to rename before re-running init", async () => { + // 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"); + }); + + it("renames a runtime rule by directory alone", async () => { await runtimeRule("no-eval"); + await valeRule("no-eval"); - const error = (await migration(join(cwd, ".taskless")).catch( - (error_: unknown) => error_ - )) as CLIError; - expect(error.message).toMatch(/Rename one of those directories before/); - expect(error.message).toContain("init"); - expect(error.message).toContain("nothing is renamed for you"); + await migration(join(cwd, ".taskless")); + + const directory = rulePath("runtime", "no-eval-runtime"); + expect(await exists(join(directory, "check.ts"))).toBe(true); + }); + + // 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 () => { @@ -164,12 +282,53 @@ describe("migration 0009 refuses a colliding project", () => { 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 From ce2f16e8b05dd2b8bbb21b2b6cba475b0b5d47eb Mon Sep 17 00:00:00 2001 From: Jakob Heuser Date: Tue, 22 Sep 2026 13:06:49 -0700 Subject: [PATCH 3/6] refactor(filesystem): split the manifest out of migrate.ts MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `migrate.ts` did two unrelated jobs: the taskless.json manifest, and the migration registry and runner. Because they shared a module, importing "read the manifest" also loaded every migration — so a migration that reached for anything reading the manifest closed a loop through the runner. That is what left `migrations["9"]` holding `undefined`, and the local `pathExists` in 0009 treated the symptom rather than the cause. The manifest moves to `filesystem/manifest.ts`, which imports nothing from `migrate.ts`. All six importers point at it directly; nothing is re-exported for compatibility, per the styleguide. `readRawManifest` and `writeRawManifest` become exported because the runner reads and stamps the raw version. 0009 now imports the shared `pathExists` from `rules/reconcile-marker`, which is the proof the loop is gone. No behavior change. Nothing published exposes either module. --- .../2026-09-22-rule-id-uniqueness/proposal.md | 3 +- .../2026-09-22-rule-id-uniqueness/tasks.md | 5 +- packages/cli/src/commands/info.ts | 2 +- packages/cli/src/commands/init.ts | 2 +- packages/cli/src/commands/onboard.ts | 2 +- packages/cli/src/filesystem/manifest.ts | 225 ++++++++++++++++++ packages/cli/src/filesystem/migrate.ts | 203 +--------------- .../migrations/0009-unique-rule-ids.ts | 28 +-- packages/cli/src/install/state.ts | 4 +- packages/cli/src/rules/reconcile-marker.ts | 2 +- packages/cli/test/migrate-install.test.ts | 7 +- packages/cli/test/migration-registry.test.ts | 88 +++++++ 12 files changed, 334 insertions(+), 237 deletions(-) create mode 100644 packages/cli/src/filesystem/manifest.ts create mode 100644 packages/cli/test/migration-registry.test.ts 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 index 6b07f2a4..4ebba17b 100644 --- a/openspec/changes/archive/2026-09-22-rule-id-uniqueness/proposal.md +++ b/openspec/changes/archive/2026-09-22-rule-id-uniqueness/proposal.md @@ -40,6 +40,7 @@ None. - `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. Its imports stay on filesystem primitives and the layout table: `rules/reconcile-marker` imports the migration runner back, and that cycle leaves `migrations["9"]` undefined on any graph entered through `rules/files.ts`. +- `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/tasks.md b/openspec/changes/archive/2026-09-22-rule-id-uniqueness/tasks.md index 5b10f4d4..e08cf9c7 100644 --- a/openspec/changes/archive/2026-09-22-rule-id-uniqueness/tasks.md +++ b/openspec/changes/archive/2026-09-22-rule-id-uniqueness/tasks.md @@ -8,7 +8,7 @@ - [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. Next free `-N` suffix when the target is taken, sidecar left in place, every rename printed. Register `"9"` in `migrate.ts`. -- [x] 2.3 Keep the migration's imports to filesystem primitives and the layout table: `rules/reconcile-marker` imports the migration runner back, and the cycle left `migrations["9"]` undefined on any graph entered through `rules/files.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. @@ -21,4 +21,5 @@ ## 4. Release - [x] 4.1 `.changeset/rule-id-uniqueness.md`, `patch`, saying what a user holding an existing collision must do. -- [x] 4.2 `pnpm typecheck`, `pnpm lint`, `pnpm --filter @taskless/cli test`. +- [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/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 4922c0b7..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"; @@ -17,61 +17,6 @@ import ignoreScratchFiles from "./migrations/0007-ignore-scratch-files"; import dropBasedOnStyles from "./migrations/0008-drop-based-on-styles"; import uniqueRuleIds from "./migrations/0009-unique-rule-ids"; -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"; - const migrations: Migrations = { "1": init, "2": installMigration, @@ -108,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 index cc2e4523..d5068a11 100644 --- a/packages/cli/src/filesystem/migrations/0009-unique-rule-ids.ts +++ b/packages/cli/src/filesystem/migrations/0009-unique-rule-ids.ts @@ -1,4 +1,4 @@ -import { readdir, readFile, rename, stat, writeFile } from "node:fs/promises"; +import { readdir, readFile, rename, writeFile } from "node:fs/promises"; import { join } from "node:path"; import { @@ -6,6 +6,12 @@ import { metadataSidecarPath, } from "../../rules/id-uniqueness"; import { 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 { ENGINES, RULE_TESTS_DIRECTORY, @@ -335,24 +341,4 @@ function escapeForRegExp(value: string): string { return value.replaceAll(/[$()*+.?[\\\]^{|}]/g, String.raw`\$&`); } -/** - * Whether `path` exists. - * - * Local rather than `rules/reconcile-marker`'s copy, and that is load-bearing: - * `reconcile-marker` imports `readManifest`/`writeManifest` from `migrate.ts`, - * which imports this migration, and the cycle left `migrations["9"]` holding - * `undefined` whenever the graph was entered through `rules/files.ts` — - * "migrate is not a function", in the middle of a rule write. A migration - * should reach for filesystem primitives and the layout table, nothing that - * can import the runner back. - */ -async function pathExists(path: string): Promise { - try { - await stat(path); - return true; - } catch { - return false; - } -} - 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/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/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) + ); + }); +}); From 3f79b917fcfee7c927f59fb45e2ac5f62e1cdeed Mon Sep 17 00:00:00 2001 From: Jakob Heuser Date: Tue, 22 Sep 2026 13:29:45 -0700 Subject: [PATCH 4/6] feat(migrate): never rename a runtime rule Migration 9 renamed every colliding copy. It now moves only the sg and vale copies; a runtime rule holding a colliding id keeps the bare id. Runtime is the signed and blessed tier, and leaving it untouched keeps this 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 have been safe anyway -- signRuleFile hashes the content of check.ts and never its path -- so this is a precaution, not a correctness fix. The report names the 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. --- .changeset/rule-id-uniqueness.md | 6 +- .../2026-09-22-rule-id-uniqueness/proposal.md | 5 +- .../specs/cli-taskless-bootstrap/spec.md | 32 ++++++--- .../2026-09-22-rule-id-uniqueness/tasks.md | 4 +- openspec/specs/cli-taskless-bootstrap/spec.md | 32 ++++++--- .../migrations/0009-unique-rule-ids.ts | 52 +++++++++++--- packages/cli/test/rule-id-uniqueness.test.ts | 68 +++++++++++++++++-- 7 files changed, 157 insertions(+), 42 deletions(-) diff --git a/.changeset/rule-id-uniqueness.md b/.changeset/rule-id-uniqueness.md index 193cf470..afa18721 100644 --- a/.changeset/rule-id-uniqueness.md +++ b/.changeset/rule-id-uniqueness.md @@ -6,8 +6,10 @@ Two rules can no longer share an id across engines. `verify` fails a rule whose `.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 every colliding copy to `-` — `sg/no-eval` becomes `sg/no-eval-sg`, `vale/no-eval` becomes `vale/no-eval-vale`. The rename is symmetric on purpose: no engine keeps the bare id, so nobody has to work out which of their two rules silently kept the name. If `-` is already taken it uses the next free `--2`, `-3`, … and never overwrites an existing rule. +**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. -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 — so you can see exactly what moved before committing it. Update any CI config, baseline file or suppression comment that names an old id. +**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. `.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/openspec/changes/archive/2026-09-22-rule-id-uniqueness/proposal.md b/openspec/changes/archive/2026-09-22-rule-id-uniqueness/proposal.md index 4ebba17b..0193a15b 100644 --- a/openspec/changes/archive/2026-09-22-rule-id-uniqueness/proposal.md +++ b/openspec/changes/archive/2026-09-22-rule-id-uniqueness/proposal.md @@ -12,9 +12,10 @@ The metadata-sidecar clobber that #387 leads with is LATENT rather than live, an - **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 every colliding copy to `-`** on upgrade, symmetrically: no engine keeps the bare id, because any precedence rule would be arbitrary and would leave a user working out which of their two rules silently kept the name. `LATEST_SCHEMA_VERSION` becomes 9 and this repository's own `.taskless/taskless.json` is bumped with it. +- **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` in the directory alone, since `check.ts` is a fixed name and a capture's `id:` and `metadata.taskless.name` are its own. 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. +- **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. 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 index 2b923672..207915ad 100644 --- 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 @@ -2,7 +2,11 @@ ### 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 EVERY holding copy to `-`. No engine SHALL keep the bare id. Any precedence rule would be arbitrary, and a symmetric rename means no user has to work out which of their two rules silently kept the name. +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. @@ -14,23 +18,37 @@ The rename SHALL carry every reference to the id inside the rule's own directory | `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. A runtime rule carries the id in its directory alone: `check.ts` is a fixed name, and a capture file's `id:` and `metadata.taskless.name` identify the capture rather than the rule. +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 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: Every colliding copy is renamed symmetrically +#### 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 @@ -44,12 +62,6 @@ It SHALL write nothing when there is no collision. A project in that state SHALL - **AND** the `.vale.ini` breadcrumb SHALL name the new id - **AND** the `.` assignment SHALL become `.` -#### Scenario: A runtime rule is renamed by directory alone - -- **WHEN** migration 9 renames a colliding `runtime` rule -- **THEN** the directory SHALL be renamed -- **AND** `check.ts` and the capture files SHALL be left as they are - #### Scenario: A taken target name takes the next free suffix - **WHEN** `-` is already held by some engine 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 index e08cf9c7..e1a4f3c8 100644 --- a/openspec/changes/archive/2026-09-22-rule-id-uniqueness/tasks.md +++ b/openspec/changes/archive/2026-09-22-rule-id-uniqueness/tasks.md @@ -7,7 +7,7 @@ ## 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. Next free `-N` suffix when the target is taken, sidecar left in place, every rename printed. Register `"9"` in `migrate.ts`. +- [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. @@ -15,7 +15,7 @@ ## 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 moves by directory alone; 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.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 diff --git a/openspec/specs/cli-taskless-bootstrap/spec.md b/openspec/specs/cli-taskless-bootstrap/spec.md index 59b39ea0..8628a572 100644 --- a/openspec/specs/cli-taskless-bootstrap/spec.md +++ b/openspec/specs/cli-taskless-bootstrap/spec.md @@ -272,7 +272,11 @@ The migration exists because the `create-vale-rule` recipe wrote `BasedOnStyles ### 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 EVERY holding copy to `-`. No engine SHALL keep the bare id. Any precedence rule would be arbitrary, and a symmetric rename means no user has to work out which of their two rules silently kept the name. +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. @@ -284,23 +288,37 @@ The rename SHALL carry every reference to the id inside the rule's own directory | `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. A runtime rule carries the id in its directory alone: `check.ts` is a fixed name, and a capture file's `id:` and `metadata.taskless.name` identify the capture rather than the rule. +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 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: Every colliding copy is renamed symmetrically +#### 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 @@ -314,12 +332,6 @@ It SHALL write nothing when there is no collision. A project in that state SHALL - **AND** the `.vale.ini` breadcrumb SHALL name the new id - **AND** the `.` assignment SHALL become `.` -#### Scenario: A runtime rule is renamed by directory alone - -- **WHEN** migration 9 renames a colliding `runtime` rule -- **THEN** the directory SHALL be renamed -- **AND** `check.ts` and the capture files SHALL be left as they are - #### Scenario: A taken target name takes the next free suffix - **WHEN** `-` is already held by some engine diff --git a/packages/cli/src/filesystem/migrations/0009-unique-rule-ids.ts b/packages/cli/src/filesystem/migrations/0009-unique-rule-ids.ts index d5068a11..bf4da43c 100644 --- a/packages/cli/src/filesystem/migrations/0009-unique-rule-ids.ts +++ b/packages/cli/src/filesystem/migrations/0009-unique-rule-ids.ts @@ -45,7 +45,7 @@ import type { Migration } from "../types"; * | --------- | -------------------------------------------------------------------------- | * | `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` | directory only — `check.ts` is a fixed name, and a capture's `id:` and `metadata.taskless.name` are its own, not the rule's | + * | `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 @@ -53,12 +53,29 @@ import type { Migration } from "../types"; * records versions rather than rules, and the runtime reconcile join is by * content signature, so a moved-but-unchanged rule still resolves. * - * ## The rename is symmetric + * ## Runtime rules are never renamed * - * Every colliding copy becomes `-`; no engine keeps the bare id. - * Any precedence rule 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. + * 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 * @@ -78,6 +95,14 @@ import type { Migration } from "../types"; * 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. @@ -89,6 +114,17 @@ const migration: Migration = async (directory) => { 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))); @@ -191,9 +227,7 @@ async function renameRule( ...(await rewriteValeConfig(toPath, from, to)) ); } - // `runtime` carries the id in the directory name alone: `check.ts` is a - // fixed name, and a capture file's `id:` and `metadata.taskless.name` are - // the capture's own identifiers, not the rule's. + // No `runtime` branch: this is never called for one. See `NEVER_RENAMED`. return lines; } diff --git a/packages/cli/test/rule-id-uniqueness.test.ts b/packages/cli/test/rule-id-uniqueness.test.ts index 64807df1..a84d64d1 100644 --- a/packages/cli/test/rule-id-uniqueness.test.ts +++ b/packages/cli/test/rule-id-uniqueness.test.ts @@ -147,10 +147,10 @@ describe("verify refuses a rule id held by more than one engine", () => { }); describe("migration 0009 renames a colliding project", () => { - // Symmetric: neither engine keeps the bare id, because any precedence rule - // would be arbitrary and would leave a user working out which of their two - // rules silently kept the name. - it("renames every colliding copy to -", async () => { + // 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"); @@ -196,14 +196,68 @@ describe("migration 0009 renames a colliding project", () => { expect(config).not.toContain("no-eval.no-eval"); }); - it("renames a runtime rule by directory alone", async () => { + // 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")); - const directory = rulePath("runtime", "no-eval-runtime"); - expect(await exists(join(directory, "check.ts"))).toBe(true); + 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 From 6d3d4dcd99231bc4aab7cc792518b4652a5f54f3 Mon Sep 17 00:00:00 2001 From: Jakob Heuser Date: Tue, 22 Sep 2026 13:51:09 -0700 Subject: [PATCH 5/6] refactor(migrate): take the rule-id enumeration and file name from layout `occupiedRuleIds` re-read the engine directories itself, with `.taskless` and `rules` written out as string literals, and `renameRuleFile` rebuilt the rule file name as `${id}.yml`. Both facts already live somewhere: `listRuleIds` in `rules/engines.ts` derives the path from `TASKLESS_DIRECTORY`/`RULES_DIRECTORY`, and `ENGINE_LAYOUTS[engine].ruleFile` is the table that decides what a rule file is called. `ruleFilePath` is deliberately NOT used for the second one: it resolves from a `cwd` and a rule id into the rule's own directory, and by the time `renameRuleFile` runs that directory has already moved. Only the file inside it still carries the old name. No new module edges: the migration already imported `ruleDirectory` from `rules/engines` and `ENGINES` from `rules/layout`, so the import graph the manifest split repaired is untouched. --- .../migrations/0009-unique-rule-ids.ts | 46 +++++++++---------- 1 file changed, 22 insertions(+), 24 deletions(-) diff --git a/packages/cli/src/filesystem/migrations/0009-unique-rule-ids.ts b/packages/cli/src/filesystem/migrations/0009-unique-rule-ids.ts index bf4da43c..932905d5 100644 --- a/packages/cli/src/filesystem/migrations/0009-unique-rule-ids.ts +++ b/packages/cli/src/filesystem/migrations/0009-unique-rule-ids.ts @@ -5,7 +5,7 @@ import { findRuleIdCollisions, metadataSidecarPath, } from "../../rules/id-uniqueness"; -import { ruleDirectory } from "../../rules/engines"; +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 @@ -13,6 +13,7 @@ import { ruleDirectory } from "../../rules/engines"; // knows migrations exist, so the path stops here. import { pathExists } from "../../rules/reconcile-marker"; import { + ENGINE_LAYOUTS, ENGINES, RULE_TESTS_DIRECTORY, type EngineName, @@ -155,27 +156,11 @@ const migration: Migration = async (directory) => { async function occupiedRuleIds(cwd: string): Promise> { const ids = new Set(); for (const engine of ENGINES) { - for (const id of await listEngineRuleIds(cwd, engine)) ids.add(id); + for (const id of await listRuleIds(cwd, engine)) ids.add(id); } return ids; } -async function listEngineRuleIds( - cwd: string, - engine: EngineName -): Promise { - try { - const entries = await readdir(join(cwd, ".taskless", "rules", engine), { - withFileTypes: true, - }); - return entries - .filter((entry) => entry.isDirectory()) - .map((entry) => entry.name); - } catch { - return []; - } -} - /** * `-`, or the first free `--N` when that is taken. * @@ -218,12 +203,12 @@ async function renameRule( if (engine === "sg") { lines.push( - ...(await renameRuleFile(toPath, from, to)), + ...(await renameRuleFile(toPath, engine, from, to)), ...(await renameSgFixtures(toPath, from, to)) ); } else if (engine === "vale") { lines.push( - ...(await renameRuleFile(toPath, from, to)), + ...(await renameRuleFile(toPath, engine, from, to)), ...(await rewriteValeConfig(toPath, from, to)) ); } @@ -231,19 +216,32 @@ async function renameRule( return lines; } -/** `.yml` becomes `.yml`, and its own `id:` follows. */ +/** + * 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 fromFile = join(ruleDirectoryPath, `${from}.yml`); + 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, `${to}.yml`); + const toFile = join(ruleDirectoryPath, toName); await rename(fromFile, toFile); const rewritten = await rewriteIdField(toFile, from, to); return [ - ` renamed ${from}.yml -> ${to}.yml${rewritten ? " and its id: field" : ""}`, + ` renamed ${fromName} -> ${toName}${rewritten ? " and its id: field" : ""}`, ]; } From 47a4077fce7da87d8c13eec0ecb17d05add8ba23 Mon Sep 17 00:00:00 2001 From: Jakob Heuser Date: Tue, 22 Sep 2026 14:21:48 -0700 Subject: [PATCH 6/6] test(check): build the cross-engine collision after migration 9 runs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `check --rule ` selects every engine holding the id, and #385's test proved it by seeding `vale/no-eval` beside the fixture's `sg/no-eval`. Migration 9 now renames exactly that state, and `runCli` migrates on every invocation through `migrateFixture`, so the collision was renamed to `no-eval-sg`/`no-eval-vale` before `check` ever saw it: `--rule no-eval` exited `RULE_NOT_FOUND` and the test died reading `.map` of an undefined `results`. The migration invalidated the setup, not the behaviour. An id held by two engines still selects both, and a project can still reach that state — by hand, or by a merge landing a same-id rule under another engine — which is the case the new per-rule check in `verify` exists to catch. So the fixture is migrated first and the second engine's copy seeded after, with a comment naming migration 9 so the setup is not "simplified" back. Also names the consequence in the changeset: an id passed to `--rule` yesterday may not exist today, and that failure is `RULE_NOT_FOUND` rather than a quiet zero findings. --- .changeset/rule-id-uniqueness.md | 2 +- packages/cli/test/check-rule-filter.test.ts | 19 +++++++++++++++++++ 2 files changed, 20 insertions(+), 1 deletion(-) diff --git a/.changeset/rule-id-uniqueness.md b/.changeset/rule-id-uniqueness.md index afa18721..f4e329cf 100644 --- a/.changeset/rule-id-uniqueness.md +++ b/.changeset/rule-id-uniqueness.md @@ -10,6 +10,6 @@ Two rules can no longer share an id across engines. `verify` fails a rule whose **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. +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/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(