diff --git a/CHANGELOG.md b/CHANGELOG.md index 57a40a1..c97609e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,30 @@ # Changelog +## Unreleased + +### Security + +- Rejected API keys containing control characters. Gemini stores keys as `GEMINI_API_KEY=` in a `.env` file the CLI sources, so a key containing a newline injected additional environment variables (for example `GOOGLE_CLOUD_PROJECT`) into that file. The same rule now applies to Claude and Codex. +- Hardened profile file handling against path traversal, and switched the symlink guard to `symlink_metadata` so a dangling symlink can no longer redirect a credential write to its target. +- `uninstall --remove-data` now purges the system-keyring secrets it created instead of stranding them, and refuses to run when `AISW_HOME` is the user's home directory. +- Unit tests no longer resolve tool binaries from the developer's real `PATH`. Detection spawns whatever it finds to read `--version`, and running the real `claude`/`codex`/`gemini` CLIs from the test suite could rotate and invalidate live OAuth tokens. + +### Fixed + +- **Behavior change:** `aisw use --all` now exits non-zero when a tool switch fails. It previously exited `0` and reported success, contradicting the documented contract that a zero exit means success. On partial failure the `--json` output is now the standard failure envelope (`{"ok": false, "error": {...}}`) instead of `{"ok": true, ..., "warnings": [...]}`. +- `aisw use --all` now honors `--emit-env` and `--state-mode`. `--emit-env` previously fell through to the human-readable summary, which a shell hook would then `eval`. +- `aisw remove` no longer deletes credentials and profile files before checking whether a context still references the profile — the removal was rejected afterwards, leaving the data already destroyed. +- Removing the active profile now clears `active` in the same locked config mutation, so config can no longer name a profile that does not exist. +- `aisw status` no longer panics (`no entry found for key`) when `active` names a profile missing from the config; it reports the inconsistency instead. +- `aisw init --json` no longer aborts on a shell it has no hook for (for example `/bin/sh`, the default in many containers). +- `aisw doctor` no longer reports a false `credentials file missing` failure for every Gemini profile. It now checks the files a profile actually stores rather than one hardcoded name per tool, which also stopped `aisw verify` from inheriting the failure. +- Antigravity is now guarded by the generated shell hooks and included in `workspace status --json` and `status --context --json`. +- OAuth capture no longer leaves an orphaned interactive login process when a step inside the polling loop fails. + +### Performance + +- `aisw workspace check`, which the shell hook runs on every directory change, no longer probes tool binaries, credential files, and the OS keyring. It reads the active profiles from config instead. + ## 0.3.6 - 2026-06-11 ### Fixed diff --git a/src/auth/child_guard.rs b/src/auth/child_guard.rs new file mode 100644 index 0000000..240d551 --- /dev/null +++ b/src/auth/child_guard.rs @@ -0,0 +1,107 @@ +//! RAII wrapper that guarantees a spawned child process is reaped. +//! +//! The OAuth capture loops poll a live `claude auth login` / `codex login` +//! child while watching for credentials to appear. Those loops contain +//! fallible steps (reading a credential file, querying the keychain, polling +//! the child), and an early `?` return there used to leave the interactive +//! child running with the terminal still attached to it. + +use std::process::Child; + +pub(crate) struct ChildGuard { + child: Option, +} + +impl ChildGuard { + pub(crate) fn new(child: Child) -> Self { + Self { child: Some(child) } + } + + pub(crate) fn as_mut(&mut self) -> &mut Child { + self.child + .as_mut() + .expect("child is only taken in Drop, which consumes the guard") + } + + /// Kill and reap the child now. Idempotent, and safe to call on a child + /// that already exited. + pub(crate) fn terminate(&mut self) { + if let Some(mut child) = self.child.take() { + let _ = child.kill(); + let _ = child.wait(); + } + } +} + +impl Drop for ChildGuard { + fn drop(&mut self) { + self.terminate(); + } +} + +#[cfg(all(test, unix))] +mod tests { + use super::*; + use std::process::{Command, Stdio}; + + fn spawn_sleeper() -> Child { + Command::new("/bin/sh") + .args(["-c", "sleep 30"]) + .stdin(Stdio::null()) + .stdout(Stdio::null()) + .stderr(Stdio::null()) + .spawn() + .expect("sleeper should spawn") + } + + fn is_running(pid: u32) -> bool { + // Signal 0 probes for existence without delivering a signal. A reaped + // child is gone entirely; a zombie would still report as present, so + // this also proves the guard waits rather than only killing. + unsafe { libc::kill(pid as i32, 0) == 0 } + } + + #[test] + fn dropping_the_guard_kills_and_reaps_the_child() { + let _g = crate::SPAWN_LOCK.lock().unwrap_or_else(|p| p.into_inner()); + let child = spawn_sleeper(); + let pid = child.id(); + + drop(ChildGuard::new(child)); + + assert!( + !is_running(pid), + "guard must kill and reap the child on drop" + ); + } + + /// This is the case that regressed: an error inside the polling loop. + #[test] + fn an_early_error_return_does_not_orphan_the_child() { + let _g = crate::SPAWN_LOCK.lock().unwrap_or_else(|p| p.into_inner()); + let child = spawn_sleeper(); + let pid = child.id(); + + let result: anyhow::Result<()> = (|| { + let _guard = ChildGuard::new(child); + anyhow::bail!("simulated failure while polling for credentials") + })(); + + assert!(result.is_err()); + assert!(!is_running(pid), "child must not outlive a failed capture"); + } + + #[test] + fn terminate_is_idempotent() { + let _g = crate::SPAWN_LOCK.lock().unwrap_or_else(|p| p.into_inner()); + let child = spawn_sleeper(); + let pid = child.id(); + + let mut guard = ChildGuard::new(child); + guard.terminate(); + guard.terminate(); + drop(guard); + + assert!(!is_running(pid)); + } +} diff --git a/src/auth/claude/api_key.rs b/src/auth/claude/api_key.rs index 0aaeabf..c45c4a7 100644 --- a/src/auth/claude/api_key.rs +++ b/src/auth/claude/api_key.rs @@ -102,13 +102,11 @@ pub fn add_api_key_with_backend( /// Validates that the given API key is non-empty. pub fn validate_api_key(key: &str) -> Result<()> { - if key.trim().is_empty() { - bail!( - "Claude API key must not be empty.\n \ - Get your API key at console.anthropic.com → API Keys.", - ); - } - Ok(()) + crate::auth::validate_api_key_charset( + key, + "Claude", + "Get your API key at console.anthropic.com → API Keys.", + ) } /// Reads the stored API key from a profile's credentials file. diff --git a/src/auth/claude/oauth.rs b/src/auth/claude/oauth.rs index 9e3e729..9514ebc 100644 --- a/src/auth/claude/oauth.rs +++ b/src/auth/claude/oauth.rs @@ -460,9 +460,13 @@ If you need a different Claude account, fully sign out of claude.com first, then if let Some(config_dir) = target_config_dir { cmd.env("CLAUDE_CONFIG_DIR", config_dir); } - let mut child = cmd - .spawn() - .with_context(|| format!("could not spawn {}", claude_bin.display()))?; + // Guarded so that a failure anywhere in the poll loop below (keychain read, + // credential read, child poll) still reaps the interactive login child + // instead of orphaning it with the terminal attached. + let mut child = crate::auth::child_guard::ChildGuard::new( + cmd.spawn() + .with_context(|| format!("could not spawn {}", claude_bin.display()))?, + ); let deadline = Instant::now() + timeout; @@ -475,8 +479,7 @@ If you need a different Claude account, fully sign out of claude.com first, then if let Some(current) = current { let changed = keychain_before.as_deref() != Some(current.as_slice()); if changed { - let _ = child.kill(); - let _ = child.wait(); + child.terminate(); return Ok(current); } } @@ -487,8 +490,7 @@ If you need a different Claude account, fully sign out of claude.com first, then .with_context(|| format!("could not read {}", credential_path.display()))?; let changed = file_before.as_deref() != Some(current.as_slice()); if changed { - let _ = child.kill(); - let _ = child.wait(); + child.terminate(); return Ok(current); } } @@ -499,14 +501,14 @@ If you need a different Claude account, fully sign out of claude.com first, then .with_context(|| format!("could not read {}", fallback_path.display()))?; let changed = fallback_before.as_deref() != Some(current.as_slice()); if changed { - let _ = child.kill(); - let _ = child.wait(); + child.terminate(); return Ok(current); } } } if let Some(status) = child + .as_mut() .try_wait() .with_context(|| format!("could not poll {}", claude_bin.display()))? { @@ -547,8 +549,7 @@ If you need a different Claude account, fully sign out of claude.com first, then } if Instant::now() >= deadline { - let _ = child.kill(); - let _ = child.wait(); + child.terminate(); bail!( "Claude login timed out after {}s. \ The browser window may still be open.", diff --git a/src/auth/codex.rs b/src/auth/codex.rs index cfa7137..18cc34b 100644 --- a/src/auth/codex.rs +++ b/src/auth/codex.rs @@ -276,13 +276,11 @@ pub fn add_api_key_with_backend( } pub fn validate_api_key(key: &str) -> Result<()> { - if key.trim().is_empty() { - bail!( - "Codex API key must not be empty.\n \ - Get your API key at platform.openai.com → API Keys." - ); - } - Ok(()) + crate::auth::validate_api_key_charset( + key, + "Codex", + "Get your API key at platform.openai.com → API Keys.", + ) } /// Start the Codex OAuth flow using the installed `codex` binary. @@ -551,23 +549,26 @@ fn run_oauth_flow( ) -> Result { let _spinner = crate::output::start_spinner("Waiting for Codex login to complete..."); - let mut child = Command::new(codex_bin) + let child = Command::new(codex_bin) .arg("login") .env("CODEX_HOME", capture_dir) .spawn() .with_context(|| format!("could not spawn {}", codex_bin.display()))?; + // Guarded so a failure in the poll loop below cannot orphan the + // interactive login child. + let mut child = crate::auth::child_guard::ChildGuard::new(child); let auth_path = capture_dir.join(AUTH_FILE); let deadline = Instant::now() + timeout; loop { if auth_path.exists() { - let _ = child.kill(); - let _ = child.wait(); + child.terminate(); return Ok(auth_path); } if let Some(status) = child + .as_mut() .try_wait() .with_context(|| format!("could not poll {}", codex_bin.display()))? { @@ -585,8 +586,7 @@ fn run_oauth_flow( } if Instant::now() >= deadline { - let _ = child.kill(); - let _ = child.wait(); + child.terminate(); bail!( "Codex login timed out after {}s. \ If auth.json was not written, verify that config.toml has \ diff --git a/src/auth/gemini.rs b/src/auth/gemini.rs index a70283d..f8e6b77 100644 --- a/src/auth/gemini.rs +++ b/src/auth/gemini.rs @@ -211,13 +211,11 @@ pub fn add_api_key_with_backend( } pub fn validate_api_key(key: &str) -> Result<()> { - if key.trim().is_empty() { - bail!( - "Gemini API key must not be empty.\n \ - Get your API key at aistudio.google.com → Get API Key." - ); - } - Ok(()) + crate::auth::validate_api_key_charset( + key, + "Gemini", + "Get your API key at aistudio.google.com → Get API Key.", + ) } /// Read the stored API key from a profile's .env file. diff --git a/src/auth/mod.rs b/src/auth/mod.rs index c24bf3e..079eb99 100644 --- a/src/auth/mod.rs +++ b/src/auth/mod.rs @@ -1,4 +1,5 @@ pub mod antigravity; +pub(crate) mod child_guard; pub mod claude; pub mod codex; pub(crate) mod files; @@ -10,3 +11,74 @@ pub(crate) mod secure_store; pub(crate) mod system_keyring; pub(crate) mod test_overrides; pub mod token_expiry; + +use anyhow::{bail, Result}; + +/// Reject an API key that is empty or contains control characters. +/// +/// Control characters are refused because stored credentials are later +/// materialized into formats where they change meaning rather than being +/// escaped. Gemini writes `GEMINI_API_KEY=` into a `.env` file the CLI +/// sources, so a key containing a newline injects arbitrary additional +/// environment variables (for example `GOOGLE_CLOUD_PROJECT`). A real key from +/// any of these providers is a single line of printable ASCII, so nothing +/// legitimate is rejected here. +pub(crate) fn validate_api_key_charset(key: &str, tool_label: &str, help: &str) -> Result<()> { + if key.trim().is_empty() { + bail!("{tool_label} API key must not be empty.\n {help}"); + } + if let Some(bad) = key.chars().find(|ch| ch.is_control()) { + bail!( + "{tool_label} API key contains an invalid control character ({}).\n \ + Keys must be a single line — check for a stray newline from copy/paste \ + or from piping a file into --api-key.", + match bad { + '\n' => "newline".to_owned(), + '\r' => "carriage return".to_owned(), + '\t' => "tab".to_owned(), + other => format!("U+{:04X}", other as u32), + } + ); + } + Ok(()) +} + +#[cfg(test)] +mod tests { + use super::validate_api_key_charset; + + #[test] + fn accepts_a_normal_key() { + assert!(validate_api_key_charset("sk-ant-api03-AAAA", "Claude", "help").is_ok()); + } + + #[test] + fn rejects_empty_and_whitespace_keys() { + assert!(validate_api_key_charset("", "Claude", "help").is_err()); + assert!(validate_api_key_charset(" ", "Claude", "help").is_err()); + } + + /// A newline in the key would inject extra `KEY=value` lines into Gemini's + /// generated `.env` file. + #[test] + fn rejects_control_characters() { + for key in [ + "AIzaValid\nGOOGLE_CLOUD_PROJECT=attacker", + "AIzaValid\rmore", + "AIzaValid\tmore", + "AIzaValid\u{0}more", + ] { + let err = validate_api_key_charset(key, "Gemini", "help").unwrap_err(); + assert!( + err.to_string().contains("control character"), + "expected rejection for {key:?}, got: {err}" + ); + } + } + + #[test] + fn error_names_the_offending_character() { + let err = validate_api_key_charset("abc\ndef", "Gemini", "help").unwrap_err(); + assert!(err.to_string().contains("newline"), "got: {err}"); + } +} diff --git a/src/backup.rs b/src/backup.rs index 6a23535..fada79d 100644 --- a/src/backup.rs +++ b/src/backup.rs @@ -157,6 +157,12 @@ impl BackupManager { Some(n) => n.to_owned(), None => continue, }; + // Directory names under backups/ are only as trustworthy as the + // filesystem. Never let one steer a restore outside the + // profiles tree. + if crate::profile::validate_profile_name(&profile_name).is_err() { + continue; + } let dest_dir = profile_store.profile_dir(tool, &profile_name); fs::create_dir_all(&dest_dir).with_context(|| { diff --git a/src/commands/doctor.rs b/src/commands/doctor.rs index 482aad9..7a1d047 100644 --- a/src/commands/doctor.rs +++ b/src/commands/doctor.rs @@ -195,45 +195,32 @@ pub fn check_profile_permissions( continue; } - let cred_file = profile_store - .profile_dir(tool, name) - .join(credentials_filename(tool)); + let profile_dir = profile_store.profile_dir(tool, name); + + // Check every stored file rather than one hardcoded name per tool: the + // filename depends on the auth method (Gemini stores `.env` for API + // keys and `oauth_creds.json` for OAuth), so a fixed name reports a + // false "credentials file missing" failure for valid profiles. + let stored_files = match crate::auth::files::list_regular_files_recursive(&profile_dir) { + Ok(files) => files, + Err(e) => { + results.push(CheckResult::fail( + &check_name, + format!("could not read {}: {e}", profile_dir.display()), + )); + continue; + } + }; - if !cred_file.exists() { + if stored_files.is_empty() { results.push(CheckResult::fail( &check_name, - format!("credentials file missing: {}", cred_file.display()), + format!("no credential files stored in {}", profile_dir.display()), )); continue; } - match std::fs::metadata(&cred_file) { - Err(e) => results.push(CheckResult::fail( - &check_name, - format!("could not stat {}: {e}", cred_file.display()), - )), - Ok(m) => match file_mode_0600_check(&m) { - Some(0o600) => { - results.push(CheckResult::pass(&check_name, "0600 ok".to_owned())); - } - Some(mode) => { - results.push(CheckResult::fail( - &check_name, - format!( - "{} has permissions {:04o}, expected 0600", - cred_file.display(), - mode - ), - )); - } - None => { - results.push(CheckResult::warn( - &check_name, - "permission mode check not supported on this platform".to_owned(), - )); - } - }, - } + results.push(profile_permission_check(&check_name, &stored_files)); let _ = home; } @@ -241,6 +228,43 @@ pub fn check_profile_permissions( results } +/// Summarize the permission state of a profile's stored files into one check. +fn profile_permission_check( + check_name: &str, + stored_files: &[crate::auth::files::RegularFile], +) -> CheckResult { + let mut checked = 0usize; + let mut broad: Vec = Vec::new(); + + for file in stored_files { + let Ok(metadata) = std::fs::metadata(&file.path) else { + return CheckResult::fail( + check_name, + format!("could not stat {}", file.path.display()), + ); + }; + match file_mode_0600_check(&metadata) { + Some(0o600) => checked += 1, + Some(mode) => broad.push(format!("{} is {:04o}", file.path.display(), mode)), + None => { + return CheckResult::warn( + check_name, + "permission mode check not supported on this platform".to_owned(), + ) + } + } + } + + if broad.is_empty() { + CheckResult::pass(check_name, format!("0600 ok ({checked} file(s))")) + } else { + CheckResult::fail( + check_name, + format!("expected 0600, found: {}", broad.join(", ")), + ) + } +} + #[cfg(unix)] fn file_mode_0600_check(metadata: &std::fs::Metadata) -> Option { Some(metadata.permissions().mode() & 0o777) @@ -251,15 +275,6 @@ fn file_mode_0600_check(_metadata: &std::fs::Metadata) -> Option { None } -fn credentials_filename(tool: Tool) -> &'static str { - match tool { - Tool::Claude => ".credentials.json", - Tool::Codex => "auth.json", - Tool::Gemini => "oauth_credentials.json", - Tool::Antigravity => "keyring-secret.json", - } -} - // ---- rc file path helper ---- pub fn rc_path_for_shell(shell_exe: &str, user_home: &Path) -> Option { @@ -268,7 +283,7 @@ pub fn rc_path_for_shell(shell_exe: &str, user_home: &Path) -> Option { "bash" => Some(user_home.join(".bashrc")), "zsh" => Some(user_home.join(".zshrc")), "fish" => Some(user_home.join(".config").join("fish").join("config.fish")), - "pwsh" => Some(crate::commands::init::rc_file(user_home, "pwsh")), + "pwsh" => crate::commands::init::rc_file(user_home, "pwsh"), _ => None, } } diff --git a/src/commands/init.rs b/src/commands/init.rs index 5d54290..f304a29 100644 --- a/src/commands/init.rs +++ b/src/commands/init.rs @@ -149,7 +149,8 @@ pub(crate) fn run_machine( shell: InitShellStatus { rc_file: shell_name .as_deref() - .map(|shell| rc_file(user_home, shell).display().to_string()), + .and_then(|shell| rc_file(user_home, shell)) + .map(|rc| rc.display().to_string()), detected: shell_name, action: "skipped", }, @@ -244,8 +245,14 @@ pub(crate) fn normalized_shell_name(shell_env: Option<&str>) -> Option { } } -pub(crate) fn rc_file(user_home: &Path, shell: &str) -> PathBuf { - match shell { +/// Path of the rc file aisw would install the hook into for `shell`. +/// +/// Returns `None` for any shell aisw has no hook for. `normalized_shell_name` +/// passes through unrecognized shell names (`sh`, `dash`, `nu`, `ksh`, ...), so +/// this must stay total — an unsupported `$SHELL` is an ordinary configuration, +/// not a bug. +pub(crate) fn rc_file(user_home: &Path, shell: &str) -> Option { + let path = match shell { "bash" => { if cfg!(target_os = "macos") { user_home.join(".bash_profile") @@ -268,12 +275,18 @@ pub(crate) fn rc_file(user_home: &Path, shell: &str) -> PathBuf { .join("Microsoft.PowerShell_profile.ps1") } } - _ => unreachable!(), - } + _ => return None, + }; + Some(path) } fn install_shell_hook(user_home: &Path, shell: &str, confirmed: bool) -> Result<()> { - let rc = rc_file(user_home, shell); + let Some(rc) = rc_file(user_home, shell) else { + output::print_warning(format!( + "Shell '{shell}' has no aisw hook. Install one manually with 'aisw shell-hook '." + )); + return Ok(()); + }; if rc.exists() { let contents = @@ -308,7 +321,8 @@ fn install_shell_hook(user_home: &Path, shell: &str, confirmed: bool) -> Result< "\n{}\naisw shell-hook pwsh | Out-String | Invoke-Expression\n", HOOK_MARKER ), - _ => unreachable!(), + // Unreachable: `rc_file` above already returned `None` for these. + other => anyhow::bail!("no aisw shell hook is defined for '{other}'"), }; let mut file = fs::OpenOptions::new() diff --git a/src/commands/list.rs b/src/commands/list.rs index 53332f8..ed4064d 100644 --- a/src/commands/list.rs +++ b/src/commands/list.rs @@ -11,7 +11,7 @@ use crate::profile::ProfileStore; use crate::types::Tool; pub(crate) struct Row { - pub(crate) tool: &'static str, + pub(crate) tool: Tool, pub(crate) profile: String, pub(crate) active: bool, pub(crate) auth_method: &'static str, @@ -57,7 +57,7 @@ pub(crate) fn collect_rows(args: &ListArgs, home: &Path) -> Result> { for name in names { let meta = &profiles[name]; rows.push(Row { - tool: tool.binary_name(), + tool, profile: name.to_owned(), active: active == Some(name), auth_method: auth_display(meta.auth_method), @@ -112,20 +112,29 @@ pub(crate) fn collect_rows(args: &ListArgs, home: &Path) -> Result> { .unwrap_or_default() .to_ascii_lowercase() .contains(&needle) - || row.tool.to_ascii_lowercase().contains(&needle) + || row + .tool + .binary_name() + .to_ascii_lowercase() + .contains(&needle) }); } } match args.sort { Some(crate::cli::SortBy::Name) => { - rows.sort_by(|a, b| a.tool.cmp(b.tool).then_with(|| a.profile.cmp(&b.profile))); + rows.sort_by(|a, b| { + a.tool + .binary_name() + .cmp(b.tool.binary_name()) + .then_with(|| a.profile.cmp(&b.profile)) + }); } Some(crate::cli::SortBy::Recent) => { rows.sort_by(|a, b| { b.added_at .cmp(&a.added_at) - .then_with(|| a.tool.cmp(b.tool)) + .then_with(|| a.tool.binary_name().cmp(b.tool.binary_name())) .then_with(|| a.profile.cmp(&b.profile)) }); } @@ -160,21 +169,13 @@ fn print_table(rows: &[Row]) { output::print_title("Profiles"); - let mut current_tool: Option<&str> = None; + let mut current_tool: Option = None; for row in rows { if current_tool != Some(row.tool) { if current_tool.is_some() { output::print_blank_line(); } - - let tool = match row.tool { - "claude" => Tool::Claude, - "codex" => Tool::Codex, - "gemini" => Tool::Gemini, - "agy" => Tool::Antigravity, - _ => unreachable!(), - }; - output::print_tool_section(tool); + output::print_tool_section(row.tool); current_tool = Some(row.tool); } @@ -239,7 +240,7 @@ fn print_json(rows: &[Row]) -> Result<()> { for tool in Tool::ALL { let tool_name = tool.binary_name(); - let tool_rows: Vec<&Row> = rows.iter().filter(|r| r.tool == tool_name).collect(); + let tool_rows: Vec<&Row> = rows.iter().filter(|r| r.tool == tool).collect(); let active = tool_rows .iter() .find(|r| r.active) @@ -315,7 +316,7 @@ mod tests { let rows = collect_rows(&list_args(None, false), tmp.path()).unwrap(); assert_eq!(rows.len(), 1); - assert_eq!(rows[0].tool, "claude"); + assert_eq!(rows[0].tool, Tool::Claude); assert_eq!(rows[0].profile, "work"); assert_eq!(rows[0].auth_method, "api_key"); assert_eq!(rows[0].credential_backend, "file"); @@ -344,7 +345,7 @@ mod tests { let rows = collect_rows(&list_args(Some(Tool::Claude), false), tmp.path()).unwrap(); assert_eq!(rows.len(), 1); - assert_eq!(rows[0].tool, "claude"); + assert_eq!(rows[0].tool, Tool::Claude); } #[test] @@ -413,6 +414,6 @@ mod tests { args.active_only = true; let rows = collect_rows(&args, tmp.path()).unwrap(); assert_eq!(rows.len(), 1); - assert_eq!(rows[0].tool, "claude"); + assert_eq!(rows[0].tool, Tool::Claude); } } diff --git a/src/commands/remove.rs b/src/commands/remove.rs index 45786ae..d276b8b 100644 --- a/src/commands/remove.rs +++ b/src/commands/remove.rs @@ -94,6 +94,11 @@ pub(crate) fn run_inner(args: RemoveArgs, home: &Path, confirmed: bool) -> Resul bail!("operation cancelled by user."); } + // A profile referenced by a context cannot be removed. Check that *before* + // any destructive step, otherwise the credentials and profile directory are + // already gone by the time the config write rejects the removal. + ensure_not_referenced_by_context(&config, args.tool, profile_name)?; + // Final backup before deleting. let profile_dir = profile_store.profile_dir(args.tool, profile_name); let profile_meta = config @@ -121,12 +126,10 @@ pub(crate) fn run_inner(args: RemoveArgs, home: &Path, confirmed: bool) -> Resul auth::secure_store::delete_profile_secret(args.tool, profile_name)?; } profile_store.delete(args.tool, profile_name)?; + // `remove_profile` also clears `active` in the same locked mutation, so + // there is no window where `active` names a profile that no longer exists. config_store.remove_profile(args.tool, profile_name)?; - if is_active { - config_store.clear_active(args.tool)?; - } - if args.json { machine::print_success( "remove", @@ -190,7 +193,23 @@ fn precheck(args: &RemoveArgs, home: &Path) -> Result<()> { profile_name ); } - Ok(()) + ensure_not_referenced_by_context(&config, args.tool, profile_name) +} + +fn ensure_not_referenced_by_context(config: &Config, tool: Tool, profile_name: &str) -> Result<()> { + let refs = config.contexts_referencing_profile(tool, profile_name); + if refs.is_empty() { + return Ok(()); + } + bail!( + "cannot remove {} profile '{}' because it is referenced by contexts: {}.\n \ + Update or remove those contexts first, for example: aisw context unset {} --{}", + tool, + profile_name, + refs.join(", "), + refs[0], + tool.context_flag(), + ) } fn resolve_profile_name(args: &RemoveArgs, home: &Path) -> Result { diff --git a/src/commands/shell_hook.rs b/src/commands/shell_hook.rs index 8555c29..ee2bfd8 100644 --- a/src/commands/shell_hook.rs +++ b/src/commands/shell_hook.rs @@ -50,6 +50,10 @@ gemini() { command aisw workspace check --tool gemini || return $? command gemini \"$@\" } +agy() { + command aisw workspace check --tool antigravity || return $? + command agy \"$@\" +} __aisw_install_prompt_hook "; @@ -95,6 +99,12 @@ function gemini or return $status command gemini $argv end + +function agy + command aisw workspace check --tool antigravity + or return $status + command agy $argv +end "; const POWERSHELL_HOOK: &str = r#" @@ -163,6 +173,12 @@ function global:gemini { if ($LASTEXITCODE -ne 0) { return } & (__aisw_get_command_path 'gemini') @ArgsRest } +function global:agy { + param([Parameter(ValueFromRemainingArguments = $true)][string[]]$ArgsRest) + & (__aisw_bin) workspace check --tool antigravity + if ($LASTEXITCODE -ne 0) { return } + & (__aisw_get_command_path 'agy') @ArgsRest +} "#; pub fn run(args: ShellHookArgs) -> Result<()> { @@ -209,6 +225,31 @@ mod tests { assert!(BASH_ZSH_HOOK.contains("--emit-env")); } + /// Every tool aisw manages must be wrapped, or its workspace guard is + /// silently unenforceable for that tool. + #[test] + fn every_tool_binary_is_guarded_in_all_hooks() { + for tool in crate::types::Tool::ALL { + let check = format!("workspace check --tool {}", tool.context_flag()); + for (shell, hook) in [ + ("bash/zsh", BASH_ZSH_HOOK), + ("fish", FISH_HOOK), + ("pwsh", POWERSHELL_HOOK), + ] { + assert!( + hook.contains(&check), + "{shell} hook is missing a guard for {}", + tool.binary_name() + ); + assert!( + hook.contains(tool.binary_name()), + "{shell} hook does not wrap the {} binary", + tool.binary_name() + ); + } + } + } + #[test] fn fish_hook_returns_ok() { assert!(run(hook_args(Shell::Fish)).is_ok()); diff --git a/src/commands/status.rs b/src/commands/status.rs index 4bdab1c..8fd5ddc 100644 --- a/src/commands/status.rs +++ b/src/commands/status.rs @@ -22,6 +22,8 @@ pub(crate) struct ToolStatus { pub binary_found: bool, pub stored_profiles: usize, pub active_profile: Option, + /// False when `active` points at a profile with no config entry. + pub active_profile_registered: bool, pub auth_method: Option, pub credential_backend: Option, pub claude_auth_classification: Option, @@ -161,139 +163,167 @@ pub(crate) fn collect_status( None }; - let ( - active_profile, - auth_method, - credential_backend, - claude_auth_classification, - codex_auth_classification, - antigravity_auth_classification, - active_profile_added_at, - active_profile_applied, - credentials_present, - permissions_ok, - ) = if let Some(name) = active_name { - let profiles = config.profiles_for(tool); - let profile_meta = &profiles[name]; - let profile_dir = profile_store.profile_dir(tool, name); - let (creds, perms) = - check_profile_storage(&profile_dir, tool, name, profile_meta.credential_backend); - - let auth = profiles - .get(name) - .map(|m| auth_label(m.auth_method).to_owned()); - let backend = profiles - .get(name) - .map(|m| m.credential_backend.display_name().to_owned()); - let claude_auth_classification = if tool == Tool::Claude { - Some( - auth::claude::classify_profile( - user_home, - &profile_store, - name, - profile_meta.auth_method, - profile_meta.credential_backend, - )? - .as_str() - .to_owned(), - ) - } else { - None - }; - let codex_auth_classification = if tool == Tool::Codex { - Some( - auth::codex::classify_profile( - &profile_store, - name, - profile_meta.auth_method, - profile_meta.credential_backend, - )? - .as_str() - .to_owned(), - ) - } else { - None - }; - let antigravity_auth_classification = if tool == Tool::Antigravity { - Some( - auth::antigravity::classify_profile( - &profile_store, - name, - profile_meta.auth_method, - profile_meta.credential_backend, - )? - .as_str() - .to_owned(), - ) - } else { - None - }; - let added_at = profiles.get(name).map(|m| m.added_at); - let applied = if creds { - profiles - .get(name) - .map(|m| { - if should_skip_live_verification(tool, m.credential_backend) { - Ok(None) - } else { - assess_live_state( - tool, - m.auth_method, - m.credential_backend, - config.state_mode_for(tool), - &profile_store, - name, - user_home, - ) - .map(|state| { - Some(match state { - LiveActivation::Applied => true, - LiveActivation::NotApplied => false, - }) - }) - } - }) - .transpose()? - .flatten() - } else { - Some(false) - }; - ( - Some(name.to_owned()), - auth, - backend, - claude_auth_classification, - codex_auth_classification, - antigravity_auth_classification, - added_at, - applied, - creds, - perms, - ) - } else { - (None, None, None, None, None, None, None, None, false, true) + let active = match active_name { + Some(name) => { + collect_active_profile_status(&config, &profile_store, user_home, tool, name)? + } + None => ActiveProfileStatus::default(), }; statuses.push(ToolStatus { tool, binary_found, stored_profiles, - active_profile, - auth_method, - credential_backend, - claude_auth_classification, - codex_auth_classification, - antigravity_auth_classification, + active_profile: active.profile, + active_profile_registered: active.registered, + auth_method: active.auth_method, + credential_backend: active.credential_backend, + claude_auth_classification: active.claude_auth_classification, + codex_auth_classification: active.codex_auth_classification, + antigravity_auth_classification: active.antigravity_auth_classification, state_mode, - active_profile_added_at, - active_profile_applied, - credentials_present, - permissions_ok, + active_profile_added_at: active.added_at, + active_profile_applied: active.applied, + credentials_present: active.credentials_present, + permissions_ok: active.permissions_ok, }); } Ok(statuses) } +/// The part of a tool's status that only exists when a profile is active. +struct ActiveProfileStatus { + profile: Option, + /// False when `active` names a profile with no entry in `profiles`. Config + /// can reach that state through hand-editing or an interrupted `remove`; + /// reporting it beats indexing into the map and panicking. + registered: bool, + auth_method: Option, + credential_backend: Option, + claude_auth_classification: Option, + codex_auth_classification: Option, + antigravity_auth_classification: Option, + added_at: Option>, + applied: Option, + credentials_present: bool, + permissions_ok: bool, +} + +impl Default for ActiveProfileStatus { + fn default() -> Self { + Self { + profile: None, + registered: true, + auth_method: None, + credential_backend: None, + claude_auth_classification: None, + codex_auth_classification: None, + antigravity_auth_classification: None, + added_at: None, + applied: None, + credentials_present: false, + permissions_ok: true, + } + } +} + +fn collect_active_profile_status( + config: &Config, + profile_store: &ProfileStore, + user_home: &Path, + tool: Tool, + name: &str, +) -> Result { + let Some(meta) = config.profiles_for(tool).get(name) else { + return Ok(ActiveProfileStatus { + profile: Some(name.to_owned()), + registered: false, + ..ActiveProfileStatus::default() + }); + }; + + let profile_dir = profile_store.profile_dir(tool, name); + let (credentials_present, permissions_ok) = + check_profile_storage(&profile_dir, tool, name, meta.credential_backend); + + // Only the owning tool's classifier runs; the others stay `None`. + let mut claude_auth_classification = None; + let mut codex_auth_classification = None; + let mut antigravity_auth_classification = None; + match tool { + Tool::Claude => { + claude_auth_classification = Some( + auth::claude::classify_profile( + user_home, + profile_store, + name, + meta.auth_method, + meta.credential_backend, + )? + .as_str() + .to_owned(), + ); + } + Tool::Codex => { + codex_auth_classification = Some( + auth::codex::classify_profile( + profile_store, + name, + meta.auth_method, + meta.credential_backend, + )? + .as_str() + .to_owned(), + ); + } + Tool::Antigravity => { + antigravity_auth_classification = Some( + auth::antigravity::classify_profile( + profile_store, + name, + meta.auth_method, + meta.credential_backend, + )? + .as_str() + .to_owned(), + ); + } + Tool::Gemini => {} + } + + let applied = if !credentials_present { + Some(false) + } else if should_skip_live_verification(tool, meta.credential_backend) { + None + } else { + Some( + assess_live_state( + tool, + meta.auth_method, + meta.credential_backend, + config.state_mode_for(tool), + profile_store, + name, + user_home, + )? == LiveActivation::Applied, + ) + }; + + Ok(ActiveProfileStatus { + profile: Some(name.to_owned()), + registered: true, + auth_method: Some(auth_label(meta.auth_method).to_owned()), + credential_backend: Some(meta.credential_backend.display_name().to_owned()), + claude_auth_classification, + codex_auth_classification, + antigravity_auth_classification, + added_at: Some(meta.added_at), + applied, + credentials_present, + permissions_ok, + }) +} + fn apply_status_filters(statuses: &mut Vec, args: &StatusArgs) { if let Some(tool) = args.tool { statuses.retain(|s| s.tool == tool); @@ -406,6 +436,9 @@ fn status_message(s: &ToolStatus) -> &'static str { } return "no active profile"; } + if !s.active_profile_registered { + return "active profile is missing from aisw config \u{2014} run 'aisw repair --apply' or re-add it"; + } if !s.credentials_present { return match s.credential_backend.as_deref() { Some("system_keyring") => "secure credentials missing from the managed system keyring", @@ -545,13 +578,11 @@ fn print_json( "active": context_status.active, "matches": context_status.matches, "drift_candidates": context_status.drift_candidates, - "profiles": context_status.mapped_profiles.as_ref().map(|profiles| { - serde_json::json!({ - "claude": profiles.get(&Tool::Claude), - "codex": profiles.get(&Tool::Codex), - "gemini": profiles.get(&Tool::Gemini), - }) - }).unwrap_or(serde_json::Value::Null), + "profiles": context_status + .mapped_profiles + .as_ref() + .map(mapped_profiles_json) + .unwrap_or(serde_json::Value::Null), "unmanaged_tools": context_status.unmanaged_tools.iter().map(|(tool, active_profile)| { serde_json::json!({ "tool": tool.binary_name(), @@ -578,6 +609,39 @@ fn print_json( Ok(()) } +/// Context-to-profile mapping as JSON, keyed by tool. +/// +/// Iterates `Tool::ALL` so newly supported tools appear automatically rather +/// than being forgotten in a hand-written object literal. +fn mapped_profiles_json( + mapped_profiles: &std::collections::HashMap, +) -> serde_json::Value { + let map = Tool::ALL + .iter() + .map(|tool| { + ( + tool.context_flag().to_owned(), + serde_json::json!(mapped_profiles.get(tool)), + ) + }) + .collect::>(); + serde_json::Value::Object(map) +} + +/// Map of each tool to its active profile name, read straight from config. +/// +/// Context classification only needs profile *names*, so callers that do not +/// otherwise need a full `collect_status` (binary detection, credential reads, +/// keyring access, live-match comparison) should use this instead. +pub(crate) fn active_profiles_from_config( + config: &Config, +) -> std::collections::HashMap> { + Tool::ALL + .iter() + .map(|tool| (*tool, config.active_for(*tool).map(str::to_owned))) + .collect() +} + pub(crate) fn derive_context_status( config: &Config, statuses: &[ToolStatus], @@ -594,7 +658,13 @@ pub(crate) fn derive_context_status( ) }) .collect::>(); + derive_context_status_from_active(config, &active_profiles) +} +pub(crate) fn derive_context_status_from_active( + config: &Config, + active_profiles: &std::collections::HashMap>, +) -> DerivedContextStatus { let mut matches = Vec::new(); let mut drift_candidates = Vec::new(); for (name, context) in config.contexts() { @@ -1179,6 +1249,7 @@ mod tests { binary_found: true, stored_profiles: 1, active_profile: Some("work".to_owned()), + active_profile_registered: true, auth_method: Some("api_key".to_owned()), credential_backend: Some("file".to_owned()), claude_auth_classification: Some("api_key".to_owned()), @@ -1195,6 +1266,7 @@ mod tests { binary_found: true, stored_profiles: 1, active_profile: None, + active_profile_registered: true, auth_method: None, credential_backend: None, claude_auth_classification: None, @@ -1236,6 +1308,7 @@ mod tests { binary_found: true, stored_profiles: 1, active_profile: Some("old".to_owned()), + active_profile_registered: true, auth_method: Some("api_key".to_owned()), credential_backend: Some("file".to_owned()), claude_auth_classification: Some("api_key".to_owned()), @@ -1252,6 +1325,7 @@ mod tests { binary_found: true, stored_profiles: 1, active_profile: Some("new".to_owned()), + active_profile_registered: true, auth_method: Some("api_key".to_owned()), credential_backend: Some("file".to_owned()), claude_auth_classification: None, diff --git a/src/commands/uninstall.rs b/src/commands/uninstall.rs index b8bfe52..c0faa13 100644 --- a/src/commands/uninstall.rs +++ b/src/commands/uninstall.rs @@ -3,10 +3,14 @@ use std::path::{Path, PathBuf}; use anyhow::{bail, Context, Result}; +use crate::auth::secure_store; +use crate::backup::BackupManager; use crate::cli::UninstallArgs; use crate::commands::init::{rc_file, HOOK_MARKER}; +use crate::config::{ConfigStore, CredentialBackend}; use crate::output; use crate::runtime; +use crate::types::Tool; const SHELLS: [&str; 4] = ["bash", "zsh", "fish", "pwsh"]; @@ -50,7 +54,13 @@ pub(crate) fn run_inner(args: UninstallArgs, home: &Path, user_home: &Path) -> R removed_hooks.push(rc.display().to_string()); } + let mut purged_secrets = 0usize; let removed_data = if args.remove_data && home.exists() { + ensure_safe_to_delete(home, user_home)?; + // Deleting AISW_HOME alone would strand credentials in the OS keyring, + // which is the opposite of what "remove my data" means. Purge them + // first, while the config and backup index that name them still exist. + purged_secrets = purge_managed_secrets(home); fs::remove_dir_all(home).with_context(|| format!("could not remove {}", home.display()))?; true } else { @@ -71,6 +81,12 @@ pub(crate) fn run_inner(args: UninstallArgs, home: &Path, user_home: &Path) -> R output::print_effects_header(); if removed_data { output::print_effect(format!("Deleted {}.", home.display())); + if purged_secrets > 0 { + output::print_effect(format!( + "Removed {purged_secrets} aisw-managed system keyring entr{}.", + if purged_secrets == 1 { "y" } else { "ies" } + )); + } } else if plan.data_dir_exists { output::print_effect(format!("Kept {}.", home.display())); } else { @@ -101,7 +117,9 @@ struct Plan { fn build_plan(home: &Path, user_home: &Path) -> Result { let mut shell_hook_files = Vec::new(); for shell in SHELLS { - let rc = rc_file(user_home, shell); + let Some(rc) = rc_file(user_home, shell) else { + continue; + }; if rc.exists() && file_contains_hook(&rc)? { shell_hook_files.push(rc); } @@ -113,6 +131,71 @@ fn build_plan(home: &Path, user_home: &Path) -> Result { }) } +/// Refuse to recursively delete a path that is clearly not an aisw home. +/// +/// `AISW_HOME` is user-supplied, so a stray `AISW_HOME=$HOME` would otherwise +/// turn `uninstall --remove-data --yes` into `rm -rf ~`. +fn ensure_safe_to_delete(home: &Path, user_home: &Path) -> Result<()> { + if home.parent().is_none() { + bail!( + "refusing to delete {} — AISW_HOME must not be a filesystem root.", + home.display() + ); + } + if home == user_home { + bail!( + "refusing to delete {} — AISW_HOME must not be your home directory.\n \ + Point AISW_HOME at a dedicated directory such as ~/.aisw and retry.", + home.display() + ); + } + Ok(()) +} + +/// Delete every system-keyring secret aisw created under this home. +/// +/// Best effort: a keyring that is locked or unavailable must not block the +/// filesystem cleanup, so failures are counted as "not purged" rather than +/// aborting the uninstall. Returns the number of entries removed. +fn purge_managed_secrets(home: &Path) -> usize { + let mut purged = 0usize; + + let mut keyring_profiles = Vec::new(); + if let Ok(config) = ConfigStore::new(home).load() { + for tool in Tool::ALL { + for (name, meta) in config.profiles_for(tool) { + if meta.credential_backend == CredentialBackend::SystemKeyring { + keyring_profiles.push((tool, name.clone())); + } + } + } + } + + for (tool, name) in &keyring_profiles { + if secure_store::delete_profile_secret(*tool, name).is_ok() { + purged += 1; + } + } + + // Backups only carry a keyring secret when their profile was keyring-backed, + // so restrict the sweep to those profiles rather than probing every backup. + if let Ok(backups) = BackupManager::new(home).list() { + for entry in backups { + let keyring_backed = keyring_profiles + .iter() + .any(|(tool, name)| *tool == entry.tool && *name == entry.profile); + if keyring_backed + && secure_store::delete_backup_secret(entry.tool, &entry.profile, &entry.backup_id) + .is_ok() + { + purged += 1; + } + } + } + + purged +} + fn file_contains_hook(path: &Path) -> Result { let contents = fs::read_to_string(path).with_context(|| format!("could not read {}", path.display()))?; diff --git a/src/commands/use_.rs b/src/commands/use_.rs index 4392430..11eeb69 100644 --- a/src/commands/use_.rs +++ b/src/commands/use_.rs @@ -31,7 +31,14 @@ pub fn run(args: UseArgs, home: &Path) -> Result<()> { if profile_name.is_empty() { anyhow::bail!("--all requires --profile "); } - run_all_in(profile_name, args.json, home, &user_home) + run_all_in( + profile_name, + args.state_mode, + args.emit_env, + args.json, + home, + &user_home, + ) } else { let tool = args .tool @@ -50,6 +57,8 @@ pub fn run(args: UseArgs, home: &Path) -> Result<()> { pub(crate) fn run_all_in( profile_name: &str, + state_mode_override: Option, + emit_env: bool, json: bool, home: &Path, user_home: &Path, @@ -64,17 +73,22 @@ pub(crate) fn run_all_in( for tool in Tool::ALL { let profiles = config.profiles_for(tool); if !profiles.contains_key(profile_name) { - output::print_info(format!( - "(skipped {} — no profile named '{}')", - tool, profile_name - )); + if !emit_env { + output::print_info(format!( + "(skipped {} — no profile named '{}')", + tool, profile_name + )); + } continue; } + // `--state-mode` only applies to tools that support it; passing it to + // the others would make the whole `--all` switch fail. + let tool_state_mode = state_mode_override.filter(|_| tool.supports_state_mode()); match run_for_tool( tool, Some(profile_name), - None, - false, + tool_state_mode, + emit_env, false, home, user_home, @@ -90,6 +104,25 @@ pub(crate) fn run_all_in( if switched == 0 && errors.is_empty() { anyhow::bail!("no tool has a profile named '{}'", profile_name); } + + // A tool without a matching profile is a *skip* and stays successful, but a + // tool that was attempted and failed must not exit 0 — scripts rely on the + // exit code to know whether the switch actually happened. + if !errors.is_empty() { + anyhow::bail!( + "{} of {} attempted tool switches failed:\n {}", + errors.len(), + errors.len() + switched, + errors.join("\n ") + ); + } + + // In --emit-env mode stdout is a shell script the caller evals; anything + // else written there would be executed as shell input. + if emit_env { + return Ok(()); + } + if json { let after_backup_ids = backup_ids_for(home, None)?; machine::print_success( @@ -100,14 +133,11 @@ pub(crate) fn run_all_in( "state_mode": state_mode_map(home, &affected_tools)?, "live_match": live_match_map(home, user_home, &affected_tools)?, "backup_ids": diff_backup_ids(&before_backup_ids, &after_backup_ids), - "warnings": errors, + "warnings": Vec::::new(), }), )?; - } else { - for e in &errors { - output::print_warning(e); - } } + Ok(()) } @@ -1498,7 +1528,7 @@ mod tests { setup_codex_api_key_profile(&home, "work"); setup_gemini_api_key_profile(&home, "work"); - run_all_in("work", false, &home, &user_home).unwrap(); + run_all_in("work", None, false, false, &home, &user_home).unwrap(); let config = ConfigStore::new(&home).load().unwrap(); assert_eq!(config.active_for(Tool::Claude), Some("work")); @@ -1518,7 +1548,7 @@ mod tests { setup_claude_api_key_profile(&home, "work"); // Only Claude has "work" - run_all_in("work", false, &home, &user_home).unwrap(); + run_all_in("work", None, false, false, &home, &user_home).unwrap(); let config = ConfigStore::new(&home).load().unwrap(); assert_eq!(config.active_for(Tool::Claude), Some("work")); @@ -1533,7 +1563,7 @@ mod tests { let user_home = tmp.path().join("uhome"); std::fs::create_dir_all(&home).unwrap(); - let err = run_all_in("work", false, &home, &user_home).unwrap_err(); + let err = run_all_in("work", None, false, false, &home, &user_home).unwrap_err(); assert!( err.to_string().contains("no tool has a profile"), "unexpected: {}", @@ -1553,7 +1583,7 @@ mod tests { setup_claude_api_key_profile(&home, "work"); setup_codex_api_key_profile(&home, "work"); - run_all_in("work", true, &home, &user_home).unwrap(); + run_all_in("work", None, false, true, &home, &user_home).unwrap(); let config = ConfigStore::new(&home).load().unwrap(); assert_eq!(config.active_for(Tool::Claude), Some("work")); diff --git a/src/commands/verify.rs b/src/commands/verify.rs index a476f42..2d6a29e 100644 --- a/src/commands/verify.rs +++ b/src/commands/verify.rs @@ -18,7 +18,12 @@ enum VerifyStatus { #[derive(Debug, Clone, Serialize)] struct ToolVerification { + /// Binary name, which is the stable identifier in the JSON contract + /// (`agy` for Antigravity, not the enum's `antigravity`). tool: &'static str, + /// Typed counterpart used for presentation. Kept out of the JSON contract. + #[serde(skip)] + tool_kind: Tool, status: VerifyStatus, active_profile: Option, stored_profiles: usize, @@ -149,6 +154,7 @@ fn tool_verification(tool: &status::ToolStatus) -> ToolVerification { ToolVerification { tool: tool.tool.binary_name(), + tool_kind: tool.tool, status, active_profile: tool.active_profile.clone(), stored_profiles: tool.stored_profiles, @@ -210,11 +216,7 @@ fn print_text(report: &VerifyReport) { crate::output::print_blank_line(); for tool in &report.tools { - crate::output::print_tool_section(match tool.tool { - "claude" => Tool::Claude, - "codex" => Tool::Codex, - _ => Tool::Gemini, - }); + crate::output::print_tool_section(tool.tool_kind); crate::output::print_kv( "Status", match tool.status { @@ -244,6 +246,7 @@ mod tests { binary_found: true, stored_profiles: 1, active_profile: Some("work".to_owned()), + active_profile_registered: true, auth_method: Some("api_key".to_owned()), credential_backend: Some("file".to_owned()), claude_auth_classification: None, @@ -359,6 +362,7 @@ mod tests { &[ ToolVerification { tool: "claude", + tool_kind: Tool::Claude, status: VerifyStatus::Pass, active_profile: Some("work".to_owned()), stored_profiles: 1, @@ -367,6 +371,7 @@ mod tests { }, ToolVerification { tool: "codex", + tool_kind: Tool::Codex, status: VerifyStatus::Fail, active_profile: Some("work".to_owned()), stored_profiles: 1, diff --git a/src/commands/workspace.rs b/src/commands/workspace.rs index 53d1cbf..5f94d86 100644 --- a/src/commands/workspace.rs +++ b/src/commands/workspace.rs @@ -297,11 +297,7 @@ fn status(args: WorkspaceStatusArgs, home: &Path) -> Result<()> { "matched_rule": status.matched_rule, "expected_context": status.expected_context, "active_context": status.active_context, - "active_profiles": { - "claude": status.active_profiles.get(&Tool::Claude).cloned().flatten(), - "codex": status.active_profiles.get(&Tool::Codex).cloned().flatten(), - "gemini": status.active_profiles.get(&Tool::Gemini).cloned().flatten(), - }, + "active_profiles": active_profiles_json(&status.active_profiles), "status": status.status.as_str(), "recommended_command": status.recommended_command, }))? @@ -586,6 +582,25 @@ fn remove_git_remote_rule(config: &mut WorkspaceConfig, pattern: &str) -> Option Some(config.git_remote_rules.remove(index).context) } +/// Machine-readable active-profile map, keyed by tool. +/// +/// Built from `Tool::ALL` so a newly supported tool cannot be silently omitted +/// from the JSON contract the way Antigravity previously was. +fn active_profiles_json( + active_profiles: &std::collections::HashMap>, +) -> serde_json::Value { + let map = Tool::ALL + .iter() + .map(|tool| { + ( + tool.context_flag().to_owned(), + json!(active_profiles.get(tool).cloned().flatten()), + ) + }) + .collect::>(); + serde_json::Value::Object(map) +} + fn active_profiles_summary( active_profiles: &std::collections::HashMap>, ) -> String { diff --git a/src/config.rs b/src/config.rs index 733912b..464a425 100644 --- a/src/config.rs +++ b/src/config.rs @@ -170,6 +170,14 @@ impl Config { pub fn context(&self, name: &str) -> Option<&ContextEntry> { self.contexts.get(name) } + + /// Names of saved contexts that map `tool` to `profile_name`, sorted. + /// + /// A profile cannot be removed while any context still references it, so + /// callers must consult this *before* performing destructive work. + pub fn contexts_referencing_profile(&self, tool: Tool, profile_name: &str) -> Vec { + contexts_referencing_profile(self, tool, profile_name) + } } pub struct ConfigStore { @@ -238,6 +246,12 @@ impl ConfigStore { }) } + /// Remove a profile and, in the same locked mutation, clear it from + /// `active` if it was the active profile for `tool`. + /// + /// Clearing `active` here rather than in a follow-up `clear_active` call + /// keeps the two writes atomic: a crash between them would otherwise leave + /// `active` pointing at a profile that no longer exists. pub fn remove_profile(&self, tool: Tool, name: &str) -> Result { self.with_mutating_config(|config| { let context_refs = contexts_referencing_profile(config, tool, name); @@ -260,6 +274,10 @@ impl ConfigStore { .into()); } + if tool_active(config, tool).as_deref() == Some(name) { + *tool_active_mut(config, tool) = None; + } + Ok(()) }) } diff --git a/src/profile.rs b/src/profile.rs index 763e092..9ed7434 100644 --- a/src/profile.rs +++ b/src/profile.rs @@ -1,5 +1,5 @@ use std::fs; -use std::path::{Path, PathBuf}; +use std::path::{Component, Path, PathBuf}; use anyhow::{bail, Context, Result}; @@ -18,12 +18,23 @@ impl ProfileStore { } } + /// Path a profile's files live under. + /// + /// This is the display/lookup form and does not validate `name`. Every + /// operation that reads or writes through the returned path goes via + /// [`Self::validated_profile_dir`] so a name that escaped validation + /// elsewhere can never resolve outside the profiles tree. pub fn profile_dir(&self, tool: Tool, name: &str) -> PathBuf { self.home.join("profiles").join(tool.dir_name()).join(name) } + fn validated_profile_dir(&self, tool: Tool, name: &str) -> Result { + validate_profile_name(name)?; + Ok(self.profile_dir(tool, name)) + } + pub fn exists(&self, tool: Tool, name: &str) -> bool { - self.profile_dir(tool, name).is_dir() + validate_profile_name(name).is_ok() && self.profile_dir(tool, name).is_dir() } pub fn create(&self, tool: Tool, name: &str) -> Result { @@ -44,7 +55,7 @@ impl ProfileStore { } pub fn delete(&self, tool: Tool, name: &str) -> Result<()> { - let dir = self.profile_dir(tool, name); + let dir = self.validated_profile_dir(tool, name)?; if !dir.is_dir() { bail!( "profile '{}' not found for {}.\n \ @@ -59,9 +70,8 @@ impl ProfileStore { } pub fn rename(&self, tool: Tool, old_name: &str, new_name: &str) -> Result<()> { - validate_profile_name(new_name)?; - let old_dir = self.profile_dir(tool, old_name); - let new_dir = self.profile_dir(tool, new_name); + let old_dir = self.validated_profile_dir(tool, old_name)?; + let new_dir = self.validated_profile_dir(tool, new_name)?; if old_name == new_name { bail!("profile '{}' is already named '{}'.", old_name, new_name); @@ -126,14 +136,13 @@ impl ProfileStore { filename: &str, contents: &[u8], ) -> Result<()> { - let dir = self.profile_dir(tool, name); - let dest = dir.join(filename); + let dest = self.profile_file_path(tool, name, filename)?; reject_symlink(&dest)?; if let Some(parent) = dest.parent() { fs::create_dir_all(parent) .with_context(|| format!("could not create {}", parent.display()))?; } - let tmp = dest.with_extension("tmp"); + let tmp = staging_path_for(&dest); fs::write(&tmp, contents).with_context(|| format!("could not write {}", tmp.display()))?; set_permissions_600(&tmp)?; fs::rename(&tmp, &dest) @@ -148,8 +157,7 @@ impl ProfileStore { dest_filename: &str, ) -> Result<()> { reject_symlink(src)?; - let dir = self.profile_dir(tool, name); - let dest = dir.join(dest_filename); + let dest = self.profile_file_path(tool, name, dest_filename)?; reject_symlink(&dest)?; if let Some(parent) = dest.parent() { fs::create_dir_all(parent) @@ -161,11 +169,19 @@ impl ProfileStore { } pub fn read_file(&self, tool: Tool, name: &str, filename: &str) -> Result> { - let path = self.profile_dir(tool, name).join(filename); + let path = self.profile_file_path(tool, name, filename)?; reject_symlink(&path)?; fs::read(&path).with_context(|| format!("could not read {}", path.display())) } + /// Resolve `filename` inside a profile directory, rejecting anything that + /// would escape it (absolute paths, `..`, drive/root prefixes). + fn profile_file_path(&self, tool: Tool, name: &str, filename: &str) -> Result { + let dir = self.validated_profile_dir(tool, name)?; + validate_relative_filename(filename)?; + Ok(dir.join(filename)) + } + pub fn check_permissions(&self, path: &Path) -> Result<()> { check_permissions_600(path) } @@ -194,13 +210,55 @@ pub fn validate_profile_name(name: &str) -> Result<()> { Ok(()) } +/// Reject a path that is itself a symlink. +/// +/// Uses `symlink_metadata` rather than `Path::exists`: `exists()` follows +/// links, so a *dangling* symlink reports as absent and would slip through, +/// letting a later write create the link's target instead of the intended file. fn reject_symlink(path: &Path) -> Result<()> { - if path.exists() && path.is_symlink() { - bail!("refusing to operate on symlink: {}", path.display()); + match fs::symlink_metadata(path) { + Ok(meta) if meta.file_type().is_symlink() => { + bail!("refusing to operate on symlink: {}", path.display()) + } + _ => Ok(()), + } +} + +/// Validate a profile-relative file name such as `auth.json` or +/// `nested/state.json`. +fn validate_relative_filename(filename: &str) -> Result<()> { + if filename.is_empty() { + bail!("profile file name must not be empty"); + } + + let path = Path::new(filename); + for component in path.components() { + match component { + Component::Normal(_) => {} + Component::CurDir => {} + Component::ParentDir | Component::RootDir | Component::Prefix(_) => bail!( + "profile file name '{}' must stay inside the profile directory", + filename + ), + } } Ok(()) } +/// Sibling temp path used to stage an atomic write. +/// +/// Appends a suffix instead of replacing the extension so that sibling files +/// sharing a stem (`auth.json` and `auth.toml`) never stage through the same +/// temp path, and includes the pid so concurrent processes do not collide. +fn staging_path_for(dest: &Path) -> PathBuf { + let file_name = dest + .file_name() + .map(|name| name.to_string_lossy().into_owned()) + .unwrap_or_else(|| "profile".to_owned()); + let parent = dest.parent().unwrap_or_else(|| Path::new(".")); + parent.join(format!(".{}.aisw-tmp-{}", file_name, std::process::id())) +} + #[cfg(unix)] fn set_permissions_600(path: &Path) -> Result<()> { use std::os::unix::fs::PermissionsExt; @@ -244,6 +302,103 @@ mod tests { ProfileStore::new(dir) } + /// A name that escaped validation elsewhere must never resolve outside the + /// profiles tree, even though `profile_dir` itself does not validate. + #[test] + fn traversal_profile_names_are_rejected_by_every_file_operation() { + let dir = tempdir().unwrap(); + let s = store(dir.path()); + + for name in ["../escape", "..", "a/b", "/abs"] { + assert!( + !s.exists(Tool::Claude, name), + "exists() must not accept '{name}'" + ); + assert!(s.delete(Tool::Claude, name).is_err(), "delete '{name}'"); + assert!( + s.write_file(Tool::Claude, name, "f.json", b"{}").is_err(), + "write_file '{name}'" + ); + assert!( + s.read_file(Tool::Claude, name, "f.json").is_err(), + "read_file '{name}'" + ); + assert!( + s.rename(Tool::Claude, name, "ok").is_err(), + "rename from '{name}'" + ); + } + } + + /// A stored file name must not be able to climb out of its profile. + #[test] + fn traversal_file_names_are_rejected() { + let dir = tempdir().unwrap(); + let s = store(dir.path()); + s.create(Tool::Claude, "work").unwrap(); + + let outside = dir.path().join("stolen.json"); + for filename in ["../../stolen.json", "/etc/passwd", ""] { + let err = s + .write_file(Tool::Claude, "work", filename, b"secret") + .unwrap_err(); + assert!( + err.to_string().contains("profile file name"), + "unexpected error for '{filename}': {err}" + ); + } + assert!(!outside.exists(), "write must not escape the profile dir"); + } + + /// `Path::exists` follows symlinks, so a *dangling* link previously slipped + /// past the symlink guard and the write created the link's target. + #[test] + #[cfg(unix)] + fn write_file_refuses_to_follow_a_dangling_symlink() { + let dir = tempdir().unwrap(); + let s = store(dir.path()); + s.create(Tool::Codex, "work").unwrap(); + + let target = dir.path().join("outside-target.json"); + let link = s.profile_dir(Tool::Codex, "work").join("auth.json"); + std::os::unix::fs::symlink(&target, &link).unwrap(); + assert!(!target.exists(), "link target starts out dangling"); + + let err = s + .write_file(Tool::Codex, "work", "auth.json", b"secret") + .unwrap_err(); + assert!( + err.to_string().contains("symlink"), + "expected a symlink refusal, got: {err}" + ); + assert!( + !target.exists(), + "write must not create the symlink's target" + ); + } + + /// Sibling files sharing a stem must not stage through the same temp path. + #[test] + fn files_sharing_a_stem_do_not_collide_while_staging() { + let dir = tempdir().unwrap(); + let s = store(dir.path()); + s.create(Tool::Codex, "work").unwrap(); + + s.write_file(Tool::Codex, "work", "auth.json", b"json") + .unwrap(); + s.write_file(Tool::Codex, "work", "auth.toml", b"toml") + .unwrap(); + + assert_eq!( + s.read_file(Tool::Codex, "work", "auth.json").unwrap(), + b"json" + ); + assert_eq!( + s.read_file(Tool::Codex, "work", "auth.toml").unwrap(), + b"toml" + ); + } + #[test] fn validate_name_ok() { for name in &[ diff --git a/src/tool_detection.rs b/src/tool_detection.rs index 14d423d..4597510 100644 --- a/src/tool_detection.rs +++ b/src/tool_detection.rs @@ -28,16 +28,31 @@ enum VersionSource { Custom(VersionFn), } +/// PATH used for detection that is not given an explicit search path. +/// +/// Safety default for unit-test binaries: never resolve against the +/// developer's real PATH. Detection spawns the discovered binary to read +/// `--version`, and the real `claude`/`codex`/`gemini` CLIs run against the +/// developer's live home — concurrent test invocations have rotated and +/// invalidated real OAuth tokens. Tests that need detection must pass an +/// explicit path (`detect_at_path` / `detect_in`) or set +/// `AISW_TOOL_PATH_TEST_DIR`. +#[cfg(test)] +fn ambient_path() -> std::ffi::OsString { + crate::auth::test_overrides::var("AISW_TOOL_PATH_TEST_DIR").unwrap_or_default() +} + +#[cfg(not(test))] +fn ambient_path() -> std::ffi::OsString { + std::env::var_os("PATH").unwrap_or_default() +} + pub fn detect(tool: Tool) -> Option { - detect_at( - tool, - std::env::var_os("PATH").unwrap_or_default(), - VersionSource::Capture, - ) + detect_at(tool, ambient_path(), VersionSource::Capture) } pub fn detect_all() -> HashMap> { - let path = std::env::var_os("PATH").unwrap_or_default(); + let path = ambient_path(); Tool::ALL .into_iter() .map(|t| (t, detect_in(t, path.clone()))) @@ -170,6 +185,51 @@ mod tests { Some("injected 1.2.3".to_owned()) } + /// Unit tests must never resolve tools from the developer's real PATH. + /// + /// Detection spawns whatever it finds to read `--version`, and the real + /// `claude`/`codex`/`gemini` CLIs operate on the developer's live home. + /// Running them from the test suite has rotated and invalidated real OAuth + /// tokens, logging the developer out. Keep detection hermetic. + #[test] + fn ambient_detection_never_reaches_the_real_path() { + let _g = crate::SPAWN_LOCK.lock().unwrap_or_else(|p| p.into_inner()); + + // Even with a real-looking PATH exported, ambient detection must not use it. + let dir = tempdir().unwrap(); + make_dummy_binary(dir.path(), "claude", "claude 9.9.9", true); + let guard = crate::auth::test_overrides::EnvVarGuard::set("PATH", dir.path().as_os_str()); + + assert!( + detect(Tool::Claude).is_none(), + "detect() must ignore the ambient PATH inside unit tests" + ); + assert!( + detect_all().values().all(Option::is_none), + "detect_all() must ignore the ambient PATH inside unit tests" + ); + + drop(guard); + } + + /// The explicit opt-in still works, so tests that need detection can have it. + #[test] + fn ambient_detection_honors_the_test_override_dir() { + let _g = crate::SPAWN_LOCK.lock().unwrap_or_else(|p| p.into_inner()); + + let dir = tempdir().unwrap(); + make_dummy_binary(dir.path(), "gemini", "gemini 1.2.3", true); + let guard = crate::auth::test_overrides::EnvVarGuard::set( + "AISW_TOOL_PATH_TEST_DIR", + dir.path().as_os_str(), + ); + + let detected = detect(Tool::Gemini).expect("override dir should be searched"); + assert_eq!(detected.binary_path, dir.path().join("gemini")); + + drop(guard); + } + #[test] fn detect_missing_returns_none() { let dir = tempdir().unwrap(); diff --git a/src/types.rs b/src/types.rs index 647a29e..5ec0cad 100644 --- a/src/types.rs +++ b/src/types.rs @@ -30,6 +30,20 @@ impl Tool { } } + /// The long-flag name this tool is selected by on `aisw context` commands + /// (`--claude`, `--codex`, `--gemini`, `--antigravity`). + /// + /// This is deliberately separate from `binary_name`, which is `agy` for + /// Antigravity and would not be a valid flag. + pub fn context_flag(&self) -> &'static str { + match self { + Tool::Claude => "claude", + Tool::Codex => "codex", + Tool::Gemini => "gemini", + Tool::Antigravity => "antigravity", + } + } + pub fn display_name(&self) -> &'static str { match self { Tool::Claude => "Claude Code", diff --git a/src/workspace.rs b/src/workspace.rs index 16a0b8f..6b549aa 100644 --- a/src/workspace.rs +++ b/src/workspace.rs @@ -5,7 +5,9 @@ use std::path::{Component, Path, PathBuf}; use anyhow::{anyhow, Context, Result}; use serde::{Deserialize, Serialize}; -use crate::commands::status::{collect_status, derive_context_status, DerivedContextStatus}; +use crate::commands::status::{ + active_profiles_from_config, derive_context_status_from_active, DerivedContextStatus, +}; use crate::config::{Config, ConfigStore}; use crate::error::AiswError; use crate::types::Tool; @@ -288,29 +290,18 @@ pub fn resolve_binding(home: &Path, cwd: &Path) -> Result { }) } +/// Resolve the workspace binding and compare it against the active profiles. +/// +/// Deliberately reads active profiles from config instead of running a full +/// `collect_status`: classification only needs profile names, and the shell +/// hook calls this on every directory change, where probing tool binaries, +/// credential files and the OS keyring would add real per-prompt latency. pub fn collect_workspace_status(home: &Path, cwd: &Path) -> Result { let binding = resolve_binding(home, cwd)?; let config_store = ConfigStore::new(home); let config = config_store.load()?; - let user_home = dirs::home_dir().unwrap_or_else(|| PathBuf::from(".")); - let statuses = collect_status( - home, - &user_home, - &std::env::var_os("PATH").unwrap_or_default(), - )?; - let context_status = derive_context_status(&config, &statuses); - let active_profiles = Tool::ALL - .iter() - .map(|tool| { - ( - *tool, - statuses - .iter() - .find(|status| status.tool == *tool) - .and_then(|status| status.active_profile.clone()), - ) - }) - .collect::>(); + let active_profiles = active_profiles_from_config(&config); + let context_status = derive_context_status_from_active(&config, &active_profiles); let status = classify_workspace_state(&config, &binding, &active_profiles, &context_status); let recommended_command = match status { diff --git a/tests/all_commands_smoke.rs b/tests/all_commands_smoke.rs new file mode 100644 index 0000000..5574963 --- /dev/null +++ b/tests/all_commands_smoke.rs @@ -0,0 +1,411 @@ +//! End-to-end smoke coverage for every top-level command and subcommand. +//! +//! The audit touched shared plumbing (config writes, profile paths, tool +//! detection, JSON contracts, shell hooks). This suite exercises the full CLI +//! surface against a sandboxed home so a regression in any one command shows up +//! as a failing test rather than as a broken command nobody ran. + +mod common; + +use common::TestEnv; + +const CLAUDE_KEY: &str = "sk-ant-api03-AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA"; +const CLAUDE_KEY_ALT: &str = "sk-ant-api03-BBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBB"; +const CODEX_KEY: &str = "sk-codex-test-key-12345"; +const GEMINI_KEY: &str = "AIzatest1234567890ABCDEF"; + +/// A sandbox with all four tool binaries faked and one profile per tool. +fn env_with_profiles() -> TestEnv { + let env = TestEnv::new(); + for tool in ["claude", "codex", "gemini", "agy"] { + env.add_fake_tool(tool, "1.0.0"); + } + env.cmd() + .args(["add", "claude", "work", "--api-key", CLAUDE_KEY]) + .assert() + .success(); + env.cmd() + .args(["add", "codex", "work", "--api-key", CODEX_KEY]) + .assert() + .success(); + env.cmd() + .args(["add", "gemini", "work", "--api-key", GEMINI_KEY]) + .assert() + .success(); + env +} + +/// Run a command, require exit 0, and return stdout. +fn ok(env: &TestEnv, args: &[&str]) -> String { + let output = env.output(args); + assert!( + output.status.success(), + "aisw {} failed (exit {:?})\nstdout:\n{}\nstderr:\n{}", + args.join(" "), + output.status.code(), + String::from_utf8_lossy(&output.stdout), + String::from_utf8_lossy(&output.stderr), + ); + String::from_utf8_lossy(&output.stdout).into_owned() +} + +/// Run a command expecting valid JSON on stdout and exit 0. +fn ok_json(env: &TestEnv, args: &[&str]) -> serde_json::Value { + let stdout = ok(env, args); + serde_json::from_str(&stdout).unwrap_or_else(|e| { + panic!( + "aisw {} did not emit valid JSON: {e}\n{stdout}", + args.join(" ") + ) + }) +} + +#[test] +fn version_and_capabilities() { + let env = TestEnv::new(); + assert!(!ok(&env, &["version"]).trim().is_empty()); + let json = ok_json(&env, &["version", "--json"]); + assert!(json.get("version").is_some() || json["result"].get("version").is_some()); + + assert!(!ok(&env, &["capabilities"]).trim().is_empty()); + ok_json(&env, &["capabilities", "--json"]); +} + +#[test] +fn init_variants() { + let env = TestEnv::new(); + env.add_fake_tool("claude", "claude 1.0.0"); + + ok(&env, &["init", "--yes", "--no-shell-hook"]); + let json = ok_json(&env, &["init", "--json", "--no-shell-hook"]); + assert_eq!(json["ok"], true); + let detect = ok_json( + &env, + &["init", "--json", "--no-shell-hook", "--detect-live"], + ); + assert_eq!(detect["ok"], true); +} + +#[test] +fn add_variants_across_tools() { + let env = TestEnv::new(); + for tool in ["claude", "codex", "gemini", "agy"] { + env.add_fake_tool(tool, "1.0.0"); + } + + // --api-key, --label, --json + let json = ok_json( + &env, + &[ + "add", + "claude", + "work", + "--api-key", + CLAUDE_KEY, + "--label", + "Work", + "--json", + ], + ); + assert_eq!(json["ok"], true); + + // --set-active + ok( + &env, + &[ + "add", + "claude", + "second", + "--api-key", + CLAUDE_KEY_ALT, + "--set-active", + ], + ); + let status = ok_json(&env, &["status", "--json"]); + let claude = status + .as_array() + .unwrap() + .iter() + .find(|s| s["tool"] == "claude") + .unwrap(); + assert_eq!(claude["active_profile"], "second"); + + // --from-env + let out = env + .cmd() + .env("OPENAI_API_KEY", CODEX_KEY) + .args(["add", "codex", "fromenv", "--from-env"]) + .output() + .unwrap(); + assert!( + out.status.success(), + "--from-env failed: {}", + String::from_utf8_lossy(&out.stderr) + ); + + // --credential-backend file is always valid. + ok( + &env, + &[ + "add", + "gemini", + "filed", + "--api-key", + GEMINI_KEY, + "--credential-backend", + "file", + ], + ); +} + +#[test] +fn list_all_filters() { + let env = env_with_profiles(); + + ok(&env, &["list"]); + ok_json(&env, &["list", "--json"]); + ok(&env, &["list", "claude"]); + ok(&env, &["list", "--tool", "codex"]); + ok(&env, &["list", "--search", "work"]); + ok(&env, &["list", "--sort", "name"]); + ok(&env, &["list", "--sort", "recent"]); + ok(&env, &["list", "--active-only"]); +} + +#[test] +fn status_all_filters() { + let env = env_with_profiles(); + ok(&env, &["use", "claude", "work"]); + + ok(&env, &["status"]); + ok_json(&env, &["status", "--json"]); + ok(&env, &["status", "--tool", "claude"]); + ok(&env, &["status", "--search", "work"]); + ok(&env, &["status", "--sort", "name"]); + ok(&env, &["status", "--sort", "recent"]); + ok(&env, &["status", "--active-only"]); + ok_json(&env, &["status", "--context", "--json"]); +} + +#[test] +fn use_variants() { + let env = env_with_profiles(); + + ok(&env, &["use", "claude", "work"]); + ok_json(&env, &["use", "codex", "work", "--json"]); + ok(&env, &["use", "claude", "work", "--state-mode", "shared"]); + ok(&env, &["use", "claude", "work", "--state-mode", "isolated"]); + ok(&env, &["use", "--all", "--profile", "work"]); + ok(&env, &["use", "claude", "work", "--emit-env"]); + ok(&env, &["use", "--all", "--profile", "work", "--emit-env"]); +} + +#[test] +fn context_full_lifecycle() { + let env = env_with_profiles(); + + ok_json( + &env, + &[ + "context", "create", "team", "--claude", "work", "--codex", "work", "--json", + ], + ); + ok(&env, &["context", "list"]); + ok_json(&env, &["context", "list", "--json"]); + ok(&env, &["context", "list", "--search", "team"]); + ok_json( + &env, + &["context", "set", "team", "--gemini", "work", "--json"], + ); + ok_json(&env, &["context", "unset", "team", "--gemini", "--json"]); + ok_json(&env, &["context", "use", "team", "--json"]); + ok(&env, &["context", "use", "team", "--emit-env"]); + ok_json(&env, &["context", "rename", "team", "squad", "--json"]); + ok_json(&env, &["context", "remove", "squad", "--yes", "--json"]); +} + +#[test] +fn rename_and_remove() { + let env = env_with_profiles(); + + ok_json(&env, &["rename", "claude", "work", "renamed", "--json"]); + ok_json(&env, &["remove", "claude", "renamed", "--yes", "--json"]); + + // --force path: remove the active profile. + ok(&env, &["use", "codex", "work"]); + ok(&env, &["remove", "codex", "work", "--yes", "--force"]); +} + +#[test] +fn backup_list_and_restore() { + let env = env_with_profiles(); + // A switch creates a backup. + ok(&env, &["use", "claude", "work"]); + + ok(&env, &["backup", "list"]); + let json = ok_json(&env, &["backup", "list", "--json"]); + let entries = json.as_array().expect("backup list is an array"); + assert!(!entries.is_empty(), "a switch should produce a backup"); + + ok(&env, &["backup", "list", "--tool", "claude"]); + ok(&env, &["backup", "list", "--search", "work"]); + ok(&env, &["backup", "list", "--sort", "name"]); + ok(&env, &["backup", "list", "--sort", "recent"]); + ok(&env, &["backup", "list", "--active-only"]); + + let id = entries[0]["backup_id"].as_str().expect("backup id"); + ok_json(&env, &["backup", "restore", id, "--yes", "--json"]); +} + +#[test] +fn doctor_verify_repair() { + let env = env_with_profiles(); + + // doctor/verify exit non-zero when something is unhealthy, so only assert + // that they run and emit valid JSON. + let doctor = env.output(&["doctor", "--json"]); + serde_json::from_slice::(&doctor.stdout).expect("doctor emits JSON"); + let verify = env.output(&["verify", "--json"]); + serde_json::from_slice::(&verify.stdout).expect("verify emits JSON"); + env.output(&["doctor"]); + env.output(&["verify"]); + + ok(&env, &["repair", "--dry-run"]); + ok_json(&env, &["repair", "--json", "--dry-run"]); + ok_json(&env, &["repair", "--json", "--apply"]); + ok_json( + &env, + &["repair", "--json", "--apply", "--fix", "home,permissions"], + ); + + // After a repair --apply, the installation is healthy. + let doctor_after = env.output(&["doctor", "--json"]); + let json: serde_json::Value = serde_json::from_slice(&doctor_after.stdout).unwrap(); + let failures: Vec<_> = json["checks"] + .as_array() + .unwrap() + .iter() + .filter(|c| c["status"] == "fail") + .collect(); + assert!( + failures.is_empty(), + "unexpected doctor failures: {failures:#?}" + ); +} + +#[test] +fn shell_hook_every_shell() { + let env = TestEnv::new(); + for shell in ["bash", "zsh", "fish", "pwsh"] { + let hook = ok(&env, &["shell-hook", shell]); + assert!(hook.contains("aisw"), "{shell} hook looks empty:\n{hook}"); + } +} + +#[test] +fn workspace_and_project_bindings() { + let env = env_with_profiles(); + ok(&env, &["context", "create", "team", "--claude", "work"]); + + ok_json(&env, &["workspace", "guard", "--mode", "warn", "--json"]); + ok_json(&env, &["workspace", "guard", "--mode", "strict", "--json"]); + // Return to warn so `check` cannot hard-fail the rest of the test. + ok_json(&env, &["workspace", "guard", "--mode", "warn", "--json"]); + + ok_json( + &env, + &[ + "workspace", + "bind", + "--git-remote", + "github.com/acme/*", + "--context", + "team", + "--json", + ], + ); + ok_json(&env, &["workspace", "status", "--json"]); + ok(&env, &["workspace", "status"]); + ok_json(&env, &["workspace", "doctor", "--json"]); + ok(&env, &["workspace", "doctor"]); + ok(&env, &["workspace", "check"]); + ok(&env, &["workspace", "check", "--tool", "claude"]); + ok(&env, &["workspace", "check", "--prompt"]); + ok_json( + &env, + &[ + "workspace", + "unbind", + "--git-remote", + "github.com/acme/*", + "--json", + ], + ); + + ok(&env, &["project-bindings", "list"]); + ok_json(&env, &["project-bindings", "list", "--json"]); +} + +#[test] +fn uninstall_dry_run_then_apply() { + let env = env_with_profiles(); + + ok(&env, &["uninstall", "--dry-run"]); + ok(&env, &["uninstall", "--dry-run", "--remove-data"]); + assert!(env.aisw_home.exists(), "--dry-run must not delete anything"); + + ok(&env, &["uninstall", "--yes"]); + assert!( + env.aisw_home.exists(), + "uninstall without --remove-data keeps data" + ); + + ok(&env, &["uninstall", "--yes", "--remove-data"]); + assert!(!env.aisw_home.exists(), "--remove-data deletes AISW_HOME"); +} + +/// Global flags must work on every command, not just a few. +#[test] +fn global_flags_apply_across_commands() { + let env = env_with_profiles(); + + for args in [ + vec!["--no-color", "list"], + vec!["--quiet", "list"], + vec!["--non-interactive", "list"], + vec!["--no-color", "status"], + vec!["--quiet", "status"], + vec!["--non-interactive", "backup", "list"], + vec!["--no-color", "context", "list"], + ] { + ok(&env, &args); + } +} + +/// `--json` must never write to stderr on success — machine consumers parse +/// stdout and treat stderr as a failure signal. +#[test] +fn json_mode_keeps_stderr_clean() { + let env = env_with_profiles(); + ok(&env, &["use", "claude", "work"]); + + for args in [ + vec!["list", "--json"], + vec!["status", "--json"], + vec!["backup", "list", "--json"], + vec!["context", "list", "--json"], + vec!["project-bindings", "list", "--json"], + vec!["workspace", "status", "--json"], + vec!["repair", "--json", "--dry-run"], + vec!["version", "--json"], + vec!["capabilities", "--json"], + ] { + let output = env.output(&args); + assert!( + output.stderr.is_empty(), + "aisw {} wrote to stderr in --json mode:\n{}", + args.join(" "), + String::from_utf8_lossy(&output.stderr) + ); + } +} diff --git a/tests/audit_regressions.rs b/tests/audit_regressions.rs new file mode 100644 index 0000000..12e4e56 --- /dev/null +++ b/tests/audit_regressions.rs @@ -0,0 +1,600 @@ +//! Regression tests for defects found during the command-by-command audit. +//! +//! Each test pins a specific failure that shipped previously, so a future +//! refactor cannot silently reintroduce it. + +mod common; + +use common::TestEnv; + +const CLAUDE_KEY: &str = "sk-ant-api03-AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA"; + +/// Gemini stores API keys as `GEMINI_API_KEY=` in a `.env` file the CLI +/// sources. Validation only rejected empty keys, so a key containing a newline +/// injected additional environment variables into that file. +#[test] +fn api_keys_with_control_characters_are_rejected() { + let env = TestEnv::new(); + env.add_fake_tool("gemini", "gemini 1.0.0"); + env.add_fake_tool("claude", "claude 1.0.0"); + env.add_fake_tool("codex", "codex 1.0.0"); + + let injected = "AIzaLegitLooking123\nGOOGLE_CLOUD_PROJECT=attacker-project"; + let output = env + .cmd() + .args(["add", "gemini", "work", "--api-key", injected]) + .output() + .unwrap(); + + assert!( + !output.status.success(), + "a key containing a newline must be rejected" + ); + assert!( + String::from_utf8_lossy(&output.stderr).contains("control character"), + "stderr should explain the rejection: {}", + String::from_utf8_lossy(&output.stderr) + ); + + let env_file = env + .aisw_home + .join("profiles") + .join("gemini") + .join("work") + .join(".env"); + assert!( + !env_file.exists(), + "no profile should be written for a rejected key" + ); + + // Every tool applies the same rule. + for (tool, key) in [ + ("claude", "sk-ant-api03-AAAA\nFOO=bar"), + ("codex", "sk-codex-AAAA\rFOO=bar"), + ] { + let output = env + .cmd() + .args(["add", tool, "injected", "--api-key", key]) + .output() + .unwrap(); + assert!( + !output.status.success(), + "{tool} must reject a key containing a control character" + ); + } +} + +/// The normal case must keep working — this rule rejects control characters +/// only, not ordinary key charsets. +#[test] +fn ordinary_api_keys_are_still_accepted() { + let env = TestEnv::new(); + env.add_fake_tool("gemini", "gemini 1.0.0"); + env.cmd() + .args([ + "add", + "gemini", + "work", + "--api-key", + "AIzatest1234567890ABCDEF", + ]) + .assert() + .success(); + + let env_file = env + .aisw_home + .join("profiles") + .join("gemini") + .join("work") + .join(".env"); + let contents = std::fs::read_to_string(env_file).unwrap(); + assert_eq!(contents, "GEMINI_API_KEY=AIzatest1234567890ABCDEF\n"); +} + +fn config_with_dangling_active() -> &'static str { + r#"{ + "version": 2, + "active": {"claude": "ghost", "codex": null, "gemini": null, "antigravity": null}, + "profiles": {"claude": {}, "codex": {}, "gemini": {}, "antigravity": {}}, + "contexts": {}, + "settings": {"backup_on_switch": true, "max_backups": 10} +}"# +} + +/// `active` naming a profile with no config entry used to index a `HashMap` +/// directly and abort the process with "no entry found for key". +#[test] +fn status_reports_dangling_active_profile_instead_of_panicking() { + let env = TestEnv::new(); + env.add_fake_tool("claude", "claude 1.0.0"); + std::fs::write( + env.aisw_home.join("config.json"), + config_with_dangling_active(), + ) + .unwrap(); + + let output = env.output(&["status"]); + let stderr = String::from_utf8_lossy(&output.stderr); + assert!( + !stderr.contains("panicked"), + "status must not panic on a dangling active profile:\n{stderr}" + ); + assert!(output.status.success(), "status should still exit 0"); + + let stdout = String::from_utf8_lossy(&output.stdout); + assert!( + stdout.contains("missing from aisw config"), + "status should explain the dangling active profile:\n{stdout}" + ); +} + +#[test] +fn status_json_reports_dangling_active_profile_instead_of_panicking() { + let env = TestEnv::new(); + env.add_fake_tool("claude", "claude 1.0.0"); + std::fs::write( + env.aisw_home.join("config.json"), + config_with_dangling_active(), + ) + .unwrap(); + + let output = env.output(&["status", "--json"]); + assert!(output.status.success(), "status --json should exit 0"); + + let json: serde_json::Value = serde_json::from_slice(&output.stdout).expect("valid json"); + let claude = json + .as_array() + .expect("array") + .iter() + .find(|entry| entry["tool"] == "claude") + .expect("claude entry"); + assert_eq!(claude["active_profile"], "ghost"); + assert_eq!(claude["credentials_present"], false); +} + +/// `remove` used to snapshot, delete the keyring secret, and delete the profile +/// directory *before* the config write rejected the removal, destroying +/// credentials for a profile it then refused to remove. +#[test] +fn remove_rejects_context_referenced_profile_before_deleting_anything() { + let env = TestEnv::new(); + env.add_fake_tool("claude", "claude 1.0.0"); + env.cmd() + .args(["add", "claude", "work", "--api-key", CLAUDE_KEY]) + .assert() + .success(); + env.cmd() + .args(["context", "create", "team", "--claude", "work"]) + .assert() + .success(); + + let output = env.output(&["remove", "claude", "work", "--yes"]); + assert!( + !output.status.success(), + "remove must fail while a context references the profile" + ); + let stderr = String::from_utf8_lossy(&output.stderr); + assert!( + stderr.contains("referenced by contexts"), + "error should name the blocking context:\n{stderr}" + ); + + let profile_dir = env.aisw_home.join("profiles").join("claude").join("work"); + assert!( + profile_dir.is_dir(), + "profile directory must survive a rejected remove" + ); + assert!( + profile_dir.join(".credentials.json").is_file(), + "credentials must survive a rejected remove" + ); + + // The profile is still fully usable afterwards. + env.cmd().args(["use", "claude", "work"]).assert().success(); +} + +/// Removing the active profile must clear `active` atomically with the profile +/// deletion, so config never names a profile that no longer exists. +#[test] +fn removing_active_profile_leaves_no_dangling_active_entry() { + let env = TestEnv::new(); + env.add_fake_tool("claude", "claude 1.0.0"); + env.cmd() + .args(["add", "claude", "work", "--api-key", CLAUDE_KEY]) + .assert() + .success(); + env.cmd().args(["use", "claude", "work"]).assert().success(); + + env.cmd() + .args(["remove", "claude", "work", "--yes", "--force"]) + .assert() + .success(); + + let config: serde_json::Value = + serde_json::from_str(&std::fs::read_to_string(env.aisw_home.join("config.json")).unwrap()) + .unwrap(); + assert_eq!(config["active"]["claude"], serde_json::Value::Null); + assert!(config["profiles"]["claude"] + .as_object() + .expect("object") + .is_empty()); + + // And status stays healthy. + let output = env.output(&["status"]); + assert!(output.status.success()); + assert!(!String::from_utf8_lossy(&output.stderr).contains("panicked")); +} + +/// Antigravity is a first-class tool, so its binary must be guarded by the +/// shell hook like every other tool. +#[test] +fn shell_hooks_guard_the_antigravity_binary() { + let env = TestEnv::new(); + for shell in ["bash", "zsh", "fish", "pwsh"] { + let output = env.output(&["shell-hook", shell]); + assert!(output.status.success(), "shell-hook {shell} should succeed"); + let hook = String::from_utf8_lossy(&output.stdout); + assert!( + hook.contains("workspace check --tool antigravity"), + "{shell} hook must guard antigravity:\n{hook}" + ); + assert!( + hook.contains("agy"), + "{shell} hook must wrap the agy binary:\n{hook}" + ); + } +} + +/// `workspace status --json` omitted antigravity from `active_profiles`, so a +/// GUI reading the contract could not see the Antigravity account. +#[test] +fn workspace_status_json_includes_every_tool() { + let env = TestEnv::new(); + let output = env.output(&["workspace", "status", "--json"]); + assert!(output.status.success()); + + let json: serde_json::Value = serde_json::from_slice(&output.stdout).expect("valid json"); + let active = json["active_profiles"].as_object().expect("object"); + for tool in ["claude", "codex", "gemini", "antigravity"] { + assert!( + active.contains_key(tool), + "active_profiles is missing '{tool}': {active:?}" + ); + } +} + +/// `status --context --json` omitted antigravity from the mapped profiles. +#[test] +fn status_context_json_includes_every_tool() { + let env = TestEnv::new(); + env.add_fake_tool("claude", "claude 1.0.0"); + env.add_fake_tool("agy", "agy 1.0.0"); + env.cmd() + .args(["add", "claude", "work", "--api-key", CLAUDE_KEY]) + .assert() + .success(); + env.cmd() + .args(["context", "create", "team", "--claude", "work"]) + .assert() + .success(); + env.cmd().args(["use", "claude", "work"]).assert().success(); + + let output = env.output(&["status", "--context", "--json"]); + assert!(output.status.success()); + + let json: serde_json::Value = serde_json::from_slice(&output.stdout).expect("valid json"); + let profiles = json["context"]["profiles"] + .as_object() + .expect("mapped profiles object"); + for tool in ["claude", "codex", "gemini", "antigravity"] { + assert!( + profiles.contains_key(tool), + "context profiles is missing '{tool}': {profiles:?}" + ); + } + assert_eq!(profiles["claude"], "work"); +} + +/// `uninstall --remove-data` deleted AISW_HOME but stranded credentials in the +/// OS keyring forever. +#[test] +#[cfg(unix)] +fn uninstall_remove_data_purges_system_keyring_secrets() { + let env = TestEnv::new(); + env.add_fake_tool("claude", "claude 1.0.0"); + env.cmd() + .args([ + "add", + "claude", + "work", + "--api-key", + CLAUDE_KEY, + "--credential-backend", + "system-keyring", + ]) + .assert() + .success(); + + let keychain = env.fake_home.join("keychain"); + assert!( + secret_files(&keychain) > 0, + "test keyring should hold the profile secret before uninstall" + ); + + env.cmd() + .args(["uninstall", "--remove-data", "--yes"]) + .assert() + .success(); + + assert!(!env.aisw_home.exists(), "AISW_HOME should be deleted"); + assert_eq!( + secret_files(&keychain), + 0, + "keyring secrets must not outlive --remove-data" + ); +} + +/// Count stored secrets in the fake keyring tree used by the test harness. +#[cfg(unix)] +fn secret_files(root: &std::path::Path) -> usize { + fn walk(path: &std::path::Path, count: &mut usize) { + let Ok(entries) = std::fs::read_dir(path) else { + return; + }; + for entry in entries.flatten() { + let path = entry.path(); + if path.is_dir() { + walk(&path, count); + } else if path.file_name().is_some_and(|name| name == "secret") { + *count += 1; + } + } + } + + let mut count = 0; + walk(root, &mut count); + count +} + +const CODEX_KEY: &str = "sk-codex-test-key-12345"; +const GEMINI_KEY: &str = "AIzatest1234567890ABCDEF"; + +/// `doctor` looked for one hardcoded credential filename per tool. Gemini never +/// writes that name, so every valid Gemini profile produced a hard failure and +/// `aisw doctor` exited 1 — which also dragged `aisw verify` down with it. +#[test] +fn doctor_passes_for_a_healthy_profile_of_every_tool() { + let env = TestEnv::new(); + for tool in ["claude", "codex", "gemini", "agy"] { + env.add_fake_tool(tool, "1.0.0"); + } + env.cmd().args(["init", "--yes"]).assert().success(); + env.cmd() + .args(["add", "claude", "work", "--api-key", CLAUDE_KEY]) + .assert() + .success(); + env.cmd() + .args(["add", "codex", "work", "--api-key", CODEX_KEY]) + .assert() + .success(); + env.cmd() + .args(["add", "gemini", "work", "--api-key", GEMINI_KEY]) + .assert() + .success(); + + let output = env.output(&["doctor", "--json"]); + let json: serde_json::Value = serde_json::from_slice(&output.stdout).expect("valid json"); + let failures: Vec<&serde_json::Value> = json["checks"] + .as_array() + .expect("checks array") + .iter() + .filter(|check| check["status"] == "fail") + .collect(); + assert!( + failures.is_empty(), + "doctor should not fail for healthy profiles: {failures:#?}" + ); + assert!( + output.status.success(), + "doctor should exit 0 for healthy profiles" + ); +} + +/// The permission check must still catch a genuinely world-readable credential. +#[test] +#[cfg(unix)] +fn doctor_still_fails_on_broad_credential_permissions() { + use std::os::unix::fs::PermissionsExt; + + let env = TestEnv::new(); + env.add_fake_tool("gemini", "gemini 1.0.0"); + env.cmd() + .args(["add", "gemini", "work", "--api-key", GEMINI_KEY]) + .assert() + .success(); + + let env_file = env + .aisw_home + .join("profiles") + .join("gemini") + .join("work") + .join(".env"); + std::fs::set_permissions(&env_file, std::fs::Permissions::from_mode(0o644)).unwrap(); + + let output = env.output(&["doctor", "--json"]); + let json: serde_json::Value = serde_json::from_slice(&output.stdout).expect("valid json"); + let check = json["checks"] + .as_array() + .expect("checks array") + .iter() + .find(|check| check["name"] == "permissions/gemini/work") + .expect("gemini permission check"); + assert_eq!(check["status"], "fail", "broad permissions must fail"); + assert!(!output.status.success(), "doctor should exit non-zero"); +} + +/// `use --all` dropped `--emit-env`, so the shell hook's +/// `aisw use --all --profile X --emit-env` performed the full switch instead of +/// printing exports — and then the hook ran the switch a second time. +#[test] +fn use_all_honors_emit_env() { + let env = TestEnv::new(); + env.add_fake_tool("claude", "claude 1.0.0"); + env.add_fake_tool("codex", "codex 1.0.0"); + env.cmd() + .args(["add", "claude", "work", "--api-key", CLAUDE_KEY]) + .assert() + .success(); + env.cmd() + .args(["add", "codex", "work", "--api-key", CODEX_KEY]) + .assert() + .success(); + + let output = env.output(&["use", "--all", "--profile", "work", "--emit-env"]); + assert!( + output.status.success(), + "use --all --emit-env should succeed" + ); + + let stdout = String::from_utf8_lossy(&output.stdout); + assert!( + stdout.contains("export ") || stdout.contains("set -gx "), + "--emit-env must print shell exports, got:\n{stdout}" + ); + // stdout is eval'd by the shell hook, so it must contain nothing else. + for line in stdout.lines().filter(|line| !line.trim().is_empty()) { + assert!( + line.starts_with("export ") + || line.starts_with("unset ") + || line.starts_with("set -gx ") + || line.starts_with("set -e "), + "non-shell line would be eval'd by the hook: {line}" + ); + } + + // The switch is still recorded, exactly once. + let config: serde_json::Value = + serde_json::from_str(&std::fs::read_to_string(env.aisw_home.join("config.json")).unwrap()) + .unwrap(); + assert_eq!(config["active"]["claude"], "work"); + assert_eq!(config["active"]["codex"], "work"); +} + +/// `use --all` dropped `--state-mode`, silently ignoring the flag. +#[test] +fn use_all_honors_state_mode() { + let env = TestEnv::new(); + env.add_fake_tool("codex", "codex 1.0.0"); + env.cmd() + .args(["add", "codex", "work", "--api-key", CODEX_KEY]) + .assert() + .success(); + + env.cmd() + .args([ + "use", + "--all", + "--profile", + "work", + "--state-mode", + "shared", + ]) + .assert() + .success(); + + let config: serde_json::Value = + serde_json::from_str(&std::fs::read_to_string(env.aisw_home.join("config.json")).unwrap()) + .unwrap(); + assert_eq!( + config["settings"]["codex"]["state_mode"], "shared", + "--state-mode must reach the tools that support it" + ); +} + +/// A tool that was attempted and failed must not leave the exit code at 0. +#[test] +fn use_all_exits_non_zero_when_a_tool_switch_fails() { + let env = TestEnv::new(); + env.add_fake_tool("claude", "claude 1.0.0"); + env.add_fake_tool("codex", "codex 1.0.0"); + env.cmd() + .args(["add", "claude", "work", "--api-key", CLAUDE_KEY]) + .assert() + .success(); + env.cmd() + .args(["add", "codex", "work", "--api-key", CODEX_KEY]) + .assert() + .success(); + + // Break just the codex profile's stored credentials. + std::fs::remove_dir_all(env.aisw_home.join("profiles").join("codex").join("work")).unwrap(); + + let output = env.output(&["use", "--all", "--profile", "work"]); + assert!( + !output.status.success(), + "use --all must fail when an attempted switch errors\nstdout:\n{}", + String::from_utf8_lossy(&output.stdout) + ); + assert!( + String::from_utf8_lossy(&output.stderr).contains("codex"), + "the failing tool should be named: {}", + String::from_utf8_lossy(&output.stderr) + ); +} + +/// `init --json` looked up the rc file for whatever `$SHELL` reported and hit +/// an `unreachable!()` for anything but bash/zsh/fish/pwsh — so it aborted with +/// exit 101 under a plain `/bin/sh`, which is the default in most containers. +#[test] +fn init_json_survives_an_unsupported_shell() { + for shell in ["/bin/sh", "/bin/dash", "/usr/bin/nu", "/usr/bin/ksh"] { + let env = TestEnv::new(); + let output = env + .cmd() + .env("SHELL", shell) + .args(["init", "--json", "--no-shell-hook"]) + .output() + .unwrap(); + + let stderr = String::from_utf8_lossy(&output.stderr); + assert!( + !stderr.contains("panicked"), + "init --json panicked under SHELL={shell}:\n{stderr}" + ); + assert!( + output.status.success(), + "init --json should succeed under SHELL={shell}: {stderr}" + ); + + let json: serde_json::Value = serde_json::from_slice(&output.stdout).expect("valid json"); + assert_eq!(json["ok"], true); + assert_eq!( + json["result"]["shell"]["rc_file"], + serde_json::Value::Null, + "an unsupported shell has no aisw rc file" + ); + } +} + +/// AISW_HOME is user-supplied; `--remove-data` must not turn into `rm -rf ~`. +#[test] +fn uninstall_refuses_to_delete_the_user_home_directory() { + let env = TestEnv::new(); + let output = env + .cmd() + .env("AISW_HOME", &env.fake_home) + .args(["uninstall", "--remove-data", "--yes"]) + .output() + .unwrap(); + + assert!( + !output.status.success(), + "uninstall must refuse when AISW_HOME is the home directory" + ); + assert!( + String::from_utf8_lossy(&output.stderr).contains("must not be your home directory"), + "stderr should explain the refusal: {}", + String::from_utf8_lossy(&output.stderr) + ); + assert!(env.fake_home.exists(), "home directory must be untouched"); +}