diff --git a/AGENTS.md b/AGENTS.md index ead46d01..853bebce 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`): 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/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..f6770c68 100644 --- a/docs/workflow-syntax.md +++ b/docs/workflow-syntax.md @@ -400,6 +400,117 @@ Because paths are normalized lexically instead of resolving to their real paths: > Setting `working_dir` doesn't restrict the model's filesystem access. The model can still read and write files outside this directory if it uses absolute paths or parent directory traversals (e.g., `../`). Avoid relying on this configuration to sandbox untrusted model execution. > On the `claude-agent-sdk` provider the directory is also a trust boundary in the other direction: the `claude` CLI loads `CLAUDE.md` and `.claude/settings*.json` (including hooks) from wherever it runs, so pointing `working_dir` at an untrusted checkout means running that checkout's instructions. +### Target-Repository Skills (`settings_dir`) + +`settings_dir` names a second directory whose Claude Code *project* settings +tier the agent reads skills from. It applies to `claude-agent-sdk` agents in a +workflow that sets `runtime.provider.setting_sources`, and is ignored by every +other provider. + +```yaml +workflow: + runtime: + provider: + name: claude-agent-sdk + setting_sources: [project] + +agents: + - name: judge + settings_dir: "{{ setup_worktree.output.worktree_path }}" + prompt: Review the change against this repository's conventions. +``` + +#### Why it is separate from `working_dir` + +An agent's cwd does two unrelated jobs, and on this provider they conflict. +The `claude` CLI supports the MCP Roots protocol and advertises exactly one +root — its cwd. A filesystem MCP server therefore **discards the directories +in its own argv** and permits cwd alone; `--add-dir` takes no part in that +negotiation, so it cannot widen what a server allows. cwd is simultaneously +the directory the `project` settings tier resolves against. + +So pointing `working_dir` at a target repository to pick up that repository's +skills also narrows the agent's only MCP root onto it, and any sibling path +the step still has to read — an artifacts directory, a second checkout — is +denied. Widening cwd back loses the repository's conventions. + +`settings_dir` splits the two. Skills are discovered from cwd **and** from +`settings_dir`, so cwd can stay wide enough to contain everything the agent +must read: + +```yaml +agents: + - name: judge + # No working_dir: cwd stays the launch directory, which contains both the + # worktree and the artifacts this judge reads through the filesystem MCP. + settings_dir: "{{ setup_worktree.output.worktree_path }}" +``` + +#### What it does and does not carry + +Measured against the CLI: + +| Named via `settings_dir` | Granted? | +|---|---| +| **Filesystem access for the model's built-in tools** (`Read`, `Edit`, `Bash`, …) | **yes — unconditionally**, see below | +| `.claude/skills` | **yes** — listed and invocable | +| `CLAUDE.md` | no | +| `.claude/rules/*.md` | no | +| `.claude/settings.json` `env` | no | +| `.claude/settings.json` `hooks` | **not established** — see below | +| `.claude/agents` | no | + +> ⚠️ **The filesystem grant does not depend on `setting_sources`.** This field +> maps to the SDK's `add_dirs`, whose own contract is *"additional directories +> Claude can access beyond the current working directory"* — so naming a +> directory here widens the model's built-in file tools to that tree whether or +> not any settings tier is enabled. Measured 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. + +**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 +does not compose with `working_dir` into "everything, anywhere": + +> An agent that needs a target repository's **rules or instructions** as well +> as a cwd wide enough for its MCP servers cannot get both from these fields. +> One directory cannot be narrow and wide at once. `settings_dir` recovers the +> skills; anything else is a caller-side trade — keep `working_dir` on the +> repository and arrange for every path the agent reads to sit beneath it. + +#### Resolution and restrictions + +- Resolved exactly like `working_dir` — Jinja2-rendered, `~`-expanded, + relative paths resolved against the workflow file's directory, normalized + with `os.path.normpath`, and existence-checked before any provider call. +- Per-agent only. There is no `runtime.settings_dir`, because the repository + whose conventions apply is what varies between steps. +- Rejected on `wait`, `set`, `terminate`, `script`, `human_gate`, `questions` + and `workflow` step types — none has an LLM session to apply a settings tier + to, and accepting it silently would suggest conventions had been loaded when + none had. + +> ⚠️ A settings tier brings everything that tier defines. Enable +> `setting_sources` and point `settings_dir` only at repositories trusted to +> the same degree as the workflow itself. + ### Session Continuity (`session_key`) By default each agent execution starts a fresh provider session, so an agent diff --git a/examples/claude-agent-sdk-settings-dir.yaml b/examples/claude-agent-sdk-settings-dir.yaml new file mode 100644 index 00000000..1f6afbb3 --- /dev/null +++ b/examples/claude-agent-sdk-settings-dir.yaml @@ -0,0 +1,134 @@ +# Target-repository skills without narrowing the MCP root (`settings_dir`) +# +# A reviewer agent that loads a TARGET repository's own `.claude/skills` while +# keeping filesystem access wide enough to also read a sibling artifacts +# directory. Two directories, two jobs, set independently. +# +# THE PROBLEM THIS EXAMPLE EXISTS FOR +# On `claude-agent-sdk`, an agent's cwd does two unrelated jobs at once: +# +# 1. The `claude` CLI supports the MCP Roots protocol and advertises +# exactly ONE root -- its cwd. `@modelcontextprotocol/server-filesystem` +# uses the directories in its own argv only while the client does NOT +# support Roots, so with this CLI it discards them and permits cwd +# alone. cwd is therefore the only handle on what the agent may read. +# (`--add-dir` cannot widen this: it takes no part in that negotiation.) +# +# 2. cwd is also what the `project` settings tier resolves against, so it +# decides WHOSE conventions load. +# +# Point `working_dir` at the repository under review and job 2 is satisfied +# while job 1 breaks: the artifacts directory outside that repository is now +# denied. Widen `working_dir` back and job 2 breaks instead -- the reviewer +# loads the ORCHESTRATOR's conventions while reviewing someone else's code. +# +# THE FIX +# `settings_dir` carries job 2 on its own. Skills are discovered from cwd +# AND from `settings_dir`, so cwd stays wide: +# +# working_dir (or none) -> the single MCP root: keep it wide +# settings_dir -> whose .claude/skills load: keep it narrow +# +# Only skills travel this way. `CLAUDE.md`, `.claude/settings.json` (so +# `env` and `hooks`) and `.claude/agents` all keep following cwd -- which +# cuts favourably here: the target repository's skills arrive without its +# hooks also running. +# +# Pre-requisites: +# pip install conductor[claude-agent-sdk] +# npm install -g @anthropic-ai/claude-code # the `claude` CLI +# claude login +# npx -y @modelcontextprotocol/server-filesystem --help # warms the server +# +# Usage -- `workspace` must CONTAIN both the repo and the artifacts directory: +# conductor run examples/claude-agent-sdk-settings-dir.yaml \ +# --input workspace=/path/to/workspace \ +# --input repo=/path/to/workspace/target-repo \ +# --input artifacts=/path/to/workspace/artifacts +# +# where: +# workspace -- contains BOTH the repo and the artifacts directory. It +# becomes cwd, and therefore the filesystem MCP server's +# only root: everything the agent reads must live under it. +# repo -- the target repository, under `workspace`. Supplies the +# skills, via `settings_dir`. +# artifacts -- prior findings, under `workspace` but OUTSIDE `repo` -- +# the path a narrowed cwd would deny. +# +# Validation only (no execution / no API calls): +# conductor validate examples/claude-agent-sdk-settings-dir.yaml + +workflow: + name: claude-agent-sdk-settings-dir + description: > + Reviewer that loads a target repository's own skills via `settings_dir` + while keeping a wide cwd, so its filesystem MCP server can still reach a + sibling artifacts directory. + version: "1.0.0" + entry_point: review + + runtime: + provider: + name: claude-agent-sdk + # Opt in to the `project` tier. Off by default, so a run does not + # inherit whatever the machine happens to have installed. A tier brings + # everything it defines: enable it only for repositories trusted to the + # same degree as this workflow. + setting_sources: [project] + default_model: claude-sonnet-4-5 + + # cwd for every agent: the wide directory. The filesystem MCP server's + # argv roots below are discarded by the server itself (see the header), so + # THIS is what actually decides what the agent can read. + working_dir: "{{ workflow.input.workspace }}" + + mcp_servers: + filesystem: + type: stdio + command: npx + args: + - "-y" + - "@modelcontextprotocol/server-filesystem" + # Declared for a client that does not negotiate Roots. The `claude` + # CLI does, so it permits cwd alone and these are ignored -- kept + # because they are still the honest declaration of intent, not + # because they take effect here. + - "{{ workflow.input.repo }}" + - "{{ workflow.input.artifacts }}" + +agents: + - name: review + # The narrow half: this repository's `.claude/skills` become listed and + # invocable, with cwd still the wide workspace above. Templated, since the + # directory under review is normally an upstream step's output. + settings_dir: "{{ workflow.input.repo }}" + prompt: | + You are reviewing the repository at {{ workflow.input.repo }}. + + First, list the skills available to you. Any skill this repository ships + in its own `.claude/skills` should be among them -- that is what + `settings_dir` provides, and you should follow its conventions rather + than any you would apply by default. + + Then read the prior findings under {{ workflow.input.artifacts }} using + the filesystem MCP server. That directory sits outside the repository, + so a run that had narrowed the working directory onto the repository + could not read it at all. + + Report: + - which repository-supplied skills you found + - what the prior findings say + - whether the repository's conventions change your reading of them + output: + skills_found: + type: string + description: Repository-supplied skills the session offered. + findings_summary: + type: string + description: What the artifacts directory contained. + routes: + - to: "$end" + +output: + skills_found: "{{ review.output.skills_found }}" + findings_summary: "{{ review.output.findings_summary }}" diff --git a/src/conductor/config/schema.py b/src/conductor/config/schema.py index 743df14e..85e0f772 100644 --- a/src/conductor/config/schema.py +++ b/src/conductor/config/schema.py @@ -1358,6 +1358,66 @@ class AgentDef(BaseModel): wait/set/terminate/human_gate/workflow step types. """ + settings_dir: str | None = None + """Directory whose Claude Code *project* settings tier this agent loads. + + ``claude-agent-sdk`` only -- ``conductor validate`` refuses it against a + provider that cannot apply it, rather than dropping it silently. Resolved + by the engine exactly like :attr:`working_dir` (Jinja-rendered, + ``~``-expanded, made absolute against the workflow file's directory, + ``normpath``-normalised, existence-checked), then forwarded to the SDK as + ``ClaudeAgentOptions.add_dirs``. Rejected on + wait/set/terminate/script/human_gate/questions/workflow step types. + + **Two effects, and only one of them is conditional.** Skill discovery + requires ``runtime.provider.setting_sources`` to enable the ``project`` + tier; ``conductor validate`` warns when this field is set without it, + since the skills half is then a no-op. The *filesystem* grant is + unconditional: ``add_dirs``' own SDK contract is "additional directories + Claude can access beyond the current working directory", so naming a + directory here widens the model's built-in ``Read``/``Edit``/``Bash`` + tools to that tree regardless of any settings tier. It does **not** widen + what a filesystem MCP server permits -- that stays cwd alone, which is why + this field exists. Point it only at a directory the agent may read. + + It exists because ``working_dir`` was doing two unrelated jobs. The CLI + supports MCP Roots and advertises exactly one root — its cwd — so a + filesystem MCP server discards the directories in its own argv and + permits cwd alone. That makes cwd the *only* handle on what an agent can + read, while it is simultaneously the directory the ``project`` settings + tier resolves against. Narrowing cwd onto a target repository to pick up + that repository's skills therefore also narrowed the MCP root below any + sibling path the step still had to read, and widening it back lost the + repository's conventions. + + ``settings_dir`` splits them: the *skills* of every directory named here + are discovered and invocable regardless of cwd, so cwd can stay wide + enough to contain everything the agent must read. + + The split is not total, and the remainder is deliberate. A directory + named here contributes its ``.claude/skills`` and nothing else — not + ``CLAUDE.md``, not ``.claude/rules/*.md``, not ``.claude/settings.json`` + (so no ``env`` and no ``hooks``), not ``.claude/agents``, all of which + continue to follow cwd. This field is the *skills* portion of a project + tier, not a cwd-independent way to load one: instructions, rules and + hooks still require ``working_dir`` pointed at the directory. + + So the two fields do not compose into "everything, anywhere". An agent + needing a target repository's rules *and* a cwd wide enough for its MCP + servers cannot have both from these fields alone -- one directory cannot + be simultaneously narrow and wide. ``settings_dir`` recovers the skills; + the rest is a caller-side trade. + + Example — a judge reviewing a target repository while reading artifacts + from a sibling directory:: + + agents: + judge: + settings_dir: "{{ setup_worktree.output.worktree_path }}" + # No working_dir: cwd stays the launch directory, which contains + # both the worktree and the artifacts the judge must read. + """ + stdin: str | None = None """Payload written to the script subprocess's stdin (script type only). @@ -2007,6 +2067,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 +2132,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 +2167,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 +2235,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 +2260,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 +2335,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 +2411,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 +2750,19 @@ class ProviderSettings(BaseModel): installed, so a run reproduces on another developer's laptop. Set it to opt a workflow back in. The motivating case is an agent working - inside a *target* repository that ships its own ``.claude/skills`` — with - ``working_dir`` pointed at that repo, ``["project"]`` loads that repo's - skills and instructions natively, without the repo needing to package - them as a Claude Code plugin (the CLI has ``--plugin-dir`` but no - ``--skill-dir``, so a plugin root is otherwise the only handle). + against a *target* repository that ships its own ``.claude/skills``: + ``["project"]`` loads that repository's skills natively, without it + needing to package them as a Claude Code plugin (the CLI has + ``--plugin-dir`` but no ``--skill-dir``, so a plugin root is otherwise + the only handle). + + Which directory the ``project`` tier reads is chosen per agent. Skills + come from cwd (:attr:`AgentDef.working_dir`) *and* from + :attr:`AgentDef.settings_dir`; everything else the tier defines -- + ``CLAUDE.md``, ``.claude/settings.json``, ``.claude/agents`` — follows + cwd alone. Prefer ``settings_dir`` when the agent also needs a wider cwd: + the CLI advertises cwd as its sole MCP root, so narrowing cwd onto the + target repository narrows what the agent's MCP servers may read. Each tier brings everything that tier defines, hooks included: ``project`` reads ``/.claude/settings.json``, whose ``hooks`` entries run diff --git a/src/conductor/config/validator.py b/src/conductor/config/validator.py index 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 7fc23b5e..cc1094d7 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,57 @@ def _resolve_agent_working_dir( if not Path(resolved).is_dir(): raise ExecutionError( - f"Agent '{agent.name}': working_dir '{resolved}' does not exist or " + f"Agent '{agent.name}': {field} '{resolved}' does not exist or " f"is not a directory (rendered from '{raw}')", agent_name=agent.name, suggestion=( "Create the directory before the agent runs (e.g. via a " - "script step) or fix the working_dir template." + f"script step) or fix the {field} template." ), ) + return resolved - return agent.model_copy(update={"working_dir": resolved}) + def _resolve_agent_working_dir( + self, agent: AgentDef, agent_context: dict[str, Any] + ) -> AgentDef: + """Resolve an agent's ``working_dir`` and ``settings_dir``, returning a copy. + + The name says ``working_dir`` only for historical reasons -- it is + referenced by name from four other modules' comments, so renaming it + costs more than it explains. Grep for ``settings_dir`` and this is + where it is resolved. + + ``working_dir`` precedence is ``agent.working_dir`` over + ``runtime.working_dir``; ``settings_dir`` is per-agent only, since the + directory whose conventions apply is what varies between steps. Both + are resolved by :meth:`_resolve_agent_directory`. An agent setting + neither is returned unchanged, leaving the provider its own cwd. + + The two are independent on purpose: ``working_dir`` becomes the + session cwd, which the CLI advertises as its sole MCP root, while + ``settings_dir`` only adds a directory whose project-tier skills are + discoverable. An agent can therefore keep a wide cwd — wide enough + for every path its MCP servers must reach — and still load a + narrower target repository's skills. + """ + update: dict[str, Any] = {} + + raw = agent.working_dir + if raw is None: + raw = self.config.workflow.runtime.working_dir + if raw is not None: + update["working_dir"] = self._resolve_agent_directory( + agent, agent_context, "working_dir", raw + ) + + if agent.settings_dir is not None: + update["settings_dir"] = self._resolve_agent_directory( + agent, agent_context, "settings_dir", agent.settings_dir + ) + + if not update: + return agent + return agent.model_copy(update=update) def _build_pricing_overrides(self) -> dict[str, ModelPricing] | None: """Build pricing overrides from workflow cost configuration. @@ -4452,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. @@ -6261,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) @@ -6747,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 b247b8d0..9da8a84b 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. @@ -692,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 @@ -975,9 +961,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 +995,28 @@ 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/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 [], output_format=_build_output_format(agent.output) if agent.output else None, max_turns=max_turns, permission_mode=permission_mode, diff --git a/tests/test_config/test_set_schema.py b/tests/test_config/test_set_schema.py index 9581e734..f4587ab9 100644 --- a/tests/test_config/test_set_schema.py +++ b/tests/test_config/test_set_schema.py @@ -132,6 +132,7 @@ class TestSetAgentDefForbiddenFields: ("args", ["x"], "cannot have 'args'"), ("env", {"K": "v"}, "cannot have 'env'"), ("working_dir", "/tmp", "cannot have 'working_dir'"), + ("settings_dir", "/tmp", "cannot have 'settings_dir'"), ("timeout", 5, "cannot have 'timeout'"), ("workflow", "x.yaml", "cannot have 'workflow'"), ("input_mapping", {"a": "1"}, "cannot have 'input_mapping'"), diff --git a/tests/test_config/test_settings_dir_schema.py b/tests/test_config/test_settings_dir_schema.py new file mode 100644 index 00000000..b59d0541 --- /dev/null +++ b/tests/test_config/test_settings_dir_schema.py @@ -0,0 +1,183 @@ +"""Schema tests for ``AgentDef.settings_dir``. + +``settings_dir`` names the directory whose Claude Code *project* settings +tier an agent loads skills from. It exists because ``working_dir`` was doing +two unrelated jobs at once: the CLI advertises its cwd as its sole MCP root, +so narrowing cwd onto a target repository to pick up that repository's +skills also narrowed what the agent's MCP servers were permitted to read. + +These tests pin the field's shape and, more importantly, that it stays +*independent* of ``working_dir`` -- a schema that coupled them, or that +silently accepted the field on a step type with no LLM session to apply it +to, would reintroduce the confusion the split removes. +""" + +from __future__ import annotations + +from pathlib import Path + +import pytest +from pydantic import ValidationError + +from conductor.config.schema import ( + AgentDef, + GateOption, + OutputField, + ProviderSettings, + RouteDef, + RuntimeConfig, + WorkflowConfig, + WorkflowDef, +) +from conductor.config.validator import validate_workflow_config +from conductor.exceptions import ConfigurationError + + +class TestSettingsDirAccepted: + """Provider-backed agents take the field, alone or alongside cwd.""" + + def test_accepted_on_a_plain_llm_agent(self) -> None: + agent = AgentDef(name="judge", prompt="review", settings_dir="/repo") + assert agent.settings_dir == "/repo" + + def test_defaults_to_none(self) -> None: + """Omitting it must add no directory -- the tier is opt-in.""" + assert AgentDef(name="judge", prompt="review").settings_dir is None + + def test_independent_of_working_dir(self) -> None: + """The point of the field: both set, to different directories. + + A wide cwd keeps every path the agent's MCP servers must reach inside + the single negotiated root, while the narrow settings_dir supplies the + target repository's conventions. + """ + agent = AgentDef( + name="judge", prompt="review", working_dir="/wide", settings_dir="/wide/repo" + ) + assert (agent.working_dir, agent.settings_dir) == ("/wide", "/wide/repo") + + def test_accepts_a_template(self) -> None: + """The directory a step reviews is normally an upstream step's output, + so the raw value is a Jinja template the engine renders.""" + agent = AgentDef( + name="judge", prompt="review", settings_dir="{{ setup.output.worktree_path }}" + ) + assert agent.settings_dir == "{{ setup.output.worktree_path }}" + + +class TestSettingsDirRejectedOnNonProviderSteps: + """Every step type with no LLM session rejects it. + + Accepting it silently is the failure mode that matters here: an author + would see a green ``conductor validate`` and conclude the target + repository's conventions were loaded when nothing had been. + """ + + @pytest.mark.parametrize( + ("kwargs",), + [ + ({"type": "wait", "duration": "1s"},), + ({"type": "set", "value": "x"},), + ({"type": "terminate", "status": "success", "reason": "done"},), + ({"type": "script", "command": "echo hi"},), + ({"type": "workflow", "workflow": "child.yaml"},), + ( + { + "type": "human_gate", + "prompt": "ok?", + "options": [GateOption(label="OK", value="ok", route="$end")], + }, + ), + ], + ) + def test_rejected(self, kwargs: dict) -> None: + with pytest.raises(ValidationError, match="cannot have 'settings_dir'"): + AgentDef(name="bad", settings_dir="/repo", **kwargs) + + def test_error_names_the_step_type(self) -> None: + """So the message says which step to fix, not merely that one is wrong.""" + with pytest.raises(ValidationError, match="wait agents cannot have 'settings_dir'"): + AgentDef(name="bad", type="wait", duration="1s", settings_dir="/repo") + + +class TestSettingsDirValidation: + """``conductor validate`` must not report success on a silent no-op. + + Every case here was green before these checks existed, which is the point: + the step-type rejections above guard authoring mistakes nobody makes, while + the three below are the ones an author actually makes -- wrong provider, + forgotten `setting_sources`, typo'd upstream step name. A green validate + for any of them tells the author their target repository's conventions + loaded when nothing did. + """ + + @staticmethod + def _config(provider: object, settings_dir: str, tmp_path: Path) -> WorkflowConfig: + return WorkflowConfig( + workflow=WorkflowDef( + name="w", + entry_point="a", + runtime=RuntimeConfig(provider=provider), # type: ignore[arg-type] + ), + agents=[ + AgentDef( + name="a", + prompt="hi", + settings_dir=settings_dir, + output={"r": OutputField(type="string")}, + routes=[RouteDef(to="$end")], + ) + ], + output={"r": "{{ a.output.r }}"}, + ) + + def test_rejected_on_a_provider_that_cannot_apply_it(self, tmp_path: Path) -> None: + """Only ``claude-agent-sdk`` has an ``add_dirs`` to put it in. + + Same class as ``working_dir``, whose capability docstring gives the + reason: silently ignoring the directory runs the agent against the + wrong repository while reporting success. + """ + config = self._config("copilot", str(tmp_path), tmp_path) + + with pytest.raises(ConfigurationError, match="does not apply it"): + validate_workflow_config(config) + + def test_accepted_on_claude_agent_sdk_with_setting_sources(self, tmp_path: Path) -> None: + """The supported combination raises nothing.""" + config = self._config( + ProviderSettings(name="claude-agent-sdk", setting_sources=["project"]), + str(tmp_path), + tmp_path, + ) + + validate_workflow_config(config) # no raise + + def test_warns_when_no_settings_tier_is_enabled(self, tmp_path: Path) -> None: + """A warning, not an error, and the distinction is load-bearing. + + Without ``setting_sources`` no ``project`` tier exists, so the skills + half -- the reason the field is normally set -- is a no-op. The + filesystem half still applies, so the workflow is not broken; erroring + would refuse a configuration that does something. + """ + config = self._config("claude-agent-sdk", str(tmp_path), tmp_path) + + warnings = validate_workflow_config(config) + + assert any("setting_sources" in w for w in warnings), warnings + assert any("no skills will be discovered" in w for w in warnings), warnings + + def test_template_referencing_an_unknown_step_is_caught(self, tmp_path: Path) -> None: + """The field is normally templated from an upstream step, so a typo'd + step name is the likely authoring error. It must fail at validate, as + the same typo in ``working_dir`` already does, rather than at run + time.""" + config = self._config( + ProviderSettings(name="claude-agent-sdk", setting_sources=["project"]), + "{{ nonexistent_step.output.path }}", + tmp_path, + ) + + with pytest.raises(ConfigurationError, match="nonexistent_step"): + validate_workflow_config(config) diff --git a/tests/test_engine/test_workflow.py b/tests/test_engine/test_workflow.py index 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..cda33b6d 100644 --- a/tests/test_providers/test_claude_agent_sdk.py +++ b/tests/test_providers/test_claude_agent_sdk.py @@ -3228,3 +3228,218 @@ 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_does_not_become_a_second_cwd(self, tmp_path: Path) -> None: + """The boundary of what this field can deliver, pinned deliberately. + + ``add_dirs`` carries a directory's ``.claude/skills`` and nothing + else: ``CLAUDE.md``, ``.claude/rules/*.md``, ``.claude/settings.json`` + (so ``env`` and ``hooks``) and ``.claude/agents`` all follow cwd + instead -- measured against the CLI, not inferred. So a + ``settings_dir`` must never be quietly promoted into ``cwd`` in an + attempt to widen what it loads: that would hand the agent the narrow + directory as its sole MCP root, which is the exact defect this field + exists to avoid. + + An agent needing a repository's rules *and* a wide cwd cannot have + both from these two fields, and this test is what keeps that trade + visible rather than papered over. + """ + wide = tmp_path / "wide" + wide.mkdir() + narrow = wide / "repo" + narrow.mkdir() + captured: dict = {} + + async def fake_query(**kwargs): + captured["cwd"] = kwargs["options"].cwd + captured["add_dirs"] = kwargs["options"].add_dirs + yield _result(result="ok") + + with patch("conductor.providers.claude_agent_sdk.query", fake_query): + provider = ClaudeAgentSdkProvider() + await provider.execute( + agent=AgentDef( + name="t", prompt="hi", working_dir=str(wide), settings_dir=str(narrow) + ), + context={}, + rendered_prompt="hi", + ) + + # The line that pins it: cwd is the wide directory exactly. The + # earlier `narrow not in cwd` substring check added nothing -- it also + # passes for an implementation that sets cwd to an unrelated third + # directory, so it read as a guard without being one. + assert captured["cwd"] == str(wide) + assert captured["add_dirs"] == [str(narrow)] + + @patch("conductor.providers.claude_agent_sdk.CLAUDE_AGENT_SDK_AVAILABLE", True) + async def test_settings_dir_passed_verbatim(self, tmp_path: Path) -> None: + """Not re-resolved, matching ``cwd``: the engine already rendered, + absolutized and existence-checked it, and ``resolve()`` here would + collapse the symlink aliases the engine preserves on purpose.""" + real = tmp_path / "real" + real.mkdir() + link = tmp_path / "link" + link.symlink_to(real, target_is_directory=True) + captured: dict = {} + + async def fake_query(**kwargs): + captured["add_dirs"] = kwargs["options"].add_dirs + yield _result(result="ok") + + with patch("conductor.providers.claude_agent_sdk.query", fake_query): + provider = ClaudeAgentSdkProvider() + await provider.execute( + agent=AgentDef(name="t", prompt="hi", settings_dir=str(link)), + context={}, + rendered_prompt="hi", + ) + + assert captured["add_dirs"] == [str(link)]