Skip to content

fix: re-read persona system_prompt at spawn time for non-pack agents #729

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
tellaho wants to merge 5 commits into
base: main
Choose a base branch
from
Open

fix: re-read persona system_prompt at spawn time for non-pack agents #729

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
3ed474a
fix: re-read persona system_prompt at spawn time for non-pack agents
tellaho May 22, 2026
49b60d6
fix: apply persona resolution to provider deploys + tighten missing-p…
tellaho May 27, 2026
848d982
feat: implement Option A precedence — agent-level overrides win over …
tellaho May 27, 2026
f81f73f
fix: migrate existing persona-backed agents to clear stale prompt/mod…
tellaho May 27, 2026
febe55e
perf: move persona-defaults migration to one-shot startup path
tellaho May 28, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion desktop/scripts/check-file-sizes.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ const overrides = new Map([
["src-tauri/src/commands/agents.rs", 881], // remote agent lifecycle routing (local + provider branches) + scope enforcement + persona pack metadata wiring + mcp_toolsets field + NIP-OA auth_tag in deploy payload
["src-tauri/src/commands/messages.rs", 515], // feed multi-query + NIP-50 search + forum thread resolution + thread ref + reactions via REQ + edit_message media_tags param (Slack-style attachment-editable edits)
["src-tauri/src/nostr_convert.rs", 1150], // 12 Nostr event→model converters (channels, profiles, members, notes, search, agents, relay members) + rank_user_search_results helper for NIP-50 user search + 33 unit tests
["src-tauri/src/managed_agents/runtime.rs", 1110], // ... + respond-to gate env (SPROUT_ACP_RESPOND_TO[_ALLOWLIST]) + per-mode env builder + tests + persona/agent env_vars spawn merge (helper + tests now in env_vars.rs)
["src-tauri/src/managed_agents/runtime.rs", 915], // spawn logic + respond-to env builder + persona resolution helper + Option A precedence resolver (tests extracted to runtime/tests.rs)
["src-tauri/src/managed_agents/discovery.rs", 680], // KNOWN_ACP_PROVIDERS catalog + resolve_command cache + login_shell_path + classify_provider (four-state: Available/AdapterMissing/CliMissing/NotInstalled) + discover_acp_providers with dynamic install_hint + known_acp_provider/known_acp_provider_exact + normalize_agent_args + 15 unit tests
["src-tauri/src/managed_agents/types.rs", 745], // ManagedAgentRecord/Summary + Create/Update request structs + AcpProviderCatalogEntry + InstallRuntimeResult + RespondTo enum + validate_respond_to_allowlist + tests + persona/agent env_vars field
["src-tauri/src/managed_agents/backend.rs", 700], // provider IPC, validation, discovery, binary resolution + tests + redact_secrets_with for user env values + env_secrets_from_request + redact_env_values_in (shared with model discovery)
Expand Down
59 changes: 45 additions & 14 deletions desktop/src-tauri/src/commands/agents.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,15 +44,32 @@ fn build_deploy_payload(
crate::managed_agents::resolve_persona_env(app, record.persona_id.as_deref())?;
let merged_env = crate::managed_agents::merged_user_env(&persona_env, &record.env_vars);

// Resolve system prompt and model via the persona (same as local spawn).
// Re-reads the persona store so edits take effect without recreating the agent.
let personas = load_personas(app).unwrap_or_else(|e| {
eprintln!(
"sprout-desktop: failed to load personas for deploy payload (agent {}) — proceeding without persona context: {e}",
record.name
);
Vec::new()
});
let (effective_prompt, effective_model) =
crate::managed_agents::resolve_effective_prompt_and_model(
record.persona_id.as_deref(),
&personas,
record.system_prompt.as_deref(),
record.model.as_deref(),
);

Ok(serde_json::json!({
"name": &record.name,
"relay_url": &record.relay_url,
"private_key_nsec": &record.private_key_nsec,
"auth_tag": &record.auth_tag,
"agent_command": &record.agent_command,
"agent_args": &record.agent_args,
"system_prompt": &record.system_prompt,
"model": &record.model,
"system_prompt": &effective_prompt,
"model": &effective_model,
"turn_timeout_seconds": record.turn_timeout_seconds,
"idle_timeout_seconds": record.idle_timeout_seconds,
"max_turn_duration_seconds": record.max_turn_duration_seconds,
Expand Down Expand Up @@ -392,18 +409,32 @@ pub async fn create_managed_agent(
.parallelism
.filter(|count| (1..=32).contains(count))
.unwrap_or(DEFAULT_AGENT_PARALLELISM),
system_prompt: input
.system_prompt
.as_deref()
.map(str::trim)
.filter(|value| !value.is_empty())
.map(str::to_string),
model: input
.model
.as_deref()
.map(str::trim)
.filter(|value| !value.is_empty())
.map(str::to_string),
// When persona_id is set, do NOT snapshot the persona's system_prompt
// or model onto the agent record. These fields are reserved for explicit
// per-agent overrides set via the Edit Agent dialog. At spawn time,
// resolve_effective_prompt_and_model() reads the live persona values as
// the default and only uses agent-record values when they're Some (i.e.
// the user explicitly set an override).
system_prompt: if requested_persona_id.is_some() {
None
} else {
input
.system_prompt
.as_deref()
.map(str::trim)
.filter(|value| !value.is_empty())
.map(str::to_string)
},
model: if requested_persona_id.is_some() {
None
} else {
input
.model
.as_deref()
.map(str::trim)
.filter(|value| !value.is_empty())
.map(str::to_string)
},
mcp_toolsets: input
.mcp_toolsets
.as_deref()
Expand Down
13 changes: 10 additions & 3 deletions desktop/src-tauri/src/managed_agents/restore.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
use super::{
find_managed_agent_mut, kill_stale_tracked_processes, load_managed_agents, save_managed_agents,
spawn_agent_child, sync_managed_agent_processes, BackendKind, ManagedAgentProcess,
find_managed_agent_mut, kill_stale_tracked_processes, load_managed_agents, load_personas,
save_managed_agents, spawn_agent_child, storage::migrate_clear_persona_defaults,
sync_managed_agent_processes, BackendKind, ManagedAgentProcess,
};
use crate::app_state::AppState;
use crate::util;
Expand Down Expand Up @@ -36,11 +37,17 @@ pub fn restore_managed_agents_on_launch(
}

let mut records = load_managed_agents(app)?;

// One-shot migration: clear persona-default snapshots only when the
// stored value still matches the persona's current default.
let personas = load_personas(app).unwrap_or_default();
let mut changed = migrate_clear_persona_defaults(&mut records, &personas);

let mut runtimes = state
.managed_agent_processes
.lock()
.map_err(|error| error.to_string())?;
let mut changed = sync_managed_agent_processes(&mut records, &mut runtimes);
changed |= sync_managed_agent_processes(&mut records, &mut runtimes);
changed |= kill_stale_tracked_processes(&mut records, &runtimes);

let tracked_pids: Vec<u32> = records
Expand Down
253 changes: 64 additions & 189 deletions desktop/src-tauri/src/managed_agents/runtime.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -500,6 +500,53 @@ pub(crate) fn build_respond_to_env(
Ok((set, remove))
}

/// Resolve the effective system prompt and model for a managed agent.
///
/// Precedence (Option A — agent override wins when explicitly set):
/// 1. If the agent record has an explicit `system_prompt` / `model` → use it.
/// These are only populated when a user explicitly sets an override via the
/// Edit Agent dialog (persona-backed agents have these fields cleared at
/// creation time to avoid stale snapshots).
/// 2. Else if `persona_id` resolves to a persona → use the persona's live values.
/// 3. Else → None (no prompt / no model).
///
/// This matches how env_vars work: per-agent overrides persona on collision.
///
/// Extracted from `spawn_agent_child` for testability.
pub(crate) fn resolve_effective_prompt_and_model(
persona_id: Option<&str>,
personas: &[super::PersonaRecord],
record_prompt: Option<&str>,
record_model: Option<&str>,
) -> (Option<String>, Option<String>) {
// Agent-record values win when explicitly set (user override via Edit dialog).
let effective_prompt = if record_prompt.is_some() {
record_prompt.map(|s| s.to_owned())
} else {
// Fall back to persona's live value.
persona_id.and_then(|pid| {
personas
.iter()
.find(|p| p.id == pid)
.map(|p| p.system_prompt.clone())
})
};

let effective_model = if record_model.is_some() {
record_model.map(|s| s.to_owned())
} else {
// Fall back to persona's live value.
persona_id.and_then(|pid| {
personas
.iter()
.find(|p| p.id == pid)
.and_then(|p| p.model.clone())
})
};

(effective_prompt, effective_model)
}

/// Spawn an agent process without holding any locks on records or runtimes.
/// Returns the child process and log path on success. The caller is responsible
/// for updating `ManagedAgentRecord` fields and inserting into the runtimes map.
Expand Down Expand Up @@ -622,30 +669,22 @@ pub fn spawn_agent_child(
command.env("SPROUT_ACP_PERSONA_NAME", persona_name);
}

// Resolve system prompt and model: prefer the persona definition (if a
// persona pack is configured and the persona matched), otherwise fall back
// to the record-level overrides.
let has_persona_pack =
record.persona_pack_path.is_some() && record.persona_name_in_pack.is_some();
let persona_prompt_and_model: Option<(String, Option<String>)> = has_persona_pack
.then(|| {
record
.persona_id
.as_deref()
.and_then(|pid| {
super::load_personas(app)
.ok()?
.into_iter()
.find(|p| p.id == pid)
})
.map(|p| (p.system_prompt, p.model))
})
.flatten();

let (effective_prompt, effective_model) = match persona_prompt_and_model {
Some((prompt, model)) => (Some(prompt), model),
None => (record.system_prompt.clone(), record.model.clone()),
};
// Resolve system prompt and model via the extracted helper. Re-reads
Copy link
Copy Markdown
Collaborator

@wesbillman wesbillman May 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 This re-read is only wired into local spawn_agent_child(). Provider-backed starts/redeploys still build their payload from record.system_prompt and record.model in build_deploy_payload(), so persona edits will not propagate there. If provider agents are in scope, this effective prompt/model resolver should be shared with the deploy payload path too; if not, the PR description should call out that the fix is local-only.

Copy link
Copy Markdown
Collaborator Author

@tellaho tellaho May 28, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Resolved in e29c895c — wired resolve_effective_prompt_and_model into build_deploy_payload() so provider-backed agents also pick up live persona values (with the same Option A precedence: agent override wins when set).

// the persona store at spawn time so edits take effect without
// deleting and recreating the agent.
let personas = super::load_personas(app).unwrap_or_else(|e| {
eprintln!(
"sprout-desktop: failed to load personas for agent {} — spawning with no persona context: {e}",
record.name
);
Vec::new()
});
let (effective_prompt, effective_model) = resolve_effective_prompt_and_model(
record.persona_id.as_deref(),
&personas,
record.system_prompt.as_deref(),
record.model.as_deref(),
);

if let Some(prompt) = &effective_prompt {
command.env("SPROUT_ACP_SYSTEM_PROMPT", prompt);
Expand Down Expand Up @@ -870,168 +909,4 @@ pub fn stop_managed_agent_process(
}

#[cfg(test)]
mod tests {
use crate::managed_agents::known_acp_provider;

#[test]
fn sprout_agent_has_mcp_hooks() {
let p = known_acp_provider("sprout-agent").expect("should resolve");
assert!(p.mcp_hooks);
assert_eq!(p.mcp_command, Some("sprout-dev-mcp"));
}

#[test]
fn sprout_agent_resolved_via_path() {
assert!(known_acp_provider("/usr/local/bin/sprout-agent").is_some_and(|p| p.mcp_hooks));
}

#[test]
fn goose_has_no_mcp_hooks() {
let p = known_acp_provider("goose").expect("should resolve");
assert!(!p.mcp_hooks);
assert_eq!(p.mcp_command, None);
}

#[test]
fn unknown_command_returns_none() {
assert!(known_acp_provider("custom-agent").is_none());
}

// ── build_respond_to_env tests ───────────────────────────────────────

use super::build_respond_to_env;
use crate::managed_agents::types::{ManagedAgentRecord, RespondTo};

/// Construct a minimal record fixture for env-building tests. Only the
/// fields read by `build_respond_to_env` matter here.
fn fixture(
respond_to: RespondTo,
allowlist: Vec<String>,
auth_tag: Option<String>,
) -> ManagedAgentRecord {
ManagedAgentRecord {
pubkey: "p".into(),
name: "n".into(),
persona_id: None,
private_key_nsec: "nsec1fake".into(),
auth_tag,
relay_url: "ws://localhost:3000".into(),
acp_command: "sprout-acp".into(),
agent_command: "goose".into(),
agent_args: vec![],
mcp_command: "sprout-mcp-server".into(),
turn_timeout_seconds: 320,
idle_timeout_seconds: None,
max_turn_duration_seconds: None,
parallelism: 1,
system_prompt: None,
model: None,
mcp_toolsets: None,
env_vars: std::collections::BTreeMap::new(),
start_on_app_launch: false,
runtime_pid: None,
backend: Default::default(),
backend_agent_id: None,
provider_binary_path: None,
persona_pack_path: None,
persona_name_in_pack: None,
created_at: "now".into(),
updated_at: "now".into(),
last_started_at: None,
last_stopped_at: None,
last_exit_code: None,
last_error: None,
respond_to,
respond_to_allowlist: allowlist,
}
}

#[test]
fn build_env_owner_only_sets_mode_and_removes_others() {
let rec = fixture(RespondTo::OwnerOnly, vec![], Some("tag".into()));
let (set, remove) = build_respond_to_env(&rec, Some("owner")).unwrap();
let set_map: std::collections::HashMap<_, _> = set.into_iter().collect();
assert_eq!(
set_map.get("SPROUT_ACP_RESPOND_TO").map(String::as_str),
Some("owner-only")
);
assert!(!set_map.contains_key("SPROUT_ACP_RESPOND_TO_ALLOWLIST"));
assert!(remove.contains(&"SPROUT_ACP_RESPOND_TO_ALLOWLIST"));
// auth_tag is present → no AGENT_OWNER fallback fires.
assert!(remove.contains(&"SPROUT_ACP_AGENT_OWNER"));
}

#[test]
fn build_env_allowlist_sets_both_envs_and_joins() {
let a = "a".repeat(64);
let b = "b".repeat(64);
let rec = fixture(
RespondTo::Allowlist,
vec![a.clone(), b.clone()],
Some("tag".into()),
);
let (set, _remove) = build_respond_to_env(&rec, Some("owner")).unwrap();
let set_map: std::collections::HashMap<_, _> = set.into_iter().collect();
assert_eq!(
set_map.get("SPROUT_ACP_RESPOND_TO").map(String::as_str),
Some("allowlist")
);
assert_eq!(
set_map
.get("SPROUT_ACP_RESPOND_TO_ALLOWLIST")
.map(String::as_str),
Some(format!("{a},{b}").as_str()),
);
}

#[test]
fn build_env_anyone_omits_allowlist_var() {
let rec = fixture(RespondTo::Anyone, vec![], Some("tag".into()));
let (set, remove) = build_respond_to_env(&rec, Some("owner")).unwrap();
let set_map: std::collections::HashMap<_, _> = set.into_iter().collect();
assert_eq!(
set_map.get("SPROUT_ACP_RESPOND_TO").map(String::as_str),
Some("anyone")
);
assert!(!set_map.contains_key("SPROUT_ACP_RESPOND_TO_ALLOWLIST"));
assert!(remove.contains(&"SPROUT_ACP_RESPOND_TO_ALLOWLIST"));
}

#[test]
fn build_env_legacy_record_without_auth_tag_emits_agent_owner() {
let rec = fixture(RespondTo::OwnerOnly, vec![], None);
let (set, remove) = build_respond_to_env(&rec, Some("ownerhex")).unwrap();
let set_map: std::collections::HashMap<_, _> = set.into_iter().collect();
assert_eq!(
set_map.get("SPROUT_ACP_AGENT_OWNER").map(String::as_str),
Some("ownerhex")
);
assert!(!remove.contains(&"SPROUT_ACP_AGENT_OWNER"));
}

#[test]
fn build_env_legacy_record_without_owner_hex_removes_agent_owner() {
// No owner available to forward → make sure we don't inherit a leaked
// env var from the parent.
let rec = fixture(RespondTo::OwnerOnly, vec![], None);
let (_set, remove) = build_respond_to_env(&rec, None).unwrap();
assert!(remove.contains(&"SPROUT_ACP_AGENT_OWNER"));
}

#[test]
fn build_env_rejects_corrupted_allowlist() {
let rec = fixture(
RespondTo::Allowlist,
vec!["not-hex".into()],
Some("tag".into()),
);
assert!(build_respond_to_env(&rec, Some("owner")).is_err());
}

#[test]
fn build_env_rejects_empty_allowlist_in_allowlist_mode() {
let rec = fixture(RespondTo::Allowlist, vec![], Some("tag".into()));
let err = build_respond_to_env(&rec, Some("owner")).unwrap_err();
assert!(err.contains("at least one pubkey"));
}
}
mod tests;
Loading
Loading