-
Notifications
You must be signed in to change notification settings - Fork 310
Add pre-execution shell command analysis #534
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,5 @@ | ||
| Execute a shell command. Use this for file operations, running scripts, building projects, git commands, running subprocesses, and any system-level operations. Be careful with destructive operations. The command runs with a 60 second timeout by default. | ||
| Execute a shell command. Use this for file operations, running scripts, building projects, git commands, running subprocesses, and any system-level operations. Commands are analyzed before execution, and destructive or suspicious patterns may be rejected pending confirmation. The command runs with a 60 second timeout by default. | ||
|
|
||
| Use the optional `env` parameter to set per-command environment variables (e.g. `[{"key": "RUST_LOG", "value": "debug"}]`). Dangerous variables that enable library injection (LD_PRELOAD, NODE_OPTIONS, etc.) are blocked. | ||
|
|
||
| To install tools that persist across restarts, place binaries in the persistent tools directory at $SPACEBOT_DIR/tools/bin (already on PATH). For example: `curl -fsSL https://example.com/tool -o $SPACEBOT_DIR/tools/bin/tool && chmod +x $SPACEBOT_DIR/tools/bin/tool` | ||
| To install tools that persist across restarts, place binaries in the persistent tools directory at $SPACEBOT_DIR/tools/bin (already on PATH). For example: `curl -fsSL https://example.com/tool -o $SPACEBOT_DIR/tools/bin/tool && chmod +x $SPACEBOT_DIR/tools/bin/tool` |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,10 +1,12 @@ | ||
| //! Shell tool for executing shell commands and subprocesses (task workers only). | ||
| //! | ||
| //! This is the unified execution tool — it replaces the previous `shell` + `exec` | ||
| //! split. Commands run through `sh -c` with optional per-command environment | ||
| //! variables. Dangerous env vars that enable library injection are blocked. | ||
| //! split. Commands are analyzed before execution, then run through `sh -c` with | ||
| //! optional per-command environment variables. Dangerous env vars that enable | ||
| //! library injection are blocked. | ||
|
|
||
| use crate::sandbox::Sandbox; | ||
| use crate::tools::shell_analysis::{CommandAnalysis, ShellAnalyzer}; | ||
| use rig::completion::ToolDefinition; | ||
| use rig::tool::Tool; | ||
| use schemars::JsonSchema; | ||
|
|
@@ -37,12 +39,19 @@ const DANGEROUS_ENV_VARS: &[&str] = &[ | |
| pub struct ShellTool { | ||
| workspace: PathBuf, | ||
| sandbox: Arc<Sandbox>, | ||
| analyzer: ShellAnalyzer, | ||
| } | ||
|
|
||
| impl ShellTool { | ||
| /// Create a new shell tool with sandbox containment. | ||
| pub fn new(workspace: PathBuf, sandbox: Arc<Sandbox>) -> Self { | ||
| Self { workspace, sandbox } | ||
| let analyzer = ShellAnalyzer::new(workspace.clone()); | ||
|
|
||
| Self { | ||
| workspace, | ||
| sandbox, | ||
| analyzer, | ||
| } | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -98,6 +107,8 @@ pub struct ShellOutput { | |
| pub stderr: String, | ||
| /// Formatted summary for LLM consumption. | ||
| pub summary: String, | ||
| /// Pre-execution analysis metadata for UI and worker logic. | ||
| pub analysis: CommandAnalysis, | ||
| } | ||
|
|
||
| impl Tool for ShellTool { | ||
|
|
@@ -227,6 +238,20 @@ impl Tool for ShellTool { | |
| } | ||
| } | ||
|
|
||
| let analysis = self.analyzer.analyze(&args.command, &working_dir); | ||
| if analysis.requires_confirmation { | ||
| return Err(ShellError { | ||
| message: format!( | ||
| "Command requires confirmation: {}", | ||
| analysis | ||
| .confirmation_reason | ||
| .as_deref() | ||
| .unwrap_or("the command was flagged as risky before execution") | ||
| ), | ||
| exit_code: -1, | ||
| }); | ||
| } | ||
|
Comment on lines
+241
to
+253
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Check if there's any confirmation bypass mechanism elsewhere in the codebase
rg -n "requires_confirmation|confirmation_reason" --type=rs -C3Repository: spacedriveapp/spacebot Length of output: 92 🏁 Script executed: #!/bin/bash
# Search for confirmation-related code in Rust files
rg -n "requires_confirmation|confirmation_reason" --glob="*.rs" -C3Repository: spacedriveapp/spacebot Length of output: 5534 🏁 Script executed: #!/bin/bash
# Also check the PR description or comments about the intended behavior
git log --oneline --all -n 20 | head -20Repository: spacedriveapp/spacebot Length of output: 1270 🏁 Script executed: #!/bin/bash
# Check if there's a bypass flag or confirmation approval mechanism
rg -n "bypass|approval|confirm.*true|force" --glob="*.rs" src/tools/shell.rs -C2Repository: spacedriveapp/spacebot Length of output: 592 🏁 Script executed: #!/bin/bash
# Check the function signature and args for the shell tool
rg -n "struct.*Args|pub.*confirm|confirmed.*execution" --glob="*.rs" src/tools/shell.rs -C5Repository: spacedriveapp/spacebot Length of output: 444 🏁 Script executed: #!/bin/bash
# Check if there's a separate execution path or function for confirmed commands
rg -n "async fn.*execute|async fn.*call" --glob="*.rs" src/tools/shell.rs -B2 -A10Repository: spacedriveapp/spacebot Length of output: 718 🏁 Script executed: #!/bin/bash
# Look for the full commit message of the most recent pre-execution analysis commit
git show --format=fuller 0268b9b | head -100Repository: spacedriveapp/spacebot Length of output: 3816 🏁 Script executed: #!/bin/bash
# Check if there are any tests that show confirmation being handled
rg -n "requires_confirmation.*true|blocked.*confirmation" --glob="*.rs" -A10 -B2Repository: spacedriveapp/spacebot Length of output: 48 🏁 Script executed: #!/bin/bash
# Look at more complete context around the error return to see if there's any other path
sed -n '230,260p' src/tools/shell.rsRepository: spacedriveapp/spacebot Length of output: 1243 🏁 Script executed: #!/bin/bash
# Check if there's a bypass or force flag in any of the args or config
rg -n "force|bypass|override.*confirm|skip.*check" --glob="*.rs" src/tools/Repository: spacedriveapp/spacebot Length of output: 1835 Hard-blocked commands have no recovery pathway despite commit message promising "pending confirmation." The PR description states "Risky commands are blocked pending confirmation," but Either add a 🤖 Prompt for AI Agents |
||
|
|
||
| // Build per-command env map for sandbox-aware injection. The sandbox | ||
| // injects these via --setenv (bubblewrap) or .env() (other backends), | ||
| // so they always reach the inner sandboxed process. | ||
|
|
@@ -270,20 +295,26 @@ impl Tool for ShellTool { | |
| let exit_code = output.status.code().unwrap_or(-1); | ||
| let success = output.status.success(); | ||
|
|
||
| let summary = format_shell_output(exit_code, &stdout, &stderr); | ||
| let summary = format_shell_output(exit_code, &stdout, &stderr, analysis.expects_no_output); | ||
|
|
||
| Ok(ShellOutput { | ||
| success, | ||
| exit_code, | ||
| stdout, | ||
| stderr, | ||
| summary, | ||
| analysis, | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| /// Format shell output for display. | ||
| fn format_shell_output(exit_code: i32, stdout: &str, stderr: &str) -> String { | ||
| fn format_shell_output( | ||
| exit_code: i32, | ||
| stdout: &str, | ||
| stderr: &str, | ||
| expects_no_output: bool, | ||
| ) -> String { | ||
| let mut output = String::new(); | ||
|
|
||
| output.push_str(&format!("Exit code: {}\n", exit_code)); | ||
|
|
@@ -299,7 +330,11 @@ fn format_shell_output(exit_code: i32, stdout: &str, stderr: &str) -> String { | |
| } | ||
|
|
||
| if stdout.is_empty() && stderr.is_empty() { | ||
| output.push_str("\n[No output]\n"); | ||
| if exit_code == 0 && expects_no_output { | ||
| output.push_str("\nDone\n"); | ||
| } else { | ||
| output.push_str("\n[No output]\n"); | ||
| } | ||
| } | ||
|
|
||
| output | ||
|
|
@@ -354,6 +389,6 @@ pub struct ShellResult { | |
| impl ShellResult { | ||
| /// Format as a readable string for LLM consumption. | ||
| pub fn format(&self) -> String { | ||
| format_shell_output(self.exit_code, &self.stdout, &self.stderr) | ||
| format_shell_output(self.exit_code, &self.stdout, &self.stderr, false) | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,12 @@ | ||
| //! Pre-execution analysis for shell commands. | ||
|
|
||
| mod analyzer; | ||
| mod categorizer; | ||
| mod parser; | ||
| mod security; | ||
| mod types; | ||
|
|
||
| pub(crate) use analyzer::ShellAnalyzer; | ||
| pub use types::{ | ||
| CommandAnalysis, CommandCategory, DetectedPattern, DurationHint, PatternType, RiskLevel, | ||
| }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Heads-up:
analysis.requires_confirmationcurrently hard-blocks execution, butShellArgsdoesn't include anyconfirm/ackflag. If the intention is “ask user then re-run”, we probably need an explicit arg (default false) or a structured “needs_confirmation” error that carriesanalysisso the caller can display details and retry.