vinzenz/hyperhive
vinzenz/hyperhive
Watch
0
0
Fork
You've already forked hyperhive
 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

fix(#2393): ff-merge config prs via forge api instead of pushing main

Browse source
This commit is contained in:
damocles 2026-07-13 11:18:43 +02:00
commit 6e0f6893cf
4 changed files with 135 additions and 197 deletions
Download patch file Download diff file Expand all files Collapse all files

30
hive-c0re/src/actions.rs
View file

@ -290,33 +290,27 @@ async fn run_merge_config_pr(
Err(e) => return (Err(anyhow::anyhow!("read applied/main: {e:#}")), None),
};
// 4. THE merge: fast-forward the forge repo's main to the reviewed head.
coord.set_queue_step(queue_entry_id, "fast-forward forge main");
match crate::forge::ff_push_to_main(&repo, &reviewed).await {
// 4. THE merge: fast-forward-only merge the reviewed head to `main` via the
// forge API, pinned to the reviewed sha (`head_commit_id`). This one call
// both advances `main` to the reviewed head and marks the PR merged — no
// direct push to the protected branch. Unlike the old push-then-mark split,
// a failure here means `main` was NOT advanced, so it's fatal: we must not
// deploy a head the forge didn't merge.
coord.set_queue_step(queue_entry_id, "fast-forward-merge PR");
match crate::forge::merge_config_pr_ff(&repo, pr, &reviewed).await {
Ok(()) => {}
Err(crate::forge::ForgeMergeError::NotFastForward { .. }) => {
Err(crate::forge::ForgeMergeError::HeadDrift { expected, actual }) => {
return (
Err(anyhow::anyhow!(
"PR #{pr}: forge main raced ahead of reviewed {reviewed}; re-review before merging"
"PR #{pr} head drifted before merge (reviewed {expected}, now {actual}); re-review before merging"
)),
None,
);
}
Err(e) => return (Err(anyhow::anyhow!("ff-push PR #{pr} to main: {e}")), None),
Err(e) => return (Err(anyhow::anyhow!("ff-merge PR #{pr}: {e}")), None),
}
// 5. Mark the PR merged. Best-effort: main is already at the reviewed
// head, so the deploy is correct regardless — a failure here (incl.
// HeadDrift vs the forge PR record) is logged, not fatal.
coord.set_queue_step(queue_entry_id, "mark PR merged");
if let Err(e) = crate::forge::mark_pr_merged(&repo, pr, &reviewed).await {
tracing::warn!(
agent = %approval.agent, %id, %pr, error = ?e,
"mark PR merged failed; main already at reviewed head, continuing to deploy"
);
}
// 6. Shared deploy tail. target == finalize == the reviewed head;
// 5. Shared deploy tail. target == finalize == the reviewed head;
// never a first spawn (the agent already exists).
deploy_applied_target(
coord,

4
hive-c0re/src/forge/mod.rs
View file

@ -10,8 +10,8 @@ mod repos;
mod users;
pub use pr_merge::{
ForgeMergeError, config_repo, fetch_pr_head_into_applied, ff_push_to_main, mark_pr_merged,
pr_head_sha, pr_is_open,
ForgeMergeError, config_repo, fetch_pr_head_into_applied, merge_config_pr_ff, pr_head_sha,
pr_is_open,
};
pub use repos::{
create_agent_repo, ensure_config_repo, ensure_knowledge_repo, ensure_meta_remote, ensure_repo,

183
hive-c0re/src/forge/pr_merge.rs
View file

@ -4,7 +4,6 @@
//! boundary; moved verbatim from the `forge` module root.
use anyhow::Context;
use forgejo_api::ForgejoError;
use forgejo_api::structs::{MergePullRequestOption, MergePullRequestOptionDo, StateType};
use super::{CONFIG_ORG, api, core_token, forge_git_url};
@ -14,33 +13,30 @@ use super::{CONFIG_ORG, api, core_token, forge_git_url};
// dashboard-approve-driven config-change flow).
//
// The dashboard-approve-driven flow has hive-c0re verify an operator-approved
// config PR, then land it: ff-push the verified sha to the protected default
// branch (= the merge) and mark the PR merged manually. These three fns are
// config PR, then land it: fast-forward-merge the verified sha into the
// protected default branch via the forge merge API (= the merge). These fns are
// the forge-side mechanics the c0re approve-handler (`run_merge_config_pr`)
// orchestrates; the orchestration fetches the verified sha into the agent's
// applied repo before calling `ff_push_to_main`. The core token is sourced
// internally (`core_token`), never passed in. `repo` is the agent's editable
// forge config repo in `owner/name` form (e.g. `agent-configs/<agent>`).
// applied repo (for the eval-verify) before calling `merge_config_pr_ff`. The
// core token is sourced internally (`core_token`), never passed in. `repo` is
// the agent's editable forge config repo in `owner/name` form (e.g.
// `agent-configs/<agent>`).
// ---------------------------------------------------------------------------
/// Typed failure for the merge primitives so the c0re approve-handler can
/// `match` recoverable drift (refresh the request sha + re-verify) against a
/// hard failure (fail the approval). The two drift variants carry the observed
/// sha so the handler can re-pin to it; `Other` is everything else (transport,
/// API, unexpected) and is not auto-retried.
/// `match` recoverable drift (surface a re-review message) against a hard
/// failure (fail the approval). `HeadDrift` carries the observed sha; `Other`
/// is everything else (a raced non-ff `main`, transport, API, unexpected) and
/// is not auto-retried.
#[derive(Debug)]
pub enum ForgeMergeError {
/// The PR head moved off `expected` (now at `actual`), seen at
/// mark-merged time. The handler's pre-merge `pr_head_sha` re-read is the
/// primary drift gate; this is the belt-and-suspenders race catch.
/// The PR head moved off `expected` (now at `actual`) since the handler's
/// pre-merge `pr_head_sha` re-read — caught by the `head_commit_id` pin on
/// the merge call, which refuses to merge a head that isn't the reviewed sha.
HeadDrift { expected: String, actual: String },
/// `main` (`actual_head`) is not a descendant of `expected_ancestor`, so
/// pushing the verified sha would not be a fast-forward — `main` raced.
NotFastForward {
expected_ancestor: String,
actual_head: String,
},
/// Transport / API / unexpected failure — hard-fail, no auto-retry.
/// Transport / API / unexpected failure — hard-fail, no auto-retry. Covers
/// a raced non-fast-forwardable `main` (the `fast-forward-only` merge is
/// refused by the forge) as well.
Other(anyhow::Error),
}
@ -50,13 +46,6 @@ impl std::fmt::Display for ForgeMergeError {
Self::HeadDrift { expected, actual } => {
write!(f, "PR head drifted: expected {expected}, found {actual}")
}
Self::NotFastForward {
expected_ancestor,
actual_head,
} => write!(
f,
"not a fast-forward: main {actual_head} is not a descendant of {expected_ancestor}"
),
Self::Other(e) => write!(f, "{e}"),
}
}
@ -77,8 +66,8 @@ fn repo_agent_name(repo: &str) -> &str {
/// Resolve a PR's head sha via `git ls-remote <repo> refs/pull/<pr>/head`
/// (Forgejo exposes PR heads there). Pure read, no mutation — the handler's
/// primary drift gate (compare against the approved sha), and `mark_pr_merged`
/// uses it to detect head-drift on failure.
/// primary drift gate (compare against the approved sha), and
/// `merge_config_pr_ff` uses it to classify a merge-API rejection as head-drift.
///
/// # Errors
/// `Other` on transport failure or an empty/missing ref.
@ -145,9 +134,8 @@ pub fn config_repo(agent: &str) -> String {
/// Fetch PR #`pr`'s head into the agent's applied repo via
/// `refs/pull/<pr>/head` (which Forgejo always serves — a bare-sha fetch can
/// be refused by uploadpack policy). This makes the reviewed head an object
/// in the applied repo so the ancestor check in [`ff_push_to_main`], the
/// `git_update_ref(main, …)` in the deploy tail, and the eval-verify all
/// resolve it locally before the irreversible push.
/// in the applied repo so the `git_update_ref(main, …)` in the deploy tail and
/// the eval-verify both resolve it locally before the irreversible merge.
///
/// # Errors
/// `Other` on transport failure or a non-zero git exit.
@ -173,103 +161,20 @@ pub async fn fetch_pr_head_into_applied(repo: &str, pr: u64) -> Result<(), Forge
Ok(())
}
/// Fast-forward the forge repo's `main` to `sha` — THE merge in the PR flow.
/// Reads `main`'s current sha (`git ls-remote … refs/heads/main`), verifies it
/// is a strict ancestor of `sha` (`git merge-base --is-ancestor`, run in the
/// agent's applied repo where the orchestration has already fetched `sha`),
/// then does a **non-force** `git push <sha>:refs/heads/main`. The ancestor
/// pre-check and the non-force push are two independent guards: either catches
/// a raced `main` (→ `NotFastForward`) rather than clobbering reviewed history.
/// Fast-forward-merge PR `pr` on `repo` to `sha` via the Forgejo merge API —
/// THE merge in the config-PR flow. Uses `Do=fast-forward-only` so `main` only
/// ever advances by fast-forward (never a merge commit; a raced non-ff `main`
/// is refused by the forge → `Other`), and pins `head_commit_id = sha` so the
/// forge atomically refuses to merge a head that isn't the reviewed sha —
/// closing the check-then-merge race without ever pushing the protected branch.
/// `core` needs only the repo's merge whitelist, never push access to `main`.
/// The single call fast-forwards `main` to exactly `sha` and marks the PR
/// merged.
///
/// # Errors
/// `NotFastForward` if `main` raced ahead of `sha`; `Other` on transport/other.
pub async fn ff_push_to_main(repo: &str, sha: &str) -> Result<(), ForgeMergeError> {
let token = core_token()
.ok_or_else(|| ForgeMergeError::Other(anyhow::anyhow!("forge core token absent")))?;
let url = forge_git_url(&token, repo);
let applied = crate::paths::applied_dir(repo_agent_name(repo));
// Current `main` on the forge repo.
let ls = crate::lifecycle::git_command()
.args(["ls-remote", &url, "refs/heads/main"])
.output()
.await
.context("git ls-remote main")?;
if !ls.status.success() {
return Err(ForgeMergeError::Other(anyhow::anyhow!(
"git ls-remote {repo} refs/heads/main failed ({}): {}",
ls.status,
String::from_utf8_lossy(&ls.stderr).trim()
)));
}
let ls_out = String::from_utf8_lossy(&ls.stdout);
let main_sha = ls_out.split_whitespace().next().unwrap_or("").to_string();
// Strict-ancestor check: `main` must be an ancestor of `sha` for a true
// fast-forward. Skip when `main` is unborn (empty) — the push creates it.
if !main_sha.is_empty() {
let anc = crate::lifecycle::git_command()
.current_dir(&applied)
.args(["merge-base", "--is-ancestor", &main_sha, sha])
.output()
.await
.context("git merge-base --is-ancestor")?;
match anc.status.code() {
Some(0) => {} // ancestor → fast-forward safe
Some(1) => {
return Err(ForgeMergeError::NotFastForward {
expected_ancestor: main_sha,
actual_head: sha.to_string(),
});
}
_ => {
return Err(ForgeMergeError::Other(anyhow::anyhow!(
"git merge-base --is-ancestor errored ({}): {}",
anc.status,
String::from_utf8_lossy(&anc.stderr).trim()
)));
}
}
}
// Non-force push `sha` → `main`. Without `--force`, git rejects a
// non-fast-forward (a race between the check above and now), surfaced as
// `NotFastForward` rather than clobbering the remote.
let push = crate::lifecycle::git_command()
.current_dir(&applied)
.args(["push", &url, &format!("{sha}:refs/heads/main")])
.output()
.await
.context("git push sha:main")?;
if !push.status.success() {
let stderr = String::from_utf8_lossy(&push.stderr);
if stderr.contains("non-fast-forward") || stderr.contains("fetch first") {
return Err(ForgeMergeError::NotFastForward {
expected_ancestor: main_sha,
actual_head: sha.to_string(),
});
}
return Err(ForgeMergeError::Other(anyhow::anyhow!(
"git push {repo} {sha}:main failed ({}): {}",
push.status,
stderr.trim()
)));
}
Ok(())
}
/// Mark PR `pr` as **manually merged** at `sha` (Forgejo
/// `repo_merge_pull_request` with `Do=manually-merged`, `MergeCommitID=sha`).
/// `ff_push_to_main` must have already set `main` to `sha` (Forgejo requires
/// the branch already be at the merge commit). On an API rejection, re-reads
/// the PR head to distinguish drift (`HeadDrift`) from a generic failure
/// (`Other`) — best-effort, since the handler's pre-merge head re-read is
/// the real gate. A transport failure skips the drift re-read (it couldn't
/// reach the forge either) and surfaces as `Other` directly.
///
/// # Errors
/// `HeadDrift` if the PR head no longer matches `sha`; `Other` otherwise.
pub async fn mark_pr_merged(repo: &str, pr: u64, sha: &str) -> Result<(), ForgeMergeError> {
/// `HeadDrift` if the live PR head no longer matches `sha` (the pin rejected
/// it); `Other` for a raced non-ff `main`, transport, or API failure.
pub async fn merge_config_pr_ff(repo: &str, pr: u64, sha: &str) -> Result<(), ForgeMergeError> {
let token = core_token()
.ok_or_else(|| ForgeMergeError::Other(anyhow::anyhow!("forge core token absent")))?;
let (owner, name) = repo.split_once('/').ok_or_else(|| {
@ -278,13 +183,13 @@ pub async fn mark_pr_merged(repo: &str, pr: u64, sha: &str) -> Result<(), ForgeM
let index = i64::try_from(pr)
.map_err(|_| ForgeMergeError::Other(anyhow::anyhow!("PR index {pr} overflows i64")))?;
let body = MergePullRequestOption {
r#do: MergePullRequestOptionDo::ManuallyMerged,
merge_commit_id: Some(sha.to_owned()),
r#do: MergePullRequestOptionDo::FastForwardOnly,
merge_commit_id: None,
merge_message_field: None,
merge_title_field: None,
delete_branch_after_merge: None,
force_merge: None,
head_commit_id: None,
head_commit_id: Some(sha.to_owned()),
merge_when_checks_succeed: None,
};
let client = api(&token).map_err(ForgeMergeError::Other)?;
@ -293,21 +198,21 @@ pub async fn mark_pr_merged(repo: &str, pr: u64, sha: &str) -> Result<(), ForgeM
.await
{
Ok(()) => Ok(()),
Err(ForgejoError::ReqwestError(e)) => Err(ForgeMergeError::Other(
anyhow::Error::from(e).context("POST pulls/<pr>/merge (manually-merged)"),
)),
Err(e) => {
// Best-effort drift detection: if the live head no longer matches
// `sha`, that's a head-drift race; otherwise surface as a hard
// failure.
// Classify: a live head that no longer matches `sha` is drift (the
// `head_commit_id` pin rejected it); everything else — a raced
// non-ff `main`, transport, an API error — is a hard failure.
// Best-effort, since the handler's pre-merge re-read is the primary
// gate; a transport failure that also breaks this read falls through
// to `Other`.
match pr_head_sha(repo, pr).await {
Ok(actual) if actual != sha => Err(ForgeMergeError::HeadDrift {
expected: sha.to_string(),
actual,
}),
_ => Err(ForgeMergeError::Other(anyhow::Error::from(e).context(
format!("mark PR #{pr} in {repo} manually-merged at {sha}"),
))),
_ => Err(ForgeMergeError::Other(
anyhow::Error::from(e).context(format!("ff-merge PR #{pr} in {repo} to {sha}")),
)),
}
}
}

115
hive-c0re/src/forge/repos.rs
View file

@ -11,8 +11,8 @@ use anyhow::{Context, Result};
use forgejo_api::structs::{
AddCollaboratorOption, AddCollaboratorOptionPermission, CreateBranchProtectionOption,
CreateOrgOption, CreateRepoOption, CreateTeamOption, CreateTeamOptionPermission,
EditRepoOption, EditTeamOption, EditTeamOptionPermission, MigrateRepoOptions,
MigrateRepoOptionsService, Repository,
EditBranchProtectionOption, EditRepoOption, EditTeamOption, EditTeamOptionPermission,
MigrateRepoOptions, MigrateRepoOptionsService, Repository,
};
use forgejo_api::{ApiErrorKind, ForgejoError};
use reqwest::StatusCode;
@ -795,36 +795,35 @@ async fn apply_operator_branch_protection(repo: &str, token: &str) -> Result<()>
/// Apply branch protection to an `agent-configs/<name>` repo's `main` so it
/// can serve as the agent-editable, PR-merge config surface:
/// - **push + merge whitelists are `core`-only** — the agent (a write
/// collaborator) can push feature branches and open config PRs, but only
/// hive-c0re lands on `main`, via its verify-and-ff-push merge handler
/// (`run_merge_config_pr`). The agent can never push `main` directly.
/// - **operator-team approval is required** to merge, and the author (not in
/// the team) cannot self-approve.
/// - **force-pushing `main` stays impossible** — `main` only ever advances by
/// fast-forward. The merge handler's `ff_push_to_main` is already a
/// non-force push, so it lands fine. The legacy `push_config` mirror is
/// also ff-only (non-fast-forward is caught and silently skipped).
/// (Auto force-push is intentionally not allowed: per operator directive a
/// silent force-push is a bug, not a feature. The raw-HTTP predecessor
/// sent `"enable_force_push":false` + `"allow_manual_merge":true` in this
/// body; neither is a `CreateBranchProtectionOption` field, so Forgejo
/// ignored both keys — dropping them changes nothing: force-push
/// protection defaults to off, and `allow_manual_merge` is a *repo*
/// setting, not a branch-protection one.)
/// - **`main` is never directly pushable** — no push is enabled on the
/// protected branch, so neither the agent (a write collaborator) nor
/// hive-c0re can `git push` it. It only advances via the config-PR merge
/// handler (`run_merge_config_pr`), which fast-forward-*merges* the reviewed
/// head through the forge merge API (`Do=fast-forward-only`,
/// `head_commit_id` pinned to the reviewed sha).
/// - **merge is whitelisted to `core`** — only hive-c0re can merge a config PR;
/// the agent can push feature branches + open PRs but can't land them.
/// - **the operator's dashboard approval is the gate** — approval happens on
/// the `MergeConfigPr` card and hive-c0re only merges an approved PR. There's
/// deliberately no Forgejo `required_approvals` review requirement: the flow
/// never does an in-forge review, so requiring one would only dead-block the
/// `core` merge. The dashboard approval + the `core`-only merge whitelist are
/// the real gate.
/// - **fast-forward-only** — `main` only ever advances by fast-forward; a raced
/// non-ff `main` is refused by the merge API rather than force-moved.
///
/// Idempotent: an existing rule for the branch (200/409/422) is success.
async fn apply_config_repo_branch_protection(repo: &str, token: &str) -> Result<()> {
let client = api(token)?;
let mut rule = main_branch_protection_option();
rule.enable_push_whitelist = Some(true);
rule.push_whitelist_usernames = Some(vec!["core".to_owned()]);
// Only `core` may merge (through the config-PR merge API); no push is
// enabled at all, so `main` can only advance via that merge. Push +
// approval defaults are off in `main_branch_protection_option`, so a fresh
// rule needs nothing but the merge whitelist. (`config_repo_protection_edit`
// must clear the old push/approval fields explicitly, since a PATCH leaves
// unset fields untouched.)
rule.enable_merge_whitelist = Some(true);
rule.merge_whitelist_usernames = Some(vec!["core".to_owned()]);
rule.enable_approvals_whitelist = Some(true);
rule.approvals_whitelist_teams = Some(vec![OPERATORS_TEAM.to_owned()]);
rule.required_approvals = Some(1);
rule.block_on_official_review_requests = Some(true);
let Err(create_err) = client
.repo_create_branch_protection(CONFIG_ORG, repo, rule)
.await
@ -832,33 +831,73 @@ async fn apply_config_repo_branch_protection(repo: &str, token: &str) -> Result<
tracing::info!(%repo, "forge: applied config-repo branch protection");
return Ok(());
};
// A create failure 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: verify the `main` rule actually exists, and on failure
// surface the create error (its Display carries Forgejo's validation
// message) so the real reason is in the journal.
// Create failed. This is ambiguous — "rule already exists" (the common
// idempotent case) OR a silent rejection that created no rule. Either way a
// create never *updates* an existing rule, and repos protected under an
// older shape (the push-based / approval-gated rules) carry stale settings.
// So converge the existing rule with a PATCH that explicitly clears them:
// it fixes those stale repos on the next boot's `ensure_config_repo` pass,
// is a harmless no-op when the rule is already correct, and still fails
// loudly when no rule can be established (you can't edit a rule that isn't
// there) — so it can't leave a repo silently unprotected.
match client
.repo_get_branch_protection(CONFIG_ORG, repo, "main")
.repo_edit_branch_protection(CONFIG_ORG, repo, "main", config_repo_protection_edit())
.await
{
Ok(_) => {
// Debug, not info: this PATCH runs on every `ensure_config_repo`
// boot pass for every existing repo (create → 409 → converge), so
// it's almost always a no-op re-assertion — info-logging it would
// be N lines of noise per boot on a many-agent hive.
tracing::debug!(
%repo, create_error = %create_err,
"forge: config-repo branch protection already present"
"forge: converged existing config-repo branch protection"
);
Ok(())
}
Err(check_err) => anyhow::bail!(
Err(edit_err) => anyhow::bail!(
"branch protection for {CONFIG_ORG}/{repo} not applied: create failed \
({create_err}); GET main rule failed ({check_err}), no `main` rule present"
({create_err}); edit of existing `main` rule failed ({edit_err})"
),
}
}
/// The `EditBranchProtectionOption` that converges an existing
/// `agent-configs/<name>` `main` rule to the current desired shape: merge
/// whitelisted to `core`, **no direct push at all**, and **no in-forge approval
/// requirement** (the dashboard approval + `core`-only merge whitelist are the
/// gate). The push/approval fields are set to their explicit off-values, not
/// left `None`, so a repo carrying an older push-based or approval-gated rule is
/// actually *converged* rather than merely re-asserted — a PATCH leaves unset
/// fields untouched. Every field unrelated to this policy stays `None`.
fn config_repo_protection_edit() -> EditBranchProtectionOption {
EditBranchProtectionOption {
apply_to_admins: None,
approvals_whitelist_teams: None,
approvals_whitelist_username: None,
block_on_official_review_requests: Some(false),
block_on_outdated_branch: None,
block_on_rejected_reviews: None,
dismiss_stale_approvals: None,
enable_approvals_whitelist: Some(false),
enable_merge_whitelist: Some(true),
enable_push: Some(false),
enable_push_whitelist: Some(false),
enable_status_check: None,
ignore_stale_approvals: None,
merge_whitelist_teams: None,
merge_whitelist_usernames: Some(vec!["core".to_owned()]),
protected_file_patterns: None,
push_whitelist_deploy_keys: None,
push_whitelist_teams: None,
push_whitelist_usernames: Some(Vec::new()),
require_signed_commits: None,
required_approvals: Some(0),
status_check_contexts: None,
unprotected_file_patterns: None,
}
}
/// Create a repo for `agent` in the c0re-owned [`AGENTS_ORG`] and wire the
/// perms: the org owns it (perms stay c0re-managed), the agent is added
/// as a **write** collaborator (not owner — can push + open PRs but can't