-
Notifications
You must be signed in to change notification settings - Fork 1.5k
feat(shell) add saved output refs for Reborn shell #4154
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
Changes from all commits
b7d3fa2
49320c2
66499b2
39d9127
87569c5
cc655c2
449f894
5698ea0
953299c
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,15 +1,17 @@ | ||
| use ironclaw_extensions::{CapabilityManifest, ExtensionError}; | ||
| use ironclaw_filesystem::FilesystemError; | ||
| use std::time::Duration; | ||
|
|
||
| use ironclaw_host_api::{ | ||
| EffectKind, PermissionMode, ResourceCeiling, ResourceEstimate, ResourceProfile, | ||
| RuntimeDispatchErrorKind, SandboxQuota, ScopedPath, | ||
| RuntimeDispatchErrorKind, SandboxQuota, ScopedPath, VirtualPath, | ||
| }; | ||
| use serde_json::{Value, json}; | ||
|
|
||
| use crate::{ | ||
| CommandExecutionRequest, FirstPartyCapabilityError, FirstPartyCapabilityRequest, | ||
| RuntimeProcessError, | ||
| RuntimeProcessError, SavedCommandOutput, SavedCommandOutputSanitization, | ||
| process_output::saved_output_filename, | ||
| }; | ||
|
|
||
| use super::{FIRST_PARTY_MAX_OUTPUT_BYTES, first_party_capability_manifest}; | ||
|
|
@@ -22,12 +24,13 @@ pub const SHELL_CAPABILITY_ID: &str = "builtin.shell"; | |
| const DEFAULT_SHELL_WALL_CLOCK_MS: u64 = 120_000; | ||
| const MAX_SHELL_WALL_CLOCK_MS: u64 = 120_000; | ||
| const MAX_SHELL_TIMEOUT_SECS: u64 = MAX_SHELL_WALL_CLOCK_MS / 1000; | ||
| const DEFAULT_SHELL_OUTPUT_BYTES: u64 = crate::process_port::COMMAND_MAX_OUTPUT_SIZE as u64; | ||
| const DEFAULT_SHELL_OUTPUT_BYTES: u64 = crate::process_output::COMMAND_MAX_OUTPUT_SIZE as u64; | ||
| const SAVED_OUTPUT_SCOPED_DIR: &str = "command-outputs"; | ||
|
|
||
| pub(super) fn manifest() -> Result<CapabilityManifest, ExtensionError> { | ||
| first_party_capability_manifest( | ||
| SHELL_CAPABILITY_ID, | ||
| "Execute shell commands with the copied v1 shell validation and output shape", | ||
| "Execute shell commands with copied v1 validation and saved-file references for large local output", | ||
| vec![ | ||
| EffectKind::DispatchCapability, | ||
| EffectKind::SpawnProcess, | ||
|
|
@@ -87,32 +90,138 @@ pub(super) async fn dispatch( | |
| .await | ||
| .map_err(process_error)?; | ||
|
|
||
| let saved_output_path = | ||
| publish_saved_output_for_file_read(request, output.saved_output.as_ref()).await?; | ||
| let rendered_output = render_shell_output( | ||
| &output.output, | ||
| output.saved_output.as_ref(), | ||
| saved_output_path.as_deref(), | ||
| ); | ||
| let output_value = json!({ | ||
| "output": output.output, | ||
| "output": rendered_output, | ||
| "exit_code": output.exit_code, | ||
| "success": output.exit_code == 0, | ||
| "sandboxed": output.sandboxed, | ||
| }); | ||
| Ok((output_value, output.duration)) | ||
| } | ||
|
|
||
| async fn publish_saved_output_for_file_read( | ||
| request: &FirstPartyCapabilityRequest, | ||
| saved_output: Option<&SavedCommandOutput>, | ||
| ) -> Result<Option<String>, FirstPartyCapabilityError> { | ||
| let Some(saved_output) = saved_output else { | ||
| return Ok(None); | ||
| }; | ||
| let Some((scoped_path, virtual_path)) = saved_output_publish_path(request, saved_output) else { | ||
| let _ = tokio::fs::remove_file(&saved_output.path).await; | ||
| return Ok(None); | ||
| }; | ||
| let content = tokio::fs::read(&saved_output.path) | ||
| .await | ||
| .map_err(|_| FirstPartyCapabilityError::new(RuntimeDispatchErrorKind::OperationFailed))?; | ||
| if let Some(parent) = virtual_parent(&virtual_path) { | ||
| match request.services.filesystem.create_dir_all(&parent).await { | ||
| Ok(()) | Err(FilesystemError::Unsupported { .. }) => {} | ||
| Err(_) => { | ||
| return Err(FirstPartyCapabilityError::new( | ||
| RuntimeDispatchErrorKind::OperationFailed, | ||
| )); | ||
| } | ||
| } | ||
| } | ||
| request | ||
| .services | ||
| .filesystem | ||
| .write_file(&virtual_path, &content) | ||
| .await | ||
| .map_err(|_| FirstPartyCapabilityError::new(RuntimeDispatchErrorKind::OperationFailed))?; | ||
| let _ = tokio::fs::remove_file(&saved_output.path).await; | ||
| Ok(Some(scoped_path)) | ||
| } | ||
|
|
||
| fn saved_output_publish_path( | ||
| request: &FirstPartyCapabilityRequest, | ||
| saved_output: &SavedCommandOutput, | ||
| ) -> Option<(String, VirtualPath)> { | ||
| let mounts = request.mounts.as_ref()?; | ||
| let grant = mounts | ||
| .mounts | ||
| .iter() | ||
| .find(|grant| grant.permissions.read && grant.permissions.write)?; | ||
| let filename = saved_output_filename(saved_output); | ||
| let relative_path = format!("{SAVED_OUTPUT_SCOPED_DIR}/{filename}"); | ||
| let scoped_path = format!( | ||
| "{}/{}", | ||
| grant.alias.as_str().trim_end_matches('/'), | ||
| relative_path | ||
| ); | ||
| let virtual_path = mounts | ||
| .scoped_path(scoped_path.clone()) | ||
| .and_then(|path| mounts.resolve(&path)) | ||
| .ok()?; | ||
| Some((scoped_path, virtual_path)) | ||
| } | ||
|
|
||
| fn virtual_parent(path: &VirtualPath) -> Option<VirtualPath> { | ||
| let (parent, _) = path.as_str().rsplit_once('/')?; | ||
| if parent.is_empty() { | ||
| None | ||
| } else { | ||
| VirtualPath::new(parent.to_string()).ok() | ||
| } | ||
| } | ||
|
|
||
| fn render_shell_output( | ||
| output: &str, | ||
| saved_output: Option<&SavedCommandOutput>, | ||
| saved_output_path: Option<&str>, | ||
| ) -> String { | ||
| let Some(saved_output) = saved_output else { | ||
| return output.to_string(); | ||
| }; | ||
| let Some(saved_output_path) = saved_output_path else { | ||
| return format!( | ||
| "{output}\n\nFull output was captured but no file_read-accessible scoped path was available" | ||
| ); | ||
| }; | ||
| let mut note = match saved_output.sanitization { | ||
| SavedCommandOutputSanitization::Blocked => { | ||
| format!( | ||
| "Full output was not saved because it matched secret-leak blocking rules; marker saved to: {saved_output_path}" | ||
| ) | ||
| } | ||
| SavedCommandOutputSanitization::Redacted => { | ||
| format!("Full output saved to: {saved_output_path} (secret-like values redacted)") | ||
| } | ||
| SavedCommandOutputSanitization::Clean => { | ||
| format!("Full output saved to: {saved_output_path}") | ||
| } | ||
| }; | ||
| note.push_str("\nUse file_read to inspect it"); | ||
| if saved_output.stream_was_capped { | ||
| note.push_str(&format!( | ||
| " (saved output capped at {} bytes per stream)", | ||
| saved_output.max_saved_stream_size | ||
| )); | ||
| } | ||
| format!("{output}\n\n{note}") | ||
| } | ||
|
|
||
| fn reject_unbacked_scoped_workdir( | ||
| request: &FirstPartyCapabilityRequest, | ||
| workdir: Option<&str>, | ||
| ) -> Result<(), FirstPartyCapabilityError> { | ||
| let Some(workdir) = workdir else { | ||
| return Ok(()); | ||
| }; | ||
| let Some(mounts) = request | ||
| .mounts | ||
| .as_ref() | ||
| .filter(|mounts| !mounts.mounts.is_empty()) | ||
| else { | ||
| return Ok(()); | ||
| }; | ||
|
|
||
| let Some(workdir) = workdir else { | ||
| return Err(FirstPartyCapabilityError::new( | ||
| RuntimeDispatchErrorKind::Client, | ||
| )); | ||
| }; | ||
| let scoped_path = ScopedPath::new(workdir.to_string()) | ||
| .map_err(|_| FirstPartyCapabilityError::new(RuntimeDispatchErrorKind::InputEncode))?; | ||
| let (_virtual_path, grant) = mounts | ||
|
|
@@ -149,3 +258,73 @@ fn process_error(error: RuntimeProcessError) -> FirstPartyCapabilityError { | |
| }; | ||
| FirstPartyCapabilityError::new(kind) | ||
| } | ||
|
|
||
| #[cfg(test)] | ||
|
Collaborator
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. Medium — No caller-level test for small-output secret-blocking in shell dispatch. CLAUDE.md rule 'Test through the caller, not just the helper': capture_command_output_blocks_secret_like_small_preview tests the helper, but there is no test driving shell dispatch end-to-end with a ProcessPort mock that returns a secret-containing inline output. render_shell_output is never called when saved_output is None, so the blocked marker in the small-output path is never verified to appear in the final JSON response 'output' field. Fix: Add an integration test in first_party_builtin_tools.rs (or shell.rs tests) that mocks ProcessPort to return output containing a secret pattern within inline limits and asserts the shell tool JSON response body contains COMMAND_OUTPUT_BLOCKED_MARKER. |
||
| mod tests { | ||
| use super::*; | ||
|
|
||
| #[test] | ||
| fn render_shell_output_preserves_unsaved_output() { | ||
| assert_eq!(render_shell_output("hello", None, None), "hello"); | ||
| } | ||
|
|
||
| #[test] | ||
| fn render_shell_output_reports_redacted_saved_output() { | ||
| let saved = saved_output(); | ||
|
|
||
| let rendered = render_shell_output( | ||
| "preview", | ||
| Some(&SavedCommandOutput { | ||
| sanitization: SavedCommandOutputSanitization::Redacted, | ||
| ..saved | ||
| }), | ||
| Some("/workspace/command-outputs/command.log"), | ||
| ); | ||
|
|
||
| assert!(rendered.contains("Full output saved to: /workspace/command-outputs/command.log")); | ||
| assert!(!rendered.contains("/tmp/command.log")); | ||
| assert!(rendered.contains("secret-like values redacted")); | ||
| assert!(rendered.contains("use file_read to inspect it")); | ||
| } | ||
|
|
||
| #[test] | ||
| fn render_shell_output_reports_blocked_saved_output() { | ||
| let rendered = render_shell_output( | ||
| "preview", | ||
| Some(&SavedCommandOutput { | ||
| sanitization: SavedCommandOutputSanitization::Blocked, | ||
| ..saved_output() | ||
| }), | ||
| Some("/workspace/command-outputs/command.log"), | ||
| ); | ||
|
|
||
| assert!(rendered.contains("Full output was not saved because")); | ||
| assert!(rendered.contains("marker saved to: /workspace/command-outputs/command.log")); | ||
| assert!(!rendered.contains("/tmp/command.log")); | ||
| } | ||
|
|
||
| #[test] | ||
| fn render_shell_output_reports_stream_cap() { | ||
| let rendered = render_shell_output( | ||
| "preview", | ||
| Some(&SavedCommandOutput { | ||
| stream_was_capped: true, | ||
| max_saved_stream_size: 123, | ||
| ..saved_output() | ||
| }), | ||
| Some("/workspace/command-outputs/command.log"), | ||
| ); | ||
|
|
||
| assert!(rendered.contains("saved output capped at 123 bytes per stream")); | ||
| } | ||
|
|
||
| fn saved_output() -> SavedCommandOutput { | ||
| SavedCommandOutput { | ||
| path: std::path::PathBuf::from("/tmp/command.log"), | ||
| sanitization: SavedCommandOutputSanitization::Clean, | ||
| stream_was_capped: false, | ||
| max_saved_stream_size: 16, | ||
| expires_at_unix_secs: 1, | ||
| } | ||
| } | ||
| } | ||
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.
High — Saved-output file path exposed to model context verbatim.
render_shell_output appends the absolute temp-file path (e.g. /tmp/ironclaw-command-output-.log) to the model-facing output string for all three sanitization states, including Blocked. This leaks the host filesystem temp-dir layout and absolute path into the LLM context. The error-handling rule explicitly forbids exposing absolute paths in user/model-facing output. An adversarial prompt could use this path to reference and exfiltrate the file via subsequent tool calls.
Fix: Either (a) strip the directory component and expose only the UUID filename token, or (b) expose a logical ref ID that maps server-side to the real path and can only be resolved via a dedicated read_saved_output tool. Never embed the raw absolute temp path in model context.