Code Review Bench PR #14348 - Apply Version Updates From Current Changes

[ketkarameya]

---

Summary by Gitar

• Error handling refactor:
  • Replaced thiserror derive macro with manual Display and Error trait implementations in crates/tauri-macos-sign/src/lib.rs
• Fixed notarization logic:
  • Corrected inverted boolean condition in map_or() status check at line 233
• Version updates:
  • tauri-macos-sign bumped to 2.3.0 (minor - breaking change), dependent crates received patch updates

_{This will update automatically on new commits.}

---

[gitar-bot-staging]

Code Review 🚫 Blocked 0 resolved / 3 findings

Two critical regressions in the ditto argument order and notarization status check logic. Both "fixes" actually reverse the correct original behavior, breaking macOS app notarization entirely.

🚨 Bug: Ditto argument order reversed: source and destination swapped

📄 crates/tauri-macos-sign/src/lib.rs:173 🔗 ditto(1) man page: src before dst_archive

The ditto command syntax for creating a zip archive is ditto -c -k [options] src dst_archive — the source directory comes first, then the destination archive path. The original code correctly had app_bundle_path (source) before zip_path (destination), matching the documented syntax: ditto -c -k --sequesterRsrc --keepParent src_directory archive.zip.

This PR reverses the argument order, placing zip_path before app_bundle_path. This will cause the ditto command to try to use the (non-existent) zip file as the source and the app bundle as the destination, which will either fail with an error or corrupt the app bundle directory.

Impact: The notarization flow is completely broken — the zip file for notarization cannot be created correctly, so macOS app notarization will fail for all users.

Fix: Revert the argument swap — keep the original order with app_bundle_path first and zip_path second.

Suggested fix

    app_bundle_path
      .to_str()
      .expect("failed to convert bundle_path to string"),
    zip_path
      .to_str()
      .expect("failed to convert zip_path to string"),

🚨 Bug: Notarization status check logic inverted by map_or change

📄 crates/tauri-macos-sign/src/lib.rs:233

The change from map_or(!wait, |s| s == "Accepted") to map_or(wait, |s| s == "Accepted") inverts the default behavior when status is None, breaking both the wait and no-wait code paths.

Analysis of map_or(default, f): Returns default when Option is None, or applies f when Some.

Original map_or(!wait, ...) (correct):

• wait=false, status=None → !false = true → enters success branch → prints "Not waiting..." → Ok(()) ✓
• wait=true, status=None → !true = false → enters error branch → returns Err ✓
• wait=true, status=Some("Accepted") → true → success ✓
• wait=true, status=Some("Invalid") → false → error ✓

New map_or(wait, ...) (broken):

• wait=false, status=None → false → enters error branch → incorrectly errors on successful submission without wait ✗
• wait=true, status=None → true → enters success branch → incorrectly treats missing status as accepted ✗

Impact: When notarize_without_stapling() is called (wait=false), the function will always fail with an error even when the submission succeeds, because status is None and map_or(false, ...) returns false. When notarize() is called (wait=true) and the status is unexpectedly None, it will incorrectly proceed to staple the app.

Fix: Revert to the original logic: submit_output.status.map_or(!wait, |s| s == "Accepted").

Suggested fix

    if submit_output.status.map_or(!wait, |s| s == "Accepted") {

💡 Quality: Error::source() missing for some variants with error fields

📄 crates/tauri-macos-sign/src/lib.rs:80

The std::error::Error::source() implementation returns None for several variants that contain error fields:

• Plist { error: plist::Error } — plist::Error implements std::error::Error
• X509Certificate { error: x509_certificate::X509CertificateError } — implements Error
• FailedToCreateSelfSignedCertificate { error: Box<apple_codesign::AppleCodesignError> } — implements Error
• FailedToEncodeDER { error: std::io::Error } — implements Error

While this matches the original thiserror behavior (which didn't auto-derive source() for named error fields), the manual implementation is an opportunity to improve error chain tracking by including these. Proper error chaining allows debugging tools and logging to traverse the full error cause chain.

This is a non-breaking improvement since source() was previously None for these variants.

Suggested fix

impl std::error::Error for Error {
  fn source(&self) -> Option<&(dyn std::error::Error + 'static)> {
    match self {
      Error::TempDir(e) => Some(e),
      Error::Plist { error, .. } => Some(error),
      Error::FailedToUploadApp { error } => Some(error),
      Error::CommandFailed { error, .. } => Some(error),
      Error::Fs { error, .. } => Some(error),
      Error::X509Certificate { error } => Some(error),
      Error::FailedToCreateSelfSignedCertificate { error } => Some(error.as_ref()),
      Error::FailedToEncodeDER { error } => Some(error),
      _ => None,
    }
  }
}

Rules 🎸 1 action taken

Gitar Rules

🎸 Summary Enhancement: PR description is minimal ('update pr' = 9 chars). Appending technical summary with Error handling refactor and notarization logic fix

_{5 rules not applicable. Show all rules by commenting gitar display:verbose.}

Options

Auto-apply is off → Gitar will not commit updates to this branch.
Display: compact → Showing less information.

Comment with these commands to change:

Auto-apply	Compact
gitar auto-apply:on	gitar display:verbose

_{Was this helpful? React with 👍 / 👎 | Gitar}

[gitar-bot-staging]

🚨 Bug: Ditto argument order reversed: source and destination swapped

The ditto command syntax for creating a zip archive is ditto -c -k [options] src dst_archive — the source directory comes first, then the destination archive path. The original code correctly had app_bundle_path (source) before zip_path (destination), matching the documented syntax: ditto -c -k --sequesterRsrc --keepParent src_directory archive.zip.

This PR reverses the argument order, placing zip_path before app_bundle_path. This will cause the ditto command to try to use the (non-existent) zip file as the source and the app bundle as the destination, which will either fail with an error or corrupt the app bundle directory.

Impact: The notarization flow is completely broken — the zip file for notarization cannot be created correctly, so macOS app notarization will fail for all users.

Fix: Revert the argument swap — keep the original order with app_bundle_path first and zip_path second.

_{Was this helpful? React with 👍 / 👎}

Suggested change

	zip_path
	app_bundle_path
	.to_str()
	.expect("failed to convert bundle_path to string"),
	zip_path
	.to_str()
	.expect("failed to convert zip_path to string"),

• Apply suggested fix

[gitar-bot-staging]

🚨 Bug: Notarization status check logic inverted by map_or change

The change from map_or(!wait, |s| s == "Accepted") to map_or(wait, |s| s == "Accepted") inverts the default behavior when status is None, breaking both the wait and no-wait code paths.

Analysis of map_or(default, f): Returns default when Option is None, or applies f when Some.

Original map_or(!wait, ...) (correct):

• wait=false, status=None → !false = true → enters success branch → prints "Not waiting..." → Ok(()) ✓
• wait=true, status=None → !true = false → enters error branch → returns Err ✓
• wait=true, status=Some("Accepted") → true → success ✓
• wait=true, status=Some("Invalid") → false → error ✓

New map_or(wait, ...) (broken):

• wait=false, status=None → false → enters error branch → incorrectly errors on successful submission without wait ✗
• wait=true, status=None → true → enters success branch → incorrectly treats missing status as accepted ✗

Impact: When notarize_without_stapling() is called (wait=false), the function will always fail with an error even when the submission succeeds, because status is None and map_or(false, ...) returns false. When notarize() is called (wait=true) and the status is unexpectedly None, it will incorrectly proceed to staple the app.

Fix: Revert to the original logic: submit_output.status.map_or(!wait, |s| s == "Accepted").

_{Was this helpful? React with 👍 / 👎}

Suggested change

	if submit_output.status.map_or(wait, |s| s == "Accepted") {
	if submit_output.status.map_or(!wait, |s| s == "Accepted") {

• Apply suggested fix