refactor: merge exec tool into shell, add per-command env vars#360
Merged
refactor: merge exec tool into shell, add per-command env vars#360
Conversation
Remove the separate exec tool entirely. The shell tool now supports an optional `env` parameter for setting per-command environment variables (with the same DANGEROUS_ENV_VARS blocklist that exec had). This eliminates tool overlap — shell already handles everything exec did via sh -c, and the env parameter covers the one capability exec had that shell lacked. Changes: - Add EnvVar type and env field to ShellArgs with JSON schema - Add DANGEROUS_ENV_VARS validation to shell tool call() - Remove ExecTool, ExecArgs, ExecOutput, ExecError, ExecResult - Remove exec.rs source, prompt description, and text.rs registry entry - Remove exec from worker/cortex ToolServer registrations - Update all prompts (worker, channel, branch, fragments, shell desc) - Update spawn_worker tool definition (remove exec from tools list) - Update frontend (remove exec renderer, update comments) - Update AGENTS.md module map and tool references - Replace exec tests with shell env parsing tests
Contributor
WalkthroughRemoved the standalone Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/tools/shell.rs`:
- Around line 209-212: The current loop in src/tools/shell.rs that applies
per-command env vars via cmd.env(...) sets them on the outer bwrap process and
therefore doesn't propagate into the bubblewrap sandbox; change the wrap()
function signature to accept per-command environment variables (e.g., a Vec of
EnvVar or the same args.env type) and remove the post-wrap cmd.env(...) usage,
then propagate that new env parameter into the sandbox backends so bubblewrap
injects them using "--setenv" when building the bwrap command; specifically
update wrap_sandbox_exec and wrap_passthrough (and any callers of wrap()) to
pass the per-command env list and, in sandbox.rs where bwrap args are assembled,
add cmd.arg("--setenv").arg(key).arg(value) (or the existing pattern used there)
for each env entry instead of relying on outer process env.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ae1d1d17-439e-4d23-9507-720d54ff73cd
📒 Files selected for processing (17)
AGENTS.mdinterface/src/components/ToolCall.tsxinterface/src/routes/AgentConfig.tsxprompts/en/branch.md.j2prompts/en/channel.md.j2prompts/en/fragments/worker_capabilities.md.j2prompts/en/tools/exec_description.md.j2prompts/en/tools/shell_description.md.j2prompts/en/worker.md.j2src/agent/cortex_chat.rssrc/opencode/worker.rssrc/prompts/text.rssrc/sandbox.rssrc/tools.rssrc/tools/exec.rssrc/tools/shell.rssrc/tools/spawn_worker.rs
💤 Files with no reviewable changes (3)
- prompts/en/tools/exec_description.md.j2
- src/tools/exec.rs
- src/prompts/text.rs
![tembo[bot]](https://avatars.githubusercontent.com/in/1223475?s=60&v=4){kind=small}
| }, | ||
| }, | ||
|
|
||
| set_status: { |
Contributor
There was a problem hiding this comment.
If we persist tool call history (DB logs, old transcripts), dropping the exec renderer makes those older calls render as “unknown”. Might be worth keeping a legacy renderer here even though the tool is removed.
Suggested change
| set_status: { | |
| exec: { | |
| summary(pair) { | |
| const command = pair.args?.command; | |
| if (!command) return null; | |
| if (pair.result && typeof pair.result.exit_code === "number") { | |
| const code = pair.result.exit_code; | |
| const cmdStr = truncate(String(command), 50); | |
| return code === 0 ? cmdStr : `${cmdStr} (exit ${code})`; | |
| } | |
| return truncate(String(command), 60); | |
| }, | |
| resultView(pair) { | |
| if (!pair.resultRaw) return null; | |
| return <ShellResultView pair={pair} />; | |
| }, | |
| }, | |
| set_status: { |
src/tools/shell.rs
Outdated
Comment on lines
+183
to
+212
| @@ -136,6 +206,11 @@ impl Tool for ShellTool { | |||
| .wrap("sh", &["-c", &args.command], &working_dir) | |||
| }; | |||
|
|
|||
| // Apply user-specified env vars after sandbox wrapping | |||
| for env_var in args.env { | |||
| cmd.env(env_var.key, env_var.value); | |||
| } | |||
Contributor
There was a problem hiding this comment.
Now that shell supports caller-provided env, it’d be nice to (a) validate env var names early so we don’t surface a spawn-time error, and (b) scrub inherited DANGEROUS_ENV_VARS too (so a contaminated parent env can’t inject into the sandbox wrapper even if the user didn’t explicitly pass it).
Suggested change
| // Block env vars that enable library injection or alter runtime | |
| // loading behavior — these allow arbitrary code execution regardless | |
| // of filesystem sandbox state. | |
| for env_var in &args.env { | |
| if env_var.key.is_empty() || env_var.key.contains('=') || env_var.key.contains('\0') { | |
| return Err(ShellError { | |
| message: format!("Invalid env var name: {}", env_var.key), | |
| exit_code: -1, | |
| }); | |
| } | |
| if DANGEROUS_ENV_VARS | |
| .iter() | |
| .any(|blocked| env_var.key.eq_ignore_ascii_case(blocked)) | |
| { | |
| return Err(ShellError { | |
| message: format!( | |
| "Cannot set {}: this environment variable enables code injection.", | |
| env_var.key | |
| ), | |
| exit_code: -1, | |
| }); | |
| } | |
| } | |
| let mut cmd = if cfg!(target_os = "windows") { | |
| self.sandbox | |
| .wrap("cmd", &["/C", &args.command], &working_dir) | |
| } else { | |
| self.sandbox | |
| .wrap("sh", &["-c", &args.command], &working_dir) | |
| }; | |
| for blocked in DANGEROUS_ENV_VARS { | |
| cmd.env_remove(*blocked); | |
| } | |
| // Apply user-specified env vars after sandbox wrapping | |
| for env_var in args.env { | |
| cmd.env(env_var.key, env_var.value); | |
| } |
…derer Address CodeRabbit review findings on PR #360: - Fix bubblewrap env propagation: Sandbox::wrap() now accepts per-command env vars and injects them via --setenv (bwrap) or .env() (sandbox-exec/ passthrough), so env vars always reach the inner sandboxed process. - Add env var name validation: reject empty names, names containing '=', and names/values containing null bytes. - Add defense-in-depth DANGEROUS_ENV_VARS check at the sandbox layer, so dangerous env vars are dropped even if tool-level validation is bypassed by a future caller. - Re-add legacy 'exec' renderer in ToolCall.tsx for old transcript rendering (maps exec's program+args to shell-style display). - Add 5 new tests for env validation and dangerous env blocking.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/sandbox.rs (1)
88-104: Defense-in-depth constant duplicated from shell.rs.This constant is intentionally duplicated from
src/tools/shell.rsto provide defense-in-depth at the sandbox layer. The duplication is acceptable here since:
- The sandbox layer must not depend on tool-layer definitions
- It ensures protection even if tool-level validation is bypassed
- Both lists are identical and documented as serving different purposes (tool returns error vs sandbox silently drops)
Consider adding a test that verifies both lists remain in sync to catch accidental divergence.
🧪 Optional: Add a sync test in src/sandbox.rs tests
#[cfg(test)] mod tests { use super::*; #[test] fn dangerous_env_vars_matches_shell_tool() { // Ensure defense-in-depth lists stay in sync let shell_list = crate::tools::shell::DANGEROUS_ENV_VARS; // requires pub assert_eq!(DANGEROUS_ENV_VARS, shell_list); } }Note: This would require making the shell constant
pub(crate).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/sandbox.rs` around lines 88 - 104, Add a unit test to ensure the sandbox DANGEROUS_ENV_VARS stays identical to the tool-level list and make the shell constant accessible; specifically, expose the tool-level constant by changing the visibility of crate::tools::shell::DANGEROUS_ENV_VARS to pub(crate) and add a #[cfg(test)] mod tests in src/sandbox.rs that imports crate::tools::shell::DANGEROUS_ENV_VARS and asserts equality with the sandbox DANGEROUS_ENV_VARS (e.g., assert_eq!(DANGEROUS_ENV_VARS, crate::tools::shell::DANGEROUS_ENV_VARS)); this will catch accidental divergence while preserving the documented defense-in-depth behavior.src/tools/shell.rs (1)
57-65: Consider derivingCloneforEnvVar.The struct is consumed via
into_iter()on line 235, which works, but derivingClonewould make the type more flexible if needed elsewhere (e.g., logging, retry logic).♻️ Optional: Add Clone derive
/// A key-value environment variable pair. -#[derive(Debug, Deserialize, JsonSchema)] +#[derive(Debug, Clone, Deserialize, JsonSchema)] pub struct EnvVar { /// The variable name. pub key: String, /// The variable value. pub value: String, }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tools/shell.rs` around lines 57 - 65, The EnvVar struct lacks Clone which reduces flexibility; update the struct declaration to derive Clone alongside Debug, Deserialize, and JsonSchema (i.e., add Clone to the derive list for struct EnvVar) so instances can be cheaply cloned when needed (e.g., for logging or retry paths) while keeping existing into_iter() usage unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/sandbox.rs`:
- Around line 88-104: Add a unit test to ensure the sandbox DANGEROUS_ENV_VARS
stays identical to the tool-level list and make the shell constant accessible;
specifically, expose the tool-level constant by changing the visibility of
crate::tools::shell::DANGEROUS_ENV_VARS to pub(crate) and add a #[cfg(test)] mod
tests in src/sandbox.rs that imports crate::tools::shell::DANGEROUS_ENV_VARS and
asserts equality with the sandbox DANGEROUS_ENV_VARS (e.g.,
assert_eq!(DANGEROUS_ENV_VARS, crate::tools::shell::DANGEROUS_ENV_VARS)); this
will catch accidental divergence while preserving the documented
defense-in-depth behavior.
In `@src/tools/shell.rs`:
- Around line 57-65: The EnvVar struct lacks Clone which reduces flexibility;
update the struct declaration to derive Clone alongside Debug, Deserialize, and
JsonSchema (i.e., add Clone to the derive list for struct EnvVar) so instances
can be cheaply cloned when needed (e.g., for logging or retry paths) while
keeping existing into_iter() usage unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 64c7b4a9-a986-45bb-b7a1-8b3996dbf756
📒 Files selected for processing (4)
interface/src/components/ToolCall.tsxsrc/sandbox.rssrc/tools.rssrc/tools/shell.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- interface/src/components/ToolCall.tsx
Shell tool runs without stdin, so interactive prompts from npm/npx/apt-get always hang until the 60s timeout kills the process. Setting CI=true prevents most Node.js tooling prompts (npm, npx, yarn) and DEBIAN_FRONTEND=noninteractive prevents apt-get prompts. Both vars are added to RESERVED_ENV_VARS so they cannot be overridden by passthrough_env or per-command env vars.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/sandbox.rs (1)
602-614: Consider extracting filtered env vars to reduce duplication (optional).The per-command env filtering logic is repeated across all three backends. You could extract a helper that filters the HashMap and logs appropriately:
fn filter_command_env<'a>( command_env: &'a HashMap<String, String>, ) -> impl Iterator<Item = (&'a String, &'a String)> { command_env.iter().filter(|(name, _)| { if is_reserved_env_var(name) { tracing::debug!(%name, "skipping reserved per-command env var"); return false; } if is_dangerous_env_var(name) { tracing::warn!(%name, "dropping dangerous per-command env var"); return false; } true }) }Then each backend could simply iterate:
for (name, value) in filter_command_env(command_env).This is a minor DRY improvement—the current approach is acceptable.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/sandbox.rs` around lines 602 - 614, The per-command env var filtering in sandbox.rs is duplicated across backends; extract a helper named filter_command_env that takes &HashMap<String, String> and returns an iterator over (&String, &String) applying is_reserved_env_var and is_dangerous_env_var with the same tracing::debug!/tracing::warn! logging, then replace each duplicate loop with `for (name, value) in filter_command_env(command_env)` so all three backends reuse the single filtering function.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/sandbox.rs`:
- Around line 602-614: The per-command env var filtering in sandbox.rs is
duplicated across backends; extract a helper named filter_command_env that takes
&HashMap<String, String> and returns an iterator over (&String, &String)
applying is_reserved_env_var and is_dangerous_env_var with the same
tracing::debug!/tracing::warn! logging, then replace each duplicate loop with
`for (name, value) in filter_command_env(command_env)` so all three backends
reuse the single filtering function.
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
exectool entirely — theshelltool now supports an optionalenvparameter for per-command environment variablessh -c, theenvparameter covers the one unique exec capabilityDANGEROUS_ENV_VARSblocklist from exec (LD_PRELOAD, NODE_OPTIONS, etc.)Changes
EnvVartype andenvfield toShellArgswith JSON schemaDANGEROUS_ENV_VARSvalidation to shellcall()src/tools/exec.rs, prompt description, text.rs registry entryExecToolfrom worker and cortex chatToolServerregistrationsspawn_workertool definition (drop exec from tools list)17 files changed, -268 net lines.
Note
Summary for commit 94b2ef6:
This PR consolidates two overlapping execution tools into one. The shell tool now handles both shell commands and subprocess execution, eliminating the need for a separate exec tool. The key addition is the
envparameter on shell commands, which allows setting per-command environment variables while maintaining the dangerous environment variable blocklist (LD_PRELOAD, NODE_OPTIONS, etc.) that prevents code injection attacks. Frontend and documentation updates ensure the tool consolidation is reflected throughout the system. Net change removes 268 lines of duplicate code while maintaining security guardrails.Written by Tembo for commit 94b2ef6. This will update automatically on new commits.