diff --git a/desktop/scripts/check-file-sizes.mjs b/desktop/scripts/check-file-sizes.mjs index 1963acc3bc..9b491beab8 100644 --- a/desktop/scripts/check-file-sizes.mjs +++ b/desktop/scripts/check-file-sizes.mjs @@ -123,8 +123,7 @@ const overrides = new Map([ // activity-feed threads avatar_url into build_managed_agent_summary for the // assistant-bubble pinned snapshot. // +1 for agent_pubkey field in setup payload (config-nudge card wire). - // persona-blank-fallback: resolve_effective_prompt_model_provider gains a - // record_provider param + applies persona_field_with_record_fallback. +5 lines. + // effective-config resolver: single resolver replaces the old fallback chain. // global-agent-config: spawn_agent_child loads global config and merges as // lowest env layer (+8 lines). Queued to split. ["src-tauri/src/managed_agents/runtime.rs", 2216], @@ -208,7 +207,8 @@ const overrides = new Map([ // mapper. This is the existing API boundary; split remains queued. // team-instructions-first-class: createManagedAgent Tauri bridge threads the // new teamId input through to the backend (+1 line). - ["src/shared/api/tauri.ts", 1305], + // +2 for model_source field in RawManagedAgent + fromRawManagedAgent mapping. + ["src/shared/api/tauri.ts", 1307], // doctor-npm-eacces-preflight: hint field added to InstallStepResult (+1 line). // codex-acp-package-swap: "adapter_outdated" variant added to AcpAvailabilityStatus (+1 line). // doctor-install-reliability: AuthStatus tagged union + nodeRequired/authStatus/ @@ -442,7 +442,13 @@ const overrides = new Map([ // (if let Some(provider_update) = input.provider { record.provider = provider_update; }). // +8: harness_override thread-through in update_managed_agent so a deliberate // Custom pin routes to update_time_agent_command_override (comment + call). - ["src-tauri/src/commands/agent_models.rs", 1079], + // +22 (1079 -> 1101): Finding 2 — model discovery now resolves through + // resolve_effective_model_provider instead of raw record bytes + // (saved_agent_model_discovery_config takes personas/global and the + // get_agent_models call site loads global config), plus + // apply_model_provider_prompt_update's linked-instance write-guard + // extraction and its regression tests. + ["src-tauri/src/commands/agent_models.rs", 1101], // global-agent-config: get_agent_config_surface / write_agent_config_field / // put_agent_session_config commands + GlobalAgentConfig serde types. New file // in this PR; queued to split with the command module refactor. @@ -450,7 +456,14 @@ const overrides = new Map([ // is_safe_to_reveal allowlist + baked_env_thinking_effort_is_unmasked test. // +1: doctor-install-reliability: login_hint: None added to goose_runtime test stub. // +1: doctor-install-reliability review fixes: auth_probe_args: None added to stub. - ["src-tauri/src/commands/agent_config.rs", 1021], + // +59 (1021 -> 1080): Finding 2 (definition-authoritative config surface) — + // resolve_config_surface now clears a linked instance's own + // system_prompt/model/provider before computing had_* so stale + // materialized snapshot bytes can never be tagged BuzzExplicit and + // shadow the definition/global fallthrough; the dead persona-model + // re-tag branch this replaced is removed, plus two new regression tests + // pinning the stale-record-never-outranks-definition contract. + ["src-tauri/src/commands/agent_config.rs", 1080], // codex-install-auto-restart review-fixes: should_restart_after_install // takes pid_alive:bool (pure predicate, no OS-dependent call); 3 racy // cache tests replaced with 6 pure availability_drift predicate tests; diff --git a/desktop/src-tauri/src/commands/agent_config.rs b/desktop/src-tauri/src/commands/agent_config.rs index f6141a53eb..b26efa2a89 100644 --- a/desktop/src-tauri/src/commands/agent_config.rs +++ b/desktop/src-tauri/src/commands/agent_config.rs @@ -38,6 +38,13 @@ pub struct RuntimeFileConfigSubset { /// Resolve the config surface with persona and global default values applied. /// +/// Linked instances are definition-authoritative: the record's own +/// system_prompt/model/provider are cleared before applying, so a stale +/// materialized snapshot can never shadow the persona's current values or a +/// blank-definition fallthrough to global defaults (mirrors +/// `effective_config::resolve_linked`). Definition-less instances keep their +/// own explicit values. +/// /// The pipeline: resolve the linked persona's prompt/model/provider, inject /// each into the record only where the record lacks its own value, let /// `read_config_surface` tag those injected fields `BuzzExplicit`, then re-tag @@ -58,6 +65,22 @@ fn resolve_config_surface( session_cache: Option<&SessionConfigCache>, global: &GlobalAgentConfig, ) -> RuntimeConfigSurface { + // Linked instances are definition-authoritative (mirrors + // `effective_config::resolve_linked`): the record's own + // system_prompt/model/provider fields are, at best, a stale materialized + // snapshot from the last `apply_persona_snapshot` — never a legitimate + // live override, since `update_managed_agent` blocks writing these three + // fields for linked instances. Clear them before computing `had_*` below + // so a stale byte can never masquerade as BuzzExplicit and suppress + // definition/global injection. Env var overrides (set via the advanced + // env-vars editor) are untouched — those remain a legitimate + // per-instance override regardless of link status. + if record.persona_id.is_some() { + record.system_prompt = None; + record.model = None; + record.provider = None; + } + let had_prompt = record.system_prompt.is_some() || record.env_vars.contains_key("BUZZ_ACP_SYSTEM_PROMPT"); let had_model = record.model.is_some(); @@ -171,22 +194,6 @@ fn resolve_config_surface( retag_global_default(&mut surface.normalized.provider); } - // Re-tag persona-snapshotted model from BuzzExplicit to PersonaDefault. - // Persona-created agents have record.model set at create time from the - // persona snapshot — had_model is true, but the model came from the persona, - // not an explicit user choice. Re-tag when the record model matches the - // persona model and no live override is active. Only applies when a persona - // is actually linked — non-persona agents with an explicit model keep BuzzExplicit. - if had_model && !model_overridden && record.persona_id.is_some() { - if let (Some(ref record_model), Some(ref persona_model_val)) = - (&record.model, &persona_model) - { - if record_model == persona_model_val { - retag_persona_default(&mut surface.normalized.model); - } - } - } - surface } @@ -699,11 +706,64 @@ mod tests { } } - /// A model the user set explicitly in Buzz must never be re-tagged to - /// `PersonaDefault`, even when the linked persona also has a model. + /// Definition-authoritative: a stale materialized `record.model` on a + /// linked instance must never outrank (or even be consulted against) the + /// linked persona's model. `update_managed_agent` already blocks writing + /// model/provider/prompt for linked instances, so a non-`None` value here + /// can only be leftover snapshot bytes from before a persona edit — the + /// panel must report the persona's current model, tagged `PersonaDefault`, + /// not the stale byte as `BuzzExplicit`. + #[test] + fn linked_stale_record_model_never_outranks_persona_model() { + let mut record = agent_record(); + record.model = Some("stale-explicit-model".to_string()); + let personas = vec![persona_with_model("persona-model")]; + + let surface = resolve_config_surface( + record, + &personas, + Some(goose_runtime()), + None, + &Default::default(), + ); + + let model = surface.normalized.model.as_ref().expect("model resolved"); + assert_eq!(model.value.as_deref(), Some("persona-model")); + assert_eq!(model.origin, ConfigOrigin::PersonaDefault); + } + + /// Definition-authoritative, blank-definition case: a linked instance + /// whose persona has no model of its own must fall through to the global + /// default, tagged `GlobalDefault` — mirroring + /// `effective_config::resolve_linked`'s `None => global` arm. A stale + /// materialized record model must not shadow this fallthrough either. #[test] - fn explicit_record_model_outranks_persona_and_keeps_buzz_explicit_origin() { + fn linked_blank_definition_model_falls_through_to_global_default() { let mut record = agent_record(); + record.model = Some("stale-explicit-model".to_string()); + let mut persona = persona_with_model("unused"); + persona.model = None; + let personas = vec![persona]; + let global = crate::managed_agents::GlobalAgentConfig { + model: Some("global-model".to_string()), + ..Default::default() + }; + + let surface = + resolve_config_surface(record, &personas, Some(goose_runtime()), None, &global); + + let model = surface.normalized.model.as_ref().expect("model resolved"); + assert_eq!(model.value.as_deref(), Some("global-model")); + assert_eq!(model.origin, ConfigOrigin::GlobalDefault); + } + + /// A definition-less (no `persona_id`) instance's own explicit model IS + /// authoritative — the stale-record clearing above is scoped to linked + /// instances only. + #[test] + fn definition_less_explicit_record_model_keeps_buzz_explicit_origin() { + let mut record = agent_record(); + record.persona_id = None; record.model = Some("explicit-model".to_string()); let personas = vec![persona_with_model("persona-model")]; diff --git a/desktop/src-tauri/src/commands/agent_models.rs b/desktop/src-tauri/src/commands/agent_models.rs index 97772b12a5..3e403e21a6 100644 --- a/desktop/src-tauri/src/commands/agent_models.rs +++ b/desktop/src-tauri/src/commands/agent_models.rs @@ -71,9 +71,13 @@ pub async fn get_agent_models( // ModelPicker can persist a selected model but not rewrite the saved // provider/env snapshot, and runtime spawn reads that same snapshot. - // Discover models against the record snapshot so an out-of-date persona - // cannot offer models for a provider this agent will not launch with. - let discovery = saved_agent_model_discovery_config(record, &effective_command); + // Discover models against the resolver's effective model/provider — + // definition-authoritative for linked instances — so discovery can + // never query a stale provider this agent will not actually launch + // with. + let global = crate::managed_agents::load_global_agent_config(&app).unwrap_or_default(); + let discovery = + saved_agent_model_discovery_config(record, &effective_command, &personas, &global); ( resolved, @@ -136,26 +140,37 @@ struct SavedAgentModelDiscoveryConfig { env: BTreeMap, } +/// Resolve the model/provider discovery config from the same authoritative +/// source spawn uses (`resolve_effective_model_provider`) — linked instances +/// read their definition, never a stale materialized `record.model`/ +/// `record.provider`, so model discovery cannot query a provider this agent +/// will not actually launch with. Definition-less instances keep their own +/// record values, matching spawn's `resolve_definition_less` arm. fn saved_agent_model_discovery_config( record: &crate::managed_agents::ManagedAgentRecord, agent_command: &str, + personas: &[crate::managed_agents::AgentDefinition], + global: &crate::managed_agents::GlobalAgentConfig, ) -> SavedAgentModelDiscoveryConfig { + let (effective_model, effective_provider) = + crate::managed_agents::resolve_effective_model_provider(record, personas, global); + let mut derived_env = BTreeMap::new(); if let Some(meta) = known_acp_runtime(agent_command) { for (key, value) in crate::managed_agents::runtime_metadata_env_vars( meta.model_env_var, meta.provider_env_var, meta.provider_locked, - record.model.as_deref(), - record.provider.as_deref(), + effective_model.as_deref(), + effective_provider.as_deref(), ) { derived_env.insert(key.to_string(), value.to_string()); } } SavedAgentModelDiscoveryConfig { - model: record.model.clone(), - provider: record.provider.clone(), + model: effective_model, + provider: effective_provider, env: crate::managed_agents::merged_user_env(&derived_env, &record.env_vars), } } @@ -766,6 +781,35 @@ async fn discover_databricks_models( })) } +/// Apply an `UpdateManagedAgentRequest`'s model/provider/system_prompt patch +/// to `record`, enforcing the linked-instance write guard: a definition-linked +/// record's model/provider/prompt are definition-authoritative (see +/// `effective_config::resolve_linked`), so writes to these three fields are +/// silently dropped for a linked instance rather than persisting a byte the +/// resolver will never read. Definition-less instances accept the patch +/// as-is. Extracted so the guard is exercised by both `update_managed_agent` +/// and its regression tests — a test that reimplements this check instead of +/// calling it can go green after the real guard is deleted. +fn apply_model_provider_prompt_update( + record: &mut crate::managed_agents::ManagedAgentRecord, + model: Option>, + provider: Option>, + system_prompt: Option>, +) { + if record.persona_id.is_some() { + return; + } + if let Some(model_update) = model { + record.model = model_update; + } + if let Some(provider_update) = provider { + record.provider = provider_update; + } + if let Some(prompt_update) = system_prompt { + record.system_prompt = prompt_update; + } +} + /// Update mutable fields on an existing managed agent record. /// /// Does NOT auto-restart the agent. Runtime config changes (system prompt, @@ -804,15 +848,12 @@ pub async fn update_managed_agent( name_changed = true; } } - if let Some(model_update) = input.model { - record.model = model_update; - } - if let Some(provider_update) = input.provider { - record.provider = provider_update; - } - if let Some(prompt_update) = input.system_prompt { - record.system_prompt = prompt_update; - } + apply_model_provider_prompt_update( + record, + input.model, + input.provider, + input.system_prompt, + ); if let Some(parallelism) = input.parallelism { record.parallelism = parallelism; } diff --git a/desktop/src-tauri/src/commands/agent_models_tests.rs b/desktop/src-tauri/src/commands/agent_models_tests.rs index 0f99927c9e..092b71cfd1 100644 --- a/desktop/src-tauri/src/commands/agent_models_tests.rs +++ b/desktop/src-tauri/src/commands/agent_models_tests.rs @@ -164,7 +164,7 @@ fn redaction_env_records_value_used_for_request() { } #[test] -fn saved_agent_model_discovery_uses_record_snapshot() { +fn saved_agent_model_discovery_uses_record_snapshot_for_definition_less_agent() { let record: crate::managed_agents::ManagedAgentRecord = serde_json::from_str( r#"{ "pubkey": "abcd1234", @@ -193,7 +193,12 @@ fn saved_agent_model_discovery_uses_record_snapshot() { ) .expect("sample managed agent record"); - let config = saved_agent_model_discovery_config(&record, "goose"); + let config = saved_agent_model_discovery_config( + &record, + "goose", + &[], + &crate::managed_agents::GlobalAgentConfig::default(), + ); assert_eq!(config.model.as_deref(), Some("record-model")); assert_eq!(config.provider.as_deref(), Some("databricks")); @@ -212,6 +217,74 @@ fn saved_agent_model_discovery_uses_record_snapshot() { assert!(!config.env.contains_key("BUZZ_PRIVATE_KEY")); } +/// Definition-authoritative: a linked agent's stale materialized +/// `record.model`/`record.provider` must never drive model discovery — the +/// linked definition's current model/provider wins, mirroring spawn's +/// `resolve_effective_model_provider`. +#[test] +fn saved_agent_model_discovery_ignores_stale_record_for_linked_agent() { + let record: crate::managed_agents::ManagedAgentRecord = serde_json::from_str( + r#"{ + "pubkey": "abcd1234", + "name": "test-agent", + "persona_id": "persona-1", + "private_key_nsec": "nsec1fake", + "relay_url": "wss://localhost:3000", + "acp_command": "buzz-acp", + "agent_command": "goose", + "agent_args": [], + "mcp_command": "", + "turn_timeout_seconds": 320, + "system_prompt": null, + "model": "stale-record-model", + "provider": "stale-record-provider", + "env_vars": {}, + "created_at": "2026-01-01T00:00:00Z", + "updated_at": "2026-01-01T00:00:00Z", + "last_started_at": null, + "last_stopped_at": null, + "last_exit_code": null, + "last_error": null + }"#, + ) + .expect("sample managed agent record"); + + let persona = crate::managed_agents::AgentDefinition { + id: "persona-1".to_string(), + display_name: "Persona".to_string(), + avatar_url: None, + system_prompt: "You are a persona.".to_string(), + runtime: None, + model: Some("persona-model".to_string()), + provider: Some("anthropic".to_string()), + name_pool: Vec::new(), + is_builtin: false, + is_active: true, + source_team: None, + source_team_persona_slug: None, + env_vars: BTreeMap::new(), + respond_to: None, + respond_to_allowlist: Vec::new(), + parallelism: None, + created_at: "".to_string(), + updated_at: "".to_string(), + }; + + let config = saved_agent_model_discovery_config( + &record, + "goose", + &[persona], + &crate::managed_agents::GlobalAgentConfig::default(), + ); + + assert_eq!(config.model.as_deref(), Some("persona-model")); + assert_eq!(config.provider.as_deref(), Some("anthropic")); + assert_eq!( + config.env.get("GOOSE_MODEL").map(String::as_str), + Some("persona-model") + ); +} + // --------------------------------------------------------------------------- // Databricks provider detection // --------------------------------------------------------------------------- @@ -252,6 +325,104 @@ fn update_request_turn_timeout_parses_for_wire_compat() { assert_eq!(req.turn_timeout_seconds, Some(9999)); } +// --------------------------------------------------------------------------- +// Linked-instance write guard (model/provider/prompt) +// --------------------------------------------------------------------------- + +#[test] +fn linked_instance_ignores_model_provider_prompt_writes() { + let mut record: crate::managed_agents::ManagedAgentRecord = serde_json::from_str( + r#"{ + "pubkey": "linked1", + "name": "linked-agent", + "persona_id": "p1", + "private_key_nsec": "nsec1fake", + "relay_url": "wss://localhost:3000", + "acp_command": "buzz-acp", + "agent_command": "goose", + "agent_args": [], + "mcp_command": "", + "turn_timeout_seconds": 320, + "system_prompt": null, + "model": null, + "provider": null, + "env_vars": {}, + "created_at": "2026-01-01T00:00:00Z", + "updated_at": "2026-01-01T00:00:00Z", + "last_started_at": null, + "last_stopped_at": null, + "last_exit_code": null, + "last_error": null + }"#, + ) + .expect("linked agent record"); + + let is_linked = record.persona_id.is_some(); + assert!(is_linked, "test setup: record must be linked"); + + crate::commands::agent_models::apply_model_provider_prompt_update( + &mut record, + Some(Some("explicit-model".to_string())), + Some(Some("explicit-prov".to_string())), + Some(Some("explicit-prompt".to_string())), + ); + + assert!( + record.model.is_none(), + "linked record model must not be updated" + ); + assert!( + record.provider.is_none(), + "linked record provider must not be updated" + ); + assert!( + record.system_prompt.is_none(), + "linked record system_prompt must not be updated" + ); +} + +#[test] +fn definition_less_instance_accepts_model_provider_prompt_writes() { + let mut record: crate::managed_agents::ManagedAgentRecord = serde_json::from_str( + r#"{ + "pubkey": "standalone1", + "name": "standalone-agent", + "private_key_nsec": "nsec1fake", + "relay_url": "wss://localhost:3000", + "acp_command": "buzz-acp", + "agent_command": "goose", + "agent_args": [], + "mcp_command": "", + "turn_timeout_seconds": 320, + "system_prompt": null, + "model": null, + "provider": null, + "env_vars": {}, + "created_at": "2026-01-01T00:00:00Z", + "updated_at": "2026-01-01T00:00:00Z", + "last_started_at": null, + "last_stopped_at": null, + "last_exit_code": null, + "last_error": null + }"#, + ) + .expect("standalone agent record"); + + let is_linked = record.persona_id.is_some(); + assert!(!is_linked, "test setup: record must not be linked"); + + crate::commands::agent_models::apply_model_provider_prompt_update( + &mut record, + Some(Some("new-model".to_string())), + Some(Some("new-prov".to_string())), + Some(Some("new-prompt".to_string())), + ); + + assert_eq!(record.model.as_deref(), Some("new-model")); + assert_eq!(record.provider.as_deref(), Some("new-prov")); + assert_eq!(record.system_prompt.as_deref(), Some("new-prompt")); +} + #[test] fn is_databricks_provider_matches_both_variants() { assert!(is_databricks_provider(Some("databricks"))); diff --git a/desktop/src-tauri/src/commands/agents.rs b/desktop/src-tauri/src/commands/agents.rs index ebae5d4514..44b5ba8852 100644 --- a/desktop/src-tauri/src/commands/agents.rs +++ b/desktop/src-tauri/src/commands/agents.rs @@ -292,16 +292,16 @@ fn resolve_created_avatar_url( #[cfg(feature = "mesh-llm")] async fn ensure_relay_mesh_for_record( app: &AppHandle, - record: &ManagedAgentRecord, + model_id: Option<&str>, allow_fresh_create_start: bool, ) -> Result<(), String> { - crate::commands::ensure_relay_mesh_for_record(app, record, allow_fresh_create_start).await + crate::commands::ensure_relay_mesh_for_record(app, model_id, allow_fresh_create_start).await } #[cfg(not(feature = "mesh-llm"))] async fn ensure_relay_mesh_for_record( _app: &AppHandle, - _record: &ManagedAgentRecord, + _model_id: Option<&str>, _allow_fresh_create_start: bool, ) -> Result<(), String> { Ok(()) @@ -331,7 +331,22 @@ pub(super) async fn start_local_agent_with_preflight( return Err(format!("agent {pubkey} is not a local agent")); } - ensure_relay_mesh_for_record(app, &record_snapshot, allow_fresh_create_start).await?; + // Preflight against the same resolution spawn uses — `resolve_effective_config` + // (definition → global fallback) — never the record's own `provider`/`model`/ + // `relay_mesh` bytes. For a linked instance this reads the CURRENT definition + // directly, so a definition edit that flips `provider` to/from relay-mesh + // between saves is reflected here without needing a prospective re-snapshot; + // for a global-inherited blank definition, it also folds in the global + // default, which record-byte sniffing could never see. + let personas = load_personas(app).unwrap_or_default(); + let global = crate::managed_agents::load_global_agent_config(app).unwrap_or_default(); + let mesh_model_id = + crate::managed_agents::effective_config::resolve_effective_relay_mesh_model_id( + &record_snapshot, + &personas, + &global, + ); + ensure_relay_mesh_for_record(app, mesh_model_id.as_deref(), allow_fresh_create_start).await?; let _store_guard = state .managed_agents_store_lock @@ -355,9 +370,16 @@ pub(super) async fn start_local_agent_with_preflight( // at the end — avoids a second disk read for the same file in the same call. let personas = load_personas(app).unwrap_or_default(); if let Some(persona_id) = record.persona_id.clone() { - if let Some(persona) = personas.iter().find(|p| p.id == persona_id) { - crate::managed_agents::persona_events::apply_persona_snapshot(record, persona); - record.updated_at = crate::util::now_iso(); + match personas.iter().find(|p| p.id == persona_id) { + Some(persona) => { + crate::managed_agents::persona_events::apply_persona_snapshot(record, persona); + record.updated_at = crate::util::now_iso(); + } + None => { + return Err( + crate::managed_agents::effective_config::ORPHANED_INSTANCE_ERROR.to_string(), + ); + } } } start_managed_agent_process(app, record, &mut runtimes, Some(owner_hex))?; diff --git a/desktop/src-tauri/src/commands/agents_deploy.rs b/desktop/src-tauri/src/commands/agents_deploy.rs index be132225ea..af785711d5 100644 --- a/desktop/src-tauri/src/commands/agents_deploy.rs +++ b/desktop/src-tauri/src/commands/agents_deploy.rs @@ -4,42 +4,36 @@ use tauri::AppHandle; +#[cfg(test)] +use crate::managed_agents::AgentDefinition; use crate::{ app_state::AppState, - managed_agents::{load_personas, AgentDefinition, ManagedAgentRecord}, + managed_agents::{load_personas, ManagedAgentRecord}, relay::relay_ws_url_with_override, }; /// Resolve the deploy-specific structured model/provider for a managed agent. /// -/// Deploy uses **live-persona-first** precedence so remote agents receive -/// current config after a persona update, without requiring delete+recreate. -/// Unlike local spawn (which re-snapshots the persona onto `record` at the -/// start of every spawn), provider start does not re-snapshot — so the -/// record may hold a stale snapshot while the linked persona has moved on. +/// Delegates to the single effective-config resolver which enforces +/// definition-authoritative semantics for linked instances: +/// - **Linked:** definition → global. Stale record bytes are never consulted. +/// - **Definition-less:** instance → global. +/// - **Orphaned:** returns `(None, None)` — spawn is blocked elsewhere. /// -/// Precedence: live-persona → record (snapshot fallback) → global. -/// Symmetric for both model and provider. +/// Both local spawn and deploy now use the same resolver, so they can never +/// disagree on what model/provider an agent runs with. /// /// Exported `pub(crate)` for unit testing. -pub(crate) fn resolve_deploy_model_provider<'a>( - record: &'a ManagedAgentRecord, - personas: &'a [AgentDefinition], - global: &'a crate::managed_agents::GlobalAgentConfig, -) -> (Option<&'a str>, Option<&'a str>) { - let live_persona = record - .persona_id - .as_deref() - .and_then(|pid| personas.iter().find(|p| p.id == pid)); - let model = live_persona - .and_then(|p| p.model.as_deref()) - .or(record.model.as_deref()) - .or(global.model.as_deref()); - let provider = live_persona - .and_then(|p| p.provider.as_deref()) - .or(record.provider.as_deref()) - .or(global.provider.as_deref()); - (model, provider) +#[cfg(test)] +pub(crate) fn resolve_deploy_model_provider( + record: &ManagedAgentRecord, + personas: &[AgentDefinition], + global: &crate::managed_agents::GlobalAgentConfig, +) -> (Option, Option) { + crate::managed_agents::effective_config::resolve_effective_model_provider_pair( + record, personas, global, + ) + .unwrap_or((None, None)) } /// Build the standard agent JSON payload for provider deploy calls. @@ -84,15 +78,16 @@ pub(super) fn build_deploy_payload( let merged_env = crate::managed_agents::merged_user_env(&global_persona_merged, &record.env_vars); - // Resolve the deploy-specific structured provider/model. Uses the deploy - // resolver with live-persona → record → global precedence. let personas = load_personas(app).unwrap_or_default(); - let (effective_model, effective_provider) = - resolve_deploy_model_provider(record, &personas, &global_config); - let (effective_model, effective_provider) = ( - effective_model.map(str::to_string), - effective_provider.map(str::to_string), - ); + let cfg = crate::managed_agents::effective_config::resolve_effective_config( + record, + &personas, + &global_config, + ) + .require_resolved()?; + let effective_model = cfg.model.value; + let effective_provider = cfg.provider.value; + let effective_prompt = cfg.system_prompt.value; Ok(deploy_payload_json( record, @@ -102,6 +97,7 @@ pub(super) fn build_deploy_payload( ), effective_model, effective_provider, + effective_prompt, merged_env, )) } @@ -114,36 +110,25 @@ pub(super) fn deploy_payload_json( relay_url: String, effective_model: Option, effective_provider: Option, + effective_prompt: Option, merged_env: std::collections::BTreeMap, ) -> serde_json::Value { serde_json::json!({ "name": &record.name, - // Resolve the per-agent pin against the active workspace relay here: - // this payload crosses the host boundary to a remote provider harness - // that has no notion of the desktop's workspace, so the blank→workspace - // fallback (otherwise applied at read-time in `effective_agent_relay_url`) - // must be materialized into a concrete URL before serializing. "relay_url": relay_url, "private_key_nsec": &record.private_key_nsec, "auth_tag": &record.auth_tag, "agent_command": &record.agent_command, "agent_args": &record.agent_args, - "system_prompt": &record.system_prompt, + "system_prompt": effective_prompt, "model": effective_model, - // Structured provider from the persona record. Providers that don't - // yet read this field will fall back to env_vars or their own default - // — no protocol break. "provider": effective_provider, "turn_timeout_seconds": record.turn_timeout_seconds, "idle_timeout_seconds": record.idle_timeout_seconds, "max_turn_duration_seconds": record.max_turn_duration_seconds, "parallelism": record.parallelism, - // Inbound author gate. Providers that don't yet read these fall back - // to the harness default (`owner-only`) — no protocol break. "respond_to": record.respond_to, "respond_to_allowlist": &record.respond_to_allowlist, - // Merged persona + agent env vars. Providers that don't read this - // field will simply ignore it — no protocol break. "env_vars": merged_env, }) } diff --git a/desktop/src-tauri/src/commands/agents_tests.rs b/desktop/src-tauri/src/commands/agents_tests.rs index 1b2e0ed179..e32fc1cfe4 100644 --- a/desktop/src-tauri/src/commands/agents_tests.rs +++ b/desktop/src-tauri/src/commands/agents_tests.rs @@ -126,57 +126,56 @@ fn build_agent_archive_request_attaches_owner_auth_and_retired_reason() { })); } -/// Deploy-path regression for Fix 1 of Thufir pass-2: a persona-linked -/// provider agent with a stale record snapshot must use the live persona -/// model/provider in the deploy payload, not the stale record values. -/// -/// Scenario: agent was created with persona at model="old-model"/provider="old-prov". -/// The persona was subsequently updated to "new-model"/"new-prov" but the record -/// was NOT re-snapshotted (provider start skips re-snapshot; local spawn does it). -/// The deploy resolver must use the current persona values. -/// -/// Fails against `resolve_effective_model_provider` (record-first precedence), -/// which would return "old-model"/"old-prov" from the stale record. +/// Deploy resolver uses definition model/provider, ignoring stale record. #[test] -fn deploy_resolver_uses_live_persona_over_stale_record_snapshot() { - // Record holds the stale snapshot (created when persona had old values). +fn deploy_resolver_uses_definition_over_stale_record() { let record = bare_agent_record(Some("p1"), Some("old-model"), Some("old-prov")); - // Live persona has been updated since the record was snapshotted. let personas = vec![persona_record("p1", Some("new-model"), Some("new-prov"))]; let global = crate::managed_agents::GlobalAgentConfig::default(); let (model, provider) = resolve_deploy_model_provider(&record, &personas, &global); assert_eq!( - model, + model.as_deref(), Some("new-model"), - "deploy must use live persona model, not stale record snapshot" + "deploy must use definition model, not stale record snapshot" ); assert_eq!( - provider, + provider.as_deref(), Some("new-prov"), - "deploy must use live persona provider, not stale record snapshot" + "deploy must use definition provider, not stale record snapshot" ); } -/// Deploy resolver falls back to record when persona has no model/provider -/// (persona without structured model — fallback to record snapshot). +/// When a linked definition has blank model/provider (inherit), the deploy +/// resolver must fall through to global — stale record bytes are inert. #[test] -fn deploy_resolver_falls_back_to_record_when_persona_has_none() { - let record = bare_agent_record(Some("p1"), Some("record-model"), Some("record-prov")); - // Persona exists but has no model/provider. +fn deploy_resolver_inherits_global_when_definition_blank() { + let record = bare_agent_record(Some("p1"), Some("stale-model"), Some("stale-prov")); let personas = vec![persona_record("p1", None, None)]; - let global = crate::managed_agents::GlobalAgentConfig::default(); + let global = crate::managed_agents::GlobalAgentConfig { + model: Some("global-model".to_string()), + provider: Some("global-prov".to_string()), + ..Default::default() + }; let (model, provider) = resolve_deploy_model_provider(&record, &personas, &global); - assert_eq!(model, Some("record-model")); - assert_eq!(provider, Some("record-prov")); + assert_eq!( + model.as_deref(), + Some("global-model"), + "definition blank → global; stale record ignored" + ); + assert_eq!( + provider.as_deref(), + Some("global-prov"), + "definition blank → global; stale record ignored" + ); } -/// Deploy resolver falls back to global when both persona and record have none. +/// Deploy resolver falls back to global when both definition and record have none. #[test] -fn deploy_resolver_falls_back_to_global_when_persona_and_record_have_none() { +fn deploy_resolver_falls_back_to_global_when_definition_and_record_have_none() { let record = bare_agent_record(Some("p1"), None, None); let personas = vec![persona_record("p1", None, None)]; let global = crate::managed_agents::GlobalAgentConfig { @@ -187,8 +186,35 @@ fn deploy_resolver_falls_back_to_global_when_persona_and_record_have_none() { let (model, provider) = resolve_deploy_model_provider(&record, &personas, &global); - assert_eq!(model, Some("global-model")); - assert_eq!(provider, Some("global-prov")); + assert_eq!(model.as_deref(), Some("global-model")); + assert_eq!(provider.as_deref(), Some("global-prov")); +} + +/// Orphan: linked record with missing definition → the pure model/provider +/// pair resolver returns `(None, None)`. This is NOT the deploy refusal +/// boundary — `build_deploy_payload` refuses an orphan outright via +/// `.require_resolved()?` before this pair is ever computed. This test pins +/// the resolver's own orphan behavior, which readiness/hash also depend on. +#[test] +fn deploy_resolver_returns_none_for_orphaned_instance() { + let record = bare_agent_record(Some("missing-def"), Some("stale-model"), Some("stale-prov")); + let personas: Vec = vec![]; + let global = crate::managed_agents::GlobalAgentConfig { + model: Some("global-model".to_string()), + provider: Some("global-prov".to_string()), + ..Default::default() + }; + + let (model, provider) = resolve_deploy_model_provider(&record, &personas, &global); + + assert!( + model.is_none(), + "orphaned instance must not resolve to any model" + ); + assert!( + provider.is_none(), + "orphaned instance must not resolve to any provider" + ); } #[test] @@ -404,6 +430,7 @@ fn deploy_payload_carries_the_full_behavioral_quad() { "wss://relay.example".to_string(), Some("gpt-x".to_string()), Some("openai".to_string()), + None, std::collections::BTreeMap::new(), ); diff --git a/desktop/src-tauri/src/commands/mesh_llm.rs b/desktop/src-tauri/src/commands/mesh_llm.rs index ad55d95060..fd11273851 100644 --- a/desktop/src-tauri/src/commands/mesh_llm.rs +++ b/desktop/src-tauri/src/commands/mesh_llm.rs @@ -374,20 +374,20 @@ fn pick_serve_target_for_model( /// Non relay-mesh records are a no-op. pub(crate) async fn ensure_relay_mesh_for_record( app: &AppHandle, - record: &crate::managed_agents::ManagedAgentRecord, + model_id: Option<&str>, _allow_fresh_create_start: bool, ) -> Result<(), String> { let state = app.state::(); - let Some(model_id) = crate::managed_agents::relay_mesh_model_id(record) else { + let Some(model_id) = model_id else { return Ok(()); }; // A local serve/client runtime already owns the OpenAI ingress and its // router can resolve both `auto` and explicit remote models. Do not require // a separate relay-advertised target in that case. if state.mesh_llm_runtime.lock().await.is_some() { - return wait_for_mesh_inference(&model_id).await; + return wait_for_mesh_inference(model_id).await; } - let target = match resolve_mesh_bootstrap_target(&state, &model_id).await { + let target = match resolve_mesh_bootstrap_target(&state, model_id).await { Ok(Some(target)) => target, Ok(None) => { return Err( @@ -402,8 +402,8 @@ pub(crate) async fn ensure_relay_mesh_for_record( } }; - ensure_client_node_for_model(&state, &model_id, Some(target.endpoint_addr)).await?; - wait_for_mesh_inference(&model_id).await + ensure_client_node_for_model(&state, model_id, Some(target.endpoint_addr)).await?; + wait_for_mesh_inference(model_id).await } #[tauri::command] diff --git a/desktop/src-tauri/src/managed_agents/effective_config/mod.rs b/desktop/src-tauri/src/managed_agents/effective_config/mod.rs new file mode 100644 index 0000000000..254e3b7cb2 --- /dev/null +++ b/desktop/src-tauri/src/managed_agents/effective_config/mod.rs @@ -0,0 +1,214 @@ +use serde::Serialize; + +use super::global_config::GlobalAgentConfig; +use super::relay_mesh::{RELAY_MESH_AUTO_MODEL_ID, RELAY_MESH_PROVIDER_ID}; +use super::types::{AgentDefinition, ManagedAgentRecord}; + +#[derive(Debug, Clone, PartialEq, Eq, Serialize)] +#[serde(rename_all = "snake_case")] +pub enum ConfigSource { + Definition, + Global, + InstanceLegacy, +} + +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct ResolvedField { + pub value: Option, + pub source: ConfigSource, +} + +#[derive(Debug, Clone)] +pub struct EffectiveAgentConfig { + pub model: ResolvedField, + pub provider: ResolvedField, + pub system_prompt: ResolvedField, +} + +impl EffectiveAgentConfig { + /// The relay-mesh model id this config resolves to, or `None` when the + /// effective provider isn't relay-mesh. + /// + /// This is the single authoritative mesh decision for this config. Both + /// the mesh preflight (interactive start, restore-on-launch) AND spawn's + /// `apply_relay_mesh_env` block MUST derive their mesh gate from this + /// method — never from a separate provider comparison — so the two paths + /// are guaranteed to agree even when the stored provider string has leading + /// or trailing whitespace. The provider is trimmed before matching; + /// a blank effective model falls back to "auto", mirroring + /// `apply_relay_mesh_env`'s own rule. + pub fn relay_mesh_model_id(&self) -> Option { + if self.provider.value.as_deref().map(str::trim) != Some(RELAY_MESH_PROVIDER_ID) { + return None; + } + Some( + self.model + .value + .as_deref() + .map(str::trim) + .filter(|value| !value.is_empty()) + .unwrap_or(RELAY_MESH_AUTO_MODEL_ID) + .to_string(), + ) + } +} + +#[derive(Debug, Clone)] +pub enum EffectiveConfigResult { + Resolved(EffectiveAgentConfig), + OrphanedInstance { + record_pubkey: String, + missing_persona_id: String, + }, +} + +fn non_blank(v: Option<&str>) -> Option<&str> { + v.filter(|s| !s.trim().is_empty()) +} + +fn resolve_linked( + definition: &AgentDefinition, + global: &GlobalAgentConfig, +) -> EffectiveAgentConfig { + let model = match non_blank(definition.model.as_deref()) { + Some(m) => ResolvedField { + value: Some(m.to_owned()), + source: ConfigSource::Definition, + }, + None => ResolvedField { + value: global.model.clone(), + source: ConfigSource::Global, + }, + }; + + let provider = match non_blank(definition.provider.as_deref()) { + Some(p) => ResolvedField { + value: Some(p.to_owned()), + source: ConfigSource::Definition, + }, + None => ResolvedField { + value: global.provider.clone(), + source: ConfigSource::Global, + }, + }; + + let system_prompt = ResolvedField { + value: non_blank(Some(definition.system_prompt.as_str())).map(str::to_owned), + source: ConfigSource::Definition, + }; + + EffectiveAgentConfig { + model, + provider, + system_prompt, + } +} + +fn resolve_definition_less( + record: &ManagedAgentRecord, + global: &GlobalAgentConfig, +) -> EffectiveAgentConfig { + let model = match non_blank(record.model.as_deref()) { + Some(m) => ResolvedField { + value: Some(m.to_owned()), + source: ConfigSource::InstanceLegacy, + }, + None => ResolvedField { + value: global.model.clone(), + source: ConfigSource::Global, + }, + }; + + let provider = match non_blank(record.provider.as_deref()) { + Some(p) => ResolvedField { + value: Some(p.to_owned()), + source: ConfigSource::InstanceLegacy, + }, + None => ResolvedField { + value: global.provider.clone(), + source: ConfigSource::Global, + }, + }; + + let system_prompt = ResolvedField { + value: non_blank(record.system_prompt.as_deref()).map(str::to_owned), + source: ConfigSource::InstanceLegacy, + }; + + EffectiveAgentConfig { + model, + provider, + system_prompt, + } +} + +pub fn resolve_effective_config( + record: &ManagedAgentRecord, + definitions: &[AgentDefinition], + global: &GlobalAgentConfig, +) -> EffectiveConfigResult { + match &record.persona_id { + Some(pid) => match definitions.iter().find(|d| d.id == *pid) { + Some(def) => EffectiveConfigResult::Resolved(resolve_linked(def, global)), + None => EffectiveConfigResult::OrphanedInstance { + record_pubkey: record.pubkey.clone(), + missing_persona_id: pid.clone(), + }, + }, + None => EffectiveConfigResult::Resolved(resolve_definition_less(record, global)), + } +} + +pub fn resolve_effective_model_provider_pair( + record: &ManagedAgentRecord, + definitions: &[AgentDefinition], + global: &GlobalAgentConfig, +) -> Option<(Option, Option)> { + match resolve_effective_config(record, definitions, global) { + EffectiveConfigResult::Resolved(cfg) => Some((cfg.model.value, cfg.provider.value)), + EffectiveConfigResult::OrphanedInstance { .. } => None, + } +} + +/// The relay-mesh preflight decision for `record`, resolved the same way +/// spawn resolves its mesh env: through `resolve_effective_config` (which +/// folds in the definition → global fallback), never through the record's +/// own `provider`/`model`/`relay_mesh` bytes. +/// +/// `None` covers both "not a mesh agent" and "orphaned instance" — an orphan +/// never spawns (see `require_resolved`), so it never needs a mesh preflight +/// either; the caller's own orphan handling downstream is unaffected, this +/// just avoids tripping mesh bootstrap for a start that will be refused. +pub fn resolve_effective_relay_mesh_model_id( + record: &ManagedAgentRecord, + definitions: &[AgentDefinition], + global: &GlobalAgentConfig, +) -> Option { + match resolve_effective_config(record, definitions, global) { + EffectiveConfigResult::Resolved(cfg) => cfg.relay_mesh_model_id(), + EffectiveConfigResult::OrphanedInstance { .. } => None, + } +} + +/// The single user-facing message for a linked instance whose definition no +/// longer exists. Shared by every path that must refuse to act on an orphan: +/// the spawn boundary (`spawn_agent_child`), the interactive start command, +/// and provider deploy. +pub const ORPHANED_INSTANCE_ERROR: &str = + "This agent's configuration is missing — it may still be \ + syncing or was deleted on another device."; + +impl EffectiveConfigResult { + /// Unwrap into the resolved config, or the shared orphan-refusal error. + pub fn require_resolved(self) -> Result { + match self { + EffectiveConfigResult::Resolved(cfg) => Ok(cfg), + EffectiveConfigResult::OrphanedInstance { .. } => { + Err(ORPHANED_INSTANCE_ERROR.to_string()) + } + } + } +} + +#[cfg(test)] +mod tests; diff --git a/desktop/src-tauri/src/managed_agents/effective_config/tests.rs b/desktop/src-tauri/src/managed_agents/effective_config/tests.rs new file mode 100644 index 0000000000..f7bc0f2a83 --- /dev/null +++ b/desktop/src-tauri/src/managed_agents/effective_config/tests.rs @@ -0,0 +1,567 @@ +use super::*; +use std::collections::BTreeMap; + +fn definition( + id: &str, + model: Option<&str>, + provider: Option<&str>, + prompt: &str, +) -> AgentDefinition { + AgentDefinition { + id: id.to_string(), + display_name: "Test Definition".to_string(), + avatar_url: None, + system_prompt: prompt.to_string(), + runtime: None, + model: model.map(str::to_string), + provider: provider.map(str::to_string), + name_pool: vec![], + is_builtin: false, + is_active: true, + source_team: None, + source_team_persona_slug: None, + env_vars: BTreeMap::new(), + respond_to: None, + respond_to_allowlist: vec![], + parallelism: None, + created_at: "".to_string(), + updated_at: "".to_string(), + } +} + +fn record( + persona_id: Option<&str>, + model: Option<&str>, + provider: Option<&str>, + prompt: Option<&str>, +) -> ManagedAgentRecord { + use crate::managed_agents::{BackendKind, RespondTo}; + ManagedAgentRecord { + pubkey: "agent-pk".to_string(), + name: "Agent".to_string(), + persona_id: persona_id.map(str::to_string), + private_key_nsec: "".to_string(), + auth_tag: None, + relay_url: "ws://localhost:3000".to_string(), + avatar_url: None, + acp_command: "buzz-acp".to_string(), + agent_command: "goose".to_string(), + agent_command_override: None, + agent_args: vec![], + mcp_command: "".to_string(), + turn_timeout_seconds: 300, + idle_timeout_seconds: None, + max_turn_duration_seconds: None, + parallelism: 1, + system_prompt: prompt.map(str::to_string), + model: model.map(str::to_string), + provider: provider.map(str::to_string), + persona_source_version: None, + env_vars: BTreeMap::new(), + start_on_app_launch: false, + runtime_pid: None, + backend: BackendKind::Local, + backend_agent_id: None, + provider_binary_path: None, + team_id: None, + persona_team_dir: None, + persona_name_in_team: None, + created_at: "".to_string(), + updated_at: "".to_string(), + last_started_at: None, + last_stopped_at: None, + last_exit_code: None, + last_error: None, + last_error_code: None, + respond_to: RespondTo::OwnerOnly, + respond_to_allowlist: vec![], + display_name: None, + slug: None, + runtime: None, + name_pool: vec![], + is_builtin: false, + is_active: true, + source_team: None, + source_team_persona_slug: None, + relay_mesh: None, + auto_restart_on_config_change: false, + definition_respond_to: None, + definition_respond_to_allowlist: vec![], + definition_parallelism: None, + } +} + +fn global(model: Option<&str>, provider: Option<&str>) -> GlobalAgentConfig { + GlobalAgentConfig { + model: model.map(str::to_string), + provider: provider.map(str::to_string), + ..Default::default() + } +} + +// ── Linked instance: definition → global, record ignored ── + +#[test] +fn linked_definition_model_wins_over_stale_record() { + let rec = record( + Some("d1"), + Some("stale-model"), + Some("stale-prov"), + Some("stale prompt"), + ); + let defs = vec![definition( + "d1", + Some("def-model"), + Some("def-prov"), + "def prompt", + )]; + let g = global(Some("global-model"), Some("global-prov")); + + let result = resolve_effective_config(&rec, &defs, &g); + let cfg = match result { + EffectiveConfigResult::Resolved(c) => c, + other => panic!("expected Resolved, got {:?}", other), + }; + + assert_eq!(cfg.model.value.as_deref(), Some("def-model")); + assert_eq!(cfg.model.source, ConfigSource::Definition); + assert_eq!(cfg.provider.value.as_deref(), Some("def-prov")); + assert_eq!(cfg.provider.source, ConfigSource::Definition); + assert_eq!(cfg.system_prompt.value.as_deref(), Some("def prompt")); + assert_eq!(cfg.system_prompt.source, ConfigSource::Definition); +} + +#[test] +fn linked_inherit_global_when_definition_blank() { + let rec = record( + Some("d1"), + Some("stale-model"), + Some("stale-prov"), + Some("stale prompt"), + ); + let defs = vec![definition("d1", None, None, "")]; + let g = global(Some("global-model"), Some("global-prov")); + + let result = resolve_effective_config(&rec, &defs, &g); + let cfg = match result { + EffectiveConfigResult::Resolved(c) => c, + other => panic!("expected Resolved, got {:?}", other), + }; + + assert_eq!(cfg.model.value.as_deref(), Some("global-model")); + assert_eq!(cfg.model.source, ConfigSource::Global); + assert_eq!(cfg.provider.value.as_deref(), Some("global-prov")); + assert_eq!(cfg.provider.source, ConfigSource::Global); + assert_eq!(cfg.system_prompt.value, None); + assert_eq!(cfg.system_prompt.source, ConfigSource::Definition); +} + +#[test] +fn linked_stale_record_model_is_inert() { + let rec = record(Some("d1"), Some("stale-model"), Some("stale-prov"), None); + let defs = vec![definition("d1", None, None, "")]; + let g = global(None, None); + + let result = resolve_effective_config(&rec, &defs, &g); + let cfg = match result { + EffectiveConfigResult::Resolved(c) => c, + other => panic!("expected Resolved, got {:?}", other), + }; + + assert_eq!(cfg.model.value, None); + assert_eq!(cfg.model.source, ConfigSource::Global); + assert_eq!(cfg.provider.value, None); + assert_eq!(cfg.provider.source, ConfigSource::Global); +} + +#[test] +fn linked_definition_model_set_provider_inherits() { + let rec = record(Some("d1"), None, None, None); + let defs = vec![definition("d1", Some("def-model"), None, "prompt")]; + let g = global(None, Some("global-prov")); + + let result = resolve_effective_config(&rec, &defs, &g); + let cfg = match result { + EffectiveConfigResult::Resolved(c) => c, + other => panic!("expected Resolved, got {:?}", other), + }; + + assert_eq!(cfg.model.value.as_deref(), Some("def-model")); + assert_eq!(cfg.model.source, ConfigSource::Definition); + assert_eq!(cfg.provider.value.as_deref(), Some("global-prov")); + assert_eq!(cfg.provider.source, ConfigSource::Global); +} + +#[test] +fn linked_blank_prompt_means_no_prompt() { + let rec = record(Some("d1"), None, None, Some("stale prompt on record")); + let defs = vec![definition("d1", None, None, "")]; + let g = global(None, None); + + let result = resolve_effective_config(&rec, &defs, &g); + let cfg = match result { + EffectiveConfigResult::Resolved(c) => c, + other => panic!("expected Resolved, got {:?}", other), + }; + + assert_eq!(cfg.system_prompt.value, None); + assert_eq!(cfg.system_prompt.source, ConfigSource::Definition); +} + +#[test] +fn linked_whitespace_only_definition_model_inherits_global() { + let rec = record(Some("d1"), Some("stale"), None, None); + let defs = vec![definition("d1", Some(" "), Some(" \t"), "")]; + let g = global(Some("global-model"), Some("global-prov")); + + let result = resolve_effective_config(&rec, &defs, &g); + let cfg = match result { + EffectiveConfigResult::Resolved(c) => c, + other => panic!("expected Resolved, got {:?}", other), + }; + + assert_eq!(cfg.model.value.as_deref(), Some("global-model")); + assert_eq!(cfg.model.source, ConfigSource::Global); + assert_eq!(cfg.provider.value.as_deref(), Some("global-prov")); + assert_eq!(cfg.provider.source, ConfigSource::Global); +} + +// ── Orphaned instance ── + +#[test] +fn orphaned_linked_instance_returns_error() { + let rec = record(Some("missing-def"), None, None, None); + let defs = vec![]; + let g = global(Some("global-model"), None); + + let result = resolve_effective_config(&rec, &defs, &g); + match result { + EffectiveConfigResult::OrphanedInstance { + record_pubkey, + missing_persona_id, + } => { + assert_eq!(record_pubkey, "agent-pk"); + assert_eq!(missing_persona_id, "missing-def"); + } + other => panic!("expected OrphanedInstance, got {:?}", other), + } +} + +// ── Definition-less instance: instance → global ── + +#[test] +fn definition_less_uses_own_fields() { + let rec = record(None, Some("my-model"), Some("my-prov"), Some("my prompt")); + let defs = vec![]; + let g = global(Some("global-model"), Some("global-prov")); + + let result = resolve_effective_config(&rec, &defs, &g); + let cfg = match result { + EffectiveConfigResult::Resolved(c) => c, + other => panic!("expected Resolved, got {:?}", other), + }; + + assert_eq!(cfg.model.value.as_deref(), Some("my-model")); + assert_eq!(cfg.model.source, ConfigSource::InstanceLegacy); + assert_eq!(cfg.provider.value.as_deref(), Some("my-prov")); + assert_eq!(cfg.provider.source, ConfigSource::InstanceLegacy); + assert_eq!(cfg.system_prompt.value.as_deref(), Some("my prompt")); + assert_eq!(cfg.system_prompt.source, ConfigSource::InstanceLegacy); +} + +#[test] +fn definition_less_falls_back_to_global() { + let rec = record(None, None, None, None); + let defs = vec![]; + let g = global(Some("global-model"), Some("global-prov")); + + let result = resolve_effective_config(&rec, &defs, &g); + let cfg = match result { + EffectiveConfigResult::Resolved(c) => c, + other => panic!("expected Resolved, got {:?}", other), + }; + + assert_eq!(cfg.model.value.as_deref(), Some("global-model")); + assert_eq!(cfg.model.source, ConfigSource::Global); + assert_eq!(cfg.provider.value.as_deref(), Some("global-prov")); + assert_eq!(cfg.provider.source, ConfigSource::Global); +} + +#[test] +fn definition_less_blank_record_fields_fall_through() { + let rec = record(None, Some(" "), Some(""), Some(" ")); + let defs = vec![]; + let g = global(Some("g-model"), None); + + let result = resolve_effective_config(&rec, &defs, &g); + let cfg = match result { + EffectiveConfigResult::Resolved(c) => c, + other => panic!("expected Resolved, got {:?}", other), + }; + + assert_eq!(cfg.model.value.as_deref(), Some("g-model")); + assert_eq!(cfg.model.source, ConfigSource::Global); + assert_eq!(cfg.provider.value, None); + assert_eq!(cfg.provider.source, ConfigSource::Global); + assert_eq!(cfg.system_prompt.value, None); +} + +// ── Convenience helper ── + +#[test] +fn model_provider_pair_returns_none_for_orphan() { + let rec = record(Some("missing"), None, None, None); + assert_eq!( + resolve_effective_model_provider_pair(&rec, &[], &global(None, None)), + None + ); +} + +#[test] +fn model_provider_pair_returns_resolved_values() { + let rec = record(Some("d1"), None, None, None); + let defs = vec![definition("d1", Some("m"), Some("p"), "")]; + let g = global(None, None); + + let pair = resolve_effective_model_provider_pair(&rec, &defs, &g); + assert_eq!(pair, Some((Some("m".to_string()), Some("p".to_string())))); +} + +// ── require_resolved: the shared orphan-refusal contract used by +// build_deploy_payload and spawn_agent_child ── + +#[test] +fn require_resolved_returns_shared_error_for_orphan() { + let rec = record(Some("missing"), None, None, None); + let error = resolve_effective_config(&rec, &[], &global(None, None)) + .require_resolved() + .expect_err("orphan must not resolve"); + assert_eq!(error, ORPHANED_INSTANCE_ERROR); +} + +#[test] +fn require_resolved_returns_config_for_resolved() { + let rec = record(Some("d1"), None, None, None); + let defs = vec![definition("d1", Some("m"), Some("p"), "prompt")]; + let cfg = resolve_effective_config(&rec, &defs, &global(None, None)) + .require_resolved() + .expect("linked instance with a live definition must resolve"); + assert_eq!(cfg.model.value.as_deref(), Some("m")); +} + +#[test] +fn require_resolved_refuses_orphan_only() { + let orphan = record(Some("missing"), None, None, None); + assert_eq!( + resolve_effective_config(&orphan, &[], &global(None, None)) + .require_resolved() + .unwrap_err(), + ORPHANED_INSTANCE_ERROR, + ); + + let linked = record(Some("d1"), None, None, None); + let defs = vec![definition("d1", Some("m"), None, "")]; + assert!( + resolve_effective_config(&linked, &defs, &global(None, None)) + .require_resolved() + .is_ok() + ); + + // Definition-less instances are never orphaned regardless of how bare + // their own fields are — orphan status only applies to a dangling link. + let bare = record(None, None, None, None); + assert!(resolve_effective_config(&bare, &[], &global(None, None)) + .require_resolved() + .is_ok()); +} + +// ── Morgan's exact regression sequence ── + +#[test] +fn morgans_sequence_inherit_explicit_inherit() { + let g = global(Some("claude-opus-4-6"), Some("anthropic")); + + // Step 1: fresh agent with inherited model → resolves global + let rec_step1 = record(Some("d1"), None, None, None); + let defs = vec![definition("d1", None, None, "agent prompt")]; + let cfg1 = match resolve_effective_config(&rec_step1, &defs, &g) { + EffectiveConfigResult::Resolved(c) => c, + other => panic!("step 1: {:?}", other), + }; + assert_eq!(cfg1.model.value.as_deref(), Some("claude-opus-4-6")); + assert_eq!(cfg1.model.source, ConfigSource::Global); + + // Step 2: set explicit model on definition + let defs_explicit = vec![definition( + "d1", + Some("goose-gpt-5-6-sol"), + Some("databricks"), + "agent prompt", + )]; + let cfg2 = match resolve_effective_config(&rec_step1, &defs_explicit, &g) { + EffectiveConfigResult::Resolved(c) => c, + other => panic!("step 2: {:?}", other), + }; + assert_eq!(cfg2.model.value.as_deref(), Some("goose-gpt-5-6-sol")); + assert_eq!(cfg2.model.source, ConfigSource::Definition); + + // Step 3: switch back to inherit — even with stale record bytes + let rec_stale = record( + Some("d1"), + Some("goose-gpt-5-6-sol"), + Some("databricks"), + None, + ); + let defs_inherit = vec![definition("d1", None, None, "agent prompt")]; + let cfg3 = match resolve_effective_config(&rec_stale, &defs_inherit, &g) { + EffectiveConfigResult::Resolved(c) => c, + other => panic!("step 3: {:?}", other), + }; + assert_eq!(cfg3.model.value.as_deref(), Some("claude-opus-4-6")); + assert_eq!(cfg3.model.source, ConfigSource::Global); + assert_eq!(cfg3.provider.value.as_deref(), Some("anthropic")); + assert_eq!(cfg3.provider.source, ConfigSource::Global); +} + +// ── relay-mesh preflight resolution (Wes review 2 on #1968) ── +// +// Both regressions below reproduce the exact defects: the preflight must +// key off `resolve_effective_config`'s resolution — the same one spawn's +// mesh env consults — not the record's own `provider`/`model` bytes. + +#[test] +fn relay_mesh_model_id_none_for_non_mesh_config() { + let rec = record(Some("d1"), None, None, None); + let defs = vec![definition("d1", Some("m"), Some("anthropic"), "")]; + let g = global(None, None); + + assert_eq!(resolve_effective_relay_mesh_model_id(&rec, &defs, &g), None); +} + +#[test] +fn relay_mesh_model_id_defaults_to_auto_when_model_blank() { + let rec = record(Some("d1"), None, None, None); + let defs = vec![definition("d1", None, Some(RELAY_MESH_PROVIDER_ID), "")]; + let g = global(None, None); + + assert_eq!( + resolve_effective_relay_mesh_model_id(&rec, &defs, &g).as_deref(), + Some(RELAY_MESH_AUTO_MODEL_ID) + ); +} + +/// Switch-away regression (Wes finding 1): a linked definition that used to +/// be relay-mesh but was edited to another provider must NOT trigger the mesh +/// preflight — even though the record's own stale bytes still say +/// `provider: relay-mesh`. The old `relay_mesh_config(record)` sniff read +/// those stale record bytes directly and returned Some; the resolver-driven +/// decision must read the definition's CURRENT provider and return None. +#[test] +fn switch_away_from_relay_mesh_clears_preflight_despite_stale_record_bytes() { + let rec = record(Some("d1"), Some("auto"), Some(RELAY_MESH_PROVIDER_ID), None); + let defs = vec![definition( + "d1", + Some("claude-opus-4-6"), + Some("anthropic"), + "", + )]; + let g = global(None, None); + + assert_eq!( + resolve_effective_relay_mesh_model_id(&rec, &defs, &g), + None, + "definition switched away from relay-mesh — no mesh preflight should fire" + ); +} + +/// Global-inheritance regression (Wes finding 2): a blank linked definition +/// falling through to a relay-mesh GLOBAL default must trigger the mesh +/// preflight, even though the record carries `provider: None` and no legacy +/// env — all three of the old `relay_mesh_config` branches would miss here. +#[test] +fn global_relay_mesh_default_triggers_preflight_for_blank_definition() { + let rec = record(Some("d1"), None, None, None); + let defs = vec![definition("d1", None, None, "")]; + let g = global(Some("Qwen3"), Some(RELAY_MESH_PROVIDER_ID)); + + assert_eq!( + resolve_effective_relay_mesh_model_id(&rec, &defs, &g).as_deref(), + Some("Qwen3"), + "global relay-mesh default must trigger the mesh preflight for a blank definition" + ); +} + +/// Symmetric case: a definition-less (legacy) instance inheriting the global +/// relay-mesh default must also trigger the preflight. +#[test] +fn global_relay_mesh_default_triggers_preflight_for_definition_less_instance() { + let rec = record(None, None, None, None); + let defs = vec![]; + let g = global(None, Some(RELAY_MESH_PROVIDER_ID)); + + assert_eq!( + resolve_effective_relay_mesh_model_id(&rec, &defs, &g).as_deref(), + Some(RELAY_MESH_AUTO_MODEL_ID) + ); +} + +/// Orphaned instance: no mesh preflight regardless of stale record bytes — +/// the caller's own orphan refusal is what matters, not a mesh bootstrap for +/// a start that will never happen. +#[test] +fn orphaned_instance_never_triggers_mesh_preflight() { + let rec = record( + Some("missing-def"), + Some("auto"), + Some(RELAY_MESH_PROVIDER_ID), + None, + ); + let g = global(None, None); + + assert_eq!(resolve_effective_relay_mesh_model_id(&rec, &[], &g), None); +} + +// ── Whitespace-provider parity (trim-mismatch regression, T1) ── +// +// The old spawn gate was `effective_provider.as_deref() == Some(RELAY_MESH_PROVIDER_ID)` +// — an exact compare — while `relay_mesh_model_id()` trims before matching. +// A stored provider of " relay-mesh " would make preflight fire while spawn +// skipped the mesh-env block. After the fix spawn derives its gate from +// `relay_mesh_model_id()` too, so the trim semantics are identical. +// +// These tests verify `relay_mesh_model_id()` returns Some for padded providers +// on every resolution path, confirming the helper that both callers now use +// handles whitespace correctly. + +/// Definition provider with leading/trailing whitespace — resolver preserves +/// the raw string, but `relay_mesh_model_id()` trims before matching, so the +/// mesh preflight fires correctly. +#[test] +fn whitespace_provider_in_definition_triggers_mesh_decision() { + let rec = record(Some("d1"), None, None, None); + let defs = vec![definition("d1", Some("qwen3"), Some(" relay-mesh "), "")]; + let g = global(None, None); + + let mesh_id = resolve_effective_relay_mesh_model_id(&rec, &defs, &g); + assert_eq!( + mesh_id.as_deref(), + Some("qwen3"), + "padded definition provider must be treated as relay-mesh by the shared helper" + ); +} + +/// Global provider with leading/trailing whitespace — same assertion for the +/// global-inherit path (blank definition falls through to padded global). +#[test] +fn whitespace_provider_in_global_triggers_mesh_decision() { + let rec = record(Some("d1"), None, None, None); + let defs = vec![definition("d1", None, None, "")]; + let g = global(Some("qwen3"), Some("\trelay-mesh\t")); + + let mesh_id = resolve_effective_relay_mesh_model_id(&rec, &defs, &g); + assert_eq!( + mesh_id.as_deref(), + Some("qwen3"), + "padded global provider must be treated as relay-mesh by the shared helper" + ); +} diff --git a/desktop/src-tauri/src/managed_agents/env_vars.rs b/desktop/src-tauri/src/managed_agents/env_vars.rs index 6318a5c838..c652dae427 100644 --- a/desktop/src-tauri/src/managed_agents/env_vars.rs +++ b/desktop/src-tauri/src/managed_agents/env_vars.rs @@ -285,8 +285,12 @@ pub(crate) fn merged_user_env( /// Look up the live env map of `persona_id` within an already-loaded persona /// slice. Returns an empty map for standalone agents (`None`) and for links -/// to personas that no longer exist (an orphaned agent spawns from its own -/// overrides alone — same fallback the prompt/model resolution uses). +/// to personas that no longer exist. The latter is an orphaned instance, +/// which `spawn_agent_child` refuses before this is ever read at spawn time +/// (see `effective_config::resolve_effective_config`'s `OrphanedInstance` +/// arm via `require_resolved`) — an empty map here only matters for +/// non-spawn callers like readiness/hash that must still resolve +/// a slice for a record whose orphan status hasn't been checked yet. pub(crate) fn live_persona_env( personas: &[super::types::AgentDefinition], persona_id: Option<&str>, diff --git a/desktop/src-tauri/src/managed_agents/global_config/mod.rs b/desktop/src-tauri/src/managed_agents/global_config/mod.rs index 57993fc040..162f447981 100644 --- a/desktop/src-tauri/src/managed_agents/global_config/mod.rs +++ b/desktop/src-tauri/src/managed_agents/global_config/mod.rs @@ -1,13 +1,14 @@ //! Global agent configuration defaults. //! //! A single `global-agent-config.json` record that applies to ALL managed -//! agents. Per-agent config always wins; global provides the lowest -//! user-settable layer below persona. +//! agents. For a linked instance, the definition wins over global; for a +//! definition-less (legacy) instance, the record's own fields win over +//! global. Global is always the lowest user-settable layer. //! //! # Precedence (low → high) //! //! ```text -//! baked build env < GLOBAL < persona < per-agent < Buzz-identity +//! baked build env < GLOBAL < definition (linked) / instance (legacy) < Buzz-identity //! ``` //! //! # Semantics @@ -38,8 +39,10 @@ use crate::managed_agents::types::{AgentDefinition, ManagedAgentRecord}; /// so the config vocabulary is consistent across all three tiers. /// /// `env_vars` is the lowest user-settable env layer — global < persona < agent. -/// `provider` / `model` are fallback defaults: effective provider/model = -/// `agent → persona → global → None`. +/// `provider` / `model` are fallback defaults, resolved via +/// `effective_config::resolve_effective_config`: for a linked instance, +/// definition → global (the record's own `provider`/`model` bytes are never +/// consulted); for a definition-less instance, instance → global. #[derive(Debug, Clone, Default, Serialize, Deserialize, PartialEq, Eq)] pub struct GlobalAgentConfig { /// Global env vars injected into ALL agents unconditionally. @@ -204,44 +207,28 @@ pub fn save_global_agent_config(app: &AppHandle, config: &GlobalAgentConfig) -> atomic_write_json_restricted(&path, &payload) } -/// Resolve the effective model and provider for an agent using the -/// precedence chain: `agent record → linked persona → global defaults → None`. +/// Resolve the effective model and provider for an agent. /// -/// This is the single source of truth used by readiness evaluation, spawn, -/// and deploy-payload construction. All three paths must use this function so -/// they agree on what model/provider the agent will actually run with. +/// Delegates to `effective_config::resolve_effective_config` which enforces +/// definition-authoritative semantics for linked instances: +/// - **Linked:** definition → global. Record bytes are never consulted. +/// - **Definition-less:** instance → global. +/// - **Orphaned:** returns `(None, None)`. This function is a display/ +/// readiness/hash convenience, not the spawn gate — an orphan must never +/// reach a real spawn regardless of what this returns, because +/// `spawn_agent_child` refuses orphans itself via +/// `effective_config::resolve_effective_config(...).require_resolved()` +/// before resolving model/provider/prompt for the process env. /// -/// # Arguments -/// * `record` — the `ManagedAgentRecord` (may have `None` for model/provider) -/// * `personas` — all current persona records (looked up by `record.persona_id`) -/// * `global` — global agent config defaults -/// -/// # Returns -/// `(effective_model, effective_provider)` — both `Option<&str>`. -pub(crate) fn resolve_effective_model_provider<'a>( - record: &'a ManagedAgentRecord, - personas: &'a [AgentDefinition], - global: &'a GlobalAgentConfig, -) -> (Option<&'a str>, Option<&'a str>) { - let (persona_model, persona_provider) = record - .persona_id - .as_deref() - .and_then(|pid| personas.iter().find(|p| p.id == pid)) - .map(|p| (p.model.as_deref(), p.provider.as_deref())) - .unwrap_or((None, None)); - - let effective_model = record - .model - .as_deref() - .or(persona_model) - .or(global.model.as_deref()); - let effective_provider = record - .provider - .as_deref() - .or(persona_provider) - .or(global.provider.as_deref()); - - (effective_model, effective_provider) +/// Returns owned `String`s because the effective value may come from a +/// tier that outlives none of the input borrows (e.g. global config). +pub(crate) fn resolve_effective_model_provider( + record: &ManagedAgentRecord, + personas: &[AgentDefinition], + global: &GlobalAgentConfig, +) -> (Option, Option) { + super::effective_config::resolve_effective_model_provider_pair(record, personas, global) + .unwrap_or((None, None)) } #[cfg(test)] diff --git a/desktop/src-tauri/src/managed_agents/global_config/tests.rs b/desktop/src-tauri/src/managed_agents/global_config/tests.rs index 6eb4338034..33b93d8a52 100644 --- a/desktop/src-tauri/src/managed_agents/global_config/tests.rs +++ b/desktop/src-tauri/src/managed_agents/global_config/tests.rs @@ -376,15 +376,14 @@ fn persona(id: &str, model: Option<&str>, provider: Option<&str>) -> AgentDefini } } -/// Tier 1 — agent record wins: record has explicit model/provider; they must -/// outrank both the linked persona and the global defaults. Fails against any -/// implementation that prefers global or persona over the record. +/// Linked instance: definition (persona) wins — stale record bytes are +/// ignored. This is the core model-inheritance fix. #[test] -fn resolve_agent_record_wins_over_persona_and_global() { +fn resolve_definition_wins_over_stale_record_for_linked_instance() { let mut record = bare_record(); record.persona_id = Some("p1".to_string()); - record.model = Some("record-model".to_string()); - record.provider = Some("record-provider".to_string()); + record.model = Some("stale-record-model".to_string()); + record.provider = Some("stale-record-provider".to_string()); let personas = vec![persona( "p1", Some("persona-model"), @@ -398,11 +397,15 @@ fn resolve_agent_record_wins_over_persona_and_global() { let (model, provider) = resolve_effective_model_provider(&record, &personas, &global); - assert_eq!(model, Some("record-model"), "record model must win"); assert_eq!( - provider, - Some("record-provider"), - "record provider must win" + model.as_deref(), + Some("persona-model"), + "definition model must win over stale record" + ); + assert_eq!( + provider.as_deref(), + Some("persona-provider"), + "definition provider must win over stale record" ); } @@ -428,12 +431,12 @@ fn resolve_persona_fallback_when_record_has_none() { let (model, provider) = resolve_effective_model_provider(&record, &personas, &global); assert_eq!( - model, + model.as_deref(), Some("persona-model"), "persona model must be used when record has none" ); assert_eq!( - provider, + provider.as_deref(), Some("persona-provider"), "persona provider must be used when record has none" ); @@ -458,12 +461,12 @@ fn resolve_global_fallback_when_record_and_persona_have_none() { let (model, provider) = resolve_effective_model_provider(&record, &personas, &global); assert_eq!( - model, + model.as_deref(), Some("global-model"), "global model must be used when record and persona have none" ); assert_eq!( - provider, + provider.as_deref(), Some("global-provider"), "global provider must be used when record and persona have none" ); @@ -519,8 +522,8 @@ fn resolve_global_fallback_when_no_persona_linked() { let (model, provider) = resolve_effective_model_provider(&record, &personas, &global); - assert_eq!(model, Some("global-model")); - assert_eq!(provider, Some("global-provider")); + assert_eq!(model.as_deref(), Some("global-model")); + assert_eq!(provider.as_deref(), Some("global-provider")); } /// All-None: no source provides model/provider → both must be None. @@ -543,18 +546,15 @@ fn resolve_all_none_when_no_source_provides_values() { ); } -/// Partial tier — record has model but not provider; persona has provider but -/// not model; global has both. Each field resolves independently through the -/// three-tier chain. +/// Each field resolves independently: definition model=None → global fills +/// model; definition has provider → definition wins for provider. Stale +/// record bytes are ignored for linked instances. #[test] fn resolve_each_field_resolves_independently_through_tiers() { let mut record = bare_record(); record.persona_id = Some("p1".to_string()); - record.model = Some("record-model".to_string()); - // record.provider = None → falls through to persona + record.model = Some("stale-record-model".to_string()); let personas = vec![persona("p1", None, Some("persona-provider"))]; - // persona.model = None → global fills model if record also had none, but - // record has model here so global is not needed for model. let global = GlobalAgentConfig { model: Some("global-model".to_string()), provider: Some("global-provider".to_string()), @@ -563,11 +563,15 @@ fn resolve_each_field_resolves_independently_through_tiers() { let (model, provider) = resolve_effective_model_provider(&record, &personas, &global); - assert_eq!(model, Some("record-model"), "record wins for model"); assert_eq!( - provider, + model.as_deref(), + Some("global-model"), + "definition model=None → global fills model; stale record ignored" + ); + assert_eq!( + provider.as_deref(), Some("persona-provider"), - "persona wins for provider when record has none" + "definition provider wins" ); } diff --git a/desktop/src-tauri/src/managed_agents/mod.rs b/desktop/src-tauri/src/managed_agents/mod.rs index 3a9c66b1ab..84be967409 100644 --- a/desktop/src-tauri/src/managed_agents/mod.rs +++ b/desktop/src-tauri/src/managed_agents/mod.rs @@ -8,6 +8,7 @@ pub(crate) use agent_env::{ mod backend; pub(crate) mod config_bridge; mod discovery; +pub(crate) mod effective_config; mod env_vars; pub(crate) mod git_bash; pub(crate) mod global_config; diff --git a/desktop/src-tauri/src/managed_agents/persona_events.rs b/desktop/src-tauri/src/managed_agents/persona_events.rs index 21b5794995..026ec06b92 100644 --- a/desktop/src-tauri/src/managed_agents/persona_events.rs +++ b/desktop/src-tauri/src/managed_agents/persona_events.rs @@ -359,10 +359,14 @@ pub fn persona_event_content(record: &AgentDefinition) -> PersonaEventContent { } /// A persona's spawn-relevant config, pinned onto a `ManagedAgentRecord` at -/// create time. After the snapshot, spawn and deploy read these fields off the -/// record and never the live persona, so an agent stays pinned to the config -/// it was created with — restart reuses the snapshot, delete+respawn rewrites -/// it. +/// create time for display/backward-compat purposes. For a linked instance, +/// spawn and deploy do NOT read these snapshotted fields — they resolve +/// model/provider/prompt live from the definition on every spawn (see +/// `effective_config::resolve_effective_config`), so a definition edit +/// propagates on the next restart without delete+respawn. The snapshot still +/// matters for `runtime` (materialized per B5, no live-read path yet) and for +/// `persona_source_version`, the drift basis the Agents menu compares against +/// the definition's current content hash. pub struct PersonaSnapshot { pub system_prompt: Option, pub model: Option, @@ -377,24 +381,6 @@ pub struct PersonaSnapshot { pub source_version: String, } -/// Apply persona-wins-when-set precedence for a single optional string field. -/// -/// Returns the persona's value when it is non-`None` and non-whitespace-only; -/// otherwise falls back to the record's value with the same blank filter applied. -/// Returns `None` only when both are blank — a genuinely unconfigured field stays -/// unconfigured. -/// -/// This is the single source of truth for the precedence rule used by -/// `persona_snapshot_with_agent_config_fallback`, `build_deploy_payload`, and -/// `resolve_effective_prompt_model_provider` so the three paths cannot drift. -pub fn persona_field_with_record_fallback( - persona_value: Option<&str>, - record_value: Option<&str>, -) -> Option { - let non_blank = |v: Option<&str>| v.filter(|s| !s.trim().is_empty()).map(str::to_owned); - non_blank(persona_value).or_else(|| non_blank(record_value)) -} - /// Build the pinned snapshot for an agent created from `persona`. /// /// The persona's `system_prompt` is always present, so it is wrapped in @@ -411,51 +397,14 @@ pub fn persona_snapshot(persona: &AgentDefinition) -> PersonaSnapshot { } } -/// Build the pinned snapshot for an **existing** agent record being re-snapshotted -/// from its linked persona (on spawn or app-launch restore). -/// -/// Precedence rule: when the persona sets `model` or `provider` (non-`None`, non-empty), -/// the persona wins — this is the expected inheritance. When the persona leaves -/// these fields blank (`None` or empty string), the agent record's own values are -/// preserved instead. This prevents a persona with no configured model/provider from -/// clobbering a value the user already set on the agent, which would trap the agent -/// in a permanent "needs configuration" loop that users cannot escape. +/// Re-pin `record` to `persona`: build a snapshot via [`persona_snapshot`] +/// and mirror it onto the record — the definition quad +/// (`system_prompt`/`model`/`provider`/`runtime`), the env-override +/// self-heal, and the `persona_source_version` drift basis. /// -/// `source_version` is always updated to the current persona content hash so the -/// drift badge clears correctly even when model/provider are not touched. -/// -/// Env vars are not part of the snapshot: `record.env_vars` (agent overrides) -/// is left untouched and the live persona env is merged underneath at read time. -/// -/// The two fields (`model`, `provider`) are independent: a persona that sets only -/// `model` wins on `model` while the agent's `provider` is preserved, and vice versa. -pub fn persona_snapshot_with_agent_config_fallback( - persona: &AgentDefinition, - current_agent_model: Option<&str>, - current_agent_provider: Option<&str>, -) -> PersonaSnapshot { - // Delegate system_prompt and source_version to persona_snapshot so future - // PersonaSnapshot field additions stay automatically consistent. - let base = persona_snapshot(persona); - - // Apply the shared precedence rule: persona wins when non-blank, else - // the agent record's value is preserved so a configured agent stays configured. - let model = persona_field_with_record_fallback(base.model.as_deref(), current_agent_model); - let provider = - persona_field_with_record_fallback(base.provider.as_deref(), current_agent_provider); - - PersonaSnapshot { - model, - provider, - ..base - } -} - -/// Re-pin `record` to `persona`: build the snapshot (via -/// [`persona_snapshot_with_agent_config_fallback`], so blank persona -/// `model`/`provider` preserve the record's own values) and mirror it onto the -/// record — the definition quad (`system_prompt`/`model`/`provider`/`runtime`), -/// the env-override self-heal, and the `persona_source_version` drift basis. +/// Definition-authoritative: blank definition model/provider produce `None` +/// on the record. The effective-config resolver falls through to the global +/// default at read time; stale materialized record bytes are never preserved. /// /// This is the single apply used by every snapshot-apply site: the spawn /// re-pin (`start_local_agent_with_preflight`), the launch backfill and @@ -467,11 +416,7 @@ pub fn persona_snapshot_with_agent_config_fallback( /// caller's concern, and `spawn_config_hash` (which applies this to a clone) /// must stay pure. pub fn apply_persona_snapshot(record: &mut ManagedAgentRecord, persona: &AgentDefinition) { - let snapshot = persona_snapshot_with_agent_config_fallback( - persona, - record.model.as_deref(), // fallback: record.model - record.provider.as_deref(), // fallback: record.provider - ); + let snapshot = persona_snapshot(persona); if let Some(prompt) = snapshot.system_prompt { record.system_prompt = Some(prompt); } @@ -488,5 +433,33 @@ pub fn apply_persona_snapshot(record: &mut ManagedAgentRecord, persona: &AgentDe .retain(|k, v| persona.env_vars.get(k) != Some(v)); record.persona_source_version = Some(snapshot.source_version); } + +/// Preview what `record` would look like immediately after the start/restore +/// paths re-pin it to its linked persona, without mutating `record` itself. +/// +/// Every decision made ahead of the real re-pin — the relay-mesh preflight in +/// `start_local_agent_with_preflight`, the restart-badge hash in +/// `spawn_config_hash` — needs to reason about spawn-time state, not +/// pre-snapshot bytes, so a persona edit that flips a field (e.g. `provider` +/// to/from relay-mesh) between saves is reflected in the decision instead of +/// the stale value the real [`apply_persona_snapshot`] is about to overwrite +/// anyway. Idempotent: applying it to an already-current record is a no-op, +/// so the spawn-time stamp and later recomputes agree when nothing changed. +/// +/// Orphaned records (persona deleted) pass through unchanged: the caller's +/// own orphan handling — refusing to spawn, hashing as `(None, None, None)` +/// — runs on the real record downstream, not on this preview. +pub fn preview_prospective_persona_snapshot( + record: &ManagedAgentRecord, + personas: &[AgentDefinition], +) -> ManagedAgentRecord { + let mut preview = record.clone(); + if let Some(persona_id) = preview.persona_id.clone() { + if let Some(persona) = personas.iter().find(|p| p.id == persona_id) { + apply_persona_snapshot(&mut preview, persona); + } + } + preview +} #[cfg(test)] mod tests; diff --git a/desktop/src-tauri/src/managed_agents/persona_events/tests.rs b/desktop/src-tauri/src/managed_agents/persona_events/tests.rs index d62cc5b4bb..27d3b0ce06 100644 --- a/desktop/src-tauri/src/managed_agents/persona_events/tests.rs +++ b/desktop/src-tauri/src/managed_agents/persona_events/tests.rs @@ -1,4 +1,141 @@ use super::*; +use crate::managed_agents::{BackendKind, ManagedAgentRecord, RespondTo}; + +/// A linked instance record with no persona-derived fields set yet — the +/// state right after creation, before any snapshot apply. +fn sample_record() -> ManagedAgentRecord { + ManagedAgentRecord { + pubkey: "p".repeat(64), + name: "agent".into(), + persona_id: Some("test-persona".into()), + private_key_nsec: "nsec1fake".into(), + auth_tag: None, + relay_url: "ws://localhost:3000".into(), + avatar_url: None, + acp_command: "buzz-acp".into(), + agent_command: "goose".into(), + agent_command_override: None, + agent_args: vec![], + mcp_command: String::new(), + turn_timeout_seconds: 320, + idle_timeout_seconds: None, + max_turn_duration_seconds: None, + parallelism: 1, + system_prompt: None, + model: None, + provider: None, + persona_source_version: None, + env_vars: BTreeMap::new(), + start_on_app_launch: false, + auto_restart_on_config_change: true, + runtime_pid: None, + backend: BackendKind::Local, + backend_agent_id: None, + provider_binary_path: None, + team_id: None, + persona_team_dir: None, + persona_name_in_team: None, + created_at: "now".into(), + updated_at: "now".into(), + last_started_at: None, + last_stopped_at: None, + last_exit_code: None, + last_error: None, + last_error_code: None, + respond_to: RespondTo::OwnerOnly, + respond_to_allowlist: vec![], + display_name: None, + slug: None, + runtime: None, + name_pool: Vec::new(), + is_builtin: false, + is_active: true, + source_team: None, + source_team_persona_slug: None, + definition_respond_to: None, + definition_respond_to_allowlist: Vec::new(), + definition_parallelism: None, + relay_mesh: None, + } +} + +// ── preview_prospective_persona_snapshot (Finding 4: relay-mesh preflight +// ordering) ─────────────────────────────────────────────────────────────── + +/// Regression for the relay-mesh preflight-ordering defect: the preflight +/// used to check `ensure_relay_mesh_for_record` against the record's stale +/// pre-snapshot bytes, so a persona edit that flips `provider` to +/// `relay-mesh` between saves would not trigger the mesh preflight on the +/// very next start — only on the restart after that, once the real +/// `apply_persona_snapshot` had already landed. The preview must reflect the +/// same prospective re-snapshot the real spawn path applies. +#[test] +fn preview_reflects_persona_edit_flipping_provider_into_relay_mesh() { + let record = sample_record(); // provider: None (not relay-mesh) + let mut persona = sample_persona(); + persona.id = "test-persona".into(); + persona.provider = Some("relay-mesh".into()); + persona.model = Some("auto".into()); + + let preview = preview_prospective_persona_snapshot(&record, &[persona]); + + assert_eq!( + preview.provider.as_deref(), + Some("relay-mesh"), + "preview must carry the persona's current provider, not the record's stale None" + ); + assert_eq!(preview.model.as_deref(), Some("auto")); +} + +/// Preview reflects a persona edit flipping AWAY from relay-mesh too — +/// symmetric with the into-mesh case above. +#[test] +fn preview_reflects_persona_edit_flipping_provider_out_of_relay_mesh() { + let mut record = sample_record(); + record.provider = Some("relay-mesh".into()); + record.model = Some("auto".into()); + let mut persona = sample_persona(); + persona.id = "test-persona".into(); + persona.provider = Some("anthropic".into()); + persona.model = Some("claude-opus-4".into()); + + let preview = preview_prospective_persona_snapshot(&record, &[persona]); + + assert_eq!(preview.provider.as_deref(), Some("anthropic")); + assert_eq!(preview.model.as_deref(), Some("claude-opus-4")); +} + +/// The preview must not mutate the record passed in — callers (the mesh +/// preflight) need the original bytes intact for the real snapshot apply +/// that follows. +#[test] +fn preview_does_not_mutate_input_record() { + let record = sample_record(); + let original = record.clone(); + let mut persona = sample_persona(); + persona.id = "test-persona".into(); + persona.provider = Some("relay-mesh".into()); + + let _ = preview_prospective_persona_snapshot(&record, &[persona]); + + assert_eq!(record.provider, original.provider); + assert_eq!(record.model, original.model); +} + +/// Orphaned instance (persona deleted): the preview must pass the record +/// through unchanged so downstream orphan handling (refuse to spawn) sees +/// the same bytes it always would. +#[test] +fn preview_passes_through_unchanged_when_persona_missing() { + let mut record = sample_record(); + record.provider = Some("anthropic".into()); + record.persona_id = Some("deleted-persona".into()); + + let preview = preview_prospective_persona_snapshot(&record, &[]); + + assert_eq!(preview.provider.as_deref(), Some("anthropic")); + assert_eq!(preview.persona_id.as_deref(), Some("deleted-persona")); +} fn sample_persona() -> AgentDefinition { AgentDefinition { @@ -437,43 +574,6 @@ fn persona_content_hash_changes_on_edit() { ); } -// ── persona_field_with_record_fallback ──────────────────────────────────── - -#[test] -fn field_fallback_persona_present_wins() { - assert_eq!( - persona_field_with_record_fallback(Some("persona-model"), Some("record-model")), - Some("persona-model".to_owned()), - ); -} - -#[test] -fn field_fallback_persona_blank_uses_record() { - assert_eq!( - persona_field_with_record_fallback(None, Some("record-model")), - Some("record-model".to_owned()), - ); - assert_eq!( - persona_field_with_record_fallback(Some(" "), Some("record-model")), - Some("record-model".to_owned()), - ); -} - -#[test] -fn field_fallback_both_blank_is_none() { - assert_eq!(persona_field_with_record_fallback(None, None), None); - assert_eq!(persona_field_with_record_fallback(Some(""), Some("")), None); -} - -#[test] -fn field_fallback_record_blank_is_none() { - assert_eq!( - persona_field_with_record_fallback(None, Some(" ")), - None, - "whitespace-only record value must also be treated as blank" - ); -} - // ── PersonaSnapshot.runtime ─────────────────────────────────────────────── /// (b) The snapshot carries the persona's runtime VERBATIM — including None, @@ -484,7 +584,7 @@ fn field_fallback_record_blank_is_none() { #[test] fn snapshot_runtime_verbatim_from_persona() { let persona = sample_persona(); // runtime = Some("goose") - let snap = persona_snapshot_with_agent_config_fallback(&persona, Some("gpt-4"), Some("openai")); + let snap = persona_snapshot(&persona); assert_eq!( snap.runtime.as_deref(), Some("goose"), @@ -493,15 +593,14 @@ fn snapshot_runtime_verbatim_from_persona() { let mut no_runtime = sample_persona(); no_runtime.runtime = None; - let snap = - persona_snapshot_with_agent_config_fallback(&no_runtime, Some("gpt-4"), Some("openai")); + let snap = persona_snapshot(&no_runtime); assert_eq!( snap.runtime, None, "persona runtime None must produce None snapshot (clears stale materialized value)" ); } -// ── persona_snapshot_with_agent_config_fallback ──────────────────────────── +// ── persona_snapshot (definition-authoritative) ────────────────────────── /// Helper: a persona with no model/provider configured. fn blank_model_persona() -> AgentDefinition { @@ -512,25 +611,22 @@ fn blank_model_persona() -> AgentDefinition { } } -/// (a) Persona leaves model/provider blank, agent record has values → -/// record values preserved AND source_version still updated to current hash. +/// Definition-authoritative: blank definition model/provider → snapshot is +/// None. The effective-config resolver handles global fallback at read time. #[test] -fn fallback_preserves_record_values_when_persona_blank() { +fn blank_definition_clears_model_provider() { let persona = blank_model_persona(); let expected_version = persona_content_hash(&persona_event_content(&persona)); - let snapshot = - persona_snapshot_with_agent_config_fallback(&persona, Some("gpt-4o"), Some("openai")); + let snapshot = persona_snapshot(&persona); - assert_eq!( - snapshot.model.as_deref(), - Some("gpt-4o"), - "blank persona model must fall back to agent record value" + assert!( + snapshot.model.is_none(), + "blank definition model must produce None" ); - assert_eq!( - snapshot.provider.as_deref(), - Some("openai"), - "blank persona provider must fall back to agent record value" + assert!( + snapshot.provider.is_none(), + "blank definition provider must produce None" ); assert_eq!( snapshot.source_version, expected_version, @@ -538,124 +634,102 @@ fn fallback_preserves_record_values_when_persona_blank() { ); } -/// (b) Persona has model/provider set → persona wins over agent record. +/// Persona has model/provider set → snapshot carries them verbatim. #[test] -fn fallback_persona_wins_when_set() { +fn snapshot_carries_persona_model_provider() { let persona = sample_persona(); // has model=Some("claude-opus-4"), provider=Some("anthropic") - let snapshot = persona_snapshot_with_agent_config_fallback( - &persona, - Some("gpt-4o"), // agent had a different model - Some("openai"), // agent had a different provider - ); + let snapshot = persona_snapshot(&persona); assert_eq!( snapshot.model.as_deref(), Some("claude-opus-4"), - "persona model must win when persona has a value" + "persona model must be carried verbatim" ); assert_eq!( snapshot.provider.as_deref(), Some("anthropic"), - "persona provider must win when persona has a value" + "persona provider must be carried verbatim" ); } -/// (c) Both blank → snapshot keeps None; a genuinely unconfigured agent +/// Both blank → snapshot keeps None; a genuinely unconfigured agent /// stays unconfigured (no fabricated values). #[test] -fn fallback_both_blank_stays_none() { +fn both_blank_stays_none() { let persona = blank_model_persona(); - let snapshot = persona_snapshot_with_agent_config_fallback( - &persona, None, // agent also has no model - None, // agent also has no provider - ); + let snapshot = persona_snapshot(&persona); assert!( snapshot.model.is_none(), - "neither persona nor agent has model — snapshot must be None" + "blank persona model → snapshot None" ); assert!( snapshot.provider.is_none(), - "neither persona nor agent has provider — snapshot must be None" + "blank persona provider → snapshot None" ); } -/// Whitespace-only values on the persona are treated as blank; agent -/// fallback applies. +/// Whitespace-only values on the definition are preserved verbatim in the +/// snapshot. The effective-config resolver treats whitespace as blank. #[test] -fn fallback_treats_whitespace_only_persona_value_as_blank() { +fn whitespace_only_definition_preserved_verbatim() { let mut persona = sample_persona(); persona.model = Some(" ".to_string()); persona.provider = Some("\t".to_string()); - let snapshot = persona_snapshot_with_agent_config_fallback( - &persona, - Some("claude-opus-4"), - Some("anthropic"), - ); + let snapshot = persona_snapshot(&persona); assert_eq!( snapshot.model.as_deref(), - Some("claude-opus-4"), - "whitespace-only persona model must be treated as blank" + Some(" "), + "whitespace-only model is preserved verbatim in snapshot" ); assert_eq!( snapshot.provider.as_deref(), - Some("anthropic"), - "whitespace-only persona provider must be treated as blank" + Some("\t"), + "whitespace-only provider is preserved verbatim in snapshot" ); } -/// Cross-field independence: persona sets model but not provider → model -/// comes from persona, provider falls back to the record. This is the -/// practically common case (model-only personas). +/// Cross-field independence: definition sets model but not provider → model +/// comes from definition, provider is None (global fallback at read time). #[test] -fn fallback_persona_model_set_provider_blank_uses_record_provider() { - let mut persona = sample_persona(); // model=Some("claude-opus-4"), provider=Some("anthropic") - persona.provider = None; // blank provider on persona - - let snapshot = persona_snapshot_with_agent_config_fallback( - &persona, - Some("gpt-4o"), // record model (should be overridden by persona) - Some("openai"), // record provider (should be preserved) - ); +fn definition_model_set_provider_blank_produces_none_provider() { + let mut persona = sample_persona(); + persona.provider = None; + + let snapshot = persona_snapshot(&persona); assert_eq!( snapshot.model.as_deref(), Some("claude-opus-4"), - "persona model must win when persona has a value" + "definition model must be used" ); - assert_eq!( - snapshot.provider.as_deref(), - Some("openai"), - "record provider must be used when persona provider is blank" + assert!( + snapshot.provider.is_none(), + "definition provider=None → snapshot None" ); } -/// Inverse: persona sets provider but not model → provider comes from -/// persona, model falls back to the record. +/// Definition provider set, model blank → model=None (global fallback at +/// read time), provider from definition. #[test] -fn fallback_persona_provider_set_model_blank_uses_record_model() { - let mut persona = sample_persona(); // model=Some("claude-opus-4"), provider=Some("anthropic") - persona.model = None; // blank model on persona - - let snapshot = persona_snapshot_with_agent_config_fallback( - &persona, - Some("gpt-4o"), // record model (should be preserved) - Some("openai"), // record provider (should be overridden by persona) - ); +fn definition_provider_set_model_blank_produces_none_model() { + let mut persona = sample_persona(); + persona.model = None; - assert_eq!( - snapshot.model.as_deref(), - Some("gpt-4o"), - "record model must be used when persona model is blank" + let snapshot = persona_snapshot(&persona); + + assert!( + snapshot.model.is_none(), + "definition model=None → snapshot None" ); assert_eq!( snapshot.provider.as_deref(), Some("anthropic"), - "persona provider must win when persona has a value" + "definition provider must be used" ); } diff --git a/desktop/src-tauri/src/managed_agents/readiness.rs b/desktop/src-tauri/src/managed_agents/readiness.rs index 87ee6241ee..2a349f247c 100644 --- a/desktop/src-tauri/src/managed_agents/readiness.rs +++ b/desktop/src-tauri/src/managed_agents/readiness.rs @@ -98,11 +98,6 @@ pub(crate) fn resolve_effective_agent_env( // Layer 1: baked build defaults (floor — internal builds only; OSS = empty). let mut env = baked_build_env(); - // Layer 2: runtime metadata env vars (model / provider keys derived from - // the record's structured fields, with global as fallback). - // - // Uses the shared resolver to guarantee readiness and spawn agree on the - // effective model/provider: agent → persona → global → None. let (effective_model, effective_provider) = super::global_config::resolve_effective_model_provider(record, personas, global); @@ -111,8 +106,8 @@ pub(crate) fn resolve_effective_agent_env( rt.model_env_var, rt.provider_env_var, rt.provider_locked, - effective_model, - effective_provider, + effective_model.as_deref(), + effective_provider.as_deref(), ) { env.insert(key.to_string(), value.to_string()); } @@ -138,7 +133,11 @@ pub(crate) fn resolve_effective_agent_env( // Buzz shared compute is a native Buzz provider. Translate it to buzz-agent's // OpenAI-compatible transport only in the effective runtime environment. #[cfg(feature = "mesh-llm")] - super::apply_relay_mesh_env(&mut env, effective_provider, effective_model); + super::apply_relay_mesh_env( + &mut env, + effective_provider.as_deref(), + effective_model.as_deref(), + ); EffectiveAgentEnv { env, diff --git a/desktop/src-tauri/src/managed_agents/relay_mesh.rs b/desktop/src-tauri/src/managed_agents/relay_mesh.rs index a47122df80..934155e881 100644 --- a/desktop/src-tauri/src/managed_agents/relay_mesh.rs +++ b/desktop/src-tauri/src/managed_agents/relay_mesh.rs @@ -1,6 +1,3 @@ -#[cfg(feature = "mesh-llm")] -use super::{ManagedAgentRecord, RelayMeshConfig}; - pub const RELAY_MESH_API_BASE_URL: &str = "http://127.0.0.1:9337/v1"; pub const RELAY_MESH_API_KEY_PLACEHOLDER: &str = "buzz-mesh-local"; pub const RELAY_MESH_PROVIDER_ID: &str = "relay-mesh"; @@ -46,146 +43,11 @@ pub fn apply_relay_mesh_env( env.insert("BUZZ_AGENT_THINKING_EFFORT".to_string(), "none".to_string()); } -/// Resolve a record's relay-mesh config, typed field first. -/// -/// Source of truth is the typed `record.relay_mesh` field. For records saved -/// before that field existed, fall back to detecting the relay-mesh preset -/// from `env_vars` (the legacy discriminator). New records carry the typed -/// field and need no env-var sniffing at all. -#[cfg(feature = "mesh-llm")] -pub fn relay_mesh_config(record: &ManagedAgentRecord) -> Option { - if record.provider.as_deref() == Some(RELAY_MESH_PROVIDER_ID) { - let model_ref = record - .model - .as_deref() - .map(str::trim) - .filter(|value| !value.is_empty()) - .unwrap_or(RELAY_MESH_AUTO_MODEL_ID) - .to_string(); - return Some(RelayMeshConfig { model_ref }); - } - if let Some(config) = &record.relay_mesh { - return Some(config.clone()); - } - relay_mesh_model_id_from_env(record).map(|model_ref| RelayMeshConfig { model_ref }) -} - -/// Returns the relay-mesh model id for agents whose provider env points at the -/// local mesh client endpoint created by Buzz's relay-mesh preset. -/// -/// Prefer [`relay_mesh_config`]; this remains as a convenience for call sites -/// that only need the model id. -#[cfg(feature = "mesh-llm")] -pub fn relay_mesh_model_id(record: &ManagedAgentRecord) -> Option { - relay_mesh_config(record).map(|config| config.model_ref) -} - -/// Legacy env-var discriminator: detects the relay-mesh preset purely from the -/// four preset env vars. Used as a fallback for records saved before the typed -/// `relay_mesh` field existed. -#[cfg(feature = "mesh-llm")] -fn relay_mesh_model_id_from_env(record: &ManagedAgentRecord) -> Option { - let base_url = record.env_vars.get("OPENAI_COMPAT_BASE_URL")?.trim(); - if base_url.trim_end_matches('/') != RELAY_MESH_API_BASE_URL { - return None; - } - let provider = record.env_vars.get("BUZZ_AGENT_PROVIDER")?.trim(); - if provider != "openai" { - return None; - } - let api_key = record.env_vars.get("OPENAI_COMPAT_API_KEY")?.trim(); - if api_key != RELAY_MESH_API_KEY_PLACEHOLDER { - return None; - } - record - .env_vars - .get("OPENAI_COMPAT_MODEL") - .map(|value| value.trim()) - .filter(|value| !value.is_empty()) - .map(str::to_string) -} - #[cfg(all(test, feature = "mesh-llm"))] mod tests { use std::collections::BTreeMap; use super::*; - use crate::managed_agents::{BackendKind, ManagedAgentRecord, RespondTo}; - - fn fixture() -> ManagedAgentRecord { - ManagedAgentRecord { - pubkey: "p".into(), - name: "n".into(), - persona_id: None, - private_key_nsec: "nsec1fake".into(), - auth_tag: Some("tag".into()), - relay_url: "ws://localhost:3000".into(), - avatar_url: None, - acp_command: "buzz-acp".into(), - agent_command: "goose".into(), - agent_command_override: None, - agent_args: vec![], - mcp_command: String::new(), - turn_timeout_seconds: 320, - idle_timeout_seconds: None, - max_turn_duration_seconds: None, - parallelism: 1, - system_prompt: None, - model: None, - env_vars: BTreeMap::new(), - start_on_app_launch: false, - auto_restart_on_config_change: true, - runtime_pid: None, - backend: BackendKind::Local, - backend_agent_id: None, - provider_binary_path: None, - team_id: None, - persona_team_dir: None, - persona_name_in_team: None, - persona_source_version: None, - provider: None, - created_at: "now".into(), - updated_at: "now".into(), - last_started_at: None, - last_stopped_at: None, - last_exit_code: None, - last_error: None, - last_error_code: None, - respond_to: RespondTo::OwnerOnly, - respond_to_allowlist: vec![], - display_name: None, - slug: None, - runtime: None, - name_pool: Vec::new(), - is_builtin: false, - is_active: true, - source_team: None, - source_team_persona_slug: None, - definition_respond_to: None, - definition_respond_to_allowlist: Vec::new(), - definition_parallelism: None, - relay_mesh: None, - } - } - - #[test] - fn relay_mesh_model_id_detects_mesh_preset_env() { - let mut rec = fixture(); - rec.env_vars = BTreeMap::from([ - ("BUZZ_AGENT_PROVIDER".to_string(), "openai".to_string()), - ( - "OPENAI_COMPAT_BASE_URL".to_string(), - "http://127.0.0.1:9337/v1/".to_string(), - ), - ("OPENAI_COMPAT_MODEL".to_string(), "Qwen3".to_string()), - ( - "OPENAI_COMPAT_API_KEY".to_string(), - RELAY_MESH_API_KEY_PLACEHOLDER.to_string(), - ), - ]); - - assert_eq!(relay_mesh_model_id(&rec).as_deref(), Some("Qwen3")); - } #[test] fn native_provider_uses_context_safe_non_reasoning_budget() { @@ -205,95 +67,4 @@ mod tests { Some("none") ); } - - #[test] - fn relay_mesh_model_id_ignores_non_mesh_openai_env() { - let mut rec = fixture(); - rec.env_vars = BTreeMap::from([ - ("BUZZ_AGENT_PROVIDER".to_string(), "openai".to_string()), - ( - "OPENAI_COMPAT_BASE_URL".to_string(), - "https://api.openai.com/v1".to_string(), - ), - ("OPENAI_COMPAT_MODEL".to_string(), "gpt-5".to_string()), - ]); - - assert_eq!(relay_mesh_model_id(&rec), None); - } - - #[test] - fn relay_mesh_model_id_ignores_user_openai_on_same_local_port() { - let mut rec = fixture(); - rec.env_vars = BTreeMap::from([ - ("BUZZ_AGENT_PROVIDER".to_string(), "openai".to_string()), - ( - "OPENAI_COMPAT_BASE_URL".to_string(), - "http://127.0.0.1:9337/v1".to_string(), - ), - ("OPENAI_COMPAT_MODEL".to_string(), "Qwen3".to_string()), - ("OPENAI_COMPAT_API_KEY".to_string(), "real-key".to_string()), - ]); - - assert_eq!(relay_mesh_model_id(&rec), None); - } - - #[test] - fn native_provider_fields_are_authoritative() { - // The whole point: a typed record needs no env-var sniffing to be - // recognized as a relay-mesh agent. - let mut rec = fixture(); - rec.provider = Some(RELAY_MESH_PROVIDER_ID.to_string()); - rec.model = Some("Qwen3".to_string()); - assert!(rec.env_vars.is_empty()); - assert_eq!( - relay_mesh_config(&rec), - Some(RelayMeshConfig { - model_ref: "Qwen3".to_string() - }) - ); - assert_eq!(relay_mesh_model_id(&rec).as_deref(), Some("Qwen3")); - } - - #[test] - fn native_provider_fields_win_over_legacy_config() { - let mut rec = fixture(); - rec.provider = Some(RELAY_MESH_PROVIDER_ID.to_string()); - rec.model = Some("native-model".to_string()); - rec.relay_mesh = Some(RelayMeshConfig { - model_ref: "typed-model".to_string(), - }); - rec.env_vars = BTreeMap::from([ - ("BUZZ_AGENT_PROVIDER".to_string(), "openai".to_string()), - ( - "OPENAI_COMPAT_BASE_URL".to_string(), - "http://127.0.0.1:9337/v1".to_string(), - ), - ("OPENAI_COMPAT_MODEL".to_string(), "env-model".to_string()), - ( - "OPENAI_COMPAT_API_KEY".to_string(), - RELAY_MESH_API_KEY_PLACEHOLDER.to_string(), - ), - ]); - assert_eq!(relay_mesh_model_id(&rec).as_deref(), Some("native-model")); - } - - #[test] - fn legacy_record_falls_back_to_env_sniff() { - // Records saved before the typed field still resolve via env vars. - let mut rec = fixture(); - rec.relay_mesh = None; - rec.env_vars = BTreeMap::from([ - ("BUZZ_AGENT_PROVIDER".to_string(), "openai".to_string()), - ( - "OPENAI_COMPAT_BASE_URL".to_string(), - "http://127.0.0.1:9337/v1".to_string(), - ), - ("OPENAI_COMPAT_MODEL".to_string(), "Qwen3".to_string()), - ( - "OPENAI_COMPAT_API_KEY".to_string(), - RELAY_MESH_API_KEY_PLACEHOLDER.to_string(), - ), - ]); - assert_eq!(relay_mesh_model_id(&rec).as_deref(), Some("Qwen3")); - } } diff --git a/desktop/src-tauri/src/managed_agents/restore.rs b/desktop/src-tauri/src/managed_agents/restore.rs index e4fc18b39f..5ef6640b45 100644 --- a/desktop/src-tauri/src/managed_agents/restore.rs +++ b/desktop/src-tauri/src/managed_agents/restore.rs @@ -1,5 +1,3 @@ -#[cfg(feature = "mesh-llm")] -use super::relay_mesh_model_id; use super::{ find_managed_agent_mut, kill_stale_tracked_processes, load_managed_agents, load_personas, save_managed_agents, spawn_agent_child, sync_managed_agent_processes, BackendKind, @@ -23,10 +21,9 @@ type AgentSpawnResult = (String, SpawnResult); /// `model`/`provider` were clobbered by the old unconditional snapshot code before /// this fix — are skipped here; they self-heal on the next manual start via the /// start-path re-snapshot in `start_local_agent_with_preflight`. -/// If the linked persona is gone, we log loudly and leave the snapshot empty — -/// the record's own `system_prompt`/`model` (possibly empty for persona-created -/// agents) is then all the config that remains, which is the same fallback an -/// orphaned agent already gets. +/// If the linked persona is gone, we log loudly and leave the record untouched — +/// it stays orphaned and `spawn_agent_child` refuses to start it (see +/// `effective_config::resolve_effective_config`'s `OrphanedInstance` arm). pub fn backfill_persona_snapshots(app: &tauri::AppHandle) -> Result<(), String> { let state = app.state::(); let _store_guard = state @@ -53,7 +50,7 @@ pub fn backfill_persona_snapshots(app: &tauri::AppHandle) -> Result<(), String> } let Some(persona) = personas.iter().find(|p| p.id == persona_id) else { eprintln!( - "buzz-desktop: persona-snapshot backfill: agent {} links persona {persona_id} which no longer exists; leaving snapshot empty — it will spawn from its record fields", + "buzz-desktop: persona-snapshot backfill: agent {} links persona {persona_id} which no longer exists; leaving it orphaned — spawn will refuse it", record.pubkey ); continue; @@ -178,6 +175,9 @@ pub async fn restore_managed_agents_on_launch( continue; }; let Some(persona) = personas_for_snapshot.iter().find(|p| p.id == persona_id) else { + // Orphaned: no current persona to re-snapshot from. Leave the + // record as-is — `spawn_agent_child` (Phase B below) refuses to + // spawn it and Phase C persists the refusal to `last_error`. continue; }; super::persona_events::apply_persona_snapshot(record, persona); @@ -212,16 +212,26 @@ pub async fn restore_managed_agents_on_launch( #[cfg(feature = "mesh-llm")] let agents_to_start = { + // Preflight against the same resolution spawn uses — `resolve_effective_config` + // (definition → global fallback) — never the record's own `provider`/`model`/ + // `relay_mesh` bytes. See `start_local_agent_with_preflight` in `commands/agents.rs` + // for the identical rationale on the interactive path. + let personas = load_personas(app).unwrap_or_default(); + let global = super::load_global_agent_config(app).unwrap_or_default(); let mut mesh_preflight_failures = std::collections::HashSet::new(); for record in &agents_to_start { - if relay_mesh_model_id(record).is_none() { + let mesh_model_id = super::effective_config::resolve_effective_relay_mesh_model_id( + record, &personas, &global, + ); + if mesh_model_id.is_none() { continue; } // Auto-start after relaunch: re-resolve a live bootstrap target and // dial it. Skip (with an actionable error) only when no live target // serves this model right now. if let Err(error) = - crate::commands::ensure_relay_mesh_for_record(app, record, false).await + crate::commands::ensure_relay_mesh_for_record(app, mesh_model_id.as_deref(), false) + .await { persist_restore_error(app, &state, &record.pubkey, error)?; mesh_preflight_failures.insert(record.pubkey.clone()); diff --git a/desktop/src-tauri/src/managed_agents/runtime.rs b/desktop/src-tauri/src/managed_agents/runtime.rs index 7429c4ffa1..b953cdde23 100644 --- a/desktop/src-tauri/src/managed_agents/runtime.rs +++ b/desktop/src-tauri/src/managed_agents/runtime.rs @@ -1311,12 +1311,37 @@ pub fn build_managed_agent_summary( } }; - // Display contract: show the pinned record snapshot — what the agent - // actually runs — not the live persona. The drift flags below signal when - // the snapshot has fallen behind an edited persona; showing live values - // next to an "out of date" badge would contradict it. let (persona_out_of_date, persona_orphaned) = persona_drift_state(record, personas); + let global_for_summary = + crate::managed_agents::load_global_agent_config(app).unwrap_or_default(); + let effective_cfg = crate::managed_agents::effective_config::resolve_effective_config( + record, + personas, + &global_for_summary, + ); + let (effective_model, effective_provider, effective_prompt, model_source) = match effective_cfg + { + crate::managed_agents::effective_config::EffectiveConfigResult::Resolved(cfg) => { + let source = cfg.model.source.clone(); + ( + cfg.model.value, + cfg.provider.value, + cfg.system_prompt.value, + Some(source), + ) + } + crate::managed_agents::effective_config::EffectiveConfigResult::OrphanedInstance { + record_pubkey, + missing_persona_id, + } => { + eprintln!( + "orphaned agent instance: pubkey={record_pubkey}, missing_persona_id={missing_persona_id}" + ); + (None, None, None, None) + } + }; + // Restart badge: the running process stamped its effective spawn config // at launch; recompute from current disk state and flag drift. Only a // tracked live process can drift — stopped agents spawn fresh, and @@ -1345,7 +1370,13 @@ pub fn build_managed_agent_summary( runtime.adapter_availability.as_ref(), super::adapter_availability_cached(), ); - hash_drift || availability_drift + // An orphan can never be restarted successfully — + // `spawn_agent_child` refuses it before any process side effect — + // so `needs_restart` must never fire for one regardless of hash or + // availability drift. Surfacing "Restart required" here would offer + // an action guaranteed to fail; the UI shows `persona_orphaned` + // instead (see `ManagedAgentSummary::persona_orphaned`). + restart_eligible(persona_orphaned, hash_drift, availability_drift) }); // Resolve the effective harness the same way, then derive args/mcp from it, @@ -1372,10 +1403,11 @@ pub fn build_managed_agent_summary( idle_timeout_seconds: record.idle_timeout_seconds, max_turn_duration_seconds: record.max_turn_duration_seconds, parallelism: record.parallelism, - system_prompt: record.system_prompt.clone(), + system_prompt: effective_prompt, avatar_url: record.avatar_url.clone(), - model: record.model.clone(), - provider: record.provider.clone(), + model: effective_model, + model_source, + provider: effective_provider, persona_out_of_date, persona_orphaned, needs_restart, @@ -1399,6 +1431,19 @@ pub fn build_managed_agent_summary( }) } +/// Pure predicate: should the "Restart required" badge fire? +/// +/// An orphaned linked instance (its persona/definition no longer exists) +/// can never be restarted successfully — `spawn_agent_child` refuses to +/// spawn it before any process side effect. Surfacing "Restart required" +/// for one would offer an action guaranteed to fail, so this always +/// returns `false` for an orphan regardless of drift. Extracted for unit +/// testing without `AppHandle`/global state, following the +/// `availability_drift` pattern in `discovery.rs`. +fn restart_eligible(persona_orphaned: bool, hash_drift: bool, availability_drift: bool) -> bool { + !persona_orphaned && (hash_drift || availability_drift) +} + pub fn find_managed_agent_mut<'a>( records: &'a mut [ManagedAgentRecord], pubkey: &str, @@ -1480,6 +1525,33 @@ pub fn spawn_agent_child( if let Some(error) = spawn_key_refusal(record) { return Err(error); } + // Resolve the effective harness (agent command) from the linked persona, so + // persona harness edits propagate on the next spawn; an explicit per-agent + // override wins. `agent_args` and `mcp_command` are pure derivations of the + // command, so we recompute them from the effective value rather than the + // frozen record snapshot. Mirrors the model resolution below. + let personas = super::load_personas(app).unwrap_or_default(); + let teams = super::load_teams(app).unwrap_or_default(); + // Load global config once; used for runtime_metadata_env_vars (model/provider fallback) + // and for the env-var merge at spawn time. + let global = crate::managed_agents::load_global_agent_config(app).unwrap_or_default(); + + // Resolve model/provider/prompt ONCE, here, at the shared spawn boundary — + // the single source both the env writes below and `spawn_config_hash` + // read from. Previously prompt was read from the record's own (possibly + // stale, Phase-A-snapshot) bytes while model/provider were resolved live + // from `personas`; a definition edit landing between a caller's snapshot + // apply and this spawn could hand a fresh model/provider to a stale + // prompt. This also folds in orphan refusal via `require_resolved`: every + // caller (interactive start, launch restore, `start_managed_agent_process`) + // inherits it — no caller can bypass this by reaching `spawn_agent_child` + // directly. Checked before any side effect (log marker, log file, process + // spawn) so a refused spawn leaves no trace. + let effective_cfg = crate::managed_agents::effective_config::resolve_effective_config( + record, &personas, &global, + ) + .require_resolved()?; + let log_path = managed_agent_log_path(app, &record.pubkey)?; append_log_marker( &log_path, @@ -1495,16 +1567,6 @@ pub fn spawn_agent_child( let stderr = stdout .try_clone() .map_err(|error| format!("failed to clone log handle: {error}"))?; - // Resolve the effective harness (agent command) from the linked persona, so - // persona harness edits propagate on the next spawn; an explicit per-agent - // override wins. `agent_args` and `mcp_command` are pure derivations of the - // command, so we recompute them from the effective value rather than the - // frozen record snapshot. Mirrors the model resolution below. - let personas = super::load_personas(app).unwrap_or_default(); - let teams = super::load_teams(app).unwrap_or_default(); - // Load global config once; used for runtime_metadata_env_vars (model/provider fallback) - // and for the env-var merge at spawn time. - let global = crate::managed_agents::load_global_agent_config(app).unwrap_or_default(); let effective_command = super::record_agent_command(record, &personas); let agent_args = normalize_agent_args(&effective_command, record.agent_args.clone()); let resolved_acp_command = resolve_command(&record.acp_command) @@ -1726,37 +1788,40 @@ pub fn spawn_agent_child( command.env_remove("BUZZ_ACP_TEAM_INSTRUCTIONS"); } - // System prompt via the shared spawn-effective filter — the SAME function the - // config hash digests, so env write and badge cannot disagree (see - // `effective_spawn_prompt` for the Some("")/None collapse). Model and provider - // use the shared - // resolver: agent → persona → global → None, so a global-default-only agent - // spawns with the correct provider/model env. - let effective_prompt = super::spawn_hash::effective_spawn_prompt(record); - let (effective_model, effective_provider) = - crate::managed_agents::resolve_effective_model_provider(record, &personas, &global); + // Prompt, model, and provider all come from the single `effective_cfg` + // resolved at the top of this function — the SAME resolve `spawn_config_hash` + // performs below, so env write and restart badge cannot disagree. Linked + // instances never consult the record's own model/provider/prompt bytes; + // definition-less instances fall back to their own fields, then global. + // + // Derive the mesh decision BEFORE moving fields out — `relay_mesh_model_id` + // is the single authoritative gate; the mesh-llm block below MUST use it + // rather than re-deriving from `effective_provider` to keep preflight and + // spawn semantics in lock-step (see `EffectiveAgentConfig::relay_mesh_model_id`). + #[cfg(feature = "mesh-llm")] + let mesh_model_id = effective_cfg.relay_mesh_model_id(); + let effective_prompt = effective_cfg.system_prompt.value; + let effective_model = effective_cfg.model.value; + let effective_provider = effective_cfg.provider.value; if let Some(prompt) = &effective_prompt { command.env("BUZZ_ACP_SYSTEM_PROMPT", prompt); } else { command.env_remove("BUZZ_ACP_SYSTEM_PROMPT"); } - if let Some(model) = effective_model { + if let Some(model) = effective_model.as_deref() { command.env("BUZZ_ACP_MODEL", model); } else { command.env_remove("BUZZ_ACP_MODEL"); } - // Baked-in provider defaults for internal builds (buzz-releases sets - // BUZZ_BUILD_BUZZ_AGENT_* at compile time; OSS builds bake nothing). - // Written FIRST so that record/persona metadata env vars below override them. build_buzz_agent_provider_defaults(&mut command); if let Some(meta) = runtime_meta { for (key, value) in runtime_metadata_env_vars( meta.model_env_var, meta.provider_env_var, meta.provider_locked, - effective_model, - effective_provider, + effective_model.as_deref(), + effective_provider.as_deref(), ) { command.env(key, value); } @@ -1846,10 +1911,17 @@ pub fn spawn_agent_child( // Buzz shared compute is stored as a native provider; derive the OpenAI-compatible // transport at spawn time and scrub any unrelated ambient OpenAI key. + // Gate on `mesh_model_id` (derived from `effective_cfg.relay_mesh_model_id()` + // above) — not on `effective_provider` directly — so the mesh gate here + // uses the same trim semantics as the preflight callers. #[cfg(feature = "mesh-llm")] - if effective_provider == Some(super::RELAY_MESH_PROVIDER_ID) { + if let Some(ref mesh_model_id) = mesh_model_id { let mut mesh_env = std::collections::BTreeMap::new(); - super::apply_relay_mesh_env(&mut mesh_env, effective_provider, effective_model); + super::apply_relay_mesh_env( + &mut mesh_env, + Some(super::RELAY_MESH_PROVIDER_ID), + Some(mesh_model_id.as_str()), + ); command.env_remove("OPENAI_API_KEY"); for (key, value) in mesh_env { command.env(key, value); @@ -2092,15 +2164,11 @@ pub(crate) fn runtime_metadata_env_vars<'a>( /// Resolve the effective (prompt, model, provider) triple for a persona-linked agent. /// /// Given a persona_id, finds the persona in the list and returns its system_prompt, -/// model, and provider as the authoritative values. When the persona leaves `model` -/// or `provider` blank (None or whitespace-only), falls back to the record's own -/// field using the same precedence rule as `persona_snapshot_with_agent_config_fallback` -/// so the display surface matches spawn behavior. Falls back to the record's own -/// prompt/model/provider when no persona is linked or found. +/// Resolve effective prompt/model/provider using definition-authoritative +/// semantics for linked instances. /// /// Used by `agent_config.rs` to inject persona defaults into the config surface -/// before running the reader, so BuzzExplicit-tagged fields can be re-tagged to -/// PersonaDefault for fields the record did not independently set. +/// before running the reader. pub(crate) fn resolve_effective_prompt_model_provider( persona_id: Option<&str>, personas: &[crate::managed_agents::types::AgentDefinition], @@ -2108,13 +2176,16 @@ pub(crate) fn resolve_effective_prompt_model_provider( record_model: Option, record_provider: Option, ) -> (Option, Option, Option) { - let fallback = crate::managed_agents::persona_events::persona_field_with_record_fallback; match persona_id.and_then(|pid| personas.iter().find(|p| p.id == pid)) { - Some(p) => ( - Some(p.system_prompt.clone()), - fallback(p.model.as_deref(), record_model.as_deref()), // fallback: record.model - fallback(p.provider.as_deref(), record_provider.as_deref()), // fallback: record.provider - ), + Some(p) => { + fn non_blank(v: Option<&str>) -> Option { + v.filter(|s| !s.trim().is_empty()).map(str::to_owned) + } + let prompt = non_blank(Some(&p.system_prompt)); + let model = non_blank(p.model.as_deref()); + let provider = non_blank(p.provider.as_deref()); + (prompt, model, provider) + } None => (record_prompt, record_model, record_provider), } } diff --git a/desktop/src-tauri/src/managed_agents/runtime/tests.rs b/desktop/src-tauri/src/managed_agents/runtime/tests.rs index 06d3ac6db3..ae9727e1b8 100644 --- a/desktop/src-tauri/src/managed_agents/runtime/tests.rs +++ b/desktop/src-tauri/src/managed_agents/runtime/tests.rs @@ -308,20 +308,16 @@ fn persona_with_provider( // that the old create-time env baking silently blocked. use crate::managed_agents::env_vars::{live_persona_env, merged_user_env}; -use crate::managed_agents::persona_events::persona_snapshot; use std::collections::BTreeMap; /// Apply a persona snapshot onto a record, mirroring `create_managed_agent`: -/// snapshotted prompt/model/provider/source_version are pinned, with the -/// system_prompt unwrapped (the persona always carries one). `env_vars` is -/// deliberately NOT touched — it stays agent overrides only. +/// links the record to `persona.id`, then delegates the actual snapshot +/// (prompt/model/provider/runtime/source_version) to the real production +/// `apply_persona_snapshot` — so a change to that function's behavior is +/// exercised by these tests instead of silently diverging from it. fn pin_persona(record: &mut ManagedAgentRecord, persona: &crate::managed_agents::AgentDefinition) { - let snapshot = persona_snapshot(persona); record.persona_id = Some(persona.id.clone()); - record.system_prompt = snapshot.system_prompt; - record.model = snapshot.model; - record.provider = snapshot.provider; - record.persona_source_version = Some(snapshot.source_version); + crate::managed_agents::persona_events::apply_persona_snapshot(record, persona); } fn persona_v( @@ -421,19 +417,31 @@ fn agent_env_overrides_win_over_persona_env_at_spawn() { } #[test] -fn orphaned_agent_spawns_from_its_own_overrides() { - // Persona deleted: the live merge degrades to the record's own overrides. +fn orphaned_agent_refused_at_spawn_boundary() { + // Persona deleted: `spawn_agent_child` must refuse before any process + // side effect, not silently degrade to the record's stale overrides. + // `require_resolved` on the shared resolver is the pure predicate + // `spawn_agent_child` checks first — this pins the contract without + // needing a real `AppHandle`. let persona = persona_v("p", "prompt", &[("ANTHROPIC_API_KEY", "persona-key")]); let mut record = fixture(RespondTo::Anyone, vec![], Some("tag".into())); record.env_vars = BTreeMap::from([("EXTRA".to_string(), "agent-value".to_string())]); pin_persona(&mut record, &persona); - let env = spawn_user_env(&record, &[]); - assert_eq!(env.get("EXTRA").map(String::as_str), Some("agent-value")); + // The persona is absent from the live catalog — same shape restore/start + // see when a persona was deleted on another device. + let no_personas: &[crate::managed_agents::AgentDefinition] = &[]; + let error = crate::managed_agents::effective_config::resolve_effective_config( + &record, + no_personas, + &Default::default(), + ) + .require_resolved() + .unwrap_err(); assert_eq!( - env.get("ANTHROPIC_API_KEY"), - None, - "a deleted persona's env must not linger" + error, + crate::managed_agents::effective_config::ORPHANED_INSTANCE_ERROR.to_string(), + "an orphaned linked instance must be refused, not spawned from its own overrides" ); } @@ -760,3 +768,37 @@ fn own_group_grandchild_detected_by_ancestor_walk() { unsafe { libc::kill(-(intermediate_pid as i32), libc::SIGKILL) }; let _ = intermediate.wait(); } + +// ── restart_eligible tests ────────────────────────────────────────────── + +#[test] +fn restart_eligible_true_when_non_orphan_has_hash_drift() { + assert!(super::restart_eligible(false, true, false)); +} + +#[test] +fn restart_eligible_true_when_non_orphan_has_availability_drift() { + assert!(super::restart_eligible(false, false, true)); +} + +#[test] +fn restart_eligible_false_when_orphan_has_hash_drift() { + // An orphan can never be restarted successfully — spawn refuses it — + // so hash drift alone must not surface "Restart required". + assert!(!super::restart_eligible(true, true, false)); +} + +#[test] +fn restart_eligible_false_when_orphan_has_availability_drift() { + assert!(!super::restart_eligible(true, false, true)); +} + +#[test] +fn restart_eligible_false_when_orphan_has_no_drift() { + assert!(!super::restart_eligible(true, false, false)); +} + +#[test] +fn restart_eligible_false_when_non_orphan_has_no_drift() { + assert!(!super::restart_eligible(false, false, false)); +} diff --git a/desktop/src-tauri/src/managed_agents/spawn_hash.rs b/desktop/src-tauri/src/managed_agents/spawn_hash.rs index 94b41c7555..307ad37515 100644 --- a/desktop/src-tauri/src/managed_agents/spawn_hash.rs +++ b/desktop/src-tauri/src/managed_agents/spawn_hash.rs @@ -27,24 +27,14 @@ use std::hash::{DefaultHasher, Hash, Hasher}; use super::{ + effective_config::{resolve_effective_config, EffectiveConfigResult}, known_acp_runtime, normalize_agent_args, - persona_events::apply_persona_snapshot, + persona_events::preview_prospective_persona_snapshot, resolve_effective_agent_env, types::{AgentDefinition, ManagedAgentRecord, TeamRecord}, GlobalAgentConfig, }; -/// The prompt a spawn would actually deliver: `Some("")` collapses to `None` -/// because an empty `BUZZ_ACP_SYSTEM_PROMPT` is no prompt. -/// -/// The single source of truth for the spawn env write AND the config hash. -pub(crate) fn effective_spawn_prompt(record: &ManagedAgentRecord) -> Option { - record - .system_prompt - .clone() - .filter(|prompt| !prompt.is_empty()) -} - /// Resolve the current instructions for this instance's deployment-time team binding. /// A deleted team deliberately degrades to no team section. pub(crate) fn effective_team_instructions( @@ -77,12 +67,7 @@ pub(crate) fn spawn_config_hash( // when nothing changed. The persona env itself reaches the hash through // `resolve_effective_agent_env` below; `persona_source_version` is set on // the clone but is not a hash input. - let mut record = record.clone(); - if let Some(persona_id) = record.persona_id.clone() { - if let Some(persona) = personas.iter().find(|p| p.id == persona_id) { - apply_persona_snapshot(&mut record, persona); - } - } + let record = preview_prospective_persona_snapshot(record, personas); let record = &record; let effective_command = crate::managed_agents::record_agent_command(record, personas); @@ -104,15 +89,28 @@ pub(crate) fn spawn_config_hash( // BTreeMap iteration is ordered, so this is deterministic. effective.env.hash(&mut hasher); - // Record fields the spawn env writes read directly. The relay is hashed - // resolved: a blank record relay spawns on the workspace relay, so a - // workspace relay change must trip the badge. + // Relay is hashed resolved: a blank record relay spawns against the active + // workspace relay, so a workspace relay change means a restart would + // change what runs. crate::relay::effective_agent_relay_url(&record.relay_url, workspace_relay).hash(&mut hasher); - // Prompt and runtime-layered team instructions use the same resolver as spawn. - effective_spawn_prompt(record).hash(&mut hasher); + // Team instructions use the same resolver as spawn. effective_team_instructions(record, teams).hash(&mut hasher); - record.model.hash(&mut hasher); - record.provider.hash(&mut hasher); + // Prompt, model, and provider all come from ONE `resolve_effective_config` + // call — the SAME resolve `spawn_agent_child` performs for the env write, + // so env write and this badge cannot disagree. An orphaned link (missing + // definition) hashes as if all three were absent: `spawn_agent_child` + // refuses to spawn an orphan regardless, so this is a display-only + // convenience, not the spawn gate. + let (resolved_prompt, resolved_model, resolved_provider) = + match resolve_effective_config(record, personas, global) { + EffectiveConfigResult::Resolved(cfg) => { + (cfg.system_prompt.value, cfg.model.value, cfg.provider.value) + } + EffectiveConfigResult::OrphanedInstance { .. } => (None, None, None), + }; + resolved_prompt.hash(&mut hasher); + resolved_model.hash(&mut hasher); + resolved_provider.hash(&mut hasher); record.auth_tag.hash(&mut hasher); record.respond_to.as_str().hash(&mut hasher); // The allowlist is hashed as the env receives it: spawn sets diff --git a/desktop/src-tauri/src/managed_agents/spawn_hash/tests.rs b/desktop/src-tauri/src/managed_agents/spawn_hash/tests.rs index 1f53a365f3..d7e4bc08f6 100644 --- a/desktop/src-tauri/src/managed_agents/spawn_hash/tests.rs +++ b/desktop/src-tauri/src/managed_agents/spawn_hash/tests.rs @@ -416,18 +416,123 @@ fn missing_definition_leaves_materialized_runtime_in_hash() { ); } +// ── Global default trips hash for linked inherited agents ───────────────── + #[test] -fn effective_spawn_prompt_matches_hash_semantics() { - // The env write and the hash share effective_spawn_prompt — this row - // pins the helper's own semantics so a refactor of either caller cannot - // silently diverge from the contract. - let mut r = record(); - r.system_prompt = Some(String::new()); +fn global_model_change_trips_hash_for_linked_inherited_agent() { + let mut rec = record(); + rec.persona_id = Some("p1".into()); + rec.model = Some("stale-record-model".into()); + + let personas = vec![persona("p1", Some("goose"), "prompt")]; + + let global_a = GlobalAgentConfig { + model: Some("model-a".to_string()), + provider: Some("prov-a".to_string()), + ..Default::default() + }; + let global_b = GlobalAgentConfig { + model: Some("model-b".to_string()), + provider: Some("prov-b".to_string()), + ..Default::default() + }; + + let hash_a = spawn_config_hash(&rec, &personas, &[], "wss://ws.example", &global_a); + let hash_b = spawn_config_hash(&rec, &personas, &[], "wss://ws.example", &global_b); + + assert_ne!( + hash_a, hash_b, + "changing the global default must trip the hash for a linked inherited agent" + ); +} + +#[test] +fn global_model_change_trips_hash_without_model_env_var() { + let mut rec = record(); + rec.persona_id = Some("p1".into()); + rec.agent_command = "some-harness-without-model-env".into(); + + let personas = vec![{ + let mut p = persona("p1", None, "prompt"); + p.model = None; + p.provider = None; + p + }]; + + let global_a = GlobalAgentConfig { + model: Some("model-a".to_string()), + ..Default::default() + }; + let global_b = GlobalAgentConfig { + model: Some("model-b".to_string()), + ..Default::default() + }; + + let hash_a = spawn_config_hash(&rec, &personas, &[], "wss://ws.example", &global_a); + let hash_b = spawn_config_hash(&rec, &personas, &[], "wss://ws.example", &global_b); + + assert_ne!( + hash_a, hash_b, + "global model change must trip hash even without a model_env_var runtime" + ); +} + +#[test] +fn linked_instance_stale_prompt_bytes_are_inert_at_hash_time() { + // Regression for the split-resolve defect: prompt used to be read from + // the record's own (possibly Phase-A-snapshot-stale) bytes while + // model/provider were resolved live from the definition. A definition + // edit landing between a caller's snapshot apply and spawn could hand a + // fresh model/provider to a stale prompt, and the hash (which already + // resolved model/provider live) would silently agree with a spawn that + // wrote the stale prompt. Now both come from one `resolve_effective_config` + // call, so a record whose own `system_prompt` bytes disagree with the + // live definition must hash exactly as if the record carried the + // definition's prompt verbatim — the record's prompt bytes are inert for + // a linked instance. + let mut rec = record(); + rec.persona_id = Some("p1".into()); + rec.system_prompt = Some("stale prompt on record".into()); + + let mut matching_bytes = rec.clone(); + matching_bytes.system_prompt = Some("live prompt".into()); + + let personas = [persona("p1", Some("goose"), "live prompt")]; + assert_eq!( - effective_spawn_prompt(&r), - None, - "empty collapses to absent" + spawn_config_hash( + &rec, + &personas, + &[], + "wss://ws.example", + &Default::default() + ), + spawn_config_hash( + &matching_bytes, + &personas, + &[], + "wss://ws.example", + &Default::default() + ), + "record's own system_prompt bytes must not affect the hash of a linked instance" + ); +} + +#[test] +fn linked_instance_prompt_model_provider_resolve_from_one_call() { + // The prompt for a linked instance must track the definition, exactly + // like model/provider — a definition prompt edit trips the hash even + // though the record's own (stale) system_prompt bytes are unchanged. + let mut rec = record(); + rec.persona_id = Some("p1".into()); + rec.system_prompt = Some("stale".into()); + + let before = [persona("p1", Some("goose"), "old definition prompt")]; + let after = [persona("p1", Some("goose"), "new definition prompt")]; + + assert_ne!( + spawn_config_hash(&rec, &before, &[], "wss://ws.example", &Default::default()), + spawn_config_hash(&rec, &after, &[], "wss://ws.example", &Default::default()), + "linked instance prompt must resolve from the live definition, not stale record bytes" ); - r.system_prompt = Some("real".into()); - assert_eq!(effective_spawn_prompt(&r).as_deref(), Some("real")); } diff --git a/desktop/src-tauri/src/managed_agents/types.rs b/desktop/src-tauri/src/managed_agents/types.rs index 66b51f8c3a..72ce0ecf38 100644 --- a/desktop/src-tauri/src/managed_agents/types.rs +++ b/desktop/src-tauri/src/managed_agents/types.rs @@ -259,13 +259,19 @@ pub struct ManagedAgentRecord { /// Desired LLM model ID. Matches AgentModelInfo.id from discovery. /// The harness re-discovers the correct ACP switching metadata at session /// creation by matching this ID against the fresh session/new response. + /// For a linked instance this is a legacy/display snapshot only — spawn + /// and deploy resolve the effective model from the definition, never + /// from this field (see `effective_config::resolve_effective_config`). + /// For a definition-less instance this field is authoritative. #[serde(default)] pub model: Option, - /// LLM inference provider snapshotted from the persona at create time - /// (e.g. 'databricks', 'anthropic'). Spawn and deploy read this, never the - /// live persona — so the agent stays pinned to the provider it was created - /// with across restarts. `#[serde(default)]` so pre-existing records - /// deserialize as `None` and get backfilled on first load. + /// LLM inference provider. For a linked instance this is a legacy/display + /// snapshot only — spawn and deploy resolve the effective provider from + /// the definition, never from this field (see + /// `effective_config::resolve_effective_config`). For a definition-less + /// instance this field is authoritative. `#[serde(default)]` so + /// pre-existing records deserialize as `None` and get backfilled on + /// first load. #[serde(default)] pub provider: Option, /// Content hash of the persona at the time this agent was created — the @@ -476,7 +482,11 @@ pub struct ManagedAgentSummary { pub system_prompt: Option, pub avatar_url: Option, pub model: Option, - /// LLM inference provider, from the agent's pinned record snapshot. + #[serde(skip_serializing_if = "Option::is_none")] + pub model_source: Option, + /// LLM inference provider, resolved the same way as `model`/`model_source` + /// (definition → global for linked instances; instance → global for + /// definition-less instances). `None` for an orphaned instance. pub provider: Option, /// `true` when the linked persona has been edited since this agent was /// created — the running agent uses the older pinned snapshot. The UI @@ -485,9 +495,11 @@ pub struct ManagedAgentSummary { /// persona is gone, so there is nothing newer to drift toward). pub persona_out_of_date: bool, /// `true` when the agent was created from a persona that no longer exists. - /// Distinct from out-of-date: there is no current persona to respawn into, - /// so the UI should not prompt a respawn — the pinned snapshot is all the - /// config that remains. + /// Distinct from out-of-date: there is no current persona to respawn into. + /// An orphaned agent also cannot be (re)started — `spawn_agent_child` + /// refuses it (see `effective_config::resolve_effective_config`'s + /// `OrphanedInstance` arm via `require_resolved`) — so the UI + /// should surface that it's stuck, not merely stale. pub persona_orphaned: bool, /// `true` when the running process was spawned with a config that no /// longer matches what a spawn would use today — a plain restart would diff --git a/desktop/src/features/agents/lib/agentCardModelLabel.test.mjs b/desktop/src/features/agents/lib/agentCardModelLabel.test.mjs new file mode 100644 index 0000000000..696a055d20 --- /dev/null +++ b/desktop/src/features/agents/lib/agentCardModelLabel.test.mjs @@ -0,0 +1,58 @@ +import assert from "node:assert/strict"; +import test from "node:test"; + +import { resolveAgentCardModelLabel } from "./agentCardModelLabel.ts"; + +test("resolveAgentCardModelLabel — unspawned definition with explicit model renders the model, not inherited", () => { + const label = resolveAgentCardModelLabel({ + agent: undefined, + personaModel: "gpt-5", + defaultModel: "claude-sonnet", + }); + assert.equal(label, "gpt-5"); +}); + +test("resolveAgentCardModelLabel — unspawned definition with no model renders the default", () => { + const label = resolveAgentCardModelLabel({ + agent: undefined, + personaModel: null, + defaultModel: "claude-sonnet", + }); + assert.equal(label, "Default model (claude-sonnet)"); +}); + +test("resolveAgentCardModelLabel — linked instance inheriting the global default ignores stale persona.model", () => { + const label = resolveAgentCardModelLabel({ + agent: { modelSource: "global", model: "stale-model" }, + personaModel: "gpt-5", + defaultModel: "claude-sonnet", + }); + assert.equal(label, "Default model (claude-sonnet)"); +}); + +test("resolveAgentCardModelLabel — linked instance with no modelSource (legacy/unset) is treated as inherited", () => { + const label = resolveAgentCardModelLabel({ + agent: { modelSource: null, model: "stale-model" }, + personaModel: "gpt-5", + defaultModel: "claude-sonnet", + }); + assert.equal(label, "Default model (claude-sonnet)"); +}); + +test("resolveAgentCardModelLabel — linked instance with an explicit resolved model renders that model", () => { + const label = resolveAgentCardModelLabel({ + agent: { modelSource: "definition", model: "gpt-5" }, + personaModel: "should-not-be-used", + defaultModel: "claude-sonnet", + }); + assert.equal(label, "gpt-5"); +}); + +test("resolveAgentCardModelLabel — non-inherited agent with a blank resolved model falls back to the default", () => { + const label = resolveAgentCardModelLabel({ + agent: { modelSource: "instance_legacy", model: " " }, + personaModel: null, + defaultModel: "claude-sonnet", + }); + assert.equal(label, "Default model (claude-sonnet)"); +}); diff --git a/desktop/src/features/agents/lib/agentCardModelLabel.ts b/desktop/src/features/agents/lib/agentCardModelLabel.ts new file mode 100644 index 0000000000..4ec06f10c5 --- /dev/null +++ b/desktop/src/features/agents/lib/agentCardModelLabel.ts @@ -0,0 +1,44 @@ +import { formatAgentModelLabel } from "./formatAgentModelLabel"; +import type { ManagedAgent } from "@/shared/api/types"; + +/** + * Model label for the Agents-page card, shared by the definition-grouped + * card (`AgentPersonaCard`) and the standalone/legacy card + * (`StandaloneAgentCard`). + * + * A materialized `agent` is authoritative once it exists: its `modelSource` + * says whether the *effective* config (from `resolve_effective_config` on + * the backend) came from the global default or from an explicit + * definition/instance value, so the card never has to re-derive that from + * raw model bytes. Absent or `"global"` renders the default-model label; + * anything else renders the agent's own resolved model. + * + * With NO materialized instance yet (a definition that has never been + * started), there is no `modelSource` to read — the definition itself is + * authoritative, so an explicit `personaModel` must render directly rather + * than falling through to "inherited" for lack of an instance. + */ +export function resolveAgentCardModelLabel(input: { + agent: Pick | undefined; + personaModel: string | null | undefined; + defaultModel: string; +}): string { + if (input.agent) { + const isInherited = + !input.agent.modelSource || input.agent.modelSource === "global"; + if (isInherited) { + return formatDefaultModelLabel(input.defaultModel); + } + return input.agent.model?.trim() + ? formatAgentModelLabel(input.agent.model) + : formatDefaultModelLabel(input.defaultModel); + } + return input.personaModel?.trim() + ? formatAgentModelLabel(input.personaModel) + : formatDefaultModelLabel(input.defaultModel); +} + +export function formatDefaultModelLabel(defaultModel: string) { + const model = defaultModel.trim(); + return model ? `Default model (${model})` : "Default model"; +} diff --git a/desktop/src/features/agents/ui/AgentInstanceEditDialog.tsx b/desktop/src/features/agents/ui/AgentInstanceEditDialog.tsx index 8be5f4719b..bb641e1088 100644 --- a/desktop/src/features/agents/ui/AgentInstanceEditDialog.tsx +++ b/desktop/src/features/agents/ui/AgentInstanceEditDialog.tsx @@ -676,30 +676,36 @@ export function AgentInstanceEditDialog({ parsedParallelism > 0 && parsedParallelism !== agent.parallelism ? parsedParallelism : undefined, - // Use tri-state: send null to clear, value to set, omit if unchanged. + // Linked instances defer model/provider/systemPrompt to the definition. systemPrompt: - (systemPrompt.trim() || null) !== agent.systemPrompt - ? systemPrompt.trim() || null - : undefined, + linkedPersona != null + ? undefined + : (systemPrompt.trim() || null) !== agent.systemPrompt + ? systemPrompt.trim() || null + : undefined, model: - normalizedModel !== (agent.model ?? null) - ? normalizedModel - : undefined, + linkedPersona != null + ? undefined + : normalizedModel !== (agent.model ?? null) + ? normalizedModel + : undefined, // Tri-state provider persistence keyed on providerRuntimeCapability: // "capable" → persist: value if changed, omit if unchanged. // "locked" → clear: send null if provider was set, else omit. // "unknown" → omit always (never send null for a transient state). // llmProviderFieldVisible is for UX visibility only; not used here. provider: - providerRuntimeCapability === "capable" - ? normalizedSubmitProvider !== (agent.provider ?? null) - ? normalizedSubmitProvider - : undefined - : providerRuntimeCapability === "locked" - ? (agent.provider ?? null) !== null - ? null + linkedPersona != null + ? undefined + : providerRuntimeCapability === "capable" + ? normalizedSubmitProvider !== (agent.provider ?? null) + ? normalizedSubmitProvider : undefined - : undefined, // "unknown" → omit always + : providerRuntimeCapability === "locked" + ? (agent.provider ?? null) !== null + ? null + : undefined + : undefined, // "unknown" → omit always envVars: envVarsEqual(submitEnvVars, agent.envVars) ? undefined : submitEnvVars, diff --git a/desktop/src/features/agents/ui/EditAgentAdvancedFields.tsx b/desktop/src/features/agents/ui/EditAgentAdvancedFields.tsx index 7559db0a8a..3f85ce3ae6 100644 --- a/desktop/src/features/agents/ui/EditAgentAdvancedFields.tsx +++ b/desktop/src/features/agents/ui/EditAgentAdvancedFields.tsx @@ -277,29 +277,32 @@ export function EditAgentAdvancedFields({ - {/* System prompt override */} -
- -
-