feat: Recommendations v0.9.1 — Homebrew, CodeQL, coverage, async#109
Merged
feat: Recommendations v0.9.1 — Homebrew, CodeQL, coverage, async#109
Conversation
Remove stale versiontracker.rb (v0.7.2) superseded by docdyhr/homebrew-tap Formula/macversiontracker.rb. Rewrite release-homebrew.yml to clone/update the tap repo instead of the deleted root formula. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Enable scanning of GitHub Actions workflow YAML files so CodeQL can detect and auto-close stale actions/missing-workflow-permissions alerts. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…g, profiling Push coverage from 78% to ~80%: - error_handlers.py: 0% -> 100% (30 tests, all 8 handler functions) - async_homebrew.py: 82% -> ~95% (18 tests, network error paths) - config.py: 71% -> ~85% (13 tests, ConfigValidator + brew path) - profiling.py: 53% -> 97% (32 tests, PerformanceProfiler + decorators) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add CaskAutoUpdateChecker(AsyncBatchProcessor) for concurrent cask auto-update checks via the Homebrew JSON API. get_casks_with_auto_updates() now tries async first and falls back to the sequential sync implementation on error. Updated 3 existing tests to mock the async path so they exercise the sync fallback. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
Reviewer's GuideRefactors the Homebrew release workflow to update a dedicated tap repo, enhances CodeQL scanning for Actions workflows, wires an async-based Homebrew auto-update checker with sync fallback, and adds focused tests to close coverage gaps in profiling, error handling, async Homebrew, and config validation logic. Sequence diagram for async Homebrew auto-update check with sync fallbacksequenceDiagram
participant Caller as CallerCode
participant Homebrew as homebrew_module
participant AsyncHomebrew as async_homebrew_module
participant Checker as CaskAutoUpdateChecker
participant BrewAPI as HomebrewAPI
Caller->>Homebrew: get_casks_with_auto_updates(cask_names)
activate Homebrew
Homebrew->>AsyncHomebrew: import async_get_casks_with_auto_updates
Note over Homebrew,AsyncHomebrew: If import or async execution fails,
Note over Homebrew,AsyncHomebrew: an exception is raised and caught in homebrew
Homebrew->>AsyncHomebrew: async_get_casks_with_auto_updates(cask_names)
activate AsyncHomebrew
AsyncHomebrew->>AsyncHomebrew: validate cask_names
AsyncHomebrew->>Checker: new CaskAutoUpdateChecker(batch_size, max_concurrency, rate_limit)
activate Checker
AsyncHomebrew->>Checker: process_all_async(cask_names)
loop per_batch
Checker->>BrewAPI: fetch_cask_info(cask_name, use_cache)
alt auto_updates flag true
BrewAPI-->>Checker: cask_info(auto_updates true)
Checker-->>AsyncHomebrew: cask_name
else caveats mention auto update
BrewAPI-->>Checker: cask_info(caveats text)
Checker-->>AsyncHomebrew: cask_name
else no auto update indicators
BrewAPI-->>Checker: cask_info(no auto update)
Checker-->>AsyncHomebrew: None
end
alt NetworkError or TimeoutError or HomebrewError
BrewAPI-->>Checker: error
Checker-->>Checker: log error
Checker-->>AsyncHomebrew: None
end
end
AsyncHomebrew-->>Homebrew: list_of_names_filtered
deactivate Checker
deactivate AsyncHomebrew
Homebrew-->>Caller: list_of_names_filtered
deactivate Homebrew
rect rgb(255,240,240)
Note over Caller,Homebrew: Sync fallback path
Caller->>Homebrew: get_casks_with_auto_updates(cask_names)
activate Homebrew
Homebrew->>AsyncHomebrew: import async_get_casks_with_auto_updates
AsyncHomebrew-->>Homebrew: raises Exception
Homebrew->>Homebrew: log warning and fall back to sync
loop per_cask
Homebrew->>BrewAPI: has_auto_updates(cask_name)
alt has_auto_updates true
BrewAPI-->>Homebrew: true
Homebrew-->>Homebrew: append cask_name
else has_auto_updates false
BrewAPI-->>Homebrew: false
end
end
Homebrew-->>Caller: auto_update_casks_sync
deactivate Homebrew
end
Updated class diagram for async Homebrew auto-update componentsclassDiagram
class AsyncBatchProcessor~T_in,T_out~ {
+int batch_size
+int max_concurrency
+float rate_limit
+__init__(batch_size, max_concurrency, rate_limit)
+process_all_async(items) T_out[]
+process_item(item) T_out
+handle_error(item, error) T_out
}
class CaskAutoUpdateChecker {
+int batch_size
+int max_concurrency
+float rate_limit
+bool use_cache
+list~str~ AUTO_UPDATE_PATTERNS
+__init__(batch_size=10, max_concurrency=5, rate_limit=1.0, use_cache=True)
+process_item(cask_name) str | None
+handle_error(item, error) str | None
}
class async_homebrew_module {
+async_get_casks_with_auto_updates(cask_names, rate_limit=1.0) list~str~
}
class homebrew_module {
+get_casks_with_auto_updates(cask_names) list~str~
}
class HomebrewAPI {
+fetch_cask_info(cask_name, use_cache) dict
+has_auto_updates(cask_name) bool
}
class Errors {
<<exception types>>
NetworkError
TimeoutError
HomebrewError
}
AsyncBatchProcessor <|-- CaskAutoUpdateChecker
async_homebrew_module ..> CaskAutoUpdateChecker : creates
async_homebrew_module ..> HomebrewAPI : uses fetch_cask_info
async_homebrew_module ..> Errors : catches
homebrew_module ..> async_homebrew_module : imports and calls
homebrew_module ..> HomebrewAPI : uses has_auto_updates (sync fallback)
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
🔒 Security Analysis ReportSecurity Analysis ReportGenerated: Mon Mar 2 22:55:43 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 3 issues, and left some high level feedback:
- In
release-homebrew.yml, theupdate-homebrewjob is declared withpermissions: contents: read, but thePush updated formula to tapstep performsgit push; this will requirecontents: writeor the push will fail under fine-grained workflow permissions. - In
async_get_casks_with_auto_updates,max_concurrencyis computed asint(5 / rate_limit), which can become 0 forrate_limit > 5; consider enforcing a minimum of 1 (e.g.,max(1, int(...))) to avoid invalid concurrency settings.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `release-homebrew.yml`, the `update-homebrew` job is declared with `permissions: contents: read`, but the `Push updated formula to tap` step performs `git push`; this will require `contents: write` or the push will fail under fine-grained workflow permissions.
- In `async_get_casks_with_auto_updates`, `max_concurrency` is computed as `int(5 / rate_limit)`, which can become 0 for `rate_limit > 5`; consider enforcing a minimum of 1 (e.g., `max(1, int(...))`) to avoid invalid concurrency settings.
## Individual Comments
### Comment 1
<location path=".github/workflows/release-homebrew.yml" line_range="71" />
<code_context>
+ '"${FORMULA_PATH}" > formula.tmp
</code_context>
<issue_to_address>
**suggestion (bug_risk):** The awk update no longer validates that the URL and sha256 lines were actually updated, which can silently leave the formula stale.
Previously, the script tracked `updated_url`/`updated_sha` and exited non-zero if either wasn’t changed, so regex/format issues surfaced as failures. The new awk version always exits 0, even if nothing matched. Please add an `END` check (or similar guard) so we fail when no URL or sha256 was updated, avoiding silently stale or partially updated formulas.
</issue_to_address>
### Comment 2
<location path="versiontracker/async_homebrew.py" line_range="464-473" />
<code_context>
+
+
+@async_to_sync
+async def async_get_casks_with_auto_updates(cask_names: list[str], rate_limit: float = 1.0) -> list[str]:
+ """Get casks with auto-updates enabled (async version).
+
+ Args:
+ cask_names: List of cask names to check
+ rate_limit: Rate limit between requests
+
+ Returns:
+ List of cask names that have auto-updates enabled
+ """
+ if not cask_names:
+ return []
+
+ processor = CaskAutoUpdateChecker(
+ batch_size=10,
+ max_concurrency=int(5 / rate_limit),
+ rate_limit=rate_limit,
+ )
</code_context>
<issue_to_address>
**issue (bug_risk):** `max_concurrency=int(5 / rate_limit)` can become zero or negative (and raises on rate_limit=0), which is likely invalid for the batch processor.
If `rate_limit` is 0.0 this will raise `ZeroDivisionError`, and for `rate_limit > 5.0` it yields 0, which is likely invalid for `max_concurrency`. Consider enforcing `rate_limit > 0` and clamping `max_concurrency` to at least 1, e.g.:
```python
if rate_limit <= 0:
raise ValueError("rate_limit must be > 0")
max_concurrency = max(1, int(5 / rate_limit))
```
Alternatively, you could decouple `max_concurrency` from `rate_limit` and let callers configure them separately.
</issue_to_address>
### Comment 3
<location path="tests/test_config_validation_coverage.py" line_range="102-111" />
<code_context>
+ @patch.object(ConfigLoader, "detect_brew_path", return_value="/usr/local/bin/brew")
+ @patch.object(ConfigLoader, "load_from_file")
+ @patch.object(ConfigLoader, "load_from_env")
+ def test_get_nonexistent_nested_key_raises_without_default(self, _env, _file, _brew):
+ """Config.get('ui.nonexistent') raises KeyError when no default and default is None."""
+ config = Config()
+ # When default is None (the default parameter), and key is not found,
+ # it returns None per the code logic (default is not None check fails
+ # because default IS None)
+ # Actually looking at the code: if default is not None: return default
+ # else: raise KeyError
+ with pytest.raises(KeyError, match="not found"):
+ config.get("ui.nonexistent_key")
+
+ @patch.object(ConfigLoader, "detect_brew_path", return_value="/usr/local/bin/brew")
</code_context>
<issue_to_address>
**nitpick:** Clarify or remove outdated inline comment about Config.get() behavior.
The inline comment still reflects the old behavior (returning `None` when `default` is `None`), while this test correctly asserts the current behavior (raising `KeyError`). Please update the comment to match the current logic or remove it to avoid confusing future readers.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| echo "Updated formula preview:" | ||
| sed -n '1,20p' versiontracker.rb | ||
| ' "${FORMULA_PATH}" > formula.tmp | ||
| mv formula.tmp "${FORMULA_PATH}" |
Contributor
There was a problem hiding this comment.
suggestion (bug_risk): The awk update no longer validates that the URL and sha256 lines were actually updated, which can silently leave the formula stale.
Previously, the script tracked updated_url/updated_sha and exited non-zero if either wasn’t changed, so regex/format issues surfaced as failures. The new awk version always exits 0, even if nothing matched. Please add an END check (or similar guard) so we fail when no URL or sha256 was updated, avoiding silently stale or partially updated formulas.
Comment on lines
+464
to
+473
| async def async_get_casks_with_auto_updates(cask_names: list[str], rate_limit: float = 1.0) -> list[str]: | ||
| """Get casks with auto-updates enabled (async version). | ||
|
|
||
| Args: | ||
| cask_names: List of cask names to check | ||
| rate_limit: Rate limit between requests | ||
|
|
||
| Returns: | ||
| List of cask names that have auto-updates enabled | ||
| """ |
Contributor
There was a problem hiding this comment.
issue (bug_risk): max_concurrency=int(5 / rate_limit) can become zero or negative (and raises on rate_limit=0), which is likely invalid for the batch processor.
If rate_limit is 0.0 this will raise ZeroDivisionError, and for rate_limit > 5.0 it yields 0, which is likely invalid for max_concurrency. Consider enforcing rate_limit > 0 and clamping max_concurrency to at least 1, e.g.:
if rate_limit <= 0:
raise ValueError("rate_limit must be > 0")
max_concurrency = max(1, int(5 / rate_limit))Alternatively, you could decouple max_concurrency from rate_limit and let callers configure them separately.
Comment on lines
+102
to
+111
| def test_get_nonexistent_nested_key_raises_without_default(self, _env, _file, _brew): | ||
| """Config.get('ui.nonexistent') raises KeyError when no default and default is None.""" | ||
| config = Config() | ||
| # When default is None (the default parameter), and key is not found, | ||
| # it returns None per the code logic (default is not None check fails | ||
| # because default IS None) | ||
| # Actually looking at the code: if default is not None: return default | ||
| # else: raise KeyError | ||
| with pytest.raises(KeyError, match="not found"): | ||
| config.get("ui.nonexistent_key") |
Contributor
There was a problem hiding this comment.
nitpick: Clarify or remove outdated inline comment about Config.get() behavior.
The inline comment still reflects the old behavior (returning None when default is None), while this test correctly asserts the current behavior (raising KeyError). Please update the comment to match the current logic or remove it to avoid confusing future readers.
![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: 7565078627
ℹ️ 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
+26
to
+27
| permissions: | ||
| contents: read |
There was a problem hiding this comment.
In .github/workflows/release-homebrew.yml, this job is configured with permissions: contents: read but later commits and runs git push origin main to update the tap, so the push path cannot succeed during release/dispatch runs using secrets.GITHUB_TOKEN. As written, the automation will stop at the final push step and never publish formula updates unless the workflow uses a token with write access for that repository.
Useful? React with 👍 / 👎.
capsys capture was unreliable in CI when other tests modified
builtins.print. Switch to patch("builtins.print") for the two
simple assertion tests.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
🔒 Security Analysis ReportSecurity Analysis ReportGenerated: Mon Mar 2 23:03:41 UTC 2026 Bandit Security ScanSafety Check ResultsPip-Audit Results |
capsys stdout capture is unreliable in CI when other tests
interfere with builtins.print. Use patch("builtins.print")
consistently for all print_report assertion tests.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
🔒 Security Analysis ReportSecurity Analysis ReportGenerated: Mon Mar 2 23:09:43 UTC 2026 Bandit Security ScanSafety Check ResultsPip-Audit Results |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
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
versiontracker.rb(v0.7.2), rewriterelease-homebrew.ymlto update the tap repo (docdyhr/homebrew-tap) instead of the deleted root formulaactionslanguage tocodeql-analysis.ymlto detect workflow permission issues and auto-close 15 stale alertserror_handlers.py(0%→100%),profiling.py(53%→97%),async_homebrew.pyerror paths,config.pyvalidation paths — overall coverage 78%→~80%CaskAutoUpdateChecker(AsyncBatchProcessor)+ wireget_casks_with_auto_updates()to try async first with sync fallbackTest plan
ruff checkandruff formatcleanmypy versiontrackerclean (only pre-existing errors in experimental/)brew install docdyhr/tap/macversiontrackervalidated locally (v0.9.0)🤖 Generated with Claude Code
Summary by Sourcery
Update Homebrew release workflow to target the tap repository, introduce async-based Homebrew auto-update checks with synchronous fallback, expand security scanning, and significantly increase test coverage for error handling, profiling, async Homebrew logic, and config validation.
New Features:
Enhancements:
Build:
CI:
Tests: