diff --git a/backend/src/errors/mod.rs b/backend/src/errors/mod.rs index cb06c5863..ba28840b9 100644 --- a/backend/src/errors/mod.rs +++ b/backend/src/errors/mod.rs @@ -88,6 +88,11 @@ pub enum AppError { #[error("Bad request: {0}")] BadRequest(String), + /// Known unusable stored credential, distinct from malformed requests or + /// infrastructure failures. Keep the established public bad-request contract. + #[error("Bad request: {0}")] + CredentialUnavailable(String), + #[error("{context} request body exceeds the configured limit of {max_bytes} bytes")] RequestBodyTooLarge { max_bytes: usize, context: String }, @@ -708,7 +713,9 @@ pub enum AppError { impl AppError { fn status_code(&self) -> StatusCode { match self { - Self::BadRequest(_) | Self::ValidationError(_) => StatusCode::BAD_REQUEST, + Self::BadRequest(_) | Self::CredentialUnavailable(_) | Self::ValidationError(_) => { + StatusCode::BAD_REQUEST + } Self::RequestBodyTooLarge { .. } => StatusCode::PAYLOAD_TOO_LARGE, Self::Unauthorized(_) | Self::AuthenticationFailed(_) | Self::TokenExpired => { StatusCode::UNAUTHORIZED @@ -919,7 +926,7 @@ impl AppError { pub(crate) fn error_code(&self) -> u32 { match self { - Self::BadRequest(_) => 1000, + Self::BadRequest(_) | Self::CredentialUnavailable(_) => 1000, Self::RequestBodyTooLarge { .. } => 11700, Self::Unauthorized(_) => 1001, Self::Forbidden(_) => 1002, @@ -1168,7 +1175,7 @@ impl AppError { pub(crate) fn error_key(&self) -> &str { match self { - Self::BadRequest(_) => "bad_request", + Self::BadRequest(_) | Self::CredentialUnavailable(_) => "bad_request", Self::RequestBodyTooLarge { .. } => "request_body_too_large", Self::Unauthorized(_) => "unauthorized", Self::Forbidden(_) => "forbidden", @@ -1501,6 +1508,23 @@ mod tests { assert!(payload["message"].as_str().unwrap().contains("2048 bytes")); } + #[tokio::test] + async fn credential_unavailable_preserves_bad_request_wire_contract() { + let error = AppError::CredentialUnavailable("API key is failed".into()); + assert_eq!(error.oauth_error_code(), "invalid_request"); + let response = error.into_response(); + assert_eq!(response.status(), StatusCode::BAD_REQUEST); + let payload: Value = + serde_json::from_slice(&to_bytes(response.into_body(), 4096).await.unwrap()).unwrap(); + let previous = + serde_json::to_value(AppError::BadRequest("API key is failed".into()).response_body()) + .unwrap(); + assert_eq!(payload, previous); + assert_eq!(payload["error_code"], 1000); + assert_eq!(payload["error"], "bad_request"); + assert_eq!(payload["message"], "Bad request: API key is failed"); + } + #[test] fn status_codes() { assert_eq!( diff --git a/backend/src/handlers/proxy.rs b/backend/src/handlers/proxy.rs index 5ff8170ee..9e14ff3fc 100644 --- a/backend/src/handlers/proxy.rs +++ b/backend/src/handlers/proxy.rs @@ -46,7 +46,7 @@ use crate::telemetry::{TelemetryContext, TelemetryEvent, emit_event}; /// the `return Err(...)` sites in this file. fn proxy_error_telemetry_fields(err: &AppError) -> (u16, u32) { match err { - AppError::BadRequest(_) => (400, 1000), + AppError::BadRequest(_) | AppError::CredentialUnavailable(_) => (400, 1000), AppError::Unauthorized(_) => (401, 1001), AppError::Forbidden(_) => (403, 1002), AppError::NotFound(_) => (404, 1003), @@ -9430,7 +9430,12 @@ mod tests { fn proxy_error_telemetry_fields_maps_common_errors() { use super::proxy_error_telemetry_fields; use crate::errors::AppError; - + assert_eq!( + proxy_error_telemetry_fields(&AppError::CredentialUnavailable( + "API key is failed".into() + )), + (400, 1000), + ); assert_eq!( proxy_error_telemetry_fields(&AppError::BadRequest("x".into())), (400, 1000) diff --git a/backend/src/handlers/service_pool_inspection_tests.rs b/backend/src/handlers/service_pool_inspection_tests.rs index ad72098cf..d3828d757 100644 --- a/backend/src/handlers/service_pool_inspection_tests.rs +++ b/backend/src/handlers/service_pool_inspection_tests.rs @@ -243,3 +243,526 @@ async fn pool_inspection_draft_selection_is_independent_of_search_paging_and_sav ); fixture.state.db.drop().await.unwrap(); } + +#[tokio::test] +async fn pool_inspection_unavailable_credentials_do_not_poison_healthy_peers() { + let fixture = fixture( + "pool_unavailable_credentials", + StatusCode::OK, + "priority", + false, + ) + .await; + bind_first_credential(&fixture).await; + let db = &fixture.state.db; + let first = db + .collection::("user_services") + .find_one(doc! {"slug":"review-first"}) + .await + .unwrap() + .unwrap(); + let first_id = first.get_str("_id").unwrap(); + let key_id = first.get_str("api_key_id").unwrap(); + let peers = format!("{first_id},{}", fixture.second_member_id); + let decrypts = fixture.state.encryption_keys.decrypt_stats(); + for status in [ + "failed", + "inactive", + "expired", + "revoked", + "refresh_failed", + "pending_auth", + ] { + db.collection::("user_api_keys") + .update_one(doc! {"_id":key_id}, doc! {"$set":{"status":status}}) + .await + .unwrap(); + for (query, health) in [ + (json!({"check_operation":false}), false), + (json!({"method":"POST","path":"perform"}), false), + ( + json!({"check_operation":false,"selected_only":true,"peer_ids":peers}), + false, + ), + (json!({"check_operation":false}), true), + (json!({"method":"POST","path":"perform"}), true), + ] { + let result = inspect(&fixture, query, health).await; + assert_eq!(result.candidates.len(), 2, "{status}"); + let unavailable = result + .candidates + .iter() + .find(|r| r.user_service_id == first_id) + .unwrap(); + assert!(!unavailable.eligible, "{status}"); + assert_eq!( + unavailable.reason.as_deref(), + Some("credential_unavailable") + ); + assert!( + result + .candidates + .iter() + .find(|r| r.user_service_id == fixture.second_member_id) + .unwrap() + .eligible + ); + } + assert_eq!(decrypts, fixture.state.encryption_keys.decrypt_stats()); + let response = call(&fixture, "{}").await; + assert_eq!(response.status(), StatusCode::OK); + assert_eq!(response.headers()["x-nyxid-pool-member"], "review-second"); + assert_eq!(response.headers()["x-nyxid-pool-attempts"], "1"); + to_bytes(response.into_body(), 1024).await.unwrap(); + } + // Active metadata without material is unavailable too, including incomplete OAuth. + for credential_type in ["bearer", "oauth2", "gcp_service_account", "node_managed"] { + db.collection::("user_api_keys").update_one( + doc! {"_id":key_id}, + doc! {"$set":{"status":"active","credential_type":credential_type},"$unset":{"credential_encrypted":""}}, + ).await.unwrap(); + let result = inspect(&fixture, json!({"check_operation":false}), false).await; + assert_eq!( + result + .candidates + .iter() + .find(|r| r.user_service_id == first_id) + .unwrap() + .reason + .as_deref(), + Some("credential_unavailable") + ); + assert!( + result + .candidates + .iter() + .find(|r| r.user_service_id == fixture.second_member_id) + .unwrap() + .eligible + ); + } + assert_eq!(decrypts, fixture.state.encryption_keys.decrypt_stats()); + assert!(fixture.first.requests.lock().await.is_empty()); + assert_eq!(fixture.second.requests.lock().await.len(), 6); + let key = db + .collection::("user_api_keys") + .find_one(doc! {"_id":key_id}) + .await + .unwrap() + .unwrap(); + assert!(!key.contains_key("last_used_at")); + db.drop().await.unwrap(); +} + +#[tokio::test] +async fn pool_inspection_credential_integrity_errors_still_fail() { + let fixture = fixture( + "pool_credential_integrity", + StatusCode::OK, + "priority", + false, + ) + .await; + bind_first_credential(&fixture).await; + let db = &fixture.state.db; + let first = db + .collection::("user_services") + .find_one(doc! {"slug":"review-first"}) + .await + .unwrap() + .unwrap(); + let key_id = first.get_str("api_key_id").unwrap(); + let inspect_result = || { + handler::pool_candidates( + State(fixture.state.clone()), + fixture.auth.clone(), + Path(fixture.pool_id.clone()), + Query(serde_json::from_value(json!({"check_operation":false})).unwrap()), + ) + }; + // Deserialization failures from actual database reads must propagate. + db.collection::("user_api_keys") + .update_one(doc! {"_id":key_id}, doc! {"$set":{"status":17}}) + .await + .unwrap(); + assert!(matches!( + inspect_result().await, + Err(crate::errors::AppError::DatabaseError(_)) + )); + db.collection::("user_api_keys") + .delete_one(doc! {"_id":key_id}) + .await + .unwrap(); + assert!(matches!( + inspect_result().await, + Err(crate::errors::AppError::Internal(_)) + )); + assert!(matches!( + try_call(&fixture, "{}").await, + Err(crate::errors::AppError::Internal(_)) + )); + db.collection::("user_endpoints") + .delete_one(doc! {"_id":first.get_str("endpoint_id").unwrap()}) + .await + .unwrap(); + assert!(matches!( + inspect_result().await, + Err(crate::errors::AppError::Internal(_)) + )); + assert!(fixture.first.requests.lock().await.is_empty()); + assert!(fixture.second.requests.lock().await.is_empty()); + db.drop().await.unwrap(); +} + +#[tokio::test] +async fn pool_inspection_failed_agent_override_skips_only_its_member() { + let mut fixture = fixture("pool_failed_override", StatusCode::OK, "priority", false).await; + bind_first_credential(&fixture).await; + let db = &fixture.state.db; + let first = db + .collection::("user_services") + .find_one(doc! {"slug":"review-first"}) + .await + .unwrap() + .unwrap(); + let first_id = first.get_str("_id").unwrap(); + let mut override_key = db + .collection::("user_api_keys") + .find_one(doc! {"_id":first.get_str("api_key_id").unwrap()}) + .await + .unwrap() + .unwrap(); + let override_id = Uuid::new_v4().to_string(); + override_key.insert("_id", &override_id); + override_key.insert("status", "failed"); + db.collection::("user_api_keys") + .insert_one(override_key) + .await + .unwrap(); + let agent_id = Uuid::new_v4().to_string(); + fixture.auth.api_key_id = Some(agent_id.clone()); + db.collection::("agent_service_bindings").insert_one(doc! { + "_id":Uuid::new_v4().to_string(),"api_key_id":&agent_id,"user_id":fixture.auth.user_id.to_string(), + "user_service_id":first_id,"user_api_key_id":&override_id, + "created_at":mongodb::bson::DateTime::now(),"updated_at":mongodb::bson::DateTime::now(), + }).await.unwrap(); + let decrypts = fixture.state.encryption_keys.decrypt_stats(); + for (query, health) in [ + (json!({"check_operation":false}), false), + (json!({"method":"POST","path":"perform"}), false), + ( + json!({"check_operation":false,"selected_only":true,"peer_ids":format!("{first_id},{}",fixture.second_member_id)}), + false, + ), + (json!({"check_operation":false}), true), + (json!({"method":"POST","path":"perform"}), true), + ] { + let result = inspect(&fixture, query, health).await; + assert_eq!( + result + .candidates + .iter() + .find(|r| r.user_service_id == first_id) + .unwrap() + .reason + .as_deref(), + Some("credential_unavailable") + ); + assert!( + result + .candidates + .iter() + .find(|r| r.user_service_id == fixture.second_member_id) + .unwrap() + .eligible + ); + } + let owner = fixture.auth.user_id.to_string(); + let plan = crate::services::service_pool_service::plan_candidates_with_allowlist( + db, + &fixture.state.encryption_keys, + &owner, + Some(&agent_id), + &owner, + "review-route", + &Method::POST, + Some("perform"), + 2, + Some(b"{}"), + None, + None, + ) + .await + .unwrap(); + assert_eq!(plan.candidates.len(), 1); + assert_eq!(plan.candidates[0].service.id, fixture.second_member_id); + assert_eq!(decrypts, fixture.state.encryption_keys.decrypt_stats()); + db.collection::("user_api_keys") + .delete_one(doc! {"_id":&override_id}) + .await + .unwrap(); + let result = handler::health( + State(fixture.state.clone()), + fixture.auth.clone(), + Path(fixture.pool_id.clone()), + Query(serde_json::from_value(json!({"method":"POST","path":"perform"})).unwrap()), + ) + .await; + assert!(matches!(result, Err(crate::errors::AppError::Internal(_)))); + db.drop().await.unwrap(); +} + +#[tokio::test] +async fn pool_inspection_node_platform_and_noauth_do_not_require_server_user_credentials() { + let mut fixture = fixture( + "pool_credential_bindings", + StatusCode::OK, + "priority", + false, + ) + .await; + bind_first_credential(&fixture).await; + let db = &fixture.state.db; + let first = db + .collection::("user_services") + .find_one(doc! {"slug":"review-first"}) + .await + .unwrap() + .unwrap(); + let first_id = first.get_str("_id").unwrap(); + let key_id = first.get_str("api_key_id").unwrap(); + db.collection::("user_api_keys").update_one(doc! {"_id":key_id},doc! {"$set":{"status":"failed","credential_type":"node_managed"},"$unset":{"credential_encrypted":""}}).await.unwrap(); + let decrypts = fixture.state.encryption_keys.decrypt_stats(); + let node_id = Uuid::new_v4().to_string(); + db.collection::("user_services") + .update_one(doc! {"_id":first_id}, doc! {"$set":{"node_id":&node_id}}) + .await + .unwrap(); + let now = chrono::Utc::now(); + db.collection::("nodes").insert_one(doc! { + "_id":&node_id,"user_id":fixture.auth.user_id.to_string(),"name":"pool test node","status":"online","auth_token_hash":"test-only","is_active":true, + "created_at":mongodb::bson::DateTime::from_chrono(now),"updated_at":mongodb::bson::DateTime::from_chrono(now), + "connection_owner":{"instance_name":"test","generation_id":"test","connection_id":"test","internal_base_url":"http://127.0.0.1:1", + "claimed_at":mongodb::bson::DateTime::from_chrono(now),"renewed_at":mongodb::bson::DateTime::from_chrono(now),"expires_at":mongodb::bson::DateTime::from_chrono(now+chrono::Duration::hours(1)),"http_cancellation":true}, + }).await.unwrap(); + let node_result = inspect(&fixture, json!({"check_operation":false}), false).await; + assert!(node_result.candidates.iter().all(|r| r.eligible)); + // Node-owned credentials remain excluded by the effective route ACL. + fixture.auth.allow_all_nodes = false; + fixture.auth.allowed_node_ids = vec![]; + + for health in [false, true] { + let result = inspect(&fixture, json!({"check_operation":false}), health).await; + assert_eq!(result.candidates.len(), 1); + assert_eq!( + result.candidates[0].user_service_id, + fixture.second_member_id + ); + } + fixture.auth.allow_all_nodes = true; + fixture.auth.allow_all_services = false; + fixture.auth.allowed_service_ids = vec![fixture.second_member_id.clone()]; + assert_eq!( + inspect(&fixture, json!({"check_operation":false}), false) + .await + .candidates + .len(), + 1 + ); + fixture.auth.allow_all_services = true; + db.collection::("user_services") + .update_one( + doc! {"_id":first_id}, + doc! {"$set":{"auth_method":"none"},"$unset":{"node_id":""}}, + ) + .await + .unwrap(); + assert!( + inspect(&fixture, json!({"check_operation":false}), false) + .await + .candidates + .iter() + .all(|r| r.eligible) + ); + db.collection::("downstream_services").update_one(doc! {"_id":first.get_str("catalog_service_id").unwrap()},doc! {"$set":{"auth_method":"bearer","service_category":"internal","requires_user_credential":false,"visibility":"public","credential_encrypted":mongodb::bson::Binary{subtype:mongodb::bson::spec::BinarySubtype::Generic,bytes:vec![1]}}}).await.unwrap(); + db.collection::("user_services") + .update_one( + doc! {"_id":first_id}, + doc! {"$set":{"auth_method":"bearer","credential_binding":"platform"}}, + ) + .await + .unwrap(); + db.collection::("node_service_bindings").insert_one(doc! { + "_id":Uuid::new_v4().to_string(),"node_id":&node_id,"user_id":fixture.auth.user_id.to_string(), + "service_id":first.get_str("catalog_service_id").unwrap(),"is_active":true,"priority":0, + "created_at":mongodb::bson::DateTime::from_chrono(now),"updated_at":mongodb::bson::DateTime::from_chrono(now), + }).await.unwrap(); + fixture.auth.allow_all_nodes = false; + let platform = inspect(&fixture, json!({"check_operation":false}), false).await; + let row = platform + .candidates + .iter() + .find(|r| r.user_service_id == first_id) + .unwrap(); + assert!(row.eligible); + assert_eq!(row.credential_binding, "platform"); + assert_eq!(decrypts, fixture.state.encryption_keys.decrypt_stats()); + assert!(fixture.first.requests.lock().await.is_empty()); + db.drop().await.unwrap(); +} + +#[test] +fn pool_credential_unavailability_is_narrowly_classified() { + use crate::errors::AppError; + use crate::services::service_pool_service::member_unavailable; + assert!(member_unavailable(&AppError::CredentialUnavailable( + "failed".into() + ))); + assert!(!member_unavailable(&AppError::BadRequest( + "API key is failed".into() + ))); + assert!(!member_unavailable(&AppError::BadRequest( + "Invalid proxy path".into() + ))); + assert!(!member_unavailable(&AppError::Internal( + "missing key row".into() + ))); + assert!(!member_unavailable(&AppError::ValidationError( + "invalid platform route".into() + ))); +} + +#[tokio::test] +async fn pool_inspection_name_search_precedes_paging_and_preserves_scope() { + let mut fixture = fixture("pool_name_search", StatusCode::OK, "priority", false).await; + let db = &fixture.state.db; + let services: Vec = db + .collection::("user_services") + .find(doc! {}) + .sort(doc! {"_id":1}) + .await + .unwrap() + .try_collect() + .await + .unwrap(); + let first_id = services[0].get_str("_id").unwrap(); + let last = &services[1]; + let last_id = last.get_str("_id").unwrap(); + let label = "Preferred member [West]"; + db.collection::("user_endpoints") + .update_one( + doc! {"_id":last.get_str("endpoint_id").unwrap()}, + doc! {"$set":{"label":label}}, + ) + .await + .unwrap(); + let decrypts = fixture.state.encryption_keys.decrypt_stats(); + let page = inspect(&fixture, json!({"check_operation":false,"limit":1}), false).await; + assert_eq!(page.candidates[0].user_service_id, first_id); + for search in ["preferred MEMBER [west]", last.get_str("slug").unwrap()] { + let found = inspect( + &fixture, + json!({"check_operation":false,"search":search,"limit":1}), + false, + ) + .await; + assert_eq!(found.candidates.len(), 1); + assert_eq!(found.candidates[0].user_service_id, last_id); + assert_eq!(found.candidates[0].name, label); + assert!(!found.has_more); + } + assert!( + inspect( + &fixture, + json!({"check_operation":false,"search":".*"}), + false + ) + .await + .candidates + .is_empty() + ); + // Matching foreign-owned instances must never enter the candidate result. + let mut foreign = last.clone(); + foreign.insert("_id", Uuid::new_v4().to_string()); + foreign.insert("user_id", Uuid::new_v4().to_string()); + db.collection::("user_services") + .insert_one(foreign) + .await + .unwrap(); + assert_eq!( + inspect( + &fixture, + json!({"check_operation":false,"search":label}), + false + ) + .await + .candidates + .len(), + 1 + ); + fixture.auth.allow_all_services = false; + fixture.auth.allowed_service_ids = vec![first_id.to_owned()]; + assert!( + inspect( + &fixture, + json!({"check_operation":false,"search":label}), + false + ) + .await + .candidates + .is_empty() + ); + let selected = inspect(&fixture, json!({"check_operation":false,"search":"no match","selected_only":true,"peer_ids":format!("{first_id},{last_id}"),"limit":1,"after":"999"}), false).await; + assert_eq!(selected.candidates.len(), 1); + assert_eq!(selected.candidates[0].user_service_id, first_id); + fixture.auth.allow_all_services = true; + // Platform resolution also displays the endpoint label, with no credential materialization. + db.collection::("downstream_services").update_one( + doc! {"_id":last.get_str("catalog_service_id").unwrap()}, + doc! {"$set":{"auth_method":"bearer","service_category":"internal","requires_user_credential":false,"visibility":"public","credential_encrypted":mongodb::bson::Binary{subtype:mongodb::bson::spec::BinarySubtype::Generic,bytes:vec![1]}}}, + ).await.unwrap(); + db.collection::("user_services") + .update_one( + doc! {"_id":last_id}, + doc! {"$set":{"credential_binding":"platform","auth_method":"bearer"}}, + ) + .await + .unwrap(); + let platform = inspect( + &fixture, + json!({"check_operation":false,"search":label}), + false, + ) + .await; + assert_eq!(platform.candidates.len(), 1); + assert_eq!(platform.candidates[0].name, label); + assert_eq!(platform.candidates[0].credential_binding, "platform"); + assert!(platform.candidates[0].eligible); + // Matching-name pages retain stable pagination, without consuming unrelated rows. + db.collection::("user_endpoints") + .update_one( + doc! {"_id":services[0].get_str("endpoint_id").unwrap()}, + doc! {"$set":{"label":"Another member [West]"}}, + ) + .await + .unwrap(); + let page = inspect( + &fixture, + json!({"check_operation":false,"search":"member [West]","limit":1}), + false, + ) + .await; + assert_eq!(page.candidates.len(), 1); + assert!(page.has_more); + let next = inspect(&fixture, json!({"check_operation":false,"search":"member [West]","limit":1,"after":page.next_cursor}), false).await; + assert_eq!(next.candidates.len(), 1); + assert!(!next.has_more); + assert_ne!( + page.candidates[0].user_service_id, + next.candidates[0].user_service_id + ); + assert_eq!(decrypts, fixture.state.encryption_keys.decrypt_stats()); + assert!(fixture.first.requests.lock().await.is_empty()); + assert!(fixture.second.requests.lock().await.is_empty()); + db.drop().await.unwrap(); +} diff --git a/backend/src/services/proxy_service.rs b/backend/src/services/proxy_service.rs index 8c179feb2..b0977c382 100644 --- a/backend/src/services/proxy_service.rs +++ b/backend/src/services/proxy_service.rs @@ -3037,7 +3037,7 @@ async fn finish_resolution( } if api_key.status != "active" { - return Err(AppError::BadRequest(format!( + return Err(AppError::CredentialUnavailable(format!( "API key is {}", api_key.status ))); @@ -3347,7 +3347,7 @@ pub async fn read_agent_credential_override_identity( ) .await?; if api_key.status != "active" || !credential_is_materializable(db, &api_key).await? { - return Err(AppError::BadRequest( + return Err(AppError::CredentialUnavailable( "Bound credential is not executable".to_string(), )); } @@ -3407,7 +3407,7 @@ pub async fn resolve_agent_credential_override_identity( .await?; if api_key.status != "active" { - return Err(AppError::BadRequest(format!( + return Err(AppError::CredentialUnavailable(format!( "Override credential is {}", api_key.status ))); @@ -3636,11 +3636,13 @@ pub(crate) async fn credential_is_materializable( fn missing_user_api_key_credential_error(api_key: &UserApiKey) -> AppError { match api_key.credential_type.as_str() { - "oauth2" if api_key.provider_config_id.is_some() => AppError::BadRequest( + "oauth2" if api_key.provider_config_id.is_some() => AppError::CredentialUnavailable( "OAuth connection is not complete. Connect your account first.".to_string(), ), - "oauth2" => AppError::BadRequest("OAuth token has no credential stored".to_string()), - _ => AppError::BadRequest( + "oauth2" => { + AppError::CredentialUnavailable("OAuth token has no credential stored".to_string()) + } + _ => AppError::CredentialUnavailable( "No credential stored. Add a credential or route through a node.".to_string(), ), } @@ -8514,7 +8516,9 @@ mod tests { credential_epoch: 1, }; let err = missing_user_api_key_credential_error(&key); - assert!(matches!(err, AppError::BadRequest(m) if m.contains("OAuth connection"))); + assert!( + matches!(err, AppError::CredentialUnavailable(m) if m.contains("OAuth connection")) + ); } #[test] @@ -8546,7 +8550,7 @@ mod tests { credential_epoch: 1, }; let err = missing_user_api_key_credential_error(&key); - assert!(matches!(err, AppError::BadRequest(m) if m.contains("No credential"))); + assert!(matches!(err, AppError::CredentialUnavailable(m) if m.contains("No credential"))); } // ---- forward header: AWS and GCP prefixes ---- diff --git a/backend/src/services/service_pool_inspection.rs b/backend/src/services/service_pool_inspection.rs index e8d2c9450..b53bf3848 100644 --- a/backend/src/services/service_pool_inspection.rs +++ b/backend/src/services/service_pool_inspection.rs @@ -2,7 +2,7 @@ use super::{proxy_service, service_pool_health_service as health, service_pool_service}; use crate::{ crypto::aes::EncryptionKeys, - errors::AppResult, + errors::{AppError, AppResult}, models::{ service_pool::{PoolMemberContract, PoolStrategy, ServicePool}, user_service::UserService, @@ -112,32 +112,27 @@ pub async fn inspect( filter.insert("_id", doc! { "$in": pool.map(|p| p.members.iter().filter(|m| allowed_services.is_none_or(|a| a.contains(&m.user_service_id))).map(|m| m.user_service_id.clone()).collect::>()).unwrap_or_default() }); } else if query.selected_only { filter.insert("_id", doc! {"$in": query.peer_ids.unwrap_or_default().iter().filter(|id| allowed_services.is_none_or(|allowed| allowed.contains(*id))).cloned().collect::>()}); - } else { - if let Some(search) = query.search.filter(|s| !s.is_empty()) { - filter.insert( - "slug", - doc! { "$regex": regex::escape(search), "$options":"i" }, - ); - } } let limit = if query.members_only || query.selected_only { 50 } else { query.limit.clamp(1, 100) as usize }; - let mut services: Vec = + let search = query + .search + .filter(|search| !query.members_only && !query.selected_only && !search.is_empty()); + let mut services: Vec = if let Some(search) = search { + search_services(db, filter, search, offset, limit + 1).await? + } else { crate::services::service_history::collection(db, "user_services") .find(filter) .sort(doc! {"_id":1}) - .skip(if query.members_only || query.selected_only { - 0 - } else { - offset - }) + .skip(offset) .limit((limit + 1) as i64) .await? .try_collect() - .await?; + .await? + }; let has_more = services.len() > limit; services.truncate(limit); let next_cursor = has_more.then(|| (offset + limit as u64).to_string()); @@ -282,54 +277,64 @@ pub async fn inspect( row.reason = Some("operation_unsupported".into()); } } - if saved_operation && priority && row.reason.is_none() { - let credential_override = if let Some(agent) = agent_key { - proxy_service::read_agent_credential_override_identity( - db, - actor, - agent, - &service.id, - &resolution.target, - ) - .await? - } else { - None - }; - if let Some(pool) = pool { - let scope = health::scope_from_resolution( - db, - &pool.id, - owner, - pool.config_revision, - &resolution, - member.and_then(|m| m.model.clone()), - credential_override.as_ref(), - method, - &native_path, - ) - .await; - match scope { - Ok(scope) => { - if let Some(current) = - health::load_for_scope(db, &scope).await? - { - row.cooldown_until = current - .cooldown_until - .filter(|until| *until > chrono::Utc::now()); - row.consecutive_failures = - current.consecutive_failures as i64; - row.last_status = current.last_status; - } - } - Err(error) if service_pool_service::member_unavailable(&error) => { - row.reason = Some("operation_unsupported".into()) + let credential_override = if let Some(agent) = agent_key { + match proxy_service::read_agent_credential_override_identity( + db, + actor, + agent, + &service.id, + &resolution.target, + ) + .await + { + Ok(identity) => identity, + Err(AppError::CredentialUnavailable(_)) => { + row.reason = Some("credential_unavailable".into()); + None + } + Err(error) => return Err(error), + } + } else { + None + }; + if saved_operation + && priority + && row.reason.is_none() + && let Some(pool) = pool + { + let scope = health::scope_from_resolution( + db, + &pool.id, + owner, + pool.config_revision, + &resolution, + member.and_then(|m| m.model.clone()), + credential_override.as_ref(), + method, + &native_path, + ) + .await; + match scope { + Ok(scope) => { + if let Some(current) = health::load_for_scope(db, &scope).await? { + row.cooldown_until = current + .cooldown_until + .filter(|until| *until > chrono::Utc::now()); + row.consecutive_failures = current.consecutive_failures as i64; + row.last_status = current.last_status; } - Err(error) => return Err(error), } + Err(error) if service_pool_service::member_unavailable(&error) => { + row.reason = Some("operation_unsupported".into()) + } + Err(error) => return Err(error), } } } Ok(None) => row.reason = Some("unavailable".into()), + Err(AppError::CredentialUnavailable(_)) => { + row.reason = Some("credential_unavailable".into()) + } Err(error) if service_pool_service::member_unavailable(&error) => { row.reason = Some("unavailable".into()) } @@ -432,3 +437,34 @@ pub async fn inspect( next_cursor, }) } + +/// Search before pagination, projecting only the referenced endpoint label. +/// Both personal and platform resolution use this label as the displayed name. +async fn search_services( + db: &mongodb::Database, + filter: mongodb::bson::Document, + search: &str, + offset: u64, + limit: usize, +) -> AppResult> { + let pattern = doc! {"$regex":regex::escape(search),"$options":"i"}; + Ok(db + .collection::("user_services") + .aggregate([ + doc! {"$match":filter}, + doc! {"$sort":{"_id":1}}, + doc! {"$lookup":{ + "from":"user_endpoints","localField":"endpoint_id","foreignField":"_id", + "pipeline":[{"$project":{"_id":0,"label":1}}],"as":"search_endpoint", + }}, + doc! {"$match":{"$or":[{"slug":&pattern},{"search_endpoint.label":pattern}]}}, + doc! {"$skip":offset as i64}, + doc! {"$limit":limit as i64}, + doc! {"$unset":"search_endpoint"}, + ]) + .max_time(std::time::Duration::from_secs(5)) + .await? + .with_type::() + .try_collect() + .await?) +} diff --git a/backend/src/services/service_pool_service.rs b/backend/src/services/service_pool_service.rs index 8576adec1..8095757fc 100644 --- a/backend/src/services/service_pool_service.rs +++ b/backend/src/services/service_pool_service.rs @@ -630,6 +630,7 @@ pub fn member_unavailable(error: &AppError) -> bool { | AppError::ApiKeyScopeForbidden(_) | AppError::Forbidden(_) | AppError::RequiredServiceNotConnected { .. } + | AppError::CredentialUnavailable(_) ) } @@ -923,14 +924,19 @@ pub async fn plan_candidates_with_allowlist( } let override_identity = match actor_api_key_id { Some(agent_key_id) => { - crate::services::proxy_service::read_agent_credential_override_identity( + match crate::services::proxy_service::read_agent_credential_override_identity( db, actor_user_id, agent_key_id, &service.id, &resolution.target, ) - .await? + .await + { + Ok(identity) => identity, + Err(AppError::CredentialUnavailable(_)) => continue, + Err(error) => return Err(error), + } } None => None, }; diff --git a/docs/API.md b/docs/API.md index d24add461..be9ee30c5 100644 --- a/docs/API.md +++ b/docs/API.md @@ -3771,7 +3771,7 @@ Candidate and health inspection accept these query parameters: | `strategy`, `member_contract` | Draft routing strategy and contract for candidate inspection. Health uses the saved configuration. | | `peer_ids`, `declared_peer_ids` | Comma-separated draft connection UUIDs and Same API compatibility declarations, capped at 50 IDs each. | | `selected_only` | With `true`, candidate inspection returns the selected `peer_ids` independently of search and pagination. | -| `search`, `limit`, `after` | Candidate inventory search and pagination. `limit` defaults to 100 and is clamped to 1–100. | +| `search`, `limit`, `after` | Case-insensitive name or slug search, applied before candidate pagination. `limit` defaults to 100 and is clamped to 1–100. | | `org_id` | Organization owner for new-pool candidate inspection. Existing-pool routes resolve the owner from the pool. | Responses contain `operation_checked`, `method`, `path`, `candidates`, @@ -3782,6 +3782,12 @@ protocol, compatibility requirements, and cooldown metadata. Inventory results do not establish that a particular operation can execute. Inspection is read-only and never decrypts credentials or sends a request to a provider. +A connection with an inactive stored credential or missing credential material +returns `eligible: false` with `reason: "credential_unavailable"`; other +connections remain in the response. Repair that connection before selecting it. +Priority execution skips such members before dispatch. Database and data +integrity errors still fail the request. + ### Proxy #### ANY /api/v1/proxy/{service_id}/{*path} diff --git a/docs/SERVICE_POOLS.md b/docs/SERVICE_POOLS.md index f4e546ff7..aa6dcb7c7 100644 --- a/docs/SERVICE_POOLS.md +++ b/docs/SERVICE_POOLS.md @@ -260,6 +260,22 @@ Disable or cooldown state. Saved operation checks still report both. Legacy round-robin/weighted pools never use priority cooldown health. Health always uses the saved configuration and directly fetches its members, including unavailable rows. +If a connection's credential is failed, inactive, or missing, inspection marks +that connection `credential_unavailable` while keeping healthy connections +available. Reconnect or update the affected connection in Services. Priority +pools skip known unavailable credentials before sending a request, so a broken +primary credential does not block a healthy backup. Inspection checks stored +state; it does not test credentials against the provider. + +Use the dashboard's searchable multi-select dropdown to choose connections. +Search matches connection names and slugs across the inventory. +It stays open while selecting; select a checked connection again to remove it. +The selected connections appear in the form with priority and model settings. +Unavailable connections show a repair reason and cannot be added. +The dashboard requires at least one selected connection when creating a pool. +The CLI and API still support creating an empty draft and adding members later; +existing empty drafts remain editable in the dashboard. + Save settings and members together with `nyxid pool update --file update.json`: ```json diff --git a/docs/plans/service-pool-relook-validation.md b/docs/plans/service-pool-relook-validation.md index f52c042b0..2f8fea757 100644 --- a/docs/plans/service-pool-relook-validation.md +++ b/docs/plans/service-pool-relook-validation.md @@ -48,6 +48,7 @@ bash scripts/check-rci-backend-boundary.sh npm --prefix frontend test npm --prefix frontend run lint npm --prefix frontend run build +npm --prefix frontend run test:e2e -- e2e/service-pool-picker.spec.ts ``` Initial local results: 108 service-pool backend tests, 39 supplemental pool tests, 14 node-dispatch tests, one billing-route test, 15 adapter tests, and 31 CLI pool tests passed. The full frontend suite passed 4,084 tests in 406 files before the final DOM and overlay corrections. The 24 focused component and hook tests, TypeScript, and ESLint passed after those corrections. Workspace Clippy, formatting, the backend boundary check, and the production frontend build passed. @@ -62,6 +63,54 @@ Layout checks used 128-character names and 80-character slugs at 390, 768, 1024, The permanent component regressions cover nonmodal menu ownership, repeated cancellation, keyboard navigation into all four dialogs, Tab and Escape behavior, reopening, and focus restoration to another pool's name button. Backend regressions assert both attempt audit rows for 429 fallback and weighted legacy routing with a single attempt. +## Unavailable-credential regression + +The follow-up to PR #1731 reproduced a candidate-inventory HTTP 400 with +`Bad request: API key is failed` using a real local backend and a disposable +connection whose stored key status was `failed`. A permanent backend regression +failed with the same error before the correction. + +Regression coverage now checks six nonactive credential states, active keys with +missing credential material, and unavailable agent overrides across inventory, +explicit operations, selected draft members, and saved health. It checks healthy +backup selection before dispatch, zero inspection decryptions or last-used +writes, preserved node/platform/no-auth behavior, service/node scope filtering, +and propagation of malformed or missing database records. Direct proxy errors +retain their HTTP 400 payload and telemetry classification. Component tests +cover the disabled credential row, healthy selection, the new-pool connection +requirement, and edits to existing empty drafts. + +The searchable multi-select dropdown keeps selections visible, supports +select/deselect without closing, preserves member configuration across search, +and restores trigger focus when Escape closes the dropdown inside the editor. +Component tests cover its 50-member limit, unavailable options, loading, empty +results, retry, and pagination. + +The full frontend suite passed 4,113 tests in 411 files after the dropdown change; +the 31 focused pool component/hook tests, lint, and production build also passed. +All 113 backend service-pool tests passed after the credential correction. +After adding name search, all ten inspection tests passed, including literal, +case-insensitive matching before pagination, owner/service scope restrictions, +platform labels, and selected-member independence. The 131 proxy-service, +21 error-contract, and 14 proxy-telemetry tests passed. Workspace Clippy, +formatting, and the backend boundary check passed. +The final independent live server/CLI/browser smoke passed 11 checks covering +the exact HTTP 400 contract, empty drafts, candidate inventory and operation +checks, name search before pagination, saved health, and healthy fallback with +one dispatch attempt. The browser searched displayed names, selected and +deselected connections, handled unavailable rows, repeated three Escape/reopen +cycles, checked the 390-pixel layout, and created/edited/deleted a pool against +the backend, with zero window, page, or console errors in the production build. + +The browser regression also captures window errors directly because Vite's +overlay can hide ResizeObserver errors from Playwright's page-error listener. +Changing selected cards below the connection picker now leaves its anchor in +place. The permanent browser test reproduced the observer error in all three +runs against the previous placement and passed all five runs after the fix. +It covers search shrinking and expanding, repeated selection and deselection, +mobile-to-desktop resizing, and Escape/focus restoration. The 31 focused tests, +TypeScript, scoped lint, and production build passed after this layout change. + ## Verification scope The local backend runs target service pools and their integration boundaries; they are not a claim that every backend test ran locally. The PR's CI jobs run the full selected backend, CLI, frontend, feature, and coverage suites. diff --git a/frontend/e2e/service-pool-picker.spec.ts b/frontend/e2e/service-pool-picker.spec.ts new file mode 100644 index 000000000..ad49a1231 --- /dev/null +++ b/frontend/e2e/service-pool-picker.spec.ts @@ -0,0 +1,189 @@ +import { expect, test, type Page } from "@playwright/test"; + +const ids = [ + "11111111-1111-4111-8111-111111111111", + "22222222-2222-4222-8222-222222222222", + "33333333-3333-4333-8333-333333333333", +]; +const candidates = ids.map((id, index) => ({ + user_service_id: id, + name: index === 2 ? "Failed connection" : `Local pool member ${index + 1}`, + slug: index === 2 ? "failed-connection" : `local-pool-member-${index + 1}`, + is_active: true, + eligible: false, + reason: + index === 2 + ? "credential_unavailable" + : "compatibility_declaration_required", + credential_binding: index === 2 ? "user" : "none", + protocol: null, + catalog_service_id: null, + requires_compatibility_declaration: true, + cooldown_until: null, + consecutive_failures: 0, + last_status: null, +})); + +async function mockConnections(page: Page) { + await page.route("**/api/v1/**", async (route) => { + const url = new URL(route.request().url()); + const path = url.pathname.replace("/api/v1", ""); + let body: unknown = {}; + if (path === "/users/me") { + body = { + id: "44444444-4444-4444-8444-444444444444", + email: "pool-layout@example.test", + display_name: "Local review", + role: "user", + is_active: true, + email_verified: true, + created_at: "2026-10-01T00:00:00Z", + }; + } else if (path === "/orgs") body = { orgs: [], organizations: [] }; + else if (path === "/catalog") body = { entries: [] }; + else if (path === "/keys" || path === "/api-keys") body = { keys: [] }; + else if (path === "/service-pools") body = { pools: [] }; + else if (path.endsWith("/candidates")) { + const search = (url.searchParams.get("search") ?? "").toLowerCase(); + const peers = (url.searchParams.get("peer_ids") ?? "").split(","); + body = { + candidates: candidates.filter((row) => + url.searchParams.get("selected_only") === "true" + ? peers.includes(row.user_service_id) + : row.name.toLowerCase().includes(search) || + row.slug.includes(search), + ), + has_more: false, + next_cursor: null, + operation_checked: false, + method: null, + path: null, + }; + } else if (path === "/runtime-config") { + body = { + release_integrity: { + enabled: false, + manifest_url: null, + verification_ttl_secs: 300, + }, + }; + } else if (path === "/nodes") body = { nodes: [] }; + else if (path === "/providers") body = { providers: [] }; + else if (path === "/services") body = { services: [] }; + await route.fulfill({ json: body }); + }); +} + +test("pool multi-select survives responsive resize and changing member cards without observer errors", async ({ + page, +}, testInfo) => { + const runtimeErrors: string[] = []; + page.on("pageerror", (error) => runtimeErrors.push(error.message)); + page.on("console", (entry) => { + if (entry.type() === "error") runtimeErrors.push(entry.text()); + }); + // Vite's overlay may stop propagation before Playwright receives pageerror. + // Record browser errors in capture phase too; never suppress the event. + await page.addInitScript(() => { + const errors: string[] = []; + Object.assign(window, { poolLayoutErrors: errors }); + window.addEventListener( + "error", + (event) => { + if (event.message) errors.push(event.message); + }, + true, + ); + }); + await mockConnections(page); + await page.setViewportSize({ width: 1440, height: 1000 }); + await page.goto("/keys?tab=pools"); + await page.getByRole("button", { name: /^Create pool$/i }).click(); + const form = page.getByRole("dialog", { + name: "Create service pool", + exact: true, + }); + await form.getByLabel("Name", { exact: true }).fill("Responsive pool"); + const trigger = form.getByRole("button", { + name: "Choose connections", + exact: true, + }); + await trigger.click(); + const picker = page.getByRole("dialog", { + name: "Choose pool connections", + exact: true, + }); + const search = picker.getByRole("combobox", { + name: "Search candidate services", + }); + await expect( + picker.getByRole("option", { name: "Failed connection", exact: true }), + ).toHaveAttribute("aria-disabled", "true"); + await page.screenshot({ + path: testInfo.outputPath("picker-desktop.png"), + animations: "disabled", + }); + await page.setViewportSize({ width: 390, height: 844 }); + await page.screenshot({ + path: testInfo.outputPath("picker-mobile.png"), + animations: "disabled", + }); + for (const element of [form, picker]) { + expect( + await element.evaluate( + (node) => node.scrollWidth <= node.clientWidth + 1, + ), + ).toBe(true); + const bounds = await element.boundingBox(); + expect(bounds!.x).toBeGreaterThanOrEqual(0); + expect(bounds!.x + bounds!.width).toBeLessThanOrEqual(391); + } + await page.setViewportSize({ width: 1440, height: 1000 }); + for (let member = 1; member <= 2; member++) { + await search.fill(`Local pool member ${member}`); + const option = picker.getByRole("option", { + name: `Local pool member ${member}`, + exact: true, + }); + await option.click(); + await expect(option).toHaveAttribute("aria-selected", "true"); + await expect(picker).toBeVisible(); + if (member === 1) { + await search.fill(""); + await expect(picker.getByRole("option")).toHaveCount(3); + } + if (member === 2) { + for (let cycle = 0; cycle < 3; cycle++) { + await option.click(); + await expect(option).toHaveAttribute("aria-selected", "false"); + await option.click(); + await expect(option).toHaveAttribute("aria-selected", "true"); + } + } + } + await picker.getByRole("button", { name: "Done", exact: true }).click(); + await expect(trigger).toBeFocused(); + for (let member = 1; member <= 2; member++) { + await form + .getByRole("checkbox", { + name: `Confirm API compatibility for member ${member}`, + }) + .check(); + } + await expect( + form.getByRole("button", { name: "Create pool", exact: true }), + ).toBeEnabled(); + for (let cycle = 0; cycle < 3; cycle++) { + await trigger.click(); + await search.press("Escape"); + await expect(trigger).toBeFocused(); + await expect(form).toBeVisible(); + } + expect(runtimeErrors).toEqual([]); + expect( + await page.evaluate( + () => + (window as Window & { poolLayoutErrors: string[] }).poolLayoutErrors, + ), + ).toEqual([]); +}); diff --git a/frontend/src/components/dashboard/pool-connection-picker.test.tsx b/frontend/src/components/dashboard/pool-connection-picker.test.tsx new file mode 100644 index 000000000..d42adbc6e --- /dev/null +++ b/frontend/src/components/dashboard/pool-connection-picker.test.tsx @@ -0,0 +1,232 @@ +import { useState } from "react"; +import { render, screen, waitFor } from "@testing-library/react"; +import userEvent from "@testing-library/user-event"; +import { describe, expect, it, vi } from "vitest"; +import { + Dialog, + DialogContent, + DialogDescription, + DialogTitle, +} from "@/components/ui/dialog"; +import type { PoolCandidate } from "@/schemas/pools"; +import { PoolConnectionPicker } from "./pool-connection-picker"; + +const candidate: PoolCandidate = { + user_service_id: "one", + name: "Primary connection", + slug: "primary", + is_active: true, + credential_binding: "user", + protocol: null, + eligible: true, + reason: null, + catalog_service_id: "catalog", + requires_compatibility_declaration: false, + cooldown_until: null, + consecutive_failures: 0, + last_status: null, +}; +const backup = { + ...candidate, + user_service_id: "two", + name: "Backup connection", +}; +const failed = { + ...candidate, + user_service_id: "failed", + name: "Failed connection", + eligible: false, + reason: "credential_unavailable", +}; +const defaults = { + rows: [candidate, backup, failed], + selectedIds: [], + search: "", + onSearch: vi.fn(), + onToggle: vi.fn(), + isLoading: false, + isSearching: false, + isError: false, + error: null, + onRetry: vi.fn(), + hasNextPage: false, + isFetchingNextPage: false, + onLoadMore: vi.fn(), +}; +function Harness({ initialIds = [] }: { initialIds?: string[] }) { + const [ids, setIds] = useState(initialIds); + const [search, setSearch] = useState(""); + return ( + + + Pool editor + Configure your pool + + (row.name || row.slug).toLowerCase().includes(search.toLowerCase()), + )} + onToggle={(row) => + setIds((current) => + current.includes(row.user_service_id) + ? current.filter((id) => id !== row.user_service_id) + : [...current, row.user_service_id], + ) + } + /> + + + ); +} +async function openPicker(user: ReturnType) { + const trigger = screen.getByRole("button", { name: "Choose connections" }); + await user.click(trigger); + const input = screen.getByRole("combobox", { + name: "Search candidate services", + }); + await waitFor(() => expect(input).toHaveFocus()); + return { trigger, input }; +} +describe("pool connection picker", () => { + it("selects and deselects multiple rows without closing; search retains selection", async () => { + const user = userEvent.setup(); + render(); + const { input, trigger } = await openPicker(user); + expect( + screen.getByRole("listbox", { name: "Connections" }), + ).toHaveAttribute("aria-multiselectable", "true"); + await user.click(screen.getByRole("option", { name: candidate.name })); + await user.click(screen.getByRole("option", { name: backup.name })); + expect( + screen.getByRole("option", { name: candidate.name }), + ).toHaveAttribute("aria-selected", "true"); + expect(screen.getByRole("status")).toHaveTextContent("2 of 50 selected"); + expect(input).toHaveFocus(); + await user.type(input, "Backup"); + expect( + screen.queryByRole("option", { name: candidate.name }), + ).not.toBeInTheDocument(); + expect(screen.getByRole("option", { name: backup.name })).toHaveAttribute( + "aria-selected", + "true", + ); + await user.clear(input); + expect( + screen.getByRole("option", { name: candidate.name }), + ).toHaveAttribute("aria-selected", "true"); + await user.click(screen.getByRole("option", { name: backup.name })); + expect(screen.getByRole("option", { name: backup.name })).toHaveAttribute( + "aria-selected", + "false", + ); + await user.click(screen.getByRole("button", { name: "Done" })); + await waitFor(() => expect(trigger).toHaveFocus()); + await user.click(trigger); + expect( + screen.getByRole("option", { name: candidate.name }), + ).toHaveAttribute("aria-selected", "true"); + for (let cycle = 0; cycle < 3; cycle++) { + await user.click(screen.getByRole("option", { name: backup.name })); + await user.click(screen.getByRole("option", { name: backup.name })); + await user.keyboard("{Escape}"); + await waitFor(() => expect(trigger).toHaveFocus()); + expect(trigger).toHaveTextContent("1 connection selected"); + await user.click(trigger); + expect(screen.getByRole("option", { name: backup.name })).toHaveAttribute( + "aria-selected", + "false", + ); + } + }); + it("supports arrow/enter toggling, announces disabled reasons, and Escape closes only the picker then restores focus", async () => { + const user = userEvent.setup(); + render(); + const { trigger, input } = await openPicker(user); + await user.keyboard( + "{ArrowDown}{Enter}{ArrowDown}{Enter}{ArrowDown}{Enter}", + ); + expect(screen.getByRole("status")).toHaveTextContent("2 of 50 selected"); + const failedOption = screen.getByRole("option", { name: failed.name }); + expect(input).toHaveAttribute("aria-activedescendant", failedOption.id); + expect(failedOption).toHaveAttribute("aria-disabled", "true"); + expect(failedOption).toHaveAccessibleDescription( + /Credentials unavailable.*Reconnect/, + ); + expect(failedOption).toHaveAttribute("aria-selected", "false"); + await user.keyboard("{Escape}"); + await waitFor(() => expect(trigger).toHaveFocus()); + expect(screen.queryByRole("combobox")).not.toBeInTheDocument(); + expect(screen.getByRole("dialog", { name: "Pool editor" })).toBeVisible(); + }); + it("enforces 50 connections while allowing deselection, including an unavailable selected row", async () => { + const user = userEvent.setup(); + render( + `other-${n}`), + ]} + />, + ); + await openPicker(user); + expect(screen.getByRole("option", { name: backup.name })).toHaveAttribute( + "aria-disabled", + "true", + ); + await user.click(screen.getByRole("option", { name: backup.name })); + expect(screen.getByRole("status")).toHaveTextContent("50 of 50 selected"); + await user.click(screen.getByRole("option", { name: failed.name })); + expect(screen.getByRole("status")).toHaveTextContent("49 of 50 selected"); + expect(screen.getByRole("option", { name: backup.name })).toHaveAttribute( + "aria-disabled", + "false", + ); + expect(screen.getByRole("option", { name: failed.name })).toHaveAttribute( + "aria-disabled", + "true", + ); + }); + it("keeps retry and paging inside the dropdown and distinguishes loading, empty search, and empty inventory", async () => { + const user = userEvent.setup(); + const retry = vi.fn(); + const more = vi.fn(); + const view = render( + , + ); + await openPicker(user); + expect(screen.getByText("Loading connections…")).toBeVisible(); + expect(screen.queryByText(/No connections/)).not.toBeInTheDocument(); + view.rerender( + , + ); + expect(screen.getByRole("alert")).toHaveTextContent( + "Temporary inventory failure", + ); + await user.click(screen.getByRole("button", { name: "Retry" })); + expect(retry).toHaveBeenCalledTimes(1); + view.rerender( + , + ); + expect(screen.getByText("No connections match this search.")).toBeVisible(); + view.rerender(); + expect(screen.getByText(/No connections yet/)).toBeVisible(); + view.rerender( + , + ); + await user.click( + screen.getByRole("button", { name: "Load more connections" }), + ); + expect(more).toHaveBeenCalledTimes(1); + expect(screen.getByRole("combobox")).toBeVisible(); + }); +}); diff --git a/frontend/src/components/dashboard/pool-connection-picker.tsx b/frontend/src/components/dashboard/pool-connection-picker.tsx new file mode 100644 index 000000000..4982bba65 --- /dev/null +++ b/frontend/src/components/dashboard/pool-connection-picker.tsx @@ -0,0 +1,251 @@ +import { useEffect, useId, useRef, useState } from "react"; +import { Check, ChevronsUpDown } from "lucide-react"; +import { ErrorBanner } from "@/components/shared/error-banner"; +import { Button } from "@/components/ui/button"; +import { Input } from "@/components/ui/input"; +import { + Popover, + PopoverContent, + PopoverTrigger, +} from "@/components/ui/popover"; +import type { PoolCandidate } from "@/schemas/pools"; +import { bindingLabel, message, protocolLabel, reason } from "./pool-labels"; + +type Props = { + rows: PoolCandidate[]; + selectedIds: string[]; + search: string; + onSearch: (search: string) => void; + onToggle: (row: PoolCandidate) => void; + isLoading: boolean; + isSearching: boolean; + isError: boolean; + error: unknown; + onRetry: () => void; + hasNextPage: boolean; + isFetchingNextPage: boolean; + onLoadMore: () => void; +}; + +export function PoolConnectionPicker({ + rows, + selectedIds, + search, + onSearch, + onToggle, + isLoading, + isSearching, + isError, + error, + onRetry, + hasNextPage, + isFetchingNextPage, + onLoadMore, +}: Props) { + const [open, setOpen] = useState(false); + const [activeId, setActiveId] = useState(null); + const id = useId(); + const listId = `${id}-list`; + const input = useRef(null); + const list = useRef(null); + const activeIndex = rows.findIndex((row) => row.user_service_id === activeId); + const busy = isLoading || isSearching; + useEffect(() => { + if (activeIndex >= 0) + list.current + ?.querySelector(`[data-index="${activeIndex}"]`) + ?.scrollIntoView?.({ block: "nearest" }); + }, [activeIndex]); + function disabled(row: PoolCandidate) { + return ( + !selectedIds.includes(row.user_service_id) && + (selectedIds.length >= 50 || + (!row.eligible && row.reason !== "compatibility_declaration_required")) + ); + } + function choose(row: PoolCandidate) { + if (!disabled(row)) onToggle(row); + input.current?.focus(); + } + function navigate(direction: number) { + if (!rows.length) return; + const next = + activeIndex < 0 + ? direction > 0 + ? 0 + : rows.length - 1 + : (activeIndex + direction + rows.length) % rows.length; + setActiveId(rows[next]!.user_service_id); + } + return ( +
+ + + + + { + event.preventDefault(); + input.current?.focus(); + }} + onEscapeKeyDown={(event) => { + event.preventDefault(); + event.stopPropagation(); + setOpen(false); + }} + > + = 0 ? `${id}-option-${activeIndex}` : undefined + } + autoComplete="off" + placeholder="Search your connections" + className="shrink-0 focus-visible:border-primary focus-visible:ring-1 focus-visible:ring-primary/40" + value={search} + onChange={(event) => { + setActiveId(null); + onSearch(event.target.value); + }} + onKeyDown={(event) => { + if (event.nativeEvent.isComposing) return; + if (event.key === "ArrowDown" || event.key === "ArrowUp") { + event.preventDefault(); + navigate(event.key === "ArrowDown" ? 1 : -1); + } else if (event.key === "Enter") { + event.preventDefault(); + if (activeIndex >= 0) choose(rows[activeIndex]!); + } + }} + /> +

+ Select multiple connections. Select again to remove. +

+
+ {isError && ( +
+ +
+ )} + {busy && ( +

+ {isLoading ? "Loading connections…" : "Searching connections…"} +

+ )} +
+ {rows.map((row, index) => { + const selected = selectedIds.includes(row.user_service_id); + const unavailable = disabled(row); + return ( +
event.preventDefault()} + onClick={() => choose(row)} + > + +
+

+ {row.name || row.slug} +

+
+

+ {row.slug} · {bindingLabel(row.credential_binding)} + {row.protocol + ? ` · ${protocolLabel(row.protocol)}` + : ""} +

+ {row.reason &&

{reason(row)}

} +
+
+
+ ); + })} +
+ {!busy && !isError && rows.length === 0 && ( +

+ {search + ? "No connections match this search." + : "No connections yet. Connect a service in the Services tab first, then return here."} +

+ )} + {hasNextPage && ( + + )} +
+
+

+ {selectedIds.length} of 50 selected +

+ +
+ {selectedIds.length >= 50 && ( +

+ A pool supports up to 50 connections. Remove one to choose + another. +

+ )} + + +
+ ); +} diff --git a/frontend/src/components/dashboard/pool-connections-editor.tsx b/frontend/src/components/dashboard/pool-connections-editor.tsx index 9684bc05d..b50beb5b7 100644 --- a/frontend/src/components/dashboard/pool-connections-editor.tsx +++ b/frontend/src/components/dashboard/pool-connections-editor.tsx @@ -4,7 +4,6 @@ import { ErrorBanner } from "@/components/shared/error-banner"; import { Button } from "@/components/ui/button"; import { Checkbox } from "@/components/ui/checkbox"; import { Input } from "@/components/ui/input"; -import { Skeleton } from "@/components/ui/skeleton"; import { usePoolCandidates, usePoolHealth } from "@/hooks/use-pools"; import type { CreateServicePoolInput, @@ -13,7 +12,8 @@ import type { ServicePoolMember, } from "@/schemas/pools"; import { NumberInput, Toggle } from "./pool-controls"; -import { bindingLabel, message, protocolLabel, reason } from "./pool-labels"; +import { bindingLabel, reason } from "./pool-labels"; +import { PoolConnectionPicker } from "./pool-connection-picker"; import { PoolOperationCheck, type PoolOperation } from "./pool-operation-check"; function newMember(id: string): ServicePoolMember { @@ -100,9 +100,37 @@ export function PoolConnectionsEditor({ .map((m) => (m.user_service_id === id ? { ...m, ...patch } : m)), ); } - const available = rows.filter( - (row) => !members.some((m) => m.user_service_id === row.user_service_id), - ); + function toggleMember(row: PoolCandidate) { + const current = form.getValues("members"); + if ( + current.some((member) => member.user_service_id === row.user_service_id) + ) { + form.setValue( + "members", + current.filter( + (member) => member.user_service_id !== row.user_service_id, + ), + ); + return; + } + if ( + current.length >= 50 || + (!row.eligible && row.reason !== "compatibility_declaration_required") + ) + return; + setSelectedLabels((labels) => ({ ...labels, [row.user_service_id]: row })); + const nextPriority = + priority && current.length + ? Math.min( + 4294967295, + Math.max(...current.map((member) => member.priority ?? 0)) + 1, + ) + : 0; + form.setValue("members", [ + ...current, + { ...newMember(row.user_service_id), priority: nextPriority }, + ]); + } const orderedMembers = members .map((member, index) => ({ member, index })) .sort((a, b) => @@ -122,10 +150,36 @@ export function PoolConnectionsEditor({ : "Add the connections that should share traffic."}

+
+

Select connections

+ member.user_service_id!)} + search={search} + onSearch={setSearch} + onToggle={toggleMember} + isLoading={candidates.isLoading} + isSearching={ + search !== settledSearch || + (candidates.isFetching && !candidates.isFetchingNextPage) + } + isError={candidates.isError} + error={candidates.error} + onRetry={() => { + void candidates.refetch(); + }} + hasNextPage={candidates.hasNextPage} + isFetchingNextPage={candidates.isFetchingNextPage} + onLoadMore={() => { + void candidates.fetchNextPage(); + }} + /> +
{members.length === 0 && (
+ {!pool &&

Add at least one connection to create a pool.

} {priority - ? "Add a primary connection below, then a backup." + ? "Choose a primary connection, then a backup." : "Add the connections that should share traffic."}{" "} You can mix platform access and your own keys.
@@ -246,108 +300,6 @@ export function PoolConnectionsEditor({ ); })} -
- - {candidates.isError && ( - { - void candidates.refetch(); - }} - /> - )} - {candidates.isLoading && } - {!candidates.isLoading && - !candidates.isError && - available.length === 0 && ( -

- {search - ? "No connections match this search." - : members.length > 0 - ? "All connections on this page have been added." - : "No connections yet. Connect a service in the Services tab first, then return here."} -

- )} -
- {available.map((row) => ( -
-
-

- {row.name || row.slug} -

-

- {row.slug} · {bindingLabel(row.credential_binding)} - {row.protocol ? ` · ${protocolLabel(row.protocol)}` : ""} -

- {row.reason && ( -

- {reason(row)} -

- )} -
- -
- ))} -
- {candidates.hasNextPage && ( - - )} - {members.length >= 50 && ( -

- A pool supports up to 50 connections. -

- )} -
{!aiChat && (
diff --git a/frontend/src/components/dashboard/pool-editor.tsx b/frontend/src/components/dashboard/pool-editor.tsx index 56afea269..d9133b10d 100644 --- a/frontend/src/components/dashboard/pool-editor.tsx +++ b/frontend/src/components/dashboard/pool-editor.tsx @@ -54,7 +54,15 @@ export function PoolEditor({ const create = useCreateServicePool(); const update = useUpdateServicePool(); const form = useAppForm({ - resolver: zodResolver(createServicePoolSchema), + resolver: zodResolver( + createServicePoolSchema.refine( + (input) => Boolean(pool) || input.members.length > 0, + { + path: ["members"], + message: "Add at least one connection to create a pool.", + }, + ), + ), mode: "onChange", defaultValues: { slug: pool?.slug ?? "", diff --git a/frontend/src/components/dashboard/pool-labels.ts b/frontend/src/components/dashboard/pool-labels.ts index 60ac63b31..59baa5394 100644 --- a/frontend/src/components/dashboard/pool-labels.ts +++ b/frontend/src/components/dashboard/pool-labels.ts @@ -8,6 +8,8 @@ export const strategyLabels = { }; const reasonLabels: Record = { unavailable: "Connection unavailable", + credential_unavailable: + "Credentials unavailable. Reconnect or update this connection in Services.", inactive: "Service disabled", disabled: "Member disabled", cooldown: "Cooling down", diff --git a/frontend/src/components/dashboard/service-pools-tab.test.tsx b/frontend/src/components/dashboard/service-pools-tab.test.tsx index fd1ad23bb..15338de58 100644 --- a/frontend/src/components/dashboard/service-pools-tab.test.tsx +++ b/frontend/src/components/dashboard/service-pools-tab.test.tsx @@ -93,6 +93,119 @@ beforeEach(() => { }); describe("pool atomic editor", () => { + it("explains an unavailable credential while allowing a healthy connection to create the pool", async () => { + const user = userEvent.setup(); + mocks.candidates.mockReturnValue( + page([ + { + ...candidate, + user_service_id: "failed-id", + name: "Failed connection", + eligible: false, + reason: "credential_unavailable", + }, + candidate, + ]), + ); + render(); + fireEvent.change(screen.getByLabelText("Name"), { + target: { value: "Usable pool" }, + }); + const create = screen.getByRole("button", { name: "Create pool" }); + expect(create).toBeDisabled(); + expect( + screen.getByText("Add at least one connection to create a pool."), + ).toBeVisible(); + await user.click( + screen.getByRole("button", { name: "Choose connections" }), + ); + expect( + screen.getByText( + "Credentials unavailable. Reconnect or update this connection in Services.", + ), + ).toBeVisible(); + expect( + screen.getByRole("option", { name: "Failed connection" }), + ).toHaveAttribute("aria-disabled", "true"); + await user.click(screen.getByRole("option", { name: "Failed connection" })); + expect(create).toBeDisabled(); + await user.click(screen.getByRole("option", { name: "My connection" })); + await user.click(screen.getByRole("button", { name: "Done" })); + await waitFor(() => expect(create).toBeEnabled()); + await user.click(create); + expect(mocks.create).toHaveBeenCalledWith( + expect.objectContaining({ + members: [expect.objectContaining({ user_service_id: "member-id" })], + }), + ); + }); + it("keeps empty existing drafts editable while new pools wait for a connection", async () => { + const user = userEvent.setup(); + mocks.candidates.mockReturnValue({ isLoading: true }); + const view = render(); + fireEvent.change(screen.getByLabelText("Name"), { + target: { value: "New pool" }, + }); + expect(screen.getByRole("button", { name: "Create pool" })).toBeDisabled(); + view.unmount(); + render(); + fireEvent.change(screen.getByLabelText("Name"), { + target: { value: "Empty draft renamed" }, + }); + await waitFor(() => expect(saveButton()).toBeEnabled()); + await user.click(saveButton()); + expect(mocks.update).toHaveBeenCalledWith( + expect.objectContaining({ members: [], name: "Empty draft renamed" }), + ); + }); + it("keeps configured routing and models when searching and reopening the picker", async () => { + const user = userEvent.setup(); + const configured = { + ...pool.members[0]!, + priority: 7, + weight: 11, + model: "provider model", + same_api_compatible: true, + }; + render( + , + ); + const trigger = screen.getByRole("button", { name: "Choose connections" }); + await user.click(trigger); + await user.type( + screen.getByRole("combobox", { name: "Search candidate services" }), + "provider model", + ); + await user.keyboard("{Escape}"); + await waitFor(() => expect(trigger).toHaveFocus()); + await user.click(trigger); + expect( + screen.getByRole("combobox", { name: "Search candidate services" }), + ).toHaveValue("provider model"); + await user.click(screen.getByRole("button", { name: "Done" })); + expect(screen.getByLabelText("Priority for member 1")).toHaveValue(7); + expect(screen.getByLabelText("Weight for member 1")).toHaveValue(11); + expect(screen.getByLabelText("Model for member 1")).toHaveValue( + "provider model", + ); + expect(saveButton()).toBeDisabled(); + fireEvent.change(screen.getByLabelText("Name"), { + target: { value: "Configured pool" }, + }); + await waitFor(() => expect(saveButton()).toBeEnabled()); + await user.click(saveButton()); + expect(mocks.update).toHaveBeenCalledWith( + expect.objectContaining({ members: [configured] }), + ); + }); it("keeps displayed member numbers and edits aligned after priority reordering", async () => { const user = userEvent.setup(); const second = { @@ -271,9 +384,12 @@ describe("pool atomic editor", () => { onClose={vi.fn()} />, ); - await user.click(screen.getByRole("button", { name: "Add" })); + await user.click( + screen.getByRole("button", { name: "Choose connections" }), + ); + await user.click(screen.getByRole("option", { name: "Custom connection" })); await user.type( - screen.getByRole("textbox", { name: "Search candidate services" }), + screen.getByRole("combobox", { name: "Search candidate services" }), "hide selected", ); await waitFor(() => @@ -281,6 +397,7 @@ describe("pool atomic editor", () => { screen.getByText("No connections match this search."), ).toBeVisible(), ); + await user.click(screen.getByRole("button", { name: "Done" })); for (const n of [1, 2]) { const checkbox = screen.getByLabelText( `Confirm API compatibility for member ${n}`, @@ -316,10 +433,15 @@ describe("pool atomic editor", () => { }); const user = userEvent.setup(); render(); + await user.click( + screen.getByRole("button", { name: "Choose connections" }), + ); expect( screen.getByText("Protocol is incompatible with this pool"), ).toBeVisible(); - expect(screen.getByRole("button", { name: "Add" })).toBeDisabled(); + expect( + screen.getByRole("option", { name: "Different API" }), + ).toHaveAttribute("aria-disabled", "true"); await user.click( screen.getByRole("button", { name: "Load more connections" }), ); @@ -478,7 +600,11 @@ describe("pool management", () => { view.rerender(); await user.type(screen.getByLabelText("Name"), "Research routing"); expect(screen.getByLabelText("Pool slug")).toHaveValue("research-routing"); - await user.click(screen.getByRole("button", { name: "Add" })); + await user.click( + screen.getByRole("button", { name: "Choose connections" }), + ); + await user.click(screen.getByRole("option", { name: "My connection" })); + await user.click(screen.getByRole("button", { name: "Done" })); await user.click(screen.getByRole("button", { name: "Create pool" })); expect(mocks.create).toHaveBeenCalledWith( expect.objectContaining({ org_id: "org-id", slug: "research-routing" }), @@ -492,7 +618,7 @@ describe("pool management", () => { await waitFor(() => expect(screen.getByLabelText("Pool slug")).toHaveValue("a".repeat(79)), ); - expect(screen.getByRole("button", { name: "Create pool" })).toBeEnabled(); + expect(screen.getByRole("button", { name: "Create pool" })).toBeDisabled(); }); it("hides the owner selector without manageable organizations", () => {