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

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