From b4fd7ae00b048395bdbd8a310c5da417b5c3e567 Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Tue, 25 Aug 2026 07:14:31 -0400 Subject: [PATCH] fix(sandbox): materialize prompt blobs in runtime storage, not the checkout Remote prompt-value materialization wrote demoted values to {working_directory}/.fabro/blobs inside the repository checkout, so a later checkpoint could commit them and leak them into the run pull request. Give each sandbox a run-scoped runtime directory outside the source checkout as part of the Sandbox contract: - Sandbox::runtime_directory() names the directory; host-local sandboxes return None because the engine owns a host-side runtime directory (RunScratch) for those runs. - Docker creates /fabro/runtime at initialize with umask 077 and uploads runtime files with mode 0600. - Daytona creates /home/daytona/fabro/runtime with mode 0700. - Both remote materialization paths in fabro-workflow share one materialization-path helper built on the new contract. The paths keep the runtime/blobs suffix, so durable context still normalizes to blob://sha256/... references. - Local materialization now writes owner-private directories and files on Unix. Regression coverage: an integration test runs remote-style prompt demotion against a real git checkout, then a real checkpoint commit, and asserts the checkout stays clean, the agent-facing file is readable, and a deleted materialized file is recreated from the durable blob store. A real-Docker test verifies the runtime directory and blob file permissions inside a container. Fixes #798 Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_012Kmn5jyrdpyCdvcfvmEDvA --- docs/public/agents/outputs.mdx | 2 +- docs/public/execution/context.mdx | 2 +- .../fabro-sandbox/src/daytona/mod.rs | 47 ++++ lib/components/fabro-sandbox/src/docker.rs | 66 ++++- lib/components/fabro-sandbox/src/sandbox.rs | 18 ++ .../fabro-sandbox/tests/docker_streaming.rs | 75 ++++++ lib/components/fabro-workflow/src/artifact.rs | 162 ++++++++++-- .../src/handler/llm/preamble.rs | 12 +- .../tests/it/git_integration.rs | 232 ++++++++++++++++++ .../fabro-workflow/tests/it/integration.rs | 14 +- 10 files changed, 595 insertions(+), 35 deletions(-) diff --git a/docs/public/agents/outputs.mdx b/docs/public/agents/outputs.mdx index 47bd1770e..81cfdeb16 100644 --- a/docs/public/agents/outputs.mdx +++ b/docs/public/agents/outputs.mdx @@ -237,7 +237,7 @@ Captured stage artifacts such as screenshots, videos, reports, and traces still For remote sandboxes (Docker, Daytona), execution-time file access happens inside the sandbox filesystem. -- Blob refs are materialized into `{working_directory}/.fabro/blobs/{blob_hash}.json` +- Blob refs are materialized into the sandbox runtime directory, `{runtime_directory}/blobs/{blob_hash}.json`. This directory lives outside the repository checkout, so materialized blobs never show up in `git status` or in checkpoint commits. - Explicit non-blob `file://` refs keep the existing copy-on-demand behavior and are copied into `{working_directory}/.fabro/artifacts/{filename}` when needed In both cases, downstream handlers and agents continue to consume ordinary `file://` pointers during execution. diff --git a/docs/public/execution/context.mdx b/docs/public/execution/context.mdx index bc7e19d15..ead66336d 100644 --- a/docs/public/execution/context.mdx +++ b/docs/public/execution/context.mdx @@ -250,7 +250,7 @@ Checkpoints and checkpoint-completed events persist these `blob://` refs, not ho Before Fabro builds a preamble or starts the next stage, it resolves any blob refs into execution-local files so handlers and agents still see normal `file://` references: - Local execution materializes blobs under `{run_dir}/runtime/blobs/{blob_hash}.json` -- Remote sandboxes materialize blobs under `{working_directory}/.fabro/blobs/{blob_hash}.json` +- Remote sandboxes materialize blobs under the sandbox runtime directory, `{runtime_directory}/blobs/{blob_hash}.json`. This directory lives outside the repository checkout, so materialized blobs never appear in `git status` and are never committed by a checkpoint. These materialized `file://` paths are runtime-only. They are not written back into durable context snapshots. diff --git a/lib/components/fabro-sandbox/src/daytona/mod.rs b/lib/components/fabro-sandbox/src/daytona/mod.rs index a7a0a8d12..8e2eaf7af 100644 --- a/lib/components/fabro-sandbox/src/daytona/mod.rs +++ b/lib/components/fabro-sandbox/src/daytona/mod.rs @@ -61,6 +61,7 @@ const DAYTONA_BASH_SESSION_REMEDIATION: &str = "Daytona ran the direct command t pub(crate) const WORKING_DIRECTORY: &str = "/home/daytona/workspace"; pub(crate) const REPOS_ROOT: &str = "/home/daytona/repos"; +pub(crate) const RUNTIME_DIRECTORY: &str = "/home/daytona/fabro/runtime"; const DEFAULT_SNAPSHOT: &str = "daytona-medium"; pub const DEFAULT_DAYTONA_API_URL: &str = "https://app.daytona.io/api"; pub(crate) const DAYTONA_DASHBOARD_SANDBOXES_URL: &str = @@ -786,6 +787,41 @@ impl DaytonaSandbox { Self::probe_bash_session(sandbox).await } + /// Create the run-scoped Fabro runtime directory outside the repository + /// checkout, with owner-private permissions on each created level. + async fn create_runtime_directory(sandbox: &daytona_sdk::Sandbox) -> crate::Result<()> { + let fs_svc = sandbox + .fs() + .await + .map_err(|e| crate::Error::context("Failed to get Daytona fs service", e))?; + let runtime_parent = Path::new(RUNTIME_DIRECTORY) + .parent() + .map(|parent| parent.to_string_lossy().to_string()); + if let Some(runtime_parent) = runtime_parent { + fs_svc + .create_folder(&runtime_parent, Some("0700")) + .await + .map_err(|e| { + wrap_fs_error( + "Failed to create Daytona runtime parent directory", + &runtime_parent, + e, + ) + })?; + } + fs_svc + .create_folder(RUNTIME_DIRECTORY, Some("0700")) + .await + .map_err(|e| { + wrap_fs_error( + "Failed to create Daytona runtime directory", + RUNTIME_DIRECTORY, + e, + ) + })?; + Ok(()) + } + /// Probe Bash over the direct process-exec transport. async fn probe_bash_exec(sandbox: &daytona_sdk::Sandbox) -> crate::Result<()> { let start = Instant::now(); @@ -1470,6 +1506,13 @@ impl Sandbox for DaytonaSandbox { return Err(self.fail_init(init_start, err)); } + if let Err(runtime_error) = Self::create_runtime_directory(&sandbox).await { + let err = self + .finish_failed_initialization(sandbox, runtime_error) + .await; + return Err(self.fail_init(init_start, err)); + } + let clone_decision = clone_source::decide_clone( self.config.skip_clone, self.clone_origin_url.as_deref(), @@ -1872,6 +1915,10 @@ impl Sandbox for DaytonaSandbox { .map_or(WORKING_DIRECTORY, String::as_str) } + fn runtime_directory(&self) -> Option<&str> { + Some(RUNTIME_DIRECTORY) + } + fn platform(&self) -> &'static str { "linux" } diff --git a/lib/components/fabro-sandbox/src/docker.rs b/lib/components/fabro-sandbox/src/docker.rs index 0b08b97b4..65f36a086 100644 --- a/lib/components/fabro-sandbox/src/docker.rs +++ b/lib/components/fabro-sandbox/src/docker.rs @@ -49,6 +49,7 @@ const DOCKER_BASH_REQUIREMENT: &str = "Docker sandboxes require /bin/bash for ev pub(crate) const WORKING_DIRECTORY: &str = "/workspace"; pub(crate) const REPOS_ROOT: &str = "/repos"; +pub(crate) const RUNTIME_DIRECTORY: &str = "/fabro/runtime"; const DEFAULT_GIT_CLONE_DEPTH: usize = RunCloneSettings::DEFAULT_DEPTH.unsigned_abs() as usize; const GIT_CLONE_TIMEOUT: Duration = Duration::from_mins(5); #[cfg(test)] @@ -717,6 +718,28 @@ impl DockerSandbox { Ok(()) } + /// Create the run-scoped Fabro runtime directory outside the repository + /// checkout. The umask keeps every created level owner-private. + async fn create_runtime_directory(&self) -> crate::Result<()> { + let result = self + .docker_exec_shell( + &format!("umask 077 && mkdir -p {}", shell_quote(RUNTIME_DIRECTORY)), + 10_000, + Some("/"), + None, + None, + ) + .await?; + if !result.is_success() { + return Err(crate::Error::message(format!( + "Failed to create Docker runtime directory (exit {}): {}", + result.display_exit_code(), + result.stderr + ))); + } + Ok(()) + } + /// Verify the container evaluates commands as non-login Bash. /// /// Shared by fresh initialization and by `start` after a reconnect, so a @@ -1144,14 +1167,16 @@ impl DockerSandbox { .to_string_lossy() .to_string(); + // Fabro runtime files stay owner-private; repository files keep the + // conventional world-readable mode. + let is_runtime_path = container_path.starts_with(&format!("{RUNTIME_DIRECTORY}/")); + let mkdir_cmd = if is_runtime_path { + format!("umask 077 && mkdir -p {}", shell_quote(&parent_dir)) + } else { + format!("mkdir -p {}", shell_quote(&parent_dir)) + }; let result = self - .docker_exec_shell( - &format!("mkdir -p {}", shell_quote(&parent_dir)), - 10_000, - Some("/"), - None, - None, - ) + .docker_exec_shell(&mkdir_cmd, 10_000, Some("/"), None, None) .await?; if !result.is_success() { return Err(crate::Error::message(format!( @@ -1160,7 +1185,8 @@ impl DockerSandbox { ))); } - let tar_bytes = build_single_file_tar(&file_name, bytes)?; + let file_mode = if is_runtime_path { 0o600 } else { 0o644 }; + let tar_bytes = build_single_file_tar(&file_name, bytes, file_mode)?; let upload_opts = UploadToContainerOptions { path: parent_dir, no_overwrite_dir_non_dir: "false".to_string(), @@ -1660,7 +1686,7 @@ fn bash_remediation(image: &str) -> String { format!("Failed to start Docker container from image '{image}'. {DOCKER_BASH_REQUIREMENT}") } -fn build_single_file_tar(file_name: &str, bytes: &[u8]) -> crate::Result> { +fn build_single_file_tar(file_name: &str, bytes: &[u8], mode: u32) -> crate::Result> { let mut tar_builder = tar::Builder::new(Vec::new()); let mut header = tar::Header::new_gnu(); header @@ -1670,7 +1696,7 @@ fn build_single_file_tar(file_name: &str, bytes: &[u8]) -> crate::Result u64::try_from(bytes.len()) .map_err(|_| crate::Error::message("file is too large for tar header"))?, ); - header.set_mode(0o644); + header.set_mode(mode); header.set_cksum(); tar_builder .append(&header, bytes) @@ -1788,6 +1814,10 @@ impl Sandbox for DockerSandbox { .cached_os_version .set(format!("linux {}", uname_output.trim())); + if let Err(e) = self.create_runtime_directory().await { + return Err(self.fail_init(init_start, e)); + } + let clone_decision = clone_source::decide_clone( self.config.skip_clone, self.clone_origin_url.as_deref(), @@ -2313,6 +2343,10 @@ impl Sandbox for DockerSandbox { .map_or(WORKING_DIRECTORY, String::as_str) } + fn runtime_directory(&self) -> Option<&str> { + Some(RUNTIME_DIRECTORY) + } + async fn ssh_access_command(&self) -> crate::Result> { Ok(Some(docker_access_command( self.container_id()?, @@ -3005,16 +3039,26 @@ mod tests { #[test] fn single_file_tar_contains_named_file() { - let bytes = build_single_file_tar("nested.txt", b"hello").unwrap(); + let bytes = build_single_file_tar("nested.txt", b"hello", 0o644).unwrap(); let mut archive = tar::Archive::new(Cursor::new(bytes)); let mut entries = archive.entries().unwrap(); let mut entry = entries.next().unwrap().unwrap(); assert_eq!(entry.path().unwrap().to_string_lossy(), "nested.txt"); + assert_eq!(entry.header().mode().unwrap(), 0o644); let mut content = String::new(); entry.read_to_string(&mut content).unwrap(); assert_eq!(content, "hello"); } + #[test] + fn single_file_tar_applies_private_mode() { + let bytes = build_single_file_tar("blob.json", b"{}", 0o600).unwrap(); + let mut archive = tar::Archive::new(Cursor::new(bytes)); + let mut entries = archive.entries().unwrap(); + let entry = entries.next().unwrap().unwrap(); + assert_eq!(entry.header().mode().unwrap(), 0o600); + } + fn test_docker_sandbox(docker: Docker, container_id: &str) -> DockerSandbox { let sandbox = DockerSandbox::with_docker_client( docker, diff --git a/lib/components/fabro-sandbox/src/sandbox.rs b/lib/components/fabro-sandbox/src/sandbox.rs index a08094df6..3cb2cb221 100644 --- a/lib/components/fabro-sandbox/src/sandbox.rs +++ b/lib/components/fabro-sandbox/src/sandbox.rs @@ -1441,6 +1441,24 @@ pub trait Sandbox: Send + Sync { } async fn cleanup(&self) -> crate::Result<()>; fn working_directory(&self) -> &str; + /// Run-scoped directory for Fabro-owned runtime files inside the sandbox, + /// or `None` when the sandbox has no such directory. + /// + /// The directory sits outside every repository checkout, so runtime files + /// Fabro materializes beneath it — for example oversized prompt values + /// projected out of the durable blob store — never appear in `git status` + /// and can never be committed by a checkpoint. Its contents are + /// disposable: everything beneath it can be recreated from durable + /// storage on demand. + /// + /// Providers that provision an isolated per-run environment (Docker, + /// Daytona) create the directory during initialization with private + /// permissions and return its path. Sandboxes that execute directly on + /// the worker host return `None`; the workflow engine owns a host-side + /// runtime directory for those runs. + fn runtime_directory(&self) -> Option<&str> { + None + } fn platform(&self) -> &str; fn os_version(&self) -> String; /// Return a human-readable identifier for the sandbox (e.g. container ID, diff --git a/lib/components/fabro-sandbox/tests/docker_streaming.rs b/lib/components/fabro-sandbox/tests/docker_streaming.rs index c04ccf1a0..1dfca5555 100644 --- a/lib/components/fabro-sandbox/tests/docker_streaming.rs +++ b/lib/components/fabro-sandbox/tests/docker_streaming.rs @@ -395,3 +395,78 @@ async fn docker_glob_matches_patterns_containing_a_path_separator() { "`**/SKILL.md` should match files nested several levels deep, got: {recursive:?}" ); } + +// The Fabro runtime directory is where prompt blobs materialize, so it must +// exist after initialization, sit outside the repository checkout, and stay +// owner-private along with the files written beneath it (issue #798). +#[tokio::test] +#[ignore = "requires real Docker container lifecycle; run explicitly when changing Docker runtime directory setup"] +async fn docker_runtime_directory_is_private_and_outside_workspace() { + let image = "buildpack-deps:noble"; + let Ok(docker) = Docker::connect_with_local_defaults() else { + return; + }; + if docker.inspect_image(image).await.is_err() { + return; + } + + let sandbox = DockerSandbox::new( + DockerSandboxOptions { + image: image.to_string(), + auto_pull: false, + skip_clone: true, + ..DockerSandboxOptions::default() + }, + None, + None, + None, + None, + None, + ) + .expect("docker sandbox should construct"); + sandbox + .initialize() + .await + .expect("docker sandbox should initialize"); + + let runtime_directory = sandbox + .runtime_directory() + .expect("docker sandbox should expose a runtime directory") + .to_string(); + assert!( + !runtime_directory.starts_with(sandbox.working_directory()), + "runtime directory {runtime_directory} must sit outside the workspace" + ); + + let blob_path = format!("{runtime_directory}/blobs/test-blob.json"); + sandbox + .write_file(&blob_path, "{}") + .await + .expect("runtime blob write should succeed"); + + let modes = sandbox + .exec_command( + &format!("stat -c '%a' {runtime_directory} {blob_path}"), + 10_000, + None, + None, + None, + ) + .await + .expect("stat should run"); + let readback = sandbox.read_file_text(&blob_path).await; + + sandbox + .cleanup() + .await + .expect("docker cleanup should succeed"); + + assert!(modes.is_success(), "stat failed: {}", modes.stderr); + let modes: Vec<&str> = modes.stdout.split_whitespace().collect(); + assert_eq!( + modes, + ["700", "600"], + "runtime directory and blob file should be owner-private" + ); + assert_eq!(readback.expect("runtime blob should be readable"), "{}"); +} diff --git a/lib/components/fabro-workflow/src/artifact.rs b/lib/components/fabro-workflow/src/artifact.rs index 3fb27588f..42e167930 100644 --- a/lib/components/fabro-workflow/src/artifact.rs +++ b/lib/components/fabro-workflow/src/artifact.rs @@ -9,6 +9,7 @@ use fabro_types::{ use futures::future::BoxFuture; use serde_json::Value; use tokio::fs; +use tokio::io::AsyncWriteExt; use crate::context::{self, Context}; use crate::error::{Error, Result}; @@ -295,22 +296,40 @@ async fn materialize_value_bytes( return Ok(path.display().to_string()); } - let remote_path = format!("{}/.fabro/blobs/{blob_hash}.json", env.working_directory()); + let remote_path = remote_materialized_blob_path(env, &blob_hash)?; if !env .file_exists(&remote_path) .await .map_err(|e| Error::engine_with_source("failed to check blob existence", e))? { persist_blob(bytes, run_store).await?; - let content = std::str::from_utf8(bytes) - .map_err(|e| Error::engine_with_source("artifact blob was not valid UTF-8 JSON", e))?; - env.write_file(&remote_path, content).await.map_err(|e| { - Error::engine_with_source("failed to write artifact blob to sandbox", e) - })?; + write_remote_blob_file(env, &remote_path, bytes).await?; } Ok(remote_path) } +/// The sandbox file that materializes one blob for agent reads. +/// +/// The file lives beneath the sandbox's run-scoped runtime directory, never +/// the repository checkout, so materialization cannot dirty `git status` and +/// a later checkpoint can never commit it. The `runtime/blobs` suffix keeps +/// the path recognizable as a managed blob reference, so durable storage +/// still records `blob://sha256/...` instead of this execution-local path. +fn remote_materialized_blob_path(env: &dyn Sandbox, blob_hash: &BlobHash) -> Result { + let runtime_directory = env.runtime_directory().ok_or_else(|| { + Error::engine("sandbox exposes no runtime directory for blob materialization") + })?; + Ok(format!("{runtime_directory}/blobs/{blob_hash}.json")) +} + +async fn write_remote_blob_file(env: &dyn Sandbox, path: &str, bytes: &[u8]) -> Result<()> { + let content = std::str::from_utf8(bytes) + .map_err(|e| Error::engine_with_source("artifact blob was not valid UTF-8 JSON", e))?; + env.write_file(path, content) + .await + .map_err(|e| Error::engine_with_source("failed to write artifact blob to sandbox", e)) +} + async fn persist_blob(bytes: &[u8], run_store: &RunStoreHandle) -> Result<()> { run_store .write_blob(bytes) @@ -715,33 +734,43 @@ async fn materialize_blob_ref( return Ok(format!("{ARTIFACT_POINTER_PREFIX}{}", path.display())); } - let remote_path = format!("{}/.fabro/blobs/{blob_hash}.json", env.working_directory()); + let remote_path = remote_materialized_blob_path(env, blob_hash)?; if !env .file_exists(&remote_path) .await .map_err(|e| Error::engine_with_source("failed to check blob existence", e))? { let bytes = read_required_blob(blob_hash, run_store).await?; - let content = String::from_utf8(bytes.to_vec()) - .map_err(|e| Error::engine_with_source("artifact blob was not valid UTF-8 JSON", e))?; - env.write_file(&remote_path, &content).await.map_err(|e| { - Error::engine_with_source("failed to write artifact blob to sandbox", e) - })?; + write_remote_blob_file(env, &remote_path, &bytes).await?; } Ok(format!("{ARTIFACT_POINTER_PREFIX}{remote_path}")) } +/// Write a materialized blob file, keeping created directories and the file +/// itself owner-private where the platform supports modes. async fn write_local_blob_file(path: &Path, bytes: &[u8]) -> Result<()> { if let Some(parent) = path.parent() { - fs::create_dir_all(parent).await.map_err(|err| { + let mut builder = fs::DirBuilder::new(); + builder.recursive(true); + #[cfg(unix)] + builder.mode(0o700); + builder.create(parent).await.map_err(|err| { Error::Io(format!( "creating artifact blob directory {}: {err}", parent.display() )) })?; } - fs::write(path, bytes) + let mut options = fs::OpenOptions::new(); + options.write(true).create(true).truncate(true); + #[cfg(unix)] + options.mode(0o600); + let mut file = options + .open(path) + .await + .map_err(|err| Error::Io(format!("writing artifact blob {}: {err}", path.display())))?; + file.write_all(bytes) .await .map_err(|err| Error::Io(format!("writing artifact blob {}: {err}", path.display()))) } @@ -1269,6 +1298,7 @@ mod tests { accessible: bool, written: Mutex>, working_dir: String, + runtime_dir: Option, exists_calls: Mutex, } @@ -1278,9 +1308,15 @@ mod tests { accessible, written: Mutex::new(Vec::new()), working_dir: working_dir.to_string(), + runtime_dir: None, exists_calls: Mutex::new(0), } } + + fn with_runtime_directory(mut self, runtime_dir: &str) -> Self { + self.runtime_dir = Some(runtime_dir.to_string()); + self + } } #[async_trait::async_trait] @@ -1370,6 +1406,10 @@ mod tests { &self.working_dir } + fn runtime_directory(&self) -> Option<&str> { + self.runtime_dir.as_deref() + } + fn platform(&self) -> &str { "linux" } @@ -1499,6 +1539,100 @@ mod tests { assert!(details["preview"].as_str().unwrap().starts_with("ooo")); } + #[tokio::test] + async fn demote_materializes_remote_values_under_sandbox_runtime_directory() { + let run_store: RunStoreHandle = make_run_store("prompt-demote-remote").await.into(); + let run_dir = tempfile::tempdir().unwrap(); + let env = TestSyncEnv::new(false, "/workspace").with_runtime_directory("/fabro/runtime"); + + let oversized = serde_json::json!("x".repeat(PROMPT_INLINE_VALUE_MAX + 1)); + let expected_bytes = serde_json::to_vec(&oversized).unwrap(); + let expected_path = format!( + "/fabro/runtime/blobs/{}.json", + BlobHash::new(&expected_bytes) + ); + let mut values = HashMap::from([("dataset".to_string(), oversized)]); + + demote_large_values_for_prompt( + &mut values, + &mut HashMap::new(), + &run_store, + &env, + run_dir.path(), + ) + .await; + + let details = prompt_large_value(&values["dataset"]) + .expect("oversized remote context value should demote"); + assert_eq!(details.path, expected_path); + let written = env.written.lock().unwrap(); + assert_eq!(written.len(), 1); + assert_eq!(written[0].0, expected_path); + assert_eq!(written[0].1.as_bytes(), expected_bytes); + assert!( + !written[0].0.starts_with("/workspace"), + "materialization must stay outside the repository checkout" + ); + } + + #[tokio::test] + async fn demote_keeps_value_inline_when_sandbox_has_no_runtime_directory() { + let run_store: RunStoreHandle = make_run_store("prompt-demote-no-runtime").await.into(); + let run_dir = tempfile::tempdir().unwrap(); + let env = TestSyncEnv::new(false, "/workspace"); + + let oversized = serde_json::json!("x".repeat(PROMPT_INLINE_VALUE_MAX + 1)); + let mut values = HashMap::from([("dataset".to_string(), oversized.clone())]); + + demote_large_values_for_prompt( + &mut values, + &mut HashMap::new(), + &run_store, + &env, + run_dir.path(), + ) + .await; + + assert_eq!(values["dataset"], oversized); + assert!(env.written.lock().unwrap().is_empty()); + } + + #[tokio::test] + async fn resolve_context_materializes_remote_blob_refs_under_runtime_directory() { + let run_store = make_run_store("remote-blob-ref-resolution").await; + let report = serde_json::json!({"kind": "report"}); + let report_bytes = serde_json::to_vec(&report).unwrap(); + let blob_hash = run_store.write_blob(&report_bytes).await.unwrap(); + let context = Context::new(); + context.set("report", fabro_types::format_blob_ref(&blob_hash).into()); + let env = TestSyncEnv::new(false, "/workspace").with_runtime_directory("/fabro/runtime"); + let run_dir = tempfile::tempdir().unwrap(); + + let resolved = + resolved_context_snapshot(&context, &run_store.clone().into(), &env, run_dir.path()) + .await + .unwrap(); + + let expected_path = format!("/fabro/runtime/blobs/{blob_hash}.json"); + assert_eq!( + resolved["report"], + serde_json::json!(format!("file://{expected_path}")) + ); + let written = env.written.lock().unwrap(); + assert_eq!(written.len(), 1); + assert_eq!(written[0].0, expected_path); + assert_eq!(written[0].1.as_bytes(), report_bytes); + + // Durable normalization keeps the blob reference, not the + // execution-local runtime path. + let mut durable = resolved; + normalize_durable_updates(&mut durable); + assert_eq!( + durable["report"], + serde_json::json!(fabro_types::format_blob_ref(&blob_hash)) + ); + } + #[tokio::test] async fn demote_skips_keys_the_preamble_never_renders() { let run_store: RunStoreHandle = make_run_store("prompt-demote-hidden").await.into(); diff --git a/lib/components/fabro-workflow/src/handler/llm/preamble.rs b/lib/components/fabro-workflow/src/handler/llm/preamble.rs index 87a43dca7..d4e2aa6f4 100644 --- a/lib/components/fabro-workflow/src/handler/llm/preamble.rs +++ b/lib/components/fabro-workflow/src/handler/llm/preamble.rs @@ -788,7 +788,7 @@ mod tests { "security_findings", large_prompt_value( 1_843_279, - "/workspace/.fabro/blobs/findings.json", + "/fabro/runtime/blobs/findings.json", "{\"findings\":[\n{\"severity\":\"high\"}", ), ); @@ -798,7 +798,7 @@ mod tests { keys::COMMAND_OUTPUT.to_string(), large_prompt_value( 12 * 1024, - "/workspace/.fabro/blobs/output.json", + "/fabro/runtime/blobs/output.json", "first result\nsecond result", ), ); @@ -819,12 +819,12 @@ mod tests { "\n## Completed stages\n", "- **scan**: succeeded\n", " - Script: `scan --json`\n", - " - Output (12.0 KB; full value: `/workspace/.fabro/blobs/output.json`)\n", + " - Output (12.0 KB; full value: `/fabro/runtime/blobs/output.json`)\n", " Preview: first result\n", " second result…\n", "\n## Context\n", "- security_findings (1.8 MB; full value: ", - "`/workspace/.fabro/blobs/findings.json`)\n", + "`/fabro/runtime/blobs/findings.json`)\n", " Preview: {\"findings\":[\n", " {\"severity\":\"high\"}…\n", ) @@ -1703,7 +1703,7 @@ mod tests { "security_findings", large_prompt_value( 1_843_279, - "/workspace/.fabro/blobs/findings.json", + "/fabro/runtime/blobs/findings.json", "{\"findings\": [\n{\"message\": \"a | b\"}]}", ), ); @@ -1718,7 +1718,7 @@ mod tests { assert!(preamble.contains(concat!( "| security_findings | 1.8 MB; full value: ", - "`/workspace/.fabro/blobs/findings.json`; Preview: ", + "`/fabro/runtime/blobs/findings.json`; Preview: ", "{\"findings\": [ {\"message\": \"a \\| b\"}]}… |", ))); assert!(!preamble.contains("fabroLargeValue")); diff --git a/lib/components/fabro-workflow/tests/it/git_integration.rs b/lib/components/fabro-workflow/tests/it/git_integration.rs index bcdf8f9e0..59f9afdb8 100644 --- a/lib/components/fabro-workflow/tests/it/git_integration.rs +++ b/lib/components/fabro-workflow/tests/it/git_integration.rs @@ -298,3 +298,235 @@ async fn git_checkpoint_skips_start_node() { assert!(!checkpoint_node_ids.contains(&"start")); assert!(checkpoint_node_ids.contains(&"work")); } + +/// Sandbox double for remote-style runs: commands and files operate on a real +/// local checkout, but the workflow engine's run directory is reported as +/// inaccessible (as it is for Docker/Daytona) and the sandbox exposes a +/// runtime directory outside the checkout. +struct RemoteRuntimeSandbox { + inner: fabro_agent::LocalSandbox, + hidden_path: String, + runtime_directory: String, +} + +#[async_trait::async_trait] +impl Sandbox for RemoteRuntimeSandbox { + async fn read_file_bytes(&self, path: &str) -> fabro_sandbox::Result> { + self.inner.read_file_bytes(path).await + } + + async fn write_file(&self, path: &str, content: &str) -> fabro_sandbox::Result<()> { + self.inner.write_file(path, content).await + } + + async fn delete_file(&self, path: &str) -> fabro_sandbox::Result<()> { + self.inner.delete_file(path).await + } + + async fn file_exists(&self, path: &str) -> fabro_sandbox::Result { + if path == self.hidden_path { + return Ok(false); + } + self.inner.file_exists(path).await + } + + async fn list_directory( + &self, + path: &str, + depth: Option, + ) -> fabro_sandbox::Result> { + self.inner.list_directory(path, depth).await + } + + async fn exec_command( + &self, + command: &str, + timeout_ms: u64, + working_dir: Option<&str>, + env_vars: Option<&HashMap>, + cancel_token: Option, + ) -> fabro_sandbox::Result { + self.inner + .exec_command(command, timeout_ms, working_dir, env_vars, cancel_token) + .await + } + + async fn grep( + &self, + pattern: &str, + path: &str, + options: &fabro_sandbox::GrepOptions, + ) -> fabro_sandbox::Result> { + self.inner.grep(pattern, path, options).await + } + + async fn download_file_to_local( + &self, + remote_path: &str, + local_path: &Path, + ) -> fabro_sandbox::Result<()> { + self.inner + .download_file_to_local(remote_path, local_path) + .await + } + + async fn upload_file_from_local( + &self, + local_path: &Path, + remote_path: &str, + ) -> fabro_sandbox::Result<()> { + self.inner + .upload_file_from_local(local_path, remote_path) + .await + } + + async fn initialize(&self) -> fabro_sandbox::Result<()> { + Ok(()) + } + + async fn cleanup(&self) -> fabro_sandbox::Result<()> { + Ok(()) + } + + fn working_directory(&self) -> &str { + self.inner.working_directory() + } + + fn runtime_directory(&self) -> Option<&str> { + Some(&self.runtime_directory) + } + + fn platform(&self) -> &str { + self.inner.platform() + } + + fn os_version(&self) -> String { + self.inner.os_version() + } +} + +fn git_status_porcelain(repo_dir: &Path) -> String { + let output = Command::new("git") + .args(["status", "--porcelain"]) + .current_dir(repo_dir) + .output() + .expect("git status --porcelain should run"); + assert_success(&output, "git status --porcelain"); + String::from_utf8(output.stdout).expect("git status output should be UTF-8") +} + +fn git_committed_files(repo_dir: &Path, sha: &str) -> String { + let output = Command::new("git") + .args(["show", "--name-only", "--format=", sha]) + .current_dir(repo_dir) + .output() + .expect("git show --name-only should run"); + assert_success(&output, "git show --name-only"); + String::from_utf8(output.stdout).expect("git show output should be UTF-8") +} + +/// Remote-style prompt demotion must materialize blobs in the sandbox runtime +/// directory, outside the checkout, so a real checkpoint commit can never pick +/// them up, and re-resolution must recreate a deleted materialized file from +/// the durable blob store. Regression test for issue #798. +#[tokio::test] +async fn remote_prompt_demotion_stays_outside_checkout_and_survives_checkpoint() { + use std::time::Duration; + + use fabro_store::test_support as store_test_support; + use fabro_types::settings::run::RunCheckpointSettings; + use fabro_workflow::context::Context; + use fabro_workflow::git::GitAuthor; + use fabro_workflow::runtime_store::RunStoreHandle; + use fabro_workflow::{artifact, sandbox_git}; + use object_store::memory::InMemory; + + let dir = tempfile::tempdir().unwrap(); + let repo_dir = dir.path().join("repo"); + init_repo(&repo_dir); + let runtime_dir = dir.path().join("fabro").join("runtime"); + let run_dir = dir.path().join("run"); + std::fs::create_dir_all(&run_dir).unwrap(); + + let sandbox = RemoteRuntimeSandbox { + inner: fabro_agent::LocalSandbox::new(repo_dir.clone()), + hidden_path: run_dir.to_string_lossy().to_string(), + runtime_directory: runtime_dir.to_string_lossy().to_string(), + }; + + let store = store_test_support::test_database( + Arc::new(InMemory::new()), + "runs/", + Duration::from_millis(1), + None, + ); + let run_store: RunStoreHandle = store.create_run(&fixtures::RUN_2).await.unwrap().into(); + + let oversized = serde_json::json!("x".repeat(64 * 1024)); + let oversized_bytes = serde_json::to_vec(&oversized).unwrap(); + let mut values = HashMap::from([("dataset".to_string(), oversized.clone())]); + artifact::demote_large_values_for_prompt( + &mut values, + &mut HashMap::new(), + &run_store, + &sandbox, + &run_dir, + ) + .await; + + let marker = values["dataset"] + .get("fabroLargeValue") + .expect("oversized value should demote to a marker"); + let blob_path = marker["path"].as_str().unwrap().to_string(); + assert!( + blob_path.starts_with(&runtime_dir.to_string_lossy().to_string()), + "materialized blob {blob_path} should live under the sandbox runtime directory" + ); + assert!( + !blob_path.starts_with(&repo_dir.to_string_lossy().to_string()), + "materialized blob {blob_path} must not live inside the checkout" + ); + + // The agent-facing path is readable through the sandbox. + let contents = sandbox.read_file_bytes(&blob_path).await.unwrap(); + assert_eq!(contents, oversized_bytes); + + // Materialization leaves the checkout clean, and a real checkpoint commit + // stages no runtime blob file. + assert_eq!(git_status_porcelain(&repo_dir), ""); + let sha = sandbox_git::git_checkpoint( + &sandbox, + &fixtures::RUN_2.to_string(), + "work", + "succeeded", + 1, + None, + &RunCheckpointSettings::default(), + &GitAuthor::default(), + ) + .await + .expect("checkpoint commit should succeed"); + assert_eq!(git_committed_files(&repo_dir, &sha).trim(), ""); + assert_eq!(git_status_porcelain(&repo_dir), ""); + + // Removing the materialized file and resolving the value again recreates + // it from the durable blob store. + std::fs::remove_file(&blob_path).unwrap(); + let blob_hash = fabro_types::BlobHash::new(&oversized_bytes); + let context = Context::new(); + context.set( + "report", + serde_json::json!(fabro_types::format_blob_ref(&blob_hash)), + ); + let resolved = artifact::resolved_context_snapshot(&context, &run_store, &sandbox, &run_dir) + .await + .unwrap(); + assert_eq!( + resolved["report"], + serde_json::json!(format!("file://{blob_path}")) + ); + assert_eq!( + sandbox.read_file_bytes(&blob_path).await.unwrap(), + oversized_bytes + ); +} diff --git a/lib/components/fabro-workflow/tests/it/integration.rs b/lib/components/fabro-workflow/tests/it/integration.rs index b72b3cdf8..6c176f79d 100644 --- a/lib/components/fabro-workflow/tests/it/integration.rs +++ b/lib/components/fabro-workflow/tests/it/integration.rs @@ -10136,6 +10136,10 @@ impl fabro_agent::Sandbox for RemoteMockEnv { Ok(self.existing_paths.lock().unwrap().contains(path)) } + fn runtime_directory(&self) -> Option<&str> { + Some("/fabro/runtime") + } + async fn list_directory( &self, _path: &str, @@ -10441,8 +10445,14 @@ async fn downstream_remote_execution_resolves_response_blob_refs_as_text() { assert!( written .iter() - .all(|(path, _)| path.contains("/.fabro/blobs/")), - "nothing is written outside the sandbox blob directory" + .all(|(path, _)| path.starts_with("/fabro/runtime/blobs/")), + "nothing is written outside the sandbox runtime blob directory" + ); + assert!( + written + .iter() + .all(|(path, _)| !path.starts_with("/sandbox")), + "nothing is written inside the repository checkout" ); }