chore: improve exception approvals to handle approvals better (#28)

Author: julio-rocketchat

CLAUDE.md

@@ -58,7 +58,7 @@ Two separate Node.js processes:
 - **`pull_request` trigger (default):** on opened/synchronize/reopened, creates a Check Run in `queued` state, enqueues a BullMQ job, returns 200
 - **`workflow_run` trigger:** on `pull_request` events, caches PR metadata in Redis (TTL 7 days) and creates a `skipped` Check Run; on `workflow_run completed` events matching the configured workflow name and conclusion, looks up cached PR metadata (falls back to GitHub API if cache is cold) then enqueues the scan
 - **`workflow_job` trigger:** same two-stage pattern as `workflow_run` but gates on a single named job completing rather than the whole workflow
-- **`issue_comment` trigger:** parses `/layne exception-approve` commands from PR comments; validates the commenter is an authorized exception approver; stores exceptions in Redis keyed to the current head SHA; re-enqueues the scan if the current check run is in `failure` state
+- **`issue_comment` trigger:** parses `/layne exception-approve` commands from PR comments; validates the commenter is an authorized exception approver; stores exceptions in Redis scoped to the PR (not the commit SHA); re-enqueues the scan if the current check run is in `failure` state
 - Job ID is deduplicated by `{repo}#{pr}@{sha}` - duplicate webhook deliveries are no-ops (Redis lock + queue check)
 - Exported `app` and `processWebhookRequest` for use in tests
 
@@ -81,9 +81,9 @@ Two separate Node.js processes:
 9. Run scanners in parallel via `src/dispatcher.js` → `dispatch()`
 10. Validate finding locations against the actual file content (`src/location-validator.js` → `validateFindingLocations`)
 11. Suppress findings that have a `// SECURITY:` comment at the merge base (`src/suppressor.js` → `suppressFindings`)
-12. Filter to actionable findings; stamp each with a deterministic `_findingId` (`LAYNE-xxxxxxxx`) via `src/exception-approvals.js` → `generateFindingId`
+12. Filter to actionable findings; stamp each with a deterministic `_findingId` (`LAYNE-xxxxxxxxxxxxxxxx`) via `src/exception-approvals.js` → `generateFindingId`
 13. Convert findings to annotations via `src/reporter.js` → `buildAnnotations()`
-14. If `exceptionApprovers` is configured: load stored exceptions from Redis and call `buildExceptionSummary` to potentially override conclusion to `success`
+14. If `exceptionApprovers` is configured: load stored exceptions from Redis (`loadExceptions`), remove stale ones whose flagged line changed (`filterStaleExceptions`), resolve approvals that survived a line-number shift via rebase (`resolveDriftedExceptions`), then call `buildExceptionSummary` to potentially override conclusion to `success`
 15. Complete Check Run
 16. Post PR comment if `comment.enabled` via `src/commenter.js` → `postComment`
 17. Apply/remove PR labels via `src/github.js` → `ensureLabelsExist` + `setLabels`

src/__tests__/exception-approvals.test.js

@@ -17,7 +17,7 @@ vi.mock('child_process', () => ({
   execFile: mockExecFile,
 }));
 
-const { createWorkspace, setupRepo, getChangedFiles, getChangedLineRanges, checkoutFiles, cleanupWorkspace } = await import('../fetcher.js');
+const { createWorkspace, setupRepo, getChangedFiles, getChangedLineRanges, checkoutFiles, cleanupWorkspace, fetchCommit } = await import('../fetcher.js');
 
 // ---------------------------------------------------------------------------
 // Helpers
@@ -179,6 +179,44 @@ describe('getChangedFiles()', () => {
 
 // ---------------------------------------------------------------------------
 
+describe('fetchCommit()', () => {
+  beforeEach(() => vi.clearAllMocks());
+
+  it('fetches the given SHA with --depth 1 and --filter=blob:none', async () => {
+    await fetchCommit({ workspacePath: '/tmp/ws', sha: 'deadbeef' });
+
+    const [cmd, args] = mockExecFile.mock.calls[0];
+    expect(cmd).toBe('git');
+    expect(args).toContain('fetch');
+    expect(args).toContain('--depth');
+    expect(args).toContain('1');
+    expect(args).toContain('--filter=blob:none');
+    expect(args).toContain('origin');
+    expect(args).toContain('deadbeef');
+  });
+
+  it('uses Git protocol v2', async () => {
+    await fetchCommit({ workspacePath: '/tmp/ws', sha: 'deadbeef' });
+    expect(mockExecFile.mock.calls[0][1]).toContain('protocol.version=2');
+  });
+
+  it('runs inside the given workspace path', async () => {
+    await fetchCommit({ workspacePath: '/tmp/my-workspace', sha: 'abc' });
+    const args = mockExecFile.mock.calls[0][1];
+    expect(args).toContain('-C');
+    expect(args).toContain('/tmp/my-workspace');
+  });
+
+  it('throws when git exits with a non-zero code', async () => {
+    mockExecFile.mockImplementationOnce((cmd, args, cb) =>
+      cb(new Error('unknown revision'), '', '')
+    );
+    await expect(fetchCommit({ workspacePath: '/tmp/ws', sha: 'bad' })).rejects.toThrow('unknown revision');
+  });
+});
+
+// ---------------------------------------------------------------------------
+
 describe('getChangedLineRanges()', () => {
   beforeEach(() => vi.clearAllMocks());
 
@@ -206,6 +244,24 @@ describe('getChangedLineRanges()', () => {
     });
   });
 
+  it('scopes the diff to specific files when files array is provided', async () => {
+    mockExecFile.mockImplementationOnce((cmd, args, cb) => cb(null, '', ''));
+    await getChangedLineRanges({ workspacePath: '/tmp/ws', baseSha: 'b', headSha: 'h', files: ['src/a.js', 'src/b.js'] });
+
+    const args = mockExecFile.mock.calls[0][1];
+    expect(args).toContain('--');
+    expect(args).toContain('src/a.js');
+    expect(args).toContain('src/b.js');
+  });
+
+  it('does not append -- when files array is empty', async () => {
+    mockExecFile.mockImplementationOnce((cmd, args, cb) => cb(null, '', ''));
+    await getChangedLineRanges({ workspacePath: '/tmp/ws', baseSha: 'b', headSha: 'h' });
+
+    const args = mockExecFile.mock.calls[0][1];
+    expect(args).not.toContain('--');
+  });
+
   it('ignores deleted hunks that have no lines in the head revision', async () => {
     mockExecFile.mockImplementationOnce((cmd, args, cb) => cb(null, [
       'diff --git a/src/app.js b/src/app.js',

src/__tests__/fetcher.test.js

@@ -976,13 +976,13 @@ describe('issue_comment handler', () => {
     ));
 
     expect(storeExceptions).toHaveBeenCalledWith(expect.objectContaining({
-      owner:      'org',
-      repo:       'my-repo',
-      prNumber:   42,
-      headSha:    'abc123',
-      findingIds: ['LAYNE-a3f29c81'],
-      approver:   'alice',
-      reason:     'test cred',
+      owner:           'org',
+      repo:            'my-repo',
+      prNumber:        42,
+      approvedHeadSha: 'abc123',
+      findingIds:      ['LAYNE-a3f29c81'],
+      approver:        'alice',
+      reason:          'test cred',
     }));
   });
 

src/__tests__/server.test.js

@@ -112,9 +112,11 @@ vi.mock('../location-validator.js', () => ({
 }));
 
 vi.mock('../exception-approvals.js', () => ({
-  generateFindingId:    vi.fn().mockReturnValue('LAYNE-a3f29c81'),
-  loadExceptions:       vi.fn().mockResolvedValue(new Map()),
-  buildExceptionSummary: vi.fn(({ baseSummary }) => ({ conclusion: 'failure', summary: baseSummary })),
+  generateFindingId:        vi.fn().mockReturnValue('LAYNE-a3f29c81'),
+  loadExceptions:           vi.fn().mockResolvedValue(new Map()),
+  filterStaleExceptions:    vi.fn(async ({ exceptions }) => exceptions),
+  resolveDriftedExceptions: vi.fn(async () => new Map()),
+  buildExceptionSummary:    vi.fn(({ baseSummary }) => ({ conclusion: 'failure', summary: baseSummary })),
 }));
 
 const { Worker: MockWorker }              = await import('bullmq');
@@ -130,7 +132,7 @@ const { loadScanConfig }                  = await import('../config.js');
 const { notify }                          = await import('../notifiers/index.js');
 const { postComment }                     = await import('../commenter.js');
 const { redis }                           = await import('../queue.js');
-const { generateFindingId, loadExceptions, buildExceptionSummary } = await import('../exception-approvals.js');
+const { generateFindingId, loadExceptions, filterStaleExceptions, resolveDriftedExceptions, buildExceptionSummary } = await import('../exception-approvals.js');
 const { createScanContext, filterFindingsToChangedLines } = await import('../scan-context.js');
 const { processJob, shutdown }            = await import('../worker.js');
 
@@ -933,11 +935,50 @@ describe('processJob()', () => {
         owner:      'org',
         repo:       'repo',
         prNumber:   7,
-        headSha:    'abc123',
         findingIds: ['LAYNE-a3f29c81'],
       }));
     });
 
+    it('calls filterStaleExceptions with loaded exceptions after loadExceptions', async () => {
+      const loaded = new Map([['LAYNE-a3f29c81', { approver: 'alice', reason: 'ok', timestamp: '', approvedHeadSha: 'abc123' }]]);
+      loadExceptions.mockResolvedValueOnce(loaded);
+      buildExceptionSummary.mockReturnValueOnce({ conclusion: 'success', summary: 'Excepted.' });
+
+      await processJob(baseJob);
+
+      expect(filterStaleExceptions).toHaveBeenCalledWith(expect.objectContaining({
+        exceptions:     loaded,
+        currentHeadSha: 'abc123',
+      }));
+    });
+
+    it('calls resolveDriftedExceptions with unmatched blocking findings after filterStaleExceptions', async () => {
+      // filterStaleExceptions returns empty — no direct exception match for the finding.
+      filterStaleExceptions.mockResolvedValueOnce(new Map());
+
+      await processJob(baseJob);
+
+      expect(resolveDriftedExceptions).toHaveBeenCalledWith(expect.objectContaining({
+        unmatchedFindings: expect.arrayContaining([
+          expect.objectContaining({ _findingId: 'LAYNE-a3f29c81' }),
+        ]),
+        currentHeadSha: 'abc123',
+      }));
+    });
+
+    it('merges drifted exceptions into the exceptions map passed to buildExceptionSummary', async () => {
+      filterStaleExceptions.mockResolvedValueOnce(new Map());
+      const drifted = new Map([['LAYNE-a3f29c81', { approver: 'bob', reason: 'drift', timestamp: '' }]]);
+      resolveDriftedExceptions.mockResolvedValueOnce(drifted);
+      buildExceptionSummary.mockReturnValueOnce({ conclusion: 'success', summary: 'Drift pass.' });
+
+      await processJob(baseJob);
+
+      expect(buildExceptionSummary).toHaveBeenCalledWith(expect.objectContaining({
+        exceptions: expect.objectContaining({ size: 1 }),
+      }));
+    });
+
     it('does not call loadExceptions when there are no blocking findings', async () => {
       const lowFinding = { ...finding, severity: 'low' };
       dispatch.mockResolvedValueOnce([lowFinding]);

src/__tests__/worker.test.js

@@ -1,12 +1,13 @@
 import crypto from 'crypto';
 import { getTeamMembers } from './github.js';
 import { redis } from './queue.js';
+import { fetchCommit, getChangedLineRanges, buildLineMapForFile } from './fetcher.js';
 
 const EXCEPTION_TTL = 30 * 24 * 60 * 60; // 30 days in seconds
 
 export function generateFindingId(finding) {
   const input = `${finding.tool}:${finding.file}:${finding.line ?? finding.startLine}`;
-  return 'LAYNE-' + crypto.createHash('sha256').update(input).digest('hex').slice(0, 8);
+  return 'LAYNE-' + crypto.createHash('sha256').update(input).digest('hex').slice(0, 16);
 }
 
 // Returns { ids, reason } | { ids, reason: null, error } | null
@@ -21,10 +22,10 @@ export function parseExceptionCommand(body) {
   const afterCommand = commandLine.slice(commandIndex + '/layne exception-approve'.length).trim();
 
   const tokens = afterCommand.split(/\s+/).filter(Boolean);
-  const ids = tokens.filter(t => /^LAYNE-[0-9a-f]{8}$/.test(t));
+  const ids = tokens.filter(t => /^LAYNE-[0-9a-f]{16}$/.test(t));
 
   if (ids.length === 0) {
-    return { ids: [], reason: null, error: 'No valid finding IDs found. IDs must match LAYNE-xxxxxxxx format.' };
+    return { ids: [], reason: null, error: 'No valid finding IDs found. IDs must match LAYNE-xxxxxxxxxxxxxxxx format.' };
   }
 
   const reasonIndex = tokens.findIndex(t => t === 'reason:');
@@ -40,19 +41,23 @@ export function parseExceptionCommand(body) {
   return { ids, reason };
 }
 
-export async function storeExceptions({ owner, repo, prNumber, headSha, findingIds, approver, reason }) {
-  const value = JSON.stringify({ approver, reason, timestamp: new Date().toISOString() });
+export async function storeExceptions({ owner, repo, prNumber, approvedHeadSha, findingIds, approver, reason }) {
+  const value = JSON.stringify({ approver, reason, timestamp: new Date().toISOString(), approvedHeadSha });
+  const setKey = `layne:exception-ids:${owner}/${repo}#${prNumber}`;
 
-  await Promise.all(findingIds.map(findingId => {
-    const key = `layne:exception:${owner}/${repo}#${prNumber}@${headSha}:${findingId}`;
-    return redis.set(key, value, 'EX', EXCEPTION_TTL);
-  }));
+  await Promise.all([
+    ...findingIds.map(findingId => {
+      const key = `layne:exception:${owner}/${repo}#${prNumber}:${findingId}`;
+      return redis.set(key, value, 'EX', EXCEPTION_TTL);
+    }),
+    redis.sadd(setKey, ...findingIds).then(() => redis.expire(setKey, EXCEPTION_TTL)),
+  ]);
 }
 
-// Returns Map<findingId, { approver, reason, timestamp }>
-export async function loadExceptions({ owner, repo, prNumber, headSha, findingIds }) {
+// Returns Map<findingId, { approver, reason, timestamp, approvedHeadSha }>
+export async function loadExceptions({ owner, repo, prNumber, findingIds }) {
   const results = await Promise.all(findingIds.map(async findingId => {
-    const key = `layne:exception:${owner}/${repo}#${prNumber}@${headSha}:${findingId}`;
+    const key = `layne:exception:${owner}/${repo}#${prNumber}:${findingId}`;
     const val = await redis.get(key);
     return [findingId, val ? JSON.parse(val) : null];
   }));
@@ -64,6 +69,132 @@ export async function loadExceptions({ owner, repo, prNumber, headSha, findingId
   return map;
 }
 
+// Removes exceptions whose flagged line was changed between the approval commit and the
+// current head. Groups by approvedHeadSha to minimise git fetches - one fetch per unique
+// approval SHA regardless of how many findings were approved in that commit.
+// On fetch or diff failure the entire group is invalidated (conservative fallback).
+export async function filterStaleExceptions({ exceptions, findings, workspacePath, currentHeadSha }) {
+  if (exceptions.size === 0) return exceptions;
+
+  const findingById = new Map(findings.map(f => [f._findingId, f]));
+
+  // Group exception findingIds by the SHA at which they were approved.
+  const bySha = new Map();
+  for (const [findingId, data] of exceptions) {
+    const { approvedHeadSha } = data;
+    if (approvedHeadSha === currentHeadSha) continue;
+    if (!bySha.has(approvedHeadSha)) bySha.set(approvedHeadSha, []);
+    bySha.get(approvedHeadSha).push(findingId);
+  }
+
+  if (bySha.size === 0) return exceptions;
+
+  const filtered = new Map(exceptions);
+
+  for (const [approvedHeadSha, findingIds] of bySha) {
+    const files = [...new Set(findingIds.map(id => findingById.get(id)?.file).filter(Boolean))];
+
+    let changedRanges;
+    try {
+      await fetchCommit({ workspacePath, sha: approvedHeadSha });
+      changedRanges = await getChangedLineRanges({
+        workspacePath, baseSha: approvedHeadSha, headSha: currentHeadSha, files,
+      });
+    } catch (err) {
+      console.warn(`[exception-approvals] Could not check staleness for ${approvedHeadSha}: ${err.message} — invalidating as a precaution`);
+      for (const id of findingIds) filtered.delete(id);
+      continue;
+    }
+
+    for (const findingId of findingIds) {
+      const finding = findingById.get(findingId);
+      if (!finding) continue;
+
+      const line   = finding.line ?? finding.startLine;
+      const ranges = changedRanges[finding.file] ?? [];
+      if (ranges.some(r => line >= r.start && line <= r.end)) {
+        console.log(`[exception-approvals] Invalidating exception ${findingId}: ${finding.file}:${line} changed since approval`);
+        filtered.delete(findingId);
+      }
+    }
+  }
+
+  return filtered;
+}
+
+// For blocking findings that have no matching stored exception, checks whether the finding
+// is a line-shifted version of a previously approved one (e.g. a rebase added lines above
+// the flagged code). Uses the PR-scoped exception ID set to load all stored exceptions,
+// then builds a per-file line map (headLine → baseLine) between the approval SHA and the
+// current head. If the current finding's line maps back to a line that was approved, the
+// exception is carried forward (only possible for unchanged context lines — modified lines
+// map to null in the line map and are therefore never matched).
+export async function resolveDriftedExceptions({
+  unmatchedFindings,
+  owner, repo, prNumber,
+  workspacePath,
+  currentHeadSha,
+}) {
+  if (unmatchedFindings.length === 0) return new Map();
+
+  const setKey = `layne:exception-ids:${owner}/${repo}#${prNumber}`;
+  const allIds = await redis.smembers(setKey);
+  if (allIds.length === 0) return new Map();
+
+  const allExceptions = await loadExceptions({ owner, repo, prNumber, findingIds: allIds });
+  if (allExceptions.size === 0) return new Map();
+
+  // Group stored exceptions by approvedHeadSha, skipping the current SHA (no drift possible).
+  const bySha = new Map();
+  for (const [findingId, data] of allExceptions) {
+    if (data.approvedHeadSha === currentHeadSha) continue;
+    if (!bySha.has(data.approvedHeadSha)) bySha.set(data.approvedHeadSha, new Map());
+    bySha.get(data.approvedHeadSha).set(findingId, data);
+  }
+
+  if (bySha.size === 0) return new Map();
+
+  const resolved = new Map();
+  const affectedFiles = [...new Set(unmatchedFindings.map(f => f.file))];
+
+  for (const [approvedHeadSha, exceptionsAtSha] of bySha) {
+    try {
+      await fetchCommit({ workspacePath, sha: approvedHeadSha });
+    } catch (err) {
+      console.warn(`[exception-approvals] Could not fetch ${approvedHeadSha} for drift check: ${err.message} — skipping`);
+      continue;
+    }
+
+    for (const file of affectedFiles) {
+      const findingsInFile = unmatchedFindings.filter(f => f.file === file && !resolved.has(f._findingId));
+      if (findingsInFile.length === 0) continue;
+
+      let lineMap;
+      try {
+        lineMap = await buildLineMapForFile({ workspacePath, baseSha: approvedHeadSha, headSha: currentHeadSha, filePath: file });
+      } catch (err) {
+        console.warn(`[exception-approvals] Could not build line map for ${file}@${approvedHeadSha}: ${err.message} — skipping`);
+        continue;
+      }
+
+      for (const finding of findingsInFile) {
+        const currentLine = finding.line ?? finding.startLine;
+        const originalLine = lineMap.get(currentLine);
+        if (originalLine === null || originalLine === undefined) continue;
+
+        const oldFindingId = generateFindingId({ tool: finding.tool, file: finding.file, line: originalLine });
+        const exception = exceptionsAtSha.get(oldFindingId);
+        if (exception) {
+          console.log(`[exception-approvals] Drift resolved: ${finding._findingId} (line ${currentLine}) ← ${oldFindingId} (line ${originalLine}) approved by @${exception.approver} at ${approvedHeadSha}`);
+          resolved.set(finding._findingId, exception);
+        }
+      }
+    }
+  }
+
+  return resolved;
+}
+
 export function buildExceptionSummary({ findings, exceptions, baseSummary }) {
   const blockingFindings = findings.filter(f =>
     f._findingId && (f.severity === 'critical' || f.severity === 'high')