fix(delegation): allow explicit code_execution + honor explicit toolsets - #34294
fix(delegation): allow explicit code_execution + honor explicit toolsets#34294Kyzcreig wants to merge 2 commits into
Conversation
|
I found one issue that looks worth fixing before merge. Title says "allow explicit
The orchestrator re-add mechanism at line 967 ( Why it matters: Suggested fix: Keep # In _strip_blocked_tools, remove "code_execution" from blocked_toolset_names
# (as the PR already does), but KEEP the strip call at line 955:
blocked_toolset_names = {
"delegation",
"clarify",
"memory",
# "code_execution" removed — subagents already have terminal
}This achieves the stated goal (allow explicit |
fb3d1c1 to
3fe1add
Compare
Two related fixes to delegate_task toolset scoping: 1. Remove code_execution from the default subagent block list. Subagents already inherit `terminal` (a strictly larger capability), so blocking only `execute_code` was asymmetric and prevented legitimate use cases — e.g. "compute SHA256 + sum of primes" would silently fail because the subagent had no execute_code tool despite requesting toolsets=['code_execution']. 2. Stop calling _strip_blocked_tools() on explicit caller-requested toolsets. When a user/parent agent explicitly passes `toolsets=[...]` to delegate_task(), their intent wins over the implicit safety block list. Inherited/default toolsets still get the strip applied. This prevents silent toolset deletion which was the root cause of the debugging session that surfaced this bug. Repro before fix: parent with toolsets=['hermes-cli'] delegating with toolsets=['code_execution'] → intersection preserves code_execution → _strip_blocked_tools deletes it → child gets [] toolsets → falls back to default tool surface, missing execute_code. After fix: child receives ['code_execution'] as requested. Tests updated: - TestStripBlockedTools.test_removes_blocked_toolsets: assert code_execution survives strip. - TestStripBlockedTools.test_code_execution_no_longer_blocked: new regression test pinning the explicit allow. Local patch on main; upstream PR deferred per Ace's call.
3fe1add to
08de1dd
Compare
teknium1
left a comment
There was a problem hiding this comment.
Thanks for addressing the child-toolset behavior. The current implementation needs re-scoping against a later mainline design change.
Problems
- The stated explicit-toolsets behavior is no longer present in the model-facing API: commit
ba0bc01d1f740c562b55925e404b82a48809c364deliberately removed it. Currentrun_agent.py:5718-5725does not forwardtoolsets, andtests/tools/test_async_delegation.py:489-510locks that in. - Removing the strip in
tools/delegate_tool.py:1136also admitsdelegation,clarify, andmemorythrough the internal explicit branch. This conflicts with the role-gated delegation re-add attools/delegate_tool.py:1139-1144and the documented blocked-toolset contract inwebsite/docs/user-guide/features/delegation.md:157-161. - Removing
execute_codefrom the blocklist changes inherited children too, not only an explicit request, and needs a production-path regression test plus documentation if that is the intended policy.
Suggested changes
- Rebase the proposal on parent-toolset inheritance, retaining the safety strip for
delegation,clarify, andmemory. - Treat code_execution inheritance as a focused policy change with an end-to-end child construction test and corresponding docs update.
This is an automated hermes-sweeper review.
| child_toolsets, parent_toolsets | ||
| ) | ||
| child_toolsets = _strip_blocked_tools(child_toolsets) | ||
| # Explicit caller requests bypass the default block list (memory, |
There was a problem hiding this comment.
Removing this strip admits delegation, clarify, and memory along with code_execution. delegation is currently re-added only for an effective orchestrator role below; please retain the safety strip for these three toolsets rather than letting an explicit internal request bypass that gate.
Caught by @liuhao1024 on NousResearch#34294. The PR title promises "allow explicit code_execution", but removing `_strip_blocked_tools()` from the explicit-request branch also unblocked `delegation`, `clarify`, and `memory` for any caller-supplied toolsets list. `delegation` is the serious one: it is granted by ROLE via the orchestrator re-add in `_build_child_agent`, deliberately not by request. Letting a caller request it means a non-orchestrator subagent can be handed delegation and spawn further subagents -- exactly the recursion the gate exists to prevent. Reproduced end-to-end against `_build_child_agent` with a mocked `run_agent.AIAgent`, a depth-0 non-orchestrator parent, and an explicit toolsets request: pre-PR base (dc3f61c): terminal this PR as filed: clarify, code_execution, delegation, memory, terminal <- gate bypassed after this commit: code_execution, terminal That is @liuhao1024's suggested shape: `code_execution` stays out of the strip set (so an explicit request obtains it -- the PR's actual goal), and the strip call is restored so the three safety-critical toolsets are removed on every path, inherited or explicit. Tests: an end-to-end child-construction assertion that explicit `code_execution` is honored while `delegation`/`clarify`/`memory` are not, plus a unit assertion on the strip set. The E2E test goes RED with this commit's implementation reverted. Co-authored-by: liuhao1024 <liuhao1024@users.noreply.github.com>
|
@liuhao1024 — you were right, and thank you for catching this. Apologies for the slow reply. This is a real safety bug, not a nit, and it's fixed in Your core claim:
I reproduced it end-to-end rather than just reading the diff —
So your table was accurate on all four rows. A non-orchestrator child was being handed
That's exactly the invariant I broke. I'd written a docstring justifying the removal as "silent deletion of explicitly requested toolsets is a footgun; user intent wins" — which is a defensible principle for ergonomic toolsets and simply the wrong principle for a recursion gate. Role-granted and request-granted are not interchangeable, and I collapsed them. The fix is your suggested shape. child_toolsets = _strip_blocked_tools(child_toolsets)I also rewrote the Tests: an E2E child-construction assertion that explicit One thing I'd value your view on, since you clearly read this path closely: the sweeper review notes that |
|
Closing this — but I owe a correction to my own comment above, because the reason I'm closing is not the reason I'd have given yesterday. @liuhao1024 — your finding is now satisfied on Upstream landed Verified by executing main's own code rather than reading the diff:
So the outcome is: your objection was upheld, and the feature shipped without the bypass. My I also want to correct something I nearly got wrong. I was initially told this PR was moot because the explicit-toolsets API had been deleted by Thank you for the review. It was specific, it was correct on all four rows of your table, and it caught a real recursion-gate bypass rather than a style nit — the |
Delegated tasks stripped
code_executionand did not honor explicitly-requested toolsets._strip_blocked_toolsnow keepscode_executionwhen explicitly requested and explicit toolsets are honored. Includes test_delegate.py coverage.