From 83545f1b867ba33c498bf3201e948c3f0db0c405 Mon Sep 17 00:00:00 2001 From: npub1qyvc0c5kl4gqv2fd97fsk46tu378sqgy35vc83rvgfwne90sel7s0ed67d <011987e296fd5006292d2f930b574be47c7801048d1983c46c425d3c95f0cffd@buzz.block.builderlab.xyz> Date: Sun, 26 Jul 2026 13:38:53 -0400 Subject: [PATCH] test(agent-models): bind linked-agent regression to get_agent_models' seam MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Extract agent_model_discovery_config — a pure helper returning the command/args/model/provider/provider_env_var/env that get_agent_models consumes (the descriptor-backed successor to the deleted saved_agent_model_discovery_config). The command now destructures the struct instead of composing the resolvers inline, and both discovery tests assert on the helper. Review finding (Wren + Dawn, independently): the rewritten model_discovery_ignores_stale_record_for_linked_agent asserted on the two general-purpose resolvers directly, so mutating get_agent_models back to stale record.model/record.provider bytes survived the full suite — main's original test killed that exact mutation. With the helper as the tested seam, the mutation now fails the test. Test/refactor only; no production behavior change. Co-authored-by: Tyler Longwell Signed-off-by: Tyler Longwell --- desktop/scripts/check-file-sizes.mjs | 6 +- .../src-tauri/src/commands/agent_models.rs | 128 ++++++++++++------ .../src/commands/agent_models_tests.rs | 56 ++++---- 3 files changed, 113 insertions(+), 77 deletions(-) diff --git a/desktop/scripts/check-file-sizes.mjs b/desktop/scripts/check-file-sizes.mjs index 84e967b4d..61dc61da0 100644 --- a/desktop/scripts/check-file-sizes.mjs +++ b/desktop/scripts/check-file-sizes.mjs @@ -536,7 +536,11 @@ const overrides = new Map([ // descriptor path also deletes saved_agent_model_discovery_config, whose // callers now use resolve_effective_model_provider + the descriptor env // directly (net wash after the merge of the deltas above). - ["src-tauri/src/commands/agent_models.rs", 1114], + // +38 (1114 -> 1152): agent_model_discovery_config extracted as a pure, + // test-bindable seam (struct + helper + docs) so the linked-agent + // regression test kills the stale-record mutation at get_agent_models' + // consumption point (review finding, Wren + Dawn). + ["src-tauri/src/commands/agent_models.rs", 1152], // global-agent-config: get_agent_config_surface / write_agent_config_field / // put_agent_session_config commands + GlobalAgentConfig serde types. New file // in this PR; queued to split with the command module refactor. diff --git a/desktop/src-tauri/src/commands/agent_models.rs b/desktop/src-tauri/src/commands/agent_models.rs index 5b471f5f1..ca1fe9bdf 100644 --- a/desktop/src-tauri/src/commands/agent_models.rs +++ b/desktop/src-tauri/src/commands/agent_models.rs @@ -38,15 +38,7 @@ pub async fn get_agent_models( app: AppHandle, state: State<'_, AppState>, ) -> Result { - let ( - resolved_acp, - agent_command, - agent_args, - persisted_model, - saved_provider, - provider_env_var, - merged_env, - ) = { + let (resolved_acp, agent_command, discovery) = { let _store_guard = state .managed_agents_store_lock .lock() @@ -79,47 +71,29 @@ pub async fn get_agent_models( let personas = load_personas(&app).unwrap_or_default(); let global = load_global_agent_config(&app).unwrap_or_default(); - // Single typed descriptor — same resolver as spawn_agent_child. - // Returns Err on dangling harness id, propagating it to the caller. - let descriptor = - crate::managed_agents::resolve_effective_harness_descriptor(record, &personas, &global) - .map_err(|e| model_discovery_error(&pubkey, &e))?; + // Single pure helper — descriptor + authoritative model/provider + // resolver, packaged so the linked-agent regression test binds the + // exact values this command consumes. Returns Err on dangling harness + // id, propagating it to the caller. + let discovery = agent_model_discovery_config(record, &personas, &global) + .map_err(|e| model_discovery_error(&pubkey, &e))?; - let resolved_agent = resolve_command(&descriptor.command) + let resolved_agent = resolve_command(&discovery.command) .map(|p| p.display().to_string()) - .unwrap_or_else(|| descriptor.command.clone()); + .unwrap_or_else(|| discovery.command.clone()); - // ModelPicker can persist a selected model but not rewrite the saved - // provider/env snapshot, and runtime spawn reads that same snapshot. - // Model and provider come from the same authoritative resolver spawn - // uses (`resolve_effective_model_provider`) — linked instances read - // their definition, never a stale materialized `record.model` / - // `record.provider`, so discovery cannot query a provider this agent - // will not actually launch with. Definition-less instances keep their - // own record values, matching spawn's `resolve_definition_less` arm. - // When the resolver yields no provider, `effective_discovery_provider` - // below recovers the provider the agent will actually launch with from - // the runtime's own provider env var, read out of the descriptor env - // (which already layers definition/persona/global values the same way - // spawn does). - let (discovery_model, discovery_provider) = - crate::managed_agents::resolve_effective_model_provider(record, &personas, &global); - let discovery_provider_env_var = - known_acp_runtime(&descriptor.command).and_then(|meta| meta.provider_env_var); - let discovery_env = descriptor.env; - let args = descriptor.args; - - ( - resolved, - resolved_agent, - args, - discovery_model, - discovery_provider, - discovery_provider_env_var, - discovery_env, - ) + (resolved, resolved_agent, discovery) }; // store lock released — subprocess runs without holding the lock + let AgentModelDiscoveryConfig { + args: agent_args, + model: persisted_model, + provider: saved_provider, + provider_env_var, + env: merged_env, + command: _, + } = discovery; + let merged_env = discovery_env_with_baked_floor(merged_env); // Resolve against the baked/process env when the record saved no provider, // so a build-provided provider still gets live discovery. @@ -180,6 +154,70 @@ fn model_discovery_error(pubkey: &str, error: &str) -> String { ) } +/// Everything `get_agent_models` needs from the record + context, resolved in +/// one pure step so the linked-agent regression test can bind the exact values +/// the command consumes. +#[derive(Debug, PartialEq, Eq)] +struct AgentModelDiscoveryConfig { + /// Effective harness command (descriptor-resolved), for `resolve_command`. + command: String, + /// Effective harness args (descriptor-resolved). + args: Vec, + /// Model from the authoritative resolver spawn uses — linked instances + /// read their definition, never stale `record.model` bytes. + model: Option, + /// Provider from the same authoritative resolver — never stale + /// `record.provider` bytes for linked instances. + provider: Option, + /// The runtime's provider env var (e.g. `GOOSE_PROVIDER`), so discovery + /// can recover the provider from the env when the resolver yields none. + /// `None` for runtimes that do not take a provider, or an unknown command. + provider_env_var: Option<&'static str>, + /// The descriptor's fully layered env (definition/persona/global/agent). + env: BTreeMap, +} + +/// Resolve the model-discovery config for a saved agent — the descriptor-backed +/// successor to the old `saved_agent_model_discovery_config`. +/// +/// Command/args/env come from `resolve_effective_harness_descriptor` (the same +/// resolver as `spawn_agent_child`); model/provider come from +/// `resolve_effective_model_provider` (#1968's definition-authoritative +/// contract) — linked instances read their definition, never a stale +/// materialized `record.model`/`record.provider`, so discovery cannot query a +/// provider this agent will not actually launch with. Definition-less +/// instances keep their own record values, matching spawn's +/// `resolve_definition_less` arm. When the resolver yields no provider, +/// `effective_discovery_provider` recovers the provider the agent will +/// actually launch with from the runtime's own provider env var, read out of +/// the descriptor env (which already layers definition/persona/global values +/// the same way spawn does). +/// +/// Returns `Err("DANGLING_HARNESS_ID:")` from the descriptor resolver when +/// the harness id no longer exists; the caller routes it through +/// `model_discovery_error`. +fn agent_model_discovery_config( + record: &crate::managed_agents::ManagedAgentRecord, + personas: &[crate::managed_agents::AgentDefinition], + global: &crate::managed_agents::GlobalAgentConfig, +) -> Result { + let descriptor = + crate::managed_agents::resolve_effective_harness_descriptor(record, personas, global)?; + let (model, provider) = + crate::managed_agents::resolve_effective_model_provider(record, personas, global); + let provider_env_var = + known_acp_runtime(&descriptor.command).and_then(|meta| meta.provider_env_var); + + Ok(AgentModelDiscoveryConfig { + command: descriptor.command, + args: descriptor.args, + model, + provider, + provider_env_var, + env: descriptor.env, + }) +} + #[derive(Debug, Deserialize)] #[serde(rename_all = "camelCase")] pub struct DiscoverAgentModelsInput { diff --git a/desktop/src-tauri/src/commands/agent_models_tests.rs b/desktop/src-tauri/src/commands/agent_models_tests.rs index 960ca2587..d98460109 100644 --- a/desktop/src-tauri/src/commands/agent_models_tests.rs +++ b/desktop/src-tauri/src/commands/agent_models_tests.rs @@ -194,37 +194,33 @@ fn saved_agent_model_discovery_uses_record_snapshot_for_definition_less_agent() ) .expect("sample managed agent record"); - // resolve_effective_harness_descriptor is the single resolver used by - // get_agent_models — verify it layers env correctly and strips reserved keys. - let descriptor = crate::managed_agents::resolve_effective_harness_descriptor( - &record, - &[], - &Default::default(), - ) - .expect("descriptor should resolve for a valid record"); + // agent_model_discovery_config is the single helper get_agent_models + // consumes — verify it layers env correctly, strips reserved keys, and + // keeps the record's own model/provider for a definition-less instance + // (matching spawn's `resolve_definition_less` arm). + let discovery = agent_model_discovery_config(&record, &[], &Default::default()) + .expect("discovery config should resolve for a valid record"); - assert_eq!(descriptor.command.as_str(), "goose"); + assert_eq!(discovery.command.as_str(), "goose"); + assert_eq!(discovery.model.as_deref(), Some("record-model")); + assert_eq!(discovery.provider.as_deref(), Some("databricks")); assert_eq!( - descriptor.env.get("GOOSE_MODEL").map(String::as_str), + discovery.env.get("GOOSE_MODEL").map(String::as_str), Some("record-model") ); assert_eq!( - descriptor.env.get("GOOSE_PROVIDER").map(String::as_str), + discovery.env.get("GOOSE_PROVIDER").map(String::as_str), Some("databricks") ); assert_eq!( - descriptor.env.get("OPENAI_API_KEY").map(String::as_str), + discovery.env.get("OPENAI_API_KEY").map(String::as_str), Some("record-key") ); // Reserved keys are stripped from the descriptor env. - assert!(!descriptor.env.contains_key("BUZZ_PRIVATE_KEY")); - // get_agent_models recovers the provider env var from the runtime metadata - // for the descriptor's command (the inline equivalent of the old - // SavedAgentModelDiscoveryConfig.provider_env_var). - assert_eq!( - known_acp_runtime(&descriptor.command).and_then(|meta| meta.provider_env_var), - Some("GOOSE_PROVIDER") - ); + assert!(!discovery.env.contains_key("BUZZ_PRIVATE_KEY")); + // The provider env var is recovered from the runtime metadata for the + // effective command (the old SavedAgentModelDiscoveryConfig.provider_env_var). + assert_eq!(discovery.provider_env_var, Some("GOOSE_PROVIDER")); } // --------------------------------------------------------------------------- @@ -408,27 +404,25 @@ fn model_discovery_ignores_stale_record_for_linked_agent() { updated_at: "".to_string(), }; - // get_agent_models resolves model/provider through the authoritative - // resolver (the same one spawn uses) — the stale record bytes must lose. + // agent_model_discovery_config is the single helper get_agent_models + // consumes — the stale record bytes must lose to the persona's current + // model/provider (the same authoritative resolver spawn uses). let personas = [persona]; let global = crate::managed_agents::GlobalAgentConfig::default(); - let (model, provider) = - crate::managed_agents::resolve_effective_model_provider(&record, &personas, &global); - assert_eq!(model.as_deref(), Some("persona-model")); - assert_eq!(provider.as_deref(), Some("anthropic")); + let discovery = agent_model_discovery_config(&record, &personas, &global) + .expect("discovery config should resolve for a linked record"); + assert_eq!(discovery.model.as_deref(), Some("persona-model")); + assert_eq!(discovery.provider.as_deref(), Some("anthropic")); // And the discovery env comes from the descriptor, whose layering also // resolves through the definition — the derived model env var must carry // the persona's model, not the stale record snapshot. - let descriptor = - crate::managed_agents::resolve_effective_harness_descriptor(&record, &personas, &global) - .expect("descriptor should resolve for a linked record"); assert_eq!( - descriptor.env.get("GOOSE_MODEL").map(String::as_str), + discovery.env.get("GOOSE_MODEL").map(String::as_str), Some("persona-model") ); assert_eq!( - descriptor.env.get("GOOSE_PROVIDER").map(String::as_str), + discovery.env.get("GOOSE_PROVIDER").map(String::as_str), Some("anthropic") ); }