vinzenz/hyperhive
vinzenz/hyperhive
Watch
0
0
Fork
You've already forked hyperhive
 0
Code Activity

Compare commits

base: vinzenz:be2e64fa8915ed97ed8e0d1236967afbf6cbd71c
Branches Tags
vinzenz:main
..
compare: vinzenz:6ed88ad70af281a2c9b0fad30574cf8c0af2dd8f
Branches Tags
vinzenz:main
11 changed files with 59 additions and 1511 deletions
Download patch file Download diff file Expand all files Collapse all files

60
docs/swarm/review-classifier.md
View file

@ -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 and pushes the try-out
branches with that token, and the classifier service never sees it.
with any other email. The controller posts the reviews 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,10 +254,7 @@ 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 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.
is then enough.
The repo also needs a rules file on its default branch (see
[Repo configuration](#repo-configuration)). Without one, a request posts a
@ -467,49 +464,10 @@ 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** (`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/<you>-<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 noreply address,
`<login, lowercased>@noreply.<forge domain>`. It's the address the forge
itself uses for an account that keeps its email private, so the commit
links to your account and your email stays out of the repo's history. 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
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.
## Run store and retention
@ -617,3 +575,7 @@ 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.

15
docs/swarm/ui.md
View file

@ -112,21 +112,10 @@ 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 a run 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 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

83
frontend/packages/swarm-ui/src/pages/review-tryout/ReviewTryOutPage.tsx
View file

@ -13,18 +13,13 @@
// **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 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.
// 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.
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";
@ -37,14 +32,13 @@ import { ClassificationEditor } from "./ClassificationEditor.js";
import { TryOutResult } from "./TryOutResult.js";
import {
draftFromRule,
draftProblem,
duplicateName,
hasTypedText,
prNumber,
repoProblem,
rowsProblem,
runProblem,
toBody,
type ClassificationDraft,
type OpenPrAnswer,
type OpenPull,
type RulesAnswer,
type TryOutResult as Result,
@ -53,7 +47,6 @@ 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
@ -98,12 +91,6 @@ 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<ProblemDetails | null>(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<string[]>([]);
@ -218,8 +205,12 @@ export function ReviewTryOutPage() {
};
}, [runId]);
const openPrProblem = rowsProblem(repo, drafts);
const problem = runProblem(repo, pr, drafts);
const problem =
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;
async function run(e: Event) {
e.preventDefault();
@ -273,34 +264,6 @@ 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);
@ -316,9 +279,9 @@ export function ReviewTryOutPage() {
<Panel title="review try-out" icon="🧪">
<p>
Run a review classification against a real PR and see how the model
answers it. A run posts nothing to the forge, and nothing is saved:
answers it. Nothing is posted to the forge, and nothing is saved:
change the classification and run it again until it answers the way
you want. Then open a PR to add it to the repo's rules.
you want.
</p>
<form class="ui-tryout-form" onSubmit={(e) => void run(e)}>
<div class="ui-tryout-target">
@ -401,30 +364,8 @@ export function ReviewTryOutPage() {
>
{running ? "running…" : "run"}
</Button>
<Button
onClick={() => void openPr()}
disabled={opening || openPrProblem !== null}
>
{opening ? "pushing…" : "open a PR"}
</Button>
</div>
{problem !== null ? <p class="ui-tryout-hint">{problem}</p> : null}
{openError ? (
<ApiErrorPanel
context="the branch for the PR could not be pushed"
problem={openError}
/>
) : null}
{opened ? (
<p class="ui-tryout-loaded">
Pushed <code>{opened.branch}</code> to {opened.repo}, changing{" "}
<code>{opened.path}</code> with you as the author.{" "}
<a href={opened.compare_url} target="_blank" rel="noopener">
Open the PR on the forge
</a>
.
</p>
) : null}
</form>
</Panel>
<ConfirmDialog

43
frontend/packages/swarm-ui/src/pages/review-tryout/TryOutTypes.ts
View file

@ -1,6 +1,5 @@
// Wire shapes of the review classifier's try-out endpoints
// (`POST /api/review-classifier/try-out`, `GET .../try-out/{id}`,
// `POST .../open-pr`), and the
// (`POST /api/review-classifier/try-out`, `GET .../try-out/{id}`), and the
// checks the form makes before a request is worth sending. The backend stays
// the real gate: these only keep an obviously bad form from a round trip.
@ -35,17 +34,6 @@ export interface RulesAnswer {
classifications: ClassificationBody[];
}
/** The open-PR route's answer (`POST /api/review-classifier/open-pr`): the
* branch the classifications were pushed on, authored by the operator. */
export interface OpenPrAnswer {
branch: string;
/** The rules file the commit wrote. */
path: string;
/** The forge's page comparing the branch with the default branch, where the
* operator opens the PR. */
compare_url: string;
}
/** An open PR, as `GET /api/pulls?repo=…` lists it. */
export interface OpenPull {
number: number;
@ -150,35 +138,6 @@ export function duplicateName(drafts: ClassificationDraft[]): string | null {
return null;
}
/** What keeps the rows from going to the repo as a PR, or null: the repo, then
* each row, then a shared name. Unlike a run, it needs no PR number. */
export function rowsProblem(
repo: string,
drafts: ClassificationDraft[],
): string | null {
return (
repoProblem(repo) ??
drafts.map(draftProblem).find((p) => p !== null) ??
duplicateName(drafts) ??
null
);
}
/** What keeps the rows from a run, or null: everything `rowsProblem` checks,
* then the PR number. The page shows this one reason for both of its buttons,
* so whatever disables opening a PR comes before the PR number it doesn't
* need. */
export function runProblem(
repo: string,
pr: string,
drafts: ClassificationDraft[],
): string | null {
return (
rowsProblem(repo, drafts) ??
(prNumber(pr) === null ? "a PR number is a whole number above 0" : null)
);
}
export function toBody(d: ClassificationDraft): ClassificationBody {
return {
name: d.name.trim(),

143
frontend/packages/swarm-ui/test/tryout-types.test.mjs
View file

@ -1,143 +0,0 @@
// The review try-out form's pure logic: the repo and row checks, the gate on
// opening a PR from the rows, and the request body each row becomes. Run by
// `npm test`, which Node runs with its built-in test runner and type
// stripping, so the `.ts` module is imported as it is.
import assert from "node:assert/strict";
import { test } from "node:test";
import {
draftFromRule,
draftProblem,
duplicateName,
hasTypedText,
prNumber,
repoProblem,
rowsProblem,
runProblem,
thresholdValue,
toBody,
} from "../src/pages/review-tryout/TryOutTypes.ts";
// A row ready to send, with `over` replacing any of its fields.
function row(over = {}) {
return {
key: 0,
name: "no-ads",
description: "No ads.",
polarity: "negative",
threshold: "80",
...over,
};
}
test("a repo is owner/name with forge-safe characters in each part", () => {
assert.equal(repoProblem("hyperhive/hyperhive"), null);
assert.equal(repoProblem(" o/r.name_x-1 "), null);
for (const bad of ["", "o", "o/", "/r", "o/r/x", "o/..", "./r", "o/r s"]) {
assert.match(repoProblem(bad), /owner\/name/, bad);
}
});
test("a row needs a name, then a description, then a whole 0-100 threshold", () => {
assert.equal(draftProblem(row()), null);
assert.match(draftProblem(row({ name: " " })), /a name/);
assert.match(
draftProblem(row({ name: " ", description: "" })),
/a name/,
"the name is checked first",
);
assert.match(
draftProblem(row({ description: " " })),
/no-ads needs a description/,
);
assert.match(
draftProblem(row({ description: "", threshold: "x" })),
/description/,
"the description is checked before the threshold",
);
for (const bad of ["", "101", "-1", "1.5", "x"]) {
assert.match(draftProblem(row({ threshold: bad })), /threshold/, bad);
}
assert.equal(draftProblem(row({ threshold: " 100 " })), null);
assert.equal(draftProblem(row({ threshold: "0" })), null);
});
test("two rows with one trimmed name are refused", () => {
assert.equal(duplicateName([row(), row({ name: "other" })]), null);
assert.match(
duplicateName([row(), row({ name: " no-ads " })]),
/two classifications are named no-ads/,
);
});
test("opening a PR is gated by the repo, then each row, then a shared name", () => {
const good = [row(), row({ name: "other" })];
assert.equal(rowsProblem("o/r", good), null);
// Each check on its own.
assert.match(rowsProblem("", good), /owner\/name/);
assert.match(rowsProblem("o/r", [row(), row({ name: "" })]), /a name/);
assert.match(rowsProblem("o/r", [row(), row()]), /two classifications/);
// The order: a bad repo wins over a bad row, and a bad row over a shared
// name.
assert.match(rowsProblem("o", [row({ name: "" })]), /owner\/name/);
assert.match(
rowsProblem("o/r", [row(), row({ threshold: "x" }), row()]),
/threshold/,
);
// Rows are checked in their order.
assert.match(
rowsProblem("o/r", [row({ description: "" }), row({ name: "" })]),
/no-ads needs a description/,
);
});
test("opening a PR needs no PR number, which a run does", () => {
assert.equal(prNumber(""), null);
assert.equal(rowsProblem("o/r", [row()]), null);
assert.equal(prNumber(" 7 "), 7);
assert.equal(prNumber("0"), null);
assert.equal(prNumber("7a"), null);
});
test("a row becomes the request's classification", () => {
assert.deepEqual(
toBody(
row({ name: " no-ads ", description: " No ads. ", threshold: " 60 " }),
),
{
name: "no-ads",
description: " No ads. ",
polarity: "negative",
threshold: 60,
},
);
assert.equal(thresholdValue("101"), null);
});
test("a loaded rule becomes a row that sends back the same rule", () => {
const rule = {
name: "no-ads",
description: "No ads.",
polarity: "negative",
threshold: 80,
};
const d = draftFromRule(rule, 3);
assert.deepEqual(d, { key: 3, ...rule, threshold: "80" });
assert.deepEqual(toBody(d), rule);
});
test("only a typed name or description counts as the user's work", () => {
assert.equal(hasTypedText([row({ name: " ", description: "" })]), false);
assert.equal(hasTypedText([row({ name: "", description: "x" })]), true);
});
test("the page's one hint names what disables opening a PR before the PR number", () => {
// No PR number and a bad row: the row is what disables both buttons.
assert.match(runProblem("o/r", "", [row({ name: "" })]), /a name/);
assert.match(runProblem("o", "", [row()]), /owner\/name/);
// Rows ready but no PR number: only the run is held back.
assert.match(runProblem("o/r", "", [row()]), /PR number/);
assert.equal(rowsProblem("o/r", [row()]), null);
assert.equal(runProblem("o/r", "7", [row()]), null);
});

4
swarm-controller/README.md
View file

@ -62,9 +62,7 @@ The swarm's control plane. The swarm UI and `swarmctl` are its clients.
requests, and the newest 100 finished runs, one document in the
`review-classifier-runs` KV bucket, which this daemon alone writes
(`POST /api/review-classifier/try-out`, `GET /api/review-classifier/try-out/{id}`,
`GET /api/review-classifier/rules`, `GET /api/review-classifier/runs`), and
the try-out page's branch for a PR, pushed as the classifier's forge account
(`POST /api/review-classifier/open-pr`) →
`GET /api/review-classifier/rules`, `GET /api/review-classifier/runs`) →
[`docs/swarm/review-classifier.md`](../docs/swarm/review-classifier.md).
- **Review classifier settings** — the classifier's runtime, model, effort and how
many requested reviews run at once, one document in the

44
swarm-controller/src/forge.rs
View file

@ -123,19 +123,9 @@ pub struct DefaultBranchFiles {
pub branch: String,
/// The commit the files were read at.
pub commit: String,
/// The repo's page on the forge, as a browser reaches it.
pub web_url: Option<url::Url>,
/// Each asked-for path that exists there, in the order asked.
pub files: Vec<BranchFile>,
}
/// 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<u8>,
/// 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
@ -1355,26 +1345,19 @@ impl Client {
let mut trees = HashMap::new();
let mut files = Vec::new();
for path in paths {
if let Some((sha, bytes)) = self.file_at(owner, repo, &commit, path, &mut trees).await?
{
files.push(BranchFile {
path: (*path).to_owned(),
sha,
bytes,
});
if let Some(bytes) = self.file_at(owner, repo, &commit, path, &mut trees).await? {
files.push(((*path).to_owned(), bytes));
}
}
Ok(Some(DefaultBranchFiles {
branch,
commit,
web_url: info.html_url,
files,
}))
}
/// 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.
/// 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,
@ -1382,7 +1365,7 @@ impl Client {
commit: &str,
path: &str,
trees: &mut HashMap<String, Vec<GitEntry>>,
) -> Result<Option<(String, Vec<u8>)>> {
) -> Result<Option<Vec<u8>>> {
let mut tree = commit.to_owned();
let mut segments = path.split('/').peekable();
while let Some(name) = segments.next() {
@ -1400,10 +1383,7 @@ impl Client {
return Ok(None);
};
if kind == "blob" {
return self
.blob(owner, repo, &sha)
.await
.map(|bytes| Some((sha, bytes)));
return self.blob(owner, repo, &sha).await.map(Some);
}
tree = sha;
}
@ -2454,11 +2434,7 @@ mod tests {
assert_eq!(read.commit, "def456");
assert_eq!(
read.files,
[BranchFile {
path: RULES_YML.to_owned(),
sha: "b1".to_owned(),
bytes: b"classifications: []\n".to_vec(),
}]
[(RULES_YML.to_owned(), b"classifications: []\n".to_vec())]
);
// Each tree is read once for both paths.
let seen = seen.lock().unwrap();

294
swarm-controller/src/forge/classifier_account.rs
View file

@ -3,14 +3,11 @@
//! `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`).
//! 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 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, and write to
//! push a branch.
//! protection's approvals whitelist) for its approval to count.
//!
//! 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
@ -20,16 +17,13 @@
use std::sync::Arc;
use anyhow::{Context, Result, bail};
use forgejo_api::structs::{
ChangeFileOperation, ChangeFileOperationOperation, ChangeFilesOptions, CreatePullReviewOptions,
Identity, RepoSearchQuery,
};
use forgejo_api::structs::{CreatePullReviewOptions, 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, is_not_found};
use super::agent_token::{Decision, Observed, classify_named};
/// The classifier's forge login: the reviewer someone requests.
pub const LOGIN: &str = "review-classifier";
@ -37,8 +31,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 and push branches, both of 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, 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.
@ -68,29 +62,6 @@ 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<String>,
pub content: Vec<u8>,
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
@ -150,8 +121,19 @@ impl Client {
commit: Option<&str>,
review: Review,
) -> Result<Option<String>> {
self.as_classifier()
.await?
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::<forge::Credential>(&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")?
.repo_create_pull_review(
owner,
repo,
@ -168,63 +150,6 @@ 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<()> {
push_branch(&self.as_classifier().await?, owner, repo, commit).await
}
/// The forge account `login` as a commit author: its full name, or its
/// login when it has none, and its [`noreply_address`], so the commit
/// links to the account without showing its email. `None` when the
/// forge has no such account.
///
/// # Errors
/// When the forge refuses the read, or this daemon's forge URL has no
/// host.
pub async fn commit_author(&self, login: &str) -> Result<Option<CommitAuthor>> {
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 login = user.login.as_deref().unwrap_or(login);
let email = noreply_address(&self.url, login)?;
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<Forgejo> {
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::<forge::Credential>(&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(
@ -300,60 +225,6 @@ impl Client {
}
}
/// The address the forge itself puts on a web commit by `login` when the
/// account keeps its email private: the lowercased login at
/// `NO_REPLY_ADDRESS`. The forge module leaves that at its default,
/// `noreply.<DOMAIN>`, and sets `DOMAIN` from
/// `services.hyperhive.swarm.forge.domain`, the option this daemon's forge
/// URL `forge` is built from, so the domain is that URL's host.
fn noreply_address(forge: &url::Url, login: &str) -> Result<String> {
let domain = forge
.host_str()
.with_context(|| format!("the forge URL {forge} names no host"))?;
Ok(format!("{}@noreply.{domain}", login.to_lowercase()))
}
/// Make `commit` with `forge`'s identity as committer, through the contents
/// API.
async fn push_branch(forge: &Forgejo, owner: &str, repo: &str, commit: BranchCommit) -> Result<()> {
use base64::Engine as _;
let branch = commit.branch.clone();
forge
.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(())
}
/// Check the account now and every [`RECONCILE_INTERVAL`] after, and call
/// `enqueue`, which inserts a `MintClassifierForgeToken` node, when it needs
/// a job. A pass that fails is logged and retried on the next tick.
@ -435,133 +306,6 @@ mod tests {
assert!(lockdown_patch(&seen, LOGIN).is_none());
}
/// The one request `push_branch` sends for `sha`, as a stand-in forge
/// saw it: method, path and JSON body.
async fn pushed(sha: Option<&str>) -> (String, String, serde_json::Value) {
use axum::http::{Method, StatusCode, Uri, header::CONTENT_TYPE};
let seen = Arc::new(std::sync::Mutex::new(Vec::new()));
let recorded = Arc::clone(&seen);
let app = axum::Router::new().fallback(
move |method: Method, uri: Uri, body: axum::body::Bytes| {
let recorded = Arc::clone(&recorded);
async move {
recorded.lock().unwrap().push((
method.to_string(),
uri.path().to_owned(),
serde_json::from_slice(&body).unwrap_or_default(),
));
(
StatusCode::CREATED,
[(CONTENT_TYPE, "application/json")],
r#"{"commit":{"sha":"c1"}}"#,
)
}
},
);
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 forge = Forgejo::new(Auth::Token("stub-token"), url).unwrap();
let commit = BranchCommit {
base: "main".to_owned(),
branch: "review-classifier/alice-0a1b2c3d".to_owned(),
path: ".hyperhive/review-classifier.yml".to_owned(),
sha: sha.map(str::to_owned),
content: b"classifications: []\n".to_vec(),
message: "review classifier: add no-ads".to_owned(),
author: CommitAuthor {
name: "Alice".to_owned(),
email: "alice@example.test".to_owned(),
},
};
push_branch(&forge, "o", "r", commit).await.expect("pushed");
let mut seen = seen.lock().unwrap();
assert_eq!(seen.len(), 1, "{seen:?}");
seen.remove(0)
}
#[tokio::test]
async fn a_push_is_one_commit_on_a_new_branch_authored_by_the_operator() {
let (method, path, body) = pushed(Some("b1")).await;
assert_eq!(method, "POST");
assert_eq!(path, "/api/v1/repos/o/r/contents");
assert_eq!(body["branch"], "main");
assert_eq!(body["new_branch"], "review-classifier/alice-0a1b2c3d");
assert_eq!(body["force_overwrite_new_branch"], false);
assert_eq!(body["message"], "review classifier: add no-ads");
assert_eq!(
body["author"],
serde_json::json!({ "name": "Alice", "email": "alice@example.test" })
);
assert_eq!(body["committer"], serde_json::Value::Null);
assert_eq!(
body["files"],
serde_json::json!([{
"operation": "update",
"path": ".hyperhive/review-classifier.yml",
"sha": "b1",
// base64 of "classifications: []\n"
"content": "Y2xhc3NpZmljYXRpb25zOiBbXQo=",
"from_path": null,
}])
);
}
#[tokio::test]
async fn a_push_without_a_blob_sha_creates_the_file() {
let (_, _, body) = pushed(None).await;
assert_eq!(body["files"][0]["operation"], "create");
assert_eq!(body["files"][0]["sha"], serde_json::Value::Null);
}
#[tokio::test]
async fn a_commit_author_is_the_accounts_name_at_its_noreply_address_or_none() {
use super::super::tests::stub_forge;
let (client, _) = stub_forge(
(404, "{}"),
(
200,
r#"{"login":"Alice","full_name":" Alice A ","email":"private@x.test"}"#,
),
(404, "{}"),
)
.await;
let domain = client.url.host_str().expect("a host").to_owned();
assert_eq!(
client.commit_author("alice").await.expect("read"),
Some(CommitAuthor {
name: "Alice A".to_owned(),
email: format!("alice@noreply.{domain}"),
})
);
let (client, _) = stub_forge(
(404, "{}"),
(200, r#"{"login":"bob","full_name":"","email":""}"#),
(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);
}
#[test]
fn the_noreply_address_is_the_lowercased_login_at_noreply_dot_the_forge_host() {
let forge = url::Url::parse("https://Forge.Example:8443/").expect("a URL");
assert_eq!(
noreply_address(&forge, "Alice-B").expect("a host"),
"alice-b@noreply.forge.example"
);
}
#[tokio::test]
async fn the_job_locks_down_the_classifiers_own_account() {
use super::super::tests::{lockdown_patch, stub_forge};

1
swarm-controller/src/main.rs
View file

@ -3420,7 +3420,6 @@ 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,

46
swarm-controller/src/review_classifier.rs
View file

@ -2,8 +2,7 @@
//! `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. [`open_pr`] puts classifications on
//! a new branch of the repo, for a PR.
//! 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:
@ -40,7 +39,6 @@ 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;
@ -498,7 +496,7 @@ async fn rules(
),
));
}
[file] => (&file.path, &file.bytes),
[file] => file,
_ => {
return Err(error_problem(
StatusCode::UNPROCESSABLE_ENTITY,
@ -767,23 +765,17 @@ 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<std::sync::Mutex<Vec<TryOut>>>,
pub(crate) pushed: Arc<std::sync::Mutex<Vec<(String, BranchCommit)>>>,
pub(crate) push_refusal: Option<StatusCode>,
}
impl Fake {
@ -793,37 +785,10 @@ 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<Option<CommitAuthor>> {
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<Settings> {
Ok(self.settings.clone())
@ -860,15 +825,10 @@ 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)| BranchFile {
path: (*f).to_owned(),
sha: format!("sha-{f}"),
bytes: body.clone().into_bytes(),
})
.map(|(f, body)| ((*f).to_owned(), body.clone().into_bytes()))
.collect(),
})),
("o", "empty") => Ok(None),

837
swarm-controller/src/review_classifier/open_pr.rs
View file

@ -1,837 +0,0 @@
//! `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<Classification>,
}
/// 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<Output = anyhow::Result<Option<CommitAuthor>>> + Send;
fn push(
&self,
owner: &str,
repo: &str,
commit: BranchCommit,
) -> impl Future<Output = anyhow::Result<()>> + Send;
}
impl BranchSink for forge::Client {
async fn commit_author(&self, login: &str) -> anyhow::Result<Option<CommitAuthor>> {
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<AppState>,
headers: HeaderMap,
body: Result<Json<OpenPrRequest>, JsonRejection>,
) -> Result<(StatusCode, Json<OpenPrBranch>), 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<F: FileSource + BranchSink + Sync>(
forge: &F,
operator: &str,
owner: &str,
name: &str,
rows: Vec<Classification>,
) -> Result<(StatusCode, Json<OpenPrBranch>), 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<T>(work: impl Future<Output = T>) -> Result<T, ProblemDetails> {
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<String, String> {
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<StatusCode> {
match e.downcast_ref::<ForgejoError>()? {
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<String>,
content: Vec<u8>,
message: String,
}
/// `rows` merged into `existing`, or why they can't be.
fn merge(existing: Option<&BranchFile>, rows: Vec<Classification>) -> Result<Merged, String> {
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(), &current, &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>, 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<String> = 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}");
}
}
}