diff --git a/crates/biorouter-server/src/routes/agent.rs b/crates/biorouter-server/src/routes/agent.rs index 8dc1604f8..e9f761dc9 100644 --- a/crates/biorouter-server/src/routes/agent.rs +++ b/crates/biorouter-server/src/routes/agent.rs @@ -3052,20 +3052,24 @@ mod new_session_provider_binding_tests { let working_dir = "/tmp/biorouter-new-chat-private-provider"; let mut overrides = provider_overrides( "versa_azure", - biorouter::providers::versa_azure::VERSA_AZURE_DEPLOYMENT, + biorouter::providers::versa_azure::VERSA_AZURE_DEFAULT_MODEL, None, ); overrides.insert("VERSA_AZURE_API_KEY".into(), "test-api-key".into()); overrides.insert( - "AZURE_OPENAI_ENDPOINT".into(), + "VERSA_AZURE_ENDPOINT".into(), biorouter::providers::versa_azure::VERSA_AZURE_ENDPOINT.into(), ); overrides.insert( - "AZURE_OPENAI_DEPLOYMENT_NAME".into(), - biorouter::providers::versa_azure::VERSA_AZURE_DEPLOYMENT.into(), + "VERSA_AZURE_DEPLOYMENT_NAME".into(), + biorouter::providers::versa_azure::deployment_for_model( + biorouter::providers::versa_azure::VERSA_AZURE_DEFAULT_MODEL, + ) + .expect("the default model has a deployment") + .into(), ); overrides.insert( - "AZURE_OPENAI_API_VERSION".into(), + "VERSA_AZURE_API_VERSION".into(), biorouter::providers::versa_azure::VERSA_AZURE_API_VERSION.into(), ); let request = || StartAgentRequest { diff --git a/crates/biorouter/src/agents/subagent_tool.rs b/crates/biorouter/src/agents/subagent_tool.rs index 6e43afcbe..793af091c 100644 --- a/crates/biorouter/src/agents/subagent_tool.rs +++ b/crates/biorouter/src/agents/subagent_tool.rs @@ -6045,7 +6045,7 @@ mod tests { "not-a-real-key".to_string(), ), ( - "AZURE_OPENAI_ENDPOINT".to_string(), + "VERSA_AZURE_ENDPOINT".to_string(), crate::providers::versa_azure::VERSA_AZURE_ENDPOINT.to_string(), ), ]) diff --git a/crates/biorouter/src/privacy/config_keys.rs b/crates/biorouter/src/privacy/config_keys.rs index 814599443..7d5fbe719 100644 --- a/crates/biorouter/src/privacy/config_keys.rs +++ b/crates/biorouter/src/privacy/config_keys.rs @@ -54,36 +54,50 @@ pub const NOT_CAPABILITY_CONFIG_KEYS: &[(&str, &str)] = &[ ("LLAMACPP_TIMEOUT", "transport timeout"), ("LLAMACPP_STARTUP_TIMEOUT", "sidecar readiness deadline"), ("LLAMACPP_CONTEXT_SIZE", "token budget"), - // ⚠ The four endpoint keys below MOVE where a Private-badged provider sends - // traffic, but they cannot RAISE a tier: Task 5 name-keys versa_azure and - // versa_bedrock Private regardless of endpoint, and azure.rs ships the - // UCSF gateway as a PUBLIC provider's default for the same reason. - // Pointing a private-badged provider off-site is a real and different - // problem — it belongs to Task 5's tier definition and to Open question 5, - // not to DR-16 — and it is recorded here rather than left unstated. - ( - "AZURE_OPENAI_ENDPOINT", - "moves a Private provider's endpoint; does not raise a tier (see Task 5)", - ), - ("AZURE_OPENAI_DEPLOYMENT_NAME", "deployment selection"), - ("AZURE_OPENAI_API_VERSION", "wire version"), - // Versa's own namespace for the same three overrides. They exist because - // onboarding used to write the `AZURE_OPENAI_*` keys above on Versa's - // behalf, which made the PUBLIC `azure_openai` card report itself - // Configured whenever a user connected UCSF's PRIVATE Versa. Same meaning, - // same classification as their legacy twins: they move a Private - // provider's endpoint, they do not raise a tier. + // ⚠ The two endpoint keys below MOVE where a Private-badged provider sends + // traffic, and since `e2e4eb9d` that moves its tier as well: `tier()` + // follows the endpoint an instance resolved (`ucsf_gateway_tier`), so an + // off-site value demotes it to Public, and deleting that value restores + // Private. These rows used to say the keys "cannot RAISE a tier" because + // Task 5 name-keyed versa_* Private regardless of endpoint, and that + // stopped being true. The classification rests on this instead: the only + // value that reads Private is the UCSF gateway's own host, so no write can + // make an off-site endpoint look Private, and a raise through one of these + // keys is always a return to the institution's gateway. Whether even that + // raise should be a user act, as it is for `OLLAMA_HOST`, is an open DR-16 + // question, recorded here rather than left unstated. + // + // Versa Azure's three overrides, in its own namespace. It used to share the + // public `azure_openai` card's `AZURE_OPENAI_*` keys, which went wrong both + // ways: onboarding WROTE them on Versa's behalf, so connecting UCSF's + // PRIVATE Versa made that PUBLIC card report itself Configured (hence this + // namespace, 2026-09-03); and Versa went on READING them as a fallback, so + // whatever that card was set up with — a company resource's endpoint, + // deployment and API version — steered every Versa request (read removed + // 2026-09-11). No tier-input file reads the `AZURE_OPENAI_*` keys now, so + // they have no rows here; `azure.rs` still reads them and is not a + // tier-input file, because `azure_openai` is Public wherever it points. ( "VERSA_AZURE_ENDPOINT", - "moves a Private provider's endpoint; does not raise a tier (see Task 5)", + "moves a Private provider's endpoint; only the UCSF gateway reads Private (see above)", ), ("VERSA_AZURE_DEPLOYMENT_NAME", "deployment selection"), ("VERSA_AZURE_API_VERSION", "wire version"), + // Versa Bedrock's two overrides, in its own namespace since 2026-09-11. It + // used to declare and read the public Amazon Bedrock card's `AWS_REGION` and + // an `AWS_ENDPOINT_URL_BEDROCK` key, then fall back to the process + // environment, so the public side's values steered Versa and a Versa setup + // configured the public card. No tier-input file reads an `AWS_*` key now, + // so none has a row; `bedrock.rs` still reads them and is not a tier-input + // file, because `aws_bedrock` is Public wherever it points. + ( + "VERSA_BEDROCK_ENDPOINT", + "moves a Private provider's endpoint; only the UCSF gateway reads Private (see above)", + ), ( - "AWS_ENDPOINT_URL_BEDROCK", - "moves a Private provider's endpoint; does not raise a tier (see Task 5)", + "VERSA_BEDROCK_REGION", + "SigV4 signing region; the endpoint, not the region, decides where a request goes", ), - ("AWS_REGION", "region selection"), ("BEDROCK_MAX_RETRIES", "retry policy"), ("BEDROCK_INITIAL_RETRY_INTERVAL_MS", "retry policy"), ("BEDROCK_BACKOFF_MULTIPLIER", "retry policy"), @@ -183,10 +197,10 @@ mod tests { // of the two lists. Adding a config read to any of them fails this test // until someone decides whether it determines capability. That is the // checkable list: it does not depend on anyone remembering a rule. - let scanned = scan_get_param_keys(); // 26 today + let scanned = scan_get_param_keys(); // 23 today assert_eq!( scanned.len(), - 26, + 23, "the tier-input files' config surface changed: {scanned:?}" ); for key in &scanned { @@ -205,6 +219,21 @@ mod tests { // survives. assert!(CAPABILITY_CONFIG_KEYS.contains(&"BIOROUTER_PROVIDER")); assert_eq!(CAPABILITY_CONFIG_KEYS.len(), 5); + + // …and the other way round: every classified key is still READ by a + // tier-input file. Without this, a read that goes away leaves its row + // behind — the count above moves, someone edits the number, and the + // lists quietly start classifying keys nothing reads. + let classified = CAPABILITY_CONFIG_KEYS + .iter() + .copied() + .chain(NOT_CAPABILITY_CONFIG_KEYS.iter().map(|(key, _why)| *key)); + for key in classified.filter(|key| *key != "BIOROUTER_PROVIDER") { + assert!( + scanned.iter().any(|read| read == key), + "{key} is classified but no tier-input file reads it; delete its row" + ); + } } #[test] diff --git a/crates/biorouter/src/providers/bedrock.rs b/crates/biorouter/src/providers/bedrock.rs index 3bfbed5d8..d913ffbdc 100644 --- a/crates/biorouter/src/providers/bedrock.rs +++ b/crates/biorouter/src/providers/bedrock.rs @@ -83,15 +83,23 @@ impl BedrockProvider { set_aws_env_vars(config.all_values()); set_aws_env_vars(config.all_secrets()); - // Normalize AWS_ENDPOINT_URL_BEDROCK → AWS_ENDPOINT_URL_BEDROCK_RUNTIME. - // The AWS SDK for Rust reads the service-specific key AWS_ENDPOINT_URL_BEDROCK_RUNTIME, - // but users (and older configs) often set the shorter AWS_ENDPOINT_URL_BEDROCK. - // Accept either: if only the short form is set, promote it to the correct key. - if std::env::var("AWS_ENDPOINT_URL_BEDROCK_RUNTIME").is_err() { - if let Ok(url) = std::env::var("AWS_ENDPOINT_URL_BEDROCK") { - std::env::set_var("AWS_ENDPOINT_URL_BEDROCK_RUNTIME", url); - } - } + // ⚠ `AWS_ENDPOINT_URL_BEDROCK` is not an endpoint for this provider. The + // AWS SDK aims Bedrock Runtime at `AWS_ENDPOINT_URL_BEDROCK_RUNTIME`, + // from the environment or from `config.yaml` through the export above, + // or at an AWS profile's `services` section; that is how a VPC endpoint + // or a proxy is meant to be set. `AWS_ENDPOINT_URL_BEDROCK` is the name + // the SDK derives for a different service, the Bedrock control plane. + // + // This used to promote it to `AWS_ENDPOINT_URL_BEDROCK_RUNTIME`, added + // on 2026-04-12 for configs and setup scripts that used the short name, + // a month before Versa Bedrock existed. But every Biorouter surface that + // has written that key wrote it for Versa Bedrock: its onboarding card on + // every connect, its Settings form on every save, `biorouter configure` + // when asked to. So once Versa was set up, the promotion made UCSF's + // gateway THIS provider's endpoint, and the user's own AWS-signed + // requests went there and were refused. Versa reads its own + // `VERSA_BEDROCK_ENDPOINT` now, but installs keep the old key, so it has + // to be ignored here, not merely left unwritten (2026-09-11). // Use load_defaults() which supports AWS SSO, profiles, and environment variables let mut loader = aws_config::defaults(aws_config::BehaviorVersion::latest()); @@ -137,6 +145,19 @@ impl BedrockProvider { }) } + /// The same client with only its HTTP transport replaced. Everything + /// `from_env` resolved — endpoint, region, credentials — is kept, so a + /// request captured through it is the request production would have sent. + #[cfg(test)] + pub(crate) fn with_http_client( + mut self, + http_client: impl aws_sdk_bedrockruntime::config::HttpClient + 'static, + ) -> Self { + let config = self.client.config().to_builder().http_client(http_client); + self.client = Client::from_conf(config.build()); + self + } + fn load_retry_config(config: &crate::config::Config) -> RetryConfig { let max_retries = config .get_param::("BEDROCK_MAX_RETRIES") diff --git a/crates/biorouter/src/providers/bedrock_namespace_tests.rs b/crates/biorouter/src/providers/bedrock_namespace_tests.rs new file mode 100644 index 000000000..a26ac8b0e --- /dev/null +++ b/crates/biorouter/src/providers/bedrock_namespace_tests.rs @@ -0,0 +1,544 @@ +//! The public `aws_bedrock` provider and UCSF's private `versa_bedrock` must not +//! steer each other. The Bedrock twin of `versa_azure`'s `routing_tests`. +//! +//! Declared in `providers/mod.rs` as +//! `#[cfg(all(test, feature = "aws-providers"))] mod bedrock_namespace_tests;`. +//! `aws-providers` is a default feature, so a plain `cargo test -p biorouter +//! --lib` runs every row here. A `--no-default-features` build compiles neither +//! provider, and this module goes with them. +//! +//! **What they shared.** Versa declared the public card's `AWS_REGION` and an +//! `AWS_ENDPOINT_URL_BEDROCK` key as its own, read both (and then the process +//! environment) as overrides, and its setup surfaces wrote both. `bedrock.rs` +//! exports every `AWS_*` config value and secret into the process environment +//! and promoted `AWS_ENDPOINT_URL_BEDROCK` to the variable the AWS SDK reads, so +//! the UCSF gateway Versa persisted became the public provider's endpoint. And +//! the SDK reads `AWS_BEARER_TOKEN_BEDROCK` from the environment on its own, and +//! authenticates with it instead of signing whenever it is there. +//! +//! **How each row measures.** Every provider is built the way production builds +//! it, through `from_env`, and only its HTTP transport is then swapped for the +//! SDK's own capture client (`with_http_client`). So every assertion is on the +//! request production would have sent: its host, its path, its `Authorization` +//! header. Nothing leaves the process, even on the rows whose bug aims a request +//! at public AWS. The stand-in is the capture client rather than wiremock +//! because the thing under test is the host the SDK resolved, and aiming the +//! endpoint at a local server would overwrite exactly that. +//! +//! The config rows pin their inputs with `with_config_overrides`, which +//! `get_param` consults before the environment and the file. The environment +//! rows cannot: the SDK reads the environment through its own shim, and the +//! public provider's `std::env::set_var` is part of what is under test. Calling +//! it in this multi-threaded binary is unsound, and its writes would leak into +//! every test running beside it. So those rows re-execute this test binary and +//! run in a child process that STARTS with the environment the scenario +//! describes and a config root of its own, as +//! `workflow::local_workflows::tests::listing_workflows_survives_a_deleted_working_directory` +//! does for a deleted working directory. + +use super::base::Provider; +use super::bedrock::{BedrockProvider, BEDROCK_DEFAULT_MODEL}; +use super::versa_bedrock::{ + VersaBedrockProvider, VERSA_BEDROCK_DEFAULT_ENDPOINT, VERSA_BEDROCK_DEFAULT_MODEL, + VERSA_BEDROCK_DEFAULT_REGION, +}; +use crate::conversation::message::Message; +use crate::model::ModelConfig; +use crate::privacy::ProviderTier; +use aws_smithy_http_client::test_util::capture_request; +use std::collections::HashMap; + +const VERSA_ACCESS_KEY: &str = "VERSATESTACCESSKEY"; +const VERSA_SECRET_KEY: &str = "versa-test-secret-key"; +const PUBLIC_ACCESS_KEY: &str = "PUBLICTESTACCESSKEY"; +const PUBLIC_SECRET_KEY: &str = "public-test-secret-key"; +/// The public card's Bedrock API key, in the variable the AWS SDK reads it from. +const PUBLIC_BEARER_TOKEN: &str = "public-bedrock-api-key"; +/// What the public card could point at: its user's own AWS region. +const PUBLIC_ENDPOINT: &str = "https://bedrock-runtime.eu-central-1.amazonaws.com"; +const PUBLIC_REGION: &str = "eu-central-1"; +const UCSF_GATEWAY_HOST: &str = "unified-api.ucsf.edu"; + +/// One request, as it would have left the machine. +#[derive(Debug, serde::Serialize, serde::Deserialize)] +struct Sent { + host: String, + path: String, + authorization: String, +} + +impl Sent { + fn of(request: &aws_smithy_runtime_api::client::orchestrator::HttpRequest) -> Self { + let url = url::Url::parse(request.uri()).expect("the SDK sends an absolute URI"); + Self { + host: url.host_str().unwrap_or_default().to_string(), + path: url.path().to_string(), + authorization: request + .headers() + .get("authorization") + .unwrap_or_default() + .to_string(), + } + } + + /// `(access key id, signing region)`, if the request was SigV4-signed. + fn signed_by(&self) -> Option<(&str, &str)> { + let credential = self + .authorization + .strip_prefix("AWS4-HMAC-SHA256 Credential=")?; + let mut scope = credential.split(',').next()?.split('/'); + let key = scope.next()?; + let _date = scope.next()?; + let region = scope.next()?; + Some((key, region)) + } +} + +/// What a row observed: for Versa, the endpoint, region and tier the instance +/// resolved; for either provider, the request it sent. +#[derive(Debug, serde::Serialize, serde::Deserialize)] +struct Observed { + resolved: Option<(String, String, String)>, + sent: Sent, +} + +/// Send one turn and return the request it made. The stand-in answers 200 with +/// an empty body, which does not parse, so the turn fails AFTER the request is +/// made — and the request is what is measured. Every row builds with +/// `BEDROCK_MAX_RETRIES=0`, because the stand-in answers once. +async fn turn(provider: &dyn Provider) { + let _ = provider + .complete("system", &[Message::user().with_text("hello")], &[]) + .await; +} + +async fn versa_sent(provider: VersaBedrockProvider) -> Sent { + let (http, captured) = capture_request(None); + let provider = provider.with_http_client(http); + turn(&provider).await; + Sent::of(&captured.expect_request()) +} + +async fn public_sent(provider: BedrockProvider) -> Sent { + let (http, captured) = capture_request(None); + let provider = provider.with_http_client(http); + turn(&provider).await; + Sent::of(&captured.expect_request()) +} + +/// Versa's credentials and Versa's own two overrides as given, blank meaning +/// absent, so the machine running the suite cannot leak its own configuration +/// into what is measured. +fn versa_config(endpoint: &str, region: &str) -> HashMap { + HashMap::from([ + ( + "VERSA_BEDROCK_ACCESS_KEY_ID".into(), + VERSA_ACCESS_KEY.into(), + ), + ( + "VERSA_BEDROCK_SECRET_ACCESS_KEY".into(), + VERSA_SECRET_KEY.into(), + ), + ("VERSA_BEDROCK_ENDPOINT".into(), endpoint.into()), + ("VERSA_BEDROCK_REGION".into(), region.into()), + ("BEDROCK_MAX_RETRIES".into(), "0".into()), + ]) +} + +async fn versa_bound(overrides: HashMap) -> VersaBedrockProvider { + crate::config::with_config_overrides( + overrides, + VersaBedrockProvider::from_env(ModelConfig::new_or_fail(VERSA_BEDROCK_DEFAULT_MODEL)), + ) + .await + .unwrap_or_else(|e| panic!("Versa Bedrock must construct from its credentials alone: {e}")) +} + +/// The endpoint and region a bound instance will be restored with, and its tier. +fn resolved(provider: &VersaBedrockProvider) -> (String, String, String) { + let binding = serde_json::to_value(provider.restore_binding()).unwrap(); + ( + binding["endpoint"].as_str().unwrap_or_default().to_string(), + binding["region"].as_str().unwrap_or_default().to_string(), + format!("{:?}", provider.tier()), + ) +} + +fn shipped() -> (String, String, String) { + ( + VERSA_BEDROCK_DEFAULT_ENDPOINT.to_string(), + VERSA_BEDROCK_DEFAULT_REGION.to_string(), + format!("{:?}", ProviderTier::Private), + ) +} + +// ------------------------------------------------------------------ the rule + +/// Configuring UCSF's PRIVATE Versa Bedrock must not configure the PUBLIC +/// Amazon Bedrock card, or hand it a value. +/// +/// `aws_bedrock` declares two keys, both required and both defaulted, and for +/// such a provider `check_provider_configured` says Configured as soon as EITHER +/// is in `config.yaml`. Versa declared one of them, `AWS_REGION`, and its setup +/// surfaces persisted it: the Settings form seeds a declared key's default and +/// `DefaultSubmitHandler` submits it, and the onboarding card wrote it on every +/// connect. Every other `AWS_*` key belongs to the public side as well, declared +/// or not: `bedrock.rs` and `sagemaker_tgi.rs` export each one into the process +/// environment, where the AWS SDK reads them. So this asserts the namespace, not +/// the one key that happened to leak. +#[test] +fn versa_declares_no_key_the_public_bedrock_provider_reads() { + let versa = VersaBedrockProvider::metadata(); + let public = BedrockProvider::metadata(); + let public_keys: Vec<&str> = public + .config_keys + .iter() + .map(|key| key.name.as_str()) + .collect(); + assert!( + public.config_keys.iter().any(|key| key.required), + "if the public provider stops having a key its configured-check turns on, \ + this test is vacuous; re-derive it rather than deleting it" + ); + + let versa_keys = versa.config_keys.iter().map(|key| key.name.as_str()); + let shared: Vec<&str> = versa_keys + .clone() + .filter(|name| public_keys.contains(name)) + .collect(); + let outside: Vec<&str> = versa_keys + .filter(|name| !name.starts_with("VERSA_BEDROCK_")) + .collect(); + assert!( + shared.is_empty() && outside.is_empty(), + "versa_bedrock declares {shared:?}, which the PUBLIC aws_bedrock provider \ + declares too, so a Versa setup marks that card Configured and hands it the \ + value. It declares {outside:?} outside its own namespace, and every \ + `AWS_*` key is the public providers' too: `bedrock.rs` exports each one \ + into the process environment, where the AWS SDK reads it. Two providers \ + of different privacy tiers must not share a config key." + ); +} + +// ------------------------------------------------------ public card → Versa + +/// What the public Amazon Bedrock card, or `bedrock.rs`'s export of it, leaves +/// where Versa used to look: its user's own AWS region, and an endpoint in it. +/// Versa read both whenever its own were unset. Its requests, signed with +/// UCSF-issued keys, then went to that user's AWS region, which refused the +/// keys, and the instance turned Public. The region alone was enough to sign +/// every request for a region other than the gateway's. +#[tokio::test] +async fn the_public_cards_endpoint_and_region_never_reach_versa() { + let mut public_card = versa_config("", ""); + public_card.insert("AWS_ENDPOINT_URL_BEDROCK".into(), PUBLIC_ENDPOINT.into()); + public_card.insert("AWS_REGION".into(), PUBLIC_REGION.into()); + + let versa = versa_bound(public_card).await; + assert_eq!( + resolved(&versa), + shipped(), + "the public Amazon Bedrock card's endpoint or region reached a Versa chat" + ); + + let sent = versa_sent(versa).await; + assert_eq!(sent.host, UCSF_GATEWAY_HOST, "{sent:?}"); + assert!(sent.path.starts_with("/general/awsai/model/"), "{sent:?}"); + assert_eq!( + sent.signed_by(), + Some((VERSA_ACCESS_KEY, VERSA_BEDROCK_DEFAULT_REGION)), + "{sent:?}" + ); +} + +/// The escape hatch survives, in Versa's own namespace. An operator can still +/// repoint the endpoint or the region, a blank value still means the shipped +/// default, and an endpoint off the gateway still demotes the instance: the +/// demotion guards a key anyone can write, not only the public card. +#[tokio::test] +async fn versas_own_overrides_still_steer_it() { + let blank = versa_bound(versa_config(" ", "")).await; + assert_eq!(resolved(&blank), shipped(), "blank must mean the default"); + + let repointed = "https://unified-api.ucsf.edu/general/awsai-v2"; + let custom = versa_bound(versa_config(repointed, "us-east-2")).await; + assert_eq!( + resolved(&custom), + ( + repointed.to_string(), + "us-east-2".to_string(), + format!("{:?}", ProviderTier::Private) + ) + ); + let sent = versa_sent(custom).await; + assert_eq!(sent.host, UCSF_GATEWAY_HOST, "{sent:?}"); + assert!( + sent.path.starts_with("/general/awsai-v2/model/"), + "{sent:?}" + ); + assert_eq!( + sent.signed_by(), + Some((VERSA_ACCESS_KEY, "us-east-2")), + "{sent:?}" + ); + + let off_site = versa_bound(versa_config(PUBLIC_ENDPOINT, "")).await; + assert_eq!( + resolved(&off_site).2, + format!("{:?}", ProviderTier::Public), + "an endpoint off the UCSF gateway must demote the instance" + ); +} + +/// The public card's Bedrock API key, alone in the environment, where the AWS +/// SDK's own documentation tells its user to put it. +/// +/// This one reaches past Versa's own code. `AWS_BEARER_TOKEN_BEDROCK` is the +/// SDK's variable for a Bedrock API key; the SDK reads it itself and, finding +/// it, authenticates with that bearer token instead of signing. So a Versa chat +/// that looked entirely right, with the UCSF gateway, the gateway's region and a +/// Private tier, sent the public card's API key to UCSF in its `Authorization` +/// header, and Versa's own keys signed nothing. +#[tokio::test] +async fn the_public_cards_api_key_never_rides_on_a_versa_request() { + const SCENARIO: &str = "versa-beside-a-bedrock-api-key"; + if child_scenario().as_deref() == Some(SCENARIO) { + report_versa().await; + return; + } + + let observed = run_child( + "the_public_cards_api_key_never_rides_on_a_versa_request", + SCENARIO, + "{}\n", + &[("AWS_BEARER_TOKEN_BEDROCK", PUBLIC_BEARER_TOKEN)], + ); + assert_signed_by_versa_for_the_gateway(&observed); +} + +/// Everything the environment can hold for the PUBLIC side, all at once, and +/// none of it may steer a Versa request: what `bedrock.rs` exports and promotes, +/// what a shell holds for the AWS CLI, and the public card's own credentials. +#[tokio::test] +async fn nothing_in_the_process_environment_steers_versa() { + const SCENARIO: &str = "versa-in-a-public-environment"; + if child_scenario().as_deref() == Some(SCENARIO) { + report_versa().await; + return; + } + + let observed = run_child( + "nothing_in_the_process_environment_steers_versa", + SCENARIO, + "{}\n", + &[ + ("AWS_ENDPOINT_URL_BEDROCK", PUBLIC_ENDPOINT), + ("AWS_ENDPOINT_URL_BEDROCK_RUNTIME", PUBLIC_ENDPOINT), + ("AWS_REGION", PUBLIC_REGION), + ("AWS_BEARER_TOKEN_BEDROCK", PUBLIC_BEARER_TOKEN), + ("AWS_ACCESS_KEY_ID", PUBLIC_ACCESS_KEY), + ("AWS_SECRET_ACCESS_KEY", PUBLIC_SECRET_KEY), + ], + ); + assert_signed_by_versa_for_the_gateway(&observed); +} + +/// The child half of the two rows above: a Versa chat bound with its +/// credentials and nothing else, reported with the one request it sent. +async fn report_versa() { + let versa = versa_bound(versa_config("", "")).await; + report(Observed { + resolved: Some(resolved(&versa)), + sent: versa_sent(versa).await, + }); +} + +/// The shipped endpoint, region and tier, and a request to the gateway signed +/// with Versa's own keys for the gateway's region, carrying no bearer token. One +/// comparison, so a failure shows every part of the request at once. +fn assert_signed_by_versa_for_the_gateway(observed: &Observed) { + let sent = &observed.sent; + assert_eq!( + ( + observed.resolved.clone(), + sent.host.as_str(), + sent.signed_by(), + ), + ( + Some(shipped()), + UCSF_GATEWAY_HOST, + Some((VERSA_ACCESS_KEY, VERSA_BEDROCK_DEFAULT_REGION)), + ), + "the process environment steered a Versa request: {sent:?}" + ); + assert!( + !sent.authorization.contains(PUBLIC_BEARER_TOKEN), + "the public card's API key rode along on a Versa request: {sent:?}" + ); +} + +// ------------------------------------------------------ Versa → public card + +/// The other direction. Versa's setup persisted `AWS_ENDPOINT_URL_BEDROCK` +/// pointing at the UCSF gateway, and `bedrock.rs` promoted that key to the +/// variable the SDK reads. So once the PUBLIC provider was built, it sent the +/// user's own AWS-signed requests to UCSF's gateway, which refused them. +/// Existing installs keep that key, so the public provider has to ignore it; it +/// is not enough to stop writing it. +#[tokio::test] +async fn versas_persisted_endpoint_never_becomes_the_public_providers() { + const SCENARIO: &str = "public-after-a-versa-setup"; + if child_scenario().as_deref() == Some(SCENARIO) { + report(Observed { + resolved: None, + sent: public_sent(public_bound().await).await, + }); + return; + } + + // Exactly what the Versa Bedrock onboarding card wrote on every connect. + let config_yaml = format!( + "AWS_ENDPOINT_URL_BEDROCK: {VERSA_BEDROCK_DEFAULT_ENDPOINT}\n\ + AWS_REGION: {VERSA_BEDROCK_DEFAULT_REGION}\n" + ); + let observed = run_child( + "versas_persisted_endpoint_never_becomes_the_public_providers", + SCENARIO, + &config_yaml, + &[ + ("AWS_ACCESS_KEY_ID", PUBLIC_ACCESS_KEY), + ("AWS_SECRET_ACCESS_KEY", PUBLIC_SECRET_KEY), + ], + ); + let sent = &observed.sent; + assert_eq!( + sent.host, "bedrock-runtime.us-west-2.amazonaws.com", + "Versa's persisted endpoint became the public provider's: {sent:?}" + ); + assert_eq!( + sent.signed_by(), + Some((PUBLIC_ACCESS_KEY, "us-west-2")), + "{sent:?}" + ); +} + +/// …while the public provider still follows the AWS SDK's own variable for +/// this service, which is how a VPC endpoint or a proxy is meant to be set. +#[tokio::test] +async fn the_public_provider_still_follows_the_sdks_endpoint_variable() { + const SCENARIO: &str = "public-with-its-own-endpoint"; + if child_scenario().as_deref() == Some(SCENARIO) { + report(Observed { + resolved: None, + sent: public_sent(public_bound().await).await, + }); + return; + } + + let vpc_endpoint = + "https://vpce-0123456789abcdef0-abcdefgh.bedrock-runtime.us-west-2.vpce.amazonaws.com"; + let observed = run_child( + "the_public_provider_still_follows_the_sdks_endpoint_variable", + SCENARIO, + "AWS_REGION: us-west-2\n", + &[ + ("AWS_ACCESS_KEY_ID", PUBLIC_ACCESS_KEY), + ("AWS_SECRET_ACCESS_KEY", PUBLIC_SECRET_KEY), + ("AWS_ENDPOINT_URL_BEDROCK_RUNTIME", vpc_endpoint), + ], + ); + assert_eq!( + observed.sent.host, + url::Url::parse(vpc_endpoint).unwrap().host_str().unwrap(), + "{:?}", + observed.sent + ); +} + +async fn public_bound() -> BedrockProvider { + crate::config::with_config_overrides( + HashMap::from([("BEDROCK_MAX_RETRIES".into(), "0".into())]), + BedrockProvider::from_env(ModelConfig::new_or_fail(BEDROCK_DEFAULT_MODEL)), + ) + .await + .unwrap_or_else(|e| panic!("the public provider must construct from env credentials: {e}")) +} + +// ------------------------------------------------------------ child process + +/// Names the scenario a re-executed copy of this binary is to run. A +/// test-private key: no production reader resolves it. +const CHILD: &str = "BIOROUTER_TEST_BEDROCK_NAMESPACE_CHILD"; +const REPORT: &str = "BEDROCK_NAMESPACE_OBSERVED "; + +fn child_scenario() -> Option { + std::env::var(CHILD).ok() +} + +fn report(observed: Observed) { + println!("{REPORT}{}", serde_json::to_string(&observed).unwrap()); +} + +/// Re-run `test`, a test in this module, as a child process whose half of the +/// test runs `scenario`. It starts with `env` and with none of the AWS, Versa or +/// Bedrock settings this process inherited, over a config root of its own that +/// holds `config_yaml`, with no AWS profile files and no instance metadata. +fn run_child(test: &str, scenario: &str, config_yaml: &str, env: &[(&str, &str)]) -> Observed { + // A child that reached a parent half would spawn its own child, and so on: + // stop at the first one rather than fork without end. + assert!( + child_scenario().is_none(), + "the child half of `{test}` did not claim scenario `{scenario}`" + ); + let root = tempfile::tempdir().unwrap(); + let config_dir = root.path().join("config"); + std::fs::create_dir_all(&config_dir).unwrap(); + std::fs::write(config_dir.join("config.yaml"), config_yaml).unwrap(); + + let mut command = std::process::Command::new(std::env::current_exe().unwrap()); + command.args([ + "--exact", + "--nocapture", + &format!("providers::bedrock_namespace_tests::{test}"), + ]); + for (name, _) in std::env::vars_os() { + let name = name.to_string_lossy(); + if ["AWS_", "VERSA_", "BEDROCK_"] + .iter() + .any(|prefix| name.starts_with(prefix)) + { + command.env_remove(name.as_ref()); + } + } + let output = command + .env(CHILD, scenario) + .env("BIOROUTER_PATH_ROOT", root.path()) + .env("BIOROUTER_DISABLE_KEYRING", "true") + .env("AWS_CONFIG_FILE", root.path().join("aws-config")) + .env( + "AWS_SHARED_CREDENTIALS_FILE", + root.path().join("aws-credentials"), + ) + .env("AWS_EC2_METADATA_DISABLED", "true") + .envs(env.iter().copied()) + .output() + .unwrap(); + + let stdout = String::from_utf8_lossy(&output.stdout); + let stderr = String::from_utf8_lossy(&output.stderr); + let line = stdout + .lines() + .find_map(|line| line.strip_prefix(REPORT)) + .unwrap_or_else(|| { + panic!( + "the child half of `{test}` reported nothing.\n\ + --- child stdout ---\n{stdout}\n--- child stderr ---\n{stderr}" + ) + }); + assert!( + output.status.success(), + "the child half of `{test}` failed.\n--- child stdout ---\n{stdout}\n\ + --- child stderr ---\n{stderr}" + ); + serde_json::from_str(line).unwrap() +} diff --git a/crates/biorouter/src/providers/factory.rs b/crates/biorouter/src/providers/factory.rs index d872c9072..79eea02a9 100644 --- a/crates/biorouter/src/providers/factory.rs +++ b/crates/biorouter/src/providers/factory.rs @@ -510,7 +510,7 @@ pub(crate) mod tests { ), ( "versa_azure", - "Institution(ucsf): `ucsf_gateway_affiliation` on the resolved AZURE_OPENAI_ENDPOINT", + "Institution(ucsf): `ucsf_gateway_affiliation` on the resolved VERSA_AZURE_ENDPOINT", ), ]; #[cfg(feature = "aws-providers")] diff --git a/crates/biorouter/src/providers/mod.rs b/crates/biorouter/src/providers/mod.rs index b22889262..f978c5aea 100644 --- a/crates/biorouter/src/providers/mod.rs +++ b/crates/biorouter/src/providers/mod.rs @@ -8,6 +8,8 @@ pub mod azureauth; pub mod base; #[cfg(feature = "aws-providers")] pub mod bedrock; +#[cfg(all(test, feature = "aws-providers"))] +mod bedrock_namespace_tests; pub mod canonical; pub mod claude_code; pub mod codex; @@ -132,11 +134,11 @@ pub(crate) fn is_loopback_host(url: &str) -> bool { /// The tier of a provider that reaches the UCSF gateway and nothing else. /// -/// Demotion only, never promotion: `versa_azure` shares all three -/// `AZURE_OPENAI_*` keys with the public `azure_openai` provider, and -/// `bedrock.rs` sets `AWS_ENDPOINT_URL_BEDROCK_RUNTIME` process-globally, so an -/// endpoint that is not the gateway means the transcript is going somewhere -/// this build cannot vouch for. +/// Demotion only, never promotion: each Versa provider's endpoint is +/// user-writable config (`VERSA_AZURE_ENDPOINT`, `VERSA_BEDROCK_ENDPOINT`), and +/// until 2026-09-11 each also read the public card's keys, so an endpoint that +/// is not the gateway means the transcript is going somewhere this build cannot +/// vouch for. pub(crate) fn ucsf_gateway_tier(endpoint: &str) -> ProviderTier { if host_of(endpoint).as_deref() == Some(UCSF_GATEWAY_HOST) { ProviderTier::Private @@ -164,10 +166,8 @@ pub(crate) fn self_hosted_tier(base_url: &str) -> ProviderTier { /// ones a test thought to list. A name-keyed table (`versa_* => ucsf`) would /// keep claiming the institution for a Versa module repointed at another host, /// which `tier()` had already demoted to Public: a private-looking badge on a -/// public flow. The three `AZURE_OPENAI_*` keys are shared with the public -/// `azure_openai` provider and `bedrock.rs` sets -/// `AWS_ENDPOINT_URL_BEDROCK_RUNTIME` process-globally, so that repointing is a -/// config edit away. +/// public flow. Each Versa endpoint is user-writable config, so that repointing +/// is a config edit away. pub(crate) fn ucsf_gateway_affiliation(endpoint: &str) -> Option { match ucsf_gateway_tier(endpoint) { ProviderTier::Private => Some(*UCSF_AFFILIATION), diff --git a/crates/biorouter/src/providers/tier_tests.rs b/crates/biorouter/src/providers/tier_tests.rs index 077b64bc4..0aae62eef 100644 --- a/crates/biorouter/src/providers/tier_tests.rs +++ b/crates/biorouter/src/providers/tier_tests.rs @@ -291,11 +291,11 @@ fn only_a_loopback_host_reads_as_this_machine() { #[test] fn versa_demotes_when_its_endpoint_is_not_the_ucsf_gateway() { use crate::privacy::ProviderTier::{Private, Public}; - // versa_azure reads AZURE_OPENAI_ENDPOINT, the same key the public - // azure_openai provider reads, and versa_bedrock falls back to - // AWS_ENDPOINT_URL_BEDROCK_RUNTIME, which bedrock.rs sets PROCESS-GLOBALLY - // with std::env::set_var. The shipped constants are asserted rather than - // their text, so moving a default off the gateway fails here too. + // Both Versa endpoints are user-writable config (VERSA_AZURE_ENDPOINT, + // VERSA_BEDROCK_ENDPOINT), and until 2026-09-11 each also read the public + // card's keys: AZURE_OPENAI_ENDPOINT, and AWS_ENDPOINT_URL_BEDROCK plus the + // process environment. The shipped constants are asserted rather than their + // text, so moving a default off the gateway fails here too. assert_eq!(versa_tier_for_endpoint(VERSA_AZURE_ENDPOINT), Private); assert_eq!( versa_tier_for_endpoint("https://unified-api.ucsf.edu/general"), diff --git a/crates/biorouter/src/providers/versa_azure.rs b/crates/biorouter/src/providers/versa_azure.rs index c3d78f24f..7c681196d 100644 --- a/crates/biorouter/src/providers/versa_azure.rs +++ b/crates/biorouter/src/providers/versa_azure.rs @@ -26,27 +26,141 @@ use crate::providers::utils::RequestLog; use rmcp::model::Tool; pub const VERSA_AZURE_ENDPOINT: &str = "https://unified-api.ucsf.edu/general"; -pub const VERSA_AZURE_DEPLOYMENT: &str = "gpt-5.5-2026-04-24"; +/// The model a fresh Versa Azure chat starts on. +/// +/// ⚠ A MODEL, not a deployment. This constant used to be `VERSA_AZURE_DEPLOYMENT` +/// and every request posted to it, whatever model the chat named — so choosing +/// any of the other eight models changed the label, the context gauge and the +/// cost basis while `gpt-5.5` kept answering (2026-09-10 QA run, finding F1). +/// A request now posts to the deployment [`VERSA_AZURE_DEPLOYMENTS`] maps its +/// own model to. +pub const VERSA_AZURE_DEFAULT_MODEL: &str = "gpt-5.5-2026-04-24"; pub const VERSA_AZURE_API_VERSION: &str = "2025-01-01-preview"; pub const VERSA_AZURE_DOC_URL: &str = "http://biorouter.ucsf.edu/docs"; -// Versa proxies Azure OpenAI deployments; the authoritative list lives on the -// (login-gated) UCSF wiki "Models, deployments, and API endpoints in UCSF -// Versa". Public UCSF Versa docs are MyAccess-gated, so keep this list to -// deployments verified against the UCSF endpoint. Removed: o1-2024-12-17 and -// o3-mini-2025-01-31 (deprecated on Azure, retiring Jul/Aug 2026). -pub const VERSA_AZURE_KNOWN_MODELS: &[&str] = &[ - "gpt-5.5-2026-04-24", - "gpt-5.4-mini-2026-03-17", - "gpt-5.4-nano-2026-03-17", - "gpt-5.2-2025-12-11", - "gpt-5-2025-08-07", - "gpt-4.1-2025-04-14", - "gpt-4.1-mini-2025-04-14", - "gpt-4o-2024-11-20", - "o4-mini-2025-04-16", +/// Every model this provider offers, paired with the Azure deployment that +/// serves it at the UCSF gateway. The ONE list: `metadata()` advertises exactly +/// these models, and a request for one of them posts to exactly this deployment. +/// +/// Measured on 2026-09-11, not inferred. The gateway has no listing endpoint — +/// `GET openai/deployments` and `GET openai/models` both answer 405 — so every +/// deployment was sent a one-shot completion, and each answered 200 with its own +/// name as `model`. The names are the full dated ids: the short aliases +/// (`gpt-5.5`, `gpt-4.1`, `gpt-4o`) are all `DeploymentNotFound`. The same run +/// sent each deployment a prompt just over its `MODEL_CONTEXT_WINDOWS` entry, +/// and every refusal named that window — for the gpt-5 family as an INPUT limit, +/// the window less the 128k reserved for output — so the registry needed no +/// change. +/// +/// The authoritative list lives on the login-gated UCSF wiki ("Models, +/// deployments, and API endpoints in UCSF Versa"). o1-2024-12-17 and +/// o3-mini-2025-01-31 were removed earlier (deprecated on Azure, retiring +/// Jul/Aug 2026) and still answered on 2026-09-11; they stay removed. +/// `gpt-5-mini-2025-08-07` and `gpt-5-nano-2025-08-07` also answered and are +/// not offered yet. +pub const VERSA_AZURE_DEPLOYMENTS: &[(&str, &str)] = &[ + ("gpt-5.5-2026-04-24", "gpt-5.5-2026-04-24"), + ("gpt-5.4-mini-2026-03-17", "gpt-5.4-mini-2026-03-17"), + ("gpt-5.4-nano-2026-03-17", "gpt-5.4-nano-2026-03-17"), + ("gpt-5.2-2025-12-11", "gpt-5.2-2025-12-11"), + ("gpt-5-2025-08-07", "gpt-5-2025-08-07"), + ("gpt-4.1-2025-04-14", "gpt-4.1-2025-04-14"), + ("gpt-4.1-mini-2025-04-14", "gpt-4.1-mini-2025-04-14"), + ("gpt-4o-2024-11-20", "gpt-4o-2024-11-20"), + ("o4-mini-2025-04-16", "o4-mini-2025-04-16"), ]; +/// The deployment the UCSF gateway serves `model` from, if the catalog knows one. +pub fn deployment_for_model(model: &str) -> Option<&'static str> { + VERSA_AZURE_DEPLOYMENTS + .iter() + .find(|(name, _)| *name == model) + .map(|(_, deployment)| *deployment) +} + +fn is_catalog_deployment(value: &str) -> bool { + VERSA_AZURE_DEPLOYMENTS + .iter() + .any(|(_, deployment)| *deployment == value) +} + +/// What a restore binding stores in `deployment` for a model no deployment +/// serves, so a restored chat refuses it exactly as the live one did. +/// +/// ⚠ A marker inside the existing `deployment: String`, not an `Option`, because +/// the binding is format v1 and has shipped (1.89.x–1.90.x): a changed shape is +/// a row those builds cannot parse, and a failed restore is a 500 on resume — +/// the chat would not open at all. The marker passes `validate_path_component`, +/// names what it means when someone reads the row, and a build that predates +/// this change posts to it and gets the gateway's `DeploymentNotFound` +/// (measured 2026-09-11) — a failed turn, never an answer from another +/// deployment. +const NO_DEPLOYMENT_ROUTE: &str = + "no-versa-deployment-serves-this-model.biorouter-refuses-to-send-the-turn"; + +/// The deployment override `value` amounts to, if any. +/// +/// Judges BOTH sources of one — a configured `VERSA_AZURE_DEPLOYMENT_NAME` and +/// the `deployment` a session's restore binding stored — so a live provider and +/// a restored one cannot disagree about whether an override is in force. +/// +/// ⚠ A value that names a CATALOG deployment is not an override, and the fix +/// turns on it: +/// +/// * Nobody chose it. The onboarding card upserts `VERSA_AZURE_DEPLOYMENT_NAME` +/// with the shipped default on every connect. Honouring it would pin every +/// onboarded install to one model — F1 again, in exactly the installs the +/// QA sandbox did not have. +/// * It is what every stored binding says. Before this change `deployment` +/// was the fixed default whatever the model, and a subagent's model override +/// still rewrites the binding's model without touching its route +/// (`subagent_tool.rs`). Re-deriving a catalog value from the model is what +/// heals those rows instead of carrying the wrong route forward. +/// * It adds nothing. Choosing that model reaches that deployment honestly; +/// an override can only make some other model's label point at it. +/// +/// What is left is the real escape hatch: a deployment the catalog does not +/// know, which then serves every request. +/// +/// A row written by a build that still read the public `azure_openai` card's +/// `AZURE_OPENAI_DEPLOYMENT_NAME` may carry that card's deployment here, and +/// nothing in the row tells it from a chosen one — so it is honoured like one +/// until the chat's model is picked again, which rebuilds through `from_env`. +fn explicit_override(value: Option<&str>) -> Option { + let value = value?.trim(); + (!value.is_empty() && value != NO_DEPLOYMENT_ROUTE && !is_catalog_deployment(value)) + .then(|| value.to_string()) +} + +/// The route a restore binding stores, and the one place that decides it: the +/// override if one is in force, else the model's catalog deployment, else +/// [`NO_DEPLOYMENT_ROUTE`]. [`explicit_override`] reads it back. +fn stored_route(deployment_override: Option<&str>, model: &str) -> String { + deployment_override + .or_else(|| deployment_for_model(model)) + .unwrap_or(NO_DEPLOYMENT_ROUTE) + .to_string() +} + +/// The refusal for a model no deployment serves, raised before the payload is +/// built — so nothing is logged and nothing leaves the machine. +/// +/// `RequestFailed` whose text says "deployment not found", which +/// `ProviderError::kind` classifies as `ModelUnavailable`: not transient, so the +/// turn stops on the first attempt instead of retrying a request that can never +/// succeed, and the desktop titles it "Model unavailable". +fn no_deployment_error(model: &str) -> ProviderError { + let available = VERSA_AZURE_DEPLOYMENTS + .iter() + .map(|(name, _)| *name) + .collect::>() + .join(", "); + ProviderError::RequestFailed(format!( + "no Versa deployment for model `{model}` (Azure deployment not found, so nothing \ + was sent); available: {available}. Switch this chat to one of those models." + )) +} + fn versa_azure_model_supports_vision(name: &str) -> bool { !name.contains("codex") && (name.starts_with("gpt-5") @@ -59,13 +173,17 @@ fn versa_azure_model_supports_vision(name: &str) -> bool { #[derive(Debug)] pub struct VersaAzureProvider { api_client: ApiClient, - deployment_name: String, + /// A deployment EVERY request posts to, whatever model it names — the + /// operator's escape hatch for a deployment the catalog does not know yet. + /// `None`, the normal case: each request posts to the deployment its own + /// model maps to (see [`explicit_override`] for what can set it). + deployment_override: Option, api_version: String, model: ModelConfig, name: String, /// The endpoint this instance resolved at construction. `tier()` reads it, - /// never the provider's name — the three `AZURE_OPENAI_*` keys are shared - /// with the public `azure_openai` provider and are user-writable. + /// never the provider's name — `VERSA_AZURE_ENDPOINT` is user-writable, so + /// an instance can resolve somewhere that is not the UCSF gateway. resolved_endpoint: String, credential_source: VersaAzureCredentialSource, } @@ -77,7 +195,10 @@ impl Serialize for VersaAzureProvider { { use serde::ser::SerializeStruct; let mut state = serializer.serialize_struct("VersaAzureProvider", 2)?; - state.serialize_field("deployment_name", &self.deployment_name)?; + state.serialize_field( + "deployment_name", + &stored_route(self.deployment_override.as_deref(), &self.model.model_name), + )?; state.serialize_field("api_version", &self.api_version)?; state.end() } @@ -108,18 +229,13 @@ impl AuthProvider for VersaAzureAuthProvider { } } -/// Resolve one endpoint override: this provider's own key first, the public -/// Azure provider's key second, the shipped UCSF default last. +/// Resolve one override: this provider's own key, else the shipped UCSF +/// default. There is no third source — see `from_env` for the one there was. /// -/// Pure, and split out from `from_env`, because the ORDER is the whole fix and -/// a closure inside a function that reads global config cannot be tested. /// A blank stored value is treated as absent -- writing an empty string into /// the box in Advanced means "use the default", not "point at nowhere". -fn resolve_override(fallback: &str, own: Option, legacy: Option) -> String { - [own, legacy] - .into_iter() - .flatten() - .find(|value| !value.trim().is_empty()) +fn resolve_override(fallback: &str, own: Option) -> String { + own.filter(|value| !value.trim().is_empty()) .unwrap_or_else(|| fallback.to_string()) } @@ -127,44 +243,78 @@ impl VersaAzureProvider { pub async fn from_env(model: ModelConfig) -> Result { let config = crate::config::Config::global(); - // Overrides are read from this provider's OWN namespace first, and only - // then from the public Azure provider's keys. + // Overrides are read from this provider's OWN namespace, and nowhere + // else. // - // ⚠ The order is the fix, not a preference. `AZURE_OPENAI_ENDPOINT` and - // its two siblings belong to the `azure_openai` card, and - // `check_provider_configured` reads exactly those to decide whether that - // card says Configured. While onboarding wrote them on Versa's behalf, + // ⚠ `AZURE_OPENAI_ENDPOINT` and its two siblings belong to the public + // `azure_openai` card — `check_provider_configured` reads them to decide + // whether that card says Configured — and sharing them went wrong in + // both directions. Onboarding WROTE them on Versa's behalf, so // connecting UCSF's PRIVATE Versa lit up the PUBLIC Azure OpenAI card as - // configured -- a provider the user never set up, sitting one click away - // in the same grid, and Public where Versa is Private. The legacy read - // stays so an install that customised those keys before this change - // keeps resolving to the same endpoint. + // configured; that is why these `VERSA_AZURE_*` keys exist + // (2026-09-03). And Versa went on READING them as a fallback, so + // whatever that card was set up with steered Versa: a company Azure + // resource's endpoint received every Versa request — the transcript, + // with `VERSA_AZURE_API_KEY` in its `api-key` header — refused the key, + // and the instance turned Public. Its deployment and API version rode + // along. The fallback is gone (2026-09-11). + // + // Every Versa setup form prefilled the endpoint constant, so an endpoint + // this drops was either typed over the prefill or carried in from the + // public card — the bug itself. The API version is the exception: the + // onboarding card of 2026-05-30 to 2026-07-02 prefilled `2024-10-21` + // into the legacy key, and those installs now send the shipped version + // every other install sends. + // // ⚠ Every key below is a STRING LITERAL passed straight to `get_param`, // and it has to stay that way. `privacy::config_keys` scans this file for // literal-keyed `get_param` calls to build the list of keys that move a // provider's tier, and a key assembled at runtime — even one as innocent - // as a `|own, legacy|` closure parameter — is invisible to that scan. The - // first draft of this fix did exactly that and took the three - // `AZURE_OPENAI_*` keys off the privacy surface without anyone deciding - // to; two tests in that module caught it. + // as an `|own, legacy|` closure parameter — is invisible to that scan. + // The first draft of the 2026-09-03 namespacing did exactly that and + // took three keys off the privacy surface without anyone deciding to; + // two tests in that module caught it. let endpoint = resolve_override( VERSA_AZURE_ENDPOINT, config.get_param::("VERSA_AZURE_ENDPOINT").ok(), - config.get_param::("AZURE_OPENAI_ENDPOINT").ok(), - ); - let deployment_name = resolve_override( - VERSA_AZURE_DEPLOYMENT, - config - .get_param::("VERSA_AZURE_DEPLOYMENT_NAME") - .ok(), - config - .get_param::("AZURE_OPENAI_DEPLOYMENT_NAME") - .ok(), ); + // There is no shipped default deployment any more: the model picks it. + // What the configuration names is a CANDIDATE override, and + // `explicit_override` decides whether it is one — which is what keeps + // the default the onboarding card persists from pinning every model. + // + // ⚠ Versa's OWN key, and no fallback. `AZURE_OPENAI_DEPLOYMENT_NAME` + // used to be read after it, and it is not Versa's to read: it is the + // one key the public `azure_openai` card requires and ships no default + // for, so it names a deployment on whatever Azure resource the user set + // THAT card up with (`my-gpt4o`, or `gpt-4o` — Azure's habit of naming a + // deployment after its model). The catalog knows no such name, so it + // became an override serving every Versa request: DeploymentNotFound on + // each turn, or — where it is a real UCSF deployment the catalog does + // not offer — a silent answer from the wrong model. The fallback was + // kept for installs whose pre-2026-09-03 Versa forms wrote that key, + // and those forms prefilled catalog deployments, which are not + // overrides; only a value someone typed over the prefill is dropped. + let configured_deployment = config + .get_param::("VERSA_AZURE_DEPLOYMENT_NAME") + .unwrap_or_default(); + let deployment_override = explicit_override(Some(&configured_deployment)); + match &deployment_override { + Some(deployment) => tracing::info!( + deployment = %deployment, + "Versa Azure: a configured deployment override is in force; every request \ + posts to it, whatever model the chat selected" + ), + None if !configured_deployment.trim().is_empty() => tracing::debug!( + configured = %configured_deployment.trim(), + "Versa Azure: the configured deployment is one the catalog already maps to a \ + model, so it is not an override; each model posts to its own deployment" + ), + None => {} + } let api_version = resolve_override( VERSA_AZURE_API_VERSION, config.get_param::("VERSA_AZURE_API_VERSION").ok(), - config.get_param::("AZURE_OPENAI_API_VERSION").ok(), ); // ⚠ `.ok()` here used to discard the REASON the key was unavailable, and @@ -207,26 +357,54 @@ impl VersaAzureProvider { } }; - Self::from_resolved( + Self::build( model, SecretFreeEndpoint::new(endpoint)?, - deployment_name, + deployment_override, api_version, credential_source, ) } + /// Rebuild the provider a restore binding describes. + /// + /// `deployment` is the route the binding stored (see [`stored_route`]), and + /// it is read back through [`explicit_override`] — the rule `from_env` + /// applies to configuration — rather than trusted as the deployment to post + /// to. That is what makes a row the old code wrote (a catalog deployment + /// beside a DIFFERENT model) post to its own model's deployment now, instead + /// of carrying the wrong route forward forever. + /// + /// Never fails for want of a deployment: a model none serves is refused per + /// request, before anything is sent. Failing here would fail the restore, + /// and a failed restore is a chat that will not open. pub(crate) fn from_resolved( model: ModelConfig, endpoint: SecretFreeEndpoint, - deployment_name: String, + deployment: String, + api_version: String, + credential_source: VersaAzureCredentialSource, + ) -> Result { + Self::build( + model, + endpoint, + explicit_override(Some(&deployment)), + api_version, + credential_source, + ) + } + + fn build( + model: ModelConfig, + endpoint: SecretFreeEndpoint, + deployment_override: Option, api_version: String, credential_source: VersaAzureCredentialSource, ) -> Result { let binding = ProviderRestoreBinding::VersaAzure { model: model.clone(), endpoint: endpoint.clone(), - deployment: deployment_name.clone(), + deployment: stored_route(deployment_override.as_deref(), &model.model_name), api_version: api_version.clone(), credential_source, }; @@ -256,7 +434,7 @@ impl VersaAzureProvider { Ok(Self { api_client, - deployment_name, + deployment_override, api_version, model, name: Self::metadata().name, @@ -265,15 +443,30 @@ impl VersaAzureProvider { }) } + /// The deployment a request for `model` posts to: the override if one is in + /// force, else the deployment the catalog maps `model` to. + /// + /// It takes the model the REQUEST names, not the one this provider was built + /// with, because they differ: `complete_fast` sends the fast model through + /// `complete_with_model`, and it used to land on the main deployment too. + fn deployment_for(&self, model: &str) -> Result<&str, ProviderError> { + match &self.deployment_override { + Some(deployment) => Ok(deployment), + None => deployment_for_model(model).ok_or_else(|| no_deployment_error(model)), + } + } + /// The single source of truth for the Azure deployment path, shared by the /// blocking and streaming paths so they cannot drift. - fn chat_completions_path(&self) -> String { - build_chat_completions_path(&self.deployment_name, &self.api_version) + fn chat_completions_path(&self, model: &str) -> Result { + Ok(build_chat_completions_path( + self.deployment_for(model)?, + &self.api_version, + )) } - async fn post(&self, payload: &Value) -> Result { - let path = self.chat_completions_path(); - let response = self.api_client.response_post(&path, payload).await?; + async fn post(&self, path: &str, payload: &Value) -> Result { + let response = self.api_client.response_post(path, payload).await?; handle_response_openai_compat(response).await } @@ -306,9 +499,9 @@ impl VersaAzureProvider { #[async_trait] impl Provider for VersaAzureProvider { fn metadata() -> ProviderMetadata { - let models = VERSA_AZURE_KNOWN_MODELS + let models = VERSA_AZURE_DEPLOYMENTS .iter() - .map(|&name| { + .map(|&(name, _)| { let info = ModelInfo::new(name, ModelConfig::new_or_fail(name).context_limit()); if versa_azure_model_supports_vision(name) { info.with_vision() @@ -322,7 +515,7 @@ impl Provider for VersaAzureProvider { "versa_azure", "Versa API Azure", "UCSF ChatGPT via Azure OpenAI. API Key only; endpoint and deployment are pre-configured.", - VERSA_AZURE_DEPLOYMENT, + VERSA_AZURE_DEFAULT_MODEL, models, VERSA_AZURE_DOC_URL, // ⚠ The API key, and NOTHING else. This provider's own description @@ -340,13 +533,18 @@ impl Provider for VersaAzureProvider { // Configured — pointing at the UCSF endpoint while carrying the // weaker Public tier. Reported from the field. // - // Removing them costs nothing: `from_env` reads the same names with - // `unwrap_or_else(|_| VERSA_AZURE_*)`, so an operator who sets them - // in the environment or config still overrides, and an install that - // sets nothing still gets the UCSF gateway. + // Removing them costs nothing: an install that sets nothing still + // gets the UCSF gateway, because `from_env` falls back to the + // `VERSA_AZURE_*` constants, and an operator overrides through + // Versa's own `VERSA_AZURE_*` keys (see `from_env`). vec![ConfigKey::new("VERSA_AZURE_API_KEY", true, true, None)], ) - .with_unlisted_models() + // ⚠ No `with_unlisted_models()`, which this provider used to declare. A + // model the catalog does not map has no deployment and is refused + // before it is sent, so "Enter a model not listed..." could only offer + // a choice that fails — or, with an override in force, one that changes + // nothing but the label. + // // The shipped endpoint is the UCSF gateway, so a default install is // Private. An instance that resolved elsewhere says so itself, below. .with_tier(ProviderTier::Private) @@ -366,7 +564,10 @@ impl Provider for VersaAzureProvider { model: model_without_restore_marker(self.model.clone()), endpoint: SecretFreeEndpoint::new(self.resolved_endpoint.clone()) .expect("resolved Versa Azure endpoint must remain valid"), - deployment: self.deployment_name.clone(), + // Decided afresh on every call — the override if one is in force, + // else THIS model's deployment — so a rebind can never leave a route + // behind that names some other model's deployment. + deployment: stored_route(self.deployment_override.as_deref(), &self.model.model_name), api_version: self.api_version.clone(), credential_source: self.credential_source, } @@ -398,6 +599,9 @@ impl Provider for VersaAzureProvider { messages: &[Message], tools: &[Tool], ) -> Result<(Message, ProviderUsage), ProviderError> { + // First, before the payload exists: a model no deployment serves is + // refused here, so nothing is logged and nothing is sent. + let path = self.chat_completions_path(&model_config.model_name)?; let payload = create_request( model_config, system, @@ -410,7 +614,7 @@ impl Provider for VersaAzureProvider { let response = self .with_retry(|| async { let payload_clone = payload.clone(); - self.post(&payload_clone).await + self.post(&path, &payload_clone).await }) .await .inspect_err(|e| { @@ -441,10 +645,11 @@ impl Provider for VersaAzureProvider { messages: &[Message], tools: &[Tool], ) -> Result { + // Same order as `complete_with_model`: refuse before anything is built. + let path = self.chat_completions_path(&self.model.model_name)?; let payload = self.build_stream_payload(system, messages, tools)?; let mut log = RequestLog::start(&self.model, &payload)?; - let path = self.chat_completions_path(); let response = self .with_retry(|| async { let resp = self.api_client.response_post(&path, &payload).await?; @@ -463,27 +668,16 @@ impl Provider for VersaAzureProvider { mod tests { use super::*; + /// That the public card's keys are never read is asserted where it can be + /// seen — on a real `from_env` in `routing_tests` — not here. #[test] - fn an_override_prefers_versas_own_key_then_the_legacy_one_then_the_default() { - // Own key wins. - assert_eq!( - resolve_override("default", Some("own".into()), Some("legacy".into())), - "own" - ); - // An install that customised the shared key BEFORE the namespace existed - // still resolves to the endpoint it chose. - assert_eq!( - resolve_override("default", None, Some("legacy".into())), - "legacy" - ); + fn an_override_is_versas_own_key_or_the_default() { + assert_eq!(resolve_override("default", Some("own".into())), "own"); // Nothing stored: the shipped UCSF default, which is why onboarding never // needed to write these keys at all. - assert_eq!(resolve_override("default", None, None), "default"); + assert_eq!(resolve_override("default", None), "default"); // Blank is absent, not "point at nowhere". - assert_eq!( - resolve_override("default", Some(" ".into()), None), - "default" - ); + assert_eq!(resolve_override("default", Some(" ".into())), "default"); } use crate::providers::api_client::AuthMethod; @@ -506,9 +700,9 @@ mod tests { VersaAzureProvider { api_client, - deployment_name: VERSA_AZURE_DEPLOYMENT.to_string(), + deployment_override: None, api_version: VERSA_AZURE_API_VERSION.to_string(), - model: ModelConfig::new_or_fail(VERSA_AZURE_DEPLOYMENT), + model: ModelConfig::new_or_fail(VERSA_AZURE_DEFAULT_MODEL), name: "versa_azure".to_string(), resolved_endpoint: VERSA_AZURE_ENDPOINT.to_string(), credential_source: VersaAzureCredentialSource::ApiKey, @@ -522,10 +716,9 @@ mod tests { /// this provider calls it, or hands it the right field. Replace the body of /// `tier()` with an unconditional `Private`, or point it at a field that is /// not the resolved endpoint, and every one of those tests still passes. - /// This one does not: the three `AZURE_OPENAI_*` keys are shared with the - /// public `azure_openai` provider, so a `tier()` that ignores the endpoint - /// hands a private badge to a provider posting transcripts wherever the - /// user's config points. + /// This one does not: `VERSA_AZURE_ENDPOINT` is user-writable config, so a + /// `tier()` that ignores the endpoint hands a private badge to a provider + /// posting transcripts wherever that config points. #[test] fn tier_follows_the_endpoint_this_instance_resolved() { let shipped = test_provider(); @@ -552,8 +745,8 @@ mod tests { /// it or hands it the right field. Returning an unconditional /// `Institution("ucsf")` here — or keying it on `get_name()`, which is the /// obvious implementation — passes every one of those tests and hands a UCSF - /// badge to an instance posting prompts wherever the user's shared - /// `AZURE_OPENAI_ENDPOINT` points. + /// badge to an instance posting prompts wherever the user's + /// `VERSA_AZURE_ENDPOINT` points. #[test] fn affiliation_follows_the_endpoint_this_instance_resolved() { use crate::privacy::affiliation::{InstitutionId, ModelAffiliation}; @@ -598,7 +791,10 @@ mod tests { let encoded = serde_json::to_value(provider.restore_binding()).unwrap(); assert_eq!(encoded["kind"], "versa_azure"); assert_eq!(encoded["endpoint"], VERSA_AZURE_ENDPOINT); - assert_eq!(encoded["deployment"], VERSA_AZURE_DEPLOYMENT); + assert_eq!( + encoded["deployment"], + deployment_for_model(VERSA_AZURE_DEFAULT_MODEL).unwrap() + ); assert_eq!(encoded["api_version"], VERSA_AZURE_API_VERSION); assert_eq!(encoded["credential_source"], "api_key"); assert!(!encoded.to_string().contains("test-key")); @@ -613,9 +809,9 @@ mod tests { HashMap::from([("VERSA_AZURE_API_KEY".into(), String::new())]), async { VersaAzureProvider::from_resolved( - ModelConfig::new_or_fail(VERSA_AZURE_DEPLOYMENT), + ModelConfig::new_or_fail(VERSA_AZURE_DEFAULT_MODEL), endpoint(), - VERSA_AZURE_DEPLOYMENT.into(), + VERSA_AZURE_DEFAULT_MODEL.into(), VERSA_AZURE_API_VERSION.into(), VersaAzureCredentialSource::ApiKey, ) @@ -634,9 +830,9 @@ mod tests { )]), async { VersaAzureProvider::from_resolved( - ModelConfig::new_or_fail(VERSA_AZURE_DEPLOYMENT), + ModelConfig::new_or_fail(VERSA_AZURE_DEFAULT_MODEL), endpoint(), - VERSA_AZURE_DEPLOYMENT.into(), + VERSA_AZURE_DEFAULT_MODEL.into(), VERSA_AZURE_API_VERSION.into(), VersaAzureCredentialSource::AzureCli, ) @@ -693,7 +889,9 @@ mod tests { "Versa Azure cannot inject into a running HTTP response, so a queued steer must restart it" ); assert_eq!( - provider.chat_completions_path(), + provider + .chat_completions_path(&provider.model.model_name) + .unwrap(), "openai/deployments/gpt-5.5-2026-04-24/chat/completions?api-version=2025-01-01-preview" ); } @@ -717,7 +915,7 @@ mod tests { /// response when `stream_options.include_usage` is set. #[test] fn streaming_payload_sets_stream_and_usage_options() { - let model = ModelConfig::new_or_fail(VERSA_AZURE_DEPLOYMENT); + let model = ModelConfig::new_or_fail(VERSA_AZURE_DEFAULT_MODEL); let payload = create_request(&model, "sys", &[], &[], &ImageFormat::OpenAi, true) .expect("streaming request should build"); @@ -731,13 +929,168 @@ mod tests { #[test] fn non_streaming_payload_does_not_set_stream() { - let model = ModelConfig::new_or_fail(VERSA_AZURE_DEPLOYMENT); + let model = ModelConfig::new_or_fail(VERSA_AZURE_DEFAULT_MODEL); let payload = create_request(&model, "sys", &[], &[], &ImageFormat::OpenAi, false) .expect("request should build"); assert!(payload.get("stream").is_none()); assert!(payload.get("stream_options").is_none()); } + + /// The map the requests are routed by IS the measured snapshot, and the + /// default model is on it. + #[test] + fn the_deployment_map_is_the_measured_snapshot() { + assert_eq!(VERSA_AZURE_DEPLOYMENTS, super::routing_tests::MEASURED); + for (model, deployment) in VERSA_AZURE_DEPLOYMENTS { + assert_eq!(deployment_for_model(model), Some(*deployment)); + } + assert!(deployment_for_model(VERSA_AZURE_DEFAULT_MODEL).is_some()); + assert_eq!(deployment_for_model("gpt-4.1-bogus-qa-probe"), None); + // The short aliases are DeploymentNotFound at the gateway (measured). + for alias in ["gpt-5.5", "gpt-4.1", "gpt-4o"] { + assert_eq!(deployment_for_model(alias), None, "{alias}"); + } + } + + #[test] + fn only_a_deployment_the_catalog_does_not_know_is_an_override() { + assert_eq!(explicit_override(None), None); + assert_eq!(explicit_override(Some("")), None); + assert_eq!(explicit_override(Some(" ")), None); + assert_eq!(explicit_override(Some(NO_DEPLOYMENT_ROUTE)), None); + for (_, deployment) in VERSA_AZURE_DEPLOYMENTS { + assert_eq!( + explicit_override(Some(deployment)), + None, + "{deployment} is a catalog deployment; the model picks it, an override cannot" + ); + } + assert_eq!( + explicit_override(Some(" ucsf-preview-deployment ")).as_deref(), + Some("ucsf-preview-deployment") + ); + assert!(!is_catalog_deployment(NO_DEPLOYMENT_ROUTE)); + + // Every default a setup surface ever persisted must stay a non-override: + // the onboarding card writes gpt-5.5 into `VERSA_AZURE_DEPLOYMENT_NAME`, + // and a session row written by a build that still read the legacy + // `AZURE_OPENAI_DEPLOYMENT_NAME` stored that key's default as its route. + // Today each is a catalog deployment, which is the only reason it is + // ignored — so trimming one from the catalog would silently turn it into + // an override and pin every chat that carries it. If this fails after a + // trim, keep recognising the value (a retired-defaults list beside the + // catalog) rather than editing it out of this test. + for shipped_default in [ + "gpt-5.2-2025-12-11", // provider default 2026-05-07 .. 2026-07-02 + "gpt-5.5-2026-04-24", // provider default since; the onboarding card's + ] { + assert_eq!( + explicit_override(Some(shipped_default)), + None, + "{shipped_default} was persisted by a setup form nobody chose it in" + ); + } + } + + /// A model no deployment serves still needs a VALID binding: `build` + /// validates it, and a binding that fails validation is a restore that + /// fails — a chat that will not open. Through the exact envelope a session + /// row stores, and back, the refusal survives. + #[tokio::test] + async fn an_unmapped_models_refusal_survives_the_session_row() { + use crate::providers::provider_binding::PersistedStandaloneProviderBinding; + use std::collections::HashMap; + + let unmapped = "gpt-4.1-bogus-qa-probe"; + let endpoint = || SecretFreeEndpoint::new(VERSA_AZURE_ENDPOINT.into()).unwrap(); + let restored = crate::config::with_config_overrides( + HashMap::from([("VERSA_AZURE_API_KEY".into(), "test-key".into())]), + async { + let bound = VersaAzureProvider::build( + ModelConfig::new_or_fail(unmapped), + endpoint(), + None, + VERSA_AZURE_API_VERSION.into(), + VersaAzureCredentialSource::ApiKey, + ) + .expect("binding a model no deployment serves must not fail"); + let binding = bound.restore_binding(); + assert_eq!( + serde_json::to_value(&binding).unwrap()["deployment"], + NO_DEPLOYMENT_ROUTE + ); + let row = + crate::providers::persisted_model_config_from_binding("versa_azure", binding) + .expect("the marker must pass the binding's own validation"); + let ProviderRestoreBinding::VersaAzure { + model, + endpoint, + deployment, + api_version, + credential_source, + } = PersistedStandaloneProviderBinding::from_model_config(&row) + .unwrap() + .expect("a Versa row carries its exact route") + .into_binding("versa_azure") + .unwrap() + else { + panic!("a Versa binding came back as another provider's"); + }; + VersaAzureProvider::from_resolved( + model, + endpoint, + deployment, + api_version, + credential_source, + ) + .unwrap() + }, + ) + .await; + let error = restored + .chat_completions_path(unmapped) + .expect_err("the restored chat forgot that this model has no deployment"); + assert!(error.to_string().contains("no Versa deployment for model")); + } + + /// `subagent_tool.rs` gives a child another model by rewriting only the + /// binding's MODEL, leaving the parent's route in place. Through the factory + /// path the child really takes, it must still post to its own model's + /// deployment. + #[tokio::test] + async fn a_binding_whose_model_was_rewritten_follows_the_new_model() { + use std::collections::HashMap; + + crate::config::with_config_overrides( + HashMap::from([("VERSA_AZURE_API_KEY".into(), "test-key".into())]), + async { + let parent = VersaAzureProvider::from_resolved( + ModelConfig::new_or_fail(VERSA_AZURE_DEFAULT_MODEL), + SecretFreeEndpoint::new(VERSA_AZURE_ENDPOINT.into()).unwrap(), + VERSA_AZURE_DEFAULT_MODEL.into(), + VERSA_AZURE_API_VERSION.into(), + VersaAzureCredentialSource::ApiKey, + ) + .unwrap(); + let mut binding = parent.restore_binding(); + binding.model_mut().model_name = "gpt-4o-2024-11-20".into(); + let row = + crate::providers::persisted_model_config_from_binding("versa_azure", binding) + .unwrap(); + let child = crate::providers::create_from_persisted("versa_azure", row) + .await + .unwrap(); + assert_eq!(child.get_model_config().model_name, "gpt-4o-2024-11-20"); + assert_eq!( + serde_json::to_value(child.restore_binding()).unwrap()["deployment"], + "gpt-4o-2024-11-20", + "the child kept its parent's deployment" + ); + }, + ) + .await; + } } #[cfg(test)] @@ -805,3 +1158,462 @@ mod shared_namespace_tests { assert_eq!(versa.config_keys[0].name, "VERSA_AZURE_API_KEY"); } } + +/// F1 of the 2026-09-10 QA run: the model a chat names must be the model that +/// answers, or the turn must fail and say so. It was neither — every request +/// posted to gpt-5.5's deployment, and a model that does not exist at all +/// completed a turn normally. +/// +/// Each provider here is built the way production builds it, through +/// `from_env` or `from_resolved`, and only its HTTP client is then pointed at a +/// local stand-in, so every assertion is on the path a real request took. (Both +/// constructors insist on an HTTPS endpoint, which a local server cannot offer; +/// re-aiming the client is the one liberty taken.) +#[cfg(test)] +mod routing_tests { + use super::*; + use crate::providers::errors::ProviderErrorKind; + use std::collections::HashMap; + use wiremock::matchers::method; + use wiremock::{Mock, MockServer, Request, ResponseTemplate}; + + /// The catalog as measured on 2026-09-11 (see `VERSA_AZURE_DEPLOYMENTS`), + /// written out rather than read back from the constant, so changing the + /// catalog is a deliberate edit here as well — re-probe the gateway first. + pub(super) const MEASURED: &[(&str, &str)] = &[ + ("gpt-5.5-2026-04-24", "gpt-5.5-2026-04-24"), + ("gpt-5.4-mini-2026-03-17", "gpt-5.4-mini-2026-03-17"), + ("gpt-5.4-nano-2026-03-17", "gpt-5.4-nano-2026-03-17"), + ("gpt-5.2-2025-12-11", "gpt-5.2-2025-12-11"), + ("gpt-5-2025-08-07", "gpt-5-2025-08-07"), + ("gpt-4.1-2025-04-14", "gpt-4.1-2025-04-14"), + ("gpt-4.1-mini-2025-04-14", "gpt-4.1-mini-2025-04-14"), + ("gpt-4o-2024-11-20", "gpt-4o-2024-11-20"), + ("o4-mini-2025-04-16", "o4-mini-2025-04-16"), + ]; + + /// The QA run's own probe: a deployment that does not exist. + const UNMAPPED: &str = "gpt-4.1-bogus-qa-probe"; + + /// A stand-in gateway. It answers every completion with the deployment named + /// in the request path as `model` — which is what the real one does + /// (measured), and what `token_events.model_id` ends up recording. + async fn gateway() -> MockServer { + let server = MockServer::start().await; + Mock::given(method("POST")) + .respond_with(|request: &Request| { + let deployment = request.url.path().split('/').nth(3).unwrap_or_default(); + ResponseTemplate::new(200).set_body_json(serde_json::json!({ + "id": "chatcmpl-routing-test", + "object": "chat.completion", + "created": 0, + "model": deployment, + "choices": [{ + "index": 0, + "message": {"role": "assistant", "content": "ready"}, + "finish_reason": "stop" + }], + "usage": {"prompt_tokens": 1, "completion_tokens": 1, "total_tokens": 2} + })) + }) + .mount(&server) + .await; + server + } + + async fn requested_paths(server: &MockServer) -> Vec { + server + .received_requests() + .await + .unwrap_or_default() + .iter() + .map(|request| request.url.path().to_string()) + .collect() + } + + /// The `api-version` each request carried, in order. + async fn requested_api_versions(server: &MockServer) -> Vec { + server + .received_requests() + .await + .unwrap_or_default() + .iter() + .filter_map(|request| { + request + .url + .query_pairs() + .find(|(name, _)| name == "api-version") + .map(|(_, version)| version.into_owned()) + }) + .collect() + } + + fn path_of(deployment: &str) -> String { + format!("/openai/deployments/{deployment}/chat/completions") + } + + fn aimed_at(mut provider: VersaAzureProvider, server: &MockServer) -> VersaAzureProvider { + provider.api_client = ApiClient::new( + server.uri(), + AuthMethod::ApiKey { + header_name: "api-key".to_string(), + key: "test-key".to_string(), + }, + ) + .expect("api client builds"); + provider + } + + /// A key, Versa's deployment key as given, and its other two override keys + /// blank — blank is absent — so the machine running the suite cannot leak + /// its own configuration into what is being measured. Plus the public + /// `azure_openai` card's deployment key as given, which `from_env` must not + /// read; the tests that set it are how that is known. + fn config(deployment: &str, public_deployment: &str) -> HashMap { + HashMap::from([ + ("VERSA_AZURE_API_KEY".into(), "test-key".into()), + ("VERSA_AZURE_ENDPOINT".into(), String::new()), + ("VERSA_AZURE_API_VERSION".into(), String::new()), + ("VERSA_AZURE_DEPLOYMENT_NAME".into(), deployment.into()), + ( + "AZURE_OPENAI_DEPLOYMENT_NAME".into(), + public_deployment.into(), + ), + ]) + } + + async fn bound(model: &str, overrides: HashMap) -> VersaAzureProvider { + crate::config::with_config_overrides( + overrides, + VersaAzureProvider::from_env(ModelConfig::new_or_fail(model)), + ) + .await + .unwrap_or_else(|e| panic!("binding {model} must never fail for want of a route: {e}")) + } + + /// A provider rebuilt from a restore binding that stored `deployment`. + async fn restored(model: &str, deployment: &str) -> VersaAzureProvider { + crate::config::with_config_overrides(config("", ""), async { + VersaAzureProvider::from_resolved( + ModelConfig::new_or_fail(model), + SecretFreeEndpoint::new(VERSA_AZURE_ENDPOINT.into()).unwrap(), + deployment.into(), + VERSA_AZURE_API_VERSION.into(), + VersaAzureCredentialSource::ApiKey, + ) + }) + .await + .unwrap_or_else(|e| panic!("a restore of {model} must never fail for want of a route: {e}")) + } + + fn prompt() -> Vec { + vec![Message::user().with_text("Reply with the single word ready.")] + } + + /// The model the stand-in says answered — what `token_events.model_id` would + /// record. + async fn answered_by(provider: &VersaAzureProvider) -> Result { + provider + .complete("system", &prompt(), &[]) + .await + .map(|(_, usage)| usage.model) + } + + #[tokio::test] + async fn each_catalog_model_posts_to_its_own_deployment() { + let server = gateway().await; + for (model, _) in MEASURED { + let provider = aimed_at(bound(model, config("", "")).await, &server); + let answered = answered_by(&provider) + .await + .unwrap_or_else(|e| panic!("{model}: {e}")); + assert_eq!( + answered, *model, + "the chat named {model}; another deployment answered" + ); + } + let expected: Vec = MEASURED.iter().map(|(_, d)| path_of(d)).collect(); + assert_eq!(requested_paths(&server).await, expected); + } + + /// `complete_fast` hands the FAST model to `complete_with_model`, so the + /// deployment has to follow the model a request names, not the one the chat + /// was bound to. It used to land on the chat's deployment too. + #[tokio::test] + async fn a_request_posts_to_the_deployment_of_the_model_it_names() { + let server = gateway().await; + let provider = aimed_at(bound("gpt-5.5-2026-04-24", config("", "")).await, &server); + let (_, usage) = provider + .complete_with_model( + &ModelConfig::new_or_fail("gpt-5.4-mini-2026-03-17"), + "system", + &prompt(), + &[], + ) + .await + .unwrap(); + assert_eq!(usage.model, "gpt-5.4-mini-2026-03-17"); + assert_eq!( + requested_paths(&server).await, + vec![path_of("gpt-5.4-mini-2026-03-17")] + ); + } + + /// The QA probe itself. Refused readably, and NOTHING reaches the gateway — + /// where it used to complete normally on gpt-5.5. + #[tokio::test] + async fn an_unmapped_model_is_refused_before_any_request_is_sent() { + let server = gateway().await; + let provider = aimed_at(bound(UNMAPPED, config("", "")).await, &server); + + let completed = answered_by(&provider) + .await + .expect_err("a model no deployment serves was answered"); + let streamed = match provider.stream("system", &prompt(), &[]).await { + Ok(_) => panic!("a model no deployment serves was streamed"), + Err(error) => error, + }; + for error in [&completed, &streamed] { + let text = error.to_string(); + assert!( + text.contains(&format!("no Versa deployment for model `{UNMAPPED}`")), + "{text}" + ); + for (model, _) in MEASURED { + assert!( + text.contains(model), + "the refusal must offer {model}: {text}" + ); + } + assert_eq!(error.kind(), ProviderErrorKind::ModelUnavailable, "{text}"); + assert!( + !crate::agents::mistakes::is_recoverable(error), + "a retry can never succeed, so the turn must stop on the first one: {text}" + ); + } + assert!( + requested_paths(&server).await.is_empty(), + "a refused turn reached the gateway" + ); + + // Positive control: the same stand-in DOES see a catalog model, so the + // silence above is the refusal and not a server nobody could reach. + let control = aimed_at(bound("gpt-5.5-2026-04-24", config("", "")).await, &server); + answered_by(&control).await.unwrap(); + assert_eq!( + requested_paths(&server).await, + vec![path_of("gpt-5.5-2026-04-24")] + ); + } + + /// The onboarding card upserts `VERSA_AZURE_DEPLOYMENT_NAME` with the + /// shipped default on every connect, and until 2026-09-03 the setup form + /// wrote the legacy key with the default of its day. Neither was a choice, + /// so neither may pin the chat to one model. + #[tokio::test] + async fn a_persisted_default_deployment_does_not_pin_the_model() { + let server = gateway().await; + for overrides in [ + config("gpt-5.5-2026-04-24", ""), + config("", "gpt-5.2-2025-12-11"), + config("gpt-5.5-2026-04-24", "gpt-5.2-2025-12-11"), + ] { + let provider = aimed_at(bound("gpt-4.1-2025-04-14", overrides).await, &server); + assert_eq!(answered_by(&provider).await.unwrap(), "gpt-4.1-2025-04-14"); + } + // Nor is one of them licence to answer a model no deployment serves. + let probe = aimed_at( + bound(UNMAPPED, config("gpt-5.5-2026-04-24", "")).await, + &server, + ); + assert!(answered_by(&probe).await.is_err()); + assert_eq!( + requested_paths(&server).await, + vec![path_of("gpt-4.1-2025-04-14"); 3] + ); + } + + /// `AZURE_OPENAI_DEPLOYMENT_NAME` belongs to the PUBLIC `azure_openai` + /// provider: it is the one key that card requires and ships no default for, + /// so whoever set it up typed a deployment on THEIR Azure resource into it. + /// Versa used to read it as a fallback, and a name the catalog does not know + /// then served every Versa request. None of these may route one — each + /// fails differently at the real gateway, so each is here. A catalog name in + /// that key was never an override; for those see + /// `a_persisted_default_deployment_does_not_pin_the_model`. + #[tokio::test] + async fn the_public_azure_cards_deployment_never_routes_a_versa_request() { + let server = gateway().await; + let public_deployments = [ + // A company deployment: DeploymentNotFound on every Versa turn. + "my-gpt4o", + // Azure's habit of naming a deployment after its model. The short + // aliases are DeploymentNotFound at the gateway (measured). + "gpt-4o", + // A real UCSF deployment the catalog does not offer: the gateway + // ANSWERS, so the wrong model replies and nothing says so — F1. + "gpt-5-mini-2025-08-07", + ]; + for public_deployment in public_deployments { + let chat = aimed_at( + bound("gpt-4.1-2025-04-14", config("", public_deployment)).await, + &server, + ); + let answered = answered_by(&chat).await.unwrap(); + assert_eq!( + requested_paths(&server).await.pop().unwrap_or_default(), + path_of("gpt-4.1-2025-04-14"), + "the public azure_openai card's deployment `{public_deployment}` routed a \ + Versa request for gpt-4.1-2025-04-14" + ); + assert_eq!(answered, "gpt-4.1-2025-04-14"); + // The route a reopened chat reuses is the model's own too, so the + // other card's value cannot be persisted into the session row. + assert_eq!( + serde_json::to_value(chat.restore_binding()).unwrap()["deployment"], + "gpt-4.1-2025-04-14", + "`{public_deployment}` was written into the restore binding" + ); + + // Nor may it rescue a model no deployment serves. + let probe = aimed_at( + bound(UNMAPPED, config("", public_deployment)).await, + &server, + ); + assert!( + answered_by(&probe).await.is_err(), + "`{public_deployment}` answered for a model no Versa deployment serves" + ); + } + assert_eq!( + requested_paths(&server).await, + vec![path_of("gpt-4.1-2025-04-14"); public_deployments.len()] + ); + } + + /// The public card's other two keys. Wherever `VERSA_AZURE_ENDPOINT` was + /// blank, its endpoint used to become Versa's, so a company's Azure + /// resource received every Versa request — the transcript, with + /// `VERSA_AZURE_API_KEY` in its `api-key` header — refused the key, and the + /// instance turned Public. Its API version rode along on every request to + /// the gateway. + #[tokio::test] + async fn the_public_azure_cards_endpoint_and_api_version_never_reach_versa() { + let server = gateway().await; + let mut public_card = config("", "my-gpt4o"); + public_card.insert( + "AZURE_OPENAI_ENDPOINT".into(), + "https://contoso.openai.azure.com".into(), + ); + // Azure's GA version — and what Versa's own onboarding card wrote into + // this key from 2026-05-30 to 2026-07-02, which is why it is this one. + public_card.insert("AZURE_OPENAI_API_VERSION".into(), "2024-10-21".into()); + + let chat = bound("gpt-4.1-2025-04-14", public_card).await; + let binding = serde_json::to_value(chat.restore_binding()).unwrap(); + assert_eq!( + (&binding["endpoint"], chat.tier(), &binding["api_version"]), + ( + &serde_json::json!(VERSA_AZURE_ENDPOINT), + ProviderTier::Private, + &serde_json::json!(VERSA_AZURE_API_VERSION), + ), + "the public azure_openai card's endpoint or API version reached a Versa chat" + ); + + // And on the wire: the version the gateway was actually sent. + answered_by(&aimed_at(chat, &server)).await.unwrap(); + assert_eq!( + requested_api_versions(&server).await, + vec![VERSA_AZURE_API_VERSION] + ); + } + + /// Every row written before this change stores gpt-5.5's deployment, + /// whatever its model — the QA run read exactly that off a rebound chat. A + /// restore must re-derive the route from the model, not carry it forward. + #[tokio::test] + async fn a_row_written_before_this_change_posts_to_its_own_models_deployment() { + let server = gateway().await; + let rebound = aimed_at( + restored("gpt-4.1-2025-04-14", "gpt-5.5-2026-04-24").await, + &server, + ); + assert_eq!(answered_by(&rebound).await.unwrap(), "gpt-4.1-2025-04-14"); + assert_eq!( + serde_json::to_value(rebound.restore_binding()).unwrap()["deployment"], + "gpt-4.1-2025-04-14", + "the stale route was carried into the next binding" + ); + + let probe = aimed_at(restored(UNMAPPED, "gpt-5.5-2026-04-24").await, &server); + assert!( + answered_by(&probe).await.is_err(), + "the QA probe's own row still answers from gpt-5.5" + ); + assert_eq!( + requested_paths(&server).await, + vec![path_of("gpt-4.1-2025-04-14")] + ); + } + + /// The escape hatch survives: a deployment the catalog does not know serves + /// every request, whatever the model names, and it survives a restore. + #[tokio::test] + async fn an_explicit_override_still_wins_for_every_model() { + let server = gateway().await; + let custom = "ucsf-preview-deployment"; + + let live = aimed_at( + bound("gpt-4.1-2025-04-14", config(custom, "")).await, + &server, + ); + assert_eq!(answered_by(&live).await.unwrap(), custom); + // Versa's own key is the one way to set it, and the public card's key + // naming some other deployment does not compete with it. + let beside_public = aimed_at( + bound("gpt-4.1-2025-04-14", config(custom, "my-gpt4o")).await, + &server, + ); + assert_eq!(answered_by(&beside_public).await.unwrap(), custom); + // An override is the operator saying where requests go, so it serves a + // model the catalog does not list as well. + let unlisted = aimed_at(bound(UNMAPPED, config(custom, "")).await, &server); + assert_eq!(answered_by(&unlisted).await.unwrap(), custom); + + let binding = serde_json::to_value(live.restore_binding()).unwrap(); + assert_eq!(binding["deployment"], custom); + let restored = aimed_at(restored("gpt-4.1-2025-04-14", custom).await, &server); + assert_eq!(answered_by(&restored).await.unwrap(), custom); + assert_eq!(requested_paths(&server).await, vec![path_of(custom); 4]); + } + + /// The advertised catalog is exactly the measured one, and nothing outside + /// it can be chosen. + #[test] + fn the_advertised_catalog_is_exactly_the_measured_deployments() { + let metadata = VersaAzureProvider::metadata(); + let advertised: Vec<&str> = metadata + .known_models + .iter() + .map(|model| model.name.as_str()) + .collect(); + let measured: Vec<&str> = MEASURED.iter().map(|(model, _)| *model).collect(); + assert_eq!( + advertised, measured, + "the advertised catalog changed; re-probe the gateway and update MEASURED" + ); + assert_eq!(metadata.default_model, "gpt-5.5-2026-04-24"); + assert!(measured.contains(&metadata.default_model.as_str())); + assert!( + !metadata.allows_unlisted_models, + "\"Enter a model not listed...\" would offer a model that can only be refused" + ); + for model in measured { + assert!( + ModelConfig::has_declared_context_window(model), + "{model} has no MODEL_CONTEXT_WINDOWS entry of its own" + ); + } + } +} diff --git a/crates/biorouter/src/providers/versa_bedrock.rs b/crates/biorouter/src/providers/versa_bedrock.rs index 3fbf003d9..0a78d9c76 100644 --- a/crates/biorouter/src/providers/versa_bedrock.rs +++ b/crates/biorouter/src/providers/versa_bedrock.rs @@ -80,9 +80,8 @@ pub struct VersaBedrockProvider { #[serde(skip)] name: String, /// The endpoint this instance resolved at construction. `tier()` reads it, - /// never the provider's name — the last fallback in the chain below is - /// `AWS_ENDPOINT_URL_BEDROCK_RUNTIME`, which `bedrock.rs` sets - /// process-globally with `std::env::set_var`. + /// never the provider's name — `VERSA_BEDROCK_ENDPOINT` is user-writable, so + /// an instance can resolve somewhere that is not the UCSF gateway. #[serde(skip)] resolved_endpoint: String, #[serde(skip)] @@ -95,33 +94,42 @@ impl VersaBedrockProvider { pub async fn from_env(model: ModelConfig) -> Result { let config = crate::config::Config::global(); - // Endpoint: configurable, but always falls back to the UCSF MuleSoft proxy - // so a fresh install with just the key + secret works out of the box. + // Overrides come from this provider's OWN namespace and nowhere else. A + // blank value is absent, and absent is the UCSF gateway, so a fresh + // install with just the key and secret works out of the box. + // + // ⚠ Not `AWS_ENDPOINT_URL_BEDROCK` or `AWS_REGION`, and not the process + // environment. The `AWS_*` namespace is the public `aws_bedrock` card's: + // it declares `AWS_REGION`, and `bedrock.rs` exports every `AWS_*` + // config value and secret into the environment. Sharing it went wrong in + // both directions. Versa read those two keys, then + // `AWS_ENDPOINT_URL_BEDROCK` and `AWS_ENDPOINT_URL_BEDROCK_RUNTIME` from + // the environment, as fallbacks. So the public card's region, or an + // endpoint left by its export or by a shell, steered Versa: UCSF-issued + // keys signed requests for someone's own AWS region, which refused them, + // and the instance turned Public. Versa's setup also WROTE both keys, so + // connecting it marked the public card Configured, and handed it UCSF's + // gateway as an endpoint. The fallbacks are gone (2026-09-11). + // + // Where nothing was set, every Versa setup surface prefilled the shipped + // defaults, and neither default has changed: the region has been + // us-west-2 since Versa Bedrock shipped (2026-05-07), and the endpoint + // has been UCSF's gateway since it became configurable (2026-05-12). So a + // value this drops was either typed over that prefill or came from the + // public side, and the second is the bug itself. + // + // ⚠ Each key is a STRING LITERAL passed straight to `get_param`, as in + // `versa_azure`, because `privacy::config_keys` scans this file for them. let endpoint_url: String = config - .get_param::("AWS_ENDPOINT_URL_BEDROCK") + .get_param::("VERSA_BEDROCK_ENDPOINT") .ok() .filter(|s| !s.trim().is_empty()) - .or_else(|| { - std::env::var("AWS_ENDPOINT_URL_BEDROCK") - .ok() - .filter(|s| !s.trim().is_empty()) - }) - .or_else(|| { - std::env::var("AWS_ENDPOINT_URL_BEDROCK_RUNTIME") - .ok() - .filter(|s| !s.trim().is_empty()) - }) .unwrap_or_else(|| VERSA_BEDROCK_DEFAULT_ENDPOINT.to_string()); let region: String = config - .get_param::("AWS_REGION") + .get_param::("VERSA_BEDROCK_REGION") .ok() .filter(|s| !s.trim().is_empty()) - .or_else(|| { - std::env::var("AWS_REGION") - .ok() - .filter(|s| !s.trim().is_empty()) - }) .unwrap_or_else(|| VERSA_BEDROCK_DEFAULT_REGION.to_string()); let retry_config = Self::load_retry_config(config); @@ -198,6 +206,17 @@ impl VersaBedrockProvider { Credentials::new(access_key_id, secret_access_key, None, None, "VersaBedrock"); let loader = aws_config::defaults(aws_config::BehaviorVersion::latest()) .credentials_provider(credentials) + // ⚠ SigV4 with the credentials above, chosen in code. The AWS SDK + // reads `AWS_BEARER_TOKEN_BEDROCK` from the process environment by + // itself, and unless the auth scheme was chosen in code it then + // authenticates with that bearer token instead of signing. That + // variable is where AWS tells a user to put a Bedrock API key, i.e. + // the PUBLIC card's credential. Without this line a Versa chat that + // looked entirely right (UCSF gateway, us-west-2, Private) sent the + // public card's API key to UCSF, and Versa's own keys signed + // nothing. A preference set on this loader counts as chosen in code + // (`Origin::is_client_config`), so the SDK leaves it alone. + .auth_scheme_preference(["sigv4".into()]) .region(aws_config::Region::new(region.clone())) .endpoint_url(endpoint.as_str()); #[cfg(test)] @@ -238,6 +257,20 @@ impl VersaBedrockProvider { }) } + /// The same client with only its HTTP transport replaced. Everything the + /// constructor resolved — endpoint, region, credentials, auth scheme — is + /// kept, so a request captured through it is the request production would + /// have sent. + #[cfg(test)] + pub(crate) fn with_http_client( + mut self, + http_client: impl aws_sdk_bedrockruntime::config::HttpClient + 'static, + ) -> Self { + let config = self.client.config().to_builder().http_client(http_client); + self.client = Client::from_conf(config.build()); + self + } + fn load_retry_config(config: &crate::config::Config) -> RetryConfig { let max_retries = config .get_param::("BEDROCK_MAX_RETRIES") @@ -367,21 +400,24 @@ impl Provider for VersaBedrockProvider { VERSA_BEDROCK_DEFAULT_MODEL, models, VERSA_BEDROCK_DOC_LINK, + // ⚠ The key and secret, and NOTHING else, as the description above + // says. This used to declare `AWS_ENDPOINT_URL_BEDROCK` and + // `AWS_REGION`, and the setup form persists a declared key's default + // (DefaultProviderSetupForm seeds it as a value; DefaultSubmitHandler + // submits it). `AWS_REGION` is one of the two keys the PUBLIC + // `aws_bedrock` card declares, both required and both defaulted, so + // `check_provider_configured` calls that card Configured once either + // is in `config.yaml`: setting up UCSF's private Versa lit up the + // public, commercial Amazon Bedrock card. `versa_azure` had the same + // defect with the public Azure card (2026-09-03). + // + // Dropping them costs nothing. An install that sets nothing still + // reaches the UCSF gateway through the constants above, and an + // operator overrides through Versa's own `VERSA_BEDROCK_*` keys (see + // `from_env`). vec![ ConfigKey::new("VERSA_BEDROCK_ACCESS_KEY_ID", true, true, None), ConfigKey::new("VERSA_BEDROCK_SECRET_ACCESS_KEY", true, true, None), - ConfigKey::new( - "AWS_ENDPOINT_URL_BEDROCK", - false, - false, - Some(VERSA_BEDROCK_DEFAULT_ENDPOINT), - ), - ConfigKey::new( - "AWS_REGION", - false, - false, - Some(VERSA_BEDROCK_DEFAULT_REGION), - ), ], ) .with_unlisted_models() @@ -752,10 +788,9 @@ mod tests { /// `tier_tests.rs`, but a test of the predicate alone cannot see whether /// this provider calls it, or hands it the right field. Replace the body of /// `tier()` with an unconditional `Private` and every one of those tests - /// still passes. This one does not — and the demotion matters most here, - /// because the last fallback in `from_env`'s endpoint chain is - /// `AWS_ENDPOINT_URL_BEDROCK_RUNTIME`, which `bedrock.rs` sets - /// **process-globally** with `std::env::set_var`. + /// still passes. This one does not. The demotion is still needed although + /// `from_env` no longer falls back to the public side's keys: + /// `VERSA_BEDROCK_ENDPOINT` is user-writable config. #[tokio::test] async fn tier_follows_the_endpoint_this_instance_resolved() { let shipped = provider_at(VERSA_BEDROCK_DEFAULT_ENDPOINT).await; @@ -773,12 +808,9 @@ mod tests { } /// DR-26 (Task 46) rule, **wired** — the same argument as the tier test - /// above, for the third axis, and it matters most here: the last fallback in - /// `from_env`'s endpoint chain is `AWS_ENDPOINT_URL_BEDROCK_RUNTIME`, which - /// `bedrock.rs` sets **process-globally** with `std::env::set_var`. An - /// affiliation keyed on the provider's name would keep claiming `ucsf` for - /// an instance that another provider's construction had already repointed at - /// a plain AWS region. + /// above, for the third axis. `VERSA_BEDROCK_ENDPOINT` is user-writable, and + /// an affiliation keyed on the provider's name would keep claiming `ucsf` + /// for an instance repointed at a plain AWS region. #[tokio::test] async fn affiliation_follows_the_endpoint_this_instance_resolved() { use crate::privacy::affiliation::{InstitutionId, ModelAffiliation}; diff --git a/crates/biorouter/tests/versa_gpt_stream_probe.rs b/crates/biorouter/tests/versa_gpt_stream_probe.rs index 48d72109c..c2297ed17 100644 --- a/crates/biorouter/tests/versa_gpt_stream_probe.rs +++ b/crates/biorouter/tests/versa_gpt_stream_probe.rs @@ -1,6 +1,6 @@ //! Opt-in synthetic provider/decoder probe; returned tools are never executed. //! Requires BIOROUTER_RUN_VERSA_GPT_PROBE=1, BIOROUTER_VERSA_GPT_PROBE_MODEL, -//! matching AZURE_OPENAI_DEPLOYMENT_NAME, AZURE_OPENAI_ENDPOINT, and a NEW +//! matching VERSA_AZURE_DEPLOYMENT_NAME, VERSA_AZURE_ENDPOINT, and a NEW //! BIOROUTER_VERSA_GPT_PROBE_OUTPUT directly under /tmp. CASE defaults to todo; //! BIOROUTER_VERSA_GPT_PROBE_CASE=python_sqlite requests a medium file instead. //! gpt-5.6 is an unverified requested deployment, not a claim of availability. @@ -186,10 +186,10 @@ async fn manual_versa_gpt_stream_probe() { ); let model = approved_model( &required("BIOROUTER_VERSA_GPT_PROBE_MODEL"), - &required("AZURE_OPENAI_DEPLOYMENT_NAME"), + &required("VERSA_AZURE_DEPLOYMENT_NAME"), ) .unwrap_or_else(|class| panic!("{class}")); - approved_endpoint(&required("AZURE_OPENAI_ENDPOINT")).unwrap_or_else(|class| panic!("{class}")); + approved_endpoint(&required("VERSA_AZURE_ENDPOINT")).unwrap_or_else(|class| panic!("{class}")); let case = std::env::var("BIOROUTER_VERSA_GPT_PROBE_CASE").unwrap_or_else(|_| "todo".into()); synthetic_case(&case).unwrap_or_else(|class| panic!("{class}")); let path = required("BIOROUTER_VERSA_GPT_PROBE_OUTPUT"); diff --git a/crates/biorouter/tests/versa_stream_wire_probe.rs b/crates/biorouter/tests/versa_stream_wire_probe.rs index d7490cc7a..f6d403e63 100644 --- a/crates/biorouter/tests/versa_stream_wire_probe.rs +++ b/crates/biorouter/tests/versa_stream_wire_probe.rs @@ -145,11 +145,12 @@ fn transport_http1_only(mode: &str) -> ProbeResult { impl Settings { fn load() -> ProbeResult { let config = Config::global(); - let endpoint = configured_string(config, "AWS_ENDPOINT_URL_BEDROCK") - .or_else(|| nonempty_env("AWS_ENDPOINT_URL_BEDROCK_RUNTIME")) + // Versa's own keys only, as `VersaBedrockProvider::from_env` reads them: + // the `AWS_*` ones belong to the public Amazon Bedrock card. + let endpoint = configured_string(config, "VERSA_BEDROCK_ENDPOINT") .unwrap_or_else(|| VERSA_BEDROCK_DEFAULT_ENDPOINT.into()); validate_endpoint(&endpoint)?; - let region = configured_string(config, "AWS_REGION") + let region = configured_string(config, "VERSA_BEDROCK_REGION") .unwrap_or_else(|| VERSA_BEDROCK_DEFAULT_REGION.into()); if region.len() > 32 || !region diff --git a/docs/agent-drafter/testing/app-test-drive-runbook.md b/docs/agent-drafter/testing/app-test-drive-runbook.md index db4db59c4..045a9873f 100644 --- a/docs/agent-drafter/testing/app-test-drive-runbook.md +++ b/docs/agent-drafter/testing/app-test-drive-runbook.md @@ -117,8 +117,10 @@ print('wrote', envf, '(len', len(key), ')') PY ``` -The provider env vars (`AZURE_OPENAI_ENDPOINT`, deployment, api-version) are already in -`~/.config/biorouter/config.yaml`; only the key needs supplying. +Only the key needs supplying: `versa_azure`'s endpoint, deployment and API version are compiled +in, and `VERSA_AZURE_ENDPOINT` / `_DEPLOYMENT_NAME` / `_API_VERSION` in +`~/.config/biorouter/config.yaml` override them. It does not read the public Azure provider's +`AZURE_OPENAI_*` keys, so those change nothing here. ### 1.5 Start the daemon diff --git a/docs/security/privacy-tiers.md b/docs/security/privacy-tiers.md index 8153afa87..853be5f09 100644 --- a/docs/security/privacy-tiers.md +++ b/docs/security/privacy-tiers.md @@ -677,6 +677,21 @@ reads `AZURE_OPENAI_ENDPOINT` / `AZURE_OPENAI_DEPLOYMENT_NAME` / `AZURE_OPENAI_A compiled-in UCSF gateway (`unified-api.ucsf.edu`), computed at construction when the endpoint is already resolved. +> **Update (2026-09-11).** `versa_azure` now reads only its own `VERSA_AZURE_ENDPOINT` / +> `_DEPLOYMENT_NAME` / `_API_VERSION`, so the shared-key half of this hazard is closed at the source: +> whatever the public `azure_openai` card is set up with no longer reaches it. The demotion rule is +> unchanged and still needed, because `VERSA_AZURE_ENDPOINT` is user-writable config. +> +> **Update (2026-09-11), Bedrock.** `versa_bedrock` now reads only its own `VERSA_BEDROCK_ENDPOINT` / +> `VERSA_BEDROCK_REGION`, with no fallback to an `AWS_*` key or to the process environment, and +> `bedrock.rs` no longer promotes `AWS_ENDPOINT_URL_BEDROCK` to `AWS_ENDPOINT_URL_BEDROCK_RUNTIME`. +> That closes the fallback named above in both directions. The shared namespace also hid a crossing +> no endpoint check can see: the AWS SDK reads `AWS_BEARER_TOKEN_BEDROCK` from the environment itself +> and authenticated Versa's requests with that token. So an instance that resolved the gateway, and +> was rightly Private, carried the public card's Bedrock API key to UCSF. `versa_bedrock` now chooses +> SigV4 in code. The demotion rule is unchanged and still needed, because `VERSA_BEDROCK_ENDPOINT` +> is user-writable config. + **Never keyed on a model id.** `us.anthropic.claude-opus-4-8` appears in both `BEDROCK_KNOWN_MODELS` and `VERSA_BEDROCK_KNOWN_MODELS`. Any model-name badge is wrong by construction. diff --git a/docs/testing/process-global-state.md b/docs/testing/process-global-state.md index 1338ed11a..2dabb0607 100644 --- a/docs/testing/process-global-state.md +++ b/docs/testing/process-global-state.md @@ -69,18 +69,19 @@ The four highest-traffic literal keys are `BIOROUTER_PATH_ROOT` (6 sites), `PATH ### Production code that writes the environment -Five sites, and they are not a test concern — they mutate the environment of a multi-threaded daemon. +Four sites, and they are not a test concern — they mutate the environment of a multi-threaded daemon. | Site | What it writes | |---|---| | `providers/bedrock.rs:79` | every `AWS_*` config value **and secret**, from `from_env` | -| `providers/bedrock.rs:90-92` | an unlocked read-then-write promoting `AWS_ENDPOINT_URL_BEDROCK` | | `providers/sagemaker_tgi.rs:52` | the same `AWS_*` dump | | `config/base.rs:1504` | `BIOROUTER_DISABLE_KEYRING=1` on keyring fallback; races `Config::default`'s read at `:225`, which `GLOBAL_CONFIG` then freezes | | `agents/test_sandbox.rs:37` | safe twice over — a `#[ctor]` that runs before `main`, in a module gated at its declaration site | The two `AWS_*` dumps leak credentials into every subprocess spawned afterwards, which is the exact unsoundness the `CONFIG_OVERRIDES` task-local was introduced to avoid. +A fifth site, an unlocked read-then-write at `providers/bedrock.rs:90-92` that promoted `AWS_ENDPOINT_URL_BEDROCK` to `AWS_ENDPOINT_URL_BEDROCK_RUNTIME`, was removed on 2026-09-11. It is how the UCSF gateway that Versa Bedrock's setup persisted became the public `aws_bedrock` provider's endpoint; see `providers/bedrock_namespace_tests.rs`. + ### Test writers | Shape | Count | @@ -130,6 +131,7 @@ Verdicts: **fixed**, **live** (a reader can observe another test's write today), | `pending_user_action::USER_PROOF_AVAILABLE` | 6 lib readers | none in `--lib` | **latent** | | `SkillsClient::new` resolving `Paths::config_dir()` | `agents/skills_extension.rs:807` | — | **accepted** — PR #193 calls the synchronous constructor read "the property we want": the root a client seeds into is the one that was ambient when it was built | | `AWS_*` written by production | any `env::var` reader in the process | `providers/bedrock.rs:79`, `sagemaker_tgi.rs:52` | **open** — not a test hazard; recorded here because it is the same mechanism | +| `AWS_BEARER_TOKEN_BEDROCK` | the AWS SDK itself: Bedrock Runtime's `From<&SdkConfig>` reads it and prefers bearer auth unless the auth scheme was chosen in code, reached from `VersaBedrockProvider::from_resolved` | a shell, or `providers/bedrock.rs:79`'s export | **fixed** 2026-09-11 — Versa's loader chooses SigV4 in code. A reader inside a dependency is invisible to every `env::var` scan in this document. | ### `/skills`, spelled eleven ways diff --git a/ui/desktop/src/components/onboarding/InstitutionalSetupCard.test.tsx b/ui/desktop/src/components/onboarding/InstitutionalSetupCard.test.tsx index e473575fa..847a9befc 100644 --- a/ui/desktop/src/components/onboarding/InstitutionalSetupCard.test.tsx +++ b/ui/desktop/src/components/onboarding/InstitutionalSetupCard.test.tsx @@ -27,6 +27,17 @@ async function connectVersaAzure() { await waitFor(() => expect(mockCheckProvider).toHaveBeenCalled()); } +async function connectVersaBedrock() { + const onSuccess = vi.fn(); + render(); + fireEvent.click(screen.getByRole('tab', { name: /Bedrock/i })); + fireEvent.change(screen.getByLabelText(/Access Key ID/i), { target: { value: 'an-id' } }); + fireEvent.change(screen.getByLabelText(/Secret Access Key/i), { target: { value: 'a-secret' } }); + fireEvent.click(screen.getByRole('button', { name: /Connect to Versa Bedrock/i })); + // Past `checkProvider`, so every write the connect makes has been recorded. + await waitFor(() => expect(onSuccess).toHaveBeenCalledWith('versa_bedrock')); +} + describe('InstitutionalSetupCard', () => { beforeEach(() => { vi.clearAllMocks(); @@ -55,4 +66,27 @@ describe('InstitutionalSetupCard', () => { expect(written).toContain('VERSA_AZURE_DEPLOYMENT_NAME'); expect(written).toContain('VERSA_AZURE_API_VERSION'); }); + + it('never writes a key in the public AWS namespace when connecting UCSF Versa Bedrock', async () => { + // Connecting UCSF's PRIVATE Versa Bedrock used to write `AWS_REGION` and + // `AWS_ENDPOINT_URL_BEDROCK`. The public `aws_bedrock` card declares + // `AWS_REGION`, so the write marked that card Configured and replaced its + // region; and `bedrock.rs` exports every `AWS_*` key into the process + // environment, which is how the UCSF gateway became the public provider's + // endpoint. + await connectVersaBedrock(); + const written = mockUpsert.mock.calls.map((c) => c[0] as string); + expect(written.filter((key) => key.startsWith('AWS_'))).toEqual([]); + }); + + it('writes the Versa Bedrock credentials and overrides, then selects the provider', async () => { + await connectVersaBedrock(); + expect(mockUpsert.mock.calls).toEqual([ + ['VERSA_BEDROCK_ACCESS_KEY_ID', 'an-id', true], + ['VERSA_BEDROCK_SECRET_ACCESS_KEY', 'a-secret', true], + ['VERSA_BEDROCK_ENDPOINT', 'https://unified-api.ucsf.edu/general/awsai', false], + ['VERSA_BEDROCK_REGION', 'us-west-2', false], + ['BIOROUTER_PROVIDER', 'versa_bedrock', false], + ]); + }); }); diff --git a/ui/desktop/src/components/onboarding/InstitutionalSetupCard.tsx b/ui/desktop/src/components/onboarding/InstitutionalSetupCard.tsx index 3a6739f9f..a297f40be 100644 --- a/ui/desktop/src/components/onboarding/InstitutionalSetupCard.tsx +++ b/ui/desktop/src/components/onboarding/InstitutionalSetupCard.tsx @@ -15,9 +15,13 @@ interface InstitutionalSetupCardProps { type VersaFlavor = 'azure' | 'bedrock'; +// Versa Bedrock's own keys, the only ones `versa_bedrock.rs` reads. This card +// used to write `AWS_ENDPOINT_URL_BEDROCK` and `AWS_REGION`, which belong to the +// PUBLIC Amazon Bedrock card: the write marked that card Configured and replaced +// its region, and the public provider took UCSF's gateway as its endpoint. const VERSA_BEDROCK_DEFAULTS = { - AWS_ENDPOINT_URL_BEDROCK: 'https://unified-api.ucsf.edu/general/awsai', - AWS_REGION: 'us-west-2', + VERSA_BEDROCK_ENDPOINT: 'https://unified-api.ucsf.edu/general/awsai', + VERSA_BEDROCK_REGION: 'us-west-2', }; const VERSA_AZURE_DEFAULTS = { @@ -61,9 +65,9 @@ export default function InstitutionalSetupCard({ const [bedrockSecretKey, setBedrockSecretKey] = useState(''); const [azureApiKey, setAzureApiKey] = useState(''); const [bedrockEndpoint, setBedrockEndpoint] = useState( - VERSA_BEDROCK_DEFAULTS.AWS_ENDPOINT_URL_BEDROCK + VERSA_BEDROCK_DEFAULTS.VERSA_BEDROCK_ENDPOINT ); - const [bedrockRegion, setBedrockRegion] = useState(VERSA_BEDROCK_DEFAULTS.AWS_REGION); + const [bedrockRegion, setBedrockRegion] = useState(VERSA_BEDROCK_DEFAULTS.VERSA_BEDROCK_REGION); const [azureEndpoint, setAzureEndpoint] = useState(VERSA_AZURE_DEFAULTS.VERSA_AZURE_ENDPOINT); const [azureDeployment, setAzureDeployment] = useState( VERSA_AZURE_DEFAULTS.VERSA_AZURE_DEPLOYMENT_NAME @@ -91,8 +95,8 @@ export default function InstitutionalSetupCard({ if (flavor === 'bedrock') { await upsert('VERSA_BEDROCK_ACCESS_KEY_ID', bedrockAccessKey.trim(), true); await upsert('VERSA_BEDROCK_SECRET_ACCESS_KEY', bedrockSecretKey.trim(), true); - await upsert('AWS_ENDPOINT_URL_BEDROCK', bedrockEndpoint.trim(), false); - await upsert('AWS_REGION', bedrockRegion.trim(), false); + await upsert('VERSA_BEDROCK_ENDPOINT', bedrockEndpoint.trim(), false); + await upsert('VERSA_BEDROCK_REGION', bedrockRegion.trim(), false); await checkProvider({ body: { provider: 'versa_bedrock' }, throwOnError: true }); await upsert('BIOROUTER_PROVIDER', 'versa_bedrock', false); onSuccess('versa_bedrock'); @@ -263,7 +267,7 @@ export default function InstitutionalSetupCard({ <>
- + > = { AZURE_OPENAI_DEPLOYMENT_NAME: 'gpt-5.5-2026-04-24', AZURE_OPENAI_API_VERSION: '2025-01-01-preview', }, - versa_bedrock: { - AWS_ENDPOINT_URL_BEDROCK: 'https://unified-api.ucsf.edu/general/awsai', - AWS_REGION: 'us-west-2', - }, }; const envToPrettyName = (envVar: string) => {