feat(sdk): enforce disabled_agents deny-list at sub-agent spawn time - #4557
feat(sdk): enforce disabled_agents deny-list at sub-agent spawn time#4557georgeglarson wants to merge 7 commits into
Conversation
Add AgentContext.disabled_agents (mirror of disabled_skills) and enforce it where sub-agents are created: TaskManager._create_task/_resume_task raise a ValueError naming the disabled type (surfaces to the LLM as a retryable tool error), and the delegate tool's spawn path returns an error observation. Enforcement lives at spawn time because the sub-agent registry is process-global and shared across conversations, so registration-time filtering cannot be per-conversation. Co-authored-by: openhands <openhands@all-hands.dev>
Co-authored-by: openhands <openhands@all-hands.dev>
|
📁 PR Artifacts Notice This PR contains a |
…covery) Co-authored-by: openhands <openhands@all-hands.dev>
Co-authored-by: openhands <openhands@all-hands.dev>
Co-authored-by: openhands <openhands@all-hands.dev>
DisabledAgentError (a ValueError) is raised by the deny-list guard and caught specifically in TaskExecutor: the model gets the same error observation text, but a policy refusal no longer logs an ERROR-level traceback (it rides the generic exception path no more). Evidence logs refreshed: no Traceback/ERROR in the refusal path; live e2e still shows refusal + recovery with a real model. Co-authored-by: openhands <openhands@all-hands.dev>
|
🤖 OpenHands is reviewing this PR. Head commit: This comment was posted by an AI agent (OpenHands). |
all-hands-bot
left a comment
There was a problem hiding this comment.
This review was created by an AI agent (OpenHands) on behalf of the repository maintainers.
Summary
This PR adds AgentContext.disabled_agents: list[str], a per-conversation sub-agent deny-list mirroring the existing disabled_skills field, and enforces it at spawn time in both the task tool (TaskManager._create_task/_resume_task) and the delegate tool (DelegateExecutor._spawn_agents).
Risk Assessment: Low
The change is additive and backward-compatible:
disabled_agentsis a newAgentContextfield withdefault_factory=list, so existing persisted settings payloads load unchanged — no schema-version bump is required (the change is purely additive, not incompatible). This mirrors howdisabled_skillswas introduced.- Enforcement at spawn time (rather than registration time) is the correct design given the sub-agent registry is process-global and shared across conversations on the agent-server. Registration-time filtering would disable the agent for every conversation in the process.
- Both sub-agent spawn paths are covered. The task tool raises
DisabledAgentError(ValueError), whichTaskExecutor.__call__catches before the genericExceptionhandler and logs at INFO (a policy outcome, not a crash). The delegate path early-returns an error observation before any sub-agent conversation is created, so no cleanup is needed. _delegate_tasksonly delegates to existing sub-agents (it errors on missing IDs rather than spawning), so there is no bypass path through delegation.- The deny-list check uses exact list membership (
subagent_type in disabled), not substring matching, so disabling"general-purpose"cannot accidentally block"general".
Material Findings
None. No bugs, security issues, or design flaws found on the changed lines.
Minor observations (not blocking)
_check_agent_enabledis called inside_tasks_lockin_resume_taskbut outside the lock in_create_task. This is harmless since the check is a read of an effectively-immutable list, but the asymmetry is slightly surprising.- The two error messages differ slightly in wording (task: "Sub-agent 'X' is disabled..."; delegate: "Sub-agent type(s) disabled...: X"). Both are clear and the inconsistency is cosmetic.
Tests cover field round-trip, create/refusal, resume/refusal, an enabled-type control, delegate spawn refusal, and the clean-error (no ERROR-level log) behavior. The .pr/ demo scripts and logs provide end-to-end evidence consistent with the repo PR-artifacts convention.
HUMAN:
Tested and verified by human. e2e logs attached
AGENT:
End-to-end evidence (the template asks for more than unit tests): two demo scripts in
.pr/, run against the editable install of the checkout under test, no LLM calls involved. Real registry, TaskManager, TaskExecutor, and DelegateExecutor throughout.Each script was run on this branch and on clean upstream/main (d98fd95); full output for all four runs is in
.pr/demo-logs/.On clean main:
AgentContexthas nodisabled_agents(anagent_settings.agent_context.disabled_agentskey in the start-conversation payload is silently dropped during validation), spawning and resuming the "disabled" type proceed, and delegate spawn reports "Successfully spawned".On this branch: the field lands on the built agent, create and resume refuse with
Sub-agent 'general-purpose' is disabled for this conversation (agent_context.disabled_agents). Choose another sub-agent type.(returned to the calling LLM as an ordinary, retryable tool error), delegate spawn returns an error observation and creates nothing, and types not on the list spawn and resume normally.Why
The sub-agent side of the SDK has no per-conversation deny-list (skills have
AgentContext.disabled_skills; sub-agents have nothing equivalent). The GUI Sub-Agents page (OpenHands/OpenHands#16662) persists adisabled_agentspreference that today cannot even reach the agent: the key is silently dropped during validation. And because the sub-agent registry is process-global, an agent-server cannot unregister an agent for one conversation without removing it for all of them. Details in #4556.Summary
AgentContext.disabled_agents: list[str], a deny-list mirroringdisabled_skills; it rides the existing AgentContext plumbing (StartConversationRequest, settings schema).TaskManager._create_task/_resume_taskraise a ValueError naming the disabled type (surfaces as a retryable tool error); the delegate tool's spawn path returns an error observation.Issue Number
Fixes #4556
How to Test
Live demos (no API key needed):
Unit tests:
Video/Screenshots
Design Doc
Not included; the change is ~40 LOC plus tests, and
.pr/carries runnable demos with reference output instead.Type
Notes
agent-server-adapter.tspassing the GUI preference into the agent_context payload (can ride with feat(settings): add Sub-Agents page to Customize OpenHands#16662).