feat(ui): Rich-based terminal UI module with modern interaction patterns #581
Conversation
Complete UI overhaul with modern CLI patterns: - Console: Themed singleton with semantic message methods (success, error, warning, info, command, step) - Prompts: 6-option command menu (run/trust/dry-run/explain/edit/cancel) with keyboard navigation and Tab to add instructions - Progress: Spinners, progress bars, and step counters - Errors: Structured error panels with suggested fixes Auto-detection for common errors (container conflict, port in use, etc.) - Explain: Command breakdown with explanations for docker, apt, pip, git - Trust: Session and global trust levels to reduce prompt friction - Panels: AI response panels, status panels, code panels, diff panels Patterns based on Claude Code, Cursor, and GitHub Copilot CLI. Closes #[UX issues]
mikejmorgan-ai
added
integration
📝 WalkthroughWalkthroughIntroduces comprehensive UI module for Cortex with themed console output, interactive command prompts, error handling with suggestions, command explanation utilities, progress indicators, trust management system, themed panels, and core theme definitions. Includes test coverage for all new components. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
Summary of ChangesHello @mikejmorgan-ai, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request delivers a significant enhancement to the application's command-line interface by integrating the powerful Rich library. The changes aim to elevate the user experience by providing a more visually appealing, interactive, and intelligent terminal environment. This includes standardized message types, advanced interactive prompts, proactive error suggestions, and a robust trust system, all designed to make CLI interactions more intuitive and efficient. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive terminal UI module built on the Rich library, significantly enhancing the user experience with modern interaction patterns. The new features include semantic console messages, an interactive command menu, structured error displays with suggested fixes, trust management, and various progress indicators. The implementation demonstrates a good understanding of Rich's capabilities and provides a solid foundation for an intuitive CLI. The code is generally well-structured and includes initial test coverage. However, there are several areas for improvement regarding robustness, API consistency, and error handling, particularly concerning user input and dynamic command parsing.
| elif error_type == "permission_denied": | ||
| fix_cmd = fix_cmd.format(original_command=command) |
There was a problem hiding this comment.
The auto_suggest_fix function currently handles container_name, port, and original_command placeholders. However, the not_found error's fix_template uses {package} which is not formatted, leading to an unformatted suggestion. Ensure all placeholders in COMMON_ERRORS fix_template strings are correctly handled and formatted.
| elif error_type == "permission_denied": | |
| fix_cmd = fix_cmd.format(original_command=command) | |
| elif error_type == "permission_denied": | |
| fix_cmd = fix_cmd.format(original_command=command) | |
| elif error_type == "not_found": | |
| # This requires identifying the missing package, which might be complex. | |
| # For now, ensure the template is handled or provide a generic message. | |
| pass # Placeholder for future logic to identify and format {package} | |
| return SuggestedFix(command=fix_cmd, description=info["fix_description"]) |
| def indeterminate_progress(message: str) -> Progress: | ||
| return Progress(SpinnerColumn(style="cortex"), TextColumn("[info]{task.description}[/]"), console=console.rich, transient=True) | ||
|
|
||
|
|
||
| def download_progress(description: str, total: float) -> Progress: | ||
| return Progress(SpinnerColumn(style="cortex"), TextColumn("[info]{task.description}[/]"), BarColumn(bar_width=30, style="secondary", complete_style="cortex", finished_style="success"), TaskProgressColumn(), "•", TimeRemainingColumn(), console=console.rich, transient=True) |
There was a problem hiding this comment.
The functions indeterminate_progress and download_progress return Progress objects directly, unlike spinner and progress_bar which are context managers. This creates an inconsistency in the API for managing progress indicators. For better consistency and idiomatic rich usage, consider converting these functions into context managers as well.
| def indeterminate_progress(message: str) -> Progress: | |
| return Progress(SpinnerColumn(style="cortex"), TextColumn("[info]{task.description}[/]"), console=console.rich, transient=True) | |
| def download_progress(description: str, total: float) -> Progress: | |
| return Progress(SpinnerColumn(style="cortex"), TextColumn("[info]{task.description}[/]"), BarColumn(bar_width=30, style="secondary", complete_style="cortex", finished_style="success"), TaskProgressColumn(), "•", TimeRemainingColumn(), console=console.rich, transient=True) | |
| @contextmanager | |
| def indeterminate_progress(message: str): | |
| progress = Progress(SpinnerColumn(style="cortex"), TextColumn("[info]{task.description}[/]"), console=console.rich, transient=True) | |
| with progress: | |
| progress.add_task(message, total=None) | |
| yield progress | |
| @contextmanager | |
| def download_progress(description: str, total: float): | |
| progress = Progress(SpinnerColumn(style="cortex"), TextColumn("[info]{task.description}[/]"), BarColumn(bar_width=30, style="secondary", complete_style="cortex", finished_style="success"), TaskProgressColumn(), "•", TimeRemainingColumn(), console=console.rich, transient=True) | |
| with progress: | |
| progress.add_task(description, total=total) | |
| yield progress |
| while True: | ||
| response = input(" > ").strip().lower() | ||
| for action in actions: | ||
| if response == action[0].lower() or response == action.lower(): | ||
| return action | ||
| console.warning(f"Please enter one of: {', '.join(a[0] for a in actions)}") |
There was a problem hiding this comment.
The input() calls in show_error and show_conflict are blocking. While acceptable for simple CLI interactions, this can limit the responsiveness of the UI and make it harder to integrate with more complex event-driven systems or asynchronous operations in the future. For a rich UI, consider exploring non-blocking input mechanisms if the application's complexity grows.
| display_value = value if len(value) < 50 else value[:47] + "..." | ||
| lines.append(f" [secondary]{key}:[/] {display_value}") |
There was a problem hiding this comment.
The truncation logic for display_value in the show_error context (value[:47] + "...") might cut off critical information for the user. While brevity is good, important details could be lost. Consider providing an option to view the full context or truncating more intelligently, perhaps by word boundaries.
| if arg.startswith('-') and i + 1 < len(args) and not args[i + 1].startswith('-'): | ||
| parts.append(CommandPart(text=f"{arg} {args[i+1]}", explanation=f"{exp}: {args[i+1]}", is_flag=True)) | ||
| i += 2 | ||
| else: |
There was a problem hiding this comment.
The logic for parsing command arguments and flags in explain_command is a bit complex, especially with the i + 1 < len(args) and not args[i + 1].startswith('-') condition. This might not cover all variations of flag-value pairs (e.g., --flag=value or flags that don't take an immediate next argument). Consider a more robust parsing approach that explicitly handles different flag formats and their associated values.
| while True: | ||
| response = input(" > ").strip().lower() | ||
| if response in ('', 'y', 'yes') and default: | ||
| return (True, False) | ||
| elif response in ('y', 'yes'): | ||
| return (True, False) | ||
| elif response in ('n', 'no'): | ||
| return (False, False) | ||
| elif response == 'a' and allow_dont_ask: | ||
| return (True, True) | ||
| elif response == '' and not default: | ||
| return (False, False) | ||
| console.warning("Please enter Y, N" + (", or A" if allow_dont_ask else "")) |
There was a problem hiding this comment.
The confirm function also uses a blocking input() call. While common in CLIs, for a Rich-based UI, this can interrupt the visual flow. Consider if there are alternative non-blocking input patterns that could be used, or if the blocking nature is acceptable for this specific interaction.
| response = input(" > ").strip() | ||
| if response.isdigit() and 0 < int(response) <= len(options): | ||
| return int(response) - 1 | ||
| console.warning(f"Please enter 1-{len(options)}") | ||
| except (KeyboardInterrupt, EOFError): | ||
| return -1 |
There was a problem hiding this comment.
The select function, like others, relies on a blocking input() call. This design choice should be consistent across all interactive prompts. If the goal is a highly dynamic and responsive UI, blocking inputs might become a bottleneck. Also, the try-except block for int(response) is good, but the warning message could be more user-friendly.
| if len(parts) > 1 and base in ('docker', 'git', 'apt', 'pip', 'npm', 'systemctl'): | ||
| return f"{base} {parts[1]}" |
There was a problem hiding this comment.
The _get_command_type method hardcodes a list of commands ('docker', 'git', 'apt', 'pip', 'npm', 'systemctl') to determine if a command has a two-part type (e.g., docker run). This list will need manual updates as new commands are supported or if existing ones change. Consider making this list configurable or extensible, perhaps loaded from a separate configuration file or dynamically discovered, to improve maintainability and flexibility.
| for method in ["success", "error", "warning", "info", "command", "step", "thinking", "secondary"]: | ||
| assert callable(getattr(console, method)) |
There was a problem hiding this comment.
The test_message_methods in TestConsole only asserts that the console methods are callable. This is a very basic check. To ensure correctness, these tests should verify that the methods actually produce the expected output or correctly interact with the underlying rich.console object (e.g., by mocking _console.print and checking its call arguments).
| def test_auto_suggest_container_conflict(self): | ||
| fix = auto_suggest_fix('container name "/ollama" already in use') | ||
| assert fix is not None | ||
| assert "docker rm" in fix.command |
There was a problem hiding this comment.
The test_auto_suggest_container_conflict is a good start, but the auto_suggest_fix function has multiple error types and complex regex matching. More comprehensive test cases are needed to cover all COMMON_ERRORS types (e.g., port_in_use, permission_denied, not_found) and various edge cases, including when placeholders are not found or multiple matches occur.
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
| @@ -0,0 +1,66 @@ | |||
| """Cortex Progress - Progress indicators and spinners.""" | |||
|
|
|||
| from typing import Optional | |||
| @@ -0,0 +1,149 @@ | |||
| """Cortex Prompts - Interactive prompt system with modern UX patterns.""" | |||
|
|
|||
| import sys | |||
| from typing import Optional, List | ||
| from dataclasses import dataclass | ||
| from rich.panel import Panel | ||
| from .theme import SYMBOLS, PANEL_STYLES |
| @@ -0,0 +1,64 @@ | |||
| """Tests for Cortex UI module.""" | |||
| import pytest | |||
| from cortex.ui.theme import COLORS, SYMBOLS, CORTEX_THEME, PANEL_STYLES | |||
| import pytest | ||
| from cortex.ui.theme import COLORS, SYMBOLS, CORTEX_THEME, PANEL_STYLES | ||
| from cortex.ui.console import console, CortexConsole | ||
| from cortex.ui.prompts import CommandPrompt, MenuAction, PromptResult, COMMAND_MENU_OPTIONS |
| from cortex.ui.theme import COLORS, SYMBOLS, CORTEX_THEME, PANEL_STYLES | ||
| from cortex.ui.console import console, CortexConsole | ||
| from cortex.ui.prompts import CommandPrompt, MenuAction, PromptResult, COMMAND_MENU_OPTIONS | ||
| from cortex.ui.progress import StepsProgress |
| from cortex.ui.console import console, CortexConsole | ||
| from cortex.ui.prompts import CommandPrompt, MenuAction, PromptResult, COMMAND_MENU_OPTIONS | ||
| from cortex.ui.progress import StepsProgress | ||
| from cortex.ui.trust import TrustManager, TrustScope |
| from cortex.ui.prompts import CommandPrompt, MenuAction, PromptResult, COMMAND_MENU_OPTIONS | ||
| from cortex.ui.progress import StepsProgress | ||
| from cortex.ui.trust import TrustManager, TrustScope | ||
| from cortex.ui.explain import parse_command, get_explanation |
| from cortex.ui.progress import StepsProgress | ||
| from cortex.ui.trust import TrustManager, TrustScope | ||
| from cortex.ui.explain import parse_command, get_explanation | ||
| from cortex.ui.errors import auto_suggest_fix, COMMON_ERRORS |
| data = json.load(f) | ||
| self._global_commands = set(data.get("commands", [])) | ||
| self._global_command_types = set(data.get("command_types", [])) | ||
| except (json.JSONDecodeError, IOError): |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🤖 Fix all issues with AI agents
In @cortex/ui/__init__.py:
- Around line 3-10: The imports in cortex/ui/__init__.py are not alphabetized
causing Ruff I001; reorder the top-level import statements alphabetically by
module name (for example: .console, .errors, .explain, .panels, .progress,
.prompts, .theme, .trust) while preserving the existing imported symbols
(CortexConsole/console, show_error/etc., explain_command/etc., ai_thinking/etc.,
spinner/etc., CommandPrompt/etc., COLORS/etc., TrustManager/etc.) so only the
import lines are re-sorted and no other code changes are made.
In @cortex/ui/console.py:
- Around line 32-35: The method signature for error uses legacy typing
Optional[str]; update it to use the new union syntax (str | None) in the error
method definition (error(self, message: str, details: str | None) -> None) and
remove the now-unused Optional import from the module if present; ensure no
other references in cortex/ui/console.py still use Optional so
linting/type-checking passes.
- Around line 3-8: Imports at the top of the module are unsorted and you’re
still importing typing.Optional; reorder imports (stdlib first, third-party rich
packages next, then local package imports) and update any type annotations that
use Optional[...] to use the PEP 604 union syntax (e.g., Foo | None). Remove the
now-unused Optional import from typing and ensure all function signatures,
variable annotations, and return types that used Optional are updated to the X |
None form (search for "Optional[" and replace accordingly), keeping the existing
symbols like CORTEX_THEME, SYMBOLS, and PANEL_STYLES untouched.
- Around line 11-27: Remove trailing whitespace and extra blank-line whitespace
throughout the CortexConsole class, add concise docstrings to the public members
__new__, rich (property) and print to meet docstring guidelines, and make the
singleton initialization in __new__ thread-safe (e.g., guard creation of
_instance/_console with a threading.Lock or use double-checked locking) so
simultaneous CortexConsole() calls cannot create multiple instances; reference
the class name CortexConsole, the __new__ method, the _instance and _console
attributes, the rich property, and the print method when applying these changes.
In @cortex/ui/errors.py:
- Around line 86-89: The container name regex in the container_conflict branch
uses r'name ["\']?/?(\w+)' which fails to match hyphenated names; update the
pattern used in the error_type == "container_conflict" block (where match is
assigned and fix_cmd is formatted) to capture hyphens as well (e.g., use a
character class like [\w-]+ or [-\w]+ instead of \w+) so match.group(1) will
include names such as "my-nginx-container" before calling
fix_cmd.format(container_name=...).
- Around line 17-22: The "not_found" entry in COMMON_ERRORS uses a {package}
placeholder but auto_suggest_fix does not populate "package", leaving the
suggested fix unsubstituted; either change
COMMON_ERRORS["not_found"]["fix_template"] to a generic/safe placeholder such as
{original_command} or implement extraction logic inside auto_suggest_fix to
detect "command not found" errors (parse the missing command from the error
message) and set a "package" value before formatting the template—update the
branch in auto_suggest_fix that handles "not_found"/"not found" so it supplies
the "package" key (or switch the template) and ensure the formatted fix uses
only keys that are guaranteed to be present.
In @cortex/ui/panels.py:
- Around line 12-14: The ai_thinking function declares an unused parameter
animated; remove the animated parameter from ai_thinking's signature and any
callers, or if you prefer to keep the API, document it and add a TODO and a
brief comment inside ai_thinking noting planned animation support; reference the
ai_thinking function and SYMBOLS['thinking']/console.print call so you update
the signature and any places that call ai_thinking accordingly.
In @cortex/ui/progress.py:
- Around line 61-66: indeterminate_progress and download_progress create a
Progress but never add a task or use the incoming parameters; modify each
function to create the Progress, call progress.add_task(...) using the incoming
message/description and total (for download_progress) so callers get a
ready-to-use Progress with an active task (e.g., in indeterminate_progress call
add_task(message, total=None) and in download_progress call
add_task(description, total=total)), then return the Progress; ensure you
reference the existing function names indeterminate_progress and
download_progress and preserve existing columns and transient/console settings.
In @cortex/ui/trust.py:
- Around line 38-41: _save_global_trust currently writes CONFIG_FILE without
handling write failures; wrap the write in a try/except that catches OSError
(including IOError/PermissionError), perform an atomic save (write to a temp
file in the same directory then os.replace to CONFIG_FILE) and on failure log
the error via the module/logger and avoid raising (or surface a clear error) so
the application doesn't crash; ensure you still create the parent dir
(CONFIG_FILE.parent.mkdir) before the temp write and continue to serialize
self._global_commands and self._global_command_types as before.
- Around line 59-63: Replace the insecure string prefix check with a proper
filesystem containment check: convert both the candidate directory and each
trusted entry in self._session_directories to pathlib.Path objects, call
resolve() (or absolute()) on both to normalize symlinks, and then test
containment using Path.is_relative_to(trusted_path) (or fallback to comparing
trusted_path == candidate_path or catching ValueError from
candidate_path.relative_to(trusted_path)). Update the block that currently uses
directory.startswith(trusted_dir) to perform this Path-based check so only true
subpaths of a trusted directory pass.
In @tests/test_ui.py:
- Line 6: The import StepsProgress in tests/test_ui.py is unused; either remove
the import line referencing StepsProgress or add a test that exercises
StepsProgress (e.g., instantiate StepsProgress, verify initial state, call
step() and assert the current counter increments). Update the file to eliminate
the unused import or commit the new test referencing the StepsProgress symbol so
the import is used.
🧹 Nitpick comments (16)
cortex/ui/progress.py (2)
9-20: Add docstrings for public API.
StepsProgressis a public class but lacks a docstring. Per coding guidelines, docstrings are required for all public APIs.📝 Suggested docstring
class StepsProgress: + """Track and display step-by-step progress through a multi-step operation. + + Args: + title: The title displayed for this progress sequence. + total: The total number of steps expected. + """ def __init__(self, title: str, total: int):
23-28: Add docstring and return type hint forspinner.This public context manager lacks a docstring and explicit return type annotation. As per coding guidelines, type hints and docstrings are required for public APIs.
📝 Suggested improvements
+from typing import Optional, Generator +from contextlib import contextmanager + @contextmanager -def spinner(message: str): +def spinner(message: str) -> Generator[Progress, None, None]: + """Display an indeterminate spinner with a message. + + Args: + message: The message to display alongside the spinner. + + Yields: + The Rich Progress instance. + """ progress = Progress(...)tests/test_ui.py (2)
42-46: Consider testing via public API instead of private method.Testing
_get_command_typedirectly is fragile since private methods may change. Consider testing through the publicis_trustedoradd_session_trustmethods instead.💡 Alternative approach
class TestTrust: def test_trust_by_command_type(self): manager = TrustManager() manager.add_session_trust("docker run -d nginx", TrustScope.COMMAND_TYPE) # Should trust any "docker run" command assert manager.is_trusted("docker run --rm alpine") assert not manager.is_trusted("docker pull nginx")
12-61: Test coverage appears minimal for new modules.The current tests provide basic smoke tests but may not meet the >80% coverage guideline. Key untested areas include:
- Progress indicators (
spinner,progress_bar,steps)- Panel helpers (
ai_response,code_panel,diff_panel)- Error display functions (
show_error,show_warning)- Edge cases in
auto_suggest_fix(port patterns, permission denied)Consider expanding coverage in a follow-up. Do you want me to help generate additional test cases?
cortex/ui/explain.py (2)
40-45: Add docstring forparse_command.This is a public function exported via
__init__.pyand tested intest_ui.py. Per coding guidelines, docstrings are required for public APIs.📝 Suggested docstring
def parse_command(cmd: str) -> Tuple[str, List[str]]: + """Parse a shell command into base command and arguments. + + Args: + cmd: The shell command string to parse. + + Returns: + A tuple of (base_command, list_of_arguments). + """ try: parts = shlex.split(cmd)
91-92: Moveimport reto module level.Placing imports inside functions is generally discouraged unless for conditional/lazy loading. Since
reis a lightweight stdlib module, moving it to the top improves readability and follows PEP 8 conventions.🔧 Suggested fix
At the top of the file:
from typing import List, Tuple from dataclasses import dataclass import shlex +import re from rich.panel import PanelIn the function:
def explain_pipeline(cmd: str) -> None: - import re parts = re.split(r'\s*(\||\&\&|\|\|)\s*', cmd)cortex/ui/errors.py (1)
25-54: Add docstrings for public error display functions.
show_error,show_conflict, andshow_warningare public APIs exported in__init__.pybut lack docstrings. Per coding guidelines, docstrings are required for all public APIs.Also applies to: 57-69
cortex/ui/panels.py (1)
17-28: Add docstrings for public panel functions.These panel helpers are part of the public API surface but lack docstrings. Per coding guidelines, docstrings are required for all public APIs.
Also applies to: 31-34, 37-38, 46-52, 55-63, 66-67
cortex/ui/trust.py (3)
17-26: Add class and method docstrings for public API.Per coding guidelines, docstrings are required for all public APIs. The
TrustManagerclass and its public methods (is_trusted,add_session_trust,add_directory_trust,add_global_trust,remove_trust,clear_session_trust,clear_all_trust,list_trusted) lack docstrings.Example docstring for the class
class TrustManager: """Manages command trust levels to reduce interactive prompt friction. Supports session-scoped trust (cleared on restart) and global trust (persisted to ~/.config/cortex/trust.json). Trust can be granted at command, command-type, or directory granularity. """ CONFIG_FILE = Path.home() / ".config" / "cortex" / "trust.json"
28-36: Silent failure on corrupted config file.Loading silently swallows
IOErrorandJSONDecodeError, which is acceptable for robustness, but consider logging a warning so users know their trust config was reset.Optional: Add logging for corrupted config
except (json.JSONDecodeError, IOError): - pass + # Could not load trust config; starting fresh + pass # Consider logging a warning here
43-50: Consider expanding command type detection.The
_get_command_typemethod handles common commands well. However, commands likesudo docker runorpython -m pip installwon't derive the expected type.This is a nice-to-have enhancement for future iterations—current implementation covers the primary use cases.
cortex/ui/console.py (1)
37-38: Double space after warning symbol.There's a double space between
{SYMBOLS['warning']}[/]and[warning]which may be intentional for alignment, but appears inconsistent with other methods likeinfo(line 41) which use single space.def warning(self, message: str) -> None: - self._console.print(f"[warning_symbol]{SYMBOLS['warning']}[/] [warning]{message}[/]") + self._console.print(f"[warning_symbol]{SYMBOLS['warning']}[/] [warning]{message}[/]")cortex/ui/prompts.py (4)
12-35: Add docstrings to public classes.Per coding guidelines, docstrings are required for all public APIs.
MenuAction,MenuOption, andPromptResultshould have brief documentation.Example docstrings
class MenuAction(Enum): """Actions available in the command execution menu.""" RUN = "run" # ... @dataclass class MenuOption: """A single option in the command menu.""" key: str label: str action: MenuAction description: str = "" @dataclass class PromptResult: """Result returned from CommandPrompt.show().""" action: MenuAction command: str additional_instructions: str | None = None trust_scope: str | None = None
81-102: Input loop doesn't re-render menu after invalid input.After displaying a warning for invalid input, the menu is not re-displayed, which may confuse users who need to scroll up to see the options. Consider re-rendering the menu or at least showing available shortcuts inline.
Also,
response in '123456'checks character membership which works, butresponse.isdigit() and 1 <= int(response) <= 6would be more explicit and handle multi-digit input edge cases.Proposed improvement
def show(self) -> PromptResult: console.print(self._render_command_panel()) console.print(self._render_menu()) - + while True: try: response = input("\n > ").strip() except (KeyboardInterrupt, EOFError): return PromptResult(action=MenuAction.CANCEL, command=self.command) - - if response in '123456': + + if response.isdigit() and 1 <= int(response) <= 6: return PromptResult(action=COMMAND_MENU_OPTIONS[int(response) - 1].action, command=self.command, additional_instructions=self.additional_instructions) elif response.lower() in ('q', 'esc', 'cancel'): return PromptResult(action=MenuAction.CANCEL, command=self.command)
105-105: Return type annotation is incomplete.The return type is
tuplebut should betuple[bool, bool]for clarity.-def confirm(message: str, details: Optional[List[str]] = None, default: bool = True, allow_dont_ask: bool = True) -> tuple: +def confirm(message: str, details: list[str] | None = None, default: bool = True, allow_dont_ask: bool = True) -> tuple[bool, bool]:
134-149: Add docstring and consider return type clarity.The function returns
-1on cancel but the type hint is justint. Consider using a more explicit return type or documenting the sentinel value.Proposed improvement
-def select(message: str, options: List[str], default: int = 0) -> int: +def select(message: str, options: list[str], default: int = 0) -> int: + """Display a selection menu and return chosen index, or -1 if cancelled.""" console.print(f"\n [muted]{message}[/]\n")
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
cortex/ui/__init__.pycortex/ui/console.pycortex/ui/errors.pycortex/ui/explain.pycortex/ui/panels.pycortex/ui/progress.pycortex/ui/prompts.pycortex/ui/theme.pycortex/ui/trust.pytests/test_ui.py
🧰 Additional context used
📓 Path-based instructions (3)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Follow PEP 8 style guide
Type hints required in Python code
Docstrings required for all public APIs
Files:
tests/test_ui.pycortex/ui/__init__.pycortex/ui/theme.pycortex/ui/explain.pycortex/ui/trust.pycortex/ui/progress.pycortex/ui/panels.pycortex/ui/prompts.pycortex/ui/errors.pycortex/ui/console.py
tests/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Maintain >80% test coverage for pull requests
Files:
tests/test_ui.py
{setup.py,setup.cfg,pyproject.toml,**/__init__.py}
📄 CodeRabbit inference engine (AGENTS.md)
Use Python 3.10 or higher as the minimum supported version
Files:
cortex/ui/__init__.py
🧬 Code graph analysis (8)
tests/test_ui.py (6)
cortex/ui/console.py (2)
CortexConsole(11-75)command(43-44)cortex/ui/prompts.py (3)
CommandPrompt(48-102)MenuAction(12-19)PromptResult(31-35)cortex/ui/progress.py (1)
StepsProgress(9-20)cortex/ui/trust.py (3)
TrustManager(17-109)TrustScope(10-14)_get_command_type(43-50)cortex/ui/explain.py (2)
parse_command(40-45)get_explanation(48-60)cortex/ui/errors.py (1)
auto_suggest_fix(81-97)
cortex/ui/__init__.py (6)
cortex/ui/console.py (1)
CortexConsole(11-75)cortex/ui/prompts.py (5)
CommandPrompt(48-102)MenuAction(12-19)PromptResult(31-35)confirm(105-131)select(134-149)cortex/ui/progress.py (5)
spinner(24-28)progress_bar(32-47)steps(51-58)indeterminate_progress(61-62)download_progress(65-66)cortex/ui/errors.py (5)
show_error(25-54)show_conflict(57-69)show_warning(72-78)auto_suggest_fix(81-97)SuggestedFix(12-14)cortex/ui/explain.py (3)
explain_command(63-88)explain_pipeline(91-103)CommandPart(11-14)cortex/ui/trust.py (2)
TrustManager(17-109)TrustScope(10-14)
cortex/ui/explain.py (2)
cortex/ui/console.py (3)
rich(23-24)panel(72-75)step(46-47)cortex/ui/progress.py (1)
step(15-17)
cortex/ui/progress.py (1)
cortex/ui/console.py (4)
rich(23-24)step(46-47)success(29-30)info(40-41)
cortex/ui/panels.py (1)
cortex/ui/console.py (3)
rich(23-24)panel(72-75)code(61-63)
cortex/ui/prompts.py (1)
cortex/ui/console.py (4)
rich(23-24)panel(72-75)command(43-44)warning(37-38)
cortex/ui/errors.py (1)
cortex/ui/console.py (5)
rich(23-24)panel(72-75)command(43-44)warning(37-38)info(40-41)
cortex/ui/console.py (1)
cortex/ui/progress.py (1)
step(15-17)
🪛 GitHub Actions: CI
cortex/ui/__init__.py
[error] 3-3: Import block is un-sorted or un-formatted
🪛 GitHub Check: lint
cortex/ui/__init__.py
[failure] 3-10: Ruff (I001)
cortex/ui/init.py:3:1: I001 Import block is un-sorted or un-formatted
cortex/ui/console.py
[failure] 36-36: Ruff (W293)
cortex/ui/console.py:36:1: W293 Blank line contains whitespace
[failure] 32-32: Ruff (UP045)
cortex/ui/console.py:32:44: UP045 Use X | None for type annotations
[failure] 31-31: Ruff (W293)
cortex/ui/console.py:31:1: W293 Blank line contains whitespace
[failure] 28-28: Ruff (W293)
cortex/ui/console.py:28:1: W293 Blank line contains whitespace
[failure] 25-25: Ruff (W293)
cortex/ui/console.py:25:1: W293 Blank line contains whitespace
[failure] 21-21: Ruff (W293)
cortex/ui/console.py:21:1: W293 Blank line contains whitespace
[failure] 15-15: Ruff (W293)
cortex/ui/console.py:15:1: W293 Blank line contains whitespace
[failure] 13-13: Ruff (W293)
cortex/ui/console.py:13:1: W293 Blank line contains whitespace
[failure] 3-8: Ruff (I001)
cortex/ui/console.py:3:1: I001 Import block is un-sorted or un-formatted
🪛 GitHub Check: Lint
cortex/ui/__init__.py
[failure] 3-10: Ruff (I001)
cortex/ui/init.py:3:1: I001 Import block is un-sorted or un-formatted
cortex/ui/console.py
[failure] 36-36: Ruff (W293)
cortex/ui/console.py:36:1: W293 Blank line contains whitespace
[failure] 32-32: Ruff (UP045)
cortex/ui/console.py:32:44: UP045 Use X | None for type annotations
[failure] 31-31: Ruff (W293)
cortex/ui/console.py:31:1: W293 Blank line contains whitespace
[failure] 28-28: Ruff (W293)
cortex/ui/console.py:28:1: W293 Blank line contains whitespace
[failure] 25-25: Ruff (W293)
cortex/ui/console.py:25:1: W293 Blank line contains whitespace
[failure] 21-21: Ruff (W293)
cortex/ui/console.py:21:1: W293 Blank line contains whitespace
[failure] 15-15: Ruff (W293)
cortex/ui/console.py:15:1: W293 Blank line contains whitespace
[failure] 13-13: Ruff (W293)
cortex/ui/console.py:13:1: W293 Blank line contains whitespace
[failure] 3-8: Ruff (I001)
cortex/ui/console.py:3:1: I001 Import block is un-sorted or un-formatted
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: test (3.12)
- GitHub Check: test (3.10)
🔇 Additional comments (10)
cortex/ui/theme.py (1)
1-60: LGTM!The theme module is well-structured with clear color definitions, symbols, and panel styles. The separation of concerns (colors, symbols, theme styles, panel configurations) makes it easy to maintain and extend.
cortex/ui/__init__.py (1)
12-20: LGTM on the public API surface.The
__all__list comprehensively exports all the new UI components, and the version is appropriately defined for this initial release.cortex/ui/explain.py (1)
86-88: Blockinginput()without context may be unexpected.The
input()call blocks execution waiting for user input. While acceptable for interactive CLI use, consider:
- Documenting this behavior in the docstring
- Allowing callers to skip blocking by using the existing
show_panel=FalseparameterThe current design is reasonable for the interactive use case.
cortex/ui/errors.py (1)
19-19: Verify thekill -9suggestion is appropriate for your users.The port-in-use fix suggests
lsof -ti:{port} | xargs kill -9, which forcefully terminates processes without allowing graceful shutdown. This could cause data loss for database processes or similar.Consider offering a gentler alternative first, or documenting the risk.
cortex/ui/panels.py (1)
41-43: Consider ordering diff output: deletions first, then additions.The current implementation shows deletions before additions, which is standard. However, the list comprehension order in the code (deletions + additions) correctly implements this.
One minor improvement: empty diffs would show an empty panel. Consider handling this edge case.
cortex/ui/trust.py (2)
10-14: LGTM!The
TrustScopeenum is well-defined with clear semantic values for the different trust levels.
112-112: Module-level singleton instantiation triggers file I/O at import time.Creating
trust_managerat module load will read from disk immediately. This is typically fine, but be aware it could slow down imports or cause issues if the config file is on a network filesystem.Verify this is acceptable for your use case and consider lazy initialization if import-time I/O becomes problematic.
cortex/ui/console.py (2)
46-47: LGTM!The
stepmethod correctly formats progress output with current/total counts. This aligns well with usage incortex/ui/progress.py(lines 14-16).
68-75: LGTM!Panel methods are well-implemented with appropriate fallbacks for missing styles and optional titles.
cortex/ui/prompts.py (1)
38-45: LGTM!The menu options are well-structured with clear labels, actions, and descriptions. The 6-option design aligns with PR objectives.
| from .console import console, CortexConsole | ||
| from .theme import COLORS, SYMBOLS, CORTEX_THEME, PANEL_STYLES | ||
| from .prompts import CommandPrompt, MenuAction, PromptResult, confirm, select | ||
| from .progress import spinner, progress_bar, steps, indeterminate_progress, download_progress | ||
| from .errors import show_error, show_conflict, show_warning, auto_suggest_fix, SuggestedFix | ||
| from .explain import explain_command, explain_pipeline, CommandPart | ||
| from .trust import TrustManager, TrustScope, trust_manager | ||
| from .panels import ai_thinking, ai_response, status_panel, code_panel, diff_panel, summary_panel, welcome_banner, help_footer |
There was a problem hiding this comment.
Fix unsorted imports to pass CI.
The pipeline is failing due to Ruff I001 (unsorted imports). Reorder imports alphabetically by module name.
🔧 Sorted imports
"""Cortex UI - Modern terminal interface components."""
-from .console import console, CortexConsole
-from .theme import COLORS, SYMBOLS, CORTEX_THEME, PANEL_STYLES
-from .prompts import CommandPrompt, MenuAction, PromptResult, confirm, select
-from .progress import spinner, progress_bar, steps, indeterminate_progress, download_progress
from .errors import show_error, show_conflict, show_warning, auto_suggest_fix, SuggestedFix
from .explain import explain_command, explain_pipeline, CommandPart
-from .trust import TrustManager, TrustScope, trust_manager
from .panels import ai_thinking, ai_response, status_panel, code_panel, diff_panel, summary_panel, welcome_banner, help_footer
+from .progress import spinner, progress_bar, steps, indeterminate_progress, download_progress
+from .prompts import CommandPrompt, MenuAction, PromptResult, confirm, select
+from .console import console, CortexConsole
+from .theme import COLORS, SYMBOLS, CORTEX_THEME, PANEL_STYLES
+from .trust import TrustManager, TrustScope, trust_manager📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| from .console import console, CortexConsole | |
| from .theme import COLORS, SYMBOLS, CORTEX_THEME, PANEL_STYLES | |
| from .prompts import CommandPrompt, MenuAction, PromptResult, confirm, select | |
| from .progress import spinner, progress_bar, steps, indeterminate_progress, download_progress | |
| from .errors import show_error, show_conflict, show_warning, auto_suggest_fix, SuggestedFix | |
| from .explain import explain_command, explain_pipeline, CommandPart | |
| from .trust import TrustManager, TrustScope, trust_manager | |
| from .panels import ai_thinking, ai_response, status_panel, code_panel, diff_panel, summary_panel, welcome_banner, help_footer | |
| from .console import console, CortexConsole | |
| from .errors import show_error, show_conflict, show_warning, auto_suggest_fix, SuggestedFix | |
| from .explain import explain_command, explain_pipeline, CommandPart | |
| from .panels import ai_thinking, ai_response, status_panel, code_panel, diff_panel, summary_panel, welcome_banner, help_footer | |
| from .progress import spinner, progress_bar, steps, indeterminate_progress, download_progress | |
| from .prompts import CommandPrompt, MenuAction, PromptResult, confirm, select | |
| from .theme import COLORS, SYMBOLS, CORTEX_THEME, PANEL_STYLES | |
| from .trust import TrustManager, TrustScope, trust_manager |
🧰 Tools
🪛 GitHub Actions: CI
[error] 3-3: Import block is un-sorted or un-formatted
🪛 GitHub Check: lint
[failure] 3-10: Ruff (I001)
cortex/ui/init.py:3:1: I001 Import block is un-sorted or un-formatted
🪛 GitHub Check: Lint
[failure] 3-10: Ruff (I001)
cortex/ui/init.py:3:1: I001 Import block is un-sorted or un-formatted
🤖 Prompt for AI Agents
In @cortex/ui/__init__.py around lines 3 - 10, The imports in
cortex/ui/__init__.py are not alphabetized causing Ruff I001; reorder the
top-level import statements alphabetically by module name (for example:
.console, .errors, .explain, .panels, .progress, .prompts, .theme, .trust) while
preserving the existing imported symbols (CortexConsole/console,
show_error/etc., explain_command/etc., ai_thinking/etc., spinner/etc.,
CommandPrompt/etc., COLORS/etc., TrustManager/etc.) so only the import lines are
re-sorted and no other code changes are made.
| from typing import Optional | ||
| from rich.console import Console as RichConsole | ||
| from rich.panel import Panel | ||
| from rich.syntax import Syntax | ||
| from rich.markdown import Markdown | ||
| from .theme import CORTEX_THEME, SYMBOLS, PANEL_STYLES |
There was a problem hiding this comment.
Fix import sorting and use modern type annotation syntax.
Static analysis flagged unsorted imports (I001) and outdated Optional usage (UP045). Per PEP 604 (Python 3.10+), prefer X | None over Optional[X].
Proposed fix
-from typing import Optional
-from rich.console import Console as RichConsole
-from rich.panel import Panel
-from rich.syntax import Syntax
-from rich.markdown import Markdown
-from .theme import CORTEX_THEME, SYMBOLS, PANEL_STYLES
+from rich.console import Console as RichConsole
+from rich.markdown import Markdown
+from rich.panel import Panel
+from rich.syntax import Syntax
+
+from .theme import CORTEX_THEME, PANEL_STYLES, SYMBOLS🧰 Tools
🪛 GitHub Check: lint
[failure] 3-8: Ruff (I001)
cortex/ui/console.py:3:1: I001 Import block is un-sorted or un-formatted
🪛 GitHub Check: Lint
[failure] 3-8: Ruff (I001)
cortex/ui/console.py:3:1: I001 Import block is un-sorted or un-formatted
🤖 Prompt for AI Agents
In @cortex/ui/console.py around lines 3 - 8, Imports at the top of the module
are unsorted and you’re still importing typing.Optional; reorder imports (stdlib
first, third-party rich packages next, then local package imports) and update
any type annotations that use Optional[...] to use the PEP 604 union syntax
(e.g., Foo | None). Remove the now-unused Optional import from typing and ensure
all function signatures, variable annotations, and return types that used
Optional are updated to the X | None form (search for "Optional[" and replace
accordingly), keeping the existing symbols like CORTEX_THEME, SYMBOLS, and
PANEL_STYLES untouched.
| class CortexConsole: | ||
| """Themed console with semantic message methods.""" | ||
|
|
||
| _instance: Optional['CortexConsole'] = None | ||
|
|
||
| def __new__(cls) -> 'CortexConsole': | ||
| if cls._instance is None: | ||
| cls._instance = super().__new__(cls) | ||
| cls._instance._console = RichConsole(theme=CORTEX_THEME) | ||
| return cls._instance | ||
|
|
||
| @property | ||
| def rich(self) -> RichConsole: | ||
| return self._console | ||
|
|
||
| def print(self, *args, **kwargs) -> None: | ||
| self._console.print(*args, **kwargs) |
There was a problem hiding this comment.
Remove trailing whitespace and add docstrings to public methods.
Multiple blank lines contain whitespace (W293 on lines 13, 15, 21, 25, 28, 31, 36). Additionally, public methods lack docstrings per coding guidelines.
The singleton pattern implemented via __new__ is not thread-safe. If CortexConsole() is called from multiple threads simultaneously during first initialization, multiple instances could be created.
Fix whitespace and update type annotation
class CortexConsole:
"""Themed console with semantic message methods."""
-
+
_instance: Optional['CortexConsole'] = None
-
+
def __new__(cls) -> 'CortexConsole':
if cls._instance is None:
cls._instance = super().__new__(cls)
cls._instance._console = RichConsole(theme=CORTEX_THEME)
return cls._instance
-
+
@property
def rich(self) -> RichConsole:
+ """Return the underlying Rich Console instance."""
return self._console🧰 Tools
🪛 GitHub Check: lint
[failure] 25-25: Ruff (W293)
cortex/ui/console.py:25:1: W293 Blank line contains whitespace
[failure] 21-21: Ruff (W293)
cortex/ui/console.py:21:1: W293 Blank line contains whitespace
[failure] 15-15: Ruff (W293)
cortex/ui/console.py:15:1: W293 Blank line contains whitespace
[failure] 13-13: Ruff (W293)
cortex/ui/console.py:13:1: W293 Blank line contains whitespace
🪛 GitHub Check: Lint
[failure] 25-25: Ruff (W293)
cortex/ui/console.py:25:1: W293 Blank line contains whitespace
[failure] 21-21: Ruff (W293)
cortex/ui/console.py:21:1: W293 Blank line contains whitespace
[failure] 15-15: Ruff (W293)
cortex/ui/console.py:15:1: W293 Blank line contains whitespace
[failure] 13-13: Ruff (W293)
cortex/ui/console.py:13:1: W293 Blank line contains whitespace
🤖 Prompt for AI Agents
In @cortex/ui/console.py around lines 11 - 27, Remove trailing whitespace and
extra blank-line whitespace throughout the CortexConsole class, add concise
docstrings to the public members __new__, rich (property) and print to meet
docstring guidelines, and make the singleton initialization in __new__
thread-safe (e.g., guard creation of _instance/_console with a threading.Lock or
use double-checked locking) so simultaneous CortexConsole() calls cannot create
multiple instances; reference the class name CortexConsole, the __new__ method,
the _instance and _console attributes, the rich property, and the print method
when applying these changes.
| def error(self, message: str, details: Optional[str] = None) -> None: | ||
| self._console.print(f"[error_symbol]{SYMBOLS['error']}[/] [error]{message}[/]") | ||
| if details: | ||
| self._console.print(f" [secondary]{details}[/]") |
There was a problem hiding this comment.
Update type annotation syntax.
Use str | None instead of Optional[str] per UP045.
Proposed fix
- def error(self, message: str, details: Optional[str] = None) -> None:
+ def error(self, message: str, details: str | None = None) -> None:
self._console.print(f"[error_symbol]{SYMBOLS['error']}[/] [error]{message}[/]")
if details:
self._console.print(f" [secondary]{details}[/]")🧰 Tools
🪛 GitHub Check: lint
[failure] 32-32: Ruff (UP045)
cortex/ui/console.py:32:44: UP045 Use X | None for type annotations
🪛 GitHub Check: Lint
[failure] 32-32: Ruff (UP045)
cortex/ui/console.py:32:44: UP045 Use X | None for type annotations
🤖 Prompt for AI Agents
In @cortex/ui/console.py around lines 32 - 35, The method signature for error
uses legacy typing Optional[str]; update it to use the new union syntax (str |
None) in the error method definition (error(self, message: str, details: str |
None) -> None) and remove the now-unused Optional import from the module if
present; ensure no other references in cortex/ui/console.py still use Optional
so linting/type-checking passes.
| COMMON_ERRORS = { | ||
| "container_conflict": {"pattern": "container name .* already in use", "title": "Container Name Conflict", "fix_template": "docker rm -f {container_name}", "fix_description": "Remove the existing container"}, | ||
| "port_in_use": {"pattern": "port .* already in use", "title": "Port Already In Use", "fix_template": "lsof -ti:{port} | xargs kill -9", "fix_description": "Kill the process using the port"}, | ||
| "permission_denied": {"pattern": "permission denied", "title": "Permission Denied", "fix_template": "sudo {original_command}", "fix_description": "Run with elevated privileges"}, | ||
| "not_found": {"pattern": "command not found|not found", "title": "Command Not Found", "fix_template": "apt install {package}", "fix_description": "Install the missing package"}, | ||
| } |
There was a problem hiding this comment.
not_found error type has unresolved placeholder.
The "not_found" entry uses {package} in the fix template, but auto_suggest_fix (lines 81-97) has no extraction logic for this error type. This will leave {package} unsubstituted in the suggested fix.
🔧 Option 1: Add extraction logic
elif error_type == "permission_denied":
fix_cmd = fix_cmd.format(original_command=command)
+ elif error_type == "not_found":
+ # Try to extract package name from error
+ match = re.search(r"(\w+): command not found", error_lower)
+ if match:
+ fix_cmd = fix_cmd.format(package=match.group(1))
+ else:
+ fix_cmd = fix_cmd.format(package="<package>")
return SuggestedFix(command=fix_cmd, description=info["fix_description"])🔧 Option 2: Use generic placeholder in template
- "not_found": {"pattern": "command not found|not found", "title": "Command Not Found", "fix_template": "apt install {package}", "fix_description": "Install the missing package"},
+ "not_found": {"pattern": "command not found|not found", "title": "Command Not Found", "fix_template": "apt install <package-name>", "fix_description": "Install the missing package"},🤖 Prompt for AI Agents
In @cortex/ui/errors.py around lines 17 - 22, The "not_found" entry in
COMMON_ERRORS uses a {package} placeholder but auto_suggest_fix does not
populate "package", leaving the suggested fix unsubstituted; either change
COMMON_ERRORS["not_found"]["fix_template"] to a generic/safe placeholder such as
{original_command} or implement extraction logic inside auto_suggest_fix to
detect "command not found" errors (parse the missing command from the error
message) and set a "package" value before formatting the template—update the
branch in auto_suggest_fix that handles "not_found"/"not found" so it supplies
the "package" key (or switch the template) and ensure the formatted fix uses
only keys that are guaranteed to be present.
| def indeterminate_progress(message: str) -> Progress: | ||
| return Progress(SpinnerColumn(style="cortex"), TextColumn("[info]{task.description}[/]"), console=console.rich, transient=True) | ||
|
|
||
|
|
||
| def download_progress(description: str, total: float) -> Progress: | ||
| return Progress(SpinnerColumn(style="cortex"), TextColumn("[info]{task.description}[/]"), BarColumn(bar_width=30, style="secondary", complete_style="cortex", finished_style="success"), TaskProgressColumn(), "•", TimeRemainingColumn(), console=console.rich, transient=True) |
There was a problem hiding this comment.
Unused parameters in indeterminate_progress and download_progress.
Both functions create a Progress instance but don't add a task or use their parameters (message, description, total). The caller would need to manually call add_task() on the returned Progress, which differs from the context manager patterns above and may cause confusion.
Consider either:
- Adding the task within these functions (consistent with
spinner), or - Documenting that callers must add their own tasks.
🔧 Option 1: Add task within function (recommended for consistency)
def indeterminate_progress(message: str) -> Progress:
- return Progress(SpinnerColumn(style="cortex"), TextColumn("[info]{task.description}[/]"), console=console.rich, transient=True)
+ """Create and start an indeterminate progress indicator.
+
+ Args:
+ message: The message to display.
+
+ Returns:
+ A Progress instance with the task already added.
+ """
+ progress = Progress(SpinnerColumn(style="cortex"), TextColumn("[info]{task.description}[/]"), console=console.rich, transient=True)
+ progress.add_task(message, total=None)
+ return progress
def download_progress(description: str, total: float) -> Progress:
- return Progress(SpinnerColumn(style="cortex"), TextColumn("[info]{task.description}[/]"), BarColumn(bar_width=30, style="secondary", complete_style="cortex", finished_style="success"), TaskProgressColumn(), "•", TimeRemainingColumn(), console=console.rich, transient=True)
+ """Create a download-style progress bar with ETA.
+
+ Args:
+ description: The description to display.
+ total: The total size/count for progress calculation.
+
+ Returns:
+ A Progress instance with the task already added.
+ """
+ progress = Progress(SpinnerColumn(style="cortex"), TextColumn("[info]{task.description}[/]"), BarColumn(bar_width=30, style="secondary", complete_style="cortex", finished_style="success"), TaskProgressColumn(), "•", TimeRemainingColumn(), console=console.rich, transient=True)
+ progress.add_task(description, total=total)
+ return progress🤖 Prompt for AI Agents
In @cortex/ui/progress.py around lines 61 - 66, indeterminate_progress and
download_progress create a Progress but never add a task or use the incoming
parameters; modify each function to create the Progress, call
progress.add_task(...) using the incoming message/description and total (for
download_progress) so callers get a ready-to-use Progress with an active task
(e.g., in indeterminate_progress call add_task(message, total=None) and in
download_progress call add_task(description, total=total)), then return the
Progress; ensure you reference the existing function names
indeterminate_progress and download_progress and preserve existing columns and
transient/console settings.
| def confirm(message: str, details: Optional[List[str]] = None, default: bool = True, allow_dont_ask: bool = True) -> tuple: | ||
| content_lines = [f"[primary]{message}[/]"] | ||
| if details: | ||
| content_lines.append("") | ||
| for detail in details: | ||
| content_lines.append(f" [muted]•[/] {detail}") | ||
| content_lines.append("") | ||
| options = ["[highlight][Y][/] [primary]Yes[/]" if default else "[muted][y][/] [muted]Yes[/]"] | ||
| options.append("[muted][n][/] [muted]No[/]" if default else "[highlight][N][/] [primary]No[/]") | ||
| if allow_dont_ask: | ||
| options.append("[muted][a][/] [muted]Yes, don't ask again[/]") | ||
| content_lines.append(" " + " ".join(options)) | ||
| console.print(Panel("\n".join(content_lines), title="[highlight]─ ACTION REQUIRED [/]", border_style="highlight", padding=(1, 2))) | ||
|
|
||
| while True: | ||
| response = input(" > ").strip().lower() | ||
| if response in ('', 'y', 'yes') and default: | ||
| return (True, False) | ||
| elif response in ('y', 'yes'): | ||
| return (True, False) | ||
| elif response in ('n', 'no'): | ||
| return (False, False) | ||
| elif response == 'a' and allow_dont_ask: | ||
| return (True, True) | ||
| elif response == '' and not default: | ||
| return (False, False) | ||
| console.warning("Please enter Y, N" + (", or A" if allow_dont_ask else "")) |
There was a problem hiding this comment.
Missing exception handling and redundant conditions in confirm().
-
confirm()doesn't handleKeyboardInterrupt/EOFError, unlikeCommandPrompt.show()andselect(). This inconsistency could cause unexpected crashes. -
Lines 121-124 have redundant logic:
if response in ('', 'y', 'yes') and defaultfollowed byelif response in ('y', 'yes')means 'y'/'yes' is checked twice.
Proposed fix
def confirm(message: str, details: Optional[List[str]] = None, default: bool = True, allow_dont_ask: bool = True) -> tuple:
+ """Display a confirmation prompt and return (confirmed, dont_ask_again)."""
content_lines = [f"[primary]{message}[/]"]
# ... panel rendering ...
console.print(Panel("\n".join(content_lines), title="[highlight]─ ACTION REQUIRED [/]", border_style="highlight", padding=(1, 2)))
-
+
while True:
- response = input(" > ").strip().lower()
- if response in ('', 'y', 'yes') and default:
- return (True, False)
- elif response in ('y', 'yes'):
+ try:
+ response = input(" > ").strip().lower()
+ except (KeyboardInterrupt, EOFError):
+ return (False, False)
+
+ if response in ('y', 'yes'):
return (True, False)
elif response in ('n', 'no'):
return (False, False)
elif response == 'a' and allow_dont_ask:
return (True, True)
- elif response == '' and not default:
- return (False, False)
+ elif response == '':
+ return (default, False)
console.warning("Please enter Y, N" + (", or A" if allow_dont_ask else ""))| def _save_global_trust(self) -> None: | ||
| self.CONFIG_FILE.parent.mkdir(parents=True, exist_ok=True) | ||
| with open(self.CONFIG_FILE, 'w') as f: | ||
| json.dump({"commands": list(self._global_commands), "command_types": list(self._global_command_types)}, f, indent=2) |
There was a problem hiding this comment.
File write lacks error handling.
_save_global_trust can raise IOError or PermissionError if the config directory is not writable. This would crash the application unexpectedly.
Proposed fix to handle write errors
def _save_global_trust(self) -> None:
- self.CONFIG_FILE.parent.mkdir(parents=True, exist_ok=True)
- with open(self.CONFIG_FILE, 'w') as f:
- json.dump({"commands": list(self._global_commands), "command_types": list(self._global_command_types)}, f, indent=2)
+ try:
+ self.CONFIG_FILE.parent.mkdir(parents=True, exist_ok=True)
+ with open(self.CONFIG_FILE, 'w') as f:
+ json.dump({
+ "commands": list(self._global_commands),
+ "command_types": list(self._global_command_types)
+ }, f, indent=2)
+ except (IOError, OSError):
+ pass # Best-effort persistence; consider logging warning🤖 Prompt for AI Agents
In @cortex/ui/trust.py around lines 38 - 41, _save_global_trust currently writes
CONFIG_FILE without handling write failures; wrap the write in a try/except that
catches OSError (including IOError/PermissionError), perform an atomic save
(write to a temp file in the same directory then os.replace to CONFIG_FILE) and
on failure log the error via the module/logger and avoid raising (or surface a
clear error) so the application doesn't crash; ensure you still create the
parent dir (CONFIG_FILE.parent.mkdir) before the temp write and continue to
serialize self._global_commands and self._global_command_types as before.
| if directory: | ||
| directory = os.path.abspath(directory) | ||
| for trusted_dir in self._session_directories: | ||
| if directory.startswith(trusted_dir): | ||
| return True |
There was a problem hiding this comment.
Path traversal vulnerability in directory trust check.
Using startswith on string paths is insecure. A trusted directory /home/user/proj would incorrectly match /home/user/project-evil because the string starts with the same prefix.
Recommended fix using Path containment check
if directory:
directory = os.path.abspath(directory)
for trusted_dir in self._session_directories:
- if directory.startswith(trusted_dir):
+ try:
+ Path(directory).relative_to(trusted_dir)
return True
+ except ValueError:
+ continue
return FalseAlternatively, ensure the trusted directory ends with a separator:
if directory:
directory = os.path.abspath(directory)
for trusted_dir in self._session_directories:
- if directory.startswith(trusted_dir):
+ if directory == trusted_dir or directory.startswith(trusted_dir + os.sep):
return True
return False🤖 Prompt for AI Agents
In @cortex/ui/trust.py around lines 59 - 63, Replace the insecure string prefix
check with a proper filesystem containment check: convert both the candidate
directory and each trusted entry in self._session_directories to pathlib.Path
objects, call resolve() (or absolute()) on both to normalize symlinks, and then
test containment using Path.is_relative_to(trusted_path) (or fallback to
comparing trusted_path == candidate_path or catching ValueError from
candidate_path.relative_to(trusted_path)). Update the block that currently uses
directory.startswith(trusted_dir) to perform this Path-based check so only true
subpaths of a trusted directory pass.
| from cortex.ui.theme import COLORS, SYMBOLS, CORTEX_THEME, PANEL_STYLES | ||
| from cortex.ui.console import console, CortexConsole | ||
| from cortex.ui.prompts import CommandPrompt, MenuAction, PromptResult, COMMAND_MENU_OPTIONS | ||
| from cortex.ui.progress import StepsProgress |
There was a problem hiding this comment.
Unused import: StepsProgress.
StepsProgress is imported but not used in any test. Either add tests for it or remove the import.
🔧 Suggested fix
-from cortex.ui.progress import StepsProgressOr add a test:
class TestProgress:
def test_steps_progress_increments(self):
tracker = StepsProgress("Test", 3)
assert tracker.current == 0
tracker.step("Step 1")
assert tracker.current == 1📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| from cortex.ui.progress import StepsProgress |
🤖 Prompt for AI Agents
In @tests/test_ui.py at line 6, The import StepsProgress in tests/test_ui.py is
unused; either remove the import line referencing StepsProgress or add a test
that exercises StepsProgress (e.g., instantiate StepsProgress, verify initial
state, call step() and assert the current counter increments). Update the file
to eliminate the unused import or commit the new test referencing the
StepsProgress symbol so the import is used.
|
Superseded by #595 Rich output formatting. Thank you! |
Summary
Complete terminal UI overhaul implementing modern CLI patterns from Claude Code, Cursor, and GitHub Copilot.
Features
Console Messages
6-Option Command Menu
Error Display with Suggested Fixes
Trust Management
Progress Indicators
Technical Details
Closes
Bounty Impact
Implements $400 worth of UX issues (+$400 post-funding bonus)
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.