feat: add systemd service helper with plain-English explanations #557
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughAdds a new Systemd helper module and integrates a Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant CLI as Cortex CLI
participant Helper as SystemdHelper
participant Systemctl as systemctl
participant Journal as journalctl
User->>CLI: cortex systemd diagnose <service>
CLI->>Helper: diagnose_failure(<service>, lines)
Helper->>Systemctl: systemctl show/status <service>
Systemctl-->>Helper: service metadata & state
Helper->>Journal: journalctl -u <service> -n <lines>
Journal-->>Helper: recent logs
Helper->>Helper: analyze logs, map exit codes, build report
Helper-->>CLI: formatted diagnosis output
CLI-->>User: display report
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
CLA Verification PassedAll contributors have signed the CLA.
|
Summary of ChangesHello @jeremylongshore, 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 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 an excellent and very useful systemd helper feature. The implementation is well-structured, includes comprehensive unit tests, and provides a clear user experience. My feedback focuses on fixing a few bugs, improving the robustness of parsing logic, and increasing maintainability by reducing code duplication and removing unused code. Overall, this is a great addition to the cortex CLI.
| action = getattr(args, "systemd_action", None) | ||
|
|
||
| if action == "status": | ||
| try: | ||
| run_status_command(args.service) | ||
| return 0 | ||
| except RuntimeError as e: | ||
| console.print(f"[red]Error: {e}[/red]") | ||
| return 1 | ||
|
|
||
| elif action == "diagnose": | ||
| try: | ||
| run_diagnose_command(args.service) | ||
| return 0 | ||
| except RuntimeError as e: | ||
| console.print(f"[red]Error: {e}[/red]") | ||
| return 1 | ||
|
|
||
| elif action == "deps": | ||
| try: | ||
| run_deps_command(args.service) | ||
| return 0 | ||
| except RuntimeError as e: | ||
| console.print(f"[red]Error: {e}[/red]") | ||
| return 1 | ||
|
|
||
| elif action == "generate": | ||
| try: | ||
| run_generate_command() | ||
| return 0 | ||
| except RuntimeError as e: | ||
| console.print(f"[red]Error: {e}[/red]") | ||
| return 1 | ||
|
|
||
| else: | ||
| console.print("[bold]Systemd Service Helper[/bold]") | ||
| console.print() | ||
| console.print("Commands:") | ||
| console.print(" status <service> - Explain service status in plain English") | ||
| console.print(" diagnose <service> - Diagnose why a service failed") | ||
| console.print(" deps <service> - Show service dependencies") | ||
| console.print(" generate - Interactive unit file generator") | ||
| return 0 |
There was a problem hiding this comment.
This block has two issues:
- Bug: The
--linesargument for thediagnosecommand is parsed but never used, so the user's input is always ignored. - Maintainability: The
try...exceptblock is repeated for each subcommand, leading to duplicated code.
I suggest refactoring this into a single try...except block that also correctly passes the lines argument. This is cleaner, fixes the bug, and makes error handling easier to manage.
Note that this change requires updating the run_diagnose_command function in cortex/systemd_helper.py to accept the lines argument, which I've commented on separately.
action = getattr(args, "systemd_action", None)
if not action:
console.print("[bold]Systemd Service Helper[/bold]")
console.print()
console.print("Commands:")
console.print(" status <service> - Explain service status in plain English")
console.print(" diagnose <service> - Diagnose why a service failed")
console.print(" deps <service> - Show service dependencies")
console.print(" generate - Interactive unit file generator")
return 0
try:
if action == "status":
run_status_command(args.service)
elif action == "diagnose":
run_diagnose_command(args.service, lines=args.lines)
elif action == "deps":
run_deps_command(args.service)
elif action == "generate":
run_generate_command()
except RuntimeError as e:
console.print(f"[red]Error: {e}[/red]")
return 1
return 0
cortex/systemd_helper.py
Outdated
| # Count indentation (each level is 2 spaces or tree chars) | ||
| indent = 0 | ||
| clean_line = line | ||
| for char in line: | ||
| if char in " │├└─": | ||
| indent += 1 | ||
| else: | ||
| break | ||
|
|
||
| # Get the service name | ||
| clean_line = re.sub(r"[│├└─\s]+", "", line).strip() | ||
| if not clean_line: | ||
| continue | ||
|
|
||
| level = indent // 2 |
There was a problem hiding this comment.
The current logic for parsing the dependency tree from systemctl list-dependencies is fragile. The re.sub on line 381 will incorrectly strip spaces from service names that contain them (e.g., 'my service.service' would become 'myservice.service').
A more robust approach would be to use a method that separates the indentation prefix from the service name without altering the name itself.
| # Count indentation (each level is 2 spaces or tree chars) | |
| indent = 0 | |
| clean_line = line | |
| for char in line: | |
| if char in " │├└─": | |
| indent += 1 | |
| else: | |
| break | |
| # Get the service name | |
| clean_line = re.sub(r"[│├└─\s]+", "", line).strip() | |
| if not clean_line: | |
| continue | |
| level = indent // 2 | |
| # Determine indentation and service name more robustly | |
| indent = len(line) - len(line.lstrip(" │├└─")) | |
| clean_line = line.lstrip(" │├└─").strip() | |
| if not clean_line: | |
| continue | |
| level = indent // 2 |
| unit_content = self.generate_unit_file( | ||
| description=description, | ||
| exec_start=exec_start, | ||
| service_name=service_name, |
There was a problem hiding this comment.
To align with the removal of the unused service_name parameter from generate_unit_file, this argument should also be removed from the function call.
tests/test_systemd_helper.py
Outdated
| # diagnose_failure calls get_service_status (which calls subprocess.run) | ||
| # then calls subprocess.run for journalctl | ||
| with patch("subprocess.run", side_effect=[mock_status, mock_status, mock_journal]): | ||
| report = helper.diagnose_failure("myservice") |
There was a problem hiding this comment.
The tests for diagnose_failure don't cover the lines parameter. It would be beneficial to add a test case that passes a custom lines value and then asserts that the journalctl command was called with the corresponding -n argument. This would ensure the feature works as expected and prevent future regressions.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In @cortex/cli.py:
- Around line 2030-2036: The diagnose CLI parses a --lines option but never
passes it into the diagnose routine; update the call to run_diagnose_command to
pass args.lines (e.g., run_diagnose_command(args.service, args.lines) in
cortex/cli.py) and update the run_diagnose_command signature in
cortex/systemd_helper.py to accept a lines parameter with the existing default
(e.g., def run_diagnose_command(service, lines=50):) and forward that value into
diagnose_failure so the CLI option is honored; also update any other callers of
run_diagnose_command if present.
In @cortex/systemd_helper.py:
- Around line 120-125: The subprocess.run call that executes ["systemctl",
"show", service_name, "--no-pager"] can fail silently; update the function (the
block using subprocess.run and the local variable result for service_name) to
handle failures by either using check=True or by inspecting result.returncode
and, on non-zero, raising or returning a clear error containing result.stderr
(and service_name) instead of proceeding with an empty properties dict; ensure
the error path is logged or propagated so callers of this helper see real
failure reasons rather than a misleading empty status.
- Around line 455-457: The Environment line builder currently emits raw values
(Environment={key}={value}) which breaks unit files when values contain spaces;
update the loop that appends these lines (the block producing
f"Environment={key}={value}") to properly quote and escape the value per systemd
unit file rules (e.g., emit Environment=KEY="escaped value"), ensuring you
escape existing double quotes and backslashes inside value (or use a safe
quoting utility such as shlex.quote-like logic adapted for systemd) before
formatting the string.
🧹 Nitpick comments (4)
tests/test_systemd_helper.py (1)
289-343: Diagnose failure tests cover key error patterns.The tests verify detection of permission errors, port conflicts, and missing files. Consider adding a test for the case when
journalctltimes out or returns an error.💡 Optional: Add timeout handling test
def test_diagnose_journalctl_timeout(self, helper): status_output = """ActiveState=failed SubState=failed """ mock_status = MagicMock(returncode=0, stdout=status_output) with patch("subprocess.run", side_effect=[mock_status, mock_status, subprocess.TimeoutExpired("journalctl", 15)]): # Should handle gracefully, not raise report = helper.diagnose_failure("myservice") assert "myservice" in reportcortex/systemd_helper.py (3)
7-20: Remove unused imports.
Path(line 11),box(line 14), andTable(line 17) are imported but never used in this module.♻️ Proposed fix
import re import subprocess from dataclasses import dataclass from enum import Enum -from pathlib import Path from typing import Optional -from rich import box from rich.panel import Panel from rich.prompt import Confirm, Prompt -from rich.table import Table from rich.tree import Tree from cortex.branding import console
509-523: Thestart_on_bootvariable is collected but not used.The wizard asks if the service should start on boot (line 513), but this value isn't passed to
generate_unit_file. TheWantedBy=multi-user.targetis always included regardless.While the current behavior is likely the intended default (you'd want
WantedByfor enable to work), if the user answers "No" to start on boot, they might expect the[Install]section to be omitted.Consider either:
- Removing the prompt if it doesn't affect the output
- Using the value to conditionally include the
[Install]section
353-358: Add error handling for list-dependencies failures.Similar to other subprocess calls,
systemctl list-dependenciesshould handle potential failures gracefully.♻️ Proposed fix
- result = subprocess.run( - ["systemctl", "list-dependencies", service_name, "--no-pager"], - capture_output=True, - text=True, - timeout=10, - ) + try: + result = subprocess.run( + ["systemctl", "list-dependencies", service_name, "--no-pager"], + capture_output=True, + text=True, + timeout=10, + ) + except subprocess.TimeoutExpired: + tree = Tree(f"[bold cyan]{service_name}[/bold cyan]") + tree.add("[yellow]Timed out fetching dependencies[/yellow]") + return tree
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
cortex/cli.pycortex/systemd_helper.pydemo_systemd.castdemo_systemd.shtests/test_systemd_helper.py
🧰 Additional context used
📓 Path-based instructions (2)
**/*.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_systemd_helper.pycortex/cli.pycortex/systemd_helper.py
tests/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Maintain >80% test coverage for pull requests
Files:
tests/test_systemd_helper.py
🧬 Code graph analysis (2)
demo_systemd.sh (2)
cortex/cli.py (2)
systemd(2004-2062)status(1006-1014)cortex/permission_manager.py (1)
diagnose(65-85)
cortex/cli.py (1)
cortex/systemd_helper.py (4)
run_deps_command(562-571)run_diagnose_command(551-559)run_generate_command(574-577)run_status_command(540-548)
🔇 Additional comments (8)
demo_systemd.sh (1)
1-36: LGTM! Demo script is clear and well-structured.The script effectively demonstrates the systemd helper commands with appropriate pacing. A few minor observations:
- The error suppression on line 26 (
2>/dev/null) is appropriate for demo purposes sincesshtypically won't be in a failed state.- Consider adding a note in the comment about making the script executable (
chmod +x demo_systemd.sh) for users who want to run it directly.demo_systemd.cast (1)
1-122: LGTM! Valid asciinema recording file.The recording file is properly formatted and demonstrates the systemd helper features effectively. The simulated outputs align with the expected CLI behavior.
cortex/cli.py (2)
2004-2062: Well-structured systemd command handler.The implementation follows the established patterns in the codebase:
- Lazy imports inside the method
- Consistent error handling with
RuntimeError- Helpful fallback usage message when no action is specified
The method is properly documented with type hints and docstring.
2334-2365: Argument parser configuration is complete and well-organized.The subparser setup correctly mirrors the other CLI commands in the file, with appropriate help text for each action.
tests/test_systemd_helper.py (2)
98-209: Good test coverage for service status retrieval.The tests properly mock subprocess calls and verify the parsing logic for various service states. The fixture pattern is clean and reusable.
442-498: Run command wrapper tests are thorough.The tests correctly mock both subprocess calls and Rich prompts for the interactive generator. The side_effect sequences properly simulate the multi-step subprocess interactions.
cortex/systemd_helper.py (2)
68-100: Well-designed class with proper initialization checks.The
_check_systemd_availablemethod appropriately validates the environment before allowing operations. Good defensive programming.
405-470: Unit file generation is comprehensive and follows systemd conventions.The method generates valid unit file content with sensible defaults. The structure follows systemd best practices with
Type=simple,Restart=on-failure, andWantedBy=multi-user.target.
Anshgrover23
left a comment
Anshgrover23
left a comment
There was a problem hiding this comment.
@jeremylongshore Address bot comments.
|
Thanks for the review feedback! I've addressed all the bot comments: Changes made:
All 37 tests passing. Ready for re-review. |
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In @cortex/systemd_helper.py:
- Around line 531-538: The service_name argument is passed into
generate_unit_file but never used; either remove it from the signature and all
calls (including the call in interactive_unit_generator where unit_content is
created) or update generate_unit_file to embed service_name into the generated
unit content (e.g., as a Description or a drop-in/alias field) so it's not
unused; change the function signature for generate_unit_file and all call sites
(or add usage inside generate_unit_file) and update any docstrings/comments
accordingly, referencing generate_unit_file and interactive_unit_generator to
locate the call and parameter.
- Around line 142-153: The current mapping in cortex/systemd_helper.py treats
ActiveState="active" with SubState="exited" as UNKNOWN; update the conditional
in the function that sets `state` (the block using
ServiceState.RUNNING/FAILED/INACTIVE/ACTIVATING/DEACTIVATING/UNKNOWN) to
explicitly handle `active_state == "active" and sub_state == "exited"` and map
it to the appropriate enum (e.g., ServiceState.EXITED or ServiceState.SUCCEEDED
if that exists) before the final else branch, so active+exited services are
reported correctly.
- Around line 464-472: The Environment value builder doesn't escape dollar signs
for systemd variable expansion; update the loop that handles environment items
so any '$' in value is replaced with '$$' before further escaping (i.e., do
value = value.replace("$", "$$") then escape backslashes and double quotes as
done now), and ensure this replacement is applied for both the quoted branch
(escaped_value used in f'Environment={key}="{escaped_value}"') and the unquoted
branch so literal '$' is preserved.
🧹 Nitpick comments (2)
cortex/systemd_helper.py (2)
7-12:shleximport is currently unused.The
shlexmodule is imported but not utilized. The custom quoting logic ingenerate_unit_file(lines 467-472) handles escaping manually. Consider usingshlex.quote()for more robust shell escaping, or remove the import if not needed.♻️ Optional: Use shlex.quote for environment values
if environment: for key, value in environment.items(): # Quote values that contain spaces or special characters - if " " in value or '"' in value or "'" in value or "\\" in value: - # Escape existing double quotes and backslashes - escaped_value = value.replace("\\", "\\\\").replace('"', '\\"') - lines.append(f'Environment={key}="{escaped_value}"') - else: - lines.append(f"Environment={key}={value}") + # Always quote to handle edge cases safely + lines.append(f"Environment={key}={shlex.quote(value)}")
21-31:ServiceState.STOPPEDis defined but never assigned.The
STOPPEDstate is declared butget_service_statusnever assigns it—stopped services map toINACTIVE. This is technically correct (systemd uses "inactive" for stopped services), but the unused enum member may cause confusion.Consider either removing
STOPPEDor documenting why it exists for future use.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cortex/cli.pycortex/systemd_helper.py
🧰 Additional context used
📓 Path-based instructions (1)
**/*.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:
cortex/systemd_helper.pycortex/cli.py
🧬 Code graph analysis (1)
cortex/cli.py (1)
cortex/systemd_helper.py (4)
run_deps_command(578-587)run_diagnose_command(566-575)run_generate_command(590-593)run_status_command(555-563)
🔇 Additional comments (5)
cortex/cli.py (2)
2004-2063: LGTM! Well-structured systemd command handler.The implementation follows the established patterns in this file:
- Proper docstring with Args and Returns sections
- Consistent error handling with
RuntimeErrorcatch blocks- Fallback help output when no action is specified
- Clean delegation to
run_*_commandfunctions fromsystemd_helper
2334-2365: Argument parser configuration looks correct.The subparser setup follows the established patterns and correctly wires up all four systemd actions with appropriate arguments.
cortex/systemd_helper.py (3)
360-369: Good timeout handling for dependency listing.The timeout handling with a graceful fallback message is well implemented here.
555-593: Wrapper functions are well-structured.Each function properly instantiates
SystemdHelper(which validates systemd availability) and delegates to the appropriate method. The CLI layer correctly catchesRuntimeErrorexceptions that may be raised.
1-18: Well-documented module with proper type hints.The module follows the coding guidelines with PEP 8 style, type hints on all public methods, and comprehensive docstrings. Good use of the Rich library for CLI output.
| if active_state == "active" and sub_state == "running": | ||
| state = ServiceState.RUNNING | ||
| elif active_state == "failed": | ||
| state = ServiceState.FAILED | ||
| elif active_state == "inactive": | ||
| state = ServiceState.INACTIVE | ||
| elif active_state == "activating": | ||
| state = ServiceState.ACTIVATING | ||
| elif active_state == "deactivating": | ||
| state = ServiceState.DEACTIVATING | ||
| else: | ||
| state = ServiceState.UNKNOWN |
There was a problem hiding this comment.
Services with active state but exited sub-state are mapped to UNKNOWN.
A service that completed successfully (e.g., Type=oneshot) will have ActiveState=active and SubState=exited. The current logic only maps active + running to RUNNING, leaving active + exited as UNKNOWN.
🔧 Suggested fix to handle active+exited
if active_state == "active" and sub_state == "running":
state = ServiceState.RUNNING
+ elif active_state == "active" and sub_state == "exited":
+ state = ServiceState.STOPPED # Oneshot service completed successfully
elif active_state == "failed":
state = ServiceState.FAILED🤖 Prompt for AI Agents
In @cortex/systemd_helper.py around lines 142 - 153, The current mapping in
cortex/systemd_helper.py treats ActiveState="active" with SubState="exited" as
UNKNOWN; update the conditional in the function that sets `state` (the block
using ServiceState.RUNNING/FAILED/INACTIVE/ACTIVATING/DEACTIVATING/UNKNOWN) to
explicitly handle `active_state == "active" and sub_state == "exited"` and map
it to the appropriate enum (e.g., ServiceState.EXITED or ServiceState.SUCCEEDED
if that exists) before the final else branch, so active+exited services are
reported correctly.
cortex/systemd_helper.py
Outdated
| # Get recent logs | ||
| result = subprocess.run( | ||
| ["journalctl", "-u", service_name, "-n", str(lines), "--no-pager"], | ||
| capture_output=True, | ||
| text=True, | ||
| timeout=15, | ||
| ) |
There was a problem hiding this comment.
Missing exception handling for journalctl subprocess call.
Unlike get_service_status and show_dependencies, this subprocess call lacks exception handling for TimeoutExpired and FileNotFoundError. If journalctl is unavailable or times out, an unhandled exception will propagate.
🐛 Proposed fix to add exception handling
# Get recent logs
- result = subprocess.run(
- ["journalctl", "-u", service_name, "-n", str(lines), "--no-pager"],
- capture_output=True,
- text=True,
- timeout=15,
- )
+ try:
+ result = subprocess.run(
+ ["journalctl", "-u", service_name, "-n", str(lines), "--no-pager"],
+ capture_output=True,
+ text=True,
+ timeout=15,
+ )
+ logs = result.stdout
+ except subprocess.TimeoutExpired:
+ logs = ""
+ report.append("[yellow]Timed out fetching journal logs[/yellow]")
+ except FileNotFoundError:
+ logs = ""
+ report.append("[yellow]journalctl not found - cannot fetch logs[/yellow]")
- logs = result.stdoutThere was a problem hiding this comment.
@jeremylongshore Address this one as well all of these.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| if environment: | ||
| for key, value in environment.items(): | ||
| # Quote values that contain spaces or special characters | ||
| if " " in value or '"' in value or "'" in value or "\\" in value: | ||
| # Escape existing double quotes and backslashes | ||
| escaped_value = value.replace("\\", "\\\\").replace('"', '\\"') | ||
| lines.append(f'Environment={key}="{escaped_value}"') | ||
| else: | ||
| lines.append(f"Environment={key}={value}") |
There was a problem hiding this comment.
Environment variable handling may not escape $ correctly.
Systemd interprets $ in Environment values for variable expansion. If a value contains $, it should be escaped as $$ to be treated literally. The current quoting logic handles spaces and quotes but not dollar signs.
🔧 Suggested fix to escape dollar signs
if environment:
for key, value in environment.items():
# Quote values that contain spaces or special characters
- if " " in value or '"' in value or "'" in value or "\\" in value:
+ # Escape $ for systemd (use $$ for literal $)
+ escaped_value = value.replace("$", "$$")
+ if " " in escaped_value or '"' in escaped_value or "'" in escaped_value or "\\" in escaped_value:
# Escape existing double quotes and backslashes
- escaped_value = value.replace("\\", "\\\\").replace('"', '\\"')
+ escaped_value = escaped_value.replace("\\", "\\\\").replace('"', '\\"')
lines.append(f'Environment={key}="{escaped_value}"')
else:
- lines.append(f"Environment={key}={value}")
+ lines.append(f"Environment={key}={escaped_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.
| if environment: | |
| for key, value in environment.items(): | |
| # Quote values that contain spaces or special characters | |
| if " " in value or '"' in value or "'" in value or "\\" in value: | |
| # Escape existing double quotes and backslashes | |
| escaped_value = value.replace("\\", "\\\\").replace('"', '\\"') | |
| lines.append(f'Environment={key}="{escaped_value}"') | |
| else: | |
| lines.append(f"Environment={key}={value}") | |
| if environment: | |
| for key, value in environment.items(): | |
| # Quote values that contain spaces or special characters | |
| # Escape $ for systemd (use $$ for literal $) | |
| escaped_value = value.replace("$", "$$") | |
| if " " in escaped_value or '"' in escaped_value or "'" in escaped_value or "\\" in escaped_value: | |
| # Escape existing double quotes and backslashes | |
| escaped_value = escaped_value.replace("\\", "\\\\").replace('"', '\\"') | |
| lines.append(f'Environment={key}="{escaped_value}"') | |
| else: | |
| lines.append(f"Environment={key}={escaped_value}") |
🤖 Prompt for AI Agents
In @cortex/systemd_helper.py around lines 464 - 472, The Environment value
builder doesn't escape dollar signs for systemd variable expansion; update the
loop that handles environment items so any '$' in value is replaced with '$$'
before further escaping (i.e., do value = value.replace("$", "$$") then escape
backslashes and double quotes as done now), and ensure this replacement is
applied for both the quoted branch (escaped_value used in
f'Environment={key}="{escaped_value}"') and the unquoted branch so literal '$'
is preserved.
| unit_content = self.generate_unit_file( | ||
| description=description, | ||
| exec_start=exec_start, | ||
| service_name=service_name, | ||
| user=user, | ||
| working_dir=working_dir, | ||
| restart=restart, | ||
| ) |
There was a problem hiding this comment.
service_name parameter passed to generate_unit_file but never used in the generated content.
The service_name is collected from the user and passed to generate_unit_file, but the method never incorporates it into the unit file output. It's only used for display purposes in interactive_unit_generator (line 541).
Either remove service_name from generate_unit_file's parameters, or document that it's intentionally unused in the generated content (since the filename determines the service name in systemd).
🤖 Prompt for AI Agents
In @cortex/systemd_helper.py around lines 531 - 538, The service_name argument
is passed into generate_unit_file but never used; either remove it from the
signature and all calls (including the call in interactive_unit_generator where
unit_content is created) or update generate_unit_file to embed service_name into
the generated unit content (e.g., as a Description or a drop-in/alias field) so
it's not unused; change the function signature for generate_unit_file and all
call sites (or add usage inside generate_unit_file) and update any
docstrings/comments accordingly, referencing generate_unit_file and
interactive_unit_generator to locate the call and parameter.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
Adding myself to enable CLA checks on PRs cortexlinux#557 and cortexlinux#558. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
@Anshgrover23 I've addressed the bot comments in my latest commits. Also, I've created PR #568 to add myself to cla-signers.json - once merged, the CLA check should pass. Ready for re-review when you have a chance! |
Anshgrover23
left a comment
Anshgrover23
left a comment
There was a problem hiding this comment.
@jeremylongshore Address CodeRabbit comments.
|
CLA PR #568 has been merged. Could you please re-run the CLA check? Thanks! |
|
recheck |
|
The CLA failure is because of the Could you please add |
Adding myself to enable CLA checks on PRs cortexlinux#557 and cortexlinux#558. Co-authored-by: jeremylongshore <jeremylongshore@users.noreply.github.com> Co-authored-by: Claude Opus 4.5 <noreply@anthropic.com>
Implements the systemd service helper feature requested in cortexlinux#448: - Plain-English status explanations for any service - Failure diagnosis with log analysis and actionable suggestions - Visual dependency tree display - Interactive unit file generator wizard CLI commands added: - cortex systemd status <service> - cortex systemd diagnose <service> - cortex systemd deps <service> - cortex systemd generate Includes 37 unit tests with 95% code coverage. Closes cortexlinux#448
- Pass --lines CLI option to run_diagnose_command (fixes unused arg) - Add error handling for systemctl show failures with proper error messages - Add timeout handling to list-dependencies call - Fix Environment line quoting for values containing spaces/special chars - Remove unused imports (Path, box, Table) - Add shlex import for potential future quoting needs
jeremylongshore
force-pushed
the
feature/systemd-helper
branch
from
96b62aa to
2ee1378
Compare
|
Rebased and removed the Co-Authored-By trailers. CLA check is now passing. Ready for review! |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @cortex/systemd_helper.py:
- Around line 414-424: The method signature for generate_unit_file still uses
typing.Optional[...] which is causing CI failures; update the annotations to use
PEP 604 union syntax: change Optional[str] -> str | None for service_name, user,
working_dir; change Optional[list[str]] -> list[str] | None for after; change
Optional[dict[str, str]] -> dict[str, str] | None for environment (keep return
type str unchanged and do not add imports).
- Around line 7-12: Remove the unused import of shlex from
cortex/systemd_helper.py; locate the import statement that reads "import shlex"
(alongside imports like re, subprocess, dataclass, Enum, Optional) and delete
that line so the module no longer includes an unused import.
- Around line 52-64: The annotations pid: Optional[int] = None and exit_code:
Optional[int] = None should use modern union syntax: pid: int | None = None and
exit_code: int | None = None; update any other Optional[...] usages in this file
similarly and remove the now-unused Optional import from the top (line ~12) if
it’s not used elsewhere; ensure static typing checks pass after replacing
Optional with the pipe-style unions for the ServiceState-related fields inside
the class/ dataclass where name, state, description, load_state, active_state,
sub_state, pid, memory, cpu, started_at, exit_code, main_pid_code are defined.
🧹 Nitpick comments (1)
cortex/systemd_helper.py (1)
267-272: Consider handling journalctl failures more gracefully.The
subprocess.runcall for journalctl doesn't handle a non-zero return code. If journalctl fails (e.g., no logs available, permission denied), the code continues with potentially empty or error output without informing the user.♻️ Suggested improvement
# Get recent logs - result = subprocess.run( - ["journalctl", "-u", service_name, "-n", str(lines), "--no-pager"], - capture_output=True, - text=True, - timeout=15, - ) - - logs = result.stdout + try: + result = subprocess.run( + ["journalctl", "-u", service_name, "-n", str(lines), "--no-pager"], + capture_output=True, + text=True, + timeout=15, + ) + logs = result.stdout + except (subprocess.TimeoutExpired, FileNotFoundError) as e: + report.append(f"[yellow]Warning: Could not retrieve logs: {e}[/yellow]") + logs = ""
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
cortex/cli.pycortex/systemd_helper.pydemo_systemd.castdemo_systemd.shtests/test_systemd_helper.py
🚧 Files skipped from review as they are similar to previous changes (1)
- demo_systemd.sh
🧰 Additional context used
📓 Path-based instructions (2)
**/*.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:
cortex/cli.pytests/test_systemd_helper.pycortex/systemd_helper.py
tests/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Maintain >80% test coverage for pull requests
Files:
tests/test_systemd_helper.py
🧬 Code graph analysis (2)
cortex/cli.py (1)
cortex/systemd_helper.py (4)
run_deps_command(578-587)run_diagnose_command(566-575)run_generate_command(590-593)run_status_command(555-563)
cortex/systemd_helper.py (1)
tests/test_systemd_helper.py (6)
helper(102-105)helper(215-217)helper(269-271)helper(293-295)helper(350-352)helper(376-378)
🪛 GitHub Actions: CI
cortex/systemd_helper.py
[error] 58-58: ruff check failed. UP045 Use X | None for type annotations. Command: 'ruff check . --output-format=github'.
🪛 GitHub Check: lint
cortex/systemd_helper.py
[failure] 423-423: Ruff (UP045)
cortex/systemd_helper.py:423:22: UP045 Use X | None for type annotations
[failure] 422-422: Ruff (UP045)
cortex/systemd_helper.py:422:16: UP045 Use X | None for type annotations
[failure] 420-420: Ruff (UP045)
cortex/systemd_helper.py:420:22: UP045 Use X | None for type annotations
[failure] 419-419: Ruff (UP045)
cortex/systemd_helper.py:419:15: UP045 Use X | None for type annotations
[failure] 418-418: Ruff (UP045)
cortex/systemd_helper.py:418:23: UP045 Use X | None for type annotations
[failure] 62-62: Ruff (UP045)
cortex/systemd_helper.py:62:16: UP045 Use X | None for type annotations
[failure] 58-58: Ruff (UP045)
cortex/systemd_helper.py:58:10: UP045 Use X | None for type annotations
🪛 GitHub Check: Lint
cortex/systemd_helper.py
[failure] 423-423: Ruff (UP045)
cortex/systemd_helper.py:423:22: UP045 Use X | None for type annotations
[failure] 422-422: Ruff (UP045)
cortex/systemd_helper.py:422:16: UP045 Use X | None for type annotations
[failure] 420-420: Ruff (UP045)
cortex/systemd_helper.py:420:22: UP045 Use X | None for type annotations
[failure] 419-419: Ruff (UP045)
cortex/systemd_helper.py:419:15: UP045 Use X | None for type annotations
[failure] 418-418: Ruff (UP045)
cortex/systemd_helper.py:418:23: UP045 Use X | None for type annotations
[failure] 62-62: Ruff (UP045)
cortex/systemd_helper.py:62:16: UP045 Use X | None for type annotations
[failure] 58-58: Ruff (UP045)
cortex/systemd_helper.py:58:10: UP045 Use X | None for type annotations
⏰ 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.12)
- GitHub Check: test (3.10)
- GitHub Check: test (3.11)
🔇 Additional comments (20)
demo_systemd.cast (1)
1-122: Demo artifact looks good.The asciinema recording correctly demonstrates the new
cortex systemdcommands including status explanations, dependency visualization, and the help output. The format and content align with the PR objectives.cortex/cli.py (4)
2004-2063: Well-structured command handler.The
systemdmethod follows the established pattern in this CLI module. The error handling properly catchesRuntimeErrorfrom the helper functions and displays user-friendly messages. The fallback help output when no action is specified is helpful.
2334-2365: Argument parsing correctly wired.The subparser configuration properly defines all four systemd actions with appropriate arguments. The
--linesoption defaults match the helper function signature.
2103-2103: Help table entry added.Correctly adds the systemd command to the Rich help table.
2628-2629: Routing correctly added.The main() routing properly dispatches to
cli.systemd(args)when the systemd command is invoked.tests/test_systemd_helper.py (8)
1-19: Well-organized test module.The test file has clear structure with separate test classes for each component. Imports are clean and the module docstring properly describes the purpose.
22-65: Good coverage of data structures.Tests properly validate the
ServiceStateenum values andServiceStatusdataclass initialization, including default values and full field population.
67-96: Initialization scenarios well covered.Tests properly cover the four initialization paths: success, systemd unavailable, systemctl not found, and timeout. The exception matching is specific and appropriate.
98-209: Comprehensive status retrieval tests.The tests cover all service states (running, failed, inactive, activating, deactivating), validate proper parsing of systemctl output, and confirm the
.servicesuffix normalization behavior.
273-286: Efficient parametrized exit code testing.Good use of
@pytest.mark.parametrizeto cover multiple exit codes efficiently.
289-343: Diagnosis scenario tests validate key patterns.Tests cover common failure scenarios (permission errors, port conflicts, missing files) and verify the diagnostic report contains appropriate guidance.
372-439: Unit file generation tests are thorough.Tests validate the generated unit file content for various configurations including user, working directory, restart behavior, After directives, and environment variables.
442-499: Command wrapper tests verify integration.Tests properly mock the subprocess calls and Rich prompts to validate the run_* functions work correctly without requiring actual systemd.
cortex/systemd_helper.py (7)
21-31: Service state enum is well-defined.Good coverage of systemd service states with descriptive values.
66-98: SystemdHelper initialization with proper availability check.Good error handling for missing systemctl, non-systemd systems, and timeouts.
99-180: Service status parsing is robust.The method properly normalizes service names, handles subprocess errors, and parses the key-value output from
systemctl show. State determination logic correctly maps activeState/subState combinations.
360-369: Good timeout handling for dependency listing.The TimeoutExpired exception is caught and provides a user-friendly fallback message. This matches the fix mentioned in the PR comments.
464-472: Environment variable quoting handles common cases.The manual escaping for values with spaces and special characters follows systemd conventions. The quoting logic correctly escapes backslashes and double quotes.
487-552: Interactive generator provides clear UX.The wizard collects essential information through prompted questions and provides helpful installation instructions. The generated output is displayed in a styled panel.
555-593: Command wrappers follow clean interface pattern.Each
run_*_commandfunction creates a helper instance and delegates appropriately. The functions are well-documented with Args/Returns sections.
Anshgrover23
left a comment
Anshgrover23
left a comment
There was a problem hiding this comment.
@jeremylongshore Lint is failing.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @cortex/systemd_helper.py:
- Around line 266-271: Wrap the subprocess.run call that invokes journalctl in a
try/except that catches subprocess.TimeoutExpired (the same pattern used in
_check_systemd_available and get_service_status); on TimeoutExpired, log a clear
timeout error including the exception details and return or set a safe result
(e.g., an empty output or a CompletedProcess-like fallback with a non-zero
returncode and stderr describing the timeout) so the function handles the
timeout consistently instead of letting the exception propagate.
🧹 Nitpick comments (3)
cortex/systemd_helper.py (3)
8-8: Unused import:shlexThe
shlexmodule is imported but never used in this file.🧹 Proposed fix
import re -import shlex import subprocess
413-423: Unused parameter:service_nameingenerate_unit_file.The
service_nameparameter is accepted but never used in the generated unit file content. It's only used by the caller (interactive_unit_generator) for display purposes after generation. Consider either:
- Removing it from this method's signature, or
- Adding it as a comment in the generated file.
Option 1: Remove unused parameter
def generate_unit_file( self, description: str, exec_start: str, - service_name: str | None = None, user: str | None = None, working_dir: str | None = None, restart: bool = True, after: list[str] | None = None, environment: dict[str, str] | None = None, ) -> str:Then update the call site in
interactive_unit_generator:unit_content = self.generate_unit_file( description=description, exec_start=exec_start, - service_name=service_name, user=user, working_dir=working_dir, restart=restart, )Option 2: Use it in the generated file
# [Unit] section lines.append("[Unit]") lines.append(f"Description={description}") + if service_name: + lines.append(f"# Service: {service_name}")
526-537:start_on_bootdoesn't affect the generated unit file.The
start_on_bootprompt result (line 527) only affects the printed installation instructions, but the generated unit file always includesWantedBy=multi-user.targetin the[Install]section (line 481). If the user answers "no" to start on boot, the generated file will still contain the install section that enables boot-time startup.Consider either:
- Conditionally omitting the
[Install]section whenstart_on_bootis False, or- Clarifying in the prompt that this only affects the printed instructions.
🛠️ Proposed fix to conditionally include Install section
Update
generate_unit_filesignature and logic:def generate_unit_file( self, description: str, exec_start: str, service_name: str | None = None, user: str | None = None, working_dir: str | None = None, restart: bool = True, after: list[str] | None = None, environment: dict[str, str] | None = None, + install: bool = True, ) -> str:- # [Install] section - lines.append("[Install]") - lines.append("WantedBy=multi-user.target") - lines.append("") + # [Install] section + if install: + lines.append("[Install]") + lines.append("WantedBy=multi-user.target") + lines.append("")Then pass the value in
interactive_unit_generator:unit_content = self.generate_unit_file( description=description, exec_start=exec_start, service_name=service_name, user=user, working_dir=working_dir, restart=restart, + install=start_on_boot, )
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cortex/systemd_helper.py
🧰 Additional context used
📓 Path-based instructions (1)
**/*.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:
cortex/systemd_helper.py
🧬 Code graph analysis (1)
cortex/systemd_helper.py (1)
tests/test_systemd_helper.py (6)
helper(102-105)helper(215-217)helper(269-271)helper(293-295)helper(350-352)helper(376-378)
⏰ 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.11)
- GitHub Check: test (3.10)
- GitHub Check: test (3.12)
🔇 Additional comments (6)
cortex/systemd_helper.py (6)
20-29: LGTM with note onSTOPPEDstate.The enum is well-defined. Note that
ServiceState.STOPPEDis defined but never assigned inget_service_status()- the code maps toINACTIVEinstead. This appears intentional since systemd uses "inactive" terminology, but consider removingSTOPPEDif it's not needed or documenting when it would be used.
32-62: LGTM!Well-structured dataclass with comprehensive type hints and documentation. The attribute docstrings clearly explain each field's purpose.
74-96: LGTM!Good defensive coding with proper timeout handling and exception handling for
FileNotFoundErrorandTimeoutExpired. The RuntimeError messages are clear and actionable.
98-179: LGTM!Solid implementation with:
- Proper input validation for empty service names.
- Safe parsing using
partition()which correctly handles values containing=.- Defensive
isdigit()checks before integer conversion.- Appropriate timeout handling.
- Clear error messages on failure.
589-592: Missing return type hint onrun_generate_command.Per coding guidelines, type hints are required. The other helper functions have
-> None, but this one is missing it.-def run_generate_command() -> None: +def run_generate_command() -> None: """Run the interactive unit file generator."""Actually, looking again the type hint is present. Disregard.
554-592: LGTM!The helper functions are well-structured with proper type hints and docstrings. They correctly delegate to
SystemdHelpermethods and format output using Rich panels.
cortex/systemd_helper.py
Outdated
| result = subprocess.run( | ||
| ["journalctl", "-u", service_name, "-n", str(lines), "--no-pager"], | ||
| capture_output=True, | ||
| text=True, | ||
| timeout=15, | ||
| ) |
There was a problem hiding this comment.
Missing timeout exception handling for journalctl call.
The subprocess.run call has timeout=15 but the TimeoutExpired exception is not caught. This will cause an unhandled exception if journalctl takes too long, unlike the consistent handling in _check_systemd_available and get_service_status.
🛠️ Proposed fix
# Get recent logs
- result = subprocess.run(
- ["journalctl", "-u", service_name, "-n", str(lines), "--no-pager"],
- capture_output=True,
- text=True,
- timeout=15,
- )
-
- logs = result.stdout
+ try:
+ result = subprocess.run(
+ ["journalctl", "-u", service_name, "-n", str(lines), "--no-pager"],
+ capture_output=True,
+ text=True,
+ timeout=15,
+ )
+ logs = result.stdout
+ except subprocess.TimeoutExpired:
+ logs = ""
+ report.append("[yellow]Timed out fetching journal logs.[/yellow]")🤖 Prompt for AI Agents
In @cortex/systemd_helper.py around lines 266 - 271, Wrap the subprocess.run
call that invokes journalctl in a try/except that catches
subprocess.TimeoutExpired (the same pattern used in _check_systemd_available and
get_service_status); on TimeoutExpired, log a clear timeout error including the
exception details and return or set a safe result (e.g., an empty output or a
CompletedProcess-like fallback with a non-zero returncode and stderr describing
the timeout) so the function handles the timeout consistently instead of letting
the exception propagate.
There was a problem hiding this comment.
@jeremylongshore We do not need a demo script remove it, just show us in video itself.
Adding myself to enable CLA checks on PRs cortexlinux#557 and cortexlinux#558. Co-authored-by: jeremylongshore <jeremylongshore@users.noreply.github.com> Co-authored-by: Claude Opus 4.5 <noreply@anthropic.com>
Adding myself to enable CLA checks on PRs cortexlinux#557 and cortexlinux#558.
Security fixes (CRITICAL): - Add proper escaping for $, backticks, and newlines in Environment= values - Prevent shell injection via malicious environment variables Security fixes (HIGH): - Increase journalctl timeout to 30s + add --since "1 hour ago" - Add subprocess error handling with returncode check - Add FileNotFoundError handling for systemctl/journalctl - Add service name validation regex (^[a-zA-Z0-9_@.-]+$) - Add robust dependency tree parsing with error recovery - Remove unused shlex import Quality improvements: - Define timeout constants (SYSTEMCTL_TIMEOUT, JOURNALCTL_TIMEOUT) - Add comprehensive exit code explanations (141, 142, 129, 2, etc.) - Add formula for signal codes (128+N) - Add sudo notes to installation instructions - Standardize error message format Tests: - Add 62 comprehensive unit tests - Test service name validation with malicious inputs - Test environment variable escaping edge cases - Test timeout and error handling scenarios - 91% coverage on systemd_helper.py Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Alphabetically sort imports in test_systemd_helper.py to comply with ruff's I001 import block sorting rules. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
2 participants
Summary
Implements the systemd service helper feature requested in #448:
Demo
CLI Commands Added
Related Issue
Closes #448
Type of Change
AI Disclosure
Used Claude Code (Claude Opus 4.5) for implementation, test writing, and demo recording.
Testing
pytest tests/test_systemd_helper.py -vChecklist
Summary by CodeRabbit
New Features
Demos
Tests
✏️ Tip: You can customize this high-level summary in your review settings.