fix(score-risk): give plain application files a baseline score of 1#9
fix(score-risk): give plain application files a baseline score of 1#9derekmisler wants to merge 5 commits into
Conversation
Previously, let score = 0 meant any file with no special signals (security path, large change, many hunks, error handling) stayed at score 0. The auto-filter-diff Phase 1 then excluded ALL score-0 files, leaving nothing for the reviewer to look at when a PR contained only ordinary application code. Fix: change the starting score to 1 for any file that survives the early-exit rules (1 = exclude-paths prefix, 2 = generated-file marker). Rule 6 (test/doc/config reset) still resets to 0 before rule 7, so tests, docs, JSON configs, and generated files continue to be auto-excluded. Only intentional low-risk files score 0; normal application code now scores ≥1 and is kept for review. Score changes with new baseline: plain app file (no signals) 0 → 1 security path only 2 → 3 large change only 2 → 3 many hunks only 1 → 2 error handling only 1 → 2 security + large 4 → 5 security + large + hunks 5 → 6 security + large + hunks + err 6 → 7 test/doc/config (rule 6 reset) 0 → 0 (unchanged) exclude-path prefix (rule 1) 0 → 0 (unchanged) generated marker (rule 2) 0 → 0 (unchanged) Updated all test expectations accordingly and added a dedicated 'baseline score 1 for plain application files' suite with six new test cases covering src/app.ts, server.ts, types.ts, TeamDetail.tsx, entityEnrichment.ts, and internal/handler/http.go.
docker-agent
left a comment
docker-agent
left a comment
There was a problem hiding this comment.
Assessment: 🟢 APPROVE
The baseline score change from 0 to 1 is correct and internally consistent. Rules 1, 2, and 6 all short-circuit or reset to 0 before the baseline is applied, so the only files affected are plain application files with no special signals — exactly the intended behaviour. All 427 tests pass.
Three very minor comment/documentation observations (not blocking):
-
Line 87 (
score-risk.test.ts): The comment"session_service.pb.go" would score via the security-path ruledropped the numeric score. With baseline=1, the correct value is now +3 (1 baseline + 2 security-path), not the old +2. Stating the new total would help future readers understand the rule interaction. -
Line 279 (
score-risk.test.ts): The commentrule 3 (+2) + baseline (1) are reset by rule 6is accurate, but doesn't mention that rule 7 (error-handling keywords) can still fire after rule 6 resets to 0. The module-level JSDoc already documents this, so it's a minor gap rather than a real bug. -
Line 528 (
score-risk.test.ts): The assertionexpect(scores['backend/gen/foo.pb.go']).toBe(1)is correct for the new baseline when no exclude prefix is provided andmakeDiffproduces no generated-marker lines. Worth a comment clarifying thatmakeDiffdoesn't emit aCode generatedmarker line, so rule 2 doesn't fire — otherwise readers may expect generated-file suppression here.
None of these are logic errors in the changed code. The fix is correct.
- Rule 1 comment: state the numeric score (3) that the security-path file would receive without the exclude-prefix short-circuit - Rule 6 loop comment: note that rule 7 can still add +1 after the reset (even though no error-handling keywords fire in this particular diff fixture) - Edge-case comment: explain that makeDiff() emits no 'Code generated' added line, so rule 2 does not fire and the baseline-1 score stands
derekmisler
left a comment
derekmisler
left a comment
There was a problem hiding this comment.
Thanks for the thorough review. All three comments were accurate — addressed them in the follow-up commit (0d93edf):
- Clarified the security-path score from "via the security-path rule" to the exact value (3 = baseline 1 + security +2).
- Added a note that rule 7 can still fire after the rule 6 reset, even though the diff fixture in that loop doesn't contain error-handling keywords.
- Explained that
makeDiff()emits noCode generatedadded line, so rule 2 doesn't fire and the baseline-1 score stands.
docker-agent
left a comment
docker-agent
left a comment
There was a problem hiding this comment.
Assessment: 🟢 APPROVE
The baseline score change from 0 to 1 is correctly placed — after the early-exit rules (rule 1: exclude-paths prefix, rule 2: generated-file marker) that already set score = 0 and continue, and before the additive rules (3–5, 7) and the reset rule (rule 6: test/doc/config). This means plain application files correctly receive a score of 1, while test/doc/config files are still reset to 0 by rule 6.
All updated test expectations in score-risk.test.ts accurately reflect the +1 baseline shift, and the 6 new baseline test cases cover the intended file types. No bugs were introduced.
…sets, dotfiles
The baseline-score-1 change made any file NOT matched by the old
TEST_FILE_RE survive auto-exclusion with score 1. Lock files, binary
assets, dotfiles, CSS, LICENSE, and .env.example were not matched and
would incorrectly reach the reviewer.
Changes:
- Rename TEST_FILE_RE → LOW_RISK_FILE_RE to reflect the broader scope
- Add LOW_RISK_FILE_EXTRA_RE (RegExp constructor so the source can be
split over labelled lines) covering:
Lock files: yarn.lock, package-lock.json, Cargo.lock, go.sum,
Gemfile.lock, pnpm-lock.yaml, composer.lock, poetry.lock
Assets: .svg, .png, .jpg/.jpeg, .gif, .ico, .woff/.woff2, .eot, .ttf
CSS: .css
Dotfiles: .gitignore, .gitattributes, .editorconfig,
.prettierrc[.*], .eslintignore, .dockerignore
Misc: LICENSE[.*], .env.example
- Add helper isLowRiskFile() that checks both regexes
- Update rule 6 comment to say 'low-risk file' instead of 'test/doc/config'
Tests (463 total, +36 vs previous):
- New describe block for each new category (lock files, assets, CSS,
dotfiles, legal/example) — one test per path, plus:
- lock file with security keyword still scores 0 (rule 6 beats rule 3)
- dotfile with error keyword scores 1 (rule 7 still applies after reset)
- Regression cases in the baseline-1 suite confirming yarn.lock,
pnpm-lock.yaml, and .gitignore score 0 (not promoted to 1)
|
Fixed in commit a5bd00c. Renamed TEST_FILE_RE → LOW_RISK_FILE_RE and added LOW_RISK_FILE_EXTRA_RE (via RegExp constructor so the source can be split over labelled sections) covering all the missing categories:
Added 36 new tests: one per path for the new categories plus edge cases (lock file with security keyword still scores 0, dotfile with error keyword scores 1 after rule-7 fires post-reset) and regression cases in the baseline-1 suite confirming yarn.lock, pnpm-lock.yaml, and .gitignore stay at 0. |
Previously, Phase 1 could remove every file from the diff when all changed files scored 0 (e.g. a PR touching only lock files, CSS, or dotfiles). The downstream steps were then silently skipped, leaving the reviewer with no feedback at all. Fix: after Phase 1 filtering, if keptSections would be empty and the original diff had sections, reset to keeping all sections (skip Phase 1 for this diff). Phase 2 (progressive cap) still applies, so timeout protection is preserved. Behaviour: Mixed PR (some low-risk, some high-risk) → Phase 1 removes score-0 files ✓ All-low-risk PR (e.g. only lock files) → Phase 1 kept all instead ✓ Large all-low-risk PR → Phase 1 kept all, Phase 2 trims ✓ New AutoFilterResult field allFilesKept: true signals the fallback fired. index.ts logs: 'All files scored 0 — keeping all files for review'. Tests: 8 new cases covering single score-0 file, lock-file-only PR, all-score-0 + Phase 2 cap, round-trip fidelity, and allFilesKept=false for normal/empty/no-exclusion cases. Total: 470 (all passing).
2 participants
Problem
let score = 0inscore-risk.tsmeant any file with no special signals (security path, large change, many hunks, error handling) stayed at score 0. The auto-filter-diff Phase 1 then excluded ALL score-0 files, so PRs containing only ordinary application code (e.g.types.ts,server.ts,TeamDetail.tsx) had everything auto-excluded — leaving nothing for the reviewer.Fix
Changed
let score = 0tolet score = 1(the baseline is set after the early-exit rules 1 and 2 that already set score=0 and continue). Rule 6 (test/doc/config reset) still resets to 0, so tests, docs, JSON configs, and generated files continue to be auto-excluded correctly.Tests
score-risk.test.tsto reflect the new baselineapp.ts,server.ts,types.ts,TeamDetail.tsx,entityEnrichment.ts,http.go)auto-filter-diff.test.tsrequired no changes (passes explicit riskScores, independent of scoreFiles)