Skip to content

feat(delegation): enforce per-child tool allowlists - #89193

Open
rperezga wants to merge 1 commit into
NousResearch:mainfrom
rperezga:feat/delegate-tool-allowlist
Open

feat(delegation): enforce per-child tool allowlists#89193
rperezga wants to merge 1 commit into
NousResearch:mainfrom
rperezga:feat/delegate-tool-allowlist

Conversation

@rperezga

Copy link
Copy Markdown

Summary

Adds an optional exact-name tool_allowlist to delegate_task, both top-level and per-task.

When present, delegated children use deny-all semantics: their final builtin + MCP tool surface is intersected with the listed names. An empty list denies every tool. When absent, behavior is unchanged.

Security boundary

The restriction is enforced in runtime code rather than prompt instructions:

  1. Intersect the fully materialized child snapshot after inheritance, blocked-tool removal, builtin resolution, and MCP resolution.
  2. Reject any model-generated call outside the allowlist in the shared sequential/concurrent execution middleware, before plugins, callbacks, or dispatch.
  3. Reapply the intersection after MCP refresh so late MCP connections cannot reopen capabilities.
  4. Propagate the field through the normal AIAgent._dispatch_delegate_task path and registry fallback.

Rejected calls return a structured result:

{"error":"...","error_type":"tool_allowlist_block","tool":"write_file"}

Compatibility

  • tool_allowlist omitted: existing leaf/orchestrator inheritance remains intact.
  • Per-task values override the top-level value.
  • Unknown names are harmless: the final surface remains an intersection.
  • Duplicate names are normalized; malformed lists fail before any child is spawned.

Tests

scripts/run_tests.sh -j 3 \
  tests/tools/test_delegate.py \
  tests/tools/test_refresh_agent_mcp_tools.py \
  tests/run_agent/test_tool_call_guardrail_runtime.py \
  -q

=== Summary: 3 files, 96 tests passed, 0 failed (100% complete) ===

An isolated runtime exercise built real child AIAgent instances and emitted fabricated model tool calls:

allowlisted final tools=["read_file", "search_files"]
write_file -> tool_allowlist_block
patch -> tool_allowlist_block
terminal -> tool_allowlist_block
mcp__roshhome__update_request -> tool_allowlist_block
filesystem baseline/post identical=true
seed SHA-256 unchanged=true
legacy child tools=["patch", "process", "read_file", "search_files", "terminal", "write_file"]
PASS: runtime veto, MCP intersection, filesystem integrity, legacy regression

No deployment or runtime restart is part of this PR.

@alt-glitch alt-glitch added type/feature New feature or request comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint tool/delegate Subagent delegation tool/mcp MCP client and OAuth P3 Low — cosmetic, nice to have needs-decision Awaiting maintainer decision before any implementation labels Aug 18, 2026

@andrexibiza andrexibiza left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed exact head 34496edd1e408dc2dcb8003bf26ee3fc1301dbd1 against base c1358e45d26c1c340c04659258e582c45630409c, including the full 7-file patch, current delegation inheritance, Tool Search assembly/unwrap behavior, MCP refresh, the prior subagent privilege-escalation class (#3188 salvaged/merged via #3269 with @dieutx authorship preserved), and the adjacent public subagent lifecycle work in #63359.

The runtime veto itself is placed in the right choke point: direct and tool_call-unwrapped model calls reach _run_agent_tool_execution_middleware with the real function name before hooks/dispatch, and MCP refresh reapplies the boundary. But I see two blockers before this can be treated as a hard per-child capability boundary.

1. Nested orchestrators can widen the allowlist again through enabled_toolsets.

_apply_child_tool_allowlist() narrows child.tools, valid_tool_names, _context_engine_tool_names, and stores _tool_allowlist, but deliberately leaves child.enabled_toolsets unchanged. _build_child_agent() still derives a descendant's inherited surface from parent_agent.enabled_toolsets whenever it is non-None; for role="orchestrator", it also re-adds delegation as a role grant. The new schema then lets that orchestrator choose a fresh tool_allowlist for its own children.

That recreates the same privilege-escalation shape #3269 closed at the toolset layer, just one layer lower. Concrete case: root spawns an orchestrator whose exact allowlist is ['delegate_task', 'read_file'], while the inherited toolsets still include file. That child can call delegate_task(..., tool_allowlist=['write_file']); the grandchild is rebuilt from the broad inherited file toolset and the new allowlist leaves write_file exposed. The parent's supposed hard boundary is therefore not a capability ceiling across the delegation tree.

The descendant rule needs to be monotonic: once a parent has a non-None exact-name ceiling, every descendant request must be intersected with that ceiling (and an omitted descendant allowlist should inherit the ceiling, not revert to broad toolset inheritance). I would pin this with a real orchestrator→grandchild regression proving a child restricted to {delegate_task, read_file} cannot mint a grandchild with write_file, terminal, or an MCP write tool.

2. The exact-name boundary is applied after Tool Search assembly, so positive MCP/plugin allowlisting is not production-correct.

model_tools.get_tool_definitions() performs progressive Tool Search assembly as its final step: deferrable MCP/plugin tools are removed from the model-facing list and replaced by tool_search / tool_describe / tool_call. _apply_child_tool_allowlist() runs only after AIAgent has materialized that assembled snapshot. Consequently an allowlist like ['mcp__roshhome__update_request'] does not retain that MCP tool under the normal Tool Search path—the underlying name is no longer present in child.tools, while the bridge names are not in the allowlist, so the allowed capability disappears.

The inverse is also leaky at the exposure layer: if callers add the bridge names so the child can discover deferred tools, handle_function_call() rebuilds the raw bridge catalog from enabled_toolsets / disabled_toolsets, not _tool_allowlist. tool_call execution is still caught because the executor unwraps to the underlying name before the new runtime veto, which is good defense-in-depth; however tool_search / tool_describe can surface names/descriptions/schemas outside the exact allowlist, and the PR's advertised "exposes and executes only the intersection" contract is no longer true.

The current test_allowlist_intersects_final_builtin_and_mcp_snapshot mocks a child whose tools already contains a direct MCP definition, so it bypasses exactly this production assembly path. I think the clean shape is to apply the exact-name filter to the raw definitions before Tool Search assembly (or teach the definition/bridge APIs an exact allowed-name ceiling), then assemble the bridge from only those allowed deferrables; keep the executor veto and refresh-time recheck as defense-in-depth. Please add a real-AIAgent/tool-search regression where one MCP tool is allowed and a sibling MCP tool is denied, proving the allowed one remains discoverable/callable and the denied one is neither discoverable nor executable.

Interlocks / topology: #89193 is complementary to #63359's allowed_toolsets lifecycle surface, not a duplicate; if both land, they should converge on one monotonic capability primitive rather than having plugin-launched and model-launched children inherit authority differently. #3269 is the important prior invariant: descendants may narrow parent authority, never regain authority from a broader representation that survived behind the narrowed surface.

Remote status on this exact head is not green yet: both the CI and Docker workflow runs are currently action_required, so the PR's 96 local focused tests are the only executed evidence visible to me right now.

I would keep this as COMMENT rather than a formal requested-changes state because the architecture is close and the runtime dispatch guard is sound, but the two gaps above are merge blockers for the feature's stated security contract.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint needs-decision Awaiting maintainer decision before any implementation P3 Low — cosmetic, nice to have tool/delegate Subagent delegation tool/mcp MCP client and OAuth type/feature New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants