feat: Systemd service helper with plain English explanations #597
Conversation
mikejmorgan-ai
requested review from
Anshgrover23 and
Suyashd999
as code owners
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (3)
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. 📝 WalkthroughWalkthroughThis PR introduces a comprehensive systemd service helper module that enables users to manage systemd services through plain English commands. The implementation adds CLI integration, a core SystemdHelper class with methods for status explanation, failure diagnosis, dependency visualization, and unit file generation, along with extensive test coverage. Changes
Sequence DiagramssequenceDiagram
actor User
participant CLI as CLI<br/>(cortex/cli.py)
participant Helper as SystemdHelper<br/>(systemd_helper.py)
participant systemd as systemd/systemctl
participant journalctl as journalctl<br/>(logs)
User->>CLI: systemd [service] status
CLI->>Helper: systemd(service, action="status")
Helper->>systemd: systemctl show [service]
systemd-->>Helper: unit properties
Helper->>Helper: parse into ServiceStatus
Helper->>Helper: explain_status()
Helper-->>CLI: formatted status
CLI-->>User: display rich output
User->>CLI: systemd [service] diagnose
CLI->>Helper: systemd(service, action="diagnose")
Helper->>systemd: systemctl show [service]
systemd-->>Helper: unit properties
Helper->>Helper: identify failure patterns
Helper->>journalctl: journalctl -u [service]
journalctl-->>Helper: recent logs
Helper->>Helper: analyze logs + match FAILURE_SOLUTIONS
Helper->>Helper: generate recommendations
Helper-->>CLI: diagnosis with fixes
CLI-->>User: display diagnosis + commands
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 significantly enhances the 'cortex' CLI by integrating a powerful new 'systemd' subcommand. This feature aims to demystify systemd service management for users by providing plain English explanations of service statuses, diagnosing failures with practical solutions, and visualizing service dependencies. It streamlines common systemd operations into an intuitive CLI experience, making system administration more accessible. 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
|
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
| Issue: #448 | ||
| """ | ||
|
|
||
| import os |
| from dataclasses import dataclass, field | ||
| from enum import Enum | ||
| from pathlib import Path | ||
| from typing import Any, Dict, List, Optional, Tuple |
| from typing import Any, Dict, List, Optional, Tuple | ||
|
|
||
| from rich import box | ||
| from rich.console import Console |
![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/systemd_helper.py`:
- Around line 94-114: The ServiceConfig dataclass uses deprecated typing names
from typing; update annotations to modern built-in generics and union syntax:
replace Optional[str] with str | None for user, group, working_directory,
exec_stop, exec_reload; replace Dict[str, str] with dict[str, str] for
environment; replace List[str] with list[str] for wants, after, wanted_by; keep
ServiceType as-is if it’s a concrete enum/class; adjust any default_factory type
hints accordingly so all annotations use built-in generics and PEP 604 union
syntax.
- Around line 454-456: The loop that builds unit Environment lines (iterating
config.environment and calling lines.append(f"Environment={key}={value}")) can
produce invalid systemd unit entries when values contain spaces or special
chars; update the code that constructs these lines (the for key, value in
config.environment.items() block in cortex/systemd_helper.py) to quote and
escape environment values: if a value contains spaces or characters that require
quoting, wrap it in double quotes and escape any embedded double quotes and
backslashes so the resulting lines.append receives a valid systemd
Environment=KEY="escaped value" string.
- Around line 10-24: The imports in cortex/systemd_helper.py are unsorted and
the file uses legacy typing constructs; reorder and alphabetize imports
(standard library first, then third-party, then local) and remove unused typing
imports, then update all type annotations to use built-in generics and PEP 604
unions (e.g., replace List[str] with list[str], Dict[str, str] with dict[str,
str], Tuple[int, str, str] with tuple[int, str, str], Optional[str] with str |
None) throughout the file (look for annotations in functions and dataclasses
such as any occurrences of Optional, List, Dict, Tuple, and update imports to
drop those names from typing and keep only needed names like Any if still
required).
🧹 Nitpick comments (4)
cortex/systemd_helper.py (2)
169-180: Consider more specific exception handling.Catching a bare
Exceptionsuppresses all errors silently, including unexpected ones. Consider catching more specific exceptions or at least logging the error in verbose mode.♻️ Proposed fix
def _run_journalctl(self, service: str, lines: int = 50) -> str: """Get recent logs for a service.""" try: result = subprocess.run( ["journalctl", "-u", service, "-n", str(lines), "--no-pager"], capture_output=True, text=True, timeout=30 ) return result.stdout - except Exception: + except (FileNotFoundError, subprocess.TimeoutExpired, OSError): return ""
281-289: Clarify the conditional logic for displaying commands.The check
if "{service}" in cmd:on line 288 tests the original template string, which is correct. However, this could be clearer sincecmd_formattedwas already created. Consider adding a comment or refactoring for clarity.♻️ Suggestion for clarity
if status.active_state == "failed" or status.result not in ["success", ""]: parts.append(f"\n**Failure reason:** {status.result}") if status.result in FAILURE_SOLUTIONS: parts.append("**Suggested fixes:**") for desc, cmd in FAILURE_SOLUTIONS[status.result]: - cmd_formatted = cmd.format(service=service) parts.append(f" - {desc}") - if "{service}" in cmd: + # Only show command if it's a runnable command (contains {service} placeholder) + if "{service}" in cmd: + cmd_formatted = cmd.format(service=service) parts.append(f" Run: `{cmd_formatted}`")tests/test_systemd_helper.py (2)
267-315: Diagnosis tests cover the log analysis feature.Good tests for permission detection in logs. Consider adding tests for other log patterns (port conflicts, missing files) defined in
diagnose_failure.Consider adding test cases for other log analysis patterns:
"address already in use"→ port conflict detection"no such file"/"not found"→ missing file detection"connection refused"→ connection issue detection"out of memory"→ memory issue detection
516-524: ServiceType enum test is minimal but sufficient.The test verifies all enum values are correctly defined. The test file ends at line 524 without a final newline.
Add a newline at end of file:
def test_service_types(self): """Test all service types are defined.""" assert ServiceType.SIMPLE.value == "simple" assert ServiceType.FORKING.value == "forking" assert ServiceType.ONESHOT.value == "oneshot" assert ServiceType.NOTIFY.value == "notify" +
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
cortex/cli.pycortex/systemd_helper.pytests/test_systemd_helper.py
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Follow PEP 8 style guide for Python code
Include type hints in Python code
Add docstrings for all public APIs in Python code
Use dry-run mode by default for all installation operations
Do not use silent sudo - require explicit user confirmation for privilege escalation
Implement Firejail sandboxing for execution of untrusted code
Log all operations to ~/.cortex/history.db for audit purposes
Files:
cortex/systemd_helper.pytests/test_systemd_helper.pycortex/cli.py
tests/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Maintain test coverage above 80% for pull requests
Files:
tests/test_systemd_helper.py
🧬 Code graph analysis (2)
cortex/systemd_helper.py (1)
cortex/branding.py (2)
cx_print(49-69)cx_header(82-88)
cortex/cli.py (1)
cortex/systemd_helper.py (1)
run_systemd_helper(598-627)
🪛 GitHub Actions: CI
cortex/systemd_helper.py
[error] 10-10: Ruff: I001 Import block is un-sorted or un-formatted.
🪛 GitHub Check: lint
cortex/systemd_helper.py
[failure] 109-109: Ruff (UP006)
cortex/systemd_helper.py:109:12: UP006 Use list instead of List for type annotation
[failure] 108-108: Ruff (UP006)
cortex/systemd_helper.py:108:12: UP006 Use list instead of List for type annotation
[failure] 105-105: Ruff (UP006)
cortex/systemd_helper.py:105:18: UP006 Use dict instead of Dict for type annotation
[failure] 104-104: Ruff (UP045)
cortex/systemd_helper.py:104:24: UP045 Use X | None for type annotations
[failure] 103-103: Ruff (UP045)
cortex/systemd_helper.py:103:12: UP045 Use X | None for type annotations
[failure] 102-102: Ruff (UP045)
cortex/systemd_helper.py:102:11: UP045 Use X | None for type annotations
[failure] 16-16: Ruff (UP035)
cortex/systemd_helper.py:16:1: UP035 typing.Tuple is deprecated, use tuple instead
[failure] 16-16: Ruff (UP035)
cortex/systemd_helper.py:16:1: UP035 typing.List is deprecated, use list instead
[failure] 16-16: Ruff (UP035)
cortex/systemd_helper.py:16:1: UP035 typing.Dict is deprecated, use dict instead
[failure] 10-24: Ruff (I001)
cortex/systemd_helper.py:10:1: I001 Import block is un-sorted or un-formatted
🪛 GitHub Check: Lint
cortex/systemd_helper.py
[failure] 109-109: Ruff (UP006)
cortex/systemd_helper.py:109:12: UP006 Use list instead of List for type annotation
[failure] 108-108: Ruff (UP006)
cortex/systemd_helper.py:108:12: UP006 Use list instead of List for type annotation
[failure] 105-105: Ruff (UP006)
cortex/systemd_helper.py:105:18: UP006 Use dict instead of Dict for type annotation
[failure] 104-104: Ruff (UP045)
cortex/systemd_helper.py:104:24: UP045 Use X | None for type annotations
[failure] 103-103: Ruff (UP045)
cortex/systemd_helper.py:103:12: UP045 Use X | None for type annotations
[failure] 102-102: Ruff (UP045)
cortex/systemd_helper.py:102:11: UP045 Use X | None for type annotations
[failure] 16-16: Ruff (UP035)
cortex/systemd_helper.py:16:1: UP035 typing.Tuple is deprecated, use tuple instead
[failure] 16-16: Ruff (UP035)
cortex/systemd_helper.py:16:1: UP035 typing.List is deprecated, use list instead
[failure] 16-16: Ruff (UP035)
cortex/systemd_helper.py:16:1: UP035 typing.Dict is deprecated, use dict instead
[failure] 10-24: Ruff (I001)
cortex/systemd_helper.py:10: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). (3)
- GitHub Check: test (3.10)
- GitHub Check: test (3.12)
- GitHub Check: test (3.11)
🔇 Additional comments (15)
cortex/systemd_helper.py (5)
153-167: Subprocess call is safe from command injection.The use of list-based arguments to
subprocess.runcorrectly avoids shell injection risks. The timeout handling and error cases are well covered.
182-238: Status parsing logic is correct.The method properly handles the
.servicesuffix normalization and parses systemd's key=value output format correctly. Memory conversion to MB and CPU time to seconds are handled appropriately.
469-510: Unit file creation from description is well-implemented.The auto-name generation sanitizes the description properly, and forking detection from trailing
&is a nice touch. The method correctly delegates togenerate_unit_file.
512-570: Display methods integrate well with branding module.Good use of the project's
cx_print,cx_header, and Rich components for consistent UI. The color coding for service states (green/red/yellow/cyan) provides clear visual feedback.
598-627: Entry point function is clean and well-documented.The function properly dispatches to the appropriate helper methods based on action. Consider logging operations to
~/.cortex/history.dbas per coding guidelines.Based on coding guidelines, all operations should be logged to
~/.cortex/history.dbfor audit purposes. This module doesn't currently implement audit logging. Verify if this is required for read-only systemd status operations.cortex/cli.py (3)
1183-1187: CLI method correctly delegates to the helper module.The implementation follows the established pattern of lazy-importing the helper module and delegating to it. This is consistent with other CLI methods in the file.
2307-2317: Parser configuration is correct and user-friendly.The argument setup provides good defaults (
statusaction), clear choices, and a helpful description. The optional action withnargs="?"allows users to simply runcortex systemd nginxfor the common case.
2714-2719: Dispatch logic handles arguments safely.Using
getattrwith defaults ensures robustness if arguments are missing. This follows the defensive pattern used elsewhere in the CLI dispatcher.tests/test_systemd_helper.py (7)
1-20: Test file is well-organized with clear structure.Good use of pytest fixtures and class-based test organization. The imports correctly bring in all public API elements for testing.
23-55: ServiceConfig tests cover defaults and custom values.Tests verify the dataclass default values including service type, restart policy, and target dependencies. Custom value tests confirm all fields are properly stored.
117-157: SystemdHelper initialization and error handling tests are solid.Good coverage of edge cases: systemctl not found (FileNotFoundError), successful execution, and timeout handling. These ensure graceful degradation when systemd is unavailable.
173-202: Status parsing test verifies key field extraction.The test validates parsing of LoadState, ActiveState, SubState, MainPID, MemoryCurrent, and TasksCurrent. The
.servicesuffix addition is also verified.
341-386: Unit file generation tests are comprehensive.Tests verify both basic and full unit file generation including all Service section options (Type, User, Group, WorkingDirectory, Environment, Restart, After). The generated output format is validated.
427-435: Auto-name generation test validates sanitization.The test confirms that "My Super Cool Application" correctly becomes "my-super-cool-application.service", verifying the lowercase conversion and special character replacement.
471-513: Entry point tests use mocking effectively.Tests correctly mock the SystemdHelper class to verify that the entry point dispatches to the right methods (display_status, display_diagnosis, show_dependencies_tree) based on the action parameter.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| import os | ||
| import re | ||
| import subprocess | ||
| from dataclasses import dataclass, field | ||
| from enum import Enum | ||
| from pathlib import Path | ||
| from typing import Any, Dict, List, Optional, Tuple | ||
|
|
||
| from rich import box | ||
| from rich.console import Console | ||
| from rich.panel import Panel | ||
| from rich.table import Table | ||
| from rich.tree import Tree | ||
|
|
||
| from cortex.branding import console, cx_print, cx_header, CORTEX_CYAN |
There was a problem hiding this comment.
Fix import sorting and use modern type annotations.
Static analysis and pipeline are failing due to unsorted imports and deprecated typing constructs. Use built-in generics (list, dict, tuple) and union syntax (X | None) instead of importing from typing.
🔧 Proposed fix
import os
import re
import subprocess
from dataclasses import dataclass, field
from enum import Enum
from pathlib import Path
-from typing import Any, Dict, List, Optional, Tuple
from rich import box
from rich.console import Console
from rich.panel import Panel
from rich.table import Table
from rich.tree import Tree
from cortex.branding import console, cx_print, cx_header, CORTEX_CYANThen update type annotations throughout the file (e.g., Optional[str] → str | None, List[str] → list[str], Dict[str, str] → dict[str, str], Tuple[int, str, str] → tuple[int, str, str]).
🧰 Tools
🪛 GitHub Actions: CI
[error] 10-10: Ruff: I001 Import block is un-sorted or un-formatted.
🪛 GitHub Check: lint
[failure] 16-16: Ruff (UP035)
cortex/systemd_helper.py:16:1: UP035 typing.Tuple is deprecated, use tuple instead
[failure] 16-16: Ruff (UP035)
cortex/systemd_helper.py:16:1: UP035 typing.List is deprecated, use list instead
[failure] 16-16: Ruff (UP035)
cortex/systemd_helper.py:16:1: UP035 typing.Dict is deprecated, use dict instead
[failure] 10-24: Ruff (I001)
cortex/systemd_helper.py:10:1: I001 Import block is un-sorted or un-formatted
🪛 GitHub Check: Lint
[failure] 16-16: Ruff (UP035)
cortex/systemd_helper.py:16:1: UP035 typing.Tuple is deprecated, use tuple instead
[failure] 16-16: Ruff (UP035)
cortex/systemd_helper.py:16:1: UP035 typing.List is deprecated, use list instead
[failure] 16-16: Ruff (UP035)
cortex/systemd_helper.py:16:1: UP035 typing.Dict is deprecated, use dict instead
[failure] 10-24: Ruff (I001)
cortex/systemd_helper.py:10:1: I001 Import block is un-sorted or un-formatted
🤖 Prompt for AI Agents
In `@cortex/systemd_helper.py` around lines 10 - 24, The imports in
cortex/systemd_helper.py are unsorted and the file uses legacy typing
constructs; reorder and alphabetize imports (standard library first, then
third-party, then local) and remove unused typing imports, then update all type
annotations to use built-in generics and PEP 604 unions (e.g., replace List[str]
with list[str], Dict[str, str] with dict[str, str], Tuple[int, str, str] with
tuple[int, str, str], Optional[str] with str | None) throughout the file (look
for annotations in functions and dataclasses such as any occurrences of
Optional, List, Dict, Tuple, and update imports to drop those names from typing
and keep only needed names like Any if still required).
| @dataclass | ||
| class ServiceConfig: | ||
| """Configuration for generating a systemd unit file.""" | ||
|
|
||
| name: str | ||
| description: str | ||
| exec_start: str | ||
| service_type: ServiceType = ServiceType.SIMPLE | ||
| user: Optional[str] = None | ||
| group: Optional[str] = None | ||
| working_directory: Optional[str] = None | ||
| environment: Dict[str, str] = field(default_factory=dict) | ||
| restart: str = "on-failure" | ||
| restart_sec: int = 5 | ||
| wants: List[str] = field(default_factory=list) | ||
| after: List[str] = field(default_factory=lambda: ["network.target"]) | ||
| wanted_by: List[str] = field(default_factory=lambda: ["multi-user.target"]) | ||
| exec_stop: Optional[str] = None | ||
| exec_reload: Optional[str] = None | ||
| timeout_start_sec: int = 90 | ||
| timeout_stop_sec: int = 90 |
There was a problem hiding this comment.
Update type annotations to modern syntax.
Per static analysis hints, replace deprecated typing constructs with built-in generics and union syntax.
🔧 Proposed fix
`@dataclass`
class ServiceConfig:
"""Configuration for generating a systemd unit file."""
name: str
description: str
exec_start: str
service_type: ServiceType = ServiceType.SIMPLE
- user: Optional[str] = None
- group: Optional[str] = None
- working_directory: Optional[str] = None
- environment: Dict[str, str] = field(default_factory=dict)
+ user: str | None = None
+ group: str | None = None
+ working_directory: str | None = None
+ environment: dict[str, str] = field(default_factory=dict)
restart: str = "on-failure"
restart_sec: int = 5
- wants: List[str] = field(default_factory=list)
- after: List[str] = field(default_factory=lambda: ["network.target"])
- wanted_by: List[str] = field(default_factory=lambda: ["multi-user.target"])
- exec_stop: Optional[str] = None
- exec_reload: Optional[str] = None
+ wants: list[str] = field(default_factory=list)
+ after: list[str] = field(default_factory=lambda: ["network.target"])
+ wanted_by: list[str] = field(default_factory=lambda: ["multi-user.target"])
+ exec_stop: str | None = None
+ exec_reload: str | None = None
timeout_start_sec: int = 90
timeout_stop_sec: int = 90📝 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.
| @dataclass | |
| class ServiceConfig: | |
| """Configuration for generating a systemd unit file.""" | |
| name: str | |
| description: str | |
| exec_start: str | |
| service_type: ServiceType = ServiceType.SIMPLE | |
| user: Optional[str] = None | |
| group: Optional[str] = None | |
| working_directory: Optional[str] = None | |
| environment: Dict[str, str] = field(default_factory=dict) | |
| restart: str = "on-failure" | |
| restart_sec: int = 5 | |
| wants: List[str] = field(default_factory=list) | |
| after: List[str] = field(default_factory=lambda: ["network.target"]) | |
| wanted_by: List[str] = field(default_factory=lambda: ["multi-user.target"]) | |
| exec_stop: Optional[str] = None | |
| exec_reload: Optional[str] = None | |
| timeout_start_sec: int = 90 | |
| timeout_stop_sec: int = 90 | |
| `@dataclass` | |
| class ServiceConfig: | |
| """Configuration for generating a systemd unit file.""" | |
| name: str | |
| description: str | |
| exec_start: str | |
| service_type: ServiceType = ServiceType.SIMPLE | |
| user: str | None = None | |
| group: str | None = None | |
| working_directory: str | None = None | |
| environment: dict[str, str] = field(default_factory=dict) | |
| restart: str = "on-failure" | |
| restart_sec: int = 5 | |
| wants: list[str] = field(default_factory=list) | |
| after: list[str] = field(default_factory=lambda: ["network.target"]) | |
| wanted_by: list[str] = field(default_factory=lambda: ["multi-user.target"]) | |
| exec_stop: str | None = None | |
| exec_reload: str | None = None | |
| timeout_start_sec: int = 90 | |
| timeout_stop_sec: int = 90 |
🧰 Tools
🪛 GitHub Check: lint
[failure] 109-109: Ruff (UP006)
cortex/systemd_helper.py:109:12: UP006 Use list instead of List for type annotation
[failure] 108-108: Ruff (UP006)
cortex/systemd_helper.py:108:12: UP006 Use list instead of List for type annotation
[failure] 105-105: Ruff (UP006)
cortex/systemd_helper.py:105:18: UP006 Use dict instead of Dict for type annotation
[failure] 104-104: Ruff (UP045)
cortex/systemd_helper.py:104:24: UP045 Use X | None for type annotations
[failure] 103-103: Ruff (UP045)
cortex/systemd_helper.py:103:12: UP045 Use X | None for type annotations
[failure] 102-102: Ruff (UP045)
cortex/systemd_helper.py:102:11: UP045 Use X | None for type annotations
🪛 GitHub Check: Lint
[failure] 109-109: Ruff (UP006)
cortex/systemd_helper.py:109:12: UP006 Use list instead of List for type annotation
[failure] 108-108: Ruff (UP006)
cortex/systemd_helper.py:108:12: UP006 Use list instead of List for type annotation
[failure] 105-105: Ruff (UP006)
cortex/systemd_helper.py:105:18: UP006 Use dict instead of Dict for type annotation
[failure] 104-104: Ruff (UP045)
cortex/systemd_helper.py:104:24: UP045 Use X | None for type annotations
[failure] 103-103: Ruff (UP045)
cortex/systemd_helper.py:103:12: UP045 Use X | None for type annotations
[failure] 102-102: Ruff (UP045)
cortex/systemd_helper.py:102:11: UP045 Use X | None for type annotations
🤖 Prompt for AI Agents
In `@cortex/systemd_helper.py` around lines 94 - 114, The ServiceConfig dataclass
uses deprecated typing names from typing; update annotations to modern built-in
generics and union syntax: replace Optional[str] with str | None for user,
group, working_directory, exec_stop, exec_reload; replace Dict[str, str] with
dict[str, str] for environment; replace List[str] with list[str] for wants,
after, wanted_by; keep ServiceType as-is if it’s a concrete enum/class; adjust
any default_factory type hints accordingly so all annotations use built-in
generics and PEP 604 union syntax.
| for key, value in config.environment.items(): | ||
| lines.append(f"Environment={key}={value}") | ||
|
|
There was a problem hiding this comment.
Environment values with spaces may need quoting.
Systemd requires quotes around environment variable values that contain spaces. The current implementation may produce invalid unit files for values like Environment=KEY=value with spaces.
🔧 Proposed fix
for key, value in config.environment.items():
- lines.append(f"Environment={key}={value}")
+ # Quote values containing spaces or special characters
+ if ' ' in value or '"' in value:
+ escaped_value = value.replace('"', '\\"')
+ lines.append(f'Environment="{key}={escaped_value}"')
+ else:
+ lines.append(f"Environment={key}={value}")📝 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.
| for key, value in config.environment.items(): | |
| lines.append(f"Environment={key}={value}") | |
| for key, value in config.environment.items(): | |
| # Quote values containing spaces or special characters | |
| if ' ' in value or '"' in value: | |
| escaped_value = value.replace('"', '\\"') | |
| lines.append(f'Environment="{key}={escaped_value}"') | |
| else: | |
| lines.append(f"Environment={key}={value}") |
🤖 Prompt for AI Agents
In `@cortex/systemd_helper.py` around lines 454 - 456, The loop that builds unit
Environment lines (iterating config.environment and calling
lines.append(f"Environment={key}={value}")) can produce invalid systemd unit
entries when values contain spaces or special chars; update the code that
constructs these lines (the for key, value in config.environment.items() block
in cortex/systemd_helper.py) to quote and escape environment values: if a value
contains spaces or characters that require quoting, wrap it in double quotes and
escape any embedded double quotes and backslashes so the resulting lines.append
receives a valid systemd Environment=KEY="escaped value" string.
![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 very useful systemd helper to the Cortex CLI. The feature is well-implemented with plain English explanations, failure diagnosis, and dependency visualization, which will greatly improve the user experience when managing services. The code is well-structured, and the inclusion of comprehensive unit tests is excellent.
I've provided a few suggestions to improve maintainability and correctness in the new systemd_helper.py module. These include making exception handling more specific, improving the robustness of parser logic, and clarifying the conditions for failure diagnosis.
Overall, this is a great addition to the project.
| timeout=30 | ||
| ) | ||
| return result.stdout | ||
| except Exception: |
There was a problem hiding this comment.
Using a broad except Exception: can hide underlying issues, such as journalctl not being found or permission errors, making debugging difficult as the function will silently return an empty string. It's better to catch more specific exceptions to make the code more robust and predictable.
| except Exception: | |
| except (FileNotFoundError, subprocess.TimeoutExpired, OSError): |
| if key == "LoadState": | ||
| status.load_state = value | ||
| elif key == "ActiveState": | ||
| status.active_state = value | ||
| elif key == "SubState": | ||
| status.sub_state = value | ||
| elif key == "Description": | ||
| status.description = value | ||
| elif key == "MainPID": | ||
| status.main_pid = int(value) if value.isdigit() else 0 | ||
| elif key == "MemoryCurrent": | ||
| if value.isdigit(): | ||
| mb = int(value) / 1024 / 1024 | ||
| status.memory = f"{mb:.1f} MB" | ||
| elif key == "CPUUsageNSec": | ||
| if value.isdigit(): | ||
| sec = int(value) / 1_000_000_000 | ||
| status.cpu = f"{sec:.2f}s" | ||
| elif key == "TasksCurrent": | ||
| status.tasks = int(value) if value.isdigit() else 0 | ||
| elif key == "ActiveEnterTimestamp": | ||
| status.since = value | ||
| elif key == "Result": | ||
| status.result = value | ||
| elif key == "FragmentPath": | ||
| status.fragment_path = value | ||
| elif key == "Documentation": | ||
| if value: | ||
| status.docs = value.split() |
There was a problem hiding this comment.
This long if/elif chain for parsing the systemctl show output can be hard to maintain and extend. Consider refactoring this into a more data-driven approach, such as a dictionary mapping property names to parsing functions. This would make the code more declarative and easier to manage when new properties are added.
For example, you could define a parsers dictionary:
_STATUS_PARSERS = {
"LoadState": lambda s, v: setattr(s, 'load_state', v),
"ActiveState": lambda s, v: setattr(s, 'active_state', v),
"MainPID": lambda s, v: setattr(s, 'main_pid', int(v) if v.isdigit() else 0),
"MemoryCurrent": lambda s, v: setattr(s, 'memory', f"{int(v) / 1024 / 1024:.1f} MB" if v.isdigit() else ''),
# etc.
}
# and in the loop:
key, value = ...
if key in self._STATUS_PARSERS:
self._STATUS_PARSERS[key](status, value)| if status.active_state not in ["failed", "inactive"] and status.result == "success": | ||
| return True, f"Service '{service}' is running normally.", [] |
There was a problem hiding this comment.
The current logic for determining if a service is running normally is a bit complex and can lead to incorrect behavior. For instance, it will attempt to diagnose a service that was gracefully stopped (inactive state with success result). A simpler and more direct check would be to only diagnose services that are actually in a failed state, as this is the purpose of the diagnose command.
| if status.active_state not in ["failed", "inactive"] and status.result == "success": | |
| return True, f"Service '{service}' is running normally.", [] | |
| if status.active_state != "failed": | |
| return True, f"Service '{service}' is not in a failed state (current state: {status.active_state}).", [] |
| if "=" not in line: | ||
| continue | ||
| key, value = line.split("=", 1) | ||
| key_lower = key.lower().replace("by", "_by") |
There was a problem hiding this comment.
The string replacement key.lower().replace("by", "_by") is brittle. It works for the current set of properties (WantedBy, RequiredBy), but it might lead to incorrect keys if systemd adds new properties containing "by". A more robust and explicit approach is to use a dictionary to map property names to your internal keys.
For example:
KEY_MAP = {
"Wants": "wants",
"Requires": "requires",
"After": "after",
"Before": "before",
"WantedBy": "wanted_by",
"RequiredBy": "required_by",
}
# ...
key_lower = KEY_MAP.get(key)
if key_lower in deps and value:
# ...
#448) - Add systemd_helper.py with comprehensive systemd management: - Plain English status explanations - Failure diagnosis with actionable recommendations - Visual dependency tree display - Unit file generation from simple descriptions - Features: - Explains service states (active, failed, masked, etc.) - Analyzes logs for common issues (permissions, port conflicts) - Generates unit files from description + command - Shows dependency relationships visually - Add 36 comprehensive tests - Integrate `cortex systemd <service> [status|diagnose|deps]` command Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
mikejmorgan-ai
force-pushed
the
feature/issue-448-systemd-helper
branch
from
20eb0c8 to
c908b80
Compare
|
Summary
cortex systemd <service>command for plain English systemd managementFeatures
Usage
Example Output
Test plan
Fixes #448
🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
systemdcommand for service management with actions:status: Display service state, configuration, and resource usagediagnose: Analyze service failures with logs and recommended solutionsdeps: Visualize service dependency trees--verboseflag for detailed outputTests
✏️ Tip: You can customize this high-level summary in your review settings.