test(agent-models): bind linked-agent regression to get_agent_models' seam

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 <tlongwell@block.xyz> Signed-off-by: Tyler Longwell <tlongwell@block.xyz>
2026-08-18 06:50:31 +02:00 · 2026-07-26 13:38:53 -04:00
parent 872148fd94
commit 83545f1b86
3 changed files with 113 additions and 77 deletions
@@ -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.
@@ -38,15 +38,7 @@ pub async fn get_agent_models(
    app: AppHandle,
    state: State<'_, AppState>,
 ) -> Result<AgentModelsResponse, String> {
-    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<String>,
+    /// Model from the authoritative resolver spawn uses — linked instances
+    /// read their definition, never stale `record.model` bytes.
+    model: Option<String>,
+    /// Provider from the same authoritative resolver — never stale
+    /// `record.provider` bytes for linked instances.
+    provider: Option<String>,
+    /// 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<String, String>,
+}
+
+/// 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:<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<AgentModelDiscoveryConfig, String> {
+    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 {
@@ -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")
    );
 }