diff --git a/hive-c0re/src/dashboard/misc_api.rs b/hive-c0re/src/dashboard/misc_api.rs index d4c8d175..eb8fcadf 100644 --- a/hive-c0re/src/dashboard/misc_api.rs +++ b/hive-c0re/src/dashboard/misc_api.rs @@ -196,6 +196,7 @@ pub(super) async fn post_request_spawn( "", None, "operator", + None, ) { Ok(id) => { tracing::info!(%id, %name, "operator: spawn approval queued via dashboard"); diff --git a/hive-c0re/src/forge/mod.rs b/hive-c0re/src/forge/mod.rs index 167e631f..78da1236 100644 --- a/hive-c0re/src/forge/mod.rs +++ b/hive-c0re/src/forge/mod.rs @@ -10,7 +10,7 @@ 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_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 ca268140..865cce5c 100644 --- a/hive-c0re/src/forge/pr_merge.rs +++ b/hive-c0re/src/forge/pr_merge.rs @@ -5,7 +5,7 @@ use anyhow::Context; use forgejo_api::ForgejoError; -use forgejo_api::structs::{MergePullRequestOption, MergePullRequestOptionDo}; +use forgejo_api::structs::{MergePullRequestOption, MergePullRequestOptionDo, StateType}; use super::{CONFIG_ORG, api, core_token, forge_git_url}; @@ -112,6 +112,30 @@ pub async fn pr_head_sha(repo: &str, pr: u64) -> Result Ok(sha.to_string()) } +/// Check whether PR `pr` on `repo` is still open. Returns `Ok(true)` if +/// open, `Ok(false)` if closed or merged, or an error on transport failure. +/// +/// Called at submission time to give an early, actionable error rather than +/// queuing an approval card that will fail later in the approve handler. +/// +/// # Errors +/// `Other` on transport failure or a missing/malformed PR response. +pub async fn pr_is_open(repo: &str, pr: u64) -> Result { + 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)?; + let pull = client + .repo_get_pull_request(owner, name, index) + .await + .map_err(|e| ForgeMergeError::Other(anyhow::Error::from(e).context("GET pull request")))?; + Ok(pull.state == Some(StateType::Open)) +} + /// Full `owner/name` path of an agent's config repo on the forge — the /// `agent-configs` org mirror that the PR-merge flow reads + fast-forwards. pub fn config_repo(agent: &str) -> String { diff --git a/hive-c0re/src/server.rs b/hive-c0re/src/server.rs index f43da1db..ed7e589a 100644 --- a/hive-c0re/src/server.rs +++ b/hive-c0re/src/server.rs @@ -86,6 +86,7 @@ async fn dispatch(req: &HostRequest, coord: Arc) -> HostResponse { "", None, "operator", + None, )?; tracing::info!(%id, %name, "spawn approval queued"); HostResponse::success() diff --git a/hive-c0re/src/socket_server/config_approvals.rs b/hive-c0re/src/socket_server/config_approvals.rs index 1516a4cf..76ede8a0 100644 --- a/hive-c0re/src/socket_server/config_approvals.rs +++ b/hive-c0re/src/socket_server/config_approvals.rs @@ -83,6 +83,7 @@ pub(super) fn handle_request_update_meta_inputs( &commit_ref, description, requester, + None, ) .map_err(|e| anyhow::anyhow!("{e:#}")) { @@ -162,7 +163,21 @@ async fn submit_merge_config_pr( ); } let repo = crate::forge::config_repo(agent); + // Verify the PR is still open before queueing an approval that would + // fail at approve time anyway (a closed/merged PR has no live head ref + // for the drift gate to compare against). + if !crate::forge::pr_is_open(&repo, pr_number) + .await + .map_err(|e| anyhow::anyhow!("check PR state for {agent} PR #{pr_number}: {e}"))? + { + anyhow::bail!( + "PR #{pr_number} on {repo} is closed or already merged — \ + request_merge_config_pr requires an open PR" + ); + } // Fetch the current PR head sha — becomes the "reviewed" sha. + // Submitted together with the approval row (atomic single INSERT) so a + // crash between submit and set_fetched_sha cannot leave a stranded row. 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}"))?; @@ -174,12 +189,9 @@ async fn submit_merge_config_pr( &pr_number.to_string(), description, submitter, + Some(&sha), // atomic: sha inserted with the row, not in a separate UPDATE ) .map_err(|e| anyhow::anyhow!("queue merge_config_pr approval row: {e:#}"))?; - coord - .approvals - .set_fetched_sha(id, &sha) - .map_err(|e| anyhow::anyhow!("persist fetched_sha: {e:#}"))?; let sha_short = sha[..sha.len().min(12)].to_owned(); coord.emit_approval_added(crate::coordinator::ApprovalAdded { id, @@ -249,6 +261,7 @@ pub(crate) fn submit_init_config( // parent); it's also the submitter the approval events route // back to. No declared parent = operator-initiated path. parent.unwrap_or("operator"), + None, // no sha for InitConfig ) .map_err(|e| anyhow::anyhow!("queue approval row: {e:#}"))?; tracing::info!(%id, %name, "init_config approval queued"); @@ -309,6 +322,7 @@ pub(crate) async fn submit_apply_commit( commit_ref, description, submitter, + None, // sha resolved after git_fetch_to_tag below; set via set_fetched_sha ) .map_err(|e| anyhow::anyhow!("queue approval row: {e:#}"))?; let tag = format!("proposal/{id}"); diff --git a/hive-c0re/src/socket_server/schedules.rs b/hive-c0re/src/socket_server/schedules.rs index a8aa0783..92f9416a 100644 --- a/hive-c0re/src/socket_server/schedules.rs +++ b/hive-c0re/src/socket_server/schedules.rs @@ -62,6 +62,7 @@ pub(super) fn handle_request_schedule_prompt( &commit_ref, payload.description.as_deref(), requester, + None, ) { Ok(id) => id, Err(e) => { diff --git a/hive-c0re/src/stores/approvals.rs b/hive-c0re/src/stores/approvals.rs index 338c8f13..cb7e8ce3 100644 --- a/hive-c0re/src/stores/approvals.rs +++ b/hive-c0re/src/stores/approvals.rs @@ -70,6 +70,12 @@ impl Approvals { }) } + /// Insert a new pending approval row. `fetched_sha` may be supplied + /// when the sha is already known at submission time (e.g. `MergeConfigPr` + /// fetches the PR head before inserting), making the insert + sha-set + /// atomic. Pass `None` when the sha is resolved after insertion (e.g. + /// `ApplyCommit`'s `git_fetch_to_tag` step) and call [`set_fetched_sha`] + /// separately. pub fn submit_kind( &self, agent: &str, @@ -77,19 +83,22 @@ impl Approvals { commit_ref: &str, description: Option<&str>, submitter: &str, + fetched_sha: Option<&str>, ) -> Result { let conn = self.conn.lock().unwrap(); conn.execute( "INSERT INTO approvals - (agent, kind, commit_ref, requested_at, status, description, submitter) - VALUES (?1, ?2, ?3, ?4, 'pending', ?5, ?6)", + (agent, kind, commit_ref, requested_at, status, description, submitter, + fetched_sha) + VALUES (?1, ?2, ?3, ?4, 'pending', ?5, ?6, ?7)", params![ agent, kind.as_str(), commit_ref, now_unix(), description, - submitter + submitter, + fetched_sha, ], )?; Ok(conn.last_insert_rowid()) @@ -415,6 +424,7 @@ mod tests { "", Some("scaffold"), "bitburner", + None, ) .expect("submit init_config"); let pending = db @@ -428,11 +438,11 @@ mod tests { #[test] fn mixed_kinds_all_listed() { let (_dir, _path, db) = open_temp(); - db.submit_kind("a", ApprovalKind::ApplyCommit, "deadbeef", None, "a") + db.submit_kind("a", ApprovalKind::ApplyCommit, "deadbeef", None, "a", None) .unwrap(); - db.submit_kind("b", ApprovalKind::Spawn, "", None, "b") + db.submit_kind("b", ApprovalKind::Spawn, "", None, "b", None) .unwrap(); - db.submit_kind("c", ApprovalKind::InitConfig, "", None, "c") + db.submit_kind("c", ApprovalKind::InitConfig, "", None, "c", None) .unwrap(); let pending = db.pending().expect("pending"); assert_eq!(pending.len(), 3, "all three kinds must be visible"); @@ -451,6 +461,7 @@ mod tests { "cafef00d", Some("test"), "bitburner", + None, ) .unwrap(); let row = db.mark_cancelled(id, "manager").expect("cancel"); @@ -470,7 +481,7 @@ mod tests { // final — re-cancelling errors instead of silently overwriting. let (_dir, _path, db) = open_temp(); let id = db - .submit_kind("a", ApprovalKind::Spawn, "deadbeef", None, "a") + .submit_kind("a", ApprovalKind::Spawn, "deadbeef", None, "a", None) .unwrap(); db.mark_cancelled(id, "manager").expect("first cancel"); let err = db @@ -485,7 +496,14 @@ mod tests { // whole list — collect_lenient skips it instead of failing. let (_dir, path, db) = open_temp(); let good = db - .submit_kind("good", ApprovalKind::ApplyCommit, "cafe", None, "good") + .submit_kind( + "good", + ApprovalKind::ApplyCommit, + "cafe", + None, + "good", + None, + ) .unwrap(); let raw = Connection::open(&path).unwrap(); raw.execute( @@ -508,7 +526,14 @@ mod tests { // fall back to the root agent. let (_dir, path, db) = open_temp(); let id = db - .submit_kind("child", ApprovalKind::ApplyCommit, "cafe", None, "parent") + .submit_kind( + "child", + ApprovalKind::ApplyCommit, + "cafe", + None, + "parent", + None, + ) .unwrap(); assert_eq!(db.submitter_of(id).unwrap().as_deref(), Some("parent")); @@ -522,4 +547,25 @@ mod tests { let legacy_id = raw.last_insert_rowid(); assert_eq!(db.submitter_of(legacy_id).unwrap(), None); } + + #[test] + fn fetched_sha_in_insert_is_readable_via_get() { + // `submit_kind` with `Some(sha)` must store it atomically in the + // INSERT — the `get()` row must reflect it without a separate + // `set_fetched_sha` call. This is the MergeConfigPr path. + let (_dir, _path, db) = open_temp(); + let sha = "abc1234567890abc1234567890abc1234567890ab"; + let id = db + .submit_kind( + "janet", + ApprovalKind::MergeConfigPr, + "42", + None, + "ruth", + Some(sha), + ) + .unwrap(); + let row = db.get(id).unwrap().expect("row must exist"); + assert_eq!(row.fetched_sha.as_deref(), Some(sha)); + } }