fix: Correct version mismatch causing noUpdateNotifier to fail

crates/turborepo-lib/src/shim/mod.rs

@@ -15,7 +15,7 @@ use thiserror::Error;
 use tiny_gradient::{GradientStr, RGB};
 use tracing::{debug, warn};
 pub use turbo_state::TurboState;
-use turbo_updater::display_update_check;
+use turbo_updater::{display_update_check, UpdateCheckConfig};
 use turbopath::AbsoluteSystemPathBuf;
 use turborepo_repository::{
     inference::{RepoMode, RepoState},
@@ -343,7 +343,7 @@ fn try_check_for_updates(
 ) {
     let package_manager = package_manager.unwrap_or(&PackageManager::Npm);
 
-    if args.should_check_for_update() && !config.no_update_notifier() {
+    if args.should_check_for_update() {
         // custom footer for update message
         let footer = format!(
             "Follow {username} for updates: {url}",
@@ -359,16 +359,17 @@ fn try_check_for_updates(
             None
         };
         // check for updates
-        let _ = display_update_check(
-            "turbo",
-            "https://github.com/vercel/turborepo",
-            Some(&footer),
+        let _ = display_update_check(UpdateCheckConfig {
+            package_name: "turbo",
+            github_repo: "https://github.com/vercel/turborepo",
+            footer: Some(&footer),
             current_version,
             // use default for timeout (800ms)
-            None,
+            timeout: None,
             interval,
             package_manager,
-        );
+            config_no_update: config.no_update_notifier(),
+        });
     }
 }
 

crates/turborepo-updater/src/lib.rs

@@ -83,34 +83,64 @@ fn get_tag_from_version(pre: &semver::Prerelease) -> VersionTag {
     }
 }
 
-fn should_skip_notification() -> bool {
-    NOTIFIER_DISABLE_VARS
+fn should_skip_notification(config_no_update: bool) -> bool {
+    if config_no_update {
+        return true;
+    }
+
+    if NOTIFIER_DISABLE_VARS
         .iter()
         .chain(ENVIRONMENTAL_DISABLE_VARS.iter())
         .any(|var| std::env::var(var).is_ok())
-        || !atty::is(atty::Stream::Stdout)
+    {
+        return true;
+    }
+
+    if !atty::is(atty::Stream::Stdout) {
+        return true;
+    }
+
+    false
 }
 
-pub fn display_update_check(
-    package_name: &str,
-    github_repo: &str,
-    footer: Option<&str>,
-    current_version: &str,
-    timeout: Option<Duration>,
-    interval: Option<Duration>,
-    package_manager: &PackageManager,
-) -> Result<(), UpdateNotifierError> {
+/// Configuration for the update check notification.
+#[derive(Debug)]
+pub struct UpdateCheckConfig<'a> {
+    /// The name of the package to check for updates.
+    pub package_name: &'a str,
+    /// The GitHub repository URL for the changelog link.
+    pub github_repo: &'a str,
+    /// Optional footer text to display below the update message.
+    pub footer: Option<&'a str>,
+    /// The current version of the package.
+    pub current_version: &'a str,
+    /// Timeout for the update check request. Defaults to 800ms.
+    pub timeout: Option<Duration>,
+    /// Interval between update checks. Defaults to 24 hours.
+    pub interval: Option<Duration>,
+    /// The package manager being used.
+    pub package_manager: &'a PackageManager,
+    /// Whether update notifications are disabled via config.
+    pub config_no_update: bool,
+}
+
+pub fn display_update_check(config: UpdateCheckConfig) -> Result<(), UpdateNotifierError> {
     // bail early if the user has disabled update notifications
-    if should_skip_notification() {
+    if should_skip_notification(config.config_no_update) {
         return Ok(());
     }
 
-    let version = check_for_updates(package_name, current_version, timeout, interval);
+    let version = check_for_updates(
+        config.package_name,
+        config.current_version,
+        config.timeout,
+        config.interval,
+    );
 
     if let Ok(Some(version)) = version {
         let latest_version = version.to_string();
 
-        let update_cmd = match package_manager {
+        let update_cmd = match config.package_manager {
             PackageManager::Npm => style("npx @turbo/codemod@latest update").cyan().bold(),
             PackageManager::Yarn | PackageManager::Berry => {
                 style("yarn dlx @turbo/codemod@latest update").cyan().bold()
@@ -128,13 +158,13 @@ pub fn display_update_check(
             Run \"{update_cmd}\" to update
             ",
             version_prefix = style("v").dim(),
-            current_version = style(current_version).dim(),
+            current_version = style(config.current_version).dim(),
             latest_version = style(latest_version).green().bold(),
-            github_repo = github_repo,
+            github_repo = config.github_repo,
             update_cmd = update_cmd
         );
 
-        if let Some(footer) = footer {
+        if let Some(footer) = config.footer {
             return ui::message(&format!("{msg}\n{footer}"));
         }
 
@@ -170,3 +200,247 @@ pub fn check_for_updates(
 
     Ok(data)
 }
+
+#[cfg(test)]
+mod tests {
+    use std::sync::Mutex;
+
+    use super::*;
+
+    // Mutex to ensure env var tests don't interfere with each other
+    static ENV_MUTEX: Mutex<()> = Mutex::new(());
+
+    // Helper to run tests with specific env vars set, then clean up
+    fn with_env_vars<F, R>(vars: &[(&str, Option<&str>)], f: F) -> R
+    where
+        F: FnOnce() -> R,
+    {
+        let _guard = ENV_MUTEX.lock().unwrap();
+
+        // Store original values and set new ones
+        let originals: Vec<_> = vars
+            .iter()
+            .map(|(key, value)| {
+                let original = std::env::var(key).ok();
+                unsafe {
+                    match value {
+                        Some(v) => std::env::set_var(key, v),
+                        None => std::env::remove_var(key),
+                    }
+                }
+                (*key, original)
+            })
+            .collect();
+
+        let result = f();
+
+        // Restore original values
+        for (key, original) in originals {
+            unsafe {
+                match original {
+                    Some(v) => std::env::set_var(key, v),
+                    None => std::env::remove_var(key),
+                }
+            }
+        }
+
+        result
+    }
+
+    #[test]
+    fn test_skip_notification_when_config_no_update_is_true() {
+        with_env_vars(&[("NO_UPDATE_NOTIFIER", None), ("CI", None)], || {
+            assert!(
+                should_skip_notification(true),
+                "should skip when config_no_update is true"
+            );
+        });
+    }
+
+    #[test]
+    fn test_skip_notification_when_no_update_notifier_env_set() {
+        with_env_vars(&[("NO_UPDATE_NOTIFIER", Some("1")), ("CI", None)], || {
+            assert!(
+                should_skip_notification(false),
+                "should skip when NO_UPDATE_NOTIFIER is set"
+            );
+        });
+    }
+
+    #[test]
+    fn test_skip_notification_when_no_update_notifier_env_empty() {
+        // Even an empty string means the var is set
+        with_env_vars(&[("NO_UPDATE_NOTIFIER", Some("")), ("CI", None)], || {
+            assert!(
+                should_skip_notification(false),
+                "should skip when NO_UPDATE_NOTIFIER is set (even if empty)"
+            );
+        });
+    }
+
+    #[test]
+    fn test_skip_notification_when_ci_env_set() {
+        with_env_vars(
+            &[("NO_UPDATE_NOTIFIER", None), ("CI", Some("true"))],
+            || {
+                assert!(
+                    should_skip_notification(false),
+                    "should skip when CI is set"
+                );
+            },
+        );
+    }
+
+    #[test]
+    fn test_skip_notification_when_ci_env_set_to_any_value() {
+        with_env_vars(&[("NO_UPDATE_NOTIFIER", None), ("CI", Some("1"))], || {
+            assert!(
+                should_skip_notification(false),
+                "should skip when CI is set to any value"
+            );
+        });
+    }
+
+    #[test]
+    fn test_skip_notification_when_both_env_vars_set() {
+        with_env_vars(
+            &[("NO_UPDATE_NOTIFIER", Some("1")), ("CI", Some("true"))],
+            || {
+                assert!(
+                    should_skip_notification(false),
+                    "should skip when both env vars are set"
+                );
+            },
+        );
+    }
+
+    #[test]
+    fn test_skip_notification_non_tty() {
+        // In test environment, stdout is typically not a TTY, so this tests the
+        // non-TTY path. When running in a non-TTY environment with no env vars,
+        // should still skip due to non-TTY.
+        with_env_vars(&[("NO_UPDATE_NOTIFIER", None), ("CI", None)], || {
+            let result = should_skip_notification(false);
+            let _ = result;
+        });
+    }
+
+    #[test]
+    fn test_get_tag_canary_prerelease() {
+        let pre = semver::Prerelease::new("canary.1").unwrap();
+        let tag = get_tag_from_version(&pre);
+        assert!(
+            matches!(tag, VersionTag::Canary),
+            "canary prerelease should return Canary tag"
+        );
+    }
+
+    #[test]
+    fn test_get_tag_canary_prerelease_with_suffix() {
+        let pre = semver::Prerelease::new("canary.123.abcdef").unwrap();
+        let tag = get_tag_from_version(&pre);
+        assert!(
+            matches!(tag, VersionTag::Canary),
+            "canary prerelease with suffix should return Canary tag"
+        );
+    }
+
+    #[test]
+    fn test_get_tag_empty_prerelease() {
+        let pre = semver::Prerelease::EMPTY;
+        let tag = get_tag_from_version(&pre);
+        assert!(
+            matches!(tag, VersionTag::Latest),
+            "empty prerelease should return Latest tag"
+        );
+    }
+
+    #[test]
+    fn test_get_tag_alpha_prerelease() {
+        let pre = semver::Prerelease::new("alpha.1").unwrap();
+        let tag = get_tag_from_version(&pre);
+        assert!(
+            matches!(tag, VersionTag::Latest),
+            "alpha prerelease should return Latest tag"
+        );
+    }
+
+    #[test]
+    fn test_get_tag_beta_prerelease() {
+        let pre = semver::Prerelease::new("beta.2").unwrap();
+        let tag = get_tag_from_version(&pre);
+        assert!(
+            matches!(tag, VersionTag::Latest),
+            "beta prerelease should return Latest tag"
+        );
+    }
+
+    #[test]
+    fn test_get_tag_rc_prerelease() {
+        let pre = semver::Prerelease::new("rc.1").unwrap();
+        let tag = get_tag_from_version(&pre);
+        assert!(
+            matches!(tag, VersionTag::Latest),
+            "rc prerelease should return Latest tag"
+        );
+    }
+
+    #[test]
+    fn test_version_tag_display_latest() {
+        let tag = VersionTag::Latest;
+        assert_eq!(tag.to_string(), "latest");
+    }
+
+    #[test]
+    fn test_version_tag_display_canary() {
+        let tag = VersionTag::Canary;
+        assert_eq!(tag.to_string(), "canary");
+    }
+
+    #[test]
+    fn test_version_parsing_stable() {
+        let version = SemVerVersion::parse("2.0.0").unwrap();
+        let tag = get_tag_from_version(&version.pre);
+        assert!(matches!(tag, VersionTag::Latest));
+    }
+
+    #[test]
+    fn test_version_parsing_canary() {
+        let version = SemVerVersion::parse("2.0.0-canary.1").unwrap();
+        let tag = get_tag_from_version(&version.pre);
+        assert!(matches!(tag, VersionTag::Canary));
+    }
+
+    #[test]
+    fn test_version_parsing_complex_canary() {
+        let version = SemVerVersion::parse("2.1.0-canary.20231201.abc123").unwrap();
+        let tag = get_tag_from_version(&version.pre);
+        assert!(matches!(tag, VersionTag::Canary));
+    }
+
+    #[test]
+    fn test_notifier_disable_vars_contains_no_update_notifier() {
+        assert!(
+            NOTIFIER_DISABLE_VARS.contains(&"NO_UPDATE_NOTIFIER"),
+            "NOTIFIER_DISABLE_VARS should contain NO_UPDATE_NOTIFIER"
+        );
+    }
+
+    #[test]
+    fn test_environmental_disable_vars_contains_ci() {
+        assert!(
+            ENVIRONMENTAL_DISABLE_VARS.contains(&"CI"),
+            "ENVIRONMENTAL_DISABLE_VARS should contain CI"
+        );
+    }
+
+    #[test]
+    fn test_default_timeout_is_reasonable() {
+        assert_eq!(DEFAULT_TIMEOUT, Duration::from_millis(800));
+    }
+
+    #[test]
+    fn test_default_interval_is_one_day() {
+        assert_eq!(DEFAULT_INTERVAL, Duration::from_secs(60 * 60 * 24));
+    }
+}