diff --git a/hive-c0re/src/actions.rs b/hive-c0re/src/actions.rs index 68b6bb66..5a6bcc45 100644 --- a/hive-c0re/src/actions.rs +++ b/hive-c0re/src/actions.rs @@ -198,9 +198,6 @@ pub async fn run_approval_merge_config_pr( let approval = fetch_approval_for_worker(coord, approval_id, ApprovalKind::MergeConfigPr)?; let agent_dir = crate::paths::agent_runtime_dir(&approval.agent); let applied_dir = crate::paths::applied_dir(&approval.agent); - // Captured up front to scope the failure-comment's build-log lookup to - // rows this deploy produced (see `post_merge_failure_to_pr`). - let since_ts = hive_sh4re::wire_time::now_unix(); coord.set_queue_step(queue_entry_id, "merge config pr"); let (result, terminal_tag) = run_merge_config_pr(coord, &approval, &agent_dir, &applied_dir, queue_entry_id).await; @@ -213,84 +210,9 @@ pub async fn run_approval_merge_config_pr( if let Err(e) = crate::forge::push_config(&approval.agent).await { tracing::warn!(agent = %approval.agent, error = ?e, "forge: push_config after merge failed"); } - // On a failed deploy, surface the failing build log back onto the PR so - // the manager sees why it was rejected without leaving the forge. - if let Err(e) = &result { - post_merge_failure_to_pr(coord, &approval, since_ts, e).await; - } finish_approval(coord, &approval, result, terminal_tag, false) } -/// Max stderr bytes to inline in a PR failure comment. Keeps the comment -/// readable and under forge's size limits while still carrying the tail -/// where the nix/build error actually surfaces. -const PR_FAIL_LOG_TAIL_BYTES: usize = 4000; - -/// On a failed `MergeConfigPr` deploy, post the failing build log back to the -/// config PR as a comment so the manager sees the rejection reason on the PR -/// itself. Best-effort: any error here is logged, never allowed to disturb the -/// approval-resolution path. -/// -/// The failing `build_log` row is located heuristically: the most recent `fail` -/// row for this agent that started at/after `since_ts` (the caller's function -/// entry). Because deploys are serialised per agent through the queue, that is -/// the step which just failed — `verify`, `prepare-deploy`, `prebuild`, or the -/// container rebuild. Pre-build failures (drift gate, fetch) create no `build_log` -/// row, so the comment then carries only the error text. -async fn post_merge_failure_to_pr( - coord: &Arc, - approval: &hive_sh4re::Approval, - since_ts: i64, - err: &anyhow::Error, -) { - let Ok(pr) = approval.commit_ref.parse::() else { - return; - }; - let repo = crate::forge::config_repo(&approval.agent); - - let log_section = coord - .build_logs - .list_recent_for_agent(&approval.agent, 10) - .ok() - .and_then(|rows| { - rows.into_iter() - .find(|r| r.status.as_deref() == Some("fail") && r.started_at >= since_ts) - }) - .and_then(|row| coord.build_logs.get_full(row.id).ok().flatten()) - .map(|full| { - let tail = tail_bytes(full.stderr.trim_end(), PR_FAIL_LOG_TAIL_BYTES); - format!( - "\n\n**Failing step:** `{}` (build log #{})\n\n```\n{tail}\n```", - full.header.kind, full.header.id - ) - }) - .unwrap_or_default(); - - let body = format!( - "## ⚠️ config deploy failed\n\n\ - Approval #{} to merge this PR could not be deployed:\n\n\ - ```\n{err:#}\n```{log_section}", - approval.id - ); - - if let Err(e) = crate::forge::post_pr_comment(&repo, pr, &body).await { - tracing::warn!(agent = %approval.agent, %pr, error = ?e, "post merge-failure comment to PR failed"); - } -} - -/// Return the last `max_bytes` of `s`, snapped to a char boundary, prefixed -/// with an elision marker when truncated. -fn tail_bytes(s: &str, max_bytes: usize) -> String { - if s.len() <= max_bytes { - return s.to_owned(); - } - let mut start = s.len() - max_bytes; - while start < s.len() && !s.is_char_boundary(start) { - start += 1; - } - format!("[… truncated …]\n{}", &s[start..]) -} - /// PR-merge config pipeline. `approval.commit_ref` is the PR number; /// `approval.fetched_sha` is the PR head sha the operator reviewed. Steps: /// 1. drift gate — re-read the live PR head; if it moved since review, abort diff --git a/hive-c0re/src/forge/config_pr_poll.rs b/hive-c0re/src/forge/config_pr_poll.rs index ba233b44..b74b587a 100644 --- a/hive-c0re/src/forge/config_pr_poll.rs +++ b/hive-c0re/src/forge/config_pr_poll.rs @@ -93,12 +93,32 @@ pub async fn poll_open_config_prs(core_token: &str, coord: &Arc) -> }; open_prs.insert((agent.to_owned(), pr_number)); - // `submit_merge_config_pr` is idempotent + PR-drift aware: it - // no-ops when an approval pinned to this PR's *current* head is - // already pending, and cancels+re-queues when the head has drifted. - // So the poll can call it unconditionally — it backstops both a - // missed `opened` webhook (no approval yet) and a missed - // `synchronize` (stale approval whose head moved). + // Skip if a pending approval already exists for this PR. + match coord + .approvals + .has_pending_merge_config_pr(agent, pr_number) + { + Ok(true) => { + tracing::debug!( + %agent, %pr_number, + "config-pr poll: approval already pending, skipping" + ); + continue; + } + Ok(false) => {} + Err(e) => { + tracing::warn!( + %agent, %pr_number, error = ?e, + "config-pr poll: DB check failed, skipping" + ); + continue; + } + } + + tracing::info!( + %agent, %pr_number, + "config-pr poll: queuing missed MergeConfigPr approval" + ); let description = format!("PR #{pr_number} on {CONFIG_ORG}/{agent} (poll fallback)"); if let Err(e) = crate::socket_server::submit_merge_config_pr( coord, @@ -111,7 +131,7 @@ pub async fn poll_open_config_prs(core_token: &str, coord: &Arc) -> { tracing::warn!( %agent, %pr_number, error = ?e, - "config-pr poll: failed to reconcile MergeConfigPr approval" + "config-pr poll: failed to queue MergeConfigPr approval" ); } } diff --git a/hive-c0re/src/forge/mod.rs b/hive-c0re/src/forge/mod.rs index 26708c91..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, merge_config_pr_ff, post_pr_comment, - 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 bd259e6e..4207e963 100644 --- a/hive-c0re/src/forge/pr_merge.rs +++ b/hive-c0re/src/forge/pr_merge.rs @@ -218,38 +218,6 @@ pub async fn merge_config_pr_ff(repo: &str, pr: u64, sha: &str) -> Result<(), Fo } } -/// Post a comment to PR (= issue) `pr` on `repo` as the core forge user. -/// PRs are issues in Forgejo, so the PR number is the issue index. Used to -/// surface a failed config-approval deploy's build log back onto the PR so -/// the manager sees why it was rejected without leaving the forge. `repo` -/// is `owner/name`. -/// -/// # Errors -/// `Other` on absent core token, malformed repo, or transport/API failure. -pub async fn post_pr_comment(repo: &str, pr: u64, body: &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(|| { - ForgeMergeError::Other(anyhow::anyhow!("forge repo `{repo}` is not owner/name")) - })?; - let index = i64::try_from(pr) - .map_err(|_| ForgeMergeError::Other(anyhow::anyhow!("PR index {pr} overflows i64")))?; - let client = api(&token).map_err(ForgeMergeError::Other)?; - client - .issue_create_comment( - owner, - name, - index, - forgejo_api::structs::CreateIssueCommentOption { - body: body.to_owned(), - updated_at: None, - }, - ) - .await - .map_err(|e| ForgeMergeError::Other(anyhow::Error::from(e).context("post PR comment")))?; - Ok(()) -} - #[cfg(test)] mod tests { use super::repo_agent_name; diff --git a/hive-c0re/src/socket_server/config_approvals.rs b/hive-c0re/src/socket_server/config_approvals.rs index 6a3ea8f3..37bdc2e7 100644 --- a/hive-c0re/src/socket_server/config_approvals.rs +++ b/hive-c0re/src/socket_server/config_approvals.rs @@ -157,31 +157,6 @@ pub(crate) async fn submit_merge_config_pr( let sha = crate::forge::pr_head_sha(&repo, pr_number) .await .map_err(|e| anyhow::anyhow!("fetch PR head sha for {agent} PR #{pr_number}: {e}"))?; - // Both the webhook (`synchronize`) and the poll fallback call this on - // every PR update. If an approval for this PR is already pending, reconcile - // it against the live head sha rather than blindly queuing another: - // - same sha → the PR hasn't moved, so this is a duplicate signal — no-op. - // - drifted sha → the reviewed head is stale. Don't mutate the - // pending row in place (that races a concurrent approve); cancel it and - // fall through to queue a FRESH approval pinned to the new head. - if let Some((old_id, old_sha)) = coord.approvals.pending_merge_config_pr(agent, pr_number)? { - if old_sha.as_deref() == Some(sha.as_str()) { - return Ok(old_id); - } - let cancelled = coord - .approvals - .mark_cancelled(old_id, "config PR updated — superseded by a fresh approval") - .map_err(|e| anyhow::anyhow!("cancel superseded merge_config_pr approval: {e:#}"))?; - coord.emit_approval_resolved(crate::coordinator::ApprovalResolved { - id: old_id, - agent, - approval_kind: "merge_config_pr", - sha_short: old_sha.map(|s| s[..s.len().min(12)].to_owned()), - status: "cancelled", - note: Some("PR head moved; superseded by a fresh approval".to_owned()), - description: cancelled.description, - }); - } let id = coord .approvals .submit_kind( diff --git a/hive-c0re/src/stores/approvals.rs b/hive-c0re/src/stores/approvals.rs index da67397a..1f511e35 100644 --- a/hive-c0re/src/stores/approvals.rs +++ b/hive-c0re/src/stores/approvals.rs @@ -136,29 +136,19 @@ impl Approvals { Ok(()) } - /// Return the `(id, fetched_sha)` of the pending `merge_config_pr` - /// approval for `(agent, pr_number)`, if one exists. Drives - /// `submit_merge_config_pr`'s idempotency + PR-drift handling: same - /// `fetched_sha` → no new request (the webhook + poll both call submit, - /// so re-submits of an unchanged PR must be no-ops); a drifted head → - /// cancel this stale row and queue a fresh approval. - pub fn pending_merge_config_pr( - &self, - agent: &str, - pr_number: u64, - ) -> Result)>> { + /// Return `true` when there is already a `pending` `merge_config_pr` + /// approval for `(agent, pr_number)`. Used by the polling fallback to + /// skip re-submitting approvals that were already queued by the webhook. + pub fn has_pending_merge_config_pr(&self, agent: &str, pr_number: u64) -> Result { let conn = self.conn.lock().unwrap(); - let row = conn - .query_row( - "SELECT id, fetched_sha FROM approvals \ - WHERE agent = ?1 AND kind = 'merge_config_pr' \ - AND commit_ref = ?2 AND status = 'pending' \ - ORDER BY id DESC LIMIT 1", - params![agent, pr_number.to_string()], - |row| Ok((row.get(0)?, row.get(1)?)), - ) - .optional()?; - Ok(row) + let count: i64 = conn.query_row( + "SELECT COUNT(*) FROM approvals \ + WHERE agent = ?1 AND kind = 'merge_config_pr' \ + AND commit_ref = ?2 AND status = 'pending'", + params![agent, pr_number.to_string()], + |row| row.get(0), + )?; + Ok(count > 0) } /// Last `limit` resolved approvals (approved / denied / failed),