From 31ef250e2cecb8bee2d4d6900a0053ad745f7620 Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Sun, 29 Mar 2026 22:23:33 -0400 Subject: [PATCH] Fix asset capture robustness and retry filtering --- docs/reference/cli.mdx | 6 +- lib/crates/fabro-cli/src/args.rs | 8 ++ lib/crates/fabro-cli/src/commands/asset/cp.rs | 16 ++-- .../fabro-cli/src/commands/asset/list.rs | 6 +- lib/crates/fabro-cli/tests/it/scenario.rs | 35 ++++++- .../fabro-workflows/src/asset_snapshot.rs | 93 ++++++++++++++++--- lib/crates/fabro-workflows/src/assets.rs | 74 ++++++++++++++- 7 files changed, 213 insertions(+), 25 deletions(-) diff --git a/docs/reference/cli.mdx b/docs/reference/cli.mdx index 106a7dc4e..f42568e51 100644 --- a/docs/reference/cli.mdx +++ b/docs/reference/cli.mdx @@ -759,12 +759,14 @@ List assets (screenshots, test reports, traces) collected from a workflow run. ```bash fabro asset list fabro asset list --node verify --json +fabro asset list --node verify --retry 2 ``` | Argument / Flag | Description | |---|---| | `` | Run ID or unambiguous prefix (required) | | `--node ` | Filter to assets from a specific node | +| `--retry ` | Filter to assets from a specific retry attempt | | `--json` | Output as JSON | ## `fabro asset cp` @@ -775,6 +777,7 @@ Copy assets from a workflow run to the local filesystem. fabro asset cp ./output # all assets, flat fabro asset cp ./output --tree # preserve directory structure fabro asset cp :report.html ./output # specific file +fabro asset cp :report.html ./output --node verify --retry 2 ``` | Argument / Flag | Description | @@ -782,9 +785,10 @@ fabro asset cp :report.html ./output # specific file | `` | `RUN_ID` (all assets) or `RUN_ID:path` (specific file) | | `[DEST]` | Destination directory (defaults to `.`) | | `--node ` | Filter to assets from a specific node | +| `--retry ` | Filter to assets from a specific retry attempt | | `--tree` | Preserve `\{node\}/\{retry\}/` directory structure | -When copying all assets in flat mode, filenames must be unique across nodes. Use `--tree` or `--node` to disambiguate. +When copying all assets in flat mode, filenames must be unique across nodes and retries. Use `--tree`, `--node`, or `--retry` to disambiguate. ## `fabro provider login` diff --git a/lib/crates/fabro-cli/src/args.rs b/lib/crates/fabro-cli/src/args.rs index 53ab33cdb..82ebb3422 100644 --- a/lib/crates/fabro-cli/src/args.rs +++ b/lib/crates/fabro-cli/src/args.rs @@ -321,6 +321,10 @@ pub(crate) struct AssetListArgs { #[arg(long)] pub(crate) node: Option, + /// Filter to assets from a specific retry attempt + #[arg(long)] + pub(crate) retry: Option, + /// Output as JSON #[arg(long)] pub(crate) json: bool, @@ -339,6 +343,10 @@ pub(crate) struct AssetCpArgs { #[arg(long)] pub(crate) node: Option, + /// Filter to assets from a specific retry attempt + #[arg(long)] + pub(crate) retry: Option, + /// Preserve {node_slug}/retry_{N}/ directory structure #[arg(long)] pub(crate) tree: bool, diff --git a/lib/crates/fabro-cli/src/commands/asset/cp.rs b/lib/crates/fabro-cli/src/commands/asset/cp.rs index 051dfb9ac..be877bed7 100644 --- a/lib/crates/fabro-cli/src/commands/asset/cp.rs +++ b/lib/crates/fabro-cli/src/commands/asset/cp.rs @@ -16,7 +16,11 @@ pub(super) fn cp_command(args: &AssetCpArgs, globals: &GlobalArgs) -> Result<()> let (run_id, asset_path) = parse_source(&args.source); let run = resolve_run(&base, run_id)?; let runtime_state = RuntimeState::new(&run.path); - let entries = scan_assets(&runtime_state.assets_dir(), args.node.as_deref())?; + let entries = scan_assets( + &runtime_state.assets_dir(), + args.node.as_deref(), + args.retry, + )?; if entries.is_empty() { bail!("No assets found for this run"); @@ -33,14 +37,14 @@ pub(super) fn cp_command(args: &AssetCpArgs, globals: &GlobalArgs) -> Result<()> if matching.is_empty() { bail!("No asset matching path '{path}' found in this run"); } - if matching.len() > 1 && args.node.is_none() { - let nodes: Vec<_> = matching + if matching.len() > 1 { + let candidates: Vec<_> = matching .iter() - .map(|entry| entry.node_slug.as_str()) + .map(|entry| format!("{}:retry_{}", entry.node_slug, entry.retry)) .collect(); bail!( - "Path '{path}' exists in multiple nodes: {}. Use --node to disambiguate.", - nodes.join(", ") + "Path '{path}' matches multiple assets: {}. Use --node and/or --retry to disambiguate.", + candidates.join(", ") ); } diff --git a/lib/crates/fabro-cli/src/commands/asset/list.rs b/lib/crates/fabro-cli/src/commands/asset/list.rs index cffeda2c6..19f26226e 100644 --- a/lib/crates/fabro-cli/src/commands/asset/list.rs +++ b/lib/crates/fabro-cli/src/commands/asset/list.rs @@ -13,7 +13,11 @@ pub(super) fn list_command(args: &AssetListArgs, globals: &GlobalArgs) -> Result let base = runs_base(&cli_settings.storage_dir()); let run = resolve_run(&base, &args.run_id)?; let runtime_state = RuntimeState::new(&run.path); - let entries = scan_assets(&runtime_state.assets_dir(), args.node.as_deref())?; + let entries = scan_assets( + &runtime_state.assets_dir(), + args.node.as_deref(), + args.retry, + )?; if args.json { println!("{}", serde_json::to_string_pretty(&entries)?); diff --git a/lib/crates/fabro-cli/tests/it/scenario.rs b/lib/crates/fabro-cli/tests/it/scenario.rs index 3dfc89c5f..634418cc2 100644 --- a/lib/crates/fabro-cli/tests/it/scenario.rs +++ b/lib/crates/fabro-cli/tests/it/scenario.rs @@ -783,7 +783,7 @@ fn local_run_lifecycle() { ); // 6. Seed a synthetic asset so asset list/cp have something to work with. - let asset_dir = RuntimeState::new(&run_dir).asset_stage_dir("step1", 0); + let asset_dir = RuntimeState::new(&run_dir).asset_stage_dir("step1", 1); std::fs::create_dir_all(&asset_dir).unwrap(); std::fs::write(asset_dir.join("output.txt"), "asset-content-42").unwrap(); std::fs::write( @@ -791,8 +791,16 @@ fn local_run_lifecycle() { r#"{"files_copied":1,"total_bytes":16,"files_skipped":0,"download_errors":0,"hash_errors":0,"captured_assets":[{"path":"output.txt","mime":"text/plain","content_md5":"f02439728c0a94b7bfc465acb1201a1f","content_sha256":"0af9dea3e1c2dec968531c18c9331659b8268e8c9cf24b01cda7b8ce51d2ff00","bytes":16}]}"#, ) .unwrap(); + let retry_two_dir = RuntimeState::new(&run_dir).asset_stage_dir("step1", 2); + std::fs::create_dir_all(&retry_two_dir).unwrap(); + std::fs::write(retry_two_dir.join("output.txt"), "asset-content-84").unwrap(); + std::fs::write( + retry_two_dir.join("manifest.json"), + r#"{"files_copied":1,"total_bytes":16,"files_skipped":0,"download_errors":0,"hash_errors":0,"captured_assets":[{"path":"output.txt","mime":"text/plain","content_md5":"5b4e23e40a1630f9caa15a4cb6cfb79b","content_sha256":"1f71e0df61fc3b4e1ee3aba7ceac9ae391af22595b5b5630d97d34cf33d4d540","bytes":16}]}"#, + ) + .unwrap(); - // 7. asset list — now shows the seeded asset + // 7. asset list — now shows the seeded assets let asset_list_out2 = fabro_home(&["asset", "list", &run_id, "--json"]).success(); let asset_list_stdout2 = String::from_utf8(asset_list_out2.get_output().stdout.clone()).unwrap(); @@ -800,13 +808,21 @@ fn local_run_lifecycle() { .expect("asset list --json should produce a JSON array"); assert_eq!( assets.len(), - 1, - "should have one asset: {asset_list_stdout2}" + 2, + "should have two assets: {asset_list_stdout2}" ); assert_eq!(assets[0]["relative_path"].as_str(), Some("output.txt")); assert_eq!(assets[0]["node_slug"].as_str(), Some("step1")); + let retry_filtered_out = + fabro_home(&["asset", "list", &run_id, "--retry", "1", "--json"]).success(); + let retry_filtered_stdout = + String::from_utf8(retry_filtered_out.get_output().stdout.clone()).unwrap(); + let retry_filtered_assets: Vec = serde_json::from_str(&retry_filtered_stdout) + .expect("asset list --json should produce a JSON array"); + assert_eq!(retry_filtered_assets.len(), 1); + assert_eq!(retry_filtered_assets[0]["retry"].as_u64(), Some(1)); - // 8. asset cp — copy the asset out + // 8. asset cp — ambiguous without --retry when multiple retries captured the same path let asset_dest = tmp.path().join("asset_copy"); fabro_home(&[ "asset", @@ -814,6 +830,15 @@ fn local_run_lifecycle() { &format!("{run_id}:output.txt"), asset_dest.to_str().unwrap(), ]) + .failure(); + fabro_home(&[ + "asset", + "cp", + &format!("{run_id}:output.txt"), + asset_dest.to_str().unwrap(), + "--retry", + "1", + ]) .success(); let copied = std::fs::read_to_string(asset_dest.join("output.txt")).unwrap(); assert_eq!( diff --git a/lib/crates/fabro-workflows/src/asset_snapshot.rs b/lib/crates/fabro-workflows/src/asset_snapshot.rs index e462659e1..5ab1a46aa 100644 --- a/lib/crates/fabro-workflows/src/asset_snapshot.rs +++ b/lib/crates/fabro-workflows/src/asset_snapshot.rs @@ -1,8 +1,8 @@ use fabro_agent::Sandbox; +use fabro_sandbox::shell_quote; use serde::{Deserialize, Serialize}; use sha2::{Digest, Sha256}; use std::path::Path; -use tokio::fs; use tracing::{debug, warn}; /// A file discovered by the find command. @@ -67,12 +67,12 @@ const MAX_TOTAL_SIZE: u64 = 50 * 1024 * 1024; /// Globs with `/` are treated as directory patterns: the trailing `/**` (if any) is stripped /// and the remainder is matched via `-path '*/{dir}/*'`. pub fn build_find_command(root: &str, platform: &str, globs: &[String]) -> String { - let mut cmd = format!("find {root}"); + let mut cmd = format!("find {}", shell_quote(root)); // Prune excluded directories let prune_parts: Vec = EXCLUDE_DIRS .iter() - .map(|d| format!("-name '{d}'")) + .map(|d| format!("-name {}", shell_quote(d))) .collect(); cmd.push_str(" \\( "); cmd.push_str(&prune_parts.join(" -o ")); @@ -86,10 +86,10 @@ pub fn build_find_command(root: &str, platform: &str, globs: &[String]) -> Strin if glob.contains('/') { // Directory-style glob: strip trailing /** and match as path let dir = glob.trim_end_matches("/**").trim_end_matches("/*"); - conditions.push(format!(" -path '*/{dir}/*'")); + conditions.push(format!(" -path {}", shell_quote(&format!("*/{dir}/*")))); } else { // Filename glob - conditions.push(format!(" -name '{glob}'")); + conditions.push(format!(" -name {}", shell_quote(glob))); } } cmd.push_str(&conditions.join(" -o")); @@ -275,6 +275,31 @@ fn compute_asset_info( }) } +fn write_asset_manifest(stage_dir: &Path, summary: &AssetCollectionSummary) -> Result<(), String> { + let json = serde_json::to_string_pretty(summary) + .map_err(|e| format!("failed to serialize manifest: {e}"))?; + let manifest_path = stage_dir.join("manifest.json"); + if let Some(parent) = manifest_path.parent() { + std::fs::create_dir_all(parent).map_err(|e| { + format!( + "failed to create manifest directory {}: {e}", + parent.display() + ) + })?; + } + std::fs::write(&manifest_path, json) + .map_err(|e| format!("failed to write {}: {e}", manifest_path.display()))?; + Ok(()) +} + +fn cleanup_asset_stage_dir(stage_dir: &Path) -> Result<(), String> { + if !stage_dir.exists() { + return Ok(()); + } + std::fs::remove_dir_all(stage_dir) + .map_err(|e| format!("failed to clean up {}: {e}", stage_dir.display())) +} + /// Collect asset files matching the configured globs that were created during this stage. pub async fn collect_assets( sandbox: &dyn Sandbox, @@ -348,12 +373,12 @@ pub async fn collect_assets( }; if files_copied > 0 { - if let Ok(json) = serde_json::to_string_pretty(&summary) { - let manifest_path = stage_dir.join("manifest.json"); - if let Some(parent) = manifest_path.parent() { - let _ = fs::create_dir_all(parent).await; - } - let _ = fs::write(&manifest_path, json).await; + if let Err(e) = write_asset_manifest(stage_dir, &summary) { + let cleanup_suffix = match cleanup_asset_stage_dir(stage_dir) { + Ok(()) => String::new(), + Err(cleanup_err) => format!("; cleanup failed: {cleanup_err}"), + }; + return Err(format!("{e}{cleanup_suffix}")); } } @@ -399,6 +424,7 @@ mod tests { use super::*; use fabro_agent::sandbox::ExecResult; use std::collections::HashMap; + use std::fs; /// Minimal mock sandbox for asset_snapshot tests. struct AssetMockSandbox { @@ -637,6 +663,15 @@ mod tests { assert!(cmd.contains("-name '*.trace.zip'")); } + #[test] + fn build_find_command_shell_quotes_root_and_globs() { + let globs = vec!["test result's/**".to_string(), "*.trace zip".to_string()]; + let cmd = build_find_command("/workspace with spaces", "linux", &globs); + assert!(cmd.starts_with(&format!("find {}", shell_quote("/workspace with spaces")))); + assert!(cmd.contains(&format!("-path {}", shell_quote("*/test result's/*")))); + assert!(cmd.contains(&format!("-name {}", shell_quote("*.trace zip")))); + } + #[test] fn build_find_command_darwin() { let globs = vec!["test-results/**".to_string()]; @@ -749,6 +784,42 @@ mod tests { assert_eq!(summary.hash_errors, 0); } + #[cfg(unix)] + #[test] + fn write_asset_manifest_failure_cleans_up_stage_dir() { + use std::fs::Permissions; + use std::os::unix::fs::PermissionsExt; + + let parent = tempfile::tempdir().unwrap(); + let stage_dir = parent.path().join("stage"); + fs::create_dir_all(stage_dir.join("test-results")).unwrap(); + fs::write(stage_dir.join("test-results/report.xml"), "").unwrap(); + fs::set_permissions(&stage_dir, Permissions::from_mode(0o555)).unwrap(); + + let summary = AssetCollectionSummary { + files_copied: 1, + total_bytes: 7, + files_skipped: 0, + download_errors: 0, + hash_errors: 0, + captured_assets: vec![CapturedAssetInfo { + path: "test-results/report.xml".to_string(), + mime: "text/xml".to_string(), + content_md5: "f1430934c390c118ed2f148e1d44d36c".to_string(), + content_sha256: "28e51ddac37391b99c2b9053f1122d0bf84b02365e6fd8c6e8667378bd00f436" + .to_string(), + bytes: 7, + }], + }; + + let err = write_asset_manifest(&stage_dir, &summary).unwrap_err(); + assert!(err.contains("failed to write")); + + fs::set_permissions(&stage_dir, Permissions::from_mode(0o755)).unwrap(); + cleanup_asset_stage_dir(&stage_dir).unwrap(); + assert!(!stage_dir.exists()); + } + #[test] fn collect_asset_paths_from_manifests() { let tmp = tempfile::tempdir().unwrap(); diff --git a/lib/crates/fabro-workflows/src/assets.rs b/lib/crates/fabro-workflows/src/assets.rs index 7e4c9608b..d08c4ce35 100644 --- a/lib/crates/fabro-workflows/src/assets.rs +++ b/lib/crates/fabro-workflows/src/assets.rs @@ -20,7 +20,11 @@ fn serialize_path(path: &Path, serializer: S) -> Result) -> Result> { +pub fn scan_assets( + assets_dir: &Path, + node_filter: Option<&str>, + retry_filter: Option, +) -> Result> { let Ok(nodes) = std::fs::read_dir(assets_dir) else { return Ok(Vec::new()); }; @@ -49,6 +53,12 @@ pub fn scan_assets(assets_dir: &Path, node_filter: Option<&str>) -> Result) -> Result