Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
54 changes: 26 additions & 28 deletions desktop/src-tauri/src/managed_agents/global_config/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -204,44 +204,42 @@ pub fn save_global_agent_config(app: &AppHandle, config: &GlobalAgentConfig) ->
atomic_write_json_restricted(&path, &payload)
}

/// Resolve the effective model and provider for an agent using the
/// precedence chain: `agent record → linked persona → global defaults → None`.
/// Resolve the effective model and provider for an agent.
///
/// This is the single source of truth used by readiness evaluation, spawn,
/// and deploy-payload construction. All three paths must use this function so
/// they agree on what model/provider the agent will actually run with.
/// Definition-linked instances resolve model through `definition → global → None`.
/// Their materialized record model is a snapshot, not an override, so consulting
/// it would let stale bytes beat a newly edited definition for one spawn.
/// Definition-less legacy instances resolve model through `record → global → None`.
/// Provider retains its existing `record → definition → global` precedence; this
/// launch fix deliberately does not change provider semantics.
///
/// # Arguments
/// * `record` — the `ManagedAgentRecord` (may have `None` for model/provider)
/// * `personas` — all current persona records (looked up by `record.persona_id`)
/// * `global` — global agent config defaults
///
/// # Returns
/// `(effective_model, effective_provider)` — both `Option<&str>`.
/// This is the single source of truth used by readiness evaluation and spawn.
/// Both paths must use this function so they agree on what model/provider the
/// agent will actually run with.
pub(crate) fn resolve_effective_model_provider<'a>(
record: &'a ManagedAgentRecord,
personas: &'a [AgentDefinition],
global: &'a GlobalAgentConfig,
) -> (Option<&'a str>, Option<&'a str>) {
let (persona_model, persona_provider) = record
if let Some(persona) = record
.persona_id
.as_deref()
.and_then(|pid| personas.iter().find(|p| p.id == pid))
.map(|p| (p.model.as_deref(), p.provider.as_deref()))
.unwrap_or((None, None));

let effective_model = record
.model
.as_deref()
.or(persona_model)
.or(global.model.as_deref());
let effective_provider = record
.provider
.as_deref()
.or(persona_provider)
.or(global.provider.as_deref());
.and_then(|pid| personas.iter().find(|persona| persona.id == pid))
{
return (
persona.model.as_deref().or(global.model.as_deref()),
record
.provider
.as_deref()
.or(persona.provider.as_deref())
.or(global.provider.as_deref()),
);
}

(effective_model, effective_provider)
(
record.model.as_deref().or(global.model.as_deref()),
record.provider.as_deref().or(global.provider.as_deref()),
)
}

#[cfg(test)]
Expand Down
84 changes: 56 additions & 28 deletions desktop/src-tauri/src/managed_agents/global_config/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -376,19 +376,19 @@ fn persona(id: &str, model: Option<&str>, provider: Option<&str>) -> AgentDefini
}
}

/// Tier 1 — agent record wins: record has explicit model/provider; they must
/// outrank both the linked persona and the global defaults. Fails against any
/// implementation that prefers global or persona over the record.
/// Linked definitions are authoritative for model: stale materialized record
/// bytes must not win over an edited definition on the next spawn. Provider
/// retains the existing record-first behavior.
#[test]
fn resolve_agent_record_wins_over_persona_and_global() {
fn resolve_linked_definition_model_wins_over_stale_record() {
let mut record = bare_record();
record.persona_id = Some("p1".to_string());
record.model = Some("record-model".to_string());
record.provider = Some("record-provider".to_string());
record.model = Some("stale-record-model".to_string());
record.provider = Some("stale-record-provider".to_string());
let personas = vec![persona(
"p1",
Some("persona-model"),
Some("persona-provider"),
Some("edited-definition-model"),
Some("edited-definition-provider"),
)];
let global = GlobalAgentConfig {
model: Some("global-model".to_string()),
Expand All @@ -398,12 +398,29 @@ fn resolve_agent_record_wins_over_persona_and_global() {

let (model, provider) = resolve_effective_model_provider(&record, &personas, &global);

assert_eq!(model, Some("record-model"), "record model must win");
assert_eq!(
provider,
Some("record-provider"),
"record provider must win"
);
assert_eq!(model, Some("edited-definition-model"));
assert_eq!(provider, Some("stale-record-provider"));
}

/// Clearing a linked definition model means inherit global; stale materialized
/// record bytes must not survive the first restart.
#[test]
fn resolve_linked_definition_model_clear_uses_global_over_stale_record() {
let mut record = bare_record();
record.persona_id = Some("p1".to_string());
record.model = Some("stale-record-model".to_string());
record.provider = Some("stale-record-provider".to_string());
let personas = vec![persona("p1", None, None)];
let global = GlobalAgentConfig {
model: Some("global-model".to_string()),
provider: Some("global-provider".to_string()),
..Default::default()
};

let (model, provider) = resolve_effective_model_provider(&record, &personas, &global);

assert_eq!(model, Some("global-model"));
assert_eq!(provider, Some("stale-record-provider"));
}

/// Tier 2 — persona fallback: record has no model/provider; the linked
Expand Down Expand Up @@ -543,18 +560,15 @@ fn resolve_all_none_when_no_source_provides_values() {
);
}

/// Partial tier — record has model but not provider; persona has provider but
/// not model; global has both. Each field resolves independently through the
/// three-tier chain.
/// Linked fields resolve independently: a blank definition model inherits the
/// global model even when the record still carries stale bytes, while provider
/// retains its existing record → definition → global chain.
#[test]
fn resolve_each_field_resolves_independently_through_tiers() {
fn resolve_linked_fields_use_their_intended_precedence() {
let mut record = bare_record();
record.persona_id = Some("p1".to_string());
record.model = Some("record-model".to_string());
// record.provider = None → falls through to persona
record.model = Some("stale-record-model".to_string());
let personas = vec![persona("p1", None, Some("persona-provider"))];
// persona.model = None → global fills model if record also had none, but
// record has model here so global is not needed for model.
let global = GlobalAgentConfig {
model: Some("global-model".to_string()),
provider: Some("global-provider".to_string()),
Expand All @@ -563,12 +577,26 @@ fn resolve_each_field_resolves_independently_through_tiers() {

let (model, provider) = resolve_effective_model_provider(&record, &personas, &global);

assert_eq!(model, Some("record-model"), "record wins for model");
assert_eq!(
provider,
Some("persona-provider"),
"persona wins for provider when record has none"
);
assert_eq!(model, Some("global-model"));
assert_eq!(provider, Some("persona-provider"));
}

/// Definition-less instances retain their own explicit values over globals.
#[test]
fn resolve_definition_less_record_wins_over_global() {
let mut record = bare_record();
record.model = Some("record-model".to_string());
record.provider = Some("record-provider".to_string());
let global = GlobalAgentConfig {
model: Some("global-model".to_string()),
provider: Some("global-provider".to_string()),
..Default::default()
};

let (model, provider) = resolve_effective_model_provider(&record, &[], &global);

assert_eq!(model, Some("record-model"));
assert_eq!(provider, Some("record-provider"));
}

// ── IPC serialization ─────────────────────────────────────────────────────────
Expand Down
14 changes: 8 additions & 6 deletions desktop/src-tauri/src/managed_agents/persona_events.rs
Original file line number Diff line number Diff line change
Expand Up @@ -451,12 +451,14 @@ pub fn persona_snapshot_with_agent_config_fallback(
}
}

/// Re-pin `record` to `persona`: build the snapshot (via
/// [`persona_snapshot_with_agent_config_fallback`], so blank persona
/// `model`/`provider` preserve the record's own values) and mirror it onto the
/// Re-pin `record` to `persona`: build the snapshot and mirror it onto the
/// record — the definition quad (`system_prompt`/`model`/`provider`/`runtime`),
/// the env-override self-heal, and the `persona_source_version` drift basis.
///
/// Model is definition-authoritative: a blank definition model clears stale
/// materialized bytes so spawn can resolve the current global default. Provider
/// retains its existing blank-definition fallback behavior.
///
/// This is the single apply used by every snapshot-apply site: the spawn
/// re-pin (`start_local_agent_with_preflight`), the launch backfill and
/// restore re-snapshot (`restore.rs`), and the prospective re-snapshot inside
Expand All @@ -469,13 +471,13 @@ pub fn persona_snapshot_with_agent_config_fallback(
pub fn apply_persona_snapshot(record: &mut ManagedAgentRecord, persona: &AgentDefinition) {
let snapshot = persona_snapshot_with_agent_config_fallback(
persona,
record.model.as_deref(), // fallback: record.model
record.provider.as_deref(), // fallback: record.provider
record.model.as_deref(),

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Non-blocking: persona_snapshot_with_agent_config_fallback still computes snapshot.model from the record fallback, but it's now dead — only snapshot.provider is consumed below. Dropping the model arg (or passing None) would make the code match the new doc comment instead of computing-then-discarding.

record.provider.as_deref(),
);
if let Some(prompt) = snapshot.system_prompt {
record.system_prompt = Some(prompt);
}
record.model = snapshot.model;
record.model = persona.model.clone();

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Blocking: this assignment can't distinguish stale materialized inheritance bytes from an explicit per-instance model the user set via update_managed_agent() (commands/agent_models.rs:807-809) or the ModelPicker persist path (ModelPicker.tsx:156-158 — whose comment calls it "the only reachable lever from this surface"). Every snapshot-apply site (spawn re-pin, restore, launch backfill) now wipes an explicit pick back to persona.model, and the restart badge never trips because spawn_config_hash applies the same prospective snapshot — hash before and after the user's pick is identical. Net: user picks a model, gets no badge, restarts, silently runs something else.

There's also an asymmetry hazard: provider keeps its record fallback, so an explicit provider survives restart while its paired model is wiped — the agent can respawn with a mismatched combo (e.g. user-set provider=openai + a global default Anthropic model).

Either preserve explicit instance models (needs provenance the record can't currently express — the #1968 territory this PR carves around), or gate/redirect the ModelPicker persist path for definition-linked agents in the same PR. Worth tests for both cases: stale inherited model converges to the new global default, and an explicit instance model survives restart.

record.provider = snapshot.provider;
record.runtime = snapshot.runtime;
// env_vars stay overrides-only. Self-heal records written before the env
Expand Down
27 changes: 27 additions & 0 deletions desktop/src-tauri/src/managed_agents/spawn_hash/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,33 @@ fn persona(id: &str, runtime: Option<&str>, prompt: &str) -> AgentDefinition {
}
}

#[test]
fn inherited_persona_model_converges_after_one_restart() {
let mut rec = record();
rec.persona_id = Some("persona".into());
rec.system_prompt = Some("prompt".into());
rec.model = Some("stale-model".into());
let definition = persona("persona", Some("goose"), "prompt");
let global = GlobalAgentConfig {
model: Some("new-default-model".into()),
..Default::default()
};

let hash_before_restart = spawn_config_hash(
&rec,
std::slice::from_ref(&definition),
&[],
"wss://ws.example",
&global,
);
apply_persona_snapshot(&mut rec, &definition);
assert_eq!(rec.model, None);
let hash_after_restart =
spawn_config_hash(&rec, &[definition], &[], "wss://ws.example", &global);

assert_eq!(hash_before_restart, hash_after_restart);
}

#[test]
fn hash_is_deterministic() {
let rec = record();
Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,10 @@
import assert from "node:assert/strict";
import test from "node:test";

import { startManagedAgentWithRules } from "./managedAgentControlActions.ts";
import {
respawnManagedAgentWithRules,
startManagedAgentWithRules,
} from "./managedAgentControlActions.ts";

function agent(overrides = {}) {
return {
Expand Down Expand Up @@ -77,3 +80,19 @@ test("ordinary local agents still start normally", async () => {
});
assert.equal(calledWith, "deadbeef".repeat(8));
});

test("running local agents stop before restart", async () => {
const calls = [];
const runningAgent = agent({ status: "running" });

await respawnManagedAgentWithRules({
agent: runningAgent,
stopManagedAgent: async (pubkey) => calls.push(["stop", pubkey]),
startManagedAgent: async (pubkey) => calls.push(["start", pubkey]),
});

assert.deepEqual(calls, [
["stop", runningAgent.pubkey],
["start", runningAgent.pubkey],
]);
});
21 changes: 13 additions & 8 deletions desktop/src/features/profile/ui/UserProfilePanel.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ import {
} from "@/features/agents/lib/instanceInputForDefinition";
import {
isManagedAgentActive,
respawnManagedAgentWithRules,
startManagedAgentWithRules,
stopManagedAgentWithRules,
} from "@/features/agents/lib/managedAgentControlActions";
Expand Down Expand Up @@ -451,10 +452,19 @@ export function UserProfilePanel({
],
);

const handleAgentPrimaryAction = React.useCallback(async () => {
const handleAgentPrimaryAction = async (restart = false) => {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Blocking: this function is passed unchanged as the primary-action prop (line 834) and ends up installed directly as the button's onClick in ProfileQuickAction, so React invokes it with the click SyntheticEvent as the first argument — restart becomes a truthy event object, not false. For a running local agent, clicking Stop takes the restart branch, stops-then-starts the agent, and toasts "Restarted". TypeScript can't catch this since (restart?: boolean) => void is assignable to () => void.

Suggest splitting into two zero-arg named callbacks (or wrapping the prop: handleAgentPrimaryAction={() => void handleAgentPrimaryAction()}), plus a UI-level regression test asserting Stop only stops while Restart stops-then-starts — the new unit tests only cover the respawnManagedAgentWithRules helper, not this wiring.

if (!managedAgent) return;

try {
if (restart && managedAgent.backend.type === "local") {
await respawnManagedAgentWithRules({
agent: managedAgent,
startManagedAgent: startAgentMutation.mutateAsync,
stopManagedAgent: stopAgentMutation.mutateAsync,
});
toast.success(`Restarted ${managedAgent.name}.`);
return;
}
if (isManagedAgentActive(managedAgent)) {
const result = await stopManagedAgentWithRules({
agent: managedAgent,
Expand All @@ -480,13 +490,7 @@ export function UserProfilePanel({
error instanceof Error ? error.message : "Agent action failed.",
);
}
}, [
channelsQuery.data,
managedAgent,
relayAgentsQuery.data,
startAgentMutation.mutateAsync,
stopAgentMutation.mutateAsync,
]);
};

const handleInstantiateAgent = React.useCallback(async () => {
if (!resolvedPersona) return;
Expand Down Expand Up @@ -828,6 +832,7 @@ export function UserProfilePanel({
followMutation={followMutation}
agentInstruction={agentInstruction}
handleAgentPrimaryAction={handleAgentPrimaryAction}
handleAgentRestart={() => void handleAgentPrimaryAction(true)}
handleEditAgent={handleEditAgent}
handleEditPersona={canEditPersona ? handleEditPersona : undefined}
handleInstantiateAgent={handleInstantiateAgent}
Expand Down
Loading
Loading