Data

{
  "owner": "mirendev",
  "repo": "runtime",
  "number": 969,
  "verdict": "caveats",
  "event": "comment",
  "summary": "This is a draft, so I'm judging readiness to hand off to human review. The core logic is solid and I'd be comfortable with most of it, but there's one concrete gap worth flagging before this graduates.\n\n**What's here and what I checked**\n\nThe PR adds `pkg/release/symlink.go` — a self-contained `EnsurePathSymlink` function — and wires it into two places: `ServerInstall` (linux CLI) and `Manager.UpgradeServer` (the upgrade path). The atomic swap via `os.CreateTemp` + `os.Remove` + `os.Symlink` + `os.Rename` is the right pattern for keeping an in-use binary's inode alive during replacement on Linux. The `ErrPathManagedElsewhere` sentinel and the `symlinkPointsInto` guard are well-reasoned: they prevent silently hijacking a Homebrew/Cask-managed link. The test coverage in `symlink_test.go` is thorough — it covers creation, stale-file replacement, stale-managed-symlink replacement, foreign-symlink preservation, and idempotency.\n\n**The one real gap: `ServerUninstall` doesn't remove the symlink**\n\nWhen `miren server uninstall` runs, it tears down the systemd service and optionally removes `/var/lib/miren`, but it never touches `/usr/local/bin/miren`. After uninstall, the symlink points at a binary that no longer exists (or will be removed). A user who then types `miren` gets a broken command with no obvious hint about what happened. Given that the PR specifically creates the symlink during install, cleaning it up on uninstall is the expected counterpart. It's not a security issue, but it is a correctness gap that a human reviewer will almost certainly ask about.\n\n**One minor implementation note (non-blocking)**\n\nIn `EnsurePathSymlink`, `os.CreateTemp` opens a file and returns an `*os.File`. The code calls `tmp.Close()` but there's a window between `Close` and `os.Remove` where the temp file sits on disk open-file-descriptorless. This is fine on Linux (the file still exists until `Remove`) and the code handles it correctly — I just want to note the sequence is correct, not a bug.\n\n**Summary**\n\nThe symlink mechanism itself is ready. The missing `ServerUninstall` cleanup is the one thing I'd want addressed (or at least consciously deferred with a TODO) before this goes to a human reviewer.",
  "comments": [
    {
      "path": "cli/commands/server_install.go",
      "line": 558,
      "side": "RIGHT",
      "body": "`ServerInstall` creates `/usr/local/bin/miren` as a symlink, but `ServerUninstall` never removes it. After uninstalling, `miren` on `$PATH` points at a deleted binary. Consider calling `os.Remove(release.SystemCLIPath)` (or a dedicated `release.RemovePathSymlink`) during uninstall — but only when it is the symlink we own (i.e. points into `releaseBinPath`'s directory), to avoid removing a link belonging to another tool.",
      "ai_prompt": "In cli/commands/server_install.go, the ServerUninstall function (starting around line 558) does not clean up the /usr/local/bin/miren symlink that ServerInstall creates. Add a step at the end of ServerUninstall (before the final return nil) that removes release.SystemCLIPath if and only if it is a symlink pointing into /var/lib/miren/release (i.e. use release.EnsurePathSymlink's existing symlinkPointsInto helper or an equivalent check via os.Readlink). If the symlink belongs to another tool, leave it alone and emit a ctx.Info note instead. If the remove fails, emit a ctx.Warn rather than returning an error, consistent with the best-effort pattern used in ServerInstall."
    }
  ],
  "posted_to_pr": true,
  "draft": true
}