fix(cli): correct raw-scope Vale line numbers - #314
Conversation
|
@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}
…et as the line Review on #314 caught a real gap in the line fix: `startColumn`/ `endColumn` still came straight from `finding.Span` unchanged, but `Span` is measured against the line Vale (wrongly) attributed a leading-newline match to, not the corrected line the range now reports. Confirmed against the real binary with a raw match preceded by a non-blank line: Vale reports `Span: [89, 117]`, which is that 89-character... rather, that preceding line's length (88) plus one through the whole match's length minus one — not a real column on any line. The original fixture's raw targets both happened to follow a *blank* line, whose zero length made the uncorrected column look right by coincidence. The character immediately after a `\n` is always column 1 of the next line, regardless of the preceding line's length or how many leading newlines the match opened with. So once a match has any leading newlines, `rawScopeColumns` now reports column 0 as the start and the match's length after stripping those newlines, minus one, as the end. Also closes the second review item: the "two leading newlines" line-advancement case was only covered synthetically. Added a real-Vale-binary rule anchored on `\n\n` to the existing fixture, so the newline *count*, not just its presence, is pinned against the vendor's actual behavior for both line and column. Mutation-tested: reverting `rawScopeColumns` to pass `Span` through unchanged failed exactly the 4 tests written for it (2 synthetic + 2 real-binary); nudging the reset start column from 0 to 1 was caught by the same 4; collapsing `leadingNewlines` to "1 if any, else 0" was caught by the two-newline synthetic test and its real-binary counterpart, and nothing else failed in any case.
Both flagged items were real, and both are fixed:
New commit: correct raw-scope columns for the same leading-newline offset as the line — a8208d6. Verified: |
…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.
…et as the line Review on #314 caught a real gap in the line fix: `startColumn`/ `endColumn` still came straight from `finding.Span` unchanged, but `Span` is measured against the line Vale (wrongly) attributed a leading-newline match to, not the corrected line the range now reports. Confirmed against the real binary with a raw match preceded by a non-blank line: Vale reports `Span: [89, 117]`, which is that 89-character... rather, that preceding line's length (88) plus one through the whole match's length minus one — not a real column on any line. The original fixture's raw targets both happened to follow a *blank* line, whose zero length made the uncorrected column look right by coincidence. The character immediately after a `\n` is always column 1 of the next line, regardless of the preceding line's length or how many leading newlines the match opened with. So once a match has any leading newlines, `rawScopeColumns` now reports column 0 as the start and the match's length after stripping those newlines, minus one, as the end. Also closes the second review item: the "two leading newlines" line-advancement case was only covered synthetically. Added a real-Vale-binary rule anchored on `\n\n` to the existing fixture, so the newline *count*, not just its presence, is pinned against the vendor's actual behavior for both line and column. Mutation-tested: reverting `rawScopeColumns` to pass `Span` through unchanged failed exactly the 4 tests written for it (2 synthetic + 2 real-binary); nudging the reset start column from 0 to 1 was caught by the same 4; collapsing `leadingNewlines` to "1 if any, else 0" was caught by the two-newline synthetic test and its real-binary counterpart, and nothing else failed in any case.
a8208d6 to
9ca3302
Compare
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