fix: skip avatar reconciliation for legacy agent records (#933)

Signed-off-by: npub1mn7jgtj4w2pd0g0zeuhxsa6jy6p0rewxz4kujt98my82ahfmp72sxjexk7 <dcfd242e557282d7a1e2cf2e6877522682f1e5c6156dc92ca7d90eaedd3b0f95@sprout-oss.stage.blox.sqprod.co> Signed-off-by: npub1fgdl5qqnh3k3f2xkqrvt7cujalhm623x4s7fdjdj5yrtp5fzjl9qrjpucw <4a1bfa0013bc6d14a8d600d8bf6392efefbd2a26ac3c96c9b2a106b0d12297ca@sprout-oss.stage.blox.sqprod.co> Co-authored-by: npub1fgdl5qqnh3k3f2xkqrvt7cujalhm623x4s7fdjdj5yrtp5fzjl9qrjpucw <4a1bfa0013bc6d14a8d600d8bf6392efefbd2a26ac3c96c9b2a106b0d12297ca@sprout-oss.stage.blox.sqprod.co>
2026-08-18 06:50:31 +02:00 · 2026-06-09 19:52:11 -04:00
parent 165b9f7a5f
commit 73cd8d082d
3 changed files with 175 additions and 52 deletions
@@ -30,7 +30,7 @@ const rules = [
 // Do not add to this list; split the file instead. Remove each entry as its
 // file is broken up. Tracked as a follow-up.
 const overrides = new Map([
-  ["src-tauri/src/commands/agents.rs", 1208],
+  ["src-tauri/src/commands/agents.rs", 1287],
  ["src-tauri/src/managed_agents/nest.rs", 1420],
  ["src-tauri/src/managed_agents/runtime.rs", 1465],
  ["src-tauri/src/managed_agents/persona_card.rs", 1050],
@@ -695,17 +695,25 @@ pub async fn create_managed_agent(
 }

 /// Data needed for background profile reconciliation after agent start.
-struct ProfileReconcileData {
-    private_key_nsec: String,
-    name: String,
-    relay_url: String,
-    /// Expected avatar URL for the published profile. Resolved at start from the
-    /// record's persisted `avatar_url` (the exact URL published at creation),
-    /// falling back to persona/command derivation only for pre-existing records
-    /// that have no stored value — so old records still self-heal without
-    /// regressing a user-overridden avatar.
-    avatar_url: Option<String>,
-    auth_tag: Option<String>,
+pub(crate) struct ProfileReconcileData {
+    pub(crate) private_key_nsec: String,
+    pub(crate) name: String,
+    pub(crate) relay_url: String,
+    /// Expected avatar URL for the published profile. `None` for legacy records
+    /// that predate the `avatar_url` field — these will be backfilled from the
+    /// relay's existing kind:0 profile on first reconciliation.
+    pub(crate) avatar_url: Option<String>,
+    pub(crate) auth_tag: Option<String>,
+    /// The agent's pubkey (hex). Needed to update the persisted record during
+    /// avatar backfill migration.
+    pub(crate) pubkey: String,
+    /// The agent's command (e.g. "goose"). Used as fallback when no profile
+    /// exists on the relay during avatar backfill.
+    pub(crate) agent_command: String,
+    /// Persona ID if this agent was created from a persona. Used during avatar
+    /// backfill to recover the correct avatar from the persona record when the
+    /// relay profile has been corrupted.
+    pub(crate) persona_id: Option<String>,
 }

 #[tauri::command]
@@ -745,14 +753,15 @@ pub async fn start_managed_agent(

        let record = find_managed_agent_mut(&mut records, &pubkey)?;

-        let expected_avatar = reconcile_avatar(record.avatar_url.as_deref(), &record.agent_command);
-
        let reconcile = ProfileReconcileData {
            private_key_nsec: record.private_key_nsec.clone(),
            name: record.name.clone(),
            relay_url: record.relay_url.clone(),
-            avatar_url: expected_avatar,
+            avatar_url: record.avatar_url.clone(),
            auth_tag: record.auth_tag.clone(),
+            pubkey: record.pubkey.clone(),
+            agent_command: record.agent_command.clone(),
+            persona_id: record.persona_id.clone(),
        };

        let target = if record.backend == BackendKind::Local {
@@ -812,7 +821,8 @@ pub async fn start_managed_agent(
    // ── Profile reconciliation (fire-and-forget) ────────────────────────────
    // On successful start, spawn a background task to ensure the agent's kind:0
    // profile is published on the relay. This self-heals cases where the initial
-    // profile sync at creation time failed silently.
+    // profile sync at creation time failed silently. For legacy records (pre-PR-921)
+    // with no persisted avatar, this also backfills the avatar from the relay.
    if result.is_ok() {
        let reconcile_pubkey = pubkey.clone();
        let reconcile_app = app.clone();
@@ -820,7 +830,8 @@ pub async fn start_managed_agent(
            use tauri::Manager;
            let state = reconcile_app.state::<AppState>();
            if let Err(e) =
-                reconcile_agent_profile(&state, &reconcile_pubkey, &reconcile_data).await
+                reconcile_agent_profile(&state, &reconcile_app, &reconcile_pubkey, &reconcile_data)
+                    .await
            {
                eprintln!(
                    "sprout-desktop: profile reconciliation failed for agent {reconcile_pubkey}: {e}"
@@ -832,29 +843,95 @@ pub async fn start_managed_agent(
    result
 }

+/// Resolve the avatar to backfill for a legacy agent record (pre-PR-921, no
+/// stored `avatar_url`).
+///
+/// Priority: the persona's avatar wins, because the old reconciliation code
+/// could have overwritten the relay's kind:0 `picture` with the command default
+/// — making the relay an unreliable source for persona-backed agents. Only fall
+/// back to the relay's `picture`, then the command icon, for agents with no
+/// persona avatar to recover from.
+fn resolve_legacy_avatar(
+    persona_avatar: Option<String>,
+    relay_picture: Option<String>,
+    agent_command: &str,
+) -> String {
+    persona_avatar
+        .or(relay_picture)
+        .or_else(|| managed_agent_avatar_url(agent_command))
+        .unwrap_or_default()
+}
+
 /// Reconcile an agent's kind:0 profile on the relay.
 ///
 /// Queries the relay for the agent's existing profile and re-publishes if missing
 /// or stale (display_name or picture mismatch). This is fire-and-forget — errors
 /// are returned to the caller for logging but never block agent startup.
 ///
+/// For legacy records (pre-PR-921) where `avatar_url` is `None`, this function
+/// backfills via `resolve_legacy_avatar` — preferring the persona record's avatar
+/// over the relay's `picture`, since the old code may have corrupted the relay
+/// profile — and persists the updated record. After backfill, normal
+/// reconciliation proceeds.
+///
 /// Query and publish both target the agent's stored `relay_url` so that, under
 /// an active workspace relay override, reconciliation reads and writes the same
 /// relay the agent's profile actually lives on.
-async fn reconcile_agent_profile(
+pub(crate) async fn reconcile_agent_profile(
    state: &AppState,
+    app: &AppHandle,
    agent_pubkey: &str,
    data: &ProfileReconcileData,
 ) -> Result<(), String> {
    use crate::relay::{query_agent_profile, sync_managed_agent_profile};

-    // Compare against the avatar persisted at creation time — never re-derive it.
-    let expected_avatar = data.avatar_url.as_deref();
-
-    // Query the same relay the profile is published to (the stored relay_url).
+    // Query the relay for the agent's existing kind:0 profile.
    let existing = query_agent_profile(state, &data.relay_url, agent_pubkey).await?;

-    if !profile_needs_sync(existing.as_ref(), &data.name, expected_avatar) {
+    // Resolve the expected avatar — backfilling for legacy records that have no
+    // stored avatar_url yet.
+    let expected_avatar = match data.avatar_url.as_deref() {
+        Some(url) => url.to_string(),
+        None => {
+            // Legacy record: the relay profile may have been corrupted by the
+            // old reconciliation code (it overwrote the persona avatar with the
+            // command default), so the persona record is the authoritative source.
+            let persona_avatar = data.persona_id.as_ref().and_then(|pid| {
+                load_personas(app)
+                    .ok()?
+                    .into_iter()
+                    .find(|p| p.id == *pid)?
+                    .avatar_url
+            });
+
+            let backfilled = resolve_legacy_avatar(
+                persona_avatar,
+                existing.as_ref().and_then(|info| info.picture.clone()),
+                &data.agent_command,
+            );
+
+            // Persist the backfilled avatar so this migration only runs once.
+            if !backfilled.is_empty() {
+                let _store_guard = state
+                    .managed_agents_store_lock
+                    .lock()
+                    .map_err(|e| e.to_string())?;
+                let mut records = load_managed_agents(app)?;
+                if let Some(record) = records.iter_mut().find(|r| r.pubkey == data.pubkey) {
+                    record.avatar_url = Some(backfilled.clone());
+                    save_managed_agents(app, &records)?;
+                }
+            }
+
+            backfilled
+        }
+    };
+
+    if expected_avatar.is_empty() {
+        return Ok(());
+    }
+
+    if !profile_needs_sync(existing.as_ref(), &data.name, Some(&expected_avatar)) {
        return Ok(());
    }

@@ -866,7 +943,7 @@ async fn reconcile_agent_profile(
        &data.relay_url,
        &agent_keys,
        &data.name,
-        expected_avatar,
+        Some(&expected_avatar),
        data.auth_tag.as_deref(),
    )
    .await
@@ -890,18 +967,6 @@ fn profile_needs_sync(
    }
 }

-/// Resolve the avatar a managed agent's profile should reconcile against.
-/// Stored value (persisted at creation) wins; legacy records that predate the
-/// field (`stored == None`) fall back to the command-based derivation — the
-/// same source the create path used. Persona config is never consulted: doing
-/// so diverges from what was published and overwrites user intent on restart.
-fn reconcile_avatar(stored: Option<&str>, agent_command: &str) -> Option<String> {
-    match stored {
-        Some(url) => Some(url.to_string()),
-        None => managed_agent_avatar_url(agent_command),
-    }
-}
-
 #[tauri::command]
 pub fn stop_managed_agent(
    pubkey: String,
@@ -1182,26 +1247,40 @@ mod tests {
        assert!(profile_needs_sync(Some(&existing), "Duncan", None));
    }

-    /// Legacy records (`avatar_url: None`) must reconcile against
-    /// `managed_agent_avatar_url(agent_command)` — never persona config —
-    /// matching what the original create path published.
    #[test]
-    fn reconcile_avatar_legacy_record_uses_command_not_persona() {
-        let resolved = reconcile_avatar(None, "goose");
-
-        assert_eq!(resolved, managed_agent_avatar_url("goose"));
-        assert!(
-            resolved.is_some(),
-            "goose command should have a known avatar"
+    fn legacy_avatar_prefers_persona_over_corrupted_relay_picture() {
+        // The regression: the relay picture was overwritten with the command
+        // default. The persona avatar must win so the correct avatar is restored.
+        let resolved = resolve_legacy_avatar(
+            Some("https://x/persona.png".to_string()),
+            Some("https://x/default-icon.png".to_string()),
+            "goose",
        );
+
+        assert_eq!(resolved, "https://x/persona.png");
    }

-    /// New records persist their avatar at creation; the stored value is used
-    /// verbatim, never falling back to command derivation.
    #[test]
-    fn reconcile_avatar_stored_value_wins() {
-        let resolved = reconcile_avatar(Some("https://custom/avatar.png"), "goose");
+    fn legacy_avatar_falls_back_to_relay_picture_without_persona() {
+        let resolved =
+            resolve_legacy_avatar(None, Some("https://x/relay.png".to_string()), "goose");

-        assert_eq!(resolved.as_deref(), Some("https://custom/avatar.png"));
+        assert_eq!(resolved, "https://x/relay.png");
+    }
+
+    #[test]
+    fn legacy_avatar_falls_back_to_command_icon_when_no_persona_or_relay() {
+        use crate::managed_agents::managed_agent_avatar_url;
+
+        let resolved = resolve_legacy_avatar(None, None, "goose");
+
+        assert_eq!(resolved, managed_agent_avatar_url("goose").unwrap());
+    }
+
+    #[test]
+    fn legacy_avatar_empty_when_nothing_resolves() {
+        let resolved = resolve_legacy_avatar(None, None, "totally-unknown-command");
+
+        assert!(resolved.is_empty());
    }
 }
@@ -164,6 +164,8 @@ pub async fn restore_managed_agents_on_launch(
        .lock()
        .map_err(|error| error.to_string())?;

+    let mut successfully_spawned: Vec<String> = Vec::new();
+
    for (pubkey, result) in spawn_results {
        let record = match find_managed_agent_mut(&mut records, &pubkey) {
            Ok(r) => r,
@@ -178,7 +180,8 @@ pub async fn restore_managed_agents_on_launch(
                record.last_stopped_at = None;
                record.last_exit_code = None;
                record.last_error = None;
-                runtimes.insert(pubkey, ManagedAgentProcess { child, log_path });
+                runtimes.insert(pubkey.clone(), ManagedAgentProcess { child, log_path });
+                successfully_spawned.push(pubkey);
            }
            Err(error) => {
                record.updated_at = util::now_iso();
@@ -187,8 +190,49 @@ pub async fn restore_managed_agents_on_launch(
        }
    }

+    // Collect profile reconciliation data for successfully spawned agents before
+    // releasing the lock. This mirrors the fire-and-forget pattern in
+    // start_managed_agent — ensuring boot-restored agents get the same profile
+    // self-healing as UI-started agents.
+    let reconcile_items: Vec<(String, crate::commands::ProfileReconcileData)> =
+        successfully_spawned
+            .iter()
+            .filter_map(|pubkey| {
+                let record = records.iter().find(|r| r.pubkey == *pubkey)?;
+                Some((
+                    pubkey.clone(),
+                    crate::commands::ProfileReconcileData {
+                        private_key_nsec: record.private_key_nsec.clone(),
+                        name: record.name.clone(),
+                        relay_url: record.relay_url.clone(),
+                        avatar_url: record.avatar_url.clone(),
+                        auth_tag: record.auth_tag.clone(),
+                        pubkey: record.pubkey.clone(),
+                        agent_command: record.agent_command.clone(),
+                        persona_id: record.persona_id.clone(),
+                    },
+                ))
+            })
+            .collect();
+
    save_managed_agents(app, &records)?;

+    // ── Profile reconciliation (fire-and-forget) ────────────────────────────
+    // Spawn background tasks to ensure each restored agent's kind:0 profile is
+    // published on the relay. Same pattern as the UI start path.
+    for (pubkey, data) in reconcile_items {
+        let reconcile_app = app.clone();
+        tauri::async_runtime::spawn(async move {
+            let state = reconcile_app.state::<AppState>();
+            if let Err(e) =
+                crate::commands::reconcile_agent_profile(&state, &reconcile_app, &pubkey, &data)
+                    .await
+            {
+                eprintln!("sprout-desktop: profile reconciliation failed for agent {pubkey}: {e}");
+            }
+        });
+    }
+
    Ok(())
 }