From 5739b39f071ca27ac514754dfecacda5d51ca3f8 Mon Sep 17 00:00:00 2001
From: Danylo <danylo@flamingo.cx>
Date: Wed, 23 Sep 2026 15:38:53 +0300
Subject: [PATCH 1/4] fix(client): harden tool supervision, updates and
 reinstall against partial failures

---
 clients/openframe-client/src/lib.rs           |   8 +-
 .../src/platform/preferences_writer.rs        |  31 ++++-
 .../src/platform/tool_updater/gui_app.rs      | 105 +++++++++++++--
 .../src/platform/tool_updater/standard.rs     |  51 ++++++-
 .../platform/tool_updater/standard_tests.rs   |  63 +++++++++
 clients/openframe-client/src/service.rs       |  40 +++++-
 .../src/services/github_download_service.rs   |  34 ++++-
 .../services/github_download_service_tests.rs |  73 +++++++++++
 .../services/initial_configuration_service.rs |   6 +-
 .../src/services/last_known_good_service.rs   |  13 +-
 .../src/services/tool_agent_update_service.rs |  99 ++++++++++++--
 .../src/services/tool_restart_service.rs      |  31 +----
 .../src/services/tool_run_manager.rs          | 124 +++++++++++++++---
 13 files changed, 584 insertions(+), 94 deletions(-)
 create mode 100644 clients/openframe-client/src/platform/tool_updater/standard_tests.rs
 create mode 100644 clients/openframe-client/src/services/github_download_service_tests.rs

diff --git a/clients/openframe-client/src/lib.rs b/clients/openframe-client/src/lib.rs
index 1cafd22bf1..eef0de74b6 100644
--- a/clients/openframe-client/src/lib.rs
+++ b/clients/openframe-client/src/lib.rs
@@ -762,8 +762,12 @@ impl Client {
         self.script_schedule_execution_listener.start().await?;
         info!("Script schedule execution listener started");
 
-        // Start tool run manager
-        self.tool_run_manager.run().await?;
+        // Start tool run manager. A tool lane that cannot start must not end the service
+        // core — the agent still has to heartbeat, stream logs and accept remote commands,
+        // which is how an operator repairs that tool in the first place.
+        if let Err(e) = self.tool_run_manager.run().await {
+            error!("Failed to start tool run manager: {:#}", e);
+        }
 
         // Start mesh self-heal watcher (re-fetch .msh + bounce agent if held on a stale MeshID).
         self.mesh_self_heal_service.run().await?;
diff --git a/clients/openframe-client/src/platform/preferences_writer.rs b/clients/openframe-client/src/platform/preferences_writer.rs
index e35d221274..bc06605586 100644
--- a/clients/openframe-client/src/platform/preferences_writer.rs
+++ b/clients/openframe-client/src/platform/preferences_writer.rs
@@ -19,8 +19,16 @@ pub fn write<'a>(
     }
 
     for (key, value) in &prefs {
-        let status = Command::new("sudo")
+        // `launchctl asuser` first, `sudo -u` only as a fallback — the same order
+        // `user_session::launch_as_user` uses. From a LaunchDaemon there is no user
+        // session bootstrap, so a bare `sudo -u defaults write` is rejected by cfprefsd
+        // ("Could not write domain ...; exiting") and the app then launches with none of
+        // its configuration, because preferences are the only channel GuiApp args travel.
+        let status = Command::new("launchctl")
             .args([
+                "asuser",
+                &user.uid.to_string(),
+                "sudo",
                 "-u",
                 &user.username,
                 "defaults",
@@ -32,8 +40,25 @@ pub fn write<'a>(
             .status()
             .with_context(|| format!("Failed to write preference '{}'", key))?;
 
-        if !status.success() {
-            anyhow::bail!("defaults write failed for '{}': exit {}", key, status);
+        if status.success() {
+            continue;
+        }
+
+        let fallback = Command::new("sudo")
+            .args([
+                "-u",
+                &user.username,
+                "defaults",
+                "write",
+                bundle_id,
+                key,
+                value,
+            ])
+            .status()
+            .with_context(|| format!("Failed to write preference '{}'", key))?;
+
+        if !fallback.success() {
+            anyhow::bail!("defaults write failed for '{}': {}", key, fallback);
         }
     }
 
diff --git a/clients/openframe-client/src/platform/tool_updater/gui_app.rs b/clients/openframe-client/src/platform/tool_updater/gui_app.rs
index 1d44999ee4..d62f9fc6dc 100644
--- a/clients/openframe-client/src/platform/tool_updater/gui_app.rs
+++ b/clients/openframe-client/src/platform/tool_updater/gui_app.rs
@@ -1,13 +1,13 @@
 use anyhow::{Context, Result};
 use async_trait::async_trait;
-use std::path::PathBuf;
+use std::path::{Path, PathBuf};
 use tracing::{error, info, warn};
 
 use super::{ToolUpdater, ToolUpdaterDeps, UpdateContext};
 use crate::models::{DownloadConfiguration, Installation, InstalledTool};
 use crate::platform::preferences_writer::{args_to_pairs, write as write_preferences};
-use crate::platform::remove_app_bundle;
 use crate::platform::user_session::{get_console_user, launch_as_user};
+use crate::platform::DirectoryManager;
 
 pub struct GuiAppToolUpdater {
     deps: ToolUpdaterDeps,
@@ -17,6 +17,42 @@ impl GuiAppToolUpdater {
     pub fn new(deps: ToolUpdaterDeps) -> Self {
         Self { deps }
     }
+
+    fn backup_path_for(bundle: &Path) -> PathBuf {
+        bundle.with_extension("app.update-backup")
+    }
+
+    /// Renames the installed `.app` to a sibling backup so a failed download can be undone.
+    /// The old code deleted the bundle outright and then downloaded its replacement, so a
+    /// dropped connection left no app at all and a rollback that could only log
+    /// "reinstall required". A rename is atomic, same-volume, and costs nothing.
+    async fn move_bundle_aside(
+        executable_path: &str,
+        tool_agent_id: &str,
+    ) -> Result<Option<PathBuf>> {
+        let Some(bundle) = DirectoryManager::find_app_bundle_path(Path::new(executable_path))
+        else {
+            warn!(tool_id = %tool_agent_id, "No .app bundle in {} — updating without a backup", executable_path);
+            return Ok(None);
+        };
+        if !bundle.exists() {
+            return Ok(None);
+        }
+
+        let backup = Self::backup_path_for(&bundle);
+        if backup.exists() {
+            tokio::fs::remove_dir_all(&backup).await.ok();
+        }
+        tokio::fs::rename(&bundle, &backup).await.with_context(|| {
+            format!(
+                "Failed to move {} aside to {}",
+                bundle.display(),
+                backup.display()
+            )
+        })?;
+        info!(tool_id = %tool_agent_id, "Old app bundle kept at {}", backup.display());
+        Ok(Some(backup))
+    }
 }
 
 #[async_trait]
@@ -34,8 +70,15 @@ impl ToolUpdater for GuiAppToolUpdater {
 
         tokio::time::sleep(tokio::time::Duration::from_secs(2)).await;
 
+        let backup_path = match &tool.installation {
+            Installation::GuiApp {
+                executable_path, ..
+            } => Self::move_bundle_aside(executable_path, tool_agent_id).await?,
+            _ => None,
+        };
+
         Ok(UpdateContext {
-            backup_path: None,
+            backup_path,
             needs_restart: true,
         })
     }
@@ -49,20 +92,15 @@ impl ToolUpdater for GuiAppToolUpdater {
         let tool_agent_id = &tool.tool_agent_id;
         info!(tool_id = %tool_agent_id, "Applying GuiApp update");
 
-        let Installation::GuiApp {
-            executable_path,
-            bundle_id,
-        } = &tool.installation
-        else {
+        let Installation::GuiApp { bundle_id, .. } = &tool.installation else {
             anyhow::bail!(
                 "Expected GuiApp installation type for tool: {}",
                 tool_agent_id
             );
         };
 
-        info!(tool_id = %tool_agent_id, "Removing old app bundle");
-        remove_app_bundle(executable_path).await?;
-
+        // The old bundle was moved aside in `prepare`, so the destination is already free
+        // and `rollback` can put it back if this download fails.
         let applications_dir = PathBuf::from("/Applications");
 
         info!(tool_id = %tool_agent_id, "Downloading and installing new version from: {}", config.link);
@@ -93,6 +131,17 @@ impl ToolUpdater for GuiAppToolUpdater {
         let tool_agent_id = &tool.tool_agent_id;
         info!(tool_id = %tool_agent_id, "Finalizing GuiApp update");
 
+        // `finalize` only runs after a successful apply, so the kept-aside bundle has done
+        // its job. Drop it here rather than after the relaunch, so the early return below
+        // cannot leak a full copy of the app into /Applications.
+        if let Some(backup) = &ctx.backup_path {
+            if backup.exists() {
+                if let Err(e) = tokio::fs::remove_dir_all(backup).await {
+                    warn!(tool_id = %tool_agent_id, "Failed to remove the old app bundle {}: {:#}", backup.display(), e);
+                }
+            }
+        }
+
         if !ctx.needs_restart {
             info!(tool_id = %tool_agent_id, "Restart not requested, skipping");
             return Ok(());
@@ -154,10 +203,38 @@ impl ToolUpdater for GuiAppToolUpdater {
         Ok(())
     }
 
-    async fn rollback(&self, tool: &InstalledTool, _ctx: &UpdateContext) -> Result<()> {
+    async fn rollback(&self, tool: &InstalledTool, ctx: &UpdateContext) -> Result<()> {
         let tool_agent_id = &tool.tool_agent_id;
-        warn!(tool_id = %tool_agent_id,
-              "Rollback requested for GuiApp but no backup available. User should reinstall from server.");
+
+        let Some(backup) = &ctx.backup_path else {
+            warn!(tool_id = %tool_agent_id,
+                  "Rollback requested for GuiApp but no backup was taken. User should reinstall from server.");
+            return Ok(());
+        };
+
+        let Installation::GuiApp {
+            executable_path, ..
+        } = &tool.installation
+        else {
+            anyhow::bail!("Expected GuiApp installation type for tool: {tool_agent_id}");
+        };
+        let Some(bundle) = DirectoryManager::find_app_bundle_path(Path::new(executable_path))
+        else {
+            anyhow::bail!("Could not resolve the .app bundle path for {tool_agent_id}");
+        };
+
+        // A partially extracted new bundle must not block the restore.
+        if bundle.exists() {
+            tokio::fs::remove_dir_all(&bundle).await.ok();
+        }
+        tokio::fs::rename(backup, &bundle).await.with_context(|| {
+            format!(
+                "Failed to restore the previous app bundle from {}",
+                backup.display()
+            )
+        })?;
+
+        info!(tool_id = %tool_agent_id, "Restored the previous app bundle: {}", bundle.display());
         Ok(())
     }
 }
diff --git a/clients/openframe-client/src/platform/tool_updater/standard.rs b/clients/openframe-client/src/platform/tool_updater/standard.rs
index ae48522047..e653182045 100644
--- a/clients/openframe-client/src/platform/tool_updater/standard.rs
+++ b/clients/openframe-client/src/platform/tool_updater/standard.rs
@@ -16,8 +16,53 @@ impl StandardToolUpdater {
     pub fn new(deps: ToolUpdaterDeps) -> Self {
         Self { deps }
     }
+
+    /// Honour the executable path the installer recorded. A folder-extraction config
+    /// installs to e.g. `<tool>/bin/tool`, not `<tool>/agent` — writing to the latter
+    /// regardless left the tool running its old binary while the record, and the backend,
+    /// reported the new version. `ServiceToolUpdater::resolve_executable_path` already
+    /// does this; Standard was the outlier.
+    fn resolve_executable_path(&self, tool: &InstalledTool) -> std::path::PathBuf {
+        let agent_path = self
+            .deps
+            .directory_manager
+            .get_agent_path(&tool.tool_agent_id);
+
+        let recorded = match &tool.installation {
+            Installation::Standard {
+                executable_path: Some(exec_path),
+            } => Some(exec_path.as_str()),
+            _ => None,
+        };
+
+        resolve_recorded_executable(agent_path, recorded)
+    }
 }
 
+/// Resolve a recorded executable path against the default `<tool>/agent` path, mirroring
+/// `DirectoryManager::get_tool_executable_path`. Absolute paths (unix `/…`, Windows `C:\…`)
+/// are taken as-is; a relative path is joined onto the tool's own directory.
+///
+/// Split out from the method above purely so it can be tested without constructing a whole
+/// `ToolUpdaterDeps` — it is the part that decides which file an update overwrites, and
+/// getting it wrong silently leaves the tool running its old binary.
+fn resolve_recorded_executable(
+    agent_path: std::path::PathBuf,
+    recorded: Option<&str>,
+) -> std::path::PathBuf {
+    match recorded {
+        Some(exec_path) if exec_path.starts_with('/') || exec_path.contains(':') => {
+            std::path::PathBuf::from(exec_path)
+        }
+        Some(exec_path) => agent_path.parent().unwrap_or(&agent_path).join(exec_path),
+        None => agent_path,
+    }
+}
+
+#[cfg(test)]
+#[path = "standard_tests.rs"]
+mod tests;
+
 #[async_trait]
 impl ToolUpdater for StandardToolUpdater {
     async fn prepare(&self, tool: &InstalledTool) -> Result<UpdateContext> {
@@ -31,7 +76,7 @@ impl ToolUpdater for StandardToolUpdater {
             .await
             .with_context(|| format!("Failed to stop tool: {}", tool_agent_id))?;
 
-        let agent_path = self.deps.directory_manager.get_agent_path(tool_agent_id);
+        let agent_path = self.resolve_executable_path(tool);
         clear_aside_binary(&agent_path, tool_agent_id).await;
         log_update_survivors(&self.deps, tool).await;
 
@@ -52,7 +97,7 @@ impl ToolUpdater for StandardToolUpdater {
         let tool_agent_id = &tool.tool_agent_id;
         info!(tool_id = %tool_agent_id, "Applying Standard tool update");
 
-        let agent_path = self.deps.directory_manager.get_agent_path(tool_agent_id);
+        let agent_path = self.resolve_executable_path(tool);
         download_and_write_binary(&self.deps, config, &agent_path, tool_agent_id).await?;
         Ok(None)
     }
@@ -74,7 +119,7 @@ impl ToolUpdater for StandardToolUpdater {
         let tool_agent_id = &tool.tool_agent_id;
         info!(tool_id = %tool_agent_id, "Rolling back Standard tool update");
 
-        let agent_path = self.deps.directory_manager.get_agent_path(tool_agent_id);
+        let agent_path = self.resolve_executable_path(tool);
         restore_from_backup(ctx.backup_path.as_ref(), &agent_path, tool_agent_id).await
     }
 }
diff --git a/clients/openframe-client/src/platform/tool_updater/standard_tests.rs b/clients/openframe-client/src/platform/tool_updater/standard_tests.rs
new file mode 100644
index 0000000000..1fcb67a7d2
--- /dev/null
+++ b/clients/openframe-client/src/platform/tool_updater/standard_tests.rs
@@ -0,0 +1,63 @@
+use super::resolve_recorded_executable;
+use std::path::PathBuf;
+
+fn agent_path() -> PathBuf {
+    PathBuf::from("/Library/Application Support/OpenFrame/fleetmdm-agent/agent")
+}
+
+#[test]
+fn no_recorded_path_falls_back_to_the_default_agent_path() {
+    assert_eq!(
+        resolve_recorded_executable(agent_path(), None),
+        agent_path()
+    );
+}
+
+/// The case the fix exists for: a folder-extraction config installs to `<tool>/bin/orbit`,
+/// so writing to `<tool>/agent` would leave the tool running its old binary while the
+/// record — and the backend — reported the new version.
+#[test]
+fn relative_recorded_path_is_joined_onto_the_tool_directory() {
+    assert_eq!(
+        resolve_recorded_executable(agent_path(), Some("bin/orbit")),
+        PathBuf::from("/Library/Application Support/OpenFrame/fleetmdm-agent/bin/orbit")
+    );
+}
+
+#[test]
+fn absolute_unix_recorded_path_is_taken_as_is() {
+    assert_eq!(
+        resolve_recorded_executable(agent_path(), Some("/opt/orbit/bin/orbit")),
+        PathBuf::from("/opt/orbit/bin/orbit")
+    );
+}
+
+/// Windows absolute paths are detected by the drive colon, not a leading slash.
+#[test]
+fn windows_absolute_recorded_path_is_taken_as_is() {
+    assert_eq!(
+        resolve_recorded_executable(
+            PathBuf::from("C:\\ProgramData\\OpenFrame\\fleetmdm-agent\\agent.exe"),
+            Some("C:\\Program Files\\Orbit\\orbit.exe")
+        ),
+        PathBuf::from("C:\\Program Files\\Orbit\\orbit.exe")
+    );
+}
+
+/// A bare filename still lands beside the default agent binary rather than replacing it.
+#[test]
+fn bare_filename_resolves_next_to_the_agent_binary() {
+    assert_eq!(
+        resolve_recorded_executable(agent_path(), Some("orbit")),
+        PathBuf::from("/Library/Application Support/OpenFrame/fleetmdm-agent/orbit")
+    );
+}
+
+/// Equivalent to the default: the resolver must not disturb the common single-binary case.
+#[test]
+fn recorded_path_equal_to_the_default_resolves_to_the_default() {
+    assert_eq!(
+        resolve_recorded_executable(agent_path(), Some("agent")),
+        agent_path()
+    );
+}
diff --git a/clients/openframe-client/src/service.rs b/clients/openframe-client/src/service.rs
index cfd5f8b93c..db97c49082 100644
--- a/clients/openframe-client/src/service.rs
+++ b/clients/openframe-client/src/service.rs
@@ -83,14 +83,22 @@ fn windows_service_main(_args: Vec<std::ffi::OsString>) {
     };
 
     // Report that the service is running
-    let _ = set_service_status(&status_handle, ServiceState::Running);
+    let _ = set_service_status(
+        &status_handle,
+        ServiceState::Running,
+        ServiceExitCode::Win32(0),
+    );
 
     // Create a Tokio runtime and run the service core
     let rt = match Runtime::new() {
         Ok(runtime) => runtime,
         Err(e) => {
             eprintln!("Failed to create Tokio runtime: {:?}", e);
-            let _ = set_service_status(&status_handle, ServiceState::Stopped);
+            let _ = set_service_status(
+                &status_handle,
+                ServiceState::Stopped,
+                ServiceExitCode::ServiceSpecific(1),
+            );
             return;
         }
     };
@@ -113,18 +121,36 @@ fn windows_service_main(_args: Vec<std::ffi::OsString>) {
         }
     });
 
+    // Report a non-zero exit code so the SCM treats this as a failure. The recovery
+    // ladder installed at register time (10s/60s/300s) plus
+    // `set_failure_actions_on_non_crash_failures` only fire when SERVICE_STOPPED
+    // carries a non-zero code; reporting Win32(0) on both paths made a dead core
+    // indistinguishable from an operator-requested stop, so nothing ever restarted it.
+    // The reason goes through tracing, not stderr, or it never reaches openframe.log.
     if let Err(e) = result {
-        eprintln!("Service core failed: {:?}", e);
-        let _ = set_service_status(&status_handle, ServiceState::Stopped);
+        error!("Service core failed: {:#}", e);
+        let _ = set_service_status(
+            &status_handle,
+            ServiceState::Stopped,
+            ServiceExitCode::ServiceSpecific(1),
+        );
     } else {
         info!("Service stopped gracefully");
-        let _ = set_service_status(&status_handle, ServiceState::Stopped);
+        let _ = set_service_status(
+            &status_handle,
+            ServiceState::Stopped,
+            ServiceExitCode::Win32(0),
+        );
     }
 }
 
 /// Helper function to set service status
 #[cfg(windows)]
-fn set_service_status(status_handle: &ServiceStatusHandle, state: ServiceState) -> Result<()> {
+fn set_service_status(
+    status_handle: &ServiceStatusHandle,
+    state: ServiceState,
+    exit_code: ServiceExitCode,
+) -> Result<()> {
     let status = ServiceStatus {
         service_type: ServiceType::OWN_PROCESS,
         current_state: state,
@@ -133,7 +159,7 @@ fn set_service_status(status_handle: &ServiceStatusHandle, state: ServiceState)
         } else {
             ServiceControlAccept::empty()
         },
-        exit_code: ServiceExitCode::Win32(0),
+        exit_code,
         checkpoint: 0,
         wait_hint: std::time::Duration::from_secs(5),
         process_id: None,
diff --git a/clients/openframe-client/src/services/github_download_service.rs b/clients/openframe-client/src/services/github_download_service.rs
index 7b8b09881b..6b58e4e1fd 100644
--- a/clients/openframe-client/src/services/github_download_service.rs
+++ b/clients/openframe-client/src/services/github_download_service.rs
@@ -8,9 +8,41 @@ use bytes::Bytes;
 use reqwest::Client;
 use std::io::Cursor;
 use std::path::Path;
+#[cfg(target_os = "macos")]
+use std::path::{Component, PathBuf};
 use tokio::time::Duration;
 use tracing::{info, warn};
 
+/// Join an archive entry path under `target_dir`, refusing anything that could escape it.
+/// `Path::join` with an absolute entry *replaces* the base, and `..` is resolved by the OS,
+/// so an unsanitised entry in a downloaded archive writes anywhere on disk — as root, and
+/// with the archive's own mode bits. `tar::Archive::unpack` guards this; this hand-rolled
+/// loop has to do it itself.
+///
+/// Gated to macOS alongside its only caller, `extract_all_from_tar_gz`: on other platforms
+/// `download_and_extract_all` refuses outright, so an ungated helper here is dead code and
+/// `clippy -D warnings` rejects it.
+#[cfg(target_os = "macos")]
+fn safe_join(target_dir: &Path, entry_path: &Path) -> Result<PathBuf> {
+    for component in entry_path.components() {
+        match component {
+            Component::Normal(_) | Component::CurDir => {}
+            _ => {
+                return Err(anyhow!(
+                    "Refusing archive entry with an unsafe path: {}",
+                    entry_path.display()
+                ))
+            }
+        }
+    }
+    Ok(target_dir.join(entry_path))
+}
+
+// macOS-gated alongside `safe_join` itself.
+#[cfg(all(test, target_os = "macos"))]
+#[path = "github_download_service_tests.rs"]
+mod tests;
+
 #[derive(Clone)]
 pub struct GithubDownloadService {
     http_client: Client,
@@ -409,7 +441,7 @@ impl GithubDownloadService {
         for entry_result in archive.entries().context("Failed to read tar entries")? {
             let mut entry = entry_result.context("Failed to read tar entry")?;
             let path = entry.path().context("Failed to get entry path")?;
-            let dest_path = target_dir.join(&path);
+            let dest_path = safe_join(target_dir, &path)?;
 
             if entry.header().entry_type().is_dir() {
                 fs::create_dir_all(&dest_path).with_context(|| {
diff --git a/clients/openframe-client/src/services/github_download_service_tests.rs b/clients/openframe-client/src/services/github_download_service_tests.rs
new file mode 100644
index 0000000000..d91ede58d5
--- /dev/null
+++ b/clients/openframe-client/src/services/github_download_service_tests.rs
@@ -0,0 +1,73 @@
+use super::safe_join;
+use std::path::{Path, PathBuf};
+
+fn target() -> PathBuf {
+    PathBuf::from("/Applications")
+}
+
+fn join(entry: &str) -> Option<PathBuf> {
+    safe_join(&target(), Path::new(entry)).ok()
+}
+
+// ---------------------------------------------------------------- accepted
+
+/// tar archives routinely prefix entries with `./`; rejecting those would break every
+/// legitimate download.
+#[test]
+fn accepts_leading_current_dir() {
+    assert_eq!(
+        join("./OpenFrame.app/Contents/MacOS/openframe-chat"),
+        Some(target().join("./OpenFrame.app/Contents/MacOS/openframe-chat"))
+    );
+}
+
+#[test]
+fn accepts_a_plain_nested_entry() {
+    assert_eq!(
+        join("OpenFrame.app/Contents/Info.plist"),
+        Some(target().join("OpenFrame.app/Contents/Info.plist"))
+    );
+}
+
+#[test]
+fn accepts_a_bare_filename() {
+    assert_eq!(
+        join("pax_global_header"),
+        Some(target().join("pax_global_header"))
+    );
+}
+
+// ---------------------------------------------------------------- rejected
+
+/// The reason this helper exists: `Path::join` with an absolute entry *replaces* the base,
+/// so an unsanitised archive entry would write outside the target — as root.
+#[test]
+fn rejects_an_absolute_entry() {
+    assert!(join("/Library/LaunchDaemons/evil.plist").is_none());
+}
+
+#[test]
+fn rejects_parent_traversal() {
+    assert!(join("../../../../etc/cron.d/evil").is_none());
+}
+
+/// `..` must be rejected wherever it appears, not just at the start — `components()`
+/// normalises `.` but never `..`, so an interior segment still escapes.
+#[test]
+fn rejects_interior_parent_traversal() {
+    assert!(join("OpenFrame.app/../../../etc/passwd").is_none());
+}
+
+#[test]
+fn rejects_a_root_relative_entry() {
+    assert!(join("//srv/evil").is_none());
+}
+
+/// Every rejected entry must fail loudly rather than resolve to something surprising.
+#[test]
+fn rejection_names_the_offending_entry() {
+    let err = safe_join(&target(), Path::new("../escape"))
+        .expect_err("traversal must be refused")
+        .to_string();
+    assert!(err.contains("../escape"), "unhelpful error: {err}");
+}
diff --git a/clients/openframe-client/src/services/initial_configuration_service.rs b/clients/openframe-client/src/services/initial_configuration_service.rs
index 77ec07d0a9..2a1f17d895 100644
--- a/clients/openframe-client/src/services/initial_configuration_service.rs
+++ b/clients/openframe-client/src/services/initial_configuration_service.rs
@@ -107,7 +107,11 @@ impl InitialConfigurationService {
     pub fn save(&self, config: &InitialConfiguration) -> Result<()> {
         let config_json = serde_json::to_string_pretty(config)
             .context("Failed to serialize initial configuration to JSON")?;
-        fs::write(&self.config_file_path, config_json).with_context(|| {
+        // Atomic: a torn write here is unrecoverable in software — `is_configured()` goes
+        // false, the service parks in the awaiting-auth gate, and NATS never connects, so
+        // the machine cannot be repaired remotely. `agent_config.json` next door already
+        // writes this way.
+        crate::utils::fs::atomic_write(&self.config_file_path, config_json).with_context(|| {
             format!(
                 "Failed to write initial configuration file: {:?}",
                 self.config_file_path
diff --git a/clients/openframe-client/src/services/last_known_good_service.rs b/clients/openframe-client/src/services/last_known_good_service.rs
index c5b409a93b..073af366a3 100644
--- a/clients/openframe-client/src/services/last_known_good_service.rs
+++ b/clients/openframe-client/src/services/last_known_good_service.rs
@@ -133,12 +133,21 @@ impl LastKnownGoodService {
                 );
                 self.promote(running_version).await
             }
+            // Any other mismatch: the running binary is NOT the anchored one, so it must
+            // not become the reserve. This arm used to seed it anyway — its message said
+            // "running below anchor" but nothing checked the direction, so the common case
+            // (running *newer* than the anchor, i.e. an update whose verification has not
+            // promoted yet) copied an unverified binary into the reserve while the anchor
+            // still named the old version. A rollback then preferred that reserve, restored
+            // the very binary that had just failed, and deleted the good backup behind it.
+            // No reserve is strictly safer than a wrong one: the pre-swap backup still
+            // covers rollback, and the next verified update re-promotes a correct reserve.
             Some(anchor_version) => {
                 warn!(
-                    "Rollback protection degraded: reserve missing, running {} below anchor {} — rebuilding reserve from running binary, anchor unchanged",
+                    "Rollback protection degraded: reserve missing and running {} does not match anchor {} — leaving the reserve unset for a verified update to rebuild",
                     running_version, anchor_version
                 );
-                self.copy_running_to_reserve()
+                Ok(())
             }
             None => {
                 info!(
diff --git a/clients/openframe-client/src/services/tool_agent_update_service.rs b/clients/openframe-client/src/services/tool_agent_update_service.rs
index 4eb08631f2..423e9ff2ad 100644
--- a/clients/openframe-client/src/services/tool_agent_update_service.rs
+++ b/clients/openframe-client/src/services/tool_agent_update_service.rs
@@ -3,10 +3,10 @@ use crate::models::tool_agent_update_message::{AssetUpdate, ToolAgentUpdateMessa
 use crate::models::{Installation, InstalledAsset, ToolRecordState};
 use crate::platform::{
     binary_writer, clear_aside_binary, detect_actual_installation, needs_migration, run_migration,
-    run_update, DirectoryManager, ToolUpdaterDeps,
+    run_update, system_service, DirectoryManager, ToolUpdaterDeps,
 };
 use crate::services::agent_configuration_service::AgentConfigurationService;
-use crate::services::tool_run_manager::ToolRunManager;
+use crate::services::tool_run_manager::{ToolRunManager, UpdatingGuard};
 use crate::services::GithubDownloadService;
 use crate::services::InstalledAgentMessagePublisher;
 use crate::services::InstalledToolsService;
@@ -152,8 +152,32 @@ impl ToolAgentUpdateService {
             return Ok(());
         }
 
-        // Mark as updating once for all updates
-        self.tool_run_manager.mark_updating(tool_agent_id).await;
+        // Serialise against install/reinstall/uninstall/restart, which all take this same
+        // lock. Updates arrive on a different NATS stream, so without it an uninstall can
+        // delete the registry record while this update holds a stale copy and then writes
+        // it back — resurrecting a record for a tool whose binary is gone. Deferring (like
+        // uninstall and restart do) rather than blocking keeps a slow neighbour from
+        // pushing this past ack_wait into a duplicate delivery.
+        let tool_lock = self.tool_run_manager.tool_lock(tool_agent_id).await;
+        let lock_guard = match tool_lock.try_lock_owned() {
+            Ok(guard) => guard,
+            Err(_) => {
+                info!(
+                    "Tool {} is busy with another operation, deferring update for redelivery",
+                    tool_agent_id
+                );
+                anyhow::bail!(
+                    "tool {} busy with another operation, deferring update for redelivery",
+                    tool_agent_id
+                );
+            }
+        };
+
+        // Mark as updating once for all updates; the guard clears the flag — and only then
+        // releases the lock — on return, panic or cancellation. A leaked flag parks the
+        // tool's supervisor and blocks every client self-update for the process lifetime.
+        let _updating =
+            UpdatingGuard::acquire(&self.tool_run_manager, tool_agent_id, Some(lock_guard)).await;
 
         // A Standard->GuiApp migration self-relaunches, so only relaunch here if it was already a GUI app.
         let was_gui_before_update =
@@ -168,9 +192,6 @@ impl ToolAgentUpdateService {
             )
             .await;
 
-        // Clear updating flag - for Standard tools the run manager relaunches them via this flag.
-        self.tool_run_manager.clear_updating(tool_agent_id).await;
-
         if result.is_ok() && needs_repair {
             if let Err(e) = self
                 .installed_tools_service
@@ -210,24 +231,74 @@ impl ToolAgentUpdateService {
             );
             self.do_tool_update(new_version, message, installed_tool)
                 .await?;
-        } else if !assets_to_update.is_empty() {
-            // Only assets to update - stop tool once before all asset updates
+        }
+
+        let mut stopped_for_assets = false;
+        if !needs_tool_update && !assets_to_update.is_empty() {
+            // Only assets to update - stop tool once before all asset updates.
+            // `stop_installed_tool`, not `stop_tool`: for Installation::Service this stops
+            // the OS service itself. A process-pattern kill leaves the launchd job loaded,
+            // and `launchctl load` on an already-loaded job is a no-op that still reports
+            // success — so the restart below would silently start nothing on macOS.
             info!(tool_id = %tool_agent_id, "Stopping tool for asset updates");
             self.tool_kill_service
-                .stop_tool(tool_agent_id)
+                .stop_installed_tool(installed_tool, false)
                 .await
                 .with_context(|| {
                     format!("Failed to stop tool {} for asset updates", tool_agent_id)
                 })?;
+            stopped_for_assets = true;
         }
 
-        // 2. Asset updates (tool already stopped by tool_update or above)
+        // 2. Asset updates (tool already stopped by tool_update or above).
+        // The result is held rather than propagated so the restart below still runs: a tool
+        // we stopped must be started again even when an asset failed halfway.
+        let mut asset_result = Ok(());
         for asset in assets_to_update {
-            self.do_asset_update(tool_agent_id, asset, installed_tool)
-                .await?;
+            asset_result = self
+                .do_asset_update(tool_agent_id, asset, installed_tool)
+                .await;
+            if asset_result.is_err() {
+                break;
+            }
         }
 
-        Ok(())
+        // A Service install is not supervised by the run manager — `run_tool` returns early
+        // for it and `run()` never starts services — so an asset-only update that stopped
+        // the tool has to start it again. Without this a routine asset bump (e.g. osqueryd)
+        // silently takes the agent down until a reboot or a reinstall. Supervised installs
+        // are relaunched by their run loop once the updating flag clears.
+        if stopped_for_assets {
+            if let Installation::Service { service_name, .. } = &installed_tool.installation {
+                self.restart_service_after_assets(tool_agent_id, service_name)
+                    .await;
+            }
+        }
+
+        asset_result
+    }
+
+    /// Start a Service tool back up after an asset-only update.
+    ///
+    /// Retry and start-verification deliberately live in `system_service::start_service`
+    /// rather than here, so there is one implementation of "starting a service is flaky"
+    /// (see #2230, which gives it a backoff schedule and classifies non-retryable codes).
+    ///
+    /// Returns nothing instead of an error on purpose: every asset version was already
+    /// persisted and published by `do_asset_update`, so a redelivery finds nothing left to
+    /// update and acks immediately — propagating an error here would look like a retry
+    /// while actually discarding the message with the tool still stopped. A loud log is the
+    /// honest signal; the alternative is a failure that silently disappears.
+    async fn restart_service_after_assets(&self, tool_agent_id: &str, service_name: &str) {
+        info!(tool_id = %tool_agent_id, service = %service_name, "Restarting service tool after asset updates");
+
+        if let Err(e) = system_service::start_service(service_name).await {
+            error!(
+                tool_id = %tool_agent_id, service = %service_name,
+                "Service tool could not be restarted after asset updates - it will stay down until a reinstall or reboot: {:#}",
+                e
+            );
+        }
     }
 
     async fn do_tool_update(
diff --git a/clients/openframe-client/src/services/tool_restart_service.rs b/clients/openframe-client/src/services/tool_restart_service.rs
index c3f8809377..2dafa7791d 100644
--- a/clients/openframe-client/src/services/tool_restart_service.rs
+++ b/clients/openframe-client/src/services/tool_restart_service.rs
@@ -1,7 +1,7 @@
 use crate::config::service_stop::TOOL_RESTART_TIMEOUT_SECS;
 use crate::models::{Installation, InstalledTool};
 use crate::platform::system_service;
-use crate::services::tool_run_manager::ToolRunManager;
+use crate::services::tool_run_manager::{ToolRunManager, UpdatingGuard};
 use crate::services::InstalledToolsService;
 use crate::services::ToolKillService;
 use anyhow::{Context, Result};
@@ -16,27 +16,6 @@ pub enum RestartOutcome {
     Busy,
 }
 
-/// Clears the updating flag on drop (surviving cancellation and panic), releasing the tool lock only after the flag clears.
-struct UpdatingGuard {
-    tool_run_manager: ToolRunManager,
-    tool_agent_id: String,
-    lock_guard: Option<tokio::sync::OwnedMutexGuard<()>>,
-}
-
-impl Drop for UpdatingGuard {
-    fn drop(&mut self) {
-        if let Ok(handle) = tokio::runtime::Handle::try_current() {
-            let manager = self.tool_run_manager.clone();
-            let tool_agent_id = self.tool_agent_id.clone();
-            let lock_guard = self.lock_guard.take();
-            handle.spawn(async move {
-                manager.clear_updating(&tool_agent_id).await;
-                drop(lock_guard);
-            });
-        }
-    }
-}
-
 #[derive(Clone)]
 pub struct ToolRestartService {
     installed_tools_service: InstalledToolsService,
@@ -64,12 +43,8 @@ impl ToolRestartService {
             Ok(guard) => guard,
             Err(_) => return Ok(RestartOutcome::Busy),
         };
-        self.tool_run_manager.mark_updating(tool_agent_id).await;
-        let _updating = UpdatingGuard {
-            tool_run_manager: self.tool_run_manager.clone(),
-            tool_agent_id: tool_agent_id.to_string(),
-            lock_guard: Some(lock_guard),
-        };
+        let _updating =
+            UpdatingGuard::acquire(&self.tool_run_manager, tool_agent_id, Some(lock_guard)).await;
         // Hard cap so a wedged OS call can't hold the flag/lock forever and freeze callers (e.g. mesh self-heal).
         let outcome = tokio::time::timeout(
             Duration::from_secs(TOOL_RESTART_TIMEOUT_SECS),
diff --git a/clients/openframe-client/src/services/tool_run_manager.rs b/clients/openframe-client/src/services/tool_run_manager.rs
index db4ad4f398..bfe44178ec 100644
--- a/clients/openframe-client/src/services/tool_run_manager.rs
+++ b/clients/openframe-client/src/services/tool_run_manager.rs
@@ -29,6 +29,8 @@ use windows::{
 };
 
 const RETRY_DELAY_SECONDS: u64 = 5;
+/// Ceiling for the escalating retry delay after a leftover process refuses to die.
+const KILL_RETRY_MAX_DELAY_SECONDS: u64 = 300;
 
 /// Under the supervision lock, so a concurrent resume either finds the loop alive or relaunches its tool.
 async fn shutdown_break(
@@ -664,11 +666,18 @@ impl ToolRunManager {
         }
 
         for tool in tools {
-            if self.try_mark_running(&tool.tool_agent_id).await {
-                info!("Running tool {}", tool.tool_agent_id);
-                self.run_tool(tool, false).await?;
+            let tool_id = tool.tool_agent_id.clone();
+            if self.try_mark_running(&tool_id).await {
+                info!("Running tool {}", tool_id);
+                // One tool must never abort the startup sequence: this error reached
+                // Client::start() through `?` and ended the service core, so a single
+                // tool that could not be launched took the whole agent down with it.
+                if let Err(e) = self.run_tool(tool, false).await {
+                    warn!(tool_id = %tool_id, "Failed to start tool during startup: {:#}", e);
+                    self.clear_running_tool(&tool_id).await;
+                }
             } else {
-                warn!("Tool {} is already running - skipping", tool.tool_agent_id);
+                warn!("Tool {} is already running - skipping", tool_id);
             }
         }
 
@@ -685,7 +694,14 @@ impl ToolRunManager {
         }
 
         info!("Running new single tool {}", installed_tool.tool_agent_id);
-        self.run_tool(installed_tool, true).await
+        let tool_id = installed_tool.tool_agent_id.clone();
+        // Release the mark taken above if supervision never started, or the tool stays
+        // marked running forever and every later attempt skips it as already running.
+        let result = self.run_tool(installed_tool, true).await;
+        if result.is_err() {
+            self.clear_running_tool(&tool_id).await;
+        }
+        result
     }
 
     async fn try_mark_running(&self, tool_id: &str) -> bool {
@@ -714,24 +730,12 @@ impl ToolRunManager {
             return Ok(());
         }
 
-        #[cfg(not(target_os = "windows"))]
-        self.tool_kill_service
-            .stop_tool(&tool.tool_agent_id)
-            .await?;
-
-        // Windows GUI apps are owned by the HKLM Run autorun, not us — never kill them.
-        #[cfg(target_os = "windows")]
-        if !tool.installation.is_gui_app() {
-            self.tool_kill_service
-                .stop_tool(&tool.tool_agent_id)
-                .await?;
-        }
-
         let updating_tools = self.updating_tools.clone();
         let shutting_down = self.shutting_down.clone();
         let params_processor = self.params_processor.clone();
         let running_tools = self.running_tools.clone();
         let installed_tools_service = self.installed_tools_service.clone();
+        let tool_kill_service = self.tool_kill_service.clone();
         let mut installation = tool.installation.clone();
         let mut run_command_args = tool.run_command_args.clone();
 
@@ -768,6 +772,37 @@ impl ToolRunManager {
 
                 let log_attempt = launch_backoff.should_log();
 
+                // Leftovers from a previous run have to be gone before we launch, or two
+                // instances fight over the same state. The kill belongs inside the loop:
+                // a process that will not die is a retryable launch failure like any other
+                // — run once ahead of the loop, its error propagated out of Client::start()
+                // and ended the service core over one stuck tool, and skipping the launch
+                // instead would leave the tool unsupervised until someone reinstalled it.
+                // Windows GUI apps are owned by the HKLM Run autorun, not us — never kill them.
+                #[cfg(target_os = "windows")]
+                let kill_leftovers = !installation.is_gui_app();
+                #[cfg(not(target_os = "windows"))]
+                let kill_leftovers = true;
+
+                if kill_leftovers {
+                    if let Err(e) = tool_kill_service.stop_tool(&tool.tool_agent_id).await {
+                        let failures = launch_backoff.record_failure(log_attempt);
+                        // Escalate the *retry* delay, not just the logging. A process that
+                        // survives three force-kills is usually wedged in the kernel and
+                        // only a reboot clears it, so a fixed 5s retry would mean a full
+                        // sysinfo process scan every 5s for the life of the machine.
+                        let delay =
+                            (RETRY_DELAY_SECONDS * failures).min(KILL_RETRY_MAX_DELAY_SECONDS);
+                        if log_attempt {
+                            error!(tool_id = %tool.tool_agent_id, failed_attempts = failures,
+                                   "Failed to stop leftover tool processes - not launching, retrying in {} seconds: {:#}",
+                                   delay, e);
+                        }
+                        sleep(Duration::from_secs(delay)).await;
+                        continue;
+                    }
+                }
+
                 let processed_args =
                     match params_processor.process(&tool.tool_agent_id, run_command_args.clone()) {
                         Ok(args) => args,
@@ -877,10 +912,22 @@ impl ToolRunManager {
                                     let prefs = crate::platform::preferences_writer::args_to_pairs(
                                         &processed_args,
                                     );
+                                    // Preferences are the ONLY channel a GuiApp's args travel
+                                    // on macOS, so launching after a failed write starts the
+                                    // app with no serverUrl and reports success — the app
+                                    // then stays misconfigured until someone reinstalls it.
+                                    // Treat it as a launch failure and retry instead.
                                     if let Err(e) =
                                         crate::platform::preferences_writer::write(bid, prefs)
                                     {
-                                        error!(tool_id = %tool.tool_agent_id, "Failed to write preferences: {:#}", e);
+                                        let failures = launch_backoff.record_failure(log_attempt);
+                                        if log_attempt {
+                                            error!(tool_id = %tool.tool_agent_id, failed_attempts = failures,
+                                                   "Failed to write GuiApp preferences - not launching, retrying in {} seconds: {:#}",
+                                                   RETRY_DELAY_SECONDS, e);
+                                        }
+                                        sleep(Duration::from_secs(RETRY_DELAY_SECONDS)).await;
+                                        continue;
                                     }
 
                                     if tool.tool_agent_id == "openframe-chat" {
@@ -1041,6 +1088,45 @@ impl ToolRunManager {
     }
 }
 
+/// Clears the updating flag on drop (surviving cancellation and panic), releasing the tool lock only after the flag clears.
+/// Lives here rather than in one caller because the update, restart and install paths all need the same guarantee:
+/// a leaked flag parks the tool's supervisor, defers its connection processing, and blocks every client self-update.
+pub struct UpdatingGuard {
+    tool_run_manager: ToolRunManager,
+    tool_agent_id: String,
+    lock_guard: Option<tokio::sync::OwnedMutexGuard<()>>,
+}
+
+impl UpdatingGuard {
+    /// Marks the tool as updating and holds `lock_guard` (if any) until the flag clears again.
+    pub async fn acquire(
+        tool_run_manager: &ToolRunManager,
+        tool_agent_id: &str,
+        lock_guard: Option<tokio::sync::OwnedMutexGuard<()>>,
+    ) -> Self {
+        tool_run_manager.mark_updating(tool_agent_id).await;
+        Self {
+            tool_run_manager: tool_run_manager.clone(),
+            tool_agent_id: tool_agent_id.to_string(),
+            lock_guard,
+        }
+    }
+}
+
+impl Drop for UpdatingGuard {
+    fn drop(&mut self) {
+        if let Ok(handle) = tokio::runtime::Handle::try_current() {
+            let manager = self.tool_run_manager.clone();
+            let tool_agent_id = self.tool_agent_id.clone();
+            let lock_guard = self.lock_guard.take();
+            handle.spawn(async move {
+                manager.clear_updating(&tool_agent_id).await;
+                drop(lock_guard);
+            });
+        }
+    }
+}
+
 #[cfg(test)]
 #[path = "tool_run_manager_tests.rs"]
 mod tests;

From 3b5b818af08551c0725b518f47240beabad0147b Mon Sep 17 00:00:00 2001
From: Danylo <danylo@flamingo.cx>
Date: Wed, 23 Sep 2026 19:01:22 +0300
Subject: [PATCH 2/4] fix(client): address review - guard the leftover kill,
 lock before reading, reuse the canonical path resolver, trim comments

---
 clients/openframe-client/src/lib.rs           |  4 +-
 .../src/platform/tool_updater/gui_app.rs      | 18 ++++-
 .../src/platform/tool_updater/service.rs      | 19 +-----
 .../src/platform/tool_updater/standard.rs     | 60 ++++-------------
 .../platform/tool_updater/standard_tests.rs   | 63 ------------------
 clients/openframe-client/src/service.rs       |  8 +--
 .../src/services/tool_agent_update_service.rs | 66 +++++++++++--------
 .../src/services/tool_run_manager.rs          | 65 +++++++-----------
 8 files changed, 94 insertions(+), 209 deletions(-)
 delete mode 100644 clients/openframe-client/src/platform/tool_updater/standard_tests.rs

diff --git a/clients/openframe-client/src/lib.rs b/clients/openframe-client/src/lib.rs
index eef0de74b6..733b554f50 100644
--- a/clients/openframe-client/src/lib.rs
+++ b/clients/openframe-client/src/lib.rs
@@ -762,9 +762,7 @@ impl Client {
         self.script_schedule_execution_listener.start().await?;
         info!("Script schedule execution listener started");
 
-        // Start tool run manager. A tool lane that cannot start must not end the service
-        // core — the agent still has to heartbeat, stream logs and accept remote commands,
-        // which is how an operator repairs that tool in the first place.
+        // A tool lane that cannot start must not end the service core.
         if let Err(e) = self.tool_run_manager.run().await {
             error!("Failed to start tool run manager: {:#}", e);
         }
diff --git a/clients/openframe-client/src/platform/tool_updater/gui_app.rs b/clients/openframe-client/src/platform/tool_updater/gui_app.rs
index d62f9fc6dc..ad3e854642 100644
--- a/clients/openframe-client/src/platform/tool_updater/gui_app.rs
+++ b/clients/openframe-client/src/platform/tool_updater/gui_app.rs
@@ -18,8 +18,14 @@ impl GuiAppToolUpdater {
         Self { deps }
     }
 
+    /// Sibling of the bundle so the rename stays on one volume, dot-prefixed so an
+    /// interrupted update never leaves a second app visible in Finder.
     fn backup_path_for(bundle: &Path) -> PathBuf {
-        bundle.with_extension("app.update-backup")
+        let name = bundle
+            .file_name()
+            .map(|n| n.to_string_lossy().to_string())
+            .unwrap_or_else(|| "bundle".to_string());
+        bundle.with_file_name(format!(".{name}.update-backup"))
     }
 
     /// Renames the installed `.app` to a sibling backup so a failed download can be undone.
@@ -35,11 +41,17 @@ impl GuiAppToolUpdater {
             warn!(tool_id = %tool_agent_id, "No .app bundle in {} — updating without a backup", executable_path);
             return Ok(None);
         };
+        let backup = Self::backup_path_for(&bundle);
+
+        // A backup left by a run that died mid-download is the only copy of the app. Adopt
+        // it when the bundle is gone; drop it as stale when the bundle is back.
         if !bundle.exists() {
+            if backup.exists() {
+                info!(tool_id = %tool_agent_id, "Adopting the backup left by an interrupted update");
+                return Ok(Some(backup));
+            }
             return Ok(None);
         }
-
-        let backup = Self::backup_path_for(&bundle);
         if backup.exists() {
             tokio::fs::remove_dir_all(&backup).await.ok();
         }
diff --git a/clients/openframe-client/src/platform/tool_updater/service.rs b/clients/openframe-client/src/platform/tool_updater/service.rs
index 95adf0b376..e4c81188b0 100644
--- a/clients/openframe-client/src/platform/tool_updater/service.rs
+++ b/clients/openframe-client/src/platform/tool_updater/service.rs
@@ -22,24 +22,9 @@ impl ServiceToolUpdater {
     }
 
     fn resolve_executable_path(&self, tool: &InstalledTool) -> PathBuf {
-        let agent_path = self
-            .deps
+        self.deps
             .directory_manager
-            .get_agent_path(&tool.tool_agent_id);
-
-        if let Installation::Service {
-            executable_path: Some(exec_path),
-            ..
-        } = &tool.installation
-        {
-            if exec_path.starts_with('/') || exec_path.contains(':') {
-                PathBuf::from(exec_path)
-            } else {
-                agent_path.parent().unwrap_or(&agent_path).join(exec_path)
-            }
-        } else {
-            agent_path
-        }
+            .get_tool_executable_path(&tool.tool_agent_id, tool.installation.executable_path())
     }
 
     /// Check if download config targets an .app bundle
diff --git a/clients/openframe-client/src/platform/tool_updater/standard.rs b/clients/openframe-client/src/platform/tool_updater/standard.rs
index e653182045..8b49ebccd8 100644
--- a/clients/openframe-client/src/platform/tool_updater/standard.rs
+++ b/clients/openframe-client/src/platform/tool_updater/standard.rs
@@ -16,53 +16,8 @@ impl StandardToolUpdater {
     pub fn new(deps: ToolUpdaterDeps) -> Self {
         Self { deps }
     }
-
-    /// Honour the executable path the installer recorded. A folder-extraction config
-    /// installs to e.g. `<tool>/bin/tool`, not `<tool>/agent` — writing to the latter
-    /// regardless left the tool running its old binary while the record, and the backend,
-    /// reported the new version. `ServiceToolUpdater::resolve_executable_path` already
-    /// does this; Standard was the outlier.
-    fn resolve_executable_path(&self, tool: &InstalledTool) -> std::path::PathBuf {
-        let agent_path = self
-            .deps
-            .directory_manager
-            .get_agent_path(&tool.tool_agent_id);
-
-        let recorded = match &tool.installation {
-            Installation::Standard {
-                executable_path: Some(exec_path),
-            } => Some(exec_path.as_str()),
-            _ => None,
-        };
-
-        resolve_recorded_executable(agent_path, recorded)
-    }
-}
-
-/// Resolve a recorded executable path against the default `<tool>/agent` path, mirroring
-/// `DirectoryManager::get_tool_executable_path`. Absolute paths (unix `/…`, Windows `C:\…`)
-/// are taken as-is; a relative path is joined onto the tool's own directory.
-///
-/// Split out from the method above purely so it can be tested without constructing a whole
-/// `ToolUpdaterDeps` — it is the part that decides which file an update overwrites, and
-/// getting it wrong silently leaves the tool running its old binary.
-fn resolve_recorded_executable(
-    agent_path: std::path::PathBuf,
-    recorded: Option<&str>,
-) -> std::path::PathBuf {
-    match recorded {
-        Some(exec_path) if exec_path.starts_with('/') || exec_path.contains(':') => {
-            std::path::PathBuf::from(exec_path)
-        }
-        Some(exec_path) => agent_path.parent().unwrap_or(&agent_path).join(exec_path),
-        None => agent_path,
-    }
 }
 
-#[cfg(test)]
-#[path = "standard_tests.rs"]
-mod tests;
-
 #[async_trait]
 impl ToolUpdater for StandardToolUpdater {
     async fn prepare(&self, tool: &InstalledTool) -> Result<UpdateContext> {
@@ -76,7 +31,10 @@ impl ToolUpdater for StandardToolUpdater {
             .await
             .with_context(|| format!("Failed to stop tool: {}", tool_agent_id))?;
 
-        let agent_path = self.resolve_executable_path(tool);
+        let agent_path = self
+            .deps
+            .directory_manager
+            .get_tool_executable_path(&tool.tool_agent_id, tool.installation.executable_path());
         clear_aside_binary(&agent_path, tool_agent_id).await;
         log_update_survivors(&self.deps, tool).await;
 
@@ -97,7 +55,10 @@ impl ToolUpdater for StandardToolUpdater {
         let tool_agent_id = &tool.tool_agent_id;
         info!(tool_id = %tool_agent_id, "Applying Standard tool update");
 
-        let agent_path = self.resolve_executable_path(tool);
+        let agent_path = self
+            .deps
+            .directory_manager
+            .get_tool_executable_path(&tool.tool_agent_id, tool.installation.executable_path());
         download_and_write_binary(&self.deps, config, &agent_path, tool_agent_id).await?;
         Ok(None)
     }
@@ -119,7 +80,10 @@ impl ToolUpdater for StandardToolUpdater {
         let tool_agent_id = &tool.tool_agent_id;
         info!(tool_id = %tool_agent_id, "Rolling back Standard tool update");
 
-        let agent_path = self.resolve_executable_path(tool);
+        let agent_path = self
+            .deps
+            .directory_manager
+            .get_tool_executable_path(&tool.tool_agent_id, tool.installation.executable_path());
         restore_from_backup(ctx.backup_path.as_ref(), &agent_path, tool_agent_id).await
     }
 }
diff --git a/clients/openframe-client/src/platform/tool_updater/standard_tests.rs b/clients/openframe-client/src/platform/tool_updater/standard_tests.rs
deleted file mode 100644
index 1fcb67a7d2..0000000000
--- a/clients/openframe-client/src/platform/tool_updater/standard_tests.rs
+++ /dev/null
@@ -1,63 +0,0 @@
-use super::resolve_recorded_executable;
-use std::path::PathBuf;
-
-fn agent_path() -> PathBuf {
-    PathBuf::from("/Library/Application Support/OpenFrame/fleetmdm-agent/agent")
-}
-
-#[test]
-fn no_recorded_path_falls_back_to_the_default_agent_path() {
-    assert_eq!(
-        resolve_recorded_executable(agent_path(), None),
-        agent_path()
-    );
-}
-
-/// The case the fix exists for: a folder-extraction config installs to `<tool>/bin/orbit`,
-/// so writing to `<tool>/agent` would leave the tool running its old binary while the
-/// record — and the backend — reported the new version.
-#[test]
-fn relative_recorded_path_is_joined_onto_the_tool_directory() {
-    assert_eq!(
-        resolve_recorded_executable(agent_path(), Some("bin/orbit")),
-        PathBuf::from("/Library/Application Support/OpenFrame/fleetmdm-agent/bin/orbit")
-    );
-}
-
-#[test]
-fn absolute_unix_recorded_path_is_taken_as_is() {
-    assert_eq!(
-        resolve_recorded_executable(agent_path(), Some("/opt/orbit/bin/orbit")),
-        PathBuf::from("/opt/orbit/bin/orbit")
-    );
-}
-
-/// Windows absolute paths are detected by the drive colon, not a leading slash.
-#[test]
-fn windows_absolute_recorded_path_is_taken_as_is() {
-    assert_eq!(
-        resolve_recorded_executable(
-            PathBuf::from("C:\\ProgramData\\OpenFrame\\fleetmdm-agent\\agent.exe"),
-            Some("C:\\Program Files\\Orbit\\orbit.exe")
-        ),
-        PathBuf::from("C:\\Program Files\\Orbit\\orbit.exe")
-    );
-}
-
-/// A bare filename still lands beside the default agent binary rather than replacing it.
-#[test]
-fn bare_filename_resolves_next_to_the_agent_binary() {
-    assert_eq!(
-        resolve_recorded_executable(agent_path(), Some("orbit")),
-        PathBuf::from("/Library/Application Support/OpenFrame/fleetmdm-agent/orbit")
-    );
-}
-
-/// Equivalent to the default: the resolver must not disturb the common single-binary case.
-#[test]
-fn recorded_path_equal_to_the_default_resolves_to_the_default() {
-    assert_eq!(
-        resolve_recorded_executable(agent_path(), Some("agent")),
-        agent_path()
-    );
-}
diff --git a/clients/openframe-client/src/service.rs b/clients/openframe-client/src/service.rs
index db97c49082..61ab64f7ae 100644
--- a/clients/openframe-client/src/service.rs
+++ b/clients/openframe-client/src/service.rs
@@ -121,12 +121,8 @@ fn windows_service_main(_args: Vec<std::ffi::OsString>) {
         }
     });
 
-    // Report a non-zero exit code so the SCM treats this as a failure. The recovery
-    // ladder installed at register time (10s/60s/300s) plus
-    // `set_failure_actions_on_non_crash_failures` only fire when SERVICE_STOPPED
-    // carries a non-zero code; reporting Win32(0) on both paths made a dead core
-    // indistinguishable from an operator-requested stop, so nothing ever restarted it.
-    // The reason goes through tracing, not stderr, or it never reaches openframe.log.
+    // A non-zero exit code is what makes SCM treat this as a failure and fire the
+    // configured recovery ladder; Win32(0) on both paths meant nothing ever restarted.
     if let Err(e) = result {
         error!("Service core failed: {:#}", e);
         let _ = set_service_status(
diff --git a/clients/openframe-client/src/services/tool_agent_update_service.rs b/clients/openframe-client/src/services/tool_agent_update_service.rs
index 423e9ff2ad..d9930c0468 100644
--- a/clients/openframe-client/src/services/tool_agent_update_service.rs
+++ b/clients/openframe-client/src/services/tool_agent_update_service.rs
@@ -69,6 +69,25 @@ impl ToolAgentUpdateService {
             tool_agent_id, new_version
         );
 
+        // Lock before reading anything: everything below reads and rewrites the registry
+        // record, and a concurrent uninstall holding this lock can delete it underneath us —
+        // a stale copy written back afterwards resurrects a tool whose binary is gone.
+        // Defer rather than block, like uninstall and restart do.
+        let tool_lock = self.tool_run_manager.tool_lock(tool_agent_id).await;
+        let _lock_guard = match tool_lock.try_lock_owned() {
+            Ok(guard) => guard,
+            Err(_) => {
+                info!(
+                    "Tool {} busy with another operation, deferring update",
+                    tool_agent_id
+                );
+                anyhow::bail!(
+                    "tool {} busy, deferring update for redelivery",
+                    tool_agent_id
+                );
+            }
+        };
+
         // Check if tool is installed
         let mut installed_tool = match self
             .installed_tools_service
@@ -152,32 +171,11 @@ impl ToolAgentUpdateService {
             return Ok(());
         }
 
-        // Serialise against install/reinstall/uninstall/restart, which all take this same
-        // lock. Updates arrive on a different NATS stream, so without it an uninstall can
-        // delete the registry record while this update holds a stale copy and then writes
-        // it back — resurrecting a record for a tool whose binary is gone. Deferring (like
-        // uninstall and restart do) rather than blocking keeps a slow neighbour from
-        // pushing this past ack_wait into a duplicate delivery.
-        let tool_lock = self.tool_run_manager.tool_lock(tool_agent_id).await;
-        let lock_guard = match tool_lock.try_lock_owned() {
-            Ok(guard) => guard,
-            Err(_) => {
-                info!(
-                    "Tool {} is busy with another operation, deferring update for redelivery",
-                    tool_agent_id
-                );
-                anyhow::bail!(
-                    "tool {} busy with another operation, deferring update for redelivery",
-                    tool_agent_id
-                );
-            }
-        };
-
         // Mark as updating once for all updates; the guard clears the flag — and only then
         // releases the lock — on return, panic or cancellation. A leaked flag parks the
         // tool's supervisor and blocks every client self-update for the process lifetime.
         let _updating =
-            UpdatingGuard::acquire(&self.tool_run_manager, tool_agent_id, Some(lock_guard)).await;
+            UpdatingGuard::acquire(&self.tool_run_manager, tool_agent_id, Some(_lock_guard)).await;
 
         // A Standard->GuiApp migration self-relaunches, so only relaunch here if it was already a GUI app.
         let was_gui_before_update =
@@ -241,13 +239,13 @@ impl ToolAgentUpdateService {
             // and `launchctl load` on an already-loaded job is a no-op that still reports
             // success — so the restart below would silently start nothing on macOS.
             info!(tool_id = %tool_agent_id, "Stopping tool for asset updates");
+            stopped_for_assets = true;
             self.tool_kill_service
                 .stop_installed_tool(installed_tool, false)
                 .await
                 .with_context(|| {
                     format!("Failed to stop tool {} for asset updates", tool_agent_id)
                 })?;
-            stopped_for_assets = true;
         }
 
         // 2. Asset updates (tool already stopped by tool_update or above).
@@ -269,9 +267,25 @@ impl ToolAgentUpdateService {
         // silently takes the agent down until a reboot or a reinstall. Supervised installs
         // are relaunched by their run loop once the updating flag clears.
         if stopped_for_assets {
-            if let Installation::Service { service_name, .. } = &installed_tool.installation {
-                self.restart_service_after_assets(tool_agent_id, service_name)
-                    .await;
+            match &installed_tool.installation {
+                Installation::Service { service_name, .. } => {
+                    self.restart_service_after_assets(tool_agent_id, service_name)
+                        .await;
+                }
+                // The macOS GuiApp supervisor exits once the app is verified running, so
+                // nothing is left to relaunch it; spawn a fresh one. Windows GuiApps are
+                // relaunched by relaunch_windows_gui_app at the end of process_update.
+                #[cfg(target_os = "macos")]
+                Installation::GuiApp { .. } => {
+                    if let Err(e) = self
+                        .tool_run_manager
+                        .run_new_tool(installed_tool.clone())
+                        .await
+                    {
+                        warn!(tool_id = %tool_agent_id, "Failed to relaunch GuiApp after asset updates: {:#}", e);
+                    }
+                }
+                _ => {}
             }
         }
 
diff --git a/clients/openframe-client/src/services/tool_run_manager.rs b/clients/openframe-client/src/services/tool_run_manager.rs
index bfe44178ec..7289615ec0 100644
--- a/clients/openframe-client/src/services/tool_run_manager.rs
+++ b/clients/openframe-client/src/services/tool_run_manager.rs
@@ -556,10 +556,7 @@ impl ToolRunManager {
             }
             let tool_id = tool.tool_agent_id.clone();
             info!(tool_id = %tool_id, "Relaunching tool supervisor after the aborted update");
-            if let Err(e) = self.run_tool(tool, false).await {
-                warn!(tool_id = %tool_id, "Failed to relaunch tool after the aborted update: {:#}", e);
-                self.clear_running_tool(&tool_id).await;
-            }
+            self.run_tool(tool, false).await;
         }
         info!("Tool run manager: supervision resumed after the aborted update");
         Ok(())
@@ -669,13 +666,7 @@ impl ToolRunManager {
             let tool_id = tool.tool_agent_id.clone();
             if self.try_mark_running(&tool_id).await {
                 info!("Running tool {}", tool_id);
-                // One tool must never abort the startup sequence: this error reached
-                // Client::start() through `?` and ended the service core, so a single
-                // tool that could not be launched took the whole agent down with it.
-                if let Err(e) = self.run_tool(tool, false).await {
-                    warn!(tool_id = %tool_id, "Failed to start tool during startup: {:#}", e);
-                    self.clear_running_tool(&tool_id).await;
-                }
+                self.run_tool(tool, false).await;
             } else {
                 warn!("Tool {} is already running - skipping", tool_id);
             }
@@ -694,14 +685,8 @@ impl ToolRunManager {
         }
 
         info!("Running new single tool {}", installed_tool.tool_agent_id);
-        let tool_id = installed_tool.tool_agent_id.clone();
-        // Release the mark taken above if supervision never started, or the tool stays
-        // marked running forever and every later attempt skips it as already running.
-        let result = self.run_tool(installed_tool, true).await;
-        if result.is_err() {
-            self.clear_running_tool(&tool_id).await;
-        }
-        result
+        self.run_tool(installed_tool, true).await;
+        Ok(())
     }
 
     async fn try_mark_running(&self, tool_id: &str) -> bool {
@@ -720,14 +705,14 @@ impl ToolRunManager {
     }
 
     #[allow(unused_variables)]
-    async fn run_tool(&self, tool: InstalledTool, new_tool: bool) -> Result<()> {
+    async fn run_tool(&self, tool: InstalledTool, new_tool: bool) {
         if tool.installation.is_service() {
             info!(
                 "Installation::Service for {} - self-managed, skipping launch",
                 tool.tool_agent_id
             );
             self.clear_running_tool(&tool.tool_agent_id).await;
-            return Ok(());
+            return;
         }
 
         let updating_tools = self.updating_tools.clone();
@@ -741,6 +726,10 @@ impl ToolRunManager {
 
         tokio::spawn(async move {
             let mut launch_backoff = FailureLogBackoff::new();
+            // Only on the first pass, and again after a supervised child exits. A launch
+            // retry must not re-kill: a GuiApp that needs longer than the 3s verify window
+            // would be killed by the next iteration, forever.
+            let mut kill_leftovers_now = true;
             loop {
                 // Self-update in progress — stop the loop entirely
                 if shutdown_break(&shutting_down, &running_tools, &tool.tool_agent_id).await {
@@ -772,25 +761,18 @@ impl ToolRunManager {
 
                 let log_attempt = launch_backoff.should_log();
 
-                // Leftovers from a previous run have to be gone before we launch, or two
-                // instances fight over the same state. The kill belongs inside the loop:
-                // a process that will not die is a retryable launch failure like any other
-                // — run once ahead of the loop, its error propagated out of Client::start()
-                // and ended the service core over one stuck tool, and skipping the launch
-                // instead would leave the tool unsupervised until someone reinstalled it.
-                // Windows GUI apps are owned by the HKLM Run autorun, not us — never kill them.
+                // A leftover that will not die is a retryable launch failure, not a fatal
+                // one. Windows GUI apps belong to the HKLM Run autorun — never kill them.
                 #[cfg(target_os = "windows")]
                 let kill_leftovers = !installation.is_gui_app();
                 #[cfg(not(target_os = "windows"))]
                 let kill_leftovers = true;
 
-                if kill_leftovers {
+                if kill_leftovers && kill_leftovers_now {
                     if let Err(e) = tool_kill_service.stop_tool(&tool.tool_agent_id).await {
                         let failures = launch_backoff.record_failure(log_attempt);
-                        // Escalate the *retry* delay, not just the logging. A process that
-                        // survives three force-kills is usually wedged in the kernel and
-                        // only a reboot clears it, so a fixed 5s retry would mean a full
-                        // sysinfo process scan every 5s for the life of the machine.
+                        // Escalate the retry delay, not just the logging: each attempt is a
+                        // full process-table scan and a wedged process only clears on reboot.
                         let delay =
                             (RETRY_DELAY_SECONDS * failures).min(KILL_RETRY_MAX_DELAY_SECONDS);
                         if log_attempt {
@@ -801,6 +783,7 @@ impl ToolRunManager {
                         sleep(Duration::from_secs(delay)).await;
                         continue;
                     }
+                    kill_leftovers_now = false;
                 }
 
                 let processed_args =
@@ -912,11 +895,8 @@ impl ToolRunManager {
                                     let prefs = crate::platform::preferences_writer::args_to_pairs(
                                         &processed_args,
                                     );
-                                    // Preferences are the ONLY channel a GuiApp's args travel
-                                    // on macOS, so launching after a failed write starts the
-                                    // app with no serverUrl and reports success — the app
-                                    // then stays misconfigured until someone reinstalls it.
-                                    // Treat it as a launch failure and retry instead.
+                                    // Preferences are the only channel GuiApp args travel on
+                                    // macOS; launching without them starts an unconfigured app.
                                     if let Err(e) =
                                         crate::platform::preferences_writer::write(bid, prefs)
                                     {
@@ -1080,17 +1060,16 @@ impl ToolRunManager {
                                "Failed to wait for tool process - restarting in {} seconds: {:#}", RETRY_DELAY_SECONDS, e);
                     }
                 }
+                kill_leftovers_now = true;
                 sleep(Duration::from_secs(RETRY_DELAY_SECONDS)).await;
             }
         });
-
-        Ok(())
     }
 }
 
-/// Clears the updating flag on drop (surviving cancellation and panic), releasing the tool lock only after the flag clears.
-/// Lives here rather than in one caller because the update, restart and install paths all need the same guarantee:
-/// a leaked flag parks the tool's supervisor, defers its connection processing, and blocks every client self-update.
+/// Clears the updating flag on drop (surviving cancellation and panic), releasing the tool
+/// lock only once the flag is clear. Used by the update and restart paths; install and
+/// uninstall still pair mark/clear by hand.
 pub struct UpdatingGuard {
     tool_run_manager: ToolRunManager,
     tool_agent_id: String,

From dd8d4cd48d2ef8603d7112ed297f65a2e65a4cf9 Mon Sep 17 00:00:00 2001
From: Danylo <danylo@flamingo.cx>
Date: Wed, 23 Sep 2026 19:02:29 +0300
Subject: [PATCH 3/4] revert(client): leave ServiceToolUpdater path resolution
 to #2230, which owns that file

---
 .../src/platform/tool_updater/service.rs      | 19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)

diff --git a/clients/openframe-client/src/platform/tool_updater/service.rs b/clients/openframe-client/src/platform/tool_updater/service.rs
index e4c81188b0..95adf0b376 100644
--- a/clients/openframe-client/src/platform/tool_updater/service.rs
+++ b/clients/openframe-client/src/platform/tool_updater/service.rs
@@ -22,9 +22,24 @@ impl ServiceToolUpdater {
     }
 
     fn resolve_executable_path(&self, tool: &InstalledTool) -> PathBuf {
-        self.deps
+        let agent_path = self
+            .deps
             .directory_manager
-            .get_tool_executable_path(&tool.tool_agent_id, tool.installation.executable_path())
+            .get_agent_path(&tool.tool_agent_id);
+
+        if let Installation::Service {
+            executable_path: Some(exec_path),
+            ..
+        } = &tool.installation
+        {
+            if exec_path.starts_with('/') || exec_path.contains(':') {
+                PathBuf::from(exec_path)
+            } else {
+                agent_path.parent().unwrap_or(&agent_path).join(exec_path)
+            }
+        } else {
+            agent_path
+        }
     }
 
     /// Check if download config targets an .app bundle

From 051613f111cfa667b636aca7cacbc1fd5413a70e Mon Sep 17 00:00:00 2001
From: Danylo <danylo@flamingo.cx>
Date: Wed, 23 Sep 2026 19:05:26 +0300
Subject: [PATCH 4/4] style(client): drop explanatory comments, keep only
 constant and API docs

---
 clients/openframe-client/src/lib.rs           |  1 -
 .../src/platform/preferences_writer.rs        |  5 ---
 .../src/platform/tool_updater/gui_app.rs      | 14 --------
 clients/openframe-client/src/service.rs       |  2 --
 .../src/services/github_download_service.rs   | 11 +------
 .../services/github_download_service_tests.rs | 13 --------
 .../services/initial_configuration_service.rs |  4 ---
 .../src/services/last_known_good_service.rs   |  9 ------
 .../src/services/tool_agent_update_service.rs | 32 -------------------
 .../src/services/tool_run_manager.rs          | 14 ++------
 10 files changed, 3 insertions(+), 102 deletions(-)

diff --git a/clients/openframe-client/src/lib.rs b/clients/openframe-client/src/lib.rs
index 733b554f50..9913f87d79 100644
--- a/clients/openframe-client/src/lib.rs
+++ b/clients/openframe-client/src/lib.rs
@@ -762,7 +762,6 @@ impl Client {
         self.script_schedule_execution_listener.start().await?;
         info!("Script schedule execution listener started");
 
-        // A tool lane that cannot start must not end the service core.
         if let Err(e) = self.tool_run_manager.run().await {
             error!("Failed to start tool run manager: {:#}", e);
         }
diff --git a/clients/openframe-client/src/platform/preferences_writer.rs b/clients/openframe-client/src/platform/preferences_writer.rs
index bc06605586..f9a3126783 100644
--- a/clients/openframe-client/src/platform/preferences_writer.rs
+++ b/clients/openframe-client/src/platform/preferences_writer.rs
@@ -19,11 +19,6 @@ pub fn write<'a>(
     }
 
     for (key, value) in &prefs {
-        // `launchctl asuser` first, `sudo -u` only as a fallback — the same order
-        // `user_session::launch_as_user` uses. From a LaunchDaemon there is no user
-        // session bootstrap, so a bare `sudo -u defaults write` is rejected by cfprefsd
-        // ("Could not write domain ...; exiting") and the app then launches with none of
-        // its configuration, because preferences are the only channel GuiApp args travel.
         let status = Command::new("launchctl")
             .args([
                 "asuser",
diff --git a/clients/openframe-client/src/platform/tool_updater/gui_app.rs b/clients/openframe-client/src/platform/tool_updater/gui_app.rs
index ad3e854642..655c309ca4 100644
--- a/clients/openframe-client/src/platform/tool_updater/gui_app.rs
+++ b/clients/openframe-client/src/platform/tool_updater/gui_app.rs
@@ -18,8 +18,6 @@ impl GuiAppToolUpdater {
         Self { deps }
     }
 
-    /// Sibling of the bundle so the rename stays on one volume, dot-prefixed so an
-    /// interrupted update never leaves a second app visible in Finder.
     fn backup_path_for(bundle: &Path) -> PathBuf {
         let name = bundle
             .file_name()
@@ -28,10 +26,6 @@ impl GuiAppToolUpdater {
         bundle.with_file_name(format!(".{name}.update-backup"))
     }
 
-    /// Renames the installed `.app` to a sibling backup so a failed download can be undone.
-    /// The old code deleted the bundle outright and then downloaded its replacement, so a
-    /// dropped connection left no app at all and a rollback that could only log
-    /// "reinstall required". A rename is atomic, same-volume, and costs nothing.
     async fn move_bundle_aside(
         executable_path: &str,
         tool_agent_id: &str,
@@ -43,8 +37,6 @@ impl GuiAppToolUpdater {
         };
         let backup = Self::backup_path_for(&bundle);
 
-        // A backup left by a run that died mid-download is the only copy of the app. Adopt
-        // it when the bundle is gone; drop it as stale when the bundle is back.
         if !bundle.exists() {
             if backup.exists() {
                 info!(tool_id = %tool_agent_id, "Adopting the backup left by an interrupted update");
@@ -111,8 +103,6 @@ impl ToolUpdater for GuiAppToolUpdater {
             );
         };
 
-        // The old bundle was moved aside in `prepare`, so the destination is already free
-        // and `rollback` can put it back if this download fails.
         let applications_dir = PathBuf::from("/Applications");
 
         info!(tool_id = %tool_agent_id, "Downloading and installing new version from: {}", config.link);
@@ -143,9 +133,6 @@ impl ToolUpdater for GuiAppToolUpdater {
         let tool_agent_id = &tool.tool_agent_id;
         info!(tool_id = %tool_agent_id, "Finalizing GuiApp update");
 
-        // `finalize` only runs after a successful apply, so the kept-aside bundle has done
-        // its job. Drop it here rather than after the relaunch, so the early return below
-        // cannot leak a full copy of the app into /Applications.
         if let Some(backup) = &ctx.backup_path {
             if backup.exists() {
                 if let Err(e) = tokio::fs::remove_dir_all(backup).await {
@@ -235,7 +222,6 @@ impl ToolUpdater for GuiAppToolUpdater {
             anyhow::bail!("Could not resolve the .app bundle path for {tool_agent_id}");
         };
 
-        // A partially extracted new bundle must not block the restore.
         if bundle.exists() {
             tokio::fs::remove_dir_all(&bundle).await.ok();
         }
diff --git a/clients/openframe-client/src/service.rs b/clients/openframe-client/src/service.rs
index 61ab64f7ae..e28987d772 100644
--- a/clients/openframe-client/src/service.rs
+++ b/clients/openframe-client/src/service.rs
@@ -121,8 +121,6 @@ fn windows_service_main(_args: Vec<std::ffi::OsString>) {
         }
     });
 
-    // A non-zero exit code is what makes SCM treat this as a failure and fire the
-    // configured recovery ladder; Win32(0) on both paths meant nothing ever restarted.
     if let Err(e) = result {
         error!("Service core failed: {:#}", e);
         let _ = set_service_status(
diff --git a/clients/openframe-client/src/services/github_download_service.rs b/clients/openframe-client/src/services/github_download_service.rs
index 6b58e4e1fd..5286ab3bb6 100644
--- a/clients/openframe-client/src/services/github_download_service.rs
+++ b/clients/openframe-client/src/services/github_download_service.rs
@@ -13,15 +13,7 @@ use std::path::{Component, PathBuf};
 use tokio::time::Duration;
 use tracing::{info, warn};
 
-/// Join an archive entry path under `target_dir`, refusing anything that could escape it.
-/// `Path::join` with an absolute entry *replaces* the base, and `..` is resolved by the OS,
-/// so an unsanitised entry in a downloaded archive writes anywhere on disk — as root, and
-/// with the archive's own mode bits. `tar::Archive::unpack` guards this; this hand-rolled
-/// loop has to do it itself.
-///
-/// Gated to macOS alongside its only caller, `extract_all_from_tar_gz`: on other platforms
-/// `download_and_extract_all` refuses outright, so an ungated helper here is dead code and
-/// `clippy -D warnings` rejects it.
+/// `Path::join` with an absolute entry replaces the base, and `..` is resolved by the OS.
 #[cfg(target_os = "macos")]
 fn safe_join(target_dir: &Path, entry_path: &Path) -> Result<PathBuf> {
     for component in entry_path.components() {
@@ -38,7 +30,6 @@ fn safe_join(target_dir: &Path, entry_path: &Path) -> Result<PathBuf> {
     Ok(target_dir.join(entry_path))
 }
 
-// macOS-gated alongside `safe_join` itself.
 #[cfg(all(test, target_os = "macos"))]
 #[path = "github_download_service_tests.rs"]
 mod tests;
diff --git a/clients/openframe-client/src/services/github_download_service_tests.rs b/clients/openframe-client/src/services/github_download_service_tests.rs
index d91ede58d5..b2d3f59510 100644
--- a/clients/openframe-client/src/services/github_download_service_tests.rs
+++ b/clients/openframe-client/src/services/github_download_service_tests.rs
@@ -9,10 +9,6 @@ fn join(entry: &str) -> Option<PathBuf> {
     safe_join(&target(), Path::new(entry)).ok()
 }
 
-// ---------------------------------------------------------------- accepted
-
-/// tar archives routinely prefix entries with `./`; rejecting those would break every
-/// legitimate download.
 #[test]
 fn accepts_leading_current_dir() {
     assert_eq!(
@@ -37,10 +33,6 @@ fn accepts_a_bare_filename() {
     );
 }
 
-// ---------------------------------------------------------------- rejected
-
-/// The reason this helper exists: `Path::join` with an absolute entry *replaces* the base,
-/// so an unsanitised archive entry would write outside the target — as root.
 #[test]
 fn rejects_an_absolute_entry() {
     assert!(join("/Library/LaunchDaemons/evil.plist").is_none());
@@ -50,9 +42,6 @@ fn rejects_an_absolute_entry() {
 fn rejects_parent_traversal() {
     assert!(join("../../../../etc/cron.d/evil").is_none());
 }
-
-/// `..` must be rejected wherever it appears, not just at the start — `components()`
-/// normalises `.` but never `..`, so an interior segment still escapes.
 #[test]
 fn rejects_interior_parent_traversal() {
     assert!(join("OpenFrame.app/../../../etc/passwd").is_none());
@@ -62,8 +51,6 @@ fn rejects_interior_parent_traversal() {
 fn rejects_a_root_relative_entry() {
     assert!(join("//srv/evil").is_none());
 }
-
-/// Every rejected entry must fail loudly rather than resolve to something surprising.
 #[test]
 fn rejection_names_the_offending_entry() {
     let err = safe_join(&target(), Path::new("../escape"))
diff --git a/clients/openframe-client/src/services/initial_configuration_service.rs b/clients/openframe-client/src/services/initial_configuration_service.rs
index 2a1f17d895..07d8ce86d0 100644
--- a/clients/openframe-client/src/services/initial_configuration_service.rs
+++ b/clients/openframe-client/src/services/initial_configuration_service.rs
@@ -107,10 +107,6 @@ impl InitialConfigurationService {
     pub fn save(&self, config: &InitialConfiguration) -> Result<()> {
         let config_json = serde_json::to_string_pretty(config)
             .context("Failed to serialize initial configuration to JSON")?;
-        // Atomic: a torn write here is unrecoverable in software — `is_configured()` goes
-        // false, the service parks in the awaiting-auth gate, and NATS never connects, so
-        // the machine cannot be repaired remotely. `agent_config.json` next door already
-        // writes this way.
         crate::utils::fs::atomic_write(&self.config_file_path, config_json).with_context(|| {
             format!(
                 "Failed to write initial configuration file: {:?}",
diff --git a/clients/openframe-client/src/services/last_known_good_service.rs b/clients/openframe-client/src/services/last_known_good_service.rs
index 073af366a3..5687fc51d5 100644
--- a/clients/openframe-client/src/services/last_known_good_service.rs
+++ b/clients/openframe-client/src/services/last_known_good_service.rs
@@ -133,15 +133,6 @@ impl LastKnownGoodService {
                 );
                 self.promote(running_version).await
             }
-            // Any other mismatch: the running binary is NOT the anchored one, so it must
-            // not become the reserve. This arm used to seed it anyway — its message said
-            // "running below anchor" but nothing checked the direction, so the common case
-            // (running *newer* than the anchor, i.e. an update whose verification has not
-            // promoted yet) copied an unverified binary into the reserve while the anchor
-            // still named the old version. A rollback then preferred that reserve, restored
-            // the very binary that had just failed, and deleted the good backup behind it.
-            // No reserve is strictly safer than a wrong one: the pre-swap backup still
-            // covers rollback, and the next verified update re-promotes a correct reserve.
             Some(anchor_version) => {
                 warn!(
                     "Rollback protection degraded: reserve missing and running {} does not match anchor {} — leaving the reserve unset for a verified update to rebuild",
diff --git a/clients/openframe-client/src/services/tool_agent_update_service.rs b/clients/openframe-client/src/services/tool_agent_update_service.rs
index d9930c0468..25b227bd2c 100644
--- a/clients/openframe-client/src/services/tool_agent_update_service.rs
+++ b/clients/openframe-client/src/services/tool_agent_update_service.rs
@@ -69,10 +69,6 @@ impl ToolAgentUpdateService {
             tool_agent_id, new_version
         );
 
-        // Lock before reading anything: everything below reads and rewrites the registry
-        // record, and a concurrent uninstall holding this lock can delete it underneath us —
-        // a stale copy written back afterwards resurrects a tool whose binary is gone.
-        // Defer rather than block, like uninstall and restart do.
         let tool_lock = self.tool_run_manager.tool_lock(tool_agent_id).await;
         let _lock_guard = match tool_lock.try_lock_owned() {
             Ok(guard) => guard,
@@ -171,9 +167,6 @@ impl ToolAgentUpdateService {
             return Ok(());
         }
 
-        // Mark as updating once for all updates; the guard clears the flag — and only then
-        // releases the lock — on return, panic or cancellation. A leaked flag parks the
-        // tool's supervisor and blocks every client self-update for the process lifetime.
         let _updating =
             UpdatingGuard::acquire(&self.tool_run_manager, tool_agent_id, Some(_lock_guard)).await;
 
@@ -234,10 +227,6 @@ impl ToolAgentUpdateService {
         let mut stopped_for_assets = false;
         if !needs_tool_update && !assets_to_update.is_empty() {
             // Only assets to update - stop tool once before all asset updates.
-            // `stop_installed_tool`, not `stop_tool`: for Installation::Service this stops
-            // the OS service itself. A process-pattern kill leaves the launchd job loaded,
-            // and `launchctl load` on an already-loaded job is a no-op that still reports
-            // success — so the restart below would silently start nothing on macOS.
             info!(tool_id = %tool_agent_id, "Stopping tool for asset updates");
             stopped_for_assets = true;
             self.tool_kill_service
@@ -249,8 +238,6 @@ impl ToolAgentUpdateService {
         }
 
         // 2. Asset updates (tool already stopped by tool_update or above).
-        // The result is held rather than propagated so the restart below still runs: a tool
-        // we stopped must be started again even when an asset failed halfway.
         let mut asset_result = Ok(());
         for asset in assets_to_update {
             asset_result = self
@@ -261,20 +248,12 @@ impl ToolAgentUpdateService {
             }
         }
 
-        // A Service install is not supervised by the run manager — `run_tool` returns early
-        // for it and `run()` never starts services — so an asset-only update that stopped
-        // the tool has to start it again. Without this a routine asset bump (e.g. osqueryd)
-        // silently takes the agent down until a reboot or a reinstall. Supervised installs
-        // are relaunched by their run loop once the updating flag clears.
         if stopped_for_assets {
             match &installed_tool.installation {
                 Installation::Service { service_name, .. } => {
                     self.restart_service_after_assets(tool_agent_id, service_name)
                         .await;
                 }
-                // The macOS GuiApp supervisor exits once the app is verified running, so
-                // nothing is left to relaunch it; spawn a fresh one. Windows GuiApps are
-                // relaunched by relaunch_windows_gui_app at the end of process_update.
                 #[cfg(target_os = "macos")]
                 Installation::GuiApp { .. } => {
                     if let Err(e) = self
@@ -292,17 +271,6 @@ impl ToolAgentUpdateService {
         asset_result
     }
 
-    /// Start a Service tool back up after an asset-only update.
-    ///
-    /// Retry and start-verification deliberately live in `system_service::start_service`
-    /// rather than here, so there is one implementation of "starting a service is flaky"
-    /// (see #2230, which gives it a backoff schedule and classifies non-retryable codes).
-    ///
-    /// Returns nothing instead of an error on purpose: every asset version was already
-    /// persisted and published by `do_asset_update`, so a redelivery finds nothing left to
-    /// update and acks immediately — propagating an error here would look like a retry
-    /// while actually discarding the message with the tool still stopped. A loud log is the
-    /// honest signal; the alternative is a failure that silently disappears.
     async fn restart_service_after_assets(&self, tool_agent_id: &str, service_name: &str) {
         info!(tool_id = %tool_agent_id, service = %service_name, "Restarting service tool after asset updates");
 
diff --git a/clients/openframe-client/src/services/tool_run_manager.rs b/clients/openframe-client/src/services/tool_run_manager.rs
index 7289615ec0..00538029bc 100644
--- a/clients/openframe-client/src/services/tool_run_manager.rs
+++ b/clients/openframe-client/src/services/tool_run_manager.rs
@@ -726,9 +726,6 @@ impl ToolRunManager {
 
         tokio::spawn(async move {
             let mut launch_backoff = FailureLogBackoff::new();
-            // Only on the first pass, and again after a supervised child exits. A launch
-            // retry must not re-kill: a GuiApp that needs longer than the 3s verify window
-            // would be killed by the next iteration, forever.
             let mut kill_leftovers_now = true;
             loop {
                 // Self-update in progress — stop the loop entirely
@@ -761,8 +758,7 @@ impl ToolRunManager {
 
                 let log_attempt = launch_backoff.should_log();
 
-                // A leftover that will not die is a retryable launch failure, not a fatal
-                // one. Windows GUI apps belong to the HKLM Run autorun — never kill them.
+                // Windows GUI apps belong to the HKLM Run autorun — never kill them.
                 #[cfg(target_os = "windows")]
                 let kill_leftovers = !installation.is_gui_app();
                 #[cfg(not(target_os = "windows"))]
@@ -771,8 +767,6 @@ impl ToolRunManager {
                 if kill_leftovers && kill_leftovers_now {
                     if let Err(e) = tool_kill_service.stop_tool(&tool.tool_agent_id).await {
                         let failures = launch_backoff.record_failure(log_attempt);
-                        // Escalate the retry delay, not just the logging: each attempt is a
-                        // full process-table scan and a wedged process only clears on reboot.
                         let delay =
                             (RETRY_DELAY_SECONDS * failures).min(KILL_RETRY_MAX_DELAY_SECONDS);
                         if log_attempt {
@@ -895,8 +889,6 @@ impl ToolRunManager {
                                     let prefs = crate::platform::preferences_writer::args_to_pairs(
                                         &processed_args,
                                     );
-                                    // Preferences are the only channel GuiApp args travel on
-                                    // macOS; launching without them starts an unconfigured app.
                                     if let Err(e) =
                                         crate::platform::preferences_writer::write(bid, prefs)
                                     {
@@ -1067,9 +1059,7 @@ impl ToolRunManager {
     }
 }
 
-/// Clears the updating flag on drop (surviving cancellation and panic), releasing the tool
-/// lock only once the flag is clear. Used by the update and restart paths; install and
-/// uninstall still pair mark/clear by hand.
+/// Clears the updating flag on drop, releasing the tool lock only once the flag is clear.
 pub struct UpdatingGuard {
     tool_run_manager: ToolRunManager,
     tool_agent_id: String,