diff --git a/docs/swarm/review-classifier.md b/docs/swarm/review-classifier.md index c2fbc746..61376371 100644 --- a/docs/swarm/review-classifier.md +++ b/docs/swarm/review-classifier.md @@ -257,8 +257,9 @@ ones. The route answers: `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 +- **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 diff --git a/swarm-controller/src/forge.rs b/swarm-controller/src/forge.rs index 070a6b11..7a6805ab 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, RepoGetRawFileQuery, RepoListPullRequestsQuery, - RepoListPullRequestsQueryState, RepoSearchQuery, StateType, + RepoGetPullRequestCommitsQuery, RepoListPullRequestsQuery, RepoListPullRequestsQueryState, + RepoSearchQuery, StateType, }; use forgejo_api::{ApiErrorKind, Auth, Forgejo, ForgejoError}; use futures_util::{StreamExt as _, TryStreamExt as _}; @@ -184,6 +184,10 @@ async fn fetch_token() -> Result { /// Typed forgejo client, built once at startup from env. pub struct Client { api: Forgejo, + /// The same token as `api`, for the one request `api` can't shape: + /// forgejo-api escapes every `/` in a file path, so [`raw_file_url`] + /// builds that path itself. + http: reqwest::Client, /// The forge's base URL, kept so a client authenticated as someone /// else can be built against the same forge (see /// [`agent_token`]'s read-back). @@ -323,11 +327,28 @@ impl Client { /// host has not published it yet, or the client failing to build. async fn connect(url: &url::Url) -> Result { let token = fetch_token().await?; - let api = Forgejo::new(Auth::Token(&token), url.clone()).context("build forgejo client")?; - Ok(Self { - api, - url: url.clone(), - }) + Self::with_token(&token, url.clone()) + } + + /// A client for `url` holding `token`. + /// + /// # Errors + /// A token that is not a valid header value, or a client failing to + /// build. + fn with_token(token: &str, url: url::Url) -> Result { + let api = Forgejo::new(Auth::Token(token), url.clone()).context("build forgejo client")?; + let mut auth = reqwest::header::HeaderValue::try_from(format!("token {token}")) + .context("the forge token is not a valid header value")?; + auth.set_sensitive(true); + let http = reqwest::Client::builder() + .default_headers( + [(reqwest::header::AUTHORIZATION, auth)] + .into_iter() + .collect(), + ) + .build() + .context("build forge HTTP client")?; + Ok(Self { api, http, url }) } /// Creation options for an empty repo defaulting to `main`. @@ -1115,11 +1136,12 @@ 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. + /// 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. /// /// # Errors - /// A forge failure, with the [`ForgejoError`] underneath; a 404 on the - /// first call means the repo does not exist ([`is_repo_not_found`]). + /// A forge failure; 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, @@ -1137,27 +1159,21 @@ impl Client { 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}"))? + 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 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})") - }); - } + if let Some(bytes) = self.raw_file(owner, repo, path, &commit).await? { + files.push(((*path).to_owned(), bytes)); } } Ok(Some(DefaultBranchFiles { @@ -1167,6 +1183,28 @@ impl Client { })) } + /// The file at `path` in `owner/repo` at `rev`, or `None` on a 404. + async fn raw_file( + &self, + owner: &str, + repo: &str, + path: &str, + rev: &str, + ) -> Result>> { + let what = || format!("read {path} of {owner}/{repo} at {rev}"); + let response = self + .http + .get(raw_file_url(&self.url, owner, repo, path, rev)?) + .send() + .await + .with_context(what)?; + match response.status() { + StatusCode::NOT_FOUND => Ok(None), + s if s.is_success() => Ok(Some(response.bytes().await.with_context(what)?.to_vec())), + s => Err(anyhow::anyhow!("the forge answered {s}")).with_context(what), + } + } + /// 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. @@ -1526,6 +1564,31 @@ fn folds_into_success(e: &ForgejoError, existing: bool) -> bool { /// Used by [`crate::issue_report::get_issue_report`] to tell "no such repo" /// (404, retrying won't help) apart from every other forge failure (503, /// might clear up) — see that module for why the distinction matters. +/// The API URL of the file at `path` in `owner/repo` at `rev`, on the forge +/// at `base`. Each segment of `path` is escaped on its own and the segments +/// are joined with a literal `/`, so no proxy or router in front of the forge +/// has to decode a `%2F`. Like forgejo-api's own requests, it ignores any +/// path `base` has. +/// +/// # Errors +/// A `base` that can't carry a path, such as a `data:` URL. +fn raw_file_url( + base: &url::Url, + owner: &str, + repo: &str, + path: &str, + rev: &str, +) -> Result { + let mut url = base.clone(); + url.path_segments_mut() + .map_err(|()| anyhow::anyhow!("forge URL {base} can't carry a path"))? + .clear() + .extend(["api", "v1", "repos", owner, repo, "raw"]) + .extend(path.split('/')); + url.query_pairs_mut().clear().append_pair("ref", rev); + Ok(url) +} + pub(crate) fn is_repo_not_found(e: &ForgejoError) -> bool { match e { ForgejoError::ApiError(api) => match api.error_kind() { @@ -1552,8 +1615,7 @@ fn conflict_error() -> ForgejoError { impl Client { /// A client for a test's own forge at `url`, holding a placeholder token. pub(crate) fn for_test_forge(url: url::Url) -> Self { - let api = Forgejo::new(Auth::Token("stub-token"), url.clone()).unwrap(); - Self { api, url } + Self::with_token("stub-token", url).unwrap() } } @@ -1800,8 +1862,7 @@ mod tests { 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) + (Client::with_token("stub-token", url).unwrap(), seen) } /// The `PATCH` [`Client::ensure_agent_user`] sent, if any. @@ -2035,4 +2096,122 @@ mod tests { .collect(); assert_eq!(methods, ["GET"]); } + + #[test] + fn a_raw_file_url_keeps_the_slash_between_path_segments() { + let base = url::Url::parse("https://forge.example/").unwrap(); + let url = raw_file_url( + &base, + "o", + "r", + ".hyperhive/review-classifier.yml", + "def456", + ) + .unwrap(); + assert_eq!( + url.as_str(), + "https://forge.example/api/v1/repos/o/r/raw/.hyperhive/review-classifier.yml?ref=def456" + ); + // A segment's own reserved characters are still escaped. + let odd = raw_file_url(&base, "o", "r", "a b/c?d#e", "x").unwrap(); + assert_eq!(odd.path(), "/api/v1/repos/o/r/raw/a%20b/c%3Fd%23e"); + } + + /// A stand-in forge holding `o/r` with default branch `main` at + /// `def456`, where only `.hyperhive/review-classifier.yml` exists, under + /// that literal path. `branch` is the status the branch lookup answers. + /// Records every request's path and query. + async fn rules_forge(branch: u16) -> (Client, Arc>>) { + use axum::http::{StatusCode, Uri}; + + 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 = Arc::clone(&recorded); + async move { + recorded.lock().unwrap().push(uri.to_string()); + let (status, body) = match uri.path() { + "/api/v1/repos/o/r" => (200, r#"{"default_branch":"main","empty":false}"#), + "/api/v1/repos/o/r/branches/main" if branch == 200 => { + (200, r#"{"name":"main","commit":{"id":"def456"}}"#) + } + "/api/v1/repos/o/r/branches/main" => (branch, r#"{"message":"no"}"#), + "/api/v1/repos/o/r/raw/.hyperhive/review-classifier.yml" + if uri.query() == Some("ref=def456") => + { + (200, "classifications: []\n") + } + _ => (404, r#"{"message":"not found"}"#), + }; + (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 }); + (Client::with_token("stub-token", url).unwrap(), seen) + } + + #[tokio::test] + async fn default_branch_files_reads_each_path_at_the_branch_head() { + let (client, seen) = rules_forge(200).await; + let read = client + .default_branch_files( + "o", + "r", + &[ + ".hyperhive/review-classifier.yml", + ".hyperhive/review-classifier.json", + ], + ) + .await + .unwrap() + .expect("the branch has a commit"); + assert_eq!(read.branch, "main"); + assert_eq!(read.commit, "def456"); + assert_eq!( + read.files, + [( + ".hyperhive/review-classifier.yml".to_owned(), + b"classifications: []\n".to_vec() + )] + ); + assert!( + seen.lock().unwrap().iter().all(|p| !p.contains("%2F")), + "{seen:?}" + ); + } + + #[tokio::test] + async fn a_missing_default_branch_is_no_commit_not_no_repo() { + let (client, _) = rules_forge(404).await; + let read = client + .default_branch_files("o", "r", &[".hyperhive/review-classifier.yml"]) + .await + .unwrap(); + assert_eq!(read, None); + } + + #[tokio::test] + async fn an_unknown_repo_is_a_forge_404_and_a_failed_branch_read_is_not() { + let (client, _) = rules_forge(500).await; + let unknown = client + .default_branch_files("o", "elsewhere", &[]) + .await + .unwrap_err(); + let fe = unknown + .downcast_ref::() + .expect("a forge error"); + assert!(is_repo_not_found(fe), "{unknown:#}"); + let failed = client + .default_branch_files("o", "r", &[]) + .await + .unwrap_err(); + assert!( + !failed + .downcast_ref::() + .is_some_and(is_repo_not_found), + "{failed:#}" + ); + } } diff --git a/swarm-controller/src/forge/docs_grant.rs b/swarm-controller/src/forge/docs_grant.rs index 435b5fea..57ecc233 100644 --- a/swarm-controller/src/forge/docs_grant.rs +++ b/swarm-controller/src/forge/docs_grant.rs @@ -206,7 +206,6 @@ mod tests { use axum::extract::{Path, State}; use axum::http::{StatusCode, header::CONTENT_TYPE}; - use forgejo_api::{Auth, Forgejo}; use super::*; @@ -255,8 +254,7 @@ mod tests { 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 }, docs) + (Client::with_token("stub-token", url).unwrap(), docs) } async fn check( diff --git a/swarm-controller/src/forge/legacy_tokens.rs b/swarm-controller/src/forge/legacy_tokens.rs index 917ab5e3..ead41410 100644 --- a/swarm-controller/src/forge/legacy_tokens.rs +++ b/swarm-controller/src/forge/legacy_tokens.rs @@ -113,8 +113,6 @@ mod tests { use std::collections::BTreeMap; use std::sync::{Arc, Mutex}; - use forgejo_api::{Auth, Forgejo}; - use super::super::agent_token::{AGENT_TOKEN_NAME, MintReason}; use super::*; @@ -229,8 +227,7 @@ mod tests { 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 }, deletes) + (Client::with_token("stub-token", url).unwrap(), deletes) } const MIX: [&str; 5] = [ diff --git a/swarm-controller/src/review_classifier.rs b/swarm-controller/src/review_classifier.rs index 384e870d..5a5e33c2 100644 --- a/swarm-controller/src/review_classifier.rs +++ b/swarm-controller/src/review_classifier.rs @@ -401,8 +401,9 @@ pub(crate) async fn get_try_out( 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 \ + (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`, \ @@ -457,7 +458,7 @@ async fn rules( .ok_or_else(|| { error_problem( StatusCode::NOT_FOUND, - &format!("{repo} has no commit, so no rules file"), + &format!("{repo} has no commit on its default branch, so no rules file"), ) })?; let [yml, json] = RULES_PATHS; @@ -698,7 +699,8 @@ pub(crate) mod tests { /// 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. + /// has no commit, `o/down` fails with something other than a 404, and + /// `o/hang` never answers. #[derive(Clone)] pub(crate) struct Fake { reply: Reply, @@ -743,6 +745,7 @@ pub(crate) mod tests { })), ("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(), ), @@ -1073,6 +1076,8 @@ classifications: 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() @@ -1122,6 +1127,11 @@ classifications: 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()]), &[]), @@ -1169,6 +1179,9 @@ classifications: 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 [ @@ -1184,7 +1197,7 @@ classifications: "?repo=o/empty", Vec::new(), StatusCode::NOT_FOUND, - "no commit", + "no commit on its default branch", ), ("?repo=o/r", Vec::new(), StatusCode::NOT_FOUND, "neither"), ( @@ -1205,6 +1218,12 @@ classifications: 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; @@ -1213,4 +1232,25 @@ classifications: 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); + } }