Conversation
…add coverage tests Security: - aiohttp>=3.13.4 closes 10 CVEs (CVE-2026-34513 through -34525 and GHSA variants) Config: - [tool.black] line-length 88 → 120 to match ruff (eliminates formatter conflict) Tests: - tests/test_finder_new_coverage.py: 40 new tests for apps/finder.py critical paths (get_applications, get_applications_from_system_profiler, batch error handling, rate limiter, async fallback, progress config) finder.py coverage: 69.9% → 82.4% (+12.5 pp) - tests/test_comparator_coverage.py: 56 new tests for version/comparator.py uncovered branches (_compare_base_versions, _compare_build_numbers, _compare_prerelease, _apply_version_truncation, get_version_info, etc.) comparator.py coverage: 86.3% → 95.0% (+8.7 pp) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
Reviewer's GuideUpdates dependency and formatting configuration, and adds comprehensive new tests to increase coverage for the finder and version comparator modules, especially around async Homebrew paths, error handling, and complex version comparison edge cases. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
🔒 Security Analysis ReportSecurity Analysis ReportGenerated: Fri Apr 3 13:08:42 UTC 2026 Bandit Security ScanSafety Check ResultsPip-Audit Results |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- Several test docstrings refer to concrete source line numbers (e.g.
Line 197:); these will drift as the code evolves, so consider rephrasing them in terms of behavior/branches rather than specific line numbers to keep the tests maintainable. - The async Homebrew tests repeat fairly involved
patch.dict(sys.modules, ...)and_is_async_homebrew_availablepatching; factoring this into a small helper or fixture would reduce duplication and make the intent of each test clearer.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Several test docstrings refer to concrete source line numbers (e.g. `Line 197:`); these will drift as the code evolves, so consider rephrasing them in terms of behavior/branches rather than specific line numbers to keep the tests maintainable.
- The async Homebrew tests repeat fairly involved `patch.dict(sys.modules, ...)` and `_is_async_homebrew_available` patching; factoring this into a small helper or fixture would reduce duplication and make the intent of each test clearer.
## Individual Comments
### Comment 1
<location path="tests/test_comparator_coverage.py" line_range="257-262" />
<code_context>
+ assert len(result_v1) == 3
+ assert max_len == 3
+
+ def test_neither_flag_no_truncation(self):
+ """No flags → no truncation."""
+ v1 = (1, 2, 3, 4)
+ v2 = (1, 2, 3, 5)
+ result_v1, result_v2, max_len = _apply_version_truncation(v1, v2, 4, False, False)
+ assert max_len == 4
+
+
</code_context>
<issue_to_address>
**suggestion (testing):** Extend no-truncation test to also verify that the returned tuples are unchanged
Currently this only checks that `max_len` stays 4 when both flags are False. To also guard against subtle mutations of the inputs, add assertions that `result_v1 == v1` and `result_v2 == v2` so the test verifies the tuples themselves are unchanged.
```suggestion
def test_neither_flag_no_truncation(self):
"""No flags → no truncation."""
v1 = (1, 2, 3, 4)
v2 = (1, 2, 3, 5)
result_v1, result_v2, max_len = _apply_version_truncation(v1, v2, 4, False, False)
assert result_v1 == v1
assert result_v2 == v2
assert max_len == 4
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+257
to
+262
| def test_neither_flag_no_truncation(self): | ||
| """No flags → no truncation.""" | ||
| v1 = (1, 2, 3, 4) | ||
| v2 = (1, 2, 3, 5) | ||
| result_v1, result_v2, max_len = _apply_version_truncation(v1, v2, 4, False, False) | ||
| assert max_len == 4 |
Contributor
There was a problem hiding this comment.
suggestion (testing): Extend no-truncation test to also verify that the returned tuples are unchanged
Currently this only checks that max_len stays 4 when both flags are False. To also guard against subtle mutations of the inputs, add assertions that result_v1 == v1 and result_v2 == v2 so the test verifies the tuples themselves are unchanged.
Suggested change
| def test_neither_flag_no_truncation(self): | |
| """No flags → no truncation.""" | |
| v1 = (1, 2, 3, 4) | |
| v2 = (1, 2, 3, 5) | |
| result_v1, result_v2, max_len = _apply_version_truncation(v1, v2, 4, False, False) | |
| assert max_len == 4 | |
| def test_neither_flag_no_truncation(self): | |
| """No flags → no truncation.""" | |
| v1 = (1, 2, 3, 4) | |
| v2 = (1, 2, 3, 5) | |
| result_v1, result_v2, max_len = _apply_version_truncation(v1, v2, 4, False, False) | |
| assert result_v1 == v1 | |
| assert result_v2 == v2 | |
| assert max_len == 4 |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f1c5a67c07
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+300
to
+302
| result = check_brew_install_candidates([("App1", "1.0")], rate_limit=1) | ||
| # Verify async result is returned when async path succeeded | ||
| assert isinstance(result, list) |
There was a problem hiding this comment.
check_brew_install_candidates exits early when is_homebrew_available() is false, which is the default on non-Darwin CI; in that case this test still passes because it only checks isinstance(result, list). As written, the test can report success without ever executing the async path it claims to cover, so regressions in lines 587–598 of versiontracker/apps/finder.py would go undetected. Patch Homebrew availability (or assert mock_async was called and its return value is preserved) so the targeted branch is actually exercised.
Useful? React with 👍 / 👎.
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
aiohttp>=3.9.0→>=3.13.4— closes 10 CVEs (CVE-2026-34513 through CVE-2026-34525 + GHSA variants), all fixed in 3.13.4[tool.black] line-lengthfrom 88 → 120 to match[tool.ruff] line-length = 120— eliminates formatter conflict where black and ruff disagreedCoverage improvements
apps/finder.pyversion/comparator.pyNew test files
tests/test_finder_new_coverage.py—get_applications,get_applications_from_system_profiler(system/path filtering, DataParsingError),_check_cask_installable_with_cache, batch error escalation (all 4 error types atMAX_ERRORS), async path / fallback,_create_rate_limiter(int/attr/dict inputs),_get_existing_brewserror handling,_should_show_progresstests/test_comparator_coverage.py—_compare_base_versions(< and >),_compare_build_numbers(all None combos),_compare_prerelease(unknown Unicode type γ/δ triggering warning),_process_prerelease_suffix(string suffix),_apply_version_truncation(build-metadata and prerelease truncation),get_version_difference(prerelease/metadata versions, None inputs),get_version_info(None, malformed, NEWER, OUTDATED, empty cases),_handle_empty_version_casesTest plan
🤖 Generated with Claude Code
Summary by Sourcery
Update dependencies and testing to improve security and coverage.
Bug Fixes:
Enhancements:
Tests: