diff --git a/docs/swarm/README.md b/docs/swarm/README.md index 72693dc0..8c89734c 100644 --- a/docs/swarm/README.md +++ b/docs/swarm/README.md @@ -318,10 +318,14 @@ the agent runs on, or on a forge container there. ### Swarm-wide forge webhooks -At startup the controller registers two Forgejo hooks pointing at -itself — a `push` hook on `internal/knowledge` and a `pull_request` hook -on the `agent-configs` org, both under -`https:///webhook/forge/`. +At startup the controller registers its Forgejo hooks pointing at +itself, all under `https:///webhook/forge/`: + +- a `push` hook on `internal/knowledge` +- a `pull_request` hook on the `agent-configs` org +- an instance-wide `push` system hook, for commit metrics +- an instance-wide `pull_request_review_request` system hook, which starts + the [review classifier](review-classifier.md#trigger-and-lifecycle) The controller **interprets** a delivery and sends hives a specific message — _the knowledge repo changed_, _deploy agent `foo` at rev diff --git a/docs/swarm/credentials.md b/docs/swarm/credentials.md index 11fe8bfc..33d87b39 100644 --- a/docs/swarm/credentials.md +++ b/docs/swarm/credentials.md @@ -74,6 +74,7 @@ of the cell says how. | `swarm/services//oidc/client` | authelia | that service's own reader, under its own certificate — the queue responder's under `swarm-nats-auth`'s, the homeserver's under `swarm-matrix-ctl`'s, the forge's under `swarm-forge-publisher`'s, the review classifier's under `swarm-review-classifier`'s; the bao UI's by the store's granter | must be stated | must be stated | | `swarm/services/ci-runner/registration-token` | `swarm-controller`, from the forge's admin API, at start and in a five-minute pass; its grant on the path is write-only | `swarm-bao-ci-runner` on the runner's host, under the runner's own certificate, into a file only `swarm-ci-runner-register` in the runner's container loads; no other principal's policy reads it | ❌ the forge hands out one token until an admin resets it; the controller writes a reset token within five minutes | ❌ `swarm-bao-ci-runner` reads it when it starts, at boot or deploy, and restarts the runner when the value changed | | `swarm/services/review-classifier/anthropic-api-key` | the operator, by hand in the store's UI, field `api_key` | `swarm-review-classifier` in its container, under its own certificate, at the start of every run; held for that run and handed to `claude`'s environment only; no other principal's policy reads it | ❌ an operator's key; replaced by writing a new one | ✅ read on every run, so a written or removed key takes effect on the next run | +| `swarm/services/review-classifier/forge/token` | `swarm-controller`, with the forge's admin API, for its `review-classifier` account, at start and in a five-minute pass | `swarm-controller` under its own certificate, before it decides whether to mint, and on every review it posts as the account; the classifier service's policy doesn't read it | ✅ the controller's pass re-mints when the stored token is missing or no longer matches the forge (last eight characters and scopes) | ✅ the controller reads the store on every review it posts | | _(not in the store)_ a hive's mTLS leaf | the store's own PKI, or an operator placing it by hand | its own client, off disk — the exception above, because it's what makes every other row's pull possible | must be stated | must be stated | diff --git a/docs/swarm/review-classifier.md b/docs/swarm/review-classifier.md index 61376371..10536245 100644 --- a/docs/swarm/review-classifier.md +++ b/docs/swarm/review-classifier.md @@ -68,30 +68,36 @@ classifications: ## Trigger and lifecycle -The classifier has a dedicated forge service account. Requesting that -account as a reviewer on a PR starts a run: +The classifier has a dedicated forge service account, `review-classifier` +(see [The service account](#the-service-account)). Requesting that account +as a reviewer on a PR starts a run: -1. The forge sends the review-request webhook to +1. The forge sends the `pull_request_review_request` delivery to [`swarm-controller`](../../swarm-controller/README.md), the swarm's one - webhook owner. -2. The controller hands the run to the classifier service as an async - message. -3. The classifier runs every classification in the repo's config, read from - the default branch, against the PR's commits and diff. -4. The service account posts the result on the PR as a review. + webhook owner, on its instance-wide hook `/webhook/forge/review-request`. + A delivery for any other reviewer, or a removed request, does nothing. +2. The controller reads the PR and the repo's rules from the default branch, + and hands the run to the classifier service as one queue request on + `$SWARM.review-classifier.try-out`, the subject a try-out uses. +3. The classifier runs every classification against the PR's commits and + diff. +4. The controller posts the result on the PR as the service account's + review, on the commit it read. -A verdict belongs to the commit it ran on. Once the PR's head moves, -requesting the service account as a reviewer again starts a fresh run on the -new head. +A verdict belongs to the commit it ran on. A push starts no run. Once the +PR's head moves, requesting the service account as a reviewer again starts a +fresh run on the new head. -That only holds if the bot's review on an older commit stops counting once -the head moves. Either the repo enables Forgejo's stale-review dismissal -(see [PR review gate](../process/pr-review-gate.md)), or the service -dismisses its own earlier review when it posts a new one. Where neither -applies, an old approval keeps counting and an old request-changes keeps -blocking. +The re-request also retires the account's earlier review. The forge clears +the `official` flag on a reviewer's earlier reviews when it's requested +again, and only official reviews count toward the required approvals or +block a merge. Removing the request before the new review lands restores +the earlier one. A repo whose branch protection also dismisses stale approvals (see +[PR review gate](../process/pr-review-gate.md)) stops counting an approval +on an older commit even without a re-request. -Runs go in parallel, up to a maximum set at swarm level. +At most two requested reviews run at once. More wait in the controller's +memory, so a controller restart drops them, and nothing gets posted for them. ## Components @@ -137,6 +143,31 @@ and from every agent's account. It's the reviewer someone requests, it authors every classifier review, and it pushes the branches the try-out page creates. +`swarm-controller` creates it as `review-classifier`, with the email +`review-classifier@services.hyperhive.local` and no repo creation, and mints +its one token, `swarm-review-classifier` with scope `write:repository`, into +the store at `swarm/services/review-classifier/forge/token`. A pass at start +and every five minutes re-mints when the token is missing or no longer +matches the forge. The controller refuses an existing account by that name +with any other email. The controller posts the reviews with that token, and +the classifier service never sees it. + +**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 +collaborator: + +- with **read**, so someone can request it as a reviewer, and its reviews show, + but its approval doesn't count +- with **write**, so its approval counts toward the required approvals and + 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. + +The repo also needs a rules file on its default branch (see +[Repo configuration](#repo-configuration)). Without one, a request posts a +comment saying so. + ## The classification run ### Shared context @@ -209,6 +240,18 @@ like any other ([`pr-review-gate.md`](../process/pr-review-gate.md)). The Repos that rely on it typically raise their required approval count to account for it. +A run with no clean verdict never approves. When the PR or the rules don't +load, the run fails, or **any** classification has no answer, the account +posts a **comment** instead, starting with `**No verdict:**` and the reason, +followed by the table when there is one. A comment neither approves nor +blocks the merge: nine timeouts and one pass post a comment, not an +approval. + +The controller flattens every cell to one line, with `|`, backticks, brackets, `<`, `>`, +`&`, `#` and other markup characters escaped, and a zero-width space after +each `@`, so a model's `reason` can't break the table, embed markup, or +mention anyone. + ## Runtimes The classifier runs on one of three runtimes, chosen at swarm level: @@ -302,9 +345,6 @@ These get settled, and filled in here, as the work reaches them: and how the recent-runs page reads them. - **Oversized diffs.** What happens when the diff doesn't fit the model's context. Lockfiles already collapse to one line each, as in `hive-forge diff`. -- **Stale reviews.** Whether the repo's stale-review dismissal setting - retires the bot's review on an older commit, or the service dismisses its - own earlier review when it posts a new one. - **Budget.** Any per-run or per-day spend limit on top of the concurrency cap. - **Diff-leaves-swarm policy.** On the API key and ACP runtimes the diff @@ -315,20 +355,17 @@ These get settled, and filled in here, as the work reaches them: - **The "no tools" mechanism on ACP.** How the ACP runtime runs a session with no tools at all. The claude runtimes pass `--tools ""`. - **Credential paths.** Where in bao the ACP and subscription runtimes' - credentials and the service account's forge token live, and which component - holds the token. The API key is at - `secret/swarm/services/review-classifier/anthropic-api-key`, field `api_key`. + credentials live. The API key is at + `secret/swarm/services/review-classifier/anthropic-api-key`, field `api_key`, + and the service account's forge token at + `secret/swarm/services/review-classifier/forge/token`. - **Model naming across runtimes.** How the swarm-level model setting handles runtime-specific model names. - **ACP cost labels.** How an ACP run's cost gets a repo and PR label, so it reads from the metrics store like the claude runtimes' cost. -- **Async message medium for review runs.** Which queue subjects or streams - carry a webhook-started run. A try-out is one request on - `$SWARM.review-classifier.try-out`, answered on its reply subject. - **Reviewer handoff.** How a later reviewer, human or agent, learns the classifier passed a PR before it starts its own review. -- **Opt-in details.** Beyond the config file: the service account's access - to a repo, so someone can request it as reviewer and it can push try-out - branches. +- **Try-out branch access.** The service account's access for pushing try-out + branches, beyond what a repo admin gives it for reviews. - **Commit author for try-out PRs.** How the classifier derives the commit author's name and email from the swarm UI login. diff --git a/nix/host-modules/swarm-bao.nix b/nix/host-modules/swarm-bao.nix index 3eb11a8f..664c1241 100644 --- a/nix/host-modules/swarm-bao.nix +++ b/nix/host-modules/swarm-bao.nix @@ -437,6 +437,10 @@ let capabilities = ["create", "update"] } + path "${credentialMountPath}/data/${classifierForgeTokenLeaf}" { + capabilities = ["create", "read", "update"] + } + path "${credentialMountPath}/data/${swarmAppserviceTokenLeaf}" { capabilities = ["read"] } @@ -567,6 +571,12 @@ let # wrote in memory); the CI runner's fetch unit is its one reader. ciRunnerTokenLeaf = "swarm/services/ci-runner/registration-token"; + # The review classifier's forge token, the nix half of + # `swarm_secret_client::review_classifier::forge_token_path`. The controller + # mints it, reads it back before deciding whether to re-mint, and posts the + # classifier's reviews with it; no other principal's policy reaches it. + classifierForgeTokenLeaf = "swarm/services/review-classifier/forge/token"; + # The UI's OIDC client secret. The publisher writes it; the granter reads it # into the `oidc` auth method's config, which is the only copy bao uses. baoUiClientLeaf = "swarm/services/${cfg.ui.oidc.clientId}/oidc/client"; diff --git a/nix/module-eval/bao-grants.nix b/nix/module-eval/bao-grants.nix index ba116aac..aa23817b 100644 --- a/nix/module-eval/bao-grants.nix +++ b/nix/module-eval/bao-grants.nix @@ -1377,6 +1377,19 @@ let lib.hasInfix "path \"secret/data/swarm/services/ci-runner/registration-token\" {\n capabilities = [\"create\", \"update\"]\n}" s && !(lib.hasInfix "secret/data/swarm/services/*" s); } + { + # `forge::classifier_account` mints the review classifier's forge token + # and reads it back to decide on a re-mint and to post reviews. Pinned + # as the whole stanza, and spelled to the leaf, for the reason above. + name = "the controller's grant on the review classifier is its forge token alone"; + ok = + let + s = baoGrantHere.systemd.services.swarm-bao-controller-policy.script; + in + lib.hasInfix "path \"secret/data/swarm/services/review-classifier/forge/token\" {\n capabilities = [\"create\", \"read\", \"update\"]\n}" s + && !(lib.hasInfix "secret/data/swarm/services/review-classifier/*" s) + && !(lib.hasInfix "secret/data/swarm/services/review-classifier/anthropic-api-key" s); + } { # The runner's role is the token's only reader. Its policy is the one # leaf, read-only, under the runner's own subject. diff --git a/swarm-controller/src/forge.rs b/swarm-controller/src/forge.rs index eefe0934..4e1748c7 100644 --- a/swarm-controller/src/forge.rs +++ b/swarm-controller/src/forge.rs @@ -40,6 +40,7 @@ use crate::webhook::DeliveryKind; use swarm_queue_client::review_classifier::Commit; pub mod agent_token; +pub mod classifier_account; pub mod docs_grant; pub mod hive_token; pub mod legacy_tokens; @@ -1289,6 +1290,9 @@ impl Client { // in an org created after this hook was registered, with no // per-org registration to keep in sync as orgs come and go. DeliveryKind::VcsActivity => ("push", HookScope::Instance), + // Instance-wide too: any repo whose admin gave the + // classifier's account access can request its review. + DeliveryKind::ReviewRequest => ("pull_request_review_request", HookScope::Instance), }; self.ensure_hook(&scope, &target_url, event, secret) .await diff --git a/swarm-controller/src/forge/classifier_account.rs b/swarm-controller/src/forge/classifier_account.rs new file mode 100644 index 00000000..38999014 --- /dev/null +++ b/swarm-controller/src/forge/classifier_account.rs @@ -0,0 +1,328 @@ +//! The review classifier's forge account, [`LOGIN`]: created here with the +//! admin API and given one token, stored at +//! `swarm_secret_client::review_classifier::forge_token_path`. Requesting +//! this account as a reviewer starts a classifier run, and this daemon posts +//! the result as the account's review (`crate::review_classifier::review`). +//! The classifier itself never holds the token. +//! +//! 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. +//! +//! 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 +//! eight characters and scopes is kept, and a pass whose reads fail queues +//! nothing. + +use std::sync::Arc; + +use anyhow::{Context, Result, bail}; +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}; + +/// The classifier's forge login: the reviewer someone requests. +pub const LOGIN: &str = "review-classifier"; + +/// The forge's name for the account's one token. +pub const TOKEN_NAME: &str = "swarm-review-classifier"; + +/// What the token may do: post reviews, which the forge files under the +/// repository scope. Which repos is the account's access, granted by hand. +pub const TOKEN_SCOPES: &str = "write:repository"; + +/// How often [`spawn`] re-checks the account and its token. +const RECONCILE_INTERVAL: std::time::Duration = std::time::Duration::from_mins(5); + +/// The email this module creates the account with, and the one it requires +/// before acting on an account named [`LOGIN`]. Under its own domain, so it +/// equals no agent's or hive's account email. +const EMAIL: &str = "review-classifier@services.hyperhive.local"; + +/// Whether a pass that found `seen` queues a `MintClassifierForgeToken` job. +/// [`Observed::Unknown`] never does, so a forge or store outage can't rotate +/// the token, and neither does a [`Decision::Conflict`], which the job would +/// refuse. +pub fn needs_job(seen: Observed) -> bool { + matches!( + seen, + Observed::NoForgeUser | Observed::Decided(Decision::Mint(_)) + ) +} + +/// One review as the classifier's account posts it. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct Review { + /// `APPROVED`, `REQUEST_CHANGES` or `COMMENT`. + pub event: &'static str, + pub body: String, +} + +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 + /// `MintClassifierForgeToken` node. + /// + /// # Errors + /// When the store or the forge refuses a step, when an account named + /// [`LOGIN`] exists with another email, or when the read-back does not + /// show the new token. + pub async fn ensure_classifier_forge_token(&self) -> Result<()> { + let path = forge_token_path()?; + self.ensure_classifier_user().await?; + let store = crate::store::connect() + .await + .context("logging in to the swarm secret store")?; + let (stored, listed) = self.read_classifier_token(&store, &path).await?; + let reason = match classify_named(TOKEN_NAME, TOKEN_SCOPES, stored.as_ref(), &listed) { + Decision::Keep => { + tracing::debug!("review classifier forge token is current; left as it is"); + return Ok(()); + } + Decision::Conflict => bail!( + "the forge lists more than one {TOKEN_NAME:?} token for {LOGIN:?}; delete them \ + by id and re-run" + ), + Decision::Mint(reason) => reason, + }; + let credential = self + .replace_token(&store, LOGIN, TOKEN_NAME, TOKEN_SCOPES, &listed, &path) + .await?; + + Forgejo::new(Auth::Token(&credential.value), self.url.clone()) + .context("building a forge client with the new token")? + .repo_search(RepoSearchQuery::default()) + .await + .with_context(|| format!("using {LOGIN}'s new forge token"))?; + let (stored, listed) = self.read_classifier_token(&store, &path).await?; + let now = classify_named(TOKEN_NAME, TOKEN_SCOPES, stored.as_ref(), &listed); + if now != Decision::Keep { + bail!("{LOGIN}'s new forge token does not read back as current: {now:?}"); + } + tracing::info!(?reason, %path, "review classifier forge token minted and stored"); + Ok(()) + } + + /// Post `review` on PR `number` of `owner/repo` as [`LOGIN`], on + /// `commit` when one is given. Reads the token from the store on every + /// call. + /// + /// # Errors + /// When the store holds no token yet, or the forge refuses the review. + pub async fn post_classifier_review( + &self, + owner: &str, + repo: &str, + number: i64, + commit: Option<&str>, + review: Review, + ) -> Result<()> { + let path = forge_token_path()?; + let store = crate::store::connect() + .await + .context("logging in to the swarm secret store")?; + let Some(credential) = store + .read_optional::(&path) + .await + .with_context(|| format!("reading {path}"))? + else { + bail!("nothing at {path}: the classifier's forge token is not minted yet"); + }; + Forgejo::new(Auth::Token(&credential.value), self.url.clone()) + .context("building a forge client with the classifier's token")? + .repo_create_pull_review( + owner, + repo, + number, + CreatePullReviewOptions { + body: Some(review.body), + comments: None, + commit_id: commit.map(str::to_owned), + event: Some(review.event.to_owned()), + }, + ) + .await + .with_context(|| { + format!("posting {LOGIN}'s review on PR {number} of {owner}/{repo}") + })?; + Ok(()) + } + + /// The stored token at `path` and every token the forge lists for + /// [`LOGIN`], which must exist. + async fn read_classifier_token( + &self, + store: &swarm_secret_client::SecretStore, + path: &str, + ) -> Result<( + Option, + Vec, + )> { + let stored = store + .read_optional(path) + .await + .with_context(|| format!("reading {path}"))?; + let Some(listed) = self.list_agent_tokens(LOGIN).await? else { + bail!("the forge has no user {LOGIN:?} after this job created it"); + }; + Ok((stored, listed)) + } + + /// Create [`LOGIN`] if there is none, refuse one with another email, and + /// lock it out of creating repos. + async fn ensure_classifier_user(&self) -> Result<()> { + self.create_user(LOGIN, EMAIL).await?; + let user = self + .api + .user_get(LOGIN) + .await + .with_context(|| format!("read forge user {LOGIN}"))?; + if user.email.as_deref() != Some(EMAIL) { + bail!( + "forge account `{LOGIN}` exists but is not the review classifier's (email {:?}, \ + expected `{EMAIL}`); refusing to mint a token for it", + user.email + ); + } + self.api + .admin_edit_user(LOGIN, super::repo_creation_lockdown(LOGIN)) + .await + .with_context(|| format!("disable repo creation for forge user {LOGIN}"))?; + Ok(()) + } + + /// What the store and the forge say about the account's token. + async fn observe_classifier(&self) -> Result { + let path = forge_token_path()?; + let store = crate::store::connect() + .await + .context("logging in to the swarm secret store")?; + let stored = match store.read_optional::(&path).await { + Ok(stored) => stored, + Err(e) => { + tracing::warn!(error = %e, "review classifier forge token: store read failed"); + return Ok(Observed::Unknown); + } + }; + Ok(match self.list_agent_tokens(LOGIN).await { + Ok(Some(listed)) => Observed::Decided(classify_named( + TOKEN_NAME, + TOKEN_SCOPES, + stored.as_ref(), + &listed, + )), + Ok(None) => Observed::NoForgeUser, + Err(e) => { + tracing::warn!(error = %format!("{e:#}"), "review classifier forge token: forge read failed"); + Observed::Unknown + } + }) + } +} + +/// 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. +pub fn spawn(client: Arc, enqueue: impl Fn() + Send + 'static) { + tokio::spawn(async move { + let mut ticker = tokio::time::interval(RECONCILE_INTERVAL); + loop { + ticker.tick().await; + let Some(outcome) = super::within_forge_deadline( + "review classifier forge token", + client.observe_classifier(), + ) + .await + else { + continue; + }; + match outcome { + Ok(seen) if needs_job(seen) => { + tracing::info!(?seen, "review classifier forge token: queueing a job"); + enqueue(); + } + Ok(seen) => tracing::debug!(?seen, "review classifier forge token: no job"), + Err(e) => tracing::warn!( + error = %format!("{e:#}"), + retry_in_s = RECONCILE_INTERVAL.as_secs(), + "review classifier forge token: pass failed; retrying next tick" + ), + } + } + }); +} + +#[cfg(test)] +mod tests { + use super::super::agent_token::MintReason; + use super::*; + + #[test] + fn the_token_writes_repositories_and_nothing_else() { + assert_eq!(TOKEN_SCOPES, "write:repository"); + assert_ne!(TOKEN_NAME, super::super::agent_token::AGENT_TOKEN_NAME); + assert_ne!(TOKEN_NAME, super::super::hive_token::HIVE_TOKEN_NAME); + } + + #[test] + fn a_mint_or_a_missing_account_queues_and_nothing_else_does() { + assert!(needs_job(Observed::NoForgeUser)); + assert!(needs_job(Observed::Decided(Decision::Mint( + MintReason::NotStored + )))); + assert!(!needs_job(Observed::Decided(Decision::Keep))); + assert!(!needs_job(Observed::Decided(Decision::Conflict))); + assert!(!needs_job(Observed::Unknown)); + } + + #[test] + fn the_accounts_email_is_no_agents() { + assert_ne!(EMAIL, super::super::agent_email(LOGIN)); + assert_ne!(EMAIL, super::super::legacy_agent_email(LOGIN)); + } + + /// An existing account named like the classifier with an agent's email + /// is refused before the lockdown, so no token is minted for it. + #[tokio::test] + async fn the_job_refuses_an_account_that_is_not_the_classifiers() { + use super::super::tests::{lockdown_patch, stub_forge}; + + let (client, seen) = stub_forge( + (409, r#"{"message":"user already exists"}"#), + (200, r#"{"email":"review-classifier@hyperhive.local"}"#), + (200, "{}"), + ) + .await; + let err = client + .ensure_classifier_user() + .await + .expect_err("an agent's account is not the classifier's"); + assert!(format!("{err:#}").contains("not the review classifier's")); + assert!(lockdown_patch(&seen, LOGIN).is_none()); + } + + #[tokio::test] + async fn the_job_locks_down_the_classifiers_own_account() { + use super::super::tests::{lockdown_patch, stub_forge}; + + let (client, seen) = stub_forge( + (409, r#"{"message":"user already exists"}"#), + ( + 200, + r#"{"email":"review-classifier@services.hyperhive.local"}"#, + ), + (200, "{}"), + ) + .await; + client + .ensure_classifier_user() + .await + .expect("the classifier's own account"); + let body = lockdown_patch(&seen, LOGIN).expect("no lockdown PATCH sent"); + assert_eq!(body["max_repo_creation"], 0); + } +} diff --git a/swarm-controller/src/forge_account.rs b/swarm-controller/src/forge_account.rs index afdc1d7d..c923ebd8 100644 --- a/swarm-controller/src/forge_account.rs +++ b/swarm-controller/src/forge_account.rs @@ -165,6 +165,7 @@ mod tests { create_gate: std::sync::Arc::default(), accounts: Accounts::Fake(Arc::clone(store)), try_outs: super::super::review_classifier::TryOuts::default(), + review_slots: super::super::review_slots(), } } diff --git a/swarm-controller/src/github_account.rs b/swarm-controller/src/github_account.rs index 60e6d924..2867d00a 100644 --- a/swarm-controller/src/github_account.rs +++ b/swarm-controller/src/github_account.rs @@ -135,6 +135,7 @@ mod tests { create_gate: std::sync::Arc::default(), accounts: Accounts::Fake(Arc::clone(store)), try_outs: super::super::review_classifier::TryOuts::default(), + review_slots: super::super::review_slots(), } } diff --git a/swarm-controller/src/linked_accounts.rs b/swarm-controller/src/linked_accounts.rs index b37bc251..fe133158 100644 --- a/swarm-controller/src/linked_accounts.rs +++ b/swarm-controller/src/linked_accounts.rs @@ -970,6 +970,7 @@ pub(crate) mod tests { create_gate: std::sync::Arc::default(), accounts: super::Accounts::Fake(Arc::clone(store)), try_outs: super::super::review_classifier::TryOuts::default(), + review_slots: super::super::review_slots(), } } diff --git a/swarm-controller/src/main.rs b/swarm-controller/src/main.rs index 604b12a2..54ccc0fd 100644 --- a/swarm-controller/src/main.rs +++ b/swarm-controller/src/main.rs @@ -147,6 +147,10 @@ enum SwarmNodeKind { /// the `hives` team, and holds a live token in the swarm store, where /// that hive's hive-c0re reads it. See `forge::hive_token`. MintHiveForgeToken { hive: String }, + /// Make sure the review classifier's forge account exists and holds a + /// live token in the swarm store, where this daemon reads it to post the + /// classifier's reviews. See `forge::classifier_account`. + MintClassifierForgeToken, /// Re-mint `agent`'s queue secret if it is still stored and old enough. /// See `agent_renewal`. /// @@ -187,6 +191,7 @@ impl hive_jobq_wire::WireNode for SwarmNodeKind { SwarmNodeKind::MintAgentMatrixAccount { .. } => "mint_agent_matrix_account".to_owned(), SwarmNodeKind::MintHiveSenderToken { .. } => "mint_hive_sender_token".to_owned(), SwarmNodeKind::MintHiveForgeToken { .. } => "mint_hive_forge_token".to_owned(), + SwarmNodeKind::MintClassifierForgeToken => "mint_classifier_forge_token".to_owned(), SwarmNodeKind::RenewAgentQueueCredential { .. } => { "renew_agent_queue_credential".to_owned() } @@ -217,6 +222,7 @@ impl hive_jobq_wire::WireNode for SwarmNodeKind { } SwarmNodeKind::MintHiveSenderToken { hive } | SwarmNodeKind::MintHiveForgeToken { hive } => serde_json::json!({ "hive": hive }), + SwarmNodeKind::MintClassifierForgeToken => serde_json::json!({}), SwarmNodeKind::TriggerDeploy { hive, agent, rev } => { serde_json::json!({ "agent": agent, "hive": hive, "rev": rev }) } @@ -373,6 +379,7 @@ async fn run_swarm_node( mint_hive_sender(deps.matrix_homeserver.as_deref(), &hive).await } SwarmNodeKind::MintHiveForgeToken { hive } => mint_hive_forge_token(forge, &hive).await, + SwarmNodeKind::MintClassifierForgeToken => mint_classifier_forge_token(forge).await, SwarmNodeKind::RenewAgentQueueCredential { agent } => renew_queue_credential(&agent).await, SwarmNodeKind::SetAgentWanted { hive, agent } => match deps.wanted { None => Outcome::Failed( @@ -527,6 +534,30 @@ async fn mint_hive_forge_token( } } +/// The `MintClassifierForgeToken` arm, lifted out for the same reason. +async fn mint_classifier_forge_token( + forge: Option>, +) -> hive_jobq::scheduler::Outcome { + use hive_jobq::scheduler::Outcome; + + let Some(client) = forge else { + return Outcome::Failed( + "no forge access here (SWARM_CONTROLLER_FORGE_URL unset, or its token \ + not in the swarm secret store), so no review classifier forge token can be minted" + .to_owned(), + ); + }; + match forge_node( + "mint_classifier_forge_token", + client.ensure_classifier_forge_token(), + ) + .await + { + Ok(()) => Outcome::Done, + Err(e) => Outcome::Failed(format!("{e:#}")), + } +} + /// The `MintAgentMatrixAccount` arm, lifted out for the same reason. async fn mint_matrix_account( homeserver: Option<&str>, @@ -862,6 +893,16 @@ struct AppState { /// The review classifier try-outs started here, until their results /// expire. try_outs: review_classifier::TryOuts, + /// Held by each requested review's run, so at most + /// `review_classifier::review::MAX_CONCURRENT_REVIEWS` go at once. + review_slots: Arc, +} + +/// A fresh [`AppState::review_slots`]. +fn review_slots() -> Arc { + Arc::new(tokio::sync::Semaphore::new( + review_classifier::review::MAX_CONCURRENT_REVIEWS, + )) } /// Env var the controller's NixOS module sets from @@ -2488,6 +2529,20 @@ fn queue_hive_forge_token_mints( Ok(ids) } +/// Insert one `MintClassifierForgeToken` job and return its node's id. +/// `forge::classifier_account::spawn`'s periodic pass is the one caller. +fn queue_classifier_forge_token_mint( + sched: &Mutex>, +) -> Result> { + sched + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner) + .insert_job(None, |b| { + vec![b.node(SwarmNodeKind::MintClassifierForgeToken).guid()] + }) + .map_err(|e| anyhow::anyhow!("{e}")) +} + /// The homeserver agents' own matrix accounts are minted on. Unset is this /// controller creating agents with no matrix account, which the mint node /// reports by name rather than `main` refusing to start. @@ -2548,6 +2603,12 @@ fn spawn_forge_workers( } }); let sched = Arc::clone(jobq); + forge::classifier_account::spawn(Arc::clone(&client), move || { + if let Err(e) = queue_classifier_forge_token_mint(&sched) { + tracing::warn!(error = %format!("{e:#}"), "review classifier forge token: queueing failed"); + } + }); + let sched = Arc::clone(jobq); forge::docs_grant::spawn(Arc::clone(&client), move |agents| { if let Err(e) = queue_docs_grants(&sched, agents) { tracing::warn!(error = %format!("{e:#}"), "docs grants: queueing failed"); @@ -3214,6 +3275,7 @@ async fn main() -> Result<()> { create_gate: Arc::default(), accounts: linked_accounts::Accounts::Live, try_outs: review_classifier::TryOuts::default(), + review_slots: review_slots(), }; let app = build_app(state); @@ -3402,6 +3464,7 @@ mod tests { create_gate: std::sync::Arc::default(), accounts: super::linked_accounts::Accounts::Fake(std::sync::Arc::default()), try_outs: super::review_classifier::TryOuts::default(), + review_slots: super::review_slots(), }; (state, sched) } @@ -4316,6 +4379,10 @@ mod tests { hive: "pr1ma".to_owned(), }, ), + ( + "mint_classifier_forge_token", + SwarmNodeKind::MintClassifierForgeToken, + ), ]; for (label, kind) in nodes { let mut sched = hive_jobq::scheduler::Scheduler::new( diff --git a/swarm-controller/src/matrix_account.rs b/swarm-controller/src/matrix_account.rs index b29b549b..944a6483 100644 --- a/swarm-controller/src/matrix_account.rs +++ b/swarm-controller/src/matrix_account.rs @@ -579,6 +579,7 @@ mod tests { create_gate: std::sync::Arc::default(), accounts: Accounts::Fake(Arc::clone(store)), try_outs: super::super::review_classifier::TryOuts::default(), + review_slots: super::super::review_slots(), } } diff --git a/swarm-controller/src/review_classifier.rs b/swarm-controller/src/review_classifier.rs index ef01dd56..f1cec47c 100644 --- a/swarm-controller/src/review_classifier.rs +++ b/swarm-controller/src/review_classifier.rs @@ -14,6 +14,8 @@ //! //! The operator is the `Remote-User` header the gateway sets, as for //! `agent_input`; the classifier names them in its journal line. +//! +//! [`review`] runs the same rules on a review request and posts the result. use std::collections::{BTreeMap, HashMap, HashSet}; use std::fmt::Write as _; @@ -35,6 +37,8 @@ use utoipa::{IntoParams, ToSchema}; use crate::forge::{self, DefaultBranchFiles, PullForReview}; use crate::{AppState, error_problem}; +pub(crate) mod review; + /// How long a finished run's result stays readable. pub(crate) const RESULT_TTL: Duration = Duration::from_hours(1); @@ -707,6 +711,12 @@ pub(crate) mod tests { files: Vec<(&'static str, String)>, } + impl Fake { + pub(crate) fn new(reply: Reply, files: Vec<(&'static str, String)>) -> Self { + Self { reply, files } + } + } + impl PullSource for Fake { async fn pull( &self, @@ -1017,6 +1027,15 @@ pub(crate) mod tests { }) } + /// A JSON rules file holding the one `datetime` classification. + pub(crate) fn rules_json() -> (&'static str, String) { + let [_, json] = RULES_PATHS; + ( + json, + rules_value(&serde_json::json!([datetime()]), &[]).to_string(), + ) + } + /// `file` as both formats, each with its path. fn both_formats(file: &serde_json::Value) -> [(&'static str, String); 2] { let [yml, json] = RULES_PATHS; diff --git a/swarm-controller/src/review_classifier/review.rs b/swarm-controller/src/review_classifier/review.rs new file mode 100644 index 00000000..1888570d --- /dev/null +++ b/swarm-controller/src/review_classifier/review.rs @@ -0,0 +1,586 @@ +//! Real reviews: a review request for the classifier's forge account +//! ([`LOGIN`]) runs the repo's rules against the PR and posts the result as +//! that account's review, on the commit the run read. +//! +//! Only a review request starts a run; a push does not. A fresh request +//! retires the account's earlier review: the forge clears the `official` flag +//! on a reviewer's previous reviews when they are requested again, and only +//! official reviews count toward approvals or block a merge. +//! +//! A run with no clean verdict never approves: see [`no_clean_verdict_event`]. +//! At most [`MAX_CONCURRENT_REVIEWS`] runs go at once; the rest wait here, in +//! memory, so a restart drops them. + +use std::fmt::Write as _; +use std::future::Future; +use std::sync::Arc; + +use serde::Deserialize; +use swarm_queue_client::review_classifier::{Overall, Reply, Row, TryOut, Verdict}; + +use super::{FileSource, PullSource, RunSink, split_repo}; +use crate::AppState; +use crate::forge::{self, classifier_account::LOGIN, classifier_account::Review}; + +/// The most review runs in flight at once. +pub(crate) const MAX_CONCURRENT_REVIEWS: usize = 2; + +/// The forge's review events. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +enum Event { + Approve, + RequestChanges, + Comment, +} + +impl Event { + fn forge(self) -> &'static str { + match self { + Self::Approve => "APPROVED", + Self::RequestChanges => "REQUEST_CHANGES", + Self::Comment => "COMMENT", + } + } +} + +/// What a run posts when it has no clean verdict: the rules didn't load, the +/// PR couldn't be read, the run failed, or any classification has no answer. +/// A comment neither approves nor blocks the merge. +fn no_clean_verdict_event() -> Event { + Event::Comment +} + +/// A review request for [`LOGIN`]. +#[derive(Debug, Clone, PartialEq, Eq)] +pub(crate) struct Requested { + owner: String, + name: String, + pr: i64, + /// Who asked for the review. + requester: String, +} + +#[derive(Deserialize)] +struct Payload { + action: String, + number: i64, + requested_reviewer: Option, + repository: Repository, + sender: Option, +} + +#[derive(Deserialize)] +struct User { + login: String, +} + +#[derive(Deserialize)] +struct Repository { + full_name: String, +} + +impl Requested { + /// The request in a `pull_request_review_request` delivery, when it + /// asks for [`LOGIN`]'s review. A removed request, a request for anyone + /// else and an unparsable body are `None`. + fn parse(body: &[u8]) -> Option { + let payload: Payload = match serde_json::from_slice(body) { + Ok(p) => p, + Err(e) => { + tracing::warn!(error = %e, "webhook: review-request delivery did not parse"); + return None; + } + }; + let reviewer = payload.requested_reviewer?; + if payload.action != "review_requested" || !reviewer.login.eq_ignore_ascii_case(LOGIN) { + return None; + } + let (owner, name) = split_repo(&payload.repository.full_name).ok()?; + Some(Self { + owner: owner.to_owned(), + name: name.to_owned(), + pr: Some(payload.number).filter(|n| *n > 0)?, + requester: payload.sender.map(|s| s.login).unwrap_or_default(), + }) + } +} + +/// Start a review run for a verified `review-request` delivery that asks for +/// [`LOGIN`], and return at once. +pub(crate) fn on_delivery(state: &AppState, body: &[u8]) { + let Some(request) = Requested::parse(body) else { + return; + }; + let Some(forge) = state.forge.get().map(Arc::clone) else { + tracing::warn!( + repo = %format!("{}/{}", request.owner, request.name), + pr = request.pr, + "review requested, but this host has no forge access yet; nothing is posted" + ); + return; + }; + let queue = Queue(state.status.as_ref().map(|s| s.queue_client())); + let slots = Arc::clone(&state.review_slots); + tokio::spawn(async move { + let Ok(_slot) = slots.acquire_owned().await else { + return; + }; + Box::pin(run(&*forge, &*forge, &queue, &*forge, &request)).await; + }); +} + +/// Where a review is posted: the forge in production, a fake in the tests. +pub(crate) trait ReviewPoster { + fn post( + &self, + owner: &str, + repo: &str, + number: i64, + commit: Option<&str>, + review: Review, + ) -> impl Future> + Send; +} + +impl ReviewPoster for forge::Client { + async fn post( + &self, + owner: &str, + repo: &str, + number: i64, + commit: Option<&str>, + review: Review, + ) -> anyhow::Result<()> { + self.post_classifier_review(owner, repo, number, commit, review) + .await + } +} + +/// The swarm queue, when this host has one. +#[derive(Clone)] +struct Queue(Option); + +impl RunSink for Queue { + async fn run(&self, request: Vec) -> Reply { + let Some(client) = &self.0 else { + return Reply::Failed { + error: "no swarm queue is configured on this host".to_owned(), + }; + }; + if let Err(e) = swarm_queue_client::ensure_connected(client) { + return Reply::Failed { + error: swarm_queue_client::chain(&e), + }; + } + client.run(request).await + } +} + +/// How far a run got. +#[derive(Debug, Clone, PartialEq, Eq)] +enum Outcome { + /// Nothing reached the classifier, for this reason. + NotRun(String), + Ran(Reply), +} + +/// Read the PR and the rules, run them, and post the review. +async fn run( + files: &impl FileSource, + pulls: &impl PullSource, + sink: &impl RunSink, + poster: &impl ReviewPoster, + req: &Requested, +) { + let repo = format!("{}/{}", req.owner, req.name); + let (head, outcome) = classify(files, pulls, sink, req).await; + let review = review_for(&outcome); + let event = review.event; + let sent = forge::within_forge_deadline( + "review classifier review", + poster.post(&req.owner, &req.name, req.pr, head.as_deref(), review), + ) + .await; + match sent { + Some(Ok(())) => tracing::info!( + %repo, pr = req.pr, head_sha = head.as_deref().unwrap_or_default(), event, + "review classifier review posted" + ), + Some(Err(e)) => tracing::warn!( + %repo, pr = req.pr, error = %format!("{e:#}"), + "posting the review classifier's review failed" + ), + None => {} + } +} + +/// The PR's head, when it was read, and how far the run got. +async fn classify( + files: &impl FileSource, + pulls: &impl PullSource, + sink: &impl RunSink, + req: &Requested, +) -> (Option, Outcome) { + let repo = format!("{}/{}", req.owner, req.name); + let pull = match forge::within_forge_deadline( + "review classifier PR read", + pulls.pull(&req.owner, &req.name, req.pr), + ) + .await + { + Some(Ok(pull)) => pull, + Some(Err(e)) => { + return ( + None, + Outcome::NotRun(format!("reading the PR failed: {e:#}")), + ); + } + None => { + return ( + None, + Outcome::NotRun("the forge did not answer within the deadline".to_owned()), + ); + } + }; + let head = Some(pull.head_sha.clone()); + let rules = match super::rules(files, &req.owner, &req.name).await { + Ok(axum::Json(rules)) => rules, + Err(problem) => { + let detail = problem.detail.unwrap_or_default(); + return ( + head, + Outcome::NotRun(format!("the rules did not load: {detail}")), + ); + } + }; + let request = TryOut { + operator: req.requester.clone(), + repo, + pr: req.pr.unsigned_abs(), + head_sha: pull.head_sha, + commits: pull.commits, + diff: pull.diff, + classifications: rules.classifications, + }; + let outcome = match serde_json::to_vec(&request) { + Ok(payload) => Outcome::Ran(sink.run(payload).await), + Err(e) => Outcome::NotRun(format!("encoding the run failed: {e}")), + }; + (head, outcome) +} + +/// The review a run's outcome posts. +fn review_for(outcome: &Outcome) -> Review { + let (event, body) = match outcome { + Outcome::NotRun(reason) => (no_clean_verdict_event(), no_verdict(reason, &[])), + Outcome::Ran(Reply::Failed { error }) => ( + no_clean_verdict_event(), + no_verdict(&format!("the run failed: {error}"), &[]), + ), + Outcome::Ran(Reply::Done { rows, overall }) => { + let unanswered = rows.iter().filter(|r| r.error.is_some()).count(); + if unanswered > 0 { + let reason = format!( + "{unanswered} of {} classifications got no answer", + rows.len() + ); + (no_clean_verdict_event(), no_verdict(&reason, rows)) + } else { + let (event, said) = match overall { + Overall::Approve => (Event::Approve, "approve"), + Overall::RequestChanges => (Event::RequestChanges, "request changes"), + }; + (event, format!("{}\n**Overall: {said}.**\n", table(rows))) + } + } + }; + Review { + event: event.forge(), + body, + } +} + +/// A review body for a run without a clean verdict. +fn no_verdict(reason: &str, rows: &[Row]) -> String { + let mut body = format!( + "**No verdict:** {}. This review doesn't approve or request changes.\n", + cell(reason) + ); + if !rows.is_empty() { + body.push('\n'); + body.push_str(&table(rows)); + } + body +} + +/// The result table, one row per classification. +fn table(rows: &[Row]) -> String { + let mut out = String::from( + "| classification | polarity | verdict | certainty | threshold | counted | reason |\n\ + | --- | --- | --- | --- | --- | --- | --- |\n", + ); + for r in rows { + let polarity = match r.polarity { + swarm_queue_client::review_classifier::Polarity::Positive => "positive", + swarm_queue_client::review_classifier::Polarity::Negative => "negative", + }; + let (verdict, certainty, counted, reason) = if let Some(error) = &r.error { + ("no answer", String::new(), "no", error.as_str()) + } else { + let verdict = match r.verdict { + Verdict::Match => "match", + Verdict::NoMatch => "no match", + Verdict::NotApplicable => "not applicable", + }; + let counted = match (r.counted, r.passed) { + (false, _) => "no", + (true, true) => "yes ✅", + (true, false) => "yes ❌", + }; + (verdict, r.certainty.to_string(), counted, r.reason.as_str()) + }; + let _ = writeln!( + out, + "| {} | {polarity} | {verdict} | {certainty} | {} | {counted} | {} |", + cell(&r.name), + r.threshold, + cell(reason) + ); + } + out +} + +/// `text` as one table cell's inline text: on one line, with every character +/// that could end the cell, start markup or raw HTML escaped, and every `@` +/// followed by a zero-width space, so no name after it is a live mention. +fn cell(text: &str) -> String { + let mut out = String::with_capacity(text.len()); + for (i, word) in text.split_whitespace().enumerate() { + if i > 0 { + out.push(' '); + } + for c in word.chars() { + match c { + '\\' | '|' | '`' | '*' | '_' | '~' | '[' | ']' | '(' | ')' | '!' | '<' | '>' + | '&' | '#' => { + out.push('\\'); + out.push(c); + } + '@' => out.push_str("@\u{200b}"), + c => out.push(c), + } + } + } + out +} + +#[cfg(test)] +mod tests { + use std::sync::Mutex; + + use swarm_queue_client::review_classifier::Polarity; + + use super::*; + use crate::review_classifier::tests::{Fake, rules_json}; + + /// Every review [`run`] posted, with the commit it named. + #[derive(Default)] + struct Posted(Mutex, Review)>>); + + impl ReviewPoster for Posted { + async fn post( + &self, + owner: &str, + repo: &str, + number: i64, + commit: Option<&str>, + review: Review, + ) -> anyhow::Result<()> { + self.0.lock().expect("unpoisoned").push(( + format!("{owner}/{repo}#{number}"), + commit.map(str::to_owned), + review, + )); + Ok(()) + } + } + + fn row(error: Option<&str>, counted: bool, passed: bool) -> Row { + Row { + name: "datetime".to_owned(), + polarity: Polarity::Positive, + verdict: if error.is_some() { + Verdict::NotApplicable + } else { + Verdict::Match + }, + certainty: if error.is_some() { 0 } else { 90 }, + threshold: 80, + counted, + passed, + reason: String::new(), + error: error.map(str::to_owned), + } + } + + fn request(name: &str) -> Requested { + Requested { + owner: "o".to_owned(), + name: name.to_owned(), + pr: 7, + requester: "alice".to_owned(), + } + } + + /// What one run against the fake forge posts. + async fn posted(reply: Reply, repo: &str) -> (String, Option, Review) { + let fake = Fake::new(reply, vec![rules_json()]); + let poster = Posted::default(); + run(&fake, &fake, &fake, &poster, &request(repo)).await; + let mut all = poster.0.into_inner().expect("unpoisoned"); + assert_eq!(all.len(), 1, "one review per run"); + all.remove(0) + } + + #[tokio::test] + async fn nine_timeouts_and_one_pass_do_not_approve() { + let mut rows: Vec = (0..9) + .map(|_| row(Some("timed out after 3 minutes"), false, false)) + .collect(); + rows.push(row(None, true, true)); + // The classifier's own verdict for these rows is approve. + let reply = Reply::Done { + rows, + overall: Overall::Approve, + }; + let (_, commit, review) = posted(reply, "r").await; + assert_eq!(review.event, "COMMENT"); + assert!( + review + .body + .starts_with("**No verdict:** 9 of 10 classifications got no answer."), + "{}", + review.body + ); + assert!(review.body.contains("timed out after 3 minutes")); + assert_eq!(commit.as_deref(), Some("abc123")); + } + + #[tokio::test] + async fn a_clean_run_posts_its_verdict_on_the_head_commit() { + let (target, commit, review) = posted( + Reply::Done { + rows: vec![row(None, true, true)], + overall: Overall::Approve, + }, + "r", + ) + .await; + assert_eq!(target, "o/r#7"); + assert_eq!(commit.as_deref(), Some("abc123")); + assert_eq!(review.event, "APPROVED"); + assert!( + review.body.ends_with("**Overall: approve.**\n"), + "{}", + review.body + ); + + let (_, _, review) = posted( + Reply::Done { + rows: vec![row(None, true, false)], + overall: Overall::RequestChanges, + }, + "r", + ) + .await; + assert_eq!(review.event, "REQUEST_CHANGES"); + assert!(review.body.contains("| yes ❌ |"), "{}", review.body); + } + + #[tokio::test] + async fn no_rules_a_failed_run_or_no_pr_is_a_comment() { + let approve = || Reply::Done { + rows: vec![row(None, true, true)], + overall: Overall::Approve, + }; + // `o/elsewhere` has no PR 7. + for (reply, repo, says, commit) in [ + ( + Reply::Failed { + error: "no API key".to_owned(), + }, + "r", + "the run failed: no API key", + Some("abc123"), + ), + (approve(), "elsewhere", "reading the PR failed", None), + ] { + let (_, posted_commit, review) = posted(reply, repo).await; + assert_eq!(review.event, "COMMENT", "{repo}"); + assert!(review.body.contains(says), "{repo}: {}", review.body); + assert_eq!(posted_commit.as_deref(), commit, "{repo}"); + } + + let fake = Fake::new(approve(), Vec::new()); + let poster = Posted::default(); + run(&fake, &fake, &fake, &poster, &request("r")).await; + let (_, _, review) = poster.0.into_inner().expect("unpoisoned").remove(0); + assert_eq!(review.event, "COMMENT"); + assert!( + review.body.contains("the rules did not load") && review.body.contains("neither"), + "{}", + review.body + ); + } + + #[test] + fn a_hostile_reason_mentions_nobody_and_stays_in_its_cell() { + let mut r = row(None, true, true); + r.reason = "ok | @mara please merge\n| x | ![](http://e/p.png) `".to_owned(); + let body = table(&[r]); + let line = body.lines().nth(2).expect("a data row"); + assert_eq!(body.lines().count(), 3, "{body}"); + assert!(!line.contains("@mara"), "{line}"); + assert!(line.contains("@\u{200b}mara"), "{line}"); + // Seven cells: eight unescaped pipes. + let unescaped = line + .char_indices() + .filter(|(i, c)| *c == '|' && (*i == 0 || line.as_bytes()[i - 1] != b'\\')) + .count(); + assert_eq!(unescaped, 8, "{line}"); + assert!( + line.contains("\\!\\[\\]") && line.contains("\\"), + "{line}" + ); + } + + #[test] + fn only_a_request_for_the_classifier_parses() { + let payload = |action: &str, reviewer: Option<&str>| { + serde_json::json!({ + "action": action, + "number": 7, + "requested_reviewer": reviewer.map(|l| serde_json::json!({ "login": l })), + "repository": { "full_name": "o/r" }, + "sender": { "login": "alice" }, + }) + .to_string() + }; + assert_eq!( + Requested::parse(payload("review_requested", Some(LOGIN)).as_bytes()), + Some(request("r")) + ); + assert_eq!( + Requested::parse(payload("review_requested", Some("Review-Classifier")).as_bytes()), + Some(request("r")), + "the forge's logins are case-insensitive" + ); + for body in [ + payload("review_request_removed", Some(LOGIN)), + payload("review_requested", Some("argus")), + payload("review_requested", None), + "{}".to_owned(), + ] { + assert_eq!(Requested::parse(body.as_bytes()), None, "{body}"); + } + } +} diff --git a/swarm-controller/src/webhook.rs b/swarm-controller/src/webhook.rs index 1803d22d..73aabfef 100644 --- a/swarm-controller/src/webhook.rs +++ b/swarm-controller/src/webhook.rs @@ -221,6 +221,10 @@ pub(super) enum DeliveryKind { /// `repo_create_hook`) — different scope, same event name. #[strum(serialize = "vcs-activity")] VcsActivity, + /// `pull_request_review_request` events on every repo in the instance, + /// for the review classifier's account (`crate::review_classifier::review`). + #[strum(serialize = "review-request")] + ReviewRequest, } /// The route prefix a registered `target_url` must point at. @@ -236,7 +240,12 @@ impl DeliveryKind { /// Every kind, for the registration sweep. An array rather than a /// hand-written list at the call site, so adding a kind cannot leave one /// hook unregistered. - pub(super) const ALL: [Self; 3] = [Self::Knowledge, Self::ConfigPr, Self::VcsActivity]; + pub(super) const ALL: [Self; 4] = [ + Self::Knowledge, + Self::ConfigPr, + Self::VcsActivity, + Self::ReviewRequest, + ]; /// The `target_url` to register with Forgejo for this kind, given the /// swarm's public base URL. @@ -341,7 +350,9 @@ pub(super) fn verify( /// [`announce_knowledge_change`]); a `ConfigPr` delivery updates /// `crate::config_pr::ConfigPrCache` immediately (see that module's doc /// comment for why both this and a periodic poll write the same cache), and a -/// merge queues a deploy of the merged commit on the agent's hive. +/// merge queues a deploy of the merged commit on the agent's hive. A +/// `ReviewRequest` delivery naming the review classifier's account starts a +/// review run in the background. /// /// Returns 200 on an accepted delivery so Forgejo does not retry. A refused /// one answers 401 (bad signature) or 503 (this daemon has no secret), and @@ -351,7 +362,8 @@ pub(super) fn verify( path = "/webhook/forge/{kind}", params( ("kind" = String, Path, description = "which swarm-wide hook this delivery arrived on: \ - `knowledge` or `config-pr`") + `knowledge`, `config-pr`, `vcs-activity` or \ + `review-request`") ), request_body( content = String, @@ -414,6 +426,7 @@ pub(super) async fn post_webhook_forge( tracing::warn!("webhook: vcs-activity delivery did not parse as a push payload"); } } + DeliveryKind::ReviewRequest => crate::review_classifier::review::on_delivery(&state, &body), } (StatusCode::OK, "ok").into_response() diff --git a/swarm-queue-client/src/review_classifier.rs b/swarm-queue-client/src/review_classifier.rs index ae6c7cb9..cb048ec7 100644 --- a/swarm-queue-client/src/review_classifier.rs +++ b/swarm-queue-client/src/review_classifier.rs @@ -1,5 +1,6 @@ -//! The review classifier's try-out: the subject swarm-controller sends a run -//! on, and what travels each way. +//! The review classifier's runs: the subject swarm-controller sends a run +//! on, and what travels each way. A try-out and a requested review are the +//! same request. //! //! Three crates name these: the controller sends the request, the classifier //! answers it, and the auth-callout responder grants the one subject to each. @@ -8,7 +9,7 @@ use std::time::Duration; use serde::{Deserialize, Serialize}; -/// The subject a try-out request goes to. The controller is its one +/// The subject every run goes to. The controller is its one /// publisher and the classifier its one subscriber; the classifier answers /// on the request's reply subject. pub const SUBJECT: &str = "$SWARM.review-classifier.try-out"; @@ -34,7 +35,8 @@ pub const RUN_DEADLINE: Duration = Duration::from_mins(31); /// answer about it. #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] pub struct TryOut { - /// The swarm UI user who started the run, as the gateway named them. + /// Who started the run: the swarm UI user, as the gateway named them, or + /// the forge user who requested the classifier's review. pub operator: String, /// `owner/name`. pub repo: String, diff --git a/swarm-secret-client/src/review_classifier.rs b/swarm-secret-client/src/review_classifier.rs index a7771c10..f9968116 100644 --- a/swarm-secret-client/src/review_classifier.rs +++ b/swarm-secret-client/src/review_classifier.rs @@ -1,9 +1,12 @@ -//! The review classifier agreement: where its inference key and its queue -//! client secret live in the store, and what each object holds. +//! The review classifier agreement: where its inference key, its queue +//! client secret and its forge token live in the store, and what each object +//! holds. //! //! The operator writes the key by hand in the store's UI; the classifier -//! reads both under its own certificate, the key once per run. Its read -//! stanza in `swarm-bao.nix` names exactly these two paths. +//! reads the key and the client secret under its own certificate, the key +//! once per run. Its read stanza in `swarm-bao.nix` names exactly these two +//! paths. The forge token is `swarm-controller`'s alone: it mints it, and +//! posts the classifier's reviews with it. use serde::{Deserialize, Serialize}; @@ -35,6 +38,17 @@ pub struct ApiKey { pub api_key: String, } +/// The path holding the token of the classifier's forge account, as a +/// [`crate::forge::Credential`]. +/// +/// # Errors +/// Never in practice: the name segment is a constant. The `Result` is +/// [`principal_prefix`]'s. +pub fn forge_token_path() -> Result { + let prefix = principal_prefix(Kind::Service, SERVICE)?; + Ok(format!("{prefix}/forge/token")) +} + /// The path holding the queue client secret of the classifier's OIDC client /// `client_id`, where `swarm-secret-publisher.nix` puts every swarm service's. /// @@ -61,11 +75,15 @@ mod tests { #[test] fn the_paths_are_the_ones_the_store_grants() { // Spelled out: the operator types the first into the store's UI, and - // `swarm-bao.nix` grants both literals. + // `swarm-bao.nix` grants each literal. assert_eq!( api_key_path().expect("a constant segment is legal"), "swarm/services/review-classifier/anthropic-api-key" ); + assert_eq!( + forge_token_path().expect("a constant segment is legal"), + "swarm/services/review-classifier/forge/token" + ); assert_eq!( client_secret_path("swarm-review-classifier").expect("a plain id is legal"), "swarm/services/swarm-review-classifier/oidc/client"