-
Notifications
You must be signed in to change notification settings - Fork 3
Implement multi-factor risk assessment for refactoring operations #52
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
Important Review skippedBot user detected. To trigger a single review, invoke the You can disable this status message by setting the 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. Comment |
Co-authored-by: omsherikar <180152315+omsherikar@users.noreply.github.com>
Co-authored-by: omsherikar <180152315+omsherikar@users.noreply.github.com>
…ng parameters Co-authored-by: omsherikar <180152315+omsherikar@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR implements a comprehensive multi-factor risk assessment system for refactoring operations. The system evaluates five weighted risk factors (change type 30%, test coverage 25%, dependencies 20%, impact scope 15%, complexity 10%) to generate risk scores from 0.0 to 1.0, providing detailed breakdowns and visual indicators throughout the CLI and result displays.
Key Changes:
- New
RiskAssessorclass with five-factor weighted risk calculation algorithm - Integration of risk assessment into all four refactorer types with appropriate change type classifications
- Enhanced visualization with color-coded risk indicators and detailed factor breakdowns in CLI and result outputs
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
refactron/core/risk_assessment.py |
Core risk assessment module with weighted scoring, test coverage detection, dependency analysis, and impact scope calculation |
refactron/refactorers/magic_number_refactorer.py |
Integrated risk assessment with EXTRACTION change type for constant extraction operations |
refactron/refactorers/reduce_parameters_refactorer.py |
Added risk calculation with API_CHANGE type for parameter reduction operations |
refactron/refactorers/simplify_conditionals_refactorer.py |
Implemented risk assessment with LOGIC_CHANGE type for control flow modifications |
refactron/refactorers/add_docstring_refactorer.py |
Added risk calculation with RENAMING type, capped at 0.1 for documentation-only changes |
refactron/core/refactor_result.py |
Enhanced diff display with risk icons and detailed factor breakdowns |
refactron/cli.py |
Added color-coded risk visualization helper function with detailed breakdowns |
tests/test_risk_assessment.py |
Comprehensive test suite with 20 tests covering all risk calculation paths |
tests/test_refactorers.py |
Updated existing tests to accommodate realistic risk scores from advanced assessment |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…nstant Co-authored-by: omsherikar <180152315+omsherikar@users.noreply.github.com>
|
@copilot Some CI are failing fix them |
Co-authored-by: omsherikar <180152315+omsherikar@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 13 comments.
Comments suppressed due to low confidence (1)
refactron/core/risk_assessment.py:7
- Import of 'Set' is not used.
from typing import Dict, List, Optional, Set
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
refactron/core/risk_assessment.py
Outdated
| change_type: ChangeType, | ||
| affected_lines: Optional[List[int]] = None, | ||
| operation_description: str = "", | ||
| ) -> tuple[float, RiskFactors]: |
Copilot
AI
Dec 17, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The type hint uses lowercase tuple[float, RiskFactors] which requires Python 3.9+, but the project specifies requires-python = ">=3.8" in pyproject.toml. This will cause a TypeError on Python 3.8. Import Tuple from the typing module (which is already imported at line 7) and change the return type to Tuple[float, RiskFactors] for Python 3.8 compatibility.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot apply changes based on this feedback
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in commit 4c9de4b. Changed type hint from tuple[float, RiskFactors] to Tuple[float, RiskFactors] for Python 3.8 compatibility.
| source_code: str, | ||
| change_type: ChangeType, | ||
| affected_lines: Optional[List[int]] = None, | ||
| operation_description: str = "", |
Copilot
AI
Dec 17, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The operation_description parameter is defined but never used in the calculate_risk_score method. Either remove it if it's not needed, or incorporate it into the risk assessment logic (e.g., for logging or storing in RiskFactors metadata). Currently it adds unnecessary API complexity without providing value.
refactron/cli.py
Outdated
| if risk_score < 0.3: | ||
| risk_color = "green" | ||
| risk_label = "✓ SAFE" | ||
| elif risk_score < 0.5: | ||
| risk_color = "blue" | ||
| risk_label = "⚡ LOW RISK" | ||
| elif risk_score < 0.7: | ||
| risk_color = "yellow" | ||
| risk_label = "⚠ MODERATE RISK" | ||
| elif risk_score < 0.9: | ||
| risk_color = "red" | ||
| risk_label = "❌ HIGH RISK" | ||
| else: | ||
| risk_color = "bright_red" | ||
| risk_label = "🔴 CRITICAL RISK" |
Copilot
AI
Dec 17, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The risk level thresholds (0.3, 0.5, 0.7, 0.9) and their corresponding labels are duplicated between _print_operation_with_risk in cli.py and _get_risk_icon in refactor_result.py. This creates maintenance burden and risk of inconsistency. Consider extracting a shared utility function or using the get_risk_level method from RiskAssessor class to determine these labels consistently across the codebase.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot apply changes based on this feedback
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in commit 3f2103e. Centralized risk display logic in RiskAssessor class:
- Added
RISK_DISPLAY_LABELSandRISK_DISPLAY_COLORSclass constants - Created
get_risk_display_label()andget_risk_display_color()helper methods - Updated both
cli.pyandrefactor_result.pyto use these centralized methods - Eliminated all duplicated threshold values (0.3, 0.5, 0.7, 0.9)
refactron/core/risk_assessment.py
Outdated
| elif total_deps < 30: | ||
| return 0.5 | ||
| else: | ||
| return 0.7 | ||
|
|
||
| except SyntaxError: | ||
| return 0.5 # Unknown, assume moderate risk | ||
|
|
Copilot
AI
Dec 17, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The dependency risk thresholds (5, 15, 30) and return values (0.1, 0.3, 0.5, 0.7) are hardcoded magic numbers. Consider extracting these as class constants for better maintainability and to make it easier to tune the risk assessment in the future. For example: DEPENDENCY_THRESHOLD_LOW = 5, DEPENDENCY_THRESHOLD_MEDIUM = 15, DEPENDENCY_THRESHOLD_HIGH = 30.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot apply changes based on this feedback
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…agic numbers, and populate test_file_exists Co-authored-by: omsherikar <180152315+omsherikar@users.noreply.github.com>
…gic in RiskAssessor Co-authored-by: omsherikar <180152315+omsherikar@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def _get_risk_icon(self, risk_score: float) -> str: | ||
| """Get visual indicator for risk level.""" | ||
| from refactron.core.risk_assessment import RiskAssessor | ||
|
|
||
| assessor = RiskAssessor() | ||
| return assessor.get_risk_display_label(risk_score) |
Copilot
AI
Dec 17, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A new RiskAssessor instance is created every time _get_risk_icon is called, which happens for each operation in show_diff. Since show_diff can iterate over many operations (line 53), this could create many unnecessary RiskAssessor objects. Consider creating a single RiskAssessor instance at the class level or reusing one instance within the show_diff method.
| from refactron.core.risk_assessment import RiskAssessor | ||
|
|
||
| # Determine risk level and color using centralized logic | ||
| assessor = RiskAssessor() |
Copilot
AI
Dec 17, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A new RiskAssessor instance is created in _print_operation_with_risk for every operation printed. If this function is called multiple times (e.g., in a loop for many operations), it creates unnecessary objects. Consider accepting a RiskAssessor instance as a parameter or creating it once in the calling code and reusing it.
| def _print_operation_with_risk(operation) -> None: | ||
| """Print refactoring operation with color-coded risk visualization.""" | ||
| from rich.panel import Panel | ||
|
|
||
| from refactron.core.risk_assessment import RiskAssessor | ||
|
|
||
| # Determine risk level and color using centralized logic | ||
| assessor = RiskAssessor() | ||
| risk_score = operation.risk_score | ||
| risk_color = assessor.get_risk_display_color(risk_score) | ||
| risk_label = assessor.get_risk_display_label(risk_score) | ||
|
|
||
| # Build content | ||
| content = f"[bold]{operation.operation_type}[/bold]\n" | ||
| content += f"Location: {operation.file_path}:{operation.line_number}\n" | ||
| content += f"Risk: [{risk_color}]{risk_score:.2f} - {risk_label}[/{risk_color}]\n" | ||
| content += f"\n{operation.description}" | ||
|
|
||
| # Add risk factors if available | ||
| if "risk_factors" in operation.metadata: | ||
| risk_factors = operation.metadata["risk_factors"] | ||
| content += "\n\n[bold]Risk Breakdown:[/bold]\n" | ||
| content += f" • Impact Scope: {risk_factors.get('impact_scope', 0):.2f}\n" | ||
| content += f" • Change Type: {risk_factors.get('change_type_risk', 0):.2f}\n" | ||
| content += f" • Test Coverage: {risk_factors.get('test_coverage_risk', 0):.2f}\n" | ||
| content += f" • Dependencies: {risk_factors.get('dependency_risk', 0):.2f}\n" | ||
| content += f" • Complexity: {risk_factors.get('complexity_risk', 0):.2f}" | ||
|
|
||
| if risk_factors.get("test_file_exists"): | ||
| content += "\n\n✓ Test file exists" | ||
| else: | ||
| content += "\n\n⚠ No test file found - higher risk" | ||
|
|
||
| panel = Panel(content, border_style=risk_color, expand=False) | ||
| console.print(panel) | ||
|
|
Copilot
AI
Dec 17, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The function _print_operation_with_risk is defined but never called anywhere in the codebase. The refactor command at line 309 uses result.show_diff() instead. Either this function should be integrated into the CLI workflow (replacing or complementing show_diff), or it should be removed if it's not needed. If it's intended for future use, consider adding a TODO comment explaining when and how it should be used.
| for py_file in self.project_root.rglob("*.py"): | ||
| if py_file == file_path: | ||
| continue | ||
|
|
Copilot
AI
Dec 17, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The for loop that begins on line 448 iterates through all Python files but doesn't do anything with them - it just continues immediately after the check on line 450. This loop appears to serve no purpose, as all the actual work happens in the second for loop starting at line 457. This first loop should be removed to eliminate dead code.
| for py_file in self.project_root.rglob("*.py"): | |
| if py_file == file_path: | |
| continue |
| # Find dependent test files | ||
| test_file = self._find_test_file(file_path) | ||
| if test_file: | ||
| impact["dependent_tests"].append(str(test_file)) |
Copilot
AI
Dec 17, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test file lookup is called every time analyze_dependency_impact is invoked and may result in duplicate test files in the dependent_tests list. If analyze_dependency_impact is called multiple times for the same file, the same test file will be appended repeatedly. Consider checking if the test file is already in the list before appending, or ensuring this method is only called once per file.
| impact["dependent_tests"].append(str(test_file)) | |
| test_file_str = str(test_file) | |
| if test_file_str not in impact["dependent_tests"]: | |
| impact["dependent_tests"].append(test_file_str) |
| continue | ||
|
|
||
| # Limit the number of files scanned to avoid excessive I/O on large projects. | ||
| max_files = getattr(self, "max_dependency_scan_files", 1000) |
Copilot
AI
Dec 17, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The max_files limit is retrieved using getattr with a default of 1000, but this value is not defined as a class constant. Consider extracting this magic number to a class constant like MAX_DEPENDENCY_SCAN_FILES = 1000 for consistency with other constants in the class and to make it easier to configure.
Implementation Plan for Advanced Risk Assessment
refactron/core/risk_assessment.py)Summary
Successfully implemented advanced risk assessment with multi-factor scoring. All 54 risk assessment tests pass with 89% coverage. Code is Python 3.8+ compatible with centralized risk display logic.
Original prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.