Skip to content

feat(roborev): add roborev code review daemon service #1745

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
shunkakinoki merged 2 commits into from
May 10, 2026
Merged

feat(roborev): add roborev code review daemon service #1745

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
772d932
feat(roborev): add roborev code review daemon service
shunkakinoki May 10, 2026
2cda980
feat(makefile): bake HOST/HOSTNAME into build and switch + add robore…
shunkakinoki May 10, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions home-manager/services/default.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ let
qmd = ./qmd;
openclaw = ./openclaw;
paperclip = ./paperclip;
roborev = ./roborev;
sshAgent = ./ssh-agent;
tmuxSessionLogger = import ./tmux-session-logger { inherit pkgs; };
in
Expand All @@ -48,6 +49,7 @@ in
qmd
openclaw
paperclip
roborev
sshAgent
tmuxSessionLogger
]
8 changes: 8 additions & 0 deletions home-manager/services/roborev/activate.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
#!/usr/bin/env bash
# Create roborev data directory with correct permissions
# Usage: activate.sh <data_dir>
set -euo pipefail
DATA_DIR="$1"

mkdir -p "$DATA_DIR"
chmod 700 "$DATA_DIR"
61 changes: 61 additions & 0 deletions home-manager/services/roborev/default.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
{
config,
lib,
pkgs,
inputs,
...
}:
let
inherit (inputs.host) isGalactica isMatic;
homeDir = config.home.homeDirectory;
roborevBin = "${homeDir}/.local/bin/roborev";
dataDir = "${homeDir}/.roborev";
enabled = isGalactica || isMatic;
in
lib.mkIf enabled {
home.activation.roborevSetup = lib.hm.dag.entryAfter [ "writeBoundary" ] ''
$DRY_RUN_CMD ${pkgs.bash}/bin/bash "${./activate.sh}" "${dataDir}"
'';

launchd.agents.roborev = lib.mkIf pkgs.stdenv.isDarwin {
enable = true;
config = {
ProgramArguments = [
roborevBin
"daemon"
"run"
];
KeepAlive = true;
RunAtLoad = true;
EnvironmentVariables = {
HOME = homeDir;
ROBOREV_DATA_DIR = dataDir;
PATH = "${homeDir}/.local/bin:/opt/homebrew/bin:/usr/local/bin:/usr/bin:/bin";

@gemini-code-assist gemini-code-assist Bot May 10, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

high

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";

@cubic-dev-ai cubic-dev-ai Bot May 10, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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>

};
StandardOutPath = "/tmp/roborev.log";
StandardErrorPath = "/tmp/roborev.error.log";
Comment on lines +35 to +36

@gemini-code-assist gemini-code-assist Bot May 10, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

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";

};
};

systemd.user.services.roborev = lib.mkIf pkgs.stdenv.isLinux {
Unit = {
Description = "roborev code review daemon";
Documentation = [ "https://github.com/roborev-dev/roborev" ];
After = [ "network.target" ];
};
Service = {
Type = "notify";

@indent-zero indent-zero Bot May 10, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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 is KeepAlive/Restart-managed anyway).
  • Keep Type = "notify" and pin the min version somewhere visible (README/install.sh) so a stale install isn't silently broken.

ExecStart = "${roborevBin} daemon run";

@gemini-code-assist gemini-code-assist Bot May 10, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

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";

Restart = "on-failure";

@indent-zero indent-zero Bot May 10, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

Suggested change
Restart = "on-failure";
Restart = "always";

RestartSec = 5;
Environment = [
"HOME=${homeDir}"
"ROBOREV_DATA_DIR=${dataDir}"
"PATH=${homeDir}/.local/bin:${homeDir}/.nix-profile/bin:/usr/local/bin:/usr/bin:/bin"

@gemini-code-assist gemini-code-assist Bot May 10, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

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"

];
};
Install = {
WantedBy = [ "default.target" ];
};
};
}
Loading