fix: harden desktop startup and diagnostics - #343

internal/daemon/restart_integration_test.go (1)

199-201: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Assert the persisted replacement start time.

Lines 199-201 inject the replacement process start time, but the test does not verify that the restart operation stores it. A regression that omits or replaces this identity value can still pass. Assert that the persisted replacement start-time field equals the injected value.

As per path instructions, “Ensure tests verify behavior outcomes, not just function calls.”

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/daemon/restart_integration_test.go` around lines 199 - 201, Update
the restart integration test around the processStartedAt stub to retain the
injected timestamp and assert that the persisted replacement start-time field
equals that exact value after the restart operation. Verify the stored outcome
rather than only the injected function call, while preserving the existing
restart assertions.

Source: Path instructions

desktop/src-tauri/src/health_monitor.rs (1)

279-322: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add the negative case for boot-timestamp drift.

The test asserts only is_some(). FakeProbe returns a Compozy identity for every call, so the assertion passes whenever the drift branch attaches, including when ownership verification is removed. Add a case where the executable is not app-owned, or where the provenance hash does not match, and assert observed_identity returns None. Also assert that the returned identity carries the recorded PID, so the test fails when the branch attaches to the wrong process.

💚 Suggested additional assertions

-        assert!(observed_identity(&home, &processes, &probe).is_some());
+        let observed = observed_identity(&home, &processes, &probe).expect("drift runtime attaches");
+        assert_eq!(observed.record.pid, record.pid);
+
+        let foreign = FakeProcesses {
+            starts: HashMap::from([(record.pid, observed_start)]),
+            executables: HashMap::from([(record.pid, PathBuf::from("/usr/bin/other"))]),
+        };
+        assert!(observed_identity(&home, &foreign, &probe).is_none());

As per path instructions: "Verify tests can fail when business logic changes".

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@desktop/src-tauri/src/health_monitor.rs` around lines 279 - 322, Add negative
coverage to the test around observed_identity: use a non-app-owned executable or
mismatched provenance hash and assert it returns None, while retaining the valid
case and asserting the returned identity contains record.pid. Ensure the test
verifies both ownership/provenance rejection and correct PID attachment for the
forward boot-timestamp drift branch.

Source: Path instructions

desktop/src-tauri/src/shell.rs (1)

235-292: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Extract the shared RecordedDaemonStart handling.

The Awaiting and StartTimeMismatch arms are identical except for the recovery call. Both repeat the recovery_attempted guard and the four-outcome match. A small helper removes the duplication and keeps the two paths in sync when outcomes change.

♻️ Suggested consolidation

+    fn handle_recorded_start(
+        &self,
+        recovery_attempted: &mut bool,
+        start: impl FnOnce() -> RecordedDaemonStart,
+    ) -> bool {
+        if *recovery_attempted {
+            self.publish_error(ShellErrorCode::RuntimeStartFailed);
+            return false;
+        }
+        match start() {
+            RecordedDaemonStart::Attached { identity, owned } => {
+                self.finish_attach(*identity, owned);
+                false
+            }
+            RecordedDaemonStart::Recovered => {
+                *recovery_attempted = true;
+                true
+            }
+            RecordedDaemonStart::Unverified => {
+                self.publish_error(ShellErrorCode::RuntimeUnhealthy);
+                false
+            }
+            RecordedDaemonStart::Failed => {
+                self.publish_error(ShellErrorCode::RuntimeStartFailed);
+                false
+            }
+        }
+    }

Each arm then calls the helper and continues the loop only when it returns true.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@desktop/src-tauri/src/shell.rs` around lines 235 - 292, Extract the shared
recovery-attempt guard and RecordedDaemonStart outcome match into a helper near
the surrounding shell logic, accepting the appropriate recovery result and
returning whether the caller should continue. Update both Resolution::Awaiting
and Resolution::StartTimeMismatch arms to call this helper, preserving their
distinct recovery functions and existing attach, retry, unhealthy, and failure
behavior.

desktop/src-tauri/src/diagnostics/export_tests.rs (1)

140-195: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add a test for the size limit.

The suite covers consent, redaction, permissions, collisions, and symlinks. DiagnosticExportError::TooLarge and the removal of the oversized bundle at export.rs lines 72-75 are untested. Write a log file large enough to exceed MAX_DIAGNOSTIC_BUNDLE_BYTES, then assert both the error and that no bundle file remains.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@desktop/src-tauri/src/diagnostics/export_tests.rs` around lines 140 - 195,
The diagnostic export tests need coverage for oversized bundles. Add a test
alongside the existing export tests that creates a log file exceeding
MAX_DIAGNOSTIC_BUNDLE_BYTES, invokes export, asserts
DiagnosticExportError::TooLarge, and verifies the generated bundle path does not
remain.

internal/cli/app_diagnose_bundle_logs.go (2)

129-148: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low value

Confirm the intended behavior when a line exceeds the remaining budget.

The loop calls continue when a redacted line would cross appDiagnosticBundleLogTailMaxBytes. Later, shorter lines are still appended. The result is a tail with gaps in the middle rather than a contiguous suffix. If a contiguous tail is the contract for the support bundle, use break instead.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/cli/app_diagnose_bundle_logs.go` around lines 129 - 148, The
redacted log tail should remain a contiguous suffix when a line exceeds the
remaining appDiagnosticBundleLogTailMaxBytes budget. Update
redactAppDiagnosticBundleLogTail to stop processing subsequent lines at that
boundary instead of skipping the oversized line and appending later lines.

---

207-226: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low value

Resolve the user home directory once per tail, not once per line.

redactAppDiagnosticBundleLogMessage calls os.UserHomeDir() for every log line. Pass the resolved home from redactAppDiagnosticBundleLogTail instead. This also makes the failure behavior explicit: today a os.UserHomeDir() error silently drops every line in the tail.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/cli/app_diagnose_bundle_logs.go` around lines 207 - 226, Resolve
os.UserHomeDir once in redactAppDiagnosticBundleLogTail and pass the result into
redactAppDiagnosticBundleLogMessage for each line. Update the redaction helper
signature and remove its per-line home-directory lookup, while preserving
explicit handling of resolution errors in the tail flow rather than silently
dropping individual lines.

internal/cli/app_diagnose_bundle_archive.go (1)

219-221: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Report an empty bundle file with an accurate message.

The size check returns "exceeds its size limit" for both an empty file and an oversized file. An operator who receives this message for a 0-byte bundle gets misleading guidance. Split the two conditions.

♻️ Proposed message split

-	if info.Size() < 1 || info.Size() > appDiagnosticBundleMaxBytes {
-		return errors.New("app diagnose: app control bundle exceeds its size limit")
-	}
+	if info.Size() < 1 {
+		return errors.New("app diagnose: app control bundle is empty")
+	}
+	if info.Size() > appDiagnosticBundleMaxBytes {
+		return errors.New("app diagnose: app control bundle exceeds its size limit")
+	}

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/cli/app_diagnose_bundle_archive.go` around lines 219 - 221, Update
the size validation in the app diagnose bundle archive flow to handle
info.Size() < 1 separately from info.Size() > appDiagnosticBundleMaxBytes:
return an accurate empty-bundle error for zero-byte files, while preserving the
existing size-limit error for oversized bundles.

internal/cli/app_test.go (1)

345-379: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add coverage for the remaining archive validation rules.

This subtest covers the unexpected-entry rejection only. validateAppDiagnosticBundleArchiveEntry in internal/cli/app_diagnose_bundle_archive.go also rejects duplicate entries, oversized manifest entries, oversized log entries, and an aggregate size above appDiagnosticBundleContentMaxBytes. These are the boundary rules that protect the bundle reader from a hostile archive. Extend this suite with table cases for each rule.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/cli/app_test.go` around lines 345 - 379, Extend the diagnostic
bundle archive validation tests around the existing “Should reject an unexpected
live bundle entry” subtest with table-driven cases covering duplicate entries,
manifest entries exceeding their limit, log entries exceeding their limit, and
aggregate content exceeding appDiagnosticBundleContentMaxBytes. Build valid
archive fixtures for each case, invoke readAppDiagnosticBundleManifest, and
assert rejection with the appropriate validation error.

Source: Path instructions

desktop/src-tauri/src/diagnostics/report.rs (1)

75-87: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Share one phase vocabulary with the startup marker.

boot_phase_from_state produces the phase strings, and is_diagnostic_phase in diagnostics/startup_marker.rs validates them from a second hardcoded list. If one list changes, marker_document_is_valid silently rejects valid markers and previous-crash data disappears. Consider a shared constant slice, or make is_diagnostic_phase reuse the same source list.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@desktop/src-tauri/src/diagnostics/report.rs` around lines 75 - 87, The phase
vocabulary in boot_phase_from_state and is_diagnostic_phase is duplicated;
centralize it so both validation and state mapping use the same source of truth.
Update the diagnostics phase definition and make is_diagnostic_phase reuse it,
while preserving the existing strings returned by boot_phase_from_state and
marker_document_is_valid behavior.

desktop/src-tauri/src/runtime/update_cadence.rs (1)

6-13: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

The cadence thread cannot be stopped.

The loop never exits and the thread holds a strong Arc<DesktopController>. The controller is therefore never dropped, and an update check can start while the application shuts down. Hold a Weak reference so the thread ends after the controller is dropped, or pass an explicit stop flag.

♻️ Proposed change using a weak reference

-pub fn spawn(controller: Arc<DesktopController>, interval: Duration) {
+pub fn spawn(controller: &Arc<DesktopController>, interval: Duration) {
+    let controller = Arc::downgrade(controller);
     std::thread::spawn(move || {
         loop {
             std::thread::sleep(interval);
-            controller.check_updates();
+            let Some(controller) = controller.upgrade() else {
+                return;
+            };
+            controller.check_updates();
         }
     });
 }

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@desktop/src-tauri/src/runtime/update_cadence.rs` around lines 6 - 13, Update
update-cadence::spawn to downgrade the controller Arc to a Weak reference before
starting the thread, then upgrade it inside the loop before calling
check_updates and exit when upgrading fails. Ensure the spawned thread no longer
retains a strong DesktopController reference during shutdown.

desktop/src-tauri/src/main.rs (1)

182-197: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

The panic guard suppresses every panic after the first one.

writing_panic is set to true and is never reset. After the first panic is written, any later panic on another thread skips logging::bootstrap_error. The guard appears to be intended as re-entrancy protection, not as a one-shot switch. Reset the flag after the write so later panics are still recorded.

♻️ Proposed change to reset the re-entrancy guard

     panic::set_hook(Box::new(move |info| {
         if !writing_panic.swap(true, Ordering::AcqRel) {
             let message = sanitize_public_text(&info.to_string());
             if let Err(error) = logging::bootstrap_error(&logs_dir, &boot_id, "panic", &message) {
                 eprintln!(
                     "{}",
                     sanitize_public_text(&format!("write panic fallback log: {error}"))
                 );
             }
+            writing_panic.store(false, Ordering::Release);
         }
         previous_hook(info);
     }));

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@desktop/src-tauri/src/main.rs` around lines 182 - 197, Update
install_panic_hook so writing_panic is reset after each attempted
logging::bootstrap_error call, while remaining set during the write to prevent
re-entrant panic handling. Ensure the reset occurs even when bootstrap_error
returns an error, allowing later independent panics to be recorded.

desktop/src-tauri/src/runtime/boot_timestamp_drift_tests.rs (1)

161-190: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add coverage for the Unreachable outcome.

The tests cover Attached, NotOwned, and IdentityMismatch. No test drives Identity::Unreachable into BoundBootTimestampDrift::Unreachable. That variant is the only outcome a caller can distinguish from a hard mismatch, so a mapping regression would pass unnoticed.

♻️ Proposed additional case

     let foreign = FakeProbe(Identity::Foreign);
+    let unreachable = FakeProbe(Identity::Unreachable);
@@
+    assert!(matches!(
+        bound_app_owned_runtime_with_boot_timestamp_drift(
+            &home, &processes, &unreachable, &record, observed
+        ),
+        BoundBootTimestampDrift::Unreachable
+    ));

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@desktop/src-tauri/src/runtime/boot_timestamp_drift_tests.rs` around lines 161
- 190, Add a test case in
should_require_a_compozy_identity_for_a_boot_timestamp_drift_runtime using a
FakeProbe configured with Identity::Unreachable, then call
bound_app_owned_runtime_with_boot_timestamp_drift and assert it returns
BoundBootTimestampDrift::Unreachable. Keep the existing identity mismatch
assertions unchanged.

desktop/src-tauri/src/runtime/update_lifecycle_tests.rs (1)

305-330: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

No test covers StalledRuntimeRecovery::Failed.

FakeRecoveryProcesses::force_terminate on line 53 always removes the pid, so the process always exits. stop_process therefore never returns an error and recover_verified_runtime never returns StalledRuntimeRecovery::Failed. That variant reports an unrecoverable runtime to the user and has no coverage.

Add a fake that keeps the process alive after the forced termination.

♻️ Proposed additional case

+#[test]
+fn should_report_a_failed_recovery_when_the_runtime_survives_forced_termination() {
+    let (_directory, home, record) = owned_runtime();
+    let processes = FakeRecoveryProcesses {
+        starts: Mutex::new(HashMap::from([(record.pid, record.started_at)])),
+        executables: HashMap::from([(record.pid, home.app_binary.clone())]),
+        graceful_stops: AtomicUsize::new(0),
+        forced_stops: AtomicUsize::new(0),
+        stop_on_graceful: false,
+        stop_on_forced: false,
+    };
+    let delay = FakeDelay;
+
+    assert_eq!(
+        recover_stalled_runtime(
+            &home,
+            &record,
+            &processes,
+            &delay,
+            Duration::from_millis(1),
+            Duration::from_millis(1),
+        ),
+        StalledRuntimeRecovery::Failed
+    );
+    assert!(home.daemon_info.exists());
+}

The fake needs a stop_on_forced field that gates the removal on line 55.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@desktop/src-tauri/src/runtime/update_lifecycle_tests.rs` around lines 305 -
330, Extend FakeRecoveryProcesses with a stop_on_forced flag and gate the pid
removal in force_terminate on that flag, then add a test covering
recover_verified_runtime returning StalledRuntimeRecovery::Failed when forced
termination leaves the process alive. Keep the existing recovery test configured
to stop on forced termination so its Recovered behavior remains unchanged.

desktop/src-tauri/src/record.rs (1)

241-255: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

This test asserts only a constructor field assignment.

should_preserve_the_observed_operating_system_process_start_time passes started_at into AppRecord::new and then reads back record.started_at. No business logic can break this assertion. The test cannot fail when startup-time behavior changes.

Make it meaningful by asserting the serialized contract, which is what consumers read:

♻️ Proposed change to assert the persisted contract

-        let record = AppRecord::new(
-            42,
-            started_at,
-            report(&ShellState::Resolving),
-            ShellState::Resolving,
-        );
-
-        assert_eq!(record.started_at, started_at);
+        let record = AppRecord::new(
+            42,
+            started_at,
+            report(&ShellState::Resolving),
+            ShellState::Resolving,
+        );
+
+        let value = serde_json::to_value(record).expect("record serializes");
+        assert_eq!(value["started_at"], "2026-08-11T12:00:00Z");

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@desktop/src-tauri/src/record.rs` around lines 241 - 255, Update
should_preserve_the_observed_operating_system_process_start_time to serialize
the constructed AppRecord and assert the persisted representation contains the
observed started_at value, rather than directly comparing record.started_at.
Reuse the existing serialization approach and expected field format used by
AppRecord consumers or nearby tests.

desktop/src-tauri/src/runtime/update_lifecycle.rs (1)

273-289: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

stop no longer escalates to a forced termination, and it drops the error detail.

Two points:

1. stop calls only self.processes.terminate(pid) and waits. stop_process on line 137 already implements graceful termination, waiting, and forced escalation. A runtime that ignores SIGTERM now fails the update instead of being stopped. If the removal of the forced path is intentional, add a short comment that states why the update path must not force-kill.
2. map_err(|_| RuntimeUpdaterError::Lifecycle) on line 288 discards the io::Error. Other failures in this file are logged, for example line 310. Log the cause before mapping.

♻️ Proposed change to reuse `stop_process` and log the cause

     fn stop(&self, pid: u32) -> Result<(), RuntimeUpdaterError> {
         let delay = ThreadDelay;
-        self.processes
-            .terminate(pid)
-            .and_then(|()| {
-                wait_for_exit(
-                    &self.processes,
-                    pid,
-                    &delay,
-                    STOP_DEADLINE,
-                    STOP_POLL_INTERVAL,
-                )
-                .then_some(())
-                .ok_or_else(|| io::Error::other("runtime process did not exit after termination"))
-            })
-            .map_err(|_| RuntimeUpdaterError::Lifecycle)
+        stop_process(
+            &self.processes,
+            pid,
+            &delay,
+            STOP_DEADLINE,
+            STOP_POLL_INTERVAL,
+        )
+        .map_err(|error| {
+            crate::logging::error(format!("stop runtime process {pid}: {error}"));
+            RuntimeUpdaterError::Lifecycle
+        })
     }

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@desktop/src-tauri/src/runtime/update_lifecycle.rs` around lines 273 - 289,
Update stop to reuse the existing stop_process flow so runtime termination waits
gracefully and escalates to forced termination when necessary, unless the
removal is intentional—in that case, document the reason inline. Preserve the
wait deadline and interval behavior, and log the underlying termination or wait
error before mapping it to RuntimeUpdaterError::Lifecycle instead of discarding
it.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes