diff --git a/src/openhuman/agent/harness/definition.rs b/src/openhuman/agent/harness/definition.rs index 944e56954..8106c5a4e 100644 --- a/src/openhuman/agent/harness/definition.rs +++ b/src/openhuman/agent/harness/definition.rs @@ -312,6 +312,57 @@ impl AgentTier { } } +impl std::fmt::Display for AgentTier { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.write_str(self.as_str()) + } +} + +/// Single source of truth for the spawn-hierarchy rule: is a `parent`-tier +/// agent allowed to delegate to a `child`-tier agent? +/// +/// Returns `Ok(())` for the legal handoffs and `Err(reason)` for the three +/// forbidden shapes, where `reason` is a tier-only human-readable explanation +/// (no agent ids — callers prepend their own context): +/// +/// - `Worker → *` — workers are leaf executors and must not spawn anything. +/// - `Chat → Chat` — the chat tier is a leaf in its own dimension; cloning it +/// defeats the fast-path and risks unbounded `chat → chat → …` chains. +/// - `Reasoning → Reasoning` — reasoning agents compose downward into workers, +/// not into each other (a depth-blowing recursion of slow models). +/// +/// Note this forbids same-tier and worker-as-parent hops, **not** upward hops: +/// `reasoning → chat` is a real, intentional builtin edge (the `subconscious` +/// reasoner can hand a follow-up back to the `orchestrator` chat agent), so it +/// must stay legal. The harness'es `MAX_SPAWN_DEPTH` cap bounds chain length +/// independently of tier direction. +/// +/// This is the static authoring rule the loader walks over declared `subagents` +/// pairs at boot (see +/// [`crate::openhuman::agent_registry::agents::validate_tier_hierarchy`]). The +/// runtime spawn gate (`run_subagent`) reuses it as defense-in-depth, but +/// deliberately exempts worker *parents* — at runtime a worker only reaches the +/// spawn chokepoint via the documented collapsed `delegate_to_integrations_agent` +/// path (→ `integrations_agent`, itself a worker), which the loader intentionally +/// leaves untouched. +pub fn validate_tier_transition(parent: AgentTier, child: AgentTier) -> Result<(), String> { + match (parent, child) { + (AgentTier::Worker, _) => Err(format!( + "a `worker` tier agent must not spawn `{}` — workers are leaf executors", + child.as_str() + )), + (AgentTier::Chat, AgentTier::Chat) => Err( + "the chat tier is a leaf in its own dimension — hand off to a `reasoning` or \ + `worker` agent instead" + .to_string(), + ), + (AgentTier::Reasoning, AgentTier::Reasoning) => { + Err("reasoning agents compose downward into workers, not into each other".to_string()) + } + _ => Ok(()), + } +} + // ───────────────────────────────────────────────────────────────────────────── // Subagent delegation entries // ───────────────────────────────────────────────────────────────────────────── diff --git a/src/openhuman/agent/harness/definition_tests.rs b/src/openhuman/agent/harness/definition_tests.rs index ddf3c8441..fe4a0c5da 100644 --- a/src/openhuman/agent/harness/definition_tests.rs +++ b/src/openhuman/agent/harness/definition_tests.rs @@ -281,3 +281,61 @@ max_iterations = 1 assert_eq!(def.iteration_policy, IterationPolicy::Strict); assert_eq!(def.effective_max_iterations(), 1); } + +// ── Spawn-hierarchy tier rule (validate_tier_transition) ──────────────────── +// Single source of truth shared by the boot loader walk and the runtime +// spawn gate in `run_subagent` (issue #4098). + +#[test] +fn tier_transition_allows_legal_descending_handoffs() { + // chat → reasoning, chat → worker, reasoning → worker are the only + // legal hops; each strictly descends the hierarchy. + assert!(validate_tier_transition(AgentTier::Chat, AgentTier::Reasoning).is_ok()); + assert!(validate_tier_transition(AgentTier::Chat, AgentTier::Worker).is_ok()); + assert!(validate_tier_transition(AgentTier::Reasoning, AgentTier::Worker).is_ok()); +} + +#[test] +fn tier_transition_rejects_chat_to_chat() { + let err = validate_tier_transition(AgentTier::Chat, AgentTier::Chat) + .expect_err("chat→chat must be rejected"); + // Wording must carry the substrings the loader diagnostics rely on. + assert!(err.contains("chat") && err.contains("leaf"), "got: {err}"); +} + +#[test] +fn tier_transition_rejects_reasoning_to_reasoning() { + let err = validate_tier_transition(AgentTier::Reasoning, AgentTier::Reasoning) + .expect_err("reasoning→reasoning must be rejected"); + assert!(err.contains("reasoning"), "got: {err}"); +} + +#[test] +fn tier_transition_allows_upward_reasoning_to_chat() { + // Upward delegation is intentionally legal: the `subconscious` reasoner + // hands follow-ups back to the `orchestrator` chat agent. Only same-tier + // and worker-as-parent hops are forbidden. + assert!(validate_tier_transition(AgentTier::Reasoning, AgentTier::Chat).is_ok()); +} + +#[test] +fn tier_transition_rejects_worker_as_parent() { + // A worker is a leaf executor — it may not spawn any tier, including + // another worker. + for child in [AgentTier::Chat, AgentTier::Reasoning, AgentTier::Worker] { + let err = validate_tier_transition(AgentTier::Worker, child) + .expect_err("worker must not spawn any tier"); + assert!( + err.contains("worker") && err.contains("leaf"), + "worker→{} reason should call out the worker-leaf rule, got: {err}", + child.as_str() + ); + } +} + +#[test] +fn tier_display_matches_as_str() { + assert_eq!(AgentTier::Chat.to_string(), "chat"); + assert_eq!(AgentTier::Reasoning.to_string(), "reasoning"); + assert_eq!(AgentTier::Worker.to_string(), "worker"); +} diff --git a/src/openhuman/agent/harness/subagent_runner/ops/runner.rs b/src/openhuman/agent/harness/subagent_runner/ops/runner.rs index e72c3df57..957433eac 100644 --- a/src/openhuman/agent/harness/subagent_runner/ops/runner.rs +++ b/src/openhuman/agent/harness/subagent_runner/ops/runner.rs @@ -11,7 +11,8 @@ use std::sync::Arc; use std::time::Instant; use crate::openhuman::agent::harness::definition::{ - AgentDefinition, IterationPolicy, PromptSource, + validate_tier_transition, AgentDefinition, AgentDefinitionRegistry, AgentTier, IterationPolicy, + PromptSource, }; use crate::openhuman::agent::harness::fork_context::{current_parent, ParentExecutionContext}; use crate::openhuman::agent::harness::subagent_runner::extract_tool::ExtractFromResultTool; @@ -37,6 +38,58 @@ use super::provider::{ resolve_subagent_provider, user_is_signed_in_to_composio, LazyToolkitResolver, }; +/// Runtime spawn-hierarchy gate decision for one delegation hop. +/// +/// `parent_def` is the resolved parent agent definition (looked up from the +/// global registry by its definition id) or `None` when the parent can't be +/// resolved — e.g. a dynamically-named agent (model-council juror) or a custom +/// agent absent from the registry, or any context where the registry isn't +/// initialised. A `None` parent yields `Ok(())`: we skip rather than mask, the +/// same defensive posture the loader takes for unknown child ids. +/// +/// A **worker** parent is also exempted. At runtime a worker only reaches the +/// spawn chokepoint via the documented collapsed `delegate_to_integrations_agent` +/// path (→ `integrations_agent`, itself a worker) — a shape the loader +/// intentionally leaves untouched. Re-denying it here would turn valid custom +/// worker agents that use `{ skills = "*" }` into runtime failures. The +/// worker-leaf authoring rule stays enforced statically at boot, and the +/// per-parent allowlist gate blocks any other worker spawn. +/// +/// For chat / reasoning parents the hop is checked against +/// [`validate_tier_transition`] (the single source of truth shared with the +/// boot loader walk); a forbidden hop is logged and becomes a +/// [`SubagentRunError::TierViolation`]. Logging lives here (rather than at the +/// call site) so the deny path is exercised by this fn's unit tests. +pub(crate) fn tier_gate_decision( + parent_def: Option<&AgentDefinition>, + child: &AgentDefinition, + parent_agent_id: &str, + task_id: &str, +) -> Result<(), SubagentRunError> { + let Some(parent_def) = parent_def else { + return Ok(()); + }; + if parent_def.agent_tier == AgentTier::Worker { + return Ok(()); + } + if let Err(reason) = validate_tier_transition(parent_def.agent_tier, child.agent_tier) { + tracing::warn!( + parent_agent = %parent_agent_id, + parent_tier = %parent_def.agent_tier, + child_agent = %child.id, + child_tier = %child.agent_tier, + task_id = %task_id, + "[subagent_runner] blocked tier-violating delegation: {reason}" + ); + return Err(SubagentRunError::TierViolation { + parent_tier: parent_def.agent_tier, + child_tier: child.agent_tier, + reason, + }); + } + Ok(()) +} + /// Run a sub-agent based on its definition and a task prompt. /// /// This is the primary entry point for agent delegation. It performs the following: @@ -90,6 +143,17 @@ pub async fn run_subagent( }); } + // Runtime spawn-hierarchy (tier) gate — defense-in-depth alongside the + // depth gate above. The loader validates *declared* `subagents` pairs + // statically at boot (`validate_tier_hierarchy`), but dynamic, custom, + // or model-chosen spawns reach this chokepoint without ever passing + // through that walk. Resolve the parent's tier from the registry by its + // definition id; `tier_gate_decision` rejects (and logs) any forbidden + // chat/reasoning hop while exempting unresolved + worker parents. + let parent_def = + AgentDefinitionRegistry::global().and_then(|reg| reg.get(&parent.agent_definition_id)); + tier_gate_decision(parent_def, definition, &parent.agent_definition_id, &task_id)?; + tracing::info!( agent_id = %definition.id, task_id = %task_id, diff --git a/src/openhuman/agent/harness/subagent_runner/ops_tests.rs b/src/openhuman/agent/harness/subagent_runner/ops_tests.rs index 9674cb595..ab079a7a9 100644 --- a/src/openhuman/agent/harness/subagent_runner/ops_tests.rs +++ b/src/openhuman/agent/harness/subagent_runner/ops_tests.rs @@ -1542,3 +1542,99 @@ fn prefix_tier_refuses_ambiguous_and_short_slugs() { // Short slug below the length gate never engages the prefix tier. assert!(resolver.resolve("NOTION").is_none()); } + +// ── Runtime spawn-hierarchy (tier) gate (issue #4098) ─────────────────────── +// `tier_gate_decision` is the pure decision the runtime gate in `run_subagent` +// applies to each delegation hop. Tested directly so the deny/allow/skip +// table is covered without standing up a global registry or a live spawn. + +// Thin wrapper to call the gate with throwaway log-context ids. +fn gate(parent: Option<&AgentDefinition>, child: &AgentDefinition) -> Result<(), SubagentRunError> { + super::runner::tier_gate_decision(parent, child, "parent-agent", "task-1") +} + +#[test] +fn tier_gate_skips_when_parent_unresolved() { + use crate::openhuman::agent::harness::definition::AgentTier; + // No resolvable parent definition (e.g. registry uninitialised, or a + // dynamically-named model-council juror / custom agent absent from it) → + // skip rather than mask. Even a would-be-illegal child tier passes, because + // we have no parent tier to judge against. + let mut child = make_def_named_tools(&[]); + child.agent_tier = AgentTier::Chat; + assert!(gate(None, &child).is_ok()); +} + +#[test] +fn tier_gate_allows_legal_descending_hops() { + use crate::openhuman::agent::harness::definition::AgentTier; + let mut parent = make_def_named_tools(&[]); + let mut child = make_def_named_tools(&[]); + + // chat → worker + parent.agent_tier = AgentTier::Chat; + child.agent_tier = AgentTier::Worker; + assert!(gate(Some(&parent), &child).is_ok()); + + // chat → reasoning + child.agent_tier = AgentTier::Reasoning; + assert!(gate(Some(&parent), &child).is_ok()); + + // reasoning → worker + parent.agent_tier = AgentTier::Reasoning; + child.agent_tier = AgentTier::Worker; + assert!(gate(Some(&parent), &child).is_ok()); +} + +#[test] +fn tier_gate_allows_worker_parent_for_collapsed_integration() { + use crate::openhuman::agent::harness::definition::AgentTier; + // A worker only reaches the runtime spawn chokepoint via the documented + // collapsed `delegate_to_integrations_agent` path (→ `integrations_agent`, + // itself a worker). The gate must NOT re-deny that — the worker-leaf rule + // is a static boot-time authoring constraint, not a runtime one. Regression + // for the wildcard-integration case (CodeRabbit P2 on PR #4102). + let mut parent = make_def_named_tools(&[]); + let child = make_def_named_tools(&[]); // worker by default + parent.agent_tier = AgentTier::Worker; + assert!(gate(Some(&parent), &child).is_ok()); +} + +#[test] +fn tier_gate_denies_chat_to_chat() { + use crate::openhuman::agent::harness::definition::AgentTier; + let mut parent = make_def_named_tools(&[]); + let mut child = make_def_named_tools(&[]); + parent.agent_tier = AgentTier::Chat; + child.agent_tier = AgentTier::Chat; + + let err = + gate(Some(&parent), &child).expect_err("chat→chat must be denied at the runtime gate"); + match err { + SubagentRunError::TierViolation { + parent_tier, + child_tier, + reason, + } => { + assert_eq!(parent_tier, AgentTier::Chat); + assert_eq!(child_tier, AgentTier::Chat); + assert!( + reason.contains("chat") && reason.contains("leaf"), + "got: {reason}" + ); + } + other => panic!("expected TierViolation, got: {other:?}"), + } +} + +#[test] +fn tier_gate_allows_upward_reasoning_to_chat() { + use crate::openhuman::agent::harness::definition::AgentTier; + // Upward delegation is intentionally legal (subconscious reasoner → + // orchestrator chat). The gate must not deny it. + let mut parent = make_def_named_tools(&[]); + let mut child = make_def_named_tools(&[]); + parent.agent_tier = AgentTier::Reasoning; + child.agent_tier = AgentTier::Chat; + assert!(gate(Some(&parent), &child).is_ok()); +} diff --git a/src/openhuman/agent/harness/subagent_runner/types.rs b/src/openhuman/agent/harness/subagent_runner/types.rs index c67f45d2a..f29073a55 100644 --- a/src/openhuman/agent/harness/subagent_runner/types.rs +++ b/src/openhuman/agent/harness/subagent_runner/types.rs @@ -7,6 +7,7 @@ use std::path::PathBuf; use std::time::Duration; use thiserror::Error; +use crate::openhuman::agent::harness::definition::AgentTier; use crate::openhuman::inference::provider::ChatMessage; /// Per-spawn options that override or augment what the @@ -178,6 +179,16 @@ pub enum SubagentRunError { max_depth: usize, }, + #[error( + "delegation blocked by the spawn-hierarchy gate: a `{parent_tier}` agent may not \ + delegate to a `{child_tier}` agent — {reason}" + )] + TierViolation { + parent_tier: AgentTier, + child_tier: AgentTier, + reason: String, + }, + #[error("sub-agent exceeded maximum iterations ({0})")] MaxIterationsExceeded(usize), } diff --git a/src/openhuman/agent_registry/agents/loader.rs b/src/openhuman/agent_registry/agents/loader.rs index 58b4390cb..65b442813 100644 --- a/src/openhuman/agent_registry/agents/loader.rs +++ b/src/openhuman/agent_registry/agents/loader.rs @@ -35,7 +35,8 @@ //! collision. use crate::openhuman::agent::harness::definition::{ - AgentDefinition, AgentTier, DefinitionSource, PromptBuilder, PromptSource, SubagentEntry, + validate_tier_transition, AgentDefinition, AgentTier, DefinitionSource, PromptBuilder, + PromptSource, SubagentEntry, }; use anyhow::{Context, Result}; use std::collections::HashMap; @@ -305,22 +306,18 @@ pub fn validate_tier_hierarchy(defs: &[AgentDefinition]) -> Result<()> { // Same-tier delegation is forbidden for chat and reasoning. // (Chat→Chat would defeat the whole point of the fast tier; // Reasoning→Reasoning produces a depth-blowing recursion of - // slow models.) - match (def.agent_tier, child_tier) { - (AgentTier::Chat, AgentTier::Chat) => anyhow::bail!( - "agent `{parent}` (chat) lists `{child}` (chat) in subagents — the chat tier \ - is a leaf in its own dimension. Hand off to a `reasoning` or `worker` agent \ - instead.", + // slow models.) The pair-rule lives in `validate_tier_transition` + // (the single source of truth shared with the runtime spawn gate + // in `run_subagent`); here we wrap its reason with the offending + // agent ids + tiers for a boot-time-friendly diagnostic. + if let Err(reason) = validate_tier_transition(def.agent_tier, child_tier) { + anyhow::bail!( + "agent `{parent}` ({ptier}) lists `{child}` ({ctier}) in subagents — {reason}", parent = def.id, + ptier = def.agent_tier.as_str(), child = child_id, - ), - (AgentTier::Reasoning, AgentTier::Reasoning) => anyhow::bail!( - "agent `{parent}` (reasoning) lists `{child}` (reasoning) in subagents — \ - reasoning agents compose downward into workers, not into each other.", - parent = def.id, - child = child_id, - ), - _ => {} + ctier = child_tier.as_str(), + ); } } } diff --git a/src/openhuman/agent_registry/agents/orchestrator/prompt.md b/src/openhuman/agent_registry/agents/orchestrator/prompt.md index e2260f50a..6be9b0255 100644 --- a/src/openhuman/agent_registry/agents/orchestrator/prompt.md +++ b/src/openhuman/agent_registry/agents/orchestrator/prompt.md @@ -59,7 +59,7 @@ You can open and operate native apps on this machine, but you do it by **delegat - **You are the chat tier.** You run on a fast UX-focused model (TTFT > deep reasoning). When a task needs sustained multi-step thinking — planning across many steps, comparing several non-obvious options, untangling ambiguous requirements — **delegate to the reasoning tier (`delegate_plan`)** rather than reasoning through it yourself. Your job at that point is to brief the planner well and synthesise its output back to the user. - **Never spawn yourself** — You cannot delegate to another chat-tier agent (Orchestrator or otherwise). The chat tier is a leaf in its own dimension. -- **Spawn hierarchy (hard rule).** Allowed handoffs from here: `chat → worker` (fast path) or `chat → reasoning → worker` (deep path). Never `chat → chat` and never `chat → reasoning → reasoning`. The loader rejects same-tier delegation at boot; a runtime depth gate capping chains at 3 hops is a planned follow-up — until it lands, this rule is enforced by you, by the planner's matching rule, and by the static loader check. +- **Spawn hierarchy (hard rule).** Allowed handoffs from here: `chat → worker` (fast path) or `chat → reasoning → worker` (deep path). Never `chat → chat` and never `chat → reasoning → reasoning`. This is enforced in depth: the loader rejects same-tier delegation at boot, and the spawn chokepoint denies any tier-violating or over-deep spawn at runtime (a depth gate caps chains at 3 hops and a tier gate rejects the forbidden hops). Those gates are a safety net, not a license to mis-route — still follow the hierarchy yourself, as does the planner's matching rule. - **Minimise sub-agents** — Use the fewest agents necessary. Simple questions don't need a DAG. - **Direct-first always** — First try direct reply or direct tools; delegate only when required by task complexity/capability gaps. - **Context is expensive** — Pass only relevant context to sub-agents, not everything.