From bdea3002df41d33af2b69b2a7a1b2fddbbdb1ce8 Mon Sep 17 00:00:00 2001 From: Jack Date: Sun, 6 Sep 2026 23:09:35 -0500 Subject: [PATCH] fix(desktop): make starter profiles optional and removable Signed-off-by: Jack --- desktop/playwright.config.ts | 1 + desktop/src-tauri/src/commands/teams/mod.rs | 37 +++++-- desktop/src-tauri/src/commands/teams/tests.rs | 47 ++++++++ .../src-tauri/src/managed_agents/personas.rs | 6 +- .../src/managed_agents/personas/tests.rs | 59 +++++++++- desktop/src-tauri/src/managed_agents/teams.rs | 45 ++++---- .../src/managed_agents/teams_tests.rs | 35 +++--- desktop/src-tauri/src/migration/fold.rs | 34 ++++-- desktop/src/features/agents/ui/AgentsView.tsx | 31 ++++++ .../src/features/onboarding/welcomeGuide.ts | 49 ++++----- .../src/features/onboarding/welcomeKickoff.ts | 1 + .../onboarding/welcomeTeamOptOut.test.mjs | 54 +++++++++ desktop/src/testing/e2eBridge.ts | 18 ++- desktop/tests/e2e/removable-defaults.spec.ts | 103 ++++++++++++++++++ desktop/tests/helpers/bridge.ts | 1 + 15 files changed, 421 insertions(+), 100 deletions(-) create mode 100644 desktop/src/features/onboarding/welcomeTeamOptOut.test.mjs create mode 100644 desktop/tests/e2e/removable-defaults.spec.ts diff --git a/desktop/playwright.config.ts b/desktop/playwright.config.ts index aad2580dad0..005e663cdc0 100644 --- a/desktop/playwright.config.ts +++ b/desktop/playwright.config.ts @@ -20,6 +20,7 @@ export default defineConfig({ name: "smoke", testMatch: [ "**/smoke.spec.ts", + "**/removable-defaults.spec.ts", "**/owned-agent-discovery.spec.ts", "**/thread-head-stale-edit.spec.ts", "**/sidebar-offcanvas-rail.spec.ts", diff --git a/desktop/src-tauri/src/commands/teams/mod.rs b/desktop/src-tauri/src/commands/teams/mod.rs index 208ac3a7117..4d2c303a2b1 100644 --- a/desktop/src-tauri/src/commands/teams/mod.rs +++ b/desktop/src-tauri/src/commands/teams/mod.rs @@ -512,6 +512,21 @@ pub async fn update_team(input: UpdateTeamRequest, app: AppHandle) -> Result Result, String>, + retain: impl FnOnce(&str), +) -> Result, String> { + let cascaded = delete()?; + // Built-in teams are device-local templates, not owner-published records. + // Removing one must not delete another device's customized copy. + if !team.is_builtin { + retain(&team.id); + } + Ok(cascaded) +} + #[tauri::command] pub async fn delete_team(id: String, app: AppHandle) -> Result<(), String> { use tauri::Manager; @@ -521,16 +536,18 @@ pub async fn delete_team(id: String, app: AppHandle) -> Result<(), String> { .managed_agents_store_lock .lock() .map_err(|error| error.to_string())?; - let cascaded_persona_d_tags = delete_team_with_cascade(&app, &id)?; - // delete_team_with_cascade rejects built-in teams via validate_team_deletion, - // so reaching here means this team was owner-published — tombstone it. The - // d_tag is the team id, captured before the record left the store. - tombstone_team_pending(&app, &state, &id); - // The catalog projection is a separate coordinate with its own - // retained head, so the 30176 tombstone above does not retract it. - // Without this, deleting a shared team would leave a live catalog - // entry the owner can no longer see or unshare. - pending::tombstone_team_catalog_pending(&app, &state, &id); + let team = load_teams(&app)? + .into_iter() + .find(|team| team.id == id) + .ok_or_else(|| format!("team {id} not found"))?; + let cascaded_persona_d_tags = delete_team_and_retain( + &team, + || delete_team_with_cascade(&app, &id), + |id| { + tombstone_team_pending(&app, &state, id); + pending::tombstone_team_catalog_pending(&app, &state, id); + }, + )?; // Tombstone the cascaded personas too, so their orphaned kind:30175 heads // don't linger on the relay (F4). Each d-tag was captured pre-removal. for persona_d_tag in &cascaded_persona_d_tags { diff --git a/desktop/src-tauri/src/commands/teams/tests.rs b/desktop/src-tauri/src/commands/teams/tests.rs index 89942c5ff27..5438af488af 100644 --- a/desktop/src-tauri/src/commands/teams/tests.rs +++ b/desktop/src-tauri/src/commands/teams/tests.rs @@ -9,6 +9,53 @@ use buzz_core_pkg::kind::KIND_TEAM; use nostr::JsonUtil; use std::path::{Path, PathBuf}; +#[test] +fn builtin_delete_is_local_only_and_durable_while_custom_delete_retains_tombstone() { + use tauri::Manager; + let dir = tempfile::tempdir().unwrap(); + let mut context = tauri::test::mock_context(tauri::test::noop_assets()); + context.config_mut().identifier = dir.path().join("app").to_string_lossy().into_owned(); + let app = tauri::test::mock_builder().build(context).unwrap(); + assert_eq!(app.path().app_data_dir().unwrap(), dir.path().join("app")); + let keys = nostr::Keys::generate(); + let db = dir.path().join("retention.db"); + let conn = open_retention_db(&db).unwrap(); + let mut welcome = team(); + welcome.id = "builtin-team:welcome".into(); + welcome.name = "Welcome Team".into(); + welcome.source_dir = None; + welcome.is_builtin = true; + let mut custom = welcome.clone(); + custom.id = "custom:lookalike".into(); + custom.is_builtin = false; + save_teams(app.handle(), &[welcome.clone(), custom.clone()]).unwrap(); + delete_team_and_retain( + &welcome, + || delete_team_with_cascade(app.handle(), &welcome.id), + |id| tombstone_team_at(&db, &keys, id).unwrap(), + ) + .unwrap(); + let reloaded = load_teams(app.handle()).unwrap(); + assert_eq!(reloaded.len(), 1); + assert_eq!(reloaded[0].id, custom.id); + assert!( + get_pending_sync(&conn).unwrap().is_empty(), + "local built-in opt-out must never publish an owner deletion" + ); + delete_team_and_retain( + &custom, + || delete_team_with_cascade(app.handle(), &custom.id), + |id| tombstone_team_at(&db, &keys, id).unwrap(), + ) + .unwrap(); + assert!(load_teams(app.handle()).unwrap().is_empty()); + assert_eq!( + get_pending_sync(&conn).unwrap().len(), + 1, + "custom owner deletion still syncs" + ); +} + fn team() -> TeamRecord { TeamRecord { id: "team-abc".to_string(), diff --git a/desktop/src-tauri/src/managed_agents/personas.rs b/desktop/src-tauri/src/managed_agents/personas.rs index 094d0a1a478..0f17325aeb5 100644 --- a/desktop/src-tauri/src/managed_agents/personas.rs +++ b/desktop/src-tauri/src/managed_agents/personas.rs @@ -47,7 +47,7 @@ const BUILT_IN_PERSONAS: &[BuiltInPersona] = &[ ], model: None, runtime: None, - default_active: true, + default_active: false, }, BuiltInPersona { id: "builtin:honey", @@ -57,7 +57,7 @@ const BUILT_IN_PERSONAS: &[BuiltInPersona] = &[ name_pool: &["Honey"], model: None, runtime: None, - default_active: true, + default_active: false, }, BuiltInPersona { id: POLLEN_PERSONA_ID, @@ -67,7 +67,7 @@ const BUILT_IN_PERSONAS: &[BuiltInPersona] = &[ name_pool: &[POLLEN_DISPLAY_NAME], model: None, runtime: None, - default_active: true, + default_active: false, }, ]; diff --git a/desktop/src-tauri/src/managed_agents/personas/tests.rs b/desktop/src-tauri/src/managed_agents/personas/tests.rs index a52f6aa3b19..3f0fe704f1f 100644 --- a/desktop/src-tauri/src/managed_agents/personas/tests.rs +++ b/desktop/src-tauri/src/managed_agents/personas/tests.rs @@ -34,7 +34,7 @@ fn custom_persona(id: &str, display_name: &str) -> AgentDefinition { } #[test] -fn merge_personas_adds_missing_built_ins() { +fn merge_personas_adds_missing_built_ins_as_optional_templates() { let (records, changed) = merge_personas(Vec::new(), "2026-03-19T00:00:00Z"); assert!(changed); @@ -53,9 +53,9 @@ fn merge_personas_adds_missing_built_ins() { .filter(|record| record.is_active) .map(|record| record.id.as_str()) .collect(); - assert_eq!( - active_ids, - vec!["builtin:fizz", "builtin:honey", "builtin:bumble"] + assert!( + active_ids.is_empty(), + "starter templates must be explicitly added" ); } @@ -124,7 +124,10 @@ fn merge_personas_adds_fizz_and_retires_old_builtins_for_existing_store() { .find(|record| record.id == "builtin:fizz") .expect("fizz built-in should exist"); assert!(fizz.is_builtin); - assert!(fizz.is_active); + assert!( + !fizz.is_active, + "upgrades must not activate new starter templates" + ); let solo = records .iter() @@ -412,3 +415,49 @@ fn fizz_builtin_resolves_to_buzz_agent() { "Fizz must resolve to buzz-agent specifically" ); } + +#[test] +fn optional_templates_survive_real_store_reload_and_keep_custom_profiles() { + use tauri::Manager; + let dir = tempfile::tempdir().unwrap(); + let mut context = tauri::test::mock_context(tauri::test::noop_assets()); + // Absolute identifier confines Tauri's joined app_data_dir to this tempdir + // without mutating process environment or reaching the user's installation. + context.config_mut().identifier = dir + .path() + .join("isolated-app") + .to_string_lossy() + .into_owned(); + let app = tauri::test::mock_builder().build(context).unwrap(); + assert_eq!( + app.path().app_data_dir().unwrap(), + dir.path().join("isolated-app") + ); + let mut personas = super::load_personas(app.handle()).unwrap(); + assert!(personas.iter().all(|p| !p.is_active)); + let fizz = personas + .iter_mut() + .find(|p| p.id == "builtin:fizz") + .unwrap(); + fizz.is_active = true; + fizz.display_name = "My customized starter".into(); + personas.push(custom_persona("custom:lookalike", "Fizz")); + super::save_personas(app.handle(), &personas).unwrap(); + let mut personas = super::load_personas(app.handle()).unwrap(); + let fizz = personas + .iter_mut() + .find(|p| p.id == "builtin:fizz") + .unwrap(); + validate_persona_activation_change(fizz, false, false, false).unwrap(); + fizz.is_active = false; + super::save_personas(app.handle(), &personas).unwrap(); + for _ in 0..2 { + let reloaded = super::load_personas(app.handle()).unwrap(); + let fizz = reloaded.iter().find(|p| p.id == "builtin:fizz").unwrap(); + assert!(!fizz.is_active); + assert_eq!(fizz.display_name, "My customized starter"); + assert!(reloaded + .iter() + .any(|p| p.id == "custom:lookalike" && p.is_active)); + } +} diff --git a/desktop/src-tauri/src/managed_agents/teams.rs b/desktop/src-tauri/src/managed_agents/teams.rs index 9d6d17aa9ad..12f3f87c6f3 100644 --- a/desktop/src-tauri/src/managed_agents/teams.rs +++ b/desktop/src-tauri/src/managed_agents/teams.rs @@ -76,7 +76,7 @@ fn built_in_team_order(built_ins: &[BuiltInTeam], id: &str) -> Option { built_ins.iter().position(|team| team.id == id) } -/// Add missing built-in teams, purge pristine retired teams, demote stale +/// Purge pristine retired teams, demote stale /// built-ins, and preserve any user customizations to existing built-in teams /// (name, description, persona membership). Returns the merged list and whether /// the store changed. @@ -92,7 +92,7 @@ fn merge_teams_impl( ) -> (Vec, bool) { let mut changed = false; - // Seed missing built-ins / re-promote existing ones that were downgraded. + // Preserve existing built-ins, but never recreate a deleted or unselected team. for built_in in built_in_team_records(built_ins, now) { if let Some(existing) = stored.iter_mut().find(|record| record.id == built_in.id) { if !existing.is_builtin { @@ -100,9 +100,6 @@ fn merge_teams_impl( existing.updated_at = now.to_string(); changed = true; } - } else { - stored.push(built_in); - changed = true; } } @@ -143,13 +140,20 @@ fn merge_teams_impl( (stored, changed) } -/// Reject deletion of built-in teams. Mirrors `validate_persona_deletion` -/// for personas — built-ins always come back via `merge_teams` on the -/// next load, so blocking the delete avoids a confusing "keeps coming -/// back" UX. -pub fn validate_team_deletion(team: &TeamRecord) -> Result<(), String> { - if team.is_builtin { - return Err("Built-in teams cannot be deleted.".to_string()); +/// Reject deletion while managed instances still depend on the team. +/// Built-in status is provenance, not a deletion restriction. +pub fn validate_team_deletion( + team: &TeamRecord, + agents: &[ManagedAgentRecord], +) -> Result<(), String> { + let referencing = agents_referencing_team(agents, team); + if !referencing.is_empty() { + return Err(format!( + "Cannot delete team \"{}\": {} agent(s) still reference it ({}). Delete or reconfigure them first.", + team.name, + referencing.len(), + referencing.join(", ") + )); } Ok(()) } @@ -244,25 +248,18 @@ fn agents_referencing_team<'a>( /// vec is empty. For catalog-adopted teams (`catalog_source` present), member /// copies matching this publication's provenance are deactivated (re-activatable /// on re-add), not deleted. -pub fn delete_team_with_cascade(app: &AppHandle, team_id: &str) -> Result, String> { +pub fn delete_team_with_cascade( + app: &AppHandle, + team_id: &str, +) -> Result, String> { let mut teams = load_teams(app)?; let team = teams .iter() .find(|record| record.id == team_id) .ok_or_else(|| format!("team {team_id} not found"))?; - validate_team_deletion(team)?; - let agents = crate::managed_agents::load_managed_agents(app)?; - let referencing = agents_referencing_team(&agents, team); - if !referencing.is_empty() { - return Err(format!( - "Cannot delete team \"{team_id}\": {} agent(s) still reference it ({}). \ - Delete or reconfigure them first.", - referencing.len(), - referencing.join(", ") - )); - } + validate_team_deletion(team, &agents)?; let mut cascaded_persona_d_tags = Vec::new(); diff --git a/desktop/src-tauri/src/managed_agents/teams_tests.rs b/desktop/src-tauri/src/managed_agents/teams_tests.rs index fc6f0f1a97b..e526ed8331a 100644 --- a/desktop/src-tauri/src/managed_agents/teams_tests.rs +++ b/desktop/src-tauri/src/managed_agents/teams_tests.rs @@ -56,7 +56,7 @@ fn sort_teams_empty_is_noop() { } #[test] -fn merge_teams_adds_missing_built_ins() { +fn merge_teams_does_not_add_missing_built_ins() { let synthetic = BuiltInTeam { id: "builtin-team:test", name: "Test Team", @@ -67,10 +67,8 @@ fn merge_teams_adds_missing_built_ins() { let (records, changed) = merge_teams_impl(&[synthetic], &[], Vec::new(), "2026-05-07T00:00:00Z"); - assert!(changed); - assert_eq!(records.len(), 1); - assert!(records.iter().all(|r| r.is_builtin)); - assert_eq!(records[0].id, "builtin-team:test"); + assert!(!changed); + assert!(records.is_empty()); } #[test] @@ -111,7 +109,7 @@ fn merge_teams_preserves_unrelated_user_teams() { merge_teams_impl(&[synthetic], &[], vec![user_team], "2026-05-07T00:00:00Z"); assert!(records.iter().any(|t| t.id == "user-uuid")); - assert!(records.iter().any(|t| t.id == "builtin-team:test")); + assert!(!records.iter().any(|t| t.id == "builtin-team:test")); } #[test] @@ -155,12 +153,11 @@ fn merge_teams_repromotes_existing_builtin_marked_as_custom() { } #[test] -fn validate_team_deletion_rejects_built_ins() { +fn validate_team_deletion_allows_built_ins() { let mut built_in = team("builtin-team:fizz", "Fizz"); built_in.is_builtin = true; - let err = validate_team_deletion(&built_in).unwrap_err(); - assert_eq!(err, "Built-in teams cannot be deleted."); + assert!(validate_team_deletion(&built_in, &[]).is_ok()); } // ── agents_referencing_team ───────────────────────────────────────────── @@ -331,10 +328,17 @@ fn migration_customized_fizz_is_demoted_to_user_team() { } #[test] -fn welcome_team_is_seeded_and_idempotent() { - let (records, changed) = merge_teams(Vec::new(), "2026-07-01T00:00:00Z"); +fn welcome_team_is_optional_and_empty_store_stays_empty() { + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("teams.json"); + assert!(load_teams_readonly(&path).unwrap().is_empty()); + std::fs::write(&path, b"[]").unwrap(); + assert!(load_teams_readonly(&path).unwrap().is_empty()); +} - assert!(changed); +#[test] +fn welcome_team_template_is_preserved_and_idempotent() { + let records = super::built_in_team_records(super::BUILT_IN_TEAMS, "2026-07-01T00:00:00Z"); assert_eq!(records.len(), 1); let welcome = &records[0]; assert_eq!(welcome.id, "builtin-team:welcome"); @@ -364,7 +368,7 @@ fn welcome_team_is_seeded_and_idempotent() { #[test] fn welcome_team_seed_does_not_overwrite_customization() { - let (mut records, _) = merge_teams(Vec::new(), "2026-07-01T00:00:00Z"); + let mut records = super::built_in_team_records(super::BUILT_IN_TEAMS, "2026-07-01T00:00:00Z"); let welcome = records .iter_mut() .find(|team| team.id == "builtin-team:welcome") @@ -401,9 +405,8 @@ fn load_teams_readonly_absent_file_performs_no_write() { let records = load_teams_readonly(&path).unwrap(); - // Returns the merged built-in list without persisting it. - assert_eq!(records.len(), 1); - assert_eq!(records[0].id, "builtin-team:welcome"); + // A fresh store has no selected teams, and reads do not opt in. + assert!(records.is_empty()); // The file must still NOT exist — no write-on-load side effect. assert!( diff --git a/desktop/src-tauri/src/migration/fold.rs b/desktop/src-tauri/src/migration/fold.rs index 727982e448d..98d6e0fa3c9 100644 --- a/desktop/src-tauri/src/migration/fold.rs +++ b/desktop/src-tauri/src/migration/fold.rs @@ -10,8 +10,8 @@ use std::path::Path; /// ([`AgentDefinition::into_agent_record`]) appended to `managed-agents.json` /// via the definition-preserving save; the old file is renamed to /// `personas.json.bak` so a second boot is a no-op and the data survives for -/// manual recovery. Built-ins are skipped — `merge_personas` regenerates them -/// from code on every load, exactly as before. +/// manual recovery. Built-ins are folded too: their local activation choices +/// and authored configuration must survive the store transition. /// /// Ordering (see `run_boot_migrations`): runs after the JSON-level /// `personas.json` migrations (which must see the legacy file) and BEFORE @@ -63,9 +63,9 @@ fn fold_personas_in_dir(base_dir: &Path) -> Result, String> { let mut folded = 0usize; for persona in personas { - // Built-ins regenerate from code; a slug already in the store means - // a previous partial fold got that far — never duplicate. - if persona.is_builtin || existing.contains(&persona.id) { + // A slug already in the store means a previous partial fold got + // that far — never duplicate or overwrite its newer local state. + if existing.contains(&persona.id) { continue; } all.push(persona.into_agent_record()); @@ -202,10 +202,10 @@ mod tests { let base = dir.path().join("agents"); let folded = fold_personas_in_dir(&base).unwrap(); - assert_eq!(folded, Some(1), "custom folds, builtin skipped"); + assert_eq!(folded, Some(2), "custom and built-in choices both fold"); let records = read_agents_json(dir.path()); - assert_eq!(records.len(), 2, "definition + preserved instance"); + assert_eq!(records.len(), 3, "definitions + preserved instance"); let def = records .iter() .find(|r| r.get("slug").is_some()) @@ -221,6 +221,26 @@ mod tests { assert!(base.join("personas.json.bak").exists(), ".bak left behind"); } + #[test] + fn fold_preserves_builtin_opt_out_and_customization() { + let dir = tempfile::tempdir().unwrap(); + let mut persona = custom_persona_json("builtin:fizz", "goose"); + persona["is_builtin"] = serde_json::json!(true); + persona["is_active"] = serde_json::json!(false); + persona["display_name"] = serde_json::json!("My Fizz"); + write_personas_json(dir.path(), &serde_json::json!([persona])); + let base = dir.path().join("agents"); + assert_eq!(fold_personas_in_dir(&base).unwrap(), Some(1)); + let records = read_agents_json(dir.path()); + let definition: crate::managed_agents::ManagedAgentRecord = + serde_json::from_value(records[0].clone()).unwrap(); + let view = definition.to_definition_view().unwrap(); + assert!(!view.is_active); + assert_eq!(view.display_name, "My Fizz"); + assert_eq!(view.runtime.as_deref(), Some("goose")); + assert_eq!(fold_personas_in_dir(&base).unwrap(), None); + } + #[test] fn fold_is_idempotent_across_partial_runs() { // Simulate a crash after the store write but before the rename: the diff --git a/desktop/src/features/agents/ui/AgentsView.tsx b/desktop/src/features/agents/ui/AgentsView.tsx index a31fec44c49..8799189a512 100644 --- a/desktop/src/features/agents/ui/AgentsView.tsx +++ b/desktop/src/features/agents/ui/AgentsView.tsx @@ -146,6 +146,37 @@ export function AgentsView() { + + + + + + {(personas.personasQuery.data ?? []) + .filter((persona) => persona.isBuiltIn) + .map((persona) => ( + { + void personas.handleSetActive( + persona, + true, + "library", + ); + }} + > + {persona.isActive ? "Added" : "Add"}{" "} + {persona.displayName} + + ))} + +