From 1d5ac9a959f721e4eb30c41af37b7d97e744c190 Mon Sep 17 00:00:00 2001 From: Chris Throup Date: Mon, 7 Sep 2026 16:16:58 +0200 Subject: [PATCH 1/4] feat(claude-agent-sdk): select the project settings tier with settings_dir An agent's cwd was doing two unrelated jobs on this provider, and they conflict. The `claude` 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` settings tier resolves against. Pointing `working_dir` at 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; widening it back lost the repository's conventions. `_stdio_path_args` existed to prevent exactly this, forwarding every stdio server's directory args into `add_dirs` with a docstring asserting it "restores the declared scope". It cannot: `--add-dir` takes no part in Roots negotiation, it widens the CLI's own file tools. Measured, cwd alone is the effective allowlist whether or not the declared roots are also passed. That helper is removed rather than documented as ineffective, because it also had a live cost -- deriving skill discovery from server arguments grants conventions from directories the author named as data. Add a per-agent `settings_dir`, which is now the only source of `add_dirs`. Skills are discovered from cwd *and* from `settings_dir`, so cwd can stay wide. Measured against the real CLI and through the SDK surface the provider uses: a named directory contributes its `.claude/skills` (listed and invocable) and nothing else -- not CLAUDE.md, `.claude/settings.json` (so no env, no hooks) or `.claude/agents`, which keep following cwd. That asymmetry cuts favourably: a target repository's skills arrive without its hooks. `WorkflowEngine._resolve_agent_directory` resolves both fields so they cannot drift. `settings_dir` is per-agent only -- the repository whose conventions apply is what varies between steps -- and is rejected on every step type with no LLM session, where accepting it silently would suggest conventions had loaded when none had. Tests: the negotiation rule itself is pinned against the real server with no LLM (`test_mcp_roots_negotiation.py`), two runs differing only in whether the client advertises `roots`, since every option here rests on that precedence. `grep add_dir tests/` previously returned nothing, which is the most direct explanation for how an ineffective mitigation shipped and stayed. Co-Authored-By: Claude Opus 5 --- AGENTS.md | 3 +- docs/providers/experimental.md | 2 +- docs/workflow-syntax.md | 74 +++++++ examples/claude-agent-sdk-settings-dir.yaml | 134 ++++++++++++ src/conductor/config/schema.py | 75 ++++++- src/conductor/engine/workflow.py | 79 ++++--- src/conductor/providers/claude_agent_sdk.py | 42 ++-- tests/test_config/test_set_schema.py | 1 + tests/test_config/test_settings_dir_schema.py | 87 ++++++++ tests/test_engine/test_workflow.py | 133 ++++++++++++ .../test_mcp_roots_negotiation.py | 193 ++++++++++++++++++ tests/test_providers/test_claude_agent_sdk.py | 170 +++++++++++++++ 12 files changed, 940 insertions(+), 53 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 ead46d01..4c619547 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -422,7 +422,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 the unconditional `setting_sources=[]` (see **Skills** below) stops the CLI loading `CLAUDE.md`, project settings, and hooks from it — so cwd no longer drags ambient instructions in. `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 the unconditional `setting_sources=[]` (see **Skills** below) stops the CLI loading `CLAUDE.md`, project settings, and hooks from it — so cwd no longer drags ambient instructions in. `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`, PDA-95): 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. An earlier fork-local `_stdio_path_args` helper derived `add_dirs` from every stdio server's directory arguments, with a docstring asserting this "restores the declared scope"; it cannot — `--add-dir` takes no part in Roots negotiation, it widens the CLI's own file tools. That helper is **removed**; do not reintroduce deriving `add_dirs` from server arguments, which 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/settings.json` (so no `env`, no `hooks`) or `.claude/agents`, which all keep following cwd. `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=[]` **unconditionally**, for the same reason `strict_mcp_config=True` is unconditional a few lines away. 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 but their files stay readable. diff --git a/docs/providers/experimental.md b/docs/providers/experimental.md index 2ff58139..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 `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` is pinned empty as of [#352](https://github.com/microsoft/conductor/issues/352) so ambient instructions, settings, hooks, and skills are not inherited. 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 b99e51ca..d3f0c831 100644 --- a/docs/workflow-syntax.md +++ b/docs/workflow-syntax.md @@ -400,6 +400,80 @@ 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 + +A directory named here contributes its `.claude/skills` and nothing else. +`CLAUDE.md`, `.claude/settings.json` (so `env` and `hooks`) and +`.claude/agents` all continue to follow cwd. Instructions therefore still need +`working_dir` or `--workspace-instructions`; this field is the skills half +only. + +That asymmetry is a measured property of the CLI, not a design choice +Conductor makes, and it cuts favourably: enabling a tier for a target +repository brings that repository's skills without also running its hooks. + +#### 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 743df14e..ef69a070 100644 --- a/src/conductor/config/schema.py +++ b/src/conductor/config/schema.py @@ -1358,6 +1358,49 @@ 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. + + Only meaningful on ``claude-agent-sdk`` agents in a workflow that sets + ``runtime.provider.setting_sources`` (see + :attr:`ProviderSettings.setting_sources`); ignored by every other + provider. 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/human_gate/questions/workflow step types. + + 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/settings.json`` (so no ``env`` and no + ``hooks``), not ``.claude/agents``, all of which continue to follow cwd. + Instructions therefore still need ``working_dir`` (or + ``--workspace-instructions``); this field is the skills half only. + + 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). @@ -2007,6 +2050,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": @@ -2070,6 +2115,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": @@ -2103,6 +2150,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: @@ -2169,6 +2218,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'") @@ -2192,6 +2243,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: @@ -2265,6 +2318,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: @@ -2339,6 +2394,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: @@ -2676,11 +2733,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/engine/workflow.py b/src/conductor/engine/workflow.py index 7fc23b5e..7f9b29d3 100644 --- a/src/conductor/engine/workflow.py +++ b/src/conductor/engine/workflow.py @@ -610,28 +610,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(): @@ -641,16 +636,52 @@ 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. + + ``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. diff --git a/src/conductor/providers/claude_agent_sdk.py b/src/conductor/providers/claude_agent_sdk.py index b247b8d0..4b787e61 100644 --- a/src/conductor/providers/claude_agent_sdk.py +++ b/src/conductor/providers/claude_agent_sdk.py @@ -302,23 +302,6 @@ def _server_tool_filters(mcp_servers: dict[str, Any]) -> dict[str, set[str]]: } -def _stdio_path_args(mcp_servers: dict[str, Any]) -> list[str]: - """Absolute directory args of stdio MCP servers, for ``add_dirs``. - - The CLI's MCP Roots override a server's own path args with cwd + - ``--add-dir``, so a server declared with two directories is rooted at one - and the rest denied. Forwarding these restores the declared scope. - """ - paths: list[str] = [] - for config in mcp_servers.values(): - if config.get("type") != "stdio": - continue - for arg in config.get("args") or []: - if isinstance(arg, str) and arg.startswith("/") and Path(arg).is_dir(): - paths.append(arg) - return sorted(set(paths)) - - def _resolve_skill_filter(skill_names: list[str], setting_sources: list[str]) -> list[str] | str: """Value for ``ClaudeAgentOptions.skills`` — the name-level skill filter. @@ -975,9 +958,6 @@ async def _execute_session( ) server_denied = _server_filter_denials(enumerated_mcp_tools, self._server_tool_filters) - # Restores the MCP scope the CLI's Roots would otherwise collapse to cwd. - add_dirs = _stdio_path_args({**self._mcp_servers, **(extra_mcp_servers or {})}) - sdk_tools, permission_mode, allowed_tools, disallowed_tools = self._resolve_tool_config( tools, agent, @@ -1012,8 +992,26 @@ 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, - # MCP Roots collapse a server's own path args to cwd without these. - add_dirs=add_dirs, + # The authored ``settings_dir`` and nothing else. An earlier + # version derived this from the directory args of every stdio MCP + # server, believing it restored a scope those args had lost. It + # cannot: 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 argv + # directories are discarded by the server itself. ``--add-dir`` is + # not part of Roots negotiation and so cannot put them back; 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. + # + # What it does do is make 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/settings.json or .claude/agents, which stay + # with cwd). That is the one job it is used for here, so the value + # is the author's ``settings_dir`` rather than a guess derived + # from server arguments. + 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..9f1373ef --- /dev/null +++ b/tests/test_config/test_settings_dir_schema.py @@ -0,0 +1,87 @@ +"""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 + +import pytest +from pydantic import ValidationError + +from conductor.config.schema import AgentDef, GateOption + + +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") diff --git a/tests/test_engine/test_workflow.py b/tests/test_engine/test_workflow.py index a24d99e6..e6ee2895 100644 --- a/tests/test_engine/test_workflow.py +++ b/tests/test_engine/test_workflow.py @@ -3817,6 +3817,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( @@ -3833,6 +3834,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, @@ -3856,6 +3858,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: @@ -3874,6 +3877,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")], ), @@ -4615,3 +4619,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..e149e485 --- /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. + +Conductor's ``claude-agent-sdk`` provider once derived +``ClaudeAgentOptions.add_dirs`` from the directory arguments of every stdio +MCP server, on the stated belief that forwarding them "restores the declared +scope" the CLI would otherwise collapse. It does not, 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 4e798a7a..2bd4e2ac 100644 --- a/tests/test_providers/test_claude_agent_sdk.py +++ b/tests/test_providers/test_claude_agent_sdk.py @@ -3228,3 +3228,173 @@ async def test_plugin_servers_are_not_cached_as_the_workflow_set(self) -> None: provider._enumerated_mcp_tools = {"docs__read"} # Passing an explicit set bypasses the cache rather than overwriting it. assert await provider._enumerate_mcp_tools() == {"docs__read"} + + +class TestSettingsDirAddDirs: + """``settings_dir`` is the only source of ``ClaudeAgentOptions.add_dirs``. + + This class exists because the field it replaced had none. ``add_dirs`` was + previously derived from the directory arguments of every stdio MCP server, + on the stated belief that forwarding them "restores the declared scope" a + server had lost. It does not, and nothing caught that: a + ``grep add_dir tests/`` found no matches at all, so an ineffective + mitigation shipped and stayed while reading the source suggested the + problem was handled. + + The mechanism, measured rather than reasoned about (see + ``TestMcpRootIsCwdNotServerArgs`` below, which pins it 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, which is why deriving it from server args could never + work. + + 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_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 13c746e65ecc16cdb6b06d3bd9359696096fd6b6 Mon Sep 17 00:00:00 2001 From: Chris Throup Date: Mon, 7 Sep 2026 16:26:55 +0200 Subject: [PATCH 2/4] docs(claude-agent-sdk): settings_dir carries skills only, not rules or hooks Measured, having previously only tested skills: a directory named via `add_dirs` contributes its `.claude/skills` and nothing else. `CLAUDE.md`, `.claude/rules/*.md` (symlinked or not), `.claude/settings.json` (so `env` and `hooks`) and `.claude/agents` all follow cwd. Two runs against the real CLI, differing only in cwd, with a fixture whose `.claude/rules` is a symlink to `.agents/rules` and whose `CLAUDE.md` never mentions it: cwd=repo, no --add-dir -> RULE=set HOOK=set CLAUDEMD=set cwd=wide, --add-dir repo -> RULE=NONE HOOK=NONE CLAUDEMD=NONE, skill=yes So `settings_dir` is the *skills portion* of a project tier, not a cwd-independent way to load one, and it does not compose with `working_dir` into "everything, anywhere": an agent needing a repository's rules or instructions as well as a cwd wide enough for its MCP servers cannot get both from these fields, because one directory cannot be narrow and wide at once. The earlier docs named settings.json and agents but omitted rules, which is the omission most likely to mislead someone choosing between the two fields. Adds a test pinning that a `settings_dir` is never promoted into `cwd` to widen what it loads -- that would hand the agent the narrow directory as its sole MCP root, the exact defect the field exists to avoid. Co-Authored-By: Claude Opus 5 --- AGENTS.md | 2 +- docs/workflow-syntax.md | 28 +++++++++---- src/conductor/config/schema.py | 15 +++++-- src/conductor/providers/claude_agent_sdk.py | 6 ++- tests/test_providers/test_claude_agent_sdk.py | 41 +++++++++++++++++++ 5 files changed, 77 insertions(+), 15 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 4c619547..b521ba2b 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -423,7 +423,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 the unconditional `setting_sources=[]` (see **Skills** below) stops the CLI loading `CLAUDE.md`, project settings, and hooks from it — so cwd no longer drags ambient instructions in. `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`, PDA-95): 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. An earlier fork-local `_stdio_path_args` helper derived `add_dirs` from every stdio server's directory arguments, with a docstring asserting this "restores the declared scope"; it cannot — `--add-dir` takes no part in Roots negotiation, it widens the CLI's own file tools. That helper is **removed**; do not reintroduce deriving `add_dirs` from server arguments, which 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/settings.json` (so no `env`, no `hooks`) or `.claude/agents`, which all keep following cwd. `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`, PDA-95): 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. An earlier fork-local `_stdio_path_args` helper derived `add_dirs` from every stdio server's directory arguments, with a docstring asserting this "restores the declared scope"; it cannot — `--add-dir` takes no part in Roots negotiation, it widens the CLI's own file tools. That helper is **removed**; do not reintroduce deriving `add_dirs` from server arguments, which 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`, no `hooks`) 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. `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=[]` **unconditionally**, for the same reason `strict_mcp_config=True` is unconditional a few lines away. 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 but their files stay readable. diff --git a/docs/workflow-syntax.md b/docs/workflow-syntax.md index d3f0c831..03e511e9 100644 --- a/docs/workflow-syntax.md +++ b/docs/workflow-syntax.md @@ -449,14 +449,26 @@ agents: #### What it does and does not carry A directory named here contributes its `.claude/skills` and nothing else. -`CLAUDE.md`, `.claude/settings.json` (so `env` and `hooks`) and -`.claude/agents` all continue to follow cwd. Instructions therefore still need -`working_dir` or `--workspace-instructions`; this field is the skills half -only. - -That asymmetry is a measured property of the CLI, not a design choice -Conductor makes, and it cuts favourably: enabling a tier for a target -repository brings that repository's skills without also running its hooks. +Measured against the CLI, all of the following continue to follow cwd: + +| Named via `settings_dir` | Loaded? | +|---|---| +| `.claude/skills` | **yes** — listed and invocable | +| `CLAUDE.md` | no | +| `.claude/rules/*.md` | no | +| `.claude/settings.json` (`env`, `hooks`) | no | +| `.claude/agents` | no | + +So 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 diff --git a/src/conductor/config/schema.py b/src/conductor/config/schema.py index ef69a070..d0f2b371 100644 --- a/src/conductor/config/schema.py +++ b/src/conductor/config/schema.py @@ -1386,10 +1386,17 @@ class AgentDef(BaseModel): 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/settings.json`` (so no ``env`` and no - ``hooks``), not ``.claude/agents``, all of which continue to follow cwd. - Instructions therefore still need ``working_dir`` (or - ``--workspace-instructions``); this field is the skills half only. + ``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:: diff --git a/src/conductor/providers/claude_agent_sdk.py b/src/conductor/providers/claude_agent_sdk.py index 4b787e61..b81dc9bd 100644 --- a/src/conductor/providers/claude_agent_sdk.py +++ b/src/conductor/providers/claude_agent_sdk.py @@ -1007,8 +1007,10 @@ async def _execute_session( # What it does do is make 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/settings.json or .claude/agents, which stay - # with cwd). That is the one job it is used for here, so the value + # CLAUDE.md, .claude/rules/*.md, .claude/settings.json or + # .claude/agents, which all stay with cwd -- measured, so this is + # the skills portion of a project tier rather than a + # cwd-independent way to load one). That is its one job, so the value # is the author's ``settings_dir`` rather than a guess derived # from server arguments. add_dirs=[agent.settings_dir] if agent.settings_dir else [], diff --git a/tests/test_providers/test_claude_agent_sdk.py b/tests/test_providers/test_claude_agent_sdk.py index 2bd4e2ac..ab4fc0ba 100644 --- a/tests/test_providers/test_claude_agent_sdk.py +++ b/tests/test_providers/test_claude_agent_sdk.py @@ -3374,6 +3374,47 @@ async def fake_query(**kwargs): 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", + ) + + assert captured["cwd"] == str(wide) + assert str(narrow) not in (captured["cwd"] or "") + @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, From fefbdc56c33a8dfa3e178342e6f7f418451e0fc0 Mon Sep 17 00:00:00 2001 From: Chris Throup Date: Mon, 7 Sep 2026 17:05:25 +0200 Subject: [PATCH 3/4] fix(claude-agent-sdk): validate settings_dir, and document its filesystem grant Two independent reviews of the preceding commits, one of which saw only the diff. Four issues, all confirmed by execution before fixing. 1. No capability carve-out. `capabilities.py` states the rule in its own words for the sibling field -- "silently ignoring the directory would run the agent in the wrong repository" -- and `working_dir` and `session_key` both have validator checks. `settings_dir` had none, so it validated green on `copilot` and was discarded. Adds `ProviderCapabilities.settings_dir` (True only on claude-agent-sdk, the only provider with an `add_dirs`) and the matching error. 2. Silent no-op without `setting_sources`. With no tier enabled the skills half does nothing, and validate said nothing. Now a warning, not an error, because the filesystem half still applies -- see 3. 3. The docs omitted the effect that matters most. `add_dirs`' own SDK contract is "additional directories Claude can access beyond the current working directory", so a `settings_dir` widens the model's built-in Read/Edit/Bash to that tree -- and unconditionally, with no settings tier at all. The docs table answered "no" to four things and never mentioned filesystem access. Measured with `setting_sources` unset and `permission_mode: default`: the same read outside cwd is refused for permissions without `settings_dir` and succeeds with it. It still does not widen what an MCP server permits. 4. `AGENTS.md` carried a ticket key -- the first in that file, where all 82 other provenance markers are upstream issue refs. Dropped; the mechanism sentence already carried the content. Also: `settings_dir` escaped `_collect_template_strings`, so a typo'd upstream step name failed at run time where the same typo in `working_dir` fails at validate. The three call-site comments named only `working_dir` despite resolving both. And `test_settings_dir_does_not_become_a_second_cwd` asserted a substring that also passes for an unrelated cwd -- replaced with the assertion its docstring claimed. Tests: the three validator behaviours are covered against the real shipped descriptors, not a patched harness. Every case was green before this commit, which is why they exist. Co-Authored-By: Claude Opus 5 --- AGENTS.md | 2 +- docs/workflow-syntax.md | 26 ++++- src/conductor/config/schema.py | 26 +++-- src/conductor/config/validator.py | 42 ++++++++ src/conductor/engine/workflow.py | 11 ++- src/conductor/providers/capabilities.py | 13 +++ src/conductor/providers/claude_agent_sdk.py | 3 + tests/test_config/test_settings_dir_schema.py | 98 ++++++++++++++++++- tests/test_providers/test_claude_agent_sdk.py | 6 +- 9 files changed, 208 insertions(+), 19 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index b521ba2b..853bebce 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -423,7 +423,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 the unconditional `setting_sources=[]` (see **Skills** below) stops the CLI loading `CLAUDE.md`, project settings, and hooks from it — so cwd no longer drags ambient instructions in. `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`, PDA-95): 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. An earlier fork-local `_stdio_path_args` helper derived `add_dirs` from every stdio server's directory arguments, with a docstring asserting this "restores the declared scope"; it cannot — `--add-dir` takes no part in Roots negotiation, it widens the CLI's own file tools. That helper is **removed**; do not reintroduce deriving `add_dirs` from server arguments, which 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`, no `hooks`) 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. `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. An earlier fork-local `_stdio_path_args` helper derived `add_dirs` from every stdio server's directory arguments, with a docstring asserting this "restores the declared scope"; it cannot — `--add-dir` takes no part in Roots negotiation, it widens the CLI's own file tools. That helper is **removed**; do not reintroduce deriving `add_dirs` from server arguments, which 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`, no `hooks`) 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: with `setting_sources` unset and `permission_mode: default`, a read outside cwd is refused without it and succeeds with it). 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=[]` **unconditionally**, for the same reason `strict_mcp_config=True` is unconditional a few lines away. 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 but their files stay readable. diff --git a/docs/workflow-syntax.md b/docs/workflow-syntax.md index 03e511e9..2561835c 100644 --- a/docs/workflow-syntax.md +++ b/docs/workflow-syntax.md @@ -448,19 +448,35 @@ agents: #### What it does and does not carry -A directory named here contributes its `.claude/skills` and nothing else. -Measured against the CLI, all of the following continue to follow cwd: +Measured against the CLI: -| Named via `settings_dir` | Loaded? | +| 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`, `hooks`) | no | | `.claude/agents` | no | -So this field is the *skills portion* of a project tier, not a -cwd-independent way to load one. It cuts favourably in one direction — +> ⚠️ **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 with `setting_sources` unset and +> `permission_mode: default`: without `settings_dir` a read outside cwd is +> refused for permissions; with it, the same read succeeds. +> +> 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. + +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": diff --git a/src/conductor/config/schema.py b/src/conductor/config/schema.py index d0f2b371..85e0f772 100644 --- a/src/conductor/config/schema.py +++ b/src/conductor/config/schema.py @@ -1361,14 +1361,24 @@ class AgentDef(BaseModel): settings_dir: str | None = None """Directory whose Claude Code *project* settings tier this agent loads. - Only meaningful on ``claude-agent-sdk`` agents in a workflow that sets - ``runtime.provider.setting_sources`` (see - :attr:`ProviderSettings.setting_sources`); ignored by every other - provider. 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/human_gate/questions/workflow step types. + ``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 diff --git a/src/conductor/config/validator.py b/src/conductor/config/validator.py index 6fe614ed..60a173af 100644 --- a/src/conductor/config/validator.py +++ b/src/conductor/config/validator.py @@ -1203,6 +1203,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 @@ -1738,6 +1743,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. @@ -2383,6 +2400,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 7f9b29d3..cc1094d7 100644 --- a/src/conductor/engine/workflow.py +++ b/src/conductor/engine/workflow.py @@ -651,6 +651,11 @@ def _resolve_agent_working_dir( ) -> 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 @@ -4483,7 +4488,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. @@ -6292,7 +6297,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) @@ -6778,7 +6783,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 d53dc191..63d4b3ef 100644 --- a/src/conductor/providers/capabilities.py +++ b/src/conductor/providers/capabilities.py @@ -153,6 +153,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 b81dc9bd..9da8a84b 100644 --- a/src/conductor/providers/claude_agent_sdk.py +++ b/src/conductor/providers/claude_agent_sdk.py @@ -675,6 +675,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 diff --git a/tests/test_config/test_settings_dir_schema.py b/tests/test_config/test_settings_dir_schema.py index 9f1373ef..b59d0541 100644 --- a/tests/test_config/test_settings_dir_schema.py +++ b/tests/test_config/test_settings_dir_schema.py @@ -14,10 +14,23 @@ from __future__ import annotations +from pathlib import Path + import pytest from pydantic import ValidationError -from conductor.config.schema import AgentDef, GateOption +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: @@ -85,3 +98,86 @@ 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_providers/test_claude_agent_sdk.py b/tests/test_providers/test_claude_agent_sdk.py index ab4fc0ba..cda33b6d 100644 --- a/tests/test_providers/test_claude_agent_sdk.py +++ b/tests/test_providers/test_claude_agent_sdk.py @@ -3412,8 +3412,12 @@ async def fake_query(**kwargs): 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 str(narrow) not in (captured["cwd"] or "") + 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: From e7ea3ac34cc9c00aa1522306aa86dcbf21ce47e2 Mon Sep 17 00:00:00 2001 From: Chris Throup Date: Mon, 7 Sep 2026 17:15:47 +0200 Subject: [PATCH 4/4] docs(claude-agent-sdk): hooks under settings_dir are unknown, not measured The docs table listed `.claude/settings.json` (`env`, `hooks`) as a single "no" row. The `env` half is measured -- an env var declared there reads back empty. The `hooks` half is not: the probe grepped the CLI's debug output for a hook's marker and found nothing, but also found nothing in the control, where cwd was the directory and the hook should have run. A probe that returns nothing in its own baseline cannot support a negative. Splits the row and says so. Hooks are now documented as undetermined -- do not rely on them running, do not rely on them being suppressed -- with the instrument that would settle it named (a hook with an observable side effect, such as writing a file). The row was correct in direction and unearned in confidence, which is the version that survives review. Co-Authored-By: Claude Opus 5 --- docs/workflow-syntax.md | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/docs/workflow-syntax.md b/docs/workflow-syntax.md index 2561835c..f6770c68 100644 --- a/docs/workflow-syntax.md +++ b/docs/workflow-syntax.md @@ -456,7 +456,8 @@ Measured against the CLI: | `.claude/skills` | **yes** — listed and invocable | | `CLAUDE.md` | no | | `.claude/rules/*.md` | no | -| `.claude/settings.json` (`env`, `hooks`) | no | +| `.claude/settings.json` `env` | no | +| `.claude/settings.json` `hooks` | **not established** — see below | | `.claude/agents` | no | > ⚠️ **The filesystem grant does not depend on `setting_sources`.** This field @@ -475,6 +476,14 @@ 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 honestly unknown, not a measured negative.** The probe +behind it grepped the CLI's debug output for a hook's marker and found nothing +-- but it also found nothing in the control, where cwd *was* the directory and +the hook demonstrably should have run. A probe that returns nothing in its own +baseline cannot support a negative, so treat hooks as undetermined: do not +rely on them running, and do not rely on them being suppressed. Establishing +it needs a hook with an observable side effect, such as writing a file. + 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