Skip to content

[lxt] Allow disabling delegation tools#63

Draft
Lixtt wants to merge 1 commit into
AI45Lab:mainfrom
Lixtt:lxt/disable-delegation-tools-env
Draft

[lxt] Allow disabling delegation tools#63
Lixtt wants to merge 1 commit into
AI45Lab:mainfrom
Lixtt:lxt/disable-delegation-tools-env

Conversation

@Lixtt
Copy link
Copy Markdown
Contributor

@Lixtt Lixtt commented Jun 5, 2026

Summary

  • add A3S_CODE_DISABLE_DELEGATION_TOOLS to skip registering task and parallel_task
  • keep agent registry loading so session worker definitions remain available
  • add parser unit tests for common truthy and falsey values

Validation

  • /mnt/shared-storage-user/lixiangtian/.cargo/bin/cargo fmt --check
  • /mnt/shared-storage-user/lixiangtian/.cargo/bin/cargo test -p a3s-code-core disable_delegation_tools -- --nocapture

Copy link
Copy Markdown
Contributor

@ZhiXiao-Lin ZhiXiao-Lin left a comment

Choose a reason for hiding this comment

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

Architecture review — A3S_CODE_DISABLE_DELEGATION_TOOLS

TL;DR: The implementation is correct and closes a real gap, but the mechanism — a process-global env var read directly in the capability-wiring layer — diverges from how this crate models configuration. I'd reshape it as a typed option before merging (or at minimum land it as an explicitly-scoped ops kill-switch with docs + an integration test).

What's right

  • Gating placement is correct. Returning early before register_task_with_mcp is the only registration site for both task and parallel_task, so neither reaches tool_executor.definitions() and the model can't emit them.
  • It composes with auto-delegation for free. build_auto_delegation_plan already guards on registry().contains("task") / ("parallel_task") (agent/auto_delegation.rs), so disabling the tools turns auto-delegation into a clean no-op — no dangling delegation path, no panic.
  • The stated invariant holds. Agents and workers are loaded before the early return, so session.agent_registry stays populated and register_worker_agent keeps working downstream.
  • The parser is solid and well unit-tested (trim + case-insensitive + the right truthy/falsey set).

Main concern — configuration shape

This is the first direct std::env::var business-logic read in core/src. Everywhere else, configuration is typed (CodeConfig, AutoDelegationConfig) and env only enters through the HCL env() bridge in config/loader.rs, then flows via SessionOptions builders. Reading a raw env var inside register_task_capability bypasses that pipeline and brings the usual downsides:

  • Not per-session. AutoDelegationConfig / SessionOptions::with_auto_delegation_enabled are already per-session; a process-global env var can't express "tenant A off, tenant B on" and silently overrides the typed path for every session in the process.
  • Not introspectable. The boolean is computed and discarded — nothing on the session/config records "delegation tools disabled", so audit/debug/SDK callers can't query it.
  • Not persist/resume-stable. Because the flag lives in ambient process env rather than SessionData, resuming a session re-reads whatever the env happens to be now; flip it between save and resume and the resumed session silently gets a different toolset than when it was created.
  • Hard to test without flakiness. std::env::set_var is process-global and Rust tests run in parallel — which is likely why this PR only unit-tests the parser and not the actual gating behavior.

To be clear, the feature itself is justified: auto_delegation.enabled = false only stops automatic delegation — the manual task/parallel_task tools stay registered (config/mod.rs even documents this). So there genuinely was no way to remove the manual tools. It's the delivery mechanism I'd change, not the goal.

Suggested shape

Make the typed layer the source of truth and (optionally) keep an env var as an ops override that feeds it — mirroring how auto_parallel already overrides AutoDelegationConfig.auto_parallel:

  1. Add e.g. AutoDelegationConfig.allow_manual_delegation: bool (default true), or SessionOptions::with_delegation_tools_enabled(bool).
  2. Gate register_task_with_mcp on that field.
  3. Optionally let the HCL loader read env("A3S_CODE_...") into it.

That restores per-session control, introspection, resume-stability, and testability in one move.

Secondary

  • Naming: prefer positive framing to match auto_delegation.enabledallow_* / ENABLE_* reads better than a DISABLE_* name combined with truthy values (double negative).
  • Tests: add an integration test asserting the session's tool set excludes task/parallel_task while agent_registry stays populated. The pattern already exists (e.g. test_session_initializes_without_legacy_agentic_tools).
  • Docs: README/CHANGELOG aren't updated; per repo policy the feature isn't "done" until they are. Please also note the intended use (cost control / debugging) and an explicit caveat that this is not a security sandbox — the parent agent can still call bash, MCP tools, skills, etc. Disabling delegation does not confine it.
  • Dead weight when off: with the tools gone, the still-populated registry is only reachable for introspection / worker registration, not invocation. That's fine, but a one-line comment on why it's intentionally kept would help future readers.

Happy to help reshape this into the typed-option form if useful.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants