Repository navigation
feat(vale): schema-check per-rule .vale.ini in verify (slice 1) - #365
Conversation
|
@claude /review |
|
Claude finished @theCodeDrift's task in 4m 57s —— View job Claude finished reviewingTodo list
Review mode: incremental — read 0 prior review thread(s) before reviewing. ScopeThis PR is spec-only (283 lines, all under Scenario-preservation check (per Findings (posted inline)
Everything else (constraint-id lists, rejection/advisory counts matching the "7 rejections, 4 advisories" claim, changeset placement on the bottom PR per the stacking guidance, PR issue reference formatting) checked out consistent. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
…01 notice requirement, state the patch case Review on #365: - the notices requirement's motivating scenario (a top-level assignment, W101) is now a schema rejection before Vale runs; the requirement is MODIFIED to cover diagnostics the schema cannot foresee and keeps the scenario, re-pointed at the rejection - a NO matcher before every YES moves from advisory to rejection: with BasedOnStyles empty it is dead or overridden in every config - the foreign-key scenario no longer reads like per-line stripping - the proposal says why slice 2's refusal is still patch under 0.y.z Refs #359
All four findings applied in 40e5e0f: the W101 notices requirement is now MODIFIED and re-pointed at diagnostics the schema cannot foresee; — AI Coding Agent |
theCodeDrift
added
the
Open OpenSpec
Adds the vale-config-schema OpenSpec change: parse each rule's .vale.ini into a lossless AST with @jedmao/ini-parser, validate it with a zod schema keyed by the rule id, attribute rejections in verify under vale-config-* constraints, and refuse the Vale run at assembly on a rejected config while ast-grep still runs. Assembly stays verbatim concatenation. Stacked forward in two slices; this commit carries only the proposal. Refs #359
…01 notice requirement, state the patch case Review on #365: - the notices requirement's motivating scenario (a top-level assignment, W101) is now a schema rejection before Vale runs; the requirement is MODIFIED to cover diagnostics the schema cannot foresee and keeps the scenario, re-pointed at the rejection - a NO matcher before every YES moves from advisory to rejection: with BasedOnStyles empty it is dead or overridden in every config - the foreign-key scenario no longer reads like per-line stripping - the proposal says why slice 2's refusal is still patch under 0.y.z Refs #359
40e5e0f to
5e56981
Compare
Add @jedmao/ini-parser (exact 0.2.4) and schemas/vale-config.ts, which parses a rule's config into an ordered AST (resolve: false, delimiter /=/, blank-line sections folded back into their matcher, source lines attached by a side-by-side walk) and validates it keyed by the rule id: no root keys, breadcrumb on every matcher, own key only, YES/NO values, empty BasedOnStyles, a matcher, a YES somewhere, and no NO before every YES. A repeated key, a [*] matcher and a .taskless/** matcher are advisories. The vendor contract pins that the parser reads each corpus glob shape as the matcher Vale enables, including a character class and a numeric segment.
…raints verifyOneRule for vale runs the config schema in place of its two substring checks. Each rejection reaches errors and violations under its vale-config-* constraint, so verify --json pairs it with a constraintId the way it already does for sg; advisories ride on notice. The eight constraints join RULE_CONSTRAINTS with a scenario each in the constraints suite, and reference.json is regenerated. The shipped demo Vale config gains its tskl) rule breadcrumb, which the schema now requires and taskless demo vale must verify under. The W101 check test keeps its run-time assertion (check is untouched until the next slice) and now expects verify to reject the misplaced assignment first.
… tick slice 1 buildIsolatingConfig said a repeat inside one matcher is discarded; since Vale 3.21.0 the last assignment wins there too. The changeset records what verify now refuses and advises; check is unchanged in this slice.
theCodeDrift
changed the title
|
@claude /review |
|
Claude finished @theCodeDrift's task in 5m 31s —— View job Claude finished reviewingTodo list
Review mode: incremental — read 4 prior review thread(s) before reviewing. All four are
I checked that all four are actually reflected in the current diff (they are: the requirement is MODIFIED with the reworded scenario, ScopeThis revision is no longer spec-only — it adds the slice 1 implementation: Findings (1 posted inline)1. [New] Concretely, Suggested fix: derive Everything else checked out
|
Read. The one inline finding ( — AI Coding Agent |
…nal verdict Vale merges same-glob sections and keeps the last assignment, so a YES that a later NO overrides never reaches it. The schema now folds sections by name, keeps each matcher's final verdict, and reads both the enabled-somewhere and disable-after-enable checks from those verdicts: a config whose only YES is overridden is rejected as present but off, while the repeat itself stays an advisory. Spec scenarios updated to match, with sibling fixtures pinning the accepted shape.
…01 notice requirement, state the patch case Review on #365: - the notices requirement's motivating scenario (a top-level assignment, W101) is now a schema rejection before Vale runs; the requirement is MODIFIED to cover diagnostics the schema cannot foresee and keeps the scenario, re-pointed at the rejection - a NO matcher before every YES moves from advisory to rejection: with BasedOnStyles empty it is dead or overridden in every config - the foreign-key scenario no longer reads like per-line stripping - the proposal says why slice 2's refusal is still patch under 0.y.z Refs #359
1 participant
Stack (root → tip):
What
Slice 1 of #359: a per-rule
.taskless/rules/vale/<id>/.vale.iniis parsed into an ordered AST and validated against a schema keyed by the rule id, andverifyreports each rejection under avale-config-*constraint.checkis untouched; slice 2 (assembly refusal, dispatch, recipe, ledger, archive) stacks on top.The OpenSpec change is on this branch too, under
openspec/changes/vale-config-schema/:proposal.md, the two spec deltas (cli-vale-rule-engineADDED + MODIFIED,cli-rule-validationMODIFIED, every MODIFIED block restated in full), andtasks.mdwith slice 1 ticked.Decisions to review
@jedmao/ini-parser@0.2.4, chosen by measurement overini/@nodecraft/ini(section names nest on., source order lost),iniparser(drops thetskl) rulekey),config-ini-parser(truncates it torule; GPL-3.0),js-ini(works, but discards comments and needs a merge-strategy option to see duplicate keys). Lossless, ordered AST; MIT; zero deps; pinned exactly. Bake-off table in vale: parse per-rule .vale.ini into an AST, validate it with a schema, refuse a bad config at assembly #359.checkrun (reaches the exit code; ast-grep still runs), the treatment a malformed ast-grep rule already gets. Omitting the rule with a notice would produce a rule that verifies, runs, and reports nothing — the failure the engine's design exists to close. (Slice 2.)sections; the parser'stoString()is measured not round-trip safe and is never called.verifystarts naming the rejection one release beforecheckstarts refusing it.[*], and.taskless/**are reported but not fatal. ANOmatcher before everyYESis a rejection (moved from the advisory list in the plan-review commit: withBasedOnStylesempty such aNOis dead or overridden in every config, so refusing it turns away nothing an author meant). Everything the schema cannot read as a well-formed self-scoping config is fatal.Slice 1
packages/cli/src/schemas/vale-config.ts(new).parseValeRuleConfig(source)constructs the parser withresolve: false(the default JSON-parses values, so2024would come back a number) anddelimiter: /=/(the default also splits on:), and returns a plain{ sections }AST with a 1-basedlineattached to every header and property by walking the source's non-blank lines side by side with the AST. Two parser behaviours are undone: a blank line is reported as a new section with an empty name and the lines after it land there, so those sections are folded back into their predecessor (Vale reads them as the same matcher); root properties arrive as a section with no name and are kept apart, because the first rejection is about exactly them.validateValeRuleConfig(ruleId, source)returns{ rejections: RuleViolation[], advisories: string[], sections: string[] }. A zod schema over the AST pushes onecustomissue per rejection withparams.constraintId, which is how a rejection is attributed without matching on its own wording:vale-config-no-root-keys[matcher](StylesPath, a rule assignment Vale wouldW101)vale-config-breadcrumb-requiredtskl) rule = <id>, or one naming another rulevale-config-own-key-onlyBasedOnStyles, and<id>.<id>(a<style>.<check>naming another rule is called out as a cross-rule override)vale-config-value-yes-no<id>.<id>assigned anything butYES/NOvale-config-based-on-styles-emptyBasedOnStylesnon-emptyvale-config-matcher-required[matcher]at allvale-config-enabled-somewhereYESvale-config-disable-after-enableNOmatcher before everyYES, naming theYESthat re-enables itAdvisories: a key assigned twice in one matcher (or once in each of two same-glob sections, which Vale merges), a
[*]matcher, a.taskless/**matcher. Every message names the rule, the matcher or key, and the line.Measured against the vendored 3.22.0 before writing the
value-yes-norationale:yesandmaybeleave the rule off with no diagnostic,error/suggestionturn it on at that level, andYES # noteis read asYESby Vale but kept whole by the parser underresolve: false, so that one line is rejected here with the value quoted. Noted in the module docblock as the one too-strict edge.RULE_CONSTRAINTSgains the eightvale-config-*entries (allenforcedBy: "verify"), each with a scenario inconstraints.test.tsthat asserts the message text and the attribution.reference.jsonregenerated.verifyOneRuleforvaleruns the schema in place of its two substring checks; rejections reacherrorsandviolationsviaviolate, advisories joinnotice. The two prior messages ("declares no matcher", "never enables … present but off") survive verbatim undervale-config-matcher-requiredandvale-config-enabled-somewhere.assets/demo-vale/.vale.inigains its breadcrumb, which the schema requires andtaskless demo valemust verify under.buildIsolatingConfig's docblock no longer says a repeat inside one matcher is discarded.ESM interop. The parser is CJS with
__esModuleandexports.default = Parser. The vite bundle honours the marker and hands a default import the class; vitest externalises the dependency and Node's loader hands itmodule.exports, with the class under.default. The module readsiniParser.default ?? iniParser, and the builtdist/index.jswas checked to resolve it to the class.Verified
pnpm build,pnpm typecheck,pnpm lint(includingpnpm cli checkover the prose),pnpm --filter @taskless/cli test: 96 files, 1575 tests, all green.test/vale-config-schema.test.ts: 26 cases overtest/fixtures/vale-config/(one fixture per rejection and advisory, the dogfood shape with[*.md],[2024/**],[docs/[a-z]*.md]and a trailing.taskless/**block, a blank line inside a matcher, line numbers on every node).test/vale-vendor-contract.test.ts: for[*.md],[docs/**/*.md],[docs/[a-z]*.md],[2024/**], the parser's section name equals the glob and Vale fires exactly on the files it names. One measured surprise recorded there:*crosses/, so[*.md]matchesdocs/deep.mdwhile the sibling block's literal[CLAUDE.md]does not recurse.test/verify-test-commands.test.ts:verify --jsonon a foreign key carriesconstraintId: "vale-config-own-key-only"with the same message inerrors; a repeated key passes with the advisory onnotice.test/mixed-engine-check.test.tsW101 case:verifynow rejects the misplaced assignment;checkstill surfaces W101 at run time until slice 2.Refs #359