fix(claude-agent-sdk): grant default tool preset when tools: is omitted#269
Conversation
An agent that omits `tools:` was receiving zero tools instead of the default claude_code preset, so e.g. a "read a file and answer" agent came up with no filesystem tools and failed. Root cause is a lost distinction across the executor↔provider boundary. `resolve_agent_tools(agent.tools, workflow_tools)` returns `workflow_tools.copy()` for an omitted `tools:`, which is `[]` whenever the workflow declares no `runtime` MCP tools (the common case for this provider, which rejects `runtime.mcp_servers` at the factory). The executor therefore always hands the provider a concrete list and never `None`, making `_resolve_tool_config`'s `tools is None -> preset` branch dead code: an omitted `tools:` always hit `if not tools: return [], None` and got no tools. Fix it provider-locally: when the resolved list is empty, consult the raw `agent.tools` field (the only place the omitted-vs-explicit signal survives) to disambiguate. `agent.tools is None` (omitted) grants the claude_code preset with bypassPermissions; explicit `tools: []` still disables all tools; an explicit non-empty allowlist still raises ProviderError. The shared `resolve_agent_tools` is left untouched to avoid affecting the copilot/claude providers. Docstrings and the preset comment are updated to reflect the now-reachable semantics. Adds tests demonstrating the regression (omitted tools -> preset, including end-to-end through AgentExecutor) and an example workflow. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The preceding commit made an omitted `tools:` grant the claude_code preset, but that disambiguation only works when the executor-resolved list is empty. When a workflow declares a non-empty workflow-level `tools:`, an agent that omits `tools:` inherits that list (resolve_agent_tools returns a copy), so it reaches the provider as a non-empty allowlist. The provider then refuses it at execute time — correct, but late and with a misleading message claiming the agent "declares tools=[...]" when it declared none. `conductor validate` passed, so the failure only surfaced mid-run (a realistic footgun when migrating a Copilot workflow that carried a workflow-level `tools:` block). Per user decision, fix by rejecting early rather than honoring or silently re-granting: - Honoring the inherited tools is not viable — workflow tool names do not map to Claude CLI tool IDs (the reason the provider refuses non-empty lists); real passthrough needs a name-translation layer (tracked follow-up). - Granting the preset on omit anyway would silently over-grant (full filesystem/bash/web + bypassPermissions instead of the declared subset). Changes: - validator: error when an agent omits `tools:`, the workflow declares a non-empty `tools:`, and the resolved provider is non-passthrough. Sits beside the existing per-agent allowlist check and keys off each agent's *resolved* provider, so mixed-provider workflows are not over-rejected; explicit `tools: []` stays valid as the opt-out. - claude_agent_sdk: soften the runtime ProviderError to say the agent "resolves to" tools (declared or inherited) instead of "declares" them, and point the suggestion at both the per-agent and workflow-level fields. Kept as defense-in-depth for any path that bypasses validation. - docs/comparison: document the new validate-time rejection. - tests: validator coverage (inherited errors; explicit `[]` and passthrough provider still pass) + updated provider-error match strings. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…e time Follow-up review fixes for the default-tools work on this branch. Per user decision, address findings 1, 2, and 5 from the branch review. validator: the per-agent tools cross-check (explicit-allowlist refusal + the omitted-tools workflow-inheritance footgun) only iterated config.agents, so a for_each group's INLINE agent — which runs at runtime with workflow_tools=config.tools exactly like a top-level agent — slipped past `conductor validate` and only failed mid-iteration with the same confusing runtime error this branch set out to catch early. Extract the two checks into a shared _check_agent_tools helper and run it over for_each inline agents too. Scoped to the tools check; the remaining per-agent capability checks for inline agents are a separate, broader follow-up. (finding 2) claude_agent_sdk: AgentDef always carries `tools`, so the defensive getattr(agent, "tools", None) is now a plain agent.tools, matching how execute() reads its other fields. (finding 5) README: the claude-agent-sdk note claimed workflow-level `tools` and `runtime.mcp_servers` are "ignored". Neither is — mcp_servers is rejected at the factory and a workflow-level `tools:` now errors at validate for any agent that omits `tools:`. Reworded to the actual behavior plus the two correct remediations. (finding 1) tests: TestForEachInlineToolsCrossCheck covers inline omit -> error, inline tools:[] -> pass, inline non-empty list -> error, and inline omit against a passthrough provider -> pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…default-tools-preset
c40fc75 to
54ecd1a
Compare
|
@microsoft-github-policy-service agree |
jrob5756
left a comment
There was a problem hiding this comment.
Thanks so much for contributing to Conductor! I took a pass over this one. Nice fix: it turns a silent zero-tools failure into a loud one at both validate and runtime, and the root-cause write-up in the provider is genuinely useful for the next person who trips over this.
The code itself checks out. I ran the suite (40 pass), validated the new example, and confirmed the footgun gets rejected at validate time and that a mixed-provider workflow only flags the offending agent.
My one real concern is that the regression tests for this fix don't actually run in CI (details inline). The rest are smaller test-coverage and comment notes.
| ) | ||
|
|
||
|
|
||
| class TestOmittedToolsDefaultPreset: |
There was a problem hiding this comment.
Heads up: this module is gated by pytest.importorskip("claude_agent_sdk") (line 11), and CI runs uv sync --group dev without the claude-agent-sdk extra (it lives in [project.optional-dependencies], not the dev group). So every test in this class, plus the end-to-end one below, is skipped in CI, which leaves the actual behavior this PR fixes with no automated guard. A later change that reverts _resolve_tool_config back to if tools is None would stay green.
Installing the extra in the test job would close that:
- run: uv sync --group dev --extra claude-agent-sdkThe skip gate predates this PR, but this is the first change to put load-bearing tests under it, so it seems worth handling here.
There was a problem hiding this comment.
Ah thanks for letting me know! I've changed that now, does the CI automatically rerun on push or do we need to re-trigger to test it?
… accurate Co-authored-by: Jason Robert <jasont.robert@gmail.com>
…rate Co-authored-by: Jason Robert <jasont.robert@gmail.com>
Co-authored-by: Jason Robert <jasont.robert@gmail.com>
…describe what actually happens for omitted tools Co-authored-by: Jason Robert <jasont.robert@gmail.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Addresses review feedback on the reworded tool-refusal error: - test_explicit_non_empty_tools_still_raises now captures the exception and asserts the (also-reworded) `suggestion`, locking in the "set 'tools: []'" escape hatch alongside the message. - Adds test_inherited_workflow_tools_raise_with_inheritance_wording to cover the inheritance path (agent.tools is None + a non-empty resolved list), asserting the new "inherited from the workflow-level" wording. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #269 +/- ##
=======================================
Coverage ? 89.03%
=======================================
Files ? 69
Lines ? 12281
Branches ? 0
=======================================
Hits ? 10934
Misses ? 1347
Partials ? 0 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
jrob5756
left a comment
There was a problem hiding this comment.
Thanks @lukeellison! All review feedback is addressed: the reworded refusal message and its suggestion are now pinned by tests, the inherited-tools path is covered, and the regression tests actually run in CI now that the claude-agent-sdk extra is installed. All checks green. 🚀
Bump version 0.1.19 -> 0.1.20 and finalize the changelog. Changelog (0.1.20): - Added: Hermes provider (#235), cost budget enforcement (#212), external-workflow-friction knobs — output_mode / max_parse_recovery_attempts / gate-respond CLI / Windows paths (#234), templated reasoning.effort & context_tier (#263), scoped applyTo instruction loading (#238). - Fixed: Copilot model attribution for auto-routed runs (#268), claude-agent-sdk default tool preset when tools: omitted (#269). Re-locked uv.lock to record 0.1.20. Quality gates green locally (ruff, ty, pytest excl. real_api/performance: 3673 passed). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Thanks for your help - happy to contribute! 😊 |
Summary
An agent that omits
tools:on theclaude-agent-sdkprovider was receiving zero tools instead of the fullclaude_codepreset. A simple "read a file and answer" agent came up with no filesystem tools and failed.Root cause
The distinction between omitted
tools:(means "all tools") and explicittools: [](means "no tools") was lost at the executor→provider boundary.resolve_agent_tools(agent.tools, workflow_tools)returns an empty list for an omittedtools:whenever the workflow declares no MCP tools — the common case for this provider, which rejectsruntime.mcp_serversat the factory. Both cases therefore arrived at the provider as[], so the provider disabled all tools for an agent that had simply forgotten to declare any.What changed
claude_agent_sdk.py): disambiguate from the rawagent.toolsfield — the only place the omitted-vs-explicit signal survives.agent.tools is None) →claude_codepreset +bypassPermissions(matches the bareclaudeCLI)tools: []→ no tools, no permission bypassconfig/validator.py): a workflow-leveltools:block combined with an agent that omitstools:would inherit a non-empty list at runtime and hit that same refusal mid-run, with a misleading "declares tools=[…]" message.conductor validatenow rejects this early — for both top-level agents andfor_eachinline agents — keyed off each agent's resolved provider, so mixed-provider workflows aren't over-rejected.tools/mcp_serversare "ignored" — they aren't (MCP rejected at the factory, tools rejected at validate). Corrected, plus a newexamples/claude-agent-sdk-repo-qa.yamldemonstrating the fix.Scope
Deliberately tight — this fixes the omitted-tools bug and the directly-coupled validate-time footgun, nothing more.
Out of scope: inline
for_eachagents currently receive only the tools capability cross-check; the other per-agent capability checks (reasoning effort, MCP, output schema, session timeout) remain top-level-only. This was deemed out of scope to keep the PR focused — a follow-up issue should be filed to extend the remaining capability checks to inlinefor_eachagents.Testing
AgentExecutor), explicit[]→ no tools, explicit non-empty → raises.for_eachinline agents; explicit[]and passthrough providers still pass.make checkclean (lint + typecheck); full test suite green under CI markers (-m "not real_api and not performance"); all examples validate.Related
Builds on the experimental-provider capability framework (#241).
🤖 Generated with Claude Code