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