From 6c3d1dfb76411bbd3d8cb9631e61a9e20f26b9a3 Mon Sep 17 00:00:00 2001 From: David Abram Date: Wed, 21 Oct 2026 10:08:54 +0200 Subject: [PATCH 1/4] runtime+storage: Classify telemetry credential lookup outcomes Return typed missing, expired, unusable, and valid states from auth.db, and map them to the corresponding managed telemetry fallbacks. Update runtime, multiprocess, and storage tests for the new classification while preserving ownership checks. Plan: remove-telemetry-readiness-marker (T01) Co-authored-by: SCE --- cli/src/app.rs | 14 +- .../services/auth_db/multiprocess_tests.rs | 7 +- cli/src/services/managed_telemetry.rs | 92 +++-- .../services/telemetry_credential_storage.rs | 301 +++++++++------ .../remove-telemetry-readiness-marker.md | 348 ++++++++++++++++++ context/sce/telemetry-credential.md | 15 +- 6 files changed, 631 insertions(+), 146 deletions(-) create mode 100644 context/plans/remove-telemetry-readiness-marker.md diff --git a/cli/src/app.rs b/cli/src/app.rs index 00f749123..e6efdd018 100644 --- a/cli/src/app.rs +++ b/cli/src/app.rs @@ -913,20 +913,20 @@ pub(crate) mod tests { } async fn stalled_keychain_lookup() -> Result< - Option, + services::telemetry_credential_storage::TelemetryCredentialLookup, services::token_storage::TokenStorageError, > { let _ = tokio::task::spawn_blocking(|| std::thread::sleep(Duration::from_hours(1))).await; - Ok(None) + Ok(services::telemetry_credential_storage::TelemetryCredentialLookup::Missing) } async fn stalled_blocking_database_lookup() -> Result< - Option, + services::telemetry_credential_storage::TelemetryCredentialLookup, services::token_storage::TokenStorageError, > { tokio::task::yield_now().await; std::thread::sleep(Duration::from_hours(1)); - Ok(None) + Ok(services::telemetry_credential_storage::TelemetryCredentialLookup::Missing) } #[test] @@ -990,7 +990,11 @@ pub(crate) mod tests { false, || true, |_: &std::path::Path| Some("repository-a".to_string()), - |_: String| stalled_keychain_lookup(), + |_: String| async { + stalled_keychain_lookup().await.map( + services::telemetry_credential_storage::TelemetryCredentialLookup::into_valid, + ) + }, )); finish_runtime(runtime); let mut stdout = Vec::new(); diff --git a/cli/src/services/auth_db/multiprocess_tests.rs b/cli/src/services/auth_db/multiprocess_tests.rs index 659be9b8b..52f0b0b93 100644 --- a/cli/src/services/auth_db/multiprocess_tests.rs +++ b/cli/src/services/auth_db/multiprocess_tests.rs @@ -29,7 +29,7 @@ use crate::services::observability::traits::{NoopLogger, Telemetry}; use crate::services::telemetry_credential_storage::{ bind_repository_telemetry, load_repository_telemetry_credential, load_telemetry_credential, save_telemetry_credential, test_access_token, RepositoryBindingMode, TelemetryCredential, - TelemetryIdentity, + TelemetryCredentialLookup, TelemetryIdentity, }; const CHILD_TEST: &str = "services::auth_db::multiprocess_tests::auth_db_child_process"; @@ -1051,6 +1051,7 @@ async fn auth_db_child_process() { let credential = load_telemetry_credential() .await .expect("credential lookup succeeds") + .into_valid() .expect("credential present"); println!("{TOKEN_PREFIX}{}", credential.token); } @@ -1067,7 +1068,7 @@ async fn auth_db_child_process() { load_telemetry_credential() .await .expect("missing auth DB is not an error"), - None + TelemetryCredentialLookup::Missing ); assert!(!auth_db_path().expect("auth DB path").exists()); } @@ -1134,7 +1135,7 @@ async fn run_binding_child(role: &str) { load_telemetry_credential() .await .expect("missing auth DB is not an error"), - None + TelemetryCredentialLookup::Missing ); } "bind" => { diff --git a/cli/src/services/managed_telemetry.rs b/cli/src/services/managed_telemetry.rs index c8368b8c4..4d42b2af7 100644 --- a/cli/src/services/managed_telemetry.rs +++ b/cli/src/services/managed_telemetry.rs @@ -12,7 +12,7 @@ use trace_export::{ManagedSpanKind, ManagedTraceTelemetry}; use crate::services::default_paths::telemetry_readiness_path; use crate::services::telemetry_credential_storage::{ - load_telemetry_credential, TelemetryCredential, + load_telemetry_credential, TelemetryCredential, TelemetryCredentialLookup, }; use crate::services::telemetry_readiness::{self, ReadinessState}; use crate::services::token_storage::TokenStorageError; @@ -212,17 +212,23 @@ pub(crate) async fn load_credential_within_deadline( lookup: Fut, ) -> Result where - Fut: Future, TokenStorageError>> + Send + 'static, + Fut: Future> + Send + 'static, { match run_with_deadline(CREDENTIAL_LOOKUP_DEADLINE, lookup).await { None => Err(TelemetryFallback::new( TelemetryFallbackReason::LookupTimeout, )), Some(Err(error)) => Err(TelemetryFallback::from_storage_error(&error)), - Some(Ok(None)) => Err(TelemetryFallback::new( + Some(Ok(TelemetryCredentialLookup::Missing)) => Err(TelemetryFallback::new( + TelemetryFallbackReason::NotConfigured, + )), + Some(Ok(TelemetryCredentialLookup::Expired)) => Err(TelemetryFallback::new( + TelemetryFallbackReason::CredentialExpired, + )), + Some(Ok(TelemetryCredentialLookup::Unusable)) => Err(TelemetryFallback::new( TelemetryFallbackReason::CredentialUnavailable, )), - Some(Ok(Some(credential))) => Ok(credential), + Some(Ok(TelemetryCredentialLookup::Valid(credential))) => Ok(credential), } } @@ -259,7 +265,7 @@ pub(crate) async fn inspect_health_with( where R: FnOnce() -> anyhow::Result, H: FnOnce() -> bool, - Fut: Future, TokenStorageError>> + Send + 'static, + Fut: Future> + Send + 'static, { if !telemetry_enabled { return ManagedTelemetryHealth::DisabledByConfig { @@ -329,9 +335,7 @@ impl RuntimeTelemetry { ) -> (Self, Option) where R: FnOnce() -> bool, - Fut: Future, TokenStorageError>> - + Send - + 'static, + Fut: Future> + Send + 'static, { if !telemetry_enabled { return ( @@ -357,9 +361,7 @@ impl RuntimeTelemetry { span_kind: ManagedSpanKind, ) -> (Self, Option) where - Fut: Future, TokenStorageError>> - + Send - + 'static, + Fut: Future> + Send + 'static, { let credential = match load_credential_within_deadline(lookup).await { Ok(credential) => credential, @@ -483,7 +485,7 @@ mod tests { || false, async move { flag.store(true, std::sync::atomic::Ordering::SeqCst); - Ok(None) + Ok(TelemetryCredentialLookup::Missing) }, ManagedSpanKind::ClaudeHook, ) @@ -496,7 +498,7 @@ mod tests { assert!(!looked_up.load(std::sync::atomic::Ordering::SeqCst)); } - fn forbidden_lookup() -> Result, TokenStorageError> { + fn forbidden_lookup() -> Result { panic!("credential lookup must not run when telemetry is disabled by config") } @@ -591,7 +593,7 @@ mod tests { ); let (expired, fallback) = RuntimeTelemetry::resolve_with( - async { Ok(Some(credential("bao-token", 1))) }, + async { Ok(TelemetryCredentialLookup::Valid(credential("bao-token", 1))) }, ManagedSpanKind::ClaudeHook, ) .await; @@ -601,20 +603,36 @@ mod tests { assert!(!fallback.reason.is_failure()); assert_eq!(fallback.detail, None); - let (missing, fallback) = - RuntimeTelemetry::resolve_with(async { Ok(None) }, ManagedSpanKind::ClaudeHook).await; - assert!(matches!(missing, RuntimeTelemetry::Noop(_))); - assert_eq!( - fallback.map(|fallback| fallback.reason), - Some(TelemetryFallbackReason::CredentialUnavailable) - ); + for (lookup, reason) in [ + ( + TelemetryCredentialLookup::Missing, + TelemetryFallbackReason::NotConfigured, + ), + ( + TelemetryCredentialLookup::Expired, + TelemetryFallbackReason::CredentialExpired, + ), + ( + TelemetryCredentialLookup::Unusable, + TelemetryFallbackReason::CredentialUnavailable, + ), + ] { + let (telemetry, fallback) = + RuntimeTelemetry::resolve_with(async { Ok(lookup) }, ManagedSpanKind::ClaudeHook) + .await; + assert!(matches!(telemetry, RuntimeTelemetry::Noop(_))); + let fallback = fallback.expect("unusable lookup is diagnosed"); + assert_eq!(fallback.reason, reason); + assert!(!fallback.reason.is_failure()); + assert_eq!(fallback.detail, None); + } } fn tracked_lookup( - result: Result, TokenStorageError>, + result: Result, ) -> ( std::sync::Arc, - impl Future, TokenStorageError>> + Send + 'static, + impl Future> + Send + 'static, ) { let looked_up = std::sync::Arc::new(std::sync::atomic::AtomicBool::new(false)); let flag = looked_up.clone(); @@ -636,7 +654,9 @@ mod tests { TelemetryFallbackReason::CredentialExpired, ), ] { - let (looked_up, lookup) = tracked_lookup(Ok(Some(credential("bao-token", NOW + 60)))); + let (looked_up, lookup) = tracked_lookup(Ok(TelemetryCredentialLookup::Valid( + credential("bao-token", NOW + 60), + ))); let health = inspect_health_with(true, || Ok(readiness), || false, lookup, NOW).await; assert_eq!(health, ManagedTelemetryHealth::Disabled(reason)); assert!(!looked_up.load(std::sync::atomic::Ordering::SeqCst)); @@ -646,11 +666,14 @@ mod tests { #[tokio::test] async fn health_with_readiness_classifies_lookup_outcomes() { let cases: Vec<( - Result, TokenStorageError>, + Result, ManagedTelemetryHealth, )> = vec![ ( - Ok(Some(credential("bao-token", NOW + 60))), + Ok(TelemetryCredentialLookup::Valid(credential( + "bao-token", + NOW + 60, + ))), ManagedTelemetryHealth::Healthy, ), ( @@ -658,11 +681,22 @@ mod tests { ManagedTelemetryHealth::Disabled(TelemetryFallbackReason::CredentialInaccessible), ), ( - Ok(None), + Ok(TelemetryCredentialLookup::Missing), + ManagedTelemetryHealth::Disabled(TelemetryFallbackReason::NotConfigured), + ), + ( + Ok(TelemetryCredentialLookup::Expired), + ManagedTelemetryHealth::Disabled(TelemetryFallbackReason::CredentialExpired), + ), + ( + Ok(TelemetryCredentialLookup::Unusable), ManagedTelemetryHealth::Disabled(TelemetryFallbackReason::CredentialUnavailable), ), ( - Ok(Some(credential("bao-token", NOW))), + Ok(TelemetryCredentialLookup::Valid(credential( + "bao-token", + NOW, + ))), ManagedTelemetryHealth::Disabled(TelemetryFallbackReason::CredentialExpired), ), ( @@ -691,7 +725,7 @@ mod tests { true, || Ok(ReadinessState::Ready), || false, - std::future::pending::, TokenStorageError>>(), + std::future::pending::>(), NOW, ) .await; diff --git a/cli/src/services/telemetry_credential_storage.rs b/cli/src/services/telemetry_credential_storage.rs index 1d1afeeee..2392f4f00 100644 --- a/cli/src/services/telemetry_credential_storage.rs +++ b/cli/src/services/telemetry_credential_storage.rs @@ -67,12 +67,59 @@ impl fmt::Debug for TelemetryCredential { } } -impl TelemetryCredential { - fn is_valid_at(&self, now_unix_seconds: u64) -> bool { - !self.token.trim().is_empty() - && !self.accessor.trim().is_empty() - && self.identity.is_complete() - && self.expires_at_unix_seconds > now_unix_seconds +#[derive(Clone, Debug, Eq, PartialEq)] +pub enum TelemetryCredentialLookup { + Missing, + Expired, + Unusable, + Valid(TelemetryCredential), +} + +impl TelemetryCredentialLookup { + pub fn into_valid(self) -> Option { + match self { + Self::Valid(credential) => Some(credential), + Self::Missing | Self::Expired | Self::Unusable => None, + } + } +} + +struct StoredTelemetryCredential { + token: String, + accessor: String, + expires_at: i64, + identity: TelemetryIdentity, + session_access_token: Option, +} + +impl StoredTelemetryCredential { + fn classify(self, now_unix_seconds: u64) -> TelemetryCredentialLookup { + let Ok(expires_at_unix_seconds) = u64::try_from(self.expires_at) else { + return TelemetryCredentialLookup::Unusable; + }; + if self.token.trim().is_empty() + || self.accessor.trim().is_empty() + || !self.identity.is_complete() + { + return TelemetryCredentialLookup::Unusable; + } + let owned = self + .session_access_token + .as_deref() + .and_then(TelemetryIdentity::from_access_token) + .is_some_and(|session_identity| session_identity == self.identity); + if !owned { + return TelemetryCredentialLookup::Unusable; + } + if expires_at_unix_seconds <= now_unix_seconds { + return TelemetryCredentialLookup::Expired; + } + TelemetryCredentialLookup::Valid(TelemetryCredential { + token: self.token, + accessor: self.accessor, + expires_at_unix_seconds, + identity: self.identity, + }) } } @@ -89,9 +136,9 @@ pub async fn save_telemetry_credential( save_with(open_or_create_auth_db().await?, credential).await } -pub async fn load_telemetry_credential() -> Result, TokenStorageError> { +pub async fn load_telemetry_credential() -> Result { let Some(db) = open_existing_auth_db(CredentialAccess::NonInteractive).await? else { - return Ok(None); + return Ok(TelemetryCredentialLookup::Missing); }; load_with(db, current_unix_timestamp_seconds()?).await } @@ -167,37 +214,25 @@ async fn save_with( async fn load_with( db: &EncryptedTursoDb, now_unix_seconds: u64, -) -> Result, TokenStorageError> { - let rows: Vec> = db +) -> Result { + let rows: Vec = db .query_map( "SELECT t.token, t.accessor, t.expires_at_unix_seconds, t.user_id, t.workspace_id, \ a.access_token \ - FROM telemetry_credentials t JOIN auth_credentials a ON a.id = ?2 \ + FROM telemetry_credentials t LEFT JOIN auth_credentials a ON a.id = ?2 \ WHERE t.id = ?1", (TELEMETRY_CREDENTIAL_ROW_ID, WORKOS_SESSION_ROW_ID), |row| { - let token: String = row.get(0)?; - let accessor: String = row.get(1)?; - let expires_at: i64 = row.get(2)?; - let user_id: String = row.get(3)?; - let workspace_id: String = row.get(4)?; - let session_access_token: String = row.get(5)?; - Ok(u64::try_from(expires_at) - .ok() - .map(|expires_at_unix_seconds| { - ( - TelemetryCredential { - token, - accessor, - expires_at_unix_seconds, - identity: TelemetryIdentity { - user_id, - workspace_id, - }, - }, - session_access_token, - ) - })) + Ok(StoredTelemetryCredential { + token: row.get(0)?, + accessor: row.get(1)?, + expires_at: row.get(2)?, + identity: TelemetryIdentity { + user_id: row.get(3)?, + workspace_id: row.get(4)?, + }, + session_access_token: row.get(5)?, + }) }, ) .await @@ -206,13 +241,9 @@ async fn load_with( Ok(rows .into_iter() .next() - .flatten() - .filter(|(credential, session_access_token)| { - credential.is_valid_at(now_unix_seconds) - && TelemetryIdentity::from_access_token(session_access_token).as_ref() - == Some(&credential.identity) - }) - .map(|(credential, _)| credential)) + .map_or(TelemetryCredentialLookup::Missing, |stored| { + stored.classify(now_unix_seconds) + })) } async fn load_for_repository_with( @@ -220,7 +251,7 @@ async fn load_for_repository_with( now_unix_seconds: u64, repository_id: &str, ) -> Result, TokenStorageError> { - let Some(credential) = load_with(db, now_unix_seconds).await? else { + let Some(credential) = load_with(db, now_unix_seconds).await?.into_valid() else { return Ok(None); }; let authorized = repository_bound_to(db, repository_id, &credential.identity).await?; @@ -260,7 +291,7 @@ async fn bind_repository_with( if repository_id.trim().is_empty() { return Ok(false); } - let Some(credential) = load_with(db, now_unix_seconds).await? else { + let Some(credential) = load_with(db, now_unix_seconds).await?.into_valid() else { return Ok(false); }; let sql = match mode { @@ -472,66 +503,106 @@ mod tests { let loaded = load_with(&reopened, now()) .await .expect("load should succeed"); - assert_eq!(loaded, Some(saved)); - } - - #[tokio::test] - async fn telemetry_credential_storage_does_not_return_expired_record() { - let (_guard, db) = open_db().await; - save_with(&db, &credential("tok-expired", now() - 10)) - .await - .expect("save should succeed"); - - assert_eq!( - load_with(&db, now()).await.expect("load should succeed"), - None - ); + assert_eq!(loaded, TelemetryCredentialLookup::Valid(saved)); } - #[tokio::test] - async fn telemetry_credential_storage_does_not_return_corrupt_record() { - let (_guard, db) = open_db().await; + async fn insert_raw_credential(db: &TestDb, token: &str, user_id: &str, expires_at: i64) { db.execute( "INSERT OR REPLACE INTO telemetry_credentials \ - (id, token, accessor, expires_at_unix_seconds, stored_at_unix_seconds) \ - VALUES (1, '', 'acc', ?1, 0)", - (i64::try_from(FAR_FUTURE).expect("fits"),), + (id, token, accessor, expires_at_unix_seconds, stored_at_unix_seconds, user_id, workspace_id) \ + VALUES (1, ?1, 'acc', ?2, 0, ?3, ?4)", + ( + token, + expires_at, + user_id, + if user_id.is_empty() { "" } else { WORKSPACE_A }, + ), ) .await - .expect("empty token row should insert"); - assert_eq!( - load_with(&db, now()).await.expect("load should succeed"), - None - ); + .expect("raw telemetry row should insert"); + } - db.execute( - "INSERT OR REPLACE INTO telemetry_credentials \ - (id, token, accessor, expires_at_unix_seconds, stored_at_unix_seconds) \ - VALUES (1, 'tok', 'acc', -5, 0)", - (), - ) - .await - .expect("negative expiry row should insert"); - assert_eq!( - load_with(&db, now()).await.expect("load should succeed"), - None - ); + #[derive(Clone, Copy, Debug)] + enum StoredState { + NoRow, + Valid, + Expired, + NoWorkosSession, + IdentityMismatch, + LegacyIdentityless, + EmptyToken, + NegativeExpiry, } #[tokio::test] - async fn telemetry_credential_is_rejected_without_owning_workos_session() { + async fn telemetry_credential_lookup_classifies_auth_db_states() { + let far_future = i64::try_from(FAR_FUTURE).expect("fits"); let (_guard, db) = open_db().await; - save_with(&db, &credential("tok-orphan", FAR_FUTURE)) - .await - .expect("save should succeed"); - db.execute("DELETE FROM auth_credentials", ()) - .await - .expect("workos row should be cleared"); - - assert_eq!( - load_with(&db, now()).await.expect("load should succeed"), - None - ); + for (state, expected) in [ + (StoredState::NoRow, TelemetryCredentialLookup::Missing), + ( + StoredState::Valid, + TelemetryCredentialLookup::Valid(credential("tok-valid", FAR_FUTURE)), + ), + (StoredState::Expired, TelemetryCredentialLookup::Expired), + ( + StoredState::NoWorkosSession, + TelemetryCredentialLookup::Unusable, + ), + ( + StoredState::IdentityMismatch, + TelemetryCredentialLookup::Unusable, + ), + ( + StoredState::LegacyIdentityless, + TelemetryCredentialLookup::Unusable, + ), + (StoredState::EmptyToken, TelemetryCredentialLookup::Unusable), + ( + StoredState::NegativeExpiry, + TelemetryCredentialLookup::Unusable, + ), + ] { + delete_with(&db).await.expect("delete should succeed"); + seed_session(&db, USER_A, WORKSPACE_A).await; + match state { + StoredState::NoRow => {} + StoredState::Valid => save_with(&db, &credential("tok-valid", FAR_FUTURE)) + .await + .expect("save should succeed"), + StoredState::Expired => save_with(&db, &credential("tok-expired", now() - 10)) + .await + .expect("save should succeed"), + StoredState::NoWorkosSession => { + save_with(&db, &credential("tok-orphan", FAR_FUTURE)) + .await + .expect("save should succeed"); + db.execute("DELETE FROM auth_credentials", ()) + .await + .expect("workos row should be cleared"); + } + StoredState::IdentityMismatch => { + save_with(&db, &credential("tok-a", FAR_FUTURE)) + .await + .expect("save should succeed"); + seed_session(&db, USER_B, WORKSPACE_A).await; + } + StoredState::LegacyIdentityless => { + insert_raw_credential(&db, "tok-legacy", "", far_future).await; + } + StoredState::EmptyToken => { + insert_raw_credential(&db, "", USER_A, far_future).await; + } + StoredState::NegativeExpiry => { + insert_raw_credential(&db, "tok", USER_A, -5).await; + } + } + assert_eq!( + load_with(&db, now()).await.expect("load should succeed"), + expected, + "{state:?}" + ); + } } #[tokio::test] @@ -564,15 +635,23 @@ mod tests { save_with(&db, &credential("tok-first", FAR_FUTURE)) .await .expect("save should succeed"); - let first = render_telemetry_headers(load_with(&db, now()).await) - .expect("stored credential should render"); + let first = render_telemetry_headers( + load_with(&db, now()) + .await + .map(TelemetryCredentialLookup::into_valid), + ) + .expect("stored credential should render"); assert_eq!(first, r#"{"Authorization":"Bearer tok-first"}"#); save_with(&db, &credential("tok-second", FAR_FUTURE)) .await .expect("replacement save should succeed"); - let second = render_telemetry_headers(load_with(&db, now()).await) - .expect("replacement should render"); + let second = render_telemetry_headers( + load_with(&db, now()) + .await + .map(TelemetryCredentialLookup::into_valid), + ) + .expect("replacement should render"); assert_eq!(second, r#"{"Authorization":"Bearer tok-second"}"#); } @@ -581,14 +660,22 @@ mod tests { use crate::services::auth_command::render_telemetry_headers; let (_guard, db) = open_db().await; - let missing = render_telemetry_headers(load_with(&db, now()).await) - .expect_err("missing credential should fail"); + let missing = render_telemetry_headers( + load_with(&db, now()) + .await + .map(TelemetryCredentialLookup::into_valid), + ) + .expect_err("missing credential should fail"); save_with(&db, &credential("tok-expired", now() - 10)) .await .expect("save should succeed"); - let expired = render_telemetry_headers(load_with(&db, now()).await) - .expect_err("expired credential should fail"); + let expired = render_telemetry_headers( + load_with(&db, now()) + .await + .map(TelemetryCredentialLookup::into_valid), + ) + .expect_err("expired credential should fail"); db.execute( "INSERT OR REPLACE INTO telemetry_credentials \ @@ -598,8 +685,12 @@ mod tests { ) .await .expect("malformed row should insert"); - let malformed = render_telemetry_headers(load_with(&db, now()).await) - .expect_err("malformed credential should fail"); + let malformed = render_telemetry_headers( + load_with(&db, now()) + .await + .map(TelemetryCredentialLookup::into_valid), + ) + .expect_err("malformed credential should fail"); for error in [missing, expired, malformed] { assert_eq!(error.class().exit_code(), 4); @@ -667,6 +758,7 @@ mod tests { let loaded = load_with(&db, now()) .await .expect("load should succeed") + .into_valid() .expect("one valid record should exist"); assert!(loaded.token.starts_with("tok-")); assert_eq!(loaded.accessor, format!("accessor-{}", loaded.token)); @@ -700,7 +792,7 @@ mod tests { seed_session(&db, user_id, workspace_id).await; assert_eq!( load_with(&db, now()).await.expect("load should succeed"), - None + TelemetryCredentialLookup::Unusable ); } @@ -709,6 +801,7 @@ mod tests { load_with(&db, now()) .await .expect("load should succeed") + .into_valid() .map(|credential| credential.token), Some("tok-a".to_string()) ); @@ -728,7 +821,7 @@ mod tests { assert_eq!( load_with(&db, now()).await.expect("load should succeed"), - None + TelemetryCredentialLookup::Unusable ); assert!( !bind_repository_with(&db, now(), REPOSITORY_A, RepositoryBindingMode::Enroll) @@ -869,7 +962,7 @@ mod tests { assert!(result.is_err()); assert_eq!( load_with(&db, now()).await.expect("load should succeed"), - None + TelemetryCredentialLookup::Missing ); } diff --git a/context/plans/remove-telemetry-readiness-marker.md b/context/plans/remove-telemetry-readiness-marker.md new file mode 100644 index 000000000..3c6392443 --- /dev/null +++ b/context/plans/remove-telemetry-readiness-marker.md @@ -0,0 +1,348 @@ +# Plan: remove-telemetry-readiness-marker + +## Change summary + +Remove the non-secret `/sce/telemetry-ready.json` readiness hint and make the encrypted `auth.db` the only source of truth for whether a usable telemetry credential exists. The effective access policy (global, Git-root `.sce/config.json`, and explicit config layers) still decides whether telemetry is permitted. The OS credential store still guards the encryption key. Nothing replaces the marker: no cache file, no new database, no new persisted state. + +This replaces existing behavior. Today every telemetry consumer must see both the marker and a valid `auth.db` row. The marker can go stale, it adds write and clear steps to login, logout, and session replacement, and it duplicates what `auth.db` already records. Its one real job is to keep hooks away from `auth.db` and the Keychain when telemetry was never provisioned. This plan keeps that protection with an existence check on `auth.db` plus a typed lookup. It also closes a hazard that removing the marker would otherwise widen: today the read-only lookup path silently migrates an outdated `auth.db`. + +### Current implementation findings + +Marker module and path: + +- `cli/src/services/telemetry_readiness.rs`: `ReadinessState { NotConfigured, Expired, Ready }`, `mark_ready`, `clear`, `is_ready`, `state`, `mark_ready_at`, `clear_at`, `state_at`, `is_ready_at` (test-only), `temp_path_for`. Its three tests: `readiness_state_requires_valid_unexpired_marker_and_auth_db`, `marked_readiness_is_ready_until_expiry_and_holds_no_secret`, `clearing_readiness_disables_it_and_is_idempotent`. +- `cli/src/services/mod.rs:40`: `pub mod telemetry_readiness;`. +- `cli/src/services/default_paths.rs:257`: `telemetry_readiness_path()`. + +Code that reads the marker: + +- `cli/src/services/managed_telemetry.rs`: + - `RuntimeTelemetry::resolve` passes `|| telemetry_readiness::is_ready(now)` into `resolve_when_ready`. When that returns false, the result is Noop `NotConfigured` and `auth.db` is never opened. + - `inspect_health` passes `telemetry_readiness::state(now)` and `readiness_hint_present()` into `inspect_health_with`. `NotConfigured` and `Expired` hints return without a lookup. + - `ManagedTelemetryHealth::DisabledByConfig { readiness_hint_present }`. +- `cli/src/services/auth_command/mod.rs`: `run_telemetry_headers` passes `|| telemetry_readiness::is_ready(..)` as `ready_probe` to `telemetry_headers_with`. When it fails, the helper fails closed before repository resolution, which spawns `git config`. +- `cli/src/services/doctor/render.rs:318-330`: `readiness_hint_present` decides whether the `OPT_IN_NOTE` line is shown under `[PASS] Managed telemetry (disabled by config)`. + +Code that writes or clears the marker (`cli/src/services/auth_command/mod.rs`): + +- `provision_telemetry_credential` calls `record_telemetry_readiness(.., mark_ready, clear)`. The result is `Issued` only when the credential was issued, persisted, and the marker was written. Otherwise the marker is cleared and the result is `Unavailable`. +- `run_logout` passes `telemetry_readiness::clear` into `run_logout_with`. The clear runs first, and a failure aborts logout. +- `save_new_session_tokens` passes `telemetry_readiness::clear` into `replace_session_with`, before the old credential is invalidated and the new session is saved. + +Tests that depend on the marker: + +- `auth_command/mod.rs`: + - `telemetry_readiness_is_recorded_only_after_credential_persistence` + - `new_session_invalidates_previous_telemetry_credential_before_saving` (expects `["clear", "invalidate", "save"]`) + - the logout tests that expect `["clear-readiness", "delete-workos", "delete-telemetry"]` (around lines 972, 1293, and 1668) + - the `telemetry_headers_with` table test, which has a `ready` column and a `probed` flag +- `managed_telemetry.rs`: + - `claude_hook_without_readiness_never_looks_up_credentials` + - `telemetry_disabled_by_config_never_probes_readiness_or_looks_up_credentials` + - `health_disabled_by_config_never_probes_readiness_or_looks_up_credentials` + - `health_without_readiness_or_with_expired_readiness_never_looks_up_credentials` + - `health_with_readiness_classifies_lookup_outcomes` + - `health_lookup_is_bounded_by_the_credential_lookup_deadline` + - `claude_hook_with_inaccessible_credential_store_skips_quietly` +- `auth_db/multiprocess_tests.rs`: + - `seed()` calls `telemetry_readiness::mark_ready(FAR_FUTURE)` (line 1211) + - `doctor_telemetry_health_classifies_real_auth_db_states_without_mutating_them` writes an expired marker and removes it (lines 481-492) + - `claude_hook_with_telemetry_disabled_by_config_never_resolves_the_credential` asserts the marker exists (line 872) +- `app/doctor_reconciliation_cli_tests.rs`: lines 647, 656, and 769 write `FAR_FUTURE_READINESS` or assert the marker is absent. + +Credential lookup today: + +- `telemetry_credential_storage::load_telemetry_credential` and `load_repository_telemetry_credential` call `token_storage::open_existing_auth_db(CredentialAccess::NonInteractive)`. That calls `EncryptedTursoDb::open_existing_at`, which returns `None` when `auth.db` is missing; `open_lock_file(.., false)` runs before any key resolution. +- `load_with` uses an inner `JOIN auth_credentials` and returns `Option`. Several different states all collapse to `None`: no row, no WorkOS session, expired, identity-less (pre-`004`), empty fields, and an ownership mismatch. +- Today the `credential_expired` diagnosis comes only from the marker (`ReadinessState::Expired`) or from `select_mode` on a returned row. A storage-level expiry is reported as `credential_unavailable`. +- Hazard: `EncryptedTursoDb::open_locked` always calls `TursoConnectionCore::ensure_migrations_current()`. On an outdated or schemaless `auth.db`, the non-interactive hook lookup therefore runs a `BEGIN IMMEDIATE` migration. Today the marker usually prevents this, because only a current-binary login writes it. Without the marker, every admitted Claude hook would migrate a legacy `auth.db` that has no credential, and `auth-db.md` documents an upstream Turso race when many processes migrate concurrently. + +Other facts the plan relies on: + +- `sce setup` no longer creates `auth.db`; `AuthDbLifecycle::setup` only creates the parent directory. So `auth.db` exists only after `sce auth login`, or on installs whose `auth.db` was created by an older `sce setup`. + - Drift: `context/overview.md:71` still says the auth_db provider runs `AuthDb::new()` setup. +- `managed_span_kind` (`cli/src/app.rs:308`) admits Claude hooks and ordinary commands to the startup lookup. `hooks` that are not Claude hooks, `setup`, and `auth telemetry-headers` stay Noop without a lookup. +- `CREDENTIAL_LOOKUP_DEADLINE` (2 s) bounds both lookups through `run_with_deadline`. +- `context/decisions/2026-10-20-consent-gated-access-policy-precedence.md:53` already rejects inferring consent from `telemetry-ready.json` or `auth.db`. This plan keeps that: `auth.db` affects credential readiness only, never consent. + +### Existing vs proposed readiness flow + +Existing: + +```text +policy disabled -> Noop(disabled_by_config) +policy enabled + -> marker missing/corrupt or auth.db missing -> Noop(not_configured) [no DB, no Keychain] + -> marker expired -> Noop(credential_expired) [no DB, no Keychain] + -> marker ready -> non-interactive lookup (may MIGRATE an outdated DB) + valid -> Managed; None -> credential_unavailable; errors -> classified +``` + +Proposed: + +```text +policy disabled -> Noop(disabled_by_config) [no stat of credentials, no DB, no Keychain] +policy enabled + -> auth.db missing (open without O_CREAT) -> Noop(not_configured) [no key resolution] + -> auth.db exists -> resolve key with CredentialAccess::NonInteractive + key inaccessible -> Noop(credential_inaccessible) + open, schema not current -> Noop(credential_unavailable) [never migrates, not cached] + open, schema current -> typed lookup within CREDENTIAL_LOOKUP_DEADLINE + Missing (no telemetry row) -> Noop(not_configured) + Expired (owned, complete, expired) -> Noop(credential_expired) + Unusable (no WorkOS session, identity mismatch, legacy/empty/negative row) -> Noop(credential_unavailable) + Valid -> Managed (bearer); exporter init failure -> exporter_init_failed + DB/lock error -> database_error; path error -> path_resolution_error; deadline -> lookup_timeout +``` + +The header helper follows the same policy gate and destination tripwire. It then checks that `auth.db` exists (a stat, so a never-logged-in machine spawns no `git`), resolves the repository, and prints a bearer only for `Valid` with a matching repository binding. Every other outcome fails closed with `Telemetry credential is unavailable.` + +### Credential lookup and fallback matrix + +| State | Runtime fallback (log level) | Doctor text / JSON `state`/`reason` | Header helper | +| --- | --- | --- | --- | +| Policy not `Enabled` | `disabled_by_config` (debug) | `[PASS] (disabled by config)` / `disabled_by_config`; `OPT_IN_NOTE` only when `auth.db` exists | fail closed before tripwire | +| `auth.db` missing | `not_configured` (debug) | `[PASS] (not configured)` / `not_configured` | fail closed, no `git` spawn | +| `auth.db` exists, no telemetry row (never provisioned, post-logout, telemetry-disabled login) | `not_configured` (debug) | `[PASS] (not configured)` / `not_configured` | fail closed | +| Row present, no WorkOS session / ownership mismatch / legacy identity-less / malformed | `credential_unavailable` (debug) | `[WARN]` / `disabled`/`credential_unavailable`, `auth_login` | fail closed | +| Row owned and complete but expired | `credential_expired` (debug) | `[WARN]` / `disabled`/`credential_expired`, `auth_login` | fail closed | +| Schema not current (outdated/schemaless DB) | `credential_unavailable` (debug) | `[WARN]` / `disabled`/`credential_unavailable`, `auth_login` | fail closed | +| Key not silently accessible | `credential_inaccessible` (debug) | `[WARN]` / `disabled`/`credential_inaccessible`, `auth_login` | fail closed | +| Wrong key, lock conflict, I/O | `database_error` (warn) | `[WARN]` / `disabled`/`database_error`, `manual_steps` | fail closed | +| State dir unresolvable | `path_resolution_error` (warn) | `[WARN]` / `path_resolution_error`, `manual_steps` | fail closed | +| Deadline exceeded | `lookup_timeout` (warn) | `[WARN]` / `lookup_timeout`, `manual_steps` | fail closed | +| Valid, exporter fails | `exporter_init_failed` (warn) | n/a (doctor starts no exporter) | n/a | +| Valid, repository unbound or bound elsewhere | runtime: Managed | `[PASS]` | fail closed | +| Valid | Managed | `[PASS] Managed telemetry` / `healthy` | bearer printed | + +`telemetry-ready.json`, whether present, missing, expired, or malformed, changes no row of this table. + +### Lifecycle changes + +- **Login:** `provision_telemetry_credential` returns `Issued` exactly when `issue_and_save_telemetry_credential` succeeds; only then does it enroll the working-directory repository (`EnrollIfUnbound`). Issuance or persistence failure returns `Unavailable` and writes nothing, so a credential stored earlier stays in place on the stored and renewed session paths, as the save-on-issue contract requires. On a fresh device login, `replace_session_with` still deletes the old telemetry credential before saving the new session, with the marker clear removed from the front of that sequence. A login with telemetry disabled stays `Disabled`: no issuance, no load, no enrollment. +- **Logout:** `run_logout_with` deletes the WorkOS row and the telemetry row with the existing APIs. It still attempts both and surfaces the first storage error, and it runs no policy check. Nothing comes before the deletes. A later lookup classifies `Missing`, so the result is `not_configured`. +- **Runtime:** `RuntimeTelemetry::resolve` checks policy and then runs the typed lookup. There is no marker probe. +- **Doctor:** `inspect_health` checks policy and then runs the same typed lookup under the deadline. `--fix` is unchanged: `manual_only`, no repair, and no storage initialization, key creation, or prompt. +- **Partial, stale, and concurrent operations:** saves stay single-statement `INSERT OR REPLACE` of row 1, and the last complete writer wins. Without a marker, a login that crashes after the save is simply ready, and a crash before the save is simply not ready. Previously the marker and the row could disagree in either direction. Concurrent readers share the multiprocess WAL, and readiness lookups never write. + +### Keychain, concurrency, and performance + +- **More reads, only after an `auth.db` exists.** When the policy is `Enabled` and `auth.db` exists but the marker would have been missing or expired, each admitted Claude hook and each admitted command now does one more non-interactive key read plus one encrypted open per process. Those cases are: after logout, a login with telemetry disabled, failed provisioning, a legacy setup-created `auth.db`, and expired credentials. The open costs about 3 ms of serialized cold open, a read-only `has_pending_migrations` check, and one `SELECT`, all under `CREDENTIAL_LOOKUP_DEADLINE`. + - Nothing changes when the policy is not `Enabled` or `auth.db` is missing (fresh installs). + - Previously ready installs do the same work as today. + - Exposure is limited to users who opted in with `telemetry.enabled: true`. +- **Hook safety:** lookups keep `CredentialAccess::NonInteractive`. On macOS that is `SecKeychainSetUserInteractionAllowed(false)`, so an unauthorized binary or a locked keychain fails with `errSecInteractionNotAllowed` and no dialog appears. Lookups never create a key: `open_existing_at` resolves the key only after an existing-file open succeeds, and `NonInteractive` never generates one. After T02, readiness lookups never migrate. An outdated schema yields `credential_unavailable` without writes and without caching the handle, so a later `sce auth login` in the same process still migrates. +- **Diagnostic shift:** a user with a legacy setup-created `auth.db` whose Keychain item this binary may not silently read will now see `[WARN] credential_inaccessible` (next action `auth_login`) instead of `[PASS] not configured`. This is accurate, because a login is required, but it is a visible change. +- **Linux and Windows:** the adapter has no interaction switch there; `NonInteractive` only guarantees that no key is created. This limitation already exists, but more hook invocations now reach Secret Service when `auth.db` exists. See Open questions. +- **Unchanged:** destination-conflict checks, repository-binding isolation, the compatibility lock, the cold-open lock, the busy timeout, code signing, Keychain ACLs, and encryption. +- **Follow-up:** verifying in a fresh process that a Keychain item is silently accessible right after `sce auth login` is a separate change, not part of this plan. + +## Acceptance criteria + +- [ ] AC1: A valid, owned, unexpired telemetry credential in `auth.db` enables managed telemetry, doctor health, and the header helper when no `telemetry-ready.json` exists. + - Validate: `nix develop -c ./scripts/run-cli-cargo.sh test --manifest-path cli/Cargo.toml valid_credential_without_readiness_marker` passes, covering runtime `Managed`, doctor `healthy`, and the helper bearer for the bound repository in a sandboxed state root. +- [ ] AC2: `telemetry-ready.json` is inert. A present-but-ready marker with no telemetry row reports `not_configured`. An expired or malformed marker next to a valid credential reports `healthy`. The file is never read, written, or removed. + - Validate: `nix develop -c ./scripts/run-cli-cargo.sh test --manifest-path cli/Cargo.toml legacy_readiness_marker_is_ignored` passes, and asserts the marker bytes are unchanged afterwards. +- [ ] AC3: The readiness classification matches the fallback matrix for a missing `auth.db`, an existing DB with no row, a valid row, an expired row, a missing WorkOS session, an identity mismatch, and a legacy identity-less row. A missing `auth.db` creates no DB file, sidecar, or key. + - Validate: `nix develop -c ./scripts/run-cli-cargo.sh test --manifest-path cli/Cargo.toml telemetry_credential_lookup_classifies` and `... doctor_telemetry_health` pass. +- [ ] AC4: Readiness lookups never migrate or initialize `auth.db`. Concurrent hook lookups against an outdated-schema `auth.db` leave its bytes and `__sce_migrations` unchanged, and each reports `credential_unavailable`. + - Validate: `nix develop -c ./scripts/run-cli-cargo.sh test --manifest-path cli/Cargo.toml readiness_lookup_never_migrates` passes. +- [ ] AC5: Telemetry disabled by policy never reaches the credential lookup, even with a valid credential stored. Doctor shows `OPT_IN_NOTE` only when `auth.db` exists. + - Validate: the policy-disabled tests in `managed_telemetry.rs`, `auth_command/mod.rs`, `doctor/render.rs`, and `claude_hook_with_telemetry_disabled_by_config_never_resolves_the_credential` pass. +- [ ] AC6: Login reports `issued` exactly when the credential is persisted, `unavailable` on issuance or persistence failure (keeping the prior row on the stored and renewed paths), and `disabled` with no issuance when telemetry is off. Logout deletes both rows, and a following lookup reports `not_configured`. + - Validate: the `auth_command/mod.rs` login and logout tests pass, and `nix develop -c ./scripts/run-cli-cargo.sh test --manifest-path cli/Cargo.toml logout_then_lookup_reports_not_configured` passes. +- [ ] AC7: No production code, test, or current-state context references the marker. + - Validate: `nix shell nixpkgs#ripgrep -c rg -n "telemetry-ready|telemetry_readiness|ReadinessState|mark_ready|readiness_hint" cli/ context/ --glob '!context/plans/**' --glob '!context/decisions/**' --glob '!context/handovers/**'` returns no matches. +- [ ] AC8: On macOS, an admitted Claude hook and `sce auth telemetry-headers` with an unauthorized or locked Keychain item show no dialog and resolve `credential_inaccessible` / fail closed. A previously authorized install keeps exporting after the marker file is deleted. + - Validate: manual inspection on macOS: delete `~/Library/Application Support/sce/telemetry-ready.json` (or the platform state root), then run a Claude session and `sce doctor --format json` and check that `managed_telemetry.state` is `healthy` with no prompt. Then lock the login keychain and rerun: no dialog, and `credential_inaccessible`. + +### Full validation + +- `nix flake check` + +### Context sync + +- `context/sce/telemetry-credential.md`: remove the "Telemetry readiness hint" section; rewrite Runtime use, Doctor health check (the table row "Hint ready, no usable credential" and the `telemetry-ready.json` opt-in sentence), and Hidden header helper for the `auth.db`-only flow and the typed lookup. +- `context/cli/access-policy.md`: the Enforcement bullets for managed telemetry, doctor, the header helper, login, and logout. +- `context/sce/cli-observability-contract.md`: the managed telemetry startup paragraph. +- `context/sce/auth-db.md`: Concurrency and schema readiness. Telemetry lookups open without migrating. +- `context/sce/shared-turso-db.md` and `context/glossary.md`: the new non-migrating encrypted existing-DB open. +- `context/overview.md:71`: fix the drift that says the auth_db provider runs `AuthDb::new()` setup. + +## Task context synchronization lifecycle + +- **Task context synchronization:** every task carries `pending | synced | blocked`. A completed task must be `synced` before another task can start or the plan can finish. +- For `blocked`, record **Blocker**, **Required action**, and **Retry condition** beside the status. Never infer `synced` from conversation history; write every lifecycle transition to the plan file. + +## Constraints and non-goals + +- **In scope:** + - `cli/src/services/telemetry_credential_storage.rs` + - `cli/src/services/token_storage.rs` + - `cli/src/services/db/mod.rs` (the encrypted existing-DB open only) + - `cli/src/services/managed_telemetry.rs` + - `cli/src/services/auth_command/mod.rs` + - `cli/src/services/doctor/render.rs` and `cli/src/services/doctor/managed_telemetry.rs` (only if a field rename requires it) + - `cli/src/services/telemetry_readiness.rs` (deleted) + - `cli/src/services/mod.rs` + - `cli/src/services/default_paths.rs` + - `cli/src/services/auth_db/multiprocess_tests.rs` + - `cli/src/app/doctor_reconciliation_cli_tests.rs` + - the context files listed under Context sync +- **Out of scope:** + - new interactive `sce setup` questions, or folding login into setup + - Claude-native settings reconciliation + - changes to telemetry consent or `auth.mode` precedence + - new `auth.db` schema or migrations + - replacing or reconfiguring the Keychain adapter, code signing, ACLs, or encryption + - fresh-process Keychain access verification + - deleting existing `telemetry-ready.json` files + - changing `sce sync`'s `NonInteractive` `load_tokens`, which keeps migrating + - Linux Secret Service prompt suppression +- **Constraints:** + - Keep `CredentialAccess::NonInteractive` and `CREDENTIAL_LOOKUP_DEADLINE` on every readiness path. + - Keep the exact `TelemetryFallbackReason` strings, doctor JSON fields, problem kinds, `next_action` values, and fixed text, including `OPT_IN_NOTE` and `Telemetry credential is unavailable.`. + - Keep the multiprocess WAL, compatibility lock, and cold-open lock behavior. + - Use static dispatch and typed enums, not boxed traits. + - Follow the no-comments-in-code preference. +- **Non-goal:** + - a replacement readiness cache, marker, or `auth.db` side table + - a migration step that deletes the old marker + - a generic "lookup policy" abstraction over every `open_existing_auth_db` caller + +## Assumptions + +- `OPT_IN_NOTE` under `disabled by config` keys off whether `auth.db` exists (a stat, with no credential access). That is the nearest marker-free signal for "an earlier login provisioned telemetry", and the note text stays unchanged. +- An `auth.db` with no telemetry row classifies as `not_configured` (`[PASS]`), matching the requested tree "Missing -> Not configured". A row that exists but is unusable stays `credential_unavailable`, whose doctor summary ("was configured, but its credential is no longer available") is still accurate. +- An outdated or schemaless `auth.db` on the readiness path maps to `credential_unavailable` with next action `auth_login`, because login migrates the DB through its interactive open. No new `TelemetryFallbackReason` is added. +- The header helper keeps a stat-only `auth.db` existence probe in place of `ready_probe`, so a never-logged-in machine spawns no `git config` process every refresh. A file-existence check is not persisted readiness state. +- Test filters named in acceptance criteria are the intended new or renamed test names; the implementing task creates them. + +## Task stack + +- [x] T01: `Return a typed telemetry credential lookup from auth.db` (status:done) + - Task ID: T01 + - Scope: In — typed lookup classification in storage and its mapping into `TelemetryFallbackReason`. Out — marker removal, the migration behavior on open, and helper changes. + - Files and functions: + - `cli/src/services/telemetry_credential_storage.rs`: a new `TelemetryCredentialLookup { Missing, Expired, Unusable, Valid(TelemetryCredential) }` (derive `Clone, Debug, Eq, PartialEq`, redacted through `TelemetryCredential`'s `Debug`); `load_with`; `load_telemetry_credential`; `load_for_repository_with`. + - `cli/src/services/managed_telemetry.rs`: `load_credential_within_deadline`, `inspect_health_with`, `RuntimeTelemetry::resolve_with`, and the lookup future bounds. + - Steps: + 1. Change `load_with` to `LEFT JOIN auth_credentials` and classify: + - no telemetry row: `Missing` + - incomplete or legacy identity, empty token or accessor, or negative expiry: `Unusable` + - no session row, or session claims that do not equal the identity: `Unusable` + - expiry not after `now`: `Expired` + - otherwise: `Valid` + 2. Change `load_telemetry_credential` to return `Result`, with a missing DB mapping to `Missing`. + 3. Keep `load_repository_telemetry_credential` returning `Option`: `Valid` plus a binding gives `Some`, everything else gives `None`. + 4. Map in `load_credential_within_deadline`: `Missing` to `NotConfigured`, `Expired` to `CredentialExpired`, `Unusable` to `CredentialUnavailable`, and `Valid` to the credential. + - Dependencies: none + - Done when: the lookup distinguishes missing, expired, unusable, and valid rows without opening anything new. All existing ownership, identity, and repository-binding checks still hold. The runtime and doctor map the new variants exactly as in the matrix. The marker still gates the lookup, because T03 removes that gate. + - Verify: + - A table-driven `telemetry_credential_lookup_classifies_auth_db_states` test in `telemetry_credential_storage.rs` covers no row, valid, expired, no WorkOS session, identity mismatch, and a legacy identity-less row. It replaces the separate expired and corrupt `None` assertions instead of duplicating them. + - The existing ownership, identity-switch, and repository-binding tests pass unchanged. + - `health_with_readiness_classifies_lookup_outcomes` is extended with `Missing`, `Expired`, and `Unusable` rows. + - `nix develop -c ./scripts/run-cli-cargo.sh test --manifest-path cli/Cargo.toml telemetry_credential` passes. + - Completed: 2026-10-21 + - Files changed: + - `cli/src/services/telemetry_credential_storage.rs` + - `cli/src/services/managed_telemetry.rs` + - `cli/src/services/auth_db/multiprocess_tests.rs` + - `cli/src/app.rs` + - Result: Added `TelemetryCredentialLookup { Missing, Expired, Unusable, Valid }` with `into_valid()`. `load_with` now uses `LEFT JOIN auth_credentials` and classifies a private `StoredTelemetryCredential` row: a negative expiry, an empty token or accessor, an incomplete identity, a missing session, or a session identity mismatch gives `Unusable`; an expiry at or before `now` gives `Expired`. `load_telemetry_credential` returns the typed lookup, and a missing DB maps to `Missing`. `load_for_repository_with`, `bind_repository_with`, and `load_repository_telemetry_credential` keep `Option`/`bool` through `into_valid()`. `load_credential_within_deadline` maps `Missing` to `NotConfigured`, `Expired` to `CredentialExpired`, and `Unusable` to `CredentialUnavailable`. The marker gate is unchanged. + - Verify: + - `telemetry_credential_lookup_classifies_auth_db_states` covers no row, valid, expired, no WorkOS session, identity mismatch, legacy identity-less, empty token, and negative expiry. It replaces the removed `telemetry_credential_storage_does_not_return_expired_record`, `telemetry_credential_storage_does_not_return_corrupt_record`, and `telemetry_credential_is_rejected_without_owning_workos_session`: passed. + - The ownership, identity-switch, legacy, and repository-binding tests pass. Their assertions changed only for the new return type (`into_valid()`, or `Unusable`/`Missing` in place of `None`). + - `health_with_readiness_classifies_lookup_outcomes` is extended with `Missing`, `Expired`, and `Unusable` rows. `claude_hook_credential_failure_or_expiry_resolves_noop_with_reason` now covers all three non-valid variants: passed. + - `nix develop -c ./scripts/run-cli-cargo.sh test --manifest-path cli/Cargo.toml telemetry_credential`: 27 passed. + - The additional `managed_telemetry` (42), `stalled` (10), and `multiprocess` (20) filters passed, and `clippy --all-targets -D warnings` is clean. + - Deviation: `cli/src/app.rs` test stubs (`stalled_keychain_lookup`, `stalled_blocking_database_lookup`, and the header-helper stall call) were retyped to the lookup enum. The task did not list this file, but the change in the lookup future type required it. + - Context impact: localized. When the marker is ready, an expired stored row now reports `credential_expired`, and a missing telemetry row now reports `not_configured`; both previously reported `credential_unavailable`. All other reasons are unchanged. The marker-free current-state context is rewritten when T03 removes the gate. + - Context synchronization: synced + +- [ ] T02: `Open auth.db for telemetry lookups without running migrations` (status:todo) + - Task ID: T02 + - Scope: In — a non-migrating encrypted existing-DB open used only by the two non-interactive telemetry lookups. Out — `load_tokens`, `delete_*`, `bind_repository_telemetry`, `sce sync`, and every interactive path, which all keep migrating. + - Files and functions: + - `cli/src/services/db/mod.rs`: a new `EncryptedTursoDb::open_existing_current_at(path, access)` next to `open_existing_at`. It reuses `open_lock_file(.., false)`, `resolve_encryption_key`, and the connect path, but replaces `ensure_migrations_current` with the read-only `has_pending_migrations`. It returns a typed result that distinguishes missing, schema-not-current, and open. + - `cli/src/services/token_storage.rs`: a lookup-only opener next to `open_existing_auth_db`. It reuses the cached `AUTH_DB` when that is set, and caches only a current-schema handle. + - `cli/src/services/telemetry_credential_storage.rs`: `load_telemetry_credential` and `load_repository_telemetry_credential` use that opener. Schema-not-current maps to `TelemetryCredentialLookup::Unusable` and `None` respectively. + - Steps: + 1. Factor the shared connect step out of `open_locked` so the migrating and non-migrating opens differ only in the schema step. + 2. On schema-not-current, drop the connection and do not populate `AUTH_DB`. + 3. Keep error typing unchanged: `CredentialAccessUnavailable`, `DatabaseLocked`, `Database`, `PathResolution`. + - Dependencies: T01 + - Done when: + - A telemetry lookup never runs a migration or DDL statement. The `BEGIN IMMEDIATE`-holder reader test still passes. + - An outdated or schemaless `auth.db` is left byte-identical after concurrent lookups. + - A process that ran a lookup on an outdated DB can still migrate it through a later interactive open. + - Verify: + - A new `readiness_lookup_never_migrates_outdated_auth_db` multiprocess test in `auth_db/multiprocess_tests.rs`: seed a DB at an older migration, run 8 concurrent `load_telemetry_credential` children, then assert the file and `__sce_migrations` are unchanged and every child reports the unusable classification. + - Extend the existing `current_schema_reader_succeeds_while_writer_holds_begin_immediate`-style test, or keep it unchanged if it already covers the current-schema path. + - `nix develop -c ./scripts/run-cli-cargo.sh test --manifest-path cli/Cargo.toml multiprocess` passes. + - Context synchronization: pending + +- [ ] T03: `Gate runtime, doctor and header helper on auth.db instead of the marker` (status:todo) + - Task ID: T03 + - Scope: In — remove every marker read. Out — marker writes and clears in login and logout, and deleting the module (both in T04). + - Files and functions: + - `cli/src/services/managed_telemetry.rs`: `RuntimeTelemetry::resolve` and `resolve_when_ready` (drop the `ready` parameter; keep the policy-first gate), `inspect_health` and `inspect_health_with` (drop the `readiness` parameter; replace `hint_present` with an `auth_db_present` probe), `readiness_hint_present` (replace it with `auth_db_path().is_ok_and(|p| p.is_file())`), `ManagedTelemetryHealth::DisabledByConfig { readiness_hint_present }` (rename the field to `auth_db_present`). + - `cli/src/services/doctor/render.rs:318-330` and its tests (around line 1320): the renamed field. Keep the note text. + - `cli/src/services/auth_command/mod.rs`: `run_telemetry_headers` and `telemetry_headers_with` (replace `ready_probe` with the `auth_db` existence probe; keep the ordering policy → tripwire → probe → repository → lookup). + - `cli/src/services/telemetry_readiness.rs`: delete `ReadinessState`, `is_ready`, `state`, `state_at`, `is_ready_at`, and their tests, so nothing becomes dead code. `mark_ready`, `clear`, and their helpers stay until T04. + - `cli/src/services/auth_db/multiprocess_tests.rs`: rewrite `doctor_telemetry_health_classifies_real_auth_db_states_without_mutating_them` to vary credential state instead of marker state. Drop the marker assertion in `claude_hook_with_telemetry_disabled_by_config_never_resolves_the_credential`. + - `cli/src/app/doctor_reconciliation_cli_tests.rs` (around lines 640-790): stop writing `FAR_FUTURE_READINESS`. + - Steps: + 1. Delete the readiness probes. + 2. Rename the existing unit tests to marker-free names. For example, `claude_hook_without_readiness_never_looks_up_credentials` becomes a missing-`auth.db` case covered by the T01 mapping, `health_without_readiness_or_with_expired_readiness_never_looks_up_credentials` is removed, and the `*_never_probes_readiness_*` tests assert that neither the lookup nor the `auth_db_present` probe runs when disabled, except where the note needs the probe. + - Dependencies: T01, T02 + - Done when: + - No runtime, doctor, or helper code path reads `telemetry-ready.json`. + - A valid credential without the marker yields `Managed`, `healthy`, and a bearer. + - Marker presence, absence, expiry, or corruption has no effect. + - Disabled policy still short-circuits before any lookup. + - The helper on a machine without `auth.db` fails closed without resolving the repository. + - Verify: + - A new `valid_credential_without_readiness_marker_resolves_managed_telemetry_and_headers` multiprocess test: seed without the marker, then assert hook fallback is none, doctor reports `healthy`, and the helper prints the bearer for the bound repository. + - A new `legacy_readiness_marker_is_ignored` table test with marker cases (absent, valid, expired, malformed) crossed with credential cases (valid, none). It asserts the doctor label matches the credential case only, and that the marker bytes are unchanged. + - The existing 32-process concurrent hook-span test and the 32-process missing-DB helper test pass without seeding the marker. + - `nix develop -c ./scripts/run-cli-cargo.sh test --manifest-path cli/Cargo.toml managed_telemetry`, `... telemetry_headers`, `... doctor`, and `... multiprocess` pass. + - Context synchronization: pending + +- [ ] T04: `Remove readiness marker writes from login and logout and delete the module` (status:todo) + - Task ID: T04 + - Scope: In — the login, logout, and session-replacement lifecycle, plus deleting the marker module and path. Out — any change to provisioning conditions, enrollment, policy gates, or storage APIs. + - Files and functions: + - `cli/src/services/auth_command/mod.rs`: + - `provision_telemetry_credential`: `Issued` iff `issue_and_save_telemetry_credential` is `Ok`, then `enroll_current_repository_if_unbound`; otherwise `Unavailable`. + - Delete `record_telemetry_readiness`. + - `run_logout` and `run_logout_with`: drop the `clear_readiness` parameter and the first step. + - `save_new_session_tokens` and `replace_session_with`: drop the `clear_readiness` parameter; the order becomes invalidate → save. + - Remove `use crate::services::telemetry_readiness;`. + - Delete `cli/src/services/telemetry_readiness.rs`. + - `cli/src/services/mod.rs`: remove `pub mod telemetry_readiness;`. + - `cli/src/services/default_paths.rs`: remove `telemetry_readiness_path`, and update any path-catalog regression test that lists it. + - `cli/src/services/auth_db/multiprocess_tests.rs`: `seed()` stops calling `mark_ready`. + - Steps: + 1. Make the changes above. + 2. Replace `telemetry_readiness_is_recorded_only_after_credential_persistence` with a `provisioning_result_reflects_credential_persistence` test over the `issue_and_save` outcome, if `provision_telemetry_credential` needs a seam. Otherwise rely on the existing provisioning tests. + 3. Update the event-order assertions to `["invalidate", "save"]` and `["delete-workos", "delete-telemetry"]`. + 4. Add `logout_then_lookup_reports_not_configured`: real sandboxed DB, seed, logout path deletes, then a lookup classifies `Missing`. + - Dependencies: T03 + - Done when: + - Login and logout neither write nor remove any readiness file. + - The login result reflects only credential persistence. + - A disabled-telemetry login issues nothing. + - Logout under `auth.mode: disabled` still deletes both rows. + - No reference to `telemetry_readiness`, `telemetry_readiness_path`, `mark_ready`, or `telemetry-ready.json` remains under `cli/`. + - Verify: + - `nix develop -c ./scripts/run-cli-cargo.sh test --manifest-path cli/Cargo.toml auth_command` passes. + - `... logout_then_lookup_reports_not_configured` passes. + - `nix shell nixpkgs#ripgrep -c rg -n "telemetry_readiness|telemetry-ready|mark_ready" cli/` returns nothing. + - `nix flake check` passes. + - Context synchronization: pending + +## Open questions + +- On Linux, the keyring adapter has no non-interactive switch, so a locked Secret Service collection might show an unlock prompt. This limitation already exists. Removing the marker means more hook invocations reach Secret Service: every admitted hook while `auth.db` exists and telemetry is enabled, instead of only marker-ready installs. Is this acceptable for this plan? The smaller and safer alternative is a separate follow-up that adds a Linux non-interactive guard. Folding that into this plan would mean changing the Keychain adapter, which is listed as out of scope. +- `OPT_IN_NOTE` now keys off whether `auth.db` exists, not off the marker, so some users who never provisioned telemetry will also see it: those who logged in while telemetry was disabled, and those with a legacy setup-created `auth.db`. Keep the note as it is, or drop it entirely? Dropping it would also remove the `auth_db_present` probe. diff --git a/context/sce/telemetry-credential.md b/context/sce/telemetry-credential.md index 9981cd3f8..52105fc37 100644 --- a/context/sce/telemetry-credential.md +++ b/context/sce/telemetry-credential.md @@ -15,12 +15,17 @@ The CLI holds one telemetry-only OpenBao credential, issued by the Control Plane ```mermaid flowchart LR Issue[issue_telemetry_credential] --> Save[save: INSERT OR REPLACE row 1] - Save --> Load[load: unexpired valid row only] - Load -->|expired or corrupt| None[None, never mints] + Save --> Load[load: classify row 1] + Load --> Lookup[Missing / Expired / Unusable / Valid, never mints] Logout[sce auth logout] --> Delete[delete row] ``` -- Lookup never mints or refreshes; an expired, empty-field, negative-expiry, or identity-less (pre-`004` legacy) row is returned as `None`, so legacy rows require one `sce auth login`. +- Lookup never mints or refreshes. `load_telemetry_credential` returns a `TelemetryCredentialLookup`: + - `Missing`: no `auth.db`, or no telemetry row. + - `Unusable`: an empty token or accessor, a negative expiry, an identity-less (pre-`004` legacy) row, no WorkOS session, or a session identity that differs from the stored one. Legacy rows therefore require one `sce auth login`. + - `Expired`: an owned, complete row whose expiry is not after now. + - `Valid`: anything else. + - The runtime and doctor map `Missing` to `not_configured`, `Expired` to `credential_expired`, and `Unusable` to `credential_unavailable`. Repository lookups and enrollment accept only `Valid`. - Ownership: every lookup decodes the stored WorkOS session's access-token claims and returns the credential only when `(sub, workspace_id)` equals the identity stored with it. Switching user or workspace therefore makes an older credential unusable even if it was never deleted. A new device-authorization login also deletes the stored telemetry credential before saving the new WorkOS session (`replace_session_with`), so a failed re-issuance leaves no credential; if that delete fails, the new session is not saved. Renewal of the same session keeps the credential. - Repository bindings: `telemetry_repository_bindings` maps a repository ID (the canonical `repository_identity` hash, from `agent_trace.repository_id` or the configured remote) to the `(user_id, workspace_id)` it was enrolled under. Enrollment requires a valid, session-owned credential and records that credential's identity. `sce setup` no longer enrolls repositories, because it must not open the encrypted auth DB or touch the OS credential store. The explicit rebinding mode (`Enroll`) currently has no production caller. A successful `sce auth login` enrolls the working-directory repository only if it is unbound (`EnrollIfUnbound`), so a login never silently moves a repository to another workspace. Bindings survive logout and rotation; they authorize nothing without a matching credential. - Telemetry readiness hint: `cli/src/services/telemetry_readiness.rs` keeps a non-secret file, `/sce/telemetry-ready.json` (`{"expires_at_unix_seconds": N}`), next to `auth.db`. @@ -49,10 +54,10 @@ flowchart LR | Health | Text row | JSON `state` / `reason` | Problem `kind` | `next_action` | | --- | --- | --- | --- | --- | | Telemetry not `Enabled` by policy | `[PASS] Managed telemetry (disabled by config)` | `disabled_by_config` / `disabled_by_config` | none | none | -| No hint or no `auth.db` | `[PASS] Managed telemetry (not configured)` | `not_configured` / `not_configured` | none | none | +| No hint, no `auth.db`, or no telemetry row | `[PASS] Managed telemetry (not configured)` | `not_configured` / `not_configured` | none | none | | Credential valid | `[PASS] Managed telemetry` | `healthy` / `null` | none | none | | Store refuses silent access | `[WARN]` | `disabled` / `credential_inaccessible` | `managed_telemetry_credential_inaccessible` | `auth_login` | -| Hint ready, no usable credential | `[WARN]` | `disabled` / `credential_unavailable` | `managed_telemetry_credential_unavailable` | `auth_login` | +| Hint ready, unusable row (no WorkOS session, identity mismatch, legacy or malformed) | `[WARN]` | `disabled` / `credential_unavailable` | `managed_telemetry_credential_unavailable` | `auth_login` | | Hint or credential expired | `[WARN]` | `disabled` / `credential_expired` | `managed_telemetry_credential_expired` | `auth_login` | | Lookup deadline exceeded | `[WARN]` | `disabled` / `lookup_timeout` | `managed_telemetry_lookup_timeout` | `manual_steps` | | Auth DB open/read failure | `[WARN]` | `disabled` / `database_error` | `managed_telemetry_database_error` | `manual_steps` | From f1f5a06959a9ddec77819de44a512cc2c002c755 Mon Sep 17 00:00:00 2001 From: David Abram Date: Wed, 21 Oct 2026 10:15:47 +0200 Subject: [PATCH 2/4] runtime: Keep telemetry readiness lookups non-migrating Prevent non-interactive telemetry credential lookups from mutating outdated or incomplete auth.db files while preserving migration for interactive paths. Add a current-schema-only encrypted open result and use it to classify outdated databases as unavailable without caching them, with multiprocess coverage for unchanged files and later interactive migration. Plan: `remove-telemetry-readiness-marker`, T02. Co-authored-by: SCE --- .../services/auth_db/multiprocess_tests.rs | 70 +++++++++++++++++++ cli/src/services/db/mod.rs | 67 +++++++++++++++--- .../services/telemetry_credential_storage.rs | 23 +++--- cli/src/services/token_storage.rs | 25 ++++++- context/glossary.md | 4 +- .../remove-telemetry-readiness-marker.md | 18 ++++- context/sce/auth-db.md | 6 +- context/sce/shared-turso-db.md | 2 +- 8 files changed, 186 insertions(+), 29 deletions(-) diff --git a/cli/src/services/auth_db/multiprocess_tests.rs b/cli/src/services/auth_db/multiprocess_tests.rs index 52f0b0b93..4c1358e04 100644 --- a/cli/src/services/auth_db/multiprocess_tests.rs +++ b/cli/src/services/auth_db/multiprocess_tests.rs @@ -46,6 +46,7 @@ const TOKEN_PREFIX: &str = "SCE_AUTH_DB_CHILD_TOKEN="; const HEALTH_PREFIX: &str = "SCE_AUTH_DB_CHILD_TELEMETRY_HEALTH="; const CONCURRENT_PROCESSES: usize = 32; const CONCURRENT_MIGRATORS: usize = 4; +const CONCURRENT_LOOKUPS: usize = 8; const CHILD_KILL_AFTER: Duration = Duration::from_mins(1); const CHILD_POLL_INTERVAL: Duration = Duration::from_millis(10); const FAR_FUTURE: u64 = 4_102_444_800; @@ -510,6 +511,31 @@ fn concurrent_migration_of_schemaless_db_applies_each_migration_once() { sandbox.run("verify-schema"); } +#[test] +fn readiness_lookup_never_migrates_outdated_auth_db() { + let _fan_out = serialize_fan_out(); + let sandbox = Sandbox::new(); + sandbox.run("create-outdated"); + let auth_db = sandbox.sce_state_dir().join("auth.db"); + let before = std::fs::read(&auth_db).expect("outdated auth DB"); + + let lookups = (0..CONCURRENT_LOOKUPS) + .map(|_| { + sandbox + .command("lookup-outdated") + .spawn() + .expect("spawn lookup") + }) + .collect::>(); + for child in lookups { + wait_success("lookup-outdated", child); + } + + assert_eq!(std::fs::read(&auth_db).expect("auth DB"), before); + sandbox.run("verify-outdated"); + sandbox.run("lookup-then-migrate"); +} + #[test] fn outdated_schema_is_migrated_and_partial_schema_fails_clearly() { let _fan_out = serialize_fan_out(); @@ -1084,6 +1110,9 @@ async fn auth_db_child_process() { assert!(path.parent().expect("auth DB parent").is_dir()); assert_eq!(std::fs::read(&path).ok(), before); } + "create-outdated" | "lookup-outdated" | "verify-outdated" | "lookup-then-migrate" => { + run_outdated_schema_child(&role).await; + } "create-schemaless" => { let db = EncryptedTursoDb::::new() .await @@ -1295,6 +1324,47 @@ async fn applied_migrations(db: &EncryptedTursoDb) -> Vec .expect("migration ids") } +async fn run_outdated_schema_child(role: &str) { + match role { + "create-outdated" | "verify-outdated" => { + let db = EncryptedTursoDb::::new() + .await + .expect("pre-binding schema opens"); + assert_eq!(applied_migrations(&db).await.len(), PRE_BINDING_MIGRATIONS); + } + "lookup-outdated" => assert_outdated_lookups_are_unusable().await, + "lookup-then-migrate" => { + assert_outdated_lookups_are_unusable().await; + let db = crate::services::token_storage::open_or_create_auth_db() + .await + .expect("interactive open migrates outdated auth DB"); + assert_eq!(applied_migrations(db).await.len(), expected_migrations()); + assert_eq!( + load_telemetry_credential() + .await + .expect("lookup after migration"), + TelemetryCredentialLookup::Missing + ); + } + other => panic!("unknown outdated-schema child role: {other}"), + } +} + +async fn assert_outdated_lookups_are_unusable() { + assert_eq!( + load_telemetry_credential() + .await + .expect("outdated auth DB lookup is not an error"), + TelemetryCredentialLookup::Unusable + ); + assert_eq!( + load_repository_telemetry_credential(HEADERS_REPOSITORY.to_string()) + .await + .expect("outdated auth DB repository lookup is not an error"), + None + ); +} + async fn upgrade_outdated_schema() { let old = EncryptedTursoDb::::new() .await diff --git a/cli/src/services/db/mod.rs b/cli/src/services/db/mod.rs index 67941f18c..b09da5d47 100644 --- a/cli/src/services/db/mod.rs +++ b/cli/src/services/db/mod.rs @@ -936,6 +936,12 @@ pub struct EncryptedTursoDb { _open_lock: fs::File, } +pub enum ExistingDbOpen { + Missing, + SchemaNotCurrent, + Open(T), +} + impl TursoDb { /// Open or create the database at the spec-provided canonical path. /// @@ -1431,10 +1437,58 @@ impl EncryptedTursoDb { .map(Some) } + pub async fn open_existing_current_at( + db_path: impl AsRef, + access: encryption_key::CredentialAccess, + ) -> Result> { + let db_name = M::db_name(); + let db_path = db_path.as_ref(); + let Some(open_lock) = open_lock::open_lock_file(db_path, false)? else { + return Ok(ExistingDbOpen::Missing); + }; + let encryption_key = resolve_encryption_key(db_path, db_name, access).await?; + let db = Box::pin(Self::connect_with_retry( + db_path, + &encryption_key, + open_lock, + )) + .await?; + if db + .core + .has_pending_migrations() + .await + .with_context(|| format!("failed to check {db_name} schema readiness"))? + { + return Ok(ExistingDbOpen::SchemaNotCurrent); + } + Ok(ExistingDbOpen::Open(db)) + } + async fn open_locked( db_path: PathBuf, encryption_key: String, open_lock: fs::File, + ) -> Result { + let db_name = M::db_name(); + let mut db = Box::pin(Self::connect_with_retry( + &db_path, + &encryption_key, + open_lock, + )) + .await?; + + db.core + .ensure_migrations_current() + .await + .with_context(|| format!("failed to run {db_name} migrations"))?; + + Ok(db) + } + + async fn connect_with_retry( + db_path: &Path, + encryption_key: &str, + open_lock: fs::File, ) -> Result { let db_name = M::db_name(); let retry_policy = resolve_connection_open_retry_policy::(); @@ -1449,7 +1503,7 @@ impl EncryptedTursoDb { CONNECTION_OPEN_RETRY_HINT, |_| async { let attempt = - Self::connect_locked(&db_path, &encryption_key, &open_lock, busy_timeout).await; + Self::connect_locked(db_path, encryption_key, &open_lock, busy_timeout).await; last_attempt_locked.store( attempt.as_ref().is_err_and(is_database_lock_conflict), Ordering::Relaxed, @@ -1466,17 +1520,10 @@ impl EncryptedTursoDb { } })?; - let mut db = Self { + Ok(Self { core: TursoConnectionCore::new(conn), _open_lock: open_lock, - }; - - db.core - .ensure_migrations_current() - .await - .with_context(|| format!("failed to run {db_name} migrations"))?; - - Ok(db) + }) } async fn connect_locked( diff --git a/cli/src/services/telemetry_credential_storage.rs b/cli/src/services/telemetry_credential_storage.rs index 2392f4f00..a26069acf 100644 --- a/cli/src/services/telemetry_credential_storage.rs +++ b/cli/src/services/telemetry_credential_storage.rs @@ -6,9 +6,9 @@ use base64::Engine; use serde::Deserialize; use crate::services::db::encryption_key::CredentialAccess; -use crate::services::db::{DbSpec, EncryptedTursoDb}; +use crate::services::db::{DbSpec, EncryptedTursoDb, ExistingDbOpen}; use crate::services::token_storage::{ - open_existing_auth_db, open_or_create_auth_db, TokenStorageError, + open_existing_auth_db, open_existing_current_auth_db, open_or_create_auth_db, TokenStorageError, }; const TELEMETRY_CREDENTIAL_ROW_ID: i64 = 1; @@ -137,19 +137,22 @@ pub async fn save_telemetry_credential( } pub async fn load_telemetry_credential() -> Result { - let Some(db) = open_existing_auth_db(CredentialAccess::NonInteractive).await? else { - return Ok(TelemetryCredentialLookup::Missing); - }; - load_with(db, current_unix_timestamp_seconds()?).await + match open_existing_current_auth_db(CredentialAccess::NonInteractive).await? { + ExistingDbOpen::Missing => Ok(TelemetryCredentialLookup::Missing), + ExistingDbOpen::SchemaNotCurrent => Ok(TelemetryCredentialLookup::Unusable), + ExistingDbOpen::Open(db) => load_with(db, current_unix_timestamp_seconds()?).await, + } } pub async fn load_repository_telemetry_credential( repository_id: String, ) -> Result, TokenStorageError> { - let Some(db) = open_existing_auth_db(CredentialAccess::NonInteractive).await? else { - return Ok(None); - }; - load_for_repository_with(db, current_unix_timestamp_seconds()?, &repository_id).await + match open_existing_current_auth_db(CredentialAccess::NonInteractive).await? { + ExistingDbOpen::Missing | ExistingDbOpen::SchemaNotCurrent => Ok(None), + ExistingDbOpen::Open(db) => { + load_for_repository_with(db, current_unix_timestamp_seconds()?, &repository_id).await + } + } } pub async fn bind_repository_telemetry( diff --git a/cli/src/services/token_storage.rs b/cli/src/services/token_storage.rs index afedb8803..ffd771d9f 100644 --- a/cli/src/services/token_storage.rs +++ b/cli/src/services/token_storage.rs @@ -9,7 +9,7 @@ use crate::services::auth_db::AuthDb; use crate::services::db::encryption_key::{ CredentialAccess, CredentialAccessDenied, CredentialAccessUnavailable, }; -use crate::services::db::DatabaseLocked; +use crate::services::db::{DatabaseLocked, ExistingDbOpen}; use crate::services::default_paths::auth_db_path; use crate::services::error::AuthStorageFailure; @@ -51,6 +51,29 @@ pub(crate) async fn open_existing_auth_db( Ok(AUTH_DB.get()) } +pub(crate) async fn open_existing_current_auth_db( + access: CredentialAccess, +) -> Result, TokenStorageError> { + if let Some(db) = AUTH_DB.get() { + return Ok(ExistingDbOpen::Open(db)); + } + let db_path = + auth_db_path().map_err(|error| TokenStorageError::PathResolution(error.to_string()))?; + match AuthDb::open_existing_current_at(db_path, access) + .await + .map_err(|error| open_error(&error))? + { + ExistingDbOpen::Missing => Ok(ExistingDbOpen::Missing), + ExistingDbOpen::SchemaNotCurrent => Ok(ExistingDbOpen::SchemaNotCurrent), + ExistingDbOpen::Open(db) => { + let _ = AUTH_DB.set(db); + Ok(AUTH_DB + .get() + .map_or(ExistingDbOpen::Missing, ExistingDbOpen::Open)) + } + } +} + #[derive(Clone, Debug, Eq, PartialEq, Serialize, Deserialize)] pub struct StoredTokens { pub access_token: String, diff --git a/context/glossary.md b/context/glossary.md index b5d472e1b..dc52f3b97 100644 --- a/context/glossary.md +++ b/context/glossary.md @@ -77,7 +77,7 @@ - `progress reporter contract`: Library-independent, consumer-typed sync seam in `cli/src/services/sync/progress.rs` where `ProgressReporter` carries an arbitrary consumer event type, supports explicit successful finalization and closure-based collectors, and provides `NoopProgressReporter` for callers without human progress output. The sync-owned `SyncProgressEvent` in `cli/src/services/sync/sync.rs` carries lifecycle, accepted-batch, and stream-completion payloads for the three concurrent remote Agent Trace streams (`messages`, `parts`, `agent_traces`); `services::sync::progress` also owns the fixed `indicatif` terminal adapter and focused contract tests, while `sync/command.rs` selects text versus JSON behavior. No top-level `services::progress` module exists. - `remote sync streams`: The three Agent Trace streams `sce sync` uploads to the control plane — `messages`, `parts`, `agent_traces` — as distinct from the four local capture streams, which also include the local-only `diff_traces`. `sce sync --format json` reports only these three streams. See `context/cli/agent-trace-sync-command.md`. - `local Turso adapter`: Module in `cli/src/services/local_db/mod.rs` that defines `LocalDbSpec` and exposes `LocalDb` as a `TursoDb` alias. It resolves the canonical local DB path with `local_db_path()`, currently declares zero migrations, and inherits retry-backed `new()`, `execute()`, `query()`, and `query_map()` behavior from the shared generic adapter. -- `encrypted Turso adapter`: Generic native async adapter seam in `cli/src/services/db/mod.rs` exposed as `EncryptedTursoDb`, structurally parallel to `TursoDb` and owning no Tokio runtime. Its constructor resolves the encryption key via `encryption_key::get_or_create_encryption_key(&db_path, db_name)`, which derives a Turso-compatible 64-character hex key from non-empty `SCE_AUTH_DB_ENCRYPTION_KEY` env-secret text before falling back to OS credential-store keyring get-or-create behavior; credential-store default registration is guarded by stable `OnceLock` plus an atomic in-progress flag so errors or panics leave initialization retryable without mutex poisoning. The adapter enables Turso local encryption with strict `aegis256` cipher selection through `turso::EncryptionOpts`, wraps encrypted local open/connect in the default DB connection-open retry policy, and runs embedded migrations after retry has produced a connection; execute, query, query_map, and migration operations are async and preserve `__sce_migrations` tracking parity. +- `encrypted Turso adapter`: Generic native async adapter seam in `cli/src/services/db/mod.rs` exposed as `EncryptedTursoDb`, structurally parallel to `TursoDb` and owning no Tokio runtime. Its constructor resolves the encryption key via `encryption_key::get_or_create_encryption_key(&db_path, db_name)`, which derives a Turso-compatible 64-character hex key from non-empty `SCE_AUTH_DB_ENCRYPTION_KEY` env-secret text before falling back to OS credential-store keyring get-or-create behavior; credential-store default registration is guarded by stable `OnceLock` plus an atomic in-progress flag so errors or panics leave initialization retryable without mutex poisoning. The adapter enables Turso local encryption with strict `aegis256` cipher selection through `turso::EncryptionOpts`, wraps encrypted local open/connect in the default DB connection-open retry policy, and runs embedded migrations after retry has produced a connection; execute, query, query_map, and migration operations are async and preserve `__sce_migrations` tracking parity. `open_existing_current_at` is its non-migrating existing-DB open, returning `ExistingDbOpen::{Missing, SchemaNotCurrent, Open}` and used only by telemetry readiness lookups. - `auth DB adapter`: Module in `cli/src/services/auth_db/mod.rs` that defines `AuthDbSpec` and exposes `AuthDb` as an `EncryptedTursoDb` alias. It resolves the canonical `/sce/auth.db` path with `auth_db_path()`, keeps encryption mandatory with `SCE_AUTH_DB_ENCRYPTION_KEY` env-secret precedence before OS keyring fallback and no plaintext mode, and embeds ordered auth migrations where baseline SQL creates `auth_credentials` without `user_id`, with `updated_at`, and a trigger that auto-refreshes `updated_at` on row updates. Auth runtime token-storage is now wired through `cli/src/services/token_storage.rs`, which persists tokens via the `auth_credentials` table in the encrypted auth DB instead of a JSON file. - `telemetry credential`: Telemetry-only OpenBao token and accessor issued by the Control Plane `POST /auth/telemetry/credential`, stored as one fixed row in the encrypted auth DB `telemetry_credentials` table by `cli/src/services/telemetry_credential_storage.rs`, returned only while unexpired, removed by `sce auth logout`, not revoked server-side by local deletion, and used as the sole bearer secret for managed OTLP export (never the accessor); see `context/sce/telemetry-credential.md`. - `sce.hook span`: The single root-of-process managed trace span a Claude-invoked SCE hook exports in place of `sce.command`, carrying `sce.agent=claude`, the hook event, tool name, outcome, and the Claude `session_id`/`tool_use_id` as correlation-only attributes. It is parented on a valid Claude-provided `TRACEPARENT` (the `claude_code.interaction` span, so it sits beside `claude_code.tool`) and is otherwise an independent root. Its only exported children are `sce.mutation_scope` (one per adapter seam call) and, beneath it, `sce.git_snapshot` and `sce.persistence`, each emitted only when that operation actually runs and carrying only `sce.outcome`. Admission is Claude-only (Claude-only hook subcommands by argv; shared hook subcommands and `sce policy bash` only with `CLAUDECODE` and `CLAUDE_PROJECT_DIR` present); see `context/sce/cli-observability-contract.md`. @@ -97,7 +97,7 @@ - `bash policy satisfied_by`: Optional field on a custom `policies.bash` entry listing wrapper argv prefixes that already satisfy the policy. When the matched command was unwrapped from one of these wrappers (outermost first, tracked by `NormalizedSegment.wrappers` in `cli/src/services/bash_policy.rs`), the policy does not fire, so a policy steering `rg` toward nix stays quiet for `nix shell nixpkgs#ripgrep -c rg ...` while still blocking a bare `rg`. Custom-policy-only; presets cannot declare satisfying wrappers. Exact argv-prefix matching only. See `context/sce/bash-tool-policy-enforcement-contract.md`. - `DbSpec`: Service-specific database metadata trait in `cli/src/services/db/mod.rs` that supplies a diagnostic database name, canonical path resolver, ordered embedded migration list, and config-file lookup key (`db_config_key()`) for `TursoDb`. - `TursoDb`: Generic unencrypted Turso adapter in `cli/src/services/db/mod.rs`; owns the concrete connection and service marker but no Tokio runtime. Its construction, open, migration, execute/query/materialization, checkpoint, readiness, and transaction APIs are native async operations awaited by connected callers; retry and migration policies remain adapter-owned. -- `TursoConnectionCore`: Removed with the async persistence migration. `TursoDb` and `EncryptedTursoDb` now share async helper functions without a runtime-owning core or synchronous executor bridge. +- `TursoConnectionCore`: Private connection-plus-spec-marker core in `cli/src/services/db/mod.rs` shared by `TursoDb` and `EncryptedTursoDb`; it owns the migration readiness check (`has_pending_migrations`) and serialized migration (`ensure_migrations_current`) but no Tokio runtime or synchronous executor bridge. - `no-migration DB open path`: `TursoDb::open_without_migrations()` / `TursoDb::open_without_migrations_at(path)` plus Agent Trace adapter-specific no-migration seams; opens/connects a local Turso database with parent-directory creation and configured connection-open retry but does not create `__sce_migrations` or run embedded schema migrations. Active Agent Trace hook callers first try the repository-scoped no-migration path and then fall back to migration-running initialization when readiness or repository metadata validation fails. - `TursoDb migration readiness check`: Public methods on `TursoDb` in `cli/src/services/db/mod.rs` for non-mutating schema-readiness verification: `migration_metadata_problems(&self) -> Result>` queries `__sce_migrations` metadata and compares applied IDs against `M::migrations()`, returning problems (missing table, incomplete migrations, unexpected migrations) or an empty list when ready; `ensure_schema_ready(&self, setup_guidance: &str) -> Result<()>` calls `migration_metadata_problems()` and bails with a formatted error including `M::db_name()` and the caller-provided guidance string when problems are found. `RepositoryAgentTraceDb::ensure_schema_ready_for_hooks()` delegates to `TursoDb::ensure_schema_ready()` with the Agent Trace–specific `AGENT_TRACE_SCHEMA_SETUP_GUIDANCE` constant. - `database_retry config namespace`: Nested config namespace under `policies.database_retry` in `sce/config.json`, authored in `config/pkl/base/sce-config-schema.pkl` and parsed/resolved in `cli/src/services/config/`. Supports per-database overrides (`local_db`, `agent_trace_db`, `auth_db`) each with optional `connection_open` and `query` objects containing `max_attempts`, `timeout_ms`, `initial_backoff_ms`, `max_backoff_ms`; `agent_trace_db` alone also accepts `busy_timeout_ms` and `contention_deadline_ms`. Validated against JSON Schema at config load and surfaced in `sce config show`/`validate`. Parsed into `DatabaseRetryConfig` (`local_db`/`auth_db` as `Option`, `agent_trace_db` as `Option` wrapping a `PerDbRetryConfig` plus the two Agent Trace keys), initialized at app startup via the `DATABASE_RETRY_CONFIG` `OnceLock`, and consumed by config-aware retry and busy-timeout resolution in DB adapters with fallback to hardcoded defaults. diff --git a/context/plans/remove-telemetry-readiness-marker.md b/context/plans/remove-telemetry-readiness-marker.md index 3c6392443..fad67e47a 100644 --- a/context/plans/remove-telemetry-readiness-marker.md +++ b/context/plans/remove-telemetry-readiness-marker.md @@ -260,7 +260,7 @@ The header helper follows the same policy gate and destination tripwire. It then - Context impact: localized. When the marker is ready, an expired stored row now reports `credential_expired`, and a missing telemetry row now reports `not_configured`; both previously reported `credential_unavailable`. All other reasons are unchanged. The marker-free current-state context is rewritten when T03 removes the gate. - Context synchronization: synced -- [ ] T02: `Open auth.db for telemetry lookups without running migrations` (status:todo) +- [x] T02: `Open auth.db for telemetry lookups without running migrations` (status:done) - Task ID: T02 - Scope: In — a non-migrating encrypted existing-DB open used only by the two non-interactive telemetry lookups. Out — `load_tokens`, `delete_*`, `bind_repository_telemetry`, `sce sync`, and every interactive path, which all keep migrating. - Files and functions: @@ -280,7 +280,21 @@ The header helper follows the same policy gate and destination tripwire. It then - A new `readiness_lookup_never_migrates_outdated_auth_db` multiprocess test in `auth_db/multiprocess_tests.rs`: seed a DB at an older migration, run 8 concurrent `load_telemetry_credential` children, then assert the file and `__sce_migrations` are unchanged and every child reports the unusable classification. - Extend the existing `current_schema_reader_succeeds_while_writer_holds_begin_immediate`-style test, or keep it unchanged if it already covers the current-schema path. - `nix develop -c ./scripts/run-cli-cargo.sh test --manifest-path cli/Cargo.toml multiprocess` passes. - - Context synchronization: pending + - Completed: 2026-10-21 + - Files changed: + - `cli/src/services/db/mod.rs` + - `cli/src/services/token_storage.rs` + - `cli/src/services/telemetry_credential_storage.rs` + - `cli/src/services/auth_db/multiprocess_tests.rs` + - Result: Added `ExistingDbOpen { Missing, SchemaNotCurrent, Open(T) }` and `EncryptedTursoDb::open_existing_current_at`. That open shares a new `connect_with_retry` step with `open_locked`. It keeps `open_lock_file(.., false)` before key resolution, but replaces `ensure_migrations_current` with the read-only `has_pending_migrations`. On pending migrations it drops the connection and returns `SchemaNotCurrent`. `token_storage::open_existing_current_auth_db` reuses a cached `AUTH_DB` and caches only an `Open` handle. Error typing is unchanged through `open_error`. `load_telemetry_credential` maps `SchemaNotCurrent` to `Unusable`, and `load_repository_telemetry_credential` maps it to `None`. `load_tokens`, `save_tokens`, `bind_repository_telemetry`, `delete_*`, and interactive opens still go through the migrating paths. + - Verify: + - New `readiness_lookup_never_migrates_outdated_auth_db`: a pre-binding (3-migration) `auth.db` and 8 concurrent `lookup-outdated` children, each asserting `Unusable` and a repository lookup of `None`. Afterwards the `auth.db` bytes are unchanged and `__sce_migrations` still has 3 rows. A `lookup-then-migrate` child gets `Unusable` and then migrates through `open_or_create_auth_db` to the full migration set in the same process: passed. + - `current_schema_reader_performs_no_migration_write_under_held_write_lock` already covers the current-schema path under a held `BEGIN IMMEDIATE`; kept unchanged: passed. + - `nix develop -c ./scripts/run-cli-cargo.sh test --manifest-path cli/Cargo.toml multiprocess`: 21 passed. + - The additional `telemetry_credential` (27), `managed_telemetry` (42), and `stalled` (10) filters passed, and `clippy --all-targets -D warnings` is clean. + - Deviation: the new child roles are dispatched through a `run_outdated_schema_child` helper, so `auth_db_child_process` stays within clippy's `too_many_lines` limit. + - Context impact: localized. Telemetry readiness lookups no longer migrate an outdated or schemaless `auth.db`. They report `credential_unavailable` (`Unusable`) instead, and a later interactive open still migrates. This affects `context/sce/auth-db.md` (concurrency and schema readiness), and `shared-turso-db.md`/`glossary.md` (the new non-migrating encrypted existing-DB open). + - Context synchronization: synced - [ ] T03: `Gate runtime, doctor and header helper on auth.db instead of the marker` (status:todo) - Task ID: T03 diff --git a/context/sce/auth-db.md b/context/sce/auth-db.md index 796563303..5ca396b98 100644 --- a/context/sce/auth-db.md +++ b/context/sce/auth-db.md @@ -52,9 +52,9 @@ Storage operations live in `cli/src/services/telemetry_credential_storage.rs`: ` - Many short-lived `sce` processes read `auth.db` at once: every admitted Claude hook resolves the telemetry credential at startup. `EncryptedTursoDb::new()` therefore opens with Turso multiprocess WAL, alongside encryption, so concurrent readers share the file. Multiprocess WAL creates `auth.db-wal` and the `auth.db-tshm` coordination file next to the database. SCE also creates `auth.db-open.lock` once `auth.db` exists, to serialize Turso cold opens (see [Serialized cold opens](#serialized-cold-opens-after-a-legacy-write)). - A current schema is opened read-only: no `CREATE TABLE IF NOT EXISTS __sce_migrations` or other DDL runs when every embedded migration ID is already recorded. A missing or outdated DB is migrated in one `BEGIN IMMEDIATE` transaction, with a retry only for genuine `Busy`. A partially migrated schema fails with the failing migration ID rather than being treated as current. Details: [shared-turso-db.md](shared-turso-db.md). -- Telemetry credential lookups are existing-DB read paths. Both `load_telemetry_credential` (the managed-telemetry lookup used by every command and admitted hook) and `load_repository_telemetry_credential` (the `sce auth telemetry-headers` lookup that Claude Code's `otelHeadersHelper` runs) go through `token_storage::get_existing_auth_db`. That helper reuses the process `AuthDb` singleton when it is already open. Otherwise it calls `EncryptedTursoDb::open_existing_at`, which opens the database file without `O_CREAT` before resolving the encryption key. An absent file returns no credential, with no DB file, `-wal`/`-tshm` file, encryption key, or migration created. The existence check is the `open()` itself, not a separate `Path::exists()`. Path-resolution errors stay `TokenStorageError::PathResolution`. Only `sce auth`, `sce setup`, and other writers create the DB. This also keeps concurrent hooks and header-helper calls on a never-logged-in machine from racing to create it. +- Telemetry credential lookups are existing-DB read paths. Both `load_telemetry_credential` (the managed-telemetry lookup used by every command and admitted hook) and `load_repository_telemetry_credential` (the `sce auth telemetry-headers` lookup that Claude Code's `otelHeadersHelper` runs) go through `token_storage::open_existing_current_auth_db`. That helper reuses the process `AuthDb` singleton when it is already open. Otherwise it calls `EncryptedTursoDb::open_existing_current_at`, which opens the database file without `O_CREAT` before resolving the encryption key. An absent file returns no credential, with no DB file, `-wal`/`-tshm` file, encryption key, or migration created. These lookups never migrate: an outdated, schemaless, or partially migrated `auth.db` is reported as `SchemaNotCurrent`, the connection is dropped and not cached, and the lookup classifies `Unusable` (repository lookup: none). A later interactive open in the same process (for example `sce auth login`) still migrates it. The existence check is the `open()` itself, not a separate `Path::exists()`. Path-resolution errors stay `TokenStorageError::PathResolution`. Only `sce auth`, `sce setup`, and other writers create the DB. This also keeps concurrent hooks and header-helper calls on a never-logged-in machine from racing to create it. - The auth DB busy timeout is 1000 ms. Hook reads stay bounded by `CREDENTIAL_LOOKUP_DEADLINE` (2s), which aborts the lookup regardless of busy waits or retries. -- Regression coverage (`cli/src/services/auth_db/multiprocess_tests.rs`, child processes of the test binary with sandboxed `HOME`/`XDG_*` and an env-provided `SCE_AUTH_DB_ENCRYPTION_KEY`; never the real DB or keyring): a reader succeeds while another process holds the DB open; 32 reader processes × 3 rounds all resolve the credential; a current-schema reader succeeds while another process holds `BEGIN IMMEDIATE`, which proves it performs no migration write; both telemetry lookups on a missing DB return none and create nothing (the repository lookup runs with an empty `SCE_AUTH_DB_ENCRYPTION_KEY`, which fails any key resolution, so it also proves no key is resolved or created); 32 concurrent `sce auth telemetry-headers` runs (through `app::run`, inside a Git repository) on a missing DB all fail closed with `Telemetry credential is unavailable.` and leave no `auth.db*` file; the helper prints the bearer only for the bound repository; 4 processes concurrently migrating an existing schemaless DB record each migration once and pass `PRAGMA integrity_check`; an outdated schema upgrades and a partial one fails clearly and rolls back; 32 concurrent hook processes export correlated `sce.hook` spans. The mixed-version tests are listed under [Mixed-version compatibility lock](#mixed-version-compatibility-lock). +- Regression coverage (`cli/src/services/auth_db/multiprocess_tests.rs`, child processes of the test binary with sandboxed `HOME`/`XDG_*` and an env-provided `SCE_AUTH_DB_ENCRYPTION_KEY`; never the real DB or keyring): a reader succeeds while another process holds the DB open; 32 reader processes × 3 rounds all resolve the credential; a current-schema reader succeeds while another process holds `BEGIN IMMEDIATE`, which proves it performs no migration write; 8 concurrent telemetry lookups against a pre-binding (outdated) DB all classify `Unusable` and leave `auth.db` byte-identical with `__sce_migrations` unchanged, after which a lookup-then-interactive-open in one process migrates it; both telemetry lookups on a missing DB return none and create nothing (the repository lookup runs with an empty `SCE_AUTH_DB_ENCRYPTION_KEY`, which fails any key resolution, so it also proves no key is resolved or created); 32 concurrent `sce auth telemetry-headers` runs (through `app::run`, inside a Git repository) on a missing DB all fail closed with `Telemetry credential is unavailable.` and leave no `auth.db*` file; the helper prints the bearer only for the bound repository; 4 processes concurrently migrating an existing schemaless DB record each migration once and pass `PRAGMA integrity_check`; an outdated schema upgrades and a partial one fails clearly and rolls back; 32 concurrent hook processes export correlated `sce.hook` spans. The mixed-version tests are listed under [Mixed-version compatibility lock](#mixed-version-compatibility-lock). ### Upstream Turso multiprocess race @@ -130,7 +130,7 @@ Remaining limits: a mixed old/new deployment is still not usable at the same tim ## Token storage integration - `cli/src/services/token_storage.rs` uses `AuthDb` through async `save_tokens`, `load_tokens`, and `delete_tokens` operations and a `tokio::sync::OnceCell`. Only a successful open is cached, so a failed non-interactive open never blocks a later interactive one in the same process. -- Open paths: `open_or_create_auth_db()` (`AuthDb::new()`, interactive key access) is used only to persist a new session or telemetry credential during `sce auth login`. Every other path uses `open_existing_auth_db(access)` (`EncryptedTursoDb::open_existing_at(path, access)`), which returns `None` for a missing `auth.db` and never creates the file or a key. `load_tokens(access)` and `save_tokens(token, access)` take the caller's `CredentialAccess`; `delete_tokens` is interactive and returns `false` for a missing DB. +- Open paths: `open_or_create_auth_db()` (`AuthDb::new()`, interactive key access) is used only to persist a new session or telemetry credential during `sce auth login`. Every other path uses `open_existing_auth_db(access)` (`EncryptedTursoDb::open_existing_at(path, access)`), which returns `None` for a missing `auth.db`, never creates the file or a key, and migrates an existing outdated DB. The two telemetry readiness lookups instead use `open_existing_current_auth_db(access)`, which returns `ExistingDbOpen::{Missing, SchemaNotCurrent, Open}` and caches only a current-schema handle. `load_tokens(access)` and `save_tokens(token, access)` take the caller's `CredentialAccess`; `delete_tokens` is interactive and returns `false` for a missing DB. - Auth commands and the generic async control-plane credential store directly await token persistence on the application runtime. Database operations do not cross a `spawn_blocking` boundary; the OS keyring adapter retains its platform-specific synchronous behavior where required. - `token_file_path()` returns the auth DB path from `auth_db_path()` instead of a JSON file path. - `TokenStorageError` exposes `PathResolution`, `Database`, `DatabaseLocked`, and `CredentialAccessUnavailable` variants. `CredentialAccessUnavailable` is mapped from the typed `encryption_key::CredentialAccessUnavailable` (non-interactive) or `encryption_key::CredentialAccessDenied` (interactive) errors and carries no secret. `DatabaseLocked` is mapped from the typed `db::DatabaseLocked` error. `TokenStorageError::failure()` returns the `AuthStorageFailure` cause used for user guidance. Former `Io`, `Serialization`, `CorruptedTokenFile`, and `Permission` variants have been removed. diff --git a/context/sce/shared-turso-db.md b/context/sce/shared-turso-db.md index b664a4142..43f1790bc 100644 --- a/context/sce/shared-turso-db.md +++ b/context/sce/shared-turso-db.md @@ -29,7 +29,7 @@ - `count_write_contention(body) -> (T, WriteContentionCounts)` (`#[cfg(test)]`, `pub(crate)`): runs `body` and reports the write-contention `attempts`, `outer_retries`, and `exhaustions` it caused on the current thread. Test instrumentation only; not production telemetry and not shared across threads or processes. - `record_write_contention_timeline(body) -> (T, Vec<(Instant, WriteContentionTimelineEvent)>)` (`#[cfg(test)]`, `pub(crate)`): runs `body` and returns the `Instant`-stamped attempt start/end, backoff requested and backoff slept events of the write-contention retry on the current thread. Test-only measurement instrumentation used by the lock-contention suite; never logged in production. - `count_read_statements(body) -> (T, usize)` (`#[cfg(test)]`, `pub(crate)`): runs `body` and reports how many `TursoDb` read statements (`query_map`) it issued on the current thread, each counted once in its synchronous prelude before the retry wrapper. A deterministic seam for tests that must prove an operation reads from a single DB snapshot — one statement — rather than several independent `SELECT`s a concurrent commit could tear across (first used by `MutationTraceStore::load_tree_roots` / `load_all_tree_roots`). Not compiled into production builds. -- `EncryptedTursoDb`: encrypted-adapter seam parallel to `TursoDb` with the same static shape (connection and spec marker). `EncryptedTursoDb::new()` resolves the encryption key via `encryption_key::get_or_create_encryption_key()` (environment variable `SCE_AUTH_DB_ENCRYPTION_KEY` with OS credential-store fallback), enables Turso experimental local encryption, applies strict `aegis256` cipher selection through `turso::EncryptionOpts` during local DB open/connect, opens with `experimental_multiprocess_wal(true)` like `TursoDb` (without it Turso takes an exclusive, non-blocking lock on the whole file for the process lifetime, so a concurrent `sce` process fails with `LockingError`), installs the auth DB busy timeout, and wraps that open/connect block in the same connection-open retry policy resolved from `policies.database_retry..connection_open`. Inside that retry loop, before Turso opens the file, it takes the shared mixed-version compatibility lock from `db/open_lock.rs` (an OFD `F_RDLCK` on Linux, classic `F_SETLK` `F_RDLCK` elsewhere on Unix) on a descriptor the `EncryptedTursoDb` owns for its lifetime. That lock excludes pre-multiprocess SCE binaries, which hold a classic `F_WRLCK` on the whole file ([auth-db.md](auth-db.md#mixed-version-compatibility-lock)). `EncryptedTursoDb::open_existing_at(path)` is the existing-only variant: it opens the file without `O_CREAT` and returns `None` for an absent file before any encryption-key resolution, file creation, or migration. After connect it migrates only when needed: a read-only readiness check (`__sce_migrations` exists and records every embedded ID) lets a current schema open with no write at all; otherwise all pending migrations run inside one `BEGIN IMMEDIATE` transaction, which serializes concurrent initializers (a waiter re-reads the metadata under the lock and applies nothing) and rolls back the whole attempt on any migration failure. Only `Busy`/`BusySnapshot` on begin or commit is retried, under `MIGRATION_RETRY_POLICY` (10 attempts, 25..200ms backoff, each attempt bounded by the 1000 ms busy timeout); a deterministic migration failure (for example a column added by a partially applied `004` whose ID was never recorded) fails after one attempt with the migration ID in the error chain. After migrating, the readiness check runs again and a schema that is still behind fails the open. +- `EncryptedTursoDb`: encrypted-adapter seam parallel to `TursoDb` with the same static shape (connection and spec marker). `EncryptedTursoDb::new()` resolves the encryption key via `encryption_key::get_or_create_encryption_key()` (environment variable `SCE_AUTH_DB_ENCRYPTION_KEY` with OS credential-store fallback), enables Turso experimental local encryption, applies strict `aegis256` cipher selection through `turso::EncryptionOpts` during local DB open/connect, opens with `experimental_multiprocess_wal(true)` like `TursoDb` (without it Turso takes an exclusive, non-blocking lock on the whole file for the process lifetime, so a concurrent `sce` process fails with `LockingError`), installs the auth DB busy timeout, and wraps that open/connect block in the same connection-open retry policy resolved from `policies.database_retry..connection_open`. Inside that retry loop, before Turso opens the file, it takes the shared mixed-version compatibility lock from `db/open_lock.rs` (an OFD `F_RDLCK` on Linux, classic `F_SETLK` `F_RDLCK` elsewhere on Unix) on a descriptor the `EncryptedTursoDb` owns for its lifetime. That lock excludes pre-multiprocess SCE binaries, which hold a classic `F_WRLCK` on the whole file ([auth-db.md](auth-db.md#mixed-version-compatibility-lock)). `EncryptedTursoDb::open_existing_at(path)` is the existing-only variant: it opens the file without `O_CREAT` and returns `None` for an absent file before any encryption-key resolution, file creation, or migration. After connect it migrates only when needed: a read-only readiness check (`__sce_migrations` exists and records every embedded ID) lets a current schema open with no write at all; otherwise all pending migrations run inside one `BEGIN IMMEDIATE` transaction, which serializes concurrent initializers (a waiter re-reads the metadata under the lock and applies nothing) and rolls back the whole attempt on any migration failure. Only `Busy`/`BusySnapshot` on begin or commit is retried, under `MIGRATION_RETRY_POLICY` (10 attempts, 25..200ms backoff, each attempt bounded by the 1000 ms busy timeout); a deterministic migration failure (for example a column added by a partially applied `004` whose ID was never recorded) fails after one attempt with the migration ID in the error chain. After migrating, the readiness check runs again and a schema that is still behind fails the open. `EncryptedTursoDb::open_existing_current_at(path, access)` is the non-migrating existing-only variant: it shares the same lock, key-resolution, and connect-with-retry step but runs only the read-only readiness check, returning `ExistingDbOpen::Missing` for an absent file, `ExistingDbOpen::SchemaNotCurrent` (connection dropped) when any embedded migration is unrecorded, and `ExistingDbOpen::Open(db)` otherwise; it never issues DDL or `BEGIN IMMEDIATE`. - `EncryptedTursoDb` exposes async `execute()` and `query_map()` methods; operation methods use the same config-driven query retry policy as `TursoDb`. - Both public adapters keep concrete Turso connections and marker types; there is no shared runtime-owning core or synchronous executor bridge. - Shared lifecycle helpers: From 3d06ebb0514e538099e6fdc3d08a65419e285b40 Mon Sep 17 00:00:00 2001 From: David Abram Date: Wed, 21 Oct 2026 14:01:13 +0200 Subject: [PATCH 3/4] runtime: Gate managed telemetry on auth.db instead of the readiness marker Make managed telemetry startup, doctor health checks, and the Claude header helper use auth.db presence and typed credential lookup directly. Remove readiness-marker reads and state handling so legacy telemetry-ready.json no longer controls readiness or causes credential access on fresh installs, while preserving the existing login/logout marker writes for follow-up cleanup. Update focused tests and synchronized context to document the auth.db-only behavior. Plan: remove-telemetry-readiness-marker (T03) Co-authored-by: SCE --- .../app/doctor_reconciliation_cli_tests.rs | 14 -- cli/src/services/auth_command/mod.rs | 24 ++- .../services/auth_db/multiprocess_tests.rs | 168 +++++++++++++++--- cli/src/services/doctor/managed_telemetry.rs | 6 +- cli/src/services/doctor/render.rs | 15 +- cli/src/services/managed_telemetry.rs | 139 +++++---------- cli/src/services/telemetry_readiness.rs | 140 +-------------- context/architecture.md | 2 +- context/cli/access-policy.md | 6 +- context/context-map.md | 2 +- context/overview.md | 2 +- .../remove-telemetry-readiness-marker.md | 23 ++- context/sce/cli-observability-contract.md | 2 +- context/sce/telemetry-credential.md | 25 ++- 14 files changed, 247 insertions(+), 321 deletions(-) diff --git a/cli/src/app/doctor_reconciliation_cli_tests.rs b/cli/src/app/doctor_reconciliation_cli_tests.rs index 7793242d0..aac1d3fa4 100644 --- a/cli/src/app/doctor_reconciliation_cli_tests.rs +++ b/cli/src/app/doctor_reconciliation_cli_tests.rs @@ -18,7 +18,6 @@ const RECONCILIATION_DETAIL_PREFIX: &str = "Snapshot ref reconciliation"; const AUTH_DB_TEST_SECRET: &str = "doctor-cli-test-secret"; const UNREADABLE_AUTH_DB: &[u8] = b"not an encrypted auth database"; -const FAR_FUTURE_READINESS: &[u8] = br#"{"expires_at_unix_seconds":4102444800}"#; const TELEMETRY_ENABLED_CONFIG: &str = r#"{"telemetry":{"enabled":true}}"#; const TELEMETRY_OPT_IN_NOTE: &str = "Managed telemetry export is off because telemetry.enabled is not true; set telemetry.enabled: true in the global SCE config to resume it."; @@ -644,7 +643,6 @@ async fn doctor_on_fresh_state_reports_unconfigured_telemetry_without_a_warning( let warnings = global_state_warnings(&report); assert!(warnings.is_empty(), "{warnings:#?}"); assert!(!auth_dir.join("auth.db").exists()); - assert!(!auth_dir.join("telemetry-ready.json").exists()); } #[tokio::test(flavor = "multi_thread")] @@ -653,9 +651,7 @@ async fn doctor_fix_reports_unreadable_telemetry_credentials_without_auth_side_e let auth_dir = sandbox.state_root().join("sce"); std::fs::create_dir_all(&auth_dir).expect("auth dir"); let auth_db = auth_dir.join("auth.db"); - let readiness = auth_dir.join("telemetry-ready.json"); std::fs::write(&auth_db, UNREADABLE_AUTH_DB).expect("auth DB"); - std::fs::write(&readiness, FAR_FUTURE_READINESS).expect("readiness"); let config = sandbox.telemetry_enabled_config(); let env = [ (AUTH_DB_ENCRYPTION_KEY_ENV, AUTH_DB_TEST_SECRET), @@ -699,10 +695,6 @@ async fn doctor_fix_reports_unreadable_telemetry_credentials_without_auth_side_e std::fs::read(&auth_db).expect("auth DB kept"), UNREADABLE_AUTH_DB ); - assert_eq!( - std::fs::read(&readiness).expect("readiness kept"), - FAR_FUTURE_READINESS - ); assert!(!fix.stdout.contains(AUTH_DB_TEST_SECRET)); assert!(!fix.stderr.contains(AUTH_DB_TEST_SECRET)); } @@ -766,9 +758,7 @@ async fn doctor_with_telemetry_disabled_by_config_never_reads_the_auth_db() { std::fs::create_dir_all(&auth_dir).expect("auth dir"); let auth_db = auth_dir.join("auth.db"); - let readiness = auth_dir.join("telemetry-ready.json"); std::fs::write(&auth_db, UNREADABLE_AUTH_DB).expect("auth DB"); - std::fs::write(&readiness, FAR_FUTURE_READINESS).expect("readiness"); let report = sandbox .run_with_env(&["doctor", "--format", "json"], &env) @@ -798,8 +788,4 @@ async fn doctor_with_telemetry_disabled_by_config_never_reads_the_auth_db() { std::fs::read(&auth_db).expect("auth DB kept"), UNREADABLE_AUTH_DB ); - assert_eq!( - std::fs::read(&readiness).expect("readiness kept"), - FAR_FUTURE_READINESS - ); } diff --git a/cli/src/services/auth_command/mod.rs b/cli/src/services/auth_command/mod.rs index f75ccfafe..5951f72f3 100644 --- a/cli/src/services/auth_command/mod.rs +++ b/cli/src/services/auth_command/mod.rs @@ -17,7 +17,7 @@ use crate::services::error::{AuthStorageFailure, CliError, UserError}; use crate::services::managed_telemetry::claude_helper::{ repository_id_for, visible_destination_conflict, }; -use crate::services::managed_telemetry::{run_with_deadline, CREDENTIAL_LOOKUP_DEADLINE}; +use crate::services::managed_telemetry::{self, run_with_deadline, CREDENTIAL_LOOKUP_DEADLINE}; use crate::services::output_format::OutputFormat; use crate::services::style::{label, prompt_label, prompt_value, value}; use crate::services::telemetry_credential_storage; @@ -115,7 +115,7 @@ async fn run_telemetry_headers() -> Result { working_dir, telemetry_enabled, visible_destination_conflict(|key| std::env::var(key).ok()), - || telemetry_readiness::is_ready(current_unix_seconds()), + managed_telemetry::auth_db_present, repository_id_for, telemetry_credential_storage::load_repository_telemetry_credential, ) @@ -131,7 +131,7 @@ pub(crate) async fn telemetry_headers_with( working_dir: Option, telemetry_enabled: bool, destination_conflict: bool, - ready_probe: P, + auth_db_probe: P, resolve_repository: R, lookup: L, ) -> Result @@ -150,7 +150,7 @@ where if !telemetry_enabled || destination_conflict { return Err(telemetry_headers_unavailable()); } - let Some(working_dir) = working_dir.filter(|_| ready_probe()) else { + let Some(working_dir) = working_dir.filter(|_| auth_db_probe()) else { return Err(telemetry_headers_unavailable()); }; let credential = run_with_deadline(CREDENTIAL_LOOKUP_DEADLINE, async move { @@ -467,12 +467,6 @@ where TelemetryProvisioningResult::Unavailable } -fn current_unix_seconds() -> u64 { - std::time::SystemTime::now() - .duration_since(std::time::UNIX_EPOCH) - .map_or(0, |elapsed| elapsed.as_secs()) -} - async fn enroll_current_repository_if_unbound() { let Some(repository_id) = std::env::current_dir() .ok() @@ -1006,7 +1000,7 @@ mod tests { Some("repo-a"), ), ( - "not ready", + "no auth db", working_dir.clone(), true, false, @@ -1031,7 +1025,7 @@ mod tests { None, ), ]; - for (case, dir, telemetry_enabled, conflict, ready, repository) in cases { + for (case, dir, telemetry_enabled, conflict, auth_db, repository) in cases { let probed = Arc::new(AtomicBool::new(false)); let resolved = Arc::new(AtomicBool::new(false)); let looked_up = Arc::new(AtomicBool::new(false)); @@ -1043,7 +1037,7 @@ mod tests { let probed = Arc::clone(&probed); move || { probed.store(true, Ordering::SeqCst); - ready + auth_db } }, { @@ -1066,7 +1060,9 @@ mod tests { assert_eq!(error.to_string(), TELEMETRY_HEADERS_UNAVAILABLE, "{case}"); assert!(!looked_up.load(Ordering::SeqCst), "{case}: lookup ran"); if !telemetry_enabled { - assert!(!probed.load(Ordering::SeqCst), "{case}: readiness probed"); + assert!(!probed.load(Ordering::SeqCst), "{case}: auth.db probed"); + } + if !telemetry_enabled || !auth_db { assert!( !resolved.load(Ordering::SeqCst), "{case}: repository resolved" diff --git a/cli/src/services/auth_db/multiprocess_tests.rs b/cli/src/services/auth_db/multiprocess_tests.rs index 4c1358e04..5d1f9efbb 100644 --- a/cli/src/services/auth_db/multiprocess_tests.rs +++ b/cli/src/services/auth_db/multiprocess_tests.rs @@ -23,13 +23,15 @@ use crate::services::managed_telemetry::claude_helper::tests::git_repository_wit use crate::services::managed_telemetry::trace_export::tests::{ capture_requests_until, loopback_hook_telemetry, trace_spans, }; -use crate::services::managed_telemetry::trace_export::ClaudeHookCommand; -use crate::services::managed_telemetry::{inspect_health, ManagedTelemetryHealth}; +use crate::services::managed_telemetry::trace_export::{ClaudeHookCommand, ManagedSpanKind}; +use crate::services::managed_telemetry::{ + inspect_health, ManagedTelemetryHealth, RuntimeTelemetry, TelemetryFallbackReason, +}; use crate::services::observability::traits::{NoopLogger, Telemetry}; use crate::services::telemetry_credential_storage::{ - bind_repository_telemetry, load_repository_telemetry_credential, load_telemetry_credential, - save_telemetry_credential, test_access_token, RepositoryBindingMode, TelemetryCredential, - TelemetryCredentialLookup, TelemetryIdentity, + bind_repository_telemetry, delete_telemetry_credential, load_repository_telemetry_credential, + load_telemetry_credential, save_telemetry_credential, test_access_token, RepositoryBindingMode, + TelemetryCredential, TelemetryCredentialLookup, TelemetryIdentity, }; const CHILD_TEST: &str = "services::auth_db::multiprocess_tests::auth_db_child_process"; @@ -44,6 +46,15 @@ const WORKSPACE_ID: &str = "workspace_mp"; const READY: &str = "SCE_AUTH_DB_CHILD_READY"; const TOKEN_PREFIX: &str = "SCE_AUTH_DB_CHILD_TOKEN="; const HEALTH_PREFIX: &str = "SCE_AUTH_DB_CHILD_TELEMETRY_HEALTH="; +const HOOK_TELEMETRY_PREFIX: &str = "SCE_AUTH_DB_CHILD_HOOK_TELEMETRY="; +const HOOK_CREDENTIAL_VALID: &str = "credential_valid"; +const LEGACY_READINESS_MARKER: &str = "telemetry-ready.json"; +const LEGACY_READINESS_MARKER_CASES: [(&str, Option<&[u8]>); 4] = [ + ("absent", None), + ("valid", Some(br#"{"expires_at_unix_seconds":4102444800}"#)), + ("expired", Some(br#"{"expires_at_unix_seconds":1}"#)), + ("malformed", Some(b"not json")), +]; const CONCURRENT_PROCESSES: usize = 32; const CONCURRENT_MIGRATORS: usize = 4; const CONCURRENT_LOOKUPS: usize = 8; @@ -464,6 +475,27 @@ fn telemetry_health(sandbox: &Sandbox, secret: &str) -> String { health.into_iter().next().expect("one health line") } +fn hook_telemetry(sandbox: &Sandbox) -> String { + let stdout = sandbox.run("hook-resolve"); + let resolved = prefixed_lines(&stdout, HOOK_TELEMETRY_PREFIX); + assert_eq!(resolved.len(), 1, "{stdout}"); + resolved + .into_iter() + .next() + .expect("one hook telemetry line") +} + +fn write_legacy_readiness_marker(sandbox: &Sandbox, marker: Option<&[u8]>) -> PathBuf { + let path = sandbox.sce_state_dir().join(LEGACY_READINESS_MARKER); + match marker { + Some(bytes) => std::fs::write(&path, bytes).expect("legacy marker"), + None => { + let _ = std::fs::remove_file(&path); + } + } + path +} + #[test] fn doctor_telemetry_health_on_fresh_state_resolves_no_key_and_creates_no_auth_db() { let _fan_out = serialize_fan_out(); @@ -477,23 +509,84 @@ fn doctor_telemetry_health_classifies_real_auth_db_states_without_mutating_them( let _fan_out = serialize_fan_out(); let sandbox = Sandbox::new(); sandbox.seed(); - let state = sandbox.sce_state_dir(); - let auth_db = std::fs::read(state.join("auth.db")).expect("seeded auth DB"); - let readiness = state.join("telemetry-ready.json"); + let auth_db = sandbox.sce_state_dir().join("auth.db"); + for (case, mutation, secret, expected) in [ + ("valid", None, TEST_SECRET, "healthy"), + ("wrong key", None, WRONG_SECRET, "database_error"), + ("missing key", None, "", "database_error"), + ( + "expired", + Some("seed-expired-credential"), + TEST_SECRET, + "credential_expired", + ), + ( + "no telemetry row", + Some("delete-telemetry-credential"), + TEST_SECRET, + "not_configured", + ), + ] { + if let Some(role) = mutation { + sandbox.run(role); + } + let before = std::fs::read(&auth_db).expect("auth DB"); + assert_eq!(telemetry_health(&sandbox, secret), expected, "{case}"); + assert_eq!(std::fs::read(&auth_db).expect("auth DB"), before, "{case}"); + } +} - assert_eq!(telemetry_health(&sandbox, TEST_SECRET), "healthy"); - assert_eq!(telemetry_health(&sandbox, WRONG_SECRET), "database_error"); - assert_eq!(telemetry_health(&sandbox, ""), "database_error"); - assert_eq!( - std::fs::read(state.join("auth.db")).expect("auth DB"), - auth_db +#[test] +fn valid_credential_without_readiness_marker_resolves_managed_telemetry_and_headers() { + let _fan_out = serialize_fan_out(); + let sandbox = Sandbox::new(); + let bound = git_repository_with_remote( + sandbox.dir.path(), + HEADERS_REPOSITORY, + "https://github.com/acme/bound-repo", ); + sandbox.seed(); + let mut bind = sandbox.command("bind"); + bind.current_dir(&bound); + wait_success("bind", bind.spawn().expect("spawn bind")); + let marker = write_legacy_readiness_marker(&sandbox, None); + assert!(!marker.exists()); - std::fs::write(&readiness, br#"{"expires_at_unix_seconds":1}"#).expect("expire readiness"); - assert_eq!(telemetry_health(&sandbox, ""), "credential_expired"); - std::fs::remove_file(&readiness).expect("remove readiness"); - assert_eq!(telemetry_health(&sandbox, ""), "not_configured"); - assert_eq!(read_token(&sandbox.run("read")), Some(TOKEN)); + assert_eq!(hook_telemetry(&sandbox), HOOK_CREDENTIAL_VALID); + assert_eq!(telemetry_health(&sandbox, TEST_SECRET), "healthy"); + let enabled = headers_policy_config(&sandbox, true); + assert_headers_served(spawn_headers(&sandbox, &bound, Some(&enabled))); + assert!(!marker.exists()); +} + +#[test] +fn legacy_readiness_marker_is_ignored() { + let _fan_out = serialize_fan_out(); + for (credential_case, delete_credential, expected_health, expected_hook) in [ + ("valid credential", false, "healthy", HOOK_CREDENTIAL_VALID), + ("no credential", true, "not_configured", "not_configured"), + ] { + let sandbox = Sandbox::new(); + sandbox.seed(); + if delete_credential { + sandbox.run("delete-telemetry-credential"); + } + for (marker_case, marker) in LEGACY_READINESS_MARKER_CASES { + let case = format!("{credential_case}, {marker_case} marker"); + let path = write_legacy_readiness_marker(&sandbox, marker); + assert_eq!( + telemetry_health(&sandbox, TEST_SECRET), + expected_health, + "{case}" + ); + assert_eq!(hook_telemetry(&sandbox), expected_hook, "{case}"); + assert_eq!( + std::fs::read(&path).ok().as_deref(), + marker, + "{case}: marker bytes changed" + ); + } + } } #[test] @@ -895,7 +988,6 @@ fn claude_hook_with_telemetry_disabled_by_config_never_resolves_the_credential() sandbox.seed(); let state = sandbox.sce_state_dir(); let auth_db = std::fs::read(state.join("auth.db")).expect("seeded auth DB"); - assert!(state.join("telemetry-ready.json").is_file()); assert_eq!( policy_hook_fallback_reasons(&sandbox, false), @@ -1167,6 +1259,42 @@ async fn run_binding_child(role: &str) { TelemetryCredentialLookup::Missing ); } + "hook-resolve" => { + let (telemetry, fallback) = RuntimeTelemetry::resolve_when_enabled( + true, + load_telemetry_credential(), + ManagedSpanKind::ClaudeHook, + ) + .await; + let label = match (&telemetry, fallback) { + (RuntimeTelemetry::Managed(_), None) => HOOK_CREDENTIAL_VALID, + (_, Some(fallback)) + if fallback.reason == TelemetryFallbackReason::ExporterInitFailed => + { + HOOK_CREDENTIAL_VALID + } + (_, Some(fallback)) => fallback.reason.as_str(), + (RuntimeTelemetry::Noop(_), None) => "noop", + }; + telemetry.shutdown().await; + println!("{HOOK_TELEMETRY_PREFIX}{label}"); + } + "seed-expired-credential" => save_telemetry_credential(&TelemetryCredential { + token: TOKEN.to_string(), + accessor: "accessor-multiprocess".to_string(), + expires_at_unix_seconds: 1, + identity: TelemetryIdentity { + user_id: USER_ID.to_string(), + workspace_id: WORKSPACE_ID.to_string(), + }, + }) + .await + .expect("expired telemetry credential seeds"), + "delete-telemetry-credential" => { + assert!(delete_telemetry_credential() + .await + .expect("telemetry credential deletes")); + } "bind" => { let repository_id = repository_id_for(&std::env::current_dir().expect("cwd")) .expect("repository identity"); diff --git a/cli/src/services/doctor/managed_telemetry.rs b/cli/src/services/doctor/managed_telemetry.rs index 4d051f880..a7da3b336 100644 --- a/cli/src/services/doctor/managed_telemetry.rs +++ b/cli/src/services/doctor/managed_telemetry.rs @@ -233,10 +233,8 @@ mod tests { #[test] fn telemetry_disabled_by_config_reports_no_problem_and_a_stable_json_state() { - for readiness_hint_present in [false, true] { - let health = ManagedTelemetryHealth::DisabledByConfig { - readiness_hint_present, - }; + for auth_db_present in [false, true] { + let health = ManagedTelemetryHealth::DisabledByConfig { auth_db_present }; assert_eq!(problem(health), None); assert_eq!( json(health), diff --git a/cli/src/services/doctor/render.rs b/cli/src/services/doctor/render.rs index 49e269827..021187285 100644 --- a/cli/src/services/doctor/render.rs +++ b/cli/src/services/doctor/render.rs @@ -314,15 +314,10 @@ fn managed_telemetry_node(health: ManagedTelemetryHealth) -> DoctorDisplayNode { DoctorDisplayStatus::Pass, Vec::new(), ), - ( - ManagedTelemetryHealth::DisabledByConfig { - readiness_hint_present, - }, - None, - ) => top_level_node( + (ManagedTelemetryHealth::DisabledByConfig { auth_db_present }, None) => top_level_node( "Managed telemetry (disabled by config)", DoctorDisplayStatus::Pass, - if readiness_hint_present { + if auth_db_present { vec![DoctorDisplayDetail::Note(managed_telemetry::OPT_IN_NOTE)] } else { Vec::new() @@ -1315,16 +1310,16 @@ mod managed_telemetry_render_tests { } #[test] - fn telemetry_disabled_by_config_renders_pass_row_with_opt_in_line_only_for_ready_hint() { + fn telemetry_disabled_by_config_renders_pass_row_with_opt_in_line_only_when_auth_db_exists() { assert_eq!( rendered(ManagedTelemetryHealth::DisabledByConfig { - readiness_hint_present: false + auth_db_present: false }), [" [PASS] Managed telemetry (disabled by config)"] ); assert_eq!( rendered(ManagedTelemetryHealth::DisabledByConfig { - readiness_hint_present: true + auth_db_present: true }), [ " [PASS] Managed telemetry (disabled by config)", diff --git a/cli/src/services/managed_telemetry.rs b/cli/src/services/managed_telemetry.rs index 4d42b2af7..b00f4c5de 100644 --- a/cli/src/services/managed_telemetry.rs +++ b/cli/src/services/managed_telemetry.rs @@ -10,11 +10,10 @@ use crate::services::error::CliError; use crate::services::observability::traits::{Logger, NoopTelemetry, Telemetry}; use trace_export::{ManagedSpanKind, ManagedTraceTelemetry}; -use crate::services::default_paths::telemetry_readiness_path; +use crate::services::default_paths::auth_db_path; use crate::services::telemetry_credential_storage::{ load_telemetry_credential, TelemetryCredential, TelemetryCredentialLookup, }; -use crate::services::telemetry_readiness::{self, ReadinessState}; use crate::services::token_storage::TokenStorageError; pub const MANAGED_OTLP_ENDPOINT: &str = "https://otel.crocoderlab.dev"; @@ -236,60 +235,45 @@ where pub(crate) enum ManagedTelemetryHealth { Healthy, Disabled(TelemetryFallbackReason), - DisabledByConfig { readiness_hint_present: bool }, + DisabledByConfig { auth_db_present: bool }, } pub(crate) async fn inspect_health(telemetry_enabled: bool) -> ManagedTelemetryHealth { - let now = now_unix_seconds(); inspect_health_with( telemetry_enabled, - || telemetry_readiness::state(now), - readiness_hint_present, + auth_db_present, load_telemetry_credential(), - now, + now_unix_seconds(), ) .await } -fn readiness_hint_present() -> bool { - telemetry_readiness_path().is_ok_and(|path| path.is_file()) +pub(crate) fn auth_db_present() -> bool { + auth_db_path().is_ok_and(|path| path.is_file()) } -pub(crate) async fn inspect_health_with( +pub(crate) async fn inspect_health_with( telemetry_enabled: bool, - readiness: R, - hint_present: H, + auth_db_present: P, lookup: Fut, now_unix_seconds: u64, ) -> ManagedTelemetryHealth where - R: FnOnce() -> anyhow::Result, - H: FnOnce() -> bool, + P: FnOnce() -> bool, Fut: Future> + Send + 'static, { if !telemetry_enabled { return ManagedTelemetryHealth::DisabledByConfig { - readiness_hint_present: hint_present(), + auth_db_present: auth_db_present(), }; } - let Ok(readiness) = readiness() else { - return ManagedTelemetryHealth::Disabled(TelemetryFallbackReason::PathResolutionError); - }; - match readiness { - ReadinessState::NotConfigured => { - ManagedTelemetryHealth::Disabled(TelemetryFallbackReason::NotConfigured) - } - ReadinessState::Expired => { - ManagedTelemetryHealth::Disabled(TelemetryFallbackReason::CredentialExpired) - } - ReadinessState::Ready => match load_credential_within_deadline(lookup).await { - Err(fallback) => ManagedTelemetryHealth::Disabled(fallback.reason), - Ok(credential) => match select_mode(Some(credential), now_unix_seconds) { - ManagedTelemetryMode::Managed { .. } => ManagedTelemetryHealth::Healthy, - ManagedTelemetryMode::Noop => { - ManagedTelemetryHealth::Disabled(TelemetryFallbackReason::CredentialExpired) - } - }, + match load_credential_within_deadline(lookup).await { + Err(fallback) => ManagedTelemetryHealth::Disabled(fallback.reason), + Ok(credential) => match select_mode(Some(credential), now_unix_seconds) { + ManagedTelemetryMode::Managed { .. } => ManagedTelemetryHealth::Healthy, + ManagedTelemetryMode::Noop => { + ManagedTelemetryHealth::Disabled(TelemetryFallbackReason::CredentialExpired) + } }, } } @@ -314,27 +298,21 @@ impl RuntimeTelemetry { let Some(span_kind) = span_kind else { return Self::Noop(NoopTelemetry); }; - let (telemetry, fallback) = Self::resolve_when_ready( - telemetry_enabled, - || telemetry_readiness::is_ready(now_unix_seconds()), - load_telemetry_credential(), - span_kind, - ) - .await; + let (telemetry, fallback) = + Self::resolve_when_enabled(telemetry_enabled, load_telemetry_credential(), span_kind) + .await; if let Some(fallback) = fallback { fallback.log(logger); } telemetry } - pub(crate) async fn resolve_when_ready( + pub(crate) async fn resolve_when_enabled( telemetry_enabled: bool, - ready: R, lookup: Fut, span_kind: ManagedSpanKind, ) -> (Self, Option) where - R: FnOnce() -> bool, Fut: Future> + Send + 'static, { if !telemetry_enabled { @@ -345,14 +323,6 @@ impl RuntimeTelemetry { )), ); } - if !ready() { - return ( - Self::Noop(NoopTelemetry), - Some(TelemetryFallback::new( - TelemetryFallbackReason::NotConfigured, - )), - ); - } Self::resolve_with(lookup, span_kind).await } @@ -476,13 +446,12 @@ mod tests { } #[tokio::test] - async fn claude_hook_without_readiness_never_looks_up_credentials() { + async fn claude_hook_without_auth_db_resolves_not_configured() { let looked_up = std::sync::Arc::new(std::sync::atomic::AtomicBool::new(false)); let flag = looked_up.clone(); - let (telemetry, fallback) = RuntimeTelemetry::resolve_when_ready( + let (telemetry, fallback) = RuntimeTelemetry::resolve_when_enabled( true, - || false, async move { flag.store(true, std::sync::atomic::Ordering::SeqCst); Ok(TelemetryCredentialLookup::Missing) @@ -492,10 +461,10 @@ mod tests { .await; assert!(matches!(telemetry, RuntimeTelemetry::Noop(_))); - let fallback = fallback.expect("disabled telemetry is diagnosed"); + let fallback = fallback.expect("missing credential is diagnosed"); assert_eq!(fallback.reason, TelemetryFallbackReason::NotConfigured); assert!(!fallback.reason.is_failure()); - assert!(!looked_up.load(std::sync::atomic::Ordering::SeqCst)); + assert!(looked_up.load(std::sync::atomic::Ordering::SeqCst)); } fn forbidden_lookup() -> Result { @@ -503,11 +472,10 @@ mod tests { } #[tokio::test] - async fn telemetry_disabled_by_config_never_probes_readiness_or_looks_up_credentials() { + async fn telemetry_disabled_by_config_never_looks_up_credentials() { for span_kind in [ManagedSpanKind::Command, ManagedSpanKind::ClaudeHook] { - let (telemetry, fallback) = RuntimeTelemetry::resolve_when_ready( + let (telemetry, fallback) = RuntimeTelemetry::resolve_when_enabled( false, - || panic!("readiness must not be probed when telemetry is disabled by config"), async { forbidden_lookup() }, span_kind, ) @@ -523,20 +491,14 @@ mod tests { } #[tokio::test] - async fn health_disabled_by_config_never_probes_readiness_or_looks_up_credentials() { - for hint in [false, true] { - let health = inspect_health_with( - false, - || panic!("readiness must not be probed when telemetry is disabled by config"), - || hint, - async { forbidden_lookup() }, - NOW, - ) - .await; + async fn health_disabled_by_config_never_looks_up_credentials() { + for present in [false, true] { + let health = + inspect_health_with(false, || present, async { forbidden_lookup() }, NOW).await; assert_eq!( health, ManagedTelemetryHealth::DisabledByConfig { - readiness_hint_present: hint + auth_db_present: present } ); } @@ -544,9 +506,8 @@ mod tests { #[tokio::test] async fn claude_hook_with_inaccessible_credential_store_skips_quietly() { - let (telemetry, fallback) = RuntimeTelemetry::resolve_when_ready( + let (telemetry, fallback) = RuntimeTelemetry::resolve_when_enabled( true, - || true, async { Err(TokenStorageError::CredentialAccessUnavailable) }, ManagedSpanKind::ClaudeHook, ) @@ -643,28 +604,7 @@ mod tests { } #[tokio::test] - async fn health_without_readiness_or_with_expired_readiness_never_looks_up_credentials() { - for (readiness, reason) in [ - ( - ReadinessState::NotConfigured, - TelemetryFallbackReason::NotConfigured, - ), - ( - ReadinessState::Expired, - TelemetryFallbackReason::CredentialExpired, - ), - ] { - let (looked_up, lookup) = tracked_lookup(Ok(TelemetryCredentialLookup::Valid( - credential("bao-token", NOW + 60), - ))); - let health = inspect_health_with(true, || Ok(readiness), || false, lookup, NOW).await; - assert_eq!(health, ManagedTelemetryHealth::Disabled(reason)); - assert!(!looked_up.load(std::sync::atomic::Ordering::SeqCst)); - } - } - - #[tokio::test] - async fn health_with_readiness_classifies_lookup_outcomes() { + async fn health_classifies_lookup_outcomes() { let cases: Vec<( Result, ManagedTelemetryHealth, @@ -710,9 +650,13 @@ mod tests { ]; for (result, expected) in cases { let (looked_up, lookup) = tracked_lookup(result); - let health = - inspect_health_with(true, || Ok(ReadinessState::Ready), || false, lookup, NOW) - .await; + let health = inspect_health_with( + true, + || panic!("auth.db presence must not be probed when telemetry is enabled"), + lookup, + NOW, + ) + .await; assert_eq!(health, expected); assert!(looked_up.load(std::sync::atomic::Ordering::SeqCst)); } @@ -723,7 +667,6 @@ mod tests { let started = std::time::Instant::now(); let health = inspect_health_with( true, - || Ok(ReadinessState::Ready), || false, std::future::pending::>(), NOW, diff --git a/cli/src/services/telemetry_readiness.rs b/cli/src/services/telemetry_readiness.rs index 00e9b2444..b1e85685f 100644 --- a/cli/src/services/telemetry_readiness.rs +++ b/cli/src/services/telemetry_readiness.rs @@ -5,17 +5,10 @@ use std::path::{Path, PathBuf}; use anyhow::{Context, Result}; use serde::{Deserialize, Serialize}; -use crate::services::default_paths::{auth_db_path, telemetry_readiness_path}; +use crate::services::default_paths::telemetry_readiness_path; const TEMP_SUFFIX: &str = "tmp"; -#[derive(Clone, Copy, Debug, Eq, PartialEq)] -pub(crate) enum ReadinessState { - NotConfigured, - Expired, - Ready, -} - #[derive(Clone, Copy, Debug, Eq, PartialEq, Serialize, Deserialize)] struct TelemetryReadiness { expires_at_unix_seconds: u64, @@ -29,19 +22,7 @@ pub fn clear() -> Result<()> { clear_at(&telemetry_readiness_path()?) } -pub fn is_ready(now_unix_seconds: u64) -> bool { - state(now_unix_seconds).is_ok_and(|state| state == ReadinessState::Ready) -} - -pub(crate) fn state(now_unix_seconds: u64) -> Result { - Ok(state_at( - &telemetry_readiness_path()?, - &auth_db_path()?, - now_unix_seconds, - )) -} - -pub(crate) fn mark_ready_at(path: &Path, expires_at_unix_seconds: u64) -> Result<()> { +fn mark_ready_at(path: &Path, expires_at_unix_seconds: u64) -> Result<()> { let parent = path.parent().with_context(|| { format!( "telemetry readiness path '{}' has no parent", @@ -63,7 +44,7 @@ pub(crate) fn mark_ready_at(path: &Path, expires_at_unix_seconds: u64) -> Result }) } -pub(crate) fn clear_at(path: &Path) -> Result<()> { +fn clear_at(path: &Path) -> Result<()> { match fs::remove_file(path) { Ok(()) => Ok(()), Err(error) if error.kind() == ErrorKind::NotFound => Ok(()), @@ -71,123 +52,8 @@ pub(crate) fn clear_at(path: &Path) -> Result<()> { } } -#[cfg(test)] -pub(crate) fn is_ready_at(path: &Path, auth_db_path: &Path, now_unix_seconds: u64) -> bool { - state_at(path, auth_db_path, now_unix_seconds) == ReadinessState::Ready -} - -pub(crate) fn state_at(path: &Path, auth_db_path: &Path, now_unix_seconds: u64) -> ReadinessState { - if !auth_db_path.is_file() { - return ReadinessState::NotConfigured; - } - match fs::read(path) - .ok() - .and_then(|bytes| serde_json::from_slice::(&bytes).ok()) - { - None => ReadinessState::NotConfigured, - Some(readiness) if readiness.expires_at_unix_seconds > now_unix_seconds => { - ReadinessState::Ready - } - Some(_) => ReadinessState::Expired, - } -} - fn temp_path_for(path: &Path) -> PathBuf { let mut name = path.as_os_str().to_os_string(); name.push(format!(".{}.{TEMP_SUFFIX}", std::process::id())); PathBuf::from(name) } - -#[cfg(test)] -mod tests { - use super::*; - - const NOW: u64 = 1_000; - - struct Paths { - _dir: tempfile::TempDir, - readiness: PathBuf, - auth_db: PathBuf, - } - - fn paths_with_auth_db() -> Paths { - let dir = tempfile::tempdir().unwrap(); - let readiness = dir.path().join("sce").join("telemetry-ready.json"); - let auth_db = dir.path().join("sce").join("auth.db"); - fs::create_dir_all(auth_db.parent().unwrap()).unwrap(); - fs::write(&auth_db, b"encrypted").unwrap(); - Paths { - _dir: dir, - readiness, - auth_db, - } - } - - #[test] - fn readiness_state_requires_valid_unexpired_marker_and_auth_db() { - for (case, marker, auth_db, expected) in [ - ("missing marker", None, true, ReadinessState::NotConfigured), - ( - "malformed marker", - Some(b"not json".to_vec()), - true, - ReadinessState::NotConfigured, - ), - ( - "valid marker", - Some(format!(r#"{{"expires_at_unix_seconds":{}}}"#, NOW + 1).into_bytes()), - true, - ReadinessState::Ready, - ), - ( - "expired marker", - Some(format!(r#"{{"expires_at_unix_seconds":{NOW}}}"#).into_bytes()), - true, - ReadinessState::Expired, - ), - ( - "missing auth DB", - Some(format!(r#"{{"expires_at_unix_seconds":{}}}"#, NOW + 1).into_bytes()), - false, - ReadinessState::NotConfigured, - ), - ] { - let paths = paths_with_auth_db(); - if let Some(marker) = marker { - fs::write(&paths.readiness, marker).unwrap(); - } - if !auth_db { - fs::remove_file(&paths.auth_db).unwrap(); - } - assert_eq!( - state_at(&paths.readiness, &paths.auth_db, NOW), - expected, - "{case}" - ); - } - } - - #[test] - fn marked_readiness_is_ready_until_expiry_and_holds_no_secret() { - let paths = paths_with_auth_db(); - mark_ready_at(&paths.readiness, NOW + 1).unwrap(); - - assert!(is_ready_at(&paths.readiness, &paths.auth_db, NOW)); - assert!(!is_ready_at(&paths.readiness, &paths.auth_db, NOW + 1)); - assert_eq!( - fs::read_to_string(&paths.readiness).unwrap(), - format!(r#"{{"expires_at_unix_seconds":{}}}"#, NOW + 1) - ); - } - - #[test] - fn clearing_readiness_disables_it_and_is_idempotent() { - let paths = paths_with_auth_db(); - mark_ready_at(&paths.readiness, NOW + 1).unwrap(); - - clear_at(&paths.readiness).unwrap(); - clear_at(&paths.readiness).unwrap(); - - assert!(!is_ready_at(&paths.readiness, &paths.auth_db, NOW)); - } -} diff --git a/context/architecture.md b/context/architecture.md index e54488b2f..04d0118b4 100644 --- a/context/architecture.md +++ b/context/architecture.md @@ -62,7 +62,7 @@ The repository includes a new placeholder Rust binary crate at `cli/`. - `cli/src/services/output_format.rs` defines the canonical shared CLI output-format contract (`OutputFormat`) for supporting commands, with deterministic `text|json` parsing and command-scoped actionable invalid-value guidance. - `cli/src/services/config/types.rs` is the canonical owner for the shared runtime/config primitive seam used by the CLI: `LogLevel`, `LogFormat`, `SCE_LOG_LEVEL`, `SCE_LOG_FORMAT`, `SCE_LOG_DIR`, `DEFAULT_LOG_FILE_RETENTION_LIMIT`, and the shared bool parsing helpers used by both config resolution and observability bootstrap; `cli/src/services/config/mod.rs` re-exports those primitives through the facade. - `cli/src/services/capabilities.rs` defines the current broad CLI capability traits consumed by the borrowed, compile-time-typed `AppContext`: `FsOps` with `StdFsOps` as a method-less filesystem capability marker and `GitOps` with `ProcessGitOps` for git command execution and availability checks. Existing service internals do not consume these traits directly yet; command execution uses narrow accessors and repo-root-scoped context derivation. -- `cli/src/services/lifecycle.rs` defines the current compile-safe lifecycle seam. `ServiceLifecycle` has default no-op generic `diagnose`, `fix`, and `setup` methods over `C: HasRepoRoot`, with lifecycle-owned health, fix, and setup result types so the trait contract is not publicly anchored to doctor/setup module types or the full `AppContext` shape. The same module owns the static `LifecycleProvider` enum and shared `lifecycle_providers(include_hooks)` catalog/factory, returning providers in deterministic order (config → local_db → auth_db → agent_trace_db → hooks when requested); enum dispatch calls each concrete provider through generic context methods without boxed lifecycle-provider allocation or repo-root trait-object context erasure. Hooks exposes a `HooksLifecycle` provider in `cli/src/services/hooks/lifecycle.rs` for hook rollout diagnosis/fix/setup using lifecycle-owned health records plus the canonical required-hook installer. Config exposes a `ConfigLifecycle` provider in `cli/src/services/config/lifecycle.rs` for global/repo-local config validation and repo-local `.sce/config.json` bootstrap. local_db exposes a `LocalDbLifecycle` provider in `cli/src/services/local_db/lifecycle.rs` for canonical local DB path health, parent-directory readiness/bootstrap, and `LocalDb::new()` setup. auth_db exposes an `AuthDbLifecycle` provider in `cli/src/services/auth_db/lifecycle.rs` for canonical auth DB path health, parent-directory readiness/bootstrap, and `AuthDb::new()` setup. agent_trace_db exposes an `AgentTraceDbLifecycle` provider in `cli/src/services/agent_trace_db/lifecycle.rs` for setup-time repository-scoped Agent Trace storage initialization when a repo root is available and repository Agent Trace DB path health/fix from resolved repository identity, returning an actionable "requires a Git repository" diagnostic outside repository context (no global/checkout fallback path; the former fallback was removed by the `retire-legacy-agent-trace-db` plan). Doctor runtime aggregates the full provider catalog for `diagnose` and `fix` and adapts lifecycle records into doctor report/fix records at the orchestration boundary; setup command aggregates the shared catalog for `setup` with hooks included only when requested and adapts hook setup outcomes before rendering setup-owned messages. +- `cli/src/services/lifecycle.rs` defines the current compile-safe lifecycle seam. `ServiceLifecycle` has default no-op generic `diagnose`, `fix`, and `setup` methods over `C: HasRepoRoot`, with lifecycle-owned health, fix, and setup result types so the trait contract is not publicly anchored to doctor/setup module types or the full `AppContext` shape. The same module owns the static `LifecycleProvider` enum and shared `lifecycle_providers(include_hooks)` catalog/factory, returning providers in deterministic order (config → local_db → auth_db → agent_trace_db → hooks when requested); enum dispatch calls each concrete provider through generic context methods without boxed lifecycle-provider allocation or repo-root trait-object context erasure. Hooks exposes a `HooksLifecycle` provider in `cli/src/services/hooks/lifecycle.rs` for hook rollout diagnosis/fix/setup using lifecycle-owned health records plus the canonical required-hook installer. Config exposes a `ConfigLifecycle` provider in `cli/src/services/config/lifecycle.rs` for global/repo-local config validation and repo-local `.sce/config.json` bootstrap. local_db exposes a `LocalDbLifecycle` provider in `cli/src/services/local_db/lifecycle.rs` for canonical local DB path health, parent-directory readiness/bootstrap, and `LocalDb::new()` setup. auth_db exposes an `AuthDbLifecycle` provider in `cli/src/services/auth_db/lifecycle.rs` for canonical auth DB path health and parent-directory readiness/bootstrap; its setup creates only the parent directory and never creates `auth.db` or resolves a key. agent_trace_db exposes an `AgentTraceDbLifecycle` provider in `cli/src/services/agent_trace_db/lifecycle.rs` for setup-time repository-scoped Agent Trace storage initialization when a repo root is available and repository Agent Trace DB path health/fix from resolved repository identity, returning an actionable "requires a Git repository" diagnostic outside repository context (no global/checkout fallback path; the former fallback was removed by the `retire-legacy-agent-trace-db` plan). Doctor runtime aggregates the full provider catalog for `diagnose` and `fix` and adapts lifecycle records into doctor report/fix records at the orchestration boundary; setup command aggregates the shared catalog for `setup` with hooks included only when requested and adapts hook setup outcomes before rendering setup-owned messages. - Agent Trace lifecycle setup resolves `agent_trace.repository_id` / `agent_trace.repository_remote` and creates or migrates the repository-scoped DB through `agent_trace_storage::resolve_agent_trace_storage(...)`; hook runtime uses the same storage identity/path resolution and no-migration open path, with missing or stale schema failing open through the existing `Run 'sce setup'.` guidance. - `cli/src/services/auth_command/mod.rs` defines the implemented auth command surface for `sce auth login|logout|whoami`, including device-flow login, stored-credential validation/renewal through login with device-flow fallback, logout, and Control Plane `/me`-backed whoami rendering in text/JSON formats; text mode uses flat `Email`, `First Name`, `Last Name`, `Role`, `Permissions`, and `Organization Name` labels, with optional names and missing role/permissions/workspace values handled deterministically. Logged-out text returns exact login guidance and renewal reports retain the `login` operation label. `cli/src/services/auth_command/command.rs` owns the `AuthCommand` payload used by the static `RuntimeCommand` enum. There is no public renewal or status subcommand. - `cli/src/services/db/mod.rs` provides shared async Turso infrastructure: `DbSpec` supplies a service name, path, ordered migrations, and config key; `TursoDb` / `EncryptedTursoDb` own concrete connections but no Tokio runtime. Construction, migration, query/materialization, transaction, readiness, and checkpoint operations are awaited through `run_with_retry` on the application runtime. SQL, one-connection transaction scope, migration metadata, retry classification, WAL, encryption, and per-database policy remain adapter-owned; see `context/sce/shared-turso-db.md`. diff --git a/context/cli/access-policy.md b/context/cli/access-policy.md index 5b4d8a017..232f18f3b 100644 --- a/context/cli/access-policy.md +++ b/context/cli/access-policy.md @@ -122,9 +122,9 @@ The note documents a coverage limit: telemetry opt-out does not remove Claude-na ## Enforcement -- **Managed telemetry startup** (commands and admitted Claude hooks): `cli/src/app.rs` takes `telemetry_enabled()` from the startup `ResolvedObservabilityRuntimeConfig`. When telemetry is not `Enabled`, `RuntimeTelemetry::resolve` returns Noop with fallback reason `disabled_by_config` (logged at `debug`). This happens before the readiness hint is read, `auth.db` is opened, or the credential store is touched. See [telemetry credential](../sce/telemetry-credential.md). -- **`sce doctor` managed-telemetry health:** doctor resolves the policy for the inspected repository root. A resolution failure counts as not enabled. When telemetry is not `Enabled`, it reports `[PASS] Managed telemetry (disabled by config)` (JSON `state` / `reason` `disabled_by_config`). It may check whether the readiness hint file exists, but never reads it or touches `auth.db`. See [telemetry credential: doctor health check](../sce/telemetry-credential.md#doctor-health-check). -- **`sce auth telemetry-headers`** (Claude's `otelHeadersHelper`): the helper resolves the policy from its working directory through `resolve_auth_runtime_config`. A missing working directory or a resolution failure counts as not enabled. When telemetry is not `Enabled`, it fails closed with the existing fixed `Telemetry credential is unavailable.` diagnostic and empty stdout. This happens before the destination tripwire, the readiness hint, repository resolution, and the `auth.db` lookup. See [telemetry credential: hidden header helper](../sce/telemetry-credential.md#hidden-header-helper). +- **Managed telemetry startup** (commands and admitted Claude hooks): `cli/src/app.rs` takes `telemetry_enabled()` from the startup `ResolvedObservabilityRuntimeConfig`. When telemetry is not `Enabled`, `RuntimeTelemetry::resolve` returns Noop with fallback reason `disabled_by_config` (logged at `debug`). This happens before `auth.db` is opened or the credential store is touched. See [telemetry credential](../sce/telemetry-credential.md). +- **`sce doctor` managed-telemetry health:** doctor resolves the policy for the inspected repository root. A resolution failure counts as not enabled. When telemetry is not `Enabled`, it reports `[PASS] Managed telemetry (disabled by config)` (JSON `state` / `reason` `disabled_by_config`). It may check whether `auth.db` exists (for the opt-in note), but never opens it or touches the credential store. See [telemetry credential: doctor health check](../sce/telemetry-credential.md#doctor-health-check). +- **`sce auth telemetry-headers`** (Claude's `otelHeadersHelper`): the helper resolves the policy from its working directory through `resolve_auth_runtime_config`. A missing working directory or a resolution failure counts as not enabled. When telemetry is not `Enabled`, it fails closed with the existing fixed `Telemetry credential is unavailable.` diagnostic and empty stdout. This happens before the destination tripwire, the `auth.db` existence check, repository resolution, and the `auth.db` lookup. See [telemetry credential: hidden header helper](../sce/telemetry-credential.md#hidden-header-helper). - **`sce setup` Claude telemetry install:** with the Claude target selected, setup resolves the policy for the setup repository root through `resolve_auth_runtime_config`. A resolution failure counts as not enabled. When telemetry is not `Enabled`, it skips the Claude-native install with the fixed line `Claude telemetry: skipped; SCE telemetry is disabled by config.` and leaves user Claude settings unchanged. See [telemetry credential: Claude-native telemetry](../sce/telemetry-credential.md#claude-native-telemetry). - **`sce auth login` / `sce auth whoami`:** both resolve the policy from the working directory through `resolve_auth_runtime_config` and check `auth_allowed()` before any `token_storage` call. A resolution failure is a command error. - Under `disabled`, both fail with `UserError::AuthDisabledByConfig` (key `auth.disabled_by_config`, runtime class, exit code 4) before `auth.db` or the credential store is touched. The fixed message names `auth.mode: disabled` and the config file of the deciding layer: the global SCE config, the repository `.sce/config.json`, the `--config` file, or the `SCE_CONFIG_FILE` file (`AuthPolicyConfigFile`). It tells the user to set `auth.mode` to `optional` or `mandatory` there. diff --git a/context/context-map.md b/context/context-map.md index 2b46e22e7..267047db4 100644 --- a/context/context-map.md +++ b/context/context-map.md @@ -25,7 +25,7 @@ Feature/domain context: - `context/cli/async-persistence-guarantees.md` (verified guarantees and named limits of the directly awaited Turso persistence migration — exclusive `&mut` transactions, deferred rollback, credential-rotation and terminal-sync behavior, lease/descriptor protection — plus PR1–PR4 staging and the PR3 cleanup ledger) - `context/cli/agent-trace-sync-command.md` (composed local-to-control-plane `sce sync` architecture: the `hooks/plugins → repository Agent Trace DB → AgentTraceExportReader → sce sync → HTTPS + WorkOS Bearer → control plane` data flow, the `sce auth login` / `cd ` / `sce sync` user flow, the no-local-cursor/no-`agent-trace-sync.db`/no-Turso-Sync/no-`BridgeLock`/no-local-DWH invariants, the four-local-capture-streams versus three-remote-streams boundary with its `diff_traces` release gate, the optional `commitPatchId` field on the existing `agent_traces` stream with its Control Plane release dependency and no-backfill limit, and `401`/`409`/ambiguous-batch-failure recovery semantics) - `context/cli/config-precedence-contract.md` (implemented `sce config` show/validate command contract, deterministic `flags > env > config file > defaults` resolution order, focused `config/resolver.rs` ownership for config discovery/merge/runtime precedence plus default-discovered invalid-file degradation, focused `config/render.rs` ownership for `show`/`validate` text+JSON output construction, canonical `$schema` acceptance for startup-loaded `sce/config.json` files, shared auth-key env/config/optional baked-default support starting with `workos_client_id`, shared runtime resolution for flat logging observability keys including config-file/default `log_to_file`, `log_dir` / `SCE_LOG_DIR` with `/sce/logs` defaulting plus config-file/default-only positive `log_file_retention_limit`, config-file-only `agent_trace.repository_id`/`agent_trace.repository_remote` repository-identity keys with default remote `origin`, `agent_trace.auto_sync` resolution with omitted-value fallback `false`, explicit-false opt-out, and setup-written explicit `true` bootstrap behavior for the post-commit trigger boundary, the catalog-derived `integrations.optional_workflows` optional-workflow selection key, schema-validated per-layer `auth.mode` / `telemetry.enabled` sections, JSON-pointer-prefixed schema-validation errors, canonical Pkl-generated `sce/config.json` schema ownership plus CLI embedding/reuse contract including `policies.attribution_hooks.enabled` default-true/explicit-false opt-out metadata, config-file selection order, `show` provenance output, and trimmed `validate` output contract) -- `context/cli/access-policy.md` (effective access policy resolved from user (global), repository (Git-root `.sce/config.json`), and explicit (`--config` / `SCE_CONFIG_FILE`) config layers, where an explicit file cannot lift a global or repository restriction: `auth.mode` most-restrictive-wins, consent-gated `telemetry.enabled` state and disabled reasons, degraded-config shutdown of telemetry and background cloud operations, fixed contradiction warnings, `sce config show` policy reporting, exposure on the auth/hook/observability runtime projections, and enforcement in managed telemetry startup, `sce doctor` health (`disabled_by_config`), the hidden `sce auth telemetry-headers` helper (fail closed before readiness, repository resolution, and `auth.db`), `sce auth login`/`whoami` (`auth.disabled_by_config` rejection before credential storage, `disabled` telemetry provisioning without consent, `mandatory` unauthenticated whoami failure), `sce sync` (`auth.disabled_by_config` before storage or credentials), post-commit auto-sync plus its doctor row (launch only when background cloud operations are allowed), and the `sce setup` Claude-native telemetry install (skipped with a fixed line unless telemetry is enabled); decision in `context/decisions/2026-10-20-consent-gated-access-policy-precedence.md`) +- `context/cli/access-policy.md` (effective access policy resolved from user (global), repository (Git-root `.sce/config.json`), and explicit (`--config` / `SCE_CONFIG_FILE`) config layers, where an explicit file cannot lift a global or repository restriction: `auth.mode` most-restrictive-wins, consent-gated `telemetry.enabled` state and disabled reasons, degraded-config shutdown of telemetry and background cloud operations, fixed contradiction warnings, `sce config show` policy reporting, exposure on the auth/hook/observability runtime projections, and enforcement in managed telemetry startup, `sce doctor` health (`disabled_by_config`), the hidden `sce auth telemetry-headers` helper (fail closed before the `auth.db` existence check, repository resolution, and the `auth.db` lookup), `sce auth login`/`whoami` (`auth.disabled_by_config` rejection before credential storage, `disabled` telemetry provisioning without consent, `mandatory` unauthenticated whoami failure), `sce sync` (`auth.disabled_by_config` before storage or credentials), post-commit auto-sync plus its doctor row (launch only when background cloud operations are allowed), and the `sce setup` Claude-native telemetry install (skipped with a fixed line unless telemetry is enabled); decision in `context/decisions/2026-10-20-consent-gated-access-policy-precedence.md`) - `context/cli/capability-traits.md` (current broad CLI capability seam in `cli/src/services/capabilities.rs`, including `FsOps`/`StdFsOps` (method-less marker), `GitOps`/`ProcessGitOps` (command execution and availability), compile-time-typed borrowed AppContext wiring with scoped borrowing across directly awaited commands with associated-type narrow capability accessors plus `ContextWithRepoRoot` repo-root-scoped context derivation, generic command execution bounds, and test-only unimplemented stubs; current service internals do not consume fs/git traits until later lifecycle migration tasks) - `context/cli/service-lifecycle.md` (current async lifecycle seam in `cli/src/services/lifecycle.rs`, including default no-op `ServiceLifecycle` diagnose/fix/setup methods against narrow `HasRepoRoot`, lifecycle-owned health/fix/setup result types with generic setup messages, doctor/setup adapter boundaries, the static `LifecycleProvider` enum catalog/dispatcher, hook/config/local_db/auth_db/agent_trace_db lifecycle providers including setup-time repository-scoped Agent Trace DB initialization plus checkout identity diagnostics, implemented doctor aggregation over diagnose/fix providers, and implemented setup aggregation over `setup` providers in order config → local_db → auth_db → agent_trace_db → hooks when requested) - `context/cli/mutation-trace-protocol.md` (pure, dependency-free `cli/src/services/mutation_trace/` refinement of the verified `spec/mutation_cursor.qnt` mutation-cursor protocol: the full pure action set — `prepare`/`commit` transition logic, attribution/mutation-event materialization, snapshot-failure/database-failure taint actions, scope abandonment, and recovery with an explicit observed-tree input — plus cross-action sequence/invariant tests and a module-level Quint refinement matrix are all complete (`types.rs`, `protocol.rs`, `mod.rs`, `tests.rs`, registered with `#[allow(dead_code)]`); opaque-newtype identity refinement decisions vs. the Quint model's bounded enums, and the `coordinator.rs` target end-state seam this layout leaves room for but does not create — `store.rs`, `runtime/git_snapshot.rs`, and `runtime/coordinator.rs` (including its public `coordinate()` entrypoint) now exist, built out by the `mutation-cursor-store-persistence` and `mutation-cursor-runtime-coordinator` plans respectively; `protocol.rs` is invoked only through the runtime, and the runtime is now driven by the generic `sce hooks mutation-scope` CLI ingress and a first Claude Code adapter driver, registered by `sce setup` and reachable by a real Claude Code session, now proven against real Git repositories and a real Agent Trace DB by the `claude-mutation-scope-integration` plan's T08 regressions) diff --git a/context/overview.md b/context/overview.md index 310cf7209..b495ab3ad 100644 --- a/context/overview.md +++ b/context/overview.md @@ -68,7 +68,7 @@ The same runtime also emits stable CLI stderr diagnostics: internal failures use The app runtime now also includes a structured observability baseline in `cli/src/services/observability.rs`: deterministic env-controlled log threshold/format (`SCE_LOG_LEVEL` defaults to `error`; `SCE_LOG_FORMAT` defaults to `text`), default-backed log-directory routing (`SCE_LOG_DIR` / config-file `log_dir` / `/sce/logs`) with per-operation machine-local dated file selection, optional session filename partitioning, and creation-triggered retention of direct regular `*.log` files to 10 entries, stable lifecycle event IDs, normal logger records routed only to files when file logging is enabled and only error-level logger records routed to `stderr` when file logging is disabled, direct file-write diagnostics remaining on `stderr`, and `observability::traits` boundaries for logger and telemetry behavior. When the effective access policy enables telemetry (`telemetry.enabled`, see `context/cli/access-policy.md`) and the encrypted auth DB holds a valid unexpired telemetry credential, commands also export `sce.command` spans (Claude-invoked SCE hooks export one root `sce.hook` span instead, with `sce.mutation_scope`/`sce.git_snapshot`/`sce.persistence` children only for operations that ran; other hooks export nothing) and allowlisted logger events as OTLP/HTTP protobuf to the hardcoded `https://otel.crocoderlab.dev` `/v1/traces` and `/v1/logs` paths (bearer token only, redirects disabled, ambient `OTEL_EXPORTER_OTLP_*` and repo config cannot redirect it, fail-open and bounded); with telemetry not enabled by policy or without a credential nothing is exported, and `/v1/metrics` is reserved with no metrics pipeline. One application-level multi-thread Tokio runtime directly awaits static command dispatch through async credentials, lifecycle providers, hooks, sync/export, and Turso persistence. The shared Turso adapters own no runtime or executor bridge, and foreground hooks return after required persistence completes. See [application execution runtime](architecture.md#application-execution-runtime). The app command dispatcher now enforces a centralized stdout/stderr stream contract in `cli/src/app.rs`: command success payloads are emitted on stdout only, while redacted user-facing diagnostics and text-mode sync progress are emitted on stderr; JSON sync remains silent. `cli/src/app.rs` also now runs through explicit startup phases — dependency check, observability config resolution, runtime initialization, command parse/execute, and output rendering — with the app runtime carrying logger/telemetry plus static command-catalog state across those phases while preserving the existing exit-code and degraded-startup contracts. Within that lifecycle, `parse_command_phase` delegates clap-to-runtime conversion to `cli/src/services/parse/command_runtime.rs`, which returns a static `RuntimeCommand` enum, `services::app_support::execute_command_phase` logs around enum-owned `execute_with_stderr(...)` dispatch, and generic `RunOutcome` rendering logs classified errors through the logger trait boundary without coupling render support to the production logger type. Command payload structs for `help`, `version`, `completion`, `auth`, `config`, `setup`, `doctor`, `hooks`, and `sync` live in service-owned `command.rs` files; `cli/src/services/command_registry.rs` owns the deterministic static command-name catalog and enum variants instead of boxed command trait objects. -The CLI now also enforces a shared output-format parser contract in `cli/src/services/output_format.rs`, with canonical `--format ` parsing and command-specific actionable invalid-value guidance reused by `config` and `version` services. A compile-safe service lifecycle seam also exists in `cli/src/services/lifecycle.rs`: `ServiceLifecycle` exposes default no-op `diagnose`, `fix`, and `setup` methods against the narrow `HasRepoRoot` accessor, uses lifecycle-owned health/fix/setup result types, and owns the shared static `LifecycleProvider` enum catalog/factory with deterministic config → local*db → auth_db → agent_trace_db → hooks ordering and no boxed provider aggregation. Hooks has a `services/hooks/lifecycle.rs` provider for hook rollout diagnosis/fix/setup, config has a `services/config/lifecycle.rs` provider for global/repo-local config validation plus repo-local config bootstrap, local_db has a `services/local_db/lifecycle.rs` provider for canonical local DB path health, parent-directory readiness/bootstrap, and `LocalDb::new()` setup, auth_db has a `services/auth_db/lifecycle.rs` provider for canonical auth DB path health, parent-directory readiness/bootstrap, and `AuthDb::new()` setup, and agent_trace_db has a `services/agent_trace_db/lifecycle.rs` provider for repository-scoped Agent Trace DB setup and repository DB path health/parent readiness from resolved repository identity, returning an actionable "requires a Git repository" diagnostic outside repository context (no global/checkout fallback path). Doctor runtime aggregates the full shared provider catalog for `diagnose` and `fix` and adapts lifecycle records into doctor-owned output records; setup command aggregates the shared provider catalog for `setup` with hooks included only when requested and adapts lifecycle setup outcomes before rendering setup-owned messages. +The CLI now also enforces a shared output-format parser contract in `cli/src/services/output_format.rs`, with canonical `--format ` parsing and command-specific actionable invalid-value guidance reused by `config` and `version` services. A compile-safe service lifecycle seam also exists in `cli/src/services/lifecycle.rs`: `ServiceLifecycle` exposes default no-op `diagnose`, `fix`, and `setup` methods against the narrow `HasRepoRoot` accessor, uses lifecycle-owned health/fix/setup result types, and owns the shared static `LifecycleProvider` enum catalog/factory with deterministic config → local*db → auth_db → agent_trace_db → hooks ordering and no boxed provider aggregation. Hooks has a `services/hooks/lifecycle.rs` provider for hook rollout diagnosis/fix/setup, config has a `services/config/lifecycle.rs` provider for global/repo-local config validation plus repo-local config bootstrap, local_db has a `services/local_db/lifecycle.rs` provider for canonical local DB path health, parent-directory readiness/bootstrap, and `LocalDb::new()` setup, auth_db has a `services/auth_db/lifecycle.rs` provider for canonical auth DB path health and parent-directory readiness/bootstrap; its setup creates only the parent directory and never creates `auth.db` or resolves a key, and agent_trace_db has a `services/agent_trace_db/lifecycle.rs` provider for repository-scoped Agent Trace DB setup and repository DB path health/parent readiness from resolved repository identity, returning an actionable "requires a Git repository" diagnostic outside repository context (no global/checkout fallback path). Doctor runtime aggregates the full shared provider catalog for `diagnose` and `fix` and adapts lifecycle records into doctor-owned output records; setup command aggregates the shared provider catalog for `setup` with hooks included only when requested and adapts lifecycle setup outcomes before rendering setup-owned messages. Agent Trace lifecycle setup now resolves repository storage and initializes `/sce/repos//agent-trace.db` via `RepositoryAgentTraceDb`; hook runtime shares the identity/path resolution but opens through a separate no-migration path and never migrates (missing or stale schema yields `Run 'sce setup'.` guidance). The CLI now also includes a shared text styling service in `cli/src/services/style.rs` that provides deterministic color enablement via `owo-colors`, automatic TTY detection, and `NO_COLOR` compliance for human-facing text output; stdout help/text surfaces, stderr diagnostics, and interactive prompt-adjacent text now reuse that shared styling policy while JSON, completion, and other non-interactive/machine-readable flows remain unstyled. The service exports color-detection, conditional styling, help/diagnostic/label/prompt styling, and `banner_with_gradient()` helpers for use across command surfaces while preserving pipe-safe output for non-interactive environments. The `setup` command includes an `inquire`-backed target-selection flow: default interactive selection for OpenCode/Claude/Pi/Codex/All with required-hook installation in the same run, explicit non-interactive target flags (`--opencode`, `--claude`, `--pi`, `--codex`, `--all`), standalone `--bootstrap-context` for additive durable-context baseline creation without integration installs, deterministic mutually-exclusive validation, and non-destructive cancellation exits; the former `--both` flag was removed in favor of `--all` (opencode+claude+pi+codex). Every normal successful setup path also ensures the same context baseline after the Git gate. Workflows the catalog marks optional are installed only when a repository opts in: the repeatable `sce setup --workflow ` flag names the selection for a run, an omitted flag reuses the selection persisted in `integrations.optional_workflows`, and the resolved selection filters the installed target assets and is written back to repo-local config. Interactive runs ask for the selection instead: a multi-select prompt follows target selection with every row unchecked on a first run and pre-checked from the persisted selection afterwards, cancelling it exits non-destructively like the target prompt, and the prompt is skipped when the catalog marks no workflow optional. `brownfield` is currently the only optional workflow, so a default run installs the five core workflows and no brownfield assets. `sce doctor` scopes its integration checks to that same recorded selection, so it never reports an unselected optional workflow's files as missing. diff --git a/context/plans/remove-telemetry-readiness-marker.md b/context/plans/remove-telemetry-readiness-marker.md index fad67e47a..517f114c7 100644 --- a/context/plans/remove-telemetry-readiness-marker.md +++ b/context/plans/remove-telemetry-readiness-marker.md @@ -296,7 +296,7 @@ The header helper follows the same policy gate and destination tripwire. It then - Context impact: localized. Telemetry readiness lookups no longer migrate an outdated or schemaless `auth.db`. They report `credential_unavailable` (`Unusable`) instead, and a later interactive open still migrates. This affects `context/sce/auth-db.md` (concurrency and schema readiness), and `shared-turso-db.md`/`glossary.md` (the new non-migrating encrypted existing-DB open). - Context synchronization: synced -- [ ] T03: `Gate runtime, doctor and header helper on auth.db instead of the marker` (status:todo) +- [x] T03: `Gate runtime, doctor and header helper on auth.db instead of the marker` (status:done) - Task ID: T03 - Scope: In — remove every marker read. Out — marker writes and clears in login and logout, and deleting the module (both in T04). - Files and functions: @@ -321,7 +321,26 @@ The header helper follows the same policy gate and destination tripwire. It then - A new `legacy_readiness_marker_is_ignored` table test with marker cases (absent, valid, expired, malformed) crossed with credential cases (valid, none). It asserts the doctor label matches the credential case only, and that the marker bytes are unchanged. - The existing 32-process concurrent hook-span test and the 32-process missing-DB helper test pass without seeding the marker. - `nix develop -c ./scripts/run-cli-cargo.sh test --manifest-path cli/Cargo.toml managed_telemetry`, `... telemetry_headers`, `... doctor`, and `... multiprocess` pass. - - Context synchronization: pending + - Completed: 2026-10-21 + - Files changed: + - `cli/src/services/managed_telemetry.rs` + - `cli/src/services/doctor/render.rs` + - `cli/src/services/doctor/managed_telemetry.rs` + - `cli/src/services/auth_command/mod.rs` + - `cli/src/services/telemetry_readiness.rs` + - `cli/src/services/auth_db/multiprocess_tests.rs` + - `cli/src/app/doctor_reconciliation_cli_tests.rs` + - Result: `RuntimeTelemetry::resolve_when_ready` is renamed to `resolve_when_enabled` and loses its `ready` probe: once the policy is enabled, the typed lookup runs directly. A missing `auth.db` classifies as `Missing` → `not_configured` without key resolution. `inspect_health_with(enabled, auth_db_present, lookup, now)` loses the readiness parameter and its `NotConfigured`/`Expired` short-circuits. It probes `auth_db_present` only when the policy is disabled, and otherwise runs the typed lookup under `CREDENTIAL_LOOKUP_DEADLINE`. The new `managed_telemetry::auth_db_present()` (`auth_db_path().is_ok_and(|p| p.is_file())`) replaces `readiness_hint_present`. `ManagedTelemetryHealth::DisabledByConfig { auth_db_present }` replaces `readiness_hint_present` in doctor rendering and JSON tests; the note text is unchanged. In `telemetry_headers_with`, `ready_probe` becomes `auth_db_probe`, wired to `auth_db_present`; the order stays policy → tripwire → probe → repository → lookup. The now-unused `current_unix_seconds` was removed from `auth_command/mod.rs`. `telemetry_readiness.rs` keeps only `mark_ready`, `clear`, and their private helpers. `ReadinessState`, `is_ready`, `state`, `state_at`, `is_ready_at`, and the module's tests are deleted. + - Verify: + - New `valid_credential_without_readiness_marker_resolves_managed_telemetry_and_headers`: seed, bind, remove the marker, then a `hook-resolve` child running `RuntimeTelemetry::resolve_when_enabled` reports `credential_valid` (`Managed`, or `exporter_init_failed`, which is reached only after a `Valid` lookup; the Nix build sandbox has no platform certificates for the rustls client), doctor reports `healthy`, and the helper serves the bearer for the bound repository: passed. + - New `legacy_readiness_marker_is_ignored`: markers absent, valid, expired, and malformed, crossed with a valid credential (`healthy`/`credential_valid`) and a deleted telemetry row (`not_configured`/`not_configured`). Marker bytes are unchanged afterwards: passed. + - `doctor_telemetry_health_classifies_real_auth_db_states_without_mutating_them` now varies credential state: valid → `healthy`; wrong or missing key → `database_error`; expired row → `credential_expired`; deleted row → `not_configured`. The `auth.db` bytes are unchanged by each health check: passed. `claude_hook_with_telemetry_disabled_by_config_never_resolves_the_credential` no longer asserts the marker exists: passed. + - The header table test renames `not ready` to `no auth db` and asserts that the repository is never resolved when the probe fails: passed. + - The 32-process missing-DB helper test and the hook-entry span test pass without the marker. The hook-entry span test never seeded it. + - `managed_telemetry` (42), `telemetry_headers` (11), `doctor` (35), `multiprocess` (23), `telemetry_credential` (27), `stalled` (10), and `auth_command` (20) all passed. `clippy --all-targets -D warnings` is clean. + - Deviation: `resolve_when_ready` was renamed to `resolve_when_enabled` because it no longer takes a readiness probe. The multiprocess tests write a legacy marker through a `LEGACY_READINESS_MARKER = "telemetry-ready.json"` fixture constant, which AC7's `rg` pattern will match. T04 or final validation must accept or exclude that test fixture. The AC2 test cannot drop it. The hook-resolve child counts `exporter_init_failed` as a valid credential, because exporter construction depends on the environment (the Nix check sandbox cannot build the rustls client), while T03 concerns only the readiness decision. `nix flake check` passes. + - Context impact: localized. When the policy is enabled, the runtime, doctor, and header helper no longer read `telemetry-ready.json`. Readiness comes only from the `auth.db` typed lookup, and the helper checks only whether `auth.db` exists. Doctor's `OPT_IN_NOTE` now keys off whether `auth.db` exists. This affects `context/sce/telemetry-credential.md` (Runtime use, Doctor health check, Hidden header helper), `context/cli/access-policy.md` (managed telemetry, doctor, and helper enforcement), and `context/sce/cli-observability-contract.md` (the startup paragraph). Login and logout still write and clear the marker until T04. + - Context synchronization: synced - [ ] T04: `Remove readiness marker writes from login and logout and delete the module` (status:todo) - Task ID: T04 diff --git a/context/sce/cli-observability-contract.md b/context/sce/cli-observability-contract.md index ce6cc5318..42c4f60ff 100644 --- a/context/sce/cli-observability-contract.md +++ b/context/sce/cli-observability-contract.md @@ -95,7 +95,7 @@ Runtime observability consumes the shared resolved observability config from `cl ## Managed trace export -- Defined in `cli/src/services/managed_telemetry.rs` and `cli/src/services/managed_telemetry/trace_export.rs` (plus `log_export.rs`). After startup, `RuntimeTelemetry::resolve` returns Noop (`disabled_by_config`) before any hint or auth DB access when the effective access policy's telemetry state is not `Enabled` ([access-policy.md](../cli/access-policy.md)); otherwise it returns Noop (`not_configured`) without opening the auth DB unless the non-secret telemetry readiness hint is present ([telemetry-credential.md](telemetry-credential.md)). Otherwise it loads the stored telemetry credential from the encrypted auth DB, with non-interactive credential-store access, in a spawned task bounded by `CREDENTIAL_LOOKUP_DEADLINE` (2s); absent, expired, malformed, unavailable, ownerless (no stored WorkOS session), or timed-out credentials select `NoopTelemetry`. Startup never issues or refreshes a credential. The fallback is not silent: `RuntimeTelemetry::resolve` logs one `sce.telemetry.fallback` record with `reason` = `disabled_by_config`, `not_configured`, `credential_inaccessible`, `credential_unavailable`, `credential_expired`, `lookup_timeout`, `database_error`, `path_resolution_error`, or `exporter_init_failed`, at forced `warn` for failures and `debug` otherwise. Database and path errors add the redacted storage error text as `error`; no credential, bearer, key, or payload is logged, and the record never reaches stdout or, with `log_to_file=true`, stderr. +- Defined in `cli/src/services/managed_telemetry.rs` and `cli/src/services/managed_telemetry/trace_export.rs` (plus `log_export.rs`). After startup, `RuntimeTelemetry::resolve` returns Noop (`disabled_by_config`) before any auth DB access when the effective access policy's telemetry state is not `Enabled` ([access-policy.md](../cli/access-policy.md)); otherwise a missing `auth.db` returns Noop (`not_configured`) without resolving a key ([telemetry-credential.md](telemetry-credential.md)). Otherwise it loads the stored telemetry credential from the encrypted auth DB, with non-interactive credential-store access, in a spawned task bounded by `CREDENTIAL_LOOKUP_DEADLINE` (2s); absent, expired, malformed, unavailable, ownerless (no stored WorkOS session), or timed-out credentials select `NoopTelemetry`. Startup never issues or refreshes a credential. The fallback is not silent: `RuntimeTelemetry::resolve` logs one `sce.telemetry.fallback` record with `reason` = `disabled_by_config`, `not_configured`, `credential_inaccessible`, `credential_unavailable`, `credential_expired`, `lookup_timeout`, `database_error`, `path_resolution_error`, or `exporter_init_failed`, at forced `warn` for failures and `debug` otherwise. Database and path errors add the redacted storage error text as `error`; no credential, bearer, key, or payload is logged, and the record never reaches stdout or, with `log_to_file=true`, stderr. - Admission (`managed_span_kind` in `cli/src/app.rs`, decided from argv before credential lookup): Claude-invoked SCE hooks resolve the `sce.hook` span kind, every other hook and the hidden `sce auth telemetry-headers` helper use `NoopTelemetry` without a credential lookup, and all remaining commands use the `sce.command` span kind. `sce hooks claude-mutation-scope` and `sce hooks claude-model-state` are admitted from argv alone. The shared `sce hooks diff-trace`, `sce hooks conversation-trace`, and `sce policy bash` are admitted only when both `CLAUDECODE` and `CLAUDE_PROJECT_DIR` are present in the environment (checked for presence only; values never read); `CLAUDE_PROJECT_DIR` is set for Claude command hooks but not for the Claude Bash tool (evidence: [claude-hook-trace-propagation.md](claude-hook-trace-propagation.md)). Other hooks (commit hooks, Codex, OpenCode, Pi) stay Noop. Admitted hooks use the same `CREDENTIAL_LOOKUP_DEADLINE`, `SHUTDOWN_TIMEOUT`, and runtime grace bounds as commands. Decision record: [2026-10-17-claude-hook-managed-trace-admission.md](../decisions/2026-10-17-claude-hook-managed-trace-admission.md). - Export is OTLP/HTTP protobuf to the hardcoded `https://otel.crocoderlab.dev/v1/traces`. A pinned HTTP client sets `Authorization: Bearer ` (token only, never the accessor), disables redirects, rebuilds the request to the fixed endpoint, and bounds requests to 3s. Ambient `OTEL_EXPORTER_OTLP_*` values cannot redirect it. - Each command emits one `sce.command` span with `sce.command.name`, `sce.outcome` (`success`/`failure`), and `sce.error.class` (the `SCE-ERR-*` code on failure); resource attributes are `service.name=sce` and `service.version`. Raw arguments, paths, and error chains are not exported. diff --git a/context/sce/telemetry-credential.md b/context/sce/telemetry-credential.md index 52105fc37..676a3fb3f 100644 --- a/context/sce/telemetry-credential.md +++ b/context/sce/telemetry-credential.md @@ -28,44 +28,39 @@ flowchart LR - The runtime and doctor map `Missing` to `not_configured`, `Expired` to `credential_expired`, and `Unusable` to `credential_unavailable`. Repository lookups and enrollment accept only `Valid`. - Ownership: every lookup decodes the stored WorkOS session's access-token claims and returns the credential only when `(sub, workspace_id)` equals the identity stored with it. Switching user or workspace therefore makes an older credential unusable even if it was never deleted. A new device-authorization login also deletes the stored telemetry credential before saving the new WorkOS session (`replace_session_with`), so a failed re-issuance leaves no credential; if that delete fails, the new session is not saved. Renewal of the same session keeps the credential. - Repository bindings: `telemetry_repository_bindings` maps a repository ID (the canonical `repository_identity` hash, from `agent_trace.repository_id` or the configured remote) to the `(user_id, workspace_id)` it was enrolled under. Enrollment requires a valid, session-owned credential and records that credential's identity. `sce setup` no longer enrolls repositories, because it must not open the encrypted auth DB or touch the OS credential store. The explicit rebinding mode (`Enroll`) currently has no production caller. A successful `sce auth login` enrolls the working-directory repository only if it is unbound (`EnrollIfUnbound`), so a login never silently moves a repository to another workspace. Bindings survive logout and rotation; they authorize nothing without a matching credential. -- Telemetry readiness hint: `cli/src/services/telemetry_readiness.rs` keeps a non-secret file, `/sce/telemetry-ready.json` (`{"expires_at_unix_seconds": N}`), next to `auth.db`. - - `sce auth login` writes it only after the credential has been issued and persisted. If provisioning, persistence, or writing the hint fails, the hint is cleared and the result is `unavailable`. - - Logout and a new device-authorization session clear it before anything else. - - It counts as ready only while the file parses, is unexpired, and `auth.db` exists. A missing, corrupt, expired, or orphaned hint means telemetry is disabled. `telemetry_readiness::state` reports the same check as `NotConfigured` (no `auth.db`, or a missing or corrupt hint), `Expired`, or `Ready`, without opening `auth.db`. `is_ready` is `state == Ready`. - - It is a fast-path hint, not authorization. The credential is still validated from the auth DB. Its purpose is that unauthenticated hooks never open `auth.db` or touch the Keychain, including existing installs that have `auth.db` but no hint, until the next `sce auth login`. +- Legacy readiness file: `sce auth login` still writes a non-secret `/sce/telemetry-ready.json` after a credential is issued and persisted, and logout and a new device-authorization session still clear it. No runtime, doctor, or header-helper path reads it. Whether it is present, missing, expired, or malformed changes no readiness result; `auth.db` is the only source of truth. - Concurrent saves are single-statement atomic replacements of one fixed row; the last complete writer wins. - `sce auth logout` deletes the WorkOS credential and the telemetry credential, attempting both even when one fails and surfacing the first storage error. `credentials_removed` is true when either record was deleted; output fields are otherwise unchanged. - Telemetry save/delete never touches the WorkOS `auth_credentials` row. ## Runtime use -- `managed_telemetry::RuntimeTelemetry::resolve` first checks the effective access policy ([access-policy.md](../cli/access-policy.md)). When telemetry is not `Enabled`, the result is Noop with reason `disabled_by_config` (logged at `debug`), and neither the readiness hint, `auth.db`, nor the credential store is touched. Otherwise it checks the readiness hint. Without it, the result is Noop with reason `not_configured` (logged at `debug`), and the auth DB is not opened. With the hint, it reads the stored credential through the non-interactive credential-store path at CLI startup (never issuing one). If the store refuses silent access, the reason is `credential_inaccessible` (logged at `debug`, so a denied Keychain does not flood the log). `sce setup` is excluded from managed telemetry so that it never reaches the credential store. a valid unexpired `token` is used only as the `Authorization: Bearer` credential for managed OTLP trace and log export; the `accessor` is never transmitted. Absent, expired, malformed, or unavailable rows select `NoopTelemetry`. Every Noop fallback is classified (`disabled_by_config`, `not_configured`, `credential_inaccessible`, `credential_unavailable`, `credential_expired`, `lookup_timeout`, `database_error`, `path_resolution_error`, `exporter_init_failed`) and logged locally as `sce.telemetry.fallback`; failures log at `warn` (forced, so they reach the log file at the default `error` level, never stderr while `log_to_file=true`), with the redacted `TokenStorageError` text for database and path errors, and policy-disabled telemetry, a missing hint, an inaccessible credential store, or missing or expired credentials log at `debug`. Concurrent lookups from many hook processes are supported by the auth DB's multiprocess WAL and migration-free current-schema open ([auth-db.md](auth-db.md)). Both this lookup and the header helper's repository lookup are existing-DB reads: a missing `auth.db` yields no credential and creates no DB file or encryption key. Claude-invoked SCE hooks are admitted to this lookup and export one `sce.hook` span. Every other hook and this hidden header helper skip the lookup and stay Noop. See [cli-observability-contract.md](cli-observability-contract.md). +- `managed_telemetry::RuntimeTelemetry::resolve` first checks the effective access policy ([access-policy.md](../cli/access-policy.md)). When telemetry is not `Enabled`, the result is Noop with reason `disabled_by_config` (logged at `debug`), and neither `auth.db` nor the credential store is touched. Otherwise it runs the typed lookup through the non-interactive credential-store path at CLI startup (never issuing one). A missing `auth.db` is `Missing`, so the result is Noop with reason `not_configured` (logged at `debug`) without resolving a key; an existing `auth.db` resolves its key non-interactively and is opened without migrating ([auth-db.md](auth-db.md)). If the store refuses silent access, the reason is `credential_inaccessible` (logged at `debug`, so a denied Keychain does not flood the log). `sce setup` is excluded from managed telemetry so that it never reaches the credential store. a valid unexpired `token` is used only as the `Authorization: Bearer` credential for managed OTLP trace and log export; the `accessor` is never transmitted. Absent, expired, malformed, or unavailable rows select `NoopTelemetry`. Every Noop fallback is classified (`disabled_by_config`, `not_configured`, `credential_inaccessible`, `credential_unavailable`, `credential_expired`, `lookup_timeout`, `database_error`, `path_resolution_error`, `exporter_init_failed`) and logged locally as `sce.telemetry.fallback`; failures log at `warn` (forced, so they reach the log file at the default `error` level, never stderr while `log_to_file=true`), with the redacted `TokenStorageError` text for database and path errors, and policy-disabled telemetry, a missing `auth.db` or telemetry row, an inaccessible credential store, or missing or expired credentials log at `debug`. Concurrent lookups from many hook processes are supported by the auth DB's multiprocess WAL and migration-free current-schema open ([auth-db.md](auth-db.md)). Both this lookup and the header helper's repository lookup are existing-DB reads: a missing `auth.db` yields no credential and creates no DB file or encryption key. Claude-invoked SCE hooks are admitted to this lookup and export one `sce.hook` span. Every other hook and this hidden header helper skip the lookup and stay Noop. See [cli-observability-contract.md](cli-observability-contract.md). ## Doctor health check `sce doctor` reports managed telemetry as the last `Environment` row and as a top-level JSON `managed_telemetry` object. `managed_telemetry::inspect_health` computes it once per invocation, and it is read-only: - It never issues a credential, contacts the Control Plane, or starts an OTLP exporter. -- It checks the effective access policy for the inspected repository root first; an unresolvable config counts as not enabled. When telemetry is not `Enabled`, it returns `DisabledByConfig` without reading the readiness hint, opening `auth.db`, or touching the credential store. It only checks whether `telemetry-ready.json` exists, to decide on the opt-in line. -- Otherwise it reads the readiness hint. `NotConfigured` and `Expired` hints return without opening `auth.db` or touching the credential store, so a fresh install never reaches the Keychain. -- With a `Ready` hint it runs the runtime lookup: `load_telemetry_credential` (existing DB only, `CredentialAccess::NonInteractive`) under `CREDENTIAL_LOOKUP_DEADLINE`. Errors are classified by the same `TelemetryFallbackReason` mapping the runtime uses. +- It checks the effective access policy for the inspected repository root first; an unresolvable config counts as not enabled. When telemetry is not `Enabled`, it returns `DisabledByConfig { auth_db_present }` without opening `auth.db` or touching the credential store. It only checks whether `auth.db` exists (a stat), to decide on the opt-in line. +- Otherwise it runs the runtime lookup: `load_telemetry_credential` (existing DB only, `CredentialAccess::NonInteractive`) under `CREDENTIAL_LOOKUP_DEADLINE`. Errors and lookup outcomes are classified by the same `TelemetryFallbackReason` mapping the runtime uses. A missing `auth.db` returns `not_configured` before any key resolution, so a fresh install never reaches the Keychain. - `--fix` adds no repair for it. The problem is `manual_only`, so fix mode reports one `manual` fix result. Doctor never runs `sce auth login`, changes Keychain ACLs, or recreates a key. | Health | Text row | JSON `state` / `reason` | Problem `kind` | `next_action` | | --- | --- | --- | --- | --- | | Telemetry not `Enabled` by policy | `[PASS] Managed telemetry (disabled by config)` | `disabled_by_config` / `disabled_by_config` | none | none | -| No hint, no `auth.db`, or no telemetry row | `[PASS] Managed telemetry (not configured)` | `not_configured` / `not_configured` | none | none | +| No `auth.db`, or no telemetry row | `[PASS] Managed telemetry (not configured)` | `not_configured` / `not_configured` | none | none | | Credential valid | `[PASS] Managed telemetry` | `healthy` / `null` | none | none | | Store refuses silent access | `[WARN]` | `disabled` / `credential_inaccessible` | `managed_telemetry_credential_inaccessible` | `auth_login` | -| Hint ready, unusable row (no WorkOS session, identity mismatch, legacy or malformed) | `[WARN]` | `disabled` / `credential_unavailable` | `managed_telemetry_credential_unavailable` | `auth_login` | -| Hint or credential expired | `[WARN]` | `disabled` / `credential_expired` | `managed_telemetry_credential_expired` | `auth_login` | +| Unusable row (no WorkOS session, identity mismatch, legacy or malformed) or schema not current | `[WARN]` | `disabled` / `credential_unavailable` | `managed_telemetry_credential_unavailable` | `auth_login` | +| Credential expired | `[WARN]` | `disabled` / `credential_expired` | `managed_telemetry_credential_expired` | `auth_login` | | Lookup deadline exceeded | `[WARN]` | `disabled` / `lookup_timeout` | `managed_telemetry_lookup_timeout` | `manual_steps` | | Auth DB open/read failure | `[WARN]` | `disabled` / `database_error` | `managed_telemetry_database_error` | `manual_steps` | | State path unresolvable | `[WARN]` | `disabled` / `path_resolution_error` | `managed_telemetry_path_resolution_error` | `manual_steps` | - The JSON object has `state`, `reason`, `kind`, `severity` (`warning` or `null`), and `next_action`. A warning also appears in `problems` (category `global_state`, severity `warning`, fixability `manual_only`) with the same `next_action`. It never makes doctor `not_ready`. - Every warning says that Claude Code hooks continue working and only telemetry export is disabled. -- When telemetry is disabled by config and `telemetry-ready.json` exists (an earlier login provisioned telemetry), the `disabled by config` row adds one fixed line: `Managed telemetry export is off because telemetry.enabled is not true; set telemetry.enabled: true in the global SCE config to resume it.` +- When telemetry is disabled by config and `auth.db` exists (an earlier login stored credentials), the `disabled by config` row adds one fixed line: `Managed telemetry export is off because telemetry.enabled is not true; set telemetry.enabled: true in the global SCE config to resume it.` - Only macOS `credential_inaccessible` recommends choosing "Always Allow" in the Keychain dialog shown by `sce auth login`. Other platforms name their store (Secret Service, Windows Credential Manager) instead. Expiry, missing credentials, timeouts, and DB or path errors never mention it. - Output uses fixed text only. It never includes `TokenStorageError` details, Keychain error codes, keys, WorkOS tokens, or OpenBao tokens. The detailed reason stays in the local `sce.telemetry.fallback` log event that the same doctor invocation's startup lookup writes. - Code: `cli/src/services/doctor/managed_telemetry.rs` (classification text and JSON) and `cli/src/services/managed_telemetry.rs` (`inspect_health`, `inspect_health_with`). @@ -76,8 +71,8 @@ flowchart LR - Launch context (verified against Claude Code 2.1.284 with a real `claude -p` run): Claude runs the helper with no arguments, with its working directory set to the directory Claude was launched from (possibly a subdirectory), and without `CLAUDE_PROJECT_DIR`. Claude's documentation states that it does not pass `OTEL_*` variables to subprocesses. The helper therefore uses only the working directory, ignores `CLAUDE_PROJECT_DIR` (a value inherited from a parent session could name another project), and never requires an `OTEL_*` variable to be present. - Authorization: the helper walks up to the enclosing Git root, resolves the repository ID with the same precedence as Agent Trace storage, and prints headers only when that repository is bound to the identity of a valid credential that the current WorkOS session owns (see Repository bindings). An unbound repository, a repository bound to another user or workspace, a directory outside Git, or a repository without a resolvable identity gets no token. `.claude/settings.json`, SCE hook markers, and remote names are never treated as proof of ownership. - Destination tripwire: if `OTEL_EXPORTER_OTLP_ENDPOINT`, a per-signal `OTEL_EXPORTER_OTLP_{TRACES,METRICS,LOGS}_ENDPOINT`, or `BETA_TRACING_ENDPOINT` is visible to the helper with a non-canonical value, the helper refuses. This catches visible misconfiguration only. It is not endpoint pinning (see Security boundaries). -- Policy gate: the helper first resolves the effective access policy from its working directory ([access-policy.md](../cli/access-policy.md)). When telemetry is not `Enabled` (including `auth.mode: disabled`, a skipped invalid discovered config, or an unresolvable config), it fails closed before the destination tripwire, the readiness hint, repository resolution, or the auth DB. Repository discovery is Git-root relative, so a launch from a subdirectory applies the same repository policy as a launch from the root. `repository_id_for` uses the same `.git` ancestor walk, so policy and repository identity describe the same root. -- Without the readiness hint the helper fails closed before resolving the repository or opening the auth DB. With it, the credential lookup uses the non-interactive credential-store path and never prompts. +- Policy gate: the helper first resolves the effective access policy from its working directory ([access-policy.md](../cli/access-policy.md)). When telemetry is not `Enabled` (including `auth.mode: disabled`, a skipped invalid discovered config, or an unresolvable config), it fails closed before the destination tripwire, the `auth.db` existence check, repository resolution, or the auth DB. Repository discovery is Git-root relative, so a launch from a subdirectory applies the same repository policy as a launch from the root. `repository_id_for` uses the same `.git` ancestor walk, so policy and repository identity describe the same root. +- The helper then checks only whether `auth.db` exists (a stat). Without it the helper fails closed before resolving the repository, so a never-logged-in machine spawns no `git` process. With it, the credential lookup uses the non-interactive credential-store path and never prompts. - Repository resolution and the credential lookup together are bounded by `CREDENTIAL_LOOKUP_DEADLINE`. Resolution spawns one `git config --get` process, which is needed to compute the repository ID; there is no network call and no issuance. Every failure exits nonzero with the fixed stderr diagnostic `Telemetry credential is unavailable.` and empty stdout. That includes a missing `auth.db`, which the helper never creates or migrates, so concurrent helper calls on a never-logged-in machine cannot race to initialize it. The helper never initializes managed exporter telemetry. - It is omitted from `sce --help`, `sce auth --help`, and shell completions. From d9a561794d0ca52debec27b5353ae2e653b19d9f Mon Sep 17 00:00:00 2001 From: David Abram Date: Wed, 21 Oct 2026 14:10:18 +0200 Subject: [PATCH 4/4] runtime: Remove telemetry readiness marker lifecycle Use the persisted auth.db telemetry credential as the sole readiness source by removing marker writes and clears, and delete the obsolete readiness module and path. Keep logout and session replacement deletion ordering, derive provisioning status from credential persistence, and cover the post-logout lookup state. Plan: remove-telemetry-readiness-marker (T04) Co-authored-by: SCE --- cli/src/services/auth_command/mod.rs | 135 +++--------------- .../services/auth_db/multiprocess_tests.rs | 30 +++- cli/src/services/default_paths.rs | 4 - cli/src/services/mod.rs | 1 - cli/src/services/telemetry_readiness.rs | 59 -------- context/cli/access-policy.md | 4 +- .../remove-telemetry-readiness-marker.md | 73 ++++++++-- context/sce/telemetry-credential.md | 4 +- 8 files changed, 111 insertions(+), 199 deletions(-) delete mode 100644 cli/src/services/telemetry_readiness.rs diff --git a/cli/src/services/auth_command/mod.rs b/cli/src/services/auth_command/mod.rs index 5951f72f3..adcd58a98 100644 --- a/cli/src/services/auth_command/mod.rs +++ b/cli/src/services/auth_command/mod.rs @@ -21,7 +21,6 @@ use crate::services::managed_telemetry::{self, run_with_deadline, CREDENTIAL_LOO use crate::services::output_format::OutputFormat; use crate::services::style::{label, prompt_label, prompt_value, value}; use crate::services::telemetry_credential_storage; -use crate::services::telemetry_readiness; use crate::services::token_storage::{self, StoredTokens, TokenStorageError}; pub const NAME: &str = "auth"; @@ -272,25 +271,21 @@ where pub async fn run_logout(format: AuthFormat) -> Result { run_logout_with( format, - telemetry_readiness::clear, token_storage::delete_tokens(), telemetry_credential_storage::delete_telemetry_credential(), ) .await } -async fn run_logout_with( +async fn run_logout_with( format: AuthFormat, - clear_readiness: C, delete_workos: W, delete_telemetry: T, ) -> Result where - C: FnOnce() -> Result<()>, W: Future>, T: Future>, { - clear_readiness().map_err(unexpected_auth_command_error)?; let workos_result = delete_workos.await; let telemetry_result = delete_telemetry.await; let workos_deleted = workos_result.map_err(auth_storage_error)?; @@ -397,25 +392,21 @@ where async fn save_new_session_tokens(token: &auth::TokenResponse) -> Result { replace_session_with( - telemetry_readiness::clear, telemetry_credential_storage::delete_telemetry_credential(), || token_storage::save_tokens(token, CredentialAccess::Interactive), ) .await } -async fn replace_session_with( - clear_readiness: C, +async fn replace_session_with( invalidate_telemetry: D, save_session: S, ) -> Result where - C: FnOnce() -> Result<()>, D: Future>, S: FnOnce() -> SF, SF: Future>, { - clear_readiness().map_err(unexpected_auth_command_error)?; invalidate_telemetry.await.map_err(login_storage_error)?; save_session().await.map_err(login_storage_error) } @@ -437,34 +428,11 @@ where async fn provision_telemetry_credential( client: &AuthenticatedControlPlaneClient, ) -> TelemetryProvisioningResult { - let provisioned = client.issue_and_save_telemetry_credential().await; - let result = record_telemetry_readiness( - provisioned.as_ref().ok(), - telemetry_readiness::mark_ready, - telemetry_readiness::clear, - ); - if result == TelemetryProvisioningResult::Issued { - enroll_current_repository_if_unbound().await; - } - result -} - -fn record_telemetry_readiness( - provisioned: Option<&telemetry_credential_storage::TelemetryCredential>, - mark_ready: M, - clear: C, -) -> TelemetryProvisioningResult -where - M: FnOnce(u64) -> Result<()>, - C: FnOnce() -> Result<()>, -{ - let recorded = provisioned - .is_some_and(|credential| mark_ready(credential.expires_at_unix_seconds).is_ok()); - if recorded { - return TelemetryProvisioningResult::Issued; + if client.issue_and_save_telemetry_credential().await.is_err() { + return TelemetryProvisioningResult::Unavailable; } - let _ = clear(); - TelemetryProvisioningResult::Unavailable + enroll_current_repository_if_unbound().await; + TelemetryProvisioningResult::Issued } async fn enroll_current_repository_if_unbound() { @@ -860,10 +828,6 @@ mod tests { async fn new_session_invalidates_previous_telemetry_credential_before_saving() { let order = std::sync::Mutex::new(Vec::new()); let saved = replace_session_with( - || { - order.lock().unwrap().push("clear"); - Ok(()) - }, async { order.lock().unwrap().push("invalidate"); Ok(true) @@ -876,17 +840,13 @@ mod tests { .await .expect("replacement should succeed"); assert_eq!(saved, stored_tokens()); - assert_eq!(*order.lock().unwrap(), ["clear", "invalidate", "save"]); + assert_eq!(*order.lock().unwrap(), ["invalidate", "save"]); let save_called = std::sync::atomic::AtomicBool::new(false); - let failed = replace_session_with( - || Ok(()), - async { Err(storage_failure()) }, - || async { - save_called.store(true, std::sync::atomic::Ordering::SeqCst); - Ok(stored_tokens()) - }, - ) + let failed = replace_session_with(async { Err(storage_failure()) }, || async { + save_called.store(true, std::sync::atomic::Ordering::SeqCst); + Ok(stored_tokens()) + }) .await; assert!(!save_called.load(std::sync::atomic::Ordering::SeqCst)); assert_login_storage_guidance( @@ -894,11 +854,9 @@ mod tests { AuthStorageFailure::Database, ); - let failed = replace_session_with( - || Ok(()), - async { Ok(true) }, - || async { Err(TokenStorageError::CredentialAccessUnavailable) }, - ) + let failed = replace_session_with(async { Ok(true) }, || async { + Err(TokenStorageError::CredentialAccessUnavailable) + }) .await; assert_login_storage_guidance( &failed.expect_err("save failure fails login"), @@ -965,7 +923,6 @@ mod tests { async fn logout_storage_failure_keeps_command_storage_guidance() { let error = run_logout_with( AuthFormat::Json, - || Ok(()), async { Err(TokenStorageError::CredentialAccessUnavailable) }, async { Ok(false) }, ) @@ -1265,7 +1222,7 @@ mod tests { } #[tokio::test] - async fn logout_clears_readiness_and_attempts_both_deletions_before_reporting() { + async fn logout_attempts_both_deletions_before_reporting() { use AuthStorageFailure::{CredentialAccess, DatabaseLocked}; fn deletion(result: Result) -> Result { @@ -1288,10 +1245,6 @@ mod tests { let events = std::sync::Mutex::new(Vec::new()); let result = run_logout_with( AuthFormat::Json, - || { - events.lock().unwrap().push("clear-readiness"); - Ok(()) - }, async { events.lock().unwrap().push("delete-workos"); deletion(workos) @@ -1305,7 +1258,7 @@ mod tests { assert_eq!( *events.lock().unwrap(), - ["clear-readiness", "delete-workos", "delete-telemetry"], + ["delete-workos", "delete-telemetry"], "{workos:?} {telemetry:?}" ); match (result, expected) { @@ -1320,56 +1273,6 @@ mod tests { } } - #[test] - fn telemetry_readiness_is_recorded_only_after_credential_persistence() { - let credential = telemetry_credential("tok-ready"); - let marked = std::cell::Cell::new(None); - let cleared = std::cell::Cell::new(false); - - let issued = record_telemetry_readiness( - Some(&credential), - |expires_at| { - marked.set(Some(expires_at)); - Ok(()) - }, - || { - cleared.set(true); - Ok(()) - }, - ); - assert_eq!(issued, TelemetryProvisioningResult::Issued); - assert_eq!(marked.get(), Some(credential.expires_at_unix_seconds)); - assert!(!cleared.get()); - - marked.set(None); - let failed = record_telemetry_readiness( - None, - |expires_at| { - marked.set(Some(expires_at)); - Ok(()) - }, - || { - cleared.set(true); - Ok(()) - }, - ); - assert_eq!(failed, TelemetryProvisioningResult::Unavailable); - assert_eq!(marked.get(), None); - assert!(cleared.get()); - - cleared.set(false); - let unrecorded = record_telemetry_readiness( - Some(&credential), - |_| Err(anyhow!("read-only state directory")), - || { - cleared.set(true); - Ok(()) - }, - ); - assert_eq!(unrecorded, TelemetryProvisioningResult::Unavailable); - assert!(cleared.get()); - } - fn access_policy(mode: AuthMode, source: ValueSource, telemetry: bool) -> ResolvedAccessPolicy { use crate::services::config::{ ResolvedValue, TelemetryDisabledReason, TelemetryPolicyState, @@ -1663,10 +1566,6 @@ mod tests { let events = std::sync::Mutex::new(Vec::new()); let output = run_logout_with( AuthFormat::Json, - || { - events.lock().unwrap().push("clear-readiness"); - Ok(()) - }, async { events.lock().unwrap().push("delete-workos"); Ok(true) @@ -1681,7 +1580,7 @@ mod tests { assert_eq!( *events.lock().unwrap(), - ["clear-readiness", "delete-workos", "delete-telemetry"] + ["delete-workos", "delete-telemetry"] ); assert!(output.contains("\"credentials_removed\": true")); } diff --git a/cli/src/services/auth_db/multiprocess_tests.rs b/cli/src/services/auth_db/multiprocess_tests.rs index 5d1f9efbb..059064518 100644 --- a/cli/src/services/auth_db/multiprocess_tests.rs +++ b/cli/src/services/auth_db/multiprocess_tests.rs @@ -559,6 +559,35 @@ fn valid_credential_without_readiness_marker_resolves_managed_telemetry_and_head assert!(!marker.exists()); } +#[test] +fn logout_then_lookup_reports_not_configured() { + let _fan_out = serialize_fan_out(); + let sandbox = Sandbox::new(); + sandbox.seed(); + assert_eq!(telemetry_health(&sandbox, TEST_SECRET), "healthy"); + + let logout = run_cli( + &sandbox, + sandbox.dir.path(), + &["sce", "auth", "logout", "--format", "json"], + None, + ); + assert!(logout.success, "{}{}", logout.stdout, logout.stderr); + assert!( + logout.stdout.contains("\"credentials_removed\": true"), + "{}", + logout.stdout + ); + + assert!(sandbox.sce_state_dir().join("auth.db").is_file()); + assert_eq!(telemetry_health(&sandbox, TEST_SECRET), "not_configured"); + assert_eq!(hook_telemetry(&sandbox), "not_configured"); + assert!(!sandbox + .sce_state_dir() + .join(LEGACY_READINESS_MARKER) + .exists()); +} + #[test] fn legacy_readiness_marker_is_ignored() { let _fan_out = serialize_fan_out(); @@ -1366,7 +1395,6 @@ async fn seed() { }) .await .expect("telemetry credential seeds"); - crate::services::telemetry_readiness::mark_ready(FAR_FUTURE).expect("readiness seeds"); } fn writer_rows() -> Vec { diff --git a/cli/src/services/default_paths.rs b/cli/src/services/default_paths.rs index d0e42c61d..20ece4ef3 100644 --- a/cli/src/services/default_paths.rs +++ b/cli/src/services/default_paths.rs @@ -254,10 +254,6 @@ pub fn auth_db_path() -> anyhow::Result { Ok(sce_state_dir()?.join("auth.db")) } -pub fn telemetry_readiness_path() -> anyhow::Result { - Ok(sce_state_dir()?.join("telemetry-ready.json")) -} - pub fn agent_trace_db_path_for_repository(repository_id: &str) -> anyhow::Result { let state_root = resolve_sce_default_locations()? .roots() diff --git a/cli/src/services/mod.rs b/cli/src/services/mod.rs index 2f484e8e0..2612c646b 100644 --- a/cli/src/services/mod.rs +++ b/cli/src/services/mod.rs @@ -37,6 +37,5 @@ pub mod structured_patch; pub mod style; pub mod sync; pub mod telemetry_credential_storage; -pub mod telemetry_readiness; pub mod token_storage; pub mod version; diff --git a/cli/src/services/telemetry_readiness.rs b/cli/src/services/telemetry_readiness.rs deleted file mode 100644 index b1e85685f..000000000 --- a/cli/src/services/telemetry_readiness.rs +++ /dev/null @@ -1,59 +0,0 @@ -use std::fs; -use std::io::ErrorKind; -use std::path::{Path, PathBuf}; - -use anyhow::{Context, Result}; -use serde::{Deserialize, Serialize}; - -use crate::services::default_paths::telemetry_readiness_path; - -const TEMP_SUFFIX: &str = "tmp"; - -#[derive(Clone, Copy, Debug, Eq, PartialEq, Serialize, Deserialize)] -struct TelemetryReadiness { - expires_at_unix_seconds: u64, -} - -pub fn mark_ready(expires_at_unix_seconds: u64) -> Result<()> { - mark_ready_at(&telemetry_readiness_path()?, expires_at_unix_seconds) -} - -pub fn clear() -> Result<()> { - clear_at(&telemetry_readiness_path()?) -} - -fn mark_ready_at(path: &Path, expires_at_unix_seconds: u64) -> Result<()> { - let parent = path.parent().with_context(|| { - format!( - "telemetry readiness path '{}' has no parent", - path.display() - ) - })?; - fs::create_dir_all(parent) - .with_context(|| format!("failed to create '{}'", parent.display()))?; - let bytes = serde_json::to_vec(&TelemetryReadiness { - expires_at_unix_seconds, - }) - .context("failed to serialize telemetry readiness")?; - let temp_path = temp_path_for(path); - fs::write(&temp_path, bytes) - .with_context(|| format!("failed to write '{}'", temp_path.display()))?; - fs::rename(&temp_path, path).with_context(|| { - let _ = fs::remove_file(&temp_path); - format!("failed to replace '{}'", path.display()) - }) -} - -fn clear_at(path: &Path) -> Result<()> { - match fs::remove_file(path) { - Ok(()) => Ok(()), - Err(error) if error.kind() == ErrorKind::NotFound => Ok(()), - Err(error) => Err(error).with_context(|| format!("failed to remove '{}'", path.display())), - } -} - -fn temp_path_for(path: &Path) -> PathBuf { - let mut name = path.as_os_str().to_os_string(); - name.push(format!(".{}.{TEMP_SUFFIX}", std::process::id())); - PathBuf::from(name) -} diff --git a/context/cli/access-policy.md b/context/cli/access-policy.md index 232f18f3b..65fac6afa 100644 --- a/context/cli/access-policy.md +++ b/context/cli/access-policy.md @@ -128,9 +128,9 @@ The note documents a coverage limit: telemetry opt-out does not remove Claude-na - **`sce setup` Claude telemetry install:** with the Claude target selected, setup resolves the policy for the setup repository root through `resolve_auth_runtime_config`. A resolution failure counts as not enabled. When telemetry is not `Enabled`, it skips the Claude-native install with the fixed line `Claude telemetry: skipped; SCE telemetry is disabled by config.` and leaves user Claude settings unchanged. See [telemetry credential: Claude-native telemetry](../sce/telemetry-credential.md#claude-native-telemetry). - **`sce auth login` / `sce auth whoami`:** both resolve the policy from the working directory through `resolve_auth_runtime_config` and check `auth_allowed()` before any `token_storage` call. A resolution failure is a command error. - Under `disabled`, both fail with `UserError::AuthDisabledByConfig` (key `auth.disabled_by_config`, runtime class, exit code 4) before `auth.db` or the credential store is touched. The fixed message names `auth.mode: disabled` and the config file of the deciding layer: the global SCE config, the repository `.sce/config.json`, the `--config` file, or the `SCE_CONFIG_FILE` file (`AuthPolicyConfigFile`). It tells the user to set `auth.mode` to `optional` or `mandatory` there. - - Login provisions a telemetry credential only when telemetry is `Enabled`. Otherwise `telemetry_provisioning` is `disabled`, with the fixed message ``Telemetry credential provisioning is disabled by SCE config. Set `telemetry.enabled: true` in the global SCE config to enable it.``, shown in text as a `Note:` line. Disabled provisioning issues nothing, writes no readiness hint, and enrolls no repository. + - Login provisions a telemetry credential only when telemetry is `Enabled`. Otherwise `telemetry_provisioning` is `disabled`, with the fixed message ``Telemetry credential provisioning is disabled by SCE config. Set `telemetry.enabled: true` in the global SCE config to enable it.``, shown in text as a `Note:` line. Disabled provisioning issues nothing and enrolls no repository. - Under `mandatory`, `whoami` without stored credentials fails with `auth.not_authenticated`. Under `optional`, it renders the unauthenticated success report as before. -- **`sce auth logout`:** performs no policy check in any mode, so it still clears the readiness hint and deletes both stored credential rows under `disabled`. +- **`sce auth logout`:** performs no policy check in any mode, so it still deletes both stored credential rows under `disabled`. A later telemetry lookup classifies the missing row as `not_configured`. - **`sce sync`:** resolves the policy from the repository root before it resolves Agent Trace storage or builds the control-plane client. Under `disabled` it fails with `auth.disabled_by_config` (same message and deciding-file naming as login), so `auth.db` and the credential store are never reached. `optional` and `mandatory` keep the existing behavior. See [sync command: error classification](sync-command.md#error-classification). - **Post-commit auto-sync:** the hook launches `sce sync` only when `auto_sync_allowed(agent_trace.auto_sync)` holds, so auth `disabled` or a degraded config suppresses the launch. A suppressed launch is silent apart from a `debug` log, and the hook still succeeds. See [automatic Agent Trace synchronization](agent-trace-auto-sync.md#trigger-boundary). - **`sce doctor` auto-sync row:** `post_commit_auto_sync.enabled` is the effective value (`auto_sync_allowed` of the configured value). When the policy suppresses auto-sync, the row reports state `disabled`. `source` / `config_source` still describe `agent_trace.auto_sync`. diff --git a/context/plans/remove-telemetry-readiness-marker.md b/context/plans/remove-telemetry-readiness-marker.md index 517f114c7..ac8496986 100644 --- a/context/plans/remove-telemetry-readiness-marker.md +++ b/context/plans/remove-telemetry-readiness-marker.md @@ -139,22 +139,18 @@ The header helper follows the same policy gate and destination tripwire. It then ## Acceptance criteria -- [ ] AC1: A valid, owned, unexpired telemetry credential in `auth.db` enables managed telemetry, doctor health, and the header helper when no `telemetry-ready.json` exists. +- [x] AC1: A valid, owned, unexpired telemetry credential in `auth.db` enables managed telemetry, doctor health, and the header helper when no `telemetry-ready.json` exists. - Validate: `nix develop -c ./scripts/run-cli-cargo.sh test --manifest-path cli/Cargo.toml valid_credential_without_readiness_marker` passes, covering runtime `Managed`, doctor `healthy`, and the helper bearer for the bound repository in a sandboxed state root. -- [ ] AC2: `telemetry-ready.json` is inert. A present-but-ready marker with no telemetry row reports `not_configured`. An expired or malformed marker next to a valid credential reports `healthy`. The file is never read, written, or removed. +- [x] AC2: `telemetry-ready.json` is inert. A present-but-ready marker with no telemetry row reports `not_configured`. An expired or malformed marker next to a valid credential reports `healthy`. The file is never read, written, or removed. - Validate: `nix develop -c ./scripts/run-cli-cargo.sh test --manifest-path cli/Cargo.toml legacy_readiness_marker_is_ignored` passes, and asserts the marker bytes are unchanged afterwards. -- [ ] AC3: The readiness classification matches the fallback matrix for a missing `auth.db`, an existing DB with no row, a valid row, an expired row, a missing WorkOS session, an identity mismatch, and a legacy identity-less row. A missing `auth.db` creates no DB file, sidecar, or key. +- [x] AC3: The readiness classification matches the fallback matrix for a missing `auth.db`, an existing DB with no row, a valid row, an expired row, a missing WorkOS session, an identity mismatch, and a legacy identity-less row. A missing `auth.db` creates no DB file, sidecar, or key. - Validate: `nix develop -c ./scripts/run-cli-cargo.sh test --manifest-path cli/Cargo.toml telemetry_credential_lookup_classifies` and `... doctor_telemetry_health` pass. -- [ ] AC4: Readiness lookups never migrate or initialize `auth.db`. Concurrent hook lookups against an outdated-schema `auth.db` leave its bytes and `__sce_migrations` unchanged, and each reports `credential_unavailable`. +- [x] AC4: Readiness lookups never migrate or initialize `auth.db`. Concurrent hook lookups against an outdated-schema `auth.db` leave its bytes and `__sce_migrations` unchanged, and each reports `credential_unavailable`. - Validate: `nix develop -c ./scripts/run-cli-cargo.sh test --manifest-path cli/Cargo.toml readiness_lookup_never_migrates` passes. -- [ ] AC5: Telemetry disabled by policy never reaches the credential lookup, even with a valid credential stored. Doctor shows `OPT_IN_NOTE` only when `auth.db` exists. +- [x] AC5: Telemetry disabled by policy never reaches the credential lookup, even with a valid credential stored. Doctor shows `OPT_IN_NOTE` only when `auth.db` exists. - Validate: the policy-disabled tests in `managed_telemetry.rs`, `auth_command/mod.rs`, `doctor/render.rs`, and `claude_hook_with_telemetry_disabled_by_config_never_resolves_the_credential` pass. -- [ ] AC6: Login reports `issued` exactly when the credential is persisted, `unavailable` on issuance or persistence failure (keeping the prior row on the stored and renewed paths), and `disabled` with no issuance when telemetry is off. Logout deletes both rows, and a following lookup reports `not_configured`. +- [x] AC6: Login reports `issued` exactly when the credential is persisted, `unavailable` on issuance or persistence failure (keeping the prior row on the stored and renewed paths), and `disabled` with no issuance when telemetry is off. Logout deletes both rows, and a following lookup reports `not_configured`. - Validate: the `auth_command/mod.rs` login and logout tests pass, and `nix develop -c ./scripts/run-cli-cargo.sh test --manifest-path cli/Cargo.toml logout_then_lookup_reports_not_configured` passes. -- [ ] AC7: No production code, test, or current-state context references the marker. - - Validate: `nix shell nixpkgs#ripgrep -c rg -n "telemetry-ready|telemetry_readiness|ReadinessState|mark_ready|readiness_hint" cli/ context/ --glob '!context/plans/**' --glob '!context/decisions/**' --glob '!context/handovers/**'` returns no matches. -- [ ] AC8: On macOS, an admitted Claude hook and `sce auth telemetry-headers` with an unauthorized or locked Keychain item show no dialog and resolve `credential_inaccessible` / fail closed. A previously authorized install keeps exporting after the marker file is deleted. - - Validate: manual inspection on macOS: delete `~/Library/Application Support/sce/telemetry-ready.json` (or the platform state root), then run a Claude session and `sce doctor --format json` and check that `managed_telemetry.state` is `healthy` with no prompt. Then lock the login keychain and rerun: no dialog, and `credential_inaccessible`. ### Full validation @@ -342,7 +338,7 @@ The header helper follows the same policy gate and destination tripwire. It then - Context impact: localized. When the policy is enabled, the runtime, doctor, and header helper no longer read `telemetry-ready.json`. Readiness comes only from the `auth.db` typed lookup, and the helper checks only whether `auth.db` exists. Doctor's `OPT_IN_NOTE` now keys off whether `auth.db` exists. This affects `context/sce/telemetry-credential.md` (Runtime use, Doctor health check, Hidden header helper), `context/cli/access-policy.md` (managed telemetry, doctor, and helper enforcement), and `context/sce/cli-observability-contract.md` (the startup paragraph). Login and logout still write and clear the marker until T04. - Context synchronization: synced -- [ ] T04: `Remove readiness marker writes from login and logout and delete the module` (status:todo) +- [x] T04: `Remove readiness marker writes from login and logout and delete the module` (status:done) - Task ID: T04 - Scope: In — the login, logout, and session-replacement lifecycle, plus deleting the marker module and path. Out — any change to provisioning conditions, enrollment, policy gates, or storage APIs. - Files and functions: @@ -373,9 +369,62 @@ The header helper follows the same policy gate and destination tripwire. It then - `... logout_then_lookup_reports_not_configured` passes. - `nix shell nixpkgs#ripgrep -c rg -n "telemetry_readiness|telemetry-ready|mark_ready" cli/` returns nothing. - `nix flake check` passes. - - Context synchronization: pending + - Completed: 2026-10-21 + - Files changed: + - `cli/src/services/auth_command/mod.rs` + - `cli/src/services/auth_db/multiprocess_tests.rs` + - `cli/src/services/default_paths.rs` + - `cli/src/services/mod.rs` + - `cli/src/services/telemetry_readiness.rs` (deleted) + - Result: `provision_telemetry_credential` returns `Unavailable` when `issue_and_save_telemetry_credential` fails. Otherwise it enrolls the working-directory repository (`EnrollIfUnbound`) and returns `Issued`. `record_telemetry_readiness` is deleted. `run_logout_with` and `replace_session_with` no longer take `clear_readiness`, so logout runs delete-WorkOS → delete-telemetry and session replacement runs invalidate → save. The `telemetry_readiness` module, its `mod.rs` entry, `default_paths::telemetry_readiness_path`, and the `mark_ready` call in the multiprocess `seed()` are removed. No path-catalog test listed the readiness path. + - Verify: + - `nix develop -c ./scripts/run-cli-cargo.sh test --manifest-path cli/Cargo.toml auth_command`: 19 passed. The event-order assertions are now `["invalidate", "save"]` and `["delete-workos", "delete-telemetry"]`, and `logout_clears_readiness_and_attempts_both_deletions_before_reporting` is renamed `logout_attempts_both_deletions_before_reporting`. + - New `logout_then_lookup_reports_not_configured` (multiprocess). It seeds a real sandboxed `auth.db` (doctor reports `healthy`), runs `sce auth logout --format json` through the CLI child (`credentials_removed: true`), and checks that `auth.db` still exists, that doctor and the hook resolver then report `not_configured`, and that no marker file exists: passed. + - `nix shell nixpkgs#ripgrep -c rg -n "telemetry_readiness|telemetry-ready|mark_ready" cli/`: the only match is `multiprocess_tests.rs:51`, `LEGACY_READINESS_MARKER = "telemetry-ready.json"`. That is the AC2 fixture from T03, accepted as described below. + - The additional `multiprocess` (24), `managed_telemetry` (42), `telemetry_credential` (27), `telemetry_headers` (11), and `doctor` (35) filters passed. + - `nix flake check`: all checks passed (clippy, fmt, tests). + - Deviation: `telemetry_readiness_is_recorded_only_after_credential_persistence` is deleted, not replaced. Without a marker, the provisioning result maps directly from the `Result` of `issue_and_save_telemetry_credential` and needs no test seam (plan step 2). The `LEGACY_READINESS_MARKER` test fixture remains, as T03 noted, because the AC2 `legacy_readiness_marker_is_ignored` test writes that file to prove it is inert. AC7's `rg` must exclude or accept that one fixture line. + - Context impact: localized. Login no longer writes the readiness marker, and logout and session replacement no longer clear it. The login result reflects only whether the credential was saved. No marker file, module, or path remains in production code. This affects `context/sce/telemetry-credential.md` (the readiness hint section and the login and logout lifecycle) and `context/cli/access-policy.md` (the login and logout enforcement bullets). + - Context synchronization: synced ## Open questions - On Linux, the keyring adapter has no non-interactive switch, so a locked Secret Service collection might show an unlock prompt. This limitation already exists. Removing the marker means more hook invocations reach Secret Service: every admitted hook while `auth.db` exists and telemetry is enabled, instead of only marker-ready installs. Is this acceptable for this plan? The smaller and safer alternative is a separate follow-up that adds a Linux non-interactive guard. Folding that into this plan would mean changing the Keychain adapter, which is listed as out of scope. - `OPT_IN_NOTE` now keys off whether `auth.db` exists, not off the marker, so some users who never provisioned telemetry will also see it: those who logged in while telemetry was disabled, and those with a legacy setup-created `auth.db`. Keep the note as it is, or drop it entirely? Dropping it would also remove the `auth_db_present` probe. + +## Validation Report + +**Status:** validated +**Date:** 2026-10-21 + +### Commands run + +- `nix flake check` -> exit 0 (all checks passed; x86_64-linux only, darwin systems omitted) +- `nix develop -c ./scripts/run-cli-cargo.sh test --manifest-path cli/Cargo.toml valid_credential_without_readiness_marker` -> exit 0 (1 passed) +- `nix develop -c ./scripts/run-cli-cargo.sh test --manifest-path cli/Cargo.toml legacy_readiness_marker_is_ignored` -> exit 0 (1 passed) +- `nix develop -c ./scripts/run-cli-cargo.sh test --manifest-path cli/Cargo.toml telemetry_credential_lookup_classifies` -> exit 0 (1 passed) +- `nix develop -c ./scripts/run-cli-cargo.sh test --manifest-path cli/Cargo.toml doctor_telemetry_health` -> exit 0 (2 passed) +- `nix develop -c ./scripts/run-cli-cargo.sh test --manifest-path cli/Cargo.toml readiness_lookup_never_migrates` -> exit 0 (1 passed) +- `nix develop -c ./scripts/run-cli-cargo.sh test --manifest-path cli/Cargo.toml logout_then_lookup_reports_not_configured` -> exit 0 (1 passed) +- `nix develop -c ./scripts/run-cli-cargo.sh test --manifest-path cli/Cargo.toml auth_command` -> exit 0 (19 passed) +- `nix develop -c ./scripts/run-cli-cargo.sh test --manifest-path cli/Cargo.toml disabled_by_config` -> exit 0 (10 passed) + +### Success-criteria verification + +- [x] AC1: Valid credential enables runtime, doctor, and header helper without the marker -> `valid_credential_without_readiness_marker_resolves_managed_telemetry_and_headers` passed +- [x] AC2: `telemetry-ready.json` is inert -> `legacy_readiness_marker_is_ignored` passed (asserts marker bytes unchanged) +- [x] AC3: Classification matches the fallback matrix; missing `auth.db` creates nothing -> `telemetry_credential_lookup_classifies_auth_db_states` and both `doctor_telemetry_health*` tests passed +- [x] AC4: Readiness lookups never migrate -> `readiness_lookup_never_migrates_outdated_auth_db` passed +- [x] AC5: Disabled policy never reaches the lookup; `OPT_IN_NOTE` only when `auth.db` exists -> `disabled_by_config` filter (10 tests across managed_telemetry, doctor render, auth_command, multiprocess, doctor CLI) passed +- [x] AC6: Login/logout lifecycle -> `auth_command` (19) and `logout_then_lookup_reports_not_configured` passed + +### Failed checks and follow-ups + +- None. + +### Residual risks + +- Linux Secret Service may prompt on more hook invocations now that `auth.db` presence admits the lookup (plan Open question 1, unresolved). +- `OPT_IN_NOTE` now appears for users with an `auth.db` who never provisioned telemetry (plan Open question 2, unresolved). +- macOS Keychain no-dialog behavior was not manually verified; the former AC8 was removed from the plan. +- `nix flake check` covered x86_64-linux only. diff --git a/context/sce/telemetry-credential.md b/context/sce/telemetry-credential.md index 676a3fb3f..9de9fb4ec 100644 --- a/context/sce/telemetry-credential.md +++ b/context/sce/telemetry-credential.md @@ -8,7 +8,7 @@ The CLI holds one telemetry-only OpenBao credential, issued by the Control Plane - Identity: the credential is bound to the `(sub, workspace_id)` claims of the exact WorkOS access token sent with the issuance request, the same verified claims Control Plane reads into `CurrentUser` to mint the token. If that bearer has no complete identity (no `workspace_id`, or an undecodable token), issuance fails with the fixed invalid-response error and nothing is saved. - Save-on-issue: `issue_and_save_telemetry_credential` persists only after successful issuance, so any failure leaves previously stored state intact. - Storage: `cli/src/services/telemetry_credential_storage.rs` (`save_telemetry_credential`, `load_telemetry_credential`, `delete_telemetry_credential`) over the shared `AuthDb`; schema in [auth-db.md](auth-db.md). -- Auth login provisioning: when the effective access policy enables telemetry, every successful `sce auth login` path — fresh device authorization, valid stored credentials, and renewed stored credentials — invokes `AuthenticatedControlPlaneClient::issue_and_save_telemetry_credential` after WorkOS persistence. The deterministic `telemetry_provisioning` result reports `issued`, `unavailable`, or `disabled`; unavailable provisioning preserves WorkOS authentication success and gives text callers a fixed secret-free retry warning. When telemetry is not enabled the result is `disabled` with a fixed note naming `telemetry.enabled`, and login issues no credential, writes no readiness hint, and enrolls no repository (see [effective access policy](../cli/access-policy.md#enforcement)). +- Auth login provisioning: when the effective access policy enables telemetry, every successful `sce auth login` path — fresh device authorization, valid stored credentials, and renewed stored credentials — invokes `AuthenticatedControlPlaneClient::issue_and_save_telemetry_credential` after WorkOS persistence. The deterministic `telemetry_provisioning` result reports `issued`, `unavailable`, or `disabled`; unavailable provisioning preserves WorkOS authentication success and gives text callers a fixed secret-free retry warning. When telemetry is not enabled the result is `disabled` with a fixed note naming `telemetry.enabled`, and login issues no credential and enrolls no repository (see [effective access policy](../cli/access-policy.md#enforcement)). ## Lifecycle @@ -28,7 +28,7 @@ flowchart LR - The runtime and doctor map `Missing` to `not_configured`, `Expired` to `credential_expired`, and `Unusable` to `credential_unavailable`. Repository lookups and enrollment accept only `Valid`. - Ownership: every lookup decodes the stored WorkOS session's access-token claims and returns the credential only when `(sub, workspace_id)` equals the identity stored with it. Switching user or workspace therefore makes an older credential unusable even if it was never deleted. A new device-authorization login also deletes the stored telemetry credential before saving the new WorkOS session (`replace_session_with`), so a failed re-issuance leaves no credential; if that delete fails, the new session is not saved. Renewal of the same session keeps the credential. - Repository bindings: `telemetry_repository_bindings` maps a repository ID (the canonical `repository_identity` hash, from `agent_trace.repository_id` or the configured remote) to the `(user_id, workspace_id)` it was enrolled under. Enrollment requires a valid, session-owned credential and records that credential's identity. `sce setup` no longer enrolls repositories, because it must not open the encrypted auth DB or touch the OS credential store. The explicit rebinding mode (`Enroll`) currently has no production caller. A successful `sce auth login` enrolls the working-directory repository only if it is unbound (`EnrollIfUnbound`), so a login never silently moves a repository to another workspace. Bindings survive logout and rotation; they authorize nothing without a matching credential. -- Legacy readiness file: `sce auth login` still writes a non-secret `/sce/telemetry-ready.json` after a credential is issued and persisted, and logout and a new device-authorization session still clear it. No runtime, doctor, or header-helper path reads it. Whether it is present, missing, expired, or malformed changes no readiness result; `auth.db` is the only source of truth. +- Readiness source: `auth.db` is the only source of truth for whether a usable telemetry credential exists. Provisioning reports `issued` exactly when the credential was issued and persisted, then enrolls the working-directory repository; issuance or persistence failure reports `unavailable` and writes nothing. No separate readiness file exists; a legacy readiness file left in the state root by an older binary is never read, written, or removed. - Concurrent saves are single-statement atomic replacements of one fixed row; the last complete writer wins. - `sce auth logout` deletes the WorkOS credential and the telemetry credential, attempting both even when one fails and surfacing the first storage error. `credentials_removed` is true when either record was deleted; output fields are otherwise unchanged. - Telemetry save/delete never touches the WorkOS `auth_credentials` row.