feat(cli): dispatch rules by engine directory by thecodedrift · Pull Request #80 · taskless/skills

thecodedrift · 2026-08-02T17:18:13Z

Stack (root → tip):

feat(cli): add migration 0004 for the engine-partitioned .taskless layout #79
- ➡️ feat(cli): dispatch rules by engine directory #80 (you are here)
  - feat(cli): discover runtime rules under the runtime engine directory #81
    - feat(cli): run ast-grep against the committed sg config #73

The directory a rule sits in under .taskless/ is its engine. Dispatch reads the path and never parses a rule file to decide who owns it, so adding an engine is a directory, not a discriminator.

planEngineDispatch resolves which engine directories are present; an unrecognized directory is ignored rather than guessed at, so a .taskless/ written by a newer CLI degrades to running the engines this one understands instead of handing a rule to the wrong parser.

Two compatibility behaviors are deliberate and worth reviewing as such:

Pre-0004 .taskless/rules/ still runs as ast-grep. An unmigrated checkout, or a producer that keeps naming the old path, executes rather than being silently ignored. Both sources are scanned and findings de-duplicated on the match itself, since a rule id can legitimately exist in only one of them.
A delivered rule that names no engine is ast-grep. The delivery API carries no engine discriminator — rules[].content is documented as an ast-grep definition — so absence means ast-grep, permanently, not for a migration window. An unrecognized engine is the opposite case: it means the payload is newer than the CLI, so it throws and writes nothing rather than filing the rule where the wrong parser will read it.

Part of a merge-down stack (see the base branch). Not independently shippable: rules have moved but runtime discovery still reads the old path until the next PR.

Refs OSS-24

Built on top of #79

Migration 0004 moves .taskless/ to an engine-partitioned layout: ast-grep rules go to sg/rules/ and sg/rule-tests/, the runtime tree to runtime/rules/ and runtime/rule-tests/, and vale/ is scaffolded but inert until the Vale engine lands. Files move without their bytes being touched, so runtime rule signatures survive the migration.

Also anchors the sgconfig.yml gitignore pattern to .taskless/. The pattern was unanchored, so it would match a same-named file anywhere in the tree.

This PR carries the OpenSpec change docs for the whole effort, including the spec deltas the later PRs implement.

This is the bottom of a merge-down stack — the units are only correct together, since this migration relocates rules that nothing yet knows how to find. Nothing here reaches main on its own: the PRs above merge down into this branch, and the accumulated result is what merges once. The change directory stays unarchived here by design; the archive gate skips non-tip PRs and the tip archives it.

Refs OSS-24

On a stack, one changeset describes the whole change and lives on the bottom PR — the one targeting `main`, which is where the check has to pass. Branches above inherit that file, so it is never *added* in a child's own diff and every mid-stack PR failed. Labelling them `skip-changeset` records a deliberate "ships no release note," which is false, so the workflow bypasses on the base ref instead. The `on: branches: [main]` filter does not hold for stacked PRs in practice — the workflow still runs on a PR based on a feature branch — so the base is re-checked inside the step rather than trusted. Observed on #80, #81, and #73, all of which ran and failed the check with a non-main base.

…laim Review pushed back on three statements, and two of them were wrong. `propagate_stack.py` does return you to the branch you started on — normal completion, rebase conflict, balloon guard, and push failure all check it out before returning. Only a failure to check out a *child* (exit 6, typically that branch being held by a worktree) returns early without it. That is the path that actually bit, so the warning now names it instead of describing the script's normal behavior. The `branches: [main]` claim is reframed rather than dropped. It is documented to filter on the base branch, and the guard should be unreachable — but #73, #80, and #81 each produced a failing `Require a changeset` check run with an `openspec/partition-engine-*` base, and a workflow that never triggers produces no check run at all. The comment now cites that evidence and presents the check as a defensive guard, without teaching that Actions filters are unreliable in general. Also replaces the "structural failures are acceptable" guidance with the opposite rule: a mid-stack PR that is red is incomplete, and the fix belongs in it. That advice let this stack sit on 20 failures that turned out to be two real defects.

…l target `branches: [main]` reads as "only PRs whose base is main," and filtered that way when a PR had one base. Under GitHub's stacked-PR support a stacked PR targets `main` eventually, so the filter matches the eventual target and these workflows run on mid-stack PRs too — which is why #73, #80, and #81 each produced a failing `Require a changeset` run with an `openspec/partition-engine-*` base. That makes the base-ref guard required rather than defensive, and generalizes: a workflow whose correctness depends on "is this the PR that merges to main" has to establish that itself. Corrects the OpenSpec archive-check section too, which told readers a stacked PR would not run that check at all.

On a stack, one changeset describes the whole change and lives on the bottom PR — the one targeting `main`, which is where the check has to pass. Branches above inherit that file, so it is never *added* in a child's own diff and every mid-stack PR failed. Labelling them `skip-changeset` records a deliberate "ships no release note," which is false, so the workflow bypasses on the base ref instead. The `on: branches: [main]` filter does not hold for stacked PRs in practice — the workflow still runs on a PR based on a feature branch — so the base is re-checked inside the step rather than trusted. Observed on #80, #81, and #73, all of which ran and failed the check with a non-main base.

…laim Review pushed back on three statements, and two of them were wrong. `propagate_stack.py` does return you to the branch you started on — normal completion, rebase conflict, balloon guard, and push failure all check it out before returning. Only a failure to check out a *child* (exit 6, typically that branch being held by a worktree) returns early without it. That is the path that actually bit, so the warning now names it instead of describing the script's normal behavior. The `branches: [main]` claim is reframed rather than dropped. It is documented to filter on the base branch, and the guard should be unreachable — but #73, #80, and #81 each produced a failing `Require a changeset` check run with an `openspec/partition-engine-*` base, and a workflow that never triggers produces no check run at all. The comment now cites that evidence and presents the check as a defensive guard, without teaching that Actions filters are unreliable in general. Also replaces the "structural failures are acceptable" guidance with the opposite rule: a mid-stack PR that is red is incomplete, and the fix belongs in it. That advice let this stack sit on 20 failures that turned out to be two real defects.

…l target `branches: [main]` reads as "only PRs whose base is main," and filtered that way when a PR had one base. Under GitHub's stacked-PR support a stacked PR targets `main` eventually, so the filter matches the eventual target and these workflows run on mid-stack PRs too — which is why #73, #80, and #81 each produced a failing `Require a changeset` run with an `openspec/partition-engine-*` base. That makes the base-ref guard required rather than defensive, and generalizes: a workflow whose correctness depends on "is this the PR that merges to main" has to establish that itself. Corrects the OpenSpec archive-check section too, which told readers a stacked PR would not run that check at all.

thecodedrift · 2026-08-03T19:21:51Z

@claude /review

claude · 2026-08-03T19:22:06Z

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

Review: dispatch rules by engine directory

Reviewed the diff against origin/openspec/partition-engine-1-migration (10 files, +728/-127). The directory-based dispatch model (rules/engines.ts) and its two compatibility behaviors (legacy .taskless/rules/ still runs as ast-grep; an engine-less delivered rule defaults permanently to sg) are implemented cleanly and match the PR description. test/engine-dispatch.test.ts is thorough for the dispatch/dedup/ingest logic it covers.

Two issues worth fixing before merge, plus a minor nit — posted as inline comments:

1. packages/cli/src/rules/engines.ts contains a literal raw NUL byte, not the text \0.
Confirmed via hex dump: dedupeFindings's .join("\0") has an actual 0x00 byte between the quotes, not the two ASCII characters \ and 0. This is why the PR's file list shows this brand-new 257-line file as +0/-0 and git diff --stat reports Bin 0 -> 8435 bytes — git's binary-file heuristic trips on the NUL byte, so the file renders as a binary diff in GitHub's UI and can't be reviewed line-by-line there (I could only inspect it by reading the file directly and hex-dumping it; inline comments on it are rejected by GitHub's API for the same reason). It also breaks plain-text tooling (grep without -a, file, etc.) on this file. The dedup-key logic itself is fine; the fix is just replacing the raw byte with the literal "\0" escape sequence (or any printable delimiter) so the file stays normal text. I couldn't leave an inline comment on the file itself for the reason above — flagging here.

2. An unrecognized engine from the server silently no-ops rule create/rule improve (exit 0, no message). (packages/cli/src/rules/files.ts:30, via resolveIngestEngine in rules/engines.ts)
resolveIngestEngine throws a bare CLIError with no prior console output. The top-level handler in index.ts treats any CLIError as "already printed output, exitCode already set" and does nothing further — an invariant the other CLIError sites (commands/rules.ts's fail() helper) uphold by printing before throwing, but this new throw site doesn't. Since writeRuleFile/writeRuleTestFile are called directly in commands/rules.ts's createCommand/improveCommand with no surrounding try/catch, the error propagates all the way to the top and is swallowed: the CLI exits 0 with no error text, looking like success. With multiple rules in one batch, earlier rules may already be written to disk before the throw, so a run can silently partially succeed. The "write nothing for this rule" behavior itself is correct — the gap is that the failure never reaches the user. engine-dispatch.test.ts's coverage of this path calls writeRuleFile directly rather than through the rule create/rule improve command, so it wouldn't catch this.

3. (Minor/nit) commands/check.ts's runtimeEnabled gate compares dispatch.find(...).executor to "runtime-harness", but that value is static (never varies with whether .taskless/runtime/ is actually present on disk — present isn't read anywhere in src/). Not a functional bug since discoverRuntimeRules degrades gracefully on a missing directory anyway, just reads as more disk-dependent than it is.

Everything else — verify.ts's layout resolution (sg-then-legacy, remembering which layout won for the test run), files.ts's delete-from-every-layout logic, sgconfig.ts's default change, and the detect/scan.ts rule-style probe — looks correct and consistent with the stated compatibility guarantees.
(branch)

Copilot

Pull request overview

Updates the CLI to treat the directory a rule lives in under .taskless/ as the engine discriminator (rather than inspecting rule contents), enabling engine-partitioned layouts while retaining compatibility with the legacy .taskless/rules/ path.

Changes:

Update check to discover ast-grep rule sources by engine directory (plus legacy), scan each source, and de-duplicate identical findings.
Update rule verify + service-delivered rule ingest to resolve/write rules and tests into the engine-partitioned sg/ directories (with legacy fallback where applicable).
Add/expand tests covering sgconfig generation, engine dispatch behavior, ingest behavior, and de-duplication.

Reviewed changes

Copilot reviewed 9 out of 10 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
packages/cli/test/sgconfig.test.ts	Updates sgconfig generation expectations and adds coverage for legacy layout options.
packages/cli/test/engine-dispatch.test.ts	Adds new tests for directory-based engine dispatch, legacy-source behavior, de-duping, and ingest routing.
packages/cli/src/rules/verify.ts	Verifies rules across engine/legacy layouts and runs `sg test` against the resolved layout.
packages/cli/src/rules/files.ts	Routes rule/test file writes by resolved ingest engine; deletes rule files across both layouts.
packages/cli/src/filesystem/sgconfig.ts	Allows generating sgconfig for an explicit rules+tests directory pair (defaults to `sg/*`).
packages/cli/src/detect/scan.ts	Detects rule styles from either the new `sg/rules` layout or the legacy `rules` layout.
packages/cli/src/commands/rules.ts	Updates delete command messaging to reference the engine-partitioned path.
packages/cli/src/commands/check.ts	Switches static-rule discovery to engine sources + legacy, scans per source, and de-dupes results.
openspec/changes/partition-rules-by-engine/tasks.md	Updates task tracking/status for the engine-partition rollout.

Suppressed comments (1)

packages/cli/src/rules/verify.ts:304

If the rule file is not found, the returned error message only references the sg/rules path even though the resolver also checks the legacy .taskless/rules/ path. Including both candidate locations makes the error accurate for pre-migration trees.

      schema: {
        valid: false,
        errors: [
          `Rule file not found: .taskless/${ENGINE_LAYOUTS.sg.rulesDirectory}/${ruleId}.yml`,
        ],

Add `rules/engines.ts` as the single place that answers "which engine owns this rule": the top-level `.taskless/<engine>/` directory does, and no rule file is ever parsed to decide. `sg` maps to ast-grep and `runtime` to the harness; `vale` is recognized but has no executor yet, and a directory that is not a known engine is ignored rather than handed to someone else's parser. `check` now calls `ensureTasklessDirectory` itself, keeping the migration trigger that `generateSgConfig` used to provide, then scans each ast-grep source it finds. The legacy `.taskless/rules/` stays readable alongside `sg/rules/`, so a producer that keeps naming the old path still runs; findings from the two are merged and identical matches collapsed. Service-delivered rules are filed by the engine their payload identifies — `sg` when it identifies none, permanently, since the API carries no engine discriminator. An engine this CLI does not know throws before touching the filesystem instead of silently defaulting to ast-grep. The remaining hardcoded `.taskless/rules` literals (verify, files, check, rules, the detect probe) now resolve through the same module, each tolerating both layouts. Reconcile needed no change: reported paths derive from the discovery root and the server joins on content signature, both verified by test. Test failures drop from 20 to 9, all in runtime-check.test.ts (group 3). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

…both layouts Review on #80 surfaced two real defects and three misleading messages. `engines.ts` held a literal NUL byte where `dedupeFindings` joins its key, rather than the two-character escape. Git's binary heuristic tripped on it, so a brand-new 257-line file rendered as `Bin 0 -> 8435 bytes` and could not be reviewed line-by-line on GitHub, inline comments on it were rejected, and plain `grep` reported only "Binary file matches". It now uses an escape sequence. An unrecognized engine from the server exited 0 with no output. `index.ts` treated every `CLIError` as "already printed, exit code already set" — true of the `fail()` helpers, false of `resolveIngestEngine`, which throws bare. A failure therefore read as success. `CLIError` now carries `reported`, defaulting to false so reporting is a claim a caller makes rather than something assumed; the handler prints and sets a non-zero exit otherwise. This also fixes the same latent silence for SCAFFOLD_VERSION_MISMATCH and SCAFFOLD_CONFLICT. `runtimeEnabled` read only the engine's executor, which comes from the static layout table and is always `runtime-harness` — so it was unconditionally true and the presence check was decorative. It now requires `present` as well. `check` no longer scaffolds `.taskless/` in a project that has none: it is a read-only command, and doing so also broke on read-only checkouts. The rule-delete, rule-file, and test-file not-found messages named only the `sg` paths while the lookups also check the legacy layout, pointing users on an unmigrated tree at a location their rule was never in.

thecodedrift · 2026-08-05T21:46:57Z

Re: @claude[bot] — "Claude finished @thecodedrift's task — Review: dispatch rules by engine directory … Two issues worth fixing before merge, plus a minor nit"
#80

Both issues were real and are fixed; details on the inline threads.

The NUL byte is confirmed — offset 6296, in dedupeFindings's join. Your diagnosis of the consequence was exactly right, and it explains something I saw and did not chase: my own git show --stat reported Bin 8435 -> 8997 bytes for this file earlier and I read past it. Now replaced with an escape sequence, and the PR diff renders 256 0 — line-by-line reviewable, so inline comments on it will work from here.

The silent exit 0 was the more valuable find, and I fixed the invariant rather than the call site. CLIError now carries reported, defaulting to false, so "already printed" is something a throw site claims rather than something the handler assumes. That also closes the identical silence at two throw sites neither of us had listed — SCAFFOLD_VERSION_MISMATCH and SCAFFOLD_CONFLICT, both of which would have exited 0 with no output.

Suite is 419 on this branch, with new tests for the reporting contract and for check not scaffolding .taskless/.

— AI Coding Agent

thecodedrift · 2026-08-05T21:47:07Z

Re: @copilot-pull-request-reviewer — "## Pull request overview — Updates the CLI to treat the directory a rule lives in under .taskless/ as the engine discriminator…"

All four findings addressed (replied and resolved inline): runtimeEnabled now gates on present as well as the executor, the rule-delete and both verify.ts not-found messages name the legacy layout alongside sg, and check no longer scaffolds .taskless/ in a project that has none. The last one had been guarded in an earlier revision and was lost when this branch was reconciled against its parent — good catch.

— AI Coding Agent

One change gets one changeset. When a stack lands forward rather than as a single merge-down, every PR after the first finds that changeset already on `main` from the PR below it — so its own diff adds none and the check fails, even though the release note exists and covers it. Counting additions only also made the "grow the changeset as the stack lands" rule in CLAUDE.md impossible to satisfy, since growing it is a modification. The check now counts added OR modified changesets, and this PR extends the existing note with the behaviour it actually adds rather than filing a second entry for something that ships once.

…both layouts Review on #80 surfaced two real defects and three misleading messages. `engines.ts` held a literal NUL byte where `dedupeFindings` joins its key, rather than the two-character escape. Git's binary heuristic tripped on it, so a brand-new 257-line file rendered as `Bin 0 -> 8435 bytes` and could not be reviewed line-by-line on GitHub, inline comments on it were rejected, and plain `grep` reported only "Binary file matches". It now uses an escape sequence. An unrecognized engine from the server exited 0 with no output. `index.ts` treated every `CLIError` as "already printed, exit code already set" — true of the `fail()` helpers, false of `resolveIngestEngine`, which throws bare. A failure therefore read as success. `CLIError` now carries `reported`, defaulting to false so reporting is a claim a caller makes rather than something assumed; the handler prints and sets a non-zero exit otherwise. This also fixes the same latent silence for SCAFFOLD_VERSION_MISMATCH and SCAFFOLD_CONFLICT. `runtimeEnabled` read only the engine's executor, which comes from the static layout table and is always `runtime-harness` — so it was unconditionally true and the presence check was decorative. It now requires `present` as well. `check` no longer scaffolds `.taskless/` in a project that has none: it is a read-only command, and doing so also broke on read-only checkouts. The rule-delete, rule-file, and test-file not-found messages named only the `sg` paths while the lookups also check the legacy layout, pointing users on an unmigrated tree at a location their rule was never in.

This PR became the bottom of the stack when #80 merged, so the release note has to be visible in its own diff. One change gets one changeset: this extends the existing note rather than adding a second entry for something that ships once.

This was referenced Aug 2, 2026

feat(cli): add migration 0004 for the engine-partitioned .taskless layout #79

Merged

feat(cli): discover runtime rules under the runtime engine directory #81

Merged

feat(cli): run ast-grep against the committed sg config #73

Merged

thecodedrift force-pushed the openspec/partition-engine-2-dispatch branch from 655179e to 34ee5f2 Compare August 2, 2026 18:31

thecodedrift added the skip-changeset PR intentionally ships no release note (bypasses the changeset requirement) label Aug 2, 2026

thecodedrift force-pushed the openspec/partition-engine-2-dispatch branch from 34ee5f2 to 720c831 Compare August 2, 2026 21:36

thecodedrift removed the skip-changeset PR intentionally ships no release note (bypasses the changeset requirement) label Aug 2, 2026

thecodedrift mentioned this pull request Aug 2, 2026

fix(ci): bypass the changeset requirement on mid-stack PRs #83

Merged

thecodedrift force-pushed the openspec/partition-engine-2-dispatch branch from 720c831 to f02ab43 Compare August 3, 2026 05:21

thecodedrift force-pushed the openspec/partition-engine-2-dispatch branch from f02ab43 to 2837632 Compare August 3, 2026 18:58

thecodedrift marked this pull request as ready for review August 3, 2026 19:20

Copilot AI review requested due to automatic review settings August 3, 2026 19:20

Copilot started reviewing on behalf of thecodedrift August 3, 2026 19:21 View session

Copilot AI reviewed Aug 3, 2026

View reviewed changes

Comment thread packages/cli/src/commands/check.ts

Comment thread packages/cli/src/commands/rules.ts Outdated

Comment thread packages/cli/src/commands/check.ts

Comment thread packages/cli/src/rules/verify.ts

claude Bot reviewed Aug 3, 2026

View reviewed changes

Comment thread packages/cli/src/rules/files.ts

claude Bot reviewed Aug 3, 2026

View reviewed changes

Comment thread packages/cli/src/commands/check.ts Outdated

thecodedrift force-pushed the openspec/partition-engine-2-dispatch branch from 2837632 to ee5ef75 Compare August 3, 2026 21:06

thecodedrift force-pushed the openspec/partition-engine-2-dispatch branch from ee5ef75 to 286af03 Compare August 4, 2026 15:05

Base automatically changed from openspec/partition-engine-1-migration to main August 4, 2026 15:07

thecodedrift force-pushed the openspec/partition-engine-2-dispatch branch from 286af03 to 70c21a5 Compare August 4, 2026 16:05

Copilot started work on behalf of thecodedrift August 5, 2026 21:47 View session

Copilot finished work on behalf of thecodedrift August 5, 2026 21:47

thecodedrift merged commit 7b662e5 into main Aug 6, 2026
4 checks passed

thecodedrift deleted the openspec/partition-engine-2-dispatch branch August 6, 2026 02:35

Navigation Menu

Uh oh!

feat(cli): dispatch rules by engine directory - #80

feat(cli): dispatch rules by engine directory#80
thecodedrift merged 3 commits into
mainfrom
openspec/partition-engine-2-dispatch

thecodedrift commented Aug 2, 2026 •

edited by github-actions Bot

Loading

Uh oh!

thecodedrift commented Aug 3, 2026

Uh oh!

claude Bot commented Aug 3, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

thecodedrift commented Aug 5, 2026

Uh oh!

thecodedrift commented Aug 5, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

thecodedrift commented Aug 2, 2026 • edited by github-actions Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Built on top of #79

Uh oh!

thecodedrift commented Aug 3, 2026

Uh oh!

claude Bot commented Aug 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review: dispatch rules by engine directory

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

thecodedrift commented Aug 5, 2026

Uh oh!

thecodedrift commented Aug 5, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

thecodedrift commented Aug 2, 2026 •

edited by github-actions Bot

Loading

claude Bot commented Aug 3, 2026 •

edited

Loading