Sprint 3: Review quality + coverage enforcement - #19
Merged
Conversation
- New src/risk_classifier.py: pure Python file risk scoring (HIGH/MEDIUM/LOW) using lines changed, path sensitivity, new file flag, test coverage gaps, caller count, and file type. All weights/thresholds configurable via config.py. - Task 8: inject risk classification table into review prompt (_build_review_context) - Task 9: add risk-based depth instructions + 100% coverage requirement to Step 4 of review-pr-core.md - Task 10: add files_clean[] to FindingsFile model, _parse_findings_file, and review-pr-core.md Step 7 schema - Task 11: coverage gate in post_findings._evaluate_gate (hard/log modes), coverage penalty in PRScorer.apply_coverage_penalty, and summary display showing 'X / Y (Z%)'. Coverage gate only fires when agent uses files_clean[]. - Task 12: batch_max_turns default changed from 15 to 40; ReviewJobConfig.max_turns default also changed from 15 to 40. All turn limits now 40. - config.py: coverage_gate_mode, risk_high_threshold, risk_medium_threshold fields - 28 new unit tests in test_coverage_system.py; all 249 unit tests pass
…ew checklists Co-Authored-By: Siddharth Deshpande <siddharthd@iprdgroup.com>
Co-Authored-By: Siddharth Deshpande
Tasks 22-27 verified: languages.yml, stack_detector.py, 15 lang-rules files, review_job integration, file_filter registry mode. 302 unit tests pass.
…k detector, 15 lang-rules verified) @
Co-Authored-By: Siddharth Deshpande <siddharthd@iprdgroup.com>
…hes, 331 tests verified) @
The 5 review mode files (security, architecture, performance, migration, standard) were referenced by path in review-pr-core.md but never loaded into the prompt. The model was told to "apply the security checklist" without ever receiving it — explaining zero security findings across 100+ PRs. Adds _build_review_modes_section() to inline all mode checklists (~3.5K tokens) into the user prompt alongside diffs and config.
- Add mandatory multi-turn review process (read diffs → verify with tools → output findings) instead of single-turn output - Replace previous_response_id with sliding window (last 3 exchanges) to cap per-turn token cost on Responses API - Add nudge logic: when agent produces text without findings JSON, prompt it to continue reviewing or force output near deadline - Skip temperature/seed for codex models - Prefix activity loggers with codehawk namespace
…pdates - Lower min confidence filter from 0.7 to 0.5 to surface more findings - Expand LOW risk review depth to include style, naming, error handling - Require test file review at LOW depth instead of skipping - Reduce default batch size from 25 to 10 files - Add title auto-fill from message when agent omits it - Update test fixtures and thresholds to match new confidence floor - Point integration tests at PR 6658 with CHECK_NEW mode and dry_run
Replace the single 38-turn agent loop with two focused passes: - Pass 1 (scan): single-turn, no tools — identifies candidates from diffs - Pass 2 (verify): short agent loop (10 turns), full history, no sliding window - Fallback: reverts to single-pass on any failure Live results on PR #6658 (98 files): - Tokens: 11.3M → 1.37M (88% reduction) - Cost: $23.69 → $3.41 (86% reduction) - Duration: 34min → 9min (73% reduction) - Findings: 17 high-quality findings with concrete suggestions New files: review-scan.md, review-verify.md, ScanCandidate model Config: two_pass_enabled, scan_pass_max_retries, verify_pass_max_turns Tests: 34 new unit tests, 365 total passing
- csharp.md: add .NET Framework 4.x, EF 6.x, ASP.NET Web API 5.x sections - react.md: expand React 16 with DO NOT suggest guards, React Router v5, Redux 3.x - javascript.md: add ES2017-ES2019 section with pre-optional-chaining rules, Node 12.x - python.md: expand from 15 to 94 items using BluB0X code_review_rules.md as source (security, logging, HTTP timeouts, async, OpenAI clients, config, dependencies)
… scan fixes Lang-rules injection: - Always inject lang-rules alongside .codereview.md (not either/or) - Detect languages from changed file extensions per batch (not workspace config) - CSS/SCSS/LESS registered in languages.yml with 53-item css.md ruleset Lang-rules expanded: - csharp: +LINQ section, .NET Framework 4.x, EF 6.x, Web API 5.x - java: +Java 8+ section (Stream, Optional, CompletableFuture, DateTime) - javascript: +12 security/correctness rules (fetch errors, FormData, prototype pollution) - react: +Accessibility (10 items), +Testing (5 items), expanded React 16 guards - go: +6 rules (pointer receivers, context cancel, http timeout, race detector) - typescript: fixed duplicate TS 4.x section, +5 rules Multi-category scan fixes: - review-scan.md: per-file checklist structure forces all categories per file - System prompt: detailed category hints with specific patterns to look for - Examples: 5 multi-category examples (security, performance, architecture, error_handling, correctness) - scoring.md + review-verify.md: corrected confidence threshold docs (0.7 → 0.5 matching code) Results: security and error_handling findings now appearing (was always 0 before)
feat(review): dual-scan architecture — split Pass 1 into standard + deep scans Pass 1A handles correctness, error_handling, testing, code_style with a focused system prompt. Pass 1B handles security, performance, architecture with a specialist prompt. Candidates are merged with dedup by (file, line, category). Also strips unknown keys from findings to prevent schema pollution. @
- Re-enable sliding window in verify pass (was False, causing token escalation from 6K→39K per turn as full history accumulated) - Reduce verify_pass_max_turns from 10 to 7 (eliminates expensive late turns while keeping enough tool budget for verification) - Lower smart_diff_threshold_kb from 30 to 15 (more diffs summarized, agent drills in via tool calls when needed) - Inject risk context into Pass 1B deep scan prompt (analysis param was accepted but never used — model was flying blind on which files to scrutinize for security/perf/arch issues) Results on PR #6658: - Cost: $4.36 → $3.46 (20% reduction) - Security findings: 1 → 3 (XSS + upload guard + exception leak) - Total findings: 23 → 27
…tput Treat model JSON output as untrusted input. Schema validation errors now warn and continue instead of raising SystemExit. - Add SEVERITY_REMAP: major→warning, blocker→critical, minor→suggestion, etc. - _normalize_findings guarantees all required fields with correct types: missing fields get defaults, wrong types get coerced, out-of-range confidence gets clamped, non-dict findings get skipped - _parse_findings_file uses .get() with defaults and try/except per item: one broken finding doesn't kill the rest - Integration test catches SystemExit from quality gate failures Triggered by PR 6686 where model returned severity="major" (not in schema enum) causing the entire review to crash with SystemExit(1).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
files_clean[]schema, coverage gate (hard/log mode), coverage penalty scoring, batch turn budget aligned to 40testingcategory, expanded to 9 valid categories (architecture, correctness, error_handling), architecture + performance review checklists.cr/Test plan