Skip to content

verify: refuse two rules that share an id across engines #387

Description

@thecodedrift

Nothing in the CLI keeps .taskless/rules/sg/no-eval/ and .taskless/rules/vale/no-eval/ from both existing, and a collision is silent today. verify should be the place that catches it.

What is true now

The id contract has no uniqueness component: packages/cli/src/rules/validate-id.ts:5 is /^[a-z0-9][a-z0-9-]*$/.

The write path never looks at the other engines. writeRuleFile (packages/cli/src/rules/files.ts:67-82) validates the id, resolves the engine, and mkdirs ruleDirectory(cwd, engine, id). deliver.ts polices path safety, case folding, ancestor/descendant conflicts and symlinks, all scoped to the one rule directory being written.

The read path never compares. assemble.ts:182 and :251 each call listRuleIds(cwd, <engine>) for their own engine; commands/check.ts:193 and :287 hold both lists and never diff them.

This is already documented as deliberate, at packages/cli/src/rules/engines.ts:158-168:

THIS RETURNS EVERY MATCH, NOT THE FIRST. It used to return the first, on the stated grounds that "a rule id is globally unique by construction (<slug>-<sha1>), so at most one engine can hold it". That was never true of ids this CLI accepts: isValidRuleId is /^[a-z0-9][a-z0-9-]*$/ with no sha component and no cross-engine check, and the shipped demonstration rules (no-eval-call, prefer-use-over-utilize, env-keys-declared) carry no suffix at all. <slug>-<sha1> describes SERVER-GENERATED ids and was written as though it described every id.

Why it matters, worst first

The metadata sidecar is keyed by id alone and silently clobbers. writeRuleMetaFiles (files.ts:203-211) writes .taskless/rule-metadata/{key}.yml with no engine segment. Two colliding rules share one file: the second rule create or rule improve overwrites the first's metadata with no warning, and rule meta <id> returns the wrong rule's. This is real data loss, and it happens whether or not anyone ever runs check.

Delete takes the survivor's metadata with it. deleteRuleFiles (files.ts:318-321) unconditionally removes rule-metadata/{id}.yml. The directory delete itself is safe — it returns ambiguous and refuses with RULE_ID_AMBIGUOUS — but once the other engine's copy is later removed by path, the metadata is already gone.

Human check output cannot tell them apart. util/format.ts:15 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 (schemas/check.ts:5-6), so a machine consumer keying on (source, ruleId) is correct and one keying on ruleId alone silently merges two rules.

What to do

  • verify fails when any id appears under more than one engine, naming both paths and telling the user to rename one. Both id lists are already in hand at commands/check.ts:193/:287, and findRuleEngines (engines.ts:176-196) already returns exactly this answer.
  • Match the wording rules delete already produces for RULE_ID_AMBIGUOUS (commands/rules.ts:778), so the two surfaces describe the same condition the same way.
  • Do NOT make writeRuleFile refuse. check's repair path calls it, so a hard refusal would brick repair for both colliding rules — strictly worse than today. A warning there is fine; the failure belongs in verify.
  • Leave check --rule <id> selecting both engines (rules/rule-filter.ts). It over-selects rather than mis-selects, the findings carry source, and once verify refuses the collision the case stops arising.
  • Decide separately whether the metadata sidecar should gain an engine segment. Renaming a rule is not free either: the sidecar, the .tests/ fixtures, and the server-side id all reference the old name.

Open question

Does the server enforce uniqueness of rules[].id within or across engines at generation time? The OpenAPI document (packages/cli/src/generated/api.d.ts:234-286) models delivery as a discriminated union on engine with id documented as "The rule directory name under .taskless/rules/<engine>/", so it has no reason to. If it does not, a client-side refusal on delivery would turn a silent collision into a failure the user cannot fix, because they did not author the id. That is why this is a verify check and not a delivery check.

Refs #379

The check runs per rule, not only over the whole project

verify is invoked on a single rule as well as on the tree, so the uniqueness check has to be part of verifying one rule: given a rule, its id must not appear under any other engine. A whole-project pass that only diffs the two id lists would report nothing when an author verifies the rule they just wrote, which is exactly the moment the collision is cheapest to fix. findRuleEngines (packages/cli/src/rules/engines.ts:176-196) already answers this for a single id, so the per-rule form is the natural one and the project-wide form falls out of running it for each rule.

A migration has to come with it

A new scaffold version needs a migration that checks every existing project for collisions on upgrade. Without one, the check ships and only ever fires for rules written after it, while a project that already has sg/no-eval and vale/no-eval keeps silently sharing one metadata sidecar.

The migration can detect but must not rename. Nothing tells it 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.

So the migration's only correct behavior is to refuse, naming both directories and telling the user to rename one. Migration is (directory: string) => Promise<void> (packages/cli/src/filesystem/types.ts:1), so throwing is available. Three consequences to settle before writing it:

  • Migrations run on init, demo, onboard and rule delivery. A throwing migration therefore blocks all four until the user renames a rule. That is the intended wall, but it should be reached with a message that names both paths and the exact rename to perform, not a stack trace.
  • check and verify refuse a stale scaffold with SCAFFOLD_MIGRATION_REQUIRED naming init, and init is the command the migration would block. Make sure the two messages compose into a path out rather than a loop: the migration's message has to tell the user what to do before re-running init.
  • Idempotency still applies. A project with no collision must be read and not rewritten, leaving git status clean, the way 0008 does.

Number it 0009 (LATEST_SCHEMA_VERSION is currently 8) and bump the repo's own .taskless/taskless.json in the same PR — packages/cli/test/dogfood-scaffold-current.test.ts fails otherwise, by design.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    CLIRelated to the taskless CLI

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions