feat: add dead code detector tool#54
Open
nnennandukwe wants to merge 1 commit intofeature/44-complexity-analysisfrom
Open
feat: add dead code detector tool#54nnennandukwe wants to merge 1 commit intofeature/44-complexity-analysisfrom
nnennandukwe wants to merge 1 commit intofeature/44-complexity-analysisfrom
Conversation
Adds a new dead_code_detection MCP tool that identifies unused imports, unused variables, unused functions, unused parameters, unreachable code after return/raise/break/continue, and redundant conditions (if True, if False, while False). Builds a usage graph from the Astroid AST to track symbol definitions vs references. Supports ignore_patterns for skipping intentional symbols and per-category check toggles. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
Review Summary by QodoAdd dead code detection MCP tool with AST analysis
WalkthroughsDescription• Adds new dead_code_detection MCP tool for identifying unused Python code - Detects unused imports, variables, functions, parameters - Detects unreachable code after return/raise/break/continue - Detects redundant conditions (if True, if False, while False) • Implements DeadCodeDetector class with AST-based analysis using Astroid - Builds usage graph to track symbol definitions vs references - Supports __all__ awareness for public API exports - Includes stub function detection and framework decorator awareness • Registers tool in MCP server with full JSON Schema and validation - Supports per-category check toggles and regex ignore patterns - Handles both file_path and source_code inputs with error handling • Adds 64 comprehensive tests covering all detection patterns and edge cases Diagramflowchart LR
A["Python Source Code"] -->|parse| B["Astroid AST"]
B -->|build| C["UsageGraph"]
C -->|analyze| D["DeadCodeDetector"]
D -->|detect| E["DeadCodeResult"]
E -->|format| F["MCP Tool Response"]
G["MCP Server"] -->|register| H["dead_code_detection Tool"]
H -->|invoke| D
File Changes1. src/workshop_mcp/dead_code_detection/__init__.py
|
Contributor
Code Review by Qodo
1. Dead code tool wrong path
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+1
to
+8
| """Dead code detection tools for identifying unused Python code.""" | ||
|
|
||
| __version__ = "0.1.0" | ||
|
|
||
| from .detector import DeadCodeDetector, DeadCodeResult, DeadCodeSummary, detect_dead_code | ||
| from .patterns import DeadCodeCategory, DeadCodeIssue | ||
| from .usage_graph import UsageGraph, build_usage_graph | ||
|
|
Contributor
There was a problem hiding this comment.
1. Dead code tool wrong path 📎 Requirement gap ✓ Correctness
• The dead code detection implementation is added under src/workshop_mcp/dead_code_detection/ instead of the required src/workshop_mcp/tools/dead_code_detection/ location. • This breaks the repository’s prescribed tool structure/discoverability expectations and can cause import/packaging inconsistencies for MCP tools.
Agent Prompt
## Issue description
The dead code detection tool code is not located in the required tool module directory (`src/workshop_mcp/tools/dead_code_detection/`).
## Issue Context
The compliance checklist mandates this directory structure for discoverability/consistency.
## Fix Focus Areas
- src/workshop_mcp/dead_code_detection/__init__.py[1-18]
- src/workshop_mcp/dead_code_detection/detector.py[1-450]
- src/workshop_mcp/dead_code_detection/patterns.py[1-57]
- src/workshop_mcp/dead_code_detection/usage_graph.py[1-99]
- src/workshop_mcp/server.py[15-20]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+86
to
+120
| def detect_all(self) -> DeadCodeResult: | ||
| """Run all enabled dead code checks. | ||
|
|
||
| Returns: | ||
| DeadCodeResult with issues and summary. | ||
| """ | ||
| result = DeadCodeResult() | ||
|
|
||
| if self.check_imports: | ||
| issues = self._detect_unused_imports() | ||
| result.issues.extend(issues) | ||
| result.summary.unused_imports = len(issues) | ||
|
|
||
| if self.check_variables: | ||
| issues = self._detect_unused_variables() | ||
| result.issues.extend(issues) | ||
| result.summary.unused_variables = len(issues) | ||
|
|
||
| if self.check_functions: | ||
| issues = self._detect_unused_functions() | ||
| result.issues.extend(issues) | ||
| result.summary.unused_functions = len(issues) | ||
|
|
||
| param_issues = self._detect_unused_parameters() | ||
| result.issues.extend(param_issues) | ||
| result.summary.unused_parameters = len(param_issues) | ||
|
|
||
| unreachable_issues = self._detect_unreachable_code() | ||
| result.issues.extend(unreachable_issues) | ||
| result.summary.unreachable_blocks = len(unreachable_issues) | ||
|
|
||
| redundant_issues = self._detect_redundant_conditions() | ||
| result.issues.extend(redundant_issues) | ||
| result.summary.redundant_conditions = len(redundant_issues) | ||
|
|
Contributor
There was a problem hiding this comment.
2. No unused class detection 📎 Requirement gap ✓ Correctness
• The detector currently reports unused imports/variables/functions/parameters plus unreachable and redundant conditions, but does not implement detection for unused classes or class attributes. • This means the tool does not meet the required detection coverage and will miss entire categories of dead code that the interface promises to detect.
Agent Prompt
## Issue description
The tool does not detect unused classes or unused class attributes, but compliance requires both.
## Issue Context
The current implementation only checks imports/variables/functions/parameters/unreachable/redundant, and `DeadCodeCategory` does not include class/class-attribute categories.
## Fix Focus Areas
- src/workshop_mcp/dead_code_detection/patterns.py[7-16]
- src/workshop_mcp/dead_code_detection/detector.py[86-240]
- src/workshop_mcp/dead_code_detection/usage_graph.py[35-99]
- tests/test_dead_code_detection.py[1-702]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+846
to
+907
| # Read source from file if needed | ||
| if file_path and not source_code: | ||
| source_code = Path(file_path).read_text(encoding="utf-8") | ||
|
|
||
| detector = DeadCodeDetector( | ||
| source_code=source_code, | ||
| file_path=file_path, | ||
| check_imports=check_imports, | ||
| check_variables=check_variables, | ||
| check_functions=check_functions, | ||
| ignore_patterns=ignore_patterns, | ||
| ) | ||
|
|
||
| detection_result = detector.detect_all() | ||
|
|
||
| issues_data = [ | ||
| { | ||
| "tool": "dead_code", | ||
| "category": issue.category.value, | ||
| "severity": issue.severity.value, | ||
| "message": issue.message, | ||
| "line": issue.line, | ||
| "name": issue.name, | ||
| "suggestion": issue.suggestion, | ||
| } | ||
| for issue in detection_result.issues | ||
| ] | ||
|
|
||
| result = { | ||
| "content": [ | ||
| { | ||
| "type": "json", | ||
| "json": { | ||
| "success": True, | ||
| "file_analyzed": file_path or "source_code", | ||
| "issues": issues_data, | ||
| "summary": asdict(detection_result.summary), | ||
| }, | ||
| } | ||
| ], | ||
| } | ||
| return self._success_response(request_id, result) | ||
|
|
||
| except ValueError as exc: | ||
| logger.warning("ValueError in dead_code_detection: %s", exc) | ||
| return self._error_response( | ||
| request_id, | ||
| JsonRpcError(-32602, "Invalid parameters"), | ||
| ) | ||
| except FileNotFoundError as exc: | ||
| logger.warning("FileNotFoundError in dead_code_detection: %s", exc) | ||
| return self._error_response( | ||
| request_id, | ||
| JsonRpcError(-32602, "Resource not found"), | ||
| ) | ||
| except SyntaxError as exc: | ||
| logger.warning("SyntaxError in dead_code_detection: %s", exc) | ||
| return self._error_response( | ||
| request_id, | ||
| JsonRpcError(-32602, "Invalid source code syntax"), | ||
| ) | ||
| except SecurityValidationError as exc: |
Contributor
There was a problem hiding this comment.
3. Permissionerror not handled 📘 Rule violation ⛯ Reliability
• The server reads file_path via Path(file_path).read_text(...) but does not handle PermissionError/permission-denied cases explicitly. • This can produce an internal error response instead of a graceful, user-safe, structured error when the file exists but is not readable.
Agent Prompt
## Issue description
`dead_code_detection` reads files and can raise `PermissionError`, which is not handled explicitly, leading to generic internal errors.
## Issue Context
Compliance requires graceful handling of permission issues for filesystem access.
## Fix Focus Areas
- src/workshop_mcp/server.py[820-923]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+81
to
+86
| self.check_imports = check_imports | ||
| self.check_variables = check_variables | ||
| self.check_functions = check_functions | ||
| self._ignore_res = [re.compile(p) for p in (ignore_patterns or [])] | ||
|
|
||
| def detect_all(self) -> DeadCodeResult: |
Contributor
There was a problem hiding this comment.
4. Invalid regex not validated 📘 Rule violation ⛯ Reliability
• User-provided ignore_patterns are compiled with re.compile(...) without validation; invalid patterns can raise re.error at runtime. • This violates robust edge case management because a common user input error (bad regex) becomes a tool failure instead of a clear invalid-params response.
Agent Prompt
## Issue description
Invalid regex strings in `ignore_patterns` can raise `re.error` and cause an internal error instead of a structured invalid-params response.
## Issue Context
This is an external input field, so invalid patterns are a foreseeable user error and should be handled explicitly.
## Fix Focus Areas
- src/workshop_mcp/dead_code_detection/detector.py[43-86]
- src/workshop_mcp/server.py[829-923]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+43
to
+52
| def __init__( | ||
| self, | ||
| source_code: str | None = None, | ||
| *, | ||
| file_path: str | None = None, | ||
| check_imports: bool = True, | ||
| check_variables: bool = True, | ||
| check_functions: bool = True, | ||
| ignore_patterns: list[str] | None = None, | ||
| ): |
Contributor
There was a problem hiding this comment.
5. deadcodedetector.init missing return hint 📘 Rule violation ✓ Correctness
• DeadCodeDetector.__init__ does not declare an explicit return type (-> None). • This violates the project requirement that all new/modified functions include type hints, and can reduce mypy/static-analysis clarity.
Agent Prompt
## Issue description
`DeadCodeDetector.__init__` is missing an explicit return type annotation.
## Issue Context
The compliance checklist requires type hints for all functions.
## Fix Focus Areas
- src/workshop_mcp/dead_code_detection/detector.py[43-52]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+88
to
+96
| # For attribute access like 'os.path.join', we still want 'os' as referenced | ||
| elif isinstance(node, astroid.Attribute): | ||
| # Walk down to the leftmost Name in the chain | ||
| leftmost = node | ||
| while isinstance(leftmost, astroid.Attribute): | ||
| leftmost = leftmost.expr | ||
| if isinstance(leftmost, astroid.Name): | ||
| graph.references.add(leftmost.name) | ||
|
|
Contributor
There was a problem hiding this comment.
6. Method calls not counted 🐞 Bug ✓ Correctness
• Class methods are analyzed for “unused function” status, but the usage graph does not record attribute/member names as references. • As a result, calls like obj.method() / self.method() won’t mark method as referenced, leading to false-positive UNUSED_FUNCTION issues for real codebases.
Agent Prompt
## Issue description
Class methods can be falsely flagged as unused because `UsageGraph` does not record attribute/method names from calls like `self.foo()`/`obj.foo()`, but `DeadCodeDetector` checks class-level functions (methods) using only `graph.is_referenced(method_name)`.
## Issue Context
`UsageGraph._collect_references()` currently only adds the leftmost name of an attribute chain (e.g., `self` for `self.foo()`), which means method names will not appear in `graph.references`.
## Fix Focus Areas
- src/workshop_mcp/dead_code_detection/usage_graph.py[70-99]
- src/workshop_mcp/dead_code_detection/detector.py[188-239]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+820
to
+848
| if file_path: | ||
| try: | ||
| self.path_validator.validate_exists(file_path, must_be_file=True) | ||
| except PathValidationError as e: | ||
| return self._error_response( | ||
| request_id, | ||
| JsonRpcError(-32602, str(e)), | ||
| ) | ||
|
|
||
| check_imports = arguments.get("check_imports", True) | ||
| check_variables = arguments.get("check_variables", True) | ||
| check_functions = arguments.get("check_functions", True) | ||
| ignore_patterns = arguments.get("ignore_patterns") | ||
|
|
||
| if ignore_patterns is not None and ( | ||
| not isinstance(ignore_patterns, list) | ||
| or not all(isinstance(p, str) for p in ignore_patterns) | ||
| ): | ||
| return self._error_response( | ||
| request_id, | ||
| JsonRpcError(-32602, "ignore_patterns must be a list of strings"), | ||
| ) | ||
|
|
||
| try: | ||
| logger.info("Executing dead code detection") | ||
|
|
||
| # Read source from file if needed | ||
| if file_path and not source_code: | ||
| source_code = Path(file_path).read_text(encoding="utf-8") |
Contributor
There was a problem hiding this comment.
7. Toctou on file read 🐞 Bug ⛨ Security
• The server validates a path with PathValidator.validate_exists() (which resolves symlinks), but then reads the file using the original user-provided string. • Between validation and read, a symlink can be swapped to point outside allowed roots, turning a validated request into an arbitrary file read.
Agent Prompt
## Issue description
File reads occur using the original `file_path` string after validation, allowing a symlink swap between validation and open to bypass allowed-root constraints.
## Issue Context
`PathValidator.validate_exists()` already returns a resolved `Path` (via `Path.resolve()`), which should be used for subsequent filesystem operations.
## Fix Focus Areas
- src/workshop_mcp/server.py[820-849]
- src/workshop_mcp/security/path_validator.py[102-178]
ⓘ 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 #45
Depends on: #53 (complexity analysis), #52 (core module)
Adds a new
dead_code_detectionMCP tool that identifies unused and unreachable Python code using Astroid AST analysis and a symbol usage graph.What changed
New
dead_code_detection/module with four files:detector.py—DeadCodeDetectorclass orchestrating all checks, plusdetect_dead_code()convenience functionusage_graph.py—UsageGraphclass andbuild_usage_graph()that walks the AST to track symbol definitions vs references, including__all__awarenesspatterns.py—DeadCodeCategoryenum,DeadCodeIssuedataclass,Severityenum,EXTERNALLY_USED_DECORATORSset, andIGNORED_PARAMETERSset__init__.py— public API re-exportsserver.pychanges (incremental on top of feat: add complexity analyzer tool #53):from .dead_code_detection import DeadCodeDetectorimportdead_code_detectiontool in_handle_list_toolswith full JSON Schema (includingcheck_imports,check_variables,check_functions,ignore_patternsoptions)elif name == "dead_code_detection"dispatch in_handle_call_tool_execute_dead_code_detection()handler method with validation,ignore_patternstype checking, and error handling64 new tests in
tests/test_dead_code_detection.pycovering:__all__exemption, multi-name partial usage__all__exemption, public vs private severityDead code patterns detected
unused_importunused_variableunused_function_)unused_functionunused_parameterunreachable_codeif True/if False/while Falseredundant_conditionAdditions beyond the original issue
UsageGraphwith__all__support — symbols listed in__all__are automatically exempted from unused detection, since they're part of the public APIpass,..., ordocstring + passare skipped (these are interface stubs)@property,@staticmethod,@classmethod,@abstractmethod,@pytest.fixture,@app.route,@router.get/post/put/delete,@click.command,@celery.taskare not flagged as unusedinfoseverity (lower confidence, could be used externally) while private_-prefixed functions getwarning(higher confidence they're truly dead)detect_dead_code()convenience function — functional API alternative to the class-basedDeadCodeDetectorWhat was NOT included from the issue
self.attr) which adds significant complexity. Deferred.Test plan
poetry run pytest— 205 from base + 64 new)ruff checkandruff format --checkpassgit diff)🤖 Generated with Claude Code