diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 95360a67..61b3b37c 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -101,7 +101,10 @@ jobs: run: uv python install ${{ matrix.python-version }} - name: Install dependencies - run: uv sync --group dev + # Include the claude-agent-sdk extra so the provider's regression tests + # (gated by `pytest.importorskip("claude_agent_sdk")`) actually run in + # CI instead of silently skipping. + run: uv sync --group dev --extra claude-agent-sdk - name: Remove bundled Copilot CLI binary run: | diff --git a/README.md b/README.md index cada0bb1..b80b1fd3 100644 --- a/README.md +++ b/README.md @@ -241,7 +241,7 @@ workflow: Requires the `claude` CLI to be installed and authenticated. Install the SDK: `uv add 'claude-agent-sdk>=0.1.0'` -> **Note:** The `claude-agent-sdk` provider delegates tool and MCP server management to the `claude` CLI. Workflow-level `tools` and `runtime.mcp_servers` fields are ignored — configure these through your Claude Code settings instead. +> **Note:** The `claude-agent-sdk` provider delegates tool and MCP management to the `claude` CLI; workflow-level tool/MCP config is **not** bridged into it. `runtime.mcp_servers` is rejected at the factory, and a workflow-level `tools:` block is rejected at `conductor validate` for any agent that omits `tools:` (it would otherwise inherit a list the CLI can't map). Omit `tools:` to grant the full `claude_code` preset, set an agent's `tools: []` to disable all tools, and configure MCP servers through your Claude Code settings instead. **See also:** [Claude Documentation](docs/providers/claude.md) | [Provider Comparison](docs/providers/comparison.md) | [Migration Guide](docs/providers/migration.md) diff --git a/docs/providers/comparison.md b/docs/providers/comparison.md index b61e2a38..237fed18 100644 --- a/docs/providers/comparison.md +++ b/docs/providers/comparison.md @@ -153,7 +153,8 @@ The `claude-agent-sdk` provider does not bridge workflow-level tools/MCP into th - `runtime.mcp_servers` — **rejected at the factory** with a clear error. Translation to the CLI's MCP configuration is not implemented. Configure MCP servers through your Claude Code settings instead. - Per-agent `tools: []` — disables all tools for that agent. - Per-agent `tools: [list]` — **refused loudly**. Workflow tool names do not translate to Claude CLI tool IDs; silently passing them through would risk granting the wrong native tool. -- Omitting `tools:` entirely — grants the full `claude_code` preset (filesystem, bash, web), matching the bare `claude` CLI experience. +- Workflow-level `tools:` combined with an agent that omits `tools:` — **rejected at `conductor validate`**. The agent would otherwise inherit that non-empty list at runtime and hit the same refusal with a confusing message. Remove the workflow-level `tools:` (so omitting `tools:` grants the preset) or set the agent's `tools: []`. +- Omitting `tools:` entirely (with no workflow-level `tools:`) — grants the full `claude_code` preset (filesystem, bash, web), matching the bare `claude` CLI experience. - `temperature` and `max_tokens` are **rejected at the factory** — sampling behavior is controlled by the CLI. ### Example Claude Agent SDK Workflow diff --git a/examples/claude-agent-sdk-repo-qa.yaml b/examples/claude-agent-sdk-repo-qa.yaml new file mode 100644 index 00000000..0e43d590 --- /dev/null +++ b/examples/claude-agent-sdk-repo-qa.yaml @@ -0,0 +1,77 @@ +# Repo Q&A with the claude-agent-sdk provider (default tools preset) +# +# This single-agent workflow asks the claude-agent-sdk provider to READ a file +# in its working directory and answer a question about it. The agent does NOT +# declare a `tools:` list — it relies on the provider's default behavior. +# +# WHY THIS EXAMPLE EXISTS (the bug it guards against): +# Before the fix, an agent that omitted `tools:` received NO tools and could +# not read the file: the executor's resolve_agent_tools() turned the omitted +# `tools:` into an empty list (this workflow declares no workflow-level `tools:` to +# inherit), and the provider could not tell that empty list apart from an +# explicit `tools: []`, so it disabled every tool. The agent came up with +# ZERO filesystem tools and failed to read the file. +# +# After the fix, the provider inspects the raw `agent.tools` field: an OMITTED +# `tools:` (agent.tools is None) now grants the full `claude_code` preset +# (Read / Bash / Edit / Glob / Grep / WebFetch / ...), exactly like the bare +# `claude` CLI. An explicit `tools: []` still disables all tools. +# +# Pre-requisites: +# pip install conductor[claude-agent-sdk] # or: uv add 'claude-agent-sdk>=0.1.64' +# npm install -g @anthropic-ai/claude-code # the `claude` CLI +# claude login +# +# Usage (run from the repo root so README.md is in the working directory): +# conductor run examples/claude-agent-sdk-repo-qa.yaml \ +# --input question="In one sentence, what is Conductor?" +# +# Validation only (no execution / no API calls): +# conductor validate examples/claude-agent-sdk-repo-qa.yaml + +workflow: + name: claude-agent-sdk-repo-qa + description: > + Single claude-agent-sdk agent that reads a file in its working + directory and answers a question about it. Demonstrates the default + claude_code tool preset granted when `tools:` is omitted. + version: "1.0.0" + entry_point: repo_reader + + runtime: + provider: claude-agent-sdk + # claude-sonnet-4-5 is the SDK's documented default; adjust to whatever + # your Claude Code backend (anthropic.com / Vertex AI / Bedrock) expects. + default_model: claude-sonnet-4-5 + + input: + question: + type: string + required: true + description: A question to answer using a file in the working directory. + +agents: + - name: repo_reader + description: Read README.md in the working directory and answer the question. + # NOTE: there is intentionally NO `tools:` field here. Omitting it grants + # the full `claude_code` preset (Read/Bash/Edit/...). Before the fix this + # agent received no tools and could not read the file; after the fix it can. + prompt: | + Read the file README.md in your current working directory, then answer + the following question using only what that file says. Cite the relevant + phrase from the file. + + Question: {{ workflow.input.question }} + output: + answer: + type: string + description: The answer, grounded in README.md. + evidence: + type: string + description: A short quote from README.md that supports the answer. + routes: + - to: $end + +output: + answer: "{{ repo_reader.output.answer }}" + evidence: "{{ repo_reader.output.evidence }}" diff --git a/src/conductor/config/validator.py b/src/conductor/config/validator.py index e5de78c4..512fcfd9 100644 --- a/src/conductor/config/validator.py +++ b/src/conductor/config/validator.py @@ -1597,6 +1597,38 @@ def _caps_for(name: str) -> ProviderCapabilities | None: cache[name] = None # type: ignore[assignment] return cache.get(name) + def _check_agent_tools(agent: AgentDef, provider_name: str, caps: ProviderCapabilities) -> None: + """Tools-capability cross-check shared by top-level and for_each agents. + + Two failure modes against a non-passthrough provider: + + * Explicit non-empty ``tools:`` — a declared allowlist the provider + cannot honor; silently granting different tools is a security + regression. + * Omitted ``tools:`` + non-empty workflow-level ``tools:`` — the agent + inherits that list at runtime (``resolve_agent_tools`` returns a copy) + and hits the same refusal mid-run (now a ``resolves to tools=[...]`` + ``ProviderError``) rather than failing fast at validate. An explicit + ``tools: []`` is the "no tools" opt-out and stays valid. + """ + if agent.tools and not caps.workflow_tools_passthrough: + errors.append( + f"Agent '{agent.name}' declares tools={agent.tools!r} but provider " + f"'{provider_name}' does not honor per-agent tool allowlists " + f"(capabilities.workflow_tools_passthrough=False). Silently " + f"granting different tools than declared is a security regression." + ) + elif agent.tools is None and config.tools and not caps.workflow_tools_passthrough: + errors.append( + f"Agent '{agent.name}' omits 'tools:' and would inherit the " + f"workflow-level tools={config.tools!r}, but provider " + f"'{provider_name}' does not honor tool allowlists " + f"(capabilities.workflow_tools_passthrough=False). Remove the " + f"workflow-level 'tools:' so omitting 'tools:' grants the " + f"provider's default tool preset, or set this agent's " + f"'tools: []' to disable all tools." + ) + # ----- Workflow-level: MCP servers ----- # An mcp_servers block applies only to provider-backed agents that # actually resolve to a provider lacking MCP support. If every LLM @@ -1677,17 +1709,10 @@ def _caps_for(name: str) -> ProviderCapabilities | None: f"(capabilities.mcp_tools=False)." ) - # tools allowlist: any explicit non-empty list against a provider - # that doesn't pass through is a security regression risk. An empty - # list ("no tools") is fine — the provider may honor it as - # "no tools" semantics. Only non-empty triggers the security concern. - if agent.tools and not caps.workflow_tools_passthrough: - errors.append( - f"Agent '{agent.name}' declares tools={agent.tools!r} but provider " - f"'{provider_name}' does not honor per-agent tool allowlists " - f"(capabilities.workflow_tools_passthrough=False). Silently " - f"granting different tools than declared is a security regression." - ) + # tools allowlist (explicit non-empty list) and the omitted-tools + # inheritance footgun against non-passthrough providers. Shared with + # the for_each inline-agent pass below. + _check_agent_tools(agent, provider_name, caps) # reasoning.effort: validate per-agent override OR workflow-wide # default against the supported levels tuple. Per-agent override @@ -1743,6 +1768,24 @@ def _caps_for(name: str) -> ProviderCapabilities | None: f"(capabilities.max_session_seconds=False)." ) + # ----- For-each inline agents: tools capability cross-check ----- + # A for_each group carries an INLINE ``AgentDef`` (not in ``config.agents``) + # that runs with ``workflow_tools=config.tools``, exactly like a top-level + # agent. The per-agent loop above skips it, so re-run the tools check here — + # otherwise an inline agent that omits ``tools:`` against a non-passthrough + # provider slips past ``validate`` and fails mid-iteration with the same + # confusing runtime error. (Extending the remaining per-agent capability + # checks to inline agents is tracked in #270.) + for fe in config.for_each: + inline_agent = fe.agent + if not _is_llm_agent(inline_agent): + continue + inline_provider = _resolved_provider_name(inline_agent, default_provider) + inline_caps = _caps_for(inline_provider) + if inline_caps is None: + continue # error already recorded by _caps_for + _check_agent_tools(inline_agent, inline_provider, inline_caps) + # ----- Concurrency safety in parallel / for_each groups ----- agent_by_name = {a.name: a for a in config.agents} for pg in config.parallel: diff --git a/src/conductor/providers/claude_agent_sdk.py b/src/conductor/providers/claude_agent_sdk.py index fb06b44b..61f7a41c 100644 --- a/src/conductor/providers/claude_agent_sdk.py +++ b/src/conductor/providers/claude_agent_sdk.py @@ -83,9 +83,12 @@ def _build_output_format(output: dict[str, OutputField]) -> dict[str, Any]: } -# Default tool preset granted when an agent does not declare a `tools:` list. -# This mirrors the SDK's `claude_code` preset (filesystem, bash, web, etc.) — -# i.e. the same behavior the user gets when running the `claude` CLI directly. +# Default tool preset granted when an agent omits the `tools:` list. This +# mirrors the SDK's `claude_code` preset (filesystem, bash, web, etc.) — i.e. +# the same behavior the user gets when running the `claude` CLI directly. It is +# selected from the RAW ``agent.tools is None`` signal, NOT from the executor's +# resolved list: for an agent that declares no `tools:`, the executor returns the +# workflow-tools copy, which is empty only when the workflow declares no `tools:`. _DEFAULT_TOOL_PRESET: dict[str, str] = {"type": "preset", "preset": "claude_code"} # Display-only previews for the verbose CLI pretty-printer (NOT surfaced @@ -444,15 +447,24 @@ def _resolve_tool_config( identifiers. We therefore refuse to forward a non-empty allowlist to the SDK rather than silently grant the wrong native tools. + The ``tools`` argument is the executor's *resolved* list from + :func:`conductor.executor.agent.resolve_agent_tools`. That function + erases the distinction between an omitted ``tools:`` and an explicit + ``tools: []``: both arrive here as an empty list whenever the + workflow declares no workflow-level ``tools:`` (``config.tools`` is + empty; a non-empty list makes an omitted agent resolve non-empty). We + therefore consult the RAW ``agent.tools`` field — the only place the + omitted-vs-explicit signal survives — to pick the default. + Semantics: - * ``tools is None`` — no allowlist declared. Fall back to the - ``claude_code`` preset (filesystem, bash, web) and bypass - permissions, matching what the user gets from the bare - ``claude`` CLI. Backward compatible. - * ``tools == []`` — explicit "no tools" request. Pass an empty - list to the SDK so all tools are disabled. Drop the permission - bypass because there are no tools to permit. + * ``tools`` empty (``[]`` or ``None``) and ``agent.tools is None`` — + the agent omitted ``tools:``. Fall back to the ``claude_code`` + preset (filesystem, bash, web) and bypass permissions, matching + what the user gets from the bare ``claude`` CLI. + * ``tools`` empty and ``agent.tools == []`` — explicit "no tools" + request. Pass an empty list to the SDK so all tools are disabled. + Drop the permission bypass because there are no tools to permit. * ``tools`` non-empty — raise ``ProviderError``. Workflow tool name → CLI tool ID translation is not implemented (tracked as a follow-up). Silently dropping the allowlist would be a @@ -460,8 +472,10 @@ def _resolve_tool_config( the wrong native tool. Refuse loudly. Args: - tools: The workflow ``tools:`` allowlist for this agent. - agent: The agent definition (used for the error message). + tools: The executor-resolved ``tools:`` allowlist for this agent. + agent: The agent definition. ``agent.tools`` carries the raw + omitted-vs-explicit-empty signal; ``agent.name`` is used in + the error message. Returns: A ``(sdk_tools, permission_mode)`` tuple suitable for @@ -470,18 +484,24 @@ def _resolve_tool_config( Raises: ProviderError: If ``tools`` is a non-empty list. """ - if tools is None: - return _DEFAULT_TOOL_PRESET, "bypassPermissions" if not tools: + # The executor passes [] for BOTH "omitted (no workflow tools to + # inherit)" and explicit "tools: []". Disambiguate via the raw + # per-agent field, which the executor's resolution erased. + if agent.tools is None: + # Omitted -> default claude_code preset (filesystem/bash/web). + return _DEFAULT_TOOL_PRESET, "bypassPermissions" + # Explicit `tools: []` -> no tools, no permission bypass. return [], None raise ProviderError( - f"Agent '{agent.name}' declares tools={tools!r}, but " - "claude-agent-sdk does not support per-agent workflow tool " - "allowlists (workflow tool names do not translate to Claude " - "CLI tool IDs).", + f"Agent '{agent.name}' resolves to tools={tools!r} (declared on " + "the agent or inherited from the workflow-level 'tools:' list), " + "but claude-agent-sdk does not support workflow tool allowlists " + "(workflow tool names do not translate to Claude CLI tool IDs).", suggestion=( - "Remove the 'tools:' field to grant the full claude_code " - "preset, or set 'tools: []' to disable all tools." + "Omit both the per-agent and workflow-level 'tools:' to grant " + "the full claude_code preset, or set 'tools: []' to disable " + "all tools." ), ) diff --git a/tests/test_config/test_validator_capabilities.py b/tests/test_config/test_validator_capabilities.py index b5257c1c..304e0725 100644 --- a/tests/test_config/test_validator_capabilities.py +++ b/tests/test_config/test_validator_capabilities.py @@ -48,10 +48,14 @@ def _build_workflow( parallel: list[ParallelGroup] | None = None, for_each: list[ForEachDef] | None = None, mcp_servers: dict[str, MCPServerDef] | None = None, + tools: list[str] | None = None, ) -> WorkflowConfig: runtime_kwargs: dict[str, Any] = {"provider": "copilot"} if mcp_servers is not None: runtime_kwargs["mcp_servers"] = mcp_servers + workflow_kwargs: dict[str, Any] = {} + if tools is not None: + workflow_kwargs["tools"] = tools return WorkflowConfig( workflow=WorkflowDef( name="test", @@ -61,6 +65,7 @@ def _build_workflow( agents=agents, parallel=parallel or [], for_each=for_each or [], + **workflow_kwargs, ) @@ -144,6 +149,117 @@ def test_omitted_tools_against_no_passthrough_does_not_error(self, patch_caps: A config = _build_workflow(agents=[AgentDef(name="a", prompt="hi")]) validate_workflow_config(config) # no raise + def test_omitted_tools_inherits_workflow_tools_against_no_passthrough_errors( + self, patch_caps: Any + ) -> None: + """Omitted ``tools:`` + non-empty workflow ``tools:`` + no passthrough. + + An omitted per-agent ``tools:`` inherits the workflow-level list at + runtime; a non-passthrough provider would then refuse it at execute + time with a confusing "declares tools=[...]" error even though the + agent declared none. Catch it at validate time instead. + """ + patch_caps({"copilot": _caps(workflow_tools_passthrough=False)}) + config = _build_workflow( + agents=[AgentDef(name="a", prompt="hi")], + tools=["search", "read_file"], + ) + with pytest.raises(ConfigurationError, match="omits 'tools:' and would inherit"): + validate_workflow_config(config) + + def test_explicit_empty_tools_with_workflow_tools_no_passthrough_passes( + self, patch_caps: Any + ) -> None: + """Explicit ``tools: []`` opts out of inheritance, so it stays valid.""" + patch_caps({"copilot": _caps(workflow_tools_passthrough=False)}) + config = _build_workflow( + agents=[AgentDef(name="a", prompt="hi", tools=[])], + tools=["search"], + ) + validate_workflow_config(config) # no raise + + def test_omitted_tools_inherits_workflow_tools_with_passthrough_passes( + self, patch_caps: Any + ) -> None: + """A passthrough provider honors the inherited list, so no error.""" + patch_caps({"copilot": _caps(workflow_tools_passthrough=True)}) + config = _build_workflow( + agents=[AgentDef(name="a", prompt="hi")], + tools=["search"], + ) + validate_workflow_config(config) # no raise + + +class TestForEachInlineToolsCrossCheck: + """The tools cross-check must also cover a for_each group's INLINE agent. + + A ``for_each`` group carries an inline ``AgentDef`` that is NOT in + ``config.agents`` but runs at runtime with ``workflow_tools=config.tools``, + exactly like a top-level agent. Without an explicit pass it would slip past + ``validate`` and fail mid-iteration with the same confusing error. + """ + + def _for_each_config( + self, + *, + inline: AgentDef, + tools: list[str] | None = None, + ) -> WorkflowConfig: + # The entry agent opts out with ``tools: []`` so only the inline agent + # can trip the check — isolating the assertion to the for_each path. + return _build_workflow( + agents=[AgentDef(name="entry", prompt="hi", tools=[])], + tools=tools, + for_each=[ + ForEachDef( + name="loop", + type="for_each", + source="entry.output.items", + **{"as": "item"}, + agent=inline, + ) + ], + ) + + def test_inline_omitted_tools_inherits_workflow_tools_errors(self, patch_caps: Any) -> None: + """Inline agent omits ``tools:`` + non-empty workflow ``tools:`` -> error.""" + patch_caps({"copilot": _caps(workflow_tools_passthrough=False)}) + config = self._for_each_config( + inline=AgentDef(name="inner", prompt="{{ item }}"), + tools=["search"], + ) + with pytest.raises( + ConfigurationError, match="Agent 'inner' omits 'tools:' and would inherit" + ): + validate_workflow_config(config) + + def test_inline_explicit_empty_tools_passes(self, patch_caps: Any) -> None: + """Inline ``tools: []`` opts out of inheritance, so it stays valid.""" + patch_caps({"copilot": _caps(workflow_tools_passthrough=False)}) + config = self._for_each_config( + inline=AgentDef(name="inner", prompt="{{ item }}", tools=[]), + tools=["search"], + ) + validate_workflow_config(config) # no raise + + def test_inline_explicit_nonempty_tools_errors(self, patch_caps: Any) -> None: + """An explicit non-empty inline allowlist against a non-passthrough provider errors.""" + patch_caps({"copilot": _caps(workflow_tools_passthrough=False)}) + config = self._for_each_config( + inline=AgentDef(name="inner", prompt="{{ item }}", tools=["search"]), + ) + with pytest.raises(ConfigurationError, match="Agent 'inner' declares tools="): + validate_workflow_config(config) + + def test_inline_omitted_tools_with_passthrough_passes(self, patch_caps: Any) -> None: + """A passthrough provider honors the inherited list, so no error.""" + patch_caps({"copilot": _caps(workflow_tools_passthrough=True)}) + config = self._for_each_config( + inline=AgentDef(name="inner", prompt="{{ item }}"), + tools=["search"], + ) + validate_workflow_config(config) # no raise + class TestReasoningEffortCrossCheck: def test_unsupported_level_errors(self, patch_caps: Any) -> None: diff --git a/tests/test_providers/test_claude_agent_sdk.py b/tests/test_providers/test_claude_agent_sdk.py index 348fab0b..563194b0 100644 --- a/tests/test_providers/test_claude_agent_sdk.py +++ b/tests/test_providers/test_claude_agent_sdk.py @@ -576,11 +576,15 @@ async def fake_query(**kwargs): @patch("conductor.providers.claude_agent_sdk.CLAUDE_AGENT_SDK_AVAILABLE", True) async def test_empty_tools_list_disables_tools(self) -> None: - """``tools: []`` disables ALL tools and drops the permission bypass. + """Explicit ``tools: []`` disables ALL tools and drops the permission bypass. Regression test for #241 (A1): previously the empty list was silently ignored and the agent got the full ``claude_code`` preset (filesystem/bash/web) — a security regression. + + The agent here declares ``tools: []`` explicitly (``agent.tools == []``), + which is what distinguishes it from an omitted ``tools:`` (the latter + gets the preset — see :class:`TestOmittedToolsDefaultPreset`). """ options_mock = Mock() @@ -592,7 +596,7 @@ async def fake_query(**kwargs): patch("conductor.providers.claude_agent_sdk.ClaudeAgentOptions", options_mock), ): provider = ClaudeAgentSdkProvider() - agent = AgentDef(name="test", prompt="hi") + agent = AgentDef(name="test", prompt="hi", tools=[]) await provider.execute(agent=agent, context={}, rendered_prompt="hi", tools=[]) call_kwargs = options_mock.call_args[1] @@ -616,7 +620,107 @@ async def fake_query(**kwargs): with patch("conductor.providers.claude_agent_sdk.query", fake_query): provider = ClaudeAgentSdkProvider() agent = AgentDef(name="my_agent", prompt="hi") - with pytest.raises(ProviderError, match="does not support per-agent workflow tool"): + with pytest.raises(ProviderError, match="does not support workflow tool allowlists"): + await provider.execute( + agent=agent, + context={}, + rendered_prompt="hi", + tools=["search", "read_file"], + ) + + +class TestOmittedToolsDefaultPreset: + """An agent that omits ``tools:`` must receive the ``claude_code`` preset. + + Regression test for the executor↔provider contract bug: the executor's + ``resolve_agent_tools(agent.tools, workflow_tools)`` returns + ``workflow_tools.copy()`` (``[]`` when the workflow declares no + ``runtime`` MCP tools) for an omitted ``tools:``, so the provider is + ALWAYS handed a concrete list and never ``None``. Before the fix, the + provider could not tell "omitted (defaults to all)" from explicit + ``tools: []`` (both arrive as ``[]``) and granted ZERO tools to an agent + that simply forgot to declare ``tools:`` — e.g. a "read a file and + answer" agent came up with no filesystem tools and failed. + + The provider distinguishes the two cases by inspecting the raw + ``agent.tools`` field, which preserves the omitted (``None``) vs. + explicit-empty (``[]``) distinction the executor erases. + """ + + @patch("conductor.providers.claude_agent_sdk.CLAUDE_AGENT_SDK_AVAILABLE", True) + async def test_omitted_tools_with_empty_executor_list_grants_preset(self) -> None: + """The real bug: executor passes ``tools=[]`` for an omitted ``tools:``. + + ``AgentDef`` defaults ``tools`` to ``None`` (omitted), and the + executor turns that into ``[]`` before calling the provider. The + provider must still grant the ``claude_code`` preset, not no tools. + """ + options_mock = Mock() + + async def fake_query(**kwargs): + yield _result(result="done") + + with ( + patch("conductor.providers.claude_agent_sdk.query", fake_query), + patch("conductor.providers.claude_agent_sdk.ClaudeAgentOptions", options_mock), + ): + provider = ClaudeAgentSdkProvider() + # agent.tools is None (omitted), but the executor erases that to [] + # before calling the provider — exactly what AgentExecutor does. + agent = AgentDef(name="reader", prompt="read a file and answer") + assert agent.tools is None + await provider.execute(agent=agent, context={}, rendered_prompt="hi", tools=[]) + + call_kwargs = options_mock.call_args[1] + assert call_kwargs["tools"] == {"type": "preset", "preset": "claude_code"}, ( + "An agent that omits `tools:` must receive the claude_code preset " + "even though the executor hands the provider an empty list." + ) + assert call_kwargs["permission_mode"] == "bypassPermissions" + + @patch("conductor.providers.claude_agent_sdk.CLAUDE_AGENT_SDK_AVAILABLE", True) + async def test_explicit_empty_tools_still_disables_tools(self) -> None: + """An agent that explicitly declares ``tools: []`` still gets no tools. + + The executor passes ``[]`` here too, but ``agent.tools == []`` (not + ``None``) records the explicit opt-out, so the provider disables all + tools and drops the permission bypass. + """ + options_mock = Mock() + + async def fake_query(**kwargs): + yield _result(result="done") + + with ( + patch("conductor.providers.claude_agent_sdk.query", fake_query), + patch("conductor.providers.claude_agent_sdk.ClaudeAgentOptions", options_mock), + ): + provider = ClaudeAgentSdkProvider() + agent = AgentDef(name="no_tools", prompt="hi", tools=[]) + assert agent.tools == [] + await provider.execute(agent=agent, context={}, rendered_prompt="hi", tools=[]) + + call_kwargs = options_mock.call_args[1] + assert call_kwargs["tools"] == [] + assert call_kwargs["permission_mode"] is None + + @patch("conductor.providers.claude_agent_sdk.CLAUDE_AGENT_SDK_AVAILABLE", True) + @patch("conductor.providers.claude_agent_sdk.ClaudeAgentOptions", Mock) + async def test_explicit_non_empty_tools_still_raises(self) -> None: + """An explicit non-empty per-agent allowlist is still refused loudly. + + Captures the exception so both the message AND the ``suggestion`` (also + reworded by this fix) are pinned — a regression that drops the + "set 'tools: []'" escape hatch from the suggestion is caught here. + """ + + async def fake_query(**kwargs): + yield _result(result="done") + + with patch("conductor.providers.claude_agent_sdk.query", fake_query): + provider = ClaudeAgentSdkProvider() + agent = AgentDef(name="my_agent", prompt="hi", tools=["search", "read_file"]) + with pytest.raises(ProviderError) as exc: await provider.execute( agent=agent, context={}, @@ -624,6 +728,80 @@ async def fake_query(**kwargs): tools=["search", "read_file"], ) + assert "does not support workflow tool allowlists" in str(exc.value) + assert exc.value.suggestion is not None + assert "tools: []" in exc.value.suggestion + + @patch("conductor.providers.claude_agent_sdk.CLAUDE_AGENT_SDK_AVAILABLE", True) + @patch("conductor.providers.claude_agent_sdk.ClaudeAgentOptions", Mock) + async def test_inherited_workflow_tools_raise_with_inheritance_wording(self) -> None: + """An omitted ``tools:`` that inherits a non-empty workflow-level list. + + This is the inheritance path: ``agent.tools is None`` (omitted) but the + executor resolved a non-empty list from the workflow-level ``tools:`` and + handed it to the provider. The refusal must name the workflow-level + inheritance — not just "declared on the agent" — so the user knows where + the list came from. This guards the new "inherited from the + workflow-level" wording the fix introduced. + """ + + async def fake_query(**kwargs): + yield _result(result="done") + + with patch("conductor.providers.claude_agent_sdk.query", fake_query): + provider = ClaudeAgentSdkProvider() + # Omitted per-agent tools (None), but the executor resolved a + # non-empty list inherited from the workflow-level `tools:`. + agent = AgentDef(name="inheritor", prompt="hi") + assert agent.tools is None + with pytest.raises(ProviderError) as exc: + await provider.execute( + agent=agent, + context={}, + rendered_prompt="hi", + tools=["search", "read_file"], + ) + + assert "inherited from the workflow-level" in str(exc.value) + assert exc.value.suggestion is not None + assert "tools: []" in exc.value.suggestion + + @patch("conductor.providers.claude_agent_sdk.CLAUDE_AGENT_SDK_AVAILABLE", True) + async def test_executor_to_provider_end_to_end_grants_preset(self) -> None: + """End-to-end through AgentExecutor: an omitted ``tools:`` reaches the + provider as the ``claude_code`` preset, with NO workflow tools declared. + + This pins the full call chain that the original bug broke: + ``AgentExecutor.execute`` → ``resolve_agent_tools(None, [])`` → ``[]`` + → ``provider.execute(tools=[])`` → preset. + """ + from conductor.executor.agent import AgentExecutor + + options_mock = Mock() + captured: dict = {} + + async def fake_query(**kwargs): + yield _result(structured_output={"answer": "from the file"}) + + with ( + patch("conductor.providers.claude_agent_sdk.query", fake_query), + patch("conductor.providers.claude_agent_sdk.ClaudeAgentOptions", options_mock), + ): + provider = ClaudeAgentSdkProvider() + # No workflow-level tools — resolve_agent_tools returns []. + executor = AgentExecutor(provider, workflow_tools=[]) + agent = AgentDef( + name="reader", + prompt="Read README.md and answer.", + output={"answer": OutputField(type="string")}, + ) + assert agent.tools is None + await executor.execute(agent=agent, context={}) + captured.update(options_mock.call_args[1]) + + assert captured["tools"] == {"type": "preset", "preset": "claude_code"} + assert captured["permission_mode"] == "bypassPermissions" + class TestAgentTurnStartOrdering: """Provider parity: agent_turn_start event ordering (#241 / A3).