feat(orchestration): local-exec device tool + trust gate + async completion forward (stacked on #4738)#4753
Merged
senamakel merged 1 commit intoJul 10, 2026
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (9)
🚧 Files skipped from review as they are similar to previous changes (8)
📝 WalkthroughWalkthroughThis PR adds cycleId-aware hosted cloud pushes, a hosted GET read surface (sessions/messages/steering), a device-authoritative exec gate tracking cycle origins, a gated ChangesHosted orchestration flow and execution gate
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Socket
participant EffectExecutor
participant ExecGate
participant LocalAgentTask
participant Cloud
Socket->>EffectExecutor: orch:tool_call (run_local_agent)
EffectExecutor->>ExecGate: cycle_is_master(cycle_id)
ExecGate-->>EffectExecutor: allow / deny
alt allowed
EffectExecutor->>LocalAgentTask: spawn run_local_agent
EffectExecutor-->>Socket: accepted/running result
LocalAgentTask->>LocalAgentTask: execute SpawnSubagentTool
LocalAgentTask->>Cloud: push_event(tool_completion)
Cloud-->>LocalAgentTask: cycleId
LocalAgentTask->>ExecGate: record_cycle_origin
else denied
EffectExecutor-->>Socket: error result
end
Possibly related PRs
Suggested labels: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
coderabbitai
Bot
added
agent
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4ae1f80c21
ℹ️ 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".
| /// hosted brain ships every terminal reply as a `send_dm`; the device decides | ||
| /// whether that reply is for a peer or for the user's own window. | ||
| fn is_self_session(session_id: &str) -> bool { | ||
| session_id.is_empty() || session_id == "master" || session_id == "subconscious" |
There was a problem hiding this comment.
In the peer-DM path, ingest::classify_message assigns plain external DMs to session_id master while counterpart_agent_id remains the peer. With this predicate, handle_send_dm takes the self-session return path for those hosted replies and never calls execute_send_dm, so responses to external Master-window DMs are only cached locally and are not sent over Signal. Please key self replies on the local counterpart, e.g. LOCAL_MASTER_AGENT, instead of the session id alone.
Useful? React with 👍 / 👎.
| use crate::openhuman::tools::traits::Tool; | ||
| // 1. Run the local sub-agent synchronously to completion (real output). | ||
| let tool = crate::openhuman::agent_orchestration::tools::SpawnSubagentTool::new(); | ||
| let result = tool.execute(run_args).await.map_err(|e| e.to_string())?; |
There was a problem hiding this comment.
For manifest-compliant run_local_agent calls, the args contain only agent_id/prompt/context. SpawnSubagentTool defaults blocking to false, so from this socket background task with no current_parent it routes into SpawnAsyncSubagentTool and returns spawn_async_subagent called outside of an agent turn; with a parent it would still forward only an async reference. As a result the device tool accepts the request but forwards a failed/placeholder completion instead of the local sub-agent's output. Force blocking: true or call a blocking runner before forwarding.
Useful? React with 👍 / 👎.
| ts, | ||
| "tool_completion", | ||
| ); | ||
| super::cloud::push_event(&config, &envelope).await?; |
There was a problem hiding this comment.
This new forward site ignores the cycleId returned by push_event, unlike the master ask and ingest forwarders. The tool_completion event wakes a fresh Master cycle; if that follow-up cycle needs another local-execution tool, exec_gate has no origin recorded for it and fails closed, so multi-step local work stalls after the first completion. Record the returned cycle id for the same counterpart/session before returning.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
scripts/ci/orch-ip-gate.sh (1)
38-41: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueModel-metadata check only covers
.tomlfiles.If agent-tier/model routing metadata were reintroduced in a different format (Rust source, JSON, YAML config), this check wouldn't catch it. Consider whether the metadata format is guaranteed to stay TOML-only, or broaden the scan.
🤖 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 `@scripts/ci/orch-ip-gate.sh` around lines 38 - 41, The model-metadata gate in orch-ip-gate.sh only scans orchestration TOML files, so reintroduced agent_tier or [model] routing metadata in other formats could slip through. Update the check around the grep in the metadata validation block to either explicitly document that TOML is the only supported source of this metadata or broaden the scan in the relevant orchestration area to include the additional config/source formats you want covered.src/openhuman/orchestration/mod.rs (1)
41-111: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoff
mod.rsshould be export-only; move hosted-client lifecycle logic into a sibling module.This block adds a static (
HOSTED_CLIENT_TASKS) plus business logic (abort_hosted_client_tasks,start_hosted_client_services,stop_hosted_client_services) directly inmod.rs. Per repo convention,mod.rsfiles must contain onlymod/pub mod,pub use, and controller-schema wiring. Consider relocating this into a domain sibling (e.g.hosted_client.rs) and re-exporting the two public entrypoints from here.As per coding guidelines: "
mod.rsfiles must be export-only:mod/pub mod,pub use, and controller schema wiring only; no business logic."🤖 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/mod.rs` around lines 41 - 111, The hosted-client lifecycle logic is incorrectly implemented inside mod.rs instead of being export-only. Move HOSTED_CLIENT_TASKS, abort_hosted_client_tasks, start_hosted_client_services, and stop_hosted_client_services into a sibling module such as hosted_client.rs, then re-export the public entrypoints from mod.rs. Keep mod.rs limited to module declarations, pub use, and controller-schema wiring, and preserve the existing behavior around task abort/restart and orchestration.enabled.Source: Coding guidelines
app/src/components/intelligence/pixiGraphRenderer.test.ts (1)
65-67: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueStub
removeChildren()diverges slightly from real PIXI API.Real
Container.removeChildren()acceptsbeginIndex/endIndexand returns the removed children array; this stub always clears everything and returnsundefined. Fine as long as the code under test doesn't consume the return value or a partial range.🤖 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 `@app/src/components/intelligence/pixiGraphRenderer.test.ts` around lines 65 - 67, The `removeChildren()` stub in `pixiGraphRenderer.test.ts` does not match the real PIXI `Container.removeChildren()` API, which can take `beginIndex`/`endIndex` and returns the removed children array. Update the stub on the test container mock to accept those parameters and return the removed items so any code using partial removal or the return value behaves consistently with PIXI.src/openhuman/orchestration/effect_executor.rs (1)
196-222: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winStructured
ok/is_error/tool_namefields left unset on the completion message.
okis computed but only embedded in free text; the message's own structured columns (used elsewhere for filtering/rendering) stay at their defaults.♻️ Proposed fix
super::store::insert_message( conn, &super::types::OrchestrationMessage { id: format!("tool-completion:{task_id}:{seq}"), agent_id: counterpart.to_string(), session_id: session_id.to_string(), chat_kind: super::types::ChatKind::Master, role: "system".to_string(), body: body.clone(), timestamp: now.clone(), seq, + tool_name: Some(agent_id.to_string()), + ok: Some(ok), + is_error: Some(!ok), ..Default::default() }, )?;🤖 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 196 - 222, The completion message built in `effect_executor.rs` computes `ok` but never writes it into the structured fields on `super::types::OrchestrationMessage`, so downstream filtering/rendering can’t use it. Update the `OrchestrationMessage` created in the `insert_message` path to set the appropriate completion metadata fields (`ok`, `is_error`, and `tool_name`) alongside the existing `body`, and keep the free-text body as a human-readable summary only. Use the existing `body` construction and the `tool-completion:{task_id}:{seq}` message creation block as the place to set these fields.
🤖 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.
Inline comments:
In `@src/openhuman/orchestration/effect_executor.rs`:
- Around line 578-610: The evict handler in handle_evict is treating a callId as
successfully completed as soon as it is seen, so retries after a real
execute_evict failure get re-acked as success. Update the dedupe flow so
is_duplicate_call/effect tracking reflects the actual outcome: only mark the
call as completed after execute_evict succeeds, or store and replay the prior
(ok, error) result for duplicates. Keep the duplicate protection in
effect_executor::handle_evict, but make sure failed attempts are not recorded as
successful; apply the same pattern to handle_send_dm if it uses the same dedupe
cache.
- Around line 161-199: The background sub-agent flow in
run_local_agent_and_forward can exit early on tool.execute() errors, which
prevents any completion from being forwarded to the originating session. Change
the error handling in run_local_agent_and_forward so invocation failures are
converted into a failure completion payload and still persisted/forwarded as a
tool_completion event, instead of using ? to return immediately; keep the
tokio::spawn caller’s logging, but ensure it never leaves the brain waiting
forever for a completion. If forwarding/persistence still fails after retries,
add a last-resort local failure record using the existing status/world_obs-style
path so the failure is visible on the next poll.
In `@src/openhuman/orchestration/migrate_history.rs`:
- Around line 74-88: The replay path in migrate_history::replay_session is not
preserving the backend cycle origin returned by push_event, so Master-session
turns can’t be recognized later by exec_gate::cycle_is_master. Update the replay
flow around super::cloud::push_event to capture the returned cycleId and record
it the same way the live master/ingest paths do for run_local_agent
authorization. Use the existing replay_session and push_event symbols to wire
the origin through the resumed event handling instead of discarding it.
In `@src/openhuman/orchestration/store.rs`:
- Around line 741-751: Wrap append_world_obs in in_immediate_txn so the
MAX(seq)+1 calculation and INSERT run atomically under a write lock; this
prevents concurrent writers from racing on world_obs and producing duplicate seq
values that later get deduped away. Update the append_world_obs function to
perform both the query_row and execute inside the immediate transaction path,
keeping the existing session_id, seq, note, ts insert logic unchanged.
---
Nitpick comments:
In `@app/src/components/intelligence/pixiGraphRenderer.test.ts`:
- Around line 65-67: The `removeChildren()` stub in `pixiGraphRenderer.test.ts`
does not match the real PIXI `Container.removeChildren()` API, which can take
`beginIndex`/`endIndex` and returns the removed children array. Update the stub
on the test container mock to accept those parameters and return the removed
items so any code using partial removal or the return value behaves consistently
with PIXI.
In `@scripts/ci/orch-ip-gate.sh`:
- Around line 38-41: The model-metadata gate in orch-ip-gate.sh only scans
orchestration TOML files, so reintroduced agent_tier or [model] routing metadata
in other formats could slip through. Update the check around the grep in the
metadata validation block to either explicitly document that TOML is the only
supported source of this metadata or broaden the scan in the relevant
orchestration area to include the additional config/source formats you want
covered.
In `@src/openhuman/orchestration/effect_executor.rs`:
- Around line 196-222: The completion message built in `effect_executor.rs`
computes `ok` but never writes it into the structured fields on
`super::types::OrchestrationMessage`, so downstream filtering/rendering can’t
use it. Update the `OrchestrationMessage` created in the `insert_message` path
to set the appropriate completion metadata fields (`ok`, `is_error`, and
`tool_name`) alongside the existing `body`, and keep the free-text body as a
human-readable summary only. Use the existing `body` construction and the
`tool-completion:{task_id}:{seq}` message creation block as the place to set
these fields.
In `@src/openhuman/orchestration/mod.rs`:
- Around line 41-111: The hosted-client lifecycle logic is incorrectly
implemented inside mod.rs instead of being export-only. Move
HOSTED_CLIENT_TASKS, abort_hosted_client_tasks, start_hosted_client_services,
and stop_hosted_client_services into a sibling module such as hosted_client.rs,
then re-export the public entrypoints from mod.rs. Keep mod.rs limited to module
declarations, pub use, and controller-schema wiring, and preserve the existing
behavior around task abort/restart and orchestration.enabled.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 31274cef-d5c5-48b5-b30e-a6d88898fa77
📒 Files selected for processing (89)
.env.example.github/workflows/ci-lite.ymlapp/src/components/intelligence/IntelligenceSubconsciousTab.tsxapp/src/components/intelligence/TinyPlaceOrchestrationTab.test.tsxapp/src/components/intelligence/TinyPlaceOrchestrationTab.tsxapp/src/components/intelligence/__tests__/IntelligenceSubconsciousTab.test.tsxapp/src/components/intelligence/pixiGraphRenderer.test.tsapp/src/components/orchestration/AgentChatPanel.tsxapp/src/components/orchestration/__tests__/AgentChatPanel.test.tsxapp/src/hooks/__tests__/useSubconscious.test.tsapp/src/lib/i18n/ar.tsapp/src/lib/i18n/bn.tsapp/src/lib/i18n/de.tsapp/src/lib/i18n/en.tsapp/src/lib/i18n/es.tsapp/src/lib/i18n/fr.tsapp/src/lib/i18n/hi.tsapp/src/lib/i18n/id.tsapp/src/lib/i18n/it.tsapp/src/lib/i18n/ko.tsapp/src/lib/i18n/pl.tsapp/src/lib/i18n/pt.tsapp/src/lib/i18n/ru.tsapp/src/lib/i18n/zh-CN.tsapp/src/lib/orchestration/orchestrationClient.tsapp/src/pages/__tests__/OrchestrationPage.test.tsxapp/src/utils/tauriCommands/__tests__/subconscious.test.tsapp/src/utils/tauriCommands/subconscious.tsdocs/TEST-COVERAGE-MATRIX.mdscripts/ci/orch-ip-gate.shsrc/core/event_bus/events.rssrc/core/jsonrpc.rssrc/openhuman/about_app/catalog_data.rssrc/openhuman/agent_registry/agents/loader.rssrc/openhuman/config/schema/load/env_overlay.rssrc/openhuman/config/schema/orchestration.rssrc/openhuman/credentials/ops.rssrc/openhuman/mcp_server/resources.rssrc/openhuman/orchestration/bus.rssrc/openhuman/orchestration/cloud.rssrc/openhuman/orchestration/effect_executor.rssrc/openhuman/orchestration/exec_gate.rssrc/openhuman/orchestration/frontend_agent/agent.tomlsrc/openhuman/orchestration/frontend_agent/graph.rssrc/openhuman/orchestration/frontend_agent/mod.rssrc/openhuman/orchestration/frontend_agent/prompt.mdsrc/openhuman/orchestration/frontend_agent/prompt.rssrc/openhuman/orchestration/graph/build.rssrc/openhuman/orchestration/graph/compress.rssrc/openhuman/orchestration/graph/mod.rssrc/openhuman/orchestration/graph/state.rssrc/openhuman/orchestration/graph/tests.rssrc/openhuman/orchestration/graph/world_diff.rssrc/openhuman/orchestration/ingest.rssrc/openhuman/orchestration/master_agent/agent.tomlsrc/openhuman/orchestration/master_agent/mod.rssrc/openhuman/orchestration/master_agent/prompt.mdsrc/openhuman/orchestration/master_agent/prompt.rssrc/openhuman/orchestration/master_reporter/agent.tomlsrc/openhuman/orchestration/master_reporter/mod.rssrc/openhuman/orchestration/master_reporter/prompt.mdsrc/openhuman/orchestration/master_reporter/prompt.rssrc/openhuman/orchestration/migrate_history.rssrc/openhuman/orchestration/mod.rssrc/openhuman/orchestration/ops.rssrc/openhuman/orchestration/reasoning_agent/agent.tomlsrc/openhuman/orchestration/reasoning_agent/graph.rssrc/openhuman/orchestration/reasoning_agent/mod.rssrc/openhuman/orchestration/reasoning_agent/prompt.mdsrc/openhuman/orchestration/reasoning_agent/prompt.rssrc/openhuman/orchestration/reasoning_agent/steering.rssrc/openhuman/orchestration/schemas.rssrc/openhuman/orchestration/steering.rssrc/openhuman/orchestration/store.rssrc/openhuman/orchestration/sync.rssrc/openhuman/orchestration/tools.rssrc/openhuman/orchestration/world_diff_uploader.rssrc/openhuman/orchestration/world_model.rssrc/openhuman/socket/event_handlers.rssrc/openhuman/subconscious/factory.rssrc/openhuman/subconscious/mod.rssrc/openhuman/subconscious/profiles/mod.rssrc/openhuman/subconscious/profiles/tinyplace.rssrc/openhuman/subconscious/profiles/tinyplace_tests.rssrc/openhuman/tinyagents/topology.rssrc/openhuman/tools/ops.rssrc/openhuman/voice/dictation_listener.rstests/orchestration_exec_gate.rstests/orchestration_hosted_client.rs
💤 Files with no reviewable changes (36)
- src/openhuman/orchestration/frontend_agent/prompt.md
- src/openhuman/orchestration/reasoning_agent/prompt.md
- src/openhuman/subconscious/profiles/mod.rs
- src/openhuman/orchestration/master_reporter/prompt.md
- src/openhuman/orchestration/master_reporter/agent.toml
- src/openhuman/orchestration/master_reporter/prompt.rs
- src/openhuman/orchestration/master_agent/prompt.md
- src/openhuman/orchestration/graph/world_diff.rs
- src/openhuman/orchestration/frontend_agent/prompt.rs
- src/openhuman/orchestration/steering.rs
- src/openhuman/orchestration/frontend_agent/mod.rs
- src/openhuman/orchestration/master_agent/agent.toml
- src/openhuman/orchestration/master_agent/prompt.rs
- src/openhuman/orchestration/reasoning_agent/mod.rs
- src/openhuman/orchestration/reasoning_agent/prompt.rs
- src/openhuman/orchestration/frontend_agent/agent.toml
- src/openhuman/subconscious/mod.rs
- src/openhuman/orchestration/reasoning_agent/graph.rs
- src/openhuman/tinyagents/topology.rs
- src/openhuman/orchestration/reasoning_agent/agent.toml
- src/openhuman/orchestration/reasoning_agent/steering.rs
- src/openhuman/orchestration/graph/tests.rs
- src/openhuman/mcp_server/resources.rs
- src/openhuman/orchestration/graph/state.rs
- src/openhuman/orchestration/frontend_agent/graph.rs
- .env.example
- src/openhuman/orchestration/graph/compress.rs
- app/src/components/intelligence/IntelligenceSubconsciousTab.tsx
- src/openhuman/orchestration/master_reporter/mod.rs
- src/openhuman/orchestration/graph/mod.rs
- src/openhuman/subconscious/profiles/tinyplace.rs
- src/openhuman/subconscious/profiles/tinyplace_tests.rs
- src/openhuman/config/schema/load/env_overlay.rs
- src/core/event_bus/events.rs
- src/openhuman/orchestration/graph/build.rs
- src/openhuman/orchestration/master_agent/mod.rs
sanil-23
force-pushed
the
feat/1074-r3-local-exec-device-tool
branch
2 times, most recently
from
d4f0813 to
7081117
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@src/openhuman/orchestration/cloud.rs`:
- Around line 85-105: Record the `cycleId` returned by `push_event` inside
`resume_pending` so replayed sessions keep their device-authoritative origin.
Update the `resume_pending` flow in `migrate_history` to capture the
`Option<String>` from `push_event` and persist/pass it through to whatever
session state `exec_gate` reads, instead of discarding it. Use the existing
`push_event` and `exec_gate` symbols to trace where the origin must be stored
and restored.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 56d80bdb-8721-4b76-9e6d-660c17b25073
📒 Files selected for processing (14)
src/openhuman/orchestration/cloud.rssrc/openhuman/orchestration/effect_executor.rssrc/openhuman/orchestration/exec_gate.rssrc/openhuman/orchestration/ingest.rssrc/openhuman/orchestration/migrate_history.rssrc/openhuman/orchestration/mod.rssrc/openhuman/orchestration/schemas.rssrc/openhuman/orchestration/store.rssrc/openhuman/orchestration/sync.rssrc/openhuman/socket/event_handlers.rssrc/openhuman/subconscious/factory_tests.rssrc/openhuman/voice/dictation_listener.rstests/orchestration_exec_gate.rstests/orchestration_hosted_client.rs
✅ Files skipped from review due to trivial changes (1)
- src/openhuman/voice/dictation_listener.rs
🚧 Files skipped from review as they are similar to previous changes (10)
- tests/orchestration_exec_gate.rs
- src/openhuman/orchestration/migrate_history.rs
- src/openhuman/socket/event_handlers.rs
- src/openhuman/orchestration/ingest.rs
- src/openhuman/orchestration/exec_gate.rs
- src/openhuman/orchestration/sync.rs
- src/openhuman/orchestration/mod.rs
- src/openhuman/orchestration/schemas.rs
- src/openhuman/orchestration/effect_executor.rs
- src/openhuman/orchestration/store.rs
sanil-23
force-pushed
the
feat/1074-r3-local-exec-device-tool
branch
from
7081117 to
a43e06c
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@src/openhuman/orchestration/mod.rs`:
- Around line 43-111: Move the hosted-client lifecycle state and logic out of
`mod.rs` into `ops.rs`: relocate `HOSTED_CLIENT_TASKS`,
`abort_hosted_client_tasks`, `start_hosted_client_services`, and
`stop_hosted_client_services` so `mod.rs` stays export-only and re-export the
public functions from there, following the existing
`start_message_drain_supervisor` pattern. While moving
`start_hosted_client_services`, avoid duplicating the drain-and-abort behavior
by reusing the same helper used by
`stop_hosted_client_services`/`abort_hosted_client_tasks`, or extract a shared
helper around the task mutex to handle replacement and cleanup consistently.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 840c4fdd-95e9-4bd0-9bf4-ea57f48a3c6e
📒 Files selected for processing (8)
src/openhuman/orchestration/cloud.rssrc/openhuman/orchestration/effect_executor.rssrc/openhuman/orchestration/exec_gate.rssrc/openhuman/orchestration/ingest.rssrc/openhuman/orchestration/mod.rssrc/openhuman/orchestration/schemas.rssrc/openhuman/socket/event_handlers.rstests/orchestration_exec_gate.rs
🚧 Files skipped from review as they are similar to previous changes (5)
- tests/orchestration_exec_gate.rs
- src/openhuman/socket/event_handlers.rs
- src/openhuman/orchestration/cloud.rs
- src/openhuman/orchestration/exec_gate.rs
- src/openhuman/orchestration/effect_executor.rs
Contributor
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@src/openhuman/orchestration/mod.rs`:
- Around line 43-111: Move the hosted-client lifecycle state and logic out of
`mod.rs` into `ops.rs`: relocate `HOSTED_CLIENT_TASKS`,
`abort_hosted_client_tasks`, `start_hosted_client_services`, and
`stop_hosted_client_services` so `mod.rs` stays export-only and re-export the
public functions from there, following the existing
`start_message_drain_supervisor` pattern. While moving
`start_hosted_client_services`, avoid duplicating the drain-and-abort behavior
by reusing the same helper used by
`stop_hosted_client_services`/`abort_hosted_client_tasks`, or extract a shared
helper around the task mutex to handle replacement and cleanup consistently.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 840c4fdd-95e9-4bd0-9bf4-ea57f48a3c6e
📒 Files selected for processing (8)
src/openhuman/orchestration/cloud.rssrc/openhuman/orchestration/effect_executor.rssrc/openhuman/orchestration/exec_gate.rssrc/openhuman/orchestration/ingest.rssrc/openhuman/orchestration/mod.rssrc/openhuman/orchestration/schemas.rssrc/openhuman/socket/event_handlers.rstests/orchestration_exec_gate.rs
🚧 Files skipped from review as they are similar to previous changes (5)
- tests/orchestration_exec_gate.rs
- src/openhuman/socket/event_handlers.rs
- src/openhuman/orchestration/cloud.rs
- src/openhuman/orchestration/exec_gate.rs
- src/openhuman/orchestration/effect_executor.rs
🛑 Comments failed to post (1)
src/openhuman/orchestration/mod.rs (1)
43-111: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Move hosted-client lifecycle logic out of
mod.rsintoops.rs.This adds a global
Mutexstate and three non-trivial functions (task spawn/abort lifecycle) directly inmod.rs. Per the repo'smod.rsconvention, this file should stay export-only (mod/pub use/schema wiring), and business logic belongs inops.rs— which this same file already re-exports from (pub use ops::start_message_drain_supervisor;), so there's existing precedent for where this logic should live.♻️ Proposed fix: relocate to `ops.rs`, re-export from `mod.rs`
-// ── Hosted-client background services (login-gated) ────────────────────────── - -use std::sync::Mutex; - -use tokio::task::JoinHandle; - -use crate::openhuman::config::Config; - -/// Join handles for the per-login hosted-client loops (read-sync + world-diff -/// uploader), so a logout→login aborts the old session's loops before starting -/// fresh ones — no duplicates, and never a loop bound to a previous session's -/// config/workspace. -static HOSTED_CLIENT_TASKS: Mutex<Vec<JoinHandle<()>>> = Mutex::new(Vec::new()); - -fn abort_hosted_client_tasks() { /* ... */ } - -pub async fn start_hosted_client_services(config: &Config) { /* ... */ } - -pub fn stop_hosted_client_services() { /* ... */ } +pub use ops::{start_hosted_client_services, stop_hosted_client_services};Then add the moved static + functions to
ops.rs(same bodies), where they sit alongsidestart_message_drain_supervisor.While relocating, consider having
start_hosted_client_servicescall the same drain-and-abort helper used bystop_hosted_client_services/abort_hosted_client_tasks(it currently reimplements the drain+abort loop inline at lines 78-83) to remove the duplication, e.g. by extracting afn replace_tasks(new: Vec<JoinHandle<()>>)helper that both call under one lock.🤖 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/mod.rs` around lines 43 - 111, Move the hosted-client lifecycle state and logic out of `mod.rs` into `ops.rs`: relocate `HOSTED_CLIENT_TASKS`, `abort_hosted_client_tasks`, `start_hosted_client_services`, and `stop_hosted_client_services` so `mod.rs` stays export-only and re-export the public functions from there, following the existing `start_message_drain_supervisor` pattern. While moving `start_hosted_client_services`, avoid duplicating the drain-and-abort behavior by reusing the same helper used by `stop_hosted_client_services`/`abort_hosted_client_tasks`, or extract a shared helper around the task mutex to handle replacement and cleanup consistently.Source: Coding guidelines
3 tasks
sanil-23
force-pushed
the
feat/1074-r3-local-exec-device-tool
branch
from
a43e06c to
5a2ecf2
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/openhuman/config/schemas/mod.rs (1)
32-35: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove duplicate
#[cfg(test)]attribute.Lines 32 and 33 both apply
#[cfg(test)]to the sameusestatement. Harmless but redundant.🧹 Proposed fix
-#[cfg(test)] #[cfg(test)] pub(crate) use schema_defs::schemas;🤖 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/config/schemas/mod.rs` around lines 32 - 35, Remove the duplicated conditional attribute on the re-export in mod.rs: the `pub(crate) use schema_defs::schemas;` statement is preceded by two identical `#[cfg(test)]` attributes. Keep only one `#[cfg(test)]` on that item and leave the `schemas` re-export unchanged.
🤖 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.
Inline comments:
In `@src/openhuman/orchestration/effect_executor.rs`:
- Line 242: The forwarded completion path in effect_executor::run_local_agent is
discarding the backend cycle origin returned by super::cloud::push_event, which
later breaks cycle_is_master checks for the new cycle. Update this forwarding
branch to capture the returned cycleId/origin the same way the other forward
sites do, and persist it alongside the envelope so subsequent run_local_agent
calls recognize the origin correctly.
---
Nitpick comments:
In `@src/openhuman/config/schemas/mod.rs`:
- Around line 32-35: Remove the duplicated conditional attribute on the
re-export in mod.rs: the `pub(crate) use schema_defs::schemas;` statement is
preceded by two identical `#[cfg(test)]` attributes. Keep only one
`#[cfg(test)]` on that item and leave the `schemas` re-export unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b261247d-dae0-4e16-96e6-ed9c7e2d498f
📒 Files selected for processing (19)
src/openhuman/channels/providers/web/mod.rssrc/openhuman/config/schema/load/mod.rssrc/openhuman/config/schemas/mod.rssrc/openhuman/inference/local/service/ollama_admin/mod.rssrc/openhuman/inference/provider/reliable.rssrc/openhuman/mcp_server/tools/mod.rssrc/openhuman/memory/chat.rssrc/openhuman/memory/schema/mod.rssrc/openhuman/memory/tree_source/file.rssrc/openhuman/orchestration/cloud.rssrc/openhuman/orchestration/effect_executor.rssrc/openhuman/orchestration/exec_gate.rssrc/openhuman/orchestration/ingest.rssrc/openhuman/orchestration/migrate_history.rssrc/openhuman/orchestration/mod.rssrc/openhuman/orchestration/schemas.rssrc/openhuman/sandbox/docker.rssrc/openhuman/socket/event_handlers.rstests/orchestration_exec_gate.rs
✅ Files skipped from review due to trivial changes (5)
- src/openhuman/memory/schema/mod.rs
- src/openhuman/memory/tree_source/file.rs
- src/openhuman/sandbox/docker.rs
- src/openhuman/inference/provider/reliable.rs
- src/openhuman/memory/chat.rs
🚧 Files skipped from review as they are similar to previous changes (6)
- tests/orchestration_exec_gate.rs
- src/openhuman/orchestration/schemas.rs
- src/openhuman/orchestration/exec_gate.rs
- src/openhuman/orchestration/ingest.rs
- src/openhuman/orchestration/mod.rs
- src/openhuman/orchestration/cloud.rs
…letion forward Give the hosted brain back local execution (retired in the hosted-only migration) as a gated device tool, restricted to Master-chat cycles. - `run_local_agent` device tool: spawns a local sub-agent (code_executor, researcher, tools_agent, …) on the device and forwards its result. - `exec_gate`: device-authoritative trust gate. Records cycleId -> (counterpart, session) at forward time; authorizes local execution only when the cycle's counterpart is LOCAL_MASTER_AGENT. Unknown/A2A cycles fail closed. Enforced in effect_executor (the capability holder), so an injected/compromised cloud brain cannot induce local execution for an agent-to-agent cycle. - Async completion model: run_local_agent returns an immediate `accepted` ack (inside the device-tool timeout, so the cycle never blocks) and runs the sub-agent in the background; on completion the result is forwarded up as a `tool_completion` event, which wakes a fresh cycle that reasons over it. - cloud.push_event now returns the backend cycleId (origin capture). - Integration tests for the gate (master allowed, A2A/unknown denied). No backend change: executeLoop already surfaces device-declared tools to the model, the /v1/events route already wakes a cycle on any event, and the event `kind` field is unconstrained. Stacked on tinyhumansai#4738. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
sanil-23
force-pushed
the
feat/1074-r3-local-exec-device-tool
branch
from
5a2ecf2 to
47cc95a
Compare
senamakel
added a commit
to senamakel/openhuman
that referenced
this pull request
Jul 10, 2026
…ck CI) tests/orchestration_effect_executor_e2e.rs (added by tinyhumansai#4738) still called dispatch_device_tool/handle_tool_call with the old sync 2-arg signatures after tinyhumansai#4753 made them async/3-arg — broken identically on upstream/main. It only surfaced in this PR because the Motion A test-debt fix touched agent_orchestration files, pulling the orchestration domain into CI rust-core-coverage's scope. Convert the two affected tests to #[tokio::test] + .await, and pass the cycle_id arg (the exec gate is bypassed for non-local-exec tools like device_status). Claude-Session: https://claude.ai/code/session_01U8NDGbt9tKj443VquzycLB
7 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Give the hosted orchestration brain back local execution (retired in the hosted-only migration, #4738) as a gated device tool, restricted to Master-chat cycles, with an async completion-forward so the result re-enters the brain via a fresh wake cycle.
Problem
#4738 moved the brain to the cloud and deleted the client's local
code_executor/spawn_async_subagentfrom the orchestration path — so the hosted brain lost the ability to run code / touch files on the user's machine. We want that back, but only when the human is driving their own agent (Master chat), never when an external agent's DM (A2A) triggered the cycle — otherwise a prompt-injected reasoning turn could induce local code execution / file exfiltration (confused-deputy).Solution (scope: core)
run_local_agentdevice tool (orchestration/effect_executor.rs) — declared in the device manifest; spawns a local sub-agent (code_executor,researcher,tools_agent, …) viaSpawnSubagentTooland forwards its result.orchestration/exec_gate.rs) — recordscycleId -> (counterpart, session)at forward time (from what the device itself forwarded, not what the backend asserts) and authorizes local-execution tools only when the cycle's counterpart isLOCAL_MASTER_AGENT. Unknown / A2A cycles fail closed. Enforced ineffect_executor— on the device that holds the capability — so an injected or compromised cloud brain cannot induce local execution for an A2A cycle.run_local_agentreturns an immediateacceptedack (well inside the device-tool timeout, so the wake cycle never blocks) and runs the sub-agent in the background; on completion the result is forwarded up as atool_completionevent, which wakes a fresh cycle that reasons over it.cloud::push_eventnow returns the backendcycleId(from the202), recorded at both forward sites (ingest.rs,schemas.rs).No backend change:
executeLoopalready surfaces device-declared tools to the model, the/v1/eventsroute already firesrunWakeForCycleon any non-duplicate event, and the eventkindfield is unconstrained — so thetool_completionforward rides the existing ingest→wake path.Submission Checklist
tests/orchestration_exec_gate.rs(master authorized, A2A denied, unknown fail-closed,cycle_targetrouting) + inlineexec_gateunit tests.N/A: theRust Core Coveragegate can't run —cargo test --libis broken repo-wide onmain(the refactor(orchestration): hosted-only brain — retire the client local orchestration engine (Phase 4) #4738 merge commite5c6507cis also red on this check; ~30 unrelated modules fail to compile in thecfg(test)build, the known breakageorchestration/cloud.rsdocuments). The R3 gate logic is covered via the integration test (which links the compiled lib, bypassingcfg(test)); therun_local_agentspawn/forward path needs a runtime (e2e follow-up).N/A:Coverage Matrix Syncpasses; device-tool wiring has no matrix feature id.## Related.N/A: behind the hosted path, no release-cut UI surface.## Related.Impact
code_executor) on the user's machine when the human asks; the result arrives asynchronously as a follow-up cycle, so long tasks never time out the original cycle.Known follow-ups (not in this PR)
toolCallIdcorrelation for the completion (today it re-enters as flattenedrawInputtext — works with the current backend prompt).agent_idallowlist insiderun_local_agent(today any registered sub-agent is spawnable on a Master cycle).Related
Summary by CodeRabbit
New Features
Bug Fixes