fix(agents): run custom registry agents with their real tools (flows + chat + tasks)#5121
Conversation
…+ chat + tasks)
The agent factory required a harness AgentDefinition and never consulted
config.agent_registry.entries, so a user-created CUSTOM agent degraded to
persona-only in flows (RegistryFallback, tools ignored) and hard-errored in
chat/task-dispatcher ('agent definition not found'). Add the reverse bridge
definition_from_registry_entry + a sync find_custom_in_config helper, and a
fallback in from_config_for_agent(+_with_profile) that synthesizes an
AgentDefinition from a custom entry and runs it through the normal
build_session_agent_inner. Flows try the harness path before the fallback.
Same tool-filter/approval/security path — custom agents can't grant tools the
user lacks.
![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: 267f60d6d6
ℹ️ 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".
| if allowlist.is_empty() || allowlist == ["*"] { | ||
| ToolScope::Wildcard |
There was a problem hiding this comment.
When a custom agent is created or edited with no tools selected, the UI and schema describe that as an empty tool_allowlist while "*" means all tools; this branch converts that empty list into ToolScope::Wildcard, so the newly-synthesized custom agent is allowed to see every enabled tool instead of none. This affects any custom agent saved without selecting tools (including existing configs that omitted the field), and it bypasses the per-agent least-privilege setting the editor shows.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in ed52640 — allowlist_to_tool_scope now only maps exactly ["*"] to Wildcard; an empty allowlist maps to Named(vec![]) (tool-less), matching how the forward direction (tools_to_allowlist) renders Named(vec![]) back to []. Added a regression test (definition_from_registry_entry_empty_allowlist_stays_tool_less).
| .find(|entry| entry.id == id && matches!(entry.source, AgentRegistrySource::Custom)) | ||
| .cloned() |
There was a problem hiding this comment.
This lookup returns custom entries regardless of enabled, and resolve_target_definition immediately synthesizes them into runnable definitions for chat/task/MCP factory paths. As a result, a disabled custom agent can still run if an existing profile or direct caller references its id; flows explicitly route disabled entries to the fallback path so they are rejected, but these direct factory paths now ignore the disabled flag.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in b6bfeb7 — find_custom_in_config now filters on entry.enabled, so a disabled custom entry is treated as a miss (None) everywhere this lookup is used (chat/task-dispatcher factory paths and the flows routing), same as an unknown id — matching the enforcement route_custom_entry_lookup/run_via_registry_fallback already had on the flows side. Added find_custom_in_config_ignores_disabled_custom_entries regression test.
| harness so the custom agent runs with its real tool belt instead of \ | ||
| the persona-only completion fallback" | ||
| ); | ||
| self.run_via_harness(agent_ref, request, conn).await |
There was a problem hiding this comment.
For an enabled custom agent_ref, this now switches from run_via_registry_fallback to the harness path, but run_via_harness resolves only the node's own model and passes None for the registry entry model, while the session builder does not apply the synthesized definition's model. Therefore a flow custom agent with model: "hint:reasoning" or a raw BYOK model and no per-node override now runs on the default chat model, whereas the old fallback inserted entry.model into the completion request.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 141dc8c — run_via_harness now takes an entry_model: Option<&str> parameter threaded from the custom registry entry's own model at the CUSTOM-REGISTRY call site (None for a shipped/TOML harness definition, which has no such entry), and passes it into resolve_node_model as the fallback below the node's own override — restoring the precedence run_via_registry_fallback already gave entry.model.
📝 WalkthroughWalkthroughCustom registry entries are looked up from config, synthesized into harness definitions with their tool and model settings, resolved by both agent builders, and routed through full harness execution when enabled. Unknown and disabled entries retain fallback behavior. ChangesCustom registry execution
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Runner
participant RegistryLookup
participant AgentBuilder
participant Harness
Runner->>RegistryLookup: find enabled custom entry by agent_ref
RegistryLookup-->>Runner: custom entry and model
Runner->>AgentBuilder: resolve target definition
AgentBuilder->>RegistryLookup: synthesize AgentDefinition
RegistryLookup-->>AgentBuilder: CustomRegistry definition
AgentBuilder->>Harness: build and run with configured tools and model
Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
coderabbitai
Bot
added
agent
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
`builder_tests.rs`'s new `from_config_for_agent_still_errors_for_a_genuinely_unknown_id` used `.expect_err()` on `Result<Agent, _>`, but `Agent` has no `Debug` impl (it holds `Box<dyn Tool>` / provider trait objects). This is a hard compile error under `--lib` test compilation in both the default and `--no-default-features` builds, and was the actual root cause of both the "Rust Feature-Gate Smoke (gates off)" and "Rust Core Coverage" CI failures on tinyhumansai#5121 (neither job got past compiling the test binary). Switch to `.is_err()` + `.err().unwrap()`, which doesn't require Debug.
definition_from_registry_entry's allowlist_to_tool_scope collapsed an empty tool_allowlist to ToolScope::Wildcard, granting a custom agent saved with no tools selected every enabled tool instead of none. The settings UI/schema mean "no tools" by an empty list and "all tools" by ["*"] specifically — tools_to_allowlist (the forward direction) already renders Named(vec![]) back to [], never ["*"], so this collapsed the two shapes asymmetrically and bypassed the least-privilege setting the editor shows. Fix: only exactly ["*"] maps to Wildcard; every other list (including empty) maps to Named(allowlist), matching the forward direction. Addresses @chatgpt-codex-connector P1 review comment on src/openhuman/agent_registry/defaults.rs:139.
find_custom_in_config returned a matching Custom-source entry regardless of its enabled flag, so a disabled custom agent referenced directly by chat/task-dispatcher (e.g. via an existing profile) would still be synthesized into a runnable AgentDefinition and executed — bypassing the disabled flag that the flows path already enforces explicitly via route_custom_entry_lookup / run_via_registry_fallback. Fix: filter on entry.enabled in find_custom_in_config, so a disabled custom entry is treated as a miss (None) everywhere this lookup is used, same as an unknown id. Addresses @chatgpt-codex-connector P2 review comment on src/openhuman/agent_registry/ops.rs:225.
run_via_harness resolved the node model with resolve_node_model(&req, None) unconditionally, so once a custom flow agent_ref was routed through the harness (issue B38), its own entry.model (e.g. "hint:reasoning" or a raw BYOK model id) was dropped in favor of the default chat model whenever the flow node had no per-node model override — a regression versus run_via_registry_fallback, which always honored entry.model. Fix: thread the custom entry's model through as run_via_harness's new entry_model parameter (None for a shipped/TOML harness definition, which has no such entry), so resolve_node_model's existing precedence (node override > entry model > definition default) applies on both paths. Addresses @chatgpt-codex-connector P2 review comment on src/openhuman/tinyflows/caps.rs:929.
coderabbitai
Bot
added
the
rust-core
|
Pushed 4 commits addressing the review cycle: Root cause of the 2 originally-red gates (Feature-Gate Smoke + Rust Core Coverage): both jobs were failing at the same compile error, not a real feature-gate bug or a coverage shortfall — The 3 Codex review comments — all fixed and replied to inline:
Unrelated blocker found while watching CI: All new/changed tests pass locally under both default and |
coderabbitai
Bot
removed
the
rust-core
…initialized build_session_agent_inner's delegation-tool-and-visibility computation matched on (target_def, AgentDefinitionRegistry::global()), with a catch-all (_, None) arm for "registry not initialised" that discarded target_def entirely, treating it as "no filter" (empty visible == no tools restricted). That's correct when target_def is also None, but wrong when target_def is Some(_) — in particular for a CustomRegistry definition synthesized by resolve_target_definition / definition_from_registry_entry, which is deliberately built straight from config.agent_registry.entries WITHOUT ever consulting the harness registry, precisely so a custom agent can build even before/without AgentDefinitionRegistry::init_global. Hitting that arm silently dropped the custom agent's ToolScope::Named allowlist, leaving visible_tool_names_for_test() empty instead of the named tools — regressing the least-privilege contract from the P1 fix earlier in this review cycle. Splits (_, None) into (Some(def), None) — apply def.tools (Named or Wildcard) same as the (Some, Some) arm, just without delegation-tool synthesis (which needs the registry to resolve subagents) — and (None, None), the genuine no-definition/no-registry case. Reproduced deterministically in isolation (from_config_for_agent_synthesizes_custom_registry_entry_with_named_scope run alone, with no prior test in the same binary having initialized the global registry) — not global-state pollution making a good test wrongly fail, but pollution from OTHER tests' registry-init calls masking a real bug when this test happened to run after them. Verified the fix does not regress the (Some, Some) or (None, Some) arms via the full agent::harness::session::builder:: + agent_registry:: + tinyflows:: suite (307 passed).
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/openhuman/agent/harness/session/builder/factory.rs (2)
164-164: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winUse the shared resolver for reflection-chunk sessions too.
from_config_for_agent_with_reflection_chunksstill performs only a global harness-registry lookup on Lines 138-142. A config-only custom agent will therefore receiveNoneand enter the legacy path, losing its custom prompt/tool scope and potentially exposing unrestricted tools. Route that constructor throughresolve_target_definition(config, agent_id)?and add a regression test.Suggested fix
- let target_def: Option<crate::openhuman::agent::harness::definition::AgentDefinition> = - match AgentDefinitionRegistry::global() { - Some(reg) => reg.get(agent_id).cloned(), - None => None, - }; + let target_def = resolve_target_definition(config, agent_id)?;🤖 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/agent/harness/session/builder/factory.rs` at line 164, Update from_config_for_agent_with_reflection_chunks to obtain the target definition through the shared resolve_target_definition(config, agent_id)? resolver instead of only the global harness registry lookup, preserving config-only custom agents’ prompt and tool scope. Add a regression test covering a config-only custom agent and verifying the reflection-chunk session uses its resolved definition.
1186-1260: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd branch/error logs for the silent resolution paths in
resolve_target_definition. The harness-registry hit, uninitialized-registry error, and unknown-id error still return without a grep-friendly message; the caller only logs successful builds, so these cases are hard to distinguish in logs.🤖 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/agent/harness/session/builder/factory.rs` around lines 1186 - 1260, Update resolve_target_definition to add grep-friendly logs before returning from the harness-registry hit, the uninitialized-registry error, and the unknown-agent error paths. Include the agent_id and clearly identify whether the definition was resolved from the harness registry or why resolution failed, while preserving the existing return behavior.Source: Coding guidelines
🤖 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/agent/harness/session/builder/factory.rs`:
- Around line 851-854: Preserve the distinction between ToolScope::Named with no
names and ToolScope::Wildcard when constructing filter and visible_tool_names:
represent an empty named scope explicitly so the logic around lines 875-880
yields no visible tools, while only ToolScope::Wildcard yields unrestricted
access. Update or retain regression coverage verifying an empty tool_allowlist
does not expose registered tools.
---
Outside diff comments:
In `@src/openhuman/agent/harness/session/builder/factory.rs`:
- Line 164: Update from_config_for_agent_with_reflection_chunks to obtain the
target definition through the shared resolve_target_definition(config,
agent_id)? resolver instead of only the global harness registry lookup,
preserving config-only custom agents’ prompt and tool scope. Add a regression
test covering a config-only custom agent and verifying the reflection-chunk
session uses its resolved definition.
- Around line 1186-1260: Update resolve_target_definition to add grep-friendly
logs before returning from the harness-registry hit, the uninitialized-registry
error, and the unknown-agent error paths. Include the agent_id and clearly
identify whether the definition was resolved from the harness registry or why
resolution failed, while preserving the existing return behavior.
🪄 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: d6c56539-4242-4e4b-8449-b7fea73ad58c
📒 Files selected for processing (2)
src/openhuman/agent/harness/session/builder/builder_tests.rssrc/openhuman/agent/harness/session/builder/factory.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/openhuman/agent/harness/session/builder/builder_tests.rs
| let filter: Option<std::collections::HashSet<String>> = match &def.tools { | ||
| ToolScope::Named(names) => Some(names.iter().cloned().collect()), | ||
| ToolScope::Wildcard => None, | ||
| }; |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Preserve the distinction between an empty named scope and wildcard access.
ToolScope::Named(empty) becomes an empty HashSet, but Lines 875-880 treat an empty set as “no filter.” Thus an empty tool_allowlist exposes every registered tool, contradicting the stated contract that only ["*"] maps to ToolScope::Wildcard. Carry an explicit “named scope present” state through visible_tool_names (or otherwise represent empty named scopes distinctly), and retain regression coverage for empty allowlists.
🤖 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/agent/harness/session/builder/factory.rs` around lines 851 -
854, Preserve the distinction between ToolScope::Named with no names and
ToolScope::Wildcard when constructing filter and visible_tool_names: represent
an empty named scope explicitly so the logic around lines 875-880 yields no
visible tools, while only ToolScope::Wildcard yields unrestricted access. Update
or retain regression coverage verifying an empty tool_allowlist does not expose
registered tools.
1 participant
Cross-cutting fix (B38). The agent factory
from_config_for_agentonly read harnessAgentDefinitions, neverconfig.agent_registry.entries— so a user's CUSTOM agent degraded to persona-only in flows (tools silently ignored) and hard-errored in chat + task-dispatcher.Fix (synthesize-on-lookup): add the reverse bridge
definition_from_registry_entry(agent_registry/defaults.rs) + syncfind_custom_in_config; on a harness-registry miss, synthesize anAgentDefinitionfrom the custom entry and run it through the samebuild_session_agent_inner; flows try the harness path before the persona fallback. Fixes all three surfaces at once.Security: flows through the identical
SecurityPolicy/tool-filter/approval-gate path;tool_allowlist→ToolScope::Named; a custom agent can't grant tools the user doesn't have. (Local core-crate compile is infeasibly slow on this machine's external SSD — the implementing agent reached test-binary compilation, so the lib compiles; CI runs the full suite.)Summary by CodeRabbit
*) and empty allowlist handling.