swarm-ui: set the review classifier's keys from the classifier settings
The classifier's Anthropic and ACP provider keys were written by hand in the
secret store's UI. The controller now stores them: GET
/api/review-classifier/keys says whether each is set and when it changed, and
PUT /api/review-classifier/keys/{runtime} writes one. No route answers a key.
The controller's store policy gains create and update on the two leaves with
no read of their values, and a metadata read for the set state. The swarm UI's
classifier settings card gets a key form beside the settings, saved apart
because it goes to the secret store.
This commit is contained in:
parent
c5d601e3a2
commit
6875a4f0f8
3 changed files with 628 additions and 20 deletions
|
|
@ -40,6 +40,7 @@ use self::settings::{Settings, SettingsSource};
|
|||
use crate::forge::{self, DefaultBranchFiles, PullForReview};
|
||||
use crate::routes::{AppState, error_problem};
|
||||
|
||||
pub(crate) mod keys;
|
||||
pub(crate) mod metrics;
|
||||
pub(crate) mod open_pr;
|
||||
pub(crate) mod review;
|
||||
|
|
|
|||
323
swarm-controller/src/review_classifier/keys.rs
Normal file
323
swarm-controller/src/review_classifier/keys.rs
Normal file
|
|
@ -0,0 +1,323 @@
|
|||
//! `GET /api/review-classifier/keys` and `PUT /api/review-classifier/keys/{runtime}`:
|
||||
//! the keys the classifier's two runtimes run on, written from the swarm UI
|
||||
//! instead of by hand in the secret store's UI.
|
||||
//!
|
||||
//! The controller only writes. Its store policy holds `create` and `update`
|
||||
//! on the two leaves and no `read` of their values, so no route here answers a
|
||||
//! key, and the one metadata read tells whether a key is set and when it last
|
||||
//! changed. The classifier reads the key from the same leaf on every run, so
|
||||
//! a new key applies from the next run with no restart.
|
||||
|
||||
use std::future::Future;
|
||||
|
||||
use axum::Json;
|
||||
use axum::extract::rejection::JsonRejection;
|
||||
use axum::extract::{Path, State};
|
||||
use axum::http::{HeaderMap, StatusCode};
|
||||
use problem_details::ProblemDetails;
|
||||
use serde::{Deserialize, Serialize};
|
||||
use swarm_secret_client::{
|
||||
SecretStore,
|
||||
acp::ProviderKey,
|
||||
review_classifier::{ApiKey, acp_provider_key_path, api_key_path},
|
||||
};
|
||||
use utoipa::ToSchema;
|
||||
|
||||
use crate::routes::{AppState, error_problem};
|
||||
|
||||
/// The most bytes a key may have. Provider keys run to a few hundred.
|
||||
const MAX_KEY_BYTES: usize = 4096;
|
||||
|
||||
/// Which runtime a key is for, as the path spells it.
|
||||
#[derive(Debug, Clone, Copy, PartialEq, Eq)]
|
||||
enum Which {
|
||||
/// The Anthropic API key the `api-key` runtime runs `claude` with.
|
||||
ApiKey,
|
||||
/// The provider key the `acp` runtime runs its agent with.
|
||||
Acp,
|
||||
}
|
||||
|
||||
impl Which {
|
||||
fn parse(s: &str) -> Option<Self> {
|
||||
match s {
|
||||
"api-key" => Some(Self::ApiKey),
|
||||
"acp" => Some(Self::Acp),
|
||||
_ => None,
|
||||
}
|
||||
}
|
||||
|
||||
fn path(self) -> Result<String, swarm_secret_client::Error> {
|
||||
match self {
|
||||
Self::ApiKey => api_key_path(),
|
||||
Self::Acp => acp_provider_key_path(),
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
/// Whether one key is stored. Never the key.
|
||||
#[derive(Debug, Clone, PartialEq, Eq, Serialize, ToSchema)]
|
||||
pub(crate) struct KeyState {
|
||||
/// A key is stored.
|
||||
set: bool,
|
||||
/// When the key last changed, as the store spells it (RFC 3339), or
|
||||
/// `null` when none is stored.
|
||||
updated: Option<String>,
|
||||
}
|
||||
|
||||
/// Both keys' state, what both routes answer.
|
||||
#[derive(Debug, Clone, PartialEq, Eq, Serialize, ToSchema)]
|
||||
pub(crate) struct Keys {
|
||||
/// The Anthropic API key of the `api-key` runtime.
|
||||
api_key: KeyState,
|
||||
/// The provider key of the `acp` runtime.
|
||||
acp: KeyState,
|
||||
}
|
||||
|
||||
/// The body of a `PUT`: the key.
|
||||
#[derive(Deserialize, ToSchema)]
|
||||
#[serde(deny_unknown_fields)]
|
||||
pub(crate) struct KeyBody {
|
||||
/// The key. Surrounding whitespace is dropped, so a pasted trailing
|
||||
/// newline is harmless.
|
||||
api_key: String,
|
||||
}
|
||||
|
||||
/// The two store calls a route makes, so a test can stand in for the store.
|
||||
trait KeyStore {
|
||||
/// Store `key` at `path`, replacing what's there.
|
||||
fn put(&self, which: Which, key: &str) -> impl Future<Output = Result<(), String>> + Send;
|
||||
|
||||
/// When the leaf last changed, or `None` when nothing is stored.
|
||||
fn updated(&self, which: Which) -> impl Future<Output = Result<Option<String>, String>> + Send;
|
||||
}
|
||||
|
||||
impl KeyStore for SecretStore {
|
||||
async fn put(&self, which: Which, key: &str) -> Result<(), String> {
|
||||
let path = which.path().map_err(|e| e.to_string())?;
|
||||
let result = match which {
|
||||
Which::ApiKey => {
|
||||
self.write(
|
||||
&path,
|
||||
&ApiKey {
|
||||
api_key: key.to_owned(),
|
||||
},
|
||||
)
|
||||
.await
|
||||
}
|
||||
Which::Acp => {
|
||||
self.write(
|
||||
&path,
|
||||
&ProviderKey {
|
||||
api_key: key.to_owned(),
|
||||
},
|
||||
)
|
||||
.await
|
||||
}
|
||||
};
|
||||
result.map_err(|e| format!("writing {path}: {e}"))
|
||||
}
|
||||
|
||||
async fn updated(&self, which: Which) -> Result<Option<String>, String> {
|
||||
let path = which.path().map_err(|e| e.to_string())?;
|
||||
self.updated_time(&path)
|
||||
.await
|
||||
.map_err(|e| format!("reading {path}'s metadata: {e}"))
|
||||
}
|
||||
}
|
||||
|
||||
/// A key to store, or why it's refused.
|
||||
fn check(key: &str) -> Result<&str, String> {
|
||||
let key = key.trim();
|
||||
if key.is_empty() {
|
||||
return Err("the key is empty".to_owned());
|
||||
}
|
||||
if key.len() > MAX_KEY_BYTES {
|
||||
return Err(format!("the key is over {MAX_KEY_BYTES} bytes"));
|
||||
}
|
||||
if key.chars().any(|c| c.is_whitespace() || c.is_control()) {
|
||||
return Err("the key holds whitespace or a control character".to_owned());
|
||||
}
|
||||
Ok(key)
|
||||
}
|
||||
|
||||
async fn state_of(store: &impl KeyStore, which: Which) -> Result<KeyState, String> {
|
||||
let updated = store.updated(which).await?;
|
||||
Ok(KeyState {
|
||||
set: updated.is_some(),
|
||||
updated,
|
||||
})
|
||||
}
|
||||
|
||||
async fn both(store: &impl KeyStore) -> Result<Keys, String> {
|
||||
Ok(Keys {
|
||||
api_key: state_of(store, Which::ApiKey).await?,
|
||||
acp: state_of(store, Which::Acp).await?,
|
||||
})
|
||||
}
|
||||
|
||||
fn store_problem(why: &str) -> ProblemDetails {
|
||||
tracing::warn!(error = %why, "review classifier keys: store failure");
|
||||
error_problem(
|
||||
StatusCode::SERVICE_UNAVAILABLE,
|
||||
&format!("the secret store refused: {why}"),
|
||||
)
|
||||
}
|
||||
|
||||
async fn connect() -> Result<SecretStore, ProblemDetails> {
|
||||
crate::store::connect()
|
||||
.await
|
||||
.map_err(|e| store_problem(&format!("logging in: {e}")))
|
||||
}
|
||||
|
||||
/// Whether each runtime's key is stored.
|
||||
#[utoipa::path(
|
||||
get,
|
||||
path = "/api/review-classifier/keys",
|
||||
responses(
|
||||
(status = 200, description = "whether each key is stored and when it last changed; \
|
||||
never a key", body = Keys),
|
||||
(status = 503, description = "the secret store couldn't be reached, or refused \
|
||||
(problem+json)", body = String),
|
||||
),
|
||||
tag = "review-classifier"
|
||||
)]
|
||||
pub(crate) async fn get_keys(State(_state): State<AppState>) -> Result<Json<Keys>, ProblemDetails> {
|
||||
let store = connect().await?;
|
||||
both(&store).await.map(Json).map_err(|e| store_problem(&e))
|
||||
}
|
||||
|
||||
/// Store a runtime's key.
|
||||
#[utoipa::path(
|
||||
put,
|
||||
path = "/api/review-classifier/keys/{runtime}",
|
||||
params(("runtime" = String, Path, description = "`api-key` or `acp`")),
|
||||
request_body = KeyBody,
|
||||
responses(
|
||||
(status = 200, description = "both keys' state after the write; never a key",
|
||||
body = Keys),
|
||||
(status = 400, description = "not JSON or not this shape, a runtime other than \
|
||||
`api-key` or `acp`, or a key that is empty, over 4096 bytes or holds \
|
||||
whitespace (problem+json)", body = String),
|
||||
(status = 401, description = "the request carries no `Remote-User`, so there is \
|
||||
no operator to name (problem+json)", body = String),
|
||||
(status = 503, description = "the secret store couldn't be reached, or refused \
|
||||
(problem+json)", body = String),
|
||||
),
|
||||
tag = "review-classifier"
|
||||
)]
|
||||
pub(crate) async fn put_key(
|
||||
State(_state): State<AppState>,
|
||||
headers: HeaderMap,
|
||||
Path(runtime): Path<String>,
|
||||
body: Result<Json<KeyBody>, JsonRejection>,
|
||||
) -> Result<Json<Keys>, ProblemDetails> {
|
||||
let operator = super::operator(&headers).ok_or_else(|| {
|
||||
error_problem(
|
||||
StatusCode::UNAUTHORIZED,
|
||||
"no Remote-User header: the gateway names the operator, and this request did not \
|
||||
come through it",
|
||||
)
|
||||
})?;
|
||||
let which = Which::parse(&runtime).ok_or_else(|| {
|
||||
error_problem(
|
||||
StatusCode::BAD_REQUEST,
|
||||
&format!("{runtime:?} is not a runtime with a key: use `api-key` or `acp`"),
|
||||
)
|
||||
})?;
|
||||
let Json(body) = body.map_err(|e| error_problem(StatusCode::BAD_REQUEST, &e.body_text()))?;
|
||||
let key = check(&body.api_key).map_err(|why| error_problem(StatusCode::BAD_REQUEST, &why))?;
|
||||
let store = connect().await?;
|
||||
store.put(which, key).await.map_err(|e| store_problem(&e))?;
|
||||
// The key itself is never logged.
|
||||
tracing::info!(%operator, ?which, "set a review classifier key");
|
||||
both(&store).await.map(Json).map_err(|e| store_problem(&e))
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
mod tests {
|
||||
use std::sync::Mutex;
|
||||
|
||||
use super::*;
|
||||
|
||||
/// A store holding what was put, with a leaf's time when it has a key.
|
||||
#[derive(Default)]
|
||||
struct Fake {
|
||||
put: Mutex<Vec<(Which, String)>>,
|
||||
refuse: bool,
|
||||
}
|
||||
|
||||
impl KeyStore for Fake {
|
||||
async fn put(&self, which: Which, key: &str) -> Result<(), String> {
|
||||
if self.refuse {
|
||||
return Err("denied".to_owned());
|
||||
}
|
||||
self.put.lock().unwrap().push((which, key.to_owned()));
|
||||
Ok(())
|
||||
}
|
||||
|
||||
async fn updated(&self, which: Which) -> Result<Option<String>, String> {
|
||||
if self.refuse {
|
||||
return Err("denied".to_owned());
|
||||
}
|
||||
let put = self.put.lock().unwrap();
|
||||
Ok(put
|
||||
.iter()
|
||||
.any(|(w, _)| *w == which)
|
||||
.then(|| "2026-11-11T10:00:00Z".to_owned()))
|
||||
}
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn a_runtime_is_one_of_two_words() {
|
||||
assert_eq!(Which::parse("api-key"), Some(Which::ApiKey));
|
||||
assert_eq!(Which::parse("acp"), Some(Which::Acp));
|
||||
assert_eq!(Which::parse("api_key"), None);
|
||||
assert_eq!(Which::parse(""), None);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn the_leaves_are_the_ones_the_policy_grants() {
|
||||
assert_eq!(
|
||||
Which::ApiKey.path().unwrap(),
|
||||
"swarm/services/review-classifier/anthropic-api-key"
|
||||
);
|
||||
assert_eq!(
|
||||
Which::Acp.path().unwrap(),
|
||||
"swarm/services/review-classifier/acp-provider"
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn a_pasted_newline_is_dropped_and_a_bad_key_refused() {
|
||||
assert_eq!(check("sk-ant-x\n"), Ok("sk-ant-x"));
|
||||
assert!(check(" ").is_err());
|
||||
assert!(check("two words").is_err());
|
||||
assert!(check("a\u{0}b").is_err());
|
||||
assert!(check(&"k".repeat(MAX_KEY_BYTES + 1)).is_err());
|
||||
assert!(check(&"k".repeat(MAX_KEY_BYTES)).is_ok());
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn the_state_says_set_once_a_key_is_stored_and_never_holds_it() {
|
||||
let store = Fake::default();
|
||||
let before = both(&store).await.unwrap();
|
||||
assert!(!before.api_key.set && !before.acp.set);
|
||||
assert_eq!(before.api_key.updated, None);
|
||||
|
||||
store.put(Which::ApiKey, "sk-secret").await.unwrap();
|
||||
let after = both(&store).await.unwrap();
|
||||
assert!(after.api_key.set && !after.acp.set);
|
||||
assert!(after.api_key.updated.is_some());
|
||||
let json = serde_json::to_string(&after).unwrap();
|
||||
assert!(!json.contains("sk-secret"), "{json}");
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn a_refusing_store_is_an_error_not_unset() {
|
||||
let store = Fake {
|
||||
refuse: true,
|
||||
..Fake::default()
|
||||
};
|
||||
assert!(both(&store).await.is_err());
|
||||
}
|
||||
}
|
||||
|
|
@ -702,6 +702,8 @@ pub(crate) fn build_app(state: AppState) -> axum::Router {
|
|||
review_classifier::settings::get_settings,
|
||||
review_classifier::settings::put_settings
|
||||
))
|
||||
.routes(routes!(review_classifier::keys::get_keys))
|
||||
.routes(routes!(review_classifier::keys::put_key))
|
||||
.routes(routes!(notify_flows::get_flows, notify_flows::put_flows))
|
||||
.routes(routes!(notify_router_api::get_deliveries))
|
||||
.routes(routes!(
|
||||
|
|
|
|||
Loading…
Reference in a new issue