test(cli): check a project both engines have work in, and fix what it found - #100
Conversation
… found
The unit suites each mock at the seam they test -- `vale-orchestration`
stubs `runVale`, `vale-run` generates a config into a temp directory.
Nothing ran a committed project the way a user has one: both engine
directories populated, both native configs on disk as authored, and the
real binary resolved for each. Two bugs lived in that gap, and both made
Vale silently inert rather than loudly broken.
**Vale never ran on a whole-project check.** Given no path arguments Vale
prints its usage text and exits 0, which reaches the mapper as "output
that is not JSON" and reports the engine failed. `taskless check` with no
paths -- the ordinary invocation -- therefore produced zero Vale findings
and one spurious engine failure, every time. ast-grep is why this was
easy to miss: it takes targets from its config and is content with none,
so the two engines disagree about what "no paths" means. `runVale` now
defaults to `.`, the project root it already runs in.
**The scaffolded `.vale.ini` could not resolve a rule.** Vale reads
`StylesPath` as a directory *of styles*, so a rule at
`vale/rules/no-simply.yml` is the `no-simply` rule of the `rules` style
and its check is `rules.no-simply` -- the name `stripRulesPrefix` undoes
and the shape `verify.ts` generates. `0004` wrote `StylesPath = rules`,
making that directory a style containing no rules: every check resolved
to nothing and Vale returned `{}`. The failure shape is the bad one --
`rule verify` passes, because verify generates its own correct config,
and the rule then never fires in `check`.
The existing migration test asserted `.vale.ini` exists. It never asked
whether anything under it could resolve, which is how a config this wrong
survived. The new scaffold test goes through `init` rather than importing
the constant, and was confirmed to fail against the old value first.
Also corrects `0001`'s README, which told users Vale was "inert; nothing
runs it yet" -- true when written, false as of this stack.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Jwc9FFroR3mTZ4hLiSkkX3
There was a problem hiding this comment.
Pull request overview
Adds fixture-based end-to-end integration coverage for taskless check on a project that contains both ast-grep and Vale rules, and fixes two issues that previously made Vale silently inert in real-world runs.
Changes:
- Add an integration test + committed fixture project that exercises both engines together via the built CLI (JSON + human output paths).
- Fix Vale invocation for whole-project checks by defaulting to
.when no explicit paths are provided. - Fix the scaffolded
.vale.inimigration to useStylesPath = .so rules undervale/rules/actually resolve, and update init README guidance accordingly.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| packages/cli/test/mixed-engine-check.test.ts | New end-to-end tests validating mixed-engine findings, attribution, and scaffolded config behavior. |
| packages/cli/test/fixtures/mixed-engines-project/sample.js | JS fixture designed to trigger ast-grep findings (error + warning). |
| packages/cli/test/fixtures/mixed-engines-project/README.md | Markdown fixture designed to trigger Vale findings. |
| packages/cli/test/fixtures/mixed-engines-project/.taskless/vale/rules/no-simply.yml | Vale rule fixture (warning) used by the mixed-engine project. |
| packages/cli/test/fixtures/mixed-engines-project/.taskless/vale/rules/no-obviously.yml | Vale rule fixture (error) used by the mixed-engine project. |
| packages/cli/test/fixtures/mixed-engines-project/.taskless/vale/.vale.ini | Committed Vale config fixture using StylesPath = . and enabling both rules. |
| packages/cli/test/fixtures/mixed-engines-project/.taskless/taskless.json | Fixture manifest pinned to schema version 4 to avoid migrations during the test. |
| packages/cli/test/fixtures/mixed-engines-project/.taskless/sg/sgconfig.yml | Committed sg config fixture pointing at rules/. |
| packages/cli/test/fixtures/mixed-engines-project/.taskless/sg/rules/no-eval.yml | ast-grep rule fixture (error). |
| packages/cli/test/fixtures/mixed-engines-project/.taskless/sg/rules/no-console-warn.yml | ast-grep rule fixture (warning). |
| packages/cli/src/rules/vale/run.ts | Ensure Vale runs on whole-project checks by passing . when no paths are provided. |
| packages/cli/src/filesystem/migrations/0004-vale-engine.ts | Fix scaffolded .vale.ini StylesPath so rules resolve and actually fire in check. |
| packages/cli/src/filesystem/migrations/0001-init.ts | Update scaffold README text to reflect Vale is run by check alongside ast-grep. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
@claude /review |
|
Claude finished @thecodedrift's task in 3m 12s —— View job Review complete
Overall assessmentThis is a solid PR — the two bugs it fixes (Vale defaulting to no target on a whole-project
Two issues posted inline:
Minor, non-blocking observations (not worth inline comments):
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Two comments that were true when written and are false as of this stack, both flagged in review. `0004`'s migration docstring still said `vale/` "stays inert (no engine reads it yet)" -- contradicting the corrected `VALE_CONFIG_CONTENT` comment twelve lines above it and the `0001` README in the same PR. The accurate distinction is quiet, not inert: `check` reads and runs the config, and the scaffold simply enables no rules yet. That difference is the reason the scaffolded config has to be one Vale can resolve rules under at all. `DispatchOptions.paths` never stated that empty means "the whole project", which is precisely the gap the Vale bug fell into: the engines disagree natively about empty input -- ast-grep takes targets from its config and is content with none, Vale prints usage and exits 0 -- so passing it straight through ran one correctly and reduced the other to a parse failure on every whole-project check. Documented on the field a future engine integrator reads, so the next one decides what empty means for its executor rather than assuming the caller narrowed it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Jwc9FFroR3mTZ4hLiSkkX3
Thanks for independently verifying both fixes rather than taking the description's word for it — particularly the check that Also useful: your note that Both inline items are handled, in On the non-blocking observations:
Three CLI spawns against an identical fixture — declining, with reasoning. The saving is real (~330ms each) but the tests are not as identical as they look:
— AI Coding Agent |
Vale has no reason to know `.taskless/` is ours, and defaulting a pathless check to `.` walks straight into it. With a rule enabled, the first thing a user saw after authoring their first prose rule was findings in the committed `.vale.ini` and in the rule definition they had just written -- prose complaints about the machinery, pointing at files nobody wrote as prose. Section globs cannot fix this: `.taskless/README.md` matches `[*.md]` as readily as any document, so even a correctly scoped config lints it. `--glob=!.taskless/**` filters which files Vale walks without touching which rules apply to them -- measured, with a config scoped to `[*.md]` and a `.txt` file present, the user's scoping still decides what is checked and only the exclusion changes. Applied only when we chose `.` ourselves. An explicit path is a request, and silently declining to check a file someone named would be worse than checking one they did not -- covered by its own test. Split out of #101 rather than left there. That issue bundled this with the question of whether to exclude build output and vendored trees, and those are not the same kind of problem: excluding `dist/` is a product default someone could reasonably disagree with, while Taskless linting its own metadata is wrong under every reading. #101 keeps the part that is genuinely a judgement call. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Jwc9FFroR3mTZ4hLiSkkX3
Stack (root → tip):
Fixture-based integration coverage for a project that has both ast-grep and Vale rules, run end-to-end through the built CLI — the same shape as the existing
check.test.ts/fixtures/taskless-projectpattern, extended to two engines.It found two bugs on the first run. Both made Vale silently inert rather than loudly broken, which is why nothing above caught them.
Why the existing tests missed this
Each unit suite mocks at the seam it is testing —
vale-orchestrationstubsrunVale,vale-rungenerates a config into a temp directory,vale-verifybuilds its own isolating config. None of them ran a committed project the way a user has one: both engine directories populated, both native configs on disk as authored, and the real binary resolved for each.Bug 1 — Vale never ran on a whole-project check
Given no path arguments, Vale prints its usage text and exits 0. That reaches the mapper as
Vale produced output that is not JSONand is reported as an engine failure. Sotaskless checkwith no paths — the ordinary invocation — produced zero Vale findings and one spurious engine failure, every time.ast-grep is why it was easy to miss: it takes targets from its config and is content with none, so the two engines disagree about what "no paths" means.
runValenow defaults to., the project root it already runs in.Bug 2 — the scaffolded
.vale.inicould not resolve a ruleVale reads
StylesPathas a directory of styles. A rule atvale/rules/no-simply.ymlis therefore theno-simplyrule of therulesstyle, and its check isrules.no-simply— the namestripRulesPrefixexists to undo, and the shapeverify.tsgenerates.Migration
0004wroteStylesPath = rules, making that directory a style containing no rules. Every check resolved to nothing and Vale returned{}.The failure shape is the bad one:
rule verifypasses, because verify generates its own correct config, and the rule then never fires incheck. Measured against the real binary — the layout is byte-identical either way, so this is invisible to inspection.migrate-engine-layout.test.tsasserted.vale.iniexists. It never asked whether anything under it could resolve, which is how a config this wrong survived.What is covered
ast-grepandvalefindingssample.js, prose rules →README.md, with severitiesno-simply, notrules.no-simplyinit-ed project actually firesThe scaffold test goes through
initrather than importing the constant, so it tests the config a user actually gets. It was confirmed to fail against the old value before the fix.Two assertions originally passed vacuously (
.every()on an empty array) and now assert presence first — worth noting, since that is the same class of mistake as the bugs above.Also corrects
0001's README, which told users Vale was "Scaffolded and inert; nothing runs it yet" — true when written, false as of this stack.Notes
withVale-gated; the host-independent case is deliberately ungated.vale/run.ts,0004-vale-engine.ts) belong to earlier PRs, but the tests that expose them live here, and neither bug is reachable until feat(cli): dispatch the three engines concurrently and merge their findings #94 gives Vale an executor.Refs #71