feat(cli): read settings under a prefix mise does not strip

cli/src/cli/shell.rs

@@ -96,8 +96,10 @@ impl Shell {
         let parsed = usage::parse::parse(&spec, &args)?;
         debug!("{parsed:?}");
 
-        let overridden = env::shell_program_override(shell, |key| env::var(key).ok());
-        let program = overridden.clone().unwrap_or_else(|| shell.to_string());
+        let overridden = env::shell_program_override_entry(shell, |key| env::var(key).ok());
+        let program = overridden
+            .as_ref()
+            .map_or_else(|| shell.to_string(), |(_, value)| value.clone());
         debug!("running {program}");
 
         let mut cmd = std::process::Command::new(&program);
@@ -116,12 +118,11 @@ impl Shell {
         env::apply_parsed_env(&mut cmd, &parsed.as_env());
 
         // Name the program: the bare io error says only "No such file or directory", which for
-        // an overridden shell gives no clue that the variable is what pointed here.
+        // an overridden shell gives no clue that the variable is what pointed here. And name the
+        // variable the user actually set — the legacy `USAGE_SHELL_*` is still read, so assuming
+        // the current spelling would send them to one they never touched.
         let mut child = cmd.spawn().map_err(|err| match &overridden {
-            Some(_) => miette::miette!(
-                "failed to run `{program}` (from ${}): {err}",
-                env::shell_var_name(shell)
-            ),
+            Some((key, _)) => miette::miette!("failed to run `{program}` (from ${key}): {err}"),
             None => miette::miette!("failed to run `{program}`: {err}"),
         })?;
         let result = child.wait().into_diagnostic()?;
@@ -217,7 +218,10 @@ mod tests {
     fn the_hint_names_the_script_and_the_override() {
         let hint = wsl_path_hint("bash", 127, r"C:\Users\me\script.sh").unwrap();
         assert!(hint.contains(r"C:\Users\me\script.sh"), "{hint}");
-        assert!(hint.contains("USAGE_SHELL_BASH"), "{hint}");
+        // The current spelling, since the hint is telling someone what to set. The legacy
+        // `USAGE_SHELL_BASH` is still read, but pointing them at it would send them to the one
+        // a mise task never receives.
+        assert!(hint.contains("USAGECLI_SHELL_BASH"), "{hint}");
         // Hedged, because a script really can exit 127 on its own.
         assert!(hint.contains("ignore this"), "{hint}");
     }

cli/src/env.rs

@@ -7,6 +7,82 @@ pub fn var_true(key: &str) -> bool {
     matches!(var(key), Ok(v) if v == "1" || v == "true")
 }
 
+/// The prefix this CLI's own settings live under.
+///
+/// Not `USAGE_`, which is the same namespace the parser writes a spec's values into
+/// (`usage_<arg>`). On Windows environment variable names are case-insensitive, so
+/// `USAGE_DEBUG` and a spec's `usage_debug` are one variable there, and two things follow:
+/// a script with an ordinary `--debug` flag cannot read `$usage_debug` when the setting is
+/// also set, and mise — which clears `usage_*` before a task so its own parsed arguments
+/// cannot leak in, comparing the first six characters case-insensitively — takes the
+/// settings with it. Six characters is why no `USAGE_…` spelling escapes: `USAGE_CLI_` and
+/// `USAGE_SETTING_` are cleared just the same.
+const SETTING_PREFIX: &str = "USAGECLI_";
+
+/// The prefix the settings used to live under, still read so nothing that set it breaks.
+const LEGACY_SETTING_PREFIX: &str = "USAGE_";
+
+/// The environment variable naming setting `name`, e.g. `SHELL_BASH` -> `USAGECLI_SHELL_BASH`.
+pub fn setting_var_name(name: &str) -> String {
+    format!("{SETTING_PREFIX}{name}")
+}
+
+/// The value of setting `name`, preferring the current spelling over the legacy one.
+///
+/// First one set wins and the rest are not looked at, which is what makes the old name an
+/// alias rather than a second setting — the same order `usage-config` gives `deprecated_envs`.
+/// Nothing is warned about: one name set is the ordinary case, and the settings read here are
+/// read before there is a logger to warn with.
+///
+/// An empty or blank value reads as unset at each name, matching the `FOO= cmd` convention for
+/// switching something off, so blanking the current name falls through to the legacy one
+/// rather than to nothing.
+pub fn setting(name: &str, lookup: impl Fn(&str) -> Option<String>) -> Option<String> {
+    setting_entry(name, lookup).map(|(_, value)| value)
+}
+
+/// As [`setting`], and also which spelling supplied it.
+///
+/// The name matters to anything that reports back: telling someone their `USAGECLI_SHELL_BASH`
+/// could not be started, when what they set was `USAGE_SHELL_BASH`, sends them looking at a
+/// variable they never touched.
+pub fn setting_entry(
+    name: &str,
+    lookup: impl Fn(&str) -> Option<String>,
+) -> Option<(String, String)> {
+    [
+        format!("{SETTING_PREFIX}{name}"),
+        format!("{LEGACY_SETTING_PREFIX}{name}"),
+    ]
+    .into_iter()
+    .find_map(|key| {
+        let value = lookup(&key)?;
+        let value = value.trim();
+        (!value.is_empty()).then(|| (key, value.to_string()))
+    })
+}
+
+/// The log filter to use, under either spelling of each setting.
+///
+/// Resolved rather than read straight by `env_logger`: `Env::filter_or` falls back to its
+/// default only when the variable is *unset*, so a blank `USAGECLI_LOG` would be taken as the
+/// filter instead of falling through to `USAGE_LOG` — which is the rule [`setting`] promises.
+///
+/// Precedence: trace over debug over an explicit level, and `info` if none of them says
+/// otherwise.
+pub fn log_filter(lookup: impl Fn(&str) -> Option<String>) -> String {
+    // By reference, so the caller's closure need not be `Copy` — `&F` is itself `Fn` when `F`
+    // is, which is what lets one lookup answer all three settings.
+    let on = |name: &str| matches!(setting(name, &lookup), Some(v) if v == "1" || v == "true");
+    if on("TRACE") {
+        return "trace".to_string();
+    }
+    if on("DEBUG") {
+        return "debug".to_string();
+    }
+    setting("LOG", &lookup).unwrap_or_else(|| "info".to_string())
+}
+
 /// Hand the parsed spec's variables to a command we are about to spawn.
 ///
 /// On Windows this is not just `Command::env`. The executable search order there puts the
@@ -107,9 +183,16 @@ pub fn append_to_wslenv<'a>(
 }
 
 /// Keyed by the *program* rather than the subcommand, because that is what the value names.
-/// `usage powershell` runs `pwsh`, so its variable is `USAGE_SHELL_PWSH`.
+/// `usage powershell` runs `pwsh`, so its variable is `USAGECLI_SHELL_PWSH`.
+///
+/// The current spelling, since this is what error messages tell people to set. The legacy
+/// `USAGE_SHELL_*` is still read — see [`setting`].
 pub fn shell_var_name(shell: &str) -> String {
-    format!("USAGE_SHELL_{}", shell.to_ascii_uppercase())
+    setting_var_name(&shell_setting_name(shell))
+}
+
+fn shell_setting_name(shell: &str) -> String {
+    format!("SHELL_{}", shell.to_ascii_uppercase())
 }
 
 /// The shell program to run in place of `shell`, if one was configured.
@@ -131,9 +214,15 @@ pub fn shell_program_override(
     shell: &str,
     lookup: impl Fn(&str) -> Option<String>,
 ) -> Option<String> {
-    let value = lookup(&shell_var_name(shell))?;
-    let value = value.trim();
-    (!value.is_empty()).then(|| value.to_string())
+    setting(&shell_setting_name(shell), lookup)
+}
+
+/// As [`shell_program_override`], and also the variable that named it.
+pub fn shell_program_override_entry(
+    shell: &str,
+    lookup: impl Fn(&str) -> Option<String>,
+) -> Option<(String, String)> {
+    setting_entry(&shell_setting_name(shell), lookup)
 }
 
 #[cfg(test)]
@@ -275,10 +364,24 @@ mod tests {
     #[test]
     fn shell_var_names_cover_every_shell_subcommand() {
         // These are the four programs `Cli::run` dispatches to; `powershell` runs `pwsh`.
-        assert_eq!(shell_var_name("bash"), "USAGE_SHELL_BASH");
-        assert_eq!(shell_var_name("zsh"), "USAGE_SHELL_ZSH");
-        assert_eq!(shell_var_name("fish"), "USAGE_SHELL_FISH");
-        assert_eq!(shell_var_name("pwsh"), "USAGE_SHELL_PWSH");
+        assert_eq!(shell_var_name("bash"), "USAGECLI_SHELL_BASH");
+        assert_eq!(shell_var_name("zsh"), "USAGECLI_SHELL_ZSH");
+        assert_eq!(shell_var_name("fish"), "USAGECLI_SHELL_FISH");
+        assert_eq!(shell_var_name("pwsh"), "USAGECLI_SHELL_PWSH");
+    }
+
+    #[test]
+    fn the_prefix_is_one_mise_does_not_clear() {
+        // mise clears a task's `usage_*` so its own parsed arguments cannot leak in, comparing
+        // the first six characters case-insensitively. Everything this CLI wants to survive
+        // that has to differ inside those six — which is the whole reason for the spelling.
+        for shell in ["bash", "zsh", "fish", "pwsh"] {
+            let name = shell_var_name(shell);
+            assert!(
+                !name[.."usage_".len()].eq_ignore_ascii_case("usage_"),
+                "{name} would be cleared before a mise task ran"
+            );
+        }
     }
 
     #[test]
@@ -320,5 +423,107 @@ mod tests {
             shell_program_override("bash", from(&[("USAGE_SHELL_ZSH", "/bin/zsh")])),
             None
         );
+        assert_eq!(
+            shell_program_override("bash", from(&[("USAGECLI_SHELL_ZSH", "/bin/zsh")])),
+            None
+        );
+    }
+
+    // The tests above read the legacy name, which is the point: they were written before the
+    // rename and still pass, so they are what says nothing that set it has broken.
+
+    #[test]
+    fn the_current_name_is_read() {
+        assert_eq!(
+            shell_program_override("bash", from(&[("USAGECLI_SHELL_BASH", "/usr/bin/bash")])),
+            Some("/usr/bin/bash".to_string())
+        );
+    }
+
+    #[test]
+    fn the_current_name_wins_over_the_legacy_one() {
+        assert_eq!(
+            shell_program_override(
+                "bash",
+                from(&[
+                    ("USAGECLI_SHELL_BASH", "/current"),
+                    ("USAGE_SHELL_BASH", "/legacy"),
+                ])
+            ),
+            Some("/current".to_string())
+        );
+    }
+
+    #[test]
+    fn a_blanked_current_name_falls_through_to_the_legacy_one() {
+        // Blank reads as unset at each name rather than at the setting, so `USAGECLI_X= ` does
+        // not switch off a legacy `USAGE_X` the way it switches off its own.
+        assert_eq!(
+            shell_program_override(
+                "bash",
+                from(&[
+                    ("USAGECLI_SHELL_BASH", "  "),
+                    ("USAGE_SHELL_BASH", "/legacy")
+                ])
+            ),
+            Some("/legacy".to_string())
+        );
+    }
+
+    #[test]
+    fn the_log_filter_falls_back_through_both_spellings() {
+        assert_eq!(log_filter(from(&[])), "info");
+        assert_eq!(log_filter(from(&[("USAGE_LOG", "warn")])), "warn");
+        assert_eq!(log_filter(from(&[("USAGECLI_LOG", "warn")])), "warn");
+        // A blank current name is unset at that name, so the legacy one still answers. This is
+        // what `Env::filter_or` could not do: its default applies only to an *unset* variable,
+        // so a blank `USAGECLI_LOG` would have been taken as the filter itself.
+        assert_eq!(
+            log_filter(from(&[("USAGECLI_LOG", "  "), ("USAGE_LOG", "warn")])),
+            "warn"
+        );
+    }
+
+    #[test]
+    fn the_log_filter_keeps_trace_over_debug_over_a_level() {
+        assert_eq!(
+            log_filter(from(&[("USAGE_DEBUG", "1"), ("USAGE_LOG", "warn")])),
+            "debug"
+        );
+        assert_eq!(
+            log_filter(from(&[("USAGECLI_TRACE", "true"), ("USAGE_DEBUG", "1")])),
+            "trace"
+        );
+        // Only `1` and `true` switch it on; anything else is not a level request.
+        assert_eq!(log_filter(from(&[("USAGECLI_DEBUG", "0")])), "info");
+    }
+
+    #[test]
+    fn a_failed_lookup_reports_the_name_that_answered() {
+        assert_eq!(
+            setting_entry("SHELL_BASH", from(&[("USAGE_SHELL_BASH", "/legacy")])),
+            Some(("USAGE_SHELL_BASH".to_string(), "/legacy".to_string()))
+        );
+        assert_eq!(
+            setting_entry("SHELL_BASH", from(&[("USAGECLI_SHELL_BASH", "/current")])),
+            Some(("USAGECLI_SHELL_BASH".to_string(), "/current".to_string()))
+        );
+    }
+
+    #[test]
+    fn settings_are_not_limited_to_shells() {
+        // The same resolution serves USAGECLI_DEBUG, _TRACE and _LOG, which `main` reads.
+        assert_eq!(
+            setting("DEBUG", from(&[("USAGE_DEBUG", "1")])),
+            Some("1".to_string())
+        );
+        assert_eq!(
+            setting(
+                "LOG",
+                from(&[("USAGECLI_LOG", "trace"), ("USAGE_LOG", "debug")])
+            ),
+            Some("trace".to_string())
+        );
+        assert_eq!(setting("TRACE", from(&[])), None);
     }
 }

cli/src/main.rs

@@ -1,22 +1,24 @@
-use env_logger::Env;
 use usage_cli::env;
 
 fn main() -> miette::Result<()> {
-    set_log_env_vars();
-    env_logger::builder()
+    // The filter is resolved by `env::log_filter` rather than named to `env_logger`, which falls
+    // back to its default only when a variable is *unset* — a blank `USAGECLI_LOG` would be
+    // taken as the filter rather than falling through to `USAGE_LOG`.
+    //
+    // Nothing is written back into the environment either. The old shape set `USAGE_LOG` with
+    // `set_var`, which a spawned script then inherited — and on Windows, where variable names
+    // are case-insensitive, that is the same variable a spec's own `log` argument writes, so
+    // usage was overwriting a value the script was about to read.
+    let mut builder = env_logger::builder();
+    builder
         .format_timestamp(None)
-        .parse_env(Env::default().filter_or("USAGE_LOG", "info"))
-        .init();
+        .parse_filters(&env::log_filter(|key| env::var(key).ok()));
+    // `Env::default()` used to bring this along; `parse_filters` covers only the filter.
+    if let Ok(style) = env::var(env_logger::DEFAULT_WRITE_STYLE_ENV) {
+        builder.parse_write_style(&style);
+    }
+    builder.init();
 
     let args: Vec<_> = env::args().collect();
     usage_cli::run(&args)
 }
-
-fn set_log_env_vars() {
-    if env::var_true("USAGE_DEBUG") {
-        env::set_var("USAGE_LOG", "debug");
-    }
-    if env::var_true("USAGE_TRACE") {
-        env::set_var("USAGE_LOG", "trace");
-    }
-}

cli/tests/shell_completions_integration.rs

@@ -4,19 +4,31 @@ use std::path::{Path, PathBuf};
 use std::process::{Command, Output};
 use std::sync::atomic::{AtomicUsize, Ordering};
 
-/// The program to run for `shell`, honouring the same `USAGE_SHELL_<SHELL>` override the CLI
-/// itself reads (`shell_program_override` in `cli/src/env.rs`).
+/// The program to run for `shell`, honouring the same `USAGECLI_SHELL_<SHELL>` override the CLI
+/// itself reads (`shell_program_override` in `cli/src/env.rs`), and the legacy
+/// `USAGE_SHELL_<SHELL>` behind it.
 ///
 /// It matters on Windows, where the executable search order puts the system directory ahead of
 /// `PATH` and installing WSL puts `bash.exe` there — so a bare `bash` is the WSL launcher,
 /// which cannot open the Windows paths these tests write. Pointing the variable at a real bash
 /// is what makes them runnable. Unset, which is every other platform, means the bare name.
+///
+/// The current spelling is also the one that survives `mise run`, which clears `usage_*` from a
+/// task's environment — so it is what lets these tests be driven by `mise r test` rather than by
+/// a bare `cargo test`.
 fn shell_program(shell: &str) -> String {
-    env::var(format!("USAGE_SHELL_{}", shell.to_ascii_uppercase()))
-        .ok()
-        .map(|value| value.trim().to_string())
-        .filter(|value| !value.is_empty())
-        .unwrap_or_else(|| shell.to_string())
+    let upper = shell.to_ascii_uppercase();
+    [
+        format!("USAGECLI_SHELL_{upper}"),
+        format!("USAGE_SHELL_{upper}"),
+    ]
+    .iter()
+    .find_map(|key| {
+        let value = env::var(key).ok()?;
+        let value = value.trim().to_string();
+        (!value.is_empty()).then_some(value)
+    })
+    .unwrap_or_else(|| shell.to_string())
 }
 
 /// A path as a POSIX shell will read it.