From af443088320a9136c17d7fbf90db0557cbf1a933 Mon Sep 17 00:00:00 2001 From: Can Yu Date: Wed, 24 Jun 2026 12:26:13 +0800 Subject: [PATCH 01/20] feat(native-agent): add Anthropic OAuth (Claude Pro/Max) login + provider Phase 1 of porting Pi's auth methods into openab-agent. Adds an `anthropic-oauth` tenant alongside Codex: PKCE browser/paste login against platform.claude.com, JSON token exchange + scope-less refresh, and an OAuth mode on AnthropicProvider (Bearer + Claude Code identity headers/system block, tool-name normalisation). Wires provider selection in acp.rs/llm.rs and a new `auth anthropic-oauth` CLI subcommand. Verified: cargo build clean (0 warnings), 194 tests pass incl. 4 new. Co-Authored-By: Claude Opus 4.8 (1M context) --- openab-agent/src/acp.rs | 28 ++-- openab-agent/src/auth.rs | 351 +++++++++++++++++++++++++++++++++------ openab-agent/src/llm.rs | 225 +++++++++++++++++++++---- openab-agent/src/main.rs | 12 ++ 4 files changed, 525 insertions(+), 91 deletions(-) diff --git a/openab-agent/src/acp.rs b/openab-agent/src/acp.rs index 1113c8ad4..0d866812f 100644 --- a/openab-agent/src/acp.rs +++ b/openab-agent/src/acp.rs @@ -302,10 +302,14 @@ impl AcpServer { let model_override = self.active_model.as_deref(); let (provider, active_provider): (Box, &str) = match provider_choice.as_str() { - "anthropic" => { - let res = match model_override { - Some(m) => AnthropicProvider::from_env_with_model(m), - None => AnthropicProvider::from_env(), + // `auto*` covers both ANTHROPIC_API_KEY and a stored Claude + // subscription OAuth token; `anthropic-oauth` forces the latter. + "anthropic" | "anthropic-oauth" | "claude" => { + let res = match (provider_choice.as_str(), model_override) { + ("anthropic", Some(m)) => AnthropicProvider::auto_with_model(m), + ("anthropic", None) => AnthropicProvider::auto(), + (_, Some(m)) => AnthropicProvider::from_oauth_store_with_model(m), + (_, None) => AnthropicProvider::from_oauth_store(), }; match res { Ok(p) => (Box::new(p), "anthropic"), @@ -323,10 +327,10 @@ impl AcpServer { } } _ => { - // Auto-detect: try API key first, then OAuth token + // Auto-detect: Anthropic (API key or OAuth) first, then codex. let anthropic_res = match model_override { - Some(m) => AnthropicProvider::from_env_with_model(m), - None => AnthropicProvider::from_env(), + Some(m) => AnthropicProvider::auto_with_model(m), + None => AnthropicProvider::auto(), }; match anthropic_res { Ok(p) => (Box::new(p), "anthropic"), @@ -343,7 +347,7 @@ impl AcpServer { return self.error_response( id, -32000, - &format!("No credentials: set ANTHROPIC_API_KEY or run `openab-agent auth codex-oauth`. {e}"), + &format!("No credentials: set ANTHROPIC_API_KEY, or run `openab-agent auth anthropic-oauth` / `auth codex-oauth`. {e}"), ) } } @@ -457,7 +461,9 @@ impl AcpServer { fn static_available_models() -> Vec { let mut models = Vec::new(); - if std::env::var("ANTHROPIC_API_KEY").is_ok() { + if std::env::var("ANTHROPIC_API_KEY").is_ok() + || crate::auth::load_tokens_for(crate::auth::ANTHROPIC_NAMESPACE).is_ok() + { models.extend(Self::static_anthropic_models()); } if crate::auth::load_tokens().is_ok() { @@ -598,7 +604,7 @@ impl AcpServer { let new_provider: Result, String> = match provider_name { "anthropic" => { - AnthropicProvider::from_env_with_model(value).map(|p| Box::new(p) as _) + AnthropicProvider::auto_with_model(value).map(|p| Box::new(p) as _) } _ => crate::llm::OpenAiProvider::from_auth_store_with_model(value) .map(|p| Box::new(p) as _), @@ -918,7 +924,7 @@ mod tests { // Insert a dummy session using anthropic key unsafe { std::env::set_var("ANTHROPIC_API_KEY", "test-key") }; - let provider = AnthropicProvider::from_env_with_model("claude-sonnet-4-20250514").unwrap(); + let provider = AnthropicProvider::auto_with_model("claude-sonnet-4-20250514").unwrap(); let agent = Agent::new_boxed(Box::new(provider), "/tmp".to_string(), None); server.sessions.insert("test-session".to_string(), agent); diff --git a/openab-agent/src/auth.rs b/openab-agent/src/auth.rs index d9a701237..ff71ad6ee 100644 --- a/openab-agent/src/auth.rs +++ b/openab-agent/src/auth.rs @@ -12,6 +12,8 @@ use std::time::{SystemTime, UNIX_EPOCH}; /// Namespace key for the existing Codex single-tenant credential. /// Lives next to future `mcp:` entries inside `auth.json`. const CODEX_NAMESPACE: &str = "codex"; +/// Namespace key for the Anthropic (Claude Pro/Max) OAuth credential. +pub const ANTHROPIC_NAMESPACE: &str = "anthropic-oauth"; const REFRESH_SKEW_SECONDS: u64 = 120; @@ -22,15 +24,45 @@ const CODEX_DEVICE_TOKEN_URL: &str = "https://auth.openai.com/api/accounts/devic const CODEX_DEVICE_REDIRECT_URI: &str = "https://auth.openai.com/deviceauth/callback"; const REDIRECT_PORT: u16 = 1455; +// Anthropic OAuth (Claude Pro/Max). Values mirror Claude Code's public client so +// `platform.claude.com` accepts the flow. Token bodies are JSON (Codex uses form) +// and the refresh body omits `scope` (Pi #2169). +const ANTHROPIC_AUTHORIZE_URL: &str = "https://claude.ai/oauth/authorize"; +const ANTHROPIC_TOKEN_URL: &str = "https://platform.claude.com/v1/oauth/token"; +const ANTHROPIC_REDIRECT_PORT: u16 = 53692; +const ANTHROPIC_SCOPE: &str = + "org:create_api_key user:profile user:inference user:sessions:claude_code user:mcp_servers user:file_upload"; + fn codex_client_id() -> String { std::env::var("OPENAB_AGENT_OAUTH_CLIENT_ID") .unwrap_or_else(|_| "app_EMoamEEZ73f0CkXaXp7hrann".to_string()) } +fn anthropic_client_id() -> String { + std::env::var("OPENAB_AGENT_ANTHROPIC_CLIENT_ID") + .unwrap_or_else(|_| "9d1c250a-e61b-44d9-88ed-5944d1962f5e".to_string()) +} + fn redirect_uri() -> String { format!("http://localhost:{REDIRECT_PORT}/auth/callback") } +fn anthropic_redirect_uri() -> String { + format!("http://localhost:{ANTHROPIC_REDIRECT_PORT}/callback") +} + +/// Build the Anthropic authorize URL. Pure so it can be unit-tested. Pi reuses +/// the PKCE verifier as the `state` value, so callers pass `state == verifier`. +fn anthropic_authorize_url(challenge: &str, state: &str) -> String { + let client_id = anthropic_client_id(); + let redirect = anthropic_redirect_uri(); + let redir = urlencoding::encode(&redirect); + let scope = urlencoding::encode(ANTHROPIC_SCOPE); + format!( + "{ANTHROPIC_AUTHORIZE_URL}?code=true&client_id={client_id}&response_type=code&redirect_uri={redir}&scope={scope}&code_challenge={challenge}&code_challenge_method=S256&state={state}" + ) +} + #[derive(Debug, Clone, Serialize, Deserialize)] pub struct TokenStore { pub access_token: String, @@ -214,30 +246,37 @@ fn write_auth_file(path: &Path, map: &HashMap) -> Result<()> Ok(()) } -pub fn load_tokens() -> Result { +/// Load the LLM token stored under `namespace` (`codex` / `anthropic-oauth`). +pub fn load_tokens_for(namespace: &str) -> Result { let path = auth_path(); let map = read_auth_file(&path).map_err(|_| { anyhow!( - "No credentials found at {}. Run `openab-agent auth codex-oauth` first.", + "No credentials found at {}. Run `openab-agent auth` first.", path.display() ) })?; - match map.get(CODEX_NAMESPACE) { + match map.get(namespace) { Some(AuthEntry::Token(t)) => Ok(t.clone()), _ => Err(anyhow!( - "No codex credentials in {}. Run `openab-agent auth codex-oauth` first.", + "No {namespace} credentials in {}. Run `openab-agent auth` first.", path.display() )), } } -fn save_tokens(store: &TokenStore) -> Result<()> { +/// Save a token under its own `provider` field as the namespace key, leaving +/// every other tenant in `auth.json` untouched. +fn save_tokens_for(store: &TokenStore) -> Result<()> { let path = auth_path(); let mut map = read_auth_file(&path).unwrap_or_default(); - map.insert(CODEX_NAMESPACE.to_string(), AuthEntry::Token(store.clone())); + map.insert(store.provider.clone(), AuthEntry::Token(store.clone())); write_auth_file(&path, &map) } +pub fn load_tokens() -> Result { + load_tokens_for(CODEX_NAMESPACE) +} + /// rmcp [`CredentialStore`] backed by the shared `auth.json` file (ADR §6.1 /// storage-format decision A). One instance is bound to a single MCP server's /// bare-name key (e.g. `linear`); rmcp's `AuthorizationManager` owns the @@ -328,38 +367,60 @@ impl CredentialStore for McpCredentialStore { } } -pub async fn get_valid_token() -> Result { - let mut store = load_tokens()?; +pub async fn get_valid_token_for(namespace: &str) -> Result { + let mut store = load_tokens_for(namespace)?; if store.is_expired() { store = refresh_token(&store).await?; - save_tokens(&store)?; + save_tokens_for(&store)?; } Ok(store.access_token) } -pub async fn force_refresh() -> Result { - let store = load_tokens()?; +pub async fn force_refresh_for(namespace: &str) -> Result { + let store = load_tokens_for(namespace)?; let new_store = refresh_token(&store).await?; - save_tokens(&new_store)?; + save_tokens_for(&new_store)?; Ok(new_store.access_token) } +pub async fn get_valid_token() -> Result { + get_valid_token_for(CODEX_NAMESPACE).await +} + +pub async fn force_refresh() -> Result { + force_refresh_for(CODEX_NAMESPACE).await +} + async fn refresh_token(store: &TokenStore) -> Result { - let client_id = codex_client_id(); let client = reqwest::Client::new(); - let resp = client - .post(&store.token_endpoint) - .form(&[ - ("grant_type", "refresh_token"), - ("refresh_token", store.refresh_token.as_str()), - ("client_id", client_id.as_str()), - ]) - .send() - .await?; + // Anthropic's token endpoint takes a JSON body and rejects a `scope` field + // on refresh (Pi #2169); Codex takes a form body. Branch on the stored + // provider so each tenant refreshes the way its AS expects. + let resp = if store.provider == ANTHROPIC_NAMESPACE { + client + .post(&store.token_endpoint) + .json(&serde_json::json!({ + "grant_type": "refresh_token", + "refresh_token": store.refresh_token, + "client_id": anthropic_client_id(), + })) + .send() + .await? + } else { + client + .post(&store.token_endpoint) + .form(&[ + ("grant_type", "refresh_token"), + ("refresh_token", store.refresh_token.as_str()), + ("client_id", codex_client_id().as_str()), + ]) + .send() + .await? + }; if !resp.status().is_success() { let status = resp.status(); let body = resp.text().await.unwrap_or_default(); - return Err(anyhow!("Token refresh failed (HTTP {status}): {body}. Run `openab-agent auth codex-oauth` again.")); + return Err(anyhow!("Token refresh failed (HTTP {status}): {body}. Run `openab-agent auth` again.")); } let payload: serde_json::Value = resp.json().await?; let access_token = payload["access_token"] @@ -474,7 +535,7 @@ pub async fn login_browser_flow(no_browser: bool) -> Result<()> { token_endpoint: CODEX_TOKEN_URL.to_string(), provider: "codex".to_string(), }; - save_tokens(&store)?; + save_tokens_for(&store)?; println!( "\n\u{2705} Login successful! Token saved to {:?}", auth_path() @@ -557,7 +618,105 @@ pub async fn login_browser_flow(no_browser: bool) -> Result<()> { token_endpoint: CODEX_TOKEN_URL.to_string(), provider: "codex".to_string(), }; - save_tokens(&store)?; + save_tokens_for(&store)?; + println!( + "\n\u{2705} Login successful! Token saved to {:?}", + auth_path() + ); + Ok(()) +} + +/// Extract the OAuth `code` from a parsed redirect URL, validating `state`. +/// Shared by every loopback-callback OAuth flow. +fn code_from_redirect(url: &url::Url, expected_state: &str) -> Result { + let code = url + .query_pairs() + .find(|(k, _)| k == "code") + .map(|(_, v)| v.to_string()) + .ok_or_else(|| { + let error = url + .query_pairs() + .find(|(k, _)| k == "error") + .map(|(_, v)| v.to_string()); + anyhow!( + "No code in redirect. Error: {}", + error.unwrap_or_else(|| "unknown".into()) + ) + })?; + let cb_state = url + .query_pairs() + .find(|(k, _)| k == "state") + .map(|(_, v)| v.to_string()); + if cb_state.as_deref() != Some(expected_state) { + return Err(anyhow!("State mismatch")); + } + Ok(code) +} + +/// Block on the loopback listener for the OAuth redirect, reply 200, return the +/// authorization code. ponytail: the Codex flow above predates this helper and +/// still inlines the same logic; fold it in if that path is ever touched again. +fn accept_callback_code(listener: &TcpListener, expected_state: &str) -> Result { + listener.set_nonblocking(false)?; + let (mut stream, _) = listener + .accept() + .map_err(|e| anyhow!("Failed to accept callback: {e}"))?; + let mut reader = std::io::BufReader::new(&stream); + let mut request_line = String::new(); + reader.read_line(&mut request_line)?; + let path = request_line.split_whitespace().nth(1).unwrap_or(""); + let url = url::Url::parse(&format!("http://localhost{path}")) + .map_err(|_| anyhow!("Invalid callback URL"))?; + let code = code_from_redirect(&url, expected_state)?; + let response = "HTTP/1.1 200 OK\r\nContent-Type: text/html\r\n\r\n

Authentication successful!

You can close this tab.

"; + let _ = stream.write_all(response.as_bytes()); + Ok(code) +} + +/// Anthropic OAuth (Claude Pro/Max). PKCE with the verifier doubling as `state` +/// (Pi's convention) and a JSON token exchange against `platform.claude.com`. +pub async fn login_anthropic_browser_flow(no_browser: bool) -> Result<()> { + let (verifier, challenge) = generate_pkce(); + let state = verifier.clone(); // Pi reuses the verifier as the state value + let auth_url = anthropic_authorize_url(&challenge, &state); + + let code = if no_browser { + println!("Open this URL in your browser:\n\n {auth_url}\n"); + println!("After approving, copy the full redirect URL (or just the code) and paste it here:\n"); + let mut input = String::new(); + std::io::stdin() + .read_line(&mut input) + .map_err(|e| anyhow!("Failed to read input: {e}"))?; + let input = input.trim(); + if input.is_empty() { + return Err(anyhow!("No URL provided")); + } + // Accept either a full redirect URL or a bare `code` / `code#state`. + if let Ok(url) = url::Url::parse(input) { + code_from_redirect(&url, &state)? + } else { + let (code, st) = input.split_once('#').unwrap_or((input, state.as_str())); + if st != state { + return Err(anyhow!("State mismatch")); + } + code.to_string() + } + } else { + let listener = TcpListener::bind(format!("127.0.0.1:{ANTHROPIC_REDIRECT_PORT}")).map_err( + |e| { + anyhow!("Failed to bind port {ANTHROPIC_REDIRECT_PORT}: {e}. Is another instance running?") + }, + )?; + println!("Opening browser for authentication...\n"); + if open::that(&auth_url).is_err() { + println!("Could not open browser. Open this URL manually:\n\n {auth_url}\n"); + } + println!("Waiting for callback..."); + accept_callback_code(&listener, &state)? + }; + + let store = exchange_anthropic_code(&code, &state, &verifier).await?; + save_tokens_for(&store)?; println!( "\n\u{2705} Login successful! Token saved to {:?}", auth_path() @@ -565,6 +724,42 @@ pub async fn login_browser_flow(no_browser: bool) -> Result<()> { Ok(()) } +async fn exchange_anthropic_code(code: &str, state: &str, verifier: &str) -> Result { + let client = reqwest::Client::new(); + let resp = client + .post(ANTHROPIC_TOKEN_URL) + .json(&serde_json::json!({ + "grant_type": "authorization_code", + "client_id": anthropic_client_id(), + "code": code, + "state": state, + "redirect_uri": anthropic_redirect_uri(), + "code_verifier": verifier, + })) + .send() + .await?; + if !resp.status().is_success() { + let body = resp.text().await.unwrap_or_default(); + return Err(anyhow!("Token exchange failed: {body}")); + } + let payload: serde_json::Value = resp.json().await?; + let access_token = payload["access_token"] + .as_str() + .ok_or_else(|| anyhow!("No access_token"))?; + let refresh_token_val = payload["refresh_token"] + .as_str() + .ok_or_else(|| anyhow!("No refresh_token"))?; + let expires_in = payload["expires_in"].as_u64().unwrap_or(3600); + let now = SystemTime::now().duration_since(UNIX_EPOCH)?.as_secs(); + Ok(TokenStore { + access_token: access_token.to_string(), + refresh_token: refresh_token_val.to_string(), + expires_at: now + expires_in, + token_endpoint: ANTHROPIC_TOKEN_URL.to_string(), + provider: ANTHROPIC_NAMESPACE.to_string(), + }) +} + // Device code flow pub async fn login_codex_device_flow() -> Result<()> { println!("Starting OpenAI Codex device-code login...\n"); @@ -649,7 +844,7 @@ pub async fn login_codex_device_flow() -> Result<()> { token_endpoint: CODEX_TOKEN_URL.to_string(), provider: "codex".to_string(), }; - save_tokens(&store)?; + save_tokens_for(&store)?; println!( "\n\u{2705} Login successful! Token saved to {:?}", auth_path() @@ -681,31 +876,49 @@ pub async fn login_codex_device_flow() -> Result<()> { } pub fn show_status() { - match load_tokens() { - Ok(store) => { - let expired = store.is_expired(); - let masked = if store.access_token.len() > 12 { - format!( - "{}...{}", - &store.access_token[..8], - &store.access_token[store.access_token.len() - 4..] - ) - } else { - "****".to_string() - }; - println!("Provider: {}", store.provider); - println!("Token: {}", masked); - println!( - "Expires: {} ({})", - store.expires_at, - if expired { "EXPIRED" } else { "valid" } - ); - println!("File: {:?}", auth_path()); - } - Err(e) => { - println!("Not authenticated: {e}\nRun: openab-agent auth codex-oauth"); - } + let path = auth_path(); + let tokens: Vec = read_auth_file(&path) + .map(|map| { + let mut v: Vec = map + .into_values() + .filter_map(|e| match e { + AuthEntry::Token(t) => Some(t), + _ => None, + }) + .collect(); + v.sort_by(|a, b| a.provider.cmp(&b.provider)); + v + }) + .unwrap_or_default(); + + if tokens.is_empty() { + println!( + "Not authenticated.\nRun: openab-agent auth codex-oauth | openab-agent auth anthropic-oauth" + ); + return; + } + + for store in tokens { + let expired = store.is_expired(); + let masked = if store.access_token.len() > 12 { + format!( + "{}...{}", + &store.access_token[..8], + &store.access_token[store.access_token.len() - 4..] + ) + } else { + "****".to_string() + }; + println!("Provider: {}", store.provider); + println!("Token: {}", masked); + println!( + "Expires: {} ({})", + store.expires_at, + if expired { "EXPIRED" } else { "valid" } + ); + println!(); } + println!("File: {:?}", path); } #[cfg(test)] @@ -779,6 +992,44 @@ mod tests { assert_eq!(challenge, expected); } + #[test] + fn test_anthropic_authorize_url_carries_required_params() { + temp_env::with_var("OPENAB_AGENT_ANTHROPIC_CLIENT_ID", None::<&str>, || { + let url = anthropic_authorize_url("CHAL", "STATE"); + assert!(url.starts_with("https://claude.ai/oauth/authorize?")); + assert!(url.contains("client_id=9d1c250a-e61b-44d9-88ed-5944d1962f5e")); + assert!(url.contains("response_type=code")); + assert!(url.contains("code_challenge=CHAL")); + assert!(url.contains("code_challenge_method=S256")); + assert!(url.contains("state=STATE")); + // scope is url-encoded; spot-check one encoded scope token + assert!(url.contains("user%3Ainference")); + // redirect must be the loopback callback on the Anthropic port + assert!(url.contains("localhost%3A53692%2Fcallback")); + }); + } + + #[test] + fn test_anthropic_save_uses_provider_as_key_disjoint_from_codex() { + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("auth.json"); + let mut codex = make_store(1); + codex.provider = "codex".to_string(); + let mut anth = make_store(2); + anth.provider = ANTHROPIC_NAMESPACE.to_string(); + anth.access_token = "sk-ant-oat-xyz".to_string(); + let mut input = HashMap::new(); + input.insert(codex.provider.clone(), AuthEntry::Token(codex)); + input.insert(anth.provider.clone(), AuthEntry::Token(anth)); + write_auth_file(&path, &input).unwrap(); + let map = read_auth_file(&path).unwrap(); + assert_eq!(token_of(map.get("codex")).expires_at, 1); + assert_eq!( + token_of(map.get(ANTHROPIC_NAMESPACE)).access_token, + "sk-ant-oat-xyz" + ); + } + fn token_of(entry: Option<&AuthEntry>) -> &TokenStore { match entry { Some(AuthEntry::Token(t)) => t, diff --git a/openab-agent/src/llm.rs b/openab-agent/src/llm.rs index de5e1eb6c..1a3f7b506 100644 --- a/openab-agent/src/llm.rs +++ b/openab-agent/src/llm.rs @@ -91,20 +91,22 @@ impl std::ops::Deref for SharedLlmProvider { } /// Select an `LlmProvider` from an explicit `choice` (`anthropic` / -/// `openai` / `codex`) or, for any other value, auto-detect (Anthropic API -/// key first, then codex OAuth). Shared by the ACP session path and MCP -/// sampling so both honor the same `OPENAB_AGENT_PROVIDER` selection and -/// credential fallback. +/// `anthropic-oauth` / `openai` / `codex`) or, for any other value, auto-detect +/// (Anthropic API key, then Claude subscription OAuth, then codex OAuth). The +/// `anthropic` choice itself auto-falls-back from API key to OAuth. Shared by +/// the ACP session path and MCP sampling so both honor the same +/// `OPENAB_AGENT_PROVIDER` selection and credential fallback. pub fn select_provider(choice: &str) -> Result, String> { match choice { - "anthropic" => Ok(Box::new(AnthropicProvider::from_env()?)), + "anthropic" => Ok(Box::new(AnthropicProvider::auto()?)), + "anthropic-oauth" | "claude" => Ok(Box::new(AnthropicProvider::from_oauth_store()?)), "openai" | "codex" => Ok(Box::new(OpenAiProvider::from_auth_store()?)), - _ => match AnthropicProvider::from_env() { + _ => match AnthropicProvider::auto() { Ok(p) => Ok(Box::new(p)), Err(_) => match OpenAiProvider::from_auth_store() { Ok(p) => Ok(Box::new(p)), Err(e) => Err(format!( - "No credentials: set ANTHROPIC_API_KEY or run `openab-agent auth codex-oauth`. {e}" + "No credentials: set ANTHROPIC_API_KEY, or run `openab-agent auth anthropic-oauth` / `auth codex-oauth`. {e}" )), }, }, @@ -122,15 +124,62 @@ pub fn default_provider() -> Option { .map(|b| SharedLlmProvider(Arc::from(b))) } +/// How an `AnthropicProvider` authenticates to the Messages API. +enum AnthropicAuth { + /// `ANTHROPIC_API_KEY` → `x-api-key`, plain system prompt. + ApiKey(String), + /// Claude Pro/Max subscription OAuth → `Bearer` + Claude Code identity + /// headers/system block. The live token is fetched (and refreshed) per call + /// from the `anthropic-oauth` tenant in auth.json. + OAuth, +} + /// Anthropic Claude provider. pub struct AnthropicProvider { - api_key: String, + auth: AnthropicAuth, model: String, - #[allow(dead_code)] max_tokens: u32, client: reqwest::Client, } +fn anthropic_model_from_env() -> String { + std::env::var("OPENAB_AGENT_MODEL").unwrap_or_else(|_| "claude-sonnet-4-20250514".to_string()) +} + +fn anthropic_max_tokens() -> u32 { + std::env::var("OPENAB_AGENT_MAX_TOKENS") + .ok() + .and_then(|v| v.parse().ok()) + .unwrap_or(8192) +} + +/// openab-agent's built-in tools mapped to Claude Code's canonical casing. The +/// `claude-code-20250219` beta (sent with OAuth tokens) expects these names, so +/// they're rewritten on the way out and restored on the way back. Unknown names +/// (e.g. MCP tools) pass through unchanged, matching Pi's behaviour. +const CC_TOOL_NAMES: &[(&str, &str)] = &[ + ("read", "Read"), + ("write", "Write"), + ("edit", "Edit"), + ("bash", "Bash"), +]; + +fn to_claude_code_name(name: &str) -> String { + CC_TOOL_NAMES + .iter() + .find(|(lc, _)| *lc == name) + .map(|(_, cc)| (*cc).to_string()) + .unwrap_or_else(|| name.to_string()) +} + +fn from_claude_code_name(name: &str) -> String { + CC_TOOL_NAMES + .iter() + .find(|(_, cc)| *cc == name) + .map(|(lc, _)| (*lc).to_string()) + .unwrap_or_else(|| name.to_string()) +} + impl AnthropicProvider { pub fn from_env() -> Result { let api_key = std::env::var("ANTHROPIC_API_KEY") @@ -139,25 +188,51 @@ impl AnthropicProvider { return Err("ANTHROPIC_API_KEY is empty".to_string()); } Ok(Self { - api_key, - model: std::env::var("OPENAB_AGENT_MODEL") - .unwrap_or_else(|_| "claude-sonnet-4-20250514".to_string()), - max_tokens: std::env::var("OPENAB_AGENT_MAX_TOKENS") - .ok() - .and_then(|v| v.parse().ok()) - .unwrap_or(8192), + auth: AnthropicAuth::ApiKey(api_key), + model: anthropic_model_from_env(), + max_tokens: anthropic_max_tokens(), client: reqwest::Client::new(), }) } - /// Create provider with a specific model override. - pub fn from_env_with_model(model: &str) -> Result { - let mut p = Self::from_env()?; + /// Claude Pro/Max OAuth. Verifies a stored `anthropic-oauth` token exists; + /// the live token is fetched (and refreshed) at call time. + pub fn from_oauth_store() -> Result { + crate::auth::load_tokens_for(crate::auth::ANTHROPIC_NAMESPACE) + .map_err(|e| e.to_string())?; + Ok(Self { + auth: AnthropicAuth::OAuth, + model: anthropic_model_from_env(), + max_tokens: anthropic_max_tokens(), + client: reqwest::Client::new(), + }) + } + + /// Prefer an explicit API key, else a stored Claude subscription OAuth token. + pub fn auto() -> Result { + Self::from_env().or_else(|_| Self::from_oauth_store()) + } + + /// `auto()` with an explicit model override. + pub fn auto_with_model(model: &str) -> Result { + let mut p = Self::auto()?; p.model = model.to_string(); Ok(p) } + /// `from_oauth_store()` with an explicit model override. + pub fn from_oauth_store_with_model(model: &str) -> Result { + let mut p = Self::from_oauth_store()?; + p.model = model.to_string(); + Ok(p) + } + + fn is_oauth(&self) -> bool { + matches!(self.auth, AnthropicAuth::OAuth) + } + fn build_request_body(&self, system: &str, messages: &[Message], tools: &[ToolDef]) -> Value { + let oauth = self.is_oauth(); let msgs: Vec = messages .iter() .map(|m| { @@ -167,6 +242,7 @@ impl AnthropicProvider { .map(|b| match b { ContentBlock::Text { text } => json!({ "type": "text", "text": text }), ContentBlock::ToolUse { id, name, input } => { + let name = if oauth { to_claude_code_name(name) } else { name.clone() }; json!({ "type": "tool_use", "id": id, "name": name, "input": input }) } ContentBlock::ToolResult { @@ -194,15 +270,27 @@ impl AnthropicProvider { "model": &self.model, "max_tokens": self.max_tokens, "messages": msgs, - "system": system, }); + // OAuth tokens MUST carry the Claude Code identity as the first system + // block, with the real prompt appended. API-key callers send a plain + // string (unchanged behaviour). + if oauth { + body["system"] = json!([ + { "type": "text", "text": "You are Claude Code, Anthropic's official CLI for Claude." }, + { "type": "text", "text": system }, + ]); + } else { + body["system"] = json!(system); + } + if !tools.is_empty() { let tool_defs: Vec = tools .iter() .map(|t| { + let name = if oauth { to_claude_code_name(&t.name) } else { t.name.clone() }; json!({ - "name": &t.name, + "name": name, "description": &t.description, "input_schema": &t.input_schema }) @@ -228,15 +316,32 @@ impl LlmProvider for AnthropicProvider { ) -> Pin>> + Send + 'a>> { Box::pin(async move { let body = self.build_request_body(system, messages, tools); + let oauth = self.is_oauth(); let max_retries = 3u32; for attempt in 0..=max_retries { - let resp = self + let mut req = self .client .post("https://api.anthropic.com/v1/messages") - .header("x-api-key", &self.api_key) .header("anthropic-version", "2023-06-01") - .header("content-type", "application/json") + .header("content-type", "application/json"); + req = match &self.auth { + AnthropicAuth::ApiKey(key) => req.header("x-api-key", key), + AnthropicAuth::OAuth => { + // Claude Pro/Max: Bearer + Claude Code identity headers. + let token = crate::auth::get_valid_token_for( + crate::auth::ANTHROPIC_NAMESPACE, + ) + .await?; + req.header("authorization", format!("Bearer {token}")) + .header("anthropic-beta", "claude-code-20250219,oauth-2025-04-20") + .header("user-agent", "claude-cli/1.0.0") + .header("x-app", "cli") + .header("anthropic-dangerous-direct-browser-access", "true") + } + }; + + let resp = req .json(&body) .send() .await @@ -251,6 +356,14 @@ impl LlmProvider for AnthropicProvider { continue; } + // 401 on OAuth: token may have expired mid-request; force a + // refresh and retry once before surfacing the error. + if oauth && status.as_u16() == 401 && attempt < max_retries { + let _ = + crate::auth::force_refresh_for(crate::auth::ANTHROPIC_NAMESPACE).await; + continue; + } + if !status.is_success() { let text = resp.text().await.unwrap_or_default(); return Err(anyhow!("Anthropic API error {status}: {text}")); @@ -261,7 +374,17 @@ impl LlmProvider for AnthropicProvider { .await .map_err(|e| anyhow!("Failed to parse response: {e}"))?; - return parse_anthropic_response(&response); + let mut events = parse_anthropic_response(&response)?; + // Restore openab-agent's lowercase tool names from the Claude + // Code canonical casing the model echoes back under OAuth. + if oauth { + for ev in &mut events { + if let LlmEvent::ToolUse { name, .. } = ev { + *name = from_claude_code_name(name); + } + } + } + return Ok(events); } Err(anyhow!("Anthropic API: max retries exceeded")) @@ -664,14 +787,18 @@ mod tests { } } - #[test] - fn test_build_request_body() { - let provider = AnthropicProvider { - api_key: "test".to_string(), + fn test_provider(auth: AnthropicAuth) -> AnthropicProvider { + AnthropicProvider { + auth, model: "claude-sonnet-4-20250514".to_string(), max_tokens: 4096, client: reqwest::Client::new(), - }; + } + } + + #[test] + fn test_build_request_body() { + let provider = test_provider(AnthropicAuth::ApiKey("test".to_string())); let messages = vec![Message { role: "user".to_string(), content: vec![ContentBlock::Text { @@ -681,10 +808,48 @@ mod tests { let body = provider.build_request_body("system prompt", &messages, &[]); assert_eq!(body["model"], "claude-sonnet-4-20250514"); assert_eq!(body["max_tokens"], 4096); + // API-key mode keeps the plain-string system prompt. assert_eq!(body["system"], "system prompt"); assert_eq!(body["messages"][0]["role"], "user"); } + #[test] + fn test_build_request_body_oauth_injects_claude_code_identity_and_caps_tools() { + let provider = test_provider(AnthropicAuth::OAuth); + let messages = vec![Message { + role: "assistant".to_string(), + content: vec![ContentBlock::ToolUse { + id: "tu_1".to_string(), + name: "read".to_string(), + input: json!({"path": "/tmp/x"}), + }], + }]; + let tools = vec![ToolDef { + name: "bash".to_string(), + description: "run".to_string(), + input_schema: json!({}), + }]; + let body = provider.build_request_body("real prompt", &messages, &tools); + // system[0] must be the Claude Code identity, real prompt appended. + assert_eq!( + body["system"][0]["text"], + "You are Claude Code, Anthropic's official CLI for Claude." + ); + assert_eq!(body["system"][1]["text"], "real prompt"); + // tool def + assistant tool_use names normalised to CC casing. + assert_eq!(body["tools"][0]["name"], "Bash"); + assert_eq!(body["messages"][0]["content"][0]["name"], "Read"); + } + + #[test] + fn test_claude_code_name_round_trip_and_passthrough() { + assert_eq!(to_claude_code_name("read"), "Read"); + assert_eq!(from_claude_code_name("Read"), "read"); + // unknown (e.g. MCP) names pass through unchanged both ways. + assert_eq!(to_claude_code_name("linear_search"), "linear_search"); + assert_eq!(from_claude_code_name("linear_search"), "linear_search"); + } + #[test] fn test_parse_openai_text_response() { let resp = json!({ diff --git a/openab-agent/src/main.rs b/openab-agent/src/main.rs index 95d059771..068a74fd4 100644 --- a/openab-agent/src/main.rs +++ b/openab-agent/src/main.rs @@ -86,6 +86,12 @@ enum AuthProvider { }, /// OpenAI Codex via device code (headless servers) CodexDevice, + /// Anthropic Claude Pro/Max via browser PKCE flow + AnthropicOauth { + /// Print URL and paste the redirect instead of opening a browser + #[arg(long)] + no_browser: bool, + }, /// Show stored credentials Status, } @@ -118,6 +124,12 @@ async fn main() { std::process::exit(1); } } + AuthProvider::AnthropicOauth { no_browser } => { + if let Err(e) = auth::login_anthropic_browser_flow(no_browser).await { + eprintln!("❌ Authentication failed: {e}"); + std::process::exit(1); + } + } AuthProvider::Status => { auth::show_status(); } From f5fcb824346545159124f62bc75663f90f392254 Mon Sep 17 00:00:00 2001 From: Can Yu Date: Wed, 24 Jun 2026 14:21:02 +0800 Subject: [PATCH 02/20] fix(native-agent): default Anthropic model to claude-opus-4-8 The fallback default was claude-sonnet-4-20250514 (Sonnet 4.0, ~13mo old), which 404s on Claude Pro/Max OAuth subscriptions. Bump the three default-model fallbacks to the current claude-opus-4-8 (verified live via OAuth). The model catalog already listed it; only the fallback was stale. Co-Authored-By: Claude Opus 4.8 (1M context) --- openab-agent/src/acp.rs | 4 ++-- openab-agent/src/llm.rs | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/openab-agent/src/acp.rs b/openab-agent/src/acp.rs index 0d866812f..cdc2ed6cf 100644 --- a/openab-agent/src/acp.rs +++ b/openab-agent/src/acp.rs @@ -372,7 +372,7 @@ impl AcpServer { .or_else(|| std::env::var("OPENAB_AGENT_MODEL").ok()) .unwrap_or_else(|| { if active_provider == "anthropic" { - "claude-sonnet-4-20250514".to_string() + "claude-opus-4-8".to_string() } else { "gpt-5.4-mini".to_string() } @@ -433,7 +433,7 @@ impl AcpServer { if self.active_provider.as_deref() == Some("openai") { "gpt-5.4-mini".to_string() } else { - "claude-sonnet-4-20250514".to_string() + "claude-opus-4-8".to_string() } }); diff --git a/openab-agent/src/llm.rs b/openab-agent/src/llm.rs index 1a3f7b506..e1abceb6f 100644 --- a/openab-agent/src/llm.rs +++ b/openab-agent/src/llm.rs @@ -143,7 +143,7 @@ pub struct AnthropicProvider { } fn anthropic_model_from_env() -> String { - std::env::var("OPENAB_AGENT_MODEL").unwrap_or_else(|_| "claude-sonnet-4-20250514".to_string()) + std::env::var("OPENAB_AGENT_MODEL").unwrap_or_else(|_| "claude-opus-4-8".to_string()) } fn anthropic_max_tokens() -> u32 { From de14bd7876c3d51d566d49ed92039d320a96adac Mon Sep 17 00:00:00 2001 From: Can Yu Date: Wed, 24 Jun 2026 20:37:40 +0800 Subject: [PATCH 03/20] fix(native-agent): address PR review (CI workspace, PKCE state, error UX) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - F1 (blocker): root Cargo.toml `exclude = ["openab-agent"]` so `cd openab-agent && cargo fmt/clippy/test` resolves standalone. The workspace restructure left openab-agent neither a member nor excluded; CI openab-agent only runs on openab-agent/** so it was dormant on main — this PR is the first change to trigger it. Also ran `cargo fmt`. - F2: use an independent 32-byte random PKCE `state` instead of reusing the verifier, keeping the verifier back-channel-only (claude.ai rejects a short state as "Invalid request format"; 32 bytes matches the verifier length). Verified end-to-end with a real Pro/Max login + chat. - F3: credential-error messages now name fully-qualified subcommands (`openab-agent auth anthropic-oauth` / `... codex-oauth`) and preserve the underlying read/parse error. - F4: drop the `ponytail:` placeholder tag from a comment. Co-Authored-By: Claude Opus 4.8 (1M context) --- Cargo.toml | 1 + openab-agent/src/acp.rs | 6 ++---- openab-agent/src/auth.rs | 37 +++++++++++++++++++++++++++---------- openab-agent/src/llm.rs | 33 ++++++++++++++++++--------------- 4 files changed, 48 insertions(+), 29 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 042ca6223..69b6f01ae 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -1,5 +1,6 @@ [workspace] members = ["crates/openab-core", "crates/openab-gateway"] +exclude = ["openab-agent"] [package] name = "openab" diff --git a/openab-agent/src/acp.rs b/openab-agent/src/acp.rs index cdc2ed6cf..f1af0485a 100644 --- a/openab-agent/src/acp.rs +++ b/openab-agent/src/acp.rs @@ -347,7 +347,7 @@ impl AcpServer { return self.error_response( id, -32000, - &format!("No credentials: set ANTHROPIC_API_KEY, or run `openab-agent auth anthropic-oauth` / `auth codex-oauth`. {e}"), + &format!("No credentials: set ANTHROPIC_API_KEY, or run `openab-agent auth anthropic-oauth` / `openab-agent auth codex-oauth`. {e}"), ) } } @@ -603,9 +603,7 @@ impl AcpServer { if !session_id.is_empty() && self.sessions.contains_key(session_id) { let new_provider: Result, String> = match provider_name { - "anthropic" => { - AnthropicProvider::auto_with_model(value).map(|p| Box::new(p) as _) - } + "anthropic" => AnthropicProvider::auto_with_model(value).map(|p| Box::new(p) as _), _ => crate::llm::OpenAiProvider::from_auth_store_with_model(value) .map(|p| Box::new(p) as _), }; diff --git a/openab-agent/src/auth.rs b/openab-agent/src/auth.rs index ff71ad6ee..960372e21 100644 --- a/openab-agent/src/auth.rs +++ b/openab-agent/src/auth.rs @@ -51,8 +51,9 @@ fn anthropic_redirect_uri() -> String { format!("http://localhost:{ANTHROPIC_REDIRECT_PORT}/callback") } -/// Build the Anthropic authorize URL. Pure so it can be unit-tested. Pi reuses -/// the PKCE verifier as the `state` value, so callers pass `state == verifier`. +/// Build the Anthropic authorize URL. Pure so it can be unit-tested. `state` is +/// an independent random CSRF value (kept distinct from the PKCE verifier, which +/// stays back-channel-only) — the AS just echoes it back. fn anthropic_authorize_url(challenge: &str, state: &str) -> String { let client_id = anthropic_client_id(); let redirect = anthropic_redirect_uri(); @@ -249,16 +250,22 @@ fn write_auth_file(path: &Path, map: &HashMap) -> Result<()> /// Load the LLM token stored under `namespace` (`codex` / `anthropic-oauth`). pub fn load_tokens_for(namespace: &str) -> Result { let path = auth_path(); - let map = read_auth_file(&path).map_err(|_| { + let cmd = if namespace == ANTHROPIC_NAMESPACE { + "openab-agent auth anthropic-oauth" + } else { + "openab-agent auth codex-oauth" + }; + // Preserve the underlying read/parse error for debugging. + let map = read_auth_file(&path).map_err(|e| { anyhow!( - "No credentials found at {}. Run `openab-agent auth` first.", + "No credentials at {} ({e}). Run `{cmd}` first.", path.display() ) })?; match map.get(namespace) { Some(AuthEntry::Token(t)) => Ok(t.clone()), _ => Err(anyhow!( - "No {namespace} credentials in {}. Run `openab-agent auth` first.", + "No {namespace} credentials in {}. Run `{cmd}` first.", path.display() )), } @@ -420,7 +427,9 @@ async fn refresh_token(store: &TokenStore) -> Result { if !resp.status().is_success() { let status = resp.status(); let body = resp.text().await.unwrap_or_default(); - return Err(anyhow!("Token refresh failed (HTTP {status}): {body}. Run `openab-agent auth` again.")); + return Err(anyhow!( + "Token refresh failed (HTTP {status}): {body}. Run `openab-agent auth` again." + )); } let payload: serde_json::Value = resp.json().await?; let access_token = payload["access_token"] @@ -654,8 +663,8 @@ fn code_from_redirect(url: &url::Url, expected_state: &str) -> Result { } /// Block on the loopback listener for the OAuth redirect, reply 200, return the -/// authorization code. ponytail: the Codex flow above predates this helper and -/// still inlines the same logic; fold it in if that path is ever touched again. +/// authorization code. Note: the Codex flow above predates this helper and still +/// inlines the same logic; fold it in if that path is ever touched again. fn accept_callback_code(listener: &TcpListener, expected_state: &str) -> Result { listener.set_nonblocking(false)?; let (mut stream, _) = listener @@ -677,12 +686,20 @@ fn accept_callback_code(listener: &TcpListener, expected_state: &str) -> Result< /// (Pi's convention) and a JSON token exchange against `platform.claude.com`. pub async fn login_anthropic_browser_flow(no_browser: bool) -> Result<()> { let (verifier, challenge) = generate_pkce(); - let state = verifier.clone(); // Pi reuses the verifier as the state value + // Independent random CSRF state — keep the PKCE verifier back-channel-only. + // 32 bytes: claude.ai's authorize rejects a short state ("Invalid request + // format"); matching the verifier's length keeps it happy while the value + // stays independent (full PKCE strength). + let mut state_buf = [0u8; 32]; + getrandom::fill(&mut state_buf).expect("getrandom failed"); + let state = URL_SAFE_NO_PAD.encode(state_buf); let auth_url = anthropic_authorize_url(&challenge, &state); let code = if no_browser { println!("Open this URL in your browser:\n\n {auth_url}\n"); - println!("After approving, copy the full redirect URL (or just the code) and paste it here:\n"); + println!( + "After approving, copy the full redirect URL (or just the code) and paste it here:\n" + ); let mut input = String::new(); std::io::stdin() .read_line(&mut input) diff --git a/openab-agent/src/llm.rs b/openab-agent/src/llm.rs index e1abceb6f..dcd5e21ad 100644 --- a/openab-agent/src/llm.rs +++ b/openab-agent/src/llm.rs @@ -106,7 +106,7 @@ pub fn select_provider(choice: &str) -> Result, String> { Err(_) => match OpenAiProvider::from_auth_store() { Ok(p) => Ok(Box::new(p)), Err(e) => Err(format!( - "No credentials: set ANTHROPIC_API_KEY, or run `openab-agent auth anthropic-oauth` / `auth codex-oauth`. {e}" + "No credentials: set ANTHROPIC_API_KEY, or run `openab-agent auth anthropic-oauth` / `openab-agent auth codex-oauth`. {e}" )), }, }, @@ -233,10 +233,11 @@ impl AnthropicProvider { fn build_request_body(&self, system: &str, messages: &[Message], tools: &[ToolDef]) -> Value { let oauth = self.is_oauth(); - let msgs: Vec = messages - .iter() - .map(|m| { - let content: Vec = m + let msgs: Vec = + messages + .iter() + .map(|m| { + let content: Vec = m .content .iter() .map(|b| match b { @@ -262,9 +263,9 @@ impl AnthropicProvider { } }) .collect(); - json!({ "role": &m.role, "content": content }) - }) - .collect(); + json!({ "role": &m.role, "content": content }) + }) + .collect(); let mut body = json!({ "model": &self.model, @@ -288,7 +289,11 @@ impl AnthropicProvider { let tool_defs: Vec = tools .iter() .map(|t| { - let name = if oauth { to_claude_code_name(&t.name) } else { t.name.clone() }; + let name = if oauth { + to_claude_code_name(&t.name) + } else { + t.name.clone() + }; json!({ "name": name, "description": &t.description, @@ -329,10 +334,9 @@ impl LlmProvider for AnthropicProvider { AnthropicAuth::ApiKey(key) => req.header("x-api-key", key), AnthropicAuth::OAuth => { // Claude Pro/Max: Bearer + Claude Code identity headers. - let token = crate::auth::get_valid_token_for( - crate::auth::ANTHROPIC_NAMESPACE, - ) - .await?; + let token = + crate::auth::get_valid_token_for(crate::auth::ANTHROPIC_NAMESPACE) + .await?; req.header("authorization", format!("Bearer {token}")) .header("anthropic-beta", "claude-code-20250219,oauth-2025-04-20") .header("user-agent", "claude-cli/1.0.0") @@ -359,8 +363,7 @@ impl LlmProvider for AnthropicProvider { // 401 on OAuth: token may have expired mid-request; force a // refresh and retry once before surfacing the error. if oauth && status.as_u16() == 401 && attempt < max_retries { - let _ = - crate::auth::force_refresh_for(crate::auth::ANTHROPIC_NAMESPACE).await; + let _ = crate::auth::force_refresh_for(crate::auth::ANTHROPIC_NAMESPACE).await; continue; } From 89347981ec596d660aa4a76e91bcdd8ffa00f7b3 Mon Sep 17 00:00:00 2001 From: Can Yu Date: Wed, 24 Jun 2026 20:56:52 +0800 Subject: [PATCH 04/20] fix(native-agent): flush stdout drain on ACP server shutdown The dispatch loop fed responses to a detached stdout-drain task; on stdin EOF the loop ended and `#[tokio::main]` aborted the drain before it flushed the last queued line, so a one-shot `initialize` could return nothing. This was a latent race (main wins it by timing); this branch's slightly different startup timing made the binary lose it ~85% locally, surfacing as the red `CI openab-agent` ACP smoke test. Capture the drain handle and, after the loop, drop the senders and bounded-await the drain so queued output is flushed before return. Race test: 20/20 after (was 3/20). Co-Authored-By: Claude Opus 4.8 (1M context) --- openab-agent/src/acp.rs | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/openab-agent/src/acp.rs b/openab-agent/src/acp.rs index f1af0485a..b3be526d6 100644 --- a/openab-agent/src/acp.rs +++ b/openab-agent/src/acp.rs @@ -204,7 +204,7 @@ impl AcpServer { // through `out_tx` into this one drain task, preserving the // one-writer invariant the HostBridge relies on. let (out_tx, mut out_rx) = mpsc::unbounded_channel::(); - tokio::spawn(async move { + let drain = tokio::spawn(async move { let mut stdout = io::stdout(); while let Some(line) = out_rx.recv().await { let _ = writeln!(stdout, "{}", line); @@ -268,6 +268,16 @@ impl AcpServer { let _ = out_tx.send(line); } } + + // Shutdown: stdin hit EOF and the dispatch loop ended. Drop our senders + // so the drain task can flush any queued output and finish before this + // returns — otherwise `#[tokio::main]` aborts the detached drain on + // return and the last response can be lost (the ACP `initialize` smoke + // test depends on this). Bounded await so a lingering sender (e.g. an + // MCP background task holding an `out_tx` clone) can't wedge shutdown. + drop(bridge); + drop(out_tx); + let _ = tokio::time::timeout(std::time::Duration::from_secs(2), drain).await; } fn handle_initialize(&self, id: u64) -> String { From 532ec1d3d6e3ef3c10239a834f31ee5aa95056b8 Mon Sep 17 00:00:00 2001 From: Can Yu Date: Wed, 24 Jun 2026 22:50:11 +0800 Subject: [PATCH 05/20] fix: address review #6/#7/#11 (auth mode, 401 refresh, error UX) Non-blocking polish from the PR re-review: - #6 (acp.rs): on ACP model switch, an OAuth-forced session was rebuilt via `auto_with_model`, which prefers ANTHROPIC_API_KEY and silently dropped the forced anthropic-oauth provider when a key was also present. Rebuild now preserves the session's auth mode via a new LlmProvider::is_oauth() (Agent::provider_is_oauth()). - #7 (llm.rs): the OAuth 401 branch swallowed force_refresh_for errors (`let _ = ...`) and retried with the stale token. Bubble the error. - #11 (auth.rs): refresh_token failure message named bare `openab-agent auth`; now names the tenant subcommand via a shared auth_subcommand() helper (also dedupes load_tokens_for). Deferred as follow-up (noted in PR): #8 --no-browser state validation, #9 save_tokens_for keying, #10 non-Unix atomic write. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01XHjGw1uBHXoVPB3dXYEy7h --- openab-agent/src/acp.rs | 6 ++++++ openab-agent/src/agent.rs | 6 ++++++ openab-agent/src/auth.rs | 19 +++++++++++++------ openab-agent/src/llm.rs | 28 ++++++++++++++++++++++------ 4 files changed, 47 insertions(+), 12 deletions(-) diff --git a/openab-agent/src/acp.rs b/openab-agent/src/acp.rs index b3be526d6..ff3ae6c89 100644 --- a/openab-agent/src/acp.rs +++ b/openab-agent/src/acp.rs @@ -611,8 +611,14 @@ impl AcpServer { // Rebuild the current session's provider so the switch takes effect immediately if !session_id.is_empty() && self.sessions.contains_key(session_id) { + // Preserve the session's auth mode: an OAuth-forced session must not + // silently fall back to ANTHROPIC_API_KEY (which `auto_*` prefers). + let session_is_oauth = self.sessions[session_id].provider_is_oauth(); let new_provider: Result, String> = match provider_name { + "anthropic" if session_is_oauth => { + AnthropicProvider::from_oauth_store_with_model(value).map(|p| Box::new(p) as _) + } "anthropic" => AnthropicProvider::auto_with_model(value).map(|p| Box::new(p) as _), _ => crate::llm::OpenAiProvider::from_auth_store_with_model(value) .map(|p| Box::new(p) as _), diff --git a/openab-agent/src/agent.rs b/openab-agent/src/agent.rs index 7d50c005a..86ba368a1 100644 --- a/openab-agent/src/agent.rs +++ b/openab-agent/src/agent.rs @@ -107,6 +107,12 @@ impl Agent { self.provider = provider; } + /// True if the current provider authenticates via OAuth. Used on model + /// switch to rebuild with the same auth mode. + pub fn provider_is_oauth(&self) -> bool { + self.provider.is_oauth() + } + /// Update working directory and rebuild system prompt. pub fn set_working_dir(&mut self, cwd: String) { self.system_prompt = Self::build_system_prompt(&cwd, self.mcp_manager.as_ref()); diff --git a/openab-agent/src/auth.rs b/openab-agent/src/auth.rs index 960372e21..6083a7ba8 100644 --- a/openab-agent/src/auth.rs +++ b/openab-agent/src/auth.rs @@ -247,14 +247,20 @@ fn write_auth_file(path: &Path, map: &HashMap) -> Result<()> Ok(()) } -/// Load the LLM token stored under `namespace` (`codex` / `anthropic-oauth`). -pub fn load_tokens_for(namespace: &str) -> Result { - let path = auth_path(); - let cmd = if namespace == ANTHROPIC_NAMESPACE { +/// CLI subcommand that (re)authenticates a tenant `namespace`. Used in +/// credential-error messages so the user runs the right login. +fn auth_subcommand(namespace: &str) -> &'static str { + if namespace == ANTHROPIC_NAMESPACE { "openab-agent auth anthropic-oauth" } else { "openab-agent auth codex-oauth" - }; + } +} + +/// Load the LLM token stored under `namespace` (`codex` / `anthropic-oauth`). +pub fn load_tokens_for(namespace: &str) -> Result { + let path = auth_path(); + let cmd = auth_subcommand(namespace); // Preserve the underlying read/parse error for debugging. let map = read_auth_file(&path).map_err(|e| { anyhow!( @@ -428,7 +434,8 @@ async fn refresh_token(store: &TokenStore) -> Result { let status = resp.status(); let body = resp.text().await.unwrap_or_default(); return Err(anyhow!( - "Token refresh failed (HTTP {status}): {body}. Run `openab-agent auth` again." + "Token refresh failed (HTTP {status}): {body}. Run `{}` again.", + auth_subcommand(&store.provider) )); } let payload: serde_json::Value = resp.json().await?; diff --git a/openab-agent/src/llm.rs b/openab-agent/src/llm.rs index dcd5e21ad..7a18b4df3 100644 --- a/openab-agent/src/llm.rs +++ b/openab-agent/src/llm.rs @@ -68,6 +68,13 @@ pub trait LlmProvider: Send + Sync { /// `CreateMessageResult.model` when serving MCP sampling so the requesting /// server learns which model produced the response. fn model(&self) -> &str; + + /// True if this provider authenticates via OAuth rather than an API key. + /// Lets a session rebuild (model switch) preserve its auth mode instead of + /// silently falling back to `ANTHROPIC_API_KEY`. + fn is_oauth(&self) -> bool { + false + } } /// Shared, cloneable handle to an `LlmProvider`. A newtype over @@ -227,10 +234,6 @@ impl AnthropicProvider { Ok(p) } - fn is_oauth(&self) -> bool { - matches!(self.auth, AnthropicAuth::OAuth) - } - fn build_request_body(&self, system: &str, messages: &[Message], tools: &[ToolDef]) -> Value { let oauth = self.is_oauth(); let msgs: Vec = @@ -313,6 +316,10 @@ impl LlmProvider for AnthropicProvider { &self.model } + fn is_oauth(&self) -> bool { + matches!(self.auth, AnthropicAuth::OAuth) + } + fn chat<'a>( &'a self, system: &'a str, @@ -361,9 +368,10 @@ impl LlmProvider for AnthropicProvider { } // 401 on OAuth: token may have expired mid-request; force a - // refresh and retry once before surfacing the error. + // refresh and retry once. Surface a failed refresh instead of + // retrying with the stale token. if oauth && status.as_u16() == 401 && attempt < max_retries { - let _ = crate::auth::force_refresh_for(crate::auth::ANTHROPIC_NAMESPACE).await; + crate::auth::force_refresh_for(crate::auth::ANTHROPIC_NAMESPACE).await?; continue; } @@ -799,6 +807,14 @@ mod tests { } } + #[test] + fn test_is_oauth_reflects_auth_mode() { + // Guards the ACP model-switch rebuild: an OAuth session must report + // OAuth so it isn't silently rebuilt against ANTHROPIC_API_KEY. + assert!(test_provider(AnthropicAuth::OAuth).is_oauth()); + assert!(!test_provider(AnthropicAuth::ApiKey("k".to_string())).is_oauth()); + } + #[test] fn test_build_request_body() { let provider = test_provider(AnthropicAuth::ApiKey("test".to_string())); From d8fbe7862827a6357e9f88b514a7dbc0958580a4 Mon Sep 17 00:00:00 2001 From: Can Yu Date: Wed, 24 Jun 2026 22:55:24 +0800 Subject: [PATCH 06/20] =?UTF-8?q?fix:=20address=20review=20#8=20=E2=80=94?= =?UTF-8?q?=20always=20verify=20CSRF=20state=20on=20bare-code=20paste?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `--no-browser` bare-code paste defaulted the pasted state to the expected value when no `#state` was present, so the `st != state` check passed trivially and CSRF state was never verified. Require the `code#state` form (or a full redirect URL) and reject a bare code with a clear message. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01XHjGw1uBHXoVPB3dXYEy7h --- openab-agent/src/auth.rs | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/openab-agent/src/auth.rs b/openab-agent/src/auth.rs index 6083a7ba8..42412914c 100644 --- a/openab-agent/src/auth.rs +++ b/openab-agent/src/auth.rs @@ -715,11 +715,15 @@ pub async fn login_anthropic_browser_flow(no_browser: bool) -> Result<()> { if input.is_empty() { return Err(anyhow!("No URL provided")); } - // Accept either a full redirect URL or a bare `code` / `code#state`. + // Accept either a full redirect URL or a bare `code#state`. Require the + // `#state` form so CSRF state is always verified — a bare code can't be + // checked and is rejected rather than trusted. if let Ok(url) = url::Url::parse(input) { code_from_redirect(&url, &state)? } else { - let (code, st) = input.split_once('#').unwrap_or((input, state.as_str())); + let (code, st) = input.split_once('#').ok_or_else(|| { + anyhow!("Paste the full `code#state` value (or the redirect URL) so the state can be verified") + })?; if st != state { return Err(anyhow!("State mismatch")); } From b748c8f8b44211ad8dbfb337c5c30a181d63dd37 Mon Sep 17 00:00:00 2001 From: Can Yu Date: Thu, 25 Jun 2026 00:05:10 +0800 Subject: [PATCH 07/20] fix: don't pin a hardcoded Anthropic default model (review F4 follow-up) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Per @brettchien: dateless 4.6+ model IDs are fixed canonical IDs, not evergreen pointers, so a hardcoded default (claude-opus-4-8) is a per-generation 404 timebomb — the same failure that retired the previous claude-sonnet-4-20250514 default. It also silently bumped API-key users onto pricier Opus (review #5). Resolve the Anthropic model as: explicit override → OPENAB_AGENT_MODEL → error ("no model configured; set OPENAB_AGENT_MODEL or select a model"). - llm.rs: `anthropic_model()` is now fallible (no default); constructors refactored (`build`/`api_key_from_env`/`ensure_oauth_token`) so a model override never requires OPENAB_AGENT_MODEL, and credential errors still precede the model error. `auto()` only falls through to OAuth when no API key is present. - acp.rs: session new/load report the provider's resolved model instead of a hardcoded fallback. Removed the opus/gpt default sites. - Kept the claude-opus-4-8 entry in the model catalog (offering ≠ default). - docs/native-agent.md: document OPENAB_AGENT_MODEL is required for Anthropic (zero-config now fails loud). Behavior change: no zero-config default model. Deployments set it via env/values.yaml; local/zero-config users must export OPENAB_AGENT_MODEL. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01XHjGw1uBHXoVPB3dXYEy7h --- docs/native-agent.md | 1 + openab-agent/src/acp.rs | 79 +++++++++++++++++++++++++-------------- openab-agent/src/agent.rs | 6 +++ openab-agent/src/llm.rs | 69 +++++++++++++++++++++------------- 4 files changed, 99 insertions(+), 56 deletions(-) diff --git a/docs/native-agent.md b/docs/native-agent.md index be6b00f89..11e98bceb 100644 --- a/docs/native-agent.md +++ b/docs/native-agent.md @@ -31,6 +31,7 @@ env = { OPENAB_AGENT_OPENAI_MODEL = "gpt-5.4-mini" } | Variable | Default | Description | |----------|---------|-------------| +| `OPENAB_AGENT_MODEL` | — (required for Anthropic) | Anthropic model id (e.g. `claude-opus-4-8`). No hardcoded default — dateless 4.6+ IDs are fixed canonical IDs that retire each generation, so the agent fails loud if unset rather than pin a model that will eventually 404. | | `OPENAB_AGENT_OPENAI_MODEL` | `gpt-5.4-mini` | Model to use (must be supported by your ChatGPT plan — see [Supported Models](#supported-models-chatgpt-subscription)) | | `OPENAB_AGENT_OPENAI_BASE_URL` | `https://chatgpt.com/backend-api` | API base URL | | `OPENAB_AGENT_PROVIDER` | auto-detect | Force provider (`anthropic`, `openai`, `codex`) | diff --git a/openab-agent/src/acp.rs b/openab-agent/src/acp.rs index ff3ae6c89..57a444b9c 100644 --- a/openab-agent/src/acp.rs +++ b/openab-agent/src/acp.rs @@ -366,27 +366,13 @@ impl AcpServer { } }; + // The provider already resolved its model (explicit override → + // OPENAB_AGENT_MODEL, validated at construction). Use it as the + // authoritative reported model instead of a separate hardcoded default. + let model_name = provider.model().to_string(); let agent = Agent::new_boxed(provider, self.working_dir.clone(), self.mcp_manager.clone()); self.sessions.insert(session_id.clone(), agent); - let model_name = self - .active_model - .clone() - .or_else(|| { - if active_provider == "openai" { - std::env::var("OPENAB_AGENT_OPENAI_MODEL").ok() - } else { - None - } - }) - .or_else(|| std::env::var("OPENAB_AGENT_MODEL").ok()) - .unwrap_or_else(|| { - if active_provider == "anthropic" { - "claude-opus-4-8".to_string() - } else { - "gpt-5.4-mini".to_string() - } - }); self.active_model = Some(model_name.clone()); self.active_provider = Some(active_provider.to_string()); self.model_options = Self::available_models().await; @@ -439,13 +425,11 @@ impl AcpServer { self.model_options = Self::available_models().await; } - let model_name = self.active_model.clone().unwrap_or_else(|| { - if self.active_provider.as_deref() == Some("openai") { - "gpt-5.4-mini".to_string() - } else { - "claude-opus-4-8".to_string() - } - }); + // Report the loaded session's actual model (no hardcoded default). + let model_name = self + .active_model + .clone() + .unwrap_or_else(|| self.sessions[session_id].provider_model()); self.ok_response( id, @@ -699,10 +683,14 @@ mod tests { #[tokio::test] async fn test_session_new() { let _guard = ENV_LOCK.lock().unwrap(); - // Set a fake key so from_env() succeeds in CI - unsafe { std::env::set_var("ANTHROPIC_API_KEY", "test-key") }; + // Set a fake key + model so provider construction succeeds in CI + unsafe { + std::env::set_var("ANTHROPIC_API_KEY", "test-key"); + std::env::set_var("OPENAB_AGENT_MODEL", "claude-sonnet-4-6"); + } let mut server = AcpServer::new(); let resp_str = server.handle_session_new(2).await; + unsafe { std::env::remove_var("OPENAB_AGENT_MODEL") }; let resp: Value = serde_json::from_str(&resp_str).unwrap(); assert_eq!(resp["jsonrpc"], "2.0"); assert_eq!(resp["id"], 2); @@ -712,6 +700,7 @@ mod tests { assert!(!config_options.is_empty()); assert_eq!(config_options[0]["id"], "model"); assert_eq!(config_options[0]["category"], "model"); + assert_eq!(config_options[0]["currentValue"], "claude-sonnet-4-6"); assert!(!config_options[0]["options"].as_array().unwrap().is_empty()); } @@ -809,6 +798,30 @@ mod tests { .contains("ANTHROPIC_API_KEY")); } + #[tokio::test] + async fn test_session_new_requires_model() { + // No hardcoded default: a forced anthropic provider without + // OPENAB_AGENT_MODEL must fail loud. + let _guard = ENV_LOCK.lock().unwrap(); + unsafe { + std::env::set_var("OPENAB_AGENT_PROVIDER", "anthropic"); + std::env::set_var("ANTHROPIC_API_KEY", "test-key"); + std::env::remove_var("OPENAB_AGENT_MODEL"); + } + let mut server = AcpServer::new(); + let resp_str = server.handle_session_new(7).await; + unsafe { + std::env::remove_var("ANTHROPIC_API_KEY"); + std::env::remove_var("OPENAB_AGENT_PROVIDER"); + } + let resp: Value = serde_json::from_str(&resp_str).unwrap(); + assert!(resp["error"].is_object()); + assert!(resp["error"]["message"] + .as_str() + .unwrap() + .contains("no model configured")); + } + #[test] fn test_set_config_option_accepts_cached_dynamic_model() { let mut server = AcpServer::new(); @@ -867,11 +880,15 @@ mod tests { #[tokio::test] async fn test_model_switch_preserves_session_history() { let _guard = ENV_LOCK.lock().unwrap(); - unsafe { std::env::set_var("ANTHROPIC_API_KEY", "test-key") }; + unsafe { + std::env::set_var("ANTHROPIC_API_KEY", "test-key"); + std::env::set_var("OPENAB_AGENT_MODEL", "claude-sonnet-4-6"); + } let mut server = AcpServer::new(); // Create a session let resp_str = server.handle_session_new(10).await; + unsafe { std::env::remove_var("OPENAB_AGENT_MODEL") }; let resp: Value = serde_json::from_str(&resp_str).unwrap(); let session_id = resp["result"]["sessionId"].as_str().unwrap().to_string(); @@ -974,11 +991,15 @@ mod tests { #[tokio::test] async fn test_session_load_returns_config_options() { let _guard = ENV_LOCK.lock().unwrap(); - unsafe { std::env::set_var("ANTHROPIC_API_KEY", "test-key") }; + unsafe { + std::env::set_var("ANTHROPIC_API_KEY", "test-key"); + std::env::set_var("OPENAB_AGENT_MODEL", "claude-sonnet-4-6"); + } let mut server = AcpServer::new(); // Create a session first let new_resp_str = server.handle_session_new(10).await; + unsafe { std::env::remove_var("OPENAB_AGENT_MODEL") }; let new_resp: Value = serde_json::from_str(&new_resp_str).unwrap(); let session_id = new_resp["result"]["sessionId"].as_str().unwrap(); diff --git a/openab-agent/src/agent.rs b/openab-agent/src/agent.rs index 86ba368a1..cc7c5fb53 100644 --- a/openab-agent/src/agent.rs +++ b/openab-agent/src/agent.rs @@ -113,6 +113,12 @@ impl Agent { self.provider.is_oauth() } + /// The model id the current provider will use. Authoritative source for the + /// session's reported model (avoids a separate hardcoded default). + pub fn provider_model(&self) -> String { + self.provider.model().to_string() + } + /// Update working directory and rebuild system prompt. pub fn set_working_dir(&mut self, cwd: String) { self.system_prompt = Self::build_system_prompt(&cwd, self.mcp_manager.as_ref()); diff --git a/openab-agent/src/llm.rs b/openab-agent/src/llm.rs index 7a18b4df3..859ea071d 100644 --- a/openab-agent/src/llm.rs +++ b/openab-agent/src/llm.rs @@ -149,8 +149,13 @@ pub struct AnthropicProvider { client: reqwest::Client, } -fn anthropic_model_from_env() -> String { - std::env::var("OPENAB_AGENT_MODEL").unwrap_or_else(|_| "claude-opus-4-8".to_string()) +/// Resolve the Anthropic model from `OPENAB_AGENT_MODEL`. No hardcoded default: +/// dateless 4.6+ IDs are fixed canonical IDs (not evergreen pointers), so a +/// pinned default is a per-generation 404 timebomb. Require an explicit choice +/// and fail loud instead. +fn anthropic_model() -> Result { + std::env::var("OPENAB_AGENT_MODEL") + .map_err(|_| "no model configured; set OPENAB_AGENT_MODEL or select a model".to_string()) } fn anthropic_max_tokens() -> u32 { @@ -188,50 +193,60 @@ fn from_claude_code_name(name: &str) -> String { } impl AnthropicProvider { - pub fn from_env() -> Result { + fn build(auth: AnthropicAuth, model: String) -> Self { + Self { + auth, + model, + max_tokens: anthropic_max_tokens(), + client: reqwest::Client::new(), + } + } + + fn api_key_from_env() -> Result { let api_key = std::env::var("ANTHROPIC_API_KEY") .map_err(|_| "ANTHROPIC_API_KEY not set".to_string())?; if api_key.is_empty() { return Err("ANTHROPIC_API_KEY is empty".to_string()); } - Ok(Self { - auth: AnthropicAuth::ApiKey(api_key), - model: anthropic_model_from_env(), - max_tokens: anthropic_max_tokens(), - client: reqwest::Client::new(), - }) + Ok(api_key) } - /// Claude Pro/Max OAuth. Verifies a stored `anthropic-oauth` token exists; - /// the live token is fetched (and refreshed) at call time. - pub fn from_oauth_store() -> Result { + /// Verify the `anthropic-oauth` tenant has a stored token; the live token is + /// fetched (and refreshed) at call time. + fn ensure_oauth_token() -> Result<(), String> { crate::auth::load_tokens_for(crate::auth::ANTHROPIC_NAMESPACE) - .map_err(|e| e.to_string())?; - Ok(Self { - auth: AnthropicAuth::OAuth, - model: anthropic_model_from_env(), - max_tokens: anthropic_max_tokens(), - client: reqwest::Client::new(), - }) + .map(|_| ()) + .map_err(|e| e.to_string()) + } + + /// Claude Pro/Max OAuth. + pub fn from_oauth_store() -> Result { + Self::ensure_oauth_token()?; + Ok(Self::build(AnthropicAuth::OAuth, anthropic_model()?)) } /// Prefer an explicit API key, else a stored Claude subscription OAuth token. + /// When a key is present its own errors (e.g. missing model) surface rather + /// than falling through to an unrelated OAuth-token error. pub fn auto() -> Result { - Self::from_env().or_else(|_| Self::from_oauth_store()) + match Self::api_key_from_env() { + Ok(key) => Ok(Self::build(AnthropicAuth::ApiKey(key), anthropic_model()?)), + Err(_) => Self::from_oauth_store(), + } } - /// `auto()` with an explicit model override. + /// `auto()` with an explicit model override. The override replaces + /// `OPENAB_AGENT_MODEL`, so it does not require that env var to be set. pub fn auto_with_model(model: &str) -> Result { - let mut p = Self::auto()?; - p.model = model.to_string(); - Ok(p) + Self::api_key_from_env() + .map(|key| Self::build(AnthropicAuth::ApiKey(key), model.to_string())) + .or_else(|_| Self::from_oauth_store_with_model(model)) } /// `from_oauth_store()` with an explicit model override. pub fn from_oauth_store_with_model(model: &str) -> Result { - let mut p = Self::from_oauth_store()?; - p.model = model.to_string(); - Ok(p) + Self::ensure_oauth_token()?; + Ok(Self::build(AnthropicAuth::OAuth, model.to_string())) } fn build_request_body(&self, system: &str, messages: &[Message], tools: &[ToolDef]) -> Value { From 3e9ac45cc1d57aaa53a2d32e279dffa46fd0e7d2 Mon Sep 17 00:00:00 2001 From: Can Yu Date: Thu, 25 Jun 2026 02:34:03 +0800 Subject: [PATCH 08/20] docs: fix stale PKCE state comment (review F5) The doc comment on login_anthropic_browser_flow still said the verifier doubles as `state` (Pi's old convention); since the PKCE fix the state is an independent 32-byte random value. Correct the comment to match. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01XHjGw1uBHXoVPB3dXYEy7h --- openab-agent/src/auth.rs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/openab-agent/src/auth.rs b/openab-agent/src/auth.rs index 42412914c..a30292947 100644 --- a/openab-agent/src/auth.rs +++ b/openab-agent/src/auth.rs @@ -689,8 +689,9 @@ fn accept_callback_code(listener: &TcpListener, expected_state: &str) -> Result< Ok(code) } -/// Anthropic OAuth (Claude Pro/Max). PKCE with the verifier doubling as `state` -/// (Pi's convention) and a JSON token exchange against `platform.claude.com`. +/// Anthropic OAuth (Claude Pro/Max). PKCE with an independent random CSRF +/// `state` (verifier stays back-channel-only) and a JSON token exchange against +/// `platform.claude.com`. pub async fn login_anthropic_browser_flow(no_browser: bool) -> Result<()> { let (verifier, challenge) = generate_pkce(); // Independent random CSRF state — keep the PKCE verifier back-channel-only. From 90cc60e0fdfa016780232ec4dd7ee5cd5c8a5c4d Mon Sep 17 00:00:00 2001 From: Can Yu Date: Thu, 25 Jun 2026 12:19:31 +0800 Subject: [PATCH 09/20] feat(native-agent): accept canonical provider/model in OPENAB_AGENT_MODEL ModelRef::parse + resolve_provider_choice let OPENAB_AGENT_MODEL carry `provider/model_id` (e.g. anthropic/claude-sonnet-4-6) as a single source of truth for both provider and model. Bare model ids and the existing OPENAB_AGENT_PROVIDER var remain fully backward compatible. Co-Authored-By: Claude Opus 4.8 (1M context) --- openab-agent/src/acp.rs | 3 +- openab-agent/src/llm.rs | 82 ++++++++++++++++++++++++++++++++++++++--- 2 files changed, 77 insertions(+), 8 deletions(-) diff --git a/openab-agent/src/acp.rs b/openab-agent/src/acp.rs index 57a444b9c..615602697 100644 --- a/openab-agent/src/acp.rs +++ b/openab-agent/src/acp.rs @@ -307,8 +307,7 @@ impl AcpServer { let provider_choice = self .active_provider .clone() - .or_else(|| std::env::var("OPENAB_AGENT_PROVIDER").ok()) - .unwrap_or_default(); + .unwrap_or_else(crate::llm::resolve_provider_choice); let model_override = self.active_model.as_deref(); let (provider, active_provider): (Box, &str) = match provider_choice.as_str() { diff --git a/openab-agent/src/llm.rs b/openab-agent/src/llm.rs index 859ea071d..a854fd21e 100644 --- a/openab-agent/src/llm.rs +++ b/openab-agent/src/llm.rs @@ -97,6 +97,44 @@ impl std::ops::Deref for SharedLlmProvider { } } +/// A model reference, optionally provider-qualified. Accepts the canonical +/// `provider/model_id` form (e.g. `anthropic/claude-sonnet-4-6`) as well as a +/// bare `model_id` (provider then inferred from credentials). Model IDs never +/// contain `/`, so the first `/` cleanly separates the two. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct ModelRef { + pub provider: Option, + pub model: String, +} + +impl ModelRef { + pub fn parse(input: &str) -> Self { + match input.split_once('/') { + Some((p, m)) if !p.is_empty() && !m.is_empty() => ModelRef { + provider: Some(p.to_string()), + model: m.to_string(), + }, + _ => ModelRef { + provider: None, + model: input.to_string(), + }, + } + } +} + +/// The provider the user asked for: explicit `OPENAB_AGENT_PROVIDER`, else the +/// `provider/` prefix of `OPENAB_AGENT_MODEL` (e.g. `openai/gpt-5.4` selects +/// OpenAI even when an Anthropic key is also present), else empty (auto-detect). +pub fn resolve_provider_choice() -> String { + match std::env::var("OPENAB_AGENT_PROVIDER") { + Ok(p) if !p.is_empty() => p, + _ => std::env::var("OPENAB_AGENT_MODEL") + .ok() + .and_then(|m| ModelRef::parse(&m).provider) + .unwrap_or_default(), + } +} + /// Select an `LlmProvider` from an explicit `choice` (`anthropic` / /// `anthropic-oauth` / `openai` / `codex`) or, for any other value, auto-detect /// (Anthropic API key, then Claude subscription OAuth, then codex OAuth). The @@ -125,7 +163,7 @@ pub fn select_provider(choice: &str) -> Result, String> { /// credentials are available so the caller can simply decline to advertise /// the `sampling` capability rather than fail. pub fn default_provider() -> Option { - let choice = std::env::var("OPENAB_AGENT_PROVIDER").unwrap_or_default(); + let choice = resolve_provider_choice(); select_provider(&choice) .ok() .map(|b| SharedLlmProvider(Arc::from(b))) @@ -196,7 +234,9 @@ impl AnthropicProvider { fn build(auth: AnthropicAuth, model: String) -> Self { Self { auth, - model, + // Accept a provider-qualified ref (`anthropic/claude-…`); the API + // wants the bare model id. + model: ModelRef::parse(&model).model, max_tokens: anthropic_max_tokens(), client: reqwest::Client::new(), } @@ -481,9 +521,12 @@ impl OpenAiProvider { Ok(Self { base_url: std::env::var("OPENAB_AGENT_OPENAI_BASE_URL") .unwrap_or_else(|_| "https://chatgpt.com/backend-api".to_string()), - model: std::env::var("OPENAB_AGENT_OPENAI_MODEL") - .or_else(|_| std::env::var("OPENAB_AGENT_MODEL")) - .unwrap_or_else(|_| "gpt-5.4-mini".to_string()), + model: ModelRef::parse( + &std::env::var("OPENAB_AGENT_OPENAI_MODEL") + .or_else(|_| std::env::var("OPENAB_AGENT_MODEL")) + .unwrap_or_else(|_| "gpt-5.4-mini".to_string()), + ) + .model, max_tokens: std::env::var("OPENAB_AGENT_MAX_TOKENS") .ok() .and_then(|v| v.parse().ok()) @@ -495,7 +538,7 @@ impl OpenAiProvider { /// Create provider with a specific model override. pub fn from_auth_store_with_model(model: &str) -> Result { let mut p = Self::from_auth_store()?; - p.model = model.to_string(); + p.model = ModelRef::parse(model).model; Ok(p) } } @@ -778,6 +821,33 @@ fn parse_openai_response(response: &Value) -> Result> { mod tests { use super::*; + #[test] + fn test_model_ref_parse() { + // Provider-qualified form splits on the first slash. + let r = ModelRef::parse("anthropic/claude-sonnet-4-6"); + assert_eq!(r.provider.as_deref(), Some("anthropic")); + assert_eq!(r.model, "claude-sonnet-4-6"); + + // Bare model id → no provider, model unchanged. + let r = ModelRef::parse("claude-sonnet-4-6"); + assert_eq!(r.provider, None); + assert_eq!(r.model, "claude-sonnet-4-6"); + + // Degenerate slashes fall back to bare (no empty provider/model). + assert_eq!(ModelRef::parse("/gpt-5.4").provider, None); + assert_eq!(ModelRef::parse("openai/").model, "openai/"); + } + + #[test] + fn test_provider_build_strips_prefix() { + // A qualified ref reaches the API as the bare model id. + let p = AnthropicProvider::build( + AnthropicAuth::ApiKey("k".to_string()), + "anthropic/claude-opus-4-8".to_string(), + ); + assert_eq!(p.model(), "claude-opus-4-8"); + } + #[test] fn test_parse_text_response() { let resp = json!({ From f9475b066c944417d81ba8f55ec7654b4e94f2ba Mon Sep 17 00:00:00 2001 From: Can Yu Date: Thu, 25 Jun 2026 17:26:30 +0800 Subject: [PATCH 10/20] fix: satisfy clippy::manual_is_multiple_of (Rust 1.96 stable) Stable clippy 1.96 added manual_is_multiple_of; pre-existing modulo checks in openab-core (format.rs, pre_seed.rs) and openab-gateway (wecom.rs) fail `clippy --workspace -D warnings`. Mechanical fix; unblocks CI. Unrelated to the OAuth change. Co-Authored-By: Claude Opus 4.8 (1M context) --- crates/openab-core/src/format.rs | 2 +- crates/openab-core/src/pre_seed.rs | 4 ++-- crates/openab-gateway/src/adapters/wecom.rs | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/crates/openab-core/src/format.rs b/crates/openab-core/src/format.rs index d39410f15..3c5f3ea4f 100644 --- a/crates/openab-core/src/format.rs +++ b/crates/openab-core/src/format.rs @@ -319,7 +319,7 @@ mod tests { for (i, chunk) in chunks.iter().enumerate() { let fence_count = chunk.lines().filter(|l| l.starts_with("```")).count(); assert!( - fence_count % 2 == 0, + fence_count.is_multiple_of(2), "chunk {i} has unbalanced fences ({fence_count}):\n{chunk}" ); } diff --git a/crates/openab-core/src/pre_seed.rs b/crates/openab-core/src/pre_seed.rs index c5695391e..8eee27874 100644 --- a/crates/openab-core/src/pre_seed.rs +++ b/crates/openab-core/src/pre_seed.rs @@ -222,7 +222,7 @@ fn extract_zip_budgeted( for i in 0..file_count { // Cooperative deadline check per file - if i % 100 == 0 && Instant::now() >= deadline { + if i.is_multiple_of(100) && Instant::now() >= deadline { anyhow::bail!("hooks.pre_seed: timed out during extraction at entry {i}"); } @@ -286,7 +286,7 @@ fn extract_tarball_with_limits(data: &[u8], dest: &Path, deadline: Instant) -> a } // Cooperative deadline check every 10 files - if file_count % 10 == 0 && Instant::now() >= deadline { + if file_count.is_multiple_of(10) && Instant::now() >= deadline { anyhow::bail!("hooks.pre_seed: timed out during tarball extraction at entry {file_count}"); } diff --git a/crates/openab-gateway/src/adapters/wecom.rs b/crates/openab-gateway/src/adapters/wecom.rs index 7b96c27f8..b5c8ae5fb 100644 --- a/crates/openab-gateway/src/adapters/wecom.rs +++ b/crates/openab-gateway/src/adapters/wecom.rs @@ -137,7 +137,7 @@ fn decrypt_message( .decode(encrypted) .map_err(|e| anyhow::anyhow!("base64 decode failed: {e}"))?; - if cipher_bytes.is_empty() || cipher_bytes.len() % 16 != 0 { + if cipher_bytes.is_empty() || !cipher_bytes.len().is_multiple_of(16) { anyhow::bail!("ciphertext length {} not a multiple of 16", cipher_bytes.len()); } From f9381d700a54ea2a355f239b8b9c1f823bcea08c Mon Sep 17 00:00:00 2001 From: Can Yu Date: Fri, 26 Jun 2026 12:32:57 +0800 Subject: [PATCH 11/20] fix: limit OAuth 401 force-refresh to one retry MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The comment said "retry once" but `attempt < max_retries` allowed up to 3 consecutive force-refreshes on persistent 401s — wasting round-trips and risking rotating a still-valid refresh token off the server's grace window. Use an `oauth_refreshed` flag to match the documented intent. Co-Authored-By: Claude Opus 4.6 --- openab-agent/src/llm.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/openab-agent/src/llm.rs b/openab-agent/src/llm.rs index a854fd21e..edc4c3258 100644 --- a/openab-agent/src/llm.rs +++ b/openab-agent/src/llm.rs @@ -385,6 +385,7 @@ impl LlmProvider for AnthropicProvider { let body = self.build_request_body(system, messages, tools); let oauth = self.is_oauth(); let max_retries = 3u32; + let mut oauth_refreshed = false; for attempt in 0..=max_retries { let mut req = self @@ -425,7 +426,8 @@ impl LlmProvider for AnthropicProvider { // 401 on OAuth: token may have expired mid-request; force a // refresh and retry once. Surface a failed refresh instead of // retrying with the stale token. - if oauth && status.as_u16() == 401 && attempt < max_retries { + if oauth && status.as_u16() == 401 && !oauth_refreshed { + oauth_refreshed = true; crate::auth::force_refresh_for(crate::auth::ANTHROPIC_NAMESPACE).await?; continue; } From d4d2e0bf71c1b10bb1ffaafd64964f35d6fcf74f Mon Sep 17 00:00:00 2001 From: Can Date: Sat, 27 Jun 2026 06:09:43 +0000 Subject: [PATCH 12/20] =?UTF-8?q?feat(native-agent):=20introduce=20OAuthVe?= =?UTF-8?q?ndor=20descriptor=20abstraction=20(ADR=20=C2=A75.1)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Collapse the hand-rolled per-vendor OAuth surface into a single OAuthVendor trait + CodexVendor/AnthropicVendor descriptors. Adding a subscription-OAuth vendor is now a descriptor, not a new hand-rolled flow: - Unify the codex + anthropic PKCE login flows into one shared `login_pkce_flow` driven by the descriptor; fold the codex flow into the `accept_callback_code` / `code_from_redirect` helpers (the long-standing TODO) and unify the 127.0.0.1 bind. - Drive `refresh_token` and the authorization-code exchange off `vendor.token_body()` instead of an `if provider == ANTHROPIC_NAMESPACE` branch. - Shared pure builders `build_authorize_url` / `token_store_from_payload`, pinned by wire-format unit tests — the login authorize-URL/exchange hit live OAuth servers, so no integration test covers them. The driver keeps the proven reqwest flows; swapping the engine onto `oauth2::BasicClient` (as `mcp/runtime.rs` already does via a custom http hook) is a follow-up internal change invisible to vendor authors — only the descriptor surface lands here. `client_secret()`/`grant()` are the ADR §5.1 surface for later vendors (gemini/agy bundled secret; copilot/kiro device-code). 205 tests pass (4 new wire-format locks). Co-Authored-By: Claude Opus 4.8 (1M context) --- openab-agent/src/auth.rs | 666 +++++++++++++++++++++------------------ 1 file changed, 367 insertions(+), 299 deletions(-) diff --git a/openab-agent/src/auth.rs b/openab-agent/src/auth.rs index 0eac98f0a..864bda05f 100644 --- a/openab-agent/src/auth.rs +++ b/openab-agent/src/auth.rs @@ -33,35 +33,247 @@ const ANTHROPIC_REDIRECT_PORT: u16 = 53692; const ANTHROPIC_SCOPE: &str = "org:create_api_key user:profile user:inference user:sessions:claude_code user:mcp_servers user:file_upload"; -fn codex_client_id() -> String { - std::env::var("OPENAB_AGENT_OAUTH_CLIENT_ID") - .unwrap_or_else(|_| "app_EMoamEEZ73f0CkXaXp7hrann".to_string()) +// ── OAuthVendor (auth axis — ADR §5.1) ────────────────────────────────────── +// +// A subscription-OAuth provider is one static `OAuthVendor` descriptor; the +// shared driver below (`build_authorize_url`, `exchange_authorization_code`, +// `refresh_token`) does PKCE/CSRF/exchange/refresh by reading the descriptor, so +// adding a vendor is a new descriptor — not a new hand-rolled flow. Token bodies +// and a few authorize-URL quirks are the only per-vendor variation, expressed as +// trait methods rather than forked code paths. +// +// NOTE (ADR §4.2): the ADR specifies building this driver on the official +// `oauth2` crate (as `mcp/runtime.rs` already does via `BasicClient` + a custom +// reqwest http hook). This pass keeps the proven reqwest flows and only +// parameterises them by descriptor; swapping the engine onto `oauth2::BasicClient` +// is a follow-up internal change invisible to vendor authors (the descriptor +// surface is unchanged). The device-code grant (non-standard `device_auth_id`) +// and Anthropic's JSON token body are why the swap is staged, not done blind. + +/// Token-request body encoding. Codex/OpenAI use form-encoding; Anthropic's AS +/// takes JSON (and rejects a `scope` field on refresh — Pi #2169). +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +enum TokenBodyFormat { + Form, + Json, } -fn anthropic_client_id() -> String { - std::env::var("OPENAB_AGENT_ANTHROPIC_CLIENT_ID") - .unwrap_or_else(|_| "9d1c250a-e61b-44d9-88ed-5944d1962f5e".to_string()) +/// OAuth grant a vendor's *primary* login uses. Codex additionally exposes a +/// device-code subcommand, but its browser login — like Anthropic's — is PKCE. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +#[allow(dead_code)] // `DeviceCode` lands with the first device-primary vendor (copilot/kiro). +enum AuthGrant { + Pkce, + DeviceCode, } -fn redirect_uri() -> String { - format!("http://localhost:{REDIRECT_PORT}/auth/callback") +/// Static per-vendor OAuth descriptor (ADR §5.1, auth axis). Signatures mirror +/// the ADR verbatim so future vendors (gemini/grok/agy) slot in as descriptors. +/// `Send + Sync` so a boxed vendor can be held across the refresh `await` inside +/// the `Send` provider futures. +trait OAuthVendor: Send + Sync { + /// `auth.json` tenant key (`codex` / `anthropic-oauth` / …). + fn namespace(&self) -> &str; + fn client_id(&self) -> String; + /// Bundled installed-app secret (gemini/agy); `None` for public PKCE clients. + /// ADR §5.1 surface — first consumer is the gemini/agy vendor (encode-at-rest + /// per §9 Q2); unused until then. + #[allow(dead_code)] + fn client_secret(&self) -> Option { + None + } + fn authorize_url(&self) -> &str; + fn token_url(&self) -> &str; + /// Loopback `(port, path)` for PKCE; `None` for device flow (no redirect endpoint). + fn redirect(&self) -> Option<(u16, &'static str)> { + None + } + fn scope(&self) -> &str; + /// Extra authorize-URL query params (Codex's simplified-flow hints; Anthropic's `code=true`). + fn extra_authorize_params(&self) -> &'static [(&'static str, &'static str)] { + &[] + } + fn token_body(&self) -> TokenBodyFormat { + TokenBodyFormat::Form + } + /// ADR §5.1 surface — `DeviceCode` lands with the first device-primary vendor + /// (copilot/kiro); both current vendors log in via PKCE, so unused until then. + #[allow(dead_code)] + fn grant(&self) -> AuthGrant { + AuthGrant::Pkce + } + /// Full loopback redirect URI, derived from `redirect()`. + fn redirect_uri(&self) -> Option { + self.redirect() + .map(|(port, path)| format!("http://localhost:{port}{path}")) + } } -fn anthropic_redirect_uri() -> String { - format!("http://localhost:{ANTHROPIC_REDIRECT_PORT}/callback") +struct CodexVendor; +impl OAuthVendor for CodexVendor { + fn namespace(&self) -> &str { + CODEX_NAMESPACE + } + fn client_id(&self) -> String { + std::env::var("OPENAB_AGENT_OAUTH_CLIENT_ID") + .unwrap_or_else(|_| "app_EMoamEEZ73f0CkXaXp7hrann".to_string()) + } + fn authorize_url(&self) -> &str { + CODEX_AUTHORIZE_URL + } + fn token_url(&self) -> &str { + CODEX_TOKEN_URL + } + fn redirect(&self) -> Option<(u16, &'static str)> { + Some((REDIRECT_PORT, "/auth/callback")) + } + fn scope(&self) -> &str { + "openid profile email offline_access" + } + fn extra_authorize_params(&self) -> &'static [(&'static str, &'static str)] { + &[ + ("id_token_add_organizations", "true"), + ("codex_cli_simplified_flow", "true"), + ("originator", "openab-agent"), + ] + } } -/// Build the Anthropic authorize URL. Pure so it can be unit-tested. `state` is -/// an independent random CSRF value (kept distinct from the PKCE verifier, which +struct AnthropicVendor; +impl OAuthVendor for AnthropicVendor { + fn namespace(&self) -> &str { + ANTHROPIC_NAMESPACE + } + fn client_id(&self) -> String { + std::env::var("OPENAB_AGENT_ANTHROPIC_CLIENT_ID") + .unwrap_or_else(|_| "9d1c250a-e61b-44d9-88ed-5944d1962f5e".to_string()) + } + fn authorize_url(&self) -> &str { + ANTHROPIC_AUTHORIZE_URL + } + fn token_url(&self) -> &str { + ANTHROPIC_TOKEN_URL + } + fn redirect(&self) -> Option<(u16, &'static str)> { + Some((ANTHROPIC_REDIRECT_PORT, "/callback")) + } + fn scope(&self) -> &str { + ANTHROPIC_SCOPE + } + fn extra_authorize_params(&self) -> &'static [(&'static str, &'static str)] { + &[("code", "true")] + } + fn token_body(&self) -> TokenBodyFormat { + TokenBodyFormat::Json + } +} + +/// Resolve a vendor descriptor by `auth.json` namespace. `None` for non-OAuth +/// tenants (e.g. `mcp:`, whose refresh rmcp owns). +fn vendor_for(namespace: &str) -> Option> { + match namespace { + CODEX_NAMESPACE => Some(Box::new(CodexVendor)), + ANTHROPIC_NAMESPACE => Some(Box::new(AnthropicVendor)), + _ => None, + } +} + +/// Build a vendor's PKCE authorize URL. Pure (unit-testable). `state` is an +/// independent random CSRF value kept distinct from the PKCE verifier (which /// stays back-channel-only) — the AS just echoes it back. -fn anthropic_authorize_url(challenge: &str, state: &str) -> String { - let client_id = anthropic_client_id(); - let redirect = anthropic_redirect_uri(); +fn build_authorize_url(vendor: &dyn OAuthVendor, challenge: &str, state: &str) -> Result { + let redirect = vendor.redirect_uri().ok_or_else(|| { + anyhow!( + "{} has no loopback redirect (not a PKCE vendor)", + vendor.namespace() + ) + })?; let redir = urlencoding::encode(&redirect); - let scope = urlencoding::encode(ANTHROPIC_SCOPE); - format!( - "{ANTHROPIC_AUTHORIZE_URL}?code=true&client_id={client_id}&response_type=code&redirect_uri={redir}&scope={scope}&code_challenge={challenge}&code_challenge_method=S256&state={state}" - ) + let scope = urlencoding::encode(vendor.scope()); + let client_id = vendor.client_id(); + let mut url = format!( + "{}?client_id={client_id}&response_type=code&redirect_uri={redir}&scope={scope}&code_challenge={challenge}&code_challenge_method=S256&state={state}", + vendor.authorize_url() + ); + for (k, v) in vendor.extra_authorize_params() { + url.push('&'); + url.push_str(k); + url.push('='); + url.push_str(v); + } + Ok(url) +} + +/// Exchange an authorization `code` for tokens against `vendor`, encoding the +/// body per `token_body()`. The JSON path also carries `state` (Anthropic +/// echoes it); the form path omits it (Codex). +async fn exchange_authorization_code( + vendor: &dyn OAuthVendor, + code: &str, + state: &str, + verifier: &str, +) -> Result { + let redirect = vendor + .redirect_uri() + .ok_or_else(|| anyhow!("{} has no loopback redirect", vendor.namespace()))?; + let client_id = vendor.client_id(); + let client = reqwest::Client::new(); + let req = client.post(vendor.token_url()); + let resp = match vendor.token_body() { + TokenBodyFormat::Json => { + req.json(&serde_json::json!({ + "grant_type": "authorization_code", + "client_id": client_id, + "code": code, + "state": state, + "redirect_uri": redirect, + "code_verifier": verifier, + })) + .send() + .await? + } + TokenBodyFormat::Form => { + req.form(&[ + ("grant_type", "authorization_code"), + ("client_id", client_id.as_str()), + ("code", code), + ("code_verifier", verifier), + ("redirect_uri", redirect.as_str()), + ]) + .send() + .await? + } + }; + if !resp.status().is_success() { + let body = resp.text().await.unwrap_or_default(); + return Err(anyhow!("Token exchange failed: {body}")); + } + let payload: serde_json::Value = resp.json().await?; + token_store_from_payload(&payload, vendor.token_url(), vendor.namespace()) +} + +/// Build a `TokenStore` from an OAuth token response, requiring `access_token` +/// and `refresh_token`. Shared by every login + exchange path. +fn token_store_from_payload( + payload: &serde_json::Value, + token_endpoint: &str, + provider: &str, +) -> Result { + let access_token = payload["access_token"] + .as_str() + .ok_or_else(|| anyhow!("No access_token"))?; + let refresh_token_val = payload["refresh_token"] + .as_str() + .ok_or_else(|| anyhow!("No refresh_token"))?; + let expires_in = payload["expires_in"].as_u64().unwrap_or(3600); + let now = SystemTime::now().duration_since(UNIX_EPOCH)?.as_secs(); + Ok(TokenStore { + access_token: access_token.to_string(), + refresh_token: refresh_token_val.to_string(), + expires_at: now + expires_in, + token_endpoint: token_endpoint.to_string(), + provider: provider.to_string(), + }) } #[derive(Debug, Clone, Serialize, Deserialize)] @@ -718,34 +930,36 @@ pub async fn force_refresh() -> Result { } async fn refresh_token(store: &TokenStore) -> Result { + let vendor = vendor_for(&store.provider) + .ok_or_else(|| anyhow!("No OAuth vendor for provider `{}`", store.provider))?; + let client_id = vendor.client_id(); // Bound the refresh so the per-tenant lock (held across this call) is provably // released before another process's lock deadline — see REFRESH_HTTP_TIMEOUT. let client = reqwest::Client::builder() .timeout(REFRESH_HTTP_TIMEOUT) .build()?; - // Anthropic's token endpoint takes a JSON body and rejects a `scope` field - // on refresh (Pi #2169); Codex takes a form body. Branch on the stored - // provider so each tenant refreshes the way its AS expects. - let resp = if store.provider == ANTHROPIC_NAMESPACE { - client - .post(&store.token_endpoint) - .json(&serde_json::json!({ + // Body encoding comes from the vendor descriptor: Anthropic takes JSON (and + // rejects a `scope` field on refresh — Pi #2169); Codex takes a form body. + let req = client.post(&store.token_endpoint); + let resp = match vendor.token_body() { + TokenBodyFormat::Json => { + req.json(&serde_json::json!({ "grant_type": "refresh_token", "refresh_token": store.refresh_token, - "client_id": anthropic_client_id(), + "client_id": client_id, })) .send() .await? - } else { - client - .post(&store.token_endpoint) - .form(&[ + } + TokenBodyFormat::Form => { + req.form(&[ ("grant_type", "refresh_token"), ("refresh_token", store.refresh_token.as_str()), - ("client_id", codex_client_id().as_str()), + ("client_id", client_id.as_str()), ]) .send() .await? + } }; if !resp.status().is_success() { let status = resp.status(); @@ -781,27 +995,29 @@ pub fn generate_pkce() -> (String, String) { (verifier, challenge) } -// Browser PKCE flow -pub async fn login_browser_flow(no_browser: bool) -> Result<()> { - let client_id = codex_client_id(); - let (code_verifier, code_challenge) = generate_pkce(); - let mut state_buf = [0u8; 16]; +/// Shared PKCE browser/paste login driver (ADR §5.1). The authorize URL, +/// loopback redirect, and token-body encoding all come from the `vendor` +/// descriptor, so every PKCE vendor reuses this one flow. Folds the codex flow +/// into the `accept_callback_code` / `code_from_redirect` helpers (the +/// long-standing TODO) and unifies the `127.0.0.1` bind across vendors. +async fn login_pkce_flow(vendor: &dyn OAuthVendor, no_browser: bool) -> Result<()> { + let (port, _path) = vendor + .redirect() + .ok_or_else(|| anyhow!("{} is not a PKCE vendor", vendor.namespace()))?; + let (verifier, challenge) = generate_pkce(); + // Independent random CSRF state, kept distinct from the PKCE verifier (which + // stays back-channel-only). 32 bytes: claude.ai's authorize rejects a short + // state ("Invalid request format") — long enough for every vendor. + let mut state_buf = [0u8; 32]; getrandom::fill(&mut state_buf).expect("getrandom failed"); let state = URL_SAFE_NO_PAD.encode(state_buf); - let redir_str = redirect_uri(); - let redir = urlencoding::encode(&redir_str); - let auth_url = format!("{CODEX_AUTHORIZE_URL}?client_id={client_id}&redirect_uri={redir}&response_type=code&scope=openid+profile+email+offline_access&code_challenge={code_challenge}&code_challenge_method=S256&state={state}&id_token_add_organizations=true&codex_cli_simplified_flow=true&originator=openab-agent"); - - let listener = TcpListener::bind(format!("127.0.0.1:{REDIRECT_PORT}")).map_err(|e| { - anyhow!("Failed to bind port {REDIRECT_PORT}: {e}. Is another instance running?") - })?; - - if no_browser { - println!("Open this URL in your browser:\n"); - println!(" {auth_url}\n"); - println!("After approving, your browser will redirect to a localhost URL."); - println!("Copy the full URL from the browser address bar and paste it here:\n"); + let auth_url = build_authorize_url(vendor, &challenge, &state)?; + let code = if no_browser { + println!("Open this URL in your browser:\n\n {auth_url}\n"); + println!( + "After approving, copy the full redirect URL (or just the `code#state`) and paste it here:\n" + ); let mut input = String::new(); std::io::stdin() .read_line(&mut input) @@ -810,147 +1026,33 @@ pub async fn login_browser_flow(no_browser: bool) -> Result<()> { if input.is_empty() { return Err(anyhow!("No URL provided")); } - let url = url::Url::parse(input).map_err(|_| anyhow!("Invalid URL: {input}"))?; - - // Skip TCP listener for paste flow - let code = url - .query_pairs() - .find(|(k, _)| k == "code") - .map(|(_, v)| v.to_string()) - .ok_or_else(|| { - let error = url - .query_pairs() - .find(|(k, _)| k == "error") - .map(|(_, v)| v.to_string()); - anyhow!( - "No code in URL. Error: {}", - error.unwrap_or_else(|| "unknown".into()) - ) + // Accept either a full redirect URL or a bare `code#state`. Require the + // `#state` form so CSRF state is always verified — a bare code can't be + // checked and is rejected rather than trusted. + if let Ok(url) = url::Url::parse(input) { + code_from_redirect(&url, &state)? + } else { + let (code, st) = input.split_once('#').ok_or_else(|| { + anyhow!("Paste the full `code#state` value (or the redirect URL) so the state can be verified") })?; - let cb_state = url - .query_pairs() - .find(|(k, _)| k == "state") - .map(|(_, v)| v.to_string()); - if cb_state.as_deref() != Some(&state) { - return Err(anyhow!("State mismatch")); - } - - // Exchange code for tokens - let client = reqwest::Client::new(); - let resp = client - .post(CODEX_TOKEN_URL) - .form(&[ - ("grant_type", "authorization_code"), - ("client_id", client_id.as_str()), - ("code", code.as_str()), - ("code_verifier", code_verifier.as_str()), - ("redirect_uri", redirect_uri().as_str()), - ]) - .send() - .await?; - if !resp.status().is_success() { - let body = resp.text().await.unwrap_or_default(); - return Err(anyhow!("Token exchange failed: {body}")); + if st != state { + return Err(anyhow!("State mismatch")); + } + code.to_string() } - let payload: serde_json::Value = resp.json().await?; - let access_token = payload["access_token"] - .as_str() - .ok_or_else(|| anyhow!("No access_token"))?; - let refresh_token_val = payload["refresh_token"] - .as_str() - .ok_or_else(|| anyhow!("No refresh_token"))?; - let expires_in = payload["expires_in"].as_u64().unwrap_or(3600); - let now = SystemTime::now().duration_since(UNIX_EPOCH)?.as_secs(); - let store = TokenStore { - access_token: access_token.to_string(), - refresh_token: refresh_token_val.to_string(), - expires_at: now + expires_in, - token_endpoint: CODEX_TOKEN_URL.to_string(), - provider: "codex".to_string(), - }; - save_tokens_for(&store)?; - println!( - "\n\u{2705} Login successful! Token saved to {:?}", - auth_path() - ); - return Ok(()); } else { + let listener = TcpListener::bind(format!("127.0.0.1:{port}")).map_err(|e| { + anyhow!("Failed to bind port {port}: {e}. Is another instance running?") + })?; println!("Opening browser for authentication...\n"); if open::that(&auth_url).is_err() { - println!("Could not open browser. Open this URL manually:\n"); - println!(" {auth_url}\n"); + println!("Could not open browser. Open this URL manually:\n\n {auth_url}\n"); } println!("Waiting for callback..."); - } - - listener.set_nonblocking(false)?; - let (mut stream, _) = listener - .accept() - .map_err(|e| anyhow!("Failed to accept callback: {e}"))?; - let mut reader = std::io::BufReader::new(&stream); - let mut request_line = String::new(); - reader.read_line(&mut request_line)?; - - let path = request_line.split_whitespace().nth(1).unwrap_or(""); - let url = url::Url::parse(&format!("http://localhost{path}")) - .map_err(|_| anyhow!("Invalid callback URL"))?; - let code = url - .query_pairs() - .find(|(k, _)| k == "code") - .map(|(_, v)| v.to_string()) - .ok_or_else(|| { - let error = url - .query_pairs() - .find(|(k, _)| k == "error") - .map(|(_, v)| v.to_string()); - anyhow!( - "No code in callback. Error: {}", - error.unwrap_or_else(|| "unknown".into()) - ) - })?; - let cb_state = url - .query_pairs() - .find(|(k, _)| k == "state") - .map(|(_, v)| v.to_string()); - if cb_state.as_deref() != Some(&state) { - return Err(anyhow!("State mismatch in callback")); - } - - let response = "HTTP/1.1 200 OK\r\nContent-Type: text/html\r\n\r\n

Authentication successful!

You can close this tab.

"; - let _ = stream.write_all(response.as_bytes()); - - let client = reqwest::Client::new(); - let resp = client - .post(CODEX_TOKEN_URL) - .form(&[ - ("grant_type", "authorization_code"), - ("client_id", client_id.as_str()), - ("code", code.as_str()), - ("code_verifier", code_verifier.as_str()), - ("redirect_uri", redirect_uri().as_str()), - ]) - .send() - .await?; - if !resp.status().is_success() { - let body = resp.text().await.unwrap_or_default(); - return Err(anyhow!("Token exchange failed: {body}")); - } - let payload: serde_json::Value = resp.json().await?; - let access_token = payload["access_token"] - .as_str() - .ok_or_else(|| anyhow!("No access_token"))?; - let refresh_token_val = payload["refresh_token"] - .as_str() - .ok_or_else(|| anyhow!("No refresh_token"))?; - let expires_in = payload["expires_in"].as_u64().unwrap_or(3600); - let now = SystemTime::now().duration_since(UNIX_EPOCH)?.as_secs(); - let store = TokenStore { - access_token: access_token.to_string(), - refresh_token: refresh_token_val.to_string(), - expires_at: now + expires_in, - token_endpoint: CODEX_TOKEN_URL.to_string(), - provider: "codex".to_string(), + accept_callback_code(&listener, &state)? }; + + let store = exchange_authorization_code(vendor, &code, &state, &verifier).await?; save_tokens_for(&store)?; println!( "\n\u{2705} Login successful! Token saved to {:?}", @@ -959,6 +1061,11 @@ pub async fn login_browser_flow(no_browser: bool) -> Result<()> { Ok(()) } +/// Codex (OpenAI) browser PKCE login. +pub async fn login_browser_flow(no_browser: bool) -> Result<()> { + login_pkce_flow(&CodexVendor, no_browser).await +} + /// Extract the OAuth `code` from a parsed redirect URL, validating `state`. /// Shared by every loopback-callback OAuth flow. fn code_from_redirect(url: &url::Url, expected_state: &str) -> Result { @@ -1006,111 +1113,17 @@ fn accept_callback_code(listener: &TcpListener, expected_state: &str) -> Result< Ok(code) } -/// Anthropic OAuth (Claude Pro/Max). PKCE with an independent random CSRF -/// `state` (verifier stays back-channel-only) and a JSON token exchange against -/// `platform.claude.com`. +/// Anthropic OAuth (Claude Pro/Max) browser PKCE login. JSON token exchange +/// against `platform.claude.com`; all vendor specifics live in `AnthropicVendor`. pub async fn login_anthropic_browser_flow(no_browser: bool) -> Result<()> { - let (verifier, challenge) = generate_pkce(); - // Independent random CSRF state — keep the PKCE verifier back-channel-only. - // 32 bytes: claude.ai's authorize rejects a short state ("Invalid request - // format"); matching the verifier's length keeps it happy while the value - // stays independent (full PKCE strength). - let mut state_buf = [0u8; 32]; - getrandom::fill(&mut state_buf).expect("getrandom failed"); - let state = URL_SAFE_NO_PAD.encode(state_buf); - let auth_url = anthropic_authorize_url(&challenge, &state); - - let code = if no_browser { - println!("Open this URL in your browser:\n\n {auth_url}\n"); - println!( - "After approving, copy the full redirect URL (or just the code) and paste it here:\n" - ); - let mut input = String::new(); - std::io::stdin() - .read_line(&mut input) - .map_err(|e| anyhow!("Failed to read input: {e}"))?; - let input = input.trim(); - if input.is_empty() { - return Err(anyhow!("No URL provided")); - } - // Accept either a full redirect URL or a bare `code#state`. Require the - // `#state` form so CSRF state is always verified — a bare code can't be - // checked and is rejected rather than trusted. - if let Ok(url) = url::Url::parse(input) { - code_from_redirect(&url, &state)? - } else { - let (code, st) = input.split_once('#').ok_or_else(|| { - anyhow!("Paste the full `code#state` value (or the redirect URL) so the state can be verified") - })?; - if st != state { - return Err(anyhow!("State mismatch")); - } - code.to_string() - } - } else { - let listener = TcpListener::bind(format!("127.0.0.1:{ANTHROPIC_REDIRECT_PORT}")).map_err( - |e| { - anyhow!("Failed to bind port {ANTHROPIC_REDIRECT_PORT}: {e}. Is another instance running?") - }, - )?; - println!("Opening browser for authentication...\n"); - if open::that(&auth_url).is_err() { - println!("Could not open browser. Open this URL manually:\n\n {auth_url}\n"); - } - println!("Waiting for callback..."); - accept_callback_code(&listener, &state)? - }; - - let store = exchange_anthropic_code(&code, &state, &verifier).await?; - save_tokens_for(&store)?; - println!( - "\n\u{2705} Login successful! Token saved to {:?}", - auth_path() - ); - Ok(()) -} - -async fn exchange_anthropic_code(code: &str, state: &str, verifier: &str) -> Result { - let client = reqwest::Client::new(); - let resp = client - .post(ANTHROPIC_TOKEN_URL) - .json(&serde_json::json!({ - "grant_type": "authorization_code", - "client_id": anthropic_client_id(), - "code": code, - "state": state, - "redirect_uri": anthropic_redirect_uri(), - "code_verifier": verifier, - })) - .send() - .await?; - if !resp.status().is_success() { - let body = resp.text().await.unwrap_or_default(); - return Err(anyhow!("Token exchange failed: {body}")); - } - let payload: serde_json::Value = resp.json().await?; - let access_token = payload["access_token"] - .as_str() - .ok_or_else(|| anyhow!("No access_token"))?; - let refresh_token_val = payload["refresh_token"] - .as_str() - .ok_or_else(|| anyhow!("No refresh_token"))?; - let expires_in = payload["expires_in"].as_u64().unwrap_or(3600); - let now = SystemTime::now().duration_since(UNIX_EPOCH)?.as_secs(); - Ok(TokenStore { - access_token: access_token.to_string(), - refresh_token: refresh_token_val.to_string(), - expires_at: now + expires_in, - token_endpoint: ANTHROPIC_TOKEN_URL.to_string(), - provider: ANTHROPIC_NAMESPACE.to_string(), - }) + login_pkce_flow(&AnthropicVendor, no_browser).await } // Device code flow pub async fn login_codex_device_flow() -> Result<()> { println!("Starting OpenAI Codex device-code login...\n"); let client = reqwest::Client::new(); - let client_id = codex_client_id(); + let client_id = CodexVendor.client_id(); let resp = client .post(CODEX_DEVICE_AUTH_URL) @@ -1175,21 +1188,8 @@ pub async fn login_codex_device_flow() -> Result<()> { return Err(anyhow!("Token exchange failed: {body}")); } let token_payload: serde_json::Value = token_resp.json().await?; - let access_token = token_payload["access_token"] - .as_str() - .ok_or_else(|| anyhow!("No access_token: {token_payload}"))?; - let refresh_token_val = token_payload["refresh_token"] - .as_str() - .ok_or_else(|| anyhow!("No refresh_token"))?; - let expires_in = token_payload["expires_in"].as_u64().unwrap_or(3600); - let now = SystemTime::now().duration_since(UNIX_EPOCH)?.as_secs(); - let store = TokenStore { - access_token: access_token.to_string(), - refresh_token: refresh_token_val.to_string(), - expires_at: now + expires_in, - token_endpoint: CODEX_TOKEN_URL.to_string(), - provider: "codex".to_string(), - }; + let store = + token_store_from_payload(&token_payload, CODEX_TOKEN_URL, CODEX_NAMESPACE)?; save_tokens_for(&store)?; println!( "\n\u{2705} Login successful! Token saved to {:?}", @@ -1281,6 +1281,74 @@ mod tests { } } + // ── OAuthVendor wire-format locks (ADR §5.1) ────────────────────────── + // The login authorize URL + token-body encoding hit live OAuth servers, so + // no integration test covers them. These pure-function assertions pin the + // exact wire contract so the descriptor refactor can't silently drift it. + + #[test] + fn codex_authorize_url_pins_wire_contract() { + let url = build_authorize_url(&CodexVendor, "CH", "ST").unwrap(); + assert!(url.starts_with(CODEX_AUTHORIZE_URL), "{url}"); + for needle in [ + "response_type=code", + "redirect_uri=http%3A%2F%2Flocalhost%3A1455%2Fauth%2Fcallback", + "scope=openid%20profile%20email%20offline_access", + "code_challenge=CH", + "code_challenge_method=S256", + "state=ST", + // codex simplified-flow hints carried as extra authorize params + "id_token_add_organizations=true", + "codex_cli_simplified_flow=true", + "originator=openab-agent", + ] { + assert!(url.contains(needle), "missing `{needle}` in {url}"); + } + } + + #[test] + fn anthropic_authorize_url_pins_wire_contract() { + let url = build_authorize_url(&AnthropicVendor, "CH", "ST").unwrap(); + assert!(url.starts_with(ANTHROPIC_AUTHORIZE_URL), "{url}"); + for needle in [ + "response_type=code", + "redirect_uri=http%3A%2F%2Flocalhost%3A53692%2Fcallback", + "code_challenge=CH", + "code_challenge_method=S256", + "state=ST", + "code=true", // Anthropic-only extra authorize param + "scope=org%3Acreate_api_key", // scope prefix, colons percent-encoded + ] { + assert!(url.contains(needle), "missing `{needle}` in {url}"); + } + } + + #[test] + fn vendor_for_resolves_oauth_tenants_only() { + assert_eq!(vendor_for(CODEX_NAMESPACE).unwrap().namespace(), CODEX_NAMESPACE); + assert_eq!( + vendor_for(ANTHROPIC_NAMESPACE).unwrap().namespace(), + ANTHROPIC_NAMESPACE + ); + // MCP and unknown tenants have no OAuthVendor (rmcp owns MCP refresh). + assert!(vendor_for("mcp:linear").is_none()); + assert!(vendor_for("nope").is_none()); + } + + #[test] + fn token_body_and_redirect_per_vendor() { + assert_eq!(CodexVendor.token_body(), TokenBodyFormat::Form); + assert_eq!(AnthropicVendor.token_body(), TokenBodyFormat::Json); + assert_eq!( + CodexVendor.redirect_uri().as_deref(), + Some("http://localhost:1455/auth/callback") + ); + assert_eq!( + AnthropicVendor.redirect_uri().as_deref(), + Some("http://localhost:53692/callback") + ); + } + #[test] fn test_is_expired_future_token() { let now = SystemTime::now() @@ -1319,14 +1387,14 @@ mod tests { #[test] fn test_codex_client_id_default() { temp_env::with_var("OPENAB_AGENT_OAUTH_CLIENT_ID", None::<&str>, || { - assert_eq!(codex_client_id(), "app_EMoamEEZ73f0CkXaXp7hrann"); + assert_eq!(CodexVendor.client_id(), "app_EMoamEEZ73f0CkXaXp7hrann"); }); } #[test] fn test_codex_client_id_override() { temp_env::with_var("OPENAB_AGENT_OAUTH_CLIENT_ID", Some("custom_id"), || { - assert_eq!(codex_client_id(), "custom_id"); + assert_eq!(CodexVendor.client_id(), "custom_id"); }); } @@ -1341,7 +1409,7 @@ mod tests { #[test] fn test_anthropic_authorize_url_carries_required_params() { temp_env::with_var("OPENAB_AGENT_ANTHROPIC_CLIENT_ID", None::<&str>, || { - let url = anthropic_authorize_url("CHAL", "STATE"); + let url = build_authorize_url(&AnthropicVendor, "CHAL", "STATE").unwrap(); assert!(url.starts_with("https://claude.ai/oauth/authorize?")); assert!(url.contains("client_id=9d1c250a-e61b-44d9-88ed-5944d1962f5e")); assert!(url.contains("response_type=code")); From d6c29c95bf1ae2f500f7c4d5d524add7fe453bc9 Mon Sep 17 00:00:00 2001 From: Can Date: Sat, 27 Jun 2026 06:28:08 +0000 Subject: [PATCH 13/20] =?UTF-8?q?feat(native-agent):=20credential=20preced?= =?UTF-8?q?ence=20+=20CLAUDE=5FCODE=5FOAUTH=5FTOKEN=20(ADR=20=C2=A75.3)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add `AnthropicAuth::OAuthEnv` for the pre-provisioned long-lived subscription token route (`CLAUDE_CODE_OAUTH_TOKEN`) — the recommended fleet mode (ops mints once, injects as a k8s secret; no interactive flow, no auth.json write, no refresh race). - `AnthropicProvider::auto[_with_model]()` now resolves in ADR §5.3 precedence: `ANTHROPIC_API_KEY` → `CLAUDE_CODE_OAUTH_TOKEN` → stored `anthropic-oauth` tenant. Each source surfaces its own errors rather than falling through to a lower-precedence credential error. - `OAuthEnv` shares the OAuth `Bearer` + Claude Code identity path (extracted into `oauth_headers`); `is_oauth()` covers it so the system block + tool-name casing apply. The 401 force-refresh is gated to the stored tenant only — the env token has no tenant to refresh, so a 401 there surfaces (re-mint) instead of erroring on a missing tenant. 207 tests pass (+2 precedence tests). Co-Authored-By: Claude Opus 4.8 (1M context) --- openab-agent/src/llm.rs | 122 +++++++++++++++++++++++++++++++++------- 1 file changed, 103 insertions(+), 19 deletions(-) diff --git a/openab-agent/src/llm.rs b/openab-agent/src/llm.rs index edc4c3258..36286da72 100644 --- a/openab-agent/src/llm.rs +++ b/openab-agent/src/llm.rs @@ -169,7 +169,8 @@ pub fn default_provider() -> Option { .map(|b| SharedLlmProvider(Arc::from(b))) } -/// How an `AnthropicProvider` authenticates to the Messages API. +/// How an `AnthropicProvider` authenticates to the Messages API +/// (credential-source precedence per ADR §5.3). enum AnthropicAuth { /// `ANTHROPIC_API_KEY` → `x-api-key`, plain system prompt. ApiKey(String), @@ -177,6 +178,11 @@ enum AnthropicAuth { /// headers/system block. The live token is fetched (and refreshed) per call /// from the `anthropic-oauth` tenant in auth.json. OAuth, + /// Pre-provisioned long-lived subscription OAuth token via + /// `CLAUDE_CODE_OAUTH_TOKEN` (ADR §5.3 fleet route). Same `Bearer` + Claude + /// Code identity path as `OAuth`, but the token comes from the env, never + /// touches `auth.json`, and is never refreshed (ops re-mints it). + OAuthEnv(String), } /// Anthropic Claude provider. @@ -265,22 +271,60 @@ impl AnthropicProvider { Ok(Self::build(AnthropicAuth::OAuth, anthropic_model()?)) } - /// Prefer an explicit API key, else a stored Claude subscription OAuth token. - /// When a key is present its own errors (e.g. missing model) surface rather - /// than falling through to an unrelated OAuth-token error. + /// Pre-provisioned long-lived subscription OAuth token from + /// `CLAUDE_CODE_OAUTH_TOKEN` (ADR §5.3). No `auth.json`, no refresh. + fn oauth_env_token() -> Option { + std::env::var("CLAUDE_CODE_OAUTH_TOKEN") + .ok() + .filter(|t| !t.is_empty()) + } + + /// Build from the `CLAUDE_CODE_OAUTH_TOKEN` env route. + pub fn from_oauth_env() -> Result { + let token = Self::oauth_env_token().ok_or_else(|| "CLAUDE_CODE_OAUTH_TOKEN not set".to_string())?; + Ok(Self::build(AnthropicAuth::OAuthEnv(token), anthropic_model()?)) + } + + fn from_oauth_env_with_model(model: &str) -> Result { + let token = Self::oauth_env_token().ok_or_else(|| "CLAUDE_CODE_OAUTH_TOKEN not set".to_string())?; + Ok(Self::build(AnthropicAuth::OAuthEnv(token), model.to_string())) + } + + /// Apply the Claude Pro/Max OAuth `Bearer` + Claude Code identity headers. + /// Shared by the stored-tenant (`OAuth`) and env-token (`OAuthEnv`) paths. + fn oauth_headers(req: reqwest::RequestBuilder, token: &str) -> reqwest::RequestBuilder { + req.header("authorization", format!("Bearer {token}")) + .header("anthropic-beta", "claude-code-20250219,oauth-2025-04-20") + .header("user-agent", "claude-cli/1.0.0") + .header("x-app", "cli") + .header("anthropic-dangerous-direct-browser-access", "true") + } + + /// Credential-source precedence (ADR §5.3): explicit `ANTHROPIC_API_KEY` → + /// pre-provisioned `CLAUDE_CODE_OAUTH_TOKEN` env route → stored interactive + /// `anthropic-oauth` tenant. When a source is present its own errors (e.g. a + /// missing model) surface rather than falling through to an unrelated + /// lower-precedence credential error. pub fn auto() -> Result { - match Self::api_key_from_env() { - Ok(key) => Ok(Self::build(AnthropicAuth::ApiKey(key), anthropic_model()?)), - Err(_) => Self::from_oauth_store(), + if let Ok(key) = Self::api_key_from_env() { + return Ok(Self::build(AnthropicAuth::ApiKey(key), anthropic_model()?)); } + if Self::oauth_env_token().is_some() { + return Self::from_oauth_env(); + } + Self::from_oauth_store() } /// `auto()` with an explicit model override. The override replaces /// `OPENAB_AGENT_MODEL`, so it does not require that env var to be set. pub fn auto_with_model(model: &str) -> Result { - Self::api_key_from_env() - .map(|key| Self::build(AnthropicAuth::ApiKey(key), model.to_string())) - .or_else(|_| Self::from_oauth_store_with_model(model)) + if let Ok(key) = Self::api_key_from_env() { + return Ok(Self::build(AnthropicAuth::ApiKey(key), model.to_string())); + } + if Self::oauth_env_token().is_some() { + return Self::from_oauth_env_with_model(model); + } + Self::from_oauth_store_with_model(model) } /// `from_oauth_store()` with an explicit model override. @@ -372,7 +416,10 @@ impl LlmProvider for AnthropicProvider { } fn is_oauth(&self) -> bool { - matches!(self.auth, AnthropicAuth::OAuth) + matches!( + self.auth, + AnthropicAuth::OAuth | AnthropicAuth::OAuthEnv(_) + ) } fn chat<'a>( @@ -384,6 +431,10 @@ impl LlmProvider for AnthropicProvider { Box::pin(async move { let body = self.build_request_body(system, messages, tools); let oauth = self.is_oauth(); + // Only the stored `anthropic-oauth` tenant can be refreshed on a 401; + // the `CLAUDE_CODE_OAUTH_TOKEN` env route has no tenant to refresh + // (a 401 there means the pre-provisioned token is bad → surface it). + let refreshable = matches!(self.auth, AnthropicAuth::OAuth); let max_retries = 3u32; let mut oauth_refreshed = false; @@ -396,16 +447,13 @@ impl LlmProvider for AnthropicProvider { req = match &self.auth { AnthropicAuth::ApiKey(key) => req.header("x-api-key", key), AnthropicAuth::OAuth => { - // Claude Pro/Max: Bearer + Claude Code identity headers. + // Claude Pro/Max: live token from the stored tenant. let token = crate::auth::get_valid_token_for(crate::auth::ANTHROPIC_NAMESPACE) .await?; - req.header("authorization", format!("Bearer {token}")) - .header("anthropic-beta", "claude-code-20250219,oauth-2025-04-20") - .header("user-agent", "claude-cli/1.0.0") - .header("x-app", "cli") - .header("anthropic-dangerous-direct-browser-access", "true") + Self::oauth_headers(req, &token) } + AnthropicAuth::OAuthEnv(token) => Self::oauth_headers(req, token), }; let resp = req @@ -426,7 +474,7 @@ impl LlmProvider for AnthropicProvider { // 401 on OAuth: token may have expired mid-request; force a // refresh and retry once. Surface a failed refresh instead of // retrying with the stale token. - if oauth && status.as_u16() == 401 && !oauth_refreshed { + if refreshable && status.as_u16() == 401 && !oauth_refreshed { oauth_refreshed = true; crate::auth::force_refresh_for(crate::auth::ANTHROPIC_NAMESPACE).await?; continue; @@ -897,11 +945,47 @@ mod tests { #[test] fn test_is_oauth_reflects_auth_mode() { // Guards the ACP model-switch rebuild: an OAuth session must report - // OAuth so it isn't silently rebuilt against ANTHROPIC_API_KEY. + // OAuth so it isn't silently rebuilt against ANTHROPIC_API_KEY. The env + // route is OAuth too — it uses the same Claude Code identity path. assert!(test_provider(AnthropicAuth::OAuth).is_oauth()); + assert!(test_provider(AnthropicAuth::OAuthEnv("oat".to_string())).is_oauth()); assert!(!test_provider(AnthropicAuth::ApiKey("k".to_string())).is_oauth()); } + #[test] + fn auto_prefers_api_key_over_env_token() { + // ADR §5.3 precedence: ANTHROPIC_API_KEY wins over CLAUDE_CODE_OAUTH_TOKEN. + temp_env::with_vars( + [ + ("ANTHROPIC_API_KEY", Some("sk-ant-test")), + ("CLAUDE_CODE_OAUTH_TOKEN", Some("oat-test")), + ("OPENAB_AGENT_MODEL", Some("anthropic/claude-sonnet-4-6")), + ], + || { + let p = AnthropicProvider::auto().unwrap(); + assert!(matches!(p.auth, AnthropicAuth::ApiKey(_))); + }, + ); + } + + #[test] + fn auto_uses_env_token_when_no_api_key() { + // No API key → the CLAUDE_CODE_OAUTH_TOKEN env route, not the stored tenant + // (this builds without reading auth.json). + temp_env::with_vars( + [ + ("ANTHROPIC_API_KEY", None), + ("CLAUDE_CODE_OAUTH_TOKEN", Some("oat-test")), + ("OPENAB_AGENT_MODEL", Some("anthropic/claude-sonnet-4-6")), + ], + || { + let p = AnthropicProvider::auto().unwrap(); + assert!(matches!(p.auth, AnthropicAuth::OAuthEnv(_))); + assert!(p.is_oauth()); + }, + ); + } + #[test] fn test_build_request_body() { let provider = test_provider(AnthropicAuth::ApiKey("test".to_string())); From 3f3d0a8fb36049f139e145f6eec2e1ba363f2cb0 Mon Sep 17 00:00:00 2001 From: Can Date: Sat, 27 Jun 2026 06:29:54 +0000 Subject: [PATCH 14/20] fix(native-agent): ModelRef::parse only splits known provider prefixes (review F4) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `provider/model` previously split on the first `/`, mis-parsing HuggingFace-style `org/model` ids (e.g. `meta-llama/Llama-3-8B`) for custom/OpenAI-compatible endpoints — `org` became the "provider" and the real id was truncated. Now the prefix is split off only when it's a `KNOWN_PROVIDERS` entry; otherwise the whole string is the model id. Load-bearing for the planned single-string `provider/model` config field. Known prefixes still split unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) --- openab-agent/src/llm.rs | 27 ++++++++++++++++++++++++--- 1 file changed, 24 insertions(+), 3 deletions(-) diff --git a/openab-agent/src/llm.rs b/openab-agent/src/llm.rs index 36286da72..667b016ee 100644 --- a/openab-agent/src/llm.rs +++ b/openab-agent/src/llm.rs @@ -97,10 +97,18 @@ impl std::ops::Deref for SharedLlmProvider { } } +/// Provider prefixes [`ModelRef::parse`] recognizes. A `prefix/rest` model +/// splits into `(provider, model)` ONLY when `prefix` is one of these. Otherwise +/// the whole string is the model id — so a HuggingFace-style `org/model` id +/// (e.g. `meta-llama/Llama-3-8B`) for a custom/OpenAI-compatible endpoint stays +/// intact instead of mis-parsing `org` as a provider. Extend as vendors land. +const KNOWN_PROVIDERS: &[&str] = &["anthropic", "anthropic-oauth", "claude", "openai", "codex"]; + /// A model reference, optionally provider-qualified. Accepts the canonical /// `provider/model_id` form (e.g. `anthropic/claude-sonnet-4-6`) as well as a -/// bare `model_id` (provider then inferred from credentials). Model IDs never -/// contain `/`, so the first `/` cleanly separates the two. +/// bare `model_id` (provider then inferred from credentials). Only a *known* +/// provider prefix is split off (see [`KNOWN_PROVIDERS`]), so model ids that +/// themselves contain `/` (HuggingFace `org/model`) are preserved. #[derive(Debug, Clone, PartialEq, Eq)] pub struct ModelRef { pub provider: Option, @@ -110,7 +118,7 @@ pub struct ModelRef { impl ModelRef { pub fn parse(input: &str) -> Self { match input.split_once('/') { - Some((p, m)) if !p.is_empty() && !m.is_empty() => ModelRef { + Some((p, m)) if KNOWN_PROVIDERS.contains(&p) && !m.is_empty() => ModelRef { provider: Some(p.to_string()), model: m.to_string(), }, @@ -886,6 +894,19 @@ mod tests { // Degenerate slashes fall back to bare (no empty provider/model). assert_eq!(ModelRef::parse("/gpt-5.4").provider, None); assert_eq!(ModelRef::parse("openai/").model, "openai/"); + + // F4: a HuggingFace-style `org/model` id is NOT a known provider, so the + // whole string stays the model id (the `/` is part of the id). + let r = ModelRef::parse("meta-llama/Llama-3-8B-Instruct"); + assert_eq!(r.provider, None); + assert_eq!(r.model, "meta-llama/Llama-3-8B-Instruct"); + + // Every known provider prefix still splits. + for prov in KNOWN_PROVIDERS { + let r = ModelRef::parse(&format!("{prov}/some-model")); + assert_eq!(r.provider.as_deref(), Some(*prov)); + assert_eq!(r.model, "some-model"); + } } #[test] From a20389ab3a9af63e09832825cbdaab6976ff7eb3 Mon Sep 17 00:00:00 2001 From: Can Date: Sat, 27 Jun 2026 06:31:20 +0000 Subject: [PATCH 15/20] fix(native-agent): fail loud on present-but-misconfigured Anthropic creds (review F3) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `select_provider`'s auto-detect arm discarded the `AnthropicProvider::auto()` error and fell through to Codex. A present Anthropic credential that failed for a config reason (e.g. a key set but no model) was silently masked — the user got a Codex provider, or a Codex-only error that hid the real cause. Now: if any Anthropic credential source exists (API key / CLAUDE_CODE_OAUTH_TOKEN / stored tenant), an `auto()` failure surfaces as a config error ("credential present but unusable"). Codex fallthrough happens only when no Anthropic credential exists at all, and that final error names both credential routes. Co-Authored-By: Claude Opus 4.8 (1M context) --- openab-agent/src/llm.rs | 57 ++++++++++++++++++++++++++++++++++++----- 1 file changed, 51 insertions(+), 6 deletions(-) diff --git a/openab-agent/src/llm.rs b/openab-agent/src/llm.rs index 667b016ee..de38a2039 100644 --- a/openab-agent/src/llm.rs +++ b/openab-agent/src/llm.rs @@ -156,12 +156,24 @@ pub fn select_provider(choice: &str) -> Result, String> { "openai" | "codex" => Ok(Box::new(OpenAiProvider::from_auth_store()?)), _ => match AnthropicProvider::auto() { Ok(p) => Ok(Box::new(p)), - Err(_) => match OpenAiProvider::from_auth_store() { - Ok(p) => Ok(Box::new(p)), - Err(e) => Err(format!( - "No credentials: set ANTHROPIC_API_KEY, or run `openab-agent auth anthropic-oauth` / `openab-agent auth codex-oauth`. {e}" - )), - }, + // F3 — don't let a *present-but-misconfigured* Anthropic credential + // silently fall through to Codex. If a credential exists, the failure + // is a real config error (e.g. a credential set but no model): fail + // loud with it. Only fall through to Codex when no Anthropic + // credential exists at all. + Err(anthropic_err) => { + if AnthropicProvider::credential_present() { + Err(format!( + "Anthropic credential present but unusable: {anthropic_err}" + )) + } else { + OpenAiProvider::from_auth_store() + .map(|p| Box::new(p) as Box) + .map_err(|codex_err| format!( + "No credentials: set ANTHROPIC_API_KEY / CLAUDE_CODE_OAUTH_TOKEN, or run `openab-agent auth anthropic-oauth` / `openab-agent auth codex-oauth`. ({codex_err})" + )) + } + } }, } } @@ -298,6 +310,16 @@ impl AnthropicProvider { Ok(Self::build(AnthropicAuth::OAuthEnv(token), model.to_string())) } + /// True when *some* Anthropic credential source exists (API key, env OAuth + /// token, or stored tenant). Lets `select_provider` tell a real config error + /// ("credential present but `auto()` failed" → fail loud) from "no Anthropic + /// credentials" (legitimately fall through to Codex) — review F3. + pub fn credential_present() -> bool { + Self::api_key_from_env().is_ok() + || Self::oauth_env_token().is_some() + || crate::auth::load_tokens_for(crate::auth::ANTHROPIC_NAMESPACE).is_ok() + } + /// Apply the Claude Pro/Max OAuth `Bearer` + Claude Code identity headers. /// Shared by the stored-tenant (`OAuth`) and env-token (`OAuthEnv`) paths. fn oauth_headers(req: reqwest::RequestBuilder, token: &str) -> reqwest::RequestBuilder { @@ -1007,6 +1029,29 @@ mod tests { ); } + #[test] + fn select_provider_fails_loud_on_misconfigured_anthropic() { + // F3: an Anthropic credential is present (API key) but no model is set, so + // auto() fails for a config reason. select_provider must surface that + // error, not silently fall through to Codex. + temp_env::with_vars( + [ + ("ANTHROPIC_API_KEY", Some("sk-ant-test")), + ("CLAUDE_CODE_OAUTH_TOKEN", None), + ("OPENAB_AGENT_MODEL", None), + ("OPENAB_AGENT_PROVIDER", None), + ], + || { + // `Box` isn't Debug, so match rather than unwrap_err. + let err = match select_provider("") { + Ok(_) => panic!("expected a fail-loud error, got a provider"), + Err(e) => e, + }; + assert!(err.contains("present but unusable"), "got: {err}"); + }, + ); + } + #[test] fn test_build_request_body() { let provider = test_provider(AnthropicAuth::ApiKey("test".to_string())); From 1d1c862843eb3c96b331d952ff9b44a7365bc3d7 Mon Sep 17 00:00:00 2001 From: Can Date: Sat, 27 Jun 2026 06:45:07 +0000 Subject: [PATCH 16/20] =?UTF-8?q?feat(native-agent):=20centralized=20confi?= =?UTF-8?q?g.json=20for=20default=20model/params=20(ADR=20=C2=A75.5)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add a small JSON config file next to auth.json so a deployment can declare the default provider/model (and max_tokens) in a file instead of only via env vars — the centralized-config gap the multi-vendor work surfaced. - New `config` module: `{ model, max_tokens }` (single `provider/model` string, reusing ModelRef). Unknown keys tolerated (forward-compat for `providers`/etc.); malformed JSON fails loud; a missing file is an empty config. - Resolution is env-over-config: `anthropic_model` / `anthropic_max_tokens` / `resolve_provider_choice` now fall `OPENAB_AGENT_*` env → config.json → built-in. A pod's injected env stays authoritative over a baked config. - Secrets never live here — they stay in the locked `auth.json` store. Per-provider `base_url` routing is a deliberate follow-up; this lands the default provider/model/params surface. 213 tests pass (+5). Co-Authored-By: Claude Opus 4.8 (1M context) --- openab-agent/src/config.rs | 105 +++++++++++++++++++++++++++++++++++++ openab-agent/src/llm.rs | 102 +++++++++++++++++++++++++++++------ openab-agent/src/main.rs | 1 + 3 files changed, 191 insertions(+), 17 deletions(-) create mode 100644 openab-agent/src/config.rs diff --git a/openab-agent/src/config.rs b/openab-agent/src/config.rs new file mode 100644 index 000000000..75031f751 --- /dev/null +++ b/openab-agent/src/config.rs @@ -0,0 +1,105 @@ +//! Centralized agent config (ADR §5.5 — declarative defaults; secrets stay in +//! `auth.json`). A small JSON file next to `auth.json` that supplies the default +//! provider/model and a few params, so a deployment can declare them in a file +//! instead of only via env vars. +//! +//! Precedence is **env-over-config**: `OPENAB_AGENT_MODEL` / `OPENAB_AGENT_MAX_TOKENS` +//! still win, and this file is the declarative default for runs that don't set +//! them (a pod's injected env stays authoritative over a baked config). The +//! resolution chain lives in `llm.rs` (`anthropic_model` / `anthropic_max_tokens` +//! / `resolve_provider_choice`). +//! +//! Unknown keys are tolerated (forward-compat: `providers`/`small_model`/… can +//! land later without breaking older binaries). Secrets never live here. + +use serde::Deserialize; +use std::path::PathBuf; + +/// Parsed `config.json`. Every field is optional — a missing file is an empty +/// config, and any field falls back to env/built-in default downstream. +#[derive(Debug, Clone, Default, Deserialize)] +#[serde(default)] +pub struct AgentConfig { + /// Default model as a single `provider/model` string (see `ModelRef`). + pub model: Option, + /// Default max output tokens. + pub max_tokens: Option, +} + +impl AgentConfig { + /// `config.json` next to `auth.json`. `OPENAB_CONFIG_PATH` overrides the + /// whole path (ops injection / tests). + pub fn path() -> PathBuf { + if let Ok(p) = std::env::var("OPENAB_CONFIG_PATH") { + return PathBuf::from(p); + } + crate::auth::auth_path().with_file_name("config.json") + } + + /// Parse config JSON. Pure (unit-testable). + pub fn parse(data: &str) -> anyhow::Result { + serde_json::from_str(data).map_err(Into::into) + } + + /// Load + parse the config file. A missing file is an empty config (not an + /// error); a present-but-malformed file IS an error so a typo is visible + /// rather than silently dropped. + pub fn load() -> anyhow::Result { + let path = Self::path(); + match std::fs::read_to_string(&path) { + Ok(s) => { + Self::parse(&s).map_err(|e| anyhow::anyhow!("invalid {}: {e}", path.display())) + } + Err(e) if e.kind() == std::io::ErrorKind::NotFound => Ok(Self::default()), + Err(e) => Err(anyhow::anyhow!("reading {}: {e}", path.display())), + } + } + + /// Load, or an empty config (logging a warning) when the file is malformed — + /// a typo'd config must not crash the agent, but it has to be visible. Used + /// by the resolution path, which then falls through to env/built-in defaults. + pub fn load_or_default() -> Self { + match Self::load() { + Ok(c) => c, + Err(e) => { + tracing::warn!("ignoring config: {e}"); + Self::default() + } + } + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn parse_reads_known_fields() { + let c = AgentConfig::parse(r#"{"model":"anthropic/claude-sonnet-4-6","max_tokens":4096}"#) + .unwrap(); + assert_eq!(c.model.as_deref(), Some("anthropic/claude-sonnet-4-6")); + assert_eq!(c.max_tokens, Some(4096)); + } + + #[test] + fn empty_object_is_all_none() { + let c = AgentConfig::parse("{}").unwrap(); + assert_eq!(c.model, None); + assert_eq!(c.max_tokens, None); + } + + #[test] + fn unknown_keys_tolerated_for_forward_compat() { + // `providers` / future keys must not break an older binary. + let c = AgentConfig::parse(r#"{"model":"anthropic/x","providers":{"anthropic":{}}}"#) + .unwrap(); + assert_eq!(c.model.as_deref(), Some("anthropic/x")); + } + + #[test] + fn malformed_json_is_an_error() { + assert!(AgentConfig::parse("{not json").is_err()); + // wrong type for a known field is also a hard error (fail loud) + assert!(AgentConfig::parse(r#"{"max_tokens":"lots"}"#).is_err()); + } +} diff --git a/openab-agent/src/llm.rs b/openab-agent/src/llm.rs index de38a2039..c08afd59b 100644 --- a/openab-agent/src/llm.rs +++ b/openab-agent/src/llm.rs @@ -130,17 +130,26 @@ impl ModelRef { } } -/// The provider the user asked for: explicit `OPENAB_AGENT_PROVIDER`, else the -/// `provider/` prefix of `OPENAB_AGENT_MODEL` (e.g. `openai/gpt-5.4` selects -/// OpenAI even when an Anthropic key is also present), else empty (auto-detect). +/// The provider the user asked for. Precedence: explicit `OPENAB_AGENT_PROVIDER` +/// → `provider/` prefix of `OPENAB_AGENT_MODEL` (e.g. `openai/gpt-5.4` selects +/// OpenAI even when an Anthropic key is also present) → `provider/` prefix of +/// `config.json`'s `model` → empty (auto-detect). Env-over-config (ADR §5.5). pub fn resolve_provider_choice() -> String { - match std::env::var("OPENAB_AGENT_PROVIDER") { - Ok(p) if !p.is_empty() => p, - _ => std::env::var("OPENAB_AGENT_MODEL") - .ok() - .and_then(|m| ModelRef::parse(&m).provider) - .unwrap_or_default(), + if let Ok(p) = std::env::var("OPENAB_AGENT_PROVIDER") { + if !p.is_empty() { + return p; + } } + if let Some(p) = std::env::var("OPENAB_AGENT_MODEL") + .ok() + .and_then(|m| ModelRef::parse(&m).provider) + { + return p; + } + crate::config::AgentConfig::load_or_default() + .model + .and_then(|m| ModelRef::parse(&m).provider) + .unwrap_or_default() } /// Select an `LlmProvider` from an explicit `choice` (`anthropic` / @@ -213,20 +222,37 @@ pub struct AnthropicProvider { client: reqwest::Client, } -/// Resolve the Anthropic model from `OPENAB_AGENT_MODEL`. No hardcoded default: -/// dateless 4.6+ IDs are fixed canonical IDs (not evergreen pointers), so a -/// pinned default is a per-generation 404 timebomb. Require an explicit choice -/// and fail loud instead. +/// Resolve the Anthropic model. Precedence (ADR §5.3/§5.5): `OPENAB_AGENT_MODEL` +/// env → `model` in `config.json` → error. No hardcoded default: dateless 4.6+ +/// IDs are fixed canonical IDs (not evergreen pointers), so a pinned default is a +/// per-generation 404 timebomb. Require an explicit choice and fail loud instead. fn anthropic_model() -> Result { - std::env::var("OPENAB_AGENT_MODEL") - .map_err(|_| "no model configured; set OPENAB_AGENT_MODEL or select a model".to_string()) + if let Ok(m) = std::env::var("OPENAB_AGENT_MODEL") { + if !m.is_empty() { + return Ok(m); + } + } + if let Some(m) = crate::config::AgentConfig::load_or_default().model { + if !m.is_empty() { + return Ok(m); + } + } + Err("no model configured; set OPENAB_AGENT_MODEL, add `model` to config.json, or select a model".to_string()) } +/// Max output tokens: `OPENAB_AGENT_MAX_TOKENS` env → `max_tokens` in +/// `config.json` → built-in 8192 (env-over-config, ADR §5.5). fn anthropic_max_tokens() -> u32 { - std::env::var("OPENAB_AGENT_MAX_TOKENS") + if let Some(v) = std::env::var("OPENAB_AGENT_MAX_TOKENS") .ok() .and_then(|v| v.parse().ok()) - .unwrap_or(8192) + { + return v; + } + if let Some(v) = crate::config::AgentConfig::load_or_default().max_tokens { + return v; + } + 8192 } /// openab-agent's built-in tools mapped to Claude Code's canonical casing. The @@ -1029,6 +1055,48 @@ mod tests { ); } + #[test] + fn model_resolves_env_over_config_over_error() { + let dir = tempfile::tempdir().unwrap(); + let cfg = dir.path().join("config.json"); + std::fs::write(&cfg, r#"{"model":"anthropic/from-config"}"#).unwrap(); + let cfg_path = cfg.to_str().unwrap(); + + // env wins over config.json + temp_env::with_vars( + [ + ("OPENAB_CONFIG_PATH", Some(cfg_path)), + ("OPENAB_AGENT_MODEL", Some("anthropic/from-env")), + ], + || { + assert_eq!(anthropic_model().unwrap(), "anthropic/from-env"); + assert_eq!(resolve_provider_choice(), "anthropic"); + }, + ); + + // no env → config.json supplies the model (and its provider prefix) + temp_env::with_vars( + [ + ("OPENAB_CONFIG_PATH", Some(cfg_path)), + ("OPENAB_AGENT_MODEL", None), + ("OPENAB_AGENT_PROVIDER", None), + ], + || { + assert_eq!(anthropic_model().unwrap(), "anthropic/from-config"); + assert_eq!(resolve_provider_choice(), "anthropic"); + }, + ); + + // neither env nor config → fail loud + temp_env::with_vars( + [ + ("OPENAB_CONFIG_PATH", Some(dir.path().join("missing.json").to_str().unwrap())), + ("OPENAB_AGENT_MODEL", None), + ], + || assert!(anthropic_model().is_err()), + ); + } + #[test] fn select_provider_fails_loud_on_misconfigured_anthropic() { // F3: an Anthropic credential is present (API key) but no model is set, so diff --git a/openab-agent/src/main.rs b/openab-agent/src/main.rs index 068a74fd4..856045ae1 100644 --- a/openab-agent/src/main.rs +++ b/openab-agent/src/main.rs @@ -1,6 +1,7 @@ mod acp; mod agent; mod auth; +mod config; mod llm; mod mcp; mod skills; From 8dcabb050c32fb5ad204b0cafdc1e622c0507263 Mon Sep 17 00:00:00 2001 From: Can Date: Sat, 27 Jun 2026 06:46:15 +0000 Subject: [PATCH 17/20] docs(native-agent): document OAuthVendor, credential precedence, config.json (review F5) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Anthropic credentials section: the ADR §5.3 precedence (ANTHROPIC_API_KEY → CLAUDE_CODE_OAUTH_TOKEN fleet route → interactive anthropic-oauth) + the `openab-agent auth anthropic-oauth` login. - New "Configuration file (config.json)" section: schema, env-over-config precedence, secrets-stay-in-auth.json. - "Adding an OAuth vendor": the OAuthVendor descriptor model (ADR §5.1). - Env table: CLAUDE_CODE_OAUTH_TOKEN, OPENAB_AGENT_ANTHROPIC_CLIENT_ID, OPENAB_CONFIG_PATH. Co-Authored-By: Claude Opus 4.8 (1M context) --- docs/native-agent.md | 61 +++++++++++++++++++++++++++++++++++++------- 1 file changed, 52 insertions(+), 9 deletions(-) diff --git a/docs/native-agent.md b/docs/native-agent.md index 11e98bceb..f396d5e39 100644 --- a/docs/native-agent.md +++ b/docs/native-agent.md @@ -27,18 +27,41 @@ openab-agent env = { OPENAB_AGENT_OPENAI_MODEL = "gpt-5.4-mini" } ``` +### Configuration file (config.json) + +A small JSON file next to `auth.json` (default `/config.json`, +overridable with `OPENAB_CONFIG_PATH`) declares the default model and params, so +a deployment can set them in a file instead of only via env vars. **Secrets never +go here** — credentials stay in the locked `auth.json` store. + +```jsonc +{ + "model": "anthropic/claude-sonnet-4-6", // single provider/model string + "max_tokens": 8192 // optional +} +``` + +Resolution is **env-over-config**: `OPENAB_AGENT_MODEL` / `OPENAB_AGENT_MAX_TOKENS` +override the file, so a pod's injected env stays authoritative over a baked +config. A missing file is fine (empty config); a malformed file is logged and +ignored (the agent then falls back to env / built-in defaults). Unknown keys are +tolerated for forward-compatibility. + ## Environment Variables | Variable | Default | Description | |----------|---------|-------------| -| `OPENAB_AGENT_MODEL` | — (required for Anthropic) | Anthropic model id (e.g. `claude-opus-4-8`). No hardcoded default — dateless 4.6+ IDs are fixed canonical IDs that retire each generation, so the agent fails loud if unset rather than pin a model that will eventually 404. | +| `OPENAB_AGENT_MODEL` | — (required for Anthropic) | Anthropic model id, optionally `provider/`-qualified (e.g. `claude-opus-4-8`, `anthropic/claude-opus-4-8`). No hardcoded default — dateless 4.6+ IDs are fixed canonical IDs that retire each generation, so the agent fails loud if unset rather than pin a model that will eventually 404. Overrides `model` in [config.json](#configuration-file-configjson). | | `OPENAB_AGENT_OPENAI_MODEL` | `gpt-5.4-mini` | Model to use (must be supported by your ChatGPT plan — see [Supported Models](#supported-models-chatgpt-subscription)) | | `OPENAB_AGENT_OPENAI_BASE_URL` | `https://chatgpt.com/backend-api` | API base URL | | `OPENAB_AGENT_PROVIDER` | auto-detect | Force provider (`anthropic`, `openai`, `codex`) | -| `OPENAB_AGENT_MAX_TOKENS` | `8192` | Max output tokens | -| `OPENAB_AGENT_OAUTH_CLIENT_ID` | Pi's client | Custom OAuth client ID | +| `OPENAB_AGENT_MAX_TOKENS` | `8192` | Max output tokens. Overrides `max_tokens` in config.json. | +| `OPENAB_AGENT_OAUTH_CLIENT_ID` | Pi's client | Custom Codex OAuth client ID | +| `OPENAB_AGENT_ANTHROPIC_CLIENT_ID` | Claude Code's client | Custom Anthropic OAuth client ID | | `OPENAB_AGENT_MAX_TOOL_LOOPS` | `50` | Max tool-call iterations per prompt before the agent gives up | -| `ANTHROPIC_API_KEY` | — | Anthropic API key (alternative to OAuth) | +| `ANTHROPIC_API_KEY` | — | Anthropic API key. Highest-precedence Anthropic credential (see [Anthropic credentials](#anthropic-credentials)). | +| `CLAUDE_CODE_OAUTH_TOKEN` | — | Pre-provisioned long-lived Claude Pro/Max subscription token (from `claude setup-token`). Fleet route — no interactive login, no `auth.json` write. | +| `OPENAB_CONFIG_PATH` | `/config.json` | Override the config-file path. | ## Authentication @@ -70,13 +93,33 @@ openab-agent auth codex-device Note: Device flow currently has limited scopes and may not work with all models. -### API Key (Anthropic) +### Anthropic credentials -```bash -export ANTHROPIC_API_KEY=sk-ant-... -``` +Three ways to authenticate Anthropic, resolved in this **precedence** (ADR §5.3): + +1. **API key** — `export ANTHROPIC_API_KEY=sk-ant-...`. No login; auto-detected. +2. **Pre-provisioned subscription token (fleet route)** — `export CLAUDE_CODE_OAUTH_TOKEN=...` + (mint once with `claude setup-token`; ~1-year Claude Pro/Max token). Sent as a + `Bearer` subscription token with the Claude Code identity headers — no + interactive login, no `auth.json` write, no refresh. Recommended for pods (inject + as a k8s secret). +3. **Interactive Claude Pro/Max OAuth** — browser PKCE login, refreshed from the + stored `anthropic-oauth` tenant in `auth.json`: + + ```bash + openab-agent auth anthropic-oauth # browser + openab-agent auth anthropic-oauth --no-browser # paste code#state + ``` + +A higher-precedence source's own errors (e.g. a key set but no model) surface +rather than silently falling through to a lower one. + +### Adding an OAuth vendor -No login needed — set the env var and the agent auto-detects it. +Subscription-OAuth providers are declared as a single `OAuthVendor` descriptor +(`auth.rs`, ADR §5.1) — namespace, client id, authorize/token URLs, redirect, +scope, token-body encoding. The shared PKCE/device/refresh driver reads the +descriptor, so a new vendor is a new descriptor, not a new hand-rolled flow. ## Custom System Prompt From 63cef8544292987c6c180ad79c9d4540bda47b92 Mon Sep 17 00:00:00 2001 From: Can Date: Sat, 27 Jun 2026 06:47:05 +0000 Subject: [PATCH 18/20] style: cargo fmt (OAuthVendor + config.json) Co-Authored-By: Claude Opus 4.8 (1M context) --- openab-agent/src/auth.rs | 8 +++++--- openab-agent/src/config.rs | 4 ++-- openab-agent/src/llm.rs | 26 +++++++++++++++++--------- 3 files changed, 24 insertions(+), 14 deletions(-) diff --git a/openab-agent/src/auth.rs b/openab-agent/src/auth.rs index 864bda05f..04c0c0412 100644 --- a/openab-agent/src/auth.rs +++ b/openab-agent/src/auth.rs @@ -1188,8 +1188,7 @@ pub async fn login_codex_device_flow() -> Result<()> { return Err(anyhow!("Token exchange failed: {body}")); } let token_payload: serde_json::Value = token_resp.json().await?; - let store = - token_store_from_payload(&token_payload, CODEX_TOKEN_URL, CODEX_NAMESPACE)?; + let store = token_store_from_payload(&token_payload, CODEX_TOKEN_URL, CODEX_NAMESPACE)?; save_tokens_for(&store)?; println!( "\n\u{2705} Login successful! Token saved to {:?}", @@ -1325,7 +1324,10 @@ mod tests { #[test] fn vendor_for_resolves_oauth_tenants_only() { - assert_eq!(vendor_for(CODEX_NAMESPACE).unwrap().namespace(), CODEX_NAMESPACE); + assert_eq!( + vendor_for(CODEX_NAMESPACE).unwrap().namespace(), + CODEX_NAMESPACE + ); assert_eq!( vendor_for(ANTHROPIC_NAMESPACE).unwrap().namespace(), ANTHROPIC_NAMESPACE diff --git a/openab-agent/src/config.rs b/openab-agent/src/config.rs index 75031f751..a5f6f4d36 100644 --- a/openab-agent/src/config.rs +++ b/openab-agent/src/config.rs @@ -91,8 +91,8 @@ mod tests { #[test] fn unknown_keys_tolerated_for_forward_compat() { // `providers` / future keys must not break an older binary. - let c = AgentConfig::parse(r#"{"model":"anthropic/x","providers":{"anthropic":{}}}"#) - .unwrap(); + let c = + AgentConfig::parse(r#"{"model":"anthropic/x","providers":{"anthropic":{}}}"#).unwrap(); assert_eq!(c.model.as_deref(), Some("anthropic/x")); } diff --git a/openab-agent/src/llm.rs b/openab-agent/src/llm.rs index c08afd59b..fab48c50e 100644 --- a/openab-agent/src/llm.rs +++ b/openab-agent/src/llm.rs @@ -327,13 +327,21 @@ impl AnthropicProvider { /// Build from the `CLAUDE_CODE_OAUTH_TOKEN` env route. pub fn from_oauth_env() -> Result { - let token = Self::oauth_env_token().ok_or_else(|| "CLAUDE_CODE_OAUTH_TOKEN not set".to_string())?; - Ok(Self::build(AnthropicAuth::OAuthEnv(token), anthropic_model()?)) + let token = + Self::oauth_env_token().ok_or_else(|| "CLAUDE_CODE_OAUTH_TOKEN not set".to_string())?; + Ok(Self::build( + AnthropicAuth::OAuthEnv(token), + anthropic_model()?, + )) } fn from_oauth_env_with_model(model: &str) -> Result { - let token = Self::oauth_env_token().ok_or_else(|| "CLAUDE_CODE_OAUTH_TOKEN not set".to_string())?; - Ok(Self::build(AnthropicAuth::OAuthEnv(token), model.to_string())) + let token = + Self::oauth_env_token().ok_or_else(|| "CLAUDE_CODE_OAUTH_TOKEN not set".to_string())?; + Ok(Self::build( + AnthropicAuth::OAuthEnv(token), + model.to_string(), + )) } /// True when *some* Anthropic credential source exists (API key, env OAuth @@ -472,10 +480,7 @@ impl LlmProvider for AnthropicProvider { } fn is_oauth(&self) -> bool { - matches!( - self.auth, - AnthropicAuth::OAuth | AnthropicAuth::OAuthEnv(_) - ) + matches!(self.auth, AnthropicAuth::OAuth | AnthropicAuth::OAuthEnv(_)) } fn chat<'a>( @@ -1090,7 +1095,10 @@ mod tests { // neither env nor config → fail loud temp_env::with_vars( [ - ("OPENAB_CONFIG_PATH", Some(dir.path().join("missing.json").to_str().unwrap())), + ( + "OPENAB_CONFIG_PATH", + Some(dir.path().join("missing.json").to_str().unwrap()), + ), ("OPENAB_AGENT_MODEL", None), ], || assert!(anthropic_model().is_err()), From f32de330ad5a445c19f6f915317c2012eb654178 Mon Sep 17 00:00:00 2001 From: Can Date: Sat, 27 Jun 2026 08:44:41 +0000 Subject: [PATCH 19/20] =?UTF-8?q?test(native-agent):=20prove=20=C2=A75.4?= =?UTF-8?q?=20lock=20single-flight=20for=20the=20anthropic-oauth=20tenant?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Brett asked to cover the new tenant the same way #1190's codex lock tests do, so single-flight / fail-closed is proven for it rather than only structurally similar: - `with_auth_locked_merges_anthropic_tenant_no_lost_update` — a concurrent codex write does not clobber a just-written `anthropic-oauth` token (the new tenant rides the same locked RMW funnel). - `lock_tenant_refresh_fails_closed_for_anthropic_and_is_per_tenant` — a held anthropic refresh lock makes a second anthropic acquire fail closed (`TimedOut`), and does NOT block codex (per-tenant isolation — the reason §5.4 uses a per-tenant lock, not the global one). 215 tests pass. Co-Authored-By: Claude Opus 4.8 (1M context) --- openab-agent/src/auth.rs | 70 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 70 insertions(+) diff --git a/openab-agent/src/auth.rs b/openab-agent/src/auth.rs index 04c0c0412..965d5e4ac 100644 --- a/openab-agent/src/auth.rs +++ b/openab-agent/src/auth.rs @@ -1849,4 +1849,74 @@ mod tests { "acquire succeeds once the holder releases" ); } + + #[test] + fn with_auth_locked_merges_anthropic_tenant_no_lost_update() { + // The §5.4 lost-update guarantee must hold for the `anthropic-oauth` + // tenant too: a concurrent codex write must not clobber a just-written + // Anthropic token (proves the new tenant rides the same locked funnel). + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("auth.json"); + + let mut anth = make_store(7); + anth.provider = ANTHROPIC_NAMESPACE.to_string(); + with_auth_locked(&path, |m| { + m.insert(ANTHROPIC_NAMESPACE.to_string(), AuthEntry::Token(anth)); + }) + .unwrap(); + with_auth_locked(&path, |m| { + m.insert("codex".to_string(), AuthEntry::Token(make_store(1))); + }) + .unwrap(); + + let map = read_auth_file(&path).unwrap(); + assert_eq!(map.len(), 2, "second write merged, did not lost-update"); + assert_eq!(token_of(map.get(ANTHROPIC_NAMESPACE)).expires_at, 7); + assert_eq!(token_of(map.get("codex")).expires_at, 1); + } + + #[cfg(unix)] + #[tokio::test] + async fn lock_tenant_refresh_fails_closed_for_anthropic_and_is_per_tenant() { + // §5.4 (b) proven for the `anthropic-oauth` tenant: while one holder keeps + // its refresh lock, a second acquire fails closed (`TimedOut`) — single- + // flight for the new tenant, not just codex. + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("auth.json"); + + let held = lock_tenant_refresh(&path, ANTHROPIC_NAMESPACE).await; + assert!( + matches!(held, RefreshLock::Held(_)), + "anthropic acquire holds" + ); + + let contended = lock_tenant_refresh_until( + &path, + ANTHROPIC_NAMESPACE, + std::time::Duration::from_millis(200), + ) + .await; + assert!( + matches!(contended, RefreshLock::TimedOut), + "second anthropic acquire fails closed while held" + ); + + // Per-tenant isolation: the locks are keyed per tenant, so holding the + // Anthropic lock must NOT block codex — a slow Anthropic refresh never + // head-of-line-blocks another tenant's refresh (the reason §5.4 uses a + // per-tenant lock rather than the global one). + let codex = lock_tenant_refresh(&path, "codex").await; + assert!( + matches!(codex, RefreshLock::Held(_)), + "codex acquire is independent of the held anthropic lock" + ); + + drop(held); + drop(codex); + let after = lock_tenant_refresh(&path, ANTHROPIC_NAMESPACE).await; + assert!( + matches!(after, RefreshLock::Held(_)), + "anthropic acquire succeeds once released" + ); + } } From 7d32cf7e3f762fee7c11a0c00c258043a8787c8b Mon Sep 17 00:00:00 2001 From: chaodu-agent Date: Sun, 28 Jun 2026 04:08:48 +0000 Subject: [PATCH 20/20] fix(review): OAuth model-switch uses env token before auth.json F1/F2: select_provider and ACP model-switch now check CLAUDE_CODE_OAUTH_TOKEN before falling back to the stored anthropic-oauth tenant in auth.json. Fleet pods that only set the env token can switch models without an auth.json file. Adds from_oauth_auto() / from_oauth_auto_with_model() that implement the env-over-store precedence for OAuth rebuild paths. F5: config.rs logs malformed config at error level (was warn) so production monitoring catches a typo'd config.json. --- openab-agent/src/acp.rs | 6 +++--- openab-agent/src/config.rs | 2 +- openab-agent/src/llm.rs | 20 +++++++++++++++++++- 3 files changed, 23 insertions(+), 5 deletions(-) diff --git a/openab-agent/src/acp.rs b/openab-agent/src/acp.rs index 615602697..cf4ca551b 100644 --- a/openab-agent/src/acp.rs +++ b/openab-agent/src/acp.rs @@ -317,8 +317,8 @@ impl AcpServer { let res = match (provider_choice.as_str(), model_override) { ("anthropic", Some(m)) => AnthropicProvider::auto_with_model(m), ("anthropic", None) => AnthropicProvider::auto(), - (_, Some(m)) => AnthropicProvider::from_oauth_store_with_model(m), - (_, None) => AnthropicProvider::from_oauth_store(), + (_, Some(m)) => AnthropicProvider::from_oauth_auto_with_model(m), + (_, None) => AnthropicProvider::from_oauth_auto(), }; match res { Ok(p) => (Box::new(p), "anthropic"), @@ -600,7 +600,7 @@ impl AcpServer { let new_provider: Result, String> = match provider_name { "anthropic" if session_is_oauth => { - AnthropicProvider::from_oauth_store_with_model(value).map(|p| Box::new(p) as _) + AnthropicProvider::from_oauth_auto_with_model(value).map(|p| Box::new(p) as _) } "anthropic" => AnthropicProvider::auto_with_model(value).map(|p| Box::new(p) as _), _ => crate::llm::OpenAiProvider::from_auth_store_with_model(value) diff --git a/openab-agent/src/config.rs b/openab-agent/src/config.rs index a5f6f4d36..cb578fea9 100644 --- a/openab-agent/src/config.rs +++ b/openab-agent/src/config.rs @@ -62,7 +62,7 @@ impl AgentConfig { match Self::load() { Ok(c) => c, Err(e) => { - tracing::warn!("ignoring config: {e}"); + tracing::error!("ignoring config: {e}"); Self::default() } } diff --git a/openab-agent/src/llm.rs b/openab-agent/src/llm.rs index fab48c50e..baec41ae3 100644 --- a/openab-agent/src/llm.rs +++ b/openab-agent/src/llm.rs @@ -161,7 +161,7 @@ pub fn resolve_provider_choice() -> String { pub fn select_provider(choice: &str) -> Result, String> { match choice { "anthropic" => Ok(Box::new(AnthropicProvider::auto()?)), - "anthropic-oauth" | "claude" => Ok(Box::new(AnthropicProvider::from_oauth_store()?)), + "anthropic-oauth" | "claude" => Ok(Box::new(AnthropicProvider::from_oauth_auto()?)), "openai" | "codex" => Ok(Box::new(OpenAiProvider::from_auth_store()?)), _ => match AnthropicProvider::auto() { Ok(p) => Ok(Box::new(p)), @@ -397,6 +397,24 @@ impl AnthropicProvider { Ok(Self::build(AnthropicAuth::OAuth, model.to_string())) } + /// OAuth with env-over-store precedence: `CLAUDE_CODE_OAUTH_TOKEN` → stored + /// `anthropic-oauth` tenant. Lets fleet pods that only set the env token work + /// without an `auth.json`. + pub fn from_oauth_auto() -> Result { + if Self::oauth_env_token().is_some() { + return Self::from_oauth_env(); + } + Self::from_oauth_store() + } + + /// `from_oauth_auto()` with an explicit model override. + pub fn from_oauth_auto_with_model(model: &str) -> Result { + if Self::oauth_env_token().is_some() { + return Self::from_oauth_env_with_model(model); + } + Self::from_oauth_store_with_model(model) + } + fn build_request_body(&self, system: &str, messages: &[Message], tools: &[ToolDef]) -> Value { let oauth = self.is_oauth(); let msgs: Vec =