diff --git a/AGENTS.md b/AGENTS.md index 51c6124e..9710bcd0 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -431,7 +431,8 @@ Conductor: - The SDK applies it as the `claude` subprocess's cwd (`_internal/transport/subprocess_cli.py` as of 0.2.87 passes it to `open_process` and sets `PWD`), so stdio MCP servers pick it up by **inheriting** it from that subprocess. There is deliberately no per-server stamping as in `copilot.py::_mcp_servers_for_cwd`: the SDK's `McpStdioServerConfig` has no cwd field, so `_translate_mcp_servers` is left alone. Inheritance is a property of the CLI binary, not of the SDK, so it is documented rather than asserted by a test. - The path is passed **verbatim** — `WorkflowEngine._resolve_agent_working_dir` has already rendered, absolutized, normalised, and existence-checked it, and re-resolving here would collapse the symlink aliases the engine preserves on purpose. The `ClaudeAgentOptions(...)` construction lives **inside** `execute`'s `try` so the `os.getcwd()` fallback can't escape as a bare `OSError` when the process cwd has been deleted (`copilot.py` resolves its cwd inside its try for the same reason). - There is no provider-side `is_dir()` guard: a directory that vanishes after the engine's check surfaces as the SDK's `CLIConnectionError("Working directory does not exist: ")`, wrapped in `ProviderError`. That is only defensible because `_classify_startup_failure` special-cases it — `CLIConnectionError` otherwise yields firewall/binary advice and `is_retryable=True`, which is wrong for all three launch failures (missing dir; path is a file → `ENOTDIR`; unreadable dir → `EACCES`; the latter two reach the SDK's generic "Failed to start Claude Code" arm, not its dedicated one). - - Knock-on effects: cwd is the project key for the CLI's on-disk transcript directory, and it is where a project `.mcp.json` and `.claude/` tree would be looked for. The unconditional `strict_mcp_config=True` stops a `.mcp.json` there from injecting undeclared servers, and `setting_sources=[]` (empty by default; see **Skills** below) stops the CLI loading `CLAUDE.md`, project settings, and hooks from it — so cwd no longer drags ambient instructions in unless the workflow opts in via `runtime.provider.setting_sources`, which is exactly a request to load them from that directory. `add_dirs` (the SDK's `--add-dir` passthrough) is a separate axis Conductor does not set. + - Knock-on effects: cwd is the project key for the CLI's on-disk transcript directory, and it is where a project `.mcp.json` and `.claude/` tree would be looked for. The unconditional `strict_mcp_config=True` stops a `.mcp.json` there from injecting undeclared servers, and `setting_sources=[]` (empty by default; see **Skills** below) stops the CLI loading `CLAUDE.md`, project settings, and hooks from it — so cwd no longer drags ambient instructions in unless the workflow opts in via `runtime.provider.setting_sources`, which is exactly a request to load them from that directory. `add_dirs` (the SDK's `--add-dir` passthrough) is a separate axis, carrying the per-agent `settings_dir` and nothing else — see **Target-repository skills** below. +- **Target-repository skills** (`settings_dir`): the per-agent `AgentDef.settings_dir` is the **only** source of `ClaudeAgentOptions.add_dirs`, and it selects which directory's *project* settings tier contributes skills. It exists because cwd was doing two conflicting jobs on this provider. The CLI supports the MCP Roots protocol and advertises exactly one root — its cwd — so `@modelcontextprotocol/server-filesystem` **discards the directories in its own argv** and permits cwd alone; cwd is simultaneously what the `project` tier resolves against. Narrowing cwd onto a target repository to reach its skills therefore narrowed the agent's only MCP root below any sibling path the step still had to read. Do not derive `add_dirs` from a server's directory arguments to compensate: `--add-dir` takes no part in Roots negotiation, it widens the CLI's own file tools, and doing so would also silently widen skill discovery to directories the author named as data. What `add_dirs` *does* do, measured: a named directory's `.claude/skills` become listed and invocable with cwd elsewhere entirely — and only those, not `CLAUDE.md`, `.claude/rules/*.md`, `.claude/settings.json` (so no `env` and no `hooks` — the hooks negative is measured with a side-effect probe whose control fires) or `.claude/agents`, which all keep following cwd. That makes it the *skills third* of what a cwd-resolved `project` tier loads, not a replacement for it. It carries a second, **unconditional** effect the skills framing hides: `add_dirs` is "additional directories Claude can access" per the SDK's own contract, so a `settings_dir` widens the model's built-in `Read`/`Edit`/`Bash` to that tree with no settings tier enabled at all (measured against `claude` CLI 2.1.263 at `permission_mode: "default"` with `setting_sources` unset: a read outside cwd is refused without it and succeeds with it; an agent omitting `tools:` runs under `bypassPermissions`, where the grant is unobservable because reads already succeed). It does not widen what an MCP server permits. `capabilities.py::settings_dir` gates the field and `config/validator.py` errors against a provider that cannot apply it — enforced **twice**, with `AgentExecutor._reject_unsupported_settings_dir` repeating it at run time, because `conductor run` never calls the static validator — warning when the agent's session will not enable the `project` tier (also twice: `config/validator.py` at validate, and `claude_agent_sdk.py::execute` at run time, since the filesystem grant applies even when the skills half no-ops; the run-time half is latched per `(resolved directory, cause)` — not per agent, because a `for_each` member is renamed per item and would otherwise warn once per iteration, and not per directory alone, because the remedy differs by cause and one line would prescribe a fix wrong for the agent it does not name; two agents sharing a directory *and* a cause do collapse to one line). The opt-out remedy assumes the `project` tier is otherwise present: an agent with `skills: []` under a `user`/`local`-only tier is told to remove the opt-out, which is necessary but not sufficient, so that author reaches a working config in two steps rather than one. Deliberate -- distinguishing it needs a third cause value and a wider latch key to serve a combination requiring two unusual settings at once. `WorkflowEngine._resolve_agent_directory` resolves both fields so they cannot drift, and `settings_dir` is per-agent only (no `runtime.` counterpart: the repository whose conventions apply is what varies between steps). `tests/test_integration/test_mcp_roots_negotiation.py` pins the negotiation rule itself against the real server with no LLM — two runs differing only in whether the client advertises `roots` — since every option here rests on it (marked `real_api`: it fetches the server from npm and pins *upstream's* behaviour, not Conductor's). The resolved value is emitted on `agent_started` / `parallel_agent_started` / `for_each_agent_started` alongside `working_dir`, because it is a trust decision — another repository's conventions plus a filesystem grant — and one the run output would otherwise never mention. - **Skills** (issue #352): `supports_native_skills=True`. Skills are enabled through the SDK, not prompt injection, and three options move together in `execute`: - `plugins=[{"type": "local", "path": }]` + `skills=[":"]`. The SDK has no skill-*directory* surface, so `_resolve_skill_plugins` maps each directory back to the plugin that owns it via `skills/registry.py::resolve_skill_plugin`. That resolver is deliberately strict, because every one of these mistakes otherwise produces a name the CLI silently resolves to nothing: it bounds the upward walk (`_PLUGIN_SEARCH_DEPTH`), requires the skill to actually live under the candidate's `skills/` directory, requires `SKILL.md` to exist and its frontmatter `name` to equal the directory name (the CLI resolves by frontmatter name; Conductor sends the directory name), and rejects names outside `[A-Za-z0-9_.-]+` since they are joined into a comma-delimited `--allowedTools` value. A skill under no plugin root returns `None`; a plugin that is present but unusable raises `SkillPluginError`, which the provider re-raises as a `ProviderError` carrying the real reason rather than a blanket "not part of a plugin". Two plugins claiming one qualified name are refused too — deduping the clash away would drop a declared skill. All of it is `is_retryable=False`: these never become valid on a retry, and a checkout path containing "connection" would otherwise trip the retryability heuristic. Note providers are constructed lazily, so this surfaces when the first agent on this provider runs, **not** at `conductor validate`. - `setting_sources=[]` **by default**, for the same reason `strict_mcp_config=True` is unconditional a few lines away — but opt-in per workflow via `runtime.provider.setting_sources` (`user`/`project`/`local`, issue #501). Left unset, the CLI loads user, project, and local settings, which between them bring in ambient skills, `CLAUDE.md`, and hooks the workflow never declared, varying by machine and launch directory. Conductor surfaces instruction files through its own opt-in `--workspace-instructions`; settings and hooks have no equivalent. `skills=[]` and `skills=None` are **not** interchangeable upstream: `None` means "CLI defaults apply", and setting `skills` while leaving `setting_sources` at `None` makes the SDK re-default it to `["user", "project"]` — so the two options are coupled and dropping either re-opens the issue. The `[]` is also invisible in argv (it travels in the SDK's `initialize` control request), which is why the argv-based tests are paired with options-level assertions. The list is a context filter, not a sandbox: undeclared skills are hidden from the model's listing and rejected by the `Skill` tool, but their files stay readable. Opting a workflow in (`runtime.provider.setting_sources: [project]`, the motivating case being an agent whose `working_dir` is a *target* repo shipping its own `.claude/skills` — the CLI has `--plugin-dir` but no `--skill-dir`) loads that tier **with its hooks**, so it is only for repositories trusted as much as the workflow itself, and the field is workflow-global while `working_dir` is per agent. Two couplings follow: `_resolve_skill_filter` resolves `skills` to `"all"` when a tier is enabled and the workflow named none itself (tier-discovered skills never pass through `skill_names`, so `[]` would load the repo's skills and then hide every one), and a per-agent `skills: []` opts that agent out of the tier entirely — hooks included — keeping it the one opt-out. diff --git a/CHANGELOG.md b/CHANGELOG.md index 001881ff..9528fcf8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -47,6 +47,25 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 entirely. See [`examples/claude-agent-sdk-setting-sources.yaml`](examples/claude-agent-sdk-setting-sources.yaml). +- **Per-agent `settings_dir` on `claude-agent-sdk`** (#513) — selects which + directory's `project` settings tier supplies an agent's **skills**, + independently of `working_dir`. The CLI advertises exactly one MCP root — + its cwd — and a filesystem MCP server that sees a Roots-capable client + discards the directories in its own argv, so pointing `working_dir` at a + target repository to pick up its skills also narrowed the agent's only MCP + root onto it. `settings_dir` splits the two, letting cwd stay wide enough + for every path the agent must read. It carries a second, unconditional + effect: `add_dirs` widens the model's built-in `Read`/`Edit`/`Bash` to that + tree regardless of any settings tier, though no Conductor configuration + reaches a permission mode where that is observable today. Only the skills + of that directory travel — not `CLAUDE.md`, `.claude/rules/*.md`, + `.claude/settings.json` or `.claude/agents`, all measured. Refused at + `conductor validate` *and* at run time on a provider that cannot apply it; + a `settings_dir` whose `project` tier is not enabled warns in both places + too, since the filesystem grant applies even when the skills half no-ops. + Reported on the agent lifecycle events so the grant is auditable. See + [`examples/claude-agent-sdk-settings-dir.yaml`](examples/claude-agent-sdk-settings-dir.yaml). + ### Fixed - **Context compaction window guard against token-dense drift** (#507) — the @@ -76,6 +95,17 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 (`reason: "estimate_unavailable"`) rather than vanishing into stderr. See [Workflow Syntax → Context Compaction](docs/workflow-syntax.md#context-compaction). +### Changed + +- **A `working_dir` or `settings_dir` template that renders empty is now an + error** (#513). Previously an empty render resolved to the workflow file's + own directory — `Path("")` is `Path(".")`, which is not absolute, so it was + joined onto that directory and passed the existence check — and the agent + ran there. A value meaning "nothing" silently becoming something real is + the defect; for `settings_dir` it would also have granted the model access + to the workflow's own tree. Both fields now fail before the provider call, + naming the field and the template it came from. + ## [0.1.36](https://github.com/microsoft/conductor/compare/v0.1.35...v0.1.36) - 2026-09-02 ### Added diff --git a/docs/providers/experimental.md b/docs/providers/experimental.md index f849ee9b..b6df0da3 100644 --- a/docs/providers/experimental.md +++ b/docs/providers/experimental.md @@ -101,7 +101,7 @@ adopting one does not inflate the install surface for others. | Provider | Upstream pin | Maintainer | Capability carve-outs | |---|---|---|---| -| `claude-agent-sdk` | `claude-agent-sdk>=0.2.82` | `@lesandiz (best-effort)` | no `workflow_tools_passthrough`, no `reasoning_effort`, `prompt_injection` structured output, no `checkpoint_resume` (agents without a `session_key` carry no session state across a resume). Supports `mcp_tools` as of [#335](https://github.com/microsoft/conductor/issues/335), except that a narrowing per-server `tools:` filter is refused (no SDK equivalent). Supports `working_dir` as of [#348](https://github.com/microsoft/conductor/issues/348); the CLI would load `CLAUDE.md` and `.claude/settings*.json` from that directory, but `setting_sources` is empty by default as of [#352](https://github.com/microsoft/conductor/issues/352) so ambient instructions, settings, hooks, and skills are not inherited unless a workflow opts in via `runtime.provider.setting_sources` ([#501](https://github.com/microsoft/conductor/issues/501)) — which loads the named tiers **including their hooks**, so only for repositories trusted as much as the workflow. Declares `session_continuity`: an agent with a `session_key` reuses one Claude session across executions, and the session map survives `conductor resume` — see [Session Continuity](../workflow-syntax.md#session-continuity-session_key). | +| `claude-agent-sdk` | `claude-agent-sdk>=0.2.82` | `@lesandiz (best-effort)` | no `workflow_tools_passthrough`, no `reasoning_effort`, `prompt_injection` structured output, no `checkpoint_resume` (agents without a `session_key` carry no session state across a resume). Supports `mcp_tools` as of [#335](https://github.com/microsoft/conductor/issues/335), except that a narrowing per-server `tools:` filter is refused (no SDK equivalent). Supports `working_dir` as of [#348](https://github.com/microsoft/conductor/issues/348); the CLI would load `CLAUDE.md` and `.claude/settings*.json` from that directory, but `setting_sources` is empty by default as of [#352](https://github.com/microsoft/conductor/issues/352) so ambient instructions, settings, hooks, and skills are not inherited unless a workflow opts in via `runtime.provider.setting_sources` ([#501](https://github.com/microsoft/conductor/issues/501)) — which loads the named tiers **including their hooks**, so only for repositories trusted as much as the workflow. Which directory that `project` tier reads **skills** from is chosen per agent with `settings_dir` (cwd alone governs the CLI's sole MCP root, so the two are deliberately separate) — see [Target-Repository Skills](../workflow-syntax.md#target-repository-skills-settings_dir). Declares `session_continuity`: an agent with a `session_key` reuses one Claude session across executions, and the session map survives `conductor resume` — see [Session Continuity](../workflow-syntax.md#session-continuity-session_key). | | `hermes` | `hermes-agent` | `(community contribution)` | no `mcp_tools`, `prompt_injection` structured output, no `working_dir` | | `aca` | `azure-identity>=1.19.0` | `(unassigned)` | no `workflow_tools_passthrough` (the wrapped in-container `CopilotProvider` never applies the `tools:` allowlist to the SDK session), no `working_dir` (only the separate, container-relative `sandbox.working_dir` is honored — not the generic host-resolved field), `prompt_injection` structured output (inherits the inner Copilot provider), no `checkpoint_resume` (ephemeral sandbox sessions, no volume mount). Declares `interrupt`/`max_session_seconds` as `True`, but the shipped runner MVP doesn't fully back either yet — see [Known Gaps](./aca.md#known-gaps-runner-mvp). | diff --git a/docs/workflow-syntax.md b/docs/workflow-syntax.md index 8de7b9ce..a7763744 100644 --- a/docs/workflow-syntax.md +++ b/docs/workflow-syntax.md @@ -402,6 +402,128 @@ 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/skills` the agent may +use, and whose tree the model's built-in file tools may read. It carries the +*skills* third of a Claude Code `project` settings tier and nothing else of it +— the table below is exact about which — and the filesystem half applies +whether or not any tier is enabled. It applies only to `claude-agent-sdk` agents. +Setting it against any other provider is an **error**, reported by `conductor +validate` and again at run time — not a silently dropped field. The skills half +additionally requires `runtime.provider.setting_sources` to enable the `project` +tier; the filesystem grant below applies either way. + +```yaml +workflow: + runtime: + provider: + name: claude-agent-sdk + setting_sources: [project] + +agents: + - name: judge + settings_dir: "{{ setup_worktree.output.worktree_path }}" + prompt: Review the change against this repository's conventions. +``` + +#### Why it is separate from `working_dir` + +An agent's cwd does two unrelated jobs, and on this provider they conflict. +The `claude` CLI supports the MCP Roots protocol and advertises exactly one +root — its cwd. A filesystem MCP server therefore **discards the directories +in its own argv** and permits cwd alone; `--add-dir` takes no part in that +negotiation, so it cannot widen what a server allows. cwd is simultaneously +the directory the `project` settings tier resolves against. + +So pointing `working_dir` at a target repository to pick up that repository's +skills also narrows the agent's only MCP root onto it, and any sibling path +the step still has to read — an artifacts directory, a second checkout — is +denied. Widening cwd back loses the repository's conventions. + +`settings_dir` splits the two. Skills are discovered from cwd **and** from +`settings_dir`, so cwd can stay wide enough to contain everything the agent +must read: + +```yaml +agents: + - name: judge + # No working_dir: cwd stays the launch directory, which contains both the + # worktree and the artifacts this judge reads through the filesystem MCP. + settings_dir: "{{ setup_worktree.output.worktree_path }}" +``` + +#### What it does and does not carry + +Measured against the CLI: + +| Named via `settings_dir` | Granted? | +|---|---| +| **Filesystem access for the model's built-in tools** (`Read`, `Edit`, `Bash`, …) | **yes — unconditionally**, see below | +| `.claude/skills` | **yes** — listed and invocable | +| `CLAUDE.md` | no | +| `.claude/rules/*.md` | no | +| `.claude/settings.json` `env` | no | +| `.claude/settings.json` `hooks` | no — measured, see below | +| `.claude/agents` | no | + +> ⚠️ **The filesystem grant does not depend on `setting_sources`.** This field +> maps to the SDK's `add_dirs`, whose own contract is *"additional directories +> Claude can access beyond the current working directory"* — so naming a +> directory here widens the model's built-in file tools to that tree whether or +> not any settings tier is enabled. Measured against `claude` CLI 2.1.263 at +> `permission_mode: "default"` with `setting_sources` unset: a read outside +> cwd is refused without `settings_dir` and succeeds with it. (Later CLI +> builds no longer accept that mode by name; Conductor never passes it +> explicitly, so the reproduction needs the version above.) Note an agent that omits `tools:` runs +> under `bypassPermissions`, where reads already succeed everywhere, so the +> grant only becomes observable once permissions are in play. +> +> Skill discovery is the *reason* to set this field; the filesystem grant is +> its unavoidable companion. Point it at a directory the agent is entitled to +> read. +> +> `conductor validate` warns when `settings_dir` is set without the `project` +> tier enabled, and so does the run itself — otherwise the only effect an +> author would get is the one they did not ask for. + +Note this grant is for the model's **built-in** tools only. It does not widen +what a filesystem MCP server permits — that stays cwd alone, which is the +whole reason this field exists. + +**The `hooks` row is a measured negative.** A `PreToolUse` hook that appends +to a file (an observable side effect, not a log line) runs when `working_dir` +is the repository and the `project` tier is enabled, and does **not** run when +the same repository is reached only through `settings_dir` — with or without a +tier enabled. The control firing is what makes the negative meaningful. + +Setting aside the filesystem grant, this field is the *skills portion* of a +project tier, not a cwd-independent way to load one. It cuts favourably in one direction — +a target repository's skills arrive without its hooks also running — but it +does not compose with `working_dir` into "everything, anywhere": + +> An agent that needs a target repository's **rules or instructions** as well +> as a cwd wide enough for its MCP servers cannot get both from these fields. +> One directory cannot be narrow and wide at once. `settings_dir` recovers the +> skills; anything else is a caller-side trade — keep `working_dir` on the +> repository and arrange for every path the agent reads to sit beneath it. + +#### Resolution and restrictions + +- Resolved exactly like `working_dir` — Jinja2-rendered, `~`-expanded, + relative paths resolved against the workflow file's directory, normalized + with `os.path.normpath`, and existence-checked before any provider call. +- Per-agent only. There is no `runtime.settings_dir`, because the repository + whose conventions apply is what varies between steps. +- Rejected on `wait`, `set`, `terminate`, `script`, `human_gate`, `questions` + and `workflow` step types — none has an LLM session to apply a settings tier + to, and accepting it silently would suggest conventions had been loaded when + none had. + +> ⚠️ A settings tier brings everything that tier defines. Enable +> `setting_sources` and point `settings_dir` only at repositories trusted to +> the same degree as the workflow itself. + ### Session Continuity (`session_key`) By default each agent execution starts a fresh provider session, so an agent diff --git a/examples/claude-agent-sdk-settings-dir.yaml b/examples/claude-agent-sdk-settings-dir.yaml new file mode 100644 index 00000000..658531aa --- /dev/null +++ b/examples/claude-agent-sdk-settings-dir.yaml @@ -0,0 +1,155 @@ +# 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 + + input: + workspace: + type: string + required: true + description: > + Wide directory containing BOTH the repository under review and the + artifacts directory. Becomes every agent's cwd, and therefore the + filesystem MCP server's only root. + repo: + type: string + required: true + description: > + The repository under review, inside `workspace`. Its own + `.claude/skills` are loaded via `settings_dir`. + artifacts: + type: string + required: true + description: > + Prior findings, inside `workspace` but OUTSIDE `repo` -- the path that + a narrowed cwd would make unreadable. + + runtime: + provider: + name: claude-agent-sdk + # Opt in to the `project` tier. Off by default, so a run does not + # inherit whatever the machine happens to have installed. A tier brings + # everything it defines: enable it only for repositories trusted to the + # same degree as this workflow. + setting_sources: [project] + default_model: claude-sonnet-4-5 + + # cwd for every agent: the wide directory. The filesystem MCP server's + # argv roots below are discarded by the server itself (see the header), so + # THIS is what actually decides what the agent can read. + working_dir: "{{ workflow.input.workspace }}" + + mcp_servers: + filesystem: + type: stdio + command: npx + args: + - "-y" + - "@modelcontextprotocol/server-filesystem" + # Declared for a client that does not negotiate Roots. The `claude` + # CLI does, so it permits cwd alone and these are ignored -- kept + # because they are still the honest declaration of intent, not + # because they take effect here. + - "{{ workflow.input.repo }}" + - "{{ workflow.input.artifacts }}" + +agents: + - name: review + # The narrow half: this repository's `.claude/skills` become listed and + # invocable, with cwd still the wide workspace above. Templated, since the + # directory under review is normally an upstream step's output. + settings_dir: "{{ workflow.input.repo }}" + prompt: | + You are reviewing the repository at {{ workflow.input.repo }}. + + First, list the skills available to you. Any skill this repository ships + in its own `.claude/skills` should be among them -- that is what + `settings_dir` provides, and you should follow its conventions rather + than any you would apply by default. + + Then read the prior findings under {{ workflow.input.artifacts }} using + the filesystem MCP server. That directory sits outside the repository, + so a run that had narrowed the working directory onto the repository + could not read it at all. + + Report: + - which repository-supplied skills you found + - what the prior findings say + - whether the repository's conventions change your reading of them + output: + skills_found: + type: string + description: Repository-supplied skills the session offered. + findings_summary: + type: string + description: What the artifacts directory contained. + routes: + - to: "$end" + +output: + skills_found: "{{ review.output.skills_found }}" + findings_summary: "{{ review.output.findings_summary }}" diff --git a/src/conductor/config/schema.py b/src/conductor/config/schema.py index c84d17b1..cc0b2cef 100644 --- a/src/conductor/config/schema.py +++ b/src/conductor/config/schema.py @@ -1381,6 +1381,83 @@ class AgentDef(BaseModel): wait/set/terminate/human_gate/workflow step types. """ + settings_dir: Annotated[str, StringConstraints(strip_whitespace=True, min_length=1)] | None = ( + None + ) + """Directory whose ``.claude/skills`` this agent may use, and whose tree the + model's built-in file tools may read. + + Both halves of that first line are deliberate: this carries the *skills* + third of a Claude Code ``project`` settings tier and nothing else of it, + and it widens the model's filesystem access unconditionally. Details below. + + ``claude-agent-sdk`` only -- a provider that cannot apply it refuses it + both at ``conductor validate`` and at run time, rather than dropping it + silently (``conductor run`` never calls the static validator). Resolved + by the engine exactly like :attr:`working_dir` (Jinja-rendered, + ``~``-expanded, made absolute against the workflow file's directory, + ``normpath``-normalised, existence-checked), then forwarded to the SDK as + ``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; both ``conductor validate`` and the run itself warn when this field + is set without it, since the skills half is then a no-op and + ``conductor run`` never calls the static validator. The *filesystem* grant is + unconditional: ``add_dirs``' own SDK contract is "additional directories + Claude can access beyond the current working directory", so naming a + directory here widens the model's built-in ``Read``/``Edit``/``Bash`` + 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. + + That grant is currently latent rather than reachable from a workflow: + Conductor runs this provider either with the full ``claude_code`` preset + under ``bypassPermissions`` (``tools:`` omitted), where reads already + succeed everywhere, or with ``tools: []``, where the model holds at most + the ``Skill`` loader and no file tool at all. So it is a property of the + SDK contract to design against rather than an exposure today. Point it + only at a directory the agent may read. + + It exists because ``working_dir`` was doing two unrelated jobs. The CLI + supports MCP Roots and advertises exactly one root — its cwd — so a + 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: + - name: judge + settings_dir: "{{ setup_worktree.output.worktree_path }}" + # No working_dir: cwd stays the launch directory, which contains + # both the worktree and the artifacts the judge must read. + """ + stdin: str | None = None """Payload written to the script subprocess's stdin (script type only). @@ -2030,6 +2107,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 is not None: + raise ValueError("human_gate agents cannot have 'settings_dir'") if self.session_key is not None: raise ValueError("human_gate agents cannot have 'session_key'") elif self.type == "questions": @@ -2093,6 +2172,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 is not None: + raise ValueError("questions agents cannot have 'settings_dir'") if self.session_key is not None: raise ValueError("questions agents cannot have 'session_key'") elif self.type == "script": @@ -2126,6 +2207,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 is not None: + raise ValueError("script agents cannot have 'settings_dir'") if self.max_depth is not None: raise ValueError("script agents cannot have 'max_depth'") if self.reasoning is not None: @@ -2192,6 +2275,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 is not None: + raise ValueError("workflow agents cannot have 'settings_dir'") elif self.type == "wait": if self.duration is None: raise ValueError("wait agents require 'duration'") @@ -2215,6 +2300,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 is not None: + raise ValueError("wait agents cannot have 'settings_dir'") if self.timeout is not None: raise ValueError("wait agents cannot have 'timeout'") if self.workflow: @@ -2288,6 +2375,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 is not None: + raise ValueError("set agents cannot have 'settings_dir'") if self.timeout is not None: raise ValueError("set agents cannot have 'timeout'") if self.workflow: @@ -2362,6 +2451,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 is not None: + raise ValueError("terminate agents cannot have 'settings_dir'") if self.timeout is not None: raise ValueError("terminate agents cannot have 'timeout'") if self.timeout_seconds is not None: @@ -2699,11 +2790,19 @@ class ProviderSettings(BaseModel): installed, so a run reproduces on another developer's laptop. Set it to opt a workflow back in. The motivating case is an agent working - inside a *target* repository that ships its own ``.claude/skills`` — with - ``working_dir`` pointed at that repo, ``["project"]`` loads that repo's - skills and instructions natively, without the repo needing to package - them as a Claude Code plugin (the CLI has ``--plugin-dir`` but no - ``--skill-dir``, so a plugin root is otherwise the only handle). + against a *target* repository that ships its own ``.claude/skills``: + ``["project"]`` loads that repository's skills natively, without it + needing to package them as a Claude Code plugin (the CLI has + ``--plugin-dir`` but no ``--skill-dir``, so a plugin root is otherwise + the only handle). + + Which directory the ``project`` tier reads is chosen per agent. Skills + come from cwd (:attr:`AgentDef.working_dir`) *and* from + :attr:`AgentDef.settings_dir`; everything else the tier defines -- + ``CLAUDE.md``, ``.claude/settings.json``, ``.claude/agents`` — follows + cwd alone. Prefer ``settings_dir`` when the agent also needs a wider cwd: + the CLI advertises cwd as its sole MCP root, so narrowing cwd onto the + target repository narrows what the agent's MCP servers may read. Each tier brings everything that tier defines, hooks included: ``project`` reads ``/.claude/settings.json``, whose ``hooks`` entries run diff --git a/src/conductor/config/validator.py b/src/conductor/config/validator.py index 90615827..1442cf15 100644 --- a/src/conductor/config/validator.py +++ b/src/conductor/config/validator.py @@ -1272,6 +1272,11 @@ def _collect_template_strings( templates.append((f"agent '{agent.name}' args[{i}]", arg)) if agent.working_dir: templates.append((f"agent '{agent.name}' working_dir", agent.working_dir)) + # getattr for the same reason the 'set' bindings below use it: duck-typed + # test fixtures predate this field and would raise on direct access. + settings_dir = getattr(agent, "settings_dir", None) + if settings_dir: + templates.append((f"agent '{agent.name}' settings_dir", settings_dir)) # 'set' step bindings — value: single expression, values: named expressions. # Use getattr so duck-typed test fixtures without these attributes still @@ -1807,6 +1812,25 @@ def _is_llm_agent(agent: AgentDef) -> bool: return agent.type in _LLM_AGENT_TYPES +def _project_tier_enabled(config: WorkflowConfig, agent: AgentDef) -> bool: + """True iff this agent's session will enable the ``project`` settings tier. + + ``settings_dir`` feeds the ``project`` tier and nothing else, so any tier + is not enough: ``user`` reads ``~/.claude`` and ``local`` is cwd-bound, so + neither can make a ``settings_dir``'s skills discoverable. A per-agent + ``skills: []`` opts the agent out of the tiers entirely + (``claude_agent_sdk.py::execute`` computes ``effective_sources`` that way), + so the check is per agent rather than per workflow. + + ``runtime.provider`` is either the bare string shorthand (no tiers, by + definition) or a ``ProviderSettings`` carrying ``setting_sources``. + """ + if agent.skills == []: + return False + provider = config.workflow.runtime.provider + return "project" in (getattr(provider, "setting_sources", None) or []) + + def _resolved_provider_name(agent: AgentDef, default: str) -> str: """The provider name an agent will actually use at runtime. @@ -2453,6 +2477,58 @@ 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 _project_tier_enabled(config, agent): + # A warning, not an error: the FILESYSTEM half of settings_dir + # applies regardless, so the workflow is not broken -- but the + # skill discovery it is normally set for is a no-op without the + # project tier enabled, and a green validate would imply otherwise. + # + # Three distinct causes, each with a different remedy (or none), so + # the message branches rather than prescribing one fix that may be + # impossible to apply. + common = ( + f"Agent '{agent.name}' sets settings_dir={agent.settings_dir!r} but " + f"its session will not enable the 'project' settings tier, so no " + f"skills will be discovered from that directory. The directory is " + f"still granted to the model's built-in file tools." + ) + if agent.skills == []: + warnings.append( + f"{common} The agent's own 'skills: []' opts it out of the " + f"settings tiers entirely. Remove it to let the tier apply, or " + f"remove settings_dir if the filesystem grant was not intended." + ) + elif provider_name != default_provider: + # setting_sources lives on the single workflow-level + # ProviderSettings and the schema rejects it unless that + # provider is claude-agent-sdk, so telling this author to add + # it would produce a ValidationError. + warnings.append( + f"{common} The settings tier is workflow-scoped " + f"(runtime.provider.setting_sources) and cannot be enabled for " + f"an agent that overrides its provider, since the schema " + f"accepts setting_sources only when runtime.provider is " + f"'claude-agent-sdk' (it is {default_provider!r}). Move " + f"the provider to runtime.provider to enable the tier, or keep " + f"settings_dir for the filesystem grant alone." + ) + else: + warnings.append( + f"{common} Add 'project' to runtime.provider.setting_sources to " + f"load that repository's skills." + ) + # session_key: a provider that ignores it starts a fresh session every # execution, silently discarding the context the author asked to keep. if agent.session_key is not None and not caps.session_continuity: diff --git a/src/conductor/engine/validator.py b/src/conductor/engine/validator.py index 611ea32d..0d441a4d 100644 --- a/src/conductor/engine/validator.py +++ b/src/conductor/engine/validator.py @@ -227,6 +227,24 @@ def _build_validator_agent(self, agent: AgentDef) -> AgentDef: tools=[], output=_VALIDATOR_OUTPUT_SCHEMA, working_dir=agent.working_dir, + # Deliberately NOT inherited, unlike working_dir. This grader + # runs with ``tools=[]``, which yields at most the ``Skill`` + # loader and never Read/Edit/Bash -- so the filesystem half of + # settings_dir has no file tool to widen, and grading an output + # against a rubric needs no skills. Inheriting it would hand the + # grader a tree it has no way to use: a wider grant than the run + # needs. + # + # (Not "no skill can be invoked": with a settings tier enabled the + # grader does hold the ``Skill`` tool, since + # ``_resolve_tool_config`` grants it back for ``tools: []``.) + # + # Built field by field rather than by ``model_copy`` for the same + # reason: a copy would also carry ``validator`` (making the grader + # validate itself), ``session_key`` (two sessions appending to one + # transcript, which config/validator.py refuses for concurrent + # executions) and ``routes``. Add new fields here explicitly. + settings_dir=None, ) def _parse(self, content: Any) -> tuple[bool, list[str], bool]: diff --git a/src/conductor/engine/workflow.py b/src/conductor/engine/workflow.py index 2efd7cfc..902201e1 100644 --- a/src/conductor/engine/workflow.py +++ b/src/conductor/engine/workflow.py @@ -17,7 +17,7 @@ import uuid from dataclasses import dataclass, field from pathlib import Path -from typing import TYPE_CHECKING, Any +from typing import TYPE_CHECKING, Any, Literal from conductor.duration import parse_duration from conductor.engine.checkpoint import CheckpointManager, CheckpointTrigger @@ -593,29 +593,56 @@ 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: Literal["working_dir", "settings_dir"], + 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). + + Applies to ``working_dir`` as well as ``settings_dir``: a value that + renders empty previously resolved to the workflow file's own directory + and ran there, which is the same footgun in a quieter form, so both + are refused. + Raises: + ExecutionError: if the value renders empty, or if the resolved + path is not an existing directory — both before any provider + call. + """ rendered = self.renderer.render(raw, agent_context) + # A template can render empty even when the raw value passed the schema's + # min_length -- an --input given as `repo=`, a script step that printed + # nothing, a `set` binding evaluating to "". Path("") is Path("."), which + # is not absolute, so it would join onto the workflow file's own directory + # and pass the is_dir() check below: a value meaning "nothing" silently + # becoming somewhere real. + # + # Refused for BOTH fields, deliberately. settings_dir is the dangerous + # one -- there the workflow's own tree would be handed to the model's + # file tools -- but working_dir running an agent in the wrong directory + # is the same defect without the grant, so neither is worth keeping. + if not rendered.strip(): + raise ExecutionError( + f"Agent '{agent.name}': {field} rendered to an empty string from '{raw}'", + agent_name=agent.name, + suggestion=( + f"An empty value would resolve to the workflow file's own " + f"directory. Check that the input or upstream step feeding " + f"{field} produced a path." + ), + ) path = Path(rendered).expanduser() if not path.is_absolute(): base = self._workflow_dir if self._workflow_dir is not None else Path.cwd() @@ -624,16 +651,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 + + 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, field="working_dir", raw=raw + ) + + if agent.settings_dir is not None: + update["settings_dir"] = self._resolve_agent_directory( + agent, agent_context, field="settings_dir", raw=agent.settings_dir + ) - return agent.model_copy(update={"working_dir": resolved}) + if not update: + return agent + return agent.model_copy(update=update) def _build_pricing_overrides(self) -> dict[str, ModelPricing] | None: """Build pricing overrides from workflow cost configuration. @@ -4426,7 +4494,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. @@ -4458,6 +4526,13 @@ async def _execute_loop(self, current_agent_name: str) -> dict[str, Any]: } if is_llm_agent: started_payload["working_dir"] = resolved_agent.working_dir + # Emitted alongside working_dir because it is a + # trust decision: settings_dir loads another + # repository's conventions AND widens the model's + # built-in file tools to that tree. A grant the + # dashboard and the JSONL log never mention cannot + # be audited after the fact. + started_payload["settings_dir"] = resolved_agent.settings_dir self._emit("agent_started", started_payload) # Handle terminate steps — explicit workflow exit with a @@ -6157,7 +6232,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) @@ -6171,6 +6246,7 @@ async def execute_single_agent(agent: AgentDef) -> tuple[str, Any]: "group_name": parallel_group.name, "agent_name": agent.name, "working_dir": resolved_agent.working_dir, + "settings_dir": resolved_agent.settings_dir, }, ) @@ -6643,7 +6719,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) @@ -6659,6 +6735,7 @@ async def execute_single_item(item: Any, index: int, key: str) -> tuple[str, Any "agent_name": qualified_agent.name, "item_key": key, "working_dir": qualified_agent.working_dir, + "settings_dir": qualified_agent.settings_dir, }, ) diff --git a/src/conductor/executor/agent.py b/src/conductor/executor/agent.py index 21381f81..d3f98602 100644 --- a/src/conductor/executor/agent.py +++ b/src/conductor/executor/agent.py @@ -381,6 +381,7 @@ async def execute( # decided by the agent and the provider alone, so there is nothing to # learn from rendering a prompt or calling a model first. self._reject_unsupported_session_key(agent) + self._reject_unsupported_settings_dir(agent) # Render model field if it contains template expressions if is_jinja_template(agent.model): @@ -1130,3 +1131,45 @@ def _reject_unsupported_session_key(self, agent: AgentDef) -> None: "reports this before a run starts." ), ) + + def _reject_unsupported_settings_dir(self, agent: AgentDef) -> None: + """Refuse ``settings_dir`` on a provider that cannot apply it. + + Mirrors the ``capabilities.settings_dir`` check in + :func:`conductor.config.validator.validate_workflow_config`. + ``conductor validate`` already rejects the combination, but + ``conductor run`` never invokes the static validator — and the + engine renders, absolutizes and existence-checks the directory for + *every* provider, so an author sees the field processed and then + handed to a provider that never reads it. The agent answers from + whatever conventions its cwd happened to supply and the run exits 0, + which is the silent-wrong-answer case the capability exists to stop. + + A provider with no ``CAPABILITIES`` is left alone, for the reason + given in :meth:`_reject_unsupported_skills`. + + Args: + agent: The agent whose ``settings_dir`` is being checked. Agents + that declare none return immediately. + + Raises: + ExecutionError: If the provider declares + ``capabilities.settings_dir=False``. + """ + if agent.settings_dir is None: + return + capabilities = getattr(type(self.provider), "CAPABILITIES", None) + if capabilities is None or capabilities.settings_dir: + return + raise ExecutionError( + f"Agent '{agent.name}' sets settings_dir={agent.settings_dir!r} but " + f"provider '{type(self.provider).__name__}' does not apply it " + f"(capabilities.settings_dir=False), so the agent would load " + f"whatever conventions its working directory supplies instead.", + agent_name=agent.name, + suggestion=( + "Use working_dir, or override the agent to a provider that " + "applies it (claude-agent-sdk). 'conductor validate' reports " + "this before a run starts." + ), + ) diff --git a/src/conductor/providers/capabilities.py b/src/conductor/providers/capabilities.py index 9d042762..94828c41 100644 --- a/src/conductor/providers/capabilities.py +++ b/src/conductor/providers/capabilities.py @@ -143,6 +143,22 @@ 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`` are refused twice -- by ``conductor validate`` and + again at run time by + :meth:`conductor.executor.agent.AgentExecutor._reject_unsupported_settings_dir`, + because ``conductor run`` never invokes the static validator. Both are + needed for the same reason: the field selects which repository's + conventions the agent loads *and* widens the model's built-in file tools + to that tree, so silently ignoring it would run the agent against the + wrong conventions while reporting success. Distinct from ``working_dir`` because the two are + deliberately independent axes -- cwd is the sole root a filesystem MCP + server gets, while this only adds a directory. Defaults to ``False`` + (conservative).""" + skills: bool = False """``True`` when the provider exposes :mod:`conductor.skills` content to the agent. The user-facing contract is the same regardless of diff --git a/src/conductor/providers/claude_agent_sdk.py b/src/conductor/providers/claude_agent_sdk.py index b1b6519e..3803864e 100644 --- a/src/conductor/providers/claude_agent_sdk.py +++ b/src/conductor/providers/claude_agent_sdk.py @@ -668,6 +668,9 @@ class ClaudeAgentSdkProvider(AgentProvider): # rather than being stamped individually as they are for Copilot: # the SDK's ``McpStdioServerConfig`` has no cwd field. working_dir=True, + # ``settings_dir`` reaches ``ClaudeAgentOptions.add_dirs``, the CLI's + # ``--add-dir``. It is the only provider that has anywhere to put it. + settings_dir=True, # Skills are loaded natively: the owning plugin is registered via # ``ClaudeAgentOptions.plugins`` and enabled by its qualified name # through ``skills``, so the model reads the frontmatter up front @@ -738,6 +741,28 @@ def __init__( # the point, and by cwd because the CLI stores transcripts per working # directory, so one key under two directories is two sessions. self._session_ids: dict[tuple[str, str], str] = {} + # settings_dir values already warned about for having no `project` + # tier, keyed by `(resolved directory, cause)`: + # + # - The directory, not the agent name: the engine renames a for_each + # member per item (`[]`, engine/workflow.py), so any + # name-bearing key emits one line per item -- the very case latching + # exists to prevent. All members of one loop share a directory and a + # cause, so they collapse to one line. + # - Not a bare flag: `settings_dir` is Jinja-rendered per execution, so + # one agent can name several directories across loop-backs, and each + # is a distinct grant the operator needs told about. + # - Plus the cause, because the remedy below depends on it: two agents + # can name the same directory for different reasons, and one line + # would prescribe a fix that is wrong for the other. Bounded at two + # lines per directory. + # + # The residual cost, accepted: two agents naming the same directory + # for the SAME reason warn once, naming only the first. The remedy is + # then identical for both, so the second line would add nothing. + # + # Matches the `_warned` convention in claude.py and engine/workflow.py. + self._settings_dir_tier_warned: set[tuple[str, bool]] = set() self._resume_session_ids: dict[tuple[str, str], str] = {} # Slots currently executing, so a second execution cannot resume a # session the first still has open — see :meth:`_claim_session_slot`. @@ -961,6 +986,51 @@ async def _execute_session( # reads off ``agent.tools``. effective_sources: list[SettingSource] = [] if agent.skills == [] else self._setting_sources + # A settings_dir whose `project` tier is not enabled discovers no + # skills -- and the filesystem grant applies anyway, so the one effect + # the author did not ask for is the only one they get. + # ``conductor validate`` warns about this, but ``conductor run`` never + # calls the static validator, so without this the run is silent about + # a no-op the author is relying on. Warned rather than raised, matching + # validate's own choice: the workflow is not wrong, just ineffective. + opted_out = agent.skills == [] + if ( + agent.settings_dir is not None + and "project" not in effective_sources + and (agent.settings_dir, opted_out) not in self._settings_dir_tier_warned + ): + self._settings_dir_tier_warned.add((agent.settings_dir, opted_out)) + # The remedy depends on the cause, as it does in + # config/validator.py: telling an author to add 'project' when + # their own `skills: []` is what zeroed the tier sends them to add + # a value that is already there, and the warning keeps firing. + # + # The other arm covers two of validator.py's causes at once -- a + # missing tier, and a per-agent provider override, where the tier + # cannot be enabled at all because the schema accepts + # `setting_sources` only when `runtime.provider` is + # 'claude-agent-sdk'. The provider does not know the + # workflow-level provider name, so the wording names the + # requirement rather than prescribing an edit that would be + # refused on that path. + remedy = ( + "This agent's own 'skills: []' opts it out of the settings tiers " + "entirely; remove it to let the tier apply" + if opted_out + else "Enable the 'project' tier via runtime.provider.setting_sources, " + "which requires runtime.provider itself to be 'claude-agent-sdk'" + ) + logger.warning( + "Agent '%s' sets settings_dir=%r but its session does not enable the " + "'project' settings tier, so no skills are discovered from that " + "directory. The directory is still granted to the model's built-in " + "file tools. %s, or remove settings_dir if the filesystem grant was " + "not intended.", + agent.name, + agent.settings_dir, + remedy, + ) + sdk_tools, permission_mode = self._resolve_tool_config( tools, agent, @@ -993,6 +1063,34 @@ async def _execute_session( # so pass it through verbatim rather than re-resolving — that would # collapse the symlink aliases the engine preserves. cwd=resolved_cwd, + # The authored ``settings_dir`` and nothing else. Two effects, + # and the order matters because the second is easy to miss. + # + # (1) UNCONDITIONAL: per the SDK's own contract this is + # "additional directories Claude can access beyond the current + # working directory", so this line widens the model's built-in + # Read/Edit/Bash to that tree with no settings tier enabled at + # all (measured). It does NOT widen what an MCP server permits. + # + # (2) CONDITIONAL on ``setting_sources`` enabling the ``project`` + # tier: the directory's ``.claude/skills`` become listed and + # invocable with cwd elsewhere entirely, and only those — not + # CLAUDE.md, .claude/rules/*.md, .claude/settings.json (so no + # env and no hooks) or .claude/agents, which all stay with cwd. + # So it is the skills portion of a project tier rather than a + # cwd-independent way to load one. + # + # Do not extend this to the directory args of stdio MCP servers to + # widen what those servers may read: it cannot work. A filesystem + # MCP server uses its argv directories only when the client does + # not support MCP Roots, and the CLI does support Roots — + # advertising exactly one, its cwd — so the server discards its + # argv directories and permits cwd alone. ``--add-dir`` takes no + # part in that negotiation; it widens the CLI's own file tools, + # never what a server permits. Measured: cwd alone is the + # effective allowlist whether or not every declared root is also + # passed here. + add_dirs=[agent.settings_dir] if agent.settings_dir else [], output_format=_build_output_format(agent.output) if agent.output else None, max_turns=max_turns, permission_mode=permission_mode, diff --git a/tests/test_config/test_set_schema.py b/tests/test_config/test_set_schema.py index 9581e734..f4587ab9 100644 --- a/tests/test_config/test_set_schema.py +++ b/tests/test_config/test_set_schema.py @@ -132,6 +132,7 @@ class TestSetAgentDefForbiddenFields: ("args", ["x"], "cannot have 'args'"), ("env", {"K": "v"}, "cannot have 'env'"), ("working_dir", "/tmp", "cannot have 'working_dir'"), + ("settings_dir", "/tmp", "cannot have 'settings_dir'"), ("timeout", 5, "cannot have 'timeout'"), ("workflow", "x.yaml", "cannot have 'workflow'"), ("input_mapping", {"a": "1"}, "cannot have 'input_mapping'"), diff --git a/tests/test_config/test_settings_dir_schema.py b/tests/test_config/test_settings_dir_schema.py new file mode 100644 index 00000000..9e6a0956 --- /dev/null +++ b/tests/test_config/test_settings_dir_schema.py @@ -0,0 +1,299 @@ +"""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": "questions", "questions": [{"id": "a", "text": "x"}]},), + ( + { + "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) + + +class TestEmptySettingsDirIsRefused: + """An empty or whitespace-only ``settings_dir`` is rejected at the schema. + + ``Path("")`` is ``Path(".")``, which is not absolute, so an empty value + would be joined onto the workflow file's own directory, pass the engine's + ``is_dir()`` check, and be forwarded as a real ``add_dirs`` entry -- the + grant is unconditional, so a value meaning "nothing" would hand the model + file access to the workflow's own tree. Rejecting at the type boundary is + what keeps the four layers (schema guards, engine, validator, provider) + agreeing on what "set" means. + """ + + @pytest.mark.parametrize("value", ["", " ", " ", "\t", "\n"]) + def test_blank_is_rejected(self, value: str) -> None: + with pytest.raises(ValidationError): + AgentDef(name="a", prompt="p", settings_dir=value) + + def test_surrounding_whitespace_is_stripped(self) -> None: + agent = AgentDef(name="a", prompt="p", settings_dir=" /repo ") + assert agent.settings_dir == "/repo" + + def test_a_blank_value_cannot_bypass_a_step_type_rejection(self) -> None: + """The step-type guards use ``is not None``, so "" cannot slip past. + + With truthiness guards and no schema constraint, ``settings_dir=""`` + was accepted on a ``wait`` step despite the documented rejection. + """ + with pytest.raises(ValidationError): + AgentDef(name="w", type="wait", duration="1s", settings_dir="") + + +class TestProjectTierWarningCauses: + """The no-skills warning must fire for every cause, with a usable remedy. + + ``settings_dir`` feeds the ``project`` tier and nothing else, so a check + for *any* tier stayed silent on ``['user']`` and ``['local']`` -- neither + of which can make a ``settings_dir``'s skills discoverable -- and on a + per-agent ``skills: []``, which zeroes the tier for that agent in + ``claude_agent_sdk.py::execute``. Each cause has a different remedy, and + one of them (a per-agent provider override) cannot be fixed by adding + ``setting_sources`` at all, so a single prescriptive message was advice + the author could not act on. + """ + + @staticmethod + def _warn(provider: object, agent_extra: dict, tmp_path: Path) -> str | None: + config = WorkflowConfig( + workflow=WorkflowDef( + name="w", + entry_point="a", + runtime=RuntimeConfig(provider=provider), # type: ignore[arg-type] + ), + agents=[ + AgentDef( + name="a", + prompt="hi", + settings_dir=str(tmp_path), + output={"r": OutputField(type="string")}, + routes=[RouteDef(to="$end")], + **agent_extra, + ) + ], + output={"r": "{{ a.output.r }}"}, + ) + hits = [w for w in validate_workflow_config(config) if "settings_dir" in w] + return hits[0] if hits else None + + def test_no_setting_sources_names_the_project_tier(self, tmp_path: Path) -> None: + warning = self._warn(ProviderSettings(name="claude-agent-sdk"), {}, tmp_path) + assert warning is not None + assert "setting_sources" in warning + + @pytest.mark.parametrize("tier", ["user", "local"]) + def test_a_non_project_tier_still_warns(self, tier: str, tmp_path: Path) -> None: + """``user`` reads ``~/.claude`` and ``local`` is cwd-bound.""" + warning = self._warn( + ProviderSettings(name="claude-agent-sdk", setting_sources=[tier]), # type: ignore[list-item] + {}, + tmp_path, + ) + assert warning is not None, f"{tier} tier cannot serve a settings_dir" + + def test_project_tier_is_silent(self, tmp_path: Path) -> None: + assert ( + self._warn( + ProviderSettings(name="claude-agent-sdk", setting_sources=["project"]), + {}, + tmp_path, + ) + is None + ) + + def test_agent_skills_opt_out_warns_and_names_skills(self, tmp_path: Path) -> None: + """``skills: []`` disables the tier for that agent, tier or not.""" + warning = self._warn( + ProviderSettings(name="claude-agent-sdk", setting_sources=["project"]), + {"skills": []}, + tmp_path, + ) + assert warning is not None + assert "skills: []" in warning + + def test_provider_override_does_not_advise_the_impossible(self, tmp_path: Path) -> None: + """``setting_sources`` is schema-rejected unless runtime.provider is the SDK. + + So telling an author with a per-agent override to add it produces a + ``ValidationError``; the warning must say the tier is workflow-scoped + instead. + """ + warning = self._warn("copilot", {"provider": "claude-agent-sdk"}, tmp_path) + assert warning is not None + assert "workflow-scoped" in warning + assert "Add 'project' to runtime.provider.setting_sources" not in warning diff --git a/tests/test_engine/test_validator.py b/tests/test_engine/test_validator.py index 3375faa1..857fca8b 100644 --- a/tests/test_engine/test_validator.py +++ b/tests/test_engine/test_validator.py @@ -138,6 +138,34 @@ def test_system_prompt_contains_criteria(self) -> None: v = OutputValidator()._build_validator_agent(agent) assert "VERY_SPECIFIC_RUBRIC" in (v.system_prompt or "") + def test_settings_dir_is_not_inherited(self) -> None: + """The grader must not inherit the primary agent's ``settings_dir``. + + ``working_dir`` IS inherited, so this is a deliberate asymmetry rather + than an omission: the grader runs with ``tools=[]``, which yields at + most the ``Skill`` loader and never Read/Edit/Bash, so the filesystem + grant would widen access to a tree it has no way to use. + + Pinned because the consequence is latent, not active: today there is + no file tool for the grant to widen, but ``_resolve_tool_config``'s + carve-outs have already grown once (the ``Skill`` grant-back). The + next time a tool is granted back to a ``tools: []`` agent this line + becomes load-bearing, and without this assertion nothing would notice + if it had drifted. + """ + agent = AgentDef( + name="reviewer", + prompt="x", + working_dir="/tmp", + settings_dir="/tmp", + validator=ValidatorConfig(criteria="check"), + ) + + v = OutputValidator()._build_validator_agent(agent) + + assert v.settings_dir is None + assert v.working_dir == "/tmp", "working_dir is still inherited" + def test_no_tools_and_has_output_schema(self) -> None: v = OutputValidator()._build_validator_agent(_agent()) assert v.tools == [] diff --git a/tests/test_engine/test_workflow.py b/tests/test_engine/test_workflow.py index 7679d2a8..8fcbe460 100644 --- a/tests/test_engine/test_workflow.py +++ b/tests/test_engine/test_workflow.py @@ -3583,6 +3583,7 @@ class _RecordingWorkingDirProvider: def __init__(self) -> None: self.seen: list[tuple[str, str | None]] = [] + self.seen_settings_dir: list[tuple[str, str | None]] = [] self.calls: int = 0 async def execute( @@ -3599,6 +3600,7 @@ async def execute( ): self.calls += 1 self.seen.append((agent.name, agent.working_dir)) + self.seen_settings_dir.append((agent.name, agent.settings_dir)) content = dict.fromkeys(agent.output or {}, f"{agent.name}-ok") return AgentOutput( content=content, @@ -3622,6 +3624,7 @@ def _single_agent_config( *, working_dir: str | None = None, runtime_working_dir: str | None = None, + settings_dir: str | None = None, model: str = "gpt-4", max_tokens: int | None = None, ) -> WorkflowConfig: @@ -3640,6 +3643,7 @@ def _single_agent_config( model=model, prompt="Do work", working_dir=working_dir, + settings_dir=settings_dir, output={"result": OutputField(type="string")}, routes=[RouteDef(to="$end")], ), @@ -4381,3 +4385,530 @@ 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 + + @pytest.mark.asyncio + async def test_a_template_rendering_empty_is_refused(self, tmp_path: Path) -> None: + """An empty render must not resolve to the workflow's own directory. + + The schema rejects a literal blank, but a *template* can still render + empty at run time -- ``--input repo=``, a script step that printed + nothing, a ``set`` binding evaluating to "". ``Path("")`` is + ``Path(".")``, which is not absolute, so without this guard the value + would be joined onto the workflow file's directory, pass ``is_dir()``, + and be forwarded as a real ``add_dirs`` entry -- and the filesystem + grant is unconditional, so a value meaning "nothing" would hand the + model access to the workflow's own tree. + """ + provider = _RecordingWorkingDirProvider() + engine = WorkflowEngine( + _single_agent_config(settings_dir="{{ workflow.input.repo }}"), + provider, + workflow_path=_workflow_file(tmp_path), + ) + + with pytest.raises(ExecutionError) as exc_info: + await engine.run({"repo": ""}) + + assert "empty string" in str(exc_info.value) + assert "settings_dir" in str(exc_info.value) + assert provider.calls == 0 + + @pytest.mark.asyncio + async def test_an_empty_working_dir_render_is_refused_too(self, tmp_path: Path) -> None: + """The same guard covers ``working_dir``, and that is a deliberate change. + + An empty-rendering ``working_dir`` previously resolved to the workflow + file's own directory and ran there -- the same "nothing means + something" footgun, just without the filesystem grant that makes the + ``settings_dir`` case dangerous. Pinned in its own right so a later + edit narrowing the guard to ``settings_dir`` cannot silently restore + it, and so the behaviour change is visible to anyone reading the tests. + """ + provider = _RecordingWorkingDirProvider() + engine = WorkflowEngine( + _single_agent_config(working_dir="{{ workflow.input.repo }}"), + provider, + workflow_path=_workflow_file(tmp_path), + ) + + with pytest.raises(ExecutionError) as exc_info: + await engine.run({"repo": ""}) + + assert "empty string" in str(exc_info.value) + assert "working_dir" in str(exc_info.value) + assert provider.calls == 0 + + +class TestSettingsDirObservability: + """``settings_dir`` must appear in the events, on all three agent paths. + + It is a trust decision, not a convenience: naming a directory loads + another repository's conventions *and* widens the model's built-in + ``Read``/``Edit``/``Bash`` to that tree. A grant that the dashboard and the + JSONL event log never mention cannot be audited after a run, so it rides + alongside ``working_dir`` wherever that is already emitted -- linear, + parallel-group and for-each. Pinning all three is the point: emitting it on + one path only would leave the other two silent about the same grant. + """ + + @pytest.mark.asyncio + async def test_agent_started_carries_settings_dir(self, tmp_path: Path) -> None: + target = tmp_path / "repo" + target.mkdir() + events: list[tuple[str, dict]] = [] + engine = WorkflowEngine( + _single_agent_config(settings_dir=str(target)), + _RecordingWorkingDirProvider(), + workflow_path=_workflow_file(tmp_path), + ) + engine._emit = lambda t, d=None: events.append((t, d or {})) # type: ignore[method-assign] + + await engine.run({}) + + started = [d for t, d in events if t == "agent_started"] + assert started, [t for t, _ in events] + assert started[0]["settings_dir"] == os.path.normpath(str(target)) + + @pytest.mark.asyncio + async def test_agent_started_reports_none_when_unset(self, tmp_path: Path) -> None: + """Absent rather than missing: a consumer can tell "no grant" from + "this Conductor did not report one".""" + events: list[tuple[str, dict]] = [] + engine = WorkflowEngine( + _single_agent_config(), + _RecordingWorkingDirProvider(), + workflow_path=_workflow_file(tmp_path), + ) + engine._emit = lambda t, d=None: events.append((t, d or {})) # type: ignore[method-assign] + + await engine.run({}) + + started = [d for t, d in events if t == "agent_started"] + assert started and "settings_dir" in started[0] + assert started[0]["settings_dir"] is None + + @staticmethod + def _parallel_config(dir_a: Path, dir_b: Path) -> WorkflowConfig: + return WorkflowConfig( + workflow=WorkflowDef( + name="sd-ev-parallel", + entry_point="fan", + runtime=RuntimeConfig(provider="claude-agent-sdk"), + context=ContextConfig(mode="accumulate"), + limits=LimitsConfig(max_iterations=10), + ), + agents=[ + AgentDef( + name="member_a", + model="gpt-4", + prompt="A", + settings_dir=str(dir_a), + output={"r": OutputField(type="string")}, + ), + AgentDef( + name="member_b", + model="gpt-4", + prompt="B", + settings_dir=str(dir_b) if dir_b else None, + output={"r": OutputField(type="string")}, + ), + ], + parallel=[ + ParallelGroup( + name="fan", + agents=["member_a", "member_b"], + routes=[RouteDef(to="$end")], + ) + ], + output={}, + ) + + @pytest.mark.asyncio + async def test_parallel_agent_started_carries_settings_dir(self, tmp_path: Path) -> None: + """The parallel fan-out path, which the linear test cannot see. + + Resolution and emission are different code paths: deleting the field + from this payload leaves ``provider.execute`` receiving the right + directory, so ``TestSettingsDirInGroups`` stays green while the event + goes silent. + """ + dir_a = tmp_path / "a" + dir_b = tmp_path / "b" + dir_a.mkdir() + dir_b.mkdir() + events: list[tuple[str, dict]] = [] + engine = WorkflowEngine( + self._parallel_config(dir_a, dir_b), + _RecordingWorkingDirProvider(), + workflow_path=_workflow_file(tmp_path), + ) + engine._emit = lambda t, d=None: events.append((t, d or {})) # type: ignore[method-assign] + + await engine.run({}) + + started = [d for t, d in events if t == "parallel_agent_started"] + assert {d["agent_name"]: d["settings_dir"] for d in started} == { + "member_a": os.path.normpath(str(dir_a)), + "member_b": os.path.normpath(str(dir_b)), + }, started + + @pytest.mark.asyncio + async def test_parallel_agent_started_reports_none_when_unset(self, tmp_path: Path) -> None: + """Negative control, matching ``TestWorkingDirEvents``' own pattern.""" + dir_a = tmp_path / "a" + dir_a.mkdir() + config = self._parallel_config(dir_a, None) # type: ignore[arg-type] + events: list[tuple[str, dict]] = [] + engine = WorkflowEngine( + config, _RecordingWorkingDirProvider(), workflow_path=_workflow_file(tmp_path) + ) + engine._emit = lambda t, d=None: events.append((t, d or {})) # type: ignore[method-assign] + + await engine.run({}) + + started = {d["agent_name"]: d for t, d in events if t == "parallel_agent_started"} + assert started["member_b"]["settings_dir"] is None + assert started["member_a"]["settings_dir"] == os.path.normpath(str(dir_a)) + + @pytest.mark.asyncio + async def test_for_each_agent_started_carries_settings_dir_per_item( + self, tmp_path: Path + ) -> None: + """The for-each path, per iteration. + + This is where a templated ``settings_dir`` most needs auditing: the + value varies per item, so an event omitting it makes the grant + unauditable exactly where it changes. + """ + for name in ("one", "two"): + (tmp_path / name).mkdir() + config = WorkflowConfig( + workflow=WorkflowDef( + name="sd-ev-for-each", + entry_point="lister", + runtime=RuntimeConfig(provider="claude-agent-sdk"), + context=ContextConfig(mode="accumulate"), + limits=LimitsConfig(max_iterations=10), + ), + agents=[ + AgentDef( + name="lister", + model="gpt-4", + prompt="List", + output={"repos": OutputField(type="array")}, + routes=[RouteDef(to="fans")], + ), + ], + for_each=[ + ForEachDef( + name="fans", + type="for_each", + source="lister.output.repos", + **{"as": "repo"}, + agent=AgentDef( + name="fan_agent", + model="gpt-4", + prompt="Work {{ repo }}", + settings_dir=str(tmp_path / "{{ repo }}"), + output={"r": OutputField(type="string")}, + ), + routes=[RouteDef(to="$end")], + ), + ], + output={}, + ) + events: list[tuple[str, dict]] = [] + + async def _execute(agent, context, rendered_prompt, tools=None, **kwargs): + content = {"repos": ["one", "two"]} if agent.name == "lister" else {"r": "ok"} + return AgentOutput( + content=content, + raw_response=None, + model=agent.model, + input_tokens=1, + output_tokens=1, + ) + + provider = _RecordingWorkingDirProvider() + provider.execute = _execute # type: ignore[method-assign] + engine = WorkflowEngine(config, provider, workflow_path=_workflow_file(tmp_path)) + engine._emit = lambda t, d=None: events.append((t, d or {})) # type: ignore[method-assign] + + await engine.run({}) + + started = [d for t, d in events if t == "for_each_agent_started"] + assert sorted(d["settings_dir"] for d in started) == sorted( + [os.path.normpath(str(tmp_path / "one")), os.path.normpath(str(tmp_path / "two"))] + ), started + + +class TestSettingsDirInGroups: + """``settings_dir`` must resolve inside parallel groups and for-each loops. + + All three agent paths route through ``_resolve_agent_working_dir``, so the + field does reach them -- but nothing pinned that, and the for-each site is + the one where a per-iteration template (``{{ item }}``) is most likely to + be used. It resolves *after* loop-variable injection, an ordering a future + refactor of the for-each body could break with no test to notice: the + agent would then silently load a different iteration's skills. + """ + + @pytest.mark.asyncio + async def test_for_each_resolves_settings_dir_per_iteration(self, tmp_path: Path) -> None: + for name in ("one", "two"): + (tmp_path / name).mkdir() + config = WorkflowConfig( + workflow=WorkflowDef( + name="sd-for-each", + entry_point="lister", + runtime=RuntimeConfig(provider="claude-agent-sdk"), + context=ContextConfig(mode="accumulate"), + limits=LimitsConfig(max_iterations=10), + ), + agents=[ + AgentDef( + name="lister", + model="gpt-4", + prompt="List", + output={"repos": OutputField(type="array")}, + routes=[RouteDef(to="fans")], + ), + ], + for_each=[ + ForEachDef( + name="fans", + type="for_each", + source="lister.output.repos", + **{"as": "repo"}, + agent=AgentDef( + name="fan_agent", + model="gpt-4", + prompt="Work {{ repo }}", + settings_dir=str(tmp_path / "{{ repo }}"), + output={"r": OutputField(type="string")}, + ), + routes=[RouteDef(to="$end")], + ), + ], + output={}, + ) + seen: list[tuple[str, str | None]] = [] + + async def _execute(agent, context, rendered_prompt, tools=None, **kwargs): + if agent.name == "lister": + return AgentOutput( + content={"repos": ["one", "two"]}, + raw_response=None, + model=agent.model, + input_tokens=1, + output_tokens=1, + ) + seen.append((agent.name, agent.settings_dir)) + return AgentOutput( + content={"r": "ok"}, + raw_response=None, + model=agent.model, + input_tokens=1, + output_tokens=1, + ) + + provider = _RecordingWorkingDirProvider() + provider.execute = _execute # type: ignore[method-assign] + engine = WorkflowEngine(config, provider, workflow_path=_workflow_file(tmp_path)) + + await engine.run({}) + + assert sorted(d for _, d in seen) == sorted( + [os.path.normpath(str(tmp_path / "one")), os.path.normpath(str(tmp_path / "two"))] + ), seen + + @pytest.mark.asyncio + async def test_parallel_member_resolves_its_own_settings_dir(self, tmp_path: Path) -> None: + dir_a = tmp_path / "a" + dir_b = tmp_path / "b" + dir_a.mkdir() + dir_b.mkdir() + config = WorkflowConfig( + workflow=WorkflowDef( + name="sd-parallel", + entry_point="fan", + runtime=RuntimeConfig(provider="claude-agent-sdk"), + context=ContextConfig(mode="accumulate"), + limits=LimitsConfig(max_iterations=10), + ), + agents=[ + AgentDef( + name="member_a", + model="gpt-4", + prompt="A", + settings_dir=str(dir_a), + output={"r": OutputField(type="string")}, + ), + AgentDef( + name="member_b", + model="gpt-4", + prompt="B", + settings_dir=str(dir_b), + output={"r": OutputField(type="string")}, + ), + ], + parallel=[ + ParallelGroup( + name="fan", + agents=["member_a", "member_b"], + routes=[RouteDef(to="$end")], + ) + ], + output={}, + ) + seen: list[tuple[str, str | None]] = [] + + async def _execute(agent, context, rendered_prompt, tools=None, **kwargs): + seen.append((agent.name, agent.settings_dir)) + return AgentOutput( + content={"r": "ok"}, + raw_response=None, + model=agent.model, + input_tokens=1, + output_tokens=1, + ) + + provider = _RecordingWorkingDirProvider() + provider.execute = _execute # type: ignore[method-assign] + engine = WorkflowEngine(config, provider, workflow_path=_workflow_file(tmp_path)) + + await engine.run({}) + + assert sorted(seen) == [ + ("member_a", os.path.normpath(str(dir_a))), + ("member_b", os.path.normpath(str(dir_b))), + ], seen diff --git a/tests/test_executor/test_agent.py b/tests/test_executor/test_agent.py index f0e75b7a..f3a93035 100644 --- a/tests/test_executor/test_agent.py +++ b/tests/test_executor/test_agent.py @@ -897,6 +897,60 @@ async def close(self) -> None: return None +class TestSettingsDirCapabilityRejection: + """``capabilities.settings_dir=False`` must hold at run time too. + + ``conductor run`` never calls the static validator, and the engine + renders, absolutizes and existence-checks the directory for *every* + provider -- so an author saw the field processed and then handed to a + provider that never reads it. The agent answered from whatever + conventions its cwd supplied and the run exited 0, which is the + silent-wrong-answer case the capability exists to prevent. Mirrors + :class:`TestSessionKeyCapabilityRejection`, and the four ``_reject_*`` + helpers that exist for the same reason. + """ + + @staticmethod + def _agent(tmp_path) -> AgentDef: + return AgentDef(name="review", prompt="hi", settings_dir=str(tmp_path)) + + @staticmethod + def _copilot(calls: list[str] | None = None) -> CopilotProvider: + def mock_handler(agent, prompt, context): + if calls is not None: + calls.append(agent.name) + return {"answer": "x"} + + return CopilotProvider(mock_handler=mock_handler) + + @pytest.mark.asyncio + async def test_provider_that_cannot_apply_it_is_refused(self, tmp_path) -> None: + assert CopilotProvider.CAPABILITIES.settings_dir is False + + with pytest.raises(ExecutionError) as exc_info: + await AgentExecutor(self._copilot()).execute(self._agent(tmp_path), {}) + + assert "does not apply it" in str(exc_info.value) + assert exc_info.value.agent_name == "review" + + @pytest.mark.asyncio + async def test_the_refusal_precedes_the_provider_call(self, tmp_path) -> None: + """An answer from the wrong repository's conventions is worse than none.""" + calls: list[str] = [] + with pytest.raises(ExecutionError): + await AgentExecutor(self._copilot(calls)).execute(self._agent(tmp_path), {}) + + assert calls == [] + + @pytest.mark.asyncio + async def test_an_agent_without_settings_dir_is_untouched(self) -> None: + output = await AgentExecutor(self._copilot()).execute( + AgentDef(name="review", prompt="hi"), {} + ) + + assert output.content == {"answer": "x"} + + class TestSessionKeyCapabilityRejection: """``capabilities.session_continuity=False`` must hold at run time too. diff --git a/tests/test_integration/test_mcp_roots_negotiation.py b/tests/test_integration/test_mcp_roots_negotiation.py new file mode 100644 index 00000000..e60278f5 --- /dev/null +++ b/tests/test_integration/test_mcp_roots_negotiation.py @@ -0,0 +1,211 @@ +"""Pins the MCP Roots rule that governs what a filesystem MCP server permits. + +``settings_dir`` governs skill discovery while cwd alone governs what a +filesystem MCP server permits, and the two cannot be collapsed into one +option. Deriving ``ClaudeAgentOptions.add_dirs`` from the directory arguments +of every stdio MCP server looks like it would widen that server's scope back; +it cannot, and the reason is a property of the *server*, not of Conductor: + +``@modelcontextprotocol/server-filesystem`` uses the directories in its argv +only while the connected client does not support MCP Roots. A client that +advertises the ``roots`` capability is asked for its roots at +post-initialization, and whatever it answers **replaces** the argv +directories outright. The Claude CLI advertises Roots and offers exactly one +root -- its cwd -- so a server declared with two directories ends up +permitting one, and ``--add-dir`` cannot put the others back because it takes +no part in that negotiation. + +This test pins the rule itself with a hand-rolled JSON-RPC client and no LLM: +two runs, byte-identical but for the client's ``roots`` capability. That is +what isolates the cause to Roots negotiation rather than to cwd derivation, +to ``--add-dir`` handling, or to any Conductor code path -- and what would +fail if a future server version changed the precedence, which is the +assumption ``settings_dir`` rests on. +""" + +from __future__ import annotations + +import json +import os +import shutil +import subprocess +import tempfile +import time +from pathlib import Path + +import pytest + +_SERVER = "@modelcontextprotocol/server-filesystem" +_ADOPTED = "Updated allowed directories from MCP roots" + +# ``real_api`` because this fetches @modelcontextprotocol/server-filesystem from +# npm: it pins *upstream's* negotiation behaviour rather than Conductor's own +# code, so an npm outage or a new server release must not redden an unrelated +# PR. The repo's convention for a test reaching an external service is an +# opt-in marker (cf. ``real_api`` / ``install_scripts`` / ``performance`` in +# pyproject.toml); CI runs ``-m "not real_api and not performance"``. +pytestmark = [ + pytest.mark.real_api, + pytest.mark.skipif( + shutil.which("npx") is None, + reason="npx not available; needs the real filesystem MCP server", + ), +] + + +def _list_allowed_directories( + *, 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. + """ + # ``shutil.which`` finds ``npx.cmd`` on Windows but ``CreateProcess`` only + # appends ``.exe``, so a bare "npx" would fail to launch there while the + # skipif above says it is present. Pass the resolved path. + npx = shutil.which("npx") + assert npx is not None # guarded by the module-level skipif + # A plain file rather than NamedTemporaryFile: the stderr poll below reopens + # it by name, which is unsupported while the handle is open on Windows. + err_path = Path(tempfile.mkdtemp(prefix="mcp-roots-")) / "server.err" + with err_path.open("w") as errf: + proc = subprocess.Popen( # noqa: S603 + [npx, "-y", _SERVER, *root_dirs], + 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 err_path.read_text(errors="replace") + + try: + send( + { + "jsonrpc": "2.0", + "id": 1, + "method": "initialize", + "params": { + "protocolVersion": "2024-11-05", + "capabilities": {"roots": {"listChanged": False}} if roots else {}, + "clientInfo": {"name": "conductor-roots-probe", "version": "1"}, + }, + } + ) + deadline = time.monotonic() + 120 + while time.monotonic() < deadline: + line = proc.stdout.readline() + if not line: + pytest.fail(f"server exited early; stderr:\n{server_stderr()}") + message = json.loads(line) + + if message.get("id") == 1: + send({"jsonrpc": "2.0", "method": "notifications/initialized"}) + # A client with no roots capability is never asked for + # roots, so nothing else will arrive to sequence against. + if roots is None: + call_tool() + elif message.get("method") == "roots/list": + send({"jsonrpc": "2.0", "id": message["id"], "result": {"roots": roots}}) + # The server swaps its allowlist in the continuation of its + # own ``listRoots()`` await, so a tool call sent straight + # after this reply races it and reads the pre-swap list -- + # which is exactly the false negative that made an earlier + # version of this probe report "argv roots survive". + swap = time.monotonic() + 30 + while _ADOPTED not in server_stderr() and time.monotonic() < swap: + time.sleep(0.05) + call_tool() + elif message.get("id") == 2: + return str(message["result"]["content"][0]["text"]) + pytest.fail(f"timed out; stderr:\n{server_stderr()}") + finally: + proc.kill() + proc.wait(timeout=30) + raise AssertionError("unreachable") + + +@pytest.fixture +def roots_tree(tmp_path: Path) -> dict[str, str]: + """Two declared server roots and a cwd that is neither, nor kin to either.""" + for name in ("rootA", "rootB", "cwdC"): + (tmp_path / name).mkdir() + (tmp_path / "rootB" / "target.txt").write_text("hello-from-rootB\n") + return {n: str(tmp_path / n) for n in ("rootA", "rootB", "cwdC")} + + +def test_argv_roots_honoured_when_client_declares_no_roots(roots_tree: dict[str, str]) -> None: + """The server is not broken and cwd is irrelevant to it. + + Without the capability the argv directories are the allowlist, in full -- + the baseline that makes the contrast below attributable to negotiation. + """ + allowed = _list_allowed_directories( + root_dirs=[roots_tree["rootA"], roots_tree["rootB"]], + cwd=roots_tree["cwdC"], + roots=None, + ) + + assert roots_tree["rootA"] in allowed + assert roots_tree["rootB"] in allowed + assert roots_tree["cwdC"] not in allowed + + +def test_single_advertised_root_replaces_every_argv_root(roots_tree: dict[str, str]) -> None: + """The defect, in one assertion: a client's sole root wins outright. + + Identical argv and identical cwd to the test above. Declaring ``roots`` + and answering with cwd alone -- what the Claude CLI does -- discards both + declared directories. No value of ``add_dirs`` changes this, which is why + ``settings_dir`` governs skill discovery and cwd alone governs MCP scope. + """ + allowed = _list_allowed_directories( + root_dirs=[roots_tree["rootA"], roots_tree["rootB"]], + cwd=roots_tree["cwdC"], + roots=[{"uri": f"file://{roots_tree['cwdC']}", "name": "cwd"}], + ) + + assert roots_tree["cwdC"] in allowed + assert roots_tree["rootA"] not in allowed + assert roots_tree["rootB"] not in allowed + + +def test_a_root_containing_both_declared_roots_permits_both(roots_tree: dict[str, str]) -> None: + """Why a single root is sufficient, and the basis of the recommended fix. + + One advertised root still permits everything beneath it, so an agent whose + cwd is a common parent reaches both declared roots. That is what makes + "keep cwd wide, select conventions with ``settings_dir``" work rather than + needing a server per root. + """ + parent = str(Path(roots_tree["rootA"]).parent) + + allowed = _list_allowed_directories( + root_dirs=[roots_tree["rootA"], roots_tree["rootB"]], + cwd=parent, + roots=[{"uri": f"file://{parent}", "name": "cwd"}], + ) + + assert parent in allowed + assert os.path.commonpath([parent, roots_tree["rootB"]]) == parent diff --git a/tests/test_providers/test_claude_agent_sdk.py b/tests/test_providers/test_claude_agent_sdk.py index b2150328..f3cb17ee 100644 --- a/tests/test_providers/test_claude_agent_sdk.py +++ b/tests/test_providers/test_claude_agent_sdk.py @@ -3083,3 +3083,484 @@ def test_windows_skips_only_the_driveless_path(self) -> None: "a Windows user's local install must still be found" ) assert any(p.endswith(".npm-global/bin/claude") for p in probed) + + +class TestSettingsDirAddDirs: + """``settings_dir`` is the only source of ``ClaudeAgentOptions.add_dirs``. + + The mechanism these tests rest on is measured rather than reasoned about + (``tests/test_integration/test_mcp_roots_negotiation.py`` pins it against + the real server without an LLM): ``@modelcontextprotocol/server-filesystem`` + uses its argv directories only when the client does not support MCP Roots; + the Claude CLI does support Roots and advertises exactly one, its cwd; so + the server discards its argv directories and permits cwd alone. + ``--add-dir`` does not participate in that negotiation, so it cannot be + used to widen what an MCP server permits -- which is why this field is + fed only by the author's ``settings_dir`` and never derived from server + arguments. + + What ``add_dirs`` does do is make a directory's *project* settings tier + contribute its skills, independent of cwd -- which is what lets an agent + keep a cwd wide enough for its MCP servers while loading a narrower + target repository's conventions. + """ + + @patch("conductor.providers.claude_agent_sdk.CLAUDE_AGENT_SDK_AVAILABLE", True) + async def test_settings_dir_becomes_add_dirs(self, tmp_path: Path) -> None: + """Requirement: the authored directory reaches the SDK option.""" + captured: dict = {} + + async def fake_query(**kwargs): + captured["add_dirs"] = kwargs["options"].add_dirs + yield _result(result="ok") + + with patch("conductor.providers.claude_agent_sdk.query", fake_query): + provider = ClaudeAgentSdkProvider() + await provider.execute( + agent=AgentDef(name="t", prompt="hi", settings_dir=str(tmp_path)), + context={}, + rendered_prompt="hi", + ) + + assert captured["add_dirs"] == [str(tmp_path)] + + @patch("conductor.providers.claude_agent_sdk.CLAUDE_AGENT_SDK_AVAILABLE", True) + async def test_no_settings_dir_sends_no_add_dirs(self) -> None: + """An agent that names no directory adds none. + + The empty list matters: the CLI would otherwise be handed a directory + whose skills, being in an enabled settings tier, become listed and + invocable -- ambient content the workflow never declared. + """ + captured: dict = {} + + async def fake_query(**kwargs): + captured["add_dirs"] = kwargs["options"].add_dirs + yield _result(result="ok") + + with patch("conductor.providers.claude_agent_sdk.query", fake_query): + provider = ClaudeAgentSdkProvider() + await provider.execute( + agent=AgentDef(name="t", prompt="hi"), context={}, rendered_prompt="hi" + ) + + assert captured["add_dirs"] == [] + + @patch("conductor.providers.claude_agent_sdk.CLAUDE_AGENT_SDK_AVAILABLE", True) + async def test_stdio_server_dir_args_are_not_forwarded(self, tmp_path: Path) -> None: + """The regression this class is named for. + + A stdio server's own directory arguments must NOT reach ``add_dirs``. + Forwarding them was measurably ineffective, and reinstating it would + silently widen skill discovery to every declared MCP root -- granting + content from directories the author named as *data*, not as a source + of conventions. + """ + root_a = tmp_path / "rootA" + root_a.mkdir() + root_b = tmp_path / "rootB" + root_b.mkdir() + captured: dict = {} + + async def fake_query(**kwargs): + captured["add_dirs"] = kwargs["options"].add_dirs + yield _result(result="ok") + + with patch("conductor.providers.claude_agent_sdk.query", fake_query): + provider = ClaudeAgentSdkProvider( + mcp_servers={ + "filesystem": { + "type": "stdio", + "command": "npx", + "args": [ + "-y", + "@modelcontextprotocol/server-filesystem", + str(root_a), + str(root_b), + ], + } + } + ) + await provider.execute( + agent=AgentDef(name="t", prompt="hi"), context={}, rendered_prompt="hi" + ) + + assert captured["add_dirs"] == [] + + @patch("conductor.providers.claude_agent_sdk.CLAUDE_AGENT_SDK_AVAILABLE", True) + async def test_settings_dir_is_independent_of_cwd(self, tmp_path: Path) -> None: + """The whole point of the field: the two directories are unrelated. + + ``cwd`` becomes the session's sole MCP root; ``settings_dir`` only + adds a project tier to read skills from. An agent must be able to set + a wide cwd and a narrow settings_dir at once -- neither derived from + nor constrained by the other. + """ + wide = tmp_path / "wide" + wide.mkdir() + narrow = wide / "repo" + narrow.mkdir() + captured: dict = {} + + async def fake_query(**kwargs): + captured["cwd"] = kwargs["options"].cwd + captured["add_dirs"] = kwargs["options"].add_dirs + yield _result(result="ok") + + with patch("conductor.providers.claude_agent_sdk.query", fake_query): + provider = ClaudeAgentSdkProvider() + await provider.execute( + agent=AgentDef( + name="t", + prompt="hi", + working_dir=str(wide), + settings_dir=str(narrow), + ), + context={}, + rendered_prompt="hi", + ) + + assert captured["cwd"] == str(wide) + assert captured["add_dirs"] == [str(narrow)] + + @patch("conductor.providers.claude_agent_sdk.CLAUDE_AGENT_SDK_AVAILABLE", True) + async def test_settings_dir_does_not_become_a_second_cwd(self, tmp_path: Path) -> None: + """The boundary of what this field can deliver, pinned deliberately. + + ``add_dirs`` carries a directory's ``.claude/skills`` and nothing + else: ``CLAUDE.md``, ``.claude/rules/*.md``, ``.claude/settings.json`` + (so ``env`` and ``hooks``) and ``.claude/agents`` all follow cwd + instead -- measured against the CLI, not inferred. So a + ``settings_dir`` must never be quietly promoted into ``cwd`` in an + attempt to widen what it loads: that would hand the agent the narrow + directory as its sole MCP root, which is the exact defect this field + exists to avoid. + + An agent needing a repository's rules *and* a wide cwd cannot have + both from these two fields, and this test is what keeps that trade + visible rather than papered over. + """ + wide = tmp_path / "wide" + wide.mkdir() + narrow = wide / "repo" + narrow.mkdir() + captured: dict = {} + + async def fake_query(**kwargs): + captured["cwd"] = kwargs["options"].cwd + captured["add_dirs"] = kwargs["options"].add_dirs + yield _result(result="ok") + + with patch("conductor.providers.claude_agent_sdk.query", fake_query): + provider = ClaudeAgentSdkProvider() + await provider.execute( + agent=AgentDef( + name="t", prompt="hi", working_dir=str(wide), settings_dir=str(narrow) + ), + context={}, + rendered_prompt="hi", + ) + + # The line that pins it: cwd is the wide directory exactly. The + # earlier `narrow not in cwd` substring check added nothing -- it also + # passes for an implementation that sets cwd to an unrelated third + # directory, so it read as a guard without being one. + assert captured["cwd"] == str(wide) + assert captured["add_dirs"] == [str(narrow)] + + @patch("conductor.providers.claude_agent_sdk.CLAUDE_AGENT_SDK_AVAILABLE", True) + async def test_settings_dir_passed_verbatim(self, tmp_path: Path) -> None: + """Not re-resolved, matching ``cwd``: the engine already rendered, + absolutized and existence-checked it, and ``resolve()`` here would + collapse the symlink aliases the engine preserves on purpose.""" + real = tmp_path / "real" + real.mkdir() + link = tmp_path / "link" + link.symlink_to(real, target_is_directory=True) + captured: dict = {} + + async def fake_query(**kwargs): + captured["add_dirs"] = kwargs["options"].add_dirs + yield _result(result="ok") + + with patch("conductor.providers.claude_agent_sdk.query", fake_query): + provider = ClaudeAgentSdkProvider() + await provider.execute( + agent=AgentDef(name="t", prompt="hi", settings_dir=str(link)), + context={}, + rendered_prompt="hi", + ) + + assert captured["add_dirs"] == [str(link)] + + @pytest.mark.asyncio + async def test_settings_dir_reaches_the_cli_as_add_dir(self, tmp_path: Path) -> None: + """The SDK still turns ``add_dirs`` into the ``--add-dir`` argv flag. + + Asserting ``options.add_dirs`` alone proves only that Conductor set the + field. ``settings_dir`` has no fallback delivery path -- there is no + prompt-injection equivalent that could carry a settings tier -- and the + pin is ``claude-agent-sdk>=0.2.82``, a floor with no ceiling, so a lock + bump that renamed or dropped the flag would leave every other test in + this class green with the feature silently dead. + """ + from claude_agent_sdk._internal.transport.subprocess_cli import ( + SubprocessCLITransport, + ) + + target = tmp_path / "target" + target.mkdir() + + async def options_for(settings_dir: str | None): + captured: dict = {} + + async def fake_query(**kwargs): + captured["options"] = kwargs["options"] + yield _result(result="ok") + + with patch("conductor.providers.claude_agent_sdk.query", fake_query): + provider = ClaudeAgentSdkProvider() + await provider.execute( + agent=AgentDef(name="t", prompt="hi", settings_dir=settings_dir), + context={}, + rendered_prompt="hi", + ) + return captured["options"] + + def argv(options) -> list[str]: + # Deliberately local rather than reusing ``TestSkillsWiring._argv``: + # that is another class's private helper, and importing across test + # classes couples them. Both wrap the same three SDK calls; if the + # SDK's command builder moves, both fail together rather than one + # silently passing. + transport = SubprocessCLITransport(prompt="hi", options=options) + transport._cli_path = "/usr/bin/claude" + return transport._build_command() + + with_dir = argv(await options_for(str(target))) + assert "--add-dir" in with_dir, with_dir + assert with_dir[with_dir.index("--add-dir") + 1] == str(target) + + # Negative control: without a settings_dir the flag is absent entirely, + # so the assertion above cannot pass against an always-emitted flag. + assert "--add-dir" not in argv(await options_for(None)) + + @pytest.mark.asyncio + @pytest.mark.parametrize( + ("sources", "agent_skills", "expect_warning"), + [ + (None, None, True), + (["user"], None, True), + (["local"], None, True), + (["project"], None, False), + (["project"], [], True), + ], + ) + async def test_a_settings_dir_with_no_project_tier_warns_at_run_time( + self, + tmp_path: Path, + caplog: pytest.LogCaptureFixture, + sources: list[str] | None, + agent_skills: list[str] | None, + expect_warning: bool, + ) -> None: + """``conductor run`` must not be silent when the skills half no-ops. + + ``conductor validate`` warns about this, but ``conductor run`` never + calls the static validator -- the same reason the four ``_reject_*`` + helpers exist. Without this the author gets the one effect they did + not ask for (the filesystem grant, which applies regardless) and no + diagnostic about the one they did. + """ + target = tmp_path / "repo" + target.mkdir() + + async def fake_query(**kwargs): + yield _result(result="ok") + + kwargs = {} if sources is None else {"setting_sources": sources} + with patch("conductor.providers.claude_agent_sdk.query", fake_query): + provider = ClaudeAgentSdkProvider(**kwargs) # type: ignore[arg-type] + with caplog.at_level(logging.WARNING): + await provider.execute( + agent=AgentDef( + name="judge", + prompt="hi", + settings_dir=str(target), + skills=agent_skills, + ), + context={}, + rendered_prompt="hi", + ) + + hits = [r for r in caplog.records if "no skills are discovered" in r.message] + assert bool(hits) is expect_warning, [r.message for r in caplog.records] + + @pytest.mark.asyncio + async def test_the_tier_warning_is_latched_per_directory_and_cause( + self, tmp_path: Path, caplog: pytest.LogCaptureFixture + ) -> None: + """Once per directory, and specifically once across a for_each. + + Latched because the condition is static while executions are not. + Keyed by the resolved directory rather than the agent name because + the engine renames a for_each member per item (``[]``), + so a name-keyed latch emits one line per item -- the exact case + latching exists to prevent. Not a bare flag either: a second agent + naming a *different* directory must still be reported, since naming + the directory is the point of the warning. The key is additionally + paired with the cause, since the remedy depends on it -- see + :meth:`test_two_causes_on_one_directory_both_warn`. + """ + target = tmp_path / "repo" + other = tmp_path / "other" + target.mkdir() + other.mkdir() + + async def fake_query(**kwargs): + yield _result(result="ok") + + with patch("conductor.providers.claude_agent_sdk.query", fake_query): + provider = ClaudeAgentSdkProvider() + with caplog.at_level(logging.WARNING): + # Eight for_each iterations over one directory, as the engine + # drives them: same settings_dir, a fresh name each time. + for key in range(8): + await provider.execute( + agent=AgentDef(name=f"fan[{key}]", prompt="hi", settings_dir=str(target)), + context={}, + rendered_prompt="hi", + ) + await provider.execute( + agent=AgentDef(name="judge", prompt="hi", settings_dir=str(other)), + context={}, + rendered_prompt="hi", + ) + + warned = [ + (r.args[0], r.args[1]) + for r in caplog.records + if "no skills are discovered" in r.message and r.args + ] + assert warned == [("fan[0]", str(target)), ("judge", str(other))], warned + + @pytest.mark.asyncio + async def test_the_tier_warning_remedy_matches_the_cause( + self, tmp_path: Path, caplog: pytest.LogCaptureFixture + ) -> None: + """Advice an author can act on, as ``config/validator.py`` does. + + Telling an author to add ``'project'`` when their own ``skills: []`` + is what zeroed the tier sends them to add a value already present, + and the warning keeps firing. + """ + target = tmp_path / "repo" + target.mkdir() + + async def fake_query(**kwargs): + yield _result(result="ok") + + async def remedy_for(sources: list[str] | None, skills: list[str] | None) -> str: + caplog.clear() + kwargs = {} if sources is None else {"setting_sources": sources} + with patch("conductor.providers.claude_agent_sdk.query", fake_query): + provider = ClaudeAgentSdkProvider(**kwargs) # type: ignore[arg-type] + with caplog.at_level(logging.WARNING): + await provider.execute( + agent=AgentDef( + name="judge", + prompt="hi", + settings_dir=str(target), + skills=skills, + ), + context={}, + rendered_prompt="hi", + ) + hits = [r for r in caplog.records if "no skills are discovered" in r.message] + assert hits, [r.message for r in caplog.records] + return hits[0].message + + no_tier = await remedy_for(None, None) + assert "Enable the 'project' tier via runtime.provider.setting_sources" in no_tier + + opted_out = await remedy_for(["project"], []) + assert "'skills: []' opts it out" in opted_out + assert "Enable the 'project' tier" not in opted_out, "advice is a no-op for this cause" + + @pytest.mark.asyncio + async def test_two_causes_on_one_directory_both_warn( + self, tmp_path: Path, caplog: pytest.LogCaptureFixture + ) -> None: + """The latch key includes the cause, because the remedy depends on it. + + Keying on the directory alone dedupes a for_each correctly, but two + agents can name one directory for different reasons -- and then a + single line prescribes a fix that is wrong for the agent it does not + name. The pair keeps the for_each collapse (all members share a cause) + while letting a differently-caused agent through. + """ + target = tmp_path / "repo" + target.mkdir() + + async def fake_query(**kwargs): + yield _result(result="ok") + + with patch("conductor.providers.claude_agent_sdk.query", fake_query): + provider = ClaudeAgentSdkProvider() + with caplog.at_level(logging.WARNING): + await provider.execute( + agent=AgentDef( + name="opted_out", prompt="hi", settings_dir=str(target), skills=[] + ), + context={}, + rendered_prompt="hi", + ) + await provider.execute( + agent=AgentDef(name="no_tier", prompt="hi", settings_dir=str(target)), + context={}, + rendered_prompt="hi", + ) + + hits = [r for r in caplog.records if "no skills are discovered" in r.message] + assert [r.args[0] for r in hits] == ["opted_out", "no_tier"], [r.args[0] for r in hits] + assert "'skills: []' opts it out" in hits[0].message + assert "Enable the 'project' tier" in hits[1].message + + @pytest.mark.asyncio + async def test_the_no_tier_remedy_does_not_prescribe_a_rejected_edit( + self, tmp_path: Path, caplog: pytest.LogCaptureFixture + ) -> None: + """The no-tier arm must not prescribe an edit the schema would reject. + + This arm serves two of ``config/validator.py``'s causes at once: a + missing tier, and a per-agent ``provider: claude-agent-sdk`` override + under a different ``runtime.provider``, where ``factory.py`` forwards + no ``setting_sources`` and the schema would then reject adding it. + + The provider cannot tell those two apart -- it never receives the + workflow-level provider name, only ``setting_sources`` -- which is + why one shared arm is the right design and why this test can pin + only the wording that is true of both. The override path itself is + covered at validate time, where the config *is* visible: + ``test_config/test_settings_dir_schema.py:: + TestProjectTierWarningCauses:: + test_provider_override_does_not_advise_the_impossible``. + """ + target = tmp_path / "repo" + target.mkdir() + + async def fake_query(**kwargs): + yield _result(result="ok") + + with patch("conductor.providers.claude_agent_sdk.query", fake_query): + provider = ClaudeAgentSdkProvider() + with caplog.at_level(logging.WARNING): + await provider.execute( + agent=AgentDef(name="judge", prompt="hi", settings_dir=str(target)), + context={}, + rendered_prompt="hi", + ) + + hits = [r for r in caplog.records if "no skills are discovered" in r.message] + assert hits + assert "requires runtime.provider itself to be 'claude-agent-sdk'" in hits[0].message