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

swarm-controller: read review rules at a literal nested path

Browse source 
forgejo-api escapes the whole file path of a raw-file request, so the
rules file went out as `raw/.hyperhive%2Freview-classifier.yml` and
reached the forge only if every proxy and router in between decoded the
`%2F`. The controller now sends that one request itself: `raw_file_url`
escapes each path segment on its own and joins them with `/`, and
`forge::Client` holds a second HTTP client with the same token for it.

A 404 from the default-branch lookup now means "no commit on the
default branch", like an empty repo, instead of falling through to the
route's "no repo" answer. Only the repo lookup's 404 still means the
repo doesn't exist.

Tests cover the URL shape, the reads against a loopback stub forge
that serves only the literal path, the branch 404, 503 with no forge
access, 503 at the forge deadline on a paused clock, and a bad
polarity value.

Part of #5096.
This commit is contained in:
atlas 2026-10-12 08:32:00 +02:00 • committed by mara
commit 2b1573c2ba
5 changed files with 259 additions and 44 deletions
Download patch file Download diff file Expand all files Collapse all files

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

@ -25,8 +25,8 @@ use forgejo_api::structs::{
CreateHookOption, CreateHookOptionConfig, CreateHookOptionType, CreateRepoOption,
CreateUserOption, EditUserOption, IssueListIssuesQuery, IssueListIssuesQueryState,
IssueListIssuesQueryType, RepoDownloadPullDiffOrPatchQuery, RepoGetContentsQuery,
RepoGetPullRequestCommitsQuery, RepoGetRawFileQuery, RepoListPullRequestsQuery,
RepoListPullRequestsQueryState, RepoSearchQuery, StateType,
RepoGetPullRequestCommitsQuery, RepoListPullRequestsQuery, RepoListPullRequestsQueryState,
RepoSearchQuery, StateType,
};
use forgejo_api::{ApiErrorKind, Auth, Forgejo, ForgejoError};
use futures_util::{StreamExt as _, TryStreamExt as _};
@ -184,6 +184,10 @@ async fn fetch_token() -> Result<String> {
/// 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).
@ -323,11 +327,28 @@ impl Client {
/// host has not published it yet, or the client failing to build.
async fn connect(url: &url::Url) -> Result<Self> {
let token = fetch_token().await?;
let api = Forgejo::new(Auth::Token(&token), url.clone()).context("build forgejo client")?;
Ok(Self {
api,
url: url.clone(),
})
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<Self> {
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 })
}
/// Creation options for an empty repo defaulting to `main`.
@ -1115,11 +1136,12 @@ impl Client {
}
/// The `paths` of `owner/repo` that exist on its default branch, all
/// read at the branch's head commit. `None` for a repo with no commit.
/// 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.
///
/// # Errors
/// A forge failure, with the [`ForgejoError`] underneath; a 404 on the
/// first call means the repo does not exist ([`is_repo_not_found`]).
/// A forge failure; 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,
@ -1137,27 +1159,21 @@ impl Client {
let branch = info
.default_branch
.with_context(|| format!("{owner}/{repo} names no default branch"))?;
let commit = self
.api
.repo_get_branch(owner, repo, &branch)
.await
.with_context(|| format!("read branch {branch} of {owner}/{repo}"))?
let head = match self.api.repo_get_branch(owner, repo, &branch).await {
Ok(head) => head,
Err(e) if is_repo_not_found(&e) => return Ok(None),
Err(e) => {
return Err(e).with_context(|| format!("read branch {branch} of {owner}/{repo}"));
}
};
let commit = head
.commit
.and_then(|c| c.id)
.with_context(|| format!("branch {branch} of {owner}/{repo} names no commit"))?;
let mut files = Vec::new();
for path in paths {
let query = RepoGetRawFileQuery {
r#ref: Some(commit.clone()),
};
match self.api.repo_get_raw_file(owner, repo, path, query).await {
Ok(bytes) => files.push(((*path).to_owned(), bytes.to_vec())),
Err(e) if is_repo_not_found(&e) => {}
Err(e) => {
return Err(e).with_context(|| {
format!("read {path} of {owner}/{repo} at {branch} ({commit})")
});
}
if let Some(bytes) = self.raw_file(owner, repo, path, &commit).await? {
files.push(((*path).to_owned(), bytes));
}
}
Ok(Some(DefaultBranchFiles {
@ -1167,6 +1183,28 @@ impl Client {
}))
}
/// The file at `path` in `owner/repo` at `rev`, or `None` on a 404.
async fn raw_file(
&self,
owner: &str,
repo: &str,
path: &str,
rev: &str,
) -> Result<Option<Vec<u8>>> {
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),
}
}
/// Register the swarm-wide hooks against this controller, so a real
/// forge event reaches [`crate::webhook`] instead of the endpoint only
/// being reachable by hand.
@ -1526,6 +1564,31 @@ 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<url::Url> {
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() {
@ -1552,8 +1615,7 @@ 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 {
let api = Forgejo::new(Auth::Token("stub-token"), url.clone()).unwrap();
Self { api, url }
Self::with_token("stub-token", url).unwrap()
}
}
@ -1800,8 +1862,7 @@ 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 });
let api = Forgejo::new(Auth::Token("stub-token"), url.clone()).unwrap();
(Client { api, url }, seen)
(Client::with_token("stub-token", url).unwrap(), seen)
}
/// The `PATCH` [`Client::ensure_agent_user`] sent, if any.
@ -2035,4 +2096,122 @@ mod tests {
.collect();
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");
}
/// 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<std::sync::Mutex<Vec<String>>>) {
use axum::http::{StatusCode, Uri};
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);
async move {
recorded.lock().unwrap().push(uri.to_string());
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/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")
}
_ => (404, r#"{"message":"not found"}"#),
};
(StatusCode::from_u16(status).unwrap(), body)
}
});
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)
}
#[tokio::test]
async fn default_branch_files_reads_each_path_at_the_branch_head() {
let (client, seen) = rules_forge(200).await;
let read = client
.default_branch_files(
"o",
"r",
&[
".hyperhive/review-classifier.yml",
".hyperhive/review-classifier.json",
],
)
.await
.unwrap()
.expect("the branch has a commit");
assert_eq!(read.branch, "main");
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:?}"
);
}
#[tokio::test]
async fn a_missing_default_branch_is_no_commit_not_no_repo() {
let (client, _) = rules_forge(404).await;
let read = client
.default_branch_files("o", "r", &[".hyperhive/review-classifier.yml"])
.await
.unwrap();
assert_eq!(read, None);
}
#[tokio::test]
async fn an_unknown_repo_is_a_forge_404_and_a_failed_branch_read_is_not() {
let (client, _) = rules_forge(500).await;
let unknown = client
.default_branch_files("o", "elsewhere", &[])
.await
.unwrap_err();
let fe = unknown
.downcast_ref::<ForgejoError>()
.expect("a forge error");
assert!(is_repo_not_found(fe), "{unknown:#}");
let failed = client
.default_branch_files("o", "r", &[])
.await
.unwrap_err();
assert!(
!failed
.downcast_ref::<ForgejoError>()
.is_some_and(is_repo_not_found),
"{failed:#}"
);
}
}