diff --git a/script/upstream/analyze.test.ts b/script/upstream/analyze.test.ts index 954bf0ef4c..2d082cf116 100644 --- a/script/upstream/analyze.test.ts +++ b/script/upstream/analyze.test.ts @@ -1,5 +1,5 @@ import { describe, test, expect } from "bun:test" -import { parseDiffForMarkerWarnings } from "./analyze" +import { parseDiffForMarkerWarnings, computeMarkedLines } from "./analyze" // Helper to create a unified diff string from lines function makeDiff(hunks: string): string { @@ -300,3 +300,93 @@ describe("parseDiffForMarkerWarnings", () => { expect(warnings[0].context).toContain("UpgradeIndicator") }) }) + +describe("computeMarkedLines", () => { + test("covers the marker lines and everything between", () => { + const content = ["const a = 1", "// altimate_change start — x", "const b = 2", "// altimate_change end", "const c = 3"].join("\n") + const marked = computeMarkedLines(content) + expect(marked.has(1)).toBe(false) // before block + expect(marked.has(2)).toBe(true) // start marker + expect(marked.has(3)).toBe(true) // inside + expect(marked.has(4)).toBe(true) // end marker + expect(marked.has(5)).toBe(false) // after block + }) + + test("handles nested start/end pairs with a depth counter", () => { + const content = [ + "const a = 1", // 1 + "// altimate_change start — outer", // 2 + "const b = 2", // 3 + "// altimate_change start — inner", // 4 + "const c = 3", // 5 + "// altimate_change end", // 6 (closes inner; still inside outer) + "const d = 4", // 7 still inside outer + "// altimate_change end", // 8 (closes outer) + "const e = 5", // 9 outside + ].join("\n") + const marked = computeMarkedLines(content) + expect(marked.has(1)).toBe(false) + expect(marked.has(3)).toBe(true) + expect(marked.has(5)).toBe(true) + expect(marked.has(7)).toBe(true) // still inside outer after inner closed — the bug a single openBlock would miss + expect(marked.has(8)).toBe(true) // outer end marker + expect(marked.has(9)).toBe(false) + }) + + test("unbalanced extra end does not drive depth negative", () => { + const content = ["// altimate_change end", "const a = 1", "// altimate_change start — x", "const b = 2", "// altimate_change end"].join("\n") + const marked = computeMarkedLines(content) + expect(marked.has(2)).toBe(false) // not inside any block + expect(marked.has(4)).toBe(true) // inside the real block + }) +}) + +describe("parseDiffForMarkerWarnings + full-file coverage (context-window false positive)", () => { + // Reproduces the worker.ts:183 CI failure: a line is MODIFIED deep inside a + // pre-existing `altimate_change` block, but the block's `start` marker is + // further than the diff's ±context window, so it never appears in the hunk. + const diffModifyingInsideBlock = makeDiff( + `@@ -180,7 +180,7 @@ + const ctxA = 1 + const ctxB = 2 + const ctxC = 3 +-const trace = oldTrace() ++const trace = newTrace() + const ctxD = 4 + const ctxE = 5 + const ctxF = 6`, + ) + + test("WITHOUT coverage map: false-positives (documents the bug)", () => { + const warnings = parseDiffForMarkerWarnings("worker.ts", diffModifyingInsideBlock) + expect(warnings).toHaveLength(1) + expect(warnings[0].context).toContain("trace = newTrace()") + }) + + test("WITH coverage map saying line 183 is inside a marked block: no warning", () => { + // Pretend the full file has a marked block spanning lines 160-210; the + // changed line (183) is covered even though no marker appears in the hunk. + const marked = new Set() + for (let i = 160; i <= 210; i++) marked.add(i) + const warnings = parseDiffForMarkerWarnings("worker.ts", diffModifyingInsideBlock, marked) + expect(warnings).toEqual([]) + }) + + test("WITH coverage map NOT covering the line: still warns (no over-suppression)", () => { + const marked = new Set([1, 2, 3]) // unrelated lines + const warnings = parseDiffForMarkerWarnings("worker.ts", diffModifyingInsideBlock, marked) + expect(warnings).toHaveLength(1) + }) + + test("end-to-end: computeMarkedLines feeds the parser for a real reconstructed file", () => { + // Build a file where the change at line 183 sits inside a 160-210 block. + const fileLines: string[] = [] + for (let i = 1; i <= 159; i++) fileLines.push(`const before${i} = ${i}`) + fileLines.push("// altimate_change start — big block") // 160 + for (let i = 161; i <= 209; i++) fileLines.push(`const inside${i} = ${i}`) + fileLines.push("// altimate_change end") // 210 + const marked = computeMarkedLines(fileLines.join("\n")) + const warnings = parseDiffForMarkerWarnings("worker.ts", diffModifyingInsideBlock, marked) + expect(warnings).toEqual([]) + }) +}) diff --git a/script/upstream/analyze.ts b/script/upstream/analyze.ts index 5539b12667..3875fc1538 100644 --- a/script/upstream/analyze.ts +++ b/script/upstream/analyze.ts @@ -733,7 +733,45 @@ function isUpstreamShared(file: string, config: MergeConfig): boolean { } // altimate_change start — exported for unit testing -export function parseDiffForMarkerWarnings(file: string, diffOutput: string): MarkerWarning[] { +/** + * Compute the set of 1-based line numbers in `content` that are covered by an + * `altimate_change start … altimate_change end` block (the marker lines + * themselves are covered too). A depth counter handles nested blocks, so an + * inner start/end pair inside an outer one is tracked correctly. + * + * This is derived from the FULL file, independent of any diff context window — + * which is the whole point. The diff-based tracker in + * `parseDiffForMarkerWarnings` only sees ±N context lines around a change, so a + * line modified deep inside a large pre-existing marked block (its `start` + * marker outside the hunk) looks unmarked and false-positives. Passing the + * full-file coverage in lets the parser suppress those false positives. + */ +export function computeMarkedLines(content: string): Set { + const marked = new Set() + const lines = content.split("\n") + let depth = 0 + for (let i = 0; i < lines.length; i++) { + const line = lines[i] + if (line.includes("altimate_change start")) { + depth++ + marked.add(i + 1) + continue + } + if (line.includes("altimate_change end")) { + marked.add(i + 1) + depth = Math.max(0, depth - 1) + continue + } + if (depth > 0) marked.add(i + 1) + } + return marked +} + +export function parseDiffForMarkerWarnings( + file: string, + diffOutput: string, + markedLines?: Set, +): MarkerWarning[] { const warnings: MarkerWarning[] = [] if (!diffOutput.trim()) return warnings @@ -790,7 +828,12 @@ export function parseDiffForMarkerWarnings(file: string, diffOutput: string): Ma if (content.startsWith("import ")) continue if (content.startsWith("export ")) continue - if (!inMarkerBlock) { + // A line is covered if either the in-hunk tracker saw its enclosing + // `start` marker, OR the full-file coverage map (when provided) says this + // line sits inside a marked block. The full-file map is what rescues + // changes whose `start` marker lives outside the diff's context window. + const coveredByFullFile = markedLines?.has(currentLine) ?? false + if (!inMarkerBlock && !coveredByFullFile) { if (!hasNewCode) { hasNewCode = true newCodeStart = currentLine @@ -827,7 +870,22 @@ function checkFileForMarkers(file: string, base?: string): MarkerWarning[] { return [] } - return parseDiffForMarkerWarnings(file, diffOutput) + // Coverage from the FULL post-change file, so a change deep inside a + // pre-existing marked block isn't false-flagged just because its `start` + // marker fell outside the diff's ±5-line context window. The "new" side is + // HEAD when diffing against a base, or the working tree otherwise — matching + // the side the `+`/context line numbers in the diff refer to. + let markedLines: Set | undefined + try { + const newContent = base + ? execSync(`git show HEAD:"${file}"`, { cwd: root, encoding: "utf-8" }) + : require("fs").readFileSync(require("path").join(root, file), "utf-8") + markedLines = computeMarkedLines(newContent) + } catch { + markedLines = undefined + } + + return parseDiffForMarkerWarnings(file, diffOutput, markedLines) } function runMarkerCheck(config: MergeConfig, base?: string, strict?: boolean): number {