Skip to content

fix(cli): give every ast-grep rule a .tests/ so one rule can't fail them all - #154

Open
thecodedrift wants to merge 1 commit into
mainfrom
fix/sg-tests-dir
Open

fix(cli): give every ast-grep rule a .tests/ so one rule can't fail them all#154
thecodedrift wants to merge 1 commit into
mainfrom
fix/sg-tests-dir

Conversation

@thecodedrift

Copy link
Copy Markdown
Member

Migration 0005 creates a rule's .tests/ only as a side effect of moving a test file into it. moveEngineRules creates rules/sg/<id>/ and never touches .tests/; moveEngineTests creates it only for a filename matching ^(?<id>.+?)-\d{8}-test\.ya?ml$ and skips anything else. So an sg rule that had no test file at version 3 — or one whose name lacked the timestamp — arrives at version 5 with no .tests/.

assembleSgConfig then emitted a testDir for every rule id with no existence check, and ast-grep 0.41.0 treats a missing testDir as fatal to the whole invocation:

Error: Cannot read rule directory .taskless/rules/sg/no-eval/.tests
╰▻ IO error … No such file or directory (os error 2)     # exit 6

--filter does not scope it. The sharpest form, on an already-migrated project, is that test scoped to a healthy rule still fails, naming the broken one:

$ taskless test .taskless/rules/sg/no-var
✗ sg/no-var
    Error: Cannot read rule directory .taskless/rules/sg/no-eval/.tests
1 of 1 rule(s) failed.

That is why adding a correct new rule does not help — the error names a rule the author never touched. sg-specific: Vale reads a missing tests directory as empty.

Fixes #150

Two parts, and the second is not redundant

0005 now gives every rules/sg/<id>/ a .tests/, with a .gitkeep when it would otherwise be empty. Amended in place rather than added as 0006: 0004/0005 are unreleased (stable is 0.10.2; only @taskless/cli-nightly carries them), and amending an unreleased migration is the established pattern here — 0004 was amended twice this way under .changeset/idempotent-rules-partition.md. The .gitkeep is committed, not gitignored: git does not track empty directories, so without it a repaired .tests/ never reaches CI or a fresh clone. A test pins that .taskless/.gitignore never mentions it.

assembleSgConfig now omits a testDir that is not on disk. This is what actually rescues an already-broken install: runMigrations short-circuits once the manifest reads version 5, so a project a nightly already stamped never re-runs the amended migration. It also covers a hand-made mkdir .taskless/rules/sg/<id>/ on any version. Nothing becomes a silent pass — verify still reports "No test file found" and test still reports "Skipped: no test file found", both of which read the rule directory rather than this config.

Measured against ast-grep 0.41.0

Empty .tests/sg test and sg scan both exit 0. A .tests/.gitkeep → both exit 0, and the .gitkeep is not picked up as a test case (Running 1 tests for the tree, not 2). testConfigs: with no entries → Running 0 tests, exit 0, and check still reports findings normally. The assembler emits exactly that bare-key form when nothing qualifies.

The ingest path was deliberately left alone

commands/rules.ts gates writeRuleTestFile behind if (rule.tests), so an API-delivered rule with no tests also produces a rule directory with no .tests/. Not fixed there: the assemble check already makes any such directory harmless however it arose, so an ingest-side mkdir would be a third enforcement point for one invariant and the weakest of the three — it covers only rules arriving through create/improve, and would not have fixed this bug, since those directories are already missing on disk. A rule delivered with no tests is precisely what verify exists to flag, and the next improve-rule round-trip creates the directory by writing a real test into it.

The migration is different, and does warrant its own fix: it is a one-shot transformation whose output the user then commits, with nothing coming back later to repair it.

Two sibling gaps, verified and not fixed here

Both reproduce, but neither is silent, which is the material difference:

  • Vale per-rule .vale.ini — written only for matchers carrying a tskl) rule = <id> breadcrumb, so a hand-written matcher yields an effectively disabled rule. But the migration prints a notice, leaves the orphan file, and verify says "has no .vale.ini, so nothing scopes it and it will never run." Fixing it means inventing an attribution heuristic for hand-written matchers, which 0005's docblock explicitly declines to guess at.
  • Runtime captures/ — created only when there is a loose *.yml to move. verify reports it, and nothing hard-fails, so no other rule is affected. This is the closest analogue to Possible Missing Test Migrations #150 and worth a follow-up; it is a two-line change to the same helper.

Tests

assemble.test.ts — a testDir absent from disk is omitted; the general invariant that every emitted testDir exists (parsed and stated); and a config is still emitted when no rule has a tests directory, asserting the exact bare testConfigs: form measured as accepted.

migrate-engine-layout.test.ts — the existing fixture already reproduced this bug via its untimestamped rule-tests/no-eval-test.yml, with a comment at :107 noting 0005 "cannot attribute it and leaves it behind". It now asserts every rules/sg/*/ has a .tests/, that no-eval got a .gitkeep, and that a .gitkeep never lands beside a real test file.

…hem all

Migration 0005 created a rule's `.tests/` only as a side effect of moving a
test file into it, so an sg rule with no test at version 3 — or one whose
filename lacked the `-YYYYMMDD-test.yml` suffix the migration attributes by —
arrived at version 5 with no tests directory. Assembly named it in
`testConfigs` anyway, and ast-grep 0.41.0 aborts the whole invocation on a
`testDir` it cannot read (exit 6), which `--filter` does not scope away:
`taskless test` on any rule died naming a rule the author never touched.

0005 now ensures `rules/sg/<id>/.tests/`, gitkeeped when empty so the repair
survives a commit. `assembleSgConfig` separately omits a `testDir` that is not
on disk, which rescues a project a nightly already stamped at version 5 — those
never re-run the migration — and a rule directory made by hand. Neither turns a
missing test into a pass; verify and test still report it.

Fixes #150

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Cyga14bww8rmazH2XrF8ms
Copilot AI lite review requested due to automatic review settings August 24, 2026 04:59

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Possible Missing Test Migrations

2 participants