diff --git a/Cargo.lock b/Cargo.lock index db8ce7f9..85d2b862 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4843,6 +4843,7 @@ dependencies = [ "reqwest", "serde", "serde_json", + "serde_norway", "sha2 0.11.0", "strum", "swarm-authelia-bridge-sock", diff --git a/docs/swarm/review-classifier.md b/docs/swarm/review-classifier.md index ed1053aa..c2fbc746 100644 --- a/docs/swarm/review-classifier.md +++ b/docs/swarm/review-classifier.md @@ -18,23 +18,31 @@ that reviewer. ## Repo configuration -A repo opts in with a config file in a `.hyperhive/` directory at its root, -in YAML or JSON. The classifier always reads it from the repo's **default -branch**, never from the PR, so a PR can't edit the rules it's judged by. +A repo opts in with a rules file at its root, in YAML as +`.hyperhive/review-classifier.yml` or in JSON as +`.hyperhive/review-classifier.json`. It holds one of the two; with both, the +rules don't load. `swarm-controller` always reads the file from the repo's +**default branch**, never from the PR, so a PR can't edit the rules it's +judged by. It reads it with its own forge token on every load, with no cache, +so a rule merged to the default branch counts from the next load on. -Each classification has: +The file holds one key, `classifications`: a list of one to ten entries, each +with exactly these four keys: -- a **name**, short and unique in the repo, shown in the result table -- a **description**: the yes/no question the model answers about the PR -- a **certainty threshold** from 0 to 100 -- a **polarity**: _positive_ means a match is the wanted outcome, and - _negative_ means a match is a violation +- `name`: short and unique in the repo, shown in the result table +- `description`: the yes/no question the model answers about the PR +- `threshold`: the certainty, 0 to 100, a verdict needs to count +- `polarity`: `positive` means a match is the wanted outcome, and + `negative` means a match is a violation -An illustrative example. The file name and every field name in it are -placeholders, not a fixed schema: +Any other key, at the top or in an entry, is an error: a misspelt key fails +the load instead of vanishing. An empty list, more than ten entries, an empty +name or description, and two entries with one name are errors too. Nothing +falls back to a pass: a repo whose file doesn't load has no rules. + +An example: ```yaml -# illustrative only: file name and field names aren't fixed classifications: - name: datetime description: All datetimes carry time zone information. @@ -241,6 +249,24 @@ and sends it to the classifier as one queue request on stays open. The controller keeps the result in memory for an hour; there is no run store. +`GET /api/review-classifier/rules?repo=owner/name` returns the repo's own +rules, for the page to load into its editor; a run posts them like typed +ones. The route answers: + +- **200** with `path` (the file read), `branch` (the default branch), + `commit` (the commit the route read the file at) and `classifications`, in + the file's order and in the POST's shape +- **400** for a `repo` that's missing or not `owner/name` +- **404** for a repo the forge doesn't know, a repo with no commit, or a + default branch with neither file +- **422** for both files on the default branch, or a file that doesn't parse + or breaks a rule from [Repo configuration](#repo-configuration); the + `detail` names the file and what's wrong +- **503** when this host has no forge token yet, or the forge failed or + didn't answer within two minutes + +Every error is problem+json with the reason in `detail`. + When it's ready, **open a PR** has the service account push a branch that adds the classification to the repo's config, with you, the swarm UI user, as commit author. The page then links you to the forge's new-PR page for diff --git a/swarm-controller/Cargo.toml b/swarm-controller/Cargo.toml index c99cb3da..ed93a41f 100644 --- a/swarm-controller/Cargo.toml +++ b/swarm-controller/Cargo.toml @@ -79,6 +79,7 @@ hive-types.workspace = true reqwest = { workspace = true, features = ["blocking"] } serde.workspace = true serde_json.workspace = true +serde_norway = "0.9.42" strum.workspace = true swarm-authelia-bridge-sock.workspace = true # The queue connect (token mint + auth callback + reconnect) is shared with diff --git a/swarm-controller/src/forge.rs b/swarm-controller/src/forge.rs index d408130f..070a6b11 100644 --- a/swarm-controller/src/forge.rs +++ b/swarm-controller/src/forge.rs @@ -25,8 +25,8 @@ use forgejo_api::structs::{ CreateHookOption, CreateHookOptionConfig, CreateHookOptionType, CreateRepoOption, CreateUserOption, EditUserOption, IssueListIssuesQuery, IssueListIssuesQueryState, IssueListIssuesQueryType, RepoDownloadPullDiffOrPatchQuery, RepoGetContentsQuery, - RepoGetPullRequestCommitsQuery, RepoListPullRequestsQuery, RepoListPullRequestsQueryState, - RepoSearchQuery, StateType, + RepoGetPullRequestCommitsQuery, RepoGetRawFileQuery, RepoListPullRequestsQuery, + RepoListPullRequestsQueryState, RepoSearchQuery, StateType, }; use forgejo_api::{ApiErrorKind, Auth, Forgejo, ForgejoError}; use futures_util::{StreamExt as _, TryStreamExt as _}; @@ -65,6 +65,18 @@ pub struct PullForReview { pub diff: String, } +/// Files on a repo's default branch, as [`Client::default_branch_files`] reads +/// them. +#[derive(Clone, Debug, PartialEq, Eq)] +pub struct DefaultBranchFiles { + pub branch: String, + /// The commit the files were read at. + pub commit: String, + /// Each asked-for path that exists there, with its bytes, in the order + /// asked. + pub files: Vec<(String, Vec)>, +} + /// One row of [`Client::issue_report`] — an open issue plus the two facts /// derived from its dependency graph that swarm-ui's issue-report page /// sorts/filters on. See that function's doc comment for how both are @@ -1102,6 +1114,59 @@ impl Client { }) } + /// The `paths` of `owner/repo` that exist on its default branch, all + /// read at the branch's head commit. `None` for a repo with no commit. + /// + /// # Errors + /// A forge failure, with the [`ForgejoError`] underneath; a 404 on the + /// first call means the repo does not exist ([`is_repo_not_found`]). + pub async fn default_branch_files( + &self, + owner: &str, + repo: &str, + paths: &[&str], + ) -> Result> { + let info = self + .api + .repo_get(owner, repo) + .await + .with_context(|| format!("get repo {owner}/{repo}"))?; + if info.empty.unwrap_or(false) { + return Ok(None); + } + let branch = info + .default_branch + .with_context(|| format!("{owner}/{repo} names no default branch"))?; + let commit = self + .api + .repo_get_branch(owner, repo, &branch) + .await + .with_context(|| format!("read branch {branch} of {owner}/{repo}"))? + .commit + .and_then(|c| c.id) + .with_context(|| format!("branch {branch} of {owner}/{repo} names no commit"))?; + let mut files = Vec::new(); + for path in paths { + let query = RepoGetRawFileQuery { + r#ref: Some(commit.clone()), + }; + match self.api.repo_get_raw_file(owner, repo, path, query).await { + Ok(bytes) => files.push(((*path).to_owned(), bytes.to_vec())), + Err(e) if is_repo_not_found(&e) => {} + Err(e) => { + return Err(e).with_context(|| { + format!("read {path} of {owner}/{repo} at {branch} ({commit})") + }); + } + } + } + Ok(Some(DefaultBranchFiles { + branch, + commit, + files, + })) + } + /// Register the swarm-wide hooks against this controller, so a real /// forge event reaches [`crate::webhook`] instead of the endpoint only /// being reachable by hand. diff --git a/swarm-controller/src/main.rs b/swarm-controller/src/main.rs index 15018542..604b12a2 100644 --- a/swarm-controller/src/main.rs +++ b/swarm-controller/src/main.rs @@ -3281,6 +3281,7 @@ fn build_app(state: AppState) -> axum::Router { .routes(routes!(issue_report::get_issue_report)) .routes(routes!(review_classifier::post_try_out)) .routes(routes!(review_classifier::get_try_out)) + .routes(routes!(review_classifier::get_rules)) .routes(routes!(webhook::post_webhook_forge)) .split_for_parts(); // Just the JSON, not the UI — Swagger UI itself is nginx-hosted from diff --git a/swarm-controller/src/review_classifier.rs b/swarm-controller/src/review_classifier.rs index 78affff2..384e870d 100644 --- a/swarm-controller/src/review_classifier.rs +++ b/swarm-controller/src/review_classifier.rs @@ -1,6 +1,8 @@ //! `POST /api/review-classifier/try-out` and //! `GET /api/review-classifier/try-out/{id}`: run classifications against a //! PR from the swarm UI, then read the result back. +//! `GET /api/review-classifier/rules`: a repo's own classifications, from +//! the rules file on its default branch. //! //! The POST reads the PR with this daemon's forge token, sends one request to //! the review classifier on [`SUBJECT`] and answers 202 with an id at once: @@ -8,27 +10,29 @@ //! holds a request open. The reply is kept in memory, where the GET reads it //! until [`RESULT_TTL`] after it arrived. Nothing reaches the forge. //! +//! The rules are read with the same token on every request, with no cache. +//! //! The operator is the `Remote-User` header the gateway sets, as for //! `agent_input`; the classifier names them in its journal line. -use std::collections::{HashMap, HashSet}; +use std::collections::{BTreeMap, HashMap, HashSet}; use std::fmt::Write as _; use std::future::Future; use std::sync::{Arc, Mutex}; use std::time::{Duration, Instant}; use axum::Json; -use axum::extract::rejection::JsonRejection; -use axum::extract::{Path, State}; +use axum::extract::rejection::{JsonRejection, QueryRejection}; +use axum::extract::{Path, Query, State}; use axum::http::{HeaderMap, StatusCode}; use problem_details::ProblemDetails; use serde::{Deserialize, Serialize}; use swarm_queue_client::review_classifier::{ Classification, MAX_CLASSIFICATIONS, Overall, RUN_DEADLINE, Reply, Row, SUBJECT, TryOut, }; -use utoipa::ToSchema; +use utoipa::{IntoParams, ToSchema}; -use crate::forge::{self, PullForReview}; +use crate::forge::{self, DefaultBranchFiles, PullForReview}; use crate::{AppState, error_problem}; /// How long a finished run's result stays readable. @@ -37,6 +41,13 @@ pub(crate) const RESULT_TTL: Duration = Duration::from_hours(1); /// The header the gateway names the session's user in. const OPERATOR_HEADER: &str = "remote-user"; +/// Where a repo keeps its rules, as YAML or as JSON. A repo holds one of the +/// two. +const RULES_PATHS: [&str; 2] = [ + ".hyperhive/review-classifier.yml", + ".hyperhive/review-classifier.json", +]; + /// A run to start. #[derive(Debug, Deserialize, ToSchema)] pub(crate) struct TryOutRequest { @@ -53,6 +64,45 @@ pub(crate) struct TryOutAccepted { id: String, } +/// Whose rules to read. +#[derive(Debug, Deserialize, IntoParams)] +pub(crate) struct RulesQuery { + /// `owner/name`. + repo: String, +} + +/// A repo's classifications, as its default branch holds them. +#[derive(Debug, Serialize, ToSchema)] +pub(crate) struct Rules { + /// The file they were read from: `.hyperhive/review-classifier.yml` or + /// `.hyperhive/review-classifier.json`. + path: String, + /// The repo's default branch. + branch: String, + /// The commit on `branch` the file was read at. + commit: String, + /// One to ten, each with a distinct name, in the file's order. + classifications: Vec, +} + +/// A rules file: the one list, and nothing beside it. +#[derive(Deserialize)] +#[serde(deny_unknown_fields)] +struct RulesFile { + classifications: Vec, +} + +/// One entry of a rules file. `unknown` collects every key a +/// [`Classification`] doesn't have, which `deny_unknown_fields` can't do +/// beside a `flatten`. +#[derive(Deserialize)] +struct Rule { + #[serde(flatten)] + classification: Classification, + #[serde(flatten)] + unknown: BTreeMap, +} + /// `running` lasts at most the run deadline, 31 minutes: ten /// classifications at three minutes each, one after another, and a minute /// more. @@ -187,6 +237,28 @@ impl PullSource for forge::Client { } } +/// Where a repo's files are read from: the forge in production, a fake in +/// the tests. +pub(crate) trait FileSource { + fn default_branch_files( + &self, + owner: &str, + repo: &str, + paths: &[&str], + ) -> impl Future>> + Send; +} + +impl FileSource for forge::Client { + async fn default_branch_files( + &self, + owner: &str, + repo: &str, + paths: &[&str], + ) -> anyhow::Result> { + forge::Client::default_branch_files(self, owner, repo, paths).await + } +} + /// Where a run is sent: the queue in production, a fake in the tests. pub(crate) trait RunSink: Clone + Send + Sync + 'static { fn run(&self, request: Vec) -> impl Future + Send; @@ -319,6 +391,133 @@ pub(crate) async fn get_try_out( }) } +/// Read a repo's classifications from its default branch. +#[utoipa::path( + get, + path = "/api/review-classifier/rules", + params(RulesQuery), + responses( + (status = 200, description = "the classifications the default branch's rules file \ + holds, and the commit it was read at", body = Rules), + (status = 400, description = "no `repo`, or a repo that is not owner/name with \ + segments of `[A-Za-z0-9._-]` other than `.` and `..` (problem+json)", body = String), + (status = 404, description = "the forge knows no such repo, the repo has no commit, \ + or its default branch holds neither `.hyperhive/review-classifier.yml` nor \ + `.hyperhive/review-classifier.json` (problem+json)", body = String), + (status = 422, description = "the default branch holds both files; or the file does \ + not parse as its extension says; or it holds a key other than `classifications`, \ + or a classification with a key other than `name`, `description`, `polarity` and \ + `threshold`; or it fails a check the try-out POST makes: no classification or \ + more than ten, an empty trimmed name or description, two with one trimmed name, \ + or a threshold over 100 (problem+json)", body = String), + (status = 503, description = "this host has no forge token yet; or the forge failed \ + or did not answer within two minutes (problem+json)", body = String), + ), + tag = "review-classifier" +)] +pub(crate) async fn get_rules( + State(state): State, + query: Result, QueryRejection>, +) -> Result, ProblemDetails> { + let Query(query) = query.map_err(|e| error_problem(StatusCode::BAD_REQUEST, &e.body_text()))?; + let (owner, name) = + split_repo(&query.repo).map_err(|d| error_problem(StatusCode::BAD_REQUEST, &d))?; + #[cfg(test)] + if let Some(fake) = state.try_outs.fake.clone() { + return rules(&*fake, owner, name).await; + } + let Some(forge) = state.forge.get() else { + return Err(error_problem( + StatusCode::SERVICE_UNAVAILABLE, + "no forge access on this host yet", + )); + }; + rules(forge.as_ref(), owner, name).await +} + +/// Read the rules file of `owner/name` and parse it. +async fn rules( + files: &impl FileSource, + owner: &str, + name: &str, +) -> Result, ProblemDetails> { + let repo = format!("{owner}/{name}"); + let read = forge::within_forge_deadline( + "review rules", + files.default_branch_files(owner, name, &RULES_PATHS), + ) + .await + .ok_or_else(|| { + error_problem( + StatusCode::SERVICE_UNAVAILABLE, + "the forge did not answer within the deadline", + ) + })? + .map_err(|e| rules_problem(&e, &repo))? + .ok_or_else(|| { + error_problem( + StatusCode::NOT_FOUND, + &format!("{repo} has no commit, so no rules file"), + ) + })?; + let [yml, json] = RULES_PATHS; + let (path, bytes) = match read.files.as_slice() { + [] => { + return Err(error_problem( + StatusCode::NOT_FOUND, + &format!( + "{repo}'s default branch {} holds neither {yml} nor {json}", + read.branch + ), + )); + } + [file] => file, + _ => { + return Err(error_problem( + StatusCode::UNPROCESSABLE_ENTITY, + &format!( + "{repo}'s default branch {} holds both {yml} and {json}: keep one", + read.branch + ), + )); + } + }; + let classifications = parse_rules(path, bytes) + .map_err(|d| error_problem(StatusCode::UNPROCESSABLE_ENTITY, &d))?; + tracing::info!(%repo, branch = %read.branch, commit = %read.commit, %path, "review rules read"); + Ok(Json(Rules { + path: path.clone(), + branch: read.branch, + commit: read.commit, + classifications, + })) +} + +/// The classifications in the rules file at `path`, parsed as JSON for a +/// `.json` path and as YAML otherwise, or what is wrong with them. +fn parse_rules(path: &str, bytes: &[u8]) -> Result, String> { + let file: RulesFile = if std::path::Path::new(path) + .extension() + .is_some_and(|e| e == "json") + { + serde_json::from_slice(bytes).map_err(|e| format!("{path}: {e}"))? + } else { + serde_norway::from_slice(bytes).map_err(|e| format!("{path}: {e}"))? + }; + let mut classifications = Vec::with_capacity(file.classifications.len()); + for rule in file.classifications { + if let Some(key) = rule.unknown.keys().next() { + return Err(format!( + "{path}: classification {:?} has an unknown field {key:?}", + rule.classification.name.trim() + )); + } + classifications.push(rule.classification); + } + check_classifications(&classifications).map_err(|e| format!("{path}: {e}"))?; + Ok(classifications) +} + /// A request that passed every check, with its repo split. #[derive(Debug)] struct Run { @@ -331,40 +530,12 @@ struct Run { impl Run { /// The request, or what is wrong with it. fn checked(req: TryOutRequest) -> Result { - let (owner, name) = req - .repo - .split_once('/') - .filter(|(o, n)| segment(o) && segment(n)) - .ok_or_else(|| format!("repo {:?} is not owner/name", req.repo))?; + let (owner, name) = split_repo(&req.repo)?; let pr = i64::try_from(req.pr) .ok() .filter(|n| *n > 0) .ok_or_else(|| format!("{} is not a PR number", req.pr))?; - let count = req.classifications.len(); - if count == 0 || count > MAX_CLASSIFICATIONS { - return Err(format!( - "{count} classifications: a run carries 1 to {MAX_CLASSIFICATIONS}" - )); - } - let mut names = HashSet::new(); - for c in &req.classifications { - let name = c.name.trim(); - if name.is_empty() { - return Err("a classification has no name".to_owned()); - } - if c.description.trim().is_empty() { - return Err(format!("classification {name} has no description")); - } - if c.threshold > 100 { - return Err(format!( - "classification {name}'s threshold {} is over 100", - c.threshold - )); - } - if !names.insert(name) { - return Err(format!("two classifications are named {name}")); - } - } + check_classifications(&req.classifications)?; Ok(Self { owner: owner.to_owned(), name: name.to_owned(), @@ -374,6 +545,43 @@ impl Run { } } +/// `repo` as its owner and name, or what is wrong with it. +fn split_repo(repo: &str) -> Result<(&str, &str), String> { + repo.split_once('/') + .filter(|(o, n)| segment(o) && segment(n)) + .ok_or_else(|| format!("repo {repo:?} is not owner/name")) +} + +/// What is wrong with `classifications` as one run's, if anything. +fn check_classifications(classifications: &[Classification]) -> Result<(), String> { + let count = classifications.len(); + if count == 0 || count > MAX_CLASSIFICATIONS { + return Err(format!( + "{count} classifications: a run carries 1 to {MAX_CLASSIFICATIONS}" + )); + } + let mut names = HashSet::new(); + for c in classifications { + let name = c.name.trim(); + if name.is_empty() { + return Err("a classification has no name".to_owned()); + } + if c.description.trim().is_empty() { + return Err(format!("classification {name} has no description")); + } + if c.threshold > 100 { + return Err(format!( + "classification {name}'s threshold {} is over 100", + c.threshold + )); + } + if !names.insert(name) { + return Err(format!("two classifications are named {name}")); + } + } + Ok(()) +} + /// One `owner` or `name` of a repo, as the forge allows them. fn segment(s: &str) -> bool { !s.is_empty() @@ -452,6 +660,20 @@ fn pull_problem(e: &anyhow::Error, repo: &str, pr: i64) -> ProblemDetails { } } +/// 404 when the forge knows no such repo, 503 for anything else. +fn rules_problem(e: &anyhow::Error, repo: &str) -> ProblemDetails { + match e.downcast_ref::() { + Some(fe) if forge::is_repo_not_found(fe) => { + error_problem(StatusCode::NOT_FOUND, &format!("no repo {repo}")) + } + _ => { + let detail = format!("{e:#}"); + tracing::warn!(error = %detail, %repo, "reading a repo's review rules failed"); + error_problem(StatusCode::SERVICE_UNAVAILABLE, &detail) + } + } +} + /// A fresh run id: 32 hex digits from the kernel's CSPRNG. fn mint_id() -> Result { let mut bytes = [0u8; 16]; @@ -474,11 +696,13 @@ pub(crate) mod tests { use super::*; - /// A forge holding one PR, `o/r` 7, and a classifier answering every run - /// with `reply`. + /// A forge holding one PR, `o/r` 7, with `files` on `o/r`'s default + /// branch, and a classifier answering every run with `reply`. `o/empty` + /// has no commit, and `o/down` fails with something other than a 404. #[derive(Clone)] pub(crate) struct Fake { reply: Reply, + files: Vec<(&'static str, String)>, } impl PullSource for Fake { @@ -500,6 +724,32 @@ pub(crate) mod tests { } } + impl FileSource for Fake { + async fn default_branch_files( + &self, + owner: &str, + repo: &str, + paths: &[&str], + ) -> anyhow::Result> { + match (owner, repo) { + ("o", "r") => Ok(Some(DefaultBranchFiles { + branch: "main".to_owned(), + commit: "def456".to_owned(), + files: paths + .iter() + .filter_map(|p| self.files.iter().find(|(f, _)| f == p)) + .map(|(f, body)| ((*f).to_owned(), body.clone().into_bytes())) + .collect(), + })), + ("o", "empty") => Ok(None), + ("o", "down") => Err(anyhow::anyhow!("the forge answered 500")), + _ => Err( + ForgejoError::ApiError(ApiErrorKind::NotFound { errors: None }.into()).into(), + ), + } + } + } + impl RunSink for Fake { async fn run(&self, _request: Vec) -> Reply { self.reply.clone() @@ -524,8 +774,12 @@ pub(crate) mod tests { } fn app(reply: Reply) -> axum::Router { + app_with_files(reply, Vec::new()) + } + + fn app_with_files(reply: Reply, files: Vec<(&'static str, String)>) -> axum::Router { let (mut state, _) = crate::tests::state_with_roster(); - state.try_outs.fake = Some(Arc::new(Fake { reply })); + state.try_outs.fake = Some(Arc::new(Fake { reply, files })); crate::build_app(state) } @@ -738,4 +992,225 @@ pub(crate) mod tests { assert!(!entry(None).expired(now + RUN_DEADLINE + RESULT_TTL)); assert!(entry(None).expired(now + RUN_DEADLINE + RESULT_TTL + Duration::from_secs(1))); } + + /// A rules file holding `classifications`, and `extra` beside them. + fn rules_value( + classifications: &serde_json::Value, + extra: &[(&str, &str)], + ) -> serde_json::Value { + let mut file = serde_json::json!({ "classifications": classifications }); + for (key, value) in extra { + file[*key] = (*value).into(); + } + file + } + + fn datetime() -> serde_json::Value { + serde_json::json!({ + "name": "datetime", + "description": "All datetimes carry time zone information.", + "polarity": "positive", + "threshold": 80, + }) + } + + /// `file` as both formats, each with its path. + fn both_formats(file: &serde_json::Value) -> [(&'static str, String); 2] { + let [yml, json] = RULES_PATHS; + [ + (yml, serde_norway::to_string(file).expect("encodes as YAML")), + (json, file.to_string()), + ] + } + + #[test] + fn a_rules_file_parses_the_same_as_yaml_and_as_json() { + let yaml = "\ +classifications: + - name: datetime + description: All datetimes carry time zone information. + polarity: positive + threshold: 80 + - name: no-ads + description: >- + A commit message contains a co-authored-by line or other + advertising for an AI model. + polarity: negative + threshold: 90 +"; + let json = r#"{"classifications": [ + {"name": "datetime", "description": "All datetimes carry time zone information.", + "polarity": "positive", "threshold": 80}, + {"name": "no-ads", "description": "A commit message contains a co-authored-by line or other advertising for an AI model.", + "polarity": "negative", "threshold": 90} + ]}"#; + let want = vec![ + Classification { + name: "datetime".to_owned(), + description: "All datetimes carry time zone information.".to_owned(), + polarity: Polarity::Positive, + threshold: 80, + }, + Classification { + name: "no-ads".to_owned(), + description: "A commit message contains a co-authored-by line or other \ + advertising for an AI model." + .to_owned(), + polarity: Polarity::Negative, + threshold: 90, + }, + ]; + let [yml_path, json_path] = RULES_PATHS; + assert_eq!(parse_rules(yml_path, yaml.as_bytes()), Ok(want.clone())); + assert_eq!(parse_rules(json_path, json.as_bytes()), Ok(want)); + } + + #[test] + fn a_bad_rules_file_is_an_error_in_either_format() { + let mut unknown_rule_field = datetime(); + unknown_rule_field["threshhold"] = 80.into(); + let mut over_100 = datetime(); + over_100["threshold"] = 101.into(); + let mut no_name = datetime(); + no_name["name"] = " ".into(); + let mut no_polarity = datetime(); + no_polarity + .as_object_mut() + .expect("an object") + .remove("polarity"); + let eleven: Vec<_> = (0..11) + .map(|i| { + let mut c = datetime(); + c["name"] = format!("c{i}").into(); + c + }) + .collect(); + for (case, file, says) in [ + ("no list", serde_json::json!({}), "classifications"), + ( + "an empty list", + rules_value(&serde_json::json!([]), &[]), + "0 classifications", + ), + ( + "an unknown top-level field", + rules_value(&serde_json::json!([datetime()]), &[("version", "1")]), + "version", + ), + ( + "an unknown classification field", + rules_value(&serde_json::json!([unknown_rule_field]), &[]), + "threshhold", + ), + ( + "eleven classifications", + rules_value(&eleven.into(), &[]), + "11 classifications", + ), + ( + "a threshold over 100", + rules_value(&serde_json::json!([over_100]), &[]), + "over 100", + ), + ( + "an empty name", + rules_value(&serde_json::json!([no_name]), &[]), + "no name", + ), + ( + "no polarity", + rules_value(&serde_json::json!([no_polarity]), &[]), + "polarity", + ), + ( + "one name twice", + rules_value(&serde_json::json!([datetime(), datetime()]), &[]), + "two classifications", + ), + ] { + for (path, body) in both_formats(&file) { + let err = parse_rules(path, body.as_bytes()) + .expect_err(&format!("{case} in {path} must not parse")); + assert!( + err.starts_with(path) && err.contains(says), + "{case} in {path}: {err}" + ); + } + } + let [yml, json] = RULES_PATHS; + for (path, body) in [(yml, "classifications: [\n"), (json, "{")] { + let err = parse_rules(path, body.as_bytes()).expect_err("must not parse"); + assert!(err.starts_with(path), "{err}"); + } + } + + async fn get_rules(app: &axum::Router, query: &str) -> (StatusCode, serde_json::Value) { + let req = Request::get(format!("/api/review-classifier/rules{query}")) + .body(Body::empty()) + .expect("a request"); + send(app, req).await + } + + #[tokio::test] + async fn a_repos_rules_are_200_with_the_commit_they_were_read_at() { + let file = rules_value(&serde_json::json!([datetime()]), &[]); + for (path, body) in both_formats(&file) { + let app = app_with_files(done(), vec![(path, body)]); + let (status, rules) = get_rules(&app, "?repo=o/r").await; + assert_eq!(status, StatusCode::OK, "{path}: {rules}"); + assert_eq!(rules["path"], path); + assert_eq!(rules["branch"], "main"); + assert_eq!(rules["commit"], "def456"); + assert_eq!(rules["classifications"], serde_json::json!([datetime()])); + } + } + + #[tokio::test] + async fn a_repo_without_rules_or_with_bad_ones_is_an_error() { + let good = rules_value(&serde_json::json!([datetime()]), &[]); + let bad = rules_value(&serde_json::json!([]), &[]); + let [yml, json] = RULES_PATHS; + let one = |path: &'static str, file: &serde_json::Value| vec![(path, file.to_string())]; + for (query, files, want, says) in [ + ("", Vec::new(), StatusCode::BAD_REQUEST, "repo"), + ("?repo=o", Vec::new(), StatusCode::BAD_REQUEST, "owner/name"), + ( + "?repo=o/elsewhere", + Vec::new(), + StatusCode::NOT_FOUND, + "no repo o/elsewhere", + ), + ( + "?repo=o/empty", + Vec::new(), + StatusCode::NOT_FOUND, + "no commit", + ), + ("?repo=o/r", Vec::new(), StatusCode::NOT_FOUND, "neither"), + ( + "?repo=o/down", + Vec::new(), + StatusCode::SERVICE_UNAVAILABLE, + "500", + ), + ( + "?repo=o/r", + [one(yml, &good), one(json, &good)].concat(), + StatusCode::UNPROCESSABLE_ENTITY, + "keep one", + ), + ( + "?repo=o/r", + one(json, &bad), + StatusCode::UNPROCESSABLE_ENTITY, + "0 classifications", + ), + ] { + let app = app_with_files(done(), files); + let (status, problem) = get_rules(&app, query).await; + assert_eq!(status, want, "{query}: {problem}"); + let detail = problem["detail"].as_str().expect("a problem+json detail"); + assert!(detail.contains(says), "{query}: {detail}"); + } + } }