From 3bbdf0685ce4c05a207a5f1f294ae13b443e8cef Mon Sep 17 00:00:00 2001 From: Jacob Page Date: Wed, 17 Jun 2026 10:43:07 -0700 Subject: [PATCH 01/31] =?UTF-8?q?chore:=20baseline=20=E2=80=94=20per-env?= =?UTF-8?q?=20OAuth,=20consistent=20User-Agent,=20token=20timeout,=20env?= =?UTF-8?q?=20design+plan?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Snapshot of in-progress work before the engine-environments plan reshapes it: - PkceAuthProvider per-env OAuth (with_environment/OAuthEnvironment) — to be replaced by the Environments resolver per the plan - consistent outbound User-Agent (auto-wired from CliConfig; token traffic too) - default 30s token-endpoint timeout + shared reqwest client - design spec (docs/specs) and implementation plan (docs/plans) Co-Authored-By: Claude Opus 4.8 --- .../2026-06-17-engine-environments-plan.md | 1430 +++++++++++++++++ .../2026-06-17-engine-environments-design.md | 198 +++ src/auth/pkce.rs | 337 +++- src/cli.rs | 91 ++ src/transport/client.rs | 13 +- src/transport/injector.rs | 4 + tests/foundation.rs | 15 + 7 files changed, 2059 insertions(+), 29 deletions(-) create mode 100644 docs/plans/2026-06-17-engine-environments-plan.md create mode 100644 docs/specs/2026-06-17-engine-environments-design.md diff --git a/docs/plans/2026-06-17-engine-environments-plan.md b/docs/plans/2026-06-17-engine-environments-plan.md new file mode 100644 index 0000000..acf623f --- /dev/null +++ b/docs/plans/2026-06-17-engine-environments-plan.md @@ -0,0 +1,1430 @@ +# Engine Environments Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** Make environments a first-class `cli-engine` concept — definitions, layered resolution, sticky selection, an `env` command group, and per-environment OAuth — so consumers stop hand-rolling it. + +**Architecture:** A new engine-owned `environments` module provides an `Environments` value (compiled defaults + `environments.toml` + `_*` env-var overrides, later wins) resolving to an `Environment { name, oauth, extra }`. `CliConfig::with_environments` registers it: it seeds the existing `Middleware.env`, registers a global `--env` flag, and auto-mounts an `env list/get/set/info` group (mirroring the built-in `auth`/`config` groups). `PkceAuthProvider::with_environments` makes the environment the single source of OAuth config, replacing this work stream's unreleased `with_environment`/`OAuthEnvironment` builder. + +**Tech Stack:** Rust, `clap`, `toml_edit` (already used by `ConfigFile`), `async-trait`, `tokio`. Feature `pkce-auth` gates the provider changes. + +**Spec:** `docs/specs/2026-06-17-engine-environments-design.md` + +**Conventions for every task:** Tests-first. Run `cargo test --features pkce-auth ...` for anything touching `src/auth`. Before each commit run `cargo fmt --all` and `cargo clippy --all-targets --features pkce-auth -- -D warnings`. Commit messages end with the project's `Co-Authored-By` trailer. Work on a feature branch, not `main`. + +--- + +## File Structure + +- Create `src/environments.rs` — `OAuthConfig`, `EnvironmentDef`, `Environment`, `Environments` (definitions, layered resolution, file + env-var sources, active-env helpers). One module, one responsibility: environment definitions and resolution. +- Create `src/env_commands.rs` — the `env` command group (`list`/`get`/`set`/`info`) as a `RuntimeGroupSpec`, mirroring `src/auth/commands.rs` and `src/config_commands.rs`. +- Modify `src/lib.rs` — declare/export the new modules and public types. +- Modify `src/cli.rs` — `CliConfig.environments` field + `with_environments` builder; `ensure_env_command`; `--env` global flag registration; seed `Middleware.env` from the resolved active environment. +- Modify `src/middleware.rs` — add `environments: Option>` to `Middleware` and the per-run snapshot so handlers can resolve. +- Modify `src/command.rs` — `CommandContext::environment()` accessor (memoized resolve of the active env). +- Modify `src/config.rs` — (only if needed) a constant for the active-env config key; reuse existing `get`/`set`/`save`. +- Modify `src/auth/pkce.rs` — remove `OAuthEnvironment`/`with_environment`; add `with_environments(Arc)`; `effective_*` reads the resolved `OAuthConfig` when env-wired, else the legacy `_OAUTH_*` fallback. +- Modify `docs/concepts.md` (or add `docs/environments.md`) — document the feature. + +--- + +## Phase 1 — `environments` module: types & resolution + +### Task 1: Core types + +**Files:** +- Create: `src/environments.rs` +- Modify: `src/lib.rs` (add `pub mod environments;` and re-exports) + +- [ ] **Step 1: Write the failing test** + +In `src/environments.rs`, start the file with the types and a test module: + +```rust +//! First-class environment definitions and layered resolution. +//! +//! An [`Environments`] value holds compiled-in environment definitions and, +//! optionally, an `environments.toml` file plus `_*` env-var overrides. +//! Resolving a name merges those layers (later wins) into an [`Environment`]. + +use std::collections::BTreeMap; + +use serde::Deserialize; + +use crate::{Result, error::CliCoreError}; + +/// Standard OAuth slice of an environment, consumed by `PkceAuthProvider`. +#[derive(Clone, Debug, Default, PartialEq, Eq)] +pub struct OAuthConfig { + /// OAuth client id. + pub client_id: String, + /// Authorization endpoint. + pub auth_url: String, + /// Token endpoint. + pub token_url: String, + /// Default scopes. + pub scopes: Vec, +} + +/// A fully-resolved environment. +#[derive(Clone, Debug, PartialEq, Eq)] +pub struct Environment { + /// Environment name (e.g. `prod`). + pub name: String, + /// OAuth configuration, present when the environment participates in OAuth. + pub oauth: Option, + /// App-specific fields (for example `api_url`). + pub extra: BTreeMap, +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn oauth_config_defaults_are_empty() { + let c = OAuthConfig::default(); + assert!(c.client_id.is_empty() && c.scopes.is_empty()); + } +} +``` + +- [ ] **Step 2: Run test to verify it fails (compile-first)** + +Run: `cargo test --lib environments::tests::oauth_config_defaults_are_empty` +Expected: FAIL — `environments` module not declared in `lib.rs`. + +- [ ] **Step 3: Declare and export the module** + +In `src/lib.rs`, alongside the other `pub mod` lines, add: + +```rust +pub mod environments; +``` + +And in the public re-export area (near other `pub use`), add: + +```rust +pub use environments::{Environment, EnvironmentDef, Environments, OAuthConfig}; +``` + +(`EnvironmentDef`/`Environments` are added in Task 2; if the re-export fails to compile now, add only `Environment, OAuthConfig` here and extend the re-export in Task 2.) + +- [ ] **Step 4: Run test to verify it passes** + +Run: `cargo test --lib environments::tests::oauth_config_defaults_are_empty` +Expected: PASS. + +- [ ] **Step 5: Commit** + +```bash +git add src/environments.rs src/lib.rs +git commit -m "feat(environments): add OAuthConfig and Environment core types" +``` + +--- + +### Task 2: `EnvironmentDef` (partial layer) and `Environments` builder + +**Files:** +- Modify: `src/environments.rs` + +- [ ] **Step 1: Write the failing test** + +Add to the `tests` module: + +```rust +#[test] +fn builder_registers_compiled_environment() { + let envs = Environments::new("prod").with_environment( + "prod", + EnvironmentDef::new() + .with_client_id("prod-client") + .with_auth_url("https://api.example.com/authorize") + .with_token_url("https://api.example.com/token") + .with_scopes(&["openid"]) + .with_field("api_url", "https://api.example.com"), + ); + assert_eq!(envs.default_env(), "prod"); + assert_eq!(envs.list(), vec!["prod".to_owned()]); +} +``` + +- [ ] **Step 2: Run test to verify it fails** + +Run: `cargo test --lib environments::tests::builder_registers_compiled_environment` +Expected: FAIL — `EnvironmentDef`/`Environments` not found. + +- [ ] **Step 3: Implement the partial-layer type and builder** + +Add to `src/environments.rs` (above the `tests` module): + +```rust +/// An unresolved per-environment declaration (one layer of configuration). +/// +/// Fields are optional so layers can override individual values during +/// resolution. The same shape parses the `environments.toml` file. +#[derive(Clone, Debug, Default, Deserialize)] +pub struct EnvironmentDef { + #[serde(default)] + client_id: Option, + #[serde(default)] + auth_url: Option, + #[serde(default)] + token_url: Option, + #[serde(default)] + scopes: Option>, + /// Everything not recognised above is captured here (app-specific fields). + #[serde(flatten, default)] + extra: BTreeMap, +} + +impl EnvironmentDef { + /// Creates an empty declaration; every field falls back to lower layers. + #[must_use] + pub fn new() -> Self { + Self::default() + } + + /// Sets the OAuth client id. + #[must_use] + pub fn with_client_id(mut self, value: impl Into) -> Self { + self.client_id = Some(value.into()); + self + } + + /// Sets the authorization endpoint. + #[must_use] + pub fn with_auth_url(mut self, value: impl Into) -> Self { + self.auth_url = Some(value.into()); + self + } + + /// Sets the token endpoint. + #[must_use] + pub fn with_token_url(mut self, value: impl Into) -> Self { + self.token_url = Some(value.into()); + self + } + + /// Sets the default scopes. + #[must_use] + pub fn with_scopes(mut self, scopes: &[impl AsRef]) -> Self { + self.scopes = Some(scopes.iter().map(|s| s.as_ref().to_owned()).collect()); + self + } + + /// Sets an app-specific bag field. + #[must_use] + pub fn with_field(mut self, key: impl Into, value: impl Into) -> Self { + self.extra.insert(key.into(), value.into()); + self + } +} + +/// Engine-owned environment system: definitions + resolution + active-env state. +#[derive(Clone, Debug)] +pub struct Environments { + default: String, + defs: BTreeMap, + use_config_file: bool, + app_id: String, +} + +impl Environments { + /// Creates an environment system with the given default environment name. + #[must_use] + pub fn new(default_env: impl Into) -> Self { + Self { + default: default_env.into(), + defs: BTreeMap::new(), + use_config_file: false, + app_id: String::new(), + } + } + + /// Registers (or replaces) a compiled-in environment definition. + #[must_use] + pub fn with_environment(mut self, name: impl Into, def: EnvironmentDef) -> Self { + self.defs.insert(name.into(), def); + self + } + + /// Enables loading `//environments.toml` during resolution. + #[must_use] + pub fn with_config_file(mut self, enabled: bool) -> Self { + self.use_config_file = enabled; + self + } + + /// Sets the application id used to locate the config file. Set automatically + /// by [`crate::CliConfig::with_environments`]; only call directly in tests. + #[must_use] + pub fn with_app_id(mut self, app_id: impl Into) -> Self { + self.app_id = app_id.into(); + self + } + + /// The default environment name. + #[must_use] + pub fn default_env(&self) -> &str { + &self.default + } + + /// Enumerable environment names (compiled-in + file-defined), sorted. + #[must_use] + pub fn list(&self) -> Vec { + // File names folded in during Task 5; compiled-only for now. + self.defs.keys().cloned().collect() + } +} +``` + +Extend the `lib.rs` re-export to include `EnvironmentDef, Environments` if not already added in Task 1. + +- [ ] **Step 4: Run test to verify it passes** + +Run: `cargo test --lib environments::tests::builder_registers_compiled_environment` +Expected: PASS. + +- [ ] **Step 5: Commit** + +```bash +git add src/environments.rs src/lib.rs +git commit -m "feat(environments): add EnvironmentDef and Environments builder" +``` + +--- + +### Task 3: Layer merge + `resolve` (compiled + env-var) + +**Files:** +- Modify: `src/environments.rs` + +- [ ] **Step 1: Write the failing tests** + +Add to the `tests` module. These tests set process env vars, so serialize them on a lock and clean up: + +```rust +use std::sync::Mutex; +static ENV_LOCK: Mutex<()> = Mutex::new(()); + +fn sample() -> Environments { + Environments::new("prod") + .with_environment( + "prod", + EnvironmentDef::new() + .with_client_id("prod-client") + .with_auth_url("https://api.example.com/authorize") + .with_token_url("https://api.example.com/token") + .with_scopes(&["openid"]) + .with_field("api_url", "https://api.example.com"), + ) + .with_environment("dev", EnvironmentDef::new().with_client_id("dev-client")) +} + +#[test] +fn resolve_returns_compiled_record() { + let _g = ENV_LOCK.lock().unwrap_or_else(std::sync::PoisonError::into_inner); + let env = sample().resolve("prod").expect("prod resolves"); + let oauth = env.oauth.expect("oauth present"); + assert_eq!(oauth.client_id, "prod-client"); + assert_eq!(oauth.scopes, vec!["openid".to_owned()]); + assert_eq!(env.extra.get("api_url").map(String::as_str), Some("https://api.example.com")); +} + +#[test] +fn resolve_unknown_env_errors_with_known_names() { + let _g = ENV_LOCK.lock().unwrap_or_else(std::sync::PoisonError::into_inner); + let err = sample().resolve("nope").unwrap_err().to_string(); + assert!(err.contains("nope")); + assert!(err.contains("prod") && err.contains("dev")); +} + +#[test] +fn env_var_layer_overrides_oauth_and_known_bag_keys() { + let _g = ENV_LOCK.lock().unwrap_or_else(std::sync::PoisonError::into_inner); + // SAFETY: serialized by ENV_LOCK; removed below. + unsafe { + std::env::set_var("PROD_OAUTH_CLIENT_ID", "override-client"); + std::env::set_var("PROD_API_URL", "https://api.override.example.com"); + } + let env = sample().resolve("prod").expect("prod resolves"); + assert_eq!(env.oauth.unwrap().client_id, "override-client"); + assert_eq!(env.extra.get("api_url").map(String::as_str), Some("https://api.override.example.com")); + unsafe { + std::env::remove_var("PROD_OAUTH_CLIENT_ID"); + std::env::remove_var("PROD_API_URL"); + } +} +``` + +- [ ] **Step 2: Run tests to verify they fail** + +Run: `cargo test --lib environments::tests::resolve` +Expected: FAIL — `resolve` not found. + +- [ ] **Step 3: Implement merge + resolve** + +Add these methods to `impl Environments` and a free merge helper: + +```rust +impl Environments { + /// Resolves `name` by merging compiled defaults, the config file (Task 5), + /// and `_*` env-var overrides (later wins) into an [`Environment`]. + pub fn resolve(&self, name: &str) -> Result { + let compiled = self.defs.get(name); + let file = self.file_def(name)?; // Task 5 returns None until file support lands. + if compiled.is_none() && file.is_none() { + return Err(CliCoreError::message(format!( + "unknown environment {name:?}; known: {}", + self.list().join(", ") + ))); + } + let mut merged = EnvironmentDef::default(); + if let Some(def) = compiled { + merge_into(&mut merged, def); + } + if let Some(def) = &file { + merge_into(&mut merged, def); + } + apply_env_vars(name, &mut merged); + Ok(finalize(name, merged)) + } + + /// Loads a per-environment definition from the config file. Returns `None` + /// when the file layer is disabled or the env is absent. Implemented in Task 5. + fn file_def(&self, _name: &str) -> Result> { + Ok(None) + } +} + +/// Merges `src` into `dst`, with `src` winning on any field it sets. +fn merge_into(dst: &mut EnvironmentDef, src: &EnvironmentDef) { + if src.client_id.is_some() { + dst.client_id = src.client_id.clone(); + } + if src.auth_url.is_some() { + dst.auth_url = src.auth_url.clone(); + } + if src.token_url.is_some() { + dst.token_url = src.token_url.clone(); + } + if src.scopes.is_some() { + dst.scopes = src.scopes.clone(); + } + for (k, v) in &src.extra { + dst.extra.insert(k.clone(), v.clone()); + } +} + +/// Applies `_*` overrides: the three OAuth fields always, and any bag key +/// already present in the merged record (keyed `_`). +fn apply_env_vars(name: &str, def: &mut EnvironmentDef) { + let prefix = name.to_uppercase().replace('-', "_"); + if let Ok(v) = std::env::var(format!("{prefix}_OAUTH_CLIENT_ID")) { + def.client_id = Some(v); + } + if let Ok(v) = std::env::var(format!("{prefix}_OAUTH_AUTH_URL")) { + def.auth_url = Some(v); + } + if let Ok(v) = std::env::var(format!("{prefix}_OAUTH_TOKEN_URL")) { + def.token_url = Some(v); + } + let keys: Vec = def.extra.keys().cloned().collect(); + for key in keys { + let var = format!("{prefix}_{}", key.to_uppercase().replace('-', "_")); + if let Ok(v) = std::env::var(&var) { + def.extra.insert(key, v); + } + } +} + +/// Turns a fully-merged declaration into a resolved [`Environment`]. OAuth is +/// present when a client id was set by any layer. +fn finalize(name: &str, def: EnvironmentDef) -> Environment { + let oauth = def.client_id.as_ref().map(|client_id| OAuthConfig { + client_id: client_id.clone(), + auth_url: def.auth_url.clone().unwrap_or_default(), + token_url: def.token_url.clone().unwrap_or_default(), + scopes: def.scopes.clone().unwrap_or_default(), + }); + Environment { + name: name.to_owned(), + oauth, + extra: def.extra, + } +} +``` + +- [ ] **Step 4: Run tests to verify they pass** + +Run: `cargo test --lib environments::tests` +Expected: PASS (all resolve tests). + +- [ ] **Step 5: Commit** + +```bash +git add src/environments.rs +git commit -m "feat(environments): layered resolve with env-var overrides" +``` + +--- + +### Task 4: Config-file path helper + +**Files:** +- Modify: `src/config.rs` (reuse `config_file_path`), `src/environments.rs` + +- [ ] **Step 1: Write the failing test** + +Add to `environments::tests`: + +```rust +#[test] +fn environments_file_path_sits_next_to_config() { + let envs = sample().with_app_id("gddy").with_config_file(true); + let path = envs.config_file_path().expect("path resolves with app id"); + assert!(path.ends_with("gddy/environments.toml"), "got {path:?}"); +} +``` + +- [ ] **Step 2: Run test to verify it fails** + +Run: `cargo test --lib environments::tests::environments_file_path_sits_next_to_config` +Expected: FAIL — `config_file_path` not found. + +- [ ] **Step 3: Implement the path helper** + +In `src/config.rs`, confirm `config_file_path(app_id) -> Option` returns `//config.toml` (it does, per `config.rs:200`). Add to `impl Environments` in `src/environments.rs`: + +```rust +impl Environments { + /// Path to `environments.toml` next to the engine config file, or `None` + /// when the file layer is disabled or the config dir cannot be determined. + #[must_use] + pub fn config_file_path(&self) -> Option { + if !self.use_config_file { + return None; + } + let config = crate::config::config_file_path(&self.app_id)?; + Some(config.with_file_name("environments.toml")) + } +} +``` + +- [ ] **Step 4: Run test to verify it passes** + +Run: `cargo test --lib environments::tests::environments_file_path_sits_next_to_config` +Expected: PASS. + +- [ ] **Step 5: Commit** + +```bash +git add src/environments.rs +git commit -m "feat(environments): resolve environments.toml path" +``` + +--- + +### Task 5: Load `environments.toml` into `file_def` + +**Files:** +- Modify: `src/environments.rs` + +- [ ] **Step 1: Write the failing test** + +Add to `environments::tests` (writes a temp file; uses `tempfile`, already a dev-dependency). Inject the path via a test-only seam: + +```rust +#[test] +fn file_layer_overrides_compiled_and_adds_custom_env() { + let _g = ENV_LOCK.lock().unwrap_or_else(std::sync::PoisonError::into_inner); + let dir = tempfile::tempdir().expect("tempdir"); + let file = dir.path().join("environments.toml"); + std::fs::write( + &file, + r#" +[prod] +client_id = "file-client" + +[custom] +client_id = "custom-client" +api_url = "https://api.custom.example.com" +"#, + ) + .expect("write file"); + + let envs = sample().with_config_file(true).with_config_file_path_override(file); + + // File overrides the compiled prod client id, keeps compiled api_url. + let prod = envs.resolve("prod").expect("prod"); + assert_eq!(prod.oauth.unwrap().client_id, "file-client"); + assert_eq!(prod.extra.get("api_url").map(String::as_str), Some("https://api.example.com")); + + // Custom env exists only in the file. + let custom = envs.resolve("custom").expect("custom"); + assert_eq!(custom.oauth.unwrap().client_id, "custom-client"); + assert!(envs.list().contains(&"custom".to_owned())); +} +``` + +- [ ] **Step 2: Run test to verify it fails** + +Run: `cargo test --lib environments::tests::file_layer_overrides_compiled_and_adds_custom_env` +Expected: FAIL — `with_config_file_path_override` and real `file_def` not implemented. + +- [ ] **Step 3: Implement file loading** + +Add a path override field and parsing. Change the `Environments` struct to add `file_path_override: Option` (default `None`); update `new` accordingly. Replace `file_def` and add the parsed-file accessor + `list` update: + +```rust +impl Environments { + /// Test/advanced seam: force the environments file path. + #[must_use] + pub fn with_config_file_path_override(mut self, path: std::path::PathBuf) -> Self { + self.file_path_override = Some(path); + self.use_config_file = true; + self + } + + fn effective_file_path(&self) -> Option { + if let Some(path) = &self.file_path_override { + return Some(path.clone()); + } + self.config_file_path() + } + + /// Parses the environments file into a name -> def map. Missing file = empty. + fn file_defs(&self) -> Result> { + let Some(path) = self.effective_file_path() else { + return Ok(BTreeMap::new()); + }; + let text = match std::fs::read_to_string(&path) { + Ok(text) => text, + Err(err) if err.kind() == std::io::ErrorKind::NotFound => return Ok(BTreeMap::new()), + Err(err) => { + return Err(CliCoreError::message(format!( + "reading environments file {path:?}: {err}" + ))); + } + }; + toml_edit::de::from_str::>(&text).map_err(|err| { + CliCoreError::message(format!("parsing environments file {path:?}: {err}")) + }) + } + + fn file_def(&self, name: &str) -> Result> { + Ok(self.file_defs()?.remove(name)) + } +} +``` + +Update `list` to include file names: + +```rust + #[must_use] + pub fn list(&self) -> Vec { + let mut names: std::collections::BTreeSet = self.defs.keys().cloned().collect(); + if let Ok(file) = self.file_defs() { + names.extend(file.into_keys()); + } + names.into_iter().collect() + } +``` + +Add `use toml_edit;` is unnecessary (path-qualified). Ensure `BTreeMap` import already present. + +- [ ] **Step 4: Run test to verify it passes** + +Run: `cargo test --lib environments::tests` +Expected: PASS (all environments tests, including the file test). + +- [ ] **Step 5: Commit** + +```bash +git add src/environments.rs +git commit -m "feat(environments): load environments.toml as a resolution layer" +``` + +--- + +## Phase 2 — Active-environment persistence + +### Task 6: Read/write the active environment via `ConfigFile` + +**Files:** +- Modify: `src/environments.rs` + +- [ ] **Step 1: Write the failing test** + +Add to `environments::tests`: + +```rust +const ACTIVE_KEY: &str = "environment.active"; + +#[test] +fn active_env_round_trips_through_config_file() { + use crate::config::ConfigFile; + let mut cfg = ConfigFile::default(); + assert_eq!(Environments::active_from_config(&cfg), None); + + cfg.set(ACTIVE_KEY, "ote").expect("set"); + assert_eq!(Environments::active_from_config(&cfg).as_deref(), Some("ote")); +} + +#[test] +fn effective_active_prefers_override_then_config_then_default() { + use crate::config::ConfigFile; + let envs = sample(); + let mut cfg = ConfigFile::default(); + cfg.set(ACTIVE_KEY, "dev").expect("set"); + + assert_eq!(envs.effective_active(Some("prod"), &cfg), "prod"); // explicit wins + assert_eq!(envs.effective_active(None, &cfg), "dev"); // config next + let empty = ConfigFile::default(); + assert_eq!(envs.effective_active(None, &empty), "prod"); // default last +} +``` + +If `ConfigFile::default()` does not exist, construct via `ConfigFile::load("")` against a temp HOME, or add a `#[cfg(test)]` constructor. Check `src/config.rs` — prefer adding `impl Default for ConfigFile` returning an empty document if absent. + +- [ ] **Step 2: Run test to verify it fails** + +Run: `cargo test --lib environments::tests::active_env_round_trips_through_config_file` +Expected: FAIL — `active_from_config`/`effective_active` not found. + +- [ ] **Step 3: Implement persistence helpers** + +Add to `src/environments.rs`: + +```rust +/// Config-file key under which the sticky active environment is stored. +pub(crate) const ACTIVE_ENV_KEY: &str = "environment.active"; + +impl Environments { + /// Reads the persisted active environment from a loaded config file. + #[must_use] + pub fn active_from_config(config: &crate::config::ConfigFile) -> Option { + config.get(ACTIVE_ENV_KEY) + } + + /// Resolves the active environment name with precedence: + /// explicit `--env` override > persisted active > configured default. + #[must_use] + pub fn effective_active( + &self, + flag: Option<&str>, + config: &crate::config::ConfigFile, + ) -> String { + flag.map(ToOwned::to_owned) + .or_else(|| Self::active_from_config(config)) + .unwrap_or_else(|| self.default.clone()) + } + + /// Persists `name` as the active environment (loads, sets, saves a fresh + /// config file for `app_id`). Validates that `name` resolves first. + pub fn persist_active(&self, name: &str) -> Result<()> { + self.resolve(name)?; // reject unknown names + let mut config = crate::config::ConfigFile::load(&self.app_id); + config.set(ACTIVE_ENV_KEY, name)?; + config.save() + } +} +``` + +If `ConfigFile` lacks `Default`, add to `src/config.rs`: + +```rust +impl Default for ConfigFile { + fn default() -> Self { + Self { doc: toml_edit::DocumentMut::new(), path: None } + } +} +``` + +(Match `ConfigFile`'s real field names; adjust if they differ.) + +- [ ] **Step 4: Run tests to verify they pass** + +Run: `cargo test --lib environments::tests` +Expected: PASS. + +- [ ] **Step 5: Commit** + +```bash +git add src/environments.rs src/config.rs +git commit -m "feat(environments): persist and resolve the active environment" +``` + +--- + +## Phase 3 — Engine wiring + +### Task 7: `Middleware.environments` field + +**Files:** +- Modify: `src/middleware.rs` + +- [ ] **Step 1: Write the failing test** + +Add to the existing tests in `src/middleware.rs` (or create a small `#[cfg(test)] mod env_wire_tests`): + +```rust +#[test] +fn middleware_carries_optional_environments() { + use std::sync::Arc; + let mut mw = Middleware::new(); + assert!(mw.environments.is_none()); + mw.environments = Some(Arc::new(crate::environments::Environments::new("prod"))); + assert_eq!(mw.environments.as_ref().unwrap().default_env(), "prod"); +} +``` + +- [ ] **Step 2: Run test to verify it fails** + +Run: `cargo test --lib middleware_carries_optional_environments` +Expected: FAIL — no `environments` field. + +- [ ] **Step 3: Add the field** + +In `src/middleware.rs`, add to `struct Middleware` (near `config`): + +```rust + /// Optional environment system, set by `CliConfig::with_environments`. + pub environments: Option>, +``` + +Initialize it to `None` in `Middleware::new()` and in any other constructor/`Default`. If a per-run snapshot struct clones selected middleware fields, add `environments: self.environments.clone()` there too (search for where `config` is cloned into the snapshot and mirror it). + +- [ ] **Step 4: Run test to verify it passes** + +Run: `cargo test --lib middleware_carries_optional_environments` +Expected: PASS. + +- [ ] **Step 5: Commit** + +```bash +git add src/middleware.rs +git commit -m "feat(environments): thread environments through middleware" +``` + +--- + +### Task 8: `CliConfig.environments` + `with_environments` + +**Files:** +- Modify: `src/cli.rs` + +- [ ] **Step 1: Write the failing test** + +Add to the `user_agent_tests` module's file (or a new `#[cfg(test)] mod env_config_tests` at the end of `src/cli.rs`): + +```rust +#[test] +fn with_environments_stores_and_app_id_is_injected() { + let cfg = CliConfig::new("gddy", "GoDaddy CLI", "gddy") + .with_environments(crate::environments::Environments::new("prod").with_config_file(true)); + let envs = cfg.environments.as_ref().expect("environments set"); + // app_id is stamped from CliConfig so the file path resolves. + assert!(envs.config_file_path().is_some()); +} +``` + +- [ ] **Step 2: Run test to verify it fails** + +Run: `cargo test --lib env_config_tests::with_environments_stores_and_app_id_is_injected` +Expected: FAIL — no `environments` field/builder. + +- [ ] **Step 3: Add field + builder** + +In `src/cli.rs`, add to `struct CliConfig` (near `auth_providers`): + +```rust + /// Optional first-class environment system. + pub environments: Option, +``` + +Add the builder (near `with_default_auth_provider`), stamping `app_id` so file/persistence paths resolve: + +```rust + /// Registers a first-class environment system: mounts the `env` command + /// group, registers the global `--env` flag, and seeds the active + /// environment into middleware. The provided `Environments` has this + /// config's `app_id` applied so its config file and persistence resolve. + #[must_use] + pub fn with_environments(mut self, environments: crate::environments::Environments) -> Self { + self.environments = Some(environments.with_app_id(self.app_id.clone())); + self + } +``` + +`#[derive(Default)]` covers the new `Option` field. + +- [ ] **Step 4: Run test to verify it passes** + +Run: `cargo test --lib env_config_tests::with_environments_stores_and_app_id_is_injected` +Expected: PASS. + +- [ ] **Step 5: Commit** + +```bash +git add src/cli.rs +git commit -m "feat(environments): add CliConfig::with_environments" +``` + +--- + +### Task 9: Seed active env into middleware + register `--env` flag + +**Files:** +- Modify: `src/cli.rs` + +- [ ] **Step 1: Write the failing test** + +Add to `env_config_tests`: + +```rust +#[tokio::test] +async fn env_flag_overrides_default_and_reaches_middleware_env() { + use crate::{CommandResult, CommandSpec, RuntimeCommandSpec}; + use serde_json::json; + let mut cli = Cli::new( + CliConfig::new("envtest", "Env test", "envtest") + .with_environments( + crate::environments::Environments::new("prod") + .with_environment("prod", crate::environments::EnvironmentDef::new()) + .with_environment("ote", crate::environments::EnvironmentDef::new()), + ), + ); + cli.add_command(RuntimeCommandSpec::new_with_context( + CommandSpec::new("whichenv", "echo env").no_auth(true), + async |ctx| { Ok(CommandResult::new(json!({ "env": ctx.environment()?.name }))) }, + )); + let out = cli.run(["envtest", "whichenv", "--env", "ote", "--output", "json"]).await; + assert_eq!(out.exit_code, 0); + assert!(out.rendered.contains("\"env\"")); + assert!(out.rendered.contains("ote")); +} +``` + +(Adjust the `new_with_context` closure form to match `auth/commands.rs` usage; see Task 11 for the exact handler signature and the `ctx.environment()` accessor it depends on — implement Task 10 and 11's accessor first if running strictly in order. If so, reorder: do Task 10 before this test passes.) + +- [ ] **Step 2: Run test to verify it fails** + +Run: `cargo test --features pkce-auth --lib env_config_tests::env_flag_overrides_default_and_reaches_middleware_env` +Expected: FAIL — `--env` not registered / `ctx.environment()` missing. + +- [ ] **Step 3: Register the flag and seed middleware** + +In `Cli::new` (`src/cli.rs`), after `middleware.config = …` and before building `cli`, add: + +```rust + if let Some(environments) = &config.environments { + let environments = std::sync::Arc::new(environments.clone()); + // Seed the sticky/default active env now; the --env flag (applied + // per run) overrides it in run_with_depth. + middleware.env = environments.effective_active(None, &middleware.config); + middleware.environments = Some(environments); + } +``` + +Register the `--env` global flag when environments are configured. In `Cli::new`, where the root command is assembled (after `register_global_flags(root)`), add: + +```rust + if config.environments.is_some() { + root = root.arg( + clap::Arg::new("env") + .long("env") + .global(true) + .value_name("ENV") + .help("Target environment"), + ); + } +``` + +Apply the flag per run: in `run_with_depth` (`src/cli.rs:1190`), after args are parsed into matches but before middleware executes the command, set the per-run env from the flag when present. Locate where the per-run middleware/`MiddlewareRequest` is built and insert: + +```rust + // --env overrides the seeded active environment for this invocation. + if let Some(env) = parsed_global_env(&text_args) { + if let Some(environments) = &self.middleware.environments { + // Validate; unknown env -> error envelope. + environments.resolve(&env)?; + } + run_env_override = Some(env); + } +``` + +Add a small helper near the other `text_args` parsing helpers: + +```rust +fn parsed_global_env(text_args: &[String]) -> Option { + let mut iter = text_args.iter(); + while let Some(arg) = iter.next() { + if arg == "--env" { + return iter.next().cloned(); + } + if let Some(value) = arg.strip_prefix("--env=") { + return Some(value.to_owned()); + } + } + None +} +``` + +Thread `run_env_override` into the per-run middleware so `MiddlewareRequest.env`/`Middleware.env` reflects it. Mirror how the existing code copies `self.middleware` into the per-run middleware (search `run_with_depth` for where it clones middleware) and set `.env` to the override when present, else keep the seeded value. + +> Implementation note: if `run_with_depth` already clones `self.middleware` into a mutable per-run value, set `per_run.env = run_env_override.unwrap_or_else(|| self.middleware.env.clone())`. Keep the change minimal and within the existing clone path. + +- [ ] **Step 4: Run test to verify it passes** (after Task 10 accessor exists) + +Run: `cargo test --features pkce-auth --lib env_config_tests::env_flag_overrides_default_and_reaches_middleware_env` +Expected: PASS. + +- [ ] **Step 5: Commit** + +```bash +git add src/cli.rs +git commit -m "feat(environments): register --env and seed active env into middleware" +``` + +--- + +### Task 10: `CommandContext::environment()` accessor + +**Files:** +- Modify: `src/command.rs` + +- [ ] **Step 1: Write the failing test** + +The behavior is exercised by Task 9's integration test. Add a focused unit check in `src/command.rs` tests if a context can be constructed there; otherwise rely on Task 9. Minimum: add a doctest on the method (Step 3). + +- [ ] **Step 2: Run Task 9 test to verify it fails** + +Run: `cargo test --features pkce-auth --lib env_config_tests::env_flag_overrides_default_and_reaches_middleware_env` +Expected: FAIL — `ctx.environment()` missing. + +- [ ] **Step 3: Implement the accessor** + +In `src/command.rs`, add to `impl CommandContext` (near `config`): + +```rust + /// Resolves the active [`Environment`](crate::environments::Environment) for + /// this invocation. + /// + /// The active environment name is `Middleware.env` (set from `--env`, the + /// persisted active env, or the configured default). Returns an error if no + /// environment system was registered via + /// [`CliConfig::with_environments`](crate::CliConfig::with_environments) or + /// if the active name does not resolve. + pub fn environment(&self) -> Result { + let environments = self.middleware.environments.as_ref().ok_or_else(|| { + crate::error::CliCoreError::message("no environment system configured") + })?; + environments.resolve(&self.middleware.env) + } +``` + +(Return an owned `Environment` to avoid borrowing/memoization complexity; resolution is cheap and runs at most a few times per invocation. If the per-run snapshot exposes env differently than `self.middleware.env`, use that field name.) + +- [ ] **Step 4: Run test to verify it passes** + +Run: `cargo test --features pkce-auth --lib env_config_tests::env_flag_overrides_default_and_reaches_middleware_env` +Expected: PASS. + +- [ ] **Step 5: Commit** + +```bash +git add src/command.rs +git commit -m "feat(environments): add CommandContext::environment accessor" +``` + +--- + +## Phase 4 — `env` command group + +### Task 11: `env list/get/set/info` group, auto-mounted + +**Files:** +- Create: `src/env_commands.rs` +- Modify: `src/lib.rs` (add `mod env_commands;`), `src/cli.rs` (`ensure_env_command`, call it) + +- [ ] **Step 1: Write the failing tests** + +Add an integration test to `tests/foundation.rs` (mirrors existing `Cli::run` tests): + +```rust +#[tokio::test] +async fn env_group_lists_gets_and_sets_active_environment() { + use cli_engine::environments::{EnvironmentDef, Environments}; + let cli = Cli::new( + CliConfig::new("envcmds", "Env cmds", "envcmds").with_environments( + Environments::new("prod") + .with_environment("prod", EnvironmentDef::new().with_field("api_url", "https://p")) + .with_environment("ote", EnvironmentDef::new().with_field("api_url", "https://o")), + ), + ); + + let list = cli.run(["envcmds", "env", "list", "--output", "json"]).await; + assert_eq!(list.exit_code, 0); + assert!(list.rendered.contains("prod") && list.rendered.contains("ote")); + + let get = cli.run(["envcmds", "env", "get", "--output", "json"]).await; + assert_eq!(get.exit_code, 0); + assert!(get.rendered.contains("prod")); // default active + + let info = cli.run(["envcmds", "env", "info", "--env", "ote", "--output", "json"]).await; + assert_eq!(info.exit_code, 0); + assert!(info.rendered.contains("https://o")); +} +``` + +> Note: `env set` persists to the real per-app config file. Do NOT assert `set` persistence in a shared-process test unless the test isolates `$HOME`/`$XDG_CONFIG_HOME` via a temp dir guard. Cover `set` persistence in a `#[cfg(test)]` unit test in `environments.rs` (Task 6 already covers the round-trip) and keep this integration test to `list`/`get`/`info`. + +- [ ] **Step 2: Run test to verify it fails** + +Run: `cargo test --features pkce-auth --test foundation env_group_lists_gets_and_sets_active_environment` +Expected: FAIL — `env` group not mounted. + +- [ ] **Step 3: Implement the group** + +Create `src/env_commands.rs`, mirroring `src/auth/commands.rs` structure and `src/config_commands.rs`: + +```rust +//! Built-in `env` command group: list/get/set/info for environments. + +use serde_json::json; + +use crate::{ + CommandResult, CommandSpec, GroupSpec, RuntimeCommandSpec, RuntimeGroupSpec, + error::CliCoreError, +}; + +/// Builds the built-in `env` command group. +pub fn env_command_group() -> RuntimeGroupSpec { + RuntimeGroupSpec::new(GroupSpec::new("env", "Manage the active environment")) + .with_command(RuntimeCommandSpec::new_with_context( + CommandSpec::new("list", "List known environments").no_auth(true), + async |ctx| { + let envs = ctx + .middleware + .environments + .as_ref() + .ok_or_else(|| CliCoreError::message("no environment system configured"))?; + let active = ctx.middleware.env.clone(); + let items: Vec<_> = envs + .list() + .into_iter() + .map(|name| json!({ "name": name, "active": name == active })) + .collect(); + Ok(CommandResult::new(json!(items))) + }, + )) + .with_command(RuntimeCommandSpec::new_with_context( + CommandSpec::new("get", "Show the active environment").no_auth(true), + async |ctx| { + Ok(CommandResult::new(json!({ "active": ctx.middleware.env }))) + }, + )) + .with_command(RuntimeCommandSpec::new_with_context( + CommandSpec::new("info", "Show the resolved active environment").no_auth(true), + async |ctx| { + let env = ctx.environment()?; + let oauth = env.oauth.map(|o| { + json!({ "client_id": o.client_id, "auth_url": o.auth_url, "token_url": o.token_url, "scopes": o.scopes }) + }); + Ok(CommandResult::new(json!({ + "name": env.name, + "oauth": oauth, + "extra": env.extra, + }))) + }, + )) + .with_command(RuntimeCommandSpec::new_with_context( + CommandSpec::new("set", "Set and persist the active environment") + .no_auth(true) + .with_arg(clap::Arg::new("name").required(true)), + async |ctx| { + let envs = ctx + .middleware + .environments + .as_ref() + .ok_or_else(|| CliCoreError::message("no environment system configured"))?; + let name = ctx + .args + .get("name") + .and_then(|v| v.as_str()) + .ok_or_else(|| CliCoreError::message("missing environment name"))?; + envs.persist_active(name)?; + Ok(CommandResult::new(json!({ "active": name }))) + }, + )) +} +``` + +> Adjust `ctx.args` access to match how `auth/commands.rs` reads arguments (e.g. it may use `ctx.args.get(...)` or a typed accessor). Copy that idiom exactly. + +In `src/lib.rs` add `mod env_commands;` (private; only `cli.rs` needs it). In `src/cli.rs`, add `ensure_env_command` mirroring `ensure_config_command`: + +```rust + fn ensure_env_command(&mut self) { + if has_subcommand(&self.root, "env") { + return; + } + let group = crate::env_commands::env_command_group(); + let mut prefix = Vec::new(); + group.register_commands(&mut prefix, &mut self.commands); + let mut prefix = Vec::new(); + let clap_group = runtime_group_clap_command_with_schema_help( + &group, + &mut prefix, + &self.middleware.schema_registry, + ); + self.root = self.root.clone().subcommand(clap_group); + let category = self + .config + .admin_category + .clone() + .unwrap_or_else(|| DEFAULT_ADMIN_CATEGORY.to_owned()); + if !self.module_entries.iter().any(|e| e.name == "env") { + self.module_entries.push(ModuleHelpEntry { + category, + name: "env".to_owned(), + short: "Manage the active environment".to_owned(), + }); + } + self.refresh_root_long(); + } +``` + +Call it in `Cli::new`, right after the environments are seeded into middleware: + +```rust + if cli.config.environments.is_some() { + cli.ensure_env_command(); + } +``` + +- [ ] **Step 4: Run test to verify it passes** + +Run: `cargo test --features pkce-auth --test foundation env_group_lists_gets_and_sets_active_environment` +Expected: PASS. + +- [ ] **Step 5: Commit** + +```bash +git add src/env_commands.rs src/lib.rs src/cli.rs +git commit -m "feat(environments): add built-in env command group" +``` + +--- + +## Phase 5 — OAuth tie-in (replace `with_environment`) + +### Task 12: Remove `OAuthEnvironment`/`with_environment`; add `with_environments` + +**Files:** +- Modify: `src/auth/pkce.rs` + +- [ ] **Step 1: Write the failing test** + +Replace the existing `environment_override_selects_per_env_oauth_config` and `unconfigured_environment_falls_back_to_base_config` tests in `pkce.rs` with environment-sourced equivalents: + +```rust +fn envs_for_test() -> std::sync::Arc { + use crate::environments::{EnvironmentDef, Environments}; + std::sync::Arc::new( + Environments::new("prod").with_environment( + "prod", + EnvironmentDef::new() + .with_client_id("prod-client") + .with_auth_url("https://prod.example.com/auth") + .with_token_url("https://prod.example.com/token") + .with_scopes(&["openid", "prod.read"]), + ), + ) +} + +#[test] +fn environment_wired_provider_sources_oauth_from_resolver() { + let provider = PkceAuthProvider::new("godaddy", "https://base/auth", "https://base/token", "base-client", &["openid"]) + .with_environments(envs_for_test()); + assert_eq!(provider.effective_client_id("prod"), "prod-client"); + assert_eq!(provider.effective_auth_url("prod"), "https://prod.example.com/auth"); + assert_eq!(provider.effective_token_url("prod"), "https://prod.example.com/token"); + assert_eq!(provider.effective_scopes("prod"), vec!["openid".to_owned(), "prod.read".to_owned()]); +} + +#[test] +fn non_wired_provider_uses_base_config() { + let provider = PkceAuthProvider::new("godaddy", "https://base/auth", "https://base/token", "base-client", &["openid"]); + assert_eq!(provider.effective_client_id("anything"), "base-client"); + assert_eq!(provider.effective_scopes("anything"), vec!["openid".to_owned()]); +} +``` + +- [ ] **Step 2: Run tests to verify they fail** + +Run: `cargo test --features pkce-auth --lib auth::pkce::tests::environment_wired_provider_sources_oauth_from_resolver` +Expected: FAIL — `with_environments` not found; `OAuthEnvironment` still referenced elsewhere. + +- [ ] **Step 3: Replace the per-env map with a resolver** + +In `src/auth/pkce.rs`: + +1. Delete the `OAuthEnvironment` struct, its `impl`, the `environments: HashMap` field, the `with_environment` method, and any `OAuthEnvironment` doctest/re-export. +2. Add a resolver field and builder: + +```rust + /// Optional environment resolver; when set, per-env OAuth config comes from + /// the resolved environment instead of the base config / legacy env override. + environments: Option>, +``` + +```rust + /// Sources per-environment OAuth config from a shared [`Environments`]. + /// + /// Given an `env`, the provider resolves the environment and uses its + /// `OAuthConfig`. This is the single-source-of-truth path; prefer it over + /// the base `client_id`/`auth_url`/`token_url` when the consumer registers + /// environments via [`CliConfig::with_environments`](crate::CliConfig::with_environments). + #[must_use] + pub fn with_environments( + mut self, + environments: std::sync::Arc, + ) -> Self { + self.environments = Some(environments); + self + } +``` + +3. Initialize `environments: None` in `new`. +4. Rewrite the `effective_*` methods to prefer the resolver, then the legacy `_OAUTH_*` env override (kept for non-wired providers), then base: + +```rust + fn resolved_oauth(&self, env: &str) -> Option { + self.environments + .as_ref() + .and_then(|envs| envs.resolve(env).ok()) + .and_then(|resolved| resolved.oauth) + } + + fn effective_client_id(&self, env: &str) -> String { + if let Some(oauth) = self.resolved_oauth(env) { + if !oauth.client_id.is_empty() { + return oauth.client_id; + } + } + let key = format!("{}_OAUTH_CLIENT_ID", self.env_prefix); + std::env::var(&key).unwrap_or_else(|_| self.client_id.clone()) + } + + fn effective_auth_url(&self, env: &str) -> String { + if let Some(oauth) = self.resolved_oauth(env) { + if !oauth.auth_url.is_empty() { + return oauth.auth_url; + } + } + let key = format!("{}_OAUTH_AUTH_URL", self.env_prefix); + std::env::var(&key).unwrap_or_else(|_| self.auth_url.clone()) + } + + fn effective_token_url(&self, env: &str) -> String { + if let Some(oauth) = self.resolved_oauth(env) { + if !oauth.token_url.is_empty() { + return oauth.token_url; + } + } + let key = format!("{}_OAUTH_TOKEN_URL", self.env_prefix); + std::env::var(&key).unwrap_or_else(|_| self.token_url.clone()) + } + + fn effective_scopes(&self, env: &str) -> Vec { + if let Some(oauth) = self.resolved_oauth(env) { + if !oauth.scopes.is_empty() { + return oauth.scopes; + } + } + self.scopes.clone() + } +``` + +5. Remove the now-stale `perenv_provider` test helper and the old per-env tests replaced in Step 1. Keep all token-timeout, shared-client, and User-Agent tests/code from the prior work stream unchanged. +6. Remove `OAuthEnvironment` from any `lib.rs`/module re-exports. + +- [ ] **Step 4: Run tests to verify they pass** + +Run: `cargo test --features pkce-auth --lib auth::pkce::tests` +Expected: PASS (new env-wired tests + retained timeout/UA tests). + +- [ ] **Step 5: Commit** + +```bash +git add src/auth/pkce.rs src/lib.rs +git commit -m "feat(environments)!: source PkceAuthProvider OAuth from Environments" +``` + +--- + +## Phase 6 — Docs & final verification + +### Task 13: Document environments + +**Files:** +- Create: `docs/environments.md`; Modify: `docs/concepts.md` (add a short pointer) + +- [ ] **Step 1: Write the doc** + +Create `docs/environments.md` covering: the three resolution layers and precedence; the `environments.toml` schema (`[]` tables with `client_id`/`auth_url`/`token_url`/`scopes` typed and any other key as a string bag field); the `_*` env-var convention; the sticky active env + `--env` + `env list/get/set/info`; and the `PkceAuthProvider::with_environments` single-source pattern with a runnable example. Add a one-line pointer + link in `docs/concepts.md`. + +- [ ] **Step 2: Verify doctests/build** + +Run: `cargo test --doc --features pkce-auth` +Expected: PASS (any fenced `rust` examples compile). + +- [ ] **Step 3: Commit** + +```bash +git add docs/environments.md docs/concepts.md +git commit -m "docs(environments): document first-class environments" +``` + +--- + +### Task 14: Full verification sweep + +- [ ] **Step 1: Format + lint (both feature modes)** + +```bash +cargo fmt --all --check +cargo clippy --all-targets --features pkce-auth -- -D warnings +cargo clippy --all-targets -- -D warnings +``` +Expected: clean. + +- [ ] **Step 2: Tests + doctests + docs** + +```bash +cargo test --features pkce-auth --all-targets +cargo test --doc --features pkce-auth +RUSTDOCFLAGS='-D warnings' cargo doc --no-deps --features pkce-auth +cargo rustdoc --lib --features pkce-auth -- -W missing-docs +``` +Expected: all pass; missing-docs count zero. + +- [ ] **Step 3: Final commit if any fmt/doc fixups were needed** + +```bash +git add -A +git commit -m "chore(environments): formatting and doc fixups" +``` + +--- + +## Self-Review Notes (for the implementer) + +- **Spec coverage:** Tasks 1–5 = core/resolution/file; Task 6 = active-env persistence; Tasks 7–10 = engine wiring + context accessor; Task 11 = `env` group; Task 12 = OAuth single-source + `with_environment` removal; Task 13 = docs. Migration (gddy re-login, gdx greenfield) happens in those repos and is out of scope here. +- **Ordering caveat:** Task 9's integration test depends on the `ctx.environment()` accessor from Task 10 — implement Task 10's accessor before expecting Task 9's test to go green (or treat 9+10 as a pair). +- **Verify-as-you-go:** Several wiring snippets (`run_with_depth` per-run middleware clone, `ctx.args` idiom, `ConfigFile` field names for `Default`) must be matched to the real code at implementation time; the referenced mirror points (`ensure_config_command`, `auth_command_group`, `CommandContext::config`) are exact. +- **Non-negotiables:** env-var/config-file tests serialize on a lock and restore process state; `unsafe { set_var }` is test-only and paired with `remove_var`. diff --git a/docs/specs/2026-06-17-engine-environments-design.md b/docs/specs/2026-06-17-engine-environments-design.md new file mode 100644 index 0000000..e60ada0 --- /dev/null +++ b/docs/specs/2026-06-17-engine-environments-design.md @@ -0,0 +1,198 @@ +# First-Class Environments in cli-engine — Design + +Date: 2026-06-17 +Status: Approved (design); pending implementation plan + +## Context + +Two consumers of `cli-engine` — `gddy` (the GoDaddy developer CLI) and `gdx` — +each independently reimplement "multiple environments" (e.g. `dev`/`test`/`ote`/`prod`): + +- `gddy` (`src/environments/mod.rs`, `src/auth.rs`, `src/env/mod.rs`): a rich, + runtime-extensible system — compiled built-ins, a `~/.config/gddy/environments.toml` + file, and `_*` env-var overrides; records carry `client_id`, `auth_url`, + `token_url`, `api_url`, `domains_api_url`, `account_url`, `api_key`/`api_secret`; a + global `--env` flag with a sticky `.gdenv` state file; and `env list/get/set/info` + commands. It builds one `PkceAuthProvider` per environment on demand, with the + provider name set to the env name. +- `gdx` (`rust/src/auth.rs`): a hardcoded `match env { "dev"|"test"|"ote"|"prod" }` + mapping to `client_id` + `api_base`. No environment config file. + +The engine already owns *part* of an environment concept: `Middleware.env`, +default-env injection (`middleware.rs:586`), `CommandMeta::fixed_env()`, and the +`env` argument passed to `AuthProvider::get_credential`. It does not own the +`--env` flag, active-env persistence, environment definitions/resolution, or the +`env` command group — so each consumer rebuilds those. + +**Goal:** make environments a first-class engine mechanism so `gddy` and `gdx` +delete their hand-rolled flag/persistence/subcommands/resolution and net-remove +code, with the environment as the single source of truth for both API base URLs +and per-environment OAuth config. + +This supersedes the unreleased `PkceAuthProvider::with_environment` / +`OAuthEnvironment` builder added earlier in this work stream (still uncommitted), +which is replaced by the resolver described here. + +## Decisions (from brainstorming) + +1. Primary objective: **kill duplication** — both consumers migrate and remove code. +2. Boundary: **engine owns the format + resolution** (definitions, file schema, + env-var convention, merging), not just the lifecycle. +3. Record shape: **standard typed OAuth fields + a `BTreeMap` bag** + for app-specific fields. No generics on `Cli`/`Middleware`. +4. Resolution: **three layers, later wins** — compiled defaults < config file < + env-var overrides. +5. Selection: **sticky active env persisted in the per-app config file**, a `--env` + override, and an auto-mounted `env list/get/set/info` group. +6. OAuth tie-in: **the environment is the single source**; `PkceAuthProvider` reads + its OAuth config from the resolver. Replaces the unreleased `with_environment`. + +Rejected alternative: a generic `Environment` threaded through `Cli`/`Middleware` +(fully typed end-to-end but a large blast radius). The string bag is chosen instead. + +## Core Types + +```rust +/// Standard OAuth slice of an environment, consumed by PkceAuthProvider. +#[derive(Clone, Debug, Default)] +pub struct OAuthConfig { + pub client_id: String, + pub auth_url: String, + pub token_url: String, + pub scopes: Vec, +} + +/// One fully-resolved environment. +#[derive(Clone, Debug)] +pub struct Environment { + pub name: String, + /// Present when the environment participates in OAuth. + pub oauth: Option, + /// App-specific fields (api_url, domains_api_url, account_url, …). + pub extra: BTreeMap, +} + +/// Engine-owned environment system: definitions + resolution + active-env state. +/// Built once by the consumer and shared (Arc) between CliConfig and any +/// PkceAuthProvider that is environment-driven. +#[derive(Clone, Debug)] +pub struct Environments { /* compiled defaults, file path, config handle, default name */ } +``` + +Builder sketch: + +```rust +Environments::new("prod") // default env name + .with_environment("prod", EnvironmentDef::new() + .with_oauth(OAuthConfig { client_id: "…".into(), auth_url: "…".into(), + token_url: "…".into(), scopes: vec!["openid".into()] }) + .with_field("api_url", "https://api.godaddy.com")) + .with_environment("ote", /* … */) + .with_config_file(true); // enable ~/.config//environments.toml + env-var layer +``` + +`EnvironmentDef` is the *unresolved* per-env declaration (the same shape used to +parse the TOML file); `Environment` is the *resolved* result after merging layers. + +## Resolution + +`Environments::resolve(&self, name: &str) -> Result` merges, later wins: + +1. **Compiled defaults** declared via `with_environment` on `CliConfig`/`Environments`. +2. **Config file** `~/.config//environments.toml` (XDG/`%APPDATA%` per platform, + same dir the engine already uses for credentials/config). Schema mirrors `gddy`'s: + standard OAuth keys parse into `OAuthConfig`; every other key lands in `extra`. + Enables user-defined custom environments. +3. **Env-var overrides**, keyed by env name (uppercased, `-`→`_`): + - `_OAUTH_CLIENT_ID`, `_OAUTH_AUTH_URL`, `_OAUTH_TOKEN_URL` + - `_` for bag fields, e.g. `PROD_API_URL`. + +Behavior: +- Unknown env name → typed error listing enumerable env names. +- `Environments::list()` returns compiled + file-defined envs. Env-var-only envs are + resolvable but not enumerable (matches `gddy`). +- Merge is field-level: a file or env-var layer may override individual OAuth fields + or individual bag keys without restating the whole record. + +## Selection & Lifecycle + +- The engine registers a **global `--env` flag** only when environments are configured. +- **Sticky active env** stored in the existing per-app `ConfigFile` under a new + `active_environment` key (no separate state file). Precedence for the active env: + `--env` value > persisted active > `Environments` default. +- Auto-mounted **`env` command group** (mounted like the built-in `auth` group): + - `env list` — enumerable environments, marking the active one. + - `env get` — prints the active environment name. + - `env set ` — validates via `resolve()` (so env-var-only envs are accepted), + then persists to `ConfigFile`. + - `env info` — shows the resolved active environment (OAuth endpoints + bag), + with secrets omitted. +- The resolved active env name is written to the existing `Middleware.env`, so + default-env injection (`middleware.rs:586`) and `CommandMeta::fixed_env()` keep + working unchanged. + +## Engine Wiring & Handler Access + +- `CliConfig::with_environments(Environments)` — registers the system: mounts the + `env` group, registers `--env`, seeds `Middleware.env` from the resolved active env. +- Handlers access the resolved active environment through a context accessor: + `ctx.environment() -> Result<&Environment>` (resolved once per run and memoized, + like the credential resolver). Handlers read base URLs from `env.extra`: + `env.extra.get("api_url")`. +- `--schema` / `--dry-run` short-circuits must not force file/env resolution + (resolution is lazy, consistent with credential-store resolution today). + +## OAuth Tie-In + +- `PkceAuthProvider::with_environments(Arc)` **replaces** the unreleased + `with_environment` / `OAuthEnvironment` builder (removed outright — no deprecation + needed since it is uncommitted). +- An environment-wired provider, given `env`, calls `environments.resolve(env)?` and + uses the resulting `OAuthConfig` (`client_id`/`auth_url`/`token_url`/`scopes`) for the + flow. One definition drives both API base URLs (bag) and OAuth; custom/file/env-var + environments get OAuth automatically. +- The pre-existing released `_OAUTH_*` env override remains **only** for + providers that are *not* environment-wired (back-compat for current consumers). + Environment-wired providers use the `_OAUTH_*` layer instead. +- Provider-level config that is genuinely not per-env (redirect URI/port, app_id, + identity claims, token timeout, shared client) stays on `PkceAuthProvider` as today. + +## Migration & Back-Compat + +- **`with_environment` removal**: safe — uncommitted in this work stream. +- **gddy credential storage keys**: today provider-name = env-name yields keys + `gddy//`. A single environment-wired provider (e.g. named `godaddy`) + yields `gddy/godaddy/`. This changes the keys, so existing users re-authenticate + once after upgrade. **Decision: accept the one-time re-login**, documented in the + changelog. No storage-key compat shim. +- **gddy config file**: the engine's `environments.toml` schema is a superset of + gddy's existing file, so migration is near-zero (standard OAuth keys typed, the rest + into `extra`). gddy's URL derivation (`auth_url`/`token_url` from `api_url`) becomes + declaring explicit URLs in compiled defaults, or specifying them per custom env; + the engine does not own a derivation rule. +- **gdx**: greenfield — declares compiled defaults, deletes its `match env` OAuth maps, + and may drop its bespoke env plumbing. gdx's `ote`→`test` SSO aliasing stays + app-side (it is not OAuth-environment resolution). +- The per-env token timeout, shared token client, and User-Agent work from this work + stream are unaffected and carry forward. + +## Testing + +- **Unit:** resolution precedence across all three layers; field-level merge; + unknown-env error contents; `list()` enumeration vs env-var-only resolvability; + env-var keying (`_OAUTH_*`, `_`); active-env persistence round-trip + through `ConfigFile`; lazy resolution (no file/env access on `--schema`/`--dry-run`). +- **Integration (`Cli::run`):** `env set`/`get`/`list`/`info`; `--env` overrides the + sticky active env; an environment-wired `PkceAuthProvider` selects per-env OAuth + (verified with the offline request-builder seam already used for UA/timeout tests). +- **Doctests** on the public builders (`Environments`, `EnvironmentDef`, `OAuthConfig`, + `CliConfig::with_environments`, `PkceAuthProvider::with_environments`). +- Env-var and config-file tests serialize on a lock and restore process state, matching + existing `credential_store_config`/User-Agent test conventions. + +## Clarifications (decided; called out for implementation) + +- `env set` validates via `resolve()` so env-var-only environments can be selected. +- Exact reconciliation of the `_OAUTH_*` (environment layer) vs `_OAUTH_*` + (legacy provider layer) precedence is: environment-wired providers use the environment + layer exclusively; non-wired providers retain the legacy layer. diff --git a/src/auth/pkce.rs b/src/auth/pkce.rs index 148d3b3..b514cfc 100644 --- a/src/auth/pkce.rs +++ b/src/auth/pkce.rs @@ -77,6 +77,9 @@ use crate::{ const REDIRECT_PORT_DEFAULT: u16 = 7443; const TOKEN_EXPIRY_BUFFER_SECS: i64 = 30; +/// Default timeout applied to OAuth token-endpoint requests (exchange/refresh) +/// so a stalled token server cannot hang the CLI indefinitely. +const TOKEN_REQUEST_TIMEOUT_DEFAULT: Duration = Duration::from_secs(30); /// Stored token with expiry tracking. /// @@ -124,6 +127,74 @@ impl StoredToken { } } +/// Per-environment OAuth configuration for [`PkceAuthProvider`]. +/// +/// A single provider often serves several environments (for example `dev`, +/// `test`, and `prod`) that each use a different OAuth client id and set of +/// endpoints. Register one of these per environment with +/// [`PkceAuthProvider::with_environment`]. Any field left unset falls back to +/// the provider's base configuration, so an override can change just the client +/// id while reusing the base endpoints, or vice versa. +/// +/// # Examples +/// +/// ``` +/// use cli_engine::auth::pkce::OAuthEnvironment; +/// +/// let prod = OAuthEnvironment::new() +/// .with_client_id("prod-client-id") +/// .with_auth_url("https://api.example.com/v2/oauth2/authorize") +/// .with_token_url("https://api.example.com/v2/oauth2/token"); +/// # let _ = prod; +/// ``` +#[derive(Debug, Clone, Default)] +pub struct OAuthEnvironment { + client_id: Option, + auth_url: Option, + token_url: Option, + scopes: Option>, +} + +impl OAuthEnvironment { + /// Creates an empty override. Every field falls back to the provider's base + /// configuration until set. + #[must_use] + pub fn new() -> Self { + Self::default() + } + + /// Overrides the OAuth client id for this environment. + #[must_use] + pub fn with_client_id(mut self, client_id: impl Into) -> Self { + self.client_id = Some(client_id.into()); + self + } + + /// Overrides the authorization endpoint for this environment. + #[must_use] + pub fn with_auth_url(mut self, auth_url: impl Into) -> Self { + self.auth_url = Some(auth_url.into()); + self + } + + /// Overrides the token endpoint for this environment. + #[must_use] + pub fn with_token_url(mut self, token_url: impl Into) -> Self { + self.token_url = Some(token_url.into()); + self + } + + /// Overrides the default scopes requested for this environment. + /// + /// Replaces the base scope set for this environment rather than extending + /// it. Leave unset to reuse the provider's base scopes. + #[must_use] + pub fn with_scopes(mut self, scopes: &[impl AsRef]) -> Self { + self.scopes = Some(scopes.iter().map(|s| s.as_ref().to_owned()).collect()); + self + } +} + /// OAuth 2.0 PKCE authentication provider. /// /// Stores one token per `(env, provider)` pair in the system keychain. @@ -135,8 +206,20 @@ pub struct PkceAuthProvider { token_url: String, client_id: String, scopes: Vec, + /// Per-environment configuration overrides keyed by env name. Looked up by + /// the `env` passed to [`AuthProvider::get_credential`]; unmatched envs use + /// the base `client_id`/`auth_url`/`token_url`/`scopes` above. + environments: HashMap, redirect_port: u16, redirect_uri: Option, + /// Timeout applied to token-endpoint requests (exchange and refresh). + token_timeout: Duration, + /// Shared HTTP client for token-endpoint traffic, built once and reused by + /// exchange and refresh so connections and TLS configuration are pooled + /// rather than rebuilt per request. The user-agent and timeout are applied + /// per request (not baked into the client) so they reflect the value + /// published at execution time, not at provider construction. + client: reqwest::Client, app_id: String, env_prefix: String, /// Explicit storage backend injected via [`PkceAuthProvider::with_storage`]. @@ -183,8 +266,11 @@ impl PkceAuthProvider { token_url: token_url.into(), client_id: client_id.into(), scopes: scopes.iter().map(|s| s.as_ref().to_owned()).collect(), + environments: HashMap::new(), redirect_port: REDIRECT_PORT_DEFAULT, redirect_uri: None, + token_timeout: TOKEN_REQUEST_TIMEOUT_DEFAULT, + client: reqwest::Client::new(), app_id: String::new(), env_prefix, storage_override: None, @@ -198,6 +284,48 @@ impl PkceAuthProvider { } } + /// Registers per-environment OAuth configuration. + /// + /// Use this when one provider serves several environments that each use a + /// different OAuth client id or set of endpoints (for example `dev`, + /// `test`, and `prod`). The override is selected by the `env` argument + /// passed to [`AuthProvider::get_credential`]; environments without an + /// override use the base configuration supplied to + /// [`PkceAuthProvider::new`]. Registering the same env twice replaces the + /// earlier override. + /// + /// Provider-prefixed environment variables + /// (`_OAUTH_CLIENT_ID`, `_AUTH_URL`, `_TOKEN_URL`) still take + /// precedence over both the per-environment override and the base + /// configuration, so operators retain a single escape hatch. + /// + /// # Examples + /// + /// ``` + /// use cli_engine::auth::pkce::{OAuthEnvironment, PkceAuthProvider}; + /// + /// let provider = PkceAuthProvider::new( + /// "godaddy", + /// "https://api.godaddy.com/v2/oauth2/authorize", + /// "https://api.godaddy.com/v2/oauth2/token", + /// "prod-client-id", + /// &["openid", "profile"], + /// ) + /// .with_environment( + /// "dev", + /// OAuthEnvironment::new() + /// .with_client_id("dev-client-id") + /// .with_auth_url("https://api.dev-godaddy.com/v2/oauth2/authorize") + /// .with_token_url("https://api.dev-godaddy.com/v2/oauth2/token"), + /// ); + /// # let _ = provider; + /// ``` + #[must_use] + pub fn with_environment(mut self, env: impl Into, config: OAuthEnvironment) -> Self { + self.environments.insert(env.into(), config); + self + } + /// Sets the local redirect server port (default: 7443). #[must_use] pub fn with_redirect_port(mut self, port: u16) -> Self { @@ -205,6 +333,17 @@ impl PkceAuthProvider { self } + /// Sets the timeout applied to token-endpoint requests (authorization-code + /// exchange and refresh). + /// + /// Defaults to 30 seconds. This bounds only the HTTP token requests; the + /// interactive browser/callback wait has its own separate timeout. + #[must_use] + pub fn with_token_timeout(mut self, timeout: Duration) -> Self { + self.token_timeout = timeout; + self + } + /// Overrides the redirect URI sent to the authorization server. /// /// By default the redirect URI is `http://127.0.0.1:{port}/callback`. Use @@ -317,19 +456,44 @@ impl PkceAuthProvider { } } - fn effective_client_id(&self) -> String { - let key = format!("{}_OAUTH_CLIENT_ID", self.env_prefix); - std::env::var(&key).unwrap_or_else(|_| self.client_id.clone()) + /// Resolves a value for `env` with precedence: provider-prefixed env var + /// (operator escape hatch) > per-environment override > base configuration. + fn effective_value( + &self, + env: &str, + var_suffix: &str, + select: impl Fn(&OAuthEnvironment) -> Option<&String>, + base: &str, + ) -> String { + let key = format!("{}_OAUTH_{var_suffix}", self.env_prefix); + if let Ok(value) = std::env::var(&key) { + return value; + } + self.environments + .get(env) + .and_then(select) + .map_or_else(|| base.to_owned(), Clone::clone) + } + + fn effective_client_id(&self, env: &str) -> String { + self.effective_value(env, "CLIENT_ID", |e| e.client_id.as_ref(), &self.client_id) + } + + fn effective_auth_url(&self, env: &str) -> String { + self.effective_value(env, "AUTH_URL", |e| e.auth_url.as_ref(), &self.auth_url) } - fn effective_auth_url(&self) -> String { - let key = format!("{}_OAUTH_AUTH_URL", self.env_prefix); - std::env::var(&key).unwrap_or_else(|_| self.auth_url.clone()) + fn effective_token_url(&self, env: &str) -> String { + self.effective_value(env, "TOKEN_URL", |e| e.token_url.as_ref(), &self.token_url) } - fn effective_token_url(&self) -> String { - let key = format!("{}_OAUTH_TOKEN_URL", self.env_prefix); - std::env::var(&key).unwrap_or_else(|_| self.token_url.clone()) + /// Default scopes for `env`: a per-environment scope override when set, + /// otherwise the provider's base scopes. + fn effective_scopes(&self, env: &str) -> Vec { + self.environments + .get(env) + .and_then(|e| e.scopes.clone()) + .unwrap_or_else(|| self.scopes.clone()) } fn effective_redirect_uri(&self) -> String { @@ -424,7 +588,8 @@ impl PkceAuthProvider { if let Some(token) = self.existing_token(env).await? { return Ok(token); } - self.reauthenticate(env, &self.scopes).await + let scopes = self.effective_scopes(env); + self.reauthenticate(env, &scopes).await } /// Returns a usable token from the in-memory cache, keychain, or a refresh — @@ -442,7 +607,7 @@ impl PkceAuthProvider { } if let Some(refresh_token) = token.refresh_token.as_deref() && let Ok(mut refreshed) = self - .refresh_access_token(refresh_token, &token.scopes) + .refresh_access_token(env, refresh_token, &token.scopes) .await { if refreshed.refresh_token.is_none() { @@ -459,7 +624,7 @@ impl PkceAuthProvider { /// Runs a fresh interactive PKCE flow requesting exactly `scopes`, replacing /// any stored token for `env`. async fn reauthenticate(&self, env: &str, scopes: &[String]) -> Result { - let token = self.run_pkce_flow_with(scopes).await?; + let token = self.run_pkce_flow_with(env, scopes).await?; // Persist first — the keychain write overwrites the existing entry for // this env — and only update the in-memory cache after a successful // save. This avoids destroying a still-valid token if the save fails @@ -471,11 +636,11 @@ impl PkceAuthProvider { /// Runs the browser PKCE flow requesting exactly `scopes` (used both for the /// default login and for scope step-up, which requests a wider union). - async fn run_pkce_flow_with(&self, scopes: &[String]) -> Result { + async fn run_pkce_flow_with(&self, env: &str, scopes: &[String]) -> Result { let (code_verifier, code_challenge) = pkce_challenge(); let state = random_state(); - let client_id = self.effective_client_id(); - let auth_url = self.effective_auth_url(); + let client_id = self.effective_client_id(env); + let auth_url = self.effective_auth_url(env); let redirect_uri = self.effective_redirect_uri(); let scope = scopes.join(" "); @@ -507,19 +672,39 @@ impl PkceAuthProvider { let code = wait_for_callback(listener, &state, &callback_path, Duration::from_secs(120)).await?; - self.exchange_code_for_token(&code, &code_verifier, scopes) + self.exchange_code_for_token(env, &code, &code_verifier, scopes) .await } + /// Builds a POST to an OAuth token endpoint on the provider's shared client. + /// + /// Token traffic does not go through [`HttpClient`](crate::transport::HttpClient) + /// — that client is built for authenticated, JSON-bodied backend calls, + /// whereas the token endpoint is unauthenticated and form-encoded. The + /// user-agent and timeout are attached here per request (read at call time) + /// so every outbound call, including credential acquisition and refresh, is + /// attributed consistently and bounded. + fn token_request(&self, token_url: &str, params: &[(&str, &str)]) -> reqwest::RequestBuilder { + self.client + .post(token_url) + .header( + reqwest::header::USER_AGENT, + crate::transport::client::default_user_agent(), + ) + .timeout(self.token_timeout) + .form(params) + } + async fn exchange_code_for_token( &self, + env: &str, code: &str, code_verifier: &str, requested_scopes: &[String], ) -> Result { let redirect_uri = self.effective_redirect_uri(); - let client_id = self.effective_client_id(); - let token_url = self.effective_token_url(); + let client_id = self.effective_client_id(env); + let token_url = self.effective_token_url(env); let params = [ ("grant_type", "authorization_code"), @@ -528,9 +713,8 @@ impl PkceAuthProvider { ("code", code), ("code_verifier", code_verifier), ]; - let response = reqwest::Client::new() - .post(&token_url) - .form(¶ms) + let response = self + .token_request(&token_url, ¶ms) .send() .await .map_err(|err| CliCoreError::message(format!("token request failed: {err}")))?; @@ -548,19 +732,19 @@ impl PkceAuthProvider { async fn refresh_access_token( &self, + env: &str, refresh_token: &str, prior_scopes: &[String], ) -> Result { - let client_id = self.effective_client_id(); - let token_url = self.effective_token_url(); + let client_id = self.effective_client_id(env); + let token_url = self.effective_token_url(env); let params = [ ("grant_type", "refresh_token"), ("client_id", &client_id), ("refresh_token", refresh_token), ]; - let response = reqwest::Client::new() - .post(&token_url) - .form(¶ms) + let response = self + .token_request(&token_url, ¶ms) .send() .await .map_err(|err| CliCoreError::message(format!("token refresh failed: {err}")))?; @@ -591,6 +775,7 @@ impl AuthProvider for PkceAuthProvider { async fn get_credential_for(&self, req: &CredentialRequest<'_>) -> Result { let env = req.env; let required = &req.meta.scopes; + let defaults = self.effective_scopes(env); // Look for a usable token WITHOUT launching a flow, so we can pick the // scope set for a single login rather than authenticating twice (e.g. @@ -602,7 +787,7 @@ impl AuthProvider for PkceAuthProvider { // server has no silent scope-expansion grant, so in non-interactive // contexts we fail fast rather than hang on the callback timeout. let granted = granted_scopes(&token); - match plan_step_up(&self.scopes, &granted, required, session_is_interactive()) { + match plan_step_up(&defaults, &granted, required, session_is_interactive()) { StepUp::Covered => return Ok(self.build_credential(env, &token)), StepUp::MissingNonInteractive(missing) => { return Err(missing_scope_error(env, &missing)); @@ -618,7 +803,7 @@ impl AuthProvider for PkceAuthProvider { } // No usable token: authenticate once, requesting defaults ∪ required. - let union = union_scopes(&self.scopes, &[], required); + let union = union_scopes(&defaults, &[], required); let token = self.reauthenticate(env, &union).await?; ensure_granted(env, &token, required)?; Ok(self.build_credential(env, &token)) @@ -1035,6 +1220,102 @@ mod tests { } } + fn perenv_provider() -> PkceAuthProvider { + PkceAuthProvider::new( + "perenv", + "https://base.example.com/auth", + "https://base.example.com/token", + "base-client", + &["openid"], + ) + .with_environment( + "prod", + OAuthEnvironment::new() + .with_client_id("prod-client") + .with_auth_url("https://prod.example.com/auth") + .with_token_url("https://prod.example.com/token") + .with_scopes(&["openid", "prod.read"]), + ) + } + + /// A per-environment override wins over the provider's base configuration + /// for the matching env, so one provider can serve dev/test/prod with + /// distinct OAuth clients and endpoints. + #[test] + fn environment_override_selects_per_env_oauth_config() { + let provider = perenv_provider(); + assert_eq!(provider.effective_client_id("prod"), "prod-client"); + assert_eq!( + provider.effective_auth_url("prod"), + "https://prod.example.com/auth" + ); + assert_eq!( + provider.effective_token_url("prod"), + "https://prod.example.com/token" + ); + assert_eq!( + provider.effective_scopes("prod"), + vec!["openid".to_owned(), "prod.read".to_owned()] + ); + } + + /// An environment with no registered override falls back to the base + /// client id, endpoints, and scopes. + #[test] + fn unconfigured_environment_falls_back_to_base_config() { + let provider = perenv_provider(); + assert_eq!(provider.effective_client_id("dev"), "base-client"); + assert_eq!( + provider.effective_auth_url("dev"), + "https://base.example.com/auth" + ); + assert_eq!( + provider.effective_token_url("dev"), + "https://base.example.com/token" + ); + assert_eq!(provider.effective_scopes("dev"), vec!["openid".to_owned()]); + } + + /// OAuth token traffic must carry the engine's configured default + /// user-agent so it is attributed consistently with all other outbound + /// calls (some upstream WAFs reject requests without a User-Agent). + #[test] + fn token_request_carries_default_user_agent() { + let _guard = crate::transport::client::UA_TEST_LOCK + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner); + crate::transport::set_default_user_agent("ua-probe/7.7"); + let provider = test_provider().with_token_timeout(Duration::from_secs(12)); + let request = provider + .token_request( + "https://example.com/token", + &[("grant_type", "refresh_token")], + ) + .build() + .expect("token request should build"); + let header = request + .headers() + .get(reqwest::header::USER_AGENT) + .expect("token request should set a user-agent"); + assert_eq!(header, "ua-probe/7.7"); + assert_eq!(request.timeout(), Some(&Duration::from_secs(12))); + crate::transport::set_default_user_agent("cli/dev"); + } + + /// OAuth token requests must not hang indefinitely: the provider applies a + /// 30s timeout by default. + #[test] + fn default_token_timeout_is_thirty_seconds() { + assert_eq!(test_provider().token_timeout, Duration::from_secs(30)); + } + + /// The default token timeout can be overridden per provider. + #[test] + fn with_token_timeout_overrides_default() { + let provider = test_provider().with_token_timeout(Duration::from_secs(5)); + assert_eq!(provider.token_timeout, Duration::from_secs(5)); + } + /// store_cached_token + cached_token round-trip: the mechanism used by /// the persistence fix must reliably write and read tokens from the cache. #[tokio::test] diff --git a/src/cli.rs b/src/cli.rs index 0e0e5cb..f8f049b 100644 --- a/src/cli.rs +++ b/src/cli.rs @@ -210,6 +210,10 @@ pub struct CliConfig { pub views: Vec, /// Providers registered before command execution starts. pub auth_providers: Vec>, + /// Optional override for the process-wide outbound User-Agent. When unset, + /// the engine derives `name/version` from this config. See + /// [`CliConfig::user_agent_string`]. + pub user_agent: Option, /// Optional authorization gatekeeper injected into middleware. pub authz: Option>, /// Optional audit recorder injected into middleware. @@ -289,6 +293,38 @@ impl CliConfig { self } + /// Overrides the outbound User-Agent string for all HTTP traffic. + /// + /// When unset, the engine derives `name/version` from this config (see + /// [`CliConfig::user_agent_string`]). Set this when the upstream APIs expect + /// a specific product token. The resolved value is applied process-wide on + /// execution via [`crate::transport::set_default_user_agent`], so it reaches + /// both command [`HttpClient`](crate::transport::HttpClient)s and the + /// engine's own OAuth token requests. + #[must_use] + pub fn with_user_agent(mut self, user_agent: impl Into) -> Self { + self.user_agent = Some(user_agent.into()); + self + } + + /// Returns the outbound User-Agent string the CLI presents on HTTP requests. + /// + /// Resolution order: + /// 1. an explicit [`with_user_agent`](Self::with_user_agent) override; + /// 2. otherwise `name/version` (for example `gdx/1.2.3`); + /// 3. otherwise just `name` when no build version is set. + #[must_use] + pub fn user_agent_string(&self) -> String { + if let Some(user_agent) = &self.user_agent { + return user_agent.clone(); + } + if self.build.version.is_empty() { + self.name.clone() + } else { + format!("{}/{}", self.name, self.build.version) + } + } + /// Adds one domain module. #[must_use] pub fn with_module(mut self, module: Module) -> Self { @@ -845,6 +881,7 @@ impl Cli { E: Write, Shutdown: Future, { + self.install_default_user_agent(); let output = run_until_signal(self.run(args), shutdown).await; if output.exit_code == 130 && output.rendered == "command interrupted\n" @@ -860,6 +897,17 @@ impl Cli { Ok(process_exit_code(output.exit_code)) } + /// Publishes the configured outbound User-Agent process-wide so that + /// command [`HttpClient`](crate::transport::HttpClient)s and the engine's + /// own OAuth token requests share it. + /// + /// Called from the execution entrypoints rather than [`Cli::new`] so that + /// merely constructing a `Cli` (as tests do in bulk) does not mutate global + /// state. See [`CliConfig::user_agent_string`] for resolution order. + fn install_default_user_agent(&self) { + crate::transport::set_default_user_agent(self.config.user_agent_string()); + } + /// Registers an auth provider after construction. pub fn register_auth_provider(&mut self, provider: Arc) -> &mut Self { self.middleware.auth.register(provider); @@ -2784,3 +2832,46 @@ async fn run_streaming_command( }), } } + +#[cfg(test)] +mod user_agent_tests { + use super::*; + + #[test] + fn user_agent_string_derives_name_and_version_by_default() { + let config = + CliConfig::new("gdx", "GoDaddy CLI", "gdx").with_build(BuildInfo::new("1.2.3")); + assert_eq!(config.user_agent_string(), "gdx/1.2.3"); + } + + #[test] + fn user_agent_string_prefers_explicit_override() { + let config = CliConfig::new("gdx", "GoDaddy CLI", "gdx") + .with_build(BuildInfo::new("1.2.3")) + .with_user_agent("gdx-cli/9.9 (custom)"); + assert_eq!(config.user_agent_string(), "gdx-cli/9.9 (custom)"); + } + + #[test] + fn user_agent_string_omits_version_when_absent() { + let config = CliConfig::new("gdx", "GoDaddy CLI", "gdx"); + assert_eq!(config.user_agent_string(), "gdx"); + } + + #[test] + fn install_default_user_agent_publishes_config_value() { + let _guard = crate::transport::client::UA_TEST_LOCK + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner); + crate::transport::set_default_user_agent("cli/dev"); + let cli = Cli::new( + CliConfig::new("uatest", "UA test", "uatest").with_build(BuildInfo::new("4.5.6")), + ); + cli.install_default_user_agent(); + assert_eq!( + crate::transport::client::default_user_agent(), + "uatest/4.5.6" + ); + crate::transport::set_default_user_agent("cli/dev"); + } +} diff --git a/src/transport/client.rs b/src/transport/client.rs index 8a2c42a..3eb1c2f 100644 --- a/src/transport/client.rs +++ b/src/transport/client.rs @@ -28,7 +28,12 @@ pub fn set_default_user_agent(user_agent: impl Into) { } } -fn default_user_agent() -> String { +/// Returns the process-wide default user-agent set via +/// [`set_default_user_agent`], or the builtin default when none was set. +/// +/// Used by [`HttpClientBuilder`] and by the engine's OAuth token requests so +/// that all outbound traffic carries the same user-agent. +pub(crate) fn default_user_agent() -> String { DEFAULT_USER_AGENT .get_or_init(|| RwLock::new(BUILTIN_DEFAULT_USER_AGENT.to_owned())) .read() @@ -38,6 +43,12 @@ fn default_user_agent() -> String { ) } +/// Serializes unit tests that mutate the process-wide default user-agent so +/// they cannot observe one another's writes. Integration tests in +/// `tests/foundation.rs` run in a separate binary and use their own lock. +#[cfg(test)] +pub(crate) static UA_TEST_LOCK: std::sync::Mutex<()> = std::sync::Mutex::new(()); + #[derive(serde::Deserialize)] struct GraphQlError { message: String, diff --git a/src/transport/injector.rs b/src/transport/injector.rs index 1ade0ea..46be9a7 100644 --- a/src/transport/injector.rs +++ b/src/transport/injector.rs @@ -259,6 +259,10 @@ impl ClientCredentialsInjector { reqwest::header::CONTENT_TYPE, "application/x-www-form-urlencoded", ) + .header( + reqwest::header::USER_AGENT, + crate::transport::client::default_user_agent(), + ) .form(&form) .send(), ) diff --git a/tests/foundation.rs b/tests/foundation.rs index 85a3649..b0e88aa 100644 --- a/tests/foundation.rs +++ b/tests/foundation.rs @@ -442,6 +442,9 @@ async fn cli_runtime_root_help_includes_find_commands_without_modules() { #[tokio::test] async fn cli_execute_from_writes_success_to_stdout_and_errors_to_stderr() { + // execute_from publishes the config-derived User-Agent process-wide, so this + // test shares the lock with the user-agent tests and restores the default. + let _ua_guard = USER_AGENT_TEST_LOCK.lock().await; let mut cli = Cli::new(CliConfig { name: "my-cli".to_owned(), short: "Developer tooling".to_owned(), @@ -483,10 +486,14 @@ async fn cli_execute_from_writes_success_to_stdout_and_errors_to_stderr() { assert!(stdout.is_empty()); let rendered = String::from_utf8(stderr).expect("utf8"); assert!(rendered.contains("missing")); + transport::set_default_user_agent("cli/dev"); } #[tokio::test] async fn cli_execute_from_shutdown_signal_writes_interrupt_to_stderr() { + // execute_from_until_signal publishes the config-derived User-Agent + // process-wide; share the lock and restore the default like the tests above. + let _ua_guard = USER_AGENT_TEST_LOCK.lock().await; let shutdown_count = Arc::new(AtomicUsize::new(0)); let shutdown_for_closure = Arc::clone(&shutdown_count); let mut cli = Cli::new(CliConfig { @@ -520,6 +527,7 @@ async fn cli_execute_from_shutdown_signal_writes_interrupt_to_stderr() { "command interrupted\n" ); assert_eq!(shutdown_count.load(Ordering::SeqCst), 1); + transport::set_default_user_agent("cli/dev"); } #[tokio::test] @@ -4936,12 +4944,17 @@ async fn provider_bearer_injector_empty_token_does_not_short_circuit_cache_prese #[tokio::test] async fn client_credentials_injector_requests_and_caches_bearer_token() { + // The injector tags its token request with the process default User-Agent; + // hold the user-agent lock and pin the default so the assertion is stable. + let _ua_guard = USER_AGENT_TEST_LOCK.lock().await; + transport::set_default_user_agent("cli/dev"); let token_requests = Arc::new(AtomicUsize::new(0)); let token_requests_for_server = Arc::clone(&token_requests); let server = TestServer::sequence(vec![Box::new(move |request| { token_requests_for_server.fetch_add(1, Ordering::SeqCst); assert!(request.contains("POST /token HTTP/1.1")); assert!(request.contains("content-type: application/x-www-form-urlencoded")); + assert!(request.contains("user-agent: cli/dev")); assert!(request.contains("grant_type=client_credentials")); assert!(request.contains("client_id=client")); assert!(request.contains("client_secret=secret")); @@ -5504,6 +5517,7 @@ async fn http_client_do_raw_sends_method_content_type_body_and_decodes_json() { #[tokio::test] async fn http_client_do_raw_optional_none_body_matches_legacy_nil_reader() { + let _ua_guard = USER_AGENT_TEST_LOCK.lock().await; let server = TestServer::new(|request| { assert!(request.contains("OPTIONS /raw HTTP/1.1")); assert!(request.contains("user-agent: cli/dev")); @@ -5646,6 +5660,7 @@ async fn http_client_etag_if_match_and_multipart_without_response_skip_success_d #[tokio::test] async fn http_client_post_raw_none_body_omits_json_content_type_preserves_legacy() { + let _ua_guard = USER_AGENT_TEST_LOCK.lock().await; let server = TestServer::new(|request| { assert!(request.contains("POST /raw HTTP/1.1")); assert!(request.contains("user-agent: cli/dev")); From c7e0b8dae9cac6d00eb03c6f5a5e74fb73d0ea2f Mon Sep 17 00:00:00 2001 From: Jacob Page Date: Wed, 17 Jun 2026 10:48:13 -0700 Subject: [PATCH 02/31] feat(environments): add OAuthConfig and Environment core types Co-Authored-By: Claude Sonnet 4.6 --- src/environments.rs | 42 ++++++++++++++++++++++++++++++++++++++++++ src/lib.rs | 3 +++ 2 files changed, 45 insertions(+) create mode 100644 src/environments.rs diff --git a/src/environments.rs b/src/environments.rs new file mode 100644 index 0000000..b3ab1da --- /dev/null +++ b/src/environments.rs @@ -0,0 +1,42 @@ +//! First-class environment definitions and layered resolution. +//! +//! An [`Environments`] value holds compiled-in environment definitions and, +//! optionally, an `environments.toml` file plus `_*` env-var overrides. +//! Resolving a name merges those layers (later wins) into an [`Environment`]. + +use std::collections::BTreeMap; + +/// Standard OAuth slice of an environment, consumed by `PkceAuthProvider`. +#[derive(Clone, Debug, Default, PartialEq, Eq)] +pub struct OAuthConfig { + /// OAuth client id. + pub client_id: String, + /// Authorization endpoint. + pub auth_url: String, + /// Token endpoint. + pub token_url: String, + /// Default scopes. + pub scopes: Vec, +} + +/// A fully-resolved environment. +#[derive(Clone, Debug, PartialEq, Eq)] +pub struct Environment { + /// Environment name (e.g. `prod`). + pub name: String, + /// OAuth configuration, present when the environment participates in OAuth. + pub oauth: Option, + /// App-specific fields (for example `api_url`). + pub extra: BTreeMap, +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn oauth_config_defaults_are_empty() { + let c = OAuthConfig::default(); + assert!(c.client_id.is_empty() && c.scopes.is_empty()); + } +} diff --git a/src/lib.rs b/src/lib.rs index bc40336..c3dca91 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -71,6 +71,8 @@ pub mod command; pub mod config; /// Built-in `config` command group. pub mod config_commands; +/// First-class environment definitions and layered resolution. +pub mod environments; /// Shared error type and error traits. pub mod error; /// Global framework flags and flag-extraction helpers. @@ -120,6 +122,7 @@ pub use config::{ credential_store_env_var, resolve_credential_store, resolve_credential_store_with, }; pub use config_commands::config_command_group; +pub use environments::{Environment, OAuthConfig}; pub use error::{ CliCoreError, DetailedError, ExitCoder, Result, exit_code_for_error, exit_code_for_exit_coder, }; From ec270d25c310cccd9a345fe425b89360b74f7221 Mon Sep 17 00:00:00 2001 From: Jacob Page Date: Wed, 17 Jun 2026 10:50:46 -0700 Subject: [PATCH 03/31] feat(environments): add EnvironmentDef and Environments builder Co-Authored-By: Claude Sonnet 4.6 --- src/environments.rs | 450 ++++++++++++++++++++++++++++++++++++++++++++ src/lib.rs | 2 +- 2 files changed, 451 insertions(+), 1 deletion(-) diff --git a/src/environments.rs b/src/environments.rs index b3ab1da..42cc1fc 100644 --- a/src/environments.rs +++ b/src/environments.rs @@ -6,6 +6,10 @@ use std::collections::BTreeMap; +use serde::Deserialize; + +use crate::{Result, error::CliCoreError}; + /// Standard OAuth slice of an environment, consumed by `PkceAuthProvider`. #[derive(Clone, Debug, Default, PartialEq, Eq)] pub struct OAuthConfig { @@ -30,13 +34,459 @@ pub struct Environment { pub extra: BTreeMap, } +/// An unresolved per-environment declaration (one layer of configuration). +/// +/// Fields are optional so layers can override individual values during +/// resolution. The same shape parses the `environments.toml` file. +#[derive(Clone, Debug, Default, Deserialize)] +pub struct EnvironmentDef { + #[serde(default)] + client_id: Option, + #[serde(default)] + auth_url: Option, + #[serde(default)] + token_url: Option, + #[serde(default)] + scopes: Option>, + /// Everything not recognised above is captured here (app-specific fields). + #[serde(flatten, default)] + extra: BTreeMap, +} + +impl EnvironmentDef { + /// Creates an empty declaration; every field falls back to lower layers. + #[must_use] + pub fn new() -> Self { + Self::default() + } + + /// Sets the OAuth client id. + #[must_use] + pub fn with_client_id(mut self, value: impl Into) -> Self { + self.client_id = Some(value.into()); + self + } + + /// Sets the authorization endpoint. + #[must_use] + pub fn with_auth_url(mut self, value: impl Into) -> Self { + self.auth_url = Some(value.into()); + self + } + + /// Sets the token endpoint. + #[must_use] + pub fn with_token_url(mut self, value: impl Into) -> Self { + self.token_url = Some(value.into()); + self + } + + /// Sets the default scopes. + #[must_use] + pub fn with_scopes(mut self, scopes: &[impl AsRef]) -> Self { + self.scopes = Some(scopes.iter().map(|s| s.as_ref().to_owned()).collect()); + self + } + + /// Sets an app-specific bag field. + #[must_use] + pub fn with_field(mut self, key: impl Into, value: impl Into) -> Self { + self.extra.insert(key.into(), value.into()); + self + } +} + +/// Engine-owned environment system: definitions + resolution + active-env state. +#[derive(Clone, Debug)] +pub struct Environments { + default: String, + defs: BTreeMap, + use_config_file: bool, + app_id: String, + file_path_override: Option, +} + +impl Environments { + /// Creates an environment system with the given default environment name. + #[must_use] + pub fn new(default_env: impl Into) -> Self { + Self { + default: default_env.into(), + defs: BTreeMap::new(), + use_config_file: false, + app_id: String::new(), + file_path_override: None, + } + } + + /// Registers (or replaces) a compiled-in environment definition. + #[must_use] + pub fn with_environment(mut self, name: impl Into, def: EnvironmentDef) -> Self { + self.defs.insert(name.into(), def); + self + } + + /// Enables loading `//environments.toml` during resolution. + #[must_use] + pub fn with_config_file(mut self, enabled: bool) -> Self { + self.use_config_file = enabled; + self + } + + /// Sets the application id used to locate the config file. Set automatically + /// by [`crate::CliConfig::with_environments`]; only call directly in tests. + #[must_use] + pub fn with_app_id(mut self, app_id: impl Into) -> Self { + self.app_id = app_id.into(); + self + } + + /// Test/advanced seam: force the environments file path. + #[must_use] + pub fn with_config_file_path_override(mut self, path: std::path::PathBuf) -> Self { + self.file_path_override = Some(path); + self.use_config_file = true; + self + } + + /// The default environment name. + #[must_use] + pub fn default_env(&self) -> &str { + &self.default + } + + /// Enumerable environment names (compiled-in + file-defined), sorted. + #[must_use] + pub fn list(&self) -> Vec { + let mut names: std::collections::BTreeSet = self.defs.keys().cloned().collect(); + if let Ok(file) = self.file_defs() { + names.extend(file.into_keys()); + } + names.into_iter().collect() + } + + /// Resolves `name` by merging compiled defaults, the config file, + /// and `_*` env-var overrides (later wins) into an [`Environment`]. + /// + /// # Errors + /// + /// Returns an error when `name` is not known to any layer or when the + /// environments file exists but cannot be read or parsed. + pub fn resolve(&self, name: &str) -> Result { + let compiled = self.defs.get(name); + let file = self.file_def(name)?; + if compiled.is_none() && file.is_none() { + return Err(CliCoreError::message(format!( + "unknown environment {name:?}; known: {}", + self.list().join(", ") + ))); + } + let mut merged = EnvironmentDef::default(); + if let Some(def) = compiled { + merge_into(&mut merged, def); + } + if let Some(def) = &file { + merge_into(&mut merged, def); + } + apply_env_vars(name, &mut merged); + Ok(finalize(name, merged)) + } + + /// Path to `environments.toml` next to the engine config file, or `None` + /// when the file layer is disabled or the config dir cannot be determined. + #[must_use] + pub fn config_file_path(&self) -> Option { + if !self.use_config_file { + return None; + } + let config = crate::config::config_file_path(&self.app_id)?; + Some(config.with_file_name("environments.toml")) + } + + fn effective_file_path(&self) -> Option { + if let Some(path) = &self.file_path_override { + return Some(path.clone()); + } + self.config_file_path() + } + + /// Parses the environments file into a name -> def map. Missing file = empty. + fn file_defs(&self) -> Result> { + let Some(path) = self.effective_file_path() else { + return Ok(BTreeMap::new()); + }; + let text = match std::fs::read_to_string(&path) { + Ok(text) => text, + Err(err) if err.kind() == std::io::ErrorKind::NotFound => return Ok(BTreeMap::new()), + Err(err) => { + return Err(CliCoreError::message(format!( + "reading environments file {path:?}: {err}" + ))); + } + }; + toml_edit::de::from_str::>(&text).map_err(|err| { + CliCoreError::message(format!("parsing environments file {path:?}: {err}")) + }) + } + + fn file_def(&self, name: &str) -> Result> { + Ok(self.file_defs()?.remove(name)) + } + + /// Config-file key under which the sticky active environment is stored. + pub(crate) const ACTIVE_ENV_KEY: &'static str = "environment.active"; + + /// Reads the persisted active environment from a loaded config file. + #[must_use] + pub fn active_from_config(config: &crate::config::ConfigFile) -> Option { + config.get(Self::ACTIVE_ENV_KEY) + } + + /// Resolves the active environment name with precedence: + /// explicit `--env` override > persisted active > configured default. + #[must_use] + pub fn effective_active( + &self, + flag: Option<&str>, + config: &crate::config::ConfigFile, + ) -> String { + flag.map(ToOwned::to_owned) + .or_else(|| Self::active_from_config(config)) + .unwrap_or_else(|| self.default.clone()) + } + + /// Persists `name` as the active environment (loads, sets, saves a fresh + /// config file for `app_id`). Validates that `name` resolves first. + /// + /// # Errors + /// + /// Returns an error when `name` does not resolve to a known environment, or + /// when the config file cannot be written. + pub fn persist_active(&self, name: &str) -> Result<()> { + self.resolve(name)?; // reject unknown names + let mut config = crate::config::ConfigFile::load(&self.app_id); + config.set(Self::ACTIVE_ENV_KEY, name)?; + config.save() + } +} + +/// Merges `src` into `dst`, with `src` winning on any field it sets. +fn merge_into(dst: &mut EnvironmentDef, src: &EnvironmentDef) { + if src.client_id.is_some() { + dst.client_id = src.client_id.clone(); + } + if src.auth_url.is_some() { + dst.auth_url = src.auth_url.clone(); + } + if src.token_url.is_some() { + dst.token_url = src.token_url.clone(); + } + if src.scopes.is_some() { + dst.scopes = src.scopes.clone(); + } + for (k, v) in &src.extra { + dst.extra.insert(k.clone(), v.clone()); + } +} + +/// Applies `_*` overrides: the three OAuth fields always, and any bag key +/// already present in the merged record (keyed `_`). +fn apply_env_vars(name: &str, def: &mut EnvironmentDef) { + let prefix = name.to_uppercase().replace('-', "_"); + if let Ok(v) = std::env::var(format!("{prefix}_OAUTH_CLIENT_ID")) { + def.client_id = Some(v); + } + if let Ok(v) = std::env::var(format!("{prefix}_OAUTH_AUTH_URL")) { + def.auth_url = Some(v); + } + if let Ok(v) = std::env::var(format!("{prefix}_OAUTH_TOKEN_URL")) { + def.token_url = Some(v); + } + let keys: Vec = def.extra.keys().cloned().collect(); + for key in keys { + let var = format!("{prefix}_{}", key.to_uppercase().replace('-', "_")); + if let Ok(v) = std::env::var(&var) { + def.extra.insert(key, v); + } + } +} + +/// Turns a fully-merged declaration into a resolved [`Environment`]. OAuth is +/// present when a client id was set by any layer. +fn finalize(name: &str, def: EnvironmentDef) -> Environment { + let oauth = def.client_id.as_ref().map(|client_id| OAuthConfig { + client_id: client_id.clone(), + auth_url: def.auth_url.clone().unwrap_or_default(), + token_url: def.token_url.clone().unwrap_or_default(), + scopes: def.scopes.clone().unwrap_or_default(), + }); + Environment { + name: name.to_owned(), + oauth, + extra: def.extra, + } +} + #[cfg(test)] +#[allow(clippy::unwrap_used, clippy::expect_used, unsafe_code)] mod tests { use super::*; + use std::sync::Mutex; + static ENV_LOCK: Mutex<()> = Mutex::new(()); + + fn sample() -> Environments { + Environments::new("prod") + .with_environment( + "prod", + EnvironmentDef::new() + .with_client_id("prod-client") + .with_auth_url("https://api.example.com/authorize") + .with_token_url("https://api.example.com/token") + .with_scopes(&["openid"]) + .with_field("api_url", "https://api.example.com"), + ) + .with_environment("dev", EnvironmentDef::new().with_client_id("dev-client")) + } + #[test] fn oauth_config_defaults_are_empty() { let c = OAuthConfig::default(); assert!(c.client_id.is_empty() && c.scopes.is_empty()); } + + #[test] + fn builder_registers_compiled_environment() { + let envs = Environments::new("prod").with_environment( + "prod", + EnvironmentDef::new() + .with_client_id("prod-client") + .with_auth_url("https://api.example.com/authorize") + .with_token_url("https://api.example.com/token") + .with_scopes(&["openid"]) + .with_field("api_url", "https://api.example.com"), + ); + assert_eq!(envs.default_env(), "prod"); + assert_eq!(envs.list(), vec!["prod".to_owned()]); + } + + #[test] + fn resolve_returns_compiled_record() { + let _g = ENV_LOCK + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner); + let env = sample().resolve("prod").expect("prod resolves"); + let oauth = env.oauth.expect("oauth present"); + assert_eq!(oauth.client_id, "prod-client"); + assert_eq!(oauth.scopes, vec!["openid".to_owned()]); + assert_eq!( + env.extra.get("api_url").map(String::as_str), + Some("https://api.example.com") + ); + } + + #[test] + fn resolve_unknown_env_errors_with_known_names() { + let _g = ENV_LOCK + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner); + let err = sample().resolve("nope").unwrap_err().to_string(); + assert!(err.contains("nope")); + assert!(err.contains("prod") && err.contains("dev")); + } + + #[test] + fn env_var_layer_overrides_oauth_and_known_bag_keys() { + let _g = ENV_LOCK + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner); + // SAFETY: serialized by ENV_LOCK; removed below. + unsafe { + std::env::set_var("PROD_OAUTH_CLIENT_ID", "override-client"); + std::env::set_var("PROD_API_URL", "https://api.override.example.com"); + } + let env = sample().resolve("prod").expect("prod resolves"); + assert_eq!(env.oauth.unwrap().client_id, "override-client"); + assert_eq!( + env.extra.get("api_url").map(String::as_str), + Some("https://api.override.example.com") + ); + unsafe { + std::env::remove_var("PROD_OAUTH_CLIENT_ID"); + std::env::remove_var("PROD_API_URL"); + } + } + + #[test] + fn environments_file_path_sits_next_to_config() { + let envs = sample().with_app_id("gddy").with_config_file(true); + let path = envs.config_file_path().expect("path resolves with app id"); + assert!(path.ends_with("gddy/environments.toml"), "got {path:?}"); + } + + #[test] + fn file_layer_overrides_compiled_and_adds_custom_env() { + let _g = ENV_LOCK + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner); + let dir = tempfile::tempdir().expect("tempdir"); + let file = dir.path().join("environments.toml"); + std::fs::write( + &file, + r#" +[prod] +client_id = "file-client" + +[custom] +client_id = "custom-client" +api_url = "https://api.custom.example.com" +"#, + ) + .expect("write file"); + + let envs = sample() + .with_config_file(true) + .with_config_file_path_override(file); + + // File overrides the compiled prod client id, keeps compiled api_url. + let prod = envs.resolve("prod").expect("prod"); + assert_eq!(prod.oauth.unwrap().client_id, "file-client"); + assert_eq!( + prod.extra.get("api_url").map(String::as_str), + Some("https://api.example.com") + ); + + // Custom env exists only in the file. + let custom = envs.resolve("custom").expect("custom"); + assert_eq!(custom.oauth.unwrap().client_id, "custom-client"); + assert!(envs.list().contains(&"custom".to_owned())); + } + + const ACTIVE_KEY: &str = "environment.active"; + + #[test] + fn active_env_round_trips_through_config_file() { + use crate::config::ConfigFile; + let mut cfg = ConfigFile::default(); + assert_eq!(Environments::active_from_config(&cfg), None); + + cfg.set(ACTIVE_KEY, "ote").expect("set"); + assert_eq!( + Environments::active_from_config(&cfg).as_deref(), + Some("ote") + ); + } + + #[test] + fn effective_active_prefers_override_then_config_then_default() { + use crate::config::ConfigFile; + let envs = sample(); + let mut cfg = ConfigFile::default(); + cfg.set(ACTIVE_KEY, "dev").expect("set"); + + assert_eq!(envs.effective_active(Some("prod"), &cfg), "prod"); // explicit wins + assert_eq!(envs.effective_active(None, &cfg), "dev"); // config next + let empty = ConfigFile::default(); + assert_eq!(envs.effective_active(None, &empty), "prod"); // default last + } } diff --git a/src/lib.rs b/src/lib.rs index c3dca91..7549a48 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -122,7 +122,7 @@ pub use config::{ credential_store_env_var, resolve_credential_store, resolve_credential_store_with, }; pub use config_commands::config_command_group; -pub use environments::{Environment, OAuthConfig}; +pub use environments::{Environment, EnvironmentDef, Environments, OAuthConfig}; pub use error::{ CliCoreError, DetailedError, ExitCoder, Result, exit_code_for_error, exit_code_for_exit_coder, }; From 98a7f1d1fa4d5b392ac6b90062f186027ce009d5 Mon Sep 17 00:00:00 2001 From: Jacob Page Date: Wed, 17 Jun 2026 10:51:51 -0700 Subject: [PATCH 04/31] feat(environments): layered resolve with env-var overrides, file layer, and active-env persistence Tasks 3-6: resolve() with env-var override layer, environments.toml path helper, file-based resolution layer with path override seam, and active-env read/write via ConfigFile. Also fix broken intra-doc link forward-referencing with_environments. Co-Authored-By: Claude Sonnet 4.6 --- src/environments.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/environments.rs b/src/environments.rs index 42cc1fc..87ab50c 100644 --- a/src/environments.rs +++ b/src/environments.rs @@ -134,7 +134,7 @@ impl Environments { } /// Sets the application id used to locate the config file. Set automatically - /// by [`crate::CliConfig::with_environments`]; only call directly in tests. + /// by `CliConfig::with_environments`; only call directly in tests. #[must_use] pub fn with_app_id(mut self, app_id: impl Into) -> Self { self.app_id = app_id.into(); From 1d03e0675e5406ff0ad2c2cecf6ce7f59b0d8db6 Mon Sep 17 00:00:00 2001 From: Jacob Page Date: Wed, 17 Jun 2026 11:04:53 -0700 Subject: [PATCH 05/31] =?UTF-8?q?refactor(environments):=20address=20revie?= =?UTF-8?q?w=20=E2=80=94=20non=5Fexhaustive,=20partial-oauth=20docs,=20pan?= =?UTF-8?q?ic-safe=20test?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-Authored-By: Claude Opus 4.8 --- src/environments.rs | 97 +++++++++++++++++++++++++++++++++++---------- 1 file changed, 76 insertions(+), 21 deletions(-) diff --git a/src/environments.rs b/src/environments.rs index 87ab50c..200353d 100644 --- a/src/environments.rs +++ b/src/environments.rs @@ -11,6 +11,11 @@ use serde::Deserialize; use crate::{Result, error::CliCoreError}; /// Standard OAuth slice of an environment, consumed by `PkceAuthProvider`. +/// +/// `auth_url`, `token_url`, and `scopes` may be empty when a layer set only +/// `client_id`. Consumers should treat empty endpoint strings as "fall back to +/// the provider's default base endpoints". +#[non_exhaustive] #[derive(Clone, Debug, Default, PartialEq, Eq)] pub struct OAuthConfig { /// OAuth client id. @@ -24,6 +29,7 @@ pub struct OAuthConfig { } /// A fully-resolved environment. +#[non_exhaustive] #[derive(Clone, Debug, PartialEq, Eq)] pub struct Environment { /// Environment name (e.g. `prod`). @@ -156,6 +162,9 @@ impl Environments { } /// Enumerable environment names (compiled-in + file-defined), sorted. + /// + /// File parse errors are silently swallowed; compiled names are still + /// returned. A fallible variant can be added later if needed. #[must_use] pub fn list(&self) -> Vec { let mut names: std::collections::BTreeSet = self.defs.keys().cloned().collect(); @@ -168,17 +177,27 @@ impl Environments { /// Resolves `name` by merging compiled defaults, the config file, /// and `_*` env-var overrides (later wins) into an [`Environment`]. /// + /// When only `client_id` was set on the matching layer(s), the returned + /// [`Environment`]'s `oauth.auth_url` / `oauth.token_url` will be empty + /// strings. Consumers should treat empty endpoint strings as "fall back to + /// the provider's default base endpoints". + /// /// # Errors /// /// Returns an error when `name` is not known to any layer or when the /// environments file exists but cannot be read or parsed. pub fn resolve(&self, name: &str) -> Result { let compiled = self.defs.get(name); - let file = self.file_def(name)?; + // Parse the file once; reuse for both membership check and merge. + let mut all_file_defs = self.file_defs()?; + let file = all_file_defs.remove(name); if compiled.is_none() && file.is_none() { + let mut known: std::collections::BTreeSet = self.defs.keys().cloned().collect(); + known.extend(all_file_defs.into_keys()); + let known_list: Vec = known.into_iter().collect(); return Err(CliCoreError::message(format!( "unknown environment {name:?}; known: {}", - self.list().join(", ") + known_list.join(", ") ))); } let mut merged = EnvironmentDef::default(); @@ -229,10 +248,6 @@ impl Environments { }) } - fn file_def(&self, name: &str) -> Result> { - Ok(self.file_defs()?.remove(name)) - } - /// Config-file key under which the sticky active environment is stored. pub(crate) const ACTIVE_ENV_KEY: &'static str = "environment.active"; @@ -291,6 +306,12 @@ fn merge_into(dst: &mut EnvironmentDef, src: &EnvironmentDef) { /// Applies `_*` overrides: the three OAuth fields always, and any bag key /// already present in the merged record (keyed `_`). +/// +/// The prefix is `name.to_uppercase().replace('-', "_")`, so environment names +/// that differ only by `-` vs `_` map to the same prefix and will collide. +/// +/// Scopes are intentionally not env-var overridable; set them via the +/// compiled-in layer or the `environments.toml` file. fn apply_env_vars(name: &str, def: &mut EnvironmentDef) { let prefix = name.to_uppercase().replace('-', "_"); if let Ok(v) = std::env::var(format!("{prefix}_OAUTH_CLIENT_ID")) { @@ -314,16 +335,23 @@ fn apply_env_vars(name: &str, def: &mut EnvironmentDef) { /// Turns a fully-merged declaration into a resolved [`Environment`]. OAuth is /// present when a client id was set by any layer. fn finalize(name: &str, def: EnvironmentDef) -> Environment { - let oauth = def.client_id.as_ref().map(|client_id| OAuthConfig { - client_id: client_id.clone(), - auth_url: def.auth_url.clone().unwrap_or_default(), - token_url: def.token_url.clone().unwrap_or_default(), - scopes: def.scopes.clone().unwrap_or_default(), + let EnvironmentDef { + client_id, + auth_url, + token_url, + scopes, + extra, + } = def; + let oauth = client_id.map(|id| OAuthConfig { + client_id: id, + auth_url: auth_url.unwrap_or_default(), + token_url: token_url.unwrap_or_default(), + scopes: scopes.unwrap_or_default(), }); Environment { name: name.to_owned(), oauth, - extra: def.extra, + extra, } } @@ -335,6 +363,15 @@ mod tests { use std::sync::Mutex; static ENV_LOCK: Mutex<()> = Mutex::new(()); + /// RAII guard that removes an env var on drop, even if a test panics. + struct EnvGuard(&'static str); + impl Drop for EnvGuard { + fn drop(&mut self) { + // SAFETY: test holds ENV_LOCK; clean up on any exit including panic. + unsafe { std::env::remove_var(self.0) } + } + } + fn sample() -> Environments { Environments::new("prod") .with_environment( @@ -395,26 +432,44 @@ mod tests { assert!(err.contains("prod") && err.contains("dev")); } + #[test] + fn resolve_with_only_client_id_yields_partial_oauth() { + let _g = ENV_LOCK + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner); + let envs = Environments::new("dev") + .with_environment("dev", EnvironmentDef::new().with_client_id("dev-only")); + let env = envs.resolve("dev").expect("dev resolves"); + let oauth = env.oauth.expect("oauth present when client_id is set"); + assert_eq!(oauth.client_id, "dev-only"); + assert!( + oauth.auth_url.is_empty(), + "auth_url should be empty (fall back to provider default)" + ); + assert!( + oauth.token_url.is_empty(), + "token_url should be empty (fall back to provider default)" + ); + assert!(oauth.scopes.is_empty()); + } + #[test] fn env_var_layer_overrides_oauth_and_known_bag_keys() { let _g = ENV_LOCK .lock() .unwrap_or_else(std::sync::PoisonError::into_inner); - // SAFETY: serialized by ENV_LOCK; removed below. - unsafe { - std::env::set_var("PROD_OAUTH_CLIENT_ID", "override-client"); - std::env::set_var("PROD_API_URL", "https://api.override.example.com"); - } + // SAFETY: serialized by ENV_LOCK; guards remove vars on any exit incl. panic. + unsafe { std::env::set_var("PROD_OAUTH_CLIENT_ID", "override-client") }; + let _g1 = EnvGuard("PROD_OAUTH_CLIENT_ID"); + unsafe { std::env::set_var("PROD_API_URL", "https://api.override.example.com") }; + let _g2 = EnvGuard("PROD_API_URL"); + let env = sample().resolve("prod").expect("prod resolves"); assert_eq!(env.oauth.unwrap().client_id, "override-client"); assert_eq!( env.extra.get("api_url").map(String::as_str), Some("https://api.override.example.com") ); - unsafe { - std::env::remove_var("PROD_OAUTH_CLIENT_ID"); - std::env::remove_var("PROD_API_URL"); - } } #[test] From b19e17501ed7814b61aa2693ac17ff225fd5a4d2 Mon Sep 17 00:00:00 2001 From: Jacob Page Date: Wed, 17 Jun 2026 11:10:41 -0700 Subject: [PATCH 06/31] feat(environments): thread environments through middleware Co-Authored-By: Claude Opus 4.8 --- src/middleware.rs | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/src/middleware.rs b/src/middleware.rs index 98204ad..ec0c276 100644 --- a/src/middleware.rs +++ b/src/middleware.rs @@ -514,6 +514,13 @@ pub struct Middleware { /// module registration via /// [`ModuleContext::config`](crate::module::ModuleContext::config). pub config: Arc, + /// Optional first-class environment system. + /// + /// Set by [`CliConfig::with_environments`](crate::CliConfig::with_environments) + /// and cloned into each per-run middleware snapshot. Handlers resolve the + /// active environment through + /// [`CommandContext::environment`](crate::command::CommandContext::environment). + pub environments: Option>, } /// Rendered result produced by middleware. @@ -1054,3 +1061,22 @@ impl From for Value { Value::String(error.to_string()) } } + +#[cfg(test)] +mod env_wire_tests { + use super::*; + + #[test] + fn middleware_carries_optional_environments() { + use std::sync::Arc; + let mut mw = Middleware::new(); + assert!(mw.environments.is_none()); + mw.environments = Some(Arc::new(crate::environments::Environments::new("prod"))); + assert_eq!( + mw.environments + .as_ref() + .map(|envs| envs.default_env().to_owned()), + Some("prod".to_owned()) + ); + } +} From 0d86f600a2fddc9c6ba8d73e2c0104617bb72d0b Mon Sep 17 00:00:00 2001 From: Jacob Page Date: Wed, 17 Jun 2026 11:12:53 -0700 Subject: [PATCH 07/31] feat(environments): add CliConfig::with_environments Co-Authored-By: Claude Opus 4.8 --- src/cli.rs | 38 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 38 insertions(+) diff --git a/src/cli.rs b/src/cli.rs index f8f049b..2e6d31b 100644 --- a/src/cli.rs +++ b/src/cli.rs @@ -254,6 +254,13 @@ pub struct CliConfig { /// to a binary that never opted in. Populate via [`CliConfig::with_argv0_alias`] /// and [`CliConfig::with_argv0_personality`]. pub argv0_routes: BTreeMap, + /// Optional first-class environment system. + /// + /// Registered via [`CliConfig::with_environments`]. When set, the engine + /// registers a global `--env` flag, seeds the active environment into + /// middleware, and exposes it to handlers through + /// [`CommandContext::environment`](crate::command::CommandContext::environment). + pub environments: Option, } impl CliConfig { @@ -293,6 +300,22 @@ impl CliConfig { self } + /// Registers a first-class environment system. + /// + /// When set, [`Cli::new`] registers a global `--env` flag, seeds the active + /// environment into middleware (explicit `--env` > persisted active > + /// configured default), and exposes the resolved environment to handlers via + /// [`CommandContext::environment`](crate::command::CommandContext::environment). + /// + /// The provided [`Environments`](crate::environments::Environments) has this + /// config's `app_id` applied so its config file and active-env persistence + /// resolve to the application's config directory. + #[must_use] + pub fn with_environments(mut self, environments: crate::environments::Environments) -> Self { + self.environments = Some(environments.with_app_id(self.app_id.clone())); + self + } + /// Overrides the outbound User-Agent string for all HTTP traffic. /// /// When unset, the engine derives `name/version` from this config (see @@ -2875,3 +2898,18 @@ mod user_agent_tests { crate::transport::set_default_user_agent("cli/dev"); } } + +#[cfg(test)] +mod env_config_tests { + use super::*; + + #[test] + fn with_environments_stores_and_app_id_is_injected() { + let cfg = CliConfig::new("gddy", "GoDaddy CLI", "gddy").with_environments( + crate::environments::Environments::new("prod").with_config_file(true), + ); + let envs = cfg.environments.as_ref().expect("environments set"); + // app_id is stamped from CliConfig so the file path resolves. + assert!(envs.config_file_path().is_some()); + } +} From 4f3468ae50586cc9c2cba9bc8ecc35ecbb6d35f3 Mon Sep 17 00:00:00 2001 From: Jacob Page Date: Wed, 17 Jun 2026 11:17:03 -0700 Subject: [PATCH 08/31] feat(environments): add CommandContext::environment accessor Co-Authored-By: Claude Opus 4.8 --- src/command.rs | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/src/command.rs b/src/command.rs index 5679a4b..4322a1d 100644 --- a/src/command.rs +++ b/src/command.rs @@ -127,6 +127,27 @@ impl CommandContext { &self.middleware.config } + /// Resolves the active [`Environment`](crate::environments::Environment) for + /// this invocation. + /// + /// The active environment name is `self.middleware.env`, seeded at startup + /// from the persisted active environment or configured default and + /// overridden per invocation by the global `--env` flag. Resolution merges + /// the compiled-in definition, the `environments.toml` file layer, and + /// `_*` environment-variable overrides. + /// + /// # Errors + /// + /// Returns an error if no environment system was registered via + /// [`CliConfig::with_environments`](crate::CliConfig::with_environments) or + /// if the active name does not resolve to a known environment. + pub fn environment(&self) -> Result { + let environments = self.middleware.environments.as_ref().ok_or_else(|| { + crate::error::CliCoreError::message("no environment system configured") + })?; + environments.resolve(&self.middleware.env) + } + /// Deserializes the raw argument matches into a typed args struct. /// /// Use this with `#[derive(clap::Args)]` structs to get type-safe access From 36bf86eb771fbbbc3bc9066492cffac90acadbfe Mon Sep 17 00:00:00 2001 From: Jacob Page Date: Wed, 17 Jun 2026 11:17:03 -0700 Subject: [PATCH 09/31] feat(environments): register --env and seed active env into middleware Co-Authored-By: Claude Opus 4.8 --- src/cli.rs | 83 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 83 insertions(+) diff --git a/src/cli.rs b/src/cli.rs index 2e6d31b..47610ab 100644 --- a/src/cli.rs +++ b/src/cli.rs @@ -747,6 +747,15 @@ impl Cli { if let Some(register_flags) = &config.register_flags { root = register_flags(root); } + if config.environments.is_some() { + root = root.arg( + clap::Arg::new("env") + .long("env") + .global(true) + .value_name("ENV") + .help("Target environment"), + ); + } let intro = config .long .as_deref() @@ -771,6 +780,13 @@ impl Cli { middleware .human_views .merge(&global_human_view_registry_snapshot()); + if let Some(environments) = &config.environments { + let environments = Arc::new(environments.clone()); + // Seed the sticky/default active environment now; the global `--env` + // flag overrides it per invocation in `run_with_depth`. + middleware.env = environments.effective_active(None, &middleware.config); + middleware.environments = Some(environments); + } let mut cli = Self { config, @@ -1365,6 +1381,11 @@ impl Cli { if let Err(err) = self.apply_config_flags(&matches, &mut middleware) { return self.finish_run(render_cli_error(&middleware, &err, &self.config.app_id)); } + // Validate and apply `--env` for built-in paths (help/tree/guide/group + // help) so they reflect the selected environment and reject unknowns. + if let Err(err) = self.apply_env_flag(&matches, &mut middleware) { + return self.finish_run(render_cli_error(&middleware, &err, &self.config.app_id)); + } let command_path = command_path_from_matches(&self.config.name, &matches); if command_path == "help" { @@ -1447,6 +1468,11 @@ impl Cli { if let Err(err) = self.apply_config_flags(&matches, &mut middleware) { return self.finish_run(render_cli_error(&middleware, &err, &self.config.app_id)); } + // The global `--env` flag overrides the seeded active environment for + // this invocation; an unknown name surfaces as an error envelope. + if let Err(err) = self.apply_env_flag(&matches, &mut middleware) { + return self.finish_run(render_cli_error(&middleware, &err, &self.config.app_id)); + } let leaf = leaf_matches(&matches); let args = command_args_from_matches(leaf, &command.spec, false); @@ -1929,6 +1955,23 @@ impl Cli { Ok(()) } + /// Applies the global `--env` override to a per-run middleware snapshot. + /// + /// The flag is only registered when environments are configured, so when it + /// is present `middleware.environments` is set too. Validates the requested + /// name against the registered environments and updates `middleware.env`, + /// returning an error for an unknown environment. + fn apply_env_flag(&self, matches: &ArgMatches, middleware: &mut Middleware) -> Result<()> { + if let (Some(env), Some(environments)) = ( + matches.get_one::("env"), + middleware.environments.clone(), + ) { + environments.resolve(env)?; + middleware.env = env.clone(); + } + Ok(()) + } + fn run_pre_run( &self, middleware: &mut Middleware, @@ -2912,4 +2955,44 @@ mod env_config_tests { // app_id is stamped from CliConfig so the file path resolves. assert!(envs.config_file_path().is_some()); } + + #[tokio::test] + async fn env_flag_overrides_default_and_reaches_middleware_env() { + use crate::{CommandResult, CommandSpec, RuntimeCommandSpec}; + use serde_json::json; + let mut cli = Cli::new( + CliConfig::new("envtest", "Env test", "envtest").with_environments( + crate::environments::Environments::new("prod") + .with_environment("prod", crate::environments::EnvironmentDef::new()) + .with_environment("ote", crate::environments::EnvironmentDef::new()), + ), + ); + cli.add_command(RuntimeCommandSpec::new_with_context( + CommandSpec::new("whichenv", "echo env").no_auth(true), + async |ctx| { + Ok(CommandResult::new( + json!({ "env": ctx.environment()?.name }), + )) + }, + )); + let out = cli + .run(["envtest", "whichenv", "--env", "ote", "--output", "json"]) + .await; + assert_eq!(out.exit_code, 0, "rendered: {}", out.rendered); + assert!(out.rendered.contains("\"env\"")); + assert!(out.rendered.contains("ote")); + } + + #[tokio::test] + async fn unknown_env_flag_produces_error_envelope() { + let cli = Cli::new( + CliConfig::new("envtest2", "Env test", "envtest2").with_environments( + crate::environments::Environments::new("prod") + .with_environment("prod", crate::environments::EnvironmentDef::new()), + ), + ); + let out = cli.run(["envtest2", "tree", "--env", "nope"]).await; + assert_ne!(out.exit_code, 0); + assert!(out.rendered.contains("nope")); + } } From 2fd09c6d704d2940d780c8d44567a77bf6f4c37e Mon Sep 17 00:00:00 2001 From: Jacob Page Date: Wed, 17 Jun 2026 11:28:37 -0700 Subject: [PATCH 10/31] feat(environments): add built-in env command group Mount env list/get/set/info automatically when CliConfig::with_environments is used. Mirrors the ensure_config_command pattern exactly. Integration tests cover list/get/info; set persistence is already unit-tested in environments.rs (Task 6). Co-Authored-By: Claude Sonnet 4.6 --- src/cli.rs | 35 +++++++++++++++++ src/env_commands.rs | 91 +++++++++++++++++++++++++++++++++++++++++++++ src/lib.rs | 2 + tests/foundation.rs | 50 +++++++++++++++++++++++++ 4 files changed, 178 insertions(+) create mode 100644 src/env_commands.rs diff --git a/src/cli.rs b/src/cli.rs index 47610ab..ae79020 100644 --- a/src/cli.rs +++ b/src/cli.rs @@ -828,6 +828,9 @@ impl Cli { if cli.config.config_commands { cli.ensure_config_command(); } + if cli.config.environments.is_some() { + cli.ensure_env_command(); + } cli } @@ -1917,6 +1920,38 @@ impl Cli { self.refresh_root_long(); } + /// Mounts the built-in `env` command group and files it under the admin + /// help category. Idempotent and yields to a consumer-defined `env` + /// subcommand if one already exists. + fn ensure_env_command(&mut self) { + if has_subcommand(&self.root, "env") { + return; + } + let group = crate::env_commands::env_command_group(); + let mut prefix = Vec::new(); + group.register_commands(&mut prefix, &mut self.commands); + let mut prefix = Vec::new(); + let clap_group = runtime_group_clap_command_with_schema_help( + &group, + &mut prefix, + &self.middleware.schema_registry, + ); + self.root = self.root.clone().subcommand(clap_group); + let category = self + .config + .admin_category + .clone() + .unwrap_or_else(|| DEFAULT_ADMIN_CATEGORY.to_owned()); + if !self.module_entries.iter().any(|e| e.name == "env") { + self.module_entries.push(ModuleHelpEntry { + category, + name: "env".to_owned(), + short: "Manage the active environment".to_owned(), + }); + } + self.refresh_root_long(); + } + fn default_auth_provider(&self) -> String { if !self.middleware.default_auth_provider.is_empty() { return self.middleware.default_auth_provider.clone(); diff --git a/src/env_commands.rs b/src/env_commands.rs new file mode 100644 index 0000000..b8a4006 --- /dev/null +++ b/src/env_commands.rs @@ -0,0 +1,91 @@ +//! Built-in `env` command group: list/get/set/info for environments. +//! +//! Mounted automatically when [`crate::CliConfig::with_environments`] is used. +//! The group exposes: +//! +//! - `env list` — list known environments with active flag. +//! - `env get` — print the active environment name. +//! - `env set ` — persist the active environment. +//! - `env info` — show the fully resolved active environment. + +use serde_json::json; + +use crate::{ + CommandResult, CommandSpec, GroupSpec, RuntimeCommandSpec, RuntimeGroupSpec, + error::CliCoreError, +}; + +/// Builds the built-in `env` command group. +#[must_use] +pub fn env_command_group() -> RuntimeGroupSpec { + RuntimeGroupSpec::new(GroupSpec::new("env", "Manage the active environment")) + .with_command(RuntimeCommandSpec::new_with_context( + CommandSpec::new("list", "List known environments").no_auth(true), + async |ctx| { + let envs = ctx + .middleware + .environments + .as_ref() + .ok_or_else(|| CliCoreError::message("no environment system configured"))?; + let active = ctx.middleware.env.clone(); + let items: Vec<_> = envs + .list() + .into_iter() + .map(|name| { + let is_active = name == active; + json!({ "name": name, "active": is_active }) + }) + .collect(); + Ok(CommandResult::new(json!(items))) + }, + )) + .with_command(RuntimeCommandSpec::new_with_context( + CommandSpec::new("get", "Show the active environment name").no_auth(true), + async |ctx| Ok(CommandResult::new(json!({ "active": ctx.middleware.env }))), + )) + .with_command(RuntimeCommandSpec::new_with_context( + CommandSpec::new("info", "Show the resolved active environment").no_auth(true), + async |ctx| { + let env = ctx.environment()?; + let oauth = env.oauth.map(|o| { + json!({ + "client_id": o.client_id, + "auth_url": o.auth_url, + "token_url": o.token_url, + "scopes": o.scopes, + }) + }); + Ok(CommandResult::new(json!({ + "name": env.name, + "oauth": oauth, + "extra": env.extra, + }))) + }, + )) + .with_command(RuntimeCommandSpec::new_with_context( + CommandSpec::new("set", "Set and persist the active environment") + .no_auth(true) + .with_arg(clap::Arg::new("name").required(true)), + async |ctx| { + let envs = ctx + .middleware + .environments + .as_ref() + .ok_or_else(|| CliCoreError::message("no environment system configured"))?; + let name = string_arg(&ctx.args, "name"); + if name.is_empty() { + return Err(CliCoreError::message("missing environment name")); + } + envs.persist_active(&name)?; + Ok(CommandResult::new(json!({ "active": name }))) + }, + )) +} + +/// Reads a required string argument, defaulting to empty when absent. +fn string_arg(args: &serde_json::Map, name: &str) -> String { + args.get(name) + .and_then(serde_json::Value::as_str) + .unwrap_or_default() + .to_owned() +} diff --git a/src/lib.rs b/src/lib.rs index 7549a48..520437d 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -71,6 +71,8 @@ pub mod command; pub mod config; /// Built-in `config` command group. pub mod config_commands; +/// Built-in `env` command group (private; only `cli.rs` consumes it). +mod env_commands; /// First-class environment definitions and layered resolution. pub mod environments; /// Shared error type and error traits. diff --git a/tests/foundation.rs b/tests/foundation.rs index b0e88aa..dcabf6e 100644 --- a/tests/foundation.rs +++ b/tests/foundation.rs @@ -9900,6 +9900,56 @@ async fn auth_registered_after_construction_is_categorized() { ); } +#[tokio::test] +async fn env_group_lists_gets_and_shows_info_for_active_environment() { + use cli_engine::environments::{EnvironmentDef, Environments}; + + let cli = Cli::new( + CliConfig::new("envcmds", "Env cmds", "envcmds").with_environments( + Environments::new("prod") + .with_environment( + "prod", + EnvironmentDef::new().with_field("api_url", "https://p"), + ) + .with_environment( + "ote", + EnvironmentDef::new().with_field("api_url", "https://o"), + ), + ), + ); + + // env list returns both environments. + let list = cli + .run(["envcmds", "env", "list", "--output", "json"]) + .await; + assert_eq!(list.exit_code, 0, "env list failed: {}", list.rendered); + assert!( + list.rendered.contains("prod") && list.rendered.contains("ote"), + "env list missing environments: {}", + list.rendered + ); + + // env get returns the default active environment. + let get = cli.run(["envcmds", "env", "get", "--output", "json"]).await; + assert_eq!(get.exit_code, 0, "env get failed: {}", get.rendered); + assert!( + get.rendered.contains("prod"), + "env get missing default env: {}", + get.rendered + ); + + // env info --env ote shows ote's extra fields. + let info = cli + .run(["envcmds", "env", "info", "--env", "ote", "--output", "json"]) + .await; + assert_eq!(info.exit_code, 0, "env info failed: {}", info.rendered); + assert!( + info.rendered.contains("https://o"), + "env info missing ote api_url: {}", + info.rendered + ); +} + #[derive(Debug)] struct RecordingEnvProvider { envs: Arc>>, From d8c1a029b0769bf8549a18fefb8dc8001bc4e576 Mon Sep 17 00:00:00 2001 From: Jacob Page Date: Wed, 17 Jun 2026 11:40:04 -0700 Subject: [PATCH 11/31] feat(environments)!: source PkceAuthProvider OAuth from Environments Replace the unreleased per-env OAuthEnvironment/with_environment builder with a shared Environments resolver. with_environments(Arc) wires the provider; effective_* prefers the resolved OAuthConfig field when non-empty, then the legacy _OAUTH_* env var, then base config. Empty resolved fields fall through so a partial env inherits base endpoints. Co-Authored-By: Claude Opus 4.8 --- src/auth/pkce.rs | 275 +++++++++++++++++++++-------------------------- 1 file changed, 123 insertions(+), 152 deletions(-) diff --git a/src/auth/pkce.rs b/src/auth/pkce.rs index b514cfc..4853511 100644 --- a/src/auth/pkce.rs +++ b/src/auth/pkce.rs @@ -41,7 +41,15 @@ //! .with_auth_provider(provider); //! ``` //! -//! Override endpoints and client ID via environment variables: +//! For per-environment OAuth config (different client id or endpoints per env), +//! wire the provider to a shared +//! [`Environments`](crate::environments::Environments) with +//! [`PkceAuthProvider::with_environments`](crate::auth::pkce::PkceAuthProvider::with_environments); +//! the resolved environment then drives the OAuth config for the active `env`. +//! +//! Without a wired resolver (or for a field the resolved environment leaves +//! empty), endpoints and client ID can still be overridden via environment +//! variables: //! - `_OAUTH_CLIENT_ID` //! - `_OAUTH_AUTH_URL` //! - `_OAUTH_TOKEN_URL` @@ -127,74 +135,6 @@ impl StoredToken { } } -/// Per-environment OAuth configuration for [`PkceAuthProvider`]. -/// -/// A single provider often serves several environments (for example `dev`, -/// `test`, and `prod`) that each use a different OAuth client id and set of -/// endpoints. Register one of these per environment with -/// [`PkceAuthProvider::with_environment`]. Any field left unset falls back to -/// the provider's base configuration, so an override can change just the client -/// id while reusing the base endpoints, or vice versa. -/// -/// # Examples -/// -/// ``` -/// use cli_engine::auth::pkce::OAuthEnvironment; -/// -/// let prod = OAuthEnvironment::new() -/// .with_client_id("prod-client-id") -/// .with_auth_url("https://api.example.com/v2/oauth2/authorize") -/// .with_token_url("https://api.example.com/v2/oauth2/token"); -/// # let _ = prod; -/// ``` -#[derive(Debug, Clone, Default)] -pub struct OAuthEnvironment { - client_id: Option, - auth_url: Option, - token_url: Option, - scopes: Option>, -} - -impl OAuthEnvironment { - /// Creates an empty override. Every field falls back to the provider's base - /// configuration until set. - #[must_use] - pub fn new() -> Self { - Self::default() - } - - /// Overrides the OAuth client id for this environment. - #[must_use] - pub fn with_client_id(mut self, client_id: impl Into) -> Self { - self.client_id = Some(client_id.into()); - self - } - - /// Overrides the authorization endpoint for this environment. - #[must_use] - pub fn with_auth_url(mut self, auth_url: impl Into) -> Self { - self.auth_url = Some(auth_url.into()); - self - } - - /// Overrides the token endpoint for this environment. - #[must_use] - pub fn with_token_url(mut self, token_url: impl Into) -> Self { - self.token_url = Some(token_url.into()); - self - } - - /// Overrides the default scopes requested for this environment. - /// - /// Replaces the base scope set for this environment rather than extending - /// it. Leave unset to reuse the provider's base scopes. - #[must_use] - pub fn with_scopes(mut self, scopes: &[impl AsRef]) -> Self { - self.scopes = Some(scopes.iter().map(|s| s.as_ref().to_owned()).collect()); - self - } -} - /// OAuth 2.0 PKCE authentication provider. /// /// Stores one token per `(env, provider)` pair in the system keychain. @@ -206,10 +146,10 @@ pub struct PkceAuthProvider { token_url: String, client_id: String, scopes: Vec, - /// Per-environment configuration overrides keyed by env name. Looked up by - /// the `env` passed to [`AuthProvider::get_credential`]; unmatched envs use - /// the base `client_id`/`auth_url`/`token_url`/`scopes` above. - environments: HashMap, + /// Optional environment resolver; when set, per-env OAuth config comes from + /// the resolved environment instead of the base config / legacy env override. + /// Looked up by the `env` passed to [`AuthProvider::get_credential`]. + environments: Option>, redirect_port: u16, redirect_uri: Option, /// Timeout applied to token-endpoint requests (exchange and refresh). @@ -266,7 +206,7 @@ impl PkceAuthProvider { token_url: token_url.into(), client_id: client_id.into(), scopes: scopes.iter().map(|s| s.as_ref().to_owned()).collect(), - environments: HashMap::new(), + environments: None, redirect_port: REDIRECT_PORT_DEFAULT, redirect_uri: None, token_timeout: TOKEN_REQUEST_TIMEOUT_DEFAULT, @@ -284,25 +224,42 @@ impl PkceAuthProvider { } } - /// Registers per-environment OAuth configuration. + /// Sources per-environment OAuth config from a shared + /// [`Environments`](crate::environments::Environments). /// - /// Use this when one provider serves several environments that each use a - /// different OAuth client id or set of endpoints (for example `dev`, - /// `test`, and `prod`). The override is selected by the `env` argument - /// passed to [`AuthProvider::get_credential`]; environments without an - /// override use the base configuration supplied to - /// [`PkceAuthProvider::new`]. Registering the same env twice replaces the - /// earlier override. + /// Given an `env`, the provider resolves the environment and uses its + /// [`OAuthConfig`](crate::environments::OAuthConfig). This is the + /// single-source-of-truth path; prefer it over the base + /// `client_id`/`auth_url`/`token_url` when the consumer registers + /// environments via + /// [`CliConfig::with_environments`](crate::CliConfig::with_environments). /// - /// Provider-prefixed environment variables - /// (`_OAUTH_CLIENT_ID`, `_AUTH_URL`, `_TOKEN_URL`) still take - /// precedence over both the per-environment override and the base - /// configuration, so operators retain a single escape hatch. + /// Precedence per OAuth field, for the resolved env: the resolved + /// environment's value when non-empty; otherwise the legacy + /// provider-prefixed env var (`_OAUTH_CLIENT_ID`, `_AUTH_URL`, + /// `_TOKEN_URL`); otherwise the base configuration supplied to + /// [`PkceAuthProvider::new`]. An empty field on the resolved environment + /// falls through, so a partial environment can override only the client id + /// while inheriting the provider's base endpoints. /// /// # Examples /// /// ``` - /// use cli_engine::auth::pkce::{OAuthEnvironment, PkceAuthProvider}; + /// use std::sync::Arc; + /// use cli_engine::{ + /// auth::pkce::PkceAuthProvider, + /// environments::{EnvironmentDef, Environments}, + /// }; + /// + /// let environments = Arc::new( + /// Environments::new("prod").with_environment( + /// "dev", + /// EnvironmentDef::new() + /// .with_client_id("dev-client-id") + /// .with_auth_url("https://api.dev-godaddy.com/v2/oauth2/authorize") + /// .with_token_url("https://api.dev-godaddy.com/v2/oauth2/token"), + /// ), + /// ); /// /// let provider = PkceAuthProvider::new( /// "godaddy", @@ -311,18 +268,15 @@ impl PkceAuthProvider { /// "prod-client-id", /// &["openid", "profile"], /// ) - /// .with_environment( - /// "dev", - /// OAuthEnvironment::new() - /// .with_client_id("dev-client-id") - /// .with_auth_url("https://api.dev-godaddy.com/v2/oauth2/authorize") - /// .with_token_url("https://api.dev-godaddy.com/v2/oauth2/token"), - /// ); + /// .with_environments(environments); /// # let _ = provider; /// ``` #[must_use] - pub fn with_environment(mut self, env: impl Into, config: OAuthEnvironment) -> Self { - self.environments.insert(env.into(), config); + pub fn with_environments( + mut self, + environments: Arc, + ) -> Self { + self.environments = Some(environments); self } @@ -456,44 +410,56 @@ impl PkceAuthProvider { } } - /// Resolves a value for `env` with precedence: provider-prefixed env var - /// (operator escape hatch) > per-environment override > base configuration. - fn effective_value( - &self, - env: &str, - var_suffix: &str, - select: impl Fn(&OAuthEnvironment) -> Option<&String>, - base: &str, - ) -> String { - let key = format!("{}_OAUTH_{var_suffix}", self.env_prefix); - if let Ok(value) = std::env::var(&key) { - return value; - } + /// Resolves the [`OAuthConfig`](crate::environments::OAuthConfig) for `env` + /// from the wired [`Environments`](crate::environments::Environments), if + /// any. Returns `None` when no resolver is wired, the env does not resolve, + /// or the resolved environment carries no OAuth slice. + fn resolved_oauth(&self, env: &str) -> Option { self.environments - .get(env) - .and_then(select) - .map_or_else(|| base.to_owned(), Clone::clone) + .as_ref() + .and_then(|envs| envs.resolve(env).ok()) + .and_then(|resolved| resolved.oauth) } fn effective_client_id(&self, env: &str) -> String { - self.effective_value(env, "CLIENT_ID", |e| e.client_id.as_ref(), &self.client_id) + if let Some(oauth) = self.resolved_oauth(env) + && !oauth.client_id.is_empty() + { + return oauth.client_id; + } + let key = format!("{}_OAUTH_CLIENT_ID", self.env_prefix); + std::env::var(&key).unwrap_or_else(|_| self.client_id.clone()) } fn effective_auth_url(&self, env: &str) -> String { - self.effective_value(env, "AUTH_URL", |e| e.auth_url.as_ref(), &self.auth_url) + if let Some(oauth) = self.resolved_oauth(env) + && !oauth.auth_url.is_empty() + { + return oauth.auth_url; + } + let key = format!("{}_OAUTH_AUTH_URL", self.env_prefix); + std::env::var(&key).unwrap_or_else(|_| self.auth_url.clone()) } fn effective_token_url(&self, env: &str) -> String { - self.effective_value(env, "TOKEN_URL", |e| e.token_url.as_ref(), &self.token_url) + if let Some(oauth) = self.resolved_oauth(env) + && !oauth.token_url.is_empty() + { + return oauth.token_url; + } + let key = format!("{}_OAUTH_TOKEN_URL", self.env_prefix); + std::env::var(&key).unwrap_or_else(|_| self.token_url.clone()) } - /// Default scopes for `env`: a per-environment scope override when set, - /// otherwise the provider's base scopes. + /// Default scopes for `env`: the resolved environment's scopes when + /// non-empty, otherwise the provider's base scopes. fn effective_scopes(&self, env: &str) -> Vec { - self.environments - .get(env) - .and_then(|e| e.scopes.clone()) - .unwrap_or_else(|| self.scopes.clone()) + if let Some(oauth) = self.resolved_oauth(env) + && !oauth.scopes.is_empty() + { + return oauth.scopes; + } + self.scopes.clone() } fn effective_redirect_uri(&self) -> String { @@ -1220,30 +1186,34 @@ mod tests { } } - fn perenv_provider() -> PkceAuthProvider { - PkceAuthProvider::new( - "perenv", - "https://base.example.com/auth", - "https://base.example.com/token", - "base-client", - &["openid"], - ) - .with_environment( - "prod", - OAuthEnvironment::new() - .with_client_id("prod-client") - .with_auth_url("https://prod.example.com/auth") - .with_token_url("https://prod.example.com/token") - .with_scopes(&["openid", "prod.read"]), + fn envs_for_test() -> Arc { + use crate::environments::{EnvironmentDef, Environments}; + Arc::new( + Environments::new("prod").with_environment( + "prod", + EnvironmentDef::new() + .with_client_id("prod-client") + .with_auth_url("https://prod.example.com/auth") + .with_token_url("https://prod.example.com/token") + .with_scopes(&["openid", "prod.read"]), + ), ) } - /// A per-environment override wins over the provider's base configuration - /// for the matching env, so one provider can serve dev/test/prod with - /// distinct OAuth clients and endpoints. + /// A provider wired to an [`Environments`](crate::environments::Environments) + /// resolver sources its per-env OAuth config (client id, endpoints, scopes) + /// from the resolved environment, making the environment the single source + /// of truth. #[test] - fn environment_override_selects_per_env_oauth_config() { - let provider = perenv_provider(); + fn environment_wired_provider_sources_oauth_from_resolver() { + let provider = PkceAuthProvider::new( + "godaddy", + "https://base/auth", + "https://base/token", + "base-client", + &["openid"], + ) + .with_environments(envs_for_test()); assert_eq!(provider.effective_client_id("prod"), "prod-client"); assert_eq!( provider.effective_auth_url("prod"), @@ -1259,21 +1229,22 @@ mod tests { ); } - /// An environment with no registered override falls back to the base - /// client id, endpoints, and scopes. + /// A provider with no environment resolver falls back to the base client id, + /// endpoints, and scopes for every env. #[test] - fn unconfigured_environment_falls_back_to_base_config() { - let provider = perenv_provider(); - assert_eq!(provider.effective_client_id("dev"), "base-client"); - assert_eq!( - provider.effective_auth_url("dev"), - "https://base.example.com/auth" + fn non_wired_provider_uses_base_config() { + let provider = PkceAuthProvider::new( + "godaddy", + "https://base/auth", + "https://base/token", + "base-client", + &["openid"], ); + assert_eq!(provider.effective_client_id("anything"), "base-client"); assert_eq!( - provider.effective_token_url("dev"), - "https://base.example.com/token" + provider.effective_scopes("anything"), + vec!["openid".to_owned()] ); - assert_eq!(provider.effective_scopes("dev"), vec!["openid".to_owned()]); } /// OAuth token traffic must carry the engine's configured default From 888bb68aadb2ddfe1b049fce4dd0300c2b66c225 Mon Sep 17 00:00:00 2001 From: Jacob Page Date: Wed, 17 Jun 2026 11:43:17 -0700 Subject: [PATCH 12/31] fix(environments): guard --env read so CLIs without environments don't panic MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit apply_env_flag eagerly called matches.get_one::("env"), but the --env arg is only registered when environments are configured. clap panics on get_one for an undefined arg, so every CLI that doesn't use environments panicked on every run (caught by the full argv0_dispatch + foundation suites, which per-task --lib verification had skipped). Guard on middleware.environments first — the same condition that registers the flag. Co-Authored-By: Claude Opus 4.8 --- src/cli.rs | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/src/cli.rs b/src/cli.rs index ae79020..3732e9f 100644 --- a/src/cli.rs +++ b/src/cli.rs @@ -1997,10 +1997,15 @@ impl Cli { /// name against the registered environments and updates `middleware.env`, /// returning an error for an unknown environment. fn apply_env_flag(&self, matches: &ArgMatches, middleware: &mut Middleware) -> Result<()> { - if let (Some(env), Some(environments)) = ( - matches.get_one::("env"), - middleware.environments.clone(), - ) { + // Guard on the environment system FIRST. The `--env` arg is only + // registered when environments are configured (the same condition that + // sets `middleware.environments`); calling `matches.get_one("env")` for + // an arg that was never registered panics in clap, which would break + // every CLI that does not use environments. + let Some(environments) = middleware.environments.clone() else { + return Ok(()); + }; + if let Some(env) = matches.get_one::("env") { environments.resolve(env)?; middleware.env = env.clone(); } From fcb328a1bd07fa12b241b5e642d54683e3a851fc Mon Sep 17 00:00:00 2001 From: Jacob Page Date: Wed, 17 Jun 2026 11:49:45 -0700 Subject: [PATCH 13/31] =?UTF-8?q?refactor(environments):=20address=20revie?= =?UTF-8?q?w=20minors=20=E2=80=94=20as=5Fref,=20--env=20help,=20env=20set?= =?UTF-8?q?=20tier,=20resolve=20trace?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - src/cli.rs apply_env_flag: use .as_ref() instead of .clone() on middleware.environments to avoid an unnecessary Arc refcount bump; Environments::resolve takes &self so the borrow suffices. - src/cli.rs --env flag: improve help text from "Target environment" to "Override the active environment (see: env list)". - src/env_commands.rs set: add .with_tier(Tier::Mutate).mutates(true) so --dry-run short-circuits the config-file write before it persists. - src/auth/pkce.rs resolved_oauth: replace .ok() with a match arm that logs the discarded error via tracing::debug! (env name + error, no token/secret); fallback behavior is unchanged. Co-Authored-By: Claude Opus 4.8 --- docs/concepts.md | 4 +--- src/auth/pkce.rs | 16 ++++++++++++---- src/cli.rs | 4 ++-- src/env_commands.rs | 4 +++- 4 files changed, 18 insertions(+), 10 deletions(-) diff --git a/docs/concepts.md b/docs/concepts.md index 02c8d07..6270c7b 100644 --- a/docs/concepts.md +++ b/docs/concepts.md @@ -406,9 +406,7 @@ path. ## Environments -Environment selection is application-defined. If an application needs an environment flag, it can -register one through `CliConfig::register_flags` and store the parsed value on middleware through -`CliConfig::apply_flags`. Auth providers receive the environment value during credential operations. +`cli_engine` provides a first-class environment system with layered resolution, a config-file layer, env-var overrides, sticky active-env persistence, and per-environment OAuth for `PkceAuthProvider`; see [Environments](environments.md) for the full reference. ## Risk Tiers diff --git a/src/auth/pkce.rs b/src/auth/pkce.rs index 4853511..0a9fc5c 100644 --- a/src/auth/pkce.rs +++ b/src/auth/pkce.rs @@ -415,10 +415,18 @@ impl PkceAuthProvider { /// any. Returns `None` when no resolver is wired, the env does not resolve, /// or the resolved environment carries no OAuth slice. fn resolved_oauth(&self, env: &str) -> Option { - self.environments - .as_ref() - .and_then(|envs| envs.resolve(env).ok()) - .and_then(|resolved| resolved.oauth) + let envs = self.environments.as_ref()?; + match envs.resolve(env) { + Ok(resolved) => resolved.oauth, + Err(e) => { + tracing::debug!( + env, + error = %e, + "environment resolve failed; falling back to base OAuth config" + ); + None + } + } } fn effective_client_id(&self, env: &str) -> String { diff --git a/src/cli.rs b/src/cli.rs index 3732e9f..c221c96 100644 --- a/src/cli.rs +++ b/src/cli.rs @@ -753,7 +753,7 @@ impl Cli { .long("env") .global(true) .value_name("ENV") - .help("Target environment"), + .help("Override the active environment (see: env list)"), ); } let intro = config @@ -2002,7 +2002,7 @@ impl Cli { // sets `middleware.environments`); calling `matches.get_one("env")` for // an arg that was never registered panics in clap, which would break // every CLI that does not use environments. - let Some(environments) = middleware.environments.clone() else { + let Some(environments) = middleware.environments.as_ref() else { return Ok(()); }; if let Some(env) = matches.get_one::("env") { diff --git a/src/env_commands.rs b/src/env_commands.rs index b8a4006..8e555f6 100644 --- a/src/env_commands.rs +++ b/src/env_commands.rs @@ -11,7 +11,7 @@ use serde_json::json; use crate::{ - CommandResult, CommandSpec, GroupSpec, RuntimeCommandSpec, RuntimeGroupSpec, + CommandResult, CommandSpec, GroupSpec, RuntimeCommandSpec, RuntimeGroupSpec, Tier, error::CliCoreError, }; @@ -65,6 +65,8 @@ pub fn env_command_group() -> RuntimeGroupSpec { .with_command(RuntimeCommandSpec::new_with_context( CommandSpec::new("set", "Set and persist the active environment") .no_auth(true) + .with_tier(Tier::Mutate) + .mutates(true) .with_arg(clap::Arg::new("name").required(true)), async |ctx| { let envs = ctx From 354041766cb7243f2994cedb7b3e3500aea03bfd Mon Sep 17 00:00:00 2001 From: Jacob Page Date: Wed, 17 Jun 2026 11:49:53 -0700 Subject: [PATCH 14/31] docs(environments): document first-class environments Adds docs/environments.md covering the three resolution layers and their precedence, the environments.toml schema (OAuth keys + extra bag), the env-var override convention and the -/_ prefix-collision caveat, sticky active env + --env override + the built-in env list/get/set/info commands, and the PkceAuthProvider::with_environments single-source pattern with a full runnable example (no_run). Updates docs/concepts.md Environments section to a one-line pointer to the new reference doc. Co-Authored-By: Claude Opus 4.8 --- docs/environments.md | 161 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 161 insertions(+) create mode 100644 docs/environments.md diff --git a/docs/environments.md b/docs/environments.md new file mode 100644 index 0000000..29288ac --- /dev/null +++ b/docs/environments.md @@ -0,0 +1,161 @@ +# Environments + +`cli_engine` provides a first-class environment system that lets CLIs target named deployment environments — `prod`, `ote`, `dev`, and any others the application defines — without consumers having to wire flags, config lookups, or OAuth overrides by hand. + +When `CliConfig::with_environments` is called, the engine: + +- Registers a global `--env` flag on every command. +- Seeds the active environment into middleware at startup. +- Exposes the resolved environment to handlers via `CommandContext::environment`. +- Mounts the built-in `env list / get / set / info` commands under the admin help category. + +## Resolution Layers + +`Environments::resolve(name)` builds a fully-merged `Environment` by applying three layers in order. +Later layers win over earlier ones. + +1. **Compiled-in defaults** — `EnvironmentDef` values registered with `Environments::with_environment` in the application source code. +2. **`environments.toml`** — the file at `//environments.toml`, when enabled with `Environments::with_config_file(true)`. +3. **Environment-variable overrides** — `_OAUTH_CLIENT_ID`, `_OAUTH_AUTH_URL`, `_OAUTH_TOKEN_URL`, and `_` for each bag key already present in the merged record. + +A name that is unknown to all three layers — not in compiled defaults, not in the file, and not resolvable — returns an error listing the known names. + +## environments.toml Schema + +The file uses one top-level TOML table per environment name: + +```toml +[prod] +client_id = "prod-client-id" +auth_url = "https://api.example.com/v2/oauth2/authorize" +token_url = "https://api.example.com/v2/oauth2/token" +scopes = ["openid", "profile"] +api_url = "https://api.example.com" + +[ote] +client_id = "ote-client-id" +auth_url = "https://api.ote.example.com/v2/oauth2/authorize" +token_url = "https://api.ote.example.com/v2/oauth2/token" +api_url = "https://api.ote.example.com" +``` + +The recognized OAuth keys — `client_id`, `auth_url`, `token_url`, and `scopes` (an array of strings) — are parsed into the typed `OAuthConfig` slice of the resolved `Environment`. +Every other string key is captured as a free-form field in `Environment::extra` (for example `api_url` above). + +## Environment-Variable Overrides + +The prefix is the environment name uppercased with `-` replaced by `_` (`ote` → `OTE`, `prod-us` → `PROD_US`). +Names that differ only by `-` vs `_` map to the same prefix and will collide; avoid such names. + +The three OAuth fields are always overridable: + +| Variable | Field overridden | +| --- | --- | +| `_OAUTH_CLIENT_ID` | `oauth.client_id` | +| `_OAUTH_AUTH_URL` | `oauth.auth_url` | +| `_OAUTH_TOKEN_URL` | `oauth.token_url` | + +Scopes are **not** env-var overridable; set them in the compiled-in layer or `environments.toml`. + +Bag keys in `Environment::extra` are overridable via `_` only when the key is already present in the merged record after layers 1 and 2. +For example, `api_url` must exist in either the compiled defaults or the file before `PROD_API_URL` has any effect. + +## Active Environment + +The active environment controls which environment is targeted when no `--env` flag is passed. + +**Precedence** (highest first): + +1. `--env ` on the command line. +2. The `environment.active` key in the per-application config file (persisted by `env set`). +3. The default set in `Environments::new(default_env)`. + +`env set ` validates the name against all three layers and then writes `environment.active` to the config file. +The next invocation (without `--env`) picks it up from layer 2. + +The built-in commands are: + +| Command | Description | +| --- | --- | +| `env list` | Lists all known environments (compiled + file), marking the active one. | +| `env get` | Prints the active environment name. | +| `env set ` | Validates and persists `name` as the active environment. | +| `env info` | Prints the fully resolved active environment including OAuth and extra fields. | + +`env set` is marked `Tier::Mutate` so `--dry-run` short-circuits the config-file write. + +## Per-Environment OAuth via PkceAuthProvider + +`PkceAuthProvider::with_environments(Arc)` wires the provider to the same `Environments` instance, making it the single source of truth for per-environment OAuth config. + +When the provider resolves a credential for `env`, it calls `Environments::resolve(env)` and uses the resulting `OAuthConfig`. +Each field falls through to the next source when empty: + +1. The resolved environment's field (when non-empty). +2. The legacy provider-prefixed env var (`_OAUTH_CLIENT_ID`, `_AUTH_URL`, `_TOKEN_URL`), where `` is the provider name uppercased with `-` → `_`. +3. The base configuration supplied to `PkceAuthProvider::new`. + +Scopes follow the same pattern: the resolved environment's scopes when non-empty, otherwise the provider's base scopes. + +If `Environments::resolve` fails — because the name is unknown or the environments file cannot be parsed — the provider logs the error at `TRACE`/`DEBUG` level and falls back to the next source. +No token or secret is included in the log; only the environment name and error message appear. + +## Example + +```rust,no_run +use std::sync::Arc; +use cli_engine::{ + BuildInfo, Cli, CliConfig, + auth::pkce::PkceAuthProvider, + environments::{EnvironmentDef, Environments}, +}; + +let environments = Arc::new( + Environments::new("prod") + .with_environment( + "prod", + EnvironmentDef::new() + .with_client_id("prod-client-id") + .with_auth_url("https://api.example.com/v2/oauth2/authorize") + .with_token_url("https://api.example.com/v2/oauth2/token") + .with_scopes(&["openid", "profile"]) + .with_field("api_url", "https://api.example.com"), + ) + .with_environment( + "ote", + EnvironmentDef::new() + .with_client_id("ote-client-id") + .with_auth_url("https://api.ote.example.com/v2/oauth2/authorize") + .with_token_url("https://api.ote.example.com/v2/oauth2/token") + .with_field("api_url", "https://api.ote.example.com"), + ) + .with_config_file(true), +); + +let provider = Arc::new( + PkceAuthProvider::new( + "primary", + "https://api.example.com/v2/oauth2/authorize", + "https://api.example.com/v2/oauth2/token", + "fallback-client-id", + &["openid"], + ) + .with_environments(Arc::clone(&environments)), +); + +let cli = Cli::new( + CliConfig::new("my-cli", "My CLI", "my-cli") + .with_build(BuildInfo::new(env!("CARGO_PKG_VERSION"))) + .with_default_auth_provider("primary") + .with_auth_provider(provider) + .with_environments((*environments).clone()), +); +``` + +With this setup: + +- Running `my-cli env list` prints `ote` and `prod`, marking whichever is active. +- Running `my-cli env set ote` persists `ote` as active; subsequent invocations target OTE. +- Running `my-cli --env prod ` overrides the active environment for that invocation only. +- `PROD_OAUTH_CLIENT_ID=override my-cli --env prod auth login` injects the override at the env-var layer. +- A user-supplied `environments.toml` in the config directory can add new environments or override fields without recompiling. From 56da50b004694de9f5526136334d65969e4cdebf Mon Sep 17 00:00:00 2001 From: Jacob Page Date: Wed, 17 Jun 2026 12:09:09 -0700 Subject: [PATCH 15/31] fix(environments)!: share one app_id-stamped Environments Arc between CliConfig and provider CliConfig::with_environments auto-stamped app_id onto a hidden copy of the Environments, but a PkceAuthProvider is wired with a separate Arc that never received app_id. With an empty app_id, config_file_path returns None, so the provider's environments.toml file layer always resolved empty: a file-defined environment (or a file override of a compiled env's client_id) was visible to env info but invisible to the actual OAuth login. Make the shared instance explicit: with_environments now takes Arc and stores it as-is (no hidden stamp), and Cli::new reuses that same Arc for middleware. The consumer sets app_id on the Environments and shares one Arc between the provider and CliConfig. Co-Authored-By: Claude Opus 4.8 --- docs/environments.md | 10 ++++- src/auth/pkce.rs | 89 ++++++++++++++++++++++++++++++++++++++++++++ src/cli.rs | 62 +++++++++++++++++++++--------- src/environments.rs | 11 +++++- tests/foundation.rs | 4 +- 5 files changed, 153 insertions(+), 23 deletions(-) diff --git a/docs/environments.md b/docs/environments.md index 29288ac..820aa3d 100644 --- a/docs/environments.md +++ b/docs/environments.md @@ -41,6 +41,7 @@ api_url = "https://api.ote.example.com" The recognized OAuth keys — `client_id`, `auth_url`, `token_url`, and `scopes` (an array of strings) — are parsed into the typed `OAuthConfig` slice of the resolved `Environment`. Every other string key is captured as a free-form field in `Environment::extra` (for example `api_url` above). +The `extra` bag is printed verbatim by `env info`, so it must not hold secrets. ## Environment-Variable Overrides @@ -87,6 +88,8 @@ The built-in commands are: ## Per-Environment OAuth via PkceAuthProvider `PkceAuthProvider::with_environments(Arc)` wires the provider to the same `Environments` instance, making it the single source of truth for per-environment OAuth config. +Build one `Arc` — with `Environments::with_app_id()` set on it — and share that same `Arc` between the provider and `CliConfig::with_environments`. +If the two receive different instances, a file-defined environment (or a file override of a compiled environment's `client_id`) is visible to `env info` yet invisible to the actual OAuth login. When the provider resolves a credential for `env`, it calls `Environments::resolve(env)` and uses the resulting `OAuthConfig`. Each field falls through to the next source when empty: @@ -110,8 +113,11 @@ use cli_engine::{ environments::{EnvironmentDef, Environments}, }; +// Build one Arc and share it. `with_app_id` must match the +// CliConfig app_id ("my-cli") so the environments.toml file path resolves. let environments = Arc::new( Environments::new("prod") + .with_app_id("my-cli") .with_environment( "prod", EnvironmentDef::new() @@ -148,7 +154,9 @@ let cli = Cli::new( .with_build(BuildInfo::new(env!("CARGO_PKG_VERSION"))) .with_default_auth_provider("primary") .with_auth_provider(provider) - .with_environments((*environments).clone()), + // The same Arc the provider was wired with — not a separate copy — so the + // file layer and active-env persistence resolve identically for both. + .with_environments(environments), ); ``` diff --git a/src/auth/pkce.rs b/src/auth/pkce.rs index 0a9fc5c..399aaa9 100644 --- a/src/auth/pkce.rs +++ b/src/auth/pkce.rs @@ -1149,6 +1149,7 @@ async fn parse_token_response( } #[cfg(test)] +#[allow(unsafe_code)] mod tests { use serde_json::json; @@ -1814,4 +1815,92 @@ mod tests { let cred = provider.status("dev").await.expect("status"); assert_eq!(cred.token, "filetok"); } + + /// Serializes env-var access and removes the vars on drop (matching the + /// convention in `src/environments.rs` tests). Vars persist process-wide, so + /// concurrent tests would otherwise see each other's writes. + static ENV_LOCK: std::sync::Mutex<()> = std::sync::Mutex::new(()); + + /// RAII guard removing an env var on drop, even on panic, while ENV_LOCK held. + struct EnvGuard(&'static str); + impl Drop for EnvGuard { + fn drop(&mut self) { + // SAFETY: the test holds ENV_LOCK for the guard's lifetime. + unsafe { std::env::remove_var(self.0) } + } + } + + /// The fix: a provider wired to a shared `Arc` whose + /// `environments.toml` file layer defines `prod` with a different `client_id` + /// resolves the FILE's client id. This proves the provider's file layer + /// resolves — the shared, app_id-stamped instance reaches the provider rather + /// than an unstamped copy whose file path is `None`. + #[test] + fn wired_provider_resolves_client_id_from_environments_file() { + use crate::environments::{EnvironmentDef, Environments}; + + let dir = tempfile::tempdir().expect("tempdir"); + let file = dir.path().join("environments.toml"); + std::fs::write( + &file, + r#" +[prod] +client_id = "file-prod-client" +"#, + ) + .expect("write environments.toml"); + + let environments = Arc::new( + Environments::new("prod") + .with_app_id("x") + .with_environment( + "prod", + EnvironmentDef::new().with_client_id("compiled-prod-client"), + ) + .with_config_file_path_override(file), + ); + + let provider = PkceAuthProvider::new( + "godaddy", + "https://base/auth", + "https://base/token", + "base-client", + &["openid"], + ) + .with_environments(environments); + + // The file overrides the compiled client id, which itself overrides the + // provider's base — proving the wired provider reads the file layer. + assert_eq!(provider.effective_client_id("prod"), "file-prod-client"); + } + + /// Legacy escape hatch: a NON-wired provider still honors the provider-prefixed + /// `_OAUTH_CLIENT_ID` env var, and a wired provider whose resolved env + /// yields a non-empty client id takes precedence over that legacy var. + #[test] + fn legacy_oauth_client_id_env_var_overrides_base_but_yields_to_wired_env() { + use crate::environments::{EnvironmentDef, Environments}; + + let _lock = ENV_LOCK + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner); + // `` = provider name uppercased, '-' -> '_'. Provider is "test". + // SAFETY: serialized by ENV_LOCK; guard removes the var on any exit. + unsafe { std::env::set_var("TEST_OAUTH_CLIENT_ID", "legacy-client") }; + let _guard = EnvGuard("TEST_OAUTH_CLIENT_ID"); + + // Non-wired provider: the legacy var overrides the base client id. + let bare = test_provider(); + assert_eq!(bare.effective_client_id("prod"), "legacy-client"); + + // Wired provider whose resolved env carries a non-empty client id: the + // resolved environment wins over the legacy var. + let environments = Arc::new( + Environments::new("prod") + .with_app_id("x") + .with_environment("prod", EnvironmentDef::new().with_client_id("env-client")), + ); + let wired = test_provider().with_environments(environments); + assert_eq!(wired.effective_client_id("prod"), "env-client"); + } } diff --git a/src/cli.rs b/src/cli.rs index c221c96..76c8efc 100644 --- a/src/cli.rs +++ b/src/cli.rs @@ -260,7 +260,7 @@ pub struct CliConfig { /// registers a global `--env` flag, seeds the active environment into /// middleware, and exposes it to handlers through /// [`CommandContext::environment`](crate::command::CommandContext::environment). - pub environments: Option, + pub environments: Option>, } impl CliConfig { @@ -307,12 +307,33 @@ impl CliConfig { /// configured default), and exposes the resolved environment to handlers via /// [`CommandContext::environment`](crate::command::CommandContext::environment). /// - /// The provided [`Environments`](crate::environments::Environments) has this - /// config's `app_id` applied so its config file and active-env persistence - /// resolve to the application's config directory. + /// The [`Environments`](crate::environments::Environments) is stored as-is, so + /// the consumer is responsible for configuring it before wrapping it in an + /// `Arc`: + /// + /// - Call + /// [`Environments::with_app_id`](crate::environments::Environments::with_app_id) + /// with the **same** `app_id` passed to [`CliConfig::new`], so the config + /// file and active-environment persistence resolve to the application's + /// config directory. (An empty `app_id` makes + /// [`Environments::config_file_path`](crate::environments::Environments::config_file_path) + /// return `None`, silently disabling the `environments.toml` file layer.) + /// - Call + /// [`Environments::with_config_file(true)`](crate::environments::Environments::with_config_file) + /// if the application loads a user-editable `environments.toml`. + /// - **Share the same `Arc`** with any + /// [`PkceAuthProvider::with_environments`](crate::auth::pkce::PkceAuthProvider::with_environments): + /// the provider's OAuth file layer and active-environment persistence must + /// resolve against the identical, `app_id`-stamped instance the engine sees, + /// or a file-defined environment (or a file override of a compiled + /// environment's `client_id`) will be visible to `env info` yet invisible to + /// the actual OAuth login. #[must_use] - pub fn with_environments(mut self, environments: crate::environments::Environments) -> Self { - self.environments = Some(environments.with_app_id(self.app_id.clone())); + pub fn with_environments( + mut self, + environments: Arc, + ) -> Self { + self.environments = Some(environments); self } @@ -781,11 +802,12 @@ impl Cli { .human_views .merge(&global_human_view_registry_snapshot()); if let Some(environments) = &config.environments { - let environments = Arc::new(environments.clone()); // Seed the sticky/default active environment now; the global `--env` - // flag overrides it per invocation in `run_with_depth`. + // flag overrides it per invocation in `run_with_depth`. The same + // `Arc` the consumer shared with any `PkceAuthProvider` is reused, so + // the file layer and active-env persistence resolve consistently. middleware.env = environments.effective_active(None, &middleware.config); - middleware.environments = Some(environments); + middleware.environments = Some(Arc::clone(environments)); } let mut cli = Self { @@ -2987,12 +3009,16 @@ mod env_config_tests { use super::*; #[test] - fn with_environments_stores_and_app_id_is_injected() { - let cfg = CliConfig::new("gddy", "GoDaddy CLI", "gddy").with_environments( - crate::environments::Environments::new("prod").with_config_file(true), - ); + fn with_environments_stores_shared_arc_with_consumer_app_id() { + // The consumer sets app_id on the Environments before sharing the Arc; + // CliConfig stores it as-is, so the file path resolves only because the + // consumer stamped the matching app_id (not because the engine did). + let cfg = CliConfig::new("gddy", "GoDaddy CLI", "gddy").with_environments(Arc::new( + crate::environments::Environments::new("prod") + .with_app_id("gddy") + .with_config_file(true), + )); let envs = cfg.environments.as_ref().expect("environments set"); - // app_id is stamped from CliConfig so the file path resolves. assert!(envs.config_file_path().is_some()); } @@ -3001,11 +3027,11 @@ mod env_config_tests { use crate::{CommandResult, CommandSpec, RuntimeCommandSpec}; use serde_json::json; let mut cli = Cli::new( - CliConfig::new("envtest", "Env test", "envtest").with_environments( + CliConfig::new("envtest", "Env test", "envtest").with_environments(Arc::new( crate::environments::Environments::new("prod") .with_environment("prod", crate::environments::EnvironmentDef::new()) .with_environment("ote", crate::environments::EnvironmentDef::new()), - ), + )), ); cli.add_command(RuntimeCommandSpec::new_with_context( CommandSpec::new("whichenv", "echo env").no_auth(true), @@ -3026,10 +3052,10 @@ mod env_config_tests { #[tokio::test] async fn unknown_env_flag_produces_error_envelope() { let cli = Cli::new( - CliConfig::new("envtest2", "Env test", "envtest2").with_environments( + CliConfig::new("envtest2", "Env test", "envtest2").with_environments(Arc::new( crate::environments::Environments::new("prod") .with_environment("prod", crate::environments::EnvironmentDef::new()), - ), + )), ); let out = cli.run(["envtest2", "tree", "--env", "nope"]).await; assert_ne!(out.exit_code, 0); diff --git a/src/environments.rs b/src/environments.rs index 200353d..888c7a4 100644 --- a/src/environments.rs +++ b/src/environments.rs @@ -139,8 +139,15 @@ impl Environments { self } - /// Sets the application id used to locate the config file. Set automatically - /// by `CliConfig::with_environments`; only call directly in tests. + /// Sets the application id used to locate the config file. + /// + /// The consumer must set this to the same `app_id` passed to + /// [`CliConfig::new`](crate::CliConfig::new) before sharing the + /// [`Environments`] with both + /// [`CliConfig::with_environments`](crate::CliConfig::with_environments) and + /// [`PkceAuthProvider::with_environments`](crate::auth::pkce::PkceAuthProvider::with_environments), + /// or [`config_file_path`](Self::config_file_path) returns `None` and the + /// `environments.toml` file layer silently resolves empty. #[must_use] pub fn with_app_id(mut self, app_id: impl Into) -> Self { self.app_id = app_id.into(); diff --git a/tests/foundation.rs b/tests/foundation.rs index dcabf6e..52bf124 100644 --- a/tests/foundation.rs +++ b/tests/foundation.rs @@ -9905,7 +9905,7 @@ async fn env_group_lists_gets_and_shows_info_for_active_environment() { use cli_engine::environments::{EnvironmentDef, Environments}; let cli = Cli::new( - CliConfig::new("envcmds", "Env cmds", "envcmds").with_environments( + CliConfig::new("envcmds", "Env cmds", "envcmds").with_environments(Arc::new( Environments::new("prod") .with_environment( "prod", @@ -9915,7 +9915,7 @@ async fn env_group_lists_gets_and_shows_info_for_active_environment() { "ote", EnvironmentDef::new().with_field("api_url", "https://o"), ), - ), + )), ); // env list returns both environments. From 13b63a52445383c3248c47889fae109c1829fde1 Mon Sep 17 00:00:00 2001 From: Jacob Page Date: Wed, 17 Jun 2026 12:30:45 -0700 Subject: [PATCH 16/31] fix(docs): avoid intra-doc links to feature-gated pkce from non-gated code The CI Rust/Docs step runs `cargo doc` without features as well as with `pkce-auth`. Two intra-doc links to `PkceAuthProvider::with_environments` (in CliConfig and Environments docs) failed the no-feature build because the `pkce` module is feature-gated (`no item named pkce in module auth`), and `-D warnings` promotes broken-intra-doc-links to an error. Use plain code spans referencing the `pkce-auth` feature instead. Co-Authored-By: Claude Opus 4.8 --- src/cli.rs | 4 ++-- src/environments.rs | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/cli.rs b/src/cli.rs index 76c8efc..2f8254f 100644 --- a/src/cli.rs +++ b/src/cli.rs @@ -321,8 +321,8 @@ impl CliConfig { /// - Call /// [`Environments::with_config_file(true)`](crate::environments::Environments::with_config_file) /// if the application loads a user-editable `environments.toml`. - /// - **Share the same `Arc`** with any - /// [`PkceAuthProvider::with_environments`](crate::auth::pkce::PkceAuthProvider::with_environments): + /// - **Share the same `Arc`** with any `PkceAuthProvider::with_environments` + /// (available with the `pkce-auth` feature): /// the provider's OAuth file layer and active-environment persistence must /// resolve against the identical, `app_id`-stamped instance the engine sees, /// or a file-defined environment (or a file override of a compiled diff --git a/src/environments.rs b/src/environments.rs index 888c7a4..e3d0851 100644 --- a/src/environments.rs +++ b/src/environments.rs @@ -145,7 +145,7 @@ impl Environments { /// [`CliConfig::new`](crate::CliConfig::new) before sharing the /// [`Environments`] with both /// [`CliConfig::with_environments`](crate::CliConfig::with_environments) and - /// [`PkceAuthProvider::with_environments`](crate::auth::pkce::PkceAuthProvider::with_environments), + /// `PkceAuthProvider::with_environments` (with the `pkce-auth` feature), /// or [`config_file_path`](Self::config_file_path) returns `None` and the /// `environments.toml` file layer silently resolves empty. #[must_use] From 784172d63675186994299b24490ecd651d79d472 Mon Sep 17 00:00:00 2001 From: Jacob Page Date: Wed, 17 Jun 2026 12:41:42 -0700 Subject: [PATCH 17/31] docs(environments): correct env-var layer semantics (override, not define) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Copilot review: the design spec and environments.md implied env-var-only environments are resolvable/selectable, but resolve() treats an env as known only if defined by a compiled default or environments.toml — env vars only override fields of a defined env. Corrected the wording in both docs. Co-Authored-By: Claude Opus 4.8 --- docs/environments.md | 3 ++- .../2026-06-17-engine-environments-design.md | 16 ++++++++++------ 2 files changed, 12 insertions(+), 7 deletions(-) diff --git a/docs/environments.md b/docs/environments.md index 820aa3d..cc3776d 100644 --- a/docs/environments.md +++ b/docs/environments.md @@ -71,7 +71,8 @@ The active environment controls which environment is targeted when no `--env` fl 2. The `environment.active` key in the per-application config file (persisted by `env set`). 3. The default set in `Environments::new(default_env)`. -`env set ` validates the name against all three layers and then writes `environment.active` to the config file. +`env set ` validates that the environment is defined — by a compiled default or `environments.toml` — and then writes `environment.active` to the config file. +Environment variables override fields of a defined environment but cannot define a new, selectable environment on their own, so a name known only through `_*` variables is rejected. The next invocation (without `--env`) picks it up from layer 2. The built-in commands are: diff --git a/docs/specs/2026-06-17-engine-environments-design.md b/docs/specs/2026-06-17-engine-environments-design.md index e60ada0..3be4496 100644 --- a/docs/specs/2026-06-17-engine-environments-design.md +++ b/docs/specs/2026-06-17-engine-environments-design.md @@ -109,8 +109,9 @@ parse the TOML file); `Environment` is the *resolved* result after merging layer Behavior: - Unknown env name → typed error listing enumerable env names. -- `Environments::list()` returns compiled + file-defined envs. Env-var-only envs are - resolvable but not enumerable (matches `gddy`). +- `Environments::list()` returns compiled + file-defined envs. Environment-variable + layers only *override fields* of an environment already defined by a compiled + default or the file; env vars alone do not define a new, selectable environment. - Merge is field-level: a file or env-var layer may override individual OAuth fields or individual bag keys without restating the whole record. @@ -123,8 +124,8 @@ Behavior: - Auto-mounted **`env` command group** (mounted like the built-in `auth` group): - `env list` — enumerable environments, marking the active one. - `env get` — prints the active environment name. - - `env set ` — validates via `resolve()` (so env-var-only envs are accepted), - then persists to `ConfigFile`. + - `env set ` — validates via `resolve()` (the name must be defined by a + compiled default or `environments.toml`), then persists to `ConfigFile`. - `env info` — shows the resolved active environment (OAuth endpoints + bag), with secrets omitted. - The resolved active env name is written to the existing `Middleware.env`, so @@ -179,7 +180,8 @@ Behavior: ## Testing - **Unit:** resolution precedence across all three layers; field-level merge; - unknown-env error contents; `list()` enumeration vs env-var-only resolvability; + unknown-env error contents; `list()` enumeration; env-var layers override fields of + a defined env (not define new ones); env-var keying (`_OAUTH_*`, `_`); active-env persistence round-trip through `ConfigFile`; lazy resolution (no file/env access on `--schema`/`--dry-run`). - **Integration (`Cli::run`):** `env set`/`get`/`list`/`info`; `--env` overrides the @@ -192,7 +194,9 @@ Behavior: ## Clarifications (decided; called out for implementation) -- `env set` validates via `resolve()` so env-var-only environments can be selected. +- `env set` validates via `resolve()`; the name must be defined by a compiled default + or `environments.toml`. Environment variables override fields of a defined env but + cannot define a new, selectable environment on their own. - Exact reconciliation of the `_OAUTH_*` (environment layer) vs `_OAUTH_*` (legacy provider layer) precedence is: environment-wired providers use the environment layer exclusively; non-wired providers retain the legacy layer. From 4c7cd871ff4d645d8f8e2e3adc976256c241ddef Mon Sep 17 00:00:00 2001 From: Jacob Page Date: Wed, 17 Jun 2026 12:51:33 -0700 Subject: [PATCH 18/31] refactor: panic-safe UA reset in tests + document resolve() blocking I/O Copilot review round 2: - tests/foundation.rs: the two execute_from tests reset the process-wide User-Agent at the end; a panicking assert would skip it and leak the derived UA into later tests. Replace the manual reset with a RestoreDefaultUserAgent RAII guard (drops while the UA lock is still held). - src/environments.rs: document that Environments::resolve performs blocking filesystem I/O when the config-file layer is enabled, so it isn't called per-request from async handlers. Co-Authored-By: Claude Opus 4.8 --- src/environments.rs | 8 ++++++++ tests/foundation.rs | 21 +++++++++++++++++---- 2 files changed, 25 insertions(+), 4 deletions(-) diff --git a/src/environments.rs b/src/environments.rs index e3d0851..508f61f 100644 --- a/src/environments.rs +++ b/src/environments.rs @@ -189,6 +189,14 @@ impl Environments { /// strings. Consumers should treat empty endpoint strings as "fall back to /// the provider's default base endpoints". /// + /// # Blocking + /// + /// When the config-file layer is enabled (via + /// [`with_config_file`](Self::with_config_file)), this performs synchronous + /// filesystem I/O to read and parse `environments.toml`. Resolve the + /// environment once at startup (or off the latency-sensitive path) rather + /// than calling it per request inside an async handler. + /// /// # Errors /// /// Returns an error when `name` is not known to any layer or when the diff --git a/tests/foundation.rs b/tests/foundation.rs index 52bf124..b7f088e 100644 --- a/tests/foundation.rs +++ b/tests/foundation.rs @@ -150,6 +150,18 @@ impl RecordingTransportLogger { static USER_AGENT_TEST_LOCK: Mutex<()> = Mutex::const_new(()); +/// Restores the process-wide default User-Agent to the builtin on drop, so a +/// panicking assertion in a test that publishes a config-derived UA cannot leak +/// it into later tests. Hold alongside (declared after) the `USER_AGENT_TEST_LOCK` +/// guard so the reset runs while the lock is still held. +struct RestoreDefaultUserAgent; + +impl Drop for RestoreDefaultUserAgent { + fn drop(&mut self) { + transport::set_default_user_agent("cli/dev"); + } +} + #[test] fn tier_string_forms_and_mutating_parity() { assert_eq!(Tier::Read.to_string(), "read"); @@ -443,8 +455,10 @@ async fn cli_runtime_root_help_includes_find_commands_without_modules() { #[tokio::test] async fn cli_execute_from_writes_success_to_stdout_and_errors_to_stderr() { // execute_from publishes the config-derived User-Agent process-wide, so this - // test shares the lock with the user-agent tests and restores the default. + // test shares the lock with the user-agent tests and restores the default + // on exit (including panic) via the RAII guard, while the lock is held. let _ua_guard = USER_AGENT_TEST_LOCK.lock().await; + let _restore_ua = RestoreDefaultUserAgent; let mut cli = Cli::new(CliConfig { name: "my-cli".to_owned(), short: "Developer tooling".to_owned(), @@ -486,14 +500,14 @@ async fn cli_execute_from_writes_success_to_stdout_and_errors_to_stderr() { assert!(stdout.is_empty()); let rendered = String::from_utf8(stderr).expect("utf8"); assert!(rendered.contains("missing")); - transport::set_default_user_agent("cli/dev"); } #[tokio::test] async fn cli_execute_from_shutdown_signal_writes_interrupt_to_stderr() { // execute_from_until_signal publishes the config-derived User-Agent - // process-wide; share the lock and restore the default like the tests above. + // process-wide; share the lock and restore the default (panic-safe) like above. let _ua_guard = USER_AGENT_TEST_LOCK.lock().await; + let _restore_ua = RestoreDefaultUserAgent; let shutdown_count = Arc::new(AtomicUsize::new(0)); let shutdown_for_closure = Arc::clone(&shutdown_count); let mut cli = Cli::new(CliConfig { @@ -527,7 +541,6 @@ async fn cli_execute_from_shutdown_signal_writes_interrupt_to_stderr() { "command interrupted\n" ); assert_eq!(shutdown_count.load(Ordering::SeqCst), 1); - transport::set_default_user_agent("cli/dev"); } #[tokio::test] From c5906f645f633dab781cbe8cc9a2961bdfd7c033 Mon Sep 17 00:00:00 2001 From: Jacob Page Date: Wed, 17 Jun 2026 12:59:49 -0700 Subject: [PATCH 19/31] docs(environments): accurate list() error wording + blocking note on environment() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Copilot review round 3: - Environments::list() swallows ANY file read/parse error, not just parse errors — corrected the doc comment. - CommandContext::environment() can block on filesystem I/O via resolve(); added a # Blocking note advising call-once-per-invocation. Co-Authored-By: Claude Opus 4.8 --- src/command.rs | 8 ++++++++ src/environments.rs | 7 +++++-- 2 files changed, 13 insertions(+), 2 deletions(-) diff --git a/src/command.rs b/src/command.rs index 4322a1d..d9e3047 100644 --- a/src/command.rs +++ b/src/command.rs @@ -136,6 +136,14 @@ impl CommandContext { /// the compiled-in definition, the `environments.toml` file layer, and /// `_*` environment-variable overrides. /// + /// # Blocking + /// + /// When the `environments.toml` file layer is enabled, this performs + /// synchronous filesystem I/O via + /// [`Environments::resolve`](crate::environments::Environments::resolve). + /// Call it once per invocation and reuse the result rather than calling it + /// repeatedly inside an async handler on a latency-sensitive path. + /// /// # Errors /// /// Returns an error if no environment system was registered via diff --git a/src/environments.rs b/src/environments.rs index 508f61f..9f61d3c 100644 --- a/src/environments.rs +++ b/src/environments.rs @@ -170,8 +170,11 @@ impl Environments { /// Enumerable environment names (compiled-in + file-defined), sorted. /// - /// File parse errors are silently swallowed; compiled names are still - /// returned. A fallible variant can be added later if needed. + /// Any error from reading or parsing the environments file (missing file, + /// permission/read error, or malformed TOML) is silently swallowed and only + /// the compiled-in names are returned. Use [`resolve`](Self::resolve) when + /// you need those errors surfaced; a fallible listing variant can be added + /// later if needed. #[must_use] pub fn list(&self) -> Vec { let mut names: std::collections::BTreeSet = self.defs.keys().cloned().collect(); From 10784952dfcc727e442ca265f479ae929dba1787 Mon Sep 17 00:00:00 2001 From: Jacob Page Date: Wed, 17 Jun 2026 13:11:56 -0700 Subject: [PATCH 20/31] refactor(environments): clear persist_active error, explicit env system, panic-safe lib tests Copilot review round 4: - persist_active: validate app_id up front and return a clear, actionable error instead of a misleading ConfigFile::save() path failure (+ test). - env_commands: set .with_system("env") on all four commands, matching the config/auth built-in groups for consistent output attribution. - lib tests (cli.rs, pkce.rs): replace manual UA reset with a shared RestoreDefaultUserAgent RAII guard (in transport::client) so a panicking assertion can't leak the mutated UA into later tests. Co-Authored-By: Claude Opus 4.8 --- src/auth/pkce.rs | 2 +- src/cli.rs | 2 +- src/env_commands.rs | 13 ++++++++++--- src/environments.rs | 24 ++++++++++++++++++++++++ src/transport/client.rs | 14 ++++++++++++++ 5 files changed, 50 insertions(+), 5 deletions(-) diff --git a/src/auth/pkce.rs b/src/auth/pkce.rs index 399aaa9..59693be 100644 --- a/src/auth/pkce.rs +++ b/src/auth/pkce.rs @@ -1264,6 +1264,7 @@ mod tests { let _guard = crate::transport::client::UA_TEST_LOCK .lock() .unwrap_or_else(std::sync::PoisonError::into_inner); + let _restore = crate::transport::client::RestoreDefaultUserAgent; crate::transport::set_default_user_agent("ua-probe/7.7"); let provider = test_provider().with_token_timeout(Duration::from_secs(12)); let request = provider @@ -1279,7 +1280,6 @@ mod tests { .expect("token request should set a user-agent"); assert_eq!(header, "ua-probe/7.7"); assert_eq!(request.timeout(), Some(&Duration::from_secs(12))); - crate::transport::set_default_user_agent("cli/dev"); } /// OAuth token requests must not hang indefinitely: the provider applies a diff --git a/src/cli.rs b/src/cli.rs index 2f8254f..0d50de7 100644 --- a/src/cli.rs +++ b/src/cli.rs @@ -2991,6 +2991,7 @@ mod user_agent_tests { let _guard = crate::transport::client::UA_TEST_LOCK .lock() .unwrap_or_else(std::sync::PoisonError::into_inner); + let _restore = crate::transport::client::RestoreDefaultUserAgent; crate::transport::set_default_user_agent("cli/dev"); let cli = Cli::new( CliConfig::new("uatest", "UA test", "uatest").with_build(BuildInfo::new("4.5.6")), @@ -3000,7 +3001,6 @@ mod user_agent_tests { crate::transport::client::default_user_agent(), "uatest/4.5.6" ); - crate::transport::set_default_user_agent("cli/dev"); } } diff --git a/src/env_commands.rs b/src/env_commands.rs index 8e555f6..5bcf9e4 100644 --- a/src/env_commands.rs +++ b/src/env_commands.rs @@ -20,7 +20,9 @@ use crate::{ pub fn env_command_group() -> RuntimeGroupSpec { RuntimeGroupSpec::new(GroupSpec::new("env", "Manage the active environment")) .with_command(RuntimeCommandSpec::new_with_context( - CommandSpec::new("list", "List known environments").no_auth(true), + CommandSpec::new("list", "List known environments") + .no_auth(true) + .with_system("env"), async |ctx| { let envs = ctx .middleware @@ -40,11 +42,15 @@ pub fn env_command_group() -> RuntimeGroupSpec { }, )) .with_command(RuntimeCommandSpec::new_with_context( - CommandSpec::new("get", "Show the active environment name").no_auth(true), + CommandSpec::new("get", "Show the active environment name") + .no_auth(true) + .with_system("env"), async |ctx| Ok(CommandResult::new(json!({ "active": ctx.middleware.env }))), )) .with_command(RuntimeCommandSpec::new_with_context( - CommandSpec::new("info", "Show the resolved active environment").no_auth(true), + CommandSpec::new("info", "Show the resolved active environment") + .no_auth(true) + .with_system("env"), async |ctx| { let env = ctx.environment()?; let oauth = env.oauth.map(|o| { @@ -65,6 +71,7 @@ pub fn env_command_group() -> RuntimeGroupSpec { .with_command(RuntimeCommandSpec::new_with_context( CommandSpec::new("set", "Set and persist the active environment") .no_auth(true) + .with_system("env") .with_tier(Tier::Mutate) .mutates(true) .with_arg(clap::Arg::new("name").required(true)), diff --git a/src/environments.rs b/src/environments.rs index 9f61d3c..cd51951 100644 --- a/src/environments.rs +++ b/src/environments.rs @@ -297,6 +297,16 @@ impl Environments { /// when the config file cannot be written. pub fn persist_active(&self, name: &str) -> Result<()> { self.resolve(name)?; // reject unknown names + // Persisting writes the engine config file, which is keyed by app_id. + // Validate it up front so a missing/invalid app_id yields a clear, + // actionable error rather than a misleading "no config path" failure + // from ConfigFile::save() that points at XDG/HOME. + if crate::config::config_file_path(&self.app_id).is_none() { + return Err(CliCoreError::message(format!( + "cannot persist active environment {name:?}: the environment system has no usable app_id; \ + set one via Environments::with_app_id (matching the CliConfig app_id)" + ))); + } let mut config = crate::config::ConfigFile::load(&self.app_id); config.set(Self::ACTIVE_ENV_KEY, name)?; config.save() @@ -410,6 +420,20 @@ mod tests { assert!(c.client_id.is_empty() && c.scopes.is_empty()); } + /// `persist_active` without an `app_id` returns a clear, actionable error + /// (mentioning `app_id`) rather than a misleading config-path failure. + #[test] + fn persist_active_without_app_id_errors_clearly() { + let err = sample() + .persist_active("prod") + .expect_err("persist without app_id should fail"); + let message = err.to_string(); + assert!( + message.contains("app_id"), + "error should mention app_id, got: {message}" + ); + } + #[test] fn builder_registers_compiled_environment() { let envs = Environments::new("prod").with_environment( diff --git a/src/transport/client.rs b/src/transport/client.rs index 3eb1c2f..2e36954 100644 --- a/src/transport/client.rs +++ b/src/transport/client.rs @@ -49,6 +49,20 @@ pub(crate) fn default_user_agent() -> String { #[cfg(test)] pub(crate) static UA_TEST_LOCK: std::sync::Mutex<()> = std::sync::Mutex::new(()); +/// Restores the process-wide default user-agent to the builtin on drop, so a +/// panicking assertion in a test that mutates it cannot leak the value into +/// later tests in this binary. Declare it after acquiring [`UA_TEST_LOCK`] so +/// the reset runs while the lock is still held. +#[cfg(test)] +pub(crate) struct RestoreDefaultUserAgent; + +#[cfg(test)] +impl Drop for RestoreDefaultUserAgent { + fn drop(&mut self) { + set_default_user_agent(BUILTIN_DEFAULT_USER_AGENT); + } +} + #[derive(serde::Deserialize)] struct GraphQlError { message: String, From 29bfcbb4987383bd735680e3f884a1c4e1710219 Mon Sep 17 00:00:00 2001 From: Jacob Page Date: Wed, 17 Jun 2026 13:17:24 -0700 Subject: [PATCH 21/31] docs(environments): clarify extra bag values must be TOML strings Copilot review round 5: Environment::extra is BTreeMap, so a non-OAuth environments.toml key with a non-string value (int/bool/array) fails to parse. Documented that extra values must be strings. Co-Authored-By: Claude Opus 4.8 --- docs/environments.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/docs/environments.md b/docs/environments.md index cc3776d..8798e3c 100644 --- a/docs/environments.md +++ b/docs/environments.md @@ -40,7 +40,8 @@ api_url = "https://api.ote.example.com" ``` The recognized OAuth keys — `client_id`, `auth_url`, `token_url`, and `scopes` (an array of strings) — are parsed into the typed `OAuthConfig` slice of the resolved `Environment`. -Every other string key is captured as a free-form field in `Environment::extra` (for example `api_url` above). +Every other key is captured as a free-form field in `Environment::extra`, which is a `BTreeMap` — so these values **must be TOML strings** (for example `api_url` above). +A non-OAuth key whose value is a number, boolean, or array fails to parse; quote it as a string instead. The `extra` bag is printed verbatim by `env info`, so it must not hold secrets. ## Environment-Variable Overrides From aa607161f5a5293af9767ebeb8c15be700a9079e Mon Sep 17 00:00:00 2001 From: Jacob Page Date: Wed, 17 Jun 2026 13:24:33 -0700 Subject: [PATCH 22/31] docs(environments): fix stale design status + resolver log level Copilot review round 6: - design spec frontmatter said the plan was pending; the plan now exists. - environments.md said resolver failures log at TRACE/DEBUG; they log at DEBUG. Co-Authored-By: Claude Opus 4.8 --- docs/environments.md | 2 +- docs/specs/2026-06-17-engine-environments-design.md | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/environments.md b/docs/environments.md index 8798e3c..3bcff84 100644 --- a/docs/environments.md +++ b/docs/environments.md @@ -102,7 +102,7 @@ Each field falls through to the next source when empty: Scopes follow the same pattern: the resolved environment's scopes when non-empty, otherwise the provider's base scopes. -If `Environments::resolve` fails — because the name is unknown or the environments file cannot be parsed — the provider logs the error at `TRACE`/`DEBUG` level and falls back to the next source. +If `Environments::resolve` fails — because the name is unknown or the environments file cannot be parsed — the provider logs the error at `DEBUG` level and falls back to the next source. No token or secret is included in the log; only the environment name and error message appear. ## Example diff --git a/docs/specs/2026-06-17-engine-environments-design.md b/docs/specs/2026-06-17-engine-environments-design.md index 3be4496..33445b1 100644 --- a/docs/specs/2026-06-17-engine-environments-design.md +++ b/docs/specs/2026-06-17-engine-environments-design.md @@ -1,7 +1,7 @@ # First-Class Environments in cli-engine — Design Date: 2026-06-17 -Status: Approved (design); pending implementation plan +Status: Implemented (plan in `docs/plans/2026-06-17-engine-environments-plan.md`) ## Context From 76cddc29d2bf8ab2b7628a4cf1cdddc0683d0af5 Mon Sep 17 00:00:00 2001 From: Jacob Page Date: Wed, 17 Jun 2026 13:30:35 -0700 Subject: [PATCH 23/31] test(foundation): pin default UA under lock in raw-body UA assertions Copilot review round 7: two HttpClient tests asserted user-agent: cli/dev while only taking USER_AGENT_TEST_LOCK, relying on other tests' cleanup. Pin the default explicitly under the lock and restore via RestoreDefaultUserAgent so the expectation is self-contained and panic-safe. Co-Authored-By: Claude Opus 4.8 --- tests/foundation.rs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/tests/foundation.rs b/tests/foundation.rs index b7f088e..22ae516 100644 --- a/tests/foundation.rs +++ b/tests/foundation.rs @@ -5531,6 +5531,8 @@ async fn http_client_do_raw_sends_method_content_type_body_and_decodes_json() { #[tokio::test] async fn http_client_do_raw_optional_none_body_matches_legacy_nil_reader() { let _ua_guard = USER_AGENT_TEST_LOCK.lock().await; + let _restore_ua = RestoreDefaultUserAgent; + transport::set_default_user_agent("cli/dev"); let server = TestServer::new(|request| { assert!(request.contains("OPTIONS /raw HTTP/1.1")); assert!(request.contains("user-agent: cli/dev")); @@ -5674,6 +5676,8 @@ async fn http_client_etag_if_match_and_multipart_without_response_skip_success_d #[tokio::test] async fn http_client_post_raw_none_body_omits_json_content_type_preserves_legacy() { let _ua_guard = USER_AGENT_TEST_LOCK.lock().await; + let _restore_ua = RestoreDefaultUserAgent; + transport::set_default_user_agent("cli/dev"); let server = TestServer::new(|request| { assert!(request.contains("POST /raw HTTP/1.1")); assert!(request.contains("user-agent: cli/dev")); From 9f2475b81e3ec15d2cd719af656c9432be4af346 Mon Sep 17 00:00:00 2001 From: Jacob Page Date: Wed, 17 Jun 2026 14:04:15 -0700 Subject: [PATCH 24/31] docs(environments): note list() performs blocking file I/O Copilot review round 8: document that Environments::list() does synchronous filesystem I/O when the config-file layer is enabled, like resolve(). Co-Authored-By: Claude Opus 4.8 --- src/environments.rs | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/environments.rs b/src/environments.rs index cd51951..79b3969 100644 --- a/src/environments.rs +++ b/src/environments.rs @@ -175,6 +175,13 @@ impl Environments { /// the compiled-in names are returned. Use [`resolve`](Self::resolve) when /// you need those errors surfaced; a fallible listing variant can be added /// later if needed. + /// + /// # Blocking + /// + /// When the config-file layer is enabled, this performs synchronous + /// filesystem I/O to read and parse `environments.toml` (like + /// [`resolve`](Self::resolve)). Avoid calling it repeatedly on a + /// latency-sensitive async path. #[must_use] pub fn list(&self) -> Vec { let mut names: std::collections::BTreeSet = self.defs.keys().cloned().collect(); From 2a6a80d073361d85b35b9c6ab5a4c8596cf6622e Mon Sep 17 00:00:00 2001 From: Jacob Page Date: Wed, 17 Jun 2026 15:02:01 -0700 Subject: [PATCH 25/31] refactor(pkce): resolve OAuth config once per token flow Previously each effective_client_id/auth_url/token_url/scopes call re-ran Environments::resolve(), so a single PKCE login/refresh re-read and re-parsed environments.toml up to ~4 times. Introduce effective_oauth(env) which resolves once and applies the resolved -> _OAUTH_* -> base precedence to all fields; run_pkce_flow_with resolves once and threads the result into exchange_code_for_token, and refresh resolves once. Drops the per-field getters in favor of the single struct. Addresses the remaining Copilot review point about redundant file reads. Co-Authored-By: Claude Opus 4.8 --- src/auth/pkce.rs | 141 +++++++++++++++++++++++++---------------------- 1 file changed, 76 insertions(+), 65 deletions(-) diff --git a/src/auth/pkce.rs b/src/auth/pkce.rs index 59693be..571c072 100644 --- a/src/auth/pkce.rs +++ b/src/auth/pkce.rs @@ -135,6 +135,17 @@ impl StoredToken { } } +/// The effective OAuth values for an environment, computed from a single +/// environment resolution. A token flow resolves this once and reuses it across +/// the authorize URL, code exchange, and refresh, rather than re-reading and +/// re-parsing `environments.toml` once per field. +struct EffectiveOAuth { + client_id: String, + auth_url: String, + token_url: String, + scopes: Vec, +} + /// OAuth 2.0 PKCE authentication provider. /// /// Stores one token per `(env, provider)` pair in the system keychain. @@ -429,45 +440,52 @@ impl PkceAuthProvider { } } - fn effective_client_id(&self, env: &str) -> String { - if let Some(oauth) = self.resolved_oauth(env) - && !oauth.client_id.is_empty() - { - return oauth.client_id; - } - let key = format!("{}_OAUTH_CLIENT_ID", self.env_prefix); - std::env::var(&key).unwrap_or_else(|_| self.client_id.clone()) - } - - fn effective_auth_url(&self, env: &str) -> String { - if let Some(oauth) = self.resolved_oauth(env) - && !oauth.auth_url.is_empty() - { - return oauth.auth_url; - } - let key = format!("{}_OAUTH_AUTH_URL", self.env_prefix); - std::env::var(&key).unwrap_or_else(|_| self.auth_url.clone()) - } - - fn effective_token_url(&self, env: &str) -> String { - if let Some(oauth) = self.resolved_oauth(env) - && !oauth.token_url.is_empty() - { - return oauth.token_url; + /// Computes the effective OAuth config for `env` with a SINGLE environment + /// resolution (at most one `environments.toml` read), then applies the + /// resolved → `_OAUTH_*` env var → base precedence to each field. + /// + /// Token flows call this once and reuse the result so they don't re-read the + /// environments file once per field. + fn effective_oauth(&self, env: &str) -> EffectiveOAuth { + let resolved = self.resolved_oauth(env); + // Resolved value when non-empty, else the provider-prefixed env var, else base. + let field = |resolved_value: Option<&String>, var_suffix: &str, base: &str| -> String { + if let Some(value) = resolved_value + && !value.is_empty() + { + return value.clone(); + } + std::env::var(format!("{}_OAUTH_{var_suffix}", self.env_prefix)) + .unwrap_or_else(|_| base.to_owned()) + }; + let scopes = match &resolved { + Some(oauth) if !oauth.scopes.is_empty() => oauth.scopes.clone(), + _ => self.scopes.clone(), + }; + EffectiveOAuth { + client_id: field( + resolved.as_ref().map(|o| &o.client_id), + "CLIENT_ID", + &self.client_id, + ), + auth_url: field( + resolved.as_ref().map(|o| &o.auth_url), + "AUTH_URL", + &self.auth_url, + ), + token_url: field( + resolved.as_ref().map(|o| &o.token_url), + "TOKEN_URL", + &self.token_url, + ), + scopes, } - let key = format!("{}_OAUTH_TOKEN_URL", self.env_prefix); - std::env::var(&key).unwrap_or_else(|_| self.token_url.clone()) } /// Default scopes for `env`: the resolved environment's scopes when /// non-empty, otherwise the provider's base scopes. fn effective_scopes(&self, env: &str) -> Vec { - if let Some(oauth) = self.resolved_oauth(env) - && !oauth.scopes.is_empty() - { - return oauth.scopes; - } - self.scopes.clone() + self.effective_oauth(env).scopes } fn effective_redirect_uri(&self) -> String { @@ -613,21 +631,21 @@ impl PkceAuthProvider { async fn run_pkce_flow_with(&self, env: &str, scopes: &[String]) -> Result { let (code_verifier, code_challenge) = pkce_challenge(); let state = random_state(); - let client_id = self.effective_client_id(env); - let auth_url = self.effective_auth_url(env); + // Resolve the OAuth config once for this whole flow (authorize + exchange). + let oauth = self.effective_oauth(env); let redirect_uri = self.effective_redirect_uri(); let scope = scopes.join(" "); let auth_params = [ ("response_type", "code"), - ("client_id", &client_id), + ("client_id", &oauth.client_id), ("redirect_uri", &redirect_uri), ("scope", &scope), ("state", &state), ("code_challenge", &code_challenge), ("code_challenge_method", "S256"), ]; - let url = url::Url::parse_with_params(&auth_url, &auth_params) + let url = url::Url::parse_with_params(&oauth.auth_url, &auth_params) .map_err(|err| CliCoreError::message(format!("invalid auth URL: {err}")))?; let (bind_port, callback_path) = self.parse_redirect_uri()?; @@ -646,7 +664,7 @@ impl PkceAuthProvider { let code = wait_for_callback(listener, &state, &callback_path, Duration::from_secs(120)).await?; - self.exchange_code_for_token(env, &code, &code_verifier, scopes) + self.exchange_code_for_token(&oauth, &code, &code_verifier, scopes) .await } @@ -671,24 +689,22 @@ impl PkceAuthProvider { async fn exchange_code_for_token( &self, - env: &str, + oauth: &EffectiveOAuth, code: &str, code_verifier: &str, requested_scopes: &[String], ) -> Result { let redirect_uri = self.effective_redirect_uri(); - let client_id = self.effective_client_id(env); - let token_url = self.effective_token_url(env); let params = [ ("grant_type", "authorization_code"), - ("client_id", &client_id), + ("client_id", &oauth.client_id), ("redirect_uri", &redirect_uri), ("code", code), ("code_verifier", code_verifier), ]; let response = self - .token_request(&token_url, ¶ms) + .token_request(&oauth.token_url, ¶ms) .send() .await .map_err(|err| CliCoreError::message(format!("token request failed: {err}")))?; @@ -710,15 +726,14 @@ impl PkceAuthProvider { refresh_token: &str, prior_scopes: &[String], ) -> Result { - let client_id = self.effective_client_id(env); - let token_url = self.effective_token_url(env); + let oauth = self.effective_oauth(env); let params = [ ("grant_type", "refresh_token"), - ("client_id", &client_id), + ("client_id", &oauth.client_id), ("refresh_token", refresh_token), ]; let response = self - .token_request(&token_url, ¶ms) + .token_request(&oauth.token_url, ¶ms) .send() .await .map_err(|err| CliCoreError::message(format!("token refresh failed: {err}")))?; @@ -1223,17 +1238,12 @@ mod tests { &["openid"], ) .with_environments(envs_for_test()); - assert_eq!(provider.effective_client_id("prod"), "prod-client"); + let oauth = provider.effective_oauth("prod"); + assert_eq!(oauth.client_id, "prod-client"); + assert_eq!(oauth.auth_url, "https://prod.example.com/auth"); + assert_eq!(oauth.token_url, "https://prod.example.com/token"); assert_eq!( - provider.effective_auth_url("prod"), - "https://prod.example.com/auth" - ); - assert_eq!( - provider.effective_token_url("prod"), - "https://prod.example.com/token" - ); - assert_eq!( - provider.effective_scopes("prod"), + oauth.scopes, vec!["openid".to_owned(), "prod.read".to_owned()] ); } @@ -1249,11 +1259,9 @@ mod tests { "base-client", &["openid"], ); - assert_eq!(provider.effective_client_id("anything"), "base-client"); - assert_eq!( - provider.effective_scopes("anything"), - vec!["openid".to_owned()] - ); + let oauth = provider.effective_oauth("anything"); + assert_eq!(oauth.client_id, "base-client"); + assert_eq!(oauth.scopes, vec!["openid".to_owned()]); } /// OAuth token traffic must carry the engine's configured default @@ -1871,7 +1879,10 @@ client_id = "file-prod-client" // The file overrides the compiled client id, which itself overrides the // provider's base — proving the wired provider reads the file layer. - assert_eq!(provider.effective_client_id("prod"), "file-prod-client"); + assert_eq!( + provider.effective_oauth("prod").client_id, + "file-prod-client" + ); } /// Legacy escape hatch: a NON-wired provider still honors the provider-prefixed @@ -1891,7 +1902,7 @@ client_id = "file-prod-client" // Non-wired provider: the legacy var overrides the base client id. let bare = test_provider(); - assert_eq!(bare.effective_client_id("prod"), "legacy-client"); + assert_eq!(bare.effective_oauth("prod").client_id, "legacy-client"); // Wired provider whose resolved env carries a non-empty client id: the // resolved environment wins over the legacy var. @@ -1901,6 +1912,6 @@ client_id = "file-prod-client" .with_environment("prod", EnvironmentDef::new().with_client_id("env-client")), ); let wired = test_provider().with_environments(environments); - assert_eq!(wired.effective_client_id("prod"), "env-client"); + assert_eq!(wired.effective_oauth("prod").client_id, "env-client"); } } From 9beae6f06b24c99c1776d9bc11db459828e78550 Mon Sep 17 00:00:00 2001 From: Jacob Page Date: Wed, 17 Jun 2026 15:07:45 -0700 Subject: [PATCH 26/31] docs: drop internal implementation plan from the PR Copilot review round 9: the implementation plan referenced internal superpowers:* tool names that aren't actionable for repo readers. It was one-off process scaffolding; remove it. The design spec (tool-agnostic) and the durable user docs (docs/environments.md, docs/concepts.md) remain. Co-Authored-By: Claude Opus 4.8 --- .../2026-06-17-engine-environments-plan.md | 1430 ----------------- .../2026-06-17-engine-environments-design.md | 2 +- 2 files changed, 1 insertion(+), 1431 deletions(-) delete mode 100644 docs/plans/2026-06-17-engine-environments-plan.md diff --git a/docs/plans/2026-06-17-engine-environments-plan.md b/docs/plans/2026-06-17-engine-environments-plan.md deleted file mode 100644 index acf623f..0000000 --- a/docs/plans/2026-06-17-engine-environments-plan.md +++ /dev/null @@ -1,1430 +0,0 @@ -# Engine Environments Implementation Plan - -> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. - -**Goal:** Make environments a first-class `cli-engine` concept — definitions, layered resolution, sticky selection, an `env` command group, and per-environment OAuth — so consumers stop hand-rolling it. - -**Architecture:** A new engine-owned `environments` module provides an `Environments` value (compiled defaults + `environments.toml` + `_*` env-var overrides, later wins) resolving to an `Environment { name, oauth, extra }`. `CliConfig::with_environments` registers it: it seeds the existing `Middleware.env`, registers a global `--env` flag, and auto-mounts an `env list/get/set/info` group (mirroring the built-in `auth`/`config` groups). `PkceAuthProvider::with_environments` makes the environment the single source of OAuth config, replacing this work stream's unreleased `with_environment`/`OAuthEnvironment` builder. - -**Tech Stack:** Rust, `clap`, `toml_edit` (already used by `ConfigFile`), `async-trait`, `tokio`. Feature `pkce-auth` gates the provider changes. - -**Spec:** `docs/specs/2026-06-17-engine-environments-design.md` - -**Conventions for every task:** Tests-first. Run `cargo test --features pkce-auth ...` for anything touching `src/auth`. Before each commit run `cargo fmt --all` and `cargo clippy --all-targets --features pkce-auth -- -D warnings`. Commit messages end with the project's `Co-Authored-By` trailer. Work on a feature branch, not `main`. - ---- - -## File Structure - -- Create `src/environments.rs` — `OAuthConfig`, `EnvironmentDef`, `Environment`, `Environments` (definitions, layered resolution, file + env-var sources, active-env helpers). One module, one responsibility: environment definitions and resolution. -- Create `src/env_commands.rs` — the `env` command group (`list`/`get`/`set`/`info`) as a `RuntimeGroupSpec`, mirroring `src/auth/commands.rs` and `src/config_commands.rs`. -- Modify `src/lib.rs` — declare/export the new modules and public types. -- Modify `src/cli.rs` — `CliConfig.environments` field + `with_environments` builder; `ensure_env_command`; `--env` global flag registration; seed `Middleware.env` from the resolved active environment. -- Modify `src/middleware.rs` — add `environments: Option>` to `Middleware` and the per-run snapshot so handlers can resolve. -- Modify `src/command.rs` — `CommandContext::environment()` accessor (memoized resolve of the active env). -- Modify `src/config.rs` — (only if needed) a constant for the active-env config key; reuse existing `get`/`set`/`save`. -- Modify `src/auth/pkce.rs` — remove `OAuthEnvironment`/`with_environment`; add `with_environments(Arc)`; `effective_*` reads the resolved `OAuthConfig` when env-wired, else the legacy `_OAUTH_*` fallback. -- Modify `docs/concepts.md` (or add `docs/environments.md`) — document the feature. - ---- - -## Phase 1 — `environments` module: types & resolution - -### Task 1: Core types - -**Files:** -- Create: `src/environments.rs` -- Modify: `src/lib.rs` (add `pub mod environments;` and re-exports) - -- [ ] **Step 1: Write the failing test** - -In `src/environments.rs`, start the file with the types and a test module: - -```rust -//! First-class environment definitions and layered resolution. -//! -//! An [`Environments`] value holds compiled-in environment definitions and, -//! optionally, an `environments.toml` file plus `_*` env-var overrides. -//! Resolving a name merges those layers (later wins) into an [`Environment`]. - -use std::collections::BTreeMap; - -use serde::Deserialize; - -use crate::{Result, error::CliCoreError}; - -/// Standard OAuth slice of an environment, consumed by `PkceAuthProvider`. -#[derive(Clone, Debug, Default, PartialEq, Eq)] -pub struct OAuthConfig { - /// OAuth client id. - pub client_id: String, - /// Authorization endpoint. - pub auth_url: String, - /// Token endpoint. - pub token_url: String, - /// Default scopes. - pub scopes: Vec, -} - -/// A fully-resolved environment. -#[derive(Clone, Debug, PartialEq, Eq)] -pub struct Environment { - /// Environment name (e.g. `prod`). - pub name: String, - /// OAuth configuration, present when the environment participates in OAuth. - pub oauth: Option, - /// App-specific fields (for example `api_url`). - pub extra: BTreeMap, -} - -#[cfg(test)] -mod tests { - use super::*; - - #[test] - fn oauth_config_defaults_are_empty() { - let c = OAuthConfig::default(); - assert!(c.client_id.is_empty() && c.scopes.is_empty()); - } -} -``` - -- [ ] **Step 2: Run test to verify it fails (compile-first)** - -Run: `cargo test --lib environments::tests::oauth_config_defaults_are_empty` -Expected: FAIL — `environments` module not declared in `lib.rs`. - -- [ ] **Step 3: Declare and export the module** - -In `src/lib.rs`, alongside the other `pub mod` lines, add: - -```rust -pub mod environments; -``` - -And in the public re-export area (near other `pub use`), add: - -```rust -pub use environments::{Environment, EnvironmentDef, Environments, OAuthConfig}; -``` - -(`EnvironmentDef`/`Environments` are added in Task 2; if the re-export fails to compile now, add only `Environment, OAuthConfig` here and extend the re-export in Task 2.) - -- [ ] **Step 4: Run test to verify it passes** - -Run: `cargo test --lib environments::tests::oauth_config_defaults_are_empty` -Expected: PASS. - -- [ ] **Step 5: Commit** - -```bash -git add src/environments.rs src/lib.rs -git commit -m "feat(environments): add OAuthConfig and Environment core types" -``` - ---- - -### Task 2: `EnvironmentDef` (partial layer) and `Environments` builder - -**Files:** -- Modify: `src/environments.rs` - -- [ ] **Step 1: Write the failing test** - -Add to the `tests` module: - -```rust -#[test] -fn builder_registers_compiled_environment() { - let envs = Environments::new("prod").with_environment( - "prod", - EnvironmentDef::new() - .with_client_id("prod-client") - .with_auth_url("https://api.example.com/authorize") - .with_token_url("https://api.example.com/token") - .with_scopes(&["openid"]) - .with_field("api_url", "https://api.example.com"), - ); - assert_eq!(envs.default_env(), "prod"); - assert_eq!(envs.list(), vec!["prod".to_owned()]); -} -``` - -- [ ] **Step 2: Run test to verify it fails** - -Run: `cargo test --lib environments::tests::builder_registers_compiled_environment` -Expected: FAIL — `EnvironmentDef`/`Environments` not found. - -- [ ] **Step 3: Implement the partial-layer type and builder** - -Add to `src/environments.rs` (above the `tests` module): - -```rust -/// An unresolved per-environment declaration (one layer of configuration). -/// -/// Fields are optional so layers can override individual values during -/// resolution. The same shape parses the `environments.toml` file. -#[derive(Clone, Debug, Default, Deserialize)] -pub struct EnvironmentDef { - #[serde(default)] - client_id: Option, - #[serde(default)] - auth_url: Option, - #[serde(default)] - token_url: Option, - #[serde(default)] - scopes: Option>, - /// Everything not recognised above is captured here (app-specific fields). - #[serde(flatten, default)] - extra: BTreeMap, -} - -impl EnvironmentDef { - /// Creates an empty declaration; every field falls back to lower layers. - #[must_use] - pub fn new() -> Self { - Self::default() - } - - /// Sets the OAuth client id. - #[must_use] - pub fn with_client_id(mut self, value: impl Into) -> Self { - self.client_id = Some(value.into()); - self - } - - /// Sets the authorization endpoint. - #[must_use] - pub fn with_auth_url(mut self, value: impl Into) -> Self { - self.auth_url = Some(value.into()); - self - } - - /// Sets the token endpoint. - #[must_use] - pub fn with_token_url(mut self, value: impl Into) -> Self { - self.token_url = Some(value.into()); - self - } - - /// Sets the default scopes. - #[must_use] - pub fn with_scopes(mut self, scopes: &[impl AsRef]) -> Self { - self.scopes = Some(scopes.iter().map(|s| s.as_ref().to_owned()).collect()); - self - } - - /// Sets an app-specific bag field. - #[must_use] - pub fn with_field(mut self, key: impl Into, value: impl Into) -> Self { - self.extra.insert(key.into(), value.into()); - self - } -} - -/// Engine-owned environment system: definitions + resolution + active-env state. -#[derive(Clone, Debug)] -pub struct Environments { - default: String, - defs: BTreeMap, - use_config_file: bool, - app_id: String, -} - -impl Environments { - /// Creates an environment system with the given default environment name. - #[must_use] - pub fn new(default_env: impl Into) -> Self { - Self { - default: default_env.into(), - defs: BTreeMap::new(), - use_config_file: false, - app_id: String::new(), - } - } - - /// Registers (or replaces) a compiled-in environment definition. - #[must_use] - pub fn with_environment(mut self, name: impl Into, def: EnvironmentDef) -> Self { - self.defs.insert(name.into(), def); - self - } - - /// Enables loading `//environments.toml` during resolution. - #[must_use] - pub fn with_config_file(mut self, enabled: bool) -> Self { - self.use_config_file = enabled; - self - } - - /// Sets the application id used to locate the config file. Set automatically - /// by [`crate::CliConfig::with_environments`]; only call directly in tests. - #[must_use] - pub fn with_app_id(mut self, app_id: impl Into) -> Self { - self.app_id = app_id.into(); - self - } - - /// The default environment name. - #[must_use] - pub fn default_env(&self) -> &str { - &self.default - } - - /// Enumerable environment names (compiled-in + file-defined), sorted. - #[must_use] - pub fn list(&self) -> Vec { - // File names folded in during Task 5; compiled-only for now. - self.defs.keys().cloned().collect() - } -} -``` - -Extend the `lib.rs` re-export to include `EnvironmentDef, Environments` if not already added in Task 1. - -- [ ] **Step 4: Run test to verify it passes** - -Run: `cargo test --lib environments::tests::builder_registers_compiled_environment` -Expected: PASS. - -- [ ] **Step 5: Commit** - -```bash -git add src/environments.rs src/lib.rs -git commit -m "feat(environments): add EnvironmentDef and Environments builder" -``` - ---- - -### Task 3: Layer merge + `resolve` (compiled + env-var) - -**Files:** -- Modify: `src/environments.rs` - -- [ ] **Step 1: Write the failing tests** - -Add to the `tests` module. These tests set process env vars, so serialize them on a lock and clean up: - -```rust -use std::sync::Mutex; -static ENV_LOCK: Mutex<()> = Mutex::new(()); - -fn sample() -> Environments { - Environments::new("prod") - .with_environment( - "prod", - EnvironmentDef::new() - .with_client_id("prod-client") - .with_auth_url("https://api.example.com/authorize") - .with_token_url("https://api.example.com/token") - .with_scopes(&["openid"]) - .with_field("api_url", "https://api.example.com"), - ) - .with_environment("dev", EnvironmentDef::new().with_client_id("dev-client")) -} - -#[test] -fn resolve_returns_compiled_record() { - let _g = ENV_LOCK.lock().unwrap_or_else(std::sync::PoisonError::into_inner); - let env = sample().resolve("prod").expect("prod resolves"); - let oauth = env.oauth.expect("oauth present"); - assert_eq!(oauth.client_id, "prod-client"); - assert_eq!(oauth.scopes, vec!["openid".to_owned()]); - assert_eq!(env.extra.get("api_url").map(String::as_str), Some("https://api.example.com")); -} - -#[test] -fn resolve_unknown_env_errors_with_known_names() { - let _g = ENV_LOCK.lock().unwrap_or_else(std::sync::PoisonError::into_inner); - let err = sample().resolve("nope").unwrap_err().to_string(); - assert!(err.contains("nope")); - assert!(err.contains("prod") && err.contains("dev")); -} - -#[test] -fn env_var_layer_overrides_oauth_and_known_bag_keys() { - let _g = ENV_LOCK.lock().unwrap_or_else(std::sync::PoisonError::into_inner); - // SAFETY: serialized by ENV_LOCK; removed below. - unsafe { - std::env::set_var("PROD_OAUTH_CLIENT_ID", "override-client"); - std::env::set_var("PROD_API_URL", "https://api.override.example.com"); - } - let env = sample().resolve("prod").expect("prod resolves"); - assert_eq!(env.oauth.unwrap().client_id, "override-client"); - assert_eq!(env.extra.get("api_url").map(String::as_str), Some("https://api.override.example.com")); - unsafe { - std::env::remove_var("PROD_OAUTH_CLIENT_ID"); - std::env::remove_var("PROD_API_URL"); - } -} -``` - -- [ ] **Step 2: Run tests to verify they fail** - -Run: `cargo test --lib environments::tests::resolve` -Expected: FAIL — `resolve` not found. - -- [ ] **Step 3: Implement merge + resolve** - -Add these methods to `impl Environments` and a free merge helper: - -```rust -impl Environments { - /// Resolves `name` by merging compiled defaults, the config file (Task 5), - /// and `_*` env-var overrides (later wins) into an [`Environment`]. - pub fn resolve(&self, name: &str) -> Result { - let compiled = self.defs.get(name); - let file = self.file_def(name)?; // Task 5 returns None until file support lands. - if compiled.is_none() && file.is_none() { - return Err(CliCoreError::message(format!( - "unknown environment {name:?}; known: {}", - self.list().join(", ") - ))); - } - let mut merged = EnvironmentDef::default(); - if let Some(def) = compiled { - merge_into(&mut merged, def); - } - if let Some(def) = &file { - merge_into(&mut merged, def); - } - apply_env_vars(name, &mut merged); - Ok(finalize(name, merged)) - } - - /// Loads a per-environment definition from the config file. Returns `None` - /// when the file layer is disabled or the env is absent. Implemented in Task 5. - fn file_def(&self, _name: &str) -> Result> { - Ok(None) - } -} - -/// Merges `src` into `dst`, with `src` winning on any field it sets. -fn merge_into(dst: &mut EnvironmentDef, src: &EnvironmentDef) { - if src.client_id.is_some() { - dst.client_id = src.client_id.clone(); - } - if src.auth_url.is_some() { - dst.auth_url = src.auth_url.clone(); - } - if src.token_url.is_some() { - dst.token_url = src.token_url.clone(); - } - if src.scopes.is_some() { - dst.scopes = src.scopes.clone(); - } - for (k, v) in &src.extra { - dst.extra.insert(k.clone(), v.clone()); - } -} - -/// Applies `_*` overrides: the three OAuth fields always, and any bag key -/// already present in the merged record (keyed `_`). -fn apply_env_vars(name: &str, def: &mut EnvironmentDef) { - let prefix = name.to_uppercase().replace('-', "_"); - if let Ok(v) = std::env::var(format!("{prefix}_OAUTH_CLIENT_ID")) { - def.client_id = Some(v); - } - if let Ok(v) = std::env::var(format!("{prefix}_OAUTH_AUTH_URL")) { - def.auth_url = Some(v); - } - if let Ok(v) = std::env::var(format!("{prefix}_OAUTH_TOKEN_URL")) { - def.token_url = Some(v); - } - let keys: Vec = def.extra.keys().cloned().collect(); - for key in keys { - let var = format!("{prefix}_{}", key.to_uppercase().replace('-', "_")); - if let Ok(v) = std::env::var(&var) { - def.extra.insert(key, v); - } - } -} - -/// Turns a fully-merged declaration into a resolved [`Environment`]. OAuth is -/// present when a client id was set by any layer. -fn finalize(name: &str, def: EnvironmentDef) -> Environment { - let oauth = def.client_id.as_ref().map(|client_id| OAuthConfig { - client_id: client_id.clone(), - auth_url: def.auth_url.clone().unwrap_or_default(), - token_url: def.token_url.clone().unwrap_or_default(), - scopes: def.scopes.clone().unwrap_or_default(), - }); - Environment { - name: name.to_owned(), - oauth, - extra: def.extra, - } -} -``` - -- [ ] **Step 4: Run tests to verify they pass** - -Run: `cargo test --lib environments::tests` -Expected: PASS (all resolve tests). - -- [ ] **Step 5: Commit** - -```bash -git add src/environments.rs -git commit -m "feat(environments): layered resolve with env-var overrides" -``` - ---- - -### Task 4: Config-file path helper - -**Files:** -- Modify: `src/config.rs` (reuse `config_file_path`), `src/environments.rs` - -- [ ] **Step 1: Write the failing test** - -Add to `environments::tests`: - -```rust -#[test] -fn environments_file_path_sits_next_to_config() { - let envs = sample().with_app_id("gddy").with_config_file(true); - let path = envs.config_file_path().expect("path resolves with app id"); - assert!(path.ends_with("gddy/environments.toml"), "got {path:?}"); -} -``` - -- [ ] **Step 2: Run test to verify it fails** - -Run: `cargo test --lib environments::tests::environments_file_path_sits_next_to_config` -Expected: FAIL — `config_file_path` not found. - -- [ ] **Step 3: Implement the path helper** - -In `src/config.rs`, confirm `config_file_path(app_id) -> Option` returns `//config.toml` (it does, per `config.rs:200`). Add to `impl Environments` in `src/environments.rs`: - -```rust -impl Environments { - /// Path to `environments.toml` next to the engine config file, or `None` - /// when the file layer is disabled or the config dir cannot be determined. - #[must_use] - pub fn config_file_path(&self) -> Option { - if !self.use_config_file { - return None; - } - let config = crate::config::config_file_path(&self.app_id)?; - Some(config.with_file_name("environments.toml")) - } -} -``` - -- [ ] **Step 4: Run test to verify it passes** - -Run: `cargo test --lib environments::tests::environments_file_path_sits_next_to_config` -Expected: PASS. - -- [ ] **Step 5: Commit** - -```bash -git add src/environments.rs -git commit -m "feat(environments): resolve environments.toml path" -``` - ---- - -### Task 5: Load `environments.toml` into `file_def` - -**Files:** -- Modify: `src/environments.rs` - -- [ ] **Step 1: Write the failing test** - -Add to `environments::tests` (writes a temp file; uses `tempfile`, already a dev-dependency). Inject the path via a test-only seam: - -```rust -#[test] -fn file_layer_overrides_compiled_and_adds_custom_env() { - let _g = ENV_LOCK.lock().unwrap_or_else(std::sync::PoisonError::into_inner); - let dir = tempfile::tempdir().expect("tempdir"); - let file = dir.path().join("environments.toml"); - std::fs::write( - &file, - r#" -[prod] -client_id = "file-client" - -[custom] -client_id = "custom-client" -api_url = "https://api.custom.example.com" -"#, - ) - .expect("write file"); - - let envs = sample().with_config_file(true).with_config_file_path_override(file); - - // File overrides the compiled prod client id, keeps compiled api_url. - let prod = envs.resolve("prod").expect("prod"); - assert_eq!(prod.oauth.unwrap().client_id, "file-client"); - assert_eq!(prod.extra.get("api_url").map(String::as_str), Some("https://api.example.com")); - - // Custom env exists only in the file. - let custom = envs.resolve("custom").expect("custom"); - assert_eq!(custom.oauth.unwrap().client_id, "custom-client"); - assert!(envs.list().contains(&"custom".to_owned())); -} -``` - -- [ ] **Step 2: Run test to verify it fails** - -Run: `cargo test --lib environments::tests::file_layer_overrides_compiled_and_adds_custom_env` -Expected: FAIL — `with_config_file_path_override` and real `file_def` not implemented. - -- [ ] **Step 3: Implement file loading** - -Add a path override field and parsing. Change the `Environments` struct to add `file_path_override: Option` (default `None`); update `new` accordingly. Replace `file_def` and add the parsed-file accessor + `list` update: - -```rust -impl Environments { - /// Test/advanced seam: force the environments file path. - #[must_use] - pub fn with_config_file_path_override(mut self, path: std::path::PathBuf) -> Self { - self.file_path_override = Some(path); - self.use_config_file = true; - self - } - - fn effective_file_path(&self) -> Option { - if let Some(path) = &self.file_path_override { - return Some(path.clone()); - } - self.config_file_path() - } - - /// Parses the environments file into a name -> def map. Missing file = empty. - fn file_defs(&self) -> Result> { - let Some(path) = self.effective_file_path() else { - return Ok(BTreeMap::new()); - }; - let text = match std::fs::read_to_string(&path) { - Ok(text) => text, - Err(err) if err.kind() == std::io::ErrorKind::NotFound => return Ok(BTreeMap::new()), - Err(err) => { - return Err(CliCoreError::message(format!( - "reading environments file {path:?}: {err}" - ))); - } - }; - toml_edit::de::from_str::>(&text).map_err(|err| { - CliCoreError::message(format!("parsing environments file {path:?}: {err}")) - }) - } - - fn file_def(&self, name: &str) -> Result> { - Ok(self.file_defs()?.remove(name)) - } -} -``` - -Update `list` to include file names: - -```rust - #[must_use] - pub fn list(&self) -> Vec { - let mut names: std::collections::BTreeSet = self.defs.keys().cloned().collect(); - if let Ok(file) = self.file_defs() { - names.extend(file.into_keys()); - } - names.into_iter().collect() - } -``` - -Add `use toml_edit;` is unnecessary (path-qualified). Ensure `BTreeMap` import already present. - -- [ ] **Step 4: Run test to verify it passes** - -Run: `cargo test --lib environments::tests` -Expected: PASS (all environments tests, including the file test). - -- [ ] **Step 5: Commit** - -```bash -git add src/environments.rs -git commit -m "feat(environments): load environments.toml as a resolution layer" -``` - ---- - -## Phase 2 — Active-environment persistence - -### Task 6: Read/write the active environment via `ConfigFile` - -**Files:** -- Modify: `src/environments.rs` - -- [ ] **Step 1: Write the failing test** - -Add to `environments::tests`: - -```rust -const ACTIVE_KEY: &str = "environment.active"; - -#[test] -fn active_env_round_trips_through_config_file() { - use crate::config::ConfigFile; - let mut cfg = ConfigFile::default(); - assert_eq!(Environments::active_from_config(&cfg), None); - - cfg.set(ACTIVE_KEY, "ote").expect("set"); - assert_eq!(Environments::active_from_config(&cfg).as_deref(), Some("ote")); -} - -#[test] -fn effective_active_prefers_override_then_config_then_default() { - use crate::config::ConfigFile; - let envs = sample(); - let mut cfg = ConfigFile::default(); - cfg.set(ACTIVE_KEY, "dev").expect("set"); - - assert_eq!(envs.effective_active(Some("prod"), &cfg), "prod"); // explicit wins - assert_eq!(envs.effective_active(None, &cfg), "dev"); // config next - let empty = ConfigFile::default(); - assert_eq!(envs.effective_active(None, &empty), "prod"); // default last -} -``` - -If `ConfigFile::default()` does not exist, construct via `ConfigFile::load("")` against a temp HOME, or add a `#[cfg(test)]` constructor. Check `src/config.rs` — prefer adding `impl Default for ConfigFile` returning an empty document if absent. - -- [ ] **Step 2: Run test to verify it fails** - -Run: `cargo test --lib environments::tests::active_env_round_trips_through_config_file` -Expected: FAIL — `active_from_config`/`effective_active` not found. - -- [ ] **Step 3: Implement persistence helpers** - -Add to `src/environments.rs`: - -```rust -/// Config-file key under which the sticky active environment is stored. -pub(crate) const ACTIVE_ENV_KEY: &str = "environment.active"; - -impl Environments { - /// Reads the persisted active environment from a loaded config file. - #[must_use] - pub fn active_from_config(config: &crate::config::ConfigFile) -> Option { - config.get(ACTIVE_ENV_KEY) - } - - /// Resolves the active environment name with precedence: - /// explicit `--env` override > persisted active > configured default. - #[must_use] - pub fn effective_active( - &self, - flag: Option<&str>, - config: &crate::config::ConfigFile, - ) -> String { - flag.map(ToOwned::to_owned) - .or_else(|| Self::active_from_config(config)) - .unwrap_or_else(|| self.default.clone()) - } - - /// Persists `name` as the active environment (loads, sets, saves a fresh - /// config file for `app_id`). Validates that `name` resolves first. - pub fn persist_active(&self, name: &str) -> Result<()> { - self.resolve(name)?; // reject unknown names - let mut config = crate::config::ConfigFile::load(&self.app_id); - config.set(ACTIVE_ENV_KEY, name)?; - config.save() - } -} -``` - -If `ConfigFile` lacks `Default`, add to `src/config.rs`: - -```rust -impl Default for ConfigFile { - fn default() -> Self { - Self { doc: toml_edit::DocumentMut::new(), path: None } - } -} -``` - -(Match `ConfigFile`'s real field names; adjust if they differ.) - -- [ ] **Step 4: Run tests to verify they pass** - -Run: `cargo test --lib environments::tests` -Expected: PASS. - -- [ ] **Step 5: Commit** - -```bash -git add src/environments.rs src/config.rs -git commit -m "feat(environments): persist and resolve the active environment" -``` - ---- - -## Phase 3 — Engine wiring - -### Task 7: `Middleware.environments` field - -**Files:** -- Modify: `src/middleware.rs` - -- [ ] **Step 1: Write the failing test** - -Add to the existing tests in `src/middleware.rs` (or create a small `#[cfg(test)] mod env_wire_tests`): - -```rust -#[test] -fn middleware_carries_optional_environments() { - use std::sync::Arc; - let mut mw = Middleware::new(); - assert!(mw.environments.is_none()); - mw.environments = Some(Arc::new(crate::environments::Environments::new("prod"))); - assert_eq!(mw.environments.as_ref().unwrap().default_env(), "prod"); -} -``` - -- [ ] **Step 2: Run test to verify it fails** - -Run: `cargo test --lib middleware_carries_optional_environments` -Expected: FAIL — no `environments` field. - -- [ ] **Step 3: Add the field** - -In `src/middleware.rs`, add to `struct Middleware` (near `config`): - -```rust - /// Optional environment system, set by `CliConfig::with_environments`. - pub environments: Option>, -``` - -Initialize it to `None` in `Middleware::new()` and in any other constructor/`Default`. If a per-run snapshot struct clones selected middleware fields, add `environments: self.environments.clone()` there too (search for where `config` is cloned into the snapshot and mirror it). - -- [ ] **Step 4: Run test to verify it passes** - -Run: `cargo test --lib middleware_carries_optional_environments` -Expected: PASS. - -- [ ] **Step 5: Commit** - -```bash -git add src/middleware.rs -git commit -m "feat(environments): thread environments through middleware" -``` - ---- - -### Task 8: `CliConfig.environments` + `with_environments` - -**Files:** -- Modify: `src/cli.rs` - -- [ ] **Step 1: Write the failing test** - -Add to the `user_agent_tests` module's file (or a new `#[cfg(test)] mod env_config_tests` at the end of `src/cli.rs`): - -```rust -#[test] -fn with_environments_stores_and_app_id_is_injected() { - let cfg = CliConfig::new("gddy", "GoDaddy CLI", "gddy") - .with_environments(crate::environments::Environments::new("prod").with_config_file(true)); - let envs = cfg.environments.as_ref().expect("environments set"); - // app_id is stamped from CliConfig so the file path resolves. - assert!(envs.config_file_path().is_some()); -} -``` - -- [ ] **Step 2: Run test to verify it fails** - -Run: `cargo test --lib env_config_tests::with_environments_stores_and_app_id_is_injected` -Expected: FAIL — no `environments` field/builder. - -- [ ] **Step 3: Add field + builder** - -In `src/cli.rs`, add to `struct CliConfig` (near `auth_providers`): - -```rust - /// Optional first-class environment system. - pub environments: Option, -``` - -Add the builder (near `with_default_auth_provider`), stamping `app_id` so file/persistence paths resolve: - -```rust - /// Registers a first-class environment system: mounts the `env` command - /// group, registers the global `--env` flag, and seeds the active - /// environment into middleware. The provided `Environments` has this - /// config's `app_id` applied so its config file and persistence resolve. - #[must_use] - pub fn with_environments(mut self, environments: crate::environments::Environments) -> Self { - self.environments = Some(environments.with_app_id(self.app_id.clone())); - self - } -``` - -`#[derive(Default)]` covers the new `Option` field. - -- [ ] **Step 4: Run test to verify it passes** - -Run: `cargo test --lib env_config_tests::with_environments_stores_and_app_id_is_injected` -Expected: PASS. - -- [ ] **Step 5: Commit** - -```bash -git add src/cli.rs -git commit -m "feat(environments): add CliConfig::with_environments" -``` - ---- - -### Task 9: Seed active env into middleware + register `--env` flag - -**Files:** -- Modify: `src/cli.rs` - -- [ ] **Step 1: Write the failing test** - -Add to `env_config_tests`: - -```rust -#[tokio::test] -async fn env_flag_overrides_default_and_reaches_middleware_env() { - use crate::{CommandResult, CommandSpec, RuntimeCommandSpec}; - use serde_json::json; - let mut cli = Cli::new( - CliConfig::new("envtest", "Env test", "envtest") - .with_environments( - crate::environments::Environments::new("prod") - .with_environment("prod", crate::environments::EnvironmentDef::new()) - .with_environment("ote", crate::environments::EnvironmentDef::new()), - ), - ); - cli.add_command(RuntimeCommandSpec::new_with_context( - CommandSpec::new("whichenv", "echo env").no_auth(true), - async |ctx| { Ok(CommandResult::new(json!({ "env": ctx.environment()?.name }))) }, - )); - let out = cli.run(["envtest", "whichenv", "--env", "ote", "--output", "json"]).await; - assert_eq!(out.exit_code, 0); - assert!(out.rendered.contains("\"env\"")); - assert!(out.rendered.contains("ote")); -} -``` - -(Adjust the `new_with_context` closure form to match `auth/commands.rs` usage; see Task 11 for the exact handler signature and the `ctx.environment()` accessor it depends on — implement Task 10 and 11's accessor first if running strictly in order. If so, reorder: do Task 10 before this test passes.) - -- [ ] **Step 2: Run test to verify it fails** - -Run: `cargo test --features pkce-auth --lib env_config_tests::env_flag_overrides_default_and_reaches_middleware_env` -Expected: FAIL — `--env` not registered / `ctx.environment()` missing. - -- [ ] **Step 3: Register the flag and seed middleware** - -In `Cli::new` (`src/cli.rs`), after `middleware.config = …` and before building `cli`, add: - -```rust - if let Some(environments) = &config.environments { - let environments = std::sync::Arc::new(environments.clone()); - // Seed the sticky/default active env now; the --env flag (applied - // per run) overrides it in run_with_depth. - middleware.env = environments.effective_active(None, &middleware.config); - middleware.environments = Some(environments); - } -``` - -Register the `--env` global flag when environments are configured. In `Cli::new`, where the root command is assembled (after `register_global_flags(root)`), add: - -```rust - if config.environments.is_some() { - root = root.arg( - clap::Arg::new("env") - .long("env") - .global(true) - .value_name("ENV") - .help("Target environment"), - ); - } -``` - -Apply the flag per run: in `run_with_depth` (`src/cli.rs:1190`), after args are parsed into matches but before middleware executes the command, set the per-run env from the flag when present. Locate where the per-run middleware/`MiddlewareRequest` is built and insert: - -```rust - // --env overrides the seeded active environment for this invocation. - if let Some(env) = parsed_global_env(&text_args) { - if let Some(environments) = &self.middleware.environments { - // Validate; unknown env -> error envelope. - environments.resolve(&env)?; - } - run_env_override = Some(env); - } -``` - -Add a small helper near the other `text_args` parsing helpers: - -```rust -fn parsed_global_env(text_args: &[String]) -> Option { - let mut iter = text_args.iter(); - while let Some(arg) = iter.next() { - if arg == "--env" { - return iter.next().cloned(); - } - if let Some(value) = arg.strip_prefix("--env=") { - return Some(value.to_owned()); - } - } - None -} -``` - -Thread `run_env_override` into the per-run middleware so `MiddlewareRequest.env`/`Middleware.env` reflects it. Mirror how the existing code copies `self.middleware` into the per-run middleware (search `run_with_depth` for where it clones middleware) and set `.env` to the override when present, else keep the seeded value. - -> Implementation note: if `run_with_depth` already clones `self.middleware` into a mutable per-run value, set `per_run.env = run_env_override.unwrap_or_else(|| self.middleware.env.clone())`. Keep the change minimal and within the existing clone path. - -- [ ] **Step 4: Run test to verify it passes** (after Task 10 accessor exists) - -Run: `cargo test --features pkce-auth --lib env_config_tests::env_flag_overrides_default_and_reaches_middleware_env` -Expected: PASS. - -- [ ] **Step 5: Commit** - -```bash -git add src/cli.rs -git commit -m "feat(environments): register --env and seed active env into middleware" -``` - ---- - -### Task 10: `CommandContext::environment()` accessor - -**Files:** -- Modify: `src/command.rs` - -- [ ] **Step 1: Write the failing test** - -The behavior is exercised by Task 9's integration test. Add a focused unit check in `src/command.rs` tests if a context can be constructed there; otherwise rely on Task 9. Minimum: add a doctest on the method (Step 3). - -- [ ] **Step 2: Run Task 9 test to verify it fails** - -Run: `cargo test --features pkce-auth --lib env_config_tests::env_flag_overrides_default_and_reaches_middleware_env` -Expected: FAIL — `ctx.environment()` missing. - -- [ ] **Step 3: Implement the accessor** - -In `src/command.rs`, add to `impl CommandContext` (near `config`): - -```rust - /// Resolves the active [`Environment`](crate::environments::Environment) for - /// this invocation. - /// - /// The active environment name is `Middleware.env` (set from `--env`, the - /// persisted active env, or the configured default). Returns an error if no - /// environment system was registered via - /// [`CliConfig::with_environments`](crate::CliConfig::with_environments) or - /// if the active name does not resolve. - pub fn environment(&self) -> Result { - let environments = self.middleware.environments.as_ref().ok_or_else(|| { - crate::error::CliCoreError::message("no environment system configured") - })?; - environments.resolve(&self.middleware.env) - } -``` - -(Return an owned `Environment` to avoid borrowing/memoization complexity; resolution is cheap and runs at most a few times per invocation. If the per-run snapshot exposes env differently than `self.middleware.env`, use that field name.) - -- [ ] **Step 4: Run test to verify it passes** - -Run: `cargo test --features pkce-auth --lib env_config_tests::env_flag_overrides_default_and_reaches_middleware_env` -Expected: PASS. - -- [ ] **Step 5: Commit** - -```bash -git add src/command.rs -git commit -m "feat(environments): add CommandContext::environment accessor" -``` - ---- - -## Phase 4 — `env` command group - -### Task 11: `env list/get/set/info` group, auto-mounted - -**Files:** -- Create: `src/env_commands.rs` -- Modify: `src/lib.rs` (add `mod env_commands;`), `src/cli.rs` (`ensure_env_command`, call it) - -- [ ] **Step 1: Write the failing tests** - -Add an integration test to `tests/foundation.rs` (mirrors existing `Cli::run` tests): - -```rust -#[tokio::test] -async fn env_group_lists_gets_and_sets_active_environment() { - use cli_engine::environments::{EnvironmentDef, Environments}; - let cli = Cli::new( - CliConfig::new("envcmds", "Env cmds", "envcmds").with_environments( - Environments::new("prod") - .with_environment("prod", EnvironmentDef::new().with_field("api_url", "https://p")) - .with_environment("ote", EnvironmentDef::new().with_field("api_url", "https://o")), - ), - ); - - let list = cli.run(["envcmds", "env", "list", "--output", "json"]).await; - assert_eq!(list.exit_code, 0); - assert!(list.rendered.contains("prod") && list.rendered.contains("ote")); - - let get = cli.run(["envcmds", "env", "get", "--output", "json"]).await; - assert_eq!(get.exit_code, 0); - assert!(get.rendered.contains("prod")); // default active - - let info = cli.run(["envcmds", "env", "info", "--env", "ote", "--output", "json"]).await; - assert_eq!(info.exit_code, 0); - assert!(info.rendered.contains("https://o")); -} -``` - -> Note: `env set` persists to the real per-app config file. Do NOT assert `set` persistence in a shared-process test unless the test isolates `$HOME`/`$XDG_CONFIG_HOME` via a temp dir guard. Cover `set` persistence in a `#[cfg(test)]` unit test in `environments.rs` (Task 6 already covers the round-trip) and keep this integration test to `list`/`get`/`info`. - -- [ ] **Step 2: Run test to verify it fails** - -Run: `cargo test --features pkce-auth --test foundation env_group_lists_gets_and_sets_active_environment` -Expected: FAIL — `env` group not mounted. - -- [ ] **Step 3: Implement the group** - -Create `src/env_commands.rs`, mirroring `src/auth/commands.rs` structure and `src/config_commands.rs`: - -```rust -//! Built-in `env` command group: list/get/set/info for environments. - -use serde_json::json; - -use crate::{ - CommandResult, CommandSpec, GroupSpec, RuntimeCommandSpec, RuntimeGroupSpec, - error::CliCoreError, -}; - -/// Builds the built-in `env` command group. -pub fn env_command_group() -> RuntimeGroupSpec { - RuntimeGroupSpec::new(GroupSpec::new("env", "Manage the active environment")) - .with_command(RuntimeCommandSpec::new_with_context( - CommandSpec::new("list", "List known environments").no_auth(true), - async |ctx| { - let envs = ctx - .middleware - .environments - .as_ref() - .ok_or_else(|| CliCoreError::message("no environment system configured"))?; - let active = ctx.middleware.env.clone(); - let items: Vec<_> = envs - .list() - .into_iter() - .map(|name| json!({ "name": name, "active": name == active })) - .collect(); - Ok(CommandResult::new(json!(items))) - }, - )) - .with_command(RuntimeCommandSpec::new_with_context( - CommandSpec::new("get", "Show the active environment").no_auth(true), - async |ctx| { - Ok(CommandResult::new(json!({ "active": ctx.middleware.env }))) - }, - )) - .with_command(RuntimeCommandSpec::new_with_context( - CommandSpec::new("info", "Show the resolved active environment").no_auth(true), - async |ctx| { - let env = ctx.environment()?; - let oauth = env.oauth.map(|o| { - json!({ "client_id": o.client_id, "auth_url": o.auth_url, "token_url": o.token_url, "scopes": o.scopes }) - }); - Ok(CommandResult::new(json!({ - "name": env.name, - "oauth": oauth, - "extra": env.extra, - }))) - }, - )) - .with_command(RuntimeCommandSpec::new_with_context( - CommandSpec::new("set", "Set and persist the active environment") - .no_auth(true) - .with_arg(clap::Arg::new("name").required(true)), - async |ctx| { - let envs = ctx - .middleware - .environments - .as_ref() - .ok_or_else(|| CliCoreError::message("no environment system configured"))?; - let name = ctx - .args - .get("name") - .and_then(|v| v.as_str()) - .ok_or_else(|| CliCoreError::message("missing environment name"))?; - envs.persist_active(name)?; - Ok(CommandResult::new(json!({ "active": name }))) - }, - )) -} -``` - -> Adjust `ctx.args` access to match how `auth/commands.rs` reads arguments (e.g. it may use `ctx.args.get(...)` or a typed accessor). Copy that idiom exactly. - -In `src/lib.rs` add `mod env_commands;` (private; only `cli.rs` needs it). In `src/cli.rs`, add `ensure_env_command` mirroring `ensure_config_command`: - -```rust - fn ensure_env_command(&mut self) { - if has_subcommand(&self.root, "env") { - return; - } - let group = crate::env_commands::env_command_group(); - let mut prefix = Vec::new(); - group.register_commands(&mut prefix, &mut self.commands); - let mut prefix = Vec::new(); - let clap_group = runtime_group_clap_command_with_schema_help( - &group, - &mut prefix, - &self.middleware.schema_registry, - ); - self.root = self.root.clone().subcommand(clap_group); - let category = self - .config - .admin_category - .clone() - .unwrap_or_else(|| DEFAULT_ADMIN_CATEGORY.to_owned()); - if !self.module_entries.iter().any(|e| e.name == "env") { - self.module_entries.push(ModuleHelpEntry { - category, - name: "env".to_owned(), - short: "Manage the active environment".to_owned(), - }); - } - self.refresh_root_long(); - } -``` - -Call it in `Cli::new`, right after the environments are seeded into middleware: - -```rust - if cli.config.environments.is_some() { - cli.ensure_env_command(); - } -``` - -- [ ] **Step 4: Run test to verify it passes** - -Run: `cargo test --features pkce-auth --test foundation env_group_lists_gets_and_sets_active_environment` -Expected: PASS. - -- [ ] **Step 5: Commit** - -```bash -git add src/env_commands.rs src/lib.rs src/cli.rs -git commit -m "feat(environments): add built-in env command group" -``` - ---- - -## Phase 5 — OAuth tie-in (replace `with_environment`) - -### Task 12: Remove `OAuthEnvironment`/`with_environment`; add `with_environments` - -**Files:** -- Modify: `src/auth/pkce.rs` - -- [ ] **Step 1: Write the failing test** - -Replace the existing `environment_override_selects_per_env_oauth_config` and `unconfigured_environment_falls_back_to_base_config` tests in `pkce.rs` with environment-sourced equivalents: - -```rust -fn envs_for_test() -> std::sync::Arc { - use crate::environments::{EnvironmentDef, Environments}; - std::sync::Arc::new( - Environments::new("prod").with_environment( - "prod", - EnvironmentDef::new() - .with_client_id("prod-client") - .with_auth_url("https://prod.example.com/auth") - .with_token_url("https://prod.example.com/token") - .with_scopes(&["openid", "prod.read"]), - ), - ) -} - -#[test] -fn environment_wired_provider_sources_oauth_from_resolver() { - let provider = PkceAuthProvider::new("godaddy", "https://base/auth", "https://base/token", "base-client", &["openid"]) - .with_environments(envs_for_test()); - assert_eq!(provider.effective_client_id("prod"), "prod-client"); - assert_eq!(provider.effective_auth_url("prod"), "https://prod.example.com/auth"); - assert_eq!(provider.effective_token_url("prod"), "https://prod.example.com/token"); - assert_eq!(provider.effective_scopes("prod"), vec!["openid".to_owned(), "prod.read".to_owned()]); -} - -#[test] -fn non_wired_provider_uses_base_config() { - let provider = PkceAuthProvider::new("godaddy", "https://base/auth", "https://base/token", "base-client", &["openid"]); - assert_eq!(provider.effective_client_id("anything"), "base-client"); - assert_eq!(provider.effective_scopes("anything"), vec!["openid".to_owned()]); -} -``` - -- [ ] **Step 2: Run tests to verify they fail** - -Run: `cargo test --features pkce-auth --lib auth::pkce::tests::environment_wired_provider_sources_oauth_from_resolver` -Expected: FAIL — `with_environments` not found; `OAuthEnvironment` still referenced elsewhere. - -- [ ] **Step 3: Replace the per-env map with a resolver** - -In `src/auth/pkce.rs`: - -1. Delete the `OAuthEnvironment` struct, its `impl`, the `environments: HashMap` field, the `with_environment` method, and any `OAuthEnvironment` doctest/re-export. -2. Add a resolver field and builder: - -```rust - /// Optional environment resolver; when set, per-env OAuth config comes from - /// the resolved environment instead of the base config / legacy env override. - environments: Option>, -``` - -```rust - /// Sources per-environment OAuth config from a shared [`Environments`]. - /// - /// Given an `env`, the provider resolves the environment and uses its - /// `OAuthConfig`. This is the single-source-of-truth path; prefer it over - /// the base `client_id`/`auth_url`/`token_url` when the consumer registers - /// environments via [`CliConfig::with_environments`](crate::CliConfig::with_environments). - #[must_use] - pub fn with_environments( - mut self, - environments: std::sync::Arc, - ) -> Self { - self.environments = Some(environments); - self - } -``` - -3. Initialize `environments: None` in `new`. -4. Rewrite the `effective_*` methods to prefer the resolver, then the legacy `_OAUTH_*` env override (kept for non-wired providers), then base: - -```rust - fn resolved_oauth(&self, env: &str) -> Option { - self.environments - .as_ref() - .and_then(|envs| envs.resolve(env).ok()) - .and_then(|resolved| resolved.oauth) - } - - fn effective_client_id(&self, env: &str) -> String { - if let Some(oauth) = self.resolved_oauth(env) { - if !oauth.client_id.is_empty() { - return oauth.client_id; - } - } - let key = format!("{}_OAUTH_CLIENT_ID", self.env_prefix); - std::env::var(&key).unwrap_or_else(|_| self.client_id.clone()) - } - - fn effective_auth_url(&self, env: &str) -> String { - if let Some(oauth) = self.resolved_oauth(env) { - if !oauth.auth_url.is_empty() { - return oauth.auth_url; - } - } - let key = format!("{}_OAUTH_AUTH_URL", self.env_prefix); - std::env::var(&key).unwrap_or_else(|_| self.auth_url.clone()) - } - - fn effective_token_url(&self, env: &str) -> String { - if let Some(oauth) = self.resolved_oauth(env) { - if !oauth.token_url.is_empty() { - return oauth.token_url; - } - } - let key = format!("{}_OAUTH_TOKEN_URL", self.env_prefix); - std::env::var(&key).unwrap_or_else(|_| self.token_url.clone()) - } - - fn effective_scopes(&self, env: &str) -> Vec { - if let Some(oauth) = self.resolved_oauth(env) { - if !oauth.scopes.is_empty() { - return oauth.scopes; - } - } - self.scopes.clone() - } -``` - -5. Remove the now-stale `perenv_provider` test helper and the old per-env tests replaced in Step 1. Keep all token-timeout, shared-client, and User-Agent tests/code from the prior work stream unchanged. -6. Remove `OAuthEnvironment` from any `lib.rs`/module re-exports. - -- [ ] **Step 4: Run tests to verify they pass** - -Run: `cargo test --features pkce-auth --lib auth::pkce::tests` -Expected: PASS (new env-wired tests + retained timeout/UA tests). - -- [ ] **Step 5: Commit** - -```bash -git add src/auth/pkce.rs src/lib.rs -git commit -m "feat(environments)!: source PkceAuthProvider OAuth from Environments" -``` - ---- - -## Phase 6 — Docs & final verification - -### Task 13: Document environments - -**Files:** -- Create: `docs/environments.md`; Modify: `docs/concepts.md` (add a short pointer) - -- [ ] **Step 1: Write the doc** - -Create `docs/environments.md` covering: the three resolution layers and precedence; the `environments.toml` schema (`[]` tables with `client_id`/`auth_url`/`token_url`/`scopes` typed and any other key as a string bag field); the `_*` env-var convention; the sticky active env + `--env` + `env list/get/set/info`; and the `PkceAuthProvider::with_environments` single-source pattern with a runnable example. Add a one-line pointer + link in `docs/concepts.md`. - -- [ ] **Step 2: Verify doctests/build** - -Run: `cargo test --doc --features pkce-auth` -Expected: PASS (any fenced `rust` examples compile). - -- [ ] **Step 3: Commit** - -```bash -git add docs/environments.md docs/concepts.md -git commit -m "docs(environments): document first-class environments" -``` - ---- - -### Task 14: Full verification sweep - -- [ ] **Step 1: Format + lint (both feature modes)** - -```bash -cargo fmt --all --check -cargo clippy --all-targets --features pkce-auth -- -D warnings -cargo clippy --all-targets -- -D warnings -``` -Expected: clean. - -- [ ] **Step 2: Tests + doctests + docs** - -```bash -cargo test --features pkce-auth --all-targets -cargo test --doc --features pkce-auth -RUSTDOCFLAGS='-D warnings' cargo doc --no-deps --features pkce-auth -cargo rustdoc --lib --features pkce-auth -- -W missing-docs -``` -Expected: all pass; missing-docs count zero. - -- [ ] **Step 3: Final commit if any fmt/doc fixups were needed** - -```bash -git add -A -git commit -m "chore(environments): formatting and doc fixups" -``` - ---- - -## Self-Review Notes (for the implementer) - -- **Spec coverage:** Tasks 1–5 = core/resolution/file; Task 6 = active-env persistence; Tasks 7–10 = engine wiring + context accessor; Task 11 = `env` group; Task 12 = OAuth single-source + `with_environment` removal; Task 13 = docs. Migration (gddy re-login, gdx greenfield) happens in those repos and is out of scope here. -- **Ordering caveat:** Task 9's integration test depends on the `ctx.environment()` accessor from Task 10 — implement Task 10's accessor before expecting Task 9's test to go green (or treat 9+10 as a pair). -- **Verify-as-you-go:** Several wiring snippets (`run_with_depth` per-run middleware clone, `ctx.args` idiom, `ConfigFile` field names for `Default`) must be matched to the real code at implementation time; the referenced mirror points (`ensure_config_command`, `auth_command_group`, `CommandContext::config`) are exact. -- **Non-negotiables:** env-var/config-file tests serialize on a lock and restore process state; `unsafe { set_var }` is test-only and paired with `remove_var`. diff --git a/docs/specs/2026-06-17-engine-environments-design.md b/docs/specs/2026-06-17-engine-environments-design.md index 33445b1..12d674e 100644 --- a/docs/specs/2026-06-17-engine-environments-design.md +++ b/docs/specs/2026-06-17-engine-environments-design.md @@ -1,7 +1,7 @@ # First-Class Environments in cli-engine — Design Date: 2026-06-17 -Status: Implemented (plan in `docs/plans/2026-06-17-engine-environments-plan.md`) +Status: Implemented ## Context From 6195697d394b4818d9cb0c3bbcf2eec5c2bf7d1f Mon Sep 17 00:00:00 2001 From: Jacob Page Date: Wed, 17 Jun 2026 15:14:41 -0700 Subject: [PATCH 27/31] refactor(env): add value_name/help to env set arg; drop drifting design spec Copilot review round 10: - env set: give the `name` positional a value_name and help text, matching the config/auth built-in groups for consistent --help output. - Remove the design spec: as a pre-implementation doc it had drifted from the shipped API (active_environment vs environment.active, &Environment vs owned, with_oauth vs with_client_id/...). The durable, accurate docs (docs/environments.md, docs/concepts.md) remain the source of truth. Co-Authored-By: Claude Opus 4.8 --- .../2026-06-17-engine-environments-design.md | 202 ------------------ src/env_commands.rs | 7 +- 2 files changed, 6 insertions(+), 203 deletions(-) delete mode 100644 docs/specs/2026-06-17-engine-environments-design.md diff --git a/docs/specs/2026-06-17-engine-environments-design.md b/docs/specs/2026-06-17-engine-environments-design.md deleted file mode 100644 index 12d674e..0000000 --- a/docs/specs/2026-06-17-engine-environments-design.md +++ /dev/null @@ -1,202 +0,0 @@ -# First-Class Environments in cli-engine — Design - -Date: 2026-06-17 -Status: Implemented - -## Context - -Two consumers of `cli-engine` — `gddy` (the GoDaddy developer CLI) and `gdx` — -each independently reimplement "multiple environments" (e.g. `dev`/`test`/`ote`/`prod`): - -- `gddy` (`src/environments/mod.rs`, `src/auth.rs`, `src/env/mod.rs`): a rich, - runtime-extensible system — compiled built-ins, a `~/.config/gddy/environments.toml` - file, and `_*` env-var overrides; records carry `client_id`, `auth_url`, - `token_url`, `api_url`, `domains_api_url`, `account_url`, `api_key`/`api_secret`; a - global `--env` flag with a sticky `.gdenv` state file; and `env list/get/set/info` - commands. It builds one `PkceAuthProvider` per environment on demand, with the - provider name set to the env name. -- `gdx` (`rust/src/auth.rs`): a hardcoded `match env { "dev"|"test"|"ote"|"prod" }` - mapping to `client_id` + `api_base`. No environment config file. - -The engine already owns *part* of an environment concept: `Middleware.env`, -default-env injection (`middleware.rs:586`), `CommandMeta::fixed_env()`, and the -`env` argument passed to `AuthProvider::get_credential`. It does not own the -`--env` flag, active-env persistence, environment definitions/resolution, or the -`env` command group — so each consumer rebuilds those. - -**Goal:** make environments a first-class engine mechanism so `gddy` and `gdx` -delete their hand-rolled flag/persistence/subcommands/resolution and net-remove -code, with the environment as the single source of truth for both API base URLs -and per-environment OAuth config. - -This supersedes the unreleased `PkceAuthProvider::with_environment` / -`OAuthEnvironment` builder added earlier in this work stream (still uncommitted), -which is replaced by the resolver described here. - -## Decisions (from brainstorming) - -1. Primary objective: **kill duplication** — both consumers migrate and remove code. -2. Boundary: **engine owns the format + resolution** (definitions, file schema, - env-var convention, merging), not just the lifecycle. -3. Record shape: **standard typed OAuth fields + a `BTreeMap` bag** - for app-specific fields. No generics on `Cli`/`Middleware`. -4. Resolution: **three layers, later wins** — compiled defaults < config file < - env-var overrides. -5. Selection: **sticky active env persisted in the per-app config file**, a `--env` - override, and an auto-mounted `env list/get/set/info` group. -6. OAuth tie-in: **the environment is the single source**; `PkceAuthProvider` reads - its OAuth config from the resolver. Replaces the unreleased `with_environment`. - -Rejected alternative: a generic `Environment` threaded through `Cli`/`Middleware` -(fully typed end-to-end but a large blast radius). The string bag is chosen instead. - -## Core Types - -```rust -/// Standard OAuth slice of an environment, consumed by PkceAuthProvider. -#[derive(Clone, Debug, Default)] -pub struct OAuthConfig { - pub client_id: String, - pub auth_url: String, - pub token_url: String, - pub scopes: Vec, -} - -/// One fully-resolved environment. -#[derive(Clone, Debug)] -pub struct Environment { - pub name: String, - /// Present when the environment participates in OAuth. - pub oauth: Option, - /// App-specific fields (api_url, domains_api_url, account_url, …). - pub extra: BTreeMap, -} - -/// Engine-owned environment system: definitions + resolution + active-env state. -/// Built once by the consumer and shared (Arc) between CliConfig and any -/// PkceAuthProvider that is environment-driven. -#[derive(Clone, Debug)] -pub struct Environments { /* compiled defaults, file path, config handle, default name */ } -``` - -Builder sketch: - -```rust -Environments::new("prod") // default env name - .with_environment("prod", EnvironmentDef::new() - .with_oauth(OAuthConfig { client_id: "…".into(), auth_url: "…".into(), - token_url: "…".into(), scopes: vec!["openid".into()] }) - .with_field("api_url", "https://api.godaddy.com")) - .with_environment("ote", /* … */) - .with_config_file(true); // enable ~/.config//environments.toml + env-var layer -``` - -`EnvironmentDef` is the *unresolved* per-env declaration (the same shape used to -parse the TOML file); `Environment` is the *resolved* result after merging layers. - -## Resolution - -`Environments::resolve(&self, name: &str) -> Result` merges, later wins: - -1. **Compiled defaults** declared via `with_environment` on `CliConfig`/`Environments`. -2. **Config file** `~/.config//environments.toml` (XDG/`%APPDATA%` per platform, - same dir the engine already uses for credentials/config). Schema mirrors `gddy`'s: - standard OAuth keys parse into `OAuthConfig`; every other key lands in `extra`. - Enables user-defined custom environments. -3. **Env-var overrides**, keyed by env name (uppercased, `-`→`_`): - - `_OAUTH_CLIENT_ID`, `_OAUTH_AUTH_URL`, `_OAUTH_TOKEN_URL` - - `_` for bag fields, e.g. `PROD_API_URL`. - -Behavior: -- Unknown env name → typed error listing enumerable env names. -- `Environments::list()` returns compiled + file-defined envs. Environment-variable - layers only *override fields* of an environment already defined by a compiled - default or the file; env vars alone do not define a new, selectable environment. -- Merge is field-level: a file or env-var layer may override individual OAuth fields - or individual bag keys without restating the whole record. - -## Selection & Lifecycle - -- The engine registers a **global `--env` flag** only when environments are configured. -- **Sticky active env** stored in the existing per-app `ConfigFile` under a new - `active_environment` key (no separate state file). Precedence for the active env: - `--env` value > persisted active > `Environments` default. -- Auto-mounted **`env` command group** (mounted like the built-in `auth` group): - - `env list` — enumerable environments, marking the active one. - - `env get` — prints the active environment name. - - `env set ` — validates via `resolve()` (the name must be defined by a - compiled default or `environments.toml`), then persists to `ConfigFile`. - - `env info` — shows the resolved active environment (OAuth endpoints + bag), - with secrets omitted. -- The resolved active env name is written to the existing `Middleware.env`, so - default-env injection (`middleware.rs:586`) and `CommandMeta::fixed_env()` keep - working unchanged. - -## Engine Wiring & Handler Access - -- `CliConfig::with_environments(Environments)` — registers the system: mounts the - `env` group, registers `--env`, seeds `Middleware.env` from the resolved active env. -- Handlers access the resolved active environment through a context accessor: - `ctx.environment() -> Result<&Environment>` (resolved once per run and memoized, - like the credential resolver). Handlers read base URLs from `env.extra`: - `env.extra.get("api_url")`. -- `--schema` / `--dry-run` short-circuits must not force file/env resolution - (resolution is lazy, consistent with credential-store resolution today). - -## OAuth Tie-In - -- `PkceAuthProvider::with_environments(Arc)` **replaces** the unreleased - `with_environment` / `OAuthEnvironment` builder (removed outright — no deprecation - needed since it is uncommitted). -- An environment-wired provider, given `env`, calls `environments.resolve(env)?` and - uses the resulting `OAuthConfig` (`client_id`/`auth_url`/`token_url`/`scopes`) for the - flow. One definition drives both API base URLs (bag) and OAuth; custom/file/env-var - environments get OAuth automatically. -- The pre-existing released `_OAUTH_*` env override remains **only** for - providers that are *not* environment-wired (back-compat for current consumers). - Environment-wired providers use the `_OAUTH_*` layer instead. -- Provider-level config that is genuinely not per-env (redirect URI/port, app_id, - identity claims, token timeout, shared client) stays on `PkceAuthProvider` as today. - -## Migration & Back-Compat - -- **`with_environment` removal**: safe — uncommitted in this work stream. -- **gddy credential storage keys**: today provider-name = env-name yields keys - `gddy//`. A single environment-wired provider (e.g. named `godaddy`) - yields `gddy/godaddy/`. This changes the keys, so existing users re-authenticate - once after upgrade. **Decision: accept the one-time re-login**, documented in the - changelog. No storage-key compat shim. -- **gddy config file**: the engine's `environments.toml` schema is a superset of - gddy's existing file, so migration is near-zero (standard OAuth keys typed, the rest - into `extra`). gddy's URL derivation (`auth_url`/`token_url` from `api_url`) becomes - declaring explicit URLs in compiled defaults, or specifying them per custom env; - the engine does not own a derivation rule. -- **gdx**: greenfield — declares compiled defaults, deletes its `match env` OAuth maps, - and may drop its bespoke env plumbing. gdx's `ote`→`test` SSO aliasing stays - app-side (it is not OAuth-environment resolution). -- The per-env token timeout, shared token client, and User-Agent work from this work - stream are unaffected and carry forward. - -## Testing - -- **Unit:** resolution precedence across all three layers; field-level merge; - unknown-env error contents; `list()` enumeration; env-var layers override fields of - a defined env (not define new ones); - env-var keying (`_OAUTH_*`, `_`); active-env persistence round-trip - through `ConfigFile`; lazy resolution (no file/env access on `--schema`/`--dry-run`). -- **Integration (`Cli::run`):** `env set`/`get`/`list`/`info`; `--env` overrides the - sticky active env; an environment-wired `PkceAuthProvider` selects per-env OAuth - (verified with the offline request-builder seam already used for UA/timeout tests). -- **Doctests** on the public builders (`Environments`, `EnvironmentDef`, `OAuthConfig`, - `CliConfig::with_environments`, `PkceAuthProvider::with_environments`). -- Env-var and config-file tests serialize on a lock and restore process state, matching - existing `credential_store_config`/User-Agent test conventions. - -## Clarifications (decided; called out for implementation) - -- `env set` validates via `resolve()`; the name must be defined by a compiled default - or `environments.toml`. Environment variables override fields of a defined env but - cannot define a new, selectable environment on their own. -- Exact reconciliation of the `_OAUTH_*` (environment layer) vs `_OAUTH_*` - (legacy provider layer) precedence is: environment-wired providers use the environment - layer exclusively; non-wired providers retain the legacy layer. diff --git a/src/env_commands.rs b/src/env_commands.rs index 5bcf9e4..5df9151 100644 --- a/src/env_commands.rs +++ b/src/env_commands.rs @@ -74,7 +74,12 @@ pub fn env_command_group() -> RuntimeGroupSpec { .with_system("env") .with_tier(Tier::Mutate) .mutates(true) - .with_arg(clap::Arg::new("name").required(true)), + .with_arg( + clap::Arg::new("name") + .value_name("NAME") + .required(true) + .help("Environment to make active (must be a known environment)"), + ), async |ctx| { let envs = ctx .middleware From 54889d1307cd471782704bd271f4f44d420741e3 Mon Sep 17 00:00:00 2001 From: Jacob Page Date: Wed, 17 Jun 2026 15:23:34 -0700 Subject: [PATCH 28/31] test(foundation): apply RestoreDefaultUserAgent guard to all UA lock tests Copilot review round 11 flagged one more UA-mutating test lacking the panic-safe guard. Rather than fix piecemeal, audited every test holding USER_AGENT_TEST_LOCK and applied the RestoreDefaultUserAgent RAII guard (and an explicit cli/dev pin where asserting the default) to all of them, so a panicking assertion can never leak a mutated default UA into later tests in this binary. Co-Authored-By: Claude Opus 4.8 --- tests/foundation.rs | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/tests/foundation.rs b/tests/foundation.rs index 22ae516..15a0150 100644 --- a/tests/foundation.rs +++ b/tests/foundation.rs @@ -4958,8 +4958,10 @@ async fn provider_bearer_injector_empty_token_does_not_short_circuit_cache_prese #[tokio::test] async fn client_credentials_injector_requests_and_caches_bearer_token() { // The injector tags its token request with the process default User-Agent; - // hold the user-agent lock and pin the default so the assertion is stable. + // hold the user-agent lock and pin the default so the assertion is stable, + // restoring it on unwind via the RAII guard while the lock is still held. let _ua_guard = USER_AGENT_TEST_LOCK.lock().await; + let _restore_ua = RestoreDefaultUserAgent; transport::set_default_user_agent("cli/dev"); let token_requests = Arc::new(AtomicUsize::new(0)); let token_requests_for_server = Arc::clone(&token_requests); @@ -5100,6 +5102,8 @@ async fn client_credentials_injector_missing_token_and_negative_expiry_match_leg #[tokio::test] async fn http_client_get_sets_headers_and_decodes_json() { let _guard = USER_AGENT_TEST_LOCK.lock().await; + let _restore_ua = RestoreDefaultUserAgent; + transport::set_default_user_agent("cli/dev"); let server = TestServer::new(|request| { assert!(request.contains("GET /thing HTTP/1.1")); assert!(request.contains("user-agent: cli/dev")); @@ -5205,6 +5209,7 @@ async fn http_client_null_json_returns_default_result_preserves_legacy_zero_valu #[tokio::test] async fn http_client_set_default_user_agent_affects_new_clients_only() { let _guard = USER_AGENT_TEST_LOCK.lock().await; + let _restore_ua = RestoreDefaultUserAgent; transport::set_default_user_agent("cli/custom"); let custom_server = TestServer::new(|request| { assert!(request.contains("GET /thing HTTP/1.1")); @@ -5241,8 +5246,6 @@ async fn http_client_set_default_user_agent_affects_new_clients_only() { .await .expect("explicit user agent should override default"); assert_eq!(value, json!({"ok": true})); - - transport::set_default_user_agent("cli/dev"); } #[tokio::test] @@ -5492,6 +5495,8 @@ async fn http_client_default_headers_can_override_json_content_type_preserves_le #[tokio::test] async fn http_client_do_raw_sends_method_content_type_body_and_decodes_json() { let _guard = USER_AGENT_TEST_LOCK.lock().await; + let _restore_ua = RestoreDefaultUserAgent; + transport::set_default_user_agent("cli/dev"); let server = TestServer::new(|request| { assert!(request.contains("OPTIONS /raw HTTP/1.1")); assert!(request.contains("content-type: application/x-www-form-urlencoded")); From 8a82432bed111a8209f298d154573e4e35d76ca8 Mon Sep 17 00:00:00 2001 From: Jacob Page Date: Wed, 17 Jun 2026 15:30:33 -0700 Subject: [PATCH 29/31] fix(environments): readable unknown-env error when no envs defined Copilot review round 12: with no compiled or file-defined environments, the unknown-env error rendered a dangling "known: ". Render "(none defined)" instead (+ test). Co-Authored-By: Claude Opus 4.8 --- src/environments.rs | 22 ++++++++++++++++++++-- 1 file changed, 20 insertions(+), 2 deletions(-) diff --git a/src/environments.rs b/src/environments.rs index 79b3969..72b3084 100644 --- a/src/environments.rs +++ b/src/environments.rs @@ -220,9 +220,13 @@ impl Environments { let mut known: std::collections::BTreeSet = self.defs.keys().cloned().collect(); known.extend(all_file_defs.into_keys()); let known_list: Vec = known.into_iter().collect(); - return Err(CliCoreError::message(format!( - "unknown environment {name:?}; known: {}", + let known_display = if known_list.is_empty() { + "(none defined)".to_owned() + } else { known_list.join(", ") + }; + return Err(CliCoreError::message(format!( + "unknown environment {name:?}; known: {known_display}" ))); } let mut merged = EnvironmentDef::default(); @@ -427,6 +431,20 @@ mod tests { assert!(c.client_id.is_empty() && c.scopes.is_empty()); } + /// With no environments defined at all, the unknown-env error renders a + /// readable placeholder instead of a dangling `known: `. + #[test] + fn resolve_unknown_env_with_no_defs_uses_placeholder() { + let err = Environments::new("prod") + .resolve("prod") + .expect_err("nothing defined should fail"); + let message = err.to_string(); + assert!( + message.contains("(none defined)"), + "expected placeholder, got: {message}" + ); + } + /// `persist_active` without an `app_id` returns a clear, actionable error /// (mentioning `app_id`) rather than a misleading config-path failure. #[test] From 66f3d1e8b466e662e20688b733a3fe0a73a35edc Mon Sep 17 00:00:00 2001 From: Jacob Page Date: Wed, 17 Jun 2026 15:40:19 -0700 Subject: [PATCH 30/31] perf(pkce): resolve default scopes lazily off the cached-token hot path get_credential_for resolved effective_scopes(env) up front on every credential lookup, triggering an environments.toml read even when a cached token already covers the required scopes. plan_step_up's coverage decision needs only granted/required, so drop defaults from it (StepUp::Reauthenticate no longer carries the union) and resolve per-env defaults only on the two paths that actually re-authenticate. Hot path (cached token covers required) now does no environment file I/O. Co-Authored-By: Claude Opus 4.8 --- src/auth/pkce.rs | 58 ++++++++++++++++++++++-------------------------- 1 file changed, 27 insertions(+), 31 deletions(-) diff --git a/src/auth/pkce.rs b/src/auth/pkce.rs index 571c072..25048c3 100644 --- a/src/auth/pkce.rs +++ b/src/auth/pkce.rs @@ -764,7 +764,6 @@ impl AuthProvider for PkceAuthProvider { async fn get_credential_for(&self, req: &CredentialRequest<'_>) -> Result { let env = req.env; let required = &req.meta.scopes; - let defaults = self.effective_scopes(env); // Look for a usable token WITHOUT launching a flow, so we can pick the // scope set for a single login rather than authenticating twice (e.g. @@ -776,14 +775,16 @@ impl AuthProvider for PkceAuthProvider { // server has no silent scope-expansion grant, so in non-interactive // contexts we fail fast rather than hang on the callback timeout. let granted = granted_scopes(&token); - match plan_step_up(&defaults, &granted, required, session_is_interactive()) { + match plan_step_up(&granted, required, session_is_interactive()) { StepUp::Covered => return Ok(self.build_credential(env, &token)), StepUp::MissingNonInteractive(missing) => { return Err(missing_scope_error(env, &missing)); } - // Union (defaults ∪ already-granted ∪ required) so step-up never - // drops scopes acquired by an earlier login or step-up. - StepUp::Reauthenticate(union) => { + // Re-consent: resolve per-env defaults only now (off the + // cached-token hot path) and request defaults ∪ already-granted ∪ + // required so step-up never drops previously-acquired scopes. + StepUp::Reauthenticate => { + let union = union_scopes(&self.effective_scopes(env), &granted, required); let token = self.reauthenticate(env, &union).await?; ensure_granted(env, &token, required)?; return Ok(self.build_credential(env, &token)); @@ -792,7 +793,7 @@ impl AuthProvider for PkceAuthProvider { } // No usable token: authenticate once, requesting defaults ∪ required. - let union = union_scopes(&defaults, &[], required); + let union = union_scopes(&self.effective_scopes(env), &[], required); let token = self.reauthenticate(env, &union).await?; ensure_granted(env, &token, required)?; Ok(self.build_credential(env, &token)) @@ -1044,19 +1045,22 @@ fn granted_scopes(token: &StoredToken) -> Vec { enum StepUp { /// The token already covers every required scope. Covered, - /// Re-authenticate requesting this scope set (defaults ∪ granted ∪ required). - Reauthenticate(Vec), + /// Re-authenticate (interactively) to acquire the missing scopes. The caller + /// builds the requested set (defaults ∪ granted ∪ required) only on this + /// path, so resolving per-env default scopes stays off the cached-token hot + /// path. + Reauthenticate, /// Scopes are missing but the session is non-interactive, so step-up cannot /// prompt; carries the missing scopes for the error message. MissingNonInteractive(Vec), } -fn plan_step_up( - defaults: &[String], - granted: &[String], - required: &[String], - interactive: bool, -) -> StepUp { +/// Decides the step-up action from what the token grants versus what the command +/// requires. Deliberately does NOT take the per-env default scopes: the coverage +/// decision needs only `granted`/`required`, so the caller can avoid resolving +/// defaults (potential `environments.toml` I/O) when a cached token already +/// covers the requirement. +fn plan_step_up(granted: &[String], required: &[String], interactive: bool) -> StepUp { let missing: Vec = required .iter() .filter(|scope| !granted.iter().any(|have| have == *scope)) @@ -1065,7 +1069,7 @@ fn plan_step_up( if missing.is_empty() { StepUp::Covered } else if interactive { - StepUp::Reauthenticate(union_scopes(defaults, granted, required)) + StepUp::Reauthenticate } else { StepUp::MissingNonInteractive(missing) } @@ -1383,30 +1387,22 @@ mod tests { #[test] fn plan_step_up_covers_reauths_and_fails_non_interactive() { - let defaults = vec!["base".to_owned()]; let granted = vec!["base".to_owned(), "read".to_owned()]; let read = vec!["read".to_owned()]; let write = vec!["write".to_owned()]; - // Already covered. - assert_eq!( - plan_step_up(&defaults, &granted, &read, true), - StepUp::Covered - ); - // Missing + interactive → reauth requesting the union. - assert_eq!( - plan_step_up(&defaults, &granted, &write, true), - StepUp::Reauthenticate(vec![ - "base".to_owned(), - "read".to_owned(), - "write".to_owned() - ]) - ); + // Already covered (decision needs only granted vs required). + assert_eq!(plan_step_up(&granted, &read, true), StepUp::Covered); + // Missing + interactive → reauth (the caller builds the union with + // per-env defaults only on this path). + assert_eq!(plan_step_up(&granted, &write, true), StepUp::Reauthenticate); // Missing + non-interactive → fail fast, carrying the missing scopes. assert_eq!( - plan_step_up(&defaults, &granted, &write, false), + plan_step_up(&granted, &write, false), StepUp::MissingNonInteractive(vec!["write".to_owned()]) ); + // The union itself (defaults ∪ granted ∪ required) is covered by + // union_scopes' own test. } /// An opaque cached token whose recorded scopes cover the requirement is From a083886f2c4f3b057762a872cf8b12d3b3b8ec37 Mon Sep 17 00:00:00 2001 From: Jacob Page Date: Wed, 17 Jun 2026 15:48:26 -0700 Subject: [PATCH 31/31] docs(transport): set_default_user_agent also governs token traffic Copilot review round 14: the doc said it only affected subsequently-created HttpClients, but the default UA now also applies to the PKCE provider's token/refresh requests and the client-credentials injector. Updated the doc. Co-Authored-By: Claude Opus 4.8 --- src/transport/client.rs | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/transport/client.rs b/src/transport/client.rs index 2e36954..9fff60d 100644 --- a/src/transport/client.rs +++ b/src/transport/client.rs @@ -19,7 +19,13 @@ const BASE_BACKOFF: Duration = Duration::from_millis(500); const BUILTIN_DEFAULT_USER_AGENT: &str = "cli/dev"; static DEFAULT_USER_AGENT: OnceLock> = OnceLock::new(); -/// Sets the user-agent used by subsequently created [`HttpClient`] values. +/// Sets the process-wide default user-agent for outbound requests. +/// +/// Applies to subsequently created [`HttpClient`] values (those that do not set +/// their own via [`HttpClientBuilder::user_agent`]) and to the engine's other +/// outbound token traffic that reads this default — the PKCE provider's +/// token/refresh requests and the client-credentials injector. A per-client +/// user-agent still overrides it for that client. pub fn set_default_user_agent(user_agent: impl Into) { let lock = DEFAULT_USER_AGENT.get_or_init(|| RwLock::new(BUILTIN_DEFAULT_USER_AGENT.to_owned()));