diff --git a/desktop/scripts/check-file-sizes.mjs b/desktop/scripts/check-file-sizes.mjs index 149b1e08cc..0c413845ce 100644 --- a/desktop/scripts/check-file-sizes.mjs +++ b/desktop/scripts/check-file-sizes.mjs @@ -179,7 +179,10 @@ const overrides = new Map([ // path instead of being dropped back to inherit. Load-bearing, not debt. // unified-agent-model 1A.1: inline test module moved to discovery/tests.rs, // ratcheting 1259 -> 802 (under the 1000 default; entry kept as a ratchet). - ["src-tauri/src/managed_agents/discovery.rs", 802], + // agent-config-propagation: the agent_command_override decision family + // (divergent / create-time / update-time / apply) moved to + // discovery/overrides.rs; ratcheting 802 -> 685 to bank the headroom. + ["src-tauri/src/managed_agents/discovery.rs", 685], // identity-import-keyring: the identity resolution state machine's behavioral // matrix (46 tests over FakeIdentityStore — probe × marker × file cells, // adoption / read-back-corruption / marker-failure arms, recovery-mode diff --git a/desktop/src-tauri/src/commands/agent_models.rs b/desktop/src-tauri/src/commands/agent_models.rs index 17938e0b90..cc64260872 100644 --- a/desktop/src-tauri/src/commands/agent_models.rs +++ b/desktop/src-tauri/src/commands/agent_models.rs @@ -839,9 +839,9 @@ pub async fn update_managed_agent( if let Some(parallelism) = input.parallelism { record.parallelism = parallelism; } - if let Some(turn_timeout_seconds) = input.turn_timeout_seconds { - record.turn_timeout_seconds = turn_timeout_seconds; - } + // turn_timeout_seconds is intentionally not applied here — + // BUZZ_ACP_TURN_TIMEOUT is deprecated and ignored by the harness. + // Use idle_timeout_seconds or max_turn_duration_seconds instead. // Store the relay override exactly as supplied (trimmed). An explicit // value pins the agent; empty falls back to the workspace relay at // read-time. A name-only edit (relay_url == None) leaves the pin intact. @@ -851,33 +851,30 @@ pub async fn update_managed_agent( if let Some(acp_command) = input.acp_command { record.acp_command = acp_command; } - // Harness edit: the persona's runtime is authoritative, so we persist an - // explicit `agent_command_override` ONLY when the user picks a command - // that diverges from the persona. An empty/whitespace value (the - // "Inherit from persona" sentinel) clears the pin back to `None`. A - // name-only edit (`agent_command == None`) leaves the pin intact. - // - // `harness_override` threads the user's explicit intent: when they pick - // a runtime/Custom command in the dialog it is a real pin even if it - // maps to the persona's own runtime, so a same-runtime pick is kept - // rather than dropped back to inherit (see - // `update_time_agent_command_override`). + // Harness edit: the persona's runtime is authoritative, so an explicit + // `agent_command_override` is persisted ONLY when the user picks a + // command that diverges from the persona, and the empty/whitespace + // "Inherit from persona" sentinel clears both the pin and the + // materialized record runtime. A name-only edit + // (`agent_command == None`) leaves the pin intact. `harness_override` + // threads the user's explicit intent — see `apply_agent_command_update` + // and `update_time_agent_command_override` for the full resolution + // rules. if let Some(agent_command) = input.agent_command { let personas = load_personas(&app).unwrap_or_default(); - record.agent_command_override = - crate::managed_agents::update_time_agent_command_override( - record.persona_id.as_deref(), - &personas, - Some(&agent_command), - input.harness_override, - ); + crate::managed_agents::apply_agent_command_update( + record, + &personas, + &agent_command, + input.harness_override, + ); } if let Some(agent_args) = input.agent_args { record.agent_args = agent_args; } - if let Some(mcp_command) = input.mcp_command { - record.mcp_command = mcp_command; - } + // mcp_command is intentionally not applied here — the effective MCP + // command is always catalog-derived (known_acp_runtime at spawn time) + // and the per-record field is never read by the runtime. if let Some(env_vars) = input.env_vars { crate::managed_agents::validate_user_env_keys(&env_vars)?; record.env_vars = env_vars; diff --git a/desktop/src-tauri/src/commands/agent_models_tests.rs b/desktop/src-tauri/src/commands/agent_models_tests.rs index 2fe72e6e1a..0f99927c9e 100644 --- a/desktop/src-tauri/src/commands/agent_models_tests.rs +++ b/desktop/src-tauri/src/commands/agent_models_tests.rs @@ -219,6 +219,39 @@ fn saved_agent_model_discovery_uses_record_snapshot() { // Parse/filter/pagination tests live in crates/buzz-agent/src/catalog.rs // (they moved there with the Option C refactor). +// --------------------------------------------------------------------------- +// Dead-knob guards: mcp_command and turn_timeout_seconds +// --------------------------------------------------------------------------- + +#[test] +fn update_request_mcp_command_parses_for_wire_compat() { + // UpdateManagedAgentRequest accepts mcpCommand for backward-compatibility + // with frontends that still send it: the deprecated field must keep + // parsing cleanly. Nothing consumes it — the patching loop in + // update_managed_agent has no mcp_command arm (the effective MCP command + // is always catalog-derived at spawn). That absent-arm invariant lives in + // the code, not in this test: it only guards the wire shape. + let req: crate::managed_agents::UpdateManagedAgentRequest = + serde_json::from_str(r#"{"pubkey": "abc", "mcpCommand": "user-override"}"#) + .expect("request with deprecated mcpCommand parses"); + assert_eq!(req.mcp_command.as_deref(), Some("user-override")); +} + +#[test] +fn update_request_turn_timeout_parses_for_wire_compat() { + // UpdateManagedAgentRequest accepts turnTimeoutSeconds for + // backward-compatibility with frontends that still send it: the deprecated + // field must keep parsing cleanly. Nothing consumes it — the patching loop + // in update_managed_agent has no turn_timeout_seconds arm + // (BUZZ_ACP_TURN_TIMEOUT is deprecated and ignored by the harness). That + // absent-arm invariant lives in the code, not in this test: it only + // guards the wire shape. + let req: crate::managed_agents::UpdateManagedAgentRequest = + serde_json::from_str(r#"{"pubkey": "abc", "turnTimeoutSeconds": 9999}"#) + .expect("request with deprecated turnTimeoutSeconds parses"); + assert_eq!(req.turn_timeout_seconds, Some(9999)); +} + #[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 1300b97d61..ab0fa04f5d 100644 --- a/desktop/src-tauri/src/commands/agents.rs +++ b/desktop/src-tauri/src/commands/agents.rs @@ -254,37 +254,14 @@ async fn start_local_agent_with_preflight( return Err(format!("agent {pubkey} is no longer a local agent")); } // Re-snapshot the persona onto the record at every spawn so the agent always - // starts with the current persona config (system_prompt, model, provider). - // This clears the "out of date" drift badge without requiring a - // delete+recreate. `record.env_vars` is NOT rewritten: it holds agent-level - // overrides only, and spawn merges the live persona env underneath at read - // time — so persona env edits refresh here too. When the persona leaves - // model or provider blank, the agent record's own configured values are - // preserved so a user-set model/provider is never clobbered by an - // unconfigured persona. + // starts with the current persona config (system_prompt, model, provider, + // runtime). This clears the "out of date" drift badge without requiring a + // delete+recreate. See `apply_persona_snapshot` for the precedence and + // env-override self-heal rules. if let Some(persona_id) = record.persona_id.clone() { let personas = load_personas(app).unwrap_or_default(); if let Some(persona) = personas.iter().find(|p| p.id == persona_id) { - let snapshot = - crate::managed_agents::persona_events::persona_snapshot_with_agent_config_fallback( - persona, - record.model.as_deref(), // fallback: record.model - record.provider.as_deref(), // fallback: record.provider - ); - if let Some(prompt) = snapshot.system_prompt { - record.system_prompt = Some(prompt); - } - record.model = snapshot.model; - record.provider = snapshot.provider; - // Self-heal records written before env refresh: persona env used to - // be baked into `record.env_vars` at create/spawn, turning inherited - // values into pseudo-overrides that shadow later persona edits. An - // override equal to the persona's current value is indistinguishable - // from inheritance, so drop it and let the live merge supply it. - record - .env_vars - .retain(|k, v| persona.env_vars.get(k) != Some(v)); - record.persona_source_version = Some(snapshot.source_version); + crate::managed_agents::persona_events::apply_persona_snapshot(record, persona); record.updated_at = crate::util::now_iso(); } } @@ -594,18 +571,14 @@ pub async fn create_managed_agent( .collect::>(), ); - let mcp_command = input - .mcp_command - .as_deref() - .map(str::trim) - .filter(|value| !value.is_empty()) - .map(str::to_string) - .unwrap_or_else( - || match crate::managed_agents::known_acp_runtime(&agent_command) { - Some(p) => p.mcp_command.unwrap_or("").to_string(), - None => String::new(), - }, - ); + // Derive MCP command exclusively from the runtime catalog — the + // per-record field is never read at spawn time so user-supplied input + // is silently discarded. Always sourcing from the catalog ensures + // new agents pick up the correct value without any stored override. + let mcp_command = match crate::managed_agents::known_acp_runtime(&agent_command) { + Some(p) => p.mcp_command.unwrap_or("").to_string(), + None => String::new(), + }; // For pack-backed personas, resolve the installed pack path and the // persona's internal name (slug). ACP's resolve_persona_by_name() @@ -699,10 +672,10 @@ pub async fn create_managed_agent( agent_command_override, agent_args, mcp_command, - turn_timeout_seconds: input - .turn_timeout_seconds - .filter(|seconds| *seconds > 0) - .unwrap_or(DEFAULT_AGENT_TURN_TIMEOUT_SECONDS), + // BUZZ_ACP_TURN_TIMEOUT is deprecated and ignored by the harness; + // store the schema default only. Use idle_timeout_seconds or + // max_turn_duration_seconds for actual turn-length control. + turn_timeout_seconds: DEFAULT_AGENT_TURN_TIMEOUT_SECONDS, // 0 or None → harness uses its own default (320s idle, 3600s max), and the CLI also clamps 0 → minimum. idle_timeout_seconds: input.idle_timeout_seconds.filter(|s| *s > 0), max_turn_duration_seconds: input.max_turn_duration_seconds.filter(|s| *s > 0), diff --git a/desktop/src-tauri/src/commands/personas/mod.rs b/desktop/src-tauri/src/commands/personas/mod.rs index c517aaf58e..db81c79655 100644 --- a/desktop/src-tauri/src/commands/personas/mod.rs +++ b/desktop/src-tauri/src/commands/personas/mod.rs @@ -111,20 +111,61 @@ pub async fn create_persona( .map_err(|e| format!("spawn_blocking failed: {e}"))? } +/// Return value of the `update_persona` command. Uses flatten so all +/// `PersonaRecord` fields appear at the top level of the JSON response, +/// alongside the optional `writeback_warning` field — backward-compatible with +/// callers that already destructure a raw persona object. +#[derive(Debug, serde::Serialize)] +pub struct UpdatePersonaResult { + #[serde(flatten)] + persona: PersonaRecord, + /// Non-`None` when the pack `.persona.md` write-back failed (non-fatal). + /// The in-app edit was already saved; the frontend can use this to surface + /// a "pack file diverged" indicator so the user knows to check the file. + #[serde(skip_serializing_if = "Option::is_none")] + pub writeback_warning: Option, +} + +/// Propagate a persona definition's display_name rename to linked agent instances. +/// Only instances whose current `name` equals `old_display_name` are updated; +/// pool-named instances (e.g. "Birch", "Compass") keep their individualised name. +/// Updates both `record.name` (relay display name) and `record.display_name`. +/// Returns the pubkeys of the records that were renamed. +fn propagate_persona_name_rename( + records: &mut [ManagedAgentRecord], + persona_id: &str, + old_display_name: &str, + new_display_name: &str, +) -> Vec { + let mut renamed = Vec::new(); + for record in records.iter_mut() { + if record.persona_id.as_deref() != Some(persona_id) { + continue; + } + if record.name != old_display_name { + continue; // pool-named instance — keep its individualised name + } + record.name = new_display_name.to_string(); + record.display_name = Some(new_display_name.to_string()); + renamed.push(record.pubkey.clone()); + } + renamed +} + #[tauri::command] pub async fn update_persona( input: UpdatePersonaRequest, app: AppHandle, -) -> Result { +) -> Result { use tauri::Manager; /// Profile sync params collected under the store lock for async relay publish. type ProfileSyncParams = Vec<(nostr::Keys, String, String, Option, Option)>; // Phase 1: synchronous save (persona record + linked agent avatar updates) - let (result, profile_sync_params) = tokio::task::spawn_blocking({ + let (result, profile_sync_params, writeback_warning) = tokio::task::spawn_blocking({ let app = app.clone(); - move || -> Result<(PersonaRecord, ProfileSyncParams), String> { + move || -> Result<(PersonaRecord, ProfileSyncParams, Option), String> { let state = app.state::(); let display_name = trim_required(&input.display_name, "Display name")?; // Do not trim system_prompt: `compose_prompt` appends pack_instructions @@ -151,8 +192,10 @@ pub async fn update_persona( return Err("Built-in agents cannot be edited.".to_string()); } - // Track whether avatar changed so we can sync linked agents. + // Track what changed so we can propagate to linked agent records. let avatar_changed = persona.avatar_url != avatar_url; + let name_changed = persona.display_name != display_name; + let old_display_name = persona.display_name.clone(); persona.display_name = display_name; persona.avatar_url = avatar_url; @@ -173,60 +216,79 @@ pub async fn update_persona( apply_persona_behavior(persona, input.behavior)?; persona.updated_at = now_iso(); + let result = persona.clone(); save_personas(&app, &personas)?; - let result = personas - .into_iter() - .find(|record| record.id == input.id) - .ok_or_else(|| format!("agent {} disappeared unexpectedly", input.id))?; // For pack-backed personas, also write the edit back to the source // `.persona.md` so that launch sync (which reads the file) becomes a // no-op rather than overwriting the record we just saved. - write_back_persona_md(&app, &result); + let writeback_warning = write_back_persona_md(&app, &result); retain_persona_pending(&app, &state, &result); try_regenerate_nest(&app); - // If the avatar changed, propagate to linked agent records and - // collect relay profile sync params for the async phase. - let sync_params: ProfileSyncParams = if avatar_changed { + // If the avatar or display_name changed, propagate to linked agent + // records and collect relay profile sync params for the async phase. + let sync_params: ProfileSyncParams = if avatar_changed || name_changed { let mut records = load_managed_agents(&app)?; let mut params: ProfileSyncParams = Vec::new(); let mut agents_modified = false; let workspace_relay = crate::relay::relay_ws_url_with_override(&state); + // Propagate the display_name rename to instances that still + // carry the old definition display_name (pool-named instances + // keep their individualised name) in one pass; the loop below + // only decides which records need a relay profile sync. + let renamed: Vec = if name_changed { + propagate_persona_name_rename( + &mut records, + &result.id, + &old_display_name, + &result.display_name, + ) + } else { + Vec::new() + }; + for record in records.iter_mut() { if record.persona_id.as_deref() != Some(&result.id) { continue; } - // Update the persisted avatar so reconciliation on next - // start agrees with what we're about to publish. - // When the persona avatar is cleared, fall back to the - // command-default icon so the record never stores `None` - // (which reconcile_agent_profile treats as "un-migrated"). - let effective_cmd = effective_agent_command( - record.persona_id.as_deref(), - std::slice::from_ref(&result), - record.agent_command_override.as_deref(), - ); - record.avatar_url = result - .avatar_url - .clone() - .or_else(|| managed_agent_avatar_url(&effective_cmd)); - agents_modified = true; - - if let Ok(agent_keys) = nostr::Keys::parse(&record.private_key_nsec) { - let relay_url = crate::relay::effective_agent_relay_url( - &record.relay_url, - &workspace_relay, + let mut record_changed = renamed.contains(&record.pubkey); + + if avatar_changed { + // Update the persisted avatar so reconciliation on next + // start agrees with what we're about to publish. + // When the persona avatar is cleared, fall back to the + // command-default icon so the record never stores `None` + // (which reconcile_agent_profile treats as "un-migrated"). + let effective_cmd = effective_agent_command( + record.persona_id.as_deref(), + std::slice::from_ref(&result), + record.agent_command_override.as_deref(), ); - params.push(( - agent_keys, - relay_url, - record.name.clone(), - record.avatar_url.clone(), - record.auth_tag.clone(), - )); + record.avatar_url = result + .avatar_url + .clone() + .or_else(|| managed_agent_avatar_url(&effective_cmd)); + record_changed = true; + } + + if record_changed { + agents_modified = true; + if let Ok(agent_keys) = nostr::Keys::parse(&record.private_key_nsec) { + let relay_url = crate::relay::effective_agent_relay_url( + &record.relay_url, + &workspace_relay, + ); + params.push(( + agent_keys, + relay_url, + record.name.clone(), + record.avatar_url.clone(), + record.auth_tag.clone(), + )); + } } } @@ -239,16 +301,16 @@ pub async fn update_persona( Vec::new() }; - Ok((result, sync_params)) + Ok((result, sync_params, writeback_warning)) } }) .await .map_err(|e| format!("spawn_blocking failed: {e}"))??; - // Phase 2: await relay profile sync for linked agents whose avatar was - // just updated. We await (rather than fire-and-forget) so the frontend - // cache invalidation that follows the mutation settlement sees the fresh - // relay profile. Best-effort — failures are logged, not surfaced. + // Phase 2: await relay profile sync for linked agents whose avatar or + // display_name was just updated. We await (rather than fire-and-forget) + // so the frontend cache invalidation that follows the mutation settlement + // sees the fresh relay profile. Best-effort — failures are logged, not surfaced. if !profile_sync_params.is_empty() { let state = app.state::(); for (agent_keys, relay_url, display_name, avatar_url, auth_tag) in profile_sync_params { @@ -262,14 +324,15 @@ pub async fn update_persona( ) .await { - eprintln!( - "buzz-desktop: relay profile sync failed after persona avatar update: {e}" - ); + eprintln!("buzz-desktop: relay profile sync failed after persona update: {e}"); } } } - Ok(result) + Ok(UpdatePersonaResult { + persona: result, + writeback_warning, + }) } mod writeback; @@ -277,6 +340,8 @@ use writeback::write_back_persona_md; #[cfg(test)] mod inbound_tests; +#[cfg(test)] +mod name_propagation_tests; #[tauri::command] pub async fn delete_persona(id: String, app: AppHandle) -> Result<(), String> { diff --git a/desktop/src-tauri/src/commands/personas/name_propagation_tests.rs b/desktop/src-tauri/src/commands/personas/name_propagation_tests.rs new file mode 100644 index 0000000000..4ca690587d --- /dev/null +++ b/desktop/src-tauri/src/commands/personas/name_propagation_tests.rs @@ -0,0 +1,159 @@ +//! Tests for `propagate_persona_name_rename` — the helper that propagates a +//! persona definition's display_name change to linked agent instances. + +use super::*; + +fn agent(persona_id: &str, name: &str, display_name: Option<&str>) -> ManagedAgentRecord { + ManagedAgentRecord { + pubkey: format!("pubkey-{name}"), + name: name.to_string(), + persona_id: Some(persona_id.to_string()), + private_key_nsec: String::new(), + auth_tag: None, + relay_url: String::new(), + avatar_url: None, + acp_command: String::new(), + agent_command: String::new(), + agent_command_override: None, + agent_args: vec![], + mcp_command: String::new(), + turn_timeout_seconds: 0, + idle_timeout_seconds: None, + max_turn_duration_seconds: None, + parallelism: 1, + system_prompt: None, + model: None, + provider: None, + persona_source_version: None, + mcp_toolsets: None, + env_vars: std::collections::BTreeMap::new(), + start_on_app_launch: false, + auto_restart_on_config_change: true, + runtime_pid: None, + backend: Default::default(), + backend_agent_id: None, + provider_binary_path: None, + persona_team_dir: None, + persona_name_in_team: None, + created_at: String::new(), + updated_at: String::new(), + last_started_at: None, + last_stopped_at: None, + last_exit_code: None, + last_error: None, + last_error_code: None, + respond_to: Default::default(), + respond_to_allowlist: vec![], + display_name: display_name.map(str::to_string), + slug: None, + runtime: None, + name_pool: vec![], + is_builtin: false, + is_active: true, + source_team: None, + source_team_persona_slug: None, + definition_respond_to: None, + definition_respond_to_allowlist: vec![], + definition_mcp_toolsets: None, + definition_parallelism: None, + relay_mesh: None, + } +} + +#[test] +fn test_rename_propagates_to_matching_instance() { + // An instance whose `name` equals the OLD persona display_name must get + // both `name` and `display_name` updated to the new value. + let mut records = vec![agent("persona-1", "Paul", Some("Paul"))]; + + let renamed = propagate_persona_name_rename(&mut records, "persona-1", "Paul", "Paul Atreides"); + + assert_eq!( + renamed, + vec!["pubkey-Paul".to_string()], + "must report the renamed record's pubkey" + ); + assert_eq!(records[0].name, "Paul Atreides", "name must be updated"); + assert_eq!( + records[0].display_name, + Some("Paul Atreides".to_string()), + "display_name must be updated" + ); + // The relay-profile sync params use `record.name`; after rename it carries + // the new display_name, so the relay profile will be published with the correct name. + assert_eq!(records[0].name, "Paul Atreides"); +} + +#[test] +fn test_rename_skips_pool_named_instance() { + // A pool-named instance (e.g. "Birch") has a name DIFFERENT from the + // persona display_name. It must keep its individualised name. + let mut records = vec![agent("persona-1", "Birch", Some("Birch"))]; + + let renamed = propagate_persona_name_rename(&mut records, "persona-1", "Paul", "Paul Atreides"); + + assert!( + renamed.is_empty(), + "pool-named instance must not be reported as renamed" + ); + assert_eq!(records[0].name, "Birch", "pool name must be preserved"); + assert_eq!( + records[0].display_name, + Some("Birch".to_string()), + "pool display_name must be preserved" + ); +} + +#[test] +fn test_rename_propagates_both_name_and_display_name() { + // Explicit dual-field check: BOTH `name` and `display_name` must be + // updated so the relay profile and the local UI are consistent. + let mut records = vec![agent("persona-1", "OldName", None)]; + + propagate_persona_name_rename(&mut records, "persona-1", "OldName", "NewName"); + + assert_eq!(records[0].name, "NewName"); + assert_eq!(records[0].display_name, Some("NewName".to_string())); +} + +#[test] +fn test_rename_only_affects_linked_persona() { + // An instance linked to a DIFFERENT persona must not be touched, even + // if it happens to carry the same display_name. + let mut records = vec![ + agent("persona-1", "Paul", Some("Paul")), + agent("persona-2", "Paul", Some("Paul")), + ]; + + propagate_persona_name_rename(&mut records, "persona-1", "Paul", "Paul Atreides"); + + assert_eq!(records[0].name, "Paul Atreides", "linked instance renamed"); + assert_eq!( + records[1].name, "Paul", + "unrelated persona's instance untouched" + ); +} + +#[test] +fn test_rename_renames_all_matching_instances_in_one_pass() { + // Several instances may carry the definition name (multi-instance deploys + // without a name pool): one call renames every match and reports each + // pubkey, which is what the relay profile sync collection keys on. + let mut records = vec![ + agent("persona-1", "Paul", Some("Paul")), + agent("persona-1", "Paul", Some("Paul")), + agent("persona-1", "Birch", Some("Birch")), + ]; + records[1].pubkey = "pubkey-Paul-2".to_string(); + + let renamed = propagate_persona_name_rename(&mut records, "persona-1", "Paul", "Duncan Idaho"); + + assert_eq!( + renamed, + vec!["pubkey-Paul".to_string(), "pubkey-Paul-2".to_string()], + "every matching instance's pubkey must be reported" + ); + assert_eq!(records[0].name, "Duncan Idaho"); + assert_eq!(records[1].name, "Duncan Idaho"); + assert_eq!(records[2].name, "Birch", "pool-named instance untouched"); +} diff --git a/desktop/src-tauri/src/commands/personas/writeback.rs b/desktop/src-tauri/src/commands/personas/writeback.rs index e5d4fa9618..12e07659fb 100644 --- a/desktop/src-tauri/src/commands/personas/writeback.rs +++ b/desktop/src-tauri/src/commands/personas/writeback.rs @@ -23,6 +23,11 @@ fn find_team_for_persona_source<'a>( /// logged and swallowed so that the in-app edit — already persisted to /// `personas.json` — always lands. /// +/// Returns `Some(warning)` when the write-back failed (non-fatal), `None` on +/// success. The warning is forwarded to the `update_persona` command result so +/// the frontend can surface a "pack file diverged" indicator instead of +/// silently drifting. +/// /// Only the four fields that the UI can set and that live in frontmatter are /// rewritten: `display_name`, `runtime`, `avatar`, and `model` (the combined /// `"provider:model"` string used by the pack format). The markdown body is @@ -42,65 +47,129 @@ fn find_team_for_persona_source<'a>( /// same key as `sync_team_from_dir` (`team_persona_key`). This handles both /// modern teams (where `team.id` equals the manifest id) and legacy/backfilled /// teams (where `team.id` is a UUID and the manifest id lives in `source_dir`). -pub(super) fn write_back_persona_md(app: &AppHandle, persona: &PersonaRecord) { +pub(super) fn write_back_persona_md(app: &AppHandle, persona: &PersonaRecord) -> Option { // Only pack-backed personas have a source file to write back to. + persona.source_team.as_ref()?; + + let result = load_teams(app).and_then(|teams| try_write_back_persona_md(&teams, persona)); + + match result { + Ok(()) => None, + Err(e) => { + eprintln!("buzz-desktop: persona-writeback: {e}"); + Some(e) + } + } +} + +/// Inner logic for write-back, extracted for testability. Takes resolved teams +/// rather than an `AppHandle` so tests can exercise the full path → symlink → +/// pack → rewrite flow without a running Tauri application. +/// +/// Returns `Err` if write-back fails for any reason (non-fatal at the call +/// site); `Ok(())` if the persona has no pack source or the file was updated +/// (or was already current). +fn try_write_back_persona_md(teams: &[TeamRecord], persona: &PersonaRecord) -> Result<(), String> { let Some(source_team_id) = &persona.source_team else { - return; + return Ok(()); // non-pack persona — nothing to write back }; let Some(slug) = &persona.source_team_persona_slug else { - eprintln!( - "buzz-desktop: persona-writeback: persona {} has source_team but no slug; skipping", + return Err(format!( + "persona {} has source_team but no slug; cannot write back", persona.id - ); - return; + )); }; - let result = (|| -> Result<(), String> { - let teams = load_teams(app)?; - let team = find_team_for_persona_source(&teams, source_team_id) - .ok_or_else(|| format!("team {source_team_id} not found"))?; - let source_dir = team - .source_dir - .as_ref() - .ok_or_else(|| "team has no source_dir (JSON-only team)".to_string())?; - - // Resolve the actual source file via the pack manifest, matching the - // same path the launch sync reads. `LoadedPersona.source_path` is the - // absolute path set by `safe_resolve` against the pack root, so it is - // correct regardless of the manifest layout. - let pack = buzz_persona_pkg::pack::load_pack(source_dir) - .map_err(|e| format!("load_pack {}: {e}", source_dir.display()))?; - let loaded = pack - .personas - .iter() - .find(|p| p.name == *slug) - .ok_or_else(|| { - format!( - "persona '{slug}' not found in pack at {}", - source_dir.display() - ) - })?; - let path = &loaded.source_path; - - let content = - std::fs::read_to_string(path).map_err(|e| format!("read {}: {e}", path.display()))?; - - let updated = rewrite_persona_md( - &content, - persona, - &loaded.prompt, - pack.pack_instructions.as_deref(), - )?; - if updated == content { - return Ok(()); - } - std::fs::write(path, &updated).map_err(|e| format!("write {}: {e}", path.display()))?; - Ok(()) - })(); + let team = find_team_for_persona_source(teams, source_team_id) + .ok_or_else(|| format!("team {source_team_id} not found"))?; + let source_dir = team + .source_dir + .as_ref() + .ok_or_else(|| "team has no source_dir (JSON-only team)".to_string())?; + + // Resolve the actual source file via the pack manifest, matching the + // same path the launch sync reads. `LoadedPersona.source_path` is the + // absolute path set by `safe_resolve` against the pack root, so it is + // correct regardless of the manifest layout. + let pack = buzz_persona_pkg::pack::load_pack(source_dir) + .map_err(|e| format!("load_pack {}: {e}", source_dir.display()))?; + let loaded = pack + .personas + .iter() + .find(|p| p.name == *slug) + .ok_or_else(|| { + format!( + "persona '{slug}' not found in pack at {}", + source_dir.display() + ) + })?; + let path = &loaded.source_path; + + // Containment: the manifest-resolved file must stay inside the pack root. + // Both sides are canonicalized so a symlinked pack dir (the legacy deploy + // layout) compares on its resolved location — the check only rejects a + // manifest that redirects the write outside the pack. + let canonical_path = path + .canonicalize() + .map_err(|e| format!("resolve {}: {e}", path.display()))?; + let canonical_root = source_dir + .canonicalize() + .map_err(|e| format!("resolve {}: {e}", source_dir.display()))?; + if !canonical_path.starts_with(&canonical_root) { + return Err(format!( + "persona file {} resolves outside its pack root {}", + canonical_path.display(), + canonical_root.display() + )); + } - if let Err(e) = result { - eprintln!("buzz-desktop: persona-writeback: {e}"); + let content = std::fs::read_to_string(&canonical_path) + .map_err(|e| format!("read {}: {e}", canonical_path.display()))?; + + let updated = rewrite_persona_md( + &content, + persona, + &loaded.prompt, + pack.pack_instructions.as_deref(), + )?; + if updated == content { + return Ok(()); } + write_file_atomic(&canonical_path, &updated) +} + +/// Replace `path`'s content atomically: write a sibling temp file, then rename +/// it over the target so a crash mid-write can never leave a truncated +/// `.persona.md`. The target's write permission is probed first because a +/// rename only needs directory permission — without the probe, an atomic +/// replace would silently defeat a deliberately read-only pack file that the +/// previous in-place write (and the surfaced warning) honored. +fn write_file_atomic(path: &std::path::Path, content: &str) -> Result<(), String> { + use std::io::Write; + + std::fs::OpenOptions::new() + .write(true) + .open(path) + .map_err(|e| format!("write {}: {e}", path.display()))?; + + let parent = path + .parent() + .ok_or_else(|| format!("no parent directory for {}", path.display()))?; + let mut tmp = tempfile::NamedTempFile::new_in(parent) + .map_err(|e| format!("create temp file in {}: {e}", parent.display()))?; + tmp.write_all(content.as_bytes()) + .map_err(|e| format!("write {}: {e}", path.display()))?; + // NamedTempFile creates with 0600; carry the target's permissions over so + // the replacement doesn't change the file's mode. + let perms = std::fs::metadata(path) + .map_err(|e| format!("stat {}: {e}", path.display()))? + .permissions(); + tmp.as_file() + .set_permissions(perms) + .map_err(|e| format!("set permissions on temp for {}: {e}", path.display()))?; + tmp.persist(path) + .map_err(|e| format!("write {}: {e}", path.display()))?; + Ok(()) } /// Rewrite a `.persona.md` file with updated frontmatter fields and, when safe, @@ -232,683 +301,4 @@ fn rewrite_persona_md( } #[cfg(test)] -mod writeback_tests { - use super::*; - use std::collections::BTreeMap; - - /// Build a minimal PersonaRecord with the fields that `rewrite_persona_md` reads. - fn persona( - display_name: &str, - runtime: Option<&str>, - avatar_url: Option<&str>, - provider: Option<&str>, - model: Option<&str>, - ) -> PersonaRecord { - // system_prompt matches the SAMPLE_MD body so the "no prompt edit" path - // is taken in rewrite_persona_md (body preserved byte-for-byte). - persona_with_prompt( - display_name, - runtime, - avatar_url, - provider, - model, - "You are Paul.\n", - ) - } - - /// Like `persona` but with an explicit system_prompt value. - fn persona_with_prompt( - display_name: &str, - runtime: Option<&str>, - avatar_url: Option<&str>, - provider: Option<&str>, - model: Option<&str>, - system_prompt: &str, - ) -> PersonaRecord { - PersonaRecord { - id: "test-id".to_string(), - display_name: display_name.to_string(), - avatar_url: avatar_url.map(str::to_string), - system_prompt: system_prompt.to_string(), - runtime: runtime.map(str::to_string), - model: model.map(str::to_string), - provider: provider.map(str::to_string), - name_pool: vec![], - is_builtin: false, - is_active: true, - source_team: Some("team-1".to_string()), - source_team_persona_slug: Some("paul".to_string()), - env_vars: BTreeMap::new(), - respond_to: None, - respond_to_allowlist: Vec::new(), - mcp_toolsets: None, - parallelism: None, - created_at: "2025-01-01T00:00:00Z".to_string(), - updated_at: "2025-01-01T00:00:00Z".to_string(), - } - } - - const SAMPLE_MD: &str = "\ ---- -name: paul -display_name: \"Paul\" -description: \"An orchestrator.\" -model: goose-claude-4-6-opus -runtime: goose -extra_key: keep-me ---- -You are Paul. -"; - - // ── rewrite_persona_md unit tests ───────────────────────────────────────── - - #[test] - fn test_rewrite_model_provider_joined_and_body_preserved() { - let p = persona( - "Paul", - Some("goose"), - None, - Some("databricks_v2"), - Some("goose-claude-opus-4-8"), - ); - let result = rewrite_persona_md(SAMPLE_MD, &p, "You are Paul.\n", None).unwrap(); - - // Body is byte-preserved. - assert!( - result.ends_with("\nYou are Paul.\n"), - "body not preserved: {result:?}" - ); - - // model key is the joined form. - assert!( - result.contains("model: databricks_v2:goose-claude-opus-4-8"), - "joined model missing: {result}" - ); - - // No separate provider key. - assert!( - !result.contains("provider:"), - "separate provider key must not be emitted: {result}" - ); - - // Unrelated key preserved. - assert!( - result.contains("extra_key: keep-me"), - "extra key lost: {result}" - ); - - // Still valid frontmatter (parses cleanly). - assert!(result.starts_with("---\n"), "must start with ---"); - } - - #[test] - fn test_rewrite_bare_model_when_provider_none() { - let p = persona("Paul", Some("goose"), None, None, Some("bare-model-id")); - let result = rewrite_persona_md(SAMPLE_MD, &p, "You are Paul.\n", None).unwrap(); - - assert!( - result.contains("model: bare-model-id"), - "bare model missing: {result}" - ); - assert!( - !result.contains("provider:"), - "provider key must not be emitted: {result}" - ); - } - - #[test] - fn test_rewrite_runtime_removed_when_none() { - let p = persona("Paul", None, None, None, Some("some-model")); - let result = rewrite_persona_md(SAMPLE_MD, &p, "You are Paul.\n", None).unwrap(); - - // runtime was in source but persona.runtime is None — key must be removed. - assert!( - !result.contains("runtime:"), - "runtime key should be removed when None: {result}" - ); - } - - #[test] - fn test_rewrite_preserves_description_and_name_and_extra_keys() { - let p = persona( - "Paul Updated", - Some("goose"), - None, - Some("anthropic"), - Some("claude-opus-4"), - ); - let result = rewrite_persona_md(SAMPLE_MD, &p, "You are Paul.\n", None).unwrap(); - - // name and description are not persona record fields — must survive untouched. - assert!(result.contains("name: paul"), "name key lost: {result}"); - assert!( - result.contains("description:"), - "description key lost: {result}" - ); - assert!( - result.contains("extra_key: keep-me"), - "extra_key lost: {result}" - ); - - // display_name updated. - assert!( - result.contains("display_name: Paul Updated") - || result.contains("display_name: \"Paul Updated\""), - "display_name not updated: {result}" - ); - } - - #[test] - fn test_rewrite_no_provider_no_model_removes_model_key() { - let p = persona("Paul", None, None, None, None); - let result = rewrite_persona_md(SAMPLE_MD, &p, "You are Paul.\n", None).unwrap(); - - // When both provider and model are cleared, the model key is removed. - assert!( - !result.contains("model:"), - "model key should be removed when both absent: {result}" - ); - } - - #[test] - fn test_rewrite_avatar_set_and_cleared() { - let with_avatar = persona( - "Paul", - Some("goose"), - Some("data:image/png;base64,abc"), - Some("openai"), - Some("gpt-4o"), - ); - let result = rewrite_persona_md(SAMPLE_MD, &with_avatar, "You are Paul.\n", None).unwrap(); - assert!( - result.contains("avatar:"), - "avatar key should be set: {result}" - ); - - let without_avatar = persona("Paul", Some("goose"), None, Some("openai"), Some("gpt-4o")); - let result = - rewrite_persona_md(SAMPLE_MD, &without_avatar, "You are Paul.\n", None).unwrap(); - assert!( - !result.contains("avatar:"), - "avatar key should be absent when None: {result}" - ); - } - - #[test] - fn test_rewrite_body_not_replaced_by_system_prompt() { - // system_prompt on the PersonaRecord is the COMPOSED prompt (body + pack instructions). - // The body of the .persona.md must not be replaced with it. - let p = persona( - "Paul", - Some("goose"), - None, - Some("databricks_v2"), - Some("goose-claude-opus-4-8"), - ); - let result = rewrite_persona_md(SAMPLE_MD, &p, "You are Paul.\n", None).unwrap(); - - // The raw body from the file ("You are Paul.") is preserved. - assert!( - result.ends_with("You are Paul.\n"), - "raw body must be preserved, not replaced by composed system_prompt: {result:?}" - ); - // The composed instructions suffix must NOT appear in the file body. - assert!( - !result.contains("# Team Instructions"), - "composed system_prompt must not be written to body: {result}" - ); - } - - // ── body (system_prompt) write-back tests ───────────────────────────────── - // - // These tests exercise the compose_prompt inversion logic in rewrite_persona_md. - - /// Frontmatter-only MD (no body to preserve) for prompt tests. - const PROMPT_MD: &str = "\ ---- -name: paul -display_name: \"Paul\" -model: goose-claude-4-6-opus ---- -You are Paul. -"; - - #[test] - fn test_prompt_edited_with_pack_instructions_body_rewritten() { - // User edits the prompt. system_prompt = new_raw_body + separator + instructions. - // Body in file should be updated to new_raw_body; Team Instructions must NOT appear. - let instructions = "Follow the rules."; - let new_raw_body = "You are Paul, a wise orchestrator."; - let composed = format!("{new_raw_body}\n\n---\n# Team Instructions\n{instructions}"); - - let mut p = persona("Paul", None, None, None, Some("goose-claude-4-6-opus")); - p.system_prompt = composed; - - let result = - rewrite_persona_md(PROMPT_MD, &p, "You are Paul.", Some(instructions)).unwrap(); - assert!( - result.ends_with("You are Paul, a wise orchestrator."), - "new body not written: {result:?}" - ); - assert!( - !result.contains("# Team Instructions"), - "Team Instructions must not appear in body: {result}" - ); - } - - #[test] - fn test_prompt_edited_no_pack_instructions_body_rewritten_verbatim() { - // No pack instructions: composed == raw. New body is system_prompt verbatim. - let new_raw_body = "You are Paul, updated."; - let mut p = persona("Paul", None, None, None, Some("goose-claude-4-6-opus")); - p.system_prompt = new_raw_body.to_string(); - - let result = rewrite_persona_md(PROMPT_MD, &p, "You are Paul.", None).unwrap(); - assert!( - result.ends_with("You are Paul, updated."), - "body not updated: {result:?}" - ); - } - - #[test] - fn test_prompt_unedited_body_preserved() { - // system_prompt equals compose_prompt(current_raw_body, instructions) exactly. - // The body section must not change even though frontmatter may be rewritten. - let instructions = "Follow the rules."; - let raw_body = "You are Paul."; - let composed = format!("{raw_body}\n\n---\n# Team Instructions\n{instructions}"); - - let mut p = persona("Paul", None, None, None, Some("goose-claude-4-6-opus")); - // Frontmatter also unchanged (matches PROMPT_MD). - p.system_prompt = composed; - - let result = rewrite_persona_md(PROMPT_MD, &p, raw_body, Some(instructions)).unwrap(); - // Body must remain "You are Paul." (no prompt edit — body section preserved). - assert!( - result.ends_with("You are Paul.\n"), - "body must be unchanged: {result:?}" - ); - // Team Instructions must not leak into the body. - assert!( - !result.contains("# Team Instructions"), - "Team Instructions must not appear: {result}" - ); - } - - #[test] - fn test_prompt_safety_guard_missing_suffix_preserves_body() { - // pack_instructions is non-empty but system_prompt does NOT end with the - // expected suffix (user edited inside the Team Instructions block, or the - // instructions drifted). The existing body must be preserved — no corruption. - let instructions = "Follow the rules."; - let mut p = persona("Paul", None, None, None, Some("goose-claude-4-6-opus")); - // system_prompt lacks the expected suffix entirely. - p.system_prompt = "Some rogue prompt with # Team Instructions in the middle".to_string(); - - let result = - rewrite_persona_md(PROMPT_MD, &p, "You are Paul.", Some(instructions)).unwrap(); - // Body preserved from the file. - assert!( - result.ends_with("You are Paul.\n"), - "body must be preserved by safety guard: {result:?}" - ); - } - - #[test] - fn test_prompt_round_trip_no_double_append() { - // After write-back, running compose_prompt on the written body + instructions - // must reproduce the stored system_prompt exactly. This proves no double-append. - let instructions = "Follow the rules."; - let new_raw_body = "You are Paul, updated for the round-trip."; - let composed = format!("{new_raw_body}\n\n---\n# Team Instructions\n{instructions}"); - - let mut p = persona("Paul", None, None, None, Some("goose-claude-4-6-opus")); - p.system_prompt = composed.clone(); - - let result = - rewrite_persona_md(PROMPT_MD, &p, "You are Paul.", Some(instructions)).unwrap(); - - // Extract the written body (everything after the last "---\n"). - let written_body = result.split("---\n").last().unwrap(); - // Re-compose: body + instructions must equal the original composed prompt. - let recomposed = format!("{written_body}\n\n---\n# Team Instructions\n{instructions}"); - assert_eq!( - recomposed, composed, - "round-trip failed — double-append would occur: recomposed={recomposed:?}" - ); - } - - #[test] - fn test_prompt_edited_with_trailing_newline_in_instructions() { - // Regression: normal instructions.md files have a trailing newline. - // compose_prompt includes it verbatim; system_prompt must NOT be trimmed - // in update_persona or the suffix-strip fails and the safety guard fires. - // - // This test verifies that pack_instructions with a trailing "\n" still - // decomposes correctly — i.e., the body is rewritten, not preserved. - let instructions = "Follow the rules.\n"; // trailing newline from file read - let new_raw_body = "You are Paul, rewritten."; - let composed = format!("{new_raw_body}\n\n---\n# Team Instructions\n{instructions}"); - // system_prompt is NOT trimmed (update_persona must preserve it as-is). - - let mut p = persona("Paul", None, None, None, Some("goose-claude-4-6-opus")); - p.system_prompt = composed; - - let result = - rewrite_persona_md(PROMPT_MD, &p, "You are Paul.", Some(instructions)).unwrap(); - assert!( - result.contains("You are Paul, rewritten."), - "body not rewritten with trailing-newline instructions: {result:?}" - ); - assert!( - !result.contains("# Team Instructions"), - "Team Instructions must not appear in body: {result}" - ); - } - - // ── write_back_persona_md path-resolution tests ─────────────────────────── - // - // These tests verify that write_back_persona_md resolves the source file via - // the pack manifest rather than by convention. This is the class of bug that - // Thufir's IMPORTANT finding caught: a pack whose manifest points at - // `personas/foo.persona.md` (not `agents/.persona.md`) must still be - // rewritten correctly. - - use tempfile::TempDir; - - /// Build a minimal pack on disk with the given personas layout. - /// - /// `persona_entries` is a list of (manifest_rel_path, file_content) pairs. - /// The manifest lists the relative paths; the files are written verbatim. - fn make_temp_pack(persona_entries: &[(&str, &str)]) -> TempDir { - let dir = TempDir::new().expect("tempdir"); - let root = dir.path(); - - std::fs::create_dir_all(root.join(".plugin")).unwrap(); - let persona_paths: Vec<&str> = persona_entries.iter().map(|(p, _)| *p).collect(); - let manifest = serde_json::json!({ - "id": "test-team", - "name": "Test Team", - "version": "0.1.0", - "personas": persona_paths, - }); - std::fs::write( - root.join(".plugin/plugin.json"), - serde_json::to_string_pretty(&manifest).unwrap(), - ) - .unwrap(); - - for (rel_path, content) in persona_entries { - let abs = root.join(rel_path); - std::fs::create_dir_all(abs.parent().unwrap()).unwrap(); - std::fs::write(&abs, content).unwrap(); - } - - dir - } - - #[test] - fn test_writeback_uses_manifest_path_not_convention() { - // Pack uses `personas/paul.persona.md` layout (not `agents/`). - // Convention-based path would derive `agents/paul.persona.md` (wrong). - // Manifest-based resolution must write to the correct `personas/` path. - let persona_md = "\ ---- -name: paul -display_name: \"Paul\" -description: \"Orchestrator\" -model: goose-claude-4-6-opus ---- -You are Paul. -"; - let dir = make_temp_pack(&[("personas/paul.persona.md", persona_md)]); - let source_dir = dir.path().to_path_buf(); - - let pack = buzz_persona_pkg::pack::load_pack(&source_dir).unwrap(); - assert_eq!(pack.personas[0].name, "paul"); - let source_path = pack.personas[0].source_path.clone(); - // File lives under personas/, not agents/. Assert on path components so - // the check is separator-agnostic (Windows uses `\`, not `/`). - assert_eq!( - source_path.file_name().and_then(|n| n.to_str()), - Some("paul.persona.md"), - "expected paul.persona.md: {}", - source_path.display() - ); - assert_eq!( - source_path - .parent() - .and_then(|p| p.file_name()) - .and_then(|n| n.to_str()), - Some("personas"), - "expected personas/ layout: {}", - source_path.display() - ); - - // Simulate what write_back_persona_md does after the fix: - // use source_path from pack, not source_dir/agents/.persona.md - let mut p = persona( - "Paul Updated", - Some("goose"), - None, - Some("databricks_v2"), - Some("goose-claude-opus-4-8"), - ); - p.source_team_persona_slug = Some("paul".to_string()); - - let content = std::fs::read_to_string(&source_path).unwrap(); - let updated = rewrite_persona_md(&content, &p, "You are Paul.\n", None).unwrap(); - std::fs::write(&source_path, &updated).unwrap(); - - let after = std::fs::read_to_string(&source_path).unwrap(); - assert!( - after.contains("databricks_v2:goose-claude-opus-4-8"), - "model not written: {after}" - ); - assert!( - after.ends_with("You are Paul.\n"), - "body not preserved: {after:?}" - ); - } - - #[test] - fn test_writeback_name_differs_from_filename() { - // Pack file is `personas/orchestrator.persona.md` but `name: paul`. - // Slug matches `name:`, not the filename. - let persona_md = "\ ---- -name: paul -display_name: \"Paul\" -description: \"Orchestrator\" -model: old-model ---- -You are Paul. -"; - let dir = make_temp_pack(&[("personas/orchestrator.persona.md", persona_md)]); - let source_dir = dir.path().to_path_buf(); - - let pack = buzz_persona_pkg::pack::load_pack(&source_dir).unwrap(); - let loaded = pack.personas.iter().find(|p| p.name == "paul").unwrap(); - let source_path = loaded.source_path.clone(); - // File basename is orchestrator, not paul - assert!( - source_path - .to_string_lossy() - .contains("orchestrator.persona.md"), - "expected orchestrator.persona.md: {}", - source_path.display() - ); - - let mut p = persona( - "Paul", - Some("goose"), - None, - Some("anthropic"), - Some("claude-4"), - ); - p.source_team_persona_slug = Some("paul".to_string()); - - let content = std::fs::read_to_string(&source_path).unwrap(); - let updated = rewrite_persona_md(&content, &p, "You are Paul.\n", None).unwrap(); - std::fs::write(&source_path, &updated).unwrap(); - - let after = std::fs::read_to_string(&source_path).unwrap(); - assert!( - after.contains("anthropic:claude-4"), - "model not written to orchestrator.persona.md: {after}" - ); - // The wrong file (paul.persona.md in agents/) must NOT exist. - assert!( - !dir.path().join("agents/paul.persona.md").exists(), - "convention-based path must not be created" - ); - } - - // ── find_team_for_persona_source tests ──────────────────────────────────── - // - // Verify that write_back_persona_md finds teams by team_persona_key, not - // team.id. Legacy/backfilled teams have a UUID `id` while PersonaRecord - // stores the manifest directory name in `source_team`; matching by `id` - // alone silently misses those teams. - - fn make_team(id: &str, source_dir: Option<&str>) -> TeamRecord { - TeamRecord { - id: id.to_string(), - name: id.to_string(), - description: None, - persona_ids: vec![], - is_builtin: false, - source_dir: source_dir.map(std::path::PathBuf::from), - is_symlink: false, - symlink_target: None, - version: None, - created_at: "2026-01-01T00:00:00Z".to_string(), - updated_at: "2026-01-01T00:00:00Z".to_string(), - } - } - - #[test] - fn test_find_team_legacy_uuid_id_matched_by_source_dir_name() { - // Legacy shape: team.id is a UUID; PersonaRecord.source_team is the - // manifest directory name. The old `team.id == source_team` predicate - // missed this — find_team_for_persona_source must match via source_dir. - let teams = vec![make_team("some-uuid-123", Some("/teams/com.test.pack"))]; - let found = find_team_for_persona_source(&teams, "com.test.pack"); - assert!( - found.is_some(), - "legacy team must be found by manifest dir name, not UUID" - ); - assert_eq!(found.unwrap().id, "some-uuid-123"); - } - - #[test] - fn test_find_team_modern_id_matched_directly() { - // Modern shape: team.id equals the manifest directory name. Must match. - let teams = vec![make_team("com.test.pack", Some("/teams/com.test.pack"))]; - let found = find_team_for_persona_source(&teams, "com.test.pack"); - assert!(found.is_some(), "modern team must be found"); - } - - #[test] - fn test_find_team_manifest_dir_name_matches_regardless_of_uuid_id() { - // Regression: the old predicate `team.id == source_team` missed legacy - // teams when source_team holds the manifest dir name, not the UUID. - // This test confirms that searching by manifest dir name always works - // even when team.id is a UUID. - let teams = vec![make_team("some-uuid-123", Some("/teams/com.test.pack"))]; - // source_team holds the manifest dir name "com.test.pack", not the UUID. - let by_dir = find_team_for_persona_source(&teams, "com.test.pack"); - assert!( - by_dir.is_some(), - "manifest dir name must find the legacy team" - ); - assert_eq!(by_dir.unwrap().id, "some-uuid-123"); - } - - #[test] - fn test_find_team_no_source_dir_falls_back_to_id() { - // JSON-only team: no source_dir, team_persona_key falls back to id. - let teams = vec![make_team("builtin-team:fizz", None)]; - let found = find_team_for_persona_source(&teams, "builtin-team:fizz"); - assert!( - found.is_some(), - "no source_dir: must match via team.id fallback" - ); - } - - #[test] - fn test_find_team_returns_none_when_no_match() { - let teams = vec![make_team("some-uuid-123", Some("/teams/com.test.pack"))]; - let not_found = find_team_for_persona_source(&teams, "com.other.pack"); - assert!(not_found.is_none(), "unrelated source_team must not match"); - } - - #[test] - fn test_multi_save_round_trip_short_circuit_holds_both_times() { - // Regression: the `==` short-circuit must hold on the second save too. - // - // Without the frontend fix (removing systemPrompt.trim()), the first - // save stores the trimmed composed prompt; on re-open the form emits the - // trimmed value, which != compose_prompt(body, instructions\n) — the `==` - // check fails, suffix-strip fails on the trimmed string, and the safety - // guard fires on every subsequent save. This test verifies the path stays - // clean across two consecutive no-edit saves. - let instructions = "Follow the rules.\n"; // realistic: trailing newline from file - // `split_frontmatter` strips the "\n" after the closing "---" delimiter - // but preserves the rest of the body verbatim. PROMPT_MD's body line ends - // with "\n", so loaded.prompt at runtime = "You are Paul.\n". - let (_, raw_body) = buzz_persona_pkg::persona::split_frontmatter(PROMPT_MD).unwrap(); - // compose_prompt equivalent (must match the real impl exactly) - let composed = format!("{raw_body}\n\n---\n# Team Instructions\n{instructions}"); - - // ── Save 1: user opens dialog, saves without editing ───────────────── - let mut p = persona("Paul", None, None, None, Some("goose-claude-4-6-opus")); - p.system_prompt = composed.clone(); // no frontend trim — system_prompt is the full composed value - - let written_1 = rewrite_persona_md(PROMPT_MD, &p, raw_body, Some(instructions)).unwrap(); - // Short-circuit must have fired: body is preserved (still "You are Paul.\n"). - assert!( - written_1.ends_with("You are Paul.\n"), - "save 1: body must be preserved by == short-circuit: {written_1:?}" - ); - assert!( - !written_1.contains("# Team Instructions"), - "save 1: Team Instructions must not appear in file body: {written_1}" - ); - - // ── Save 2: user re-opens (reads written_1), saves again without editing - // `loaded.prompt` comes from split_frontmatter on the written file — same - // raw_body as before (the write-back preserved it byte-for-byte). - let (_, body_from_file) = buzz_persona_pkg::persona::split_frontmatter(&written_1).unwrap(); - assert_eq!( - body_from_file, raw_body, - "split_frontmatter after save 1 must return the original raw_body" - ); - - // Compose again from the read-back body — must equal the stored system_prompt. - let recomposed = format!("{body_from_file}\n\n---\n# Team Instructions\n{instructions}"); - assert_eq!( - recomposed, composed, - "save 2 precondition: recomposed must equal stored system_prompt so == holds" - ); - - // Now simulate save 2: system_prompt is the recomposed value (no trim). - p.system_prompt = recomposed; - let written_2 = - rewrite_persona_md(&written_1, &p, body_from_file, Some(instructions)).unwrap(); - // Short-circuit must fire again: body still preserved, no corruption. - assert!( - written_2.ends_with("You are Paul.\n"), - "save 2: body must still be preserved by == short-circuit: {written_2:?}" - ); - assert!( - !written_2.contains("# Team Instructions"), - "save 2: Team Instructions must not appear after second save: {written_2}" - ); - // Both writes are byte-identical — no spurious mutations. - assert_eq!( - written_1, written_2, - "save 2: file content must be identical to save 1" - ); - } -} +mod tests; diff --git a/desktop/src-tauri/src/commands/personas/writeback/tests.rs b/desktop/src-tauri/src/commands/personas/writeback/tests.rs new file mode 100644 index 0000000000..4beb58e0c4 --- /dev/null +++ b/desktop/src-tauri/src/commands/personas/writeback/tests.rs @@ -0,0 +1,847 @@ +use super::*; +use std::collections::BTreeMap; + +/// Build a minimal PersonaRecord with the fields that `rewrite_persona_md` reads. +fn persona( + display_name: &str, + runtime: Option<&str>, + avatar_url: Option<&str>, + provider: Option<&str>, + model: Option<&str>, +) -> PersonaRecord { + // system_prompt matches the SAMPLE_MD body so the "no prompt edit" path + // is taken in rewrite_persona_md (body preserved byte-for-byte). + persona_with_prompt( + display_name, + runtime, + avatar_url, + provider, + model, + "You are Paul.\n", + ) +} + +/// Like `persona` but with an explicit system_prompt value. +fn persona_with_prompt( + display_name: &str, + runtime: Option<&str>, + avatar_url: Option<&str>, + provider: Option<&str>, + model: Option<&str>, + system_prompt: &str, +) -> PersonaRecord { + PersonaRecord { + id: "test-id".to_string(), + display_name: display_name.to_string(), + avatar_url: avatar_url.map(str::to_string), + system_prompt: system_prompt.to_string(), + runtime: runtime.map(str::to_string), + model: model.map(str::to_string), + provider: provider.map(str::to_string), + name_pool: vec![], + is_builtin: false, + is_active: true, + source_team: Some("team-1".to_string()), + source_team_persona_slug: Some("paul".to_string()), + env_vars: BTreeMap::new(), + respond_to: None, + respond_to_allowlist: Vec::new(), + mcp_toolsets: None, + parallelism: None, + created_at: "2025-01-01T00:00:00Z".to_string(), + updated_at: "2025-01-01T00:00:00Z".to_string(), + } +} + +const SAMPLE_MD: &str = "\ +--- +name: paul +display_name: \"Paul\" +description: \"An orchestrator.\" +model: goose-claude-4-6-opus +runtime: goose +extra_key: keep-me +--- +You are Paul. +"; + +// ── rewrite_persona_md unit tests ───────────────────────────────────────── + +#[test] +fn test_rewrite_model_provider_joined_and_body_preserved() { + let p = persona( + "Paul", + Some("goose"), + None, + Some("databricks_v2"), + Some("goose-claude-opus-4-8"), + ); + let result = rewrite_persona_md(SAMPLE_MD, &p, "You are Paul.\n", None).unwrap(); + + // Body is byte-preserved. + assert!( + result.ends_with("\nYou are Paul.\n"), + "body not preserved: {result:?}" + ); + + // model key is the joined form. + assert!( + result.contains("model: databricks_v2:goose-claude-opus-4-8"), + "joined model missing: {result}" + ); + + // No separate provider key. + assert!( + !result.contains("provider:"), + "separate provider key must not be emitted: {result}" + ); + + // Unrelated key preserved. + assert!( + result.contains("extra_key: keep-me"), + "extra key lost: {result}" + ); + + // Still valid frontmatter (parses cleanly). + assert!(result.starts_with("---\n"), "must start with ---"); +} + +#[test] +fn test_rewrite_bare_model_when_provider_none() { + let p = persona("Paul", Some("goose"), None, None, Some("bare-model-id")); + let result = rewrite_persona_md(SAMPLE_MD, &p, "You are Paul.\n", None).unwrap(); + + assert!( + result.contains("model: bare-model-id"), + "bare model missing: {result}" + ); + assert!( + !result.contains("provider:"), + "provider key must not be emitted: {result}" + ); +} + +#[test] +fn test_rewrite_runtime_removed_when_none() { + let p = persona("Paul", None, None, None, Some("some-model")); + let result = rewrite_persona_md(SAMPLE_MD, &p, "You are Paul.\n", None).unwrap(); + + // runtime was in source but persona.runtime is None — key must be removed. + assert!( + !result.contains("runtime:"), + "runtime key should be removed when None: {result}" + ); +} + +#[test] +fn test_rewrite_preserves_description_and_name_and_extra_keys() { + let p = persona( + "Paul Updated", + Some("goose"), + None, + Some("anthropic"), + Some("claude-opus-4"), + ); + let result = rewrite_persona_md(SAMPLE_MD, &p, "You are Paul.\n", None).unwrap(); + + // name and description are not persona record fields — must survive untouched. + assert!(result.contains("name: paul"), "name key lost: {result}"); + assert!( + result.contains("description:"), + "description key lost: {result}" + ); + assert!( + result.contains("extra_key: keep-me"), + "extra_key lost: {result}" + ); + + // display_name updated. + assert!( + result.contains("display_name: Paul Updated") + || result.contains("display_name: \"Paul Updated\""), + "display_name not updated: {result}" + ); +} + +#[test] +fn test_rewrite_no_provider_no_model_removes_model_key() { + let p = persona("Paul", None, None, None, None); + let result = rewrite_persona_md(SAMPLE_MD, &p, "You are Paul.\n", None).unwrap(); + + // When both provider and model are cleared, the model key is removed. + assert!( + !result.contains("model:"), + "model key should be removed when both absent: {result}" + ); +} + +#[test] +fn test_rewrite_avatar_set_and_cleared() { + let with_avatar = persona( + "Paul", + Some("goose"), + Some("data:image/png;base64,abc"), + Some("openai"), + Some("gpt-4o"), + ); + let result = rewrite_persona_md(SAMPLE_MD, &with_avatar, "You are Paul.\n", None).unwrap(); + assert!( + result.contains("avatar:"), + "avatar key should be set: {result}" + ); + + let without_avatar = persona("Paul", Some("goose"), None, Some("openai"), Some("gpt-4o")); + let result = rewrite_persona_md(SAMPLE_MD, &without_avatar, "You are Paul.\n", None).unwrap(); + assert!( + !result.contains("avatar:"), + "avatar key should be absent when None: {result}" + ); +} + +#[test] +fn test_rewrite_body_not_replaced_by_system_prompt() { + // system_prompt on the PersonaRecord is the COMPOSED prompt (body + pack instructions). + // The body of the .persona.md must not be replaced with it. + let p = persona( + "Paul", + Some("goose"), + None, + Some("databricks_v2"), + Some("goose-claude-opus-4-8"), + ); + let result = rewrite_persona_md(SAMPLE_MD, &p, "You are Paul.\n", None).unwrap(); + + // The raw body from the file ("You are Paul.") is preserved. + assert!( + result.ends_with("You are Paul.\n"), + "raw body must be preserved, not replaced by composed system_prompt: {result:?}" + ); + // The composed instructions suffix must NOT appear in the file body. + assert!( + !result.contains("# Team Instructions"), + "composed system_prompt must not be written to body: {result}" + ); +} + +// ── body (system_prompt) write-back tests ───────────────────────────────── +// +// These tests exercise the compose_prompt inversion logic in rewrite_persona_md. + +/// Frontmatter-only MD (no body to preserve) for prompt tests. +const PROMPT_MD: &str = "\ +--- +name: paul +display_name: \"Paul\" +model: goose-claude-4-6-opus +--- +You are Paul. +"; + +#[test] +fn test_prompt_edited_with_pack_instructions_body_rewritten() { + // User edits the prompt. system_prompt = new_raw_body + separator + instructions. + // Body in file should be updated to new_raw_body; Team Instructions must NOT appear. + let instructions = "Follow the rules."; + let new_raw_body = "You are Paul, a wise orchestrator."; + let composed = format!("{new_raw_body}\n\n---\n# Team Instructions\n{instructions}"); + + let mut p = persona("Paul", None, None, None, Some("goose-claude-4-6-opus")); + p.system_prompt = composed; + + let result = rewrite_persona_md(PROMPT_MD, &p, "You are Paul.", Some(instructions)).unwrap(); + assert!( + result.ends_with("You are Paul, a wise orchestrator."), + "new body not written: {result:?}" + ); + assert!( + !result.contains("# Team Instructions"), + "Team Instructions must not appear in body: {result}" + ); +} + +#[test] +fn test_prompt_edited_no_pack_instructions_body_rewritten_verbatim() { + // No pack instructions: composed == raw. New body is system_prompt verbatim. + let new_raw_body = "You are Paul, updated."; + let mut p = persona("Paul", None, None, None, Some("goose-claude-4-6-opus")); + p.system_prompt = new_raw_body.to_string(); + + let result = rewrite_persona_md(PROMPT_MD, &p, "You are Paul.", None).unwrap(); + assert!( + result.ends_with("You are Paul, updated."), + "body not updated: {result:?}" + ); +} + +#[test] +fn test_prompt_unedited_body_preserved() { + // system_prompt equals compose_prompt(current_raw_body, instructions) exactly. + // The body section must not change even though frontmatter may be rewritten. + let instructions = "Follow the rules."; + let raw_body = "You are Paul."; + let composed = format!("{raw_body}\n\n---\n# Team Instructions\n{instructions}"); + + let mut p = persona("Paul", None, None, None, Some("goose-claude-4-6-opus")); + // Frontmatter also unchanged (matches PROMPT_MD). + p.system_prompt = composed; + + let result = rewrite_persona_md(PROMPT_MD, &p, raw_body, Some(instructions)).unwrap(); + // Body must remain "You are Paul." (no prompt edit — body section preserved). + assert!( + result.ends_with("You are Paul.\n"), + "body must be unchanged: {result:?}" + ); + // Team Instructions must not leak into the body. + assert!( + !result.contains("# Team Instructions"), + "Team Instructions must not appear: {result}" + ); +} + +#[test] +fn test_prompt_safety_guard_missing_suffix_preserves_body() { + // pack_instructions is non-empty but system_prompt does NOT end with the + // expected suffix (user edited inside the Team Instructions block, or the + // instructions drifted). The existing body must be preserved — no corruption. + let instructions = "Follow the rules."; + let mut p = persona("Paul", None, None, None, Some("goose-claude-4-6-opus")); + // system_prompt lacks the expected suffix entirely. + p.system_prompt = "Some rogue prompt with # Team Instructions in the middle".to_string(); + + let result = rewrite_persona_md(PROMPT_MD, &p, "You are Paul.", Some(instructions)).unwrap(); + // Body preserved from the file. + assert!( + result.ends_with("You are Paul.\n"), + "body must be preserved by safety guard: {result:?}" + ); +} + +#[test] +fn test_prompt_round_trip_no_double_append() { + // After write-back, running compose_prompt on the written body + instructions + // must reproduce the stored system_prompt exactly. This proves no double-append. + let instructions = "Follow the rules."; + let new_raw_body = "You are Paul, updated for the round-trip."; + let composed = format!("{new_raw_body}\n\n---\n# Team Instructions\n{instructions}"); + + let mut p = persona("Paul", None, None, None, Some("goose-claude-4-6-opus")); + p.system_prompt = composed.clone(); + + let result = rewrite_persona_md(PROMPT_MD, &p, "You are Paul.", Some(instructions)).unwrap(); + + // Extract the written body (everything after the last "---\n"). + let written_body = result.split("---\n").last().unwrap(); + // Re-compose: body + instructions must equal the original composed prompt. + let recomposed = format!("{written_body}\n\n---\n# Team Instructions\n{instructions}"); + assert_eq!( + recomposed, composed, + "round-trip failed — double-append would occur: recomposed={recomposed:?}" + ); +} + +#[test] +fn test_prompt_edited_with_trailing_newline_in_instructions() { + // Regression: normal instructions.md files have a trailing newline. + // compose_prompt includes it verbatim; system_prompt must NOT be trimmed + // in update_persona or the suffix-strip fails and the safety guard fires. + // + // This test verifies that pack_instructions with a trailing "\n" still + // decomposes correctly — i.e., the body is rewritten, not preserved. + let instructions = "Follow the rules.\n"; // trailing newline from file read + let new_raw_body = "You are Paul, rewritten."; + let composed = format!("{new_raw_body}\n\n---\n# Team Instructions\n{instructions}"); + // system_prompt is NOT trimmed (update_persona must preserve it as-is). + + let mut p = persona("Paul", None, None, None, Some("goose-claude-4-6-opus")); + p.system_prompt = composed; + + let result = rewrite_persona_md(PROMPT_MD, &p, "You are Paul.", Some(instructions)).unwrap(); + assert!( + result.contains("You are Paul, rewritten."), + "body not rewritten with trailing-newline instructions: {result:?}" + ); + assert!( + !result.contains("# Team Instructions"), + "Team Instructions must not appear in body: {result}" + ); +} + +// ── write_back_persona_md path-resolution tests ─────────────────────────── +// +// These tests verify that write_back_persona_md resolves the source file via +// the pack manifest rather than by convention. This is the class of bug that +// Thufir's IMPORTANT finding caught: a pack whose manifest points at +// `personas/foo.persona.md` (not `agents/.persona.md`) must still be +// rewritten correctly. + +use tempfile::TempDir; + +/// Build a minimal pack on disk with the given personas layout. +/// +/// `persona_entries` is a list of (manifest_rel_path, file_content) pairs. +/// The manifest lists the relative paths; the files are written verbatim. +fn make_temp_pack(persona_entries: &[(&str, &str)]) -> TempDir { + let dir = TempDir::new().expect("tempdir"); + let root = dir.path(); + + std::fs::create_dir_all(root.join(".plugin")).unwrap(); + let persona_paths: Vec<&str> = persona_entries.iter().map(|(p, _)| *p).collect(); + let manifest = serde_json::json!({ + "id": "test-team", + "name": "Test Team", + "version": "0.1.0", + "personas": persona_paths, + }); + std::fs::write( + root.join(".plugin/plugin.json"), + serde_json::to_string_pretty(&manifest).unwrap(), + ) + .unwrap(); + + for (rel_path, content) in persona_entries { + let abs = root.join(rel_path); + std::fs::create_dir_all(abs.parent().unwrap()).unwrap(); + std::fs::write(&abs, content).unwrap(); + } + + dir +} + +#[test] +fn test_writeback_uses_manifest_path_not_convention() { + // Pack uses `personas/paul.persona.md` layout (not `agents/`). + // Convention-based path would derive `agents/paul.persona.md` (wrong). + // Manifest-based resolution must write to the correct `personas/` path. + let persona_md = "\ +--- +name: paul +display_name: \"Paul\" +description: \"Orchestrator\" +model: goose-claude-4-6-opus +--- +You are Paul. +"; + let dir = make_temp_pack(&[("personas/paul.persona.md", persona_md)]); + let source_dir = dir.path().to_path_buf(); + + let pack = buzz_persona_pkg::pack::load_pack(&source_dir).unwrap(); + assert_eq!(pack.personas[0].name, "paul"); + let source_path = pack.personas[0].source_path.clone(); + // File lives under personas/, not agents/. Assert on path components so + // the check is separator-agnostic (Windows uses `\`, not `/`). + assert_eq!( + source_path.file_name().and_then(|n| n.to_str()), + Some("paul.persona.md"), + "expected paul.persona.md: {}", + source_path.display() + ); + assert_eq!( + source_path + .parent() + .and_then(|p| p.file_name()) + .and_then(|n| n.to_str()), + Some("personas"), + "expected personas/ layout: {}", + source_path.display() + ); + + // Simulate what write_back_persona_md does after the fix: + // use source_path from pack, not source_dir/agents/.persona.md + let mut p = persona( + "Paul Updated", + Some("goose"), + None, + Some("databricks_v2"), + Some("goose-claude-opus-4-8"), + ); + p.source_team_persona_slug = Some("paul".to_string()); + + let content = std::fs::read_to_string(&source_path).unwrap(); + let updated = rewrite_persona_md(&content, &p, "You are Paul.\n", None).unwrap(); + std::fs::write(&source_path, &updated).unwrap(); + + let after = std::fs::read_to_string(&source_path).unwrap(); + assert!( + after.contains("databricks_v2:goose-claude-opus-4-8"), + "model not written: {after}" + ); + assert!( + after.ends_with("You are Paul.\n"), + "body not preserved: {after:?}" + ); +} + +#[test] +fn test_writeback_name_differs_from_filename() { + // Pack file is `personas/orchestrator.persona.md` but `name: paul`. + // Slug matches `name:`, not the filename. + let persona_md = "\ +--- +name: paul +display_name: \"Paul\" +description: \"Orchestrator\" +model: old-model +--- +You are Paul. +"; + let dir = make_temp_pack(&[("personas/orchestrator.persona.md", persona_md)]); + let source_dir = dir.path().to_path_buf(); + + let pack = buzz_persona_pkg::pack::load_pack(&source_dir).unwrap(); + let loaded = pack.personas.iter().find(|p| p.name == "paul").unwrap(); + let source_path = loaded.source_path.clone(); + // File basename is orchestrator, not paul + assert!( + source_path + .to_string_lossy() + .contains("orchestrator.persona.md"), + "expected orchestrator.persona.md: {}", + source_path.display() + ); + + let mut p = persona( + "Paul", + Some("goose"), + None, + Some("anthropic"), + Some("claude-4"), + ); + p.source_team_persona_slug = Some("paul".to_string()); + + let content = std::fs::read_to_string(&source_path).unwrap(); + let updated = rewrite_persona_md(&content, &p, "You are Paul.\n", None).unwrap(); + std::fs::write(&source_path, &updated).unwrap(); + + let after = std::fs::read_to_string(&source_path).unwrap(); + assert!( + after.contains("anthropic:claude-4"), + "model not written to orchestrator.persona.md: {after}" + ); + // The wrong file (paul.persona.md in agents/) must NOT exist. + assert!( + !dir.path().join("agents/paul.persona.md").exists(), + "convention-based path must not be created" + ); +} + +// ── find_team_for_persona_source tests ──────────────────────────────────── +// +// Verify that write_back_persona_md finds teams by team_persona_key, not +// team.id. Legacy/backfilled teams have a UUID `id` while PersonaRecord +// stores the manifest directory name in `source_team`; matching by `id` +// alone silently misses those teams. + +fn make_team(id: &str, source_dir: Option<&str>) -> TeamRecord { + TeamRecord { + id: id.to_string(), + name: id.to_string(), + description: None, + persona_ids: vec![], + is_builtin: false, + source_dir: source_dir.map(std::path::PathBuf::from), + is_symlink: false, + symlink_target: None, + version: None, + created_at: "2026-01-01T00:00:00Z".to_string(), + updated_at: "2026-01-01T00:00:00Z".to_string(), + } +} + +fn make_team_with_path(id: &str, source_dir: &std::path::Path) -> TeamRecord { + TeamRecord { + id: id.to_string(), + name: id.to_string(), + description: None, + persona_ids: vec![], + is_builtin: false, + source_dir: Some(source_dir.to_path_buf()), + is_symlink: false, + symlink_target: None, + version: None, + created_at: "2026-01-01T00:00:00Z".to_string(), + updated_at: "2026-01-01T00:00:00Z".to_string(), + } +} + +#[test] +fn test_find_team_legacy_uuid_id_matched_by_source_dir_name() { + // Legacy shape: team.id is a UUID; PersonaRecord.source_team is the + // manifest directory name. The old `team.id == source_team` predicate + // missed this — find_team_for_persona_source must match via source_dir. + let teams = vec![make_team("some-uuid-123", Some("/teams/com.test.pack"))]; + let found = find_team_for_persona_source(&teams, "com.test.pack"); + assert!( + found.is_some(), + "legacy team must be found by manifest dir name, not UUID" + ); + assert_eq!(found.unwrap().id, "some-uuid-123"); +} + +#[test] +fn test_find_team_modern_id_matched_directly() { + // Modern shape: team.id equals the manifest directory name. Must match. + let teams = vec![make_team("com.test.pack", Some("/teams/com.test.pack"))]; + let found = find_team_for_persona_source(&teams, "com.test.pack"); + assert!(found.is_some(), "modern team must be found"); +} + +#[test] +fn test_find_team_manifest_dir_name_matches_regardless_of_uuid_id() { + // Regression: the old predicate `team.id == source_team` missed legacy + // teams when source_team holds the manifest dir name, not the UUID. + // This test confirms that searching by manifest dir name always works + // even when team.id is a UUID. + let teams = vec![make_team("some-uuid-123", Some("/teams/com.test.pack"))]; + // source_team holds the manifest dir name "com.test.pack", not the UUID. + let by_dir = find_team_for_persona_source(&teams, "com.test.pack"); + assert!( + by_dir.is_some(), + "manifest dir name must find the legacy team" + ); + assert_eq!(by_dir.unwrap().id, "some-uuid-123"); +} + +#[test] +fn test_find_team_no_source_dir_falls_back_to_id() { + // JSON-only team: no source_dir, team_persona_key falls back to id. + let teams = vec![make_team("builtin-team:fizz", None)]; + let found = find_team_for_persona_source(&teams, "builtin-team:fizz"); + assert!( + found.is_some(), + "no source_dir: must match via team.id fallback" + ); +} + +#[test] +fn test_find_team_returns_none_when_no_match() { + let teams = vec![make_team("some-uuid-123", Some("/teams/com.test.pack"))]; + let not_found = find_team_for_persona_source(&teams, "com.other.pack"); + assert!(not_found.is_none(), "unrelated source_team must not match"); +} + +// ── legacy symlink shape + missing-runtime insert ───────────────────────── +// +// Regression: write-back for a team whose `id` is a UUID (legacy backfill) +// and whose `source_dir` is a path whose FINAL component is the +// team_persona_key (e.g. `.../agents/teams/com.wpfleger.sietch-tabr`), with +// that path being a SYMLINK to the real pack directory. The persona's +// `source_team` stores the key, not the UUID, and the pack file has no +// `runtime:` frontmatter key (write-back must INSERT it). + +/// Build a PersonaRecord matching the legacy incident shape. +fn legacy_persona(source_team: &str, runtime: Option<&str>) -> PersonaRecord { + PersonaRecord { + id: "ab5c038c-1b12-46e2-8283-d6f7c0606fce".to_string(), + display_name: "Paul Updated".to_string(), + avatar_url: None, + system_prompt: "You are Paul.\n".to_string(), + runtime: runtime.map(str::to_string), + model: Some("claude-opus-4-8".to_string()), + provider: Some("anthropic".to_string()), + name_pool: vec![], + is_builtin: false, + is_active: true, + source_team: Some(source_team.to_string()), + source_team_persona_slug: Some("paul".to_string()), + env_vars: BTreeMap::new(), + respond_to: None, + respond_to_allowlist: Vec::new(), + mcp_toolsets: None, + parallelism: None, + created_at: "2026-01-01T00:00:00Z".to_string(), + updated_at: "2026-01-01T00:00:00Z".to_string(), + } +} + +#[test] +fn test_writeback_legacy_symlink_uuid_team_inserts_missing_runtime() { + // Real-world shape: pack lives in a real directory (pack_dir), and the + // team's source_dir is a SYMLINK at `/com.wpfleger.sietch-tabr` + // pointing to pack_dir. The team record has a UUID id; persona.source_team + // is the link basename (the team_persona_key), not the UUID. + let pack_dir = TempDir::new().expect("pack_dir tempdir"); + let link_parent = TempDir::new().expect("link_parent tempdir"); + + // paul.persona.md has NO runtime: key (write-back must INSERT it). + let persona_md = "\ +--- +name: paul +display_name: \"Paul\" +description: \"Orchestrator\" +--- +You are Paul. +"; + // Build a minimal pack in pack_dir. + std::fs::create_dir_all(pack_dir.path().join(".plugin")).unwrap(); + let manifest = serde_json::json!({ + "id": "com.wpfleger.sietch-tabr", + "name": "Sietch Tabr", + "version": "0.1.0", + "personas": ["paul.persona.md"], + }); + std::fs::write( + pack_dir.path().join(".plugin/plugin.json"), + serde_json::to_string_pretty(&manifest).unwrap(), + ) + .unwrap(); + std::fs::write(pack_dir.path().join("paul.persona.md"), persona_md).unwrap(); + + // Create the symlink: `/com.wpfleger.sietch-tabr` → pack_dir + let symlink_path = link_parent.path().join("com.wpfleger.sietch-tabr"); + #[cfg(unix)] + std::os::unix::fs::symlink(pack_dir.path(), &symlink_path).expect("symlink"); + #[cfg(not(unix))] + { + // On non-Unix, skip symlink traversal test (macOS-only scenario). + return; + } + + // Team: UUID id, source_dir = symlink path (basename = team_persona_key). + let team = TeamRecord { + id: "ab5c038c-1b12-46e2-8283-d6f7c0606fce".to_string(), + name: "Sietch Tabr".to_string(), + description: None, + persona_ids: vec![], + is_builtin: false, + source_dir: Some(symlink_path.clone()), + is_symlink: true, + symlink_target: Some(pack_dir.path().to_string_lossy().to_string()), + version: None, + created_at: "2026-01-01T00:00:00Z".to_string(), + updated_at: "2026-01-01T00:00:00Z".to_string(), + }; + let teams = vec![team]; + + // persona.source_team = the basename = team_persona_key + let persona = legacy_persona("com.wpfleger.sietch-tabr", Some("goose")); + + let result = try_write_back_persona_md(&teams, &persona); + assert!( + result.is_ok(), + "write-back through symlink must succeed: {result:?}" + ); + + // Verify runtime was inserted into the pack file. + let written = std::fs::read_to_string(pack_dir.path().join("paul.persona.md")).unwrap(); + assert!( + written.contains("runtime: goose"), + "runtime must be inserted into the file: {written}" + ); + assert!( + written.contains("display_name:"), + "display_name must be present: {written}" + ); +} + +#[cfg(unix)] +#[test] +fn test_writeback_readonly_pack_file_returns_warning() { + use std::os::unix::fs::PermissionsExt; + + // Pack with a persona that has a different display_name than what we'll + // request — this forces a write (not a no-op short-circuit). + let persona_md = "\ +--- +name: paul +display_name: \"Paul Old\" +description: \"Orchestrator\" +runtime: goose +--- +You are Paul. +"; + let dir = make_temp_pack(&[("paul.persona.md", persona_md)]); + let source_dir = dir.path().to_path_buf(); + + // Make the file read-only. + let paul_path = dir.path().join("paul.persona.md"); + let mut perms = std::fs::metadata(&paul_path).unwrap().permissions(); + perms.set_mode(0o444); + std::fs::set_permissions(&paul_path, perms).unwrap(); + + let team = make_team_with_path("test-team", &source_dir); + let teams = vec![team]; + + // Persona with updated display_name — write is needed but will be denied. + let mut persona = persona("Paul New", Some("goose"), None, None, Some("some-model")); + persona.source_team = Some("test-team".to_string()); + persona.source_team_persona_slug = Some("paul".to_string()); + + let result = try_write_back_persona_md(&teams, &persona); + assert!( + result.is_err(), + "read-only pack file must return an error: {result:?}" + ); + let warning = result.unwrap_err(); + assert!( + warning.contains("write"), + "error must mention the write failure: {warning}" + ); +} + +#[test] +fn test_multi_save_round_trip_short_circuit_holds_both_times() { + // Regression: the `==` short-circuit must hold on the second save too. + // + // Without the frontend fix (removing systemPrompt.trim()), the first + // save stores the trimmed composed prompt; on re-open the form emits the + // trimmed value, which != compose_prompt(body, instructions\n) — the `==` + // check fails, suffix-strip fails on the trimmed string, and the safety + // guard fires on every subsequent save. This test verifies the path stays + // clean across two consecutive no-edit saves. + let instructions = "Follow the rules.\n"; // realistic: trailing newline from file + // `split_frontmatter` strips the "\n" after the closing "---" delimiter + // but preserves the rest of the body verbatim. PROMPT_MD's body line ends + // with "\n", so loaded.prompt at runtime = "You are Paul.\n". + let (_, raw_body) = buzz_persona_pkg::persona::split_frontmatter(PROMPT_MD).unwrap(); + // compose_prompt equivalent (must match the real impl exactly) + let composed = format!("{raw_body}\n\n---\n# Team Instructions\n{instructions}"); + + // ── Save 1: user opens dialog, saves without editing ───────────────── + let mut p = persona("Paul", None, None, None, Some("goose-claude-4-6-opus")); + p.system_prompt = composed.clone(); // no frontend trim — system_prompt is the full composed value + + let written_1 = rewrite_persona_md(PROMPT_MD, &p, raw_body, Some(instructions)).unwrap(); + // Short-circuit must have fired: body is preserved (still "You are Paul.\n"). + assert!( + written_1.ends_with("You are Paul.\n"), + "save 1: body must be preserved by == short-circuit: {written_1:?}" + ); + assert!( + !written_1.contains("# Team Instructions"), + "save 1: Team Instructions must not appear in file body: {written_1}" + ); + + // ── Save 2: user re-opens (reads written_1), saves again without editing + // `loaded.prompt` comes from split_frontmatter on the written file — same + // raw_body as before (the write-back preserved it byte-for-byte). + let (_, body_from_file) = buzz_persona_pkg::persona::split_frontmatter(&written_1).unwrap(); + assert_eq!( + body_from_file, raw_body, + "split_frontmatter after save 1 must return the original raw_body" + ); + + // Compose again from the read-back body — must equal the stored system_prompt. + let recomposed = format!("{body_from_file}\n\n---\n# Team Instructions\n{instructions}"); + assert_eq!( + recomposed, composed, + "save 2 precondition: recomposed must equal stored system_prompt so == holds" + ); + + // Now simulate save 2: system_prompt is the recomposed value (no trim). + p.system_prompt = recomposed; + let written_2 = rewrite_persona_md(&written_1, &p, body_from_file, Some(instructions)).unwrap(); + // Short-circuit must fire again: body still preserved, no corruption. + assert!( + written_2.ends_with("You are Paul.\n"), + "save 2: body must still be preserved by == short-circuit: {written_2:?}" + ); + assert!( + !written_2.contains("# Team Instructions"), + "save 2: Team Instructions must not appear after second save: {written_2}" + ); + // Both writes are byte-identical — no spurious mutations. + assert_eq!( + written_1, written_2, + "save 2: file content must be identical to save 1" + ); +} diff --git a/desktop/src-tauri/src/managed_agents/discovery.rs b/desktop/src-tauri/src/managed_agents/discovery.rs index 059ab988f1..cedd723f7e 100644 --- a/desktop/src-tauri/src/managed_agents/discovery.rs +++ b/desktop/src-tauri/src/managed_agents/discovery.rs @@ -348,125 +348,8 @@ pub fn effective_agent_command( .unwrap_or_else(default_agent_command) } -/// Decide whether a user-picked harness command is an explicit per-instance -/// pin or merely the persona's own runtime restated. Returns the override to -/// persist: `Some(picked)` when it diverges from the persona, `None` when it -/// inherits. -/// -/// Comparison is by RUNTIME IDENTITY, not raw string: a persona on the `claude` -/// runtime resolves to `claude-agent-acp`, but a client with only the -/// `claude-code-acp` adapter installed sends that command instead. Both map to -/// the same `claude` runtime, so neither is a real divergence — string equality -/// would wrongly bake a pin. An unknown/custom command (no matching runtime) -/// only inherits when it exactly equals the persona command. -pub fn divergent_agent_command_override( - persona_id: Option<&str>, - personas: &[crate::managed_agents::types::PersonaRecord], - picked_command: Option<&str>, -) -> Option { - let picked = picked_command - .map(str::trim) - .filter(|value| !value.is_empty())?; - let persona_command = effective_agent_command(persona_id, personas, None); - let same_runtime = match ( - known_acp_runtime(picked), - known_acp_runtime(&persona_command), - ) { - (Some(a), Some(b)) => std::ptr::eq(a, b), - _ => picked == persona_command, - }; - if same_runtime { - None - } else { - Some(picked.to_string()) - } -} - -/// Decide the `agent_command_override` to persist at AGENT UPDATE time. -/// -/// The edit dialog sends `agent_command` as a tri-state string: the empty -/// "inherit from persona" sentinel (clear the pin), or a concrete command -/// (pin). Resolution: -/// -/// - EMPTY / whitespace → the inherit sentinel: always `None` regardless of -/// `harness_override`, so toggling "Inherit runtime from persona" clears the -/// pin. -/// - DELIBERATE OVERRIDE (`harness_override` true, persona linked): the user -/// explicitly picked a runtime/Custom command in the dialog. This is a real -/// pin and is preserved VERBATIM — even when the picked command maps to, or -/// is byte-identical to, the persona's own runtime command. Selecting "Custom -/// command" and saving e.g. `goose` for a goose persona is a deliberate act -/// to freeze the harness against future persona runtime edits; dropping it -/// back to inherit (as [`divergent_agent_command_override`] would) defeats -/// that intent. Unlike the create-time path, there is no byte-identical -/// exception here: at create the command is machine-derived from the persona, -/// so equality means "no user divergence"; at update an equal command reached -/// the force branch only because the user picked Custom, which IS the -/// divergence. -/// - NO OVERRIDE INTENT (`harness_override` false) or NO PERSONA: defer to -/// [`divergent_agent_command_override`], which keeps the persona authoritative -/// and treats a same-runtime restatement as inherit. -pub fn update_time_agent_command_override( - persona_id: Option<&str>, - personas: &[crate::managed_agents::types::PersonaRecord], - picked_command: Option<&str>, - harness_override: bool, -) -> Option { - let picked = picked_command - .map(str::trim) - .filter(|value| !value.is_empty())?; - - if persona_id.is_some() && harness_override { - return Some(picked.to_string()); - } - - divergent_agent_command_override(persona_id, personas, Some(picked)) -} - -/// Decide the `agent_command_override` to persist at AGENT CREATE time. -/// -/// A persona-backed create receives its harness command from -/// `resolvePersonaRuntime` (frontend), which produces a divergent command in two -/// distinct cases that the backend MUST tell apart: -/// -/// - DELIBERATE OVERRIDE (`harness_override` true): the user explicitly picked a -/// runtime command in UI that exposes a runtime selector. This is a real pin -/// and is preserved when it differs from the command inheritance would spawn, -/// including installed aliases such as `claude-code-acp`. -/// - MISSING-RUNTIME FALLBACK (`harness_override` false): the persona's runtime -/// isn't installed locally, so `resolvePersonaRuntime` substitutes a fallback -/// default. This is NOT a pin — baking it would freeze the agent on the fallback -/// harness even after the persona's runtime is installed and the persona is -/// re-edited, the exact bug this resolver chain exists to prevent. Stores `None` -/// so the persona stays authoritative. -/// -/// `isOverridden` from `resolvePersonaRuntime` cannot distinguish these — it is -/// `true` for BOTH — so the caller must thread the explicit user-intent bit. -/// -/// Persona-less creates (`persona_id` is `None`, e.g. the standalone -/// CreateAgentDialog) have no persona to inherit, so the picked command is always a -/// real pin and is preserved via `divergent_agent_command_override` regardless of -/// `harness_override`. -pub fn create_time_agent_command_override( - persona_id: Option<&str>, - personas: &[crate::managed_agents::types::PersonaRecord], - picked_command: Option<&str>, - harness_override: bool, -) -> Option { - if persona_id.is_some() && !harness_override { - return None; - } - - if persona_id.is_some() && harness_override { - let picked = picked_command - .map(str::trim) - .filter(|value| !value.is_empty())?; - let inherited_command = effective_agent_command(persona_id, personas, None); - return (picked != inherited_command).then(|| picked.to_string()); - } - - divergent_agent_command_override(persona_id, personas, picked_command) -} +mod overrides; +pub use overrides::{apply_agent_command_update, create_time_agent_command_override}; fn default_agent_args(command: &str) -> Option> { match normalize_command_identity(command).as_str() { diff --git a/desktop/src-tauri/src/managed_agents/discovery/overrides.rs b/desktop/src-tauri/src/managed_agents/discovery/overrides.rs new file mode 100644 index 0000000000..1d285743c0 --- /dev/null +++ b/desktop/src-tauri/src/managed_agents/discovery/overrides.rs @@ -0,0 +1,153 @@ +//! The `agent_command_override` decision family: divergence comparison, +//! create-time and update-time resolution, and the record apply. Child module +//! of `discovery` (split under the desktop file-size cap) so the override +//! decisions stay next to the resolution ladder they feed. + +use super::{effective_agent_command, known_acp_runtime}; + +/// Decide whether a user-picked harness command is an explicit per-instance +/// pin or merely the persona's own runtime restated. Returns the override to +/// persist: `Some(picked)` when it diverges from the persona, `None` when it +/// inherits. +/// +/// Comparison is by RUNTIME IDENTITY, not raw string: a persona on the `claude` +/// runtime resolves to `claude-agent-acp`, but a client with only the +/// `claude-code-acp` adapter installed sends that command instead. Both map to +/// the same `claude` runtime, so neither is a real divergence — string equality +/// would wrongly bake a pin. An unknown/custom command (no matching runtime) +/// only inherits when it exactly equals the persona command. +pub fn divergent_agent_command_override( + persona_id: Option<&str>, + personas: &[crate::managed_agents::types::PersonaRecord], + picked_command: Option<&str>, +) -> Option { + let picked = picked_command + .map(str::trim) + .filter(|value| !value.is_empty())?; + let persona_command = effective_agent_command(persona_id, personas, None); + let same_runtime = match ( + known_acp_runtime(picked), + known_acp_runtime(&persona_command), + ) { + (Some(a), Some(b)) => std::ptr::eq(a, b), + _ => picked == persona_command, + }; + if same_runtime { + None + } else { + Some(picked.to_string()) + } +} + +/// Decide the `agent_command_override` to persist at AGENT UPDATE time. +/// +/// The edit dialog sends `agent_command` as a tri-state string: the empty +/// "inherit from persona" sentinel (clear the pin), or a concrete command +/// (pin). Resolution: +/// +/// - EMPTY / whitespace → the inherit sentinel: always `None` regardless of +/// `harness_override`, so toggling "Inherit runtime from persona" clears the +/// pin. +/// - DELIBERATE OVERRIDE (`harness_override` true, persona linked): the user +/// explicitly picked a runtime/Custom command in the dialog. This is a real +/// pin and is preserved VERBATIM — even when the picked command maps to, or +/// is byte-identical to, the persona's own runtime command. Selecting "Custom +/// command" and saving e.g. `goose` for a goose persona is a deliberate act +/// to freeze the harness against future persona runtime edits; dropping it +/// back to inherit (as [`divergent_agent_command_override`] would) defeats +/// that intent. Unlike the create-time path, there is no byte-identical +/// exception here: at create the command is machine-derived from the persona, +/// so equality means "no user divergence"; at update an equal command reached +/// the force branch only because the user picked Custom, which IS the +/// divergence. +/// - NO OVERRIDE INTENT (`harness_override` false) or NO PERSONA: defer to +/// [`divergent_agent_command_override`], which keeps the persona authoritative +/// and treats a same-runtime restatement as inherit. +pub fn update_time_agent_command_override( + persona_id: Option<&str>, + personas: &[crate::managed_agents::types::PersonaRecord], + picked_command: Option<&str>, + harness_override: bool, +) -> Option { + let picked = picked_command + .map(str::trim) + .filter(|value| !value.is_empty())?; + + if persona_id.is_some() && harness_override { + return Some(picked.to_string()); + } + + divergent_agent_command_override(persona_id, personas, Some(picked)) +} + +/// Apply an explicit `agent_command` edit to `record`: persist the override +/// pin decided by [`update_time_agent_command_override`], and on the inherit +/// sentinel (empty/whitespace command) also clear the materialized +/// `record.runtime` so the resolution ladder falls through to the live +/// definition immediately instead of silently keeping the stale instance copy. +/// +/// The runtime clear is guarded on a live persona link: for a definition-less +/// record the materialized runtime is the only harness source left after the +/// override clear, so a stray empty `agent_command` from a non-dialog caller +/// must not change what the agent runs. +pub fn apply_agent_command_update( + record: &mut crate::managed_agents::types::ManagedAgentRecord, + personas: &[crate::managed_agents::types::PersonaRecord], + agent_command: &str, + harness_override: bool, +) { + record.agent_command_override = update_time_agent_command_override( + record.persona_id.as_deref(), + personas, + Some(agent_command), + harness_override, + ); + if agent_command.trim().is_empty() && record.persona_id.is_some() { + record.runtime = None; + } +} + +/// Decide the `agent_command_override` to persist at AGENT CREATE time. +/// +/// A persona-backed create receives its harness command from +/// `resolvePersonaRuntime` (frontend), which produces a divergent command in two +/// distinct cases that the backend MUST tell apart: +/// +/// - DELIBERATE OVERRIDE (`harness_override` true): the user explicitly picked a +/// runtime command in UI that exposes a runtime selector. This is a real pin +/// and is preserved when it differs from the command inheritance would spawn, +/// including installed aliases such as `claude-code-acp`. +/// - MISSING-RUNTIME FALLBACK (`harness_override` false): the persona's runtime +/// isn't installed locally, so `resolvePersonaRuntime` substitutes a fallback +/// default. This is NOT a pin — baking it would freeze the agent on the fallback +/// harness even after the persona's runtime is installed and the persona is +/// re-edited, the exact bug this resolver chain exists to prevent. Stores `None` +/// so the persona stays authoritative. +/// +/// `isOverridden` from `resolvePersonaRuntime` cannot distinguish these — it is +/// `true` for BOTH — so the caller must thread the explicit user-intent bit. +/// +/// Persona-less creates (`persona_id` is `None`, e.g. the standalone +/// CreateAgentDialog) have no persona to inherit, so the picked command is always a +/// real pin and is preserved via `divergent_agent_command_override` regardless of +/// `harness_override`. +pub fn create_time_agent_command_override( + persona_id: Option<&str>, + personas: &[crate::managed_agents::types::PersonaRecord], + picked_command: Option<&str>, + harness_override: bool, +) -> Option { + if persona_id.is_some() && !harness_override { + return None; + } + + if persona_id.is_some() && harness_override { + let picked = picked_command + .map(str::trim) + .filter(|value| !value.is_empty())?; + let inherited_command = effective_agent_command(persona_id, personas, None); + return (picked != inherited_command).then(|| picked.to_string()); + } + + divergent_agent_command_override(persona_id, personas, picked_command) +} diff --git a/desktop/src-tauri/src/managed_agents/discovery/tests.rs b/desktop/src-tauri/src/managed_agents/discovery/tests.rs index 260bc9e3b8..10413397e2 100644 --- a/desktop/src-tauri/src/managed_agents/discovery/tests.rs +++ b/desktop/src-tauri/src/managed_agents/discovery/tests.rs @@ -1,10 +1,10 @@ use std::path::PathBuf; +use super::overrides::{divergent_agent_command_override, update_time_agent_command_override}; use super::{ - classify_runtime, create_time_agent_command_override, default_agent_command, - divergent_agent_command_override, effective_agent_command, find_via_login_shell, - managed_agent_avatar_url, normalize_agent_args, record_agent_command, - update_time_agent_command_override, BUZZ_AGENT_AVATAR_URL, CLAUDE_CODE_AVATAR_URL, + apply_agent_command_update, classify_runtime, create_time_agent_command_override, + default_agent_command, effective_agent_command, find_via_login_shell, managed_agent_avatar_url, + normalize_agent_args, record_agent_command, BUZZ_AGENT_AVATAR_URL, CLAUDE_CODE_AVATAR_URL, CODEX_AVATAR_URL, GOOSE_AVATAR_URL, }; use crate::managed_agents::AcpAvailabilityStatus; @@ -562,3 +562,46 @@ fn update_time_override_preserves_pin_for_persona_less_agent() { Some("codex-acp".to_string()) ); } + +#[test] +fn apply_agent_command_update_inherit_sentinel_clears_pin_and_runtime() { + // Choosing Inherit on a persona-linked record clears BOTH the explicit + // pin and the materialized runtime, so resolution falls through to the + // live definition immediately — not on the next spawn. + let personas = vec![persona_with_runtime("p1", Some("goose"))]; + let mut record = record_with(Some("claude"), Some("p1"), Some("codex-acp")); + + apply_agent_command_update(&mut record, &personas, "", false); + + assert_eq!(record.agent_command_override, None); + assert_eq!(record.runtime, None); + assert_eq!(record_agent_command(&record, &personas), "goose"); +} + +#[test] +fn apply_agent_command_update_sentinel_keeps_runtime_for_definition_less_record() { + // For a record with no persona link the materialized runtime is the only + // harness source left once the pin is cleared — a stray empty + // agent_command must not change what the agent runs. + let mut record = record_with(Some("claude"), None, Some("codex-acp")); + + apply_agent_command_update(&mut record, &[], "", false); + + assert_eq!(record.agent_command_override, None); + assert_eq!(record.runtime.as_deref(), Some("claude")); + assert_eq!(record_agent_command(&record, &[]), "claude-agent-acp"); +} + +#[test] +fn apply_agent_command_update_concrete_pin_keeps_materialized_runtime() { + // A concrete pick only sets the pin; the materialized runtime is left for + // the next snapshot apply. The pin shadows it in resolution either way. + let personas = vec![persona_with_runtime("p1", Some("goose"))]; + let mut record = record_with(Some("claude"), Some("p1"), None); + + apply_agent_command_update(&mut record, &personas, "codex-acp", true); + + assert_eq!(record.agent_command_override.as_deref(), Some("codex-acp")); + assert_eq!(record.runtime.as_deref(), Some("claude")); + assert_eq!(record_agent_command(&record, &personas), "codex-acp"); +} diff --git a/desktop/src-tauri/src/managed_agents/persona_events.rs b/desktop/src-tauri/src/managed_agents/persona_events.rs index faa9507989..7a03d9ce9e 100644 --- a/desktop/src-tauri/src/managed_agents/persona_events.rs +++ b/desktop/src-tauri/src/managed_agents/persona_events.rs @@ -9,7 +9,7 @@ use buzz_core_pkg::kind::KIND_PERSONA; use nostr::{EventBuilder, Kind, Tag}; use serde::{Deserialize, Serialize}; -use super::PersonaRecord; +use super::{ManagedAgentRecord, PersonaRecord}; use crate::app_state::AppState; /// The JSON body stored in a persona event's content field. @@ -334,6 +334,12 @@ pub struct PersonaSnapshot { pub system_prompt: Option, pub model: Option, pub provider: Option, + /// Preferred ACP runtime ID, copied verbatim from the persona (including + /// `None`). Unlike `model`/`provider`, there is no record-fallback: the + /// materialized instance `runtime` must mirror the definition so that + /// definition edits propagate on the next spawn rather than being silently + /// shadowed by the stale materialized value. + pub runtime: Option, /// `persona_content_hash` of the persona at snapshot time; the drift basis. pub source_version: String, } @@ -367,6 +373,7 @@ pub fn persona_snapshot(persona: &PersonaRecord) -> PersonaSnapshot { system_prompt: Some(persona.system_prompt.clone()), model: persona.model.clone(), provider: persona.provider.clone(), + runtime: persona.runtime.clone(), source_version: persona_content_hash(&persona_event_content(persona)), } } @@ -410,5 +417,43 @@ pub fn persona_snapshot_with_agent_config_fallback( ..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. +/// +/// This is the single apply used by every snapshot-apply site: the spawn +/// re-pin (`start_local_agent_with_preflight`), the launch backfill and +/// restore re-snapshot (`restore.rs`), and the prospective re-snapshot inside +/// `spawn_config_hash` — so a future `PersonaSnapshot` field addition +/// propagates to all of them at once. +/// +/// Deliberately does NOT touch `updated_at`: persistence stamps are the +/// 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: &PersonaRecord) { + let snapshot = persona_snapshot_with_agent_config_fallback( + persona, + record.model.as_deref(), // fallback: record.model + record.provider.as_deref(), // fallback: record.provider + ); + if let Some(prompt) = snapshot.system_prompt { + record.system_prompt = Some(prompt); + } + record.model = snapshot.model; + record.provider = snapshot.provider; + record.runtime = snapshot.runtime; + // env_vars stay overrides-only. Self-heal records written before the env + // refresh: persona env used to be baked into `record.env_vars`, turning + // inherited values into pseudo-overrides that shadow later persona edits. + // An override equal to the persona's current value is indistinguishable + // from inheritance, so drop it and let the live merge supply it. + record + .env_vars + .retain(|k, v| persona.env_vars.get(k) != Some(v)); + record.persona_source_version = Some(snapshot.source_version); +} #[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 87b37c3fb1..d6e46edcc3 100644 --- a/desktop/src-tauri/src/managed_agents/persona_events/tests.rs +++ b/desktop/src-tauri/src/managed_agents/persona_events/tests.rs @@ -484,6 +484,33 @@ fn field_fallback_record_blank_is_none() { ); } +// ── PersonaSnapshot.runtime ─────────────────────────────────────────────── + +/// (b) The snapshot carries the persona's runtime VERBATIM — including None, +/// which clears a stale materialized value on the instance record. Unlike +/// model/provider, runtime does not fall back to the record's own value: +/// instances have no user-owned runtime, so the definition must stay +/// authoritative. +#[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")); + assert_eq!( + snap.runtime.as_deref(), + Some("goose"), + "persona runtime Some must be copied verbatim into snapshot" + ); + + 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")); + assert_eq!( + snap.runtime, None, + "persona runtime None must produce None snapshot (clears stale materialized value)" + ); +} + // ── persona_snapshot_with_agent_config_fallback ──────────────────────────── /// Helper: a persona with no model/provider configured. diff --git a/desktop/src-tauri/src/managed_agents/restore.rs b/desktop/src-tauri/src/managed_agents/restore.rs index 1abefe8dac..efd721123c 100644 --- a/desktop/src-tauri/src/managed_agents/restore.rs +++ b/desktop/src-tauri/src/managed_agents/restore.rs @@ -61,23 +61,8 @@ pub fn backfill_persona_snapshots(app: &tauri::AppHandle) -> Result<(), String> // Layer precedence at read time: persona env < agent env. When the // persona leaves model/provider blank, the record's own configured // values are preserved — a blank persona must not clobber a - // user-configured agent. - let snapshot = super::persona_events::persona_snapshot_with_agent_config_fallback( - persona, - record.model.as_deref(), // fallback: record.model - record.provider.as_deref(), // fallback: record.provider - ); - if let Some(prompt) = snapshot.system_prompt { - record.system_prompt = Some(prompt); - } - record.model = snapshot.model; - record.provider = snapshot.provider; - // env_vars stay overrides-only; see the create-path comment. Self-heal - // pre-refresh records that baked persona env in as pseudo-overrides. - record - .env_vars - .retain(|k, v| persona.env_vars.get(k) != Some(v)); - record.persona_source_version = Some(snapshot.source_version); + // user-configured agent. See `apply_persona_snapshot`. + super::persona_events::apply_persona_snapshot(record, persona); record.updated_at = util::now_iso(); changed = true; } @@ -195,23 +180,7 @@ pub async fn restore_managed_agents_on_launch( let Some(persona) = personas_for_snapshot.iter().find(|p| p.id == persona_id) else { continue; }; - let snapshot = super::persona_events::persona_snapshot_with_agent_config_fallback( - persona, - record.model.as_deref(), // fallback: record.model - record.provider.as_deref(), // fallback: record.provider - ); - if let Some(prompt) = snapshot.system_prompt { - record.system_prompt = Some(prompt); - } - record.model = snapshot.model; - record.provider = snapshot.provider; - // env_vars stay overrides-only; see the create-path comment. - // Self-heal pre-refresh records that baked persona env in as - // pseudo-overrides. - record - .env_vars - .retain(|k, v| persona.env_vars.get(k) != Some(v)); - record.persona_source_version = Some(snapshot.source_version); + super::persona_events::apply_persona_snapshot(record, persona); record.updated_at = util::now_iso(); changed = true; } diff --git a/desktop/src-tauri/src/managed_agents/spawn_hash.rs b/desktop/src-tauri/src/managed_agents/spawn_hash.rs index e369fd443e..9b38615268 100644 --- a/desktop/src-tauri/src/managed_agents/spawn_hash.rs +++ b/desktop/src-tauri/src/managed_agents/spawn_hash.rs @@ -28,7 +28,7 @@ use std::hash::{DefaultHasher, Hash, Hasher}; use super::{ known_acp_runtime, normalize_agent_args, - persona_events::persona_snapshot_with_agent_config_fallback, + persona_events::apply_persona_snapshot, resolve_effective_agent_env, types::{ManagedAgentRecord, PersonaRecord}, }; @@ -60,29 +60,17 @@ pub(crate) fn spawn_config_hash( personas: &[PersonaRecord], workspace_relay: &str, ) -> u64 { - // Prospective re-snapshot: mirror the mutation start/restore apply to the - // record right before spawning, so the hash covers what a restart would - // actually run. Idempotent, so the spawn-time stamp (post-snapshot record) - // and later recomputes (persisted record) agree when nothing changed. + // Prospective re-snapshot: apply the same `apply_persona_snapshot` the + // start/restore paths run right before spawning, so the hash covers what a + // restart would actually run. Idempotent, so the spawn-time stamp + // (post-snapshot record) and later recomputes (persisted record) agree + // 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) { - let snapshot = persona_snapshot_with_agent_config_fallback( - persona, - record.model.as_deref(), - record.provider.as_deref(), - ); - if let Some(prompt) = snapshot.system_prompt { - record.system_prompt = Some(prompt); - } - record.model = snapshot.model; - record.provider = snapshot.provider; - // Mirror the start/restore self-heal: overrides equal to the live - // persona value are treated as inherited. The persona env itself - // reaches the hash through `resolve_effective_agent_env` below. - record - .env_vars - .retain(|k, v| persona.env_vars.get(k) != Some(v)); + apply_persona_snapshot(&mut record, persona); } } let record = &record; 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 a524457f9e..5e09aaff1a 100644 --- a/desktop/src-tauri/src/managed_agents/spawn_hash/tests.rs +++ b/desktop/src-tauri/src/managed_agents/spawn_hash/tests.rs @@ -376,6 +376,62 @@ fn team_pack_records_keep_empty_vs_absent_prompt_distinction() { ); } +/// (a) A definition-runtime edit must change spawn_config_hash for a +/// materialized, override-free record — the prospective re-snapshot now +/// copies the persona's runtime onto the record before hashing. +#[test] +fn definition_runtime_edit_changes_hash_for_materialized_record() { + let mut rec = record(); + rec.persona_id = Some("pers".into()); + rec.runtime = Some("goose".into()); // materialized runtime on instance + + let before = [persona("pers", Some("goose"), "prompt")]; + let after = [persona("pers", Some("claude"), "prompt")]; + assert_ne!( + spawn_config_hash(&rec, &before, "wss://ws.example"), + spawn_config_hash(&rec, &after, "wss://ws.example"), + "definition runtime edit must badge a materialized, override-free instance" + ); +} + +/// (c) An explicit agent_command_override (ladder step 1) must beat a +/// changed definition runtime — the badge must NOT fire for a pinned instance. +#[test] +fn agent_command_override_beats_definition_runtime_change() { + let mut rec = record(); + rec.persona_id = Some("pers".into()); + rec.runtime = Some("goose".into()); // materialized runtime + rec.agent_command_override = Some("goose".into()); // explicit per-instance pin + + let before = [persona("pers", Some("goose"), "prompt")]; + let after = [persona("pers", Some("claude"), "prompt")]; + assert_eq!( + spawn_config_hash(&rec, &before, "wss://ws.example"), + spawn_config_hash(&rec, &after, "wss://ws.example"), + "explicit override must win regardless of definition runtime change" + ); +} + +/// (d) When the linked definition is absent the prospective re-snapshot is +/// skipped entirely: the materialized runtime must still affect the hash. +#[test] +fn missing_definition_leaves_materialized_runtime_in_hash() { + let mut rec = record(); + rec.persona_id = Some("missing".into()); + rec.runtime = Some("goose".into()); // materialized runtime + + let no_personas: &[PersonaRecord] = &[]; + + let mut no_runtime = rec.clone(); + no_runtime.runtime = None; + + assert_ne!( + spawn_config_hash(&rec, no_personas, "wss://ws.example"), + spawn_config_hash(&no_runtime, no_personas, "wss://ws.example"), + "materialized runtime must still affect hash when definition is absent" + ); +} + #[test] fn effective_spawn_prompt_matches_hash_semantics() { // The env write and the hash share effective_spawn_prompt — this row diff --git a/desktop/src-tauri/src/managed_agents/types.rs b/desktop/src-tauri/src/managed_agents/types.rs index 28a734e6e3..d9944b92ae 100644 --- a/desktop/src-tauri/src/managed_agents/types.rs +++ b/desktop/src-tauri/src/managed_agents/types.rs @@ -236,9 +236,19 @@ pub struct ManagedAgentRecord { #[serde(default)] pub agent_command_override: Option, pub agent_args: Vec, + /// Create-time snapshot of the catalog MCP command. Never read at spawn — + /// the effective MCP command is always re-derived from the runtime catalog + /// (`known_acp_runtime`) — and no longer written by updates. Kept for + /// serde compatibility with existing stores. pub mcp_command: String, + /// Deprecated: `BUZZ_ACP_TURN_TIMEOUT` is ignored by the harness and the + /// desktop no longer emits or edits it. Kept for serde compatibility with + /// existing stores; use `idle_timeout_seconds` or + /// `max_turn_duration_seconds` for turn-length control. pub turn_timeout_seconds: u64, - /// Idle timeout in seconds. If set, overrides turn_timeout_seconds. + /// Idle timeout in seconds (`BUZZ_ACP_IDLE_TIMEOUT`): how long the agent + /// may stay silent on its ACP channel mid-turn before the harness times + /// the turn out. #[serde(default)] pub idle_timeout_seconds: Option, /// Absolute wall-clock cap per turn. @@ -334,7 +344,16 @@ pub struct ManagedAgentRecord { /// Absorbed from `PersonaRecord.runtime` — the preferred ACP runtime ID /// (e.g. 'goose', 'claude'). Record-first command resolution reads this /// before falling back to legacy persona lookup; populated by the store - /// migration and at create time. + /// migration and at create time, and re-mirrored from the linked + /// definition at every snapshot apply (`apply_persona_snapshot`). + /// + /// `None` means "inherit from the linked definition" (the Inherit sentinel + /// clears it). Serialization then omits the key, so boot-time + /// `materialize_agent_runtimes` re-inserts a mirror of the definition's + /// current runtime on the next launch — behaviorally identical, because + /// every apply site re-mirrors the live definition anyway. A literal + /// `"runtime": null` in the store (key present, e.g. hand-edited) is + /// honored: materialization skips it and it deserializes to `None`. #[serde(default, skip_serializing_if = "Option::is_none")] pub runtime: Option, /// Pool of short thematic names for clones of this agent. Absorbed from @@ -439,7 +458,11 @@ pub struct ManagedAgentSummary { /// concrete pin (`agent_command` above is the resolved/effective command). pub agent_command_override: Option, pub agent_args: Vec, + /// Catalog-derived from the effective harness (not the record's stored + /// field), so the UI always shows what a spawn would actually use. pub mcp_command: String, + /// Deprecated passthrough of the stored record value; the harness ignores + /// it. Kept for wire compatibility. pub turn_timeout_seconds: u64, pub idle_timeout_seconds: Option, pub max_turn_duration_seconds: Option, @@ -488,56 +511,6 @@ pub struct ManagedAgentSummary { pub respond_to_allowlist: Vec, } -#[derive(Debug, Deserialize)] -#[serde(rename_all = "camelCase")] -pub struct CreateManagedAgentRequest { - pub name: String, - #[serde(default)] - pub persona_id: Option, - pub relay_url: Option, - pub acp_command: Option, - pub agent_command: Option, - /// True when `agent_command` is a runtime command the user deliberately - /// picked for a linked persona. Distinguishes a real selection, including an - /// installed alias, from a missing-runtime fallback so a persona-backed - /// create only stores an `agent_command_override` for the former. - #[serde(default)] - pub harness_override: bool, - #[serde(default)] - pub agent_args: Vec, - pub mcp_command: Option, - pub turn_timeout_seconds: Option, - pub idle_timeout_seconds: Option, - pub max_turn_duration_seconds: Option, - pub parallelism: Option, - pub system_prompt: Option, - pub avatar_url: Option, - pub model: Option, - pub provider: Option, - pub mcp_toolsets: Option, - /// Environment variables for this agent. Layered on top of persona env. - #[serde(default)] - pub env_vars: BTreeMap, - #[serde(default)] - pub spawn_after_create: bool, - #[serde(default = "default_start_on_app_launch")] - pub start_on_app_launch: bool, - #[serde(default)] - pub backend: BackendKind, - /// `None` = caller expressed no preference: the definition's - /// `respond_to` default applies when linked, `RespondTo::default()` - /// otherwise. `Some` is an explicit instance-level choice and always - /// wins over the definition default. - #[serde(default)] - pub respond_to: Option, - /// Raw allowlist as received from the frontend. Validated and normalized - /// before being written to the record. - #[serde(default)] - pub respond_to_allowlist: Vec, - #[serde(default)] - pub relay_mesh: Option, -} - #[derive(Debug, Serialize)] pub struct CreateManagedAgentResponse { pub agent: ManagedAgentSummary, @@ -616,63 +589,6 @@ pub struct ManagedAgentPrereqsInfo { pub mcp: CommandAvailabilityInfo, } -/// Patch request for updating a managed agent's mutable fields. -/// -/// Tri-state nullable semantics via `Option>`: -/// - Field absent in JSON → `None` (don't touch) -/// - `"field": null` → `Some(None)` (clear to default) -/// - `"field": "value"` → `Some(Some("value"))` (set) -#[derive(Debug, Deserialize)] -#[serde(rename_all = "camelCase")] -pub struct UpdateManagedAgentRequest { - pub pubkey: String, - /// Absent = don't touch. Present = rename the agent. - #[serde(default)] - pub name: Option, - /// Absent = don't touch. null = clear to agent default. "id" = set. - #[serde(default)] - pub model: Option>, - #[serde(default)] - pub system_prompt: Option>, - #[serde(default)] - pub mcp_toolsets: Option>, - /// Absent = don't touch. Present = replace the env_vars map entirely. - #[serde(default)] - pub env_vars: Option>, - #[serde(default)] - pub parallelism: Option, - #[serde(default)] - pub turn_timeout_seconds: Option, - #[serde(default)] - pub relay_url: Option, - #[serde(default)] - pub acp_command: Option, - #[serde(default)] - pub agent_command: Option, - /// True when the accompanying `agent_command` is a runtime/Custom command - /// the user deliberately picked for a linked persona (i.e. the dialog is - /// not inheriting). Distinguishes a real pin — including one that maps to - /// the persona's own runtime — from a persona-authoritative restatement, - /// so a same-runtime pick is preserved instead of being dropped back to - /// inherit. Ignored when `agent_command` is absent or the inherit sentinel. - #[serde(default)] - pub harness_override: bool, - #[serde(default)] - pub agent_args: Option>, - #[serde(default)] - pub mcp_command: Option, - /// Absent = don't touch. null = clear to runtime default. "id" = set. - #[serde(default, deserialize_with = "crate::util::double_option")] - pub provider: Option>, - /// Absent = don't touch. Present = set mode. - #[serde(default)] - pub respond_to: Option, - /// Absent = don't touch. Present = replace the allowlist (validated & - /// normalized server-side). - #[serde(default)] - pub respond_to_allowlist: Option>, -} - #[derive(Debug, Serialize)] pub struct UpdateManagedAgentResponse { pub agent: ManagedAgentSummary, diff --git a/desktop/src-tauri/src/managed_agents/types/requests.rs b/desktop/src-tauri/src/managed_agents/types/requests.rs index 9838f55da8..c32b9f816a 100644 --- a/desktop/src-tauri/src/managed_agents/types/requests.rs +++ b/desktop/src-tauri/src/managed_agents/types/requests.rs @@ -1,10 +1,14 @@ -//! Persona command request types, split from `types.rs` (file-size cap). +//! Persona and managed-agent command request types, split from `types.rs` +//! (file-size cap). use std::collections::BTreeMap; use serde::Deserialize; -use super::{validate_respond_to_allowlist, PersonaRecord, RespondTo}; +use super::{ + default_start_on_app_launch, validate_respond_to_allowlist, BackendKind, PersonaRecord, + RelayMeshConfig, RespondTo, +}; /// The NIP-AP behavioral quad as one grouped request field. /// @@ -125,6 +129,134 @@ pub struct UpdatePersonaRequest { pub behavior: Option, } +#[derive(Debug, Deserialize)] +#[serde(rename_all = "camelCase")] +pub struct CreateManagedAgentRequest { + pub name: String, + #[serde(default)] + pub persona_id: Option, + pub relay_url: Option, + pub acp_command: Option, + pub agent_command: Option, + /// True when `agent_command` is a runtime command the user deliberately + /// picked for a linked persona. Distinguishes a real selection, including an + /// installed alias, from a missing-runtime fallback so a persona-backed + /// create only stores an `agent_command_override` for the former. + #[serde(default)] + pub harness_override: bool, + #[serde(default)] + pub agent_args: Vec, + /// Accepted for wire compatibility; not applied to the record. The + /// effective MCP command is always derived from the runtime catalog at + /// spawn time — a per-record override is never read. + /// + /// @deprecated — sending this field has no effect. + #[allow(dead_code)] + pub mcp_command: Option, + /// Accepted for wire compatibility; not applied to the record. + /// `BUZZ_ACP_TURN_TIMEOUT` is deprecated and ignored by the harness. + /// + /// @deprecated — sending this field has no effect. + #[allow(dead_code)] + pub turn_timeout_seconds: Option, + pub idle_timeout_seconds: Option, + pub max_turn_duration_seconds: Option, + pub parallelism: Option, + pub system_prompt: Option, + pub avatar_url: Option, + pub model: Option, + pub provider: Option, + pub mcp_toolsets: Option, + /// Environment variables for this agent. Layered on top of persona env. + #[serde(default)] + pub env_vars: BTreeMap, + #[serde(default)] + pub spawn_after_create: bool, + #[serde(default = "default_start_on_app_launch")] + pub start_on_app_launch: bool, + #[serde(default)] + pub backend: BackendKind, + /// `None` = caller expressed no preference: the definition's + /// `respond_to` default applies when linked, `RespondTo::default()` + /// otherwise. `Some` is an explicit instance-level choice and always + /// wins over the definition default. + #[serde(default)] + pub respond_to: Option, + /// Raw allowlist as received from the frontend. Validated and normalized + /// before being written to the record. + #[serde(default)] + pub respond_to_allowlist: Vec, + #[serde(default)] + pub relay_mesh: Option, +} + +/// Patch request for updating a managed agent's mutable fields. +/// +/// Tri-state nullable semantics via `Option>`: +/// - Field absent in JSON → `None` (don't touch) +/// - `"field": null` → `Some(None)` (clear to default) +/// - `"field": "value"` → `Some(Some("value"))` (set) +#[derive(Debug, Deserialize)] +#[serde(rename_all = "camelCase")] +pub struct UpdateManagedAgentRequest { + pub pubkey: String, + /// Absent = don't touch. Present = rename the agent. + #[serde(default)] + pub name: Option, + /// Absent = don't touch. null = clear to agent default. "id" = set. + #[serde(default)] + pub model: Option>, + #[serde(default)] + pub system_prompt: Option>, + #[serde(default)] + pub mcp_toolsets: Option>, + /// Absent = don't touch. Present = replace the env_vars map entirely. + #[serde(default)] + pub env_vars: Option>, + #[serde(default)] + pub parallelism: Option, + /// Accepted for wire compatibility; not applied to the stored record. + /// `BUZZ_ACP_TURN_TIMEOUT` is deprecated and ignored by the harness. + /// + /// @deprecated — sending this field has no effect. + #[allow(dead_code)] + #[serde(default)] + pub turn_timeout_seconds: Option, + #[serde(default)] + pub relay_url: Option, + #[serde(default)] + pub acp_command: Option, + #[serde(default)] + pub agent_command: Option, + /// True when the accompanying `agent_command` is a runtime/Custom command + /// the user deliberately picked for a linked persona (i.e. the dialog is + /// not inheriting). Distinguishes a real pin — including one that maps to + /// the persona's own runtime — from a persona-authoritative restatement, + /// so a same-runtime pick is preserved instead of being dropped back to + /// inherit. Ignored when `agent_command` is absent or the inherit sentinel. + #[serde(default)] + pub harness_override: bool, + #[serde(default)] + pub agent_args: Option>, + /// Accepted for wire compatibility; not applied to the stored record. + /// The effective MCP command is always catalog-derived at spawn time. + /// + /// @deprecated — sending this field has no effect. + #[allow(dead_code)] + #[serde(default)] + pub mcp_command: Option, + /// Absent = don't touch. null = clear to runtime default. "id" = set. + #[serde(default, deserialize_with = "crate::util::double_option")] + pub provider: Option>, + /// Absent = don't touch. Present = set mode. + #[serde(default)] + pub respond_to: Option, + /// Absent = don't touch. Present = replace the allowlist (validated & + /// normalized server-side). + #[serde(default)] + pub respond_to_allowlist: Option>, +} + #[cfg(test)] mod tests { use super::*; diff --git a/desktop/src/features/agents/ui/AgentInstanceEditDialog.tsx b/desktop/src/features/agents/ui/AgentInstanceEditDialog.tsx index 7ac337badc..38bbb4b0f1 100644 --- a/desktop/src/features/agents/ui/AgentInstanceEditDialog.tsx +++ b/desktop/src/features/agents/ui/AgentInstanceEditDialog.tsx @@ -96,11 +96,7 @@ export function AgentInstanceEditDialog({ agent.personaId != null && agent.agentCommandOverride == null, ); const [agentArgs, setAgentArgs] = React.useState(agent.agentArgs.join(",")); - const [mcpCommand, setMcpCommand] = React.useState(agent.mcpCommand); const [mcpToolsets, setMcpToolsets] = React.useState(agent.mcpToolsets ?? ""); - const [turnTimeoutSeconds, setTurnTimeoutSeconds] = React.useState( - String(agent.turnTimeoutSeconds), - ); const [parallelism, setParallelism] = React.useState( String(agent.parallelism), ); @@ -155,9 +151,7 @@ export function AgentInstanceEditDialog({ agent.personaId != null && agent.agentCommandOverride == null, ); setAgentArgs(agent.agentArgs.join(",")); - setMcpCommand(agent.mcpCommand); setMcpToolsets(agent.mcpToolsets ?? ""); - setTurnTimeoutSeconds(String(agent.turnTimeoutSeconds)); setParallelism(String(agent.parallelism)); setSystemPrompt(agent.systemPrompt ?? ""); setModel(agent.model ?? ""); @@ -458,7 +452,6 @@ export function AgentInstanceEditDialog({ computeEditAgentFormValidity({ name, parallelism, - turnTimeoutSeconds, agentAcpCommand: agent.acpCommand, acpCommand, respondTo, @@ -474,7 +467,6 @@ export function AgentInstanceEditDialog({ async function handleSubmit() { try { const parsedParallelism = Number.parseInt(parallelism, 10); - const parsedTimeout = Number.parseInt(turnTimeoutSeconds, 10); const parsedArgs = agentArgs .split(",") .map((v) => v.trim()) @@ -532,18 +524,10 @@ export function AgentInstanceEditDialog({ parsedArgs.join(",") !== agent.agentArgs.join(",") ? parsedArgs : undefined, - mcpCommand: - mcpCommand.trim() !== agent.mcpCommand - ? mcpCommand.trim() - : undefined, mcpToolsets: (mcpToolsets.trim() || null) !== agent.mcpToolsets ? mcpToolsets.trim() || null : undefined, - turnTimeoutSeconds: - parsedTimeout > 0 && parsedTimeout !== agent.turnTimeoutSeconds - ? parsedTimeout - : undefined, parallelism: parsedParallelism > 0 && parsedParallelism !== agent.parallelism ? parsedParallelism @@ -943,26 +927,22 @@ export function AgentInstanceEditDialog({ inheritedEnvVars={inheritedEnvVars} inheritHarness={inheritHarness} linkedPersona={linkedPersona} - mcpCommand={mcpCommand} mcpToolsets={mcpToolsets} parallelism={parallelism} relayUrl={relayUrl} requiredEnvKeys={requiredEnvKeys} selectedRuntimeId={selectedRuntimeId} systemPrompt={systemPrompt} - turnTimeoutSeconds={turnTimeoutSeconds} onAcpCommandChange={setAcpCommand} onAgentArgsChange={setAgentArgs} onAgentCommandChange={setAgentCommand} onAutoRestartChange={setAutoRestartOnConfigChange} onEnvVarsChange={setEnvVars} onInheritHarnessChange={setInheritHarness} - onMcpCommandChange={setMcpCommand} onMcpToolsetsChange={setMcpToolsets} onParallelismChange={setParallelism} onRelayUrlChange={setRelayUrl} onSystemPromptChange={setSystemPrompt} - onTurnTimeoutChange={setTurnTimeoutSeconds} /> ) : null} diff --git a/desktop/src/features/agents/ui/EditAgentAdvancedFields.tsx b/desktop/src/features/agents/ui/EditAgentAdvancedFields.tsx index d66d10fafb..fd607e5928 100644 --- a/desktop/src/features/agents/ui/EditAgentAdvancedFields.tsx +++ b/desktop/src/features/agents/ui/EditAgentAdvancedFields.tsx @@ -20,26 +20,22 @@ export function EditAgentAdvancedFields({ inheritedEnvVars, inheritHarness, linkedPersona, - mcpCommand, mcpToolsets, parallelism, relayUrl, requiredEnvKeys, selectedRuntimeId, systemPrompt, - turnTimeoutSeconds, onAcpCommandChange, onAgentArgsChange, onAgentCommandChange, onEnvVarsChange, onInheritHarnessChange, - onMcpCommandChange, onMcpToolsetsChange, onParallelismChange, onRelayUrlChange, onAutoRestartChange, onSystemPromptChange, - onTurnTimeoutChange, }: { acpCommand: string; agentArgs: string; @@ -51,26 +47,22 @@ export function EditAgentAdvancedFields({ inheritedEnvVars: Record; inheritHarness: boolean; linkedPersona: AgentPersona | null; - mcpCommand: string; mcpToolsets: string; parallelism: string; relayUrl: string; requiredEnvKeys: readonly string[]; selectedRuntimeId: string; systemPrompt: string; - turnTimeoutSeconds: string; onAcpCommandChange: (value: string) => void; onAgentArgsChange: (value: string) => void; onAgentCommandChange: (value: string) => void; onEnvVarsChange: (value: EnvVarsValue) => void; onInheritHarnessChange: (value: boolean) => void; - onMcpCommandChange: (value: string) => void; onMcpToolsetsChange: (value: string) => void; onParallelismChange: (value: string) => void; onRelayUrlChange: (value: string) => void; onAutoRestartChange: (value: boolean) => void; onSystemPromptChange: (value: string) => void; - onTurnTimeoutChange: (value: string) => void; }) { return (
@@ -181,13 +173,13 @@ export function EditAgentAdvancedFields({
- {/* MCP command */} + {/* MCP toolsets */}
onMcpCommandChange(event.target.value)} - placeholder="Optional MCP server command" - value={mcpCommand} + id="edit-agent-mcp-toolsets" + onChange={(event) => onMcpToolsetsChange(event.target.value)} + placeholder="default,canvas,forums,dms,media" + value={mcpToolsets} />
+

+ Comma-separated list of toolsets to expose via BUZZ_TOOLSETS. +

- {/* MCP toolsets */} + {/* Parallelism */}
onMcpToolsetsChange(event.target.value)} - placeholder="default,canvas,forums,dms,media" - value={mcpToolsets} + id="edit-agent-parallelism" + inputMode="numeric" + onChange={(event) => onParallelismChange(event.target.value)} + placeholder="1" + value={parallelism} />
-

- Comma-separated list of toolsets to expose via BUZZ_TOOLSETS. -

-
- - {/* Turn timeout + Parallelism side by side */} -
-
- -
- onTurnTimeoutChange(event.target.value)} - placeholder="300" - value={turnTimeoutSeconds} - /> -
-
- -
- -
- onParallelismChange(event.target.value)} - placeholder="1" - value={parallelism} - /> -
-
{/* Relay URL */} diff --git a/desktop/src/features/agents/ui/agentInstanceEditPinning.test.mjs b/desktop/src/features/agents/ui/agentInstanceEditPinning.test.mjs index b7f8fe47c3..b809a104cd 100644 --- a/desktop/src/features/agents/ui/agentInstanceEditPinning.test.mjs +++ b/desktop/src/features/agents/ui/agentInstanceEditPinning.test.mjs @@ -165,7 +165,6 @@ test("rehost_gate_blocksSave_whenSubmissionSnapshotLacksCredential", () => { const canSubmit = computeEditAgentFormValidity({ name: pinnedAgent.name, parallelism: "1", - turnTimeoutSeconds: "60", agentAcpCommand: pinnedAgent.acpCommand, acpCommand: pinnedAgent.acpCommand, respondTo: "mentions", @@ -257,7 +256,6 @@ test("editValidity_allowlistWithEmptyList_blocksSave", () => { const base = { name: pinnedAgent.name, parallelism: "1", - turnTimeoutSeconds: "60", agentAcpCommand: pinnedAgent.acpCommand, acpCommand: pinnedAgent.acpCommand, selectedRuntimeId: "claude", diff --git a/desktop/src/features/agents/ui/editAgentProviderDiscovery.test.mjs b/desktop/src/features/agents/ui/editAgentProviderDiscovery.test.mjs index 4a75f2eaa2..16278a49ce 100644 --- a/desktop/src/features/agents/ui/editAgentProviderDiscovery.test.mjs +++ b/desktop/src/features/agents/ui/editAgentProviderDiscovery.test.mjs @@ -604,7 +604,6 @@ test("editAgent_missingRequiredEnvKey_blocksSaveViaValidity", () => { const base = { name: "My Agent", parallelism: "", - turnTimeoutSeconds: "", agentAcpCommand: "", acpCommand: "", respondTo: "all", @@ -634,7 +633,6 @@ test("editAgent_customCommandPinned_blocksSaveWhenCommandEmpty", () => { const base = { name: "My Agent", parallelism: "", - turnTimeoutSeconds: "", agentAcpCommand: "", acpCommand: "", respondTo: "all", diff --git a/desktop/src/features/agents/ui/personaRuntimeModel.ts b/desktop/src/features/agents/ui/personaRuntimeModel.ts index 64e83b43ff..46612aa926 100644 --- a/desktop/src/features/agents/ui/personaRuntimeModel.ts +++ b/desktop/src/features/agents/ui/personaRuntimeModel.ts @@ -210,7 +210,6 @@ export function resolveInheritedRuntimeSubmission(input: { export interface EditAgentFormValidityInput { name: string; parallelism: string; - turnTimeoutSeconds: string; /** The command already persisted on the agent (empty when inheriting). */ agentAcpCommand: string; acpCommand: string; @@ -249,9 +248,6 @@ export function computeEditAgentFormValidity( const parallelismValid = input.parallelism.trim() === "" || !Number.isNaN(Number.parseInt(input.parallelism, 10)); - const timeoutValid = - input.turnTimeoutSeconds.trim() === "" || - !Number.isNaN(Number.parseInt(input.turnTimeoutSeconds, 10)); const acpCommandValid = !( input.agentAcpCommand && input.acpCommand.trim() === "" ); @@ -266,7 +262,6 @@ export function computeEditAgentFormValidity( return ( input.name.trim().length > 0 && parallelismValid && - timeoutValid && acpCommandValid && respondToValid && customCommandValid && diff --git a/desktop/src/shared/api/tauriPersonas.ts b/desktop/src/shared/api/tauriPersonas.ts index c276110577..b7520d7b5d 100644 --- a/desktop/src/shared/api/tauriPersonas.ts +++ b/desktop/src/shared/api/tauriPersonas.ts @@ -71,6 +71,8 @@ export type RawPersona = { parallelism?: number | null; created_at: string; updated_at: string; + /** Non-null when the pack `.persona.md` write-back failed (non-fatal). */ + writeback_warning?: string | null; }; export function fromRawPersona(persona: RawPersona): AgentPersona { @@ -123,26 +125,30 @@ export async function createPersona( export async function updatePersona( input: UpdatePersonaInput, ): Promise { - return fromRawPersona( - await invokeTauri("update_persona", { - input: { - id: input.id, - displayName: input.displayName, - avatarUrl: input.avatarUrl, - systemPrompt: input.systemPrompt, - runtime: input.runtime, - model: input.model, - provider: input.provider, - namePool: input.namePool ?? [], - // Send envVars only when caller explicitly provided it; omitting - // tells the backend "don't touch the stored env vars" so editing - // unrelated fields can't silently wipe saved credentials. - envVars: input.envVars, - // Same absent-vs-present contract as envVars for the behavioral quad. - behavior: input.behavior, - }, - }), - ); + const raw = await invokeTauri("update_persona", { + input: { + id: input.id, + displayName: input.displayName, + avatarUrl: input.avatarUrl, + systemPrompt: input.systemPrompt, + runtime: input.runtime, + model: input.model, + provider: input.provider, + namePool: input.namePool ?? [], + // Send envVars only when caller explicitly provided it; omitting + // tells the backend "don't touch the stored env vars" so editing + // unrelated fields can't silently wipe saved credentials. + envVars: input.envVars, + // Same absent-vs-present contract as envVars for the behavioral quad. + behavior: input.behavior, + }, + }); + if (raw.writeback_warning) { + console.warn( + `[updatePersona] pack write-back failed (edit saved locally): ${raw.writeback_warning}`, + ); + } + return fromRawPersona(raw); } export async function deletePersona(id: string): Promise {