From afdd344969e0377e35c3d0c7bb0fc7d59c93f221 Mon Sep 17 00:00:00 2001 From: shanu Date: Wed, 12 Aug 2026 18:32:14 +0530 Subject: [PATCH] fix(hosted/orchestration): scope turn origin for local-agent spawn MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `AGENT_TURN_ORIGIN` is a `tokio::task_local`, so it does not cross `tokio::spawn`. `effect_executor::run_local_agent` fires the local sub-agent from a bare `tokio::spawn` with no agent turn on the stack, so `turn_origin::current()` was `None` on the spawned task, the approval gate read `AgentTurnOrigin::Unknown`, and every external_effect tool the sub-agent called (cron_add / cron_update / shell / …) was denied as "agent turn has no origin label". PR #5465 fixed the four canonical spawn sites (orchestration spawn_agent, spawn_async_subagent, workflow runs, agent teams) via `capture`/`propagate`/`with_inherited_origin`. This closes the residual `effect_executor` gap. There is no ambient turn to inherit here (the device-tool bridge is not itself an agent turn), so an explicit `AgentTurnOrigin::Cli` is scoped around the spawned work — this path only runs after the device-authoritative Master-chat gate in `dispatch_device_tool` has passed, matching the turn-less internal dispatch label already used by `mcp::registry` and `task_dispatcher`. Also makes the gate's `Unknown` deny message specific and actionable: it names the missing origin label, the scheduling/external-effect tools it blocks, and frames it as an internal wiring gap rather than user error, without leaking internals. Tests: gate unit test that a `cron_add` call on a turn-less/delegated spawn resolves to a real origin (allowed) instead of `Unknown`-denied; turn_origin propagation test covering the explicit-`Cli`-across-spawn shape of the effect_executor site. Closes #5508, #5499 --- src/openhuman/agent/turn_origin.rs | 28 ++++++++ .../hosted/orchestration/effect_executor.rs | 55 +++++++++++--- src/openhuman/security/approval/gate.rs | 72 +++++++++++++++++-- ...tool_registry_approval_raw_coverage_e2e.rs | 2 +- 4 files changed, 141 insertions(+), 16 deletions(-) diff --git a/src/openhuman/agent/turn_origin.rs b/src/openhuman/agent/turn_origin.rs index 02c7530ca5..eb81066e8f 100644 --- a/src/openhuman/agent/turn_origin.rs +++ b/src/openhuman/agent/turn_origin.rs @@ -419,6 +419,34 @@ mod tests { assert!(observed.is_none()); } + /// The `hosted::orchestration::effect_executor::run_local_agent` spawn site + /// (#5508 / #5499): the device-tool bridge fires the local sub-agent from a + /// bare `tokio::spawn` where there is **no ambient turn** to inherit — unlike + /// the four sites PR #5465 fixed with `capture`/`propagate`, `capture()` here + /// is `None`, so `with_inherited_origin` would leave the task `Unknown` and + /// the gate would refuse every external-effect tool (`cron_add`, shell, …). + /// The fix scopes an **explicit** `Cli` origin on the spawned task instead + /// (device automation past the Master-chat gate is trusted, turn-less + /// internal dispatch). This pins that shape: nothing to inherit on the + /// parent, a real `Cli` origin observed across the spawn boundary. + #[tokio::test] + async fn explicit_cli_origin_survives_a_turnless_spawn() { + // No outer scope — exactly the device-tool bridge's situation. + assert!( + capture().is_none(), + "precondition: the effect_executor spawn has no ambient origin to inherit" + ); + + let observed = tokio::spawn(with_origin(AgentTurnOrigin::Cli, async { current() })) + .await + .expect("spawned task panicked"); + + assert!( + matches!(observed, Some(AgentTurnOrigin::Cli)), + "the explicitly-scoped Cli origin must be visible on the spawned task, got {observed:?}" + ); + } + #[tokio::test] async fn current_returns_none_outside_scope() { assert!(current().is_none()); diff --git a/src/openhuman/hosted/orchestration/effect_executor.rs b/src/openhuman/hosted/orchestration/effect_executor.rs index 7843654154..a5e6a07a9b 100644 --- a/src/openhuman/hosted/orchestration/effect_executor.rs +++ b/src/openhuman/hosted/orchestration/effect_executor.rs @@ -159,17 +159,44 @@ async fn run_local_agent(args: &Value, cycle_id: &str) -> Result let task_id = cycle_id.to_string(); let run_args = args.clone(); let bg_task_id = task_id.clone(); - tokio::spawn(async move { - if let Err(e) = - run_local_agent_and_forward(&counterpart, &session_id, &bg_task_id, &agent_id, run_args) - .await - { - log::warn!( - target: LOG, - "[orchestration] run_local_agent.forward_failed task={bg_task_id}: {e}" - ); - } - }); + // Scope an explicit turn origin around the spawned sub-agent work. + // + // This runs on a fresh `tokio::spawn` task with no agent turn on the stack, + // and `AGENT_TURN_ORIGIN` is a `tokio::task_local` that does NOT cross + // `tokio::spawn` — so `turn_origin::current()` here is `None`, the approval + // gate reads `AgentTurnOrigin::Unknown`, and every external_effect tool the + // sub-agent calls (cron_add / cron_update / shell / …) is refused as "no + // origin label" (issues #5508, #5499). PR #5465 fixed the four canonical + // spawn sites via capture/propagate; this is the residual one. + // + // Unlike those sites there is no ambient origin to inherit (the device-tool + // bridge is not itself an agent turn), so `with_inherited_origin(None, …)` + // would stay `Unknown` and keep failing closed. We instead scope an explicit + // `AgentTurnOrigin::Cli`: this only runs after the device-authoritative + // Master-chat gate in `dispatch_device_tool` has passed (a compromised cloud + // brain cannot reach here for an A2A cycle), so it is trusted, turn-less, + // device-initiated internal dispatch — exactly the label `mcp::registry`'s + // credential-help turn and `task_dispatcher` use for the same shape. + let bg = crate::openhuman::agent::turn_origin::with_origin( + crate::openhuman::agent::turn_origin::AgentTurnOrigin::Cli, + async move { + if let Err(e) = run_local_agent_and_forward( + &counterpart, + &session_id, + &bg_task_id, + &agent_id, + run_args, + ) + .await + { + log::warn!( + target: LOG, + "[orchestration] run_local_agent.forward_failed task={bg_task_id}: {e}" + ); + } + }, + ); + tokio::spawn(bg); Ok(json!({ "accepted": true, "taskId": task_id, @@ -190,6 +217,12 @@ async fn run_local_agent(args: &Value, cycle_id: &str) -> Result /// sub-agent; without it the nested spawn failed `NoParentContext` /// ("spawn_async_subagent called outside of an agent turn"). /// +/// The `AGENT_TURN_ORIGIN` task-local is a **separate** axis and is scoped by +/// [`run_local_agent`] around the spawn (as [`AgentTurnOrigin::Cli`]), not here — +/// `turn_origin.rs` forbids manufacturing a label inside `run_subagent`. Without +/// that outer scope the sub-agent's external_effect tools (cron_add / shell / …) +/// would reach the approval gate as `Unknown` and be refused (#5508, #5499). +/// /// We call `run_subagent` (synchronous, real `output`) rather than the /// `spawn_async_subagent` tool wrapper on purpose: the wrapper defaults to the /// async path (returning a `[async_subagent_ref]`, not the answer) and gates on diff --git a/src/openhuman/security/approval/gate.rs b/src/openhuman/security/approval/gate.rs index f99321ab1d..203f7590b2 100644 --- a/src/openhuman/security/approval/gate.rs +++ b/src/openhuman/security/approval/gate.rs @@ -833,9 +833,14 @@ impl ApprovalGate { return ( GateOutcome::Deny { reason: format!( - "{POLICY_DENIED_MARKER} Tool '{tool_name}' rejected: agent turn has \ - no origin label. Refusing external_effect tool from unlabelled call \ - site." + "{POLICY_DENIED_MARKER} '{tool_name}' was blocked because this agent \ + turn is missing its origin label, so the approval gate cannot decide \ + who requested the action. Scheduling and other external-effect tools \ + (e.g. cron_add / cron_update) are refused when the turn has no origin. \ + This is an internal wiring gap, not something you did — the work most \ + likely ran on a background task that did not carry the turn's origin \ + forward; retry from a normal chat turn, or report it so the spawn site \ + can be fixed." ), }, None, @@ -2155,7 +2160,15 @@ mod tests { .await; match outcome { - GateOutcome::Deny { reason } => assert!(reason.contains("no origin label")), + // The deny message is specific and actionable (issues #5508 / #5499, + // 2nd acceptance criterion): it names the missing origin label, calls + // out the scheduling/external-effect tools it affects, and frames it + // as an internal wiring gap rather than user error. + GateOutcome::Deny { reason } => { + assert!(reason.contains("origin label"), "reason was: {reason}"); + assert!(reason.contains("cron_add"), "reason was: {reason}"); + assert!(reason.contains("external-effect"), "reason was: {reason}"); + } other => panic!("expected deny, got {other:?}"), } } @@ -2932,6 +2945,57 @@ mod tests { assert!(matches!(outcome, GateOutcome::Allow)); } + /// Regression for #5508 / #5499: an external-effect scheduling tool + /// (`cron_add`) that runs on a freshly-spawned, turn-less task — the exact + /// shape of `hosted::orchestration::effect_executor::run_local_agent`, which + /// fires the local sub-agent from a bare `tokio::spawn` with no agent turn on + /// the stack — must NOT be `Unknown`-denied once the spawn site scopes an + /// explicit `AgentTurnOrigin::Cli` (the residual site PR #5465 did not cover). + /// + /// Both halves run inside a `tokio::spawn` so the assertion exercises the real + /// task boundary the fix crosses: `AGENT_TURN_ORIGIN` is a `tokio::task_local` + /// that does not survive `spawn`, so the origin the gate reads is whatever the + /// spawned future scopes for itself — nothing, or the fix's explicit label. + #[tokio::test] + async fn cron_add_on_a_turnless_spawn_resolves_to_a_real_origin_not_unknown_denied() { + let (gate, _dir) = test_gate(); + let gate = Arc::new(gate); + + // Precondition — mirrors the bug before the fix: a bare `tokio::spawn` + // with no ambient origin (capture() would yield None) reaches the gate as + // `Unknown`, and the scheduling tool is refused as "no origin label". + let g = gate.clone(); + let denied = tokio::spawn(async move { + g.intercept("cron_add", "schedule a job", serde_json::json!({})) + .await + }) + .await + .expect("spawned task panicked"); + match denied { + GateOutcome::Deny { reason } => { + assert!(reason.contains("origin label"), "reason was: {reason}") + } + other => panic!("unlabelled turn-less spawn must fail closed, got {other:?}"), + } + + // With the fix: `run_local_agent` scopes an explicit `Cli` origin around + // the spawned sub-agent work, so the same `cron_add` call now resolves to + // a real origin and is allowed (device-tool automation past the + // Master-chat gate) instead of being denied as unlabelled. + let g = gate.clone(); + let allowed = tokio::spawn(turn_origin::with_origin(AgentTurnOrigin::Cli, async move { + g.intercept("cron_add", "schedule a job", serde_json::json!({})) + .await + })) + .await + .expect("spawned task panicked"); + assert!( + matches!(allowed, GateOutcome::Allow), + "an explicit Cli origin scoped across the spawn must resolve cron_add \ + to a real origin and allow it, got {allowed:?}" + ); + } + #[tokio::test] async fn intercept_with_external_channel_origin_persists_and_ttl_denies() { // Non-web channel inbound (Telegram / Discord / Slack / etc.): diff --git a/tests/raw_coverage/tool_registry_approval_raw_coverage_e2e.rs b/tests/raw_coverage/tool_registry_approval_raw_coverage_e2e.rs index e562102aea..44fb50a4f4 100644 --- a/tests/raw_coverage/tool_registry_approval_raw_coverage_e2e.rs +++ b/tests/raw_coverage/tool_registry_approval_raw_coverage_e2e.rs @@ -1371,7 +1371,7 @@ async fn approval_rpc_decision_paths_persist_always_allow_and_recent_audit() { match &no_chat.0 { openhuman_core::openhuman::security::approval::GateOutcome::Deny { reason } => { assert!( - reason.contains("no origin label"), + reason.contains("origin label"), "unlabelled call should be denied for missing origin: {reason}" ); }