refactor: extract shared Astroid utilities into core module#52
Open
nnennandukwe wants to merge 1 commit into
Open
refactor: extract shared Astroid utilities into core module#52nnennandukwe wants to merge 1 commit into
nnennandukwe wants to merge 1 commit into
Conversation
Moves shared AST parsing dataclasses (FunctionInfo, LoopInfo, ImportInfo, CallInfo, ClassInfo) and utility functions (parse_source, extract_functions, extract_classes, extract_imports, extract_calls, get_source_segment) into a new workshop_mcp.core module. The performance_profiler.ast_analyzer now delegates to core, with backward-compatible re-exports to avoid breaking existing imports. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
Review Summary by QodoExtract shared Astroid utilities into core module for code reuse
WalkthroughsDescription• Extract shared Astroid AST utilities into new core module • Enable code reuse across multiple analysis tools • Add ClassInfo dataclass and extract_classes() function • Maintain backward compatibility with existing imports • Add 39 comprehensive tests for core utilities Diagramflowchart LR
A["performance_profiler<br/>ast_analyzer.py"] -->|"delegates to"| B["core<br/>ast_utils.py"]
A -->|"uses"| C["core<br/>inference.py"]
B -->|"exports"| D["FunctionInfo<br/>LoopInfo<br/>ImportInfo<br/>CallInfo<br/>ClassInfo"]
C -->|"exports"| E["infer_callable<br/>get_qualified_name"]
F["core<br/>schema.py"] -->|"exports"| G["CodeIssue<br/>Severity"]
H["performance_profiler<br/>__init__.py"] -->|"re-exports"| D
I["tests<br/>test_core.py"] -->|"validates"| B
I -->|"validates"| C
I -->|"validates"| G
File Changes1. src/workshop_mcp/core/__init__.py
|
This was referenced Feb 5, 2026
Contributor
Code Review by Qodo
1. Missing core/patterns.py
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+1
to
+17
| """Core utilities for AST analysis shared across all code quality tools.""" | ||
|
|
||
| from .ast_utils import ( | ||
| CallInfo, | ||
| ClassInfo, | ||
| FunctionInfo, | ||
| ImportInfo, | ||
| LoopInfo, | ||
| extract_calls, | ||
| extract_classes, | ||
| extract_functions, | ||
| extract_imports, | ||
| get_source_segment, | ||
| parse_source, | ||
| ) | ||
| from .inference import get_qualified_name, infer_callable | ||
| from .schema import CodeIssue, Severity |
Contributor
There was a problem hiding this comment.
1. Missing core/patterns.py 📎 Requirement gap ✓ Correctness
• The shared workshop_mcp.core package does not include a patterns.py module, even though the checklist requires it for centralized AST pattern helpers. • This prevents other tools from reusing common call/loop/context detection logic and keeps generic helpers tied to performance_profiler.
Agent Prompt
## Issue description
The PR introduces `workshop_mcp.core` but does not include the required `core/patterns.py` module and helpers. The checklist requires these helpers to be centralized in core for reuse.
## Issue Context
The current codebase still relies on `workshop_mcp.performance_profiler.patterns`, which keeps generic helpers coupled to the performance profiler.
## Fix Focus Areas
- src/workshop_mcp/core/__init__.py[1-35]
- src/workshop_mcp/performance_profiler/__init__.py[5-7]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+72
to
+197
| def parse_source(source_code: str, file_path: str | None = None) -> astroid.Module: | ||
| """Parse Python source code into an Astroid AST module. | ||
|
|
||
| Args: | ||
| source_code: Python source code as a string. | ||
| file_path: Optional file path for error reporting. | ||
|
|
||
| Returns: | ||
| Parsed Astroid Module node. | ||
|
|
||
| Raises: | ||
| SyntaxError: If the source code contains syntax errors. | ||
| """ | ||
| try: | ||
| return astroid.parse(source_code, path=file_path) | ||
| except astroid.AstroidSyntaxError as e: | ||
| raise SyntaxError(str(e)) from e | ||
|
|
||
|
|
||
| def extract_functions(tree: astroid.Module) -> list[FunctionInfo]: | ||
| """Extract all function definitions from an Astroid AST. | ||
|
|
||
| Args: | ||
| tree: Parsed Astroid Module node. | ||
|
|
||
| Returns: | ||
| List of FunctionInfo objects containing function metadata. | ||
| """ | ||
| functions = [] | ||
| for node in tree.nodes_of_class((astroid.FunctionDef, astroid.AsyncFunctionDef)): | ||
| func_info = _extract_function_info(node) | ||
| functions.append(func_info) | ||
| return functions | ||
|
|
||
|
|
||
| def extract_classes(tree: astroid.Module) -> list[ClassInfo]: | ||
| """Extract all class definitions from an Astroid AST. | ||
|
|
||
| Args: | ||
| tree: Parsed Astroid Module node. | ||
|
|
||
| Returns: | ||
| List of ClassInfo objects containing class metadata. | ||
| """ | ||
| classes = [] | ||
| for node in tree.nodes_of_class(astroid.ClassDef): | ||
| class_info = _extract_class_info(node) | ||
| classes.append(class_info) | ||
| return classes | ||
|
|
||
|
|
||
| def extract_imports(tree: astroid.Module) -> list[ImportInfo]: | ||
| """Extract all import statements from an Astroid AST. | ||
|
|
||
| Args: | ||
| tree: Parsed Astroid Module node. | ||
|
|
||
| Returns: | ||
| List of ImportInfo objects containing import metadata. | ||
| """ | ||
| imports = [] | ||
|
|
||
| # Handle regular imports | ||
| for node in tree.nodes_of_class(astroid.Import): | ||
| for name, alias in node.names: | ||
| import_info = ImportInfo( | ||
| module=name, | ||
| names=[name], | ||
| line_number=node.lineno, | ||
| is_from_import=False, | ||
| aliases={name: alias} if alias else {}, | ||
| resolved_module=None, | ||
| ) | ||
| imports.append(import_info) | ||
|
|
||
| # Handle from imports | ||
| for node in tree.nodes_of_class(astroid.ImportFrom): | ||
| if node.modname: | ||
| aliases = {} | ||
| names = [] | ||
| for name, alias in node.names: | ||
| names.append(name) | ||
| if alias: | ||
| aliases[name] = alias | ||
|
|
||
| import_info = ImportInfo( | ||
| module=node.modname, | ||
| names=names, | ||
| line_number=node.lineno, | ||
| is_from_import=True, | ||
| aliases=aliases, | ||
| resolved_module=None, | ||
| ) | ||
| imports.append(import_info) | ||
|
|
||
| return imports | ||
|
|
||
|
|
||
| def extract_calls(tree: astroid.Module) -> list[CallInfo]: | ||
| """Extract all function calls from an Astroid AST. | ||
|
|
||
| Args: | ||
| tree: Parsed Astroid Module node. | ||
|
|
||
| Returns: | ||
| List of CallInfo objects containing call metadata. | ||
| """ | ||
| calls: list[CallInfo] = [] | ||
| _extract_calls_recursive(tree, calls, None, False, False) | ||
| return calls | ||
|
|
||
|
|
||
| def get_source_segment(source_code: str, line_start: int, line_end: int) -> str: | ||
| """Get a segment of source code by line numbers. | ||
|
|
||
| Args: | ||
| source_code: Full source code string. | ||
| line_start: Starting line number (1-indexed). | ||
| line_end: Ending line number (1-indexed, inclusive). | ||
|
|
||
| Returns: | ||
| Source code segment as a string. | ||
| """ | ||
| lines = source_code.splitlines() | ||
| return "\n".join(lines[line_start - 1 : line_end]) | ||
|
|
Contributor
There was a problem hiding this comment.
2. walk_ast() not implemented 📎 Requirement gap ✓ Correctness
• The checklist requires core.ast_utils to provide walk_ast() as a reusable traversal utility, but the module currently only exposes parse/extract helpers. • Downstream tools that need generic AST traversal will still have to implement their own walking logic, defeating the purpose of shared infrastructure.
Agent Prompt
## Issue description
`walk_ast()` is required by the checklist but is missing from `src/workshop_mcp/core/ast_utils.py`.
## Issue Context
Downstream tools may need a standard AST traversal utility for consistency and reuse.
## Fix Focus Areas
- src/workshop_mcp/core/ast_utils.py[72-197]
- src/workshop_mcp/core/__init__.py[1-35]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+1
to
+41
| """Astroid type inference utilities.""" | ||
|
|
||
| import astroid | ||
|
|
||
|
|
||
| def infer_callable(node: astroid.NodeNG) -> str | None: | ||
| """Try to infer the fully qualified name of a callable. | ||
|
|
||
| Uses Astroid's inference engine to resolve what a function actually is, | ||
| providing fully qualified names when possible. | ||
|
|
||
| Args: | ||
| node: An Astroid node representing a callable expression. | ||
|
|
||
| Returns: | ||
| Fully qualified name string, or None if inference fails. | ||
| """ | ||
| try: | ||
| inferred = next(node.infer(), None) | ||
| if inferred and hasattr(inferred, "qname"): | ||
| return inferred.qname() | ||
| except (astroid.InferenceError, StopIteration): | ||
| pass | ||
| return None | ||
|
|
||
|
|
||
| def get_qualified_name(node: astroid.NodeNG) -> str | None: | ||
| """Get the qualified name for an Astroid node if available. | ||
|
|
||
| Args: | ||
| node: An Astroid node. | ||
|
|
||
| Returns: | ||
| Qualified name string, or None if not available. | ||
| """ | ||
| if hasattr(node, "qname"): | ||
| try: | ||
| return node.qname() | ||
| except Exception: # noqa: S110 | ||
| return None | ||
| return None |
Contributor
There was a problem hiding this comment.
3. is_method_of() missing 📎 Requirement gap ✓ Correctness
• The shared inference module does not implement is_method_of(), which the checklist requires as part of reusable inference helpers. • Without it, tools needing method/class relationship checks must re-implement logic or depend on performance-profiler-specific code.
Agent Prompt
## Issue description
`is_method_of()` is required by the checklist but is not present in `src/workshop_mcp/core/inference.py`.
## Issue Context
This helper is needed for reusable call/target analysis across tools.
## Fix Focus Areas
- src/workshop_mcp/core/inference.py[1-41]
- src/workshop_mcp/core/__init__.py[1-35]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+16
to
+29
| @dataclass | ||
| class CodeIssue: | ||
| """Represents a code issue detected by any analysis tool.""" | ||
|
|
||
| tool: str | ||
| category: str | ||
| severity: Severity | ||
| message: str | ||
| line: int | ||
| suggestion: str | None = None | ||
| code_snippet: str | None = None | ||
| function_name: str | None = None | ||
| end_line: int | None = None | ||
| column: int | None = None |
Contributor
There was a problem hiding this comment.
4. codeissue missing file 📎 Requirement gap ✓ Correctness
• The unified CodeIssue schema does not match the required ticket fields: it lacks the required file field and makes column optional while the checklist specifies it as part of the schema. • This breaks the goal of consistent, cross-tool issue output and may force tools to create their own per-tool schemas or add ad-hoc fields.
Agent Prompt
## Issue description
`CodeIssue` in `core/schema.py` does not match the required unified issue schema (missing `file`, and field set differs from the required list).
## Issue Context
Downstream tools depend on a consistent schema across tools, so the dataclass fields must match the checklist acceptance criteria.
## Fix Focus Areas
- src/workshop_mcp/core/schema.py[16-29]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+21
to
+33
| class TestParseSource: | ||
| """Test parse_source utility.""" | ||
|
|
||
| def test_parse_valid_code(self): | ||
| """Test parsing valid Python source code.""" | ||
| tree = parse_source("def hello(): pass") | ||
| assert tree is not None | ||
|
|
||
| def test_parse_empty_source(self): | ||
| """Test parsing empty source code.""" | ||
| tree = parse_source("") | ||
| assert tree is not None | ||
|
|
Contributor
There was a problem hiding this comment.
5. Tests lack type hints 📘 Rule violation ✓ Correctness
• New test functions were added without parameter/return type annotations (e.g., missing -> None), violating the requirement that all functions must have type hints. • This reduces consistency across the codebase and weakens static analysis benefits that the project expects from universal type annotations.
Agent Prompt
## Issue description
The new tests in `tests/test_core.py` define many functions without type hints, which violates the project requirement that all functions have type annotations.
## Issue Context
Pytest tests can still be annotated (commonly with `-> None`) without impacting test execution.
## Fix Focus Areas
- tests/test_core.py[21-60]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+5
to
7
| from .ast_analyzer import ASTAnalyzer, CallInfo, FunctionInfo, ImportInfo, LoopInfo | ||
| from .patterns import IssueCategory, PerformanceIssue, Severity | ||
| from .performance_checker import PerformanceChecker |
Contributor
There was a problem hiding this comment.
6. Profiler not using core.schema 📎 Requirement gap ✓ Correctness
• The performance profiler package still imports its own Severity and issue structures from workshop_mcp.performance_profiler.patterns rather than using workshop_mcp.core.schema/core.patterns as required by the checklist. • This keeps the profiler partially coupled to its old internal infrastructure and undermines the goal of shared, unified tooling primitives.
Agent Prompt
## Issue description
Performance profiler code still relies on `performance_profiler.patterns` for issue/severity concepts instead of using the extracted core modules as required.
## Issue Context
This PR introduces `workshop_mcp.core.schema` but the profiler is not yet using it, leaving schema duplication/inconsistency.
## Fix Focus Areas
- src/workshop_mcp/performance_profiler/__init__.py[5-19]
- src/workshop_mcp/core/schema.py[1-29]
- src/workshop_mcp/core/__init__.py[1-35]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+184
to
+196
| def get_source_segment(source_code: str, line_start: int, line_end: int) -> str: | ||
| """Get a segment of source code by line numbers. | ||
|
|
||
| Args: | ||
| source_code: Full source code string. | ||
| line_start: Starting line number (1-indexed). | ||
| line_end: Ending line number (1-indexed, inclusive). | ||
|
|
||
| Returns: | ||
| Source code segment as a string. | ||
| """ | ||
| lines = source_code.splitlines() | ||
| return "\n".join(lines[line_start - 1 : line_end]) |
Contributor
There was a problem hiding this comment.
7. Unvalidated source bounds 🐞 Bug ✓ Correctness
• get_source_segment() documents 1-indexed inclusive line bounds but performs no validation; invalid inputs (e.g., line_start<=0, line_end<line_start) can silently return incorrect snippets due to Python slicing semantics. • This can mislead downstream issue reporting since snippets are embedded into reported issues (e.g., PerformanceChecker includes code_snippet from get_source_segment). • Add explicit bounds validation (or safe clamping) to prevent silent corruption of issue context.
Agent Prompt
## Issue description
`workshop_mcp.core.ast_utils.get_source_segment()` claims to accept 1-indexed inclusive line bounds but does not validate them. Python list slicing will silently accept invalid indices (e.g., 0 or negative) and return the wrong lines, which can mislead issue reports that include code snippets.
## Issue Context
Downstream code (e.g., `PerformanceChecker`) uses `get_source_segment()` to embed snippets into reported issues. If a caller ever passes invalid bounds (from AST metadata quirks or future tools), the snippet can be wrong without any signal.
## Fix Focus Areas
- src/workshop_mcp/core/ast_utils.py[184-196]
- src/workshop_mcp/performance_profiler/performance_checker.py[80-119]
## Suggested change
- Add checks like:
- `if line_start < 1: raise ValueError(...)`
- `if line_end < line_start: raise ValueError(...)`
- `if line_end > len(lines): raise ValueError(...)`
- Add a couple of unit tests for invalid bounds (start=0, end<start, end>EOF) to ensure the behavior is explicit.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
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.
Summary
Closes #41
Extracts reusable Astroid-based AST analysis utilities from the performance profiler into a new shared
workshop_mcp.coremodule, enabling new code quality tools to leverage the same infrastructure without duplicating parsing logic.What changed
New
core/module with four files:ast_utils.py— shared dataclasses (FunctionInfo,LoopInfo,ImportInfo,CallInfo,ClassInfo) and extraction functions (parse_source,extract_functions,extract_classes,extract_imports,extract_calls,get_source_segment)inference.py—infer_callable()andget_qualified_name()for Astroid type inferenceschema.py— unifiedCodeIssuedataclass andSeverityenum for cross-tool issue reporting__init__.py— public API re-exportsRefactored
performance_profiler/ast_analyzer.pyto delegate to core utilities instead of implementing its own extraction logic (~210 lines removed, replaced with imports from core)Backward-compatible re-exports in
performance_profiler/__init__.py— addedCallInfo,FunctionInfo,ImportInfo,LoopInfoto__all__so existing imports fromworkshop_mcp.performance_profilercontinue to work39 new tests in
tests/test_core.pycovering all core utilities, dataclass construction, inference helpers, and backward compatibility verificationAdditions beyond the original issue
ClassInfodataclass andextract_classes()— not in the original issue spec but needed by the complexity analyzer (feat: Add complexity analyzer tool #44) for class-level metricsperformance_profiler/__init__.py— ensures no breakage of existing importscore.FunctionInfo is performance_profiler.ast_analyzer.FunctionInfo(same class, not a copy)What was NOT included from the issue
The original issue proposed a broader
tools/directory restructure and several additional utility functions (walk_ast(),find_calls_in_node(),is_inside_loop(),is_method_of(), etc.). These were intentionally deferred — the current extraction covers exactly what the downstream tools (#44, #45) need. Thepatterns.pypattern-matching helpers were not extracted since the performance profiler's pattern matching is specific to performance checking.Test plan
poetry run pytest)ruff checkandruff format --checkpassworkshop_mcp.performance_profiler.ast_analyzerstill resolve to the same classes🤖 Generated with Claude Code