feat(roborev): add roborev code review daemon service - #1745
Conversation
Configures launchd agent on galactica (darwin) and systemd user service on matic (linux) to run `roborev daemon run`, gated on the host flags from `lib/host.nix`. Mirrors the dolt/qmd dual-platform service pattern. Binary path defaults to `~/.local/bin/roborev` (install.sh target); data dir at `~/.roborev` is created via home activation.
|
|
You do not have enough credits to review this pull request. Please purchase more credits to continue. |
📝 WalkthroughWalkthroughThis pull request adds a new Roborev service module to Home Manager. It conditionally enables the daemon on specific hosts, creates a data directory via activation, and provides OS-specific service wiring for macOS and Linux with proper environment variables and lifecycle management. ChangesRoborev Service Module
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Mesa DescriptionTL;DRAdds the What changed?
Description generated by Mesa. Update settings |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds the roborev service to Home Manager, including an activation script for directory initialization and service definitions for Darwin (launchd) and Linux (systemd). The review feedback suggests several improvements for robustness and idiomatic Nix code: utilizing config.home.profileDirectory instead of hardcoded paths in environment variables, relocating macOS log files from /tmp to a user-specific directory to avoid permission conflicts, and quoting the ExecStart path in the systemd service to handle potential spaces in file paths.
| EnvironmentVariables = { | ||
| HOME = homeDir; | ||
| ROBOREV_DATA_DIR = dataDir; | ||
| PATH = "${homeDir}/.local/bin:/opt/homebrew/bin:/usr/local/bin:/usr/bin:/bin"; |
There was a problem hiding this comment.
The PATH for the Darwin launchd agent is missing the Nix profile directory. This will likely cause the daemon to fail if it depends on tools installed via Nix (such as git). Additionally, using config.home.profileDirectory is more idiomatic than hardcoding .nix-profile.
PATH = "${homeDir}/.local/bin:${config.home.profileDirectory}/bin:/opt/homebrew/bin:/usr/local/bin:/usr/bin:/bin";
| StandardOutPath = "/tmp/roborev.log"; | ||
| StandardErrorPath = "/tmp/roborev.error.log"; |
There was a problem hiding this comment.
Using /tmp for log files in a launchd agent is discouraged. /tmp is a shared namespace on macOS, which can lead to permission conflicts between users and security risks (e.g., symlink attacks). Furthermore, /tmp is cleared on reboot. It is better to use a user-specific path like ~/Library/Logs/.
StandardOutPath = "${homeDir}/Library/Logs/roborev.log";
StandardErrorPath = "${homeDir}/Library/Logs/roborev.error.log";
| }; | ||
| Service = { | ||
| Type = "notify"; | ||
| ExecStart = "${roborevBin} daemon run"; |
There was a problem hiding this comment.
The ExecStart path should be quoted to handle cases where the home directory path might contain spaces. While uncommon, quoting is a best practice for robustness in systemd unit files.
ExecStart = "\"${roborevBin}\" daemon run";
| Environment = [ | ||
| "HOME=${homeDir}" | ||
| "ROBOREV_DATA_DIR=${dataDir}" | ||
| "PATH=${homeDir}/.local/bin:${homeDir}/.nix-profile/bin:/usr/local/bin:/usr/bin:/bin" |
There was a problem hiding this comment.
Use config.home.profileDirectory instead of hardcoding ${homeDir}/.nix-profile for better maintainability and adherence to Home Manager idioms.
"PATH=${homeDir}/.local/bin:${config.home.profileDirectory}/bin:/usr/local/bin:/usr/bin:/bin"
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
home-manager/services/roborev/default.nix (1)
35-36: ⚡ Quick winConsider persistent log locations.
The log files in
/tmp/will be cleared on reboot. For production use, consider using persistent locations like~/Library/Logs/roborev.logon macOS or rely on system logging facilities.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@home-manager/services/roborev/default.nix` around lines 35 - 36, The service currently writes logs to ephemeral files via the StandardOutPath and StandardErrorPath settings for the roborev service; change these to persistent locations or switch to system logging: either set StandardOutPath/StandardErrorPath to a durable directory (e.g., under the user home or XDG state/logs) or replace them by configuring StandardOutput= and StandardError= to "syslog" (or add a SyslogIdentifier) so logs go to the system logger; update the roborev service definition accordingly and ensure the chosen directory exists with correct permissions before enabling the service.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@home-manager/services/roborev/default.nix`:
- Around line 35-36: The service currently writes logs to ephemeral files via
the StandardOutPath and StandardErrorPath settings for the roborev service;
change these to persistent locations or switch to system logging: either set
StandardOutPath/StandardErrorPath to a durable directory (e.g., under the user
home or XDG state/logs) or replace them by configuring StandardOutput= and
StandardError= to "syslog" (or add a SyslogIdentifier) so logs go to the system
logger; update the roborev service definition accordingly and ensure the chosen
directory exists with correct permissions before enabling the service.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9dd6d514-a247-4f99-8da7-dbcbae70a574
📒 Files selected for processing (3)
home-manager/services/default.nixhome-manager/services/roborev/activate.shhome-manager/services/roborev/default.nix
![indent-zero[bot]](https://avatars.githubusercontent.com/in/1426746?s=60&v=4){kind=small}
| Service = { | ||
| Type = "notify"; | ||
| ExecStart = "${roborevBin} daemon run"; | ||
| Restart = "on-failure"; |
There was a problem hiding this comment.
Restart semantics diverge between platforms. On galactica, KeepAlive = true makes launchd respawn the agent on any exit (including clean exit 0). On matic, Restart = "on-failure" only respawns on non-zero exit / signal. So the same operator action — e.g. a graceful SIGTERM, a self-exit on idle, or a roborev daemon self-stop — leaves the daemon dead on matic but auto-restarted on galactica.
The sibling services this PR mirrors (dolt, qmd) both use Restart = "always" precisely to match KeepAlive=true. Suggest doing the same here unless there's a reason to want clean exits to stick on Linux only.
| Restart = "on-failure"; | |
| Restart = "always"; |
| After = [ "network.target" ]; | ||
| }; | ||
| Service = { | ||
| Type = "notify"; |
There was a problem hiding this comment.
Type = "notify" is gated on roborev >= 0.50. sd_notify readiness signaling was only added upstream in v0.50 (alongside the bundled systemd units). Nothing in the activation script or this module asserts the binary at ~/.local/bin/roborev is new enough.
Failure mode with an older binary on matic: systemd waits TimeoutStartSec (default 90s) for READY=1, marks the unit failed, then Restart=on-failure + RestartSec=5 retriggers the same 90s timeout in a loop. No symptom on galactica because launchd doesn't use sd_notify.
Options:
- Drop to
Type = "simple"(matches dolt/qmd pattern, works with any roborev version, you lose readiness signaling but the daemon isKeepAlive/Restart-managed anyway). - Keep
Type = "notify"and pin the min version somewhere visible (README/install.sh) so a stale install isn't silently broken.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 3 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="home-manager/services/roborev/default.nix">
<violation number="1" location="home-manager/services/roborev/default.nix:33">
P2: Include the Home Manager profile bin directory in the launchd PATH; otherwise the daemon may not find tools installed via Nix profile.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| EnvironmentVariables = { | ||
| HOME = homeDir; | ||
| ROBOREV_DATA_DIR = dataDir; | ||
| PATH = "${homeDir}/.local/bin:/opt/homebrew/bin:/usr/local/bin:/usr/bin:/bin"; |
There was a problem hiding this comment.
P2: Include the Home Manager profile bin directory in the launchd PATH; otherwise the daemon may not find tools installed via Nix profile.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At home-manager/services/roborev/default.nix, line 33:
<comment>Include the Home Manager profile bin directory in the launchd PATH; otherwise the daemon may not find tools installed via Nix profile.</comment>
<file context>
@@ -0,0 +1,61 @@
+ EnvironmentVariables = {
+ HOME = homeDir;
+ ROBOREV_DATA_DIR = dataDir;
+ PATH = "${homeDir}/.local/bin:/opt/homebrew/bin:/usr/local/bin:/usr/bin:/bin";
+ };
+ StandardOutPath = "/tmp/roborev.log";
</file context>
…v service targets - prefix every nix invocation in nix-build/nix-switch with HOST=<host> HOSTNAME=<host> so lib/host.nix detects galactica/matic during flake eval - use sudo env HOST= HOSTNAME= for darwin-rebuild and nixos-rebuild to bypass sudo env stripping - add launchctl-roborev (gated on galactica) and systemctl-roborev (gated on matic) targets and wire them into the launchctl/systemctl aggregate targets
1 participant
Summary
home-manager/services/roborevconfiguringroborev daemon runas a launchd agent on galactica (darwin) and a systemd user service on matic (linux).inputs.host.isGalactica || inputs.host.isMatic; mirrors the dual-platform pattern used byhome-manager/services/doltandhome-manager/services/qmd.~/.roborevwith mode 0700 (matchespaperclipactivation pattern). Binary expected at~/.local/bin/roborev(defaultcurl install.shtarget).What roborev is
Local CLI for delegating code review to AI coding agents - background daemon + post-commit hook + TUI. Upstream: https://github.com/roborev-dev/roborev. Systemd unit replicated from
packaging/systemd/roborev.service(Type=notify, restart-on-failure, RestartSec=5).Test plan
nix flake checkpasses (darwin checks incl.eval-darwin-galactica)make build+make switchon galacticalaunchctl list | grep roborevshows the agent and/tmp/roborev.logpopulatesnixosConfigurations.maticon a linux host (cannot run from darwin)Summary by cubic
Adds the
roborevcode review daemon as a managed Home Manager service for galactica (launchd) and matic (systemd), auto-starting on login with restart. Also updates the Makefile to passHOST/HOSTNAMEinto builds/switches and adds convenience restart targets.New Features
home-manager/services/roborevgated by host flags (launchd on Darwin, systemd user on Linux).~/.roborev(0700); runsroborev daemon runwithROBOREV_DATA_DIRand PATH; expects~/.local/bin/roborev./tmp/roborev.logand/tmp/roborev.error.log; systemd usesType=notify,Restart=on-failure,RestartSec=5.HOST/HOSTNAMEinto all nix build/switch commands; addslaunchctl-roborevandsystemctl-roborev, wired into aggregatelaunchctl/systemctl.Migration
~/.local/bin/roborev.make nix-build/make nix-switchnow passHOST/HOSTNAMEautomatically.make launchctl-roborev(galactica) ormake systemctl-roborev(matic); check vialaunchctl list | grep roborevorsystemctl --user status roborev.Written for commit 2cda980. Summary will update on new commits.