🧹 Refactor print_plan_details to improve maintainability#789
Conversation
|
Merging to
After your PR is submitted to the merge queue, this comment will be automatically updated with its status. If the PR fails, failure details will also be posted here |
PR SummaryLow Risk Overview This reduces duplication and cyclomatic complexity in the dry-run plan output path while preserving the existing action-selection behavior (including multi- Reviewed by Cursor Bugbot for commit c3f4b5b. Configure here. |
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| def _format_action_text(label: str, icon: str, color: str) -> str: | ||
| """Format action label with appropriate color and icon based on USE_COLORS.""" | ||
| if USE_COLORS: | ||
| return f"({color}{icon} {label}{Colors.ENDC})" | ||
| return f"({icon} {label})" |
There was a problem hiding this comment.
📝 Info: Formatting output is preserved correctly across all action types
Verified that the string formatting in _format_action_text (main.py:580-584) produces identical output to the old inline code for all three cases:
- Mixed: icon
"⚠️ "(emoji+space) + format{icon} {label}→⚠️ Mixed(two spaces), matching the oldf"(⚠️ {action_label})". - Block/Allow: icon
"⛔"or"✅"(no trailing space) + format →⛔ Blockor✅ Allow(one space), matching old code.
Existing tests at tests/test_plan_details.py:41 ("⚠️ Mixed") and tests/test_ux.py:576-578 confirm the expected spacing. The color wrapping logic ({color}{icon} {label}{Colors.ENDC}) also matches the old pattern.
Was this helpful? React with 👍 or 👎 to provide feedback.
| def _get_action_text(folder: dict[str, Any]) -> str: | ||
| """Determine the action label (Block/Allow/Mixed) for a given folder.""" | ||
| action_val = None | ||
|
|
||
| # Check for multiple rule groups first | ||
| if "rule_groups" in folder and folder["rule_groups"]: | ||
| actions = {rg.get("action") for rg in folder["rule_groups"]} | ||
| if len(actions) > 1: | ||
| return _format_action_text("Mixed", "⚠️ ", Colors.WARNING) | ||
| action_val = next(iter(actions)) | ||
| # Fallback to single action if not set | ||
| elif "action" in folder: | ||
| action_val = folder["action"] | ||
|
|
||
| if action_val == 0: | ||
| return _format_action_text("Block", "⛔", Colors.FAIL) | ||
| if action_val == 1: | ||
| return _format_action_text("Allow", "✅", Colors.GREEN) | ||
|
|
||
| # Default fallback | ||
| return _format_action_text("Block (Default)", "⛔", Colors.FAIL) |
There was a problem hiding this comment.
📝 Info: New helper functions read module-level globals (USE_COLORS, Colors) same as the old inline code
Both _format_action_text and _get_action_text reference USE_COLORS and Colors as module-level globals, exactly as the old inline code within print_plan_details did. Since USE_COLORS is computed once at import time (and tests monkeypatch it), this extraction doesn't introduce any new issues with global state access. Tests in test_plan_details.py and test_ux.py that patch USE_COLORS will continue to work correctly with the extracted functions.
Was this helpful? React with 👍 or 👎 to provide feedback.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: elif changes fallback behavior from original sequential checks
- Replaced the elif with a separate condition so folder-level action is applied whenever rule_groups did not produce 0 or 1, restoring the intended cascade.
Or push these changes by commenting:
@cursor push 61cce4f48b
Preview (61cce4f48b)
diff --git a/main.py b/main.py
--- a/main.py
+++ b/main.py
@@ -594,8 +594,7 @@
if len(actions) > 1:
return _format_action_text("Mixed", "⚠️ ", Colors.WARNING)
action_val = next(iter(actions))
- # Fallback to single action if not set
- elif "action" in folder:
+ if action_val not in (0, 1) and "action" in folder:
action_val = folder["action"]
if action_val == 0:You can send follow-ups to the cloud agent here.
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit c3f4b5b. Configure here.
| action_val = next(iter(actions)) | ||
| # Fallback to single action if not set | ||
| elif "action" in folder: | ||
| action_val = folder["action"] |
There was a problem hiding this comment.
elif changes fallback behavior from original sequential checks
Low Severity
The refactored _get_action_text uses elif to check folder["action"], but the original code used separate if not action_text guards. In the original, when rule_groups existed with a single action value that was neither 0 nor 1 (e.g., None from a missing key), action_text stayed empty and the code fell back to folder["action"]. With elif, the folder["action"] fallback is now unreachable whenever rule_groups is present and truthy, potentially changing the result from the correct action label to "Block (Default)".
Reviewed by Cursor Bugbot for commit c3f4b5b. Configure here.
…elds unrecognized action
There was a problem hiding this comment.
(1 files improve in Code Health)
Gates Passed
See analysis details in CodeScene
View Improvements
| File | Code Health Impact | Categories Improved |
|---|---|---|
| main.py | 1.66 → 1.69 | Lines of Code in a Single File, Complex Method, Bumpy Road Ahead, Overall Code Complexity, Deep, Nested Complexity |
Quality Gate Profile: Pay Down Tech Debt
Install CodeScene MCP: safeguard and uplift AI-generated code. Catch issues early with our IDE extension and CLI tool.
| # Fallback to single top-level action if rule_groups didn't yield a | ||
| # recognized action (e.g., empty, missing, or None values). | ||
| if action_val not in (0, 1) and "action" in folder: | ||
| action_val = folder["action"] |
There was a problem hiding this comment.
📝 Info: Fallback condition action_val not in (0, 1) is logically equivalent to old if not action_text
The old code used the truthy check if not action_text to determine whether the rule_groups branch had set an action. The new code at main.py:600 uses action_val not in (0, 1) instead. This is equivalent because: (1) if rule_groups yielded action 0 or 1, those are the only values that would have set action_text in the old code; (2) if rule_groups yielded None (from rg.get("action") on a dict without an "action" key), None not in (0, 1) is True, correctly falling through to the top-level folder["action"] check; (3) if rule_groups was empty/missing, action_val stays None (initialized at main.py:589), also correctly falling through.
Was this helpful? React with 👍 or 👎 to provide feedback.
🎯 What: Extracted the action text formatting logic from
print_plan_detailsinto_get_action_textand_format_action_texthelper functions.\n\n💡 Why:print_plan_detailswas overly long and complex, mixing iteration, printing, and action-label resolution. By extracting the action-label resolution into dedicated helpers, we reduce cyclomatic complexity and improve modularity and readability.\n\n✅ Verification: Ranuv run pytest tests/to confirm that all tests pass. Formatted and linted code withruff.\n\n✨ Result: Improved codebase maintainability without changing behavior. The core print logic is now clean and linear.