fix(agent): SPEC-018 review fixes — agent_id persistence, managed fallback, HKEY typing

Address the SPEC-018 Phase 1 code review (reports/2026-06-03-spec018-review.md):

- Bug 2 (config.rs): stop agent_id churn on every restart. The embedded-config
  path always wins in Config::load, so the saved agent_id was never read back.
  Add Config::persisted_agent_id() and reuse a prior id from the TOML; only mint
  a new UUID when none exists.
- Bug 1 (main.rs): remove the non-functional in-process fallback in
  run_permanent_agent_managed. A managed agent's cak_ store is SYSTEM-only ACL'd,
  so a non-elevated in-process run cannot authenticate (load_cak permission-denied,
  or enroll C1 read-back failure). Return an actionable "install elevated" error
  instead of pretending to provide an agent; update the misleading comments.
- Issue 6 (startup.rs): replace the fragile transmute::<HANDLE, HKEY> with the
  windows crate's typed HKEY out-param; add SAFETY comments.

cargo check -p guruconnect --target x86_64-pc-windows-msvc passes clean.
Deferred lower-severity items tracked in #8.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

agent/src/config.rs

@@ -377,6 +377,26 @@ impl Config {
         false
     }
 
+    /// Best-effort read of a previously-persisted `agent_id` from the on-disk
+    /// TOML at [`Self::config_path`].
+    ///
+    /// The embedded blob never carries an `agent_id` (it is minted at first
+    /// run), so for a managed agent the only stable source across restarts is
+    /// the TOML that a prior run wrote via [`Self::save`]. Returns `Some(id)`
+    /// only when the file exists, parses, and contains a non-empty `agent_id`;
+    /// any missing-file / read / parse error yields `None` so the caller falls
+    /// back to generating a fresh id.
+    fn persisted_agent_id() -> Option<String> {
+        let config_path = Self::config_path();
+        let contents = std::fs::read_to_string(&config_path).ok()?;
+        let parsed: Config = toml::from_str(&contents).ok()?;
+        if parsed.agent_id.is_empty() {
+            None
+        } else {
+            Some(parsed.agent_id)
+        }
+    }
+
     /// Load configuration from embedded config, file, or environment
     pub fn load() -> Result<Self> {
         // Priority 1: Try loading from embedded config
@@ -389,7 +409,12 @@ impl Config {
                 api_key: embedded.api_key.unwrap_or_default(),
                 enrollment_key: embedded.enrollment_key,
                 site_code: embedded.site_code,
-                agent_id: generate_agent_id(),
+                // The embedded blob carries no agent_id, and load() always
+                // prefers this embedded path — so a freshly generated id would
+                // never be read back, churning the agent_id on every restart.
+                // Reuse the id a prior run persisted to the TOML if present;
+                // only mint a new one when none exists yet.
+                agent_id: Self::persisted_agent_id().unwrap_or_else(generate_agent_id),
                 hostname_override: None,
                 company: embedded.company,
                 site: embedded.site,
@@ -401,9 +426,11 @@ impl Config {
                 encoding: EncodingConfig::default(),
             };
 
-            // Save to file for persistence (so agent_id is preserved). The
-            // #[serde(skip)] enrollment fields are intentionally NOT written to
-            // the on-disk TOML — they are install-time material only.
+            // Persist so a freshly-minted agent_id is available to read back on
+            // the next launch (the embedded path always wins, so the TOML is the
+            // only place the stable id can live). The #[serde(skip)] enrollment
+            // fields are intentionally NOT written to the on-disk TOML — they are
+            // install-time material only.
             let _ = config.save();
             return Ok(config);
         }

agent/src/main.rs

@@ -475,9 +475,11 @@ pub fn run_managed_agent_service(
 ///     agent — it exits quietly.
 ///   - On first run, install (which installs + starts the service and removes the
 ///     legacy `HKCU\…\Run` autostart), then exit and let the service carry the
-///     agent. If the service install fails (e.g. not elevated), fall back to
-///     running the agent in-process for this run so the machine is not left with no
-///     agent at all.
+///     agent. The managed install REQUIRES elevation: the per-machine credential
+///     store is SYSTEM-only, so the SPEC-016 enrollment path cannot authenticate
+///     from a non-elevated, in-process context. There is therefore no in-process
+///     fallback — if the install fails, we return an actionable error telling the
+///     operator to re-run as Administrator.
 #[cfg(windows)]
 fn run_permanent_agent_managed() -> Result<()> {
     if service::is_service_installed() {
@@ -490,10 +492,23 @@ fn run_permanent_agent_managed() -> Result<()> {
 
     info!("First run - installing managed agent service");
     if let Err(e) = install::install(false) {
-        warn!(
-            "Managed service install failed ({e:#}); falling back to in-process agent for this run"
+        // No in-process fallback: a managed agent authenticates with a per-machine
+        // cak_ whose credential store is ACL'd to SYSTEM only. Running the agent in
+        // this non-elevated process would either fail to read an existing cak_
+        // (permission denied against the SYSTEM-only ACL) or, on a fresh machine,
+        // fail enrollment's C1 store-and-read-back verification — leaving the
+        // machine with no working agent while pretending otherwise. Surface a clear,
+        // actionable error instead.
+        error!(
+            "Managed agent install failed ({e:#}). The managed service must be installed \
+             elevated (Administrator) — the per-machine credential store is SYSTEM-only and \
+             an in-process fallback cannot authenticate. Re-run as Administrator."
         );
-        return run_agent_mode(None);
+        return Err(anyhow::anyhow!(
+            "managed agent install failed ({e:#}); the managed service must be installed \
+             elevated (Administrator) — the per-machine credential store is SYSTEM-only and \
+             an in-process fallback cannot authenticate. Re-run as Administrator."
+        ));
     }
 
     info!("Managed agent service installed; handing off to the service");

agent/src/startup.rs

@@ -9,7 +9,7 @@ use tracing::{info, warn};
 use windows::core::PCWSTR;
 #[cfg(windows)]
 use windows::Win32::System::Registry::{
-    RegCloseKey, RegDeleteValueW, RegOpenKeyExW, RegSetValueExW, HKEY_CURRENT_USER, KEY_WRITE,
+    RegCloseKey, RegDeleteValueW, RegOpenKeyExW, RegSetValueExW, HKEY, HKEY_CURRENT_USER, KEY_WRITE,
     REG_SZ,
 };
 
@@ -42,40 +42,39 @@ pub fn add_to_startup() -> Result<()> {
         .chain(std::iter::once(0))
         .collect();
 
+    // SAFETY: FFI into the Win32 registry API. `key_path`/`value_name`/`value_data`
+    // are NUL-terminated wide strings that outlive the calls. `RegOpenKeyExW`
+    // writes the opened key into `hkey`; we only use it after confirming success,
+    // and always pair it with `RegCloseKey`.
     unsafe {
-        let mut hkey = windows::Win32::Foundation::HANDLE::default();
+        let mut hkey = HKEY::default();
 
-        // Open the Run key
+        // Open the Run key. RegOpenKeyExW takes a `*mut HKEY` out-param.
         let result = RegOpenKeyExW(
             HKEY_CURRENT_USER,
             PCWSTR(key_path.as_ptr()),
             0,
             KEY_WRITE,
-            &mut hkey as *mut _ as *mut _,
+            &mut hkey,
         );
 
         if result.is_err() {
             anyhow::bail!("Failed to open registry key: {:?}", result);
         }
 
-        let hkey_raw = std::mem::transmute::<
-            windows::Win32::Foundation::HANDLE,
-            windows::Win32::System::Registry::HKEY,
-        >(hkey);
-
         // Set the value
         let data_bytes =
             std::slice::from_raw_parts(value_data.as_ptr() as *const u8, value_data.len() * 2);
 
         let set_result = RegSetValueExW(
-            hkey_raw,
+            hkey,
             PCWSTR(value_name.as_ptr()),
             0,
             REG_SZ,
             Some(data_bytes),
         );
 
-        let _ = RegCloseKey(hkey_raw);
+        let _ = RegCloseKey(hkey);
 
         if set_result.is_err() {
             anyhow::bail!("Failed to set registry value: {:?}", set_result);
@@ -103,15 +102,19 @@ pub fn remove_from_startup() -> Result<()> {
         .chain(std::iter::once(0))
         .collect();
 
+    // SAFETY: FFI into the Win32 registry API. `key_path`/`value_name` are
+    // NUL-terminated wide strings that outlive the calls. `RegOpenKeyExW` writes
+    // the opened key into `hkey`; we only use it after confirming success, and
+    // always pair it with `RegCloseKey`.
     unsafe {
-        let mut hkey = windows::Win32::Foundation::HANDLE::default();
+        let mut hkey = HKEY::default();
 
         let result = RegOpenKeyExW(
             HKEY_CURRENT_USER,
             PCWSTR(key_path.as_ptr()),
             0,
             KEY_WRITE,
-            &mut hkey as *mut _ as *mut _,
+            &mut hkey,
         );
 
         if result.is_err() {
@@ -119,14 +122,9 @@ pub fn remove_from_startup() -> Result<()> {
             return Ok(()); // Not an error if key doesn't exist
         }
 
-        let hkey_raw = std::mem::transmute::<
-            windows::Win32::Foundation::HANDLE,
-            windows::Win32::System::Registry::HKEY,
-        >(hkey);
+        let delete_result = RegDeleteValueW(hkey, PCWSTR(value_name.as_ptr()));
 
-        let delete_result = RegDeleteValueW(hkey_raw, PCWSTR(value_name.as_ptr()));
-
-        let _ = RegCloseKey(hkey_raw);
+        let _ = RegCloseKey(hkey);
 
         if delete_result.is_err() {
             warn!("Registry value may not exist: {:?}", delete_result);