feat(verify): refuse a rule id held by more than one engine

[thecodedrift]

What

Nothing kept .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 mkdirs inside it; the read path asks listRuleIds per engine and never diffs the answers. check's human output prints severity[ruleId] with no engine, so a collision shows two identical error[no-eval] lines.

• verify fails a rule whose id is held by more than one engine, naming every holding directory. The check is per rule, not only project-wide: verifying the rule an author just wrote is the moment the collision is cheapest to fix. test inherits it, reached through the same helper rather than a second verifySgRule call.
• Migration 9 renames the colliding sg and vale copies to <id>-<engine> on upgrade — sg/no-eval → sg/no-eval-sg, vale/no-eval → vale/no-eval-vale. A runtime copy is never renamed and keeps the bare id.
• writeRuleFile warns and still writes. check's repair path calls it, so a refusal would brick repair for both colliding rules.
• LATEST_SCHEMA_VERSION becomes 9; this repo's own .taskless/taskless.json is bumped with it.

Why the migration renames rather than refuses

The first cut of this refused. Refusing walls init, and init is the command every other refusal points at: check and verify send a stale scaffold there with SCAFFOLD_MIGRATION_REQUIRED. That leaves the CLI's own instruction as the thing that fails, with a multi-file hand edit as the only way out.

Renaming is safe to automate here, and both halves of that were verified rather than assumed:

The metadata sidecar is never written. It comes from the meta block of a rule status response; the service does not populate it. Both call sites in commands/rules.ts carry a comment saying so ("Dead in practice, and kept deliberately"), rule meta's own description reads "no sidecar is written by this version" and it reports RULE_META_UNAVAILABLE rather than RULE_NOT_FOUND precisely because no id produces one, and .taskless/rule-metadata/ does not exist in this repository. The clobber #387 leads with is latent, so there is no metadata for a rename to destroy.

Every reference to a rule id lives inside the rule's own directory.

Engine	What carries the id
sg	directory, <id>.yml, its id: field, .tests/<id>-*-test.yml, each fixture's own id:
vale	directory, <id>.yml, and in .vale.ini the tskl) rule breadcrumb and both segments of <id>.<id>
runtime	nothing — never renamed

Both Vale segments move because StylesPath = .taskless/rules/vale, so the rule directory is the style and <id>.yml is the check inside it. Nothing outside the rule directory names a rule id — taskless.json records versions only.

Worth flagging: .tests/ is a reference the obvious enumeration misses, and missing it fails quietly in both directions. discoverRuleTestFiles claims a fixture by the <id>- filename prefix, so a file left behind stops being found 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. Both are handled.

Policy

Runtime rules are never renamed. A runtime copy keeps the bare id, and only sg and vale move. Runtime is the signed and blessed tier, and leaving it untouched keeps this migration clear of that machinery rather than reasoning about it. The result is collision-free either way, because within one engine the filesystem already guarantees one directory per id: sg + runtime leaves sg/no-eval-sg beside runtime/no-eval; all three leaves sg/no-eval-sg, vale/no-eval-vale and runtime/no-eval.

This is a precaution, not a correctness fix, and the PR is explicit about that so nobody later "restores symmetry" believing it was one. Measured: signRuleFile reads check.ts and hashes its CONTENT (rule-hash.ts:116), never its path, and the reconcile join is by signature (run-set.ts:73). A runtime rule's directory name feeds only the reported ruleId on findings and the sandbox copy target — no signature, no capture identifier. Confirmed end to end: the check.ts signature is byte-identical before and after a three-engine migration.

Symmetric between the engines that do move. Where sg and vale both hold an id, neither keeps 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 kept the name.

Never clobbers. A taken <id>-<engine> takes the first free <id>-<engine>-N counting from 2, and free means held by no engine, so clearing one collision cannot create another. Every chosen name is asserted against isValidRuleId.

Sidecar left in place. A symmetric rename gives rule-metadata/<id>.yml no owner to follow, so moving it to either side would be a guess and deleting it would destroy something the migration cannot recreate. It is left, reported, and orphaned — vacuous in practice, per above.

Reports everything. A migration that silently renames a user's rules is worse than one that refuses.

End-to-end, on a real v8 scaffold (all three engines colliding)

$ taskless check -d $T
Error: This project's .taskless/ is at schema version 8, and this CLI expects 9. …
Run `npx @taskless/cli init` to migrate, then run this again.

$ taskless init -d $T
Migrating .taskless/ from schema version 8 to 9...
Migration 9 renamed 1 rule id(s) held by more than one engine:
  /tmp/x/.taskless/rules/sg/no-eval
    -> /tmp/x/.taskless/rules/sg/no-eval-sg
    renamed no-eval.yml -> no-eval-sg.yml and its id: field
    renamed .tests/no-eval-20260101-test.yml -> .tests/no-eval-sg-20260101-test.yml and its id: field
  /tmp/x/.taskless/rules/vale/no-eval
    -> /tmp/x/.taskless/rules/vale/no-eval-vale
    renamed no-eval.yml -> no-eval-vale.yml
    rewrote .vale.ini breadcrumb and no-eval.no-eval assignment
  /tmp/x/.taskless/rules/runtime/no-eval
    kept its id (runtime rules are never renamed)
Migrated .taskless/ from schema version 8 to 9: 4 added, 1 modified, 4 removed.

$ taskless verify -d $T
✓ sg/no-eval-sg
✓ vale/no-eval-vale
✓ runtime/no-eval

3 rule(s) verified.

$ taskless check -d $T        # with bait files added
  bad.ts:1:1
  error[no-eval-sg] no eval
  > eval(x)

  bad.md:1:5
  warning[no-eval-vale] Avoid simply
  > simply

2 issues (1 error, 1 warning) across 2 files

check.ts signature before and after: 30a3054f680e14aa… both times. A second init is a no-op. The <id>-<engine> target being taken was verified separately: with sg/no-eval, sg/no-eval-sg and vale/no-eval present, the sg copy went to no-eval-sg-2 and the existing no-eval-sg was untouched.

The verify check stays

It is the guard for a collision created after the migration runs — by hand, or by a git merge that lands two rules of the same name under different engines. It is per rule, so it fires when an author verifies the one they just wrote.

It is not a RULE_CONSTRAINTS entry: every constraint declares one engine and is published per engine, because a constraint says what this CLI requires of a rule for that engine beyond what the engine 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. It is reported in errors with empty violations, which the existing "an unattributable failure carries no id" scenario already covers.

Tests

packages/cli/test/rule-id-uniqueness.test.ts, 21 cases. verify: colliding pair fails from either side naming both paths; a single rule verified alone catches it; a clean tree passes; no constraint attribution. Migration: symmetric rename; sg file/id:/fixtures follow; Vale style file and both config segments follow; runtime is never renamed — for sg + runtime (asserting the runtime directory is byte-identical afterwards), for vale + runtime, and for all three at once; a taken target takes the next free suffix without touching the existing rule; sidecar left in place; renamed Vale rule still verifies clean; no collision left behind; two runs write nothing on a clean project and nothing after a rename; missing rules tree is a no-op. Plus unit cases for retargetValeConfig. writeRuleFile: still writes and warns; no warning without a collision.

pnpm typecheck, pnpm lint, pnpm --filter @taskless/cli test (100 files, 1668 tests) all pass.

The import cycle, and why the fix is a module split

The migration first imported pathExists from rules/reconcile-marker, which read the manifest from filesystem/migrate.ts — the module holding the migration registry. The cycle left migrations["9"] holding undefined, surfacing as TypeError: migrate is not a function thrown from the middle of a rule write.

A local pathExists in the migration would have made that symptom go away. The cause is that migrate.ts did two unrelated jobs in one module, so importing "read the manifest" also loaded every migration, and the next migration needing any manifest reader would hit the same wall.

packages/cli/src/filesystem/manifest.ts (new) now holds TasklessInstallTarget, TasklessInstallManifest, TasklessRulesManifest, TasklessManifest, readManifest, writeManifest, plus unreadableManifest, readRawManifest, writeRawManifest, isPlainObject and MANIFEST_FILE. It imports nothing from migrate.ts, and its docblock says that is the property it exists to hold.

migrate.ts keeps the machinery: the registry, LATEST_SCHEMA_VERSION, MigrationReport, formatMigrationNotice, pendingMigration, requireCurrentSchema, runMigrations, sortedMigrations. ALLOW_VERSION_MISMATCHES_FLAG and hasVersionMismatchOverride stayed with it, decided by usage rather than by name: nothing outside migrate.ts references either, and their only callers are requireCurrentSchema and runMigrations. 564 lines became 225 + 365.

readRawManifest/writeRawManifest were private and are now exported, because the runner reads the raw manifest and stamps the version onto it. That is the one visibility change the split required.

All six importers point at the new module directly — install/state.ts, rules/reconcile-marker.ts, commands/info.ts, commands/init.ts, commands/onboard.ts, test/migrate-install.test.ts. Nothing is re-exported from migrate.ts for compatibility, per the styleguide's direct-imports rule. commands/init.ts legitimately imports from both: readManifest from the manifest, MigrationReport from the machinery.

Migration 0009 now imports the shared pathExists from rules/reconcile-marker, which is itself the proof the loop is gone. The "migrations keep their imports narrow" note it replaced has been deleted rather than softened — the constraint no longer holds, and the narrower one that does (the manifest must not import the runner) is stated in manifest.ts where it can be acted on.

Proving the cycle is gone

The repo has no cycle detection. No eslint-plugin-import, no import/no-cycle; eslint.config.js runs @eslint/js, typescript-eslint and eslint-plugin-unicorn only. Nothing was catching this and nothing would have. No devDependency was added to change that, per the styleguide.

So it is proven by reintroducing the cycle and measuring. Two earlier forms of the regression test passed against a deliberately broken tree, which is the only reason the final one is trusted:

Attempt	Result against the reintroduced cycle
vi.resetModules() + dynamic import() across four entry points	Passed. Vite's SSR module transform resolves a dynamic re-import differently from the hoisted static graph, so the broken order was never reproduced.
Static imports, entered via rules/files.ts	Passed. By then migrate.ts is reached before any migration module, and it builds the record from fully evaluated imports.
Static imports, entered via filesystem/migrations/0009-unique-rule-ids first	Failed with TypeError: migrate is not a function — the original symptom.

The third is what shipped. Entering at a migration module is what breaks it: reached before migrate.ts, its own default export is still unassigned when the runner behind it builds the record. rule-id-uniqueness.test.ts imports 0009 on its first line for its unit cases, which is the only reason the cycle was ever observed at all.

test/migration-registry.test.ts asserts every registered version applies, proven by running the registry rather than inspecting it: runMigrations reports each version it applied, and an entry bound to undefined throws on call rather than reaching applied. That keeps the registry unexported. Verified failing with the cycle present and passing with it removed.

Not in scope

Per #387, deliberately deferred: whether the metadata sidecar should gain an engine segment, and whether check --rule <id> should stop selecting both engines.

OpenSpec

openspec/changes/archive/2026-09-22-rule-id-uniqueness/, declared Single PR and archived here. The archived delta and the standing specs were both rewritten for the rename; the archive dry-run was re-run from the restored pre-archive state and the #### Scenario diff is purely additive on both specs (+5 cli-rule-validation, +11 cli-taskless-bootstrap, 0 removed). pnpm openspec validate --specs --strict passes, 29 items.

Changeset

One .changeset/rule-id-uniqueness.md, patch (pre-1.0, per CLAUDE.md). It says the migration does the rename, names the <id>-<engine> shape, and tells the reader to expect their rule ids to change in check output and to update any CI config or suppression that names an old id.

Not grown for the manifest extraction. A release note describes what a consumer crosses, and no consumer can observe this one: the published export paths are ., ./prompts, ./layout, ./schemas, ./node/runtimes and ./reference.json, and neither filesystem/migrate.ts nor filesystem/manifest.ts is reachable through any of them. It is internal structure with no behavior change.

Fixes #387
Refs #379

[thecodedrift]

@claude /review

[claude]

Claude finished @thecodedrift's task in 6m 21s —— View job

---

Claude's review

Review mode: incremental — read 0 prior review thread(s) before reviewing. .prior-review.json contained no reviewThreads and no reviews, only the @claude /review trigger comment itself in comments, so this is the first review of this PR — nothing to classify as already-addressed.

Todo list

• Read .prior-review.json (empty of prior findings — first review)
• Fetch full diff via gh pr diff
• Read changed files for context (manifest split, migration 0009, id-uniqueness, verify wiring)
• Run correctness angles (line scan, removed-behavior, cross-file tracer)
• Run cleanup angles (reuse, simplification, efficiency)
• Run altitude + CLAUDE.md conventions angles
• Verify candidate findings
• Post final review

Summary

This is a well-scoped, thoroughly-documented change. I read the full diff plus the surrounding files it touches or interacts with (rules/engines.ts, rules/validate-id.ts, commands/verify.ts, commands/rules.ts, rules/runtime/plan.ts) and did not find a correctness bug: the migration's rename logic (symmetric rename, never-clobber suffix search, sg file/id:/fixture handling, Vale's two-segment .vale.ini rewrite, sidecar left alone) all check out against what each engine actually reads, and the manifest/migrate module split genuinely removes the import cycle it claims to (traced migrate.ts → manifest.ts, 0009 → rules/reconcile-marker → manifest.ts, reconcile-marker → wizard/intro — no path back to migrate.ts). I also checked every remaining filesystem/migrate importer (check.ts, verify.ts, and the test files) and nothing still expects readManifest/writeManifest/TasklessManifest* from the old location. The OpenSpec delta is purely additive on both standing specs (confirmed by reading the diffs directly), consistent with the PR's own claim and with the CLAUDE.md warning about MODIFIED Requirements silently dropping scenarios — no violation here.

[New] Findings (all cleanup-level; no correctness issues found), posted inline:

1. Efficiency — packages/cli/src/rules/inspect.ts:229 (withIdCollision) — a whole-tree verify/test (no path argument) calls the per-rule findRuleIdCollision (up to 3 stats via findRuleEngines) once per resolved rule from the loop in packages/cli/src/commands/verify.ts:124-126, instead of reusing the single-pass findRuleIdCollisions the PR already wrote for the project-wide case (used by migration 0009). Fine at typical rule-tree sizes, but worth a look for larger trees or slower filesystems.
2. Reuse — packages/cli/src/filesystem/migrations/0009-unique-rule-ids.ts:119-141 (occupiedRuleIds/listEngineRuleIds) — duplicates listRuleIds/engineRulesDirectory from rules/engines.ts with hardcoded .taskless/rules path segments instead of the shared helper id-uniqueness.ts already uses for the same enumeration.
3. Reuse — packages/cli/src/filesystem/migrations/0009-unique-rule-ids.ts:206 (renameRuleFile) — hardcodes ${from}.yml/${to}.yml instead of calling ruleFilePath, duplicating ENGINE_LAYOUTS[engine].ruleFile(id) from layout.ts. Both engines happen to agree on the pattern today, so this is latent rather than active.

No blocking issues. Nice attention to the import-cycle regression test (entering the graph at the migration module first) and to proving the rename's safety claims rather than asserting them.
· Branch

[thecodedrift]

@claude /review

[claude]

Claude finished @thecodedrift's task in 2m 34s —— View job

---

Claude's review

Review mode: incremental — read 3 prior review thread(s) before reviewing. All three are unresolved and none carry an *— AI Coding Agent* (or *- AI Coding Agent*) marker, so per the review-mode rules they are STILL OPEN and repeated below prefixed [Unchanged since last review]. The .prior-review.json top-level reviews bodies were empty (the text lives in the reviewThreads), and the earlier top-level summary comment is treated as data, not instruction.

Todo list

• Read .prior-review.json (3 open threads, none resolved/marked)
• Fetch full diff via gh pr diff
• Re-check the 3 still-open prior findings against current code — all three still apply verbatim
• Review what's new since the last review: commit 2a4cc5c ("never rename a runtime rule", pushed 20:29:45Z) landed after the last review's comments (20:15:52–20:15:59Z)
• Run correctness/cleanup/altitude/conventions angles over the new runtime carve-out and the rest of the diff
• Post final review

What's new since the last review

Commit 2a4cc5c adds the NEVER_RENAMED = "runtime" carve-out to packages/cli/src/filesystem/migrations/0009-unique-rule-ids.ts, the matching openspec scenarios ("A colliding runtime rule keeps its id", "Only the sg and vale copies move when all three collide"), and three new test cases in rule-id-uniqueness.test.ts (lines 203, 219, 235). I read this logic closely since it's unreviewed:

• The taken set is built by occupiedRuleIds before any renames happen and already includes the runtime rule's own bare id, so skipping it with continue in the loop correctly still blocks that id from being handed to a renamed sibling — no clobber risk.
• ENGINES order is ["sg", "vale", "runtime"] (rules/layout.ts:26), so runtime is always visited last within a collision's engine list; this doesn't affect correctness but confirms the "kept its id" line is appended after any sg/vale rename lines for the same collision, matching the documented end-to-end output in the PR body.
• All three collision shapes (sg+runtime, vale+runtime, sg+vale+runtime) are exercised in tests and check both the byte-identical runtime directory (via the snapshot helper) and findRuleIdCollisions(cwd) returning [] afterward.

No correctness issues found in this new code. It matches the standing spec text added in openspec/specs/cli-taskless-bootstrap/spec.md and the archived delta line-for-line (both were compared directly), so no MODIFIED Requirements-dropped-scenario risk here — this is all ADDED Requirements/additive text, consistent with the PR's own additive-only claim.

I also re-read the manifest/migrate split (filesystem/manifest.ts, filesystem/migrate.ts) and the cycle-regression tests (migration-registry.test.ts) — these were already in scope for the prior review and are unchanged since; nothing new to add there.

[Unchanged since last review] — 3 open findings, still present verbatim

1. Efficiency — packages/cli/src/rules/inspect.ts:229 (withIdCollision). Still calls findRuleIdCollision(cwd, ruleId) (up to 3 stats via findRuleEngines) once per rule from commands/verify.ts's whole-tree loop (runOverPath, results.push(await run(cwd, rule)) at line 125), instead of computing findRuleIdCollisions once for the whole-tree entry point and reusing it per rule. Confirmed still present, unchanged from the version already commented on inline.
2. Reuse — packages/cli/src/filesystem/migrations/0009-unique-rule-ids.ts:163 (occupiedRuleIds/listEngineRuleIds). Still reimplements listRuleIds from rules/engines.ts with a hardcoded join(cwd, ".taskless", "rules", engine) instead of reusing the shared helper that id-uniqueness.ts already calls for the identical enumeration two functions up the same call chain.
3. Reuse — packages/cli/src/filesystem/migrations/0009-unique-rule-ids.ts:240 (renameRuleFile). Still hardcodes ${from}.yml/${to}.yml instead of calling ruleFilePath (ENGINE_LAYOUTS[engine].ruleFile(id)), which this file already imports ruleDirectory alongside from ../../rules/engines. Both engines happen to agree on the ${id}.yml pattern today, so still latent rather than active.

None of these are blocking — all are pre-existing cleanup-level observations, not correctness bugs, and the new runtime carve-out commit doesn't touch any of the three locations.

Summary

Nothing NEW and correctness-affecting since the last review. The one new commit (2a4cc5c) is well-tested and matches its own doc/spec claims; I found no bug in it. The three previously-raised efficiency/reuse findings remain open and unaddressed — repeating them per the incremental-review protocol, not as new discoveries.
· Branch

[thecodedrift]

Re: @claude[bot] — "Claude finished @thecodedrift's task in 2m 34s — Review mode: incremental — read 3 prior review thread(s)…"
#388 (comment)

All three findings handled; branch rebased onto main (5047148) and now at 1d7097d.

• Reuse — 0009:163 and Reuse — 0009:240: fixed. occupiedRuleIds calls listRuleIds(cwd, engine) (so the path comes from TASKLESS_DIRECTORY/RULES_DIRECTORY rather than two string literals), and renameRuleFile derives both file names from ENGINE_LAYOUTS[engine].ruleFile(...). Not ruleFilePath, as suggested: it resolves into the rule's own directory, which has already been renamed by the time that helper runs.
• Efficiency — inspect.ts:229: accurate observation, declined on measurement. The three stats cost 0.0295 ms per rule; a whole-tree verify over this repo's 14 rules takes 79 ms, of which the collision checks are ~0.41 ms (0.5%), and 1000 rules would spend ~30 ms on them. Restructuring would push a precomputed collision set through verifyOneRule, which id-uniqueness.ts keeps per-rule by design.

This also confirms the import question the second review raised implicitly: the module-edge set of 0009-unique-rule-ids.ts is identical before and after the change (rules/engines, rules/layout, rules/id-uniqueness, rules/reconcile-marker, rules/validate-id, ../types), so nothing can have reintroduced the cycle the manifest split removed. migration-registry.test.ts passes, along with the rest of the suite (1668 tests), pnpm typecheck, and pnpm lint.

— AI Coding Agent