From 915364c918f9f9879f6477a909c8fbd889089e1f Mon Sep 17 00:00:00 2001 From: atlas Date: Wed, 14 Oct 2026 16:13:57 +0200 Subject: [PATCH] review try-out: test the branch push request; name the open-PR gating push_classifier_branch's request moves into push_branch, which takes the forge client, so a test can run it against a stand-in forge. The tests pin the POST to /repos/o/r/contents and its body: base and new branch, no force overwrite, the message, the operator as author with the service account as committer, the base64 content, and update with the blob sha or create without one. The page's open-a-PR gating becomes rowsProblem in TryOutTypes.ts, beside the other form checks. swarm-ui has no test runner, so the typecheck is the only check on it. Refs #5259 --- .../pages/review-tryout/ReviewTryOutPage.tsx | 14 +- .../src/pages/review-tryout/TryOutTypes.ts | 14 ++ .../src/forge/classifier_account.rs | 160 +++++++++++++----- 3 files changed, 140 insertions(+), 48 deletions(-) diff --git a/frontend/packages/swarm-ui/src/pages/review-tryout/ReviewTryOutPage.tsx b/frontend/packages/swarm-ui/src/pages/review-tryout/ReviewTryOutPage.tsx index 07bd804b..25c3a763 100644 --- a/frontend/packages/swarm-ui/src/pages/review-tryout/ReviewTryOutPage.tsx +++ b/frontend/packages/swarm-ui/src/pages/review-tryout/ReviewTryOutPage.tsx @@ -37,11 +37,10 @@ import { ClassificationEditor } from "./ClassificationEditor.js"; import { TryOutResult } from "./TryOutResult.js"; import { draftFromRule, - draftProblem, - duplicateName, hasTypedText, prNumber, repoProblem, + rowsProblem, toBody, type ClassificationDraft, type OpenPrAnswer, @@ -218,15 +217,10 @@ export function ReviewTryOutPage() { }; }, [runId]); - // What keeps the rows from going to the repo; a PR number is not needed. - const rowsProblem = - repoProblem(repo) ?? - drafts.map(draftProblem).find((p) => p !== null) ?? - duplicateName(drafts) ?? - null; + const openPrProblem = rowsProblem(repo, drafts); const problem = (prNumber(pr) === null ? "a PR number is a whole number above 0" : null) ?? - rowsProblem; + openPrProblem; async function run(e: Event) { e.preventDefault(); @@ -410,7 +404,7 @@ export function ReviewTryOutPage() { diff --git a/frontend/packages/swarm-ui/src/pages/review-tryout/TryOutTypes.ts b/frontend/packages/swarm-ui/src/pages/review-tryout/TryOutTypes.ts index c8233a85..0195ac1f 100644 --- a/frontend/packages/swarm-ui/src/pages/review-tryout/TryOutTypes.ts +++ b/frontend/packages/swarm-ui/src/pages/review-tryout/TryOutTypes.ts @@ -150,6 +150,20 @@ export function duplicateName(drafts: ClassificationDraft[]): string | null { return null; } +/** What keeps the rows from going to the repo as a PR, or null: the repo, then + * each row, then a shared name. Unlike a run, it needs no PR number. */ +export function rowsProblem( + repo: string, + drafts: ClassificationDraft[], +): string | null { + return ( + repoProblem(repo) ?? + drafts.map(draftProblem).find((p) => p !== null) ?? + duplicateName(drafts) ?? + null + ); +} + export function toBody(d: ClassificationDraft): ClassificationBody { return { name: d.name.trim(), diff --git a/swarm-controller/src/forge/classifier_account.rs b/swarm-controller/src/forge/classifier_account.rs index df4b03e8..aff3ec3d 100644 --- a/swarm-controller/src/forge/classifier_account.rs +++ b/swarm-controller/src/forge/classifier_account.rs @@ -181,44 +181,7 @@ impl Client { 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(()) + push_branch(&self.as_classifier().await?, owner, repo, commit).await } /// The forge account `login` as a commit author: its full name, or its @@ -338,6 +301,47 @@ impl Client { } } +/// Make `commit` with `forge`'s identity as committer, through the contents +/// API. +async fn push_branch(forge: &Forgejo, owner: &str, repo: &str, commit: BranchCommit) -> Result<()> { + use base64::Engine as _; + + let branch = commit.branch.clone(); + forge + .repo_change_files( + owner, + repo, + ChangeFilesOptions { + author: Some(Identity { + name: Some(commit.author.name), + email: Some(commit.author.email), + }), + branch: Some(commit.base), + committer: None, + dates: None, + files: vec![ChangeFileOperation { + content: Some( + base64::engine::general_purpose::STANDARD.encode(&commit.content), + ), + from_path: None, + operation: if commit.sha.is_some() { + ChangeFileOperationOperation::Update + } else { + ChangeFileOperationOperation::Create + }, + path: commit.path, + sha: commit.sha, + }], + force_overwrite_new_branch: Some(false), + message: Some(commit.message), + new_branch: Some(commit.branch), + signoff: None, + }, + ) + .await + .with_context(|| format!("pushing {LOGIN}'s branch {branch} to {owner}/{repo}"))?; + Ok(()) +} /// Check the account now and every [`RECONCILE_INTERVAL`] after, and call /// `enqueue`, which inserts a `MintClassifierForgeToken` node, when it needs /// a job. A pass that fails is logged and retried on the next tick. @@ -419,6 +423,86 @@ mod tests { assert!(lockdown_patch(&seen, LOGIN).is_none()); } + /// The one request `push_branch` sends for `sha`, as a stand-in forge + /// saw it: method, path and JSON body. + async fn pushed(sha: Option<&str>) -> (String, String, serde_json::Value) { + use axum::http::{Method, StatusCode, Uri, header::CONTENT_TYPE}; + + let seen = Arc::new(std::sync::Mutex::new(Vec::new())); + let recorded = Arc::clone(&seen); + let app = axum::Router::new().fallback( + move |method: Method, uri: Uri, body: axum::body::Bytes| { + let recorded = Arc::clone(&recorded); + async move { + recorded.lock().unwrap().push(( + method.to_string(), + uri.path().to_owned(), + serde_json::from_slice(&body).unwrap_or_default(), + )); + ( + StatusCode::CREATED, + [(CONTENT_TYPE, "application/json")], + r#"{"commit":{"sha":"c1"}}"#, + ) + } + }, + ); + let listener = tokio::net::TcpListener::bind("127.0.0.1:0").await.unwrap(); + let url = url::Url::parse(&format!("http://{}/", listener.local_addr().unwrap())).unwrap(); + tokio::spawn(async move { axum::serve(listener, app).await }); + let forge = Forgejo::new(Auth::Token("stub-token"), url).unwrap(); + let commit = BranchCommit { + base: "main".to_owned(), + branch: "review-classifier/alice-0a1b2c3d".to_owned(), + path: ".hyperhive/review-classifier.yml".to_owned(), + sha: sha.map(str::to_owned), + content: b"classifications: []\n".to_vec(), + message: "review classifier: add no-ads".to_owned(), + author: CommitAuthor { + name: "Alice".to_owned(), + email: "alice@example.test".to_owned(), + }, + }; + push_branch(&forge, "o", "r", commit).await.expect("pushed"); + let mut seen = seen.lock().unwrap(); + assert_eq!(seen.len(), 1, "{seen:?}"); + seen.remove(0) + } + + #[tokio::test] + async fn a_push_is_one_commit_on_a_new_branch_authored_by_the_operator() { + let (method, path, body) = pushed(Some("b1")).await; + assert_eq!(method, "POST"); + assert_eq!(path, "/api/v1/repos/o/r/contents"); + assert_eq!(body["branch"], "main"); + assert_eq!(body["new_branch"], "review-classifier/alice-0a1b2c3d"); + assert_eq!(body["force_overwrite_new_branch"], false); + assert_eq!(body["message"], "review classifier: add no-ads"); + assert_eq!( + body["author"], + serde_json::json!({ "name": "Alice", "email": "alice@example.test" }) + ); + assert_eq!(body["committer"], serde_json::Value::Null); + assert_eq!( + body["files"], + serde_json::json!([{ + "operation": "update", + "path": ".hyperhive/review-classifier.yml", + "sha": "b1", + // base64 of "classifications: []\n" + "content": "Y2xhc3NpZmljYXRpb25zOiBbXQo=", + "from_path": null, + }]) + ); + } + + #[tokio::test] + async fn a_push_without_a_blob_sha_creates_the_file() { + let (_, _, body) = pushed(None).await; + assert_eq!(body["files"][0]["operation"], "create"); + assert_eq!(body["files"][0]["sha"], serde_json::Value::Null); + } + #[tokio::test] async fn a_commit_author_is_the_accounts_name_and_email_or_none() { use super::super::tests::stub_forge;