ci: add a ruff (pyflakes) lint gate - #450
Open
ADanMan wants to merge 1 commit into
Open
Conversation
No static-analysis gate existed for the Python side — CI ran pytest only, nothing checked for dead imports, unused variables, undefined names, or syntax errors before review. Add ruff, scoped deliberately narrow to start (owner call, per issue andrewyng#34): select = ["E9", "F"] — syntax errors + pyflakes. No style rules, no formatter, nothing to bikeshed; just the checks with essentially zero false positives that catch real latent bugs. target-version is pinned to py312 (matching what CI actually runs) rather than derived from the 3.10 requires-python floor, which would otherwise flag `ExceptionGroup` (3.11+) as an undefined name in test_mcp_connectors.py. Fixed what it found (33 violations, all pre-existing): - 28 dead imports across coworker/ and tests/. Two (SHELL_TOOL, WRITE_TOOLS in permissions.py) are an intentional back-compat re-export per the existing comment on that import block — left in place with `# noqa: F401` rather than removed. - 2 redundant local `from ..engine import ApprovalOutcome` in manager.py that shadowed the already-imported module-level name, which is what made that module-level import itself look unused. - 1 unused local variable in a test. This is the Python half of andrewyng#34; the GUI's eslint/tsc half looks already covered by andrewyng#419 (typecheck in gui-unit). Full suite still green: 1112 passed, 1 skipped.
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.
Fixes #34 (the Python half — CI has no linter/type-checker at all today; the GUI's eslint/tsc half looks already covered by #419's typecheck-in-gui-unit).
What
Adds a
lintjob to.github/workflows/ci.ymlrunningruff check coworker tests, and a[tool.ruff]config inpyproject.toml.Deliberately narrow rule selection:
select = ["E9", "F"]— syntax errors + pyflakes only. No style rules, no formatter, nothing to bikeshed about. This is exactly the "cheapest way to catch a whole class of latent bugs (dead imports, undefined names)" the issue asks for, without opening a scope debate about house style.target-version = "py312"is pinned to match what CI actually runs (rather than derived from the>=3.10requires-pythonfloor), which otherwise makes ruff flagExceptionGroup(stdlib-builtin only since 3.11) as an undefined name intest_mcp_connectors.py— a false positive, not a real 3.10 incompatibility, since CI only runs 3.12.The issue suggested "start report-only, then ratchet to blocking." With this narrow a rule set the repo was already 33 violations away from clean, so I fixed those and shipped it blocking from the start rather than adding a report-only phase that would just get skipped over.
What it found and fixed (33 pre-existing violations)
coworker/andtests/— plain unused-import cleanup,ruff --fixafter manual review of each hit (see below).manager.py:approval_outcome()and_scheduled_approver()each had their ownfrom ..engine import ApprovalOutcome, shadowing the name already imported at module level (line 40). That shadowing is why ruff flagged the module-level import as unused — the bug was the local duplicates, not the import itself, so I removed the two local ones and kept the module-level one.events = _collect(...)intest_plan_mode.py, result never read).One deliberate non-removal:
coworker/permissions.pyre-exportsSHELL_TOOLandWRITE_TOOLSfrom.riskwith an explicit comment —# re-exported for back-compat (manager.py imports WRITE_TOOLS). Ruff can't see that manager.py depends on this re-export path, so it flagged both as unused-in-file. I left them in place and added# noqa: F401rather than deleting what's an intentional public re-export.Testing
ruff check coworker tests→ clean.1112 passed, 1 skipped(same venv/extras setup as fix(providers): mark Qwen3.8 Max Preview as vision-capable #449:uv venv --python 3.11+pip install -e ".[dev,bedrock,messaging]").ruff --fix— the permissions.py case above is why that mattered.