[lxt] Allow disabling delegation tools#63
Conversation
ZhiXiao-Lin
left a comment
There was a problem hiding this comment.
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_mcpis the only registration site for bothtaskandparallel_task, so neither reachestool_executor.definitions()and the model can't emit them. - It composes with auto-delegation for free.
build_auto_delegation_planalready guards onregistry().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_registrystays populated andregister_worker_agentkeeps 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_enabledare 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_varis 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:
- Add e.g.
AutoDelegationConfig.allow_manual_delegation: bool(defaulttrue), orSessionOptions::with_delegation_tools_enabled(bool). - Gate
register_task_with_mcpon that field. - 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.enabled—allow_*/ENABLE_*reads better than aDISABLE_*name combined with truthy values (double negative). - Tests: add an integration test asserting the session's tool set excludes
task/parallel_taskwhileagent_registrystays 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.
Summary
Validation