ci: add ruff lint with F821/F811 rules - #44
Conversation
Adds a ruff lint step to the test workflow with two rules: F821 (undefined name) and F811 (redefined unused name). Python won't report an undefined name until that code path executes, so a missing import or typo can ship undetected. F821 + F811 catch that statically with zero false positives and no style noise. Lint runs before pytest so CI fails fast on missing imports. Also fixes a pre-existing F821 in core/analyzer.py: the analyze() function called tracker.add_prior_usage() without defining tracker locally; replaced with get_global_tracker() to match the pattern used elsewhere in the file. Refs #16 (item 2).
Manual verification
|
Local test resultsRan the configured ruff rules locally on Windows from this branch. Commands run: Output: Outcome:
|
ar7casper
left a comment
There was a problem hiding this comment.
Tiny + finds a real bug — exactly what a tooling PR should look like. The rationale comment in pyproject.toml ("zero false positives, no style noise") is the right framing and will protect against future "let's add a few more rules" creep.
Two non-blocking suggestions before merge:
1. Move lint to its own job. Currently the ruff check . step lives inside python-tests, which is matrix'd over ubuntu/macos/windows. Ruff is platform-independent, so you're paying for 3 identical lint runs per PR. A separate single-OS lint job (~5 LOC) saves 2× CI minutes, parallelizes with the matrix tests instead of serializing inside them, and gives clearer "lint failed" vs "tests failed" feedback. Skip the full pip install ".[dev]" while you're at it — ruff doesn't execute the code, just parses, so it doesn't need the project's runtime deps.
2. Consider adding F823 to the rule set. F823 (local variable referenced before assignment) catches a real-bug subset of F821 in local-scope flow analysis. Same zero-FP profile as F821/F811, no style noise. F841 (unused var) and F401 (unused import) would be useful but introduce conditional-import / __init__.py re-export noise — your current selection rightly excludes them.
Neither is a merge blocker. (1) is the higher-value change; (2) is a "while you're in there."
- Extracts ruff check into a standalone `lint` job on ubuntu-latest, parallel to the matrix jobs instead of running 3x inside them - Installs only ruff (no runtime deps needed for static parsing) - Adds F823 (local var referenced before assignment) to rule set Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
joshbouncesecurity
left a comment
There was a problem hiding this comment.
Thanks for the thorough review! Both suggestions addressed in the latest commit:
-
Lint job — extracted
ruff checkinto a standalonelintjob onubuntu-latestonly. Installs justruffdirectly (no.[dev]), so it runs in parallel with the matrix jobs rather than 3× inside them. -
F823 — added to the rule set alongside F821/F811. Same zero-FP profile as you noted.
Summary
Adds a ruff lint step to the test workflow with two rules:
F821(undefined name) andF811(redefined unused name). Python won't report an undefined name until that code path executes, so a missing import can ship undetected. F821 + F811 catch that statically with zero false positives and no style noise.Configured to run before pytest so CI fails fast on missing imports.
Verified clean against
upstream/masterbaseline after fixing one pre-existing violation:libs/openant-core/core/analyzer.py:393—analyze()calledtracker.add_prior_usage(...)without definingtrackerlocally. Replaced withget_global_tracker().add_prior_usage(...)to match the pattern already used elsewhere in the same file (e.g. line 489). This is a real bug — the line wouldNameErrorat runtime whenever resuming with non-zero prior token usage.Addresses item 2 from #16 (does not close the issue).
Test plan
ruff check .exits 0 on a clean checkout (verified locally).