From 26295df979fc74bdeae30faae1e1592c8ad2dcf2 Mon Sep 17 00:00:00 2001 From: sanil-23 Date: Wed, 27 May 2026 20:48:34 +0530 Subject: [PATCH] fix(config): reconcile orphaned provider references on upgrade (#2759) (#2761) Co-authored-by: Claude Opus 4.7 (1M context) --- src/openhuman/migrations/mod.rs | 82 ++++--- src/openhuman/migrations/mod_tests.rs | 35 +++ .../reconcile_orphaned_providers.rs | 203 +++++++++++++++++ .../reconcile_orphaned_providers_tests.rs | 209 ++++++++++++++++++ 4 files changed, 504 insertions(+), 25 deletions(-) create mode 100644 src/openhuman/migrations/reconcile_orphaned_providers.rs create mode 100644 src/openhuman/migrations/reconcile_orphaned_providers_tests.rs diff --git a/src/openhuman/migrations/mod.rs b/src/openhuman/migrations/mod.rs index 69dd24ea7..1ffcacb5a 100644 --- a/src/openhuman/migrations/mod.rs +++ b/src/openhuman/migrations/mod.rs @@ -25,6 +25,7 @@ use crate::openhuman::config::Config; mod expand_autonomy_defaults; mod phase_out_profile_md; +mod reconcile_orphaned_providers; mod remove_write_auto_approve; mod repair_http_request_limits; mod retire_chat_v1_model; @@ -260,40 +261,71 @@ pub async fn run_pending(config: &mut Config) { // survives an update. Coerce to schema defaults (30s / 1 MB). Guard on // `== 5` so an earlier failed step doesn't get skipped. // - // NOTE: another in-flight migration (`reconcile_orphaned_providers`) also - // targets the 5 -> 6 transition. When that lands, both modules run inside - // this single `== 5` branch before the version is bumped to 6 — keep them - // as separate modules, one shared version bump. The tool constructors - // also clamp 0 at the point of use, so a user who crosses this gate via - // the other migration without running this one still gets working fetches. + // TWO migrations share this single 5 -> 6 transition: + // * `repair_http_request_limits` — coerce stale-zero `[http_request]` + // limits (a persisted `timeout_secs = 0` is an instant timeout that + // fails every web_fetch; serde defaults only fill *missing* keys). + // * `reconcile_orphaned_providers` — reset per-workload `*_provider` + // strings (and a dangling `primary_cloud`) that point at a cloud + // provider no longer in `cloud_providers`, which the inference factory + // hard-errors on. + // Both are independent and idempotent, so they run as separate modules + // behind one shared version bump. Bump + save only when BOTH succeed; if + // either fails, leave the gate at 5 and retry next launch (re-running the + // one that already succeeded is a no-op). Guard on `== 5` so an earlier + // failed step doesn't get skipped. if config.schema_version == 5 { + let mut all_ok = true; + match repair_http_request_limits::run(config) { - Ok(stats) => { - let previous_version = config.schema_version; - config.schema_version = 6; - if let Err(err) = config.save().await { - config.schema_version = previous_version; - log::warn!( - "[migrations] repair_http_request_limits ran but config.save failed: \ - {err:#} — rolled in-memory schema_version back to {previous_version}, \ - will retry on next launch" - ); - return; - } - log::info!( - "[migrations] schema_version bumped to 6 (repair_http_request_limits \ - timeout_repaired={} max_response_size_repaired={})", - stats.timeout_repaired, - stats.max_response_size_repaired, - ); - } + Ok(stats) => log::info!( + "[migrations] repair_http_request_limits ran (timeout_repaired={} \ + max_response_size_repaired={})", + stats.timeout_repaired, + stats.max_response_size_repaired, + ), Err(err) => { + all_ok = false; log::warn!( "[migrations] repair_http_request_limits failed: {err:#} — \ will retry on next launch" ); } } + + match reconcile_orphaned_providers::run(config) { + Ok(stats) => log::info!( + "[migrations] reconcile_orphaned_providers ran (workload_fields_scrubbed={} \ + primary_cloud_cleared={})", + stats.workload_fields_scrubbed, + stats.primary_cloud_cleared, + ), + Err(err) => { + all_ok = false; + log::warn!( + "[migrations] reconcile_orphaned_providers failed: {err:#} — \ + will retry on next launch" + ); + } + } + + if all_ok { + let previous_version = config.schema_version; + config.schema_version = 6; + if let Err(err) = config.save().await { + config.schema_version = previous_version; + log::warn!( + "[migrations] 5->6 migrations ran but config.save failed: {err:#} — \ + rolled in-memory schema_version back to {previous_version}, \ + will retry on next launch" + ); + return; + } + log::info!( + "[migrations] schema_version bumped to 6 \ + (repair_http_request_limits + reconcile_orphaned_providers)" + ); + } } } diff --git a/src/openhuman/migrations/mod_tests.rs b/src/openhuman/migrations/mod_tests.rs index 52d423cb8..72d44a651 100644 --- a/src/openhuman/migrations/mod_tests.rs +++ b/src/openhuman/migrations/mod_tests.rs @@ -326,6 +326,41 @@ async fn run_pending_v5_to_v6_repairs_http_request_limits() { ); } +/// Companion to the repair test above: the same single 5 -> 6 transition must +/// also run `reconcile_orphaned_providers`. A config at schema_version=5 with an +/// orphaned `chat_provider` (slug not in `cloud_providers`) and a dangling +/// `primary_cloud` must have both reset to managed and be bumped to v6 through +/// the full `run_pending` wiring. +#[tokio::test] +async fn run_pending_v5_to_v6_reconciles_orphaned_providers() { + let tmp = TempDir::new().unwrap(); + fs::create_dir_all(tmp.path().join("workspace")).unwrap(); + + let mut config = config_in(&tmp); + config.schema_version = 5; + // `openai` is not present in cloud_providers (left empty) → orphaned. + config.chat_provider = Some("openai:gpt-4o".to_string()); + config.primary_cloud = Some("p_missing".to_string()); + + run_pending(&mut config).await; + + assert_eq!(config.schema_version, CURRENT_SCHEMA_VERSION); + assert_eq!( + config.chat_provider, None, + "orphaned chat_provider must be reset to managed" + ); + assert_eq!( + config.primary_cloud, None, + "dangling primary_cloud must be cleared" + ); + + let on_disk = fs::read_to_string(&config.config_path).unwrap(); + assert!( + on_disk.contains("schema_version = 6"), + "saved config.toml must record schema_version=6, got:\n{on_disk}" + ); +} + /// Verify that a user at v3 with a deliberately customised /// `max_actions_per_hour` does NOT have it reset by the migration. #[tokio::test] diff --git a/src/openhuman/migrations/reconcile_orphaned_providers.rs b/src/openhuman/migrations/reconcile_orphaned_providers.rs new file mode 100644 index 000000000..738c5054c --- /dev/null +++ b/src/openhuman/migrations/reconcile_orphaned_providers.rs @@ -0,0 +1,203 @@ +//! Migration 5 → 6: reconcile orphaned per-workload provider references. +//! +//! ## The problem +//! +//! Each LLM workload is routed by a provider string in `Config` +//! (`chat_provider`, `reasoning_provider`, …) using the grammar parsed by +//! [`crate::openhuman::inference::provider::factory`]: +//! +//! ```text +//! ""/"cloud" → primary_cloud (managed) +//! "openhuman" → managed OpenHuman backend +//! "ollama:" → local Ollama +//! "lmstudio:" → local LM Studio +//! ":" → the cloud_providers entry whose slug == +//! ``` +//! +//! A `":"` string only resolves while a `cloud_providers` entry +//! with that slug still exists. When a user **removes/disables a cloud +//! provider** (e.g. OpenAI) on one build, the entry leaves `cloud_providers` +//! but the workload string keeps pointing at it — e.g. `chat_provider = +//! "openai:gpt-4o"` with no `openai` entry. Nothing reconciles this, so: +//! +//! - the AI settings panel still shows the now-gone OpenAI model for chat, and +//! - at runtime [`factory::make_cloud_provider_by_slug`] **hard-errors** +//! ("no cloud provider configured for slug 'openai'") instead of falling +//! back to managed — that workload's inference breaks. +//! +//! ## What this migration does +//! +//! A pure, idempotent pass over the persisted `Config`: +//! +//! - For each of the nine `*_provider` fields, reset to `None` (= managed) any +//! value that the factory could not resolve: a `":"` whose slug +//! is absent from `cloud_providers`, the always-managed `"openhuman:"` +//! form, or a bare non-sentinel string. Sentinels (`""`, `"cloud"`, +//! `"openhuman"`) and local providers (`ollama:`/`lmstudio:`, valid without a +//! `cloud_providers` entry) are left untouched. +//! - Clear `primary_cloud` when it points at an id no longer present in +//! `cloud_providers` (a dangling pointer; the factory already falls back to +//! the managed backend for an unresolved primary). +//! +//! Mirrors the factory's *exact*, case-sensitive grammar so "resolvable here" +//! means the same as "resolvable at inference time". The one intentional step +//! beyond the factory is normalizing `"openhuman:"` → `None`: both route +//! to the managed backend, and `None` matches what the settings UI persists. +//! +//! ## Behaviour +//! +//! - Pure in-memory mutation of `Config`; the caller (`migrations::run_pending`) +//! persists via `Config::save()` and bumps `schema_version`. +//! - Idempotent: a second run finds nothing left to scrub. +//! - Never touches keys/secrets or any other config field. + +use crate::openhuman::config::Config; +use crate::openhuman::inference::provider::factory::{ + LM_STUDIO_PROVIDER_PREFIX, OLLAMA_PROVIDER_PREFIX, PROVIDER_OPENHUMAN, +}; +use std::collections::HashSet; + +/// Counters returned by [`run`] for diagnostics. Logged at INFO once per run. +#[derive(Debug, Default, Clone)] +pub struct MigrationStats { + /// Number of `*_provider` fields reset to managed (`None`). + pub workload_fields_scrubbed: usize, + /// `true` when a dangling `primary_cloud` pointer was cleared. + pub primary_cloud_cleared: bool, +} + +/// Run the orphaned-provider reconciliation on the given `Config`. +/// +/// Synchronous — pure config mutation, no I/O. Caller persists via +/// `Config::save()` once `schema_version` is also bumped. +/// +/// Returns `anyhow::Result` for uniformity with the other migration steps in +/// [`super`] (the runner matches `Ok`/`Err` for every step). This pass has no +/// fallible operations today, so it always returns `Ok`; the signature keeps +/// the contract consistent and leaves room for future I/O-bearing logic. +pub fn run(config: &mut Config) -> anyhow::Result { + let mut stats = MigrationStats::default(); + + // Own the slug/id sets so the immutable borrow on `cloud_providers` ends + // before we take `&mut` to the workload fields below. Stored slugs/ids are + // kept verbatim (no trim) so membership matches the factory's exact, + // case-sensitive `e.slug == slug` / `e.id == id` comparisons: a provider + // stored as " openai" must NOT make a "openai:model" route look resolvable, + // since the factory would still fail it. + let known_slugs: HashSet = config + .cloud_providers + .iter() + .map(|e| e.slug.clone()) + .collect(); + let known_ids: HashSet = config + .cloud_providers + .iter() + .map(|e| e.id.clone()) + .collect(); + + let mut scrubbed = 0usize; + for (workload, field) in workload_fields(config) { + let Some(raw) = field.as_deref() else { + continue; + }; + let s = raw.trim(); + + // Managed sentinels and local providers resolve without a + // cloud_providers entry — leave them alone. + if s.is_empty() + || s == "cloud" + || s == PROVIDER_OPENHUMAN + || s.starts_with(OLLAMA_PROVIDER_PREFIX) + || s.starts_with(LM_STUDIO_PROVIDER_PREFIX) + { + continue; + } + + let scrub_reason = match s.split_once(':') { + // "openhuman:" is always the managed backend regardless of + // the suffix — normalize to None to match the bare sentinel. + Some((slug, _)) if slug.trim() == PROVIDER_OPENHUMAN => Some("openhuman-slug"), + // ":" whose slug is no longer configured — the orphan. + Some((slug, _)) if !known_slugs.contains(slug.trim()) => Some("missing-slug"), + Some(_) => None, + // Bare non-sentinel (e.g. "openai") — the factory rejects this form. + None => Some("bare-unresolvable"), + }; + + if let Some(reason) = scrub_reason { + log::info!( + "[migrations][reconcile-providers] {workload}_provider={} -> managed (None) \ + reason={reason}", + redact_provider_for_log(raw) + ); + *field = None; + scrubbed += 1; + } + } + stats.workload_fields_scrubbed = scrubbed; + + // A primary_cloud id absent from cloud_providers is a dangling pointer. + // The factory already resolves an unfound primary to the managed backend, + // so null it for consistency with the on-disk truth. + let dangling_primary = config + .primary_cloud + .as_deref() + .is_some_and(|id| !known_ids.contains(id)); + if dangling_primary { + // Don't log the raw id — primary_cloud is user-editable config; the fact + // that it dangled is the only diagnostic that matters here. + log::info!( + "[migrations][reconcile-providers] primary_cloud points at a missing cloud_providers \ + entry -> cleared (managed fallback)" + ); + config.primary_cloud = None; + stats.primary_cloud_cleared = true; + } + + log::info!( + "[migrations][reconcile-providers] done workload_fields_scrubbed={} primary_cloud_cleared={}", + stats.workload_fields_scrubbed, + stats.primary_cloud_cleared + ); + Ok(stats) +} + +/// The nine per-workload routing fields, paired with a label for logging. +/// +/// Borrowed mutably in one array literal — legal because they are disjoint +/// fields of `Config`. +/// +/// IMPORTANT: this list must stay in sync with the `*_provider` fields on +/// [`Config`]. If a tenth workload provider field is added there, add it here +/// (and bump the array length) or the migration will silently skip it. Rust +/// can't enforce this at compile time without a field-reflection macro, and a +/// serde-based count guard doesn't work because the `Option` fields +/// default to `None` and are omitted from the serialized table. +fn workload_fields(config: &mut Config) -> [(&'static str, &mut Option); 9] { + [ + ("chat", &mut config.chat_provider), + ("reasoning", &mut config.reasoning_provider), + ("agentic", &mut config.agentic_provider), + ("coding", &mut config.coding_provider), + ("memory", &mut config.memory_provider), + ("embeddings", &mut config.embeddings_provider), + ("heartbeat", &mut config.heartbeat_provider), + ("learning", &mut config.learning_provider), + ("subconscious", &mut config.subconscious_provider), + ] +} + +/// Redact a provider string for logging. Provider values are user-editable and +/// could carry sensitive content in the model segment, so keep only the +/// non-sensitive slug (the useful diagnostic) and mask the rest. Per the +/// project rule: never log secrets or full PII. +fn redact_provider_for_log(raw: &str) -> String { + match raw.trim().split_once(':') { + Some((slug, _)) => format!("{}:", slug.trim()), + None => "".to_string(), + } +} + +#[cfg(test)] +#[path = "reconcile_orphaned_providers_tests.rs"] +mod tests; diff --git a/src/openhuman/migrations/reconcile_orphaned_providers_tests.rs b/src/openhuman/migrations/reconcile_orphaned_providers_tests.rs new file mode 100644 index 000000000..d2f2c8ab7 --- /dev/null +++ b/src/openhuman/migrations/reconcile_orphaned_providers_tests.rs @@ -0,0 +1,209 @@ +//! Tests for migration 5 → 6 (`reconcile_orphaned_providers`). + +use super::*; +use crate::openhuman::config::schema::cloud_providers::CloudProviderCreds; +use crate::openhuman::config::Config; + +/// Minimal cloud-provider entry for tests — only `id` and `slug` matter here. +fn provider(id: &str, slug: &str) -> CloudProviderCreds { + CloudProviderCreds { + id: id.to_string(), + slug: slug.to_string(), + ..Default::default() + } +} + +#[test] +fn scrubs_orphaned_chat_provider_to_managed() { + // The reported bug: chat points at OpenAI, but OpenAI was removed. + let mut config = Config::default(); + config.chat_provider = Some("openai:gpt-4o".to_string()); + // cloud_providers has no `openai` entry. + + let stats = run(&mut config).expect("migration should succeed"); + + assert_eq!(stats.workload_fields_scrubbed, 1); + assert_eq!( + config.chat_provider, None, + "orphaned chat_provider -> managed" + ); +} + +#[test] +fn keeps_provider_when_slug_still_present() { + let mut config = Config::default(); + config.chat_provider = Some("openai:gpt-4o".to_string()); + config.cloud_providers = vec![provider("p_openai_1", "openai")]; + + let stats = run(&mut config).expect("migration should succeed"); + + assert_eq!(stats.workload_fields_scrubbed, 0); + assert_eq!(config.chat_provider.as_deref(), Some("openai:gpt-4o")); +} + +#[test] +fn scrubs_when_stored_slug_has_whitespace_factory_would_reject() { + // The factory compares `e.slug == slug` exactly (no trim). A provider stored + // as " openai" does NOT resolve a "openai:gpt-4o" route, so the migration + // must scrub it rather than leaving a reference the factory rejects. + let mut config = Config::default(); + config.chat_provider = Some("openai:gpt-4o".to_string()); + config.cloud_providers = vec![provider("p1", " openai")]; + + let stats = run(&mut config).expect("migration should succeed"); + + assert_eq!(stats.workload_fields_scrubbed, 1); + assert_eq!(config.chat_provider, None); +} + +#[test] +fn preserves_temperature_suffix_on_valid_slug() { + let mut config = Config::default(); + config.reasoning_provider = Some("openai:gpt-4o@0.3".to_string()); + config.cloud_providers = vec![provider("p_openai_1", "openai")]; + + let stats = run(&mut config).expect("migration should succeed"); + + assert_eq!(stats.workload_fields_scrubbed, 0); + assert_eq!( + config.reasoning_provider.as_deref(), + Some("openai:gpt-4o@0.3") + ); +} + +#[test] +fn normalizes_openhuman_colon_to_none() { + let mut config = Config::default(); + config.memory_provider = Some("openhuman:".to_string()); + config.embeddings_provider = Some("openhuman:reasoning-v1".to_string()); + + let stats = run(&mut config).expect("migration should succeed"); + + assert_eq!(stats.workload_fields_scrubbed, 2); + assert_eq!(config.memory_provider, None); + assert_eq!(config.embeddings_provider, None); +} + +#[test] +fn leaves_sentinels_and_local_providers_untouched() { + let mut config = Config::default(); + config.chat_provider = Some("openhuman".to_string()); + config.reasoning_provider = Some("cloud".to_string()); + config.agentic_provider = Some(String::new()); + config.coding_provider = None; + config.memory_provider = Some("ollama:llama3".to_string()); + config.embeddings_provider = Some("lmstudio:nomic-embed".to_string()); + + let stats = run(&mut config).expect("migration should succeed"); + + assert_eq!(stats.workload_fields_scrubbed, 0); + assert_eq!(config.chat_provider.as_deref(), Some("openhuman")); + assert_eq!(config.reasoning_provider.as_deref(), Some("cloud")); + assert_eq!(config.agentic_provider.as_deref(), Some("")); + assert_eq!(config.coding_provider, None); + assert_eq!(config.memory_provider.as_deref(), Some("ollama:llama3")); + assert_eq!( + config.embeddings_provider.as_deref(), + Some("lmstudio:nomic-embed") + ); +} + +#[test] +fn scrubs_bare_unresolvable_string() { + // A bare non-sentinel like "openai" (no colon) is rejected by the factory. + let mut config = Config::default(); + config.chat_provider = Some("openai".to_string()); + + let stats = run(&mut config).expect("migration should succeed"); + + assert_eq!(stats.workload_fields_scrubbed, 1); + assert_eq!(config.chat_provider, None); +} + +#[test] +fn scrubs_every_orphaned_workload() { + let mut config = Config::default(); + for field in [ + &mut config.chat_provider, + &mut config.reasoning_provider, + &mut config.agentic_provider, + &mut config.coding_provider, + &mut config.memory_provider, + &mut config.embeddings_provider, + &mut config.heartbeat_provider, + &mut config.learning_provider, + &mut config.subconscious_provider, + ] { + *field = Some("ghost:model-x".to_string()); + } + + let stats = run(&mut config).expect("migration should succeed"); + + assert_eq!(stats.workload_fields_scrubbed, 9); + assert_eq!(config.chat_provider, None); + assert_eq!(config.subconscious_provider, None); +} + +#[test] +fn clears_dangling_primary_cloud() { + let mut config = Config::default(); + config.primary_cloud = Some("p_openhuman_missing".to_string()); + config.cloud_providers = vec![provider("p_openai_1", "openai")]; + + let stats = run(&mut config).expect("migration should succeed"); + + assert!(stats.primary_cloud_cleared); + assert_eq!(config.primary_cloud, None); +} + +#[test] +fn keeps_valid_primary_cloud() { + let mut config = Config::default(); + config.primary_cloud = Some("p_openai_1".to_string()); + config.cloud_providers = vec![provider("p_openai_1", "openai")]; + + let stats = run(&mut config).expect("migration should succeed"); + + assert!(!stats.primary_cloud_cleared); + assert_eq!(config.primary_cloud.as_deref(), Some("p_openai_1")); +} + +#[test] +fn clean_config_is_a_no_op() { + let mut config = Config::default(); + + let stats = run(&mut config).expect("migration should succeed"); + + assert_eq!(stats.workload_fields_scrubbed, 0); + assert!(!stats.primary_cloud_cleared); +} + +#[test] +fn redact_provider_for_log_masks_model_keeps_slug() { + // Slug retained as a diagnostic; model segment masked. + assert_eq!( + redact_provider_for_log("openai:gpt-4o"), + "openai:" + ); + assert_eq!( + redact_provider_for_log(" ghost:secret-model@0.7 "), + "ghost:" + ); + // Bare string has no slug to keep — fully masked. + assert_eq!(redact_provider_for_log("openai"), ""); +} + +#[test] +fn idempotent_second_run_scrubs_nothing() { + let mut config = Config::default(); + config.chat_provider = Some("openai:gpt-4o".to_string()); + config.primary_cloud = Some("p_missing".to_string()); + + let first = run(&mut config).expect("first run should succeed"); + assert_eq!(first.workload_fields_scrubbed, 1); + assert!(first.primary_cloud_cleared); + + let second = run(&mut config).expect("second run should succeed"); + assert_eq!(second.workload_fields_scrubbed, 0); + assert!(!second.primary_cloud_cleared); +}