From 44efbe683ec9b0cb2788778cb011d73737bce432 Mon Sep 17 00:00:00 2001 From: overtrue Date: Fri, 20 Mar 2026 22:55:07 +0800 Subject: [PATCH 1/3] feat(phase-2): preserve content type during mirror sync --- crates/cli/src/commands/mirror.rs | 239 +++++++++++++++++++++++++++++- crates/cli/tests/integration.rs | 77 ++++++++++ 2 files changed, 308 insertions(+), 8 deletions(-) diff --git a/crates/cli/src/commands/mirror.rs b/crates/cli/src/commands/mirror.rs index a8312d7..0d7260d 100644 --- a/crates/cli/src/commands/mirror.rs +++ b/crates/cli/src/commands/mirror.rs @@ -64,6 +64,48 @@ struct FileInfo { etag: Option, } +trait MirrorCopySource { + async fn head_object_for_mirror( + &self, + path: &RemotePath, + ) -> rc_core::Result; + + async fn get_object_for_mirror(&self, path: &RemotePath) -> rc_core::Result>; +} + +trait MirrorCopyTarget { + async fn put_object_for_mirror( + &self, + path: &RemotePath, + data: Vec, + content_type: Option<&str>, + ) -> rc_core::Result; +} + +impl MirrorCopySource for S3Client { + async fn head_object_for_mirror( + &self, + path: &RemotePath, + ) -> rc_core::Result { + rc_core::ObjectStore::head_object(self, path).await + } + + async fn get_object_for_mirror(&self, path: &RemotePath) -> rc_core::Result> { + rc_core::ObjectStore::get_object(self, path).await + } +} + +impl MirrorCopyTarget for S3Client { + async fn put_object_for_mirror( + &self, + path: &RemotePath, + data: Vec, + content_type: Option<&str>, + ) -> rc_core::Result { + rc_core::ObjectStore::put_object(self, path, data, content_type).await + } +} + /// Execute the mirror command pub async fn execute(args: MirrorArgs, output_config: OutputConfig) -> ExitCode { let formatter = Formatter::new(output_config); @@ -297,14 +339,14 @@ pub async fn execute(args: MirrorArgs, output_config: OutputConfig) -> ExitCode .expect("semaphore should not be closed"); copy_tasks.spawn(async move { let _permit = permit; - let result = match source_client.get_object(&source_full).await { - Ok(data) => target_client - .put_object(&target_full, data, None) - .await - .map(|_| ()) - .map_err(|e| format!("Failed to upload {key}: {e}")), - Err(e) => Err(format!("Failed to download {key}: {e}")), - }; + let result = copy_object_with_metadata( + source_client.as_ref(), + target_client.as_ref(), + &source_full, + &target_full, + ) + .await + .map_err(|e| format!("Failed to copy {key}: {e}")); (key, result) }); } @@ -431,6 +473,40 @@ pub async fn execute(args: MirrorArgs, output_config: OutputConfig) -> ExitCode } } +// Mirror should preserve source content type when metadata is available, but +// still fall back to a plain byte copy if metadata lookup is not supported. +async fn copy_object_with_metadata( + source_client: &S, + target_client: &T, + source_path: &RemotePath, + target_path: &RemotePath, +) -> rc_core::Result<()> +where + S: MirrorCopySource, + T: MirrorCopyTarget, +{ + let content_type = match source_client.head_object_for_mirror(source_path).await { + Ok(source_info) => source_info.content_type, + Err(error) => { + tracing::warn!( + source = %format!( + "{}/{}/{}", + source_path.alias, source_path.bucket, source_path.key + ), + error = %error, + "Falling back to mirror upload without source content type" + ); + None + } + }; + + let data = source_client.get_object_for_mirror(source_path).await?; + target_client + .put_object_for_mirror(target_path, data, content_type.as_deref()) + .await?; + Ok(()) +} + async fn list_objects_map( client: &S3Client, path: &RemotePath, @@ -543,6 +619,153 @@ fn compare_objects_internal( #[cfg(test)] mod tests { use super::*; + use rc_core::ObjectInfo; + use std::sync::Mutex; + + #[derive(Debug)] + struct TestMirrorSource { + content_type: Option, + data: Vec, + head_calls: Mutex>, + get_calls: Mutex>, + } + + impl TestMirrorSource { + fn new(content_type: Option<&str>, data: &[u8]) -> Self { + Self { + content_type: content_type.map(ToOwned::to_owned), + data: data.to_vec(), + head_calls: Mutex::new(Vec::new()), + get_calls: Mutex::new(Vec::new()), + } + } + } + + impl MirrorCopySource for TestMirrorSource { + async fn head_object_for_mirror(&self, path: &RemotePath) -> rc_core::Result { + self.head_calls + .lock() + .expect("head call mutex should not be poisoned") + .push(path.key.clone()); + Ok(ObjectInfo { + key: path.key.clone(), + size_bytes: Some(self.data.len() as i64), + size_human: None, + last_modified: None, + etag: None, + storage_class: None, + content_type: self.content_type.clone(), + metadata: None, + is_dir: false, + }) + } + + async fn get_object_for_mirror(&self, path: &RemotePath) -> rc_core::Result> { + self.get_calls + .lock() + .expect("get call mutex should not be poisoned") + .push(path.key.clone()); + Ok(self.data.clone()) + } + } + + #[derive(Debug, PartialEq, Eq)] + struct PutCall { + key: String, + content_type: Option, + data: Vec, + } + + #[derive(Debug, Default)] + struct TestMirrorTarget { + put_calls: Mutex>, + } + + impl MirrorCopyTarget for TestMirrorTarget { + async fn put_object_for_mirror( + &self, + path: &RemotePath, + data: Vec, + content_type: Option<&str>, + ) -> rc_core::Result { + self.put_calls + .lock() + .expect("put call mutex should not be poisoned") + .push(PutCall { + key: path.key.clone(), + content_type: content_type.map(ToOwned::to_owned), + data: data.clone(), + }); + + Ok(ObjectInfo::file(path.key.clone(), data.len() as i64)) + } + } + + #[tokio::test] + async fn test_copy_object_with_metadata_preserves_content_type() { + let source = TestMirrorSource::new(Some("image/jpeg"), b"jpeg-bytes"); + let target = TestMirrorTarget::default(); + let source_path = RemotePath::new("stage", "images", "photo.jpg"); + let target_path = RemotePath::new("prod", "images", "photo.jpg"); + + copy_object_with_metadata(&source, &target, &source_path, &target_path) + .await + .expect("copy should succeed"); + + assert_eq!( + source + .head_calls + .lock() + .expect("head call mutex should not be poisoned") + .as_slice(), + ["photo.jpg"] + ); + assert_eq!( + source + .get_calls + .lock() + .expect("get call mutex should not be poisoned") + .as_slice(), + ["photo.jpg"] + ); + assert_eq!( + target + .put_calls + .lock() + .expect("put call mutex should not be poisoned") + .as_slice(), + [PutCall { + key: "photo.jpg".to_string(), + content_type: Some("image/jpeg".to_string()), + data: b"jpeg-bytes".to_vec(), + }] + ); + } + + #[tokio::test] + async fn test_copy_object_with_metadata_passes_none_when_source_has_no_content_type() { + let source = TestMirrorSource::new(None, b"plain-bytes"); + let target = TestMirrorTarget::default(); + let source_path = RemotePath::new("stage", "docs", "readme.txt"); + let target_path = RemotePath::new("prod", "docs", "readme.txt"); + + copy_object_with_metadata(&source, &target, &source_path, &target_path) + .await + .expect("copy should succeed"); + + assert_eq!( + target + .put_calls + .lock() + .expect("put call mutex should not be poisoned") + .as_slice(), + [PutCall { + key: "readme.txt".to_string(), + content_type: None, + data: b"plain-bytes".to_vec(), + }] + ); + } #[test] fn test_compare_objects_internal() { diff --git a/crates/cli/tests/integration.rs b/crates/cli/tests/integration.rs index 9cab0e3..a30ee99 100644 --- a/crates/cli/tests/integration.rs +++ b/crates/cli/tests/integration.rs @@ -2155,6 +2155,83 @@ mod mirror_operations { cleanup_bucket(config_dir.path(), &bucket_name); cleanup_bucket(config_dir.path(), &bucket_name2); } + + #[test] + fn test_mirror_preserves_content_type() { + let (config_dir, bucket_name) = match setup_with_alias("mirrorcontenttype") { + Some(v) => v, + None => { + eprintln!("Skipping: S3 test config not available"); + return; + } + }; + + let target_bucket = format!("{}-dest", bucket_name); + let output = run_rc( + &["mb", &format!("test/{}", target_bucket)], + config_dir.path(), + ); + assert!( + output.status.success(), + "Failed to create destination bucket: {}", + String::from_utf8_lossy(&output.stderr) + ); + + let temp_file = tempfile::NamedTempFile::new().expect("Failed to create temp file"); + std::fs::write(temp_file.path(), b"not-a-real-jpeg").expect("Failed to write test file"); + + let output = run_rc( + &[ + "cp", + temp_file.path().to_str().unwrap(), + &format!("test/{}/source/photo.bin", bucket_name), + "--content-type", + "image/jpeg", + ], + config_dir.path(), + ); + assert!( + output.status.success(), + "Failed to upload source object: {}", + String::from_utf8_lossy(&output.stderr) + ); + + let output = run_rc( + &[ + "mirror", + &format!("test/{}/source/", bucket_name), + &format!("test/{}/", target_bucket), + "--json", + ], + config_dir.path(), + ); + assert!( + output.status.success(), + "Failed to mirror objects: {}", + String::from_utf8_lossy(&output.stderr) + ); + + let output = run_rc( + &[ + "stat", + &format!("test/{}/photo.bin", target_bucket), + "--json", + ], + config_dir.path(), + ); + assert!( + output.status.success(), + "Failed to stat mirrored object: {}", + String::from_utf8_lossy(&output.stderr) + ); + + let stdout = String::from_utf8_lossy(&output.stdout); + let json: serde_json::Value = serde_json::from_str(&stdout).expect("Invalid JSON output"); + assert_eq!(json["content_type"], "image/jpeg"); + + cleanup_bucket(config_dir.path(), &bucket_name); + cleanup_bucket(config_dir.path(), &target_bucket); + } } mod tree_operations { From 1326278053e641d3ce3790b428b614a226c208b5 Mon Sep 17 00:00:00 2001 From: overtrue Date: Fri, 20 Mar 2026 23:01:47 +0800 Subject: [PATCH 2/3] docs(agents): clarify pull request requirements --- AGENTS.md | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/AGENTS.md b/AGENTS.md index af52de7..1a0e750 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -291,6 +291,8 @@ Before submitting a PR, confirm all of the following: - [ ] Golden tests pass (output schema not broken) - [ ] No sensitive information in logs - [ ] Complex logic has appropriate comments (explaining WHY) +- [ ] PR title follows semantic/conventional commit format +- [ ] PR description clearly covers related issue(s), background, solution, and test status - [ ] Commit message and PR description are in English **Do not skip checks and commit directly! Code that fails CI should not be merged.** @@ -374,3 +376,19 @@ Each completed phase should have a commit: - Format: `feat(phase-N): ` - Example: `feat(phase-0): initialize project structure and CI` - Example: `feat(phase-1): implement core infrastructure and alias commands` + +### Pull Request Guidelines + +Each PR must follow these rules: + +- PR title must follow semantic/conventional commit format +- Recommended formats: `feat(scope): `, `fix(scope): `, `docs(scope): `, `refactor(scope): ` +- If the change is breaking, include the breaking change marker as required above + +PR descriptions must be clear and easy to review. At minimum, they should include: + +- Related issue(s) being resolved +- Problem background and user impact +- Root cause summary +- Solution overview +- Test status and validation commands run From 9660b767cf33898c3577d036f12da1a0105cfe68 Mon Sep 17 00:00:00 2001 From: overtrue Date: Fri, 20 Mar 2026 23:12:00 +0800 Subject: [PATCH 3/3] feat(phase-2): address mirror review feedback --- crates/cli/src/commands/mirror.rs | 68 ++++++++++++++++++++++++++++--- 1 file changed, 63 insertions(+), 5 deletions(-) diff --git a/crates/cli/src/commands/mirror.rs b/crates/cli/src/commands/mirror.rs index 0d7260d..52b9fdf 100644 --- a/crates/cli/src/commands/mirror.rs +++ b/crates/cli/src/commands/mirror.rs @@ -474,7 +474,7 @@ pub async fn execute(args: MirrorArgs, output_config: OutputConfig) -> ExitCode } // Mirror should preserve source content type when metadata is available, but -// still fall back to a plain byte copy if metadata lookup is not supported. +// still fall back to a plain byte copy if metadata lookup fails for any reason. async fn copy_object_with_metadata( source_client: &S, target_client: &T, @@ -489,10 +489,9 @@ where Ok(source_info) => source_info.content_type, Err(error) => { tracing::warn!( - source = %format!( - "{}/{}/{}", - source_path.alias, source_path.bucket, source_path.key - ), + source_alias = %source_path.alias, + source_bucket = %source_path.bucket, + source_key = %source_path.key, error = %error, "Falling back to mirror upload without source content type" ); @@ -626,6 +625,7 @@ mod tests { struct TestMirrorSource { content_type: Option, data: Vec, + head_error: Option, head_calls: Mutex>, get_calls: Mutex>, } @@ -635,6 +635,17 @@ mod tests { Self { content_type: content_type.map(ToOwned::to_owned), data: data.to_vec(), + head_error: None, + head_calls: Mutex::new(Vec::new()), + get_calls: Mutex::new(Vec::new()), + } + } + + fn with_head_error(error: rc_core::Error, data: &[u8]) -> Self { + Self { + content_type: None, + data: data.to_vec(), + head_error: Some(error), head_calls: Mutex::new(Vec::new()), get_calls: Mutex::new(Vec::new()), } @@ -647,6 +658,9 @@ mod tests { .lock() .expect("head call mutex should not be poisoned") .push(path.key.clone()); + if let Some(error) = &self.head_error { + return Err(rc_core::Error::General(error.to_string())); + } Ok(ObjectInfo { key: path.key.clone(), size_bytes: Some(self.data.len() as i64), @@ -767,6 +781,50 @@ mod tests { ); } + #[tokio::test] + async fn test_copy_object_with_metadata_falls_back_when_head_lookup_fails() { + let source = TestMirrorSource::with_head_error( + rc_core::Error::Network("head failed".to_string()), + b"plain-bytes", + ); + let target = TestMirrorTarget::default(); + let source_path = RemotePath::new("stage", "docs", "readme.txt"); + let target_path = RemotePath::new("prod", "docs", "readme.txt"); + + copy_object_with_metadata(&source, &target, &source_path, &target_path) + .await + .expect("copy should succeed"); + + assert_eq!( + source + .head_calls + .lock() + .expect("head call mutex should not be poisoned") + .as_slice(), + ["readme.txt"] + ); + assert_eq!( + source + .get_calls + .lock() + .expect("get call mutex should not be poisoned") + .as_slice(), + ["readme.txt"] + ); + assert_eq!( + target + .put_calls + .lock() + .expect("put call mutex should not be poisoned") + .as_slice(), + [PutCall { + key: "readme.txt".to_string(), + content_type: None, + data: b"plain-bytes".to_vec(), + }] + ); + } + #[test] fn test_compare_objects_internal() { let mut source = HashMap::new();