fix(cli): correct raw-scope Vale line numbers

.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).

packages/cli/src/rules/vale/map.ts

@@ -143,27 +143,52 @@ 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;
+}
+
 /**
  * Map one Vale finding to the scanner-agnostic {@link CheckResult}.
  *
  * `range` collapses to a single line: Vale reports `Line` plus a `Span` of
  * 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
- * (unset, rather than a real position) must not become -1.
+ * The line is corrected for a `raw`-scope match's leading newlines (see
+ * {@link leadingNewlines}) and then 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 (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 line = Math.max(0, finding.Line + leadingNewlines(finding.Match) - 1);
   const startColumn = Math.max(0, spanStart - 1);
   const endColumn = Math.max(0, spanEnd - 1);
   return {

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,56 @@ 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);
+    });
+  });
+
   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 +203,105 @@ 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.
+   *
+   * 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)
+   */
+  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.",
+    "",
+  ].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"
+    );
+    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<string, ValeFinding[]>;
+    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);
+  });
+});
+
 describe("toValeCheckResults", () => {
   it("pushes the file key down onto each finding", () => {
     const results = toValeCheckResults({