Skip to content

feat(delegate): profile toolset isolation + MCP bypass for no_mcp orchestrators#35526

Closed
davidgut1982 wants to merge 7 commits into
NousResearch:mainfrom
davidgut1982:feat/delegation-profile-toolset-isolation
Closed

feat(delegate): profile toolset isolation + MCP bypass for no_mcp orchestrators#35526
davidgut1982 wants to merge 7 commits into
NousResearch:mainfrom
davidgut1982:feat/delegation-profile-toolset-isolation

Conversation

@davidgut1982

@davidgut1982 davidgut1982 commented May 30, 2026

Copy link
Copy Markdown
Contributor

What

Implements per-profile toolset isolation for delegated sub-agents. Profile-declared toolsets are now the single source of truth; batch-mode model-supplied toolsets are rejected when a profile is set, parent MCP context does not leak into children via inherit_mcp_toolsets, and toolset lists are deep-copied to prevent config aliasing. Four surgical fixes with dedicated tests: profile name wiring, batch injection blocking, config aliasing fix, and parent MCP bleed guard. Also fixes per-logger levels from config so DEBUG logging reaches output.

Files modified: tools/delegate_tool.py (profile wiring, injection block, copy fix, inherit guard), tests/tools/test_delegate_toolset_scope.py (new), config/agent_cfg.py (logger level application).

Why

The orchestrator pattern (no_mcp parent routing work to MCP-capable child agents) requires toolset boundaries. Before this PR, three gaps allowed boundary violations: batch-mode injection, parent MCP bleed, and config aliasing. This hardens the security model so delegation is safe at scale.

Tests

pytest tests/tools/test_delegate_toolset_scope.py -v

4 test classes pass covering all security hardening scenarios; 9 adversarial E2E scenarios confirmed no privilege escalation.

Platforms tested

Linux (CT/LXC environment, Python 3.13)

@alt-glitch alt-glitch added type/feature New feature or request P3 Low — cosmetic, nice to have tool/delegate Subagent delegation comp/tools Tool registry, model_tools, toolsets tool/mcp MCP client and OAuth labels May 30, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

Competing with #32727 — both implement profile-based MCP bypass for delegate_task and close #32668. #32727 also touches gateway/run.py, hermes_cli/main.py, and mcp_tool.py (broader scope). This PR adds batch injection blocking, aliasing prevention, and inherit_mcp guard (narrower but deeper). Maintainer should pick one.

@davidgut1982

Copy link
Copy Markdown
Contributor Author

Thanks @alt-glitch — agreed. I'll consolidate onto #35526 and fold in #32727's lazy MCP discovery (mcp_tool.py ensure_mcp_discovered/mark_eager_discovery_skipped + gateway/run.py and hermes_cli/main.py wiring) so nothing's lost, then close #32727. Will push the combined change shortly.

davidgut1982 and others added 5 commits May 31, 2026 13:33
… toolsets bypass no_mcp parent intersection (NousResearch#32668/NousResearch#32727)

Under a no_mcp orchestrator (platform_toolsets: [no_mcp, delegation,
knowledge]), expanded_parent has zero MCP entries. The previous call site
hardcoded profile_name=None into _build_child_agent, so the MCP-bypass
branch (lines 972-976 of delegate_tool.py) never activated and fat
sub-agents received an empty toolset.

Fix:
- Add _load_agent_profiles() helper to read agent_profiles from the
  top-level config (not the delegation sub-key that _load_config() returns).
- Add profile: Optional[str] param to delegate_task(); when set, resolve
  the named profile's toolsets from agent_profiles and store as the
  effective toolsets for the child.
- Change the _build_child_agent call site from profile_name=None to
  profile_name=resolved_profile_name so the MCP-bypass branch activates
  for named profiles.
- Add "profile" to DELEGATE_TASK_SCHEMA so the model reliably emits the
  field; without a formal schema entry the LLM strips it at the provider
  API boundary.
- Update registry lambda and _dispatch_delegate_task in run_agent.py to
  forward profile= through both invocation paths.

Security: the bypass is scoped to MCP toolsets only, and only when a
named profile explicitly declares them in config. Non-MCP toolsets still
go through the parent intersection. Unknown profile names fall back
gracefully (warning + no bypass).

Tests added (test_delegate_toolset_scope.py):
- TestDelegateTaskProfileWiring: verifies delegate_task() calls
  _build_child_agent with profile_name='documents' and the profile's
  resolved toolsets. Key regression guard: FAILS against pre-fix code
  (profile_name=None hardcoded) and PASSES after the fix.
- TestDelegateTaskSchemaProfile: asserts 'profile' is a declared string
  property in DELEGATE_TASK_SCHEMA and is not in required[].
- TestProfileMcpBypassEndToEnd: direct _build_child_agent tests covering
  the post-fix bypass (profile_name set → mcp-nextcloud-files retained)
  and the security baseline (profile_name=None → MCP stripped).

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…toolset injection (security)

Two privilege-escalation vectors closed:

1. Batch-mode injection (primary fix): the batch loop previously used
   `t.get("toolsets") or toolsets` for each task, so a model could name
   a valid profile (activating the mcp-* bypass in _build_child_agent)
   while supplying per-task toolsets that the profile never declared.
   Fix: when resolved_profile_name is set, all tasks in the batch receive
   profile_resolved_toolsets exclusively — per-task model-supplied toolsets
   are ignored.

2. Empty-profile bypass (secondary fix): a profile with no/empty toolsets
   still set resolved_profile_name, activating the mcp-* bypass with
   whatever caller-supplied toolsets were present. Fix: resolved_profile_name
   is only set when the profile declares a non-empty toolsets list; otherwise
   a warning is logged and the normal intersection path is used.

Tests added (tests/tools/test_delegate_toolset_scope.py):
- test_batch_injection_blocked_model_cannot_inject_evil_mcp_toolset: FAILS
  pre-fix (mcp-injected-evil present), PASSES post-fix
- test_single_task_profile_toolsets_unchanged: regression guard, PASSES both
- test_empty_profile_toolsets_bypass_not_activated: FAILS pre-fix
  (profile_name set), PASSES post-fix

Total: 16 → 19 tests, all green. tool_search: 63 pass, no collateral.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…l security hardening)

Assign list(profile_toolsets) instead of the bare reference at line ~2082
so in-place mutations of the working `toolsets` variable cannot corrupt
agent_profiles config for the process lifetime.  Also take an independent
list(toolsets) copy for profile_resolved_toolsets at line ~2110 (defence-
in-depth against future code inserted between the two assignments).

Adds TestProfileToolsetsAliasing::test_profile_toolsets_copy_prevents_config_corruption:
resolves a profile, mutates the child toolsets list returned to _build_child_agent,
and asserts the original profiles["documents"]["toolsets"] config list is unchanged.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…rent MCP bleed (security hardening)

When a named agent_profile is used, the profile's declared toolsets are
authoritative. Previously _preserve_parent_mcp_toolsets ran unconditionally
when inherit_mcp_toolsets=True, silently appending every parent MCP toolset
absent from the child's list — including ones the profile never declared.

Guard the bleed path with `and not profile_name` so inheritance is skipped
whenever delegation resolves through a profile. Non-profile (ad-hoc)
delegation continues to inherit parent MCP toolsets unchanged.

The variable `profile_name` is already in scope at the call site (function
parameter, also used at line 972 for the existing MCP bypass guard).

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…G reaches the log

_read_logging_config now returns the loggers dict (4th element).
setup_logging iterates logging.loggers.<name> entries after attaching
handlers and calls logging.getLogger(name).setLevel() for each valid
entry.  Both bare-string ("DEBUG") and dict ({level: "DEBUG"}) shapes
are accepted; invalid levels are silently skipped.

When a per-logger override is finer-grained than the configured
top-level (e.g. DEBUG while agent.log runs at INFO), the root logger
and the agent.log handler are also lowered to the minimum per-logger
level so that propagated records actually reach the file.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@davidgut1982 davidgut1982 force-pushed the feat/delegation-profile-toolset-isolation branch from c8d1948 to 74ceaa5 Compare May 31, 2026 13:33
davidgut1982 and others added 2 commits May 31, 2026 13:54
When the active platform includes the no_mcp sentinel in its toolsets,
skip eager MCP server discovery at gateway/CLI startup. Discovery is
deferred until the first delegate_task() call that targets an MCP toolset,
using a thread-safe one-shot Event/Lock pattern.

This eliminates unnecessary MCP connection overhead for api_server platform
(orchestrator) while preserving full MCP access for child agents via the
Phase 1 profile_name bypass. cli/cron/telegram platforms are unaffected:
their toolsets lack no_mcp, so the gate evaluates False and eager discovery
runs exactly as before.

Changes:
- tools/mcp_tool.py: add mark_eager_discovery_skipped() + ensure_mcp_discovered()
  (one-shot, thread-safe, failure-tolerant lazy discovery trigger)
- gateway/run.py: add _active_platform_uses_no_mcp() helper; gate the eager
  discover_mcp_tools() call in start_gateway() on the platform no_mcp check
- hermes_cli/main.py: gate the inline CLI-startup discover_mcp_tools() call in
  _prepare_agent_startup() with the same no_mcp check (covers `gateway run`)
- tools/delegate_tool.py: call ensure_mcp_discovered() before building any
  child agent that requests MCP toolsets
- tests/tools/test_mcp_lazy_discovery.py: 12 tests covering the skip flag,
  no-op/once/idempotent/thread-safe/failure paths, and platform resolution

Part of fix/profile-mcp-toolset-bypass branch (stacked on Phase 1).
Resolves: NousResearch#32668

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Pre-declare _cfg as Dict[str, Any] = {} so it is always bound and
Pyright knows the type is dict throughout the config bridge block.

Use an intermediate _expanded variable after _expand_env_vars() and
guard with isinstance(_, dict) so the re-assignment stays within the
declared dict type — _expand_env_vars has no return annotation and
Pyright infers a broad str | list | dict union.

Simplify the IPv4 network_cfg line: now that _cfg is always bound and
typed as dict, the old ('_cfg' in dir() else {}) guard is unnecessary.

Fixes Pyright errors at lines 863, 901, 917, 926, 930, 936, 978 that
were introduced by the Phase 2 lazy MCP discovery work.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@davidgut1982

Copy link
Copy Markdown
Contributor Author

Lazy MCP discovery from #32727 folded in (this PR is now the complete superset)

Folded the lazy MCP discovery work from fix/profile-mcp-toolset-bypass (#32727) into this branch via cherry-pick of:

  • 478e5380a feat: lazy MCP discovery for no_mcp platforms (Phase 2)
  • a3f2a1985 fix: resolve Pyright type errors in _cfg config bridge block

What was added to this branch:

  • tools/mcp_tool.py: _lazy_discovery_done (Event), _lazy_discovery_lock (Lock), mark_eager_discovery_skipped(), and ensure_mcp_discovered() (thread-safe double-checked-locking lazy trigger)
  • gateway/run.py: mark_eager_discovery_skipped() call at the no-MCP startup path (via _active_platform_uses_no_mcp() helper)
  • hermes_cli/main.py: Same wiring in the CLI startup path via _active_platform_uses_no_mcp_at_startup() guard, integrated with the existing elaborate startup dispatch (_is_tui_chat_launch / _command_has_dedicated_mcp_startup / _should_background_mcp_startup)
  • tests/tools/test_mcp_lazy_discovery.py: 12 tests (all passing)

Test results (run locally on this branch):

  • tests/tools/test_mcp_lazy_discovery.py: 12/12 passed
  • tests/tools/test_delegate_toolset_scope.py: 22/22 passed
  • Import check: from tools.mcp_tool import ensure_mcp_discovered, mark_eager_discovery_skipped resolves cleanly

This PR (#35526) is now the complete implementation closing #32668. #32727 (fix/profile-mcp-toolset-bypass) will be closed as superseded once this PR merges — it is intentionally left open pending that merge.

@davidgut1982

Copy link
Copy Markdown
Contributor Author

Closing this PR — the multi-agent gateway stack (feat/mga-1 through feat/mga-6, merged via #37495#37502) introduced a competing implementation of delegation profile/toolset isolation using _profile_overrides in bbcb155b0. The two approaches conflict in tools/delegate_tool.py and are not additive.

The mga stack's implementation covers the core isolation requirement. Rather than maintain two diverging approaches, this PR is being retired. If the specific security hardening cases here (batch injection blocking, parent MCP bleed guard, config aliasing deep-copy) are not covered by the mga implementation, they should be re-submitted as targeted fixes on top of the mga stack rather than as a competing full rewrite.

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

Labels

comp/tools Tool registry, model_tools, toolsets 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.

2 participants