Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
152 changes: 99 additions & 53 deletions src/score-risk/__tests__/score-risk.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,10 @@
* Each scoring rule is tested in isolation first (single-variable control),
* then in combination. Edge cases and both zero-score exit paths (exclude-paths
* prefix match and generated-file marker) are verified independently.
*
* Baseline score is 1 for any file that survives rules 1, 2, and 6 — ensuring
* plain application files are never silently auto-excluded alongside
* intentionally-low-risk files (tests, docs, generated code, JSON configs).
*/
import { describe, expect, it } from 'vitest';
import { parseExcludePrefixes, scoreFiles } from '../score-risk.js';
Expand Down Expand Up @@ -80,7 +84,7 @@ describe('scoreFiles — rule 1: exclude-paths prefix → score 0', () => {
});

it('security-path file in excluded prefix still scores 0 (prefix wins)', () => {
// "session_service.pb.go" would score 2 via the security-path rule,
// "session_service.pb.go" would score via the security-path rule,
// but the exclude-paths rule fires first and short-circuits to 0.
const diff = makeLargeDiff('backend/gen/session_service.pb.go', 200);
const scores = scoreFiles(diff, ['backend/gen/']);
Expand All @@ -91,7 +95,7 @@ describe('scoreFiles — rule 1: exclude-paths prefix → score 0', () => {
const diff = makeDiff('backend/gen/foo.pb.go') + makeDiff('src/auth/handler.go');
const scores = scoreFiles(diff, ['backend/gen/']);
expect(scores['backend/gen/foo.pb.go']).toBe(0);
expect(scores['src/auth/handler.go']).toBe(2); // security path
expect(scores['src/auth/handler.go']).toBe(3); // baseline 1 + security path +2
});

it('multiple prefixes — file matching any is scored 0', () => {
Expand All @@ -102,7 +106,7 @@ describe('scoreFiles — rule 1: exclude-paths prefix → score 0', () => {
const scores = scoreFiles(diff, ['backend/gen/', 'frontend/src/gen/']);
expect(scores['backend/gen/foo.pb.go']).toBe(0);
expect(scores['frontend/src/gen/api.ts']).toBe(0);
expect(scores['src/real.go']).toBe(0); // no rules fire for a plain file
expect(scores['src/real.go']).toBe(1); // baseline 1: plain file, no signals
});
});

Expand Down Expand Up @@ -152,8 +156,8 @@ describe('scoreFiles — rule 2: generated-file marker → score 0', () => {
'+// Code generated (too late to trigger marker check)',
]);
const scores = scoreFiles(diff, []);
// Security path +2; no other rules fired.
expect(scores['src/auth/handler.go']).toBe(2);
// Baseline 1 + security path +2 = 3; marker too late to fire.
expect(scores['src/auth/handler.go']).toBe(3);
});

it('marker on context line (not a "+" line) does NOT trigger → scored normally', () => {
Expand All @@ -163,8 +167,8 @@ describe('scoreFiles — rule 2: generated-file marker → score 0', () => {
'+real change',
]);
const scores = scoreFiles(diff, []);
// Security path matches "auth" → +2; no marker on added line.
expect(scores['src/auth/gen.go']).toBe(2);
// Baseline 1 + security path matches "auth" → +2 = 3; no marker on added line.
expect(scores['src/auth/gen.go']).toBe(3);
});
});

Expand All @@ -183,75 +187,74 @@ describe('scoreFiles — rule 3: security-sensitive path → +2', () => {
] as const;

for (const kw of KEYWORDS) {
it(`keyword "${kw}" in path → score 2`, () => {
it(`keyword "${kw}" in path → score 3 (baseline 1 + security +2)`, () => {
const diff = makeDiff(`src/${kw}/handler.go`);
const scores = scoreFiles(diff, []);
expect(scores[`src/${kw}/handler.go`]).toBe(2);
expect(scores[`src/${kw}/handler.go`]).toBe(3);
});
}

it('keyword is case-insensitive (AUTH → 2)', () => {
it('keyword is case-insensitive (AUTH → 3)', () => {
const diff = makeDiff('src/AUTH/handler.go');
const scores = scoreFiles(diff, []);
expect(scores['src/AUTH/handler.go']).toBe(2);
expect(scores['src/AUTH/handler.go']).toBe(3);
});

it('no keyword in path → 0', () => {
it('no keyword in path → baseline score 1', () => {
const diff = makeDiff('src/utils/helper.go');
const scores = scoreFiles(diff, []);
expect(scores['src/utils/helper.go']).toBe(0);
expect(scores['src/utils/helper.go']).toBe(1);
});
});

// ── Rule 4: large change (>100 added lines) → +2 ─────────────────────────────

describe('scoreFiles — rule 4: large change (>100 added lines) → +2', () => {
it('101 added lines → score 2', () => {
it('101 added lines → score 3 (baseline 1 + large +2)', () => {
const diff = makeLargeDiff('src/big.go', 101);
const scores = scoreFiles(diff, []);
expect(scores['src/big.go']).toBe(2);
expect(scores['src/big.go']).toBe(3);
});

it('100 added lines (boundary) → score 0 (not triggered)', () => {
it('100 added lines (boundary) → score 1 (baseline only; rule not triggered)', () => {
const diff = makeLargeDiff('src/big.go', 100);
const scores = scoreFiles(diff, []);
expect(scores['src/big.go']).toBe(0);
expect(scores['src/big.go']).toBe(1);
});

it('1 added line → score 0', () => {
it('1 added line → score 1 (baseline only)', () => {
const diff = makeDiff('src/small.go', ['+one line']);
const scores = scoreFiles(diff, []);
expect(scores['src/small.go']).toBe(0);
expect(scores['src/small.go']).toBe(1);
});
});

// ── Rule 5: many hunks (>3) → +1 ─────────────────────────────────────────────

describe('scoreFiles — rule 5: many hunks (>3 hunk headers) → +1', () => {
it('4 hunks → score 1', () => {
it('4 hunks → score 2 (baseline 1 + hunks +1)', () => {
const diff = makeHunksDiff('src/hunky.go', 4);
const scores = scoreFiles(diff, []);
expect(scores['src/hunky.go']).toBe(1);
expect(scores['src/hunky.go']).toBe(2);
});

it('3 hunks (boundary) → score 0 (not triggered)', () => {
it('3 hunks (boundary) → score 1 (baseline only; rule not triggered)', () => {
const diff = makeHunksDiff('src/hunky.go', 3);
const scores = scoreFiles(diff, []);
expect(scores['src/hunky.go']).toBe(0);
expect(scores['src/hunky.go']).toBe(1);
});

it('1 hunk → score 0', () => {
it('1 hunk → score 1 (baseline only)', () => {
const diff = makeDiff('src/simple.go', ['+change']);
const scores = scoreFiles(diff, []);
expect(scores['src/simple.go']).toBe(0);
expect(scores['src/simple.go']).toBe(1);
});
});

// ── Rule 6: test/doc/config file → reset score to 0 ──────────────────────────

describe('scoreFiles — rule 6: Rust/Ruby test/bench/spec suffixes → reset score to 0', () => {
it('Rust file in tests/ directory scores 0 (directory component match)', () => {
// Security keyword "virtiofs" not in path, but tests/ directory resets to 0
const diff = makeDiff('crates/vm/tests/integration/virtiofs.rs');
const scores = scoreFiles(diff, []);
expect(scores['crates/vm/tests/integration/virtiofs.rs']).toBe(0);
Expand All @@ -273,7 +276,7 @@ describe('scoreFiles — rule 6: Rust/Ruby test/bench/spec suffixes → reset sc
// "auth" in path would normally add +2 (rule 3), but tests/ directory resets to 0
const diff = makeDiff('src/auth/tests/handler_test.go');
const scores = scoreFiles(diff, []);
// Rule 7 (error handling) can still apply; rule 3 (+2) is reset by rule 6.
// Rule 7 (error handling) can still apply; rule 3 (+2) + baseline (1) are reset by rule 6.
// No error-handling keywords in the default diff content, so score stays 0.
expect(scores['src/auth/tests/handler_test.go']).toBe(0);
});
Expand Down Expand Up @@ -344,7 +347,7 @@ describe('scoreFiles — rule 6: test/doc/config file → reset score to 0', ()
'',
].join('\n');
const scores = scoreFiles(diff, []);
// Rule 6 resets to 0; rule 7 (error handling) doesn't fire here.
// Rule 6 resets to 0 (overrides baseline + rules 3-5); rule 7 (error handling) doesn't fire here.
expect(scores[p]).toBe(0);
});
}
Expand All @@ -363,42 +366,42 @@ describe('scoreFiles — rule 7: error-handling patterns → +1', () => {
const KEYWORDS = ['catch', 'rescue', 'except', 'recover', 'error', 'panic'] as const;

for (const kw of KEYWORDS) {
it(`keyword "${kw}" in an added line → +1`, () => {
it(`keyword "${kw}" in an added line → baseline 1 + error +1 = 2`, () => {
const diff = makeDiff('src/service.go', [`+handle ${kw} here`]);
const scores = scoreFiles(diff, []);
expect(scores['src/service.go']).toBe(1);
expect(scores['src/service.go']).toBe(2);
});
}

it('keyword in a context line (not added) → no +1', () => {
it('keyword in a context line (not added) → no +1 (baseline 1 only)', () => {
// Context line (space-prefixed) doesn't count.
const diff = makeDiff('src/service.go', [' existing catch block', '+new line only']);
const scores = scoreFiles(diff, []);
expect(scores['src/service.go']).toBe(0);
expect(scores['src/service.go']).toBe(1);
});

it('multiple error-handling lines still only add +1 total', () => {
it('multiple error-handling lines still only add +1 total (baseline 1 + error +1 = 2)', () => {
const diff = makeDiff('src/service.go', [
'+catch(err) {',
'+ recover();',
'+ panic(err)',
'}',
]);
const scores = scoreFiles(diff, []);
expect(scores['src/service.go']).toBe(1);
expect(scores['src/service.go']).toBe(2);
});
});

// ── Combined scoring ──────────────────────────────────────────────────────────

describe('scoreFiles — combined rules', () => {
it('security path + large change = 4', () => {
it('security path + large change = 5 (baseline 1 + security +2 + large +2)', () => {
const diff = makeLargeDiff('src/auth/handler.go', 150);
const scores = scoreFiles(diff, []);
expect(scores['src/auth/handler.go']).toBe(4); // +2 security + +2 large
expect(scores['src/auth/handler.go']).toBe(5);
});

it('security path + large change + many hunks = 5', () => {
it('security path + large change + many hunks = 6 (baseline 1 + security +2 + large +2 + hunks +1)', () => {
const lines = Array.from({ length: 110 }, (_, i) => `+line${i}`);
const hunkHeaders = Array.from(
{ length: 4 },
Expand All @@ -414,10 +417,10 @@ describe('scoreFiles — combined rules', () => {
'',
].join('\n');
const scores = scoreFiles(diff, []);
expect(scores['src/session/service.go']).toBe(5); // +2 security +2 large +1 hunks
expect(scores['src/session/service.go']).toBe(6);
});

it('security path + large change + many hunks + error handling = 6 (max realistic)', () => {
it('security path + large change + many hunks + error handling = 7 (max realistic)', () => {
const lines = Array.from({ length: 110 }, (_, i) => `+line${i}`);
const hunkHeaders = Array.from(
{ length: 4 },
Expand All @@ -434,37 +437,79 @@ describe('scoreFiles — combined rules', () => {
'',
].join('\n');
const scores = scoreFiles(diff, []);
expect(scores['src/session/service.go']).toBe(6);
expect(scores['src/session/service.go']).toBe(7);
});

it('multiple files in one diff are scored independently', () => {
const diff =
makeDiff('src/auth/handler.go') + // security → 2
makeLargeDiff('src/utils/big.go', 200) + // large → 2
makeDiff('src/auth/handler.go') + // baseline 1 + security +2 = 3
makeLargeDiff('src/utils/big.go', 200) + // baseline 1 + large +2 = 3
makeDiff('backend/gen/foo.pb.go'); // excluded → 0

const scores = scoreFiles(diff, ['backend/gen/']);
expect(scores['src/auth/handler.go']).toBe(2);
expect(scores['src/utils/big.go']).toBe(2);
expect(scores['src/auth/handler.go']).toBe(3);
expect(scores['src/utils/big.go']).toBe(3);
expect(scores['backend/gen/foo.pb.go']).toBe(0);
});
});

// ── extractFilePath regression: greedy-regex vs indexOf(' b/') ─────────────────

describe('scoreFiles — extractFilePath: indexOf regression for paths containing b/', () => {
it('path with security keyword before a b/ directory component scores 2, not 0', () => {
it('path with security keyword before a b/ directory component scores 3, not 1', () => {
// Old greedy regex: replace(/.*b\//, '') on
// "diff --git a/src/auth/b/helper.ts b/src/auth/b/helper.ts"
// matches everything up to the last 'b/' giving 'helper.ts'.
// 'helper.ts' has no security keyword → score 0.
// 'helper.ts' has no security keyword → baseline score 1 only.
//
// New indexOf(' b/'): strips "diff --git a/" prefix and splits at the
// first ' b/' separator giving 'src/auth/b/helper.ts'.
// 'src/auth/b/helper.ts' matches SECURITY_PATH_RE ('auth') → score 2.
// 'src/auth/b/helper.ts' matches SECURITY_PATH_RE ('auth') → baseline 1 + +2 = 3.
const diff = makeDiff('src/auth/b/helper.ts', ['+changed']);
const scores = scoreFiles(diff, []);
expect(scores['src/auth/b/helper.ts']).toBe(2);
expect(scores['src/auth/b/helper.ts']).toBe(3);
});
});

// ── Baseline score for plain application files ────────────────────────────────

describe('scoreFiles — baseline score 1 for plain application files', () => {
it('plain app file (src/app.ts) with a small change gets score 1, not 0', () => {
// This is the key regression test: ordinary application code must never
// be auto-excluded alongside intentionally-low-risk files.
const diff = makeDiff('src/app.ts', ['+const x = 1;']);
const scores = scoreFiles(diff, []);
expect(scores['src/app.ts']).toBe(1);
});

it('plain server file (src/server.ts) scores 1', () => {
const diff = makeDiff('src/server.ts', ['+app.listen(3000);']);
const scores = scoreFiles(diff, []);
expect(scores['src/server.ts']).toBe(1);
});

it('plain types file (src/types.ts) scores 1', () => {
const diff = makeDiff('src/types.ts', ['+export type Foo = string;']);
const scores = scoreFiles(diff, []);
expect(scores['src/types.ts']).toBe(1);
});

it('React component (src/TeamDetail.tsx) scores 1', () => {
const diff = makeDiff('src/TeamDetail.tsx', ['+export default function TeamDetail() {}']);
const scores = scoreFiles(diff, []);
expect(scores['src/TeamDetail.tsx']).toBe(1);
});

it('service file (src/entityEnrichment.ts) scores 1', () => {
const diff = makeDiff('src/entityEnrichment.ts', ['+export function enrich() {}']);
const scores = scoreFiles(diff, []);
expect(scores['src/entityEnrichment.ts']).toBe(1);
});

it('plain Go file (internal/handler/http.go) scores 1', () => {
const diff = makeDiff('internal/handler/http.go', ['+func handleRequest() {}']);
const scores = scoreFiles(diff, []);
expect(scores['internal/handler/http.go']).toBe(1);
});
});

Expand All @@ -475,16 +520,17 @@ describe('scoreFiles — edge cases', () => {
expect(scoreFiles('', [])).toEqual({});
});

it('empty exclude prefixes → all files scored normally', () => {
it('empty exclude prefixes → all files scored normally (plain files get baseline 1)', () => {
const diff = makeDiff('backend/gen/foo.pb.go');
const scores = scoreFiles(diff, []);
// No security keyword, no large change, no hunks → 0
expect(scores['backend/gen/foo.pb.go']).toBe(0);
// No security keyword, no large change, no hunks, not a test/doc/config file
// → baseline score 1
expect(scores['backend/gen/foo.pb.go']).toBe(1);
});

it('single plain file with no matching rules → score 0', () => {
it('single plain file with no matching rules → baseline score 1', () => {
const diff = makeDiff('src/utils/helper.go', ['+minor tweak']);
const scores = scoreFiles(diff, []);
expect(scores['src/utils/helper.go']).toBe(0);
expect(scores['src/utils/helper.go']).toBe(1);
});
});
15 changes: 12 additions & 3 deletions src/score-risk/score-risk.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,12 +15,17 @@
* (auth|security|crypto|session|secret|token|password|credential, case-insensitive)
* 4. Large change (>100 added lines)→ +2
* 5. Many hunks (>3 hunk headers) → +1
* 6. Test/doc/config file → score = 0 (resets 3-5 to zero)
* 6. Test/doc/config file → score = 0 (resets baseline + 3-5 to zero)
* 7. Error-handling patterns → +1
* (catch|rescue|except|recover|error|panic in any added line, case-sensitive)
*
* Files not caught by rules 1, 2, or 6 start with a baseline score of 1
* so that ordinary application code is never auto-excluded alongside
* intentionally-low-risk files (tests, docs, generated code, JSON configs).
*
* Rule 6 resets the running total to 0, then rule 7 can still add 1.
* This faithfully reproduces the existing bash behaviour.
* This faithfully reproduces the existing bash behaviour for test/doc/config
* files while ensuring plain application files always reach the reviewer.
*/

// ---------------------------------------------------------------------------
Expand Down Expand Up @@ -198,7 +203,11 @@ export function scoreFiles(diffContent: string, excludePrefixes: string[]): Risk
continue;
}

let score = 0;
// Baseline: plain application files that don't match any positive rule
// (rules 3-5) or the reset rule (6) still need review — give them score 1
// so they are not auto-excluded alongside intentionally-low-risk files.
// Rules 1 and 2 already exited early above with score 0; rule 6 resets to 0.
let score = 1;

// Rule 3: security-sensitive path → +2.
if (SECURITY_PATH_RE.test(path)) score += 2;
Expand Down
Loading