Skip to content

fix(claude-agents): do not enable built-ins the variation omits - #273

Open
apucacao wants to merge 2 commits into
mainfrom
cursor/claude-empty-tools-be6d
Open

apucacao wants to merge 2 commits into
mainfrom
cursor/claude-empty-tools-be6d

Conversation

@apucacao

@apucacao apucacao commented Nov 4, 2026

Copy link
Copy Markdown
Contributor

_partition_tools enabled every NativeTool in tool_handlers, so a variation with a missing or empty tool list still got Bash, Read, and the rest of the registered built-ins. User-defined tools were already limited to keys on the variation.

A key the variation does not declare is no longer offered. tools=[] is still sent when nothing is attached, which is what turns the Claude Agent SDK's default built-in set off. The skills handler still adds Skill when the variation names a skill.

OpenCode and the JS claude-agents handler already did this. Native graph goes through the same partition, so it follows the same rule.

Open in Web Open in Cursor 

cursoragent and others added 2 commits November 4, 2026 15:21
_partition_tools treated every NativeTool in tool_handlers as offered, so an empty or missing AI Config tool list still turned on Bash, Read, and the rest. User-defined tools were already gated on the variation's keys. A key the variation does not declare is not offered.

Co-authored-by: alexis <alexis@launchdarkly.com>
Co-authored-by: alexis <alexis@launchdarkly.com>
@apucacao
apucacao requested a review from jeffdupont November 4, 2026 15:56

@jeffdupont jeffdupont 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.

The fix is right. A built-in now reaches tools only when the variation attaches its key, for create_claude_agents_handler() and for every node to_claude_agents() runs. This matches partitionTools in launchdarkly/js-ai-sdk's claude-agents handler. I checked each variation shape: no tools, an empty tool map, only user-defined tools, some or all of the registered built-ins, and a key with no handler. All of them come out right, and Skill is still added when the variation has skills. _native_tool_aliases is still unfiltered, but ToolCatalog only walks config.tools, so a built-in the variation doesn't attach never reaches the span catalog. The claude-agents tests pass locally on 12b148c (304 passed) and ruff is clean.

1. Blocking: the README still describes the old behaviour (packages/claude-agents/README.md:189 and the example under it). It says to "place them as values in tool_handlers to enable the corresponding native Claude tool", and the example registers {"web-search": ClaudeWebSearch} without saying the variation must attach web-search. Anyone who follows it after this change gets no WebSearch. Fix: say that the built-in is enabled only when the variation's tools include that key, and note it next to the example. I couldn't leave this inline because the README isn't in the diff.

2. Non-blocking: call out the behaviour change in the release notes. Today, registering a NativeTool enables that built-in on every variation. After this change, it's enabled only on variations that attach it. Anyone who relied on the old behaviour loses the tool with no warning, and a plain fix: commit puts that in the changelog as a quiet patch. Fix: add a sentence to the squash commit body, e.g. "Registering a NativeTool no longer enables it by itself; the variation must attach its key." Use fix! if you want it to count as breaking.

One inline comment on a missing test case.

config,
handlers,
)
assert omitted.tools == []

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.

Non-blocking: these cases cover all-attached and none-attached, but not the one this fix is for: some registered built-ins attached and others not. Please add a variation that attaches only read, keep both read and bash in handlers, and assert opts.tools == ["Read"]. That's the test that catches a regression back to enabling everything registered.

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.

3 participants