diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 15376b898..644ff43c9 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -4,6 +4,10 @@ on: push: branches: [master] pull_request: + # A merge queue runs the required checks on the queue's temporary merge + # commit before merging; without this event those checks never report and + # every queued PR fails out of the queue. + merge_group: concurrency: group: ${{ github.workflow }}-${{ github.ref }} @@ -25,12 +29,17 @@ jobs: permissions: contents: read outputs: - code: ${{ steps.filter.outputs.code }} + # A merge-group run always builds: the queue's merge commit is exactly + # what must be proven, and the path filter has no PR base to diff + # against there. + code: ${{ github.event_name == 'merge_group' && 'true' || steps.filter.outputs.code }} steps: - - uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7 + - if: github.event_name != 'merge_group' + uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7 with: fetch-depth: 0 - - uses: dorny/paths-filter@ceb8a2b8f2d89434be7ff52d3de7ec3738c5cc9d # v4.0.3 + - if: github.event_name != 'merge_group' + uses: dorny/paths-filter@ceb8a2b8f2d89434be7ff52d3de7ec3738c5cc9d # v4.0.3 id: filter with: # Default predicate-quantifier ('some') matches a filter on ANY diff --git a/src/cli/git.rs b/src/cli/git.rs index 71c0c7dc5..d0202dda0 100644 --- a/src/cli/git.rs +++ b/src/cli/git.rs @@ -1510,7 +1510,7 @@ fn ship_cmd(opts: ShipArgs) { .body .clone() .unwrap_or_else(|| default_pr_body(&repo_root, &base, &head)); - match pulls::create(&client, &repo, &title, &head, &base, Some(&body)) { + match pulls::create(&client, &repo, &title, &head, &base, Some(&body), false) { Ok(pr) => { crate::ui::success(&format!("opened PR #{}: {}", pr.number, pr.html_url)); pr diff --git a/src/github/actions.rs b/src/github/actions.rs index c643a3767..24b2c4073 100644 --- a/src/github/actions.rs +++ b/src/github/actions.rs @@ -95,6 +95,35 @@ pub fn list_check_runs( /// status was still pending or red. One page of 100 covers every realistic /// commit; the combined endpoint already de-duplicates to each context's /// latest state. +/// Posts a Statuses-API status on commit `sha` under `context`. A status is +/// per commit, so a context branch protection requires becomes a merge-time +/// gate on exactly the heads that carry it: `supervisor::merge_approved_pr` +/// marks the head it judged (approval label on, no unresolved CodeRabbit +/// findings) so a repo that requires the context lets GitHub's auto-merge +/// land only judged heads, and a later push -- a new sha without it -- waits +/// for the sweep to judge it. `state` is `success`, `pending`, `failure` or +/// `error`. +pub fn create_commit_status( + client: &Client, + repo: &RepoId, + sha: &str, + state: &str, + context: &str, + description: &str, +) -> Result<(), GitHubError> { + let path = format!("/repos/{}/{}/statuses/{sha}", repo.owner, repo.repo); + client.request( + "POST", + &path, + Some(serde_json::json!({ + "state": state, + "context": context, + "description": description, + })), + )?; + Ok(()) +} + pub fn list_commit_statuses( client: &Client, repo: &RepoId, @@ -241,6 +270,31 @@ mod tests { ); } + #[test] + fn create_commit_status_posts_state_context_and_description() { + let server = MockServer::start(vec![MockResponse::json(201, r#"{"id":1}"#)]); + let client = server.client(Some("tok")); + create_commit_status( + &client, + &repo(), + "abc123", + "success", + "agentflare/judged", + "approval label on, no unresolved findings", + ) + .unwrap(); + let reqs = server.requests(); + assert_eq!(reqs[0].method, "POST"); + assert_eq!(reqs[0].path, "/repos/o/r/statuses/abc123"); + let sent: serde_json::Value = serde_json::from_str(&reqs[0].body).unwrap(); + assert_eq!(sent["state"], "success"); + assert_eq!(sent["context"], "agentflare/judged"); + assert_eq!( + sent["description"], + "approval label on, no unresolved findings" + ); + } + #[test] fn list_commit_statuses_folds_legacy_statuses_into_check_runs() { let server = MockServer::start(vec![MockResponse::json( diff --git a/src/github/client.rs b/src/github/client.rs index b02d31acd..e021f448b 100644 --- a/src/github/client.rs +++ b/src/github/client.rs @@ -2,10 +2,13 @@ //! with no credential, maps status codes to `GitHubError`, and applies //! GitHub's published client etiquette process-wide: a backoff deadline after //! any rate limit (primary or secondary) during which no call is made at all, -//! and serialized, spaced-out mutating requests. +//! serialized, spaced-out mutating requests, and conditional GETs +//! (`If-None-Match` from a per-host ETag cache, so an unchanged resource is +//! a 304 that costs no rate-limit budget instead of a re-download). use crate::github::GitHubError; use crate::github::auth; +use crate::github::etag_cache::{CachedGet, EtagCache}; use std::collections::HashMap; use std::sync::{Arc, Mutex, OnceLock}; use std::time::{Duration, Instant, SystemTime, UNIX_EPOCH}; @@ -41,11 +44,12 @@ const MUTATION_SPACING: Duration = Duration::from_secs(1); /// explicit wait -- GitHub's docs say to wait "at least one minute". const DEFAULT_SECONDARY_BACKOFF: Duration = Duration::from_secs(60); -/// Per-host throttle state, shared by every `Client` in the process aimed at -/// the same base URL. Keyed by host rather than truly global so the test -/// binary's many concurrent mock servers (one per ephemeral port) never -/// throttle each other; production only ever talks to one host, so there it -/// is effectively process-wide. +/// Per-host throttle state and ETag cache, shared by every `Client` in the +/// process aimed at the same base URL. Keyed by host rather than truly +/// global so the test binary's many concurrent mock servers (one per +/// ephemeral port) never throttle each other or see each other's cached +/// responses; production only ever talks to one host, so there it is +/// effectively process-wide. struct HostThrottle { /// No request to this host before this instant (a rate-limit backoff), /// indexed by [`backoff_slot`]: anonymous and authenticated requests @@ -55,6 +59,8 @@ struct HostThrottle { /// Held for the whole of a mutating request, so writes go out one at a /// time; stores when the previous one finished, for spacing. last_mutation: Mutex>, + /// Validators and bodies of past GETs, for conditional requests. + etags: Mutex, } fn host_throttle(base_url: &str) -> Arc { @@ -67,6 +73,7 @@ fn host_throttle(base_url: &str) -> Arc { Arc::new(HostThrottle { backoff_until: [Mutex::new(None), Mutex::new(None)], last_mutation: Mutex::new(None), + etags: Mutex::new(EtagCache::default()), }) })) } @@ -180,6 +187,17 @@ fn backoff_for(status: u16, headers: &RateHeaders, body: &str, now_epoch: u64) - None } +/// A response body as JSON: an empty body (204, or a 200 with nothing to +/// say) is `Null`, anything else must parse. Shared by the fresh and the +/// 304-served paths, so a cached empty body decodes the same way it did the +/// first time. +fn decode_body(text: &str) -> Result { + if text.trim().is_empty() { + return Ok(serde_json::Value::Null); + } + serde_json::from_str(text).map_err(|e| GitHubError::Parse(e.to_string())) +} + fn now_epoch() -> u64 { SystemTime::now() .duration_since(UNIX_EPOCH) @@ -420,22 +438,58 @@ impl Client { if let Some(tok) = token { req = req.set("Authorization", &format!("Bearer {tok}")); } + // Conditional GET: the cache is keyed by representation as well as + // URL, since `Accept` selects different bodies for the same path. + // The cached entry is held here for the length of the exchange, so + // a concurrent eviction can't leave a 304 with nothing to serve. + let cache_key = (method == "GET").then(|| format!("{accept} {url}")); + let cached: Option = cache_key.as_deref().and_then(|key| { + self.throttle + .etags + .lock() + .unwrap_or_else(|e| e.into_inner()) + .get(key) + .cloned() + }); + if let Some(c) = &cached { + req = req.set("If-None-Match", &c.etag); + } let result = match body { Some(b) => req.send_json(b), None => req.call(), }; match result { + Ok(resp) if resp.status() == 304 => { + // Unchanged since the validator we sent: GitHub charged no + // rate-limit budget for this, and the body we kept is current. + let Some(c) = cached else { + return Err(GitHubError::Transport( + "GitHub answered 304 to a request that sent no validator".to_string(), + )); + }; + let headers = RateHeaders::from_response(&resp); + decode_body(&c.body).map(|json| (json, headers)) + } Ok(resp) => { + let etag = resp.header("etag").map(str::to_string); let headers = RateHeaders::from_response(&resp); let text = resp .into_string() .map_err(|e| GitHubError::Transport(e.to_string()))?; - if text.trim().is_empty() { - return Ok((serde_json::Value::Null, headers)); + if let Some(key) = cache_key.as_deref() { + let mut etags = self + .throttle + .etags + .lock() + .unwrap_or_else(|e| e.into_inner()); + match etag { + Some(etag) => etags.insert(key, &etag, &text), + // No validator on this response: whatever we held + // for the URL is stale now. + None => etags.remove(key), + } } - serde_json::from_str(&text) - .map(|json| (json, headers)) - .map_err(|e| GitHubError::Parse(e.to_string())) + decode_body(&text).map(|json| (json, headers)) } Err(ureq::Error::Status(code, resp)) => { let headers = RateHeaders::from_response(&resp); @@ -683,6 +737,135 @@ mod tests { ); } + #[test] + fn a_repeated_get_sends_if_none_match_and_serves_the_cached_body_on_304() { + let server = MockServer::start(vec![ + MockResponse::json(200, r#"{"a":1}"#).with_header("ETag", "W/\"v1\""), + MockResponse::json(304, ""), + MockResponse::json(200, r#"{"a":2}"#).with_header("ETag", "W/\"v2\""), + MockResponse::json(304, ""), + ]); + let client = server.client(Some("tok")); + assert_eq!(client.request("GET", "/thing", None).unwrap()["a"], 1); + // Unchanged: the server says 304 and the first body is served again. + assert_eq!(client.request("GET", "/thing", None).unwrap()["a"], 1); + // Changed: a fresh body replaces the cached one and its validator. + assert_eq!(client.request("GET", "/thing", None).unwrap()["a"], 2); + assert_eq!(client.request("GET", "/thing", None).unwrap()["a"], 2); + + let reqs = server.requests(); + assert_eq!(reqs.len(), 4); + assert_eq!(reqs[0].header("if-none-match"), None); + assert_eq!(reqs[1].header("if-none-match"), Some("W/\"v1\"")); + assert_eq!(reqs[2].header("if-none-match"), Some("W/\"v1\"")); + assert_eq!(reqs[3].header("if-none-match"), Some("W/\"v2\"")); + } + + #[test] + fn a_304_for_a_cached_empty_body_decodes_to_null_like_the_first_time() { + let server = MockServer::start(vec![ + MockResponse::json(200, "").with_header("ETag", "\"empty\""), + MockResponse::json(304, ""), + ]); + let client = server.client(Some("tok")); + assert_eq!( + client.request("GET", "/nothing", None).unwrap(), + serde_json::Value::Null + ); + assert_eq!( + client.request("GET", "/nothing", None).unwrap(), + serde_json::Value::Null + ); + assert_eq!( + server.requests()[1].header("if-none-match"), + Some("\"empty\"") + ); + } + + #[test] + fn the_etag_cache_is_keyed_by_representation_and_only_used_for_gets() { + let server = MockServer::start(vec![ + MockResponse::json(200, r#"{"a":1}"#).with_header("ETag", "\"v1\""), + MockResponse::json(200, r#"{"a":1,"body_html":"x"}"#).with_header("ETag", "\"h1\""), + MockResponse::json(201, r#"{"a":1}"#).with_header("ETag", "\"p1\""), + MockResponse::json(304, ""), + ]); + let client = server.client(Some("tok")); + client.request("GET", "/thing", None).unwrap(); + client + .request_with_accept("GET", "/thing", None, "application/vnd.github.html+json") + .unwrap(); + client + .request("POST", "/thing", Some(serde_json::json!({}))) + .unwrap(); + let again = client.request("GET", "/thing", None).unwrap(); + assert_eq!(again["a"], 1); + assert!( + again.get("body_html").is_none(), + "served the json+json body" + ); + + let reqs = server.requests(); + assert_eq!( + reqs[1].header("if-none-match"), + None, + "other Accept, other key" + ); + assert_eq!( + reqs[2].header("if-none-match"), + None, + "writes are never conditional" + ); + assert_eq!(reqs[3].header("if-none-match"), Some("\"v1\"")); + } + + #[test] + fn a_response_without_an_etag_evicts_the_stale_entry() { + let server = MockServer::start(vec![ + MockResponse::json(200, r#"{"a":1}"#).with_header("ETag", "\"v1\""), + MockResponse::json(200, r#"{"a":2}"#), + MockResponse::json(200, r#"{"a":3}"#), + ]); + let client = server.client(Some("tok")); + client.request("GET", "/thing", None).unwrap(); + assert_eq!(client.request("GET", "/thing", None).unwrap()["a"], 2); + assert_eq!(client.request("GET", "/thing", None).unwrap()["a"], 3); + let reqs = server.requests(); + assert_eq!(reqs[1].header("if-none-match"), Some("\"v1\"")); + assert_eq!( + reqs[2].header("if-none-match"), + None, + "the validator went with the body it validated" + ); + } + + #[test] + fn a_rate_limit_backoff_still_refuses_a_get_that_has_a_cached_copy() { + // A cached body is only known current after the server confirms it + // with a 304, so a backoff must still keep the call off the network + // rather than serve the stale copy as if it were fresh. + let server = MockServer::start(vec![ + MockResponse::json(200, r#"{"a":1}"#).with_header("ETag", "\"v1\""), + MockResponse::json(403, r#"{"message":"secondary rate limit"}"#) + .with_header("retry-after", "60"), + ]); + let client = server.client(Some("tok")); + client.request("GET", "/thing", None).unwrap(); + assert!(matches!( + client.request("GET", "/other", None).unwrap_err(), + GitHubError::RateLimited(_) + )); + assert!(matches!( + client.request("GET", "/thing", None).unwrap_err(), + GitHubError::RateLimited(_) + )); + assert_eq!( + server.requests().len(), + 2, + "the backoff stopped the third call locally" + ); + } + #[test] fn an_anonymous_rate_limit_does_not_back_off_authenticated_calls() { let server = MockServer::start(vec![ diff --git a/src/github/etag_cache.rs b/src/github/etag_cache.rs new file mode 100644 index 000000000..79ecd1280 --- /dev/null +++ b/src/github/etag_cache.rs @@ -0,0 +1,139 @@ +//! A bounded, per-process cache of GET responses keyed by URL, holding each +//! response's `ETag` and body so `Client` can send `If-None-Match` and +//! serve the cached body when GitHub answers 304. A 304 costs no primary +//! rate-limit budget, which is what makes polling the same PRs and issues +//! every sweep tick affordable: an unchanged resource is a header exchange, +//! not a re-download that counts against the 5,000/hour allowance. +//! +//! Bounded two ways: a fixed number of entries, evicted oldest-first, and a +//! cap on the size of any one cached body, so a single huge list page can't +//! pin megabytes for the life of the daemon. + +use std::collections::{HashMap, VecDeque}; + +/// Entries kept per host. A sweep over a few hundred PRs across a dozen +/// projects touches a few hundred distinct URLs; this leaves room for the +/// bridge's issue polling on top without growing without bound. +pub(crate) const MAX_ENTRIES: usize = 1024; + +/// Bodies above this are not cached at all -- a 100-item list page of PRs +/// with full bodies is well under it, and anything bigger is rare enough +/// that re-downloading it beats holding it. +pub(crate) const MAX_BODY_BYTES: usize = 512 * 1024; + +/// One cached GET: the validator to send back, and the body it stands for. +#[derive(Debug, Clone, PartialEq, Eq)] +pub(crate) struct CachedGet { + pub etag: String, + pub body: String, +} + +#[derive(Debug, Default)] +pub(crate) struct EtagCache { + entries: HashMap, + /// Insertion order, for oldest-first eviction. A re-inserted key keeps + /// its original position: an entry that keeps changing is not one worth + /// keeping longer, and a plain FIFO is enough for a cache whose only job + /// is to stay bounded. + order: VecDeque, +} + +impl EtagCache { + /// The cached response for `key`, if one is held. + pub(crate) fn get(&self, key: &str) -> Option<&CachedGet> { + self.entries.get(key) + } + + /// Stores `body` under `key` with its validator, or drops any existing + /// entry for `key` when the body is too big to keep -- a stale validator + /// must never outlive the body it validates. + pub(crate) fn insert(&mut self, key: &str, etag: &str, body: &str) { + if body.len() > MAX_BODY_BYTES { + self.remove(key); + return; + } + if self.entries.contains_key(key) { + // Keep its slot in `order`; only the payload changes. + } else { + self.order.push_back(key.to_string()); + while self.order.len() > MAX_ENTRIES { + if let Some(oldest) = self.order.pop_front() { + self.entries.remove(&oldest); + } + } + } + self.entries.insert( + key.to_string(), + CachedGet { + etag: etag.to_string(), + body: body.to_string(), + }, + ); + } + + /// Drops `key`'s entry and its eviction slot; a no-op for an absent key. + pub(crate) fn remove(&mut self, key: &str) { + if self.entries.remove(key).is_some() { + self.order.retain(|k| k != key); + } + } + + /// Number of cached responses. + #[cfg(test)] + pub(crate) fn len(&self) -> usize { + self.entries.len() + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn insert_then_get_round_trips_and_replaces_in_place() { + let mut cache = EtagCache::default(); + cache.insert("k", "\"v1\"", "{\"a\":1}"); + assert_eq!(cache.get("k").unwrap().etag, "\"v1\""); + cache.insert("k", "\"v2\"", "{\"a\":2}"); + assert_eq!(cache.get("k").unwrap().body, "{\"a\":2}"); + assert_eq!(cache.len(), 1); + assert_eq!(cache.order.len(), 1, "a replaced key keeps one slot"); + } + + #[test] + fn the_oldest_entry_is_evicted_past_the_entry_cap() { + let mut cache = EtagCache::default(); + for i in 0..=MAX_ENTRIES { + cache.insert(&format!("k{i}"), "\"e\"", "{}"); + } + assert_eq!(cache.len(), MAX_ENTRIES); + assert!(cache.get("k0").is_none(), "the first entry goes first"); + assert!(cache.get(&format!("k{MAX_ENTRIES}")).is_some()); + } + + #[test] + fn an_oversized_body_is_not_cached_and_drops_a_stale_entry() { + let mut cache = EtagCache::default(); + cache.insert("k", "\"v1\"", "{}"); + let huge = "x".repeat(MAX_BODY_BYTES + 1); + cache.insert("k", "\"v2\"", &huge); + assert!( + cache.get("k").is_none(), + "a validator without its body would make a 304 unservable" + ); + assert!(cache.order.is_empty()); + } + + #[test] + fn remove_drops_the_entry_and_its_slot() { + let mut cache = EtagCache::default(); + cache.insert("a", "\"1\"", "{}"); + cache.insert("b", "\"2\"", "{}"); + cache.remove("a"); + assert!(cache.get("a").is_none()); + assert_eq!(cache.order, VecDeque::from(vec!["b".to_string()])); + // Removing an absent key is a no-op. + cache.remove("zzz"); + assert_eq!(cache.len(), 1); + } +} diff --git a/src/github/graphql.rs b/src/github/graphql.rs index 503cc3fd5..15cbf4a86 100644 --- a/src/github/graphql.rs +++ b/src/github/graphql.rs @@ -56,6 +56,24 @@ pub struct BatchPrData { /// every context on the head commit -- consulted when the context list /// itself came back empty. pub rollup_state: Option, + /// The PR's GraphQL node id: what the `enablePullRequestAutoMerge` and + /// `markPullRequestReadyForReview` mutations take as `pullRequestId`. + pub node_id: Option, + /// `autoMergeRequest` is non-null while GitHub's native auto-merge is + /// armed on the PR. + pub auto_merge_enabled: bool, + /// `isMergeQueueEnabled`: the base branch merges through a merge queue, + /// so a direct merge is refused and auto-merge is the way to enqueue. + pub merge_queue_enabled: bool, + /// `isInMergeQueue`: already enqueued; the queue's own CI decides now. + pub in_merge_queue: bool, + /// `isDraft`: still a draft, so it can't merge and nobody was asked to + /// review it -- agentflare opens every PR as one and is expected to have + /// flipped it by the time the item is in review. + pub is_draft: bool, + /// `baseRefName`: the branch this PR merges into, whose protection + /// decides whether arming auto-merge is safe. + pub base_ref: Option, } /// One aliased sub-query for PR `number` -- `pr` is a valid GraphQL @@ -68,8 +86,9 @@ fn pr_alias(number: u64) -> String { fn pr_subquery(number: u64) -> String { format!( - "{}: pullRequest(number: {number}) {{ state merged mergeable mergeStateStatus \ - reviewDecision headRefOid \ + "{}: pullRequest(number: {number}) {{ id state merged mergeable mergeStateStatus \ + isDraft reviewDecision headRefOid baseRefName \ + isMergeQueueEnabled isInMergeQueue autoMergeRequest {{ enabledAt }} \ labels(first: 20) {{ nodes {{ name }} }} \ commits(last: 1) {{ nodes {{ commit {{ statusCheckRollup {{ state contexts(first: 100) {{ \ nodes {{ __typename \ @@ -209,9 +228,86 @@ fn parse_batch_pr(node: &serde_json::Value) -> BatchPrData { head_sha: string(&node["headRefOid"]), review_decision: string(&node["reviewDecision"]), rollup_state: string(&rollup["state"]), + node_id: string(&node["id"]), + auto_merge_enabled: node["autoMergeRequest"].is_object(), + merge_queue_enabled: node["isMergeQueueEnabled"].as_bool().unwrap_or(false), + in_merge_queue: node["isInMergeQueue"].as_bool().unwrap_or(false), + is_draft: node["isDraft"].as_bool().unwrap_or(false), + base_ref: string(&node["baseRefName"]), } } +/// Flips a draft PR to "ready for review" -- the moment reviewers get asked +/// and a merge becomes possible. Idempotent on GitHub's side: an already +/// ready PR answers with an error naming that, which callers treat as done. +pub fn mark_ready_for_review(client: &Client, pr_node_id: &str) -> Result<(), GitHubError> { + mutate( + client, + "mutation($input: MarkPullRequestReadyForReviewInput!) { \ + markPullRequestReadyForReview(input: $input) { pullRequest { isDraft } } }", + serde_json::json!({ "input": { "pullRequestId": pr_node_id } }), + )?; + Ok(()) +} + +/// Runs one GraphQL mutation and returns its `data`, mapping a top-level +/// `errors` array to `GitHubError` the same way `batch_pr_status` does. +fn mutate( + client: &Client, + query: &str, + variables: serde_json::Value, +) -> Result { + let body = serde_json::json!({ "query": query, "variables": variables }); + let json = client.graphql(body)?; + if let Some(errors) = json.get("errors") { + return Err(graphql_error(client, errors)); + } + Ok(json.get("data").cloned().unwrap_or(serde_json::Value::Null)) +} + +/// Arms GitHub's native auto-merge on the PR with node id `pr_node_id`: +/// GitHub then merges it itself (or adds it to the merge queue, on a base +/// branch that has one) the moment every requirement -- required checks, +/// required reviews -- is satisfied. `merge_method` is GraphQL's enum +/// spelling (`SQUASH`/`MERGE`/`REBASE`); `expected_head_oid` pins the request +/// to the head the caller judged, so GitHub refuses it if the PR moved. GitHub +/// reports a PR that could be merged right now (`CLEAN`, no merge queue) as an +/// error rather than arming it, and a repository with auto-merge switched off +/// likewise -- both come back as `Err`, and the caller falls back to a direct +/// merge. +pub fn enable_auto_merge( + client: &Client, + pr_node_id: &str, + merge_method: &str, + expected_head_oid: Option<&str>, +) -> Result<(), GitHubError> { + let mut input = serde_json::json!({ "pullRequestId": pr_node_id, "mergeMethod": merge_method }); + if let Some(oid) = expected_head_oid { + input["expectedHeadOid"] = serde_json::Value::String(oid.to_string()); + } + mutate( + client, + "mutation($input: EnablePullRequestAutoMergeInput!) { \ + enablePullRequestAutoMerge(input: $input) { \ + pullRequest { autoMergeRequest { enabledAt } } } }", + serde_json::json!({ "input": input }), + )?; + Ok(()) +} + +/// Disarms a previously enabled auto-merge -- used when a condition that +/// gated arming it (no unresolved CodeRabbit findings, the approval label) +/// stops holding, so GitHub can't merge on the next approving review. +pub fn disable_auto_merge(client: &Client, pr_node_id: &str) -> Result<(), GitHubError> { + mutate( + client, + "mutation($input: DisablePullRequestAutoMergeInput!) { \ + disablePullRequestAutoMerge(input: $input) { pullRequest { number } } }", + serde_json::json!({ "input": { "pullRequestId": pr_node_id } }), + )?; + Ok(()) +} + /// One `statusCheckRollup` context: a Checks-API `CheckRun`, or a legacy /// Statuses-API `StatusContext` (third-party CI, CLA bots) -- both gate a /// merge the same way, so both count. @@ -347,6 +443,7 @@ mod tests { for field in [ "headRefOid", "reviewDecision", + "isMergeQueueEnabled isInMergeQueue autoMergeRequest { enabledAt }", "statusCheckRollup { state", "... on StatusContext { context state isRequired(pullRequestNumber: 8) }", "isRequired(pullRequestNumber: 8)", @@ -355,6 +452,126 @@ mod tests { } } + #[test] + fn batch_pr_status_reads_node_id_auto_merge_and_merge_queue_flags() { + let server = MockServer::start(vec![MockResponse::json( + 200, + r#"{"data":{"repository":{ + "pr8":{"id":"PR_kwDOAbc","merged":false,"mergeable":"MERGEABLE","mergeStateStatus":"BLOCKED", + "baseRefName":"main","isMergeQueueEnabled":true,"isInMergeQueue":false, + "autoMergeRequest":{"enabledAt":"2026-09-24T00:00:00Z"}, + "labels":{"nodes":[]},"commits":{"nodes":[]}}, + "pr9":{"id":"PR_kwDOAbd","merged":false,"mergeable":"MERGEABLE","mergeStateStatus":"CLEAN", + "autoMergeRequest":null, + "labels":{"nodes":[]},"commits":{"nodes":[]}} + }}}"#, + )]); + let client = server.client(Some("tok")); + let out = batch_pr_status(&client, &repo(), &[8, 9]).unwrap(); + assert_eq!(out[&8].node_id.as_deref(), Some("PR_kwDOAbc")); + assert!(out[&8].auto_merge_enabled); + assert!(out[&8].merge_queue_enabled); + assert!(!out[&8].in_merge_queue); + assert!(!out[&9].auto_merge_enabled); + assert!(!out[&9].merge_queue_enabled); + assert_eq!(out[&8].base_ref.as_deref(), Some("main")); + assert_eq!(out[&9].base_ref, None); + let sent: serde_json::Value = serde_json::from_str(&server.requests()[0].body).unwrap(); + assert!(sent["query"].as_str().unwrap().contains(" baseRefName ")); + assert!( + sent["query"] + .as_str() + .unwrap() + .contains("pullRequest(number: 8) { id ") + ); + } + + #[test] + fn enable_auto_merge_sends_the_mutation_pinned_to_the_expected_head() { + let server = MockServer::start(vec![MockResponse::json( + 200, + r#"{"data":{"enablePullRequestAutoMerge":{"pullRequest":{"autoMergeRequest":{"enabledAt":"x"}}}}}"#, + )]); + let client = server.client(Some("tok")); + enable_auto_merge(&client, "PR_1", "SQUASH", Some("abc123")).unwrap(); + let reqs = server.requests(); + assert_eq!(reqs[0].path, "/graphql"); + let sent: serde_json::Value = serde_json::from_str(&reqs[0].body).unwrap(); + assert!( + sent["query"] + .as_str() + .unwrap() + .contains("enablePullRequestAutoMerge(input: $input)") + ); + assert_eq!(sent["variables"]["input"]["pullRequestId"], "PR_1"); + assert_eq!(sent["variables"]["input"]["mergeMethod"], "SQUASH"); + assert_eq!(sent["variables"]["input"]["expectedHeadOid"], "abc123"); + } + + #[test] + fn enable_auto_merge_surfaces_a_clean_status_refusal_as_an_error() { + // GitHub refuses to arm auto-merge on a PR it could merge right now; + // the caller falls back to a direct merge on this error. + let server = MockServer::start(vec![MockResponse::json( + 200, + r#"{"data":{"enablePullRequestAutoMerge":null},"errors":[{"message":"Pull request is in clean status"}]}"#, + )]); + let client = server.client(Some("tok")); + let err = enable_auto_merge(&client, "PR_1", "SQUASH", None).unwrap_err(); + assert!(err.to_string().contains("clean status"), "{err}"); + } + + #[test] + fn batch_pr_status_reads_is_draft() { + let server = MockServer::start(vec![MockResponse::json( + 200, + r#"{"data":{"repository":{ + "pr3":{"isDraft":true,"merged":false,"labels":{"nodes":[]},"commits":{"nodes":[]}} + }}}"#, + )]); + let client = server.client(Some("tok")); + let out = batch_pr_status(&client, &repo(), &[3]).unwrap(); + assert!(out[&3].is_draft); + let sent: serde_json::Value = serde_json::from_str(&server.requests()[0].body).unwrap(); + assert!(sent["query"].as_str().unwrap().contains(" isDraft ")); + } + + #[test] + fn mark_ready_for_review_sends_the_mutation() { + let server = MockServer::start(vec![MockResponse::json( + 200, + r#"{"data":{"markPullRequestReadyForReview":{"pullRequest":{"isDraft":false}}}}"#, + )]); + let client = server.client(Some("tok")); + mark_ready_for_review(&client, "PR_1").unwrap(); + let sent: serde_json::Value = serde_json::from_str(&server.requests()[0].body).unwrap(); + assert!( + sent["query"] + .as_str() + .unwrap() + .contains("markPullRequestReadyForReview(input: $input)") + ); + assert_eq!(sent["variables"]["input"]["pullRequestId"], "PR_1"); + } + + #[test] + fn disable_auto_merge_sends_the_mutation() { + let server = MockServer::start(vec![MockResponse::json( + 200, + r#"{"data":{"disablePullRequestAutoMerge":{"pullRequest":{"number":1}}}}"#, + )]); + let client = server.client(Some("tok")); + disable_auto_merge(&client, "PR_1").unwrap(); + let sent: serde_json::Value = serde_json::from_str(&server.requests()[0].body).unwrap(); + assert!( + sent["query"] + .as_str() + .unwrap() + .contains("disablePullRequestAutoMerge") + ); + assert_eq!(sent["variables"]["input"]["pullRequestId"], "PR_1"); + } + #[test] fn batch_pr_status_maps_graphql_rate_limited_errors_to_rate_limited() { let server = MockServer::start(vec![MockResponse::json( diff --git a/src/github/mcp.rs b/src/github/mcp.rs index 40d4f3ff1..72ba61796 100644 --- a/src/github/mcp.rs +++ b/src/github/mcp.rs @@ -92,6 +92,9 @@ pub fn pr_status_json( "dels": pr.deletions, "files": pr.changed_files, "head": pr.head.as_ref().map(|h| &h.git_ref), + // What `pr_merge` pins to: a caller that judged CI on this snapshot + // passes it back so a commit pushed in between can't ride along. + "head_sha": pr.head.as_ref().map(|h| &h.sha), "base": pr.base.as_ref().map(|h| &h.git_ref), "url": pr.html_url, "checks_ok": checks_ok, diff --git a/src/github/mod.rs b/src/github/mod.rs index cd15da18b..c7e89bc52 100644 --- a/src/github/mod.rs +++ b/src/github/mod.rs @@ -8,6 +8,7 @@ pub mod auth; pub mod bridge; pub mod client; pub mod contents; +pub(crate) mod etag_cache; pub mod graphql; pub mod identity; pub mod init_auth; diff --git a/src/github/models.rs b/src/github/models.rs index 16e24a72b..e82b2a261 100644 --- a/src/github/models.rs +++ b/src/github/models.rs @@ -36,6 +36,14 @@ pub struct PullRequest { pub body: Option, #[serde(default)] pub draft: bool, + // The GraphQL node id, what the auto-merge and ready-for-review + // mutations address the PR by; REST returns it on every PR object. + #[serde(default)] + pub node_id: Option, + // Non-null while GitHub's native auto-merge is armed on the PR (an + // object naming who enabled it and with which method). + #[serde(default)] + pub auto_merge: Option, // Present (non-null) on both the list and single-PR endpoints, unlike // `merged: bool` which the GitHub API only returns from the single-PR // endpoint — using this instead lets `check_merge` (item #420) read diff --git a/src/github/pulls.rs b/src/github/pulls.rs index 17e939bf7..660c0fac4 100644 --- a/src/github/pulls.rs +++ b/src/github/pulls.rs @@ -10,14 +10,26 @@ fn search_items(page: &serde_json::Value) -> Vec { .unwrap_or_default() } -fn create_body(title: &str, head: &str, base: &str, body: Option<&str>) -> serde_json::Value { +fn create_body( + title: &str, + head: &str, + base: &str, + body: Option<&str>, + draft: bool, +) -> serde_json::Value { let mut v = serde_json::json!({ "title": title, "head": head, "base": base }); if let Some(b) = body { v["body"] = serde_json::Value::String(b.to_string()); } + if draft { + v["draft"] = serde_json::Value::Bool(true); + } v } +/// Opens a PR. `draft` opens it as a draft -- CI runs, but reviewers aren't +/// requested and it can't merge until `graphql::mark_ready_for_review` +/// flips it; see [`drafts_unsupported`] for the one repo type that refuses. pub fn create( client: &Client, repo: &RepoId, @@ -25,12 +37,30 @@ pub fn create( head: &str, base: &str, body: Option<&str>, + draft: bool, ) -> Result { let path = format!("/repos/{}/{}/pulls", repo.owner, repo.repo); - let json = client.request("POST", &path, Some(create_body(title, head, base, body)))?; + let json = client.request( + "POST", + &path, + Some(create_body(title, head, base, body, draft)), + )?; serde_json::from_value(json).map_err(|e| GitHubError::Parse(e.to_string())) } +/// True when `create` with `draft: true` failed because the repository +/// can't have draft PRs at all (private repos on plans without them): +/// GitHub answers 422 naming draft pull requests. The caller retries as an +/// ordinary PR; any other failure is what it is. +pub fn drafts_unsupported(err: &GitHubError) -> bool { + match err { + GitHubError::Http { status: 422, body } => { + body.to_lowercase().contains("draft pull request") + } + _ => false, + } +} + pub fn list(client: &Client, repo: &RepoId, state: &str) -> Result, GitHubError> { let path = format!( "/repos/{}/{}/pulls?state={}", @@ -213,11 +243,8 @@ pub fn get(client: &Client, repo: &RepoId, number: u64) -> Result Result<(), GitHubError> { - merge_at_head(client, repo, number, method, None) -} - -/// `merge`, pinned to `head_sha` when given: GitHub then refuses with 409 +/// Merges PR `number`, pinned to `head_sha` when given: GitHub then refuses +/// with 409 /// if the PR's head has moved since -- a commit pushed after the caller /// judged CI green must never ride along into the merge unchecked. Callers /// treat that 409 as "look again next tick" (see [`is_head_moved`]). @@ -359,11 +386,33 @@ mod tests { #[test] fn create_body_includes_optional_body_only_when_present() { - let with = create_body("t", "h", "b", Some("desc")); + let with = create_body("t", "h", "b", Some("desc"), false); assert_eq!(with["title"], "t"); assert_eq!(with["body"], "desc"); - let without = create_body("t", "h", "b", None); + assert!(without_draft_key(&with), "draft is only sent when set"); + let without = create_body("t", "h", "b", None, true); assert!(without.get("body").is_none()); + assert_eq!(without["draft"], true); + } + + fn without_draft_key(v: &serde_json::Value) -> bool { + v.get("draft").is_none() + } + + #[test] + fn drafts_unsupported_matches_githubs_422_only() { + let refused = GitHubError::Http { + status: 422, + body: r#"{"message":"Draft pull requests are not supported in this repository."}"# + .into(), + }; + assert!(drafts_unsupported(&refused)); + let other_422 = GitHubError::Http { + status: 422, + body: r#"{"message":"A pull request already exists for o:task/7."}"#.into(), + }; + assert!(!drafts_unsupported(&other_422)); + assert!(!drafts_unsupported(&GitHubError::NotFound)); } fn repo() -> RepoId { @@ -380,7 +429,7 @@ mod tests { r#"{"number":7,"html_url":"https://gh/o/r/pull/7","state":"open","title":"t"}"#, )]); let client = server.client(Some("tok")); - let pr = create(&client, &repo(), "t", "head", "main", Some("desc")).unwrap(); + let pr = create(&client, &repo(), "t", "head", "main", Some("desc"), true).unwrap(); assert_eq!(pr.number, 7); let reqs = server.requests(); @@ -390,6 +439,7 @@ mod tests { assert_eq!(sent["head"], "head"); assert_eq!(sent["base"], "main"); assert_eq!(sent["body"], "desc"); + assert_eq!(sent["draft"], true); } #[test] @@ -631,10 +681,10 @@ mod tests { } #[test] - fn merge_puts_the_chosen_method() { + fn merge_at_head_puts_the_chosen_method() { let server = MockServer::start(vec![MockResponse::json(200, r#"{"merged":true}"#)]); let client = server.client(Some("tok")); - merge(&client, &repo(), 3, "squash").unwrap(); + merge_at_head(&client, &repo(), 3, "squash", None).unwrap(); let reqs = server.requests(); assert_eq!(reqs[0].method, "PUT"); assert_eq!(reqs[0].path, "/repos/o/r/pulls/3/merge"); diff --git a/src/github/repos.rs b/src/github/repos.rs index 3155b1c8b..074988688 100644 --- a/src/github/repos.rs +++ b/src/github/repos.rs @@ -5,7 +5,8 @@ use crate::github::{Client, GitHubError, RepoId}; use std::collections::HashMap; use std::sync::{Mutex, OnceLock}; -/// The slice of `GET /repos/{owner}/{repo}` post-merge cleanup consults. +/// The slice of `GET /repos/{owner}/{repo}` the merge path and post-merge +/// cleanup consult. #[derive(Debug, Clone, PartialEq, Eq)] pub struct RepoSettings { pub default_branch: String, @@ -15,6 +16,73 @@ pub struct RepoSettings { /// The repository's GraphQL node id (`node_id`), which the atomic /// `updateRefs` branch deletion addresses the repo by. pub node_id: Option, + /// "Allow auto-merge" in the repo's settings: whether + /// `enablePullRequestAutoMerge` can be used at all. + pub allow_auto_merge: bool, + pub allow_squash_merge: bool, + pub allow_merge_commit: bool, + pub allow_rebase_merge: bool, +} + +/// A PR merge method in both spellings GitHub uses: REST's `merge_method` +/// (`squash`) and GraphQL's `PullRequestMergeMethod` enum (`SQUASH`). +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum MergeMethod { + Squash, + Merge, + Rebase, +} + +impl MergeMethod { + /// REST's `merge_method` value. + pub fn rest(self) -> &'static str { + match self { + MergeMethod::Squash => "squash", + MergeMethod::Merge => "merge", + MergeMethod::Rebase => "rebase", + } + } + + /// GraphQL's `PullRequestMergeMethod` enum value. + pub fn graphql(self) -> &'static str { + match self { + MergeMethod::Squash => "SQUASH", + MergeMethod::Merge => "MERGE", + MergeMethod::Rebase => "REBASE", + } + } +} + +impl RepoSettings { + /// What GitHub falls back to when it can't read a repo's settings: every + /// merge method allowed (GitHub's defaults), auto-merge off. Used when + /// the settings fetch itself fails, so a merge can still be attempted. + pub fn unknown(default_branch: &str) -> RepoSettings { + RepoSettings { + default_branch: default_branch.to_string(), + delete_branch_on_merge: false, + node_id: None, + allow_auto_merge: false, + allow_squash_merge: true, + allow_merge_commit: true, + allow_rebase_merge: true, + } + } + + /// The merge method the supervisor uses for this repo: squash when the + /// repo allows it (one commit per item, matching agentflare's own + /// convention), else a merge commit, else rebase. A repo that allows + /// none -- possible when the fetched flags are all false -- still gets + /// squash, so the attempt is made and GitHub says why it can't. + pub fn merge_method(&self) -> MergeMethod { + if self.allow_squash_merge || (!self.allow_merge_commit && !self.allow_rebase_merge) { + MergeMethod::Squash + } else if self.allow_merge_commit { + MergeMethod::Merge + } else { + MergeMethod::Rebase + } + } } /// `repo`'s settings, fetched once per process per host+repo: they change @@ -34,13 +102,21 @@ pub fn settings(client: &Client, repo: &RepoId) -> Result Vec { + static CACHE: OnceLock>>> = OnceLock::new(); + let key = format!( + "{} {}/{} {branch}", + client.host_key(), + repo.owner, + repo.repo + ); + let cache = CACHE.get_or_init(|| Mutex::new(HashMap::new())); + if let Some(hit) = cache + .lock() + .unwrap_or_else(|e| e.into_inner()) + .get(&key) + .cloned() + { + return hit; + } + let encoded = crate::github::encode_query(branch); + let mut contexts: Vec = Vec::new(); + let mut complete = true; + let branch_path = format!("/repos/{}/{}/branches/{encoded}", repo.owner, repo.repo); + match client.request("GET", &branch_path, None) { + Ok(json) => { + let required = &json["protection"]["required_status_checks"]; + let named = required["contexts"] + .as_array() + .into_iter() + .flatten() + .filter_map(|c| c.as_str()) + .chain( + required["checks"] + .as_array() + .into_iter() + .flatten() + .filter_map(|c| c["context"].as_str()), + ); + contexts.extend(named.map(str::to_string)); + } + Err(e) => { + complete = false; + eprintln!( + "github: could not read branch protection of {repo}@{branch}: {}", + e.log_safe() + ); + } + } + let rules_path = format!( + "/repos/{}/{}/rules/branches/{encoded}", + repo.owner, repo.repo + ); + match client.request("GET", &rules_path, None) { + Ok(json) => { + let from_rules = json + .as_array() + .into_iter() + .flatten() + .filter(|rule| rule["type"] == "required_status_checks") + .flat_map(|rule| { + rule["parameters"]["required_status_checks"] + .as_array() + .cloned() + .unwrap_or_default() + }) + .filter_map(|check| check["context"].as_str().map(str::to_string)); + contexts.extend(from_rules); + } + // No rulesets, or an endpoint this host doesn't have. + Err(GitHubError::NotFound) => {} + Err(e) => { + complete = false; + eprintln!( + "github: could not read rulesets of {repo}@{branch}: {}", + e.log_safe() + ); + } + } + contexts.sort(); + contexts.dedup(); + if complete { + cache + .lock() + .unwrap_or_else(|e| e.into_inner()) + .insert(key, contexts.clone()); + } + contexts +} + /// Fetches `repo`'s default branch via the GitHub API. Used when an explicit /// `repo` override is given, since there's no local checkout to read it from. pub fn get_default_branch(client: &Client, repo: &RepoId) -> Result { @@ -231,6 +409,75 @@ mod tests { } } + #[test] + fn required_status_contexts_merges_branch_protection_and_rulesets() { + let server = MockServer::start(vec![ + MockResponse::json( + 200, + r#"{"name":"main","protected":true,"protection":{"required_status_checks":{ + "contexts":["CI Green"], + "checks":[{"context":"CI Green","app_id":15368},{"context":"agentflare/judged","app_id":null}] + }}}"#, + ), + MockResponse::json( + 200, + r#"[{"type":"deletion"},{"type":"required_status_checks","parameters":{ + "required_status_checks":[{"context":"cargo audit"}]}}]"#, + ), + ]); + let client = server.client(Some("tok")); + let contexts = required_status_contexts(&client, &repo(), "main"); + assert_eq!( + contexts, + vec!["CI Green", "agentflare/judged", "cargo audit"] + ); + // Cached: a second ask makes no request. + assert_eq!(required_status_contexts(&client, &repo(), "main").len(), 3); + let reqs = server.requests(); + assert_eq!(reqs.len(), 2); + assert_eq!(reqs[0].path, "/repos/o/r/branches/main"); + assert_eq!(reqs[1].path, "/repos/o/r/rules/branches/main"); + } + + #[test] + fn required_status_contexts_is_empty_for_an_unprotected_or_unreadable_branch() { + let server = MockServer::start(vec![ + MockResponse::json(200, r#"{"name":"dev","protected":false}"#), + MockResponse::json(404, r#"{"message":"Not Found"}"#), + MockResponse::json(403, r#"{"message":"Resource not accessible"}"#), + MockResponse::json(404, r#"{"message":"Not Found"}"#), + ]); + let client = server.client(Some("tok")); + assert!(required_status_contexts(&client, &repo(), "dev").is_empty()); + assert!(required_status_contexts(&client, &repo(), "private").is_empty()); + assert_eq!(server.requests().len(), 4); + } + + #[test] + fn required_status_contexts_does_not_cache_a_failed_read() { + // A transient failure must not pin "nothing required" on a gated + // branch for the rest of the process: the next ask goes to GitHub + // again and finds the requirement. + let server = MockServer::start(vec![ + MockResponse::json(502, r#"{"message":"Bad Gateway"}"#), + MockResponse::json(404, r#"{"message":"Not Found"}"#), + MockResponse::json( + 200, + r#"{"name":"rel","protected":true,"protection":{"required_status_checks":{"contexts":["agentflare/judged"]}}}"#, + ), + MockResponse::json(200, "[]"), + ]); + let client = server.client(Some("tok")); + assert!(required_status_contexts(&client, &repo(), "rel").is_empty()); + assert_eq!( + required_status_contexts(&client, &repo(), "rel"), + vec!["agentflare/judged"] + ); + // Now complete, so cached: a third ask makes no request. + assert_eq!(required_status_contexts(&client, &repo(), "rel").len(), 1); + assert_eq!(server.requests().len(), 4); + } + #[test] fn get_default_branch_reads_the_field() { let server = MockServer::start(vec![MockResponse::json( @@ -242,6 +489,41 @@ mod tests { assert_eq!(server.requests()[0].path, "/repos/o/r"); } + #[test] + fn settings_reads_merge_flags_and_defaults_the_missing_ones() { + let server = MockServer::start(vec![MockResponse::json( + 200, + r#"{"default_branch":"main","allow_auto_merge":true,"allow_squash_merge":false}"#, + )]); + let client = server.client(None); + let s = settings(&client, &repo()).unwrap(); + assert!(s.allow_auto_merge); + assert!(!s.allow_squash_merge); + assert!(s.allow_merge_commit, "GitHub's default when omitted"); + assert!(s.allow_rebase_merge); + assert!(!s.delete_branch_on_merge); + assert_eq!(s.merge_method(), MergeMethod::Merge); + let _ = server.requests(); + } + + #[test] + fn merge_method_prefers_squash_then_merge_then_rebase() { + let mut s = RepoSettings::unknown("main"); + assert_eq!(s.merge_method(), MergeMethod::Squash); + s.allow_squash_merge = false; + assert_eq!(s.merge_method(), MergeMethod::Merge); + s.allow_merge_commit = false; + assert_eq!(s.merge_method(), MergeMethod::Rebase); + s.allow_rebase_merge = false; + assert_eq!( + s.merge_method(), + MergeMethod::Squash, + "nothing allowed: still try" + ); + assert_eq!(MergeMethod::Squash.rest(), "squash"); + assert_eq!(MergeMethod::Rebase.graphql(), "REBASE"); + } + #[test] fn get_default_branch_errors_when_field_missing() { let server = MockServer::start(vec![MockResponse::json(200, "{}")]); diff --git a/src/github/test_support.rs b/src/github/test_support.rs index 51e1f61b4..1b1c5bcde 100644 --- a/src/github/test_support.rs +++ b/src/github/test_support.rs @@ -46,6 +46,18 @@ pub struct RecordedRequest { pub method: String, pub path: String, pub body: String, + /// Every request header, names lower-cased, in wire order. + pub headers: Vec<(String, String)>, +} + +impl RecordedRequest { + /// The first value of header `name` (any case), if it was sent. + pub fn header(&self, name: &str) -> Option<&str> { + self.headers + .iter() + .find(|(n, _)| n.eq_ignore_ascii_case(name)) + .map(|(_, v)| v.as_str()) + } } pub struct MockServer { @@ -59,6 +71,7 @@ fn reason(status: u16) -> &'static str { 200 => "OK", 201 => "Created", 204 => "No Content", + 304 => "Not Modified", 401 => "Unauthorized", 403 => "Forbidden", 404 => "Not Found", @@ -183,16 +196,19 @@ fn handle_connection(stream: &mut std::net::TcpStream) -> Option Option { let state = req.state.as_deref().unwrap_or("open"); @@ -85,8 +99,12 @@ impl AgentflareMcp { .ok_or_else(|| ErrorData::invalid_params("number is required", None))?; let pr = pulls::get(&client, &repo, n).map_err(to_mcp_error)?; format!( - "PR #{} [{}] {}: {}", - pr.number, pr.state, pr.title, pr.html_url + "PR #{} [{}] {}: {} (head_sha {})", + pr.number, + pr.state, + pr.title, + pr.html_url, + pr.head.as_ref().map(|h| h.sha.as_str()).unwrap_or("?") ) } "pr_status" => { @@ -153,8 +171,7 @@ impl AgentflareMcp { .number .ok_or_else(|| ErrorData::invalid_params("number is required", None))?; let method = req.merge_method.as_deref().unwrap_or("merge"); - pulls::merge(&client, &repo, n, method).map_err(to_mcp_error)?; - format!("Merged PR #{n} ({method})") + merge_pr_pinned(&client, &repo, n, method, req.head_sha.as_deref())? } "pr_comment" => { let n = req @@ -383,6 +400,53 @@ impl AgentflareMcp { } } +/// `pr_merge`, pinned to a head SHA the way the supervisor's own auto-merge +/// is (`supervisor::merge_approved_pr`): `head_sha` is the head the caller +/// last saw, from `pr_get`/`pr_status`/`pr_wait`, so a commit pushed after +/// the caller judged CI green can never ride along into the merge. Without +/// one, the PR's current head is read first and pinned instead -- that still +/// closes the window between that read and the merge itself. A moved head +/// comes back as an error naming both SHAs, so the caller re-checks the new +/// head rather than retrying blind. Standalone (no `Client::new()`) so it can +/// be driven against a mock server. +fn merge_pr_pinned( + client: &crate::github::Client, + repo: &crate::github::RepoId, + number: u64, + method: &str, + head_sha: Option<&str>, +) -> Result { + use crate::github::pulls; + let current_head = |number: u64| { + pulls::get(client, repo, number) + .ok() + .and_then(|pr| pr.head.map(|h| h.sha)) + }; + let pinned = match head_sha.map(str::trim).filter(|s| !s.is_empty()) { + Some(sha) => sha.to_string(), + None => current_head(number).ok_or_else(|| { + ErrorData::internal_error( + format!("PR #{number}: could not read its head SHA to pin the merge to"), + None, + ) + })?, + }; + match pulls::merge_at_head(client, repo, number, method, Some(&pinned)) { + Ok(()) => Ok(format!("Merged PR #{number} ({method}) at {pinned}")), + Err(e) if pulls::is_head_moved(&e) => { + let now = current_head(number).unwrap_or_else(|| "unknown".to_string()); + Err(ErrorData::invalid_params( + format!( + "PR #{number} was not merged: its head moved since {pinned} (now {now}). \ + Re-check CI on the new head, then call pr_merge again with head_sha={now}" + ), + None, + )) + } + Err(e) => Err(to_mcp_error(e)), + } +} + /// Decides the git ref for `workflow_dispatch`: an explicit `git_ref` wins; /// otherwise an overridden `repo` resolves its default branch via /// `remote_default` (a GitHub API call), and the no-override case via @@ -450,4 +514,85 @@ mod tests { let err = mcp.flare_git_impl(req).unwrap_err(); assert!(err.to_string().contains("unknown action: bogus"), "{err}"); } + + use crate::github::test_support::{MockResponse, MockServer}; + + fn repo() -> crate::github::RepoId { + crate::github::RepoId { + owner: "o".into(), + repo: "r".into(), + } + } + + #[test] + fn merge_pr_pinned_sends_the_callers_head_sha_without_rereading_the_pr() { + let server = MockServer::start(vec![MockResponse::json(200, r#"{"merged":true}"#)]); + let client = server.client(Some("tok")); + let out = merge_pr_pinned(&client, &repo(), 7, "squash", Some("abc123")).unwrap(); + assert!(out.contains("Merged PR #7 (squash) at abc123"), "{out}"); + let reqs = server.requests(); + assert_eq!(reqs.len(), 1, "a caller-supplied head needs no PR read"); + assert_eq!(reqs[0].method, "PUT"); + assert_eq!(reqs[0].path, "/repos/o/r/pulls/7/merge"); + let sent: serde_json::Value = serde_json::from_str(&reqs[0].body).unwrap(); + assert_eq!(sent["sha"], "abc123"); + assert_eq!(sent["merge_method"], "squash"); + } + + #[test] + fn merge_pr_pinned_reads_and_pins_the_current_head_when_none_is_given() { + let server = MockServer::start(vec![ + MockResponse::json( + 200, + r#"{"number":7,"html_url":"u","state":"open","title":"t","head":{"ref":"task/7","sha":"live"}}"#, + ), + MockResponse::json(200, r#"{"merged":true}"#), + ]); + let client = server.client(Some("tok")); + let out = merge_pr_pinned(&client, &repo(), 7, "merge", None).unwrap(); + assert!(out.ends_with("at live"), "{out}"); + let reqs = server.requests(); + assert_eq!(reqs[0].method, "GET"); + assert_eq!(reqs[0].path, "/repos/o/r/pulls/7"); + let sent: serde_json::Value = serde_json::from_str(&reqs[1].body).unwrap(); + assert_eq!(sent["sha"], "live"); + } + + #[test] + fn merge_pr_pinned_reports_a_moved_head_with_both_shas_and_does_not_retry() { + let server = MockServer::start(vec![ + MockResponse::json( + 409, + r#"{"message":"Head branch was modified. Review and try the merge again."}"#, + ), + MockResponse::json( + 200, + r#"{"number":7,"html_url":"u","state":"open","title":"t","head":{"ref":"task/7","sha":"newer"}}"#, + ), + ]); + let client = server.client(Some("tok")); + let err = merge_pr_pinned(&client, &repo(), 7, "squash", Some("stale")).unwrap_err(); + let msg = err.to_string(); + assert!(msg.contains("head moved since stale (now newer)"), "{msg}"); + assert!(msg.contains("head_sha=newer"), "{msg}"); + let reqs = server.requests(); + assert_eq!( + reqs.len(), + 2, + "one merge attempt, one re-read for the message" + ); + assert_eq!(reqs[0].method, "PUT"); + assert_eq!(reqs[1].method, "GET"); + } + + #[test] + fn merge_pr_pinned_surfaces_other_github_errors_unchanged() { + let server = MockServer::start(vec![MockResponse::json( + 405, + r#"{"message":"Pull Request is not mergeable"}"#, + )]); + let client = server.client(Some("tok")); + let err = merge_pr_pinned(&client, &repo(), 7, "squash", Some("abc")).unwrap_err(); + assert!(err.to_string().contains("405"), "{err}"); + } } diff --git a/src/mcp_server/item.rs b/src/mcp_server/item.rs index fcbb8be45..829c3766b 100644 --- a/src/mcp_server/item.rs +++ b/src/mcp_server/item.rs @@ -1716,14 +1716,15 @@ impl AgentflareMcp { ), _ => crate::worktree::PrOutcome::NothingToPush, }; - let (pr_url, no_pr, pr_failure) = match pr_outcome { - crate::worktree::PrOutcome::Opened(url) => (Some(url), None, None), + let (opened_pr, no_pr, pr_failure) = match pr_outcome { + crate::worktree::PrOutcome::Opened(pr) => (Some(pr), None, None), crate::worktree::PrOutcome::NoPrPossible { pushed, reason } => { (None, Some((pushed, reason)), None) } crate::worktree::PrOutcome::Failed(reason) => (None, None, Some(reason)), crate::worktree::PrOutcome::NothingToPush => (None, None, None), }; + let pr_url = opened_pr.as_ref().map(|pr| pr.url.clone()); // An open PR (freshly created, or already existed) means the work // isn't actually finished: move to "in_review" instead of // "completed", leave the claim lease held (so nobody else can @@ -1835,6 +1836,25 @@ impl AgentflareMcp { None, )); } + // The work is done and the item is in review: the draft + // `push_and_open_pr` opened can go to reviewers now. A failure + // here only logs -- the review sweep retries the flip on every + // tick it still sees a draft (`PrCiStatus::Draft`). + if let (Some(item), Some(pr)) = (&item, &opened_pr) + && pr.draft + && !crate::worktree::mark_pr_ready( + item, + &repo_root, + pr.number, + pr.node_id.as_deref(), + ) + { + eprintln!( + "worktree: PR #{} for item {item_id} is still a draft; the review sweep \ + will mark it ready", + pr.number + ); + } marked } else { let moved = self.with_backend_db(|conn| { diff --git a/src/mcp_server/item_status.rs b/src/mcp_server/item_status.rs index 579c492b1..82e7f12e1 100644 --- a/src/mcp_server/item_status.rs +++ b/src/mcp_server/item_status.rs @@ -44,7 +44,7 @@ fn pr_status_summary( crate::worktree::PrCiStatus::Passing { number, labels, .. } => { ("passing", Some(number), vec![], labels) } - crate::worktree::PrCiStatus::AwaitingReview { number, labels } => { + crate::worktree::PrCiStatus::AwaitingReview { number, labels, .. } => { ("awaiting_review", Some(number), vec![], labels) } crate::worktree::PrCiStatus::Behind { number, .. } => { @@ -54,6 +54,9 @@ fn pr_status_summary( ("conflicting", Some(number), vec![], vec![]) } crate::worktree::PrCiStatus::Closed { number } => ("closed", Some(number), vec![], vec![]), + crate::worktree::PrCiStatus::Draft { number, .. } => { + ("draft", Some(number), vec![], vec![]) + } crate::worktree::PrCiStatus::Unknown => ("unknown", known_number, vec![], vec![]), }; PrStatusSummary { diff --git a/src/mcp_server/tests/item_force_tests.rs b/src/mcp_server/tests/item_force_tests.rs index 2109539cb..2cbd91833 100644 --- a/src/mcp_server/tests/item_force_tests.rs +++ b/src/mcp_server/tests/item_force_tests.rs @@ -243,6 +243,7 @@ fn a_passing_pr_is_not_evidence_unless_the_caller_is_on_its_pushed_branch() { number: 7, labels: vec![], head_sha: None, + auto_merge: Default::default(), }; // A live owner's PR going green must not let a bystander take over. assert_eq!(branch_gate(false, passing()), None); @@ -262,6 +263,9 @@ fn a_passing_pr_is_not_evidence_unless_the_caller_is_on_its_pushed_branch() { let awaiting_review = PrCiStatus::AwaitingReview { number: 8, labels: vec![], + changes_requested: false, + head_sha: None, + auto_merge: Default::default(), }; assert!( branch_gate(true, awaiting_review) diff --git a/src/mcp_server/types.rs b/src/mcp_server/types.rs index 44ad542c8..126579c0a 100644 --- a/src/mcp_server/types.rs +++ b/src/mcp_server/types.rs @@ -554,6 +554,11 @@ pub(crate) struct GitHubRequest { #[schemars(description = "Merge method for pr_merge: merge|squash|rebase (default merge)")] #[serde(default)] pub(crate) merge_method: Option, + #[schemars( + description = "pr_merge: the head commit SHA you last saw (pr_get/pr_status/pr_wait report it as head_sha). The merge is refused if the PR head has moved since; omit to pin to the head read at merge time" + )] + #[serde(default)] + pub(crate) head_sha: Option, #[schemars(description = "Reviewer logins (pr_request_review)")] #[serde(default)] pub(crate) reviewers: Option>, @@ -572,7 +577,9 @@ pub(crate) struct GitHubRequest { #[schemars(description = "Release name (release_create)")] #[serde(default)] pub(crate) name: Option, - #[schemars(description = "Mark release as draft (release_create, default false)")] + #[schemars( + description = "Open as a draft (pr_create) / mark release as draft (release_create); default false" + )] #[serde(default)] pub(crate) draft: Option, #[schemars(description = "Mark release as prerelease (release_create, default false)")] diff --git a/src/supervisor.rs b/src/supervisor.rs index 7e861ffc2..869f6a335 100644 --- a/src/supervisor.rs +++ b/src/supervisor.rs @@ -1433,7 +1433,12 @@ pub(crate) fn run_review_sweep( // chunk, or GitHub couldn't resolve that PR) is treated exactly // like any other soft-fail: `Unknown`, polled again next tick -- // never an error for the whole sweep. - let status = match batch_data.get(&number) { + let data = batch_data.get(&number); + // An auto-merge this sweep armed earlier is re-judged on every + // tick, whatever the CI state: a push since then must disarm it + // before GitHub can merge commits nobody checked for findings. + reconcile_armed_auto_merge(mcp, item, &repo_root, number, data); + let status = match data { Some(data) => crate::worktree::pr_ci_status_from_batch(number, data), None => crate::worktree::PrCiStatus::Unknown, }; @@ -1509,6 +1514,10 @@ fn handle_pr_status( checks, labels, } => { + // The repair push that may follow moves the head; an armed + // auto-merge must not be waiting to merge it the moment its + // checks go green. + disarm_our_auto_merge(mcp, item, repo_root, number, "CI is failing"); match self_repair_or_gate( mcp, queue, @@ -1530,6 +1539,7 @@ fn handle_pr_status( number, labels, head_sha, + auto_merge, } => handle_ci_green( mcp, queue, @@ -1540,6 +1550,7 @@ fn handle_pr_status( &labels, CiGreenMerge::Allowed { head_sha: head_sha.as_deref(), + auto_merge: &auto_merge, }, label_id_by_name, folder_path, @@ -1551,7 +1562,13 @@ fn handle_pr_status( // repair labels cleared, the approval gate surfaced (the whole point: // this used to read as `Pending` and never reached the gate), and // CodeRabbit findings still repaired while it waits. - crate::worktree::PrCiStatus::AwaitingReview { number, labels } => handle_ci_green( + crate::worktree::PrCiStatus::AwaitingReview { + number, + labels, + changes_requested, + head_sha, + auto_merge, + } => handle_ci_green( mcp, queue, auth_conn, @@ -1559,7 +1576,11 @@ fn handle_pr_status( item, number, &labels, - CiGreenMerge::BlockedOnReview, + CiGreenMerge::BlockedOnReview { + changes_requested, + head_sha: head_sha.as_deref(), + auto_merge: &auto_merge, + }, label_id_by_name, folder_path, repo_root, @@ -1573,6 +1594,7 @@ fn handle_pr_status( } } crate::worktree::PrCiStatus::Conflicting { number } => { + disarm_our_auto_merge(mcp, item, repo_root, number, "the PR has a merge conflict"); if auto_resolve_conflicts_enabled(repo_root) { match self_repair_or_gate( mcp, @@ -1611,6 +1633,21 @@ fn handle_pr_status( result.skipped += 1; } } + // An in-review item's PR is agentflare's to keep ready for review: + // `item_done` flips the draft it opened, and this is the retry when + // that flip failed (network, a lost race). A draft the item already + // records as flipped was converted back by a human on purpose -- + // held, never re-flipped, and never merged or self-repaired either, + // since `Draft` is decided before any CI state is looked at. + crate::worktree::PrCiStatus::Draft { number, node_id } => { + if crate::worktree::pr_marked_ready(item) { + result.waiting += 1; + } else if crate::worktree::mark_pr_ready(item, repo_root, number, node_id.as_deref()) { + result.updated += 1; + } else { + result.skipped += 1; + } + } // A review bot's own pending "review paused" status is one way a PR // reads as pending forever: the bot never reviews the new head on // its own, and nothing else here would summon it. @@ -1743,6 +1780,8 @@ pub(crate) mod notify; pub(crate) use notify::*; mod merge; use merge::*; +mod auto_merge; +use auto_merge::*; mod review_bots; pub(crate) mod review_findings; use review_bots::*; diff --git a/src/supervisor/auto_merge.rs b/src/supervisor/auto_merge.rs new file mode 100644 index 000000000..bdba6218c --- /dev/null +++ b/src/supervisor/auto_merge.rs @@ -0,0 +1,213 @@ +//! Ownership of GitHub's native auto-merge on an item's PR. The sweep arms +//! auto-merge (`merge::merge_approved_pr`) only once the approval label is +//! on and CodeRabbit findings are clear, and GitHub then merges whatever the +//! PR head is when its requirements pass -- including commits pushed after +//! the sweep judged it, since a push from a write-access account (every +//! agentflare agent) keeps auto-merge armed. So the sweep records what it +//! armed (`metadata.pr.auto_merge`: the head it judged and the PR's node +//! id) and, on every later tick, disarms its own auto-merge the moment the +//! head moves, CI fails, a conflict appears, findings come back or the label +//! is removed. Auto-merge that a human armed carries no record and is never +//! touched. Split out of `merge.rs` to keep it under the LOC gate. + +use super::*; + +/// The auto-merge this sweep armed on an item's PR, as recorded in +/// `metadata.pr.auto_merge`. +#[derive(Debug, Clone, PartialEq, Eq)] +pub(super) struct ArmedAutoMerge { + /// The head the arming was pinned to (`expectedHeadOid`); empty when + /// GitHub reported none, in which case head moves can't be judged. + pub head: String, + /// The PR's GraphQL node id, what the disarm mutation addresses. + pub node_id: String, +} + +/// The auto-merge this sweep armed on `item`'s PR, if any. `None` for a PR +/// whose auto-merge nobody here armed -- a human's, or none at all. +pub(super) fn armed_auto_merge(item: &agentflare_backend::item::Item) -> Option { + let meta = serde_json::from_str::(&item.metadata).ok()?; + let armed = meta.get("pr")?.get("auto_merge")?; + Some(ArmedAutoMerge { + head: armed["head"].as_str().unwrap_or_default().to_string(), + node_id: armed["node_id"].as_str()?.to_string(), + }) +} + +/// Records that this sweep just armed auto-merge on `item`'s PR at `head`. +pub(super) fn record_armed_auto_merge( + mcp: &AgentflareMcp, + item: &agentflare_backend::item::Item, + head: Option<&str>, + node_id: &str, +) { + set_pr_auto_merge( + mcp, + item, + Some(serde_json::json!({ "head": head.unwrap_or_default(), "node_id": node_id })), + ); +} + +/// Forgets a recorded arming -- after disarming it, or once GitHub reports +/// it gone. +pub(super) fn clear_armed_auto_merge(mcp: &AgentflareMcp, item: &agentflare_backend::item::Item) { + set_pr_auto_merge(mcp, item, None); +} + +/// Writes or removes `metadata.pr.auto_merge`, merged into the existing `pr` +/// record rather than replacing it. Best-effort like `persist_pr_identity`: +/// a db hiccup costs one redundant arm or disarm on a later tick. +fn set_pr_auto_merge( + mcp: &AgentflareMcp, + item: &agentflare_backend::item::Item, + value: Option, +) { + let outcome = mcp.with_backend_db(|conn| { + crate::mcp_server::merge_item_metadata(conn, &item.id, |merged| { + let mut pr = merged + .get("pr") + .and_then(|v| v.as_object()) + .cloned() + .unwrap_or_default(); + match value { + Some(v) => { + pr.insert("auto_merge".into(), v); + } + None => { + pr.remove("auto_merge"); + } + } + merged.insert("pr".into(), serde_json::Value::Object(pr)); + }) + }); + match outcome { + Ok(Ok(_)) => {} + Ok(Err(e)) => eprintln!( + "agentflare-supervisor: could not record auto-merge state for item #{}: {e}", + item.sequence_id + ), + Err(e) => eprintln!( + "agentflare-supervisor: could not record auto-merge state for item #{}: {e:?}", + item.sequence_id + ), + } +} + +/// Disarms auto-merge on PR `number` if this sweep armed it, and forgets the +/// arming once GitHub has confirmed the disarm; a no-op, with no network +/// call, for auto-merge nobody here armed. A disarm GitHub refused keeps the +/// record, so the next tick tries again rather than losing track of an +/// arming that is still live. Returns whether the PR is known disarmed. +pub(super) fn disarm_our_auto_merge( + mcp: &AgentflareMcp, + item: &agentflare_backend::item::Item, + repo_root: &std::path::Path, + number: u64, + why: &str, +) -> bool { + let Some(armed) = armed_auto_merge(item) else { + return false; + }; + let Some(repo) = crate::github::RepoId::resolve_from_remote(repo_root) else { + return false; + }; + let Ok(client) = crate::github::Client::new() else { + return false; + }; + if !disarm_auto_merge_with(&client, &repo, number, &armed.node_id, why) { + return false; + } + clear_armed_auto_merge(mcp, item); + true +} + +/// `disarm_our_auto_merge`'s GitHub half, split out for mock-server tests +/// the same way `merge_approved_pr` is. Returns whether GitHub accepted the +/// disarm; a refusal is logged and left for the next tick to retry. +pub(super) fn disarm_auto_merge_with( + client: &crate::github::Client, + repo: &crate::github::RepoId, + number: u64, + node_id: &str, + why: &str, +) -> bool { + match crate::github::graphql::disable_auto_merge(client, node_id) { + Ok(()) => { + eprintln!( + "agentflare-supervisor: disarmed GitHub auto-merge on PR #{number} in {repo}: {why}" + ); + true + } + Err(e) => { + eprintln!( + "agentflare-supervisor: could not disarm GitHub auto-merge on PR #{number} in \ + {repo}: {}; will retry next tick", + e.log_safe() + ); + false + } + } +} + +/// What a sweep tick does about a recorded arming, given the PR's fresh +/// snapshot. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub(super) enum ArmedVerdict { + /// Still the head it was armed on, still armed: nothing to do. + Keep, + /// GitHub no longer has it armed (merged, closed, or disarmed by + /// someone): drop the stale record. + Forget, + /// The head moved past the commit it was armed on: those commits were + /// never judged for findings, so GitHub must not merge them on this + /// arming. + Disarm, +} + +/// Judges a recorded arming against the batch snapshot. No snapshot (the +/// query for that chunk failed) keeps the record for the next tick. +pub(super) fn judge_armed_auto_merge( + armed: &ArmedAutoMerge, + data: Option<&crate::github::graphql::BatchPrData>, +) -> ArmedVerdict { + let Some(data) = data else { + return ArmedVerdict::Keep; + }; + if data.merged || data.closed || !data.auto_merge_enabled { + return ArmedVerdict::Forget; + } + match data.head_sha.as_deref() { + Some(head) if !armed.head.is_empty() && head != armed.head => ArmedVerdict::Disarm, + _ => ArmedVerdict::Keep, + } +} + +/// Runs before `handle_pr_status` for every item with a recorded arming: +/// disarms it when the head moved, forgets it when GitHub no longer has it. +/// Runs on every status, not just the CI-green ones, so a push that lands +/// while CI is still pending is caught before those checks can go green. +pub(super) fn reconcile_armed_auto_merge( + mcp: &AgentflareMcp, + item: &agentflare_backend::item::Item, + repo_root: &std::path::Path, + number: u64, + data: Option<&crate::github::graphql::BatchPrData>, +) { + let Some(armed) = armed_auto_merge(item) else { + return; + }; + match judge_armed_auto_merge(&armed, data) { + ArmedVerdict::Keep => {} + ArmedVerdict::Forget => clear_armed_auto_merge(mcp, item), + ArmedVerdict::Disarm => { + disarm_our_auto_merge( + mcp, + item, + repo_root, + number, + "the PR head moved past the commit it was armed on; the new commits have not \ + been judged yet", + ); + } + } +} diff --git a/src/supervisor/merge.rs b/src/supervisor/merge.rs index 764a8e5cd..11c6d3d40 100644 --- a/src/supervisor/merge.rs +++ b/src/supervisor/merge.rs @@ -8,14 +8,59 @@ use super::*; -/// Whether a CI-green PR may be merged this tick, and at which head. +/// Whether a CI-green PR may be merged this tick, and at which head. Both +/// arms carry the head the green verdict was made on and the PR's +/// auto-merge handle (`worktree::AutoMergeRef`): once approved, either arm +/// arms GitHub's native auto-merge when the repo allows it, and only +/// `Allowed` falls back to a direct merge. #[derive(Clone, Copy, Debug)] pub(super) enum CiGreenMerge<'a> { /// Merge once approved, pinned to `head_sha` -- the commit the green /// verdict was made on. - Allowed { head_sha: Option<&'a str> }, - /// GitHub is blocking the merge until a human review lands; don't try. - BlockedOnReview, + Allowed { + head_sha: Option<&'a str>, + auto_merge: &'a crate::worktree::AutoMergeRef, + }, + /// GitHub is blocking the merge until a human review lands; a direct + /// merge would be refused, so only auto-merge is armed. `changes_requested` + /// tells the approval card whether a reviewer asked for changes or nobody + /// has approved yet. + BlockedOnReview { + changes_requested: bool, + head_sha: Option<&'a str>, + auto_merge: &'a crate::worktree::AutoMergeRef, + }, +} + +impl<'a> CiGreenMerge<'a> { + /// The head the green verdict was made on, whichever arm this is. + pub(super) fn head_sha(self) -> Option<&'a str> { + match self { + CiGreenMerge::Allowed { head_sha, .. } + | CiGreenMerge::BlockedOnReview { head_sha, .. } => head_sha, + } + } + + /// The PR's auto-merge handle, whichever arm this is. + pub(super) fn auto_merge(self) -> &'a crate::worktree::AutoMergeRef { + match self { + CiGreenMerge::Allowed { auto_merge, .. } + | CiGreenMerge::BlockedOnReview { auto_merge, .. } => auto_merge, + } + } +} + +/// What one tick's merge path did for an approved, CI-green PR. +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub(super) enum MergeAttempt { + /// Merged just now, by the direct call; the item can be promoted. + Merged, + /// GitHub's auto-merge is armed (this tick or earlier): GitHub merges + /// the PR itself, or takes it through the merge queue, once every + /// requirement holds, and the next sweep sees it as `Merged`. + AutoMergeArmed, + /// Neither happened this tick; the next sweep looks again. + NotMerged, } /// `handle_pr_status`'s CI-green path, shared by `Passing` and @@ -87,10 +132,39 @@ pub(super) fn handle_ci_green( // unrelated reason earlier in its life (e.g. the go/no-go // decision gate below, or `skip_item`), since that gate's call // already consumed the bare-id token (item #587). - if !labels.iter().any(|l| l == PR_APPROVAL_LABEL) - && first_time_gated(&format!("pr-approval:{}", item.id)) - { - notify_pr_approval_gate(item, folder_path, number); + // + // A GitHub-review block gets its own card (and its own once-per-item + // key): telling the human to attach the approval label there would + // be wrong -- the label can't merge a review-blocked PR -- and when + // the review later lands with the label still missing, the plain + // label card is still owed. + let label_missing = !labels.iter().any(|l| l == PR_APPROVAL_LABEL); + match merge { + CiGreenMerge::Allowed { .. } => { + if label_missing && first_time_gated(&format!("pr-approval:{}", item.id)) { + notify_pr_approval_gate( + item, + folder_path, + number, + PrApprovalBlocker::ApprovalLabel, + ); + } + } + CiGreenMerge::BlockedOnReview { + changes_requested, .. + } => { + if first_time_gated(&format!("pr-review:{}", item.id)) { + notify_pr_approval_gate( + item, + folder_path, + number, + PrApprovalBlocker::GitHubReview { + changes_requested, + label_missing, + }, + ); + } + } } // CI being green and a human's approval label being attached // don't mean the PR is actually done if CodeRabbit's own review @@ -109,10 +183,7 @@ pub(super) fn handle_ci_green( if already_gated_or_in_flight(mcp, queue, item, label_id_by_name) { result.skipped += 1; } else { - let head_sha = match merge { - CiGreenMerge::Allowed { head_sha } => head_sha, - CiGreenMerge::BlockedOnReview => None, - }; + let head_sha = merge.head_sha(); let review = fetch_review_bot_state( mcp, queue, @@ -141,7 +212,7 @@ pub(super) fn handle_ci_green( PassingPrOutcome::Merged => result.promoted += 1, PassingPrOutcome::NotMerged => match merge { CiGreenMerge::Allowed { .. } => result.skipped += 1, - CiGreenMerge::BlockedOnReview => result.waiting += 1, + CiGreenMerge::BlockedOnReview { .. } => result.waiting += 1, }, PassingPrOutcome::Repair(SelfRepairOutcome::Dispatched) => result.review_repaired += 1, PassingPrOutcome::Repair(SelfRepairOutcome::Deferred) => result.waiting += 1, @@ -366,6 +437,19 @@ pub(super) fn merge_or_repair_findings( folder_path: &str, merge: CiGreenMerge<'_>, ) -> PassingPrOutcome { + if review.dispatch_needed() || review.blocks_merge() { + // Auto-merge this sweep armed on an earlier, findings-free tick + // would let the next approving review merge these findings + // untouched -- the exact thing item #628 forbids. Disarm it before + // routing to repair or holding the merge. + disarm_our_auto_merge( + mcp, + item, + repo_root, + number, + "unresolved review-bot findings hold the merge", + ); + } if review.dispatch_needed() { return PassingPrOutcome::Repair(coderabbit_repair_or_gate( mcp, @@ -394,32 +478,45 @@ pub(super) fn merge_or_repair_findings( CODERABBIT_REPAIR_COMPLETED_KEY, ); clear_stale_coderabbit_repair_label(folder_path, number, labels, summary.as_deref()); - let CiGreenMerge::Allowed { head_sha } = merge else { - return PassingPrOutcome::NotMerged; - }; - if merge_if_approved(mcp, item, repo_root, number, labels, head_sha) { + if merge_if_approved(mcp, item, repo_root, number, labels, merge) { PassingPrOutcome::Merged } else { PassingPrOutcome::NotMerged } } -/// Auto-merges a CI-green PR and promotes its item, but only once a human -/// has attached `PR_APPROVAL_LABEL` to the PR itself -- checked first and +/// Merges a CI-green PR (directly, or by arming GitHub's auto-merge) and +/// promotes its item once it has actually merged, but only once a human has +/// attached `PR_APPROVAL_LABEL` to the PR itself -- checked first and /// short-circuits before any GitHub call so an unapproved item never touches /// the network here. Only ever called from `merge_or_repair_findings`, once /// it has confirmed there are no unresolved CodeRabbit findings, so CI green /// is structurally required and findings are structurally clean: the label -/// can add a gate on top of both, never bypass either. +/// can add a gate on top of both, never bypass either. Returns whether the +/// item was promoted this tick; an armed auto-merge is promoted by a later +/// sweep, when the PR reads `Merged`. +/// +/// The one network call made without the label is disarming an auto-merge +/// this sweep itself armed while the label was still attached: a human who +/// removes the label has withdrawn the approval it stood for. Auto-merge a +/// human armed carries no record (`auto_merge::armed_auto_merge`) and is +/// left alone. pub(super) fn merge_if_approved( mcp: &AgentflareMcp, item: &agentflare_backend::item::Item, repo_root: &std::path::Path, number: u64, labels: &[String], - head_sha: Option<&str>, + merge: CiGreenMerge<'_>, ) -> bool { if !labels.iter().any(|l| l == PR_APPROVAL_LABEL) { + disarm_our_auto_merge( + mcp, + item, + repo_root, + number, + "the approval label was removed", + ); return false; } let Some(repo) = crate::github::RepoId::resolve_from_remote(repo_root) else { @@ -428,42 +525,239 @@ pub(super) fn merge_if_approved( let Ok(client) = crate::github::Client::new() else { return false; }; - merge_approved_pr(&client, &repo, number, head_sha) && promote_merged_item(mcp, item, repo_root) + let auto = merge.auto_merge(); + match merge_approved_pr(&client, &repo, number, merge) { + MergeAttempt::Merged => promote_merged_item(mcp, item, repo_root), + MergeAttempt::AutoMergeArmed => { + // Armed just now (as opposed to found already armed, which may + // be a human's): remember the head it was pinned to, so a later + // push can be caught and the arming withdrawn. + if let (false, Some(node_id)) = (auto.enabled, auto.node_id.as_deref()) { + record_armed_auto_merge(mcp, item, merge.head_sha(), node_id); + } + false + } + MergeAttempt::NotMerged => false, + } +} + +/// The commit-status context the sweep stamps on a head it has judged fit +/// to merge (approval label on, no unresolved CodeRabbit findings). Auto- +/// merge is GitHub's to fire, and a push from a write-access account keeps +/// it armed, so sweep-time reconciliation (`auto_merge::reconcile_armed_auto_merge`) +/// alone can't stop GitHub merging a head pushed and gone green between two +/// ticks. A repo that requires this context in its branch protection closes +/// that window at merge time: a new head carries no status until the sweep +/// judges it, so GitHub waits. Repos that don't require it see it as an +/// informational status only. +/// +/// Not for branches that merge through a merge queue: the queue evaluates +/// required statuses on its temporary merge-group commit, which the sweep +/// never sees, so a required `agentflare/judged` would hold every queued +/// entry forever. There the queue's own `merge_group` CI run is the gate. +pub(crate) const JUDGED_STATUS_CONTEXT: &str = "agentflare/judged"; + +/// Stamps `JUDGED_STATUS_CONTEXT` on `sha` unless it already carries it. +/// Runs before the merge path is chosen, so an already-armed auto-merge, a +/// direct merge and a review-blocked PR all get the judged head stamped -- +/// a stamp that failed on an earlier tick is retried here, and a lookup +/// failure skips the tick rather than posting blind. Soft-fails: a missing +/// status only matters to repos that require the context, where it makes +/// GitHub wait -- the safe direction. +fn mark_head_judged_if_missing( + client: &crate::github::Client, + repo: &crate::github::RepoId, + number: u64, + sha: &str, +) { + match crate::github::actions::list_commit_statuses(client, repo, sha) { + Ok(statuses) + if statuses.iter().any(|s| { + s.name == JUDGED_STATUS_CONTEXT && s.conclusion.as_deref() == Some("success") + }) => + { + return; + } + Ok(_) => {} + Err(e) => { + eprintln!( + "agentflare-supervisor: could not read statuses of head {sha} of PR #{number} \ + in {repo}: {}; leaving the judged stamp for the next tick", + e.log_safe() + ); + return; + } + } + mark_head_judged(client, repo, number, sha); +} + +/// Posts `JUDGED_STATUS_CONTEXT` on `sha`, logging a refusal. +fn mark_head_judged( + client: &crate::github::Client, + repo: &crate::github::RepoId, + number: u64, + sha: &str, +) { + if let Err(e) = crate::github::actions::create_commit_status( + client, + repo, + sha, + "success", + JUDGED_STATUS_CONTEXT, + "approval label on, no unresolved CodeRabbit findings", + ) { + eprintln!( + "agentflare-supervisor: could not mark head {sha} of PR #{number} in {repo} as \ + judged: {}", + e.log_safe() + ); + } } -/// The actual GitHub merge call for an approved, CI-green PR. Split out from +/// Why arming GitHub's auto-merge on a PR is safe: what stops GitHub from +/// merging a head pushed after the sweep judged it. Without one, the sweep +/// never arms auto-merge and merges directly, pinned to the judged head. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +enum AutoMergeGate { + /// The base branch merges through a merge queue: nothing merges without + /// the queue's own `merge_group` CI run on the exact merge-group commit, + /// and a direct merge is refused there anyway. + MergeQueue, + /// The base branch's protection requires `JUDGED_STATUS_CONTEXT`, so a + /// later push is a new sha without the status and waits for the sweep. + JudgedStatusRequired, +} + +impl std::fmt::Display for AutoMergeGate { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self { + AutoMergeGate::MergeQueue => f.write_str("the base branch's merge queue"), + AutoMergeGate::JudgedStatusRequired => { + write!(f, "the required {JUDGED_STATUS_CONTEXT} status") + } + } + } +} + +/// The merge-time gate that makes arming auto-merge on a PR into `base` +/// safe, if the branch has one. Read once per branch per process +/// (`repos::required_status_contexts`). +fn auto_merge_gate( + client: &crate::github::Client, + repo: &crate::github::RepoId, + base: &str, + merge_queue: bool, +) -> Option { + if merge_queue { + return Some(AutoMergeGate::MergeQueue); + } + crate::github::repos::required_status_contexts(client, repo, base) + .iter() + .any(|c| c == JUDGED_STATUS_CONTEXT) + .then_some(AutoMergeGate::JudgedStatusRequired) +} + +/// The actual GitHub merge for an approved, CI-green PR. Split out from /// `merge_if_approved` so tests can drive it against a mock server instead /// of `Client::new()`'s real credentials/host, mirroring `github::pulls`' -/// own test style. Squash matches this repo's existing single-commit-per-item -/// convention. Logs and falls through (never retries in-line) on failure -- -/// branch protection or a merge conflict just means the item sits until the -/// next sweep tick, same as any other `skipped` outcome. +/// own test style. /// -/// Pinned to `head_sha`, the commit the sweep's snapshot judged CI-green: the -/// sweep acts on a snapshot, and an agent may push between the fetch and -/// this call. GitHub then answers 409 instead of merging unchecked code, -/// which is an ordinary skip -- the next tick judges the new head. +/// Prefers GitHub's native auto-merge when the repo allows it and the base +/// branch has a merge-time gate (`auto_merge_gate`): armed once +/// (`autoMergeRequest` already set means nothing to do), pinned to the head +/// the sweep judged, with the merge method the repo's settings allow +/// (`repos::settings`, read once per process: squash first, matching the +/// single-commit-per-item convention). Auto-merge is what lets a review- +/// blocked PR merge the moment the review lands, and the only way onto a +/// merge queue. Without a gate, GitHub's auto-merge would also merge a head +/// pushed and gone green between two sweep ticks that nobody judged for +/// findings, so the sweep stays with the direct merge pinned to the judged +/// head instead. GitHub refuses to arm auto-merge on a PR it could merge +/// right now, so a `CiGreenMerge::Allowed` PR falls back to the direct +/// merge call when arming fails, there is no gate, or the repo has +/// auto-merge off; a review-blocked PR never does, since a direct merge +/// would only be refused too. +/// +/// Logs and falls through (never retries in-line) on failure -- branch +/// protection or a merge conflict just means the item sits until the next +/// sweep tick, same as any other `skipped` outcome. The direct merge is +/// pinned to `head_sha`, the commit the sweep's snapshot judged CI-green: an +/// agent may push between the fetch and this call, and GitHub then answers +/// 409 instead of merging unchecked code, which is an ordinary skip -- the +/// next tick judges the new head. pub(super) fn merge_approved_pr( client: &crate::github::Client, repo: &crate::github::RepoId, number: u64, - head_sha: Option<&str>, -) -> bool { - match crate::github::pulls::merge_at_head(client, repo, number, "squash", head_sha) { - Ok(()) => true, + merge: CiGreenMerge<'_>, +) -> MergeAttempt { + // The head is judged (label on, findings clear) whichever way it merges + // from here; stamp it first so a repo requiring the context lets the + // direct merge, an arming from an earlier tick, or a review landing on + // a blocked PR all go through on exactly this head. + if let Some(sha) = merge.head_sha() { + mark_head_judged_if_missing(client, repo, number, sha); + } + let settings = crate::github::repos::settings(client, repo).unwrap_or_else(|e| { + eprintln!( + "agentflare-supervisor: could not read {repo}'s merge settings ({}); assuming \ + squash and no auto-merge", + e.log_safe() + ); + crate::github::repos::RepoSettings::unknown("") + }); + let method = settings.merge_method(); + let head_sha = merge.head_sha(); + let auto = merge.auto_merge(); + if settings.allow_auto_merge { + if auto.enabled { + return MergeAttempt::AutoMergeArmed; + } + let base = auto.base_ref.as_deref().unwrap_or(&settings.default_branch); + if let (Some(node_id), Some(gate)) = ( + auto.node_id.as_deref(), + auto_merge_gate(client, repo, base, auto.merge_queue), + ) { + match crate::github::graphql::enable_auto_merge( + client, + node_id, + method.graphql(), + head_sha, + ) { + Ok(()) => { + eprintln!( + "agentflare-supervisor: armed GitHub auto-merge ({}) on PR #{number} in \ + {repo}, gated by {gate}; GitHub merges it once every requirement holds", + method.rest() + ); + return MergeAttempt::AutoMergeArmed; + } + Err(e) => eprintln!( + "agentflare-supervisor: could not arm GitHub auto-merge on PR #{number} in \ + {repo}: {}", + e.log_safe() + ), + } + } + } + if matches!(merge, CiGreenMerge::BlockedOnReview { .. }) { + return MergeAttempt::NotMerged; + } + match crate::github::pulls::merge_at_head(client, repo, number, method.rest(), head_sha) { + Ok(()) => MergeAttempt::Merged, Err(e) if crate::github::pulls::is_head_moved(&e) => { eprintln!( "agentflare-supervisor: PR #{number} in {repo} got new commits since CI was \ checked; not merging this tick" ); - false + MergeAttempt::NotMerged } Err(e) => { eprintln!( "agentflare-supervisor: auto-merge failed for PR #{number} in {repo}: {}", e.log_safe() ); - false + MergeAttempt::NotMerged } } } diff --git a/src/supervisor/notify.rs b/src/supervisor/notify.rs index e6ebce43b..030fb928d 100644 --- a/src/supervisor/notify.rs +++ b/src/supervisor/notify.rs @@ -125,7 +125,7 @@ pub(crate) fn notify_human_gate(item: &agentflare_backend::item::Item, reason: & /// `notify_human_gate`: no-ops without a configured chat id, and a send /// failure only logs -- a notification failure must never block the gate /// itself. -fn request_channel_approval(card_text: &str, approve_label: &str, callback_data: &str) { +fn request_channel_approval(card_text: &str, button: Option<(&str, &str)>) { if test_notify_disabled() { return; } @@ -140,47 +140,114 @@ fn request_channel_approval(card_text: &str, approve_label: &str, callback_data: let _turn_guard = turn_lock .lock() .unwrap_or_else(std::sync::PoisonError::into_inner); - if let Err(e) = - crate::channels::send_telegram_card(&chat_id, card_text, &[(approve_label, callback_data)]) - { + let buttons: Vec<(&str, &str)> = button.into_iter().collect(); + if let Err(e) = crate::channels::send_telegram_card(&chat_id, card_text, &buttons) { eprintln!("agentflare-supervisor: telegram card notify failed: {e}"); } } -/// Telegram-only rich variant of [`notify_human_gate`] for the one gate a -/// human can resolve with a single tap: CI is green and the only thing -/// missing is `PR_APPROVAL_LABEL`. Unlike the plain-text pings, this carries -/// an inline "Approve" button whose `callback_data` embeds the repo and PR -/// number directly (`approve:{owner}/{repo}#{number}`) -- self-contained, -/// so [`poll_telegram_approvals`] never needs to re-resolve a worktree path -/// to act on a click. Same fail-open contract as `notify_human_gate`: no-ops -/// without a configured chat id or a resolvable repo, and a send failure -/// only logs. -pub(crate) fn notify_pr_approval_gate( +/// What is holding a CI-green PR back from merging -- decides the wording of +/// [`notify_pr_approval_gate`]'s card, so a human reading it knows which +/// action actually unblocks the PR instead of being told to attach a label +/// that GitHub would still ignore. +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub(crate) enum PrApprovalBlocker { + /// GitHub would merge it; only agentflare's own `PR_APPROVAL_LABEL` is + /// missing. One tap on the card's button attaches it. + ApprovalLabel, + /// Branch protection is holding it for a human review + /// (`PrCiStatus::AwaitingReview`): an approving review is required, or + /// a reviewer requested changes. The label alone can't merge this. + GitHubReview { + changes_requested: bool, + /// Whether `PR_APPROVAL_LABEL` is still missing too, so the card can + /// say the review is the first of two steps and keep its button. + label_missing: bool, + }, +} + +/// The HTML card body for [`notify_pr_approval_gate`], split out so its +/// wording is testable without a Telegram chat. +fn pr_approval_card_text( item: &agentflare_backend::item::Item, - folder_path: &str, + repo: &crate::github::RepoId, number: u64, -) { - let Some(repo) = crate::github::RepoId::resolve_from_remote(std::path::Path::new(folder_path)) - else { - return; - }; + blocker: PrApprovalBlocker, +) -> String { let excerpt: String = item.description.chars().take(200).collect(); - let text = format!( + let pr_link = format!("PR #{number}"); + let status = match blocker { + PrApprovalBlocker::ApprovalLabel => format!( + "{pr_link} is CI-green and mergeable, awaiting {PR_APPROVAL_LABEL}." + ), + PrApprovalBlocker::GitHubReview { + changes_requested, + label_missing, + } => { + let why = if changes_requested { + "a reviewer requested changes" + } else { + "branch protection requires an approving review" + }; + let then = if label_missing { + format!(" It also still needs {PR_APPROVAL_LABEL} to auto-merge.") + } else { + String::new() + }; + format!( + "{pr_link} is CI-green, but GitHub is blocking the merge until a human review \ + lands: {why}. \ + Review it here.{then}" + ) + } + }; + format!( "\u{1F514} agentflare needs a human\n\ Repo: {repo}\n\ Item: #{} \u{2014} {}\n\ {}\n\n\ - PR #{number} is CI-green and \ - mergeable, awaiting {PR_APPROVAL_LABEL}.", + {status}", item.sequence_id, html_escape(&item.name), html_escape(&excerpt), + ) +} + +/// Telegram-only rich variant of [`notify_human_gate`] for a CI-green PR +/// waiting on a human. When `PR_APPROVAL_LABEL` is all that is missing +/// (`PrApprovalBlocker::ApprovalLabel`) the human can resolve it with a +/// single tap: the card carries an inline "Approve" button whose +/// `callback_data` embeds the repo and PR number directly +/// (`approve:{owner}/{repo}#{number}`) -- self-contained, so +/// [`poll_telegram_approvals`] never needs to re-resolve a worktree path to +/// act on a click. When GitHub itself is holding the PR for a review +/// (`PrApprovalBlocker::GitHubReview`) the card says so and links the +/// review instead of asking for a label that can't merge it; the button is +/// kept only while the label is missing as well. Same fail-open contract as +/// `notify_human_gate`: no-ops without a configured chat id or a resolvable +/// repo, and a send failure only logs. +pub(crate) fn notify_pr_approval_gate( + item: &agentflare_backend::item::Item, + folder_path: &str, + number: u64, + blocker: PrApprovalBlocker, +) { + let Some(repo) = crate::github::RepoId::resolve_from_remote(std::path::Path::new(folder_path)) + else { + return; + }; + let text = pr_approval_card_text(item, &repo, number, blocker); + let wants_label = !matches!( + blocker, + PrApprovalBlocker::GitHubReview { + label_missing: false, + .. + } ); + let callback = format!("approve:{repo}#{number}"); request_channel_approval( &text, - "\u{2705} Approve", - &format!("approve:{repo}#{number}"), + wants_label.then_some(("\u{2705} Approve", callback.as_str())), ); } @@ -202,11 +269,8 @@ pub(crate) fn notify_plan_approval_gate( html_escape(&item.name), html_escape(plan_asset_id), ); - request_channel_approval( - &text, - "\u{2705} Approve", - &format!("approve_plan:{}", item.id), - ); + let callback = format!("approve_plan:{}", item.id); + request_channel_approval(&text, Some(("\u{2705} Approve", callback.as_str()))); } /// Parse an "Approve" button's `callback_data` (`approve_plan:{item_id}`) @@ -235,6 +299,102 @@ pub(crate) fn first_time_gated(item_id: &str) -> bool { .insert(item_id.to_string()) } +#[cfg(test)] +mod pr_approval_card_tests { + use super::*; + + fn item() -> agentflare_backend::item::Item { + agentflare_backend::item::Item { + id: "item-12".into(), + project_id: "p".into(), + state_id: "s".into(), + name: "Widen the ".into(), + description: "desc".into(), + priority: "none".into(), + parent_id: None, + assignee_agent: None, + sequence_id: 12, + sort_order: 0.0, + started_at: None, + completed_at: None, + archived_at: None, + external_source: None, + external_id: None, + metadata: "{}".into(), + created_at: 0, + updated_at: 0, + deleted_at: None, + start_date: None, + due_date: None, + } + } + + fn repo() -> crate::github::RepoId { + crate::github::RepoId { + owner: "o".into(), + repo: "r".into(), + } + } + + #[test] + fn label_blocker_asks_for_the_approval_label() { + let text = pr_approval_card_text(&item(), &repo(), 7, PrApprovalBlocker::ApprovalLabel); + assert!( + text.contains("awaiting status:pr:approved"), + "{text}" + ); + assert!( + text.contains("Widen the <gate>"), + "escaped title: {text}" + ); + assert!(!text.contains("Review it here"), "{text}"); + } + + #[test] + fn review_blocker_names_the_github_review_and_links_it() { + let text = pr_approval_card_text( + &item(), + &repo(), + 7, + PrApprovalBlocker::GitHubReview { + changes_requested: false, + label_missing: false, + }, + ); + assert!( + text.contains("branch protection requires an approving review"), + "{text}" + ); + assert!( + text.contains("href=\"https://github.com/o/r/pull/7/files\">Review it here"), + "{text}" + ); + assert!( + !text.contains("awaiting status:pr:approved"), + "a review-blocked PR must not be described as waiting on the label: {text}" + ); + assert!(!text.contains("also still needs"), "{text}"); + } + + #[test] + fn review_blocker_says_changes_were_requested_and_mentions_a_missing_label() { + let text = pr_approval_card_text( + &item(), + &repo(), + 7, + PrApprovalBlocker::GitHubReview { + changes_requested: true, + label_missing: true, + }, + ); + assert!(text.contains("a reviewer requested changes"), "{text}"); + assert!( + text.contains("also still needs status:pr:approved"), + "{text}" + ); + } +} + #[cfg(test)] mod summarize_reason_tests { use super::*; diff --git a/src/supervisor/tests/auto_merge_tests.rs b/src/supervisor/tests/auto_merge_tests.rs index 054001ced..34297f428 100644 --- a/src/supervisor/tests/auto_merge_tests.rs +++ b/src/supervisor/tests/auto_merge_tests.rs @@ -6,47 +6,567 @@ //! LOC gate (`scripts/loc-gate.sh`), mirroring `stray_pr_tests.rs`. use super::*; +use crate::github::test_support::{MockResponse, MockServer}; +use crate::worktree::AutoMergeRef; + +fn gh_repo() -> crate::github::RepoId { + crate::github::RepoId { + owner: "o".into(), + repo: "r".into(), + } +} + +/// `GET /repos/o/r`: the settings read `merge_approved_pr` makes right after +/// the judged-head stamp (cached per mock server, so once per test). +fn repo_settings(allow_auto_merge: bool, allow_squash: bool) -> MockResponse { + MockResponse::json( + 200, + &format!( + r#"{{"default_branch":"main","allow_auto_merge":{allow_auto_merge},"allow_squash_merge":{allow_squash}}}"# + ), + ) +} + +/// The head's combined status without the judged context, then the stamp +/// POST: what `merge_approved_pr` exchanges first for a head it hasn't +/// stamped yet. +fn judged_missing() -> [MockResponse; 2] { + [ + MockResponse::json(200, r#"{"state":"pending","statuses":[]}"#), + MockResponse::json(201, r#"{"id":1}"#), + ] +} + +/// The head's combined status already carrying the judged context: no +/// stamp POST follows. +fn judged_present() -> MockResponse { + MockResponse::json( + 200, + r#"{"state":"success","statuses":[{"context":"agentflare/judged","state":"success"}]}"#, + ) +} + +const STATUS_PATH: &str = "/repos/o/r/commits/abc123/status?per_page=100"; + +/// `GET /repos/o/r/branches/main`: the base branch's protection requires +/// `agentflare/judged`, the merge-time gate that makes arming safe. +fn base_requires_judged() -> [MockResponse; 2] { + [ + MockResponse::json( + 200, + r#"{"name":"main","protected":true,"protection":{"required_status_checks":{"contexts":["agentflare/judged"]}}}"#, + ), + MockResponse::json(200, "[]"), + ] +} + +/// The branch and rulesets reads for a base branch with no gate at all. +fn base_ungated() -> [MockResponse; 2] { + [ + MockResponse::json(200, r#"{"name":"main","protected":false}"#), + MockResponse::json(200, "[]"), + ] +} + +/// A PR into `main`, auto-merge not yet armed. +fn unarmed() -> AutoMergeRef { + AutoMergeRef { + node_id: Some("PR_1".into()), + enabled: false, + base_ref: Some("main".into()), + merge_queue: false, + } +} + +fn allowed<'a>(head_sha: Option<&'a str>, auto_merge: &'a AutoMergeRef) -> CiGreenMerge<'a> { + CiGreenMerge::Allowed { + head_sha, + auto_merge, + } +} #[test] -fn merge_approved_pr_merges_via_squash_on_success() { - let server = crate::github::test_support::MockServer::start(vec![ - crate::github::test_support::MockResponse::json(200, r#"{"merged":true}"#), +fn merge_approved_pr_merges_directly_with_the_repos_method_when_auto_merge_is_off() { + let [status, stamp] = judged_missing(); + let server = MockServer::start(vec![ + status, + stamp, + repo_settings(false, true), + MockResponse::json(200, r#"{"merged":true}"#), ]); let client = server.client(Some("tok")); - let repo = crate::github::RepoId { - owner: "o".into(), - repo: "r".into(), - }; + let auto = unarmed(); - assert!(merge_approved_pr(&client, &repo, 42, Some("abc123"))); + assert_eq!( + merge_approved_pr(&client, &gh_repo(), 42, allowed(Some("abc123"), &auto)), + MergeAttempt::Merged + ); let reqs = server.requests(); - assert_eq!(reqs[0].method, "PUT"); - assert_eq!(reqs[0].path, "/repos/o/r/pulls/42/merge"); - let sent: serde_json::Value = serde_json::from_str(&reqs[0].body).unwrap(); + // The judged head is stamped before the merge path is chosen, so a + // repo requiring the context lets exactly this head merge. + assert_eq!(reqs[0].path, STATUS_PATH); + assert_eq!(reqs[1].method, "POST"); + assert_eq!(reqs[1].path, "/repos/o/r/statuses/abc123"); + let status: serde_json::Value = serde_json::from_str(&reqs[1].body).unwrap(); + assert_eq!(status["state"], "success"); + assert_eq!(status["context"], JUDGED_STATUS_CONTEXT); + assert_eq!(reqs[2].path, "/repos/o/r", "then the settings"); + assert_eq!(reqs[3].method, "PUT"); + assert_eq!(reqs[3].path, "/repos/o/r/pulls/42/merge"); + let sent: serde_json::Value = serde_json::from_str(&reqs[3].body).unwrap(); assert_eq!(sent["merge_method"], "squash"); // Pinned to the head the sweep judged green. assert_eq!(sent["sha"], "abc123"); + assert_eq!( + reqs.len(), + 4, + "no GraphQL call when the repo has auto-merge off" + ); +} + +#[test] +fn merge_approved_pr_leaves_the_stamp_for_the_next_tick_when_the_status_lookup_fails() { + let server = MockServer::start(vec![ + MockResponse::json(500, r#"{"message":"boom"}"#), + repo_settings(false, true), + MockResponse::json(200, r#"{"merged":true}"#), + ]); + let client = server.client(Some("tok")); + let auto = AutoMergeRef::default(); + assert_eq!( + merge_approved_pr(&client, &gh_repo(), 42, allowed(Some("abc123"), &auto)), + MergeAttempt::Merged + ); + let reqs = server.requests(); + assert_eq!(reqs.len(), 3, "no blind stamp after a failed lookup"); + assert!( + reqs.iter() + .all(|r| !r.path.starts_with("/repos/o/r/statuses/")) + ); +} + +#[test] +fn merge_approved_pr_uses_a_merge_commit_when_the_repo_forbids_squash() { + let server = MockServer::start(vec![ + repo_settings(false, false), + MockResponse::json(200, r#"{"merged":true}"#), + ]); + let client = server.client(Some("tok")); + let auto = AutoMergeRef::default(); + assert_eq!( + merge_approved_pr(&client, &gh_repo(), 42, allowed(None, &auto)), + MergeAttempt::Merged + ); + let reqs = server.requests(); + let sent: serde_json::Value = serde_json::from_str(&reqs[1].body).unwrap(); + assert_eq!(sent["merge_method"], "merge"); +} + +#[test] +fn merge_approved_pr_arms_github_auto_merge_pinned_to_the_head_when_the_repo_allows_it() { + let [status, stamp] = judged_missing(); + let [branch, rules] = base_requires_judged(); + let server = MockServer::start(vec![ + status, + stamp, + repo_settings(true, true), + branch, + rules, + MockResponse::json( + 200, + r#"{"data":{"enablePullRequestAutoMerge":{"pullRequest":{"autoMergeRequest":{"enabledAt":"x"}}}}}"#, + ), + ]); + let client = server.client(Some("tok")); + let auto = unarmed(); + + assert_eq!( + merge_approved_pr(&client, &gh_repo(), 42, allowed(Some("abc123"), &auto)), + MergeAttempt::AutoMergeArmed + ); + + let reqs = server.requests(); + assert_eq!( + reqs.len(), + 6, + "stamped, gate checked (protection + rulesets) and armed: no direct merge call follows" + ); + // The judged head is stamped before arming, so a repo requiring the + // context lets GitHub merge exactly this head, not a later push. + assert_eq!(reqs[0].path, STATUS_PATH); + assert_eq!(reqs[1].path, "/repos/o/r/statuses/abc123"); + // Arming is only safe behind a merge-time gate: the base branch's + // protection is read (once per process) before the mutation. + assert_eq!(reqs[3].path, "/repos/o/r/branches/main"); + assert_eq!(reqs[4].path, "/repos/o/r/rules/branches/main"); + assert_eq!(reqs[5].path, "/graphql"); + let sent: serde_json::Value = serde_json::from_str(&reqs[5].body).unwrap(); + assert_eq!(sent["variables"]["input"]["pullRequestId"], "PR_1"); + assert_eq!(sent["variables"]["input"]["mergeMethod"], "SQUASH"); + assert_eq!(sent["variables"]["input"]["expectedHeadOid"], "abc123"); +} + +#[test] +fn merge_approved_pr_merges_directly_when_the_base_branch_has_no_merge_time_gate() { + // The repo allows auto-merge, but nothing on `main` would stop GitHub + // merging a later, unjudged push on a standing arming: stay with the + // direct merge pinned to the judged head. + let [status, stamp] = judged_missing(); + let [branch, rules] = base_ungated(); + let server = MockServer::start(vec![ + status, + stamp, + repo_settings(true, true), + branch, + rules, + MockResponse::json(200, r#"{"merged":true}"#), + ]); + let client = server.client(Some("tok")); + let auto = unarmed(); + assert_eq!( + merge_approved_pr(&client, &gh_repo(), 42, allowed(Some("abc123"), &auto)), + MergeAttempt::Merged + ); + let reqs = server.requests(); + assert_eq!(reqs.len(), 6); + assert!( + reqs.iter().all(|r| r.path != "/graphql"), + "no arming without a gate" + ); + assert_eq!(reqs[5].method, "PUT"); + assert_eq!(reqs[5].path, "/repos/o/r/pulls/42/merge"); + let sent: serde_json::Value = serde_json::from_str(&reqs[5].body).unwrap(); + assert_eq!(sent["sha"], "abc123"); + + // A review-blocked PR on that branch is simply left for the review to + // land; the next tick after it merges directly. + let [branch, rules] = base_ungated(); + let server = MockServer::start(vec![ + judged_present(), + repo_settings(true, true), + branch, + rules, + ]); + let client = server.client(Some("tok")); + let blocked = CiGreenMerge::BlockedOnReview { + changes_requested: false, + head_sha: Some("abc123"), + auto_merge: &auto, + }; + assert_eq!( + merge_approved_pr(&client, &gh_repo(), 42, blocked), + MergeAttempt::NotMerged + ); + let reqs = server.requests(); + assert_eq!( + reqs.len(), + 4, + "status lookup, settings, gate check; no merge" + ); + assert!( + reqs.iter() + .all(|r| r.path != "/graphql" && r.method != "PUT") + ); +} + +#[test] +fn merge_approved_pr_arms_auto_merge_on_a_merge_queue_branch_without_reading_protection() { + // A merge queue is its own gate (the queue's CI runs on the merge-group + // commit) and the only way onto the queue is auto-merge. + let server = MockServer::start(vec![ + judged_present(), + repo_settings(true, true), + MockResponse::json( + 200, + r#"{"data":{"enablePullRequestAutoMerge":{"pullRequest":{"autoMergeRequest":{"enabledAt":"x"}}}}}"#, + ), + ]); + let client = server.client(Some("tok")); + let auto = AutoMergeRef { + merge_queue: true, + ..unarmed() + }; + assert_eq!( + merge_approved_pr(&client, &gh_repo(), 42, allowed(Some("abc123"), &auto)), + MergeAttempt::AutoMergeArmed + ); + let reqs = server.requests(); + assert_eq!(reqs.len(), 3, "status lookup, settings, arming mutation"); + assert_eq!(reqs[2].path, "/graphql"); +} + +#[test] +fn merge_approved_pr_arms_auto_merge_on_a_review_blocked_pr_but_never_merges_it_directly() { + // Stamped on an earlier tick: only the lookup, no second POST. + let [branch, rules] = base_requires_judged(); + let server = MockServer::start(vec![ + judged_present(), + repo_settings(true, true), + branch, + rules, + MockResponse::json( + 200, + r#"{"data":{"enablePullRequestAutoMerge":{"pullRequest":{"autoMergeRequest":{"enabledAt":"x"}}}}}"#, + ), + ]); + let client = server.client(Some("tok")); + let auto = unarmed(); + let blocked = CiGreenMerge::BlockedOnReview { + changes_requested: false, + head_sha: Some("abc123"), + auto_merge: &auto, + }; + assert_eq!( + merge_approved_pr(&client, &gh_repo(), 42, blocked), + MergeAttempt::AutoMergeArmed + ); + let reqs = server.requests(); + assert_eq!( + reqs.len(), + 5, + "status lookup, settings, gate check (protection + rulesets), arming mutation" + ); + assert_eq!(reqs[0].path, STATUS_PATH); + + // Arming refused (or the repo has auto-merge off): a review-blocked PR + // gets no direct merge attempt either -- GitHub would only refuse it. + let server = MockServer::start(vec![judged_present(), repo_settings(false, true)]); + let client = server.client(Some("tok")); + assert_eq!( + merge_approved_pr(&client, &gh_repo(), 42, blocked), + MergeAttempt::NotMerged + ); + assert_eq!( + server.requests().len(), + 2, + "status lookup and settings only" + ); +} + +#[test] +fn merge_approved_pr_falls_back_to_a_direct_merge_when_arming_auto_merge_is_refused() { + // GitHub refuses to arm auto-merge on a PR it could merge right now + // ("clean status"); the direct merge is the fallback, not a skip. + let [status, stamp] = judged_missing(); + let [branch, rules] = base_requires_judged(); + let server = MockServer::start(vec![ + status, + stamp, + repo_settings(true, true), + branch, + rules, + MockResponse::json( + 200, + r#"{"data":{"enablePullRequestAutoMerge":null},"errors":[{"message":"Pull request is in clean status"}]}"#, + ), + MockResponse::json(200, r#"{"merged":true}"#), + ]); + let client = server.client(Some("tok")); + let auto = unarmed(); + assert_eq!( + merge_approved_pr(&client, &gh_repo(), 42, allowed(Some("abc123"), &auto)), + MergeAttempt::Merged + ); + let reqs = server.requests(); + assert_eq!(reqs[6].path, "/repos/o/r/pulls/42/merge"); +} + +#[test] +fn merge_approved_pr_does_not_rearm_an_already_armed_auto_merge_but_stamps_a_missing_head() { + // Armed on an earlier tick whose stamp POST failed: the stamp is + // retried, the arming is not repeated. + let [status, stamp] = judged_missing(); + let server = MockServer::start(vec![status, stamp, repo_settings(true, true)]); + let client = server.client(Some("tok")); + let auto = AutoMergeRef { + enabled: true, + ..unarmed() + }; + assert_eq!( + merge_approved_pr(&client, &gh_repo(), 42, allowed(Some("abc123"), &auto)), + MergeAttempt::AutoMergeArmed + ); + let reqs = server.requests(); + assert_eq!( + reqs.len(), + 3, + "stamp and settings only: GitHub is already on it" + ); + assert_eq!(reqs[1].path, "/repos/o/r/statuses/abc123"); } #[test] fn merge_approved_pr_skips_when_the_head_moved_since_ci_was_checked() { // GitHub answers 409 when `sha` no longer matches the PR head: an agent // pushed after the sweep's snapshot. That is a plain skip, not a merge. - let server = crate::github::test_support::MockServer::start(vec![ - crate::github::test_support::MockResponse::json( + let [status, stamp] = judged_missing(); + let server = MockServer::start(vec![ + status, + stamp, + repo_settings(false, true), + MockResponse::json( 409, r#"{"message":"Head branch was modified. Review and try the merge again."}"#, ), ]); let client = server.client(Some("tok")); - let repo = crate::github::RepoId { - owner: "o".into(), - repo: "r".into(), + let auto = AutoMergeRef::default(); + + assert_eq!( + merge_approved_pr(&client, &gh_repo(), 42, allowed(Some("stale"), &auto)), + MergeAttempt::NotMerged + ); + assert_eq!(server.requests().len(), 4, "no retry within the tick"); +} + +#[test] +fn disarm_auto_merge_with_sends_the_disable_mutation_and_soft_fails() { + let server = MockServer::start(vec![ + MockResponse::json( + 200, + r#"{"data":{"disablePullRequestAutoMerge":{"pullRequest":{"number":42}}}}"#, + ), + MockResponse::json(500, r#"{"message":"boom"}"#), + ]); + let client = server.client(Some("tok")); + assert!(disarm_auto_merge_with( + &client, + &gh_repo(), + 42, + "PR_1", + "test" + )); + // A failure only logs and reports false, so the caller keeps its + // record and retries next tick; the sweep must not panic over it. + assert!(!disarm_auto_merge_with( + &client, + &gh_repo(), + 42, + "PR_1", + "test" + )); + let reqs = server.requests(); + assert_eq!(reqs.len(), 2); + let sent: serde_json::Value = serde_json::from_str(&reqs[0].body).unwrap(); + assert!( + sent["query"] + .as_str() + .unwrap() + .contains("disablePullRequestAutoMerge") + ); +} + +fn batch_snapshot( + head: &str, + auto_merge_enabled: bool, + merged: bool, +) -> crate::github::graphql::BatchPrData { + crate::github::graphql::BatchPrData { + merged, + closed: false, + mergeable: Some(true), + mergeable_state: Some("clean".into()), + checks: vec![], + labels: vec![], + head_sha: Some(head.to_string()), + review_decision: None, + rollup_state: None, + node_id: Some("PR_1".into()), + auto_merge_enabled, + merge_queue_enabled: false, + in_merge_queue: false, + is_draft: false, + base_ref: Some("main".into()), + } +} + +#[test] +fn judge_armed_auto_merge_disarms_on_a_moved_head_and_forgets_a_gone_arming() { + let armed = ArmedAutoMerge { + head: "abc".into(), + node_id: "PR_1".into(), }; + // Same head, still armed: leave it. + assert_eq!( + judge_armed_auto_merge(&armed, Some(&batch_snapshot("abc", true, false))), + ArmedVerdict::Keep + ); + // A push moved the head: the new commits were never judged. + assert_eq!( + judge_armed_auto_merge(&armed, Some(&batch_snapshot("def", true, false))), + ArmedVerdict::Disarm + ); + // GitHub no longer has it armed (someone disarmed it, or it merged). + assert_eq!( + judge_armed_auto_merge(&armed, Some(&batch_snapshot("def", false, false))), + ArmedVerdict::Forget + ); + assert_eq!( + judge_armed_auto_merge(&armed, Some(&batch_snapshot("abc", true, true))), + ArmedVerdict::Forget + ); + // No snapshot this tick: judge next tick instead. + assert_eq!(judge_armed_auto_merge(&armed, None), ArmedVerdict::Keep); + // Armed without a known head: a move can't be judged, so it is kept. + let headless = ArmedAutoMerge { + head: String::new(), + node_id: "PR_1".into(), + }; + assert_eq!( + judge_armed_auto_merge(&headless, Some(&batch_snapshot("def", true, false))), + ArmedVerdict::Keep + ); +} - assert!(!merge_approved_pr(&client, &repo, 42, Some("stale"))); - assert_eq!(server.requests().len(), 1, "no retry within the tick"); +#[test] +fn armed_auto_merge_is_recorded_in_pr_metadata_and_cleared_again() { + let repo = throwaway_repo(); + let mcp = test_mcp_with_repo(repo.path().to_path_buf()); + let item_id = seed_in_review_item(&mcp, Some("claude-code")); + let fetch = || { + mcp.with_backend_db(|conn| agentflare_backend::item::get(conn, &item_id).unwrap()) + .unwrap() + }; + // Existing `pr` fields survive the merge. + mcp.with_backend_db(|conn| { + crate::mcp_server::merge_item_metadata(conn, &item_id, |m| { + m.insert( + "pr".into(), + serde_json::json!({"number": 42, "branch": "task/1"}), + ); + }) + .unwrap() + }) + .unwrap(); + let item = fetch(); + assert_eq!(armed_auto_merge(&item), None, "nothing recorded yet"); + // Nothing recorded: the label-absent path must not touch the network + // (this repo has no remote, so a call would have nothing to reach + // either way -- the assertion is on the return value). + assert!(!disarm_our_auto_merge(&mcp, &item, repo.path(), 42, "test")); + + record_armed_auto_merge(&mcp, &item, Some("abc"), "PR_1"); + let item = fetch(); + assert_eq!( + armed_auto_merge(&item), + Some(ArmedAutoMerge { + head: "abc".into(), + node_id: "PR_1".into(), + }) + ); + assert_eq!(crate::worktree::pr_number_from_metadata(&item), Some(42)); + + // A gone arming is forgotten without any GitHub call. + reconcile_armed_auto_merge( + &mcp, + &item, + repo.path(), + 42, + Some(&batch_snapshot("abc", false, false)), + ); + assert_eq!(armed_auto_merge(&fetch()), None); + assert_eq!(crate::worktree::pr_number_from_metadata(&fetch()), Some(42)); } #[test] @@ -110,16 +630,17 @@ fn merge_approved_pr_returns_false_and_does_not_panic_on_github_error() { // the merge endpoint. The safety property is that this falls through to // `skipped` (no panic, no retry loop here); the sweep just polls again // next tick. - let server = crate::github::test_support::MockServer::start(vec![ - crate::github::test_support::MockResponse::json(405, r#"{"message":"not mergeable"}"#), + let server = MockServer::start(vec![ + repo_settings(false, true), + MockResponse::json(405, r#"{"message":"not mergeable"}"#), ]); let client = server.client(Some("tok")); - let repo = crate::github::RepoId { - owner: "o".into(), - repo: "r".into(), - }; + let auto = AutoMergeRef::default(); - assert!(!merge_approved_pr(&client, &repo, 42, None)); + assert_eq!( + merge_approved_pr(&client, &gh_repo(), 42, allowed(None, &auto)), + MergeAttempt::NotMerged + ); } #[test] @@ -134,7 +655,15 @@ fn merge_if_approved_skips_without_touching_network_when_label_is_absent() { .with_backend_db(|conn| agentflare_backend::item::get(conn, &item_id).unwrap()) .unwrap(); - let merged = merge_if_approved(&mcp, &item, repo.path(), 42, &["size/s".to_string()], None); + let auto = AutoMergeRef::default(); + let merged = merge_if_approved( + &mcp, + &item, + repo.path(), + 42, + &["size/s".to_string()], + allowed(None, &auto), + ); assert!(!merged); let still_in_review = mcp @@ -219,7 +748,11 @@ fn merge_or_repair_findings_never_attempts_a_merge_while_github_awaits_review() &[PR_APPROVAL_LABEL.to_string()], &label_id_by_name, "/repo", - CiGreenMerge::BlockedOnReview, + CiGreenMerge::BlockedOnReview { + changes_requested: false, + head_sha: None, + auto_merge: &AutoMergeRef::default(), + }, ); assert!(matches!(outcome, PassingPrOutcome::NotMerged)); @@ -256,7 +789,7 @@ fn merge_or_repair_findings_never_merges_a_ci_green_approved_pr_with_unresolved_ &[PR_APPROVAL_LABEL.to_string()], &label_id_by_name, "/repo", - CiGreenMerge::Allowed { head_sha: None }, + allowed(None, &AutoMergeRef::default()), ); assert!( diff --git a/src/worktree/draft.rs b/src/worktree/draft.rs new file mode 100644 index 000000000..70b33e02e --- /dev/null +++ b/src/worktree/draft.rs @@ -0,0 +1,228 @@ +//! The draft half of the PR lifecycle: `push_and_open_pr` opens every PR +//! as a draft, and this module flips it to "ready for review" once +//! `item_done` has moved the item to in_review -- and again from +//! `supervisor::run_review_sweep` if that first flip never landed, so no +//! in-review item stalls behind a draft nobody was asked to look at. +//! Split out of `mod.rs` to keep it under the repo's line-count gate. +//! +//! `metadata.pr.ready` records that agentflare itself marked the PR ready. +//! The sweep only flips a draft whose flag is missing: a draft that carries +//! it was converted back by a human, on purpose, and is left alone. + +use crate::github::identity::RepoId; + +/// True once agentflare has marked this item's PR ready for review +/// (`metadata.pr.ready`), so a later draft is a human's doing. +pub(crate) fn pr_marked_ready(item: &agentflare_backend::item::Item) -> bool { + serde_json::from_str::(&item.metadata) + .ok() + .is_some_and(|m| m["pr"]["ready"] == true) +} + +/// Marks PR `number` ready for review and records it on the item. Soft- +/// fails like the rest of the PR path: no resolvable remote or credentials +/// just means "not now", and the sweep retries next tick. Returns whether +/// the PR is known ready afterwards. +pub(crate) fn mark_pr_ready( + item: &agentflare_backend::item::Item, + repo_root: &std::path::Path, + number: u64, + node_id: Option<&str>, +) -> bool { + let Some(repo) = RepoId::resolve_from_remote(repo_root) else { + return false; + }; + let Ok(client) = crate::github::Client::new() else { + return false; + }; + match mark_pr_ready_with(&client, &repo, number, node_id) { + Ok(()) => { + persist_pr_ready(item); + true + } + Err(e) => { + eprintln!( + "worktree: could not mark PR #{number} in {repo} ready for review for item {}: {}", + item.id, + e.log_safe() + ); + false + } + } +} + +/// `mark_pr_ready`'s GitHub half, split out for mock-server tests. Fetches +/// the node id when the caller has none (the REST list endpoint the +/// branch-heuristic path uses carries it too, but an older `metadata.pr` +/// record may not), and treats GitHub's "already ready" refusal as success. +pub(crate) fn mark_pr_ready_with( + client: &crate::github::Client, + repo: &RepoId, + number: u64, + node_id: Option<&str>, +) -> Result<(), crate::github::GitHubError> { + let node_id = match node_id { + Some(id) => id.to_string(), + None => { + let pr = crate::github::pulls::get(client, repo, number)?; + if !pr.draft { + return Ok(()); + } + pr.node_id.ok_or_else(|| { + crate::github::GitHubError::Parse(format!("PR #{number} has no node_id")) + })? + } + }; + match crate::github::graphql::mark_ready_for_review(client, &node_id) { + Ok(()) => Ok(()), + Err(e) if e.to_string().to_lowercase().contains("not a draft") => Ok(()), + Err(e) => Err(e), + } +} + +/// Records `metadata.pr.ready = true` on the item, merged into the existing +/// `pr` record (number, branch) rather than replacing it. Best-effort, same +/// as `persist_pr_identity`: a db hiccup here costs at most one redundant +/// ready-for-review call from the sweep. +fn persist_pr_ready(item: &agentflare_backend::item::Item) { + let conn = match agentflare_backend::db::open_db(&crate::vent::paths::backend_db_path()) { + Ok(conn) => conn, + Err(e) => { + eprintln!( + "worktree: could not open backend db to record PR readiness for item {}: {e}", + item.id + ); + return; + } + }; + persist_pr_ready_in(&conn, item); +} + +pub(crate) fn persist_pr_ready_in( + conn: &rusqlite::Connection, + item: &agentflare_backend::item::Item, +) { + if let Err(e) = crate::mcp_server::merge_item_metadata(conn, &item.id, |merged| { + let mut pr = merged + .get("pr") + .and_then(|v| v.as_object()) + .cloned() + .unwrap_or_default(); + pr.insert("ready".into(), serde_json::Value::Bool(true)); + merged.insert("pr".into(), serde_json::Value::Object(pr)); + }) { + eprintln!( + "worktree: could not record PR readiness for item {}: {e}", + item.id + ); + } +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::github::test_support::{MockResponse, MockServer}; + + fn repo() -> RepoId { + RepoId { + owner: "o".into(), + repo: "r".into(), + } + } + + fn item_with_metadata(metadata: &str) -> agentflare_backend::item::Item { + agentflare_backend::item::Item { + id: "item-7".into(), + project_id: "p".into(), + state_id: "s".into(), + name: "n".into(), + description: String::new(), + priority: "none".into(), + parent_id: None, + assignee_agent: None, + sequence_id: 7, + sort_order: 0.0, + started_at: None, + completed_at: None, + archived_at: None, + external_source: None, + external_id: None, + metadata: metadata.into(), + created_at: 0, + updated_at: 0, + deleted_at: None, + start_date: None, + due_date: None, + } + } + + #[test] + fn pr_marked_ready_reads_the_flag_and_defaults_false() { + assert!(pr_marked_ready(&item_with_metadata( + r#"{"pr":{"number":70,"branch":"task/7","ready":true}}"# + ))); + assert!(!pr_marked_ready(&item_with_metadata( + r#"{"pr":{"number":70,"branch":"task/7"}}"# + ))); + assert!(!pr_marked_ready(&item_with_metadata("not json"))); + } + + #[test] + fn mark_pr_ready_with_uses_the_given_node_id_without_a_lookup() { + let server = MockServer::start(vec![MockResponse::json( + 200, + r#"{"data":{"markPullRequestReadyForReview":{"pullRequest":{"isDraft":false}}}}"#, + )]); + let client = server.client(Some("tok")); + mark_pr_ready_with(&client, &repo(), 70, Some("PR_70")).unwrap(); + let reqs = server.requests(); + assert_eq!(reqs.len(), 1); + assert_eq!(reqs[0].path, "/graphql"); + let sent: serde_json::Value = serde_json::from_str(&reqs[0].body).unwrap(); + assert_eq!(sent["variables"]["input"]["pullRequestId"], "PR_70"); + } + + #[test] + fn mark_pr_ready_with_looks_the_node_id_up_when_missing_and_skips_a_ready_pr() { + let server = MockServer::start(vec![ + MockResponse::json( + 200, + r#"{"number":70,"html_url":"u","state":"open","title":"t","draft":true,"node_id":"PR_70"}"#, + ), + MockResponse::json( + 200, + r#"{"data":{"markPullRequestReadyForReview":{"pullRequest":{"isDraft":false}}}}"#, + ), + MockResponse::json( + 200, + r#"{"number":71,"html_url":"u","state":"open","title":"t","draft":false,"node_id":"PR_71"}"#, + ), + ]); + let client = server.client(Some("tok")); + mark_pr_ready_with(&client, &repo(), 70, None).unwrap(); + mark_pr_ready_with(&client, &repo(), 71, None).unwrap(); + let reqs = server.requests(); + assert_eq!(reqs.len(), 3, "an already-ready PR needs no mutation"); + assert_eq!(reqs[0].path, "/repos/o/r/pulls/70"); + let sent: serde_json::Value = serde_json::from_str(&reqs[1].body).unwrap(); + assert_eq!(sent["variables"]["input"]["pullRequestId"], "PR_70"); + assert_eq!(reqs[2].path, "/repos/o/r/pulls/71"); + } + + #[test] + fn mark_pr_ready_with_treats_githubs_already_ready_refusal_as_success() { + let server = MockServer::start(vec![MockResponse::json( + 200, + r#"{"data":{"markPullRequestReadyForReview":null},"errors":[{"message":"Pull request is not a draft"}]}"#, + )]); + let client = server.client(Some("tok")); + assert!(mark_pr_ready_with(&client, &repo(), 70, Some("PR_70")).is_ok()); + } + + #[test] + fn mark_pr_ready_with_surfaces_other_errors() { + let server = MockServer::start(vec![MockResponse::json(500, r#"{"message":"boom"}"#)]); + let client = server.client(Some("tok")); + assert!(mark_pr_ready_with(&client, &repo(), 70, Some("PR_70")).is_err()); + } +} diff --git a/src/worktree/mod.rs b/src/worktree/mod.rs index f0ba1f542..ecafa001f 100644 --- a/src/worktree/mod.rs +++ b/src/worktree/mod.rs @@ -98,6 +98,8 @@ pub(crate) fn pr_number_from_metadata(item: &agentflare_backend::item::Item) -> mod discovery; pub(crate) use discovery::{discover_untracked_prs, tracked_pr_numbers}; +mod draft; +pub(crate) use draft::{mark_pr_ready, pr_marked_ready}; /// Checks whether `item`'s branch already has a merged PR — the promotion /// signal `check_merge` uses to move an item out of "in_review" (item @@ -254,6 +256,25 @@ fn find_own_pr_by_branch( Some(pr.number) } +/// What GitHub's native auto-merge needs beyond a PR number: the PR's +/// GraphQL node id (the `enablePullRequestAutoMerge` mutation's +/// `pullRequestId`), whether auto-merge is already armed on it (so the +/// sweep neither re-arms it every tick nor arms it blind), and what decides +/// whether arming is safe at all -- the base branch, whose protection may +/// require `supervisor::JUDGED_STATUS_CONTEXT`, and whether it merges +/// through a merge queue. All from the same fetch as the CI verdict +/// (GraphQL `id`/`autoMergeRequest`/`baseRefName`/`isMergeQueueEnabled`, +/// REST `node_id`/`auto_merge`/`base.ref`). +#[derive(Debug, Clone, Default, PartialEq, Eq)] +pub struct AutoMergeRef { + pub node_id: Option, + pub enabled: bool, + pub base_ref: Option, + /// The base branch merges through a merge queue (GraphQL only; REST + /// can't tell and reports `false`). + pub merge_queue: bool, +} + /// CI signal the in-review sweep (`supervisor::run_review_sweep`, item #65) /// polls per item: merged (promote), failing (self-repair), CI-green with a /// human approval label attached (auto-merge, item #194), cleanly behind the @@ -295,6 +316,7 @@ pub enum PrCiStatus { number: u64, labels: Vec, head_sha: Option, + auto_merge: AutoMergeRef, }, /// Required CI is green but GitHub's `mergeStateStatus` is `BLOCKED` on /// review: branch protection wants an approving review @@ -306,6 +328,16 @@ pub enum PrCiStatus { AwaitingReview { number: u64, labels: Vec, + /// `reviewDecision == CHANGES_REQUESTED` (a reviewer asked for + /// changes) as opposed to `REVIEW_REQUIRED` (nobody has approved + /// yet) -- the approval card tells the human which it is. + changes_requested: bool, + /// The head the green verdict was made on, and the auto-merge + /// handle: with the approval label attached and no findings, the + /// sweep arms GitHub's auto-merge here so the review landing is + /// all it takes to merge. + head_sha: Option, + auto_merge: AutoMergeRef, }, /// GitHub's own `mergeable_state == "behind"` -- mergeable, no conflict, /// just missing commits the base branch has gained since this PR was @@ -335,6 +367,19 @@ pub enum PrCiStatus { Closed { number: u64, }, + /// Still a draft. `push_and_open_pr` opens every PR as one and + /// `item_done` marks it ready once the item is in review, so a draft on + /// an in-review item means that flip never landed (`run_review_sweep` + /// retries it, see `draft::mark_pr_ready`) -- or a human converted it + /// back on purpose, which the sweep respects. Checked before CI state: + /// a draft can neither merge nor be handed to reviewers, so its checks + /// are not yet actionable either way. + Draft { + number: u64, + /// The GraphQL node id the ready-for-review mutation needs, when the + /// fetch had it. + node_id: Option, + }, Unknown, } @@ -405,6 +450,12 @@ fn pr_ci_status_impl( if pr.state == "closed" { return PrCiStatus::Closed { number: pr.number }; } + if pr.draft { + return PrCiStatus::Draft { + number: pr.number, + node_id: pr.node_id.clone(), + }; + } let head_sha = pr .head .as_ref() @@ -455,6 +506,16 @@ fn pr_ci_status_impl( review_decision: None, rollup_state: None, head_sha: Some(&sha), + auto_merge: AutoMergeRef { + node_id: pr.node_id.clone(), + enabled: pr.auto_merge.is_some(), + base_ref: pr.base.as_ref().map(|b| b.git_ref.clone()), + merge_queue: false, + }, + // REST doesn't say; a merge-queue repo's PR stays `Pending` + // on this path, as it did before. + merge_queue_enabled: false, + in_merge_queue: false, }, ) } @@ -470,6 +531,11 @@ struct MergeSignals<'a> { /// Upper-case GraphQL `statusCheckRollup.state`; always `None` on REST. rollup_state: Option<&'a str>, head_sha: Option<&'a str>, + auto_merge: AutoMergeRef, + /// The base branch merges through a merge queue (GraphQL only). + merge_queue_enabled: bool, + /// The PR is already in that queue (GraphQL only). + in_merge_queue: bool, } /// The part of the CI-status decision tree that only needs check-run data @@ -501,6 +567,7 @@ fn decide_from_checks( number, labels, head_sha: signals.head_sha.map(str::to_string), + auto_merge: signals.auto_merge.clone(), }; let pending = || PrCiStatus::Pending { number, @@ -511,6 +578,18 @@ fn decide_from_checks( signals.review_decision, Some("REVIEW_REQUIRED") | Some("CHANGES_REQUESTED") ); + let awaiting = |labels: Vec| PrCiStatus::AwaitingReview { + number, + labels, + changes_requested: signals.review_decision == Some("CHANGES_REQUESTED"), + head_sha: signals.head_sha.map(str::to_string), + auto_merge: signals.auto_merge.clone(), + }; + // Enqueued: the merge queue's own CI run on the merge group decides + // now, and GitHub merges (or kicks it back out) by itself. + if signals.in_merge_queue { + return pending(); + } let relevant: Vec = if checks.iter().any(|c| c.required) { checks.iter().filter(|c| c.required).cloned().collect() } else { @@ -534,7 +613,7 @@ fn decide_from_checks( return passing(labels); } if awaiting_review { - return PrCiStatus::AwaitingReview { number, labels }; + return awaiting(labels); } } return pending(); @@ -562,7 +641,7 @@ fn decide_from_checks( // purely on review: hand it to the approval gate rather than polling a // PR that only a human can unblock. if awaiting_review { - return PrCiStatus::AwaitingReview { number, labels }; + return awaiting(labels); } // The check-run list above only reflects what GitHub has created so far -- // gated jobs (e.g. a `build` matrix behind a `changes` job) may not exist @@ -578,6 +657,16 @@ fn decide_from_checks( // that's the exact same incomplete-snapshot window, just caught one tick // earlier, so it gets the same treatment. if matches!(signals.mergeable_state, Some("blocked") | Some("unknown")) { + // A base branch with a merge queue never reports "clean": a direct + // merge is refused there by design, and the PR reads "blocked" + // until it goes through the queue. With every required context + // green and no review outstanding, that is the queue's cue -- the + // sweep's merge path arms auto-merge, which is how a PR enters the + // queue, and the queue's own CI run on the merge group is the real + // gate for whatever a still-missing gated job would have covered. + if signals.merge_queue_enabled && signals.mergeable_state == Some("blocked") { + return passing(labels); + } return pending(); } passing(labels) @@ -601,6 +690,12 @@ pub(crate) fn pr_ci_status_from_batch( if data.closed { return PrCiStatus::Closed { number }; } + if data.is_draft { + return PrCiStatus::Draft { + number, + node_id: data.node_id.clone(), + }; + } if data.mergeable == Some(true) && data.mergeable_state.as_deref() == Some("behind") { return PrCiStatus::Behind { number, @@ -620,6 +715,14 @@ pub(crate) fn pr_ci_status_from_batch( review_decision: data.review_decision.as_deref(), rollup_state: data.rollup_state.as_deref(), head_sha: data.head_sha.as_deref(), + auto_merge: AutoMergeRef { + node_id: data.node_id.clone(), + enabled: data.auto_merge_enabled, + base_ref: data.base_ref.clone(), + merge_queue: data.merge_queue_enabled, + }, + merge_queue_enabled: data.merge_queue_enabled, + in_merge_queue: data.in_merge_queue, }, ) } @@ -821,8 +924,18 @@ fn merge_and_persist_pr_identity( number: u64, branch: &str, ) { - let pr = serde_json::json!({ "number": number, "branch": branch }); if let Err(e) = crate::mcp_server::merge_item_metadata(conn, &item.id, |merged| { + // `pr.ready` (see `draft::persist_pr_ready`) survives a re-run of + // `done` that finds the same PR again; a different PR number is a + // different PR, whose readiness is unknown. + let ready = merged + .get("pr") + .filter(|pr| pr["number"].as_u64() == Some(number)) + .is_some_and(|pr| pr["ready"] == true); + let mut pr = serde_json::json!({ "number": number, "branch": branch }); + if ready { + pr["ready"] = serde_json::Value::Bool(true); + } merged.insert("pr".into(), pr); }) { eprintln!( @@ -885,11 +998,33 @@ fn recover_pr_after_failed_create( /// never result here, by configuration" apart from "this attempt failed and /// a retry may succeed", which the old bare `Option` couldn't: a /// repo with no GitHub remote or no credentials errored `done` forever. +/// The PR `push_and_open_pr` created or found, as much of it as `item_done` +/// needs afterwards: the URL for its response, and what flipping a draft to +/// ready takes (`draft::mark_pr_ready`) without a second lookup. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct OpenedPr { + pub url: String, + pub number: u64, + pub node_id: Option, + pub draft: bool, +} + +impl From for OpenedPr { + fn from(pr: crate::github::models::PullRequest) -> Self { + OpenedPr { + url: pr.html_url, + number: pr.number, + node_id: pr.node_id, + draft: pr.draft, + } + } +} + #[derive(Debug, PartialEq, Eq)] pub enum PrOutcome { /// A PR exists for this item's branch (freshly created, or already - /// open/merged from an earlier `done`). Carries its URL. - Opened(String), + /// open/merged from an earlier `done`). + Opened(OpenedPr), /// Nothing to publish: no worktree was ever created, or the branch never /// diverged from its target. NothingToPush, @@ -1042,7 +1177,7 @@ fn open_pr_for_pushed_branch( if let Some(p) = progress { p.send(1.0, Some(1.0), Some("PR already exists".into())); } - PrOutcome::Opened(existing.html_url) + PrOutcome::Opened(existing.into()) }; if let Some(number) = pr_number_from_metadata(item) { match crate::github::pulls::get(client, repo, number) { @@ -1091,14 +1226,35 @@ fn open_pr_for_pushed_branch( return PrOutcome::Failed(format!("could not check for an existing PR: {e}")); } } - match crate::github::pulls::create( - client, - repo, - &conventional_pr_title(&item.name), - branch, - target_branch, - Some(body), - ) { + // Opened as a draft: CI starts, but nobody is asked to review and + // nothing can merge until `item_done` has recorded the item as + // in_review and flips it ready (`draft::mark_pr_ready`) -- so a `done` + // that fails between here and there leaves a PR nobody is chasing yet. + let title = conventional_pr_title(&item.name); + let create = |draft: bool| { + crate::github::pulls::create( + client, + repo, + &title, + branch, + target_branch, + Some(body), + draft, + ) + }; + let created = match create(true) { + Err(e) if crate::github::pulls::drafts_unsupported(&e) => { + eprintln!( + "worktree: {repo} does not support draft PRs ({}); opening item {}'s PR as \ + ready for review", + e.log_safe(), + item.id + ); + create(false) + } + other => other, + }; + match created { Ok(pr) => { persist_pr_identity(item, pr.number, branch); if let Err(e) = crate::github::issues::add_labels( @@ -1118,7 +1274,7 @@ fn open_pr_for_pushed_branch( if let Some(p) = progress { p.send(1.0, Some(1.0), Some("PR created".into())); } - PrOutcome::Opened(pr.html_url) + PrOutcome::Opened(pr.into()) } Err(e) => { // `create` fails this way when two workstations independently diff --git a/src/worktree/mod_tests.rs b/src/worktree/mod_tests.rs index 7ac44d56e..8af4b780d 100644 --- a/src/worktree/mod_tests.rs +++ b/src/worktree/mod_tests.rs @@ -423,9 +423,118 @@ head_sha: Some("head-sha".to_string()), review_decision: None, rollup_state: None, + node_id: Some("PR_node".to_string()), + auto_merge_enabled: false, + merge_queue_enabled: false, + in_merge_queue: false, + is_draft: false, + base_ref: Some("main".to_string()), } } + #[test] + fn pr_ci_status_from_batch_reports_draft_before_checks() { + let mut data = batch_data( + false, + Some(true), + Some("clean"), + vec![check("build", "completed", Some("success"))], + vec!["status:pr:approved".into()], + ); + data.is_draft = true; + match pr_ci_status_from_batch(101, &data) { + PrCiStatus::Draft { number, node_id } => { + assert_eq!(number, 101); + assert_eq!(node_id.as_deref(), Some("PR_node")); + } + other => panic!("a draft must never read as mergeable: {other:?}"), + } + } + + #[test] + fn pr_ci_status_rest_path_reports_draft_before_fetching_checks() { + let server = crate::github::test_support::MockServer::start(vec![ + crate::github::test_support::MockResponse::json( + 200, + r#"{"number":623,"html_url":"u","state":"open","title":"t","draft":true,"node_id":"PR_623","mergeable":true,"mergeable_state":"draft","head":{"ref":"b","sha":"abc123"}}"#, + ), + ]); + let client = server.client(Some("tok")); + let item = item_with_metadata(197, r#"{"pr":{"number":623,"branch":"b"}}"#); + let status = pr_ci_status_impl(&item, Path::new("/does/not/exist"), &client, &repo()); + assert!( + matches!(status, PrCiStatus::Draft { number: 623, ref node_id } if node_id.as_deref() == Some("PR_623")), + "{status:?}" + ); + assert_eq!(server.requests().len(), 1, "no check-run fetch for a draft"); + } + + #[test] + fn open_pr_for_pushed_branch_opens_a_draft_and_reports_it() { + let server = crate::github::test_support::MockServer::start(vec![ + crate::github::test_support::MockResponse::json(200, "[]"), + crate::github::test_support::MockResponse::json( + 201, + r#"{"number":71,"html_url":"https://gh/o/r/pull/71","state":"open","title":"t","draft":true,"node_id":"PR_71"}"#, + ), + crate::github::test_support::MockResponse::json(200, "[]"), + ]); + let client = server.client(Some("tok")); + let item = item_with_metadata(7, "{}"); + + let outcome = + open_pr_for_pushed_branch(&client, &repo(), &item, "task/7", "master", "b", "m", None); + + assert_eq!( + outcome, + PrOutcome::Opened(OpenedPr { + url: "https://gh/o/r/pull/71".into(), + number: 71, + node_id: Some("PR_71".into()), + draft: true, + }) + ); + let reqs = server.requests(); + assert_eq!(reqs[1].method, "POST"); + let sent: serde_json::Value = serde_json::from_str(&reqs[1].body).unwrap(); + assert_eq!(sent["draft"], true); + assert_eq!(reqs[2].method, "POST", "labels are added after the create"); + } + + #[test] + fn open_pr_for_pushed_branch_retries_without_draft_where_drafts_are_unsupported() { + let server = crate::github::test_support::MockServer::start(vec![ + crate::github::test_support::MockResponse::json(200, "[]"), + crate::github::test_support::MockResponse::json( + 422, + r#"{"message":"Draft pull requests are not supported in this repository."}"#, + ), + crate::github::test_support::MockResponse::json( + 201, + r#"{"number":72,"html_url":"https://gh/o/r/pull/72","state":"open","title":"t","draft":false,"node_id":"PR_72"}"#, + ), + crate::github::test_support::MockResponse::json(200, "[]"), + ]); + let client = server.client(Some("tok")); + let item = item_with_metadata(7, "{}"); + + let outcome = + open_pr_for_pushed_branch(&client, &repo(), &item, "task/7", "master", "b", "m", None); + + match outcome { + PrOutcome::Opened(pr) => { + assert_eq!(pr.number, 72); + assert!(!pr.draft); + } + other => panic!("expected Opened, got {other:?}"), + } + let reqs = server.requests(); + let first: serde_json::Value = serde_json::from_str(&reqs[1].body).unwrap(); + assert_eq!(first["draft"], true); + let second: serde_json::Value = serde_json::from_str(&reqs[2].body).unwrap(); + assert!(second.get("draft").is_none()); + } + #[test] fn pr_ci_status_from_batch_reports_merged_before_looking_at_anything_else() { let data = batch_data(true, Some(true), Some("behind"), vec![], vec![]); @@ -467,15 +576,75 @@ number, labels, head_sha, + auto_merge, } => { assert_eq!(number, 101); assert_eq!(labels, vec!["status:pr:approved".to_string()]); assert_eq!(head_sha.as_deref(), Some("head-sha")); + assert_eq!(auto_merge.node_id.as_deref(), Some("PR_node")); + assert!(!auto_merge.enabled); } other => panic!("expected Passing, got {other:?}"), } } + #[test] + fn pr_ci_status_from_batch_reports_passing_when_only_a_merge_queue_blocks_green_checks() { + // On a merge-queue base branch GitHub never says "clean"; with every + // required check green and no review due, "blocked" is the queue's + // cue, not an incomplete snapshot. + let mut data = batch_data( + false, + Some(true), + Some("blocked"), + vec![required(check("build", "completed", Some("success")))], + vec![], + ); + data.merge_queue_enabled = true; + assert!(matches!( + pr_ci_status_from_batch(101, &data), + PrCiStatus::Passing { number: 101, .. } + )); + // Already enqueued: the queue's own CI decides; nothing to do here. + data.in_merge_queue = true; + assert!(matches!( + pr_ci_status_from_batch(101, &data), + PrCiStatus::Pending { .. } + )); + // A review still due wins over the queue. + data.in_merge_queue = false; + data.review_decision = Some("REVIEW_REQUIRED".to_string()); + assert!(matches!( + pr_ci_status_from_batch(101, &data), + PrCiStatus::AwaitingReview { .. } + )); + } + + #[test] + fn pr_ci_status_from_batch_awaiting_review_carries_the_auto_merge_handle() { + let mut data = batch_data( + false, + Some(true), + Some("blocked"), + vec![required(check("build", "completed", Some("success")))], + vec![], + ); + data.review_decision = Some("REVIEW_REQUIRED".to_string()); + data.auto_merge_enabled = true; + match pr_ci_status_from_batch(101, &data) { + PrCiStatus::AwaitingReview { + head_sha, + auto_merge, + .. + } => { + assert_eq!(head_sha.as_deref(), Some("head-sha")); + assert_eq!(auto_merge.node_id.as_deref(), Some("PR_node")); + assert!(auto_merge.enabled); + } + other => panic!("expected AwaitingReview, got {other:?}"), + } + } + #[test] fn pr_ci_status_from_batch_reports_failing_checks_by_name() { let data = batch_data( @@ -586,13 +755,18 @@ vec![], ); data.review_decision = Some(decision.to_string()); - assert!( - matches!( - pr_ci_status_from_batch(101, &data), - PrCiStatus::AwaitingReview { number: 101, .. } + match pr_ci_status_from_batch(101, &data) { + PrCiStatus::AwaitingReview { + number: 101, + changes_requested, + .. + } => assert_eq!( + changes_requested, + decision == "CHANGES_REQUESTED", + "{decision}" ), - "{decision}" - ); + other => panic!("{decision}: expected AwaitingReview, got {other:?}"), + } } }