From 63e2e0f5165442e0f5c3f275a44289839fbc672c Mon Sep 17 00:00:00 2001 From: Henry Park Date: Tue, 10 Mar 2026 11:07:17 -0700 Subject: [PATCH] refactor: address PR review feedback on test credentials MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Fix TEST_CRYPTO_KEY doc comment ("32-byte hex" → "32-character key string") - Rename confusing "real"/"fake" Anthropic constant names and values - Change TEST_STRIPE_KEY from "sk-live" to "sk_test_fake123" to avoid scanners - Use test_secrets_store() helper in orchestrator and http tool tests - Clarify config_round_trip.rs doc comment about integration test visibility Co-Authored-By: Claude Opus 4.6 --- src/config/sandbox.rs | 8 ++++---- src/orchestrator/api.rs | 8 ++------ src/testing/credentials.rs | 10 +++++----- src/tools/builtin/http.rs | 29 ++++------------------------- tests/config_round_trip.rs | 5 +++-- 5 files changed, 18 insertions(+), 42 deletions(-) diff --git a/src/config/sandbox.rs b/src/config/sandbox.rs index 8dc8883e..35be4393 100644 --- a/src/config/sandbox.rs +++ b/src/config/sandbox.rs @@ -408,10 +408,10 @@ mod tests { fn parse_oauth_token_valid() { let json = format!( r#"{{"claudeAiOauth": {{"accessToken": "{}"}}}}"#, - TEST_ANTHROPIC_OAUTH_FAKE + TEST_ANTHROPIC_OAUTH_BASIC ); let token = parse_oauth_access_token(&json); - assert_eq!(token, Some(TEST_ANTHROPIC_OAUTH_FAKE.to_string())); + assert_eq!(token, Some(TEST_ANTHROPIC_OAUTH_BASIC.to_string())); } #[test] @@ -446,11 +446,11 @@ mod tests { "expiresAt": 1700000000 }} }}"#, - TEST_ANTHROPIC_OAUTH_REAL + TEST_ANTHROPIC_OAUTH_NESTED ); assert_eq!( parse_oauth_access_token(&json), - Some(TEST_ANTHROPIC_OAUTH_REAL.to_string()) + Some(TEST_ANTHROPIC_OAUTH_NESTED.to_string()) ); } diff --git a/src/orchestrator/api.rs b/src/orchestrator/api.rs index 0e8a876e..80e09073 100644 --- a/src/orchestrator/api.rs +++ b/src/orchestrator/api.rs @@ -458,7 +458,6 @@ mod tests { use crate::orchestrator::auth::TokenStore; use crate::orchestrator::job_manager::{ContainerJobConfig, ContainerJobManager}; use crate::testing::StubLlm; - use crate::testing::credentials::TEST_CRYPTO_KEY; use super::*; @@ -662,12 +661,9 @@ mod tests { #[tokio::test] async fn credentials_returns_secrets_when_store_configured() { + use crate::testing::credentials::test_secrets_store; use secrecy::SecretString; - let crypto = Arc::new( - crate::secrets::SecretsCrypto::new(SecretString::from(TEST_CRYPTO_KEY.to_string())) - .unwrap(), - ); - let secrets_store = Arc::new(crate::secrets::InMemorySecretsStore::new(crypto)); + let secrets_store = Arc::new(test_secrets_store()); // Create a secret secrets_store diff --git a/src/testing/credentials.rs b/src/testing/credentials.rs index 697370cd..7b6b7442 100644 --- a/src/testing/credentials.rs +++ b/src/testing/credentials.rs @@ -12,7 +12,7 @@ use crate::secrets::{InMemorySecretsStore, SecretsCrypto}; // ── Encryption keys ────────────────────────────────────────────────────── -/// 32-byte hex key for `SecretsCrypto::new()` in tests. +/// 32-character key string for `SecretsCrypto::new()` in tests. pub const TEST_CRYPTO_KEY: &str = "0123456789abcdef0123456789abcdef"; /// 32+ char key for web gateway `SecretsCrypto` in tests. @@ -38,13 +38,13 @@ pub const TEST_OPENAI_API_KEY_ISSUE_129: &str = "sk-test-key-for-issue-129"; pub const TEST_ANTHROPIC_OAUTH_TOKEN: &str = "sk-ant-oat01-test-token"; /// Anthropic API key for priority tests. -pub const TEST_ANTHROPIC_API_KEY: &str = "sk-ant-real-key"; +pub const TEST_ANTHROPIC_API_KEY: &str = "sk-ant-priority-key"; /// Anthropic OAuth token for sandbox config parse tests. -pub const TEST_ANTHROPIC_OAUTH_FAKE: &str = "sk-ant-oat01-fake"; +pub const TEST_ANTHROPIC_OAUTH_BASIC: &str = "sk-ant-oat01-basic"; /// Anthropic OAuth token in nested JSON parse test. -pub const TEST_ANTHROPIC_OAUTH_REAL: &str = "sk-ant-oat01-real-token"; +pub const TEST_ANTHROPIC_OAUTH_NESTED: &str = "sk-ant-oat01-primary-token"; // ── Google OAuth ───────────────────────────────────────────────────────── @@ -89,7 +89,7 @@ pub const TEST_AUTH_SECRET_TOKEN: &str = "secret-token"; // ── Stripe ────────────────────────────────────────────────────────────── /// Stripe-style test key. -pub const TEST_STRIPE_KEY: &str = "sk-live"; +pub const TEST_STRIPE_KEY: &str = "sk_test_fake123"; // ── Redaction test values ─────────────────────────────────────────────── diff --git a/src/tools/builtin/http.rs b/src/tools/builtin/http.rs index 92dcb64b..3b506c24 100644 --- a/src/tools/builtin/http.rs +++ b/src/tools/builtin/http.rs @@ -609,7 +609,7 @@ impl Tool for HttpTool { #[cfg(test)] mod tests { use super::*; - use crate::testing::credentials::{TEST_CRYPTO_KEY, TEST_OPENAI_API_KEY}; + use crate::testing::credentials::{TEST_OPENAI_API_KEY, test_secrets_store}; #[test] fn test_http_tool_schema_headers_is_array() { @@ -869,12 +869,7 @@ mod tests { let tool = HttpTool::new().with_credentials( registry, // secrets_store is not used in requires_approval, just needs to be present - Arc::new(crate::secrets::InMemorySecretsStore::new(Arc::new( - crate::secrets::SecretsCrypto::new(secrecy::SecretString::from( - TEST_CRYPTO_KEY.to_string(), - )) - .unwrap(), - ))), + Arc::new(test_secrets_store()), ); let params = serde_json::json!({ @@ -891,15 +886,7 @@ mod tests { let registry = Arc::new(SharedCredentialRegistry::new()); // Empty registry - no credential mappings - let tool = HttpTool::new().with_credentials( - registry, - Arc::new(crate::secrets::InMemorySecretsStore::new(Arc::new( - crate::secrets::SecretsCrypto::new(secrecy::SecretString::from( - TEST_CRYPTO_KEY.to_string(), - )) - .unwrap(), - ))), - ); + let tool = HttpTool::new().with_credentials(registry, Arc::new(test_secrets_store())); let params = serde_json::json!({ "method": "GET", @@ -958,15 +945,7 @@ mod tests { let registry = Arc::new(SharedCredentialRegistry::new()); registry.add_mappings(vec![CredentialMapping::bearer("test_key", "api.test.com")]); - let tool = HttpTool::new().with_credentials( - registry, - Arc::new(crate::secrets::InMemorySecretsStore::new(Arc::new( - crate::secrets::SecretsCrypto::new(secrecy::SecretString::from( - TEST_CRYPTO_KEY.to_string(), - )) - .unwrap(), - ))), - ); + let tool = HttpTool::new().with_credentials(registry, Arc::new(test_secrets_store())); // These calls should not panic in multi-thread runtime let params_no_auth = serde_json::json!({ diff --git a/tests/config_round_trip.rs b/tests/config_round_trip.rs index d0e8421b..8351ff74 100644 --- a/tests/config_round_trip.rs +++ b/tests/config_round_trip.rs @@ -12,8 +12,9 @@ use tempfile::tempdir; use ironclaw::bootstrap::{save_bootstrap_env_to, upsert_bootstrap_var_to}; -/// Fake OpenAI API key for test use only. Mirrors `TEST_OPENAI_API_KEY_LONG` -/// from `crate::testing::credentials` (unavailable in integration tests). +/// Fake OpenAI API key for test use only. Mirrors the internal +/// `TEST_OPENAI_API_KEY_LONG` constant from the main crate, which is not +/// directly available to integration tests due to `#[cfg(test)]`. const TEST_OPENAI_API_KEY_LONG: &str = "sk-test-key-1234567890"; /// Parse a .env file into a HashMap using dotenvy.