Skip to content

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

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

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

@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 enhancement New feature or request label Feb 25, 2026
Copy link

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants