spec: add SPEC-019 private Backstage session (GUI private desktop for interactive uninstall)

Extends SPEC-013 backstage from terminal-only to a private GUI desktop so a tech
can drive a stubborn uninstaller's UI invisibly to the logged-on user. Builds on
SPEC-018 broker; deep-linked from GuruRMM SPEC-030 'needs remote removal' flag.

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/credential_store.rs

@@ -146,7 +146,7 @@ pub fn store_cak(cak: &str) -> Result<()> {
             "[ENROLL] wrote the credential store but cannot read it back in THIS security \
              context (access denied). The store is ACL'd to SYSTEM + Administrators by \
              design; the managed agent must run as the GuruConnect SYSTEM service (see \
-             SPEC-017) to read it. Refusing to leave an unreadable store behind."
+             SPEC-018) to read it. Refusing to leave an unreadable store behind."
         )),
         Err(e) => Err(anyhow::Error::new(e)
             .context("stored the credential but the immediate read-back verification failed")),

agent/src/install.rs

@@ -290,6 +290,18 @@ pub fn install(force_user_install: bool) -> Result<()> {
     // Register protocol handler
     register_protocol_handler(elevated)?;
 
+    // SPEC-018: a MANAGED install (embedded config => persistent agent) installs
+    // the LocalSystem service as its single autostart and removes the per-user
+    // HKCU\…\Run entry. Attended (support-code) and viewer installs are untouched:
+    // they have no embedded config and continue to use the HKCU Run / protocol
+    // handler paths exactly as before.
+    #[cfg(windows)]
+    {
+        if crate::config::Config::has_embedded_config() {
+            install_managed_service(&exe_path)?;
+        }
+    }
+
     info!("Installation complete!");
     if elevated {
         info!("Installed system-wide to: {}", install_path.display());
@@ -300,6 +312,64 @@ pub fn install(force_user_install: bool) -> Result<()> {
     Ok(())
 }
 
+/// SPEC-018: install the managed agent as a LocalSystem service and swap out the
+/// legacy per-user `HKCU\…\Run` autostart so the service is the single managed
+/// autostart (no double-run).
+///
+/// Installing a LocalSystem service requires Administrator. If the SCM rejects the
+/// create (not elevated), we surface the error rather than silently leaving the
+/// machine with no managed autostart — a managed deployment is expected to run the
+/// install elevated. The HKCU Run entry is removed best-effort regardless.
+#[cfg(windows)]
+pub fn install_managed_service(exe_path: &std::path::Path) -> Result<()> {
+    info!("Managed install: registering LocalSystem service (SPEC-018)");
+
+    crate::service::install_service(exe_path)
+        .map_err(|e| anyhow!("failed to install the managed agent service: {e:#}"))?;
+
+    // Start the service now so the agent comes up immediately on first install
+    // rather than only on the next boot. Best-effort: the service is auto-start, so
+    // a transient start failure still self-heals on reboot.
+    if let Err(e) = crate::service::start_service() {
+        warn!(
+            "managed service installed but did not start now ({e:#}); \
+             it is auto-start and will run on next boot"
+        );
+    }
+
+    // Remove the legacy per-user autostart so the agent does not also launch in the
+    // user's session (which would double-run alongside the service).
+    if let Err(e) = crate::startup::remove_from_startup() {
+        warn!(
+            "managed service installed, but failed to remove the legacy HKCU Run \
+             autostart (harmless if it was never present): {}",
+            e
+        );
+    } else {
+        info!("removed legacy HKCU Run autostart (service is now the managed autostart)");
+    }
+
+    Ok(())
+}
+
+/// SPEC-018: remove the managed agent service and any legacy HKCU Run autostart.
+/// Idempotent — succeeds if neither is present.
+#[cfg(windows)]
+pub fn uninstall_managed_service() -> Result<()> {
+    info!("Managed uninstall: removing LocalSystem service (SPEC-018)");
+
+    // Best-effort removal of the legacy autostart first (cheap, no SCM).
+    if let Err(e) = crate::startup::remove_from_startup() {
+        warn!(
+            "failed to remove legacy HKCU Run autostart during uninstall: {}",
+            e
+        );
+    }
+
+    crate::service::uninstall_service()
+        .map_err(|e| anyhow!("failed to uninstall the managed agent service: {e:#}"))
+}
+
 /// Check if the guruconnect:// protocol handler is registered
 #[cfg(windows)]
 pub fn is_protocol_handler_registered() -> bool {

agent/src/main.rs

@@ -24,6 +24,8 @@ mod identity;
 mod input;
 mod install;
 mod sas_client;
+#[cfg(windows)]
+mod service;
 mod session;
 mod startup;
 mod transport;
@@ -182,6 +184,12 @@ enum Commands {
     /// Show detailed version and build information
     #[command(name = "version-info")]
     VersionInfo,
+
+    /// Internal: entry point invoked by the Windows Service Control Manager to run
+    /// the managed agent as a LocalSystem service (SPEC-018). Not for interactive
+    /// use — running it by hand fails because there is no controlling SCM.
+    #[command(name = "service-run", hide = true)]
+    ServiceRun,
 }
 
 fn main() -> Result<()> {
@@ -226,7 +234,24 @@ fn main() -> Result<()> {
         Some(Commands::Install {
             user_only,
             elevated,
-        }) => run_install(user_only || elevated),
+        }) => {
+            // `run_install`'s parameter is `force_user_install` — when true it
+            // skips the UAC re-elevation attempt and installs in-place with
+            // whatever rights this process already has.
+            //
+            // - `user_only`: the user explicitly asked for a per-user install;
+            //   honour it directly.
+            // - `elevated`: this is the internal, already-elevated re-exec spawned
+            //   by `try_elevate_and_install` ("install --elevated"). It must NOT
+            //   attempt to elevate AGAIN (that would loop / re-prompt), so we pass
+            //   force=true here too. This is correct even though it routes through
+            //   the "user install" parameter, because the re-exec genuinely runs
+            //   elevated: `is_elevated()` returns true inside `install()`, so the
+            //   path resolves to Program Files and the LocalSystem service installs
+            //   normally. The flag only suppresses re-elevation; it does not force a
+            //   per-user (non-elevated) install when we are already elevated.
+            run_install(user_only || elevated)
+        }
         Some(Commands::Uninstall) => run_uninstall(),
         Some(Commands::Launch { url }) => run_launch(&url),
         Some(Commands::VersionInfo) => {
@@ -236,6 +261,21 @@ fn main() -> Result<()> {
             println!("{}", build_info::full_version());
             Ok(())
         }
+        Some(Commands::ServiceRun) => {
+            // SPEC-018 Phase 1: SCM-invoked entry. Hand off to the service
+            // dispatcher, which calls back into the control loop and runs the
+            // managed-agent logic as SYSTEM. Blocks until the service stops.
+            #[cfg(windows)]
+            {
+                service::run_dispatcher()
+            }
+            #[cfg(not(windows))]
+            {
+                Err(anyhow::anyhow!(
+                    "service-run is a Windows-only entry point (SPEC-018)"
+                ))
+            }
+        }
         None => {
             // No subcommand - detect mode from filename or embedded config
             // Legacy: if support_code arg provided, use that
@@ -264,16 +304,31 @@ fn main() -> Result<()> {
                     run_agent_mode(Some(code))
                 }
                 RunMode::PermanentAgent => {
-                    // Embedded config found - run as permanent agent
+                    // Embedded config found - managed/persistent agent.
                     info!("Permanent agent mode detected (embedded config)");
-                    if !install::is_protocol_handler_registered() {
-                        // First run - install then run as agent
-                        info!("First run - installing agent");
-                        if let Err(e) = install::install(false) {
-                            warn!("Installation failed: {}", e);
-                        }
+
+                    // SPEC-018: managed mode runs as the LocalSystem service, not as
+                    // an interactive process. The service is the single autostart.
+                    //   - If the service is already installed, the service is (or
+                    //     will be) running the agent — this interactive invocation
+                    //     must NOT spawn a second agent. Exit quietly.
+                    //   - On first run, install (which installs + starts the service
+                    //     and removes the legacy HKCU Run entry), then exit and let
+                    //     the service carry the agent as SYSTEM.
+                    #[cfg(windows)]
+                    {
+                        run_permanent_agent_managed()
+                    }
+                    #[cfg(not(windows))]
+                    {
+                        if !install::is_protocol_handler_registered() {
+                            info!("First run - installing agent");
+                            if let Err(e) = install::install(false) {
+                                warn!("Installation failed: {}", e);
+                            }
+                        }
+                        run_agent_mode(None)
                     }
-                    run_agent_mode(None)
                 }
                 RunMode::Default => {
                     // No special mode detected - use legacy logic
@@ -333,10 +388,133 @@ fn run_agent_mode(support_code: Option<String>) -> Result<()> {
         if config.support_code.is_none() {
             resolve_agent_credential(&mut config).await?;
         }
-        run_agent(config).await
+        run_agent(config, None).await
     })
 }
 
+/// SPEC-018 Phase 1: run the managed/persistent agent as the LocalSystem service.
+///
+/// Invoked from the service control loop ([`service::run_service`]) once the
+/// service has reported `Running`. This is the same persistent-agent logic as
+/// [`run_agent_mode`] (load config, resolve/enroll the per-machine `cak_` per
+/// SPEC-016, hold the relay connection) — but it runs **as SYSTEM**, so the
+/// SYSTEM-ACL'd `cak_` store is finally readable in-context, and it observes the
+/// SCM `shutdown` flag for a graceful stop.
+///
+/// Returns `Ok(())` when the agent loop exits because a stop was requested, and
+/// `Err` only on an unrecoverable *local* fault (e.g. no usable credential and no
+/// enrollment material) — network errors are retried inside the loop and never
+/// surface here.
+///
+/// Phase 2 seam: this is where the session broker is wired in — the runtime
+/// started here will own the broker that spawns the per-session capture/input
+/// worker (`CreateProcessAsUserW`) and the IPC server. Phase 1 connects/enrolls
+/// only; it does not capture a desktop (a Session-0 SYSTEM process cannot).
+#[cfg(windows)]
+pub fn run_managed_agent_service(
+    shutdown: std::sync::Arc<std::sync::atomic::AtomicBool>,
+) -> Result<()> {
+    info!("Loading managed-agent configuration (running as SYSTEM)");
+
+    let mut config = config::Config::load()?;
+    // The service ONLY ever runs the managed/persistent path. A support session is
+    // an interactive, user-launched flow and must never be carried by the service.
+    config.support_code = None;
+
+    info!("Server: {}", config.server_url);
+    if let Some(ref company) = config.company {
+        info!("Company: {}", company);
+    }
+    if let Some(ref site) = config.site {
+        info!("Site: {}", site);
+    }
+
+    let rt = tokio::runtime::Runtime::new()?;
+
+    // SPEC-018 (finding M): this future runs across the `extern "system"` service
+    // entry point (ffi_service_main -> service_main -> run_service -> here). A
+    // panic that unwound across that FFI boundary is undefined behaviour (the C
+    // ABI cannot carry a Rust unwind) and would abort the process instead of
+    // taking the intended ServiceSpecific(1) fault path. Catch it here and convert
+    // it into an `Err`, which `run_service` maps to ServiceExitCode::ServiceSpecific(1)
+    // so the SCM applies its configured recovery (restart) cleanly. `Running` is
+    // already reported before we get here, so a fault does not strand StartPending.
+    let outcome = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| {
+        rt.block_on(async move {
+            // SPEC-016 Phase B: resolve the operating credential before connecting.
+            // Running as SYSTEM, the SYSTEM+Administrators-ACL'd cak_ store is now
+            // readable in-context, so the Phase B fail-fast guard is not hit on this
+            // path (it remains as a safety net for any non-SYSTEM invocation).
+            resolve_agent_credential(&mut config).await?;
+            run_agent(config, Some(shutdown)).await
+        })
+    }));
+
+    match outcome {
+        Ok(result) => result,
+        Err(panic) => {
+            // Recover a human-readable message from the panic payload for the log;
+            // do not re-panic (that would unwind across the FFI boundary again).
+            let detail = panic
+                .downcast_ref::<&str>()
+                .map(|s| s.to_string())
+                .or_else(|| panic.downcast_ref::<String>().cloned())
+                .unwrap_or_else(|| "non-string panic payload".to_string());
+            error!("managed-agent runtime panicked: {detail}");
+            Err(anyhow::anyhow!("managed-agent runtime panicked: {detail}"))
+        }
+    }
+}
+
+/// SPEC-018 Phase 1: handle an interactive launch of a MANAGED agent binary (one
+/// carrying embedded config, detected as [`config::RunMode::PermanentAgent`]).
+///
+/// Managed mode runs as the LocalSystem service, never as an interactive process:
+///   - If the service is already installed, the service is (or will be) running
+///     the agent as SYSTEM, so this interactive invocation must NOT spawn a second
+///     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. 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() {
+        info!(
+            "Managed service already installed; the service runs the agent as SYSTEM — \
+             this interactive instance has nothing to do"
+        );
+        return Ok(());
+    }
+
+    info!("First run - installing managed agent service");
+    if let Err(e) = install::install(false) {
+        // 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 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");
+    Ok(())
+}
+
 /// Resolve the per-machine operating credential for a managed agent (SPEC-016
 /// Phase B, run-mode wiring).
 ///
@@ -372,9 +550,13 @@ async fn resolve_agent_credential(config: &mut config::Config) -> Result<()> {
             // do NOT silently re-enroll. The SYSTEM+Administrators ACL is correct
             // for the target (Option A) and is deliberately kept.
             //
-            // NOTE: this guard is satisfied/removed once the GuruConnect SYSTEM
-            // service host lands (separate spec, SPEC-017) and the agent always
-            // runs as SYSTEM — at which point the store is always readable.
+            // SPEC-018 (this spec): the managed agent now runs as the GuruConnect
+            // SYSTEM service ([`run_managed_agent_service`]), so on the production
+            // managed path the store IS readable in-context and this branch is NOT
+            // hit. The guard is intentionally retained as a harmless safety net for
+            // any non-SYSTEM invocation (e.g. someone running the managed binary
+            // interactively): it still fails fast with an actionable message rather
+            // than bricking. Do NOT remove it in Phase 1.
             Err(LoadCakError::Io {
                 permission_denied: true,
                 source,
@@ -382,7 +564,7 @@ async fn resolve_agent_credential(config: &mut config::Config) -> Result<()> {
                 return Err(anyhow::anyhow!(
                     "[ENROLL] credential store is not accessible in this context \
                      ({source}) — the managed agent must run as the GuruConnect SYSTEM \
-                     service (see SPEC-017). Refusing to re-enroll."
+                     service (see SPEC-018). Refusing to re-enroll."
                 ));
             }
             // M1 — other IO error reaching the store (not access-denied): also
@@ -484,7 +666,22 @@ fn run_install(force_user_install: bool) -> Result<()> {
 fn run_uninstall() -> Result<()> {
     info!("Uninstalling GuruConnect...");
 
-    // Remove from startup
+    // SPEC-018: remove the managed LocalSystem service and the legacy HKCU Run
+    // autostart. Idempotent — no error if the service was never installed (an
+    // attended/viewer install has no service), so this is safe for every install
+    // shape. Requires Administrator to delete the service; a non-elevated uninstall
+    // still clears the per-user autostart below.
+    #[cfg(windows)]
+    {
+        if let Err(e) = install::uninstall_managed_service() {
+            warn!(
+                "Failed to remove managed service (may require Administrator): {}",
+                e
+            );
+        }
+    }
+
+    // Remove from startup (covers non-elevated / attended / viewer installs).
     if let Err(e) = startup::remove_from_startup() {
         warn!("Failed to remove from startup: {}", e);
     }
@@ -582,31 +779,62 @@ fn cleanup_on_exit() {
     }
 }
 
-/// Run the agent main loop
-async fn run_agent(config: config::Config) -> Result<()> {
+/// Run the agent main loop.
+///
+/// `service_shutdown`, when present, is the SCM cooperative-stop flag (SPEC-018):
+/// the managed-agent service passes it so the loop exits promptly on
+/// `Stop`/`Shutdown`. It is `None` for the interactive/user-launched paths, which
+/// stop via the tray exit / server control messages instead.
+async fn run_agent(
+    config: config::Config,
+    service_shutdown: Option<std::sync::Arc<std::sync::atomic::AtomicBool>>,
+) -> Result<()> {
+    use std::sync::atomic::Ordering;
+
     let elevated = install::is_elevated();
+    let running_as_service = service_shutdown.is_some();
     let mut session = session::SessionManager::new(config.clone(), elevated);
     let is_support_session = config.support_code.is_some();
     let hostname = config.hostname();
 
-    // Add to startup
-    if let Err(e) = startup::add_to_startup() {
+    // Helper: has the SCM asked us to stop?
+    let stop_requested = |flag: &Option<std::sync::Arc<std::sync::atomic::AtomicBool>>| -> bool {
+        flag.as_ref()
+            .map(|f| f.load(Ordering::SeqCst))
+            .unwrap_or(false)
+    };
+
+    // Autostart persistence:
+    //   - As the SYSTEM service (SPEC-018), the SERVICE itself is the managed
+    //     autostart — do NOT write the per-user HKCU\…\Run entry (that would be a
+    //     second, redundant autostart, and writing it from SYSTEM lands in the
+    //     wrong hive). The service install/uninstall owns lifecycle.
+    //   - Interactive/user-launched runs keep the existing HKCU Run behavior.
+    if running_as_service {
+        info!("Running as the GuruConnect SYSTEM service; service is the autostart (skipping HKCU Run)");
+    } else if let Err(e) = startup::add_to_startup() {
         warn!("Failed to add to startup: {}", e);
     }
 
-    // Create tray icon
-    let tray = match tray::TrayController::new(
-        &hostname,
-        config.support_code.as_deref(),
-        is_support_session,
-    ) {
-        Ok(t) => {
-            info!("Tray icon created");
-            Some(t)
-        }
-        Err(e) => {
-            warn!("Failed to create tray icon: {}", e);
-            None
+    // A Session-0 SYSTEM service has no interactive desktop, so a tray icon is
+    // both impossible and meaningless there (SPEC-018 Phase 2 moves the user-facing
+    // surface into the per-session worker). Only create the tray off the service.
+    let tray = if running_as_service {
+        None
+    } else {
+        match tray::TrayController::new(
+            &hostname,
+            config.support_code.as_deref(),
+            is_support_session,
+        ) {
+            Ok(t) => {
+                info!("Tray icon created");
+                Some(t)
+            }
+            Err(e) => {
+                warn!("Failed to create tray icon: {}", e);
+                None
+            }
         }
     };
 
@@ -615,6 +843,12 @@ async fn run_agent(config: config::Config) -> Result<()> {
 
     // Connect to server and run main loop
     loop {
+        // SPEC-018: honour an SCM stop request before (re)connecting.
+        if stop_requested(&service_shutdown) {
+            info!("Service stop requested; exiting agent loop");
+            return Ok(());
+        }
+
         info!("Connecting to server...");
 
         if is_support_session {
@@ -636,11 +870,22 @@ async fn run_agent(config: config::Config) -> Result<()> {
                 }
 
                 if let Err(e) = session
-                    .run_with_tray(tray.as_ref(), chat_ctrl.as_ref())
+                    .run_with_tray(tray.as_ref(), chat_ctrl.as_ref(), service_shutdown.as_ref())
                     .await
                 {
                     let error_msg = e.to_string();
 
+                    // SPEC-018 (finding H): the connected session loop broke
+                    // because the SCM asked the service to stop. The loop already
+                    // closed the WebSocket cleanly; treat this as a graceful stop
+                    // (no reconnect) so the service transitions StopPending ->
+                    // Stopped. Only the service path can produce this (it is the
+                    // only caller that passes a shutdown flag).
+                    if error_msg.contains(session::SERVICE_STOP_SENTINEL) {
+                        info!("Service stop requested during session; exiting agent loop");
+                        return Ok(());
+                    }
+
                     if error_msg.contains("USER_EXIT") {
                         info!("Session ended by user");
                         cleanup_on_exit();
@@ -713,6 +958,47 @@ async fn run_agent(config: config::Config) -> Result<()> {
         }
 
         info!("Reconnecting in 5 seconds...");
-        tokio::time::sleep(tokio::time::Duration::from_secs(5)).await;
+        // SPEC-018: poll the SCM stop flag during the backoff so a service stop is
+        // honoured within ~250ms instead of waiting the full reconnect delay.
+        if service_shutdown.is_some() {
+            for _ in 0..20 {
+                if stop_requested(&service_shutdown) {
+                    info!("Service stop requested during reconnect backoff; exiting agent loop");
+                    return Ok(());
+                }
+                tokio::time::sleep(tokio::time::Duration::from_millis(250)).await;
+            }
+        } else {
+            tokio::time::sleep(tokio::time::Duration::from_secs(5)).await;
+        }
+    }
+}
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+    use clap::CommandFactory;
+
+    /// SPEC-018 finding N1: pin the clap subcommand name to the constant the SCM
+    /// is registered with. The service is installed with `SERVICE_RUN_ARG` as its
+    /// launch argument; when the SCM starts it, clap must route that exact token
+    /// into [`Commands::ServiceRun`]. If the `#[command(name = "service-run")]`
+    /// attribute and the constant ever drift apart, the SCM would start the binary
+    /// but clap would fail to match the subcommand and the process would fall
+    /// through to default (non-service) mode and exit. Asserting against the live
+    /// clap metadata (not a second string literal) makes that drift impossible.
+    #[test]
+    #[cfg(windows)]
+    fn service_run_subcommand_matches_scm_launch_arg() {
+        let cmd = Cli::command();
+        let has_matching_subcommand = cmd
+            .get_subcommands()
+            .any(|sc| sc.get_name() == service::SERVICE_RUN_ARG);
+        assert!(
+            has_matching_subcommand,
+            "no clap subcommand named '{}' (the SCM launch arg); the ServiceRun \
+             #[command(name = ...)] attribute drifted from service::SERVICE_RUN_ARG",
+            service::SERVICE_RUN_ARG
+        );
     }
 }

agent/src/service/mod.rs

@@ -0,0 +1,520 @@
+//! Windows SYSTEM service host for the managed GuruConnect agent (SPEC-018).
+//!
+//! # Phase 1 scope (this module)
+//!
+//! Phase 1 proves the *managed/persistent* agent can run as **LocalSystem** in
+//! the isolated Session 0 across reboots and at the login screen:
+//!
+//!   1. Register the agent with the Service Control Manager (SCM) and run, when
+//!      started, the **existing persistent-agent logic** (`RunMode::PermanentAgent`
+//!      path) *as SYSTEM* — i.e. resolve/enroll the per-machine `cak_` (SPEC-016,
+//!      now readable because the SYSTEM-ACL'd store is in-context) and hold the
+//!      relay WSS connection.
+//!   2. Report a correct service lifecycle to the SCM (`StartPending` ->
+//!      `Running` -> `StopPending` -> `Stopped`) and handle `Stop`/`Shutdown`
+//!      gracefully. The control handler sets a shared shutdown flag; the agent
+//!      runtime observes it both between reconnect attempts AND inside the
+//!      connected session loop (SPEC-018 finding H), so a stop received while a
+//!      session is live breaks out promptly, closes the WS connection cleanly,
+//!      and exits — rather than waiting for the SCM to force-kill.
+//!   3. Provide install/uninstall of the service (LocalSystem, auto-start, crash
+//!      recovery) so managed mode uses the service as its single autostart
+//!      instead of the per-user `HKCU\…\Run` entry.
+//!
+//! # Phase 2 (deliberately NOT built here — see SPEC-018 §Scope)
+//!
+//! A SYSTEM service lives in Session 0 and **cannot** capture or inject the
+//! interactive desktop directly. Phase 1 therefore enrolls and connects but does
+//! **NOT** capture a desktop yet. The following are Phase 2 and are intentionally
+//! absent; the seams where they attach are called out inline below:
+//!
+//!   - the **session broker** (`WTSEnumerateSessionsW` /
+//!     `WTSGetActiveConsoleSessionId` / `WTSQueryUserToken`),
+//!   - the **per-session capture/input worker** spawned via `CreateProcessAsUserW`
+//!     into `winsta0\default`,
+//!   - **service <-> worker IPC** (the per-session ACL'd named pipe), and
+//!   - **`SERVICE_CONTROL_SESSIONCHANGE`** reaction (logon/logoff/console-connect
+//!     retarget).
+//!
+//! Phase 1 registers the control handler for `Stop`/`Shutdown`/`Interrogate`
+//! only. When Phase 2 lands, the broker hangs off the same control handler
+//! (adding `SESSIONCHANGE`) and off the same agent runtime started here.
+
+#![cfg(windows)]
+
+use std::ffi::OsString;
+use std::sync::atomic::{AtomicBool, Ordering};
+use std::sync::Arc;
+use std::time::Duration;
+
+use anyhow::{Context, Result};
+use tracing::{error, info, warn};
+
+use windows_service::{
+    define_windows_service,
+    service::{
+        ServiceAccess, ServiceControl, ServiceControlAccept, ServiceErrorControl, ServiceExitCode,
+        ServiceInfo, ServiceStartType, ServiceState, ServiceStatus, ServiceType,
+    },
+    service_control_handler::{self, ServiceControlHandlerResult},
+    service_dispatcher,
+    service_manager::{ServiceManager, ServiceManagerAccess},
+};
+
+/// Internal service name registered with the SCM (no spaces; used by `sc`,
+/// `ServiceManager`, and the control handler).
+pub const SERVICE_NAME: &str = "GuruConnectAgent";
+
+/// Human-facing display name shown in `services.msc`.
+pub const SERVICE_DISPLAY_NAME: &str = "GuruConnect Managed Agent";
+
+/// Service description shown in `services.msc`.
+pub const SERVICE_DESCRIPTION: &str =
+    "Runs the managed GuruConnect remote-support agent as LocalSystem so it is \
+     reachable at the login screen and across reboots (SPEC-018).";
+
+/// Hidden subcommand the SCM invokes to enter the service control loop. The
+/// service is registered with this as its launch argument (see [`install_service`]),
+/// and `main.rs` routes it into [`run_dispatcher`].
+pub const SERVICE_RUN_ARG: &str = "service-run";
+
+/// Hint we give the SCM for how long start/stop transitions may take before it
+/// should consider the service hung.
+const TRANSITION_WAIT: Duration = Duration::from_secs(10);
+
+// The `windows-service` dispatcher requires a `extern "system"` entry point with
+// a fixed ABI; this macro generates `ffi_service_main`, which trampolines into
+// our safe `service_main`.
+define_windows_service!(ffi_service_main, service_main);
+
+/// Enter the SCM dispatcher (called from `main.rs` for the `service-run`
+/// subcommand). Blocks until the service stops. This must be invoked by the SCM,
+/// not interactively — `service_dispatcher::start` fails with
+/// `ERROR_FAILED_SERVICE_CONTROLLER_CONNECT` (1063) if there is no controlling
+/// SCM, which is the expected outcome of running `guruconnect service-run` by hand.
+pub fn run_dispatcher() -> Result<()> {
+    service_dispatcher::start(SERVICE_NAME, ffi_service_main)
+        .context("failed to connect to the service control dispatcher (must be started by the SCM)")
+}
+
+/// SCM-invoked service body. Any error is logged; the function cannot return an
+/// error to the SCM directly, so [`run_service`] reports a failed exit code on the
+/// status handle before returning.
+fn service_main(_arguments: Vec<OsString>) {
+    if let Err(e) = run_service() {
+        error!("service exited with error: {e:#}");
+    }
+}
+
+/// Drive the full service lifecycle: register the control handler, report
+/// `Running`, run the persistent agent until a stop is requested, then report
+/// `Stopped`.
+fn run_service() -> Result<()> {
+    info!("GuruConnect managed agent service starting (running as SYSTEM in session 0)");
+
+    // Cooperative shutdown flag flipped by the SCM control handler and observed by
+    // the agent runtime. `AtomicBool` keeps the handler closure trivially `Send`
+    // and avoids holding a lock inside an SCM callback.
+    let shutdown = Arc::new(AtomicBool::new(false));
+    let shutdown_for_handler = shutdown.clone();
+
+    let event_handler = move |control_event| -> ServiceControlHandlerResult {
+        match control_event {
+            // SPEC-018 Phase 1: graceful stop. Phase 2 adds
+            // `ServiceControl::SessionChange(_)` here to drive the session broker
+            // (retarget the capture/input worker on logon/logoff/console-connect);
+            // we intentionally do not accept SESSIONCHANGE yet.
+            ServiceControl::Stop | ServiceControl::Shutdown => {
+                info!("received {control_event:?}; signalling agent to shut down");
+                // Set the cooperative-stop flag. The agent runtime observes it on
+                // every idle tick of the connected session loop and between
+                // reconnect attempts (SPEC-018 finding H), so it breaks out and
+                // closes the WebSocket cleanly within ~100ms even if a session is
+                // currently connected.
+                shutdown_for_handler.store(true, Ordering::SeqCst);
+                ServiceControlHandlerResult::NoError
+            }
+            ServiceControl::Interrogate => ServiceControlHandlerResult::NoError,
+            _ => ServiceControlHandlerResult::NotImplemented,
+        }
+    };
+
+    let status_handle = service_control_handler::register(SERVICE_NAME, event_handler)
+        .context("failed to register the service control handler")?;
+
+    // Report StartPending while we spin up the runtime and connect.
+    set_status(
+        &status_handle,
+        ServiceState::StartPending,
+        ServiceControlAccept::empty(),
+        TRANSITION_WAIT,
+    );
+
+    // Report Running and accept Stop + Shutdown. We report Running before the
+    // first connect attempt completes because the agent loop reconnects forever;
+    // "the service is up and trying" is the correct steady state, and blocking the
+    // SCM on the first relay handshake would risk a start timeout on a slow boot.
+    set_status(
+        &status_handle,
+        ServiceState::Running,
+        ServiceControlAccept::STOP | ServiceControlAccept::SHUTDOWN,
+        Duration::default(),
+    );
+    info!("service reported Running; entering managed-agent control loop");
+
+    // Run the existing persistent-agent logic as SYSTEM. This is the Phase 1
+    // payload: resolve/enroll the cak_ (SPEC-016) and hold the relay connection.
+    let run_result = crate::run_managed_agent_service(shutdown.clone());
+
+    if let Err(e) = &run_result {
+        // The agent loop only returns Err on an unrecoverable LOCAL fault (e.g. no
+        // usable credential and nothing to enroll with). Network errors are
+        // retried inside the loop and never surface here. Report the failure to
+        // the SCM so recovery actions (restart) engage.
+        error!("managed-agent control loop terminated with error: {e:#}");
+    } else {
+        info!("managed-agent control loop exited cleanly on stop request");
+    }
+
+    // Transition StopPending -> Stopped.
+    set_status(
+        &status_handle,
+        ServiceState::StopPending,
+        ServiceControlAccept::empty(),
+        TRANSITION_WAIT,
+    );
+
+    let exit_code = match run_result {
+        Ok(()) => ServiceExitCode::Win32(0),
+        // ERROR_SERVICE_SPECIFIC_ERROR-style: surface a non-zero service-specific
+        // code so the SCM treats the exit as a failure and applies recovery.
+        Err(_) => ServiceExitCode::ServiceSpecific(1),
+    };
+
+    set_status_with_exit(
+        &status_handle,
+        ServiceState::Stopped,
+        ServiceControlAccept::empty(),
+        Duration::default(),
+        exit_code,
+    );
+    info!("service reported Stopped");
+
+    Ok(())
+}
+
+/// Report a status with a zero (success) exit code.
+fn set_status(
+    handle: &service_control_handler::ServiceStatusHandle,
+    state: ServiceState,
+    accepted: ServiceControlAccept,
+    wait_hint: Duration,
+) {
+    set_status_with_exit(
+        handle,
+        state,
+        accepted,
+        wait_hint,
+        ServiceExitCode::Win32(0),
+    );
+}
+
+/// Report a status to the SCM. A failure to report is logged (best-effort) — we
+/// cannot do anything actionable about it and must not panic inside the service.
+fn set_status_with_exit(
+    handle: &service_control_handler::ServiceStatusHandle,
+    state: ServiceState,
+    accepted: ServiceControlAccept,
+    wait_hint: Duration,
+    exit_code: ServiceExitCode,
+) {
+    let status = ServiceStatus {
+        service_type: ServiceType::OWN_PROCESS,
+        current_state: state,
+        controls_accepted: accepted,
+        exit_code,
+        checkpoint: 0,
+        wait_hint,
+        process_id: None,
+    };
+    if let Err(e) = handle.set_service_status(status) {
+        warn!("failed to report service status {state:?} to the SCM: {e}");
+    }
+}
+
+// ---------------------------------------------------------------------------
+// Install / uninstall (used by install.rs for managed mode)
+// ---------------------------------------------------------------------------
+
+/// Install (or reinstall) the managed agent as a LocalSystem auto-start service
+/// pointing at `exe_path` with the [`SERVICE_RUN_ARG`] launch argument.
+///
+/// Idempotent: if the service already exists it is stopped and deleted first,
+/// then recreated, so an upgrade picks up a new binary path / config. Configures
+/// crash recovery (restart on failure) via `sc failure`.
+///
+/// Requires Administrator (SCM `CREATE_SERVICE`). Returns an error otherwise.
+pub fn install_service(exe_path: &std::path::Path) -> Result<()> {
+    let manager = ServiceManager::local_computer(
+        None::<&str>,
+        ServiceManagerAccess::CONNECT | ServiceManagerAccess::CREATE_SERVICE,
+    )
+    .context("failed to connect to the Service Control Manager (run as Administrator)")?;
+
+    // Remove any prior installation so the binary path / args are refreshed.
+    let mut deleted_existing = false;
+    if let Ok(existing) = manager.open_service(
+        SERVICE_NAME,
+        ServiceAccess::QUERY_STATUS | ServiceAccess::STOP | ServiceAccess::DELETE,
+    ) {
+        info!("existing {SERVICE_NAME} service found; removing before reinstall");
+        stop_if_running(&existing);
+        existing
+            .delete()
+            .context("failed to delete the existing service before reinstall")?;
+        drop(existing);
+        deleted_existing = true;
+    }
+
+    let service_info = ServiceInfo {
+        name: OsString::from(SERVICE_NAME),
+        display_name: OsString::from(SERVICE_DISPLAY_NAME),
+        service_type: ServiceType::OWN_PROCESS,
+        start_type: ServiceStartType::AutoStart,
+        error_control: ServiceErrorControl::Normal,
+        executable_path: exe_path.to_path_buf(),
+        launch_arguments: vec![OsString::from(SERVICE_RUN_ARG)],
+        dependencies: vec![],
+        // account_name: None => LocalSystem (the SPEC-018 requirement).
+        account_name: None,
+        account_password: None,
+    };
+
+    let service = create_service_with_retry(&manager, &service_info, deleted_existing)
+        .context("failed to create the GuruConnect managed agent service")?;
+
+    service
+        .set_description(SERVICE_DESCRIPTION)
+        .context("failed to set the service description")?;
+
+    configure_recovery();
+
+    info!(
+        "installed {SERVICE_NAME} (LocalSystem, auto-start) -> {} {}",
+        exe_path.display(),
+        SERVICE_RUN_ARG
+    );
+    Ok(())
+}
+
+/// Create the service, retrying briefly if the SCM still has the prior instance
+/// "marked for deletion" (SPEC-018 finding L1).
+///
+/// When a service is deleted, the SCM only removes it from its database once every
+/// open handle to it closes; until then a fresh `CreateService` fails with
+/// `ERROR_SERVICE_MARKED_FOR_DELETE` (1072). The previous implementation papered
+/// over this with a fixed 2s sleep after `delete()`, which is both slower than
+/// necessary in the common case and still racy on a busy box. Instead we attempt
+/// the create immediately and, only if we just deleted an existing instance and
+/// hit 1072, retry a few times with short backoff — succeeding as soon as the SCM
+/// finishes the removal, and giving up with the real error if it never does.
+///
+/// The retry is gated on `deleted_existing`: on a clean first install there was no
+/// prior instance, so a 1072 there is unexpected and is surfaced immediately
+/// rather than masked by retries.
+fn create_service_with_retry(
+    manager: &ServiceManager,
+    service_info: &ServiceInfo,
+    deleted_existing: bool,
+) -> Result<windows_service::service::Service, windows_service::Error> {
+    // ERROR_SERVICE_MARKED_FOR_DELETE (winerror.h). The service is gone from the
+    // caller's perspective but the SCM has not finished reaping it.
+    const ERROR_SERVICE_MARKED_FOR_DELETE: i32 = 1072;
+    // Bounded: ~5 attempts over ~2s total worst case (matches the old fixed sleep
+    // ceiling) but returns the instant the SCM is ready.
+    const MAX_ATTEMPTS: u32 = 5;
+    const BACKOFF: Duration = Duration::from_millis(400);
+
+    let mut attempt = 0;
+    loop {
+        attempt += 1;
+        match manager.create_service(service_info, ServiceAccess::CHANGE_CONFIG) {
+            Ok(service) => return Ok(service),
+            Err(windows_service::Error::Winapi(ref io_err))
+                if deleted_existing
+                    && io_err.raw_os_error() == Some(ERROR_SERVICE_MARKED_FOR_DELETE)
+                    && attempt < MAX_ATTEMPTS =>
+            {
+                warn!(
+                    "{SERVICE_NAME} still marked for deletion by the SCM \
+                     (attempt {attempt}/{MAX_ATTEMPTS}); retrying in {}ms",
+                    BACKOFF.as_millis()
+                );
+                std::thread::sleep(BACKOFF);
+            }
+            Err(e) => return Err(e),
+        }
+    }
+}
+
+/// Configure SCM crash-recovery so the service restarts on unexpected exit.
+///
+/// `windows-service` 0.7 does not expose `ChangeServiceConfig2` recovery actions
+/// in a stable, ergonomic form, so we mirror the established pattern used by the
+/// SAS service binary and shell out to `sc failure`. `reset=86400` clears the
+/// failure count after a day; three `restart/5000` actions retry after 5s each.
+fn configure_recovery() {
+    use std::os::windows::process::CommandExt;
+    const CREATE_NO_WINDOW: u32 = 0x0800_0000;
+
+    match std::process::Command::new("sc")
+        .args([
+            "failure",
+            SERVICE_NAME,
+            "reset=86400",
+            "actions=restart/5000/restart/5000/restart/5000",
+        ])
+        .creation_flags(CREATE_NO_WINDOW)
+        .output()
+    {
+        Ok(out) if out.status.success() => {
+            info!("configured crash-recovery (restart) for {SERVICE_NAME}");
+        }
+        Ok(out) => {
+            warn!(
+                "could not configure crash-recovery for {SERVICE_NAME} (sc failure exit {:?}); \
+                 the service will still run but will not auto-restart on crash",
+                out.status.code()
+            );
+        }
+        Err(e) => {
+            warn!("could not invoke `sc failure` to set crash-recovery for {SERVICE_NAME}: {e}");
+        }
+    }
+}
+
+/// Stop (if running) and delete the managed agent service. Idempotent: succeeds
+/// quietly if the service is not installed.
+pub fn uninstall_service() -> Result<()> {
+    let manager = ServiceManager::local_computer(None::<&str>, ServiceManagerAccess::CONNECT)
+        .context("failed to connect to the Service Control Manager (run as Administrator)")?;
+
+    match manager.open_service(
+        SERVICE_NAME,
+        ServiceAccess::QUERY_STATUS | ServiceAccess::STOP | ServiceAccess::DELETE,
+    ) {
+        Ok(service) => {
+            stop_if_running(&service);
+            service
+                .delete()
+                .context("failed to delete the managed agent service")?;
+            info!("uninstalled {SERVICE_NAME} service");
+            Ok(())
+        }
+        Err(_) => {
+            // Not installed — nothing to do (idempotent uninstall).
+            info!("{SERVICE_NAME} service is not installed; nothing to uninstall");
+            Ok(())
+        }
+    }
+}
+
+/// Start the managed agent service now (used right after a first-run install so
+/// the agent comes up without waiting for the next boot). Best-effort: logs and
+/// returns the SCM error if the start fails, but a failure is not fatal to install
+/// because the service is auto-start and will come up on the next boot regardless.
+pub fn start_service() -> Result<()> {
+    let manager = ServiceManager::local_computer(None::<&str>, ServiceManagerAccess::CONNECT)
+        .context("failed to connect to the Service Control Manager")?;
+    let service = manager
+        .open_service(
+            SERVICE_NAME,
+            ServiceAccess::START | ServiceAccess::QUERY_STATUS,
+        )
+        .context("failed to open the managed agent service to start it")?;
+
+    // If it is already running (e.g. reinstall-over-running), there is nothing to do.
+    if let Ok(status) = service.query_status() {
+        if status.current_state == ServiceState::Running
+            || status.current_state == ServiceState::StartPending
+        {
+            info!("{SERVICE_NAME} is already running/starting");
+            return Ok(());
+        }
+    }
+
+    service
+        .start::<String>(&[])
+        .context("failed to start the managed agent service")?;
+    info!("started {SERVICE_NAME}");
+    Ok(())
+}
+
+/// Report whether the managed agent service is currently installed.
+pub fn is_service_installed() -> bool {
+    match ServiceManager::local_computer(None::<&str>, ServiceManagerAccess::CONNECT) {
+        Ok(manager) => manager
+            .open_service(SERVICE_NAME, ServiceAccess::QUERY_STATUS)
+            .is_ok(),
+        Err(_) => false,
+    }
+}
+
+/// Best-effort stop of a service, waiting briefly for it to leave the running
+/// state so a subsequent `delete` does not race an in-flight stop.
+fn stop_if_running(service: &windows_service::service::Service) {
+    if let Ok(status) = service.query_status() {
+        if status.current_state != ServiceState::Stopped {
+            info!("stopping {SERVICE_NAME} before delete");
+            let _ = service.stop();
+            for _ in 0..10 {
+                std::thread::sleep(Duration::from_millis(500));
+                match service.query_status() {
+                    Ok(s) if s.current_state == ServiceState::Stopped => break,
+                    _ => continue,
+                }
+            }
+        }
+    }
+}
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    /// The launch argument the service is registered with MUST equal the hidden
+    /// `service-run` subcommand `main.rs` dispatches into [`run_dispatcher`]; a
+    /// mismatch would register a service the SCM could start but that would fall
+    /// through to normal (non-service) mode and immediately exit.
+    ///
+    /// This pins the value of the constant itself. The companion test
+    /// `tests::service_run_subcommand_matches_scm_launch_arg` in `main.rs` pins the
+    /// other half — that the clap `#[command(name = "service-run")]` attribute on
+    /// `Commands::ServiceRun` resolves to this same constant — so the two string
+    /// literals cannot silently drift apart.
+    #[test]
+    fn service_run_arg_matches_subcommand_name() {
+        assert_eq!(SERVICE_RUN_ARG, "service-run");
+    }
+
+    /// Service identifiers are non-empty and the internal name carries no spaces
+    /// (the SCM key / `sc` argument must be a single token).
+    #[test]
+    fn service_identifiers_are_well_formed() {
+        assert!(!SERVICE_NAME.is_empty());
+        assert!(
+            !SERVICE_NAME.contains(char::is_whitespace),
+            "the SCM service name must be a single whitespace-free token"
+        );
+        assert!(!SERVICE_DISPLAY_NAME.is_empty());
+        assert!(!SERVICE_DESCRIPTION.is_empty());
+    }
+
+    /// `is_service_installed` must never panic regardless of elevation/SCM access;
+    /// on a dev workstation without the service installed it returns `false`. (We
+    /// do NOT install the service in tests — that is a VM/admin integration step.)
+    #[test]
+    fn is_service_installed_is_total() {
+        let _ = is_service_installed();
+    }
+}

agent/src/session/mod.rs

@@ -41,8 +41,18 @@ use crate::proto::{message, AgentStatus, ChatMessage, Heartbeat, HeartbeatAck, M
 use crate::transport::WebSocketTransport;
 use crate::tray::{TrayAction, TrayController};
 use anyhow::Result;
+use std::sync::atomic::{AtomicBool, Ordering};
+use std::sync::Arc;
 use std::time::{Duration, Instant};
 
+/// Sentinel error string returned by [`SessionManager::run_with_tray`] when the
+/// loop breaks because the SCM asked the managed-agent service to stop (SPEC-018,
+/// finding H). The outer `run_agent` loop matches on this to treat the exit as a
+/// graceful service stop (clean WS close, no reconnect) rather than a session
+/// error. Only the service path passes a shutdown flag, so only the service path
+/// can ever produce this.
+pub const SERVICE_STOP_SENTINEL: &str = "SERVICE_STOP";
+
 // Heartbeat interval (30 seconds)
 const HEARTBEAT_INTERVAL: Duration = Duration::from_secs(30);
 // Status report interval (60 seconds)
@@ -285,16 +295,34 @@ impl SessionManager {
         Ok(())
     }
 
-    /// Run the session main loop with tray and chat event processing
+    /// Run the session main loop with tray and chat event processing.
+    ///
+    /// `service_shutdown` (SPEC-018 finding H) is the SCM cooperative-stop flag.
+    /// It is `Some(flag)` ONLY on the managed-agent service path; the
+    /// attended/viewer/interactive callers pass `None` and behave EXACTLY as
+    /// before. When present, the flag is polled on every idle tick (the natural
+    /// ~100ms seam below) so an SCM Stop/Shutdown received while CONNECTED breaks
+    /// this inner loop promptly — instead of only being observed by the outer
+    /// `run_agent` reconnect loop, which never runs while a session is connected.
+    /// On a set flag the loop closes the WebSocket cleanly (via the shared exit
+    /// path at the bottom) and returns the [`SERVICE_STOP_SENTINEL`] error, which
+    /// the outer loop maps to a graceful stop.
     pub async fn run_with_tray(
         &mut self,
         tray: Option<&TrayController>,
         chat: Option<&ChatController>,
+        service_shutdown: Option<&Arc<AtomicBool>>,
     ) -> Result<()> {
         if self.transport.is_none() {
             anyhow::bail!("Not connected");
         }
 
+        // Helper: has the SCM asked the service to stop? Always false off the
+        // service path (where `service_shutdown` is `None`).
+        let stop_requested = |flag: Option<&Arc<AtomicBool>>| -> bool {
+            flag.is_some_and(|f| f.load(Ordering::SeqCst))
+        };
+
         // Send initial status
         self.send_status().await?;
 
@@ -307,6 +335,29 @@ impl SessionManager {
 
         // Main loop
         loop {
+            // SPEC-018 (finding H): honour an SCM stop request received while the
+            // session is CONNECTED. The outer `run_agent` loop only observes the
+            // flag between connection attempts, but a managed agent spends its
+            // entire connected life inside THIS loop — so without this check an
+            // SCM Stop while connected would not break out until the connection
+            // dropped on its own. Breaking here falls through to the shared exit
+            // path below, which closes the transport cleanly (clean WS close);
+            // the sentinel tells the outer loop this was a graceful stop.
+            if stop_requested(service_shutdown) {
+                tracing::info!("Service stop requested; ending connected session loop");
+                self.release_streaming();
+                self.state = SessionState::Disconnected;
+                if let Some(transport) = self.transport.as_mut() {
+                    // Best-effort clean WebSocket close (sends a Close frame). A
+                    // failure here just means the peer/socket is already gone; the
+                    // service still stops cleanly.
+                    if let Err(e) = transport.close().await {
+                        tracing::warn!("error during clean WebSocket close on service stop: {}", e);
+                    }
+                }
+                return Err(anyhow::anyhow!(SERVICE_STOP_SENTINEL));
+            }
+
             // Process tray events
             if let Some(t) = tray {
                 if let Some(action) = t.process_events() {
@@ -745,3 +796,47 @@ impl SessionManager {
         Ok(())
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    /// SPEC-018 finding H: the connected-stop contract. When the SCM sets the
+    /// shutdown flag, `run_with_tray` returns an error whose message contains
+    /// [`SERVICE_STOP_SENTINEL`]; the outer `run_agent` loop recognises a graceful
+    /// stop with `error_msg.contains(SERVICE_STOP_SENTINEL)`. This pins that the
+    /// error the loop constructs on stop actually satisfies that match — so the
+    /// two halves (producer here, consumer in `main.rs`) cannot drift.
+    ///
+    /// A full end-to-end test of the in-loop interrupt would need a live connected
+    /// transport (a real or mocked server), which is an integration concern; this
+    /// unit test instead pins the wire contract the interrupt relies on.
+    #[test]
+    fn service_stop_sentinel_is_matched_by_outer_loop_check() {
+        let produced = anyhow::anyhow!(SERVICE_STOP_SENTINEL);
+        assert!(
+            produced.to_string().contains(SERVICE_STOP_SENTINEL),
+            "the stop error must contain the sentinel the outer loop matches on"
+        );
+        assert!(
+            !SERVICE_STOP_SENTINEL.is_empty(),
+            "the sentinel must be a non-empty, distinctive token"
+        );
+    }
+
+    /// The shutdown-flag check is a no-op (always `false`) when no flag is passed,
+    /// i.e. on the attended/viewer/interactive paths — guaranteeing the new
+    /// parameter is a pure addition that cannot alter non-service behaviour
+    /// (SPEC-018 finding H: "no regression").
+    #[test]
+    fn no_shutdown_flag_never_requests_stop() {
+        let none: Option<&Arc<AtomicBool>> = None;
+        let check = |flag: Option<&Arc<AtomicBool>>| flag.is_some_and(|f| f.load(Ordering::SeqCst));
+        assert!(!check(none));
+
+        let set = Arc::new(AtomicBool::new(true));
+        assert!(check(Some(&set)));
+        let unset = Arc::new(AtomicBool::new(false));
+        assert!(!check(Some(&unset)));
+    }
+}

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

docs/FEATURE_ROADMAP.md

@@ -68,7 +68,9 @@ Bringing GC to parity with GuruRMM's release engineering. Full plan: [SPEC-001](
 - [x] Protobuf-over-WSS transport, Zstd frame compression
 - [~] React/TS web viewer (`dashboard/src/components/RemoteViewer.tsx`) — embeddable session viewer
 - [ ] **Headless Linux mode (direct TTY access)** — P2 — Terminal-based remote access for Linux servers without GUI. PTY spawn (`openpty`), xterm.js web viewer, full ANSI/VT100 support. Enables server management, container debugging, emergency recovery via GuruConnect dashboard with audit logging. SSH replacement with centralized auth. ([SPEC-012](specs/SPEC-012-headless-linux-tty.md))
+- [ ] **Managed-agent SYSTEM service host + session broker** — P1 — convert the persistent agent from `HKCU Run` (user context) to a LocalSystem **service** that runs unattended (login screen, no user, across reboots) and spawns a per-session capture/input worker into the active desktop (Session 0 can't capture directly). Unblocks SPEC-016 Phase B end-to-end (the SYSTEM-ACL'd `cak_` store becomes readable; removes the Phase B fail-fast guard), enables true unattended access, and is the **broker primitive SPEC-013 builds on**. ([SPEC-018](specs/SPEC-018-managed-agent-service-host.md))
 - [ ] **Windows session selection and backstage mode** — P2 — Enumerate and switch between Windows user sessions (Terminal Services/RDP/Fast User Switching) and access Session 0 (backstage) for system-level admin tasks. ScreenConnect parity: session selector shows all logged-on users, instant switching without reconnect. Backstage mode provides terminal/command interface for services management without disrupting any user desktop. Critical for multi-user server environments. ([SPEC-013](specs/SPEC-013-session-selection-and-backstage.md))
+- [ ] **Private Backstage session — GUI private desktop (interactive uninstall)** — P2 — extend backstage (SPEC-013, terminal-only) to a private GUI **desktop**: run an arbitrary program (e.g. a stubborn uninstaller) on a hidden `CreateDesktop` desktop, capture it (GDI; DXGI can't target an off-screen desktop) and inject input bound to it, streamed only to the tech — the logged-on user sees nothing. Builds on SPEC-018 (service host/broker). Deep-linked from the GuruRMM "Needs remote removal" flag (RMM SPEC-030 Tier-2). ([SPEC-019](specs/SPEC-019-private-backstage-session.md))
 - [ ] **Configurable notification overlay on viewer connection** — P2 — Display a semi-transparent on-screen notification when a technician connects, showing technician name and company. Dashboard-configurable message template (supports `{{technician_name}}`, `{{company}}`, `{{time}}`), duration (5-60s), position (top-left/right, bottom-left/right, center), and dismissible behavior. Increases transparency and user awareness during remote support sessions. Compliance-friendly for privacy policies requiring user notification. ([SPEC-015](specs/SPEC-015-notification-overlay.md))
 - [ ] Multi-monitor switching — P2
 - [ ] File transfer — P3 (out of scope for native-remote-control v1)

docs/specs/SPEC-018-managed-agent-service-host.md

@@ -0,0 +1,146 @@
+# SPEC-018: Managed-Agent SYSTEM Service Host + Session Broker
+
+**Status:** Proposed
+**Priority:** P1 (blocks SPEC-016 Phase B end-to-end runtime and SPEC-013)
+**Requested By:** Mike (2026-06-02)
+**Estimated Effort:** X-Large
+
+## Overview
+
+Convert the managed/persistent GuruConnect agent from a user-context `HKCU\…\Run` autostart into a
+**Windows SYSTEM service** that runs unattended — at the login screen, with no user logged in, across
+reboots — and **brokers per-session capture/input worker processes** into the active interactive
+desktop. A SYSTEM service lives in the isolated **Session 0** and cannot capture or inject the
+interactive desktop directly, so the service spawns a worker into the target user session (the
+ScreenConnect architecture).
+
+This is foundational, not cosmetic. It unblocks three things at once:
+1. **SPEC-016 Phase B end-to-end runtime** — the per-machine `cak_` store is ACL'd to SYSTEM +
+   Administrators; today the agent runs as the interactive *user* and can't read its own store (the
+   Phase B C1 *fail-fast guard* exists precisely because of this). Running as SYSTEM makes the store
+   readable and removes the guard.
+2. **True unattended access** — a user-context agent only runs while that user is logged in. Reaching
+   a rebooted server or a machine sitting at the login screen (table-stakes for remote support)
+   requires SYSTEM.
+3. **SPEC-013 session selection / backstage** — the session-broker primitive built here is the
+   substrate SPEC-013's session-switching UX drives.
+
+**Success criteria:** the managed agent installs as an auto-start SYSTEM service; it holds the relay
+connection and performs SPEC-016 enrollment as SYSTEM (reading/writing the SYSTEM-ACL'd `cak_`); it
+spawns a capture/input worker into the active interactive session and relays frames; the worker is
+respawned/retargeted on logon/logoff/console-connect; and the Phase B fail-fast guard is removed
+because the store is now readable in-context.
+
+## Background — why this is needed (confirmed in code)
+
+- The persistent agent autostarts via `HKCU\…\Run` (`agent/src/startup.rs:21`, `STARTUP_KEY` = HKCU)
+  → interactive-user token, not SYSTEM. The only SYSTEM service today is the separate `sas_service`
+  (Secure Attention Sequence helper).
+- SPEC-016 Phase B (`agent/src/credential_store.rs`) ACLs the `cak_` store to `*S-1-5-18` (SYSTEM) +
+  `*S-1-5-32-544` (Administrators). In the current user context the agent writes but cannot read it
+  back → the Phase B fail-fast guard (`agent/src/main.rs` `resolve_agent_credential`) emits
+  "must run as the GuruConnect SYSTEM service (see SPEC-018)" instead of bricking.
+- Capture/input live in the agent process (`agent/src/capture/`, `agent/src/input/`); a Session-0
+  SYSTEM service cannot drive these against the interactive desktop without a per-session worker.
+
+## Scope
+
+### Included in v1
+
+1. **Windows service install/lifecycle** (`agent/src/install.rs` + a new service module): register the
+   managed agent as a **LocalSystem auto-start service** (`CreateServiceW` / a service crate),
+   configure failure/recovery (restart on crash), and **replace the HKCU `Run` autostart for managed
+   mode** (remove the Run entry on service install). Clean uninstall (stop + delete service).
+2. **Service control loop** (Session 0, SYSTEM): owns the persistent WSS connection to the relay,
+   performs SPEC-016 enrollment as SYSTEM (now able to read/write the `cak_` store), and dispatches
+   session/connect requests to workers. Handles `SERVICE_CONTROL_STOP`/`SHUTDOWN` and
+   `SERVICE_CONTROL_SESSIONCHANGE`.
+3. **Session broker:** enumerate sessions (`WTSEnumerateSessionsW`), resolve the active interactive
+   session (`WTSGetActiveConsoleSessionId`), obtain its user token (`WTSQueryUserToken` →
+   `DuplicateTokenEx`), and spawn a **per-session capture/input worker** into that session's desktop
+   (`CreateProcessAsUserW`, `winsta0\default`). The worker does DXGI capture + input injection in the
+   user's session; the service relays frames over the existing transport.
+4. **Service ↔ worker IPC:** a local, ACL'd channel (named pipe `\\.\pipe\guruconnect-<sessionId>`)
+   carrying frames/input/control; pipe ACL restricted to SYSTEM + the target session user.
+5. **Session-change handling:** on logon/logoff/console-connect/disconnect/lock/unlock, (re)spawn or
+   retarget the worker so the active desktop is always the one being served.
+6. **Remove the SPEC-016 Phase B fail-fast guard** once the service runs as SYSTEM (the store is
+   readable in-context); keep the SYSTEM+Administrators ACL.
+
+### Explicitly out of scope (anticipated, separate specs)
+
+- **Session-selection / backstage UX** — the operator-facing picker and Session-0/secure-desktop
+  command surface are **SPEC-013**; this spec only provides the broker primitive it drives.
+- **Login-screen / secure-desktop (winlogon) capture** beyond the broker hook — the hard
+  Secure-Desktop case is coordinated with SPEC-013; v1 here targets the active interactive session.
+- **macOS/Linux service equivalents** — future SPEC-010 (cross-platform agents).
+
+## Architecture
+
+- **Agent splits into two roles:**
+  - **service-host** (LocalSystem, Session 0): service lifecycle, relay transport, SPEC-016
+    enrollment + `cak_` store, session broker, IPC server.
+  - **session-worker** (per interactive session, user token): DXGI/GDI capture, input injection,
+    IPC client. Spawned by the service via `CreateProcessAsUserW`.
+- **Service install** (`install.rs`): `CreateServiceW` with `SERVICE_AUTO_START`, `SERVICE_WIN32_OWN_PROCESS`,
+  recovery actions; uninstall stops + deletes. Replaces managed-mode `HKCU Run`.
+- **Token handoff:** `WTSGetActiveConsoleSessionId` → `WTSQueryUserToken` → `DuplicateTokenEx`
+  (primary token) → `CreateProcessAsUserW` with `lpDesktop = "winsta0\\default"`.
+- **IPC:** named pipe per session, length-prefixed protobuf (reuse `proto/` message types where
+  sensible), pipe security descriptor granting only SYSTEM + the session user.
+- **Session events:** the service registers for `SERVICE_CONTROL_SESSIONCHANGE` and reacts to
+  `WTS_CONSOLE_CONNECT`, `WTS_SESSION_LOGON/LOGOFF`, `WTS_SESSION_LOCK/UNLOCK`.
+
+## Security considerations
+
+- **LocalSystem is maximal privilege** — minimize the service's attack surface; validate every
+  relay-delivered command; never spawn a worker except into a legitimately-enumerated active session.
+- **IPC pipe must be ACL'd** (SYSTEM + the specific session user only) so a non-admin user can't
+  inject capture/input commands by connecting to the pipe.
+- **Token hygiene:** close duplicated tokens promptly; don't leak SYSTEM or user primary tokens.
+- The SPEC-016 `cak_` store (SYSTEM-ACL'd) is now correctly readable; the fail-fast guard is removed
+  but the ACL stays.
+- **Audit:** service start/stop, enrollment-as-SYSTEM, worker spawn, session attach/retarget — written
+  to the existing event pipeline.
+
+## Implementation details
+
+- New service module (e.g. `agent/src/service/{mod.rs, broker.rs, ipc.rs}`); worker entry split out of
+  the current capture path. New `Commands` variants or an internal `--service`/`--session-worker`
+  dispatch in `agent/src/main.rs`.
+- `install.rs`: service create/recovery/delete; drop the managed-mode HKCU `Run` write.
+- `windows` crate features: `Win32_System_Services`, `Win32_System_RemoteDesktop`
+  (`WTS*`), `Win32_Security`, `Win32_System_Threading` (`CreateProcessAsUserW`),
+  `Win32_System_Pipes`.
+- Remove the `resolve_agent_credential` fail-fast guard branch added in SPEC-016 Phase B.
+
+## Testing strategy
+
+- **Service:** install → auto-start on boot → stop → uninstall on a clean VM.
+- **`cak_` end-to-end:** SYSTEM service enrolls (SPEC-016), stores + reads the `cak_`, connects — the
+  integration test SPEC-016 Phase B currently cannot run.
+- **Session broker:** worker spawns into the active session; capture/input work; survives logoff→logon
+  (respawn) and console-connect (retarget); fast-user-switch retarget.
+- **Security:** non-admin cannot connect to the IPC pipe; worker runs with the user's token (not
+  SYSTEM) in the user's desktop.
+
+## Effort estimate & dependencies
+
+- **Size:** X-Large (service host + worker split + token-handoff + IPC + session-change handling +
+  install/uninstall).
+- **Depends on:** SPEC-016 (enrollment + `cak_` store); the existing capture/input cores.
+- **Unblocks:** SPEC-016 Phase B end-to-end runtime (and the parked managed-agent enrollment test on
+  the internal beta machines); **SPEC-013** (session selection builds on this broker).
+
+## Open questions
+
+1. **Service vs. SYSTEM scheduled task** — a true Windows service (recovery, SCM integration) is the
+   standard, robust choice; recommend service. Lock in planning.
+2. **One multi-session worker vs. one worker per session** — per-session worker is simpler to reason
+   about and isolates a crash to one session; confirm.
+3. **IPC transport** — named pipe (recommended) vs. local TCP/loopback; pipe ACLing is the cleaner
+   security story.
+4. **Login-screen / Secure-Desktop capture** — how much (if any) in this spec vs. deferred to SPEC-013
+   (it needs a worker in the winlogon/secure desktop, a distinct hard problem).
+5. **Migration** — on upgrade, cleanly transition existing HKCU-`Run` managed installs to the service
+   (remove the Run entry, install the service) without a gap.

docs/specs/SPEC-019-private-backstage-session.md

@@ -0,0 +1,119 @@
+# SPEC-019: Private Backstage Session (interactive uninstall / hidden desktop)
+
+**Status:** Proposed
+**Priority:** P2
+**Requested By:** Howard (2026-06-22)
+**Estimated Effort:** Large
+
+## Overview
+
+A **private session** mode for GuruConnect: the tech gets an interactive remote view of a
+process's UI on the target machine that the **logged-on user does not see**. The motivating use
+case is GuruRMM's Tier-2 software removal — when a program has no silent uninstall (flagged
+`needs_remote` by the RMM removal engine), the tech opens a Backstage session, the stubborn
+uninstaller's GUI runs on a **private desktop**, and the tech clicks through it remotely while the
+end user's screen is undisturbed. This is GuruConnect's equivalent of ScreenConnect Backstage.
+
+**Success criteria:**
+- A tech can launch a program (e.g. an uninstaller) on a target and drive its UI remotely.
+- The logged-on user does not see the window or the interaction (private-desktop mode).
+- Works with no interactive user logged in (system-spawned private desktop).
+- Deep-linkable from the GuruRMM "Needs remote removal" flag.
+
+## The Windows constraint (why this is non-trivial)
+
+A GUI process needs an interactive **window station + desktop**. The agent runs as SYSTEM in
+**Session 0**, which is isolated and cannot present UI. So GuruConnect must either (a) run the
+target process on the **active user's desktop** and mirror it (visible to the user), or (b) create
+a **separate private desktop** (`CreateDesktop`) and stream only that desktop to the tech
+(invisible to the user). (b) is the Backstage model.
+
+## Scope
+
+### Included in v1
+- **Private-desktop capture/input:** capture a *specific* desktop's framebuffer and inject
+  input into it (not just the active console desktop), via a dedicated window station/desktop the
+  agent creates (`CreateWindowStation`/`CreateDesktop` + `SetThreadDesktop` on the capture/input
+  threads).
+- **Launch-process-in-session:** start an arbitrary command (the uninstaller) bound to that
+  private desktop, as SYSTEM or as a chosen user token.
+- **Backstage viewer mode** in the web/native viewer: a session flagged "private" with a clear
+  "user cannot see this" indicator.
+- **GuruRMM deep link:** accept a launch target (program path/uninstall string) so the RMM
+  `needs_remote` action opens straight into a Backstage session pointed at that program.
+
+### Explicitly out of scope
+- Mirroring the *active user* desktop — that is ordinary remote control (already covered); this
+  spec is specifically the **private/hidden** desktop.
+- Logging in a brand-new interactive user session (secondary-logon) — heavier and fragile;
+  considered in Future Considerations only.
+- The RMM-side removal engine + tracking (lives in GuruRMM SPEC-030; this is the GC counterpart).
+
+## Architecture
+
+- **agent (Windows):** new private-desktop subsystem — create a window station + desktop, run the
+  capture loop with `SetThreadDesktop` bound to it (DXGI won't capture an off-screen desktop, so
+  fall back to GDI `BitBlt`/`PrintWindow` for the private desktop), inject input with
+  `SendInput`/`PostMessage` targeted at that desktop's windows. Process launch via
+  `CreateProcessAsUser`/`CreateProcess` with `STARTUPINFO.lpDesktop` set to the private desktop.
+- **relay-server (Axum):** a new session kind `private`/`backstage`; same protobuf transport,
+  flagged so the dashboard/viewer render the "hidden from user" state and audit it distinctly.
+- **viewer:** render the private-desktop stream; banner indicating the user can't see it.
+- **dashboard:** "Start Backstage session" entry; accept the deep-link launch target.
+- **proto (`proto/guruconnect.proto`):** add a session-mode enum (`NORMAL`/`PRIVATE`) and a
+  `LaunchProcess { command, desktop: PRIVATE, run_as }` control message.
+
+## Implementation details
+
+- Capture: DXGI Desktop Duplication is tied to the active output/desktop; a private off-screen
+  desktop is **not** duplicatable that way — use GDI capture of the private desktop's windows
+  (`PrintWindow` per top-level window or `BitBlt` of the desktop DC after `SetThreadDesktop`).
+  Lower FPS is acceptable for clicking through an installer.
+- Input: `SendInput` operates on the calling thread's desktop — bind the input thread with
+  `SetThreadDesktop(hPrivateDesktop)` before injecting; for stubborn controls use
+  `PostMessage`/`SendMessage` to the target HWND.
+- Lifecycle: tear down the private desktop + window station when the session ends; kill orphaned
+  child processes bound to it.
+
+## Security considerations
+- A private/hidden session that the end user cannot see is **powerful and must be audited
+  loudly** — record start/stop, operator, target, and the launched command. Consider an
+  org/policy toggle to require consent or to disallow private mode per tenant.
+- Reuse GuruConnect's existing auth (JWT/support code/agent key). Launching arbitrary processes
+  as SYSTEM on a private desktop is high privilege — gate behind tech-role auth.
+- Threat model: a hidden remote session is an attractive abuse target; treat parity with the
+  existing remote-control trust boundary plus extra audit.
+
+## Testing strategy
+- Unit: desktop/window-station create+destroy; thread-desktop binding.
+- Manual: launch Notepad on a private desktop, confirm the logged-on user does NOT see it and the
+  tech can type into it; then a real GUI uninstaller (e.g. an NVIDIA/Office leftover) driven to
+  completion; then with no user logged in.
+- Integration: RMM `needs_remote` deep-link opens a Backstage session at the right program.
+
+## Relationship to existing specs (read first)
+- **SPEC-018 (managed-agent SYSTEM service host + session broker)** is the prerequisite — it
+  provides the SYSTEM service + per-session worker spawning this builds on.
+- **SPEC-013 (session selection + backstage)** already defines backstage as a **terminal/command**
+  interface (services management). SPEC-019 **extends** backstage from terminal-only to a **private
+  GUI desktop** — the ability to see and click an arbitrary program's *window* (an uninstaller)
+  that the logged-on user cannot see. Consider folding this into SPEC-013 as its "GUI backstage"
+  phase if the team prefers one spec.
+
+## Effort estimate & dependencies
+- **Large.** Depends on SPEC-018 (broker) and the existing capture/input pipeline; complements
+  SPEC-013 (backstage). The private-desktop GDI capture + input-desktop binding is the hard, novel
+  part. Unblocks GuruRMM Tier-2 interactive removal and any "do something the user shouldn't see"
+  support workflow.
+
+## Open questions
+- DXGI vs GDI for the private desktop — confirm DXGI truly can't target it; measure GDI FPS.
+- Run the uninstaller as SYSTEM or as the logged-on user's token on the private desktop? (Some
+  per-user uninstallers need the user's profile/HKCU.)
+- Default posture: should private mode require explicit per-session consent or a tenant policy?
+
+## References
+- GuruRMM SPEC-030 (remote software inventory + bulk uninstall) — the `needs_remote` flag + the
+  removal knowledge base that feeds this. Tier-2 is the last resort; the RMM vendor table shrinks
+  the set over time.
+- ScreenConnect Backstage (prior art for a hidden support session).

reports/2026-06-03-spec018-review.md

@@ -0,0 +1,79 @@
+# Code Review Notes: GuruConnect SPEC-018 Phase 1 (Managed Agent as LocalSystem Service Host)
+
+**Review date:** 2026-06-03  
+**Reviewer:** Grok (meticulous code reviewer persona)  
+**Scope:** SPEC-018 Phase 1 changes (merge 11af9df, including review-fix commit a0e0d5f). Diff: `git diff 55b9c97..11af9df` (agent/src/{install.rs,main.rs,service/mod.rs,session/mod.rs}). Full reads of: `agent/src/service/mod.rs`, relevant sections + full run_* functions in `agent/src/main.rs`, `agent/src/session/mod.rs`, `agent/src/credential_store.rs`, `agent/src/install.rs`, `agent/src/startup.rs`, `agent/src/config.rs`, `agent/src/identity.rs`, `agent/src/enroll.rs`, `agent/src/transport/websocket.rs`, `agent/Cargo.toml`, and light greps/server-side enrollment in `server/src/api/enroll.rs` + rate_limit. Also cross-checked CLAUDE.md constraints, Windows service practices, auth paths, shutdown propagation, panic guard, and recent SPEC-016 enrollment/identity code.  
+**Commands used for exploration:** `git log --oneline 55b9c97..11af9df`, `git diff ... --name-only`, file reads (chunked for large), rg/grep for `\.unwrap\(\)`, `\.expect\(`, `unsafe`, `AtomicBool|shutdown|SERVICE_STOP_SENTINEL`, `println!`, `enroll|machine_uid`, etc. No code changes performed.
+
+## Summary
+The SPEC-018 Phase 1 implementation is high-quality, security-conscious, and largely correct for its narrow scope (enrollment + relay only; no capture/input/desktop in Session 0). It correctly introduces the LocalSystem service as the single autostart for managed agents (embedded config), wires cooperative shutdown via `Arc<AtomicBool>` (observed both in outer `run_agent` reconnects and inner `SessionManager::run_with_tray` connected loop via `SERVICE_STOP_SENTINEL`), uses `panic::catch_unwind(AssertUnwindSafe)` + downcast around the Tokio runtime to protect the SCM FFI boundary (mapping to `ServiceSpecific(1)` for recovery), implements idempotent SCM install/uninstall with bounded retry for `ERROR_SERVICE_MARKED_FOR_DELETE`, configures crash recovery via `sc failure`, skips tray/autostart HKCU Run for the service path (preventing double-agents and Session-0 nonsense), and documents Phase 2 seams (broker, `CreateProcessAsUserW`, `SESSIONCHANGE`, IPC) extensively in comments. Adherence to CLAUDE.md is excellent: `tracing` (no `println` in core paths), `anyhow`/`thiserror`, async, no hardcoded secrets, strict auth (managed path always requires cak_/enroll material or legacy key; support codes only for interactive `run_agent_mode`; no unauth agent paths), UUIDs/soft-delete conventions (on server), etc. High-privilege SYSTEM surface is handled with care (ACL'd credential store readable only in-context, fail-fast guards retained as safety net, best-effort status reporting).
+
+However, there are a handful of correctness bugs (especially the non-elevated managed "fallback" that doesn't actually run an agent, and unconditional `agent_id` churn for all embedded-config managed agents on every restart), several `unwrap()`s in hot paths (session loop), a stale `#[allow(dead_code)]`, and some pre-existing but now-in-scope fragile unsafe/transmute patterns in registry code touched by the autostart changes. Service lifecycle, status reporting, and install/retry logic are solid with only minor nits. Phase 2 gaps are clearly and repeatedly called out (no omissions). Light broader review of recent enrollment/identity (SPEC-016) work (which this builds directly on) found related issues in config load priority + agent_id stability and a couple of test-only unwraps. No evidence of race conditions in the AtomicBool propagation or handler blocking. Overall: mergeable with fixes for the flagged bugs; strong engineering but not flawless on edge cases for a SYSTEM RCE surface.
+
+**Issue counts (by severity):** 2 bug, 1 bug/suggestion, 4 suggestion/nit, 1 nit. Total 8. (No issues found in: auth enforcement for agents, use of tracing/anyhow, panic guard intent + sentinel contract (tests pin it), no-double-agent logic on happy path, credential ACL + C1 verification when running as SYSTEM, Windows service handler non-blocking + recovery config, or Phase 2 documentation.)
+
+## Issues
+
+### Issue 1 -- Severity: bug
+- File: agent/src/main.rs:496 (inside `run_permanent_agent_managed`, called from PermanentAgent detection at 320 and 186)
+- Description: When `install::install(false)` fails (e.g., the managed binary is launched interactively without Administrator rights), the code does `warn!(... falling back to in-process agent for this run); return run_agent_mode(None);`. The surrounding comment (and similar in 294-296) claims this ensures "the machine is not left with no agent at all." However, for a managed/PermanentAgent binary (which has embedded `enrollment_key` + `site_code` or a prior `cak_`), `run_agent_mode` → `resolve_agent_credential` will *always* fail: (a) if a `cak_` exists, `load_cak` hits `LoadCakError::Io { permission_denied: true }` and the explicit guard returns a hard error ("must run as the GuruConnect SYSTEM service"); (b) if no `cak_` yet, it calls `enroll::run_enrollment` → `credential_store::store_cak`, whose C1 read-back verification (`load_cak` immediately after write) will see the SYSTEM+Admins ACL and return the perm-denied error, causing `store_cak` and thus enrollment to fail. The "fallback" path therefore exits with error (no relay connection) in the exact case it was meant to save. This is a direct correctness gap for the high-privilege managed path.
+- Suggestion: Remove the fallback for the `has_embedded_config()` / managed case (surface a clear error requiring elevation for first-run managed install, as already stated in `install_managed_service` docs). Or introduce a true degraded fallback (e.g., force a per-user cak_ path or legacy api_key mode) with loud warnings. Update all comments and the "falling back" log. Consider a distinct code path or flag so non-elevated managed binaries don't pretend to provide agent functionality.
+- Status: open
+
+### Issue 2 -- Severity: bug
+- File: agent/src/config.rs:392 (in `Config::load` embedded branch, lines 382-408; also interacts with `detect_run_mode:255`, `has_embedded_config:285`, `read_embedded_config:290`, and PermanentAgent paths in main.rs:166+)
+- Description: Embedded config (the trigger for `RunMode::PermanentAgent` / managed installs per SPEC-016) never includes an `agent_id`. On every `load()` via the embedded priority-1 path, it does `agent_id: generate_agent_id()` (fresh v4 UUID) unconditionally, then `let _ = config.save()`. Because `load()` always prefers `read_embedded_config().is_ok()` (the exe still carries the magic blob post-install), the toml written by `save()` is *never read back* for agent_id on subsequent launches. Consequence: every service start/restart (and every interactive launch of a managed binary) produces a brand-new `agent_id`. This value is passed to `WebSocketTransport::connect(..., &self.config.agent_id, ...)` (session/mod.rs:121) and used for server-side connection identity/tracking. While `machine_uid` + the `cak_`-bound machine row are the stable dedup keys (per identity.rs and server enroll), churning `agent_id` on every restart is still observable churn in logs, sessions, and any agent_id-keyed state. The comment "Save to file for persistence (so agent_id is preserved)" is actively false for the embedded case that managed agents always take.
+- Suggestion: In the embedded construction block, first try loading an existing `agent_id` from the on-disk toml (via a helper or the file-load logic) if present and non-empty; only generate if absent. Alternatively, persist the generated agent_id into the embedded blob at installer creation time (or treat the server's minted agent_id as authoritative and stop sending client-generated one for managed agents). This is in the "recent enrollment/identity code" under light broader review.
+- Status: open
+
+### Issue 3 -- Severity: bug
+- File: agent/src/session/mod.rs:386 (recv path), 486 (chat poll), 559 (frame send) — all inside `run_with_tray` after the `if self.transport.is_none() { bail }` guard at 316 and before the connectivity check at 579-587.
+- Description: Direct `.unwrap()` on `self.transport.as_mut().unwrap()` (and `.as_ref()` in the is_connected check is safe, but the mut ones are not). These execute on *every* message, every outgoing chat, and every video frame while streaming/connected. Although current control flow (transport only set in `connect()`, never set to `None` inside the loop, `release_streaming` doesn't touch it, and the bottom-of-loop check breaks before next iteration) makes them "safe" today, this violates the review rule to flag `unwrap()` in hot paths. A future change (e.g., error path that clears transport, or making run_with_tray re-entrant, or transport becoming Option in more states) turns this into a panic of the entire agent (especially bad under the SYSTEM service). Also present in the stop-sentinel path (which does use `if let Some` for close).
+- Suggestion: Replace with `.expect("transport must be Some inside run_with_tray after the NotConnected guard and while the outer loop considers us connected")` (or better, hold the transport behind a non-Option after the initial check, or use `if let Some(t) = ... { ... } else { break; }` guards). Audit similar patterns elsewhere in the connected loop.
+- Status: open
+
+### Issue 4 -- Severity: suggestion
+- File: agent/src/main.rs:442-466 (the `catch_unwind` + match in `run_managed_agent_service`); related: service/mod.rs:103-107 (service_main), 112 (run_service body), 167 (the call site), 250-256 (comment explaining finding M)
+- Description: The required panic guard across the SCM FFI boundary (`extern "system" ffi_service_main -> service_main -> run_service -> run_managed...`) exists and correctly converts unwind to `Err(...)` → `ServiceExitCode::ServiceSpecific(1)` (so SCM recovery restarts instead of UB/abort). The downcast logic is careful (handles &str/String, falls back without re-panic). However, the `catch_unwind(AssertUnwindSafe(|| { rt.block_on(...) }))` only covers the *inner agent runtime*. Panics originating in `run_service()` itself (e.g., in initial `service_control_handler::register`, the two `set_status` calls before/after the agent loop, the `if let Err` logging, or `set_status_with_exit` for Stopped) would still unwind out of `run_service`/`service_main` across the FFI with no guard. The top-level `if let Err(e) = run_service()` in service_main cannot catch a panic.
+- Suggestion: Wrap more of `run_service()` (or the whole body after the shutdown flag setup) in an outer `catch_unwind`, or add a guard inside `service_main`. This makes the "panic guard across SCM FFI" complete rather than scoped only to the "finding M" agent block_on. (Current scope is probably sufficient in practice, as panics are unlikely in the thin status/reporting code.)
+- Status: open
+
+### Issue 5 -- Severity: suggestion
+- File: agent/src/service/mod.rs:353 (`std::thread::sleep(BACKOFF)` inside `create_service_with_retry` loop), 470-476 (`stop_if_running` 10x 500ms polling loop); also called from install.rs:327/369 and startup paths changed by SPEC-018.
+- Description: These blocking sleeps are *only* in the synchronous install/uninstall code paths (CLI `guruconnect install` / uninstall, which run under the current user's context before handing off to the service). They are not inside the SCM control `event_handler` closure (which correctly does only `store(true)` + `NoError` / `NotImplemented` with no I/O or waits — satisfying "no blocking in handler"). However, they are still blocking the main thread during (re)install, and the fixed polling is a minor source of non-determinism/race on slow SCM. The retry logic itself (gated on `deleted_existing`, bounded attempts) is a clear improvement over the prior fixed 2s sleep.
+- Suggestion: Acceptable for install paths. Consider `std::thread::sleep` → a small async sleep if these ever move under tokio, or document "install-time only." The 500ms stop poll is fine for best-effort delete prep.
+- Status: open
+
+### Issue 6 -- Severity: bug (pre-existing, but now in scope due to SPEC-018 touching autostart/install/uninstall paths)
+- File: agent/src/startup.rs:61 (`std::mem::transmute::<HANDLE, HKEY>(hkey)` after `RegOpenKeyExW`), 122-125 (identical in `remove_from_startup`), plus similar raw pointer casts and unsafe registry blocks in install.rs:152+ (protocol handler), 380+ (is_protocol_handler_registered). Also related unsafe in credential_store (DPAPI) and identity (RegGetValueW) which are part of the enrollment/identity surface reviewed.
+- Description: The transmute from a `HANDLE` (obtained via the windows crate's `RegOpenKeyExW` call site that takes a `*mut _`) to `HKEY` for subsequent `RegSetValueExW`/`RegDeleteValueW` is a fragile, non-idiomatic, and potentially unsound hack. The `windows` 0.58 crate provides properly typed `HKEY` and safe-ish wrappers; mixing `HANDLE` + transmute + `as *mut _` casts risks wrong handle values, leaks, or UB. This code predates SPEC-018 but is executed on the managed install path (which does `remove_from_startup` inside `install_managed_service` and `uninstall_managed_service`).
+- Suggestion: Refactor to use the crate's typed registry APIs directly (pass `&mut HKEY` to the open call, or use `RegOpenKeyExW` overloads that return the handle type). Add `// SAFETY:` comments with justification for any remaining FFI. This is a correctness + maintainability issue for registry paths now used by the SYSTEM service host.
+- Status: open
+
+### Issue 7 -- Severity: nit
+- File: agent/src/transport/websocket.rs:208 (`pub async fn close(&mut self) -> Result<()> { ... }`)
+- Description: The method has `#[allow(dead_code)]` (and the call site in the pre-SPEC-018 code may have been conditional). SPEC-018 now calls it unconditionally in the service-stop path: `session/mod.rs:1134` (`if let Some(transport) = self.transport.as_mut() { if let Err(e) = transport.close().await { ... } }`). The allow is now misleading (though harmless, as the fn is live).
+- Suggestion: Remove `#[allow(dead_code)]`.
+- Status: open
+
+### Issue 8 -- Severity: nit (light broader review of recent enrollment/identity code)
+- File: agent/src/enroll.rs:329,359,360,373,380 (all in `#[cfg(test)]`); also scattered test unwraps in credential_store.rs:379+ (DPAPI tests), encoder tests, etc.
+- Description: Tests use `.unwrap()` liberally (e.g., `serde_json::to_value(&req).unwrap()`, response parses, `dpapi_protect(...).expect(...)`). This is conventional and acceptable in unit tests (they are expected to succeed on the test host), but still "remaining .unwrap()". No runtime impact. (The production paths in enroll use proper error classification into `AttemptError` and never unwrap on HTTP/JSON.)
+- Suggestion: For hygiene, change test unwraps to `?` inside `#[test] fn` (which can return Result) or `.expect("test precondition")`. Not a priority.
+- Status: open
+
+**No issues found in these areas (explicitly checked):**
+- Authentication / never accepting unauthenticated agents: Managed service path always forces `config.support_code = None` and goes through `resolve_agent_credential` (which requires cak_, enrollment material, or deprecated api_key; errors hard on nothing usable). Support-code sessions are exclusively the interactive `run_agent_mode` path. Viewer WS requires JWT (out of scope here). `run_dispatcher` / `service-run` cannot be usefully invoked interactively.
+- Cooperative shutdown + graceful WS close: AtomicBool (SeqCst) set only in handler; polled at top of connected loop (every ~1-100ms) *and* in reconnect backoff (250ms); sentinel error drives clean `transport.close()` (sends WS Close frame) + `return Ok(())` with no reconnect. Tests pin the sentinel contract and "no regression for non-service paths."
+- Service lifecycle / status / recovery / no blocking in handler: Correct `StartPending` → `Running` (accept STOP|SHUTDOWN) → (on stop) `StopPending` → `Stopped` (with Win32(0) or ServiceSpecific(1)). Handler closure is trivial move + store + match. `configure_recovery` + `sc failure` for restart-on-crash. Idempotent create/delete with retry for SCM delete races. `is_service_installed` is total (never panics).
+- No double-agents / autostart hygiene (happy path): `run_permanent_agent_managed` early-exits if service installed; managed install removes HKCU Run (best-effort); service path skips `add_to_startup` and tray creation entirely. Non-managed paths untouched.
+- Credential handling as SYSTEM: `run_managed_agent_service` runs as SYSTEM → `load_cak` (and `store_cak` during enroll) succeed because ACL grants SYSTEM full control. The perm-denied guard + C1 read-back in store_cak are retained exactly as safety net for non-SYSTEM invocations of managed binaries. No secrets logged.
+- Use of tracing/anyhow/thiserror/async/clippy-adjacent style: New code uses `tracing::{error,info,warn}`, `anyhow::Result` + `?` + `.context`, `thiserror` for `LoadCakError`. No `println!` in agent runtime (only in version-info / fallback message boxes / separate sas_service bin).
+- Phase 2 gaps called out: Explicitly and repeatedly in `service/mod.rs` module-level docs (lines 24-41), inline comments (e.g., 124, 422 in main, 1085+ in session for the "no capture yet" rationale and seams for broker/SESSIONCHANGE/CreateProcessAsUser/IPC). No silent omissions.
+- Other CLAUDE.md: Single static binary target, Win7+ (PS/CIM used for identity are present), no redist deps (icacls/sc are inbox), DB conventions on server side (UUID PKs, snake_case, events for audit), etc.
+
+**Executive verdict:** SPEC-018 Phase 1 is a solid, well-defended implementation of the SYSTEM service host with correct lifecycle, shutdown, and guardrails. The two "bug" issues (non-functional fallback on install failure for managed agents; agent_id churn on every restart) are real correctness problems that should be fixed before wider managed deployment, as they affect reliability of the high-privilege path and identity stability. The unwraps and registry transmute are lower-severity but worth cleaning for a SYSTEM binary. The rest of the work (including the excellent documentation of what's deliberately left for Phase 2) meets or exceeds the project's standards and the requirements in CLAUDE.md. Recommend addressing Issues 1-3 (and 6) prior to release; the rest can be follow-ups. The changes do not introduce new auth holes or unauthenticated agent paths.
+
+**File written:** D:\GrokTools\guru-connect-review-SPEC018.md
+
+(End of review notes.)

reports/review-2026-06-05/REMEDIATION-PLAN.md

@@ -0,0 +1,110 @@
+# GuruConnect — Remediation Plan (from the 2026-06-05 three-way review)
+
+Derived from `SYNTHESIS-three-way.md` (Claude + Gemini + Grok). Findings are grouped into
+shippable phases. Each phase is sized to a GuruConnect spec and should go through the
+standard pipeline (spec → three-way design review → build → three-way code review → deploy →
+validate). Phase numbers are priority order, not strict dependency order — P0 and parts of
+P4/P5 can run in parallel with the larger P1/P2/P3 efforts.
+
+Effort key: **S** = <0.5 day, **M** = 0.5–2 days, **L** = >2 days / multi-component.
+Finding IDs (C#/H#) reference the synthesis report.
+
+---
+
+## P0 — Emergency hardening pass (proposed SPEC-019)
+Small, self-contained, high-value. No protocol or cross-component coordination needed. Ship first.
+
+| Finding | File(s) | Action | Effort |
+|---|---|---|---|
+| **C2** | `server/src/api/downloads.rs` | Remove the `"managed-agent"` default API-key fallback and the hardcoded `wss://connect.azcomputerguru.com/...` URL; move both into `Config`/`AppState` and have the download handlers take `State`. Either implement real support-config embedding (MAGIC_MARKER + len + JSON, matching the permanent path) or correct the module docstring to stop claiming it. | M |
+| **C3** | `server/src/api/downloads.rs` | Replace every `Response::builder()…body(…).unwrap()` with a real error path (500 JSON). Reject/percent-encode (RFC 5987 `filename*=`) `Content-Disposition` values; cap input length before sanitizing. Closes an **unauthenticated panic/DoS**. | S |
+| **H1** | `server/src/api/auth.rs` (login, change_password) | Add rate-limit + failure-lockout on the login path, reusing the existing `state.rate_limits` machinery from the enroll path. Key on `(username.lower(), client_ip)`. Return 429 after threshold; record failures after constant-time work. | M |
+| **H2** | `server/src/main.rs` (bootstrap ~198–225) | Stop writing the generated admin **plaintext** to `.admin-credentials`; remove the `info!` log fallback. Prefer operator-supplied initial password via env/one-time flag (error on zero-user start if absent), or print once to stdout with a "copy now, never persisted" warning. _(Server deploys on Linux so 0o600 works — the residual risk is on-disk plaintext + the log path, not the Windows-ACL angle.)_ | S |
+| **H3** | `server/src/api/auth_logout.rs:116` | The `revoke_user_tokens` 501 stub claims partial behavior it doesn't have. Either delete the route until the session-tracking table exists, or implement the minimal real version and make the comment/error match reality. | S |
+| **H5** | `server/src/api/users.rs` + `auth` extractor | Enforce the **self-role-demotion / self-disable guard server-side** (currently only self-_delete_ is blocked; the lockout guard is client-only in `EditUserModal.tsx`). Mirror the last-admin/self guards already shipped in GuruRMM SPEC-027. | S |
+
+**Validation:** unauthenticated download fuzz (quotes/control chars in `company`/`site`), login brute-force returns 429, fresh-server bootstrap leaves no secret on disk/in logs, self-demote returns 4xx server-side.
+
+---
+
+## P1 — WebSocket auth handshake redesign (proposed SPEC-020) — the big one
+**C1 — the top convergent finding.** Secrets/tokens currently travel in WS URL query strings on
+both planes (agent: `api_key`/`support_code`/`machine_uid`; viewer: the session-scoped control token).
+
+This is partly **by current design** — `guru-connect/CLAUDE.md` Security Rules mandate
+"Viewer WebSocket: JWT token required in `token` query parameter." Both external models reject
+that. **First action: a design decision to change that spec rule.** Server-side the viewer token
+is already hardened (minted, session-bound, signature/purpose/`session_id` checked, blacklist on
+the WS plane) — only the transport channel is the problem.
+
+Workstream (L, cross-component — run a three-way *design* review before building):
+1. `proto/guruconnect.proto`: add an initial `AuthHandshake` message (agent creds / viewer token + binding).
+2. `server/src/relay/mod.rs`: accept the WS upgrade unauthenticated, then require the auth frame as the first message; fail-closed on timeout/missing. Keep all existing binding/blacklist/consent checks.
+3. `agent/src/transport/websocket.rs` (+ call sites: `session/mod.rs`, `viewer/transport.rs`, `install.rs`): connect, then send creds in the first protobuf frame. Remove secrets from the URL.
+4. `dashboard/src/features/sessions/JoinSessionModal.tsx` (`buildViewerUrl`) + `api/sessions.ts`: send the viewer token via the handshake; stop surfacing the full token-bearing URL as a copyable field.
+5. Update `guru-connect/CLAUDE.md` security rules to match.
+
+**Interim mitigation (until P1 ships):** make minted viewer tokens single-use-on-first-attach and shorten TTL; ensure the relay/NPM proxy never logs full request URLs.
+
+---
+
+## P2 — Update & embedded-config integrity (proposed SPEC-021)
+| Finding | File(s) | Action | Effort |
+|---|---|---|---|
+| **C5** | `agent/src/update.rs` | SHA-256 over the same channel is not tamper defense (the code's own TODO says so). Embed an Ed25519 public key; sign the update manifest + binary at build time; **verify signature before `install_update`**. Lock `dev_insecure_tls` to debug builds and consider removing the env escape hatch. | M |
+| **MED** | `agent/src/config.rs` (`read_embedded_config`) | Same crypto primitive: sign the appended `GURUCONFIG` blob (Ed25519) and verify before trusting `server_url`/`enrollment_key`. Add a strict max length, validate the marker sits after the PE end, and allowlist `server_url` scheme/host. Prevents an attacker blob from repointing the agent to a hostile relay. | M |
+
+Bundle these because they share the signing key infrastructure and the build-pipeline signing step. The Pluto build server / Gitea webhook pipeline will need a signing key (store in SOPS vault).
+
+---
+
+## P3 — Agent runtime correctness (proposed SPEC-022)
+| Finding | File(s) | Action | Effort |
+|---|---|---|---|
+| **C4** | `agent/src/transport/websocket.rs` | Remove `block_in_place(|| Handle::block_on(...))` from the main session loop and viewer recv. Use a dedicated recv task forwarding over `mpsc`, or async `timeout` directly on the locked stream. Fix the racy `connected` flag. | M |
+| **H7** | `agent/src/session/mod.rs` | Drive attended-consent off the main loop (spawn a task that owns only the response send) so heartbeats/status/stop-signals keep flowing during the ~60s think-time. | M |
+| **H8** | `agent/src/credential_store.rs:~309/345` | Call `icacls` by absolute path (`C:\Windows\System32\icacls.exe`) or use Win32 `SetNamedSecurityInfoW` directly; verify the resulting ACL actually denies non-admins (or log raw `icacls` output on failure) instead of silently leaving a weaker store. | S |
+| **MED** | `agent/src/encoder/h264.rs`, `decoder.rs`, `capture/{dxgi,gdi}.rs`, `session/mod.rs` | Wrap the per-frame `capture()`/`encode()` in the same `catch_unwind` + "mark capturer bad, fall back/idle" recovery used at init; make COM `ReleaseFrame`/`DeleteObject` strict (scopeguard, exactly-once per Acquire); after N consecutive H.264 errors force renegotiation/hard-switch to raw instead of per-frame silent drop; remove dead `last_frame`. | L |
+
+---
+
+## P4 — Auth/session lifetime & dashboard hardening (proposed SPEC-023)
+| Finding | File(s) | Action | Effort |
+|---|---|---|---|
+| **H4** | `server/src/auth/token_blacklist.rs:85` | Store `(token, exp)` and make `cleanup_expired` a pure time comparison instead of re-verifying every JWT signature. Capture `exp` at revoke time (cheap unsigned parse). | S |
+| **H6** | `dashboard/src/auth/AuthProvider.tsx`, `api/client.ts` | Parse/respect token `exp`; add proactive refresh-or-logout and an idle-activity timeout. Longer-term: move the JWT to an `httpOnly; Secure; SameSite=Strict` cookie managed by the server (needs a server-side cookie/refresh endpoint — coordinate with P1). | M |
+| **MED** | `dashboard/src/auth/AuthProvider.tsx` (`restore`) | Only `clearSession()` on an explicit `401`; on network (status 0) / 5xx keep the token and show an offline/retry state. Stops flaky-connection logout loops. | S |
+| **MED** | `server/src/api/users.rs` + `dashboard/src/features/users/hooks.ts` | Add a transactional `PATCH /api/users/:id` that updates core fields + permissions atomically; switch the dashboard off the two-call `updateUser`→`setUserPermissions` sequence. | M |
+
+---
+
+## P5 — Hygiene & maintainability (rolling, no dedicated spec)
+- **MED** Password policy: complexity + upper bound + breach-list (`zxcvbn`); raise min to 12. (`auth.rs`, `users.rs`, `enroll.rs`)
+- **MED** Input validation: cap + control-char-strip `machine_uid`/`hostname`/`site_code`/password on the unauthenticated enroll path before DB / rate-limit keys. (`enroll.rs`)
+- **MED** Audit correctness: `CONNECTION_REJECTED_*` events should insert `session_id = NULL` (like enrollment/removal), not a random UUID. Add `log_connection_rejected` or make `log_event` take `Option<Uuid>`. (`relay/mod.rs`, `db/events.rs`) — _also resolves the dangling-row issue noted in the earlier in-product audit assessment._
+- **MED** Generate dashboard TS types from the Rust API (`ts-rs`/`specta`) to kill type drift; interim: a snapshot-diff test. (`dashboard/src/api/types.ts`)
+- **MED** Wire the declared `VIEWER_INPUT_EVENTS_PER_SEC = 200` viewer-input rate limit (token bucket / coalesce latest mouse-move) or update the comment to match the backpressure-only reality. (`relay/mod.rs`)
+- **LOW** Remove panic sites on response/header construction (`prometheus_metrics` locks/encode, `security_headers` `from_static().unwrap()`). (`main.rs`, `middleware/security_headers.rs`)
+- **LOW** Document (or persist) that in-memory rate-limit/lockout state resets on restart; consider a bounded TTL table in Postgres for enroll/code-validate. (`rate_limit.rs`)
+- **LOW** Redundant post-decode `exp` checks; consider a small configurable leeway. (`auth/jwt.rs`)
+- **LOW** `set_client_access` should verify client UUIDs exist before storing. (`users.rs`)
+- **LOW** Dashboard polling: pause `refetchInterval` when `document.hidden`, add jitter/backoff. (`features/*/hooks.ts`)
+- **LOW** Remove `stubs.ts` from the barrel export; audit + delete or feature-gate the large `#[allow(dead_code)]` "native-remote-control"/Phase-4 surface as it's activated. (dashboard + server/agent)
+- **LOW** Standardize on the `ApiError` envelope server-wide; centralize role/permission allow-lists. (`api/*`)
+- **LOW** Dashboard `rowKey` on `m.id` while everything else keys on `agent_id` — standardize on `agent_id`. (`MachinesPage.tsx`)
+- **LOW** Treat unknown/plain-text server errors as opaque in the dashboard toasts (don't dump raw bodies). (`api/client.ts`)
+- **LOW** Drop the deprecated `document.execCommand("copy")` clipboard fallback. (`useClipboard.ts`)
+
+---
+
+## Suggested sequencing
+1. **P0** immediately (one hardening spec, ~2–3 days total, all small).
+2. Kick off the **P1** design review in parallel (it's the highest-impact and longest-lead).
+3. **P2** and **P3** next (agent-side; can overlap once P1 design is settled, since P1 also touches the agent transport).
+4. **P4** (some of it — httpOnly cookie — depends on P1's server auth changes).
+5. **P5** folded into whichever spec touches the relevant file, or a periodic cleanup pass.
+
+## Tracking
+Create coord todos (project_key `guruconnect`) for P0–P4 as parent items, with the individual
+findings as sub-tasks, plus Gitea issues on `azcomputerguru/guru-connect` for the CRITICAL/HIGH
+items so they're referenced from commits (`Fixes #N`).

reports/review-2026-06-05/SYNTHESIS-three-way.md

@@ -0,0 +1,96 @@
+# GuruConnect — Three-Way Independent Review (Claude + Gemini + Grok)
+
+**Date:** 2026-06-05
+**Reviewers:** Gemini `gemini-3.1-pro-preview`, Grok 4.3, Claude (synthesis + cross-check)
+**Scope:** server (42 Rust files), agent (34 Rust files), dashboard (66 TS/TSX files). Each model read the source via its own `read_file` tool (no embedding). Grok's server pass was split A/B because 42 files exceeded its read-file capacity.
+**Raw reports:** `gemini-{server,agent,dashboard}.md`, `grok-server-a.md`, `grok-server-b.md`, `grok-agent.md`, `grok-dashboard.md` (same folder).
+
+Findings where **both** external models independently agree are marked **[CONVERGENT]** — higher confidence. Claude's cross-checks/adjustments are in _italics_.
+
+---
+
+## The one cross-cutting theme: secrets in WebSocket URL query strings
+
+This is the **single highest-signal finding** — flagged independently as CRITICAL on **all three surfaces**:
+
+- **Agent → relay** (`agent/src/transport/websocket.rs`): `api_key`/`cak_`, `support_code`, `machine_uid` in the WS URL query. **[CONVERGENT — both models' #1 for the agent]**
+- **Dashboard → relay** (`dashboard/src/features/sessions/JoinSessionModal.tsx` `buildViewerUrl`): the session-scoped **viewer token** (the actual remote-control capability) in `?token=`. **[CONVERGENT — both models' #1 for the dashboard]**
+
+Query strings leak into reverse-proxy/relay access logs, browser history, and copy-paste. Over `wss://` the body is encrypted but the URL is routinely logged.
+
+_Claude cross-check: this is partly **by current design** — `CLAUDE.md` security rules explicitly mandate "Viewer WebSocket: JWT token required in `token` query parameter." So this isn't an accidental bug; it's a spec decision both external models reject. **Recommendation: revisit that spec rule** — move auth to the first post-connect protobuf frame or `Sec-WebSocket-Protocol`. Server-side the viewer token is already hardened (minted, session-scoped, signature+purpose+`session_id`-binding checked, blacklist-checked on the WS plane — confirmed in `grok-server-b`), so the residual risk is purely the transport channel. Fix this once, on both planes._
+
+---
+
+## CRITICAL
+
+| # | Area | Finding | Source |
+|---|------|---------|--------|
+| C1 | agent + dashboard | Secrets/tokens in WS query string (see theme above) | **[CONVERGENT]** Gemini+Grok, both surfaces |
+| C2 | `server/src/api/downloads.rs` | Unauthenticated download endpoints: hardcoded prod relay URL + `"managed-agent"` default API-key fallback; support-download docstring says "embeds code" but only renames the file (temp sessions get no embedded config). | **[CONVERGENT]** Gemini-server (key), Grok-server-a (key + URL + docstring gap) |
+| C3 | `server/src/api/downloads.rs` | `Response::builder()…body(…).unwrap()` on attacker-controlled `Content-Disposition` filename (`sanitize_filename` doesn't escape `"`/control chars) → **unauthenticated panic / worker DoS**. | Grok-server-a |
+| C4 | `agent/src/transport/websocket.rs` | `block_in_place(|| Handle::block_on(...))` inside the main async session loop (and viewer recv) — async/sync mixing → thread starvation / deadlock risk under contention; racy `connected` flag. | Grok-agent |
+| C5 | `agent/src/update.rs` | Auto-update verified by **SHA-256 only over the same channel** as the binary — no signature. A malicious/MITM relay can push SYSTEM-level code to the whole fleet. | **[CONVERGENT]** Gemini-agent (#1) + Grok-agent (HIGH) |
+
+_Claude note on C5: code carries an explicit `TODO` acknowledging the missing signature, and there's a debug-only `dev_insecure_tls` escape hatch. Both models rate this top-tier; it's the highest-impact agent issue after the query-string transport._
+
+---
+
+## HIGH
+
+| # | Area | Finding | Source |
+|---|------|---------|--------|
+| H1 | `server/src/api/auth.rs` (login/change_password) | **No rate limiting / lockout on the human login path**, despite sophisticated per-`(site_code,ip)` + timing-equalizer protection on the machine-enroll path. Unlimited online brute-force / credential stuffing. | **[CONVERGENT]** Gemini-server + Grok-server-a |
+| H2 | bootstrap admin password | First-run writes the generated admin **plaintext password to `.admin-credentials`** in CWD, with an `info!` **log fallback** on write failure. | **[CONVERGENT]** Gemini-server + Grok-server-b (Grok's "most important") |
+| H3 | `server/src/api/auth_logout.rs:116` | `revoke_user_tokens` is a **501 stub** but the comment claims partial behavior — an admin "revoke any user's tokens" endpoint that does nothing. | Grok-server-a |
+| H4 | `server/src/auth/token_blacklist.rs:85` | `cleanup_expired` re-runs **full JWT signature verification** on every blacklisted token (stores whole tokens in RAM) instead of comparing a stored `exp`. | Grok-server-a |
+| H5 | self-service role guard | Server **does not block self-role-demotion** (only self-_delete_ is blocked); the lock-out guard is **client-side only** in `EditUserModal.tsx`. | **[CONVERGENT]** Gemini-dashboard + Grok-dashboard (both cite the in-code server comment) |
+| H6 | dashboard auth lifetime | JWT in `sessionStorage`, blindly attached as Bearer; **no `exp` handling, refresh, or idle timeout** — XSS/workstation compromise → long-lived privileged access + ability to mint per-session control tokens. | **[CONVERGENT]** Gemini-dashboard + Grok-dashboard |
+| H7 | `agent/src/session/mod.rs` consent | Attended-session consent (`MessageBoxW`) is `.await`ed **inside the main loop** → for up to ~60s no heartbeats / status / stop-signal processing. | **[CONVERGENT]** Gemini-agent + Grok-agent |
+| H8 | `agent/src/credential_store.rs` | `cak_` store ACL set by shelling out to bare `icacls` (PATH-search) from a SYSTEM context → LPE if `icacls.exe` is hijacked; silent weaker store on failure. | Gemini-agent (+ Grok-agent notes the silent-failure variant) |
+
+_Claude cross-check on H2: the server **deploys on Linux** (Ubuntu 22.04 per `CLAUDE.md`), so the `0o600` chmod **is** effective and Grok's "Windows ACL is a no-op" framing doesn't apply to the deployed server. The real residual risk is the plaintext-on-disk + the `info!` log-fallback path. I'd keep this **HIGH**, not CRITICAL._
+
+---
+
+## MEDIUM (consolidated)
+
+- **Weak password policy** — min length 8 only, no complexity/upper-bound/breach check. **[CONVERGENT]** (Gemini-server, Grok-server-a)
+- **Input validation caps** — unbounded/uncontrolled `machine_uid`, `hostname`, `site_code`, password length on unauthenticated enroll; no control-char stripping before DB / rate-limit keys. (Grok-server-a)
+- **Agent embedded-config trust** — `GURUCONFIG` blob (server_url + enrollment_key) appended to the EXE is read with an unchecked length field and `from_slice`, no signature/allowlist → attacker blob can repoint the agent to a hostile relay. (Grok-agent)
+- **`support_code` derived from EXE filename** with no checksum/strength, then sent in the query string. (Grok-agent)
+- **Audit-trail correctness** — `CONNECTION_REJECTED_*` events insert a random `session_id` (dangling vs `connect_sessions`) instead of `NULL` like the enrollment/removal paths. (Grok-server-b) _Ties into the earlier in-product audit assessment: the write path works but has rough edges._
+- **Panic sites on response/header construction** — `prometheus_metrics` `registry.lock().unwrap()`/`encode().unwrap()`; `security_headers` `from_static(...).unwrap()`. (Grok-server-b)
+- **Declared-but-missing viewer input rate limit** — `VIEWER_INPUT_EVENTS_PER_SEC = 200` constant + comment promise coalescing/drop, but the path is a plain `mpsc(64)` with only backpressure; **[CONVERGENT-ish]** Gemini-server raised the same aggregate-flood concern. (Grok-server-b, Gemini-server)
+- **Non-atomic user update** — dashboard does `updateUser` then `setUserPermissions` as two calls; partial-failure leaves drift. **[CONVERGENT]** (Gemini-dashboard, Grok-dashboard) → wants a single transactional `PATCH /api/users/:id`.
+- **Hand-maintained TS type mirrors** of Rust API structs → silent type drift. **[CONVERGENT]** (both dashboard reviews) → generate via `ts-rs`/`specta`.
+- **Premature `clearSession()` on any `getMe` failure** (incl. network status 0) → flaky-connection logout loops. (Gemini-dashboard)
+- **In-memory rate-limit/lockout state** — reset on every restart/deploy → attacker gets a fresh brute-force window. (Grok-server-b; Gemini-server LOW)
+- **H.264 MFT robustness / capture resource release** — complex unsafe COM streaming-state machine, per-frame silent drops, best-effort `.ok()` releases (leak/double-release risk); per-frame `capture()` not under the same `catch_unwind` as init. (Grok-agent)
+- **Error-body passthrough** — dashboard surfaces raw server error text in toasts → potential internal-detail leak. (Grok-dashboard)
+
+## LOW / maintainability
+- Redundant post-decode `exp` checks (`leeway=0`) in `jwt.rs`. (Grok-server-a)
+- `set_client_access` stores client UUIDs without existence check (dangling refs). (Grok-server-a)
+- Large `#[allow(dead_code)]` surface ("TODO(native-remote-control)", Phase 4) — risk that future activation enables un-reviewed paths. (Grok both server + agent)
+- Audit-log table grows unbounded, no pruning/retention. (Gemini-server)
+- `react-query` polling is aggressive with no `document.hidden` pause/jitter. (Grok-dashboard)
+- `stubs.ts` dead scaffolding still in the barrel export. (Grok-dashboard)
+- Duplicated error envelopes (`ErrorResponse` vs `ApiError`) + repeated role allow-lists across handlers. (Grok-server-a)
+- Deprecated `document.execCommand("copy")` clipboard fallback. (Gemini-dashboard)
+
+---
+
+## What all three reviewers independently praised (don't regress these)
+Argon2id + timing-equalizer on the unauthenticated enroll path; fully parameterized sqlx (no SQLi surface); per-agent `cak_` identity binding that stops a client from seizing another machine's session; atomic single-use support-code consumption at bind time; trusted-proxy-aware `client_ip` extractor as the single root for rate-limit + audit; consent gate before any viewer join/stream; reattach/supersede/reap TOCTOU hardening via recheck-under-write-lock; bounded WS message sizes on both planes; minted session-scoped viewer tokens with blacklist + session-binding checks; DPAPI + ACL credential store with readback verification; thoughtful one-time-secret UX (codes/keys/passwords) and dialog focus management on the dashboard.
+
+---
+
+## Recommended fix order (Claude)
+1. **C1 / query-string secrets** — one architectural change, both planes; revisit the `CLAUDE.md` viewer-token-in-query rule. Highest leverage.
+2. **C2 + C3** — harden the unauthenticated `downloads.rs` surface (remove default key + hardcoded URL, kill the `.unwrap()` panic, fix support-embed gap). Cheap, high exposure.
+3. **H1** — add login rate-limit/lockout (reuse the enroll limiter). Cheap, high value.
+4. **C5 / update signing** — embed a public key, sign the update manifest+binary. Fleet-wide RCE prevention.
+5. **C4 + H7** — fix agent async/sync mixing and move consent off the main loop.
+6. **H2, H3, H4, H5, H6** — bootstrap-secret handling, finish/remove the logout stub, cheap blacklist cleanup, server-side self-demotion guard, dashboard token lifetime.
+7. MEDIUM/LOW as capacity allows; prioritize embedded-config signing and the audit `session_id`-NULL fix.

reports/review-2026-06-05/files-agent.txt

@@ -0,0 +1,34 @@
+/d/claudetools/projects/msp-tools/guru-connect/agent/src/bin/sas_service.rs
+/d/claudetools/projects/msp-tools/guru-connect/agent/src/capture/display.rs
+/d/claudetools/projects/msp-tools/guru-connect/agent/src/capture/dxgi.rs
+/d/claudetools/projects/msp-tools/guru-connect/agent/src/capture/gdi.rs
+/d/claudetools/projects/msp-tools/guru-connect/agent/src/capture/mod.rs
+/d/claudetools/projects/msp-tools/guru-connect/agent/src/chat/mod.rs
+/d/claudetools/projects/msp-tools/guru-connect/agent/src/config.rs
+/d/claudetools/projects/msp-tools/guru-connect/agent/src/consent/mod.rs
+/d/claudetools/projects/msp-tools/guru-connect/agent/src/credential_store.rs
+/d/claudetools/projects/msp-tools/guru-connect/agent/src/encoder/capability.rs
+/d/claudetools/projects/msp-tools/guru-connect/agent/src/encoder/color.rs
+/d/claudetools/projects/msp-tools/guru-connect/agent/src/encoder/h264.rs
+/d/claudetools/projects/msp-tools/guru-connect/agent/src/encoder/mod.rs
+/d/claudetools/projects/msp-tools/guru-connect/agent/src/encoder/raw.rs
+/d/claudetools/projects/msp-tools/guru-connect/agent/src/enroll.rs
+/d/claudetools/projects/msp-tools/guru-connect/agent/src/identity.rs
+/d/claudetools/projects/msp-tools/guru-connect/agent/src/input/keyboard.rs
+/d/claudetools/projects/msp-tools/guru-connect/agent/src/input/mod.rs
+/d/claudetools/projects/msp-tools/guru-connect/agent/src/input/mouse.rs
+/d/claudetools/projects/msp-tools/guru-connect/agent/src/install.rs
+/d/claudetools/projects/msp-tools/guru-connect/agent/src/main.rs
+/d/claudetools/projects/msp-tools/guru-connect/agent/src/sas_client.rs
+/d/claudetools/projects/msp-tools/guru-connect/agent/src/service/mod.rs
+/d/claudetools/projects/msp-tools/guru-connect/agent/src/session/mod.rs
+/d/claudetools/projects/msp-tools/guru-connect/agent/src/startup.rs
+/d/claudetools/projects/msp-tools/guru-connect/agent/src/transport/mod.rs
+/d/claudetools/projects/msp-tools/guru-connect/agent/src/transport/websocket.rs
+/d/claudetools/projects/msp-tools/guru-connect/agent/src/tray/mod.rs
+/d/claudetools/projects/msp-tools/guru-connect/agent/src/update.rs
+/d/claudetools/projects/msp-tools/guru-connect/agent/src/viewer/decoder.rs
+/d/claudetools/projects/msp-tools/guru-connect/agent/src/viewer/input.rs
+/d/claudetools/projects/msp-tools/guru-connect/agent/src/viewer/mod.rs
+/d/claudetools/projects/msp-tools/guru-connect/agent/src/viewer/render.rs
+/d/claudetools/projects/msp-tools/guru-connect/agent/src/viewer/transport.rs

reports/review-2026-06-05/files-dashboard.txt

@@ -0,0 +1,66 @@
+/d/claudetools/projects/msp-tools/guru-connect/dashboard/src/App.tsx
+/d/claudetools/projects/msp-tools/guru-connect/dashboard/src/api/auth.ts
+/d/claudetools/projects/msp-tools/guru-connect/dashboard/src/api/client.ts
+/d/claudetools/projects/msp-tools/guru-connect/dashboard/src/api/codes.ts
+/d/claudetools/projects/msp-tools/guru-connect/dashboard/src/api/index.ts
+/d/claudetools/projects/msp-tools/guru-connect/dashboard/src/api/machines.ts
+/d/claudetools/projects/msp-tools/guru-connect/dashboard/src/api/sessions.ts
+/d/claudetools/projects/msp-tools/guru-connect/dashboard/src/api/stubs.ts
+/d/claudetools/projects/msp-tools/guru-connect/dashboard/src/api/types.ts
+/d/claudetools/projects/msp-tools/guru-connect/dashboard/src/api/users.ts
+/d/claudetools/projects/msp-tools/guru-connect/dashboard/src/auth/AdminRoute.tsx
+/d/claudetools/projects/msp-tools/guru-connect/dashboard/src/auth/AuthContext.tsx
+/d/claudetools/projects/msp-tools/guru-connect/dashboard/src/auth/AuthProvider.tsx
+/d/claudetools/projects/msp-tools/guru-connect/dashboard/src/auth/ProtectedRoute.tsx
+/d/claudetools/projects/msp-tools/guru-connect/dashboard/src/components/layout/AppShell.tsx
+/d/claudetools/projects/msp-tools/guru-connect/dashboard/src/components/layout/PageHeader.tsx
+/d/claudetools/projects/msp-tools/guru-connect/dashboard/src/components/layout/Sidebar.tsx
+/d/claudetools/projects/msp-tools/guru-connect/dashboard/src/components/layout/Topbar.tsx
+/d/claudetools/projects/msp-tools/guru-connect/dashboard/src/components/layout/icons.tsx
+/d/claudetools/projects/msp-tools/guru-connect/dashboard/src/components/ui/Badge.tsx
+/d/claudetools/projects/msp-tools/guru-connect/dashboard/src/components/ui/Button.tsx
+/d/claudetools/projects/msp-tools/guru-connect/dashboard/src/components/ui/ConfirmDialog.tsx
+/d/claudetools/projects/msp-tools/guru-connect/dashboard/src/components/ui/Drawer.tsx
+/d/claudetools/projects/msp-tools/guru-connect/dashboard/src/components/ui/Input.tsx
+/d/claudetools/projects/msp-tools/guru-connect/dashboard/src/components/ui/Modal.tsx
+/d/claudetools/projects/msp-tools/guru-connect/dashboard/src/components/ui/Panel.tsx
+/d/claudetools/projects/msp-tools/guru-connect/dashboard/src/components/ui/Spinner.tsx
+/d/claudetools/projects/msp-tools/guru-connect/dashboard/src/components/ui/States.tsx
+/d/claudetools/projects/msp-tools/guru-connect/dashboard/src/components/ui/StatusDot.tsx
+/d/claudetools/projects/msp-tools/guru-connect/dashboard/src/components/ui/Table.tsx
+/d/claudetools/projects/msp-tools/guru-connect/dashboard/src/components/ui/TableSkeleton.tsx
+/d/claudetools/projects/msp-tools/guru-connect/dashboard/src/components/ui/dialogStack.ts
+/d/claudetools/projects/msp-tools/guru-connect/dashboard/src/components/ui/status.ts
+/d/claudetools/projects/msp-tools/guru-connect/dashboard/src/components/ui/toast-context.ts
+/d/claudetools/projects/msp-tools/guru-connect/dashboard/src/components/ui/toast.tsx
+/d/claudetools/projects/msp-tools/guru-connect/dashboard/src/features/auth/LoginPage.tsx
+/d/claudetools/projects/msp-tools/guru-connect/dashboard/src/features/codes/CancelCodeDialog.tsx
+/d/claudetools/projects/msp-tools/guru-connect/dashboard/src/features/codes/GenerateCodeModal.tsx
+/d/claudetools/projects/msp-tools/guru-connect/dashboard/src/features/codes/SupportCodesPage.tsx
+/d/claudetools/projects/msp-tools/guru-connect/dashboard/src/features/codes/hooks.ts
+/d/claudetools/projects/msp-tools/guru-connect/dashboard/src/features/machines/BulkRemoveMachinesDialog.tsx
+/d/claudetools/projects/msp-tools/guru-connect/dashboard/src/features/machines/DeleteMachineDialog.tsx
+/d/claudetools/projects/msp-tools/guru-connect/dashboard/src/features/machines/KeyRevealModal.tsx
+/d/claudetools/projects/msp-tools/guru-connect/dashboard/src/features/machines/MachineDetailDrawer.tsx
+/d/claudetools/projects/msp-tools/guru-connect/dashboard/src/features/machines/MachineKeysModal.tsx
+/d/claudetools/projects/msp-tools/guru-connect/dashboard/src/features/machines/MachinesPage.tsx
+/d/claudetools/projects/msp-tools/guru-connect/dashboard/src/features/machines/RemoveMachineDialog.tsx
+/d/claudetools/projects/msp-tools/guru-connect/dashboard/src/features/machines/hooks.ts
+/d/claudetools/projects/msp-tools/guru-connect/dashboard/src/features/sessions/EndSessionDialog.tsx
+/d/claudetools/projects/msp-tools/guru-connect/dashboard/src/features/sessions/JoinSessionModal.tsx
+/d/claudetools/projects/msp-tools/guru-connect/dashboard/src/features/sessions/RemoveSessionDialog.tsx
+/d/claudetools/projects/msp-tools/guru-connect/dashboard/src/features/sessions/SessionsPage.tsx
+/d/claudetools/projects/msp-tools/guru-connect/dashboard/src/features/sessions/hooks.ts
+/d/claudetools/projects/msp-tools/guru-connect/dashboard/src/features/users/CreateUserModal.tsx
+/d/claudetools/projects/msp-tools/guru-connect/dashboard/src/features/users/DeleteUserDialog.tsx
+/d/claudetools/projects/msp-tools/guru-connect/dashboard/src/features/users/EditUserModal.tsx
+/d/claudetools/projects/msp-tools/guru-connect/dashboard/src/features/users/PasswordField.tsx
+/d/claudetools/projects/msp-tools/guru-connect/dashboard/src/features/users/PermissionsField.tsx
+/d/claudetools/projects/msp-tools/guru-connect/dashboard/src/features/users/UsersPage.tsx
+/d/claudetools/projects/msp-tools/guru-connect/dashboard/src/features/users/hooks.ts
+/d/claudetools/projects/msp-tools/guru-connect/dashboard/src/features/users/password.ts
+/d/claudetools/projects/msp-tools/guru-connect/dashboard/src/lib/time.ts
+/d/claudetools/projects/msp-tools/guru-connect/dashboard/src/lib/useClipboard.ts
+/d/claudetools/projects/msp-tools/guru-connect/dashboard/src/lib/useRelayStatus.ts
+/d/claudetools/projects/msp-tools/guru-connect/dashboard/src/main.tsx
+/d/claudetools/projects/msp-tools/guru-connect/dashboard/src/vite-env.d.ts

reports/review-2026-06-05/files-server-a.txt

@@ -0,0 +1,21 @@
+/d/claudetools/projects/msp-tools/guru-connect/server/src/api/auth.rs
+/d/claudetools/projects/msp-tools/guru-connect/server/src/api/auth_logout.rs
+/d/claudetools/projects/msp-tools/guru-connect/server/src/api/changelog.rs
+/d/claudetools/projects/msp-tools/guru-connect/server/src/api/downloads.rs
+/d/claudetools/projects/msp-tools/guru-connect/server/src/api/enroll.rs
+/d/claudetools/projects/msp-tools/guru-connect/server/src/api/machine_keys.rs
+/d/claudetools/projects/msp-tools/guru-connect/server/src/api/mod.rs
+/d/claudetools/projects/msp-tools/guru-connect/server/src/api/releases.rs
+/d/claudetools/projects/msp-tools/guru-connect/server/src/api/removal.rs
+/d/claudetools/projects/msp-tools/guru-connect/server/src/api/sessions.rs
+/d/claudetools/projects/msp-tools/guru-connect/server/src/api/sites.rs
+/d/claudetools/projects/msp-tools/guru-connect/server/src/api/users.rs
+/d/claudetools/projects/msp-tools/guru-connect/server/src/auth/agent_keys.rs
+/d/claudetools/projects/msp-tools/guru-connect/server/src/auth/enrollment_keys.rs
+/d/claudetools/projects/msp-tools/guru-connect/server/src/auth/jwt.rs
+/d/claudetools/projects/msp-tools/guru-connect/server/src/auth/mod.rs
+/d/claudetools/projects/msp-tools/guru-connect/server/src/auth/password.rs
+/d/claudetools/projects/msp-tools/guru-connect/server/src/auth/token_blacklist.rs
+/d/claudetools/projects/msp-tools/guru-connect/server/src/auth/viewer_token_registry.rs
+/d/claudetools/projects/msp-tools/guru-connect/server/src/config.rs
+/d/claudetools/projects/msp-tools/guru-connect/server/src/db/agent_keys.rs

reports/review-2026-06-05/files-server-b.txt

@@ -0,0 +1,21 @@
+/d/claudetools/projects/msp-tools/guru-connect/server/src/db/enrollment_keys.rs
+/d/claudetools/projects/msp-tools/guru-connect/server/src/db/events.rs
+/d/claudetools/projects/msp-tools/guru-connect/server/src/db/machines.rs
+/d/claudetools/projects/msp-tools/guru-connect/server/src/db/mod.rs
+/d/claudetools/projects/msp-tools/guru-connect/server/src/db/releases.rs
+/d/claudetools/projects/msp-tools/guru-connect/server/src/db/sessions.rs
+/d/claudetools/projects/msp-tools/guru-connect/server/src/db/sites.rs
+/d/claudetools/projects/msp-tools/guru-connect/server/src/db/support_codes.rs
+/d/claudetools/projects/msp-tools/guru-connect/server/src/db/tenancy.rs
+/d/claudetools/projects/msp-tools/guru-connect/server/src/db/users.rs
+/d/claudetools/projects/msp-tools/guru-connect/server/src/main.rs
+/d/claudetools/projects/msp-tools/guru-connect/server/src/metrics/mod.rs
+/d/claudetools/projects/msp-tools/guru-connect/server/src/middleware/mod.rs
+/d/claudetools/projects/msp-tools/guru-connect/server/src/middleware/rate_limit.rs
+/d/claudetools/projects/msp-tools/guru-connect/server/src/middleware/security_headers.rs
+/d/claudetools/projects/msp-tools/guru-connect/server/src/relay/mod.rs
+/d/claudetools/projects/msp-tools/guru-connect/server/src/session/mod.rs
+/d/claudetools/projects/msp-tools/guru-connect/server/src/support_codes.rs
+/d/claudetools/projects/msp-tools/guru-connect/server/src/utils/ip_extract.rs
+/d/claudetools/projects/msp-tools/guru-connect/server/src/utils/mod.rs
+/d/claudetools/projects/msp-tools/guru-connect/server/src/utils/validation.rs

reports/review-2026-06-05/files-server.txt

@@ -0,0 +1,42 @@
+/d/claudetools/projects/msp-tools/guru-connect/server/src/api/auth.rs
+/d/claudetools/projects/msp-tools/guru-connect/server/src/api/auth_logout.rs
+/d/claudetools/projects/msp-tools/guru-connect/server/src/api/changelog.rs
+/d/claudetools/projects/msp-tools/guru-connect/server/src/api/downloads.rs
+/d/claudetools/projects/msp-tools/guru-connect/server/src/api/enroll.rs
+/d/claudetools/projects/msp-tools/guru-connect/server/src/api/machine_keys.rs
+/d/claudetools/projects/msp-tools/guru-connect/server/src/api/mod.rs
+/d/claudetools/projects/msp-tools/guru-connect/server/src/api/releases.rs
+/d/claudetools/projects/msp-tools/guru-connect/server/src/api/removal.rs
+/d/claudetools/projects/msp-tools/guru-connect/server/src/api/sessions.rs
+/d/claudetools/projects/msp-tools/guru-connect/server/src/api/sites.rs
+/d/claudetools/projects/msp-tools/guru-connect/server/src/api/users.rs
+/d/claudetools/projects/msp-tools/guru-connect/server/src/auth/agent_keys.rs
+/d/claudetools/projects/msp-tools/guru-connect/server/src/auth/enrollment_keys.rs
+/d/claudetools/projects/msp-tools/guru-connect/server/src/auth/jwt.rs
+/d/claudetools/projects/msp-tools/guru-connect/server/src/auth/mod.rs
+/d/claudetools/projects/msp-tools/guru-connect/server/src/auth/password.rs
+/d/claudetools/projects/msp-tools/guru-connect/server/src/auth/token_blacklist.rs
+/d/claudetools/projects/msp-tools/guru-connect/server/src/auth/viewer_token_registry.rs
+/d/claudetools/projects/msp-tools/guru-connect/server/src/config.rs
+/d/claudetools/projects/msp-tools/guru-connect/server/src/db/agent_keys.rs
+/d/claudetools/projects/msp-tools/guru-connect/server/src/db/enrollment_keys.rs
+/d/claudetools/projects/msp-tools/guru-connect/server/src/db/events.rs
+/d/claudetools/projects/msp-tools/guru-connect/server/src/db/machines.rs
+/d/claudetools/projects/msp-tools/guru-connect/server/src/db/mod.rs
+/d/claudetools/projects/msp-tools/guru-connect/server/src/db/releases.rs
+/d/claudetools/projects/msp-tools/guru-connect/server/src/db/sessions.rs
+/d/claudetools/projects/msp-tools/guru-connect/server/src/db/sites.rs
+/d/claudetools/projects/msp-tools/guru-connect/server/src/db/support_codes.rs
+/d/claudetools/projects/msp-tools/guru-connect/server/src/db/tenancy.rs
+/d/claudetools/projects/msp-tools/guru-connect/server/src/db/users.rs
+/d/claudetools/projects/msp-tools/guru-connect/server/src/main.rs
+/d/claudetools/projects/msp-tools/guru-connect/server/src/metrics/mod.rs
+/d/claudetools/projects/msp-tools/guru-connect/server/src/middleware/mod.rs
+/d/claudetools/projects/msp-tools/guru-connect/server/src/middleware/rate_limit.rs
+/d/claudetools/projects/msp-tools/guru-connect/server/src/middleware/security_headers.rs
+/d/claudetools/projects/msp-tools/guru-connect/server/src/relay/mod.rs
+/d/claudetools/projects/msp-tools/guru-connect/server/src/session/mod.rs
+/d/claudetools/projects/msp-tools/guru-connect/server/src/support_codes.rs
+/d/claudetools/projects/msp-tools/guru-connect/server/src/utils/ip_extract.rs
+/d/claudetools/projects/msp-tools/guru-connect/server/src/utils/mod.rs
+/d/claudetools/projects/msp-tools/guru-connect/server/src/utils/validation.rs

reports/review-2026-06-05/gemini-agent.md

@@ -0,0 +1,25 @@
+# GuruConnect agent review — gemini — 2026-06-05
+
+This code review of the GuruConnect agent (v2/SPEC-016/SPEC-018) identifies several critical and high-severity findings related to session security, privilege escalation, and resource management.
+
+### Prioritized Findings List
+
+| SEVERITY | FILE:LINE | FINDING | CONCRETE FIX |
+| :--- | :--- | :--- | :--- |
+| **CRITICAL** | `websocket.rs:43` | **Insecure Auth via Query String.** API keys and JWT tokens are passed in the URL query string. These are often logged by reverse proxies (nginx/IIS) and visible in process monitors, leading to session takeover or credential theft. | Move credentials to the `Authorization` header or the Protobuf handshake payload. |
+| **CRITICAL** | `sas_service.rs:136` | **Command Injection in SYSTEM SAS Service.** The named pipe server reads a string, trims it, and matches it against `"sas"`. However, if the command protocol evolves or is misused, a lack of strict length/format validation on a SYSTEM-owned pipe is a high-risk surface for local privilege escalation (LPE). | Use a fixed-size byte enum or a proper Protobuf-serialized command structure for IPC. |
+| **HIGH** | `update.rs:197` | **Insecure Auto-Update (No Signature).** Updates are verified via SHA-256 checksum sent over the same channel as the binary. A MITM attacker can serve a malicious binary + matching checksum. | Implement RSA/Ed25519 signature verification using an embedded public key before `install_update`. |
+| **HIGH** | `credential_store.rs:309` | **LPE via `icacls` Subprocess.** The agent calls `icacls.exe` to secure the `cak_` store. If an attacker places a malicious `icacls.exe` in the search path (before the agent's working directory or in a user-writable path), the SYSTEM service will execute it. | Use the absolute path `C:\Windows\System32\icacls.exe` or use Win32 `SetNamedSecurityInfoW` API directly. |
+| **HIGH** | `session.rs:498` | **Session DoS via Blocking Consent.** `prompt_consent` runs on `spawn_blocking`, but the main loop `await`s it. While heartbeats might survive a 60s timeout, a user ignoring the box stalls all other control messages (like Disconnect or AdminRestart). | Run the consent prompt as a background task that sends the response to the server without stalling the main message-processing loop. |
+| **MEDIUM** | `main.rs:260` | **Service Run Panic Boundary.** `run_managed_agent_service` uses `catch_unwind` but runs inside a `tokio` runtime. Panics in spawned tasks or certain async contexts may still escape or cause the runtime to hang without triggering the SCM recovery. | Ensure the top-level `block_on` is wrapped, but also add a custom `PanicHook` to ensure the process exits cleanly so the SCM can restart it. |
+| **MEDIUM** | `sas_service.rs:46` | **Pipe Squatting/Race.** The SAS service pipe uses a hardcoded name and `PIPE_UNLIMITED_INSTANCES`. An attacker starting before the service could squat the name. | Use `FILE_FLAG_FIRST_PIPE_INSTANCE` when creating the first pipe instance to ensure the service owns the name. |
+| **LOW** | `config.rs:335` | **Config Exposure.** `EmbeddedConfig` contains `api_key` and `enrollment_key`. While it's in the binary, it's easily extractable via strings/binwalk. | The spec mentions `cek_` is low-sensitivity, but `api_key` (legacy) should be scrubbed from the binary after the first successful enrollment. |
+
+---
+
+### Overall Assessment
+The codebase is well-structured and follows the SPEC-016/018 requirements closely, particularly the DPAPI integration and the transition to a SYSTEM service. However, it relies heavily on "security by obscurity" in its transport (credentials in URLs) and lacks a robust trust anchor for auto-updates. The use of shell-outs (`icacls`, `powershell`) from a SYSTEM context is a significant LPE risk vector.
+
+### Single Most Important Thing to Fix
+**Implement Cryptographic Signature Verification for Auto-Updates.** 
+The current checksum-only approach allows any attacker capable of intercepting the WebSocket or API traffic (e.g., via a compromised relay or DNS) to gain SYSTEM-level code execution on the entire agent fleet by pushing a "mandatory" update.

reports/review-2026-06-05/gemini-agent.md.err

@@ -0,0 +1,3 @@
+[ask-gemini] file not found: agent/src/bin/sas_service.rs
+EXIT=2 files=34
+EXIT=0 files=34

reports/review-2026-06-05/gemini-dashboard.md

@@ -0,0 +1,46 @@
+# GuruConnect dashboard review — gemini — 2026-06-05
+
+This code review focuses on the provided TypeScript/React dashboard files for GuruConnect. The codebase is generally well-structured with a strong emphasis on accessibility and clean state management using TanStack Query. However, several high-priority security and correctness issues were identified.
+
+### Prioritized Findings
+
+1. **[SEVERITY: HIGH] Non-Atomic User Updates**
+   *   **File:** `3_hooks.ts:50` (in `useUpdateUser`)
+   *   **Problem:** Updating a user's core profile and their permissions involves two sequential API calls (`updateUser` then `setUserPermissions`). If the first succeeds but the second fails (e.g., network drop, server error), the user is left in an inconsistent state. The UI attempts to warn the user, but this does not prevent the underlying data drift.
+   *   **Fix:** Implement a single atomic `PATCH /api/users/:id` endpoint on the server that accepts both core fields and permissions in one transaction. Update the frontend to use this single call.
+
+2. **[SEVERITY: HIGH] JWT Exposure in SessionStorage**
+   *   **File:** `AuthProvider.tsx:16`
+   *   **Problem:** The authentication JWT is stored in `sessionStorage`. While this prevents persistence across browser restarts, it remains fully accessible to any script on the same origin. An XSS vulnerability anywhere in the dashboard would allow an attacker to steal the operator's session token.
+   *   **Fix:** Move JWT handling to an `httpOnly`, `Secure`, `SameSite=Strict` cookie managed by the server. Remove token management from the frontend entirely, letting the browser attach the cookie automatically.
+
+3. **[SEVERITY: HIGH] Client-Side Enforcement of Security Guards**
+   *   **File:** `EditUserModal.tsx:48`
+   *   **Problem:** The logic preventing an admin from demoting themselves (which would lock them out of the admin plane) is implemented strictly in the frontend. The code explicitly notes that "the server ... does NOT currently block a self-role-demotion." Relying on the client for authorization logic is a security risk.
+   *   **Fix:** Implement the self-demotion and self-disable guards on the server within the `AdminUser` extractor or the update handler. The server must be the final authority on these state transitions.
+
+4. **[SEVERITY: HIGH] Sensitive Tokens in URL Query Strings**
+   *   **File:** `JoinSessionModal.tsx:34` (in `buildViewerUrl`)
+   *   **Problem:** The session-scoped viewer token is passed to the relay via a WebSocket query parameter (`token=...`). URLs are frequently logged by reverse proxies, load balancers, and browser history, potentially leaking the short-lived but powerful viewer tokens.
+   *   **Fix:** Pass the token via the `Sec-WebSocket-Protocol` header or a custom `Authorization` header if the WebSocket client/relay supports it. If query params must be used, ensure the relay is configured to never log the full URL.
+
+5. **[SEVERITY: MEDIUM] Hand-Maintained API Type Mirrors**
+   *   **File:** `types.ts:1`
+   *   **Problem:** All API response shapes are hand-maintained mirrors of the Rust source. This is highly prone to "type drift," where a backend change (e.g., a field rename or type change) is not reflected in the frontend, leading to silent runtime failures or `undefined` values in the UI.
+   *   **Fix:** Use a tool like `ts-rs` or `specta` in the Rust backend to automatically generate TypeScript definitions from the actual API structs. This ensures the frontend and backend are always in sync.
+
+6. **[SEVERITY: MEDIUM] Premature Session Clearing on Network Failure**
+   *   **File:** `AuthProvider.tsx:55` (in `restore`)
+   *   **Problem:** During the initial app load, if the `getMe` call fails for *any* reason (including a transient network error with status 0), the `clearSession()` function is called, wiping the user's token. A technician on a flaky connection would be logged out repeatedly.
+   *   **Fix:** Only call `clearSession()` if the error status is explicitly `401 Unauthorized`. For network errors (status 0) or server errors (5xx), the app should keep the token and show a "Retry" or "Offline" state.
+
+7. **[SEVERITY: LOW] Deprecated Clipboard API Usage**
+   *   **File:** `useClipboard.ts:25`
+   *   **Problem:** The hook uses `document.execCommand("copy")` as a fallback for non-secure contexts. This API is deprecated and has inconsistent support in modern browsers.
+   *   **Fix:** Rely exclusively on the `navigator.clipboard` API. Since the dashboard should only be served over HTTPS, the "non-secure context" fallback is largely unnecessary.
+
+### Overall Assessment
+The GuruConnect dashboard is architecturally sound and leverages modern React best practices effectively. The UI components are robust and accessibility-conscious. However, the security model relies too heavily on client-side logic and `sessionStorage`, which are vulnerable points in a remote-access platform. The most critical area for improvement is moving security-sensitive state transitions (like user role management) and session handling (JWT storage and token transmission) to more secure, server-enforced patterns.
+
+### The Single Most Important Fix
+**Move the JWT to an `httpOnly` cookie.** This single change significantly hardens the platform against session hijacking via XSS, which is the most likely attack vector for a browser-based support console.

reports/review-2026-06-05/gemini-dashboard.md.err

@@ -0,0 +1,3 @@
+[ask-gemini] file not found: dashboard/src/App.tsx
+EXIT=2 files=66
+EXIT=0 files=66

reports/review-2026-06-05/gemini-server.md

@@ -0,0 +1,29 @@
+# GuruConnect server review — gemini — 2026-06-05
+
+I have completed an independent code review of the GuruConnect server based on the provided Rust source files. Below is the prioritized list of findings, categorized by severity, followed by an overall assessment and the most critical recommendation.
+
+### **Priority Findings List**
+
+| SEVERITY | FILE:LINE | FINDING | CONCRETE FIX |
+| :--- | :--- | :--- | :--- |
+| **CRITICAL** | `downloads.rs:142` | **Insecure Default API Key:** The permanent agent download endpoint uses a hardcoded default `"managed-agent"` API key if none is provided. This is a weak, well-known credential that grants any agent immediate persistent access if enabled on the server. | Remove the default fallback. Require a valid, high-entropy API key or a session-scoped enrollment key for all permanent agent downloads. |
+| **CRITICAL** | `main.rs:163` | **Sensitive Credentials in Local File:** On first run, the server writes the initial admin password to `.admin-credentials`. While it attempts to set `0o600` permissions, this file remains a high-value target for local privilege escalation or accidental exposure in backups/logs. | Provide the initial admin password via a one-time environment variable or secure secret manager. If a file must be used, ensure it is deleted automatically after the first successful login. |
+| **HIGH** | `main.rs:430+` | **Missing Rate Limiting on Admin CRUD:** Sensitive admin endpoints (`/api/users`, `/api/releases`, etc.) lack per-IP rate limiting. A compromised low-privilege account or a brute-force attack on the admin plane could cause DoS or data corruption. | Apply the `RateLimiter` middleware to all `/api/users`, `/api/releases`, and `/api/sites` management routes. |
+| **HIGH** | `removal.rs:104` | **TOCTOU in Machine Removal:** The legacy hard-delete path for machines performs a `get_machine_by_agent_id` lookup, then an optional `uninstall` command, then a `DELETE`. A machine could reconnect or be modified between the lookup and the delete, leading to inconsistent state or missed audits. | Wrap the removal logic in a database transaction and use `SELECT ... FOR UPDATE` or conditional `DELETE` statements (e.g., `WHERE agent_id = $1 AND updated_at = $2`) to ensure atomicity. |
+| **HIGH** | `relay.rs:664` | **Unbounded Input Queue in Viewer:** The viewer's input channel has a capacity of 64 messages. While `try_send` is used (dropping excess), a large number of viewers could still pressure the agent's inbound bandwidth if not throttled correctly at the broadcast level. | Implement a global per-session input rate limit (not just per-viewer) to protect the agent from aggregate input flooding by multiple technicians. |
+| **MEDIUM** | `auth.rs:320` | **Weak Password Policy:** The server only enforces a minimum length of 8 characters for new passwords. It does not check for complexity, common patterns, or use a "pwned password" list. | Integrate a password strength library (e.g., `zxcvbn`) and reject common passwords. Increase the minimum length to 12+ characters. |
+| **MEDIUM** | `jwt.rs:207` | **Lack of Token Refresh Mechanism:** The server relies on 24-hour JWTs with no explicit refresh token flow. This encourages long-lived sessions and increases the impact of a stolen token. | Implement short-lived access tokens (e.g., 15 mins) and long-lived refresh tokens stored securely (e.g., `HttpOnly` cookies). |
+| **MEDIUM** | `downloads.rs:188` | **Path Sanitization Limit:** `sanitize_filename` truncates at 32 characters but doesn't check for reserved Windows filenames (e.g., `CON`, `PRN`, `NUL`). | Use a more robust sanitization library or explicitly reject reserved Windows filenames to prevent issues on the client-side download. |
+| **LOW** | `rate_limit.rs:94` | **In-Memory Rate Limit Persistence:** Rate limit state is purely in-memory. A server restart clears all lockouts/counters, allowing an attacker to resume brute-forcing immediately. | (Optional) Periodically persist the `RateLimitState` to the database or use a fast-access cache like Redis for distributed/persistent rate limiting. |
+| **LOW** | `events.rs:252` | **Audit Log Growth:** The `connect_session_events` table grows indefinitely with every connection and admin action. There is no automated pruning or archival logic visible. | Implement a background task to prune or archive audit events older than a configured retention period (e.g., 90 days). |
+
+---
+
+### **Overall Assessment**
+The GuruConnect server demonstrates a strong focus on security fundamentals, particularly in its use of **Argon2id** for password/enrollment hashing, **SHA-256** for per-agent keys, and the implementation of a **session-scoped viewer token** model that successfully closes common relay-hijacking vulnerabilities. The architecture is modular and tenancy-ready, and the "fail-closed" approach to authentication (especially in the `agent_ws_handler`) is commendable.
+
+However, the presence of an insecure default API key in the download path and the reliance on local files for sensitive bootstrap credentials represent significant "low-hanging fruit" for attackers. The concurrency controls in the removal paths also show potential for TOCTOU races that could complicate auditing and state management in high-load environments.
+
+### **Single Most Important Thing to Fix**
+**Remove the default `"managed-agent"` API key fallback in `downloads.rs:142`.** 
+This is a "backdoor" by default; any permanent agent download without an explicit key results in a credential that an attacker can predict and use to attempt unauthorized machine registration if the server's `AGENT_API_KEY` environment variable happens to match this weak default. All credentials should be high-entropy and unique.

reports/review-2026-06-05/gemini-server.md.err

@@ -0,0 +1,3 @@
+[ask-gemini] file not found: server/src/api/auth.rs
+EXIT=2 files=42
+EXIT=0 files=42

reports/review-2026-06-05/grok-agent.md

@@ -0,0 +1,83 @@
+# GuruConnect agent review — grok — 2026-06-05
+
+**All required files read via `read_file` (absolute paths, full content via large `limit` where needed; identity.json read first per project rules). No other files explored, no modifications made, no further tools used after the mandated reads. Review strictly limited to the 34 listed source files + identity.**
+
+**User context (from mandatory identity read):** Mike Swanson (mike, admin) on GURU-5070.
+
+### Prioritized Findings
+
+**[SEVERITY: CRITICAL] transport/websocket.rs:44-66 (and cross-references: main.rs:389 (support_code path), config.rs:441 (default), session/mod.rs:119 (connect call), viewer/transport.rs:65 (viewer token), install.rs:462 (protocol URL token), enroll.rs:195 (body but related))**
+
+Problem: Authentication material (`api_key` (post-enroll `cak_`), `support_code`, `machine_uid`, hostname, `agent_id`) is placed directly into the WebSocket connection URL as query parameters (`agent_id=...&api_key=...&support_code=...&machine_uid=...`). This is done before `connect_async`. Even over `wss://`, query strings are commonly logged (server access logs, reverse proxies, CDNs, browser history/debug tools, Referer leakage). `support_code` is the binding secret for attended/temp sessions; `cak_` (as `api_key`) is the long-lived per-machine operating credential. No post-TLS initial auth frame or subprotocol is used.
+
+Concrete fix: Perform the unauthenticated WS connect (or use a distinct unauth bootstrap), then immediately send the first protobuf `Message` carrying credentials (or a signed token) as binary payload. Alternatively, use a WebSocket subprotocol for auth handshake or HTTP upgrade header (where supported). Remove all secrets from the URL. Update all call sites and the server contract.
+
+**[SEVERITY: CRITICAL] transport/websocket.rs:104-143 (try_recv) + 147-169 (recv); session/mod.rs:385-392 (call site in main loop); viewer/transport.rs:27-60 (similar pattern)**
+
+Problem: `try_recv` does `tokio::task::block_in_place(|| Handle::current().block_on(async { ... timeout(..., stream.next()) }))` (and `recv` does direct block_on under lock). This is called from the main session loop (`run_with_tray`) on every iteration and from viewer receive task. Anti-pattern in async (especially current-thread or work-stealing runtimes); risks thread starvation, deadlocks under contention, or breaking cooperative scheduling. The 1ms timeout poll + VecDeque buffering compounds it. `connected` flag is racy with the stream lock.
+
+Concrete fix: Use purely async non-blocking patterns (e.g., `tokio::time::timeout` directly on the locked stream without `block_in_place`, or a dedicated recv task that forwards over an mpsc channel to the session). Make `WebSocketTransport` fully async-friendly; expose a proper `Stream` or async `recv` only.
+
+**[SEVERITY: HIGH] session/mod.rs:625 (handle_consent_request) + 602-656 (the await + comment) + 337-359 (main loop) + 441-454 (heartbeats only in Idle)**
+
+Problem: `handle_consent_request` does `spawn_blocking` for the `MessageBoxW` (correct for blocking UI) but the caller `.await`s it directly inside the main `run_with_tray` loop. During the entire user think-time (up to server CONSENT_TIMEOUT_SECS ~60s), the loop is suspended: no heartbeats, no status, no `try_recv` processing, no tray polling, no service_shutdown checks. Comments acknowledge this but claim it's "safe" because of server timeouts; the loop also does not send heartbeats while in the consent path (only Idle/Streaming branches do). A slow/denied consent can cause server-side timeout of the agent or missed stop signals.
+
+Concrete fix: Drive consent off the main loop (e.g., spawn a task that owns the transport send for the response only; keep the loop pumping heartbeats/status/service flag via a separate small state machine or channel). Or restructure so consent does not block the pump.
+
+**[SEVERITY: HIGH] config.rs:264-282 (extract_support_code) + 248-252 (RunMode::TempSupport) + 369-372 (main) + session/mod.rs:782 + 839 (is_support_session) + transport/websocket.rs:57-59 (passes in query) + main.rs:301-304 (filename detection)**
+
+Problem: 6-digit numeric support codes are extracted from the executable filename (split on `-`/`_`/`.` or last 6 chars) with no checksum, expiry, or strength check in the agent. The code is then passed verbatim in the WS URL query for session binding. Filename-based secrets are easily guessable/brute-forceable (especially if server has no strong rate limiting or short TTL on codes) and leak via process listings, installers, or shared binaries. No binding proof (e.g., HMAC) is added client-side.
+
+Concrete fix: Treat support codes as high-entropy one-time secrets only (server-enforced); do not derive from or embed in filenames for production attended sessions. Add client-side length/charset validation and consider a short MAC over the code + nonce if the wire protocol allows. Document that binding/enforcement is server-side only.
+
+**[SEVERITY: HIGH] main.rs:548-554 + 570-575 (LoadCakError paths in resolve_agent_credential) + credential_store.rs:169-195 (load_cak) + 109-154 (store_cak with C1 readback) — good intent but ...; cross with service/mod.rs:414 (run_managed_agent_service) and main.rs:482 (run_permanent_agent_managed)**
+
+Problem: The detailed `LoadCakError` classification (Io permission_denied vs. Decrypt tamper vs. plain Io) and fail-fast "refuse to re-enroll" logic is excellent and correctly wired for the SYSTEM service path. However, on the interactive `PermanentAgent` first-run path (`run_permanent_agent_managed`), if the service install fails (not elevated), it falls back to `run_agent_mode(None)` which will hit the same "store present but unreadable" or "no credential" error and can still attempt enrollment in some flows. The ACL is applied via `icacls` Command (credential_store.rs:345) after `create_dir_all` but before secret write (good TOCTOU ordering), yet any failure in `icacls` (localized SIDs, path quoting, policy) silently leaves a weaker store. `store_cak` does immediate C1 readback, which is strong.
+
+Concrete fix: Make the interactive managed fallback path also refuse enrollment if a store exists but is unreadable in-context (surface the exact `LoadCakError` message). Add verification that the post-`icacls` ACL actually denies non-admins (test read as low user) or at least log the raw `icacls` stdout on failure. Consider a pure-Win32 ACL path for the directory as belt-and-suspenders.
+
+**[SEVERITY: MEDIUM] encoder/h264.rs:368-375 (ProcessInput) + 310 (ProcessOutput) + 236-252 (reinit) + many unsafe blocks; decoder.rs:176-204 (similar NOTACCEPTING/NEED_MORE_INPUT/ STREAM_CHANGE dance) + 278-297 (negotiate loop with `negotiated` guard); encoder/mod.rs:105-132 (factory fallback)**
+
+Problem: The H.264 MF paths are heavily unsafe COM with manual streaming state (`streaming`, stream IDs, force_keyframe). The first-cut synchronous drain/retry logic for `MF_E_NOTACCEPTING` / `MF_E_TRANSFORM_NEED_MORE_INPUT` / `STREAM_CHANGE` / `TYPE_NOT_SET` is complex and has a "negotiated" guard to avoid infinite loops, but a single missed state transition, dimension change mid-stream, or MFT that behaves differently on real hardware can drop frames or panic/unwrap in the unsafe paths. Encoder factory falls back to raw on init error (good), but per-frame errors in `encode` surface and can stall the session loop (session/mod.rs:552: `if let Ok(encoded) = ...` silently drops). No sequence number or integrity on the wire frames beyond protobuf.
+
+Concrete fix: Add defensive checks (e.g., never trust MFT-reported sizes without validation against capture; bound the drain loops). Make H.264 failures increment a counter and force a codec renegotiation or hard switch to raw after N consecutive errors instead of per-frame silent drop. Add end-to-end frame sequence + checksum (or rely on transport) for wire integrity. Validate all `unwrap_or` / `expect` in the MF paths.
+
+**[SEVERITY: MEDIUM] session/mod.rs:153-187 (panic::catch_unwind around capture::primary_display / create_capturer / encoder / InputController) + 551 (capture in loop); capture/dxgi.rs:332 (Drop ReleaseFrame) + gdi.rs (manual ReleaseDC etc.); many `.ok()` on Release**
+
+Problem: Capture/encoder/input init is wrapped in `catch_unwind` to prevent a bad driver/DXGI from taking down the whole agent (good pragmatism), with GDI fallback. However, the runtime capture path (`capturer.capture()`) is not protected the same way; a panic there drops the frame but the `?` / `if let` in the loop can leave state inconsistent. DXGI/GDI Drops do best-effort `.ok()` ReleaseFrame / DeleteObject etc.; a prior Acquire without matching Release (or double-release on error paths) can leak or crash the device. `last_frame` in DxgiCapturer is dead.
+
+Concrete fix: Wrap the per-frame `capture()` + `encode()` in the streaming branch with a similar catch_unwind + "mark capturer bad, fall back or idle" recovery. Make resource release strict (e.g., use scopeguard or ensure ReleaseFrame is called exactly once per successful Acquire, even on early returns). Remove dead `last_frame`.
+
+**[SEVERITY: MEDIUM] config.rs:290-348 (read_embedded_config) + 314 (rposition for MAGIC_MARKER) + 325 (length from bytes) + 338 (serde_json::from_slice); 383 (load priority); main.rs:300 (has_embedded_config check)**
+
+Problem: Executable is mutated by appending `GURUCONFIG` + u32 LE length + JSON. The reader seeks from end (last 64KB), does `rposition`, then trusts the length field and does an unchecked slice + `from_slice`. A malicious or corrupted appended blob (truncated length, huge length, or JSON that deserializes to attacker-controlled `server_url` + `enrollment_key`) can cause the agent to connect to an attacker relay or exfiltrate the machine. No signature, no upper bound on length beyond the search buffer, no validation that the marker is in the expected "appended" region (post-PE).
+
+Concrete fix: Add a strict max size for embedded config (e.g., 16KB). After locating the marker, validate that the config_start is after the PE end (or at least sanity-check the JSON fields: server_url must be wss/https to a known allowlist, enrollment_key length/format). Prefer an embedded signature (ed25519 over the config blob) verified at load time before trusting any enrollment material or server_url.
+
+**[SEVERITY: MEDIUM] identity.rs:249-331 (run_powershell + CIM queries for machine_uid) + 192 (primary_disk_serial script) + 221 (query_cim_property); 289 (kill on timeout)**
+
+Problem: `machine_uid` (used for enrollment dedup + connect identity) is derived by shelling out to `powershell.exe -Command "..."` (hardcoded scripts, no user data interpolated — good) with a 10s wall-clock timeout + kill. Output is captured but never logged (good). However, the scripts rely on CIM/WMI providers that can be slow, disabled, or return localized data; `primary_disk_serial` parses `Index<TAB>serial` with `splitn`. A wedged provider or unexpected output can cause fallback to `persisted_uid` (random + file). On re-image the MachineGuid path is intentionally volatile (documented), but the whole path is used for server-side collision/revocation decisions.
+
+Concrete fix: Add stronger output validation (e.g., serials must look like plausible hardware IDs, not empty after normalize). Consider a pure-Rust or narrow WMI binding for the two scalar reads to remove the shell dependency entirely. Cache failures explicitly and surface "machine_uid used fallback (see logs)" in AgentStatus so operators know the dedup signal is weak.
+
+**[SEVERITY: MEDIUM] update.rs:163-184 (verify_checksum) + 318 (perform_update) + 189 (TODO comment) + 63 (dev_insecure_tls in download); same pattern in enroll.rs:285-304 + 296 (dev_insecure_tls)**
+
+Problem: Update integrity is a SHA-256 checksum delivered over the *same* channel as the binary (explicitly noted in comments as "transport integrity, not tamper defense"). `dev_insecure_tls` (debug + env only) disables cert verification for the version check + download. On success the old binary is renamed to `.old` and new copied in place while running; restart is detached. No signature verification (the TODO acknowledges this). A compromised/evil server (or MITM when insecure) can serve a matching checksum + malicious binary.
+
+Concrete fix: Add (or implement the TODO for) an embedded public key + signature over the manifest + binary (or at minimum over the checksum + version). Keep the TLS bypass strictly debug-only and consider removing the env escape hatch. Verify signature *before* `install_update`.
+
+**[SEVERITY: LOW] (multiple files) pervasive `#[allow(dead_code)]`, `unwrap_or_default`, best-effort `.ok()`, and incomplete paths (chat, multi-display dirty rects, VP9/HEVC, X buttons, etc.); config.rs:442 (`"dev-key"` default); hard-coded prod URLs in several places.**
+
+Problem: Indicates incomplete implementation surface. Dead paths + silent ignores can hide bugs. The dev-key default in env fallback is a footgun if a managed config load fails.
+
+Concrete fix: Audit and either implement or remove the dead_code items. Remove or guard the `"dev-key"` default behind a stronger "debug build only" + explicit warning. Centralize server URLs.
+
+### Overall Assessment
+
+The reviewed agent code demonstrates strong awareness of the Windows security model (SYSTEM service for SAS + managed agent, DPAPI+ACL credential store with readback verification + TOCTOU ordering, explicit consent for attended sessions via topmost MessageBox, tiered SAS delivery, panic-catch around risky capture/encode init, modifier release on focus loss/blur, focus-gated global keyboard hook for system keys, re-image-stable `machine_uid` derivation with documented fallbacks, and detailed error classification for enrollment/credential paths). The protobuf wire protocol, idle-vs-streaming resource release, codec negotiation with hardware guard + raw fallback, and heartbeat/status discipline are all reasonable. Many sharp edges (COM lifetime, DXGI access lost, MF streaming back-pressure) have explicit handling or guards.
+
+However, the implementation is still "first-cut / Task N" in several critical areas (H.264 is compile-verified only; many `#[allow(dead_code)]`; async/sync mixing is fragile). The review surface is entirely client-side; trust is placed heavily in the (unreviewed) relay for session binding, consent gating, revocation, and auth enforcement.
+
+**The single most important thing to fix:** Move all authentication and session-binding material (`api_key`/`cak_`, `support_code`, `machine_uid`) out of WebSocket URL query parameters (transport/websocket.rs and all call sites) into a post-connect authenticated protobuf message (or proper WS subprotocol). This is the highest-leverage security win: it directly mitigates secret leakage that enables session takeover, unauthorized access, and impersonation across both persistent and attended flows, independent of TLS. Everything else (async hygiene, consent loop suspension, H.264 robustness, update signing) is important but secondary to not putting the crown jewels in the most commonly logged part of the connection. 
+
+All findings are derived strictly from the provided file contents.

reports/review-2026-06-05/grok-agent.md.err

@@ -0,0 +1,3 @@
+[ask-grok] file not found: agent/src/bin/sas_service.rs
+EXIT=2 files=34
+EXIT=0 files=34

reports/review-2026-06-05/grok-dashboard.md

@@ -0,0 +1,111 @@
+# GuruConnect dashboard review — grok — 2026-06-05
+
+**Independent code review: GuruConnect dashboard (TypeScript/React operator console)**
+
+All 66 files were read via `read_file` (absolute paths exactly as specified). Review is strictly limited to the provided files. No modifications were made. Backend (Rust relay, protobuf wire protocol, DB, JWT minting/claims, session manager, support-code redemption, SQL, agent auth) is out of scope; findings are restricted to what is observable or implied by the dashboard code + its API client.
+
+Findings are prioritized (CRITICAL → HIGH → MEDIUM → LOW). Format per request: `[SEVERITY: X], file:line, problem, concrete fix`. Focus areas were applied literally. Only concrete, evidenced issues are listed.
+
+### CRITICAL
+
+**[SEVERITY: CRITICAL], dashboard/src/features/sessions/JoinSessionModal.tsx:29-36**  
+The short-lived viewer token (which the relay uses to grant `control` or `view_only` access to a live remote session, binding to a specific `session_id`) is placed directly into the query string when building the WS viewer URL:
+
+```ts
+const qs = new URLSearchParams({
+  session_id: token.session_id,
+  viewer_name: viewerName,
+  token: token.token,   // !!!
+});
+return `${wsProto}//${window.location.host}/ws/viewer?${qs.toString()}`;
+```
+(See: buildViewerUrl, the effect that calls mintViewerToken, the "Relay viewer URL" FieldReveal copy target, JoinSessionModal:108-112 comments, sessionsApi.mintViewerToken, and the fact that the relay reads ViewerParams from query per the comment.)
+
+**Problem**: Query-string tokens appear in browser history, server access logs (relay + any reverse proxy), proxy logs, copy-paste artifacts, and can be bookmarked/shared. This is the capability that actually attaches a viewer to an agent's screen/stream. Even with short `expires_in_secs`, a leaked token for a live attended or managed session enables immediate session viewing or takeover. The client has no additional binding (e.g., origin, viewer IP) visible here.
+
+**Fix**: Move the token out of the query string. Preferred: pass via `Authorization: Bearer` header on the WS upgrade or a subprotocol (`Sec-WebSocket-Protocol`). If the current relay wire format requires query params for the viewer plane, (a) make the minted token one-time or single-use after first successful attach, (b) bind it to the caller's IP or a very short TTL (<< current), and (c) document the exposure + require operators to treat the entire URL as a single-use secret. At minimum, stop surfacing the full URL as a casually copyable field.
+
+**[SEVERITY: CRITICAL], dashboard/src/auth/AuthProvider.tsx:7 (STORAGE_KEY), 16 (sessionStorage.getItem + tokenRef), 66-73 (login), 42-64 (restore), 75-84 (logout), combined with client.ts:86-87 and all privileged API calls**  
+The primary dashboard JWT (used for `GET /me`, listing machines/sessions/codes, `POST /codes`, `POST /sessions/:id/viewer-token` (the control/view mint), admin user/machine operations, etc.) has no client-visible expiration handling, refresh, max lifetime, or idle timeout. It lives in `sessionStorage` (survives reloads within the tab lifetime) + an in-memory ref and is blindly attached as `Bearer` on every request.
+
+**Problem**: Compromise of the operator workstation (XSS in the console, malware, devtools exfil, physical access while tab is open, or shoulder-surf of a token during debugging) yields long-lived privileged access. Because `mintViewerToken` re-uses the same bearer, the attacker can also obtain per-session control/view tokens for any machine/session the account can see. Logout is best-effort; 401 is the only forced teardown. No `exp` parsing or proactive re-auth is present.
+
+**Fix**: Server should issue short-lived access tokens (or rotate the main token) + refresh tokens (or a refresh endpoint that requires the old token + rotation). Client must: (a) parse and respect `exp` (or an explicit `expires_in`) from LoginResponse, (b) proactively refresh or force logout before expiry, (c) add an idle/activity timeout that clears the session after N minutes of inactivity, and (d) consider binding the token to the browser tab lifetime more strictly. Treat the current long-lived bearer as equivalent to persistent remote access creds.
+
+### HIGH
+
+**[SEVERITY: HIGH], dashboard/src/features/codes/api/codes.ts:37-38 (cancelCode) + SupportCodesPage.tsx:21-24 (canCancel) + 114-136 (actions) + GenerateCodeModal + hooks**  
+Support code value (the `XXX-XXX-XXX` string read aloud to the end user, the single-use bearer for attended session start) is used as a path segment: `` `/api/codes/${encodeURIComponent(code)}/cancel` ``. The client also renders `code.session_id`, `client_machine`, `client_name`, `connected_at`, and consent state directly from the polled list response. No additional client-side binding verification or proof-of-possession step exists before offering cancel or trusting "connected" state.
+
+**Problem**: An attacker with a dashboard JWT (see CRITICAL auth finding) can enumerate active codes via `listCodes` and cancel pending/connected ones (DoS on support workflows) or observe which codes have bound to which sessions/machines. The code-to-session binding and single-use guard are entirely server-side; the dashboard is a pure reflection of whatever the `/codes` and `/sessions` responses claim. A server-side race or mis-binding between code redemption and session creation would be invisible to (and unmitigated by) the client.
+
+**Fix**: (Defense-in-depth on client) Treat the human code as a high-value secret in the UI: minimize how long it is rendered in clear, add explicit "this code is now bound to session X for machine Y — confirm before cancel" extra confirmation when status==="connected", and surface the exact `session_id` + consent_state with a warning on the cancel dialog. Server must remain authoritative for binding and single-use enforcement; client should not be the only thing preventing reuse.
+
+**[SEVERITY: HIGH], dashboard/src/features/machines/MachinesPage.tsx:56 (isAdmin), 228-250 (conditional Keys/Remove buttons), 380-396 (modals), UsersPage.tsx:42+161 (self checks), EditUserModal.tsx:92-120 (lockSelfDemotion + isSelf guards), AdminRoute.tsx:15-56, AuthContext.tsx:10+93 (isAdmin = role === "admin"), and equivalent hasPermission checks throughout**  
+All privileged UI affordances (bulk remove, per-agent key mint/revoke, user create/edit/delete, admin routes) are gated exclusively by client-side `isAdmin` (exact string compare on `user.role`) or `hasPermission(p)` (`user?.permissions.includes(p) ?? false`). These derive the `canControl`/`canView` passed to JoinSessionModal and the selection rails.
+
+**Problem**: These are only UI hints. A desync (role string casing, permission array containing non-canonical values, future role like "super-admin", or a bug in the AuthProvider memo) can show/hide actions incorrectly. Self-lockout guards (disable own account, demote self from admin) are client-only in the modal; the server comment in the code acknowledges it does not currently block self-demotion. While the server re-enforces (AdminUser extractor, 403s, self-delete 400), the client state is what the operator sees and what drives which buttons are even rendered.
+
+**Fix**: Keep the client gates but make them derive from a single, narrow, well-commented `can*` helper that exactly mirrors the documented server rules (or fetch effective capabilities from `/me` or a dedicated endpoint). Add explicit client-side rejection + toast for the self-demotion/disable cases even if the server would later 4xx. Widen the `Role | string` / `Permission | string` handling only after logging the unexpected value.
+
+### MEDIUM
+
+**[SEVERITY: MEDIUM], dashboard/src/api/client.ts:65-82 (extractError), 108-113 (401 path), 118-127 (success body handling) + callers that surface raw messages**  
+Error normalization handles two envelopes but falls back to raw `res.text()` (trimmed if <300 chars) for non-JSON and for certain machines routes (plain `&'static str`). Mutation error toasts (e.g. DeleteMachineDialog, BulkRemove, users hooks) directly render `err.message`.
+
+**Problem**: Server error bodies can leak internal details, hostnames, IPs (from events), or implementation strings into the operator UI and any logs/toasts that capture them. No redaction or classification of error text is performed before display. 401 after the handler still throws, which is mostly fine but means some error paths can double-surface.
+
+**Fix**: In `extractError` (or a post-processing step in the toast surfaces), treat unknown/plain-text errors as opaque. Prefix or categorize them ("Server error: ...") rather than dumping the body verbatim. Review the Rust error responses for machines/users/sessions/codes to ensure they never emit sensitive fields on failure paths.
+
+**[SEVERITY: MEDIUM], dashboard/src/features/machines/MachinesPage.tsx:372 (rowKey={(m) => m.id}), 66-88 (selected Set keyed by agent_id + reconciliation), 78-88 (useEffect that reconciles against live agent_ids), hooks.ts:12-18, api/machines.ts:18-19 (paths use agent_id), and Machine type (both `id` and `agent_id` present)**  
+React table keys use `m.id` while selection state, bulk operations, detail drawers, key modals, remove flows, filters, and all API paths consistently use `agent_id`. Reconciliation logic keys off `agent_id`.
+
+**Problem**: If `id` (DB/synthetic) and `agent_id` (agent-reported stable identity) can ever diverge in lifetime or uniqueness (soft-delete + re-enroll, purge paths, historical rows), React list reconciliation, selection survival across polls, and "remove selected" correctness become fragile. The bulk bar and dialogs already have careful visible-set logic; a key mismatch adds another surface for state skew.
+
+**Fix**: Standardize on the stable business key (`agent_id`) for `rowKey`, React keys, and selection. Keep `id` only for display or history joins if the server distinguishes them. Add a comment in types.ts and the page explaining the two identifiers.
+
+**[SEVERITY: MEDIUM], dashboard/src/features/users/hooks.ts:58-85 (useUpdateUser — two sequential awaits), 82 (onSettled always invalidates) + EditUserModal:122-134 (sendPermissions logic) + types.ts:99-104 (UpdateUserRequest)**  
+Update is split (core fields then optional permissions PUT). On permissions failure it throws a synthetic Error; `onSettled` (not `onSuccess`) invalidates.
+
+**Problem**: Partial success window exists (role/enabled changed, permissions not yet or failed). The error message rewrite is good, but an operator who closes the modal or navigates away during the window sees inconsistent state until the next refetch. No client-side transaction or "permissions pending" indicator.
+
+**Fix**: Either make the server accept a single combined update, or surface an explicit "core saved; retry permissions" state in the modal (keep it open, highlight the PermissionsField) instead of fully rejecting the mutation. The current `onSettled` + rewritten error is already better than most, but the UX for the partial case is still a toast + stale-looking row until poll.
+
+### LOW / Architecture & Maintainability
+
+**[SEVERITY: LOW], dashboard/src/api/stubs.ts:1-18 + api/index.ts:6 (export * as stubsApi)**  
+Stubs for sessions/users that return `unknown[]` are still exported under `stubsApi` (with comments "Pass 1", "do NOT build UI against these yet"). Real implementations live in sessions.ts / users.ts and are imported directly by features.
+
+**Problem**: Dead/exported-but-unused surface that can mislead readers or cause accidental import of the stub versions. Minor namespace pollution.
+
+**Fix**: Delete or clearly mark as `@deprecated / internal scaffolding` and remove from the barrel export. (Trivial.)
+
+**[SEVERITY: LOW], dashboard/src/features/*/hooks.ts (multiple): refetchInterval + staleTime hardcoded (machines 20s/10s, sessions 8s/4s, codes 7s/3.5s, health 15s), no `enabled: !document.hidden` or jitter, no error backoff**  
+Polling is aggressive and constant.
+
+**Problem**: Unnecessary load on the relay when the tab is backgrounded or the operator is idle on another page. No adaptive behavior.
+
+**Fix**: Add a small `useVisibility` or `document.visibilityState` guard to pause refetchInterval when hidden. Consider light jitter or a single shared "activity" signal. Keep the intervals tight for consent/watch use cases but make them configurable or at least documented.
+
+**[SEVERITY: LOW], dashboard/src/api/types.ts (entire file, especially ROLES/PERMISSIONS/ROLE_DEFAULT_PERMISSIONS, all the `| string` widenings, hand-maintained comment at top) + consumers that do exact string compares or `.includes`**  
+Types are manually kept in sync with server/src/api/*.rs. Many fields are widened to tolerate drift.
+
+**Problem**: Classic source-of-truth split. A new role/permission, a renamed field, or a new CodeStatus will either cause runtime surprises or require coordinated changes across Rust + this file. The defensive `| string` + filter-to-canonical in EditUserModal etc. mitigate but do not eliminate the maintenance tax.
+
+**Fix**: (Long-term) Generate the TS types from the Rust API (OpenAPI, prost, or a small schema export). In the interim, add a test or build step that diffs the TypeScript literals against a checked-in snapshot from the server.
+
+Additional minor notes (not elevated to findings):
+- `useClipboard` execCommand fallback is deprecated but acceptable for an internal tool in non-secure contexts.
+- No `dangerouslySetInnerHTML`; React text is used safely.
+- Dialog stack + focus trap + inert + restore in Modal/Drawer is correctly implemented and shared.
+- Password generation (Web Crypto + rejection sampling) + one-time copy + wipe-on-close is done well.
+- `encodeURIComponent` is consistently used on all path segments (agent_id, code, session id, user id, key id).
+- StrictMode double-mint guards in GenerateCodeModal and JoinSessionModal are present and correct.
+
+### Overall Assessment
+
+The dashboard is a clean, well-factored React + react-query SPA with strong separation between API client, typed feature hooks, and presentational components. UI/UX details around one-time secrets (codes, keys, passwords, viewer tokens), consent state, self-lockout, stacked dialogs, and partial-failure reconciliation are thoughtful and above average for an internal operator tool. The posture "client renders what the server says; server is the authority" is consistently followed (AdminRoute, permission checks, etc. are treated as hints + defense-in-depth).
+
+The code is skeptical of its own state in the right places (reconciliation effects, mintedFor refs, onSettled invalidation, `| string` widenings) but trusts the authenticated API responses for the security-critical bindings (code → session, viewer token access mode, consent_state, machine ownership).
+
+**Single most important thing to fix**: The CRITICAL placement of viewer tokens (the actual remote-control capability tokens) into query strings in `JoinSessionModal.buildViewerUrl`. This is the direct bridge from the operator console to live agent sessions. Everything else (auth lifetime, client-side gates, support-code exposure) is important but secondary to preventing easy leakage of tokens that let an observer or attacker attach to a victim's desktop/session. Fix the wire format or the mint+display path for these tokens first.

reports/review-2026-06-05/grok-dashboard.md.err

@@ -0,0 +1 @@
+EXIT=0 files=66

reports/review-2026-06-05/grok-server-a.md

@@ -0,0 +1,62 @@
+# GuruConnect server review — grok — 2026-06-05
+
+**All specified files have been read via the `read_file` tool (absolute paths, with follow-up offset reads for the two files whose initial reads were truncated by the default ~1000-line limit: `api/enroll.rs` and a no-op offset on the short `api/mod.rs`). No other files were read. No modifications were made (no writes, edits, terminal mutations, etc.). Review is strictly limited to the content of the 21 listed files.**
+
+### Prioritized Findings
+
+**[SEVERITY: CRITICAL], projects/msp-tools/guru-connect/server/src/api/downloads.rs:94 (and 101, 139, 146, 163, 190, 232, ~same pattern in viewer/support/agent paths)**  
+`Response::builder()... .header(CONTENT_DISPOSITION, format!("attachment; filename=\"{}\"", filename)) ... .body(...).unwrap()` (and equivalent for error bodies). `sanitize_filename` (236) only maps chars and truncates; it does not escape `"` or other header-special chars. A `company`/`site` query param containing `"` (or other values that make the header invalid) causes the builder to fail; the `.unwrap()` panics the handler task. These endpoints are unauthenticated/public.  
+**Concrete fix:** Never `.unwrap()` a `Response::builder()`. Use `axum::response::Response::builder()` with proper error path returning a 500 JSON/body. For `Content-Disposition`, either reject inputs containing `"`, `\`, or control chars before formatting, or use RFC 5987/8187 `filename*=` encoding (or `percent_encoding`). Add server-side length caps before embedding/sanitizing.
+
+**[SEVERITY: CRITICAL], projects/msp-tools/guru-connect/server/src/api/downloads.rs:169 (hardcoded), +169-179 (embedding), +107-149 (support path), +78-104 (viewer)**  
+`download_agent` hardcodes `server_url: "wss://connect.azcomputerguru.com/ws/agent"` and falls back to `api_key: "managed-agent"`. The top-of-file comment claims support downloads are "with embedded code"; the implementation only mutates the filename and serves the raw base binary (no `MAGIC_MARKER` + length + JSON append like the permanent path at 196-198). `download_*` handlers take only `Query`, no `State`/`AppState`/`Config`. Dev/staging builds will embed prod relay URLs; temp support sessions get no embedded config at all.  
+**Concrete fix:** Add the required server URL + default API key (and any other embed params) to `Config`/`AppState`. Make all three download handlers take `State`, read the values, and (for support) perform equivalent embedding of a support-oriented config (or a minimal `{code, server_url}`) using the same MAGIC format. Remove the hardcoded prod string. Update the module docstring.
+
+**[SEVERITY: HIGH], projects/msp-tools/guru-connect/server/src/api/auth.rs:41 (login), 205 (change_password), +86 (verify), no rate-limiter use anywhere in the file**  
+No rate limiting, progressive backoff, or account lockout on the primary password auth path (contrast with the sophisticated per-`(site_code, ip)` + timing-equalizer + failure recording in `enroll.rs:265`). Unlimited online brute-force / password stuffing is possible against any username. `get_user_by_username` + `verify_password` cost is paid on every attempt.  
+**Concrete fix:** Inject a rate-limiter (modeled on `state.rate_limits.enroll`) into `AppState`. Key on `(username.lower(), client_ip)` (or global + per-user). Record failures on bad password (and perhaps unknown user after constant-time work). Enforce in `login` (and consider `change_password`). Return 429 + opaque message after threshold. Add the same to the auth extractor or middleware for defense-in-depth.
+
+**[SEVERITY: HIGH], projects/msp-tools/guru-connect/server/src/auth/token_blacklist.rs:85 (`cleanup_expired`)**  
+`retain(|token| jwt_config.validate_token(token).is_ok())` re-runs full signature verification + claim parsing + the redundant post-decode `exp` check on every token in the `HashSet` (full JWT strings stored verbatim). Cleanup is admin-triggered only; a large backlog (many users, long 24h tokens) does expensive crypto work and keeps the whole token (claims + sig) in RAM until then.  
+**Concrete fix:** Change the internal storage to `(token: String, exp: i64)` (capture `exp` at `revoke` time from a cheap unsigned parse or from the minting site, since we only ever insert tokens we just validated). Cleanup then becomes a pure time comparison; no `validate_token` calls. (The existing `is_revoked` path still needs the string for the WS check.)
+
+**[SEVERITY: HIGH], projects/msp-tools/guru-connect/server/src/api/auth_logout.rs:116 (`revoke_user_tokens`)**  
+Handler is a complete stub: checks `is_admin()`, logs a warning that it is "NOT IMPLEMENTED", and unconditionally returns 501. The preceding comment claims "This currently only revokes the admin's own token as a demonstration" — the code does neither. The TODO describes exactly the missing session-tracking table. An admin endpoint documented for revoking other users' tokens does nothing.  
+**Concrete fix:** Either (a) delete the route + handler until the prerequisite table exists, or (b) implement the minimal version that at least revokes the caller's own token (or all currently-blacklistable artifacts for the target via the viewer registry + current blacklist) and make the comment + error message match reality. Do not leave a 501 admin "revoke anyone" endpoint that claims partial behavior.
+
+**[SEVERITY: MEDIUM], projects/msp-tools/guru-connect/server/src/api/downloads.rs:169 + enroll.rs:169 (and similar), config.rs:36**  
+`jwt_secret` and `database_url` are `Option` in `Config::load()` with silent fallbacks. `download_agent` never consults config at all. Several paths (login, mint viewer token, agent key issuance) will fail at runtime (or embed wrong values) if secrets are missing. No startup enforcement or `debug`-only relaxation.  
+**Concrete fix:** In `Config::load()`, require `JWT_SECRET` (and `DATABASE_URL` if not in a documented dev mode). Surface a clear error. Wire the resolved values into `AppState` and the download paths (see CRITICAL above). Consider a `require_jwt_secret` helper used by `JwtConfig::new` call sites.
+
+**[SEVERITY: MEDIUM], projects/msp-tools/guru-connect/server/src/api/* (multiple) + auth/* consistency**  
+Duplicated error shapes: `api/auth.rs` + `auth_logout` use `ErrorResponse { error: String }`; `machine_keys`, `enroll`, `sessions`, `sites`, `removal` define/use a richer `ApiError { detail, error_code, status_code }` (sometimes re-exported). Role/permission allow-lists are repeated verbatim in `users.rs` (122, 210, 339, 511). Boilerplate `db unavailable` / `map_err(500)` blocks appear in almost every handler.  
+**Concrete fix:** Standardize on the `ApiError` envelope everywhere (documented in `.claude/standards`). Extract small helpers (`require_db`, `map_db_err`, `require_admin` already partially exist). Centralize the `["admin","operator","viewer"]` + permission lists (or load from a const table). Use `thiserror` + `IntoResponse` for the common 4xx/5xx cases to kill the duplication.
+
+**[SEVERITY: MEDIUM], projects/msp-tools/guru-connect/server/src/api/enroll.rs:255 (and similar length checks), users.rs:133/378, auth.rs:271**  
+Only a minimum length (8) for passwords; no upper bound, no complexity, no control-char stripping on usernames/hostnames/machine_uid/site_code before DB or rate-limiter keys. `machine_uid` and `hostname` from unauthenticated enroll can be arbitrarily long (subject only to HTTP limits).  
+**Concrete fix:** Add reasonable caps (e.g. 128-256 chars) + sanitization (trim + reject controls / NUL) on all user-controlled identifiers early, before rate-limit keys or DB. Reject or truncate passwords >128. Make the checks consistent and return the standard `ApiError`.
+
+**[SEVERITY: MEDIUM], projects/msp-tools/guru-connect/server/src/api/sessions.rs:169 (`mint_viewer_token` registration) + auth_logout.rs:74 (take_for_user + revoke)**  
+The registry + blacklist cross-revoke for viewer tokens (addressing the "CRITICAL #2" scenario documented in comments and the test in `auth_logout.rs:231`) is only as good as the `sub` matching and the fact that the WS also calls `is_revoked` on the exact string. The mint path trusts `user.user_id` from the login JWT; no additional binding to the specific dashboard session that minted it. A compromised long-lived login JWT can still mint viewer tokens until it is revoked.  
+**Concrete fix:** (Defense-in-depth) Consider also stamping a `jti` (or short session id) from the login token into the viewer token claims (or the registry entry) and checking it on the WS side, or shortening the login JWT TTL + forcing refresh. At minimum, ensure the test coverage stays and that the registry prune + blacklist path is exercised on every logout.
+
+**[SEVERITY: LOW], projects/msp-tools/guru-connect/server/src/db/agent_keys.rs + api/machine_keys.rs:215 (key_belongs_to_machine)**  
+Good scoping (the revoke checks ownership via the FK before `revoke_agent_key`), but `keyed_machine_ids` (158) and the startup use are only in comments here. No visible index or query hint for the `revoked_at IS NULL` filter in hot paths.  
+**Concrete fix:** Ensure a partial index on `(machine_id) WHERE revoked_at IS NULL` (or equivalent) exists in migrations; the code already relies on it for the "active only" semantics.
+
+**[SEVERITY: LOW], projects/msp-tools/guru-connect/server/src/api/users.rs:574 (client_ids parsing) + 589 (set without existence check)**  
+`set_client_access` parses the supplied strings as UUIDs and stores them. No verification that the UUIDs actually exist in any clients table (or that the caller is allowed to see them). Silent creation of dangling references.  
+**Concrete fix:** After parsing, do a cheap `SELECT ... WHERE id = ANY(...)` (or equivalent) and reject the whole set if any are unknown (or at least log). This is the same pattern used for machine/site existence checks elsewhere.
+
+**[SEVERITY: LOW], projects/msp-tools/guru-connect/server/src/auth/jwt.rs:212 + 289 (post-decode exp checks) + 200 (validation config)**  
+Redundant `if claims.exp < now` after `validate_exp = true` + decode. `leeway = 0` is strict (good), but clock skew between dashboard and server is possible. Viewer tokens have a separate validator.  
+**Concrete fix:** Remove the redundant checks (or keep one with a tiny documented leeway for prod clock drift). Consider exposing a small configurable leeway.
+
+Other lower-severity notes (duplication of `get_user_permissions(...).unwrap_or_default()`, best-effort audit writes that can fail silently after the main action, `OptionalUser` and dead-code annotations for future native remote control, provisional collision heuristic in enroll with extensive comments, etc.) exist but are not prioritized above.
+
+### Overall Assessment
+The reviewed surface (auth layer, JWT + viewer tokens + logout revocation, per-agent + per-site enrollment keys, the public enroll path, admin CRUD for machines/users/releases/sites/keys, downloads, and supporting DB helpers) demonstrates careful, defense-in-depth thinking in several high-risk areas: Argon2id + timing equalization for the unauthenticated enroll path, explicit cross-site + collision gating with audits, parameterized queries everywhere (no SQLi surface), plaintext secrets returned exactly once and never logged, IDOR scoping on key revocation, tiered `control` vs `view_only` stamped inside signed viewer tokens, and the registry+blacklist mechanism to close the post-logout viewer token window. The code is heavily commented with references to prior audit findings ("CRITICAL #1", "CRITICAL #2", SPEC-016 items) and includes component tests that exercise the exact revocation paths.
+
+Weaknesses are primarily in the **unauthenticated download surface** (panic vectors + hardcoded prod config + docstring/implementation mismatch), absence of brute-force protection on the human login path (despite sophisticated protection on the machine enroll path), boilerplate and inconsistent error handling, and reliance on in-memory structures (full JWTs, viewer registry) whose cleanup/lifetime is only partially exercised in the provided code. Core relay/session-manager state, the actual WS handlers that consume viewer tokens, and the support-code issuance/matching logic live outside these files, so races, deadlocks, or binding integrity issues in the live control plane cannot be assessed from this slice alone.
+
+**Single most important thing to fix:** The downloads handlers (`download_viewer`/`support`/`agent`). They are unauthenticated, accept attacker-controlled query parameters directly into header construction and (for the agent path) into embedded config, perform no `State`-driven configuration, contain multiple `.unwrap()` panic sites on the response builder, and have a documented-vs-actual behavior gap for support sessions. A single malformed or malicious download request can panic a worker, serve binaries pointing at the wrong relay, or (if embedding is required for temp sessions) produce non-functional agents. This is a high-exposure, low-trust boundary that should be hardened first.

reports/review-2026-06-05/grok-server-a.md.err

@@ -0,0 +1 @@
+EXIT=0 files=21

reports/review-2026-06-05/grok-server-b.md

@@ -0,0 +1,49 @@
+# GuruConnect server review — grok — 2026-06-05
+
+**Code review complete.** I used `read_file` on every listed absolute path exactly as specified and performed the review across all of them. No files were created, edited, or otherwise modified.
+
+I reviewed only the provided files (GuruConnect server Rust sources for the relay, session manager, DB layer, auth-adjacent middleware, support codes, IP extraction, validation, metrics, and main wiring). Focus was strictly on the requested categories: (1) security (JWT/viewer tokens, session binding/takeover, support-code single-use + binding, authz/IDOR, input validation, SQLi, secrets, rate-limit bypass via IP extraction or otherwise); (2) correctness/concurrency (races in relay/session state, reattach/supersede/reap, lock ordering, panics/unwraps, error handling, resource leaks, consent gate); (3) protocol/wire integrity (bounds, decode paths, protobuf usage); (4) architecture/maintainability (defensive patterns, tenancy stubs, dead code, best-effort DB, config sensitivity).
+
+All DB access uses parameterized `sqlx::query*` + `.bind` (no SQLi surface visible). No compile-time `query!` macros. Protobuf via prost with explicit size caps on WS upgrades. Viewer auth path (post-Task work) requires a session-scoped minted viewer token (not a raw login JWT), with signature/expiry/purpose validation, explicit blacklist check on the WS plane, and a claim `session_id` == requested session binding check that returns 403 on mismatch. Agent auth for `cak_` (per-agent) keys performs identity binding (overrides client-supplied `agent_id` with the key's canonical machine `agent_id`; suppresses client `machine_uid` for keyed agents so it cannot override the key→machine binding). Support-code consumption is atomic under the in-memory write lock (`consume_for_bind` only from Pending) + best-effort durable DB conditional UPDATE (`consumed_at IS NULL AND status='pending' AND (no expiry or future)`). Reattach/supersede/reap use snapshot + `remove_session_if` recheck under the write lock to close TOCTOU. Consent gate (attended/support-code sessions) is enforced in `join_session` (`!allows_viewer()` returns None) and driven to completion in the agent handler before the main relay loop or StartStream. Rate limiting + lockouts (login, change-password, code-validate, enroll) are per-route, use the shared trusted-proxy-aware `client_ip` extractor, and include pre-handler lockout checks. Bounded WS message/frame sizes on both planes (4 MiB agent, 64 KiB viewer). Input flood constant declared (200/s) with "drop excess" intent.
+
+**Prioritized findings** (only issues; no padding with "looks good" items). Lines are from the exact file contents returned by the tool calls. Paths use the `server/src/...` form for the crate layout (matching the listed absolute paths under `projects/msp-tools/guru-connect/server/src`).
+
+- **[SEVERITY: HIGH] server/src/main.rs:198-225 (admin bootstrap block) + 216-223 (fallback)**: On first run (no users), a 16-char random password is generated, hashed, and the *plaintext* is written to a file named `.admin-credentials` in the server's current working directory. The `set_permissions(0o600)` is inside `#[cfg(unix)]` only; on Windows (the reported environment) it is a no-op and the file has default ACLs. On `fs::write` failure the plaintext is emitted via `info!` (visible in logs). The file is left on disk for the operator to read once and delete. This is a high-privilege bootstrap secret persisted in an uncontrolled location with weak cross-platform protection and a log fallback path.
+
+  **Concrete fix**: Do not write secrets to CWD files. Either (a) require the operator to supply the initial admin password via env/one-time flag/secret manager and error if none present on zero-user startup, or (b) print the password exactly once to stdout (with a strong "copy now" warning) and never persist it; remove the file-write path entirely. Apply Windows ACLs (e.g., via `icacls` or the `windows` crate) when the Unix path is taken. Never `info!` (or any log level) the generated password—only the "written to file" or "change immediately" guidance.
+
+- **[SEVERITY: MEDIUM] server/src/relay/mod.rs:144-157 and parallel sites (e.g. 203-216, 319-334)**: `CONNECTION_REJECTED_*` audit events (no-auth, invalid code, invalid API key) are emitted via `db::events::log_event(..., Uuid::new_v4(), ...)` (non-null `session_id`). This inserts rows into `connect_session_events` with a random `session_id` that has no row in `connect_sessions` (the FK is nullable, but these are semantically dangling). Contrast with `log_enrollment_event` / `log_admin_removal` which correctly pass `NULL` for session-less events. Pollutes the audit trail and complicates joins/queries that assume a valid session anchor.
+
+  **Concrete fix**: Add (or extend) a `log_connection_rejected` helper (or make `log_event` accept `Option<Uuid>` for `session_id` and update its INSERT) and pass `None` for these pre-session auth failures. Keep the rich `details` JSON + `ip_address`. Update the callers in `agent_ws_handler` (and any viewer equivalents).
+
+- **[SEVERITY: MEDIUM] server/src/support_codes.rs:182-183 (validate_code) + 181 (the server_url field in CodeValidation)**: The public (rate-limited but unauthenticated) `GET /api/codes/:code/validate` preview always returns a hardcoded `server_url: "wss://connect.azcomputerguru.com/ws/support"` when the code is Pending or Connected. This path does not exist in the mounted routes (`/ws/agent`, `/ws/viewer`); the modern attended flow uses a minted session-scoped viewer token on `/ws/viewer`. Any consumer of the validation response (or docs/tests) gets a wrong target. Additionally, `validate_code` (preview) intentionally accepts both Pending and Connected; this is fine for preview but means a correctly guessed/high-entropy code that has already been bound will still return `valid: true` + the (wrong) URL + session_id until the code is completed/cancelled.
+
+  **Concrete fix**: Remove `server_url` from `CodeValidation` (or make it a server-configured value derived from the incoming request or a single source of truth that matches the actual WS mounts and the viewer-token flow). Keep the preview behavior as-is (it must not consume), but consider returning a generic "already in use" or omitting the session_id for Connected codes if the preview is only meant to tell a technician "this code is claimable now."
+
+- **[SEVERITY: MEDIUM] server/src/main.rs:652-658 (prometheus_metrics) + server/src/middleware/security_headers.rs:49-51 (and similar .parse().unwrap() sites)**: `registry.lock().unwrap()` + `encode(&mut buffer, &registry).unwrap()`. A poisoned lock or encode failure panics the handler. In `security_headers`, multiple `HeaderValue::from_static(...).unwrap()` (or equivalent `parse().unwrap()`) on literals for CSP, X-Frame-Options, etc. A future edit that introduces an invalid literal turns every response into a panic.
+
+  **Concrete fix**: Use `if let Ok(lock) = ...` or `.lock().expect("registry poisoned")` + graceful error response for metrics. For headers, use `const` `HeaderValue` or `.expect("statically valid security header")` / `unwrap_or_else(|_| /* static fallback */)` so construction cannot panic a live response.
+
+- **[SEVERITY: MEDIUM] server/src/rate_limit.rs:140,212,238,346 etc. (all the `Mutex` maps) + main.rs wiring + 97 (MAX_TRACKED_IPS)**: Rate limiters (`RateLimiter`, `FailureLockout`, `EnrollLimiter`) and lockout state are purely in-memory (`Arc<Mutex<HashMap<IpAddr, ...>>>` or composite `(site_code, IpAddr)`). Process restart (deploy, crash, OOM) clears all windows and active lockouts. The 15-minute code-validate and enroll lockouts therefore give an attacker a fresh window on every restart. Pruning at `MAX_TRACKED_IPS` and opportunistic window expiry exist, but persistence is absent. The `client_ip` extractor (trusted-proxies gate) is the single root for all of this plus audit; defaults are safe (loopback only) and the logic + tests are strong, but any misconfiguration of `CONNECT_TRUSTED_PROXIES` (or a future multi-hop proxy) bypasses the per-IP controls for the entire fleet.
+
+  **Concrete fix**: Document the in-memory limitation explicitly (restarts reset brute-force windows). For the support-code and enroll surfaces, consider a small persistent store (bounded TTL table in the existing Postgres, or a sidecar) keyed the same way, or accept the risk and keep restart frequency low. Keep the trusted-proxy extraction as the single source of truth (already is) and add a startup warning + metrics for "number of trusted proxies" and "XFF used vs. direct."
+
+- **[SEVERITY: MEDIUM] server/src/relay/mod.rs:51 (VIEWER_INPUT_EVENTS_PER_SEC) + comment at top of file + visible input path (input_forward task, join_session returning raw InputSender, mpsc(64) in register_agent)**: The constant and comment state that viewer→agent input (mouse/key `SendInput`) is capped at 200 events/sec per viewer with excess "DROPPED (coalesced away)" to prevent a compromised viewer from flooding the target. In the provided code the path is a plain `tokio::sync::mpsc` channel (buffer 64) pumped verbatim by the spawned forwarder; no token bucket, per-second counter, or explicit drop/coalesce is visible at the point where viewers receive their `InputSender` or in the pump. Bounded buffer gives backpressure (senders slow down), but a burst up to the buffer depth (or sustained rate limited only by WS + agent processing) can still be injected.
+
+  **Concrete fix**: Implement the declared policy where the `InputSender` is handed out (in the viewer connection handler, not shown in full but referenced) or in the forwarder: a small per-viewer (or per-session) rate limiter that drops or coalesces (e.g., keep latest mouse move) when over the cap. Wire the existing constant. If backpressure alone is intentional, update the comment to match reality and shrink the buffer.
+
+- **[SEVERITY: LOW] server/src/db/users.rs:238-269 (user_has_client_access) + callers marked dead_code**: The function correctly implements "admin or (explicit client in list) or (zero restrictions = legacy all-access)". It does a `get_user_by_id` (for role) + exact match query + count query on every call. When multi-tenancy (Phase 4) activates and/or more surfaces use this for history/machine/session reads, any missed call site or path that takes a raw `agent_id` / session id from user input without going through an equivalent check is an IDOR. Several machine/session history endpoints and the client_access surfaces are still `#[allow(dead_code)]`.
+
+  **Concrete fix**: Extract a small `can_access_machine` / `can_access_session` helper (incorporating `current_tenant_id()` once Phase 4 lands). Prefer query-level filters (join or `WHERE` with the user's client list or "no restrictions" subquery) over N+1 post-filter in handlers. Add explicit tests for a restricted non-admin user being denied another client's machine history / session list.
+
+- **[SEVERITY: LOW] server/src/main.rs:589-611 (CORS layer) + 583 (security headers order)**: CORS is narrowly scoped (exact prod origin + localhosts, credentials true, limited methods/headers) — good. However, the layer ordering (security headers after some routes, TraceLayer, etc.) and the fact that the SPA fallback + `/downloads` nest + wildcard 404s exist means any future public asset or mis-mounted route could leak. CSP still includes `'unsafe-inline'` for script/style (required for the current dashboard) and `ws: wss:`.
+
+  **Concrete fix**: Keep the narrow allowlist. Consider moving to a nonce- or hash-based CSP for the SPA if feasible in a future iteration (reduces reliance on unsafe-inline). Ensure the wildcard `any` 404s (`/api/*rest`, `/ws/*rest`) stay in place so unknown API/WS paths never reach the SPA index.html.
+
+- **[SEVERITY: LOW] Multiple files (e.g. relay/mod.rs:575, session/mod.rs:428, releases.rs:23, many `#[allow(dead_code)]` for "TODO(native-remote-control)" and integration API)**: Large surface of functions/fields/types marked dead_code because the "native remote control" / full integration API is not yet wired. This is maintainability debt and increases the chance that a future change will activate insecure or un-reviewed paths (e.g. the older support-code mark paths that are superseded by the single-use consume, the `is_code_valid` etc. that are no longer on the hot bind path).
+
+  **Concrete fix**: As the integration work lands, either delete the superseded functions or gate them behind a feature flag + explicit review. Reduce the number of `too_many_arguments` allows by introducing the params structs already referenced in the comments.
+
+**Overall assessment**: The reviewed code has clearly undergone deliberate, defense-in-depth security work on exactly the high-risk areas for a remote-support relay (viewer token binding + revocation + session claim enforcement, per-agent `cak_` identity binding that prevents client-supplied values from seizing another machine's session, atomic single-use support-code consumption at bind time with durable DB marker, trusted-proxy-aware IP extraction as the root for rate limiting + audit, consent gate before any viewer join or streaming, reattach/supersede/reap TOCTOU hardening via recheck-under-write-lock, WS message bounds on both planes, and soft-delete + filtered queries for operator purge). DB access is uniformly parameterized. Concurrency uses a consistent lock order (sessions before agents/machine_uids) and the `remove_session_if` helper. No obvious SQL injection, trivial session takeover, or missing authz on the active paths in these files. Weaknesses are primarily operational (secret handling for bootstrap), process-lifetime-only state for rate limiting, a few audit correctness nits, a hardcoded/wrong URL in a public response, and declared-but-not-fully-visible input rate enforcement. The large number of `#[allow(dead_code)]` + "Task N / SPEC-016 / Phase 4" comments indicate this is an evolving, partially implemented surface—future work must re-review the newly activated paths.
+
+**Single most important thing to fix**: The bootstrap admin credential handling in `server/src/main.rs:198-225` (and the log fallback). It places a high-privilege random password on disk in the process CWD with platform-incomplete ACLs and a path that can emit the secret in logs. For a remote-access control plane product this is the clearest high-impact, low-effort operational security gap in the reviewed files. Replace the file-write + log-fallback with an operator-supplied value or strict one-time stdout emission that is never persisted.

reports/review-2026-06-05/grok-server-b.md.err

@@ -0,0 +1 @@
+EXIT=0 files=21

reports/review-2026-06-05/grok-server.md

@@ -0,0 +1,2 @@
+# GuruConnect server review — grok — 2026-06-05
+

reports/review-2026-06-05/run-review.sh

@@ -0,0 +1,16 @@
+#!/usr/bin/env bash
+# run-review.sh <gemini|grok> <subsystem> <files-list> <out-file>
+set -uo pipefail
+TOOL="$1"; SUB="$2"; LIST="$3"; OUT="$4"
+ROOT="/d/claudetools"
+case "$TOOL" in
+  gemini) SCRIPT="$ROOT/.claude/skills/agy/scripts/ask-gemini.sh" ;;
+  grok)   SCRIPT="$ROOT/.claude/skills/grok/scripts/ask-grok.sh" ;;
+  *) echo "bad tool"; exit 2 ;;
+esac
+INSTR="You are doing an independent code review of the GuruConnect ${SUB} (a Rust/TypeScript remote-support/remote-control platform: agent <-> relay server <-> browser viewer, protobuf wire protocol, JWT auth, support-code session binding). Review ONLY the files provided. Produce a prioritized findings list. For EACH finding give: [SEVERITY: CRITICAL/HIGH/MEDIUM/LOW], file:line, the problem, and a concrete fix. Focus on: (1) security — auth/JWT, session takeover, support-code binding, input validation, SQL injection, secret handling, IDOR/authorization, rate-limit bypass; (2) correctness & concurrency — relay/session state races, deadlocks, panics/unwraps, error handling, resource leaks; (3) protocol/wire integrity; (4) architecture & maintainability. Be concrete and skeptical. End with a short overall assessment and the single most important thing to fix."
+mapfile -t FILES < "$LIST"
+echo "# GuruConnect ${SUB} review — ${TOOL} — 2026-06-05" > "$OUT"
+echo "" >> "$OUT"
+bash "$SCRIPT" review-files -i "$INSTR" "${FILES[@]}" >> "$OUT" 2>> "${OUT}.err"
+echo "EXIT=$? files=${#FILES[@]}" >> "${OUT}.err"

session-logs/2026-06-03-session.md

@@ -0,0 +1,63 @@
+# Session Log — 2026-06-03 — GuruConnect SPEC-018 review validation + fixes
+
+## User
+- **User:** Mike Swanson (mike)
+- **Machine:** GURU-5070
+- **Role:** admin
+
+---
+
+## Session Summary
+
+Mike forwarded a thorough external code review of GuruConnect SPEC-018 Phase 1 (managed agent as LocalSystem service host; merge 11af9df) performed by a Grok reviewer persona and written to `D:\GrokTools\guru-connect-review-SPEC018.md`. Task: look over the project and validate.
+
+Independently validated the two flagged bugs and Issue 6 by reading the actual code (not just relaying). Confirmed all three as real. Added a refinement the review missed: the non-functional managed fallback (Bug 1) *does* still work for a deprecated legacy-`api_key` managed binary, but is broken specifically for the modern SPEC-016 enrollment path — sharpening the fix.
+
+Copied the review into the project at `reports/2026-06-03-spec018-review.md`, claimed a coord lock on `guruconnect`, created branch `fix/spec018-review-bugs`, and had the Coding Agent implement the three fixes. `cargo check -p guruconnect --target x86_64-pc-windows-msvc` passes clean (no errors/warnings). Filed Gitea issue #8 for the deferred lower-severity items. Changes remain uncommitted on the branch pending Mike's PR-vs-direct-to-main decision.
+
+---
+
+## Key Decisions
+
+- **Validated, did not rubber-stamp.** Read the code at each cited location to confirm Bug 1 (main.rs:496), Bug 2 (config.rs:392), Issue 6 (startup.rs transmute) before acting.
+- **Bug 1 fix = remove the fallback, surface an elevation error** (rather than build a degraded fallback). Matches `install_managed_service` docs; the managed model is elevated-install. The deprecated legacy-key edge case also errors now — acceptable and honest.
+- **Bug 2 fix = read persisted agent_id from the TOML first**, generate only if absent — stops agent_id churn on every restart while keeping machine_uid/cak_ as the stable keys.
+- **Issue 6 fix = typed `HKEY` from the windows crate** (no `HANDLE`+transmute). `install.rs` was already typed (no change).
+- **Deferred Issues 3/4/5/7/8** (hot-path unwraps, panic-guard scope, nits) to Gitea #8 — lower severity, follow-ups.
+- **No commit yet** — branch held for human review of diffs + PR-vs-main choice.
+
+---
+
+## Configuration Changes
+
+**In submodule `projects/msp-tools/guru-connect` (branch `fix/spec018-review-bugs`, UNCOMMITTED):**
+- `agent/src/config.rs` — added `Config::persisted_agent_id()`; embedded branch now `agent_id: Self::persisted_agent_id().unwrap_or_else(generate_agent_id)`; corrected comment.
+- `agent/src/main.rs` — `run_permanent_agent_managed`: removed `run_agent_mode(None)` fallback, now `error!` + `Err(...)` requiring elevation; updated doc/inline comments.
+- `agent/src/startup.rs` — replaced `transmute::<HANDLE,HKEY>` with `HKEY::default()` + `&mut hkey`; added SAFETY comments.
+- Created `reports/2026-06-03-spec018-review.md` (copy of the external review).
+- Stray untracked `tmp-spec018.diff` left untouched (from the Grok session).
+
+---
+
+## Commands & Outputs
+
+- Validation greps/reads: `run_permanent_agent_managed` at main.rs:482, fallback at :496; `Config::load` embedded branch config.rs:382-409 (`agent_id: generate_agent_id()` unconditional, save() never read back); `resolve_agent_credential` main.rs:515 (load_cak permission_denied guard / enroll C1 read-back).
+- `cargo check -p guruconnect --target x86_64-pc-windows-msvc` → Finished clean, no warnings from the changes.
+- Coord lock id `0cfd6269-4548-46d4-8436-c829e42f79d8` (guruconnect / agent/src, ttl 2h, GURU-5070/claude-main).
+
+---
+
+## Pending / Incomplete Tasks
+
+- **Awaiting Mike's decision:** push branch + open PR (recommended, matches SPEC-018 PR #7 convention) vs. commit straight to `main`.
+- On decision: commit the 3 fixes + the review report, push, (PR/merge), then bump the parent-repo submodule pointer on next `/sync`, update the coord `guruconnect` component, and release lock `0cfd6269`.
+- Deferred hardening: Gitea **guru-connect#8** (Issues 3/4/5/7/8).
+
+---
+
+## Reference Information
+
+- External review: `D:\GrokTools\guru-connect-review-SPEC018.md` → copied to `reports/2026-06-03-spec018-review.md`.
+- Branch: `fix/spec018-review-bugs` (off `main` @ 11af9df).
+- Gitea issue: https://git.azcomputerguru.com/azcomputerguru/guru-connect/issues/8
+- Files: `agent/src/{config.rs,main.rs,startup.rs}`.