review try-out: open a PR with the classifications on the page
The try-out page gets an "open a PR" button. It posts the page's classifications to the new POST /api/review-classifier/open-pr, which merges them by name into the repo's rules file on the default branch and has the review-classifier forge account commit the result on a new branch, review-classifier/<user>-<8 hex>, through the contents API. The commit's author is the forge account named like the session's Remote-User, read with the controller's admin token. The answer links to the forge's compare page, where the user opens the PR as themselves. The push uses the account's existing token from the store (swarm/services/review-classifier/forge/token, scope write:repository), so the account needs write access to the repo; a refused push answers 403 naming that. In a YAML rules file, the entries the merge leaves alone keep their text, as long as the result reads back as the merged list; otherwise the file is written out whole. DefaultBranchFiles now carries each file's blob sha, which the forge needs to replace a file, and the repo's web URL for the link. Fixes #5259 Refs #5096
This commit is contained in:
parent
6ed88ad70a
commit
49b15e7775
10 changed files with 1231 additions and 53 deletions
|
|
@ -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
|
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
|
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
|
matches the forge. The controller refuses an existing account by that name
|
||||||
with any other email. The controller posts the reviews with that token, and
|
with any other email. The controller posts the reviews and pushes the try-out
|
||||||
the classifier service never sees it.
|
branches with that token, and the classifier service never sees it.
|
||||||
|
|
||||||
**Giving a repo access is a repo admin's step, by hand.** Nothing grants
|
**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
|
it automatically. In the repo's settings, add `review-classifier` as a
|
||||||
|
|
@ -254,7 +254,10 @@ collaborator:
|
||||||
its request-changes blocks the merge where the branch protection blocks on
|
its request-changes blocks the merge where the branch protection blocks on
|
||||||
rejected reviews. Where the branch protection restricts approvals to an
|
rejected reviews. Where the branch protection restricts approvals to an
|
||||||
allowlist, the account counts only once it's on that list, and read access
|
allowlist, the account counts only once it's on that list, and read access
|
||||||
is then enough.
|
is then enough for reviews.
|
||||||
|
|
||||||
|
The try-out page's **open a PR** pushes a branch to the repo itself, so it
|
||||||
|
needs **write** whatever the branch protection says.
|
||||||
|
|
||||||
The repo also needs a rules file on its default branch (see
|
The repo also needs a rules file on its default branch (see
|
||||||
[Repo configuration](#repo-configuration)). Without one, a request posts a
|
[Repo configuration](#repo-configuration)). Without one, a request posts a
|
||||||
|
|
@ -464,10 +467,46 @@ since a run also works on a merged or closed PR. Two routes feed them:
|
||||||
|
|
||||||
Every error is problem+json with the reason in `detail`.
|
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
|
When it's ready, **open a PR** (`POST /api/review-classifier/open-pr`, with
|
||||||
adds the classification to the repo's config, with you, the swarm UI user,
|
the repo and the classifications) has the service account push a branch that
|
||||||
as commit author. The page then links you to the forge's new-PR page for
|
adds the classifications to the repo's rules, with you, the swarm UI user, as
|
||||||
that branch, where you log in as usual and open the PR yourself.
|
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 email. The committer is the service
|
||||||
|
account.
|
||||||
|
- **The token** is the service account's own, read from the store for each
|
||||||
|
push (see [The service account](#the-service-account)), so the account
|
||||||
|
needs **write** access to the repo.
|
||||||
|
|
||||||
|
The route answers **201** with `branch`, `path` (the file written) and
|
||||||
|
`compare_url`, or:
|
||||||
|
|
||||||
|
- **400** for a body the try-out POST would refuse in its repo or
|
||||||
|
classifications
|
||||||
|
- **401** without the `Remote-User` header the gateway sets
|
||||||
|
- **403** when the forge refuses the push: the service account isn't a
|
||||||
|
collaborator with write access
|
||||||
|
- **404** for a repo the forge doesn't know, or one with no commit on its
|
||||||
|
default branch
|
||||||
|
- **409** when the forge has no account named like you yet, or the rules
|
||||||
|
file changed on the default branch during the push
|
||||||
|
- **422** for a default branch holding both rules files or one the `rules`
|
||||||
|
route refuses; classifications the file already holds unchanged; or a
|
||||||
|
merge that breaks a rule, such as more than ten classifications
|
||||||
|
- **503** when this host has no forge token, the service account's token
|
||||||
|
isn't minted yet, or the forge failed or didn't answer within two minutes
|
||||||
|
|
||||||
## Run store and retention
|
## Run store and retention
|
||||||
|
|
||||||
|
|
@ -575,7 +614,3 @@ These get settled, and filled in here, as the work reaches them:
|
||||||
`secret/swarm/services/review-classifier/forge/token`.
|
`secret/swarm/services/review-classifier/forge/token`.
|
||||||
- **Reviewer handoff.** How a later reviewer, human or agent, learns the
|
- **Reviewer handoff.** How a later reviewer, human or agent, learns the
|
||||||
classifier passed a PR before it starts its own review.
|
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.
|
|
||||||
|
|
|
||||||
|
|
@ -112,10 +112,21 @@ and the head commit the run read. A failed run shows its error, for example when
|
||||||
has no API key yet.
|
has no API key yet.
|
||||||
|
|
||||||
What you typed stays after a run, so you can edit the classification and run it
|
What you typed stays after a run, so you can edit the classification and run it
|
||||||
again. The page saves no classification and no result, and posts nothing to the
|
again. The page saves no classification and no result, and a run posts nothing
|
||||||
forge; like every table in the UI, the result table remembers its column
|
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.
|
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
|
### Classifier settings
|
||||||
|
|
||||||
Under the try-out form, **classifier settings** shows the runtime, model and
|
Under the try-out form, **classifier settings** shows the runtime, model and
|
||||||
|
|
|
||||||
|
|
@ -13,13 +13,18 @@
|
||||||
// **load the repo's rules** fills the rows from the rules on the repo's default
|
// **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.
|
// 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
|
// Under the form, the classifier's own settings: its model, effort and review
|
||||||
// concurrency (`ClassifierSettings`).
|
// concurrency (`ClassifierSettings`).
|
||||||
//
|
//
|
||||||
// The inputs stay as typed after a run, so the operator can edit and run
|
// 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
|
// again. Apart from the settings, the page keeps nothing you typed and no
|
||||||
// result, and posts nothing to the forge; the only thing the browser remembers
|
// result, and a run posts nothing to the forge; the only thing the browser
|
||||||
// is the result table's column filters, as for every table in the UI.
|
// remembers is the result table's column filters, as for every table in the UI.
|
||||||
import { useEffect, useState } from "preact/hooks";
|
import { useEffect, useState } from "preact/hooks";
|
||||||
import { ApiErrorPanel } from "@hive/shared/api-error-panel.js";
|
import { ApiErrorPanel } from "@hive/shared/api-error-panel.js";
|
||||||
import { readApiError, type ProblemDetails } from "@hive/shared/api-error.js";
|
import { readApiError, type ProblemDetails } from "@hive/shared/api-error.js";
|
||||||
|
|
@ -39,6 +44,7 @@ import {
|
||||||
repoProblem,
|
repoProblem,
|
||||||
toBody,
|
toBody,
|
||||||
type ClassificationDraft,
|
type ClassificationDraft,
|
||||||
|
type OpenPrAnswer,
|
||||||
type OpenPull,
|
type OpenPull,
|
||||||
type RulesAnswer,
|
type RulesAnswer,
|
||||||
type TryOutResult as Result,
|
type TryOutResult as Result,
|
||||||
|
|
@ -47,6 +53,7 @@ import "./ReviewTryOutPage.css";
|
||||||
|
|
||||||
const TRY_OUT = "/api/review-classifier/try-out";
|
const TRY_OUT = "/api/review-classifier/try-out";
|
||||||
const RULES = "/api/review-classifier/rules";
|
const RULES = "/api/review-classifier/rules";
|
||||||
|
const OPEN_PR = "/api/review-classifier/open-pr";
|
||||||
const PULL_REPOS = "/api/pulls/repos";
|
const PULL_REPOS = "/api/pulls/repos";
|
||||||
const PULLS = "/api/pulls";
|
const PULLS = "/api/pulls";
|
||||||
// How long the repo field rests before its PRs are fetched, so typing a repo
|
// How long the repo field rests before its PRs are fetched, so typing a repo
|
||||||
|
|
@ -91,6 +98,12 @@ export function ReviewTryOutPage() {
|
||||||
);
|
);
|
||||||
const [confirmLoad, setConfirmLoad] = useState(false);
|
const [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
|
// 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.
|
// repo or a PR the lists lack, a merged one say, can still be typed.
|
||||||
const [repoChoices, setRepoChoices] = useState<string[]>([]);
|
const [repoChoices, setRepoChoices] = useState<string[]>([]);
|
||||||
|
|
@ -205,12 +218,15 @@ export function ReviewTryOutPage() {
|
||||||
};
|
};
|
||||||
}, [runId]);
|
}, [runId]);
|
||||||
|
|
||||||
const problem =
|
// What keeps the rows from going to the repo; a PR number is not needed.
|
||||||
|
const rowsProblem =
|
||||||
repoProblem(repo) ??
|
repoProblem(repo) ??
|
||||||
(prNumber(pr) === null ? "a PR number is a whole number above 0" : null) ??
|
|
||||||
drafts.map(draftProblem).find((p) => p !== null) ??
|
drafts.map(draftProblem).find((p) => p !== null) ??
|
||||||
duplicateName(drafts) ??
|
duplicateName(drafts) ??
|
||||||
null;
|
null;
|
||||||
|
const problem =
|
||||||
|
(prNumber(pr) === null ? "a PR number is a whole number above 0" : null) ??
|
||||||
|
rowsProblem;
|
||||||
|
|
||||||
async function run(e: Event) {
|
async function run(e: Event) {
|
||||||
e.preventDefault();
|
e.preventDefault();
|
||||||
|
|
@ -264,6 +280,34 @@ export function ReviewTryOutPage() {
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// Pushes the rows on a new branch of the repo and keeps the link to open
|
||||||
|
// the PR from it.
|
||||||
|
async function openPr() {
|
||||||
|
const target = repo.trim();
|
||||||
|
setOpening(true);
|
||||||
|
setOpened(null);
|
||||||
|
setOpenError(null);
|
||||||
|
try {
|
||||||
|
const res = await fetch(OPEN_PR, {
|
||||||
|
method: "POST",
|
||||||
|
headers: { "content-type": "application/json" },
|
||||||
|
body: JSON.stringify({
|
||||||
|
repo: target,
|
||||||
|
classifications: drafts.map(toBody),
|
||||||
|
}),
|
||||||
|
});
|
||||||
|
if (!res.ok) {
|
||||||
|
setOpenError(await readApiError(res));
|
||||||
|
return;
|
||||||
|
}
|
||||||
|
setOpened({ ...((await res.json()) as OpenPrAnswer), repo: target });
|
||||||
|
} catch (err: unknown) {
|
||||||
|
setOpenError({ detail: String(err) });
|
||||||
|
} finally {
|
||||||
|
setOpening(false);
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
// Typed rows are the operator's work, so replacing them is asked first.
|
// Typed rows are the operator's work, so replacing them is asked first.
|
||||||
function askLoadRules() {
|
function askLoadRules() {
|
||||||
if (hasTypedText(drafts)) setConfirmLoad(true);
|
if (hasTypedText(drafts)) setConfirmLoad(true);
|
||||||
|
|
@ -279,9 +323,9 @@ export function ReviewTryOutPage() {
|
||||||
<Panel title="review try-out" icon="🧪">
|
<Panel title="review try-out" icon="🧪">
|
||||||
<p>
|
<p>
|
||||||
Run a review classification against a real PR and see how the model
|
Run a review classification against a real PR and see how the model
|
||||||
answers it. Nothing is posted to the forge, and nothing is saved:
|
answers it. A run posts nothing to the forge, and nothing is saved:
|
||||||
change the classification and run it again until it answers the way
|
change the classification and run it again until it answers the way
|
||||||
you want.
|
you want. Then open a PR to add it to the repo's rules.
|
||||||
</p>
|
</p>
|
||||||
<form class="ui-tryout-form" onSubmit={(e) => void run(e)}>
|
<form class="ui-tryout-form" onSubmit={(e) => void run(e)}>
|
||||||
<div class="ui-tryout-target">
|
<div class="ui-tryout-target">
|
||||||
|
|
@ -364,8 +408,30 @@ export function ReviewTryOutPage() {
|
||||||
>
|
>
|
||||||
{running ? "running…" : "run"}
|
{running ? "running…" : "run"}
|
||||||
</Button>
|
</Button>
|
||||||
|
<Button
|
||||||
|
onClick={() => void openPr()}
|
||||||
|
disabled={opening || rowsProblem !== null}
|
||||||
|
>
|
||||||
|
{opening ? "pushing…" : "open a PR"}
|
||||||
|
</Button>
|
||||||
</div>
|
</div>
|
||||||
{problem !== null ? <p class="ui-tryout-hint">{problem}</p> : null}
|
{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>
|
</form>
|
||||||
</Panel>
|
</Panel>
|
||||||
<ConfirmDialog
|
<ConfirmDialog
|
||||||
|
|
|
||||||
|
|
@ -1,5 +1,6 @@
|
||||||
// Wire shapes of the review classifier's try-out endpoints
|
// Wire shapes of the review classifier's try-out endpoints
|
||||||
// (`POST /api/review-classifier/try-out`, `GET .../try-out/{id}`), and the
|
// (`POST /api/review-classifier/try-out`, `GET .../try-out/{id}`,
|
||||||
|
// `POST .../open-pr`), and the
|
||||||
// checks the form makes before a request is worth sending. The backend stays
|
// 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.
|
// the real gate: these only keep an obviously bad form from a round trip.
|
||||||
|
|
||||||
|
|
@ -34,6 +35,17 @@ export interface RulesAnswer {
|
||||||
classifications: ClassificationBody[];
|
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. */
|
/** An open PR, as `GET /api/pulls?repo=…` lists it. */
|
||||||
export interface OpenPull {
|
export interface OpenPull {
|
||||||
number: number;
|
number: number;
|
||||||
|
|
|
||||||
|
|
@ -62,7 +62,9 @@ The swarm's control plane. The swarm UI and `swarmctl` are its clients.
|
||||||
requests, and the newest 100 finished runs, one document in the
|
requests, and the newest 100 finished runs, one document in the
|
||||||
`review-classifier-runs` KV bucket, which this daemon alone writes
|
`review-classifier-runs` KV bucket, which this daemon alone writes
|
||||||
(`POST /api/review-classifier/try-out`, `GET /api/review-classifier/try-out/{id}`,
|
(`POST /api/review-classifier/try-out`, `GET /api/review-classifier/try-out/{id}`,
|
||||||
`GET /api/review-classifier/rules`, `GET /api/review-classifier/runs`) →
|
`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`) →
|
||||||
[`docs/swarm/review-classifier.md`](../docs/swarm/review-classifier.md).
|
[`docs/swarm/review-classifier.md`](../docs/swarm/review-classifier.md).
|
||||||
- **Review classifier settings** — the classifier's runtime, model, effort and how
|
- **Review classifier settings** — the classifier's runtime, model, effort and how
|
||||||
many requested reviews run at once, one document in the
|
many requested reviews run at once, one document in the
|
||||||
|
|
|
||||||
|
|
@ -123,9 +123,19 @@ pub struct DefaultBranchFiles {
|
||||||
pub branch: String,
|
pub branch: String,
|
||||||
/// The commit the files were read at.
|
/// The commit the files were read at.
|
||||||
pub commit: String,
|
pub commit: String,
|
||||||
/// Each asked-for path that exists there, with its bytes, in the order
|
/// The repo's page on the forge, as a browser reaches it.
|
||||||
/// asked.
|
pub web_url: Option<url::Url>,
|
||||||
pub files: Vec<(String, Vec<u8>)>,
|
/// 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>,
|
||||||
}
|
}
|
||||||
|
|
||||||
/// One row of [`Client::issue_report`] — an open issue plus the two facts
|
/// One row of [`Client::issue_report`] — an open issue plus the two facts
|
||||||
|
|
@ -1345,19 +1355,26 @@ impl Client {
|
||||||
let mut trees = HashMap::new();
|
let mut trees = HashMap::new();
|
||||||
let mut files = Vec::new();
|
let mut files = Vec::new();
|
||||||
for path in paths {
|
for path in paths {
|
||||||
if let Some(bytes) = self.file_at(owner, repo, &commit, path, &mut trees).await? {
|
if let Some((sha, bytes)) = self.file_at(owner, repo, &commit, path, &mut trees).await?
|
||||||
files.push(((*path).to_owned(), bytes));
|
{
|
||||||
|
files.push(BranchFile {
|
||||||
|
path: (*path).to_owned(),
|
||||||
|
sha,
|
||||||
|
bytes,
|
||||||
|
});
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
Ok(Some(DefaultBranchFiles {
|
Ok(Some(DefaultBranchFiles {
|
||||||
branch,
|
branch,
|
||||||
commit,
|
commit,
|
||||||
|
web_url: info.html_url,
|
||||||
files,
|
files,
|
||||||
}))
|
}))
|
||||||
}
|
}
|
||||||
|
|
||||||
/// The file at `path` in `owner/repo` at `commit`, or `None` when no
|
/// The blob sha and bytes of the file at `path` in `owner/repo` at
|
||||||
/// blob is there. `trees` holds the tree listings already read, by sha.
|
/// `commit`, or `None` when no blob is there. `trees` holds the tree
|
||||||
|
/// listings already read, by sha.
|
||||||
async fn file_at(
|
async fn file_at(
|
||||||
&self,
|
&self,
|
||||||
owner: &str,
|
owner: &str,
|
||||||
|
|
@ -1365,7 +1382,7 @@ impl Client {
|
||||||
commit: &str,
|
commit: &str,
|
||||||
path: &str,
|
path: &str,
|
||||||
trees: &mut HashMap<String, Vec<GitEntry>>,
|
trees: &mut HashMap<String, Vec<GitEntry>>,
|
||||||
) -> Result<Option<Vec<u8>>> {
|
) -> Result<Option<(String, Vec<u8>)>> {
|
||||||
let mut tree = commit.to_owned();
|
let mut tree = commit.to_owned();
|
||||||
let mut segments = path.split('/').peekable();
|
let mut segments = path.split('/').peekable();
|
||||||
while let Some(name) = segments.next() {
|
while let Some(name) = segments.next() {
|
||||||
|
|
@ -1383,7 +1400,10 @@ impl Client {
|
||||||
return Ok(None);
|
return Ok(None);
|
||||||
};
|
};
|
||||||
if kind == "blob" {
|
if kind == "blob" {
|
||||||
return self.blob(owner, repo, &sha).await.map(Some);
|
return self
|
||||||
|
.blob(owner, repo, &sha)
|
||||||
|
.await
|
||||||
|
.map(|bytes| Some((sha, bytes)));
|
||||||
}
|
}
|
||||||
tree = sha;
|
tree = sha;
|
||||||
}
|
}
|
||||||
|
|
@ -2434,7 +2454,11 @@ mod tests {
|
||||||
assert_eq!(read.commit, "def456");
|
assert_eq!(read.commit, "def456");
|
||||||
assert_eq!(
|
assert_eq!(
|
||||||
read.files,
|
read.files,
|
||||||
[(RULES_YML.to_owned(), b"classifications: []\n".to_vec())]
|
[BranchFile {
|
||||||
|
path: RULES_YML.to_owned(),
|
||||||
|
sha: "b1".to_owned(),
|
||||||
|
bytes: b"classifications: []\n".to_vec(),
|
||||||
|
}]
|
||||||
);
|
);
|
||||||
// Each tree is read once for both paths.
|
// Each tree is read once for both paths.
|
||||||
let seen = seen.lock().unwrap();
|
let seen = seen.lock().unwrap();
|
||||||
|
|
|
||||||
|
|
@ -3,11 +3,14 @@
|
||||||
//! `swarm_secret_client::review_classifier::forge_token_path`. Requesting
|
//! `swarm_secret_client::review_classifier::forge_token_path`. Requesting
|
||||||
//! this account as a reviewer starts a classifier run, and this daemon posts
|
//! this account as a reviewer starts a classifier run, and this daemon posts
|
||||||
//! the result as the account's review (`crate::review_classifier::review`).
|
//! the result as the account's review (`crate::review_classifier::review`).
|
||||||
//! The classifier itself never holds the token.
|
//! This daemon also pushes the try-out page's branches as the account
|
||||||
|
//! (`crate::review_classifier::open_pr`). The classifier itself never holds
|
||||||
|
//! the token.
|
||||||
//!
|
//!
|
||||||
//! The account gets no repo access here. A repo admin adds it to a repo by
|
//! 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
|
//! hand: read to request it as a reviewer, write (or a place on the branch
|
||||||
//! protection's approvals whitelist) for its approval to count.
|
//! protection's approvals whitelist) for its approval to count, and write to
|
||||||
|
//! push a branch.
|
||||||
//!
|
//!
|
||||||
//! The token decision is [`super::agent_token`]'s, under this module's name
|
//! 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
|
//! and scopes, so a stored token the forge still lists with the same last
|
||||||
|
|
@ -17,13 +20,16 @@
|
||||||
use std::sync::Arc;
|
use std::sync::Arc;
|
||||||
|
|
||||||
use anyhow::{Context, Result, bail};
|
use anyhow::{Context, Result, bail};
|
||||||
use forgejo_api::structs::{CreatePullReviewOptions, RepoSearchQuery};
|
use forgejo_api::structs::{
|
||||||
|
ChangeFileOperation, ChangeFileOperationOperation, ChangeFilesOptions, CreatePullReviewOptions,
|
||||||
|
Identity, RepoSearchQuery,
|
||||||
|
};
|
||||||
use forgejo_api::{Auth, Forgejo};
|
use forgejo_api::{Auth, Forgejo};
|
||||||
use swarm_secret_client::forge;
|
use swarm_secret_client::forge;
|
||||||
use swarm_secret_client::review_classifier::forge_token_path;
|
use swarm_secret_client::review_classifier::forge_token_path;
|
||||||
|
|
||||||
use super::Client;
|
use super::Client;
|
||||||
use super::agent_token::{Decision, Observed, classify_named};
|
use super::agent_token::{Decision, Observed, classify_named, is_not_found};
|
||||||
|
|
||||||
/// The classifier's forge login: the reviewer someone requests.
|
/// The classifier's forge login: the reviewer someone requests.
|
||||||
pub const LOGIN: &str = "review-classifier";
|
pub const LOGIN: &str = "review-classifier";
|
||||||
|
|
@ -31,8 +37,8 @@ pub const LOGIN: &str = "review-classifier";
|
||||||
/// The forge's name for the account's one token.
|
/// The forge's name for the account's one token.
|
||||||
pub const TOKEN_NAME: &str = "swarm-review-classifier";
|
pub const TOKEN_NAME: &str = "swarm-review-classifier";
|
||||||
|
|
||||||
/// What the token may do: post reviews, which the forge files under the
|
/// What the token may do: post reviews and push branches, both of which the
|
||||||
/// repository scope. Which repos is the account's access, granted by hand.
|
/// forge files under the repository scope. Which repos is the account's access, granted by hand.
|
||||||
pub const TOKEN_SCOPES: &str = "write:repository";
|
pub const TOKEN_SCOPES: &str = "write:repository";
|
||||||
|
|
||||||
/// How often [`spawn`] re-checks the account and its token.
|
/// How often [`spawn`] re-checks the account and its token.
|
||||||
|
|
@ -62,6 +68,29 @@ pub struct Review {
|
||||||
pub body: String,
|
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 {
|
impl Client {
|
||||||
/// Make sure [`LOGIN`] exists and holds a live [`TOKEN_NAME`] token in
|
/// 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
|
/// the store, minting one when it does not. The whole job of the
|
||||||
|
|
@ -121,19 +150,8 @@ impl Client {
|
||||||
commit: Option<&str>,
|
commit: Option<&str>,
|
||||||
review: Review,
|
review: Review,
|
||||||
) -> Result<Option<String>> {
|
) -> Result<Option<String>> {
|
||||||
let path = forge_token_path()?;
|
self.as_classifier()
|
||||||
let store = crate::store::connect()
|
.await?
|
||||||
.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(
|
.repo_create_pull_review(
|
||||||
owner,
|
owner,
|
||||||
repo,
|
repo,
|
||||||
|
|
@ -150,6 +168,101 @@ impl Client {
|
||||||
.map(|posted| posted.html_url.map(String::from))
|
.map(|posted| posted.html_url.map(String::from))
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/// Make `commit` as [`LOGIN`]: one commit on a new branch, from the head
|
||||||
|
/// of its base. Reads the token from the store on every call.
|
||||||
|
///
|
||||||
|
/// # Errors
|
||||||
|
/// When the store holds no token yet, or the forge refuses the commit;
|
||||||
|
/// a [`forgejo_api::ForgejoError`] in the chain carries the forge's
|
||||||
|
/// status.
|
||||||
|
pub async fn push_classifier_branch(
|
||||||
|
&self,
|
||||||
|
owner: &str,
|
||||||
|
repo: &str,
|
||||||
|
commit: BranchCommit,
|
||||||
|
) -> Result<()> {
|
||||||
|
use base64::Engine as _;
|
||||||
|
|
||||||
|
let branch = commit.branch.clone();
|
||||||
|
self.as_classifier()
|
||||||
|
.await?
|
||||||
|
.repo_change_files(
|
||||||
|
owner,
|
||||||
|
repo,
|
||||||
|
ChangeFilesOptions {
|
||||||
|
author: Some(Identity {
|
||||||
|
name: Some(commit.author.name),
|
||||||
|
email: Some(commit.author.email),
|
||||||
|
}),
|
||||||
|
branch: Some(commit.base),
|
||||||
|
committer: None,
|
||||||
|
dates: None,
|
||||||
|
files: vec![ChangeFileOperation {
|
||||||
|
content: Some(
|
||||||
|
base64::engine::general_purpose::STANDARD.encode(&commit.content),
|
||||||
|
),
|
||||||
|
from_path: None,
|
||||||
|
operation: if commit.sha.is_some() {
|
||||||
|
ChangeFileOperationOperation::Update
|
||||||
|
} else {
|
||||||
|
ChangeFileOperationOperation::Create
|
||||||
|
},
|
||||||
|
path: commit.path,
|
||||||
|
sha: commit.sha,
|
||||||
|
}],
|
||||||
|
force_overwrite_new_branch: Some(false),
|
||||||
|
message: Some(commit.message),
|
||||||
|
new_branch: Some(commit.branch),
|
||||||
|
signoff: None,
|
||||||
|
},
|
||||||
|
)
|
||||||
|
.await
|
||||||
|
.with_context(|| format!("pushing {LOGIN}'s branch {branch} to {owner}/{repo}"))?;
|
||||||
|
Ok(())
|
||||||
|
}
|
||||||
|
|
||||||
|
/// The forge account `login` as a commit author: its full name, or its
|
||||||
|
/// login when it has none, and its email. `None` when the forge has no
|
||||||
|
/// such account. Read with this daemon's site-admin token, which sees an
|
||||||
|
/// email the account keeps private.
|
||||||
|
///
|
||||||
|
/// # Errors
|
||||||
|
/// When the forge refuses the read, or the account has no email.
|
||||||
|
pub async fn commit_author(&self, login: &str) -> Result<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 email = user
|
||||||
|
.email
|
||||||
|
.filter(|e| !e.trim().is_empty())
|
||||||
|
.with_context(|| format!("forge user {login} has no email"))?;
|
||||||
|
let name = user
|
||||||
|
.full_name
|
||||||
|
.map(|n| n.trim().to_owned())
|
||||||
|
.filter(|n| !n.is_empty())
|
||||||
|
.unwrap_or_else(|| login.to_owned());
|
||||||
|
Ok(Some(CommitAuthor { name, email }))
|
||||||
|
}
|
||||||
|
|
||||||
|
/// A forge client acting as [`LOGIN`], with the token the store holds.
|
||||||
|
async fn as_classifier(&self) -> Result<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
|
/// The stored token at `path` and every token the forge lists for
|
||||||
/// [`LOGIN`], which must exist.
|
/// [`LOGIN`], which must exist.
|
||||||
async fn read_classifier_token(
|
async fn read_classifier_token(
|
||||||
|
|
@ -306,6 +419,43 @@ mod tests {
|
||||||
assert!(lockdown_patch(&seen, LOGIN).is_none());
|
assert!(lockdown_patch(&seen, LOGIN).is_none());
|
||||||
}
|
}
|
||||||
|
|
||||||
|
#[tokio::test]
|
||||||
|
async fn a_commit_author_is_the_accounts_name_and_email_or_none() {
|
||||||
|
use super::super::tests::stub_forge;
|
||||||
|
|
||||||
|
let (client, _) = stub_forge(
|
||||||
|
(404, "{}"),
|
||||||
|
(
|
||||||
|
200,
|
||||||
|
r#"{"login":"alice","full_name":" Alice A ","email":"a@x.test"}"#,
|
||||||
|
),
|
||||||
|
(404, "{}"),
|
||||||
|
)
|
||||||
|
.await;
|
||||||
|
assert_eq!(
|
||||||
|
client.commit_author("alice").await.expect("read"),
|
||||||
|
Some(CommitAuthor {
|
||||||
|
name: "Alice A".to_owned(),
|
||||||
|
email: "a@x.test".to_owned(),
|
||||||
|
})
|
||||||
|
);
|
||||||
|
let (client, _) = stub_forge(
|
||||||
|
(404, "{}"),
|
||||||
|
(200, r#"{"login":"bob","full_name":"","email":"b@x.test"}"#),
|
||||||
|
(404, "{}"),
|
||||||
|
)
|
||||||
|
.await;
|
||||||
|
let bob = client.commit_author("bob").await.expect("read");
|
||||||
|
assert_eq!(bob.map(|a| a.name).as_deref(), Some("bob"));
|
||||||
|
let (client, _) = stub_forge(
|
||||||
|
(404, "{}"),
|
||||||
|
(404, r#"{"message":"user does not exist"}"#),
|
||||||
|
(404, "{}"),
|
||||||
|
)
|
||||||
|
.await;
|
||||||
|
assert_eq!(client.commit_author("nobody").await.expect("read"), None);
|
||||||
|
}
|
||||||
|
|
||||||
#[tokio::test]
|
#[tokio::test]
|
||||||
async fn the_job_locks_down_the_classifiers_own_account() {
|
async fn the_job_locks_down_the_classifiers_own_account() {
|
||||||
use super::super::tests::{lockdown_patch, stub_forge};
|
use super::super::tests::{lockdown_patch, stub_forge};
|
||||||
|
|
|
||||||
|
|
@ -3420,6 +3420,7 @@ fn build_app(state: AppState) -> axum::Router {
|
||||||
.routes(routes!(review_classifier::post_try_out))
|
.routes(routes!(review_classifier::post_try_out))
|
||||||
.routes(routes!(review_classifier::get_try_out))
|
.routes(routes!(review_classifier::get_try_out))
|
||||||
.routes(routes!(review_classifier::get_rules))
|
.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::runs::get_runs))
|
||||||
.routes(routes!(
|
.routes(routes!(
|
||||||
review_classifier::settings::get_settings,
|
review_classifier::settings::get_settings,
|
||||||
|
|
|
||||||
|
|
@ -2,7 +2,8 @@
|
||||||
//! `GET /api/review-classifier/try-out/{id}`: run classifications against a
|
//! `GET /api/review-classifier/try-out/{id}`: run classifications against a
|
||||||
//! PR from the swarm UI, then read the result back.
|
//! PR from the swarm UI, then read the result back.
|
||||||
//! `GET /api/review-classifier/rules`: a repo's own classifications, from
|
//! `GET /api/review-classifier/rules`: a repo's own classifications, from
|
||||||
//! the rules file on its default branch.
|
//! the rules file on its default branch. [`open_pr`] puts classifications on
|
||||||
|
//! a new branch of the repo, for a PR.
|
||||||
//!
|
//!
|
||||||
//! The POST reads the PR with this daemon's forge token, sends one request to
|
//! The 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:
|
//! the review classifier on [`SUBJECT`] and answers 202 with an id at once:
|
||||||
|
|
@ -39,6 +40,7 @@ use self::settings::{Settings, SettingsSource};
|
||||||
use crate::forge::{self, DefaultBranchFiles, PullForReview};
|
use crate::forge::{self, DefaultBranchFiles, PullForReview};
|
||||||
use crate::{AppState, error_problem};
|
use crate::{AppState, error_problem};
|
||||||
|
|
||||||
|
pub(crate) mod open_pr;
|
||||||
pub(crate) mod review;
|
pub(crate) mod review;
|
||||||
pub(crate) mod runs;
|
pub(crate) mod runs;
|
||||||
pub(crate) mod settings;
|
pub(crate) mod settings;
|
||||||
|
|
@ -496,7 +498,7 @@ async fn rules(
|
||||||
),
|
),
|
||||||
));
|
));
|
||||||
}
|
}
|
||||||
[file] => file,
|
[file] => (&file.path, &file.bytes),
|
||||||
_ => {
|
_ => {
|
||||||
return Err(error_problem(
|
return Err(error_problem(
|
||||||
StatusCode::UNPROCESSABLE_ENTITY,
|
StatusCode::UNPROCESSABLE_ENTITY,
|
||||||
|
|
@ -765,17 +767,23 @@ pub(crate) mod tests {
|
||||||
use tower::ServiceExt as _;
|
use tower::ServiceExt as _;
|
||||||
|
|
||||||
use super::*;
|
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
|
/// 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
|
/// branch, `settings` stored, and a classifier answering every run with
|
||||||
/// `reply` and keeping its request. `o/empty` has no commit, `o/down`
|
/// `reply` and keeping its request. `o/empty` has no commit, `o/down`
|
||||||
/// fails with something other than a 404, and `o/hang` never answers.
|
/// 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)]
|
#[derive(Clone)]
|
||||||
pub(crate) struct Fake {
|
pub(crate) struct Fake {
|
||||||
reply: Reply,
|
reply: Reply,
|
||||||
files: Vec<(&'static str, String)>,
|
files: Vec<(&'static str, String)>,
|
||||||
pub(crate) settings: Settings,
|
pub(crate) settings: Settings,
|
||||||
pub(crate) sent: Arc<std::sync::Mutex<Vec<TryOut>>>,
|
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 {
|
impl Fake {
|
||||||
|
|
@ -785,10 +793,37 @@ pub(crate) mod tests {
|
||||||
files,
|
files,
|
||||||
settings: Settings::default(),
|
settings: Settings::default(),
|
||||||
sent: Arc::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 {
|
impl SettingsSource for Fake {
|
||||||
async fn settings(&self) -> anyhow::Result<Settings> {
|
async fn settings(&self) -> anyhow::Result<Settings> {
|
||||||
Ok(self.settings.clone())
|
Ok(self.settings.clone())
|
||||||
|
|
@ -825,10 +860,15 @@ pub(crate) mod tests {
|
||||||
("o", "r") => Ok(Some(DefaultBranchFiles {
|
("o", "r") => Ok(Some(DefaultBranchFiles {
|
||||||
branch: "main".to_owned(),
|
branch: "main".to_owned(),
|
||||||
commit: "def456".to_owned(),
|
commit: "def456".to_owned(),
|
||||||
|
web_url: Some(url::Url::parse("https://forge.test/o/r").expect("a URL")),
|
||||||
files: paths
|
files: paths
|
||||||
.iter()
|
.iter()
|
||||||
.filter_map(|p| self.files.iter().find(|(f, _)| f == p))
|
.filter_map(|p| self.files.iter().find(|(f, _)| f == p))
|
||||||
.map(|(f, body)| ((*f).to_owned(), body.clone().into_bytes()))
|
.map(|(f, body)| BranchFile {
|
||||||
|
path: (*f).to_owned(),
|
||||||
|
sha: format!("sha-{f}"),
|
||||||
|
bytes: body.clone().into_bytes(),
|
||||||
|
})
|
||||||
.collect(),
|
.collect(),
|
||||||
})),
|
})),
|
||||||
("o", "empty") => Ok(None),
|
("o", "empty") => Ok(None),
|
||||||
|
|
|
||||||
837
swarm-controller/src/review_classifier/open_pr.rs
Normal file
837
swarm-controller/src/review_classifier/open_pr.rs
Normal file
|
|
@ -0,0 +1,837 @@
|
||||||
|
//! `POST /api/review-classifier/open-pr`: put classifications written on the
|
||||||
|
//! try-out page on a new branch of the repo, for the operator to open a PR
|
||||||
|
//! from.
|
||||||
|
//!
|
||||||
|
//! The classifications are merged by trimmed name into the rules file on the
|
||||||
|
//! repo's default branch: one with a name the file holds replaces that entry,
|
||||||
|
//! one with a new name goes at the end, and every other entry stays. A repo
|
||||||
|
//! with no rules file gets `.hyperhive/review-classifier.yml`. In a YAML file,
|
||||||
|
//! the entries the merge leaves alone keep their text, comments included.
|
||||||
|
//!
|
||||||
|
//! The commit is made by the classifier's forge account
|
||||||
|
//! ([`LOGIN`]) with its own token, on a branch of the repo itself, so that
|
||||||
|
//! account needs write access to the repo. Its author is the operator: the
|
||||||
|
//! `Remote-User` the gateway sets names the forge account of the same login,
|
||||||
|
//! since a forge account made by an SSO login is named after the SSO user.
|
||||||
|
//! The answer links to the forge's compare page for the branch, where the
|
||||||
|
//! operator opens the PR as themselves.
|
||||||
|
|
||||||
|
use std::fmt::Write as _;
|
||||||
|
use std::future::Future;
|
||||||
|
|
||||||
|
use axum::Json;
|
||||||
|
use axum::extract::State;
|
||||||
|
use axum::extract::rejection::JsonRejection;
|
||||||
|
use axum::http::{HeaderMap, StatusCode};
|
||||||
|
use forgejo_api::{ApiErrorKind, ForgejoError};
|
||||||
|
use problem_details::ProblemDetails;
|
||||||
|
use serde::{Deserialize, Serialize};
|
||||||
|
use swarm_queue_client::review_classifier::{Classification, Polarity};
|
||||||
|
use utoipa::ToSchema;
|
||||||
|
|
||||||
|
use super::{FileSource, RULES_PATHS, check_classifications, operator, parse_rules, split_repo};
|
||||||
|
use crate::forge::classifier_account::{BranchCommit, CommitAuthor, LOGIN};
|
||||||
|
use crate::forge::{self, BranchFile};
|
||||||
|
use crate::{AppState, error_problem};
|
||||||
|
|
||||||
|
/// Where a branch's name starts, so the forge's branch list groups them.
|
||||||
|
const BRANCH_PREFIX: &str = "review-classifier/";
|
||||||
|
|
||||||
|
/// The longest a line of a folded YAML description gets, indent included.
|
||||||
|
const YAML_WIDTH: usize = 78;
|
||||||
|
|
||||||
|
/// Classifications to propose to a repo.
|
||||||
|
#[derive(Debug, Deserialize, ToSchema)]
|
||||||
|
pub(crate) struct OpenPrRequest {
|
||||||
|
/// `owner/name`.
|
||||||
|
repo: String,
|
||||||
|
/// One to ten, each with a distinct name.
|
||||||
|
classifications: Vec<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(), ¤t, &merged)
|
||||||
|
};
|
||||||
|
let mut message = String::from("review classifier:");
|
||||||
|
for (verb, names) in [("add", &added), ("update", &changed)] {
|
||||||
|
if !names.is_empty() {
|
||||||
|
let sep = if message.ends_with(':') { " " } else { "; " };
|
||||||
|
let _ = write!(message, "{sep}{verb} {}", names.join(", "));
|
||||||
|
}
|
||||||
|
}
|
||||||
|
Ok(Merged {
|
||||||
|
path,
|
||||||
|
sha,
|
||||||
|
content: content.into_bytes(),
|
||||||
|
message,
|
||||||
|
})
|
||||||
|
}
|
||||||
|
|
||||||
|
/// `merged` as a YAML rules file. Each entry of `text` that `merged` holds
|
||||||
|
/// unchanged at the same place keeps its text, as long as the result reads
|
||||||
|
/// back as `merged`; otherwise the whole file is written out.
|
||||||
|
fn yaml(text: Option<&str>, current: &[Classification], merged: &[Classification]) -> String {
|
||||||
|
if let Some((head, entries, indent)) = text.and_then(entry_lines) {
|
||||||
|
let mut out = head;
|
||||||
|
for (i, c) in merged.iter().enumerate() {
|
||||||
|
match entries.get(i) {
|
||||||
|
Some(kept) if current.get(i) == Some(c) => out.push_str(kept),
|
||||||
|
_ => {
|
||||||
|
if !out.is_empty() && !out.ends_with('\n') {
|
||||||
|
out.push('\n');
|
||||||
|
}
|
||||||
|
out.push_str(&yaml_entry(c, &indent));
|
||||||
|
}
|
||||||
|
}
|
||||||
|
}
|
||||||
|
if parse_rules(".yml", out.as_bytes()).as_deref() == Ok(merged) {
|
||||||
|
return out;
|
||||||
|
}
|
||||||
|
}
|
||||||
|
let mut out = String::from("classifications:\n");
|
||||||
|
for c in merged {
|
||||||
|
out.push_str(&yaml_entry(c, " "));
|
||||||
|
}
|
||||||
|
out
|
||||||
|
}
|
||||||
|
|
||||||
|
/// `text` split into what comes before the first entry of its
|
||||||
|
/// `classifications:` list, the text of each entry up to the next one, and
|
||||||
|
/// the indent of their `-`. `None` when no entry follows that key. The split
|
||||||
|
/// is only a guess at the layout: [`yaml`] keeps it when what it builds reads
|
||||||
|
/// back as it should.
|
||||||
|
fn entry_lines(text: &str) -> Option<(String, Vec<String>, 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}");
|
||||||
|
}
|
||||||
|
}
|
||||||
|
}
|
||||||
Loading…
Reference in a new issue