fix(#2051): verify config-repo branch protection actually applied
apply_config_repo_branch_protection treated 200/409/422 from the create-branch-protection POST all as success. But a 422 means Forgejo *rejected* the request and created no rule — so a rejected POST silently left the agent's config repo unprotected, with nothing logged (a new agent's config repo was found with no main-branch protection and no trace of why). Don't trust the status code: - On any non-201, GET the single .../branch_protections/main rule and only treat it as success if the rule is actually present. - Otherwise return Err carrying the POST's response body, so the real Forgejo rejection reason lands in the host journal. (forge_http discarded the body; added forge_http_full that returns it.) ensure_config_repo runs on every sync_agent sweep (startup + each rebuild), so a now-Err result is logged and retried next sweep — self-healing once a real cause is fixed. Net: the failure is loud + retried instead of silently swallowed. nix fmt clean.
This commit is contained in:
parent
31433da3aa
commit
512e9ff09f
1 changed files with 48 additions and 13 deletions
|
|
@ -157,6 +157,30 @@ async fn forge_http(
|
|||
Ok(resp.status())
|
||||
}
|
||||
|
||||
/// Like [`forge_http`] but also returns the response body, so callers
|
||||
/// can log *why* Forgejo rejected a request (e.g. the validation
|
||||
/// message on a 422). Body read is best-effort — a read error yields an
|
||||
/// empty string rather than failing the whole call.
|
||||
async fn forge_http_full(
|
||||
method: reqwest::Method,
|
||||
url: &str,
|
||||
token: &str,
|
||||
body: &str,
|
||||
) -> Result<(StatusCode, String)> {
|
||||
let client = reqwest::Client::new();
|
||||
let resp = client
|
||||
.request(method, url)
|
||||
.header("Authorization", format!("token {token}"))
|
||||
.header("Content-Type", "application/json")
|
||||
.body(body.to_owned())
|
||||
.send()
|
||||
.await
|
||||
.with_context(|| format!("forge HTTP request to {url}"))?;
|
||||
let status = resp.status();
|
||||
let text = resp.text().await.unwrap_or_default();
|
||||
Ok((status, text))
|
||||
}
|
||||
|
||||
/// Ensure a forgejo user named `name` exists. Idempotent: forgejo
|
||||
/// returns a "user already exists" error which we treat as success.
|
||||
/// `admin` adds `--admin` (site admin) — used for the bootstrap
|
||||
|
|
@ -1013,19 +1037,30 @@ async fn apply_config_repo_branch_protection(repo: &str, token: &str) -> Result<
|
|||
let body = format!(
|
||||
r#"{{"branch_name":"main","enable_push_whitelist":true,"push_whitelist_usernames":["core"],"enable_merge_whitelist":true,"merge_whitelist_usernames":["core"],"enable_approvals_whitelist":true,"approvals_whitelist_teams":["{OPERATORS_TEAM}"],"required_approvals":1,"block_on_official_review_requests":true,"allow_manual_merge":true,"enable_force_push":false}}"#
|
||||
);
|
||||
let status = forge_http(reqwest::Method::POST, &url, token, &body).await?;
|
||||
match status.as_u16() {
|
||||
201 => {
|
||||
tracing::info!(%repo, "forge: applied config-repo branch protection");
|
||||
Ok(())
|
||||
}
|
||||
200 | 409 | 422 => {
|
||||
tracing::debug!(%repo, "forge: config-repo branch protection already present");
|
||||
Ok(())
|
||||
}
|
||||
other => {
|
||||
anyhow::bail!("POST {CONFIG_ORG}/{repo}/branch_protections returned HTTP {other}")
|
||||
}
|
||||
let (status, resp_body) = forge_http_full(reqwest::Method::POST, &url, token, &body).await?;
|
||||
if status.as_u16() == 201 {
|
||||
tracing::info!(%repo, "forge: applied config-repo branch protection");
|
||||
return Ok(());
|
||||
}
|
||||
// Non-201 is ambiguous: it can mean "rule already exists" (idempotent
|
||||
// success) OR a silent rejection — e.g. a 422 where Forgejo refused
|
||||
// the request and created NO rule. The old code treated 200/409/422
|
||||
// all as success, so a rejected POST left the repo unprotected with
|
||||
// no error (the reported case: a new agent's config repo had no
|
||||
// `main` rule and nothing was logged). Don't trust the status code:
|
||||
// verify the `main` rule actually exists, and on failure surface the
|
||||
// POST's response body so the real reason is in the journal.
|
||||
let main_url = format!("{url}/main");
|
||||
let check = forge_http(reqwest::Method::GET, &main_url, token, "").await?;
|
||||
if check.as_u16() == 200 {
|
||||
tracing::debug!(%repo, %status, "forge: config-repo branch protection already present");
|
||||
Ok(())
|
||||
} else {
|
||||
anyhow::bail!(
|
||||
"branch protection for {CONFIG_ORG}/{repo} not applied: POST -> HTTP {status} \
|
||||
(body: {body}); GET main -> HTTP {check}, no `main` rule present",
|
||||
body = resp_body.trim(),
|
||||
)
|
||||
}
|
||||
}
|
||||
|
||||
|
|
|
|||
Loading…
Reference in a new issue