Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions .env.example
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
21 changes: 21 additions & 0 deletions apps/gittensory-ui/src/lib/rees-analyzers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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);
25 changes: 25 additions & 0 deletions review-enrichment/analyzer-metadata.json
Original file line number Diff line number Diff line change
Expand Up @@ -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."
}
}
]
}
89 changes: 89 additions & 0 deletions review-enrichment/src/analyzers/conflict-marker.ts
Original file line number Diff line number Diff line change
@@ -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<ConflictMarkerFinding[]> {
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;
}
30 changes: 30 additions & 0 deletions review-enrichment/src/analyzers/registry.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand Down Expand Up @@ -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(
Expand Down
1 change: 1 addition & 0 deletions review-enrichment/src/render.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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: "" };

Expand Down
9 changes: 9 additions & 0 deletions review-enrichment/src/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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[];
Expand Down Expand Up @@ -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
Expand Down
1 change: 1 addition & 0 deletions review-enrichment/test/analyzer-registry.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ const EXPECTED_ANALYZERS = [
"terminology",
"todoMarker",
"magicNumber",
"conflictMarker",
];

test("analyzer descriptors cover the runtime registry in stable order", () => {
Expand Down
106 changes: 106 additions & 0 deletions review-enrichment/test/conflict-marker.test.ts
Original file line number Diff line number Diff line change
@@ -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 }), []);
});
1 change: 1 addition & 0 deletions src/review/enrichment-analyzer-names.ts
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ export const REES_ANALYZER_NAMES = [
"terminology",
"todoMarker",
"magicNumber",
"conflictMarker",
] as const;

export type ReesAnalyzerName = (typeof REES_ANALYZER_NAMES)[number];
Expand Down
3 changes: 2 additions & 1 deletion test/unit/enrichment-wire.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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([
Expand Down Expand Up @@ -663,6 +663,7 @@ describe("resolveReesAnalyzers", () => {
"looseRange",
"terminology",
"todoMarker",
"conflictMarker",
]);
});

Expand Down
Loading