fix(cli): correct raw-scope Vale line numbers - #314
Conversation
…0-based contract Vale's `raw` patterns are conventionally anchored with a leading `\n` so they can require "start of line" against the unparsed document. That `\n` is part of the reported match, and Vale attributes `Line` to the newline ending the previous line rather than to the line the flagged text is actually on, so a raw-scope finding always landed one line early on top of the mapper's existing 1-based-to-0-based conversion. `toValeCheckResult` now counts a match's leading newlines and adds them back before converting to the 0-indexed `CheckResult.range`. The other half of #297 — default-scope findings reading "one line early" — is not a bug: `CheckResult.range` is 0-indexed by design for every source (ast-grep's native range passes straight through, and `format.ts` adds 1 back for every source when it displays). Vale itself reports the correct 1-based line for a default-scope match; the issue's table compared that 0-based JSON output against a 1-based file line number. Confirmed by reading map.ts's existing comment and tests, and by running the real Vale binary against a fixture with front matter. Added tests pinning both: a default-scope rule on a fixture with front matter as a regression guard for the working-as-designed half, and both synthetic and real-Vale-binary raw-scope tests for the fix. Mutation-tested by reverting the leading-newline correction and by substituting a broken newline-counting implementation; both mutations were caught by the new tests and only those tests.
|
@claude /review |
|
Claude finished @thecodedrift's task in 6m 27s —— View job Review completeReview mode: incremental — read 0 prior review thread(s) before reviewing. Todo list
Overall assessmentThe core diagnosis and fix are sound: Two things worth a second look before merge, posted as inline comments:
Everything else — import ordering/grouping (matches |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| const [spanStart, spanEnd] = finding.Span; | ||
| const line = Math.max(0, finding.Line - 1); | ||
| const line = Math.max(0, finding.Line + leadingNewlines(finding.Match) - 1); | ||
| const startColumn = Math.max(0, spanStart - 1); |
There was a problem hiding this comment.
[New] line is corrected for a raw-scope match's leading newlines, but startColumn/endColumn are still derived from finding.Span unchanged.
Vale computes Span relative to whatever line it attributes the match to (per the docstring above, it "has no concept of a finding that crosses lines"). For a raw match that opens with \n, Vale attributes the match to the line before the flagged text — so Span is presumably measured as an offset from the start of that (wrong) line, through the leading \n, into the actual flagged text on the next line.
That happens to come out numerically right when the preceding line is empty (0 characters of offset to absorb, as in this PR's fixture — a blank line before the raw target). But if a raw pattern is anchored on \n immediately after a non-blank line (e.g. \n#\s*Heading matching right after real prose, rather than after a blank separator line), the previous line's own character count would still be baked into Span, producing a startColumn/endColumn that doesn't correspond to any real position on the now-corrected line.
Since I can't run the real Vale binary in this environment to confirm the exact column semantics, this is a plausible gap rather than a confirmed one — worth a quick check (e.g. a fixture where the raw match's leading \n follows a non-empty line) to see whether column also needs the same kind of correction, or whether Vale already reports it relative to the matched text regardless of Line.
| const result = toValeCheckResult("docs/a.md", { | ||
| ...example, | ||
| Line: 10, | ||
| Match: "\n\nSome flagged text", | ||
| }); | ||
| expect(result.range.start.line).toBe(11); | ||
| }); | ||
|
|
||
| it("leaves a default-scope match (no leading newline) unaffected", () => { | ||
| const result = toValeCheckResult("docs/a.md", { |
There was a problem hiding this comment.
[New] The "advances the line by the count of leading newlines, not just one" case (2 leading newlines) is only exercised synthetically, never against the real Vale binary — unlike the single-leading-newline case, which the real-binary suite further down pins directly (toValeCheckResult against the real Vale binary).
This synthetic test only confirms leadingNewlines/toValeCheckResult do the arithmetic they're written to do; it doesn't confirm that Vale actually attributes Line the same way (one line "too early" per leading \n) when a raw match opens with two consecutive newlines rather than one. If Vale's line-attribution for that case differs even slightly (e.g. if it always lands on the line right before the match regardless of how many \ns are consumed getting there), this test would still pass while the real fix silently mis-corrects. Given the PR already has a real-Vale-binary suite and fixture pattern in place, it would be low-cost to add one raw-scope rule anchored on \n\n (e.g. "blank line required before this heading") to close the gap the way the single-newline case is already closed.
1 participant
Summary
check --jsonreported araw-scope Vale finding's line one line earlier than the flagged text (#297).rawpatterns are conventionally anchored with a leading\nso they can require "start of line" against the unparsed document. That\nis part of Vale's reported match, and Vale attributesLineto the newline ending the previous line rather than to the line the flagged text is actually on.toValeCheckResultnow counts a match's leading newlines (viaMatch, already on the payload) and adds them back before the existing 1-based-to-0-based conversion.The other half of #297 was not a bug
The issue also reported default-scope findings landing "one line early." That is
CheckResult.rangeworking as documented: it is 0-indexed for every source (ast-grep's native range passes straight through,format.tsadds 1 back for every source when it displays), and Vale itself reports the correct 1-based line for a default-scope match. The issue's table compared that 0-based JSON payload against a 1-based file line number.Established by:
vale-map.test.ts's existing worked example, which already encodesLine: 3→range.start.line: 2format.ts:12adds+ 1back when it displaysrange.start.linecheck.tsemitsresultsverbatim under--jsonwith no re-conversionMeasured
Repro with the built CLI on a fixture with front matter (doc lines 1-based:
to be honeston line 7, raw target on line 9):range.start.line, 0-based)Before the fix, raw was one line short of the 0-based value that matches the default-scope contract (6 vs 8 would be the base+2 shift the issue measured in 1-based terms across scopes; here shown as the 0-based JSON values the built CLI actually emits).
Tests
Added to
packages/cli/test/vale-map.test.ts:toValeCheckResultcases for 1 leading newline, 2 leading newlines, no leading newline (default-scope), and an embedded (non-leading) newlinetoValeCheckResult against the real Vale binary) with a fixture that has YAML front matter, one default-scope rule, and one raw-scope rule, assertingrange.start.linefor bothMutation-tested: reverting the leading-newline correction failed exactly the 3 tests meant to catch it (2 synthetic + the real-binary raw test) while both default-scope tests stayed green; substituting a naive "count all newlines" implementation was caught by the "does not count a newline appearing after the match's start" test and nothing else.
Verification
pnpm typecheck— passpnpm test— 1351/1351 passpnpm lint— pass (check:stylereports "No issues found")check doc.md --json) both before and after the fix, shown aboveNo changeset needed for the documentation half since no behavior changed there; one changeset added (
patch, pre-1.0 per repo convention) for the raw-scope line fix.Fixes #297