Skip to content

feat: add service command (install/uninstall/start/stop/restart/statu…#764

Closed
seanly wants to merge 2 commits intosipeed:mainfrom
seanly:service
Closed

feat: add service command (install/uninstall/start/stop/restart/statu…#764
seanly wants to merge 2 commits intosipeed:mainfrom
seanly:service

Conversation

@seanly
Copy link
Copy Markdown

@seanly seanly commented Feb 25, 2026

…s/logs)

  • Add pkg/service with launchd (macOS) and systemd user (Linux) backends
  • Add picoclaw service install|uninstall|start|stop|restart|status|logs|refresh
  • service logs: support -f/--follow for tailing; -n/--lines for line count
  • main: add service case, invokedCLIName(), printHelp entry for service
  • gateway: optional check to avoid double-run when service already running

📝 Description

🗣️ Type of Change

  • 🐞 Bug fix (non-breaking change which fixes an issue)
  • ✨ New feature (non-breaking change which adds functionality)
  • 📖 Documentation update
  • ⚡ Code refactoring (no functional changes, no api changes)

🤖 AI Code Generation

  • 🤖 Fully AI-generated (100% AI, 0% Human)
  • 🛠️ Mostly AI-generated (AI draft, Human verified/modified)
  • 👨‍💻 Mostly Human-written (Human lead, AI assisted or none)

🔗 Related Issue

📚 Technical Context (Skip for Docs)

  • Reference URL:
  • Reasoning:

🧪 Test Environment

  • Hardware:
  • OS:
  • Model/Provider:
  • Channels:

📸 Evidence (Optional)

Click to view Logs/Screenshots

☑️ Checklist

  • My code/docs follow the style of this project.
  • I have performed a self-review of my own changes.
  • I have updated the documentation accordingly.

YS Liu and others added 2 commits February 25, 2026 14:17
…s/logs)

- Add pkg/service with launchd (macOS) and systemd user (Linux) backends
- Add picoclaw service install|uninstall|start|stop|restart|status|logs|refresh
- service logs: support -f/--follow for tailing; -n/--lines for line count
- main: add service case, invokedCLIName(), printHelp entry for service
- gateway: optional check to avoid double-run when service already running

Co-authored-by: Cursor <cursoragent@cursor.com>
- Add docs/service.md: install/uninstall/start/stop/restart/status/logs/refresh
- Platform support (launchd, systemd user, WSL), file locations, PATH/env
- Logs options -n/--lines and -f/--follow, troubleshooting
- README: add service to commands table with link, clarify gateway (foreground)

Co-authored-by: Cursor <cursoragent@cursor.com>
@xiaket xiaket added the type: enhancement New feature or request label Feb 25, 2026
Copy link
Copy Markdown

@nikolasdehor nikolasdehor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a substantial 1300+ line PR adding full service management (launchd + systemd). The architecture is clean — Manager interface with platform-specific implementations and build tag separation. Several observations:

Positives:

  • Good abstraction: Manager interface, commandRunner for testability, platform stubs
  • Robust launchd handling: retry logic in Install(), "already bootstrapped" handling, graceful Stop() for "no such process"
  • writeFileIfChanged avoids unnecessary daemon-reload
  • Double-run detection in gatewayCmd (checking XPC_SERVICE_NAME / INVOCATION_ID to avoid false positives when already running as a service)
  • Good documentation in docs/service.md

Issues:

  1. tailFileLines reads entire file into memory: Logs() on macOS reads the full log files with os.ReadFile. If gateway.log grows to hundreds of MB, this will spike memory. Consider using tail -n subprocess (as LogsFollow already does) or scanning from the end of the file.

  2. Plist path uses 0644 permissions: os.WriteFile(m.plistPath, []byte(plist), 0644) — the plist itself is not sensitive, but consistent 0600 would be safer since it contains the executable path and could be a target for symlink attacks if another user can write to ~/Library/LaunchAgents.

  3. No test coverage: For a 1300-line feature touching service management, the only test change is a minor fix to filesystem_test.go. The commandRunner interface is there for testability but no tests use it. At minimum, resolveServiceExecutablePath, parseServiceLogsOptions, buildSystemdPath, and appendUniquePath should have unit tests.

  4. renderLaunchdPlist is vulnerable to XML injection: If exePath or label contains characters like <, >, or &, the generated plist XML will be malformed. Use xml.EscapeText or at least validate the inputs.

  5. Uninstall calls enable after bootout: In launchdManager.Uninstall(), the sequence is bootout then enable then Remove. The enable call after bootout seems wrong — it re-enables the service right before deleting the plist, which could cause launchd to attempt to load a now-deleted file.

  6. buildSystemdPath name is misleading: This function is also used for launchd (macOS). The name should reflect that it is shared.

  7. Missing detectWSL: The function detectWSL() is called in NewManager but I do not see its definition in the diff. Is it defined elsewhere?

Overall a well-structured feature but needs tests and the security items addressed before merge.

@keithy
Copy link
Copy Markdown
Contributor

keithy commented Mar 5, 2026

#1130 Supports plugins for this kind of feature

@CLAassistant
Copy link
Copy Markdown

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.


YS Liu seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

@sipeed-bot
Copy link
Copy Markdown

sipeed-bot bot commented Mar 25, 2026

@seanly Hi! This PR has had no activity for over 2 weeks, so I'm closing it for now to keep things organized. Feel free to reopen anytime if you'd like to continue.

@sipeed-bot sipeed-bot bot closed this Mar 25, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

type: enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants