fix: tries to solve a bug where layne doesn't suppress preexisting findings with security comment (#26)

Author: julio-rocketchat

src/__tests__/suppressor.test.js

@@ -15,7 +15,22 @@ function rejectWith(err = new Error('not found')) {
   mockExecFile.mockImplementationOnce((_cmd, _args, cb) => cb(err, '', ''));
 }
 
-const BASE = { workspacePath: '/workspace', baseSha: 'base-sha' };
+// Sets up both the git diff (identity map: headLine === baseLine) and git show
+// (file content) calls for a single file. Use when line shifts are not relevant.
+function mockFile(filePath, content) {
+  const lines = content.split('\n');
+  const count = lines[lines.length - 1] === '' ? lines.length - 1 : lines.length;
+  const diff = [
+    `--- a/${filePath}`,
+    `+++ b/${filePath}`,
+    `@@ -1,${count} +1,${count} @@`,
+    ...Array(count).fill(' x'),
+  ].join('\n');
+  resolveWith(diff);    // for buildLineMapForFile (git diff)
+  resolveWith(content); // for gitShow (git show)
+}
+
+const BASE = { workspacePath: '/workspace', baseSha: 'base-sha', headSha: 'head-sha' };
 
 const finding = (overrides = {}) => ({
   file: 'src/app.js',
@@ -37,61 +52,61 @@ describe('suppressFindings()', () => {
   });
 
   it('keeps a finding when git show fails (new file)', async () => {
-    rejectWith(new Error('fatal: path not in tree'));
+    resolveWith(''); // diff: empty map → fall back to head line
+    rejectWith(new Error('fatal: path not in tree')); // show: fails
     const f = finding();
     const result = await suppressFindings([f], BASE);
     expect(result).toEqual([f]);
   });
 
   it('keeps a finding when there is no SECURITY: comment at base', async () => {
-    resolveWith('line1\nline2\nline3\nline4\nline5\nline6\n');
+    mockFile('src/app.js', 'line1\nline2\nline3\nline4\nline5\nline6\n');
     const f = finding({ line: 5 });
     const result = await suppressFindings([f], BASE);
     expect(result).toEqual([f]);
   });
 
   it('suppresses a finding when // SECURITY: reason is on the same line', async () => {
-    resolveWith('line1\nline2\nline3\nline4\nconst x = 1; // SECURITY: reviewed by alice\nline6\n');
+    mockFile('src/app.js', 'line1\nline2\nline3\nline4\nconst x = 1; // SECURITY: reviewed by alice\nline6\n');
     const result = await suppressFindings([finding({ line: 5 })], BASE);
     expect(result).toEqual([]);
   });
 
   it('suppresses a finding when // SECURITY: reason is on the line immediately above', async () => {
-    resolveWith('line1\nline2\nline3\n// SECURITY: approved in issue #42\nconst x = 1;\nline6\n');
+    mockFile('src/app.js', 'line1\nline2\nline3\n// SECURITY: approved in issue #42\nconst x = 1;\nline6\n');
     const result = await suppressFindings([finding({ line: 5 })], BASE);
     expect(result).toEqual([]);
   });
 
   it('suppresses a finding with # SECURITY: (YAML/shell comment style)', async () => {
-    resolveWith('line1\nline2\nline3\nline4\n# SECURITY: checked for injection\nline6\n');
+    mockFile('src/app.js', 'line1\nline2\nline3\nline4\n# SECURITY: checked for injection\nline6\n');
     const result = await suppressFindings([finding({ line: 5 })], BASE);
     expect(result).toEqual([]);
   });
 
   it('does NOT suppress when // SECURITY: has no text after the colon', async () => {
-    resolveWith('line1\nline2\nline3\nline4\n// SECURITY:\nline6\n');
+    mockFile('src/app.js', 'line1\nline2\nline3\nline4\n// SECURITY:\nline6\n');
     const f = finding({ line: 5 });
     const result = await suppressFindings([f], BASE);
     expect(result).toEqual([f]);
   });
 
-  it('calls execFile only once for two findings in the same file (cache hit)', async () => {
-    resolveWith('line1\nline2\nline3\nline4\nline5\n');
+  it('calls execFile exactly twice for two findings in the same file (cache hit)', async () => {
+    mockFile('src/app.js', 'line1\nline2\nline3\nline4\nline5\n');
     const f1 = finding({ line: 2 });
     const f2 = finding({ line: 4 });
     await suppressFindings([f1, f2], BASE);
-    expect(mockExecFile).toHaveBeenCalledTimes(1);
+    expect(mockExecFile).toHaveBeenCalledTimes(2);
   });
 
   it('does not crash and keeps the finding when finding.line === 1', async () => {
-    resolveWith('const x = 1;\nline2\n');
+    mockFile('src/app.js', 'const x = 1;\nline2\n');
     const f = finding({ line: 1 });
     const result = await suppressFindings([f], BASE);
     expect(result).toEqual([f]);
   });
 
   it('does not suppress an unvalidated Claude finding', async () => {
-    resolveWith('line1\n// SECURITY: unrelated prior approval\nline3\n');
     const f = finding({
       tool: 'claude',
       line: 2,
@@ -103,10 +118,11 @@ describe('suppressFindings()', () => {
     const result = await suppressFindings([f], BASE);
 
     expect(result).toEqual([f]);
+    expect(mockExecFile).not.toHaveBeenCalled();
   });
 
   it('uses suppressionLine when checking for SECURITY comments', async () => {
-    resolveWith('line1\nline2\n// SECURITY: approved in prior PR\nline4\n');
+    mockFile('src/app.js', 'line1\nline2\n// SECURITY: approved in prior PR\nline4\n');
 
     const result = await suppressFindings([finding({
       tool: 'claude',
@@ -122,14 +138,8 @@ describe('suppressFindings()', () => {
   });
 
   it('returns the correct filtered array for a mixed batch', async () => {
-    // file A: has a SECURITY comment above line 3
-    const fileAContent = 'line1\n// SECURITY: reviewed by bob\nconst y = 2;\nline4\n';
-    // file B: no SECURITY comment
-    const fileBContent = 'line1\nline2\nline3\n';
-
-    mockExecFile
-      .mockImplementationOnce((_cmd, _args, cb) => cb(null, fileAContent, ''))
-      .mockImplementationOnce((_cmd, _args, cb) => cb(null, fileBContent, ''));
+    mockFile('src/a.js', 'line1\n// SECURITY: reviewed by bob\nconst y = 2;\nline4\n');
+    mockFile('src/b.js', 'line1\nline2\nline3\n');
 
     const fA = finding({ file: 'src/a.js', line: 3 }); // should be suppressed
     const fB = finding({ file: 'src/b.js', line: 1 }); // should be kept
@@ -139,8 +149,9 @@ describe('suppressFindings()', () => {
   });
 
   it('handles git show failure for file A independently from success for file B', async () => {
-    rejectWith(new Error('not found'));
-    resolveWith('line1\nline2\nline3\n');
+    resolveWith(''); // fA: diff returns empty → fall back to head line
+    rejectWith(new Error('not found')); // fA: show fails
+    mockFile('src/b.js', 'line1\nline2\nline3\n'); // fB: diff + show succeed
 
     const fA = finding({ file: 'src/a.js', line: 2 });
     const fB = finding({ file: 'src/b.js', line: 2 });
@@ -149,4 +160,72 @@ describe('suppressFindings()', () => {
     // fA kept (git show failed), fB kept (no SECURITY comment)
     expect(result).toEqual([fA, fB]);
   });
+
+  // --- Line-shift tests ---
+
+  it('suppresses a finding on a shifted pre-existing line', async () => {
+    // PR inserts one line at the top; everything shifts down by 1.
+    // Finding is at head line 5 (ignoreSsrfValidation) → maps to base line 4.
+    // SECURITY comment is at base line 3 (lineAbove base line 4) → suppressed.
+    const diff = [
+      '--- a/src/app.js',
+      '+++ b/src/app.js',
+      '@@ -1,5 +1,6 @@',
+      '+INSERTED',
+      ' line1',
+      ' line2',
+      ' // SECURITY: approved in prior PR',
+      ' ignoreSsrfValidation: true',
+      ' line5',
+    ].join('\n');
+    resolveWith(diff);
+    resolveWith('line1\nline2\n// SECURITY: approved in prior PR\nignoreSsrfValidation: true\nline5\n');
+
+    const result = await suppressFindings([finding({ line: 5 })], BASE);
+    expect(result).toEqual([]);
+  });
+
+  it('keeps a finding on a newly added line', async () => {
+    // ignoreSsrfValidation: true is a + line in the diff — must not be suppressed
+    // even though a SECURITY comment exists nearby.
+    const diff = [
+      '--- a/src/app.js',
+      '+++ b/src/app.js',
+      '@@ -1,4 +1,5 @@',
+      ' line1',
+      ' line2',
+      '+ignoreSsrfValidation: true',
+      ' // SECURITY: pre-existing comment for something else',
+      ' line4',
+    ].join('\n');
+    resolveWith(diff); // git show is never reached for a newly added line
+
+    const result = await suppressFindings([finding({ line: 3 })], BASE);
+    expect(result).toEqual([finding({ line: 3 })]);
+    expect(mockExecFile).toHaveBeenCalledTimes(1);
+  });
+
+  it('keeps a finding when there is no SECURITY comment at the correct mapped base line', async () => {
+    // PR inserts one line at the top; finding at head line 5 → base line 4.
+    // Base line 5 has a SECURITY comment — the old (broken) suppressor would
+    // have checked base line 5 (using the head line number directly) and
+    // wrongly suppressed. The new suppressor checks base line 4 and correctly keeps.
+    const diff = [
+      '--- a/src/app.js',
+      '+++ b/src/app.js',
+      '@@ -1,5 +1,6 @@',
+      '+INSERTED',
+      ' line1',
+      ' line2',
+      ' line3',
+      ' ignoreSsrfValidation: true',
+      ' // SECURITY: comment at base line 5, not line 3',
+    ].join('\n');
+    resolveWith(diff);
+    resolveWith('line1\nline2\nline3\nignoreSsrfValidation: true\n// SECURITY: comment at base line 5, not line 3\n');
+
+    // head line 5 → base line 4; lineAbove = base line 3 = 'line3' → no match
+    const result = await suppressFindings([finding({ line: 5 })], BASE);
+    expect(result).toEqual([finding({ line: 5 })]);
+  });
 });

src/__tests__/worker.test.js

@@ -233,6 +233,7 @@ describe('processJob()', () => {
       expect(suppressFindings).toHaveBeenCalledWith(rawFindings, {
         workspacePath: '/tmp/layne-test-workspace',
         baseSha:       'merge-base-sha',
+        headSha:       'abc123',
       });
     });
 

src/fetcher.js

@@ -150,6 +150,65 @@ function isWithinPath(targetPath, basePath) {
   return targetPath === basePath || targetPath.startsWith(`${basePath}${sep}`);
 }
 
+/**
+ * Builds a Map<headLine, baseLine | null> for a single file by parsing the
+ * unified diff between baseSha and headSha.
+ *
+ * - Context lines ( ): headLine → baseLine (unchanged, possibly shifted)
+ * - Added lines (+):   headLine → null     (new in this PR, no base equivalent)
+ * - Removed lines (-): no head entry       (gone from head, base line consumed)
+ *
+ * --unified=999999 ensures every unchanged line appears as a context line so
+ * the map covers the entire head file, not just changed regions.
+ */
+export async function buildLineMapForFile({ workspacePath, baseSha, headSha, filePath }) {
+  const patch = await git([
+    '-C', workspacePath, 'diff',
+    '--unified=999999', '--no-color', '--no-ext-diff',
+    baseSha, headSha, '--', filePath,
+  ]);
+  return parseLineMap(patch, filePath);
+}
+
+function parseLineMap(patch, filePath) {
+  const map = new Map();
+  let headLine = null;
+  let baseLine = null;
+  let inFile = false;
+
+  for (const line of patch.split('\n')) {
+    if (line.startsWith('+++ ')) {
+      inFile = stripGitPathPrefix(line.slice(4).trim()) === filePath;
+      continue;
+    }
+
+    if (!inFile) continue;
+
+    if (line.startsWith('@@')) {
+      const match = line.match(/^@@ -(\d+)(?:,\d+)? \+(\d+)(?:,\d+)? @@/);
+      if (!match) continue;
+      baseLine = Number.parseInt(match[1], 10);
+      headLine = Number.parseInt(match[2], 10);
+      continue;
+    }
+
+    if (headLine === null) continue;
+
+    if (line.startsWith(' ')) {
+      map.set(headLine, baseLine);
+      headLine++;
+      baseLine++;
+    } else if (line.startsWith('+')) {
+      map.set(headLine, null);
+      headLine++;
+    } else if (line.startsWith('-')) {
+      baseLine++;
+    }
+  }
+
+  return map;
+}
+
 function parseChangedLineRanges(patch) {
   const rangesByFile = {};
   let currentFile = null;

src/suppressor.js

@@ -1,4 +1,5 @@
 import { execFile } from 'child_process';
+import { buildLineMapForFile } from './fetcher.js';
 
 const SECURITY_COMMENT_RE = /(?:\/\/|#)\s*SECURITY:\s+\S/;
 
@@ -15,11 +16,18 @@ function gitShow(workspacePath, baseSha, filePath) {
  * the base SHA — meaning the comment was reviewed and merged in a prior PR.
  * New `// SECURITY:` comments added in the current PR are invisible to this
  * function (they're not in base), so self-approval is impossible.
+ *
+ * Uses a diff-based line map to translate HEAD line numbers to base line
+ * numbers before reading the base file. This handles cases where a PR adds
+ * lines earlier in a file, shifting the finding's position relative to base.
+ * Findings on newly added lines (mapped to null in the diff) are never
+ * suppressed regardless of what comments exist nearby.
  */
-export async function suppressFindings(findings, { workspacePath, baseSha }) {
+export async function suppressFindings(findings, { workspacePath, baseSha, headSha }) {
   if (findings.length === 0) return [];
 
-  const fileCache = new Map();
+  const fileCache    = new Map();
+  const lineMapCache = new Map();
 
   async function getLines(filePath) {
     if (fileCache.has(filePath)) return fileCache.get(filePath);
@@ -34,25 +42,54 @@ export async function suppressFindings(findings, { workspacePath, baseSha }) {
     return lines;
   }
 
+  async function getLineMap(filePath) {
+    if (lineMapCache.has(filePath)) return lineMapCache.get(filePath);
+    let map = null;
+    try {
+      map = await buildLineMapForFile({ workspacePath, baseSha, headSha, filePath });
+    } catch {
+      // Diff unavailable — fall back to using head line numbers directly
+    }
+    lineMapCache.set(filePath, map);
+    return map;
+  }
+
   const kept = [];
   for (const finding of findings) {
     if (finding.tool === 'claude' && finding.locationValidated !== true) {
       kept.push(finding);
       continue;
     }
 
+    const headLineNumber = finding.suppressionLine ?? finding.startLine ?? finding.line;
+
+    const lineMap = await getLineMap(finding.file);
+    let baseLookupLine;
+
+    if (lineMap === null || !lineMap.has(headLineNumber)) {
+      // Diff unavailable or line not in map — fall back to head line number
+      baseLookupLine = headLineNumber;
+    } else {
+      const mapped = lineMap.get(headLineNumber);
+      if (mapped === null) {
+        // Newly added line in this PR — cannot have a pre-existing approval
+        kept.push(finding);
+        continue;
+      }
+      baseLookupLine = mapped;
+    }
+
     const lines = await getLines(finding.file);
     if (lines === null) {
       kept.push(finding);
       continue;
     }
 
-    const line = finding.suppressionLine ?? finding.startLine ?? finding.line;
-    const sameLine  = lines[line - 1] ?? '';
-    const lineAbove = lines[line - 2] ?? '';
+    const sameLine  = lines[baseLookupLine - 1] ?? '';
+    const lineAbove = lines[baseLookupLine - 2] ?? '';
 
     if (SECURITY_COMMENT_RE.test(sameLine) || SECURITY_COMMENT_RE.test(lineAbove)) {
-      console.log(`[suppressor] suppressed finding ${finding.file}:${line} [${finding.ruleId}] — SECURITY: comment found at base`);
+      console.log(`[suppressor] suppressed finding ${finding.file}:${headLineNumber} [${finding.ruleId}] — SECURITY: comment found at base`);
     } else {
       kept.push(finding);
     }

src/worker.js

@@ -180,7 +180,7 @@ async function runScan(job) {
     const rawFindings = await dispatch({ workspacePath, baseSha, baseRef, changedFiles, changedLineRanges, labels, owner, repo });
     const validatedFindings = await validateFindingLocations(rawFindings, { workspacePath, changedFiles, changedLineRanges });
     logFindingPlacement(validatedFindings, { owner, repo, prNumber });
-    const findings = await suppressFindings(validatedFindings, { workspacePath, baseSha: mergeBaseSha });
+    const findings = await suppressFindings(validatedFindings, { workspacePath, baseSha: mergeBaseSha, headSha });
     const actionableFindings = findings.filter(isActionableFinding);
     const discardedCount = findings.length - actionableFindings.length;
     console.log(`[worker] ${actionableFindings.length} actionable finding(s) for ${owner}/${repo} PR #${prNumber} across all tools:`);