feat(cli): dispatch the three engines concurrently and merge their findings - #94
Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces a new orchestration layer for check that dispatches the sg (ast-grep), Vale, and runtime engines concurrently, merges their findings, and derives the exit code from both findings severity and engine failures (while treating “unavailable” engines as advisory).
Changes:
- Add
rules/dispatch.tsto run all engines viaPromise.allSettled, merging results and surfacing engine failures without discarding other engines’ findings. - Wire
checkto use the new dispatcher and update engine layout metadata so Vale is executed viavale-runner. - Add orchestration tests and update the OpenSpec tasks checklist for section 2.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/cli/test/vale-orchestration.test.ts | Adds orchestration/exit-code tests for concurrent engine dispatch, Vale availability, and failure handling. |
| packages/cli/test/engine-dispatch.test.ts | Updates expectations/comments to reflect Vale now having an executor (vale-runner). |
| packages/cli/src/rules/engines.ts | Adds vale-runner executor and updates Vale engine layout to be executable. |
| packages/cli/src/rules/dispatch.ts | New shared dispatcher: concurrent engine runs, merged results, notices vs failures, exit code derivation. |
| packages/cli/src/commands/check.ts | Moves orchestration logic to runEngines() and uses deriveExitCode() for process exit + JSON success. |
| openspec/changes/add-vale-rule-engine/tasks.md | Marks orchestration tasks 2.1–2.3 as complete. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| } catch { | ||
| return false; | ||
| } | ||
| } |
| const scan = await import("../src/rules/scan"); | ||
| vi.spyOn(scan, "runAstGrepScan").mockRejectedValue( | ||
| new Error("ast-grep exploded") | ||
| ); | ||
|
|
…ndings Unit 3, tasks 2.1-2.3. `check` sequenced ast-grep then runtime inline and had no Vale at all. That block moves to rules/dispatch.ts, gains Vale, and runs all three concurrently. Vale's layout entry gains `executor: "vale-runner"`, replacing the `null` that recorded it as scaffolded but inert, and engine-dispatch.test.ts is updated to assert the new routing rather than the placeholder. allSettled, not all. `all` rejects on the first rejection and abandons the rest, so one engine throwing would discard findings the others had already produced — which is precisely the "an unavailable engine must not abort the others" requirement. Using allSettled makes that true by construction rather than by every future caller remembering to catch. A rejected engine becomes a reported failure rather than being swallowed: the engines report expected trouble as an outcome, so a throw is something unforeseen, and treating it as "no findings" is the silent-disable failure again. Exit code now has two independent causes. An error-severity finding is the ordinary one. An engine failure is the one that would be missed: a Vale that timed out or rejected its config produces no findings, so without it a broken engine exits 0 and reads exactly like a clean run. An unavailable engine stays advisory — an unsupported arch must not fail a check the other engines completed. Vale is not invoked when `.taskless/vale/rules/` is empty, per the spec. A scaffolded-but-empty engine directory is the state every `taskless init` leaves, and spawning a subprocess per check to confirm it found nothing is pure cost. Tests cover the mixed sg+vale corpus merging into one set, Vale absent while ast-grep still reports, an engine throwing without taking the others' results with it, and each exit-code cause on its own. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Jwc9FFroR3mTZ4hLiSkkX3
d3d84fd to
bf38e0f
Compare
|
@claude /review |
|
Claude finished @thecodedrift's task in 3m 53s —— View job Review: dispatch the three engines concurrently and merge their findings
Overall assessmentThe I found one bug I'd consider blocking, plus a few smaller issues, all posted inline: 1. if (astGrepSources.length === 0 && runtimeRules.length === 0) { ... return; }It never asks whether Vale has rules. A project with only 2. 3. Nothing else stood out — the |
…required Two gaps in the "One changeset, at the bottom of the stack" guidance, both found on the #71/#93/#94/#95 merging-down stack: - The section said where the changeset lives but never when to create it. Document the trigger: write it on the base branch before cutting the children, so every child inherits a file it can append to. - "Grow it incrementally" is merge-forward reasoning. Under merging down nothing lands until everything does, so a whole-change note is accurate at the only moment it is read. Scope the rule to forward, and state the constraint that holds in both shapes: the file must be in the bottom PR's own diff. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Jwc9FFroR3mTZ4hLiSkkX3
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| export async function hasValeRules(cwd: string): Promise<boolean> { | ||
| try { | ||
| const entries = await readdir( | ||
| join(cwd, ".taskless", ENGINE_LAYOUTS.vale.rulesDirectory) | ||
| ); | ||
| return entries.some((entry) => entry.endsWith(".yml")); | ||
| } catch { | ||
| return false; | ||
| } | ||
| } |
There was a problem hiding this comment.
hasValeRules swallows every readdir error, not just "directory doesn't exist" (ENOENT). If .taskless/vale/rules/ exists but is unreadable (EACCES) or hits some other unexpected IO error, this returns false and runValeEngine treats it identically to "scaffolded but empty" — no notice, no failure, Vale is just silently skipped.
That's exactly the "silent-disable failure" this PR's own docstrings elsewhere (isValeFailure in vale/run.ts) argue against: a broken/blocked engine ends up reading as a clean run instead of surfacing as a failure via allSettled. Suggest only treating ENOENT as "no rules" and rethrowing anything else so it surfaces through runValeEngine's promise rejection into outcomes/failures.
| expect(deriveExitCode(dispatched)).toBe(0); | ||
| }); | ||
|
|
||
| it("keeps a thrown engine from discarding the others' results", async () => { |
There was a problem hiding this comment.
This test (and the describe block it's in, starting at line 173) is not gated by the withVale helper used above at line 140/141, but it exercises a real runEngines call over makeMixedProject() (which writes real Vale config/rules) without mocking findValeBinary. On a machine/CI runner without the optional Vale binary installed, runVale will resolve to status: "unavailable", so line 219's expect(dispatched.results.some((r) => r.source === "vale")).toBe(true) will fail — even though the thing actually under test here (ast-grep's rejection not swallowing Vale's results) has nothing to do with whether Vale itself is installed.
Suggest either wrapping this describe block in withVale too, or mocking runVale/findValeBinary to return a deterministic ok outcome so the test is portable and isolates the allSettled behavior it's meant to cover.
Stack (root → tip):
Unit 3 of
add-vale-rule-engine. Stacked on #93, merging down. Tasks 2.1–2.3.checksequenced ast-grep then runtime inline and had no Vale at all. That block moves torules/dispatch.ts, gains Vale, and runs all three concurrently.allSettled, notallallrejects on the first rejection and abandons the rest — so one engine throwing would discard findings the others had already produced. That is precisely the "an unavailable engine must not abort the others" requirement, andallSettledmakes it true by construction rather than by every future caller remembering to catch.A rejected engine becomes a reported failure rather than being swallowed. The engines report expected trouble as an outcome, so a throw is something unforeseen — and treating it as "no findings" is the silent-disable failure again.
There's a test for exactly this: ast-grep is stubbed to reject, and Vale's findings still come back while the rejection surfaces as a failure.
Exit code now has two independent causes
The second is the one that would have been missed: a Vale that timed out produces no findings, so without it a broken engine exits 0 and reads exactly like a clean run. The third is deliberately not a failure — an unsupported arch must not fail a check the other engines completed.
Other changes
executor: "vale-runner", replacing thenullthat recorded it as scaffolded-but-inert.engine-dispatch.test.tsis updated to assert the new routing rather than the placeholder — repointing the reader in the same unit that changes the behaviour..taskless/vale/rules/is empty, per the spec. That's the state everytaskless initleaves, and spawning a subprocess per check to confirm it found nothing is pure cost.Verification
pnpm --filter @taskless/cli test→ 524 passed (10 new); lint, typecheck, prettier,openspec validate --strictclean.Section 2 is complete. Remaining: unit 4 — the engine-selection topic, its
TOPICSentry, and the archive.Refs OSS-21