diff --git a/.changeset/vale-raw-scope-line-numbers.md b/.changeset/vale-raw-scope-line-numbers.md new file mode 100644 index 00000000..ce41fa0c --- /dev/null +++ b/.changeset/vale-raw-scope-line-numbers.md @@ -0,0 +1,18 @@ +--- +"@taskless/cli": patch +--- + +Fixed `check --json` reporting a `raw`-scope Vale finding's `range.start.line` +one line earlier than the flagged text (#297). A `raw` pattern is +conventionally anchored with a leading `\n` so it can require "start of line" +against the unparsed document; that `\n` is part of Vale's reported match, and +Vale attributes `Line` to the newline ending the previous line rather than to +the line the flagged text is actually on. The mapper now counts a match's +leading newlines and adds them back before converting to the 0-indexed +`CheckResult.range` every source uses. + +`default`-scope findings were not affected: Vale already reports the correct +1-based line for them, and `range.start.line` is 0-indexed by design (every +source in `CheckResult.range` is — `format.ts` adds 1 back when it displays, +and #297's "off by one" for default-scope rules was this documented +convention compared against a 1-based file line, not a bug). diff --git a/packages/cli/src/rules/vale/map.ts b/packages/cli/src/rules/vale/map.ts index 7671e28b..09343b8f 100644 --- a/packages/cli/src/rules/vale/map.ts +++ b/packages/cli/src/rules/vale/map.ts @@ -143,6 +143,66 @@ function toFix(finding: ValeFinding): string | undefined { return typeof replacement === "string" ? replacement : undefined; } +/** + * Vale's `raw`-scope patterns are conventionally anchored with a leading + * `\n` (matching against the unparsed document lets a pattern require "start + * of line" this way, since `raw` has no notion of line boundaries otherwise). + * That leading `\n` is *part of the match*, so `Match` starts with it, and + * Vale attributes `Line` to where the match itself starts — the newline that + * *ends* the previous line — rather than to the line the flagged text is + * actually on. + * + * Measured against the real binary: a `raw` rule matching + * `\n**The base is a promise...` on a line whose true (1-based) number is 13 + * is reported by Vale as `Line: 12`, one line early, while a `default`-scope + * rule matching the same document reports the correct 1-based line with no + * such offset. Counting the match's leading newlines and adding them back + * corrects this for any number of leading newlines, not just one, and is a + * no-op for every scope that does not open a match on `\n`. + */ +function leadingNewlines(text: string): number { + let count = 0; + while (text[count] === "\n") count++; + return count; +} + +/** + * `Span` for a leading-newline `raw` match is measured in the *attributed* + * (wrong) line's coordinate space, not the corrected one, so it cannot be + * reused verbatim once {@link leadingNewlines} moves the line forward. + * + * Measured against the real binary: for a match opening with one `\n` + * preceded by a 70-character line, Vale reports `Span: [71, 129]` — 71 is + * that preceding line's length plus one, and 129 is 71 plus the *whole* + * match length (59) minus one. Vale is not tracking per-line columns here at + * all; it is counting characters from the start of the line it (wrongly) + * attributed the match to, straight through the leading `\n` and into the + * flagged text, however many characters that takes. A blank preceding line + * (length 0) makes `Span` start at 1, which happens to equal the true + * column — that coincidence is what made the original fixture look correct. + * + * The character immediately after a `\n` is always column 1 of the next + * line, independent of how long the previous line was or how many leading + * newlines the match opened with (each one just steps down one more line). + * So once a match has any leading newlines, the true start column is always + * the first column, and the true end column is however long the match is + * *after* stripping those newlines — confirmed against the real binary for + * both one and two leading newlines. `Span` is only trustworthy as-is when + * there is no leading newline to correct for. + */ +function rawScopeColumns( + span: [number, number], + match: string, + newlines: number +): [number, number] { + if (newlines === 0) { + const [spanStart, spanEnd] = span; + return [Math.max(0, spanStart - 1), Math.max(0, spanEnd - 1)]; + } + const strippedLength = match.length - newlines; + return [0, Math.max(0, strippedLength - 1)]; +} + /** * Map one Vale finding to the scanner-agnostic {@link CheckResult}. * @@ -150,22 +210,31 @@ function toFix(finding: ValeFinding): string | undefined { * columns within it, and has no concept of a finding that crosses lines, so * start and end share the line number. * - * Both are converted down by one. `CheckResult.range` is 0-indexed — ast-grep's - * native range is passed straight through by `toCheckResult`, the runtime - * harness converts its 1-based `Finding` down the same way, and `format.ts` adds - * 1 back for every source when it displays. Vale's `Line` and `Span` are both - * 1-based, so emitting them verbatim would report every finding one line and one - * column further into the file than it is. Clamped at 0 because a 0 from Vale + * The line is corrected for a `raw`-scope match's leading newlines (see + * {@link leadingNewlines}) and then converted down by one; the columns get + * their own correction (see {@link rawScopeColumns}) because `Span` is + * measured against the line Vale attributed the match to, which is no longer + * the line this range reports once the line correction moves it. Both + * corrections are no-ops when the match has no leading newline. + * `CheckResult.range` is 0-indexed — ast-grep's native range is passed + * straight through by `toCheckResult`, the runtime harness converts its + * 1-based `Finding` down the same way, and `format.ts` adds 1 back for every + * source when it displays. Vale's `Line` and `Span` are both 1-based, so + * emitting them verbatim would report every finding one line and one column + * further into the file than it is. Clamped at 0 because a 0 from Vale * (unset, rather than a real position) must not become -1. */ export function toValeCheckResult( file: string, finding: ValeFinding ): CheckResult { - const [spanStart, spanEnd] = finding.Span; - const line = Math.max(0, finding.Line - 1); - const startColumn = Math.max(0, spanStart - 1); - const endColumn = Math.max(0, spanEnd - 1); + const newlines = leadingNewlines(finding.Match); + const line = Math.max(0, finding.Line + newlines - 1); + const [startColumn, endColumn] = rawScopeColumns( + finding.Span, + finding.Match, + newlines + ); return { source: "vale", ruleId: stripRulesPrefix(finding.Check), diff --git a/packages/cli/test/vale-map.test.ts b/packages/cli/test/vale-map.test.ts index e74e7b84..dfa3e5d7 100644 --- a/packages/cli/test/vale-map.test.ts +++ b/packages/cli/test/vale-map.test.ts @@ -1,5 +1,11 @@ -import { describe, expect, it } from "vitest"; +import { spawnSync } from "node:child_process"; +import { mkdtempSync, mkdirSync, rmSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { afterEach, describe, expect, it } from "vitest"; + +import { findValeBinary } from "../src/rules/vale/binary"; import { asValeConfigError, normalizeSeverity, @@ -93,6 +99,100 @@ describe("toValeCheckResult", () => { expect(result.range.start.line).toBe(result.range.end.line); }); + describe("raw-scope leading newlines", () => { + // `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 match, and Vale attributes `Line` to where the match + // itself starts (the newline ending the previous line) rather than to + // the line the flagged text is actually on. See `leadingNewlines` in + // map.ts. + + it("advances the line by one for a single leading newline", () => { + // Vale's `Line: 12` here names the blank line before the flagged text, + // which is truly on line 13 (1-based) / 12 (0-based). + const result = toValeCheckResult("docs/a.md", { + ...example, + Line: 12, + Match: "\n**The base is a promise about the build.**", + }); + expect(result.range.start.line).toBe(12); + }); + + it("advances the line by the count of leading newlines, not just one", () => { + 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", { + ...example, + Line: 9, + Match: "To be honest", + }); + expect(result.range.start.line).toBe(8); + }); + + it("does not count a newline appearing after the match's start", () => { + // Only *leading* newlines are the artifact of the anchoring pattern; an + // embedded one is part of the matched content, not a start-of-match + // marker, and must not shift the line. + const result = toValeCheckResult("docs/a.md", { + ...example, + Line: 9, + Match: "To be honest\nabout it", + }); + expect(result.range.start.line).toBe(8); + }); + + describe("column", () => { + // `Span` for a leading-newline match is measured against the line + // Vale (wrongly) attributed the match to, not the corrected one — see + // `rawScopeColumns` in map.ts. These pin the two shapes with a + // fabricated `Span`, mirroring what a non-blank preceding line + // produces on the real binary (confirmed separately below); a + // real-Vale-binary case with an actually non-blank predecessor lives + // in the suite further down. + + it("resets the column to the start of the corrected line for a single leading newline", () => { + // A `Span` that, read against the *wrong* line, would put the match + // 40 columns in — but the real content starts at column 1 of the + // corrected line regardless of how long the previous line was. + const result = toValeCheckResult("docs/a.md", { + ...example, + Line: 9, + Span: [41, 53], + Match: "\nTo be honest", + }); + expect(result.range.start.column).toBe(0); + // Stripped match ("To be honest") is 12 characters. + expect(result.range.end.column).toBe(11); + }); + + it("resets the column the same way for two leading newlines", () => { + const result = toValeCheckResult("docs/a.md", { + ...example, + Line: 9, + Span: [41, 55], + Match: "\n\nTo be honest", + }); + expect(result.range.start.column).toBe(0); + expect(result.range.end.column).toBe(11); + }); + + it("leaves the column alone when there is no leading newline", () => { + // The pre-existing, unaffected path: `Span` already describes the + // real line, so it converts down by one exactly as before. + const result = toValeCheckResult("docs/a.md", example); + expect(result.range.start.column).toBe(0); + expect(result.range.end.column).toBe(6); + }); + }); + }); + it("joins Description and Link into note, and omits it when both are empty", () => { expect( toValeCheckResult("a.md", { ...example, Description: "Say it plainly." }) @@ -147,6 +247,185 @@ describe("toValeCheckResult", () => { }); }); +/** + * These run the real Vale binary rather than fabricating `ValeFinding` + * objects, because the whole bug (#297) was Vale's own reported `Line` + * disagreeing with the true line, and a hand-built fixture would only prove + * that `toValeCheckResult` does what we assume Vale does, not what it + * actually does. Vale ships as an `optionalDependency` for the host + * platform, so it is present in CI and absent only on an unsupported arch. + */ +const valeAvailable = findValeBinary().path !== undefined; +const withVale = valeAvailable ? describe : describe.skip; + +withVale("toValeCheckResult against the real Vale binary", () => { + const workspaces: string[] = []; + + afterEach(() => { + for (const workspace of workspaces.splice(0)) { + rmSync(workspace, { recursive: true, force: true }); + } + }); + + /** + * A document with YAML front matter (`---` on lines 1 and 3 here), since + * the issue's repro documents all carried front matter and the bug was + * measured against them. + * + * Lines 11-12 and 14-16 exist to close a gap a reviewer found in the + * original fixture: both raw-scope targets there were preceded by a + * *blank* line, whose zero length happens to make the uncorrected column + * come out right by coincidence. Line 12 is preceded by a non-blank line + * instead, and line 16 is reached by a match with *two* leading newlines, + * so both the column fix and the newline-count generalization are pinned + * against the real binary rather than only the single-newline, + * blank-predecessor case. + * + * Line numbers (1-based), annotated because the test asserts against them: + * 1: --- + * 2: title: Test + * 3: --- + * 4: (blank) + * 5: Intro paragraph. + * 6: (blank) + * 7: To be honest, this is the default-scope target. + * 8: (blank) + * 9: **The base is a promise about the build.** Raw-scope target. + * 10: (blank) + * 11: This is a long non-blank preceding line so the raw column bug would show if uncorrected. + * 12: ## Heading Right After Prose + * 13: (blank) + * 14: A line with some prose to precede the blank separator before a heading. + * 15: (blank) + * 16: ## Heading After Two Blank Lines + */ + const fixture = [ + "---", + "title: Test", + "---", + "", + "Intro paragraph.", + "", + "To be honest, this is the default-scope target.", + "", + "**The base is a promise about the build.** Raw-scope target.", + "", + "This is a long non-blank preceding line so the raw column bug would show if uncorrected.", + "## Heading Right After Prose", + "", + "A line with some prose to precede the blank separator before a heading.", + "", + "## Heading After Two Blank Lines", + ].join("\n"); + + function runValeDirect(): ValeFinding[] { + const cwd = mkdtempSync(join(tmpdir(), "vale-map-")); + workspaces.push(cwd); + mkdirSync(join(cwd, "styles", "Test"), { recursive: true }); + writeFileSync( + join(cwd, ".vale.ini"), + "StylesPath = styles\nMinAlertLevel = suggestion\n\n[*.md]\nBasedOnStyles = Test\n" + ); + writeFileSync( + join(cwd, "styles", "Test", "DefaultScope.yml"), + "extends: existence\nmessage: \"Avoid '%s'\"\nlevel: warning\nignorecase: true\ntokens:\n - 'to be honest'\n" + ); + writeFileSync( + join(cwd, "styles", "Test", "RawScope.yml"), + "extends: existence\nmessage: \"Raw hit: '%s'\"\nlevel: warning\nscope: raw\nraw:\n - '\\n\\*\\*The base is a promise.*'\n" + ); + // Anchored on a single `\n`, but preceded by a non-blank line — the case + // the blank-predecessor `RawScope` rule above cannot distinguish a + // correct column fix from a coincidentally-right one. + writeFileSync( + join(cwd, "styles", "Test", "RawScopeAfterProse.yml"), + "extends: existence\nmessage: \"Raw hit: '%s'\"\nlevel: warning\nscope: raw\nraw:\n - '\\n## Heading Right After Prose'\n" + ); + // Anchored on two leading `\n`s, to pin the newline *count* (not just + // its presence) against the real binary for both line and column. + writeFileSync( + join(cwd, "styles", "Test", "RawScopeTwoNewlines.yml"), + "extends: existence\nmessage: \"Raw hit: '%s'\"\nlevel: warning\nscope: raw\nraw:\n - '\\n\\n## Heading After Two Blank Lines'\n" + ); + writeFileSync(join(cwd, "doc.md"), fixture); + + const { path: binary } = findValeBinary(); + const result = spawnSync( + binary as string, + ["--config=.vale.ini", "--output=JSON", "doc.md"], + { cwd, encoding: "utf8" } + ); + const output = JSON.parse(result.stdout) as Record; + return output["doc.md"] ?? []; + } + + it("reports the true line for a default-scope rule", () => { + const findings = runValeDirect(); + const finding = findings.find((f) => f.Check === "Test.DefaultScope"); + expect(finding).toBeDefined(); + // True (1-based) line is 7; `CheckResult.range` is 0-indexed. + const result = toValeCheckResult("doc.md", finding!); + expect(result.range.start.line).toBe(6); + }); + + it("reports the true line for a raw-scope rule, correcting the leading-newline offset", () => { + const findings = runValeDirect(); + const finding = findings.find((f) => f.Check === "Test.RawScope"); + expect(finding).toBeDefined(); + // True (1-based) line is 9; `CheckResult.range` is 0-indexed. Vale itself + // reports `Line: 8` here (the blank line before), which this test would + // catch a regression back to if `leadingNewlines` were removed. + const result = toValeCheckResult("doc.md", finding!); + expect(result.range.start.line).toBe(8); + }); + + it("reports the true line and column when the leading newline follows a non-blank line", () => { + // The blank-predecessor `RawScope` case above has a zero-length + // preceding line, so an uncorrected column happens to come out right. + // A non-blank predecessor exposes that: Vale measures `Span` as a + // character count starting from that predecessor's own line, straight + // through the leading `\n` and into the flagged text, so an uncorrected + // column would land dozens of characters past where the text starts. + const findings = runValeDirect(); + const finding = findings.find((f) => f.Check === "Test.RawScopeAfterProse"); + expect(finding).toBeDefined(); + // Measured against the real binary: `Line: 11` (the 88-character + // preceding line), `Span: [89, 117]` (88 + 1, and 88 + 29 - 1, where 29 + // is the whole match's length including its leading `\n`). + expect(finding!.Line).toBe(11); + expect(finding!.Span).toEqual([89, 117]); + const result = toValeCheckResult("doc.md", finding!); + // True (1-based) line is 12; `CheckResult.range` is 0-indexed. + expect(result.range.start.line).toBe(11); + // The flagged text starts at the true line's first column, and its + // stripped length (without the leading newline) is 28, so the last + // character sits at 0-based column 27. + expect(result.range.start.column).toBe(0); + expect(result.range.end.column).toBe(27); + }); + + it("reports the true line and column for two leading newlines, not just one", () => { + // Closes the gap the reviewer found in the original single-newline-only + // real-binary coverage: this pins that Vale's line/column attribution + // for a raw match generalizes to N leading newlines, rather than only + // ever being off by exactly one. + const findings = runValeDirect(); + const finding = findings.find( + (f) => f.Check === "Test.RawScopeTwoNewlines" + ); + expect(finding).toBeDefined(); + // True (1-based) line is 16. Vale attributes the match to line 14 (two + // lines early, one per leading newline), the 73-character line + // preceding the blank separator. + expect(finding!.Line).toBe(14); + const result = toValeCheckResult("doc.md", finding!); + expect(result.range.start.line).toBe(15); + expect(result.range.start.column).toBe(0); + // Stripped match ("## Heading After Two Blank Lines") is 32 characters. + expect(result.range.end.column).toBe(31); + }); +}); + describe("toValeCheckResults", () => { it("pushes the file key down onto each finding", () => { const results = toValeCheckResults({