diff --git a/desktop/src-tauri/src/commands/agents_pending.rs b/desktop/src-tauri/src/commands/agents_pending.rs index 0a7f91eb854..05ca7bf0c78 100644 --- a/desktop/src-tauri/src/commands/agents_pending.rs +++ b/desktop/src-tauri/src/commands/agents_pending.rs @@ -22,8 +22,8 @@ use crate::{app_state::AppState, managed_agents::ManagedAgentRecord}; /// only runtime fields produces an identical row and never re-enqueues a /// publish. Best-effort: a failure here is logged and swallowed so a retention /// hiccup never blocks the disk-authoritative write. -pub(crate) fn retain_managed_agent_pending( - app: &AppHandle, +pub(crate) fn retain_managed_agent_pending( + app: &tauri::AppHandle, state: &AppState, record: &ManagedAgentRecord, ) { diff --git a/desktop/src-tauri/src/commands/personas/mod.rs b/desktop/src-tauri/src/commands/personas/mod.rs index ac43a4719ab..300d3180dc3 100644 --- a/desktop/src-tauri/src/commands/personas/mod.rs +++ b/desktop/src-tauri/src/commands/personas/mod.rs @@ -66,6 +66,10 @@ pub(super) use pending::tombstone_persona_pending; mod create; pub use create::create_persona; mod sharing; +#[cfg(test)] +pub(crate) use pending::{prepare_persona_publication_at, PreparedPersonaPublication}; +#[cfg(test)] +pub(crate) use sharing::publish_and_refresh_teams_at; pub use sharing::set_persona_shared; pub use sharing::update_persona_and_publish; mod update; diff --git a/desktop/src-tauri/src/commands/personas/pending.rs b/desktop/src-tauri/src/commands/personas/pending.rs index 3e4fabbcf5b..6bdcc7602ec 100644 --- a/desktop/src-tauri/src/commands/personas/pending.rs +++ b/desktop/src-tauri/src/commands/personas/pending.rs @@ -10,7 +10,7 @@ use crate::managed_agents::{ AgentDefinition, }; -pub(super) struct PreparedPersonaPublication { +pub(crate) struct PreparedPersonaPublication { pub scope: RetentionScope, pub event: nostr::Event, pub retained: RetainedEvent, @@ -64,8 +64,8 @@ pub(in crate::commands) fn retain_persona_pending_at( /// exact share tag. The explicit share toggle passes `Some(shared)`. Returning /// the retained event lets that command immediately await relay acceptance /// without rebuilding or re-signing a different NIP-33 head. -pub(super) fn prepare_persona_publication( - app: &AppHandle, +pub(super) fn prepare_persona_publication( + app: &AppHandle, state: &AppState, persona: &AgentDefinition, shared_override: Option, @@ -104,8 +104,8 @@ fn retained_persona_is_shared(row: Option<&RetainedEvent>) -> bool { /// never present an unshared persona as published. The durable share state /// lives in the retention head, so nothing is lost: the true value reappears /// once the identity is signable again. -pub(super) fn project_active_persona_sharing( - app: &AppHandle, +pub(super) fn project_active_persona_sharing( + app: &tauri::AppHandle, state: &AppState, personas: &mut [AgentDefinition], ) { @@ -156,7 +156,7 @@ fn project_persona_sharing_at( Ok(()) } -pub(super) fn prepare_persona_publication_at( +pub(crate) fn prepare_persona_publication_at( db_path: &std::path::Path, keys: &nostr::Keys, persona: &AgentDefinition, diff --git a/desktop/src-tauri/src/commands/personas/sharing.rs b/desktop/src-tauri/src/commands/personas/sharing.rs index 331ec9d0d70..8146f98580b 100644 --- a/desktop/src-tauri/src/commands/personas/sharing.rs +++ b/desktop/src-tauri/src/commands/personas/sharing.rs @@ -5,12 +5,25 @@ use crate::{ managed_agents::{ load_personas, retention::{mark_synced, open_retention_db}, + storage::managed_agents_base_dir, AgentDefinition, }, }; use super::pending::{prepare_persona_publication, PreparedPersonaPublication}; +/// Test-only observer called immediately after the `managed_agents_store_lock` +/// is acquired in `publish_and_refresh_teams_at`'s refresh section. Tests use +/// this to assert `try_lock()` fails — proving the lock is held during the +/// synchronous refresh. Moving the lock acquisition to AFTER the refresh call +/// (recreating the TOCTOU race) causes `try_lock()` to succeed, turning the +/// probe test RED. +#[cfg(test)] +type RefreshLockObserver = Box; +#[cfg(test)] +pub(crate) static REFRESH_LOCK_OBSERVER: std::sync::Mutex> = + std::sync::Mutex::new(None); + #[derive(Debug, Clone, Copy, PartialEq, Eq, serde::Serialize)] #[serde(rename_all = "kebab-case")] pub enum PersonaSharePublicationStatus { @@ -59,8 +72,13 @@ pub async fn set_persona_shared( .await .map_err(|e| format!("spawn_blocking failed: {e}"))??; + // Save persona id before `prepared` is consumed by publish_prepared_persona. + let persona_id = prepared.persona.id.clone(); + let base_dir = managed_agents_base_dir(&app)?; + let keys = prepared.scope.owner_keys.clone(); + let db_path = prepared.scope.db_path.clone(); let state = app.state::(); - publish_prepared_persona(&state, prepared).await + publish_and_refresh_teams_at(&state, prepared, &base_dir, &keys, &db_path, &persona_id).await } /// Save a persona edit AND publish its catalog head, returning the same @@ -76,6 +94,19 @@ pub async fn set_persona_shared( pub async fn update_persona_and_publish( input: crate::managed_agents::UpdatePersonaRequest, app: AppHandle, +) -> Result { + update_persona_and_publish_inner(input, app).await +} + +/// Generic core of [`update_persona_and_publish`], testable with +/// `tauri::test::MockRuntime` as well as the production `Wry` runtime. +/// +/// Extracted so tests can invoke the real command path — including the +/// `prepare_persona_publication` scope resolver — through a mock `AppHandle` +/// without the `#[tauri::command]` signature binding to `AppHandle`. +pub(crate) async fn update_persona_and_publish_inner( + input: crate::managed_agents::UpdatePersonaRequest, + app: AppHandle, ) -> Result { let (_, prepared) = super::update::update_persona_with(input, app.clone(), |app, state, persona| { @@ -93,6 +124,54 @@ pub async fn update_persona_and_publish( publish_prepared_persona(&state, prepared).await } +/// Publish a prepared persona head and refresh any shared 30178 team heads that +/// include this persona — the combined contract shared by the share toggle and +/// the publish-retry seam. +/// +/// Extracted from [`set_persona_shared`] so this two-step sequence can be +/// tested directly through `publish_and_refresh_teams_at` without a +/// `tauri::AppHandle`. Deleting the [`refresh_for_persona_at`] call from this +/// function must cause the command-path regression to fail. +pub(crate) async fn publish_and_refresh_teams_at( + state: &AppState, + prepared: PreparedPersonaPublication, + base_dir: &std::path::Path, + keys: &nostr::Keys, + db_path: &std::path::Path, + persona_id: &str, +) -> Result { + let result = publish_prepared_persona(state, prepared).await?; + // F2: refresh any shared 30178 heads that include this persona. The refresh + // reads the current team/persona definitions and may retain a new head — it + // must be serialized with team edit/unshare/delete operations that also hold + // `managed_agents_store_lock` and may retain/retract the same head. + // + // Without the lock a concurrent `set_team_shared(false)` can: + // 1. acquire the lock, retain unshared head T+1, release the lock; + // 2. refresh (unlocked) reads old shared head T, rebuilds, retains T+1; + // `retain_event` accepts equal timestamps — the refresh wins, undoing + // the explicit unshare. + // + // Acquire AFTER the network await so the lock is never held across I/O; + // the synchronous refresh completes entirely inside the critical section. + { + let _guard = state + .managed_agents_store_lock + .lock() + .map_err(|e| format!("managed_agents_store_lock poisoned: {e}"))?; + #[cfg(test)] + { + if let Ok(obs) = REFRESH_LOCK_OBSERVER.lock() { + if let Some(ref f) = *obs { + f(state); + } + } + } + let _ = crate::commands::teams::refresh_for_persona_at(base_dir, keys, db_path, persona_id); + } + Ok(result) +} + async fn publish_prepared_persona( state: &AppState, prepared: PreparedPersonaPublication, @@ -139,11 +218,11 @@ mod tests { commands::personas::pending::prepare_persona_publication_at, managed_agents::{ retention::{get_retained_event, open_retention_db, RetentionScope}, - AgentDefinition, + save_managed_agents, save_personas, AgentDefinition, ManagedAgentRecord, + UpdatePersonaRequest, }, }; use std::collections::BTreeMap; - fn persona() -> AgentDefinition { AgentDefinition { description: None, @@ -393,4 +472,521 @@ mod tests { assert!(error.contains("failed to open retention db")); } + + /// Build a headless mock app for tests that need a full `AppHandle`. + /// + /// Shares the same pattern used in `concurrent_edit_tests.rs`. Use + /// `lock_path_mutex()` + `HOME`/`XDG_DATA_HOME` overrides around this in + /// tests that touch file-backed stores. + fn mock_app() -> tauri::App { + let state = build_app_state(); + tauri::test::mock_builder() + .manage(state) + .build(tauri::test::mock_context(tauri::test::noop_assets())) + .expect("mock app builds headless") + } + + /// P2 relay-sync regression through the real `update_persona_and_publish` + /// command path: when the active retention DB path is unwritable, the relay + /// kind:0 profile sync for linked agents must still complete before the + /// error propagates. + /// + /// Drives `update_persona_and_publish_inner` — the generic core of the + /// exported `update_persona_and_publish` command — through a mock AppHandle, + /// exercising the full wiring: `prepare_persona_publication` scope resolver, + /// the strict-preparation `?`-propagation, and the phase-2 relay sync. The + /// failure is induced by replacing the active retention DB path with a + /// directory (EISDIR) after the relay override is set so the scope hash is + /// stable. + /// + /// Mutation acceptance: restoring `retain_result?` inside + /// `Ok((result, retain_result?, …))` at the blocking-phase return causes + /// phase 2 to be skipped → counter receives 0 requests → RED. + #[test] + fn test_update_and_publish_relay_profile_syncs_despite_preparation_failure() { + use crate::managed_agents::{ + load_managed_agents, load_personas, retention::active_retention_scope, + }; + use std::sync::atomic::{AtomicUsize, Ordering}; + use std::sync::Arc; + use tauri::Manager; + + #[derive(serde::Deserialize)] + #[serde(rename_all = "camelCase")] + struct PartialPublishOutcomeContract { + persona_id: String, + before_display_name: String, + after_display_name: String, + command_error_contains: String, + relay_profile_request_count: usize, + retry_publication_status: String, + } + + let contract: PartialPublishOutcomeContract = serde_json::from_str(include_str!( + "../../../../../test-fixtures/update-persona-publish-partial-outcome.json" + )) + .expect("shared partial-publish contract must parse"); + assert_eq!(contract.retry_publication_status, "published"); + + let temp = tempfile::tempdir().unwrap(); + let home = temp.path().join("home_p2_seam"); + std::fs::create_dir_all(&home).unwrap(); + + let old_home = std::env::var_os("HOME"); + let old_xdg = std::env::var_os("XDG_DATA_HOME"); + let _path_guard = crate::managed_agents::lock_path_mutex(); + std::env::set_var("HOME", &home); + std::env::set_var("XDG_DATA_HOME", &home); + + let rt = tokio::runtime::Builder::new_current_thread() + .enable_all() + .build() + .expect("build test runtime"); + + rt.block_on(async { + let app = mock_app(); + + // Spawn a local HTTP server that counts POST /events requests. + // sync_managed_agent_profile posts kind:0 profile events here. + let post_count = Arc::new(AtomicUsize::new(0)); + let post_count_clone = post_count.clone(); + let relay_server = { + use axum::{routing::post, Router}; + let app_router = Router::new().route( + "/events", + post(move |_body: String| { + let counter = post_count_clone.clone(); + async move { + counter.fetch_add(1, Ordering::SeqCst); + serde_json::json!({ + "event_id": "test-event-id", + "accepted": true, + "message": "" + }) + .to_string() + } + }), + ); + let listener = tokio::net::TcpListener::bind("127.0.0.1:0").await.unwrap(); + let addr = listener.local_addr().unwrap(); + tokio::spawn(async move { + axum::serve(listener, app_router).await.ok(); + }); + format!("http://{addr}") + }; + + // Point the workspace relay override at the counter server. + // Must be set BEFORE computing the active retention scope so the + // scope hash is stable for the sabotage step. + { + let state = app.state::(); + let mut override_slot = state + .relay_url_override + .lock() + .expect("relay_url_override must be lockable"); + *override_slot = Some(relay_server.clone()); + } + + // Seed persona "Alice" at a known revision. + let r1 = "2026-01-01T00:00:00Z"; + save_personas( + app.handle(), + &[AgentDefinition { + id: contract.persona_id.clone(), + display_name: contract.before_display_name.clone(), + updated_at: r1.to_string(), + created_at: r1.to_string(), + ..persona() + }], + ) + .expect("seed must succeed"); + + // Seed a linked agent with valid NSEC keys so sync_managed_agent_profile + // can sign and submit the kind:0 profile event. + let agent_keys = nostr::Keys::generate(); + let agent_record = ManagedAgentRecord { + pubkey: agent_keys.public_key().to_hex(), + name: contract.before_display_name.clone(), + persona_id: Some(contract.persona_id.clone()), + private_key_nsec: agent_keys.secret_key().to_secret_hex(), + 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, + env_vars: BTreeMap::new(), + start_on_app_launch: false, + auto_restart_on_config_change: false, + runtime_pid: None, + backend: Default::default(), + backend_agent_id: None, + provider_policy_pending: false, + provider_binary_path: None, + team_id: 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: Some(contract.before_display_name.clone()), + description: None, + slug: None, + runtime: None, + name_pool: vec![], + is_builtin: false, + is_active: true, + shared: false, + source_team: None, + source_team_persona_slug: None, + catalog_source: None, + team_catalog_source: None, + definition_respond_to: None, + definition_respond_to_allowlist: vec![], + definition_parallelism: None, + relay_mesh: None, + effort_level: None, + }; + save_managed_agents(app.handle(), &[agent_record]) + .expect("agent seed must succeed"); + + // Resolve the active retention scope (relay + owner → DB path) and + // sabotage it: replace the .db file path with a directory so that + // `open_retention_db` inside `prepare_persona_publication` returns + // "failed to open retention db". This exercises the production scope + // resolver rather than injecting an arbitrary bad path. + { + let state = app.state::(); + let scope = active_retention_scope(app.handle(), &state) + .expect("active_retention_scope must resolve with relay override set"); + // Remove the file if it was created by scope resolution, then + // create a directory at the same path so SQLite cannot open it. + std::fs::remove_file(&scope.db_path).ok(); + std::fs::create_dir_all(&scope.db_path) + .expect("must be able to create directory at db path for sabotage"); + } + + // Drive the REAL command path through a mock AppHandle. This exercises + // prepare_persona_publication (scope resolver + ?-propagation) and the + // phase-2 relay sync, verifying that wiring drift at the command + // boundary — e.g. update_persona_and_publish stopping to call + // update_persona_with — is caught. + let result = update_persona_and_publish_inner( + UpdatePersonaRequest { + id: contract.persona_id.clone(), + display_name: contract.after_display_name.clone(), + avatar_url: None, + description: None, + system_prompt: "Do the work.".to_string(), + runtime: None, + model: None, + provider: None, + name_pool: Vec::new(), + env_vars: None, + behavior: None, + expected_updated_at: Some(r1.to_string()), + }, + app.handle().clone(), + ) + .await; + + // The preparation failure must propagate (coordinator sees publishFailed). + assert!( + result.is_err(), + "update_persona_and_publish_inner must return Err when prepare_persona_publication fails" + ); + assert!( + result + .as_ref() + .unwrap_err() + .contains(&contract.command_error_contains), + "error must come from prepare_persona_publication scope resolver, got: {:?}", + result + ); + + // Both durable stores must reflect the edit even though strict + // publication preparation failed after the writes. + let persisted_personas = + load_personas(app.handle()).expect("persona store must reload after command error"); + let persisted_persona = persisted_personas + .iter() + .find(|persona| persona.id == contract.persona_id) + .expect("renamed persona must remain in the store"); + assert_eq!( + persisted_persona.display_name, contract.after_display_name, + "persona rename must persist before the strict preparation error propagates" + ); + + let persisted_agents = load_managed_agents(app.handle()) + .expect("managed-agent store must reload after command error"); + let persisted_agent = persisted_agents + .iter() + .find(|agent| agent.persona_id.as_deref() == Some(contract.persona_id.as_str())) + .expect("linked managed-agent record must remain in the store"); + assert_eq!( + persisted_agent.name, contract.after_display_name, + "linked record name must persist before the strict preparation error propagates" + ); + assert_eq!( + persisted_agent.display_name.as_deref(), + Some(contract.after_display_name.as_str()), + "linked record display_name must persist with the persona rename" + ); + + // Phase 2 must have run: relay sync fires despite the retain failure. + // Before the fix (retain_result? inside the blocking Ok): count is 0 → RED. + // After the fix (retain_result? after phase 2): count is 1 → GREEN. + assert_eq!( + post_count.load(Ordering::SeqCst), + contract.relay_profile_request_count, + "relay kind:0 profile sync must fire despite prepare_persona_publication failure; \ + restoring `retain_result?` before phase 2 turns this RED" + ); + }); // rt.block_on + + // Cleanup + std::env::remove_var("HOME"); + std::env::remove_var("XDG_DATA_HOME"); + match old_home { + Some(v) => std::env::set_var("HOME", v), + None => std::env::remove_var("HOME"), + } + match old_xdg { + Some(v) => std::env::set_var("XDG_DATA_HOME", v), + None => std::env::remove_var("XDG_DATA_HOME"), + } + } + + /// Behavioral race regression for P1-2: an explicit team unshare that + /// starts after refresh reads shared T must remain authoritative. + /// + /// The observer pauses refresh after its shared-T read. The competing + /// thread then announces that it is about to acquire the same store lock. + /// With the production lock in place, refresh must finish before unshare can + /// acquire; unshare writes last and the final head is unshared. Under the + /// mutation that moves refresh outside the lock, unshare acquires while + /// refresh is paused, writes unshared T+1 completely, and only then releases + /// refresh; refresh writes shared T+1 last and the final-state assertion + /// turns RED. No lock-presence assertion is involved. + #[tokio::test] + async fn test_retry_refresh_holds_store_lock_during_refresh() { + use crate::commands::teams::{prepare_team_publication_at, REFRESH_READ_OBSERVER}; + use crate::managed_agents::{ + retention::{get_retained_event, open_retention_db}, + TeamRecord, + }; + use buzz_core_pkg::kind::{event_is_shared, KIND_TEAM_CATALOG}; + use nostr::JsonUtil; + use std::collections::BTreeMap; + use std::sync::{mpsc, Arc}; + + let dir = tempfile::tempdir().unwrap(); + let db_path = dir.path().join("retention.db"); + let keys = nostr::Keys::generate(); + let owner = keys.public_key().to_hex(); + let relay_url = spawn_relay(true).await; + let state = Arc::new(build_app_state()); + + let persona_id = "catalog-reviewer"; + let team = TeamRecord { + id: "team-abc".to_string(), + name: "Test Team".to_string(), + description: None, + instructions: None, + persona_ids: vec![persona_id.to_string()], + is_builtin: false, + shared: true, + catalog_source: None, + source_dir: None, + 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(), + }; + let persona_member = AgentDefinition { + id: persona_id.to_string(), + display_name: "Catalog Reviewer".to_string(), + description: None, + avatar_url: None, + system_prompt: "Review.".to_string(), + runtime: None, + model: None, + provider: None, + name_pool: Vec::new(), + is_builtin: false, + is_active: true, + shared: false, + source_team: None, + source_team_persona_slug: None, + catalog_source: None, + team_catalog_source: None, + env_vars: BTreeMap::new(), + respond_to: None, + respond_to_allowlist: Vec::new(), + parallelism: None, + created_at: "2026-01-01T00:00:00Z".to_string(), + updated_at: "2026-01-01T00:00:00Z".to_string(), + }; + + // Seed the authoritative shared kind:30178 head T. + prepare_team_publication_at( + &db_path, + &keys, + &team, + std::slice::from_ref(&persona_member), + Some(true), + ) + .expect("seed shared team head must succeed"); + + // Seed the real file seam used by refresh_for_persona_at. The changed + // prompt guarantees the refresh rebuild is not idempotently skipped. + let teams_json = serde_json::to_string(&[&team]).expect("serialize team"); + std::fs::write(dir.path().join("teams.json"), teams_json.as_bytes()).unwrap(); + let persona_updated = AgentDefinition { + system_prompt: "Review catalog entries carefully.".to_string(), + ..persona_member.clone() + }; + let personas_json = + serde_json::to_string(&[&persona_updated]).expect("serialize updated persona"); + std::fs::write(dir.path().join("personas.json"), personas_json.as_bytes()) + .expect("write personas.json must succeed"); + + let initial_created_at = { + let conn = open_retention_db(&db_path).unwrap(); + get_retained_event(&conn, KIND_TEAM_CATALOG, &owner, "team-abc") + .unwrap() + .expect("seed head must exist") + .created_at + }; + + // Deterministic two-phase release. The observer first pauses refresh + // after shared T was read. The unshare thread announces readiness but + // cannot attempt the lock until the observer probes and releases it. + let (refresh_read_tx, refresh_read_rx) = mpsc::channel::<()>(); + let (unshare_ready_tx, unshare_ready_rx) = mpsc::channel::<()>(); + let (unshare_go_tx, unshare_go_rx) = mpsc::channel::<()>(); + let (unshare_complete_tx, unshare_complete_rx) = mpsc::channel::<()>(); + let state_observer = state.clone(); + { + let mut slot = REFRESH_READ_OBSERVER.lock().expect("observer slot"); + *slot = Some(Box::new(move || { + refresh_read_tx + .send(()) + .expect("unshare thread must receive refresh-read signal"); + unshare_ready_rx + .recv() + .expect("unshare thread must announce readiness"); + + // This probe is ordering control, not the oracle. If the caller + // holds the production lock, release the unshare to block on it + // and let refresh finish. If the lock-moved mutation leaves the + // lock free, release the unshare and require its write to finish + // before allowing refresh to resume. Only final retained state + // below decides whether the test passes. + match state_observer.managed_agents_store_lock.try_lock() { + Ok(probe_guard) => { + drop(probe_guard); + unshare_go_tx + .send(()) + .expect("unshare thread must receive release signal"); + unshare_complete_rx + .recv() + .expect("unshare must complete before unlocked refresh resumes"); + } + Err(_) => { + unshare_go_tx + .send(()) + .expect("unshare thread must receive release signal"); + } + } + })); + } + + let state_unshare = state.clone(); + let db_path_unshare = db_path.clone(); + let keys_unshare = keys.clone(); + let team_unshare = team.clone(); + let member_unshare = persona_member.clone(); + let unshare_thread = std::thread::spawn(move || { + refresh_read_rx + .recv() + .expect("refresh must announce its shared-T read"); + unshare_ready_tx + .send(()) + .expect("observer must receive readiness signal"); + unshare_go_rx + .recv() + .expect("observer must release the unshare lock attempt"); + let _guard = state_unshare + .managed_agents_store_lock + .lock() + .expect("unshare lock must not be poisoned"); + prepare_team_publication_at( + &db_path_unshare, + &keys_unshare, + &team_unshare, + &[member_unshare], + Some(false), + ) + .expect("unshare must retain its unshared head"); + // Under correct locking the observer has already returned and + // drops this receiver; under the lock-moved mutation it is waiting + // for this completion signal. Both outcomes are intentional. + let _ = unshare_complete_tx.send(()); + }); + + let prepared = prepared(&db_path, relay_url, keys.clone(), Some(true)); + let persona_id_str = prepared.persona.id.clone(); + let result = publish_and_refresh_teams_at( + &state, + prepared, + dir.path(), + &keys, + &db_path, + &persona_id_str, + ) + .await; + + { + let mut slot = REFRESH_READ_OBSERVER.lock().expect("observer slot"); + *slot = None; + } + unshare_thread + .join() + .expect("unshare thread must not panic"); + result.expect("publish_and_refresh_teams_at must succeed"); + + let conn = open_retention_db(&db_path).unwrap(); + let retained = get_retained_event(&conn, KIND_TEAM_CATALOG, &owner, "team-abc") + .expect("db query must not fail") + .expect("retained team catalog head must exist after refresh + unshare"); + assert!( + retained.created_at >= initial_created_at, + "retained head must be at or after the seed timestamp" + ); + let event = + nostr::Event::from_json(&retained.raw_event).expect("must parse retained event"); + assert!( + !event_is_shared(&event), + "final retained kind:30178 head must be UNSHARED; moving refresh outside \ + managed_agents_store_lock lets the paused refresh overwrite the completed unshare" + ); + } } diff --git a/desktop/src-tauri/src/commands/personas/update.rs b/desktop/src-tauri/src/commands/personas/update.rs index 46d0c8a99dc..dba4249de2b 100644 --- a/desktop/src-tauri/src/commands/personas/update.rs +++ b/desktop/src-tauri/src/commands/personas/update.rs @@ -16,9 +16,62 @@ use crate::{ use super::{normalize_description, pending, retain_persona_pending, trim_optional, trim_required}; +#[cfg(test)] +mod concurrent_edit_tests; #[cfg(test)] mod name_propagation_tests; +/// Test-only hook: a process-global observer called right before +/// `find_persona_for_update` while `managed_agents_store_lock` is held. +/// Tests can install a closure that asserts the lock is not re-acquirable +/// at that point, proving the guard and the comparison share the same +/// lock scope. Production builds compile this away entirely. +#[cfg(test)] +type GuardObserver = Box; +#[cfg(test)] +pub(crate) static PRE_GUARD_OBSERVER: std::sync::Mutex> = + std::sync::Mutex::new(None); + +/// Marker prefixed to the compare-and-swap rejection so the frontend can map it +/// to the "changed while you were editing" affordance rather than a generic +/// save failure. The persisted definition advanced past the revision the editor +/// was seeded with, so applying this stale full-replacement input would clobber +/// the newer writer. +pub(crate) const PERSONA_REVISION_CONFLICT: &str = "persona-revision-conflict:"; + +/// Locate the persona to edit and enforce compare-and-swap in one step, so the +/// revision guard and the write target can never diverge. +/// +/// `expected_updated_at` is the definition revision the editor was seeded with. +/// When present, it must still equal the persisted record's `updated_at`; a +/// mismatch means another writer committed since the editor opened, and this +/// full-replacement input would silently clobber their newer fields — so the +/// edit is rejected before the caller mutates anything. `None` skips the check +/// (legacy callers, instance-only saves). +/// +/// Because the caller holds the store lock across load → this guard → write, +/// the compared `updated_at` is the authoritative persisted value, closing the +/// check-to-write window a pre-write refetch alone cannot. +fn find_persona_for_update<'a>( + personas: &'a mut [AgentDefinition], + id: &str, + expected_updated_at: Option<&str>, +) -> Result<&'a mut AgentDefinition, String> { + let persona = personas + .iter_mut() + .find(|record| record.id == id) + .ok_or_else(|| format!("agent {id} not found"))?; + if let Some(expected) = expected_updated_at { + if persona.updated_at != expected { + return Err(format!( + "{PERSONA_REVISION_CONFLICT}{} changed while you were editing", + persona.display_name + )); + } + } + Ok(persona) +} + /// Return value of the `update_persona` command. Uses flatten so all /// `AgentDefinition` fields appear at the top level of the JSON response — /// backward-compatible with callers that already destructure a raw persona object. @@ -145,17 +198,19 @@ pub async fn update_persona( /// [`update_persona`] enqueues best-effort, while /// [`sharing::update_persona_and_publish`] prepares a strict publication and /// returns the event so the caller can await relay acceptance. -pub(super) async fn update_persona_with( +pub(super) async fn update_persona_with( input: UpdatePersonaRequest, - app: AppHandle, - retain: impl FnOnce(&AppHandle, &AppState, &AgentDefinition) -> Result + Send + 'static, + app: tauri::AppHandle, + retain: impl FnOnce(&tauri::AppHandle, &AppState, &AgentDefinition) -> Result + + Send + + 'static, ) -> Result<(AgentDefinition, R), String> { use tauri::Manager; // Phase 1: synchronous save (persona record + linked agent avatar updates) - let (result, retained, profile_sync_params) = tokio::task::spawn_blocking({ + let (result, retain_result, profile_sync_params) = tokio::task::spawn_blocking({ let app = app.clone(); - move || -> Result<(AgentDefinition, R, ProfileSyncParams), String> { + move || -> Result<(AgentDefinition, Result, ProfileSyncParams), String> { let state = app.state::(); let display_name = trim_required(&input.display_name, "Display name")?; let system_prompt = input.system_prompt.clone(); @@ -172,10 +227,25 @@ pub(super) async fn update_persona_with( .map_err(|error| error.to_string())?; let mut personas = load_personas(&app)?; pending::project_active_persona_sharing(&app, &state, &mut personas); - let persona = personas - .iter_mut() - .find(|record| record.id == input.id) - .ok_or_else(|| format!("agent {} not found", input.id))?; + // In test builds, fire the pre-guard observer (if installed) with a + // reference to the state while the store lock is still held. Tests + // use this to assert that `managed_agents_store_lock.try_lock()` + // fails here — proving the guard and the comparison are inside the + // same lock scope. The observer fires on every code path, so moving + // the lock acquisition to after this call turns RED. + #[cfg(test)] + { + if let Ok(observer_guard) = PRE_GUARD_OBSERVER.lock() { + if let Some(ref observer) = *observer_guard { + observer(&state); + } + } + } + let persona = find_persona_for_update( + &mut personas, + &input.id, + input.expected_updated_at.as_deref(), + )?; // Track what changed so we can propagate to linked agent records. let avatar_changed = persona.avatar_url != avatar_url; @@ -212,7 +282,15 @@ pub(super) async fn update_persona_with( let result = persona.clone(); save_personas(&app, &personas)?; - let retained = retain(&app, &state, &result)?; + // Capture the retain result WITHOUT propagating the error yet. + // Linked managed-agent persistence (name/avatar propagation) and + // relay profile sync are independent of strict publication — a + // transient publication failure must not skip those local effects. + // Return the retain error after completing all linked work so the + // coordinator sees the accurate "definition saved but publication + // failed" error and can attempt the publish-only retry seam, which + // will now find the linked identities already updated. + let retain_result = retain(&app, &state, &result); try_regenerate_nest(&app); // If the avatar, display_name, or effective description changed, @@ -291,7 +369,7 @@ pub(super) async fn update_persona_with( Vec::new() }; - Ok((result, retained, sync_params)) + Ok((result, retain_result, sync_params)) } }) .await @@ -323,5 +401,11 @@ pub(super) async fn update_persona_with( } } + // Propagate the retain error only after all linked identity effects have + // completed (local managed-agent store write and relay kind:0 profile sync). + // A strict publication failure must not skip these: the coordinator's + // publish-only retry will see the linked identities already in sync. + let retained = retain_result?; + Ok((result, retained)) } diff --git a/desktop/src-tauri/src/commands/personas/update/concurrent_edit_tests.rs b/desktop/src-tauri/src/commands/personas/update/concurrent_edit_tests.rs new file mode 100644 index 00000000000..82b1de8be16 --- /dev/null +++ b/desktop/src-tauri/src/commands/personas/update/concurrent_edit_tests.rs @@ -0,0 +1,766 @@ +//! Regression for Carl round-4 P1: the persona edit must reject a +//! compare-and-swap conflict BEFORE it mutates or writes, so a stale +//! full-replacement input cannot clobber a concurrent writer. +//! +//! Three layers of coverage: +//! +//! 1. **Comparison logic** — four unit tests against `find_persona_for_update` +//! directly: stale rejection, matching-revision pass, absent-revision skip, +//! and not-found vs conflict distinction. +//! +//! 2. **Command-path wiring** — one async test drives the real +//! `update_persona_with` against a file-backed store under a +//! `MockRuntime` `AppHandle`. Writer A commits R1→R2 through the command +//! path; writer B then submits with expected R1, is rejected with +//! `PERSONA_REVISION_CONFLICT`, and the store still reads R2. Turns RED if +//! `update_persona_with` removes the guard call entirely. +//! +//! 3. **Lock-scope assertion** — a test-only observer (`PRE_GUARD_OBSERVER`) +//! fires inside the `spawn_blocking` body right before `find_persona_for_update` +//! while `managed_agents_store_lock` is held. The observer asserts +//! `try_lock()` fails — proving the guard and the comparison share the same +//! lock scope. Moving the lock acquisition to after the comparison (recreating +//! the TOCTOU race) means the observer fires before the lock is held and +//! `try_lock()` succeeds, turning this test RED. + +use super::{ + find_persona_for_update, update_persona_with, PERSONA_REVISION_CONFLICT, PRE_GUARD_OBSERVER, +}; +use crate::app_state::build_app_state; +use crate::managed_agents::{save_personas, AgentDefinition, UpdatePersonaRequest}; + +/// A persisted persona at revision `updated_at`. The guard reads only `id`, +/// `display_name`, and `updated_at`. +fn persona(id: &str, display_name: &str, updated_at: &str) -> AgentDefinition { + AgentDefinition { + id: id.to_string(), + display_name: display_name.to_string(), + avatar_url: None, + description: None, + system_prompt: "Do the work.".to_string(), + runtime: None, + model: None, + provider: None, + name_pool: Vec::new(), + is_builtin: false, + is_active: true, + shared: false, + source_team: None, + source_team_persona_slug: None, + catalog_source: None, + team_catalog_source: None, + env_vars: std::collections::BTreeMap::new(), + respond_to: None, + respond_to_allowlist: Vec::new(), + parallelism: None, + created_at: "2026-01-01T00:00:00Z".to_string(), + updated_at: updated_at.to_string(), + } +} + +const R1: &str = "2026-01-01T00:00:00Z"; +const R2: &str = "2026-06-01T00:00:00Z"; + +#[test] +fn two_writer_overwrite_is_rejected_and_the_newer_revision_survives() { + // Writer B seeded its editor at R1. Writer A then committed R2, so the + // persisted record now reads R2 when B submits. B's compare-and-swap + // (expected R1) must be rejected before any mutation — proving A's R2 + // survives because the caller never reaches the write. + let mut personas = vec![persona("p1", "Alice", R2)]; + + let err = find_persona_for_update(&mut personas, "p1", Some(R1)) + .expect_err("a stale expected revision must be rejected"); + + assert!( + err.starts_with(PERSONA_REVISION_CONFLICT), + "rejection must carry the conflict marker so the UI shows the drift toast; got: {err}" + ); + assert!( + err.contains("Alice"), + "rejection names the persona; got: {err}" + ); + // R2 survives untouched: the guard returned before handing out a mutable + // handle, so nothing was overwritten. + assert_eq!( + personas[0].updated_at, R2, + "the newer revision is preserved" + ); + assert_eq!(personas[0].display_name, "Alice"); +} + +#[test] +fn matching_revision_resolves_the_record_for_the_write() { + // No concurrent writer: the persisted revision still equals the seed, so + // the guard hands back the record and the caller proceeds to write. + let mut personas = vec![persona("p1", "Alice", R1)]; + + let resolved = find_persona_for_update(&mut personas, "p1", Some(R1)) + .expect("a matching revision must resolve the record"); + + assert_eq!(resolved.id, "p1"); +} + +#[test] +fn absent_expected_revision_skips_the_guard() { + // Legacy callers and instance-only saves send no expected revision; the + // guard must be inert and still resolve the record regardless of drift. + let mut personas = vec![persona("p1", "Alice", R2)]; + + let resolved = find_persona_for_update(&mut personas, "p1", None) + .expect("no expected revision skips the compare-and-swap"); + + assert_eq!(resolved.updated_at, R2); +} + +#[test] +fn missing_persona_reports_not_found_not_a_conflict() { + // A resolve miss is a plain not-found error, distinct from the revision + // conflict — the UI must not show the drift toast for a deleted persona. + let mut personas = vec![persona("p1", "Alice", R1)]; + + let err = find_persona_for_update(&mut personas, "ghost", Some(R1)) + .expect_err("an unknown id must error"); + + assert!( + !err.starts_with(PERSONA_REVISION_CONFLICT), + "not a conflict" + ); + assert!( + err.contains("ghost") && err.contains("not found"), + "got: {err}" + ); +} + +/// Build a headless `MockRuntime` `AppHandle` wired with `build_app_state`. +/// The app resolves its data dir from `$HOME` / `$XDG_DATA_HOME`; the caller +/// holds the path mutex and has overridden both so all store reads/writes land +/// inside its tempdir. +fn mock_app() -> tauri::App { + let state = build_app_state(); + tauri::test::mock_builder() + .manage(state) + .build(tauri::test::mock_context(tauri::test::noop_assets())) + .expect("mock app builds headless") +} + +/// Build a minimal `UpdatePersonaRequest` for persona `id` with an optional +/// expected revision. Only `display_name` and `system_prompt` are required; +/// all other fields default to absent. +fn update_request(id: &str, display_name: &str, expected: Option<&str>) -> UpdatePersonaRequest { + UpdatePersonaRequest { + id: id.to_string(), + display_name: display_name.to_string(), + avatar_url: None, + description: None, + system_prompt: "Do the work.".to_string(), + runtime: None, + model: None, + provider: None, + name_pool: Vec::new(), + env_vars: None, + behavior: None, + expected_updated_at: expected.map(str::to_string), + } +} + +/// Command-path regression suite: runs in a single async test to prevent +/// test-isolation races on the process-global `HOME`/`XDG_DATA_HOME` +/// environment variables and the `PRE_GUARD_OBSERVER` slot. +/// +/// **Layer 2 — wiring:** Writer A commits R1→R2 through the real +/// `update_persona_with`; Writer B submits with expected R1 and is rejected; +/// the persisted store still reads R2 with A's fields intact. Turns RED if +/// the guard call is removed entirely from `update_persona_with`. +/// +/// **Layer 3 — lock scope:** the `PRE_GUARD_OBSERVER` hook fires inside +/// `spawn_blocking` right before `find_persona_for_update` while +/// `_store_guard` is in scope. The observer asserts `try_lock()` fails, +/// proving the guard and the comparison are inside the same lock scope. +/// Moving the lock acquisition to after the comparison (the TOCTOU shape) +/// means `try_lock()` succeeds and the assertion panics — turning this test +/// **RED**. +#[test] +fn command_path_and_lock_scope_regressions() { + use std::sync::atomic::{AtomicBool, Ordering}; + use std::sync::Arc; + + let temp = tempfile::tempdir().unwrap(); + let home = temp.path().join("home"); + std::fs::create_dir_all(&home).unwrap(); + + let old_home = std::env::var_os("HOME"); + let old_xdg = std::env::var_os("XDG_DATA_HOME"); + // Hold the path mutex for the entire test so concurrent tests that also + // mutate HOME cannot race against our store reads/writes. + let _path_guard = crate::managed_agents::lock_path_mutex(); + std::env::set_var("HOME", &home); + std::env::set_var("XDG_DATA_HOME", &home); + + let rt = tokio::runtime::Builder::new_current_thread() + .enable_all() + .build() + .expect("build test runtime"); + + rt.block_on(async { + let app = mock_app(); + + // ── Layer 3: lock-scope assertion ───────────────────────────────────── + // Install the observer BEFORE seeding so it fires on the first real write. + let observer_fired = Arc::new(AtomicBool::new(false)); + let observer_fired_clone = observer_fired.clone(); + { + let mut slot = PRE_GUARD_OBSERVER.lock().expect("observer slot"); + *slot = Some(Box::new(move |state: &crate::app_state::AppState| { + assert!( + state.managed_agents_store_lock.try_lock().is_err(), + "managed_agents_store_lock must already be held when find_persona_for_update \ + runs — a successful try_lock means the comparison happens outside the lock, \ + which recreates the TOCTOU race" + ); + observer_fired_clone.store(true, Ordering::SeqCst); + })); + } + + // Seed: one persona at R1 in the persisted store. + save_personas(app.handle(), &[persona("p1", "Alice", R1)]) + .expect("seed write must succeed"); + + // Layer 3 probe: a successful write fires the observer while the lock is held. + let probe_result = update_persona_with( + update_request("p1", "Alice probe", Some(R1)), + app.handle().clone(), + |_app, _state, _persona| Ok(()), + ) + .await; + + // Clear the observer immediately — must happen before any assertion that + // could panic, so it is never left installed for subsequent invocations. + { + let mut slot = PRE_GUARD_OBSERVER.lock().expect("observer slot"); + *slot = None; + } + + probe_result.expect("probe write must succeed — revision matches the seed"); + assert!( + observer_fired.load(Ordering::SeqCst), + "the pre-guard observer must have fired — if it did not, the hook is not wired" + ); + + // ── Layer 2: command-path wiring ────────────────────────────────────── + // Writer A: now at the probe's updated_at (R2). Capture it so B can use + // the original R1 as a stale seed. + // + // Re-seed at R1 so the two-writer scenario starts from a known revision. + save_personas(app.handle(), &[persona("p1", "Alice", R1)]) + .expect("re-seed write must succeed"); + + let a_result = update_persona_with( + update_request("p1", "Alice A", Some(R1)), + app.handle().clone(), + |_app, _state, _persona| Ok(()), + ) + .await; + let (a_persona, ()) = a_result.expect("writer A must succeed — revision matches the seed"); + let r2 = a_persona.updated_at.clone(); + assert_ne!(r2, R1, "the commit must advance the revision past R1"); + + // Writer B: seeded at R1, submits after A has committed R2. + let b_result = update_persona_with( + update_request("p1", "Alice B (must not land)", Some(R1)), + app.handle().clone(), + |_app, _state, _persona| Ok(()), + ) + .await; + let b_err = + b_result.expect_err("writer B must be rejected — its seed revision R1 is stale"); + + assert!( + b_err.starts_with(PERSONA_REVISION_CONFLICT), + "rejection must carry the conflict marker; got: {b_err}" + ); + + // Reload from the persisted store and confirm A's R2 survived B's attempt. + let persisted = + crate::managed_agents::load_personas(app.handle()).expect("reload must succeed"); + let stored = persisted + .iter() + .find(|p| p.id == "p1") + .expect("persona must still exist after B's rejection"); + + assert_eq!( + stored.updated_at, r2, + "A's committed revision must survive B's stale write attempt" + ); + assert_eq!( + stored.display_name, "Alice A", + "A's committed display_name must survive B's stale write attempt" + ); + }); // rt.block_on + + // ── Cleanup ─────────────────────────────────────────────────────────── + std::env::remove_var("HOME"); + std::env::remove_var("XDG_DATA_HOME"); + match old_home { + Some(v) => std::env::set_var("HOME", v), + None => std::env::remove_var("HOME"), + } + match old_xdg { + Some(v) => std::env::set_var("XDG_DATA_HOME", v), + None => std::env::remove_var("XDG_DATA_HOME"), + } +} + +/// P1-1 regression: the retain callback (the "publish" step inside +/// `update_persona_with`) runs AFTER `save_personas`, so a retain failure +/// occurs when the persona is already durable on disk. +/// +/// The command-path contract: `update_persona_with` must return `Err(_)` when +/// the retain callback returns `Err(_)`, even though the persona write already +/// persisted. The error must reach the coordinator so it can set `publishFailed` +/// and refuse to close the dialog as full success. If the retain error were +/// swallowed — e.g. the callback's `?` were removed and it returned `Ok(())` — +/// this test would become GREEN on a broken code path and catch it. +#[test] +fn retain_failure_after_persist_is_returned_not_swallowed() { + let temp = tempfile::tempdir().unwrap(); + let home = temp.path().join("home"); + std::fs::create_dir_all(&home).unwrap(); + + let old_home = std::env::var_os("HOME"); + let old_xdg = std::env::var_os("XDG_DATA_HOME"); + let _path_guard = crate::managed_agents::lock_path_mutex(); + std::env::set_var("HOME", &home); + std::env::set_var("XDG_DATA_HOME", &home); + + let rt = tokio::runtime::Builder::new_current_thread() + .enable_all() + .build() + .expect("build test runtime"); + + rt.block_on(async { + let app = mock_app(); + + // Seed one persona at R1 in the persisted store. + save_personas(app.handle(), &[persona("p1", "Alice", R1)]) + .expect("seed write must succeed"); + + // Submit via update_persona_with with a retain callback that always fails. + // The persona save runs first (line ~210 in update.rs: `save_personas`), + // then the retain callback is called. If the retain error propagates, we + // get Err; if it is silently discarded, we get Ok — the test catches both. + let result = update_persona_with( + update_request("p1", "Alice retain-fail", Some(R1)), + app.handle().clone(), + |_app, _state, _persona| -> Result<(), String> { + Err("simulated retain / publish failure after persona persisted".to_string()) + }, + ) + .await; + + assert!( + result.is_err(), + "update_persona_with must return Err when the retain callback fails — \ + the save coordinator must see this error, not a silent success; \ + got: {:?}", + result.ok() + ); + + let err = result.unwrap_err(); + assert!( + err.contains("simulated retain"), + "the error text must propagate from the retain callback; got: {err}" + ); + + // Verify the persona DID persist (save_personas ran before retain) so we + // confirm the test scenario actually exercises the post-persist failure path. + let persisted = + crate::managed_agents::load_personas(app.handle()).expect("reload must succeed"); + let stored = persisted + .iter() + .find(|p| p.id == "p1") + .expect("persona must exist"); + assert_eq!( + stored.display_name, "Alice retain-fail", + "persona fields must have persisted before retain was called" + ); + }); // rt.block_on + + // Cleanup + std::env::remove_var("HOME"); + std::env::remove_var("XDG_DATA_HOME"); + match old_home { + Some(v) => std::env::set_var("HOME", v), + None => std::env::remove_var("HOME"), + } + match old_xdg { + Some(v) => std::env::set_var("XDG_DATA_HOME", v), + None => std::env::remove_var("XDG_DATA_HOME"), + } +} + +/// P2 regression: linked managed-agent name and avatar propagation must +/// complete even when the retain/publish callback returns an error. +/// +/// Before the fix: `retain(&app, &state, &result)?` propagated the error +/// immediately, skipping the avatar/name propagation block and `save_managed_agents`. +/// A publish-only retry (`set_persona_shared`) then published the persona but +/// the linked instance still carried the old name/avatar. +/// +/// After the fix: `retain_result` is captured without `?`, all linked +/// persistence runs to completion, and THEN the retain error is propagated. +/// +/// Mutation acceptance: restoring `retain(&app, &state, &result)?` before +/// the avatar/name propagation block causes `save_managed_agents` to never run +/// and the assertion on the stored agent name turns RED. +#[test] +fn linked_instance_rename_completes_before_retain_error_propagates() { + let temp = tempfile::tempdir().unwrap(); + let home = temp.path().join("home2"); + std::fs::create_dir_all(&home).unwrap(); + + let old_home = std::env::var_os("HOME"); + let old_xdg = std::env::var_os("XDG_DATA_HOME"); + // Hold the path mutex for the entire test so concurrent tests that also + // mutate HOME cannot race against our store reads/writes. + let _path_guard = crate::managed_agents::lock_path_mutex(); + std::env::set_var("HOME", &home); + std::env::set_var("XDG_DATA_HOME", &home); + + let rt = tokio::runtime::Builder::new_current_thread() + .enable_all() + .build() + .expect("build test runtime"); + + rt.block_on(async { + let app = mock_app(); + + // Seed persona "Alice" at R1. + save_personas(app.handle(), &[persona("p1", "Alice", R1)]).expect("seed must succeed"); + + // Seed a linked agent whose name matches the persona's display_name. + let agent_record = crate::managed_agents::ManagedAgentRecord { + pubkey: "pk-alice-p2".to_string(), + name: "Alice".to_string(), + persona_id: Some("p1".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, + env_vars: std::collections::BTreeMap::new(), + start_on_app_launch: false, + auto_restart_on_config_change: false, + runtime_pid: None, + backend: Default::default(), + backend_agent_id: None, + provider_policy_pending: false, + provider_binary_path: None, + team_id: 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: Some("Alice".to_string()), + description: None, + slug: None, + runtime: None, + name_pool: vec![], + is_builtin: false, + is_active: true, + shared: false, + source_team: None, + source_team_persona_slug: None, + catalog_source: None, + team_catalog_source: None, + definition_respond_to: None, + definition_respond_to_allowlist: vec![], + definition_parallelism: None, + relay_mesh: None, + effort_level: None, + }; + crate::managed_agents::save_managed_agents(app.handle(), &[agent_record]) + .expect("agent seed must succeed"); + + // Submit a rename with a retain callback that always fails — simulates a + // strict publication/enqueue failure AFTER save_personas ran. + let result = update_persona_with( + UpdatePersonaRequest { + id: "p1".to_string(), + display_name: "Alice Renamed".to_string(), + avatar_url: None, + description: None, + system_prompt: "Do the work.".to_string(), + runtime: None, + model: None, + provider: None, + name_pool: Vec::new(), + env_vars: None, + behavior: None, + expected_updated_at: Some(R1.to_string()), + }, + app.handle().clone(), + |_app, _state, _persona| -> Result<(), String> { + Err("simulated publish failure after persona persisted".to_string()) + }, + ) + .await; + + // The retain error must propagate so the coordinator sees publishFailed. + assert!( + result.is_err(), + "update_persona_with must return Err when retain fails; \ + if this is Ok the retain error was swallowed" + ); + + // The linked agent must have been renamed BEFORE the error propagated. + // If save_managed_agents was skipped (the pre-fix path), the name is still "Alice". + let agents = crate::managed_agents::load_managed_agents(app.handle()) + .expect("agent reload must succeed"); + let stored_agent = agents + .iter() + .find(|a| a.pubkey == "pk-alice-p2") + .expect("linked agent must still exist"); + + assert_eq!( + stored_agent.name, "Alice Renamed", + "linked instance name must be propagated before the retain error is returned; \ + restoring `retain()?` before the propagation block turns this RED" + ); + }); // rt.block_on + + // Cleanup + std::env::remove_var("HOME"); + std::env::remove_var("XDG_DATA_HOME"); + match old_home { + Some(v) => std::env::set_var("HOME", v), + None => std::env::remove_var("HOME"), + } + match old_xdg { + Some(v) => std::env::set_var("XDG_DATA_HOME", v), + None => std::env::remove_var("XDG_DATA_HOME"), + } +} + +/// P2 relay-sync regression: the relay kind:0 profile sync for linked agents +/// must complete even when the retain/publish callback returns an error. +/// +/// Before the fix: `retain_result?` applied `?` inside the blocking phase's +/// return tuple, exiting before `profile_sync_params` was returned to the outer +/// function. Phase 2 (the async `sync_managed_agent_profile` loop) never ran — +/// the linked relay identity remained stale even though the local record updated. +/// +/// After the fix: `retain_result` is returned un-`?`d from the blocking phase, +/// phase 2 runs to completion, and THEN the retain error is propagated. +/// +/// Mutation acceptance: restoring `retain_result?` inside `Ok((result, retain_result?, …))` +/// causes the blocking phase to exit before `profile_sync_params` is returned, +/// so the counter server receives 0 requests and this test turns RED. +#[test] +fn linked_instance_relay_profile_syncs_despite_retain_failure() { + use std::sync::atomic::{AtomicUsize, Ordering}; + use std::sync::Arc; + use tauri::Manager; + + let temp = tempfile::tempdir().unwrap(); + let home = temp.path().join("home3"); + std::fs::create_dir_all(&home).unwrap(); + + let old_home = std::env::var_os("HOME"); + let old_xdg = std::env::var_os("XDG_DATA_HOME"); + let _path_guard = crate::managed_agents::lock_path_mutex(); + std::env::set_var("HOME", &home); + std::env::set_var("XDG_DATA_HOME", &home); + + let rt = tokio::runtime::Builder::new_current_thread() + .enable_all() + .build() + .expect("build test runtime"); + + rt.block_on(async { + let app = mock_app(); + + // Spawn a local HTTP server that counts POST /events requests. + // sync_managed_agent_profile posts kind:0 profile events here. + let post_count = Arc::new(AtomicUsize::new(0)); + let post_count_clone = post_count.clone(); + let relay_server = { + use axum::{routing::post, Router}; + let app_router = Router::new().route( + "/events", + post(move |_body: String| { + let counter = post_count_clone.clone(); + async move { + counter.fetch_add(1, Ordering::SeqCst); + serde_json::json!({ + "event_id": "test-event-id", + "accepted": true, + "message": "" + }) + .to_string() + } + }), + ); + let listener = tokio::net::TcpListener::bind("127.0.0.1:0").await.unwrap(); + let addr = listener.local_addr().unwrap(); + tokio::spawn(async move { + axum::serve(listener, app_router).await.ok(); + }); + format!("http://{addr}") + }; + + // Point the workspace relay override at the counter server so that + // relay_ws_url_with_override (used to build relay_url per record) + // routes profile syncs to our counter, not the real relay. + { + let state = app.state::(); + let mut override_slot = state + .relay_url_override + .lock() + .expect("relay_url_override must be lockable"); + *override_slot = Some(relay_server.clone()); + } + + // Seed persona "Alice" at R1. + save_personas(app.handle(), &[persona("p1", "Alice", R1)]).expect("seed must succeed"); + + // Seed a linked agent with valid NSEC keys so sync_managed_agent_profile + // can sign and submit the kind:0 profile event. + let agent_keys = nostr::Keys::generate(); + let agent_record = crate::managed_agents::ManagedAgentRecord { + pubkey: agent_keys.public_key().to_hex(), + name: "Alice".to_string(), + persona_id: Some("p1".to_string()), + private_key_nsec: agent_keys.secret_key().to_secret_hex(), + 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, + env_vars: std::collections::BTreeMap::new(), + start_on_app_launch: false, + auto_restart_on_config_change: false, + runtime_pid: None, + backend: Default::default(), + backend_agent_id: None, + provider_policy_pending: false, + provider_binary_path: None, + team_id: 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: Some("Alice".to_string()), + description: None, + slug: None, + runtime: None, + name_pool: vec![], + is_builtin: false, + is_active: true, + shared: false, + source_team: None, + source_team_persona_slug: None, + catalog_source: None, + team_catalog_source: None, + definition_respond_to: None, + definition_respond_to_allowlist: vec![], + definition_parallelism: None, + relay_mesh: None, + effort_level: None, + }; + crate::managed_agents::save_managed_agents(app.handle(), &[agent_record]) + .expect("agent seed must succeed"); + + // Submit a rename with a retain closure that always fails — simulates + // strict publication failure after persona save. The display_name + // change triggers name propagation and relay profile sync. + let result = update_persona_with( + UpdatePersonaRequest { + id: "p1".to_string(), + display_name: "Alice Renamed".to_string(), + avatar_url: None, + description: None, + system_prompt: "Do the work.".to_string(), + runtime: None, + model: None, + provider: None, + name_pool: Vec::new(), + env_vars: None, + behavior: None, + expected_updated_at: Some(R1.to_string()), + }, + app.handle().clone(), + |_app, _state, _persona| -> Result<(), String> { + Err("simulated strict publication failure after persona persisted".to_string()) + }, + ) + .await; + + // The retain error must propagate so the coordinator sees publishFailed. + assert!( + result.is_err(), + "update_persona_with must return Err when retain fails" + ); + + // Phase 2 must have run: the counter server must have received exactly + // one kind:0 profile-sync POST for the renamed linked agent. + // Before the fix (retain_result? inside the blocking Ok): profile_sync_params + // is never returned to the outer fn — count is 0, test RED. + // After the fix (retain_result? after phase 2): count is 1, test GREEN. + assert_eq!( + post_count.load(Ordering::SeqCst), + 1, + "relay kind:0 profile sync must fire despite retain failure; \ + restoring `retain_result?` before phase 2 (the pre-fix shape) turns this RED" + ); + }); // rt.block_on + + // Cleanup: restore HOME/XDG after the relay-profile-syncs test + std::env::remove_var("HOME"); + std::env::remove_var("XDG_DATA_HOME"); + match old_home { + Some(v) => std::env::set_var("HOME", v), + None => std::env::remove_var("HOME"), + } + match old_xdg { + Some(v) => std::env::set_var("XDG_DATA_HOME", v), + None => std::env::remove_var("XDG_DATA_HOME"), + } +} diff --git a/desktop/src-tauri/src/commands/teams/mod.rs b/desktop/src-tauri/src/commands/teams/mod.rs index 208ac3a7117..4b7ab61b26a 100644 --- a/desktop/src-tauri/src/commands/teams/mod.rs +++ b/desktop/src-tauri/src/commands/teams/mod.rs @@ -198,6 +198,11 @@ mod adopt; mod pending; mod sharing; pub use adopt::add_team_from_catalog; +#[cfg(test)] +pub(crate) use pending::prepare_team_publication_at; +pub(crate) use pending::refresh_for_persona_at; +#[cfg(test)] +pub(crate) use pending::REFRESH_READ_OBSERVER; pub use sharing::set_team_shared; /// Refresh the shared 30178 catalog heads of every team that includes diff --git a/desktop/src-tauri/src/commands/teams/pending.rs b/desktop/src-tauri/src/commands/teams/pending.rs index 9967e590fb7..a3e82329fbc 100644 --- a/desktop/src-tauri/src/commands/teams/pending.rs +++ b/desktop/src-tauri/src/commands/teams/pending.rs @@ -20,6 +20,22 @@ use crate::managed_agents::{ use buzz_core_pkg::kind::KIND_TEAM_CATALOG; +/// Test-only observer called inside `refresh_or_retract_shared_head_at` after +/// it reads the existing shared kind:30178 head (the authoritative T) but +/// before it retains the refreshed head (T+1). Tests use this to inject a +/// racing write — simulating a concurrent unshare — between the read and the +/// write, and to assert that the `managed_agents_store_lock` held by +/// `publish_and_refresh_teams_at` serializes the two operations correctly. +/// +/// Moving `refresh_for_persona_at` outside the lock (mutation target) causes +/// the racing unshare to win the UPSERT after `retain_event`, so the final +/// retained head is SHARED rather than UNSHARED — the prescribed RED signal. +#[cfg(test)] +type RefreshReadObserver = Box; +#[cfg(test)] +pub(crate) static REFRESH_READ_OBSERVER: std::sync::Mutex> = + std::sync::Mutex::new(None); + /// A signed catalog head, retained and awaiting relay acceptance. /// /// Only the retained-row coordinate is carried, not the signed event itself: @@ -145,7 +161,7 @@ pub(super) fn prepare_team_publication( }) } -pub(super) fn prepare_team_publication_at( +pub(crate) fn prepare_team_publication_at( db_path: &std::path::Path, keys: &nostr::Keys, team: &TeamRecord, @@ -331,6 +347,19 @@ pub(super) fn refresh_or_retract_shared_head_at( return Ok(RefreshOrRetractOutcome::Noop); } + // Fire the test-only read observer BEFORE retaining the refreshed head. + // Tests use this to race a concurrent unshare between the authoritative + // shared-T read above and the retain below — proving that the caller's + // `managed_agents_store_lock` must span this entire read+retain sequence. + #[cfg(test)] + { + if let Ok(obs) = REFRESH_READ_OBSERVER.lock() { + if let Some(ref f) = *obs { + f(); + } + } + } + // Rebuild; on failure, purge + tombstone immediately so the stale shared // head is not left public. let rebuilt = build_team_catalog_event(team, members, true); @@ -437,14 +466,15 @@ pub(super) fn refresh_shared_team_catalog_heads_for_persona( } } -/// Testable seam for [`refresh_shared_team_catalog_heads_for_persona`]. +/// File-system seam for [`refresh_shared_team_catalog_heads_for_persona`]. /// -/// Reads teams and personas from flat JSON files in `base_dir` rather than -/// through the Tauri store. Calls the SAME `resolve_and_refresh_or_retract_at` -/// that production uses — the seam is a thin file-loading shim with no -/// independent logic. Tests therefore exercise the exact production code path. -#[cfg(test)] -pub(super) fn refresh_for_persona_at( +/// Reads teams and personas from flat JSON files in `base_dir` using the same +/// dual-store rule as boot reconciliation: legacy `personas.json` when +/// non-empty, otherwise keyless definition records from `managed-agents.json` +/// (see [`crate::event_sync::read_persona_definitions`]). This makes the seam +/// correct on both pre-fold and post-fold installs. Calls the SAME +/// `resolve_and_refresh_or_retract_at` that the AppHandle path uses. +pub(crate) fn refresh_for_persona_at( base_dir: &std::path::Path, keys: &nostr::Keys, db_path: &std::path::Path, @@ -454,15 +484,28 @@ pub(super) fn refresh_for_persona_at( let teams: Vec = read_json_store(&base_dir.join("teams.json"))?; - let personas: Vec = - read_json_store(&base_dir.join("personas.json"))?; + let personas = crate::event_sync::read_persona_definitions(base_dir)?; for team in &teams { if team.is_builtin || !team.persona_ids.iter().any(|id| id == persona_id) { continue; } - // Identical call to production — no parallel implementation. - let _ = resolve_and_refresh_or_retract_at(db_path, keys, team, &personas); + let outcome = resolve_and_refresh_or_retract_at(db_path, keys, team, &personas); + match outcome { + Ok(RefreshOrRetractOutcome::RemovalQueued { ref reason }) => { + eprintln!( + "buzz-desktop: team-catalog-refresh: retracting '{}' after persona edit — {reason}", + team.name + ); + } + Err(ref e) => { + eprintln!( + "buzz-desktop: team-catalog-refresh: '{}' after persona edit — {e}", + team.name + ); + } + _ => {} + } } Ok(()) } diff --git a/desktop/src-tauri/src/commands/teams/pending/tests.rs b/desktop/src-tauri/src/commands/teams/pending/tests.rs index 7f4d31a6535..345a7405459 100644 --- a/desktop/src-tauri/src/commands/teams/pending/tests.rs +++ b/desktop/src-tauri/src/commands/teams/pending/tests.rs @@ -827,3 +827,4 @@ fn test_all_catalog_call_paths_produce_a_dominating_tombstone() { mod cross_device; mod gate; +mod retry_refresh; diff --git a/desktop/src-tauri/src/commands/teams/pending/tests/retry_refresh.rs b/desktop/src-tauri/src/commands/teams/pending/tests/retry_refresh.rs new file mode 100644 index 00000000000..c3a98ac302b --- /dev/null +++ b/desktop/src-tauri/src/commands/teams/pending/tests/retry_refresh.rs @@ -0,0 +1,193 @@ +//! Command-path regression: `set_persona_shared` publish-retry team-refresh. +//! +//! Both variants call `publish_and_refresh_teams_at` — the extracted command +//! core that production `set_persona_shared` delegates to. They cover: +//! - Pre-fold: persona definitions in `personas.json` (legacy path). +//! - Post-fold: definitions as keyless records in `managed-agents.json`; no +//! `personas.json` present (the normal state after Phase 1A.2 fold). +//! +//! Deletion mutations: +//! - Removing the `refresh_for_persona_at` call from `publish_and_refresh_teams_at` +//! turns BOTH tests RED; restoring returns GREEN. +//! - Reverting the loader to single-store (`personas.json` only) turns only the +//! post-fold test RED, proving the dual-store fix is load-bearing. + +use super::{member, prepare_team_publication_at, scoped_db, team_with_members, write_stores}; +use crate::managed_agents::retention::{get_retained_event, open_retention_db}; +use crate::managed_agents::{AgentDefinition, TeamRecord}; +use buzz_core_pkg::kind::KIND_TEAM_CATALOG; + +fn post_fold_write_stores( + base_dir: &std::path::Path, + teams: &[TeamRecord], + personas: &[AgentDefinition], +) { + std::fs::write( + base_dir.join("teams.json"), + serde_json::to_string(teams).unwrap(), + ) + .unwrap(); + // Post-fold: definitions live as keyless records in managed-agents.json. + // personas.json is absent (retired by the fold migration). + let records: Vec = personas + .iter() + .cloned() + .map(|p| p.into_agent_record()) + .collect(); + std::fs::write( + base_dir.join("managed-agents.json"), + serde_json::to_string(&records).unwrap(), + ) + .unwrap(); + assert!( + !base_dir.join("personas.json").exists(), + "post-fold fixture must not have personas.json" + ); +} + +async fn run_publish_and_refresh_for_team( + dir: &std::path::Path, + db_path: &std::path::Path, + keys: &nostr::Keys, + persona_def: &AgentDefinition, + persona_id: &str, +) { + let (event, retained, persona) = crate::commands::personas::prepare_persona_publication_at( + db_path, + keys, + persona_def, + Some(true), + ) + .unwrap(); + let prepared = crate::commands::personas::PreparedPersonaPublication { + scope: crate::managed_agents::retention::RetentionScope { + db_path: db_path.to_path_buf(), + relay_url: "http://127.0.0.1:1".to_string(), + owner_keys: keys.clone(), + }, + event, + retained, + persona, + }; + let state = crate::app_state::build_app_state(); + crate::commands::personas::publish_and_refresh_teams_at( + &state, prepared, dir, keys, db_path, persona_id, + ) + .await + .unwrap(); +} + +/// Pre-fold variant: definitions in `personas.json`. +#[tokio::test] +async fn test_set_persona_shared_publish_retry_refreshes_shared_team_head_pre_fold() { + let dir = tempfile::tempdir().unwrap(); + let keys = nostr::Keys::generate(); + let owner = keys.public_key().to_hex(); + let db_path = scoped_db(dir.path(), "wss://a.example", &owner); + let m1_before = member("m1", "Original prompt."); + let t = team_with_members("team-retry", "Retry Team", vec!["m1".to_string()]); + prepare_team_publication_at( + &db_path, + &keys, + &t, + std::slice::from_ref(&m1_before), + Some(true), + ) + .unwrap(); + let head_before = { + let conn = open_retention_db(&db_path).unwrap(); + get_retained_event(&conn, KIND_TEAM_CATALOG, &owner, "team-retry") + .unwrap() + .expect("shared head must exist before retry") + }; + assert!(head_before.content.contains("Original prompt.")); + + let m1_after = member("m1", "Updated prompt after publish-retry."); + write_stores( + dir.path(), + std::slice::from_ref(&t), + std::slice::from_ref(&m1_after), + ); + run_publish_and_refresh_for_team( + dir.path(), + &db_path, + &keys, + &member("m1", "Updated prompt after publish-retry."), + "m1", + ) + .await; + + let head_after = { + let conn = open_retention_db(&db_path).unwrap(); + get_retained_event(&conn, KIND_TEAM_CATALOG, &owner, "team-retry") + .unwrap() + .expect("shared head must still exist after retry-path refresh") + }; + assert!( + head_after + .content + .contains("Updated prompt after publish-retry."), + "team 30178 must reflect updated persona content (pre-fold)" + ); + assert!(head_after.pending_sync, "refreshed head must be queued"); + assert!(head_after.created_at >= head_before.created_at); +} + +/// Post-fold variant: definitions as keyless records in `managed-agents.json`; +/// `personas.json` absent. Reverting the loader to `personas.json` only turns +/// this test RED (zero definitions → retraction instead of refresh). +#[tokio::test] +async fn test_set_persona_shared_publish_retry_refreshes_shared_team_head_post_fold() { + let dir = tempfile::tempdir().unwrap(); + let keys = nostr::Keys::generate(); + let owner = keys.public_key().to_hex(); + let db_path = scoped_db(dir.path(), "wss://a.example", &owner); + let m1_before = member("m1", "Original prompt."); + let t = team_with_members("team-retry", "Retry Team", vec!["m1".to_string()]); + prepare_team_publication_at( + &db_path, + &keys, + &t, + std::slice::from_ref(&m1_before), + Some(true), + ) + .unwrap(); + let head_before = { + let conn = open_retention_db(&db_path).unwrap(); + get_retained_event(&conn, KIND_TEAM_CATALOG, &owner, "team-retry") + .unwrap() + .expect("shared head must exist before retry") + }; + assert!(head_before.content.contains("Original prompt.")); + + let m1_after = member("m1", "Updated prompt after publish-retry."); + post_fold_write_stores( + dir.path(), + std::slice::from_ref(&t), + std::slice::from_ref(&m1_after), + ); + run_publish_and_refresh_for_team( + dir.path(), + &db_path, + &keys, + &member("m1", "Updated prompt after publish-retry."), + "m1", + ) + .await; + + // The 30178 team head must reflect the updated prompt — not be retracted. + let head_after = { + let conn = open_retention_db(&db_path).unwrap(); + get_retained_event(&conn, KIND_TEAM_CATALOG, &owner, "team-retry") + .unwrap() + .expect("shared head must exist after post-fold retry — not retracted") + }; + assert!( + head_after + .content + .contains("Updated prompt after publish-retry."), + "team 30178 must reflect updated persona content (post-fold, managed-agents.json)" + ); + assert!(head_after.pending_sync, "refreshed head must be queued"); + assert!(head_after.created_at >= head_before.created_at); +} diff --git a/desktop/src-tauri/src/egress_guard_tests.rs b/desktop/src-tauri/src/egress_guard_tests.rs index 29e74cfb506..ec96acec92b 100644 --- a/desktop/src-tauri/src/egress_guard_tests.rs +++ b/desktop/src-tauri/src/egress_guard_tests.rs @@ -274,9 +274,17 @@ const EVENTS_INVENTORY: &[(&str, usize, usize)] = &[ ("src/archive/mod_tests.rs", 1, 0), ("src/managed_agents/persona_events/tests.rs", 1, 0), ("src/commands/team_snapshot/tests.rs", 1, 0), - // Mock-relay route in its in-file tests; production publish goes through - // the guarded boundary-1 funnel (`submit_signed_event_at_with_keys`). - ("src/commands/personas/sharing.rs", 1, 0), + // Mock-relay routes in its in-file tests (one in the existing spawn_relay + // fixture and one in the new P2 counter-server fixture); production publish + // goes through the guarded boundary-1 funnel (`submit_signed_event_at_with_keys`). + ("src/commands/personas/sharing.rs", 2, 0), + // Counter-server fixture in the P2 relay-sync regression test; production + // publish goes through the guarded boundary-1 funnel. + ( + "src/commands/personas/update/concurrent_edit_tests.rs", + 1, + 0, + ), // Loopback submit relay in `identity_archive.rs`'s in-file regen tests; // production archive/unarchive publish through the guarded boundary-1 // funnel via `submit_event`. diff --git a/desktop/src-tauri/src/event_sync.rs b/desktop/src-tauri/src/event_sync.rs index ed5b9510952..27ff2feb1b9 100644 --- a/desktop/src-tauri/src/event_sync.rs +++ b/desktop/src-tauri/src/event_sync.rs @@ -752,8 +752,7 @@ fn read_json_store(path: &Path) -> Result } /// Test-accessible alias for `read_json_store`, used by the `pending` module's -/// `refresh_for_persona_at` testable seam without re-exporting the private fn. -#[cfg(test)] +/// `refresh_for_persona_at` seam without re-exporting the private fn. pub(crate) fn read_json_store_pub( path: &Path, ) -> Result, String> { @@ -767,7 +766,7 @@ pub(crate) fn read_json_store_pub( /// agent store; `personas.json` survives only on a boot where the fold /// errored. Both callers must read the same set — a reconcile that saw an /// empty persona list would conclude every team's members were deleted. -fn read_persona_definitions( +pub(crate) fn read_persona_definitions( base_dir: &Path, ) -> Result, String> { let personas: Vec = diff --git a/desktop/src-tauri/src/managed_agents/runtime.rs b/desktop/src-tauri/src/managed_agents/runtime.rs index b10271ea1c7..bc70d68d0bb 100644 --- a/desktop/src-tauri/src/managed_agents/runtime.rs +++ b/desktop/src-tauri/src/managed_agents/runtime.rs @@ -331,6 +331,7 @@ pub fn build_managed_agent_summary( log_path, respond_to: record.respond_to, respond_to_allowlist: record.respond_to_allowlist.clone(), + effort_level: record.effort_level.clone(), }) } diff --git a/desktop/src-tauri/src/managed_agents/types.rs b/desktop/src-tauri/src/managed_agents/types.rs index 2620f0337fc..1c07909a916 100644 --- a/desktop/src-tauri/src/managed_agents/types.rs +++ b/desktop/src-tauri/src/managed_agents/types.rs @@ -586,6 +586,12 @@ pub struct ManagedAgentSummary { pub log_path: String, pub respond_to: RespondTo, pub respond_to_allowlist: Vec, + /// Canonical harness-agnostic effort level persisted on the record. + /// `None` means the agent uses the adapter default at launch. + /// Exposed here so the edit dialog's settlement comparator can detect a + /// backend-rejected effort write rather than closing as success. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub effort_level: Option, } #[derive(Debug, Serialize)] diff --git a/desktop/src-tauri/src/managed_agents/types/requests.rs b/desktop/src-tauri/src/managed_agents/types/requests.rs index 824ca4ccf3a..fb97beff810 100644 --- a/desktop/src-tauri/src/managed_agents/types/requests.rs +++ b/desktop/src-tauri/src/managed_agents/types/requests.rs @@ -132,6 +132,14 @@ pub struct UpdatePersonaRequest { /// present = validate and replace the fields as a unit. #[serde(default)] pub behavior: Option, + /// Seed-time definition revision (`updated_at`) captured when the editor + /// opened. When present, `update_persona_with` performs a lock-held + /// compare-and-swap: if the persisted persona's `updated_at` no longer + /// matches, the save is rejected before any write so a stale + /// full-replacement input cannot clobber a newer concurrent writer. Absent + /// (legacy callers, instance-only saves) skips the check. + #[serde(default)] + pub expected_updated_at: Option, } #[derive(Debug, Deserialize)] diff --git a/desktop/src-tauri/src/managed_agents/types/tests.rs b/desktop/src-tauri/src/managed_agents/types/tests.rs index 0918ab2c65c..077ba6ff02f 100644 --- a/desktop/src-tauri/src/managed_agents/types/tests.rs +++ b/desktop/src-tauri/src/managed_agents/types/tests.rs @@ -761,6 +761,7 @@ fn summary_fixture( log_path: String::new(), respond_to: RespondTo::OwnerOnly, respond_to_allowlist: Vec::new(), + effort_level: None, } } diff --git a/desktop/src/features/agents/AGENTS.md b/desktop/src/features/agents/AGENTS.md index a4dd4b346d2..9b05b70beea 100644 --- a/desktop/src/features/agents/AGENTS.md +++ b/desktop/src/features/agents/AGENTS.md @@ -153,7 +153,7 @@ with a TypeScript lookup table or an id comparison in a component. place that resolves it for dialog surfaces and publishes it through `ui/AgentRunLocationContext.tsx`; the field reads that context and lets an explicit `runLocation` prop win. Do **not** thread the value as a prop - through `AgentDefinitionDialog` / `AgentInstanceEditDialog` — neither uses + through `AgentDefinitionDialog` / `AgentEditMergedDialog` — neither uses the value itself, and the shared context keeps the dialog boundary stable. Surfaces rendered outside `AgentDialog` (e.g. `EditRespondToDialog`) pass the prop directly. Local names "your diff --git a/desktop/src/features/agents/agentManagement.ts b/desktop/src/features/agents/agentManagement.ts index 5b5e18d8727..79d37744cdb 100644 --- a/desktop/src/features/agents/agentManagement.ts +++ b/desktop/src/features/agents/agentManagement.ts @@ -148,3 +148,35 @@ export function createInputFromRequest( systemPrompt: request.request.systemPrompt, }; } + +/** Definition-field pre-fill (R6 review mode) carried into the merged edit dialog. */ +export type AgentReviewOverrides = Partial<{ + displayName: string; + systemPrompt: string; + runtime: string | undefined; + provider: string | undefined; + model: string | undefined; + respondTo: string | undefined; +}>; + +/** + * Maps an agent-origin update request to the definition-field overrides the + * merged edit dialog pre-fills in R6 owner-review mode. Only fields the agent + * actually requested are carried; an empty result yields `undefined` so the + * dialog seeds purely from the definition. + */ +export function reviewOverridesForUpdate( + request: Extract["request"], +): AgentReviewOverrides | undefined { + const overrides: AgentReviewOverrides = {}; + if (request.displayName != null) overrides.displayName = request.displayName; + if (request.systemPrompt != null) + overrides.systemPrompt = request.systemPrompt; + if (request.runtime != null) overrides.runtime = request.runtime; + if (request.provider != null) overrides.provider = request.provider; + if (request.model != null) overrides.model = request.model; + // Carry agent-requested respondTo into definition-only review mode so the + // owner sees and can modify the very access change under review. + if (request.respondTo != null) overrides.respondTo = request.respondTo; + return Object.keys(overrides).length > 0 ? overrides : undefined; +} diff --git a/desktop/src/features/agents/ui/AgentCreationPreview.tsx b/desktop/src/features/agents/ui/AgentCreationPreview.tsx index 2856f56e789..a9363e8fa96 100644 --- a/desktop/src/features/agents/ui/AgentCreationPreview.tsx +++ b/desktop/src/features/agents/ui/AgentCreationPreview.tsx @@ -63,7 +63,7 @@ export function AgentCreationPreview({ disabled?: boolean; /** When true, omit all upload/edit controls and render the avatar as a * plain display element. Use in contexts where avatar editing is - * handled by an external affordance (e.g. AgentInstanceEditDialog). */ + * handled externally (e.g. definition-only via AgentDefinitionDialog). */ hideEditControl?: boolean; label: string; onClearAvatar?: () => void; diff --git a/desktop/src/features/agents/ui/AgentDefinitionDialog.tsx b/desktop/src/features/agents/ui/AgentDefinitionDialog.tsx index c0968ad0e9d..2da68fa3ce2 100644 --- a/desktop/src/features/agents/ui/AgentDefinitionDialog.tsx +++ b/desktop/src/features/agents/ui/AgentDefinitionDialog.tsx @@ -161,15 +161,9 @@ export function AgentDefinitionDialog({ // The seed the draft is diffed against at submit: an untouched quad // submits no behavior group, keeping unrelated edits hash-quiet. const behaviorSeedRef = React.useRef(emptyPersonaBehaviorDraft); - // Tracks when the runtime was auto-seeded by the default-runtime effect in - // edit mode (i.e. the user never explicitly chose a runtime). Used to omit - // the seeded runtime from the submit payload for builtin definitions whose - // canonical runtime is null — the sync would revert it anyway. + // Tracks when the runtime was auto-seeded (not an explicit user choice). const isRuntimeAutoSeededRef = React.useRef(false); // Guards the seeding effect so it fires at most once per dialog-open. - // Without this, clearing runtime back to "" via "No preference" would re- - // trigger the effect (the `runtime` dep would pass the length guard) and - // snap the dropdown back to the default — an edit-mode regression. const hasSeededForOpenRef = React.useRef(false); const [showAdvancedFields, setShowAdvancedFields] = React.useState(false); const [isAvatarUploadPending, setIsAvatarUploadPending] = @@ -514,7 +508,7 @@ export function AgentDefinitionDialog({ !isAvatarUploadPending; // Merge global env as the base layer so credential keys satisfied via global - // config are available to model discovery — same rationale as in AgentInstanceEditDialog. + // config are available to model discovery — same rationale as in AgentEditMergedDialog. const envVarsForDiscovery = React.useMemo( () => ({ ...globalConfig.env_vars, ...envVars }), [globalConfig.env_vars, envVars], diff --git a/desktop/src/features/agents/ui/AgentDialog.tsx b/desktop/src/features/agents/ui/AgentDialog.tsx index 309bd3cdc72..c00cfe6d253 100644 --- a/desktop/src/features/agents/ui/AgentDialog.tsx +++ b/desktop/src/features/agents/ui/AgentDialog.tsx @@ -3,18 +3,12 @@ import * as React from "react"; import type { AcpRuntimeCatalogEntry, CreatePersonaInput, - ManagedAgent, UpdatePersonaInput, } from "@/shared/api/types"; -import { - runLocationForBackend, - runLocationForRunOn, -} from "../lib/agentAccessWarning"; +import { runLocationForRunOn } from "../lib/agentAccessWarning"; import { AgentRunLocationProvider } from "./AgentRunLocationContext"; import type { BackendIntent } from "../lib/instanceInputForDefinition"; import type { AgentCreateIntent } from "./agentCreateIntent"; -import type { EditAgentFocusTarget } from "@/features/agents/openEditAgentEvent"; -import { AgentInstanceEditDialog } from "./AgentInstanceEditDialog"; import { createPersonaDialogState } from "./personaDialogState"; import { AgentDefinitionDialog, @@ -45,22 +39,6 @@ type AgentDialogCreateProps = { ) => Promise; }; -type AgentDialogInstanceEditProps = { - mode: "instance-edit"; - agent: ManagedAgent; - open: boolean; - onOpenChange: (open: boolean) => void; - onUpdated?: (agent: ManagedAgent) => void; - initialFocus?: EditAgentFocusTarget; - /** - * Called when the user clicks "Edit avatar" inside the instance-edit dialog. - * Caller (UserProfilePanel) is responsible for closing this dialog and - * opening the definition-edit dialog. Only passed when the linked definition - * is editable (non-built-in, resolved). - */ - onEditLinkedPersona?: () => void; -}; - type AgentDialogDefinitionEditProps = { mode: "definition-edit"; open: boolean; @@ -80,39 +58,17 @@ type AgentDialogDefinitionEditProps = { publishCatalogUpdatesOnSave?: boolean; }; -type AgentDialogProps = - | AgentDialogCreateProps - | AgentDialogInstanceEditProps - | AgentDialogDefinitionEditProps; +type AgentDialogProps = AgentDialogCreateProps | AgentDialogDefinitionEditProps; /** * Unified entry point (Phase 1B.2/1B.3b/1B.3c): routes an intent to the form * that owns it. The definition family renders AgentDefinitionDialog — create * mode always starts the agent and includes a WhereToRunSection; * definition-edit passes the caller's PersonaDialogState-derived props - * through unchanged (edit/duplicate/import). instance-edit renders - * AgentInstanceEditDialog (persistent mount + `open` toggle — its reset - * lifecycle is keyed on [open, agent.pubkey]). + * through unchanged (duplicate/import). Edit routes use AgentEditDialog + * → AgentEditMergedDialog (one merged surface for all R1–R7 contexts). */ export function AgentDialog(props: AgentDialogProps) { - if (props.mode === "instance-edit") { - return ( - // A running instance knows its own backend, so the respond-to warning can - // name the machine it will actually run on. - - - - ); - } if (props.mode === "definition-edit") { // A definition has no instance and no run draft, so the run location stays // unknown and the warning uses its local-wording fallback. diff --git a/desktop/src/features/agents/ui/AgentEditDialog.tsx b/desktop/src/features/agents/ui/AgentEditDialog.tsx new file mode 100644 index 00000000000..e2d77cfdfaf --- /dev/null +++ b/desktop/src/features/agents/ui/AgentEditDialog.tsx @@ -0,0 +1,112 @@ +/** + * AgentEditDialog.tsx — Merged agent edit surface entry point (Phase 1 Round 2). + * + * Collapses R1–R7 onto a single dialog: AgentEditMergedDialog. + * + * All contexts — instance-with-definition, instance-only, definition-only — + * render the ONE merged surface. AgentEditMergedDialog seeds from + * AgentFormModel, renders D+I+L sections per context, and submits every + * save through the Artifact 3 coordinator (observed-state settlement for D, + * I, and per-policy writes). + * + * AgentDefinitionDialog survives for create flows (R8 duplicate seed, import) + * only — it is NOT a consumer of any R1–R7 edit route. + * AgentInstanceEditDialog has no production consumer on any R1–R7 route. + */ + +import type { ManagedAgent } from "@/shared/api/types"; +import type { EditAgentFocusTarget } from "@/features/agents/openEditAgentEvent"; +import type { AgentEditContext } from "./agentFormModel"; +export type { AgentEditContext }; +import { runLocationForBackend } from "@/features/agents/lib/agentAccessWarning"; +import { AgentRunLocationProvider } from "./AgentRunLocationContext"; +import { AgentEditMergedDialog } from "./AgentEditMergedDialog"; + +// ── Types ───────────────────────────────────────────────────────────────────── + +export type AgentEditDialogProps = { + open: boolean; + onOpenChange: (open: boolean) => void; + /** Edit context: which entity or entities to edit. */ + ctx: AgentEditContext; + /** + * Optional field to focus when the dialog opens from a card deep-link. + */ + initialFocus?: EditAgentFocusTarget; + onUpdated?: (agent: ManagedAgent) => void; + /** + * Optional pre-save validator (R6 origin permission check). + * Return a non-null string to abort with an error toast; return null to proceed. + */ + onValidate?: () => string | null; + /** + * Optional initial-values override for definition fields (R6 review mode). + */ + initialValueOverrides?: Partial<{ + displayName: string; + systemPrompt: string; + runtime: string | undefined; + provider: string | undefined; + model: string | undefined; + respondTo: string | undefined; + }>; + /** + * Definition admin actions forwarded to AgentEditMergedDialog (Artifact 4). + * Omit when the host does not own those actions. + */ + onDeleteDefinition?: () => void; + onRemoveFromMyAgents?: () => void; + onShare?: () => void; + linkedInstanceCount?: number; + isIdentityArchived?: boolean; +}; + +// ── Component ───────────────────────────────────────────────────────────────── + +export function AgentEditDialog({ + ctx, + open, + onOpenChange, + onUpdated, + onValidate, + initialValueOverrides, + initialFocus, + onDeleteDefinition, + onRemoveFromMyAgents, + onShare, + linkedInstanceCount, + isIdentityArchived, +}: AgentEditDialogProps) { + // All contexts route to the merged surface. The run-location provider wraps + // instance-present paths so RespondToField's remote-backend warning can name + // the machine the agent actually runs on. + const inst = ctx.kind !== "definition-only" ? ctx.instance : null; + const runLocation = inst ? runLocationForBackend(inst.backend) : null; + + const inner = ( + + ); + + if (inst) { + return ( + + {inner} + + ); + } + + return inner; +} diff --git a/desktop/src/features/agents/ui/AgentEditMergedDialog.tsx b/desktop/src/features/agents/ui/AgentEditMergedDialog.tsx new file mode 100644 index 00000000000..972056d98dc --- /dev/null +++ b/desktop/src/features/agents/ui/AgentEditMergedDialog.tsx @@ -0,0 +1,1021 @@ +/** + * AgentEditMergedDialog — single merged edit surface for R1–R7. + * + * instance-with-definition → D + I + L sections on one form + * instance-only → I + L sections + * definition-only → D sections only + * + * Submit: emitAgentFormDiff → runAgentSaveCoordinator (observed-state settlement). + * Submit logic extracted to useAgentEditMergedSubmit; section JSX to + * AgentEditMergedDialogDSection / AgentEditMergedDialogInstanceSection. + */ + +import * as React from "react"; + +import { + useAcpRuntimesQuery, + useAgentConfigSurface, + useBakedBuildEnvKeysQuery, + usePersonasQuery, + useStartManagedAgentMutation, + useUpdateManagedAgentMutation, + useUpdatePersonaMutation, +} from "@/features/agents/hooks"; +import { useTeamsQuery } from "@/features/agents/teamHooks"; +import { useUpdatePersonaAndPublishMutation } from "@/features/agents/lib/usePersonaCatalogRelay"; +import type { ManagedAgent, RespondToMode } from "@/shared/api/types"; +import type { EditAgentFocusTarget } from "@/features/agents/openEditAgentEvent"; +import { Button } from "@/shared/ui/button"; +import { ChooserDialogContent } from "@/shared/ui/chooser-dialog-content"; +import { Dialog } from "@/shared/ui/dialog"; + +import { + seedAgentFormModel, + hasDefinitionContext, + hasInstanceContext, + isDefinitionReadOnly, + definitionFieldsDirty, + fieldEditable, + editContextDefinition, + editContextInstance, + type AgentEditContext, +} from "./agentFormModel"; +import { + useAgentEditMergedSubmit, + buildNextAgentFormModel, + type AgentEditSubmitState, +} from "./useAgentEditMergedSubmit"; +import { AgentEditMergedDSection } from "./AgentEditMergedDialogDSection"; +import { AgentEditMergedInstanceSection } from "./AgentEditMergedDialogInstanceSection"; +import { + getDefaultPersonaRuntime, + isMissingRequiredDropdownField, +} from "./agentConfigOptions"; +import { useAgentEditRuntimeHandlers } from "./useAgentEditRuntimeHandlers"; +import { AgentCreationPreview } from "./AgentCreationPreview"; +import { AddCustomHarnessDialog } from "./AddCustomHarnessDialog"; +import { useAgentAccessOwnerOnlyQuery } from "@/features/agents/useAgentAccessOwnerOnly"; +import { isEditAgentProviderSaveValid } from "./personaRuntimeModel"; +import { + agentAiConfigurationModeSatisfied, + initialAgentAiConfigurationMode, +} from "./agentAiConfigurationPolicy"; +import type { EnvVarsValue } from "./EnvVarsEditor"; +import { formatPersonaNamePoolText } from "./personaDialogState"; +import { useAgentEditRuntimeState } from "./useAgentEditRuntimeState"; +import { useAgentEditDeepLinkFocus } from "./useAgentEditDeepLinkFocus"; + +// ── Types / Component ──────────────────────────────────────────────────────── + +export type AgentEditMergedDialogProps = { + open: boolean; + onOpenChange: (open: boolean) => void; + ctx: AgentEditContext; + /** Optional field to focus when the dialog opens from a card deep-link. */ + initialFocus?: EditAgentFocusTarget; + onUpdated?: (agent: ManagedAgent) => void; + /** + * Optional pre-save validator (R6 origin permission check). + * Return a non-null string to abort with an error toast; return null to proceed. + */ + onValidate?: () => string | null; + /** + * Optional initial-values override for definition fields (R6 review mode). + */ + initialValueOverrides?: Partial<{ + displayName: string; + systemPrompt: string; + runtime: string | undefined; + provider: string | undefined; + model: string | undefined; + respondTo: string | undefined; + }>; + /** + * Definition admin actions (Definition admin section, Artifact 4). + * These are optional — omit when the host doesn't support the action. + */ + /** + * Called to delete the definition (custom definition delete). + * When provided, a "Delete agent" action renders in the definition admin section. + */ + onDeleteDefinition?: () => void; + /** + * For built-in definitions: called to remove from My Agents (14a deactivate). + * When provided and definition isBuiltIn, "Remove from My Agents" renders. + */ + onRemoveFromMyAgents?: () => void; + /** + * Called to open the share/catalog dialog (row 15 — not shown for built-ins). + * When provided, a "Share agent" action renders in the definition admin section. + */ + onShare?: () => void; + /** + * Number of linked instances for blast-radius copy in the delete confirmation. + * Only relevant when onDeleteDefinition is provided. + */ + linkedInstanceCount?: number; + /** + * Whether the definition's identity is archived (14b). When true, an + * "Archived" flair renders. Save does not unarchive. + */ + isIdentityArchived?: boolean; +}; + +export function AgentEditMergedDialog({ + open, + onOpenChange, + ctx, + initialFocus, + onUpdated, + onValidate, + initialValueOverrides, + onDeleteDefinition, + onRemoveFromMyAgents, + onShare, + linkedInstanceCount = 0, + isIdentityArchived = false, +}: AgentEditMergedDialogProps) { + const def = editContextDefinition(ctx); + const inst = editContextInstance(ctx); + const showDef = hasDefinitionContext(ctx); + const showInst = hasInstanceContext(ctx); + const defReadOnly = isDefinitionReadOnly(ctx); + + // ── Queries / mutations ─────────────────────────────────────────────────── + const runtimesQuery = useAcpRuntimesQuery({ enabled: open }); + const runtimes = runtimesQuery.data ?? []; + const runtimeCatalogStatus = runtimesQuery.isLoading + ? ("loading" as const) + : runtimesQuery.isError + ? ("error" as const) + : ("ready" as const); + + const updatePersonaMutation = useUpdatePersonaMutation(); + const updatePersonaAndPublishMutation = + useUpdatePersonaAndPublishMutation(null); + const updateMutation = useUpdateManagedAgentMutation(); + const startMutation = useStartManagedAgentMutation(); + + const { data: bakedEnvKeys } = useBakedBuildEnvKeysQuery({ enabled: open }); + // Effort write control (ported from #4557): the config surface supplies the + // discovered effort configId/options and current effort tier. Gated on an + // open dialog with a present instance — effort is a per-instance local write. + const configSurfaceQuery = useAgentConfigSurface( + open && inst ? inst.pubkey : null, + ); + const { data: agentAccessOwnerOnly } = useAgentAccessOwnerOnlyQuery({ + // Query in both instance and definition-only contexts: rows 9–10 say + // access/parallelism are D-owned in definition-only (shown on the form). + enabled: open && (showInst || ctx.kind === "definition-only"), + }); + + const personasQuery = usePersonasQuery(); + const teamsQuery = useTeamsQuery(); + const sourceTeamName = React.useMemo(() => { + if (!def?.sourceTeam) return null; + const team = teamsQuery.data?.find((t) => t.id === def.sourceTeam); + return team?.name ?? def.sourceTeam; + }, [def?.sourceTeam, teamsQuery.data]); + const linkedPersona = React.useMemo( + () => + inst?.personaId + ? (personasQuery.data?.find((p) => p.id === inst.personaId) ?? null) + : null, + [inst?.personaId, personasQuery.data], + ); + + // ── Form state — seeded from AgentFormModel ─────────────────────────────── + // D-fields + const [displayName, setDisplayName] = React.useState(""); + const [avatarUrl, setAvatarUrl] = React.useState(""); + const [description, setDescription] = React.useState(""); + const [systemPrompt, setSystemPrompt] = React.useState(""); + const [namePoolText, setNamePoolText] = React.useState(""); + // Definition runtime (D-field) — separate from instance harness pin (I-field) + const [definitionRuntimeId, setDefinitionRuntimeId] = + React.useState("custom"); + // Track whether the D-runtime was auto-seeded (not a deliberate user choice). + // Prevents a no-op save from persisting the app default as the definition runtime. + const autoSeededDefinitionRuntimeRef = React.useRef(null); + // Definition `updatedAt` captured at seed time. The submit-path concurrent-edit + // guard compares this against the latest ctx definition: if another writer + // revised the definition while this form was open, the stale full-replacement + // input would clobber their values, so the save aborts before any write. + const seededDefinitionUpdatedAtRef = React.useRef(null); + // I-fields — harness pin state (I-section only, independent of D-runtime) + const [selectedRuntimeId, setSelectedRuntimeId] = React.useState("custom"); + // D-section LLM model/provider — for the definition (D-owned) + const [dModel, setDModel] = React.useState(""); + const [dProvider, setDProvider] = React.useState(""); + const [dIsCustomModelEditing, setDIsCustomModelEditing] = + React.useState(false); + const [dIsCustomProviderEditing, setDIsCustomProviderEditing] = + React.useState(false); + // I-section LLM model/provider — for unlinked agents (I-owned, shown only when !showDef) + const [iModel, setIModel] = React.useState(""); + const [iProvider, setIProvider] = React.useState(""); + const [iIsCustomModelEditing, setIIsCustomModelEditing] = + React.useState(false); + const [iIsCustomProviderEditing, setIIsCustomProviderEditing] = + React.useState(false); + const runtimeTouched = React.useRef(false); + // Effort picker is Save-gated: hold pending selection in dialog state. + // `effortTouched` distinguishes "user picked a value" from "seeded display". + const [effortLevel, setEffortLevel] = React.useState(null); + const effortTouched = React.useRef(false); + + // Convenience: the active model/provider — D-state when a definition is present, + // I-state when instance-only (no definition). Used for the D-section and for + // useAgentEditRuntimeState, which is always driven by the relevant layer. + const model = showDef ? dModel : iModel; + const provider = showDef ? dProvider : iProvider; + const isCustomModelEditing = showDef + ? dIsCustomModelEditing + : iIsCustomModelEditing; + const isCustomProviderEditing = showDef + ? dIsCustomProviderEditing + : iIsCustomProviderEditing; + + // I-fields + const [instanceName, setInstanceName] = React.useState(""); + const [respondTo, setRespondTo] = React.useState( + inst?.respondTo ?? null, + ); + const [respondToAllowlist, setRespondToAllowlist] = React.useState( + inst?.respondToAllowlist ?? [], + ); + const [parallelism, setParallelism] = React.useState( + String(inst?.parallelism ?? 1), + ); + const [envVars, setEnvVars] = React.useState( + def?.envVars ?? inst?.envVars ?? {}, + ); + const [instanceEnvVars, setInstanceEnvVars] = React.useState( + inst?.envVars ?? {}, + ); + // Harness-pin (I-field) + const [agentCommand, setAgentCommand] = React.useState( + inst?.agentCommand ?? "", + ); + const [inheritHarness, setInheritHarness] = React.useState( + inst != null && inst.personaId != null && inst.agentCommandOverride == null, + ); + const [agentArgs, setAgentArgs] = React.useState( + inst?.agentArgs.join(",") ?? "", + ); + const [acpCommand, setAcpCommand] = React.useState(inst?.acpCommand ?? ""); + const [originalAgentCommand, setOriginalAgentCommand] = React.useState( + inst?.agentCommand ?? "", + ); + + // L-fields + const [autoRestartOnConfigChange, setAutoRestartOnConfigChange] = + React.useState(inst?.autoRestartOnConfigChange ?? false); + const [startOnAppLaunch, setStartOnAppLaunch] = React.useState( + inst?.startOnAppLaunch ?? false, + ); + + // UI state + const [showAdvancedFields, setShowAdvancedFields] = React.useState(false); + const [isAvatarUploadPending, setIsAvatarUploadPending] = + React.useState(false); + const [aiDefaultsOpen, setAiDefaultsOpen] = React.useState(false); + const aiDefaultsTriggerRef = React.useRef(null); + const [isAddHarnessOpen, setIsAddHarnessOpen] = React.useState(false); + + // ── Seed form on open ───────────────────────────────────────────────────── + // biome-ignore lint/correctness/useExhaustiveDependencies: intentional — only re-seed on open/entity-switch + React.useEffect(() => { + if (!open) return; + + const seed = seedAgentFormModel(ctx); + setDisplayName(seed.displayName); + setAvatarUrl(seed.avatarUrl); + setDescription(seed.description); + setSystemPrompt(seed.systemPrompt); + setNamePoolText(formatPersonaNamePoolText(seed.namePool ?? [])); + // D-section model/provider — seed from definition (or instance for I-only) + setDModel(seed.model ?? ""); + setDProvider(seed.provider ?? ""); + setDIsCustomModelEditing(false); + setDIsCustomProviderEditing(false); + // I-section model/provider — only relevant for instance-only context + setIModel(seed.model ?? ""); + setIProvider(seed.provider ?? ""); + setIIsCustomModelEditing(false); + setIIsCustomProviderEditing(false); + // Rows 9–10: seed respondTo/allowlist/parallelism from the canonical seed. + // Preserve null from the seed — do NOT coerce null→"anyone" here; the + // seed already handles the instance-vs-definition context. Coercing here + // would cause phantom D-writes when an unset definition behavior opens. + setRespondTo((seed.respondTo as RespondToMode | null) ?? null); + setRespondToAllowlist(seed.respondToAllowlist); + setParallelism(seed.parallelism != null ? String(seed.parallelism) : ""); + setEnvVars(seed.envVars); + setInstanceEnvVars(seed.instanceEnvVars ?? {}); + setInstanceName(seed.instanceName ?? ""); + setAutoRestartOnConfigChange(seed.autoRestartOnConfigChange ?? false); + setStartOnAppLaunch(seed.startOnAppLaunch ?? false); + setShowAdvancedFields(false); + setIsAvatarUploadPending(false); + runtimeTouched.current = false; + setEffortLevel(null); + effortTouched.current = false; + autoSeededDefinitionRuntimeRef.current = null; + // Capture the definition revision the form is baselined against. + seededDefinitionUpdatedAtRef.current = def?.updatedAt ?? null; + + // Seed definition runtime (D-field) from definition record. + const defRuntimeId = def?.runtime?.trim() ?? ""; + setDefinitionRuntimeId(defRuntimeId || "custom"); + + if (inst) { + setAgentCommand(inst.agentCommand); + setOriginalAgentCommand(inst.agentCommand); + setInheritHarness( + inst.personaId != null && inst.agentCommandOverride == null, + ); + setAgentArgs(inst.agentArgs.join(",")); + setAcpCommand(inst.acpCommand); + // I-harness pin: match instance agentCommand to catalog entry + const matched = + runtimes.find((r) => r.command?.trim() === inst.agentCommand.trim()) ?? + runtimes.find((r) => r.id === inst.agentCommand.trim()); + setSelectedRuntimeId(matched ? matched.id : "custom"); + } else if (def) { + // definition-only: also seed I-harness selectedRuntimeId from definition for the + // D-section dropdown (no instance, so both are the same) + setSelectedRuntimeId(defRuntimeId || "custom"); + } + + // Apply any caller-supplied initial value overrides (R6 review mode). + if (initialValueOverrides) { + if (initialValueOverrides.displayName !== undefined) + setDisplayName(initialValueOverrides.displayName); + if (initialValueOverrides.systemPrompt !== undefined) + setSystemPrompt(initialValueOverrides.systemPrompt); + if (initialValueOverrides.model !== undefined) { + setDModel(initialValueOverrides.model ?? ""); + setIModel(initialValueOverrides.model ?? ""); + } + if (initialValueOverrides.provider !== undefined) { + setDProvider(initialValueOverrides.provider ?? ""); + setIProvider(initialValueOverrides.provider ?? ""); + } + if (initialValueOverrides.runtime !== undefined) + setDefinitionRuntimeId(initialValueOverrides.runtime ?? "custom"); + // R6: carry agent-requested respondTo into definition-only form. + // This makes the requested change VISIBLE in the review dialog — + // the owner sees the requested value and can accept (Save) or change it. + if (initialValueOverrides.respondTo !== undefined) + setRespondTo( + (initialValueOverrides.respondTo as RespondToMode | null) ?? null, + ); + } + }, [open, inst?.pubkey, def?.id]); + + // Auto-seed D-section runtime when the definition has no runtime configured + // and the catalog has loaded. Matches AgentDefinitionDialog's auto-seed logic. + // biome-ignore lint/correctness/useExhaustiveDependencies: intentional — only re-seed when catalog loads or dialog opens + React.useEffect(() => { + if ( + !open || + !showDef || + runtimes.length === 0 || + definitionRuntimeId !== "custom" || // already set by the user or seed + (def?.runtime?.trim() ?? "").length > 0 // definition already has a runtime + ) + return; + const defaultRuntime = getDefaultPersonaRuntime(runtimes); + if (!defaultRuntime) return; + setDefinitionRuntimeId(defaultRuntime.id); + // Record that this runtime was auto-seeded, not chosen by the user. + // useAgentEditMergedSubmit uses this to prevent persisting the auto-seed + // as a definition runtime change when no other D-fields were modified. + autoSeededDefinitionRuntimeRef.current = defaultRuntime.id; + // In definition-only context, selectedRuntimeId is the same conceptual field + // as definitionRuntimeId (there is no separate I-harness pin). Sync it so + // that model discovery runs: useAgentEditRuntimeState derives `selectedRuntime` + // from `selectedRuntimeId`, and a stale "custom" value leaves discovery unable + // to resolve a runtime → model list stays empty. + if (!inst) { + setSelectedRuntimeId(defaultRuntime.id); + } + }, [open, runtimes.length, showDef]); + + // Re-derive runtime id when catalog loads + React.useEffect(() => { + if (!open || runtimeTouched.current || runtimes.length === 0 || !inst) + return; + const matched = + runtimes.find((r) => r.command?.trim() === inst.agentCommand.trim()) ?? + runtimes.find((r) => r.id === inst.agentCommand.trim()); + if (matched) setSelectedRuntimeId(matched.id); + }, [open, runtimes, inst?.agentCommand, inst]); + + // ── Runtime / model / provider machinery ────────────────────────────────── + const { + selectedRuntime, + runtimeDropdownValue, + defRuntimeDropdownValue, + instanceRuntimeDropdownOptions, + defRuntimeDropdownOptions, + defBlankLabel, + originalRuntimeSupportsProvider, + prospectiveRuntimeId, + prospectiveRuntime, + llmProviderFieldVisible, + inheritedSubmission, + inheritedModelDefault, + inheritedProviderDefault, + inheritedEnvVarsForAdvanced, + fileSatisfiedEnvKeys, + requiredEnvKeyMissing, + effectiveProvider, + providerDropdownOptions, + providerSelectValue, + modelDropdownOptions, + modelSelectValue, + showCustomModelInput, + modelStatusMessage, + modelDiscoveryLoading, + apiKeyValue, + apiKeyIsInherited, + apiKeyInheritedLabel, + apiKeyIsRequired, + topLevelSecretEnvVar, + advancedRequiredEnvKeys, + dAdvanced, + } = useAgentEditRuntimeState({ + open, + showDef, + showInst, + runtimes, + runtimeCatalogStatus, + selectedRuntimeId, + definitionRuntimeId, + model, + provider, + isCustomModelEditing, + isCustomProviderEditing, + envVars, + instanceEnvVars, + inheritHarness, + inst, + def, + linkedPersona, + bakedEnvKeys, + originalAgentCommand, + setModel: setDModel, + setIsCustomModelEditing: setDIsCustomModelEditing, + }); + + // ── Submit validity ──────────────────────────────────────────────────────── + const modelRequired = React.useMemo( + () => isMissingRequiredDropdownField(undefined, model), + [model], + ); + const providerRequired = React.useMemo( + () => isMissingRequiredDropdownField(undefined, provider), + [provider], + ); + + // Provider Save-gate. + // + // Definition-present context (D-section picker): use the AgentDefinitionDialog + // gate — when the provider picker is visible and the pair is customized + // (provider or model set), both provider AND model must be non-empty. The + // global fallback does NOT satisfy the definition gate (that is the + // instance-dialog rule), so a runtime-less definition with a saved model and + // an empty provider keeps Save blocked (test-11 regression pin from 87dc4dccba). + // + // Instance-only context: keep isEditAgentProviderSaveValid — the instance + // record inherits the global provider, so the global fallback is valid there. + const providerValid = showDef + ? agentAiConfigurationModeSatisfied( + initialAgentAiConfigurationMode({ provider, model }), + { provider, model }, + llmProviderFieldVisible, + ) + : isEditAgentProviderSaveValid({ + llmProviderFieldVisible, + currentProvider: provider, + originalProvider: inst?.provider ?? null, + globalProvider: inheritedProviderDefault.value, + originalRuntimeSupportsProvider, + }); + + const parsedParallelism = parseInt(parallelism, 10); + + // ── Submit hook (must come before canSubmit so isSaving is available) ───── + const submitState = { + ctx, + displayName, + avatarUrl, + description, + systemPrompt, + namePoolText, + model: showDef ? dModel : iModel, + provider: showDef ? dProvider : iProvider, + respondTo, + respondToAllowlist, + parallelism, + parsedParallelism, + envVars, + instanceEnvVars, + instanceName, + autoRestartOnConfigChange, + startOnAppLaunch: showInst ? startOnAppLaunch : undefined, + definitionRuntimeId, + autoSeededDefinitionRuntimeRef, + selectedRuntimeId, + inheritHarness, + agentCommand, + agentArgs, + acpCommand, + showInst, + defReadOnly, + seededDefinitionUpdatedAt: seededDefinitionUpdatedAtRef.current, + inheritedSubmissionProvider: inheritedSubmission.provider ?? null, + runtimes, + updatePersona: ( + p: Parameters[0], + ) => updatePersonaMutation.mutateAsync(p), + updatePersonaAndPublish: ( + p: Parameters[0], + ) => updatePersonaAndPublishMutation.mutateAsync(p), + updateManagedAgent: ( + input: Parameters[0], + ) => updateMutation.mutateAsync(input), + startMutate: ( + pubkey: string, + cbs: { onSuccess: () => void; onError: (err: unknown) => void }, + ) => startMutation.mutate(pubkey, cbs), + onValidate, + onOpenChange, + onUpdated, + effortLevel, + effortTouched, + originalEffortLevel: + configSurfaceQuery.data?.normalized.thinkingEffort?.value ?? null, + } satisfies AgentEditSubmitState; + + const { + isSaving, + saveError, + handleSubmit: _handleSubmit, + } = useAgentEditMergedSubmit(submitState); + + const canSubmit = + !isSaving && + !isAvatarUploadPending && + displayName.trim().length > 0 && + providerValid && + // D-side credential gate: required keys must be satisfied when the definition + // is editable. Team-managed (defReadOnly) never blocks — user can't change them. + (!showDef || defReadOnly || !dAdvanced.requiredEnvKeyMissing) && + (showInst + ? !requiredEnvKeyMissing && + (parsedParallelism > 0 || parallelism === "") && + (selectedRuntimeId !== "custom" || + inheritHarness || + agentCommand.trim().length > 0) && + // Block submission when the visible instance name is blank (pool or not). + instanceName.trim().length > 0 + : true); + + // ── Runtime/model/provider handlers (extracted to useAgentEditRuntimeHandlers) ── + const { + handleDefinitionRuntimeChange, + handleRuntimeDropdownChange, + handleProviderDropdownChange, + handleModelDropdownChange, + selectSavedHarness, + } = useAgentEditRuntimeHandlers({ + showDef, + showInst, + dProvider, + dModel, + dIsCustomProviderEditing, + dIsCustomModelEditing, + setDProvider, + setDModel, + setDIsCustomProviderEditing, + setDIsCustomModelEditing, + envVars, + setEnvVars, + definitionRuntimeId, + setDefinitionRuntimeId, + iProvider, + iModel, + iIsCustomProviderEditing, + iIsCustomModelEditing, + setIProvider, + setIModel, + setIIsCustomProviderEditing, + setIIsCustomModelEditing, + instanceEnvVars, + setInstanceEnvVars, + selectedRuntimeId, + setSelectedRuntimeId, + setInheritHarness, + setAgentCommand, + setAgentArgs, + runtimeTouched, + setIsAddHarnessOpen, + setEffortLevel, + effortTouched, + runtimes, + selectedRuntime, + open, + }); + + // ── Focus target (R2) ────────────────────────────────────────────────────── + const { onOpenAutoFocus } = useAgentEditDeepLinkFocus({ + open, + initialFocus, + contextKey: `${inst?.pubkey ?? ""}:${def?.id ?? ""}`, + llmProviderFieldVisible, + modelDiscoveryLoading, + setShowAdvancedFields, + }); + + // ── Render ───────────────────────────────────────────────────────────────── + function handleSubmit() { + void _handleSubmit(canSubmit); + } + + const previewLabel = displayName.trim() || "Agent name"; + const previewAvatarUrl = avatarUrl.trim() || null; + + const dialogTitle = inst + ? `Edit ${inst.name}` + : def + ? `Edit ${def.displayName}` + : "Edit agent"; + + // Catalog publish affordance: only shown when the definition is shared + // and the user has dirtied at least one D-owned field. Derived from the + // canonical model diff (definitionFieldsDirty over fieldOwner) rather than a + // parallel dirty boolean — the same authority that routes emits. + const dFieldsDirty = + showDef && + !defReadOnly && + (() => { + const seed = seedAgentFormModel(ctx); + return definitionFieldsDirty( + seed, + buildNextAgentFormModel(seed, submitState), + ctx, + ); + })(); + + const publishesCatalogUpdates = !!( + def?.shared && + !defReadOnly && + dFieldsDirty + ); + + return ( + { + if (!isSaving) onOpenChange(next); + }} + open={open} + > + +
+ {publishesCatalogUpdates ? ( +

+ This agent is in the community catalog. Your changes will be + published when you save. +

+ ) : null} +
+
+ + +
+ + } + > +
+ {/* ── Identity column: avatar + preview ────────────────────── */} +
+ {/* Row 2 contract: unlinked agent (instance-only) → avatar is read-only. + Backend has no avatarUrl on UpdateManagedAgentInput, so editing is + impossible. Show disabled avatar + tooltip explaining the gap. */} + {!showDef ? ( +
+ {}} + onUploadPendingChange={() => {}} + onSelectAvatar={() => {}} + /> +

+ Avatar is shared identity — edit via the agent definition. +

+
+ ) : ( + setAvatarUrl("")} + onUploadPendingChange={setIsAvatarUploadPending} + onSelectAvatar={setAvatarUrl} + /> + )} +
+ + {/* ── Main form column ──────────────────────────────────────── */} +
+ {/* Team-managed notice */} + {defReadOnly && showDef ? ( +

+ Managed by team{sourceTeamName ? ` ${sourceTeamName}` : ""}. Its + configuration cannot be edited here. +

+ ) : null} + + {/* Built-in notice */} + {def?.isBuiltIn && !defReadOnly ? ( +

+ Built-in agent — fully editable. +

+ ) : null} + + {/* Identity-archived flair (14b) — per agent pubkey, shown for any + archived agent regardless of definition linkage. Fields remain + editable; Save does not unarchive. */} + {isIdentityArchived ? ( +

+ Archived — this agent's identity is archived on the relay. Save + will update its profile but will not unarchive. +

+ ) : null} + + {/* ── D-section: Identity + Behavior + Runtime ──────────── */} + {showDef ? ( + fieldEditable(field, ctx)} + isSaving={isSaving} + displayName={displayName} + onDisplayNameChange={setDisplayName} + description={description} + onDescriptionChange={setDescription} + systemPrompt={systemPrompt} + onSystemPromptChange={setSystemPrompt} + namePoolText={namePoolText} + onNamePoolTextChange={setNamePoolText} + envVars={envVars} + onEnvVarsChange={setEnvVars} + runtimeCatalogStatus={runtimeCatalogStatus} + runtimeDropdownValue={defRuntimeDropdownValue} + defRuntimeDropdownOptions={defRuntimeDropdownOptions} + defBlankLabel={defBlankLabel} + onRuntimeChange={(v) => { + // An explicit selection is a user choice, not an auto-seed — + // clear the ref so submit persists it even when the chosen + // runtime equals the previously auto-seeded default. + autoSeededDefinitionRuntimeRef.current = null; + handleDefinitionRuntimeChange(v); + }} + llmProviderFieldVisible={llmProviderFieldVisible} + providerSelectValue={providerSelectValue} + providerDropdownOptions={providerDropdownOptions} + onProviderChange={handleProviderDropdownChange} + isCustomProviderEditing={isCustomProviderEditing} + provider={provider} + onProviderTextChange={setDProvider} + modelSelectValue={modelSelectValue} + modelDropdownOptions={modelDropdownOptions} + onModelChange={handleModelDropdownChange} + modelDiscoveryLoading={modelDiscoveryLoading} + showCustomModelInput={showCustomModelInput} + model={model} + onModelTextChange={setDModel} + modelStatusMessage={modelStatusMessage} + showInstancePresent={showInst} + respondTo={respondTo} + respondToAllowlist={respondToAllowlist} + onRespondToChange={setRespondTo} + onAllowlistChange={setRespondToAllowlist} + agentAccessOwnerOnly={agentAccessOwnerOnly} + parallelism={parallelism} + onParallelismChange={setParallelism} + dAdvanced={dAdvanced} + /> + ) : null} + + {/* ── I-section: Instance + Runtime (when instance present) ── */} + {showInst && inst ? ( + { + setInstanceEnvVars((prev) => ({ ...prev, [key]: value })); + }} + modelSelectValue={modelSelectValue} + modelDropdownOptions={modelDropdownOptions} + onModelChange={handleModelDropdownChange} + modelDiscoveryLoading={modelDiscoveryLoading} + showCustomModelInput={showCustomModelInput} + model={model} + onModelTextChange={setIModel} + modelStatusMessage={modelStatusMessage} + modelRequired={modelRequired} + inheritedSubmission={inheritedSubmission} + inheritedModelDefault={inheritedModelDefault} + inheritedProviderDefault={inheritedProviderDefault} + aiDefaultsOpen={aiDefaultsOpen} + onAiDefaultsOpenChange={setAiDefaultsOpen} + aiDefaultsTriggerRef={aiDefaultsTriggerRef} + showAdvancedFields={showAdvancedFields} + onShowAdvancedFieldsChange={setShowAdvancedFields} + acpCommand={acpCommand} + agentArgs={agentArgs} + autoRestartOnConfigChange={autoRestartOnConfigChange} + instanceEnvVars={instanceEnvVars} + fileSatisfiedEnvKeys={fileSatisfiedEnvKeys} + advancedRequiredEnvKeys={advancedRequiredEnvKeys} + initialFocus={initialFocus} + inheritedEnvVarsForAdvanced={inheritedEnvVarsForAdvanced} + linkedPersona={linkedPersona} + prospectiveRuntimeId={prospectiveRuntimeId} + prospectiveRuntime={prospectiveRuntime} + parallelism={parallelism} + onAcpCommandChange={setAcpCommand} + onAgentArgsChange={setAgentArgs} + onAutoRestartChange={setAutoRestartOnConfigChange} + onStartOnAppLaunchChange={setStartOnAppLaunch} + onEnvVarsChange={setInstanceEnvVars} + onInheritHarnessChange={setInheritHarness} + onParallelismChange={setParallelism} + startOnAppLaunch={startOnAppLaunch} + systemPrompt={systemPrompt} + onSystemPromptChange={setSystemPrompt} + effortConfig={configSurfaceQuery.data} + effortValue={ + effortTouched.current + ? effortLevel + : (configSurfaceQuery.data?.normalized.thinkingEffort + ?.value ?? null) + } + onEffortChange={(level) => { + effortTouched.current = true; + setEffortLevel(level); + }} + /> + ) : null} + + {/* Definition admin section (Artifact 4) — rendered when a definition exists */} + {showDef && !defReadOnly && def ? ( +
+ {/* Share / catalog publish (row 15 — not shown for built-ins) */} + {onShare && !def.isBuiltIn ? ( +
+ + Share this agent in the catalog. + + +
+ ) : null} + {/* Custom definition delete with blast-radius copy */} + {onDeleteDefinition && !def.isBuiltIn ? ( +
+ + Delete this agent and all{" "} + {linkedInstanceCount > 0 + ? `${linkedInstanceCount} linked agent${linkedInstanceCount === 1 ? "" : "s"}` + : "linked agents"} + . + + +
+ ) : null} + {/* Built-in: Remove from My Agents (14a deactivate) */} + {onRemoveFromMyAgents && def.isBuiltIn ? ( +
+ + Remove this built-in agent from My Agents. + + +
+ ) : null} +
+ ) : null} + + {saveError ? ( +

{saveError.message}

+ ) : null} +
+
+ + {/* AddCustomHarnessDialog: rendered at dialog level so it works in definition-only context. */} + + + ); +} diff --git a/desktop/src/features/agents/ui/AgentEditMergedDialogDSection.tsx b/desktop/src/features/agents/ui/AgentEditMergedDialogDSection.tsx new file mode 100644 index 00000000000..030b9ac2dab --- /dev/null +++ b/desktop/src/features/agents/ui/AgentEditMergedDialogDSection.tsx @@ -0,0 +1,546 @@ +/** + * AgentEditMergedDialogDSection.tsx — Definition-field section for the merged edit surface. + * + * Renders agent name, system prompt, name pool, D-env vars, and (for all definition + * contexts) runtime, LLM provider, and model. Team-managed fields render read-only. + * + * When showInstancePresent is false (definition-only context), also renders the + * D-owned access/allowlist and parallelism controls (rows 9–10, Artifact 4). + * + * Extracted from AgentEditMergedDialog to satisfy the desktop file-size gate. + */ + +import * as React from "react"; +import { AnimatePresence, motion } from "motion/react"; + +import { cn } from "@/shared/lib/cn"; +import { Input } from "@/shared/ui/input"; +import { Textarea } from "@/shared/ui/textarea"; +import { Button } from "@/shared/ui/button"; +import type { RespondToMode } from "@/shared/api/types"; + +import { + CARD_MINT_KEY_ANNOTATIONS, + getProviderApiKeyLabel, + PERSONA_FIELD_CONTROL_CLASS, + PERSONA_FIELD_SHELL_CLASS, + PERSONA_LABEL_OPTIONAL_CLASS, + type PersonaDropdownOption, +} from "./agentConfigOptions"; +import { AgentHarnessField } from "./AgentHarnessField"; +import { PersonaDropdownField } from "./PersonaDropdownField"; +import { PersonaModelCombobox } from "./PersonaModelCombobox"; +import { PersonaProviderApiKeyField } from "./PersonaProviderApiKeyField"; +import { AdvancedRequiredBadge } from "./AdvancedRequiredBadge"; +import { EnvVarsEditor, type EnvVarsValue } from "./EnvVarsEditor"; +import { AgentIdentityFields } from "./AgentDescriptionField"; +import { OwnerOnlyAccessField } from "./OwnerOnlyAccessField"; +import { + BuzzAgentModelTuningFields, + NumericTuningFields, +} from "./buzzAgentModelTuningFields"; +import { + isBuzzAgentRuntime, + BUZZ_AGENT_THINKING_EFFORT, +} from "./buzzAgentConfig"; +import { + deriveNumericDescriptors, + structuredEnvKeys, +} from "../lib/agentConfigCore"; +import { parallelismCapHint } from "../lib/agentParallelism"; +import type { AgentFormModel } from "./agentFormModel"; +import type { DSectionAdvancedState } from "./useAgentEditRuntimeState"; + +const advancedFieldsTransition = { duration: 0.18, ease: "easeInOut" } as const; + +// ── Props ───────────────────────────────────────────────────────────────────── + +export type AgentEditMergedDSectionProps = { + /** + * Per-field editability, resolved from the ownership map (`fieldEditable` + * bound to the edit context). A D-owned field is read-only when the + * definition is team-managed; this is the single authority for enabling and + * disabling every control below — no section-level `defReadOnly` boolean. + */ + fieldEditable: (field: keyof AgentFormModel) => boolean; + isSaving: boolean; + // Identity + displayName: string; + onDisplayNameChange: (value: string) => void; + description: string; + onDescriptionChange: (value: string) => void; + // Behavior + systemPrompt: string; + onSystemPromptChange: (value: string) => void; + // Definition admin: name pool (D-field, must round-trip) + namePoolText: string; + onNamePoolTextChange: (value: string) => void; + // Definition env vars (D-field, row 8 — separate from instance overlay) + envVars: EnvVarsValue; + onEnvVarsChange: (value: EnvVarsValue) => void; + // Runtime (definition runtime D-field — shown for all definition contexts) + runtimeCatalogStatus: "loading" | "error" | "ready"; + runtimeDropdownValue: string; + defRuntimeDropdownOptions: PersonaDropdownOption[]; + defBlankLabel: string; + onRuntimeChange: (value: string) => void; + // LLM provider (D-field) + llmProviderFieldVisible: boolean; + providerSelectValue: string; + providerDropdownOptions: PersonaDropdownOption[]; + onProviderChange: (value: string) => void; + isCustomProviderEditing: boolean; + provider: string; + onProviderTextChange: (value: string) => void; + // Model (D-field) + modelSelectValue: string; + modelDropdownOptions: PersonaDropdownOption[]; + onModelChange: (value: string) => void; + modelDiscoveryLoading: boolean; + showCustomModelInput: boolean; + model: string; + onModelTextChange: (value: string) => void; + modelStatusMessage: string | null; + /** + * When false (definition-only context, no instance), renders the D-owned + * access/allowlist and parallelism controls (rows 9–10, Artifact 4). + * When true (instance is present), access/parallelism are I-owned and + * rendered in AgentEditMergedDialogInstanceSection. + */ + showInstancePresent: boolean; + // D-owned access/parallelism (shown when !showInstancePresent) + respondTo: RespondToMode | null; + respondToAllowlist: string[]; + onRespondToChange: (value: RespondToMode) => void; + onAllowlistChange: (value: string[]) => void; + agentAccessOwnerOnly: boolean | undefined; + parallelism: string; + onParallelismChange: (value: string) => void; + /** + * Definition-context advanced state (tuning knobs, env-key highlighting, + * provider API-key field) derived from the definition runtime/provider/env. + * Consumed by the Advanced section to match main's PersonaAdvancedFields. + */ + dAdvanced: DSectionAdvancedState; +}; + +// ── Component ───────────────────────────────────────────────────────────────── + +export function AgentEditMergedDSection({ + fieldEditable, + isSaving, + displayName, + onDisplayNameChange, + description, + onDescriptionChange, + systemPrompt, + onSystemPromptChange, + namePoolText, + onNamePoolTextChange, + envVars, + onEnvVarsChange, + runtimeCatalogStatus, + runtimeDropdownValue, + defRuntimeDropdownOptions, + defBlankLabel, + onRuntimeChange, + llmProviderFieldVisible, + providerSelectValue, + providerDropdownOptions, + onProviderChange, + isCustomProviderEditing, + provider, + onProviderTextChange, + modelSelectValue, + modelDropdownOptions, + onModelChange, + modelDiscoveryLoading, + showCustomModelInput, + model, + onModelTextChange, + modelStatusMessage, + showInstancePresent, + respondTo, + respondToAllowlist, + onRespondToChange, + onAllowlistChange, + agentAccessOwnerOnly, + parallelism, + onParallelismChange, + dAdvanced, +}: AgentEditMergedDSectionProps) { + const [showAdvanced, setShowAdvanced] = React.useState(false); + + // Numeric tuning descriptors — gate on catalog status so loading/error never + // collapses to "no controls": keys stay visible as generic rows. Derived from + // the DEFINITION runtime (dAdvanced.selectedRuntime), matching main's + // PersonaAdvancedFields. + const numericDescriptors = React.useMemo( + () => + runtimeCatalogStatus === "ready" + ? deriveNumericDescriptors(dAdvanced.selectedRuntime) + : [], + [runtimeCatalogStatus, dAdvanced.selectedRuntime], + ); + + // Effective hidden keys: the provider secret (rendered as the API-key field) + // + the buzz-agent effort key (rendered by BuzzAgentModelTuningFields) + the + // structured numeric keys — none of these should appear as generic env rows. + const effectiveHiddenKeys = React.useMemo( + () => [ + ...(dAdvanced.topLevelSecretEnvVar + ? [dAdvanced.topLevelSecretEnvVar] + : []), + ...(isBuzzAgentRuntime(dAdvanced.runtimeId) + ? [BUZZ_AGENT_THINKING_EFFORT] + : []), + ...structuredEnvKeys(numericDescriptors), + ], + [dAdvanced.topLevelSecretEnvVar, dAdvanced.runtimeId, numericDescriptors], + ); + + // Parallelism cap hint: the definition keeps a portable requested value; when + // the selected harness has a cap and the draft exceeds it, explain the agent + // will run at the cap — without clamping the stored value. + const parallelismHint = React.useMemo(() => { + const runtime = dAdvanced.selectedRuntime; + if (runtime?.maxParallelism === undefined || parallelism === "") { + return null; + } + const requested = parseInt(parallelism, 10); + if (Number.isNaN(requested)) return null; + return parallelismCapHint(runtime.label, runtime.maxParallelism, requested); + }, [dAdvanced.selectedRuntime, parallelism]); + + // Env-var mutation shared by both tuning-field groups: delete on empty so a + // cleared knob reverts to the inherited placeholder rather than persisting "". + const onTuningEnvVarChange = React.useCallback( + (key: string, value: string) => { + const next = { ...envVars }; + if (value === "") { + delete next[key]; + } else { + next[key] = value; + } + onEnvVarsChange(next); + }, + [envVars, onEnvVarsChange], + ); + + // Bind the definition secret key to a local so the truthy guard narrows it + // to `string` inside the API-key field's onValueChange closure (a property + // access on `dAdvanced` would not narrow through the closure). + const defSecretEnvVar = dAdvanced.topLevelSecretEnvVar; + + return ( + <> + {/* Identity: agent name + public description (definition-shared D-fields). + Uses AgentIdentityFields to match the create dialog's clamp/counter + semantics — see AgentDescriptionField.tsx and #7126. */} + + + {/* System prompt */} +
+ +
+