From d52e97f3769f324bafd8373ead99aa49a5fd892e Mon Sep 17 00:00:00 2001 From: atlas Date: Mon, 12 Oct 2026 08:47:07 +0200 Subject: [PATCH] swarm-controller: read review rules through the git tree and blob API The rules file is now read with the existing forgejo-api client: the commit's root tree, then the `.hyperhive` tree, then the `review-classifier.{yml,json}` blob, base64-decoded. Every request takes single-segment path parameters, so nothing depends on how a nested path is escaped, and the controller keeps one HTTP transport to the forge. The second reqwest client, `Client::with_token` and `raw_file_url` are gone. A tree the forge truncated is an error, not a missing file. A 404 on a tree or blob doesn't come back as a forge 404, so only the repo lookup's 404 reads as "no such repo". The `is_repo_not_found` doc comment is back on its function. Part of #5096. --- swarm-controller/Cargo.toml | 6 +- swarm-controller/src/forge.rs | 333 +++++++++++--------- swarm-controller/src/forge/docs_grant.rs | 4 +- swarm-controller/src/forge/legacy_tokens.rs | 5 +- swarm-controller/src/review_classifier.rs | 1 + 5 files changed, 202 insertions(+), 147 deletions(-) diff --git a/swarm-controller/Cargo.toml b/swarm-controller/Cargo.toml index ed93a41f..82d53077 100644 --- a/swarm-controller/Cargo.toml +++ b/swarm-controller/Cargo.toml @@ -22,9 +22,9 @@ axum.workspace = true # boundary (see #3306's design discussion: forcing that split now, over a # few idempotent CRUD-ish calls, is premature plumbing). forgejo-api.workspace = true -# Only for base64-encoding file content for `forge.rs`'s -# `repo_change_files` calls — forgejo's content API takes base64, never -# raw bytes. +# `forge.rs` encodes file content for its `repo_change_files` calls and +# decodes blobs it reads: forgejo's content and blob APIs carry base64, +# never raw bytes. base64.workspace = true # `agent_identity.rs` mints the per-agent queue secret, the one value in this # tree this daemon invents rather than receives. Straight from the kernel's diff --git a/swarm-controller/src/forge.rs b/swarm-controller/src/forge.rs index 7a6805ab..eefe0934 100644 --- a/swarm-controller/src/forge.rs +++ b/swarm-controller/src/forge.rs @@ -23,10 +23,10 @@ use forgejo_api::structs::{ AddCollaboratorOption, AddCollaboratorOptionPermission, ChangeFileOperation, ChangeFileOperationOperation, ChangeFilesOptions, CreateBranchProtectionOption, CreateHookOption, CreateHookOptionConfig, CreateHookOptionType, CreateRepoOption, - CreateUserOption, EditUserOption, IssueListIssuesQuery, IssueListIssuesQueryState, - IssueListIssuesQueryType, RepoDownloadPullDiffOrPatchQuery, RepoGetContentsQuery, - RepoGetPullRequestCommitsQuery, RepoListPullRequestsQuery, RepoListPullRequestsQueryState, - RepoSearchQuery, StateType, + CreateUserOption, EditUserOption, GetTreeQuery, GitEntry, IssueListIssuesQuery, + IssueListIssuesQueryState, IssueListIssuesQueryType, RepoDownloadPullDiffOrPatchQuery, + RepoGetContentsQuery, RepoGetPullRequestCommitsQuery, RepoListPullRequestsQuery, + RepoListPullRequestsQueryState, RepoSearchQuery, StateType, }; use forgejo_api::{ApiErrorKind, Auth, Forgejo, ForgejoError}; use futures_util::{StreamExt as _, TryStreamExt as _}; @@ -184,10 +184,6 @@ async fn fetch_token() -> Result { /// Typed forgejo client, built once at startup from env. pub struct Client { api: Forgejo, - /// The same token as `api`, for the one request `api` can't shape: - /// forgejo-api escapes every `/` in a file path, so [`raw_file_url`] - /// builds that path itself. - http: reqwest::Client, /// The forge's base URL, kept so a client authenticated as someone /// else can be built against the same forge (see /// [`agent_token`]'s read-back). @@ -327,28 +323,11 @@ impl Client { /// host has not published it yet, or the client failing to build. async fn connect(url: &url::Url) -> Result { let token = fetch_token().await?; - Self::with_token(&token, url.clone()) - } - - /// A client for `url` holding `token`. - /// - /// # Errors - /// A token that is not a valid header value, or a client failing to - /// build. - fn with_token(token: &str, url: url::Url) -> Result { - let api = Forgejo::new(Auth::Token(token), url.clone()).context("build forgejo client")?; - let mut auth = reqwest::header::HeaderValue::try_from(format!("token {token}")) - .context("the forge token is not a valid header value")?; - auth.set_sensitive(true); - let http = reqwest::Client::builder() - .default_headers( - [(reqwest::header::AUTHORIZATION, auth)] - .into_iter() - .collect(), - ) - .build() - .context("build forge HTTP client")?; - Ok(Self { api, http, url }) + let api = Forgejo::new(Auth::Token(&token), url.clone()).context("build forgejo client")?; + Ok(Self { + api, + url: url.clone(), + }) } /// Creation options for an empty repo defaulting to `main`. @@ -1139,9 +1118,14 @@ impl Client { /// read at the branch's head commit. `None` when the default branch has /// no commit: an empty repo, or a default branch that doesn't exist. /// + /// The files come through the git tree and blob API, one path segment + /// per request: forgejo-api escapes the `/` of a nested path in its + /// file endpoints. + /// /// # Errors - /// A forge failure; a [`ForgejoError`] 404 underneath comes only from the - /// first call, and means the repo does not exist ([`is_repo_not_found`]). + /// A forge failure, or a tree the forge truncated. A [`ForgejoError`] 404 + /// underneath comes only from the first call, and means the repo does + /// not exist ([`is_repo_not_found`]). pub async fn default_branch_files( &self, owner: &str, @@ -1170,9 +1154,10 @@ impl Client { .commit .and_then(|c| c.id) .with_context(|| format!("branch {branch} of {owner}/{repo} names no commit"))?; + let mut trees = HashMap::new(); let mut files = Vec::new(); for path in paths { - if let Some(bytes) = self.raw_file(owner, repo, path, &commit).await? { + if let Some(bytes) = self.file_at(owner, repo, &commit, path, &mut trees).await? { files.push(((*path).to_owned(), bytes)); } } @@ -1183,26 +1168,88 @@ impl Client { })) } - /// The file at `path` in `owner/repo` at `rev`, or `None` on a 404. - async fn raw_file( + /// The file at `path` in `owner/repo` at `commit`, or `None` when no + /// blob is there. `trees` holds the tree listings already read, by sha. + async fn file_at( &self, owner: &str, repo: &str, + commit: &str, path: &str, - rev: &str, + trees: &mut HashMap>, ) -> Result>> { - let what = || format!("read {path} of {owner}/{repo} at {rev}"); - let response = self - .http - .get(raw_file_url(&self.url, owner, repo, path, rev)?) - .send() - .await - .with_context(what)?; - match response.status() { - StatusCode::NOT_FOUND => Ok(None), - s if s.is_success() => Ok(Some(response.bytes().await.with_context(what)?.to_vec())), - s => Err(anyhow::anyhow!("the forge answered {s}")).with_context(what), + let mut tree = commit.to_owned(); + let mut segments = path.split('/').peekable(); + while let Some(name) = segments.next() { + let kind = if segments.peek().is_some() { + "tree" + } else { + "blob" + }; + let entries = self.tree(owner, repo, &tree, trees).await?; + let Some(sha) = entries + .iter() + .find(|e| e.path.as_deref() == Some(name) && e.r#type.as_deref() == Some(kind)) + .and_then(|e| e.sha.clone()) + else { + return Ok(None); + }; + if kind == "blob" { + return self.blob(owner, repo, &sha).await.map(Some); + } + tree = sha; } + Ok(None) + } + + /// The entries of tree `sha` (or of a commit's root tree), read once + /// into `trees`. + async fn tree<'t>( + &self, + owner: &str, + repo: &str, + sha: &str, + trees: &'t mut HashMap>, + ) -> Result<&'t [GitEntry]> { + if !trees.contains_key(sha) { + let listing = self + .api + .get_tree(owner, repo, sha, GetTreeQuery::default()) + .await + .map_err(|e| anyhow::anyhow!("read tree {sha} of {owner}/{repo}: {e}"))?; + if listing.truncated.unwrap_or(false) { + anyhow::bail!( + "the forge truncated tree {sha} of {owner}/{repo}, so a missing entry can't \ + be told from one past the cut" + ); + } + trees.insert(sha.to_owned(), listing.tree.unwrap_or_default()); + } + Ok(&trees[sha]) + } + + /// The bytes of blob `sha`. + async fn blob(&self, owner: &str, repo: &str, sha: &str) -> Result> { + use base64::Engine as _; + + let blob = self + .api + .get_blob(owner, repo, sha) + .await + .map_err(|e| anyhow::anyhow!("read blob {sha} of {owner}/{repo}: {e}"))?; + anyhow::ensure!( + blob.encoding.as_deref() == Some("base64"), + "blob {sha} of {owner}/{repo} is not base64 but {:?}", + blob.encoding + ); + let content: String = blob + .content + .unwrap_or_default() + .split_ascii_whitespace() + .collect(); + base64::engine::general_purpose::STANDARD + .decode(content) + .with_context(|| format!("decode blob {sha} of {owner}/{repo}")) } /// Register the swarm-wide hooks against this controller, so a real @@ -1564,31 +1611,6 @@ fn folds_into_success(e: &ForgejoError, existing: bool) -> bool { /// Used by [`crate::issue_report::get_issue_report`] to tell "no such repo" /// (404, retrying won't help) apart from every other forge failure (503, /// might clear up) — see that module for why the distinction matters. -/// The API URL of the file at `path` in `owner/repo` at `rev`, on the forge -/// at `base`. Each segment of `path` is escaped on its own and the segments -/// are joined with a literal `/`, so no proxy or router in front of the forge -/// has to decode a `%2F`. Like forgejo-api's own requests, it ignores any -/// path `base` has. -/// -/// # Errors -/// A `base` that can't carry a path, such as a `data:` URL. -fn raw_file_url( - base: &url::Url, - owner: &str, - repo: &str, - path: &str, - rev: &str, -) -> Result { - let mut url = base.clone(); - url.path_segments_mut() - .map_err(|()| anyhow::anyhow!("forge URL {base} can't carry a path"))? - .clear() - .extend(["api", "v1", "repos", owner, repo, "raw"]) - .extend(path.split('/')); - url.query_pairs_mut().clear().append_pair("ref", rev); - Ok(url) -} - pub(crate) fn is_repo_not_found(e: &ForgejoError) -> bool { match e { ForgejoError::ApiError(api) => match api.error_kind() { @@ -1615,7 +1637,8 @@ fn conflict_error() -> ForgejoError { impl Client { /// A client for a test's own forge at `url`, holding a placeholder token. pub(crate) fn for_test_forge(url: url::Url) -> Self { - Self::with_token("stub-token", url).unwrap() + let api = Forgejo::new(Auth::Token("stub-token"), url.clone()).unwrap(); + Self { api, url } } } @@ -1862,7 +1885,8 @@ mod tests { 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 }); - (Client::with_token("stub-token", url).unwrap(), seen) + let api = Forgejo::new(Auth::Token("stub-token"), url.clone()).unwrap(); + (Client { api, url }, seen) } /// The `PATCH` [`Client::ensure_agent_user`] sent, if any. @@ -2097,51 +2121,50 @@ mod tests { assert_eq!(methods, ["GET"]); } - #[test] - fn a_raw_file_url_keeps_the_slash_between_path_segments() { - let base = url::Url::parse("https://forge.example/").unwrap(); - let url = raw_file_url( - &base, - "o", - "r", - ".hyperhive/review-classifier.yml", - "def456", - ) - .unwrap(); - assert_eq!( - url.as_str(), - "https://forge.example/api/v1/repos/o/r/raw/.hyperhive/review-classifier.yml?ref=def456" - ); - // A segment's own reserved characters are still escaped. - let odd = raw_file_url(&base, "o", "r", "a b/c?d#e", "x").unwrap(); - assert_eq!(odd.path(), "/api/v1/repos/o/r/raw/a%20b/c%3Fd%23e"); - } + const RULES_YML: &str = ".hyperhive/review-classifier.yml"; + const RULES_JSON: &str = ".hyperhive/review-classifier.json"; /// A stand-in forge holding `o/r` with default branch `main` at - /// `def456`, where only `.hyperhive/review-classifier.yml` exists, under - /// that literal path. `branch` is the status the branch lookup answers. - /// Records every request's path and query. - async fn rules_forge(branch: u16) -> (Client, Arc>>) { + /// `def456`. The branch lookup answers `branch`, and the commit's root + /// tree answers `root` (a 404 for `None`). The `.hyperhive` tree holds + /// only `review-classifier.yml`. Records every request's path. + async fn rules_forge( + branch: u16, + root: Option<&'static str>, + ) -> (Client, Arc>>) { use axum::http::{StatusCode, Uri}; + use base64::Engine as _; + let blob = format!( + r#"{{"sha":"b1","encoding":"base64","content":"{}"}}"#, + base64::engine::general_purpose::STANDARD.encode("classifications: []\n") + ); let seen = Arc::new(std::sync::Mutex::new(Vec::new())); let recorded = Arc::clone(&seen); let app = axum::Router::new().fallback(move |uri: Uri| { - let recorded = Arc::clone(&recorded); + let (recorded, blob) = (Arc::clone(&recorded), blob.clone()); async move { - recorded.lock().unwrap().push(uri.to_string()); + recorded.lock().unwrap().push(uri.path().to_owned()); let (status, body) = match uri.path() { - "/api/v1/repos/o/r" => (200, r#"{"default_branch":"main","empty":false}"#), - "/api/v1/repos/o/r/branches/main" if branch == 200 => { - (200, r#"{"name":"main","commit":{"id":"def456"}}"#) + "/api/v1/repos/o/r" => { + (200, r#"{"default_branch":"main","empty":false}"#.to_owned()) } - "/api/v1/repos/o/r/branches/main" => (branch, r#"{"message":"no"}"#), - "/api/v1/repos/o/r/raw/.hyperhive/review-classifier.yml" - if uri.query() == Some("ref=def456") => - { - (200, "classifications: []\n") + "/api/v1/repos/o/r/branches/main" if branch == 200 => ( + 200, + r#"{"name":"main","commit":{"id":"def456"}}"#.to_owned(), + ), + "/api/v1/repos/o/r/branches/main" => (branch, r#"{"message":"no"}"#.to_owned()), + "/api/v1/repos/o/r/git/trees/def456" if root.is_some() => { + (200, root.unwrap_or_default().to_owned()) } - _ => (404, r#"{"message":"not found"}"#), + "/api/v1/repos/o/r/git/trees/t1" => ( + 200, + r#"{"sha":"t1","truncated":false,"tree":[ + {"path":"review-classifier.yml","type":"blob","sha":"b1"}]}"# + .to_owned(), + ), + "/api/v1/repos/o/r/git/blobs/b1" => (200, blob), + _ => (404, r#"{"message":"not found"}"#.to_owned()), }; (StatusCode::from_u16(status).unwrap(), body) } @@ -2149,21 +2172,20 @@ mod tests { 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 }); - (Client::with_token("stub-token", url).unwrap(), seen) + let api = Forgejo::new(Auth::Token("stub-token"), url.clone()).unwrap(); + (Client { api, url }, seen) } + /// A root tree with the `.hyperhive` tree and a blob beside it. + const ROOT: &str = r#"{"sha":"def456","truncated":false,"tree":[ + {"path":"README.md","type":"blob","sha":"b0"}, + {"path":".hyperhive","type":"tree","sha":"t1"}]}"#; + #[tokio::test] - async fn default_branch_files_reads_each_path_at_the_branch_head() { - let (client, seen) = rules_forge(200).await; + async fn default_branch_files_reads_each_path_through_the_trees_at_the_branch_head() { + let (client, seen) = rules_forge(200, Some(ROOT)).await; let read = client - .default_branch_files( - "o", - "r", - &[ - ".hyperhive/review-classifier.yml", - ".hyperhive/review-classifier.json", - ], - ) + .default_branch_files("o", "r", &[RULES_YML, RULES_JSON]) .await .unwrap() .expect("the branch has a commit"); @@ -2171,47 +2193,74 @@ mod tests { assert_eq!(read.commit, "def456"); assert_eq!( read.files, - [( - ".hyperhive/review-classifier.yml".to_owned(), - b"classifications: []\n".to_vec() - )] - ); - assert!( - seen.lock().unwrap().iter().all(|p| !p.contains("%2F")), - "{seen:?}" + [(RULES_YML.to_owned(), b"classifications: []\n".to_vec())] ); + // Each tree is read once for both paths. + let seen = seen.lock().unwrap(); + let reads = |p: &str| seen.iter().filter(|s| *s == p).count(); + assert_eq!(reads("/api/v1/repos/o/r/git/trees/def456"), 1, "{seen:?}"); + assert_eq!(reads("/api/v1/repos/o/r/git/trees/t1"), 1, "{seen:?}"); + } + + #[tokio::test] + async fn a_repo_without_the_directory_has_no_files() { + let root = r#"{"sha":"def456","truncated":false,"tree":[ + {"path":"README.md","type":"blob","sha":"b0"}, + {"path":".hyperhive","type":"blob","sha":"b2"}]}"#; + let (client, _) = rules_forge(200, Some(root)).await; + let read = client + .default_branch_files("o", "r", &[RULES_YML]) + .await + .unwrap() + .expect("the branch has a commit"); + assert_eq!(read.files, []); + } + + #[tokio::test] + async fn a_truncated_tree_is_an_error_not_a_missing_file() { + let root = r#"{"sha":"def456","truncated":true,"tree":[]}"#; + let (client, _) = rules_forge(200, Some(root)).await; + let err = client + .default_branch_files("o", "r", &[RULES_YML]) + .await + .unwrap_err(); + assert!(format!("{err:#}").contains("truncated"), "{err:#}"); } #[tokio::test] async fn a_missing_default_branch_is_no_commit_not_no_repo() { - let (client, _) = rules_forge(404).await; + let (client, _) = rules_forge(404, Some(ROOT)).await; let read = client - .default_branch_files("o", "r", &[".hyperhive/review-classifier.yml"]) + .default_branch_files("o", "r", &[RULES_YML]) .await .unwrap(); assert_eq!(read, None); } + /// Only the repo lookup's 404 reads as "no such repo"; a failed branch + /// read or a tree the forge doesn't have is some other failure. #[tokio::test] - async fn an_unknown_repo_is_a_forge_404_and_a_failed_branch_read_is_not() { - let (client, _) = rules_forge(500).await; + async fn only_an_unknown_repo_is_a_forge_404() { + let no_repo = |e: &anyhow::Error| { + e.downcast_ref::() + .is_some_and(is_repo_not_found) + }; + let (client, _) = rules_forge(500, Some(ROOT)).await; let unknown = client .default_branch_files("o", "elsewhere", &[]) .await .unwrap_err(); - let fe = unknown - .downcast_ref::() - .expect("a forge error"); - assert!(is_repo_not_found(fe), "{unknown:#}"); - let failed = client + assert!(no_repo(&unknown), "{unknown:#}"); + let branch = client .default_branch_files("o", "r", &[]) .await .unwrap_err(); - assert!( - !failed - .downcast_ref::() - .is_some_and(is_repo_not_found), - "{failed:#}" - ); + assert!(!no_repo(&branch), "{branch:#}"); + let (client, _) = rules_forge(200, None).await; + let tree = client + .default_branch_files("o", "r", &[RULES_YML]) + .await + .unwrap_err(); + assert!(!no_repo(&tree), "{tree:#}"); } } diff --git a/swarm-controller/src/forge/docs_grant.rs b/swarm-controller/src/forge/docs_grant.rs index 57ecc233..435b5fea 100644 --- a/swarm-controller/src/forge/docs_grant.rs +++ b/swarm-controller/src/forge/docs_grant.rs @@ -206,6 +206,7 @@ mod tests { use axum::extract::{Path, State}; use axum::http::{StatusCode, header::CONTENT_TYPE}; + use forgejo_api::{Auth, Forgejo}; use super::*; @@ -254,7 +255,8 @@ mod tests { 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 }); - (Client::with_token("stub-token", url).unwrap(), docs) + let api = Forgejo::new(Auth::Token("stub-token"), url.clone()).unwrap(); + (Client { api, url }, docs) } async fn check( diff --git a/swarm-controller/src/forge/legacy_tokens.rs b/swarm-controller/src/forge/legacy_tokens.rs index ead41410..917ab5e3 100644 --- a/swarm-controller/src/forge/legacy_tokens.rs +++ b/swarm-controller/src/forge/legacy_tokens.rs @@ -113,6 +113,8 @@ mod tests { use std::collections::BTreeMap; use std::sync::{Arc, Mutex}; + use forgejo_api::{Auth, Forgejo}; + use super::super::agent_token::{AGENT_TOKEN_NAME, MintReason}; use super::*; @@ -227,7 +229,8 @@ mod tests { 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 }); - (Client::with_token("stub-token", url).unwrap(), deletes) + let api = Forgejo::new(Auth::Token("stub-token"), url.clone()).unwrap(); + (Client { api, url }, deletes) } const MIX: [&str; 5] = [ diff --git a/swarm-controller/src/review_classifier.rs b/swarm-controller/src/review_classifier.rs index 5a5e33c2..ef01dd56 100644 --- a/swarm-controller/src/review_classifier.rs +++ b/swarm-controller/src/review_classifier.rs @@ -1232,6 +1232,7 @@ classifications: assert!(detail.contains(says), "{query}: {detail}"); } } + #[tokio::test] async fn rules_without_forge_access_on_this_host_are_503() { let (state, _) = crate::tests::state_with_roster();