fix(approval): route WebChat approval parks via origin fallback

[YellowSnnowmann]

Summary

• Fix agent-scheduled cron_add/cron_update from a chat turn silently failing: the approval park is now routable, so the approval card surfaces and the user can approve instead of the call TTL-denying.
• Approval gate now falls back to the WebChat turn origin's thread_id/client_id for approval routing when the APPROVAL_CHAT_CONTEXT task-local is absent.

Problem

cron_add (and cron_update) are external_effect tools, so the approval gate is consulted. When the orchestrator routes a "remind me…" request to scheduler_agent, that runs as an async delegated sub-agent (spawn_async_subagent). The spawn propagates the WebChat turn origin (so the gate no longer sees Unknown — that half is handled by the turn-origin-propagation work in #5527) but does not propagate the APPROVAL_CHAT_CONTEXT task-local.

Result: the gate takes the WebChat park arm but reads chat_thread_id/chat_client_id as None. ApprovalRequested is published with thread_id: None, the web-channel ApprovalSurfaceSubscriber explicitly does not surface it ("thread/client absent — NOT surfacing"), the park waits the full TTL, and the call is denied. The scheduled reminder is never created, with no card ever shown to the user.

Solution

The WebChat origin already carries the same thread_id/client_id that APPROVAL_CHAT_CONTEXT does — both are built from the same values in web_chat::start_chat. So ApprovalGate::intercept now derives the chat routing from the origin when the task-local is absent. Scope is deliberately narrow: the fallback fires only for AgentTurnOrigin::WebChat; inline chat turns (task-local present) and every other origin are byte-for-byte unchanged.

Design note: fixing this in the gate — rather than propagating a second task-local at the spawn site — is minimal and covers every path where a WebChat turn reaches the gate without the approval context, not just async sub-agents.

Relationship to #5499 / #5527

The linked issue #5499 (agent turn has no origin label) was the pre-fix shape of this failure: the origin arrived as Unknown before propagation existed. #5527 landed the origin-propagation half and closed #5499 — it scopes the turn origin for the local-agent spawn and reworded the Unknown-origin denial into an actionable message. This PR closes the residual silent-park gap #5527 did not cover: even with a valid WebChat origin, an async-delegated sub-agent's approval never surfaces because the approval context task-local is absent. Rebased on current main, so the denial-message rewording that was in an earlier revision of this PR is dropped (already present from #5527) — the diff is now purely the routing fallback and its regression test.

Submission Checklist

• Tests added or updated (happy path + at least one failure / edge case) — new webchat_origin_routes_park_when_approval_chat_context_absent proves the park is routable via the origin when APPROVAL_CHAT_CONTEXT is absent (fails on old code)
• Diff coverage ≥ 80% — the only changed lines are the routing fallback (exercised by the new test) and its debug-log branch. Ran openhuman::security::approval + openhuman::agent::turn_origin (139 pass); cargo fmt --check and cargo clippy --lib clean.
• Coverage matrix updated — N/A: behaviour-only change (no feature row added/removed/renamed)
• All affected feature IDs from the matrix are listed under ## Related — N/A: behaviour-only change
• No new external network dependencies introduced
• Manual smoke checklist updated — N/A: does not touch a release-cut surface
• Linked issue referenced in the ## Related section

Impact

• Desktop/CLI (Rust core only — src/openhuman/security/approval/gate.rs). No frontend, no schema, no migration.
• Security: strictly narrows a fail-silent gap. Previously a WebChat-origin external-effect park with no approval context silently TTL-denied; it now surfaces its approval card in the originating thread. No origin other than WebChat changes behaviour, and the gate still parks (never auto-allows) — the user must still approve.
• Pre-existing limitation (unchanged): thread_to_request is one-park-per-thread; a parent turn and a concurrent async child parking on the same thread can overwrite the chat-reply route (card-click by request_id still resolves either).

Related

• Follows up: Bug: cron_add blocked by security policy — agent turn has no origin label #5499 (closed by fix(hosted/orchestration): scope turn origin for local-agent spawn so cron tools aren't denied #5527) — closes the residual silent-park gap that fix(hosted/orchestration): scope turn origin for local-agent spawn so cron tools aren't denied #5527 did not cover.
• Related: Bug: agent cron create/update fails inconsistently across cores — permissions error, UI path works fine #5508 (the multi-core / intermittent variant of the same cron authorization problem).

Summary by CodeRabbit

• Bug Fixes
  • Improved approval routing for WebChat conversations when chat context is unavailable.
  • Approvals now remain correctly associated with the originating conversation across asynchronous operations.
  • Added coverage to verify approval mapping and resolution using propagated conversation details.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 71e94111-dc22-44e6-8460-617875061d3b

📥 Commits

Reviewing files that changed from the base of the PR and between a4dc785 and 324af1e.

📒 Files selected for processing (1)

• src/openhuman/security/approval/gate.rs

🚧 Files skipped from review as they are similar to previous changes (1)

• src/openhuman/security/approval/gate.rs

---

📝 Walkthrough

Walkthrough

WebChat approvals now use AgentTurnOrigin::WebChat identifiers when task-local approval context is unavailable. A regression test covers routing, approval resolution, and route cleanup.

Changes

Approval routing

Layer / File(s)	Summary
WebChat origin routing fallback src/openhuman/security/approval/gate.rs	The approval gate falls back to thread and client IDs from AgentTurnOrigin::WebChat. The regression test verifies mapping, approval resolution, and cleanup without APPROVAL_CHAT_CONTEXT.

Estimated code review effort: 2 (Simple) | ~10 minutes

Mergeability Score: ⚪ Minimal · up to 324af

This localized change routes approval requests from WebChat turns to the originating conversation without changing approval requirements for other origins; no actionable merge-blocking risk remains beyond normal checks and review.

Possibly related PRs

• tinyhumansai/openhuman#5527 — Both changes modify approval-gate handling of AgentTurnOrigin propagation and routing behavior.

Suggested labels: rust-core, bug

Poem

A rabbit hops through async space,
And keeps the WebChat thread in place.
When task context fades from sight,
The origin guides approvals right.
Routes close cleanly by moonlight.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely describes the WebChat approval routing fix through origin fallback.

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[tinysweeper]

tinysweeper found nothing blocking. Approving.

             $0.0124 · 16,837 in / 3,372 out · 13,458 cached (80%) · z-ai/glm-5.2
critique:    $0.0030 · 3,967 in  / 872 out   · 3,348 cached (84%)  · z-ai/glm-5.2
security:    $0.0025 · 3,946 in  / 653 out   · 3,331 cached (84%)  · z-ai/glm-5.2
tests:       $0.0043 · 3,861 in  / 1,367 out · 3,072 cached (80%)  · z-ai/glm-5.2
description: $0.0027 · 5,063 in  / 480 out   · 3,707 cached (73%)  · z-ai/glm-5.2

[M3gA-Mind]

Review — comment only (no approval)

Reviewed 324af1e4f against main 47f4a0ec1. The fix is correct and I could not falsify the security-sensitive claims. Verification notes below, then one undisclosed gap.

Verified

No cross-routing / authorization hole. AgentTurnOrigin::WebChat is constructed at exactly three sites — web_chat/ops.rs:681 (primary turn), web_chat/ops.rs:909 (spawn_parallel_turn), flows/ops.rs:6931 (copilot flows_build) — and at every one of them the origin and APPROVAL_CHAT_CONTEXT are built from the same two values in the same statement pair. So the premise is verified at each site rather than assumed. The only way a WebChat origin reaches the gate without the context is turn_origin::propagate, which is inherit-only by construction (turn_origin.rs:246-252): Some(origin) re-scopes that exact origin, None scopes nothing. It cannot manufacture or substitute a label, and both call sites carry the parent's own origin. The fallback can therefore only route to the thread/client that initiated the turn chain. Also checked that the gate never uses absence of the chat context as a safety signal — chat_ctx feeds only chat_thread_id/chat_client_id, used exclusively for routing and cleanup (gate.rs:947, 962, 983-984, 992-993, 1080, 1216) — so supplying them from the origin cannot weaken a decision.

"byte-for-byte unchanged" holds. Effective diff is one file, +94/−2. Non-WebChat ⇒ origin_chat_route is None ⇒ or_else yields None, identical. WebChat with the task-local ⇒ map returns Some ⇒ or_else never evaluated, and the task-local correctly still wins. New debug! is guarded. match &origin is a borrow; nothing reordered. (The noisy two-dot diff is just the branch having merged an older main; merge-base is an ancestor of main and git diff-tree -m 324af1e4f shows src only — no hidden revert.)

No degenerate-origin case. WebChat's thread_id/client_id are String, not Option, and start_chat trims and rejects both when empty (ops.rs:402-411), so the fallback either supplies a real pair or does not fire.

The test is non-vacuous. With .or_else(...) reverted, chat_thread_id is None, the insert at gate.rs:947 never runs, pending_for_thread("thread-async") stays None, and assert!(tries < 50) trips. Timing is sound (2s test_gate TTL vs a 500ms poll budget; Config::default() neither allowlists cron_add nor sets auto_approve_all, so it genuinely parks).

No merge-order dependency. #5465 (origin propagation, merged 08-09) and #5527 (merged today) are both on main, and turn_origin::propagate is present at spawn_async_subagent.rs:546 on main and on this branch's merge-base. The body's account of the #5499/#5527 relationship is accurate.

F1 — MEDIUM, not currently disclosed: the surfaced card is cleared by the next turn on the thread, and while invisible a bare "ok" approves the background effect

The body discloses the thread_to_request one-slot limitation. The frontend has the same single-slot problem, and unlike the Rust side it has no ownership check:

• chatRuntimeSlice.ts:1722 stores one card per thread; :1724 clearPendingApprovalForThread is an unconditional delete by threadId — no request_id check, unlike clear_thread_route_if_owned (gate.rs:1434).
• ChatRuntimeProvider.tsx:1133 (turn done), :1285 (turn error), :1372 (disconnect) all dispatch that clear.

But spawn_async_subagent returns immediately ("continue without blocking", spawn_async_subagent.rs:864), so the park now deliberately outlives the turn whose thread it is keyed to.

Scenario A — the card vanishes, the park TTL-denies anyway. User asks for the reminder; parent turn ends; child parks and the card surfaces (the fix working). User then types anything else in that thread — that turn's chat_done deletes the child's card, though its park is still open and unrelated to that turn. No surface left → TTL-deny. #5499's symptom, one turn later.

Scenario B — conversational "ok" captured as an approval. While that now-invisible park is open, thread_to_request still points at it, so web_chat/ops.rs:510 intercepts the next message. parse_approval_reply (gate.rs:159) accepts yes | y | ok | okay | approve | approved | allow. So a user replying ok to something else (a) silently approves a background external effect they weren't answering and (b) never starts a turn — start_chat returns early, so their message produces no response. Before this PR a background park never entered thread_to_request, so it could not capture a bare ok. This path is new.

Cheapest mitigation that keeps the fix: for the origin-fallback park only, publish ApprovalRequested with the origin's thread/client (card renders) but skip the thread_to_request insert. The card decides by request_id (ApprovalRequestCard.tsx:46-48) so it still resolves cleanly; only the ambiguous free-text route is withheld from background parks. Alternatively make clearPendingApprovalForThread request_id-scoped, which also fixes scenario A for foreground parks. At minimum, fold both scenarios into the "Pre-existing limitation" note so the remaining exposure is on the record.

F2 — process note: the new test is not covered by a green lane yet

Rust Core Coverage (cargo-llvm-cov) is still pending, and Rust Quality (fmt, clippy) does not build test targets for this crate lane — so no green check has compiled or run mod tests. I take the author's local run (139 pass) at face value; just worth not calling the regression test CI-verified until that lane lands.

Nits

• N1. The gate-local fallback is defensible, but the root defect is a task-local that does not cross tokio::spawn, and the gate is not its only consumer — web3/wallet/execution.rs:286-296 (whose own comment warns the helper "will silently start…" without the scope), security/egress/emit.rs:73, and tools/impl/system/install_tool.rs:122 all still break across the same boundary. An approval::propagate_chat_context beside turn_origin::propagate/turn_workspace::propagate at the four feat(agent): make an embedded turn observable, rooted, and able to finish #5465 spawn sites would cover them all — though install_tool reads absence as "background → refuse" (:312), so that one needs its own thought. Follow-up issue, not a change request here.
• N2. The frontend/backend asymmetry in N1's clear-vs-clear_thread_route_if_owned is worth fixing on its own merits; Approval waiter/pending row leaks when a parked tool call is torn down by the turn wall-clock backstop #4774 already established the ownership-check pattern on the Rust side.

Sound, narrow, well-documented change that does what it claims and does not weaken the gate. F1 is the only thing I'd want resolved or explicitly recorded before treating #5499's class of failure as closed. Approve/merge is the maintainer's call — deliberately not approving.