diff --git a/src/ci.rs b/src/ci.rs new file mode 100644 index 000000000..7b286f4d3 --- /dev/null +++ b/src/ci.rs @@ -0,0 +1,70 @@ +//! Watching CI on the Issue branch's head commit. + +use anyhow::Result; + +use crate::github::{self, Check, CheckState}; +use crate::issue::IssueUrl; +use crate::poll; +use crate::progress; + +/// How CI on a commit ended. +pub enum Ci { + /// No check or status appeared within the grace period: no CI, which + /// counts as passing. + Absent, + Passed, + /// Red, with the checks that failed. + Failed(Vec), +} + +/// Wait up to the grace period for any check or status on `sha`, then watch +/// them until they have all finished. +pub fn watch(issue: &IssueUrl, sha: &str) -> Result { + let grace = poll::grace_period(); + let short = &sha[..sha.len().min(7)]; + progress::step(format_args!( + "waiting up to {}s for CI on {short}", + grace.as_secs() + )); + let appeared = poll::within(grace, || { + Ok((!github::checks_on(issue, sha)?.is_empty()).then_some(())) + })?; + if appeared.is_none() { + progress::step(format_args!( + "no CI checks appeared on {short}; counting that as passing" + )); + return Ok(Ci::Absent); + } + + let mut reported_pending = None; + let checks = poll::until(|| { + let checks = github::checks_on(issue, sha)?; + let pending = count(&checks, CheckState::Pending); + if pending == 0 { + return Ok(Some(checks)); + } + if reported_pending != Some(pending) { + progress::step(format_args!( + "CI on {short}: {pending} of {} checks still running", + checks.len() + )); + reported_pending = Some(pending); + } + Ok(None) + })?; + let failed: Vec = checks + .into_iter() + .filter(|check| check.state == CheckState::Failed) + .collect(); + if failed.is_empty() { + progress::step(format_args!("CI passed on {short}")); + return Ok(Ci::Passed); + } + let names: Vec<&str> = failed.iter().map(|check| check.name.as_str()).collect(); + progress::step(format_args!("CI failed on {short}: {}", names.join(", "))); + Ok(Ci::Failed(failed)) +} + +fn count(checks: &[Check], state: CheckState) -> usize { + checks.iter().filter(|check| check.state == state).count() +} diff --git a/src/github.rs b/src/github.rs index 39bef52ea..f3ee63f2b 100644 --- a/src/github.rs +++ b/src/github.rs @@ -142,6 +142,96 @@ pub fn convert_to_draft(issue: &IssueUrl, branch: &str) -> Result<()> { ]) } +/// Whether a pull request can be merged into its base. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum Mergeable { + Yes, + /// It conflicts with its base. + No, + /// GitHub is still working it out. + Unknown, +} + +/// Whether the pull request whose head is `branch` can be merged. +pub fn mergeable(issue: &IssueUrl, branch: &str) -> Result { + let json = gh_json(&[ + "pr", + "view", + branch, + "--repo", + &issue.repo_slug(), + "--json", + "mergeable", + ])?; + match json["mergeable"].as_str() { + Some("MERGEABLE") => Ok(Mergeable::Yes), + Some("CONFLICTING") => Ok(Mergeable::No), + Some("UNKNOWN") => Ok(Mergeable::Unknown), + other => bail!("gh output has an unknown mergeable state {other:?}"), + } +} + +/// Where a check or status stands. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum CheckState { + Pending, + Passed, + Failed, +} + +/// A check run or a commit status. +pub struct Check { + pub name: String, + pub state: CheckState, + pub url: Option, +} + +/// Every check run and commit status on commit `sha`. +pub fn checks_on(issue: &IssueUrl, sha: &str) -> Result> { + let commit = format!("repos/{}/commits/{sha}", issue.repo_slug()); + let runs = gh_api_items(&format!("{commit}/check-runs?per_page=100"), "check_runs")?; + let statuses = gh_api_items(&format!("{commit}/status?per_page=100"), "statuses")?; + let url = |value: &Value| { + value + .as_str() + .filter(|url| !url.is_empty()) + .map(String::from) + }; + + let mut checks = Vec::new(); + for run in &runs { + let state = match (run["status"].as_str(), run["conclusion"].as_str()) { + (Some("completed"), Some("success" | "neutral" | "skipped")) => CheckState::Passed, + (Some("completed"), _) => CheckState::Failed, + _ => CheckState::Pending, + }; + checks.push(Check { + name: run["name"] + .as_str() + .context("a check run has no name")? + .to_string(), + state, + url: url(&run["details_url"]).or_else(|| url(&run["html_url"])), + }); + } + for status in &statuses { + let state = match status["state"].as_str() { + Some("success") => CheckState::Passed, + Some("pending") => CheckState::Pending, + _ => CheckState::Failed, + }; + checks.push(Check { + name: status["context"] + .as_str() + .context("a commit status has no context")? + .to_string(), + state, + url: url(&status["target_url"]), + }); + } + Ok(checks) +} + /// Run `gh `, failing with its stderr if it exits non-zero. fn gh(args: &[&str]) -> Result<()> { let output = Command::new("gh") @@ -158,6 +248,25 @@ fn gh(args: &[&str]) -> Result<()> { Ok(()) } +/// Every item of the `field` list of the GitHub API's `path`, across all its +/// pages. +fn gh_api_items(path: &str, field: &str) -> Result> { + let output = Command::new("gh") + .args(["api", "--paginate", "--jq", &format!(".{field}[]"), path]) + .output() + .context("could not run gh")?; + if !output.status.success() { + bail!( + "gh api {path} failed: {}", + String::from_utf8_lossy(&output.stderr).trim() + ); + } + serde_json::Deserializer::from_slice(&output.stdout) + .into_iter() + .collect::>() + .with_context(|| format!("gh api {path} returned invalid JSON")) +} + /// Run `gh ` and parse its stdout as JSON, failing with gh's stderr if /// it exits non-zero. fn gh_json(args: &[&str]) -> Result { diff --git a/src/main.rs b/src/main.rs index 9edd196b6..fff33f049 100644 --- a/src/main.rs +++ b/src/main.rs @@ -1,10 +1,12 @@ mod branch; +mod ci; mod failed_run; mod git; mod github; mod interrupt; mod issue; mod plugin; +mod poll; mod preflight; mod progress; mod prompt; diff --git a/src/poll.rs b/src/poll.rs new file mode 100644 index 000000000..ddc65716a --- /dev/null +++ b/src/poll.rs @@ -0,0 +1,65 @@ +//! Asking GitHub the same question until it has an answer. + +use std::time::{Duration, Instant}; + +use anyhow::{Result, bail}; + +use crate::interrupt; + +/// How long to wait for something GitHub should soon report: CI appearing +/// after a push, or whether a PR is mergeable. `THIRDSHIFT_CI_GRACE_MS` +/// overrides it, for tests. +pub fn grace_period() -> Duration { + millis_from_env("THIRDSHIFT_CI_GRACE_MS").unwrap_or(Duration::from_secs(60)) +} + +/// Ask `probe` every poll interval until it answers `Some`, and return the +/// answer. Fails with `interrupted` as soon as the Run is interrupted. +pub fn until(mut probe: impl FnMut() -> Result>) -> Result { + loop { + if let Some(answer) = probe()? { + return Ok(answer); + } + sleep(poll_interval())?; + } +} + +/// Like [`until`], but give up with `None` once `limit` has passed. +pub fn within( + limit: Duration, + mut probe: impl FnMut() -> Result>, +) -> Result> { + let deadline = Instant::now() + limit; + until(|| match probe()? { + Some(answer) => Ok(Some(Some(answer))), + None if Instant::now() >= deadline => Ok(Some(None)), + None => Ok(None), + }) +} + +/// How long to wait between two asks. `THIRDSHIFT_POLL_MS` overrides it, for +/// tests. +fn poll_interval() -> Duration { + millis_from_env("THIRDSHIFT_POLL_MS").unwrap_or(Duration::from_secs(10)) +} + +fn millis_from_env(name: &str) -> Option { + let millis = std::env::var(name).ok()?.parse().ok()?; + Some(Duration::from_millis(millis)) +} + +/// Sleep for `duration`, failing with `interrupted` as soon as the Run is +/// interrupted. +fn sleep(duration: Duration) -> Result<()> { + let deadline = Instant::now() + duration; + loop { + if interrupt::requested() { + bail!("interrupted"); + } + let left = deadline.saturating_duration_since(Instant::now()); + if left.is_zero() { + return Ok(()); + } + std::thread::sleep(left.min(Duration::from_millis(100))); + } +} diff --git a/src/prompt.rs b/src/prompt.rs index cf10e7993..a34e3d663 100644 --- a/src/prompt.rs +++ b/src/prompt.rs @@ -1,5 +1,6 @@ //! The prompts agent sessions are started with. +use crate::github::Check; use crate::issue::IssueUrl; /// The fresh prompt, for a run that starts a new Issue branch. @@ -61,3 +62,33 @@ pub fn conflict_repair(issue: &IssueUrl, base: &str, branch: &str, pr_url: &str) url = issue.url, ) } + +/// The CI-fix Repair prompt, for red CI on the PR's head commit, listing +/// each check in `failed`. +pub fn ci_fix_repair( + issue: &IssueUrl, + base: &str, + branch: &str, + pr_url: &str, + failed: &[Check], +) -> String { + let checks: String = failed + .iter() + .map(|check| match &check.url { + Some(url) => format!("- {}: {url}\n", check.name), + None => format!("- {}\n", check.name), + }) + .collect(); + format!( + "CI failed on pull request {pr_url} (branch {branch}, implementing {url}).\n\ + \n\ + Failed checks:\n\ + {checks}\ + \n\ + Read the failure logs (e.g. `gh run view --log-failed`), find the root cause, and fix it. Do not skip, disable, or weaken tests or checks to make them pass.\n\ + Run the affected checks locally, commit, and push {branch}.\n\ + \n\ + If a failure is not caused by this branch (it is flaky, or also fails on {base}), do not change code for it. Instead, add it to a \"CI notes\" section of the pull request body with a one-line explanation.\n", + url = issue.url, + ) +} diff --git a/src/run.rs b/src/run.rs index 7ae132487..17b8ac21d 100644 --- a/src/run.rs +++ b/src/run.rs @@ -5,12 +5,14 @@ use std::path::PathBuf; use anyhow::{Context, Result, anyhow, bail}; use crate::branch::{self, Selection}; +use crate::ci::{self, Ci}; use crate::failed_run::{self, FailedRun}; use crate::git::Git; -use crate::github; +use crate::github::{self, Mergeable, PullRequest}; use crate::interrupt; use crate::issue::IssueUrl; use crate::plugin::Plugin; +use crate::poll; use crate::preflight; use crate::progress; use crate::prompt; @@ -76,10 +78,7 @@ fn implement( worktree.push()?; progress::step("checking the PR"); - let pr = github::pull_request_for(issue, branch)?.context("no PR found")?; - if !pr.is_open() { - bail!("PR {} is {}, not open", pr.url, pr.state); - } + let pr = open_pr(issue, branch)?; if pr.base != base { bail!("PR targets {}, not {base}", pr.base); } @@ -87,20 +86,92 @@ fn implement( github::mark_ready(issue, branch)?; } - // Keep the PR mergeable: merge the Base branch, never rebase. - if worktree.merge_base_branch(base)? == Merge::Conflicted { - progress::step(format_args!( - "merging origin/{base} conflicted; starting a Repair" - )); - run_session( - "repair-1", - &prompt::conflict_repair(issue, base, branch, &pr.url), - )?; - worktree.ensure_base_branch_merged(base)?; - } - worktree.push()?; + repair_loop(issue, worktree, base, &pr.url, &mut run_session)?; + ensure_pr_ready_and_mergeable(issue, branch)?; if interrupt::requested() { bail!("interrupted"); } Ok(pr.url) } + +/// The most Repair sessions a Run starts, conflict and CI-fix combined. +const MAX_REPAIRS: usize = 3; + +/// Keep the PR mergeable and its CI green: merge the Base branch (never +/// rebase), push, and watch CI on the head commit, starting a Repair session +/// through `run_session` for a conflict or red CI and then going round again, +/// since the Base branch may have moved meanwhile. Fails once a Repair beyond +/// `MAX_REPAIRS` would be needed. +fn repair_loop( + issue: &IssueUrl, + worktree: &Worktree, + base: &str, + pr_url: &str, + run_session: &mut impl FnMut(&str, &str) -> Result<()>, +) -> Result<()> { + let branch = worktree.branch(); + let mut repairs = 0; + // Counts the Repair about to start, as `repair-`, or fails if it would + // be one too many. + let mut next_repair = |cause: &str| -> Result { + if repairs == MAX_REPAIRS { + bail!("repairs exhausted: {cause}"); + } + repairs += 1; + progress::step(format_args!( + "{cause}; starting Repair {repairs} of {MAX_REPAIRS}" + )); + Ok(format!("repair-{repairs}")) + }; + loop { + if worktree.merge_base_branch(base)? == Merge::Conflicted { + let kind = next_repair("conflict")?; + run_session(&kind, &prompt::conflict_repair(issue, base, branch, pr_url))?; + worktree.ensure_base_branch_merged(base)?; + worktree.push()?; + continue; + } + worktree.push()?; + match ci::watch(issue, &worktree.head()?)? { + Ci::Absent | Ci::Passed => return Ok(()), + Ci::Failed(failed) => { + let kind = next_repair("CI red")?; + run_session( + &kind, + &prompt::ci_fix_repair(issue, base, branch, pr_url, &failed), + )?; + worktree.push()?; + } + } + } +} + +/// The PR whose head is `branch`, failing unless it exists and is open. +fn open_pr(issue: &IssueUrl, branch: &str) -> Result { + let pr = github::pull_request_for(issue, branch)?.context("no PR found")?; + if !pr.is_open() { + bail!("PR {} is {}, not open", pr.url, pr.state); + } + Ok(pr) +} + +/// Fail unless the PR for `branch` is still open, ready for review, and +/// mergeable, waiting up to the grace period for GitHub to work out the last. +fn ensure_pr_ready_and_mergeable(issue: &IssueUrl, branch: &str) -> Result<()> { + progress::step("checking the PR is open, ready and mergeable"); + let pr = open_pr(issue, branch)?; + if pr.is_draft { + bail!("PR {} is a draft", pr.url); + } + let mergeable = poll::within(poll::grace_period(), || { + Ok(Some(github::mergeable(issue, branch)?).filter(|m| *m != Mergeable::Unknown)) + })?; + match mergeable { + Some(Mergeable::Yes) => Ok(()), + Some(Mergeable::No) => bail!("PR {} is not mergeable", pr.url), + _ => bail!( + "GitHub has not worked out whether PR {} is mergeable", + pr.url + ), + } +} diff --git a/src/worktree.rs b/src/worktree.rs index ea222ceba..9ecead591 100644 --- a/src/worktree.rs +++ b/src/worktree.rs @@ -100,6 +100,11 @@ impl Worktree { Ok(()) } + /// The Issue branch's head commit. + pub fn head(&self) -> Result { + self.git.run(&["rev-parse", "HEAD"]) + } + /// Fetch `origin/` and merge it into the Issue branch: a merge, /// never a rebase, so pushing it is always a fast-forward. `--ff` keeps a /// user's `merge.ff = only` from turning a clean merge into an error. diff --git a/tests/ci.rs b/tests/ci.rs new file mode 100644 index 000000000..fb1c7d20e --- /dev/null +++ b/tests/ci.rs @@ -0,0 +1,390 @@ +//! Watching CI: after each push thirdshift waits for checks on the head +//! commit, hands red CI to a CI-fix Repair, and gives up after 3 Repairs. + +mod support; + +use support::Scenario; + +/// The agent adds feature.txt and opens a PR. +const AGENT_OPENS_PR: &str = r#" +echo "feature" > feature.txt +git add feature.txt +git commit -q -m "Add feature" +gh pr create --base main --head issue-7 --title "Add feature" --body "Closes #7" +"#; + +#[test] +fn no_checks_within_the_grace_period_means_no_ci_and_success() { + let scenario = Scenario::new(); + scenario.agent_does(AGENT_OPENS_PR); + + let result = scenario.run(&[&scenario.issue_url(7)]); + + assert_eq!(result.code, Some(0), "stderr: {}", result.stderr); + assert_eq!(result.stdout, "https://github.com/acme/widgets/pull/1\n"); + assert!( + result.stderr.contains("no CI checks appeared"), + "stderr: {}", + result.stderr + ); + scenario.assert_cleaned_up("issue-7"); +} + +/// Bash that sets the check runs on the worktree's HEAD to `checks`, a JSON +/// list, on the fake GitHub. +fn checks_on_head(checks: &str) -> String { + format!("gh fake checks \"$(git rev-parse HEAD)\" '{checks}'\n") +} + +/// Bash that commits a fix, `fix-.txt`, as the CI-fix Repair would. +fn commits_fix(n: usize) -> String { + format!("echo fix > fix-{n}.txt\ngit add fix-{n}.txt\ngit commit -q -m \"Fix CI {n}\"\n") +} + +const GREEN: &str = + r#"[{"name": "test", "conclusion": "success", "url": "https://ci.example/test"}]"#; +const RED: &str = + r#"[{"name": "test", "conclusion": "failure", "url": "https://ci.example/test"}]"#; + +#[test] +fn checks_pending_then_green_is_success() { + let scenario = Scenario::new(); + scenario.agent_does(&format!( + "{AGENT_OPENS_PR}{}", + checks_on_head( + r#"[{"name": "test", "conclusion": "success", "pending_polls": 3}, + {"name": "lint", "conclusion": "skipped"}]"# + ) + )); + + let result = scenario.run(&[&scenario.issue_url(7)]); + + assert_eq!(result.code, Some(0), "stderr: {}", result.stderr); + assert_eq!(result.stdout, "https://github.com/acme/widgets/pull/1\n"); + assert!( + result.stderr.contains("1 of 2 checks still running"), + "stderr: {}", + result.stderr + ); + assert!( + result.stderr.contains("CI passed"), + "stderr: {}", + result.stderr + ); + assert_eq!(scenario.claude_calls().len(), 1); +} + +#[test] +fn red_ci_is_handed_to_a_ci_fix_repair_whose_fix_turns_it_green() { + let scenario = Scenario::new(); + scenario.agent_does(&format!( + "{AGENT_OPENS_PR}{}gh fake statuses \"$(git rev-parse HEAD)\" '{}'\n", + checks_on_head( + r#"[{"name": "test", "conclusion": "failure", "url": "https://ci.example/test"}, + {"name": "build", "conclusion": "success", "url": "https://ci.example/build"}]"# + ), + r#"[{"context": "deploy/preview", "state": "error", "url": "https://deploy.example/7"}]"#, + )); + scenario.agent_does_in_session(2, &format!("{}{}", commits_fix(1), checks_on_head(GREEN))); + + let result = scenario.run(&[&scenario.issue_url(7)]); + + assert_eq!(result.code, Some(0), "stderr: {}", result.stderr); + assert_eq!(result.stdout, "https://github.com/acme/widgets/pull/1\n"); + let calls = scenario.claude_calls(); + assert_eq!(calls.len(), 2); + assert_eq!( + calls[1]["prompt"], + "CI failed on pull request https://github.com/acme/widgets/pull/1 (branch issue-7, implementing https://github.com/acme/widgets/issues/7).\n\ + \n\ + Failed checks:\n\ + - test: https://ci.example/test\n\ + - deploy/preview: https://deploy.example/7\n\ + \n\ + Read the failure logs (e.g. `gh run view --log-failed`), find the root cause, and fix it. Do not skip, disable, or weaken tests or checks to make them pass.\n\ + Run the affected checks locally, commit, and push issue-7.\n\ + \n\ + If a failure is not caused by this branch (it is flaky, or also fails on main), do not change code for it. Instead, add it to a \"CI notes\" section of the pull request body with a one-line explanation.\n" + ); + assert_eq!(calls[1]["cwd"], calls[0]["cwd"]); + // thirdshift pushes the fix even though the Repair didn't. + assert_eq!( + scenario.origin_log("issue-7"), + Some(vec![ + "Fix CI 1".to_string(), + "Add feature".to_string(), + "Initial commit".to_string(), + ]) + ); + let logs = scenario.entries("home/.thirdshift/logs"); + assert!( + logs.iter().any(|name| name.ends_with("-repair-1.jsonl")), + "logs: {logs:?}" + ); + scenario.assert_cleaned_up("issue-7"); +} + +#[test] +fn red_ci_after_three_repairs_is_a_failed_run() { + let scenario = Scenario::new(); + scenario.agent_does(&format!("{AGENT_OPENS_PR}{}", checks_on_head(RED))); + for session in 2..=4 { + scenario.agent_does_in_session( + session, + &format!("{}{}", commits_fix(session - 1), checks_on_head(RED)), + ); + } + + let result = scenario.run(&[&scenario.issue_url(7)]); + + assert_ne!(result.code, Some(0)); + assert_eq!(result.stdout, "https://github.com/acme/widgets/pull/1\n"); + assert!( + result.stderr.contains("repairs exhausted: CI red"), + "stderr: {}", + result.stderr + ); + assert_eq!( + scenario.claude_calls().len(), + 4, + "the implement session and 3 Repairs" + ); + assert_eq!(scenario.gh_state()["prs"][0]["isDraft"], true); + assert_eq!( + scenario.origin_log("issue-7").unwrap()[..2], + [ + "thirdshift: failed run (repairs exhausted: CI red)".to_string(), + "Fix CI 3".to_string(), + ] + ); + scenario.assert_cleaned_up("issue-7"); +} + +/// Bash that pushes `file` with `content` to main from another clone, as if +/// someone else's work landed while the agent was busy. +fn base_moves_on(file: &str, content: &str) -> String { + format!( + r#" +other="$(mktemp -d)" +git clone -q https://github.com/acme/widgets.git "$other" +echo "{content}" > "$other/{file}" +git -C "$other" add {file} +git -C "$other" commit -q -m "Base moves on: {file}" +git -C "$other" push -q origin main +rm -rf "$other" +"# + ) +} + +/// The conflict Repair keeps both sides of `file` and finishes the merge. +fn resolves_conflict(file: &str) -> String { + format!("echo resolved > {file}\ngit add {file}\ngit commit -q --no-edit\n") +} + +#[test] +fn conflict_and_ci_fix_repairs_share_the_cap_of_three() { + let scenario = Scenario::new(); + // Repair 1 resolves a conflict and leaves CI red; Repair 2 fixes CI but it + // stays red; Repair 3 fixes CI again, but main moves on to a conflict, + // which would need a 4th Repair. + scenario.agent_does(&format!( + "{AGENT_OPENS_PR}{}", + base_moves_on("feature.txt", "base feature") + )); + scenario.agent_does_in_session( + 2, + &format!( + "{}{}", + resolves_conflict("feature.txt"), + checks_on_head(RED) + ), + ); + scenario.agent_does_in_session(3, &format!("{}{}", commits_fix(1), checks_on_head(RED))); + scenario.agent_does_in_session( + 4, + &format!( + "{}{}{}", + commits_fix(2), + checks_on_head(GREEN), + base_moves_on("fix-2.txt", "base fix") + ), + ); + + let result = scenario.run(&[&scenario.issue_url(7)]); + + assert_ne!(result.code, Some(0)); + assert!( + result.stderr.contains("repairs exhausted: conflict"), + "stderr: {}", + result.stderr + ); + let prompts: Vec = scenario + .claude_calls() + .iter() + .map(|call| { + call["prompt"] + .as_str() + .unwrap() + .lines() + .next() + .unwrap() + .to_string() + }) + .collect(); + assert_eq!(prompts.len(), 4, "prompts: {prompts:?}"); + assert_eq!(prompts[1], "/thirdshift:resolving-merge-conflicts"); + assert!(prompts[2].starts_with("CI failed on pull request")); + assert!(prompts[3].starts_with("CI failed on pull request")); + assert_eq!(scenario.gh_state()["prs"][0]["isDraft"], true); + scenario.assert_cleaned_up("issue-7"); +} + +#[test] +fn after_a_repair_the_base_branch_is_merged_again_before_ci_is_watched() { + let scenario = Scenario::new(); + scenario.agent_does(&format!("{AGENT_OPENS_PR}{}", checks_on_head(RED))); + // The fix is green on its own, but main moves on meanwhile, so CI is + // watched on the merge commit instead, which has no checks. + scenario.agent_does_in_session( + 2, + &format!( + "{}{}{}", + commits_fix(1), + checks_on_head(GREEN), + base_moves_on("other.txt", "other") + ), + ); + + let result = scenario.run(&[&scenario.issue_url(7)]); + + assert_eq!(result.code, Some(0), "stderr: {}", result.stderr); + assert_eq!(scenario.claude_calls().len(), 2); + assert_eq!( + scenario.origin_git(&["log", "--first-parent", "--format=%s", "issue-7"]), + "Merge remote-tracking branch 'origin/main' into issue-7\n\ + Fix CI 1\n\ + Add feature\n\ + Initial commit\n" + ); + let merge = scenario.origin_git(&["rev-parse", "issue-7"]); + let last_checks_path = scenario + .gh_calls() + .into_iter() + .filter(|call| call[0] == "api") + .filter_map(|call| call.last().cloned()) + .rfind(|path| path.contains("/check-runs")) + .unwrap(); + assert!( + last_checks_path.contains(merge.trim()), + "last watched: {last_checks_path}, merge: {merge}" + ); + assert!( + result.stderr.contains("no CI checks appeared"), + "stderr: {}", + result.stderr + ); +} + +#[test] +fn a_pr_closed_by_the_end_is_a_failed_run() { + let scenario = Scenario::new(); + scenario.agent_does(&format!("{AGENT_OPENS_PR}{}", checks_on_head(RED))); + scenario.agent_does_in_session( + 2, + &format!( + "{}{}gh fake pr issue-7 state '\"CLOSED\"'\n", + commits_fix(1), + checks_on_head(GREEN) + ), + ); + + let result = scenario.run(&[&scenario.issue_url(7)]); + + assert_ne!(result.code, Some(0)); + assert!( + result + .stderr + .contains("PR https://github.com/acme/widgets/pull/1 is closed, not open"), + "stderr: {}", + result.stderr + ); + assert_eq!(result.stdout, "", "a closed PR is not printed"); + scenario.assert_cleaned_up("issue-7"); +} + +#[test] +fn an_unmergeable_pr_at_the_end_is_a_failed_run() { + let scenario = Scenario::new(); + scenario.agent_does(&format!( + "{AGENT_OPENS_PR}gh fake pr issue-7 mergeable '\"CONFLICTING\"'\n" + )); + + let result = scenario.run(&[&scenario.issue_url(7)]); + + assert_ne!(result.code, Some(0)); + assert!( + result + .stderr + .contains("PR https://github.com/acme/widgets/pull/1 is not mergeable"), + "stderr: {}", + result.stderr + ); + assert_eq!(result.stdout, "https://github.com/acme/widgets/pull/1\n"); + assert_eq!(scenario.gh_state()["prs"][0]["isDraft"], true); +} + +#[test] +fn a_pr_sent_back_to_draft_by_the_end_is_a_failed_run() { + let scenario = Scenario::new(); + scenario.agent_does(&format!("{AGENT_OPENS_PR}{}", checks_on_head(RED))); + scenario.agent_does_in_session( + 2, + &format!( + "{}{}gh pr ready issue-7 --undo\n", + commits_fix(1), + checks_on_head(GREEN) + ), + ); + + let result = scenario.run(&[&scenario.issue_url(7)]); + + assert_ne!(result.code, Some(0)); + assert!( + result.stderr.contains("is a draft"), + "stderr: {}", + result.stderr + ); +} + +#[test] +fn waits_for_github_to_work_out_whether_the_pr_is_mergeable() { + let scenario = Scenario::new(); + scenario.agent_does(&format!( + "{AGENT_OPENS_PR}gh fake pr issue-7 unknown_polls 3\n" + )); + + let result = scenario.run(&[&scenario.issue_url(7)]); + + assert_eq!(result.code, Some(0), "stderr: {}", result.stderr); + assert_eq!(result.stdout, "https://github.com/acme/widgets/pull/1\n"); +} + +#[test] +fn a_pr_whose_mergeability_stays_unknown_is_a_failed_run() { + let scenario = Scenario::new(); + scenario.agent_does(&format!( + "{AGENT_OPENS_PR}gh fake pr issue-7 mergeable '\"UNKNOWN\"'\n" + )); + + let result = scenario.run(&[&scenario.issue_url(7)]); + + assert_ne!(result.code, Some(0)); + assert!( + result.stderr.contains( + "GitHub has not worked out whether PR https://github.com/acme/widgets/pull/1 is mergeable" + ), + "stderr: {}", + result.stderr + ); + assert_eq!(scenario.gh_state()["prs"][0]["isDraft"], true); +} diff --git a/tests/continuation.rs b/tests/continuation.rs index a82dd5529..9c1e14f90 100644 --- a/tests/continuation.rs +++ b/tests/continuation.rs @@ -287,8 +287,9 @@ fn selection_asks_github_once_for_the_pr_history() { .gh_calls() .iter() .map(|argv| argv[..2].join(" ")) + .take_while(|subcommand| subcommand != "pr view") .collect(); - assert_eq!(subcommands, vec!["issue view", "pr list", "pr view"]); + assert_eq!(subcommands, vec!["issue view", "pr list"]); assert!( scenario.gh_calls()[1] .windows(2) diff --git a/tests/fakes/gh b/tests/fakes/gh index eb9d02279..e810fc367 100644 --- a/tests/fakes/gh +++ b/tests/fakes/gh @@ -5,7 +5,21 @@ State lives in the JSON file named by $FAKE_GH_STATE: {"repo": "owner/repo", "issues": {"": "OPEN" | "CLOSED"}, - "prs": [{"number", "url", "head", "base", "state", "isDraft", "title", "body"}]} + "prs": [{"number", "url", "head", "base", "state", "isDraft", "title", "body", + "mergeable"?, "unknown_polls"?}], + "checks": {"": [{"name", "conclusion", "url", "pending_polls"?}]}, + "statuses": {"": [{"context", "state", "url"}]}} + +A PR's mergeable state reports as UNKNOWN for its first `unknown_polls` reads. +A check run reports as in progress for its first `pending_polls` reads, then as +completed with its `conclusion`. + +Tests and fake agents change that state with `gh fake …` (not a real gh +command): + + gh fake checks '' set the check runs on + gh fake statuses '' set the commit statuses on + gh fake pr '' set a field of 's newest PR Every call's argv is appended to the JSON list in $FAKE_GH_RECORD. @@ -15,6 +29,7 @@ else exits 2 so an unexpected call fails loudly. import json import os +import re import sys @@ -28,7 +43,7 @@ def save(state): json.dump(state, f, indent=2) -BOOLEAN_FLAGS = {"draft", "undo"} +BOOLEAN_FLAGS = {"draft", "undo", "paginate"} def parse(args): @@ -88,7 +103,12 @@ def pr_view(state, positional, flags): die(f'no pull requests found for branch "{head}"') pr = matches[-1] wanted = flags["json"].split(",") - print(json.dumps({key: pr_fields(pr)[key] for key in wanted})) + fields = pr_fields(pr) + if "mergeable" in wanted and pr.get("unknown_polls", 0) > 0: + pr["unknown_polls"] -= 1 + save(state) + fields["mergeable"] = "UNKNOWN" + print(json.dumps({key: fields[key] for key in wanted})) def record(args): @@ -111,6 +131,7 @@ def pr_fields(pr): "isDraft": pr["isDraft"], "number": pr["number"], "isCrossRepository": pr.get("isCrossRepository", False), + "mergeable": pr.get("mergeable", "MERGEABLE"), } @@ -153,6 +174,55 @@ def issue_view(state, positional, flags): print(json.dumps({key: fields[key] for key in wanted})) +def api(state, positional, flags): + """The two commit endpoints thirdshift reads CI from, one page, each item + of the list `--jq '.[]'` names on its own line.""" + match = re.fullmatch( + r"repos/([^/]+/[^/]+)/commits/([0-9a-f]+)/(check-runs|status)(\?.*)?", + positional[0], + ) + if not match: + die(f"fake gh: unsupported api path {positional[0]}", 2) + repo, sha, endpoint, _ = match.groups() + check_repo(state, {"repo": repo}) + if endpoint == "check-runs": + runs = [] + for check in state.get("checks", {}).get(sha, []): + pending = check.get("pending_polls", 0) > 0 + if pending: + check["pending_polls"] -= 1 + runs.append( + { + "name": check["name"], + "status": "in_progress" if pending else "completed", + "conclusion": None if pending else check["conclusion"], + "details_url": check.get("url"), + } + ) + save(state) + items, field = runs, "check_runs" + else: + items = [ + {"context": s["context"], "state": s["state"], "target_url": s.get("url")} + for s in state.get("statuses", {}).get(sha, []) + ] + field = "statuses" + if flags.get("jq") != f".{field}[]": + die(f"fake gh: unsupported --jq {flags.get('jq')}", 2) + for item in items: + print(json.dumps(item)) + + +def fake(state, args): + if args[0] in ("checks", "statuses"): + state.setdefault(args[0], {})[args[1]] = json.loads(args[2]) + elif args[0] == "pr": + [pr for pr in state["prs"] if pr["head"] == args[1]][-1][args[2]] = json.loads(args[3]) + else: + die(f"fake gh: unsupported fake command {args}", 2) + save(state) + + def main(): args = sys.argv[1:] record(args) @@ -177,6 +247,11 @@ def main(): positional, flags = parse(args[2:]) check_repo(state, flags) issue_view(state, positional, flags) + elif args[0] == "api": + positional, flags = parse(args[1:]) + api(state, positional, flags) + elif args[0] == "fake": + fake(state, args[1:]) else: die(f"fake gh: unsupported command: {args}", 2) diff --git a/tests/support/mod.rs b/tests/support/mod.rs index 68713b292..684c1f6b8 100644 --- a/tests/support/mod.rs +++ b/tests/support/mod.rs @@ -172,7 +172,10 @@ impl Scenario { .env("FAKE_GH_STATE", self.path("gh-state.json")) .env("FAKE_CLAUDE_SCRIPT", self.path("claude-script.sh")) .env("FAKE_CLAUDE_RECORD", self.path("claude-calls.json")) - .env("FAKE_GH_RECORD", self.path("gh-calls.json")); + .env("FAKE_GH_RECORD", self.path("gh-calls.json")) + // Seconds of waiting for CI become milliseconds. + .env("THIRDSHIFT_CI_GRACE_MS", "300") + .env("THIRDSHIFT_POLL_MS", "10"); command }