diff --git a/architecture/inference-routing.md b/architecture/inference-routing.md index 851823f3b..25d8dee6e 100644 --- a/architecture/inference-routing.md +++ b/architecture/inference-routing.md @@ -66,7 +66,7 @@ The gateway implements the `Inference` gRPC service defined in `proto/inference. 1. Validates that both fields are non-empty. 2. Fetches the named provider record from the store. 3. Validates the provider by resolving its route (checking that the provider type is supported and has a usable API key). -4. Unless `skip_validation` is set, performs a lightweight provider-shaped probe against the resolved upstream endpoint (for example, a tiny chat/messages request with `max_tokens: 1`) to confirm the endpoint is reachable and accepts the expected auth/request shape. +4. By default, performs a lightweight provider-shaped probe against the resolved upstream endpoint (for example, a tiny chat/messages request with `max_tokens: 1`) to confirm the endpoint is reachable and accepts the expected auth/request shape. `--no-verify` disables this probe when the endpoint is not up yet. 5. Builds a managed route spec that stores only `provider_name` and `model_id`. The spec intentionally leaves `base_url`, `api_key`, and `protocols` empty -- these are resolved dynamically at bundle time from the provider record. 6. Upserts the route with name `inference.local`. Version starts at 1 and increments monotonically on each update. @@ -92,7 +92,7 @@ File: `proto/inference.proto` Key messages: -- `SetClusterInferenceRequest` -- `provider_name` + `model_id` + optional `skip_validation` +- `SetClusterInferenceRequest` -- `provider_name` + `model_id` + optional `verify` / `no_verify` overrides, with verification enabled by default - `SetClusterInferenceResponse` -- `provider_name` + `model_id` + `version` - `GetInferenceBundleResponse` -- `repeated ResolvedRoute routes` + `revision` + `generated_at_ms` - `ResolvedRoute` -- `name`, `base_url`, `protocols`, `api_key`, `model_id`, `provider_type` diff --git a/crates/openshell-cli/src/main.rs b/crates/openshell-cli/src/main.rs index ca82095ab..ee325dcf8 100644 --- a/crates/openshell-cli/src/main.rs +++ b/crates/openshell-cli/src/main.rs @@ -905,8 +905,12 @@ enum InferenceCommands { #[arg(long)] system: bool, + /// Verify the resolved upstream endpoint before saving the route. + #[arg(long, conflicts_with = "no_verify")] + verify: bool, + /// Skip endpoint verification before saving the route. - #[arg(long)] + #[arg(long, conflicts_with = "verify")] no_verify: bool, }, @@ -925,8 +929,12 @@ enum InferenceCommands { #[arg(long)] system: bool, + /// Verify the resolved upstream endpoint before saving the route. + #[arg(long, conflicts_with = "no_verify")] + verify: bool, + /// Skip endpoint verification before saving the route. - #[arg(long)] + #[arg(long, conflicts_with = "verify")] no_verify: bool, }, @@ -1804,15 +1812,18 @@ async fn main() -> Result<()> { provider, model, system, -<<<<<<< HEAD - no_verify: _, -======= + verify, no_verify, ->>>>>>> 7f0504d8 (feat(inference): validate endpoints before saving routes) } => { let route_name = if system { "sandbox-system" } else { "" }; run::gateway_inference_set( - endpoint, &provider, &model, route_name, no_verify, &tls, + endpoint, + &provider, + &model, + route_name, + verify, + no_verify, + &tls, ) .await?; } @@ -1820,11 +1831,8 @@ async fn main() -> Result<()> { provider, model, system, -<<<<<<< HEAD - no_verify: _, -======= + verify, no_verify, ->>>>>>> 7f0504d8 (feat(inference): validate endpoints before saving routes) } => { let route_name = if system { "sandbox-system" } else { "" }; run::gateway_inference_update( @@ -1832,6 +1840,7 @@ async fn main() -> Result<()> { provider.as_deref(), model.as_deref(), route_name, + verify, no_verify, &tls, ) @@ -2565,7 +2574,7 @@ mod tests { } #[test] - fn inference_set_accepts_no_verify_flag() { + fn inference_set_accepts_verify_flag() { let cli = Cli::try_parse_from([ "openshell", "inference", @@ -2574,40 +2583,34 @@ mod tests { "openai-dev", "--model", "gpt-4.1", - "--no-verify", + "--verify", ]) - .expect("inference set should parse --no-verify"); + .expect("inference set should parse --verify"); assert!(matches!( cli.command, Some(Commands::Inference { - command: Some(InferenceCommands::Set { - no_verify: true, - .. - }) + command: Some(InferenceCommands::Set { verify: true, .. }) }) )); } #[test] - fn inference_update_accepts_no_verify_flag() { + fn inference_update_accepts_verify_flag() { let cli = Cli::try_parse_from([ "openshell", "inference", "update", "--provider", "openai-dev", - "--no-verify", + "--verify", ]) - .expect("inference update should parse --no-verify"); + .expect("inference update should parse --verify"); assert!(matches!( cli.command, Some(Commands::Inference { - command: Some(InferenceCommands::Update { - no_verify: true, - .. - }) + command: Some(InferenceCommands::Update { verify: true, .. }) }) )); } @@ -2848,4 +2851,23 @@ mod tests { }) )); } + + #[test] + fn inference_set_rejects_verify_and_no_verify_together() { + let err = Cli::try_parse_from([ + "openshell", + "inference", + "set", + "--provider", + "openai-dev", + "--model", + "gpt-4.1", + "--verify", + "--no-verify", + ]) + .expect_err("verify and no-verify should conflict"); + + assert!(err.to_string().contains("--verify")); + assert!(err.to_string().contains("--no-verify")); + } } diff --git a/crates/openshell-cli/src/run.rs b/crates/openshell-cli/src/run.rs index 878c8c2ca..802ce3e58 100644 --- a/crates/openshell-cli/src/run.rs +++ b/crates/openshell-cli/src/run.rs @@ -3390,19 +3390,39 @@ pub async fn gateway_inference_set( provider_name: &str, model_id: &str, route_name: &str, - skip_validation: bool, + verify: bool, + no_verify: bool, tls: &TlsOptions, ) -> Result<()> { + let progress = if std::io::stdout().is_terminal() { + let spinner = ProgressBar::new_spinner(); + spinner.set_style( + ProgressStyle::with_template("{spinner:.cyan} {msg} ({elapsed})") + .unwrap_or_else(|_| ProgressStyle::default_spinner()), + ); + spinner.set_message("Configuring inference..."); + spinner.enable_steady_tick(Duration::from_millis(120)); + Some(spinner) + } else { + None + }; + let mut client = grpc_inference_client(server, tls).await?; let response = client .set_cluster_inference(SetClusterInferenceRequest { provider_name: provider_name.to_string(), model_id: model_id.to_string(), route_name: route_name.to_string(), - skip_validation, + verify, + no_verify, }) - .await - .into_diagnostic()?; + .await; + + if let Some(progress) = &progress { + progress.finish_and_clear(); + } + + let response = response.into_diagnostic()?; let configured = response.into_inner(); let label = if configured.route_name == "sandbox-system" { @@ -3416,6 +3436,12 @@ pub async fn gateway_inference_set( println!(" {} {}", "Provider:".dimmed(), configured.provider_name); println!(" {} {}", "Model:".dimmed(), configured.model_id); println!(" {} {}", "Version:".dimmed(), configured.version); + if configured.validation_performed { + println!(" {}", "Validated Endpoints:".dimmed()); + for endpoint in configured.validated_endpoints { + println!(" - {} ({})", endpoint.url, endpoint.protocol); + } + } Ok(()) } @@ -3424,7 +3450,8 @@ pub async fn gateway_inference_update( provider_name: Option<&str>, model_id: Option<&str>, route_name: &str, - skip_validation: bool, + verify: bool, + no_verify: bool, tls: &TlsOptions, ) -> Result<()> { if provider_name.is_none() && model_id.is_none() { @@ -3447,15 +3474,34 @@ pub async fn gateway_inference_update( let provider = provider_name.unwrap_or(¤t.provider_name); let model = model_id.unwrap_or(¤t.model_id); + let progress = if std::io::stdout().is_terminal() { + let spinner = ProgressBar::new_spinner(); + spinner.set_style( + ProgressStyle::with_template("{spinner:.cyan} {msg} ({elapsed})") + .unwrap_or_else(|_| ProgressStyle::default_spinner()), + ); + spinner.set_message("Configuring inference..."); + spinner.enable_steady_tick(Duration::from_millis(120)); + Some(spinner) + } else { + None + }; + let response = client .set_cluster_inference(SetClusterInferenceRequest { provider_name: provider.to_string(), model_id: model.to_string(), route_name: route_name.to_string(), - skip_validation, + verify, + no_verify, }) - .await - .into_diagnostic()?; + .await; + + if let Some(progress) = &progress { + progress.finish_and_clear(); + } + + let response = response.into_diagnostic()?; let configured = response.into_inner(); let label = if configured.route_name == "sandbox-system" { @@ -3469,6 +3515,12 @@ pub async fn gateway_inference_update( println!(" {} {}", "Provider:".dimmed(), configured.provider_name); println!(" {} {}", "Model:".dimmed(), configured.model_id); println!(" {} {}", "Version:".dimmed(), configured.version); + if configured.validation_performed { + println!(" {}", "Validated Endpoints:".dimmed()); + for endpoint in configured.validated_endpoints { + println!(" - {} ({})", endpoint.url, endpoint.protocol); + } + } Ok(()) } diff --git a/crates/openshell-server/src/inference.rs b/crates/openshell-server/src/inference.rs index 35b704f17..895766e91 100644 --- a/crates/openshell-server/src/inference.rs +++ b/crates/openshell-server/src/inference.rs @@ -5,7 +5,8 @@ use openshell_core::inference::AuthHeader; use openshell_core::proto::{ ClusterInferenceConfig, GetClusterInferenceRequest, GetClusterInferenceResponse, GetInferenceBundleRequest, GetInferenceBundleResponse, InferenceRoute, Provider, ResolvedRoute, - SetClusterInferenceRequest, SetClusterInferenceResponse, inference_server::Inference, + SetClusterInferenceRequest, SetClusterInferenceResponse, ValidatedEndpoint, + inference_server::Inference, }; use std::sync::Arc; use std::time::Duration; @@ -78,16 +79,18 @@ impl Inference for InferenceService { ) -> Result, Status> { let req = request.into_inner(); let route_name = effective_route_name(&req.route_name)?; + let verify = req.verify || !req.no_verify; let route = upsert_cluster_inference_route( self.state.store.as_ref(), route_name, &req.provider_name, &req.model_id, - req.skip_validation, + verify, ) .await?; let config = route + .route .config .as_ref() .ok_or_else(|| Status::internal("managed route missing config"))?; @@ -95,8 +98,10 @@ impl Inference for InferenceService { Ok(Response::new(SetClusterInferenceResponse { provider_name: config.provider_name.clone(), model_id: config.model_id.clone(), - version: route.version, + version: route.route.version, route_name: route_name.to_string(), + validation_performed: !route.validation.is_empty(), + validated_endpoints: route.validation, })) } @@ -143,8 +148,8 @@ async fn upsert_cluster_inference_route( route_name: &str, provider_name: &str, model_id: &str, - skip_validation: bool, -) -> Result { + verify: bool, +) -> Result { if provider_name.trim().is_empty() { return Err(Status::invalid_argument("provider_name is required")); } @@ -161,9 +166,11 @@ async fn upsert_cluster_inference_route( })?; let resolved = resolve_provider_route(&provider)?; - if !skip_validation { - verify_provider_endpoint(&provider.name, model_id, &resolved).await?; - } + let validation = if verify { + vec![verify_provider_endpoint(&provider.name, model_id, &resolved).await?] + } else { + Vec::new() + }; let config = build_cluster_inference_config(&provider, model_id); @@ -193,7 +200,7 @@ async fn upsert_cluster_inference_route( .await .map_err(|e| Status::internal(format!("persist route failed: {e}")))?; - Ok(route) + Ok(UpsertedInferenceRoute { route, validation }) } fn build_cluster_inference_config(provider: &Provider, model_id: &str) -> ClusterInferenceConfig { @@ -217,6 +224,12 @@ struct ValidationProbe { body: serde_json::Value, } +#[derive(Debug)] +struct UpsertedInferenceRoute { + route: InferenceRoute, + validation: Vec, +} + fn resolve_provider_route(provider: &Provider) -> Result { let provider_type = provider.r#type.trim().to_ascii_lowercase(); @@ -332,6 +345,45 @@ fn validation_probe( ))) } +fn validated_protocol(route: &ResolvedProviderRoute) -> Result { + if route + .protocols + .iter() + .any(|protocol| protocol == "openai_chat_completions") + { + return Ok("openai_chat_completions".to_string()); + } + + if route + .protocols + .iter() + .any(|protocol| protocol == "anthropic_messages") + { + return Ok("anthropic_messages".to_string()); + } + + if route + .protocols + .iter() + .any(|protocol| protocol == "openai_responses") + { + return Ok("openai_responses".to_string()); + } + + if route + .protocols + .iter() + .any(|protocol| protocol == "openai_completions") + { + return Ok("openai_completions".to_string()); + } + + Err(Status::failed_precondition(format!( + "provider type '{}' does not expose a writable inference protocol for validation", + route.provider_type + ))) +} + fn validation_url(base_url: &str, path: &str) -> String { let base = base_url.trim_end_matches('/'); if base.ends_with("/v1") && (path == "/v1" || path.starts_with("/v1/")) { @@ -357,8 +409,9 @@ async fn verify_provider_endpoint( provider_name: &str, model_id: &str, route: &ResolvedProviderRoute, -) -> Result<(), Status> { +) -> Result { let probe = validation_probe(route, model_id)?; + let protocol = validated_protocol(route)?; let url = validation_url(&route.base_url, probe.path); let client = reqwest::Client::builder() .timeout(Duration::from_secs(5)) @@ -396,7 +449,7 @@ async fn verify_provider_endpoint( })?; if response.status().is_success() { - return Ok(()); + return Ok(ValidatedEndpoint { url, protocol }); } let status = response.status(); @@ -626,26 +679,26 @@ mod tests { CLUSTER_INFERENCE_ROUTE_NAME, "openai-dev", "gpt-4o", - true, + false, ) .await .expect("first set should succeed"); - assert_eq!(first.name, CLUSTER_INFERENCE_ROUTE_NAME); - assert_eq!(first.version, 1); + assert_eq!(first.route.name, CLUSTER_INFERENCE_ROUTE_NAME); + assert_eq!(first.route.version, 1); let second = upsert_cluster_inference_route( &store, CLUSTER_INFERENCE_ROUTE_NAME, "openai-dev", "gpt-4.1", - true, + false, ) .await .expect("second set should succeed"); - assert_eq!(second.version, 2); - assert_eq!(second.id, first.id); + assert_eq!(second.route.version, 2); + assert_eq!(second.route.id, first.route.id); - let config = second.config.as_ref().expect("config"); + let config = second.route.config.as_ref().expect("config"); assert_eq!(config.provider_name, "openai-dev"); assert_eq!(config.model_id, "gpt-4.1"); } @@ -849,14 +902,14 @@ mod tests { SANDBOX_SYSTEM_ROUTE_NAME, "anthropic-dev", "claude-sonnet-4-20250514", - true, + false, ) .await .expect("should succeed"); - assert_eq!(route.name, SANDBOX_SYSTEM_ROUTE_NAME); - assert_eq!(route.version, 1); - let config = route.config.as_ref().expect("config"); + assert_eq!(route.route.name, SANDBOX_SYSTEM_ROUTE_NAME); + assert_eq!(route.route.version, 1); + let config = route.route.config.as_ref().expect("config"); assert_eq!(config.provider_name, "anthropic-dev"); assert_eq!(config.model_id, "claude-sonnet-4-20250514"); } @@ -935,7 +988,7 @@ mod tests { SANDBOX_SYSTEM_ROUTE_NAME, "openai-dev", "gpt-4o-mini", - true, + false, ) .await .expect("upsert should succeed"); @@ -989,7 +1042,7 @@ mod tests { } #[tokio::test] - async fn upsert_cluster_route_verifies_endpoint_by_default() { + async fn upsert_cluster_route_verifies_endpoint_when_requested() { let store = Store::connect("sqlite::memory:?cache=shared") .await .expect("store"); @@ -1029,12 +1082,14 @@ mod tests { CLUSTER_INFERENCE_ROUTE_NAME, "openai-dev", "gpt-4o-mini", - false, + true, ) .await .expect("validation should succeed"); - assert_eq!(route.version, 1); + assert_eq!(route.route.version, 1); + assert_eq!(route.validation.len(), 1); + assert_eq!(route.validation[0].protocol, "openai_chat_completions"); } #[tokio::test] @@ -1068,7 +1123,7 @@ mod tests { CLUSTER_INFERENCE_ROUTE_NAME, "openai-dev", "gpt-4o-mini", - false, + true, ) .await .expect_err("validation should fail"); @@ -1090,7 +1145,7 @@ mod tests { } #[tokio::test] - async fn upsert_cluster_route_skips_validation_when_requested() { + async fn upsert_cluster_route_skips_validation_by_default() { let store = Store::connect("sqlite::memory:?cache=shared") .await .expect("store"); @@ -1112,12 +1167,13 @@ mod tests { CLUSTER_INFERENCE_ROUTE_NAME, "openai-dev", "gpt-4o-mini", - true, + false, ) .await - .expect("skip validation should persist route"); + .expect("non-verified route should persist"); - assert_eq!(route.version, 1); + assert_eq!(route.route.version, 1); + assert!(route.validation.is_empty()); } #[test] diff --git a/proto/inference.proto b/proto/inference.proto index 828927ecf..782fa0bbf 100644 --- a/proto/inference.proto +++ b/proto/inference.proto @@ -56,8 +56,13 @@ message SetClusterInferenceRequest { // Route name to target. Empty string defaults to "inference.local" (user-facing). // Use "sandbox-system" for the sandbox system-level inference route. string route_name = 3; - // Skip synchronous endpoint validation before persistence. - bool skip_validation = 4; + // Verify the resolved upstream endpoint synchronously before persistence. + bool verify = 4; +} + +message ValidatedEndpoint { + string url = 1; + string protocol = 2; } message SetClusterInferenceResponse { @@ -66,6 +71,10 @@ message SetClusterInferenceResponse { uint64 version = 3; // Route name that was configured. string route_name = 4; + // Whether endpoint verification ran as part of this request. + bool validation_performed = 5; + // The concrete endpoints that were probed during validation, when available. + repeated ValidatedEndpoint validated_endpoints = 6; } message GetClusterInferenceRequest { diff --git a/python/openshell/sandbox.py b/python/openshell/sandbox.py index 3399c2032..f10f450c3 100644 --- a/python/openshell/sandbox.py +++ b/python/openshell/sandbox.py @@ -398,13 +398,18 @@ class InferenceRouteClient: *, provider_name: str, model_id: str, + verify: bool = False, no_verify: bool = False, ) -> ClusterInferenceConfig: + if verify and no_verify: + raise ValueError("verify and no_verify are mutually exclusive") + response = self._stub.SetClusterInference( inference_pb2.SetClusterInferenceRequest( provider_name=provider_name, model_id=model_id, - skip_validation=no_verify, + verify=verify, + no_verify=no_verify, ), timeout=self._timeout, ) diff --git a/python/openshell/sandbox_test.py b/python/openshell/sandbox_test.py index 441e69f49..d7052c928 100644 --- a/python/openshell/sandbox_test.py +++ b/python/openshell/sandbox_test.py @@ -6,6 +6,8 @@ from __future__ import annotations import json from typing import TYPE_CHECKING, Any, cast +import pytest + from openshell._proto import openshell_pb2 from openshell.sandbox import ( _PYTHON_CLOUDPICKLE_BOOTSTRAP, @@ -139,7 +141,7 @@ def test_from_active_cluster_prefers_openshell_gateway_env( client.close() -def test_inference_set_cluster_forwards_no_verify_flag() -> None: +def test_inference_set_cluster_forwards_verify_flag() -> None: stub = _FakeInferenceStub() client = cast("InferenceRouteClient", object.__new__(InferenceRouteClient)) client._timeout = 30.0 @@ -148,9 +150,40 @@ def test_inference_set_cluster_forwards_no_verify_flag() -> None: result = client.set_cluster( provider_name="openai-dev", model_id="gpt-4.1", - no_verify=True, + verify=True, ) assert result.provider_name == "openai-dev" assert stub.request is not None - assert stub.request.skip_validation is True + assert stub.request.verify is True + + +def test_inference_set_cluster_forwards_no_verify_flag() -> None: + stub = _FakeInferenceStub() + client = cast("InferenceRouteClient", object.__new__(InferenceRouteClient)) + client._timeout = 30.0 + client._stub = cast("Any", stub) + + client.set_cluster( + provider_name="openai-dev", + model_id="gpt-4.1", + no_verify=True, + ) + + assert stub.request is not None + assert stub.request.no_verify is True + + +def test_inference_set_cluster_rejects_conflicting_flags() -> None: + stub = _FakeInferenceStub() + client = cast("InferenceRouteClient", object.__new__(InferenceRouteClient)) + client._timeout = 30.0 + client._stub = cast("Any", stub) + + with pytest.raises(ValueError, match="mutually exclusive"): + client.set_cluster( + provider_name="openai-dev", + model_id="gpt-4.1", + verify=True, + no_verify=True, + )