fix(orchestration): run device sub-agents from master chat; serialize master-ask seq

[YellowSnnowmann]

Summary

• Local device sub-agents (tools_agent, code_executor, researcher) requested by
  the hosted brain now actually run from a Master-chat cycle instead of failing.
• Master-chat "ask" now allocates its per-session seq inside an IMMEDIATE
  transaction, matching the DM path, so concurrent sends can't drop a wake.

Problem

• The device tool bridge ran spawn_async_subagent from a bare background task with no
  agent-turn on the stack, so current_parent() was None and the spawn was rejected
  with spawn_async_subagent called outside of an agent turn — every "check my skills"
  / "summarize my mails" request failed.
• The master-ask writer allocated MAX(seq)+1 then inserted outside a transaction,
  unlike the DM ingest writer on the same (local, "master") key. Two concurrent
  master sends could read the same max and emit a duplicate seq, which the backend
  idempotency-dedupes without waking.

Solution

• New run_local_subagent in effect_executor: installs a background root
  ParentExecutionContext via with_root_parent (provider/tools/memory/workspace from
  a config-built agent) and dispatches through run_subagent directly — the same path
  delegation / workflow / team surfaces use. run_subagent is synchronous (returns the
  real output, not an async ref) and skips the tool-only allowlist gate. Any failure
  becomes a (false, msg) completion so the brain always gets a tool_completion. The
  exec-gate (Master-chat-only) and the async forward contract are unchanged.
• Wrapped the master-ask next_session_seq→insert in store::in_immediate_txn,
  mirroring ingest.rs.

Submission Checklist

• Tests added or updated (happy path + failure/edge) — run_local_subagent
  (happy / unknown-agent / provider-error) and master-ask seq txn pending
• Diff coverage ≥ 80% — pending pnpm test:rust on the new lines
• Coverage matrix updated — N/A: orchestration device-exec is behaviour-fix
• All affected feature IDs listed under ## Related — N/A
• No new external network dependencies (reuses existing harness/composio paths)
• Manual smoke checklist updated — N/A: not a release-cut surface
• Linked issue closed via Closes #NNN in ## Related

Impact

• Desktop only (Rust core). Enables the plan's device-bound tool round-trip for
  Master-chat cycles; no change to A2A cycles (still gated by exec_gate).
• Each device sub-agent run builds a root parent (config-built agent + connected-
  integrations fetch) — a real provider/inference cost per invocation, by design.
• Security/parity: local execution stays restricted to Master-chat origin; no new
  wire fields; A2A denial path unchanged.

Summary by CodeRabbit

• New Features

  • Improved local assistant task execution and completion reporting.
  • Clarification questions and stopping reasons are now surfaced in task results.

• Bug Fixes

  • Improved reliability when running local assistant tasks, including clearer failure messages.
  • Prevented duplicate or out-of-order message sequence numbers during concurrent message handling.

[coderabbitai]

Warning

Review limit reached

You’ve reached a temporary PR review limit under our Fair Usage Limits Policy.

Your recent review volume is higher than typical usage, so adaptive limits are currently applied.

Next review available in: 12 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 6bd4dea0-640e-4f18-8900-002cf2982186

📥 Commits

Reviewing files that changed from the base of the PR and between 3c430d2 and 2049303.

📒 Files selected for processing (1)

• src/openhuman/orchestration/effect_executor.rs

📝 Walkthrough

Walkthrough

The orchestration flow adds centralized local subagent execution and result mapping, while the local Master-message path now atomically persists sequence allocation, session updates, and message insertion.

Changes

Local orchestration updates

Layer / File(s)	Summary
Local subagent execution src/openhuman/orchestration/effect_executor.rs	Adds run_local_subagent for definition lookup, subagent execution, status mapping, and failure reporting; run_local_agent_and_forward uses it for completion forwarding.
Atomic Master persistence src/openhuman/orchestration/schemas.rs	Wraps session sequence allocation, session upsert, and message insertion in one immediate transaction for the local-ask path.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Forwarder as run_local_agent_and_forward
  participant Helper as run_local_subagent
  participant Registry as AgentDefinitionRegistry
  participant Runner as run_subagent
  Forwarder->>Helper: Pass prompt and context
  Helper->>Registry: Resolve agent_id
  Helper->>Runner: Execute with root parent context
  Runner-->>Helper: Return SubagentRunStatus
  Helper-->>Forwarder: Return success flag and output

Loading

Possibly related PRs

• tinyhumansai/openhuman#4660: Modifies the related Master-chat receive/send flow in the same handler.
• tinyhumansai/openhuman#4738: Changes the same local-ask persistence and forwarding branch.
• tinyhumansai/openhuman#4753: Updates the related hosted local-agent execution path.

Suggested labels: feature, rust-core, agent, enhancement

Suggested reviewers: sanil-23, M3gA-Mind

Poem

I hop through helpers, swift and bright,
A local agent runs just right.
Master messages line up in tune,
One safe transaction under the moon.
Completion hops back—what a delight! 🐇

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately captures the two main changes: running local sub-agents from Master chat and serializing master-ask sequence allocation.
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.

---

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

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

src/openhuman/orchestration/effect_executor.rs (1)

288-309: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

Wrap the completion persist in in_immediate_txn — next_session_seq + insert_message here still run in autocommit, so the same (agent_id, session_id) can read the same MAX(seq) and write a duplicate ordinal. This path should mirror the master-ask writer and allocate + insert under one immediate transaction to avoid a dropped wake-up.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/openhuman/orchestration/effect_executor.rs` around lines 288 - 309, The
completion persistence in the tool-completion path is not atomic, allowing
duplicate session sequence values; update the block in the effect executor using
the existing in_immediate_txn helper, wrapping both next_session_seq and
insert_message in the same immediate transaction, while preserving the existing
error context and returned sequence.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Outside diff comments:
In `@src/openhuman/orchestration/effect_executor.rs`:
- Around line 288-309: The completion persistence in the tool-completion path is
not atomic, allowing duplicate session sequence values; update the block in the
effect executor using the existing in_immediate_txn helper, wrapping both
next_session_seq and insert_message in the same immediate transaction, while
preserving the existing error context and returned sequence.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 946a141d-8554-4e31-88af-1447ed886e0f

📥 Commits

Reviewing files that changed from the base of the PR and between f5b77ed and 3c430d2.

📒 Files selected for processing (2)

• src/openhuman/orchestration/effect_executor.rs
• src/openhuman/orchestration/schemas.rs

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 3c430d2473

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

[sanil-23]

Approving — the core logic is correct and verified:

• Seq serialization faithfully mirrors the ingest.rs with_connection(|c| in_immediate_txn(c, ...)) pattern on the same (local, "master") key, so it genuinely closes the concurrent MAX(seq) read-modify-write race (the duplicate-seq → backend idempotency-dedupe-without-waking failure). Inner logic is unchanged; only serialized.
• run_local_subagent installs a background root parent via with_root_parent and dispatches through run_subagent — the right fix for the turn-less NoParentContext spawn rejection. Every failure maps to (false, msg) so the brain always gets a tool_completion.
• Security preserved: local execution stays gated to Master-chat origin in dispatch_device_tool (is_local_execution_tool && !cycle_is_master → deny non_master_origin); this PR doesn't touch that gate. A2A cycles still can't induce local execution.

Non-blocking follow-ups (please address in this PR or a fast follow):

1. Tests / checklist accuracy. The checklist marks Tests added (happy/unknown-agent/provider-error) and Diff coverage ≥ 80% as done, but the body notes both are "pending" and no test code is in the diff. Coverage CI passed on the combined ratio (reindented + existing integration coverage), but the new failure branches — unknown-agent-id, provider/parent-build error, and AwaitingUser/Incomplete — are the ones worth the claimed unit tests. Please land them or correct the checkboxes.
2. codex P2 (toolkit options): SubagentRunOptions { context, ..Default::default() } forwards only context — any toolkit/integration options intended for the local run are defaulted away. Resolve or explicitly defer on the thread.
3. AwaitingUser semantics (minor): a clarification question yields (false, …), so the forwarded body reads … failed for a legitimate needs-input state. The question is surfaced, but consider a distinct marker so the brain re-prompts vs. treats it as an error.
4. Invariant hole (minor): a Config::load_or_init() failure returns Err and forwards no tool_completion, unlike every other failure path which sends (false, msg). Pre-existing behavior, but it leaves the "brain always learns the outcome" invariant with one gap.

Nice work — this is the client half of the master-chat wake round-trip and the seq fix directly closes a silent-drop class we hit on the backend side.