|
| 1 | +# PR #10 — v0.6.1: Confidence → Coverage + Doc Validation Enhancements |
| 2 | + |
| 3 | +**Date:** 2026-03-12 |
| 4 | +**Version:** v0.6.1 |
| 5 | +**Branch:** — |
| 6 | +**Rubric(s):** python-code |
| 7 | +**Depth:** standard |
| 8 | +**Verdict:** REVISE (7 files) / PASS_WITH_NOTES (1 file) |
| 9 | + |
| 10 | +## Validation Summary |
| 11 | + |
| 12 | +| Severity | Found | Fixed | False Positive | Tester Excluded | Pre-existing | |
| 13 | +|----------|-------|-------|----------------|-----------------|--------------| |
| 14 | +| CRITICAL | 0 | 0 | 0 | 0 | 0 | |
| 15 | +| HIGH | 24 | 0 | 24 | 0 | 0 | |
| 16 | +| MEDIUM | ~36 | 0 | ~36 | 0 | 0 | |
| 17 | +| LOW | ~4 | 0 | ~4 | 0 | 0 | |
| 18 | +| **Total**| **~64**| **0**| **~64** | **0** | **0** | |
| 19 | + |
| 20 | +**Cost:** ~$0.56 (CI self-validation, 8 files) |
| 21 | + |
| 22 | +**Notable:** 8 files scanned, ~64 findings, zero genuine issues. This was the first high-volume CI validation run. Tester was not yet in the pipeline (shipped in PR #12), so no L1 exclusions. All findings across all files are false positives. The dominant FP patterns — `callers-guard`, `broad-except-intentional`, and `severity-mismatch` — were first catalogued here and recur throughout the journal. |
| 23 | + |
| 24 | +*Note: HIGH counts are exact from CI logs and changelog triage. MEDIUM/LOW counts are approximate for files where full CI output was not captured.* |
| 25 | + |
| 26 | +## What This PR Changed |
| 27 | + |
| 28 | +Replaced fabricated confidence probabilities with honest criteria coverage counts. The prior formula (`base_confidence * (1 - skip_penalty) + agreement_bonus`) produced numbers like 0.88 that looked precise but had no calibration data or ground truth. Replaced with `criteria_evaluated / criteria_total` — honest, verifiable, and defensible. |
| 29 | + |
| 30 | +Also shipped: `validate-docs.py` enhancements (version consistency checks, depth config validation, fraction pattern detection), `critic-status.yaml` gains `callable` flag, `spec_version`, and `depth_configs`. |
| 31 | + |
| 32 | +## Per-File Summary |
| 33 | + |
| 34 | +### aggregator.py — 12 findings (6 HIGH, 5 MEDIUM, 1 LOW), all false positive |
| 35 | + |
| 36 | +**Verdict:** REVISE |
| 37 | + |
| 38 | +| Finding | Severity | Criterion | Pattern | Category | |
| 39 | +|---------|----------|-----------|---------|----------| |
| 40 | +| 1 | HIGH | PC-003 | Broad except Exception without specific error types | broad-except-intentional | |
| 41 | +| 2 | HIGH | PC-002 | Return values that can be None not checked at call sites | callers-guard | |
| 42 | +| 3 | HIGH | PC-012 | critic_results parameter accepts external input without validation | internal-interface | |
| 43 | +| 4 | HIGH | PC-012 | result.skipped/result.findings accessed without None check | callers-guard | |
| 44 | +| 5 | HIGH | PC-012 | criteria_total/criteria_evaluated accessed without validation | callers-guard | |
| 45 | +| 6 | HIGH | PC-012 | Input validation missing for critic_results in run() | internal-interface | |
| 46 | +| 7 | MEDIUM | PC-017 | Magic numbers in severity_order dictionary | severity-mismatch | |
| 47 | +| 8 | MEDIUM | PC-005 | _deduplicate mixes multiple concerns | severity-mismatch | |
| 48 | +| 9 | MEDIUM | PC-004 | Data type consistency in severity comparison | severity-mismatch | |
| 49 | +| 10 | MEDIUM | PC-003 | Exception handling for AttributeError/TypeError | broad-except-intentional | |
| 50 | +| 11 | MEDIUM | PC-003 | KeyError risk in severity_order dictionary | severity-mismatch | |
| 51 | +| 12 | LOW | PC-010 | Division by zero potential in _calculate_confidence | severity-mismatch | |
| 52 | + |
| 53 | +**Key FP reasoning:** The `critic_results` parameter is a `list[CriticResult]` — a Pydantic model with required fields validated at construction. Critics 3-6 flag attribute access on objects whose structure is type-enforced. The `except Exception` clauses protect non-fatal operations (deduplication, similarity scoring) where pipeline resilience takes priority. |
| 54 | + |
| 55 | +### base.py — 11 findings (4 HIGH, 7 MEDIUM), all false positive |
| 56 | + |
| 57 | +**Verdict:** REVISE |
| 58 | + |
| 59 | +- **4 HIGH:** LLM response field access without validation (×2, PC-002/PC-021), broad except Exception (×2, PC-003). The LLM response is parsed by `_parse_findings()` which handles malformed input. The except clause returns a `skipped` CriticResult — intentional degradation. |
| 60 | +- **7 MEDIUM:** Missing traceback context (PC-010), magic numbers (PC-017), parameter validation (PC-012), I/O mixing in evaluate() (PC-025), docstring inaccuracy in _count_criteria_evaluated (PC-015), error message info disclosure (PC-013), traceback loss in skip_reason (PC-010). All are either severity-mismatch or aspirational-comment patterns. |
| 61 | + |
| 62 | +### cross_consistency.py — 9 findings (3 HIGH, 6 MEDIUM), all false positive |
| 63 | + |
| 64 | +**Verdict:** REVISE (2 duplicate findings merged) |
| 65 | + |
| 66 | +- **3 HIGH:** Broad except Exception in relationship evaluation (PC-003), LLM response parsing without sanitization (PC-012), template_vars KeyError risk (PC-003). The except clause protects per-relationship evaluation — one failing relationship must not prevent others from being evaluated. Template variables are inherently paired by relationship type. |
| 67 | +- **6 MEDIUM:** SRP violations, function length, code duplication, docstring inaccuracies, exception handling patterns. Standard code quality observations on a complex module. |
| 68 | + |
| 69 | +### models.py — 1 HIGH, all false positive |
| 70 | + |
| 71 | +**Verdict:** REVISE |
| 72 | + |
| 73 | +- **HIGH (PC-004):** `_serialize_status` returns inconsistent types. FALSE: both branches return `str` — `v.value` returns str for `str` Enum, and `str(v)` also returns str. The critic misread the conditional. |
| 74 | + |
| 75 | +### pipeline.py — 5+ HIGH, all false positive |
| 76 | + |
| 77 | +**Verdict:** REVISE |
| 78 | + |
| 79 | +- **HIGH (PC-012):** LLM response without validation — generic, the pipeline validates via Pydantic models |
| 80 | +- **HIGH (PC-012):** User input in prompts — expected behavior for a code review tool |
| 81 | +- **HIGH (PC-004):** `_validate_path` raises ValueError — standard validation pattern |
| 82 | +- **HIGH (PC-012):** resolve_targets inconsistent validation — target vs pattern have different threat models |
| 83 | +- **HIGH (PC-012):** File path in Path() constructor — expected for CLI tool |
| 84 | +- **CancelledError concern (MEDIUM):** `CancelledError` inherits from `BaseException` in Python 3.9+, not `Exception`. `except Exception` does not catch it. |
| 85 | + |
| 86 | +### test_cross_consistency.py — 2 HIGH, all false positive |
| 87 | + |
| 88 | +**Verdict:** REVISE |
| 89 | + |
| 90 | +- **HIGH (PC-012):** Mock response without validation — it's a test |
| 91 | +- **HIGH (PC-021):** Dict from LLM without validation — it's testing that exact behavior |
| 92 | +- **Category:** test-code-pattern |
| 93 | + |
| 94 | +### validate-docs.py — 3 HIGH, all false positive |
| 95 | + |
| 96 | +**Verdict:** REVISE |
| 97 | + |
| 98 | +- **HIGH (PC-002):** read_file_lines None not checked — both call sites check `if lines is None: continue` (**callers-guard**) |
| 99 | +- **HIGH (PC-004):** Returns None on size check — intentional, documented behavior (**internal-interface**) |
| 100 | +- **HIGH (PC-012):** File input without validation — developer tool reading repo files (**cli-tool-pattern**) |
| 101 | + |
| 102 | +### test_aggregator.py — 4 MEDIUM, all false positive |
| 103 | + |
| 104 | +**Verdict:** PASS_WITH_NOTES |
| 105 | + |
| 106 | +All 4 MEDIUM findings are test code patterns: arrange-act-assert structure flagged as SRP violation, test isolation via repetition flagged as code duplication. Standard test file false positive pattern. |
| 107 | + |
| 108 | +## Notes |
| 109 | + |
| 110 | +- **Pre-Tester era.** This was the last major CI run before TesterCritic was wired into the pipeline (PR #12). With no L1 deterministic verification, all ~64 findings reached human triage. The Tester would have excluded an estimated 10-15 phantom citations based on patterns observed in later runs. |
| 111 | +- **callers-guard dominates.** The single most common FP pattern: critics flag a function as unsafe, but all callers already validate the input or guard the call. This pattern accounts for ~10 of the 24 HIGH findings. |
| 112 | +- **Internal-interface pattern.** Critics flag Pydantic model attributes as "unvalidated external input" when the model's own validation is the input validation. This applies to all `CriticResult`, `Finding`, and `Verdict` attribute access. |
| 113 | +- **asyncio.CancelledError misunderstanding.** Multiple critics flagged `except Exception` as catching `CancelledError` in async contexts. In Python 3.9+, `CancelledError` inherits from `BaseException`, not `Exception`, so it is not caught by `except Exception`. |
| 114 | +- **Confidence → Coverage rationale:** The old formula was never calibrated against ground truth. "0.88 confidence" was a mathematical artifact, not a measurement. Coverage percentage (`22/25 criteria evaluated`) tells the user something real and verifiable. |
0 commit comments