feat: add service command (install/uninstall/start/stop/restart/statu…#764
feat: add service command (install/uninstall/start/stop/restart/statu…#764seanly wants to merge 2 commits intosipeed:mainfrom
Conversation
…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>
nikolasdehor
left a comment
There was a problem hiding this comment.
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:
Managerinterface,commandRunnerfor testability, platform stubs - Robust launchd handling: retry logic in
Install(), "already bootstrapped" handling, gracefulStop()for "no such process" writeFileIfChangedavoids unnecessary daemon-reload- Double-run detection in
gatewayCmd(checkingXPC_SERVICE_NAME/INVOCATION_IDto avoid false positives when already running as a service) - Good documentation in
docs/service.md
Issues:
-
tailFileLinesreads entire file into memory:Logs()on macOS reads the full log files withos.ReadFile. If gateway.log grows to hundreds of MB, this will spike memory. Consider usingtail -nsubprocess (asLogsFollowalready does) or scanning from the end of the file. -
Plist path uses
0644permissions: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. -
No test coverage: For a 1300-line feature touching service management, the only test change is a minor fix to
filesystem_test.go. ThecommandRunnerinterface is there for testability but no tests use it. At minimum,resolveServiceExecutablePath,parseServiceLogsOptions,buildSystemdPath, andappendUniquePathshould have unit tests. -
renderLaunchdPlistis vulnerable to XML injection: IfexePathorlabelcontains characters like<,>, or&, the generated plist XML will be malformed. Usexml.EscapeTextor at least validate the inputs. -
Uninstallcallsenableafterbootout: InlaunchdManager.Uninstall(), the sequence isbootoutthenenablethenRemove. Theenablecall 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. -
buildSystemdPathname is misleading: This function is also used for launchd (macOS). The name should reflect that it is shared. -
Missing
detectWSL: The functiondetectWSL()is called inNewManagerbut 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.
…s/logs)
📝 Description
🗣️ Type of Change
🤖 AI Code Generation
🔗 Related Issue
📚 Technical Context (Skip for Docs)
🧪 Test Environment
📸 Evidence (Optional)
Click to view Logs/Screenshots
☑️ Checklist