Conversation
Per-worktree .cargo/config.toml isolation (item #133) never covered an ambient CARGO_TARGET_DIR env var, since Cargo's precedence always lets the env var override the config file. Strip CARGO_TARGET_DIR from the child env of every agent launched via run_launch_env/run_headless (agent_launch.rs), and add a CI job that fails the build if the var is ever set project-wide.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe change prevents launched agents from inheriting ChangesCargo target directory isolation
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant AgentCommand
participant run_launch_env
participant ChildProcess
AgentCommand->>run_launch_env: provide launch environment
run_launch_env->>run_launch_env: remove CARGO_TARGET_DIR
run_launch_env->>ChildProcess: start without CARGO_TARGET_DIR
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/ci.yml:
- Around line 133-152: Update the target-dir-guard job’s “Fail if
CARGO_TARGET_DIR is set” step to also evaluate the GitHub Actions configuration
variable vars.CARGO_TARGET_DIR, mapping it into the step environment or
otherwise checking it explicitly. Preserve the existing failure behavior and
diagnostic for either the runner environment or vars.CARGO_TARGET_DIR being
non-empty.
In `@src/agent_launch.rs`:
- Around line 389-431: Protect the process-wide CARGO_TARGET_DIR mutation in
run_launch_env_strips_ambient_cargo_target_dir with the existing global
environment lock. Acquire the lock before set_var, keep it held through the
launch and assertion, and remove_var before releasing it so parallel tests
cannot observe or modify the temporary environment.
- Around line 81-90: The environment override loop in the agent launch command
can reintroduce CARGO_TARGET_DIR after it is removed. Update the command setup
around cmd.env_remove and the env loop so CARGO_TARGET_DIR remains unset after
applying overrides, then add a regression test covering an explicit
CARGO_TARGET_DIR override.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: b0672984-8c69-4527-a6f6-29b833898a7a
📒 Files selected for processing (4)
.github/workflows/ci.ymlAGENTS.mdsrc/agent_launch.rssrc/worktree.rs
Comment on lines
+389
to
+431
| // Item #139: an ambient CARGO_TARGET_DIR must never reach the launched | ||
| // agent — Cargo's env var always outranks the worktree's isolated | ||
| // `.cargo/config.toml` (see `isolate_worktree_target_dir` in | ||
| // worktree.rs), so leaking it here would silently defeat that isolation | ||
| // for every build the agent runs. `env_remove` clones the current env | ||
| // and drops the key at that point, so this holds regardless of what any | ||
| // other test concurrently does to the ambient var. | ||
| #[cfg(unix)] | ||
| #[test] | ||
| fn run_launch_env_strips_ambient_cargo_target_dir() { | ||
| let marker = tempfile::NamedTempFile::new().unwrap(); | ||
| let marker_path = marker.path().to_path_buf(); | ||
| unsafe { | ||
| std::env::set_var("CARGO_TARGET_DIR", "/tmp/shared-target"); | ||
| } | ||
| let reg = vec![AgentSpec { | ||
| id: Agent::Aider, | ||
| display_name: "aider", | ||
| tier: Tier::Cli, | ||
| binary_names: &["sh"], | ||
| version_args: &[], | ||
| package_manager: None, | ||
| package_name: None, | ||
| }]; | ||
| let script = format!("echo -n \"$CARGO_TARGET_DIR\" > {}", marker_path.display()); | ||
| run_launch_env( | ||
| ®, | ||
| "aider", | ||
| None, | ||
| None, | ||
| &["-c".to_string(), script], | ||
| &[], | ||
| false, | ||
| ); | ||
| unsafe { | ||
| std::env::remove_var("CARGO_TARGET_DIR"); | ||
| } | ||
| let content = std::fs::read_to_string(&marker_path).unwrap(); | ||
| assert_eq!( | ||
| content, "", | ||
| "child must not inherit ambient CARGO_TARGET_DIR" | ||
| ); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the relevant region and nearby tests in src/agent_launch.rs
awk 'NR>=330 && NR<=470 { printf "%4d %s\n", NR, $0 }' src/agent_launch.rs
echo
echo "---- env-related occurrences ----"
grep -nE 'set_var|remove_var|env_remove|env::var|CARGO_TARGET_DIR|serial|test.*env' -n src/agent_launch.rs || true
echo
echo "---- repo-wide serialization/env strategy hints ----"
grep -RInE 'serial_test|#[[:space:]]*serial|std::env::set_var|std::env::remove_var|CARGO_TARGET_DIR' . --exclude-dir=.git --exclude-dir=target | head -n 200Repository: getappz/agentflare
Length of output: 13718
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "---- locks / serialization in tests ----"
grep -RInE 'GLOBAL_STATE_LOCK|Mutex<|lazy_static!.*Mutex|once_cell::sync::Lazy.*Mutex|serial_test|#[[:space:]]*serial' src crates --exclude-dir=target || true
echo
echo "---- nearby test helpers around env mutation ----"
awk 'NR>=1 && NR<=220 { printf "%4d %s\n", NR, $0 }' src/paths.rs
echo
echo "---- AGENTS.md section mentioning CARGO_TARGET_DIR ----"
awk 'NR>=78 && NR<=110 { printf "%4d %s\n", NR, $0 }' AGENTS.mdRepository: getappz/agentflare
Length of output: 8101
Serialize this env mutation in the test
This mutates process-wide CARGO_TARGET_DIR without the shared lock used elsewhere, so parallel cargo test runs can race. Guard the set_var/remove_var pair with the existing global env lock, or move the inheritance check into a subprocess.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/agent_launch.rs` around lines 389 - 431, Protect the process-wide
CARGO_TARGET_DIR mutation in run_launch_env_strips_ambient_cargo_target_dir with
the existing global environment lock. Acquire the lock before set_var, keep it
held through the launch and assertion, and remove_var before releasing it so
parallel tests cannot observe or modify the temporary environment.
macOS's /bin/sh doesn't treat -n as a flag (that's a bash builtin behavior), so echo -n printed the literal "-n" into the marker file and failed the new CARGO_TARGET_DIR-stripping regression test on CI's macos-latest runner.
dtolnay/rust-toolchain force-pushes its stable branch on every Rust release rather than tagging, so a SHA pinned to it eventually falls off the branch's history. zizmor's impostor-commit audit was flagging the stale pin (pre-existing on master since 2026-07-07, unrelated to this PR) as a supply-chain risk. Re-pinned all three occurrences (ci.yml clippy/fmt jobs, ppa-publish.yml) to stable's current HEAD.
…reintroducing it, serialize env-mutating test
5 tasks
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
1 participant
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.
Summary
CARGO_TARGET_DIRenv var always outranks the per-worktree.cargo/config.tomlCargo isolation, so the original fix never covered that case.run_launch_env/run_headless(src/agent_launch.rs) now stripCARGO_TARGET_DIRfrom every agent-launched child process (agentflare run,agentflare agents launch), so the per-worktree isolation actually takes effect.target-dir-guardCI job fails the build ifCARGO_TARGET_DIRis ever set project-wide, and is wired into the requiredci-greengate.src/worktree.rsandAGENTS.mdto reflect the fix and the one remaining gap (a bare shell opened inside a worktree, bypassingagentflare run).Test plan
cargo fmt --checkcargo build --workspace --verbosecargo test --workspace --verbose(26/26 relevant tests pass locally on Windows; new Unix-gated regression testrun_launch_env_strips_ambient_cargo_target_dirwill run on CI's ubuntu/macos legs)cargo clippy --locked --workspace --all-targets --all-features -- -D warnings -A unsafe_code -A clippy::pedantic.github/workflows/ci.ymlYAML syntaxSummary by CodeRabbit
Bug Fixes
CARGO_TARGET_DIR, ensuring per-worktree isolated build artifacts are used consistently.Documentation
CARGO_TARGET_DIR, and the remaining bypass case when runningcargodirectly.Tests
CARGO_TARGET_DIRcan’t leak to child processes, including when explicitly overridden.Chores
CARGO_TARGET_DIRis set.