Clear CodeQL quality findings and guard repeated imports

.github/codeql/codeql-config.yml

@@ -28,15 +28,14 @@ name: "openarmature-python CodeQL config"
 query-filters:
   - exclude:
       id: py/ineffectual-statement
-  # ``py/unused-import`` produces false positives on three patterns
-  # this codebase relies on:
+  # ``py/unused-import`` produces false positives on patterns this
+  # codebase relies on:
   #
-  # - Forward-reference casts: ``cast("FinishReason", x)`` /
-  #   ``cast("Checkpointer", capturing)``. CodeQL doesn't look
-  #   inside the string argument; pyright's strict mode does, AND
-  #   raises ``reportUndefinedVariable`` if the name isn't in scope
-  #   (verified empirically — removing ``FinishReason`` from
-  #   ``openai.py``'s import yields the pyright error).
+  # - Quoted casts: ``cast("X", value)``. CodeQL doesn't look inside
+  #   the string argument; pyright's strict mode does, and raises
+  #   ``reportUndefinedVariable`` if the name isn't in scope. Where the
+  #   name is already imported at runtime the quotes buy nothing, so
+  #   prefer ``cast(X, value)``, which both tools see as a use.
   # - Subscripted base classes: ``class _TracingFanOutNode(
   #   FanOutNode[State, State]):``. The base class IS used; the
   #   generic subscription happens at class-definition time.

.github/workflows/ci.yml

@@ -65,6 +65,12 @@ jobs:
         # entry quietly serves wrong data to the docs site.
         run: uv run python scripts/check_conformance_manifest.py
 
+      - name: Reject repeated imports
+        # A function-local import of something the module already imports.
+        # Ruff has no rule for it and CodeQL's only covers plain `import X`.
+        # Runs here as well as in pre-commit because a hook can be skipped.
+        run: uv run python scripts/check_repeated_imports.py
+
       - name: Lint (ruff check)
         run: uv run ruff check .
 

.github/workflows/release.yml

@@ -91,6 +91,10 @@ jobs:
         # bypassed PR review still fails before publishing.
         run: uv run python scripts/check_conformance_manifest.py
 
+      - name: Reject repeated imports
+        # Mirrors the equivalent step in ci.yml.
+        run: uv run python scripts/check_repeated_imports.py
+
       - name: Lint (ruff check)
         run: uv run ruff check .
 

.pre-commit-config.yaml

@@ -28,6 +28,11 @@ repos:
 
   - repo: local
     hooks:
+      - id: repeated-imports
+        name: repeated imports
+        entry: uv run python scripts/check_repeated_imports.py
+        language: system
+        types: [python]
       - id: pyright
         name: pyright
         entry: uv run pyright

scripts/check_repeated_imports.py

@@ -0,0 +1,160 @@
+"""Fail when a function re-imports something its module already imports.
+
+A function-local ``import asyncio`` in a module that already has
+``import asyncio`` at the top does nothing: the name is bound either way.
+CodeQL reports it as ``py/repeated-import`` and ruff has no rule for it, so
+this check runs where ruff does, at commit time and in CI.
+
+Function-local imports are not flagged in general; many are deliberate. Only
+an import whose binding and target both match a module-level runtime import is
+a repeat. That is broader than CodeQL's rule, which considers plain
+``import X`` only and only when it can resolve ``X`` to a module, so it misses
+``from X import Y`` repeats and some stdlib modules entirely.
+
+Usage:
+    python scripts/check_repeated_imports.py [FILE ...]
+
+With no arguments, checks every Python file under the source, test, example and
+script trees. Exits 1 and lists each repeat when any are found.
+"""
+
+from __future__ import annotations
+
+import ast
+import sys
+from collections.abc import Iterator
+from pathlib import Path
+
+ROOTS = ("src", "tests", "examples", "scripts")
+
+_SCOPES = (ast.FunctionDef, ast.AsyncFunctionDef, ast.ClassDef, ast.Lambda)
+
+# (bound name, what it is bound to). `import a.b` binds `a` but loads `a.b`, so
+# the target is the full dotted path; a `from` import's target is the module,
+# its relative level, and the member.
+Binding = tuple[str, tuple[object, ...]]
+
+
+def _bindings(node: ast.Import | ast.ImportFrom) -> list[Binding]:
+    if isinstance(node, ast.Import):
+        return [(alias.asname or alias.name.split(".")[0], ("import", alias.name)) for alias in node.names]
+    return [
+        (alias.asname or alias.name, ("from", node.level, node.module, alias.name))
+        for alias in node.names
+        if alias.name != "*"
+    ]
+
+
+def _is_type_checking_guard(node: ast.If) -> bool:
+    test = node.test
+    if isinstance(test, ast.Name):
+        return test.id == "TYPE_CHECKING"
+    return isinstance(test, ast.Attribute) and test.attr == "TYPE_CHECKING"
+
+
+def _module_bindings(body: list[ast.stmt]) -> set[Binding]:
+    # Only bindings every runtime path makes, so deleting a function-local
+    # import the guard flags can never leave the name undefined. An import
+    # under `if TYPE_CHECKING:` binds nothing at runtime, and a function
+    # re-importing the same name is the standard way to use it there without
+    # a circular import, so it is not a repeat.
+    found: set[Binding] = set()
+    for stmt in body:
+        if isinstance(stmt, (ast.Import, ast.ImportFrom)):
+            found.update(_bindings(stmt))
+        elif isinstance(stmt, ast.If) and _is_type_checking_guard(stmt):
+            found |= _module_bindings(stmt.orelse)
+        elif isinstance(stmt, ast.If):
+            found |= _module_bindings(stmt.body) & _module_bindings(stmt.orelse)
+        elif isinstance(stmt, ast.Try):
+            paths = [_module_bindings(stmt.body + stmt.orelse)]
+            paths += [_module_bindings(handler.body) for handler in stmt.handlers]
+            found |= paths[0].intersection(*paths[1:]) | _module_bindings(stmt.finalbody)
+    return found
+
+
+def _own_scope(fn: ast.FunctionDef | ast.AsyncFunctionDef) -> Iterator[ast.AST]:
+    # Stops at nested scopes: an import in a nested class body sets a class
+    # attribute, and nested functions are checked as functions of their own.
+    stack: list[ast.AST] = list(fn.body)
+    while stack:
+        node = stack.pop()
+        yield node
+        if not isinstance(node, _SCOPES):
+            stack.extend(ast.iter_child_nodes(node))
+
+
+def _scope_names(fn: ast.FunctionDef | ast.AsyncFunctionDef, skip: set[Binding]) -> set[str]:
+    # Every name the function's own scope binds, except through imports whose
+    # binding is in `skip`. Comprehension targets are included; that only
+    # makes the guard report less.
+    args = fn.args
+    params = [*args.posonlyargs, *args.args, *args.kwonlyargs, args.vararg, args.kwarg]
+    names = {a.arg for a in params if a is not None}
+    for node in _own_scope(fn):
+        if isinstance(node, ast.Name) and isinstance(node.ctx, (ast.Store, ast.Del)):
+            names.add(node.id)
+        elif isinstance(node, (ast.FunctionDef, ast.AsyncFunctionDef, ast.ClassDef)):
+            names.add(node.name)
+        elif isinstance(node, (ast.Import, ast.ImportFrom)):
+            names.update(name for name, target in _bindings(node) if (name, target) not in skip)
+        elif isinstance(node, (ast.ExceptHandler, ast.MatchAs, ast.MatchStar)) and node.name:
+            names.add(node.name)
+        elif isinstance(node, ast.MatchMapping) and node.rest:
+            names.add(node.rest)
+    return names
+
+
+def check(path: Path) -> list[str]:
+    try:
+        tree = ast.parse(path.read_text(), filename=str(path))
+    except SyntaxError:
+        return []  # not ours to report; the interpreter and ruff will
+    module_level = _module_bindings(tree.body)
+    if not module_level:
+        return []
+    parents = {child: node for node in ast.walk(tree) for child in ast.iter_child_nodes(node)}
+    problems: list[tuple[int, str]] = []
+    for fn in ast.walk(tree):
+        if not isinstance(fn, (ast.FunctionDef, ast.AsyncFunctionDef)):
+            continue
+        # A name bound anywhere else in this function, or in an enclosing
+        # function, would not fall through to the module import if this
+        # import were deleted. Class scopes are skipped by name resolution.
+        shadowed = _scope_names(fn, module_level)
+        outer = parents.get(fn)
+        while outer is not None:
+            if isinstance(outer, (ast.FunctionDef, ast.AsyncFunctionDef)):
+                shadowed |= _scope_names(outer, set())
+            outer = parents.get(outer)
+        for node in _own_scope(fn):
+            if not isinstance(node, (ast.Import, ast.ImportFrom)):
+                continue
+            for binding in _bindings(node):
+                if binding in module_level and binding[0] not in shadowed:
+                    problems.append(
+                        (
+                            node.lineno,
+                            f"{path}:{node.lineno}: `{binding[0]}` is already imported "
+                            f"at module level; this import in `{fn.name}` does nothing",
+                        )
+                    )
+    return [message for _, message in sorted(problems)]
+
+
+def main(argv: list[str]) -> int:
+    if argv:
+        files = [Path(a) for a in argv if a.endswith(".py")]
+    else:
+        files = sorted(p for root in ROOTS if Path(root).is_dir() for p in Path(root).rglob("*.py"))
+    problems = [line for f in files for line in check(f)]
+    for line in problems:
+        print(line)
+    if problems:
+        print(f"\n{len(problems)} repeated import(s). Delete the function-local import.")
+        return 1
+    return 0
+
+
+if __name__ == "__main__":
+    sys.exit(main(sys.argv[1:]))

src/openarmature/llm/providers/openai.py

@@ -1316,7 +1316,7 @@ def _parse_response(
             finish_reason = "tool_calls"
         if finish_reason not in {"stop", "length", "tool_calls", "content_filter", "error"}:
             finish_reason = "error"
-        finish_reason_typed = cast("FinishReason", finish_reason)
+        finish_reason_typed = cast(FinishReason, finish_reason)
 
         # Build the assistant message. Tool calls under
         # finish_reason="error" may carry malformed argument JSON per

tests/conformance/test_checkpoint.py

@@ -634,7 +634,7 @@ async def _run_seeded_resume_case(spec: Mapping[str, Any], *, top_level: Mapping
     subgraphs = _build_subgraphs_for(spec, top_level, leaf_value_recorder=leaf_values)
     built = build_graph(spec, subgraphs=subgraphs, trace=[], leaf_value_recorder=leaf_values)
     checkpointer = InMemoryCheckpointer()
-    built.builder.with_checkpointer(cast("Checkpointer", checkpointer))
+    built.builder.with_checkpointer(cast(Checkpointer, checkpointer))
     compiled = built.builder.compile()
 
     seeded_block = cast("Mapping[str, Any]", spec["seeded_record"])
@@ -710,7 +710,7 @@ async def _run_one_case(spec: Mapping[str, Any], *, top_level: Mapping[str, Any]
     if declared_schema_version is not None:
         built.state_cls.schema_version = str(declared_schema_version)
 
-    builder.with_checkpointer(cast("Checkpointer", capturing))
+    builder.with_checkpointer(cast(Checkpointer, capturing))
     compiled = builder.compile()
 
     # Per proposal 0028: ``runtime_state_subclass`` constructs a Python
@@ -1676,7 +1676,7 @@ async def _seed_and_resume(
     subgraphs = _build_subgraphs_for(outer_case, top_level, leaf_value_recorder=leaf_values)
     built = build_graph(outer_case, subgraphs=subgraphs, trace=[], leaf_value_recorder=leaf_values)
     cp = InMemoryCheckpointer()
-    built.builder.with_checkpointer(cast("Checkpointer", cp))
+    built.builder.with_checkpointer(cast(Checkpointer, cp))
     compiled = built.builder.compile()
     inv = "seeded-cov"
     await cp.save(inv, _build_seeded_record(seeded, inv))

tests/conformance/test_observability.py

@@ -7383,8 +7383,6 @@ async def _run_orphan_fallback_case(case: Mapping[str, Any], spec: Mapping[str,
 
     from openarmature.llm import OpenAIProvider, UserMessage  # noqa: PLC0415
 
-    from .adapter import build_graph  # noqa: PLC0415
-
     subgraph_specs = _merged_subgraph_specs(case, spec)
     mock_bodies = _mock_bodies_by_request_content(case, subgraph_specs)
 
@@ -8060,7 +8058,6 @@ async def _run_llm_cache_fixture_case(case: Mapping[str, Any]) -> None:
     from openarmature.graph import END, GraphBuilder
     from openarmature.llm import OpenAIProvider, UserMessage
     from openarmature.llm.response import Response
-    from openarmature.observability.otel import OTelObserver
 
     from .adapter import build_state_cls
 

tests/unit/test_langfuse_provider_fake.py

@@ -403,9 +403,6 @@ def test_the_classifier_agrees_with_a_real_langfuse_client() -> None:
     # on the path the fixture actually drives, while this file's other tests -- all
     # driving the fake -- passed. A double must not be the only witness for the
     # behaviour it exists to model, so drive the real SDK here.
-    from opentelemetry.sdk.trace import TracerProvider
-    from opentelemetry.sdk.trace.export import SimpleSpanProcessor
-    from opentelemetry.sdk.trace.export.in_memory_span_exporter import InMemorySpanExporter
 
     from tests.conformance.harness.langfuse_real_client import (
         CONFORMANCE_HOST,

tests/unit/test_langfuse_sdk_internals.py

@@ -167,7 +167,6 @@ def test_the_installed_version_is_within_the_declared_range() -> None:
     # Non-vacuity for everything above: the checks are only meaningful against a
     # version we claim to support. This also surfaces drift between what is
     # installed and what openarmature.org/compatibility records as verified.
-    import re
     from importlib.metadata import version
 
     installed = version("langfuse")