From 4ece9b6480994bc30c6b607b1587168620190f21 Mon Sep 17 00:00:00 2001 From: Nico Hinderling Date: Thu, 28 May 2026 13:18:03 -0700 Subject: [PATCH 1/3] feat(snapshots): Add --all-image-file-names-as-regex flags Add regex variants of the --all-image-file-names flags for selective snapshot uploads. Users can now specify regex patterns instead of literal file names to identify the full set of images in their test suite, enabling removal and rename detection in selective mode. All four flags (literal inline/file, regex inline/file) are mutually exclusive via a clap ArgGroup. Regex patterns are validated at parse time and anchored for full-string matching during upload validation. Co-Authored-By: Claude --- src/api/data_types/snapshots.rs | 42 ++++ src/commands/snapshots/upload.rs | 221 ++++++++++++++++-- .../snapshots/snapshots-upload-help.trycmd | 10 + 3 files changed, 251 insertions(+), 22 deletions(-) diff --git a/src/api/data_types/snapshots.rs b/src/api/data_types/snapshots.rs index 9ef75ce1af..bfa4075d27 100644 --- a/src/api/data_types/snapshots.rs +++ b/src/api/data_types/snapshots.rs @@ -35,6 +35,8 @@ pub struct SnapshotsManifest<'a> { pub selective: bool, #[serde(skip_serializing_if = "Option::is_none")] pub all_image_file_names: Option>, + #[serde(skip_serializing_if = "Option::is_none")] + pub all_image_file_names_as_regex: Option>, #[serde(flatten)] pub vcs_info: VcsInfo<'a>, } @@ -86,6 +88,7 @@ mod tests { diff_threshold: None, selective: false, all_image_file_names: None, + all_image_file_names_as_regex: None, vcs_info: empty_vcs_info(), }; let json = serde_json::to_value(&manifest).unwrap(); @@ -100,6 +103,7 @@ mod tests { diff_threshold: None, selective: true, all_image_file_names: None, + all_image_file_names_as_regex: None, vcs_info: empty_vcs_info(), }; let json = serde_json::to_value(&manifest).unwrap(); @@ -114,6 +118,7 @@ mod tests { diff_threshold: None, selective: true, all_image_file_names: None, + all_image_file_names_as_regex: None, vcs_info: empty_vcs_info(), }; let json = serde_json::to_value(&manifest).unwrap(); @@ -131,12 +136,49 @@ mod tests { diff_threshold: None, selective: true, all_image_file_names: Some(vec!["a.png".into(), "b.png".into()]), + all_image_file_names_as_regex: None, vcs_info: empty_vcs_info(), }; let json = serde_json::to_value(&manifest).unwrap(); assert_eq!(json["all_image_file_names"], json!(["a.png", "b.png"])); } + #[test] + fn manifest_omits_all_image_file_names_as_regex_when_none() { + let manifest = SnapshotsManifest { + app_id: "app".into(), + images: HashMap::new(), + diff_threshold: None, + selective: true, + all_image_file_names: None, + all_image_file_names_as_regex: None, + vcs_info: empty_vcs_info(), + }; + let json = serde_json::to_value(&manifest).unwrap(); + assert!(!json + .as_object() + .unwrap() + .contains_key("all_image_file_names_as_regex")); + } + + #[test] + fn manifest_includes_all_image_file_names_as_regex_when_some() { + let manifest = SnapshotsManifest { + app_id: "app".into(), + images: HashMap::new(), + diff_threshold: None, + selective: true, + all_image_file_names: None, + all_image_file_names_as_regex: Some(vec![".*\\.png".into(), "screens/.*".into()]), + vcs_info: empty_vcs_info(), + }; + let json = serde_json::to_value(&manifest).unwrap(); + assert_eq!( + json["all_image_file_names_as_regex"], + json!([".*\\.png", "screens/.*"]) + ); + } + #[test] fn cli_managed_fields_override_sidecar_fields() { let extra = serde_json::from_value(json!({ diff --git a/src/commands/snapshots/upload.rs b/src/commands/snapshots/upload.rs index 98c32ce742..cb040c5596 100644 --- a/src/commands/snapshots/upload.rs +++ b/src/commands/snapshots/upload.rs @@ -6,13 +6,14 @@ use std::str::FromStr as _; use std::time::Duration; use anyhow::{Context as _, Result}; -use clap::{Arg, ArgMatches, Command}; +use clap::{Arg, ArgGroup, ArgMatches, Command}; use console::style; use futures_util::StreamExt as _; use itertools::Itertools as _; use log::{debug, warn}; use objectstore_client::{ClientBuilder, ExpirationPolicy, OperationResult, Usecase}; use rayon::prelude::*; +use regex::Regex; use secrecy::ExposeSecret as _; use serde_json::Value; use sha2::{Digest as _, Sha256}; @@ -81,7 +82,6 @@ pub fn make_command(command: Command) -> Command { Arg::new("all_image_file_names") .long("all-image-file-names") .value_name("NAMES") - .conflicts_with("all_image_file_names_file") .help( "Comma-separated list of all image names (including subdirectory paths) \ in the full test suite. \ @@ -93,7 +93,6 @@ pub fn make_command(command: Command) -> Command { Arg::new("all_image_file_names_file") .long("all-image-file-names-file") .value_name("PATH") - .conflicts_with("all_image_file_names") .help( "Path to a file containing all image names (including subdirectory paths), \ one per line. \ @@ -101,6 +100,38 @@ pub fn make_command(command: Command) -> Command { Implicitly enables --selective.", ), ) + .arg( + Arg::new("all_image_file_names_as_regex") + .long("all-image-file-names-as-regex") + .value_name("PATTERNS") + .help( + "Comma-separated list of regex patterns matching all image names \ + (including subdirectory paths) in the full test suite. \ + Used with selective uploads to detect image removals and renames. \ + Implicitly enables --selective.", + ), + ) + .arg( + Arg::new("all_image_file_names_as_regex_file") + .long("all-image-file-names-as-regex-file") + .value_name("PATH") + .help( + "Path to a file containing regex patterns matching all image names \ + (including subdirectory paths), one per line. \ + Used with selective uploads to detect image removals and renames. \ + Implicitly enables --selective.", + ), + ) + .group( + ArgGroup::new("all_image_names_group") + .args([ + "all_image_file_names", + "all_image_file_names_file", + "all_image_file_names_as_regex", + "all_image_file_names_as_regex_file", + ]) + .required(false), + ) .git_metadata_args() } @@ -170,24 +201,12 @@ pub fn execute(matches: &ArgMatches) -> Result<()> { validate_image_sizes(&images)?; - let all_image_file_names = parse_all_image_file_names(matches)?; + let all_image_names = parse_all_image_names(matches)?; - let selective = matches.get_flag("selective") || all_image_file_names.is_some(); + let selective = matches.get_flag("selective") || all_image_names.is_some(); - if let Some(ref all_names) = all_image_file_names { - let all_names_set: HashSet<&str> = all_names.iter().map(|s| s.as_str()).collect(); - let mut unknown: Vec = images - .iter() - .map(|img| crate::utils::fs::path_as_url(&img.relative_path)) - .filter(|k| !all_names_set.contains(k.as_str())) - .collect(); - if !unknown.is_empty() { - unknown.sort(); - anyhow::bail!( - "The following uploaded images are not in --all-image-file-names: {}", - unknown.join(", ") - ); - } + if let Some(ref names) = all_image_names { + validate_uploaded_images(&images, names)?; } println!( @@ -202,12 +221,19 @@ pub fn execute(matches: &ArgMatches) -> Result<()> { // Build manifest from discovered images let diff_threshold = matches.get_one::("diff_threshold").copied(); + let (all_image_file_names, all_image_file_names_as_regex) = match all_image_names { + Some(AllImageNames::Literal(names)) => (Some(names), None), + Some(AllImageNames::Regex(patterns)) => (None, Some(patterns)), + None => (None, None), + }; + let manifest = SnapshotsManifest { app_id: app_id.clone(), images: manifest_entries, diff_threshold, selective, all_image_file_names, + all_image_file_names_as_regex, vcs_info, }; @@ -257,13 +283,18 @@ fn normalize_image_names(names: Vec) -> Vec { .collect() } -fn parse_all_image_file_names(matches: &ArgMatches) -> Result>> { +enum AllImageNames { + Literal(Vec), + Regex(Vec), +} + +fn parse_all_image_names(matches: &ArgMatches) -> Result> { if let Some(names_str) = matches.get_one::("all_image_file_names") { let names = normalize_image_names(split_and_trim(names_str, ',')); if names.is_empty() { anyhow::bail!("--all-image-file-names must not be empty"); } - return Ok(Some(names)); + return Ok(Some(AllImageNames::Literal(names))); } if let Some(file_path) = matches.get_one::("all_image_file_names_file") { @@ -275,12 +306,84 @@ fn parse_all_image_file_names(matches: &ArgMatches) -> Result "--all-image-file-names-file is empty or contains only blank lines: {file_path}" ); } - return Ok(Some(names)); + return Ok(Some(AllImageNames::Literal(names))); + } + + if let Some(patterns_str) = matches.get_one::("all_image_file_names_as_regex") { + let patterns = split_and_trim(patterns_str, ','); + if patterns.is_empty() { + anyhow::bail!("--all-image-file-names-as-regex must not be empty"); + } + validate_regex_patterns(&patterns)?; + return Ok(Some(AllImageNames::Regex(patterns))); + } + + if let Some(file_path) = matches.get_one::("all_image_file_names_as_regex_file") { + let content = std::fs::read_to_string(file_path).with_context(|| { + format!("Failed to read --all-image-file-names-as-regex-file: {file_path}") + })?; + let patterns = split_and_trim(&content, '\n'); + if patterns.is_empty() { + anyhow::bail!( + "--all-image-file-names-as-regex-file is empty or contains only blank lines: {file_path}" + ); + } + validate_regex_patterns(&patterns)?; + return Ok(Some(AllImageNames::Regex(patterns))); } Ok(None) } +fn validate_regex_patterns(patterns: &[String]) -> Result<()> { + for p in patterns { + Regex::new(p).with_context(|| format!("Invalid regex pattern: {p}"))?; + } + Ok(()) +} + +fn validate_uploaded_images(images: &[ImageInfo], all_names: &AllImageNames) -> Result<()> { + let image_keys: Vec = images + .iter() + .map(|img| crate::utils::fs::path_as_url(&img.relative_path)) + .collect(); + + let (mut unmatched, flag_name) = match all_names { + AllImageNames::Literal(names) => { + let set: HashSet<&str> = names.iter().map(|s| s.as_str()).collect(); + let unmatched: Vec = image_keys + .into_iter() + .filter(|k| !set.contains(k.as_str())) + .collect(); + (unmatched, "--all-image-file-names") + } + AllImageNames::Regex(patterns) => { + // Anchor patterns to match the full image name, consistent with + // the literal variant's exact-match semantics. + let compiled: Vec = patterns + .iter() + .map(|p| Regex::new(&format!("^(?:{p})$"))) + .collect::, _>>() + .expect("regex patterns were validated at parse time"); + let unmatched = image_keys + .into_iter() + .filter(|name| !compiled.iter().any(|re| re.is_match(name))) + .collect(); + (unmatched, "--all-image-file-names-as-regex") + } + }; + + if !unmatched.is_empty() { + unmatched.sort(); + anyhow::bail!( + "The following uploaded images are not matched by {flag_name}: {}", + unmatched.join(", ") + ); + } + + Ok(()) +} + fn collect_images(dir: &Path) -> Vec { WalkDir::new(dir) .follow_links(true) @@ -624,4 +727,78 @@ mod tests { vec!["img/a.png", "img/b.png", "img/c.png"] ); } + + #[test] + fn test_validate_regex_patterns_valid() { + let patterns = vec![".*\\.png".to_owned(), "screens/.*".to_owned()]; + assert!(validate_regex_patterns(&patterns).is_ok()); + } + + #[test] + fn test_validate_regex_patterns_invalid() { + let patterns = vec!["valid".to_owned(), "[invalid".to_owned()]; + let err = validate_regex_patterns(&patterns).unwrap_err(); + assert!(err.to_string().contains("Invalid regex pattern: [invalid")); + } + + #[test] + fn test_validate_uploaded_images_literal_all_matched() { + let images = vec![ + make_image_with_path("a.png"), + make_image_with_path("sub/b.png"), + ]; + let names = AllImageNames::Literal(vec!["a.png".into(), "sub/b.png".into()]); + assert!(validate_uploaded_images(&images, &names).is_ok()); + } + + #[test] + fn test_validate_uploaded_images_literal_unmatched() { + let images = vec![ + make_image_with_path("a.png"), + make_image_with_path("unknown.png"), + ]; + let names = AllImageNames::Literal(vec!["a.png".into()]); + let err = validate_uploaded_images(&images, &names).unwrap_err(); + assert!(err.to_string().contains("unknown.png")); + assert!(err.to_string().contains("--all-image-file-names")); + } + + #[test] + fn test_validate_uploaded_images_regex_all_matched() { + let images = vec![ + make_image_with_path("screens/login.png"), + make_image_with_path("screens/home.png"), + ]; + let names = AllImageNames::Regex(vec!["screens/.*\\.png".into()]); + assert!(validate_uploaded_images(&images, &names).is_ok()); + } + + #[test] + fn test_validate_uploaded_images_regex_unmatched() { + let images = vec![ + make_image_with_path("screens/login.png"), + make_image_with_path("other/widget.png"), + ]; + let names = AllImageNames::Regex(vec!["screens/.*".into()]); + let err = validate_uploaded_images(&images, &names).unwrap_err(); + assert!(err.to_string().contains("other/widget.png")); + assert!(err.to_string().contains("--all-image-file-names-as-regex")); + } + + #[test] + fn test_validate_uploaded_images_regex_is_anchored() { + let images = vec![make_image_with_path("prefix_foo_suffix.png")]; + // "foo" would substring-match, but anchored it should NOT match + let names = AllImageNames::Regex(vec!["foo".into()]); + assert!(validate_uploaded_images(&images, &names).is_err()); + } + + fn make_image_with_path(relative: &str) -> ImageInfo { + ImageInfo { + path: PathBuf::from(relative), + relative_path: PathBuf::from(relative), + width: 100, + height: 100, + } + } } diff --git a/tests/integration/_cases/snapshots/snapshots-upload-help.trycmd b/tests/integration/_cases/snapshots/snapshots-upload-help.trycmd index 7a22af5fa2..6ca2d4cf17 100644 --- a/tests/integration/_cases/snapshots/snapshots-upload-help.trycmd +++ b/tests/integration/_cases/snapshots/snapshots-upload-help.trycmd @@ -56,6 +56,16 @@ Options: Used with selective uploads to detect image removals and renames. Implicitly enables --selective. + --all-image-file-names-as-regex + Comma-separated list of regex patterns matching all image names (including subdirectory + paths) in the full test suite. Used with selective uploads to detect image removals and + renames. Implicitly enables --selective. + + --all-image-file-names-as-regex-file + Path to a file containing regex patterns matching all image names (including subdirectory + paths), one per line. Used with selective uploads to detect image removals and renames. + Implicitly enables --selective. + --head-sha The VCS commit sha to use for the upload. If not provided, the current commit sha will be used. From dae87dbd99dea74dfc69976450a7531d76176cd9 Mon Sep 17 00:00:00 2001 From: Nico Hinderling Date: Thu, 28 May 2026 13:28:49 -0700 Subject: [PATCH 2/3] docs(changelog): Add entry for regex flags Co-Authored-By: Claude --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 381eeb5cf9..ec32e04412 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,7 @@ - (snapshots) Add `snapshots diff` command for locally comparing directories of PNG snapshot images using odiff ([#3306](https://github.com/getsentry/sentry-cli/pull/3306)) - (snapshots) Add `snapshots download` command for downloading baseline snapshot images from Sentry ([#3310](https://github.com/getsentry/sentry-cli/pull/3310)) - (snapshots) Add `--all-image-file-names` and `--all-image-file-names-file` flags to `snapshots upload` for detecting image removals and renames in selective builds ([#3312](https://github.com/getsentry/sentry-cli/pull/3312)) +- (snapshots) Add `--all-image-file-names-as-regex` and `--all-image-file-names-as-regex-file` flags to `snapshots upload` for regex-based image name matching in selective builds ([#3316](https://github.com/getsentry/sentry-cli/pull/3316)) ### Fixes From bffa71dacef7527403fb0488061c78596cb86cd2 Mon Sep 17 00:00:00 2001 From: Nico Hinderling Date: Thu, 28 May 2026 14:22:12 -0700 Subject: [PATCH 3/3] fix(snapshots): Validate anchored form of regex patterns at parse time Patterns using verbose mode with comments (e.g. `(?x)foo # comment`) are valid on their own but break when anchored as `^(?:...)$` because the comment consumes the closing group. Validate the anchored form at parse time so we fail with a clear error instead of panicking during upload validation. Co-Authored-By: Claude --- src/commands/snapshots/upload.rs | 19 ++++++++++++++++--- 1 file changed, 16 insertions(+), 3 deletions(-) diff --git a/src/commands/snapshots/upload.rs b/src/commands/snapshots/upload.rs index cb040c5596..fa0b51f87d 100644 --- a/src/commands/snapshots/upload.rs +++ b/src/commands/snapshots/upload.rs @@ -335,9 +335,13 @@ fn parse_all_image_names(matches: &ArgMatches) -> Result> Ok(None) } +fn anchor_pattern(p: &str) -> String { + format!("^(?:{p})$") +} + fn validate_regex_patterns(patterns: &[String]) -> Result<()> { for p in patterns { - Regex::new(p).with_context(|| format!("Invalid regex pattern: {p}"))?; + Regex::new(&anchor_pattern(p)).with_context(|| format!("Invalid regex pattern: {p}"))?; } Ok(()) } @@ -362,9 +366,9 @@ fn validate_uploaded_images(images: &[ImageInfo], all_names: &AllImageNames) -> // the literal variant's exact-match semantics. let compiled: Vec = patterns .iter() - .map(|p| Regex::new(&format!("^(?:{p})$"))) + .map(|p| Regex::new(&anchor_pattern(p))) .collect::, _>>() - .expect("regex patterns were validated at parse time"); + .expect("anchored patterns were validated at parse time"); let unmatched = image_keys .into_iter() .filter(|name| !compiled.iter().any(|re| re.is_match(name))) @@ -741,6 +745,15 @@ mod tests { assert!(err.to_string().contains("Invalid regex pattern: [invalid")); } + #[test] + fn test_validate_regex_patterns_rejects_verbose_comment_that_breaks_anchor() { + // (?x) enables verbose mode where # starts a comment. When anchored + // as ^(?:(?x)foo # comment)$, the comment consumes the closing ")$", + // producing an invalid pattern. Validation must catch this. + let patterns = vec!["(?x)foo # comment".to_owned()]; + assert!(validate_regex_patterns(&patterns).is_err()); + } + #[test] fn test_validate_uploaded_images_literal_all_matched() { let images = vec![