[ON HOLD — upstreamed as microsoft/conductor#514] PDA-95 Separate the project settings tier from cwd with settings_dir

[throup]

Splits the two unrelated jobs working_dir was doing on the claude-agent-sdk provider, and removes a mitigation that never worked. Fixes the conductor half of PDA-95.

The claude CLI negotiates MCP Roots and advertises exactly one root — its cwd — so @modelcontextprotocol/server-filesystem discards the directories in its own argv and permits cwd alone. cwd is simultaneously what the project settings tier resolves against. Pointing working_dir at a target repository to reach its skills therefore narrowed the agent's only MCP root below any sibling path the step still had to read; widening it back lost the repository's skills.

_stdio_path_args existed to prevent exactly this, forwarding every stdio server's directory args into add_dirs with a docstring asserting it "restores the declared scope". It cannot: --add-dir takes no part in Roots negotiation. It is removed rather than documented as ineffective, because it also had a live cost — deriving skill discovery from server arguments grants conventions from directories the author named as data.

New per-agent settings_dir, now the only source of add_dirs:

agents:
  judge:
    settings_dir: "{{ setup_worktree.output.worktree_path }}"
    # No working_dir -> cwd stays wide, so the MCP root still contains
    # the artifacts directory this judge reads.

14 files, +1194 −56. Three commits: the field; a docs/test correction after further measurement; then the fixes from two independent reviews (one of which saw only the diff).

Worth scrutinising

1. settings_dir carries skills only — it is not a general "load a project tier from anywhere". Measured against the real CLI, with a fixture whose .claude/rules is a symlink to .agents/rules and a CLAUDE.md that never mentions it:

	rules	hooks	CLAUDE.md	skills
cwd = repo	yes	yes	yes	yes
cwd wide + add_dirs=[repo]	no	no	no	yes

So this recovers one third of what #4 (PDA-16) delivers, and the two demands genuinely conflict — one directory cannot be both the repo and a wide MCP root. That limit is now in the docs as a table and pinned by a test; the second commit exists because my first pass tested skills only and documented the rest optimistically.

2. The regression test runs the real MCP server with no LLM. tests/test_integration/test_mcp_roots_negotiation.py pins the negotiation rule, not our wiring: two runs, byte-identical but for whether the client advertises the roots capability, flipping the allowlist from both declared roots to cwd alone. Everything here rests on that precedence, so it is worth a look — including the deliberate wait for the server's own "Updated allowed directories" line, without which the probe races the swap and reports the opposite result. That false negative is how I first mis-measured this.

3. settings_dir is a filesystem grant, and it 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 to that tree whether or not any settings tier is enabled — the skill discovery it is normally set for is the conditional half. Measured with setting_sources unset and permission_mode: default: the same read outside cwd is refused for permissions without settings_dir and succeeds with it. It does not widen what a filesystem MCP server permits. This is the security-relevant line in the diff, and the docs table originally omitted it entirely — a blind review caught that.

First objection a reviewer will raise

Why a new field instead of just fixing the docstring? Because AC2's fallback branch ("document it as ineffective") would leave the derived add_dirs in place, and it is not inert — it silently widens skill discovery to every declared MCP root. Removing it needs no upstream coordination: _stdio_path_args is fork-local, added in 4a81064.

grep add_dir tests/ previously returned nothing, which remains the most direct explanation for how an ineffective mitigation shipped and stayed.

Verification

• uv run pytest tests/test_config tests/test_engine tests/test_providers tests/test_skills tests/test_plugins tests/test_integration/test_mcp_roots_negotiation.py → 4318 passed, 2 skipped, 3 failed. The three are pre-existing on clean main (two need a genuinely unreadable directory, one a case-sensitive filesystem) — confirmed by stashing and re-running on main.
• Full suite: 7884 passed, same three pre-existing failures.
• ruff clean; ty reports the same 6 diagnostics as main; make validate-examples passes including the new example.

What this does not fix

Judge this on its own merits, not as unblocking anything downstream. It was
motivated by a caller whose steps are denied access to a sibling artifacts
directory, and it does not fix that: those steps sit in two sibling trees,
neither containing the other, so widening one step's cwd cannot bring the
other's paths into scope. That caller needs the artifact path to sit inside
whatever cwd each reader already has — a placement decision in the caller's
repo, needing nothing from conductor. Shared symptom, different problem.

What is on offer here is narrower and stands alone: the mechanism documented
where it was previously asserted backwards, an ineffective mitigation removed,
a regression test for the negotiation rule everything rests on, and
settings_dir for the case where a step's repo-side need genuinely is skills
alone.

🤖 Generated with Claude Code

[throup]

Full measurements, per-file rationale, and the two wrong turns this went through

How the mechanism was established

Every claim below is from a run, not from reading source. The provider's own docstring asserted the opposite of the observed behaviour, so reading it is what produced the original defect.

Can the CLI advertise more than one MCP root? No.

• grep -rni roots over the installed claude_agent_sdk Python source: zero matches.
• ClaudeAgentOptions has three directory/MCP fields: mcp_servers, strict_mcp_config, add_dirs. None advertises roots.
• The bundled CLI's --help offers one directory flag, --add-dir, documented as "Additional directories to allow tool access to". No --mcp-root or equivalent.

What --add-dir actually does

Fixture: exp/rootA, exp/rootB (holds target.txt), exp/cwdC. rootB additionally ships .claude/skills/repoconv, .claude/agents/rootbagent.md, .claude/settings.json (env marker + SessionStart hook), .claude/rules → symlink to .agents/rules, and a CLAUDE.md marker.

Skills are added, and cwd is unchanged between the two runs. cwd = exp, --setting-sources project:

	repoconv in the skill listing
with --add-dir exp/rootB	yes
without it (control)	no

Invocation, not just listing: "Use the repoconv skill and tell me its magic marker" → ROOTB-CONVENTIONS-LOADED.

Additive, not a replacement. cwd = cwdC (own skill pandconv) + --add-dir rootB → both pandconv and repoconv listed.

Everything else follows cwd. One run, cwd = exp, --add-dir exp/rootB:

RULE=NONE; HOOK=NONE; CLAUDEMD=NONE; SKILL=yes

echo $ROOTB_SETTINGS_MARKER → empty. Baseline for comparison, cwd = rootB, no --add-dir:

RULE=ROOTB-RULE-LOADED; HOOK=ROOTB-HOOK-LOADED; CLAUDEMD=ROOTB-CLAUDEMD

All properties at once, plus the bug's negative control. Real filesystem MCP server declared with both roots:

cwd	--add-dir	list_allowed_directories	read rootB/target.txt	repoconv
exp (common parent)	rootB	exp	hello-from-rootB	loaded
rootA (a declared root)	rootB	rootA	Access denied … not in …/rootA	—

Row 1 is the fix working. Row 2 reproduces the reported denial and confirms --add-dir does not widen MCP scope.

Through the SDK, not only the CLI. Row 1 reproduced via ClaudeSDKClient with cwd=exp, add_dirs=[exp/rootB], setting_sources=["project"], skills="all" — the exact option surface providers/claude_agent_sdk.py builds. Identical on all three assertions.

Two wrong turns, recorded because each looked correct

1. "argv roots survive Roots negotiation." My first hand-rolled probe sent tools/call immediately after replying to roots/list and got both declared roots back — the opposite of the truth. The server swaps its allowlist in the continuation of its own listRoots() await, so the call raced the swap and read the pre-swap list. Its stderr said Updated allowed directories from MCP roots: 1 valid directories while the tool answered with two. The test now waits for that line, and the reason is a comment in it.

2. "settings_dir replaces working_dir for target-repo conventions." I tested skills, found them carried, and wrote the docs as though the rest followed. It does not — see the rules/hooks/CLAUDE.md table above. The second commit is that correction; the docs had named settings.json and agents but omitted rules, which is the omission most likely to mislead someone choosing between the fields.

Per-file rationale

File	Why
providers/claude_agent_sdk.py	Removes _stdio_path_args; add_dirs becomes [agent.settings_dir] or []. [] not None: the SDK types the field as list, and None merely happens to be falsy.
config/schema.py	The field, its docstring, and rejection on the seven step types with no LLM session. Also corrects ProviderSettings.setting_sources' docstring, which asserted the cwd coupling this removes.
engine/workflow.py	Extracts _resolve_agent_directory so both fields share rendering/absolutising/existence-checking and cannot drift. settings_dir is per-agent only — no runtime. counterpart, since the repository whose conventions apply is what varies between steps.
tests/test_integration/test_mcp_roots_negotiation.py	Pins the negotiation rule (real server, no LLM). New file.
tests/test_providers/test_claude_agent_sdk.py	settings_dir → add_dirs; server args are not forwarded; independence from cwd; verbatim passthrough; and that a settings_dir is never promoted into cwd.
tests/test_engine/test_workflow.py	Resolution, templating, relative paths, and that settings_dir alone leaves working_dir unset.
tests/test_config/test_settings_dir_schema.py	Shape and the seven rejections. New file.
examples/claude-agent-sdk-settings-dir.yaml	The reviewer-reading-a-target-repo shape, with the mechanism in the header. New file.
AGENTS.md, docs/workflow-syntax.md, docs/providers/experimental.md	The mechanism, the limits table, and a correction: the experimental table still said setting_sources was "pinned empty", which this fork's #4 already changed.

Design alternatives considered

Option	Why not
One filesystem MCP server per declared root	Works, but N processes and an aggregated list_allowed_directories. Unnecessary: a single root that contains both already permits both, which the table above shows.
Generic per-agent add_dirs: list	Maps 1:1 onto the flag, but its name advertises "tool access" while the load-bearing effect here is skill discovery — the exact misreading that produced this bug.
Docstring fix only (AC2 fallback)	Leaves the derived forwarding in place, and it is not inert. See the PR description.

Not done

.claude/rules and hooks still require working_dir, so an agent needing a repository's rules and a wide cwd cannot have both. That is stated in the docs and pinned by a test rather than worked around; the caller-side resolution is tracked on the ticket.

🤖 Generated with Claude Code

[throup]

Review findings addressed (fefbdc5)

Two independent reviews were run against this branch: one with full context (PR, comments, repo conventions), one given only the diff — barred from AGENTS.md, the tracker and gh pr view, so it had to form its own account of the change. That second reviewer found the most serious issue, which the full-context one missed: it had no framing to be anchored by.

Every finding below was re-verified by execution before fixing — a reviewer's finding is a hypothesis.

Fixed

1. No capability carve-out (blocking). capabilities.py:148 states the rule in its own words for the sibling field — "silently ignoring the directory would run the agent in the wrong repository" — and working_dir and session_key both have validator checks. settings_dir had none. Verified: settings_dir + provider: copilot → exit 0, no warning. Adds ProviderCapabilities.settings_dir (True only on claude-agent-sdk) and the error. Worse, this PR's own description called it "ignored by every other provider" as a neutral fact, which made the omission look intentional.

2. Silent no-op without setting_sources (blocking). No tier enabled → the skills half does nothing, and validate said nothing. Now a warning, deliberately not an error, because the filesystem half still applies — see 3.

3. The docs omitted the effect that matters most (blocking, from the blind review). add_dirs' SDK contract is "additional directories Claude can access beyond the current working directory". So settings_dir widens the model's built-in Read/Edit/Bash to that tree, unconditionally — no settings tier required. The docs table answered "no" to four things and never mentioned filesystem access at all.

Measured, setting_sources unset, permission_mode: default:

	native Read outside cwd
without settings_dir	"Claude requested permissions to read … but you haven't granted it yet"
with settings_dir	succeeds

Now the first row of the docs table, with a callout, plus the schema docstring and AGENTS.md. It still does not widen what an MCP server permits.

4. Ticket key in AGENTS.md (blocking). The first in that file — all 82 other provenance markers are upstream issue refs, and a fork-local key is unresolvable to an upstream reader. Dropped.

5. Templates escaped reference validation. settings_dir was missing from _collect_template_strings. Verified: {{ nonexistent_step.output.x }} → settings_dir exit 0, working_dir exit 1. Now caught, using getattr to match the convention two lines below (duck-typed fixtures predate the field — direct access broke a pre-existing test, which is how I found that out).

6. A test that read as a guard and wasn't. test_settings_dir_does_not_become_a_second_cwd asserted str(narrow) not in cwd, which also passes for an implementation setting cwd to an unrelated third directory. Replaced with the assertion its docstring claimed. Both reviewers flagged this independently.

7. Stale call-site comments. All three sites said "Resolve working_dir" while resolving both fields. Fixed. I also renamed _resolve_agent_working_dir → _resolve_agent_directories and reverted it: four other modules reference it by name in comments, so the churn cost more than it explained. A docstring note now points there instead.

Not fixed, with reasons

• The integration test imports no conductor code and cannot fail on a conductor regression. Correct, and intended: it pins an external precedence rule that every conclusion here rests on. TestSettingsDirAddDirs pins conductor's own behaviour. Renaming the file to say "probe" rather than "regression" would be fair.
• The test is non-hermetic — npx -y hits the network on a cold cache (measured 8.9s cold vs 2.8s warm) and the server is version-unpinned. Real. A pin plus an opt-in marker is the right fix; I've left it as its own change rather than widening this PR.
• The race guard degrades silently. If the server's log wording changes, the 30s wait becomes a no-op and CI just gets slower. The blind reviewer proved this by setting the marker to a string that never appears — all three tests still passed, in 62s. Asserting the marker appeared would fail loudly. Worth doing; not done here.
• The CLI half of the central claim is untested in-repo. Both reviewers noted that "the CLI advertises exactly one root, its cwd" is asserted in several places with no artifact a reader can check — the server half is verified, the CLI half rests on my manual runs. That's a fair criticism of the evidence's reproducibility and I have no cheap fix: it needs a live claude invocation, which the suite deliberately avoids.
• settings_dir is absent from agent_started event payloads (where working_dir appears), so it is invisible in the dashboard and JSONL log. Genuine gap for a field that changes which repository's conventions were loaded. Follow-up.
• No parallel/for-each/sub-workflow test, though working_dir has five. I verified by execution that all three paths resolve it (three call sites, one shared method), so this is coverage asymmetry rather than a bug — but "shared code cannot drift" is exactly the reasoning a future edit invalidates silently.

Verification

uv run pytest tests/test_config tests/test_engine tests/test_providers tests/test_skills tests/test_plugins tests/test_integration/test_mcp_roots_negotiation.py → 4318 passed, 2 skipped, 3 failed. The three are pre-existing on clean main (two need a genuinely unreadable directory, one a case-sensitive filesystem). ruff clean; ty unchanged at 6 diagnostics; make validate-examples passes.

Neither reviewer could falsify the empirical claims; the full-context one independently confirmed the zero-roots SDK result, the []-not-None type choice, and that checkpoint/resume correctly needs no persistence (checkpoints store no AgentDefs).

🤖 Generated with Claude Code

[throup]

On hold — proposed upstream instead

This repository is a fork of microsoft/conductor, and the defect this addresses is in upstream code rather than anything we added. Rather than carry a permanent fork-local patch, the fix has been raised upstream:

• Issue: claude-agent-sdk: working_dir must be both the MCP root and the settings tier, and cannot be both microsoft/conductor#513
• Pull request: feat(claude-agent-sdk): select the project settings tier with settings_dir microsoft/conductor#514

This PR stays open and unmerged until that is resolved.

• Upstream accepts → take the fix via an upstream sync and close this PR without merging.
• Upstream rejects, or stalls → merge this PR and maintain the fix as a fork-local patch.

The upstream version is narrower than this branch

Same 14 files, but +1196/-34 upstream against +1203/-56 here. The difference is one thing, and it is the reconciliation detail that matters:

This branch removes _stdio_path_args; upstream has nothing to remove. That helper is fork-local, added by 4a81064 ("enforce per-agent and per-server tool allowlists") along with the add_dirs derivation it fed. Upstream's claude_agent_sdk.py has never set add_dirs at all, so there settings_dir is a pure addition (+24/-0 in that file) rather than a replacement.

Consequences if we reconcile:

1. The removal half of this branch has no upstream counterpart. An upstream sync that brings settings_dir in will not delete our _stdio_path_args, because upstream never had it. If we take the fix that way, that deletion still has to happen here — otherwise both mechanisms write add_dirs and the fork-local one wins on the servers it matches.
2. The narrative differs, deliberately. Much of this branch's prose argues why deriving add_dirs from server argv was wrong. Upstream cannot read that as a correction of code it does not have, so there it is written forward — a "do not derive this from server arguments, here is why it cannot work" warning at the call site, plus the same reasoning in AGENTS.md and the test module docstring.
3. Rebased onto upstream main (ff4b312), which already carries setting_sources as microsoft/conductor#502 — the upstreamed form of our PDA-16 work. So the one thing settings_dir genuinely depends on is already there, and this needed no other fork-local commit. The conflicts on rebase were all adjacency with 4a81064, not dependencies.
4. AGENTS.md was rewritten against upstream's text, not ported from ours: their prose already reflects feat(claude-agent-sdk): opt-in setting_sources for target-repo skills microsoft/conductor#502's merged wording, and porting ours would have clobbered it.

Two claims from this ticket were settled while preparing the upstream version

Both were flagged on PDA-95 as open, and a third-party PR was not the place to leave them open.

Hooks are now a measured negative, not undetermined. My comment of 7 Sep recorded the hooks row as unearned, because the probe grepped --debug output and returned nothing in its own control. The instrument named there — a hook with an observable side effect — has now been run: a PreToolUse hook appending to a file fires with cwd on the repo and [project] enabled, and does not fire when the same repo is reached only via settings_dir, tier or no tier. The control firing is what makes the negative meaningful. Docs, AGENTS.md and the schema docstring now agree; they previously contradicted each other on this.

"The CLI advertises exactly one root, its cwd" now has direct evidence. Previously it rested on manual runs with no in-repo artifact. Measured against CLI 2.1.263 with an MCP server that logs the client handshake, driven through the provider: roots is advertised, the roots/list answer is exactly one root tracking cwd, and setting a settings_dir leaves that answer unchanged — which is the direct demonstration that --add-dir takes no part in Roots negotiation. That last run is what turns AC(1)'s mechanism from inference into measurement.

Still standing, unchanged

The gaps I listed on the ticket remain, and are named in the upstream PR rather than hidden:

• The CLI half of the central claim is measured but not pinned by a test — it needs a live claude, which the suite deliberately avoids.
• The integration test is non-hermetic (npx -y hits the network on a cold cache; the server is version-unpinned) and its race guard degrades silently if the server's log wording changes.
• settings_dir is absent from agent_started event payloads, so it is invisible in the dashboard and JSONL log.

Also unchanged: this does not unblock PDA-79. That correction stands exactly as recorded on the ticket and in this PR's description — PDA-79's blocked ACs need an artifacts-placement decision in pandora and nothing from conductor.

Verification of the upstream branch

Against claude CLI 2.1.263 and claude-agent-sdk 0.2.87, on upstream main (ff4b312):

• Roots negotiation test — 3 passed against the real filesystem MCP server.
• settings_dir schema (15), provider add_dirs (6), engine resolution (6) — all pass.
• ruff check, ruff format --check, ty check src — clean (ty's 6 diagnostics match main).
• Full suite — 8601 passed, 3 failed; the three are the chmod/case-sensitivity tests that fail identically on a clean checkout. Failure set byte-identical to a clean main worktree, compared test-id by test-id in both directions.

🤖 Generated with Claude Code