From 6e0f6893cf2ae47c0b27455c403127a706769722 Mon Sep 17 00:00:00 2001 From: damocles Date: Mon, 13 Jul 2026 11:18:43 +0200 Subject: [PATCH] fix(#2393): ff-merge config prs via forge api instead of pushing main --- hive-c0re/src/actions.rs | 30 +++--- hive-c0re/src/forge/mod.rs | 4 +- hive-c0re/src/forge/pr_merge.rs | 183 ++++++++------------------------ hive-c0re/src/forge/repos.rs | 115 +++++++++++++------- 4 files changed, 135 insertions(+), 197 deletions(-) diff --git a/hive-c0re/src/actions.rs b/hive-c0re/src/actions.rs index 6aba31fe..d95e588a 100644 --- a/hive-c0re/src/actions.rs +++ b/hive-c0re/src/actions.rs @@ -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, diff --git a/hive-c0re/src/forge/mod.rs b/hive-c0re/src/forge/mod.rs index 79cf118f..9bbea410 100644 --- a/hive-c0re/src/forge/mod.rs +++ b/hive-c0re/src/forge/mod.rs @@ -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, diff --git a/hive-c0re/src/forge/pr_merge.rs b/hive-c0re/src/forge/pr_merge.rs index 865cce5c..4207e963 100644 --- a/hive-c0re/src/forge/pr_merge.rs +++ b/hive-c0re/src/forge/pr_merge.rs @@ -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/`). +// 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/`). // --------------------------------------------------------------------------- /// 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 refs/pull//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//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 :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//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}")), + )), } } } diff --git a/hive-c0re/src/forge/repos.rs b/hive-c0re/src/forge/repos.rs index 452b4178..2778d070 100644 --- a/hive-c0re/src/forge/repos.rs +++ b/hive-c0re/src/forge/repos.rs @@ -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/` 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/` `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