feat: add CLI commands documentation and service management features#8280
feat: add CLI commands documentation and service management features#8280Soulter wants to merge 1 commit into
Conversation
- Updated the VitePress configuration to include links to CLI commands in both English and Chinese. - Enhanced the AstrBot deployment documentation with instructions for installing it as a system service. - Created comprehensive CLI commands documentation covering initialization, service management, configuration, and plugin management. - Added tests for CLI command aliases and service functionalities to ensure proper command registration and behavior. - Implemented service log management features, including enabling application logging and controlling log visibility.
There was a problem hiding this comment.
Code Review
This pull request introduces a background service management system for Linux, macOS, and Windows, alongside command renaming (conf to config, plug to plugin) and aliasing. It adds the service command for installation, lifecycle management, and log viewing, supported by new tests and documentation. Feedback suggests optimizing log reading for large files using backward-seek, respecting XDG_CONFIG_HOME on Linux, and using binary mode for log following to ensure safe file seeking.
| def _read_last_lines(path: Path, lines: int) -> list[str]: | ||
| with path.open("r", encoding="utf-8", errors="replace") as f: | ||
| return list(deque(f, maxlen=lines)) |
There was a problem hiding this comment.
|
|
||
|
|
||
| def _systemd_unit_path(service_name: str) -> Path: | ||
| return Path.home() / ".config" / "systemd" / "user" / f"{service_name}.service" |
There was a problem hiding this comment.
On Linux, it is recommended to respect the XDG_CONFIG_HOME environment variable for configuration and service files instead of hardcoding ~/.config.
| return Path.home() / ".config" / "systemd" / "user" / f"{service_name}.service" | |
| config_home = os.environ.get("XDG_CONFIG_HOME") | |
| base = Path(config_home) if config_home else Path.home() / ".config" | |
| return base / "systemd" / "user" / f"{service_name}.service" |
| with path.open("r", encoding="utf-8", errors="replace") as f: | ||
| f.seek(previous_position) | ||
| for line in f: | ||
| if len(paths) > 1: | ||
| click.echo(f"[{path.name}] ", nl=False) | ||
| _echo_log_line(line) | ||
| positions[path] = f.tell() |
There was a problem hiding this comment.
Seeking to a byte offset in a file opened in text mode is technically undefined behavior in Python unless the offset was obtained from tell() in the same encoding context. For log following, it is safer to open the file in binary mode, seek to the byte offset, and then decode the read content.
| with path.open("r", encoding="utf-8", errors="replace") as f: | |
| f.seek(previous_position) | |
| for line in f: | |
| if len(paths) > 1: | |
| click.echo(f"[{path.name}] ", nl=False) | |
| _echo_log_line(line) | |
| positions[path] = f.tell() | |
| with path.open("rb") as f: | |
| f.seek(previous_position) | |
| for line in f: | |
| if len(paths) > 1: | |
| click.echo(f"[{path.name}] ", nl=False) | |
| _echo_log_line(line.decode("utf-8", errors="replace")) | |
| positions[path] = f.tell() |
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The new
cmd_service.pyfile is quite large and mixes cross-platform logic, config handling, logging, and CLI wiring; consider splitting it into smaller modules (e.g., per-platform service backends and a separate logs/config helper) to keep each piece easier to navigate and maintain. - There is duplicated logic for resolving log directories/paths (e.g.,
_macos_log_dir,_windows_log_dir,_service_log_paths,_resolve_data_path,_resolve_app_log_path); consolidating this into a single logging-path utility would reduce the risk of divergence between platforms and simplify future changes.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new `cmd_service.py` file is quite large and mixes cross-platform logic, config handling, logging, and CLI wiring; consider splitting it into smaller modules (e.g., per-platform service backends and a separate logs/config helper) to keep each piece easier to navigate and maintain.
- There is duplicated logic for resolving log directories/paths (e.g., `_macos_log_dir`, `_windows_log_dir`, `_service_log_paths`, `_resolve_data_path`, `_resolve_app_log_path`); consolidating this into a single logging-path utility would reduce the risk of divergence between platforms and simplify future changes.
## Individual Comments
### Comment 1
<location path="astrbot/cli/commands/cmd_service.py" line_range="612" />
<code_context>
+ )
+
+
+def _get_service_state(service_name: str) -> ServiceState:
+ system = platform.system()
+ if system == "Linux":
</code_context>
<issue_to_address>
**issue (complexity):** Consider introducing a shared ServiceBackend abstraction and central dispatcher to handle platform-specific behavior instead of repeating platform.system() conditionals across multiple functions.
You can reduce a lot of the structural complexity without losing any functionality by introducing a small `ServiceBackend` abstraction and routing all the `platform.system()` branching through a single dispatcher.
Right now, `install`, `_get_service_state`, `_control_service`, `_uninstall_service`, `_show_service_logs`, etc. all repeat the same `if system == "Linux" / "Darwin" / "Windows"` pattern.
A minimal, incremental refactor:
1. **Introduce a backend interface + dispatcher**
```python
from typing import Protocol
class ServiceBackend(Protocol):
manager_name: str
def install(self, service_name: str, executable: Path, workdir: Path,
*, force: bool, now: bool) -> None: ...
def control(self, service_name: str, action: str) -> None: ...
def uninstall(self, service_name: str) -> Path | str: ...
def status(self, service_name: str) -> ServiceState: ...
def show_logs(self, service_name: str, lines: int, follow: bool,
*, include_stderr: bool) -> None: ...
```
```python
_BACKENDS: dict[str, ServiceBackend] = {}
def _get_backend() -> ServiceBackend:
system = platform.system()
try:
return _BACKENDS[system]
except KeyError:
raise click.ClickException(f"Unsupported platform: {system}")
```
2. **Wire existing logic into concrete backends**
You can move (or wrap) the existing per-platform helpers into classes, one per OS:
```python
class SystemdBackend:
manager_name = "systemd --user"
def install(self, service_name, executable, workdir, *, force, now):
_install_systemd_user_service(service_name, executable, workdir,
force=force, now=now)
def control(self, service_name, action):
_control_systemd_service(service_name, action)
def uninstall(self, service_name):
return _uninstall_systemd_service(service_name)
def status(self, service_name) -> ServiceState:
return _get_systemd_state(service_name)
def show_logs(self, service_name, lines, follow, *, include_stderr):
_show_journal_logs(service_name, lines, follow)
```
Likewise for `launchd` and Windows, by delegating to `_install_launch_agent`, `_start_launch_agent`/`_stop_launch_agent`, `_uninstall_launch_agent`, `_get_launchd_state`, `_show_log_files`, etc.
Then register them:
```python
_BACKENDS["Linux"] = SystemdBackend()
_BACKENDS["Darwin"] = LaunchdBackend()
_BACKENDS["Windows"] = WindowsTaskBackend()
```
3. **Simplify the command handlers and helpers**
Once the backends exist, most branching collapses to a single call:
```python
def install(...):
...
backend = _get_backend()
backend.install(service_name, astrbot_executable, astrbot_root,
force=force, now=now)
# platform-specific messaging still allowed here if needed
```
```python
def _get_service_state(service_name: str) -> ServiceState:
return _get_backend().status(service_name)
```
```python
def _control_service(service_name: str, action: str) -> None:
_get_backend().control(service_name, action)
```
```python
def _uninstall_service(service_name: str) -> Path | str:
return _get_backend().uninstall(service_name)
```
```python
def _show_service_logs(...):
_get_backend().show_logs(service_name, lines, follow,
include_stderr=include_stderr)
```
This keeps all existing behavior (you’re just moving code into methods), but:
- **Removes repeated `platform.system()` conditionals**.
- **Centralizes platform dispatch** in `_get_backend()`.
- Makes it straightforward later to split each backend into its own module (`service_systemd.py`, `service_launchd.py`, `service_windows.py`) without changing the command-layer code.
You can implement this incrementally: introduce the backends, delegate to existing helpers, then optionally move the helper implementations into their respective backend modules once tests/CI are green.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| ) | ||
|
|
||
|
|
||
| def _get_service_state(service_name: str) -> ServiceState: |
There was a problem hiding this comment.
issue (complexity): Consider introducing a shared ServiceBackend abstraction and central dispatcher to handle platform-specific behavior instead of repeating platform.system() conditionals across multiple functions.
You can reduce a lot of the structural complexity without losing any functionality by introducing a small ServiceBackend abstraction and routing all the platform.system() branching through a single dispatcher.
Right now, install, _get_service_state, _control_service, _uninstall_service, _show_service_logs, etc. all repeat the same if system == "Linux" / "Darwin" / "Windows" pattern.
A minimal, incremental refactor:
- Introduce a backend interface + dispatcher
from typing import Protocol
class ServiceBackend(Protocol):
manager_name: str
def install(self, service_name: str, executable: Path, workdir: Path,
*, force: bool, now: bool) -> None: ...
def control(self, service_name: str, action: str) -> None: ...
def uninstall(self, service_name: str) -> Path | str: ...
def status(self, service_name: str) -> ServiceState: ...
def show_logs(self, service_name: str, lines: int, follow: bool,
*, include_stderr: bool) -> None: ..._BACKENDS: dict[str, ServiceBackend] = {}
def _get_backend() -> ServiceBackend:
system = platform.system()
try:
return _BACKENDS[system]
except KeyError:
raise click.ClickException(f"Unsupported platform: {system}")- Wire existing logic into concrete backends
You can move (or wrap) the existing per-platform helpers into classes, one per OS:
class SystemdBackend:
manager_name = "systemd --user"
def install(self, service_name, executable, workdir, *, force, now):
_install_systemd_user_service(service_name, executable, workdir,
force=force, now=now)
def control(self, service_name, action):
_control_systemd_service(service_name, action)
def uninstall(self, service_name):
return _uninstall_systemd_service(service_name)
def status(self, service_name) -> ServiceState:
return _get_systemd_state(service_name)
def show_logs(self, service_name, lines, follow, *, include_stderr):
_show_journal_logs(service_name, lines, follow)Likewise for launchd and Windows, by delegating to _install_launch_agent, _start_launch_agent/_stop_launch_agent, _uninstall_launch_agent, _get_launchd_state, _show_log_files, etc.
Then register them:
_BACKENDS["Linux"] = SystemdBackend()
_BACKENDS["Darwin"] = LaunchdBackend()
_BACKENDS["Windows"] = WindowsTaskBackend()- Simplify the command handlers and helpers
Once the backends exist, most branching collapses to a single call:
def install(...):
...
backend = _get_backend()
backend.install(service_name, astrbot_executable, astrbot_root,
force=force, now=now)
# platform-specific messaging still allowed here if neededdef _get_service_state(service_name: str) -> ServiceState:
return _get_backend().status(service_name)def _control_service(service_name: str, action: str) -> None:
_get_backend().control(service_name, action)def _uninstall_service(service_name: str) -> Path | str:
return _get_backend().uninstall(service_name)def _show_service_logs(...):
_get_backend().show_logs(service_name, lines, follow,
include_stderr=include_stderr)This keeps all existing behavior (you’re just moving code into methods), but:
- Removes repeated
platform.system()conditionals. - Centralizes platform dispatch in
_get_backend(). - Makes it straightforward later to split each backend into its own module (
service_systemd.py,service_launchd.py,service_windows.py) without changing the command-layer code.
You can implement this incrementally: introduce the backends, delegate to existing helpers, then optionally move the helper implementations into their respective backend modules once tests/CI are green.
Modifications / 改动点
Screenshots or Test Results / 运行截图或测试结果
Checklist / 检查清单
😊 If there are new features added in the PR, I have discussed it with the authors through issues/emails, etc.
/ 如果 PR 中有新加入的功能,已经通过 Issue / 邮件等方式和作者讨论过。
👀 My changes have been well-tested, and "Verification Steps" and "Screenshots" have been provided above.
/ 我的更改经过了良好的测试,并已在上方提供了“验证步骤”和“运行截图”。
🤓 I have ensured that no new dependencies are introduced, OR if new dependencies are introduced, they have been added to the appropriate locations in
requirements.txtandpyproject.toml./ 我确保没有引入新依赖库,或者引入了新依赖库的同时将其添加到
requirements.txt和pyproject.toml文件相应位置。😮 My changes do not introduce malicious code.
/ 我的更改没有引入恶意代码。
Summary by Sourcery
Add cross-platform CLI service management and logging capabilities, rename core CLI commands to product-facing names with backward-compatible aliases, and document the full CLI workflow in both English and Chinese, including deployment as a background service.
New Features:
astrbot serviceCLI command group for installing, managing, and inspecting AstrBot as a user-level background service across Linux, macOS, and Windows.astrbot service logssubcommands to inspect, enable, disable, and read AstrBot log files.Enhancements:
configandpluginwhile preserving legacyconfandplugaliases in the CLI.Tests:
servicecommand group and its log-management capabilities.confandplugaliases still work while top-level help prefers the newconfigandpluginnames.