feat(cli): add service subcommand for systemd user service management#23
Conversation
Adds `adk-claw service` with install, uninstall, start, stop, restart, status, and logs subcommands. The install flow writes a user-level systemd service file (~/.config/systemd/user/adk-claw.service), enables linger so the service survives SSH logout, and embeds the install-time PATH so nvm-managed node is found correctly at runtime. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Summary of ChangesHello @MarvelNwachukwu, 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
Changelog
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
|
There was a problem hiding this comment.
Code Review
This pull request introduces a service subcommand for managing adk-claw as a systemd user service. The implementation is comprehensive, covering installation, uninstallation, and lifecycle management commands. The installation process correctly handles systemd specifics like enabling linger and preserving the PATH environment variable.
My review focuses on improving security, robustness, and user experience. I've identified a potential command injection vulnerability and suggest using execFileSync for safer command execution. I also recommend a more efficient way to retrieve the current username and adding a platform check to ensure commands are only run on systems with systemd, providing clearer feedback to users on unsupported platforms.
| process.env.LOGNAME || | ||
| execSync("whoami", { encoding: "utf-8" }).trim(); | ||
| try { | ||
| execSync(`loginctl enable-linger ${username}`, { stdio: "pipe" }); |
There was a problem hiding this comment.
Using execSync with a template literal that includes a variable can lead to command injection vulnerabilities. If the username variable contains malicious shell commands, they could be executed. It's safer to use execFileSync and pass arguments as an array to prevent shell interpretation.
You'll need to import execFileSync from node:child_process.
| execSync(`loginctl enable-linger ${username}`, { stdio: "pipe" }); | |
| execFileSync("loginctl", ["enable-linger", username], { stdio: "pipe" }); |
| if (!existsSync(CONFIG_PATH)) { | ||
| p.log.error("ADK Claw is not initialized. Run 'adk-claw init' first."); | ||
| process.exit(1); | ||
| } |
There was a problem hiding this comment.
The service management commands are specific to systemd, which is typically found on Linux. Running these commands on other operating systems like macOS or Windows will fail with a potentially confusing error message (e.g., systemctl not found). It would improve the user experience to add a check at the beginning of this function to ensure systemd is available and provide a clear error message if it's not.
For example, you could check for the existence of the systemctl binary before proceeding:
try {
execSync('which systemctl', { stdio: 'pipe' });
} catch {
p.log.error('Service management requires systemd, which was not found.');
process.exit(1);
}| const username = | ||
| process.env.USER || | ||
| process.env.LOGNAME || | ||
| execSync("whoami", { encoding: "utf-8" }).trim(); |
There was a problem hiding this comment.
Spawning a child process with execSync('whoami') to get the username is inefficient. Node.js provides a built-in, cross-platform way to get this information via os.userInfo().username. This avoids the overhead of creating a new process and is more reliable.
You'll need to import userInfo from node:os at the top of the file.
| const username = | |
| process.env.USER || | |
| process.env.LOGNAME || | |
| execSync("whoami", { encoding: "utf-8" }).trim(); | |
| const username = userInfo().username; |
Dropped dead printHelp/printVersion functions (replaced by printSplash in main). Added service entry to the splash screen command list. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Replace execSync template literal with execFileSync for loginctl enable-linger to prevent command injection - Replace execSync whoami + env vars with os.userInfo().username for a simpler, cross-platform username lookup - Add systemctl existence check at the start of serviceCommand to give a clear error on non-systemd systems instead of a cryptic failure Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Adds
adk-claw servicewith install, uninstall, start, stop, restart, status, and logs subcommands. The install flow writes a user-level systemd service file (~/.config/systemd/user/adk-claw.service), enables linger so the service survives SSH logout, and embeds the install-time PATH so nvm-managed node is found correctly at runtime.