Repository navigation
feat(vale): refuse the Vale run on a rejected .vale.ini (slice 2) - #366
Conversation
|
@claude /review |
|
Claude finished @theCodeDrift's task in 5m 52s —— View job Review completeReview mode: incremental — read 0 prior review thread(s) before reviewing.
What I checked
Findings1 new finding, posted inline on
Everything else held up: the refusal/verbatim-write split in |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Acknowledged. The one finding (advisories dropped on the blocking Vale branch) is fixed in c2421fd and its thread resolved; the rest of the review found nothing to change. — AI Coding Agent |
assembleValeConfig validates every rule's config against the config schema and, on any rejection, returns a refusal naming each rule and line without writing .taskless/.vale.ini. ruleConfigBody and sectionPatternsOf are gone: an accepted config is written verbatim under its breadcrumb, and sections come from the parsed structure. dispatch reports a refusal as the Vale engine's failure, so it reaches the exit code while ast-grep still runs; the schema's advisories join the notices. The mixed-engine e2e test that expected check to surface W101 at run time now asserts the refusal, and the run-time notice stays pinned in vale-run.test.ts through a hand-written config that bypasses the schema. enforcedBy stays "verify" for the vale-config-* constraints: every one is decidable from the files alone, which is the ordering question the field answers; check refuses and points back at verify.
create-vale-rule (topic v10) says the config is schema-checked at verify and refused at check, lists what is rejected and what is advised, and notes that a NO matcher preceding every YES is rejected. update (topic v8) gains a 0.11.3 ledger entry: a config check used to tolerate now refuses the Vale run, with the four shapes most likely to trip an existing rule and the verify command that names the line. The changeset grows by the refusal and the reason the bump stays patch.
runValeEngine threaded the schema's advisories into the notice on the ok and unavailable branches but returned only the failure when Vale timed out or crashed, so an advisory was heard only on runs where Vale did not break. The blocking branch now carries them on notice beside the failure; they are not folded into the failure because an advisory is not what failed the run. Pinned for all three outcome shapes.
Dry-run first per CLAUDE.md, against the parent's revised delta (final per-matcher verdicts): cli-vale-rule-engine 23 -> 33 scenarios, cli-rule-validation 26 -> 31, no prior scenario lost by name. Then archived for real on this tip.
The parent slice now judges enabled-somewhere and NO-before-YES by each matcher's last assignment, so a YES a later NO in the same matcher overrides no longer counts. The recipe's advisory bullet and the changeset say so, so neither reads as promising that such a config is merely advised.
c2421fd to
abbe40f
Compare
1 participant
Stack (root → tip):
Slice 2 of
vale-config-schema, stacked on #365 (slice 1). Tip of the stack; this PR archives the change.What
assembleValeConfigruns every rule's.vale.inithroughvalidateValeRuleConfig. On any rejection it returns{ status: "refused", refusals: [{ ruleId, rejections }] }and writes nothing; every refused rule is named at once, so an author with two broken configs is not sent around twice. On acceptance it writes the header, then per rule# tskl) rule = <id>and the source verbatim (a newline is appended only when the file lacks one), returning{ status: "ok", path, sections, advisories }withsectionsread from the parsed structure.ruleConfigBodyandsectionPatternsOfare deleted; the only string edit assembly ever made (dropping a copied-inStylesPath) is now a schema rejection.failure, so it reaches the exit code while ast-grep and runtime still run. The message names each rule and carries the rejection messages as written (they already carry<id>/.vale.ini line N:), and points attaskless verify. Advisories join the Vale outcome's notice alongside anything Vale itself wrote on a zero exit.DispatchOptions.valeConfigPathbecamevale: ValeAssembly | undefined;commands/check.tspassesassembled.valethrough.create-vale-rule(topic v10) states the config is schema-checked atverifyand refused atcheck, lists what is rejected and what is advised, and its "scope out with a second matcher" guidance now says the schema rejects aNOthat precedes everyYES. No.taskless/**matcher existed in its examples.update(topic v8) gains the 0.11.3 ledger paragraph with the four shapes most likely to trip an existing rule and theverifycommand that names the line.vale-config-schemaon this tip.Why
A rule whose config the schema rejects is one Vale reads as something other than what its author wrote, with a zero exit and an empty report. Slice 1 told the author at
verify; this slice stopscheckfrom running such a config to a clean pass. Refusing rather than omitting the rule is deliberate: a rule left out with a soft notice would verify, run, and report nothing, which is the silent-disable failure the engine exists to prevent. Refusing rather than stripping is the same decision from the other side.How verified
pnpm build,pnpm typecheck,pnpm lint(includingpnpm cli checkover the recipes),pnpm --filter @taskless/cli test: 96 files, 1581 tests, all green.assemble.test.ts: existing configs gained breadcrumbs (the schema now requires them); theStylesPathtest asserts a refusal undervale-config-no-root-keys; new tests assert a foreign key leaves.taskless/.vale.iniunwritten, every refused rule is named, an accepted config is written byte-for-byte under its breadcrumb (trailing spaces and blank lines intact), and advisories ride on the result.mixed-engine-check.test.ts(built CLI, real binaries): the W101 test now asserts the Vale failure namesno-simply/.vale.ini line 1:, exit 1, ast-grep results present, no Vale results, no notices; a second test covers the spec's "foreign assignment refuses the run" scenario (no-simplyassigningno-obviously.no-obviously = NO, line 5, neighbour not named); a third asserts a.taskless/**advisory reachesnoticeson a run that still happens.vale-run.test.ts, which handsrunValea hand-written W101 config directly, deliberately around the schema; its comment now says so.cli-vale-rule-engine23 → 32 scenarios,cli-rule-validation26 → 30, zero lost by name (commover sorted#### Scenariolines), reset to the wip SHA, then archived for real.pnpm openspec validate --all: 29 passed.The
enforcedBydecisionLeft as
"verify"for all eightvale-config-*constraints, with the reasoning recorded in the comment block above them inconstraints.ts. The field answers a consumer's ordering question (can this be decided from the files alone, before anything executes?), and for every entry here it can.verifyis where the author is told which line and which constraint;checkruns the same schema but only refuses and points back atverify. Widening the type to include"check"would have touched the publishedreference.jsonshape for a distinction no consumer needs to branch on. Happy to widen it if the reference format wants it.Deviations from tasks.md
.trim()s a rule's source; the verbatim test pins that trailing whitespace survives. Thesectionsfield is still deduplicated and sorted as before.refusals[]) rather than the singleruleIdthe task sketched, so one run reports every broken config.Refs #359