fix(cli): warn when a profile or flag overrides a set VOLTAGE_* variable

CHANGELOG.md

@@ -4,6 +4,7 @@ User-visible changes to `voltage`, newest first. Each pull request that changes
 
 ## Unreleased
 
+- A command that ignores a set `VOLTAGE_*` variable, because `--profile` or a flag such as `--wallet` takes precedence, says so on stderr with one `warning:` line per variable, even with `--quiet`. Before, only `voltage context` reported it, so the CLI and a tool that lets the variable win could act on different wallets in one shell. Stdout is unchanged. `auth import-key` and `profiles create`, which save what their flags name, do not warn about the flags they were given.
 - Filter `payments list` with `--check-only true|false`, and `wallets ledger list` with `--event-type adjustment`, from the refreshed API contract. Omit `--event-type` for payment ledger entries. Filters carry across pagination.
 - `webhooks test WEBHOOK_ID` without `--data` sends a test event, with a new `delivery_id` and a payload of type `test` and event `created`. Before, it failed with "This operation requires --data". `--data` still sends your own payload.
 - A list that returns every item in one response, such as `wallets list`, accepts `--all` and ignores it, because there are no more pages. Before, it rejected `--all`. `--all` still pages through lists that take `--limit`.

docs/development.md

@@ -133,7 +133,7 @@ build when they disagree. `docs/commands.md` is generated; regenerate it with
   API-key runs, the optional `VOLTAGE_API_KEY`, `VOLTAGE_ORGANIZATION_ID`,
   `VOLTAGE_ENVIRONMENT_ID`, and `VOLTAGE_WALLET_ID` from the repository's private `.env`
   (copy `.env.example`, fill it in, `chmod 600 .env`) without evaluating it as shell code,
-  keeps CLI state under `.work/staging/cli`, and refuses `--config-dir`, `--api-url`, and
+  keeps CLI state under `.work/staging/cli` through `VOLTAGE_CONFIG_DIR`, and refuses `--config-dir`, `--api-url`, and
   `--auth-url` so a staging command cannot target another deployment. A leading `--build`
   works as it does for `voltage-local`, but checks `.env` before it builds when a command
   follows; `--build` alone needs no `.env`. Browser login and

docs/guides/login-and-profiles.md

@@ -62,7 +62,12 @@ An API key cannot list organizations or environments, so with a key the CLI offe
 
 ## Profiles
 
-A profile bundles an organization, an environment, and a credential, and ignores the scope variables above and `VOLTAGE_API_KEY`; flags still override its scope for one command.
+A profile bundles an organization, an environment, and a credential, and ignores the scope variables above and `VOLTAGE_API_KEY`; flags still override its scope for one command. Other tools, such as the `voltage-api` agent skill's helper, read the same variables and let them win, so each command names on stderr every variable it ignores, even with `--quiet`:
+
+```console
+$ VOLTAGE_WALLET_ID=WALLET_ID voltage wallets list --profile staging
+warning: VOLTAGE_WALLET_ID is set but ignored because --profile staging is selected
+```
 
 ```sh
 voltage profiles create staging --org ORG_ID --env ENV_ID --account work

docs/reference/environment.md

@@ -1,6 +1,6 @@
 # Environment variables
 
-A flag beats a variable. A selected profile is a complete configuration, so `VOLTAGE_ORGANIZATION_ID`, `VOLTAGE_ENVIRONMENT_ID`, `VOLTAGE_WALLET_ID`, and `VOLTAGE_API_KEY` are ignored while one is selected; `--account` alone ignores only `VOLTAGE_API_KEY`. `voltage context` lists every variable that is set but ignored, and where each value it uses came from.
+A flag beats a variable. A selected profile is a complete configuration, so `VOLTAGE_ORGANIZATION_ID`, `VOLTAGE_ENVIRONMENT_ID`, `VOLTAGE_WALLET_ID`, and `VOLTAGE_API_KEY` are ignored while one is selected; `--account` alone ignores only `VOLTAGE_API_KEY`. Every command that reads the configuration prints one `warning:` line on stderr for each variable that is set but ignored, before it prompts or sends a request, even with `--quiet`; stdout is unchanged. `auth import-key` and `profiles create` save the scope and account their flags name, so they warn only about a profile or `--config-dir`. `voltage context` lists the same variables, and where each value it uses came from.
 
 | Variable | Used for | Flag that wins |
 |---|---|---|

scripts/voltage-local

@@ -24,8 +24,9 @@ if build:
     if status or not args:
         sys.exit(status)
 binary = root / 'target/debug/voltage'
+# A variable rather than --config-dir, so hints the CLI prints do not name a refused flag.
+env['VOLTAGE_CONFIG_DIR'] = str(root / '.work/local-demo/cli')
 if not binary.is_file():
     sys.exit('Build the development CLI first: ./scripts/voltage-local --build')
-os.execve(binary, [str(binary), '--config-dir', str(root / '.work/local-demo/cli'),
-                  '--auth-url', 'http://127.0.0.1:8081/api/v1',
+os.execve(binary, [str(binary), '--auth-url', 'http://127.0.0.1:8081/api/v1',
                   '--api-url', 'http://127.0.0.1:1/disabled-api', *args], env)

scripts/voltage-staging

@@ -100,6 +100,9 @@ if build:
 
 env = {key: value for key, value in os.environ.items() if not key.startswith("VOLTAGE_")}
 env.update({key: value for key, value in loaded.items() if key in CLI_ENV})
+# The directory is a variable rather than --config-dir, so hints the CLI prints, such as
+# the one after `profiles create`, do not name a flag this wrapper refuses.
+env["VOLTAGE_CONFIG_DIR"] = str(root / ".work/staging/cli")
 binary = root / "target/debug/voltage"
 if not binary.is_file():
     sys.exit("Build the development CLI first: ./scripts/voltage-staging --build")
@@ -108,8 +111,6 @@ os.execve(
     binary,
     [
         str(binary),
-        "--config-dir",
-        str(root / ".work/staging/cli"),
         "--auth-url",
         loaded["VOLTAGE_STAGING_AUTH_URL"],
         "--api-url",

src/auth.rs

@@ -10,8 +10,8 @@ use crate::{
     cli::{GlobalFlags, ImportKeyFlags, LoginFlags},
     config::{
         AUTH_URL, Account, AccountKind, Credential, CredentialProblem, CredentialStore,
-        InputSource, LOCK_TIMEOUT, Login, Scope, ScopeSources, Settings, Source, name_in_use,
-        read_secret, shell_word, unreadable_name,
+        IgnoredVariable, InputSource, LOCK_TIMEOUT, Login, Precedence, Scope, ScopeSources,
+        Settings, Source, name_in_use, read_secret, shell_word, unreadable_name,
     },
     error::{CredentialRejection, CredentialSources, ErrorDetail, ResponseLimit},
     secret::Secret,
@@ -576,9 +576,19 @@ pub(crate) fn environment_key_sources(
     })
 }
 
-/// `VOLTAGE_API_KEY` is set, but a profile or account selection takes precedence over it.
-pub(crate) fn ignores_ambient_api_key(flags: &GlobalFlags) -> bool {
-    std::env::var_os("VOLTAGE_API_KEY").is_some() && ambient_api_key(flags).is_none()
+/// `VOLTAGE_API_KEY` when it is set but a profile or account selection takes precedence.
+pub(crate) fn ignored_api_key(flags: &GlobalFlags) -> Option<IgnoredVariable> {
+    if std::env::var_os("VOLTAGE_API_KEY").is_none() || ambient_api_key(flags).is_some() {
+        return None;
+    }
+    Some(IgnoredVariable {
+        name: "VOLTAGE_API_KEY",
+        precedence: if flags.profile.is_some() {
+            Precedence::Profile
+        } else {
+            Precedence::Flag("--account")
+        },
+    })
 }
 
 /// The credential a command would use, chosen without reading a saved secret.

src/config.rs

@@ -162,18 +162,47 @@ impl CredentialProblem {
     }
 }
 
-/// `VOLTAGE_*` variables that are set but supply nothing, because a flag or the selected
-/// profile takes precedence. Kept beside the precedence rules that decide it.
-pub(crate) fn ignored_variables(sources: &ScopeSources, config_dir: Source) -> Vec<&'static str> {
+/// What takes precedence over a `VOLTAGE_*` variable that is set.
+#[derive(Clone, Copy, Debug, Eq, PartialEq)]
+pub(crate) enum Precedence {
+    /// The named flag supplies the value.
+    Flag(&'static str),
+    /// The selected profile is a complete configuration, so ambient variables supply nothing,
+    /// even a value the profile lacks.
+    Profile,
+}
+
+/// A `VOLTAGE_*` variable that is set but supplies nothing.
+#[derive(Clone, Copy, Debug, Eq, PartialEq)]
+pub(crate) struct IgnoredVariable {
+    pub(crate) name: &'static str,
+    pub(crate) precedence: Precedence,
+}
+
+/// `VOLTAGE_*` scope and directory variables that are set but supply nothing, because a flag
+/// or the selected profile takes precedence. Kept beside the precedence rules that decide it.
+pub(crate) fn ignored_variables(
+    sources: &ScopeSources,
+    config_dir: Source,
+) -> Vec<IgnoredVariable> {
     [
-        ("VOLTAGE_ORGANIZATION_ID", sources.org),
-        ("VOLTAGE_ENVIRONMENT_ID", sources.envs),
-        ("VOLTAGE_WALLET_ID", sources.wallet),
-        ("VOLTAGE_CONFIG_DIR", config_dir),
+        ("VOLTAGE_ORGANIZATION_ID", "--org", sources.org),
+        ("VOLTAGE_ENVIRONMENT_ID", "--env", sources.envs),
+        ("VOLTAGE_WALLET_ID", "--wallet", sources.wallet),
+        ("VOLTAGE_CONFIG_DIR", "--config-dir", config_dir),
     ]
     .into_iter()
-    .filter(|(name, source)| std::env::var_os(name).is_some() && *source != Source::Environment)
-    .map(|(name, _)| name)
+    .filter(|(name, _, source)| std::env::var_os(name).is_some() && *source != Source::Environment)
+    .map(|(name, flag, source)| IgnoredVariable {
+        name,
+        // A set variable loses only to its flag or to a profile, which leaves it `Profile`
+        // or, for a value the profile lacks, `Unset`.
+        precedence: if source == Source::Flag {
+            Precedence::Flag(flag)
+        } else {
+            Precedence::Profile
+        },
+    })
     .collect()
 }
 

src/context.rs

@@ -9,8 +9,10 @@ use crate::{
     auth::{self, Rejected, Selected},
     cli::GlobalFlags,
     config::{
-        self, API_URL, AUTH_URL, AccountKind, CredentialProblem, CredentialStore, Settings, Source,
+        self, API_URL, AUTH_URL, AccountKind, CredentialProblem, CredentialStore, IgnoredVariable,
+        Precedence, ScopeSources, Settings, Source,
     },
+    style::{Line, Role},
 };
 use serde::Serialize;
 use uuid::Uuid;
@@ -98,14 +100,52 @@ pub struct Context {
     ignored_variables: Vec<&'static str>,
 }
 
+/// Every `VOLTAGE_*` variable that is set but supplies nothing, because a flag or the selected
+/// profile takes precedence. Commands warn with this list and `voltage context` reports it, so
+/// the two cannot disagree.
+pub(crate) fn ignored_variables(
+    global: &GlobalFlags,
+    sources: &ScopeSources,
+    config_dir: Source,
+) -> Vec<IgnoredVariable> {
+    let mut ignored = config::ignored_variables(sources, config_dir);
+    ignored.extend(auth::ignored_api_key(global));
+    ignored
+}
+
+/// Warn on stderr about each ignored variable. Another tool can read the same variables with
+/// the opposite precedence, so a silent override can send two tools in one shell to different
+/// wallets. `--quiet` keeps the warning for that reason; stdout is unchanged.
+pub(crate) fn warn_ignored(global: &GlobalFlags, ignored: &[IgnoredVariable]) {
+    let terminal = global.terminal();
+    for variable in ignored {
+        terminal.important_line(ignored_warning(variable, global.profile.as_deref()));
+    }
+}
+
+/// `warning: VOLTAGE_WALLET_ID is set but ignored because --profile staging is selected`.
+fn ignored_warning(variable: &IgnoredVariable, profile: Option<&str>) -> Line {
+    let line = Line::default()
+        .paint(Role::Caution, "warning:")
+        .text(&format!(" {} is set but ignored because ", variable.name));
+    match (variable.precedence, profile) {
+        (Precedence::Profile, Some(profile)) => {
+            line.text("--profile ").text(profile).text(" is selected")
+        }
+        // Only a selected profile yields `Profile`; this keeps the sentence whole regardless.
+        (Precedence::Profile, None) => line.text("a profile is selected"),
+        (Precedence::Flag(flag), _) => line.text(flag).text(" is given"),
+    }
+}
+
 /// Resolve the context with the same precedence every command uses.
 pub fn inspect(settings: &Settings, global: &GlobalFlags) -> Result<Context> {
     let (scope, sources) = settings.sourced_scope(global.scope_selection())?;
     let (_, config_dir_source) = config::sourced_directory(global.config_dir.clone())?;
-    let mut ignored_variables = config::ignored_variables(&sources, config_dir_source);
-    if auth::ignores_ambient_api_key(global) {
-        ignored_variables.push("VOLTAGE_API_KEY");
-    }
+    let ignored_variables = ignored_variables(global, &sources, config_dir_source)
+        .into_iter()
+        .map(|ignored| ignored.name)
+        .collect();
     let (selected, credential_problem) = match auth::select(settings, &scope, global) {
         Ok(selected) => (Some(selected), None),
         Err(Rejected { problem, selected }) => (selected, Some(problem)),
@@ -208,3 +248,53 @@ pub fn inspect(settings: &Settings, global: &GlobalFlags) -> Result<Context> {
         ignored_variables,
     })
 }
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    fn shown(name: &'static str, precedence: Precedence, profile: Option<&str>) -> String {
+        let ignored = IgnoredVariable { name, precedence };
+        anstream::adapter::strip_str(ignored_warning(&ignored, profile).as_str()).to_string()
+    }
+
+    #[test]
+    fn a_warning_names_the_variable_and_what_took_its_place() {
+        assert_eq!(
+            shown("VOLTAGE_WALLET_ID", Precedence::Profile, Some("staging")),
+            "warning: VOLTAGE_WALLET_ID is set but ignored because --profile staging is selected"
+        );
+        assert_eq!(
+            shown("VOLTAGE_WALLET_ID", Precedence::Flag("--wallet"), None),
+            "warning: VOLTAGE_WALLET_ID is set but ignored because --wallet is given"
+        );
+        assert_eq!(
+            shown("VOLTAGE_API_KEY", Precedence::Flag("--account"), None),
+            "warning: VOLTAGE_API_KEY is set but ignored because --account is given"
+        );
+    }
+
+    #[test]
+    fn a_profile_warning_without_a_profile_name_stays_a_sentence() {
+        assert_eq!(
+            shown("VOLTAGE_API_KEY", Precedence::Profile, None),
+            "warning: VOLTAGE_API_KEY is set but ignored because a profile is selected"
+        );
+    }
+
+    #[test]
+    fn a_profile_name_cannot_write_control_characters_to_the_terminal() {
+        let ignored = IgnoredVariable {
+            name: "VOLTAGE_WALLET_ID",
+            precedence: Precedence::Profile,
+        };
+        // Unstripped, so an escape the name smuggled in would still be here.
+        let line = ignored_warning(&ignored, Some("evil\u{1b}]0;x\u{7}"));
+        assert!(
+            line.as_str()
+                .ends_with(" is set but ignored because --profile evil ]0;x  is selected"),
+            "{:?}",
+            line.as_str()
+        );
+    }
+}

src/startup.rs

@@ -8,7 +8,7 @@ use crate::{
         self, AuthCommand, Command, ConvertFlags, GlobalFlags, Invocation, LocalCommand,
         ParseFailure, PriceFlags, ProfileCommand,
     },
-    config::{self, Profile, Scope, Settings},
+    config::{self, Precedence, Profile, Scope, Settings},
     context,
     error::{CredentialSources, ErrorKind},
     greeting,
@@ -158,6 +158,22 @@ async fn execute(invocation: Invocation, submission: &SubmissionState) -> Result
     }
 }
 
+/// Commands that save the organization, environment, and account their flags name. A flag
+/// over a set variable is what such a command is for, so only the configuration directory,
+/// which decides where it saves, still warns.
+fn saves_selection(command: &Command) -> bool {
+    matches!(
+        command,
+        Command::Local(
+            LocalCommand::Auth {
+                command: AuthCommand::ImportKey(_),
+            } | LocalCommand::Profiles {
+                command: ProfileCommand::Create { .. },
+            }
+        )
+    )
+}
+
 /// One command once its configuration is read and its scope is complete: what the flags
 /// asked for, the saved settings, the scope, and what the command may have changed.
 /// Commands run from here; what they call receives only the parts it uses.
@@ -177,8 +193,20 @@ impl<'a> Session<'a> {
         global: &'a GlobalFlags,
         submission: &'a SubmissionState,
     ) -> Result<Self> {
-        let settings = Settings::open(config::directory(global.config_dir.clone())?)?;
+        let (dir, dir_source) = config::sourced_directory(global.config_dir.clone())?;
+        let settings = Settings::open(dir)?;
         let (mut scope, sources) = settings.sourced_scope(global.scope_selection())?;
+        let mut ignored = context::ignored_variables(global, &sources, dir_source);
+        if saves_selection(command) {
+            ignored.retain(|variable| {
+                matches!(
+                    variable.precedence,
+                    Precedence::Profile | Precedence::Flag("--config-dir")
+                )
+            });
+        }
+        // Before any prompt or request, so the warning precedes what the command does.
+        context::warn_ignored(global, &ignored);
         let credential_sources = auth::environment_key_sources(global, &sources);
         let resolver = Resolver {
             settings: &settings,