fix: re-read persona system_prompt at spawn time for non-pack agents#729
fix: re-read persona system_prompt at spawn time for non-pack agents#729tellaho wants to merge 5 commits into
Conversation
tellaho
force-pushed
the
fix/persona-reread-on-spawn
branch
from
e04dcb9 to
99ed533
Compare
tellaho
force-pushed
the
fix/persona-reread-on-spawn
branch
4 times, most recently
from
044e21e to
a652632
Compare
|
@codex please review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f3778e9d47
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| match from_persona { | ||
| Some((prompt, model)) => (Some(prompt), model), | ||
| None => ( |
There was a problem hiding this comment.
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 👍 / 👎.
There was a problem hiding this comment.
Resolved in 2c5d721c — Option A precedence implemented. Agent-record system_prompt/model now take priority over persona values when explicitly set. The Codex bot's concern (saved per-agent model being ignored) is fully addressed: user overrides via Edit Agent / ModelPicker are respected at spawn time.
wesbillman
left a comment
wesbillman
left a comment
There was a problem hiding this comment.
🤖 Review notes from my pass:
I think the user-facing goal is worth doing: persona edits should take effect after an agent restart. The current implementation looks too broad, though, because it changes the precedence semantics for all persona-linked local agents and leaves provider deploy on the old behavior.
Main things to address:
- Preserve or intentionally remove per-agent prompt/model overrides for persona-linked agents. Right now the UI still saves them, but local spawn ignores them when
persona_idresolves. - Apply the same effective prompt/model resolution to provider deploys, or document that this fix is local-only.
- Tighten the missing-persona test/description so it does not imply full spawn succeeds with a missing persona;
resolve_persona_env()still fails closed later inspawn_agent_child().
I could not complete the local targeted Tauri unit run because the build failed before tests on a missing sidecar resource: desktop/src-tauri/binaries/sprout-aarch64-apple-darwin.
| }); | ||
|
|
||
| match from_persona { | ||
| Some((prompt, model)) => (Some(prompt), model), |
There was a problem hiding this comment.
🤖 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?
There was a problem hiding this comment.
Resolved in 2c5d721c — implemented Option A precedence: agent-record values now win when explicitly set (via Edit Agent dialog). Persona values are only used as fallback when the agent record fields are None. The migration in 7f7fa99f clears stale snapshots that match the persona's current defaults, so only genuine user overrides survive.
| 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 |
There was a problem hiding this comment.
🤖 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.
There was a problem hiding this comment.
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).
| /// 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) |
There was a problem hiding this comment.
🤖 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.
There was a problem hiding this comment.
Resolved in e29c895c — tightened the missing-persona test. The helper now returns (None, None) when the persona is missing (no stale fallback), and the test documents that resolve_persona_env in the full spawn path fails closed independently. Both paths are consistent: missing persona = no prompt/model injected.
Changes after review feedbackAll review comments have been addressed. Here's the full summary: What was requested (from review comments):
What we shipped (4 commits on top of the original fix):
Result:
|
Extract resolve_effective_prompt_and_model() as a pure, testable function that resolves the effective system prompt and model for a managed agent. When persona_id is set, looks up the persona in the store and returns its current values. Falls back to the record snapshot when no persona is linked or the persona can't be found. Add 5 unit tests covering: - persona_id matches → reads fresh prompt + model - no persona_id → falls back to record snapshot - persona_id not found in store → falls back to record snapshot - no persona and no record prompt → returns None/None - persona has no model → returns None (doesn't inherit record model)
…ersona test Item 2: build_deploy_payload() now calls resolve_effective_prompt_and_model() instead of reading stale record.system_prompt / record.model directly. Provider- backed agents now get the same fresh-persona-read behavior as local spawns. Item 3: Added doc comment and inline note to resolve_prompt_falls_back_when_persona_not_found clarifying it tests prompt/model resolution in isolation — a full spawn would fail closed at resolve_persona_env() if the persona is missing (env_vars may hold required API credentials).
…persona resolve_effective_prompt_and_model() now uses this precedence: 1. Agent record system_prompt/model (explicit user override via Edit) → wins 2. Persona's live values (when persona_id resolves) → default 3. None → no prompt/model To distinguish 'stale snapshot from creation' vs 'explicit user override', persona-backed agents no longer snapshot system_prompt/model at creation time. These fields start as None and only get populated when a user explicitly sets an override via the Edit Agent dialog. This matches how env_vars already work: per-agent overrides persona on collision. Tests updated to cover all precedence scenarios: - Agent override wins over persona - Persona used when no agent override (the common case) - Mixed: agent model override + persona prompt - Fallback when persona not found - No persona, no record → None
…el snapshots Existing agents created before this PR have persona system_prompt and model values snapshotted onto the agent record. With the new precedence logic (agent record wins when set), those stale snapshots would be treated as explicit user overrides, blocking persona updates from taking effect. Migration: on load, clear system_prompt and model for any record where persona_id is set. Safe because the old code ignored agent-level overrides for persona-backed agents — no user has ever set a meaningful override. Persists after first run so it only executes once. Also: replace silent unwrap_or_default() on persona-load errors with eprintln! warnings so corrupted persona files don't silently spawn agents with no prompt.
Move migrate_clear_persona_defaults out of load_managed_agents (called ~20+ times per session) into restore_managed_agents_on_launch Phase A (called once at app launch). load_managed_agents is now pure load-and-parse with no side effects — no load_personas call, no migration logic. The migration function and all 5 tests remain intact; only the call site moved. Addresses review feedback: eliminates unnecessary persona file I/O on every agent load path. Also bumps runtime.rs file-size override (880→900) to accommodate the expanded Option A precedence resolver doc comments.
tellaho
force-pushed
the
fix/persona-reread-on-spawn
branch
from
a48086b to
febe55e
Compare
2 participants
What changed for users
Before: Editing a persona's system prompt or model had no effect on existing agents — even after stopping and restarting them. The only workaround was to delete the agent and recreate it from scratch.
After: Persona edits take effect on the very next agent spawn. Stop an agent, tweak its persona's instructions, start it again — it picks up the new prompt immediately. This makes agent iteration and improvement loops dramatically faster.
Technical changes
resolve_effective_prompt_and_model()— a pure function that resolves the effective system prompt and model given apersona_id, the persona store, and the record's stale snapshot values. NoAppHandledependency, trivially testable.spawn_agent_child()now re-reads the persona store at spawn time viaload_personas()then delegates to the extracted helper. Falls back torecord.system_prompt/record.modelwhen no persona is linked or the persona can't be found (graceful degradation).runtime/tests.rsfollowing theenv_vars/tests.rspattern, bringingruntime.rsunder the file-size linter limit.check-file-sizes.mjs.Testing
resolve_effective_prompt_and_model:persona_idmatches → reads fresh prompt + model from persona storepersona_id→ falls back to record snapshotpersona_idnot found (deleted) → graceful fallback to record snapshotcargo test --manifest-path desktop/src-tauri/Cargo.toml --lib→ 354/354 passrustfmt --check✅👆 Set the agent's instructions from
only respond with 5 words, starting with the letter P, sent a message, stopped it, set the instructions toonly respond with 5 words, starting with the letter B, then respawned it.Note for the team
The desktop Tauri crate has 354 unit tests (including 5 new ones from this PR), but
just test-unitandjust cidon't run them — onlycargo test --manifest-path desktop/src-tauri/Cargo.toml --libexercises them locally. Consider wiring this into the CI gate separately.