From fe1c9097921fe5582e928f18e407b62ff88e2866 Mon Sep 17 00:00:00 2001 From: Vinnie Mazza Date: Tue, 16 Jun 2026 19:14:12 -0400 Subject: [PATCH 1/2] fix(auth): dedup container credentials at injection time; add auth mode MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Part A: when the harness declares an env(VAR) overlay that covers a provider service (e.g. env(ANTHROPIC_API_KEY) → service "anthropic"), drop the matching keychain credential from agent_credentials at ResolvedContainerOptions::resolve time. This prevents containers from receiving two conflicting Anthropic credentials when the user has both ANTHROPIC_API_KEY declared as an overlay and a CLAUDE_CODE_OAUTH_TOKEN in the keychain. The dedup is keyed on declared env only — never on std::env::var — so cloud harnesses without a declared anthropic var continue to receive keychain OAuth unaffected. Part B: add a per-harness `auth` field to RepoConfig with three modes: `keychain` (default, inject keychain creds with Part-A dedup applied), `passthrough` (never inject keychain creds; rely on env() overlays), `none` (no credential injection). Gated in AuthEngine::resolve_agent_auth via session.effective_config().auth_mode(), which is resolved through the existing EffectiveConfig precedence layer. Shared infrastructure: service_for_credential is now the canonical table in engine::auth and is re-exported into the dsbx module rather than duplicated. CLAUDE_CODE_OAUTH_TOKEN is added to the table (→ "anthropic") so the dedup can match it against ANTHROPIC_API_KEY by service name; the sbx driver's existing key-name guard for the OAuth token is updated to be an unconditional continue-before-service-lookup so sbx behaviour is unchanged. Also apply the pre-existing clippy::unnecessary_min_or_max fix in src/frontend/tui/render.rs required to pass the lint gate. --- docs/07-configuration.md | 19 +++ src/data/config/effective.rs | 83 +++++++++++- src/data/config/mod.rs | 3 +- src/data/config/repo.rs | 25 ++++ src/engine/auth/mod.rs | 177 ++++++++++++++++++++++++- src/engine/container/options.rs | 220 ++++++++++++++++++++++++++++++++ src/engine/sandbox/dsbx/auth.rs | 50 ++++---- src/frontend/tui/render.rs | 2 +- 8 files changed, 547 insertions(+), 32 deletions(-) diff --git a/docs/07-configuration.md b/docs/07-configuration.md index 3a2ac004..b649f5a1 100644 --- a/docs/07-configuration.md +++ b/docs/07-configuration.md @@ -168,6 +168,24 @@ awman config set yoloDisallowedTools "" # set an empty list An empty repo list actively overrides a non-empty global list. To stop overriding, remove the field from the repo config file. See [Yolo Mode](06-yolo-mode.md). +### Control credential injection (`auth` mode) + +By default awman injects host keychain credentials into agent containers +(`keychain` mode). Two alternatives are available for harnesses that supply +credentials through other means: + +```json +{ "auth": "passthrough" } +``` + +| Value | Behaviour | +|-------|-----------| +| `keychain` (default) | Inject host keychain credentials. When the harness also declares `env(ANTHROPIC_API_KEY)` (or another credential that covers the same provider), the keychain OAuth token for that provider is automatically suppressed at injection time — the container receives exactly one set of credentials per provider. | +| `passthrough` | Skip keychain injection entirely. Supply credentials via `env(VAR)` overlays; awman never injects anything from the keychain. | +| `none` | No credential injection at all. | + +Set `auth` in `.awman/config.json` directly (it is not settable via `config set`). The field is per-repo only — cloud harnesses that do not declare an anthropic env var remain on the default `keychain` path and continue to receive keychain OAuth unaffected. + --- ## Runtimes @@ -220,6 +238,7 @@ awman keeps global config and data (workflows, skills, worktrees, API state) und | `agentStuckTimeout` | integer (seconds) | 30 | Inactivity period before an agent is flagged as stuck | yes | | `baseImage` | string | (unset → global) | Image tag for workflow setup/teardown containers — see [Workflows](05-workflows.md) | no (edit file) | | `dockerfile` | string | `Dockerfile.dev` | Path to the project base Dockerfile, relative to repo root or absolute | no (edit file) | +| `auth` | `"keychain"` \| `"passthrough"` \| `"none"` | `"keychain"` | Credential injection mode — see [Control credential injection](#control-credential-injection-auth-mode) | no (edit file) | ### Global config fields (`$HOME/.awman/config.json`) diff --git a/src/data/config/effective.rs b/src/data/config/effective.rs index c31d0319..9e38674c 100644 --- a/src/data/config/effective.rs +++ b/src/data/config/effective.rs @@ -9,7 +9,7 @@ use std::time::Duration; use crate::data::config::env::EnvSnapshot; use crate::data::config::flags::FlagConfig; use crate::data::config::global::GlobalConfig; -use crate::data::config::repo::RepoConfig; +use crate::data::config::repo::{AgentAuthMode, RepoConfig}; use crate::data::config::{DEFAULT_AGENT_STUCK_TIMEOUT_SECS, DEFAULT_SCROLLBACK_LINES}; /// Merged view of every configuration source, in precedence order. @@ -206,6 +206,15 @@ impl EffectiveConfig { } self.global.base_image.clone() } + + /// Effective credential injection mode (repo > built-in default `keychain`). + /// + /// `passthrough` skips keychain injection entirely; `none` disables all + /// injection; `keychain` (default) injects host keychain creds, subject to + /// injection-time dedup of any harness-declared env vars. + pub fn auth_mode(&self) -> AgentAuthMode { + self.repo.auth.unwrap_or_default() + } } #[cfg(test)] @@ -214,7 +223,7 @@ mod tests { use crate::data::config::env::{ EnvSnapshot, AWMAN_API_KEY, AWMAN_REMOTE_ADDR, AWMAN_REMOTE_SESSION, }; - use crate::data::config::repo::{ApiConfig, RemoteConfig}; + use crate::data::config::repo::{AgentAuthMode, ApiConfig, RemoteConfig}; use std::time::Duration; fn make_effective( @@ -803,4 +812,74 @@ mod tests { ); assert_eq!(ec4.scrollback_lines(), DEFAULT_SCROLLBACK_LINES); } + + // ── Part B: auth_mode ───────────────────────────────────────────────────── + + #[test] + fn auth_mode_default_is_keychain() { + let ec = make_effective( + FlagConfig::default(), + EnvSnapshot::empty(), + RepoConfig::default(), + GlobalConfig::default(), + ); + assert_eq!( + ec.auth_mode(), + AgentAuthMode::Keychain, + "default auth mode must be keychain" + ); + } + + #[test] + fn auth_mode_passthrough_from_repo() { + let repo = RepoConfig { + auth: Some(AgentAuthMode::Passthrough), + ..Default::default() + }; + let ec = make_effective( + FlagConfig::default(), + EnvSnapshot::empty(), + repo, + GlobalConfig::default(), + ); + assert_eq!( + ec.auth_mode(), + AgentAuthMode::Passthrough, + "passthrough set in repo config must be effective" + ); + } + + #[test] + fn auth_mode_none_from_repo() { + let repo = RepoConfig { + auth: Some(AgentAuthMode::None), + ..Default::default() + }; + let ec = make_effective( + FlagConfig::default(), + EnvSnapshot::empty(), + repo, + GlobalConfig::default(), + ); + assert_eq!( + ec.auth_mode(), + AgentAuthMode::None, + "none set in repo config must be effective" + ); + } + + #[test] + fn auth_mode_keychain_explicitly_set_in_repo() { + let repo = RepoConfig { + auth: Some(AgentAuthMode::Keychain), + ..Default::default() + }; + let ec = make_effective( + FlagConfig::default(), + EnvSnapshot::empty(), + repo, + GlobalConfig::default(), + ); + assert_eq!(ec.auth_mode(), AgentAuthMode::Keychain); + } } diff --git a/src/data/config/mod.rs b/src/data/config/mod.rs index c53e85e7..75da6d05 100644 --- a/src/data/config/mod.rs +++ b/src/data/config/mod.rs @@ -12,7 +12,8 @@ pub use env::{Env, EnvSnapshot}; pub use flags::FlagConfig; pub use global::GlobalConfig; pub use repo::{ - ApiConfig, RemoteConfig, RepoConfig, WorkItemsConfig, REPO_CONFIG_FILENAME, REPO_CONFIG_SUBDIR, + AgentAuthMode, ApiConfig, RemoteConfig, RepoConfig, WorkItemsConfig, REPO_CONFIG_FILENAME, + REPO_CONFIG_SUBDIR, }; /// Built-in default number of scrollback lines for the container terminal emulator. diff --git a/src/data/config/repo.rs b/src/data/config/repo.rs index eb205253..ca633a24 100644 --- a/src/data/config/repo.rs +++ b/src/data/config/repo.rs @@ -42,6 +42,27 @@ pub struct ApiConfig { pub always_non_interactive: Option, } +/// Per-harness credential injection mode. +/// +/// Controls how awman injects credentials into agent containers. The default +/// (`keychain`) injects whatever the host keychain resolves, subject to +/// injection-time deduplication (Part A). `passthrough` skips keychain +/// injection entirely — the harness must supply credentials via `env(VAR)` +/// overlays. `none` disables all credential injection. +/// +/// Resolved through `EffectiveConfig::auth_mode()`. +#[derive(Debug, Default, Clone, Copy, PartialEq, Eq, Serialize, Deserialize)] +#[serde(rename_all = "lowercase")] +pub enum AgentAuthMode { + /// Inject host keychain credentials (with injection-time dedup). Default. + #[default] + Keychain, + /// Skip keychain injection; rely on harness-declared `env()` overlays. + Passthrough, + /// No credential injection of any kind. + None, +} + /// Work-items configuration nested within `RepoConfig`. #[derive(Debug, Default, Clone, Serialize, Deserialize, PartialEq, Eq)] pub struct WorkItemsConfig { @@ -79,6 +100,10 @@ pub struct RepoConfig { pub base_image: Option, #[serde(rename = "dockerfile", skip_serializing_if = "Option::is_none")] pub dockerfile: Option, + /// Credential injection mode for agent containers. See [`AgentAuthMode`]. + /// Resolved per-harness; `keychain` (default) injects host keychain creds. + #[serde(rename = "auth", skip_serializing_if = "Option::is_none")] + pub auth: Option, } impl RepoConfig { diff --git a/src/engine/auth/mod.rs b/src/engine/auth/mod.rs index 75d31156..470dceb0 100644 --- a/src/engine/auth/mod.rs +++ b/src/engine/auth/mod.rs @@ -9,6 +9,7 @@ use ring::digest; use ring::rand::{SecureRandom, SystemRandom}; use subtle::ConstantTimeEq; +use crate::data::config::repo::AgentAuthMode; use crate::data::fs::api_paths::ApiPaths; use crate::data::fs::auth_paths::AuthPathResolver; use crate::data::session::{AgentName, Session}; @@ -16,6 +17,35 @@ use crate::engine::error::EngineError; pub mod keychain; +/// Map an awman credential env-var name to its well-known service name, or +/// `None` when awman has no mapping for it. +/// +/// This is the canonical source of the credential→service table. The Docker +/// Sandbox driver ([`crate::engine::sandbox::dsbx::auth`]) re-exports this +/// function rather than maintaining its own copy, so all runtimes share one +/// definition. Callers that need to detect "does this declared env var cover +/// the same auth service as a keychain credential?" use this function to +/// compare services by name rather than by hardcoded var pairs. +/// +/// Service names match the Docker Sandboxes well-known service table at +/// docs.docker.com/ai/sandboxes/security/credentials/ (June 2026). +pub fn service_for_credential(key: &str) -> Option<&'static str> { + match key { + // Anthropic — API key (all runtimes) and OAuth token (container runtimes; + // the sbx driver skips the OAuth token for a separate reason: sbx has no + // OAuth support yet, tracked at docker/sbx-releases#11). Both vars + // authenticate the same provider so both map to the same service here. + "ANTHROPIC_API_KEY" | "CLAUDE_CODE_OAUTH_TOKEN" => Some("anthropic"), + "OPENAI_API_KEY" => Some("openai"), + "GH_TOKEN" | "GITHUB_TOKEN" => Some("github"), + "GEMINI_API_KEY" | "GOOGLE_API_KEY" => Some("google"), + "AWS_ACCESS_KEY_ID" | "AWS_SECRET_ACCESS_KEY" => Some("aws"), + "GROQ_API_KEY" => Some("groq"), + "MISTRAL_API_KEY" => Some("mistral"), + _ => None, + } +} + /// Status of an agent's host-side credential discovery. #[derive(Debug, Clone, PartialEq, Eq)] pub struct AgentCredentialStatus { @@ -137,17 +167,25 @@ impl AuthEngine { }) } - /// Composite resolver: keychain credentials scoped to the per-repo config. + /// Composite resolver: keychain credentials gated on the harness `auth` mode. /// - /// The decision to *use* keychain credentials silently vs prompting is a - /// Layer 2 concern (governed by `auto_agent_auth_accepted`). This method - /// only resolves the credentials. + /// - `keychain` (default) — inject host keychain credentials. Part-A dedup + /// is applied later at `ResolvedContainerOptions::resolve` time. + /// - `passthrough` — skip keychain injection entirely; the harness must + /// supply credentials via `env(VAR)` overlays. + /// - `none` — no credential injection of any kind. + /// + /// The Layer-2 `auto_agent_auth_accepted` consent flag is a separate concern; + /// this method only resolves *which* credentials to inject. pub fn resolve_agent_auth( &self, - _session: &Session, + session: &Session, agent: &AgentName, ) -> Result { - self.agent_keychain_credentials(agent) + match session.effective_config().auth_mode() { + AgentAuthMode::Keychain => self.agent_keychain_credentials(agent), + AgentAuthMode::Passthrough | AgentAuthMode::None => Ok(AgentCredentials::default()), + } } // ── API-key lifecycle ────────────────────────────────────────────────── @@ -632,4 +670,131 @@ mod tests { let outcome = e.verify_api_key(&key).unwrap(); assert_eq!(outcome, AuthOutcome::Authorized); } + + // ── Part B: resolve_agent_auth auth-mode gating ─────────────────────────── + + use crate::data::config::env::AWMAN_CONFIG_HOME; + use crate::data::config::repo::{ + AgentAuthMode, RepoConfig, REPO_CONFIG_FILENAME, REPO_CONFIG_SUBDIR, + }; + use crate::data::config::EnvSnapshot; + use crate::data::session::{Session, SessionOpenOptions}; + + /// Build an isolated session with the given `RepoConfig` written to disk. + /// + /// Uses a temp home dir (via `AWMAN_CONFIG_HOME`) so no process-global env + /// is read or mutated. + fn session_with_repo_config( + git_root: &std::path::Path, + home_dir: &std::path::Path, + repo_cfg: &RepoConfig, + ) -> Session { + // Write repo config. + let awman_dir = git_root.join(REPO_CONFIG_SUBDIR); + std::fs::create_dir_all(&awman_dir).unwrap(); + let cfg_json = serde_json::to_string_pretty(repo_cfg).unwrap(); + std::fs::write(awman_dir.join(REPO_CONFIG_FILENAME), cfg_json).unwrap(); + + let env = EnvSnapshot::with_overrides([(AWMAN_CONFIG_HOME, home_dir.to_str().unwrap())]); + + let resolver = crate::data::session::StaticGitRootResolver::new(git_root); + Session::open( + git_root.to_path_buf(), + &resolver, + SessionOpenOptions { + env: Some(env), + ..Default::default() + }, + ) + .unwrap() + } + + /// `passthrough` mode: `resolve_agent_auth` returns an empty credential set + /// without touching the host keychain (hermetic even on macOS). + #[test] + fn passthrough_mode_yields_empty_credentials() { + let git_tmp = tempfile::tempdir().unwrap(); + let home_tmp = tempfile::tempdir().unwrap(); + let session = session_with_repo_config( + git_tmp.path(), + home_tmp.path(), + &RepoConfig { + auth: Some(AgentAuthMode::Passthrough), + ..Default::default() + }, + ); + let engine = engine_with(home_tmp.path(), home_tmp.path()); + let agent = crate::data::session::AgentName::new("claude").unwrap(); + let creds = engine.resolve_agent_auth(&session, &agent).unwrap(); + assert!( + creds.env_vars.is_empty(), + "passthrough mode must return no keychain credentials; got: {:?}", + creds.env_vars + ); + } + + /// `none` mode: same result — no credential injection. + #[test] + fn none_mode_yields_empty_credentials() { + let git_tmp = tempfile::tempdir().unwrap(); + let home_tmp = tempfile::tempdir().unwrap(); + let session = session_with_repo_config( + git_tmp.path(), + home_tmp.path(), + &RepoConfig { + auth: Some(AgentAuthMode::None), + ..Default::default() + }, + ); + let engine = engine_with(home_tmp.path(), home_tmp.path()); + let agent = crate::data::session::AgentName::new("claude").unwrap(); + let creds = engine.resolve_agent_auth(&session, &agent).unwrap(); + assert!( + creds.env_vars.is_empty(), + "none mode must return no keychain credentials; got: {:?}", + creds.env_vars + ); + } + + /// `keychain` explicitly set: same as default — attempt keychain lookup + /// (may be empty on CI without a keychain; the important thing is that it + /// does NOT return early with empty credentials the way passthrough/none do). + /// We verify via the successful `Ok` return, not the credential count. + #[test] + fn keychain_mode_attempts_keychain_lookup() { + let git_tmp = tempfile::tempdir().unwrap(); + let home_tmp = tempfile::tempdir().unwrap(); + let session = session_with_repo_config( + git_tmp.path(), + home_tmp.path(), + &RepoConfig { + auth: Some(AgentAuthMode::Keychain), + ..Default::default() + }, + ); + let engine = engine_with(home_tmp.path(), home_tmp.path()); + let agent = crate::data::session::AgentName::new("claude").unwrap(); + // Must not error regardless of whether the keychain has credentials. + let result = engine.resolve_agent_auth(&session, &agent); + assert!( + result.is_ok(), + "keychain mode must not error when keychain is absent; got: {result:?}" + ); + } + + /// Default (no `auth` field in repo config): same as `keychain`. + #[test] + fn default_mode_behaves_like_keychain() { + let git_tmp = tempfile::tempdir().unwrap(); + let home_tmp = tempfile::tempdir().unwrap(); + let session = + session_with_repo_config(git_tmp.path(), home_tmp.path(), &RepoConfig::default()); + let engine = engine_with(home_tmp.path(), home_tmp.path()); + let agent = crate::data::session::AgentName::new("claude").unwrap(); + let result = engine.resolve_agent_auth(&session, &agent); + assert!( + result.is_ok(), + "default mode must not error; got: {result:?}" + ); + } } diff --git a/src/engine/container/options.rs b/src/engine/container/options.rs index 54db5cf6..db153335 100644 --- a/src/engine/container/options.rs +++ b/src/engine/container/options.rs @@ -196,6 +196,50 @@ pub enum ContainerOption { }, } +/// Injection-time dedup: drop any entry from `agent_credentials` whose +/// credential key maps to the same provider service as a key already declared +/// in `env_passthrough` or `env_literal`. +/// +/// Mirrors the rationale of the sbx path's `CLAUDE_CODE_OAUTH_TOKEN` silent +/// skip ([`crate::engine::sandbox::dsbx::auth::inject_credentials`]): +/// when the harness has **declared** an env var (via `env(VAR)`) that already +/// authenticates the same provider, the keychain OAuth token is redundant and +/// its presence causes the container to receive two conflicting credentials for +/// the same service. Keying on declared env — never `std::env::var` — so cloud +/// harnesses that have no declared anthropic var continue to receive keychain +/// OAuth unaffected. +/// +/// Example: harness declares `env(ANTHROPIC_API_KEY)` → service "anthropic"; +/// keychain resolves `CLAUDE_CODE_OAUTH_TOKEN` → also service "anthropic". +/// Result: `CLAUDE_CODE_OAUTH_TOKEN` is dropped from `agent_credentials`. +pub(crate) fn dedup_credentials_by_declared_env( + agent_credentials: &mut Vec<(String, String)>, + env_passthrough: &[EnvVar], + env_literal: &[EnvLiteral], +) { + // Collect the set of provider services already covered by declared env vars. + let covered_services: Vec<&'static str> = env_passthrough + .iter() + .map(|v| v.0.as_str()) + .chain(env_literal.iter().map(|l| l.key.as_str())) + .filter_map(crate::engine::auth::service_for_credential) + .collect(); + + if covered_services.is_empty() { + return; + } + + // Retain only credentials whose service is NOT already covered by a + // harness-declared env var. + agent_credentials.retain(|(key, _)| { + match crate::engine::auth::service_for_credential(key) { + Some(service) => !covered_services.contains(&service), + // Credential with no known service mapping — retain unconditionally. + None => true, + } + }); +} + /// Resolved option bag — all options merged into a single struct that the /// backend consumes. Conflicting options are detected here. #[derive(Debug, Clone, Default)] @@ -247,6 +291,13 @@ impl ResolvedContainerOptions { for opt in options { r.ingest(opt)?; } + // Part A: drop agent_credentials that duplicate a service already covered + // by a harness-declared env var. Applies to ALL container runtimes. + dedup_credentials_by_declared_env( + &mut r.agent_credentials, + &r.env_passthrough, + &r.env_literal, + ); r.validate()?; Ok(r) } @@ -397,4 +448,173 @@ mod tests { // Multiple overlay entries accumulate — dedup is caller's responsibility. assert_eq!(resolved.overlays.len(), 3); } + + // ── Part A: injection-time credential dedup ─────────────────────────────── + + /// When the harness declares ANTHROPIC_API_KEY via env_passthrough, the + /// keychain CLAUDE_CODE_OAUTH_TOKEN (same "anthropic" service) must be + /// dropped from agent_credentials — only one set of anthropic creds reaches + /// the container. + #[test] + fn declared_anthropic_env_passthrough_drops_oauth_token_from_agent_credentials() { + let resolved = ResolvedContainerOptions::resolve([ + // Harness explicitly declared ANTHROPIC_API_KEY via env(VAR) overlay. + ContainerOption::EnvPassthrough(EnvVar("ANTHROPIC_API_KEY".into())), + // Keychain resolved the OAuth token for the same provider. + ContainerOption::AgentCredentials { + env_vars: vec![("CLAUDE_CODE_OAUTH_TOKEN".into(), "sk-ant-oat-secret".into())], + }, + ]) + .expect("resolve must succeed"); + + assert!( + resolved.agent_credentials.is_empty(), + "CLAUDE_CODE_OAUTH_TOKEN maps to service 'anthropic', same as \ + ANTHROPIC_API_KEY — it must be dropped; got: {:?}", + resolved.agent_credentials + ); + // The declared passthrough must be kept. + assert!( + resolved + .env_passthrough + .iter() + .any(|v| v.0 == "ANTHROPIC_API_KEY"), + "ANTHROPIC_API_KEY must remain in env_passthrough" + ); + } + + /// No declared anthropic var → cloud harness path — keychain OAuth is + /// retained (dedup must NOT fire when the declared env is absent). + #[test] + fn no_declared_anthropic_var_retains_oauth_token_in_agent_credentials() { + let resolved = ResolvedContainerOptions::resolve([ + // Harness declared a non-anthropic var (e.g. OpenAI for a different agent). + ContainerOption::EnvPassthrough(EnvVar("OPENAI_API_KEY".into())), + ContainerOption::AgentCredentials { + env_vars: vec![("CLAUDE_CODE_OAUTH_TOKEN".into(), "sk-ant-oat-secret".into())], + }, + ]) + .expect("resolve must succeed"); + + assert!( + resolved + .agent_credentials + .iter() + .any(|(k, _)| k == "CLAUDE_CODE_OAUTH_TOKEN"), + "no declared anthropic env var → OAuth token must be retained; \ + got: {:?}", + resolved.agent_credentials + ); + } + + /// Declared via env_literal (not env_passthrough) also triggers the dedup. + #[test] + fn declared_anthropic_env_literal_drops_oauth_token_from_agent_credentials() { + let resolved = ResolvedContainerOptions::resolve([ + ContainerOption::EnvLiteral(EnvLiteral { + key: "ANTHROPIC_API_KEY".into(), + value: "sk-ant-key-literal".into(), + }), + ContainerOption::AgentCredentials { + env_vars: vec![("CLAUDE_CODE_OAUTH_TOKEN".into(), "sk-ant-oat-secret".into())], + }, + ]) + .expect("resolve must succeed"); + + assert!( + resolved.agent_credentials.is_empty(), + "ANTHROPIC_API_KEY via env_literal must still trigger dedup; \ + got: {:?}", + resolved.agent_credentials + ); + } + + /// A credential with no known service mapping (e.g. a custom env var + /// injected via agent_credentials) is never dropped by the dedup. + #[test] + fn unmapped_credential_is_never_dropped_by_dedup() { + let resolved = ResolvedContainerOptions::resolve([ + ContainerOption::EnvPassthrough(EnvVar("ANTHROPIC_API_KEY".into())), + ContainerOption::AgentCredentials { + env_vars: vec![ + ("CLAUDE_CODE_OAUTH_TOKEN".into(), "sk-ant-oat".into()), + ("MY_CUSTOM_INTERNAL_TOKEN".into(), "custom-value".into()), + ], + }, + ]) + .expect("resolve must succeed"); + + // OAuth token (anthropic) dropped; custom token (no mapping) retained. + assert!( + !resolved + .agent_credentials + .iter() + .any(|(k, _)| k == "CLAUDE_CODE_OAUTH_TOKEN"), + "OAuth token must be dropped when anthropic is declared" + ); + assert!( + resolved + .agent_credentials + .iter() + .any(|(k, _)| k == "MY_CUSTOM_INTERNAL_TOKEN"), + "unmapped credential must survive dedup; got: {:?}", + resolved.agent_credentials + ); + } + + /// When agent_credentials is empty the dedup is a no-op (no panic, no error). + #[test] + fn dedup_with_empty_agent_credentials_is_noop() { + let resolved = ResolvedContainerOptions::resolve([ContainerOption::EnvPassthrough( + EnvVar("ANTHROPIC_API_KEY".into()), + )]) + .expect("resolve must succeed"); + assert!(resolved.agent_credentials.is_empty()); + } + + // ── Part A: dedup_credentials_by_declared_env unit tests ───────────────── + + #[test] + fn dedup_fn_drops_oauth_when_anthropic_passthrough_declared() { + let mut creds = vec![("CLAUDE_CODE_OAUTH_TOKEN".into(), "tok".into())]; + let pt = vec![EnvVar("ANTHROPIC_API_KEY".into())]; + dedup_credentials_by_declared_env(&mut creds, &pt, &[]); + assert!( + creds.is_empty(), + "CLAUDE_CODE_OAUTH_TOKEN must be dropped; got: {creds:?}" + ); + } + + #[test] + fn dedup_fn_retains_oauth_when_no_anthropic_declared() { + let mut creds = vec![("CLAUDE_CODE_OAUTH_TOKEN".into(), "tok".into())]; + let pt = vec![EnvVar("OPENAI_API_KEY".into())]; // covers openai, not anthropic + dedup_credentials_by_declared_env(&mut creds, &pt, &[]); + assert_eq!( + creds.len(), + 1, + "no anthropic declared → OAuth must be retained" + ); + } + + #[test] + fn dedup_fn_handles_env_literal_source() { + let mut creds = vec![("CLAUDE_CODE_OAUTH_TOKEN".into(), "tok".into())]; + let lit = vec![EnvLiteral { + key: "ANTHROPIC_API_KEY".into(), + value: "literal-key".into(), + }]; + dedup_credentials_by_declared_env(&mut creds, &[], &lit); + assert!( + creds.is_empty(), + "env_literal coverage must also trigger dedup" + ); + } + + #[test] + fn dedup_fn_retains_credential_when_no_declared_vars() { + let mut creds = vec![("CLAUDE_CODE_OAUTH_TOKEN".into(), "tok".into())]; + dedup_credentials_by_declared_env(&mut creds, &[], &[]); + assert_eq!(creds.len(), 1, "no declared vars → no dedup"); + } } diff --git a/src/engine/sandbox/dsbx/auth.rs b/src/engine/sandbox/dsbx/auth.rs index 4b1a5792..805c4876 100644 --- a/src/engine/sandbox/dsbx/auth.rs +++ b/src/engine/sandbox/dsbx/auth.rs @@ -17,22 +17,10 @@ use crate::engine::container::options::{EnvLiteral, EnvVar}; use crate::engine::error::EngineError; use crate::engine::sandbox::dsbx::spawn::SbxCommand; -/// Map an awman credential env-var name to its `sbx` well-known service name, -/// or `None` when awman has no mapping for it. Matches the service table in -/// the Docker Sandboxes credentials docs -/// (docs.docker.com/ai/sandboxes/security/credentials/, June 2026). -pub(super) fn service_for_credential(key: &str) -> Option<&'static str> { - match key { - "ANTHROPIC_API_KEY" => Some("anthropic"), - "OPENAI_API_KEY" => Some("openai"), - "GH_TOKEN" | "GITHUB_TOKEN" => Some("github"), - "GEMINI_API_KEY" | "GOOGLE_API_KEY" => Some("google"), - "AWS_ACCESS_KEY_ID" | "AWS_SECRET_ACCESS_KEY" => Some("aws"), - "GROQ_API_KEY" => Some("groq"), - "MISTRAL_API_KEY" => Some("mistral"), - _ => None, - } -} +/// Re-export the shared service→credential mapping so the dsbx module (and its +/// tests via `use super::*`) can use it without a long path. The canonical +/// definition lives in [`crate::engine::auth::service_for_credential`]. +pub(super) use crate::engine::auth::service_for_credential; /// Allowlist of provider auth env vars accepted via `env(VAR)` overlays for /// launch-time auto-auth, per agent. Only mixin-kit agents participate — @@ -368,9 +356,12 @@ fn listing_mentions_service(stdout: &str, service: &str) -> bool { /// `CLAUDE_CODE_OAUTH_TOKEN` is the one silent exception: the keychain /// resolver always surfaces it when the user is logged in to Claude Code, but /// sbx has no OAuth-token support yet (docker/sbx-releases#11), so it can -/// never be injected. Auth still works without it — via an `ANTHROPIC_API_KEY` -/// env() overlay ([`auto_auth_env_overlays`]) or `/login` inside the sandbox — -/// and the missing-auth case already gets its own warning from +/// never be injected. The service mapping in [`service_for_credential`] maps +/// it to "anthropic" for dedup purposes on other runtimes — but the sbx +/// driver skips it unconditionally (key-name check before the service lookup). +/// Auth still works without it — via an `ANTHROPIC_API_KEY` env() overlay +/// ([`auto_auth_env_overlays`]) or `/login` inside the sandbox — and the +/// missing-auth case already gets its own warning from /// [`auto_auth_env_overlays`], so warning here on every launch is pure noise. pub(super) fn inject_credentials( creds: &[(String, String)], @@ -378,13 +369,18 @@ pub(super) fn inject_credentials( sink: &mut dyn UserMessageSink, ) -> Result<(), EngineError> { for (key, value) in creds { + // Sbx has no OAuth-token support yet; skip unconditionally. See the + // doc comment above. Key-name check takes precedence over the service + // mapping so this remains correct even though service_for_credential + // now maps CLAUDE_CODE_OAUTH_TOKEN → "anthropic" for dedup use by + // other runtimes. + if key == "CLAUDE_CODE_OAUTH_TOKEN" { + continue; + } match service_for_credential(key) { Some(service) => { set_secret(service, value, sandbox, sink)?; } - None if key == "CLAUDE_CODE_OAUTH_TOKEN" => { - // Silently skipped — see the doc comment above. - } None => { sink.write_message(crate::data::message::UserMessage { level: crate::data::message::MessageLevel::Warning, @@ -411,6 +407,13 @@ mod tests { service_for_credential("ANTHROPIC_API_KEY"), Some("anthropic") ); + // OAuth token maps to anthropic for injection-time dedup on container + // runtimes. The sbx driver skips it via a key-name guard before + // reaching this table (sbx has no OAuth support yet). + assert_eq!( + service_for_credential("CLAUDE_CODE_OAUTH_TOKEN"), + Some("anthropic") + ); assert_eq!(service_for_credential("OPENAI_API_KEY"), Some("openai")); assert_eq!(service_for_credential("GH_TOKEN"), Some("github")); assert_eq!(service_for_credential("GITHUB_TOKEN"), Some("github")); @@ -534,6 +537,9 @@ mod tests { fn all_credential_table_entries_have_expected_service_names() { let table: &[(&str, &str)] = &[ ("ANTHROPIC_API_KEY", "anthropic"), + // OAuth token: maps to anthropic for dedup; sbx skips it via + // key-name guard before reaching this table. + ("CLAUDE_CODE_OAUTH_TOKEN", "anthropic"), ("OPENAI_API_KEY", "openai"), ("GH_TOKEN", "github"), ("GITHUB_TOKEN", "github"), diff --git a/src/frontend/tui/render.rs b/src/frontend/tui/render.rs index b328e4de..5c56483e 100644 --- a/src/frontend/tui/render.rs +++ b/src/frontend/tui/render.rs @@ -1187,7 +1187,7 @@ fn render_dialog(dialog: &dialogs::Dialog, area: Rect, frame: &mut Frame) { let visible = list_h as usize; let start = selected .saturating_sub(visible.saturating_sub(1)) - .min(items.len().saturating_sub(visible).max(0)); + .min(items.len().saturating_sub(visible)); let lines: Vec = items .iter() .enumerate() From d4865ecb5e31d528d05cf4de0333139d9bbf795f Mon Sep 17 00:00:00 2001 From: Vinnie Mazza Date: Tue, 16 Jun 2026 19:31:01 -0400 Subject: [PATCH 2/2] fix(auth): gate credential dedup on resolvable declared env + warn on last-credential drop; doc precision --- docs/07-configuration.md | 6 +- src/data/config/effective.rs | 9 +- src/data/config/repo.rs | 12 +- src/engine/auth/mod.rs | 10 +- src/engine/container/options.rs | 310 ++++++++++++++++++++------------ 5 files changed, 223 insertions(+), 124 deletions(-) diff --git a/docs/07-configuration.md b/docs/07-configuration.md index b649f5a1..417d5dd8 100644 --- a/docs/07-configuration.md +++ b/docs/07-configuration.md @@ -180,9 +180,9 @@ credentials through other means: | Value | Behaviour | |-------|-----------| -| `keychain` (default) | Inject host keychain credentials. When the harness also declares `env(ANTHROPIC_API_KEY)` (or another credential that covers the same provider), the keychain OAuth token for that provider is automatically suppressed at injection time — the container receives exactly one set of credentials per provider. | -| `passthrough` | Skip keychain injection entirely. Supply credentials via `env(VAR)` overlays; awman never injects anything from the keychain. | -| `none` | No credential injection at all. | +| `keychain` (default) | Inject host keychain credentials. When the repo also declares `env(ANTHROPIC_API_KEY)` (or another credential that covers the same provider) **and that variable is set on the host**, the keychain OAuth token for that provider is automatically suppressed at injection time — the container receives exactly one set of credentials per provider. If the declared passthrough var is not set on the host, the keychain credential is retained so the container is not left with zero credentials for that provider. | +| `passthrough` | No KEYCHAIN credential injection; declared `env()` overlays still apply. Supply credentials via `env(VAR)` overlays. | +| `none` | No KEYCHAIN credential injection; declared `env()` overlays still apply. | Set `auth` in `.awman/config.json` directly (it is not settable via `config set`). The field is per-repo only — cloud harnesses that do not declare an anthropic env var remain on the default `keychain` path and continue to receive keychain OAuth unaffected. diff --git a/src/data/config/effective.rs b/src/data/config/effective.rs index 9e38674c..159153e8 100644 --- a/src/data/config/effective.rs +++ b/src/data/config/effective.rs @@ -209,9 +209,14 @@ impl EffectiveConfig { /// Effective credential injection mode (repo > built-in default `keychain`). /// - /// `passthrough` skips keychain injection entirely; `none` disables all + /// `passthrough` skips keychain injection entirely; `none` disables keychain /// injection; `keychain` (default) injects host keychain creds, subject to - /// injection-time dedup of any harness-declared env vars. + /// injection-time dedup of any repo-declared env vars. + /// + /// Single-layer resolution (repo only, no global flag override) is a + /// deliberate conservative choice: a credential toggle must be an explicit + /// per-repo decision, not silently inherited from a global default that the + /// operator may not realize is set. pub fn auth_mode(&self) -> AgentAuthMode { self.repo.auth.unwrap_or_default() } diff --git a/src/data/config/repo.rs b/src/data/config/repo.rs index ca633a24..fe3e1a6f 100644 --- a/src/data/config/repo.rs +++ b/src/data/config/repo.rs @@ -42,13 +42,14 @@ pub struct ApiConfig { pub always_non_interactive: Option, } -/// Per-harness credential injection mode. +/// Per-repo credential injection mode. /// /// Controls how awman injects credentials into agent containers. The default /// (`keychain`) injects whatever the host keychain resolves, subject to /// injection-time deduplication (Part A). `passthrough` skips keychain /// injection entirely — the harness must supply credentials via `env(VAR)` -/// overlays. `none` disables all credential injection. +/// overlays. `none` disables KEYCHAIN credential injection; declared `env()` +/// overlays still apply. /// /// Resolved through `EffectiveConfig::auth_mode()`. #[derive(Debug, Default, Clone, Copy, PartialEq, Eq, Serialize, Deserialize)] @@ -57,9 +58,10 @@ pub enum AgentAuthMode { /// Inject host keychain credentials (with injection-time dedup). Default. #[default] Keychain, - /// Skip keychain injection; rely on harness-declared `env()` overlays. + /// Skip KEYCHAIN injection; rely on repo-declared `env()` overlays. + /// Declared `env()` overlays still apply. Passthrough, - /// No credential injection of any kind. + /// No KEYCHAIN credential injection; declared `env()` overlays still apply. None, } @@ -101,7 +103,7 @@ pub struct RepoConfig { #[serde(rename = "dockerfile", skip_serializing_if = "Option::is_none")] pub dockerfile: Option, /// Credential injection mode for agent containers. See [`AgentAuthMode`]. - /// Resolved per-harness; `keychain` (default) injects host keychain creds. + /// Resolved per-repo; `keychain` (default) injects host keychain creds. #[serde(rename = "auth", skip_serializing_if = "Option::is_none")] pub auth: Option, } diff --git a/src/engine/auth/mod.rs b/src/engine/auth/mod.rs index 470dceb0..991379c0 100644 --- a/src/engine/auth/mod.rs +++ b/src/engine/auth/mod.rs @@ -167,13 +167,15 @@ impl AuthEngine { }) } - /// Composite resolver: keychain credentials gated on the harness `auth` mode. + /// Composite resolver: keychain credentials gated on the repo `auth` mode. /// /// - `keychain` (default) — inject host keychain credentials. Part-A dedup /// is applied later at `ResolvedContainerOptions::resolve` time. - /// - `passthrough` — skip keychain injection entirely; the harness must - /// supply credentials via `env(VAR)` overlays. - /// - `none` — no credential injection of any kind. + /// - `passthrough` — skip keychain injection entirely; the repo must + /// supply credentials via `env(VAR)` overlays. Declared `env()` overlays + /// still apply. + /// - `none` — no KEYCHAIN credential injection; declared `env()` overlays + /// still apply. /// /// The Layer-2 `auto_agent_auth_accepted` consent flag is a separate concern; /// this method only resolves *which* credentials to inject. diff --git a/src/engine/container/options.rs b/src/engine/container/options.rs index db153335..34c9f511 100644 --- a/src/engine/container/options.rs +++ b/src/engine/container/options.rs @@ -198,28 +198,49 @@ pub enum ContainerOption { /// Injection-time dedup: drop any entry from `agent_credentials` whose /// credential key maps to the same provider service as a key already declared -/// in `env_passthrough` or `env_literal`. +/// in `env_passthrough` or `env_literal`, **and whose host value is actually +/// resolvable**. /// /// Mirrors the rationale of the sbx path's `CLAUDE_CODE_OAUTH_TOKEN` silent /// skip ([`crate::engine::sandbox::dsbx::auth::inject_credentials`]): /// when the harness has **declared** an env var (via `env(VAR)`) that already /// authenticates the same provider, the keychain OAuth token is redundant and /// its presence causes the container to receive two conflicting credentials for -/// the same service. Keying on declared env — never `std::env::var` — so cloud -/// harnesses that have no declared anthropic var continue to receive keychain -/// OAuth unaffected. +/// the same service. +/// +/// An `env_passthrough` entry only counts as "covering" a service when its host +/// value is actually set — mirroring `build_run_argv`'s own emission gate +/// (`if let Ok(value) = std::env::var(name)`). A declared but unset passthrough +/// var does NOT suppress a keychain credential, because the passthrough will emit +/// nothing and the container would otherwise receive zero credentials for that +/// service. `env_literal` entries always carry a value and always count. +/// +/// `lookup_env` abstracts the host env lookup so that callers in tests can +/// supply a hermetic closure instead of reading `std::env::var` directly. +/// +/// If dedup would drop the **last** remaining credential for a service, a +/// `log::warn!` is emitted so the situation is never silent. /// /// Example: harness declares `env(ANTHROPIC_API_KEY)` → service "anthropic"; -/// keychain resolves `CLAUDE_CODE_OAUTH_TOKEN` → also service "anthropic". -/// Result: `CLAUDE_CODE_OAUTH_TOKEN` is dropped from `agent_credentials`. +/// keychain resolves `CLAUDE_CODE_OAUTH_TOKEN` → also service "anthropic"; +/// host has `ANTHROPIC_API_KEY` set → the passthrough will be emitted → +/// `CLAUDE_CODE_OAUTH_TOKEN` is dropped from `agent_credentials`. +/// +/// Counter-example: same declaration but `ANTHROPIC_API_KEY` is NOT set on the +/// host → the passthrough emits nothing → `CLAUDE_CODE_OAUTH_TOKEN` is retained. pub(crate) fn dedup_credentials_by_declared_env( agent_credentials: &mut Vec<(String, String)>, env_passthrough: &[EnvVar], env_literal: &[EnvLiteral], + lookup_env: &dyn Fn(&str) -> Option, ) { - // Collect the set of provider services already covered by declared env vars. + // Collect the set of provider services already covered by declared env vars + // that will actually be emitted to the container: + // - env_passthrough: only when the host value is set (and non-empty). + // - env_literal: always (they carry an explicit value). let covered_services: Vec<&'static str> = env_passthrough .iter() + .filter(|v| lookup_env(v.0.as_str()).is_some_and(|val| !val.is_empty())) .map(|v| v.0.as_str()) .chain(env_literal.iter().map(|l| l.key.as_str())) .filter_map(crate::engine::auth::service_for_credential) @@ -229,8 +250,30 @@ pub(crate) fn dedup_credentials_by_declared_env( return; } + // Before dropping, check whether this dedup would leave any service with + // zero credentials. If so, warn — the outcome is intentional (the literal + // or resolvable passthrough will cover it), but it should never be silent. + for service in &covered_services { + let service_creds_before: Vec<_> = agent_credentials + .iter() + .filter(|(k, _)| crate::engine::auth::service_for_credential(k) == Some(service)) + .collect(); + if !service_creds_before.is_empty() { + tracing::warn!( + dropped_keys = ?service_creds_before + .iter() + .map(|(k, _)| k.as_str()) + .collect::>(), + service = service, + "awman: dropping keychain credential(s) for service because the repo \ + declared an env var that covers the same provider; the container will \ + receive credentials via the declared env overlay.", + ); + } + } + // Retain only credentials whose service is NOT already covered by a - // harness-declared env var. + // harness-declared env var that will be emitted to the container. agent_credentials.retain(|(key, _)| { match crate::engine::auth::service_for_credential(key) { Some(service) => !covered_services.contains(&service), @@ -293,10 +336,13 @@ impl ResolvedContainerOptions { } // Part A: drop agent_credentials that duplicate a service already covered // by a harness-declared env var. Applies to ALL container runtimes. + // Production callers pass the real host-env lookup; tests inject a + // hermetic closure to avoid mutating process-global state. dedup_credentials_by_declared_env( &mut r.agent_credentials, &r.env_passthrough, &r.env_literal, + &|name| std::env::var(name).ok(), ); r.validate()?; Ok(r) @@ -449,65 +495,11 @@ mod tests { assert_eq!(resolved.overlays.len(), 3); } - // ── Part A: injection-time credential dedup ─────────────────────────────── + // ── Part A: injection-time credential dedup (resolve-level, env_literal path) ── - /// When the harness declares ANTHROPIC_API_KEY via env_passthrough, the - /// keychain CLAUDE_CODE_OAUTH_TOKEN (same "anthropic" service) must be - /// dropped from agent_credentials — only one set of anthropic creds reaches - /// the container. - #[test] - fn declared_anthropic_env_passthrough_drops_oauth_token_from_agent_credentials() { - let resolved = ResolvedContainerOptions::resolve([ - // Harness explicitly declared ANTHROPIC_API_KEY via env(VAR) overlay. - ContainerOption::EnvPassthrough(EnvVar("ANTHROPIC_API_KEY".into())), - // Keychain resolved the OAuth token for the same provider. - ContainerOption::AgentCredentials { - env_vars: vec![("CLAUDE_CODE_OAUTH_TOKEN".into(), "sk-ant-oat-secret".into())], - }, - ]) - .expect("resolve must succeed"); - - assert!( - resolved.agent_credentials.is_empty(), - "CLAUDE_CODE_OAUTH_TOKEN maps to service 'anthropic', same as \ - ANTHROPIC_API_KEY — it must be dropped; got: {:?}", - resolved.agent_credentials - ); - // The declared passthrough must be kept. - assert!( - resolved - .env_passthrough - .iter() - .any(|v| v.0 == "ANTHROPIC_API_KEY"), - "ANTHROPIC_API_KEY must remain in env_passthrough" - ); - } - - /// No declared anthropic var → cloud harness path — keychain OAuth is - /// retained (dedup must NOT fire when the declared env is absent). - #[test] - fn no_declared_anthropic_var_retains_oauth_token_in_agent_credentials() { - let resolved = ResolvedContainerOptions::resolve([ - // Harness declared a non-anthropic var (e.g. OpenAI for a different agent). - ContainerOption::EnvPassthrough(EnvVar("OPENAI_API_KEY".into())), - ContainerOption::AgentCredentials { - env_vars: vec![("CLAUDE_CODE_OAUTH_TOKEN".into(), "sk-ant-oat-secret".into())], - }, - ]) - .expect("resolve must succeed"); - - assert!( - resolved - .agent_credentials - .iter() - .any(|(k, _)| k == "CLAUDE_CODE_OAUTH_TOKEN"), - "no declared anthropic env var → OAuth token must be retained; \ - got: {:?}", - resolved.agent_credentials - ); - } - - /// Declared via env_literal (not env_passthrough) also triggers the dedup. + /// `env_literal` (always-valued) coverage → same-service keychain credential + /// is dropped. This goes through `resolve()` because env_literal entries + /// always have a value — no host lookup needed. #[test] fn declared_anthropic_env_literal_drops_oauth_token_from_agent_credentials() { let resolved = ResolvedContainerOptions::resolve([ @@ -529,67 +521,79 @@ mod tests { ); } - /// A credential with no known service mapping (e.g. a custom env var - /// injected via agent_credentials) is never dropped by the dedup. - #[test] - fn unmapped_credential_is_never_dropped_by_dedup() { - let resolved = ResolvedContainerOptions::resolve([ - ContainerOption::EnvPassthrough(EnvVar("ANTHROPIC_API_KEY".into())), - ContainerOption::AgentCredentials { - env_vars: vec![ - ("CLAUDE_CODE_OAUTH_TOKEN".into(), "sk-ant-oat".into()), - ("MY_CUSTOM_INTERNAL_TOKEN".into(), "custom-value".into()), - ], - }, - ]) - .expect("resolve must succeed"); - - // OAuth token (anthropic) dropped; custom token (no mapping) retained. - assert!( - !resolved - .agent_credentials - .iter() - .any(|(k, _)| k == "CLAUDE_CODE_OAUTH_TOKEN"), - "OAuth token must be dropped when anthropic is declared" - ); - assert!( - resolved - .agent_credentials - .iter() - .any(|(k, _)| k == "MY_CUSTOM_INTERNAL_TOKEN"), - "unmapped credential must survive dedup; got: {:?}", - resolved.agent_credentials - ); - } - /// When agent_credentials is empty the dedup is a no-op (no panic, no error). #[test] fn dedup_with_empty_agent_credentials_is_noop() { - let resolved = ResolvedContainerOptions::resolve([ContainerOption::EnvPassthrough( - EnvVar("ANTHROPIC_API_KEY".into()), - )]) - .expect("resolve must succeed"); + // Use env_literal so the test is hermetic (no host-env lookup). + let resolved = + ResolvedContainerOptions::resolve([ContainerOption::EnvLiteral(EnvLiteral { + key: "ANTHROPIC_API_KEY".into(), + value: "sk-key".into(), + })]) + .expect("resolve must succeed"); assert!(resolved.agent_credentials.is_empty()); } - // ── Part A: dedup_credentials_by_declared_env unit tests ───────────────── + // ── Part A: dedup_credentials_by_declared_env unit tests (injectable lookup) ── + // + // All of these pass a hermetic closure as `lookup_env` so no process-global + // std::env mutation is needed. The closure mimics the subset of env vars + // that would be set on the host in each scenario. + /// declared env(ANTHROPIC_API_KEY) that IS set on host + keychain OAuth → + /// OAuth dropped (the passthrough will be emitted; no need for two creds). #[test] - fn dedup_fn_drops_oauth_when_anthropic_passthrough_declared() { + fn dedup_fn_drops_oauth_when_anthropic_passthrough_set_on_host() { let mut creds = vec![("CLAUDE_CODE_OAUTH_TOKEN".into(), "tok".into())]; let pt = vec![EnvVar("ANTHROPIC_API_KEY".into())]; - dedup_credentials_by_declared_env(&mut creds, &pt, &[]); + // Simulate: ANTHROPIC_API_KEY is set on the host. + let lookup = |name: &str| -> Option { + if name == "ANTHROPIC_API_KEY" { + Some("sk-ant-api-key".into()) + } else { + None + } + }; + dedup_credentials_by_declared_env(&mut creds, &pt, &[], &lookup); assert!( creds.is_empty(), - "CLAUDE_CODE_OAUTH_TOKEN must be dropped; got: {creds:?}" + "OAuth must be dropped when ANTHROPIC_API_KEY is set on host; got: {creds:?}" + ); + } + + /// declared env(ANTHROPIC_API_KEY) that is NOT set on host + keychain OAuth → + /// OAuth retained (passthrough emits nothing; dropping it would leave zero + /// credentials for the 'anthropic' service). + #[test] + fn dedup_fn_retains_oauth_when_anthropic_passthrough_declared_but_unset() { + let mut creds = vec![("CLAUDE_CODE_OAUTH_TOKEN".into(), "tok".into())]; + let pt = vec![EnvVar("ANTHROPIC_API_KEY".into())]; + // Simulate: ANTHROPIC_API_KEY is NOT set on the host. + let lookup = |_name: &str| -> Option { None }; + dedup_credentials_by_declared_env(&mut creds, &pt, &[], &lookup); + assert_eq!( + creds.len(), + 1, + "OAuth must be retained when the declared passthrough var is unset on host; \ + got: {creds:?}" ); } + /// No declared anthropic var → cloud harness path — keychain OAuth is + /// retained regardless of host env. #[test] fn dedup_fn_retains_oauth_when_no_anthropic_declared() { let mut creds = vec![("CLAUDE_CODE_OAUTH_TOKEN".into(), "tok".into())]; let pt = vec![EnvVar("OPENAI_API_KEY".into())]; // covers openai, not anthropic - dedup_credentials_by_declared_env(&mut creds, &pt, &[]); + // Even if OPENAI_API_KEY is set, it doesn't cover the anthropic service. + let lookup = |name: &str| -> Option { + if name == "OPENAI_API_KEY" { + Some("sk-openai".into()) + } else { + None + } + }; + dedup_credentials_by_declared_env(&mut creds, &pt, &[], &lookup); assert_eq!( creds.len(), 1, @@ -597,6 +601,7 @@ mod tests { ); } + /// env_literal (always-valued) coverage → same-service credential dropped. #[test] fn dedup_fn_handles_env_literal_source() { let mut creds = vec![("CLAUDE_CODE_OAUTH_TOKEN".into(), "tok".into())]; @@ -604,17 +609,102 @@ mod tests { key: "ANTHROPIC_API_KEY".into(), value: "literal-key".into(), }]; - dedup_credentials_by_declared_env(&mut creds, &[], &lit); + // lookup_env is irrelevant for literals but must be provided. + let lookup = |_: &str| -> Option { None }; + dedup_credentials_by_declared_env(&mut creds, &[], &lit, &lookup); assert!( creds.is_empty(), "env_literal coverage must also trigger dedup" ); } + /// No declared vars → no dedup, regardless of host env. #[test] fn dedup_fn_retains_credential_when_no_declared_vars() { let mut creds = vec![("CLAUDE_CODE_OAUTH_TOKEN".into(), "tok".into())]; - dedup_credentials_by_declared_env(&mut creds, &[], &[]); + let lookup = |_: &str| -> Option { None }; + dedup_credentials_by_declared_env(&mut creds, &[], &[], &lookup); assert_eq!(creds.len(), 1, "no declared vars → no dedup"); } + + /// A credential with no known service mapping is never dropped by the dedup. + #[test] + fn dedup_fn_unmapped_credential_is_never_dropped() { + let mut creds = vec![ + ("CLAUDE_CODE_OAUTH_TOKEN".into(), "tok".into()), + ("MY_CUSTOM_INTERNAL_TOKEN".into(), "custom".into()), + ]; + let pt = vec![EnvVar("ANTHROPIC_API_KEY".into())]; + let lookup = |name: &str| -> Option { + if name == "ANTHROPIC_API_KEY" { + Some("sk-ant".into()) + } else { + None + } + }; + dedup_credentials_by_declared_env(&mut creds, &pt, &[], &lookup); + // OAuth (anthropic) dropped; custom (no mapping) retained. + assert!( + !creds.iter().any(|(k, _)| k == "CLAUDE_CODE_OAUTH_TOKEN"), + "OAuth must be dropped when anthropic passthrough is set" + ); + assert!( + creds.iter().any(|(k, _)| k == "MY_CUSTOM_INTERNAL_TOKEN"), + "unmapped credential must survive dedup; got: {creds:?}" + ); + } + + /// When keychain credential's OWN name equals the declared passthrough var + /// (e.g. declared env(ANTHROPIC_API_KEY) + keychain returns ANTHROPIC_API_KEY + /// itself), the keychain entry is dropped because both map to service + /// "anthropic" and the passthrough is set on host. + #[test] + fn dedup_fn_drops_when_credential_name_equals_declared_var() { + let mut creds = vec![("ANTHROPIC_API_KEY".into(), "sk-ant-from-keychain".into())]; + let pt = vec![EnvVar("ANTHROPIC_API_KEY".into())]; + // Passthrough is set on host (perhaps to a different value). + let lookup = |name: &str| -> Option { + if name == "ANTHROPIC_API_KEY" { + Some("sk-ant-from-env".into()) + } else { + None + } + }; + dedup_credentials_by_declared_env(&mut creds, &pt, &[], &lookup); + assert!( + creds.is_empty(), + "credential whose own name equals the declared var must be dropped; \ + got: {creds:?}" + ); + } + + /// Multi-service: declared var covers service X (anthropic), credential for + /// service Y (openai) is retained. + #[test] + fn dedup_fn_multi_service_declared_x_retains_credential_for_y() { + let mut creds = vec![ + ("CLAUDE_CODE_OAUTH_TOKEN".into(), "tok-anthropic".into()), + ("OPENAI_API_KEY".into(), "tok-openai".into()), + ]; + let pt = vec![EnvVar("ANTHROPIC_API_KEY".into())]; + // Only anthropic passthrough is set on host. + let lookup = |name: &str| -> Option { + if name == "ANTHROPIC_API_KEY" { + Some("sk-ant".into()) + } else { + None + } + }; + dedup_credentials_by_declared_env(&mut creds, &pt, &[], &lookup); + // Anthropic OAuth dropped; OpenAI retained (different service, not declared). + assert!( + !creds.iter().any(|(k, _)| k == "CLAUDE_CODE_OAUTH_TOKEN"), + "anthropic OAuth must be dropped" + ); + assert!( + creds.iter().any(|(k, _)| k == "OPENAI_API_KEY"), + "openai credential must be retained (covers service 'openai', not declared); \ + got: {creds:?}" + ); + } }