feat: one directory per rule, and path-addressed verify/test - #103
feat: one directory per rule, and path-addressed verify/test#103thecodedrift wants to merge 18 commits into
Conversation
A Vale rule is currently three locations, one of which every rule in the project shares. That shared .vale.ini is where all five harness runs found silent failures — an assignment above the first matcher, a glob that missed the fixture extension, three names that had to agree with nothing reporting when they didn't. A single write-contended config is the wrong shape at any scale. Each rule becomes a directory holding its style and its own .vale.ini; check assembles the run config from them and gitignores it. Measured against Vale 3.17.1: rules/<id>/<id>.yml resolves as check <id>.<id> under StylesPath = rules and resolves to nothing under StylesPath = ., Vale rejects unknown keys in a style so scope cannot ride along inside it, and a .yml sidecar in a style directory is loaded as a rule and fails E201 while a .vale.ini in the same place is ignored. Also replaces rule verify <id> with path-addressed verify and test. An id does not name one thing — the same id can exist under two engines, which is why the id form needed an ambiguity error at all. Adds an example/ project so a reader can see an install rather than infer it from tests that build their own fixtures. Stacked on #102 and merging down: a layout change without its migration ships a project whose rules silently stop running.
Widens the change from Vale to every engine, and renames it to match. One directory per rule, identical shape everywhere: `.taskless/rules/<engine>/<id>/` holding the rule, any config that engine requires, and its tests in `.tests/`. A rule becomes one path rather than two, which is what makes `verify <path>` and `test <path>` work without an id lookup. The dot on `.tests/` is load-bearing and measured. ast-grep's ruleDirs recurses and parses every .yml beneath as a rule, so a plain `tests/` directory fails the scan with "missing field 'language'"; `__tests__/` fails the same way; a dot-directory is skipped, and `sg test` still reads it when testDir names it. Vale is unaffected either way — a `.tests/` inside a style directory is harmless even containing a .yml. That is a dependency on undocumented behavior, recorded as one in D2 with two mitigations: the failure is loud (a parse error naming the file, not a silently reinterpreted test), and a test pins it. The rejected alternative — materializing a rules-only tree for ast-grep — is written down as the fallback if the assumption ever breaks. Also drops the per-rule sg config: ast-grep expresses scoping inside the rule, so the slot would be an empty file every author creates, no author fills, and every reader learns to ignore. And renames runtime's capture rules to `captures/`, since "matcher" now means a Vale glob section in this same tree. 0005 layers on 0004; both are unreleased and both ship in this stack, so consumers run them as one upgrade and never see the intermediate layout.
thecodedrift
changed the title
…ting Started the layout refactor and hit something the proposal missed: `.taskless/rules/` is the new root and is also LEGACY_RULES_DIRECTORY, the pre-0004 flat location. Same string, different meaning. The legacy read paths turn out to be removable rather than renameable, because they are unreachable: ensureTasklessDirectory runs migrations before anything reads a rule, so 0004 has already moved .taskless/rules/*.yml to sg/rules/ and 0005 moves it again. Under the new layout a legacy lookup would resolve .taskless/rules/<id>.yml inside a tree whose real contents are rules/<engine>/<id>/ — reading the new root as though it were the old flat directory. A stale read path that resolves into the live tree is worse than no fallback. Also adds the migration precondition that follows: 0005 asserts the new root holds no top-level *.yml before writing engine directories into it, since a file still there means 0004 did not complete. Reverting the partial engines.ts refactor so this PR stays spec-only and green rather than carrying a tree with twelve broken callers.
Groups 1-3 of self-contained-rules, plus the `rule verify` removal. ENGINE_LAYOUTS now describes a rule *directory* rather than parallel rules/ and rule-tests/ trees, and every path derives from it. Tests live in `.tests/` — the dot is load-bearing, since ast-grep's ruleDirs recurses and parses every .yml beneath as a rule, and the constant carries that measurement plus the fallback if it ever breaks. Both engine configs are now assembled per run and gitignored. Assembly is deterministic — rules sorted by id, each rule's matcher order verbatim — because Vale's precedence is positional and a config built in directory-iteration order would give a rule a different effective scope per machine. Migration 0005 moves everything and rewrites nothing: capture bytes determine reconciliation hashes. It asserts `.taskless/rules/` holds no loose .yml first, since that path is both the new root and the pre-0004 flat location. Vale matchers split by their `tskl) rule` breadcrumb; one without a breadcrumb is left in place and reported rather than guessed at or dropped, because it is a user's hand edit. Deleted with their layouts: the legacy read paths (unreachable — the migrations run before any read), filesystem/sgconfig.ts (assembly replaces it), rules/owner.ts and `rule verify` (an id does not name one rule; the path form has no ambiguity case). 0004's StylesPath docstring now explains both layouts. It said `StylesPath = rules` is wrong, which was true flat and is exactly backwards here — a bare contradiction invites a future reader to revert it. Typecheck and build clean. Tests still assert the old layout; they are next.
Caught by migrating a real 0004 project and running check: ast-grep kept reporting, Vale went silent. A Vale check is named <style>.<rule>, and the style is whatever StylesPath points at. Flat, that was `rules`, so assignments read `rules.<id> = YES`. Per-rule directories make each rule its own style, so the same rule is `<id>.<id>`. The migration moved the assignment across verbatim, producing a config Vale parses happily, reports nothing for, and exits zero on — every migrated rule silently disabled, which is precisely the failure this layout exists to prevent. 0005 now rewrites the assignment as it splits, and the mapper collapses `<id>.<id>` to `<id>` so findings carry the id the user filed under. It only collapses when both halves match: a `<style>.<check>` where they differ came from a user's own Vale styles, and halving that name would report under an id identifying nothing. Verified end to end — a 0004 project migrates and both engines fire.
Fixture project, engine dispatch, and the Vale suites now build rules as directories. Adds assemble.test.ts covering what task 2.5 asks for: the StylesPath header, sorted rule order, byte-identical output across runs, each rule's own matcher order preserved, and provenance tagging. Also makes 0005 scaffold rules/<engine>/ — it prunes 0004's directories, so without this a freshly migrated project had no rules tree at all, every engine reporting absent and nowhere obvious to write a first rule. Deletes sg-committed-config.test.ts (the committed config it covers is now assembled) and rule-verify-dispatch.test.ts (rule verify is gone; its replacement lands with the path-addressed commands). 55 failures left, all layout paths in the remaining suites.
Also fixes buildIsolatingConfig, which still enabled `rules.<id>` under a StylesPath that no longer produces that name — the same silent-disable the migration hit, one file over. Verification would have reported every rule as not firing. migrate-engine-layout now asserts the end state of 0004+0005 rather than 0004's intermediate layout: both are unreleased and ship together, so a user upgrades through the pair and never sees the middle. 90 failures down to 30.
Retires the two tests describing behavior that no longer exists — a rule at the pre-0004 flat path, and merging two layouts with de-duplication. There is one tree now, so there is no overlap to collapse. Drops the `rule verify --anonymous` case with the command it covered. 30 failures down to 21.
566 passing, typecheck and lint clean. Adds engine-layout.test.ts, which is what design D2 promised: it asserts a scan stays clean with test YAML inside a rule's .tests/, and that the same file in a plain tests/ fails. The dot-directory behavior is undocumented and the binary is version-pinned, so this test is what fires at a deliberate bump — turning the discovery into a migration task with a changelog rather than a mystery in CI. Two reconcile/ingest tests needed rethinking rather than repathing. The signature test discovered the pre-migration tree to sign it, which no longer works now that discovery reads one layout; it describes the old rule directly instead, since the property under test is that the signature survives the move. Retires the assertions that only made sense with two layouts.
Group 4. `verify <path>` checks a rule has the components its engine requires; `test <path>` runs its tests and runs verify first. The split is about preconditions: an agent mid-authoring has a rule and no fixtures yet, and needs to know the rule is valid before it can write a meaningful test. So verify deliberately does not require tests. Running verify first is the ordering fix. When a rule is both malformed and under-fixtured, the fixture complaint is the less useful error and is the one that surfaced first before — the author was told their fixtures were incomplete while the reason the rule could never have run went unmentioned. A test pins that. A path names one thing, so there is no ambiguity case to report: the same id under two engines is just two paths. The engine comes from the path's <engine> segment, never from the file, which is what dispatch already does — so a rule cannot be validated by one engine and run by another. A directory means everything beneath it, which is the CI form. Verify also catches the two Vale silent-disables by name: a rule with no config of its own is scoped to nothing, and a config that never enables <id>.<id> leaves the rule present but off. 577 tests pass; typecheck and lint clean.
Group 6, and the lint fixes for group 4 that I pushed past. The tests cover behavior thoroughly but build their fixtures inside the test that reads them, so nothing in the repo showed the layout as a reader would meet it. example/ is that: two rules, one per engine, in a project small enough to take in at once. Its README explains the two things that are not self-evident — why .tests/ is dot-prefixed (ast-grep reads every .yml under the rules tree as a rule) and why only Vale has a per-rule config (it rejects unknown keys, so scope cannot live in the style file). example-project.test.ts is the other half. It runs check, verify, and test against a copy and asserts on the results, including the exact finding shape the README quotes, so a layout change that breaks the demo fails the build rather than leaving something misleading in the repo. It also asserts neither assembled config is committed — an example that checked in generated files would teach that. example/ joins the root eslint and prettier ignores: its source calls eval on purpose and its fixtures are prose written to be flagged. 582 tests pass; typecheck and lint clean.
No em dashes, no antithesis, contractions throughout, shorter sentences. Extended past the README to the rule messages and the .vale.ini comment, because those are the example's most-read prose and would have taught the habit. `Avoid eval — it executes...` became `Avoid eval. It executes...`, and the no-simply message dropped its "not what to do" tail. The README's quoted check output was regenerated from a real run so it still matches byte for byte. 582 tests pass.
Every recipe that named a path still described the flat layout. This repoints them at `.taskless/rules/<engine>/<id>/` and replaces the id-addressed `rule verify` with the path-addressed `verify` and `test`. `create-vale-rule` changes most: the shared `.vale.ini` walkthrough becomes a per-rule config with its `tskl)` breadcrumb, and the `W101` outside-a-matcher warning is gone with the situation that caused it. The old text also warned at length that verification could not catch a missing assignment. It can now, so that becomes the `<id>.<id>` naming rule and a pointer at the command. Two findings from running the recipes rather than reading them: A `consistency` rule compiles its own name into its pattern as a Go RE2 capture group, and RE2 rejects a hyphen there. Measured against Vale 3.17.1, `ize-ise` fails with E201 and takes every Vale rule in the project down with it, since Vale reads one config per run: 9 rules, 0 findings. Our own recipes teach kebab-case, so we were teaching the break. `verify` now rejects it, two tests pin it, and design D10 records it. A hyphen is a word boundary, so `obviously` fires inside `obviously-named`. The recipe stated the boundary rule but not that case, and a harness agent used a hyphenated compound as a `pass/` near-miss that then fired. Verified by extracting all nine worked rules from the *rendered* recipe and executing them, and by re-running the authoring harness with fresh agents that had no repository access. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Jwc9FFroR3mTZ4hLiSkkX3
The existing migration tests assert where files land and that their bytes survive. Both can be perfect while the migrated project reports nothing, which is how the Vale silent-disable shipped: 0004 enables a check named `rules.<id>`, 0005's StylesPath resolves the same rule as `<id>.<id>`, and carrying the old assignment forward leaves every Vale rule present, valid, enabled against a check that does not exist, and quiet. It was caught by hand, by noticing ast-grep still reported while Vale had gone silent. So this seeds a version-4 project and runs the real CLI over it after migrating: both static engines still report, the assignment is retargeted, every rule still verifies, the tests that moved still run, runtime capture bytes are unchanged, and a second run is a no-op. The ast-grep half is the control, so a silent Vale shows up as a difference between engines rather than a plausibly-empty run. The changeset also described a layout this stack has since replaced, and promised an `engine-selection` topic that #102 merged into `route`. Rewritten for what actually ships. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Jwc9FFroR3mTZ4hLiSkkX3
7.1-7.6 verified. 7.7 is blocked on #102: a change archives once on the tip, and the gate wants openspec/changes/ empty, but agent-command-and-vale-authoring still has 14 open tasks. Archiving it early would drop the spec deltas for work nobody has done. #102's delta also has to reach specs/ before this change's delta has a target. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Jwc9FFroR3mTZ4hLiSkkX3
Three conflicts, all in files both branches edited: - The changeset becomes one release note for the whole stack. It merges down atomically, so a single note is accurate at the only moment it is read. - `rule.txt` takes the rename branch's table, which supersedes the narrower `rule verify` edit made here. - `verify-rule.txt` keeps this branch's rewrite, with its See Also on the new command name. Ports the W101 notice test onto the rule-directory layout. Its 5.4 counterpart is dropped rather than ported: it asserted that a Vale rule nothing scopes reports nothing and does not fail, and this layout makes that a `verify` error instead, already covered in `verify-test-commands.test.ts`. Under a per-rule config the misplaced assignment is the case `verify` genuinely cannot catch — the file has a matcher and names the check, so only Vale knows, and only at run time — which is what the ported test now says. Also adds the local run instructions to the example README and syncs its quoted `check` output with a real run, which was missing the ast-grep rule's `note:` block. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Jwc9FFroR3mTZ4hLiSkkX3
The stack's second and final archive, which is why #102 had to go first: this change's `cli-agent-authoring` delta modifies a requirement that one introduces, so it needed a target in specs/. The archive refused twice, both times on a rename expressed as a MODIFIED. Five of the six MODIFIED entries across cli-rule-format and cli-vale-rule-engine renamed their requirement, which openspec matches by header, so each became a REMOVE of the old name plus an ADD of the new. cli-rule-format's MODIFIED section emptied out entirely. Two requirements the deltas never retired, both now describing code this change deletes: - "Both the legacy and engine-partitioned layouts are readable" promises a legacy read path that task 1.5 removes. The legacy constant now names the same string as the rules root, so a stale read path would resolve into the live tree (design D9). - "The scaffolded Vale config carries no section" describes a file that no longer exists. Verified against a fresh `init`: the scaffold contains no `.ini` at all, because scope is declared per rule. `cli-rules` and `cli-update-engine` still fail --strict; both fail on main and neither is touched by this stack. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Jwc9FFroR3mTZ4hLiSkkX3
There was a problem hiding this comment.
Pull request overview
This PR standardizes Taskless rule authoring around a “one directory per rule” layout under .taskless/rules/<engine>/<id>/, replaces id-addressed rule verify with path-addressed verify <path> / test <path>, and introduces deterministic assembly of engine configs (Vale .vale.ini + ast-grep .sgconfig.yml) from per-rule sources. It also updates migrations/spec/docs/tests accordingly and adds a committed example/ project kept honest by tests.
Changes:
- Introduce per-rule directory layout across
sg,vale, andruntime, including.tests/and per-rule.vale.ini. - Add path-addressed
verifyandtestcommands (with shared path resolution) and remove id-addressedrule verify. - Add deterministic config assembly for Vale and ast-grep; update check/dispatch, migration wiring, and fixtures; add a committed
example/project plus coverage.
Reviewed changes
Copilot reviewed 89 out of 93 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/cli/test/vale-verify.test.ts | Updates Vale verification tests to the per-rule directory + .tests/ layout and new check naming. |
| packages/cli/test/vale-run.test.ts | Updates Vale runner tests to read the assembled config and new per-rule style locations. |
| packages/cli/test/vale-orchestration.test.ts | Adjusts orchestration tests to new assembled config flow and per-rule rule tree. |
| packages/cli/test/vale-map.test.ts | Updates mapping tests to the new Vale check id shape <id>.<id> and normalization behavior. |
| packages/cli/test/sgconfig.test.ts | Removes tests for legacy sgconfig generation (now replaced by assembly). |
| packages/cli/test/sg-committed-config.test.ts | Removes tests for committed sg config handling (replaced by assembly + new layout). |
| packages/cli/test/runtime-harness.test.ts | Updates runtime harness fixture writer to new runtime rule directory structure (captures/). |
| packages/cli/test/rule-verify-dispatch.test.ts | Removes tests for id-addressed rule verify dispatch/ambiguity behavior. |
| packages/cli/test/onboard.test.ts | Updates install manifest version expectations to version 5. |
| packages/cli/test/mixed-engine-check.test.ts | Updates integration test to author Vale rules/config per-rule and validate runtime warnings behavior. |
| packages/cli/test/migrate-round-trip.test.ts | Adds end-to-end behavioral migration coverage (rules still fire, tests still run, bytes stable). |
| packages/cli/test/migrate-install.test.ts | Updates migration/version assertions to version 5. |
| packages/cli/test/init-no-interactive.test.ts | Updates init scaffold manifest version assertion to version 5. |
| packages/cli/test/fixtures/mixed-engines-project/.taskless/vale/.vale.ini | Removes old committed shared Vale config from the fixture. |
| packages/cli/test/fixtures/mixed-engines-project/.taskless/taskless.json | Bumps fixture manifest version to 5. |
| packages/cli/test/fixtures/mixed-engines-project/.taskless/sg/sgconfig.yml | Removes old committed ast-grep config from the fixture. |
| packages/cli/test/fixtures/mixed-engines-project/.taskless/rules/vale/no-simply/no-simply.yml | Adds Vale rule fixture in the new per-rule directory layout. |
| packages/cli/test/fixtures/mixed-engines-project/.taskless/rules/vale/no-simply/.vale.ini | Adds per-rule Vale config fixture enabling <id>.<id>. |
| packages/cli/test/fixtures/mixed-engines-project/.taskless/rules/vale/no-obviously/no-obviously.yml | Adds a second Vale rule fixture for mixed-engine coverage. |
| packages/cli/test/fixtures/mixed-engines-project/.taskless/rules/vale/no-obviously/.vale.ini | Adds per-rule config for the second Vale rule fixture. |
| packages/cli/test/fixtures/mixed-engines-project/.taskless/rules/sg/no-eval/no-eval.yml | Adds sg rule fixture in the new per-rule directory layout. |
| packages/cli/test/fixtures/mixed-engines-project/.taskless/rules/sg/no-console-warn/no-console-warn.yml | Adds another sg rule fixture in the new directory layout. |
| packages/cli/test/example-project.test.ts | Adds tests ensuring the committed example/ project remains a valid, non-drifting demo. |
| packages/cli/test/error-envelope.test.ts | Removes rule verify error-envelope coverage and updates delete paths to new layout. |
| packages/cli/test/engine-layout.test.ts | Adds tests pinning the .tests/ dot-directory behavior required for ast-grep discovery. |
| packages/cli/test/detect.test.ts | Updates detect tests to report .taskless/rules/sg as a rule style source. |
| packages/cli/test/bootstrap.test.ts | Updates bootstrap/migration tests to the new rule directory layout and .tests/ placement. |
| packages/cli/test/assemble.test.ts | Adds direct tests for deterministic Vale/sg config assembly outputs. |
| packages/cli/test/anonymous-flag.test.ts | Removes rule verify --anonymous coverage (command removed). |
| packages/cli/src/rules/verify.ts | Updates sg verification to new path helpers and assembly-based test execution. |
| packages/cli/src/rules/vale/verify.ts | Updates Vale verify to use per-rule directories and new StylesPath resolution. |
| packages/cli/src/rules/vale/run.ts | Switches Vale runs to the assembled config path and keeps “no paths” behavior safe (.). |
| packages/cli/src/rules/vale/map.ts | Updates Vale check id normalization to collapse only <id>.<id> and leave foreign styles intact. |
| packages/cli/src/rules/scan.ts | Switches ast-grep scanning to the assembled config path. |
| packages/cli/src/rules/runtime/discover.ts | Updates runtime discovery to load capture rules from captures/ under each rule directory. |
| packages/cli/src/rules/resolve-path.ts | Adds path→(engine, ruleId) resolution for verify/test, including directory scoping rules. |
| packages/cli/src/rules/owner.ts | Removes id-based owner detection (no longer needed with path addressing). |
| packages/cli/src/rules/inspect.ts | Adds per-engine verify/test logic used by the new CLI commands. |
| packages/cli/src/rules/files.ts | Updates rule file/test writing and deletion to operate on per-rule directories. |
| packages/cli/src/rules/dispatch.ts | Updates Vale presence detection and ast-grep dispatch to use a single assembled config. |
| packages/cli/src/rules/assemble.ts | Adds deterministic assembly for .taskless/.vale.ini and .taskless/.sgconfig.yml. |
| packages/cli/src/index.ts | Registers new top-level verify and test commands. |
| packages/cli/src/help/verify-rule.txt | Updates recipe to the new verify/test commands and path semantics. |
| packages/cli/src/help/route.txt | Updates detect output example paths to the new .taskless/rules/sg location. |
| packages/cli/src/help/improve-rule.txt | Updates rule location references for improving existing rules in the new layout. |
| packages/cli/src/help/improve-rule.anonymous.txt | Updates anonymous improvement flow to use verify/test and new file paths. |
| packages/cli/src/help/detect.txt | Updates rule style source path examples to .taskless/rules/sg. |
| packages/cli/src/help/delete-rule.txt | Updates delete-rule documentation to new layout (but currently overstates engine support). |
| packages/cli/src/help/create-sg-rule.txt | Updates sg authoring recipe for per-rule directory layout and verify/test. |
| packages/cli/src/help/create-runtime-rule.txt | Updates runtime authoring docs for captures/ + new directory layout. |
| packages/cli/src/help/create-remote-rule.txt | Updates remote authoring docs to new per-rule directory + .tests/ layout. |
| packages/cli/src/help/ci.txt | Updates CI documentation paths for runtime rules under the new root. |
| packages/cli/src/help/check.txt | Updates check documentation to new rule locations and per-rule file naming. |
| packages/cli/src/filesystem/sgconfig.ts | Removes legacy sgconfig generation (superseded by assembly). |
| packages/cli/src/filesystem/migrations/0004-vale-engine.ts | Updates docstring notes to clarify StylesPath correctness depends on layout. |
| packages/cli/src/filesystem/migrate.ts | Wires in migration 0005. |
| packages/cli/src/detect/scan.ts | Updates rule style discovery to only use the new .taskless/rules/sg root. |
| packages/cli/src/commands/verify.ts | Adds the new top-level verify and test commands operating on paths. |
| packages/cli/src/commands/rules.ts | Removes rule verify and updates delete errors for the new layout. |
| packages/cli/src/commands/check.ts | Switches check to assembled config generation and new sg rule discovery. |
| openspec/specs/cli-rule-validation/spec.md | Adds the new capability spec for path-addressed validation/testing. |
| openspec/specs/cli-agent-authoring/spec.md | Updates authoring requirements to per-rule config + verify/test usage. |
| openspec/changes/archive/2026-08-15-self-contained-rules/tasks.md | Archives task list for the self-contained rule layout change. |
| openspec/changes/archive/2026-08-15-self-contained-rules/specs/cli-vale-rule-engine/spec.md | Archived delta documenting Vale engine behavior under the new layout. |
| openspec/changes/archive/2026-08-15-self-contained-rules/specs/cli-rule-validation/spec.md | Archived delta documenting verify/test behavior and requirements. |
| openspec/changes/archive/2026-08-15-self-contained-rules/specs/cli-rule-format/spec.md | Archived delta documenting the new canonical rule layout and config assembly. |
| openspec/changes/archive/2026-08-15-self-contained-rules/specs/cli-agent-authoring/spec.md | Archived delta updating agent authoring requirements for per-rule layout. |
| openspec/changes/archive/2026-08-15-self-contained-rules/proposal.md | Archives proposal for the self-contained rules change. |
| openspec/changes/archive/2026-08-15-self-contained-rules/.openspec.yaml | Adds archive metadata for the self-contained rules change. |
| openspec/changes/archive/2026-08-15-agent-command-and-vale-authoring/resume.md | Notes the stacked PR relationship and measured constraints relied on by implementation. |
| example/README.md | Adds human-readable demo documentation for the new layout and commands. |
| example/example.html | Adds example prose file intended to trigger Vale rules. |
| example/example.cjs | Adds example JS file intended to trigger sg rules. |
| example/.taskless/taskless.json | Adds version-5 manifest for the demo project. |
| example/.taskless/rules/vale/no-simply/no-simply.yml | Adds example Vale rule style. |
| example/.taskless/rules/vale/no-simply/.vale.ini | Adds example per-rule Vale config + scope. |
| example/.taskless/rules/vale/no-simply/.tests/pass/direct.md | Adds example passing Vale fixture. |
| example/.taskless/rules/vale/no-simply/.tests/fail/hedged.md | Adds example failing Vale fixture. |
| example/.taskless/rules/sg/no-eval/no-eval.yml | Adds example sg rule. |
| example/.taskless/rules/sg/no-eval/.tests/no-eval-20260814-test.yml | Adds example sg test cases under .tests/. |
| example/.taskless/.gitignore | Ignores assembled config artifacts inside the demo project. |
| eslint.config.js | Excludes example/ from linting (demo includes intentionally “bad” content). |
| .prettierignore | Excludes example/ from formatting (fixtures and demo content are intentionally shaped). |
| .changeset/vale-rule-engine.md | Updates changeset to include new layout and verify/test command changes. |
Suppressed comments (1)
packages/cli/src/help/delete-rule.txt:26
- This removal list still describes deleting
.taskless/rules/<engine>/<id>/, but the currentrule deleteimplementation deletes only the ast-grep rule directory (.taskless/rules/sg/<id>/). Either the command needs an engine-aware argument or this doc should be narrowed to what is actually removed.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| const { stdout } = await execFileAsync("git", [ | ||
| "ls-files", | ||
| "--", | ||
| "example/.taskless", | ||
| ], { cwd: exampleSource }); | ||
| expect(stdout).not.toContain(".vale.ini\n"); | ||
| expect(stdout).not.toContain(".sgconfig.yml"); | ||
| }); |
| ## Preconditions | ||
| - `.taskless/` directory exists. | ||
| - The target rule file exists at `.taskless/sg/rules/<id>.yml`. | ||
| - The target rule directory exists at `.taskless/rules/<engine>/<id>/`. | ||
| - No auth required. |
|
@claude /review |
|
Claude finished @thecodedrift's task in 12m 42s —— View job Claude finished the reviewTodo list
Overall assessmentThis is a well-reasoned refactor — the design docs and the new test suite are unusually thorough about the failure modes they're guarding against (silent-disable, stale config, ambiguous ids). I found one high-severity correctness gap, one real performance bug, a vacuous test assertion, a doc/behavior mismatch, and a couple of smaller cleanup items. All are posted as inline comments; summary below. Most significant — Other findings (see inline comments for detail):
Branch: |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| configPath, | ||
| }); | ||
| results.push(...scan.results); | ||
| if (options.astGrepConfigPath === undefined) { |
There was a problem hiding this comment.
sg and vale no longer resolve their run config the same way — and the asymmetry looks like a real bug for Vale.
Here runAstGrepEngine explicitly skips the scan when astGrepConfigPath === undefined (i.e. when assembleSgConfig found no rules to assemble this run). runValeEngine a few lines below has no equivalent: it decides whether to invoke Vale purely from hasValeRules(cwd) (line 28, now counting rule directories rather than .yml files), then calls runVale({ cwd, paths, timeoutMs }) with no configPath. runVale then falls back to the fixed ASSEMBLED_VALE_CONFIG path (vale/run.ts:120).
That file is only (re)written when assembleValeConfig finds at least one rule contributing a config block (assemble.ts:99-129); when it finds zero blocks it returns undefined without deleting or rewriting whatever's already at .taskless/.vale.ini. check.ts computes assembled.vale but never forwards it here, so there's no way for runValeEngine to know assembly produced nothing.
Concretely: a Vale rule directory exists (style file written) but its .vale.ini is missing or not yet scoped — an ordinary mid-authoring state, and hasValeRules still returns true. If .taskless/.vale.ini was never written before, Vale gets --config pointing at a nonexistent file; if a previous run did write it (for a rule since edited/removed, or a gitignored leftover from another branch), Vale silently lints against stale content instead.
This is exactly the regression the now-deleted mixed-engine-check.test.ts test ("reports nothing and does not fail when no matcher enables the rule") used to guard against, and it wasn't replaced with an equivalent for the new per-rule layout — vale-orchestration.test.ts always calls assembleValeConfig(cwd) before runEngines in its own tests, so the "assembly returned undefined" path through check/runEngines is untested end-to-end.
Suggest either passing the assembled Vale config path into DispatchOptions (mirroring astGrepConfigPath) and skipping when undefined, or having assembleValeConfig/assembleSgConfig delete the target file when they have nothing to assemble.
| const { engine, ruleId } = rule; | ||
|
|
||
| if (engine === "sg") { | ||
| const result = await verifyRule(cwd, ruleId); |
There was a problem hiding this comment.
test <path> on an sg rule spawns sg test twice.
verifyRule (verify.ts:252-354) always runs Layer 3 whenever a test file exists — runTests (verify.ts:161-248) assembles .taskless/.sgconfig.yml and spawns the sg test subprocess — regardless of which fields the caller actually wants back.
verifyOneRule's sg branch (line 55, above) calls verifyRule just to read schema/requirements errors, discarding result.tests — but that call already ran the full test suite as a side effect. testOneRule then calls verifyOneRule (line 172, incurring that spawn) and, once verification passes, calls verifyRule again here (line 180) to get the test results it actually needs.
Net effect: one taskless test <path> on one sg rule with a .tests/ directory spawns sg test and rewrites the assembled sgconfig twice instead of once. Not incorrect, but it doubles latency for every sg rule — worse for test .taskless/rules running over many rules, or in CI.
Worth having verifyOneRule's sg branch call something that validates schema/requirements without running Layer 3 (or have testOneRule reuse the tests result already computed by the first call instead of invoking verifyRule a second time).
| // A rule is one directory, so deleting it is removing that directory. Its | ||
| // tests live inside, which is the point of the layout: there is no second | ||
| // place to remember, and no way to leave a rule half-deleted. | ||
| const directory = ruleDirectory(cwd, "sg", id); |
There was a problem hiding this comment.
deleteRuleFiles only ever deletes the sg engine's directory, but the updated docs now claim engine-generic deletion.
ruleDirectory(cwd, "sg", id) is hardcoded here — there's no engine parameter, and the caller (commands/rules.ts delete command) doesn't discover which engine owns id either (the old rulefileOwners/ruleFileLocation helpers in rules/owner.ts were deleted by this PR and nothing replaced the ambiguity/ownership check for delete). The not-found error message it prints is also sg-only: Rule "${id}" not found in .taskless/${RULES_DIRECTORY}/sg/${id}/.
This was already true before this PR (the base branch's deleteRuleFiles was also sg-only), so it's not a new runtime regression — but packages/cli/src/help/delete-rule.txt is changed by this diff to say the CLI removes .taskless/rules/<engine>/<id>/ generically ("the whole directory, which holds the rule, its .tests/, and any per-engine config"). That's now actively misleading for an agent reading the doc: running taskless rule delete <id> against a vale or runtime rule always reports RULE_NOT_FOUND, even though the directory exists, because only sg/ is ever probed.
Either scope the doc back down to sg (matching actual behavior, as the pre-PR doc did), or make deleteRuleFiles/the delete command engine-aware (resolve the engine from where the id actually lives, the same way resolve-path.ts does for verify/test).
| "ls-files", | ||
| "--", | ||
| "example/.taskless", | ||
| ], { cwd: exampleSource }); |
There was a problem hiding this comment.
This assertion is vacuous — it never actually inspects any files.
exampleSource is already resolved to <repo>/example (line 13). Running git ls-files -- example/.taskless with cwd: exampleSource makes git look for <repo>/example/example/.taskless, which doesn't exist, so stdout is always "". Both expect(stdout).not.toContain(...) assertions below then pass unconditionally, regardless of whether an assembled .vale.ini/.sgconfig.yml is actually committed under example/.taskless.
Either drop the example/ prefix from the pathspec (since cwd is already inside it), or run from the repo root and keep the example/.taskless prefix — e.g.:
const { stdout } = await execFileAsync("git", ["ls-files", "--", ".taskless"], { cwd: exampleSource });Separately, once this is fixed to actually list files, note the assertions as written (not.toContain(".vale.ini\n") / not.toContain(".sgconfig.yml")) would also reject a legitimately committed per-rule .vale.ini (e.g. example/.taskless/rules/vale/no-simply/.vale.ini, which this PR's own example project commits), not just the assembled build artifact at example/.taskless/.vale.ini. The check likely needs to be scoped to the assembled paths specifically rather than any file matching those basenames.
| const rulesStat = await stat(join(tasklessDirectory, "rules", "sg")); | ||
| expect(rulesStat.isDirectory()).toBe(true); | ||
| const testsStat = await stat(join(tasklessDirectory, "sg", "rule-tests")); | ||
| const testsStat = await stat(join(tasklessDirectory, "rules", "sg")); |
There was a problem hiding this comment.
Minor: testsStat now stats the exact same path as rulesStat (rules/sg twice). Before this PR these checked two different directories (sg/rules and sg/rule-tests); the rename collapsed both to rules/sg, so this second assertion no longer adds coverage — it's a redundant duplicate of the line above rather than verifying a second thing (e.g. rules/vale or rules/runtime also scaffolded).
| blocks.push(valeRuleBlock(ruleId, body)); | ||
| } | ||
|
|
||
| if (blocks.length === 0) return undefined; |
There was a problem hiding this comment.
check can end up running Vale against a stale or missing config.
When blocks.length === 0 this returns undefined without writing — and without deleting any .taskless/.vale.ini left over from an earlier run. In commands/check.ts the return value is captured (const assembled = await assembleEngineConfigs(cwd)) but assembled.vale is never used — it's discarded. runValeEngine in dispatch.ts (hasValeRules(cwd) gate, then runVale({ cwd, paths, timeoutMs }) with no configPath) never learns whether assembly actually produced a fresh file this run; runVale just defaults to the fixed ASSEMBLED_VALE_CONFIG path.
Compare with the ast-grep side: check.ts explicitly passes astGrepConfigPath: assembled.sg, and runAstGrepEngine in dispatch.ts returns { engine: "sg", results: [] } when that's undefined — Vale has no equivalent skip.
Concretely: a Vale rule directory can exist (e.g. the style file is written but .vale.ini isn't yet, or a rule's .vale.ini was deleted) while hasValeRules still sees the directory and returns true. assembleValeConfig then contributes zero blocks and returns undefined here, leaving whatever's already at .taskless/.vale.ini untouched — either nothing (Vale then gets --config pointed at a nonexistent file) or a stale config from a previous check run. This is exactly the "rule directory present, no matcher enables it" state the now-deleted mixed-engine-check.test.ts test ("reports nothing and does not fail when no matcher enables the rule") used to guarantee is handled quietly — no equivalent test covers it for the new per-rule layout, and vale-orchestration.test.ts's dispatch tests always call assembleValeConfig(cwd) themselves before asserting, so they never exercise "assembly ran but produced nothing" through runEngines.
Worth either threading the assembled vale path through to runValeEngine (mirroring astGrepConfigPath) and skipping when undefined, or deleting the target file here when there's nothing to assemble.
| if (engine === "sg") { | ||
| // The ast-grep verifier already layers schema and required fields; only its | ||
| // test layer is `test`'s business, so it is not consulted here. | ||
| const result = await verifyRule(cwd, ruleId); | ||
| errors.push(...result.schema.errors, ...result.requirements.errors); | ||
| return { engine, ruleId, ok: errors.length === 0, errors }; |
There was a problem hiding this comment.
test <path> spawns sg test twice per ast-grep rule.
verifyOneRule calls verifyRule(cwd, ruleId) (line 55) and uses only result.schema.errors/result.requirements.errors — but verifyRule (rules/verify.ts:252-354) unconditionally runs Layer 3 (runTests, which spawns the sg test subprocess via assembleSgConfig + spawn) whenever a test file exists, regardless of what the caller wants.
testOneRule (below, line ~172) calls verifyOneRule first — which itself already ran a full verifyRule including the subprocess spawn — and then, on success, calls verifyRule(cwd, ruleId) again (line ~180) purely to read result.tests. So one taskless test <path> on one sg rule spawns sg test twice, and assembleSgConfig gets re-run twice too. Over test .taskless/rules with N sg rules this doubles the subprocess cost for no benefit.
Consider splitting verifyRule's schema/requirements layers from its test-running layer so verifyOneRule doesn't pay for a test run it discards, or having testOneRule reuse the single verifyRule result instead of calling it twice.
| const { stdout } = await execFileAsync("git", [ | ||
| "ls-files", | ||
| "--", | ||
| "example/.taskless", | ||
| ], { cwd: exampleSource }); | ||
| expect(stdout).not.toContain(".vale.ini\n"); | ||
| expect(stdout).not.toContain(".sgconfig.yml"); | ||
| }); |
There was a problem hiding this comment.
This assertion is vacuous. cwd: exampleSource is already <repo>/example (per the constant at the top of the file), so the pathspec "example/.taskless" resolves to the nonexistent example/example/.taskless — git ls-files will always print an empty string here, regardless of what's actually committed under example/.taskless. expect(stdout).not.toContain(...) therefore passes unconditionally and this test provides no real protection against committing the generated .vale.ini/.sgconfig.yml.
Either drop example/ from the pathspec (since cwd is already there) or run from the repo root and keep the full example/.taskless pathspec. Also worth tightening the assertion to only reject the assembled artifacts (.taskless/.vale.ini, .taskless/.sgconfig.yml) rather than any .vale.ini/.sgconfig.yml anywhere under .taskless, since a legitimate per-rule .vale.ini (e.g. example/.taskless/rules/vale/no-simply/.vale.ini, which should be committed) would otherwise also match .vale.ini and be incorrectly flagged if the pathspec bug were fixed as-is.
|
|
||
| ## Preconditions | ||
| - `.taskless/` directory exists. | ||
| - The target rule file exists at `.taskless/sg/rules/<id>.yml`. | ||
| - The target rule directory exists at `.taskless/rules/<engine>/<id>/`. | ||
| - No auth required. |
There was a problem hiding this comment.
This now claims deletion works for .taskless/rules/<engine>/<id>/ generically, but deleteRuleFiles (packages/cli/src/rules/files.ts:110-127) still hardcodes ruleDirectory(cwd, "sg", id) — it only ever removes an ast-grep rule. A vale or runtime rule with the same id can never be deleted through rule delete <id>, and commands/rules.ts's error message on a miss also only ever names the sg path (.taskless/${RULES_DIRECTORY}/sg/${id}/).
This is a pre-existing limitation (the base branch's deleteRuleFiles was ast-grep-only too), but the base branch's doc accurately described that (.taskless/sg/rules/<id>.yml) — this diff changes the doc to overclaim engine-generic support without changing the implementation. Either narrow the doc back to sg, or make deleteRuleFiles engine-aware (accepting/discovering the engine the same way resolve-path.ts does for verify/test).
| function retargetCheckName(lines: string[], ruleId: string): string[] { | ||
| const escaped = ruleId.replaceAll(/[$()*+.?[\\\]^{|}]/g, String.raw`\$&`); | ||
| const assignment = new RegExp(String.raw`^(\s*)rules\.` + escaped + String.raw`(\s*=)`); | ||
| return lines.map((line) => line.replace(assignment, `$1${ruleId}.${ruleId}$2`)); | ||
| } | ||
|
|
||
| /** | ||
| * Split the committed `vale/.vale.ini` into per-rule configs. | ||
| * | ||
| * Each matcher carrying a `tskl) rule = <id>` breadcrumb belongs to that rule. | ||
| * A matcher **without** one cannot be attributed: it is a user's hand edit, and | ||
| * guessing an owner or dropping it would silently change what their check | ||
| * reports. Those are left in place and reported, so the file stays on disk with | ||
| * only the unattributable part remaining. | ||
| */ | ||
| async function splitValeConfig(directory: string): Promise<string[]> { | ||
| const configPath = join(directory, "vale", ".vale.ini"); | ||
| let source: string; | ||
| try { | ||
| source = await readFile(configPath, "utf8"); | ||
| } catch { | ||
| return []; | ||
| } | ||
|
|
||
| const lines = source.split("\n"); | ||
| const blocks: Array<{ ruleId?: string; lines: string[] }> = []; | ||
| let current: { ruleId?: string; lines: string[] } | undefined; | ||
|
|
||
| for (const line of lines) { | ||
| if (line.trimStart().startsWith("[")) { | ||
| if (current !== undefined) blocks.push(current); | ||
| current = { lines: [line] }; | ||
| continue; | ||
| } | ||
| if (current === undefined) continue; // header: StylesPath / MinAlertLevel | ||
| current.lines.push(line); | ||
| const breadcrumb = /^\s*tskl\)\s*rule\s*=\s*(?<id>\S+)/.exec(line); |
There was a problem hiding this comment.
The breadcrumb this depends on was never written by any pre-this-PR code path or doc.
splitValeConfig/retargetCheckName attribute a committed .vale.ini's matchers back to a rule id via a tskl) rule = <id> comment. But diffing this PR against origin/openspec/agent-command-and-vale-authoring shows the tskl) breadcrumb concept is introduced by this PR — the pre-PR create-vale-rule.txt and commands/rules.ts never told an author (or wrote programmatically) a breadcrumb into the committed vale/.vale.ini; migration 0004's own scaffold (VALE_CONFIG_CONTENT in 0004-vale-engine.ts) writes header-only content with no breadcrumb either. So a genuinely 0004-era .vale.ini populated per the old docs ([*.md]\nrules.<id> = YES, no breadcrumb) would have every block fail attribution here, get left behind as "orphans," and zero per-rule .vale.ini files would be created — silently disabling every Vale rule post-migration, which is exactly the failure class this whole PR is meant to prevent.
migrate-round-trip.test.ts's seedVersion4Project fixture only exercises the happy path because it hand-seeds a tskl) rule = no-simply line that no real pre-PR output would ever contain.
I think this is mitigated by the proposal's claim that "0004 and 0005 always run together — both are unreleased," so no real installation is ever in the intermediate 0004-only state this breaks on. Worth double-checking that's airtight (no canary/beta builds, no one who manually followed the pre-PR docs) before relying on it, since the migration code and its own tests read as if this path is meant to be reachable.
| // A rule directory, or a stray file where one would be. | ||
| const stats = await statOrUndefined(absolute); | ||
| if (stats === undefined) throw new RuleNotFoundError(target); | ||
| if (!stats.isDirectory()) return [{ engine, ruleId }]; | ||
| return [{ engine, ruleId }]; |
There was a problem hiding this comment.
Both arms of if (!stats.isDirectory()) return the identical value ([{ engine, ruleId }]), so the stat/isDirectory() check does nothing here — it can collapse to a single return [{ engine, ruleId }];. Minor, but the branching reads as though a stray-file-where-a-rule-directory-should-be case is handled specially, when it isn't; worth either actually rejecting that case (e.g. throwing RuleNotFoundError when it's a file, not a directory) or removing the dead branch.
| const results = []; | ||
| for (const rule of rules) { | ||
| results.push(await run(cwd, rule)); |
There was a problem hiding this comment.
Rules are checked strictly sequentially here, but verifyOneRule/testOneRule are independent per-rule work that shell out to separate subprocesses (sg test, Vale) with no ordering dependency between rules. For verify .taskless/rules / test .taskless/rules over N rules, this pays N sequential subprocess round-trips instead of running them concurrently (e.g. Promise.all), which directly inflates CI wall-clock time as the rule count grows — the same concurrency runEngines already uses across engines in dispatch.ts.
Stack (root → tip):
Why
A rule is spread across locations today, and for Vale one of them is shared by every rule in the project. Five sandboxed harness runs against
create-vale-rule(on #102) found silent failures in that shared.vale.iniand nowhere else — an assignment above the first matcher, a glob that missed the fixture's extension, three names that had to agree with nothing reporting when they didn't.The same reasoning generalizes past Vale:
sgandruntimerules are also split between arules/tree and a parallelrule-tests/tree, so no engine has a single path meaning "this rule". Fixing Vale alone would leave three layouts instead of one.What changes
One directory per rule, identical across engines:
checkassembles the Vale and ast-grep configs from the per-rule tree and gitignores them.rule verify <id>is replaced by path-addressedverify <path>(required components) andtest <path>(tests), withverifyrunning as a prerequisite layer oftest.Adds
example/— a small real project so a reader can see an install rather than infer it from tests that build their own fixtures, with achecktest so it can't drift.The dot on
.tests/is load-bearing, and measuredtests/inside the rule dirmissing field 'language'__tests__/.tests/.ymltestDir: …/.testsworks; snapshots land insideruleDirsrecurses and parses every.ymlbeneath as a rule, so a plaintests/directory hard-fails the scan.This is a dependency on undocumented behavior and D2 records it as one, with two mitigations: the failure is loud — a parse error naming the file, not a test silently reinterpreted as a rule — and a test pins it. The rejected alternative (materialize a rules-only tree for ast-grep, keeping a plain
tests/) is written down as the fallback if the assumption breaks.Other measured constraints
<id>/<id>.ymlresolves as check<id>.<id>only under aStylesPathnaming its parent; underStylesPath = .it resolves to nothing. This reverses the note in migration0004— correct for the flat layout, backwards for this one — so the task list rewrites that docstring rather than deleting it.E201), so scope cannot ride inside the style file. ast-grep can express scoping in the rule, which is why it gets no per-rule config — an empty file per rule is symmetry as decoration.Delivery shape
Stacked, merging down on #102.
0005layers on0004; both are unreleased and both ship in this stack, so consumers run them as one upgrade and never observe the intermediate layout.openspec validate --all --strictwill flag thecli-agent-authoringdelta until #102 archives — that capability is introduced there.Spec-only. Implementation follows on approval.
🤖 Generated with Claude Code
https://claude.ai/code/session_01Jwc9FFroR3mTZ4hLiSkkX3