From 8c0e8cb1656b04ad269bce3c2deeda2a943ae78a Mon Sep 17 00:00:00 2001 From: Will Pfleger Date: Sun, 26 Jul 2026 09:30:33 -0700 Subject: [PATCH] fix(desktop): make agent definition authoritative for model/provider/prompt (#1968) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary Introduces a single effective-config resolver that makes agent definitions authoritative for model, provider, system prompt, and relay-mesh routing on linked instances. Stale materialized record bytes are never consulted at spawn, deploy, readiness, hash, card summary, or mesh preflight. **Resolution semantics (field-specific):** - **model/provider (linked):** definition → global. `None` = inherit global default. Record value never consulted. - **model/provider (definition-less):** instance → global. - **system_prompt (linked):** strictly from definition; blank = no prompt. No global tier for prompt. - **system_prompt (definition-less):** from the instance. - **relay-mesh preflight (both):** driven by the same `resolve_effective_config` resolution spawn's mesh env consults. For a linked instance the record's own `provider`/`model`/`relay_mesh` bytes are never consulted; for a definition-less instance with no provider of its own, the legacy marker and env preset are the last fallback (see below). - **Orphaned (linked record, definition missing):** spawn, deploy, and mesh preflight blocked with actionable user-facing error. **Changes:** - New `effective_config` resolver module with `ConfigSource` metadata (`definition`, `global`, `instance_legacy`) - All consumers routed through the single resolver: local spawn, deploy, readiness, spawn hash, card summary, relay-mesh preflight (interactive start and restore-on-launch) - `apply_persona_snapshot` no longer preserves stale record model/provider when definition is blank - Backend `update_managed_agent` blocks model/provider/prompt writes for linked records - Frontend omits model/provider/systemPrompt submission for linked instances; system prompt override hidden - `model_source` field added to `ManagedAgentSummary` so card labels distinguish inherited (`Default model (X)`) from explicit - Spawn hash now digests resolved model/provider (not raw record fields) so global default changes trip the restart badge even for runtimes without `model_env_var` - Orphan spawn/deploy/mesh-preflight blocked with jargon-free error ("This agent's configuration is missing — it may still be syncing or was deleted on another device") - `EffectiveAgentConfig::relay_mesh_model_id()` and `resolve_effective_relay_mesh_model_id()` added; both mesh preflights (`start_local_agent_with_preflight`, `restore_managed_agents_on_launch`) call this instead of the deleted `relay_mesh_config`/`relay_mesh_model_id` record-byte sniffs - Legacy relay-mesh records keep their mesh routing. Two shipped generations predate `provider: "relay-mesh"` and are never rewritten on load: the typed `relay_mesh` marker (added when `ManagedAgentRecord` had no `provider` field), and before it the mesh preset written directly into `env_vars`. `resolve_definition_less` falls back to the marker, then to the env preset, so these records still resolve to mesh instead of silently misrouting to an unrelated provider while their stale env bytes reach the child. The fallback is skipped when the record carries an explicit `provider` — that states current intent, including a switch away from mesh — and `resolve_linked` has no legacy fallback at all - The env discriminator accepts both spellings of the two sentinels renamed in the Jun-11 window without a record migration: the provider env key (`BUZZ_AGENT_PROVIDER`, previously `SPROUT_AGENT_PROVIDER`) and the api-key value (`buzz-mesh-local`, previously `sprout-mesh-local`). Each is independent, since a record can straddle the window; the current provider-key spelling wins when both are present - Dead `persona_field_with_record_fallback` and wrapper `persona_snapshot_with_agent_config_fallback` deleted; callers use `persona_snapshot` directly - New resolver/deploy/hash/write-guard/mesh-preflight tests, including switch-away and global-inheritance regressions for both mesh preflight call sites, per-class legacy-mesh resolution (typed marker and env preset, in every rename-window spelling combination), and the paired assertions that a linked instance's legacy mesh bytes stay inert --------- Signed-off-by: Will Pfleger Co-authored-by: npub1mn7jgtj4w2pd0g0zeuhxsa6jy6p0rewxz4kujt98my82ahfmp72sxjexk7 --- desktop/scripts/check-file-sizes.mjs | 38 +- .../src-tauri/src/commands/agent_config.rs | 98 +- .../src-tauri/src/commands/agent_models.rs | 73 +- .../src/commands/agent_models_tests.rs | 175 +++- desktop/src-tauri/src/commands/agents.rs | 47 +- .../src-tauri/src/commands/agents_deploy.rs | 79 +- .../src-tauri/src/commands/agents_tests.rs | 87 +- desktop/src-tauri/src/commands/mesh_llm.rs | 14 +- .../managed_agents/effective_config/mod.rs | 318 +++++++ .../managed_agents/effective_config/tests.rs | 872 ++++++++++++++++++ .../src-tauri/src/managed_agents/env_vars.rs | 8 +- .../src/managed_agents/global_config/mod.rs | 69 +- .../src/managed_agents/global_config/tests.rs | 56 +- desktop/src-tauri/src/managed_agents/mod.rs | 1 + .../src/managed_agents/persona_events.rs | 115 +-- .../managed_agents/persona_events/tests.rs | 302 +++--- .../src-tauri/src/managed_agents/readiness.rs | 15 +- .../src/managed_agents/relay_mesh.rs | 229 ----- .../src-tauri/src/managed_agents/restore.rs | 28 +- .../src-tauri/src/managed_agents/runtime.rs | 142 ++- .../src/managed_agents/runtime/metadata.rs | 28 +- .../src/managed_agents/runtime/tests.rs | 74 +- .../src/managed_agents/spawn_hash.rs | 42 +- .../src/managed_agents/spawn_hash/tests.rs | 129 ++- desktop/src-tauri/src/managed_agents/types.rs | 45 +- .../agents/lib/agentCardModelLabel.test.mjs | 58 ++ .../agents/lib/agentCardModelLabel.ts | 44 + .../agents/ui/AgentInstanceEditDialog.tsx | 36 +- .../agents/ui/EditAgentAdvancedFields.tsx | 47 +- .../features/agents/ui/ManagedAgentRow.tsx | 14 +- .../agents/ui/UnifiedAgentsSection.tsx | 47 +- .../features/agents/ui/personaRuntimeModel.ts | 43 +- desktop/src/shared/api/tauri.ts | 3 + desktop/src/shared/api/types.ts | 1 + 34 files changed, 2548 insertions(+), 829 deletions(-) create mode 100644 desktop/src-tauri/src/managed_agents/effective_config/mod.rs create mode 100644 desktop/src-tauri/src/managed_agents/effective_config/tests.rs create mode 100644 desktop/src/features/agents/lib/agentCardModelLabel.test.mjs create mode 100644 desktop/src/features/agents/lib/agentCardModelLabel.ts diff --git a/desktop/scripts/check-file-sizes.mjs b/desktop/scripts/check-file-sizes.mjs index b74e05ae7b..9649e9bbfb 100644 --- a/desktop/scripts/check-file-sizes.mjs +++ b/desktop/scripts/check-file-sizes.mjs @@ -80,7 +80,15 @@ const overrides = new Map([ // ratcheting 1443 -> 1295. Queued to split further in the A2 fold. // global-agent-config: resolve_deploy_model_provider + visibility exports // add ~40 lines on top of the 1A.1 ratchet. Queued to split. - ["src-tauri/src/commands/agents.rs", 1340], + // +22 (1340 -> 1360): agent-config-resolver — start_local_agent_with_preflight + // uses resolve_effective_relay_mesh_model_id at both preflight call sites; + // preview_prospective_persona_snapshot helper extracted; orphan guard threaded + // through restore path. Load-bearing feature changes; queued to split. + // +9 (1360 -> 1369): start_local_agent_pairs_with_preflight — added + // personas/global load + resolve_effective_relay_mesh_model_id call to + // replace stale record-byte preflight. Same resolver pattern as + // start_local_agent_with_preflight. Load-bearing; queued to split. + ["src-tauri/src/commands/agents.rs", 1369], // agent-lifecycle-fixes: cascade-delete in delete_persona restructured into // 3-phase (stage/stop/commit) + commit_cascade_agents injectable helper for // retry-safety. Load-bearing reviewer-required change; queued to split. @@ -168,7 +176,9 @@ const overrides = new Map([ // Windows PATH-correctness fix: 3 #[cfg(windows)] test functions covering // .cmd shim rejection, .bat shim rejection, and .exe acceptance for // configure_runtime_cli (fix #2397). Test-only growth; queued to split. - ["src-tauri/src/managed_agents/runtime/tests.rs", 1041], + // +7 (1041 -> 1048): rebase onto main — this PR's resolver tests land on top + // of main's #2397 Windows shim tests. Test-only; queued to split. + ["src-tauri/src/managed_agents/runtime/tests.rs", 1055], // applyWorkspace reposDir parameter plus the validateReposDir binding, // threaded through Tauri invokes for configurable repos_dir, plus the // harness-persona-sync `harnessOverride` create-input bit — load-bearing @@ -203,7 +213,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/ @@ -438,7 +449,15 @@ 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. + // +4 (1101 -> 1105): rebase onto agents-everywhere — agents.rs function + // signatures updated for ManagedAgentRuntimeKey-keyed runtimes map. + ["src-tauri/src/commands/agent_models.rs", 1113], // 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. @@ -454,7 +473,12 @@ const overrides = new Map([ // relay from the harness-attached payload relayUrl (with effective-relay // fallback for older harnesses) instead of a required arg the frontend // wrapper never passed, which silently broke the session-config cache. - ["src-tauri/src/commands/agent_config.rs", 1050], + // +60 (1050 -> 1110): agent-config-resolver — 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 replaced; two new regression tests added. + ["src-tauri/src/commands/agent_config.rs", 1110], // 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; @@ -532,7 +556,9 @@ const overrides = new Map([ // for Databricks v1 gate; prospectiveRuntimeId default fallback for builtins. // PR-B moves default/API-key derivation into shared hooks; the explicit // hidden-key projection keeps the top-level secret out of Advanced rows. - ["src/features/agents/ui/AgentInstanceEditDialog.tsx", 1195], + // +6 (1195 -> 1201): rebase onto main — this PR's model-source label wiring + // lands on top of main's dialog growth. Queued to split. + ["src/features/agents/ui/AgentInstanceEditDialog.tsx", 1201], // AgentDefinitionDialog grew past 1000 with the following load-bearing fixes: // isRuntimeAutoSeededRef tracking for edit-mode seeding (Fizz shows models); // runtimeSupportsLlmProviderSelection guard on discovery provider (codex fix); diff --git a/desktop/src-tauri/src/commands/agent_config.rs b/desktop/src-tauri/src/commands/agent_config.rs index f6094ed529..461fed5dbd 100644 --- a/desktop/src-tauri/src/commands/agent_config.rs +++ b/desktop/src-tauri/src/commands/agent_config.rs @@ -39,6 +39,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 @@ -59,6 +66,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(); @@ -172,22 +195,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 } @@ -728,11 +735,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 explicit_record_model_outranks_persona_and_keeps_buzz_explicit_origin() { + 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 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 112f297fa7..2e423ed6dd 100644 --- a/desktop/src-tauri/src/commands/agent_models.rs +++ b/desktop/src-tauri/src/commands/agent_models.rs @@ -87,9 +87,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, @@ -161,27 +165,38 @@ 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 runtime_meta = known_acp_runtime(agent_command); + 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) = runtime_meta { 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, provider_env_var: runtime_meta.and_then(|meta| meta.provider_env_var), env: crate::managed_agents::merged_user_env(&derived_env, &record.env_vars), } @@ -775,6 +790,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, @@ -814,15 +858,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 0d6449e781..d962545bdc 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")); @@ -341,6 +346,74 @@ fn effective_discovery_provider_reads_the_runtimes_own_env_var() { ); } +/// 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 // --------------------------------------------------------------------------- @@ -381,6 +454,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 c65900b5de..61e4034b9f 100644 --- a/desktop/src-tauri/src/commands/agents.rs +++ b/desktop/src-tauri/src/commands/agents.rs @@ -293,16 +293,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(()) @@ -327,7 +327,16 @@ pub(super) async fn start_local_agent_pairs_with_preflight( if record_snapshot.backend != BackendKind::Local { return Err(format!("agent {pubkey} is not a local agent")); } - ensure_relay_mesh_for_record(app, &record_snapshot, false).await?; + let personas_for_preflight = load_personas(app).unwrap_or_default(); + let global_for_preflight = + 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_for_preflight, + &global_for_preflight, + ); + ensure_relay_mesh_for_record(app, mesh_model_id.as_deref(), false).await?; { let _store_guard = state @@ -407,7 +416,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). A linked instance's own `provider`/`model`/ + // `relay_mesh` bytes never contribute: 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 @@ -431,9 +455,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 adcdeb7ec1..87af9fa08f 100644 --- a/desktop/src-tauri/src/commands/mesh_llm.rs +++ b/desktop/src-tauri/src/commands/mesh_llm.rs @@ -481,11 +481,11 @@ 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 @@ -505,7 +505,7 @@ pub(crate) async fn ensure_relay_mesh_for_record( .await { mesh_llm::MeshRuntimeRecovery::Live => { - return wait_for_mesh_inference(&model_id).await; + return wait_for_mesh_inference(model_id).await; } mesh_llm::MeshRuntimeRecovery::Evicted | mesh_llm::MeshRuntimeRecovery::Absent => {} mesh_llm::MeshRuntimeRecovery::Debouncing => { @@ -521,7 +521,7 @@ pub(crate) async fn ensure_relay_mesh_for_record( ); } mesh_llm::MeshRuntimeRecovery::Replaced => { - return wait_for_mesh_inference(&model_id).await; + return wait_for_mesh_inference(model_id).await; } mesh_llm::MeshRuntimeRecovery::RestartRequired => { app.request_restart(); @@ -532,7 +532,7 @@ pub(crate) async fn ensure_relay_mesh_for_record( } } } - 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( @@ -556,8 +556,8 @@ pub(crate) async fn ensure_relay_mesh_for_record( // (the router resolves per-request), so it only cold-starts a Client when // there is genuinely no runtime. Falling back to Client if a serve node // crashed under local pressure is a desirable fail-safe, not a regression. - 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..e079c76a1d --- /dev/null +++ b/desktop/src-tauri/src/managed_agents/effective_config/mod.rs @@ -0,0 +1,318 @@ +use std::collections::BTreeMap; + +use serde::Serialize; + +use super::global_config::GlobalAgentConfig; +use super::relay_mesh::{ + RELAY_MESH_API_BASE_URL, RELAY_MESH_API_KEY_PLACEHOLDER, 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, + } +} + +/// The API key value the relay-mesh preset wrote before the Jun-11 rename +/// window (#960, `8f580f308`) changed `RELAY_MESH_API_KEY_PLACEHOLDER` from +/// `"sprout-mesh-local"` to its current value. The old string is persisted as a +/// *value* in `env_vars` on records created before that, and no migration ever +/// rewrote it. +const LEGACY_MESH_API_KEY_PLACEHOLDER: &str = "sprout-mesh-local"; + +/// The provider key the preset wrote before #971 (`8c8312932`) renamed it to +/// `BUZZ_AGENT_PROVIDER`. That commit changed source literals only — persisted +/// `env_vars` keys were never migrated. +const LEGACY_MESH_PROVIDER_ENV_KEY: &str = "SPROUT_AGENT_PROVIDER"; + +/// The legacy env discriminator: recognizes the relay-mesh preset purely from +/// the env vars a pre-typed-field record carries, returning its served model id. +/// +/// All three sentinels must match — the local base URL alone is not enough, +/// since a user may point their own OpenAI-compatible provider at the same +/// port. The placeholder API key is what makes this Buzz's own preset. +/// +/// Two of those sentinels were renamed in the same Jun-11 window, in separate +/// commits, with neither migrating persisted records: the provider env *key* +/// (#971) and the API key *value* (#960). Each is therefore accepted under +/// either spelling, independently — a record straddling the window carries one +/// old and one new. In both cases the current spelling is authoritative when +/// present, so a record that has since been rewritten with a non-mesh value is +/// not resurrected by the stale leftover beside it. +/// +/// `OPENAI_COMPAT_BASE_URL` and `OPENAI_COMPAT_MODEL` were never renamed. +/// Nothing beyond these two either/ors is loosened: every sentinel dropped +/// widens the false-positive surface for a user's own openai-compatible agent. +fn mesh_preset_env_model_id(env_vars: &BTreeMap) -> Option { + let base_url = env_vars.get("OPENAI_COMPAT_BASE_URL")?.trim(); + if base_url.trim_end_matches('/') != RELAY_MESH_API_BASE_URL { + return None; + } + let provider = env_vars + .get("BUZZ_AGENT_PROVIDER") + .or_else(|| env_vars.get(LEGACY_MESH_PROVIDER_ENV_KEY))? + .trim(); + if provider != "openai" { + return None; + } + let api_key = env_vars.get("OPENAI_COMPAT_API_KEY")?.trim(); + if api_key != RELAY_MESH_API_KEY_PLACEHOLDER && api_key != LEGACY_MESH_API_KEY_PLACEHOLDER { + return None; + } + non_blank(env_vars.get("OPENAI_COMPAT_MODEL").map(String::as_str)).map(str::to_owned) +} + +/// The mesh model id a definition-less record carries in legacy form, or `None` +/// when it is not a legacy mesh record. +/// +/// Two shipped record generations predate `provider: "relay-mesh"` and are +/// never rewritten on load, so they are still on disk: +/// +/// - the typed `relay_mesh` marker, added before the record had a `provider` +/// field at all; +/// - before that, the mesh preset written straight into `env_vars`. +/// +/// Consulted only by [`resolve_definition_less`]: a record with no definition +/// has nothing authoritative to be overridden by, so its own bytes are the +/// highest-precedence signal it has. A linked instance never reaches here. +fn legacy_record_mesh_model_id(record: &ManagedAgentRecord) -> Option { + match &record.relay_mesh { + // The marker itself is the mesh signal; a blank `model_ref` still means + // mesh, resolved to the auto model exactly as `apply_relay_mesh_env` + // and `relay_mesh_model_id` treat a blank model. + Some(config) => Some( + non_blank(Some(config.model_ref.as_str())) + .unwrap_or(RELAY_MESH_AUTO_MODEL_ID) + .to_owned(), + ), + None => mesh_preset_env_model_id(&record.env_vars), + } +} + +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, + }; + + let mut config = EffectiveAgentConfig { + model, + provider, + system_prompt, + }; + + // Legacy mesh compatibility. A record with an explicit `provider` has + // already stated its intent — including switching AWAY from mesh, which + // leaves the old marker and env bytes behind — so its legacy bytes are + // never consulted. Only a record that never carried a provider at all + // falls back, and both fields move together so the single mesh gate + // (`relay_mesh_model_id`) and the spawned model agree. + if non_blank(record.provider.as_deref()).is_none() { + if let Some(model_ref) = legacy_record_mesh_model_id(record) { + config.provider = ResolvedField { + value: Some(RELAY_MESH_PROVIDER_ID.to_owned()), + source: ConfigSource::InstanceLegacy, + }; + config.model = ResolvedField { + value: Some(model_ref), + source: ConfigSource::InstanceLegacy, + }; + } + } + + config +} + +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). A linked instance's own +/// `provider`/`model`/`relay_mesh` bytes never contribute; a definition-less +/// legacy record may fall back to them via `legacy_record_mesh_model_id`, +/// which is confined to `resolve_definition_less`. +/// +/// `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..81c2611d5c --- /dev/null +++ b/desktop/src-tauri/src/managed_agents/effective_config/tests.rs @@ -0,0 +1,872 @@ +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" + ); +} + +// ── Legacy relay-mesh record compatibility (Wes review 3 on #1968) ── +// +// Two record generations shipped before `provider: "relay-mesh"` existed and +// are never rewritten on load, so they are still on disk: +// +// Class A (#798) — the four mesh preset env vars in `env_vars`, no typed +// field, no `provider` (the record had no `provider` field yet). +// Class B (#879) — the typed `relay_mesh` marker, still no `provider` field. +// +// Without the fallback both classes resolve to a non-mesh provider while their +// own stale env bytes still reach the child: a silent misroute, not a clean +// failure. The fallback lives in `resolve_definition_less` only — a linked +// instance's definition stays authoritative. + +/// Class A: env-only legacy record resolves mesh via the four-env preset. +#[test] +fn legacy_record_falls_back_to_env_sniff() { + let mut rec = record(None, None, None, 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!( + resolve_effective_relay_mesh_model_id(&rec, &[], &global(None, None)).as_deref(), + Some("Qwen3"), + "env-only legacy mesh record must still resolve to its served model" + ); +} + +/// Class A, oldest shipped record: both renamed sentinels in their original +/// spelling. The preset first wrote `SPROUT_AGENT_PROVIDER` (#798) and the +/// api-key value `sprout-mesh-local`; #971 and #960 renamed each in the same +/// Jun-11 window without migrating persisted `env_vars`, so this exact shape is +/// still on disk and must resolve mesh. +#[test] +fn legacy_record_falls_back_to_env_sniff_with_pre_rename_sentinels() { + let mut rec = record(None, None, None, None); + rec.env_vars = BTreeMap::from([ + ("SPROUT_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(), + "sprout-mesh-local".to_string(), + ), + ]); + + assert_eq!( + resolve_effective_relay_mesh_model_id(&rec, &[], &global(None, None)).as_deref(), + Some("Qwen3"), + "the oldest shipped mesh record carries both pre-rename sentinels and was never migrated" + ); +} + +/// Class A straddling the rename window: old provider key with the new api-key +/// value. Proves the two either/ors are independent rather than one either/or on +/// a bundled old/new pair. +#[test] +fn legacy_record_falls_back_to_env_sniff_with_mixed_rename_sentinels() { + let mut rec = record(None, None, None, None); + rec.env_vars = BTreeMap::from([ + ("SPROUT_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!( + resolve_effective_relay_mesh_model_id(&rec, &[], &global(None, None)).as_deref(), + Some("Qwen3"), + "old provider key with new api key must resolve — the sentinels renamed independently" + ); +} + +/// The mirrored straddle: new provider key with the old api-key value. +#[test] +fn legacy_record_falls_back_to_env_sniff_with_new_provider_and_old_api_key() { + let mut rec = record(None, None, None, 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(), + "sprout-mesh-local".to_string(), + ), + ]); + + assert_eq!( + resolve_effective_relay_mesh_model_id(&rec, &[], &global(None, None)).as_deref(), + Some("Qwen3"), + "new provider key with old api key must resolve too" + ); +} + +/// Both spellings present and disagreeing: the renamed key is authoritative, +/// so a record whose current provider env is non-mesh is not resurrected as +/// mesh by the stale `SPROUT_` leftover. +#[test] +fn legacy_env_sniff_prefers_renamed_provider_key_over_stale_old_key() { + let mut rec = record(None, None, None, None); + rec.env_vars = BTreeMap::from([ + ("BUZZ_AGENT_PROVIDER".to_string(), "anthropic".to_string()), + ("SPROUT_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!( + resolve_effective_relay_mesh_model_id(&rec, &[], &global(None, None)), + None, + "the renamed key states current intent — a stale SPROUT_ key must not override it" + ); +} + +/// Class A negative: a user's own OpenAI-compatible provider on the same local +/// port is not Buzz's preset — the placeholder API key is the discriminator. +#[test] +fn legacy_env_sniff_ignores_user_openai_on_same_local_port() { + let mut rec = record(None, None, None, 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(), "real-key".to_string()), + ]); + + assert_eq!( + resolve_effective_relay_mesh_model_id(&rec, &[], &global(None, None)), + None, + "a real user key on the mesh port must not be mistaken for Buzz's preset" + ); +} + +/// Class B: typed marker with no `provider` resolves mesh, and its `model_ref` +/// wins over an unrelated global model so preflight and spawn agree. +#[test] +fn legacy_record_falls_back_to_typed_marker_without_provider() { + let mut rec = record(None, None, None, None); + rec.relay_mesh = Some(crate::managed_agents::RelayMeshConfig { + model_ref: "Qwen3".to_string(), + }); + + let cfg = match resolve_effective_config(&rec, &[], &global(Some("gpt-5"), Some("openai"))) { + EffectiveConfigResult::Resolved(cfg) => cfg, + EffectiveConfigResult::OrphanedInstance { .. } => { + panic!("definition-less is never orphaned") + } + }; + + assert_eq!(cfg.provider.value.as_deref(), Some(RELAY_MESH_PROVIDER_ID)); + assert_eq!( + cfg.relay_mesh_model_id().as_deref(), + Some("Qwen3"), + "typed marker must resolve mesh even with no provider on the record" + ); +} + +/// Class B with a blank `model_ref`: the marker is still the mesh signal, and +/// the model resolves to auto — same rule `apply_relay_mesh_env` applies. +#[test] +fn legacy_typed_marker_with_blank_model_resolves_auto() { + let mut rec = record(None, None, None, None); + rec.relay_mesh = Some(crate::managed_agents::RelayMeshConfig { + model_ref: " ".to_string(), + }); + + assert_eq!( + resolve_effective_relay_mesh_model_id(&rec, &[], &global(None, None)).as_deref(), + Some(RELAY_MESH_AUTO_MODEL_ID) + ); +} + +/// A definition-less record that was switched AWAY from mesh keeps its stale +/// marker and env bytes on disk. Its explicit `provider` states current intent, +/// so the legacy fallback must not resurrect mesh. +#[test] +fn definition_less_explicit_provider_wins_over_stale_legacy_mesh_bytes() { + let mut rec = record(None, Some("claude-opus-4-6"), Some("anthropic"), None); + rec.relay_mesh = Some(crate::managed_agents::RelayMeshConfig { + model_ref: "Qwen3".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(), "Qwen3".to_string()), + ( + "OPENAI_COMPAT_API_KEY".to_string(), + RELAY_MESH_API_KEY_PLACEHOLDER.to_string(), + ), + ]); + + let cfg = match resolve_effective_config(&rec, &[], &global(None, None)) { + EffectiveConfigResult::Resolved(cfg) => cfg, + EffectiveConfigResult::OrphanedInstance { .. } => { + panic!("definition-less is never orphaned") + } + }; + + assert_eq!(cfg.provider.value.as_deref(), Some("anthropic")); + assert_eq!(cfg.model.value.as_deref(), Some("claude-opus-4-6")); + assert_eq!( + cfg.relay_mesh_model_id(), + None, + "a switched-away legacy record must not be dragged back onto mesh" + ); +} + +/// The test that proves the fallback did not reintroduce the bug this PR +/// deletes: a LINKED record carrying both legacy mesh signals resolves purely +/// from its definition and never trips mesh. +#[test] +fn linked_record_ignores_legacy_mesh_marker_and_env() { + let mut rec = record(Some("d1"), Some("auto"), None, None); + rec.relay_mesh = Some(crate::managed_agents::RelayMeshConfig { + model_ref: "Qwen3".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(), "Qwen3".to_string()), + ( + "OPENAI_COMPAT_API_KEY".to_string(), + RELAY_MESH_API_KEY_PLACEHOLDER.to_string(), + ), + ]); + let defs = vec![definition( + "d1", + Some("claude-opus-4-6"), + Some("anthropic"), + "", + )]; + + let cfg = match resolve_effective_config(&rec, &defs, &global(None, None)) { + EffectiveConfigResult::Resolved(cfg) => cfg, + EffectiveConfigResult::OrphanedInstance { .. } => panic!("definition exists"), + }; + + assert_eq!(cfg.provider.value.as_deref(), Some("anthropic")); + assert_eq!(cfg.model.value.as_deref(), Some("claude-opus-4-6")); + assert_eq!( + cfg.relay_mesh_model_id(), + None, + "legacy record bytes must never influence a linked instance" + ); +} + +/// A linked instance whose definition inherits a blank provider from a blank +/// global must NOT pick up the record's legacy mesh bytes either — the linked +/// path has no legacy fallback at all, by construction. +#[test] +fn linked_record_with_legacy_bytes_inherits_global_not_mesh() { + let mut rec = record(Some("d1"), None, None, None); + rec.relay_mesh = Some(crate::managed_agents::RelayMeshConfig { + model_ref: "Qwen3".to_string(), + }); + let defs = vec![definition("d1", None, None, "")]; + + let cfg = match resolve_effective_config(&rec, &defs, &global(Some("gpt-5"), Some("openai"))) { + EffectiveConfigResult::Resolved(cfg) => cfg, + EffectiveConfigResult::OrphanedInstance { .. } => panic!("definition exists"), + }; + + assert_eq!(cfg.provider.value.as_deref(), Some("openai")); + assert_eq!(cfg.model.value.as_deref(), Some("gpt-5")); + assert_eq!(cfg.relay_mesh_model_id(), None); +} 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 66f3d882b9..d213142352 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 2033003bc8..5b62615a8c 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). +/// 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. /// -/// 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. -/// -/// `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); } @@ -507,5 +452,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 +fn definition_model_set_provider_blank_produces_none_provider() { + let mut persona = sample_persona(); + persona.provider = None; - 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) - ); + 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 +fn definition_provider_set_model_blank_produces_none_model() { + let mut persona = sample_persona(); + persona.model = None; - 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) - ); + let snapshot = persona_snapshot(&persona); - assert_eq!( - snapshot.model.as_deref(), - Some("gpt-4o"), - "record model must be used when persona model is blank" + 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 c04dfa88a4..fb45cc9a4e 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 fab481e12b..1910620159 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, @@ -36,10 +34,9 @@ type AgentSpawnResult = (String, SpawnOutcome); /// `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 @@ -66,7 +63,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; @@ -206,6 +203,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); @@ -240,16 +240,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). A linked instance's own `provider`/`model`/ + // `relay_mesh` bytes never contribute. 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 b9c7ec65ea..79ee8df1ac 100644 --- a/desktop/src-tauri/src/managed_agents/runtime.rs +++ b/desktop/src-tauri/src/managed_agents/runtime.rs @@ -189,12 +189,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 the // tracked live pair for THIS workspace can drift — stopped agents spawn @@ -227,7 +252,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, @@ -254,10 +285,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, @@ -281,6 +313,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, @@ -389,6 +434,33 @@ pub fn spawn_agent_child( return Err(error); } let runtime_key = ManagedAgentRuntimeKey::new(record.pubkey.clone(), relay_url)?; + // 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 = super::managed_agent_runtime_log_path(app, &runtime_key)?; append_log_marker( &log_path, @@ -404,16 +476,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) @@ -628,37 +690,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); } @@ -749,10 +814,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); diff --git a/desktop/src-tauri/src/managed_agents/runtime/metadata.rs b/desktop/src-tauri/src/managed_agents/runtime/metadata.rs index 56d2e68353..ac9e3a0a3f 100644 --- a/desktop/src-tauri/src/managed_agents/runtime/metadata.rs +++ b/desktop/src-tauri/src/managed_agents/runtime/metadata.rs @@ -24,18 +24,11 @@ pub(crate) fn runtime_metadata_env_vars<'a>( vars } -/// 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], @@ -43,13 +36,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 019bfe61d3..55fa8efba7 100644 --- a/desktop/src-tauri/src/managed_agents/runtime/tests.rs +++ b/desktop/src-tauri/src/managed_agents/runtime/tests.rs @@ -315,20 +315,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( @@ -428,19 +424,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" ); } @@ -1010,3 +1018,37 @@ fn invalid_pubkey_resolves_no_pair_key() { // the summary must fall back to the stopped/legacy-pid path, not panic. assert!(super::resolve_workspace_pair_key("not-a-key", "", "wss://one.example").is_none()); } + +// ── 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 4d80549288..8cf0014e36 100644 --- a/desktop/src-tauri/src/managed_agents/spawn_hash.rs +++ b/desktop/src-tauri/src/managed_agents/spawn_hash.rs @@ -28,24 +28,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( @@ -78,12 +68,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); @@ -109,11 +94,24 @@ pub(crate) fn spawn_config_hash( // resolved: every record spawns on the workspace relay (legacy pins // ignored), so a workspace relay change must trip the badge. 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 6438b3b37d..44ad7f4bbc 100644 --- a/desktop/src-tauri/src/managed_agents/spawn_hash/tests.rs +++ b/desktop/src-tauri/src/managed_agents/spawn_hash/tests.rs @@ -441,18 +441,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()); - assert_eq!( - effective_spawn_prompt(&r), - None, - "empty collapses to absent" +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!( + 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 af4908911b..cce2cd4039 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 @@ -390,12 +396,15 @@ pub struct ManagedAgentRecord { /// inference through Buzz's relay-mesh local endpoint; the `model_ref` is /// the served model id to route to. `None` is a normal agent. /// - /// This is the source of truth for "is this a mesh agent + which model" — - /// replacing the old practice of sniffing it back out of `env_vars` - /// (`relay_mesh_config`). Spawn-time env vars are *derived from* this, not - /// the other way around. `#[serde(default)]` so pre-existing saved records - /// deserialize as `None` and are resolved via the env-var fallback until - /// they are rewritten with this field. + /// Not the source of truth. `provider == "relay-mesh"` is, resolved through + /// `effective_config::resolve_effective_config`; spawn-time env vars are + /// derived from that resolution. This field is retained solely as a + /// backward-compatibility signal for records written before the record had + /// a `provider` field, and is consulted only for definition-less records + /// that carry no provider — after which the env-var preset is the last + /// fallback. A linked instance's marker is never read: its definition is + /// authoritative. `#[serde(default)]` so records predating the field + /// deserialize as `None`. #[serde(default, skip_serializing_if = "Option::is_none")] pub relay_mesh: Option, } @@ -478,7 +487,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 @@ -487,9 +500,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 86a70c3db6..601d57f95d 100644 --- a/desktop/src/features/agents/ui/AgentInstanceEditDialog.tsx +++ b/desktop/src/features/agents/ui/AgentInstanceEditDialog.tsx @@ -655,30 +655,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 35de0d983d..8412792a2c 100644 --- a/desktop/src/features/agents/ui/EditAgentAdvancedFields.tsx +++ b/desktop/src/features/agents/ui/EditAgentAdvancedFields.tsx @@ -211,29 +211,32 @@ export function EditAgentAdvancedFields({ - {/* System prompt override */} -
- -
-