feat(claude-agent-sdk): select the project settings tier with settings_dir - #514
Conversation
Jason Robert (jrob5756)
left a comment
There was a problem hiding this comment.
This adds settings_dir to select the claude-agent-sdk project settings tier, but three things need fixing before this merges: a docs table that describes a security feature (tools: allowlist enforcement) which doesn't exist on this branch, a capability that's checked at validate time but never enforced at run time so conductor run silently ignores it on an unsupported provider, and an empty string that resolves to the workflow's own directory and grants the model filesystem access to it. The 11 recommended findings are mostly about the same theme showing up elsewhere: warnings that don't fire for cases they should, doc/comment text that undersells or inverts what the code does, and a couple of test gaps.
Blocking:
docs/providers/experimental.md:104— capability row claims allowlist enforcement that isn't implementedsrc/conductor/providers/capabilities.py:146— docstring guarantees validate-time rejection that has no run-time counterpartsrc/conductor/engine/workflow.py:665— emptysettings_dirsilently resolves to the workflow directory and grants file access
r10 — RECOMMENDED: the validator sub-agent inherits working_dir but drops settings_dir
No path/line — this PR doesn't touch the file in question, so there's nowhere to anchor it.
engine/validator.py::_build_validator_agent builds a fresh AgentDef field by field for the output-validation sub-agent. It copies working_dir=agent.working_dir but has no matching line for settings_dir, even though it receives the already-resolved primary agent. The result: a validator step grading an agent with a settings_dir runs without that directory's project-tier skills and without the filesystem grant, with nothing recording that this was a deliberate choice. It might be fine — the validator sub-agent runs with tools=[], so arguably neither half matters — but right now it's just an omission, not a decision.
It also cuts against this PR's own claim (workflow.py:601) that working_dir and settings_dir share resolution code specifically so they can't drift. They can't drift in resolution, but _build_validator_agent is the one place in the tree that reconstructs a runtime AgentDef instead of using model_copy, so it's exactly where future fields will get dropped by default.
Suggested fix: switch _build_validator_agent to agent.model_copy(update={...}) for the fields it actually overrides, so inheritance is the default and this class of bug goes away. If dropping settings_dir here is intentional, set it to None explicitly with a comment next to working_dir explaining why.
| | Provider | Upstream pin | Maintainer | Capability carve-outs | | ||
| |---|---|---|---| | ||
| | `claude-agent-sdk` | `claude-agent-sdk>=0.2.82` | `@lesandiz (best-effort)` | no `workflow_tools_passthrough`, no `reasoning_effort`, `prompt_injection` structured output, no `checkpoint_resume` (agents without a `session_key` carry no session state across a resume). Supports `mcp_tools` as of [#335](https://github.com/microsoft/conductor/issues/335), except that a narrowing per-server `tools:` filter is refused (no SDK equivalent). Supports `working_dir` as of [#348](https://github.com/microsoft/conductor/issues/348); the CLI would load `CLAUDE.md` and `.claude/settings*.json` from that directory, but `setting_sources` is empty by default as of [#352](https://github.com/microsoft/conductor/issues/352) so ambient instructions, settings, hooks, and skills are not inherited unless a workflow opts in via `runtime.provider.setting_sources` ([#501](https://github.com/microsoft/conductor/issues/501)) — which loads the named tiers **including their hooks**, so only for repositories trusted as much as the workflow. Declares `session_continuity`: an agent with a `session_key` reuses one Claude session across executions, and the session map survives `conductor resume` — see [Session Continuity](../workflow-syntax.md#session-continuity-session_key). | | ||
| | `claude-agent-sdk` | `claude-agent-sdk>=0.2.82` | `@lesandiz (best-effort)` | no `reasoning_effort`, `prompt_injection` structured output, no `checkpoint_resume` (agents without a `session_key` carry no session state across a resume). Supports `mcp_tools` as of [#335](https://github.com/microsoft/conductor/issues/335), except that a narrowing per-server `tools:` filter is refused (no SDK equivalent). Supports `workflow_tools_passthrough`: a per-agent `tools:` allowlist is enforced by enumerating the declared stdio MCP servers and denying every tool not on the list (plus the built-in write/exec tools), since the CLI's `allowed_tools` only pre-approves and does not restrict. An allowlist alongside an http/sse server is refused — those cannot be enumerated, so the denial set would be unknown. `tools: []` alongside `mcp_servers:` is still refused (`mcp_servers_always_attached`): honoring an allowlist does not mean the provider can detach a declared server. Supports `working_dir` as of [#348](https://github.com/microsoft/conductor/issues/348); the CLI would load `CLAUDE.md` and `.claude/settings*.json` from that directory, but `setting_sources` defaults to empty as of [#352](https://github.com/microsoft/conductor/issues/352) so ambient instructions, settings, hooks, and skills are not inherited; a workflow opts back in per run with `runtime.provider.setting_sources`, and chooses per agent which directory the `project` tier reads skills from via `settings_dir` (cwd alone governs the CLI's sole MCP root, so the two are deliberately separate) — see [Target-Repository Skills](../workflow-syntax.md#target-repository-skills-settings_dir). Declares `session_continuity`: an agent with a `session_key` reuses one Claude session across executions, and the session map survives `conductor resume` — see [Session Continuity](../workflow-syntax.md#session-continuity-session_key). | |
There was a problem hiding this comment.
BLOCKING: This row was rewritten to drop the accurate carve-out (no workflow_tools_passthrough) and replace it with four sentences claiming the provider enforces a per-agent tools: allowlist by "enumerating the declared stdio MCP servers and denying every tool not on the list," plus claims about refusing an allowlist alongside an http/sse server and refusing tools: [] via something called mcp_servers_always_attached.
None of this exists on this branch. src/conductor/providers/claude_agent_sdk.py:627 still sets workflow_tools_passthrough=False, _resolve_tool_config still raises ProviderError on any non-empty allowlist, grep -rn mcp_servers_always_attached src/ returns nothing, and AGENTS.md (which this PR doesn't touch) still says allowlists are unsupported on this provider.
This reads like text pasted in from a different branch — it has nothing to do with settings_dir. It's worse than an ordinary doc slip because this table is the authoritative statement of what an experimental provider honors, and the invented text promises a security property that isn't real: a reader would believe a tools: allowlist restricts the agent, when in fact conductor validate rejects that config and the provider refuses it at execute time. It also now contradicts AGENTS.md, which this repo treats as a source of truth.
Suggested fix: restore no \workflow_tools_passthrough`,at the head of the carve-out list, delete the four sentences about allowlist enforcement, and keep only thesettings_dirclause this PR actually needs. If the passthrough work is real, put it in its own PR that flips the capability toTrue`.
| the directory would run the agent in the wrong repository. Defaults to | ||
| ``False`` (conservative).""" | ||
|
|
||
| settings_dir: bool = False |
There was a problem hiding this comment.
BLOCKING: This docstring makes a guarantee it doesn't keep: "Workflows that set settings_dir against a provider with settings_dir=False fail validation ... silently ignoring it would run the agent against the wrong conventions while reporting success." The schema docstring makes the same promise.
That's true for exactly one CLI verb. validate_workflow_config has a single caller, src/conductor/cli/validate.py:63, and conductor run never goes through it. grep -rn settings_dir src/conductor --include=*.py turns up no reader anywhere in executor/, so there's no run-time check either.
Traced the failure: provider: copilot with an agent's settings_dir: /repo — the engine renders, absolutizes, and existence-checks that directory for every provider (workflow.py:665), so the author sees the field get processed, then hands the agent to a provider that never reads it. The workflow runs against the wrong repository's conventions and exits 0.
This is a real gap against an established pattern, not a general nitpick. AgentExecutor already mirrors four comparable capability refusals at run time — _reject_unsupported_skills, _reject_unsupported_plugins, _reject_discovery_without_native_skills, _reject_unfilterable_agents — each documented as existing "because conductor run never calls the static validator." The closest sibling field, session_key, got exactly that treatment (_reject_unsupported_session_key, executor/agent.py:1093). working_dir is unguarded the same way, but its failure shows up in the transcript; this one produces a confident answer from the wrong repo.
Suggested fix: add a run-time mirror in src/conductor/executor/agent.py, called alongside the other _reject_* helpers:
def _reject_unsupported_settings_dir(self, agent: AgentDef) -> None:
if agent.settings_dir is None:
return
caps = getattr(type(self.provider), "CAPABILITIES", None)
if caps is None or caps.settings_dir:
return
raise ExecutionError(
f"Agent '{agent.name}' sets settings_dir={agent.settings_dir!r} but provider "
f"'{type(self.provider).__name__}' does not apply it "
f"(capabilities.settings_dir=False).",
agent_name=agent.name,
suggestion="Use working_dir, or override the agent to 'claude-agent-sdk'.",
)If validate-only is deliberate (matching working_dir), weaken this docstring and the one at schema.py:1387 to say "conductor validate refuses it" instead of promising the field can never be silently dropped.
| agent, agent_context, "working_dir", raw | ||
| ) | ||
|
|
||
| if agent.settings_dir is not None: |
There was a problem hiding this comment.
BLOCKING: Four layers disagree about what counts as "set" for this field, and an empty string falls through the gap into an actual filesystem grant.
Path("") is PosixPath('.'), which isn't absolute, so it gets joined onto the workflow file's directory and normalized to that directory — which always exists, so the is_dir() check at line 620 never fires. The value comes out truthy and lands in add_dirs=[<workflow dir>] at claude_agent_sdk.py:1019. I confirmed this by tracing it end to end.
| Layer | Predicate | Verdict on "" |
|---|---|---|
schema.py step-type rejections (7 sites) |
if self.settings_dir: |
unset — AgentDef(name='w', type='wait', duration='1s', settings_dir='') is accepted despite the documented rejection |
engine/workflow.py:665 |
is not None |
set — resolves to the workflow directory |
validator.py:2476 |
is not None |
set |
claude_agent_sdk.py:1019 |
truthiness | forwarded |
Per this PR's own docs the grant is unconditional: naming a directory widens the model's built-in Read/Edit/Bash to that tree even with no settings tier enabled. So a value meant as "nothing" hands the model read/write access to the workflow's own repo.
This isn't a corner case either. The shipped example templates settings_dir from upstream (examples/claude-agent-sdk-settings-dir.yaml:104, settings_dir: "{{ workflow.input.repo }}", no input: block declared). A missing variable raises under StrictUndefined, which is fine, but a variable that exists and is empty — --input repo=, a script step that exited 0 without printing anything, a set binding that evaluates to "" — renders empty and takes this path. working_dir has the same lexical quirk, but there the fallback is a harmless default cwd; here it's a grant.
Suggested fix: reject the empty value at the type boundary so all four layers agree:
# schema.py
settings_dir: str | None = Field(default=None, min_length=1)and guard the rendered result too, since a template can still render empty even when the raw value passes:
rendered = self.renderer.render(raw, agent_context)
if not rendered.strip():
raise ExecutionError(
f"Agent '{agent.name}': {field} rendered to an empty string from '{raw}'. "
f"An empty value would silently resolve to the workflow file's own directory.",
agent_name=agent.name,
suggestion=f"Check that the upstream step or input feeding {field} produced a path.",
)Then switch the seven schema guards from if self.settings_dir: to is not None so the step-type rejections can't be bypassed with an empty string.
| -- so no ``project`` tier exists for a ``settings_dir`` to be read from. | ||
| """ | ||
| provider = config.workflow.runtime.provider | ||
| return bool(getattr(provider, "setting_sources", None)) |
There was a problem hiding this comment.
RECOMMENDED: _setting_sources_enabled returns bool(setting_sources) — true for any tier — but settings_dir only feeds the project tier, per this PR's own docstrings (schema.py:1396, docs/workflow-syntax.md:407). Checked all four cases:
setting_sources |
warning emitted? |
|---|---|
[] / absent |
yes |
['user'] |
no |
['local'] |
no |
['project'] |
no (correct) |
user reads ~/.claude and local is cwd-bound — neither can make a settings_dir's skills discoverable — yet validate stays green and silent. That's the exact outcome the warning was supposed to prevent.
There's a second blind spot on the same branch: claude_agent_sdk.py:965 computes effective_sources = [] if agent.skills == [] else self._setting_sources, so a per-agent skills: [] disables the tier for that agent entirely. The helper only sees config, not the agent, so skills: [] plus settings_dir: /repo plus setting_sources: [project] validates clean while discovering nothing and still applying the filesystem grant. Neither case is covered by tests/test_config/test_settings_dir_schema.py, which only exercises ['project'] and absent.
Suggested fix:
def _project_tier_enabled(config: WorkflowConfig, agent: AgentDef) -> bool:
"""True iff this agent's session will enable the 'project' settings tier.
'user' reads ~/.claude and 'local' is cwd-bound, so neither makes a
settings_dir's skills discoverable. A per-agent `skills: []` opts the
agent out of the tier entirely (claude_agent_sdk.py::execute).
"""
if agent.skills == []:
return False
return "project" in (config.workflow.runtime.provider.setting_sources or [])Widen the warning text at line 2490 to say "does not enable the 'project' settings tier," branch it on which cause applies, and add the ['user'] and skills: [] cases to the warning test.
| f"has a surface for it; use working_dir, or move this agent to " | ||
| f"that provider." | ||
| ) | ||
| elif agent.settings_dir is not None and not _setting_sources_enabled(config): |
There was a problem hiding this comment.
RECOMMENDED: The warning ends with: "Add 'setting_sources: [project]' to runtime.provider to load that repository's skills." But setting_sources lives on the single workflow-level ProviderSettings, and schema.py:2998 rejects it whenever runtime.provider.name != 'claude-agent-sdk'.
So for runtime.provider: copilot plus an agent with provider: claude-agent-sdk and a settings_dir — a config the capability check at line 2476 explicitly allows, since _resolved_provider_name honors per-agent overrides — this warning fires and can never be satisfied. Reproduced both halves:
- validate says:
Agent 'a' sets settings_dir='/tmp' but the workflow does not set runtime.provider.setting_sources ... Add 'setting_sources: [project]' to runtime.provider - following that advice raises:
'setting_sources' is only supported when name='claude-agent-sdk' (got name='copilot')
A warning the author can't act on trains them to ignore validate output, which makes the other warnings next to it easier to dismiss too.
Suggested fix: detect the override case and give the author something they can actually do. When the agent's resolved provider differs from config.workflow.runtime.provider.name, say the settings tier is workflow-scoped and can't be enabled for a per-agent provider override — settings_dir there only provides the filesystem grant — and point at moving the provider to runtime.provider instead. If this combination isn't meant to be supported at all, make it an error.
| rendered_prompt="hi", | ||
| ) | ||
|
|
||
| assert captured["add_dirs"] == [str(tmp_path)] |
There was a problem hiding this comment.
RECOMMENDED: TestSettingsDirAddDirs only asserts options.add_dirs == [...]. That proves Conductor sets the SDK field; it doesn't prove the SDK still turns it into the --add-dir argv flag.
That gap matters more here than for a typical option, because settings_dir has no fallback delivery path — there's no prompt-injection equivalent that could carry a settings tier if the flag stopped being emitted. AGENTS.md makes the same argument for the analogous skills seam: "a negative assertion could not tell a working path from one that dropped the skills entirely." The pyproject.toml pin is claude-agent-sdk>=0.2.82 — a floor with no ceiling — so a future lock bump that renames or drops the flag would leave all six new tests green with the feature quietly dead.
The file already has the tool for this: _argv() at line 2685 wraps SubprocessCLITransport(...)._build_command() and is already used to pin --plugin-dir, --allowedTools, and --setting-sources=.
Suggested fix:
argv = self._argv(options_with_settings_dir)
assert argv[argv.index("--add-dir") + 1] == str(target)
# negative control:
assert "--add-dir" not in self._argv(options_without_settings_dir)| _SERVER = "@modelcontextprotocol/server-filesystem" | ||
| _ADOPTED = "Updated allowed directories from MCP roots" | ||
|
|
||
| pytestmark = pytest.mark.skipif( |
There was a problem hiding this comment.
RECOMMENDED: The only guard here is skipif(shutil.which("npx") is None). GitHub-hosted runners ship Node preinstalled, so npx resolves on both ubuntu-latest and windows-latest, and the file has no marker — CI runs pytest -m "not real_api and not performance" (ci.yml:166), so this test gets selected. make test selects it too.
Two separate problems:
-
Network dependency in the default suite. The test shells out to
npx -y @modelcontextprotocol/server-filesystem, unpinned, which resolves and (cold cache) downloads from npm. The repo's own convention for tests reaching an external service is an opt-in marker —real_api,install_scripts,performanceare all registered inpyproject.toml. Without one, an npm outage or an upstream release turns unrelated PRs red over a test that's pinning upstream behavior, not Conductor's own code. -
Windows.
subprocess.Popen(["npx", ...])withoutshell=Truewon't launchnpx.cmdsinceCreateProcessonly appends.exe, whileshutil.whichdoes findnpx.cmdand so doesn't skip. Separately,tempfile.NamedTemporaryFilegets reopened by name viaPath(errf.name).read_text()while still open — unsupported on Windows, and it'll raisePermissionErroron every stderr poll.
The test design itself is solid (two runs differing only in whether roots is advertised, real server, no LLM involved), which is why it's worth fixing rather than deleting.
Suggested fix:
pytestmark = [
pytest.mark.real_api, # fetches @modelcontextprotocol/server-filesystem from npm
pytest.mark.skipif(shutil.which("npx") is None, reason="needs the real filesystem MCP server"),
]Pass the resolved shutil.which("npx") path to Popen instead of the bare name, and swap the NamedTemporaryFile for tmp_path / "server.err" opened separately for reading.
| Example — a judge reviewing a target repository while reading artifacts | ||
| from a sibling directory:: | ||
|
|
||
| agents: |
There was a problem hiding this comment.
RECOMMENDED: The docstring's example uses a mapping form the schema rejects:
agents:
judge:
settings_dir: "{{ setup_worktree.output.worktree_path }}"WorkflowConfig.agents is list[AgentDef] (schema.py:3902) with no mapping coercion, so this fails to parse. Every other YAML example in this file uses the sequence form (- name: reviewer), all 48 bundled examples use it, and the new docs/workflow-syntax.md section — plus this PR's own description — use it too. Someone copying this docstring example gets a validation error on a field whose entire purpose is to be understandable from its docs.
Suggested fix:
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.| if raw is None: | ||
| return agent | ||
| def _resolve_agent_directory( | ||
| self, agent: AgentDef, agent_context: dict[str, Any], field: str, raw: str |
There was a problem hiding this comment.
RECOMMENDED:
def _resolve_agent_directory(
self, agent: AgentDef, agent_context: dict[str, Any], field: str, raw: str
) -> str:field only exists to be interpolated into the error message and suggestion at lines 622 and 627. It sits right next to raw, has the same type, and both call sites pass the pair positionally (..., "working_dir", raw and ..., "settings_dir", agent.settings_dir). Swap the two and it still type-checks, producing a nonsense error — Agent 'x': /home/me/repo '/.../working_dir' does not exist — that names the path as the field and the field as the path. Any other string is accepted here too.
The codebase already reaches for Literal aliases in this exact situation: RunMode (fleet/records.py), CheckpointTrigger (engine/checkpoint.py), PluginFlavor (plugins/manifest.py). AGENTS.md notes RunMode exists specifically "so the single write site is checked by ty" — same reasoning applies here, and this new helper is exactly the kind of shared choke point where a swapped pair would be hardest to notice.
Suggested fix:
def _resolve_agent_directory(
self,
agent: AgentDef,
agent_context: dict[str, Any],
*,
field: Literal["working_dir", "settings_dir"],
raw: str,
) -> str:Keyword-only makes the two str args impossible to transpose, and the Literal turns an invalid field name into a type error.
| # 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 }}" |
There was a problem hiding this comment.
RECOMMENDED: This file references {{ workflow.input.workspace }}, {{ workflow.input.repo }} (lines 96, 104, 106), and {{ workflow.input.artifacts }} without declaring an input: block anywhere — grep -n 'input:' finds nothing.
validator.py:1761 only runs the unknown-input check if workflow_input_names and ..., so with zero declared inputs it's skipped entirely. That means the code this PR adds at validator.py:1277-1279 — collecting settings_dir for template scanning — never actually runs against the one bundled example that has a settings_dir. A typo there would still pass make validate-examples, the CI job, and TestExamplesRegression::test_all_bundled_examples_validate.
The sibling example this one is modeled on, examples/claude-agent-sdk-setting-sources.yaml, does declare its inputs. Declaring them here also documents the --input flags this file's own header already advertises, and it turns on the check for the feature's flagship example. It also compounds the empty-string issue at workflow.py:665 — an undeclared input passed as --input repo= renders empty and silently grants the workflow directory.
Suggested fix: add the three declarations, matching claude-agent-sdk-setting-sources.yaml:
input:
workspace:
type: string
required: true
repo:
type: string
required: true
artifacts:
type: string
required: true…s_dir `working_dir` was doing two unrelated jobs on this provider. The Claude CLI supports MCP Roots and advertises exactly one root -- its cwd -- so a filesystem MCP server discards the directories in its own argv and permits cwd alone. That makes cwd the only handle on what an agent can read, while it is simultaneously the directory the `project` settings tier resolves against. Narrowing cwd onto a target repository to pick up that repository's skills therefore narrowed the agent's MCP root below any sibling path the step still had to read; widening it back lost the repository's conventions. Add a per-agent `settings_dir`, forwarded to `ClaudeAgentOptions.add_dirs`, so the two are separable: the skills of the directory named there are discovered and invocable regardless of cwd, leaving cwd free to stay wide enough for the agent's MCP servers. The split is deliberately partial. A directory named here contributes its `.claude/skills` and nothing else -- not `CLAUDE.md`, `.claude/rules/*.md`, `.claude/settings.json` (so no `env`, no `hooks`) or `.claude/agents`, all of which keep following cwd. It is the skills portion of a project tier, not a cwd-independent way to load one. `add_dirs` also carries an unconditional effect the skills framing hides: per the SDK's own contract it is "additional directories Claude can access", so a `settings_dir` widens the model's built-in Read/Edit/Bash to that tree with no settings tier enabled at all. It does not widen what an MCP server permits. Both effects are documented on the schema field, and `conductor validate` warns when the field is set with no `setting_sources`, since the skills half is then a no-op while the filesystem grant applies. `WorkflowEngine._resolve_agent_directory` is extracted from `_resolve_agent_working_dir` so `working_dir` and `settings_dir` resolve identically and cannot drift. `capabilities.py::settings_dir` gates the field; `config/validator.py` errors against a provider with nowhere to put it rather than dropping it silently. The Roots behaviour every option here rests on is pinned by tests/test_integration/test_mcp_roots_negotiation.py, which drives the real filesystem server with no LLM -- two runs differing only in whether the client advertises `roots`.
Blocking:
- docs/providers/experimental.md: the capability row had been rewritten with
text describing per-agent `tools:` allowlist enforcement -- enumerating
stdio MCP servers, refusing an allowlist alongside http/sse, an
`mcp_servers_always_attached` capability. None of that exists here:
`workflow_tools_passthrough` is False, `_resolve_tool_config` refuses any
non-empty allowlist, and the identifier appears nowhere in `src/`. The row
also lost the accurate `no workflow_tools_passthrough` carve-out. Restored
to upstream's text plus one additive sentence for `settings_dir`, so the
table no longer promises a security property the provider does not have.
- Added `AgentExecutor._reject_unsupported_settings_dir`, mirroring the
`capabilities.settings_dir` check at run time. `conductor run` never calls
the static validator, and the engine resolves the directory for every
provider, so `provider: copilot` with a `settings_dir` previously ran
against whatever conventions its cwd supplied and exited 0. Follows the
four existing `_reject_*` helpers and `session_key`'s precedent; the two
docstrings that promised validate-time-only rejection now describe both.
- An empty `settings_dir` reached `add_dirs` as a real filesystem grant.
`Path("")` is `Path(".")`, which is not absolute, so it resolved to the
workflow file's own directory and passed the `is_dir()` check. Four layers
disagreed on what "set" meant; they now agree: the field takes
`StringConstraints(strip_whitespace=True, min_length=1)` (matching
`session_key`), the seven step-type guards use `is not None` so a blank
cannot bypass them, and the engine refuses a *template* that renders empty.
Recommended:
- `_project_tier_enabled` replaces `_setting_sources_enabled`: `settings_dir`
feeds only the `project` tier, so `['user']`/`['local']` no longer validate
silently, and a per-agent `skills: []` -- which zeroes the tier in
`execute` -- is now detected.
- The no-skills warning branches on its cause. Under a per-agent provider
override it no longer advises adding `setting_sources`, which the schema
rejects unless `runtime.provider` is claude-agent-sdk.
- docs/workflow-syntax.md said the field was "ignored by every other
provider"; it is refused. The provider comment led with the skills effect
and omitted the unconditional filesystem grant -- the grant now comes first.
- The schema docstring example used a mapping under `agents:`, which the
schema rejects.
- `_resolve_agent_directory` takes `field` keyword-only as a
`Literal["working_dir", "settings_dir"]`, so the two adjacent `str`
parameters can no longer be transposed (`ty` now rejects a bad name).
- The validator sub-agent sets `settings_dir=None` explicitly: it runs with
`tools=[]`, so neither half would apply, and inheriting it would grant a
tree it cannot use. Not switched to `model_copy`, which would also carry
`validator` (recursive validation), `session_key` and `routes`.
- The roots-negotiation test is marked `real_api` (it fetches from npm and
pins upstream behaviour, not ours), passes the resolved `npx` path so it
can launch `npx.cmd` on Windows, and writes stderr to a plain file rather
than reopening a `NamedTemporaryFile` by name.
- The bundled example declares its three inputs, which turns on the
unknown-input check for the `settings_dir` template this PR added -- a typo
there was previously undetectable by `make validate-examples`.
- Tests: the missing `questions` step-type case, an argv assertion that
`--add-dir` still reaches the CLI (with a negative control), the blank and
empty-render refusals, all three warning causes, and the run-time
capability refusal. Every new branch was mutation-tested.
…ents `settings_dir` is a trust decision -- it loads another repository's conventions and, unconditionally, widens the model's built-in Read/Edit/Bash to that tree -- and nothing in the run output mentioned it. Neither the dashboard nor the JSONL event log recorded which directory an agent had been granted, so the grant could not be audited after the fact. Emitted alongside `working_dir` at all three sites that already carry it: `agent_started`, `parallel_agent_started` and `for_each_agent_started`. Emitting on one path only would have left the other two silent about the same grant. Always present (null when unset) so a consumer can distinguish "no grant" from "this Conductor did not report one".
…w gaps The empty-render guard added in 0b1598a had been narrowed to `field == "settings_dir"` in 9b15ba7 -- a mutation-test edit that leaked into that commit rather than being reverted. Restored to unconditional, and the `working_dir` half is now pinned by its own test, which is what would have caught the leak. Also from a further review pass: - The `working_dir` behaviour change is now declared rather than silent. An empty-rendering `working_dir` previously resolved to the workflow file's own directory and ran there; it is now an error. Recorded in CHANGELOG.md under Changed, in the shared helper's docstring, and tested in its own right so a later edit narrowing the guard back to `settings_dir` cannot silently restore the old behaviour. - CHANGELOG.md had no entry for this feature at all. Added under Unreleased/Added, alongside the Changed note above. - The validator sub-agent's comment justified `settings_dir=None` with "no skill can be invoked without the Skill tool", which is false: `_resolve_tool_config` grants `Skill` back for `tools: []`, so with a settings tier enabled the grader does hold it (measured: `(['Skill'], None)`). The conclusion stands on the other half -- no file tool for the grant to widen -- so the reason is corrected rather than the code. Pinned by a test, since the consequence is latent today and becomes load-bearing the next time a tool is granted back. - AGENTS.md still described `settings_dir` as enforced only in config/validator.py. It is enforced twice; the file now says so, in the same words the skills and plugins bullets use. - The schema docstring stated the filesystem grant as a live exposure. Conductor reaches only `bypassPermissions` (full preset, where reads already succeed everywhere) or `tools: []` (at most the `Skill` loader, no file tool), so the grant is a property of the SDK contract to design against rather than something reachable from a workflow today. The docs page already carried that hedge; the docstring, which is what surfaces on hover, did not. - `settings_dir` had no coverage inside parallel groups or for-each loops. Both are now tested, the for-each one asserting per-iteration resolution -- that site resolves after loop-variable injection, so a refactor could otherwise load a different iteration's skills unnoticed.
The event emission added for `settings_dir` covered three paths but only `agent_started` was tested: both group payload lines could be deleted with the suite fully green (8626 passing, and 264 passing across every file that mentions those event names). `TestSettingsDirInGroups` does not close the gap -- it asserts the value reaching `provider.execute`, which routes through `_resolve_agent_working_dir`, a different path from the payload. That is the same shape as the mutation leak the previous commit fixed: an unpinned line in a shared payload where the loss is silent. The tree already had the precedent -- `TestWorkingDirEvents` exists to pin these same two additive events for `working_dir`, with an `is None` control for each -- and the emission commit did not follow it. Adds `parallel_agent_started` and `for_each_agent_started` assertions plus a negative control, so the class now covers what its docstring claims and what `AGENTS.md` asserts. The for-each case checks per-iteration values, since that is where a templated `settings_dir` varies and most needs auditing. Verified by deleting both payload lines: all three new tests fail. Also records why the guard comment covers both fields (the `settings_dir`-only rationale sitting on that line is the reasoning that produced the earlier narrowing), and why the argv helper is local rather than reusing another test class's private one.
…o skills A `settings_dir` whose `project` tier is not enabled discovers nothing -- and the filesystem grant applies regardless, so the one effect the author did not ask for was the only one they got, with no diagnostic. Traced through the real provider: `add_dirs=['/target']` with `setting_sources=[]` and `skills=[]`, and no log output. `conductor validate` warned about this; `conductor run` never calls the static validator. That is the same gap this branch already closed twice for `capabilities.settings_dir`, and forgetting `setting_sources` is the likelier mistake of the two -- it lives at workflow scope while `settings_dir` is per agent, and the bare-string `provider: claude-agent-sdk` shorthand cannot carry it at all. Warned rather than raised, matching validate's own choice: the workflow is not wrong, just ineffective. Placed where `effective_sources` is already computed, so it also covers the two cases a workflow-level check would miss -- a non-`project` tier, and a per-agent `skills: []` that zeroes the tier for that agent. Parametrised over all five, and mutation-verified. Also: the schema docstring and the docs section opened with "Directory whose Claude Code *project* settings tier this agent loads", which over-promises (a reader expects CLAUDE.md, rules, env, hooks) and does not mention the grant at all. Both now lead with what it carries and what it grants; the prose already had the detail, but the first line is what a tooltip shows and the field name is what appears in every workflow file.
…descriptions The previous commit added a run-time mirror of the "settings_dir discovers no skills" warning and then updated only three of the five places that describe it: - AGENTS.md carried the new parenthetical AND the sentence it was meant to replace, stating the same thing twice. That file is loaded as agent instructions, so a garbled invariant there is asserted as fact by whatever reads it next. - The schema docstring still said `conductor validate` warns -- true before this branch, false after it, and an understatement of the guarantee the previous commit exists to provide. The same docstring already applies the twice-enforced idiom to the capability refusal six lines earlier. Both now match the code. The other three sites (provider message, docs section, CHANGELOG) were already correct. Also latches the warning per agent. It was firing once per execution, so a 50-item for_each emitted 50 identical lines naming the same agent and the same directory (measured: 5 warnings for 5 executions). Keyed by agent name rather than a bare flag, because a global latch would silence a *second* affected agent -- and naming the directory is the point of the warning. Follows the `_warned` latch convention in claude.py and engine/workflow.py. Two mutations confirm the test pins both properties: removing the latch, and making it global, each fail it.
The latch added in e17c0cb did not fix the case its own message cited. The engine renames a for_each member per item (`<agent>[<key>]`, engine/workflow.py), so keying on `agent.name` gave every iteration a distinct key: an 8-item loop still emitted 8 warnings naming the same directory. Measured before and after. Keyed on the resolved directory instead, which dedupes both a for_each fan-out and repeated loop-backs. The trade is recorded at the declaration: two differently-named agents naming the SAME directory now warn once, naming only the first. Accepted because the actionable content is the directory and the remedy is workflow-global, so the second line would add nothing -- and the alternatives are worse. A bare flag would hide a second directory entirely (`settings_dir` is rendered per execution, so one agent can name several), and any name-bearing key reinstates the for_each noise. The test now drives the engine's own naming -- eight `fan[N]` iterations over one directory, plus one agent naming another -- rather than a name repeated verbatim, which no group path produces. Three mutations confirm it: no latch, a global flag, and the name key each fail it. Also branches the run-time remedy on its cause, as config/validator.py already does. With `setting_sources: [project]` and an agent's own `skills: []`, the message told the author to add 'project' -- already present, so the advice was a no-op and the warning kept firing. That is the same unactionable-advice defect raised against the validate-time message earlier in this branch; the run-time mirror had inherited only one of its three branches.
…he third cause Two defects the previous commit introduced while fixing its own. The directory-only latch was justified with "the remedy is workflow-global, so the second line would add nothing" -- true of the unbranched message, and falsified by the remedy branch added in the same commit. Measured: no tier enabled, one agent with `skills: []` and one without, both naming the same directory -> a single line, naming only the first, prescribing a fix that is wrong for the agent it does not name. Now keyed on `(directory, cause)`, which keeps the for_each collapse (all members share a cause) and bounds the output at two lines per directory. The residual trade -- same directory AND same cause warns once -- is recorded, and holds, because the remedy is then identical for both. `config/validator.py` distinguishes three causes; the run-time mirror had ported two. The missing one is a per-agent `provider: claude-agent-sdk` override under a different `runtime.provider`: `providers/registry.py` forwards structured settings only to the matching provider, so the agent reaches the provider with no `setting_sources` and the warning fired telling the author to add it -- which the schema rejects unless `runtime.provider` is itself claude-agent-sdk, exactly the unactionable advice this branch fixed at validate time earlier. The provider cannot see the workflow-level provider name, so rather than plumb it through, the non-opt-out arm now names the requirement instead of prescribing the edit, which is true on both paths. AGENTS.md records the key and its accepted cost. Three mutations confirm the tests pin it: dropping the cause from the key, reverting to the agent name, and collapsing the remedy branch each fail.
…measurement No behaviour change; three gaps a review pass found in the prose around the settings_dir tier warning. The opt-out remedy assumes the `project` tier is otherwise present. An agent with `skills: []` under a `user`/`local`-only tier is told to remove the opt-out, which is necessary but not sufficient, so that author reaches a working config in two warnings rather than one. Both layers say the same thing, so there is no divergence -- but the limit was undocumented. Distinguishing it would need a third cause value and a wider latch key, to serve a combination requiring two unusual settings at once, so AGENTS.md records the limit instead. Two test docstrings described the code as it was rather than as it is: the latch test named only the directory half of a key that is now `(directory, cause)`, and the no-tier-remedy test claimed to exercise the per-agent provider override, which it cannot -- the provider never receives the workflow-level provider name, which is exactly why one shared remedy arm is right. That docstring now says what the test can and cannot pin, and points at the validate-time test that does cover the override path. The filesystem-grant measurement is now pinned to `claude` CLI 2.1.263. Later builds dropped `default` from `--permission-mode`'s accepted names, so the stated reproduction needs the version named. Conductor never passes that mode explicitly, so nothing in the code is affected.
8d9867b to
c67c4fb
Compare
|
All 14 addressed (three blocking, ten recommended inline, plus Blocking
Restored to
Recommended
Further changes of my ownI put the branch through further review passes after fixing the above. Flagging these because several are corrections to claims I made in this PR.
The empty-render guard had been narrowed to The
A comment of mine stated something false. The validator sub-agent's The schema docstring overstated the filesystem grant. It read as a live exposure, but The skills half could no-op with no run-time diagnostic. The field name and both opening lines over-promised. The docstring and docs section began "Directory whose Claude Code project settings tier this agent loads" — a reader expects Also added VerificationEvery new branch was mutation-tested — guard deleted or condition inverted, then the relevant test re-run to confirm it fails. The full list is in the PR description's Verification section, so the two cannot drift apart.
|
Jason Robert (jrob5756)
left a comment
There was a problem hiding this comment.
LGTM, thanks for contributing!
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #514 +/- ##
=======================================
Coverage ? 91.90%
=======================================
Files ? 164
Lines ? 26708
Branches ? 0
=======================================
Hits ? 24545
Misses ? 2163
Partials ? 0 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Fixes #513.
What changed
Adds a per-agent
settings_dir, forwarded toClaudeAgentOptions.add_dirs, so theprojectsettings tier's skills can be read from a directory other than cwd. Nothing changes unless a workflow sets it.WorkflowEngine._resolve_agent_directoryis extracted from_resolve_agent_working_dirso both fields are Jinja-rendered,~-expanded, absolutized against the workflow file's directory,normpath-normalised and existence-checked by the same code and cannot drift.settings_diris per-agent only — noruntime.counterpart, since the repository whose conventions apply is what varies between steps.ProviderCapabilities.settings_dirgates it (claude-agent-sdkis the onlyTrue), andconfig/validator.pyerrors against a provider with nowhere to put it rather than dropping it silently — same class asworking_dir.Two things worth review attention
1.
add_dirscarries an unconditional filesystem grant, which the skills framing hides.Per the SDK's own contract
add_dirsis "additional directories Claude can access beyond the current working directory". Sosettings_dirwidens the model's built-inRead/Edit/Bashto that tree with no settings tier enabled at all — measured atpermission_mode: "default"withsetting_sourcesunset: a read outside cwd is refused without it and succeeds with it. It does not widen what an MCP server permits — verified by recording the CLI's ownroots/listanswer, which stays cwd-only when asettings_diris set.That grant is latent rather than reachable from a workflow today, and the docs and schema docstring now say so:
_resolve_tool_configyields onlybypassPermissions(the fullclaude_codepreset, where reads already succeed everywhere) ortools: [](at most theSkillloader, no file tool at all), so Conductor never reaches a permission mode where the widening is observable. It is a property of the SDK contract to design against, not an exposure. Worth review attention because it constrains what a future carve-out in_resolve_tool_configmay safely grant back.Both effects are documented on the schema field.
conductor validateand the run itself both warn (not error) whensettings_diris set without theprojecttier enabled — the run needs its own warning becauseconductor runnever calls the static validator. A warning rather than an error because the workflow is not broken: the skills half no-ops, but the filesystem grant still applies, and the field's name suggests only the skills half. If you would rather that be an error, or the grant documented differently, say so.2. Why this is not derived from the MCP servers' own directory arguments.
That is the intuitive fix and it cannot work, which is worth stating because the code deliberately does not do it. A filesystem MCP server uses its argv directories only when the client does not support MCP Roots; the CLI does support Roots and advertises exactly one, its cwd; so the server discards its argv directories and permits cwd alone.
--add-dirtakes no part in that negotiation. Anything derived from server args and passed here would read as a mitigation while changing nothing about what the server permits — and would additionally widen skill discovery to directories the author named as data. There is a comment at the call site saying so.tests/test_integration/test_mcp_roots_negotiation.pypins the rule itself: it drives the real@modelcontextprotocol/server-filesystemover stdio with no LLM, two runs differing only in whether the client advertisesroots, and asserts the allowlist that results. It is markedreal_api, so CI's default-m "not real_api and not performance"deselects it, and it skips whennpxis absent.The complementary half — that the CLI advertises exactly one root and that it is cwd — needs a live
claude, so it is measured rather than pinned: with an MCP server logging the client handshake against CLI 2.1.263,rootsis advertised, the answer is one root tracking cwd, and adding asettings_dirleaves it unchanged.Scope, stated plainly
settings_diris the skills third of a project tier, not a cwd-independent way to load one. A directory named there contributes its.claude/skillsand nothing else — notCLAUDE.md,.claude/rules/*.md,.claude/settings.json(so noenv, nohooks), not.claude/agents, all of which keep following cwd. Established by placing each artefact in asettings_dirwith cwd elsewhere and confirming only the skills became listed — for hooks, with aPreToolUsehook writing a file, and a control run with cwd on the repository that does fire it.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 still cannot have both from these fields alone.
settings_dirrecovers the skills; the rest is a caller-side trade. That limit is documented on the field rather than left to be discovered.One behaviour change to an existing field
A
working_dirtemplate that renders to an empty string is now an error. Previously it resolved to the workflow file's own directory —Path("")isPath("."), not absolute, so it was joined onto that directory and passed the existence check — and the agent ran there.The guard exists for
settings_dir, where an empty value would hand the workflow's own tree to the model's file tools. It lives in the helper both fields share, and I extended it rather than scoping it:working_dirrunning an agent in the wrong directory is the same defect without the grant, and neither seemed worth preserving. Recorded inCHANGELOG.mdunder Changed, stated in the helper's docstring, and tested for both fields — theworking_dircase in its own right, so a later edit narrowing the guard cannot silently restore the old behaviour.Flagging it because it is a behaviour change to a stable field in a PR about a new one. If you would rather
working_dirkeep its current behaviour, scoping the guard is a one-line change and I will add the negative test instead.Verification
tests/test_integration/test_mcp_roots_negotiation.py— 3 passed under-m real_apiagainst the real filesystem MCP server. Markedreal_apibecause it fetches the server from npm and pins upstream's negotiation behaviour rather than Conductor's own code, so CI's-m "not real_api and not performance"deselects it; it also skips whennpxis absent.New tests:
tests/test_config/test_settings_dir_schema.py(29),TestSettingsDirAddDirsintests/test_providers/test_claude_agent_sdk.py(7), plus engine resolution, group-path and event-emission tests intests/test_engine/test_workflow.py.tests/test_config,tests/test_engine,tests/test_providers— pass in full.ruff check src tests,ruff format --check src tests,ty check src— clean (tyreports the same 6 pre-existing diagnostics asmain).conductor validate examples/claude-agent-sdk-settings-dir.yaml— passes.Rebased onto current
main(544a2bf), which landed fix(providers): retry transient OpenAI stream errors #506 and fix(providers): guard compaction against token estimate drift #507 after this branch was first pushed. The only conflict wasCHANGELOG.md, where both sides added Unreleased entries; resolved by putting thesettings_dirbullet under the existing### Addedand leaving fix(providers): guard compaction against token estimate drift #507's### Fixeduntouched.Full suite after the rebase: 8671 passed, 3 failed, 42 skipped. The three are the same environment-dependent
chmodtests reported on feat(claude-agent-sdk): opt-in setting_sources for target-repo skills #502 and fix(gates): read a terminal dialog reply as one multi-line turn #510, and they reproduce on a clean checkout ofmain:tests/test_skills/test_path_entries.py::TestUnreadableParent::test_unreadable_parent_is_reported_not_raised_rawtests/test_skills/test_path_entries.py::TestSkillsRootDiagnostics::test_mis_cased_skill_md_is_reportedtests/test_plugins/test_registry.py::TestUnreadableTrees::test_unreadable_skill_subdirectory_is_reportedAll three depend on a
chmod-unreadable path or a case-sensitive filesystem, neither of which holds for my user on macOS APFS.Zero regressions: the failure set on this branch is byte-identical to the failure set on a clean
mainworktree, compared test-id by test-id in both directions.Every new branch was mutation-tested — guard deleted or condition inverted, then the relevant test re-run to confirm it fails. That includes the two capability refusals, the blank and empty-render guards for both fields, all three warning causes, the
questionsstep-type rejection, the--add-dirargv assertion, the event emission, the validator sub-agent's non-inheritance, per-iteration resolution in for-each, and the tier warning's latch key.