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 2 commits into
base: main
Choose a base branch
from
+300 −190
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
2 commits
Select commit Hold shift + click to select a range
a652632
fix: re-read persona system_prompt at spawn time for non-pack agents
tellaho May 22, 2026
f3778e9
Merge branch 'main' into fix/persona-reread-on-spawn
tellaho May 26, 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 @@ -50,7 +50,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", 510], // feed multi-query + NIP-50 search + forum thread resolution + thread ref + reactions via REQ
["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", 895], // spawn logic + respond-to env builder + persona resolution helper (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
231 changes: 42 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,37 @@ pub(crate) fn build_respond_to_env(
Ok((set, remove))
}

/// Resolve the effective system prompt and model for a managed agent.
///
/// When `persona_id` is set, looks up the persona in the provided list and
/// returns its current `system_prompt` and `model`. Falls back to the
/// agent record's stale snapshot (`record_prompt`, `record_model`) when:
/// - `persona_id` is `None`
/// - the persona can't be found in the store (e.g. deleted)
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 fallback is true for the pure helper, but full spawn still calls resolve_persona_env(app, record.persona_id.as_deref())? later, which fails closed when persona_id is set and missing. The test/description around the missing-persona case may overstate runtime behavior unless the env resolution path is also adjusted.

///
/// 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>) {
let from_persona: Option<(String, Option<String>)> = persona_id.and_then(|pid| {
personas
.iter()
.find(|p| p.id == pid)
.map(|p| (p.system_prompt.clone(), p.model.clone()))
});

match from_persona {
Some((prompt, model)) => (Some(prompt), model),
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 makes the persona prompt/model always win whenever persona_id resolves, but the existing app still treats ManagedAgentRecord.system_prompt and .model as mutable per-agent settings. EditAgentDialog persists systemPrompt, and ModelPicker persists model; after this change those saved values become no-ops for persona-linked agents. In particular, if a persona has model: None, selecting a model on that agent will still save/display but spawn will remove SPROUT_ACP_MODEL. Can we either preserve explicit agent override precedence, or update the UI/data model so persona-backed agents cannot appear to have per-agent prompt/model overrides?

None => (
Comment on lines +525 to +527
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot May 26, 2026

Choose a reason for hiding this comment

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

P1 Badge Preserve per-agent model overrides for persona-linked agents

When a persona_id is present, this branch always returns the persona's model (including None) and never uses record_model, so a saved per-agent model choice is ignored at spawn time for non-pack persona agents. In practice, if a persona has no model set and the user selects one via update_managed_agent/ModelPicker, the value is persisted on the record but SPROUT_ACP_MODEL is removed on restart, causing the runtime default model to be used instead of the selected one.

Useful? React with 👍 / 👎.

record_prompt.map(|s| s.to_owned()),
record_model.map(|s| s.to_owned()),
),
}
}

/// 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 @@ -623,30 +654,16 @@ 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.

// the persona store at spawn time so edits take effect without
// deleting and recreating the agent.
let personas = super::load_personas(app).unwrap_or_default();
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 @@ -871,168 +888,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