Uh oh!
There was an error while loading. Please reload this page.
feat(working-dir): working_dir for LLM agents and their MCP servers - #297
Conversation
…, capability gate Workflow authors can now set working_dir on an LLM agent or globally via runtime.working_dir; conductor validate errors when the selected provider cannot apply the directory. - AgentDef.working_dir: docstring extended to LLM sessions and their MCP servers - RuntimeConfig.working_dir: workflow-wide default inherited by agents without their own - Reject working_dir on human_gate/workflow step types (wait/set/terminate already rejected) - ProviderCapabilities.working_dir: True for copilot/claude, False for hermes/claude-agent-sdk - validator.py: hard error on working_dir against a working_dir=False provider, covering for_each inline agents and per-agent provider overrides
…lel/for_each
The engine now resolves each LLM agent's working_dir before the provider
call: agent value wins over runtime.working_dir, both levels are
Jinja-rendered against the per-agent context, ~ is expanded, relative
paths resolve against the workflow file's directory, and a missing
directory raises ExecutionError before any provider call.
- WorkflowEngine._resolve_agent_working_dir helper (normpath, no resolve()
so symlink aliases stay distinct)
- Linear path: context trim and agent-context build moved above
agent_started; the start event now carries working_dir alongside
context_window_max for LLM agents
- Parallel groups: each member resolved against its own context snapshot
- For-each loops: per-iteration qualified agent resolved after loop
variables are injected, so {{ item }} paths work
- Regression guard: templated model no longer clobbers the resolved
working_dir (model_copy merges)…t provider The Copilot provider now stamps the engine-resolved working_dir onto the SDK session's working_directory and onto every stdio/local MCP server config per execution, without mutating the shared server mapping. - session working_directory = agent.working_dir or os.getcwd() - _mcp_servers_for_cwd builds a per-execution copy (http/sse untouched) - Resume forwards working_directory + stamped mcp_servers; a changed cwd skips the resume and creates a fresh session (logged) - Checkpoint persists copilot_session_cwds (backward compatible: pre-cwd checkpoints load with an empty mapping and keep legacy resume-by-id) - Registry and resume CLI wire the cwd mapping through to the provider
…ovider Each distinct working directory now gets its own MCPManager so stdio MCP servers are spawned with that directory as their cwd. Parallel agents with different cwds stay isolated; agents sharing a cwd reuse one manager. - MCPManager.connect_server accepts cwd and forwards it to StdioServerParameters - ClaudeProvider pools managers per resolved cwd with per-cwd locks (double-checked locking, safe under asyncio.gather) - The manager is threaded through the agentic loop as a local variable, never stored as shared mutable state - close() shuts down every pooled manager, idempotently - Pool lifecycle bounded by provider lifetime; no eviction in v1 (documented)
The synthetic validator agent now receives the primary agent's engine-resolved working_dir, so the validation LLM call runs in the same directory as the agent being graded. No re-rendering: the value arrives already resolved from the engine. Also isolates a checkpoint-listing test from sibling artifact files by giving its workflow file a unique name.
Two additive LLM-only start events carry the engine-resolved working_dir
into the JSONL event log for observability:
- parallel_agent_started, emitted per group member right after its own
resolution (agent-level value wins over the runtime default)
- for_each_agent_started, emitted per iteration after the qualified
agent's {{ item }} template resolves
The pre-existing parallel_started / for_each_item_started envelopes are
unchanged; the dashboard frontend safely drops the unknown event types.- mcp-tools.md: new Working Directory section — runtime + agent level, precedence (agent > runtime > os.getcwd()), Jinja-dynamic paths, explicit NOT-a-sandbox warning - workflow-syntax.md: working_dir for LLM agents with restrictions (rejected on wait/set/terminate/human_gate/workflow), dialog-turn and sub-workflow rules, symlink semantics (lexical normpath; aliases are distinct Claude pool keys) - examples/working-dir.yaml: set step computes the path, the LLM agent binds working_dir to it and runs an stdio MCP server there - Test pins sub-workflow non-inheritance: the child resolves its own relative working_dir against the child workflow file's directory
Jason Robert (jrob5756)
left a comment
There was a problem hiding this comment.
This is a great addition. Reviewed end to end: schema, validator, the engine resolver across linear/parallel/for_each, Copilot session/MCP stamping plus checkpoint resume, and Claude's per-cwd MCP pool. The precedence and resolution logic hold up consistently across all three execution paths. Found one bug and a couple of test gaps worth closing before merge -- inline comments below.
| self._mcp_managers[resolved_cwd] = manager | ||
| return manager |
There was a problem hiding this comment.
This caches manager even when every configured server failed to connect for resolved_cwd. The fast path above (if resolved_cwd in self._mcp_managers: return ...) means a later call for the same cwd returns this same broken manager without ever calling connect_server again -- a one-time transient failure (spawn race, directory not fully populated yet) becomes permanent for the rest of the run, and after the initial logger.error there's no further signal that this cwd is running with zero MCP tools.
has_servers() already exists and is used for exactly this check a few lines down (if mcp_manager and mcp_manager.has_servers():), so gating the cache write on it fixes this without touching the locking structure:
| self._mcp_managers[resolved_cwd] =manager | |
| returnmanager | |
| ifmanager.has_servers(): | |
| self._mcp_managers[resolved_cwd] =manager | |
| returnmanager |
There was a problem hiding this comment.
Good catch — fixed in 4849fbf. The manager is now pooled only when has_servers() returns true; a zero-connection manager is returned but not pooled, so the next agent for the same cwd retries the connect, and a warning is logged so the cwd isn't silently running with zero MCP tools. The fail-open test (test_mcp_pool_connect_failure_fail_open_per_cwd) was extended to prove the retry: it now uses a stateful fail-then-succeed fake and asserts the second call builds a new manager and reconnects.
| patch("conductor.mcp.manager.MCP_SDK_AVAILABLE", True), | ||
| patch("conductor.mcp.manager.MCPManager", fake_cls), | ||
| ): | ||
| results = await asyncio.gather( |
There was a problem hiding this comment.
The fake connect_server in _manager_factory (around line 3232) never awaits anything, so asyncio.gather here runs all four coroutines to completion sequentially rather than actually interleaving them. I removed the entire locking mechanism (guard lock, per-cwd lock, and the re-check) from _get_mcp_manager_for_cwd and re-ran this test -- it still passes. The double-checked locking this test is meant to protect only gets exercised when something inside the critical section actually suspends.
Worth giving the fake a real yield point (await asyncio.sleep(0), or gate on an asyncio.Event released after the first N callers enter) so concurrent callers genuinely race, then assert the lock still produces exactly one manager per cwd.
There was a problem hiding this comment.
Fixed in 4849fbf. The fake connect_server now starts with await asyncio.sleep(0) so the four coroutines in asyncio.gather genuinely interleave inside the critical section, and the test additionally asserts exactly one connect sequence ran per cwd (per_cwd_connects == ["/repo/a", "/repo/b"]). The double-checked locking is now actually exercised.
| logged and remaining servers still connect). The pool must preserve | ||
| it across pool keys: cwd A failing to connect leaves cwd B fully | ||
| functional, and the failed key is not cached so a later retry can | ||
| succeed. |
There was a problem hiding this comment.
This claims the failed key isn't cached so a later retry can succeed, but the test never calls _get_mcp_manager_for_cwd("/repo/bad") a second time to check that. I did -- it returns the same never-connected manager with no second connect_server attempt (see the cache-write comment on claude.py:593). Either this docstring should describe the actual behavior (manager is cached even on total connect failure, no retry), or the missing retry test/behavior should be added.
There was a problem hiding this comment.
Fixed in 4849fbf, coupled with the cache-write fix above. The docstring's claim is now true: with the manager no longer pooled when zero servers connect, the test calls _get_mcp_manager_for_cwd("/repo/bad") a second time using a stateful fake (first attempt raises, second succeeds) and asserts a new manager instance was built and reconnected (attempts["/repo/bad"] == 2).
| resolved_agent = agent | ||
| if agent.type in (None, "agent"): | ||
| resolved_agent = self._resolve_agent_working_dir(agent, agent_context) | ||
| started_payload: dict[str, Any] = { | ||
| "agent_name": agent.name, | ||
| "iteration": agent_execution_count, | ||
| "agent_type": agent.type or "agent", | ||
| "context_window_max": await self._get_context_window_for_agent( | ||
| resolved_agent | ||
| ), | ||
| } | ||
| if agent.type in (None, "agent"): | ||
| started_payload["working_dir"] = resolved_agent.working_dir |
There was a problem hiding this comment.
Minor: agent.type in (None, "agent") is checked twice here -- once to decide whether to resolve, once to decide whether to include it in the payload. Naming it once removes the risk of the two checks drifting if that tuple ever changes in only one spot:
| resolved_agent=agent | |
| ifagent.typein (None, "agent"): | |
| resolved_agent=self._resolve_agent_working_dir(agent, agent_context) | |
| started_payload: dict[str, Any] = { | |
| "agent_name": agent.name, | |
| "iteration": agent_execution_count, | |
| "agent_type": agent.typeor"agent", | |
| "context_window_max": awaitself._get_context_window_for_agent( | |
| resolved_agent | |
| ), | |
| } | |
| ifagent.typein (None, "agent"): | |
| started_payload["working_dir"] =resolved_agent.working_dir | |
| is_llm_agent=agent.typein (None, "agent") | |
| resolved_agent= ( | |
| self._resolve_agent_working_dir(agent, agent_context) | |
| ifis_llm_agent | |
| elseagent | |
| ) | |
| started_payload: dict[str, Any] = { | |
| "agent_name": agent.name, | |
| "iteration": agent_execution_count, | |
| "agent_type": agent.typeor"agent", | |
| "context_window_max": awaitself._get_context_window_for_agent( | |
| resolved_agent | |
| ), | |
| } | |
| ifis_llm_agent: | |
| started_payload["working_dir"] =resolved_agent.working_dir |
There was a problem hiding this comment.
Fixed in 79a57b4. Extracted is_llm_agent = agent.type in (None, "agent") once and reused it for both the resolve decision and the payload inclusion. Checked the rest of the file for the same double-check pattern — the call sites at ~4882 and ~5365 are single checks after an early set-step return, so no change needed there.
| resume_sid = self._resume_session_ids.get(agent.name) | ||
| if resume_sid is not None: | ||
| try: | ||
| session = await self._client.resume_session( | ||
| recorded_cwd = self._resume_session_cwds.get(agent.name) | ||
| if recorded_cwd is not None and recorded_cwd != resolved_cwd: | ||
| logger.warning( | ||
| "Skipping resume of Copilot session %s for agent '%s': " | ||
| "working directory changed from %s to %s. Creating a new session.", | ||
| resume_sid, | ||
| on_permission_request=self._default_permission_handler, | ||
| agent.name, | ||
| recorded_cwd, | ||
| resolved_cwd, | ||
| ) | ||
| resume_sid = None | ||
| if resume_sid is not None: |
There was a problem hiding this comment.
Two things here:
resume_sidgets set back toNoneon the mismatch branch purely so the secondif resume_sid is not None:on line 890 reads as "else, actually try to resume." Folding this into oneif/elseon the same condition would be easier to follow than two sequential checks of the same variable.- The
recorded_cwd is Nonecase (checkpoints saved before this PR) falls straight through toresume_sessionwith zero logging. If someone resumes an old checkpoint after upgrading and addingworking_dir:, the old session resumes under a cwd it may never have run in before, with nothing in the logs explaining that assumption. Worth anlogger.infoin that branch, e.g. right before thetry:below:
ifrecorded_cwdisNone:
logger.info(
"Resuming Copilot session %s for agent '%s' with no recorded working ""directory (checkpoint predates working_dir tracking); assuming it ""matches resolved cwd %s.", resume_sid, agent.name, resolved_cwd,
)There was a problem hiding this comment.
Fixed in 178eecb. Restructured with a should_resume flag instead of mutating resume_sid, and added the logger.info for the recorded_cwd is None branch. I checked the installed SDK: working_directory is serialized into payload["workingDirectory"] for resume_session too, so the assumption is real — but since we can't prove how the runtime interprets it without a live session, the log says "checkpoint predates working_dir tracking, preserving legacy resume-by-id behavior" rather than claiming it matches. Added test_legacy_checkpoint_resume_logs_info covering resume-by-id, no create_session fallback, and the new log.
| #### Key Restrictions and Exclusions | ||
| - **Rejected Step Types:** The `working_dir` field is strictly rejected on `wait`, `set`, `terminate`, `human_gate`, and `workflow` (sub-workflow) step types. Defining `working_dir` on these steps raises a `ValidationError` at load time. |
There was a problem hiding this comment.
This lists rejected step types and implies every other step type resolves working_dir the same way the LLM-agent path does. script steps don't -- ScriptExecutor renders agent.working_dir on its own, with no runtime.working_dir fallback, no absolutize-against-the-workflow-file, and no pre-flight existence check (a missing directory surfaces as a raw subprocess error, not the ExecutionError the LLM path raises). A one-line callout here would keep someone who sets runtime.working_dir globally from assuming it also reaches script steps.
There was a problem hiding this comment.
Fixed in 3bd94ca — added a Script Steps bullet right after Rejected Step Types, documenting that script steps honor only their own working_dir (no runtime.working_dir fallback, relative paths resolve against the conductor process cwd, missing dirs surface as a subprocess startup error).
On the behavioral divergence itself: do you think it's worth a separate issue to decide whether ScriptExecutor should be brought to parity with the LLM-agent resolution (runtime fallback + workflow-file-relative absolutize + pre-flight existence check)? That's a behavior change beyond docs, and I'm happy to do the corresponding work if you and the maintainers want it.
There was a problem hiding this comment.
This sounds like a good follow-up on its own -- please go ahead and open an issue for it, and thanks for offering to pick it up. I'd rather keep this PR scoped to LLM agents and their MCP servers as it stands.
| async with self._mcp_manager_locks_guard: | ||
| lock = self._mcp_manager_locks.setdefault(resolved_cwd, asyncio.Lock()) |
There was a problem hiding this comment.
asyncio.Lock() construction and dict.setdefault() here are both plain sync calls with no await inside, so nothing can interleave between them on a single-threaded event loop -- _mcp_manager_locks_guard isn't actually protecting a contested critical section. I checked this with a standalone repro (50 concurrent tasks across 3 cwds, a real await asyncio.sleep() inside "connect"): removing the guard and running setdefault unguarded still produces exactly one manager per cwd. If you drop it, self._mcp_manager_locks_guard and its declaring comment a few lines up can go too.
| asyncwithself._mcp_manager_locks_guard: | |
| lock=self._mcp_manager_locks.setdefault(resolved_cwd, asyncio.Lock()) | |
| lock=self._mcp_manager_locks.setdefault(resolved_cwd, asyncio.Lock()) |
There was a problem hiding this comment.
Fixed in 4849fbf. Removed _mcp_manager_locks_guard (field + comment + usage); lock creation is now a bare get/assign with a comment noting the no-await invariant between the fast-path check and the per-cwd lock acquisition. The hardened race test (real await asyncio.sleep(0) yield in the fake) still produces exactly one manager per cwd, confirming the guard was dead weight. Also dropped the now-dead provider._mcp_manager_locks_guard fixture line in test_claude_mcp_tool_filter.py.
…lock A cwd whose MCP servers all failed to connect was cached forever: the unconditional pool write in _get_mcp_manager_for_cwd meant one transient spawn failure became permanent for the whole run, with no signal after the initial error. Now the manager is pooled only when has_servers() (a partially-successful manager is still reused); a zero-connection manager is returned but not pooled so the next agent for that cwd retries, and a warning is logged. Also remove _mcp_manager_locks_guard: it guarded only a synchronous dict.setdefault with no await in the critical section, so nothing could interleave on the event loop. Lock creation is now a bare get/assign with a comment noting the no-await invariant. Tests: the fake connect_server now awaits asyncio.sleep(0) so concurrent first-callers genuinely race, has_servers() reflects real connection state, the race test asserts exactly one connect per cwd, and the fail-open test uses a stateful fail-then-succeed fake to prove a failed cwd is retried and recovers. The tool-filter integration fixture drops its now-dead guard-lock assignment. Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
A checkpoint saved before working_dir tracking was introduced has no recorded cwd, so resume_session fell through silently: the old session resumed under the resolved cwd with nothing in the logs explaining the assumption. The SDK serializes working_directory into the resume payload, so the assumption is real. Emit an INFO log for this branch stating the checkpoint predates working_dir tracking and legacy resume-by-id behavior is preserved. Restructure the resume gate to use a should_resume flag instead of mutating resume_sid to None purely so a second identical check reads as 'else'. Adds test_legacy_checkpoint_resume_logs_info covering resume-by-id, no create_session fallback, and the new INFO log. Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
The agent.type in (None, "agent") check was evaluated twice when emitting agent_started: once to decide whether to resolve working_dir and once to decide whether to include it in the payload. Extract it into a single is_llm_agent flag so the two sites cannot drift if the tuple ever changes. Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
The Rejected Step Types bullet implied every non-rejected step resolves working_dir like LLM agents. Script steps do not: ScriptExecutor renders only the agent-level field, never falls back to runtime.working_dir, never absolutizes against the workflow file, and surfaces a missing directory as a subprocess startup error rather than the LLM path's pre-provider ExecutionError. Add a Script Steps bullet documenting the divergence. Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
# Conflicts: # src/conductor/providers/claude.py # src/conductor/providers/copilot.py
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@## main #297 +/- ##
=======================================
Coverage ? 89.63% =======================================
Files ? 72 Lines ? 13001 Branches ? 0 =======================================
Hits ? 11653 Misses ? 1348 Partials ? 0 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Jason Robert (jrob5756)
left a comment
There was a problem hiding this comment.
LGTM. Approved!
Uh oh!
There was an error while loading. Please reload this page.
Summary
Adds an optional
working_dirto LLM agents (and a workflow-wideruntime.working_dirdefault) so an agent and its MCP servers run in a chosen directory. Engine-resolved with precedence agent > runtime >os.getcwd(), supporting static values and Jinja templates.What's included
RuntimeConfig.working_dir+ per-agentworking_dir; rejected onwait/set/terminate/human_gate/workflowstep types; newProviderCapabilities.working_dir(copilot/claude = true, hermes/claude-agent-sdk = false) with aconductor validategate.working_directory+ per-server MCP cwd stamping;copilot_session_cwdspersisted in checkpoints; resume skips session resume when the cwd changed.MCPManager.connect_server(cwd)), with per-cwd double-checked locking so parallel agents with different cwds stay isolated and agents sharing a cwd reuse one manager.working_dir.parallel_agent_started/for_each_agent_startedcarry the resolvedworking_dirinto the JSONL log; existing envelopes unchanged (the dashboard safely drops unknown types).docs/mcp-tools.md,docs/workflow-syntax.mdupdates, andexamples/working-dir.yaml.Scope notes (deliberate)
working_diris not a sandbox — it only sets the process cwd; docs say so explicitly.working_dir; a child resolves its own relative path against its own workflow file (pinned by test).Verification
1 failed, 3937 passed— the single failure istest_copilot_large_write.py::test_large_create_tool_call_does_not_truncate, a pre-existing environmental Copilot 403 (enterprise-policy) failure reproduced identically on cleanmain.make check(ruff + ty) green (one pre-existing ty warning indialog_evaluator.py).make validate-examplesandconductor validate examples/working-dir.yamlpass.