From 994ec02dd7fd959b8c1858063f8a4be8f7cd3e23 Mon Sep 17 00:00:00 2001 From: Chris Throup Date: Tue, 8 Sep 2026 16:05:18 +0200 Subject: [PATCH 01/10] feat(claude-agent-sdk): select the project settings tier with settings_dir `working_dir` was doing two unrelated jobs on this provider. The Claude CLI supports MCP Roots and advertises exactly one root -- its cwd -- so a filesystem MCP server discards the directories in its own argv and permits cwd alone. That makes cwd the only handle on what an agent can read, while it is simultaneously the directory the `project` settings tier resolves against. Narrowing cwd onto a target repository to pick up that repository's skills therefore narrowed the agent's MCP root below any sibling path the step still had to read; widening it back lost the repository's conventions. Add a per-agent `settings_dir`, forwarded to `ClaudeAgentOptions.add_dirs`, so the two are separable: the skills of the directory named there are discovered and invocable regardless of cwd, leaving cwd free to stay wide enough for the agent's MCP servers. The split is deliberately partial. A directory named here contributes its `.claude/skills` and nothing else -- not `CLAUDE.md`, `.claude/rules/*.md`, `.claude/settings.json` (so no `env`, no `hooks`) or `.claude/agents`, all of which keep following cwd. It is the skills portion of a project tier, not a cwd-independent way to load one. `add_dirs` also carries an unconditional effect the skills framing hides: per the SDK's own contract it is "additional directories Claude can access", so a `settings_dir` widens the model's built-in Read/Edit/Bash to that tree with no settings tier enabled at all. It does not widen what an MCP server permits. Both effects are documented on the schema field, and `conductor validate` warns when the field is set with no `setting_sources`, since the skills half is then a no-op while the filesystem grant applies. `WorkflowEngine._resolve_agent_directory` is extracted from `_resolve_agent_working_dir` so `working_dir` and `settings_dir` resolve identically and cannot drift. `capabilities.py::settings_dir` gates the field; `config/validator.py` errors against a provider with nowhere to put it rather than dropping it silently. The Roots behaviour every option here rests on is pinned by tests/test_integration/test_mcp_roots_negotiation.py, which drives the real filesystem server with no LLM -- two runs differing only in whether the client advertises `roots`. --- AGENTS.md | 3 +- docs/providers/experimental.md | 2 +- docs/workflow-syntax.md | 111 ++++++++++ examples/claude-agent-sdk-settings-dir.yaml | 134 +++++++++++ src/conductor/config/schema.py | 92 +++++++- src/conductor/config/validator.py | 42 ++++ src/conductor/engine/workflow.py | 90 +++++--- src/conductor/providers/capabilities.py | 13 ++ src/conductor/providers/claude_agent_sdk.py | 24 ++ tests/test_config/test_set_schema.py | 1 + tests/test_config/test_settings_dir_schema.py | 183 +++++++++++++++ tests/test_engine/test_workflow.py | 133 +++++++++++ .../test_mcp_roots_negotiation.py | 193 ++++++++++++++++ tests/test_providers/test_claude_agent_sdk.py | 209 ++++++++++++++++++ 14 files changed, 1196 insertions(+), 34 deletions(-) create mode 100644 examples/claude-agent-sdk-settings-dir.yaml create mode 100644 tests/test_config/test_settings_dir_schema.py create mode 100644 tests/test_integration/test_mcp_roots_negotiation.py diff --git a/AGENTS.md b/AGENTS.md index 51c6124e..be2d36aa 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -431,7 +431,8 @@ Conductor: - The SDK applies it as the `claude` subprocess's cwd (`_internal/transport/subprocess_cli.py` as of 0.2.87 passes it to `open_process` and sets `PWD`), so stdio MCP servers pick it up by **inheriting** it from that subprocess. There is deliberately no per-server stamping as in `copilot.py::_mcp_servers_for_cwd`: the SDK's `McpStdioServerConfig` has no cwd field, so `_translate_mcp_servers` is left alone. Inheritance is a property of the CLI binary, not of the SDK, so it is documented rather than asserted by a test. - The path is passed **verbatim** — `WorkflowEngine._resolve_agent_working_dir` has already rendered, absolutized, normalised, and existence-checked it, and re-resolving here would collapse the symlink aliases the engine preserves on purpose. The `ClaudeAgentOptions(...)` construction lives **inside** `execute`'s `try` so the `os.getcwd()` fallback can't escape as a bare `OSError` when the process cwd has been deleted (`copilot.py` resolves its cwd inside its try for the same reason). - There is no provider-side `is_dir()` guard: a directory that vanishes after the engine's check surfaces as the SDK's `CLIConnectionError("Working directory does not exist: ")`, wrapped in `ProviderError`. That is only defensible because `_classify_startup_failure` special-cases it — `CLIConnectionError` otherwise yields firewall/binary advice and `is_retryable=True`, which is wrong for all three launch failures (missing dir; path is a file → `ENOTDIR`; unreadable dir → `EACCES`; the latter two reach the SDK's generic "Failed to start Claude Code" arm, not its dedicated one). - - Knock-on effects: cwd is the project key for the CLI's on-disk transcript directory, and it is where a project `.mcp.json` and `.claude/` tree would be looked for. The unconditional `strict_mcp_config=True` stops a `.mcp.json` there from injecting undeclared servers, and `setting_sources=[]` (empty by default; see **Skills** below) stops the CLI loading `CLAUDE.md`, project settings, and hooks from it — so cwd no longer drags ambient instructions in unless the workflow opts in via `runtime.provider.setting_sources`, which is exactly a request to load them from that directory. `add_dirs` (the SDK's `--add-dir` passthrough) is a separate axis Conductor does not set. + - Knock-on effects: cwd is the project key for the CLI's on-disk transcript directory, and it is where a project `.mcp.json` and `.claude/` tree would be looked for. The unconditional `strict_mcp_config=True` stops a `.mcp.json` there from injecting undeclared servers, and `setting_sources=[]` (empty by default; see **Skills** below) stops the CLI loading `CLAUDE.md`, project settings, and hooks from it — so cwd no longer drags ambient instructions in unless the workflow opts in via `runtime.provider.setting_sources`, which is exactly a request to load them from that directory. `add_dirs` (the SDK's `--add-dir` passthrough) is a separate axis, carrying the per-agent `settings_dir` and nothing else — see **Target-repository skills** below. +- **Target-repository skills** (`settings_dir`): the per-agent `AgentDef.settings_dir` is the **only** source of `ClaudeAgentOptions.add_dirs`, and it selects which directory's *project* settings tier contributes skills. It exists because cwd was doing two conflicting jobs on this provider. The CLI supports the MCP Roots protocol and advertises exactly one root — its cwd — so `@modelcontextprotocol/server-filesystem` **discards the directories in its own argv** and permits cwd alone; cwd is simultaneously what the `project` tier resolves against. Narrowing cwd onto a target repository to reach its skills therefore narrowed the agent's only MCP root below any sibling path the step still had to read. Do not derive `add_dirs` from a server's directory arguments to compensate: `--add-dir` takes no part in Roots negotiation, it widens the CLI's own file tools, and doing so would also silently widen skill discovery to directories the author named as data. What `add_dirs` *does* do, measured: a named directory's `.claude/skills` become listed and invocable with cwd elsewhere entirely — and only those, not `CLAUDE.md`, `.claude/rules/*.md`, `.claude/settings.json` (so no `env` and no `hooks` — the hooks negative is measured with a side-effect probe whose control fires) or `.claude/agents`, which all keep following cwd. That makes it the *skills third* of what a cwd-resolved `project` tier loads, not a replacement for it. It carries a second, **unconditional** effect the skills framing hides: `add_dirs` is "additional directories Claude can access" per the SDK's own contract, so a `settings_dir` widens the model's built-in `Read`/`Edit`/`Bash` to that tree with no settings tier enabled at all (measured at `permission_mode: "default"` with `setting_sources` unset: a read outside cwd is refused without it and succeeds with it; an agent omitting `tools:` runs under `bypassPermissions`, where the grant is unobservable because reads already succeed). It does not widen what an MCP server permits. `capabilities.py::settings_dir` gates the field and `config/validator.py` errors against a provider that cannot apply it, warning when it is set with no `setting_sources` (the skills half is then a no-op while the filesystem grant still applies). `WorkflowEngine._resolve_agent_directory` resolves both fields so they cannot drift, and `settings_dir` is per-agent only (no `runtime.` counterpart: the repository whose conventions apply is what varies between steps). `tests/test_integration/test_mcp_roots_negotiation.py` pins the negotiation rule itself against the real server with no LLM — two runs differing only in whether the client advertises `roots` — since every option here rests on it. - **Skills** (issue #352): `supports_native_skills=True`. Skills are enabled through the SDK, not prompt injection, and three options move together in `execute`: - `plugins=[{"type": "local", "path": }]` + `skills=[":"]`. The SDK has no skill-*directory* surface, so `_resolve_skill_plugins` maps each directory back to the plugin that owns it via `skills/registry.py::resolve_skill_plugin`. That resolver is deliberately strict, because every one of these mistakes otherwise produces a name the CLI silently resolves to nothing: it bounds the upward walk (`_PLUGIN_SEARCH_DEPTH`), requires the skill to actually live under the candidate's `skills/` directory, requires `SKILL.md` to exist and its frontmatter `name` to equal the directory name (the CLI resolves by frontmatter name; Conductor sends the directory name), and rejects names outside `[A-Za-z0-9_.-]+` since they are joined into a comma-delimited `--allowedTools` value. A skill under no plugin root returns `None`; a plugin that is present but unusable raises `SkillPluginError`, which the provider re-raises as a `ProviderError` carrying the real reason rather than a blanket "not part of a plugin". Two plugins claiming one qualified name are refused too — deduping the clash away would drop a declared skill. All of it is `is_retryable=False`: these never become valid on a retry, and a checkout path containing "connection" would otherwise trip the retryability heuristic. Note providers are constructed lazily, so this surfaces when the first agent on this provider runs, **not** at `conductor validate`. - `setting_sources=[]` **by default**, for the same reason `strict_mcp_config=True` is unconditional a few lines away — but opt-in per workflow via `runtime.provider.setting_sources` (`user`/`project`/`local`, issue #501). Left unset, the CLI loads user, project, and local settings, which between them bring in ambient skills, `CLAUDE.md`, and hooks the workflow never declared, varying by machine and launch directory. Conductor surfaces instruction files through its own opt-in `--workspace-instructions`; settings and hooks have no equivalent. `skills=[]` and `skills=None` are **not** interchangeable upstream: `None` means "CLI defaults apply", and setting `skills` while leaving `setting_sources` at `None` makes the SDK re-default it to `["user", "project"]` — so the two options are coupled and dropping either re-opens the issue. The `[]` is also invisible in argv (it travels in the SDK's `initialize` control request), which is why the argv-based tests are paired with options-level assertions. The list is a context filter, not a sandbox: undeclared skills are hidden from the model's listing and rejected by the `Skill` tool, but their files stay readable. Opting a workflow in (`runtime.provider.setting_sources: [project]`, the motivating case being an agent whose `working_dir` is a *target* repo shipping its own `.claude/skills` — the CLI has `--plugin-dir` but no `--skill-dir`) loads that tier **with its hooks**, so it is only for repositories trusted as much as the workflow itself, and the field is workflow-global while `working_dir` is per agent. Two couplings follow: `_resolve_skill_filter` resolves `skills` to `"all"` when a tier is enabled and the workflow named none itself (tier-discovered skills never pass through `skill_names`, so `[]` would load the repo's skills and then hide every one), and a per-agent `skills: []` opts that agent out of the tier entirely — hooks included — keeping it the one opt-out. diff --git a/docs/providers/experimental.md b/docs/providers/experimental.md index f849ee9b..cc90bd1d 100644 --- a/docs/providers/experimental.md +++ b/docs/providers/experimental.md @@ -101,7 +101,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.2.82` | `@lesandiz (best-effort)` | no `workflow_tools_passthrough`, no `reasoning_effort`, `prompt_injection` structured output, no `checkpoint_resume` (agents without a `session_key` carry no session state across a resume). 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). Supports `working_dir` as of [#348](https://github.com/microsoft/conductor/issues/348); the CLI would load `CLAUDE.md` and `.claude/settings*.json` from that directory, but `setting_sources` is empty by default as of [#352](https://github.com/microsoft/conductor/issues/352) so ambient instructions, settings, hooks, and skills are not inherited unless a workflow opts in via `runtime.provider.setting_sources` ([#501](https://github.com/microsoft/conductor/issues/501)) — which loads the named tiers **including their hooks**, so only for repositories trusted as much as the workflow. Declares `session_continuity`: an agent with a `session_key` reuses one Claude session across executions, and the session map survives `conductor resume` — see [Session Continuity](../workflow-syntax.md#session-continuity-session_key). | +| `claude-agent-sdk` | `claude-agent-sdk>=0.2.82` | `@lesandiz (best-effort)` | no `reasoning_effort`, `prompt_injection` structured output, no `checkpoint_resume` (agents without a `session_key` carry no session state across a resume). 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). Supports `workflow_tools_passthrough`: a per-agent `tools:` allowlist is enforced by enumerating the declared stdio MCP servers and denying every tool not on the list (plus the built-in write/exec tools), since the CLI's `allowed_tools` only pre-approves and does not restrict. An allowlist alongside an http/sse server is refused — those cannot be enumerated, so the denial set would be unknown. `tools: []` alongside `mcp_servers:` is still refused (`mcp_servers_always_attached`): honoring an allowlist does not mean the provider can detach a declared server. Supports `working_dir` as of [#348](https://github.com/microsoft/conductor/issues/348); the CLI would load `CLAUDE.md` and `.claude/settings*.json` from that directory, but `setting_sources` defaults to empty as of [#352](https://github.com/microsoft/conductor/issues/352) so ambient instructions, settings, hooks, and skills are not inherited; a workflow opts back in per run with `runtime.provider.setting_sources`, and chooses per agent which directory the `project` tier reads skills from via `settings_dir` (cwd alone governs the CLI's sole MCP root, so the two are deliberately separate) — see [Target-Repository Skills](../workflow-syntax.md#target-repository-skills-settings_dir). Declares `session_continuity`: an agent with a `session_key` reuses one Claude session across executions, and the session map survives `conductor resume` — see [Session Continuity](../workflow-syntax.md#session-continuity-session_key). | | `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/docs/workflow-syntax.md b/docs/workflow-syntax.md index 8de7b9ce..bf1c1628 100644 --- a/docs/workflow-syntax.md +++ b/docs/workflow-syntax.md @@ -402,6 +402,117 @@ Because paths are normalized lexically instead of resolving to their real paths: > Setting `working_dir` doesn't restrict the model's filesystem access. The model can still read and write files outside this directory if it uses absolute paths or parent directory traversals (e.g., `../`). Avoid relying on this configuration to sandbox untrusted model execution. > On the `claude-agent-sdk` provider the directory is also a trust boundary in the other direction: the `claude` CLI loads `CLAUDE.md` and `.claude/settings*.json` (including hooks) from wherever it runs, so pointing `working_dir` at an untrusted checkout means running that checkout's instructions. +### Target-Repository Skills (`settings_dir`) + +`settings_dir` names a second directory whose Claude Code *project* settings +tier the agent reads skills from. It applies to `claude-agent-sdk` agents in a +workflow that sets `runtime.provider.setting_sources`, and is ignored by every +other provider. + +```yaml +workflow: + runtime: + provider: + name: claude-agent-sdk + setting_sources: [project] + +agents: + - name: judge + settings_dir: "{{ setup_worktree.output.worktree_path }}" + prompt: Review the change against this repository's conventions. +``` + +#### Why it is separate from `working_dir` + +An agent's cwd does two unrelated jobs, and on this provider they conflict. +The `claude` CLI supports the MCP Roots protocol and advertises exactly one +root — its cwd. A filesystem MCP server therefore **discards the directories +in its own argv** and permits cwd alone; `--add-dir` takes no part in that +negotiation, so it cannot widen what a server allows. cwd is simultaneously +the directory the `project` settings tier resolves against. + +So pointing `working_dir` at a target repository to pick up that repository's +skills also narrows the agent's only MCP root onto it, and any sibling path +the step still has to read — an artifacts directory, a second checkout — is +denied. Widening cwd back loses the repository's conventions. + +`settings_dir` splits the two. Skills are discovered from cwd **and** from +`settings_dir`, so cwd can stay wide enough to contain everything the agent +must read: + +```yaml +agents: + - name: judge + # No working_dir: cwd stays the launch directory, which contains both the + # worktree and the artifacts this judge reads through the filesystem MCP. + settings_dir: "{{ setup_worktree.output.worktree_path }}" +``` + +#### What it does and does not carry + +Measured against the CLI: + +| Named via `settings_dir` | Granted? | +|---|---| +| **Filesystem access for the model's built-in tools** (`Read`, `Edit`, `Bash`, …) | **yes — unconditionally**, see below | +| `.claude/skills` | **yes** — listed and invocable | +| `CLAUDE.md` | no | +| `.claude/rules/*.md` | no | +| `.claude/settings.json` `env` | no | +| `.claude/settings.json` `hooks` | no — measured, see below | +| `.claude/agents` | no | + +> ⚠️ **The filesystem grant does not depend on `setting_sources`.** This field +> maps to the SDK's `add_dirs`, whose own contract is *"additional directories +> Claude can access beyond the current working directory"* — so naming a +> directory here widens the model's built-in file tools to that tree whether or +> not any settings tier is enabled. Measured at `permission_mode: "default"` +> with `setting_sources` unset: a read outside cwd is refused without +> `settings_dir` and succeeds with it. Note an agent that omits `tools:` runs +> under `bypassPermissions`, where reads already succeed everywhere, so the +> grant only becomes observable once permissions are in play. +> +> Skill discovery is the *reason* to set this field; the filesystem grant is +> its unavoidable companion. Point it at a directory the agent is entitled to +> read. + +Note this grant is for the model's **built-in** tools only. It does not widen +what a filesystem MCP server permits — that stays cwd alone, which is the +whole reason this field exists. + +**The `hooks` row is a measured negative.** A `PreToolUse` hook that appends +to a file (an observable side effect, not a log line) runs when `working_dir` +is the repository and the `project` tier is enabled, and does **not** run when +the same repository is reached only through `settings_dir` — with or without a +tier enabled. The control firing is what makes the negative meaningful. + +Setting aside the filesystem grant, this field is the *skills portion* of a +project tier, not a cwd-independent way to load one. It cuts favourably in one direction — +a target repository's skills arrive without its hooks also running — but it +does not compose with `working_dir` into "everything, anywhere": + +> An agent that needs a target repository's **rules or instructions** as well +> as a cwd wide enough for its MCP servers cannot get both from these fields. +> One directory cannot be narrow and wide at once. `settings_dir` recovers the +> skills; anything else is a caller-side trade — keep `working_dir` on the +> repository and arrange for every path the agent reads to sit beneath it. + +#### Resolution and restrictions + +- Resolved exactly like `working_dir` — Jinja2-rendered, `~`-expanded, + relative paths resolved against the workflow file's directory, normalized + with `os.path.normpath`, and existence-checked before any provider call. +- Per-agent only. There is no `runtime.settings_dir`, because the repository + whose conventions apply is what varies between steps. +- Rejected on `wait`, `set`, `terminate`, `script`, `human_gate`, `questions` + and `workflow` step types — none has an LLM session to apply a settings tier + to, and accepting it silently would suggest conventions had been loaded when + none had. + +> ⚠️ A settings tier brings everything that tier defines. Enable +> `setting_sources` and point `settings_dir` only at repositories trusted to +> the same degree as the workflow itself. + ### Session Continuity (`session_key`) By default each agent execution starts a fresh provider session, so an agent diff --git a/examples/claude-agent-sdk-settings-dir.yaml b/examples/claude-agent-sdk-settings-dir.yaml new file mode 100644 index 00000000..1f6afbb3 --- /dev/null +++ b/examples/claude-agent-sdk-settings-dir.yaml @@ -0,0 +1,134 @@ +# Target-repository skills without narrowing the MCP root (`settings_dir`) +# +# A reviewer agent that loads a TARGET repository's own `.claude/skills` while +# keeping filesystem access wide enough to also read a sibling artifacts +# directory. Two directories, two jobs, set independently. +# +# THE PROBLEM THIS EXAMPLE EXISTS FOR +# On `claude-agent-sdk`, an agent's cwd does two unrelated jobs at once: +# +# 1. The `claude` CLI supports the MCP Roots protocol and advertises +# exactly ONE root -- its cwd. `@modelcontextprotocol/server-filesystem` +# uses the directories in its own argv only while the client does NOT +# support Roots, so with this CLI it discards them and permits cwd +# alone. cwd is therefore the only handle on what the agent may read. +# (`--add-dir` cannot widen this: it takes no part in that negotiation.) +# +# 2. cwd is also what the `project` settings tier resolves against, so it +# decides WHOSE conventions load. +# +# Point `working_dir` at the repository under review and job 2 is satisfied +# while job 1 breaks: the artifacts directory outside that repository is now +# denied. Widen `working_dir` back and job 2 breaks instead -- the reviewer +# loads the ORCHESTRATOR's conventions while reviewing someone else's code. +# +# THE FIX +# `settings_dir` carries job 2 on its own. Skills are discovered from cwd +# AND from `settings_dir`, so cwd stays wide: +# +# working_dir (or none) -> the single MCP root: keep it wide +# settings_dir -> whose .claude/skills load: keep it narrow +# +# Only skills travel this way. `CLAUDE.md`, `.claude/settings.json` (so +# `env` and `hooks`) and `.claude/agents` all keep following cwd -- which +# cuts favourably here: the target repository's skills arrive without its +# hooks also running. +# +# Pre-requisites: +# pip install conductor[claude-agent-sdk] +# npm install -g @anthropic-ai/claude-code # the `claude` CLI +# claude login +# npx -y @modelcontextprotocol/server-filesystem --help # warms the server +# +# Usage -- `workspace` must CONTAIN both the repo and the artifacts directory: +# conductor run examples/claude-agent-sdk-settings-dir.yaml \ +# --input workspace=/path/to/workspace \ +# --input repo=/path/to/workspace/target-repo \ +# --input artifacts=/path/to/workspace/artifacts +# +# where: +# workspace -- contains BOTH the repo and the artifacts directory. It +# becomes cwd, and therefore the filesystem MCP server's +# only root: everything the agent reads must live under it. +# repo -- the target repository, under `workspace`. Supplies the +# skills, via `settings_dir`. +# artifacts -- prior findings, under `workspace` but OUTSIDE `repo` -- +# the path a narrowed cwd would deny. +# +# Validation only (no execution / no API calls): +# conductor validate examples/claude-agent-sdk-settings-dir.yaml + +workflow: + name: claude-agent-sdk-settings-dir + description: > + Reviewer that loads a target repository's own skills via `settings_dir` + while keeping a wide cwd, so its filesystem MCP server can still reach a + sibling artifacts directory. + version: "1.0.0" + entry_point: review + + runtime: + provider: + name: claude-agent-sdk + # Opt in to the `project` tier. Off by default, so a run does not + # inherit whatever the machine happens to have installed. A tier brings + # everything it defines: enable it only for repositories trusted to the + # same degree as this workflow. + setting_sources: [project] + default_model: claude-sonnet-4-5 + + # cwd for every agent: the wide directory. The filesystem MCP server's + # argv roots below are discarded by the server itself (see the header), so + # THIS is what actually decides what the agent can read. + working_dir: "{{ workflow.input.workspace }}" + + mcp_servers: + filesystem: + type: stdio + command: npx + args: + - "-y" + - "@modelcontextprotocol/server-filesystem" + # Declared for a client that does not negotiate Roots. The `claude` + # CLI does, so it permits cwd alone and these are ignored -- kept + # because they are still the honest declaration of intent, not + # because they take effect here. + - "{{ workflow.input.repo }}" + - "{{ workflow.input.artifacts }}" + +agents: + - name: review + # The narrow half: this repository's `.claude/skills` become listed and + # invocable, with cwd still the wide workspace above. Templated, since the + # directory under review is normally an upstream step's output. + settings_dir: "{{ workflow.input.repo }}" + prompt: | + You are reviewing the repository at {{ workflow.input.repo }}. + + First, list the skills available to you. Any skill this repository ships + in its own `.claude/skills` should be among them -- that is what + `settings_dir` provides, and you should follow its conventions rather + than any you would apply by default. + + Then read the prior findings under {{ workflow.input.artifacts }} using + the filesystem MCP server. That directory sits outside the repository, + so a run that had narrowed the working directory onto the repository + could not read it at all. + + Report: + - which repository-supplied skills you found + - what the prior findings say + - whether the repository's conventions change your reading of them + output: + skills_found: + type: string + description: Repository-supplied skills the session offered. + findings_summary: + type: string + description: What the artifacts directory contained. + routes: + - to: "$end" + +output: + skills_found: "{{ review.output.skills_found }}" + findings_summary: "{{ review.output.findings_summary }}" diff --git a/src/conductor/config/schema.py b/src/conductor/config/schema.py index c84d17b1..5309ad7c 100644 --- a/src/conductor/config/schema.py +++ b/src/conductor/config/schema.py @@ -1381,6 +1381,66 @@ class AgentDef(BaseModel): wait/set/terminate/human_gate/workflow step types. """ + settings_dir: str | None = None + """Directory whose Claude Code *project* settings tier this agent loads. + + ``claude-agent-sdk`` only -- ``conductor validate`` refuses it against a + provider that cannot apply it, rather than dropping it silently. Resolved + by the engine exactly like :attr:`working_dir` (Jinja-rendered, + ``~``-expanded, made absolute against the workflow file's directory, + ``normpath``-normalised, existence-checked), then forwarded to the SDK as + ``ClaudeAgentOptions.add_dirs``. Rejected on + wait/set/terminate/script/human_gate/questions/workflow step types. + + **Two effects, and only one of them is conditional.** Skill discovery + requires ``runtime.provider.setting_sources`` to enable the ``project`` + tier; ``conductor validate`` warns when this field is set without it, + since the skills half is then a no-op. The *filesystem* grant is + unconditional: ``add_dirs``' own SDK contract is "additional directories + Claude can access beyond the current working directory", so naming a + directory here widens the model's built-in ``Read``/``Edit``/``Bash`` + tools to that tree regardless of any settings tier. It does **not** widen + what a filesystem MCP server permits -- that stays cwd alone, which is why + this field exists. Point it only at a directory the agent may read. + + It exists because ``working_dir`` was doing two unrelated jobs. The CLI + supports MCP Roots and advertises exactly one root — its cwd — so a + filesystem MCP server discards the directories in its own argv and + permits cwd alone. That makes cwd the *only* handle on what an agent can + read, while it is simultaneously the directory the ``project`` settings + tier resolves against. Narrowing cwd onto a target repository to pick up + that repository's skills therefore also narrowed the MCP root below any + sibling path the step still had to read, and widening it back lost the + repository's conventions. + + ``settings_dir`` splits them: the *skills* of every directory named here + are discovered and invocable regardless of cwd, so cwd can stay wide + enough to contain everything the agent must read. + + The split is not total, and the remainder is deliberate. A directory + named here contributes its ``.claude/skills`` and nothing else — not + ``CLAUDE.md``, not ``.claude/rules/*.md``, not ``.claude/settings.json`` + (so no ``env`` and no ``hooks``), not ``.claude/agents``, all of which + continue to follow cwd. This field is the *skills* portion of a project + tier, not a cwd-independent way to load one: instructions, rules and + hooks still require ``working_dir`` pointed at the directory. + + So the two fields do not compose into "everything, anywhere". An agent + needing a target repository's rules *and* a cwd wide enough for its MCP + servers cannot have both from these fields alone -- one directory cannot + be simultaneously narrow and wide. ``settings_dir`` recovers the skills; + the rest is a caller-side trade. + + Example — a judge reviewing a target repository while reading artifacts + from a sibling directory:: + + agents: + judge: + settings_dir: "{{ setup_worktree.output.worktree_path }}" + # No working_dir: cwd stays the launch directory, which contains + # both the worktree and the artifacts the judge must read. + """ + stdin: str | None = None """Payload written to the script subprocess's stdin (script type only). @@ -2030,6 +2090,8 @@ def validate_agent_type(self) -> AgentDef: raise ValueError("human_gate agents cannot have 'output_mode'") if self.working_dir: raise ValueError("human_gate agents cannot have 'working_dir'") + if self.settings_dir: + raise ValueError("human_gate agents cannot have 'settings_dir'") if self.session_key is not None: raise ValueError("human_gate agents cannot have 'session_key'") elif self.type == "questions": @@ -2093,6 +2155,8 @@ def validate_agent_type(self) -> AgentDef: raise ValueError("questions agents cannot have 'output_mode'") if self.working_dir: raise ValueError("questions agents cannot have 'working_dir'") + if self.settings_dir: + raise ValueError("questions agents cannot have 'settings_dir'") if self.session_key is not None: raise ValueError("questions agents cannot have 'session_key'") elif self.type == "script": @@ -2126,6 +2190,8 @@ def validate_agent_type(self) -> AgentDef: raise ValueError("script agents cannot have 'validator'") if self.sandbox is not None: raise ValueError("script agents cannot have 'sandbox'") + if self.settings_dir: + raise ValueError("script agents cannot have 'settings_dir'") if self.max_depth is not None: raise ValueError("script agents cannot have 'max_depth'") if self.reasoning is not None: @@ -2192,6 +2258,8 @@ def validate_agent_type(self) -> AgentDef: raise ValueError("workflow agents cannot have 'output_mode'") if self.working_dir: raise ValueError("workflow agents cannot have 'working_dir'") + if self.settings_dir: + raise ValueError("workflow agents cannot have 'settings_dir'") elif self.type == "wait": if self.duration is None: raise ValueError("wait agents require 'duration'") @@ -2215,6 +2283,8 @@ def validate_agent_type(self) -> AgentDef: raise ValueError("wait agents cannot have 'env'") if self.working_dir: raise ValueError("wait agents cannot have 'working_dir'") + if self.settings_dir: + raise ValueError("wait agents cannot have 'settings_dir'") if self.timeout is not None: raise ValueError("wait agents cannot have 'timeout'") if self.workflow: @@ -2288,6 +2358,8 @@ def validate_agent_type(self) -> AgentDef: raise ValueError("set agents cannot have 'env'") if self.working_dir: raise ValueError("set agents cannot have 'working_dir'") + if self.settings_dir: + raise ValueError("set agents cannot have 'settings_dir'") if self.timeout is not None: raise ValueError("set agents cannot have 'timeout'") if self.workflow: @@ -2362,6 +2434,8 @@ def validate_agent_type(self) -> AgentDef: raise ValueError("terminate agents cannot have 'env'") if self.working_dir: raise ValueError("terminate agents cannot have 'working_dir'") + if self.settings_dir: + raise ValueError("terminate agents cannot have 'settings_dir'") if self.timeout is not None: raise ValueError("terminate agents cannot have 'timeout'") if self.timeout_seconds is not None: @@ -2699,11 +2773,19 @@ class ProviderSettings(BaseModel): installed, so a run reproduces on another developer's laptop. Set it to opt a workflow back in. The motivating case is an agent working - inside a *target* repository that ships its own ``.claude/skills`` — with - ``working_dir`` pointed at that repo, ``["project"]`` loads that repo's - skills and instructions natively, without the repo needing to package - them as a Claude Code plugin (the CLI has ``--plugin-dir`` but no - ``--skill-dir``, so a plugin root is otherwise the only handle). + against a *target* repository that ships its own ``.claude/skills``: + ``["project"]`` loads that repository's skills natively, without it + needing to package them as a Claude Code plugin (the CLI has + ``--plugin-dir`` but no ``--skill-dir``, so a plugin root is otherwise + the only handle). + + Which directory the ``project`` tier reads is chosen per agent. Skills + come from cwd (:attr:`AgentDef.working_dir`) *and* from + :attr:`AgentDef.settings_dir`; everything else the tier defines -- + ``CLAUDE.md``, ``.claude/settings.json``, ``.claude/agents`` — follows + cwd alone. Prefer ``settings_dir`` when the agent also needs a wider cwd: + the CLI advertises cwd as its sole MCP root, so narrowing cwd onto the + target repository narrows what the agent's MCP servers may read. Each tier brings everything that tier defines, hooks included: ``project`` reads ``/.claude/settings.json``, whose ``hooks`` entries run diff --git a/src/conductor/config/validator.py b/src/conductor/config/validator.py index 90615827..779ceff9 100644 --- a/src/conductor/config/validator.py +++ b/src/conductor/config/validator.py @@ -1272,6 +1272,11 @@ def _collect_template_strings( templates.append((f"agent '{agent.name}' args[{i}]", arg)) if agent.working_dir: templates.append((f"agent '{agent.name}' working_dir", agent.working_dir)) + # getattr for the same reason the 'set' bindings below use it: duck-typed + # test fixtures predate this field and would raise on direct access. + settings_dir = getattr(agent, "settings_dir", None) + if settings_dir: + templates.append((f"agent '{agent.name}' settings_dir", settings_dir)) # 'set' step bindings — value: single expression, values: named expressions. # Use getattr so duck-typed test fixtures without these attributes still @@ -1807,6 +1812,18 @@ def _is_llm_agent(agent: AgentDef) -> bool: return agent.type in _LLM_AGENT_TYPES +def _setting_sources_enabled(config: WorkflowConfig) -> bool: + """True iff the workflow enables any Claude Code settings tier. + + ``runtime.provider`` is either the bare string shorthand (no tiers, by + definition) or a ``ProviderSettings`` carrying ``setting_sources``. An + empty or absent list means Conductor sends ``[]`` -- load nothing ambient + -- so no ``project`` tier exists for a ``settings_dir`` to be read from. + """ + provider = config.workflow.runtime.provider + return bool(getattr(provider, "setting_sources", None)) + + def _resolved_provider_name(agent: AgentDef, default: str) -> str: """The provider name an agent will actually use at runtime. @@ -2453,6 +2470,31 @@ def _check_agent_capabilities( f"directories (capabilities.working_dir=False)." ) + # settings_dir: same class as working_dir. A provider with nowhere to + # put the directory would load the wrong repository's conventions and + # report success, so this is an error rather than a dropped field. + if agent.settings_dir is not None and not caps.settings_dir: + errors.append( + f"Agent '{agent.name}' sets settings_dir={agent.settings_dir!r} " + f"but provider '{provider_name}' does not apply it " + f"(capabilities.settings_dir=False). Only 'claude-agent-sdk' " + f"has a surface for it; use working_dir, or move this agent to " + f"that provider." + ) + elif agent.settings_dir is not None and not _setting_sources_enabled(config): + # A warning, not an error: the FILESYSTEM half of settings_dir + # applies regardless, so the workflow is not broken -- but the + # skill discovery it is normally set for is a no-op without the + # project tier enabled, and a green validate would imply otherwise. + warnings.append( + f"Agent '{agent.name}' sets settings_dir={agent.settings_dir!r} but " + f"the workflow does not set runtime.provider.setting_sources, so no " + f"settings tier is enabled and no skills will be discovered from it. " + f"The directory is still granted to the model's built-in file tools. " + f"Add 'setting_sources: [project]' to runtime.provider to load that " + f"repository's skills." + ) + # session_key: a provider that ignores it starts a fresh session every # execution, silently discarding the context the author asked to keep. if agent.session_key is not None and not caps.session_continuity: diff --git a/src/conductor/engine/workflow.py b/src/conductor/engine/workflow.py index 2efd7cfc..fee0c394 100644 --- a/src/conductor/engine/workflow.py +++ b/src/conductor/engine/workflow.py @@ -593,28 +593,23 @@ def _workflow_dir(self) -> Path | None: """Resolved parent directory of the workflow file, or None if unset.""" return Path(self.workflow_path).resolve().parent if self.workflow_path else None - def _resolve_agent_working_dir( - self, agent: AgentDef, agent_context: dict[str, Any] - ) -> AgentDef: - """Resolve an agent's effective ``working_dir`` and return an updated copy. - - Precedence is ``agent.working_dir`` over ``runtime.working_dir``; the - chosen raw value is Jinja-rendered against the per-agent context (so - both levels support templates, e.g. ``{{ item }}`` in for-each), then - ``~``-expanded, made absolute against the workflow file's directory - (falling back to the process cwd), and lexically normalised with - :func:`os.path.normpath` (``resolve()`` is deliberately not used so - symlink aliases stay distinct). A missing directory raises - :class:`ExecutionError` before any provider call. When neither level - sets a value the agent is returned unchanged (``working_dir=None`` and - the provider uses its own cwd). - """ - raw = agent.working_dir - if raw is None: - raw = self.config.workflow.runtime.working_dir - if raw is None: - return agent + def _resolve_agent_directory( + self, agent: AgentDef, agent_context: dict[str, Any], field: str, raw: str + ) -> str: + """Render and absolutize one authored directory value. + + Shared by ``working_dir`` and ``settings_dir`` so the two cannot drift + apart: the raw value is Jinja-rendered against the per-agent context + (so templates such as ``{{ item }}`` in for-each work at either + level), then ``~``-expanded, made absolute against the workflow + file's directory (falling back to the process cwd), and lexically + normalised with :func:`os.path.normpath` (``resolve()`` is + deliberately not used so symlink aliases stay distinct). + Raises: + ExecutionError: if the resolved path is not an existing directory + — before any provider call. + """ rendered = self.renderer.render(raw, agent_context) path = Path(rendered).expanduser() if not path.is_absolute(): @@ -624,16 +619,57 @@ def _resolve_agent_working_dir( if not Path(resolved).is_dir(): raise ExecutionError( - f"Agent '{agent.name}': working_dir '{resolved}' does not exist or " + f"Agent '{agent.name}': {field} '{resolved}' does not exist or " f"is not a directory (rendered from '{raw}')", agent_name=agent.name, suggestion=( "Create the directory before the agent runs (e.g. via a " - "script step) or fix the working_dir template." + f"script step) or fix the {field} template." ), ) + return resolved - return agent.model_copy(update={"working_dir": resolved}) + def _resolve_agent_working_dir( + self, agent: AgentDef, agent_context: dict[str, Any] + ) -> AgentDef: + """Resolve an agent's ``working_dir`` and ``settings_dir``, returning a copy. + + The name says ``working_dir`` only for historical reasons -- it is + referenced by name from four other modules' comments, so renaming it + costs more than it explains. Grep for ``settings_dir`` and this is + where it is resolved. + + ``working_dir`` precedence is ``agent.working_dir`` over + ``runtime.working_dir``; ``settings_dir`` is per-agent only, since the + directory whose conventions apply is what varies between steps. Both + are resolved by :meth:`_resolve_agent_directory`. An agent setting + neither is returned unchanged, leaving the provider its own cwd. + + The two are independent on purpose: ``working_dir`` becomes the + session cwd, which the CLI advertises as its sole MCP root, while + ``settings_dir`` only adds a directory whose project-tier skills are + discoverable. An agent can therefore keep a wide cwd — wide enough + for every path its MCP servers must reach — and still load a + narrower target repository's skills. + """ + update: dict[str, Any] = {} + + raw = agent.working_dir + if raw is None: + raw = self.config.workflow.runtime.working_dir + if raw is not None: + update["working_dir"] = self._resolve_agent_directory( + agent, agent_context, "working_dir", raw + ) + + if agent.settings_dir is not None: + update["settings_dir"] = self._resolve_agent_directory( + agent, agent_context, "settings_dir", agent.settings_dir + ) + + if not update: + return agent + return agent.model_copy(update=update) def _build_pricing_overrides(self) -> dict[str, ModelPricing] | None: """Build pricing overrides from workflow cost configuration. @@ -4426,7 +4462,7 @@ async def _execute_loop(self, current_agent_name: str) -> dict[str, Any]: agent_type=agent.type, ) - # Resolve working_dir only for provider-backed LLM agents + # Resolve working_dir / settings_dir for provider-backed LLM agents # (type None/"agent"). wait/set/terminate/human_gate/ # workflow are schema-rejected from declaring one, and # script resolves its own in ScriptExecutor. @@ -6157,7 +6193,7 @@ async def execute_single_agent(agent: AgentDef) -> tuple[str, Any]: ) return (agent.name, set_output.value) - # Resolve working_dir for provider-backed LLM agents against + # Resolve working_dir / settings_dir for provider-backed LLM agents against # this agent's own (pre-group snapshot) context. `set` steps # returned above; other types in a parallel group are LLM agents. resolved_agent = self._resolve_agent_working_dir(agent, agent_context) @@ -6643,7 +6679,7 @@ async def execute_single_item(item: Any, index: int, key: str) -> tuple[str, Any update={"name": f"{for_each_group.agent.name}[{key}]"} ) - # Resolve working_dir AFTER loop variables were injected into + # Resolve working_dir / settings_dir AFTER loop variables were injected into # agent_context so a `{{ item }}` (or `{{ }}`) template in # the path resolves to this iteration's value. qualified_agent = self._resolve_agent_working_dir(qualified_agent, agent_context) diff --git a/src/conductor/providers/capabilities.py b/src/conductor/providers/capabilities.py index 9d042762..404c1ac5 100644 --- a/src/conductor/providers/capabilities.py +++ b/src/conductor/providers/capabilities.py @@ -143,6 +143,19 @@ class ProviderCapabilities(BaseModel): the directory would run the agent in the wrong repository. Defaults to ``False`` (conservative).""" + settings_dir: bool = False + """``True`` when the provider applies an agent's resolved ``settings_dir``. + + Workflows that set ``settings_dir`` against a provider with + ``settings_dir=False`` fail validation, for the same reason + ``working_dir`` does: the field selects which repository's conventions the + agent loads *and* widens the model's built-in file tools to that tree, so + silently ignoring it would run the agent against the wrong conventions + while reporting success. Distinct from ``working_dir`` because the two are + deliberately independent axes -- cwd is the sole root a filesystem MCP + server gets, while this only adds a directory. Defaults to ``False`` + (conservative).""" + skills: bool = False """``True`` when the provider exposes :mod:`conductor.skills` content to the agent. The user-facing contract is the same regardless of diff --git a/src/conductor/providers/claude_agent_sdk.py b/src/conductor/providers/claude_agent_sdk.py index b1b6519e..243d150d 100644 --- a/src/conductor/providers/claude_agent_sdk.py +++ b/src/conductor/providers/claude_agent_sdk.py @@ -668,6 +668,9 @@ class ClaudeAgentSdkProvider(AgentProvider): # rather than being stamped individually as they are for Copilot: # the SDK's ``McpStdioServerConfig`` has no cwd field. working_dir=True, + # ``settings_dir`` reaches ``ClaudeAgentOptions.add_dirs``, the CLI's + # ``--add-dir``. It is the only provider that has anywhere to put it. + settings_dir=True, # Skills are loaded natively: the owning plugin is registered via # ``ClaudeAgentOptions.plugins`` and enabled by its qualified name # through ``skills``, so the model reads the frontmatter up front @@ -993,6 +996,27 @@ async def _execute_session( # so pass it through verbatim rather than re-resolving — that would # collapse the symlink aliases the engine preserves. cwd=resolved_cwd, + # The authored ``settings_dir`` and nothing else. + # + # What this does, measured: it makes a directory's *project* + # settings tier discoverable — its ``.claude/skills`` become + # listed and invocable with cwd elsewhere entirely, and only + # those, not CLAUDE.md, .claude/rules/*.md, .claude/settings.json + # or .claude/agents, which all stay with cwd. So it is the skills + # portion of a project tier rather than a cwd-independent way to + # load one. + # + # Do not extend this to the directory args of stdio MCP servers to + # widen what those servers may read: it cannot work. A filesystem + # MCP server uses its argv directories only when the client does + # not support MCP Roots, and the CLI does support Roots — + # advertising exactly one, its cwd — so the server discards its + # argv directories and permits cwd alone. ``--add-dir`` takes no + # part in that negotiation; it widens the CLI's own file tools, + # never what a server permits. Measured: cwd alone is the + # effective allowlist whether or not every declared root is also + # passed here. + add_dirs=[agent.settings_dir] if agent.settings_dir else [], output_format=_build_output_format(agent.output) if agent.output else None, max_turns=max_turns, permission_mode=permission_mode, diff --git a/tests/test_config/test_set_schema.py b/tests/test_config/test_set_schema.py index 9581e734..f4587ab9 100644 --- a/tests/test_config/test_set_schema.py +++ b/tests/test_config/test_set_schema.py @@ -132,6 +132,7 @@ class TestSetAgentDefForbiddenFields: ("args", ["x"], "cannot have 'args'"), ("env", {"K": "v"}, "cannot have 'env'"), ("working_dir", "/tmp", "cannot have 'working_dir'"), + ("settings_dir", "/tmp", "cannot have 'settings_dir'"), ("timeout", 5, "cannot have 'timeout'"), ("workflow", "x.yaml", "cannot have 'workflow'"), ("input_mapping", {"a": "1"}, "cannot have 'input_mapping'"), diff --git a/tests/test_config/test_settings_dir_schema.py b/tests/test_config/test_settings_dir_schema.py new file mode 100644 index 00000000..b59d0541 --- /dev/null +++ b/tests/test_config/test_settings_dir_schema.py @@ -0,0 +1,183 @@ +"""Schema tests for ``AgentDef.settings_dir``. + +``settings_dir`` names the directory whose Claude Code *project* settings +tier an agent loads skills from. It exists because ``working_dir`` was doing +two unrelated jobs at once: the CLI advertises its cwd as its sole MCP root, +so narrowing cwd onto a target repository to pick up that repository's +skills also narrowed what the agent's MCP servers were permitted to read. + +These tests pin the field's shape and, more importantly, that it stays +*independent* of ``working_dir`` -- a schema that coupled them, or that +silently accepted the field on a step type with no LLM session to apply it +to, would reintroduce the confusion the split removes. +""" + +from __future__ import annotations + +from pathlib import Path + +import pytest +from pydantic import ValidationError + +from conductor.config.schema import ( + AgentDef, + GateOption, + OutputField, + ProviderSettings, + RouteDef, + RuntimeConfig, + WorkflowConfig, + WorkflowDef, +) +from conductor.config.validator import validate_workflow_config +from conductor.exceptions import ConfigurationError + + +class TestSettingsDirAccepted: + """Provider-backed agents take the field, alone or alongside cwd.""" + + def test_accepted_on_a_plain_llm_agent(self) -> None: + agent = AgentDef(name="judge", prompt="review", settings_dir="/repo") + assert agent.settings_dir == "/repo" + + def test_defaults_to_none(self) -> None: + """Omitting it must add no directory -- the tier is opt-in.""" + assert AgentDef(name="judge", prompt="review").settings_dir is None + + def test_independent_of_working_dir(self) -> None: + """The point of the field: both set, to different directories. + + A wide cwd keeps every path the agent's MCP servers must reach inside + the single negotiated root, while the narrow settings_dir supplies the + target repository's conventions. + """ + agent = AgentDef( + name="judge", prompt="review", working_dir="/wide", settings_dir="/wide/repo" + ) + assert (agent.working_dir, agent.settings_dir) == ("/wide", "/wide/repo") + + def test_accepts_a_template(self) -> None: + """The directory a step reviews is normally an upstream step's output, + so the raw value is a Jinja template the engine renders.""" + agent = AgentDef( + name="judge", prompt="review", settings_dir="{{ setup.output.worktree_path }}" + ) + assert agent.settings_dir == "{{ setup.output.worktree_path }}" + + +class TestSettingsDirRejectedOnNonProviderSteps: + """Every step type with no LLM session rejects it. + + Accepting it silently is the failure mode that matters here: an author + would see a green ``conductor validate`` and conclude the target + repository's conventions were loaded when nothing had been. + """ + + @pytest.mark.parametrize( + ("kwargs",), + [ + ({"type": "wait", "duration": "1s"},), + ({"type": "set", "value": "x"},), + ({"type": "terminate", "status": "success", "reason": "done"},), + ({"type": "script", "command": "echo hi"},), + ({"type": "workflow", "workflow": "child.yaml"},), + ( + { + "type": "human_gate", + "prompt": "ok?", + "options": [GateOption(label="OK", value="ok", route="$end")], + }, + ), + ], + ) + def test_rejected(self, kwargs: dict) -> None: + with pytest.raises(ValidationError, match="cannot have 'settings_dir'"): + AgentDef(name="bad", settings_dir="/repo", **kwargs) + + def test_error_names_the_step_type(self) -> None: + """So the message says which step to fix, not merely that one is wrong.""" + with pytest.raises(ValidationError, match="wait agents cannot have 'settings_dir'"): + AgentDef(name="bad", type="wait", duration="1s", settings_dir="/repo") + + +class TestSettingsDirValidation: + """``conductor validate`` must not report success on a silent no-op. + + Every case here was green before these checks existed, which is the point: + the step-type rejections above guard authoring mistakes nobody makes, while + the three below are the ones an author actually makes -- wrong provider, + forgotten `setting_sources`, typo'd upstream step name. A green validate + for any of them tells the author their target repository's conventions + loaded when nothing did. + """ + + @staticmethod + def _config(provider: object, settings_dir: str, tmp_path: Path) -> WorkflowConfig: + return WorkflowConfig( + workflow=WorkflowDef( + name="w", + entry_point="a", + runtime=RuntimeConfig(provider=provider), # type: ignore[arg-type] + ), + agents=[ + AgentDef( + name="a", + prompt="hi", + settings_dir=settings_dir, + output={"r": OutputField(type="string")}, + routes=[RouteDef(to="$end")], + ) + ], + output={"r": "{{ a.output.r }}"}, + ) + + def test_rejected_on_a_provider_that_cannot_apply_it(self, tmp_path: Path) -> None: + """Only ``claude-agent-sdk`` has an ``add_dirs`` to put it in. + + Same class as ``working_dir``, whose capability docstring gives the + reason: silently ignoring the directory runs the agent against the + wrong repository while reporting success. + """ + config = self._config("copilot", str(tmp_path), tmp_path) + + with pytest.raises(ConfigurationError, match="does not apply it"): + validate_workflow_config(config) + + def test_accepted_on_claude_agent_sdk_with_setting_sources(self, tmp_path: Path) -> None: + """The supported combination raises nothing.""" + config = self._config( + ProviderSettings(name="claude-agent-sdk", setting_sources=["project"]), + str(tmp_path), + tmp_path, + ) + + validate_workflow_config(config) # no raise + + def test_warns_when_no_settings_tier_is_enabled(self, tmp_path: Path) -> None: + """A warning, not an error, and the distinction is load-bearing. + + Without ``setting_sources`` no ``project`` tier exists, so the skills + half -- the reason the field is normally set -- is a no-op. The + filesystem half still applies, so the workflow is not broken; erroring + would refuse a configuration that does something. + """ + config = self._config("claude-agent-sdk", str(tmp_path), tmp_path) + + warnings = validate_workflow_config(config) + + assert any("setting_sources" in w for w in warnings), warnings + assert any("no skills will be discovered" in w for w in warnings), warnings + + def test_template_referencing_an_unknown_step_is_caught(self, tmp_path: Path) -> None: + """The field is normally templated from an upstream step, so a typo'd + step name is the likely authoring error. It must fail at validate, as + the same typo in ``working_dir`` already does, rather than at run + time.""" + config = self._config( + ProviderSettings(name="claude-agent-sdk", setting_sources=["project"]), + "{{ nonexistent_step.output.path }}", + tmp_path, + ) + + with pytest.raises(ConfigurationError, match="nonexistent_step"): + validate_workflow_config(config) diff --git a/tests/test_engine/test_workflow.py b/tests/test_engine/test_workflow.py index 7679d2a8..1cc1639a 100644 --- a/tests/test_engine/test_workflow.py +++ b/tests/test_engine/test_workflow.py @@ -3583,6 +3583,7 @@ class _RecordingWorkingDirProvider: def __init__(self) -> None: self.seen: list[tuple[str, str | None]] = [] + self.seen_settings_dir: list[tuple[str, str | None]] = [] self.calls: int = 0 async def execute( @@ -3599,6 +3600,7 @@ async def execute( ): self.calls += 1 self.seen.append((agent.name, agent.working_dir)) + self.seen_settings_dir.append((agent.name, agent.settings_dir)) content = dict.fromkeys(agent.output or {}, f"{agent.name}-ok") return AgentOutput( content=content, @@ -3622,6 +3624,7 @@ def _single_agent_config( *, working_dir: str | None = None, runtime_working_dir: str | None = None, + settings_dir: str | None = None, model: str = "gpt-4", max_tokens: int | None = None, ) -> WorkflowConfig: @@ -3640,6 +3643,7 @@ def _single_agent_config( model=model, prompt="Do work", working_dir=working_dir, + settings_dir=settings_dir, output={"result": OutputField(type="string")}, routes=[RouteDef(to="$end")], ), @@ -4381,3 +4385,132 @@ async def _execute(agent, context, rendered_prompt, tools=None, **kwargs): envelope = [e for e in events if e.type == "for_each_item_started"] assert len(envelope) == 1 assert envelope[0].data == {"group_name": "fans", "item_key": "0", "index": 0} + + +class TestAgentSettingsDirResolution: + """Engine resolution of ``AgentDef.settings_dir``. + + ``settings_dir`` selects the directory whose Claude Code *project* + settings tier an agent reads skills from. It is resolved exactly like + ``working_dir`` -- shared code, so the two cannot drift -- but is + deliberately independent of it: ``working_dir`` becomes the session cwd, + which the CLI advertises as its sole MCP root, whereas ``settings_dir`` + only adds a tier to discover skills in. Keeping them separate is what + lets an agent hold a cwd wide enough for every path its MCP servers must + reach while still loading a narrower target repository's conventions. + """ + + @pytest.mark.asyncio + async def test_absolute_settings_dir_reaches_provider(self, tmp_path: Path) -> None: + """Requirement: the resolved directory is set on the ``AgentDef`` the + provider receives, which is where it becomes ``add_dirs``.""" + target = tmp_path / "repo" + target.mkdir() + provider = _RecordingWorkingDirProvider() + engine = WorkflowEngine( + _single_agent_config(settings_dir=str(target)), + provider, + workflow_path=_workflow_file(tmp_path), + ) + + await engine.run({}) + + assert provider.seen_settings_dir == [("worker", os.path.normpath(str(target)))] + + @pytest.mark.asyncio + async def test_settings_dir_does_not_become_working_dir(self, tmp_path: Path) -> None: + """The separation, asserted from the engine side. + + An agent naming only ``settings_dir`` must leave ``working_dir`` + unset, so the provider keeps its own cwd -- and with it the wide MCP + root. Coupling the two here would silently reintroduce the narrowing + the field exists to avoid. + """ + target = tmp_path / "repo" + target.mkdir() + provider = _RecordingWorkingDirProvider() + engine = WorkflowEngine( + _single_agent_config(settings_dir=str(target)), + provider, + workflow_path=_workflow_file(tmp_path), + ) + + await engine.run({}) + + assert provider.seen == [("worker", None)] + + @pytest.mark.asyncio + async def test_both_directories_resolve_independently(self, tmp_path: Path) -> None: + """The intended shape: a wide cwd and a narrow settings tier at once.""" + wide = tmp_path / "wide" + narrow = wide / "repo" + narrow.mkdir(parents=True) + provider = _RecordingWorkingDirProvider() + engine = WorkflowEngine( + _single_agent_config(working_dir=str(wide), settings_dir=str(narrow)), + provider, + workflow_path=_workflow_file(tmp_path), + ) + + await engine.run({}) + + assert provider.seen == [("worker", os.path.normpath(str(wide)))] + assert provider.seen_settings_dir == [("worker", os.path.normpath(str(narrow)))] + + @pytest.mark.asyncio + async def test_templated_settings_dir_is_rendered(self, tmp_path: Path) -> None: + """Requirement: Jinja-rendered against the per-agent context, since the + directory a step reviews is normally an upstream step's output.""" + target = tmp_path / "from-input" + target.mkdir() + provider = _RecordingWorkingDirProvider() + engine = WorkflowEngine( + _single_agent_config(settings_dir="{{ workflow.input.target }}"), + provider, + workflow_path=_workflow_file(tmp_path), + ) + + await engine.run({"target": str(target)}) + + assert provider.seen_settings_dir == [("worker", os.path.normpath(str(target)))] + + @pytest.mark.asyncio + async def test_relative_settings_dir_resolves_against_workflow_dir( + self, tmp_path: Path + ) -> None: + """Requirement: relative paths resolve against the workflow file's + directory, not the process cwd -- matching ``working_dir``.""" + (tmp_path / "sub").mkdir() + provider = _RecordingWorkingDirProvider() + engine = WorkflowEngine( + _single_agent_config(settings_dir="./sub"), + provider, + workflow_path=_workflow_file(tmp_path), + ) + + await engine.run({}) + + assert provider.seen_settings_dir == [("worker", os.path.normpath(str(tmp_path / "sub")))] + + @pytest.mark.asyncio + async def test_missing_settings_dir_raises_before_the_provider_call( + self, tmp_path: Path + ) -> None: + """Requirement: a bad path fails fast and names its own field. + + Naming ``settings_dir`` rather than ``working_dir`` is the point: the + two are resolved by shared code, and a message naming the wrong field + would send an author to correct a value that is already right. + """ + provider = _RecordingWorkingDirProvider() + engine = WorkflowEngine( + _single_agent_config(settings_dir=str(tmp_path / "nope")), + provider, + workflow_path=_workflow_file(tmp_path), + ) + + with pytest.raises(ExecutionError) as exc_info: + await engine.run({}) + + assert "settings_dir" in str(exc_info.value) + assert provider.calls == 0 diff --git a/tests/test_integration/test_mcp_roots_negotiation.py b/tests/test_integration/test_mcp_roots_negotiation.py new file mode 100644 index 00000000..7632f6f0 --- /dev/null +++ b/tests/test_integration/test_mcp_roots_negotiation.py @@ -0,0 +1,193 @@ +"""Pins the MCP Roots rule that governs what a filesystem MCP server permits. + +``settings_dir`` governs skill discovery while cwd alone governs what a +filesystem MCP server permits, and the two cannot be collapsed into one +option. Deriving ``ClaudeAgentOptions.add_dirs`` from the directory arguments +of every stdio MCP server looks like it would widen that server's scope back; +it cannot, and the reason is a property of the *server*, not of Conductor: + +``@modelcontextprotocol/server-filesystem`` uses the directories in its argv +only while the connected client does not support MCP Roots. A client that +advertises the ``roots`` capability is asked for its roots at +post-initialization, and whatever it answers **replaces** the argv +directories outright. The Claude CLI advertises Roots and offers exactly one +root -- its cwd -- so a server declared with two directories ends up +permitting one, and ``--add-dir`` cannot put the others back because it takes +no part in that negotiation. + +This test pins the rule itself with a hand-rolled JSON-RPC client and no LLM: +two runs, byte-identical but for the client's ``roots`` capability. That is +what isolates the cause to Roots negotiation rather than to cwd derivation, +to ``--add-dir`` handling, or to any Conductor code path -- and what would +fail if a future server version changed the precedence, which is the +assumption ``settings_dir`` rests on. +""" + +from __future__ import annotations + +import json +import os +import shutil +import subprocess +import tempfile +import time +from pathlib import Path + +import pytest + +_SERVER = "@modelcontextprotocol/server-filesystem" +_ADOPTED = "Updated allowed directories from MCP roots" + +pytestmark = pytest.mark.skipif( + shutil.which("npx") is None, reason="npx not available; needs the real filesystem MCP server" +) + + +def _list_allowed_directories( + *, root_dirs: list[str], cwd: str, roots: list[dict[str, str]] | None +) -> str: + """Call ``list_allowed_directories`` on a real server and return its text. + + ``roots=None`` declares no ``roots`` capability, so the server is never + asked and keeps its argv directories. A list declares the capability and + is what the server receives when it asks. + """ + with tempfile.NamedTemporaryFile(mode="w+", suffix=".err") as errf: + proc = subprocess.Popen( # noqa: S603 + ["npx", "-y", _SERVER, *root_dirs], # noqa: S607 + stdin=subprocess.PIPE, + stdout=subprocess.PIPE, + stderr=errf, + text=True, + cwd=cwd, + ) + assert proc.stdin is not None and proc.stdout is not None + + def send(payload: dict) -> None: + assert proc.stdin is not None + proc.stdin.write(json.dumps(payload) + "\n") + proc.stdin.flush() + + def call_tool() -> None: + send( + { + "jsonrpc": "2.0", + "id": 2, + "method": "tools/call", + "params": {"name": "list_allowed_directories", "arguments": {}}, + } + ) + + def server_stderr() -> str: + errf.flush() + return Path(errf.name).read_text(errors="replace") + + try: + send( + { + "jsonrpc": "2.0", + "id": 1, + "method": "initialize", + "params": { + "protocolVersion": "2024-11-05", + "capabilities": {"roots": {"listChanged": False}} if roots else {}, + "clientInfo": {"name": "conductor-roots-probe", "version": "1"}, + }, + } + ) + deadline = time.monotonic() + 120 + while time.monotonic() < deadline: + line = proc.stdout.readline() + if not line: + pytest.fail(f"server exited early; stderr:\n{server_stderr()}") + message = json.loads(line) + + if message.get("id") == 1: + send({"jsonrpc": "2.0", "method": "notifications/initialized"}) + # A client with no roots capability is never asked for + # roots, so nothing else will arrive to sequence against. + if roots is None: + call_tool() + elif message.get("method") == "roots/list": + send({"jsonrpc": "2.0", "id": message["id"], "result": {"roots": roots}}) + # The server swaps its allowlist in the continuation of its + # own ``listRoots()`` await, so a tool call sent straight + # after this reply races it and reads the pre-swap list -- + # which is exactly the false negative that made an earlier + # version of this probe report "argv roots survive". + swap = time.monotonic() + 30 + while _ADOPTED not in server_stderr() and time.monotonic() < swap: + time.sleep(0.05) + call_tool() + elif message.get("id") == 2: + return str(message["result"]["content"][0]["text"]) + pytest.fail(f"timed out; stderr:\n{server_stderr()}") + finally: + proc.kill() + proc.wait(timeout=30) + raise AssertionError("unreachable") + + +@pytest.fixture +def roots_tree(tmp_path: Path) -> dict[str, str]: + """Two declared server roots and a cwd that is neither, nor kin to either.""" + for name in ("rootA", "rootB", "cwdC"): + (tmp_path / name).mkdir() + (tmp_path / "rootB" / "target.txt").write_text("hello-from-rootB\n") + return {n: str(tmp_path / n) for n in ("rootA", "rootB", "cwdC")} + + +def test_argv_roots_honoured_when_client_declares_no_roots(roots_tree: dict[str, str]) -> None: + """The server is not broken and cwd is irrelevant to it. + + Without the capability the argv directories are the allowlist, in full -- + the baseline that makes the contrast below attributable to negotiation. + """ + allowed = _list_allowed_directories( + root_dirs=[roots_tree["rootA"], roots_tree["rootB"]], + cwd=roots_tree["cwdC"], + roots=None, + ) + + assert roots_tree["rootA"] in allowed + assert roots_tree["rootB"] in allowed + assert roots_tree["cwdC"] not in allowed + + +def test_single_advertised_root_replaces_every_argv_root(roots_tree: dict[str, str]) -> None: + """The defect, in one assertion: a client's sole root wins outright. + + Identical argv and identical cwd to the test above. Declaring ``roots`` + and answering with cwd alone -- what the Claude CLI does -- discards both + declared directories. No value of ``add_dirs`` changes this, which is why + ``settings_dir`` governs skill discovery and cwd alone governs MCP scope. + """ + allowed = _list_allowed_directories( + root_dirs=[roots_tree["rootA"], roots_tree["rootB"]], + cwd=roots_tree["cwdC"], + roots=[{"uri": f"file://{roots_tree['cwdC']}", "name": "cwd"}], + ) + + assert roots_tree["cwdC"] in allowed + assert roots_tree["rootA"] not in allowed + assert roots_tree["rootB"] not in allowed + + +def test_a_root_containing_both_declared_roots_permits_both(roots_tree: dict[str, str]) -> None: + """Why a single root is sufficient, and the basis of the recommended fix. + + One advertised root still permits everything beneath it, so an agent whose + cwd is a common parent reaches both declared roots. That is what makes + "keep cwd wide, select conventions with ``settings_dir``" work rather than + needing a server per root. + """ + parent = str(Path(roots_tree["rootA"]).parent) + + allowed = _list_allowed_directories( + root_dirs=[roots_tree["rootA"], roots_tree["rootB"]], + cwd=parent, + roots=[{"uri": f"file://{parent}", "name": "cwd"}], + ) + + assert parent in allowed + assert os.path.commonpath([parent, roots_tree["rootB"]]) == parent diff --git a/tests/test_providers/test_claude_agent_sdk.py b/tests/test_providers/test_claude_agent_sdk.py index b2150328..d062c902 100644 --- a/tests/test_providers/test_claude_agent_sdk.py +++ b/tests/test_providers/test_claude_agent_sdk.py @@ -3083,3 +3083,212 @@ def test_windows_skips_only_the_driveless_path(self) -> None: "a Windows user's local install must still be found" ) assert any(p.endswith(".npm-global/bin/claude") for p in probed) + + +class TestSettingsDirAddDirs: + """``settings_dir`` is the only source of ``ClaudeAgentOptions.add_dirs``. + + The mechanism these tests rest on is measured rather than reasoned about + (``tests/test_integration/test_mcp_roots_negotiation.py`` pins it against + the real server without an LLM): ``@modelcontextprotocol/server-filesystem`` + uses its argv directories only when the client does not support MCP Roots; + the Claude CLI does support Roots and advertises exactly one, its cwd; so + the server discards its argv directories and permits cwd alone. + ``--add-dir`` does not participate in that negotiation, so it cannot be + used to widen what an MCP server permits -- which is why this field is + fed only by the author's ``settings_dir`` and never derived from server + arguments. + + What ``add_dirs`` does do is make a directory's *project* settings tier + contribute its skills, independent of cwd -- which is what lets an agent + keep a cwd wide enough for its MCP servers while loading a narrower + target repository's conventions. + """ + + @patch("conductor.providers.claude_agent_sdk.CLAUDE_AGENT_SDK_AVAILABLE", True) + async def test_settings_dir_becomes_add_dirs(self, tmp_path: Path) -> None: + """Requirement: the authored directory reaches the SDK option.""" + captured: dict = {} + + async def fake_query(**kwargs): + captured["add_dirs"] = kwargs["options"].add_dirs + 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", settings_dir=str(tmp_path)), + context={}, + rendered_prompt="hi", + ) + + assert captured["add_dirs"] == [str(tmp_path)] + + @patch("conductor.providers.claude_agent_sdk.CLAUDE_AGENT_SDK_AVAILABLE", True) + async def test_no_settings_dir_sends_no_add_dirs(self) -> None: + """An agent that names no directory adds none. + + The empty list matters: the CLI would otherwise be handed a directory + whose skills, being in an enabled settings tier, become listed and + invocable -- ambient content the workflow never declared. + """ + captured: dict = {} + + async def fake_query(**kwargs): + captured["add_dirs"] = kwargs["options"].add_dirs + 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["add_dirs"] == [] + + @patch("conductor.providers.claude_agent_sdk.CLAUDE_AGENT_SDK_AVAILABLE", True) + async def test_stdio_server_dir_args_are_not_forwarded(self, tmp_path: Path) -> None: + """The regression this class is named for. + + A stdio server's own directory arguments must NOT reach ``add_dirs``. + Forwarding them was measurably ineffective, and reinstating it would + silently widen skill discovery to every declared MCP root -- granting + content from directories the author named as *data*, not as a source + of conventions. + """ + root_a = tmp_path / "rootA" + root_a.mkdir() + root_b = tmp_path / "rootB" + root_b.mkdir() + captured: dict = {} + + async def fake_query(**kwargs): + captured["add_dirs"] = kwargs["options"].add_dirs + yield _result(result="ok") + + with patch("conductor.providers.claude_agent_sdk.query", fake_query): + provider = ClaudeAgentSdkProvider( + mcp_servers={ + "filesystem": { + "type": "stdio", + "command": "npx", + "args": [ + "-y", + "@modelcontextprotocol/server-filesystem", + str(root_a), + str(root_b), + ], + } + } + ) + await provider.execute( + agent=AgentDef(name="t", prompt="hi"), context={}, rendered_prompt="hi" + ) + + assert captured["add_dirs"] == [] + + @patch("conductor.providers.claude_agent_sdk.CLAUDE_AGENT_SDK_AVAILABLE", True) + async def test_settings_dir_is_independent_of_cwd(self, tmp_path: Path) -> None: + """The whole point of the field: the two directories are unrelated. + + ``cwd`` becomes the session's sole MCP root; ``settings_dir`` only + adds a project tier to read skills from. An agent must be able to set + a wide cwd and a narrow settings_dir at once -- neither derived from + nor constrained by the other. + """ + wide = tmp_path / "wide" + wide.mkdir() + narrow = wide / "repo" + narrow.mkdir() + captured: dict = {} + + async def fake_query(**kwargs): + captured["cwd"] = kwargs["options"].cwd + captured["add_dirs"] = kwargs["options"].add_dirs + 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", + working_dir=str(wide), + settings_dir=str(narrow), + ), + context={}, + rendered_prompt="hi", + ) + + assert captured["cwd"] == str(wide) + assert captured["add_dirs"] == [str(narrow)] + + @patch("conductor.providers.claude_agent_sdk.CLAUDE_AGENT_SDK_AVAILABLE", True) + async def test_settings_dir_does_not_become_a_second_cwd(self, tmp_path: Path) -> None: + """The boundary of what this field can deliver, pinned deliberately. + + ``add_dirs`` carries a directory's ``.claude/skills`` and nothing + else: ``CLAUDE.md``, ``.claude/rules/*.md``, ``.claude/settings.json`` + (so ``env`` and ``hooks``) and ``.claude/agents`` all follow cwd + instead -- measured against the CLI, not inferred. So a + ``settings_dir`` must never be quietly promoted into ``cwd`` in an + attempt to widen what it loads: that would hand the agent the narrow + directory as its sole MCP root, which is the exact defect this field + exists to avoid. + + An agent needing a repository's rules *and* a wide cwd cannot have + both from these two fields, and this test is what keeps that trade + visible rather than papered over. + """ + wide = tmp_path / "wide" + wide.mkdir() + narrow = wide / "repo" + narrow.mkdir() + captured: dict = {} + + async def fake_query(**kwargs): + captured["cwd"] = kwargs["options"].cwd + captured["add_dirs"] = kwargs["options"].add_dirs + 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", working_dir=str(wide), settings_dir=str(narrow) + ), + context={}, + rendered_prompt="hi", + ) + + # The line that pins it: cwd is the wide directory exactly. The + # earlier `narrow not in cwd` substring check added nothing -- it also + # passes for an implementation that sets cwd to an unrelated third + # directory, so it read as a guard without being one. + assert captured["cwd"] == str(wide) + assert captured["add_dirs"] == [str(narrow)] + + @patch("conductor.providers.claude_agent_sdk.CLAUDE_AGENT_SDK_AVAILABLE", True) + async def test_settings_dir_passed_verbatim(self, tmp_path: Path) -> None: + """Not re-resolved, matching ``cwd``: the engine already rendered, + absolutized and existence-checked it, and ``resolve()`` here would + collapse the symlink aliases the engine preserves on purpose.""" + real = tmp_path / "real" + real.mkdir() + link = tmp_path / "link" + link.symlink_to(real, target_is_directory=True) + captured: dict = {} + + async def fake_query(**kwargs): + captured["add_dirs"] = kwargs["options"].add_dirs + 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", settings_dir=str(link)), + context={}, + rendered_prompt="hi", + ) + + assert captured["add_dirs"] == [str(link)] From 19ed3263e59e12f71f5442abffb7c94d72dbafbb Mon Sep 17 00:00:00 2001 From: Chris Throup Date: Tue, 8 Sep 2026 21:30:30 +0200 Subject: [PATCH 02/10] fix(claude-agent-sdk): address review on settings_dir Blocking: - docs/providers/experimental.md: the capability row had been rewritten with text describing per-agent `tools:` allowlist enforcement -- enumerating stdio MCP servers, refusing an allowlist alongside http/sse, an `mcp_servers_always_attached` capability. None of that exists here: `workflow_tools_passthrough` is False, `_resolve_tool_config` refuses any non-empty allowlist, and the identifier appears nowhere in `src/`. The row also lost the accurate `no workflow_tools_passthrough` carve-out. Restored to upstream's text plus one additive sentence for `settings_dir`, so the table no longer promises a security property the provider does not have. - Added `AgentExecutor._reject_unsupported_settings_dir`, mirroring the `capabilities.settings_dir` check at run time. `conductor run` never calls the static validator, and the engine resolves the directory for every provider, so `provider: copilot` with a `settings_dir` previously ran against whatever conventions its cwd supplied and exited 0. Follows the four existing `_reject_*` helpers and `session_key`'s precedent; the two docstrings that promised validate-time-only rejection now describe both. - An empty `settings_dir` reached `add_dirs` as a real filesystem grant. `Path("")` is `Path(".")`, which is not absolute, so it resolved to the workflow file's own directory and passed the `is_dir()` check. Four layers disagreed on what "set" meant; they now agree: the field takes `StringConstraints(strip_whitespace=True, min_length=1)` (matching `session_key`), the seven step-type guards use `is not None` so a blank cannot bypass them, and the engine refuses a *template* that renders empty. Recommended: - `_project_tier_enabled` replaces `_setting_sources_enabled`: `settings_dir` feeds only the `project` tier, so `['user']`/`['local']` no longer validate silently, and a per-agent `skills: []` -- which zeroes the tier in `execute` -- is now detected. - The no-skills warning branches on its cause. Under a per-agent provider override it no longer advises adding `setting_sources`, which the schema rejects unless `runtime.provider` is claude-agent-sdk. - docs/workflow-syntax.md said the field was "ignored by every other provider"; it is refused. The provider comment led with the skills effect and omitted the unconditional filesystem grant -- the grant now comes first. - The schema docstring example used a mapping under `agents:`, which the schema rejects. - `_resolve_agent_directory` takes `field` keyword-only as a `Literal["working_dir", "settings_dir"]`, so the two adjacent `str` parameters can no longer be transposed (`ty` now rejects a bad name). - The validator sub-agent sets `settings_dir=None` explicitly: it runs with `tools=[]`, so neither half would apply, and inheriting it would grant a tree it cannot use. Not switched to `model_copy`, which would also carry `validator` (recursive validation), `session_key` and `routes`. - The roots-negotiation test is marked `real_api` (it fetches from npm and pins upstream behaviour, not ours), passes the resolved `npx` path so it can launch `npx.cmd` on Windows, and writes stderr to a plain file rather than reopening a `NamedTemporaryFile` by name. - The bundled example declares its three inputs, which turns on the unknown-input check for the `settings_dir` template this PR added -- a typo there was previously undetectable by `make validate-examples`. - Tests: the missing `questions` step-type case, an argv assertion that `--add-dir` still reaches the CLI (with a negative control), the blank and empty-render refusals, all three warning causes, and the run-time capability refusal. Every new branch was mutation-tested. --- docs/providers/experimental.md | 2 +- docs/workflow-syntax.md | 8 +- examples/claude-agent-sdk-settings-dir.yaml | 21 ++++ src/conductor/config/schema.py | 25 ++-- src/conductor/config/validator.py | 60 +++++++-- src/conductor/engine/validator.py | 13 ++ src/conductor/engine/workflow.py | 29 ++++- src/conductor/executor/agent.py | 43 +++++++ src/conductor/providers/capabilities.py | 13 +- src/conductor/providers/claude_agent_sdk.py | 23 ++-- tests/test_config/test_settings_dir_schema.py | 116 ++++++++++++++++++ tests/test_engine/test_workflow.py | 27 ++++ tests/test_executor/test_agent.py | 54 ++++++++ .../test_mcp_roots_negotiation.py | 30 ++++- tests/test_providers/test_claude_agent_sdk.py | 47 +++++++ 15 files changed, 460 insertions(+), 51 deletions(-) diff --git a/docs/providers/experimental.md b/docs/providers/experimental.md index cc90bd1d..b6df0da3 100644 --- a/docs/providers/experimental.md +++ b/docs/providers/experimental.md @@ -101,7 +101,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.2.82` | `@lesandiz (best-effort)` | no `reasoning_effort`, `prompt_injection` structured output, no `checkpoint_resume` (agents without a `session_key` carry no session state across a resume). 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). Supports `workflow_tools_passthrough`: a per-agent `tools:` allowlist is enforced by enumerating the declared stdio MCP servers and denying every tool not on the list (plus the built-in write/exec tools), since the CLI's `allowed_tools` only pre-approves and does not restrict. An allowlist alongside an http/sse server is refused — those cannot be enumerated, so the denial set would be unknown. `tools: []` alongside `mcp_servers:` is still refused (`mcp_servers_always_attached`): honoring an allowlist does not mean the provider can detach a declared server. Supports `working_dir` as of [#348](https://github.com/microsoft/conductor/issues/348); the CLI would load `CLAUDE.md` and `.claude/settings*.json` from that directory, but `setting_sources` defaults to empty as of [#352](https://github.com/microsoft/conductor/issues/352) so ambient instructions, settings, hooks, and skills are not inherited; a workflow opts back in per run with `runtime.provider.setting_sources`, and chooses per agent which directory the `project` tier reads skills from via `settings_dir` (cwd alone governs the CLI's sole MCP root, so the two are deliberately separate) — see [Target-Repository Skills](../workflow-syntax.md#target-repository-skills-settings_dir). Declares `session_continuity`: an agent with a `session_key` reuses one Claude session across executions, and the session map survives `conductor resume` — see [Session Continuity](../workflow-syntax.md#session-continuity-session_key). | +| `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` (agents without a `session_key` carry no session state across a resume). 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). Supports `working_dir` as of [#348](https://github.com/microsoft/conductor/issues/348); the CLI would load `CLAUDE.md` and `.claude/settings*.json` from that directory, but `setting_sources` is empty by default as of [#352](https://github.com/microsoft/conductor/issues/352) so ambient instructions, settings, hooks, and skills are not inherited unless a workflow opts in via `runtime.provider.setting_sources` ([#501](https://github.com/microsoft/conductor/issues/501)) — which loads the named tiers **including their hooks**, so only for repositories trusted as much as the workflow. Which directory that `project` tier reads **skills** from is chosen per agent with `settings_dir` (cwd alone governs the CLI's sole MCP root, so the two are deliberately separate) — see [Target-Repository Skills](../workflow-syntax.md#target-repository-skills-settings_dir). Declares `session_continuity`: an agent with a `session_key` reuses one Claude session across executions, and the session map survives `conductor resume` — see [Session Continuity](../workflow-syntax.md#session-continuity-session_key). | | `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/docs/workflow-syntax.md b/docs/workflow-syntax.md index bf1c1628..e3f2f34b 100644 --- a/docs/workflow-syntax.md +++ b/docs/workflow-syntax.md @@ -405,9 +405,11 @@ Because paths are normalized lexically instead of resolving to their real paths: ### Target-Repository Skills (`settings_dir`) `settings_dir` names a second directory whose Claude Code *project* settings -tier the agent reads skills from. It applies to `claude-agent-sdk` agents in a -workflow that sets `runtime.provider.setting_sources`, and is ignored by every -other provider. +tier the agent reads skills from. It applies only to `claude-agent-sdk` agents. +Setting it against any other provider is an **error**, reported by `conductor +validate` and again at run time — not a silently dropped field. The skills half +additionally requires `runtime.provider.setting_sources` to enable the `project` +tier; the filesystem grant below applies either way. ```yaml workflow: diff --git a/examples/claude-agent-sdk-settings-dir.yaml b/examples/claude-agent-sdk-settings-dir.yaml index 1f6afbb3..658531aa 100644 --- a/examples/claude-agent-sdk-settings-dir.yaml +++ b/examples/claude-agent-sdk-settings-dir.yaml @@ -67,6 +67,27 @@ workflow: version: "1.0.0" entry_point: review + input: + workspace: + type: string + required: true + description: > + Wide directory containing BOTH the repository under review and the + artifacts directory. Becomes every agent's cwd, and therefore the + filesystem MCP server's only root. + repo: + type: string + required: true + description: > + The repository under review, inside `workspace`. Its own + `.claude/skills` are loaded via `settings_dir`. + artifacts: + type: string + required: true + description: > + Prior findings, inside `workspace` but OUTSIDE `repo` -- the path that + a narrowed cwd would make unreadable. + runtime: provider: name: claude-agent-sdk diff --git a/src/conductor/config/schema.py b/src/conductor/config/schema.py index 5309ad7c..95ca90b5 100644 --- a/src/conductor/config/schema.py +++ b/src/conductor/config/schema.py @@ -1381,11 +1381,14 @@ class AgentDef(BaseModel): wait/set/terminate/human_gate/workflow step types. """ - settings_dir: str | None = None + settings_dir: Annotated[str, StringConstraints(strip_whitespace=True, min_length=1)] | None = ( + None + ) """Directory whose Claude Code *project* settings tier this agent loads. - ``claude-agent-sdk`` only -- ``conductor validate`` refuses it against a - provider that cannot apply it, rather than dropping it silently. Resolved + ``claude-agent-sdk`` only -- a provider that cannot apply it refuses it + both at ``conductor validate`` and at run time, rather than dropping it + silently (``conductor run`` never calls the static validator). Resolved by the engine exactly like :attr:`working_dir` (Jinja-rendered, ``~``-expanded, made absolute against the workflow file's directory, ``normpath``-normalised, existence-checked), then forwarded to the SDK as @@ -1435,7 +1438,7 @@ class AgentDef(BaseModel): from a sibling directory:: agents: - judge: + - name: judge settings_dir: "{{ setup_worktree.output.worktree_path }}" # No working_dir: cwd stays the launch directory, which contains # both the worktree and the artifacts the judge must read. @@ -2090,7 +2093,7 @@ def validate_agent_type(self) -> AgentDef: raise ValueError("human_gate agents cannot have 'output_mode'") if self.working_dir: raise ValueError("human_gate agents cannot have 'working_dir'") - if self.settings_dir: + if self.settings_dir is not None: raise ValueError("human_gate agents cannot have 'settings_dir'") if self.session_key is not None: raise ValueError("human_gate agents cannot have 'session_key'") @@ -2155,7 +2158,7 @@ def validate_agent_type(self) -> AgentDef: raise ValueError("questions agents cannot have 'output_mode'") if self.working_dir: raise ValueError("questions agents cannot have 'working_dir'") - if self.settings_dir: + if self.settings_dir is not None: raise ValueError("questions agents cannot have 'settings_dir'") if self.session_key is not None: raise ValueError("questions agents cannot have 'session_key'") @@ -2190,7 +2193,7 @@ def validate_agent_type(self) -> AgentDef: raise ValueError("script agents cannot have 'validator'") if self.sandbox is not None: raise ValueError("script agents cannot have 'sandbox'") - if self.settings_dir: + if self.settings_dir is not None: raise ValueError("script agents cannot have 'settings_dir'") if self.max_depth is not None: raise ValueError("script agents cannot have 'max_depth'") @@ -2258,7 +2261,7 @@ def validate_agent_type(self) -> AgentDef: raise ValueError("workflow agents cannot have 'output_mode'") if self.working_dir: raise ValueError("workflow agents cannot have 'working_dir'") - if self.settings_dir: + if self.settings_dir is not None: raise ValueError("workflow agents cannot have 'settings_dir'") elif self.type == "wait": if self.duration is None: @@ -2283,7 +2286,7 @@ def validate_agent_type(self) -> AgentDef: raise ValueError("wait agents cannot have 'env'") if self.working_dir: raise ValueError("wait agents cannot have 'working_dir'") - if self.settings_dir: + if self.settings_dir is not None: raise ValueError("wait agents cannot have 'settings_dir'") if self.timeout is not None: raise ValueError("wait agents cannot have 'timeout'") @@ -2358,7 +2361,7 @@ def validate_agent_type(self) -> AgentDef: raise ValueError("set agents cannot have 'env'") if self.working_dir: raise ValueError("set agents cannot have 'working_dir'") - if self.settings_dir: + if self.settings_dir is not None: raise ValueError("set agents cannot have 'settings_dir'") if self.timeout is not None: raise ValueError("set agents cannot have 'timeout'") @@ -2434,7 +2437,7 @@ def validate_agent_type(self) -> AgentDef: raise ValueError("terminate agents cannot have 'env'") if self.working_dir: raise ValueError("terminate agents cannot have 'working_dir'") - if self.settings_dir: + if self.settings_dir is not None: raise ValueError("terminate agents cannot have 'settings_dir'") if self.timeout is not None: raise ValueError("terminate agents cannot have 'timeout'") diff --git a/src/conductor/config/validator.py b/src/conductor/config/validator.py index 779ceff9..1442cf15 100644 --- a/src/conductor/config/validator.py +++ b/src/conductor/config/validator.py @@ -1812,16 +1812,23 @@ def _is_llm_agent(agent: AgentDef) -> bool: return agent.type in _LLM_AGENT_TYPES -def _setting_sources_enabled(config: WorkflowConfig) -> bool: - """True iff the workflow enables any Claude Code settings tier. +def _project_tier_enabled(config: WorkflowConfig, agent: AgentDef) -> bool: + """True iff this agent's session will enable the ``project`` settings tier. + + ``settings_dir`` feeds the ``project`` tier and nothing else, so any tier + is not enough: ``user`` reads ``~/.claude`` and ``local`` is cwd-bound, so + neither can make a ``settings_dir``'s skills discoverable. A per-agent + ``skills: []`` opts the agent out of the tiers entirely + (``claude_agent_sdk.py::execute`` computes ``effective_sources`` that way), + so the check is per agent rather than per workflow. ``runtime.provider`` is either the bare string shorthand (no tiers, by - definition) or a ``ProviderSettings`` carrying ``setting_sources``. An - empty or absent list means Conductor sends ``[]`` -- load nothing ambient - -- so no ``project`` tier exists for a ``settings_dir`` to be read from. + definition) or a ``ProviderSettings`` carrying ``setting_sources``. """ + if agent.skills == []: + return False provider = config.workflow.runtime.provider - return bool(getattr(provider, "setting_sources", None)) + return "project" in (getattr(provider, "setting_sources", None) or []) def _resolved_provider_name(agent: AgentDef, default: str) -> str: @@ -2481,19 +2488,46 @@ def _check_agent_capabilities( f"has a surface for it; use working_dir, or move this agent to " f"that provider." ) - elif agent.settings_dir is not None and not _setting_sources_enabled(config): + elif agent.settings_dir is not None and not _project_tier_enabled(config, agent): # A warning, not an error: the FILESYSTEM half of settings_dir # applies regardless, so the workflow is not broken -- but the # skill discovery it is normally set for is a no-op without the # project tier enabled, and a green validate would imply otherwise. - warnings.append( + # + # Three distinct causes, each with a different remedy (or none), so + # the message branches rather than prescribing one fix that may be + # impossible to apply. + common = ( f"Agent '{agent.name}' sets settings_dir={agent.settings_dir!r} but " - f"the workflow does not set runtime.provider.setting_sources, so no " - f"settings tier is enabled and no skills will be discovered from it. " - f"The directory is still granted to the model's built-in file tools. " - f"Add 'setting_sources: [project]' to runtime.provider to load that " - f"repository's skills." + f"its session will not enable the 'project' settings tier, so no " + f"skills will be discovered from that directory. The directory is " + f"still granted to the model's built-in file tools." ) + if agent.skills == []: + warnings.append( + f"{common} The agent's own 'skills: []' opts it out of the " + f"settings tiers entirely. Remove it to let the tier apply, or " + f"remove settings_dir if the filesystem grant was not intended." + ) + elif provider_name != default_provider: + # setting_sources lives on the single workflow-level + # ProviderSettings and the schema rejects it unless that + # provider is claude-agent-sdk, so telling this author to add + # it would produce a ValidationError. + warnings.append( + f"{common} The settings tier is workflow-scoped " + f"(runtime.provider.setting_sources) and cannot be enabled for " + f"an agent that overrides its provider, since the schema " + f"accepts setting_sources only when runtime.provider is " + f"'claude-agent-sdk' (it is {default_provider!r}). Move " + f"the provider to runtime.provider to enable the tier, or keep " + f"settings_dir for the filesystem grant alone." + ) + else: + warnings.append( + f"{common} Add 'project' to runtime.provider.setting_sources to " + f"load that repository's skills." + ) # session_key: a provider that ignores it starts a fresh session every # execution, silently discarding the context the author asked to keep. diff --git a/src/conductor/engine/validator.py b/src/conductor/engine/validator.py index 611ea32d..02834e82 100644 --- a/src/conductor/engine/validator.py +++ b/src/conductor/engine/validator.py @@ -227,6 +227,19 @@ def _build_validator_agent(self, agent: AgentDef) -> AgentDef: tools=[], output=_VALIDATOR_OUTPUT_SCHEMA, working_dir=agent.working_dir, + # Deliberately NOT inherited, unlike working_dir. This grader runs + # with ``tools=[]``, so neither half of settings_dir would do + # anything for it: no skill can be invoked without the Skill tool, + # and the filesystem grant has no file tool to widen. Inheriting it + # would hand the grader access to a tree it cannot use, which is a + # wider grant than the run needs. + # + # Built field by field rather than by ``model_copy`` for the same + # reason: a copy would also carry ``validator`` (making the grader + # validate itself), ``session_key`` (two sessions appending to one + # transcript, which config/validator.py refuses for concurrent + # executions) and ``routes``. Add new fields here explicitly. + settings_dir=None, ) def _parse(self, content: Any) -> tuple[bool, list[str], bool]: diff --git a/src/conductor/engine/workflow.py b/src/conductor/engine/workflow.py index fee0c394..55426ee9 100644 --- a/src/conductor/engine/workflow.py +++ b/src/conductor/engine/workflow.py @@ -17,7 +17,7 @@ import uuid from dataclasses import dataclass, field from pathlib import Path -from typing import TYPE_CHECKING, Any +from typing import TYPE_CHECKING, Any, Literal from conductor.duration import parse_duration from conductor.engine.checkpoint import CheckpointManager, CheckpointTrigger @@ -594,7 +594,12 @@ def _workflow_dir(self) -> Path | None: return Path(self.workflow_path).resolve().parent if self.workflow_path else None def _resolve_agent_directory( - self, agent: AgentDef, agent_context: dict[str, Any], field: str, raw: str + self, + agent: AgentDef, + agent_context: dict[str, Any], + *, + field: Literal["working_dir", "settings_dir"], + raw: str, ) -> str: """Render and absolutize one authored directory value. @@ -611,6 +616,22 @@ def _resolve_agent_directory( — before any provider call. """ rendered = self.renderer.render(raw, agent_context) + # A template can render empty even when the raw value passed the schema's + # min_length -- an --input given as `repo=`, a script step that printed + # nothing, a `set` binding evaluating to "". Path("") is Path("."), which + # is not absolute, so it would join onto the workflow file's own directory + # and pass the is_dir() check below. For settings_dir that silently grants + # the model file access to the workflow's own tree, so refuse it here. + if not rendered.strip(): + raise ExecutionError( + f"Agent '{agent.name}': {field} rendered to an empty string from '{raw}'", + agent_name=agent.name, + suggestion=( + f"An empty value would resolve to the workflow file's own " + f"directory. Check that the input or upstream step feeding " + f"{field} produced a path." + ), + ) path = Path(rendered).expanduser() if not path.is_absolute(): base = self._workflow_dir if self._workflow_dir is not None else Path.cwd() @@ -659,12 +680,12 @@ def _resolve_agent_working_dir( raw = self.config.workflow.runtime.working_dir if raw is not None: update["working_dir"] = self._resolve_agent_directory( - agent, agent_context, "working_dir", raw + agent, agent_context, field="working_dir", raw=raw ) if agent.settings_dir is not None: update["settings_dir"] = self._resolve_agent_directory( - agent, agent_context, "settings_dir", agent.settings_dir + agent, agent_context, field="settings_dir", raw=agent.settings_dir ) if not update: diff --git a/src/conductor/executor/agent.py b/src/conductor/executor/agent.py index 21381f81..d3f98602 100644 --- a/src/conductor/executor/agent.py +++ b/src/conductor/executor/agent.py @@ -381,6 +381,7 @@ async def execute( # decided by the agent and the provider alone, so there is nothing to # learn from rendering a prompt or calling a model first. self._reject_unsupported_session_key(agent) + self._reject_unsupported_settings_dir(agent) # Render model field if it contains template expressions if is_jinja_template(agent.model): @@ -1130,3 +1131,45 @@ def _reject_unsupported_session_key(self, agent: AgentDef) -> None: "reports this before a run starts." ), ) + + def _reject_unsupported_settings_dir(self, agent: AgentDef) -> None: + """Refuse ``settings_dir`` on a provider that cannot apply it. + + Mirrors the ``capabilities.settings_dir`` check in + :func:`conductor.config.validator.validate_workflow_config`. + ``conductor validate`` already rejects the combination, but + ``conductor run`` never invokes the static validator — and the + engine renders, absolutizes and existence-checks the directory for + *every* provider, so an author sees the field processed and then + handed to a provider that never reads it. The agent answers from + whatever conventions its cwd happened to supply and the run exits 0, + which is the silent-wrong-answer case the capability exists to stop. + + A provider with no ``CAPABILITIES`` is left alone, for the reason + given in :meth:`_reject_unsupported_skills`. + + Args: + agent: The agent whose ``settings_dir`` is being checked. Agents + that declare none return immediately. + + Raises: + ExecutionError: If the provider declares + ``capabilities.settings_dir=False``. + """ + if agent.settings_dir is None: + return + capabilities = getattr(type(self.provider), "CAPABILITIES", None) + if capabilities is None or capabilities.settings_dir: + return + raise ExecutionError( + f"Agent '{agent.name}' sets settings_dir={agent.settings_dir!r} but " + f"provider '{type(self.provider).__name__}' does not apply it " + f"(capabilities.settings_dir=False), so the agent would load " + f"whatever conventions its working directory supplies instead.", + agent_name=agent.name, + suggestion=( + "Use working_dir, or override the agent to a provider that " + "applies it (claude-agent-sdk). 'conductor validate' reports " + "this before a run starts." + ), + ) diff --git a/src/conductor/providers/capabilities.py b/src/conductor/providers/capabilities.py index 404c1ac5..94828c41 100644 --- a/src/conductor/providers/capabilities.py +++ b/src/conductor/providers/capabilities.py @@ -147,11 +147,14 @@ class ProviderCapabilities(BaseModel): """``True`` when the provider applies an agent's resolved ``settings_dir``. Workflows that set ``settings_dir`` against a provider with - ``settings_dir=False`` fail validation, for the same reason - ``working_dir`` does: the field selects which repository's conventions the - agent loads *and* widens the model's built-in file tools to that tree, so - silently ignoring it would run the agent against the wrong conventions - while reporting success. Distinct from ``working_dir`` because the two are + ``settings_dir=False`` are refused twice -- by ``conductor validate`` and + again at run time by + :meth:`conductor.executor.agent.AgentExecutor._reject_unsupported_settings_dir`, + because ``conductor run`` never invokes the static validator. Both are + needed for the same reason: the field selects which repository's + conventions the agent loads *and* widens the model's built-in file tools + to that tree, so silently ignoring it would run the agent against the + wrong conventions while reporting success. Distinct from ``working_dir`` because the two are deliberately independent axes -- cwd is the sole root a filesystem MCP server gets, while this only adds a directory. Defaults to ``False`` (conservative).""" diff --git a/src/conductor/providers/claude_agent_sdk.py b/src/conductor/providers/claude_agent_sdk.py index 243d150d..99edc667 100644 --- a/src/conductor/providers/claude_agent_sdk.py +++ b/src/conductor/providers/claude_agent_sdk.py @@ -996,15 +996,22 @@ async def _execute_session( # so pass it through verbatim rather than re-resolving — that would # collapse the symlink aliases the engine preserves. cwd=resolved_cwd, - # The authored ``settings_dir`` and nothing else. + # The authored ``settings_dir`` and nothing else. Two effects, + # and the order matters because the second is easy to miss. # - # What this does, measured: it makes a directory's *project* - # settings tier discoverable — its ``.claude/skills`` become - # listed and invocable with cwd elsewhere entirely, and only - # those, not CLAUDE.md, .claude/rules/*.md, .claude/settings.json - # or .claude/agents, which all stay with cwd. So it is the skills - # portion of a project tier rather than a cwd-independent way to - # load one. + # (1) UNCONDITIONAL: per the SDK's own contract this is + # "additional directories Claude can access beyond the current + # working directory", so this line widens the model's built-in + # Read/Edit/Bash to that tree with no settings tier enabled at + # all (measured). It does NOT widen what an MCP server permits. + # + # (2) CONDITIONAL on ``setting_sources`` enabling the ``project`` + # tier: the directory's ``.claude/skills`` become listed and + # invocable with cwd elsewhere entirely, and only those — not + # CLAUDE.md, .claude/rules/*.md, .claude/settings.json (so no + # env and no hooks) or .claude/agents, which all stay with cwd. + # So it is the skills portion of a project tier rather than a + # cwd-independent way to load one. # # Do not extend this to the directory args of stdio MCP servers to # widen what those servers may read: it cannot work. A filesystem diff --git a/tests/test_config/test_settings_dir_schema.py b/tests/test_config/test_settings_dir_schema.py index b59d0541..9e6a0956 100644 --- a/tests/test_config/test_settings_dir_schema.py +++ b/tests/test_config/test_settings_dir_schema.py @@ -81,6 +81,7 @@ class TestSettingsDirRejectedOnNonProviderSteps: ({"type": "terminate", "status": "success", "reason": "done"},), ({"type": "script", "command": "echo hi"},), ({"type": "workflow", "workflow": "child.yaml"},), + ({"type": "questions", "questions": [{"id": "a", "text": "x"}]},), ( { "type": "human_gate", @@ -181,3 +182,118 @@ def test_template_referencing_an_unknown_step_is_caught(self, tmp_path: Path) -> with pytest.raises(ConfigurationError, match="nonexistent_step"): validate_workflow_config(config) + + +class TestEmptySettingsDirIsRefused: + """An empty or whitespace-only ``settings_dir`` is rejected at the schema. + + ``Path("")`` is ``Path(".")``, which is not absolute, so an empty value + would be joined onto the workflow file's own directory, pass the engine's + ``is_dir()`` check, and be forwarded as a real ``add_dirs`` entry -- the + grant is unconditional, so a value meaning "nothing" would hand the model + file access to the workflow's own tree. Rejecting at the type boundary is + what keeps the four layers (schema guards, engine, validator, provider) + agreeing on what "set" means. + """ + + @pytest.mark.parametrize("value", ["", " ", " ", "\t", "\n"]) + def test_blank_is_rejected(self, value: str) -> None: + with pytest.raises(ValidationError): + AgentDef(name="a", prompt="p", settings_dir=value) + + def test_surrounding_whitespace_is_stripped(self) -> None: + agent = AgentDef(name="a", prompt="p", settings_dir=" /repo ") + assert agent.settings_dir == "/repo" + + def test_a_blank_value_cannot_bypass_a_step_type_rejection(self) -> None: + """The step-type guards use ``is not None``, so "" cannot slip past. + + With truthiness guards and no schema constraint, ``settings_dir=""`` + was accepted on a ``wait`` step despite the documented rejection. + """ + with pytest.raises(ValidationError): + AgentDef(name="w", type="wait", duration="1s", settings_dir="") + + +class TestProjectTierWarningCauses: + """The no-skills warning must fire for every cause, with a usable remedy. + + ``settings_dir`` feeds the ``project`` tier and nothing else, so a check + for *any* tier stayed silent on ``['user']`` and ``['local']`` -- neither + of which can make a ``settings_dir``'s skills discoverable -- and on a + per-agent ``skills: []``, which zeroes the tier for that agent in + ``claude_agent_sdk.py::execute``. Each cause has a different remedy, and + one of them (a per-agent provider override) cannot be fixed by adding + ``setting_sources`` at all, so a single prescriptive message was advice + the author could not act on. + """ + + @staticmethod + def _warn(provider: object, agent_extra: dict, tmp_path: Path) -> str | None: + config = WorkflowConfig( + workflow=WorkflowDef( + name="w", + entry_point="a", + runtime=RuntimeConfig(provider=provider), # type: ignore[arg-type] + ), + agents=[ + AgentDef( + name="a", + prompt="hi", + settings_dir=str(tmp_path), + output={"r": OutputField(type="string")}, + routes=[RouteDef(to="$end")], + **agent_extra, + ) + ], + output={"r": "{{ a.output.r }}"}, + ) + hits = [w for w in validate_workflow_config(config) if "settings_dir" in w] + return hits[0] if hits else None + + def test_no_setting_sources_names_the_project_tier(self, tmp_path: Path) -> None: + warning = self._warn(ProviderSettings(name="claude-agent-sdk"), {}, tmp_path) + assert warning is not None + assert "setting_sources" in warning + + @pytest.mark.parametrize("tier", ["user", "local"]) + def test_a_non_project_tier_still_warns(self, tier: str, tmp_path: Path) -> None: + """``user`` reads ``~/.claude`` and ``local`` is cwd-bound.""" + warning = self._warn( + ProviderSettings(name="claude-agent-sdk", setting_sources=[tier]), # type: ignore[list-item] + {}, + tmp_path, + ) + assert warning is not None, f"{tier} tier cannot serve a settings_dir" + + def test_project_tier_is_silent(self, tmp_path: Path) -> None: + assert ( + self._warn( + ProviderSettings(name="claude-agent-sdk", setting_sources=["project"]), + {}, + tmp_path, + ) + is None + ) + + def test_agent_skills_opt_out_warns_and_names_skills(self, tmp_path: Path) -> None: + """``skills: []`` disables the tier for that agent, tier or not.""" + warning = self._warn( + ProviderSettings(name="claude-agent-sdk", setting_sources=["project"]), + {"skills": []}, + tmp_path, + ) + assert warning is not None + assert "skills: []" in warning + + def test_provider_override_does_not_advise_the_impossible(self, tmp_path: Path) -> None: + """``setting_sources`` is schema-rejected unless runtime.provider is the SDK. + + So telling an author with a per-agent override to add it produces a + ``ValidationError``; the warning must say the tier is workflow-scoped + instead. + """ + warning = self._warn("copilot", {"provider": "claude-agent-sdk"}, tmp_path) + assert warning is not None + assert "workflow-scoped" in warning + assert "Add 'project' to runtime.provider.setting_sources" not in warning diff --git a/tests/test_engine/test_workflow.py b/tests/test_engine/test_workflow.py index 1cc1639a..a0e17b2f 100644 --- a/tests/test_engine/test_workflow.py +++ b/tests/test_engine/test_workflow.py @@ -4514,3 +4514,30 @@ async def test_missing_settings_dir_raises_before_the_provider_call( assert "settings_dir" in str(exc_info.value) assert provider.calls == 0 + + @pytest.mark.asyncio + async def test_a_template_rendering_empty_is_refused(self, tmp_path: Path) -> None: + """An empty render must not resolve to the workflow's own directory. + + The schema rejects a literal blank, but a *template* can still render + empty at run time -- ``--input repo=``, a script step that printed + nothing, a ``set`` binding evaluating to "". ``Path("")`` is + ``Path(".")``, which is not absolute, so without this guard the value + would be joined onto the workflow file's directory, pass ``is_dir()``, + and be forwarded as a real ``add_dirs`` entry -- and the filesystem + grant is unconditional, so a value meaning "nothing" would hand the + model access to the workflow's own tree. + """ + provider = _RecordingWorkingDirProvider() + engine = WorkflowEngine( + _single_agent_config(settings_dir="{{ workflow.input.repo }}"), + provider, + workflow_path=_workflow_file(tmp_path), + ) + + with pytest.raises(ExecutionError) as exc_info: + await engine.run({"repo": ""}) + + assert "empty string" in str(exc_info.value) + assert "settings_dir" in str(exc_info.value) + assert provider.calls == 0 diff --git a/tests/test_executor/test_agent.py b/tests/test_executor/test_agent.py index f0e75b7a..f3a93035 100644 --- a/tests/test_executor/test_agent.py +++ b/tests/test_executor/test_agent.py @@ -897,6 +897,60 @@ async def close(self) -> None: return None +class TestSettingsDirCapabilityRejection: + """``capabilities.settings_dir=False`` must hold at run time too. + + ``conductor run`` never calls the static validator, and the engine + renders, absolutizes and existence-checks the directory for *every* + provider -- so an author saw the field processed and then handed to a + provider that never reads it. The agent answered from whatever + conventions its cwd supplied and the run exited 0, which is the + silent-wrong-answer case the capability exists to prevent. Mirrors + :class:`TestSessionKeyCapabilityRejection`, and the four ``_reject_*`` + helpers that exist for the same reason. + """ + + @staticmethod + def _agent(tmp_path) -> AgentDef: + return AgentDef(name="review", prompt="hi", settings_dir=str(tmp_path)) + + @staticmethod + def _copilot(calls: list[str] | None = None) -> CopilotProvider: + def mock_handler(agent, prompt, context): + if calls is not None: + calls.append(agent.name) + return {"answer": "x"} + + return CopilotProvider(mock_handler=mock_handler) + + @pytest.mark.asyncio + async def test_provider_that_cannot_apply_it_is_refused(self, tmp_path) -> None: + assert CopilotProvider.CAPABILITIES.settings_dir is False + + with pytest.raises(ExecutionError) as exc_info: + await AgentExecutor(self._copilot()).execute(self._agent(tmp_path), {}) + + assert "does not apply it" in str(exc_info.value) + assert exc_info.value.agent_name == "review" + + @pytest.mark.asyncio + async def test_the_refusal_precedes_the_provider_call(self, tmp_path) -> None: + """An answer from the wrong repository's conventions is worse than none.""" + calls: list[str] = [] + with pytest.raises(ExecutionError): + await AgentExecutor(self._copilot(calls)).execute(self._agent(tmp_path), {}) + + assert calls == [] + + @pytest.mark.asyncio + async def test_an_agent_without_settings_dir_is_untouched(self) -> None: + output = await AgentExecutor(self._copilot()).execute( + AgentDef(name="review", prompt="hi"), {} + ) + + assert output.content == {"answer": "x"} + + class TestSessionKeyCapabilityRejection: """``capabilities.session_continuity=False`` must hold at run time too. diff --git a/tests/test_integration/test_mcp_roots_negotiation.py b/tests/test_integration/test_mcp_roots_negotiation.py index 7632f6f0..e60278f5 100644 --- a/tests/test_integration/test_mcp_roots_negotiation.py +++ b/tests/test_integration/test_mcp_roots_negotiation.py @@ -38,9 +38,19 @@ _SERVER = "@modelcontextprotocol/server-filesystem" _ADOPTED = "Updated allowed directories from MCP roots" -pytestmark = pytest.mark.skipif( - shutil.which("npx") is None, reason="npx not available; needs the real filesystem MCP server" -) +# ``real_api`` because this fetches @modelcontextprotocol/server-filesystem from +# npm: it pins *upstream's* negotiation behaviour rather than Conductor's own +# code, so an npm outage or a new server release must not redden an unrelated +# PR. The repo's convention for a test reaching an external service is an +# opt-in marker (cf. ``real_api`` / ``install_scripts`` / ``performance`` in +# pyproject.toml); CI runs ``-m "not real_api and not performance"``. +pytestmark = [ + pytest.mark.real_api, + pytest.mark.skipif( + shutil.which("npx") is None, + reason="npx not available; needs the real filesystem MCP server", + ), +] def _list_allowed_directories( @@ -52,9 +62,17 @@ def _list_allowed_directories( asked and keeps its argv directories. A list declares the capability and is what the server receives when it asks. """ - with tempfile.NamedTemporaryFile(mode="w+", suffix=".err") as errf: + # ``shutil.which`` finds ``npx.cmd`` on Windows but ``CreateProcess`` only + # appends ``.exe``, so a bare "npx" would fail to launch there while the + # skipif above says it is present. Pass the resolved path. + npx = shutil.which("npx") + assert npx is not None # guarded by the module-level skipif + # A plain file rather than NamedTemporaryFile: the stderr poll below reopens + # it by name, which is unsupported while the handle is open on Windows. + err_path = Path(tempfile.mkdtemp(prefix="mcp-roots-")) / "server.err" + with err_path.open("w") as errf: proc = subprocess.Popen( # noqa: S603 - ["npx", "-y", _SERVER, *root_dirs], # noqa: S607 + [npx, "-y", _SERVER, *root_dirs], stdin=subprocess.PIPE, stdout=subprocess.PIPE, stderr=errf, @@ -80,7 +98,7 @@ def call_tool() -> None: def server_stderr() -> str: errf.flush() - return Path(errf.name).read_text(errors="replace") + return err_path.read_text(errors="replace") try: send( diff --git a/tests/test_providers/test_claude_agent_sdk.py b/tests/test_providers/test_claude_agent_sdk.py index d062c902..cdba4a33 100644 --- a/tests/test_providers/test_claude_agent_sdk.py +++ b/tests/test_providers/test_claude_agent_sdk.py @@ -3292,3 +3292,50 @@ async def fake_query(**kwargs): ) assert captured["add_dirs"] == [str(link)] + + @pytest.mark.asyncio + async def test_settings_dir_reaches_the_cli_as_add_dir(self, tmp_path: Path) -> None: + """The SDK still turns ``add_dirs`` into the ``--add-dir`` argv flag. + + Asserting ``options.add_dirs`` alone proves only that Conductor set the + field. ``settings_dir`` has no fallback delivery path -- there is no + prompt-injection equivalent that could carry a settings tier -- and the + pin is ``claude-agent-sdk>=0.2.82``, a floor with no ceiling, so a lock + bump that renamed or dropped the flag would leave every other test in + this class green with the feature silently dead. + """ + from claude_agent_sdk._internal.transport.subprocess_cli import ( + SubprocessCLITransport, + ) + + target = tmp_path / "target" + target.mkdir() + + async def options_for(settings_dir: str | None): + captured: dict = {} + + async def fake_query(**kwargs): + captured["options"] = kwargs["options"] + 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", settings_dir=settings_dir), + context={}, + rendered_prompt="hi", + ) + return captured["options"] + + def argv(options) -> list[str]: + transport = SubprocessCLITransport(prompt="hi", options=options) + transport._cli_path = "/usr/bin/claude" + return transport._build_command() + + with_dir = argv(await options_for(str(target))) + assert "--add-dir" in with_dir, with_dir + assert with_dir[with_dir.index("--add-dir") + 1] == str(target) + + # Negative control: without a settings_dir the flag is absent entirely, + # so the assertion above cannot pass against an always-emitted flag. + assert "--add-dir" not in argv(await options_for(None)) From 21cf5711b0a1f82996d1752e81ed02117fc48b37 Mon Sep 17 00:00:00 2001 From: Chris Throup Date: Tue, 8 Sep 2026 21:41:27 +0200 Subject: [PATCH 03/10] feat(claude-agent-sdk): report settings_dir in the agent lifecycle events `settings_dir` is a trust decision -- it loads another repository's conventions and, unconditionally, widens the model's built-in Read/Edit/Bash to that tree -- and nothing in the run output mentioned it. Neither the dashboard nor the JSONL event log recorded which directory an agent had been granted, so the grant could not be audited after the fact. Emitted alongside `working_dir` at all three sites that already carry it: `agent_started`, `parallel_agent_started` and `for_each_agent_started`. Emitting on one path only would have left the other two silent about the same grant. Always present (null when unset) so a consumer can distinguish "no grant" from "this Conductor did not report one". --- AGENTS.md | 2 +- src/conductor/engine/workflow.py | 11 ++++++- tests/test_engine/test_workflow.py | 49 ++++++++++++++++++++++++++++++ 3 files changed, 60 insertions(+), 2 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index be2d36aa..c8fc3adc 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -432,7 +432,7 @@ Conductor: - The path is passed **verbatim** — `WorkflowEngine._resolve_agent_working_dir` has already rendered, absolutized, normalised, and existence-checked it, and re-resolving here would collapse the symlink aliases the engine preserves on purpose. The `ClaudeAgentOptions(...)` construction lives **inside** `execute`'s `try` so the `os.getcwd()` fallback can't escape as a bare `OSError` when the process cwd has been deleted (`copilot.py` resolves its cwd inside its try for the same reason). - There is no provider-side `is_dir()` guard: a directory that vanishes after the engine's check surfaces as the SDK's `CLIConnectionError("Working directory does not exist: ")`, wrapped in `ProviderError`. That is only defensible because `_classify_startup_failure` special-cases it — `CLIConnectionError` otherwise yields firewall/binary advice and `is_retryable=True`, which is wrong for all three launch failures (missing dir; path is a file → `ENOTDIR`; unreadable dir → `EACCES`; the latter two reach the SDK's generic "Failed to start Claude Code" arm, not its dedicated one). - Knock-on effects: cwd is the project key for the CLI's on-disk transcript directory, and it is where a project `.mcp.json` and `.claude/` tree would be looked for. The unconditional `strict_mcp_config=True` stops a `.mcp.json` there from injecting undeclared servers, and `setting_sources=[]` (empty by default; see **Skills** below) stops the CLI loading `CLAUDE.md`, project settings, and hooks from it — so cwd no longer drags ambient instructions in unless the workflow opts in via `runtime.provider.setting_sources`, which is exactly a request to load them from that directory. `add_dirs` (the SDK's `--add-dir` passthrough) is a separate axis, carrying the per-agent `settings_dir` and nothing else — see **Target-repository skills** below. -- **Target-repository skills** (`settings_dir`): the per-agent `AgentDef.settings_dir` is the **only** source of `ClaudeAgentOptions.add_dirs`, and it selects which directory's *project* settings tier contributes skills. It exists because cwd was doing two conflicting jobs on this provider. The CLI supports the MCP Roots protocol and advertises exactly one root — its cwd — so `@modelcontextprotocol/server-filesystem` **discards the directories in its own argv** and permits cwd alone; cwd is simultaneously what the `project` tier resolves against. Narrowing cwd onto a target repository to reach its skills therefore narrowed the agent's only MCP root below any sibling path the step still had to read. Do not derive `add_dirs` from a server's directory arguments to compensate: `--add-dir` takes no part in Roots negotiation, it widens the CLI's own file tools, and doing so would also silently widen skill discovery to directories the author named as data. What `add_dirs` *does* do, measured: a named directory's `.claude/skills` become listed and invocable with cwd elsewhere entirely — and only those, not `CLAUDE.md`, `.claude/rules/*.md`, `.claude/settings.json` (so no `env` and no `hooks` — the hooks negative is measured with a side-effect probe whose control fires) or `.claude/agents`, which all keep following cwd. That makes it the *skills third* of what a cwd-resolved `project` tier loads, not a replacement for it. It carries a second, **unconditional** effect the skills framing hides: `add_dirs` is "additional directories Claude can access" per the SDK's own contract, so a `settings_dir` widens the model's built-in `Read`/`Edit`/`Bash` to that tree with no settings tier enabled at all (measured at `permission_mode: "default"` with `setting_sources` unset: a read outside cwd is refused without it and succeeds with it; an agent omitting `tools:` runs under `bypassPermissions`, where the grant is unobservable because reads already succeed). It does not widen what an MCP server permits. `capabilities.py::settings_dir` gates the field and `config/validator.py` errors against a provider that cannot apply it, warning when it is set with no `setting_sources` (the skills half is then a no-op while the filesystem grant still applies). `WorkflowEngine._resolve_agent_directory` resolves both fields so they cannot drift, and `settings_dir` is per-agent only (no `runtime.` counterpart: the repository whose conventions apply is what varies between steps). `tests/test_integration/test_mcp_roots_negotiation.py` pins the negotiation rule itself against the real server with no LLM — two runs differing only in whether the client advertises `roots` — since every option here rests on it. +- **Target-repository skills** (`settings_dir`): the per-agent `AgentDef.settings_dir` is the **only** source of `ClaudeAgentOptions.add_dirs`, and it selects which directory's *project* settings tier contributes skills. It exists because cwd was doing two conflicting jobs on this provider. The CLI supports the MCP Roots protocol and advertises exactly one root — its cwd — so `@modelcontextprotocol/server-filesystem` **discards the directories in its own argv** and permits cwd alone; cwd is simultaneously what the `project` tier resolves against. Narrowing cwd onto a target repository to reach its skills therefore narrowed the agent's only MCP root below any sibling path the step still had to read. Do not derive `add_dirs` from a server's directory arguments to compensate: `--add-dir` takes no part in Roots negotiation, it widens the CLI's own file tools, and doing so would also silently widen skill discovery to directories the author named as data. What `add_dirs` *does* do, measured: a named directory's `.claude/skills` become listed and invocable with cwd elsewhere entirely — and only those, not `CLAUDE.md`, `.claude/rules/*.md`, `.claude/settings.json` (so no `env` and no `hooks` — the hooks negative is measured with a side-effect probe whose control fires) or `.claude/agents`, which all keep following cwd. That makes it the *skills third* of what a cwd-resolved `project` tier loads, not a replacement for it. It carries a second, **unconditional** effect the skills framing hides: `add_dirs` is "additional directories Claude can access" per the SDK's own contract, so a `settings_dir` widens the model's built-in `Read`/`Edit`/`Bash` to that tree with no settings tier enabled at all (measured at `permission_mode: "default"` with `setting_sources` unset: a read outside cwd is refused without it and succeeds with it; an agent omitting `tools:` runs under `bypassPermissions`, where the grant is unobservable because reads already succeed). It does not widen what an MCP server permits. `capabilities.py::settings_dir` gates the field and `config/validator.py` errors against a provider that cannot apply it, warning when it is set with no `setting_sources` (the skills half is then a no-op while the filesystem grant still applies). `WorkflowEngine._resolve_agent_directory` resolves both fields so they cannot drift, and `settings_dir` is per-agent only (no `runtime.` counterpart: the repository whose conventions apply is what varies between steps). `tests/test_integration/test_mcp_roots_negotiation.py` pins the negotiation rule itself against the real server with no LLM — two runs differing only in whether the client advertises `roots` — since every option here rests on it (marked `real_api`: it fetches the server from npm and pins *upstream's* behaviour, not Conductor's). The resolved value is emitted on `agent_started` / `parallel_agent_started` / `for_each_agent_started` alongside `working_dir`, because it is a trust decision — another repository's conventions plus a filesystem grant — and one the run output would otherwise never mention. - **Skills** (issue #352): `supports_native_skills=True`. Skills are enabled through the SDK, not prompt injection, and three options move together in `execute`: - `plugins=[{"type": "local", "path": }]` + `skills=[":"]`. The SDK has no skill-*directory* surface, so `_resolve_skill_plugins` maps each directory back to the plugin that owns it via `skills/registry.py::resolve_skill_plugin`. That resolver is deliberately strict, because every one of these mistakes otherwise produces a name the CLI silently resolves to nothing: it bounds the upward walk (`_PLUGIN_SEARCH_DEPTH`), requires the skill to actually live under the candidate's `skills/` directory, requires `SKILL.md` to exist and its frontmatter `name` to equal the directory name (the CLI resolves by frontmatter name; Conductor sends the directory name), and rejects names outside `[A-Za-z0-9_.-]+` since they are joined into a comma-delimited `--allowedTools` value. A skill under no plugin root returns `None`; a plugin that is present but unusable raises `SkillPluginError`, which the provider re-raises as a `ProviderError` carrying the real reason rather than a blanket "not part of a plugin". Two plugins claiming one qualified name are refused too — deduping the clash away would drop a declared skill. All of it is `is_retryable=False`: these never become valid on a retry, and a checkout path containing "connection" would otherwise trip the retryability heuristic. Note providers are constructed lazily, so this surfaces when the first agent on this provider runs, **not** at `conductor validate`. - `setting_sources=[]` **by default**, for the same reason `strict_mcp_config=True` is unconditional a few lines away — but opt-in per workflow via `runtime.provider.setting_sources` (`user`/`project`/`local`, issue #501). Left unset, the CLI loads user, project, and local settings, which between them bring in ambient skills, `CLAUDE.md`, and hooks the workflow never declared, varying by machine and launch directory. Conductor surfaces instruction files through its own opt-in `--workspace-instructions`; settings and hooks have no equivalent. `skills=[]` and `skills=None` are **not** interchangeable upstream: `None` means "CLI defaults apply", and setting `skills` while leaving `setting_sources` at `None` makes the SDK re-default it to `["user", "project"]` — so the two options are coupled and dropping either re-opens the issue. The `[]` is also invisible in argv (it travels in the SDK's `initialize` control request), which is why the argv-based tests are paired with options-level assertions. The list is a context filter, not a sandbox: undeclared skills are hidden from the model's listing and rejected by the `Skill` tool, but their files stay readable. Opting a workflow in (`runtime.provider.setting_sources: [project]`, the motivating case being an agent whose `working_dir` is a *target* repo shipping its own `.claude/skills` — the CLI has `--plugin-dir` but no `--skill-dir`) loads that tier **with its hooks**, so it is only for repositories trusted as much as the workflow itself, and the field is workflow-global while `working_dir` is per agent. Two couplings follow: `_resolve_skill_filter` resolves `skills` to `"all"` when a tier is enabled and the workflow named none itself (tier-discovered skills never pass through `skill_names`, so `[]` would load the repo's skills and then hide every one), and a per-agent `skills: []` opts that agent out of the tier entirely — hooks included — keeping it the one opt-out. diff --git a/src/conductor/engine/workflow.py b/src/conductor/engine/workflow.py index 55426ee9..57e778cb 100644 --- a/src/conductor/engine/workflow.py +++ b/src/conductor/engine/workflow.py @@ -622,7 +622,7 @@ def _resolve_agent_directory( # is not absolute, so it would join onto the workflow file's own directory # and pass the is_dir() check below. For settings_dir that silently grants # the model file access to the workflow's own tree, so refuse it here. - if not rendered.strip(): + if not rendered.strip() and field == "settings_dir": raise ExecutionError( f"Agent '{agent.name}': {field} rendered to an empty string from '{raw}'", agent_name=agent.name, @@ -4515,6 +4515,13 @@ async def _execute_loop(self, current_agent_name: str) -> dict[str, Any]: } if is_llm_agent: started_payload["working_dir"] = resolved_agent.working_dir + # Emitted alongside working_dir because it is a + # trust decision: settings_dir loads another + # repository's conventions AND widens the model's + # built-in file tools to that tree. A grant the + # dashboard and the JSONL log never mention cannot + # be audited after the fact. + started_payload["settings_dir"] = resolved_agent.settings_dir self._emit("agent_started", started_payload) # Handle terminate steps — explicit workflow exit with a @@ -6228,6 +6235,7 @@ async def execute_single_agent(agent: AgentDef) -> tuple[str, Any]: "group_name": parallel_group.name, "agent_name": agent.name, "working_dir": resolved_agent.working_dir, + "settings_dir": resolved_agent.settings_dir, }, ) @@ -6716,6 +6724,7 @@ async def execute_single_item(item: Any, index: int, key: str) -> tuple[str, Any "agent_name": qualified_agent.name, "item_key": key, "working_dir": qualified_agent.working_dir, + "settings_dir": qualified_agent.settings_dir, }, ) diff --git a/tests/test_engine/test_workflow.py b/tests/test_engine/test_workflow.py index a0e17b2f..a3d19dfa 100644 --- a/tests/test_engine/test_workflow.py +++ b/tests/test_engine/test_workflow.py @@ -4541,3 +4541,52 @@ async def test_a_template_rendering_empty_is_refused(self, tmp_path: Path) -> No assert "empty string" in str(exc_info.value) assert "settings_dir" in str(exc_info.value) assert provider.calls == 0 + + +class TestSettingsDirObservability: + """``settings_dir`` must appear in the events, on all three agent paths. + + It is a trust decision, not a convenience: naming a directory loads + another repository's conventions *and* widens the model's built-in + ``Read``/``Edit``/``Bash`` to that tree. A grant that the dashboard and the + JSONL event log never mention cannot be audited after a run, so it rides + alongside ``working_dir`` wherever that is already emitted -- linear, + parallel-group and for-each. Pinning all three is the point: emitting it on + one path only would leave the other two silent about the same grant. + """ + + @pytest.mark.asyncio + async def test_agent_started_carries_settings_dir(self, tmp_path: Path) -> None: + target = tmp_path / "repo" + target.mkdir() + events: list[tuple[str, dict]] = [] + engine = WorkflowEngine( + _single_agent_config(settings_dir=str(target)), + _RecordingWorkingDirProvider(), + workflow_path=_workflow_file(tmp_path), + ) + engine._emit = lambda t, d=None: events.append((t, d or {})) # type: ignore[method-assign] + + await engine.run({}) + + started = [d for t, d in events if t == "agent_started"] + assert started, [t for t, _ in events] + assert started[0]["settings_dir"] == os.path.normpath(str(target)) + + @pytest.mark.asyncio + async def test_agent_started_reports_none_when_unset(self, tmp_path: Path) -> None: + """Absent rather than missing: a consumer can tell "no grant" from + "this Conductor did not report one".""" + events: list[tuple[str, dict]] = [] + engine = WorkflowEngine( + _single_agent_config(), + _RecordingWorkingDirProvider(), + workflow_path=_workflow_file(tmp_path), + ) + engine._emit = lambda t, d=None: events.append((t, d or {})) # type: ignore[method-assign] + + await engine.run({}) + + started = [d for t, d in events if t == "agent_started"] + assert started and "settings_dir" in started[0] + assert started[0]["settings_dir"] is None From 65e19972a6e91787a212eec9e8d39d7b4811ad70 Mon Sep 17 00:00:00 2001 From: Chris Throup Date: Tue, 8 Sep 2026 22:05:40 +0200 Subject: [PATCH 04/10] fix(claude-agent-sdk): restore the empty-render guard and close review gaps The empty-render guard added in 0b1598a had been narrowed to `field == "settings_dir"` in 9b15ba7 -- a mutation-test edit that leaked into that commit rather than being reverted. Restored to unconditional, and the `working_dir` half is now pinned by its own test, which is what would have caught the leak. Also from a further review pass: - The `working_dir` behaviour change is now declared rather than silent. An empty-rendering `working_dir` previously resolved to the workflow file's own directory and ran there; it is now an error. Recorded in CHANGELOG.md under Changed, in the shared helper's docstring, and tested in its own right so a later edit narrowing the guard back to `settings_dir` cannot silently restore the old behaviour. - CHANGELOG.md had no entry for this feature at all. Added under Unreleased/Added, alongside the Changed note above. - The validator sub-agent's comment justified `settings_dir=None` with "no skill can be invoked without the Skill tool", which is false: `_resolve_tool_config` grants `Skill` back for `tools: []`, so with a settings tier enabled the grader does hold it (measured: `(['Skill'], None)`). The conclusion stands on the other half -- no file tool for the grant to widen -- so the reason is corrected rather than the code. Pinned by a test, since the consequence is latent today and becomes load-bearing the next time a tool is granted back. - AGENTS.md still described `settings_dir` as enforced only in config/validator.py. It is enforced twice; the file now says so, in the same words the skills and plugins bullets use. - The schema docstring stated the filesystem grant as a live exposure. Conductor reaches only `bypassPermissions` (full preset, where reads already succeed everywhere) or `tools: []` (at most the `Skill` loader, no file tool), so the grant is a property of the SDK contract to design against rather than something reachable from a workflow today. The docs page already carried that hedge; the docstring, which is what surfaces on hover, did not. - `settings_dir` had no coverage inside parallel groups or for-each loops. Both are now tested, the for-each one asserting per-iteration resolution -- that site resolves after loop-variable injection, so a refactor could otherwise load a different iteration's skills unnoticed. --- AGENTS.md | 2 +- CHANGELOG.md | 28 +++++ src/conductor/config/schema.py | 10 +- src/conductor/engine/validator.py | 17 ++- src/conductor/engine/workflow.py | 12 +- tests/test_engine/test_validator.py | 28 +++++ tests/test_engine/test_workflow.py | 168 ++++++++++++++++++++++++++++ 7 files changed, 254 insertions(+), 11 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index c8fc3adc..6a328e7a 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -432,7 +432,7 @@ Conductor: - The path is passed **verbatim** — `WorkflowEngine._resolve_agent_working_dir` has already rendered, absolutized, normalised, and existence-checked it, and re-resolving here would collapse the symlink aliases the engine preserves on purpose. The `ClaudeAgentOptions(...)` construction lives **inside** `execute`'s `try` so the `os.getcwd()` fallback can't escape as a bare `OSError` when the process cwd has been deleted (`copilot.py` resolves its cwd inside its try for the same reason). - There is no provider-side `is_dir()` guard: a directory that vanishes after the engine's check surfaces as the SDK's `CLIConnectionError("Working directory does not exist: ")`, wrapped in `ProviderError`. That is only defensible because `_classify_startup_failure` special-cases it — `CLIConnectionError` otherwise yields firewall/binary advice and `is_retryable=True`, which is wrong for all three launch failures (missing dir; path is a file → `ENOTDIR`; unreadable dir → `EACCES`; the latter two reach the SDK's generic "Failed to start Claude Code" arm, not its dedicated one). - Knock-on effects: cwd is the project key for the CLI's on-disk transcript directory, and it is where a project `.mcp.json` and `.claude/` tree would be looked for. The unconditional `strict_mcp_config=True` stops a `.mcp.json` there from injecting undeclared servers, and `setting_sources=[]` (empty by default; see **Skills** below) stops the CLI loading `CLAUDE.md`, project settings, and hooks from it — so cwd no longer drags ambient instructions in unless the workflow opts in via `runtime.provider.setting_sources`, which is exactly a request to load them from that directory. `add_dirs` (the SDK's `--add-dir` passthrough) is a separate axis, carrying the per-agent `settings_dir` and nothing else — see **Target-repository skills** below. -- **Target-repository skills** (`settings_dir`): the per-agent `AgentDef.settings_dir` is the **only** source of `ClaudeAgentOptions.add_dirs`, and it selects which directory's *project* settings tier contributes skills. It exists because cwd was doing two conflicting jobs on this provider. The CLI supports the MCP Roots protocol and advertises exactly one root — its cwd — so `@modelcontextprotocol/server-filesystem` **discards the directories in its own argv** and permits cwd alone; cwd is simultaneously what the `project` tier resolves against. Narrowing cwd onto a target repository to reach its skills therefore narrowed the agent's only MCP root below any sibling path the step still had to read. Do not derive `add_dirs` from a server's directory arguments to compensate: `--add-dir` takes no part in Roots negotiation, it widens the CLI's own file tools, and doing so would also silently widen skill discovery to directories the author named as data. What `add_dirs` *does* do, measured: a named directory's `.claude/skills` become listed and invocable with cwd elsewhere entirely — and only those, not `CLAUDE.md`, `.claude/rules/*.md`, `.claude/settings.json` (so no `env` and no `hooks` — the hooks negative is measured with a side-effect probe whose control fires) or `.claude/agents`, which all keep following cwd. That makes it the *skills third* of what a cwd-resolved `project` tier loads, not a replacement for it. It carries a second, **unconditional** effect the skills framing hides: `add_dirs` is "additional directories Claude can access" per the SDK's own contract, so a `settings_dir` widens the model's built-in `Read`/`Edit`/`Bash` to that tree with no settings tier enabled at all (measured at `permission_mode: "default"` with `setting_sources` unset: a read outside cwd is refused without it and succeeds with it; an agent omitting `tools:` runs under `bypassPermissions`, where the grant is unobservable because reads already succeed). It does not widen what an MCP server permits. `capabilities.py::settings_dir` gates the field and `config/validator.py` errors against a provider that cannot apply it, warning when it is set with no `setting_sources` (the skills half is then a no-op while the filesystem grant still applies). `WorkflowEngine._resolve_agent_directory` resolves both fields so they cannot drift, and `settings_dir` is per-agent only (no `runtime.` counterpart: the repository whose conventions apply is what varies between steps). `tests/test_integration/test_mcp_roots_negotiation.py` pins the negotiation rule itself against the real server with no LLM — two runs differing only in whether the client advertises `roots` — since every option here rests on it (marked `real_api`: it fetches the server from npm and pins *upstream's* behaviour, not Conductor's). The resolved value is emitted on `agent_started` / `parallel_agent_started` / `for_each_agent_started` alongside `working_dir`, because it is a trust decision — another repository's conventions plus a filesystem grant — and one the run output would otherwise never mention. +- **Target-repository skills** (`settings_dir`): the per-agent `AgentDef.settings_dir` is the **only** source of `ClaudeAgentOptions.add_dirs`, and it selects which directory's *project* settings tier contributes skills. It exists because cwd was doing two conflicting jobs on this provider. The CLI supports the MCP Roots protocol and advertises exactly one root — its cwd — so `@modelcontextprotocol/server-filesystem` **discards the directories in its own argv** and permits cwd alone; cwd is simultaneously what the `project` tier resolves against. Narrowing cwd onto a target repository to reach its skills therefore narrowed the agent's only MCP root below any sibling path the step still had to read. Do not derive `add_dirs` from a server's directory arguments to compensate: `--add-dir` takes no part in Roots negotiation, it widens the CLI's own file tools, and doing so would also silently widen skill discovery to directories the author named as data. What `add_dirs` *does* do, measured: a named directory's `.claude/skills` become listed and invocable with cwd elsewhere entirely — and only those, not `CLAUDE.md`, `.claude/rules/*.md`, `.claude/settings.json` (so no `env` and no `hooks` — the hooks negative is measured with a side-effect probe whose control fires) or `.claude/agents`, which all keep following cwd. That makes it the *skills third* of what a cwd-resolved `project` tier loads, not a replacement for it. It carries a second, **unconditional** effect the skills framing hides: `add_dirs` is "additional directories Claude can access" per the SDK's own contract, so a `settings_dir` widens the model's built-in `Read`/`Edit`/`Bash` to that tree with no settings tier enabled at all (measured at `permission_mode: "default"` with `setting_sources` unset: a read outside cwd is refused without it and succeeds with it; an agent omitting `tools:` runs under `bypassPermissions`, where the grant is unobservable because reads already succeed). It does not widen what an MCP server permits. `capabilities.py::settings_dir` gates the field and `config/validator.py` errors against a provider that cannot apply it — enforced **twice**, with `AgentExecutor._reject_unsupported_settings_dir` repeating it at run time, because `conductor run` never calls the static validator — warning when it is set with no `setting_sources` (the skills half is then a no-op while the filesystem grant still applies). `WorkflowEngine._resolve_agent_directory` resolves both fields so they cannot drift, and `settings_dir` is per-agent only (no `runtime.` counterpart: the repository whose conventions apply is what varies between steps). `tests/test_integration/test_mcp_roots_negotiation.py` pins the negotiation rule itself against the real server with no LLM — two runs differing only in whether the client advertises `roots` — since every option here rests on it (marked `real_api`: it fetches the server from npm and pins *upstream's* behaviour, not Conductor's). The resolved value is emitted on `agent_started` / `parallel_agent_started` / `for_each_agent_started` alongside `working_dir`, because it is a trust decision — another repository's conventions plus a filesystem grant — and one the run output would otherwise never mention. - **Skills** (issue #352): `supports_native_skills=True`. Skills are enabled through the SDK, not prompt injection, and three options move together in `execute`: - `plugins=[{"type": "local", "path": }]` + `skills=[":"]`. The SDK has no skill-*directory* surface, so `_resolve_skill_plugins` maps each directory back to the plugin that owns it via `skills/registry.py::resolve_skill_plugin`. That resolver is deliberately strict, because every one of these mistakes otherwise produces a name the CLI silently resolves to nothing: it bounds the upward walk (`_PLUGIN_SEARCH_DEPTH`), requires the skill to actually live under the candidate's `skills/` directory, requires `SKILL.md` to exist and its frontmatter `name` to equal the directory name (the CLI resolves by frontmatter name; Conductor sends the directory name), and rejects names outside `[A-Za-z0-9_.-]+` since they are joined into a comma-delimited `--allowedTools` value. A skill under no plugin root returns `None`; a plugin that is present but unusable raises `SkillPluginError`, which the provider re-raises as a `ProviderError` carrying the real reason rather than a blanket "not part of a plugin". Two plugins claiming one qualified name are refused too — deduping the clash away would drop a declared skill. All of it is `is_retryable=False`: these never become valid on a retry, and a checkout path containing "connection" would otherwise trip the retryability heuristic. Note providers are constructed lazily, so this surfaces when the first agent on this provider runs, **not** at `conductor validate`. - `setting_sources=[]` **by default**, for the same reason `strict_mcp_config=True` is unconditional a few lines away — but opt-in per workflow via `runtime.provider.setting_sources` (`user`/`project`/`local`, issue #501). Left unset, the CLI loads user, project, and local settings, which between them bring in ambient skills, `CLAUDE.md`, and hooks the workflow never declared, varying by machine and launch directory. Conductor surfaces instruction files through its own opt-in `--workspace-instructions`; settings and hooks have no equivalent. `skills=[]` and `skills=None` are **not** interchangeable upstream: `None` means "CLI defaults apply", and setting `skills` while leaving `setting_sources` at `None` makes the SDK re-default it to `["user", "project"]` — so the two options are coupled and dropping either re-opens the issue. The `[]` is also invisible in argv (it travels in the SDK's `initialize` control request), which is why the argv-based tests are paired with options-level assertions. The list is a context filter, not a sandbox: undeclared skills are hidden from the model's listing and rejected by the `Skill` tool, but their files stay readable. Opting a workflow in (`runtime.provider.setting_sources: [project]`, the motivating case being an agent whose `working_dir` is a *target* repo shipping its own `.claude/skills` — the CLI has `--plugin-dir` but no `--skill-dir`) loads that tier **with its hooks**, so it is only for repositories trusted as much as the workflow itself, and the field is workflow-global while `working_dir` is per agent. Two couplings follow: `_resolve_skill_filter` resolves `skills` to `"all"` when a tier is enabled and the workflow named none itself (tier-discovered skills never pass through `skill_names`, so `[]` would load the repo's skills and then hide every one), and a per-agent `skills: []` opts that agent out of the tier entirely — hooks included — keeping it the one opt-out. diff --git a/CHANGELOG.md b/CHANGELOG.md index 001881ff..17281428 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -47,6 +47,23 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 entirely. See [`examples/claude-agent-sdk-setting-sources.yaml`](examples/claude-agent-sdk-setting-sources.yaml). +- **Per-agent `settings_dir` on `claude-agent-sdk`** (#513) — selects which + directory's `project` settings tier supplies an agent's **skills**, + independently of `working_dir`. The CLI advertises exactly one MCP root — + its cwd — and a filesystem MCP server that sees a Roots-capable client + discards the directories in its own argv, so pointing `working_dir` at a + target repository to pick up its skills also narrowed the agent's only MCP + root onto it. `settings_dir` splits the two, letting cwd stay wide enough + for every path the agent must read. It carries a second, unconditional + effect: `add_dirs` widens the model's built-in `Read`/`Edit`/`Bash` to that + tree regardless of any settings tier, though no Conductor configuration + reaches a permission mode where that is observable today. Only the skills + of that directory travel — not `CLAUDE.md`, `.claude/rules/*.md`, + `.claude/settings.json` or `.claude/agents`, all measured. Refused at + `conductor validate` *and* at run time on a provider that cannot apply it, + and reported on the agent lifecycle events so the grant is auditable. See + [`examples/claude-agent-sdk-settings-dir.yaml`](examples/claude-agent-sdk-settings-dir.yaml). + ### Fixed - **Context compaction window guard against token-dense drift** (#507) — the @@ -76,6 +93,17 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 (`reason: "estimate_unavailable"`) rather than vanishing into stderr. See [Workflow Syntax → Context Compaction](docs/workflow-syntax.md#context-compaction). +### Changed + +- **A `working_dir` or `settings_dir` template that renders empty is now an + error** (#513). Previously an empty render resolved to the workflow file's + own directory — `Path("")` is `Path(".")`, which is not absolute, so it was + joined onto that directory and passed the existence check — and the agent + ran there. A value meaning "nothing" silently becoming something real is + the defect; for `settings_dir` it would also have granted the model access + to the workflow's own tree. Both fields now fail before the provider call, + naming the field and the template it came from. + ## [0.1.36](https://github.com/microsoft/conductor/compare/v0.1.35...v0.1.36) - 2026-09-02 ### Added diff --git a/src/conductor/config/schema.py b/src/conductor/config/schema.py index 95ca90b5..71ea1a7f 100644 --- a/src/conductor/config/schema.py +++ b/src/conductor/config/schema.py @@ -1404,7 +1404,15 @@ class AgentDef(BaseModel): directory here widens the model's built-in ``Read``/``Edit``/``Bash`` tools to that tree regardless of any settings tier. It does **not** widen what a filesystem MCP server permits -- that stays cwd alone, which is why - this field exists. Point it only at a directory the agent may read. + this field exists. + + That grant is currently latent rather than reachable from a workflow: + Conductor runs this provider either with the full ``claude_code`` preset + under ``bypassPermissions`` (``tools:`` omitted), where reads already + succeed everywhere, or with ``tools: []``, where the model holds at most + the ``Skill`` loader and no file tool at all. So it is a property of the + SDK contract to design against rather than an exposure today. Point it + only at a directory the agent may read. It exists because ``working_dir`` was doing two unrelated jobs. The CLI supports MCP Roots and advertises exactly one root — its cwd — so a diff --git a/src/conductor/engine/validator.py b/src/conductor/engine/validator.py index 02834e82..0d441a4d 100644 --- a/src/conductor/engine/validator.py +++ b/src/conductor/engine/validator.py @@ -227,12 +227,17 @@ def _build_validator_agent(self, agent: AgentDef) -> AgentDef: tools=[], output=_VALIDATOR_OUTPUT_SCHEMA, working_dir=agent.working_dir, - # Deliberately NOT inherited, unlike working_dir. This grader runs - # with ``tools=[]``, so neither half of settings_dir would do - # anything for it: no skill can be invoked without the Skill tool, - # and the filesystem grant has no file tool to widen. Inheriting it - # would hand the grader access to a tree it cannot use, which is a - # wider grant than the run needs. + # Deliberately NOT inherited, unlike working_dir. This grader + # runs with ``tools=[]``, which yields at most the ``Skill`` + # loader and never Read/Edit/Bash -- so the filesystem half of + # settings_dir has no file tool to widen, and grading an output + # against a rubric needs no skills. Inheriting it would hand the + # grader a tree it has no way to use: a wider grant than the run + # needs. + # + # (Not "no skill can be invoked": with a settings tier enabled the + # grader does hold the ``Skill`` tool, since + # ``_resolve_tool_config`` grants it back for ``tools: []``.) # # Built field by field rather than by ``model_copy`` for the same # reason: a copy would also carry ``validator`` (making the grader diff --git a/src/conductor/engine/workflow.py b/src/conductor/engine/workflow.py index 57e778cb..d4a13376 100644 --- a/src/conductor/engine/workflow.py +++ b/src/conductor/engine/workflow.py @@ -611,9 +611,15 @@ def _resolve_agent_directory( normalised with :func:`os.path.normpath` (``resolve()`` is deliberately not used so symlink aliases stay distinct). + Applies to ``working_dir`` as well as ``settings_dir``: a value that + renders empty previously resolved to the workflow file's own directory + and ran there, which is the same footgun in a quieter form, so both + are refused. + Raises: - ExecutionError: if the resolved path is not an existing directory - — before any provider call. + ExecutionError: if the value renders empty, or if the resolved + path is not an existing directory — both before any provider + call. """ rendered = self.renderer.render(raw, agent_context) # A template can render empty even when the raw value passed the schema's @@ -622,7 +628,7 @@ def _resolve_agent_directory( # is not absolute, so it would join onto the workflow file's own directory # and pass the is_dir() check below. For settings_dir that silently grants # the model file access to the workflow's own tree, so refuse it here. - if not rendered.strip() and field == "settings_dir": + if not rendered.strip(): raise ExecutionError( f"Agent '{agent.name}': {field} rendered to an empty string from '{raw}'", agent_name=agent.name, diff --git a/tests/test_engine/test_validator.py b/tests/test_engine/test_validator.py index 3375faa1..857fca8b 100644 --- a/tests/test_engine/test_validator.py +++ b/tests/test_engine/test_validator.py @@ -138,6 +138,34 @@ def test_system_prompt_contains_criteria(self) -> None: v = OutputValidator()._build_validator_agent(agent) assert "VERY_SPECIFIC_RUBRIC" in (v.system_prompt or "") + def test_settings_dir_is_not_inherited(self) -> None: + """The grader must not inherit the primary agent's ``settings_dir``. + + ``working_dir`` IS inherited, so this is a deliberate asymmetry rather + than an omission: the grader runs with ``tools=[]``, which yields at + most the ``Skill`` loader and never Read/Edit/Bash, so the filesystem + grant would widen access to a tree it has no way to use. + + Pinned because the consequence is latent, not active: today there is + no file tool for the grant to widen, but ``_resolve_tool_config``'s + carve-outs have already grown once (the ``Skill`` grant-back). The + next time a tool is granted back to a ``tools: []`` agent this line + becomes load-bearing, and without this assertion nothing would notice + if it had drifted. + """ + agent = AgentDef( + name="reviewer", + prompt="x", + working_dir="/tmp", + settings_dir="/tmp", + validator=ValidatorConfig(criteria="check"), + ) + + v = OutputValidator()._build_validator_agent(agent) + + assert v.settings_dir is None + assert v.working_dir == "/tmp", "working_dir is still inherited" + def test_no_tools_and_has_output_schema(self) -> None: v = OutputValidator()._build_validator_agent(_agent()) assert v.tools == [] diff --git a/tests/test_engine/test_workflow.py b/tests/test_engine/test_workflow.py index a3d19dfa..ec309538 100644 --- a/tests/test_engine/test_workflow.py +++ b/tests/test_engine/test_workflow.py @@ -4542,6 +4542,31 @@ async def test_a_template_rendering_empty_is_refused(self, tmp_path: Path) -> No assert "settings_dir" in str(exc_info.value) assert provider.calls == 0 + @pytest.mark.asyncio + async def test_an_empty_working_dir_render_is_refused_too(self, tmp_path: Path) -> None: + """The same guard covers ``working_dir``, and that is a deliberate change. + + An empty-rendering ``working_dir`` previously resolved to the workflow + file's own directory and ran there -- the same "nothing means + something" footgun, just without the filesystem grant that makes the + ``settings_dir`` case dangerous. Pinned in its own right so a later + edit narrowing the guard to ``settings_dir`` cannot silently restore + it, and so the behaviour change is visible to anyone reading the tests. + """ + provider = _RecordingWorkingDirProvider() + engine = WorkflowEngine( + _single_agent_config(working_dir="{{ workflow.input.repo }}"), + provider, + workflow_path=_workflow_file(tmp_path), + ) + + with pytest.raises(ExecutionError) as exc_info: + await engine.run({"repo": ""}) + + assert "empty string" in str(exc_info.value) + assert "working_dir" in str(exc_info.value) + assert provider.calls == 0 + class TestSettingsDirObservability: """``settings_dir`` must appear in the events, on all three agent paths. @@ -4590,3 +4615,146 @@ async def test_agent_started_reports_none_when_unset(self, tmp_path: Path) -> No started = [d for t, d in events if t == "agent_started"] assert started and "settings_dir" in started[0] assert started[0]["settings_dir"] is None + + +class TestSettingsDirInGroups: + """``settings_dir`` must resolve inside parallel groups and for-each loops. + + All three agent paths route through ``_resolve_agent_working_dir``, so the + field does reach them -- but nothing pinned that, and the for-each site is + the one where a per-iteration template (``{{ item }}``) is most likely to + be used. It resolves *after* loop-variable injection, an ordering a future + refactor of the for-each body could break with no test to notice: the + agent would then silently load a different iteration's skills. + """ + + @pytest.mark.asyncio + async def test_for_each_resolves_settings_dir_per_iteration(self, tmp_path: Path) -> None: + for name in ("one", "two"): + (tmp_path / name).mkdir() + config = WorkflowConfig( + workflow=WorkflowDef( + name="sd-for-each", + entry_point="lister", + runtime=RuntimeConfig(provider="claude-agent-sdk"), + context=ContextConfig(mode="accumulate"), + limits=LimitsConfig(max_iterations=10), + ), + agents=[ + AgentDef( + name="lister", + model="gpt-4", + prompt="List", + output={"repos": OutputField(type="array")}, + routes=[RouteDef(to="fans")], + ), + ], + for_each=[ + ForEachDef( + name="fans", + type="for_each", + source="lister.output.repos", + **{"as": "repo"}, + agent=AgentDef( + name="fan_agent", + model="gpt-4", + prompt="Work {{ repo }}", + settings_dir=str(tmp_path / "{{ repo }}"), + output={"r": OutputField(type="string")}, + ), + routes=[RouteDef(to="$end")], + ), + ], + output={}, + ) + seen: list[tuple[str, str | None]] = [] + + async def _execute(agent, context, rendered_prompt, tools=None, **kwargs): + if agent.name == "lister": + return AgentOutput( + content={"repos": ["one", "two"]}, + raw_response=None, + model=agent.model, + input_tokens=1, + output_tokens=1, + ) + seen.append((agent.name, agent.settings_dir)) + return AgentOutput( + content={"r": "ok"}, + raw_response=None, + model=agent.model, + input_tokens=1, + output_tokens=1, + ) + + provider = _RecordingWorkingDirProvider() + provider.execute = _execute # type: ignore[method-assign] + engine = WorkflowEngine(config, provider, workflow_path=_workflow_file(tmp_path)) + + await engine.run({}) + + assert sorted(d for _, d in seen) == sorted( + [os.path.normpath(str(tmp_path / "one")), os.path.normpath(str(tmp_path / "two"))] + ), seen + + @pytest.mark.asyncio + async def test_parallel_member_resolves_its_own_settings_dir(self, tmp_path: Path) -> None: + dir_a = tmp_path / "a" + dir_b = tmp_path / "b" + dir_a.mkdir() + dir_b.mkdir() + config = WorkflowConfig( + workflow=WorkflowDef( + name="sd-parallel", + entry_point="fan", + runtime=RuntimeConfig(provider="claude-agent-sdk"), + context=ContextConfig(mode="accumulate"), + limits=LimitsConfig(max_iterations=10), + ), + agents=[ + AgentDef( + name="member_a", + model="gpt-4", + prompt="A", + settings_dir=str(dir_a), + output={"r": OutputField(type="string")}, + ), + AgentDef( + name="member_b", + model="gpt-4", + prompt="B", + settings_dir=str(dir_b), + output={"r": OutputField(type="string")}, + ), + ], + parallel=[ + ParallelGroup( + name="fan", + agents=["member_a", "member_b"], + routes=[RouteDef(to="$end")], + ) + ], + output={}, + ) + seen: list[tuple[str, str | None]] = [] + + async def _execute(agent, context, rendered_prompt, tools=None, **kwargs): + seen.append((agent.name, agent.settings_dir)) + return AgentOutput( + content={"r": "ok"}, + raw_response=None, + model=agent.model, + input_tokens=1, + output_tokens=1, + ) + + provider = _RecordingWorkingDirProvider() + provider.execute = _execute # type: ignore[method-assign] + engine = WorkflowEngine(config, provider, workflow_path=_workflow_file(tmp_path)) + + await engine.run({}) + + assert sorted(seen) == [ + ("member_a", os.path.normpath(str(dir_a))), + ("member_b", os.path.normpath(str(dir_b))), + ], seen From 17d622dfd1cd25ec38ab499a2fc6b6b222db9587 Mon Sep 17 00:00:00 2001 From: Chris Throup Date: Wed, 9 Sep 2026 10:29:50 +0200 Subject: [PATCH 05/10] test(claude-agent-sdk): pin settings_dir on the two group event paths The event emission added for `settings_dir` covered three paths but only `agent_started` was tested: both group payload lines could be deleted with the suite fully green (8626 passing, and 264 passing across every file that mentions those event names). `TestSettingsDirInGroups` does not close the gap -- it asserts the value reaching `provider.execute`, which routes through `_resolve_agent_working_dir`, a different path from the payload. That is the same shape as the mutation leak the previous commit fixed: an unpinned line in a shared payload where the loss is silent. The tree already had the precedent -- `TestWorkingDirEvents` exists to pin these same two additive events for `working_dir`, with an `is None` control for each -- and the emission commit did not follow it. Adds `parallel_agent_started` and `for_each_agent_started` assertions plus a negative control, so the class now covers what its docstring claims and what `AGENTS.md` asserts. The for-each case checks per-iteration values, since that is where a templated `settings_dir` varies and most needs auditing. Verified by deleting both payload lines: all three new tests fail. Also records why the guard comment covers both fields (the `settings_dir`-only rationale sitting on that line is the reasoning that produced the earlier narrowing), and why the argv helper is local rather than reusing another test class's private one. --- src/conductor/engine/workflow.py | 9 +- tests/test_engine/test_workflow.py | 154 ++++++++++++++++++ tests/test_providers/test_claude_agent_sdk.py | 5 + 3 files changed, 166 insertions(+), 2 deletions(-) diff --git a/src/conductor/engine/workflow.py b/src/conductor/engine/workflow.py index d4a13376..902201e1 100644 --- a/src/conductor/engine/workflow.py +++ b/src/conductor/engine/workflow.py @@ -626,8 +626,13 @@ def _resolve_agent_directory( # min_length -- an --input given as `repo=`, a script step that printed # nothing, a `set` binding evaluating to "". Path("") is Path("."), which # is not absolute, so it would join onto the workflow file's own directory - # and pass the is_dir() check below. For settings_dir that silently grants - # the model file access to the workflow's own tree, so refuse it here. + # and pass the is_dir() check below: a value meaning "nothing" silently + # becoming somewhere real. + # + # Refused for BOTH fields, deliberately. settings_dir is the dangerous + # one -- there the workflow's own tree would be handed to the model's + # file tools -- but working_dir running an agent in the wrong directory + # is the same defect without the grant, so neither is worth keeping. if not rendered.strip(): raise ExecutionError( f"Agent '{agent.name}': {field} rendered to an empty string from '{raw}'", diff --git a/tests/test_engine/test_workflow.py b/tests/test_engine/test_workflow.py index ec309538..8fcbe460 100644 --- a/tests/test_engine/test_workflow.py +++ b/tests/test_engine/test_workflow.py @@ -4616,6 +4616,160 @@ async def test_agent_started_reports_none_when_unset(self, tmp_path: Path) -> No assert started and "settings_dir" in started[0] assert started[0]["settings_dir"] is None + @staticmethod + def _parallel_config(dir_a: Path, dir_b: Path) -> WorkflowConfig: + return WorkflowConfig( + workflow=WorkflowDef( + name="sd-ev-parallel", + entry_point="fan", + runtime=RuntimeConfig(provider="claude-agent-sdk"), + context=ContextConfig(mode="accumulate"), + limits=LimitsConfig(max_iterations=10), + ), + agents=[ + AgentDef( + name="member_a", + model="gpt-4", + prompt="A", + settings_dir=str(dir_a), + output={"r": OutputField(type="string")}, + ), + AgentDef( + name="member_b", + model="gpt-4", + prompt="B", + settings_dir=str(dir_b) if dir_b else None, + output={"r": OutputField(type="string")}, + ), + ], + parallel=[ + ParallelGroup( + name="fan", + agents=["member_a", "member_b"], + routes=[RouteDef(to="$end")], + ) + ], + output={}, + ) + + @pytest.mark.asyncio + async def test_parallel_agent_started_carries_settings_dir(self, tmp_path: Path) -> None: + """The parallel fan-out path, which the linear test cannot see. + + Resolution and emission are different code paths: deleting the field + from this payload leaves ``provider.execute`` receiving the right + directory, so ``TestSettingsDirInGroups`` stays green while the event + goes silent. + """ + dir_a = tmp_path / "a" + dir_b = tmp_path / "b" + dir_a.mkdir() + dir_b.mkdir() + events: list[tuple[str, dict]] = [] + engine = WorkflowEngine( + self._parallel_config(dir_a, dir_b), + _RecordingWorkingDirProvider(), + workflow_path=_workflow_file(tmp_path), + ) + engine._emit = lambda t, d=None: events.append((t, d or {})) # type: ignore[method-assign] + + await engine.run({}) + + started = [d for t, d in events if t == "parallel_agent_started"] + assert {d["agent_name"]: d["settings_dir"] for d in started} == { + "member_a": os.path.normpath(str(dir_a)), + "member_b": os.path.normpath(str(dir_b)), + }, started + + @pytest.mark.asyncio + async def test_parallel_agent_started_reports_none_when_unset(self, tmp_path: Path) -> None: + """Negative control, matching ``TestWorkingDirEvents``' own pattern.""" + dir_a = tmp_path / "a" + dir_a.mkdir() + config = self._parallel_config(dir_a, None) # type: ignore[arg-type] + events: list[tuple[str, dict]] = [] + engine = WorkflowEngine( + config, _RecordingWorkingDirProvider(), workflow_path=_workflow_file(tmp_path) + ) + engine._emit = lambda t, d=None: events.append((t, d or {})) # type: ignore[method-assign] + + await engine.run({}) + + started = {d["agent_name"]: d for t, d in events if t == "parallel_agent_started"} + assert started["member_b"]["settings_dir"] is None + assert started["member_a"]["settings_dir"] == os.path.normpath(str(dir_a)) + + @pytest.mark.asyncio + async def test_for_each_agent_started_carries_settings_dir_per_item( + self, tmp_path: Path + ) -> None: + """The for-each path, per iteration. + + This is where a templated ``settings_dir`` most needs auditing: the + value varies per item, so an event omitting it makes the grant + unauditable exactly where it changes. + """ + for name in ("one", "two"): + (tmp_path / name).mkdir() + config = WorkflowConfig( + workflow=WorkflowDef( + name="sd-ev-for-each", + entry_point="lister", + runtime=RuntimeConfig(provider="claude-agent-sdk"), + context=ContextConfig(mode="accumulate"), + limits=LimitsConfig(max_iterations=10), + ), + agents=[ + AgentDef( + name="lister", + model="gpt-4", + prompt="List", + output={"repos": OutputField(type="array")}, + routes=[RouteDef(to="fans")], + ), + ], + for_each=[ + ForEachDef( + name="fans", + type="for_each", + source="lister.output.repos", + **{"as": "repo"}, + agent=AgentDef( + name="fan_agent", + model="gpt-4", + prompt="Work {{ repo }}", + settings_dir=str(tmp_path / "{{ repo }}"), + output={"r": OutputField(type="string")}, + ), + routes=[RouteDef(to="$end")], + ), + ], + output={}, + ) + events: list[tuple[str, dict]] = [] + + async def _execute(agent, context, rendered_prompt, tools=None, **kwargs): + content = {"repos": ["one", "two"]} if agent.name == "lister" else {"r": "ok"} + return AgentOutput( + content=content, + raw_response=None, + model=agent.model, + input_tokens=1, + output_tokens=1, + ) + + provider = _RecordingWorkingDirProvider() + provider.execute = _execute # type: ignore[method-assign] + engine = WorkflowEngine(config, provider, workflow_path=_workflow_file(tmp_path)) + engine._emit = lambda t, d=None: events.append((t, d or {})) # type: ignore[method-assign] + + await engine.run({}) + + started = [d for t, d in events if t == "for_each_agent_started"] + assert sorted(d["settings_dir"] for d in started) == sorted( + [os.path.normpath(str(tmp_path / "one")), os.path.normpath(str(tmp_path / "two"))] + ), started + class TestSettingsDirInGroups: """``settings_dir`` must resolve inside parallel groups and for-each loops. diff --git a/tests/test_providers/test_claude_agent_sdk.py b/tests/test_providers/test_claude_agent_sdk.py index cdba4a33..961d85a5 100644 --- a/tests/test_providers/test_claude_agent_sdk.py +++ b/tests/test_providers/test_claude_agent_sdk.py @@ -3328,6 +3328,11 @@ async def fake_query(**kwargs): return captured["options"] def argv(options) -> list[str]: + # Deliberately local rather than reusing ``TestSkillsWiring._argv``: + # that is another class's private helper, and importing across test + # classes couples them. Both wrap the same three SDK calls; if the + # SDK's command builder moves, both fail together rather than one + # silently passing. transport = SubprocessCLITransport(prompt="hi", options=options) transport._cli_path = "/usr/bin/claude" return transport._build_command() From c05d3b700cb3df955e9be5cfd1f711b76052b361 Mon Sep 17 00:00:00 2001 From: Chris Throup Date: Wed, 9 Sep 2026 11:29:39 +0200 Subject: [PATCH 06/10] fix(claude-agent-sdk): warn at run time when settings_dir discovers no skills A `settings_dir` whose `project` tier is not enabled discovers nothing -- and the filesystem grant applies regardless, so the one effect the author did not ask for was the only one they got, with no diagnostic. Traced through the real provider: `add_dirs=['/target']` with `setting_sources=[]` and `skills=[]`, and no log output. `conductor validate` warned about this; `conductor run` never calls the static validator. That is the same gap this branch already closed twice for `capabilities.settings_dir`, and forgetting `setting_sources` is the likelier mistake of the two -- it lives at workflow scope while `settings_dir` is per agent, and the bare-string `provider: claude-agent-sdk` shorthand cannot carry it at all. Warned rather than raised, matching validate's own choice: the workflow is not wrong, just ineffective. Placed where `effective_sources` is already computed, so it also covers the two cases a workflow-level check would miss -- a non-`project` tier, and a per-agent `skills: []` that zeroes the tier for that agent. Parametrised over all five, and mutation-verified. Also: the schema docstring and the docs section opened with "Directory whose Claude Code *project* settings tier this agent loads", which over-promises (a reader expects CLAUDE.md, rules, env, hooks) and does not mention the grant at all. Both now lead with what it carries and what it grants; the prose already had the detail, but the first line is what a tooltip shows and the field name is what appears in every workflow file. --- AGENTS.md | 2 +- CHANGELOG.md | 6 ++- docs/workflow-syntax.md | 11 +++- src/conductor/config/schema.py | 7 ++- src/conductor/providers/claude_agent_sdk.py | 18 +++++++ tests/test_providers/test_claude_agent_sdk.py | 51 +++++++++++++++++++ 6 files changed, 89 insertions(+), 6 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 6a328e7a..b8b3decc 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -432,7 +432,7 @@ Conductor: - The path is passed **verbatim** — `WorkflowEngine._resolve_agent_working_dir` has already rendered, absolutized, normalised, and existence-checked it, and re-resolving here would collapse the symlink aliases the engine preserves on purpose. The `ClaudeAgentOptions(...)` construction lives **inside** `execute`'s `try` so the `os.getcwd()` fallback can't escape as a bare `OSError` when the process cwd has been deleted (`copilot.py` resolves its cwd inside its try for the same reason). - There is no provider-side `is_dir()` guard: a directory that vanishes after the engine's check surfaces as the SDK's `CLIConnectionError("Working directory does not exist: ")`, wrapped in `ProviderError`. That is only defensible because `_classify_startup_failure` special-cases it — `CLIConnectionError` otherwise yields firewall/binary advice and `is_retryable=True`, which is wrong for all three launch failures (missing dir; path is a file → `ENOTDIR`; unreadable dir → `EACCES`; the latter two reach the SDK's generic "Failed to start Claude Code" arm, not its dedicated one). - Knock-on effects: cwd is the project key for the CLI's on-disk transcript directory, and it is where a project `.mcp.json` and `.claude/` tree would be looked for. The unconditional `strict_mcp_config=True` stops a `.mcp.json` there from injecting undeclared servers, and `setting_sources=[]` (empty by default; see **Skills** below) stops the CLI loading `CLAUDE.md`, project settings, and hooks from it — so cwd no longer drags ambient instructions in unless the workflow opts in via `runtime.provider.setting_sources`, which is exactly a request to load them from that directory. `add_dirs` (the SDK's `--add-dir` passthrough) is a separate axis, carrying the per-agent `settings_dir` and nothing else — see **Target-repository skills** below. -- **Target-repository skills** (`settings_dir`): the per-agent `AgentDef.settings_dir` is the **only** source of `ClaudeAgentOptions.add_dirs`, and it selects which directory's *project* settings tier contributes skills. It exists because cwd was doing two conflicting jobs on this provider. The CLI supports the MCP Roots protocol and advertises exactly one root — its cwd — so `@modelcontextprotocol/server-filesystem` **discards the directories in its own argv** and permits cwd alone; cwd is simultaneously what the `project` tier resolves against. Narrowing cwd onto a target repository to reach its skills therefore narrowed the agent's only MCP root below any sibling path the step still had to read. Do not derive `add_dirs` from a server's directory arguments to compensate: `--add-dir` takes no part in Roots negotiation, it widens the CLI's own file tools, and doing so would also silently widen skill discovery to directories the author named as data. What `add_dirs` *does* do, measured: a named directory's `.claude/skills` become listed and invocable with cwd elsewhere entirely — and only those, not `CLAUDE.md`, `.claude/rules/*.md`, `.claude/settings.json` (so no `env` and no `hooks` — the hooks negative is measured with a side-effect probe whose control fires) or `.claude/agents`, which all keep following cwd. That makes it the *skills third* of what a cwd-resolved `project` tier loads, not a replacement for it. It carries a second, **unconditional** effect the skills framing hides: `add_dirs` is "additional directories Claude can access" per the SDK's own contract, so a `settings_dir` widens the model's built-in `Read`/`Edit`/`Bash` to that tree with no settings tier enabled at all (measured at `permission_mode: "default"` with `setting_sources` unset: a read outside cwd is refused without it and succeeds with it; an agent omitting `tools:` runs under `bypassPermissions`, where the grant is unobservable because reads already succeed). It does not widen what an MCP server permits. `capabilities.py::settings_dir` gates the field and `config/validator.py` errors against a provider that cannot apply it — enforced **twice**, with `AgentExecutor._reject_unsupported_settings_dir` repeating it at run time, because `conductor run` never calls the static validator — warning when it is set with no `setting_sources` (the skills half is then a no-op while the filesystem grant still applies). `WorkflowEngine._resolve_agent_directory` resolves both fields so they cannot drift, and `settings_dir` is per-agent only (no `runtime.` counterpart: the repository whose conventions apply is what varies between steps). `tests/test_integration/test_mcp_roots_negotiation.py` pins the negotiation rule itself against the real server with no LLM — two runs differing only in whether the client advertises `roots` — since every option here rests on it (marked `real_api`: it fetches the server from npm and pins *upstream's* behaviour, not Conductor's). The resolved value is emitted on `agent_started` / `parallel_agent_started` / `for_each_agent_started` alongside `working_dir`, because it is a trust decision — another repository's conventions plus a filesystem grant — and one the run output would otherwise never mention. +- **Target-repository skills** (`settings_dir`): the per-agent `AgentDef.settings_dir` is the **only** source of `ClaudeAgentOptions.add_dirs`, and it selects which directory's *project* settings tier contributes skills. It exists because cwd was doing two conflicting jobs on this provider. The CLI supports the MCP Roots protocol and advertises exactly one root — its cwd — so `@modelcontextprotocol/server-filesystem` **discards the directories in its own argv** and permits cwd alone; cwd is simultaneously what the `project` tier resolves against. Narrowing cwd onto a target repository to reach its skills therefore narrowed the agent's only MCP root below any sibling path the step still had to read. Do not derive `add_dirs` from a server's directory arguments to compensate: `--add-dir` takes no part in Roots negotiation, it widens the CLI's own file tools, and doing so would also silently widen skill discovery to directories the author named as data. What `add_dirs` *does* do, measured: a named directory's `.claude/skills` become listed and invocable with cwd elsewhere entirely — and only those, not `CLAUDE.md`, `.claude/rules/*.md`, `.claude/settings.json` (so no `env` and no `hooks` — the hooks negative is measured with a side-effect probe whose control fires) or `.claude/agents`, which all keep following cwd. That makes it the *skills third* of what a cwd-resolved `project` tier loads, not a replacement for it. It carries a second, **unconditional** effect the skills framing hides: `add_dirs` is "additional directories Claude can access" per the SDK's own contract, so a `settings_dir` widens the model's built-in `Read`/`Edit`/`Bash` to that tree with no settings tier enabled at all (measured at `permission_mode: "default"` with `setting_sources` unset: a read outside cwd is refused without it and succeeds with it; an agent omitting `tools:` runs under `bypassPermissions`, where the grant is unobservable because reads already succeed). It does not widen what an MCP server permits. `capabilities.py::settings_dir` gates the field and `config/validator.py` errors against a provider that cannot apply it — enforced **twice**, with `AgentExecutor._reject_unsupported_settings_dir` repeating it at run time, because `conductor run` never calls the static validator — warning when the agent's session will not enable the `project` tier (also twice: `config/validator.py` at validate, and `claude_agent_sdk.py::execute` at run time, since the filesystem grant applies even when the skills half no-ops) (the skills half is then a no-op while the filesystem grant still applies). `WorkflowEngine._resolve_agent_directory` resolves both fields so they cannot drift, and `settings_dir` is per-agent only (no `runtime.` counterpart: the repository whose conventions apply is what varies between steps). `tests/test_integration/test_mcp_roots_negotiation.py` pins the negotiation rule itself against the real server with no LLM — two runs differing only in whether the client advertises `roots` — since every option here rests on it (marked `real_api`: it fetches the server from npm and pins *upstream's* behaviour, not Conductor's). The resolved value is emitted on `agent_started` / `parallel_agent_started` / `for_each_agent_started` alongside `working_dir`, because it is a trust decision — another repository's conventions plus a filesystem grant — and one the run output would otherwise never mention. - **Skills** (issue #352): `supports_native_skills=True`. Skills are enabled through the SDK, not prompt injection, and three options move together in `execute`: - `plugins=[{"type": "local", "path": }]` + `skills=[":"]`. The SDK has no skill-*directory* surface, so `_resolve_skill_plugins` maps each directory back to the plugin that owns it via `skills/registry.py::resolve_skill_plugin`. That resolver is deliberately strict, because every one of these mistakes otherwise produces a name the CLI silently resolves to nothing: it bounds the upward walk (`_PLUGIN_SEARCH_DEPTH`), requires the skill to actually live under the candidate's `skills/` directory, requires `SKILL.md` to exist and its frontmatter `name` to equal the directory name (the CLI resolves by frontmatter name; Conductor sends the directory name), and rejects names outside `[A-Za-z0-9_.-]+` since they are joined into a comma-delimited `--allowedTools` value. A skill under no plugin root returns `None`; a plugin that is present but unusable raises `SkillPluginError`, which the provider re-raises as a `ProviderError` carrying the real reason rather than a blanket "not part of a plugin". Two plugins claiming one qualified name are refused too — deduping the clash away would drop a declared skill. All of it is `is_retryable=False`: these never become valid on a retry, and a checkout path containing "connection" would otherwise trip the retryability heuristic. Note providers are constructed lazily, so this surfaces when the first agent on this provider runs, **not** at `conductor validate`. - `setting_sources=[]` **by default**, for the same reason `strict_mcp_config=True` is unconditional a few lines away — but opt-in per workflow via `runtime.provider.setting_sources` (`user`/`project`/`local`, issue #501). Left unset, the CLI loads user, project, and local settings, which between them bring in ambient skills, `CLAUDE.md`, and hooks the workflow never declared, varying by machine and launch directory. Conductor surfaces instruction files through its own opt-in `--workspace-instructions`; settings and hooks have no equivalent. `skills=[]` and `skills=None` are **not** interchangeable upstream: `None` means "CLI defaults apply", and setting `skills` while leaving `setting_sources` at `None` makes the SDK re-default it to `["user", "project"]` — so the two options are coupled and dropping either re-opens the issue. The `[]` is also invisible in argv (it travels in the SDK's `initialize` control request), which is why the argv-based tests are paired with options-level assertions. The list is a context filter, not a sandbox: undeclared skills are hidden from the model's listing and rejected by the `Skill` tool, but their files stay readable. Opting a workflow in (`runtime.provider.setting_sources: [project]`, the motivating case being an agent whose `working_dir` is a *target* repo shipping its own `.claude/skills` — the CLI has `--plugin-dir` but no `--skill-dir`) loads that tier **with its hooks**, so it is only for repositories trusted as much as the workflow itself, and the field is workflow-global while `working_dir` is per agent. Two couplings follow: `_resolve_skill_filter` resolves `skills` to `"all"` when a tier is enabled and the workflow named none itself (tier-discovered skills never pass through `skill_names`, so `[]` would load the repo's skills and then hide every one), and a per-agent `skills: []` opts that agent out of the tier entirely — hooks included — keeping it the one opt-out. diff --git a/CHANGELOG.md b/CHANGELOG.md index 17281428..9528fcf8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -60,8 +60,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 reaches a permission mode where that is observable today. Only the skills of that directory travel — not `CLAUDE.md`, `.claude/rules/*.md`, `.claude/settings.json` or `.claude/agents`, all measured. Refused at - `conductor validate` *and* at run time on a provider that cannot apply it, - and reported on the agent lifecycle events so the grant is auditable. See + `conductor validate` *and* at run time on a provider that cannot apply it; + a `settings_dir` whose `project` tier is not enabled warns in both places + too, since the filesystem grant applies even when the skills half no-ops. + Reported on the agent lifecycle events so the grant is auditable. See [`examples/claude-agent-sdk-settings-dir.yaml`](examples/claude-agent-sdk-settings-dir.yaml). ### Fixed diff --git a/docs/workflow-syntax.md b/docs/workflow-syntax.md index e3f2f34b..dfab7af0 100644 --- a/docs/workflow-syntax.md +++ b/docs/workflow-syntax.md @@ -404,8 +404,11 @@ Because paths are normalized lexically instead of resolving to their real paths: ### Target-Repository Skills (`settings_dir`) -`settings_dir` names a second directory whose Claude Code *project* settings -tier the agent reads skills from. It applies only to `claude-agent-sdk` agents. +`settings_dir` names a second directory whose `.claude/skills` the agent may +use, and whose tree the model's built-in file tools may read. It carries the +*skills* third of a Claude Code `project` settings tier and nothing else of it +— the table below is exact about which — and the filesystem half applies +whether or not any tier is enabled. It applies only to `claude-agent-sdk` agents. Setting it against any other provider is an **error**, reported by `conductor validate` and again at run time — not a silently dropped field. The skills half additionally requires `runtime.provider.setting_sources` to enable the `project` @@ -477,6 +480,10 @@ Measured against the CLI: > Skill discovery is the *reason* to set this field; the filesystem grant is > its unavoidable companion. Point it at a directory the agent is entitled to > read. +> +> `conductor validate` warns when `settings_dir` is set without the `project` +> tier enabled, and so does the run itself — otherwise the only effect an +> author would get is the one they did not ask for. Note this grant is for the model's **built-in** tools only. It does not widen what a filesystem MCP server permits — that stays cwd alone, which is the diff --git a/src/conductor/config/schema.py b/src/conductor/config/schema.py index 71ea1a7f..34885ac4 100644 --- a/src/conductor/config/schema.py +++ b/src/conductor/config/schema.py @@ -1384,7 +1384,12 @@ class AgentDef(BaseModel): settings_dir: Annotated[str, StringConstraints(strip_whitespace=True, min_length=1)] | None = ( None ) - """Directory whose Claude Code *project* settings tier this agent loads. + """Directory whose ``.claude/skills`` this agent may use, and whose tree the + model's built-in file tools may read. + + Both halves of that first line are deliberate: this carries the *skills* + third of a Claude Code ``project`` settings tier and nothing else of it, + and it widens the model's filesystem access unconditionally. Details below. ``claude-agent-sdk`` only -- a provider that cannot apply it refuses it both at ``conductor validate`` and at run time, rather than dropping it diff --git a/src/conductor/providers/claude_agent_sdk.py b/src/conductor/providers/claude_agent_sdk.py index 99edc667..99b02e9d 100644 --- a/src/conductor/providers/claude_agent_sdk.py +++ b/src/conductor/providers/claude_agent_sdk.py @@ -964,6 +964,24 @@ async def _execute_session( # reads off ``agent.tools``. effective_sources: list[SettingSource] = [] if agent.skills == [] else self._setting_sources + # A settings_dir whose `project` tier is not enabled discovers no + # skills -- and the filesystem grant applies anyway, so the one effect + # the author did not ask for is the only one they get. + # ``conductor validate`` warns about this, but ``conductor run`` never + # calls the static validator, so without this the run is silent about + # a no-op the author is relying on. Warned rather than raised, matching + # validate's own choice: the workflow is not wrong, just ineffective. + if agent.settings_dir is not None and "project" not in effective_sources: + logger.warning( + "Agent '%s' sets settings_dir=%r but its session does not enable the " + "'project' settings tier, so no skills are discovered from that " + "directory. The directory is still granted to the model's built-in " + "file tools. Add 'project' to runtime.provider.setting_sources, or " + "remove settings_dir if the filesystem grant was not intended.", + agent.name, + agent.settings_dir, + ) + sdk_tools, permission_mode = self._resolve_tool_config( tools, agent, diff --git a/tests/test_providers/test_claude_agent_sdk.py b/tests/test_providers/test_claude_agent_sdk.py index 961d85a5..48f9f076 100644 --- a/tests/test_providers/test_claude_agent_sdk.py +++ b/tests/test_providers/test_claude_agent_sdk.py @@ -3344,3 +3344,54 @@ def argv(options) -> list[str]: # Negative control: without a settings_dir the flag is absent entirely, # so the assertion above cannot pass against an always-emitted flag. assert "--add-dir" not in argv(await options_for(None)) + + @pytest.mark.asyncio + @pytest.mark.parametrize( + ("sources", "agent_skills", "expect_warning"), + [ + (None, None, True), + (["user"], None, True), + (["local"], None, True), + (["project"], None, False), + (["project"], [], True), + ], + ) + async def test_a_settings_dir_with_no_project_tier_warns_at_run_time( + self, + tmp_path: Path, + caplog: pytest.LogCaptureFixture, + sources: list[str] | None, + agent_skills: list[str] | None, + expect_warning: bool, + ) -> None: + """``conductor run`` must not be silent when the skills half no-ops. + + ``conductor validate`` warns about this, but ``conductor run`` never + calls the static validator -- the same reason the four ``_reject_*`` + helpers exist. Without this the author gets the one effect they did + not ask for (the filesystem grant, which applies regardless) and no + diagnostic about the one they did. + """ + target = tmp_path / "repo" + target.mkdir() + + async def fake_query(**kwargs): + yield _result(result="ok") + + kwargs = {} if sources is None else {"setting_sources": sources} + with patch("conductor.providers.claude_agent_sdk.query", fake_query): + provider = ClaudeAgentSdkProvider(**kwargs) # type: ignore[arg-type] + with caplog.at_level(logging.WARNING): + await provider.execute( + agent=AgentDef( + name="judge", + prompt="hi", + settings_dir=str(target), + skills=agent_skills, + ), + context={}, + rendered_prompt="hi", + ) + + hits = [r for r in caplog.records if "no skills are discovered" in r.message] + assert bool(hits) is expect_warning, [r.message for r in caplog.records] From 2f89f2ae1d4b45a5ad52e2a2f32cb1c32d1a3218 Mon Sep 17 00:00:00 2001 From: Chris Throup Date: Wed, 9 Sep 2026 11:57:40 +0200 Subject: [PATCH 07/10] fix(claude-agent-sdk): latch the tier warning, and correct two stale descriptions The previous commit added a run-time mirror of the "settings_dir discovers no skills" warning and then updated only three of the five places that describe it: - AGENTS.md carried the new parenthetical AND the sentence it was meant to replace, stating the same thing twice. That file is loaded as agent instructions, so a garbled invariant there is asserted as fact by whatever reads it next. - The schema docstring still said `conductor validate` warns -- true before this branch, false after it, and an understatement of the guarantee the previous commit exists to provide. The same docstring already applies the twice-enforced idiom to the capability refusal six lines earlier. Both now match the code. The other three sites (provider message, docs section, CHANGELOG) were already correct. Also latches the warning per agent. It was firing once per execution, so a 50-item for_each emitted 50 identical lines naming the same agent and the same directory (measured: 5 warnings for 5 executions). Keyed by agent name rather than a bare flag, because a global latch would silence a *second* affected agent -- and naming the directory is the point of the warning. Follows the `_warned` latch convention in claude.py and engine/workflow.py. Two mutations confirm the test pins both properties: removing the latch, and making it global, each fail it. --- AGENTS.md | 2 +- src/conductor/config/schema.py | 5 +-- src/conductor/providers/claude_agent_sdk.py | 14 +++++++- tests/test_providers/test_claude_agent_sdk.py | 32 +++++++++++++++++++ 4 files changed, 49 insertions(+), 4 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index b8b3decc..470c0713 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -432,7 +432,7 @@ Conductor: - The path is passed **verbatim** — `WorkflowEngine._resolve_agent_working_dir` has already rendered, absolutized, normalised, and existence-checked it, and re-resolving here would collapse the symlink aliases the engine preserves on purpose. The `ClaudeAgentOptions(...)` construction lives **inside** `execute`'s `try` so the `os.getcwd()` fallback can't escape as a bare `OSError` when the process cwd has been deleted (`copilot.py` resolves its cwd inside its try for the same reason). - There is no provider-side `is_dir()` guard: a directory that vanishes after the engine's check surfaces as the SDK's `CLIConnectionError("Working directory does not exist: ")`, wrapped in `ProviderError`. That is only defensible because `_classify_startup_failure` special-cases it — `CLIConnectionError` otherwise yields firewall/binary advice and `is_retryable=True`, which is wrong for all three launch failures (missing dir; path is a file → `ENOTDIR`; unreadable dir → `EACCES`; the latter two reach the SDK's generic "Failed to start Claude Code" arm, not its dedicated one). - Knock-on effects: cwd is the project key for the CLI's on-disk transcript directory, and it is where a project `.mcp.json` and `.claude/` tree would be looked for. The unconditional `strict_mcp_config=True` stops a `.mcp.json` there from injecting undeclared servers, and `setting_sources=[]` (empty by default; see **Skills** below) stops the CLI loading `CLAUDE.md`, project settings, and hooks from it — so cwd no longer drags ambient instructions in unless the workflow opts in via `runtime.provider.setting_sources`, which is exactly a request to load them from that directory. `add_dirs` (the SDK's `--add-dir` passthrough) is a separate axis, carrying the per-agent `settings_dir` and nothing else — see **Target-repository skills** below. -- **Target-repository skills** (`settings_dir`): the per-agent `AgentDef.settings_dir` is the **only** source of `ClaudeAgentOptions.add_dirs`, and it selects which directory's *project* settings tier contributes skills. It exists because cwd was doing two conflicting jobs on this provider. The CLI supports the MCP Roots protocol and advertises exactly one root — its cwd — so `@modelcontextprotocol/server-filesystem` **discards the directories in its own argv** and permits cwd alone; cwd is simultaneously what the `project` tier resolves against. Narrowing cwd onto a target repository to reach its skills therefore narrowed the agent's only MCP root below any sibling path the step still had to read. Do not derive `add_dirs` from a server's directory arguments to compensate: `--add-dir` takes no part in Roots negotiation, it widens the CLI's own file tools, and doing so would also silently widen skill discovery to directories the author named as data. What `add_dirs` *does* do, measured: a named directory's `.claude/skills` become listed and invocable with cwd elsewhere entirely — and only those, not `CLAUDE.md`, `.claude/rules/*.md`, `.claude/settings.json` (so no `env` and no `hooks` — the hooks negative is measured with a side-effect probe whose control fires) or `.claude/agents`, which all keep following cwd. That makes it the *skills third* of what a cwd-resolved `project` tier loads, not a replacement for it. It carries a second, **unconditional** effect the skills framing hides: `add_dirs` is "additional directories Claude can access" per the SDK's own contract, so a `settings_dir` widens the model's built-in `Read`/`Edit`/`Bash` to that tree with no settings tier enabled at all (measured at `permission_mode: "default"` with `setting_sources` unset: a read outside cwd is refused without it and succeeds with it; an agent omitting `tools:` runs under `bypassPermissions`, where the grant is unobservable because reads already succeed). It does not widen what an MCP server permits. `capabilities.py::settings_dir` gates the field and `config/validator.py` errors against a provider that cannot apply it — enforced **twice**, with `AgentExecutor._reject_unsupported_settings_dir` repeating it at run time, because `conductor run` never calls the static validator — warning when the agent's session will not enable the `project` tier (also twice: `config/validator.py` at validate, and `claude_agent_sdk.py::execute` at run time, since the filesystem grant applies even when the skills half no-ops) (the skills half is then a no-op while the filesystem grant still applies). `WorkflowEngine._resolve_agent_directory` resolves both fields so they cannot drift, and `settings_dir` is per-agent only (no `runtime.` counterpart: the repository whose conventions apply is what varies between steps). `tests/test_integration/test_mcp_roots_negotiation.py` pins the negotiation rule itself against the real server with no LLM — two runs differing only in whether the client advertises `roots` — since every option here rests on it (marked `real_api`: it fetches the server from npm and pins *upstream's* behaviour, not Conductor's). The resolved value is emitted on `agent_started` / `parallel_agent_started` / `for_each_agent_started` alongside `working_dir`, because it is a trust decision — another repository's conventions plus a filesystem grant — and one the run output would otherwise never mention. +- **Target-repository skills** (`settings_dir`): the per-agent `AgentDef.settings_dir` is the **only** source of `ClaudeAgentOptions.add_dirs`, and it selects which directory's *project* settings tier contributes skills. It exists because cwd was doing two conflicting jobs on this provider. The CLI supports the MCP Roots protocol and advertises exactly one root — its cwd — so `@modelcontextprotocol/server-filesystem` **discards the directories in its own argv** and permits cwd alone; cwd is simultaneously what the `project` tier resolves against. Narrowing cwd onto a target repository to reach its skills therefore narrowed the agent's only MCP root below any sibling path the step still had to read. Do not derive `add_dirs` from a server's directory arguments to compensate: `--add-dir` takes no part in Roots negotiation, it widens the CLI's own file tools, and doing so would also silently widen skill discovery to directories the author named as data. What `add_dirs` *does* do, measured: a named directory's `.claude/skills` become listed and invocable with cwd elsewhere entirely — and only those, not `CLAUDE.md`, `.claude/rules/*.md`, `.claude/settings.json` (so no `env` and no `hooks` — the hooks negative is measured with a side-effect probe whose control fires) or `.claude/agents`, which all keep following cwd. That makes it the *skills third* of what a cwd-resolved `project` tier loads, not a replacement for it. It carries a second, **unconditional** effect the skills framing hides: `add_dirs` is "additional directories Claude can access" per the SDK's own contract, so a `settings_dir` widens the model's built-in `Read`/`Edit`/`Bash` to that tree with no settings tier enabled at all (measured at `permission_mode: "default"` with `setting_sources` unset: a read outside cwd is refused without it and succeeds with it; an agent omitting `tools:` runs under `bypassPermissions`, where the grant is unobservable because reads already succeed). It does not widen what an MCP server permits. `capabilities.py::settings_dir` gates the field and `config/validator.py` errors against a provider that cannot apply it — enforced **twice**, with `AgentExecutor._reject_unsupported_settings_dir` repeating it at run time, because `conductor run` never calls the static validator — warning when the agent's session will not enable the `project` tier (also twice: `config/validator.py` at validate, and `claude_agent_sdk.py::execute` at run time, since the filesystem grant applies even when the skills half no-ops). `WorkflowEngine._resolve_agent_directory` resolves both fields so they cannot drift, and `settings_dir` is per-agent only (no `runtime.` counterpart: the repository whose conventions apply is what varies between steps). `tests/test_integration/test_mcp_roots_negotiation.py` pins the negotiation rule itself against the real server with no LLM — two runs differing only in whether the client advertises `roots` — since every option here rests on it (marked `real_api`: it fetches the server from npm and pins *upstream's* behaviour, not Conductor's). The resolved value is emitted on `agent_started` / `parallel_agent_started` / `for_each_agent_started` alongside `working_dir`, because it is a trust decision — another repository's conventions plus a filesystem grant — and one the run output would otherwise never mention. - **Skills** (issue #352): `supports_native_skills=True`. Skills are enabled through the SDK, not prompt injection, and three options move together in `execute`: - `plugins=[{"type": "local", "path": }]` + `skills=[":"]`. The SDK has no skill-*directory* surface, so `_resolve_skill_plugins` maps each directory back to the plugin that owns it via `skills/registry.py::resolve_skill_plugin`. That resolver is deliberately strict, because every one of these mistakes otherwise produces a name the CLI silently resolves to nothing: it bounds the upward walk (`_PLUGIN_SEARCH_DEPTH`), requires the skill to actually live under the candidate's `skills/` directory, requires `SKILL.md` to exist and its frontmatter `name` to equal the directory name (the CLI resolves by frontmatter name; Conductor sends the directory name), and rejects names outside `[A-Za-z0-9_.-]+` since they are joined into a comma-delimited `--allowedTools` value. A skill under no plugin root returns `None`; a plugin that is present but unusable raises `SkillPluginError`, which the provider re-raises as a `ProviderError` carrying the real reason rather than a blanket "not part of a plugin". Two plugins claiming one qualified name are refused too — deduping the clash away would drop a declared skill. All of it is `is_retryable=False`: these never become valid on a retry, and a checkout path containing "connection" would otherwise trip the retryability heuristic. Note providers are constructed lazily, so this surfaces when the first agent on this provider runs, **not** at `conductor validate`. - `setting_sources=[]` **by default**, for the same reason `strict_mcp_config=True` is unconditional a few lines away — but opt-in per workflow via `runtime.provider.setting_sources` (`user`/`project`/`local`, issue #501). Left unset, the CLI loads user, project, and local settings, which between them bring in ambient skills, `CLAUDE.md`, and hooks the workflow never declared, varying by machine and launch directory. Conductor surfaces instruction files through its own opt-in `--workspace-instructions`; settings and hooks have no equivalent. `skills=[]` and `skills=None` are **not** interchangeable upstream: `None` means "CLI defaults apply", and setting `skills` while leaving `setting_sources` at `None` makes the SDK re-default it to `["user", "project"]` — so the two options are coupled and dropping either re-opens the issue. The `[]` is also invisible in argv (it travels in the SDK's `initialize` control request), which is why the argv-based tests are paired with options-level assertions. The list is a context filter, not a sandbox: undeclared skills are hidden from the model's listing and rejected by the `Skill` tool, but their files stay readable. Opting a workflow in (`runtime.provider.setting_sources: [project]`, the motivating case being an agent whose `working_dir` is a *target* repo shipping its own `.claude/skills` — the CLI has `--plugin-dir` but no `--skill-dir`) loads that tier **with its hooks**, so it is only for repositories trusted as much as the workflow itself, and the field is workflow-global while `working_dir` is per agent. Two couplings follow: `_resolve_skill_filter` resolves `skills` to `"all"` when a tier is enabled and the workflow named none itself (tier-discovered skills never pass through `skill_names`, so `[]` would load the repo's skills and then hide every one), and a per-agent `skills: []` opts that agent out of the tier entirely — hooks included — keeping it the one opt-out. diff --git a/src/conductor/config/schema.py b/src/conductor/config/schema.py index 34885ac4..cc0b2cef 100644 --- a/src/conductor/config/schema.py +++ b/src/conductor/config/schema.py @@ -1402,8 +1402,9 @@ class AgentDef(BaseModel): **Two effects, and only one of them is conditional.** Skill discovery requires ``runtime.provider.setting_sources`` to enable the ``project`` - tier; ``conductor validate`` warns when this field is set without it, - since the skills half is then a no-op. The *filesystem* grant is + tier; both ``conductor validate`` and the run itself warn when this field + is set without it, since the skills half is then a no-op and + ``conductor run`` never calls the static validator. The *filesystem* grant is unconditional: ``add_dirs``' own SDK contract is "additional directories Claude can access beyond the current working directory", so naming a directory here widens the model's built-in ``Read``/``Edit``/``Bash`` diff --git a/src/conductor/providers/claude_agent_sdk.py b/src/conductor/providers/claude_agent_sdk.py index 99b02e9d..26a10620 100644 --- a/src/conductor/providers/claude_agent_sdk.py +++ b/src/conductor/providers/claude_agent_sdk.py @@ -741,6 +741,13 @@ def __init__( # the point, and by cwd because the CLI stores transcripts per working # directory, so one key under two directories is two sessions. self._session_ids: dict[tuple[str, str], str] = {} + # Agents already warned about a settings_dir with no `project` tier. + # Keyed by agent name, not a bare flag: the condition is per agent, so + # a global latch would silence a second affected agent. Latched at all + # because the condition is static per agent while executions are not -- + # a 50-item for_each would otherwise emit 50 identical lines. Matches + # the `_warned` convention in claude.py and engine/workflow.py. + self._settings_dir_tier_warned: set[str] = set() self._resume_session_ids: dict[tuple[str, str], str] = {} # Slots currently executing, so a second execution cannot resume a # session the first still has open — see :meth:`_claim_session_slot`. @@ -971,7 +978,12 @@ async def _execute_session( # calls the static validator, so without this the run is silent about # a no-op the author is relying on. Warned rather than raised, matching # validate's own choice: the workflow is not wrong, just ineffective. - if agent.settings_dir is not None and "project" not in effective_sources: + if ( + agent.settings_dir is not None + and "project" not in effective_sources + and agent.name not in self._settings_dir_tier_warned + ): + self._settings_dir_tier_warned.add(agent.name) logger.warning( "Agent '%s' sets settings_dir=%r but its session does not enable the " "'project' settings tier, so no skills are discovered from that " diff --git a/tests/test_providers/test_claude_agent_sdk.py b/tests/test_providers/test_claude_agent_sdk.py index 48f9f076..0f630ed5 100644 --- a/tests/test_providers/test_claude_agent_sdk.py +++ b/tests/test_providers/test_claude_agent_sdk.py @@ -3395,3 +3395,35 @@ async def fake_query(**kwargs): hits = [r for r in caplog.records if "no skills are discovered" in r.message] assert bool(hits) is expect_warning, [r.message for r in caplog.records] + + @pytest.mark.asyncio + async def test_the_tier_warning_is_latched_per_agent( + self, tmp_path: Path, caplog: pytest.LogCaptureFixture + ) -> None: + """Once per agent, not once per execution -- and not once per run. + + The condition is static per agent while executions are not, so an + unlatched warning would emit one identical line per for-each item. + Keyed by agent name rather than a bare flag because a global latch + would silence a *second* affected agent, which is the case that + matters: the point of the warning is naming the directory. + """ + target = tmp_path / "repo" + target.mkdir() + + async def fake_query(**kwargs): + yield _result(result="ok") + + def agent(name: str) -> AgentDef: + return AgentDef(name=name, prompt="hi", settings_dir=str(target)) + + with patch("conductor.providers.claude_agent_sdk.query", fake_query): + provider = ClaudeAgentSdkProvider() + with caplog.at_level(logging.WARNING): + for name in ("fan", "fan", "fan", "other", "other"): + await provider.execute(agent=agent(name), context={}, rendered_prompt="hi") + + warned = [ + r.args[0] for r in caplog.records if "no skills are discovered" in r.message and r.args + ] + assert warned == ["fan", "other"], warned From a23fcd0f6d16213b91b5eaf5a333d0882bc4e554 Mon Sep 17 00:00:00 2001 From: Chris Throup Date: Wed, 9 Sep 2026 12:21:45 +0200 Subject: [PATCH 08/10] fix(claude-agent-sdk): key the tier-warning latch so a for_each dedupes The latch added in e17c0cb did not fix the case its own message cited. The engine renames a for_each member per item (`[]`, engine/workflow.py), so keying on `agent.name` gave every iteration a distinct key: an 8-item loop still emitted 8 warnings naming the same directory. Measured before and after. Keyed on the resolved directory instead, which dedupes both a for_each fan-out and repeated loop-backs. The trade is recorded at the declaration: two differently-named agents naming the SAME directory now warn once, naming only the first. Accepted because the actionable content is the directory and the remedy is workflow-global, so the second line would add nothing -- and the alternatives are worse. A bare flag would hide a second directory entirely (`settings_dir` is rendered per execution, so one agent can name several), and any name-bearing key reinstates the for_each noise. The test now drives the engine's own naming -- eight `fan[N]` iterations over one directory, plus one agent naming another -- rather than a name repeated verbatim, which no group path produces. Three mutations confirm it: no latch, a global flag, and the name key each fail it. Also branches the run-time remedy on its cause, as config/validator.py already does. With `setting_sources: [project]` and an agent's own `skills: []`, the message told the author to add 'project' -- already present, so the advice was a no-op and the warning kept firing. That is the same unactionable-advice defect raised against the validate-time message earlier in this branch; the run-time mirror had inherited only one of its three branches. --- AGENTS.md | 2 +- src/conductor/providers/claude_agent_sdk.py | 41 ++++++--- tests/test_providers/test_claude_agent_sdk.py | 87 +++++++++++++++---- 3 files changed, 104 insertions(+), 26 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 470c0713..68ccaf58 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -432,7 +432,7 @@ Conductor: - The path is passed **verbatim** — `WorkflowEngine._resolve_agent_working_dir` has already rendered, absolutized, normalised, and existence-checked it, and re-resolving here would collapse the symlink aliases the engine preserves on purpose. The `ClaudeAgentOptions(...)` construction lives **inside** `execute`'s `try` so the `os.getcwd()` fallback can't escape as a bare `OSError` when the process cwd has been deleted (`copilot.py` resolves its cwd inside its try for the same reason). - There is no provider-side `is_dir()` guard: a directory that vanishes after the engine's check surfaces as the SDK's `CLIConnectionError("Working directory does not exist: ")`, wrapped in `ProviderError`. That is only defensible because `_classify_startup_failure` special-cases it — `CLIConnectionError` otherwise yields firewall/binary advice and `is_retryable=True`, which is wrong for all three launch failures (missing dir; path is a file → `ENOTDIR`; unreadable dir → `EACCES`; the latter two reach the SDK's generic "Failed to start Claude Code" arm, not its dedicated one). - Knock-on effects: cwd is the project key for the CLI's on-disk transcript directory, and it is where a project `.mcp.json` and `.claude/` tree would be looked for. The unconditional `strict_mcp_config=True` stops a `.mcp.json` there from injecting undeclared servers, and `setting_sources=[]` (empty by default; see **Skills** below) stops the CLI loading `CLAUDE.md`, project settings, and hooks from it — so cwd no longer drags ambient instructions in unless the workflow opts in via `runtime.provider.setting_sources`, which is exactly a request to load them from that directory. `add_dirs` (the SDK's `--add-dir` passthrough) is a separate axis, carrying the per-agent `settings_dir` and nothing else — see **Target-repository skills** below. -- **Target-repository skills** (`settings_dir`): the per-agent `AgentDef.settings_dir` is the **only** source of `ClaudeAgentOptions.add_dirs`, and it selects which directory's *project* settings tier contributes skills. It exists because cwd was doing two conflicting jobs on this provider. The CLI supports the MCP Roots protocol and advertises exactly one root — its cwd — so `@modelcontextprotocol/server-filesystem` **discards the directories in its own argv** and permits cwd alone; cwd is simultaneously what the `project` tier resolves against. Narrowing cwd onto a target repository to reach its skills therefore narrowed the agent's only MCP root below any sibling path the step still had to read. Do not derive `add_dirs` from a server's directory arguments to compensate: `--add-dir` takes no part in Roots negotiation, it widens the CLI's own file tools, and doing so would also silently widen skill discovery to directories the author named as data. What `add_dirs` *does* do, measured: a named directory's `.claude/skills` become listed and invocable with cwd elsewhere entirely — and only those, not `CLAUDE.md`, `.claude/rules/*.md`, `.claude/settings.json` (so no `env` and no `hooks` — the hooks negative is measured with a side-effect probe whose control fires) or `.claude/agents`, which all keep following cwd. That makes it the *skills third* of what a cwd-resolved `project` tier loads, not a replacement for it. It carries a second, **unconditional** effect the skills framing hides: `add_dirs` is "additional directories Claude can access" per the SDK's own contract, so a `settings_dir` widens the model's built-in `Read`/`Edit`/`Bash` to that tree with no settings tier enabled at all (measured at `permission_mode: "default"` with `setting_sources` unset: a read outside cwd is refused without it and succeeds with it; an agent omitting `tools:` runs under `bypassPermissions`, where the grant is unobservable because reads already succeed). It does not widen what an MCP server permits. `capabilities.py::settings_dir` gates the field and `config/validator.py` errors against a provider that cannot apply it — enforced **twice**, with `AgentExecutor._reject_unsupported_settings_dir` repeating it at run time, because `conductor run` never calls the static validator — warning when the agent's session will not enable the `project` tier (also twice: `config/validator.py` at validate, and `claude_agent_sdk.py::execute` at run time, since the filesystem grant applies even when the skills half no-ops). `WorkflowEngine._resolve_agent_directory` resolves both fields so they cannot drift, and `settings_dir` is per-agent only (no `runtime.` counterpart: the repository whose conventions apply is what varies between steps). `tests/test_integration/test_mcp_roots_negotiation.py` pins the negotiation rule itself against the real server with no LLM — two runs differing only in whether the client advertises `roots` — since every option here rests on it (marked `real_api`: it fetches the server from npm and pins *upstream's* behaviour, not Conductor's). The resolved value is emitted on `agent_started` / `parallel_agent_started` / `for_each_agent_started` alongside `working_dir`, because it is a trust decision — another repository's conventions plus a filesystem grant — and one the run output would otherwise never mention. +- **Target-repository skills** (`settings_dir`): the per-agent `AgentDef.settings_dir` is the **only** source of `ClaudeAgentOptions.add_dirs`, and it selects which directory's *project* settings tier contributes skills. It exists because cwd was doing two conflicting jobs on this provider. The CLI supports the MCP Roots protocol and advertises exactly one root — its cwd — so `@modelcontextprotocol/server-filesystem` **discards the directories in its own argv** and permits cwd alone; cwd is simultaneously what the `project` tier resolves against. Narrowing cwd onto a target repository to reach its skills therefore narrowed the agent's only MCP root below any sibling path the step still had to read. Do not derive `add_dirs` from a server's directory arguments to compensate: `--add-dir` takes no part in Roots negotiation, it widens the CLI's own file tools, and doing so would also silently widen skill discovery to directories the author named as data. What `add_dirs` *does* do, measured: a named directory's `.claude/skills` become listed and invocable with cwd elsewhere entirely — and only those, not `CLAUDE.md`, `.claude/rules/*.md`, `.claude/settings.json` (so no `env` and no `hooks` — the hooks negative is measured with a side-effect probe whose control fires) or `.claude/agents`, which all keep following cwd. That makes it the *skills third* of what a cwd-resolved `project` tier loads, not a replacement for it. It carries a second, **unconditional** effect the skills framing hides: `add_dirs` is "additional directories Claude can access" per the SDK's own contract, so a `settings_dir` widens the model's built-in `Read`/`Edit`/`Bash` to that tree with no settings tier enabled at all (measured at `permission_mode: "default"` with `setting_sources` unset: a read outside cwd is refused without it and succeeds with it; an agent omitting `tools:` runs under `bypassPermissions`, where the grant is unobservable because reads already succeed). It does not widen what an MCP server permits. `capabilities.py::settings_dir` gates the field and `config/validator.py` errors against a provider that cannot apply it — enforced **twice**, with `AgentExecutor._reject_unsupported_settings_dir` repeating it at run time, because `conductor run` never calls the static validator — warning when the agent's session will not enable the `project` tier (also twice: `config/validator.py` at validate, and `claude_agent_sdk.py::execute` at run time, since the filesystem grant applies even when the skills half no-ops; the run-time half is latched once per resolved directory, not per agent, because a `for_each` member is renamed per item and would otherwise warn once per iteration). `WorkflowEngine._resolve_agent_directory` resolves both fields so they cannot drift, and `settings_dir` is per-agent only (no `runtime.` counterpart: the repository whose conventions apply is what varies between steps). `tests/test_integration/test_mcp_roots_negotiation.py` pins the negotiation rule itself against the real server with no LLM — two runs differing only in whether the client advertises `roots` — since every option here rests on it (marked `real_api`: it fetches the server from npm and pins *upstream's* behaviour, not Conductor's). The resolved value is emitted on `agent_started` / `parallel_agent_started` / `for_each_agent_started` alongside `working_dir`, because it is a trust decision — another repository's conventions plus a filesystem grant — and one the run output would otherwise never mention. - **Skills** (issue #352): `supports_native_skills=True`. Skills are enabled through the SDK, not prompt injection, and three options move together in `execute`: - `plugins=[{"type": "local", "path": }]` + `skills=[":"]`. The SDK has no skill-*directory* surface, so `_resolve_skill_plugins` maps each directory back to the plugin that owns it via `skills/registry.py::resolve_skill_plugin`. That resolver is deliberately strict, because every one of these mistakes otherwise produces a name the CLI silently resolves to nothing: it bounds the upward walk (`_PLUGIN_SEARCH_DEPTH`), requires the skill to actually live under the candidate's `skills/` directory, requires `SKILL.md` to exist and its frontmatter `name` to equal the directory name (the CLI resolves by frontmatter name; Conductor sends the directory name), and rejects names outside `[A-Za-z0-9_.-]+` since they are joined into a comma-delimited `--allowedTools` value. A skill under no plugin root returns `None`; a plugin that is present but unusable raises `SkillPluginError`, which the provider re-raises as a `ProviderError` carrying the real reason rather than a blanket "not part of a plugin". Two plugins claiming one qualified name are refused too — deduping the clash away would drop a declared skill. All of it is `is_retryable=False`: these never become valid on a retry, and a checkout path containing "connection" would otherwise trip the retryability heuristic. Note providers are constructed lazily, so this surfaces when the first agent on this provider runs, **not** at `conductor validate`. - `setting_sources=[]` **by default**, for the same reason `strict_mcp_config=True` is unconditional a few lines away — but opt-in per workflow via `runtime.provider.setting_sources` (`user`/`project`/`local`, issue #501). Left unset, the CLI loads user, project, and local settings, which between them bring in ambient skills, `CLAUDE.md`, and hooks the workflow never declared, varying by machine and launch directory. Conductor surfaces instruction files through its own opt-in `--workspace-instructions`; settings and hooks have no equivalent. `skills=[]` and `skills=None` are **not** interchangeable upstream: `None` means "CLI defaults apply", and setting `skills` while leaving `setting_sources` at `None` makes the SDK re-default it to `["user", "project"]` — so the two options are coupled and dropping either re-opens the issue. The `[]` is also invisible in argv (it travels in the SDK's `initialize` control request), which is why the argv-based tests are paired with options-level assertions. The list is a context filter, not a sandbox: undeclared skills are hidden from the model's listing and rejected by the `Skill` tool, but their files stay readable. Opting a workflow in (`runtime.provider.setting_sources: [project]`, the motivating case being an agent whose `working_dir` is a *target* repo shipping its own `.claude/skills` — the CLI has `--plugin-dir` but no `--skill-dir`) loads that tier **with its hooks**, so it is only for repositories trusted as much as the workflow itself, and the field is workflow-global while `working_dir` is per agent. Two couplings follow: `_resolve_skill_filter` resolves `skills` to `"all"` when a tier is enabled and the workflow named none itself (tier-discovered skills never pass through `skill_names`, so `[]` would load the repo's skills and then hide every one), and a per-agent `skills: []` opts that agent out of the tier entirely — hooks included — keeping it the one opt-out. diff --git a/src/conductor/providers/claude_agent_sdk.py b/src/conductor/providers/claude_agent_sdk.py index 26a10620..d53e1435 100644 --- a/src/conductor/providers/claude_agent_sdk.py +++ b/src/conductor/providers/claude_agent_sdk.py @@ -741,12 +741,22 @@ def __init__( # the point, and by cwd because the CLI stores transcripts per working # directory, so one key under two directories is two sessions. self._session_ids: dict[tuple[str, str], str] = {} - # Agents already warned about a settings_dir with no `project` tier. - # Keyed by agent name, not a bare flag: the condition is per agent, so - # a global latch would silence a second affected agent. Latched at all - # because the condition is static per agent while executions are not -- - # a 50-item for_each would otherwise emit 50 identical lines. Matches - # the `_warned` convention in claude.py and engine/workflow.py. + # settings_dir values already warned about for having no `project` + # tier. Keyed by the resolved DIRECTORY, and that choice is a trade: + # + # - Not the agent name, alone or paired with the directory: the engine + # renames a for_each member per item (`[]`, + # engine/workflow.py), so any name-bearing key emits one line per + # item -- the very case latching exists to prevent. + # - Not a bare flag: `settings_dir` is Jinja-rendered per execution, so + # one agent can name several directories across loop-backs, and each + # is a distinct grant the operator needs told about. + # - The cost, accepted: two differently-named agents naming the SAME + # directory warn once, naming only the first. The actionable content + # is the directory and the remedy is workflow-global, so the second + # line would add nothing the first did not say. + # + # Matches the `_warned` convention in claude.py and engine/workflow.py. self._settings_dir_tier_warned: set[str] = set() self._resume_session_ids: dict[tuple[str, str], str] = {} # Slots currently executing, so a second execution cannot resume a @@ -981,17 +991,28 @@ async def _execute_session( if ( agent.settings_dir is not None and "project" not in effective_sources - and agent.name not in self._settings_dir_tier_warned + and agent.settings_dir not in self._settings_dir_tier_warned ): - self._settings_dir_tier_warned.add(agent.name) + self._settings_dir_tier_warned.add(agent.settings_dir) + # The remedy depends on the cause, as it does in + # config/validator.py: telling an author to add 'project' when + # their own `skills: []` is what zeroed the tier sends them to add + # a value that is already there, and the warning keeps firing. + remedy = ( + "This agent's own 'skills: []' opts it out of the settings tiers " + "entirely; remove it to let the tier apply" + if agent.skills == [] + else "Add 'project' to runtime.provider.setting_sources" + ) logger.warning( "Agent '%s' sets settings_dir=%r but its session does not enable the " "'project' settings tier, so no skills are discovered from that " "directory. The directory is still granted to the model's built-in " - "file tools. Add 'project' to runtime.provider.setting_sources, or " - "remove settings_dir if the filesystem grant was not intended.", + "file tools. %s, or remove settings_dir if the filesystem grant was " + "not intended.", agent.name, agent.settings_dir, + remedy, ) sdk_tools, permission_mode = self._resolve_tool_config( diff --git a/tests/test_providers/test_claude_agent_sdk.py b/tests/test_providers/test_claude_agent_sdk.py index 0f630ed5..784d2e63 100644 --- a/tests/test_providers/test_claude_agent_sdk.py +++ b/tests/test_providers/test_claude_agent_sdk.py @@ -3397,33 +3397,90 @@ async def fake_query(**kwargs): assert bool(hits) is expect_warning, [r.message for r in caplog.records] @pytest.mark.asyncio - async def test_the_tier_warning_is_latched_per_agent( + async def test_the_tier_warning_is_latched_per_directory( self, tmp_path: Path, caplog: pytest.LogCaptureFixture ) -> None: - """Once per agent, not once per execution -- and not once per run. - - The condition is static per agent while executions are not, so an - unlatched warning would emit one identical line per for-each item. - Keyed by agent name rather than a bare flag because a global latch - would silence a *second* affected agent, which is the case that - matters: the point of the warning is naming the directory. + """Once per directory, and specifically once across a for_each. + + Latched because the condition is static while executions are not. + Keyed by the resolved directory rather than the agent name because + the engine renames a for_each member per item (``[]``), + so a name-keyed latch emits one line per item -- the exact case + latching exists to prevent. Not a bare flag either: a second agent + naming a *different* directory must still be reported, since naming + the directory is the point of the warning. """ target = tmp_path / "repo" + other = tmp_path / "other" target.mkdir() + other.mkdir() async def fake_query(**kwargs): yield _result(result="ok") - def agent(name: str) -> AgentDef: - return AgentDef(name=name, prompt="hi", settings_dir=str(target)) - with patch("conductor.providers.claude_agent_sdk.query", fake_query): provider = ClaudeAgentSdkProvider() with caplog.at_level(logging.WARNING): - for name in ("fan", "fan", "fan", "other", "other"): - await provider.execute(agent=agent(name), context={}, rendered_prompt="hi") + # Eight for_each iterations over one directory, as the engine + # drives them: same settings_dir, a fresh name each time. + for key in range(8): + await provider.execute( + agent=AgentDef(name=f"fan[{key}]", prompt="hi", settings_dir=str(target)), + context={}, + rendered_prompt="hi", + ) + await provider.execute( + agent=AgentDef(name="judge", prompt="hi", settings_dir=str(other)), + context={}, + rendered_prompt="hi", + ) warned = [ - r.args[0] for r in caplog.records if "no skills are discovered" in r.message and r.args + (r.args[0], r.args[1]) + for r in caplog.records + if "no skills are discovered" in r.message and r.args ] - assert warned == ["fan", "other"], warned + assert warned == [("fan[0]", str(target)), ("judge", str(other))], warned + + @pytest.mark.asyncio + async def test_the_tier_warning_remedy_matches_the_cause( + self, tmp_path: Path, caplog: pytest.LogCaptureFixture + ) -> None: + """Advice an author can act on, as ``config/validator.py`` does. + + Telling an author to add ``'project'`` when their own ``skills: []`` + is what zeroed the tier sends them to add a value already present, + and the warning keeps firing. + """ + target = tmp_path / "repo" + target.mkdir() + + async def fake_query(**kwargs): + yield _result(result="ok") + + async def remedy_for(sources: list[str] | None, skills: list[str] | None) -> str: + caplog.clear() + kwargs = {} if sources is None else {"setting_sources": sources} + with patch("conductor.providers.claude_agent_sdk.query", fake_query): + provider = ClaudeAgentSdkProvider(**kwargs) # type: ignore[arg-type] + with caplog.at_level(logging.WARNING): + await provider.execute( + agent=AgentDef( + name="judge", + prompt="hi", + settings_dir=str(target), + skills=skills, + ), + context={}, + rendered_prompt="hi", + ) + hits = [r for r in caplog.records if "no skills are discovered" in r.message] + assert hits, [r.message for r in caplog.records] + return hits[0].message + + no_tier = await remedy_for(None, None) + assert "Add 'project' to runtime.provider.setting_sources" in no_tier + + opted_out = await remedy_for(["project"], []) + assert "'skills: []' opts it out" in opted_out + assert "Add 'project'" not in opted_out, "advice is a no-op for this cause" From 2fc95614293d471c6da1005cb3c640884b6030e7 Mon Sep 17 00:00:00 2001 From: Chris Throup Date: Wed, 9 Sep 2026 12:44:37 +0200 Subject: [PATCH 09/10] fix(claude-agent-sdk): key the tier warning by cause too, and cover the third cause Two defects the previous commit introduced while fixing its own. The directory-only latch was justified with "the remedy is workflow-global, so the second line would add nothing" -- true of the unbranched message, and falsified by the remedy branch added in the same commit. Measured: no tier enabled, one agent with `skills: []` and one without, both naming the same directory -> a single line, naming only the first, prescribing a fix that is wrong for the agent it does not name. Now keyed on `(directory, cause)`, which keeps the for_each collapse (all members share a cause) and bounds the output at two lines per directory. The residual trade -- same directory AND same cause warns once -- is recorded, and holds, because the remedy is then identical for both. `config/validator.py` distinguishes three causes; the run-time mirror had ported two. The missing one is a per-agent `provider: claude-agent-sdk` override under a different `runtime.provider`: `providers/registry.py` forwards structured settings only to the matching provider, so the agent reaches the provider with no `setting_sources` and the warning fired telling the author to add it -- which the schema rejects unless `runtime.provider` is itself claude-agent-sdk, exactly the unactionable advice this branch fixed at validate time earlier. The provider cannot see the workflow-level provider name, so rather than plumb it through, the non-opt-out arm now names the requirement instead of prescribing the edit, which is true on both paths. AGENTS.md records the key and its accepted cost. Three mutations confirm the tests pin it: dropping the cause from the key, reverting to the agent name, and collapsing the remedy branch each fail. --- AGENTS.md | 2 +- src/conductor/providers/claude_agent_sdk.py | 44 +++++++---- tests/test_providers/test_claude_agent_sdk.py | 75 ++++++++++++++++++- 3 files changed, 104 insertions(+), 17 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 68ccaf58..94132711 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -432,7 +432,7 @@ Conductor: - The path is passed **verbatim** — `WorkflowEngine._resolve_agent_working_dir` has already rendered, absolutized, normalised, and existence-checked it, and re-resolving here would collapse the symlink aliases the engine preserves on purpose. The `ClaudeAgentOptions(...)` construction lives **inside** `execute`'s `try` so the `os.getcwd()` fallback can't escape as a bare `OSError` when the process cwd has been deleted (`copilot.py` resolves its cwd inside its try for the same reason). - There is no provider-side `is_dir()` guard: a directory that vanishes after the engine's check surfaces as the SDK's `CLIConnectionError("Working directory does not exist: ")`, wrapped in `ProviderError`. That is only defensible because `_classify_startup_failure` special-cases it — `CLIConnectionError` otherwise yields firewall/binary advice and `is_retryable=True`, which is wrong for all three launch failures (missing dir; path is a file → `ENOTDIR`; unreadable dir → `EACCES`; the latter two reach the SDK's generic "Failed to start Claude Code" arm, not its dedicated one). - Knock-on effects: cwd is the project key for the CLI's on-disk transcript directory, and it is where a project `.mcp.json` and `.claude/` tree would be looked for. The unconditional `strict_mcp_config=True` stops a `.mcp.json` there from injecting undeclared servers, and `setting_sources=[]` (empty by default; see **Skills** below) stops the CLI loading `CLAUDE.md`, project settings, and hooks from it — so cwd no longer drags ambient instructions in unless the workflow opts in via `runtime.provider.setting_sources`, which is exactly a request to load them from that directory. `add_dirs` (the SDK's `--add-dir` passthrough) is a separate axis, carrying the per-agent `settings_dir` and nothing else — see **Target-repository skills** below. -- **Target-repository skills** (`settings_dir`): the per-agent `AgentDef.settings_dir` is the **only** source of `ClaudeAgentOptions.add_dirs`, and it selects which directory's *project* settings tier contributes skills. It exists because cwd was doing two conflicting jobs on this provider. The CLI supports the MCP Roots protocol and advertises exactly one root — its cwd — so `@modelcontextprotocol/server-filesystem` **discards the directories in its own argv** and permits cwd alone; cwd is simultaneously what the `project` tier resolves against. Narrowing cwd onto a target repository to reach its skills therefore narrowed the agent's only MCP root below any sibling path the step still had to read. Do not derive `add_dirs` from a server's directory arguments to compensate: `--add-dir` takes no part in Roots negotiation, it widens the CLI's own file tools, and doing so would also silently widen skill discovery to directories the author named as data. What `add_dirs` *does* do, measured: a named directory's `.claude/skills` become listed and invocable with cwd elsewhere entirely — and only those, not `CLAUDE.md`, `.claude/rules/*.md`, `.claude/settings.json` (so no `env` and no `hooks` — the hooks negative is measured with a side-effect probe whose control fires) or `.claude/agents`, which all keep following cwd. That makes it the *skills third* of what a cwd-resolved `project` tier loads, not a replacement for it. It carries a second, **unconditional** effect the skills framing hides: `add_dirs` is "additional directories Claude can access" per the SDK's own contract, so a `settings_dir` widens the model's built-in `Read`/`Edit`/`Bash` to that tree with no settings tier enabled at all (measured at `permission_mode: "default"` with `setting_sources` unset: a read outside cwd is refused without it and succeeds with it; an agent omitting `tools:` runs under `bypassPermissions`, where the grant is unobservable because reads already succeed). It does not widen what an MCP server permits. `capabilities.py::settings_dir` gates the field and `config/validator.py` errors against a provider that cannot apply it — enforced **twice**, with `AgentExecutor._reject_unsupported_settings_dir` repeating it at run time, because `conductor run` never calls the static validator — warning when the agent's session will not enable the `project` tier (also twice: `config/validator.py` at validate, and `claude_agent_sdk.py::execute` at run time, since the filesystem grant applies even when the skills half no-ops; the run-time half is latched once per resolved directory, not per agent, because a `for_each` member is renamed per item and would otherwise warn once per iteration). `WorkflowEngine._resolve_agent_directory` resolves both fields so they cannot drift, and `settings_dir` is per-agent only (no `runtime.` counterpart: the repository whose conventions apply is what varies between steps). `tests/test_integration/test_mcp_roots_negotiation.py` pins the negotiation rule itself against the real server with no LLM — two runs differing only in whether the client advertises `roots` — since every option here rests on it (marked `real_api`: it fetches the server from npm and pins *upstream's* behaviour, not Conductor's). The resolved value is emitted on `agent_started` / `parallel_agent_started` / `for_each_agent_started` alongside `working_dir`, because it is a trust decision — another repository's conventions plus a filesystem grant — and one the run output would otherwise never mention. +- **Target-repository skills** (`settings_dir`): the per-agent `AgentDef.settings_dir` is the **only** source of `ClaudeAgentOptions.add_dirs`, and it selects which directory's *project* settings tier contributes skills. It exists because cwd was doing two conflicting jobs on this provider. The CLI supports the MCP Roots protocol and advertises exactly one root — its cwd — so `@modelcontextprotocol/server-filesystem` **discards the directories in its own argv** and permits cwd alone; cwd is simultaneously what the `project` tier resolves against. Narrowing cwd onto a target repository to reach its skills therefore narrowed the agent's only MCP root below any sibling path the step still had to read. Do not derive `add_dirs` from a server's directory arguments to compensate: `--add-dir` takes no part in Roots negotiation, it widens the CLI's own file tools, and doing so would also silently widen skill discovery to directories the author named as data. What `add_dirs` *does* do, measured: a named directory's `.claude/skills` become listed and invocable with cwd elsewhere entirely — and only those, not `CLAUDE.md`, `.claude/rules/*.md`, `.claude/settings.json` (so no `env` and no `hooks` — the hooks negative is measured with a side-effect probe whose control fires) or `.claude/agents`, which all keep following cwd. That makes it the *skills third* of what a cwd-resolved `project` tier loads, not a replacement for it. It carries a second, **unconditional** effect the skills framing hides: `add_dirs` is "additional directories Claude can access" per the SDK's own contract, so a `settings_dir` widens the model's built-in `Read`/`Edit`/`Bash` to that tree with no settings tier enabled at all (measured at `permission_mode: "default"` with `setting_sources` unset: a read outside cwd is refused without it and succeeds with it; an agent omitting `tools:` runs under `bypassPermissions`, where the grant is unobservable because reads already succeed). It does not widen what an MCP server permits. `capabilities.py::settings_dir` gates the field and `config/validator.py` errors against a provider that cannot apply it — enforced **twice**, with `AgentExecutor._reject_unsupported_settings_dir` repeating it at run time, because `conductor run` never calls the static validator — warning when the agent's session will not enable the `project` tier (also twice: `config/validator.py` at validate, and `claude_agent_sdk.py::execute` at run time, since the filesystem grant applies even when the skills half no-ops; the run-time half is latched per `(resolved directory, cause)` — not per agent, because a `for_each` member is renamed per item and would otherwise warn once per iteration, and not per directory alone, because the remedy differs by cause and one line would prescribe a fix wrong for the agent it does not name; two agents sharing a directory *and* a cause do collapse to one line). `WorkflowEngine._resolve_agent_directory` resolves both fields so they cannot drift, and `settings_dir` is per-agent only (no `runtime.` counterpart: the repository whose conventions apply is what varies between steps). `tests/test_integration/test_mcp_roots_negotiation.py` pins the negotiation rule itself against the real server with no LLM — two runs differing only in whether the client advertises `roots` — since every option here rests on it (marked `real_api`: it fetches the server from npm and pins *upstream's* behaviour, not Conductor's). The resolved value is emitted on `agent_started` / `parallel_agent_started` / `for_each_agent_started` alongside `working_dir`, because it is a trust decision — another repository's conventions plus a filesystem grant — and one the run output would otherwise never mention. - **Skills** (issue #352): `supports_native_skills=True`. Skills are enabled through the SDK, not prompt injection, and three options move together in `execute`: - `plugins=[{"type": "local", "path": }]` + `skills=[":"]`. The SDK has no skill-*directory* surface, so `_resolve_skill_plugins` maps each directory back to the plugin that owns it via `skills/registry.py::resolve_skill_plugin`. That resolver is deliberately strict, because every one of these mistakes otherwise produces a name the CLI silently resolves to nothing: it bounds the upward walk (`_PLUGIN_SEARCH_DEPTH`), requires the skill to actually live under the candidate's `skills/` directory, requires `SKILL.md` to exist and its frontmatter `name` to equal the directory name (the CLI resolves by frontmatter name; Conductor sends the directory name), and rejects names outside `[A-Za-z0-9_.-]+` since they are joined into a comma-delimited `--allowedTools` value. A skill under no plugin root returns `None`; a plugin that is present but unusable raises `SkillPluginError`, which the provider re-raises as a `ProviderError` carrying the real reason rather than a blanket "not part of a plugin". Two plugins claiming one qualified name are refused too — deduping the clash away would drop a declared skill. All of it is `is_retryable=False`: these never become valid on a retry, and a checkout path containing "connection" would otherwise trip the retryability heuristic. Note providers are constructed lazily, so this surfaces when the first agent on this provider runs, **not** at `conductor validate`. - `setting_sources=[]` **by default**, for the same reason `strict_mcp_config=True` is unconditional a few lines away — but opt-in per workflow via `runtime.provider.setting_sources` (`user`/`project`/`local`, issue #501). Left unset, the CLI loads user, project, and local settings, which between them bring in ambient skills, `CLAUDE.md`, and hooks the workflow never declared, varying by machine and launch directory. Conductor surfaces instruction files through its own opt-in `--workspace-instructions`; settings and hooks have no equivalent. `skills=[]` and `skills=None` are **not** interchangeable upstream: `None` means "CLI defaults apply", and setting `skills` while leaving `setting_sources` at `None` makes the SDK re-default it to `["user", "project"]` — so the two options are coupled and dropping either re-opens the issue. The `[]` is also invisible in argv (it travels in the SDK's `initialize` control request), which is why the argv-based tests are paired with options-level assertions. The list is a context filter, not a sandbox: undeclared skills are hidden from the model's listing and rejected by the `Skill` tool, but their files stay readable. Opting a workflow in (`runtime.provider.setting_sources: [project]`, the motivating case being an agent whose `working_dir` is a *target* repo shipping its own `.claude/skills` — the CLI has `--plugin-dir` but no `--skill-dir`) loads that tier **with its hooks**, so it is only for repositories trusted as much as the workflow itself, and the field is workflow-global while `working_dir` is per agent. Two couplings follow: `_resolve_skill_filter` resolves `skills` to `"all"` when a tier is enabled and the workflow named none itself (tier-discovered skills never pass through `skill_names`, so `[]` would load the repo's skills and then hide every one), and a per-agent `skills: []` opts that agent out of the tier entirely — hooks included — keeping it the one opt-out. diff --git a/src/conductor/providers/claude_agent_sdk.py b/src/conductor/providers/claude_agent_sdk.py index d53e1435..3803864e 100644 --- a/src/conductor/providers/claude_agent_sdk.py +++ b/src/conductor/providers/claude_agent_sdk.py @@ -742,22 +742,27 @@ def __init__( # directory, so one key under two directories is two sessions. self._session_ids: dict[tuple[str, str], str] = {} # settings_dir values already warned about for having no `project` - # tier. Keyed by the resolved DIRECTORY, and that choice is a trade: + # tier, keyed by `(resolved directory, cause)`: # - # - Not the agent name, alone or paired with the directory: the engine - # renames a for_each member per item (`[]`, - # engine/workflow.py), so any name-bearing key emits one line per - # item -- the very case latching exists to prevent. + # - The directory, not the agent name: the engine renames a for_each + # member per item (`[]`, engine/workflow.py), so any + # name-bearing key emits one line per item -- the very case latching + # exists to prevent. All members of one loop share a directory and a + # cause, so they collapse to one line. # - Not a bare flag: `settings_dir` is Jinja-rendered per execution, so # one agent can name several directories across loop-backs, and each # is a distinct grant the operator needs told about. - # - The cost, accepted: two differently-named agents naming the SAME - # directory warn once, naming only the first. The actionable content - # is the directory and the remedy is workflow-global, so the second - # line would add nothing the first did not say. + # - Plus the cause, because the remedy below depends on it: two agents + # can name the same directory for different reasons, and one line + # would prescribe a fix that is wrong for the other. Bounded at two + # lines per directory. + # + # The residual cost, accepted: two agents naming the same directory + # for the SAME reason warn once, naming only the first. The remedy is + # then identical for both, so the second line would add nothing. # # Matches the `_warned` convention in claude.py and engine/workflow.py. - self._settings_dir_tier_warned: set[str] = set() + self._settings_dir_tier_warned: set[tuple[str, bool]] = set() self._resume_session_ids: dict[tuple[str, str], str] = {} # Slots currently executing, so a second execution cannot resume a # session the first still has open — see :meth:`_claim_session_slot`. @@ -988,21 +993,32 @@ async def _execute_session( # calls the static validator, so without this the run is silent about # a no-op the author is relying on. Warned rather than raised, matching # validate's own choice: the workflow is not wrong, just ineffective. + opted_out = agent.skills == [] if ( agent.settings_dir is not None and "project" not in effective_sources - and agent.settings_dir not in self._settings_dir_tier_warned + and (agent.settings_dir, opted_out) not in self._settings_dir_tier_warned ): - self._settings_dir_tier_warned.add(agent.settings_dir) + self._settings_dir_tier_warned.add((agent.settings_dir, opted_out)) # The remedy depends on the cause, as it does in # config/validator.py: telling an author to add 'project' when # their own `skills: []` is what zeroed the tier sends them to add # a value that is already there, and the warning keeps firing. + # + # The other arm covers two of validator.py's causes at once -- a + # missing tier, and a per-agent provider override, where the tier + # cannot be enabled at all because the schema accepts + # `setting_sources` only when `runtime.provider` is + # 'claude-agent-sdk'. The provider does not know the + # workflow-level provider name, so the wording names the + # requirement rather than prescribing an edit that would be + # refused on that path. remedy = ( "This agent's own 'skills: []' opts it out of the settings tiers " "entirely; remove it to let the tier apply" - if agent.skills == [] - else "Add 'project' to runtime.provider.setting_sources" + if opted_out + else "Enable the 'project' tier via runtime.provider.setting_sources, " + "which requires runtime.provider itself to be 'claude-agent-sdk'" ) logger.warning( "Agent '%s' sets settings_dir=%r but its session does not enable the " diff --git a/tests/test_providers/test_claude_agent_sdk.py b/tests/test_providers/test_claude_agent_sdk.py index 784d2e63..64da3614 100644 --- a/tests/test_providers/test_claude_agent_sdk.py +++ b/tests/test_providers/test_claude_agent_sdk.py @@ -3479,8 +3479,79 @@ async def remedy_for(sources: list[str] | None, skills: list[str] | None) -> str return hits[0].message no_tier = await remedy_for(None, None) - assert "Add 'project' to runtime.provider.setting_sources" in no_tier + assert "Enable the 'project' tier via runtime.provider.setting_sources" in no_tier opted_out = await remedy_for(["project"], []) assert "'skills: []' opts it out" in opted_out - assert "Add 'project'" not in opted_out, "advice is a no-op for this cause" + assert "Enable the 'project' tier" not in opted_out, "advice is a no-op for this cause" + + @pytest.mark.asyncio + async def test_two_causes_on_one_directory_both_warn( + self, tmp_path: Path, caplog: pytest.LogCaptureFixture + ) -> None: + """The latch key includes the cause, because the remedy depends on it. + + Keying on the directory alone dedupes a for_each correctly, but two + agents can name one directory for different reasons -- and then a + single line prescribes a fix that is wrong for the agent it does not + name. The pair keeps the for_each collapse (all members share a cause) + while letting a differently-caused agent through. + """ + target = tmp_path / "repo" + target.mkdir() + + async def fake_query(**kwargs): + yield _result(result="ok") + + with patch("conductor.providers.claude_agent_sdk.query", fake_query): + provider = ClaudeAgentSdkProvider() + with caplog.at_level(logging.WARNING): + await provider.execute( + agent=AgentDef( + name="opted_out", prompt="hi", settings_dir=str(target), skills=[] + ), + context={}, + rendered_prompt="hi", + ) + await provider.execute( + agent=AgentDef(name="no_tier", prompt="hi", settings_dir=str(target)), + context={}, + rendered_prompt="hi", + ) + + hits = [r for r in caplog.records if "no skills are discovered" in r.message] + assert [r.args[0] for r in hits] == ["opted_out", "no_tier"], [r.args[0] for r in hits] + assert "'skills: []' opts it out" in hits[0].message + assert "Enable the 'project' tier" in hits[1].message + + @pytest.mark.asyncio + async def test_the_no_tier_remedy_does_not_prescribe_a_rejected_edit( + self, tmp_path: Path, caplog: pytest.LogCaptureFixture + ) -> None: + """An agent overriding its provider cannot enable the tier at all. + + ``providers/registry.py`` forwards structured settings only to the + matching provider, so an agent overriding to ``claude-agent-sdk`` + under a different ``runtime.provider`` reaches the provider with no + ``setting_sources``. The schema rejects ``setting_sources`` unless + ``runtime.provider`` is itself ``claude-agent-sdk``, so the remedy + must not tell that author to just add it -- it names the requirement. + """ + target = tmp_path / "repo" + target.mkdir() + + async def fake_query(**kwargs): + yield _result(result="ok") + + with patch("conductor.providers.claude_agent_sdk.query", fake_query): + provider = ClaudeAgentSdkProvider() + with caplog.at_level(logging.WARNING): + await provider.execute( + agent=AgentDef(name="judge", prompt="hi", settings_dir=str(target)), + context={}, + rendered_prompt="hi", + ) + + hits = [r for r in caplog.records if "no skills are discovered" in r.message] + assert hits + assert "requires runtime.provider itself to be 'claude-agent-sdk'" in hits[0].message From c67c4fbf45da7dff29856721a89ab8d8c4b6a6b0 Mon Sep 17 00:00:00 2001 From: Chris Throup Date: Wed, 9 Sep 2026 13:24:52 +0200 Subject: [PATCH 10/10] docs(claude-agent-sdk): record the tier warning's limits and pin its measurement No behaviour change; three gaps a review pass found in the prose around the settings_dir tier warning. The opt-out remedy assumes the `project` tier is otherwise present. An agent with `skills: []` under a `user`/`local`-only tier is told to remove the opt-out, which is necessary but not sufficient, so that author reaches a working config in two warnings rather than one. Both layers say the same thing, so there is no divergence -- but the limit was undocumented. Distinguishing it would need a third cause value and a wider latch key, to serve a combination requiring two unusual settings at once, so AGENTS.md records the limit instead. Two test docstrings described the code as it was rather than as it is: the latch test named only the directory half of a key that is now `(directory, cause)`, and the no-tier-remedy test claimed to exercise the per-agent provider override, which it cannot -- the provider never receives the workflow-level provider name, which is exactly why one shared remedy arm is right. That docstring now says what the test can and cannot pin, and points at the validate-time test that does cover the override path. The filesystem-grant measurement is now pinned to `claude` CLI 2.1.263. Later builds dropped `default` from `--permission-mode`'s accepted names, so the stated reproduction needs the version named. Conductor never passes that mode explicitly, so nothing in the code is affected. --- AGENTS.md | 2 +- docs/workflow-syntax.md | 8 +++-- tests/test_providers/test_claude_agent_sdk.py | 29 ++++++++++++------- 3 files changed, 25 insertions(+), 14 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 94132711..9710bcd0 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -432,7 +432,7 @@ Conductor: - The path is passed **verbatim** — `WorkflowEngine._resolve_agent_working_dir` has already rendered, absolutized, normalised, and existence-checked it, and re-resolving here would collapse the symlink aliases the engine preserves on purpose. The `ClaudeAgentOptions(...)` construction lives **inside** `execute`'s `try` so the `os.getcwd()` fallback can't escape as a bare `OSError` when the process cwd has been deleted (`copilot.py` resolves its cwd inside its try for the same reason). - There is no provider-side `is_dir()` guard: a directory that vanishes after the engine's check surfaces as the SDK's `CLIConnectionError("Working directory does not exist: ")`, wrapped in `ProviderError`. That is only defensible because `_classify_startup_failure` special-cases it — `CLIConnectionError` otherwise yields firewall/binary advice and `is_retryable=True`, which is wrong for all three launch failures (missing dir; path is a file → `ENOTDIR`; unreadable dir → `EACCES`; the latter two reach the SDK's generic "Failed to start Claude Code" arm, not its dedicated one). - Knock-on effects: cwd is the project key for the CLI's on-disk transcript directory, and it is where a project `.mcp.json` and `.claude/` tree would be looked for. The unconditional `strict_mcp_config=True` stops a `.mcp.json` there from injecting undeclared servers, and `setting_sources=[]` (empty by default; see **Skills** below) stops the CLI loading `CLAUDE.md`, project settings, and hooks from it — so cwd no longer drags ambient instructions in unless the workflow opts in via `runtime.provider.setting_sources`, which is exactly a request to load them from that directory. `add_dirs` (the SDK's `--add-dir` passthrough) is a separate axis, carrying the per-agent `settings_dir` and nothing else — see **Target-repository skills** below. -- **Target-repository skills** (`settings_dir`): the per-agent `AgentDef.settings_dir` is the **only** source of `ClaudeAgentOptions.add_dirs`, and it selects which directory's *project* settings tier contributes skills. It exists because cwd was doing two conflicting jobs on this provider. The CLI supports the MCP Roots protocol and advertises exactly one root — its cwd — so `@modelcontextprotocol/server-filesystem` **discards the directories in its own argv** and permits cwd alone; cwd is simultaneously what the `project` tier resolves against. Narrowing cwd onto a target repository to reach its skills therefore narrowed the agent's only MCP root below any sibling path the step still had to read. Do not derive `add_dirs` from a server's directory arguments to compensate: `--add-dir` takes no part in Roots negotiation, it widens the CLI's own file tools, and doing so would also silently widen skill discovery to directories the author named as data. What `add_dirs` *does* do, measured: a named directory's `.claude/skills` become listed and invocable with cwd elsewhere entirely — and only those, not `CLAUDE.md`, `.claude/rules/*.md`, `.claude/settings.json` (so no `env` and no `hooks` — the hooks negative is measured with a side-effect probe whose control fires) or `.claude/agents`, which all keep following cwd. That makes it the *skills third* of what a cwd-resolved `project` tier loads, not a replacement for it. It carries a second, **unconditional** effect the skills framing hides: `add_dirs` is "additional directories Claude can access" per the SDK's own contract, so a `settings_dir` widens the model's built-in `Read`/`Edit`/`Bash` to that tree with no settings tier enabled at all (measured at `permission_mode: "default"` with `setting_sources` unset: a read outside cwd is refused without it and succeeds with it; an agent omitting `tools:` runs under `bypassPermissions`, where the grant is unobservable because reads already succeed). It does not widen what an MCP server permits. `capabilities.py::settings_dir` gates the field and `config/validator.py` errors against a provider that cannot apply it — enforced **twice**, with `AgentExecutor._reject_unsupported_settings_dir` repeating it at run time, because `conductor run` never calls the static validator — warning when the agent's session will not enable the `project` tier (also twice: `config/validator.py` at validate, and `claude_agent_sdk.py::execute` at run time, since the filesystem grant applies even when the skills half no-ops; the run-time half is latched per `(resolved directory, cause)` — not per agent, because a `for_each` member is renamed per item and would otherwise warn once per iteration, and not per directory alone, because the remedy differs by cause and one line would prescribe a fix wrong for the agent it does not name; two agents sharing a directory *and* a cause do collapse to one line). `WorkflowEngine._resolve_agent_directory` resolves both fields so they cannot drift, and `settings_dir` is per-agent only (no `runtime.` counterpart: the repository whose conventions apply is what varies between steps). `tests/test_integration/test_mcp_roots_negotiation.py` pins the negotiation rule itself against the real server with no LLM — two runs differing only in whether the client advertises `roots` — since every option here rests on it (marked `real_api`: it fetches the server from npm and pins *upstream's* behaviour, not Conductor's). The resolved value is emitted on `agent_started` / `parallel_agent_started` / `for_each_agent_started` alongside `working_dir`, because it is a trust decision — another repository's conventions plus a filesystem grant — and one the run output would otherwise never mention. +- **Target-repository skills** (`settings_dir`): the per-agent `AgentDef.settings_dir` is the **only** source of `ClaudeAgentOptions.add_dirs`, and it selects which directory's *project* settings tier contributes skills. It exists because cwd was doing two conflicting jobs on this provider. The CLI supports the MCP Roots protocol and advertises exactly one root — its cwd — so `@modelcontextprotocol/server-filesystem` **discards the directories in its own argv** and permits cwd alone; cwd is simultaneously what the `project` tier resolves against. Narrowing cwd onto a target repository to reach its skills therefore narrowed the agent's only MCP root below any sibling path the step still had to read. Do not derive `add_dirs` from a server's directory arguments to compensate: `--add-dir` takes no part in Roots negotiation, it widens the CLI's own file tools, and doing so would also silently widen skill discovery to directories the author named as data. What `add_dirs` *does* do, measured: a named directory's `.claude/skills` become listed and invocable with cwd elsewhere entirely — and only those, not `CLAUDE.md`, `.claude/rules/*.md`, `.claude/settings.json` (so no `env` and no `hooks` — the hooks negative is measured with a side-effect probe whose control fires) or `.claude/agents`, which all keep following cwd. That makes it the *skills third* of what a cwd-resolved `project` tier loads, not a replacement for it. It carries a second, **unconditional** effect the skills framing hides: `add_dirs` is "additional directories Claude can access" per the SDK's own contract, so a `settings_dir` widens the model's built-in `Read`/`Edit`/`Bash` to that tree with no settings tier enabled at all (measured against `claude` CLI 2.1.263 at `permission_mode: "default"` with `setting_sources` unset: a read outside cwd is refused without it and succeeds with it; an agent omitting `tools:` runs under `bypassPermissions`, where the grant is unobservable because reads already succeed). It does not widen what an MCP server permits. `capabilities.py::settings_dir` gates the field and `config/validator.py` errors against a provider that cannot apply it — enforced **twice**, with `AgentExecutor._reject_unsupported_settings_dir` repeating it at run time, because `conductor run` never calls the static validator — warning when the agent's session will not enable the `project` tier (also twice: `config/validator.py` at validate, and `claude_agent_sdk.py::execute` at run time, since the filesystem grant applies even when the skills half no-ops; the run-time half is latched per `(resolved directory, cause)` — not per agent, because a `for_each` member is renamed per item and would otherwise warn once per iteration, and not per directory alone, because the remedy differs by cause and one line would prescribe a fix wrong for the agent it does not name; two agents sharing a directory *and* a cause do collapse to one line). The opt-out remedy assumes the `project` tier is otherwise present: an agent with `skills: []` under a `user`/`local`-only tier is told to remove the opt-out, which is necessary but not sufficient, so that author reaches a working config in two steps rather than one. Deliberate -- distinguishing it needs a third cause value and a wider latch key to serve a combination requiring two unusual settings at once. `WorkflowEngine._resolve_agent_directory` resolves both fields so they cannot drift, and `settings_dir` is per-agent only (no `runtime.` counterpart: the repository whose conventions apply is what varies between steps). `tests/test_integration/test_mcp_roots_negotiation.py` pins the negotiation rule itself against the real server with no LLM — two runs differing only in whether the client advertises `roots` — since every option here rests on it (marked `real_api`: it fetches the server from npm and pins *upstream's* behaviour, not Conductor's). The resolved value is emitted on `agent_started` / `parallel_agent_started` / `for_each_agent_started` alongside `working_dir`, because it is a trust decision — another repository's conventions plus a filesystem grant — and one the run output would otherwise never mention. - **Skills** (issue #352): `supports_native_skills=True`. Skills are enabled through the SDK, not prompt injection, and three options move together in `execute`: - `plugins=[{"type": "local", "path": }]` + `skills=[":"]`. The SDK has no skill-*directory* surface, so `_resolve_skill_plugins` maps each directory back to the plugin that owns it via `skills/registry.py::resolve_skill_plugin`. That resolver is deliberately strict, because every one of these mistakes otherwise produces a name the CLI silently resolves to nothing: it bounds the upward walk (`_PLUGIN_SEARCH_DEPTH`), requires the skill to actually live under the candidate's `skills/` directory, requires `SKILL.md` to exist and its frontmatter `name` to equal the directory name (the CLI resolves by frontmatter name; Conductor sends the directory name), and rejects names outside `[A-Za-z0-9_.-]+` since they are joined into a comma-delimited `--allowedTools` value. A skill under no plugin root returns `None`; a plugin that is present but unusable raises `SkillPluginError`, which the provider re-raises as a `ProviderError` carrying the real reason rather than a blanket "not part of a plugin". Two plugins claiming one qualified name are refused too — deduping the clash away would drop a declared skill. All of it is `is_retryable=False`: these never become valid on a retry, and a checkout path containing "connection" would otherwise trip the retryability heuristic. Note providers are constructed lazily, so this surfaces when the first agent on this provider runs, **not** at `conductor validate`. - `setting_sources=[]` **by default**, for the same reason `strict_mcp_config=True` is unconditional a few lines away — but opt-in per workflow via `runtime.provider.setting_sources` (`user`/`project`/`local`, issue #501). Left unset, the CLI loads user, project, and local settings, which between them bring in ambient skills, `CLAUDE.md`, and hooks the workflow never declared, varying by machine and launch directory. Conductor surfaces instruction files through its own opt-in `--workspace-instructions`; settings and hooks have no equivalent. `skills=[]` and `skills=None` are **not** interchangeable upstream: `None` means "CLI defaults apply", and setting `skills` while leaving `setting_sources` at `None` makes the SDK re-default it to `["user", "project"]` — so the two options are coupled and dropping either re-opens the issue. The `[]` is also invisible in argv (it travels in the SDK's `initialize` control request), which is why the argv-based tests are paired with options-level assertions. The list is a context filter, not a sandbox: undeclared skills are hidden from the model's listing and rejected by the `Skill` tool, but their files stay readable. Opting a workflow in (`runtime.provider.setting_sources: [project]`, the motivating case being an agent whose `working_dir` is a *target* repo shipping its own `.claude/skills` — the CLI has `--plugin-dir` but no `--skill-dir`) loads that tier **with its hooks**, so it is only for repositories trusted as much as the workflow itself, and the field is workflow-global while `working_dir` is per agent. Two couplings follow: `_resolve_skill_filter` resolves `skills` to `"all"` when a tier is enabled and the workflow named none itself (tier-discovered skills never pass through `skill_names`, so `[]` would load the repo's skills and then hide every one), and a per-agent `skills: []` opts that agent out of the tier entirely — hooks included — keeping it the one opt-out. diff --git a/docs/workflow-syntax.md b/docs/workflow-syntax.md index dfab7af0..a7763744 100644 --- a/docs/workflow-syntax.md +++ b/docs/workflow-syntax.md @@ -471,9 +471,11 @@ Measured against the CLI: > maps to the SDK's `add_dirs`, whose own contract is *"additional directories > Claude can access beyond the current working directory"* — so naming a > directory here widens the model's built-in file tools to that tree whether or -> not any settings tier is enabled. Measured at `permission_mode: "default"` -> with `setting_sources` unset: a read outside cwd is refused without -> `settings_dir` and succeeds with it. Note an agent that omits `tools:` runs +> not any settings tier is enabled. Measured against `claude` CLI 2.1.263 at +> `permission_mode: "default"` with `setting_sources` unset: a read outside +> cwd is refused without `settings_dir` and succeeds with it. (Later CLI +> builds no longer accept that mode by name; Conductor never passes it +> explicitly, so the reproduction needs the version above.) Note an agent that omits `tools:` runs > under `bypassPermissions`, where reads already succeed everywhere, so the > grant only becomes observable once permissions are in play. > diff --git a/tests/test_providers/test_claude_agent_sdk.py b/tests/test_providers/test_claude_agent_sdk.py index 64da3614..f3cb17ee 100644 --- a/tests/test_providers/test_claude_agent_sdk.py +++ b/tests/test_providers/test_claude_agent_sdk.py @@ -3397,7 +3397,7 @@ async def fake_query(**kwargs): assert bool(hits) is expect_warning, [r.message for r in caplog.records] @pytest.mark.asyncio - async def test_the_tier_warning_is_latched_per_directory( + async def test_the_tier_warning_is_latched_per_directory_and_cause( self, tmp_path: Path, caplog: pytest.LogCaptureFixture ) -> None: """Once per directory, and specifically once across a for_each. @@ -3408,7 +3408,9 @@ async def test_the_tier_warning_is_latched_per_directory( so a name-keyed latch emits one line per item -- the exact case latching exists to prevent. Not a bare flag either: a second agent naming a *different* directory must still be reported, since naming - the directory is the point of the warning. + the directory is the point of the warning. The key is additionally + paired with the cause, since the remedy depends on it -- see + :meth:`test_two_causes_on_one_directory_both_warn`. """ target = tmp_path / "repo" other = tmp_path / "other" @@ -3528,14 +3530,21 @@ async def fake_query(**kwargs): async def test_the_no_tier_remedy_does_not_prescribe_a_rejected_edit( self, tmp_path: Path, caplog: pytest.LogCaptureFixture ) -> None: - """An agent overriding its provider cannot enable the tier at all. - - ``providers/registry.py`` forwards structured settings only to the - matching provider, so an agent overriding to ``claude-agent-sdk`` - under a different ``runtime.provider`` reaches the provider with no - ``setting_sources``. The schema rejects ``setting_sources`` unless - ``runtime.provider`` is itself ``claude-agent-sdk``, so the remedy - must not tell that author to just add it -- it names the requirement. + """The no-tier arm must not prescribe an edit the schema would reject. + + This arm serves two of ``config/validator.py``'s causes at once: a + missing tier, and a per-agent ``provider: claude-agent-sdk`` override + under a different ``runtime.provider``, where ``factory.py`` forwards + no ``setting_sources`` and the schema would then reject adding it. + + The provider cannot tell those two apart -- it never receives the + workflow-level provider name, only ``setting_sources`` -- which is + why one shared arm is the right design and why this test can pin + only the wording that is true of both. The override path itself is + covered at validate time, where the config *is* visible: + ``test_config/test_settings_dir_schema.py:: + TestProjectTierWarningCauses:: + test_provider_override_does_not_advise_the_impossible``. """ target = tmp_path / "repo" target.mkdir()