feat(cli): Add daemon management commands to Cortex CLI #656
Conversation
- Introduced a new `daemon` command with subcommands for managing the cortexd daemon, including `install`, `uninstall`, `config`, `reload-config`, `version`, `ping`, `shutdown`, and `run-tests`. - Implemented detailed help text for each subcommand to enhance user guidance. - Added centralized error handling for IPC calls related to daemon operations, improving robustness and maintainability. - Enhanced installation and uninstallation processes with audit logging for better traceability of actions. This update significantly improves the CLI's functionality for managing the cortexd daemon.
sujay-d07
requested review from
Anshgrover23,
Suyashd999 and
mikejmorgan-ai
as code owners
CLA Verification PassedAll contributors have signed the CLA.
|
|
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. 📝 WalkthroughWalkthroughAdds a comprehensive Changes
Sequence DiagramsequenceDiagram
participant User
participant CLI as CortexCLI
participant IPC as DaemonClient
participant History as InstallationHistory
participant OS as OS/Subprocess
User->>CLI: cortex daemon <cmd> [flags]
CLI->>CLI: parse & dispatch to daemon()
alt IPC command (ping/version/config/reload/shutdown)
CLI->>IPC: instantiate client & call ipc_func
IPC-->>CLI: response or error
CLI->>History: attempt audit/history update (safe fallback)
CLI-->>User: print result or error
else Install / Uninstall / Run-tests
CLI->>OS: locate script or tests on filesystem
OS-->>CLI: path found / not found
alt found & --execute
CLI->>OS: run script or ctest via subprocess
OS-->>CLI: exit code & output
CLI->>History: record start and completion (success/failure)
CLI-->>User: display outcome
else dry-run or missing
CLI-->>User: display instructions or error
end
end
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 @sujay-d07, 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 significantly enhances the Cortex CLI by integrating a comprehensive set of commands for managing the 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 daemon management commands (install, uninstall, config, reload-config, version, ping, shutdown, run-tests) to the Cortex CLI. The changes are well-structured, with a central dispatcher for daemon commands and a helper function for IPC calls. The implementation includes audit logging for important operations.
My review focuses on improving maintainability by reducing code duplication in error handling, and on enhancing security by flagging the use of sudo bash in the uninstall script and suggesting specific patterns for preventing dangerous shell command execution. Overall, this is a solid addition of functionality.
There was a problem hiding this comment.
Pull request overview
This pull request adds daemon management commands to the Cortex CLI, enabling users to install, uninstall, configure, and interact with the cortexd background daemon service through CLI commands.
Changes:
- Added a new
daemoncommand with 8 subcommands: install, uninstall, config, reload-config, version, ping, shutdown, and run-tests - Implemented IPC communication helper method for daemon interaction
- Integrated audit logging throughout daemon operations for tracking installation history
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@cortex/cli.py`:
- Around line 2115-2168: The code currently records an installation and sets it
to FAILED when the user runs a dry-run (execute == False); change the flow so
you only create/record and update audit history when execution will actually
run: move or defer the call to history.record_installation (and assignment to
install_id) until after execute is verified true, or if your InstallationStatus
enum has a SKIPPED/NO_OP state use that instead and call
history.update_installation(install_id, InstallationStatus.SKIPPED, "...") for
the dry-run path; update references to InstallationHistory,
history.record_installation, install_id, history.update_installation and the
execute variable accordingly so dry-runs are not marked as FAILED.
- Around line 2259-2350: The uninstall flow is creating orphan
InstallationHistory entries by calling history.record_installation(...) inside
the uninstall_script block and the manual commands loop without ever updating
them; remove these record_installation(...) calls (both the one before running
the uninstall script and the per-command calls inside the manual uninstall loop)
so you don't create "in_progress" records with no update, and instead, if you
need per-command audit, use the existing install_id with
history.update_installation(install_id, InstallationStatus.IN_PROGRESS, "<append
or include command info>") or add a dedicated method to append command logs to
the existing installation record (refer to the symbols record_installation,
update_installation, install_id, the uninstall_script block and the commands
loop).
- Around line 2015-2026: Update the _daemon_ipc_call signature to include
explicit type annotations: annotate the ipc_func parameter as a callable that
accepts a DaemonClient and returns a DaemonResponse, and annotate the method
return type as tuple[bool, DaemonResponse | None]; also add or update the
necessary typing imports (e.g., Callable or other typing helpers) so
DaemonClient and DaemonResponse are referenced correctly in the annotations.
…ror handling improvements - Added a new helper method `_update_history_on_failure` to streamline error handling for installation history updates during daemon operations. - Improved the `daemon` command help text to include the new `run-tests` subcommand. - Refactored `_daemon_ipc_call` to accept a callable type for better type safety and clarity. - Enhanced error handling in various daemon commands to ensure that failures are logged without disrupting the main flow. - Introduced a new test suite for daemon command functionality, covering all major commands and their expected behaviors. These updates improve the robustness and usability of the Cortex CLI for managing the daemon.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@cortex/cli.py`:
- Around line 2018-2034: The type annotation for install_id in
_update_history_on_failure is incorrect: InstallationHistory.record_installation
returns a str, so change the parameter type from int | None to str | None (or
use Optional[str]) and ensure callers pass the string install ID; update
references to install_id in _update_history_on_failure and any callers that
assumed an int so they handle the string ID when invoking
history.update_installation(install_id, InstallationStatus.FAILED, error_msg).
In `@tests/test_daemon_commands.py`:
- Around line 318-350: The test patches Path.glob but _daemon_run_tests checks
test existence via (tests_dir / test).exists(), so the mock_glob is unused;
update the test to remove the Path.glob patch and instead patch/configure
Path.exists (or the specific Path instance returned for tests_dir / test) so the
mocked existence checks reflect the expected test files; reference the test
function name test_daemon_run_tests_success and the implementation method
_daemon_run_tests to locate the change and replace mock_glob with proper use of
mock_exists (or adjust the mock_exists return values per test filename).
♻️ Duplicate comments (2)
cortex/cli.py (2)
2252-2273: Same dry-run audit logging issue as in_daemon_install.The uninstall dry-run path also records the operation as
FAILEDwhen--executeisn't provided. Apply the same fix as suggested for_daemon_install- defer audit recording until execution is confirmed.
2160-2181: Dry-run operations should not be recorded as FAILED.When
--executeisn't provided, the operation is intentionally a dry-run preview, but it's being recorded asFAILEDin the audit history with the message "Operation cancelled (no --execute flag)". This makes audit reports misleading, as dry-runs aren't failures.Per coding guidelines, dry-run is the default behavior for installation operations. Consider either:
- Not recording audit history until
--executeis confirmed- Using a distinct status like
SKIPPEDif available🔧 Suggested approach: defer audit recording until execute
def _daemon_install(self, args: argparse.Namespace) -> int: """Install the cortexd daemon using setup_daemon.py.""" import subprocess from pathlib import Path cx_header("Installing Cortex Daemon") - # Initialize audit logging - history = InstallationHistory() - start_time = datetime.now(timezone.utc) - install_id = None - - try: - # Record operation start - install_id = history.record_installation( - InstallationType.CONFIG, - ["cortexd"], - ["cortex daemon install"], - start_time, - ) - except Exception as e: - cx_print(f"Warning: Could not initialize audit logging: {e}", "warning") - # Find setup_daemon.py daemon_dir = Path(__file__).parent.parent / "daemon" setup_script = daemon_dir / "scripts" / "setup_daemon.py" if not setup_script.exists(): error_msg = f"Setup script not found at {setup_script}" cx_print(error_msg, "error") cx_print("Please ensure the daemon directory is present.", "error") - if install_id: - try: - history.update_installation(install_id, InstallationStatus.FAILED, error_msg) - except Exception: - pass return 1 execute = getattr(args, "execute", False) if not execute: cx_print("This will build and install the cortexd daemon.", "info") # ... dry-run info ... - if install_id: - try: - history.update_installation( - install_id, - InstallationStatus.FAILED, - "Operation cancelled (no --execute flag)", - ) - except Exception: - pass return 0 + # Initialize audit logging only when executing + history = InstallationHistory() + start_time = datetime.now(timezone.utc) + install_id = None + try: + install_id = history.record_installation( + InstallationType.CONFIG, + ["cortexd"], + ["cortex daemon install"], + start_time, + ) + except Exception as e: + cx_print(f"Warning: Could not initialize audit logging: {e}", "warning") + # Run setup_daemon.py cx_print("Running daemon setup wizard...", "info")
… logging - Changed the type of `install_id` in `_update_history_on_failure` method from `int` to `str` for better consistency. - Refactored the audit logging initialization to occur only when the execution of the daemon commands is confirmed, preventing unnecessary logging for dry runs. - Enhanced error handling during installation and uninstallation processes to ensure that failures are logged appropriately without disrupting the main flow. - Added helper methods in tests to streamline the creation of mock objects for uninstall scripts and subprocess calls, improving test clarity and maintainability. These changes enhance the robustness of the Cortex CLI's daemon management commands.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@tests/test_daemon_commands.py`:
- Around line 327-338: The test currently patches CortexCLI._daemon_ipc_call and
then calls _daemon_ping, causing a circular/mock-driven test; either rename the
test to reflect that it verifies _daemon_ping integration (e.g.,
test_daemon_ping_uses_ipc_call) or change the test to directly exercise
_daemon_ipc_call (remove the patch and instead mock the underlying
DaemonClient/IPC layer) so you actually validate _daemon_ipc_call behavior;
update references to _daemon_ipc_call, _daemon_ping, and
DaemonClient/DaemonResponse in the test accordingly to match the chosen option.
- Around line 301-325: The test patching of cortex.cli.Path.exists is
ineffective because _daemon_run_tests() performs a local import from pathlib
(local Path) which shadows the module-level Path; either change the test to
patch pathlib.Path.exists so the exists() call inside _daemon_run_tests is
mocked, or refactor the implementation by moving the local "from pathlib import
Path" to the module top-level (and remove the local import) so cortex.cli.Path
is used and the existing patch will work; locate the local import and the
_daemon_run_tests function name to apply the chosen fix.
🧹 Nitpick comments (2)
cortex/cli.py (1)
2529-2537: Consider extracting test lists to a constant or configuration.The hardcoded
unit_testsandintegration_testslists may become out of sync with actual test binaries as the daemon evolves. Consider extracting these to module-level constants or reading from a configuration file for maintainability.tests/test_daemon_commands.py (1)
1-7: Consider adding tests for remaining paths.The test suite provides good coverage of the main daemon command flows. For more comprehensive coverage, consider adding tests for:
_daemon_installwith--executeand successful script execution_daemon_run_testswith--unit,--integration, and--testfilter flags- Subprocess failure scenarios in install/uninstall
These can be added in a follow-up PR if needed.
- Updated the test for `_daemon_ipc_call` to use mocks for `DaemonClient` and `InstallationHistory`, enhancing test clarity and maintainability. - Removed unused import of `Path` in `cortex/cli.py` to clean up the codebase. - Improved assertions in the test to verify the correct behavior of the IPC call and installation history updates. These changes enhance the robustness of the test suite for daemon commands.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@tests/test_daemon_commands.py`:
- Around line 20-27: Add missing type hints on the new test helper methods:
annotate TestDaemonCommands.setUp(self) -> None and
TestDaemonCommands.tearDown(self) -> None and update any other new test methods
in this module to include return type -> None; also annotate parameters for
mocks where practical (e.g., use typing.Mock or unittest.mock.Mock for injected
mock args). Ensure all new test functions and helpers in
tests/test_daemon_commands.py follow the project guideline of explicit type
hints.
♻️ Duplicate comments (1)
tests/test_daemon_commands.py (1)
151-194: Uninstall tests patch the wrongPathtarget (may hit real FS).
_daemon_uninstall()importsPathlocally, so patchingcortex.cli.Pathdoesn’t affect it. The current Path-class mock also doesn’t model path joins, so the test can fall back to the real filesystem or take the wrong branch. Prefer patchingpathlib.Path.exists(andstat/chmod) or remove the local import in_daemon_uninstall.✅ Suggested fix (patch pathlib.Path)
- `@patch`("cortex.cli.Path") + `@patch`("pathlib.Path.chmod") + `@patch`("pathlib.Path.stat") + `@patch`("pathlib.Path.exists") `@patch`("subprocess.run") - def test_daemon_uninstall_with_script( - self, mock_subprocess, mock_path_class, mock_print, mock_header - ): + def test_daemon_uninstall_with_script( + self, mock_subprocess, mock_exists, mock_stat, mock_chmod, mock_print, mock_header + ): """Test daemon uninstall with uninstall script.""" args = Mock() args.execute = True args.daemon_action = "uninstall" - # Create a mock Path object - mock_uninstall_script = self._create_mock_uninstall_script(exists=True) - self._setup_path_side_effect(mock_path_class, mock_uninstall_script) + mock_exists.return_value = True + mock_stat.return_value = Mock() self._setup_subprocess_side_effect(mock_subprocess, handle_script=True)- `@patch`("cortex.cli.Path") + `@patch`("pathlib.Path.exists") `@patch`("subprocess.run") - def test_daemon_uninstall_manual( - self, mock_subprocess, mock_path_class, mock_print, mock_header - ): + def test_daemon_uninstall_manual( + self, mock_subprocess, mock_exists, mock_print, mock_header + ): """Test daemon uninstall with manual commands (no script).""" args = Mock() args.execute = True args.daemon_action = "uninstall" - # Create a mock Path object for uninstall script that doesn't exist - mock_uninstall_script = self._create_mock_uninstall_script(exists=False) - self._setup_path_side_effect(mock_path_class, mock_uninstall_script) + mock_exists.return_value = False self._setup_subprocess_side_effect(mock_subprocess, handle_script=False)
…on command tests - Updated the `setUp` and `tearDown` methods in the `TestDaemonCommands` class to include type hints for better code clarity and adherence to type checking standards. - This change enhances the maintainability and readability of the test suite for daemon commands.
|
@Suyashd999 @Anshgrover23 Please merge this ASAP, as daemon commands are currently broken. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@cortex/cli.py`:
- Around line 2697-2722: The subprocess.run(...) invocation that executes ctest
must be wrapped in a try/except similar to the pattern in _daemon_install() to
avoid leaving audit records in-progress; catch subprocess.SubprocessError and a
general Exception around the call to subprocess.run, and on any exception call
the helper _update_history_on_failure(install_id, error) (or equivalent) so
history.update_installation gets a FAILED status and error message, then return
a non-zero exit code; keep the existing success/failure return handling for
result.returncode intact and only call _update_history_on_failure when an
exception prevents subprocess.run from completing.
🧹 Nitpick comments (1)
cortex/cli.py (1)
2219-2249: Add a timeout for daemon setup execution.A hung setup script will block the CLI; consider a timeout and explicit handling.
♻️ Suggested timeout handling
- result = subprocess.run( - [sys.executable, str(setup_script)], - check=False, - ) + result = subprocess.run( + [sys.executable, str(setup_script)], + check=False, + timeout=1800, + ) ... - except subprocess.SubprocessError as e: + except subprocess.TimeoutExpired: + error_msg = "Daemon install timed out after 1800 seconds" + cx_print(error_msg, "error") + if install_id: + try: + history.update_installation(install_id, InstallationStatus.FAILED, error_msg) + except Exception: + pass + return 1 + except subprocess.SubprocessError as e: error_msg = f"Subprocess error during daemon install: {str(e)}"
- Wrapped the subprocess call in a try-except block to handle potential subprocess errors gracefully. - Improved error messages for both subprocess errors and unexpected exceptions during test execution. - Ensured that installation history updates are logged appropriately in case of failures, maintaining the main flow of execution.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@cortex/cli.py`:
- Around line 2102-2141: The timezone mismatch occurs when calculating durations
for InstallationHistory.update_installation: start_time is timezone-aware
(created with datetime.now(timezone.utc)) but later code uses naive
datetime.now(), causing TypeError; update the code path that computes duration
(the call site that uses datetime.now() to compute elapsed time before calling
history.update_installation) to use datetime.now(timezone.utc) so both datetimes
are tz-aware, and ensure any duration/subtraction logic around install_id and
history.update_installation uses the timezone-aware now consistently.
🧹 Nitpick comments (2)
cortex/cli.py (2)
2262-2339: Add explicit sudo confirmation (or--yes) before privileged uninstall.
--executecurrently runssudocommands immediately. To align with the “no silent sudo” policy, consider prompting unless a--yesflag is provided (mirrors other CLI flows). Based on learnings, please ensure explicit user approval for elevated permissions.
2384-2394:update_installation(IN_PROGRESS, …)doesn’t append command logs.
update_installation()overwriteserror_messageand recomputes snapshots, so the “append command info” comment is misleading and can cause noisy history updates. Consider a dedicated append API or drop per-command updates. Based on learnings, keep audit logging accurate and intentional.
- Enhanced the formatting of SQL execution statements for better clarity. - Updated the handling of timestamps to use UTC for consistency. - Improved the organization of code blocks for better readability and maintainability. - Ensured consistent use of list formatting in CSV header writing.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@cortex/installation_history.py`:
- Line 18: The duration calculation fails because a naive start_time from
record_installation is being subtracted from a timezone-aware
datetime.now(timezone.utc); update the subtraction site to normalize start_time
to UTC if it's naive before computing the duration (e.g., detect if
start_time.tzinfo is None and set tzinfo=timezone.utc via start_time.replace),
then compute datetime.now(timezone.utc) - start_time_aware; alternatively,
modify record_installation to store UTC-aware timestamps (callers use naive
datetime.now currently), but at minimum make the change in the code path that
performs the subtraction (the start_time/duration logic) to avoid TypeError.
- Added a check to ensure that the start_time is converted to UTC if it is naive, improving the accuracy of timestamp handling in installation history tracking.
|
Suyashd999
left a comment
Suyashd999
left a comment
There was a problem hiding this comment.
LGTM, the commands needs to be merged on a priority, Thanks @sujay-d07
3 participants
Summary
Integrated daemon management commands. Added support for installing, uninstalling, and managing the cortexd background daemon service via CLI commands including install, uninstall, config, reload-config, version, ping, shutdown, and run-tests.
Related Issue
Closes #654
Type of Change
AI Disclosure
Please indicate if you used AI tools during development:
Used AI assistant to ensure proper integration with existing CLI structure.
Testing
Checklist
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.