diff --git a/CHANGELOG.md b/CHANGELOG.md index 39995ce..d03f6e0 100644 --- a/CHANGELOG.md +++ b/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`. diff --git a/docs/development.md b/docs/development.md index d7aca10..452e68e 100644 --- a/docs/development.md +++ b/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 diff --git a/docs/guides/login-and-profiles.md b/docs/guides/login-and-profiles.md index cfeeeb0..cebe279 100644 --- a/docs/guides/login-and-profiles.md +++ b/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 diff --git a/docs/reference/environment.md b/docs/reference/environment.md index 375d3a4..840f2b6 100644 --- a/docs/reference/environment.md +++ b/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 | |---|---|---| diff --git a/scripts/voltage-local b/scripts/voltage-local index 6d8395a..8ab33d8 100755 --- a/scripts/voltage-local +++ b/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) diff --git a/scripts/voltage-staging b/scripts/voltage-staging index f2ae052..d0387a5 100755 --- a/scripts/voltage-staging +++ b/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", diff --git a/src/auth.rs b/src/auth.rs index b053228..468514c 100644 --- a/src/auth.rs +++ b/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 { + 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. diff --git a/src/config.rs b/src/config.rs index 1887d2e..b49cf0c 100644 --- a/src/config.rs +++ b/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 { [ - ("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() } diff --git a/src/context.rs b/src/context.rs index ff95348..d8244db 100644 --- a/src/context.rs +++ b/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 { + 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 { 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 { 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() + ); + } +} diff --git a/src/startup.rs b/src/startup.rs index ab3a316..8ba36b3 100644 --- a/src/startup.rs +++ b/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 { - 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, diff --git a/tests/cli_integration.rs b/tests/cli_integration.rs index 53c22e4..0cfcbd9 100644 --- a/tests/cli_integration.rs +++ b/tests/cli_integration.rs @@ -934,12 +934,20 @@ async fn profile_credentials_override_ambient_key_and_scope() { ok(json!([])), ) .await; - cli(dir.path(), &server) + let profiled = cli(dir.path(), &server) .env("VOLTAGE_ORGANIZATION_ID", RESOURCE) .env("VOLTAGE_ENVIRONMENT_ID", RESOURCE) - .args(["wallets", "list", "--profile", "stage"]) + .args(["wallets", "list", "--profile", "stage", "--quiet"]) .assert() .success(); + // Each ignored variable is named on stderr, even under --quiet; stdout stays the result. + assert_eq!( + stderr(&profiled), + "warning: VOLTAGE_ORGANIZATION_ID is set but ignored because --profile stage is selected\n\ + warning: VOLTAGE_ENVIRONMENT_ID is set but ignored because --profile stage is selected\n\ + warning: VOLTAGE_API_KEY is set but ignored because --profile stage is selected\n" + ); + assert_eq!(json_stdout(&profiled)["data"], json!([])); cli(dir.path(), &server) .args(["wallets", "list", "--profile", "stage", "--env", RESOURCE]) .assert() @@ -947,6 +955,34 @@ async fn profile_credentials_override_ambient_key_and_scope() { server.verify().await; } +#[tokio::test] +async fn a_flag_over_a_set_variable_warns_and_a_used_variable_does_not() { + let (server, dir) = fixture().await; + respond( + &server, + route("GET", wallets_path(ORG)).and(header("x-api-key", ACCOUNT_KEY)), + ok(json!([])), + 2, + ) + .await; + let flagged = cli(dir.path(), &server) + .env("VOLTAGE_ORGANIZATION_ID", RESOURCE) + .args(["wallets", "list", "--org", ORG]) + .assert() + .success(); + assert_eq!( + stderr(&flagged), + "warning: VOLTAGE_ORGANIZATION_ID is set but ignored because --org is given\n" + ); + let ambient = cli(dir.path(), &server) + .env("VOLTAGE_ORGANIZATION_ID", ORG) + .args(["wallets", "list"]) + .assert() + .success(); + assert_eq!(stderr(&ambient), ""); + server.verify().await; +} + #[tokio::test(flavor = "multi_thread")] async fn profiles_are_created_from_scope_and_listed_without_ambient_state() { let (server, dir) = fixture().await; @@ -964,6 +1000,8 @@ async fn profiles_are_created_from_scope_and_listed_without_ambient_state() { ]; let created = cli(dir.path(), &server) .env_remove("VOLTAGE_API_KEY") + .env("VOLTAGE_ORGANIZATION_ID", RESOURCE) + .env("VOLTAGE_ENVIRONMENT_ID", RESOURCE) .args(create) .assert() .success(); @@ -971,6 +1009,8 @@ async fn profiles_are_created_from_scope_and_listed_without_ambient_state() { json_stdout(&created)["data"], json!({"profile":"stage","created":true}) ); + // The flags name what the profile saves, so the variables they override are no surprise. + assert_eq!(stderr(&created), ""); cli(dir.path(), &server) .env_remove("VOLTAGE_API_KEY") .args(create) @@ -1302,7 +1342,8 @@ async fn imported_keys_bind_their_scope_and_report_status_without_secrets() { ENV, ]; let imported = cli(dir.path(), &server) - .env_remove("VOLTAGE_API_KEY") + .env("VOLTAGE_ORGANIZATION_ID", RESOURCE) + .env("VOLTAGE_ENVIRONMENT_ID", RESOURCE) .args(import) .args(["--credential-store", "file"]) .write_stdin(" imported-secret \n") @@ -1312,6 +1353,8 @@ async fn imported_keys_bind_their_scope_and_report_status_without_secrets() { json_stdout(&imported)["data"], json!({"account":"staging-key","imported":true}) ); + // The flags name what the key is bound to, so the variables they override are no surprise. + assert_eq!(stderr(&imported), ""); let settings = Settings::open(dir.path().to_path_buf()).unwrap(); let account = &settings.config.accounts["staging-key"]; assert_eq!(account.kind, AccountKind::ApiKey);