diff --git a/Cargo.lock b/Cargo.lock index 85d2b862..db8ce7f9 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4843,7 +4843,6 @@ 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 61376371..ed1053aa 100644 --- a/docs/swarm/review-classifier.md +++ b/docs/swarm/review-classifier.md @@ -18,31 +18,23 @@ that reviewer. ## Repo configuration -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. +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. -The file holds one key, `classifications`: a list of one to ten entries, each -with exactly these four keys: +Each classification has: -- `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 +- 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 -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: +An illustrative example. The file name and every field name in it are +placeholders, not a fixed schema: ```yaml +# illustrative only: file name and field names aren't fixed classifications: - name: datetime description: All datetimes carry time zone information. @@ -249,25 +241,6 @@ 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 default branch with no commit, - in an empty repo or when the default branch doesn't exist; 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 82d53077..c99cb3da 100644 --- a/swarm-controller/Cargo.toml +++ b/swarm-controller/Cargo.toml @@ -22,9 +22,9 @@ axum.workspace = true # boundary (see #3306's design discussion: forcing that split now, over a # few idempotent CRUD-ish calls, is premature plumbing). forgejo-api.workspace = true -# `forge.rs` encodes file content for its `repo_change_files` calls and -# decodes blobs it reads: forgejo's content and blob APIs carry base64, -# never raw bytes. +# Only for base64-encoding file content for `forge.rs`'s +# `repo_change_files` calls — forgejo's content API takes base64, never +# raw bytes. base64.workspace = true # `agent_identity.rs` mints the per-agent queue secret, the one value in this # tree this daemon invents rather than receives. Straight from the kernel's @@ -79,7 +79,6 @@ 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 eefe0934..d408130f 100644 --- a/swarm-controller/src/forge.rs +++ b/swarm-controller/src/forge.rs @@ -23,10 +23,10 @@ use forgejo_api::structs::{ AddCollaboratorOption, AddCollaboratorOptionPermission, ChangeFileOperation, ChangeFileOperationOperation, ChangeFilesOptions, CreateBranchProtectionOption, CreateHookOption, CreateHookOptionConfig, CreateHookOptionType, CreateRepoOption, - CreateUserOption, EditUserOption, GetTreeQuery, GitEntry, IssueListIssuesQuery, - IssueListIssuesQueryState, IssueListIssuesQueryType, RepoDownloadPullDiffOrPatchQuery, - RepoGetContentsQuery, RepoGetPullRequestCommitsQuery, RepoListPullRequestsQuery, - RepoListPullRequestsQueryState, RepoSearchQuery, StateType, + CreateUserOption, EditUserOption, IssueListIssuesQuery, IssueListIssuesQueryState, + IssueListIssuesQueryType, RepoDownloadPullDiffOrPatchQuery, RepoGetContentsQuery, + RepoGetPullRequestCommitsQuery, RepoListPullRequestsQuery, RepoListPullRequestsQueryState, + RepoSearchQuery, StateType, }; use forgejo_api::{ApiErrorKind, Auth, Forgejo, ForgejoError}; use futures_util::{StreamExt as _, TryStreamExt as _}; @@ -65,18 +65,6 @@ 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 @@ -1114,144 +1102,6 @@ impl Client { }) } - /// The `paths` of `owner/repo` that exist on its default branch, all - /// read at the branch's head commit. `None` when the default branch has - /// no commit: an empty repo, or a default branch that doesn't exist. - /// - /// The files come through the git tree and blob API, one path segment - /// per request: forgejo-api escapes the `/` of a nested path in its - /// file endpoints. - /// - /// # Errors - /// A forge failure, or a tree the forge truncated. A [`ForgejoError`] 404 - /// underneath comes only from the first call, and 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 head = match self.api.repo_get_branch(owner, repo, &branch).await { - Ok(head) => head, - Err(e) if is_repo_not_found(&e) => return Ok(None), - Err(e) => { - return Err(e).with_context(|| format!("read branch {branch} of {owner}/{repo}")); - } - }; - let commit = head - .commit - .and_then(|c| c.id) - .with_context(|| format!("branch {branch} of {owner}/{repo} names no commit"))?; - let mut trees = HashMap::new(); - let mut files = Vec::new(); - for path in paths { - if let Some(bytes) = self.file_at(owner, repo, &commit, path, &mut trees).await? { - files.push(((*path).to_owned(), bytes)); - } - } - Ok(Some(DefaultBranchFiles { - branch, - commit, - files, - })) - } - - /// The file at `path` in `owner/repo` at `commit`, or `None` when no - /// blob is there. `trees` holds the tree listings already read, by sha. - async fn file_at( - &self, - owner: &str, - repo: &str, - commit: &str, - path: &str, - trees: &mut HashMap>, - ) -> Result>> { - let mut tree = commit.to_owned(); - let mut segments = path.split('/').peekable(); - while let Some(name) = segments.next() { - let kind = if segments.peek().is_some() { - "tree" - } else { - "blob" - }; - let entries = self.tree(owner, repo, &tree, trees).await?; - let Some(sha) = entries - .iter() - .find(|e| e.path.as_deref() == Some(name) && e.r#type.as_deref() == Some(kind)) - .and_then(|e| e.sha.clone()) - else { - return Ok(None); - }; - if kind == "blob" { - return self.blob(owner, repo, &sha).await.map(Some); - } - tree = sha; - } - Ok(None) - } - - /// The entries of tree `sha` (or of a commit's root tree), read once - /// into `trees`. - async fn tree<'t>( - &self, - owner: &str, - repo: &str, - sha: &str, - trees: &'t mut HashMap>, - ) -> Result<&'t [GitEntry]> { - if !trees.contains_key(sha) { - let listing = self - .api - .get_tree(owner, repo, sha, GetTreeQuery::default()) - .await - .map_err(|e| anyhow::anyhow!("read tree {sha} of {owner}/{repo}: {e}"))?; - if listing.truncated.unwrap_or(false) { - anyhow::bail!( - "the forge truncated tree {sha} of {owner}/{repo}, so a missing entry can't \ - be told from one past the cut" - ); - } - trees.insert(sha.to_owned(), listing.tree.unwrap_or_default()); - } - Ok(&trees[sha]) - } - - /// The bytes of blob `sha`. - async fn blob(&self, owner: &str, repo: &str, sha: &str) -> Result> { - use base64::Engine as _; - - let blob = self - .api - .get_blob(owner, repo, sha) - .await - .map_err(|e| anyhow::anyhow!("read blob {sha} of {owner}/{repo}: {e}"))?; - anyhow::ensure!( - blob.encoding.as_deref() == Some("base64"), - "blob {sha} of {owner}/{repo} is not base64 but {:?}", - blob.encoding - ); - let content: String = blob - .content - .unwrap_or_default() - .split_ascii_whitespace() - .collect(); - base64::engine::general_purpose::STANDARD - .decode(content) - .with_context(|| format!("decode blob {sha} of {owner}/{repo}")) - } - /// 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. @@ -2120,147 +1970,4 @@ mod tests { .collect(); assert_eq!(methods, ["GET"]); } - - const RULES_YML: &str = ".hyperhive/review-classifier.yml"; - const RULES_JSON: &str = ".hyperhive/review-classifier.json"; - - /// A stand-in forge holding `o/r` with default branch `main` at - /// `def456`. The branch lookup answers `branch`, and the commit's root - /// tree answers `root` (a 404 for `None`). The `.hyperhive` tree holds - /// only `review-classifier.yml`. Records every request's path. - async fn rules_forge( - branch: u16, - root: Option<&'static str>, - ) -> (Client, Arc>>) { - use axum::http::{StatusCode, Uri}; - use base64::Engine as _; - - let blob = format!( - r#"{{"sha":"b1","encoding":"base64","content":"{}"}}"#, - base64::engine::general_purpose::STANDARD.encode("classifications: []\n") - ); - let seen = Arc::new(std::sync::Mutex::new(Vec::new())); - let recorded = Arc::clone(&seen); - let app = axum::Router::new().fallback(move |uri: Uri| { - let (recorded, blob) = (Arc::clone(&recorded), blob.clone()); - async move { - recorded.lock().unwrap().push(uri.path().to_owned()); - let (status, body) = match uri.path() { - "/api/v1/repos/o/r" => { - (200, r#"{"default_branch":"main","empty":false}"#.to_owned()) - } - "/api/v1/repos/o/r/branches/main" if branch == 200 => ( - 200, - r#"{"name":"main","commit":{"id":"def456"}}"#.to_owned(), - ), - "/api/v1/repos/o/r/branches/main" => (branch, r#"{"message":"no"}"#.to_owned()), - "/api/v1/repos/o/r/git/trees/def456" if root.is_some() => { - (200, root.unwrap_or_default().to_owned()) - } - "/api/v1/repos/o/r/git/trees/t1" => ( - 200, - r#"{"sha":"t1","truncated":false,"tree":[ - {"path":"review-classifier.yml","type":"blob","sha":"b1"}]}"# - .to_owned(), - ), - "/api/v1/repos/o/r/git/blobs/b1" => (200, blob), - _ => (404, r#"{"message":"not found"}"#.to_owned()), - }; - (StatusCode::from_u16(status).unwrap(), body) - } - }); - let listener = tokio::net::TcpListener::bind("127.0.0.1:0").await.unwrap(); - let url = url::Url::parse(&format!("http://{}/", listener.local_addr().unwrap())).unwrap(); - tokio::spawn(async move { axum::serve(listener, app).await }); - let api = Forgejo::new(Auth::Token("stub-token"), url.clone()).unwrap(); - (Client { api, url }, seen) - } - - /// A root tree with the `.hyperhive` tree and a blob beside it. - const ROOT: &str = r#"{"sha":"def456","truncated":false,"tree":[ - {"path":"README.md","type":"blob","sha":"b0"}, - {"path":".hyperhive","type":"tree","sha":"t1"}]}"#; - - #[tokio::test] - async fn default_branch_files_reads_each_path_through_the_trees_at_the_branch_head() { - let (client, seen) = rules_forge(200, Some(ROOT)).await; - let read = client - .default_branch_files("o", "r", &[RULES_YML, RULES_JSON]) - .await - .unwrap() - .expect("the branch has a commit"); - assert_eq!(read.branch, "main"); - assert_eq!(read.commit, "def456"); - assert_eq!( - read.files, - [(RULES_YML.to_owned(), b"classifications: []\n".to_vec())] - ); - // Each tree is read once for both paths. - let seen = seen.lock().unwrap(); - let reads = |p: &str| seen.iter().filter(|s| *s == p).count(); - assert_eq!(reads("/api/v1/repos/o/r/git/trees/def456"), 1, "{seen:?}"); - assert_eq!(reads("/api/v1/repos/o/r/git/trees/t1"), 1, "{seen:?}"); - } - - #[tokio::test] - async fn a_repo_without_the_directory_has_no_files() { - let root = r#"{"sha":"def456","truncated":false,"tree":[ - {"path":"README.md","type":"blob","sha":"b0"}, - {"path":".hyperhive","type":"blob","sha":"b2"}]}"#; - let (client, _) = rules_forge(200, Some(root)).await; - let read = client - .default_branch_files("o", "r", &[RULES_YML]) - .await - .unwrap() - .expect("the branch has a commit"); - assert_eq!(read.files, []); - } - - #[tokio::test] - async fn a_truncated_tree_is_an_error_not_a_missing_file() { - let root = r#"{"sha":"def456","truncated":true,"tree":[]}"#; - let (client, _) = rules_forge(200, Some(root)).await; - let err = client - .default_branch_files("o", "r", &[RULES_YML]) - .await - .unwrap_err(); - assert!(format!("{err:#}").contains("truncated"), "{err:#}"); - } - - #[tokio::test] - async fn a_missing_default_branch_is_no_commit_not_no_repo() { - let (client, _) = rules_forge(404, Some(ROOT)).await; - let read = client - .default_branch_files("o", "r", &[RULES_YML]) - .await - .unwrap(); - assert_eq!(read, None); - } - - /// Only the repo lookup's 404 reads as "no such repo"; a failed branch - /// read or a tree the forge doesn't have is some other failure. - #[tokio::test] - async fn only_an_unknown_repo_is_a_forge_404() { - let no_repo = |e: &anyhow::Error| { - e.downcast_ref::() - .is_some_and(is_repo_not_found) - }; - let (client, _) = rules_forge(500, Some(ROOT)).await; - let unknown = client - .default_branch_files("o", "elsewhere", &[]) - .await - .unwrap_err(); - assert!(no_repo(&unknown), "{unknown:#}"); - let branch = client - .default_branch_files("o", "r", &[]) - .await - .unwrap_err(); - assert!(!no_repo(&branch), "{branch:#}"); - let (client, _) = rules_forge(200, None).await; - let tree = client - .default_branch_files("o", "r", &[RULES_YML]) - .await - .unwrap_err(); - assert!(!no_repo(&tree), "{tree:#}"); - } } diff --git a/swarm-controller/src/main.rs b/swarm-controller/src/main.rs index 604b12a2..15018542 100644 --- a/swarm-controller/src/main.rs +++ b/swarm-controller/src/main.rs @@ -3281,7 +3281,6 @@ 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 ef01dd56..78affff2 100644 --- a/swarm-controller/src/review_classifier.rs +++ b/swarm-controller/src/review_classifier.rs @@ -1,8 +1,6 @@ //! `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: @@ -10,29 +8,27 @@ //! 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::{BTreeMap, HashMap, HashSet}; +use std::collections::{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, QueryRejection}; -use axum::extract::{Path, Query, State}; +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_queue_client::review_classifier::{ Classification, MAX_CLASSIFICATIONS, Overall, RUN_DEADLINE, Reply, Row, SUBJECT, TryOut, }; -use utoipa::{IntoParams, ToSchema}; +use utoipa::ToSchema; -use crate::forge::{self, DefaultBranchFiles, PullForReview}; +use crate::forge::{self, PullForReview}; use crate::{AppState, error_problem}; /// How long a finished run's result stays readable. @@ -41,13 +37,6 @@ 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 { @@ -64,45 +53,6 @@ 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. @@ -237,28 +187,6 @@ 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; @@ -391,134 +319,6 @@ 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; its default branch has no \ - commit, as in an empty repo or one whose default branch doesn't exist; 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 on its default branch, 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 { @@ -531,12 +331,40 @@ struct Run { impl Run { /// The request, or what is wrong with it. fn checked(req: TryOutRequest) -> Result { - let (owner, name) = split_repo(&req.repo)?; + 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 pr = i64::try_from(req.pr) .ok() .filter(|n| *n > 0) .ok_or_else(|| format!("{} is not a PR number", req.pr))?; - check_classifications(&req.classifications)?; + 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}")); + } + } Ok(Self { owner: owner.to_owned(), name: name.to_owned(), @@ -546,43 +374,6 @@ 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() @@ -661,20 +452,6 @@ 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]; @@ -697,14 +474,11 @@ pub(crate) mod tests { use super::*; - /// 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, `o/down` fails with something other than a 404, and - /// `o/hang` never answers. + /// A forge holding one PR, `o/r` 7, and a classifier answering every run + /// with `reply`. #[derive(Clone)] pub(crate) struct Fake { reply: Reply, - files: Vec<(&'static str, String)>, } impl PullSource for Fake { @@ -726,33 +500,6 @@ 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")), - ("o", "hang") => std::future::pending().await, - _ => Err( - ForgejoError::ApiError(ApiErrorKind::NotFound { errors: None }.into()).into(), - ), - } - } - } - impl RunSink for Fake { async fn run(&self, _request: Vec) -> Reply { self.reply.clone() @@ -777,12 +524,8 @@ 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, files })); + state.try_outs.fake = Some(Arc::new(Fake { reply })); crate::build_app(state) } @@ -995,263 +738,4 @@ 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 bad_polarity = datetime(); - bad_polarity["polarity"] = "sideways".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", - ), - ( - "a bad polarity", - rules_value(&serde_json::json!([bad_polarity]), &[]), - "sideways", - ), - ( - "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 mut sideways = datetime(); - sideways["polarity"] = "sideways".into(); - let sideways = rules_value(&serde_json::json!([sideways]), &[]); - 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 on its default branch", - ), - ("?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", - ), - ( - "?repo=o/r", - one(yml, &sideways), - StatusCode::UNPROCESSABLE_ENTITY, - "sideways", - ), - ] { - 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}"); - } - } - - #[tokio::test] - async fn rules_without_forge_access_on_this_host_are_503() { - let (state, _) = crate::tests::state_with_roster(); - let (status, problem) = get_rules(&crate::build_app(state), "?repo=o/r").await; - assert_eq!(status, StatusCode::SERVICE_UNAVAILABLE, "{problem}"); - assert_eq!(problem["detail"], "no forge access on this host yet"); - } - - /// The paused clock jumps to the deadline once nothing else can run, so - /// this takes no wall time. - #[tokio::test(start_paused = true)] - async fn rules_from_a_forge_that_never_answers_are_503_at_the_deadline() { - let start = tokio::time::Instant::now(); - let (status, problem) = get_rules(&app(done()), "?repo=o/hang").await; - assert_eq!(status, StatusCode::SERVICE_UNAVAILABLE, "{problem}"); - assert_eq!( - problem["detail"], - "the forge did not answer within the deadline" - ); - assert_eq!(start.elapsed(), forge::FORGE_DEADLINE); - } }