Skip to content

Commit ccd23d5

Browse files
fix(rees): scan added lines whose content starts with ++ across all diff parsers (#2531)
Every review-enrichment diff parser guarded file headers with an unanchored `startsWith("+++")`. git renders an added line whose content is `++x` as `+` + `++x` = `+++x` (and `++ x` as `+++ x`), so that line was mistaken for a `+++ b/file` header and skipped. In secret-scan.ts a secret on such a line was never scanned (scanSecrets calls scanPatch directly). Fixes, by parser input shape: - Hunk-structured parsers (secret-scan, secret-log, iac-misconfig, redos, actions-pin, eol-check, duplication-scan, shared analysis-context) track hunk state: headers only precede the first @@, and inside a hunk the first char is the +/-/space op, so both `+++x` and `+++ x` are scanned. - Parsers that also accept header-only/headerless fragments (history, heavy-dependency) use a shared isDiffFileHeaderLine helper that matches only a real header form (`+++ b/…`/`--- a/…`/`/dev/null`), so `+++ x` content is kept while true headers are skipped — and headerless single-line diffs still work. - Manifest/lockfile parsers (dependency-scan, lockfile-drift) keep an anchored `+++ ` guard; their content never begins a line with `++`. Adds regression tests: scanPatch finds a secret on both `++x`- and `++ x`-content lines, collectAddedLines recovers the `++x` line, and isDiffFileHeaderLine has a unit covering headers vs `++`/`--` content and headerless diffs. No issue because issue creation is restricted on this repo; this consolidates a duplicated diff-parsing guard, no schema or API change.
1 parent a9d6e0c commit ccd23d5

16 files changed

Lines changed: 126 additions & 17 deletions

review-enrichment/src/analysis-context.ts

Lines changed: 13 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -309,14 +309,17 @@ export function collectAddedLines(
309309
for (const file of files) {
310310
if (!file.patch) continue;
311311
let newLine = 0;
312+
let inHunk = false;
312313
for (const line of boundedPatchLines(file.patch, options.metrics, "added_lines_patch_bytes")) {
313-
if (line.startsWith("+++") || line.startsWith("---")) continue;
314-
if (line.startsWith("diff ") || line.startsWith("index ")) continue;
315314
const hunk = /^@@ -\d+(?:,\d+)? \+(\d+)(?:,\d+)? @@/.exec(line);
316315
if (hunk) {
317316
newLine = Number(hunk[1]);
317+
inHunk = true;
318318
continue;
319319
}
320+
// Skip the pre-hunk preamble (diff/index + the `+++ `/`--- ` file headers). INSIDE a hunk the first char
321+
// is the +/-/space op, so `+++x`/`+++ x` added content is collected, not mistaken for a header.
322+
if (!inHunk) continue;
320323
if (line.startsWith("+")) {
321324
if (addedLines.length >= MAX_CONTEXT_ADDED_LINES) {
322325
options.metrics?.recordCappedWork("added_lines", 1);
@@ -365,13 +368,19 @@ export function filesHaveAddedLines(
365368
): boolean {
366369
for (const file of files) {
367370
if (!file.patch) continue;
371+
let inHunk = false;
368372
for (const line of boundedPatchLines(
369373
file.patch,
370374
options.metrics,
371375
"has_added_lines_patch_bytes",
372376
)) {
373-
if (line.startsWith("+++") || line.startsWith("---")) continue;
374-
if (line.startsWith("+")) return true;
377+
if (line.startsWith("@@")) {
378+
inHunk = true;
379+
continue;
380+
}
381+
// Inside a hunk every added line starts with `+` (including `+++x`/`+++ x` content); the `+++ `/`--- `
382+
// headers only appear in the pre-hunk preamble.
383+
if (inHunk && line.startsWith("+")) return true;
375384
}
376385
if (file.patch.length > MAX_CONTEXT_PATCH_BYTES) return true;
377386
}

review-enrichment/src/analyzers/actions-pin.ts

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -16,13 +16,16 @@ export function scanWorkflowPins(
1616
): ActionPinFinding[] {
1717
const findings: ActionPinFinding[] = [];
1818
let newLine = 0;
19+
let inHunk = false;
1920
for (const line of patch.split("\n")) {
20-
if (line.startsWith("+++") || line.startsWith("---")) continue;
2121
const hunk = /^@@ -\d+(?:,\d+)? \+(\d+)(?:,\d+)? @@/.exec(line);
2222
if (hunk) {
2323
newLine = Number(hunk[1]);
24+
inHunk = true;
2425
continue;
2526
}
27+
// Skip pre-hunk preamble; inside a hunk `+++x`/`+++ x` is added content, not a header.
28+
if (!inHunk) continue;
2629
if (line.startsWith("+")) {
2730
const match = USES_RE.exec(line.slice(1));
2831
if (match) {

review-enrichment/src/analyzers/dependency-scan.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -109,7 +109,7 @@ export function extractDependencyChanges(
109109
const sign = line[0];
110110
if (
111111
(sign !== "+" && sign !== "-") ||
112-
line.startsWith("+++") ||
112+
line.startsWith("+++ ") ||
113113
line.startsWith("---")
114114
)
115115
continue;
Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,17 @@
1+
/**
2+
* Shared unified-diff line helpers for analyzers that scan patch fragments which may or may not include hunk
3+
* headers (so they cannot rely on hunk state).
4+
*/
5+
6+
/**
7+
* True only for a real unified-diff FILE HEADER — `+++ b/path`, `--- a/path`, or `+++ `/`--- /dev/null`
8+
* (marker run + space + an `a/`/`b/` prefix or `/dev/null`).
9+
*
10+
* This deliberately does NOT match added/removed CONTENT whose text begins with `++`/`--`: git renders an
11+
* added line whose content is `++x` as `+` + `++x` = `+++x`, and `++ x` as `+++ x`. An anchored
12+
* `startsWith("+++ ")` guard skips `+++ x` as if it were a header and drops the real added line; keying on the
13+
* header's path form scans that content while still skipping true headers.
14+
*/
15+
export function isDiffFileHeaderLine(line: string): boolean {
16+
return /^(?:\+\+\+|---) (?:[ab]\/|\/dev\/null)/.test(line);
17+
}

review-enrichment/src/analyzers/duplication-scan.ts

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -184,7 +184,6 @@ export function extractAddedBlocks(patch: string | undefined): NormBlock[] {
184184
continue;
185185
}
186186
if (!inHunk) continue;
187-
if (line.startsWith("+++")) continue; // file header inside the patch, not an added line
188187
if (line.startsWith("+")) {
189188
const norm = normalizeLine(line.slice(1));
190189
if (norm === null) {

review-enrichment/src/analyzers/eol-check.ts

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -59,11 +59,17 @@ export function extractVersionPins(
5959
if (filesScanned >= MAX_EOL_FILES) break;
6060
filesScanned += 1;
6161
const base = file.path.split("/").pop() ?? file.path;
62+
let inHunk = false;
6263
for (const raw of file.patch.split("\n")) {
6364
if (linesScanned >= MAX_EOL_PATCH_LINES || pins.length >= MAX_EOL_PINS)
6465
return pins;
6566
linesScanned += 1;
66-
if (raw[0] !== "+" || raw.startsWith("+++")) continue;
67+
if (raw.startsWith("@@")) {
68+
inHunk = true;
69+
continue;
70+
}
71+
// Only added content inside a hunk; the `+++ ` header precedes the first hunk.
72+
if (!inHunk || raw[0] !== "+") continue;
6773
const line = raw.slice(1).trim();
6874
if (isDockerfile(file.path)) {
6975
const match =

review-enrichment/src/analyzers/heavy-dependency.ts

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ import type {
88
} from "../types.js";
99
import type { AnalysisContext } from "../analysis-context.js";
1010
import { extractDependencyChanges } from "./dependency-scan.js";
11+
import { isDiffFileHeaderLine } from "./diff-lines.js";
1112
import { boundedFetchJson } from "../external-fetch.js";
1213

1314
const MAX_WEIGHT_LOOKUPS = 20;
@@ -59,7 +60,8 @@ function addedPatchLines(
5960
continue;
6061
}
6162
if (raw.startsWith("\\ No newline")) continue;
62-
if (raw.startsWith("+") && !raw.startsWith("+++")) {
63+
// Skip real file headers (`+++ b/…`) but scan added CONTENT that begins with `++` (rendered `+++x`/`+++ x`).
64+
if (raw.startsWith("+") && !isDiffFileHeaderLine(raw)) {
6365
lines.push({
6466
file: file.path,
6567
line: nextLine || 1,
@@ -68,7 +70,7 @@ function addedPatchLines(
6870
nextLine += 1;
6971
continue;
7072
}
71-
if (raw.startsWith("-") && !raw.startsWith("---")) continue;
73+
if (raw.startsWith("-") && !isDiffFileHeaderLine(raw)) continue;
7274
if (nextLine) nextLine += 1;
7375
}
7476
}

review-enrichment/src/analyzers/history.ts

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@
1212
import type { AnalyzerDiagnostics, EnrichRequest, HistoryFinding } from "../types.js";
1313
import type { AnalysisContext } from "../analysis-context.js";
1414
import { boundedFetchJson } from "../external-fetch.js";
15+
import { isDiffFileHeaderLine } from "./diff-lines.js";
1516

1617
const GITHUB_API = "https://api.github.com";
1718
const GITHUB_API_VERSION = "2022-11-28";
@@ -225,7 +226,8 @@ function diffAddedText(req: EnrichRequest): string {
225226
const added: string[] = [];
226227
for (const src of sources) {
227228
for (const line of src.split("\n")) {
228-
if (line.startsWith("+") && !line.startsWith("+++")) added.push(line.slice(1));
229+
// Skip real file headers (`+++ b/…`) but keep added CONTENT that begins with `++` (rendered `+++x`/`+++ x`).
230+
if (line.startsWith("+") && !isDiffFileHeaderLine(line)) added.push(line.slice(1));
229231
}
230232
}
231233
return added.join("\n");

review-enrichment/src/analyzers/iac-misconfig.ts

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -75,10 +75,10 @@ export function scanPatchForIacMisconfig(
7575
let secureFalseLine = 0;
7676
let prodLine = 0;
7777
let debugLine = 0;
78+
let inHunk = false;
7879

7980
for (const line of patchLines(patch)) {
8081
if (limits.signal?.aborted) throw new Error("analyzer_aborted");
81-
if (line.startsWith("+++") || line.startsWith("---")) continue;
8282
const hunk = /^@@ -\d+(?:,\d+)? \+(\d+)(?:,\d+)? @@/.exec(line);
8383
if (hunk) {
8484
newLine = Number(hunk[1]);
@@ -88,8 +88,11 @@ export function scanPatchForIacMisconfig(
8888
secureFalseLine = 0;
8989
prodLine = 0;
9090
debugLine = 0;
91+
inHunk = true;
9192
continue;
9293
}
94+
// Skip pre-hunk preamble; inside a hunk `+++x`/`+++ x` is added content, not a header.
95+
if (!inHunk) continue;
9396
if (!line.startsWith("+")) {
9497
if (!line.startsWith("-")) newLine++;
9598
continue;

review-enrichment/src/analyzers/lockfile-drift.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -120,7 +120,7 @@ function* patchLines(
120120
for (const raw of patch.split("\n")) {
121121
seen += 1;
122122
if (seen > maxLines) break;
123-
if (raw.startsWith("+++") || raw.startsWith("---")) continue;
123+
if (raw.startsWith("+++ ") || raw.startsWith("---")) continue;
124124
const hunk = /^@@ -\d+(?:,\d+)? \+(\d+)(?:,\d+)? @@/.exec(raw);
125125
if (hunk) {
126126
newLine = Number(hunk[1]);

0 commit comments

Comments
 (0)