Repository navigation
Clear CodeQL quality findings and guard repeated imports - #313
Open
chris-colinsky wants to merge 1 commit into
Open
chris-colinsky wants to merge 1 commit into
chris-colinsky wants to merge 1 commit into
Conversation
Ruff has no rule for a function-local import that repeats a module-level one, and CodeQL's only covers plain `import X` it can resolve. The new script flags any function-local import whose bound name and target match a module-level runtime import, ignoring TYPE_CHECKING-only imports. It runs as a pre-commit hook and in both ci.yml and release.yml beside the conformance manifest check.
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The new guard can reject necessary imports and recommend deletions that change runtime behavior.
Review effort: Balanced
Findings: 4
Open (4)
What changed in this PR
Cleans up CodeQL quality findings and adds a repeated-import guard to development and release checks.
Changes:
- Removes redundant function-local imports from tests.
- Unquotes casts whose types are already imported at runtime.
- Adds repeated-import checks to pre-commit, CI, and release workflows.
| File | Description |
|---|---|
tests/unit/test_prompts.py |
Removes redundant prompt and datetime imports. |
tests/unit/test_prompts_langfuse.py |
Removes a repeated manager import. |
tests/unit/test_observability_otel.py |
Removes redundant observability test imports. |
tests/unit/test_observability_metadata.py |
Removes repeated validation imports. |
tests/unit/test_llm_provider.py |
Removes redundant provider test imports. |
tests/unit/test_langfuse_sdk_internals.py |
Removes a repeated re import. |
tests/unit/test_langfuse_provider_fake.py |
Removes repeated tracing imports. |
tests/conformance/test_observability.py |
Removes redundant conformance imports. |
tests/conformance/test_checkpoint.py |
Unquotes Checkpointer casts. |
src/openarmature/llm/providers/openai.py |
Unquotes the FinishReason cast. |
scripts/check_repeated_imports.py |
Adds the repeated-import checker. |
.pre-commit-config.yaml |
Registers the checker as a hook. |
.github/workflows/release.yml |
Adds release-time import checking. |
.github/workflows/ci.yml |
Adds CI import checking. |
.github/codeql/codeql-config.yml |
Updates comments recommending unquoted runtime casts. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+60
to
+61
| elif isinstance(stmt, ast.If) and not _is_type_checking_guard(stmt): | ||
| found |= _module_bindings(stmt.body) | _module_bindings(stmt.orelse) |
Comment on lines
+60
to
+66
| elif isinstance(stmt, ast.If) and not _is_type_checking_guard(stmt): | ||
| found |= _module_bindings(stmt.body) | _module_bindings(stmt.orelse) | ||
| elif isinstance(stmt, ast.Try): | ||
| for block in (stmt.body, stmt.orelse, stmt.finalbody): | ||
| found |= _module_bindings(block) | ||
| for handler in stmt.handlers: | ||
| found |= _module_bindings(handler.body) |
Comment on lines
+78
to
+87
| # Keyed on the import node, so an import inside a nested function is | ||
| # reported once against its innermost function rather than once per | ||
| # enclosing one. ast.walk is breadth-first, so the innermost function is | ||
| # the last to claim each node. | ||
| owner: dict[ast.AST, str] = {} | ||
| for fn in ast.walk(tree): | ||
| if isinstance(fn, (ast.FunctionDef, ast.AsyncFunctionDef)): | ||
| for node in ast.walk(fn): | ||
| if isinstance(node, (ast.Import, ast.ImportFrom)): | ||
| owner[node] = fn.name |
| for node, fn_name in sorted(owner.items(), key=lambda item: item[0].lineno): | ||
| assert isinstance(node, (ast.Import, ast.ImportFrom)) | ||
| for binding in _bindings(node): | ||
| if binding in module_level: |
This branch has not been deployed
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
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
Starts the 0.18.0 cycle by clearing the CodeQL code-quality findings that are real, and adding a check so the most common kind cannot come back.
Cleanup
py/repeated-importfindings: function-local imports that rebind a name the module already imports.py/unused-importfindings by unquotingcast("X", v)tocast(X, v)whereXis already imported at runtime. Thecodeql-config.ymlcomment now recommends the unquoted form.from X import Yrepeats, and modules it cannot resolve).Guard
scripts/check_repeated_imports.py. It flags a function-local import whose bound name and target match a module-level runtime import. Imports underif TYPE_CHECKING:are ignored, since re-importing those inside a function is the normal pattern.ci.ymlandrelease.yml, next to the conformance manifest check.The single commit carries both parts; its message describes only the guard.
Remaining findings
The 44 findings left are deliberate and will be dismissed in the Code Quality UI, with the reasons already recorded in
codeql-config.yml: 41 ineffectual-statement (Protocol...stubs that pyright strict requires, and one neededawait), 1 unused-import used as a subscripted base class, 1 unreachable-statement insidepytest.raises, and 1 signature mismatch from a deliberate test double.Testing
pytest: 2318 passed, 498 skippedfrom, nested, try/except bindings) and must ignore (TYPE_CHECKING, aliased imports). Re-adding a repeated import to a real test file turns the full-tree run red.