From 49b15e7775daaebc0d23214b07833afd6691ba1c Mon Sep 17 00:00:00 2001 From: atlas Date: Wed, 14 Oct 2026 15:33:49 +0200 Subject: [PATCH] review try-out: open a PR with the classifications on the page The try-out page gets an "open a PR" button. It posts the page's classifications to the new POST /api/review-classifier/open-pr, which merges them by name into the repo's rules file on the default branch and has the review-classifier forge account commit the result on a new branch, review-classifier/-<8 hex>, through the contents API. The commit's author is the forge account named like the session's Remote-User, read with the controller's admin token. The answer links to the forge's compare page, where the user opens the PR as themselves. The push uses the account's existing token from the store (swarm/services/review-classifier/forge/token, scope write:repository), so the account needs write access to the repo; a refused push answers 403 naming that. In a YAML rules file, the entries the merge leaves alone keep their text, as long as the result reads back as the merged list; otherwise the file is written out whole. DefaultBranchFiles now carries each file's blob sha, which the forge needs to replace a file, and the repo's web URL for the link. Fixes #5259 Refs #5096 --- docs/swarm/review-classifier.md | 57 +- docs/swarm/ui.md | 15 +- .../pages/review-tryout/ReviewTryOutPage.tsx | 78 +- .../src/pages/review-tryout/TryOutTypes.ts | 14 +- swarm-controller/README.md | 4 +- swarm-controller/src/forge.rs | 44 +- .../src/forge/classifier_account.rs | 188 +++- swarm-controller/src/main.rs | 1 + swarm-controller/src/review_classifier.rs | 46 +- .../src/review_classifier/open_pr.rs | 837 ++++++++++++++++++ 10 files changed, 1231 insertions(+), 53 deletions(-) create mode 100644 swarm-controller/src/review_classifier/open_pr.rs diff --git a/docs/swarm/review-classifier.md b/docs/swarm/review-classifier.md index 46f6662b..d31f1e6f 100644 --- a/docs/swarm/review-classifier.md +++ b/docs/swarm/review-classifier.md @@ -241,8 +241,8 @@ its one token, `swarm-review-classifier` with scope `write:repository`, into the store at `swarm/services/review-classifier/forge/token`. A pass at start and every five minutes re-mints when the token is missing or no longer matches the forge. The controller refuses an existing account by that name -with any other email. The controller posts the reviews with that token, and -the classifier service never sees it. +with any other email. The controller posts the reviews and pushes the try-out +branches with that token, and the classifier service never sees it. **Giving a repo access is a repo admin's step, by hand.** Nothing grants it automatically. In the repo's settings, add `review-classifier` as a @@ -254,7 +254,10 @@ collaborator: its request-changes blocks the merge where the branch protection blocks on rejected reviews. Where the branch protection restricts approvals to an allowlist, the account counts only once it's on that list, and read access - is then enough. + is then enough for reviews. + +The try-out page's **open a PR** pushes a branch to the repo itself, so it +needs **write** whatever the branch protection says. The repo also needs a rules file on its default branch (see [Repo configuration](#repo-configuration)). Without one, a request posts a @@ -464,10 +467,46 @@ since a run also works on a merged or closed PR. Two routes feed them: 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 -that branch, where you log in as usual and open the PR yourself. +When it's ready, **open a PR** (`POST /api/review-classifier/open-pr`, with +the repo and the classifications) has the service account push a branch that +adds the classifications to the repo's rules, with you, the swarm UI user, as +commit author. The page then links you to the forge's compare page for that +branch, where you log in as usual and open the PR yourself. + +- **The branch** is `review-classifier/-<8 hex digits>`, made from the + default branch's head with one commit, through the forge's contents API. +- **The file** is the rules file on the default branch, merged by name: a + classification named like an entry replaces it, a new name goes at the end, + and every other entry stays. A repo with no rules file gets + `.hyperhive/review-classifier.yml`. In a YAML file, the entries the merge + leaves alone keep their text, comments included. The merged file has to + pass the same checks the `rules` route makes. +- **The author** is the forge account named like your swarm UI login. A forge + account made by an SSO login carries the SSO username, so it's yours once + you've logged in to the forge. The commit carries its full name, or the + login when it has none, and its email. The committer is the service + account. +- **The token** is the service account's own, read from the store for each + push (see [The service account](#the-service-account)), so the account + needs **write** access to the repo. + +The route answers **201** with `branch`, `path` (the file written) and +`compare_url`, or: + +- **400** for a body the try-out POST would refuse in its repo or + classifications +- **401** without the `Remote-User` header the gateway sets +- **403** when the forge refuses the push: the service account isn't a + collaborator with write access +- **404** for a repo the forge doesn't know, or one with no commit on its + default branch +- **409** when the forge has no account named like you yet, or the rules + file changed on the default branch during the push +- **422** for a default branch holding both rules files or one the `rules` + route refuses; classifications the file already holds unchanged; or a + merge that breaks a rule, such as more than ten classifications +- **503** when this host has no forge token, the service account's token + isn't minted yet, or the forge failed or didn't answer within two minutes ## Run store and retention @@ -575,7 +614,3 @@ These get settled, and filled in here, as the work reaches them: `secret/swarm/services/review-classifier/forge/token`. - **Reviewer handoff.** How a later reviewer, human or agent, learns the classifier passed a PR before it starts its own review. -- **Try-out branch access.** The service account's access for pushing try-out - branches, beyond what a repo admin gives it for reviews. -- **Commit author for try-out PRs.** How the classifier derives the commit - author's name and email from the swarm UI login. diff --git a/docs/swarm/ui.md b/docs/swarm/ui.md index 4bcddfc7..6275e3a7 100644 --- a/docs/swarm/ui.md +++ b/docs/swarm/ui.md @@ -112,10 +112,21 @@ and the head commit the run read. A failed run shows its error, for example when has no API key yet. What you typed stays after a run, so you can edit the classification and run it -again. The page saves no classification and no result, and posts nothing to the -forge; like every table in the UI, the result table remembers its column +again. The page saves no classification and no result, and a run posts nothing +to the forge; like every table in the UI, the result table remembers its column filters in the browser. It shows no token counts, cache counts or cost. +When a classification answers the way you want, **open a PR** proposes the +classifications on the page to the repo. The classifier's forge account pushes +a branch that adds them to the repo's rules file, with you as the commit's +author, and the page links to the forge's page for opening the PR from that +branch, where you log in as usual. A classification named like one the file +already holds replaces it, and the file's other rules stay. It needs no PR +number. If the forge has no account of yours yet, log in to the forge once +first. If the classifier's account may not push to the repo, the page says so: +a repo admin adds it as a collaborator with write access +([the service account](review-classifier.md#the-service-account)). + ### Classifier settings Under the try-out form, **classifier settings** shows the runtime, model and diff --git a/frontend/packages/swarm-ui/src/pages/review-tryout/ReviewTryOutPage.tsx b/frontend/packages/swarm-ui/src/pages/review-tryout/ReviewTryOutPage.tsx index 879299d3..07bd804b 100644 --- a/frontend/packages/swarm-ui/src/pages/review-tryout/ReviewTryOutPage.tsx +++ b/frontend/packages/swarm-ui/src/pages/review-tryout/ReviewTryOutPage.tsx @@ -13,13 +13,18 @@ // **load the repo's rules** fills the rows from the rules on the repo's default // branch (`GET /api/review-classifier/rules`), to run as they are or to edit. // +// **open a PR** posts the rows to `POST /api/review-classifier/open-pr`, which +// has the classifier's forge account push a branch adding them to the repo's +// rules file, with the operator as commit author, and links to the forge's page +// for opening the PR from that branch. +// // Under the form, the classifier's own settings: its model, effort and review // concurrency (`ClassifierSettings`). // // The inputs stay as typed after a run, so the operator can edit and run // again. Apart from the settings, the page keeps nothing you typed and no -// result, and posts nothing to the forge; the only thing the browser remembers -// is the result table's column filters, as for every table in the UI. +// result, and a run posts nothing to the forge; the only thing the browser +// remembers is the result table's column filters, as for every table in the UI. import { useEffect, useState } from "preact/hooks"; import { ApiErrorPanel } from "@hive/shared/api-error-panel.js"; import { readApiError, type ProblemDetails } from "@hive/shared/api-error.js"; @@ -39,6 +44,7 @@ import { repoProblem, toBody, type ClassificationDraft, + type OpenPrAnswer, type OpenPull, type RulesAnswer, type TryOutResult as Result, @@ -47,6 +53,7 @@ import "./ReviewTryOutPage.css"; const TRY_OUT = "/api/review-classifier/try-out"; const RULES = "/api/review-classifier/rules"; +const OPEN_PR = "/api/review-classifier/open-pr"; const PULL_REPOS = "/api/pulls/repos"; const PULLS = "/api/pulls"; // How long the repo field rests before its PRs are fetched, so typing a repo @@ -91,6 +98,12 @@ export function ReviewTryOutPage() { ); const [confirmLoad, setConfirmLoad] = useState(false); + const [opening, setOpening] = useState(false); + const [opened, setOpened] = useState< + (OpenPrAnswer & { repo: string }) | null + >(null); + const [openError, setOpenError] = useState(null); + // What the two fields offer to complete to. The fields stay free text, so a // repo or a PR the lists lack, a merged one say, can still be typed. const [repoChoices, setRepoChoices] = useState([]); @@ -205,12 +218,15 @@ export function ReviewTryOutPage() { }; }, [runId]); - const problem = + // What keeps the rows from going to the repo; a PR number is not needed. + const rowsProblem = repoProblem(repo) ?? - (prNumber(pr) === null ? "a PR number is a whole number above 0" : null) ?? drafts.map(draftProblem).find((p) => p !== null) ?? duplicateName(drafts) ?? null; + const problem = + (prNumber(pr) === null ? "a PR number is a whole number above 0" : null) ?? + rowsProblem; async function run(e: Event) { e.preventDefault(); @@ -264,6 +280,34 @@ export function ReviewTryOutPage() { } } + // Pushes the rows on a new branch of the repo and keeps the link to open + // the PR from it. + async function openPr() { + const target = repo.trim(); + setOpening(true); + setOpened(null); + setOpenError(null); + try { + const res = await fetch(OPEN_PR, { + method: "POST", + headers: { "content-type": "application/json" }, + body: JSON.stringify({ + repo: target, + classifications: drafts.map(toBody), + }), + }); + if (!res.ok) { + setOpenError(await readApiError(res)); + return; + } + setOpened({ ...((await res.json()) as OpenPrAnswer), repo: target }); + } catch (err: unknown) { + setOpenError({ detail: String(err) }); + } finally { + setOpening(false); + } + } + // Typed rows are the operator's work, so replacing them is asked first. function askLoadRules() { if (hasTypedText(drafts)) setConfirmLoad(true); @@ -279,9 +323,9 @@ export function ReviewTryOutPage() {

Run a review classification against a real PR and see how the model - answers it. Nothing is posted to the forge, and nothing is saved: + answers it. A run posts nothing to the forge, and nothing is saved: change the classification and run it again until it answers the way - you want. + you want. Then open a PR to add it to the repo's rules.

void run(e)}>
@@ -364,8 +408,30 @@ export function ReviewTryOutPage() { > {running ? "running…" : "run"} +
{problem !== null ?

{problem}

: null} + {openError ? ( + + ) : null} + {opened ? ( +

+ Pushed {opened.branch} to {opened.repo}, changing{" "} + {opened.path} with you as the author.{" "} + + Open the PR on the forge + + . +

+ ) : null}
)>, + /// The repo's page on the forge, as a browser reaches it. + pub web_url: Option, + /// Each asked-for path that exists there, in the order asked. + pub files: Vec, +} + +/// One file of [`DefaultBranchFiles`]. +#[derive(Clone, Debug, PartialEq, Eq)] +pub struct BranchFile { + pub path: String, + /// The blob's sha, which the forge asks for to replace the file. + pub sha: String, + pub bytes: Vec, } /// One row of [`Client::issue_report`] — an open issue plus the two facts @@ -1345,19 +1355,26 @@ impl Client { 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)); + if let Some((sha, bytes)) = self.file_at(owner, repo, &commit, path, &mut trees).await? + { + files.push(BranchFile { + path: (*path).to_owned(), + sha, + bytes, + }); } } Ok(Some(DefaultBranchFiles { branch, commit, + web_url: info.html_url, 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. + /// The blob sha and bytes of 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, @@ -1365,7 +1382,7 @@ impl Client { commit: &str, path: &str, trees: &mut HashMap>, - ) -> Result>> { + ) -> Result)>> { let mut tree = commit.to_owned(); let mut segments = path.split('/').peekable(); while let Some(name) = segments.next() { @@ -1383,7 +1400,10 @@ impl Client { return Ok(None); }; if kind == "blob" { - return self.blob(owner, repo, &sha).await.map(Some); + return self + .blob(owner, repo, &sha) + .await + .map(|bytes| Some((sha, bytes))); } tree = sha; } @@ -2434,7 +2454,11 @@ mod tests { assert_eq!(read.commit, "def456"); assert_eq!( read.files, - [(RULES_YML.to_owned(), b"classifications: []\n".to_vec())] + [BranchFile { + path: RULES_YML.to_owned(), + sha: "b1".to_owned(), + bytes: b"classifications: []\n".to_vec(), + }] ); // Each tree is read once for both paths. let seen = seen.lock().unwrap(); diff --git a/swarm-controller/src/forge/classifier_account.rs b/swarm-controller/src/forge/classifier_account.rs index c1f68153..df4b03e8 100644 --- a/swarm-controller/src/forge/classifier_account.rs +++ b/swarm-controller/src/forge/classifier_account.rs @@ -3,11 +3,14 @@ //! `swarm_secret_client::review_classifier::forge_token_path`. Requesting //! this account as a reviewer starts a classifier run, and this daemon posts //! the result as the account's review (`crate::review_classifier::review`). -//! The classifier itself never holds the token. +//! This daemon also pushes the try-out page's branches as the account +//! (`crate::review_classifier::open_pr`). The classifier itself never holds +//! the token. //! //! The account gets no repo access here. A repo admin adds it to a repo by //! hand: read to request it as a reviewer, write (or a place on the branch -//! protection's approvals whitelist) for its approval to count. +//! protection's approvals whitelist) for its approval to count, and write to +//! push a branch. //! //! The token decision is [`super::agent_token`]'s, under this module's name //! and scopes, so a stored token the forge still lists with the same last @@ -17,13 +20,16 @@ use std::sync::Arc; use anyhow::{Context, Result, bail}; -use forgejo_api::structs::{CreatePullReviewOptions, RepoSearchQuery}; +use forgejo_api::structs::{ + ChangeFileOperation, ChangeFileOperationOperation, ChangeFilesOptions, CreatePullReviewOptions, + Identity, RepoSearchQuery, +}; use forgejo_api::{Auth, Forgejo}; use swarm_secret_client::forge; use swarm_secret_client::review_classifier::forge_token_path; use super::Client; -use super::agent_token::{Decision, Observed, classify_named}; +use super::agent_token::{Decision, Observed, classify_named, is_not_found}; /// The classifier's forge login: the reviewer someone requests. pub const LOGIN: &str = "review-classifier"; @@ -31,8 +37,8 @@ pub const LOGIN: &str = "review-classifier"; /// The forge's name for the account's one token. pub const TOKEN_NAME: &str = "swarm-review-classifier"; -/// What the token may do: post reviews, which the forge files under the -/// repository scope. Which repos is the account's access, granted by hand. +/// What the token may do: post reviews and push branches, both of which the +/// forge files under the repository scope. Which repos is the account's access, granted by hand. pub const TOKEN_SCOPES: &str = "write:repository"; /// How often [`spawn`] re-checks the account and its token. @@ -62,6 +68,29 @@ pub struct Review { pub body: String, } +/// Who a commit names as its author. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct CommitAuthor { + pub name: String, + pub email: String, +} + +/// One file written as one commit on a new branch. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct BranchCommit { + /// The branch the new one starts from, at its head. + pub base: String, + /// Must not exist yet: the forge refuses the commit when it does. + pub branch: String, + pub path: String, + /// The blob sha of the file on `base` it replaces; `None` creates it. + /// The forge refuses the commit when `base` no longer holds that blob. + pub sha: Option, + pub content: Vec, + pub message: String, + pub author: CommitAuthor, +} + impl Client { /// Make sure [`LOGIN`] exists and holds a live [`TOKEN_NAME`] token in /// the store, minting one when it does not. The whole job of the @@ -121,19 +150,8 @@ impl Client { commit: Option<&str>, review: Review, ) -> Result> { - let path = forge_token_path()?; - let store = crate::store::connect() - .await - .context("logging in to the swarm secret store")?; - let Some(credential) = store - .read_optional::(&path) - .await - .with_context(|| format!("reading {path}"))? - else { - bail!("nothing at {path}: the classifier's forge token is not minted yet"); - }; - Forgejo::new(Auth::Token(&credential.value), self.url.clone()) - .context("building a forge client with the classifier's token")? + self.as_classifier() + .await? .repo_create_pull_review( owner, repo, @@ -150,6 +168,101 @@ impl Client { .map(|posted| posted.html_url.map(String::from)) } + /// Make `commit` as [`LOGIN`]: one commit on a new branch, from the head + /// of its base. Reads the token from the store on every call. + /// + /// # Errors + /// When the store holds no token yet, or the forge refuses the commit; + /// a [`forgejo_api::ForgejoError`] in the chain carries the forge's + /// status. + pub async fn push_classifier_branch( + &self, + owner: &str, + repo: &str, + commit: BranchCommit, + ) -> Result<()> { + use base64::Engine as _; + + let branch = commit.branch.clone(); + self.as_classifier() + .await? + .repo_change_files( + owner, + repo, + ChangeFilesOptions { + author: Some(Identity { + name: Some(commit.author.name), + email: Some(commit.author.email), + }), + branch: Some(commit.base), + committer: None, + dates: None, + files: vec![ChangeFileOperation { + content: Some( + base64::engine::general_purpose::STANDARD.encode(&commit.content), + ), + from_path: None, + operation: if commit.sha.is_some() { + ChangeFileOperationOperation::Update + } else { + ChangeFileOperationOperation::Create + }, + path: commit.path, + sha: commit.sha, + }], + force_overwrite_new_branch: Some(false), + message: Some(commit.message), + new_branch: Some(commit.branch), + signoff: None, + }, + ) + .await + .with_context(|| format!("pushing {LOGIN}'s branch {branch} to {owner}/{repo}"))?; + Ok(()) + } + + /// The forge account `login` as a commit author: its full name, or its + /// login when it has none, and its email. `None` when the forge has no + /// such account. Read with this daemon's site-admin token, which sees an + /// email the account keeps private. + /// + /// # Errors + /// When the forge refuses the read, or the account has no email. + pub async fn commit_author(&self, login: &str) -> Result> { + let user = match self.api.call(|api| api.user_get(login).into_future()).await { + Ok(user) => user, + Err(e) if is_not_found(&e) => return Ok(None), + Err(e) => return Err(e).with_context(|| format!("read forge user {login}")), + }; + let email = user + .email + .filter(|e| !e.trim().is_empty()) + .with_context(|| format!("forge user {login} has no email"))?; + let name = user + .full_name + .map(|n| n.trim().to_owned()) + .filter(|n| !n.is_empty()) + .unwrap_or_else(|| login.to_owned()); + Ok(Some(CommitAuthor { name, email })) + } + + /// A forge client acting as [`LOGIN`], with the token the store holds. + async fn as_classifier(&self) -> Result { + let path = forge_token_path()?; + let store = crate::store::connect() + .await + .context("logging in to the swarm secret store")?; + let Some(credential) = store + .read_optional::(&path) + .await + .with_context(|| format!("reading {path}"))? + else { + bail!("nothing at {path}: the classifier's forge token is not minted yet"); + }; + Forgejo::new(Auth::Token(&credential.value), self.url.clone()) + .context("building a forge client with the classifier's token") + } + /// The stored token at `path` and every token the forge lists for /// [`LOGIN`], which must exist. async fn read_classifier_token( @@ -306,6 +419,43 @@ mod tests { assert!(lockdown_patch(&seen, LOGIN).is_none()); } + #[tokio::test] + async fn a_commit_author_is_the_accounts_name_and_email_or_none() { + use super::super::tests::stub_forge; + + let (client, _) = stub_forge( + (404, "{}"), + ( + 200, + r#"{"login":"alice","full_name":" Alice A ","email":"a@x.test"}"#, + ), + (404, "{}"), + ) + .await; + assert_eq!( + client.commit_author("alice").await.expect("read"), + Some(CommitAuthor { + name: "Alice A".to_owned(), + email: "a@x.test".to_owned(), + }) + ); + let (client, _) = stub_forge( + (404, "{}"), + (200, r#"{"login":"bob","full_name":"","email":"b@x.test"}"#), + (404, "{}"), + ) + .await; + let bob = client.commit_author("bob").await.expect("read"); + assert_eq!(bob.map(|a| a.name).as_deref(), Some("bob")); + let (client, _) = stub_forge( + (404, "{}"), + (404, r#"{"message":"user does not exist"}"#), + (404, "{}"), + ) + .await; + assert_eq!(client.commit_author("nobody").await.expect("read"), None); + } + #[tokio::test] async fn the_job_locks_down_the_classifiers_own_account() { use super::super::tests::{lockdown_patch, stub_forge}; diff --git a/swarm-controller/src/main.rs b/swarm-controller/src/main.rs index 8224ba5d..a8b05110 100644 --- a/swarm-controller/src/main.rs +++ b/swarm-controller/src/main.rs @@ -3420,6 +3420,7 @@ fn build_app(state: AppState) -> axum::Router { .routes(routes!(review_classifier::post_try_out)) .routes(routes!(review_classifier::get_try_out)) .routes(routes!(review_classifier::get_rules)) + .routes(routes!(review_classifier::open_pr::post_open_pr)) .routes(routes!(review_classifier::runs::get_runs)) .routes(routes!( review_classifier::settings::get_settings, diff --git a/swarm-controller/src/review_classifier.rs b/swarm-controller/src/review_classifier.rs index b0eb28f9..82ea7982 100644 --- a/swarm-controller/src/review_classifier.rs +++ b/swarm-controller/src/review_classifier.rs @@ -2,7 +2,8 @@ //! `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 rules file on its default branch. [`open_pr`] puts classifications on +//! a new branch of the repo, for a PR. //! //! 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: @@ -39,6 +40,7 @@ use self::settings::{Settings, SettingsSource}; use crate::forge::{self, DefaultBranchFiles, PullForReview}; use crate::{AppState, error_problem}; +pub(crate) mod open_pr; pub(crate) mod review; pub(crate) mod runs; pub(crate) mod settings; @@ -496,7 +498,7 @@ async fn rules( ), )); } - [file] => file, + [file] => (&file.path, &file.bytes), _ => { return Err(error_problem( StatusCode::UNPROCESSABLE_ENTITY, @@ -765,17 +767,23 @@ pub(crate) mod tests { use tower::ServiceExt as _; use super::*; + use crate::forge::BranchFile; + use crate::forge::classifier_account::{BranchCommit, CommitAuthor}; /// A forge holding one PR, `o/r` 7, with `files` on `o/r`'s default /// branch, `settings` stored, and a classifier answering every run with /// `reply` and keeping its request. `o/empty` has no commit, `o/down` /// fails with something other than a 404, and `o/hang` never answers. + /// The forge has the accounts `alice` and `bob`, and keeps each branch + /// pushed with its repo, or refuses it with `push_refusal`. #[derive(Clone)] pub(crate) struct Fake { reply: Reply, files: Vec<(&'static str, String)>, pub(crate) settings: Settings, pub(crate) sent: Arc>>, + pub(crate) pushed: Arc>>, + pub(crate) push_refusal: Option, } impl Fake { @@ -785,10 +793,37 @@ pub(crate) mod tests { files, settings: Settings::default(), sent: Arc::default(), + pushed: Arc::default(), + push_refusal: None, } } } + impl open_pr::BranchSink for Fake { + async fn commit_author(&self, login: &str) -> anyhow::Result> { + let name = match login { + "alice" => "Alice", + "bob" => "bob", + _ => return Ok(None), + }; + Ok(Some(CommitAuthor { + name: name.to_owned(), + email: format!("{login}@example.test"), + })) + } + + async fn push(&self, owner: &str, repo: &str, commit: BranchCommit) -> anyhow::Result<()> { + if let Some(status) = self.push_refusal { + return Err(ForgejoError::UnexpectedStatusCode(status).into()); + } + self.pushed + .lock() + .expect("unpoisoned") + .push((format!("{owner}/{repo}"), commit)); + Ok(()) + } + } + impl SettingsSource for Fake { async fn settings(&self) -> anyhow::Result { Ok(self.settings.clone()) @@ -825,10 +860,15 @@ pub(crate) mod tests { ("o", "r") => Ok(Some(DefaultBranchFiles { branch: "main".to_owned(), commit: "def456".to_owned(), + web_url: Some(url::Url::parse("https://forge.test/o/r").expect("a URL")), files: paths .iter() .filter_map(|p| self.files.iter().find(|(f, _)| f == p)) - .map(|(f, body)| ((*f).to_owned(), body.clone().into_bytes())) + .map(|(f, body)| BranchFile { + path: (*f).to_owned(), + sha: format!("sha-{f}"), + bytes: body.clone().into_bytes(), + }) .collect(), })), ("o", "empty") => Ok(None), diff --git a/swarm-controller/src/review_classifier/open_pr.rs b/swarm-controller/src/review_classifier/open_pr.rs new file mode 100644 index 00000000..03ba680a --- /dev/null +++ b/swarm-controller/src/review_classifier/open_pr.rs @@ -0,0 +1,837 @@ +//! `POST /api/review-classifier/open-pr`: put classifications written on the +//! try-out page on a new branch of the repo, for the operator to open a PR +//! from. +//! +//! The classifications are merged by trimmed name into the rules file on the +//! repo's default branch: one with a name the file holds replaces that entry, +//! one with a new name goes at the end, and every other entry stays. A repo +//! with no rules file gets `.hyperhive/review-classifier.yml`. In a YAML file, +//! the entries the merge leaves alone keep their text, comments included. +//! +//! The commit is made by the classifier's forge account +//! ([`LOGIN`]) with its own token, on a branch of the repo itself, so that +//! account needs write access to the repo. Its author is the operator: the +//! `Remote-User` the gateway sets names the forge account of the same login, +//! since a forge account made by an SSO login is named after the SSO user. +//! The answer links to the forge's compare page for the branch, where the +//! operator opens the PR as themselves. + +use std::fmt::Write as _; +use std::future::Future; + +use axum::Json; +use axum::extract::State; +use axum::extract::rejection::JsonRejection; +use axum::http::{HeaderMap, StatusCode}; +use forgejo_api::{ApiErrorKind, ForgejoError}; +use problem_details::ProblemDetails; +use serde::{Deserialize, Serialize}; +use swarm_queue_client::review_classifier::{Classification, Polarity}; +use utoipa::ToSchema; + +use super::{FileSource, RULES_PATHS, check_classifications, operator, parse_rules, split_repo}; +use crate::forge::classifier_account::{BranchCommit, CommitAuthor, LOGIN}; +use crate::forge::{self, BranchFile}; +use crate::{AppState, error_problem}; + +/// Where a branch's name starts, so the forge's branch list groups them. +const BRANCH_PREFIX: &str = "review-classifier/"; + +/// The longest a line of a folded YAML description gets, indent included. +const YAML_WIDTH: usize = 78; + +/// Classifications to propose to a repo. +#[derive(Debug, Deserialize, ToSchema)] +pub(crate) struct OpenPrRequest { + /// `owner/name`. + repo: String, + /// One to ten, each with a distinct name. + classifications: Vec, +} + +/// The branch the commit went on. +#[derive(Debug, Serialize, ToSchema)] +pub(crate) struct OpenPrBranch { + branch: String, + /// The rules file the commit wrote. + path: String, + /// The forge's page comparing `branch` with the default branch, where + /// the PR is opened. + compare_url: String, +} + +/// Where the commit is made: the forge in production, a fake in the tests. +pub(crate) trait BranchSink { + /// The forge account `login` as a commit author, or `None` when the + /// forge has none. + fn commit_author( + &self, + login: &str, + ) -> impl Future>> + Send; + + fn push( + &self, + owner: &str, + repo: &str, + commit: BranchCommit, + ) -> impl Future> + Send; +} + +impl BranchSink for forge::Client { + async fn commit_author(&self, login: &str) -> anyhow::Result> { + forge::Client::commit_author(self, login).await + } + + async fn push(&self, owner: &str, repo: &str, commit: BranchCommit) -> anyhow::Result<()> { + self.push_classifier_branch(owner, repo, commit).await + } +} + +/// Put classifications on a new branch, authored by the operator. +#[utoipa::path( + post, + path = "/api/review-classifier/open-pr", + request_body = OpenPrRequest, + responses( + (status = 201, description = "the branch is on the forge; open the PR at \ + `compare_url`", body = OpenPrBranch), + (status = 400, description = "the body is not this shape, or fails a check the \ + try-out POST makes on its repo or classifications (problem+json)", body = String), + (status = 401, description = "the request carries no `Remote-User`, so there is no \ + author (problem+json)", body = String), + (status = 403, description = "the forge refused the classifier's account the push: \ + it is not a collaborator with write access on the repo (problem+json)", + body = String), + (status = 404, description = "the forge knows no such repo, or its default branch \ + has no commit (problem+json)", body = String), + (status = 409, description = "the forge has no account named like the operator, who \ + has to log in to it once; or the default branch's rules file changed while the \ + commit was made (problem+json)", body = String), + (status = 422, description = "the default branch holds both rules files, or one the \ + `rules` GET refuses; the rules already hold every classification as it is; or \ + the merged rules fail the try-out POST's checks, such as more than ten \ + (problem+json)", body = String), + (status = 503, description = "this host has no forge token yet, or the classifier's \ + account has none in the store; or the forge failed or did not answer within two \ + minutes (problem+json)", body = String), + ), + tag = "review-classifier" +)] +pub(crate) async fn post_open_pr( + State(state): State, + headers: HeaderMap, + body: Result, JsonRejection>, +) -> Result<(StatusCode, Json), ProblemDetails> { + let operator = 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 Json(req) = body.map_err(|e| error_problem(StatusCode::BAD_REQUEST, &e.body_text()))?; + let (owner, name) = + split_repo(&req.repo).map_err(|d| error_problem(StatusCode::BAD_REQUEST, &d))?; + check_classifications(&req.classifications) + .map_err(|d| error_problem(StatusCode::BAD_REQUEST, &d))?; + #[cfg(test)] + if let Some(fake) = state.try_outs.fake.clone() { + return open(&*fake, &operator, owner, name, req.classifications).await; + } + let Some(forge) = state.forge.get() else { + return Err(error_problem( + StatusCode::SERVICE_UNAVAILABLE, + "no forge access on this host yet", + )); + }; + open(forge.as_ref(), &operator, owner, name, req.classifications).await +} + +/// Resolve the author, merge the rules and push the branch. +async fn open( + forge: &F, + operator: &str, + owner: &str, + name: &str, + rows: Vec, +) -> Result<(StatusCode, Json), ProblemDetails> { + let repo = format!("{owner}/{name}"); + let author = deadline(forge.commit_author(operator)) + .await? + .map_err(|e| forge_problem(&e, &repo))? + .ok_or_else(|| { + error_problem( + StatusCode::CONFLICT, + &format!( + "the forge has no account {operator}, to name as the commit's author: log in \ + to the forge once, then try again" + ), + ) + })?; + let read = deadline(forge.default_branch_files(owner, name, &RULES_PATHS)) + .await? + .map_err(|e| forge_problem(&e, &repo))? + .ok_or_else(|| { + error_problem( + StatusCode::NOT_FOUND, + &format!("{repo} has no commit on its default branch to branch from"), + ) + })?; + let existing = match read.files.as_slice() { + [] => None, + [file] => Some(file), + _ => { + return Err(error_problem( + StatusCode::UNPROCESSABLE_ENTITY, + &format!( + "{repo}'s default branch {} holds both rules files: keep one", + read.branch + ), + )); + } + }; + let merged = + merge(existing, rows).map_err(|d| error_problem(StatusCode::UNPROCESSABLE_ENTITY, &d))?; + let web_url = read.web_url.ok_or_else(|| { + error_problem( + StatusCode::SERVICE_UNAVAILABLE, + &format!("the forge named no web page for {repo}"), + ) + })?; + let branch = + branch_name(operator).map_err(|e| error_problem(StatusCode::INTERNAL_SERVER_ERROR, &e))?; + let compare_url = format!( + "{}/compare/{}...{branch}", + web_url.as_str().trim_end_matches('/'), + read.branch + ); + let commit = BranchCommit { + base: read.branch, + branch: branch.clone(), + path: merged.path.clone(), + sha: merged.sha, + content: merged.content, + message: merged.message, + author, + }; + deadline(forge.push(owner, name, commit)) + .await? + .map_err(|e| push_problem(&e, &repo))?; + tracing::info!(%repo, %branch, %operator, path = %merged.path, "review classifier branch pushed"); + Ok(( + StatusCode::CREATED, + Json(OpenPrBranch { + branch, + path: merged.path, + compare_url, + }), + )) +} + +async fn deadline(work: impl Future) -> Result { + forge::within_forge_deadline("review classifier branch", work) + .await + .ok_or_else(|| { + error_problem( + StatusCode::SERVICE_UNAVAILABLE, + "the forge did not answer within the deadline", + ) + }) +} + +/// A fresh branch for `operator`: [`BRANCH_PREFIX`], their name in the +/// characters a ref takes without question, and eight random hex digits. +fn branch_name(operator: &str) -> Result { + let mut bytes = [0u8; 4]; + getrandom::fill(&mut bytes).map_err(|e| format!("drawing a branch name: {e}"))?; + let who: String = operator + .chars() + .map(|c| { + if c.is_ascii_alphanumeric() { + c.to_ascii_lowercase() + } else { + '-' + } + }) + .collect(); + let mut name = format!("{BRANCH_PREFIX}{who}-"); + for b in bytes { + let _ = write!(name, "{b:02x}"); + } + Ok(name) +} + +/// 404 for a repo the forge doesn't know, 503 for anything else. +fn forge_problem(e: &anyhow::Error, repo: &str) -> ProblemDetails { + if forge_status(e) == Some(StatusCode::NOT_FOUND) { + return error_problem(StatusCode::NOT_FOUND, &format!("no repo {repo}")); + } + let detail = format!("{e:#}"); + tracing::warn!(error = %detail, %repo, "preparing a review classifier branch failed"); + error_problem(StatusCode::SERVICE_UNAVAILABLE, &detail) +} + +/// 403 when the account may not push, 409 when the rules file or the +/// branch changed under the commit, 503 for anything else. +fn push_problem(e: &anyhow::Error, repo: &str) -> ProblemDetails { + let detail = format!("{e:#}"); + tracing::warn!(error = %detail, %repo, "pushing a review classifier branch failed"); + match forge_status(e) { + // The forge hides a repo the account can't read behind a 404. + Some(StatusCode::FORBIDDEN | StatusCode::NOT_FOUND) => error_problem( + StatusCode::FORBIDDEN, + &format!( + "{LOGIN} may not push to {repo}: a repo admin adds it as a collaborator with \ + write access ({detail})" + ), + ), + Some(StatusCode::CONFLICT | StatusCode::UNPROCESSABLE_ENTITY) => error_problem( + StatusCode::CONFLICT, + &format!("the default branch changed while the branch was made: try again ({detail})"), + ), + _ => error_problem(StatusCode::SERVICE_UNAVAILABLE, &detail), + } +} + +/// The forge's status, when a forge answer is what failed. +fn forge_status(e: &anyhow::Error) -> Option { + match e.downcast_ref::()? { + ForgejoError::ApiError(api) => match api.error_kind() { + ApiErrorKind::Forbidden | ApiErrorKind::Unauthorized => Some(StatusCode::FORBIDDEN), + ApiErrorKind::NotFound { .. } => Some(StatusCode::NOT_FOUND), + ApiErrorKind::ValidationFailed => Some(StatusCode::UNPROCESSABLE_ENTITY), + ApiErrorKind::Other(s) => Some(*s), + _ => None, + }, + ForgejoError::UnexpectedStatusCode(s) => Some(*s), + _ => None, + } +} + +/// The rules file a commit writes. +#[derive(Debug)] +struct Merged { + path: String, + /// The blob it replaces; `None` when the repo has no rules file. + sha: Option, + content: Vec, + message: String, +} + +/// `rows` merged into `existing`, or why they can't be. +fn merge(existing: Option<&BranchFile>, rows: Vec) -> Result { + let [yml, _] = RULES_PATHS; + let (path, sha, text, current) = match existing { + Some(file) => ( + file.path.clone(), + Some(file.sha.clone()), + Some(String::from_utf8_lossy(&file.bytes).into_owned()), + parse_rules(&file.path, &file.bytes)?, + ), + None => (yml.to_owned(), None, None, Vec::new()), + }; + let mut merged = current.clone(); + let (mut added, mut changed) = (Vec::new(), Vec::new()); + for row in rows { + let row = Classification { + name: row.name.trim().to_owned(), + description: row.description.trim().to_owned(), + ..row + }; + match merged.iter().position(|c| c.name.trim() == row.name) { + Some(i) if merged[i] == row => {} + Some(i) => { + changed.push(row.name.clone()); + merged[i] = row; + } + None => { + added.push(row.name.clone()); + merged.push(row); + } + } + } + if added.is_empty() && changed.is_empty() { + return Err(format!( + "{path} already holds every classification as it is" + )); + } + check_classifications(&merged).map_err(|e| format!("{path} with these merged: {e}"))?; + let content = if std::path::Path::new(&path) + .extension() + .is_some_and(|e| e == "json") + { + let mut json = serde_json::to_string_pretty(&serde_json::json!({ + "classifications": merged, + })) + .map_err(|e| format!("encoding {path}: {e}"))?; + json.push('\n'); + json + } else { + yaml(text.as_deref(), ¤t, &merged) + }; + let mut message = String::from("review classifier:"); + for (verb, names) in [("add", &added), ("update", &changed)] { + if !names.is_empty() { + let sep = if message.ends_with(':') { " " } else { "; " }; + let _ = write!(message, "{sep}{verb} {}", names.join(", ")); + } + } + Ok(Merged { + path, + sha, + content: content.into_bytes(), + message, + }) +} + +/// `merged` as a YAML rules file. Each entry of `text` that `merged` holds +/// unchanged at the same place keeps its text, as long as the result reads +/// back as `merged`; otherwise the whole file is written out. +fn yaml(text: Option<&str>, current: &[Classification], merged: &[Classification]) -> String { + if let Some((head, entries, indent)) = text.and_then(entry_lines) { + let mut out = head; + for (i, c) in merged.iter().enumerate() { + match entries.get(i) { + Some(kept) if current.get(i) == Some(c) => out.push_str(kept), + _ => { + if !out.is_empty() && !out.ends_with('\n') { + out.push('\n'); + } + out.push_str(&yaml_entry(c, &indent)); + } + } + } + if parse_rules(".yml", out.as_bytes()).as_deref() == Ok(merged) { + return out; + } + } + let mut out = String::from("classifications:\n"); + for c in merged { + out.push_str(&yaml_entry(c, " ")); + } + out +} + +/// `text` split into what comes before the first entry of its +/// `classifications:` list, the text of each entry up to the next one, and +/// the indent of their `-`. `None` when no entry follows that key. The split +/// is only a guess at the layout: [`yaml`] keeps it when what it builds reads +/// back as it should. +fn entry_lines(text: &str) -> Option<(String, Vec, String)> { + let lines: Vec<&str> = text.split_inclusive('\n').collect(); + let key = lines + .iter() + .position(|l| l.trim_end() == "classifications:")?; + let first = key + + 1 + + lines[key + 1..] + .iter() + .position(|l| !l.trim().is_empty() && !l.trim_start().starts_with('#'))?; + let indent: String = lines[first].chars().take_while(|c| *c == ' ').collect(); + let mut entries: Vec = Vec::new(); + for line in &lines[first..] { + if line + .strip_prefix(indent.as_str()) + .is_some_and(|rest| rest.starts_with("- ")) + { + entries.push((*line).to_owned()); + } else { + entries.last_mut()?.push_str(line); + } + } + Some((lines[..first].concat(), entries, indent)) +} + +/// One classification as an entry of a YAML block list, its `-` at +/// `indent`. +fn yaml_entry(c: &Classification, indent: &str) -> String { + let polarity = match c.polarity { + Polarity::Positive => "positive", + Polarity::Negative => "negative", + }; + let inner = format!("{indent} "); + let mut out = format!("{indent}- name: {}\n", yaml_name(&c.name)); + if foldable(&c.description) { + let _ = writeln!(out, "{inner}description: >-"); + let lead = format!("{inner} "); + let mut line = String::new(); + for word in c.description.split(' ') { + if !line.is_empty() && lead.len() + line.len() + 1 + word.len() > YAML_WIDTH { + let _ = writeln!(out, "{lead}{line}"); + line.clear(); + } + if !line.is_empty() { + line.push(' '); + } + line.push_str(word); + } + let _ = writeln!(out, "{lead}{line}"); + } else { + let _ = writeln!(out, "{inner}description: {}", json_string(&c.description)); + } + let _ = writeln!(out, "{inner}polarity: {polarity}"); + let _ = writeln!(out, "{inner}threshold: {}", c.threshold); + out +} + +/// `name` bare when it can't read as anything but that string, quoted +/// otherwise. +fn yaml_name(name: &str) -> String { + let bare = name.starts_with(|c: char| c.is_ascii_lowercase()) + && name + .chars() + .all(|c| c.is_ascii_lowercase() || c.is_ascii_digit() || c == '-') + && !matches!( + name, + "y" | "n" | "yes" | "no" | "on" | "off" | "true" | "false" | "null" + ); + if bare { + name.to_owned() + } else { + json_string(name) + } +} + +/// Whether a folded block scalar reads back as exactly `s`, which is trimmed +/// and not empty: one line of printable text, words split by single spaces. +fn foldable(s: &str) -> bool { + !s.contains(" ") && !s.chars().any(char::is_control) +} + +/// `s` as a double-quoted scalar; JSON's escapes are YAML's too. +fn json_string(s: &str) -> String { + serde_json::Value::from(s).to_string() +} + +#[cfg(test)] +mod tests { + use std::sync::Arc; + + use axum::body::Body; + use axum::http::{Request, StatusCode}; + use swarm_queue_client::review_classifier::{Overall, Reply}; + use tower::ServiceExt as _; + + use super::super::tests::Fake; + use super::*; + + fn rule(name: &str, description: &str) -> Classification { + Classification { + name: name.to_owned(), + description: description.to_owned(), + polarity: Polarity::Negative, + threshold: 70, + } + } + + fn file(path: &str, text: &str) -> BranchFile { + BranchFile { + path: path.to_owned(), + sha: "b1".to_owned(), + bytes: text.as_bytes().to_vec(), + } + } + + const OWN: &str = include_str!("../../../.hyperhive/review-classifier.yml"); + + #[test] + fn a_new_rule_leaves_every_existing_entry_as_it_was_written() { + let [yml, _] = RULES_PATHS; + let before = parse_rules(yml, OWN.as_bytes()).expect("this repo's rules parse"); + let new = rule( + "no-todo", + "The diff adds a TODO or FIXME comment without a tracker reference next to it, \ + which leaves the follow-up with nobody to own it.", + ); + let merged = merge(Some(&file(yml, OWN)), vec![new.clone()]).expect("merges"); + let text = String::from_utf8(merged.content).expect("utf-8"); + assert!(text.starts_with(OWN), "{text}"); + assert_eq!(merged.sha.as_deref(), Some("b1")); + assert_eq!(merged.message, "review classifier: add no-todo"); + let after = parse_rules(yml, text.as_bytes()).expect("parses"); + assert_eq!(after, [before, vec![new.clone()]].concat()); + assert!(text.lines().all(|l| l.len() <= YAML_WIDTH), "{text}"); + + let unterminated = OWN.trim_end(); + let merged = merge(Some(&file(yml, unterminated)), vec![new]).expect("merges"); + let text = String::from_utf8(merged.content).expect("utf-8"); + assert!( + text.starts_with(&format!("{unterminated}\n - name: no-todo\n")), + "{text}" + ); + } + + #[test] + fn a_rule_with_a_known_name_replaces_that_entry_in_place() { + let [yml, json] = RULES_PATHS; + let mut before = parse_rules(yml, OWN.as_bytes()).expect("this repo's rules parse"); + let mut edited = before[1].clone(); + edited.threshold = 90; + let padded = Classification { + name: format!(" {} ", edited.name), + ..edited.clone() + }; + let merged = merge(Some(&file(yml, OWN)), vec![padded.clone()]).expect("merges"); + let after = parse_rules(yml, &merged.content).expect("parses"); + before[1] = edited.clone(); + assert_eq!(after, before); + assert_eq!( + merged.message, + format!("review classifier: update {}", edited.name) + ); + + let as_json = serde_json::json!({ "classifications": [&before[0], &before[2]] }); + let merged = merge( + Some(&file(json, &as_json.to_string())), + vec![padded, rule("x", "y")], + ) + .expect("merges"); + let after = parse_rules(json, &merged.content).expect("parses"); + assert_eq!( + after, + [ + before[0].clone(), + before[2].clone(), + edited.clone(), + rule("x", "y") + ] + ); + assert_eq!( + merged.message, + format!("review classifier: add {}, x", edited.name) + ); + } + + #[test] + fn no_rules_file_creates_the_yaml_one() { + let rules = vec![ + rule("no-ads", "No ads."), + rule("null", "N."), + rule("Yes: #1", "A \"quote\"."), + // A full line, then a double space where the wrap falls. + rule("spaced", &format!("{} y", "x".repeat(72))), + rule("broken", "A\nnewline."), + ]; + let merged = merge(None, rules.clone()).expect("merges"); + let [yml, _] = RULES_PATHS; + assert_eq!(merged.path, yml); + assert_eq!(merged.sha, None); + let after = parse_rules(yml, &merged.content).expect("parses"); + assert_eq!(after, rules); + } + + #[test] + fn a_list_the_splice_cant_follow_is_written_out_whole() { + let [yml, _] = RULES_PATHS; + let flow = + "classifications: [{name: a, description: b, polarity: negative, threshold: 70}]"; + let merged = merge(Some(&file(yml, flow)), vec![rule("c", "d")]).expect("merges"); + let after = parse_rules(yml, &merged.content).expect("parses"); + assert_eq!(after, [rule("a", "b"), rule("c", "d")]); + + // Kept as it is, `b` would point at an anchor the new `a` lacks. + let aliased = "\ +classifications: + - name: a + description: &shared Shared. + polarity: negative + threshold: 70 + - name: b + description: *shared + polarity: negative + threshold: 70 +"; + let mut a = rule("a", "Its own."); + a.threshold = 90; + let merged = merge(Some(&file(yml, aliased)), vec![a.clone()]).expect("merges"); + let after = parse_rules(yml, &merged.content).expect("parses"); + assert_eq!(after, [a, rule("b", "Shared.")]); + } + + #[test] + fn nothing_new_or_too_many_or_a_bad_file_is_refused() { + let [yml, _] = RULES_PATHS; + let before = parse_rules(yml, OWN.as_bytes()).expect("this repo's rules parse"); + let err = merge(Some(&file(yml, OWN)), before.clone()).expect_err("nothing to change"); + assert!(err.contains("already holds"), "{err}"); + let many: Vec<_> = (0..=10 - before.len()) + .map(|i| rule(&format!("extra-{i}"), "d")) + .collect(); + let err = merge(Some(&file(yml, OWN)), many).expect_err("eleven"); + assert!(err.contains("11 classifications"), "{err}"); + let err = merge(Some(&file(yml, "nope: 1")), vec![rule("a", "b")]).expect_err("bad"); + assert!(err.contains(yml), "{err}"); + } + + #[test] + fn a_branch_name_is_a_plain_ref_under_the_prefix() { + let name = branch_name("Alice Smith/../x").expect("a name"); + let (who, hex) = name + .strip_prefix(BRANCH_PREFIX) + .and_then(|r| r.rsplit_once('-')) + .expect("prefix and suffix"); + assert_eq!(who, "alice-smith----x"); + assert_eq!(hex.len(), 8); + assert!(hex.bytes().all(|b| b.is_ascii_hexdigit()), "{name}"); + } + + fn app(fake: Fake) -> axum::Router { + let (mut state, _) = crate::tests::state_with_roster(); + state.try_outs.fake = Some(Arc::new(fake)); + crate::build_app(state) + } + + fn fake() -> Fake { + Fake::new( + Reply::Done { + rows: Vec::new(), + overall: Overall::Approve, + }, + vec![super::super::tests::rules_json()], + ) + } + + fn body(repo: &str) -> String { + serde_json::json!({ + "repo": repo, + "classifications": [{ + "name": "no-ads", + "description": "No ads.", + "polarity": "negative", + "threshold": 80, + }], + }) + .to_string() + } + + async fn post( + app: &axum::Router, + user: Option<&str>, + body: String, + ) -> (StatusCode, serde_json::Value) { + let mut req = Request::post("/api/review-classifier/open-pr") + .header("content-type", "application/json"); + if let Some(user) = user { + req = req.header("remote-user", user); + } + let res = app + .clone() + .oneshot(req.body(Body::from(body)).expect("a request")) + .await + .expect("infallible"); + let status = res.status(); + let bytes = axum::body::to_bytes(res.into_body(), usize::MAX) + .await + .expect("a body"); + (status, serde_json::from_slice(&bytes).unwrap_or_default()) + } + + #[tokio::test] + async fn a_branch_is_pushed_as_the_operator_and_linked_for_the_pr() { + let fake = fake(); + let pushed = Arc::clone(&fake.pushed); + let (status, answer) = post(&app(fake), Some("alice"), body("o/r")).await; + assert_eq!(status, StatusCode::CREATED, "{answer}"); + let branch = answer["branch"].as_str().expect("a branch"); + assert!(branch.starts_with("review-classifier/alice-"), "{branch}"); + assert_eq!( + answer["compare_url"], + format!("https://forge.test/o/r/compare/main...{branch}") + ); + let [_, json] = RULES_PATHS; + assert_eq!(answer["path"], json); + let pushed = pushed.lock().expect("unpoisoned"); + let [(repo, commit)] = pushed.as_slice() else { + panic!("one push expected: {pushed:?}"); + }; + assert_eq!(repo, "o/r"); + assert_eq!( + commit.author, + CommitAuthor { + name: "Alice".to_owned(), + email: "alice@example.test".to_owned(), + } + ); + assert_eq!(commit.base, "main"); + assert_eq!(commit.branch, branch); + assert_eq!(commit.sha.as_deref(), Some(format!("sha-{json}").as_str())); + let names: Vec<_> = parse_rules(json, &commit.content) + .expect("parses") + .into_iter() + .map(|c| c.name) + .collect(); + assert_eq!(names, ["datetime", "no-ads"]); + } + + #[tokio::test] + async fn the_author_is_the_session_user_not_one_in_the_body() { + let fake = fake(); + let pushed = Arc::clone(&fake.pushed); + let mut body: serde_json::Value = serde_json::from_str(&body("o/r")).expect("json"); + body["author"] = "alice".into(); + let (status, answer) = post(&app(fake), Some("bob"), body.to_string()).await; + assert_eq!(status, StatusCode::CREATED, "{answer}"); + let pushed = pushed.lock().expect("unpoisoned"); + assert_eq!(pushed[0].1.author.email, "bob@example.test"); + } + + #[tokio::test] + async fn no_session_user_or_no_forge_account_pushes_nothing() { + for (user, want, says) in [ + (None, StatusCode::UNAUTHORIZED, "Remote-User"), + (Some(" "), StatusCode::UNAUTHORIZED, "Remote-User"), + ( + Some("mallory"), + StatusCode::CONFLICT, + "log in to the forge once", + ), + ] { + let fake = fake(); + let pushed = Arc::clone(&fake.pushed); + let (status, problem) = post(&app(fake), user, body("o/r")).await; + assert_eq!(status, want, "{user:?}: {problem}"); + let detail = problem["detail"].as_str().expect("a detail"); + assert!(detail.contains(says), "{user:?}: {detail}"); + assert!(pushed.lock().expect("unpoisoned").is_empty(), "{user:?}"); + } + } + + #[tokio::test] + async fn a_refused_push_names_the_missing_access() { + for (refusal, want, says) in [ + (StatusCode::FORBIDDEN, StatusCode::FORBIDDEN, "write access"), + (StatusCode::NOT_FOUND, StatusCode::FORBIDDEN, "write access"), + (StatusCode::CONFLICT, StatusCode::CONFLICT, "try again"), + ( + StatusCode::INTERNAL_SERVER_ERROR, + StatusCode::SERVICE_UNAVAILABLE, + "500", + ), + ] { + let mut fake = fake(); + fake.push_refusal = Some(refusal); + let (status, problem) = post(&app(fake), Some("alice"), body("o/r")).await; + assert_eq!(status, want, "{refusal}: {problem}"); + let detail = problem["detail"].as_str().expect("a detail"); + assert!(detail.contains(says), "{refusal}: {detail}"); + } + } + + #[tokio::test] + async fn a_bad_body_or_repo_is_refused_before_the_forge() { + for (body, want) in [ + ("{".to_owned(), StatusCode::BAD_REQUEST), + (body("o"), StatusCode::BAD_REQUEST), + ( + serde_json::json!({ "repo": "o/r", "classifications": [] }).to_string(), + StatusCode::BAD_REQUEST, + ), + (body("o/elsewhere"), StatusCode::NOT_FOUND), + (body("o/empty"), StatusCode::NOT_FOUND), + (body("o/down"), StatusCode::SERVICE_UNAVAILABLE), + ] { + let (status, problem) = post(&app(fake()), Some("alice"), body.clone()).await; + assert_eq!(status, want, "{body}: {problem}"); + } + } +}