refactor: decompose executor.py into modular package - #12
Conversation
🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Remove unused imports (threading, QualityDimension, QualityMetric) - Fix undefined logger -> self.logger in clean.py - Remove redundant IOError alias (OSError covers it) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
P0 fixes: - Remove deprecated Codex CLI integration (codex_cli.py, codex_client.py, codex_implementer.py) - Remove deprecated behavioral modes (Brainstorming, Introspection, Orchestration) - Implement thread-safe usage_tracker.py with functional telemetry - Clean up behavioral_manager.py (-22% lines) P1 executor decomposition: - Rename executor.py -> command_executor.py to avoid package conflict - Create executor/ package with 8 focused modules: - ast_analysis.py: Python AST semantic analysis - utils.py: Common utility functions - git_operations.py: Git and repository operations - testing.py: Test execution and pytest parsing - change_management.py: Change plans, stubs, file ops - telemetry.py: Metrics, artifacts, event recording - quality.py: Quality assessment utilities - agent_orchestration.py: Agent selection/delegation - consensus.py: Consensus building and policies Total extracted: ~3,309 lines into reusable modules. All imports verified working. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sorry @Tony363, your pull request is larger than the review limit of 150000 diff characters
|
Caution Review failedThe pull request is closed. Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughThis change removes Codex CLI and Codex-backed implementations (entire modules and agent class), replaces them with MCP-based PAL review signaling, adds comprehensive executor utilities across 8 new modules, implements a functional usage tracker, removes three deprecated behavioral modes, and updates registry and command handling accordingly. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Areas requiring extra attention:
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (23)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Kept PAL review signal pattern (tells Claude to invoke MCP tools). Deleted deprecated MODE files (Introspection, Orchestration). 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
| result = subprocess.run( | ||
| cmd, cwd=repo_root, capture_output=True, text=True, check=False | ||
| ) |
There was a problem hiding this comment.
security (python.lang.security.audit.dangerous-subprocess-use-audit): Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'.
Source: opengrep
| result = subprocess.run( | ||
| args, | ||
| cwd=str(working_dir), | ||
| capture_output=True, | ||
| text=True, | ||
| env=runtime_env, | ||
| timeout=timeout, | ||
| check=False, | ||
| ) |
There was a problem hiding this comment.
security (python.lang.security.audit.dangerous-subprocess-use-audit): Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'.
Source: opengrep
| result = subprocess.run( | ||
| cmd, cwd=repo_root, capture_output=True, text=True, check=False | ||
| ) |
There was a problem hiding this comment.
security (python.lang.security.audit.dangerous-subprocess-use-audit): Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'.
Source: opengrep
| result = subprocess.run( | ||
| command, | ||
| cwd=working_dir, | ||
| capture_output=True, | ||
| text=True, | ||
| check=False, | ||
| env=env, | ||
| ) |
There was a problem hiding this comment.
security (python.lang.security.audit.dangerous-subprocess-use-audit): Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'.
Source: opengrep
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| from .executor import ( | ||
| PythonSemanticAnalyzer as _PythonSemanticAnalyzer, | ||
| clamp_int, | ||
| coerce_float, | ||
| collect_diff_stats, | ||
| deduplicate, | ||
| detect_repo_root, | ||
| diff_snapshots, | ||
| ensure_list, | ||
| extract_changed_paths, | ||
| extract_feature_list, | ||
| extract_heading_titles, | ||
| extract_output_evidence, | ||
| format_change_entry, | ||
| generate_commit_message, | ||
| git_has_modifications, | ||
| is_artifact_change, | ||
| is_truthy, | ||
| normalize_evidence_value, | ||
| normalize_repo_root, | ||
| parse_pytest_output, | ||
| partition_change_entries, | ||
| relative_to_repo_path, | ||
| run_command, | ||
| run_requested_tests, | ||
| select_feature_owner, | ||
| should_run_tests, | ||
| slugify, | ||
| snapshot_repo_changes, | ||
| summarize_test_results, | ||
| to_list, | ||
| truncate_output, | ||
| ) |
Check notice
Code scanning / CodeQL
Unused import Note
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 9 months ago
The best way to fix the problem is to remove the names from the import statement on line 63 that are not actually used in the file. This minimizes unnecessary dependencies and improves code readability and maintainability. To do this:
- Review the import line that brings in many symbols from
.executor, and remove the names corresponding to the unused imports (i.e., all those listed in the alert variants). - Retain only the imported names that are actually used elsewhere in the file; if none from that block are used except
_PythonSemanticAnalyzer, then leave only that name (or the minimal necessary subset). - The edits should be confined to the relevant import block, without affecting other logic.
| @@ -62,36 +62,6 @@ | ||
| # Import decomposed executor modules | ||
| from .executor import ( | ||
| PythonSemanticAnalyzer as _PythonSemanticAnalyzer, | ||
| clamp_int, | ||
| coerce_float, | ||
| collect_diff_stats, | ||
| deduplicate, | ||
| detect_repo_root, | ||
| diff_snapshots, | ||
| ensure_list, | ||
| extract_changed_paths, | ||
| extract_feature_list, | ||
| extract_heading_titles, | ||
| extract_output_evidence, | ||
| format_change_entry, | ||
| generate_commit_message, | ||
| git_has_modifications, | ||
| is_artifact_change, | ||
| is_truthy, | ||
| normalize_evidence_value, | ||
| normalize_repo_root, | ||
| parse_pytest_output, | ||
| partition_change_entries, | ||
| relative_to_repo_path, | ||
| run_command, | ||
| run_requested_tests, | ||
| select_feature_owner, | ||
| should_run_tests, | ||
| slugify, | ||
| snapshot_repo_changes, | ||
| summarize_test_results, | ||
| to_list, | ||
| truncate_output, | ||
| ) | ||
|
|
||
| logger = logging.getLogger(__name__) |
| """ | ||
|
|
||
| import logging | ||
| import re |
Check notice
Code scanning / CodeQL
Unused import Note
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 9 months ago
To fix the problem, the unused import statement should be deleted. Specifically, remove the line import re from the file SuperClaude/Commands/executor/agent_orchestration.py. This makes the codebase cleaner and removes an unnecessary dependency. No other code changes, imports, or method definitions are required.
| @@ -6,7 +6,6 @@ | ||
| """ | ||
|
|
||
| import logging | ||
| import re | ||
| from typing import Any, Dict, Iterable, List, Optional, Set, Tuple | ||
|
|
||
| logger = logging.getLogger(__name__) |
| import re | ||
| from typing import Any, Dict, Iterable, List, Optional, Set, Tuple | ||
|
|
||
| logger = logging.getLogger(__name__) |
Check notice
Code scanning / CodeQL
Unused global variable Note
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 9 months ago
The best way to fix this problem is to remove the unused logger assignment. Since the assignment (logger = logging.getLogger(__name__)) has no side effects and only assigns to a variable that is never used, it can be safely deleted completely. No additional imports, definitions, or alternative code are required. Only line 12 of the shown code needs to be deleted. Do not remove the import of logging, as it may be used elsewhere in the snippet or file.
| @@ -9,7 +9,6 @@ | ||
| import re | ||
| from typing import Any, Dict, Iterable, List, Optional, Set, Tuple | ||
|
|
||
| logger = logging.getLogger(__name__) | ||
|
|
||
|
|
||
| # Default persona to agent mapping |
|
|
||
| from .utils import deduplicate, slugify | ||
|
|
||
| logger = logging.getLogger(__name__) |
Check notice
Code scanning / CodeQL
Unused global variable Note
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 9 months ago
To fix the problem, we should remove the assignment to logger—the line logger = logging.getLogger(__name__)—from the code. This change does not affect any existing functionality, as the variable is neither read nor used elsewhere. Only this single line needs to be deleted. As there are no side effects to the right-hand side of the assignment (calling logging.getLogger() registers nothing, does nothing unless used), this removal is safe and correct.
| @@ -14,7 +14,6 @@ | ||
|
|
||
| from .utils import deduplicate, slugify | ||
|
|
||
| logger = logging.getLogger(__name__) | ||
|
|
||
|
|
||
| def derive_change_plan( |
| from datetime import datetime | ||
| from typing import Any, Dict, List, Optional, Tuple | ||
|
|
||
| logger = logging.getLogger(__name__) |
Check notice
Code scanning / CodeQL
Unused global variable Note
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 9 months ago
To fix this issue, we should remove the unused global variable logger from the file. Specifically, delete the line logger = logging.getLogger(__name__) near the top of the module (line 13). This resolves the issue without affecting the module's functionality, as logger is not referenced elsewhere in the provided code.
| @@ -10,9 +10,9 @@ | ||
| from datetime import datetime | ||
| from typing import Any, Dict, List, Optional, Tuple | ||
|
|
||
| logger = logging.getLogger(__name__) | ||
|
|
||
|
|
||
|
|
||
| def serialize_assessment(assessment: Any) -> Dict[str, Any]: | ||
| """Convert a QualityAssessment dataclass into JSON-serializable dict. | ||
|
|
| from dataclasses import dataclass | ||
| from datetime import datetime | ||
| from pathlib import Path | ||
| from typing import Any, Dict, Iterable, List, Optional, Sequence, Set, Tuple |
Check notice
Code scanning / CodeQL
Unused import Note
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 9 months ago
The best way to fix this problem is to remove the unused imports from the from typing import ... line. Specifically, delete Iterable, Set, and Tuple from the import list on line 15 of SuperClaude/Commands/executor/telemetry.py. The other types (Any, Dict, List, Optional, Sequence) are in active use and should remain. This change is self-contained and does not affect any other code in the snippet.
| @@ -12,7 +12,7 @@ | ||
| from dataclasses import dataclass | ||
| from datetime import datetime | ||
| from pathlib import Path | ||
| from typing import Any, Dict, Iterable, List, Optional, Sequence, Set, Tuple | ||
| from typing import Any, Dict, List, Optional, Sequence | ||
|
|
||
| logger = logging.getLogger(__name__) | ||
|
|
2 participants
Summary
Changes
P0 Critical Fixes
codex_cli.py,codex_client.py,codex_implementer.py)usage_tracker.pywith functional telemetrybehavioral_manager.py(-22% lines)P1 Executor Decomposition
Renamed
executor.py→command_executor.pyand createdexecutor/package:ast_analysis.pyutils.pygit_operations.pytesting.pychange_management.pytelemetry.pyquality.pyagent_orchestration.pyconsensus.pyTest plan
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
--pal-reviewflag to enable automated code review feedback after executing changesRemoved Features
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.