chore: post-audit quick wins (name-based scores, CI coverage gate) - #3
Merged
Conversation
Regenerate .planning/codebase/ (7 docs) to reflect main after the audit- remediation merge (PR #2): gpt2_app.py rename, new src/ui/ and src/evaluation/ packages, calibration.py + binoculars_analyzer.py, EnsembleConfig-driven calibrated fusion, safetensors/patched-dep floors, CI, ~231 tests at ~92% coverage, and the current remaining concerns (large-benchmark eval, revision pinning, scores[0] ordering, numpy<2, py3.9 Docker base, CI mypy/coverage gates). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
M2 — remove positional score coupling: The ensemble and Binoculars analyzers read the fused primary score via result.scores[0] and the voter set via scores[1:], so reordering or inserting a score would silently break verdict math. Add AnalysisResult.get_score(name) and address scores by name: - EnsembleAnalyzer.ENSEMBLE_SCORE_NAME constant + _voter_scores() helper. - BinocularsAnalyzer.PRIMARY_SCORE_NAME constant. New test asserts the ensemble verdict is unchanged when the primary score is moved out of index 0. CI — guard against coverage erosion: Add --cov-fail-under=80 to the test job. The fast (not-slow) suite currently covers 85% of src/, so this is a real floor without breaking the green build. Tests: 54 (M2 suites incl. real-model binoculars) + get_score unit tests; fast suite 231 passed at 85% coverage. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThis PR adds a name-based ChangesNamed Score Lookup and Coverage Enforcement
Estimated code review effort: 3 (Moderate) | ~25 minutes Codebase Planning Documentation Refresh
Sequence Diagram(s)sequenceDiagram
participant Caller
participant EnsembleAnalyzer
participant AnalysisResult
Caller->>EnsembleAnalyzer: _determine_verdict(result)
EnsembleAnalyzer->>AnalysisResult: get_score(ENSEMBLE_SCORE_NAME)
AnalysisResult-->>EnsembleAnalyzer: DetectionScore or None
EnsembleAnalyzer->>EnsembleAnalyzer: _voter_scores(result)
EnsembleAnalyzer-->>Caller: verdict
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (4)
src/models/result.py (1)
110-119: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd Google-style
Args/Returnsdocstring blocks.
get_scoreis a public method with a parameter and return value but its docstring omitsArgs/Returnssections.📝 Proposed docstring update
def get_score(self, name: str) -> Optional[DetectionScore]: - """Return the first score with the given name, or None. + """Return the first score with the given name, or None. - Lets callers address scores by name instead of list position, so the - order in which scores are added is not a load-bearing contract. - """ + Lets callers address scores by name instead of list position, so the + order in which scores are added is not a load-bearing contract. + + Args: + name: The score name to look up. + + Returns: + The first matching DetectionScore, or None if not found. + """As per coding guidelines, "Public methods should include Google-style
ArgsandReturnsdocumentation blocks."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/models/result.py` around lines 110 - 119, The public method get_score in Result is missing the required Google-style docstring sections. Update its docstring to include explicit Args for name and a Returns block describing the Optional[DetectionScore] result, while keeping the existing behavior and summary intact.Source: Coding guidelines
src/analyzers/ensemble_analyzer.py (2)
348-353: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReturn type should be
List[DetectionScore], not barelist.
_voter_scoresloses element-type information for mypy/readers.Optionalis already imported here; addListand annotate precisely.🏷️ Proposed fix
- def _voter_scores(self, result: AnalysisResult) -> list: + def _voter_scores(self, result: AnalysisResult) -> List[DetectionScore]: """Contributing analyzer scores: everything except the fused primary score, restricted to non-zero weight (so a disabled analyzer's row is never counted as a voter).""" return [s for s in result.scores if s.name != self.ENSEMBLE_SCORE_NAME and s.weight > 0](Requires
Listin thetypingimport line, alongside the existingOptional.)As per coding guidelines, "Prefer
from __future__ import annotationsand typing imports such asOptional,List,Dict, andTuple," which this repo runs mypy against in CI.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/analyzers/ensemble_analyzer.py` around lines 348 - 353, The _voter_scores method in EnsembleAnalyzer currently returns a bare list, which loses the element type for mypy and readers. Update the typing in _voter_scores to return List[DetectionScore], and add List to the existing typing imports near Optional so the annotation is precise and consistent with the rest of the analyzer code.Source: Coding guidelines
275-284: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReplace the positional score comment with name-based wording
# Add ensemble score (always index 0 by contract).is outdated now thatENSEMBLE_SCORE_NAMEis the lookup key; keep the comment aligned so it doesn’t suggest positional access.📝 Proposed fix
- # Add ensemble score (always index 0 by contract). + # Add the fused primary score, addressed by name (ENSEMBLE_SCORE_NAME), + # not by list position. result.add_score( DetectionScore( name=self.ENSEMBLE_SCORE_NAME,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/analyzers/ensemble_analyzer.py` around lines 275 - 284, Update the outdated comment in EnsembleAnalyzer around the DetectionScore addition so it no longer implies the ensemble score is accessed by position; the code now uses ENSEMBLE_SCORE_NAME as the lookup key. Keep the comment aligned with the current contract by describing name-based identification instead of “always index 0,” and make the wording consistent with add_score and _interpret_ensemble_score usage..github/workflows/ci.yml (1)
54-54: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCoverage gate diverges from the Makefile's
testtarget.The Makefile's canonical
make testcommand (PYTHONPATH=src $(PYTHON) -m pytest tests/ -v --cov=src --cov-report=html --cov-report=term-missing) has no--cov-fail-under, so contributors running tests locally won't hit the same 80% gate CI now enforces. Consider adding the same--cov-fail-under=80to the Makefile target (or centralizing the threshold inpytest.ini/.coveragerc/pyproject.tomlso both consumers stay in sync).♻️ Suggested Makefile alignment
test: ## Run tests with coverage - PYTHONPATH=src $(PYTHON) -m pytest tests/ -v --cov=src --cov-report=html --cov-report=term-missing + PYTHONPATH=src $(PYTHON) -m pytest tests/ -v --cov=src --cov-report=html --cov-report=term-missing --cov-fail-under=80🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/ci.yml at line 54, The coverage threshold is enforced in CI but not in the canonical local test path, so `make test` and the workflow are out of sync. Update the Makefile `test` target to include the same `--cov-fail-under=80` used by the CI pytest command, or move the threshold into shared pytest coverage config so both `pytest` and the workflow consume the same setting. Use the `test` target and the CI pytest invocation as the two places to align.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In @.github/workflows/ci.yml:
- Line 54: The coverage threshold is enforced in CI but not in the canonical
local test path, so `make test` and the workflow are out of sync. Update the
Makefile `test` target to include the same `--cov-fail-under=80` used by the CI
pytest command, or move the threshold into shared pytest coverage config so both
`pytest` and the workflow consume the same setting. Use the `test` target and
the CI pytest invocation as the two places to align.
In `@src/analyzers/ensemble_analyzer.py`:
- Around line 348-353: The _voter_scores method in EnsembleAnalyzer currently
returns a bare list, which loses the element type for mypy and readers. Update
the typing in _voter_scores to return List[DetectionScore], and add List to the
existing typing imports near Optional so the annotation is precise and
consistent with the rest of the analyzer code.
- Around line 275-284: Update the outdated comment in EnsembleAnalyzer around
the DetectionScore addition so it no longer implies the ensemble score is
accessed by position; the code now uses ENSEMBLE_SCORE_NAME as the lookup key.
Keep the comment aligned with the current contract by describing name-based
identification instead of “always index 0,” and make the wording consistent with
add_score and _interpret_ensemble_score usage.
In `@src/models/result.py`:
- Around line 110-119: The public method get_score in Result is missing the
required Google-style docstring sections. Update its docstring to include
explicit Args for name and a Returns block describing the
Optional[DetectionScore] result, while keeping the existing behavior and summary
intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 49abec29-cf32-4a95-a483-c71a9fa501a3
📒 Files selected for processing (13)
.github/workflows/ci.yml.planning/codebase/ARCHITECTURE.md.planning/codebase/CONCERNS.md.planning/codebase/CONVENTIONS.md.planning/codebase/INTEGRATIONS.md.planning/codebase/STACK.md.planning/codebase/STRUCTURE.md.planning/codebase/TESTING.mdsrc/analyzers/binoculars_analyzer.pysrc/analyzers/ensemble_analyzer.pysrc/models/result.pytests/test_ensemble_weighted_fusion.pytests/test_result_model.py
This was referenced Jul 8, 2026
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.
Follow-up polish after the audit-remediation merge (#2).
Changes
scores[0]/scores[1:]); reordering could silently break verdict math. AddedAnalysisResult.get_score(name)and switched to name-based access (ENSEMBLE_SCORE_NAME,PRIMARY_SCORE_NAME,_voter_scores()). New test asserts the verdict is invariant to moving the primary score out of index 0.--cov-fail-under=80to the test job. Fast (not slow) suite currently covers 85%, so this is a real floor without breaking the build..planning/codebase/map to the post-remediation state (separate concern, already onmain).Validation
Not included (deferred)
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests
Documentation
Chores