From 5ef36c8e1dd2e20174d65e5e4da2cf852fbb1cca Mon Sep 17 00:00:00 2001 From: ScriptedAlchemy Date: Thu, 1 Oct 2026 19:08:53 +0000 Subject: [PATCH 1/4] fix(config): report a corrupt profile config instead of defaults A torn or unreadable profile config.toml was read through parse_or_warn_default: one process-deduplicated stderr line, then every entry became its default. A stored dashboard or memory-injection opt-out silently turned back on, the tracked hosts and pending upload count vanished, and the GitHub review sources registered nothing. UserConfig::load is now the one strict reader (a missing file is still defaults); the lenient loader, load_strict, parse_or_warn_default and its warn-once set are deleted. `tracedecay doctor` reports a corrupt profile config, or unusable github_review_sources, as an issue naming the file, the parse error and its repair. Commands that cannot read the config say so and skip the counter flush; lifecycle passes refuse; an MCP shutdown that cannot save the counter delta records it in its shutdown failures. --- .../src/agents/codex.rs | 5 +- .../src/hooks/memory_inject.rs | 10 +- .../src/advisory/github_runtime.rs | 2 +- .../github_runtime/credential_lifecycle.rs | 26 +++- crates/tracedecay-cli/src/agent_cmd.rs | 4 +- crates/tracedecay-cli/src/main.rs | 42 ++++-- crates/tracedecay-cli/src/status_cmd.rs | 33 +++-- crates/tracedecay-cli/src/update_cmd.rs | 8 +- crates/tracedecay-cli/src/upgrade.rs | 14 +- .../sweep_outcomes.rs | 35 +++++ .../src/configuration/user_settings.rs | 2 +- .../src/user_config.rs | 123 +++++------------- crates/tracedecay/src/doctor.rs | 20 ++- .../tracedecay/src/mcp/server/connection.rs | 24 +--- crates/tracedecay/src/mcp/server/ledger.rs | 42 +++--- 15 files changed, 204 insertions(+), 186 deletions(-) diff --git a/crates/tracedecay-agent-hosts/src/agents/codex.rs b/crates/tracedecay-agent-hosts/src/agents/codex.rs index 93c440ba7e..e3fae4b8bb 100644 --- a/crates/tracedecay-agent-hosts/src/agents/codex.rs +++ b/crates/tracedecay-agent-hosts/src/agents/codex.rs @@ -2350,9 +2350,10 @@ fn doctor_check_hooks( /// `~/.codex/memories/`, the holographic fact store stays the single source /// of truth and delivery is rendered prompt context only. fn doctor_suggest_native_memories_off(dc: &mut DoctorCounters, profile_root: &Path, home: &Path) { - if !crate::hooks::memory_inject::memory_injection_enabled(profile_root) { + // An unreadable profile config is the User config section's issue. + let Ok(true) = crate::hooks::memory_inject::memory_injection_enabled(profile_root) else { return; - } + }; let config_path = codex_config_path(home); let Ok(config) = load_toml_file(&config_path) else { return; diff --git a/crates/tracedecay-agent-hosts/src/hooks/memory_inject.rs b/crates/tracedecay-agent-hosts/src/hooks/memory_inject.rs index eaecd6c044..4ebad08d18 100644 --- a/crates/tracedecay-agent-hosts/src/hooks/memory_inject.rs +++ b/crates/tracedecay-agent-hosts/src/hooks/memory_inject.rs @@ -8,12 +8,14 @@ /// Whether daemon-owned memory guidance is enabled: the environment override /// wins when set, otherwise the configuration of the profile whose data /// directory is `profile_root` applies. -pub fn memory_injection_enabled(profile_root: &std::path::Path) -> bool { - injection_enabled_from( +pub fn memory_injection_enabled( + profile_root: &std::path::Path, +) -> Result { + Ok(injection_enabled_from( std::env::var("TRACEDECAY_MEMORY_INJECTION").ok().as_deref(), - tracedecay_session_memory::user_config::UserConfig::load(profile_root) + tracedecay_session_memory::user_config::UserConfig::load(profile_root)? .memory_injection_enabled, - ) + )) } fn injection_enabled_from(env_value: Option<&str>, config_flag: bool) -> bool { diff --git a/crates/tracedecay-application/src/advisory/github_runtime.rs b/crates/tracedecay-application/src/advisory/github_runtime.rs index 437e8a7151..d3793a2ca7 100644 --- a/crates/tracedecay-application/src/advisory/github_runtime.rs +++ b/crates/tracedecay-application/src/advisory/github_runtime.rs @@ -54,7 +54,7 @@ pub use anchors::{ }; pub use credential_lifecycle::{ GitHubReadOnlyCredentialLifecycleV1, GitHubReadOnlyCredentialPermissionVerifierV1, - GitHubSecretReadErrorV1, GitHubSecretReadPortV1, + GitHubSecretReadErrorV1, GitHubSecretReadPortV1, check_configured_github_review_sources_v1, }; pub use decoder::{ GitHubCanonicalReviewAnchorAuthorityV1, GitHubCanonicalReviewAnchorsV1, diff --git a/crates/tracedecay-application/src/advisory/github_runtime/credential_lifecycle.rs b/crates/tracedecay-application/src/advisory/github_runtime/credential_lifecycle.rs index 528335dbb5..f897795edd 100644 --- a/crates/tracedecay-application/src/advisory/github_runtime/credential_lifecycle.rs +++ b/crates/tracedecay-application/src/advisory/github_runtime/credential_lifecycle.rs @@ -6,6 +6,7 @@ use std::time::Duration; use serde::Deserialize; use tracedecay_domain::UserProfileId; +use tracedecay_session_memory::user_config::{ConfigSaveError, read_profile_config}; use zeroize::Zeroizing; use super::{ @@ -328,7 +329,17 @@ impl GitHubReadOnlyCredentialLifecycleV1 { secrets: Arc, verifier: Arc, ) { - let configured = load_configured_repositories(profile_root); + let configured = match read_profile_config::(profile_root) { + Ok(configured) => configured, + Err(error) => { + tracing::warn!( + event = "github_review_sources_unreadable", + %error, + "no GitHub review source is registered for this profile" + ); + return; + } + }; let mut repositories = BTreeMap::new(); for repository in configured.github_review_sources { let key = (repository.owner.clone(), repository.repository.clone()); @@ -437,12 +448,13 @@ impl GitHubReadOnlyCredentialLifecycleV1 { } } -fn load_configured_repositories(profile_root: &Path) -> ConfiguredGitHubRepositoriesV1 { - let path = profile_root.join("config.toml"); - let Ok(contents) = std::fs::read_to_string(&path) else { - return ConfiguredGitHubRepositoriesV1::default(); - }; - tracedecay_session_memory::user_config::parse_or_warn_default(&path, &contents) +/// Reads the profile's `github_review_sources` the way the daemon registers +/// them. An error means the daemon registered none; `tracedecay doctor` +/// reports it with the file to repair. +pub fn check_configured_github_review_sources_v1( + profile_root: &Path, +) -> Result<(), ConfigSaveError> { + read_profile_config::(profile_root).map(drop) } fn valid_locator(value: &str) -> bool { diff --git a/crates/tracedecay-cli/src/agent_cmd.rs b/crates/tracedecay-cli/src/agent_cmd.rs index c58e3ceb11..d28c67ca03 100644 --- a/crates/tracedecay-cli/src/agent_cmd.rs +++ b/crates/tracedecay-cli/src/agent_cmd.rs @@ -1039,10 +1039,10 @@ fn apply_canonical_component_set( Ok(HostLifecycleResult::Applied) } -fn load_host_lifecycle_user_config( +pub(crate) fn load_host_lifecycle_user_config( profile: &ProfileRoot, ) -> tracedecay_domain::errors::Result { - UserConfig::load_strict(profile.data_dir()).map_err(|error| { + UserConfig::load(profile.data_dir()).map_err(|error| { tracedecay_domain::errors::TraceDecayError::Config { message: format!("failed to load host lifecycle policy: {error}"), } diff --git a/crates/tracedecay-cli/src/main.rs b/crates/tracedecay-cli/src/main.rs index dea1596eae..328472af08 100644 --- a/crates/tracedecay-cli/src/main.rs +++ b/crates/tracedecay-cli/src/main.rs @@ -888,7 +888,34 @@ async fn run_startup_preamble(profile: &ProfileRoot, command: &Commands) { let is_first_run = !tracedecay_session_memory::user_config::UserConfig::exists(profile_root); let is_force_flush = matches!(command, Commands::Sync { .. } | Commands::Status { .. }); - let mut user_config = tracedecay_session_memory::user_config::UserConfig::load(profile_root); + match tracedecay_session_memory::user_config::UserConfig::load(profile_root) { + Ok(user_config) => { + flush_worldwide_counter(profile, command, is_force_flush, user_config).await; + } + Err(err) => eprintln!("warning: {err}"), + } + + if is_first_run && startup_policy.runs_startup_maintenance() { + eprintln!( + "note: tracedecay can optionally upload anonymous token savings counts to a worldwide counter.\n\ + \x20 Run `tracedecay enable-upload-counter` to opt in." + ); + } + + if startup_policy.runs_agent_install_check() + && let Some(home) = profile.home() + { + tracedecay_agent_hosts::agents::claude::check_install_stale(home); + } +} + +async fn flush_worldwide_counter( + profile: &ProfileRoot, + command: &Commands, + is_force_flush: bool, + mut user_config: tracedecay_session_memory::user_config::UserConfig, +) { + let profile_root = profile.data_dir(); // Skip the worldwide-counter flush on hot startup paths. `try_flush` // makes a synchronous HTTP call which can add seconds to // `tracedecay serve` startup on slow networks, long enough to blow the @@ -926,19 +953,6 @@ async fn run_startup_preamble(profile: &ProfileRoot, command: &Commands) { { eprintln!("warning: could not save tracedecay config: {err}"); } - - if is_first_run && startup_policy.runs_startup_maintenance() { - eprintln!( - "note: tracedecay can optionally upload anonymous token savings counts to a worldwide counter.\n\ - \x20 Run `tracedecay enable-upload-counter` to opt in." - ); - } - - if startup_policy.runs_agent_install_check() - && let Some(home) = profile.home() - { - tracedecay_agent_hosts::agents::claude::check_install_stale(home); - } } async fn resolve_registered_project_root( diff --git a/crates/tracedecay-cli/src/status_cmd.rs b/crates/tracedecay-cli/src/status_cmd.rs index 58acfca504..b5bfe07439 100644 --- a/crates/tracedecay-cli/src/status_cmd.rs +++ b/crates/tracedecay-cli/src/status_cmd.rs @@ -492,7 +492,14 @@ async fn handle_status_command_within( "timed out waiting for canonical worldwide-counter upload setting before status deadline" .to_string(), })??; - let mut config = tracedecay_session_memory::user_config::UserConfig::load(profile.data_dir()); + let mut config = + match tracedecay_session_memory::user_config::UserConfig::load(profile.data_dir()) { + Ok(config) => Some(config), + Err(error) => { + eprintln!("warning: {error}"); + None + } + }; let now = current_unix_timestamp(); let stdout_is_terminal = std::io::stdout().is_terminal(); let stderr_is_terminal = std::io::stderr().is_terminal(); @@ -514,18 +521,17 @@ async fn handle_status_command_within( // has expired, one refresh for the next invocation starts here so its // round-trip overlaps the render, and is joined after it within the // command deadline. - let refresh = show_online - .then(|| OnlineRefreshPlan::for_cache(&config, now)) + let online_config = config.as_ref().filter(|_| show_online); + let refresh = online_config + .map(|config| OnlineRefreshPlan::for_cache(config, now)) .filter(OnlineRefreshPlan::is_needed) .map(|plan| tokio::task::spawn_blocking(move || plan.fetch())); - let worldwide = show_online - .then_some(config.last_worldwide_total) + let worldwide = online_config + .map(|config| config.last_worldwide_total) .filter(|total| *total > 0); - let country_flags = if show_online { - config.cached_country_flags.clone() - } else { - Vec::new() - }; + let country_flags = online_config + .map(|config| config.cached_country_flags.clone()) + .unwrap_or_default(); hotpath::measure_block!("cli.status.render", { if should_print_status_logo(short, stdout_is_terminal) { // Tracked render of resources/logo.png; regenerate with @@ -594,13 +600,14 @@ async fn handle_status_command_within( if let Some(refresh) = refresh && let Some(fresh) = await_online_refresh(deadline, refresh).await - && fresh.apply(&mut config, now) + && let Some(config) = config.as_mut() + && fresh.apply(config, now) && let Err(err) = config.save_if_exists(profile.data_dir()) { eprintln!("warning: could not save tracedecay config: {err}"); } - if stdout_is_terminal { - global::check_for_update(profile, &mut config, false, true); + if stdout_is_terminal && let Some(config) = config.as_mut() { + global::check_for_update(profile, config, false, true); } Ok(()) } diff --git a/crates/tracedecay-cli/src/update_cmd.rs b/crates/tracedecay-cli/src/update_cmd.rs index 86026fb203..3787559be2 100644 --- a/crates/tracedecay-cli/src/update_cmd.rs +++ b/crates/tracedecay-cli/src/update_cmd.rs @@ -10,7 +10,9 @@ use std::path::{Path, PathBuf}; use tracedecay_runtime_core::config::ProfileRoot; -use crate::agent_cmd::{HostLifecycleCompletion, HostLifecycleSummary}; +use crate::agent_cmd::{ + HostLifecycleCompletion, HostLifecycleSummary, load_host_lifecycle_user_config, +}; use crate::upgrade::UpgradeOutcome; use tracedecay_daemon_control as daemon_control; use tracedecay_domain::errors::StoreResetRequiredV1; @@ -711,7 +713,7 @@ async fn run_post_update_mutations( // `--no-reinstall` is a durable opt-out for THIS version, not a // one-command deferral: advance the version markers so the explicit // lifecycle decision remains durable for this version. - let mut config = UserConfig::load(profile.data_dir()); + let mut config = load_host_lifecycle_user_config(profile)?; if let Err(err) = record_completed_reinstall_pass(profile, &mut config) { eprintln!("warning: {err}"); } @@ -722,7 +724,7 @@ async fn run_post_update_mutations( // MCP config. Run the full tracked-agent pass, then advance the version // markers. On failure the markers stay put so the incomplete explicit // lifecycle remains observable. - let mut config = UserConfig::load(profile.data_dir()); + let mut config = load_host_lifecycle_user_config(profile)?; // Prune tracked ids that no longer resolve to an integration (a release // renamed/removed one, or a typo landed in `installed_agents`). // The reinstall pass skips such ids, but dropping them here stops the diff --git a/crates/tracedecay-cli/src/upgrade.rs b/crates/tracedecay-cli/src/upgrade.rs index 1ed8e67166..72895da89c 100644 --- a/crates/tracedecay-cli/src/upgrade.rs +++ b/crates/tracedecay-cli/src/upgrade.rs @@ -906,12 +906,14 @@ fn preflight_asset_check(version: &str, is_beta: bool) -> Result Result { - let config = UserConfig::load_strict(profile_root) + let config = UserConfig::load(profile_root) .map_err(|error| unavailable(format!("user configuration metadata: {error}")))?; Ok(UserMetadata { installed_agents: config.installed_agents, diff --git a/crates/tracedecay-session-memory/src/user_config.rs b/crates/tracedecay-session-memory/src/user_config.rs index e657323400..5be6c429ac 100644 --- a/crates/tracedecay-session-memory/src/user_config.rs +++ b/crates/tracedecay-session-memory/src/user_config.rs @@ -4,11 +4,10 @@ //! gracefully. Keys other profile readers own (e.g. GitHub repositories) are //! preserved; retired keys are dropped at load and erased by the next write. -use std::collections::{BTreeMap, HashSet}; +use std::collections::BTreeMap; use std::fs; use std::io; use std::path::{Path, PathBuf}; -use std::sync::{Mutex, OnceLock}; use serde::{Deserialize, Serialize}; use tracedecay_domain::canonical_text::default_true; @@ -249,39 +248,25 @@ fn parse_error_line(contents: &str, err: &toml::de::Error) -> Option { Some(contents[..end].bytes().filter(|&b| b == b'\n').count() + 1) } -/// Paths for which a corrupt-config warning has already been printed this -/// process, so a hot loader (dashboard handlers, the daemon's per-request -/// config read) doesn't spam stderr once per call. -fn warned_corrupt_config_paths() -> &'static Mutex> { - static WARNED: OnceLock>> = OnceLock::new(); - WARNED.get_or_init(|| Mutex::new(HashSet::new())) -} - -/// Parses `contents` (read from `path`) as `T`, returning the default and -/// printing a one-time-per-path warning if the TOML is corrupt. -/// -/// Shared by [`UserConfig::load`] call sites so silently-defaulting readers -/// agree on what "corrupt" means and on not spamming stderr. -pub fn parse_or_warn_default(path: &Path, contents: &str) -> T +/// Reads the profile's `config.toml` as `T`, the view one profile reader +/// owns. A missing file is `T::default()`; an unreadable or unparseable one +/// is an error naming the file and its repair, never defaults that would +/// silently replace what the operator stored. +pub fn read_profile_config(profile_root: &Path) -> std::result::Result where T: Default + serde::de::DeserializeOwned, { - match toml::from_str(contents) { - Ok(value) => value, - Err(err) => { - let warned = warned_corrupt_config_paths(); - let mut seen = warned - .lock() - .unwrap_or_else(std::sync::PoisonError::into_inner); - if seen.insert(path.to_path_buf()) { - eprintln!( - "warning: could not parse config '{}' ({err}); using defaults", - path.display() - ); - } - T::default() - } - } + let path = config_path(profile_root); + let contents = match fs::read_to_string(&path) { + Ok(contents) => contents, + Err(error) if error.kind() == io::ErrorKind::NotFound => return Ok(T::default()), + Err(source) => return Err(ConfigSaveError::ExistingUnreadable { path, source }), + }; + toml::from_str(&contents).map_err(|error| ConfigSaveError::CorruptExisting { + line: parse_error_line(&contents, &error), + message: error.to_string(), + path, + }) } impl UserConfig { @@ -295,16 +280,13 @@ impl UserConfig { .unwrap_or(true) } - /// Loads the user-level config file. - /// Returns defaults if the file is missing or unreadable. A present but - /// unparseable file prints a one-time warning to stderr (see - /// [`parse_or_warn_default`]) instead of silently defaulting. - pub fn load(profile_root: &Path) -> Self { - let path = config_path(profile_root); - let Ok(contents) = std::fs::read_to_string(&path) else { - return Self::default(); - }; - parse_or_warn_default::(&path, &contents).without_retired_keys() + /// Loads the user-level config file; a missing file means defaults. + /// + /// An unreadable or malformed file is an error, not defaults: those would + /// silently turn a stored opt-out (dashboard, memory injection) back on + /// and drop the tracked hosts and pending counts. + pub fn load(profile_root: &Path) -> std::result::Result { + read_profile_config::(profile_root).map(Self::without_retired_keys) } fn without_retired_keys(mut self) -> Self { @@ -314,30 +296,6 @@ impl UserConfig { self } - /// Loads configuration without substituting defaults for an unreadable or - /// malformed persisted file. - /// - /// Lifecycle callers use this when a missing policy would enable host - /// behavior: corruption must stop the operation rather than silently turn - /// an opt-out back on. A genuinely missing file still means defaults. - pub fn load_strict(profile_root: &Path) -> std::result::Result { - let path = config_path(profile_root); - let contents = match fs::read_to_string(&path) { - Ok(contents) => contents, - Err(error) if error.kind() == io::ErrorKind::NotFound => return Ok(Self::default()), - Err(source) => { - return Err(ConfigSaveError::ExistingUnreadable { path, source }); - } - }; - toml::from_str::(&contents) - .map(Self::without_retired_keys) - .map_err(|error| ConfigSaveError::CorruptExisting { - path, - line: parse_error_line(&contents, &error), - message: error.to_string(), - }) - } - /// Saves the user-level config file atomically. /// /// The in-memory config is serialized up front so a serialize failure never @@ -365,7 +323,7 @@ impl UserConfig { // Checked outside the lock: every writer holds it and renames a whole // parseable file into place, so only an outside editor, which no lock // excludes, can corrupt the file between this check and the rename. - Self::refuse_unparseable_existing(&path)?; + read_profile_config::(profile_root)?; if let Some(parent) = path.parent() { tracedecay_runtime_core::storage::PrivateStoreIo::create_dir_all(parent).map_err( @@ -392,26 +350,6 @@ impl UserConfig { result } - fn refuse_unparseable_existing(path: &Path) -> std::result::Result<(), ConfigSaveError> { - let existing = match fs::read_to_string(path) { - Ok(existing) => existing, - Err(error) if error.kind() == io::ErrorKind::NotFound => return Ok(()), - Err(source) => { - return Err(ConfigSaveError::ExistingUnreadable { - path: path.to_path_buf(), - source, - }); - } - }; - toml::from_str::(&existing) - .map(|_| ()) - .map_err(|err| ConfigSaveError::CorruptExisting { - path: path.to_path_buf(), - line: parse_error_line(&existing, &err), - message: err.to_string(), - }) - } - fn write_locked(path: &Path, contents: &str) -> std::result::Result<(), ConfigSaveError> { // Atomic replace: write a temp file in the same directory, then rename // it over the target. `rename` is atomic on POSIX and Windows, so a @@ -538,14 +476,13 @@ mod tests { path.display() ); assert_eq!(save_error.to_string(), expected); - let load_error = - UserConfig::load_strict(profile).expect_err("a corrupt config must not load"); + let load_error = UserConfig::load(profile).expect_err("a corrupt config must not load"); assert_eq!(load_error.to_string(), expected); assert_eq!(entries(), vec!["config.toml".to_owned()]); std::fs::write(&path, "pending_upload = 0\n").unwrap(); config.save(profile).expect("a parseable config saves"); - assert_eq!(UserConfig::load_strict(profile).unwrap().pending_upload, 7); + assert_eq!(UserConfig::load(profile).unwrap().pending_upload, 7); } #[test] @@ -685,7 +622,7 @@ mod tests { ) .unwrap(); - let mut config = UserConfig::load(profile); + let mut config = UserConfig::load(profile).unwrap(); config.pending_upload = 3; config @@ -705,7 +642,7 @@ mod tests { } #[test] - fn strict_load_rejects_corrupt_dashboard_policy_instead_of_enabling_it() { + fn load_rejects_corrupt_dashboard_policy_instead_of_enabling_it() { let temp = TempDir::new().unwrap(); let profile = temp.path(); let path = config_path(profile); @@ -717,7 +654,7 @@ mod tests { .unwrap(); assert!(matches!( - UserConfig::load_strict(profile), + UserConfig::load(profile), Err(ConfigSaveError::CorruptExisting { .. }) )); } diff --git a/crates/tracedecay/src/doctor.rs b/crates/tracedecay/src/doctor.rs index 14ea9b4a7c..f4f8e9c1fc 100644 --- a/crates/tracedecay/src/doctor.rs +++ b/crates/tracedecay/src/doctor.rs @@ -18,6 +18,7 @@ use tracedecay_domain::configuration::{ use tracedecay_tool_catalog::{ApplicationSurfaceOperation, BindingSurface}; use tracedecay_agent_hosts::agents::{self, DoctorCounters, HealthcheckContext}; +use tracedecay_application::advisory::github_runtime; use tracedecay_automation_runtime::automation::effect_runtime::pending_automation_effect_resets_blocking; use tracedecay_contracts::request_identity::{GlobalRequestSurface, mint_global_request_id}; use tracedecay_contracts::{ConfigurationGetRequestV1, ConfigurationWireRequestV1}; @@ -1163,11 +1164,20 @@ fn check_user_config( "Worldwide counter upload setting unavailable from canonical configuration: {error}" )), } - if tracedecay_session_memory::user_config::config_path(profile_root).exists() { - let config = tracedecay_session_memory::user_config::UserConfig::load(profile_root); - if config.pending_upload > 0 { - dc.info(&format!("Pending upload: {} tokens", config.pending_upload)); + match tracedecay_session_memory::user_config::UserConfig::load(profile_root) { + Ok(config) => { + if config.pending_upload > 0 { + dc.info(&format!("Pending upload: {} tokens", config.pending_upload)); + } + if let Err(error) = + github_runtime::check_configured_github_review_sources_v1(profile_root) + { + dc.fail(&format!( + "GitHub review sources are unusable, none is registered: {error}" + )); + } } + Err(error) => dc.fail(&format!("Profile config is unusable: {error}")), } } @@ -1253,7 +1263,7 @@ fn warn_detected_unintegrated_host( /// The hosts the profile tracks; an unreadable profile config is a warning, /// with no host treated as tracked. fn tracked_hosts(dc: &mut DoctorCounters, profile_root: &Path) -> Vec { - match tracedecay_session_memory::user_config::UserConfig::load_strict(profile_root) { + match tracedecay_session_memory::user_config::UserConfig::load(profile_root) { Ok(config) => config.installed_agents, Err(error) => { dc.warn(&format!("Tracked hosts are unknown: {error}")); diff --git a/crates/tracedecay/src/mcp/server/connection.rs b/crates/tracedecay/src/mcp/server/connection.rs index 676e3673f4..eefeda621b 100644 --- a/crates/tracedecay/src/mcp/server/connection.rs +++ b/crates/tracedecay/src/mcp/server/connection.rs @@ -314,23 +314,13 @@ impl McpServer { .to_owned(), ), (Ok(upload_enabled), Some(profile)) => { - let profile_root = profile.data_dir(); - let mut config = - tracedecay_session_memory::user_config::UserConfig::load( - profile_root, - ); - config.pending_upload += delta; - if upload_enabled - && let Some(_total) = tracedecay_dashboard_api::cloud::flush_pending( - config.pending_upload, - ) - { - config.pending_upload = 0; - let now = tracedecay_runtime_core::tracedecay::current_timestamp(); - config.last_upload_at = now; - } - if let Err(err) = config.save(profile_root) { - tracing::warn!(error = %err, "could not save upload config during shutdown"); + if let Err(error) = super::ledger::persist_worldwide_delta( + profile.data_dir(), + delta, + upload_enabled, + ) { + failures + .push(format!("worldwide counter delta not saved: {error}")); } } } diff --git a/crates/tracedecay/src/mcp/server/ledger.rs b/crates/tracedecay/src/mcp/server/ledger.rs index 0a49b2dace..57acefa5b7 100644 --- a/crates/tracedecay/src/mcp/server/ledger.rs +++ b/crates/tracedecay/src/mcp/server/ledger.rs @@ -357,14 +357,21 @@ impl McpServer { return; } }; - let saved = tokio::task::spawn_blocking(move || { + let saved = match tokio::task::spawn_blocking(move || { persist_worldwide_delta(&profile_root, delta, upload_enabled) }) .await - .unwrap_or_else(|error| { - tracing::warn!(%error, "worldwide counter flush task failed"); - false - }); + { + Ok(Ok(())) => true, + Ok(Err(error)) => { + tracing::warn!(%error, "could not save upload config"); + false + } + Err(error) => { + tracing::warn!(%error, "worldwide counter flush task failed"); + false + } + }; if saved && let Some(last_flushed_tokens) = server.last_flushed_tokens.as_ref() { @@ -591,8 +598,14 @@ impl McpServer { } } -fn persist_worldwide_delta(profile_root: &Path, delta: u64, upload_enabled: bool) -> bool { - let mut config = tracedecay_session_memory::user_config::UserConfig::load(profile_root); +/// Adds `delta` to the profile's pending worldwide-counter count, uploading it +/// when enabled, and saves the profile config. +pub(super) fn persist_worldwide_delta( + profile_root: &Path, + delta: u64, + upload_enabled: bool, +) -> std::result::Result<(), tracedecay_session_memory::user_config::ConfigSaveError> { + let mut config = tracedecay_session_memory::user_config::UserConfig::load(profile_root)?; config.pending_upload = config.pending_upload.saturating_add(delta); if upload_enabled && tracedecay_dashboard_api::cloud::flush_pending(config.pending_upload).is_some() @@ -600,13 +613,7 @@ fn persist_worldwide_delta(profile_root: &Path, delta: u64, upload_enabled: bool config.pending_upload = 0; config.last_upload_at = tracedecay_runtime_core::tracedecay::current_timestamp(); } - match config.save(profile_root) { - Ok(()) => true, - Err(error) => { - tracing::warn!(error = %error, "could not save upload config"); - false - } - } + config.save(profile_root) } fn claim_worldwide_flush(last_flush_at: &AtomicI64, expected: i64, now: i64) -> bool { @@ -651,7 +658,7 @@ mod tests { fn disabled_upload_records_each_delta_once_after_durable_save() { let profile = tempfile::tempdir().expect("profile"); let profile = profile.path(); - let mut config = tracedecay_session_memory::user_config::UserConfig::load(profile); + let mut config = tracedecay_session_memory::user_config::UserConfig::load(profile).unwrap(); config.pending_upload = 0; config .save(profile) @@ -662,14 +669,13 @@ mod tests { for _ in 0..2 { let previous = last_flushed.load(Ordering::Acquire); if current > previous { - let saved = persist_worldwide_delta(profile, current - previous, false); - if saved { + if persist_worldwide_delta(profile, current - previous, false).is_ok() { last_flushed.store(current, Ordering::Release); } } } - let persisted = tracedecay_session_memory::user_config::UserConfig::load(profile); + let persisted = tracedecay_session_memory::user_config::UserConfig::load(profile).unwrap(); assert_eq!(persisted.pending_upload, current); assert_eq!(last_flushed.load(Ordering::Acquire), current); } From 7f7a376eb1d53c085162cb8a39f1e7ca9ceff132 Mon Sep 17 00:00:00 2001 From: ScriptedAlchemy Date: Thu, 1 Oct 2026 20:08:50 +0000 Subject: [PATCH 2/4] fix(config): report each unregistered GitHub review source --- .../src/advisory/github_runtime.rs | 3 +- .../github_runtime/credential_lifecycle.rs | 231 ++++++++++++++---- .../sweep_outcomes.rs | 23 +- crates/tracedecay/src/doctor.rs | 18 +- 4 files changed, 227 insertions(+), 48 deletions(-) diff --git a/crates/tracedecay-application/src/advisory/github_runtime.rs b/crates/tracedecay-application/src/advisory/github_runtime.rs index d3793a2ca7..cc04074df7 100644 --- a/crates/tracedecay-application/src/advisory/github_runtime.rs +++ b/crates/tracedecay-application/src/advisory/github_runtime.rs @@ -54,7 +54,8 @@ pub use anchors::{ }; pub use credential_lifecycle::{ GitHubReadOnlyCredentialLifecycleV1, GitHubReadOnlyCredentialPermissionVerifierV1, - GitHubSecretReadErrorV1, GitHubSecretReadPortV1, check_configured_github_review_sources_v1, + GitHubReviewSourceFindingV1, GitHubSecretReadErrorV1, GitHubSecretReadPortV1, + check_configured_github_review_sources_v1, }; pub use decoder::{ GitHubCanonicalReviewAnchorAuthorityV1, GitHubCanonicalReviewAnchorsV1, diff --git a/crates/tracedecay-application/src/advisory/github_runtime/credential_lifecycle.rs b/crates/tracedecay-application/src/advisory/github_runtime/credential_lifecycle.rs index f897795edd..f243d991d7 100644 --- a/crates/tracedecay-application/src/advisory/github_runtime/credential_lifecycle.rs +++ b/crates/tracedecay-application/src/advisory/github_runtime/credential_lifecycle.rs @@ -329,8 +329,8 @@ impl GitHubReadOnlyCredentialLifecycleV1 { secrets: Arc, verifier: Arc, ) { - let configured = match read_profile_config::(profile_root) { - Ok(configured) => configured, + let sources = match read_profile_config::(profile_root) { + Ok(configured) => review_sources(configured).0, Err(error) => { tracing::warn!( event = "github_review_sources_unreadable", @@ -340,32 +340,13 @@ impl GitHubReadOnlyCredentialLifecycleV1 { return; } }; - let mut repositories = BTreeMap::new(); - for repository in configured.github_review_sources { - let key = (repository.owner.clone(), repository.repository.clone()); - match repositories.entry(key) { - Entry::Vacant(entry) => { - entry.insert(Some(repository)); - } - Entry::Occupied(mut entry) => { - entry.insert(None); - } - } - } let Ok(mut registrations) = self.registrations.lock() else { return; }; - for repository in repositories.into_values().flatten() { - let key = ( - profile_id.clone(), - repository.owner.clone(), - repository.repository.clone(), - ); - match repository.access { - ConfiguredGitHubAccessV1::Public - if repository.keyring_service.is_none() - && repository.keyring_account.is_none() => - { + for source in sources { + match source { + RegistrableGitHubReviewSourceV1::Public { owner, repository } => { + let key = (profile_id.clone(), owner, repository); if register_profile_github_public_repository_v1( key.0.clone(), key.1.clone(), @@ -374,15 +355,13 @@ impl GitHubReadOnlyCredentialLifecycleV1 { registrations.push(ProfileRepositoryCredentialRegistrationV1::Public(key)); } } - ConfiguredGitHubAccessV1::OsKeyring => { - let (Some(keyring_service), Some(keyring_account)) = - (repository.keyring_service, repository.keyring_account) - else { - continue; - }; - if !valid_locator(&keyring_service) || !valid_locator(&keyring_account) { - continue; - } + RegistrableGitHubReviewSourceV1::OsKeyring { + owner, + repository, + keyring_service, + keyring_account, + } => { + let key = (profile_id.clone(), owner, repository); let authority: Arc = Arc::new(OsKeyringGitHubReadOnlyCredentialAuthorityV1 { repository_owner: key.1.clone(), @@ -404,7 +383,6 @@ impl GitHubReadOnlyCredentialLifecycleV1 { }); } } - ConfiguredGitHubAccessV1::Public => {} } } } @@ -448,13 +426,131 @@ impl GitHubReadOnlyCredentialLifecycleV1 { } } -/// Reads the profile's `github_review_sources` the way the daemon registers -/// them. An error means the daemon registered none; `tracedecay doctor` -/// reports it with the file to repair. +/// A configured `github_review_sources` entry the daemon does not register. +#[derive(Clone, Debug, PartialEq, Eq)] +pub enum GitHubReviewSourceFindingV1 { + /// Listed more than once, so none of its entries registers. + Duplicate { owner: String, repository: String }, + /// `os_keyring` access without both `keyring_service` and `keyring_account`. + MissingKeyringLocator { owner: String, repository: String }, + /// A keyring locator that is empty, padded, over 512 bytes, or carries a + /// control character. + InvalidKeyringLocator { owner: String, repository: String }, + /// `public` access that also names a keyring locator. + PublicWithKeyringLocator { owner: String, repository: String }, +} + +impl std::fmt::Display for GitHubReviewSourceFindingV1 { + fn fmt(&self, formatter: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self { + Self::Duplicate { owner, repository } => { + write!(formatter, "{owner}/{repository} is listed more than once") + } + Self::MissingKeyringLocator { owner, repository } => write!( + formatter, + "{owner}/{repository} uses os_keyring access without keyring_service and \ + keyring_account" + ), + Self::InvalidKeyringLocator { owner, repository } => write!( + formatter, + "{owner}/{repository} names a keyring locator that is empty, padded, over 512 \ + bytes or carries a control character" + ), + Self::PublicWithKeyringLocator { owner, repository } => write!( + formatter, + "{owner}/{repository} uses public access but names a keyring locator" + ), + } + } +} + +enum RegistrableGitHubReviewSourceV1 { + Public { + owner: String, + repository: String, + }, + OsKeyring { + owner: String, + repository: String, + keyring_service: String, + keyring_account: String, + }, +} + +/// Splits the configured review sources into the ones the daemon registers +/// and a finding for each entry it does not. +fn review_sources( + configured: ConfiguredGitHubRepositoriesV1, +) -> ( + Vec, + Vec, +) { + let mut findings = Vec::new(); + let mut repositories = BTreeMap::new(); + for repository in configured.github_review_sources { + match repositories.entry((repository.owner.clone(), repository.repository.clone())) { + Entry::Vacant(entry) => { + entry.insert(Some(repository)); + } + Entry::Occupied(mut entry) => { + if entry.insert(None).is_some() { + findings.push(GitHubReviewSourceFindingV1::Duplicate { + owner: repository.owner, + repository: repository.repository, + }); + } + } + } + } + let mut sources = Vec::new(); + for ConfiguredGitHubRepositoryV1 { + owner, + repository, + access, + keyring_service, + keyring_account, + } in repositories.into_values().flatten() + { + match (access, keyring_service, keyring_account) { + (ConfiguredGitHubAccessV1::Public, None, None) => { + sources.push(RegistrableGitHubReviewSourceV1::Public { owner, repository }); + } + (ConfiguredGitHubAccessV1::Public, _, _) => { + findings.push(GitHubReviewSourceFindingV1::PublicWithKeyringLocator { + owner, + repository, + }); + } + (ConfiguredGitHubAccessV1::OsKeyring, Some(keyring_service), Some(keyring_account)) + if valid_locator(&keyring_service) && valid_locator(&keyring_account) => + { + sources.push(RegistrableGitHubReviewSourceV1::OsKeyring { + owner, + repository, + keyring_service, + keyring_account, + }); + } + (ConfiguredGitHubAccessV1::OsKeyring, Some(_), Some(_)) => { + findings + .push(GitHubReviewSourceFindingV1::InvalidKeyringLocator { owner, repository }); + } + (ConfiguredGitHubAccessV1::OsKeyring, _, _) => { + findings + .push(GitHubReviewSourceFindingV1::MissingKeyringLocator { owner, repository }); + } + } + } + (sources, findings) +} + +/// The configured review sources the daemon does not register, read the way +/// it registers them. An error means it registered none. pub fn check_configured_github_review_sources_v1( profile_root: &Path, -) -> Result<(), ConfigSaveError> { - read_profile_config::(profile_root).map(drop) +) -> Result, ConfigSaveError> { + read_profile_config::(profile_root) + .map(|configured| review_sources(configured).1) } fn valid_locator(value: &str) -> bool { @@ -476,7 +572,8 @@ mod tests { use super::{ GitHubProviderPermissionVerifierV1, GitHubReadOnlyCredentialLifecycleV1, - GitHubSecretReadErrorV1, GitHubSecretReadPortV1, + GitHubReviewSourceFindingV1, GitHubSecretReadErrorV1, GitHubSecretReadPortV1, + check_configured_github_review_sources_v1, }; use crate::advisory::github_runtime::{ GitHubReadPermissionV1, ProfileGitHubReadOnlyCredentialMountOutcomeV1, @@ -559,6 +656,24 @@ repository = "duplicate" access = "os_keyring" keyring_service = "tracedecay.github" keyring_account = "read" + +[[github_review_sources]] +owner = "ScriptedAlchemy" +repository = "public-with-locator" +access = "public" +keyring_service = "tracedecay.github" + +[[github_review_sources]] +owner = "ScriptedAlchemy" +repository = "keyring-unnamed" +access = "os_keyring" + +[[github_review_sources]] +owner = "ScriptedAlchemy" +repository = "keyring-padded" +access = "os_keyring" +keyring_service = " tracedecay.github" +keyring_account = "read" "#, ) .expect("profile configuration"); @@ -684,6 +799,40 @@ keyring_account = "read" ), ProfileGitHubReadOnlyCredentialMountOutcomeV1::NotConfigured ); + for repository in ["public-with-locator", "keyring-unnamed", "keyring-padded"] { + assert_eq!( + mount_profile_github_read_only_credential_authority_v1( + &profile_id, + "ScriptedAlchemy", + repository, + ), + ProfileGitHubReadOnlyCredentialMountOutcomeV1::NotConfigured, + "{repository}" + ); + } + let owner = || "ScriptedAlchemy".to_owned(); + assert_eq!( + check_configured_github_review_sources_v1(&profile_root) + .expect("the profile configuration parses"), + [ + GitHubReviewSourceFindingV1::Duplicate { + owner: owner(), + repository: "duplicate".to_owned(), + }, + GitHubReviewSourceFindingV1::InvalidKeyringLocator { + owner: owner(), + repository: "keyring-padded".to_owned(), + }, + GitHubReviewSourceFindingV1::MissingKeyringLocator { + owner: owner(), + repository: "keyring-unnamed".to_owned(), + }, + GitHubReviewSourceFindingV1::PublicWithKeyringLocator { + owner: owner(), + repository: "public-with-locator".to_owned(), + }, + ] + ); server.join().expect("permission verifier server"); lifecycle.shutdown(); diff --git a/crates/tracedecay-cli/tests/host_journeys_suite/host_lifecycle_cli_acceptance/sweep_outcomes.rs b/crates/tracedecay-cli/tests/host_journeys_suite/host_lifecycle_cli_acceptance/sweep_outcomes.rs index 3e00bf5867..711779e01a 100644 --- a/crates/tracedecay-cli/tests/host_journeys_suite/host_lifecycle_cli_acceptance/sweep_outcomes.rs +++ b/crates/tracedecay-cli/tests/host_journeys_suite/host_lifecycle_cli_acceptance/sweep_outcomes.rs @@ -533,7 +533,7 @@ fn doctor_fails_a_corrupt_profile_config_with_its_repair() { "{doctor_stderr}" ); - fs::write(&config, installed).unwrap(); + fs::write(&config, &installed).unwrap(); let repaired = cli.run(&["doctor"]); let repaired_stderr = stderr(&repaired); assert_eq!(repaired.status.code(), Some(0), "{repaired_stderr}"); @@ -541,6 +541,27 @@ fn doctor_fails_a_corrupt_profile_config_with_its_repair() { !repaired_stderr.contains("Profile config is unusable"), "{repaired_stderr}" ); + + fs::write( + &config, + format!( + "{installed}\n[[github_review_sources]]\nowner = \"ScriptedAlchemy\"\n\ + repository = \"keyring-unnamed\"\naccess = \"os_keyring\"\n" + ), + ) + .unwrap(); + let unregistered = cli.run(&["doctor"]); + let unregistered_stderr = stderr(&unregistered); + assert_eq!(unregistered.status.code(), Some(1), "{unregistered_stderr}"); + assert!( + unregistered_stderr.contains(&format!( + "GitHub review source ScriptedAlchemy/keyring-unnamed uses os_keyring access without \ + keyring_service and keyring_account, so it is not registered; fix or remove its \ + github_review_sources entry in {}", + config.display() + )), + "{unregistered_stderr}" + ); } #[test] diff --git a/crates/tracedecay/src/doctor.rs b/crates/tracedecay/src/doctor.rs index f4f8e9c1fc..b042db7933 100644 --- a/crates/tracedecay/src/doctor.rs +++ b/crates/tracedecay/src/doctor.rs @@ -1169,12 +1169,20 @@ fn check_user_config( if config.pending_upload > 0 { dc.info(&format!("Pending upload: {} tokens", config.pending_upload)); } - if let Err(error) = - github_runtime::check_configured_github_review_sources_v1(profile_root) - { - dc.fail(&format!( + match github_runtime::check_configured_github_review_sources_v1(profile_root) { + Ok(findings) => { + for finding in findings { + dc.fail(&format!( + "GitHub review source {finding}, so it is not registered; fix or \ + remove its github_review_sources entry in {}", + tracedecay_session_memory::user_config::config_path(profile_root) + .display() + )); + } + } + Err(error) => dc.fail(&format!( "GitHub review sources are unusable, none is registered: {error}" - )); + )), } } Err(error) => dc.fail(&format!("Profile config is unusable: {error}")), From ded22643293153678848437cb9368c1e108ed32b Mon Sep 17 00:00:00 2001 From: ScriptedAlchemy Date: Thu, 1 Oct 2026 20:34:42 +0000 Subject: [PATCH 3/4] refactor(config): split review-source classification --- .../github_runtime/credential_lifecycle.rs | 68 ++++++++++--------- crates/tracedecay/src/mcp/server/ledger.rs | 19 ++---- 2 files changed, 43 insertions(+), 44 deletions(-) diff --git a/crates/tracedecay-application/src/advisory/github_runtime/credential_lifecycle.rs b/crates/tracedecay-application/src/advisory/github_runtime/credential_lifecycle.rs index f243d991d7..1b07a62353 100644 --- a/crates/tracedecay-application/src/advisory/github_runtime/credential_lifecycle.rs +++ b/crates/tracedecay-application/src/advisory/github_runtime/credential_lifecycle.rs @@ -503,45 +503,49 @@ fn review_sources( } } let mut sources = Vec::new(); - for ConfiguredGitHubRepositoryV1 { + for repository in repositories.into_values().flatten() { + match registrable_review_source(repository) { + Ok(source) => sources.push(source), + Err(finding) => findings.push(finding), + } + } + (sources, findings) +} + +fn registrable_review_source( + configured: ConfiguredGitHubRepositoryV1, +) -> Result { + let ConfiguredGitHubRepositoryV1 { owner, repository, access, keyring_service, keyring_account, - } in repositories.into_values().flatten() - { - match (access, keyring_service, keyring_account) { - (ConfiguredGitHubAccessV1::Public, None, None) => { - sources.push(RegistrableGitHubReviewSourceV1::Public { owner, repository }); - } - (ConfiguredGitHubAccessV1::Public, _, _) => { - findings.push(GitHubReviewSourceFindingV1::PublicWithKeyringLocator { - owner, - repository, - }); - } - (ConfiguredGitHubAccessV1::OsKeyring, Some(keyring_service), Some(keyring_account)) - if valid_locator(&keyring_service) && valid_locator(&keyring_account) => - { - sources.push(RegistrableGitHubReviewSourceV1::OsKeyring { - owner, - repository, - keyring_service, - keyring_account, - }); - } - (ConfiguredGitHubAccessV1::OsKeyring, Some(_), Some(_)) => { - findings - .push(GitHubReviewSourceFindingV1::InvalidKeyringLocator { owner, repository }); - } - (ConfiguredGitHubAccessV1::OsKeyring, _, _) => { - findings - .push(GitHubReviewSourceFindingV1::MissingKeyringLocator { owner, repository }); - } + } = configured; + match (access, keyring_service, keyring_account) { + (ConfiguredGitHubAccessV1::Public, None, None) => { + Ok(RegistrableGitHubReviewSourceV1::Public { owner, repository }) + } + (ConfiguredGitHubAccessV1::Public, _, _) => { + Err(GitHubReviewSourceFindingV1::PublicWithKeyringLocator { owner, repository }) + } + (ConfiguredGitHubAccessV1::OsKeyring, Some(keyring_service), Some(keyring_account)) + if valid_locator(&keyring_service) && valid_locator(&keyring_account) => + { + Ok(RegistrableGitHubReviewSourceV1::OsKeyring { + owner, + repository, + keyring_service, + keyring_account, + }) + } + (ConfiguredGitHubAccessV1::OsKeyring, Some(_), Some(_)) => { + Err(GitHubReviewSourceFindingV1::InvalidKeyringLocator { owner, repository }) + } + (ConfiguredGitHubAccessV1::OsKeyring, _, _) => { + Err(GitHubReviewSourceFindingV1::MissingKeyringLocator { owner, repository }) } } - (sources, findings) } /// The configured review sources the daemon does not register, read the way diff --git a/crates/tracedecay/src/mcp/server/ledger.rs b/crates/tracedecay/src/mcp/server/ledger.rs index 57acefa5b7..a225a0b728 100644 --- a/crates/tracedecay/src/mcp/server/ledger.rs +++ b/crates/tracedecay/src/mcp/server/ledger.rs @@ -357,21 +357,16 @@ impl McpServer { return; } }; - let saved = match tokio::task::spawn_blocking(move || { + let saved = tokio::task::spawn_blocking(move || { persist_worldwide_delta(&profile_root, delta, upload_enabled) + .inspect_err(|error| tracing::warn!(%error, "could not save upload config")) + .is_ok() }) .await - { - Ok(Ok(())) => true, - Ok(Err(error)) => { - tracing::warn!(%error, "could not save upload config"); - false - } - Err(error) => { - tracing::warn!(%error, "worldwide counter flush task failed"); - false - } - }; + .unwrap_or_else(|error| { + tracing::warn!(%error, "worldwide counter flush task failed"); + false + }); if saved && let Some(last_flushed_tokens) = server.last_flushed_tokens.as_ref() { From 77e97600ea0dd836c586ca58ed175b300d34c064 Mon Sep 17 00:00:00 2001 From: ScriptedAlchemy Date: Thu, 1 Oct 2026 23:04:56 +0000 Subject: [PATCH 4/4] style(mcp): collapse the counter flush test condition --- crates/tracedecay/src/mcp/server/ledger.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/crates/tracedecay/src/mcp/server/ledger.rs b/crates/tracedecay/src/mcp/server/ledger.rs index a225a0b728..4a5b92e495 100644 --- a/crates/tracedecay/src/mcp/server/ledger.rs +++ b/crates/tracedecay/src/mcp/server/ledger.rs @@ -663,10 +663,10 @@ mod tests { for _ in 0..2 { let previous = last_flushed.load(Ordering::Acquire); - if current > previous { - if persist_worldwide_delta(profile, current - previous, false).is_ok() { - last_flushed.store(current, Ordering::Release); - } + if current > previous + && persist_worldwide_delta(profile, current - previous, false).is_ok() + { + last_flushed.store(current, Ordering::Release); } }