diff --git a/src/openhuman/agent/harness/definition.rs b/src/openhuman/agent/harness/definition.rs index 6c23991318..5a0cd42351 100644 --- a/src/openhuman/agent/harness/definition.rs +++ b/src/openhuman/agent/harness/definition.rs @@ -662,6 +662,14 @@ pub enum DefinitionSource { Builtin, /// Loaded from a TOML file at the given absolute path. File(PathBuf), + /// Synthesized at lookup time from a user-authored + /// [`AgentRegistryEntry`](crate::openhuman::agent_registry::AgentRegistryEntry) + /// (`AgentRegistrySource::Custom`) by `agent_registry::defaults::definition_from_registry_entry`. + /// Never persisted in the [`AgentDefinitionRegistry`] — built fresh per + /// factory call so config edits take effect immediately (closes the gap + /// where custom agents ran persona-only instead of with their real tool + /// belt). + CustomRegistry, } // ───────────────────────────────────────────────────────────────────────────── diff --git a/src/openhuman/agent/harness/session/builder/builder_tests.rs b/src/openhuman/agent/harness/session/builder/builder_tests.rs index 7e44eebcda..07291137af 100644 --- a/src/openhuman/agent/harness/session/builder/builder_tests.rs +++ b/src/openhuman/agent/harness/session/builder/builder_tests.rs @@ -241,6 +241,108 @@ async fn build_session_agent_falls_back_to_global_default_when_no_definition() { ); } +// ───────────────────────────────────────────────────────────────────────────── +// B38 (Gap 2) — a custom (non-shipped) `AgentRegistryEntry` must synthesize a +// real `AgentDefinition` and run with its own `ToolScope::Named` filter, +// instead of the factory hard-erroring "agent definition '…' not found in +// registry" (chat / task-dispatcher) because it never consulted +// `config.agent_registry.entries`. +// +// Regression note: this test deliberately does NOT call +// `AgentDefinitionRegistry::init_global*` itself, so — depending on whether +// an earlier test in this binary already initialised the process-wide +// `OnceLock` singleton — it exercises `build_session_agent_inner`'s tool- +// visibility computation under EITHER state: `(Some(def), Some(registry))` +// or `(Some(def), None)`. Both arms must apply `def.tools` (the synthesized +// `ToolScope::Named` from `definition_from_registry_entry`); the `None` +// (registry-uninitialized) arm previously fell through to the catch-all +// "no registry, no filter" case and silently discarded the custom agent's +// allowlist, leaving `visible_tool_names_for_test()` empty. See the +// `(Some(def), None)` match arm in `factory.rs`'s delegation-tool-and- +// visibility block for the fix. +// ───────────────────────────────────────────────────────────────────────────── + +#[tokio::test] +async fn from_config_for_agent_synthesizes_custom_registry_entry_with_named_scope() { + use crate::openhuman::agent::harness::session::types::Agent; + use crate::openhuman::agent_registry::types::{ + AgentRegistryEntry, AgentRegistrySource, AgentSubagentPolicy, + }; + use crate::openhuman::tokenjuice::RETRIEVE_TOOL_NAME; + + let tmp = tempfile::TempDir::new().unwrap(); + let mut config = test_config(&tmp); + config.agent_registry.entries = vec![AgentRegistryEntry { + id: "finance_analyst_b38".to_string(), + name: "Finance Analyst".to_string(), + description: "Reviews spend and drafts finance summaries.".to_string(), + source: AgentRegistrySource::Custom, + enabled: true, + model: Some("hint:reasoning".to_string()), + system_prompt: Some("You are a meticulous finance analyst.".to_string()), + tool_allowlist: vec!["memory_search".to_string(), "web_search".to_string()], + tool_denylist: Vec::new(), + subagents: AgentSubagentPolicy::default(), + tags: Vec::new(), + metadata: serde_json::Value::Null, + }]; + + // Precondition: this id must NOT be a harness definition (built-in or + // workspace TOML) — the whole point is that only the config-backed + // custom registry knows about it. + assert!( + crate::openhuman::agent::harness::definition::AgentDefinitionRegistry::global() + .map(|reg| reg.get("finance_analyst_b38").is_none()) + .unwrap_or(true), + "test id must not collide with a real harness definition" + ); + + let agent = Agent::from_config_for_agent(&config, "finance_analyst_b38").expect( + "a custom agent_registry entry must synthesize a real AgentDefinition and build \ + successfully instead of erroring", + ); + + let visible = agent.visible_tool_names_for_test(); + assert!( + visible.contains("memory_search") && visible.contains("web_search"), + "the custom agent's tool_allowlist must become a real ToolScope::Named filter: {visible:?}" + ); + assert!( + visible.contains(RETRIEVE_TOOL_NAME), + "the compaction recovery tool must join any non-empty Named allowlist: {visible:?}" + ); + assert!( + !visible.contains("automate"), + "a tool outside the custom agent's allowlist must not be visible: {visible:?}" + ); +} + +#[tokio::test] +async fn from_config_for_agent_still_errors_for_a_genuinely_unknown_id() { + use crate::openhuman::agent::harness::session::types::Agent; + + let tmp = tempfile::TempDir::new().unwrap(); + let config = test_config(&tmp); + + // No harness definition AND no config.agent_registry entry for this id — + // the factory must still hard-error rather than silently building an + // unfiltered/legacy agent. + // + // Note: `Agent` intentionally has no `Debug` impl (it holds `Box` / provider trait objects), so this must use `match` + + // `.is_err()` rather than `.expect_err()`, which requires `T: Debug`. + let result = Agent::from_config_for_agent(&config, "totally_unknown_agent_id_b38"); + assert!( + result.is_err(), + "an id with no harness definition and no custom entry must error" + ); + let err = result.err().unwrap(); + assert!( + err.to_string().contains("totally_unknown_agent_id_b38"), + "error should name the unresolved agent id: {err}" + ); +} + // ── #5050 Fix 1: shared `Arc` for the per-build tool config ────────── #[test] diff --git a/src/openhuman/agent/harness/session/builder/factory.rs b/src/openhuman/agent/harness/session/builder/factory.rs index a711f138b9..89600c7a79 100644 --- a/src/openhuman/agent/harness/session/builder/factory.rs +++ b/src/openhuman/agent/harness/session/builder/factory.rs @@ -74,47 +74,11 @@ impl Agent { pub fn from_config_for_agent(config: &Config, agent_id: &str) -> Result { // Look up the target definition up front so we can fail fast // with a clear error instead of building half an agent and then - // discovering the id is unknown. The registry is a singleton - // initialised at startup; if it's not yet populated we - // conservatively fall back to the legacy "orchestrator-shaped" - // build by proceeding without a definition override. - let target_def: Option = - match AgentDefinitionRegistry::global() { - Some(reg) => match reg.get(agent_id) { - Some(def) => Some(def.clone()), - None if agent_id == "orchestrator" => { - // Orchestrator is allowed to be missing from the - // registry (legacy path, tests, pre-startup) — - // fall back to default behaviour. - log::debug!( - "[agent::builder] orchestrator definition not in registry — \ - using legacy default prompt + filter" - ); - None - } - None => { - return Err(anyhow::anyhow!( - "agent definition '{}' not found in registry", - agent_id - )); - } - }, - None => { - if agent_id != "orchestrator" { - return Err(anyhow::anyhow!( - "AgentDefinitionRegistry is not initialised — cannot \ - resolve agent '{}'. Call AgentDefinitionRegistry::init_global \ - at startup.", - agent_id - )); - } - log::debug!( - "[agent::builder] registry not initialised, orchestrator requested — \ - using legacy default prompt + filter" - ); - None - } - }; + // discovering the id is unknown. See `resolve_target_definition` + // for the full resolution order (harness registry, then the + // config-backed custom agent registry, then the orchestrator's + // legacy pre-startup fallback). + let target_def = resolve_target_definition(config, agent_id)?; log::info!( "[agent::builder] building session agent id={} \ @@ -197,30 +161,7 @@ impl Agent { profile_prompt_suffix: Option, profile: Option<&crate::openhuman::profiles::AgentProfile>, ) -> Result { - let target_def: Option = - match AgentDefinitionRegistry::global() { - Some(reg) => match reg.get(agent_id) { - Some(def) => Some(def.clone()), - None if agent_id == "orchestrator" => None, - None => { - return Err(anyhow::anyhow!( - "agent definition '{}' not found in registry", - agent_id - )); - } - }, - None => { - if agent_id != "orchestrator" { - return Err(anyhow::anyhow!( - "AgentDefinitionRegistry is not initialised — cannot \ - resolve agent '{}'. Call AgentDefinitionRegistry::init_global \ - at startup.", - agent_id - )); - } - None - } - }; + let target_def = resolve_target_definition(config, agent_id)?; Self::build_session_agent_inner( config, agent_id, @@ -885,7 +826,35 @@ impl Agent { }; (synthed, None) } - (_, None) => { + (Some(def), None) => { + // We have a target definition (either a pre-populated + // harness entry looked up before the registry singleton + // existed, or — the common case today — a `CustomRegistry` + // definition `resolve_target_definition` synthesizes + // straight from `config.agent_registry.entries` without + // ever consulting `AgentDefinitionRegistry::global()`, see + // `agent_registry::find_custom_in_config`). Delegation-tool + // synthesis needs the registry (to resolve named + // subagents), so it's skipped here, but `def.tools` is a + // real scope the caller authored and MUST still gate + // visibility — silently dropping it into the `(_, None)` + // "no registry, no filter" catch-all would leave a custom + // agent's `ToolScope::Named` allowlist entirely + // unenforced (visible tools empty rather than the named + // set), regressing the least-privilege contract this + // synthesis path exists to provide. + log::debug!( + "[agent::builder] AgentDefinitionRegistry not initialised — skipping \ + delegation tool synthesis, but still applying target definition's own \ + tool scope" + ); + let filter: Option> = match &def.tools { + ToolScope::Named(names) => Some(names.iter().cloned().collect()), + ToolScope::Wildcard => None, + }; + (Vec::new(), filter) + } + (None, None) => { log::debug!( "[agent::builder] AgentDefinitionRegistry not initialised — \ skipping delegation tool synthesis" @@ -1214,6 +1183,81 @@ impl Agent { } } +/// Resolves the `AgentDefinition` a session should be built from, given the +/// requested `agent_id`, in three steps: +/// +/// 1. **Harness registry** (`AgentDefinitionRegistry`, the process-global +/// singleton of built-in + workspace-TOML-override definitions) — a hit +/// here wins outright. +/// 2. **Config-backed custom agent registry** (`config.agent_registry.entries`, +/// `AgentRegistrySource::Custom`) — on a harness-registry miss (or the +/// registry not yet being initialised), a user-authored custom agent is +/// synthesized into a real `AgentDefinition` via +/// `agent_registry::definition_from_registry_entry` so it runs through +/// the exact same `build_session_agent_inner` path (and therefore the +/// exact same `SecurityPolicy` / tool-filtering / approval gate) as a +/// built-in. This closes the gap where a custom agent either hard-errored +/// (chat, task-dispatcher) or silently ran tool-less/persona-only (flows' +/// `RegistryFallback`) — see the cross-cutting fix in the PR that added +/// this function. +/// 3. **Orchestrator legacy fallback** — `orchestrator` alone is allowed to +/// resolve to `None` (pre-startup, tests): the caller then builds with the +/// default prompt/filter, matching pre-#1 behaviour. +/// +/// Any other id that resolves nowhere is a hard error, exactly as before this +/// function existed — only the *search order* changed, not the failure +/// contract for a genuinely-unknown id. +fn resolve_target_definition( + config: &Config, + agent_id: &str, +) -> Result> { + let registry = AgentDefinitionRegistry::global(); + + if let Some(reg) = registry { + if let Some(def) = reg.get(agent_id) { + return Ok(Some(def.clone())); + } + } + + // Harness registry miss (or not yet initialised). Before failing, check + // the config-backed custom agent registry — the one place custom + // (non-shipped) agents live. + if let Some(entry) = crate::openhuman::agent_registry::find_custom_in_config(config, agent_id) { + log::info!( + "[agent::builder] agent_id={} not found in the harness AgentDefinitionRegistry — \ + synthesizing a definition from its custom agent_registry entry so it runs with its \ + real tool belt instead of persona-only / erroring", + agent_id + ); + return Ok(Some( + crate::openhuman::agent_registry::definition_from_registry_entry(&entry), + )); + } + + if agent_id == "orchestrator" { + // Orchestrator is allowed to be missing from every source (legacy + // path, tests, pre-startup) — fall back to default behaviour. + log::debug!( + "[agent::builder] orchestrator definition not in any registry — using legacy \ + default prompt + filter" + ); + return Ok(None); + } + + if registry.is_none() { + return Err(anyhow::anyhow!( + "AgentDefinitionRegistry is not initialised — cannot resolve agent '{}'. Call \ + AgentDefinitionRegistry::init_global at startup.", + agent_id + )); + } + + Err(anyhow::anyhow!( + "agent definition '{}' not found in registry", + agent_id + )) +} + /// Resolve the `Config` the tool registry is built from (#5050, Fix 1). /// /// Normally this is the shared `base_config` — returned as a refcount bump, not a diff --git a/src/openhuman/agent/library/ops.rs b/src/openhuman/agent/library/ops.rs index 9033b01486..b993f35278 100644 --- a/src/openhuman/agent/library/ops.rs +++ b/src/openhuman/agent/library/ops.rs @@ -59,7 +59,9 @@ pub fn metadata_from_definition(def: &AgentDefinition) -> AgentDefinitionDisplay write_capable: is_write_capable(def), source: match &def.source { DefinitionSource::Builtin => AgentDefinitionSource::Builtin, - DefinitionSource::File(_) => AgentDefinitionSource::Custom, + DefinitionSource::File(_) | DefinitionSource::CustomRegistry => { + AgentDefinitionSource::Custom + } }, } } diff --git a/src/openhuman/agent_registry/defaults.rs b/src/openhuman/agent_registry/defaults.rs index 6fe73b58e8..c4e505a355 100644 --- a/src/openhuman/agent_registry/defaults.rs +++ b/src/openhuman/agent_registry/defaults.rs @@ -3,7 +3,8 @@ use serde_json::Value; use crate::openhuman::agent::harness::definition::{ - AgentDefinition, ModelSpec, SubagentEntry, ToolScope, + AgentDefinition, AgentTier, DefinitionSource, IterationPolicy, ModelSpec, PromptSource, + SandboxMode, SubagentEntry, ToolScope, TriggerMemoryAgent, }; use super::types::{AgentRegistryEntry, AgentRegistrySource, AgentSubagentPolicy}; @@ -49,6 +50,104 @@ fn default_entry_from_definition(def: AgentDefinition) -> AgentRegistryEntry { } } +/// Inverse of [`default_entry_from_definition`] — synthesizes a harness +/// [`AgentDefinition`] from a user-authored [`AgentRegistryEntry`] so a +/// custom agent (one with no shipped harness definition) can be built through +/// the same [`crate::openhuman::agent::Agent::from_config_for_agent`] factory +/// path as a built-in, and therefore run with its real tool belt rather than +/// degrade to a persona-only completion. +/// +/// Mapping (mirrors `default_entry_from_definition` field-for-field, in +/// reverse): +/// * `description` -> `when_to_use`; `name` -> `display_name`. +/// * `system_prompt` -> `PromptSource::Inline` (empty string when unset, +/// which renders as an empty subagent body rather than erroring). +/// * `model` -> `ModelSpec` via [`registry_value_to_model_spec`], the +/// inverse of [`model_to_registry_value`]. +/// * `tool_allowlist` -> `ToolScope` via [`allowlist_to_tool_scope`]: exactly +/// `["*"]` means `Wildcard`; an empty list means `Named(vec![])` (no +/// tools) — matching how `tools_to_allowlist` renders `Wildcard` as +/// `["*"]` and `Named(vec![])` as `[]`. +/// * `tool_denylist` -> `disallowed_tools` (direct clone). +/// * `subagents.allowlist` -> one `SubagentEntry::AgentId` per entry. +/// +/// Every other field is a harness-side concern a custom agent never +/// authors today, so it takes the harness's own safe default: `omit_* = +/// true` (narrow/lean prompt, matching every non-orchestrator built-in), +/// `temperature = 0.4`, `max_iterations = 8` under `IterationPolicy::Strict`, +/// `sandbox_mode = None`, `agent_tier = Worker`. `source` is stamped +/// [`DefinitionSource::CustomRegistry`] so it's visibly distinct from a +/// shipped/TOML-file definition in logs and `agent::list_definitions`. +pub fn definition_from_registry_entry(entry: &AgentRegistryEntry) -> AgentDefinition { + AgentDefinition { + id: entry.id.clone(), + when_to_use: entry.description.clone(), + display_name: Some(entry.name.clone()), + system_prompt: PromptSource::Inline(entry.system_prompt.clone().unwrap_or_default()), + omit_identity: true, + omit_memory_context: true, + omit_safety_preamble: true, + omit_skills_catalog: true, + omit_profile: true, + omit_memory_md: true, + model: registry_value_to_model_spec(entry.model.as_deref()), + temperature: 0.4, + tools: allowlist_to_tool_scope(&entry.tool_allowlist), + disallowed_tools: entry.tool_denylist.clone(), + skill_filter: None, + extra_tools: Vec::new(), + max_iterations: 8, + iteration_policy: IterationPolicy::Strict, + max_result_chars: None, + max_turn_output_tokens: None, + timeout_secs: None, + sandbox_mode: SandboxMode::None, + background: false, + trigger_memory_agent: TriggerMemoryAgent::Never, + tokenjuice_compression: Default::default(), + subagents: entry + .subagents + .allowlist + .iter() + .cloned() + .map(SubagentEntry::AgentId) + .collect(), + delegate_name: None, + agent_tier: AgentTier::Worker, + source: DefinitionSource::CustomRegistry, + graph: Default::default(), + } +} + +/// Inverse of [`model_to_registry_value`]: `None`/`"inherit"` -> `Inherit`; +/// `"hint:"` -> `Hint(role)`; anything else -> `Exact(value)`. +fn registry_value_to_model_spec(value: Option<&str>) -> ModelSpec { + match value.map(str::trim).filter(|v| !v.is_empty()) { + None => ModelSpec::Inherit, + Some("inherit") => ModelSpec::Inherit, + Some(v) => match v.strip_prefix("hint:") { + Some(hint) => ModelSpec::Hint(hint.to_string()), + None => ModelSpec::Exact(v.to_string()), + }, + } +} + +/// Inverse of [`tools_to_allowlist`]'s `Wildcard` rendering: exactly `["*"]` +/// means "all tools" (`ToolScope::Wildcard`). An **empty** allowlist is a +/// deliberate `ToolScope::Named(vec![])` — i.e. tool-less — matching what the +/// settings UI/schema mean by "no tools selected", and matching the forward +/// direction: `tools_to_allowlist(&ToolScope::Named(vec![]), &[])` already +/// renders back to `[]`, never `["*"]`. Collapsing empty to `Wildcard` here +/// would silently grant a custom agent saved with no tools selected every +/// enabled tool, bypassing the least-privilege setting the editor shows. +fn allowlist_to_tool_scope(allowlist: &[String]) -> ToolScope { + if allowlist == ["*"] { + ToolScope::Wildcard + } else { + ToolScope::Named(allowlist.to_vec()) + } +} + fn model_to_registry_value(model: &ModelSpec) -> Option { match model { ModelSpec::Inherit => Some("inherit".to_string()), @@ -85,4 +184,121 @@ mod tests { .iter() .all(|agent| agent.source == AgentRegistrySource::Default)); } + + fn custom_entry(id: &str) -> AgentRegistryEntry { + AgentRegistryEntry { + id: id.to_string(), + name: "Finance Analyst".to_string(), + description: "Reviews spend and drafts finance summaries.".to_string(), + source: AgentRegistrySource::Custom, + enabled: true, + model: Some("hint:reasoning".to_string()), + system_prompt: Some("You are a meticulous finance analyst.".to_string()), + tool_allowlist: vec!["memory_search".to_string(), "web_search".to_string()], + tool_denylist: vec!["file_write".to_string()], + subagents: AgentSubagentPolicy::from_allowlist(vec!["researcher".to_string()]), + tags: vec!["finance".to_string()], + metadata: Value::Null, + } + } + + #[test] + fn definition_from_registry_entry_preserves_tools_model_denylist_subagents() { + let entry = custom_entry("finance_analyst"); + let def = definition_from_registry_entry(&entry); + + assert_eq!(def.id, "finance_analyst"); + assert_eq!(def.when_to_use, entry.description); + assert_eq!(def.display_name(), "Finance Analyst"); + assert!(matches!( + def.model, + ModelSpec::Hint(ref hint) if hint == "reasoning" + )); + assert!(matches!( + def.tools, + ToolScope::Named(ref names) + if names == &vec!["memory_search".to_string(), "web_search".to_string()] + )); + assert_eq!(def.disallowed_tools, vec!["file_write".to_string()]); + assert_eq!( + def.subagents, + vec![SubagentEntry::AgentId("researcher".to_string())] + ); + assert_eq!(def.source, DefinitionSource::CustomRegistry); + } + + #[test] + fn definition_from_registry_entry_wildcard_allowlist_round_trips() { + let mut entry = custom_entry("wildcard_agent"); + entry.tool_allowlist = vec!["*".to_string()]; + let def = definition_from_registry_entry(&entry); + assert!(matches!(def.tools, ToolScope::Wildcard)); + + // Round trip back through the forward direction should reproduce the + // same wildcard shape `default_entry_from_definition` would emit. + assert_eq!(tools_to_allowlist(&def.tools, &[]), vec!["*".to_string()]); + } + + #[test] + fn definition_from_registry_entry_empty_allowlist_stays_tool_less() { + // Regression test (P1 review comment on this PR): an empty + // `tool_allowlist` means "no tools selected" in the settings UI/schema + // — it must synthesize a `ToolScope::Named(vec![])`, NEVER + // `ToolScope::Wildcard`. Collapsing empty to Wildcard would silently + // grant every enabled tool to a custom agent saved with no tools + // selected, bypassing the least-privilege setting the editor shows. + let mut entry = custom_entry("tool_less_agent"); + entry.tool_allowlist = Vec::new(); + let def = definition_from_registry_entry(&entry); + + assert!( + matches!(def.tools, ToolScope::Named(ref names) if names.is_empty()), + "an empty tool_allowlist must synthesize a tool-less Named([]) scope, not Wildcard: {:?}", + def.tools + ); + + // Round trip back through the forward direction must reproduce the + // same empty shape, not `["*"]`. + assert_eq!(tools_to_allowlist(&def.tools, &[]), Vec::::new()); + } + + #[test] + fn entry_to_definition_to_entry_round_trip_preserves_key_fields() { + let entry = custom_entry("finance_analyst"); + let def = definition_from_registry_entry(&entry); + + // Rebuild an entry from the synthesized definition the same way + // `default_entry_from_definition` does, and confirm the + // execution-critical fields survive the round trip. + let roundtripped = AgentRegistryEntry { + id: def.id.clone(), + name: def.display_name().to_string(), + description: def.when_to_use.clone(), + source: AgentRegistrySource::Custom, + enabled: true, + model: model_to_registry_value(&def.model), + system_prompt: None, + tool_allowlist: tools_to_allowlist(&def.tools, &def.extra_tools), + tool_denylist: def.disallowed_tools.clone(), + subagents: AgentSubagentPolicy::from_allowlist( + def.subagents + .iter() + .filter_map(|s| match s { + SubagentEntry::AgentId(id) => Some(id.clone()), + SubagentEntry::Skills(_) => None, + }) + .collect(), + ), + tags: Vec::new(), + metadata: Value::Null, + }; + + assert_eq!(roundtripped.id, entry.id); + assert_eq!(roundtripped.name, entry.name); + assert_eq!(roundtripped.description, entry.description); + assert_eq!(roundtripped.model, entry.model); + assert_eq!(roundtripped.tool_allowlist, entry.tool_allowlist); + assert_eq!(roundtripped.tool_denylist, entry.tool_denylist); + assert_eq!(roundtripped.subagents, entry.subagents); + } } diff --git a/src/openhuman/agent_registry/mod.rs b/src/openhuman/agent_registry/mod.rs index 370bfb4af3..30ab345732 100644 --- a/src/openhuman/agent_registry/mod.rs +++ b/src/openhuman/agent_registry/mod.rs @@ -13,10 +13,10 @@ mod schemas; pub mod tools; pub mod types; -pub use defaults::default_agents; +pub use defaults::{default_agents, definition_from_registry_entry}; pub use ops::{ - get_agent, list_agents, merge_entries, remove_agent, set_agent_enabled, update_agent, - upsert_custom_agent, + find_custom_in_config, get_agent, list_agents, merge_entries, remove_agent, set_agent_enabled, + update_agent, upsert_custom_agent, }; pub use schemas::{ all_controller_schemas as all_agent_registry_controller_schemas, diff --git a/src/openhuman/agent_registry/ops.rs b/src/openhuman/agent_registry/ops.rs index 3b7e7c3c86..f1c5485599 100644 --- a/src/openhuman/agent_registry/ops.rs +++ b/src/openhuman/agent_registry/ops.rs @@ -5,6 +5,7 @@ use std::collections::HashMap; use crate::openhuman::agent::harness::AgentDefinitionRegistry; use crate::openhuman::agent::Agent; use crate::openhuman::config::rpc as config_rpc; +use crate::openhuman::config::Config; use super::defaults::default_agents; use super::types::{AgentRegistryEntry, AgentRegistryPatch, AgentRegistrySource, AgentToolInfo}; @@ -197,6 +198,43 @@ pub fn merge_entries( result } +/// Synchronous, config-only lookup for a user-authored (`Custom`-source), +/// **enabled** agent registry entry by id. +/// +/// Used by the agent factory (`Agent::from_config_for_agent` family, see +/// `agent::harness::session::builder::factory`) on a harness-registry lookup +/// miss so a custom agent can be synthesized into a real +/// `AgentDefinition` (via `definition_from_registry_entry`) and run with its +/// real tool belt, instead of erroring (chat/task-dispatcher) or degrading to +/// a persona-only completion (flows). Deliberately sync — unlike +/// [`get_agent`]/[`list_agents`] — because the factory already holds a +/// `&Config` in scope and must not spawn an async config reload mid-build. +/// +/// Only `AgentRegistrySource::Custom` entries match: a `Default`-sourced +/// override (a user edit to a shipped agent, e.g. via `update_agent`) is +/// already resolvable through the harness `AgentDefinitionRegistry` by id — +/// that agent ships an `agent.toml`/builtin definition — so it never reaches +/// this fallback path. +/// +/// A **disabled** custom entry is deliberately treated as a miss (`None`), +/// same as an unknown id — never synthesized into a runnable definition here. +/// Every caller of this function (chat, task-dispatcher, flows' registry +/// routing) resolves an agent id directly to "runnable or not"; without this +/// filter a disabled custom agent referenced by an existing profile or a +/// direct caller could still run through the harness path, silently +/// bypassing the disabled flag the flows path already enforces explicitly. +pub fn find_custom_in_config(config: &Config, id: &str) -> Option { + let id = id.trim(); + config + .agent_registry + .entries + .iter() + .find(|entry| { + entry.id == id && entry.enabled && matches!(entry.source, AgentRegistrySource::Custom) + }) + .cloned() +} + fn apply_patch(entry: &mut AgentRegistryEntry, patch: AgentRegistryPatch) { if let Some(name) = patch.name { entry.name = name; @@ -294,6 +332,56 @@ mod tests { assert_eq!(merged.last().unwrap().id, "finance_analyst"); } + #[test] + fn find_custom_in_config_returns_matching_custom_entry() { + let mut config = Config::default(); + config.agent_registry.entries = vec![custom_agent("finance_analyst", true)]; + + let found = find_custom_in_config(&config, "finance_analyst"); + assert!(found.is_some()); + assert_eq!(found.unwrap().id, "finance_analyst"); + } + + #[test] + fn find_custom_in_config_ignores_default_source_entries() { + // A `Default`-sourced override (a user edit to a shipped agent) must + // NOT be picked up here — it already resolves via the harness + // `AgentDefinitionRegistry`, so this fallback should stay a miss. + let mut config = Config::default(); + config.agent_registry.entries = vec![AgentRegistryEntry { + source: AgentRegistrySource::Default, + ..custom_agent("researcher", true) + }]; + + assert!(find_custom_in_config(&config, "researcher").is_none()); + } + + #[test] + fn find_custom_in_config_misses_unknown_id() { + let mut config = Config::default(); + config.agent_registry.entries = vec![custom_agent("finance_analyst", true)]; + + assert!(find_custom_in_config(&config, "totally_unknown").is_none()); + } + + #[test] + fn find_custom_in_config_ignores_disabled_custom_entries() { + // Regression test (P2 review comment on this PR): a disabled custom + // agent must be treated as a miss here, exactly like an unknown id — + // otherwise a direct factory caller (chat, task-dispatcher) that + // references a disabled custom agent's id (e.g. via an existing + // profile) would still synthesize it into a runnable definition, + // bypassing the disabled flag that the flows path already enforces + // explicitly via `route_custom_entry_lookup`. + let mut config = Config::default(); + config.agent_registry.entries = vec![custom_agent("finance_analyst", false)]; + + assert!( + find_custom_in_config(&config, "finance_analyst").is_none(), + "a disabled custom entry must not be returned as a runnable custom agent" + ); + } + #[test] fn ensure_orchestrator_enabled_rejects_disabled_orchestrator() { let mut entry = custom_agent("orchestrator", false); diff --git a/src/openhuman/tinyflows/caps.rs b/src/openhuman/tinyflows/caps.rs index f3b9668d9f..54c2e1ef68 100644 --- a/src/openhuman/tinyflows/caps.rs +++ b/src/openhuman/tinyflows/caps.rs @@ -893,31 +893,108 @@ impl AgentRunner for OpenHumanAgentRunner { agent_ref, "[flows] agent_runner: HARNESS path — running the full agent tool loop" ); - self.run_via_harness(agent_ref, request, conn).await + // A shipped/TOML harness definition has no `entry.model` — the + // definition's own `ModelSpec` (already applied by the session + // builder) is the only model pin in play here. + self.run_via_harness(agent_ref, request, conn, None).await } AgentRoute::RegistryFallback => { - tracing::info!( - target: "flows", + // `route_for_agent_ref` only consults the harness + // `AgentDefinitionRegistry`, so a miss there used to mean + // "run the persona-only completion fallback" unconditionally + // — even for a user-created custom agent, which has real + // `tool_allowlist`/`model` settings that fallback ignores. + // + // The agent factory (`Agent::from_config_for_agent`) now + // also consults `config.agent_registry.entries` on a + // harness-registry miss and synthesizes a real + // `AgentDefinition` for any `AgentRegistrySource::Custom` + // entry it finds (issue B38/Gap 2). So: route a *known, + // enabled* custom entry through the harness turn — it gets + // its real tool belt — and reserve the persona-only + // completion for `agent_ref`s that are unknown to both the + // harness registry AND the custom config registry (or that + // are disabled, which `run_via_registry_fallback` already + // rejects with a clear error). + let custom_entry = crate::openhuman::agent_registry::find_custom_in_config( + &self.config, agent_ref, - "[flows] agent_runner: FALLBACK path — persona-shaping single completion for a \ - custom registry entry" ); - self.run_via_registry_fallback(agent_ref, request, conn) - .await + let entry_model = custom_entry.as_ref().and_then(|e| e.model.clone()); + match route_custom_entry_lookup(custom_entry.as_ref()) { + AgentRoute::Harness => { + tracing::info!( + target: "flows", + agent_ref, + "[flows] agent_runner: CUSTOM-REGISTRY path — routing through the \ + harness so the custom agent runs with its real tool belt instead of \ + the persona-only completion fallback" + ); + // Preserve the custom entry's own `model` pin (e.g. + // `hint:reasoning` or a raw BYOK model id) as the + // fallback below the node's own override — same + // precedence `run_via_registry_fallback` already gave + // it, now honored on the harness path too (P2 review + // comment on this PR: this previously regressed to the + // default chat model for a custom flow agent with no + // per-node override). + self.run_via_harness(agent_ref, request, conn, entry_model.as_deref()) + .await + } + AgentRoute::RegistryFallback => { + tracing::info!( + target: "flows", + agent_ref, + "[flows] agent_runner: FALLBACK path — persona-shaping single \ + completion for a custom registry entry" + ); + self.run_via_registry_fallback(agent_ref, request, conn) + .await + } + } } } } } +/// Decides how to run an `agent_ref` that has no harness definition, given +/// the (already-performed) config-backed custom registry lookup: an +/// [`AgentRoute::Harness`] for a known, *enabled* custom entry — the factory +/// synthesizes a real `AgentDefinition` for it (issue B38/Gap 2), so it can +/// run the full tool loop — and [`AgentRoute::RegistryFallback`] for +/// anything else (no entry at all, or a disabled one, which +/// [`OpenHumanAgentRunner::run_via_registry_fallback`] itself rejects with a +/// clear "is disabled" error rather than silently skipping it here). Pure +/// over the lookup result so the decision is unit-testable without a live +/// `Config`/registry. +pub(crate) fn route_custom_entry_lookup( + entry: Option<&crate::openhuman::agent_registry::AgentRegistryEntry>, +) -> AgentRoute { + match entry { + Some(e) if e.enabled => AgentRoute::Harness, + _ => AgentRoute::RegistryFallback, + } +} + impl OpenHumanAgentRunner { /// Full harness turn: build a real session agent for `agent_ref` and drive /// one `run_single` under the node's model override + timeout. See /// [`OpenHumanAgentRunner`] for the security/origin contract. + /// + /// `entry_model` is the custom `AgentRegistryEntry`'s own `model` pin (a + /// `hint:` or raw BYOK model id), when `agent_ref` resolved to one — + /// `None` for a shipped/TOML harness definition, which has no such entry. + /// It is the fallback below the node's own `config.model` override (see + /// `resolve_node_model`), matching the precedence + /// `run_via_registry_fallback` already gave `entry.model` — without this, + /// a custom flow agent's model pin (e.g. `hint:reasoning`) silently + /// dropped to the default chat model once routed through the harness. async fn run_via_harness( &self, agent_ref: &str, request: Value, conn: Option<&str>, + entry_model: Option<&str>, ) -> Result { use crate::openhuman::agent::Agent; @@ -931,9 +1008,9 @@ impl OpenHumanAgentRunner { } // Model precedence for a harness node: node `config.model` > the - // definition's own default. There is no custom registry `entry_model` on - // this path. - let node_model = resolve_node_model(&request, None); + // custom registry entry's own `model` pin (if any) > the definition's + // own default. + let node_model = resolve_node_model(&request, entry_model); // Apply the override the cron way (`run_agent_job`): a cloned `Config` // with a new `default_model`, so we never mutate the shared config or diff --git a/src/openhuman/tinyflows/tests.rs b/src/openhuman/tinyflows/tests.rs index 6a097c4ad0..dd36c8516f 100644 --- a/src/openhuman/tinyflows/tests.rs +++ b/src/openhuman/tinyflows/tests.rs @@ -539,8 +539,8 @@ async fn preflight_invoker_gates_the_mock_tool_path() { use super::caps::{ build_agent_result, clamp_run_timeout_secs, harness_model_default_override, - node_request_to_prompt, resolve_node_model, route_for_agent_ref, structured_output_instruction, - AgentRoute, + node_request_to_prompt, resolve_node_model, route_custom_entry_lookup, route_for_agent_ref, + structured_output_instruction, AgentRoute, }; #[test] @@ -697,3 +697,56 @@ fn route_for_agent_ref_selects_harness_for_definitions_else_fallback() { AgentRoute::RegistryFallback ); } + +// ── B38 (Gap 2): a custom agent_ref must route to the harness (real tools), +// not the persona-only completion fallback ───────────────────────────────── + +fn custom_registry_entry(enabled: bool) -> crate::openhuman::agent_registry::AgentRegistryEntry { + use crate::openhuman::agent_registry::types::{AgentRegistrySource, AgentSubagentPolicy}; + crate::openhuman::agent_registry::AgentRegistryEntry { + id: "finance_analyst".to_string(), + name: "Finance Analyst".to_string(), + description: "Reviews spend and drafts finance summaries.".to_string(), + source: AgentRegistrySource::Custom, + enabled, + model: Some("hint:reasoning".to_string()), + system_prompt: Some("You are a meticulous finance analyst.".to_string()), + tool_allowlist: vec!["memory_search".to_string()], + tool_denylist: Vec::new(), + subagents: AgentSubagentPolicy::default(), + tags: Vec::new(), + metadata: json!(null), + } +} + +#[test] +fn route_custom_entry_lookup_routes_enabled_custom_entry_to_harness() { + let entry = custom_registry_entry(true); + assert_eq!( + route_custom_entry_lookup(Some(&entry)), + AgentRoute::Harness, + "a known, enabled custom-registry agent must run through the harness (real tools), \ + not the persona-only completion fallback" + ); +} + +#[test] +fn route_custom_entry_lookup_falls_back_for_disabled_custom_entry() { + let entry = custom_registry_entry(false); + assert_eq!( + route_custom_entry_lookup(Some(&entry)), + AgentRoute::RegistryFallback, + "a disabled custom entry must still go through run_via_registry_fallback, which \ + rejects it with a clear \"is disabled\" error" + ); +} + +#[test] +fn route_custom_entry_lookup_falls_back_when_no_entry_exists() { + assert_eq!( + route_custom_entry_lookup(None), + AgentRoute::RegistryFallback, + "an agent_ref unknown to both the harness registry and the custom config registry \ + must still resolve through the fallback (which reports \"unknown agent_ref\")" + ); +}