feat(acp): honor AGENT_CWD when resolving NewSessionRequest.cwd - #4558
Open
rsaulo wants to merge 1 commit into
Open
feat(acp): honor AGENT_CWD when resolving NewSessionRequest.cwd#4558rsaulo wants to merge 1 commit into
rsaulo wants to merge 1 commit into
Conversation
PERSONA_PACK_SPEC.md already specifies this resolution order, but the
identifier existed only in that markdown file — `grep -rn AGENT_CWD
--include="*.rs"` returned zero hits, so only step 2 was implemented:
1. The AGENT_CWD environment variable on the buzz-acp process, if set.
2. std::env::current_dir() as a fallback.
3. If both fail, buzz-acp logs an error and refuses to start.
An operator following the spec to point a supervised runtime at a
workspace silently got the process directory instead. Because the agent
runtime resolves AGENTS.md/CLAUDE.md, skills, hooks and settings from the
session cwd, that agent starts without any of the target repository's
instructions and the user has to re-explain in chat what the repo already
documents.
Step 3 replaces the old `unwrap_or_else(|_| PathBuf::from("/"))`. A cwd of
"/" is dropped by workspace_section, so protocol-v1 harnesses lost the
absolute-cwd anchor entirely and scanned from the filesystem root (block#3148).
Refusing to start surfaces that instead of degrading into a full-disk
ripgrep.
An AGENT_CWD that is set but not an existing absolute directory is an
error rather than a fall-through to step 2: falling back would hand the
agent the process directory while the operator believes the workspace is
pinned, which is the silent failure this order exists to prevent. A blank
value still reads as unset, since `AGENT_CWD=${WORKSPACE}` with an unset
WORKSPACE expands to "" in every shell idiom that builds it.
Resolution happens after the setup-mode branch: setup mode never opens a
session, so a workspace typo must not block an operator from fixing the
credentials that put the agent into setup mode.
Scope: resolution stays at the session/new boundary and does not touch the
child process working directory, so the multiplexed pool (one subprocess
holding channel_id -> session_id) is unchanged and this does not add the
per-channel process growth documented in block#2961. Per-channel workspaces
remain the follow-up in block#3822; this lands the env-var form the spec already
promises, which is a strict prerequisite for it.
Testing: 8 unit tests pin the resolution order with injected env/current_dir
so they run in parallel without mutating process state. Verified against the
built binary: relative and non-existent AGENT_CWD are refused with an
actionable message, a valid one proceeds to spawn. cargo test -p buzz-acp
--lib passes 676 tests; fmt and clippy -D warnings are clean.
Signed-off-by: rsaulo <rsaulo@me.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Implements the
$AGENT_CWDresolution order thatcrates/buzz-persona/PERSONA_PACK_SPEC.mdalready specifies. Refs #3822.The gap
The spec, §
$AGENT_CWDDefinition:Before this change,
grep -rn "AGENT_CWD" --include="*.rs"returned zero hits across the repo — the identifier existed only in that markdown file. Only step 2 was implemented. An operator who followed the spec to point a supervised runtime at a workspace got the process directory instead, with no warning.That matters because the session cwd is where the agent runtime resolves
AGENTS.md/CLAUDE.md,.claude/skills, hooks and settings at bootstrap. An agent given the wrong cwd can still read files elsewhere, but it starts without any of the target repository's instructions — the user ends up re-explaining in chat what the repo already documents.What changed
config.rs—resolve_agent_cwd()plus a pure, injectableresolve_agent_cwd_with()and anAgentCwdErrorenum.lib.rs— both call sites that resolved the session cwd now use it: the long-runningPromptContextand themodelssubcommand.README.md— documentsAGENT_CWDin the Core table.Two judgement calls worth reviewing:
A set-but-unusable
AGENT_CWDis an error, not a fall-through to step 2. Falling back would hand the agent the process directory while the operator believes the workspace is pinned — exactly the silent failure this order exists to prevent, and what #3822 and its commenters ask to avoid. A blank value does still read as unset, sinceAGENT_CWD=${WORKSPACE}with an unsetWORKSPACEexpands to""in every shell idiom that builds it.Step 3 replaces
unwrap_or_else(|_| PathBuf::from("/")). A cwd of"/"is dropped byworkspace_section(), so protocol-v1 harnesses lost the absolute-cwd anchor entirely and scanned from the filesystem root — the behavior reported in #3148. Refusing to start surfaces the condition instead of degrading into a full-disk ripgrep. This is the only behavior change for existing deployments, and it only triggers whencurrent_dir()itself fails, which previously produced a broken session anyway.Scope
Resolution stays at the
session/newboundary. The child process working directory is untouched, so the multiplexed pool (one subprocess holdingchannel_id → session_id) is unchanged and this does not introduce the per-channel process growth documented in #2961.Per-channel workspaces remain the open design question in #3822. This lands the env-var form the spec already promises, which is a strict prerequisite for it: the channel-scoped version is then a matter of making the resolved value a per-channel lookup at the same call site, where
PromptSource::Channel(channel_id)is already in scope.Testing
8 unit tests pin the resolution order — env wins, fallback, blank-as-unset, trimming, relative rejected, missing directory rejected, both-unavailable refuses, and a guard that the
"/"fallback cannot come back.current_dirand the directory check are injected, so they run in parallel without mutating process-global state.Verified against the built binary:
cargo test -p buzz-acp --lib→ 676 passed, 0 failed.cargo fmt --checkandcargo clippy --all-targets -- -D warningsclean.Note for reviewers: the existing
framed_system_prompttests already takecwdas a parameter, so no existing test needed changing — contrary to what I guessed when I first sketched the patch surface in #3822.I can follow up with the channel-scoped map in a second PR if maintainers indicate the preferred configuration transport (env map, config file, or session-routing API).