fix(flows): reject an unknown agent_ref at author time instead of mid-run#5114
Conversation
…-run A flow `agent` node selects which agent runs via config.agent_ref. Previously an unknown agent_ref only failed at RUN time (run_via_registry_fallback hard- errors). Add a validate_agent_refs gate to run_builder_gates so propose/edit/ save reject a graph whose agent node references an unregistered agent, naming the bad id — fail-fast UX. A plain agent node with no agent_ref, and nodes referencing a real registered agent, pass.
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds asynchronous agent-reference validation to the builder hard-gate pipeline. Agent references resolve to harness agents or enabled custom registry entries; invalid references reject graphs before later validation and persistence. Tests cover blank, known, and unknown references. ChangesAgent Reference Validation
Estimated code review effort: 4 (Complex) | ~40 minutes Sequence Diagram(s)sequenceDiagram
participant run_builder_gates
participant validate_agent_refs
participant CustomAgentRegistry
run_builder_gates->>validate_agent_refs: Validate agent_ref values in WorkflowGraph
validate_agent_refs->>CustomAgentRegistry: Look up custom agent entries
CustomAgentRegistry-->>validate_agent_refs: Return registry result
validate_agent_refs-->>run_builder_gates: Return errors or continue to later gates
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}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/openhuman/flows/ops.rs (1)
1239-1293: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winRegistry lookup reloads config once per
RegistryFallbacknode.
agent_registry::get_agentdelegates tolist_agents(true), which callsconfig_rpc::load_config_with_timeout().awaiton every invocation (seesrc/openhuman/agent_registry/ops.rs:19-33). A graph with several customagent_refnodes therefore triggers N sequential config reads inside this loop. Consider fetching the registry list once (lazily, on the firstRegistryFallbackhit, to keep the all-Harness/no-custom-ref case free of any config read) and resolving each ref against that cached snapshot, preserving the current per-lookup fail-open behavior.🤖 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/flows/ops.rs` around lines 1239 - 1293, Update the RegistryFallback handling in the flow validation loop to lazily load the custom-agent registry once on the first such node, cache that snapshot, and resolve subsequent agent_ref values against it instead of calling agent_registry::get_agent repeatedly. Keep all-Harness flows free of registry reads and preserve the current enabled, disabled, unknown, and lookup-error outcomes, including fail-open behavior when loading the snapshot fails.
🤖 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.
Nitpick comments:
In `@src/openhuman/flows/ops.rs`:
- Around line 1239-1293: Update the RegistryFallback handling in the flow
validation loop to lazily load the custom-agent registry once on the first such
node, cache that snapshot, and resolve subsequent agent_ref values against it
instead of calling agent_registry::get_agent repeatedly. Keep all-Harness flows
free of registry reads and preserve the current enabled, disabled, unknown, and
lookup-error outcomes, including fail-open behavior when loading the snapshot
fails.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0d2c3539-b14d-4db9-8d7c-b9d141f2dc1a
📒 Files selected for processing (1)
src/openhuman/flows/ops.rs
![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: 14c7134495
ℹ️ 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".
| // `RegistryFallback` "unknown agent_ref" hard error mid-run. Almost always a | ||
| // pure in-memory harness-registry lookup; only a ref that ISN'T a harness | ||
| // definition falls through to a local config read (custom agent registry). | ||
| let agent_ref_errors = validate_agent_refs(graph).await; |
There was a problem hiding this comment.
When run_builder_gates is reached before the server bootstrap has initialized the global AgentDefinitionRegistry (for example via the generic CLI/RPC path) and the flow uses a valid workspace TOML agent under <workspace>/agents, this call gives validate_agent_refs no Config/workspace to initialize or load definitions from. route_for_agent_ref then sees an empty global registry and falls through to agent_registry::get_agent, which only merges the default/config agent registry entries, so a real harness agent definition can be rejected as an unknown agent_ref even though the runtime would load it from the active workspace. Pass the active config through and resolve against that workspace before rejecting.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in — you're right: validate_agent_refs now takes &Config and defensively calls AgentDefinitionRegistry::init_global (idempotent OnceLock) before resolving refs, so one-shot CLI/RPC paths (where default_state never bootstraps the registry) no longer falsely reject every agent_ref. +tests proving it under the scoped flows::ops filter.
… (codex tinyhumansai#5114) validate_agent_refs relied on the process-global AgentDefinitionRegistry, which is NOT initialized on one-shot CLI/RPC paths (default_state never calls bootstrap_core_runtime) — it would falsely reject every agent_ref there. Now takes &Config, defensively AgentDefinitionRegistry::init_global (idempotent), and lazily caches the custom-agent snapshot instead of reloading config per node (CodeRabbit). +3 unit tests (plain node passes / blank ref absent / harness ref resolves under defensive init).
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 429bedc887
ℹ️ 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".
| } | ||
|
|
||
| #[tokio::test] | ||
| async fn agent_ref_resolving_to_a_harness_definition_is_accepted() { |
There was a problem hiding this comment.
The new tests only exercise absent/blank refs and the harness-success path, leaving the newly added RegistryFallback branch that actually rejects unknown or disabled agent_refs unexecuted. Since this repo enforces ≥80% coverage on changed lines, the changed error-construction paths in validate_agent_refs are likely to fail the Rust diff coverage gate, and the main behavior advertised by this fix is not pinned; add at least an unknown-ref test through validate_agent_refs or run_builder_gates.
AGENTS.md reference: AGENTS.md:L86-L88
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in c154c2c — added agent_ref_unknown_is_rejected: a graph with an unregistered agent_ref (no_such_agent_xyz) is asserted to be rejected by validate_agent_refs, with the offending id present in the error. That exercises the RegistryFallback error-construction branch the plain/blank/harness tests left uncovered, so the changed error paths are now covered.
…agent-ref # Conflicts: # src/openhuman/flows/ops.rs
1 participant
What & why
A flow
agentnode picks which agent runs viaconfig.agent_ref(resolved through the registry intinyflows/caps.rs). Previously an unknownagent_refwas only caught at run time —run_via_registry_fallbackhard-errors"unknown agent_ref '…'"mid-run — so the builder could propose/save a graph that's doomed to fail when it executes.This adds a
validate_agent_refsgate torun_builder_gates, so propose / edit / save reject a graph whoseagentnode references an unregistered agent, naming the bad id. Fail-fast at author time instead of a mid-run capability error.Behavior
agentnode with an unknownagent_ref→ rejected at propose/save, error names the id.agentnode with a real registeredagent_ref(built-in harness agent) → passes.agentnode with noagent_ref(runs on the default completion) → passes (not rejected).Note: this is a fail-fast UX improvement — the runtime already hard-errors on an unknown id; this just moves the failure to author time so a bad graph never gets saved.
Tests
Added unit tests for all three cases (unknown → rejected with id, valid → passes, no-agent_ref → passes).
flows::opssuite: 194 pass,cargo checkclean,cargo fmtclean.Summary by CodeRabbit