Make builtin tools opt-in (ask_user)#1557
Conversation
Add an opt-in mechanism for built-in tools. Currently ask_user is the only tool that can be enabled using this feature. This changes things so that ask_user will be disabled by default, which is probably good because ask_user can cause problems for agents that are not running within the kagent UI or which are sub-agents. Signed-off-by: Dobes Vandermeer <dobes.vandermeer@newsela.com>
Signed-off-by: Dobes Vandermeer <dobesv@gmail.com>
Signed-off-by: Dobes Vandermeer <dobes.vandermeer@newsela.com>
EItanya
left a comment
There was a problem hiding this comment.
Ok, I definitely think this is a good use-case. However I have a couple of notes:
- I think these should be turned on by default, and if this new struct is present it will selectively turn them on.
- Given that these are all well-known, what do you think about a struct that actually has them explicitly named
"builtin_tools": {
"ask_user_question'': true,
}
etc.
Hmm ... personally with kagent in particular I want the full control to have everything off by default. But as long as I can turn things off it's OK. Currently there is only one built-in tool and it is a breaking change to enable it because it won't necessarily work with non-kagent-ui A2A clients and is a bit glitchy sometimes, so I definitely think it should be disabled by default.
I'm not totally sure I understand the proposal here, maybe I need some more context around it. The PR uses a list that you embed into your existing tools which seems more compact and clean than how I'm interpreting your suggestion: So I think it's nice to have it in the same section as the other tools and you might as well just use a list since it's a whitelist. If you do really want to have it where some tools are enabled by default, it could work so that if there's no "BuiltIn" element then it uses a default list of built-in tools, and to disable them someone would provide an empty array, like: Were you suggesting a format like: |
There was a problem hiding this comment.
Pull request overview
This PR implements an opt-in mechanism for built-in tools, making the ask_user tool opt-in instead of unconditionally enabled by default. This addresses the issue where agents that don't run within the kagent UI or are sub-agents would fail when trying to use the ask_user tool.
Changes:
- Added
Builtinas a newToolProviderTypeto the Tool schema across Go API, Python ADK, TypeScript types, and CRDs - Created a
BuiltinToolstruct to hold a list of enabled built-in tool names - Modified the Python agent configuration to only add
ask_usertool when explicitly enabled viabuiltin_toolsconfig - Updated the Go translator to pass builtin tool configurations to the agent config
- Added UI components to allow selection and management of built-in tools in the agent creation flow
- Added comprehensive tests for the feature at both unit and e2e levels
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| ui/src/types/index.ts | Added Builtin to ToolProviderType union and new BuiltinTool interface |
| ui/src/lib/toolUtils.ts | Added utility functions for builtin tool detection and display |
| ui/src/components/create/ToolsSection.tsx | Added UI rendering for builtin tools in the tool selection UI |
| ui/src/components/create/SelectToolsDialog.tsx | Implemented dialog for selecting and managing builtin tools |
| python/packages/kagent-adk/src/kagent/adk/types.py | Added builtin_tools field to AgentConfig and conditional logic to enable ask_user |
| python/packages/kagent-adk/tests/unittests/test_builtin_tools.py | Comprehensive unit tests covering all builtin tool scenarios |
| go/api/v1alpha2/agent_types.go | Added BuiltinTool type and validation rules to Tool struct |
| go/core/internal/controller/translator/agent/adk_api_translator.go | Added logic to translate builtin tools to agent config |
| go/core/internal/controller/reconciler/reconciler.go | Added no-op case for builtin tool validation |
| go/core/cli/internal/tui/workspace.go | Added display of builtin tools in CLI |
| helm/kagent-crds/templates/kagent.dev_agents.yaml | Updated CRD with builtin tool validation rules |
| go/api/config/crd/bases/kagent.dev_agents.yaml | Updated CRD base with builtin tool schema |
| go/api/adk/types.go | Added BuiltinTools field to AgentConfig Go struct |
| go/api/v1alpha2/zz_generated.deepcopy.go | Auto-generated deepcopy methods for BuiltinTool |
| go/core/test/e2e/builtin_tool_test.go | E2E tests validating builtin tools in agent config |
| Test data files | Golden test input/output files for builtin tool configuration |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
EItanya
left a comment
There was a problem hiding this comment.
Thanks for the PR — the use-case is solid and I like the direction of making ask_user opt-in. However, I think the API shape needs rethinking before we merge this.
Core concern: built-in tools aren't tool providers
The current approach adds Builtin as a new variant in the Tool discriminated union alongside McpServer and Agent. But these are fundamentally different things:
McpServerandAgentare external providers that need connection info (names, namespaces, tool selection)- Built-in tools are intrinsic agent capabilities — they don't reference external resources and don't need provider-style configuration
Shoehorning them into the tools array creates several issues:
-
Stringly-typed —
toolNames: ["ask_user"]relies on a CRD enum for validation. Since built-in tools are a small, well-known, finite set, they should be explicit struct fields with proper types. -
Overly verbose — 4 lines of YAML to flip what's essentially a boolean:
- type: Builtin builtin: toolNames: - ask_user
-
No room for per-tool config — If
ask_userlater needs a timeout, or a future built-in tool needs its own settings, a flat[]stringcan't accommodate that. Explicit struct fields can grow naturally. -
Consumes the
maxItems: 20tools limit — ABuiltinentry takes a slot meant for real external tool providers. -
Wrong level of abstraction — This is an agent-level capability toggle, not a tool provider binding.
Suggested alternative
Move this to a dedicated field on the agent spec:
type DeclarativeAgentSpec struct {
// ...existing fields...
BuiltinTools *BuiltinToolsConfig `json:"builtinTools,omitempty"`
}
type BuiltinToolsConfig struct {
AskUser *bool `json:"askUser,omitempty"`
// future built-in tools get their own fields here
}spec:
declarative:
builtinTools:
askUser: trueThis is more Kubernetes-idiomatic (explicit fields over string lists), keeps the tools array for actual external providers, and gives each built-in tool room for future per-tool configuration.
Happy to discuss further — the feature itself is definitely something we want, just want to get the API right.
Comment left by Claude on behalf of @EItanya
|
OK, I'll have to come back this another time. |
Add an opt-in mechanism for built-in tools. Currently ask_user is the only tool that can be enabled using this feature.
This changes things so that ask_user will be disabled by default, which is probably good because ask_user can cause problems for agents that are not running within the kagent UI or which are sub-agents.
Fixes #1554