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
103 changes: 99 additions & 4 deletions src/auto-filter-diff/__tests__/auto-filter-diff.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -103,14 +103,30 @@ describe('autoFilterDiff — phase 1: auto-exclude score-0 files', () => {
expect(result.remainingFiles).toBe(1);
});

it('all files excluded returns empty string', () => {
it('when ALL files are score-0, keeps every file instead of excluding them all (allFilesKept=true)', () => {
// The key fix: a PR where every changed file is low-risk should still
// be reviewed. Phase 1 detects the all-excluded condition and falls back
// to keeping all sections; autoExcludedFiles is empty, allFilesKept=true.
const diff = makeDiff('src/a.go') + makeDiff('src/b.go');
const riskScores = { 'src/a.go': 0, 'src/b.go': 0 };
const result = autoFilterDiff(diff, riskScores, 0);

expect(result.filtered).toBe('');
expect(result.remainingFiles).toBe(0);
expect(result.autoExcludedFiles).toHaveLength(2);
expect(result.allFilesKept).toBe(true);
expect(result.autoExcludedFiles).toHaveLength(0);
expect(result.remainingFiles).toBe(2);
expect(result.filtered).toContain('src/a.go');
expect(result.filtered).toContain('src/b.go');
});

it('when SOME files are score-0, normal exclusion applies (allFilesKept=false)', () => {
// Mixed PR: low-risk file is excluded, high-risk file is kept.
const diff = makeDiff('src/gen.pb.go') + makeDiff('src/auth/handler.go');
const riskScores = { 'src/gen.pb.go': 0, 'src/auth/handler.go': 2 };
const result = autoFilterDiff(diff, riskScores, 0);

expect(result.allFilesKept).toBe(false);
expect(result.autoExcludedFiles).toEqual(['src/gen.pb.go']);
expect(result.remainingFiles).toBe(1);
});

it('reports correct originalLines', () => {
Expand All @@ -124,6 +140,84 @@ describe('autoFilterDiff — phase 1: auto-exclude score-0 files', () => {
});
});

// ═════════════════════════════════════════════════════════════════════════════
// Phase 1 — never-exclude-all guarantee
// ═════════════════════════════════════════════════════════════════════════════

describe('autoFilterDiff — never exclude all files (all-score-0 fallback)', () => {
it('single score-0 file: kept (would otherwise exclude everything)', () => {
const diff = makeDiff('yarn.lock');
const riskScores = { 'yarn.lock': 0 };
const result = autoFilterDiff(diff, riskScores, 0);

expect(result.allFilesKept).toBe(true);
expect(result.autoExcludedFiles).toHaveLength(0);
expect(result.remainingFiles).toBe(1);
expect(result.filtered).toContain('yarn.lock');
});

it('many score-0 files (e.g. lock file PR): all kept', () => {
const diff =
makeDiff('yarn.lock') +
makeDiff('package-lock.json') +
makeDiff('Cargo.lock') +
makeDiff('go.sum');
const riskScores = { 'yarn.lock': 0, 'package-lock.json': 0, 'Cargo.lock': 0, 'go.sum': 0 };
const result = autoFilterDiff(diff, riskScores, 0);

expect(result.allFilesKept).toBe(true);
expect(result.autoExcludedFiles).toHaveLength(0);
expect(result.remainingFiles).toBe(4);
expect(result.filtered).toContain('yarn.lock');
expect(result.filtered).toContain('package-lock.json');
});

it('all score-0 but Phase 2 cap still trims (timeout protection preserved)', () => {
// Four score-0 files whose total exceeds the cap: Phase 1 falls back to
// keeping all, then Phase 2 removes the lowest-score files until the cap fits.
const diff =
makeDiff('src/a.go', 80) + // score 0 → all-kept fallback
makeDiff('src/b.go', 80) + // score 0
makeDiff('src/c.go', 80) + // score 0
makeDiff('src/d.go', 80); // score 0
const riskScores = { 'src/a.go': 0, 'src/b.go': 0, 'src/c.go': 0, 'src/d.go': 0 };
// Combined ~344 lines; cap at 100 → Phase 2 removes some
const result = autoFilterDiff(diff, riskScores, 100);

expect(result.allFilesKept).toBe(true); // Phase 1 fell back
expect(result.autoExcludedFiles).toHaveLength(0);
expect(result.remainingFiles).toBeGreaterThanOrEqual(1); // Phase 2 trimmed but kept ≥1
expect(result.remainingLines).toBeLessThanOrEqual(100 + 90); // rough check
expect(result.progressivelyExcludedFiles.length).toBeGreaterThan(0);
});

it('allFilesKept is false for empty diff', () => {
const result = autoFilterDiff('', {}, 0);
expect(result.allFilesKept).toBe(false);
});

it('allFilesKept is false when no files are score-0', () => {
const diff = makeDiff('src/app.ts') + makeDiff('src/server.ts');
const riskScores = { 'src/app.ts': 1, 'src/server.ts': 2 };
const result = autoFilterDiff(diff, riskScores, 0);

expect(result.allFilesKept).toBe(false);
expect(result.autoExcludedFiles).toHaveLength(0);
expect(result.remainingFiles).toBe(2);
});

it('filtered output is identical to original diff when all-kept fallback fires', () => {
// Round-trip: the fallback must return the original diff unchanged
// (Phase 2 disabled via cap=0).
const diff = makeDiff('styles/main.css') + makeDiff('.gitignore');
const riskScores = { 'styles/main.css': 0, '.gitignore': 0 };
const result = autoFilterDiff(diff, riskScores, 0);

expect(result.allFilesKept).toBe(true);
expect(result.filtered).toBe(diff);
});
});

// ═════════════════════════════════════════════════════════════════════════════
// Phase 2 — progressive cap
// ═════════════════════════════════════════════════════════════════════════════
Expand Down Expand Up @@ -248,6 +342,7 @@ describe('autoFilterDiff — edge cases', () => {
expect(result.remainingFiles).toBe(0);
expect(result.remainingLines).toBe(0);
expect(result.originalLines).toBe(0);
expect(result.allFilesKept).toBe(false);
});

it('diff with no matching risk scores is passed through unchanged', () => {
Expand Down
33 changes: 30 additions & 3 deletions src/auto-filter-diff/auto-filter-diff.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,9 @@
* Phase 1 — Auto-exclude score-0 files:
* Remove every file section whose path appears in `riskScores` with a value
* of 0. Files NOT present in `riskScores` are kept (unknown = needs review).
* Exception: if Phase 1 would remove ALL files, keep all instead — a PR
* where every changed file is low-risk still deserves a review.
* Phase 2 still applies in that case, so timeout protection is preserved.
*
* Phase 2 — Progressive cap (only when maxDiffLines > 0):
* If the remaining diff exceeds `maxDiffLines` after Phase 1, sort the kept
Expand All @@ -35,6 +38,11 @@ export interface AutoFilterResult {
remainingLines: number;
/** Line count of the original diff (split by '\n'). */
originalLines: number;
/**
* True when Phase 1 would have excluded everything and was skipped.
* The review will cover all files; Phase 2 (progressive cap) still applies.
*/
allFilesKept: boolean;
}

// ---------------------------------------------------------------------------
Expand Down Expand Up @@ -132,22 +140,40 @@ export function autoFilterDiff(
remainingFiles: 0,
remainingLines: 0,
originalLines: 0,
allFilesKept: false,
};
}

const originalLines = diffContent.split('\n').length;
const { preamble, sections } = parseSections(diffContent);

// ── Phase 1: auto-exclude score-0 files ────────────────────────────────────
const autoExcludedFiles: string[] = [];
let keptSections = sections.filter((section) => {
const candidateExcluded: string[] = [];
const afterPhase1 = sections.filter((section) => {
if (section.path in riskScores && riskScores[section.path] === 0) {
autoExcludedFiles.push(section.path);
candidateExcluded.push(section.path);
return false;
}
return true;
});

// If Phase 1 would remove every file, keep all files instead.
// A PR where every changed file is low-risk still deserves a review — the
// reviewer should see it rather than silently skipping the whole PR.
// Phase 2 (progressive cap) still applies to bound review size.
let allFilesKept = false;
let autoExcludedFiles: string[];
let keptSections: typeof sections;

if (afterPhase1.length === 0 && sections.length > 0) {
allFilesKept = true;
autoExcludedFiles = []; // nothing actually excluded
keptSections = sections; // keep everything
} else {
autoExcludedFiles = candidateExcluded;
keptSections = afterPhase1;
}

// ── Phase 2: progressive cap ───────────────────────────────────────────────
const progressivelyExcludedFiles: string[] = [];

Expand Down Expand Up @@ -188,5 +214,6 @@ export function autoFilterDiff(
remainingFiles: keptSections.length,
remainingLines,
originalLines,
allFilesKept,
};
}
6 changes: 6 additions & 0 deletions src/auto-filter-diff/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,12 @@ try {
process.stderr.write(`⏭️ Auto-excluded (score 0): ${path}\n`);
}

if (result.allFilesKept) {
process.stderr.write(
'ℹ️ All files scored 0 — keeping all files for review (Phase 2 cap still applies)\n',
);
}

for (const path of result.progressivelyExcludedFiles) {
const score = riskScores[path] ?? 'unknown';
process.stderr.write(`⏭️ Progressive cap (score ${score}): ${path}\n`);
Expand Down
Loading
Loading