fix(config): reconcile orphaned provider references on upgrade (#2759) (#2761)

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
sanil-23
2026-05-27 20:48:34 +05:30
committed by GitHub
co-authored by Claude Opus 4.7
parent 82dd26056b
commit 26295df979
4 changed files with 504 additions and 25 deletions
+57 -25
View File
@@ -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)"
);
}
}
}
+35
View File
@@ -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]
@@ -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:<model>" → local Ollama
//! "lmstudio:<model>" → local LM Studio
//! "<slug>:<model>" → the cloud_providers entry whose slug == <slug>
//! ```
//!
//! A `"<slug>:<model>"` 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 `"<slug>:<model>"` whose slug
//! is absent from `cloud_providers`, the always-managed `"openhuman:<model>"`
//! 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:<model>"` → `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<MigrationStats> {
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<String> = config
.cloud_providers
.iter()
.map(|e| e.slug.clone())
.collect();
let known_ids: HashSet<String> = 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:<model>" 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"),
// "<slug>:<model>" 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<String>` fields
/// default to `None` and are omitted from the serialized table.
fn workload_fields(config: &mut Config) -> [(&'static str, &mut Option<String>); 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!("{}:<redacted>", slug.trim()),
None => "<redacted>".to_string(),
}
}
#[cfg(test)]
#[path = "reconcile_orphaned_providers_tests.rs"]
mod tests;
@@ -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:<redacted>"
);
assert_eq!(
redact_provider_for_log(" ghost:secret-model@0.7 "),
"ghost:<redacted>"
);
// Bare string has no slug to keep — fully masked.
assert_eq!(redact_provider_for_log("openai"), "<redacted>");
}
#[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);
}