diff --git a/AGENTS.md b/AGENTS.md index 23f061c5..80316228 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -253,7 +253,12 @@ and output parity** but the following are managed by the SDK rather than Conductor: - **Retry and error handling**: The `claude-agent-sdk` package does **not** retry API failures (429s, 5xx, network errors) internally — its built-in retry logic covers only filesystem operations. Conductor wraps SDK errors in `ProviderError` and uses `stop_reason` / error subtype to set `is_retryable`, so workflow-level `retry:` configuration drives all retry behavior. Plan for transient failures with explicit `retry:` blocks in your workflow. -- **Tool execution**: Tools and MCP servers are managed by the `claude` CLI's own configuration. The provider rejects workflow-level `runtime.mcp_servers` at the factory and refuses any non-empty per-agent `tools:` list (workflow tool names do not translate to CLI tool IDs). An agent with `tools: []` runs with no tools; omitting `tools:` grants the full `claude_code` preset. +- **MCP servers** (issue #335): workflow-level `runtime.mcp_servers` **are** supported. `_translate_mcp_servers` maps each `MCPServerDef`-derived dict onto the SDK's `McpStdioServerConfig` / `McpHttpServerConfig` / `McpSSEServerConfig` shapes, and the provider passes them via `ClaudeAgentOptions`. Four details are load-bearing: + - Translation runs once in `__init__` rather than per `execute` call. Note providers are constructed **lazily** (`ProviderRegistry.get_provider` ← `WorkflowEngine._get_executor_for_agent`), so a bad server config surfaces when the first agent on this provider runs, **not** at `conductor validate` — which does not inspect per-server `tools:` filters at all. + - The config is written to a `0600` temp file (`_write_mcp_config`) and passed **by path**. Passing the dict would make the SDK serialize it into a `--mcp-config ` argv element, publishing resolved stdio `env` values and http/sse `Authorization` headers to anything that can read `/proc//cmdline`. The write happens **inside** `execute`'s `try`, so the `finally` reclaims the file on every exit path; the finally also `aclose()`s the SDK iterator first, so the `claude` subprocess is gone before its config file is. The file must use the `{"mcpServers": {...}}` envelope — the CLI rejects a bare mapping. + - `strict_mcp_config=True` is set **unconditionally**, including when the workflow declares no servers: otherwise the CLI loads project `.mcp.json`, user-global, and plugin-provided servers, and `permission_mode` bypasses approval for whatever they expose. + - A narrowing per-server `tools:` filter (anything other than the default `["*"]`) is **refused**, not ignored: forwarding the server unfiltered would grant more tools than declared, the same security regression that justifies refusing the per-agent allowlist. A dropped `timeout` only warns, since losing it cannot widen tool access. +- **Tool execution**: Per-agent `tools:` allowlists remain unsupported (`workflow_tools_passthrough=False`). The provider refuses any non-empty per-agent list because workflow tool names do not translate to CLI tool IDs. Note the SDK's `tools` option governs **built-in** tools only, and `allowed_tools` is a permission auto-approve list rather than an availability filter — so honoring an allowlist would require a permission-mode redesign, not just a name mapping. An agent with `tools: []` runs with no built-in tools (MCP servers still attach); omitting `tools:` grants the full `claude_code` preset. - **Runtime config**: `temperature` and `max_tokens` are rejected at the factory — the CLI controls sampling behavior. #### `aca.py` parity notes @@ -298,9 +303,11 @@ it will pick up the developer's real token (see `config/validator.py` rejects **any** explicit `tools:` on an `aca`-backed agent, not just a non-empty one (review follow-up, #284 E7). This mirrors the same declared carve-out on `claude_agent_sdk.py` - and `hermes.py`, except those declare `mcp_tools=False` (nothing is ever + and `hermes.py`. `hermes.py` declares `mcp_tools=False` (nothing is ever forwarded regardless of the list), so `tools: []` genuinely disables all - tools and stays valid for them. + tools and stays valid for it. `claude_agent_sdk.py` now behaves like + `aca` whenever the workflow declares `mcp_servers`, and like `hermes` + when it does not. - **`working_dir=False`**: this capability field means "applies the generic, host-resolved `agent.working_dir` / `runtime.working_dir`" — a host filesystem path the engine resolves against the workflow file's diff --git a/CHANGELOG.md b/CHANGELOG.md index 28ee97d7..3afed0cd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,47 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased](https://github.com/microsoft/conductor/compare/v0.1.26...HEAD) +### Added + +- **`claude-agent-sdk` provider now supports MCP servers** — workflow-level + `runtime.mcp_servers` are translated to the SDK's own `stdio` / `http` / + `sse` config shapes and passed through `ClaudeAgentOptions`, so an agent can + use custom MCP tool servers *and* the built-in `claude_code` tool preset at + the same time. Previously the two were mutually exclusive: the provider + declared `mcp_tools=False` and the factory rejected any workflow declaring + MCP servers. The generated config is written to a `0600` temp file and + passed by path so resolved `env` values and `Authorization` headers never + reach the `claude` CLI's command line, and `strict_mcp_config` is always + enabled so ambient project/user MCP config cannot inject undeclared servers. + A narrowing per-server `tools:` filter has no SDK equivalent and is refused + (when the first agent on this provider runs) rather than silently ignored. + See + [`examples/claude-agent-sdk-mcp.yaml`](examples/claude-agent-sdk-mcp.yaml) + and [`docs/mcp-tools.md`](docs/mcp-tools.md). + ([#335](https://github.com/microsoft/conductor/issues/335)) + +### Fixed + +- **`tools: []` no longer fails validation when no MCP servers are declared** — + the capability cross-check rejected an explicit empty allowlist against any + provider with `mcp_tools=True` and `workflow_tools_passthrough=False` (such + as `aca`), even when the workflow declared no `mcp_servers` and therefore had + nothing to forward. The check is now gated on MCP servers actually being + configured. + ([#335](https://github.com/microsoft/conductor/issues/335)) + +### Changed + +- The `claude-agent-sdk` optional dependency floor is now + `claude-agent-sdk>=0.2.82` — the 0.2.x line is what Conductor tests against. + ([#335](https://github.com/microsoft/conductor/issues/335)) +- `claude-agent-sdk` agents no longer inherit ambient MCP configuration. + Conductor now always sets `strict_mcp_config`, so a project `.mcp.json`, + user-global settings, or plugin-provided servers are ignored and only + servers declared in `runtime.mcp_servers` attach. Workflows that relied on + Claude Code's own MCP settings must declare those servers in the workflow. + ([#335](https://github.com/microsoft/conductor/issues/335)) + ## [0.1.26](https://github.com/microsoft/conductor/compare/v0.1.25...v0.1.26) - 2026-07-27 ### Added diff --git a/README.md b/README.md index 8043f87b..ba402957 100644 --- a/README.md +++ b/README.md @@ -251,9 +251,9 @@ workflow: default_model: claude-sonnet-5 ``` -Requires the `claude` CLI to be installed and authenticated. Install the SDK: `uv add 'claude-agent-sdk>=0.1.0'` +Requires the `claude` CLI to be installed and authenticated. Install the SDK: `uv add 'claude-agent-sdk>=0.2.82'` -> **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. +> **Note:** `runtime.mcp_servers` is supported — servers are translated into the SDK's own MCP config and attach alongside the built-in `claude_code` preset (a narrowing per-server `tools:` filter is refused, since the SDK cannot enforce one). Per-agent tool allowlists are not bridged: 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; an agent's `tools: []` disables the built-in tools, though declared MCP servers still attach. ### Using Hermes (Experimental) diff --git a/docs/mcp-tools.md b/docs/mcp-tools.md index b28e6e0b..88ae4874 100644 --- a/docs/mcp-tools.md +++ b/docs/mcp-tools.md @@ -89,7 +89,7 @@ mcp_servers: The configuration fields are the same as `http`. -> **Provider note:** The Claude provider only supports `stdio` servers. The `http` and `sse` types are supported by the Copilot provider only. +> **Provider note:** The Claude provider only supports `stdio` servers. The `http` and `sse` types are supported by the Copilot and Claude Agent SDK providers. ## Configuration Reference @@ -326,12 +326,12 @@ workflow: | Feature | Copilot | Claude | Claude Agent SDK | Hermes | |---|---|---|---|---| -| stdio servers | ✅ | ✅ | ❌ | ❌ | -| http servers | ✅ | ❌ | ❌ | ❌ | -| sse servers | ✅ | ❌ | ❌ | ❌ | -| Tool filtering | ✅ | ✅ | ❌ | ❌ | -| OAuth auto-auth | ✅ | N/A | ❌ | ❌ | -| env var passing | ⚠️ Bug ([#163](https://github.com/github/copilot-sdk/issues/163)) | ✅ | ❌ | ❌ | +| stdio servers | ✅ | ✅ | ✅ | ❌ | +| http servers | ✅ | ❌ | ✅ | ❌ | +| sse servers | ✅ | ❌ | ✅ | ❌ | +| Tool filtering | ✅ | ✅ | ❌ (refused) | ❌ | +| OAuth auto-auth | ✅ | N/A | ✅ | ❌ | +| env var passing | ⚠️ Bug ([#163](https://github.com/github/copilot-sdk/issues/163)) | ✅ | ✅ | ❌ | | Tool output limits | ✅ (native SDK) | ✅ (conductor-side) | ✅ (native CLI env var) | N/A | ### Copilot Provider @@ -349,6 +349,19 @@ The Claude provider uses Conductor's built-in `MCPManager` to spawn and manage M HTTP and SSE servers are not supported with the Claude provider. If configured, a warning is logged and the server is skipped. +### Claude Agent SDK Provider + +The Claude Agent SDK provider translates each server into the SDK's own MCP config shape and passes it to the `claude` CLI, which owns server lifecycle and tool execution. All three transport types are supported, and MCP tools attach *alongside* the built-in `claude_code` tool preset — see [`examples/claude-agent-sdk-mcp.yaml`](../examples/claude-agent-sdk-mcp.yaml). + +Two behaviors are specific to this provider: + +- **Per-server `tools:` filters are refused.** The SDK's MCP config has no equivalent field, so a narrowing filter cannot be enforced. Rather than forward the server unfiltered — granting more tools than the workflow declared — Conductor raises a `ProviderError` the first time an agent on this provider runs. Note `conductor validate` does not catch this today. Keep the default `tools: ["*"]`. +- **Only declared servers are reachable.** Conductor sets `strict_mcp_config`, so a project `.mcp.json` or user-global MCP setting cannot add servers the workflow never declared. + +The generated config is written to a `0600` temp file and passed to the CLI by path, so resolved `env` values and `Authorization` headers stay out of the process command line. A fresh file is written and deleted per agent execution. + +A per-server `timeout` has no SDK equivalent and is dropped with a warning. + ## Examples ### Web Search diff --git a/docs/providers/aca.md b/docs/providers/aca.md index 9bc146b1..f5e6c1e9 100644 --- a/docs/providers/aca.md +++ b/docs/providers/aca.md @@ -582,7 +582,7 @@ agents: | Capability | Value | Notes | |---|---|---| | `mcp_tools` | ✅ `True` | Full `mcp_servers` forwarded — runner-image contract. | -| `workflow_tools_passthrough` | ❌ **`False`** | The per-agent `tools:` allowlist is forwarded to the runner in the request body, but the in-container `CopilotProvider` it wraps never applies that list to the SDK session — every tool/MCP server available to the session is callable regardless of the declared allowlist. Combined with `mcp_tools=True` (below), there is no allowlist value the runner can honor — not even `tools: []` — so `conductor validate` rejects any explicit `tools:` on an `aca`-backed agent. This is a known, allowed experimental carve-out (the same gap `claude_agent_sdk` and `hermes` already declare, though those declare `mcp_tools=False` so `tools: []` stays valid for them). | +| `workflow_tools_passthrough` | ❌ **`False`** | The per-agent `tools:` allowlist is forwarded to the runner in the request body, but the in-container `CopilotProvider` it wraps never applies that list to the SDK session — every tool/MCP server available to the session is callable regardless of the declared allowlist. Combined with `mcp_tools=True` (below), there is no allowlist value the runner can honor — not even `tools: []` — so `conductor validate` rejects any explicit `tools:` on an `aca`-backed agent. This is a known, allowed experimental carve-out (the same gap `claude_agent_sdk` and `hermes` already declare; `hermes` declares `mcp_tools=False` so `tools: []` stays valid for it, and `claude_agent_sdk` behaves like `aca` here only when the workflow declares `mcp_servers`). | | `streaming_events` | ✅ `True` | Single streaming request relays event frames incrementally. | | `agent_reasoning_events` | ✅ `True` | Runner forwards reasoning frames from the inner provider. | | `reasoning_effort` | ✅ Copilot's full tuple | Inner provider (Copilot) translates reasoning effort natively. | diff --git a/docs/providers/comparison.md b/docs/providers/comparison.md index 64120034..bcc6e121 100644 --- a/docs/providers/comparison.md +++ b/docs/providers/comparison.md @@ -12,8 +12,8 @@ This guide helps you choose between GitHub Copilot, Anthropic Claude, Claude Age | **Setup** | GitHub auth | API key | `claude` CLI auth | API key (model-provider's key) | | **Model Selection** | GPT-5.2, o1 | Haiku, Sonnet, Opus | Haiku, Sonnet, Opus | Any OpenRouter-style model | | **Streaming** | Yes | No (Phase 1) | Yes | Yes | -| **Tool Support** | Yes (MCP, all types) | Yes (MCP, stdio only) | Yes (built-in, CLI-managed) | Yes (hermes toolsets) | -| **MCP Servers** | Yes | Yes (stdio) | No | No | +| **Tool Support** | Yes (MCP, all types) | Yes (MCP, stdio only) | Yes (MCP + built-in preset) | Yes (hermes toolsets) | +| **MCP Servers** | Yes | Yes (stdio) | Yes (all types) | No | | **Reasoning / Extended Thinking** | Yes (`reasoning_effort` on session) | Yes (extended `thinking` budget) | Inherits from CLI config | Yes (`reasoning_config`) | | **Speed** | Fast | Fast | Fast | Depends on model | | **Output Quality** | Excellent | Excellent | Excellent | Depends on model | @@ -25,7 +25,7 @@ This guide helps you choose between GitHub Copilot, Anthropic Claude, Claude Age | **Tool Output Limits** | native SDK spill (large_output) | conductor-side truncation+spill | native CLI env var | N/A | > **About the experimental tier.** `claude-agent-sdk` and `hermes` declare -> specific capability carve-outs (e.g. no MCP servers). `conductor validate` +> specific capability carve-outs (e.g. no per-agent tools allowlist). `conductor validate` > catches workflows that depend on those features against these providers, > and the CLI prints a one-time banner when the workflow runs. See > [docs/providers/experimental.md](./experimental.md) for the stability @@ -123,10 +123,10 @@ agents: ### Important: Tools and MCP Servers -The `claude-agent-sdk` provider does not bridge workflow-level tools/MCP into the CLI. Concretely: +The `claude-agent-sdk` provider bridges MCP servers into the CLI, but not per-agent tool allowlists. Concretely: -- `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. +- `runtime.mcp_servers` — **supported**. Servers are translated into the SDK's MCP config and attach alongside the built-in preset. Only declared servers attach: Conductor sets `strict_mcp_config`, so ambient Claude Code MCP settings are ignored. A narrowing per-server `tools:` filter is refused, since the SDK has no equivalent field. +- Per-agent `tools: []` — disables the built-in tools for that agent. Declared MCP servers still attach, so this combination is rejected at `conductor validate` when the workflow declares `mcp_servers`. - 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. - 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. diff --git a/docs/providers/experimental.md b/docs/providers/experimental.md index ec1f5941..c2171e8a 100644 --- a/docs/providers/experimental.md +++ b/docs/providers/experimental.md @@ -22,9 +22,10 @@ prints a one-time banner per provider: ```text ┌─────────────────────────────────────────────────────────────────────┐ │ ⚠ Experimental provider in use: claude-agent-sdk │ -│ (claude-agent-sdk>=0.1.0) maintained by @lesandiz (best-effort) │ -│ Limitations: no MCP servers, no per-agent tools allowlist, │ -│ reasoning_effort ignored, no checkpoint resume. │ +│ (claude-agent-sdk>=0.2.82) maintained by @lesandiz (best-effort) │ +│ Limitations: no per-agent tools allowlist, reasoning_effort │ +│ ignored, structured output via prompt injection, no checkpoint │ +│ resume, working_dir ignored. │ │ See docs/providers/experimental.md for stability policy. │ └─────────────────────────────────────────────────────────────────────┘ ``` @@ -98,7 +99,7 @@ adopting one does not inflate the install surface for others. | Provider | Upstream pin | Maintainer | Capability carve-outs | |---|---|---|---| -| `claude-agent-sdk` | `claude-agent-sdk>=0.1.0` | `@lesandiz (best-effort)` | no `mcp_tools`, no `workflow_tools_passthrough`, no `reasoning_effort`, `prompt_injection` structured output, no `checkpoint_resume`, no `working_dir` | +| `claude-agent-sdk` | `claude-agent-sdk>=0.2.82` | `@lesandiz (best-effort)` | no `workflow_tools_passthrough`, no `reasoning_effort`, `prompt_injection` structured output, no `checkpoint_resume`, no `working_dir`. Supports `mcp_tools` as of [#335](https://github.com/microsoft/conductor/issues/335), except that a narrowing per-server `tools:` filter is refused (no SDK equivalent). | | `hermes` | `hermes-agent` | `(community contribution)` | no `mcp_tools`, `prompt_injection` structured output, no `working_dir` | | `aca` | `azure-identity>=1.19.0` | `(unassigned)` | no `workflow_tools_passthrough` (the wrapped in-container `CopilotProvider` never applies the `tools:` allowlist to the SDK session), no `working_dir` (only the separate, container-relative `sandbox.working_dir` is honored — not the generic host-resolved field), `prompt_injection` structured output (inherits the inner Copilot provider), no `checkpoint_resume` (ephemeral sandbox sessions, no volume mount). Declares `interrupt`/`max_session_seconds` as `True`, but the shipped runner MVP doesn't fully back either yet — see [Known Gaps](./aca.md#known-gaps-runner-mvp). | diff --git a/examples/claude-agent-sdk-mcp.yaml b/examples/claude-agent-sdk-mcp.yaml new file mode 100644 index 00000000..cffaa669 --- /dev/null +++ b/examples/claude-agent-sdk-mcp.yaml @@ -0,0 +1,94 @@ +# claude-agent-sdk + MCP servers +# +# Runs an agent with the full `claude_code` CLI tool preset AND custom MCP tool +# servers attached at the same time. +# +# How it works: +# - Conductor translates each `mcp_servers:` entry into the SDK's own +# `McpStdioServerConfig` / `McpHttpServerConfig` / `McpSSEServerConfig` +# shape and hands it to `ClaudeAgentOptions`. +# - The config is written to a private (0600) temp file and passed by path, +# so resolved `env` values and `Authorization` headers never appear in the +# `claude` CLI's command line (visible via /proc//cmdline otherwise). +# - `strict_mcp_config` is always enabled, so an ambient project `.mcp.json` +# or a user-global MCP setting cannot inject servers this workflow never +# declared. Servers must be declared here to be reachable. +# +# Two carve-outs still apply to this provider: +# - Per-server `tools:` filters are refused (the SDK's MCP config has no +# equivalent field, and ignoring a filter would grant MORE tools than +# declared). Use `tools: ["*"]`, which is the default. +# - Per-agent `tools:` allowlists are still refused +# (`workflow_tools_passthrough=False`). Omit `tools:` to get the +# `claude_code` preset plus every declared MCP server. +# +# Pre-requisites: +# pip install conductor[claude-agent-sdk] +# # Plus the `claude` CLI: +# npm install -g @anthropic-ai/claude-code +# claude login +# # The MCP server below is fetched on demand by npx (Node.js required). +# +# Usage: +# conductor run examples/claude-agent-sdk-mcp.yaml \ +# --input question="What changed in the Python 3.13 release?" +# +# Validation only (no execution / no API calls): +# conductor validate examples/claude-agent-sdk-mcp.yaml + +workflow: + name: claude-agent-sdk-mcp + description: > + Research agent running on the experimental claude-agent-sdk provider with + a custom MCP search server attached alongside the built-in claude_code + tool preset. + entry_point: researcher + + runtime: + provider: claude-agent-sdk + default_model: claude-sonnet-4-5 + max_agent_iterations: 20 + + mcp_servers: + web-search: + type: stdio + command: npx + args: ["-y", "open-websearch@latest"] + env: + MODE: stdio + # Must stay ["*"] for this provider: a narrowing filter has no SDK + # equivalent and is refused at provider construction. + tools: ["*"] + + limits: + max_iterations: 5 + timeout_seconds: 600 + +agents: + - name: researcher + description: Researches a question using MCP search plus the built-in tools + # NOTE: no `tools:` here on purpose. Omitting it grants the claude_code + # preset; the MCP servers above are attached on top of it. Declaring an + # explicit allowlist would fail validation for this provider. + prompt: | + Research the following question and report what you find. + + Question: {{ workflow.input.question }} + + Use the web-search MCP tools for current information, and the built-in + file and shell tools if local context helps. + output: + answer: + type: string + description: A concise answer to the question + sources: + type: array + description: URLs or references supporting the answer + items: + type: string + routes: + - to: $end + +output: + answer: "{{ researcher.output.answer }}" + sources: "{{ researcher.output.sources }}" diff --git a/examples/claude-agent-sdk-repo-qa.yaml b/examples/claude-agent-sdk-repo-qa.yaml index 0e43d590..06145062 100644 --- a/examples/claude-agent-sdk-repo-qa.yaml +++ b/examples/claude-agent-sdk-repo-qa.yaml @@ -18,7 +18,7 @@ # `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' +# pip install conductor[claude-agent-sdk] # or: uv add 'claude-agent-sdk>=0.2.82' # npm install -g @anthropic-ai/claude-code # the `claude` CLI # claude login # diff --git a/examples/experimental-claude-agent-sdk.yaml b/examples/experimental-claude-agent-sdk.yaml index fb3ba20d..a09c67a0 100644 --- a/examples/experimental-claude-agent-sdk.yaml +++ b/examples/experimental-claude-agent-sdk.yaml @@ -20,7 +20,7 @@ # Pre-requisites: # pip install conductor[claude-agent-sdk] # # Or with uv: -# uv add 'claude-agent-sdk>=0.1.0' +# uv add 'claude-agent-sdk>=0.2.82' # # Plus the `claude` CLI: # npm install -g @anthropic-ai/claude-code # claude login diff --git a/pyproject.toml b/pyproject.toml index 8290fa97..abae8b2b 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -49,7 +49,10 @@ dependencies = [ [project.optional-dependencies] claude-agent-sdk = [ - "claude-agent-sdk>=0.1.64", + # `strict_mcp_config` (which the provider always sets, so ambient MCP + # config cannot attach undeclared servers) has existed since 0.1.74, but + # the 0.2.x line is what Conductor tests against. + "claude-agent-sdk>=0.2.82", ] aca = [ "azure-identity>=1.19.0", diff --git a/src/conductor/config/schema.py b/src/conductor/config/schema.py index 3a0b6cf3..4a17d287 100644 --- a/src/conductor/config/schema.py +++ b/src/conductor/config/schema.py @@ -1686,8 +1686,8 @@ class MCPServerDef(BaseModel): Supports ${VAR} and ${VAR:-default} syntax for environment variable interpolation at runtime. - Note: With the Claude provider, env vars are passed correctly to MCP - server subprocesses via the MCP SDK. However, the Copilot provider + Note: With the Claude and Claude Agent SDK providers, env vars are passed + correctly to MCP server subprocesses. However, the Copilot provider has a known bug where env vars are not passed to MCP servers. See: https://github.com/github/copilot-sdk/issues/163 """ diff --git a/src/conductor/config/validator.py b/src/conductor/config/validator.py index 7b632e43..0e244477 100644 --- a/src/conductor/config/validator.py +++ b/src/conductor/config/validator.py @@ -1613,13 +1613,15 @@ def _check_agent_tools(agent: AgentDef, provider_name: str, caps: ProviderCapabi * Explicit ``tools: []`` against a provider that still forwards the full workflow-level MCP server set regardless of the per-agent list (``capabilities.mcp_tools=True`` alongside - ``workflow_tools_passthrough=False``, e.g. ``aca`` — the in-container - runner attaches every configured MCP server unconditionally). There - is no allowlist value, empty or not, that provider can honor, so - ``tools: []`` would misleadingly pass validation while every tool - stays attached. When ``mcp_tools=False`` there is nothing to - forward regardless of the list, so ``tools: []`` genuinely disables - all tools and stays valid. + ``workflow_tools_passthrough=False`` — ``aca``, whose in-container + runner attaches every configured MCP server unconditionally, and + ``claude-agent-sdk``, where ``tools: []`` disables only the built-in + CLI tools). There is no allowlist value, empty or not, those + providers can honor, so ``tools: []`` would misleadingly pass + validation while every MCP tool stays attached. This only applies + when the workflow actually declares ``mcp_servers``: with nothing to + forward, ``tools: []`` genuinely disables all tools and stays valid + regardless of ``mcp_tools``. * 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=[...]`` @@ -1633,7 +1635,7 @@ def _check_agent_tools(agent: AgentDef, provider_name: str, caps: ProviderCapabi f"(capabilities.workflow_tools_passthrough=False). Silently " f"granting different tools than declared is a security regression." ) - elif caps.mcp_tools: + elif caps.mcp_tools and workflow_mcp_servers: errors.append( f"Agent '{agent.name}' declares 'tools: []' to disable all tools, but " f"provider '{provider_name}' forwards the full configured MCP server " diff --git a/src/conductor/providers/claude_agent_sdk.py b/src/conductor/providers/claude_agent_sdk.py index 7b7e3816..299ded33 100644 --- a/src/conductor/providers/claude_agent_sdk.py +++ b/src/conductor/providers/claude_agent_sdk.py @@ -3,8 +3,11 @@ from __future__ import annotations import asyncio +import contextlib import json import logging +import os +import tempfile import time from typing import TYPE_CHECKING, Any, Final, cast @@ -101,6 +104,175 @@ def _build_output_format(output: dict[str, OutputField]) -> dict[str, Any]: # in pyproject.toml. _DEFAULT_MODEL: Final[str] = "claude-sonnet-4-5" +# Sentinel meaning "expose every tool this server offers" in +# ``MCPServerDef.tools``. Any other value is a narrowing filter the SDK has no +# way to express — see :func:`_translate_mcp_servers`. +_ALL_TOOLS: Final[str] = "*" + + +def _translate_mcp_servers(mcp_servers: dict[str, Any]) -> dict[str, Any]: + """Translate Conductor MCP server configs into the SDK's config shapes. + + The input is the already-resolved mapping built by + :func:`conductor.cli.run._build_mcp_servers` — ``env`` values have been + expanded from the process environment and any OAuth ``Authorization`` + header has been fetched by the time it reaches us. Output matches the + SDK's ``McpStdioServerConfig`` / ``McpHttpServerConfig`` / + ``McpSSEServerConfig`` TypedDicts. + + Two Conductor fields have no SDK counterpart and are handled differently + on purpose: + + * ``tools`` — a per-server allowlist. ``["*"]`` (the default) means "no + filter" and is simply dropped. Any narrowing value is **refused**: + ignoring it would hand the model more tools than the workflow declared, + the same security regression that justifies refusing the per-agent + ``tools:`` allowlist elsewhere in this provider. + * ``timeout`` — dropped with a warning. Unlike a tool filter, losing a + timeout cannot widen tool access, so it does not warrant a hard failure. + + Args: + mcp_servers: Mapping of server name to resolved Conductor config. + + Returns: + Mapping of server name to SDK-shaped config dict. + + Raises: + ProviderError: If a server declares a narrowing ``tools`` filter, + omits a field its type requires, or carries a type this provider + cannot translate. + """ + translated: dict[str, Any] = {} + + for name, config in mcp_servers.items(): + server_type = config.get("type") or "stdio" + + tools = config.get("tools") + if tools is not None and list(tools) != [_ALL_TOOLS]: + raise ProviderError( + f"MCP server '{name}' declares a tool filter tools={list(tools)!r}, " + "but claude-agent-sdk cannot enforce per-server tool filters " + "(the SDK's MCP config has no equivalent field). Forwarding the " + "server unfiltered would grant more tools than declared.", + suggestion=( + f"Set 'tools: [\"*\"]' on MCP server '{name}' to accept every " + "tool it offers, or use the 'copilot' provider for agents that " + "need per-server tool filtering." + ), + # Config errors never become valid on a retry. Set explicitly: + # the default heuristic sniffs the message for "timeout" / + # "connection", which user-controlled server and tool names + # could otherwise trip. + is_retryable=False, + ) + + if config.get("timeout") is not None: + logger.warning( + "MCP server '%s' sets timeout=%s, which claude-agent-sdk does not " + "support; the CLI's own default will apply instead.", + name, + config["timeout"], + ) + + if server_type == "stdio": + command = config.get("command") + if not command: + raise ProviderError( + f"MCP server '{name}' is type 'stdio' but declares no 'command'.", + suggestion=f"Add a 'command:' to MCP server '{name}'.", + is_retryable=False, + ) + entry: dict[str, Any] = {"type": "stdio", "command": command} + if config.get("args"): + entry["args"] = list(config["args"]) + if config.get("env"): + entry["env"] = dict(config["env"]) + elif server_type in ("http", "sse"): + url = config.get("url") + if not url: + raise ProviderError( + f"MCP server '{name}' is type '{server_type}' but declares no 'url'.", + suggestion=f"Add a 'url:' to MCP server '{name}'.", + is_retryable=False, + ) + entry = {"type": server_type, "url": url} + if config.get("headers"): + entry["headers"] = dict(config["headers"]) + else: + raise ProviderError( + f"MCP server '{name}' has unsupported type '{server_type}' for " + "claude-agent-sdk (expected 'stdio', 'http', or 'sse').", + is_retryable=False, + ) + + translated[name] = entry + + return translated + + +def _write_mcp_config(servers: dict[str, Any]) -> str: + """Write ``servers`` to a private temp file and return its path. + + The SDK serializes a ``mcp_servers`` *dict* straight into a + ``--mcp-config `` command-line argument, which would publish resolved + stdio ``env`` values and http/sse ``Authorization`` headers to anyone who + can read ``/proc//cmdline``. Passing a path instead keeps those + secrets in a file only the current user can read. + + ``tempfile.mkstemp`` creates the file with mode ``0600`` and ``O_EXCL``, so + the secrets are never briefly world-readable. + + The payload uses the CLI's ``{"mcpServers": {...}}`` envelope — a bare + mapping is rejected with "mcpServers: Invalid input: expected record, + received undefined". + + Args: + servers: Already-translated, SDK-shaped server configs. + + Returns: + Absolute path to the config file. The caller owns its removal. + """ + fd, path = tempfile.mkstemp(prefix="conductor-mcp-", suffix=".json") + try: + # os.fdopen takes ownership of fd only once it returns; close fd + # ourselves if it raises, or the descriptor leaks. + try: + handle = os.fdopen(fd, "w", encoding="utf-8") + except BaseException: + os.close(fd) + raise + with handle: + json.dump({"mcpServers": servers}, handle) + except BaseException: + # Includes KeyboardInterrupt mid-write: a partial secrets file is + # worse than none. + _remove_mcp_config(path) + raise + return path + + +def _remove_mcp_config(path: str) -> None: + """Delete an MCP config file written by :func:`_write_mcp_config`. + + Best-effort: a cleanup failure must never mask the error that is already + propagating, so removal problems are reported and swallowed. + + Args: + path: Path returned by :func:`_write_mcp_config`. + """ + try: + os.unlink(path) + except OSError: + # WARNING, not DEBUG: this file holds resolved MCP credentials, and + # the user is the only one who can clean it up. Conductor installs no + # logging handlers, so DEBUG here would reach nobody. + logger.warning( + "Failed to remove MCP config file %s; it contains resolved MCP " + "credentials and should be deleted manually.", + path, + exc_info=True, + ) + class ClaudeAgentSdkProvider(AgentProvider): """Claude Agent SDK provider. @@ -112,12 +284,18 @@ class ClaudeAgentSdkProvider(AgentProvider): CAPABILITIES = ProviderCapabilities( tier="experimental", - # MCP servers are rejected at the factory (no translation from - # Conductor's MCP config to the SDK's MCP dict is implemented). - mcp_tools=False, - # Per-agent ``tools: []`` is honored (disables all tools). Per-agent - # ``tools: []`` is refused loudly at execute time because - # workflow tool names do not translate to Claude CLI tool IDs. + # Workflow-level ``runtime.mcp_servers`` are translated to the SDK's + # own MCP config shapes and passed via ``ClaudeAgentOptions``. Only + # declared servers attach: ``strict_mcp_config`` is always set, so + # ambient project/user MCP config is ignored. A narrowing per-server + # ``tools:`` filter has no SDK equivalent and is refused. + mcp_tools=True, + # Per-agent ``tools: []`` disables all *built-in* tools; declared MCP + # servers still attach (the SDK has no per-request MCP toggle), which + # is why the validator rejects ``tools: []`` alongside ``mcp_servers:`` + # for this provider. Per-agent ``tools: []`` is refused loudly + # at execute time because workflow tool names do not translate to + # Claude CLI tool IDs. # The capability records the strict end of that contract — when the # user declares a non-empty allowlist, the validator surfaces it as # an error before runtime hits the refusal. @@ -151,12 +329,13 @@ class ClaudeAgentSdkProvider(AgentProvider): # No global mutable state shared across calls — the SDK spawns # an independent subprocess per query() invocation. concurrent_safe=True, - # MCP servers are rejected at the factory (mcp_tools=False), so - # there is nothing to scope by directory — the SDK CLI manages its - # own cwd. Declared False so ``conductor validate`` errors when a - # workflow sets ``working_dir`` against this provider. + # The provider never forwards the engine-resolved working directory + # to ``ClaudeAgentOptions.cwd``, so an agent's ``working_dir`` would + # be silently ignored (both for the CLI itself and for any stdio MCP + # servers it spawns). Declared False so ``conductor validate`` errors + # instead of lying about where the agent runs. working_dir=False, - upstream_pin="claude-agent-sdk>=0.1.0", + upstream_pin="claude-agent-sdk>=0.2.82", maintainer="@lesandiz (best-effort)", ) @@ -165,16 +344,21 @@ def __init__( model: str | None = None, max_turns: int | None = None, max_session_seconds: float | None = None, + mcp_servers: dict[str, Any] | None = None, ) -> None: if not CLAUDE_AGENT_SDK_AVAILABLE: raise ProviderError( "Claude Agent SDK not installed", - suggestion="Install with: uv add 'claude-agent-sdk>=0.1.0'", + suggestion="Install with: uv add 'claude-agent-sdk>=0.2.82'", ) self._default_model = model or _DEFAULT_MODEL self._default_max_turns = max_turns if max_turns is not None else 50 self._max_session_seconds = max_session_seconds + # Translate once, here, rather than per execute() call. Providers are + # constructed lazily, so an untranslatable server config surfaces when + # the first agent on this provider runs — not at `conductor validate`. + self._mcp_servers = _translate_mcp_servers(mcp_servers) if mcp_servers else {} async def execute( self, @@ -225,6 +409,11 @@ async def execute( max_turns=max_turns, permission_mode=permission_mode, tools=sdk_tools, + # Unconditional, including when this workflow declares no servers: + # the CLI would otherwise load project .mcp.json, user-global, and + # plugin-provided servers, and permission_mode bypasses approval + # for whatever they expose. Only declared servers may attach. + strict_mcp_config=True, ) content_parts: list[str] = [] @@ -237,7 +426,17 @@ async def execute( pending_tools: dict[str, str] = {} session_start = time.monotonic() + # Written inside the try below, never before it: the file holds + # resolved MCP credentials, so every path out of this method must + # reach the finally that reclaims it. + mcp_config_path: str | None = None + agen: Any = None + try: + if self._mcp_servers: + mcp_config_path = _write_mcp_config(self._mcp_servers) + options.mcp_servers = mcp_config_path + # Signal "awaiting model" before entering the SDK iterator: the # SDK is about to make the first model call. Dashboards use this # to show a "waiting for model" spinner. @@ -248,7 +447,8 @@ async def execute( {"turn": "awaiting_model"}, ) - async for message in query(prompt=rendered_prompt, options=options): + agen = query(prompt=rendered_prompt, options=options) + async for message in agen: if interrupt_signal is not None and interrupt_signal.is_set(): return self._build_output( content_parts, @@ -367,6 +567,19 @@ async def execute( suggestion=_classify_error_suggestion(e), is_retryable=_is_retryable_exception(e), ) from e + finally: + # Order matters: close the SDK iterator first so the `claude` + # subprocess is gone before its config file disappears. Abandoning + # the generator (the interrupt path returns mid-loop) otherwise + # defers teardown to the GC, and on Windows unlinking a file the + # live subprocess still holds open raises PermissionError. + if agen is not None: + aclose = getattr(agen, "aclose", None) + if aclose is not None: + with contextlib.suppress(Exception): + await aclose() + if mcp_config_path is not None: + _remove_mcp_config(mcp_config_path) return self._build_output( content_parts, diff --git a/src/conductor/providers/factory.py b/src/conductor/providers/factory.py index 9012de7a..6dbba789 100644 --- a/src/conductor/providers/factory.py +++ b/src/conductor/providers/factory.py @@ -174,25 +174,12 @@ async def create_provider( if not CLAUDE_AGENT_SDK_AVAILABLE: raise ProviderError( "Claude Agent SDK provider requires claude-agent-sdk package", - suggestion="Install with: uv add 'claude-agent-sdk>=0.1.0'", + suggestion="Install with: uv add 'claude-agent-sdk>=0.2.82'", ) # claude-agent-sdk delegates the agentic loop to the underlying - # `claude` CLI, which currently does not expose hooks for - # workflow-level MCP servers, sampling temperature, or token - # caps. Silently dropping any of these would either change - # behavior (mcp tools the workflow expects suddenly missing) - # or quietly violate user intent (temperature/max_tokens). - # Refuse loudly until proper plumbing exists. - if mcp_servers: - raise ProviderError( - "claude-agent-sdk does not support workflow MCP servers " - f"(received {sorted(mcp_servers)!r}).", - suggestion=( - "Remove `runtime.mcp_servers` for this workflow, or " - "use the `copilot` or `claude` provider for agents " - "that need MCP tools." - ), - ) + # `claude` CLI, which exposes no hooks for sampling temperature or + # token caps. Silently dropping either would quietly violate user + # intent, so refuse loudly until proper plumbing exists. if temperature is not None: raise ProviderError( f"claude-agent-sdk does not support `temperature` (received {temperature!r}).", @@ -211,6 +198,7 @@ async def create_provider( model=default_model, max_turns=max_agent_iterations, max_session_seconds=max_session_seconds, + mcp_servers=mcp_servers, ) case "aca": if not AZURE_IDENTITY_AVAILABLE: diff --git a/tests/test_config/test_validator_capabilities.py b/tests/test_config/test_validator_capabilities.py index b177cf27..7e793670 100644 --- a/tests/test_config/test_validator_capabilities.py +++ b/tests/test_config/test_validator_capabilities.py @@ -176,10 +176,23 @@ def test_empty_tools_list_against_no_passthrough_with_mcp_tools_errors( patch_caps({"copilot": _caps(workflow_tools_passthrough=False, mcp_tools=True)}) config = _build_workflow( agents=[AgentDef(name="a", prompt="hi", tools=[])], + mcp_servers={"docs": MCPServerDef(command="docs-server")}, ) with pytest.raises(ConfigurationError, match="there is no way to disable tools"): validate_workflow_config(config) + def test_empty_tools_list_with_mcp_tools_but_no_servers_is_allowed( + self, patch_caps: Any + ) -> None: + """With no ``mcp_servers`` declared there is nothing to forward, so + ``tools: []`` genuinely disables every tool and must stay valid even + against an ``mcp_tools=True`` provider.""" + patch_caps({"copilot": _caps(workflow_tools_passthrough=False, mcp_tools=True)}) + config = _build_workflow( + agents=[AgentDef(name="a", prompt="hi", tools=[])], + ) + validate_workflow_config(config) # no raise + def test_non_empty_tools_list_against_no_passthrough_errors(self, patch_caps: Any) -> None: patch_caps({"copilot": _caps(workflow_tools_passthrough=False)}) config = _build_workflow( @@ -266,6 +279,49 @@ def _for_each_config( ], ) + def _for_each_mcp_config( + self, + *, + inline: AgentDef, + mcp_servers: dict[str, MCPServerDef] | None = None, + ) -> WorkflowConfig: + # The entry agent OMITS ``tools:`` (and there is no workflow-level + # ``tools:``), so it cannot trip the check itself — isolating the + # assertion to the inline agent. + return _build_workflow( + agents=[AgentDef(name="entry", prompt="hi")], + mcp_servers=mcp_servers, + for_each=[ + ForEachDef( + name="loop", + type="for_each", + source="entry.output.items", + **{"as": "item"}, + agent=inline, + ) + ], + ) + + def test_inline_empty_tools_with_mcp_servers_errors(self, patch_caps: Any) -> None: + """Mirror of the top-level case: ``tools: []`` cannot be honored when + the provider attaches every declared MCP server regardless.""" + patch_caps({"copilot": _caps(workflow_tools_passthrough=False, mcp_tools=True)}) + config = self._for_each_mcp_config( + inline=AgentDef(name="inner", prompt="{{ item }}", tools=[]), + mcp_servers={"docs": MCPServerDef(command="docs-server")}, + ) + with pytest.raises(ConfigurationError, match="there is no way to disable tools"): + validate_workflow_config(config) + + def test_inline_empty_tools_without_mcp_servers_passes(self, patch_caps: Any) -> None: + """Mirror of the top-level case: nothing to forward -> ``tools: []`` + genuinely disables everything and stays valid.""" + patch_caps({"copilot": _caps(workflow_tools_passthrough=False, mcp_tools=True)}) + config = self._for_each_mcp_config( + inline=AgentDef(name="inner", prompt="{{ item }}", tools=[]), + ) + validate_workflow_config(config) # no raise + 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, mcp_tools=False)}) @@ -1332,7 +1388,12 @@ class TestAcaRealCapabilitiesCrossCheck: it as the workflow default. """ - def _aca_workflow(self, *, agents: list[AgentDef]) -> WorkflowConfig: + def _aca_workflow( + self, + *, + agents: list[AgentDef], + mcp_servers: dict[str, MCPServerDef] | None = None, + ) -> WorkflowConfig: from conductor.config.schema import ProviderSettings return WorkflowConfig( @@ -1340,7 +1401,8 @@ def _aca_workflow(self, *, agents: list[AgentDef]) -> WorkflowConfig: name="test", entry_point=agents[0].name, runtime=RuntimeConfig( - provider=ProviderSettings(name="aca", pool_endpoint="https://pool.example.com") + provider=ProviderSettings(name="aca", pool_endpoint="https://pool.example.com"), + mcp_servers=mcp_servers or {}, ), ), agents=agents, @@ -1375,10 +1437,20 @@ def test_empty_tools_rejected_against_real_aca_capabilities(self, patch_caps: An patch_caps({"aca": AcaRuntimeProvider.CAPABILITIES}) config = self._aca_workflow( agents=[AgentDef(name="a", prompt="hi", tools=[])], + mcp_servers={"docs": MCPServerDef(command="docs-server")}, ) with pytest.raises(ConfigurationError, match="there is no way to disable tools"): validate_workflow_config(config) + def test_empty_tools_without_mcp_servers_passes_against_aca(self, patch_caps: Any) -> None: + """With no ``mcp_servers`` declared the runner has nothing to attach, + so ``tools: []`` genuinely disables every tool and must validate.""" + from conductor.providers.aca import AcaRuntimeProvider + + patch_caps({"aca": AcaRuntimeProvider.CAPABILITIES}) + config = self._aca_workflow(agents=[AgentDef(name="a", prompt="hi", tools=[])]) + validate_workflow_config(config) # no raise + def test_no_tools_against_real_aca_capabilities_passes(self, patch_caps: Any) -> None: """Positive control: omitting ``tools:`` (agent gets the provider's default preset) never trips the passthrough gate.""" @@ -1421,3 +1493,69 @@ def test_sandbox_working_dir_against_real_aca_capabilities_passes( ], ) validate_workflow_config(config) # no raise + + +class TestClaudeAgentSdkRealCapabilitiesCrossCheck: + """#335 flipped ``mcp_tools`` to True for ``claude-agent-sdk``. Pin what + that means for the validator against the REAL descriptor, not a synthetic + one — mirrors ``TestAcaRealCapabilitiesCrossCheck``. + """ + + def _sdk_workflow( + self, + *, + agents: list[AgentDef], + mcp_servers: dict[str, MCPServerDef] | None = None, + ) -> WorkflowConfig: + from conductor.config.schema import ProviderSettings + + return WorkflowConfig( + workflow=WorkflowDef( + name="test", + entry_point=agents[0].name, + runtime=RuntimeConfig( + provider=ProviderSettings(name="claude-agent-sdk"), + mcp_servers=mcp_servers or {}, + ), + ), + agents=agents, + ) + + def _patch(self, patch_caps: Any) -> None: + from conductor.providers.claude_agent_sdk import ClaudeAgentSdkProvider + + patch_caps({"claude-agent-sdk": ClaudeAgentSdkProvider.CAPABILITIES}) + + def test_mcp_servers_are_accepted(self, patch_caps: Any) -> None: + """The whole point of #335: declaring MCP servers no longer fails.""" + self._patch(patch_caps) + config = self._sdk_workflow( + agents=[AgentDef(name="a", prompt="hi")], + mcp_servers={"docs": MCPServerDef(command="docs-server")}, + ) + validate_workflow_config(config) # no raise + + def test_empty_tools_with_mcp_servers_is_rejected(self, patch_caps: Any) -> None: + """``tools: []`` disables only the built-in preset; declared MCP + servers still attach, so the combination cannot be honored.""" + self._patch(patch_caps) + config = self._sdk_workflow( + agents=[AgentDef(name="a", prompt="hi", tools=[])], + mcp_servers={"docs": MCPServerDef(command="docs-server")}, + ) + with pytest.raises(ConfigurationError, match="there is no way to disable tools"): + validate_workflow_config(config) + + def test_empty_tools_without_mcp_servers_is_allowed(self, patch_caps: Any) -> None: + """Nothing to attach -> ``tools: []`` genuinely disables everything. + Guards examples/experimental-claude-agent-sdk.yaml.""" + self._patch(patch_caps) + config = self._sdk_workflow(agents=[AgentDef(name="a", prompt="hi", tools=[])]) + validate_workflow_config(config) # no raise + + def test_non_empty_tools_is_still_rejected(self, patch_caps: Any) -> None: + """``workflow_tools_passthrough`` stays False — #335 did not change it.""" + self._patch(patch_caps) + config = self._sdk_workflow(agents=[AgentDef(name="a", prompt="hi", tools=["search"])]) + with pytest.raises(ConfigurationError, match="does not honor per-agent tool allowlists"): + validate_workflow_config(config) diff --git a/tests/test_providers/test_claude_agent_sdk.py b/tests/test_providers/test_claude_agent_sdk.py index d655630f..1ac71183 100644 --- a/tests/test_providers/test_claude_agent_sdk.py +++ b/tests/test_providers/test_claude_agent_sdk.py @@ -3,7 +3,14 @@ from __future__ import annotations import asyncio +import glob +import json +import logging +import os +import stat +import tempfile from dataclasses import dataclass, field +from pathlib import Path from unittest.mock import Mock, patch import pytest @@ -25,7 +32,12 @@ from conductor.config.schema import AgentDef, OutputField # noqa: E402 from conductor.exceptions import ProviderError # noqa: E402 -from conductor.providers.claude_agent_sdk import ClaudeAgentSdkProvider # noqa: E402 +from conductor.providers.claude_agent_sdk import ( # noqa: E402 + ClaudeAgentSdkProvider, + _remove_mcp_config, + _translate_mcp_servers, + _write_mcp_config, +) def _assistant( @@ -1683,3 +1695,395 @@ async def fake_query(**kwargs): event_callback=boom, ) assert output.content == {"response": "hi"} + + +def _mcp_temp_files() -> set[str]: + """Snapshot the MCP config files currently on disk in the temp dir.""" + return set(glob.glob(os.path.join(tempfile.gettempdir(), "conductor-mcp-*.json"))) + + +class TestMcpServerTranslation: + """Conductor MCP configs must map onto the SDK's own config shapes (#335).""" + + def test_stdio_server_translates(self) -> None: + translated = _translate_mcp_servers( + { + "docs": { + "type": "stdio", + "command": "docs-server", + "args": ["--port", "1234"], + "env": {"API_KEY": "secret"}, + "tools": ["*"], + } + } + ) + assert translated == { + "docs": { + "type": "stdio", + "command": "docs-server", + "args": ["--port", "1234"], + "env": {"API_KEY": "secret"}, + } + } + + def test_stdio_omits_empty_args_and_env(self) -> None: + """Empty collections are dropped rather than sent as empty lists/dicts.""" + translated = _translate_mcp_servers( + {"docs": {"type": "stdio", "command": "docs-server", "args": [], "tools": ["*"]}} + ) + assert translated == {"docs": {"type": "stdio", "command": "docs-server"}} + + def test_missing_type_defaults_to_stdio(self) -> None: + """Matches MCPServerDef.type's default, so a config that omits it is + not silently reshaped.""" + translated = _translate_mcp_servers({"docs": {"command": "docs-server"}}) + assert translated == {"docs": {"type": "stdio", "command": "docs-server"}} + + @pytest.mark.parametrize("server_type", ["http", "sse"]) + def test_remote_server_translates(self, server_type: str) -> None: + translated = _translate_mcp_servers( + { + "remote": { + "type": server_type, + "url": "https://mcp.example.com/tools", + "headers": {"Authorization": "Bearer tok"}, + "tools": ["*"], + } + } + ) + assert translated == { + "remote": { + "type": server_type, + "url": "https://mcp.example.com/tools", + "headers": {"Authorization": "Bearer tok"}, + } + } + + def test_remote_omits_empty_headers(self) -> None: + translated = _translate_mcp_servers( + {"remote": {"type": "http", "url": "https://x.test", "headers": {}}} + ) + assert translated == {"remote": {"type": "http", "url": "https://x.test"}} + + def test_narrowing_tool_filter_is_refused(self) -> None: + """Ignoring a per-server allowlist would grant more tools than declared.""" + with pytest.raises(ProviderError, match="cannot enforce per-server tool filters"): + _translate_mcp_servers( + {"docs": {"type": "stdio", "command": "docs-server", "tools": ["search"]}} + ) + + def test_empty_tool_filter_is_refused(self) -> None: + """``tools: []`` is still a narrowing filter the SDK cannot express.""" + with pytest.raises(ProviderError, match="cannot enforce per-server tool filters"): + _translate_mcp_servers( + {"docs": {"type": "stdio", "command": "docs-server", "tools": []}} + ) + + def test_unsupported_type_is_refused(self) -> None: + with pytest.raises(ProviderError, match="unsupported type 'grpc'"): + _translate_mcp_servers({"docs": {"type": "grpc", "url": "https://x.test"}}) + + def test_timeout_is_dropped_with_a_warning(self, caplog: pytest.LogCaptureFixture) -> None: + """A dropped timeout cannot widen tool access, so it warns instead of raising.""" + with caplog.at_level(logging.WARNING, logger="conductor.providers.claude_agent_sdk"): + translated = _translate_mcp_servers( + {"docs": {"type": "stdio", "command": "docs-server", "timeout": 5000}} + ) + assert translated == {"docs": {"type": "stdio", "command": "docs-server"}} + # Name the server and the dropped value, so the warning is actionable. + assert "docs" in caplog.text + assert "5000" in caplog.text + + @patch("conductor.providers.claude_agent_sdk.CLAUDE_AGENT_SDK_AVAILABLE", True) + def test_provider_translates_at_construction(self) -> None: + """Translation happens once, when the provider is built -- not per + execute() call. Providers are constructed lazily, so this surfaces on + the first agent that uses the provider, not at validate time.""" + with pytest.raises(ProviderError, match="cannot enforce per-server tool filters"): + ClaudeAgentSdkProvider( + mcp_servers={"docs": {"type": "stdio", "command": "d", "tools": ["search"]}} + ) + + +class TestMcpConfigFile: + """MCP secrets must reach the CLI via a private file, never via argv (#335).""" + + def test_config_file_is_owner_only_and_wrapped(self) -> None: + servers = {"docs": {"type": "stdio", "command": "docs-server"}} + path = _write_mcp_config(servers) + try: + mode = stat.S_IMODE(os.stat(path).st_mode) + payload = json.loads(Path(path).read_text(encoding="utf-8")) + finally: + _remove_mcp_config(path) + + # 0600: resolved env values and Authorization headers live in here. + assert mode == 0o600 + # The CLI rejects a bare mapping: "mcpServers: Invalid input: + # expected record, received undefined". + assert payload == {"mcpServers": servers} + + def test_remove_deletes_the_file(self) -> None: + path = _write_mcp_config({"docs": {"type": "stdio", "command": "d"}}) + assert Path(path).exists() + _remove_mcp_config(path) + assert not Path(path).exists() + + def test_write_cleans_up_when_serialization_fails(self) -> None: + """A partial secrets file is worse than none, so the write guard must + remove it before re-raising.""" + created: list[str] = [] + real_mkstemp = tempfile.mkstemp + + def spy(*args, **kwargs): + fd, path = real_mkstemp(*args, **kwargs) + created.append(path) + return fd, path + + # A set is not JSON-serializable -> json.dump raises mid-write. + with patch.object(tempfile, "mkstemp", spy), pytest.raises(TypeError): + _write_mcp_config({"docs": {"type": "stdio", "command": {"unserializable"}}}) + + assert created and not Path(created[0]).exists() + + def test_remove_is_best_effort(self, caplog: pytest.LogCaptureFixture) -> None: + """Cleanup failure must never mask the error already propagating -- but + it must still be visible, because the file holds credentials.""" + with caplog.at_level(logging.WARNING, logger="conductor.providers.claude_agent_sdk"): + _remove_mcp_config("/nonexistent/conductor-mcp-does-not-exist.json") + assert "should be deleted manually" in caplog.text + + +class TestMcpOptionsWiring: + """``execute`` must hand the SDK a config path and isolate ambient config.""" + + @patch("conductor.providers.claude_agent_sdk.CLAUDE_AGENT_SDK_AVAILABLE", True) + async def test_execute_passes_config_path_and_strict_flag(self) -> None: + captured: dict = {} + seen_payload: dict = {} + + async def fake_query(**kwargs): + options = kwargs["options"] + captured["mcp_servers"] = options.mcp_servers + captured["strict"] = options.strict_mcp_config + # The file must still exist while the SDK is consuming it. + seen_payload.update(json.loads(Path(options.mcp_servers).read_text())) + yield _result(result="ok") + + with patch("conductor.providers.claude_agent_sdk.query", fake_query): + provider = ClaudeAgentSdkProvider( + mcp_servers={"docs": {"type": "stdio", "command": "docs-server"}} + ) + await provider.execute( + agent=AgentDef(name="t", prompt="hi"), context={}, rendered_prompt="hi" + ) + + # A path, not a dict — a dict would be serialized into the CLI's argv. + assert isinstance(captured["mcp_servers"], str) + assert captured["strict"] is True + assert seen_payload == {"mcpServers": {"docs": {"type": "stdio", "command": "docs-server"}}} + assert not Path(captured["mcp_servers"]).exists() + + @patch("conductor.providers.claude_agent_sdk.CLAUDE_AGENT_SDK_AVAILABLE", True) + async def test_no_mcp_servers_leaves_sdk_defaults(self) -> None: + captured: dict = {} + + async def fake_query(**kwargs): + captured["mcp_servers"] = kwargs["options"].mcp_servers + captured["strict"] = kwargs["options"].strict_mcp_config + yield _result(result="ok") + + with patch("conductor.providers.claude_agent_sdk.query", fake_query): + provider = ClaudeAgentSdkProvider() + await provider.execute( + agent=AgentDef(name="t", prompt="hi"), context={}, rendered_prompt="hi" + ) + + assert captured["mcp_servers"] == {} + # Unconditional: without it the CLI would still load project + # .mcp.json / user-global / plugin servers. + assert captured["strict"] is True + + @patch("conductor.providers.claude_agent_sdk.CLAUDE_AGENT_SDK_AVAILABLE", True) + async def test_config_file_removed_when_query_raises(self) -> None: + captured: dict = {} + + async def fake_query(**kwargs): + captured["path"] = kwargs["options"].mcp_servers + raise RuntimeError("sdk exploded") + yield # pragma: no cover - unreachable, keeps this an async generator + + with patch("conductor.providers.claude_agent_sdk.query", fake_query): + provider = ClaudeAgentSdkProvider( + mcp_servers={"docs": {"type": "stdio", "command": "docs-server"}} + ) + with pytest.raises(ProviderError): + await provider.execute( + agent=AgentDef(name="t", prompt="hi"), context={}, rendered_prompt="hi" + ) + + assert not Path(captured["path"]).exists() + + @patch("conductor.providers.claude_agent_sdk.CLAUDE_AGENT_SDK_AVAILABLE", True) + async def test_config_file_removed_on_interrupt_return(self) -> None: + """The interrupt path returns early from inside the loop — still cleans up.""" + captured: dict = {} + interrupt = asyncio.Event() + interrupt.set() + + async def fake_query(**kwargs): + captured["path"] = kwargs["options"].mcp_servers + yield _assistant(content=[TextBlock(text="partial")]) + + with patch("conductor.providers.claude_agent_sdk.query", fake_query): + provider = ClaudeAgentSdkProvider( + mcp_servers={"docs": {"type": "stdio", "command": "docs-server"}} + ) + output = await provider.execute( + agent=AgentDef(name="t", prompt="hi"), + context={}, + rendered_prompt="hi", + interrupt_signal=interrupt, + ) + + assert output.partial is True + assert not Path(captured["path"]).exists() + + @patch("conductor.providers.claude_agent_sdk.CLAUDE_AGENT_SDK_AVAILABLE", True) + async def test_no_config_file_leaks_when_options_construction_fails(self) -> None: + """Regression: the config was once written BEFORE the try, so anything + raising in between left a file full of resolved credentials on disk -- + once per retry attempt.""" + before = _mcp_temp_files() + + deep = OutputField(type="string") + for _ in range(12): + deep = OutputField(type="object", properties={"x": deep}) + + provider = ClaudeAgentSdkProvider( + mcp_servers={"docs": {"type": "stdio", "command": "d", "env": {"K": "secret"}}} + ) + with pytest.raises(ProviderError, match="nesting exceeds 10 levels"): + await provider.execute( + agent=AgentDef(name="t", prompt="hi", output={"deep": deep}), + context={}, + rendered_prompt="hi", + ) + + assert _mcp_temp_files() == before + + @patch("conductor.providers.claude_agent_sdk.CLAUDE_AGENT_SDK_AVAILABLE", True) + async def test_secrets_reach_the_file_but_not_the_options(self) -> None: + """The whole point of the path indirection: credentials live in the + file, never in an object the SDK serializes into argv.""" + captured: dict = {} + + async def fake_query(**kwargs): + options = kwargs["options"] + captured["mcp"] = options.mcp_servers + captured["payload"] = Path(options.mcp_servers).read_text(encoding="utf-8") + yield _result(result="ok") + + with patch("conductor.providers.claude_agent_sdk.query", fake_query): + provider = ClaudeAgentSdkProvider( + mcp_servers={ + "remote": { + "type": "http", + "url": "https://mcp.example.com", + "headers": {"Authorization": "Bearer s3cret"}, + } + } + ) + await provider.execute( + agent=AgentDef(name="t", prompt="hi"), context={}, rendered_prompt="hi" + ) + + assert "s3cret" in captured["payload"] + # The options field is a path string, so nothing the SDK turns into a + # command-line argument carries the secret. + assert "s3cret" not in captured["mcp"] + + @patch("conductor.providers.claude_agent_sdk.CLAUDE_AGENT_SDK_AVAILABLE", True) + async def test_concurrent_executions_get_independent_config_files(self) -> None: + """concurrent_safe=True now covers a filesystem resource: one agent's + cleanup must not delete a sibling's live config.""" + paths: list[str] = [] + + async def fake_query(**kwargs): + path = kwargs["options"].mcp_servers + paths.append(path) + await asyncio.sleep(0.05) # overlap with the other executions + assert Path(path).exists(), f"{path} deleted while still in use" + yield _result(result="ok") + + with patch("conductor.providers.claude_agent_sdk.query", fake_query): + provider = ClaudeAgentSdkProvider( + mcp_servers={"docs": {"type": "stdio", "command": "docs-server"}} + ) + await asyncio.gather( + *( + provider.execute( + agent=AgentDef(name=f"a{i}", prompt="hi"), + context={}, + rendered_prompt="hi", + ) + for i in range(5) + ) + ) + + assert len(set(paths)) == 5 + assert not any(Path(p).exists() for p in paths) + + @patch("conductor.providers.claude_agent_sdk.CLAUDE_AGENT_SDK_AVAILABLE", True) + async def test_empty_tools_still_attaches_mcp_servers(self) -> None: + """``tools: []`` disables the CLI preset but does NOT detach MCP + servers -- the SDK has no per-request MCP toggle. ``conductor validate`` + is the only guard and ``conductor run`` never calls it, so pin the + runtime behavior. If this ever becomes "detach the servers too", change + this test deliberately rather than by accident.""" + captured: dict = {} + + async def fake_query(**kwargs): + captured["tools"] = kwargs["options"].tools + captured["mcp"] = kwargs["options"].mcp_servers + yield _result(result="ok") + + with patch("conductor.providers.claude_agent_sdk.query", fake_query): + provider = ClaudeAgentSdkProvider( + mcp_servers={"docs": {"type": "stdio", "command": "docs-server"}} + ) + await provider.execute( + agent=AgentDef(name="t", prompt="hi", tools=[]), + context={}, + rendered_prompt="hi", + tools=[], + ) + + assert captured["tools"] == [] + assert isinstance(captured["mcp"], str) + + +class TestMcpRequiredFields: + """Malformed configs must produce actionable errors, not bare KeyErrors. + + ``MCPServerDef`` guards the YAML path, but ``create_provider`` is public + API -- a library caller can pass these dicts directly. + """ + + def test_stdio_without_command_is_refused(self) -> None: + with pytest.raises(ProviderError, match="declares no 'command'"): + _translate_mcp_servers({"docs": {"type": "stdio"}}) + + @pytest.mark.parametrize("server_type", ["http", "sse"]) + def test_remote_without_url_is_refused(self, server_type: str) -> None: + with pytest.raises(ProviderError, match="declares no 'url'"): + _translate_mcp_servers({"docs": {"type": server_type}}) + + def test_config_errors_are_not_retryable(self) -> None: + """A server named e.g. 'timeout-probe' must not flip the retryability + heuristic, which sniffs the message for 'timeout'/'connection'.""" + with pytest.raises(ProviderError) as exc: + _translate_mcp_servers( + {"timeout-probe": {"type": "stdio", "command": "d", "tools": ["connection"]}} + ) + assert exc.value.is_retryable is False diff --git a/tests/test_providers/test_factory.py b/tests/test_providers/test_factory.py index 888f8eb3..11f01b0c 100644 --- a/tests/test_providers/test_factory.py +++ b/tests/test_providers/test_factory.py @@ -253,20 +253,50 @@ async def test_max_session_seconds_preserves_other_idle_recovery_defaults(self) class TestClaudeAgentSdkFactoryRejections: """Factory rejects workflow features claude-agent-sdk does not honor (#241 / A2). - Silently dropping mcp_servers, temperature, or max_tokens at the factory - boundary is a parity violation: agents that expect those features end up - running with different behavior than declared. Refuse loudly until proper - plumbing exists. + Silently dropping temperature or max_tokens at the factory boundary is a + parity violation: agents that expect those features end up running with + different behavior than declared. Refuse loudly until proper plumbing + exists. ``mcp_servers`` IS supported as of #335 — it is translated to the + SDK's own MCP config shapes and forwarded. """ @pytest.mark.asyncio - async def test_factory_rejects_mcp_servers(self) -> None: + async def test_factory_forwards_mcp_servers(self) -> None: pytest.importorskip("claude_agent_sdk") - with pytest.raises(ProviderError, match="does not support workflow MCP servers"): + from conductor.providers.claude_agent_sdk import ClaudeAgentSdkProvider + + provider = await create_provider( + "claude-agent-sdk", + validate=False, + mcp_servers={ + "docs": { + "type": "stdio", + "command": "docs-server", + "args": ["--port", "1234"], + # Dropped by the translation: no SDK equivalent. + "tools": ["*"], + "timeout": 5000, + } + }, + ) + assert isinstance(provider, ClaudeAgentSdkProvider) + # Translated to the SDK shape, not stored verbatim. + assert provider._mcp_servers == { + "docs": {"type": "stdio", "command": "docs-server", "args": ["--port", "1234"]} + } + await provider.close() + + @pytest.mark.asyncio + async def test_factory_rejects_per_server_tool_filter(self) -> None: + """A narrowing per-server allowlist has no SDK equivalent — fail fast.""" + pytest.importorskip("claude_agent_sdk") + with pytest.raises(ProviderError, match="cannot enforce per-server tool filters"): await create_provider( "claude-agent-sdk", validate=False, - mcp_servers={"docs": {"command": "docs-server"}}, + mcp_servers={ + "docs": {"type": "stdio", "command": "docs-server", "tools": ["search"]} + }, ) @pytest.mark.asyncio diff --git a/uv.lock b/uv.lock index 623a0f76..8277347b 100644 --- a/uv.lock +++ b/uv.lock @@ -433,7 +433,7 @@ requires-dist = [ { name = "anthropic", specifier = ">=0.77.0,<1.0.0" }, { name = "azure-core", extras = ["aio"], marker = "extra == 'aca'", specifier = ">=1.29.0" }, { name = "azure-identity", marker = "extra == 'aca'", specifier = ">=1.19.0" }, - { name = "claude-agent-sdk", marker = "extra == 'claude-agent-sdk'", specifier = ">=0.1.64" }, + { name = "claude-agent-sdk", marker = "extra == 'claude-agent-sdk'", specifier = ">=0.2.82" }, { name = "fastapi", specifier = ">=0.115.0" }, { name = "github-copilot-sdk", specifier = ">=1.0.0" }, { name = "httpx", specifier = ">=0.27.0" },