Skip to content

feedback.md

Latest commit

 

History

History
129 lines (81 loc) · 9.57 KB

feedback.md

File metadata and controls

129 lines (81 loc) · 9.57 KB
Raw

Sprint 3 — Review Quality + Coverage Enforcement — Code Review

Reviewer: local-codehawk-reviewer Date: 2026-05-22 23:45:00+05:30 Verdict: APPROVED

See the recent git history of this file to understand the context of this review.

Phases 1-5 Regression Check

PASS. All previously approved phases (1 through 5) re-verified against the Phase 6 commit (55e7888). No regressions found:

  • temperature=0.3 and seed=42 in openai_runner.py — unchanged (Phase 1).
  • "max 40 tool calls" in review-pr-core.md Step 0 — present; "max 10" absent (Phase 1).
  • "ZERO or minimal" absent from SYSTEM_PROMPT — correctly removed in Phase 1.
  • Step 5e verify-before-CRITICAL — present in review-pr-core.md (Phase 1).
  • failed_diffs injection in review_job.py — unchanged (Phase 1).
  • risk_classifier.py — unchanged; thresholds risk_high_threshold=0.6, risk_medium_threshold=0.3 in config.py (Phase 2).
  • files_clean in review_models.py and findings-schema.json — present (Phase 2).
  • Coverage gate hard/log modes in post_findings.py — unchanged (Phase 2).
  • batch_max_turns default 40 in config.py — unchanged (Phase 2).
  • 9 valid categories, 4 remap entries in post_findings.py — unchanged (Phase 3).
  • Architecture and performance checklist files present (Phase 3).
  • merge_similar_findings() and _write_audit_trail() in post_findings.py — unchanged (Phase 4).
  • All 15 language rules files in commands/lang-rules/ — present (Phase 5).
  • stack_detector.py with detect() function — unchanged (Phase 5).
  • _build_lang_rules_section in review_job.py — unchanged (Phase 5).

Task 28: Fix Verifier

PASS. src/fix_verifier.py created with deterministic fix verification algorithm.

  1. Deleted files -> not_relevant (no LLM). _get_file_statuses parses git diff --name-status for D entries. Findings on deleted files immediately get status="not_relevant" with reason "File was deleted or removed from the repository." No LLM call made. Unit test test_deleted_file_all_findings_not_relevant confirms. PASS.

  2. Unchanged files -> still_present (no LLM). If a file is not in the diff output (neither modified, deleted, nor renamed), all its findings get status="still_present". Unit test test_unchanged_file_all_findings_still_present_no_llm confirms _call_llm_for_verification is never called. PASS.

  3. Modified files -> file-level LLM verification (one call per file). _verify_single_file reads the full file content, builds a verification prompt with all findings for that file, and makes one _call_llm_for_verification call. Unit test test_modified_file_3_findings_one_llm_call confirms mock_llm.call_count == 1 for 3 findings on the same file. PASS.

  4. 15-file cap with overflow batching. _INDIVIDUAL_FILE_CAP = 15 and _BATCH_FILE_SIZE = 5. First 15 files processed individually, overflow files batched in groups of 5 (each still gets its own LLM call within the batch). Unit test test_20_modified_files_respects_cap confirms 20 total calls for 20 files. PASS.

  5. Blast radius checks. _annotate_blast_radius checks fixed findings with severity="critical" or category="architecture" against the graph store. If dependents exist, appends informational note. Does not change verdict. Unit tests confirm annotation for critical findings with dependents, no annotation for still_present, and no annotation when graph returns empty. PASS.

  6. ADO thread posting. _post_results_to_ado posts to ADO threads: "Verified fixed" for fixed, "File deleted/removed" for not_relevant, and "Still present" via _post_still_present_reply for still_present findings. PASS.

  7. Default to still_present on failure. _map_llm_results_to_verifications defaults to still_present when: LLM returns empty list, LLM returns None, finding index missing from LLM results, or unrecognized status string. Four separate unit tests confirm all these paths. PASS.

  8. review-pr-core.md Step 6 updated. Step 6 now reads "Fix verification is handled automatically by the system after your review completes." No sub-steps 6a/6b/6c/6d. No "Write fix_verifications" instruction. Unit tests TestReviewPrCoreStep6 confirm all three assertions. PASS.

  9. Renamed file handling. _get_file_statuses parses R100 lines, maps old path to new path in renamed_map, and adds the new path to modified set. The main loop remaps findings via renamed_map.get(file_path, file_path) before checking modified set. Unit test test_renamed_file_detected confirms. PASS.

NOTE (non-blocking): The _verify_files_with_llm overflow batching loop iterates files individually within each batch of 5, calling _verify_single_file per file. The batching structure groups calls but doesn't combine multiple files into a single LLM call. This is functionally correct and arguably better for reliability (one file per call), but the 15-file cap + batching described in the docstring ("overflow batched 5 per call") is slightly misleading — it's "batched 5 per iteration" with one call per file. No functional impact.

Task 29: Parallel Batch Execution

PASS. src/batch_review_job.py updated with ThreadPoolExecutor and retry logic.

  1. ThreadPoolExecutor with max 3 workers. Line 124: max_workers = min(3, batch_total). Line 126: with ThreadPoolExecutor(max_workers=max_workers) as pool:. Futures submitted via pool.submit(self._run_batch_with_retry, ...) and collected via as_completed(futures). PASS.

  2. Exponential backoff retry. _run_batch_with_retry at line 267: delays = [1, 2, 4]. Rate-limit detection checks for "429", "ratelimit", "too many requests" in exception string, plus exception class name check. Non-rate-limit errors raised immediately without retry. PASS.

  3. Batch failure isolation. Line 147-150: except Exception as exc: logger.error(...) — failed batches log the error but don't crash the pipeline. Other batches continue. PASS.

  4. Parallel faster than sequential. Unit test test_parallel_batches_complete_faster_than_sequential confirms 3 batches with 0.1s delay each complete in < 0.25s (not 0.3s sequential). PASS.

  5. Rate limit retry test. test_rate_limit_error_retries_with_backoff confirms first failure triggers retry with sleep(1), second attempt succeeds. PASS.

  6. Non-rate-limit errors not retried. test_non_rate_limit_error_not_retried confirms ValueError raises immediately with call_count == 1. PASS.

  7. Exhausted retries raise. test_exhausted_retries_raises_last_exception confirms 3 rate-limit failures exhaust retries and raise, with call_count == 3. PASS.

NOTE (non-blocking): The retry loop sleeps on the 3rd (final) attempt before the loop ends and raises last_exc. This wastes a 4-second sleep before the inevitable failure. A minor optimization would be to check attempt < len(delays) before sleeping on the last iteration, or restructure as initial attempt + retry loop. Not blocking since the behavior is correct — it just adds 4s of unnecessary latency on exhausted retries.

Task 30: VERIFY — Fix Verification + Parallelism

PASS. 331 unit tests pass in 16.94 seconds. 29 new Phase 6 tests covering:

  • TestGetFileStatuses (5 tests): deleted, modified, renamed detection; empty commit; git failure
  • TestVerifyFixesDeterministic (3 tests): deleted -> not_relevant, unchanged -> still_present, empty input
  • TestVerifyFixesLLM (6 tests): one call per file, 5 files = 5 calls, 20-file cap, LLM failure, garbage JSON, unreadable file
  • TestMapLLMResults (5 tests): fixed/still_present mapping, missing results, unknown status, empty results
  • TestBlastRadiusAnnotation (3 tests): critical fixed + dependents, still_present skipped, empty dependents
  • TestParallelBatchExecution (4 tests): parallel timing, rate-limit retry, non-rate-limit no-retry, exhausted retries
  • TestReviewPrCoreStep6 (3 tests): no sub-steps, "handled automatically" present, no "Write fix_verifications"

Test coverage is meaningful — all deterministic paths, LLM failure modes, retry logic, and prompt update assertions are tested. No overlapping or redundant tests. PASS.

Test Suite Health

All 331 unit tests pass (0 failures, 0 errors, 16.94s). Breakdown by phase:

Phase Tests Added Cumulative
Phase 1 224 (baseline) 224
Phase 2 28 249*
Phase 3 3 252
Phase 4 21 270*
Phase 5 32 302
Phase 6 29 331

*Phase totals adjusted per progress.json; some phases include fixture/conftest additions.

Security Check

  • fix_verifier.py uses subprocess.run with a list (not shell=True) for git diff. PASS.
  • LLM API key read from os.environ only — not hardcoded. PASS.
  • ADO posting uses BasicAuthentication with token from settings — no secrets in code. PASS.
  • File content truncated to 8000 chars in LLM prompt to prevent token abuse. PASS.

Summary

APPROVED. All three Phase 6 tasks (28-30) meet their PLAN.md "Done when" criteria. The fix verifier correctly handles all three resolution paths (deleted, unchanged, modified) with appropriate defaults-to-still_present safety. Parallel batch execution uses ThreadPoolExecutor with max 3 workers and exponential backoff retry. 331 unit tests pass with no regressions in Phases 1-5.

Two non-blocking notes documented above (overflow batching docstring wording, wasted sleep on final retry) — neither affects correctness or requires changes.

This completes the Sprint 3 — Review Quality + Coverage Enforcement final review. All 6 phases approved.