From ae56183deeac94517ca686a1a6b7d5c34a292367 Mon Sep 17 00:00:00 2001 From: jaso0n0818 Date: Sun, 5 Jul 2026 03:57:13 +0000 Subject: [PATCH] feat(enrichment): add leftover conflict-marker analyzer Closes #2032 --- .env.example | 7 +- apps/gittensory-ui/src/lib/rees-analyzers.ts | 21 ++++ review-enrichment/analyzer-metadata.json | 25 +++++ .../src/analyzers/conflict-marker.ts | 89 +++++++++++++++ review-enrichment/src/analyzers/registry.ts | 30 +++++ review-enrichment/src/render.ts | 1 + review-enrichment/src/types.ts | 9 ++ .../test/analyzer-registry.test.ts | 1 + .../test/conflict-marker.test.ts | 106 ++++++++++++++++++ src/review/enrichment-analyzer-names.ts | 1 + test/unit/enrichment-wire.test.ts | 3 +- 11 files changed, 289 insertions(+), 4 deletions(-) create mode 100644 review-enrichment/src/analyzers/conflict-marker.ts create mode 100644 review-enrichment/test/conflict-marker.test.ts diff --git a/.env.example b/.env.example index 115b0b9d8d..5e7b7c7fa1 100644 --- a/.env.example +++ b/.env.example @@ -67,22 +67,23 @@ GITTENSORY_REVIEW_ENRICHMENT=false # provenance,codeowners,secretLog,assetWeight,typosquat,commitSignature,iacMisconfig,nativeBuild # history,docCommentDrift,duplication,churnHotspot,blameLink,approvalIntegrity,ciCheckSignals # undocumentedExport,staleBranch,commitHygiene,pendingReviewRequests,testRatio,migrationSafety -# looseRange,terminology,todoMarker,magicNumber +# looseRange,terminology,todoMarker,magicNumber,conflictMarker # # Profile defaults: # fast: dependency,lockfileDrift,secret,license,installScript,heavyDependency,actionPin,eol # redos,provenance,secretLog,typosquat,iacMisconfig,nativeBuild,testRatio,migrationSafety -# looseRange,terminology,todoMarker,magicNumber +# looseRange,terminology,todoMarker,magicNumber,conflictMarker # balanced (default): dependency,lockfileDrift,secret,license,installScript,heavyDependency # actionPin,eol,redos,provenance,codeowners,secretLog,assetWeight,typosquat,commitSignature # iacMisconfig,nativeBuild,history,docCommentDrift,duplication,churnHotspot,blameLink # approvalIntegrity,ciCheckSignals,undocumentedExport,staleBranch,commitHygiene # pendingReviewRequests,testRatio,migrationSafety,looseRange,terminology,todoMarker,magicNumber +# conflictMarker # deep: dependency,lockfileDrift,secret,license,installScript,heavyDependency,actionPin,eol # redos,provenance,codeowners,secretLog,assetWeight,typosquat,commitSignature,iacMisconfig # nativeBuild,history,docCommentDrift,duplication,churnHotspot,blameLink,approvalIntegrity # ciCheckSignals,undocumentedExport,staleBranch,commitHygiene,pendingReviewRequests,testRatio -# migrationSafety,looseRange,terminology,todoMarker,magicNumber +# migrationSafety,looseRange,terminology,todoMarker,magicNumber,conflictMarker # END GENERATED REES ANALYZERS # Submitter-reputation spend control (internal-only): downgrades new/burst/low-rep diff --git a/apps/gittensory-ui/src/lib/rees-analyzers.ts b/apps/gittensory-ui/src/lib/rees-analyzers.ts index d622de7867..53ce189ba7 100644 --- a/apps/gittensory-ui/src/lib/rees-analyzers.ts +++ b/apps/gittensory-ui/src/lib/rees-analyzers.ts @@ -865,6 +865,27 @@ export const REES_ANALYZERS = [ "Precision-first: common values such as 0, 1, -1, 2, 100, 1000, and powers of ten are silent.", }, }, + { + name: "conflictMarker", + title: "Leftover conflict markers", + category: "quality", + cost: "local", + defaultEnabled: true, + profiles: ["fast", "balanced", "deep"], + requires: ["files"], + limits: { + maxFindings: 25, + }, + docs: { + summary: + "Flags leftover VCS conflict markers (`<<<<<<<`, `|||||||`, `=======`, `>>>>>>>`) accidentally committed in added lines.", + looksAt: "Added lines in every changed file.", + reports: "File, line, and the marker shape — never line content.", + network: "Pure local analyzer. No external network call.", + notes: + "Structural: an exactly-seven-character marker run at column 0. The ambiguous `=======` separator is not flagged in Markdown/AsciiDoc files, where it is a legitimate section rule.", + }, + }, ] as const satisfies readonly ReesAnalyzerDoc[]; export const REES_ANALYZER_NAMES = REES_ANALYZERS.map((analyzer) => analyzer.name); diff --git a/review-enrichment/analyzer-metadata.json b/review-enrichment/analyzer-metadata.json index fd2fc254ce..a5384b9e1c 100644 --- a/review-enrichment/analyzer-metadata.json +++ b/review-enrichment/analyzer-metadata.json @@ -973,6 +973,31 @@ "network": "Pure local analyzer. No external network call.", "notes": "Precision-first: common values such as 0, 1, -1, 2, 100, 1000, and powers of ten are silent." } + }, + { + "name": "conflictMarker", + "title": "Leftover conflict markers", + "category": "quality", + "cost": "local", + "defaultEnabled": true, + "profiles": [ + "fast", + "balanced", + "deep" + ], + "requires": [ + "files" + ], + "limits": { + "maxFindings": 25 + }, + "docs": { + "summary": "Flags leftover VCS conflict markers (`<<<<<<<`, `|||||||`, `=======`, `>>>>>>>`) accidentally committed in added lines.", + "looksAt": "Added lines in every changed file.", + "reports": "File, line, and the marker shape — never line content.", + "network": "Pure local analyzer. No external network call.", + "notes": "Structural: an exactly-seven-character marker run at column 0. The ambiguous `=======` separator is not flagged in Markdown/AsciiDoc files, where it is a legitimate section rule." + } } ] } diff --git a/review-enrichment/src/analyzers/conflict-marker.ts b/review-enrichment/src/analyzers/conflict-marker.ts new file mode 100644 index 0000000000..9080330c19 --- /dev/null +++ b/review-enrichment/src/analyzers/conflict-marker.ts @@ -0,0 +1,89 @@ +// Leftover VCS conflict-marker analyzer (#2032). Flags merge/rebase conflict markers accidentally committed in +// the ADDED lines of a PR diff — `<<<<<<<` (ours), `|||||||` (diff3 base), `=======` (separator), `>>>>>>>` +// (theirs) — a mechanical, near-zero-false-positive catch that should block a merge. Pure compute, no network. +// Detection is purely structural (a fixed run of seven identical characters at column 0), so there is no +// comment/string state to track. Line-cited via hunk headers, mirroring the sibling local analyzers. +import type { EnrichRequest, ConflictMarkerFinding } from "../types.js"; + +const MAX_FINDINGS = 25; + +// Git writes each conflict marker as EXACTLY seven identical characters at the start of the line. The ours/base/ +// theirs markers may carry a trailing space + label (a branch or commit); the separator is a bare seven `=`. +// Requiring exactly seven (not six, not eight) keeps a run of `<`/`|`/`>` unambiguous — seven of those at column +// 0 is never valid prose or code. +const OURS_RE = /^<{7}(?: .*)?$/; +const BASE_RE = /^\|{7}(?: .*)?$/; +const THEIRS_RE = /^>{7}(?: .*)?$/; +const SEPARATOR_RE = /^={7}$/; + +// A bare `=======` line is legitimate markup: a Markdown setext-H1 underline and an AsciiDoc section rule both +// use it. So the ambiguous separator is NOT flagged in markup files — but the unambiguous `<<<<<<<`/`|||||||`/ +// `>>>>>>>` markers still are, so a real conflict landing in a Markdown file is caught by those. +const MARKUP_PATH_RE = /\.(?:md|markdown|mdx|rst|adoc|asciidoc|textile)$/i; + +/** Classify one line's conflict-marker shape, or null. `allowSeparator` is false in markup files. Pure. */ +export function conflictMarkerOf( + line: string, + allowSeparator: boolean, +): ConflictMarkerFinding["marker"] | null { + if (OURS_RE.test(line)) return "<<<<<<<"; + if (BASE_RE.test(line)) return "|||||||"; + if (THEIRS_RE.test(line)) return ">>>>>>>"; + if (allowSeparator && SEPARATOR_RE.test(line)) return "======="; + return null; +} + +/** Scan one file's unified-diff patch for conflict markers on added lines, line-cited via hunk headers. Pure. */ +export function scanPatchForConflictMarkers( + path: string, + patch: string, + maxFindings: number = MAX_FINDINGS, +): ConflictMarkerFinding[] { + const findings: ConflictMarkerFinding[] = []; + if (maxFindings <= 0) return findings; + const allowSeparator = !MARKUP_PATH_RE.test(path); + let newLine = 0; + let inHunk = false; + for (const line of patch.split("\n")) { + const hunk = /^@@ -\d+(?:,\d+)? \+(\d+)(?:,\d+)? @@/.exec(line); + if (hunk) { + newLine = Number(hunk[1]); + inHunk = true; + continue; + } + // Skip the pre-hunk preamble; inside a hunk `+++x`/`+++ x` is added content, not a header. + if (!inHunk) continue; + if (line.startsWith("+")) { + const marker = conflictMarkerOf(line.slice(1), allowSeparator); + if (marker) { + findings.push({ file: path, line: newLine, marker }); + if (findings.length >= maxFindings) return findings; + } + newLine++; + } else if (!line.startsWith("-") && !line.startsWith("\\")) { + // A context line advances the new-file cursor; a removed line and a `\ No newline at end of file` + // marker do not (same class as the actions-pin / iac-misconfig line-number fix). + newLine++; + } + } + return findings; +} + +/** Analyzer entrypoint: scan every changed file's patch for leftover conflict markers. Pure, no network. */ +export async function scanConflictMarkers( + req: EnrichRequest, +): Promise { + const findings: ConflictMarkerFinding[] = []; + for (const file of req.files ?? []) { + if (!file.patch) continue; + for (const finding of scanPatchForConflictMarkers( + file.path, + file.patch, + MAX_FINDINGS - findings.length, + )) { + findings.push(finding); + if (findings.length >= MAX_FINDINGS) return findings; + } + } + return findings; +} diff --git a/review-enrichment/src/analyzers/registry.ts b/review-enrichment/src/analyzers/registry.ts index 516784b8f3..d83b3b7e3b 100644 --- a/review-enrichment/src/analyzers/registry.ts +++ b/review-enrichment/src/analyzers/registry.ts @@ -28,6 +28,7 @@ import { scanTestRatio } from "./test-ratio.js"; import { scanMigrationSafety } from "./migration-safety.js"; import { scanLooseRanges } from "./loose-range.js"; import { scanMagicNumbers } from "./magic-number.js"; +import { scanConflictMarkers } from "./conflict-marker.js"; import { scanTerminology } from "./terminology.js"; import { scanTodoMarker } from "./todo-marker.js"; import { scanTyposquat } from "./typosquat.js"; @@ -886,6 +887,35 @@ export const ANALYZER_DESCRIPTORS = [ }, run: (req, { signal }) => scanMagicNumbers(req, signal), }), + descriptor({ + name: "conflictMarker", + title: "Leftover conflict markers", + category: "quality", + cost: "local", + defaultEnabled: true, + requires: ["files"], + limits: { maxFindings: 25 }, + docs: { + summary: + "Flags leftover VCS conflict markers (`<<<<<<<`, `|||||||`, `=======`, `>>>>>>>`) accidentally committed in added lines.", + looksAt: "Added lines in every changed file.", + reports: "File, line, and the marker shape — never line content.", + network: "Pure local analyzer. No external network call.", + notes: + "Structural: an exactly-seven-character marker run at column 0. The ambiguous `=======` separator is not flagged in Markdown/AsciiDoc files, where it is a legitimate section rule.", + }, + render: (findings, helpers) => { + if (!findings.length) return []; + const lines = ["### Leftover conflict markers (unresolved merge/rebase — must be removed)"]; + for (const item of findings) { + lines.push( + `- ${helpers.safeCodeSpan(`${item.file}:${item.line}`)} — ${helpers.safeCodeSpan(item.marker)}`, + ); + } + return lines; + }, + run: (req) => scanConflictMarkers(req), + }), ] as const satisfies readonly AnyAnalyzerDescriptor[]; export const ANALYZER_NAMES = ANALYZER_DESCRIPTORS.map( diff --git a/review-enrichment/src/render.ts b/review-enrichment/src/render.ts index df05642f19..d483299f89 100644 --- a/review-enrichment/src/render.ts +++ b/review-enrichment/src/render.ts @@ -459,6 +459,7 @@ export function renderBrief( lines.push(...renderDescriptorSection("terminology", findings.terminology)); lines.push(...renderDescriptorSection("todoMarker", findings.todoMarker)); lines.push(...renderDescriptorSection("magicNumber", findings.magicNumber)); + lines.push(...renderDescriptorSection("conflictMarker", findings.conflictMarker)); if (!lines.length) return { promptSection: "", systemSuffix: "" }; diff --git a/review-enrichment/src/types.ts b/review-enrichment/src/types.ts index 175aecb10c..53f803ac82 100644 --- a/review-enrichment/src/types.ts +++ b/review-enrichment/src/types.ts @@ -447,6 +447,14 @@ export interface MagicNumberFinding { value: string; } +/** A leftover VCS conflict marker a PR committed in an added line — an unresolved merge/rebase artifact that + * must be removed (#2032, part of #1499). Reports the location + the marker shape only. */ +export interface ConflictMarkerFinding { + file: string; + line: number; + marker: "<<<<<<<" | "|||||||" | "=======" | ">>>>>>>"; +} + /** Structured analyzer output. Each analyzer fills its own key; more land as analyzers ship (#1477/#1478). */ export interface BriefFindings { dependency?: DependencyFinding[]; @@ -483,6 +491,7 @@ export interface BriefFindings { terminology?: TerminologyFinding[]; todoMarker?: TodoMarkerFinding[]; magicNumber?: MagicNumberFinding[]; + conflictMarker?: ConflictMarkerFinding[]; } /** A JSDoc/TSDoc block whose `@param` tags name parameters the adjacent function no longer declares — a diff --git a/review-enrichment/test/analyzer-registry.test.ts b/review-enrichment/test/analyzer-registry.test.ts index 0b90e20549..0a5d1cac94 100644 --- a/review-enrichment/test/analyzer-registry.test.ts +++ b/review-enrichment/test/analyzer-registry.test.ts @@ -44,6 +44,7 @@ const EXPECTED_ANALYZERS = [ "terminology", "todoMarker", "magicNumber", + "conflictMarker", ]; test("analyzer descriptors cover the runtime registry in stable order", () => { diff --git a/review-enrichment/test/conflict-marker.test.ts b/review-enrichment/test/conflict-marker.test.ts new file mode 100644 index 0000000000..83a4dece9b --- /dev/null +++ b/review-enrichment/test/conflict-marker.test.ts @@ -0,0 +1,106 @@ +// Units for the leftover conflict-marker analyzer (#2032). Own file (not enrichment.test.ts) so concurrent +// analyzer PRs don't collide. No network — pure, structural per-line detection. Runs against the compiled dist/. +import { test } from "node:test"; +import assert from "node:assert/strict"; +import { + conflictMarkerOf, + scanPatchForConflictMarkers, + scanConflictMarkers, +} from "../dist/analyzers/conflict-marker.js"; + +const patchOf = (lines) => `@@ -1,0 +1,${lines.length} @@\n${lines.map((l) => `+${l}`).join("\n")}`; + +test("conflictMarkerOf: recognizes each of the four marker shapes (with and without a label)", () => { + assert.equal(conflictMarkerOf("<<<<<<< HEAD", true), "<<<<<<<"); + assert.equal(conflictMarkerOf("<<<<<<<", true), "<<<<<<<"); + assert.equal(conflictMarkerOf("||||||| merged common ancestors", true), "|||||||"); + assert.equal(conflictMarkerOf("=======", true), "======="); + assert.equal(conflictMarkerOf(">>>>>>> feature-branch", true), ">>>>>>>"); +}); + +test("conflictMarkerOf: a run that is not exactly seven characters is not a marker", () => { + assert.equal(conflictMarkerOf("<<<<<< HEAD", true), null); // six + assert.equal(conflictMarkerOf("<<<<<<<< HEAD", true), null); // eight + assert.equal(conflictMarkerOf("======", true), null); // six + assert.equal(conflictMarkerOf("========", true), null); // eight + assert.equal(conflictMarkerOf(" <<<<<<< HEAD", true), null); // not at column 0 +}); + +test("conflictMarkerOf: a `=======` separator with a trailing label is not a bare separator", () => { + // The separator is a BARE seven `=`; anything after it (unlike ours/theirs which allow a label) is not a marker. + assert.equal(conflictMarkerOf("======= not a marker", true), null); +}); + +test("conflictMarkerOf: the `=======` separator is suppressed in markup (allowSeparator=false) but ours/theirs are not", () => { + assert.equal(conflictMarkerOf("=======", false), null); // setext-H1 underline / AsciiDoc rule + assert.equal(conflictMarkerOf("<<<<<<< HEAD", false), "<<<<<<<"); // a real conflict still caught in markup + assert.equal(conflictMarkerOf(">>>>>>> theirs", false), ">>>>>>>"); +}); + +test("scanPatchForConflictMarkers: flags a full three-way conflict on added lines with correct locations", () => { + const findings = scanPatchForConflictMarkers( + "src/app.ts", + patchOf(["<<<<<<< HEAD", "const x = 1;", "=======", "const x = 2;", ">>>>>>> other"]), + ); + assert.deepEqual(findings, [ + { file: "src/app.ts", line: 1, marker: "<<<<<<<" }, + { file: "src/app.ts", line: 3, marker: "=======" }, + { file: "src/app.ts", line: 5, marker: ">>>>>>>" }, + ]); +}); + +test("scanPatchForConflictMarkers: a markdown setext-H1 underline (=======) is not flagged", () => { + const findings = scanPatchForConflictMarkers( + "docs/guide.md", + patchOf(["My Heading", "=======", "Some prose."]), + ); + assert.deepEqual(findings, []); +}); + +test("scanPatchForConflictMarkers: a real conflict landing in a markdown file is still caught by ours/theirs", () => { + const findings = scanPatchForConflictMarkers( + "docs/guide.md", + patchOf(["<<<<<<< HEAD", "old text", "=======", "new text", ">>>>>>> branch"]), + ); + // The `=======` is suppressed in markup, but the ours/theirs markers still fire. + assert.deepEqual(findings, [ + { file: "docs/guide.md", line: 1, marker: "<<<<<<<" }, + { file: "docs/guide.md", line: 5, marker: ">>>>>>>" }, + ]); +}); + +test("scanPatchForConflictMarkers: only ADDED lines are scanned; new-file line numbers stay correct", () => { + const patch = [ + "@@ -10,2 +10,2 @@", + " function f() {", // context line 10 + "-=======", // removed, does not advance + "+>>>>>>> feature", // new-file line 11 + ].join("\n"); + assert.deepEqual(scanPatchForConflictMarkers("src/a.ts", patch), [ + { file: "src/a.ts", line: 11, marker: ">>>>>>>" }, + ]); +}); + +test("scanPatchForConflictMarkers: enforces the maxFindings cap", () => { + const lines = Array.from({ length: 30 }, () => "<<<<<<< HEAD"); + assert.equal(scanPatchForConflictMarkers("src/a.ts", patchOf(lines), 5).length, 5); + assert.deepEqual(scanPatchForConflictMarkers("src/a.ts", patchOf(lines), 0), []); +}); + +test("scanConflictMarkers: scans every changed file and honors the global cap", async () => { + const markers = Array.from({ length: 30 }, () => ">>>>>>> b"); + const findings = await scanConflictMarkers({ + repoFullName: "octo/repo", + prNumber: 1, + files: [ + { path: "src/a.ts", patch: patchOf(["const ok = true;"]) }, + { path: "src/b.ts", patch: patchOf(markers) }, + ], + }); + assert.equal(findings.length, 25); + assert.ok(findings.every((f) => f.file === "src/b.ts")); +}); + +test("scanConflictMarkers: no files yields no findings", async () => { + assert.deepEqual(await scanConflictMarkers({ repoFullName: "octo/repo", prNumber: 1 }), []); +}); diff --git a/src/review/enrichment-analyzer-names.ts b/src/review/enrichment-analyzer-names.ts index 3690d8de16..4191c2d16b 100644 --- a/src/review/enrichment-analyzer-names.ts +++ b/src/review/enrichment-analyzer-names.ts @@ -38,6 +38,7 @@ export const REES_ANALYZER_NAMES = [ "terminology", "todoMarker", "magicNumber", + "conflictMarker", ] as const; export type ReesAnalyzerName = (typeof REES_ANALYZER_NAMES)[number]; diff --git a/test/unit/enrichment-wire.test.ts b/test/unit/enrichment-wire.test.ts index 71ea8a995f..606be32c8e 100644 --- a/test/unit/enrichment-wire.test.ts +++ b/test/unit/enrichment-wire.test.ts @@ -626,7 +626,7 @@ describe("resolveReesAnalyzers", () => { resolveReesAnalyzers( env({ REES_ANALYZERS: - "dependency,lockfileDrift,secret,license,installScript,heavyDependency,actionPin,eol,redos,provenance,codeowners,secretLog,assetWeight,typosquat,commitSignature,iacMisconfig,nativeBuild,history,docCommentDrift,duplication,churnHotspot,blameLink,approvalIntegrity,ciCheckSignals,undocumentedExport,staleBranch,commitHygiene,pendingReviewRequests,testRatio,migrationSafety,looseRange,terminology,todoMarker", + "dependency,lockfileDrift,secret,license,installScript,heavyDependency,actionPin,eol,redos,provenance,codeowners,secretLog,assetWeight,typosquat,commitSignature,iacMisconfig,nativeBuild,history,docCommentDrift,duplication,churnHotspot,blameLink,approvalIntegrity,ciCheckSignals,undocumentedExport,staleBranch,commitHygiene,pendingReviewRequests,testRatio,migrationSafety,looseRange,terminology,todoMarker,conflictMarker", }), ), ).toEqual([ @@ -663,6 +663,7 @@ describe("resolveReesAnalyzers", () => { "looseRange", "terminology", "todoMarker", + "conflictMarker", ]); });