fix(#2375): pr_is_open state check at submission + atomic fetched_sha INSERT
Two hardening items from argus's review of #2374: 1. PR state check at submission: - Add `pr_is_open(repo, pr)` to forge/pr_merge.rs using `repo_get_pull_request` + `StateType` — early error if the PR is already closed or merged instead of queuing a card that fails later - Call it in `submit_merge_config_pr` before fetching the head sha 2. Atomic fetched_sha INSERT: - Add `fetched_sha: Option<&str>` to `Approvals::submit_kind` so the sha can be included in the INSERT rather than a follow-up UPDATE - MergeConfigPr already knows the sha before inserting the row (pr_head_sha runs first) → pass `Some(&sha)`, drop the separate `set_fetched_sha` call → truly atomic - ApplyCommit still needs two writes (sha resolved by git_fetch_to_tag after the row exists) → pass `None`, `set_fetched_sha` unchanged - All other callers (InitConfig, Spawn, UpdateMetaInputs, SchedulePrompt) pass `None` — no behavioural change - Add `fetched_sha_in_insert_is_readable_via_get` test covering the MergeConfigPr path
This commit is contained in:
parent
d39d05b0b3
commit
96eda4ed6b
7 changed files with 102 additions and 15 deletions
|
|
@ -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");
|
||||
|
|
|
|||
|
|
@ -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,
|
||||
|
|
|
|||
|
|
@ -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<String, ForgeMergeError>
|
|||
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<bool, 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)?;
|
||||
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 {
|
||||
|
|
|
|||
|
|
@ -86,6 +86,7 @@ async fn dispatch(req: &HostRequest, coord: Arc<Coordinator>) -> HostResponse {
|
|||
"",
|
||||
None,
|
||||
"operator",
|
||||
None,
|
||||
)?;
|
||||
tracing::info!(%id, %name, "spawn approval queued");
|
||||
HostResponse::success()
|
||||
|
|
|
|||
|
|
@ -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}");
|
||||
|
|
|
|||
|
|
@ -62,6 +62,7 @@ pub(super) fn handle_request_schedule_prompt(
|
|||
&commit_ref,
|
||||
payload.description.as_deref(),
|
||||
requester,
|
||||
None,
|
||||
) {
|
||||
Ok(id) => id,
|
||||
Err(e) => {
|
||||
|
|
|
|||
|
|
@ -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<i64> {
|
||||
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));
|
||||
}
|
||||
}
|
||||
|
|
|
|||
Loading…
Reference in a new issue