Give fault reporting a seam a test can observe

`report_anomaly` and `report_error` were associated functions calling the crash
SDK directly. They worked only because that SDK keeps a process-global client —
which meant they bypassed `Telemetry`'s state entirely and, more importantly,
**nothing could observe that a path reported a fault**.

That matters now rather than in principle. Instrumenting the background engine
is mostly about failures, and d-outcome-not-intent settled that failure counting
rides the crash sink rather than the analytics vocabulary — so the mechanism the
telemetry work leans on for failure visibility was the one with no way to assert
against it.

Both are methods over a `Faults` field now: `Live` captures via the SDK,
`DryRun` writes to the developer's console, and a cfg(test) `Recorded` variant
keeps them in memory so a test can read them back. The dry run is a real gain of
its own — until now a fault in a local build went nowhere at all, since a dev
build has no crash client.

Deliberately not a trait, per d-no-provider-traits: this is a seam for
observation, not an abstraction over vendors.

What it immediately bought — the spec's claim about logout is now checked rather
than merely written. A logout that finds nothing stored is still a success (a
second click must not error) but emits no event and reports an anomaly, because
the settings page lists hosts from those very directories, so a missing one means
the list and the erase disagree. Two tests: that the anomaly is reported, and
that a logout which does erase something reports nothing — without the second,
the first would pass on an implementation that reported every time.

The failed-send path inside `track` also routes through the field now, so the
whole fault path is observable rather than most of it.

just lint 0, cargo fmt --check 0, 307 quilt-sync tests, workspace green.

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

Author: fiskus

quilt-sync/src-tauri/src/commands/auth.rs

@@ -146,9 +146,9 @@ async fn erase_auth_command(
         // stays constant so the anomaly groups as one issue.
         Ok(ErasedAuth::NothingStored(host)) => {
             tracing.add_host(host);
-            Telemetry::report_anomaly("Logout found no stored auth for the host");
+            tracing.report_anomaly("Logout found no stored auth for the host");
         }
-        Err(err) => Telemetry::report_error(err),
+        Err(err) => tracing.report_error(err),
     }
 
     // On either success: the role cache can still hold an entry for a host whose
@@ -774,6 +774,74 @@ mod tests {
     /// A logout takes one host's stored auth and nothing else. The erase joins a
     /// parsed host onto the auth dir, so a bug in that addressing would show up
     /// here as the other host's credentials disappearing with it.
+    /// The claim the spec makes about logout, finally assertable: one that finds
+    /// nothing stored is still a **success** — a second click must not raise an
+    /// error — but it emits no event and *reports an anomaly*, because the settings
+    /// page offers logout only for hosts it listed from these very directories, so
+    /// a missing one means the list and the erase disagree about the name.
+    ///
+    /// Before fault reporting had a seam this could be read but not checked: it
+    /// went straight to a process-global crash client that no test could observe.
+    #[tokio::test]
+    async fn logging_out_a_host_with_nothing_stored_reports_an_anomaly() {
+        let data_dir = TempDir::new().expect("temp data dir");
+        seed_auth_dirs(data_dir.path(), &["a.quilt.dev"]);
+
+        let mut m = MockQuiltModel::new();
+        m.expect_clear_remote_client_cache().returning(|_| ());
+
+        let telemetry = Telemetry::default();
+        erase_auth_command(
+            data_dir.path(),
+            &m,
+            &RoleCache::default(),
+            &telemetry,
+            "never-logged-in.quilt.dev",
+        )
+        .await
+        .expect("a logout that finds nothing stored is still a success");
+
+        let reported = telemetry.reported_faults();
+        assert_eq!(
+            reported.len(),
+            1,
+            "exactly one signal per attempt: {reported:?}"
+        );
+        assert!(
+            reported[0].contains("Logout found no stored auth"),
+            "and it is the anomaly, not something else: {reported:?}"
+        );
+    }
+
+    /// The other side of the same rule: a logout that *does* erase something
+    /// reports no fault at all. Without this, the assertion above would pass on an
+    /// implementation that reported an anomaly every time.
+    #[tokio::test]
+    async fn a_successful_logout_reports_no_fault() {
+        let data_dir = TempDir::new().expect("temp data dir");
+        seed_auth_dirs(data_dir.path(), &["a.quilt.dev"]);
+
+        let mut m = MockQuiltModel::new();
+        m.expect_clear_remote_client_cache().returning(|_| ());
+
+        let telemetry = Telemetry::default();
+        erase_auth_command(
+            data_dir.path(),
+            &m,
+            &RoleCache::default(),
+            &telemetry,
+            "a.quilt.dev",
+        )
+        .await
+        .expect("logout");
+
+        assert!(
+            telemetry.reported_faults().is_empty(),
+            "erasing what was there is not an anomaly: {:?}",
+            telemetry.reported_faults()
+        );
+    }
+
     #[tokio::test]
     async fn logging_out_erases_only_its_own_host() {
         let data_dir = TempDir::new().expect("temp data dir");

quilt-sync/src-tauri/src/telemetry.rs

@@ -16,6 +16,7 @@ pub mod tracing;
 pub use event::MixpanelEvent;
 pub use install_id::InstallId;
 pub use mixpanel::Analytics;
+pub use sentry::Faults;
 pub use tracing::LogsDir;
 
 pub mod prelude {
@@ -79,8 +80,11 @@ pub struct Telemetry {
     host: AmbientHost,
     /// This install's identity, on every analytics event and every crash report
     /// so the two can be read together. `None` when it could not be persisted —
-    /// see [`install_id::load`].
+    /// see [`InstallId::load`].
     install_id: Option<InstallId>,
+    /// Where a fault goes. A field rather than a direct call to the crash SDK, so
+    /// that "this path reported a fault" is observable — see [`Faults`].
+    faults: Faults,
 }
 
 impl Telemetry {
@@ -98,6 +102,7 @@ impl Telemetry {
                 .map(::sentry::init),
             host,
             install_id,
+            faults: Faults::resolve(sinks),
         }
     }
 
@@ -135,7 +140,7 @@ impl Telemetry {
         if let Err(err) =
             mixpanel::track_event(&self.analytics, &event, self.install_id.as_ref()).await
         {
-            Sentry::capture_error(&err);
+            self.report_error(&err);
         }
     }
 
@@ -145,8 +150,8 @@ impl Telemetry {
     /// Keep `message` a constant: the crash reporter groups by it, so the
     /// variable part belongs in the host tag ([`Self::add_host`]) rather than in
     /// the text, or one anomaly becomes one issue per host.
-    pub fn report_anomaly(message: &str) {
-        Sentry::capture_message(message, Sentry::Level::Warning);
+    pub fn report_anomaly(&self, message: &str) {
+        self.faults.anomaly(message);
     }
 
     /// Report a fault to the crash reporter without failing the caller.
@@ -155,8 +160,16 @@ impl Telemetry {
     /// name its deployment and cannot. Reporting it as a fault keeps the
     /// analytics vocabulary free of error events, which would need their own
     /// design for what an error is allowed to say.
-    pub fn report_error(err: &(dyn std::error::Error + Send + Sync + 'static)) {
-        Sentry::capture_error(err);
+    pub fn report_error(&self, err: &(dyn std::error::Error + Send + Sync + 'static)) {
+        self.faults.error(err);
+    }
+
+    /// What was reported through [`Self::report_anomaly`] and
+    /// [`Self::report_error`] — the seam that made this unit worth doing. Without
+    /// it a path can claim to report a fault and no test can tell.
+    #[cfg(test)]
+    pub fn reported_faults(&self) -> Vec<String> {
+        self.faults.reported()
     }
 
     pub fn init(&self) {
@@ -181,6 +194,9 @@ impl Default for Telemetry {
             analytics: Analytics::Off,
             host: Arc::new(Mutex::new(None)),
             install_id: None,
+            // Recording rather than printing, so a test can assert what a path
+            // reported instead of a developer reading it go by.
+            faults: Faults::Recorded(Arc::new(Mutex::new(Vec::new()))),
         }
     }
 }

quilt-sync/src-tauri/src/telemetry/sentry.rs

@@ -1,7 +1,92 @@
 use semver::Version;
 
 use crate::env;
-use crate::telemetry::{AmbientHost, InstallId};
+use crate::telemetry::{AmbientHost, InstallId, Sinks};
+
+/// Where a fault goes — a signal that something should not have happened, which
+/// no event describes.
+///
+/// A field rather than a direct call to the SDK, and that is the whole point: the
+/// crash client is a process global, so reporting through it left **nothing a test
+/// could observe**. Instrumenting the background engine turns mostly on failures,
+/// and failure counting rides this sink rather than the analytics vocabulary, so
+/// the mechanism the telemetry work leans on for failure visibility was the one
+/// nothing could assert against.
+///
+/// Deliberately not a trait: this is a seam for *observation*, not an abstraction
+/// over vendors — the sinks are three different shapes and the crash SDK actively
+/// resists wrapping.
+#[derive(Clone)]
+pub enum Faults {
+    /// A release build: captured by the crash client, if one was configured.
+    Live,
+    /// A local build: written to the developer's console, because there is no
+    /// crash client to hold it. Today these vanish entirely in a dev build.
+    DryRun,
+    /// Tests: recorded in memory, so a test can assert that a path reported.
+    #[cfg(test)]
+    Recorded(std::sync::Arc<std::sync::Mutex<Vec<String>>>),
+}
+
+impl Faults {
+    pub fn resolve(sinks: Sinks) -> Self {
+        if sinks.reports_crashes() {
+            Self::Live
+        } else {
+            Self::DryRun
+        }
+    }
+
+    /// Report an anomaly: something that should not have happened, with no `Err`
+    /// to carry it.
+    ///
+    /// `message` must stay constant — the crash reporter groups by it, so a
+    /// variable part belongs in the host tag rather than the text, or one anomaly
+    /// becomes one issue per host.
+    pub fn anomaly(&self, message: &str) {
+        match self {
+            // The returned event id is discarded: nothing here correlates a fault
+            // back to its report, and returning it would invite a caller to think
+            // something does.
+            Self::Live => {
+                sentry::capture_message(message, sentry::Level::Warning);
+            }
+            Self::DryRun => eprintln!("telemetry(dry-run) anomaly: {message}"),
+            #[cfg(test)]
+            Self::Recorded(recorded) => Self::record(recorded, format!("anomaly: {message}")),
+        }
+    }
+
+    /// Report a fault the caller is not failing on.
+    pub fn error(&self, err: &(dyn std::error::Error + Send + Sync + 'static)) {
+        match self {
+            Self::Live => {
+                sentry::capture_error(err);
+            }
+            Self::DryRun => eprintln!("telemetry(dry-run) fault: {err}"),
+            #[cfg(test)]
+            Self::Recorded(recorded) => Self::record(recorded, format!("fault: {err}")),
+        }
+    }
+
+    #[cfg(test)]
+    fn record(recorded: &std::sync::Mutex<Vec<String>>, entry: String) {
+        // A poisoned lock would mean another test thread panicked mid-record.
+        // Dropping the entry is right: the panic is the failure worth reporting,
+        // and a second panic here would bury it.
+        if let Ok(mut recorded) = recorded.lock() {
+            recorded.push(entry);
+        }
+    }
+
+    #[cfg(test)]
+    pub fn reported(&self) -> Vec<String> {
+        match self {
+            Self::Recorded(recorded) => recorded.lock().map(|r| r.clone()).unwrap_or_default(),
+            _ => Vec::new(),
+        }
+    }
+}
 
 fn get_sentry_dsn() -> Option<sentry::types::Dsn> {
     env::sentry_dsn().and_then(|dsn_str| {
@@ -72,3 +157,51 @@ pub fn sentry_config(
         options
     })
 }
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+    use crate::telemetry::Sinks;
+
+    fn recorder() -> Faults {
+        Faults::Recorded(std::sync::Arc::new(std::sync::Mutex::new(Vec::new())))
+    }
+
+    /// Both kinds of fault are recorded, distinguishably and in order — a test
+    /// asserting "an anomaly was reported" must not be satisfied by an error.
+    #[test]
+    fn records_anomalies_and_errors_apart() {
+        let faults = recorder();
+
+        faults.anomaly("a constant message");
+        faults.error(&std::io::Error::other("something broke"));
+
+        assert_eq!(
+            faults.reported(),
+            vec![
+                "anomaly: a constant message".to_owned(),
+                "fault: something broke".to_owned()
+            ]
+        );
+    }
+
+    /// A build that reports nowhere observable reports *nothing* to a reader —
+    /// so a test holding a live or dry-run sink cannot accidentally pass by
+    /// reading someone else's recording.
+    #[test]
+    fn only_the_recorder_reports() {
+        assert!(Faults::Live.reported().is_empty());
+        assert!(Faults::DryRun.reported().is_empty());
+    }
+
+    /// A local build has no crash client, so its faults go to the console rather
+    /// than nowhere — which is what they did before.
+    #[test]
+    fn a_local_build_dry_runs_its_faults() {
+        assert!(matches!(
+            Faults::resolve(Sinks::Development),
+            Faults::DryRun
+        ));
+        assert!(matches!(Faults::resolve(Sinks::Production), Faults::Live));
+    }
+}