refactor(extractor): decompose PythonExtractor into focused handlers (#116)#191
Conversation
…116) PythonExtractor was a God Object (30+ methods, efferent coupling 48). Split the single 960-line class into a thin orchestrator plus focused collaborators: PythonExtractor — thin entry point: parse() + _walk() dispatch (4 methods) ImportHandler — import node walking + import_map building FunctionHandler — function/method extraction, CALLS/DI edges, local types ClassHandler — class extraction + inheritance/abstract metadata TypeResolver — type-annotation cleanup + FQN resolution _python_ast — pure leaf: name/path helpers, file_path_to_module_fqn Dependencies form a one-directional DAG onto the _python_ast leaf (no import cycles); handlers receive _pick_source_root by injection. file_path_to_module_fqn stays a real def in python_extractor.py, preserving the public import path and the self-parse FQN node. PythonExtractor is removed from _KNOWN_GOD_OBJECTS; the god-object baseline now passes without it. The decomposition changes cgis.extractors' topology from pure_utility to a layered DAG, so the extraction domain's drift binding is re-baselined to layered_dag (measured ~0.32, vs pure_utility 0.51) with a documented comment, per the self-drift gate's re-negotiation policy. All 58 python_extractor tests pass unchanged; full suite 781 passing, mypy strict clean, doc-coverage 99.5%. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
/guardian review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the monolithic PythonExtractor by decomposing it into several focused helper modules (_python_ast.py, _python_classes.py, _python_functions.py, _python_imports.py, and _python_types.py), transforming it into a clean orchestrator. Accordingly, the architectural tests and ontology configurations are updated to reflect this transition from a God Object to a layered DAG pattern. The review feedback correctly identifies a latent bug in the relative module resolution helper where slicing with [:-0] would incorrectly return an empty list when leading_dots is zero, and provides a robust code suggestion to handle this case.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary: Found 5 major type safety violations: missing annotations for module-level constants, class methods, and function return types in new extractor modules. No logic, ontology, or contract defects detected.
🤖 mistral-medium-latest · 29,632 prompt + 934 completion = 30,566 tokens · graph 6/7 files (86%)
gemini review on #191: `segments[:-trim]` with trim==0 evaluates to `segments[:0]` == [], silently dropping the whole FQN. Unreachable today (call site guards with `leading_dots > 0`) but a latent trap for tests / future callers. Branch explicitly on trim==0 → return the module unchanged. Locked with two regression tests. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Review triagegemini — guardian/mistral — 4× "Type Safety: missing annotations": ❌ declined as false positives.
No code change for the guardian findings. |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors the PythonExtractor by decomposing it from a single large class into a layered package structure with dedicated handlers for AST operations, classes, functions, imports, and types. The architectural pattern for the extraction domain is updated to layered_dag in the ontology documentation, and PythonExtractor is removed from the list of known god objects. The code review feedback identifies several critical issues: relative imports inside __init__.py files are incorrectly resolved due to over-trimming, which can be fixed by introducing an is_package flag; typed_default_parameter nodes are silently ignored during parameter type collection and require unwrapping; and Annotated should be added to the generic wrappers to support modern FastAPI dependency injection types.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| def resolve_relative_module(module_fqn: str, leading_dots: int, relative_path: str) -> str: | ||
| """Resolve a relative import to an absolute FQN. | ||
|
|
||
| Args: | ||
| module_fqn: FQN of the current file, e.g. "src.cgis.extractors.python_extractor" | ||
| leading_dots: number of leading dots in the import statement | ||
| relative_path: module path after the dots, e.g. "core" for "from ..core import x" | ||
|
|
||
| Returns "src.cgis.core" for leading_dots=2, relative_path="core" in the example above. | ||
| """ | ||
| segments = module_fqn.split(".") | ||
| trim = min(leading_dots, len(segments)) |
There was a problem hiding this comment.
When resolving relative imports, file_path_to_module_fqn strips the __init__ suffix from package files, making their FQN look like standard modules. As a result, resolve_relative_module trims one segment too many for relative imports inside __init__.py files (e.g., from . import pipeline inside src/cgis/__init__.py incorrectly resolves to src.pipeline instead of src.cgis.pipeline). Adding an is_package flag allows us to correctly adjust the number of segments to trim.
def resolve_relative_module(
module_fqn: str, leading_dots: int, relative_path: str, is_package: bool = False
) -> str:
"""Resolve a relative import to an absolute FQN.
Args:
module_fqn: FQN of the current file, e.g. "src.cgis.extractors.python_extractor"
leading_dots: number of leading dots in the import statement
relative_path: module path after the dots, e.g. "core" for "from ..core import x"
is_package: whether the current file is an __init__.py package file
Returns "src.cgis.core" for leading_dots=2, relative_path="core" in the example above.
"""
segments = module_fqn.split(".")
dots_to_trim = leading_dots - 1 if is_package and leading_dots > 0 else leading_dots
trim = min(dots_to_trim, len(segments))| base_module = ( | ||
| resolve_relative_module(module_fqn, leading_dots, raw_module_str) | ||
| if leading_dots > 0 | ||
| else raw_module_str | ||
| ) |
There was a problem hiding this comment.
Pass the is_package flag to resolve_relative_module to ensure relative imports inside __init__.py files are resolved correctly without trimming the package name segment.
| base_module = ( | |
| resolve_relative_module(module_fqn, leading_dots, raw_module_str) | |
| if leading_dots > 0 | |
| else raw_module_str | |
| ) | |
| is_package = file_path.replace("\\\\", "/").endswith("/__init__.py") or file_path.endswith("__init__.py") | |
| base_module = ( | |
| resolve_relative_module(module_fqn, leading_dots, raw_module_str, is_package=is_package) | |
| if leading_dots > 0 | |
| else raw_module_str | |
| ) |
| if not node.named_children: | ||
| return |
There was a problem hiding this comment.
When a parameter has both a type annotation and a default value (e.g., db: Session = Depends(get_db)), tree-sitter-python parses it as a typed_default_parameter node. In this case, the type field is located inside the nested typed_parameter node (accessible via the name field of the typed_default_parameter). Currently, collect_param_type directly calls node.child_by_field_name("type") on the typed_default_parameter node, which returns None. This causes all typed default parameters to be silently ignored for local type collection and speculative dependency edges. Unwrapping the typed_default_parameter to its inner typed_parameter resolves this issue.
| if not node.named_children: | |
| return | |
| if node.type == "typed_default_parameter": | |
| inner = node.child_by_field_name("name") | |
| if inner: | |
| node = inner | |
| if not node.named_children: | |
| return |
| import-map lookup that turns a bare or module-prefixed type name into a FQN. | ||
| """ | ||
|
|
||
| _GENERIC_WRAPPERS: frozenset[str] = frozenset({"Optional", "Union"}) |
There was a problem hiding this comment.
Add Annotated to _GENERIC_WRAPPERS. In modern Python and FastAPI, dependency injection is heavily declared using Annotated[DependencyType, Depends(...)]. Without Annotated in _GENERIC_WRAPPERS, clean_python_type_string returns "Annotated" instead of the actual dependency type, causing speculative dependency edges to target raw_dep:Annotated instead of the correct type.
| _GENERIC_WRAPPERS: frozenset[str] = frozenset({"Optional", "Union"}) | |
| _GENERIC_WRAPPERS: frozenset[str] = frozenset({"Optional", "Union", "Annotated"}) |
Review triage — gemini round 2All four findings are in code moved verbatim by this refactor (no logic changed), so I verified each empirically against the installed #3 — The current code already handles this correctly. The suggested fix reassigns #1 & #2 — relative imports inside #4 — Net: no code change to this PR — it stays a clean, behavior-preserving decomposition. The two genuine pre-existing gaps are tracked in #194 (same policy as #183 for the resolver split). |
…194) (#195) * fix(extractor): correct relative imports in __init__.py + unwrap Annotated[] (#194) Two pre-existing extractor correctness gaps surfaced by the #191 review. 1. Relative imports inside a package __init__.py over-trimmed by one segment. file_path_to_module_fqn strips the /__init__ suffix, so a package's FQN already IS the package — one leading dot refers to the package itself. resolve_relative_module gains an is_package flag (trim leading_dots-1 for packages); ImportHandler detects __init__.py and passes it. `from . import pipeline` in src/cgis/__init__.py now resolves to src.cgis.pipeline (was src.pipeline); `from .sub import X` → src.cgis.sub.X. 2. Annotated[T, ...] type annotations resolved to "Annotated" instead of T. Added Annotated to TypeResolver._GENERIC_WRAPPERS so it unwraps to its first arg. The DI provider in the metadata (e.g. Depends(get_db)) is unaffected — it is already captured separately as a DEPENDS_ON edge via the call-node path (verified), so this only fixes the local-type / raw_dep candidate. A third #191 finding (typed_default_parameter type allegedly dropped) was empirically disproven and not changed. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * refactor(extractor): bracket-depth-aware first-arg split in type cleaning #195 review (gemini): adding Annotated to the generic wrappers routes nested-generic first args (e.g. Annotated[dict[str, int], ...]) through the `inner.split(",")[0]` path. Empirically the final output was already correct ("dict") — the truncated "dict[str" self-heals because clean re-partitions on the first "[" — and this split predates this PR (Optional/Union used it too). So no live bug, but the correctness was accidental. Harden it: `_first_top_level_arg` splits on the first comma at bracket depth 0, so the first argument is correct by design rather than by self-healing. Behavior-preserving (verified identical on all nested cases); locked with a nested-generic regression test. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
1 participant
Summary
Closes #116.
PythonExtractorwas a God Object (30+ methods, efferent coupling 48, flagged bycgis analyzeon cgis itself). This splits the single 960-line class into a thin orchestrator plus focused collaborators.python_extractor.py__init__,parse,_walk(dispatch),_handle_decorated_definition(4 methods)_python_ast.pyfile_path_to_module_fqn,is_method, dispatch predicates_python_imports.pyImportHandler— import walking +import_mapbuilding_python_functions.pyFunctionHandler— function/method extraction, CALLS/DI edges, local types_python_classes.pyClassHandler— class extraction + inheritance/abstract metadata_python_types.pyTypeResolver— type-annotation cleanup + FQN resolutionDesign notes
_python_astleaf; handlers receive_pick_source_rootby injection (the composition root is the extractor).file_path_to_module_fqnstays a realdefinpython_extractor.py(thin delegate), keeping both thecgis.extractors.python_extractorimport path and the self-parse FQN node intact.Acceptance criteria
PythonExtractordrops below God Object threshold (cgis confirms: 4 methods)tests/unit/test_python_extractor.pypass unchanged (58)test_god_object_baseline_not_exceededpasses withPythonExtractorremoved from_KNOWN_GOD_OBJECTSReviewer attention: drift re-baseline
The decomposition legitimately changes
cgis.extractors's topology from apure_utilityshape into a layered DAG (orchestrator → handlers →_python_astcore). This breaches the self-drift gate (test_drift.py), so theextractiondomain binding indocs/ontology/patterns.yamlis re-negotiated:layered_dag0.32 (best fit, same ascgis.query) vsorchestrator0.40 vspure_utility0.51expected_pattern: pure_utility → layered_dag,drift_tolerance: 0.30 → 0.35, with an explanatory comment per the gate's documented re-negotiation policy.Verification
make format && make lint && make type-check && make pytest && make doc-coverage— all green: 781 tests pass, mypy strict clean (51 files), doc-coverage 99.5%.🤖 Generated with Claude Code