feat(agent): pure orchestrator pattern with per-skill delegation tools#496
Merged
senamakel merged 4 commits intotinyhumansai:mainfrom Apr 10, 2026
Merged
Conversation
tinyhumansai#478) Refactors the main agent from a direct tool-calling model to a pure orchestrator that delegates all work through dynamically generated tools. Architecture changes: - Orchestrator only sees generated tools (notion, gmail, research, run_code, review_code, plan, spawn_subagent) — skill tools are architecturally unreachable from the main agent - Each installed skill auto-generates a delegation tool at build time (SkillDelegationTool) that routes to skills_agent with the correct skill_filter - Static archetype tools (research, run_code, etc.) delegate to their respective sub-agents - visible_tool_specs filters the function-calling schema sent to the provider, enforcing the orchestrator boundary at the API level Prompt changes: - Rewrote AGENTS.md as a lean orchestrator prompt — no more routing tables or agent_id instructions - Orchestrator skips TOOLS.md, MEMORY.md, HEARTBEAT.md (~6k tokens saved per turn) — subagents get tool specs from the registry - Workspace .md files auto-sync via builtin-hash mechanism so prompt updates ship automatically to existing installs Bug fixes: - ModelSpec::Hint now resolves to {hint}-v1 (e.g. agentic-v1) instead of hint:agentic which the backend rejected - validate_skill_filter now uses skill_id from the engine tuple instead of splitting on __ in the raw tool name (which always failed) - Memory context forwarded to subagents via ParentExecutionContext Observability: - Added [agent] tagged logs for tool responses, agent state transitions, and delegation decisions throughout turn.rs See docs/agent-prompt-architecture.excalidraw for the visual diagram. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
📝 WalkthroughWalkthroughIntroduces an orchestrator agent architecture: an allowlist to restrict tools visible to the main agent, new orchestrator tools (skill/archetype delegators + spawn_subagent), forwarding per-turn memory context into subagents, prompt rendering changes for orchestrator mode, workspace builtin-file sync, and model-hint resolution updates. Changes
Sequence DiagramsequenceDiagram
actor User
participant Orchestrator as Orchestrator Agent
participant LLM as LLM / Provider
participant ToolRegistry as Tool Registry
participant DelegatingTool as Delegating Tool
participant SubAgent as Sub-Agent
User->>Orchestrator: user message + context
Orchestrator->>ToolRegistry: collect_orchestrator_tools()
ToolRegistry-->>Orchestrator: orchestrator-visible tools
Orchestrator->>LLM: call with orchestrator system prompt (omit TOOLS/HEARTBEAT/MEMORY) + visible tools
LLM-->>Orchestrator: returns tool call (delegating tool)
Orchestrator->>DelegatingTool: execute(tool args)
DelegatingTool->>Orchestrator: dispatch_subagent(event + options)
DelegatingTool->>SubAgent: spawn with ParentExecutionContext (includes memory_context + filtered tools)
SubAgent->>LLM: run subagent (archetype prompt + forwarded context)
SubAgent-->>DelegatingTool: subagent result
DelegatingTool-->>Orchestrator: tool result
Orchestrator->>User: aggregated response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
![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: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/openhuman/agent/agent/turn.rs (1)
623-626:⚠️ Potential issue | 🔴 CriticalFork mode should replay
visible_tool_specs, not the full registry.Line 625 captures
self.tool_specs, but the parent turn now sendsself.visible_tool_specs. In orchestrator mode, a forked child will regain hidden tools and stop being an exact replay of the parent turn.🪞 Suggested fix
harness::ForkContext { system_prompt: Arc::new(system_prompt), - tool_specs: Arc::clone(&self.tool_specs), + tool_specs: Arc::clone(&self.visible_tool_specs), message_prefix: Arc::new(messages), cache_boundary: None, fork_task_prompt, }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/openhuman/agent/agent/turn.rs` around lines 623 - 626, The ForkContext construction is capturing the full tool registry via self.tool_specs instead of the parent-turn visible subset; replace the reference to self.tool_specs with the parent-provided visible set (self.visible_tool_specs) so the fork replays exactly the parent's visible tools — e.g., in the harness::ForkContext initializer use Arc::clone(&self.visible_tool_specs) (or Arc::new(self.visible_tool_specs) as appropriate) instead of Arc::clone(&self.tool_specs) while leaving system_prompt and message_prefix unchanged.
🧹 Nitpick comments (2)
src/openhuman/agent/harness/subagent_runner.rs (1)
245-256: Skip blank context parts before rendering the[Context]block.Right now, whitespace-only values can still trigger a noisy context wrapper. Filter empties before join.
♻️ Suggested refinement
- let mut context_parts: Vec<&str> = Vec::new(); - if let Some(ref mem_ctx) = parent.memory_context { - context_parts.push(mem_ctx); - } - if let Some(ref ctx) = options.context { - context_parts.push(ctx); - } + let mut context_parts: Vec<&str> = Vec::new(); + if let Some(ref mem_ctx) = parent.memory_context { + if !mem_ctx.trim().is_empty() { + context_parts.push(mem_ctx); + } + } + if let Some(ref ctx) = options.context { + if !ctx.trim().is_empty() { + context_parts.push(ctx); + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/openhuman/agent/harness/subagent_runner.rs` around lines 245 - 256, The current construction of context_parts (used to build user_message) can include whitespace-only strings from parent.memory_context or options.context which still trigger the [Context] block; update the logic that populates context_parts (the block referencing parent.memory_context and options.context) to trim and skip any entries where .trim().is_empty() so only non-empty context pieces are pushed, then leave the rest of the user_message construction (including task_prompt) unchanged.src/openhuman/tools/ops.rs (1)
259-263: Add a focused unit test formain_agent_toolsfiltering.Given this is a compatibility path, a tiny regression test would prevent silent visibility drift.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/openhuman/tools/ops.rs` around lines 259 - 263, Add a focused unit test for the main_agent_tools function to guard the filtering behavior: create test helpers that implement the Tool trait (e.g., dummy tools with distinct name() values), call main_agent_tools with a Vec of allowed and disallowed dummy tools, and assert the returned Vec only contains tools whose name() is listed in MAIN_AGENT_TOOL_ALLOWLIST and preserves expected count/order. Use the same module path as ops.rs tests and include edge cases like empty input and all-disallowed input to catch regressions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/openhuman/agent/agent/builder.rs`:
- Around line 444-455: The orchestrator-generated tools (from
tools::orchestrator_tools::collect_orchestrator_tools()) can duplicate entries
already registered by all_tools_with_runtime() (notably "spawn_subagent"), so
before calling tools.extend(orchestrator_tools) filter out any orchestrator tool
whose name already exists in the current registry; obtain the existing names
from the current tools collection (or build a HashSet of tool.name() for tools)
and only extend with orchestrator tools whose name is not in that set, keeping
the visible HashSet consistent and preventing duplicate "spawn_subagent" specs
that confuse build() and dispatch.
In `@src/openhuman/agent/agent/turn.rs`:
- Around line 221-223: The code currently only hides tools from the outbound
schema (using self.tool_dispatcher.should_send_tool_specs() and
self.visible_tool_specs) but still resolves/executes against the full self.tools
set later, allowing hidden tools to be invoked; update the dispatch/resolution
path (the code that resolves parser-driven calls around the dispatch/resolve
block that currently references self.tools, lines ~474-491) to enforce the
visible-tool allowlist by filtering self.tools to only entries whose names
appear in self.visible_tool_specs (or by checking membership in that allowlist
before any resolution/execution) so that hidden tools cannot be dispatched even
if requested.
- Around line 373-381: The loop that logs tool results currently includes
raw/sliced output (using truncate_with_ellipsis(&r.output, 300)) which can leak
secrets; replace that with metadata-only logging and a sanitized summary
instead: remove direct use of r.output in the log and call a
sanitizer/summarizer (e.g., create or use a function like sanitize_tool_output
or summarize_tool_output) to produce a redacted one-line summary, then log
r.name, r.success, r.output.chars().count() or a content hash, and the sanitized
summary; apply the same change for the other occurrence referenced around lines
520-530 so no raw tool payload is written to logs.
In `@src/openhuman/agent/harness/definition.rs`:
- Around line 198-207: The ModelSpec::resolve implementation currently maps
ModelSpec::Hint to "{hint}-v1", which breaks the router/consumer hint contract;
change the Hint arm in the resolve method (enum ModelSpec, function resolve) to
return the consumer-compatible form "hint:{hint}" (e.g., format!("hint:{}",
hint)) so hint-based route lookups and hint stripping continue to work and do
not bypass model_routes; update the corresponding duplicate at the other
occurrence (around the other resolve usage) to the same "hint:{hint}" behavior.
In `@src/openhuman/agent/harness/executor.rs`:
- Line 567: The code returns a hardcoded string using
archetype.default_model_hint() (format!("{}-v1")) which bypasses the hint-based
router resolution; update the default branch in resolve_model to call the
existing hint-resolution path instead of constructing "{hint}-v1" directly—use
the same router/hint resolution logic the function uses for explicit hints
(refer to resolve_model and archetype.default_model_hint()) so deployments keyed
on hint:* go through the route-table lookup rather than returning a fabricated
model name.
In `@src/openhuman/agent/prompt.rs`:
- Around line 199-227: The orchestrator branch currently omits TOOLS.md,
HEARTBEAT.md, and MEMORY.md from files passed to inject_workspace_file which
also performs sync via sync_workspace_file, so those on-disk files stop being
updated; modify the logic in the block around is_orchestrator (and where
inject_workspace_file and sync_workspace_file are used) to decouple syncing from
prompt injection — keep the shorter files list for prompt rendering when
is_orchestrator is true but still call sync_workspace_file (or a small helper
like sync_workspace_files) for "TOOLS.md", "HEARTBEAT.md", and "MEMORY.md"
(using ctx.workspace_dir and the same parameters as inject_workspace_file) so
the files remain created/updated even when they are not injected into the
prompt.
In `@src/openhuman/tools/ops.rs`:
- Around line 251-253: The const declaration MAIN_AGENT_TOOL_ALLOWLIST is
failing CI formatting; run the Rust formatter (cargo fmt or rustfmt) on the file
or project and reformat the declaration for MAIN_AGENT_TOOL_ALLOWLIST so it
matches the project's rustfmt style (proper spacing/line breaks and trailing
commas as required), then commit the formatted change.
---
Outside diff comments:
In `@src/openhuman/agent/agent/turn.rs`:
- Around line 623-626: The ForkContext construction is capturing the full tool
registry via self.tool_specs instead of the parent-turn visible subset; replace
the reference to self.tool_specs with the parent-provided visible set
(self.visible_tool_specs) so the fork replays exactly the parent's visible tools
— e.g., in the harness::ForkContext initializer use
Arc::clone(&self.visible_tool_specs) (or Arc::new(self.visible_tool_specs) as
appropriate) instead of Arc::clone(&self.tool_specs) while leaving system_prompt
and message_prefix unchanged.
---
Nitpick comments:
In `@src/openhuman/agent/harness/subagent_runner.rs`:
- Around line 245-256: The current construction of context_parts (used to build
user_message) can include whitespace-only strings from parent.memory_context or
options.context which still trigger the [Context] block; update the logic that
populates context_parts (the block referencing parent.memory_context and
options.context) to trim and skip any entries where .trim().is_empty() so only
non-empty context pieces are pushed, then leave the rest of the user_message
construction (including task_prompt) unchanged.
In `@src/openhuman/tools/ops.rs`:
- Around line 259-263: Add a focused unit test for the main_agent_tools function
to guard the filtering behavior: create test helpers that implement the Tool
trait (e.g., dummy tools with distinct name() values), call main_agent_tools
with a Vec of allowed and disallowed dummy tools, and assert the returned Vec
only contains tools whose name() is listed in MAIN_AGENT_TOOL_ALLOWLIST and
preserves expected count/order. Use the same module path as ops.rs tests and
include edge cases like empty input and all-disallowed input to catch
regressions.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: b7c6a96c-b54e-4344-a682-50288965229a
📒 Files selected for processing (15)
docs/agent-prompt-architecture.excalidrawsrc/openhuman/agent/agent/builder.rssrc/openhuman/agent/agent/turn.rssrc/openhuman/agent/agent/types.rssrc/openhuman/agent/harness/archetypes.rssrc/openhuman/agent/harness/definition.rssrc/openhuman/agent/harness/executor.rssrc/openhuman/agent/harness/fork_context.rssrc/openhuman/agent/harness/subagent_runner.rssrc/openhuman/agent/prompt.rssrc/openhuman/agent/prompts/AGENTS.mdsrc/openhuman/tools/mod.rssrc/openhuman/tools/ops.rssrc/openhuman/tools/orchestrator_tools.rssrc/openhuman/tools/spawn_subagent.rs
Comment on lines
+198
to
+207
| /// Hints are resolved to `{hint}-v1` (e.g. `"agentic"` → `"agentic-v1"`) | ||
| /// which matches the backend's standard model naming convention. When | ||
| /// a `RouterProvider` is present its route table takes priority over | ||
| /// this default; when no router is configured (empty `model_routes`) | ||
| /// the resolved name goes directly to the backend. | ||
| pub fn resolve(&self, parent_model: &str) -> String { | ||
| match self { | ||
| Self::Inherit => parent_model.to_string(), | ||
| Self::Exact(name) => name.clone(), | ||
| Self::Hint(hint) => format!("hint:{hint}"), | ||
| Self::Hint(hint) => format!("{hint}-v1"), |
Contributor
There was a problem hiding this comment.
ModelSpec::Hint no longer matches router/consumer hint contract.
At Line 207, resolving hints as "{hint}-v1" breaks consumers that still expect hint:{hint} (e.g., route lookup and hint stripping paths). This makes the doc at Line 200-202 inaccurate and can bypass configured model routes.
🔧 Proposed compatibility fix
- Self::Hint(hint) => format!("{hint}-v1"),
+ Self::Hint(hint) => format!("hint:{hint}"),Also applies to: 470-472
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/openhuman/agent/harness/definition.rs` around lines 198 - 207, The
ModelSpec::resolve implementation currently maps ModelSpec::Hint to "{hint}-v1",
which breaks the router/consumer hint contract; change the Hint arm in the
resolve method (enum ModelSpec, function resolve) to return the
consumer-compatible form "hint:{hint}" (e.g., format!("hint:{}", hint)) so
hint-based route lookups and hint stripping continue to work and do not bypass
model_routes; update the corresponding duplicate at the other occurrence (around
the other resolve usage) to the same "hint:{hint}" behavior.
Contributor
Author
There was a problem hiding this comment.
Intentional — this is a bug fix, not a regression. The hint:agentic format was being sent directly to the backend when model_routes is empty (most deployments), and the backend rejects it with Model 'hint:agentic' is not available. The RouterProvider::resolve() that strips the hint: prefix is only active when model_routes is populated. Since the backend model registry uses agentic-v1, reasoning-v1, coding-v1 naming, resolving directly to {hint}-v1 works in both cases (with and without router).
Contributor
There was a problem hiding this comment.
@sanil-23, thanks for the detailed explanation — that makes complete sense. My concern was unfounded. The old hint:{hint} format was only safe when RouterProvider with a populated model_routes was in the path; without it, the raw string hit the backend and was rejected. Resolving directly to {hint}-v1 is correct for both the routed and non-routed cases, and the updated doc comment accurately captures that behaviour. Apologies for the noise.
✏️ Learnings added
Learnt from: sanil-23
Repo: tinyhumansai/openhuman PR: 496
File: src/openhuman/agent/harness/definition.rs:198-207
Timestamp: 2026-04-10T18:46:46.175Z
Learning: In `src/openhuman/agent/harness/definition.rs`, `ModelSpec::Hint(hint)` intentionally resolves to `"{hint}-v1"` (e.g., `"agentic-v1"`, `"reasoning-v1"`, `"coding-v1"`). The old `"hint:{hint}"` format was a bug: it was sent verbatim to the backend when `model_routes` is empty (the common deployment), causing backend rejections (`Model 'hint:...' is not available`). The `hint:` prefix stripping in `RouterProvider::resolve()` is only active when `model_routes` is populated. Do not flag the `{hint}-v1` resolution as a router contract violation.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: CR
Repo: tinyhumansai/openhuman PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-09T22:52:14.248Z
Learning: New or changed behavior must ship with matching documentation. Add concise rustdoc / code comments where the flow is not obvious, and update `AGENTS.md`, architecture docs, or feature docs when repository rules or user-visible behavior change.
Learnt from: graycyrus
Repo: tinyhumansai/openhuman PR: 283
File: app/test/e2e/specs/skill-execution-flow.spec.ts:67-114
Timestamp: 2026-04-03T06:28:58.632Z
Learning: In `app/test/e2e/specs/skill-execution-flow.spec.ts`, the model→agent→tool→conversation E2E path (model-triggered tool_calls) is intentionally deferred to a future PR. It is tracked via an `it.skip` placeholder at the end of the spec. The blocker is that the mock backend does not yet support returning structured `tool_calls` in chat completion responses. The current spec only covers the deterministic skill runtime RPC surface: `skills_start` → `skills_list_tools` → `skills_call_tool` → `skills_stop`.
Learnt from: sanil-23
Repo: tinyhumansai/openhuman PR: 437
File: src/openhuman/subconscious/executor.rs:33-50
Timestamp: 2026-04-08T16:46:58.073Z
Learning: In `src/openhuman/subconscious/executor.rs`, `needs_tools(title: &str)` is a UX routing convenience only — it selects the execution path (local model vs. agentic-v1 full vs. analysis-only) based on task title heuristics. Write-action prevention is enforced at a deeper layer: (1) the analysis-only prompt sent to agentic-v1 explicitly instructs it not to execute write actions for read-only tasks, and (2) tool execution is controlled by the agent runtime. Do not flag `needs_tools()` as an insufficient write-authorization boundary.
Learnt from: sanil-23
Repo: tinyhumansai/openhuman PR: 416
File: src/openhuman/memory/relex.rs:441-464
Timestamp: 2026-04-07T15:49:51.275Z
Learning: When using the `ort` Rust crate v2.x with the `load-dynamic` feature enabled, don’t require individual execution-provider feature flags (e.g., `directml`, `coreml`, `cuda`) alongside `load-dynamic` to get EP registration code. The `ort` crate already compiles EP registration via `#[cfg(any(feature = "load-dynamic", feature = "<ep_name>"))]` guards, and adding per-EP feature flags can pull in static-linking dependencies that conflict with the dynamic loading approach. At runtime, EP availability is determined by what the dynamically loaded ONNX Runtime library (`onnxruntime.dll`/`.so`/`.dylib`) supports; ort docs indicate providers like `directml`/`xnnpack`/`coreml` are available in builds when the platform supports them.
Learnt from: sanil-23
Repo: tinyhumansai/openhuman PR: 416
File: src/openhuman/memory/relex.rs:441-464
Timestamp: 2026-04-07T15:49:51.275Z
Learning: When integrating the `ort` Rust crate v2.x with the `load-dynamic` feature enabled, do NOT also require/enable individual provider EP Cargo features like `directml`, `coreml`, or `cuda`. In `ort` v2.x, EP registration for providers (e.g., DirectML, CoreML, CUDA, etc.) is already compiled in under source-level `#[cfg(any(feature = "load-dynamic", feature = "<provider>"))]` guards, such as in `ep/directml.rs`. Adding provider feature flags alongside `load-dynamic` can pull in static-linking dependencies that conflict with the dynamic-loading approach. Provider availability should be treated as runtime-determined by what the loaded `onnxruntime` library (`onnxruntime.dll`/`libonnxruntime.so`/`libonnxruntime.dylib`) actually supports.
| } | ||
| } | ||
| format!("hint:{}", archetype.default_model_hint()) | ||
| format!("{}-v1", archetype.default_model_hint()) |
Contributor
There was a problem hiding this comment.
resolve_model default path bypasses hint-based router resolution.
At Line 567, returning "{hint}-v1" directly can skip route-table hint resolution in deployments keyed on hint:*, and the updated tests lock in that incompatible behavior.
🔧 Proposed fix
- format!("{}-v1", archetype.default_model_hint())
+ format!("hint:{}", archetype.default_model_hint())Also applies to: 623-632
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/openhuman/agent/harness/executor.rs` at line 567, The code returns a
hardcoded string using archetype.default_model_hint() (format!("{}-v1")) which
bypasses the hint-based router resolution; update the default branch in
resolve_model to call the existing hint-resolution path instead of constructing
"{hint}-v1" directly—use the same router/hint resolution logic the function uses
for explicit hints (refer to resolve_model and archetype.default_model_hint())
so deployments keyed on hint:* go through the route-table lookup rather than
returning a fabricated model name.
Contributor
Author
There was a problem hiding this comment.
Same rationale as the ModelSpec::Hint comment — the executor resolve_model path mirrors the same fix. When model_routes is empty (no RouterProvider), the hint: prefix goes directly to the backend which rejects it. {hint}-v1 works in both configurations.
Contributor
There was a problem hiding this comment.
@sanil-23, thanks for the clarification — that makes sense. When no RouterProvider is in place the hint: prefix is passed verbatim to the backend and gets rejected, whereas {hint}-v1 is a valid model identifier in both configurations. My concern was misplaced; the fix is correct as implemented.
✏️ Learnings added
Learnt from: sanil-23
Repo: tinyhumansai/openhuman PR: 496
File: src/openhuman/agent/harness/executor.rs:567-567
Timestamp: 2026-04-10T18:46:53.247Z
Learning: In `src/openhuman/agent/harness/executor.rs` (and `src/openhuman/agent/harness/definition.rs`), the correct format for hint-based model resolution is `"{hint}-v1"` (e.g., `"coding-v1"`, `"reasoning-v1"`), NOT `"hint:{hint}"`. When no RouterProvider is configured, `hint:` is forwarded verbatim to the backend and rejected. `{hint}-v1` works in both the RouterProvider and direct-backend configurations. Do not flag `{hint}-v1` as bypassing hint resolution.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: sanil-23
Repo: tinyhumansai/openhuman PR: 437
File: src/openhuman/subconscious/executor.rs:33-50
Timestamp: 2026-04-08T16:46:58.073Z
Learning: In `src/openhuman/subconscious/executor.rs`, `needs_tools(title: &str)` is a UX routing convenience only — it selects the execution path (local model vs. agentic-v1 full vs. analysis-only) based on task title heuristics. Write-action prevention is enforced at a deeper layer: (1) the analysis-only prompt sent to agentic-v1 explicitly instructs it not to execute write actions for read-only tasks, and (2) tool execution is controlled by the agent runtime. Do not flag `needs_tools()` as an insufficient write-authorization boundary.
Learnt from: graycyrus
Repo: tinyhumansai/openhuman PR: 283
File: app/test/e2e/specs/skill-execution-flow.spec.ts:67-114
Timestamp: 2026-04-03T06:28:58.632Z
Learning: In `app/test/e2e/specs/skill-execution-flow.spec.ts`, the model→agent→tool→conversation E2E path (model-triggered tool_calls) is intentionally deferred to a future PR. It is tracked via an `it.skip` placeholder at the end of the spec. The blocker is that the mock backend does not yet support returning structured `tool_calls` in chat completion responses. The current spec only covers the deterministic skill runtime RPC surface: `skills_start` → `skills_list_tools` → `skills_call_tool` → `skills_stop`.
Learnt from: sanil-23
Repo: tinyhumansai/openhuman PR: 416
File: src/openhuman/memory/relex.rs:441-464
Timestamp: 2026-04-07T15:49:51.275Z
Learning: When using the `ort` Rust crate v2.x with the `load-dynamic` feature enabled, don’t require individual execution-provider feature flags (e.g., `directml`, `coreml`, `cuda`) alongside `load-dynamic` to get EP registration code. The `ort` crate already compiles EP registration via `#[cfg(any(feature = "load-dynamic", feature = "<ep_name>"))]` guards, and adding per-EP feature flags can pull in static-linking dependencies that conflict with the dynamic loading approach. At runtime, EP availability is determined by what the dynamically loaded ONNX Runtime library (`onnxruntime.dll`/`.so`/`.dylib`) supports; ort docs indicate providers like `directml`/`xnnpack`/`coreml` are available in builds when the platform supports them.
Learnt from: sanil-23
Repo: tinyhumansai/openhuman PR: 416
File: src/openhuman/memory/relex.rs:441-464
Timestamp: 2026-04-07T15:49:51.275Z
Learning: When integrating the `ort` Rust crate v2.x with the `load-dynamic` feature enabled, do NOT also require/enable individual provider EP Cargo features like `directml`, `coreml`, or `cuda`. In `ort` v2.x, EP registration for providers (e.g., DirectML, CoreML, CUDA, etc.) is already compiled in under source-level `#[cfg(any(feature = "load-dynamic", feature = "<provider>"))]` guards, such as in `ep/directml.rs`. Adding provider feature flags alongside `load-dynamic` can pull in static-linking dependencies that conflict with the dynamic-loading approach. Provider availability should be treated as runtime-determined by what the loaded `onnxruntime` library (`onnxruntime.dll`/`libonnxruntime.so`/`libonnxruntime.dylib`) actually supports.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
src/openhuman/agent/agent/turn.rs (2)
371-379:⚠️ Potential issue | 🟠 MajorStop logging raw tool output previews.
Line 372 and Line 525 log direct tool payload content (truncated, but still sensitive). This can leak secrets/PII into normal logs.
🧹 Log metadata + sanitized summary only
for r in &results { - let preview = truncate_with_ellipsis(&r.output, 300); + let summary = hooks::sanitize_tool_output(&r.output, &r.name, r.success); log::info!( - "[agent] tool response name={} success={} output_chars={}\n{}", + "[agent] tool response name={} success={} output_chars={} summary={}", r.name, r.success, r.output.chars().count(), - preview + summary ); }- log::debug!( - "[agent] tool output for {}: {}", - call.name, - truncate_with_ellipsis(&result, 500) - ); + let output_summary = hooks::sanitize_tool_output(&result, &call.name, success); + log::debug!( + "[agent] tool output summary name={} success={} output_chars={} summary={}", + call.name, + success, + result.chars().count(), + output_summary + ); @@ - let output_summary = hooks::sanitize_tool_output(&result, &call.name, success); + // reuse `output_summary` computed aboveAlso applies to: 524-528
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/openhuman/agent/agent/turn.rs` around lines 371 - 379, The current log in the loop over results prints raw/truncated tool output (truncate_with_ellipsis(&r.output)) which can leak secrets; remove the raw preview from the log and instead log only metadata (r.name, r.success, r.output.chars().count()) plus a sanitized or redacted summary. Replace the use of truncate_with_ellipsis(&r.output) in the log::info call with either a call to a sanitization/summarization helper (e.g., sanitize_tool_output(&r.output) or summarize_tool_output(&r.output)) that strips/obfuscates PII/secrets, or a fixed placeholder like "<redacted>" if no sanitizer exists; update both occurrences around the loop over results and any other log::info that currently uses truncate_with_ellipsis so logs never contain raw tool payloads.
472-492:⚠️ Potential issue | 🔴 CriticalEnforce allowlist during dispatch, not only in provider schema.
Line 472 still resolves against
self.toolswithout checking visibility. A hidden tool name can still execute if emitted by the parser path, bypassing the orchestrator boundary.🔒 Minimal guard
- let (raw_result, success) = - if let Some(tool) = self.tools.iter().find(|t| t.name() == call.name) { + let (raw_result, success) = if !self.visible_tool_names.is_empty() + && !self.visible_tool_names.contains(&call.name) + { + ( + format!("Tool '{}' is not available to this agent", call.name), + false, + ) + } else if let Some(tool) = self.tools.iter().find(|t| t.name() == call.name) { let exec = tool.execute(call.arguments.clone()); let outcome = if let Some(fork_ctx) = fork_context_for_call { harness::with_fork_context(fork_ctx, exec).await } else { exec.await }; match outcome { Ok(r) => { if !r.is_error { (r.output(), true) } else { (format!("Error: {}", r.output()), false) } } Err(e) => (format!("Error executing {}: {e}", call.name), false), } } else { (format!("Unknown tool: {}", call.name), false) };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/openhuman/agent/agent/turn.rs` around lines 472 - 492, The dispatch currently resolves against self.tools and executes any matching tool by name (using tool.execute and call.name) without verifying visibility; add an allowlist/visibility check on the discovered tool before running harness::with_fork_context or exec.await (e.g., require tool.is_visible()/tool.allowed_to_run() or equivalent) and if the check fails return a denied/unknown-tool response (instead of executing) so hidden tools cannot be invoked via the parser path; apply this check in the same matching block that finds the tool (before calling tool.execute) and adjust the error branch to reflect permission denied when visibility fails.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/openhuman/agent/agent/turn.rs`:
- Around line 371-379: The current log in the loop over results prints
raw/truncated tool output (truncate_with_ellipsis(&r.output)) which can leak
secrets; remove the raw preview from the log and instead log only metadata
(r.name, r.success, r.output.chars().count()) plus a sanitized or redacted
summary. Replace the use of truncate_with_ellipsis(&r.output) in the log::info
call with either a call to a sanitization/summarization helper (e.g.,
sanitize_tool_output(&r.output) or summarize_tool_output(&r.output)) that
strips/obfuscates PII/secrets, or a fixed placeholder like "<redacted>" if no
sanitizer exists; update both occurrences around the loop over results and any
other log::info that currently uses truncate_with_ellipsis so logs never contain
raw tool payloads.
- Around line 472-492: The dispatch currently resolves against self.tools and
executes any matching tool by name (using tool.execute and call.name) without
verifying visibility; add an allowlist/visibility check on the discovered tool
before running harness::with_fork_context or exec.await (e.g., require
tool.is_visible()/tool.allowed_to_run() or equivalent) and if the check fails
return a denied/unknown-tool response (instead of executing) so hidden tools
cannot be invoked via the parser path; apply this check in the same matching
block that finds the tool (before calling tool.execute) and adjust the error
branch to reflect permission denied when visibility fails.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2be3465c-ae6f-4362-908e-0004b97ef843
📒 Files selected for processing (3)
src/openhuman/agent/agent/turn.rssrc/openhuman/agent/prompt.rssrc/openhuman/tools/ops.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- src/openhuman/tools/ops.rs
- src/openhuman/agent/prompt.rs
senamakel
previously approved these changes
Apr 10, 2026
… sync - Enforce visible-tool allowlist at dispatch time (not just schema) - Fork mode uses visible_tool_specs (not full registry) - De-duplicate spawn_subagent when extending orchestrator tools - Raw tool output moved to debug level, info level logs metadata only - Decouple workspace file sync from prompt rendering so skipped files still get synced to disk Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
Author
|
Re: fork mode tool_specs (outside-diff comment) Fixed — |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/openhuman/agent/agent/builder.rs (1)
75-78: The rustdoc describes aNonecall path this setter doesn't expose.
visible_tool_names()only accepts aHashSet<String>, so callers cannot "PassNone" here. Reword this to "omit this setter" or "pass an empty set" so the public API docs match the actual behavior.Based on learnings, "New or changed behavior must ship with matching documentation. Add concise rustdoc / code comments where the flow is not obvious, and update
AGENTS.md, architecture docs, or feature docs when repository rules or user-visible behavior change."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/openhuman/agent/agent/builder.rs` around lines 75 - 78, The docstring for the builder method visible_tool_names is incorrect about accepting None; update the rustdoc on the visible_tool_names(found in builder.rs) to state that callers should either omit calling the setter to leave all tools visible or pass an empty HashSet to hide all tools (instead of "Pass `None`"), and add a brief clarifying comment in the visible_tool_names implementation explaining the chosen convention; also update the consumer-facing docs (e.g., AGENTS.md or architecture/feature docs) to reflect this behavior so public docs and code are consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/openhuman/agent/agent/builder.rs`:
- Around line 449-461: The current name-based de-dup allows a raw skill tool to
remain registered while the orchestrator delegator with the same name is
filtered out, yet `visible` still contains that name causing `build()` to
publish the wrong implementation; update the block that collects
`orchestrator_tools`/computes `visible`/`existing_names` (symbols:
collect_orchestrator_tools, visible, existing_names, tools.extend, build,
spawn_subagent) to detect name collisions and fail fast: compute the
intersection of orchestrator tool names and existing_names and if non-empty
return an Err or panic with a clear message (listing the colliding names) so the
PR must resolve ambiguous names before proceeding; alternatively remove collided
names from `visible` so they can't be published, but prefer the fail-fast
behavior until names are made unambiguous.
In `@src/openhuman/agent/prompt.rs`:
- Around line 383-399: The code currently uses
std::fs::read_to_string(&hash_path).unwrap_or_default(), which treats any read
error (including NotFound) as an empty hash and causes existing workspace files
(path) to be overwritten; change to explicitly match the read result: if
Ok(stored_hash) continue with the existing logic; if Err(e) and e.kind() ==
std::io::ErrorKind::NotFound then do NOT overwrite an existing path — if
path.exists() only write the sidecar/hash file with current_hash (do not write
default_content), and only write default_content when the workspace file itself
does not exist; for other read errors (Err but not NotFound) log a warning and
abort the write to avoid clobbering; update the logic around stored_hash,
current_hash, hash_path, and path to reflect this explicit match-based handling.
---
Nitpick comments:
In `@src/openhuman/agent/agent/builder.rs`:
- Around line 75-78: The docstring for the builder method visible_tool_names is
incorrect about accepting None; update the rustdoc on the
visible_tool_names(found in builder.rs) to state that callers should either omit
calling the setter to leave all tools visible or pass an empty HashSet to hide
all tools (instead of "Pass `None`"), and add a brief clarifying comment in the
visible_tool_names implementation explaining the chosen convention; also update
the consumer-facing docs (e.g., AGENTS.md or architecture/feature docs) to
reflect this behavior so public docs and code are consistent.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: fed364f4-d583-4c72-b845-b1c4c4304afc
📒 Files selected for processing (3)
src/openhuman/agent/agent/builder.rssrc/openhuman/agent/agent/turn.rssrc/openhuman/agent/prompt.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/openhuman/agent/agent/turn.rs
This was referenced Apr 11, 2026
This was referenced Apr 18, 2026
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
Refactors the main agent from a direct tool-calling model to a pure orchestrator that delegates all work through dynamically generated tools.
Token savings per LLM call
TOOLS.mdin system promptMEMORY.mdin system promptAGENTS.mdin system promptHEARTBEAT.mdin system promptThis applies to every turn the orchestrator makes (typically 1-3 per user message). Sub-agent calls are unaffected — they already use narrow prompts.
Architecture: Orchestrator -> Sub-agent delegation
The orchestrator LLM now sees a flat list of well-named tools instead of a single
spawn_subagentmega-tool:notionskills_agent(filter: notion)gmailskills_agent(filter: gmail)researchresearchersubagentrun_codecode_executorsubagentreview_codecriticsubagentplanplannersubagentspawn_subagentEach tool's
execute()internally callsrun_subagentwith the correct definition + skill_filter.Prompt architecture diagram
See
docs/agent-prompt-architecture.excalidrawfor the full visual breakdown of which.mdfiles are used by which agent.Orchestrator system prompt (from workspace files):
TOOLS.md— skipped (subagents get tool specs from registry)MEMORY.md— skipped (memory auto-injected)HEARTBEAT.md— skipped (handled by cron system)Sub-agents get only: their archetype
.mdprompt + filteredToolSpecschemas + auto-forwarded memory context.Changes
orchestrator_tools.rs(new) — generatesSkillDelegationToolper installed skill +ArchetypeDelegationToolper archetype at agent build timebuilder.rs— orchestrator uses generated tools;visible_tool_specsfilters function-calling schema sent to providerturn.rs—[agent]tagged logs for tool responses, state transitions, delegation decisions; sendsvisible_tool_specsnot full specstypes.rs— addedvisible_tool_specsandlast_memory_contextfieldsdefinition.rs—ModelSpec::Hintresolves to{hint}-v1(fixeshint:agentic->agentic-v1)archetypes.rs— orchestrator/planner usereasoning-v1; code_executor/tool_maker usecoding-v1; all others useagentic-v1; archivist useslocal-v1subagent_runner.rs— auto-injects parent's memory context into subagent promptsfork_context.rs— addedmemory_contexttoParentExecutionContextprompt.rs— orchestrator skips TOOLS/MEMORY/HEARTBEAT; workspace files auto-sync via builtin-hash mechanismAGENTS.md— lean orchestrator prompt (no routing tables, tools are self-describing)spawn_subagent.rs— fixedvalidate_skill_filterto useskill_idfrom engine tuple (was splitting on__in raw tool name, always failed)ops.rs—MAIN_AGENT_TOOL_ALLOWLISTlegacy constant, orchestrator tools are now dynamicBug fixes
ModelSpec::Hint("agentic")now resolves toagentic-v1instead ofhint:agentic(backend rejected the old format)validate_skill_filternow correctly matches skill IDs (was always returning empty available list)ParentExecutionContext.memory_context.mdfiles auto-sync when compiled-in defaults change (hash-based migration)Test plan
cargo check— compiles clean (14 pre-existing warnings)cargo test --lib— 2303 passed, 0 new failures (11 pre-existing)model_spec_resolve_hint_appends_v1,resolve_model_defaultidentity_section_creates_missing_workspace_files,identity_section_with_aieos_includes_workspace_filesgmail->skills_agent(gmail)researchtool delegates toresearchersubagentopenhuman.skills_call_tool(gmailget-profilereturned user data)yarn core:stage && yarn tauri dev) — skills loaded + orchestrator routing end-to-endGenerated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Bug Fixes